fix(release): honor severity filter in Trivy SARIF scan gate by devops-thiago · Pull Request #8 · devops-thiago/ThrillhouseBot

devops-thiago · 2026-06-13T02:17:33Z

What type of PR is this?

Description

Fixes the release workflow Trivy scan gate after #7 merged.

With format: sarif, trivy-action defaults to scanning all severities and unsets the configured severity: filter unless limit-severities-for-sarif: true is set (trivy-action README, issues #309 / #386). The release job then fails on MEDIUM/LOW findings even when severity: CRITICAL,HIGH and .trivyignore are configured.

Add limit-severities-for-sarif: true to both release scan steps so the gate, SARIF output, and .trivyignore all respect CRITICAL,HIGH.

Related Issues

N/A

How Has This Been Tested?

Unit tests
Integration tests
Manual testing

Reproduced locally with Trivy 0.70.0:

Config	Exit code
SARIF + ignore (current release workflow)	1
SARIF + ignore + severity kept (`limit-severities-for-sarif`)	0

Failed release run after #7: 27453355855 (scan job).

After merge:

gh workflow run release.yml -f version=v0.1.0

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

Screenshots / Logs

Failure	Cause	Fix in this PR
`scan` job exit 1	SARIF mode ignores `severity:` by default; gate fails on all severities	`limit-severities-for-sarif: true` on both Trivy steps

Additional Notes

SARIF uploaded to the Security tab will contain CRITICAL/HIGH only (not full LOW/MEDIUM inventory). That matches the release gate intent.
trivy-action remains pinned by SHA to v0.36.0 with Trivy v0.70.0 (post–March 2026 supply-chain incident safe versions).
No application or image changes; re-run release for existing v0.1.0 tag after merge.

trivy-action ignores severity when format is sarif unless limit-severities-for-sarif is set, so the release scan failed on MEDIUM/LOW findings despite CRITICAL,HIGH and .trivyignore.

github-actions · 2026-06-13T02:17:44Z

Dependency Review

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

Scanned Files

None

codecov · 2026-06-13T02:20:23Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.21%. Comparing base (7e76b8b) to head (c79ef30).

Additional details and impacted files

@@            Coverage Diff            @@
##               main       #8   +/-   ##
=========================================
  Coverage     98.21%   98.21%           
  Complexity     1027     1027           
=========================================
  Files            48       48           
  Lines          3023     3023           
  Branches        383      383           
=========================================
  Hits           2969     2969           
  Misses           14       14           
  Partials         40       40

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sonarqubecloud · 2026-06-13T02:20:45Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

## What type of PR is this? - [x] 🏗️ CI/CD - [x] 🔒 Security ## Description With `format: sarif`, `aquasecurity/trivy-action` scans **all** severities by default, so the configured `severity: CRITICAL,HIGH` filter (and `.trivyignore`) is ignored for both the SARIF report and the exit code unless `limit-severities-for-sarif: true` is set. This is the same fix that landed for the release gate in #8 (`release.yml`). This adds the flag to the remaining SARIF image scans so the GitHub Security tab reflects the configured gate instead of noisy LOW/MEDIUM findings: - **`ci.yml` → `trivy-image`**: default (UBI9) and distroless image scans - **`security-scan.yml` → `trivy-latest`**: scheduled `:latest` and `:latest-distroless` scans Severity filters are unchanged (`CRITICAL,HIGH`), per the issue's intent. Both jobs keep `exit-code: "0"`, so they stay report-only — this change only affects which findings are reported, not pass/fail. ```diff severity: CRITICAL,HIGH exit-code: "0" format: sarif + limit-severities-for-sarif: true output: trivy-image.sarif ``` (applied to all four SARIF trivy steps across the two jobs) ## Related Issues Fixes #12 ## How Has This Been Tested? - [ ] Unit tests - [ ] Integration tests - [x] Manual testing - Both workflow files parse as valid YAML. - Change matches the convention established in `release.yml` (#8): flag placed directly after `format: sarif`. - Note: the edited `trivy-image` / `trivy-latest` jobs are gated on `push` to `main` / `schedule`, so they do not execute on PR CI; this is a config-only change that first runs on the post-merge `main` push. ## Checklist - [x] My code follows the project's coding standards - [x] 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 - [x] My changes generate no new warnings or errors ## Screenshots / Logs N/A ## Additional Notes Out of scope for this issue: the `trivy` **fs** job in `ci.yml` also uses `format: sarif` but with `exit-code: "1"` and no flag, so it currently gates on *all* severities rather than its declared `severity: CRITICAL`. That's a separate (gating, not report-only) concern, tracked in #76.

… review path Remove per-step / code-restating narration and issue-referencing rationale blocks added during recent fixes (mostly in review() and buildResult), collapse the GitHubReviewClient pagination constant comments to one line each, and strip the (#NNN) ticket/PR references from review-path comments (main and tests). PKCS#1/#8 in RsaPrivateKeyParser are crypto standards, not tickets, and are kept. Kept: javadoc, constant-group labels, and genuinely non-obvious WHY comments (backstop, CI-null semantics, first-review vs hasContext, fetch-propagation, CI classification, the severity-merge rule, etc.). Comment-only; no logic change. Full suite: 1238 passing; spotbugs + spotless clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

## What type of PR is this? - [x] 🚀 Performance - [x] 🔧 Refactor ## Description Efficiency finding from the `release/v0.3.0` final review. **#8 — Ignore-glob filter walked twice per review.** `ReviewContextLoader.load()` computed `reviewableFiles(files)` and then called `patchesByFile(files)`, which recomputed `reviewableFiles(files)` internally. So the ignore-glob filter (a `PathMatcher` per file × configured globs) walked the changed-file list twice on every review. The already-filtered list now feeds the `DiffLineResolver` directly via a new `patchesByReviewableFiles` overload — one pass instead of two. Pure CPU/GC saving on the hot path, scaling with PR size; no behavior change. ## Related Issues N/A — surfaced by the v0.3.0 final code-review. Relates to #250 (decomposition). ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing - `ReviewDiffFormatterTest.patchesByReviewableFilesTrustsTheCallersFilterAndDropsBlankPatches`: the new overload doesn't re-filter and still drops blank patches. - Full suite green: 1372 tests, SpotBugs 0, Spotless clean. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly (CHANGELOG) - [x] My changes generate no new warnings or errors ## Additional Notes **PR 5 of 5** in the `release/v0.3.0` final-review cleanup stack — stacked on #271; merge bottom-up.

…usEvaluator cleanup + glob-filter perf (#270) ## What type of PR is this? - [x] 🐛 Bug fix - [x] 🔧 Refactor - [x] 🚀 Performance ## Description Final batch of the `release/v0.3.0` review-path cleanup (opus ultra final review). PRs #271 and #272 were merged down into this branch, so merging this PR brings the remaining three changes into release together. **#6 — Unreadable CI as a first-class signal (was a synthetic check).** #253 held APPROVE on an unread CI source by appending a synthetic `CiCheck("CI status unavailable", …)` to the offending list, which rendered as a counterfeit required check (a "Required CI Checks Status" row and a "Check **CI status unavailable** is pending" line). `evaluateCiChecks` now returns `CiEvaluation(offendingChecks, unreadable)`; `VerdictBuilder` holds `APPROVE → COMMENT` on the flag, and the summary/no-issues body disclose it as its own "CI Status Unavailable" note. `ReviewResult` gains a `ciUnreadable` component (default `false` via existing convenience ctors). **#5 — The unreadable hold now applies in both gate modes.** It previously fired only in gate-all mode; an unread source can hide a required check's true state, so `addMissingRequiredChecks` alone didn't close the gap. **#9 — Bot-check detection uses the shared `BotIdentity`.** `CiStatusEvaluator` derived its match tokens from a hardcoded `"thrillhousebot"` literal; it now derives them from the configured `BotIdentity` logins (incl. the alternate slug), like every other collaborator. **#11 — Folded `collectReadable`/`collectPaged`.** One method that pages and returns `false` on a null page/exception — dropped the one-element `boolean[]` escape hatch and the second method. **#12 — Deduped offending-check gating.** `addOffendingCheckRun`/`addOffendingStatus` shared logic now lives once in `addOffending`. **#8 — Ignore-glob filter walked once per review.** `load()` computed `reviewableFiles` and then `patchesByFile` recomputed it; the already-filtered list now feeds the line resolver via a new `patchesByReviewableFiles` overload — one glob pass instead of two on the hot path. > Fail-closed tradeoff (#5/#6): if a CI source flakes we hold to COMMENT (findings still post; a re-run approves) rather than approve over CI we couldn't read. ## Related Issues N/A — surfaced by the v0.3.0 final code-review. Refines #253; relates to #217 and #250. Consolidates the merged #271 and #272. ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing - `CiStatusEvaluatorTest`: `unreadable()` assertions, `unreadableCiInGateSpecificModeAlsoHoldsApproval` (#5), `usesConfiguredBotIdentityToIgnoreItsOwnChecks` (#9). - `ReviewOrchestratorTest.unreadableCiHoldsApprovalAndIsDisclosedAsItsOwnSignal` (#6). - `PrSummaryGeneratorTest.shouldRenderCiUnavailableNoteWhenCiUnreadableInsteadOfCelebrating` (#6). - `ReviewDiffFormatterTest.patchesByReviewableFilesTrustsTheCallersFilterAndDropsBlankPatches` (#8). - Full suite green: **1373 tests**, SpotBugs 0, Spotless clean (on the current `release/v0.3.0`). ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly (CHANGELOG) - [x] My changes generate no new warnings or errors ## Additional Notes Final PR of the `release/v0.3.0` final-review cleanup (#268 and #269 already merged). #271 and #272 were merged into this branch, so this single PR carries the remaining work (#5, #6, #8, #9, #11, #12). Rebased clean onto the current `release/v0.3.0`.

…e unreadable CI alongside offending checks (#274) ## What type of PR is this? - [x] 🐛 Bug fix - [x] ✅ Test ## Description A multi-agent audit of the integrated v0.3.0 cleanup surfaced two real issues in code shipped earlier this release, plus test gaps. **PR 1 of 4** in a stack closing out the remaining final-review items; merge bottom-up. **Completed-broadcast divergence (regression from #254 follow-up).** The post-result resilience refactor split *"mark the session completed"* and *"broadcast completion"* into independent best-effort steps. `applyReviewResult` flips the in-memory session to completed and *then* persists (`applySessionState`: `mutator.accept(session)` → `sessionPersistence.update`). So when the persistence write threw, `runPostResultStep` swallowed it but the next step still broadcast a `completed` event — the live dashboard showed the session completed while its DB row stayed `IN_PROGRESS`, reverting on reload (the exact "stuck" symptom the refactor claimed to prevent). Persist and broadcast are now **one step**: a failed write skips the broadcast, so persisted and live state stay consistent. **Unreadable-CI note dropped when a check is also failing (from #253 first-class work).** `ReviewPublisher.noIssuesBody` chained the offending-checks and unreadable-CI notes with `else if`, so a review held by *both* at once only mentioned the failing check — disagreeing with the summary table, which lists them with independent `if`s. The body now discloses both independently. ## Related Issues N/A — surfaced by the v0.3.0 final-review audit. Refines #253 and #254. ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing - `postResultPersistenceFailureDoesNotBroadcastAFalseCompletion` (renamed/corrected): a failed completion persist no longer broadcasts `completed` (only `started` fires); review kept, session never `FAILED`. - `checkRunConclusionFailure…` and the `labeler boom` (apply-labels) tests now assert the later completion step still runs and broadcasts (started + completed) — covering the early-step isolation the audit found untested. - `shouldDiscloseBothOffendingChecksAndUnreadableCiWhenHeldByEach`: body shows both notes. - Full suite green: 1377 tests, SpotBugs 0, Spotless clean. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly (CHANGELOG) - [x] My changes generate no new warnings or errors ## Additional Notes **PR 1 of 4** — stack: this → CI-status I/O (#7/#10/#15) → diff/doc (#8/#14) → dead code (#13). Merge bottom-up.

…changelog cleanup (#276) ## What type of PR is this? - [x] 🚀 Performance - [x] 🔧 Refactor - [x] 📝 Documentation ## Description Diff/doc cleanup from the v0.3.0 final-review remaining items, plus a CHANGELOG correction. **PR 3 of 4** (the CI-status PR #275 is merged). Based on release/v0.3.0. **F8 — one ignore-glob pass on the diff render.** `formatFileSection` re-ran `isIgnored()` for every file inside `buildDiffStringWithStats`, a second full glob walk on the hot review path (the context loader already computed the reviewable list for the line resolver). The loader now computes `reviewableFiles` once and passes it to `buildDiffStringWithStats`, which threads the reviewable-name set into `formatFileSection` (set membership, not a fresh glob match). The base-comparison render derives its set the same way. **G1 — /add-docs uses the no-re-walk overload.** `DocGenerationService.postSuggestions` called `patchesByFile(reviewable)` on a list it had *already* filtered, re-running the glob; it now uses `patchesByReviewableFiles`. **F14 — share the soft loaders.** The best-effort fetch-and-degrade loaders (PR details, changed files, project stack, repository instructions) were copy-pasted across `AbstractPrSuggestionGenerator`, `DocGenerationService`, and `ReviewContextLoader`. They now live once in a new `SoftLoaders` helper; all three delegate to it (and the duplicated `resolveProjectStack` wrapper is gone from both call sites). **CHANGELOG correction (separate commit).** Per the net-vs-last-release policy, `[0.3.0]` should list only the delta vs v0.2.1. Bugs that were both introduced *and* fixed within the unreleased v0.3.0 cycle net out, so their entries are removed: the completed-broadcast divergence, the unreadable-CI disclosure gap, the diagram render kill-switch, the CI bot-identity match, and the decomposition's double glob walk. Kept: features, improvements, and fixes for bugs that shipped in v0.2.1 or earlier (verified against the `v0.2.1` tag — check-run-before-review, stuck session, truncated-PR `✅`, unreadable-CI approving all reproduce there). ## Related Issues N/A — v0.3.0 final-review remaining items (#8, #14) + audit gap (G1). Relates to #250. ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing - Existing `ReviewDiffFormatterTest`, `ReviewContextLoaderTest`, `DocGenerationServiceTest`, and the `/describe`·`/changelog` generator tests exercise the reworked paths; `SoftLoaders` is fully covered through them. - Full suite green: 1374 tests, SpotBugs 0, Spotless clean, patch fully covered (no misses, no partials on changed lines). ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly (CHANGELOG) - [x] My changes generate no new warnings or errors ## Additional Notes **PR 3 of 4** — the CI-status I/O PR (#275) is merged; this is based on release/v0.3.0. The CHANGELOG cleanup spans entries originally added across the earlier final-review PRs; it lands here because this is the stack tip.

…throughs (F13) (#277) ## What type of PR is this? - [x] 🔧 Refactor ## Description Final v0.3.0 final-review item (F13): remove test-only convenience constructors / overloads / pass-throughs that no production code reaches, so the API surface is smaller and tests exercise only production-reachable shapes. **PR 4 of 4** (the diff/doc PR #276 is merged; based on release/v0.3.0). Pure cleanup — no behavior change (test count unchanged). Removed (each verified to have no `src/main` caller): - **`ReviewResult`** 10-arg and 11-arg convenience constructors — production uses the 12-arg and canonical 13-arg forms. The ~60 test sites are padded to the 12-arg form. - **`VerdictBuilder.buildResult`** 4-arg and 5-arg overloads — production reaches the builder through `build(...)`; the overloads only let tests bypass the real input derivation (diff-stat / backstop). The 19 test sites move to the canonical 8-arg `buildResult`. - **`ReviewContextLoader.buildDiffString` / `buildBaseComparison`** thin pass-throughs — `load()` uses the `*WithStats` methods and the formatter directly. The loader tests now call `diffFormatter.buildDiffString(...)` and `loader.buildBaseComparisonWithStats(...).text()`. ## Related Issues N/A — v0.3.0 final-review remaining item (#13). Relates to #250. ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing - All migrated test sites compile against the canonical signatures and pass unchanged; removing the overloads is what proves they had no production reach. - Full suite green: 1374 tests, SpotBugs 0, Spotless clean. No production behavior changed, so no CHANGELOG entry. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly (N/A — no behavior change) - [x] My changes generate no new warnings or errors ## Additional Notes **PR 4 of 4** — the last of the v0.3.0 final-review cleanup, based on release/v0.3.0 (the diff/doc PR #276 is merged). With this merged, all six requested remaining items (#7, #8, #10, #13, #14, #15) plus the audit-found regressions are closed.

…estion can't be placed (+ false-truncation, null-reviewer, docs) (#278) ## What type of PR is this? - [x] 🐛 Bug fix - [x] 📝 Documentation ## Description Correctness fixes + doc gaps from the final ultra code-review of `release/v0.3.0`. **PR 1 of 2**; the other (truncated-celebration messaging + a suggestion-range edge) follows separately. Off `release/v0.3.0`. **#1 — `/add-docs` corrupted wrapped declarations (on by default).** `postDoc` anchored every doc suggestion as a single-line GitHub suggestion (`start_line=null`), but `DocGeneratorPrompts` permits a multi-line `suggestion_old` for a declaration that wraps across lines. On commit GitHub replaced only `doc.line()`, leaving the rest of the signature in place → broken/duplicated code. It now resolves the range via `DiffLineResolver.resolveSuggestionRange` (the same #71 anchoring the review path uses) and **drops** a multi-line suggestion it can't anchor to a single hunk rather than mis-anchoring it. **#2 — Base-comparison trimming forced a false "partial review".** `omittedFiles` summed the PR diff's omitted files *and* the supplementary base↔head comparison's, so a PR whose full diff was reviewed could be held from APPROVE and labelled partial when only the regression context was trimmed. Only the PR diff's omitted files gate the verdict now. *(Pre-existing — present verbatim in the `v0.2.1` tag.)* **#3 — NPE on a prior review from a deleted account.** `isFirstVisibleReview` dereferenced each prior review's author with no null guard; a review from a since-deleted account (`user: null`) threw an NPE and failed the whole review. The author is null-checked now, matching the sibling comment/summary checks. *(Pre-existing — present verbatim in `v0.2.1`.)* **Docs (#6/#7) + cleanup (#8).** CHANGELOG `[0.3.0]` Added now lists the shipped-but-undocumented features — `/add-docs` (#56), the changed-files walkthrough (#179), and the opt-in Mermaid diagram (#181). README documents `REVIEW_DIAGRAM_ENABLED` and lists `/changelog` among the paused commands. Dropped the pointless `num()` wrapper and fixed a misattributed comment. ## Related Issues N/A — surfaced by the v0.3.0 final code-review. Relates to #71, #234, #56, #181. ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing - `DocGenerationServiceTest.postsMultiLineSuggestionAnchoredToTheWholeDeclarationRange` (start_line..line span) and `skipsMultiLineSuggestionThatCannotBeAnchored`. - `ReviewOrchestratorTest.reviewSurvivesAPriorReviewFromADeletedAccount` (null-user review → no NPE, session completes). - Full suite green: 1377 tests, SpotBugs 0, Spotless clean, patch fully covered. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly (CHANGELOG + README) - [x] My changes generate no new warnings or errors ## Additional Notes #2 and #3 fix bugs that shipped in v0.2.1 (verified against the tag), so they get CHANGELOG entries; #1 fixes a v0.3.0 feature, covered by its Added entry. PR 2 of 2 will handle the truncated-clean celebration messaging and the `resolveSuggestionRange` blank-line edge.

…ndings, summary-comment failure is best-effort Perf + resilience follow-ons from the second v0.3.0 review: - #2 DocGenerationService: generate() re-ran the ignore-glob filter (buildDiffString) even though handle() already computed the reviewable list. Thread the precomputed list through and use the reuse overload — one glob walk per /add-docs run, matching the F8 single-pass the review path already does. - #7 ReviewPublisher.postInlineComments: once the inline-comment cap was reached, the loop exited and remaining findings were neither posted nor returned, so an over-cap finding (esp. one outside the diff) was silently dropped. The loop now visits all findings and returns the over-cap ones as unanchored, so the review body still reports them — capped on inline noise, never dropped. - #9 ReviewOrchestrator: the first-review summary comment is enrichment, but a transient failure posting it aborted before postReview and surfaced a hard FAILED check for a review that would otherwise post. It's now best-effort; the review is the critical step. #8 (an orphaned CI future when the AI call throws) is intentionally not "fixed": CompletableFuture.cancel() from supplyAsync cannot interrupt the in-flight GitHub call, and the executor is unbounded, so the wasted I/O is bounded and harmless — adding a cancel would be cosmetic. Documented in the PR rather than cargo-culted. All v0.3.0-era paths (#56 / #278 / #254), so no CHANGELOG entries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…-effort summary comment (#2,#7,#9; #8 accepted) (#282) ## What type of PR is this? - [x] 🐛 Bug fix - [x] ⚡ Performance ## Description Perf + resilience follow-ons from the second ultra code-review of `release/v0.3.0`. **PR 3 of 4** (A and B already merged into release). Off `release/v0.3.0`. **#2 — `/add-docs` walked the ignore-glob filter twice.** `handle()` computes the reviewable list once, but `generate()` then called `buildDiffString(files)`, which re-ran `reviewableFiles(files)` internally. The precomputed list is now threaded through to the existing reuse overload (`buildDiffStringWithStats(files, reviewable)`) — one glob walk per run, matching the F8 single-pass the review path already does. **#7 — over-cap findings were silently dropped.** `postInlineComments`'s loop condition exited as soon as `posted` hit `maxReviewComments`, so findings past the cap were neither posted inline nor returned as unanchored — and both review-body fallbacks draw only from the unanchored list, so an over-cap finding whose line was outside the diff vanished. The loop now visits every finding and gates only the *inline post* on the cap; the rest are returned as unanchored so the review body still reports them. (Only bites when an operator lowers the cap below the finding count; default is 50.) **#9 — a transient summary-comment failure failed the whole review.** Since #254, `resultSurfaced` is set only after `postReview`, so a failure posting the first-review summary comment (enrichment) aborted into `handleReviewFailure` — a hard FAILED check + "retry" notice — even though the review itself hadn't been posted yet and would have succeeded. The summary comment is now best-effort; the review is the critical step. **#8 — orphaned CI future (intentionally not fixed).** When the AI call throws, the concurrent CI-resolution future is left running. This is **consciously accepted**: `CompletableFuture.cancel()` from `supplyAsync` cannot interrupt the in-flight GitHub call (interrupts aren't used), and the review executor is an unbounded virtual-thread-per-task pool, so the wasted I/O is bounded and harmless. Adding a `cancel()` would be cosmetic (zero runtime effect), so it's documented here rather than cargo-culted. ## Related Issues N/A — surfaced by the second v0.3.0 ultra code-review. Relates to #56, #254, #278. ## How Has This Been Tested? - [x] Unit tests - `ReviewOrchestratorTest`: `shouldRespectMaxReviewCommentsLimit` now also asserts the over-cap finding is returned as unanchored (#7); `summaryCommentFailureDoesNotAbortTheReview` (#9). - `DocGenerationServiceTest`: `summaryReportsBothCommittableSuggestionsAndNotes`, `aNoteThatGitHubRejectsIsNotCounted`. - Full suite green: 1393 tests, SpotBugs 0, Spotless clean, patch fully covered (merge-base). ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly (no CHANGELOG — all v0.3.0-era paths) - [x] My changes generate no new warnings or errors ## Additional Notes **PR 3 of 4.** All fixes are within v0.3.0-introduced paths (`/add-docs` #56, the unanchored-body #278, the `resultSurfaced` ordering #254), so they net out under the changelog policy — no CHANGELOG lines. Stacked on #281; D (cleanup) follows.

…ps, complete CI caption, pom 0.3.0 (#285) ## What type of PR is this? - [x] 🐛 Bug fix ## Description The **v0.3.0 release gate**: the functional findings from the full v0.2.1→release-tip readiness review, so the branch satisfies "new features work, no regressions/new bugs" and can be tagged. The removed-behaviour auditor confirmed **no v0.2.1 feature was broken**; this PR fixes the new-bugs/feature-gaps it did surface. Pure structural cleanup is deferred to 0.3.1 (see below). All fixes are within v0.3.0-introduced paths, so no CHANGELOG entries. **#1 — first-review body could point at a non-existent summary comment.** When no finding anchors inline, the first-review body said "see the PR summary comment above". But #282 made the summary post best-effort, so a transient comment failure left the body referencing a missing comment, and the finding descriptions (carried only by the summary's brief Key Findings) were lost. The body now lists every finding **with its description** regardless of review type — the body is the one surface guaranteed to carry the detail. **#3 — `/add-docs` silently dropped over-cap docs.** Once the per-run comment cap was hit the loop broke and remaining postable docs vanished (debug log only), so on a large PR the maintainer believed coverage was complete. The summary now discloses *"N more changed symbol(s) were not documented because the per-run comment cap was reached — re-run `/add-docs`"*. **#2 — check-run caption dropped a hold reason.** When CI was **both** offending and unreadable, `checkSummaryForResult` returned on the offending branch and omitted the unreadable disclosure that the PR review comment already shows. Both surfaces now agree. **#5 — stale javadoc.** `ReviewResult.keyFindings()` still claimed it was "shared with the review-body fallback" after #284 removed that dedup; corrected. **#14 — pom version.** `0.2.2-SNAPSHOT` → `0.3.0-SNAPSHOT` (the branch ships v0.3.0 / CHANGELOG `[0.3.0]`; required to tag). **Verified, no change:** #13 — `/changelog` `prNumber` rendering is already guarded by `AiServicePromptRenderingTest` (the #186 regression test) and `@UserMessage` is correctly on the method. **Deferred to 0.3.1** (pure structure/dedup, zero functional impact): hold-reason logic dedup (#8), `DiffLineResolver` variant-lookup unification (#7), multi-line-suggestion protocol dedup (#6), `ReviewResult` build dedup (#9), sequential CI fetch overlap (#10), dead `patchesByFile`/`DiffStats` test-only members (#4, #11), and the over-cap review-finding label (#12 — findings are surfaced, only the header wording is imprecise). ## Related Issues N/A — from the v0.2.1→v0.3.0 release-readiness review. ## How Has This Been Tested? - [x] Unit tests - `ReviewOrchestratorTest.shouldListFindingsWithDescriptionsInReviewBodyWhenNoneAnchorInlineOnFirstReview` (#1), `checkSummaryForResultShouldDiscloseUnreadableCiAlongsideAnOffendingCheck` (#2). - `DocGenerationServiceTest.capsAtMaxReviewComments` now asserts the cap-drop disclosure (#3). - Full suite green: 1394 tests, SpotBugs 0, Spotless clean, patch fully covered (merge-base). ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly (no CHANGELOG — all v0.3.0-era paths) - [x] My changes generate no new warnings or errors ## Additional Notes After this merges, the release criteria are met (features work, no regressions, pom at 0.3.0-SNAPSHOT) and `v0.3.0` can be tagged; the deferred cleanup lands in 0.3.1.