fix(review): guard /add-docs cross-batch merge with isPostable

[devops-thiago]

What type of PR is this?

• 🐛 Bug fix
• ✨ Feature
• 📝 Documentation
• 🔧 Refactor
• 🚀 Performance
• ✅ Test
• 🔒 Security
• 📦 Dependency update
• 🏗️ CI/CD

Description

The cross-batch merge loop in DocGenerationService.generateEachBatch built its dedupe key as doc.file().strip() + ":" + doc.line() before any postability check. DocGenerationResponse.DocSuggestion.file is nullable — isPostable() exists precisely because file, line, suggestion_old and suggestion_new may be null/absent. A model reply that omits "file" on one entry therefore threw NullPointerException at the .strip().

That call site is in handle(), outside planOrReportFailure's try, so the throw landed in handle()'s outer catch (RuntimeException) which only logs. The maintainer who ran /add-docs got no comment at all (not even GENERATION_FAILED), and every valid entry in the same reply was silently lost. The sibling /improve guards the identical line with improvement.isPostable() && seen.add(dedupeKey(improvement)); /add-docs lost that guard when it was migrated onto the shared generator seam in #469.

The fix mirrors PrImprovementService — gate the dedupe key with doc.isPostable():

if (doc.isPostable() && seen.add(doc.file().strip() + ":" + doc.line())) {
  merged.add(doc);
}

This fixes three defects at once:

1. The NPE — a doc entry with null file no longer throws.
2. Dedupe-key consumption — a non-postable entry no longer consumes file:line, so it can no longer block a later batch's valid suggestion for the same declaration line.
3. Misleading summary — non-postable entries no longer reach merged, so the summary no longer reports COULD_NOT_PLACE ("This usually happens after a force-push") for a reply that was simply malformed; it correctly reports NOTHING_TO_DOCUMENT.

Change is confined to DocGenerationService.java and its test.

Related Issues

Refs audit a3-P1 (validated HIGH regression introduced by #469). N/A for a tracked issue number.

How Has This Been Tested?

Red/green, per finding, on the current base before and after the one-line fix.

Test 1 — postsTheValidDocWhenAnEarlierEntryInTheSameBatchOmitsItsFile

A batch reply with two doc entries: the first omits "file" (parsed as null), the second is valid. Asserts the valid suggestion is still posted and a real comment is produced.

Red (before fix) — the NPE is thrown in the merge loop and swallowed by handle()'s outer catch, so nothing is posted. Captured verbatim from the run:

WARN  [...DocGenerationService] Failed to handle /add-docs on owner/repo #7: java.lang.NullPointerException: Cannot invoke "String.strip()" because the return value of "dev.thiagogonzaga.thrillhousebot.review.ai.DocGenerationResponse$DocSuggestion.file()" is null

and the test then fails on the missing post:

Wanted but not invoked:
reviewClient.createPullRequestComment(...);
Actually, there were zero interactions with this mock.

Green (after fix) — the null-file entry is dropped, the valid one is posted (/add-docs posted 1 suggestion(s) and 0 note(s)). Passes.

Test 2 — aNonPostableEntryDoesNotConsumeTheDedupeKeyForALaterBatchsValidOne

Batch 1 returns a non-postable doc for src/Foo.java:1 (blank suggestion_new); batch 2 returns a postable doc for the same src/Foo.java:1. Asserts the postable one is still posted and the summary is not COULD_NOT_PLACE.

Red (before fix) — the non-postable entry consumed the file:line key and suppressed the later valid suggestion; nothing was posted:

Wanted but not invoked:
reviewClient.createPullRequestComment(...);
Actually, there were zero interactions with this mock.

Green (after fix) — isPostable() gates the key, the non-postable entry is skipped, the valid suggestion posts. Passes.

Existing test that pinned the defect

Two cases of the parameterized doesNotPostSuggestionThatCannotAnchorCleanly (suggestion_new is blank, suggestion_old is omitted) previously asserted COULD_NOT_PLACE — the exact misleading verdict this fix removes. They now assert NOTHING_TO_DOCUMENT, the correct verdict for a reply carrying no usable documentation. The postable-but-unanchorable cases still assert COULD_NOT_PLACE.

Gates

• ./mvnw -B spotless:apply — BUILD SUCCESS

• ./mvnw -B clean compile spotbugs:check spotless:check — BugInstance size is 0, BUILD SUCCESS

• ./mvnw -B clean test — Tests run: 2344, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS

• Unit tests

• Integration tests

• Manual testing

Checklist

• My code follows the project's coding standards
• I have performed a self-review of my own code
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly
• My changes generate no new warnings or errors

Additional Notes

Scope is limited to the /add-docs generator and its test; no unrelated files were touched or reformatted. One existing test was changed (not disabled) to assert the corrected summary verdict — explained above.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[thrillhousebot]

🤖 ThrillhouseBot PR Summary

What this PR does

Adds a postability check before constructing a dedupe key in the cross-batch merge of DocGenerationService, fixing a NullPointerException when a doc entry omits the file field, and preventing non-postable entries from blocking later valid suggestions for the same declaration line.

Control-Flow Diagram

🔀 Show diagram

flowchart TD
  A["For each doc in response.docs()"] --> B{"doc.isPostable()?"}
  B -- "false" --> C["Skip (no key consumed, not merged)"]
  B -- "true" --> D{"seen.add(file.strip() + ':' + line)?"}
  D -- "true" --> E["Add to merged list"]
  D -- "false" --> F["Duplicate, skip"]

Loading

Changes Overview

• Files changed: 2
• Lines added: +77
• Lines removed: -9

Changed Files

File	Change	Summary
src/main/java/dev/thiagogonzaga/thrillhousebot/review/DocGenerationService.java	Modified	Guard dedupe-key construction with isPostable() to avoid NPE on null file and to exclude non-postable entries from merged list.
src/test/java/dev/thiagogonzaga/thrillhousebot/review/DocGenerationServiceTest.java	Modified	Update parameterized test to assert correct summary (NOTHING_TO_DOCUMENT) for non-postable entries; add two tests for null file handling and dedupe key consumption across batches.

Risk Assessment

Risk	Count
🔴 Critical	0
🟠 High	0
🟡 Medium	0
🔵 Low	0

No new issues found in this PR, but the review cannot be approved until CI is confirmed green.

⚠️ CI Checks Status

Some checks are still pending or have failed:

Check	Type	Status	Detail
test	check-run	⏳ Pending	-
changes	check-run	⏳ Pending	-
format	check-run	⏳ Pending	-
trivy	check-run	⏳ Pending	-
actionlint	check-run	⏳ Pending	-
frontend	check-run	⏳ Pending	-
dependency-review	check-run	⏳ Pending	-

---

Automated review by ThrillhouseBot. Reply with /review to re-run.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud