perf+fix(review): single glob pass + surface over-cap findings + best-effort summary comment (#2,#7,#9; #8 accepted)#282
Merged
Conversation
…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>
🤖 ThrillhouseBot PR SummaryWhat this PR doesReuses the already-filtered reviewable file list in doc generation to avoid glob re-walk; surfaces over-cap findings as unanchored rather than silently dropping them; makes the summary comment post best-effort so a transient failure doesn't abort the review. Changes Overview
Risk Assessment
No new issues found in this PR, but the review cannot be approved until the required checks are passing.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| docker-pr | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
8 tasks
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…ubstring (#13,#15; #12/#14/#16 deferred) (#283) ## What type of PR is this? - [x] ♻️ Refactor ## Description Cleanup batch from the second ultra code-review of `release/v0.3.0`. **PR 4 of 4.** Off `release/v0.3.0`; file-disjoint from #282 (PR C). No behaviour change. **#13 — duplicated assistant-call skeleton.** `PrDescriptionGenerator.callAssistant` and `ChangelogEntryGenerator.callAssistant` were copy-paste variants of the same `try → empty/blank → strip → catch(RuntimeException) → null` fail-soft skeleton. Extracted it into `AbstractPrSuggestionGenerator.callAssistant(command, Supplier<String>)` — the parent that already factors `loadInputs` — so each subclass keeps only its own assistant call (and `/changelog` keeps its `NONE` post-filter). Removes the drift risk the duplication carried. **#15 — clarify the intentional bot-token substring.** `CiStatusEvaluator.containsBotToken` is a lowercased substring test rather than `BotIdentity.matches` (exact login set), which the review flagged as a reuse/consistency concern. It's deliberate: a check's app slug/name lacks the `[bot]` suffix `BotIdentity` strips, so exact matching would never recognise the bot's own check. Added a comment so it isn't re-flagged or "fixed" into a regression. ### Deferred (cleanup-tier, with the reviewer's own tradeoffs) - **#12 — `ReviewResponse.Summary` telescoping constructors.** Test-only, but unlike the untested pass-throughs F13 (#277) removed, these are documented, forward to the canonical constructor with explicit defaults, **and** have dedicated defaulting tests in `ReviewResponseTest`. Removing them churns 17 call sites across 7 files (and deletes those defaulting tests) for marginal benefit. - **#14 — `MaintainerReplyService.fetchDiff`.** Can't fold onto `SoftLoaders.files` without erasing its deliberate `""`-on-failure degrade (the mention path wants empty context; `SoftLoaders` would yield `"(no changes detected)"`) — a behaviour change on a non-bug the reviewer explicitly called out. - **#16 — `DiffLineResolver` variant lookups.** Unifying the four copies needs parameterizing three distinct ambiguity policies (`presentInFileOrVariant` is intentionally first-match; the others return a null/empty sentinel), a behaviour-drift risk; the code already documents the mirror. Happy to take any of the three deferrals if you'd prefer them done — flagging the tradeoffs rather than silently churning. ## Related Issues N/A — surfaced by the second v0.3.0 ultra code-review. ## How Has This Been Tested? - [x] Unit tests - Existing `PrDescriptionGeneratorTest` / `ChangelogEntryGeneratorTest` / `CiStatusEvaluatorTest` cover the refactored paths unchanged. - Full suite green: 1390 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 — refactor only) - [x] My changes generate no new warnings or errors ## Additional Notes **PR 4 of 4** of the second-review fixes. Pure refactor + a clarifying comment; the three deferrals are documented above with rationale.
8 tasks
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…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.
8 tasks
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…the best-effort summary (Sonnet final review) (#286) ## What type of PR is this? - [x] 🐛 Bug fix ## Description The single HIGH finding from the **Sonnet 5** final pre-tag review (independent second-model pass over the whole v0.2.1→tip delta — opus had swept it twice and missed this). It's a sibling of the #285 #1 fix: same "best-effort summary failure loses disclosure" class, but in the **no-findings** path. **A truncated-but-clean first review could disclose the partial review nowhere on the PR.** When a large PR truncates the diff (`omittedFiles > 0`) with no findings, green/readable CI, and nothing unresolved, `VerdictBuilder` demotes APPROVE → COMMENT *solely* for truncation, and the truncation banner lives only in `summaryMarkdown`. `postNoIssuesReview` then early-returns ("let the summary stand alone") for a first-review COMMENT with no unresolved findings — but since #282 the summary comment is posted **best-effort** (a transient `createComment` 5xx/rate-limit is swallowed). If it fails, nothing is posted: the partial review is surfaced only on the secondary check-run caption, so a maintainer can merge believing the PR was fully reviewed. **Fix:** - `postNoIssuesReview` keeps posting the body for a **truncated** first review (`&& !result.truncated()` added to the skip). CI-only holds still skip — the failing checks are themselves visible on the PR; truncation has no other PR surface. - `noIssuesBody` discloses truncation whenever truncated (drops the `!isFirstReview` guard), so the body carries it independently of the best-effort summary. When the summary *does* post, a duplicated notice is the acceptable cost of never dropping it. ## Related Issues N/A — Sonnet final-review finding. Relates to #282, #234. ## How Has This Been Tested? - [x] Unit tests - `ReviewOrchestratorTest.truncationOnlyHeldFirstReviewStillPostsThePartialReviewBody` — the exact bug scenario: truncated-only-held first review now posts a COMMENT body with the partial-review notice. - `truncatedFirstReviewBodyNowDisclosesPartialReview` (rewritten from the test that asserted the old "omit on first review" behavior). - Existing follow-up truncation tests unchanged. - Full suite green: 1396 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 — v0.3.0-introduced) - [x] My changes generate no new warnings or errors ## Additional Notes Last finding before tagging. After this merges, the Sonnet pass is clean; JVM (1396 tests) + native build are green; pom is at 0.3.0-SNAPSHOT — `v0.3.0` is ready to tag.
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…-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.
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…ubstring (#13,#15; #12/#14/#16 deferred) (#283) ## What type of PR is this? - [x] ♻️ Refactor ## Description Cleanup batch from the second ultra code-review of `release/v0.3.0`. **PR 4 of 4.** Off `release/v0.3.0`; file-disjoint from #282 (PR C). No behaviour change. **#13 — duplicated assistant-call skeleton.** `PrDescriptionGenerator.callAssistant` and `ChangelogEntryGenerator.callAssistant` were copy-paste variants of the same `try → empty/blank → strip → catch(RuntimeException) → null` fail-soft skeleton. Extracted it into `AbstractPrSuggestionGenerator.callAssistant(command, Supplier<String>)` — the parent that already factors `loadInputs` — so each subclass keeps only its own assistant call (and `/changelog` keeps its `NONE` post-filter). Removes the drift risk the duplication carried. **#15 — clarify the intentional bot-token substring.** `CiStatusEvaluator.containsBotToken` is a lowercased substring test rather than `BotIdentity.matches` (exact login set), which the review flagged as a reuse/consistency concern. It's deliberate: a check's app slug/name lacks the `[bot]` suffix `BotIdentity` strips, so exact matching would never recognise the bot's own check. Added a comment so it isn't re-flagged or "fixed" into a regression. ### Deferred (cleanup-tier, with the reviewer's own tradeoffs) - **#12 — `ReviewResponse.Summary` telescoping constructors.** Test-only, but unlike the untested pass-throughs F13 (#277) removed, these are documented, forward to the canonical constructor with explicit defaults, **and** have dedicated defaulting tests in `ReviewResponseTest`. Removing them churns 17 call sites across 7 files (and deletes those defaulting tests) for marginal benefit. - **#14 — `MaintainerReplyService.fetchDiff`.** Can't fold onto `SoftLoaders.files` without erasing its deliberate `""`-on-failure degrade (the mention path wants empty context; `SoftLoaders` would yield `"(no changes detected)"`) — a behaviour change on a non-bug the reviewer explicitly called out. - **#16 — `DiffLineResolver` variant lookups.** Unifying the four copies needs parameterizing three distinct ambiguity policies (`presentInFileOrVariant` is intentionally first-match; the others return a null/empty sentinel), a behaviour-drift risk; the code already documents the mirror. Happy to take any of the three deferrals if you'd prefer them done — flagging the tradeoffs rather than silently churning. ## Related Issues N/A — surfaced by the second v0.3.0 ultra code-review. ## How Has This Been Tested? - [x] Unit tests - Existing `PrDescriptionGeneratorTest` / `ChangelogEntryGeneratorTest` / `CiStatusEvaluatorTest` cover the refactored paths unchanged. - Full suite green: 1390 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 — refactor only) - [x] My changes generate no new warnings or errors ## Additional Notes **PR 4 of 4** of the second-review fixes. Pure refactor + a clarifying comment; the three deferrals are documented above with rationale.
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…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.
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…the best-effort summary (Sonnet final review) (#286) ## What type of PR is this? - [x] 🐛 Bug fix ## Description The single HIGH finding from the **Sonnet 5** final pre-tag review (independent second-model pass over the whole v0.2.1→tip delta — opus had swept it twice and missed this). It's a sibling of the #285 #1 fix: same "best-effort summary failure loses disclosure" class, but in the **no-findings** path. **A truncated-but-clean first review could disclose the partial review nowhere on the PR.** When a large PR truncates the diff (`omittedFiles > 0`) with no findings, green/readable CI, and nothing unresolved, `VerdictBuilder` demotes APPROVE → COMMENT *solely* for truncation, and the truncation banner lives only in `summaryMarkdown`. `postNoIssuesReview` then early-returns ("let the summary stand alone") for a first-review COMMENT with no unresolved findings — but since #282 the summary comment is posted **best-effort** (a transient `createComment` 5xx/rate-limit is swallowed). If it fails, nothing is posted: the partial review is surfaced only on the secondary check-run caption, so a maintainer can merge believing the PR was fully reviewed. **Fix:** - `postNoIssuesReview` keeps posting the body for a **truncated** first review (`&& !result.truncated()` added to the skip). CI-only holds still skip — the failing checks are themselves visible on the PR; truncation has no other PR surface. - `noIssuesBody` discloses truncation whenever truncated (drops the `!isFirstReview` guard), so the body carries it independently of the best-effort summary. When the summary *does* post, a duplicated notice is the acceptable cost of never dropping it. ## Related Issues N/A — Sonnet final-review finding. Relates to #282, #234. ## How Has This Been Tested? - [x] Unit tests - `ReviewOrchestratorTest.truncationOnlyHeldFirstReviewStillPostsThePartialReviewBody` — the exact bug scenario: truncated-only-held first review now posts a COMMENT body with the partial-review notice. - `truncatedFirstReviewBodyNowDisclosesPartialReview` (rewritten from the test that asserted the old "omit on first review" behavior). - Existing follow-up truncation tests unchanged. - Full suite green: 1396 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 — v0.3.0-introduced) - [x] My changes generate no new warnings or errors ## Additional Notes Last finding before tagging. After this merges, the Sonnet pass is clean; JVM (1396 tests) + native build are green; pom is at 0.3.0-SNAPSHOT — `v0.3.0` is ready to tag.
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…-effort summary comment (#282) - [x] 🐛 Bug fix - [x] ⚡ Performance 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. N/A — surfaced by the second v0.3.0 ultra code-review. Relates to #56, - [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). - [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 **PR 3 of 4.** All fixes are within v0.3.0-introduced paths (`/add-docs` they net out under the changelog policy — no CHANGELOG lines. Stacked on
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…ubstring (#283) - [x] ♻️ Refactor Cleanup batch from the second ultra code-review of `release/v0.3.0`. **PR 4 of 4.** Off `release/v0.3.0`; file-disjoint from #282 (PR C). No behaviour change. **#13 — duplicated assistant-call skeleton.** `PrDescriptionGenerator.callAssistant` and `ChangelogEntryGenerator.callAssistant` were copy-paste variants of the same `try → empty/blank → strip → catch(RuntimeException) → null` fail-soft skeleton. Extracted it into `AbstractPrSuggestionGenerator.callAssistant(command, Supplier<String>)` — the parent that already factors `loadInputs` — so each subclass keeps only its own assistant call (and `/changelog` keeps its `NONE` post-filter). Removes the drift risk the duplication carried. **#15 — clarify the intentional bot-token substring.** `CiStatusEvaluator.containsBotToken` is a lowercased substring test rather than `BotIdentity.matches` (exact login set), which the review flagged as a reuse/consistency concern. It's deliberate: a check's app slug/name lacks the `[bot]` suffix `BotIdentity` strips, so exact matching would never recognise the bot's own check. Added a comment so it isn't re-flagged or "fixed" into a regression. - **#12 — `ReviewResponse.Summary` telescoping constructors.** Test-only, but unlike the untested pass-throughs F13 (#277) removed, these are documented, forward to the canonical constructor with explicit defaults, **and** have dedicated defaulting tests in `ReviewResponseTest`. Removing them churns 17 call sites across 7 files (and deletes those defaulting tests) for marginal benefit. - **#14 — `MaintainerReplyService.fetchDiff`.** Can't fold onto `SoftLoaders.files` without erasing its deliberate `""`-on-failure degrade (the mention path wants empty context; `SoftLoaders` would yield `"(no changes detected)"`) — a behaviour change on a non-bug the reviewer explicitly called out. - **#16 — `DiffLineResolver` variant lookups.** Unifying the four copies needs parameterizing three distinct ambiguity policies (`presentInFileOrVariant` is intentionally first-match; the others return a null/empty sentinel), a behaviour-drift risk; the code already documents the mirror. Happy to take any of the three deferrals if you'd prefer them done — flagging the tradeoffs rather than silently churning. N/A — surfaced by the second v0.3.0 ultra code-review. - [x] Unit tests - Existing `PrDescriptionGeneratorTest` / `ChangelogEntryGeneratorTest` / `CiStatusEvaluatorTest` cover the refactored paths unchanged. - Full suite green: 1390 tests, SpotBugs 0, Spotless clean, patch fully covered (merge-base). - [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 — refactor only) - [x] My changes generate no new warnings or errors **PR 4 of 4** of the second-review fixes. Pure refactor + a clarifying comment; the three deferrals are documented above with rationale.
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…ps, complete CI caption, pom 0.3.0 (#285) - [x] 🐛 Bug fix 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). N/A — from the v0.2.1→v0.3.0 release-readiness review. - [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). - [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 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.
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…the best-effort summary (#286) - [x] 🐛 Bug fix The single HIGH finding from the **Sonnet 5** final pre-tag review (independent second-model pass over the whole v0.2.1→tip delta — opus had swept it twice and missed this). It's a sibling of the #285 #1 fix: same "best-effort summary failure loses disclosure" class, but in the **no-findings** path. **A truncated-but-clean first review could disclose the partial review nowhere on the PR.** When a large PR truncates the diff (`omittedFiles > 0`) with no findings, green/readable CI, and nothing unresolved, `VerdictBuilder` demotes APPROVE → COMMENT *solely* for truncation, and the truncation banner lives only in `summaryMarkdown`. `postNoIssuesReview` then early-returns ("let the summary stand alone") for a first-review COMMENT with no unresolved findings — but since #282 the summary comment is posted **best-effort** (a transient `createComment` 5xx/rate-limit is swallowed). If it fails, nothing is posted: the partial review is surfaced only on the secondary check-run caption, so a maintainer can merge believing the PR was fully reviewed. **Fix:** - `postNoIssuesReview` keeps posting the body for a **truncated** first review (`&& !result.truncated()` added to the skip). CI-only holds still skip — the failing checks are themselves visible on the PR; truncation has no other PR surface. - `noIssuesBody` discloses truncation whenever truncated (drops the `!isFirstReview` guard), so the body carries it independently of the best-effort summary. When the summary *does* post, a duplicated notice is the acceptable cost of never dropping it. N/A — Sonnet final-review finding. Relates to #282, #234. - [x] Unit tests - `ReviewOrchestratorTest.truncationOnlyHeldFirstReviewStillPostsThePartialReviewBody` — the exact bug scenario: truncated-only-held first review now posts a COMMENT body with the partial-review notice. - `truncatedFirstReviewBodyNowDisclosesPartialReview` (rewritten from the test that asserted the old "omit on first review" behavior). - Existing follow-up truncation tests unchanged. - Full suite green: 1396 tests, SpotBugs 0, Spotless clean, patch fully covered (merge-base). - [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 — v0.3.0-introduced) - [x] My changes generate no new warnings or errors Last finding before tagging. After this merges, the Sonnet pass is clean; JVM (1396 tests) + native build are green; pom is at 0.3.0-SNAPSHOT — `v0.3.0` is ready to tag.
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…-effort summary comment (#282) - [x] 🐛 Bug fix - [x] ⚡ Performance 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. N/A — surfaced by the second v0.3.0 ultra code-review. Relates to #56, - [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). - [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 **PR 3 of 4.** All fixes are within v0.3.0-introduced paths (`/add-docs` they net out under the changelog policy — no CHANGELOG lines. Stacked on
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…ubstring (#283) - [x] ♻️ Refactor Cleanup batch from the second ultra code-review of `release/v0.3.0`. **PR 4 of 4.** Off `release/v0.3.0`; file-disjoint from #282 (PR C). No behaviour change. **#13 — duplicated assistant-call skeleton.** `PrDescriptionGenerator.callAssistant` and `ChangelogEntryGenerator.callAssistant` were copy-paste variants of the same `try → empty/blank → strip → catch(RuntimeException) → null` fail-soft skeleton. Extracted it into `AbstractPrSuggestionGenerator.callAssistant(command, Supplier<String>)` — the parent that already factors `loadInputs` — so each subclass keeps only its own assistant call (and `/changelog` keeps its `NONE` post-filter). Removes the drift risk the duplication carried. **#15 — clarify the intentional bot-token substring.** `CiStatusEvaluator.containsBotToken` is a lowercased substring test rather than `BotIdentity.matches` (exact login set), which the review flagged as a reuse/consistency concern. It's deliberate: a check's app slug/name lacks the `[bot]` suffix `BotIdentity` strips, so exact matching would never recognise the bot's own check. Added a comment so it isn't re-flagged or "fixed" into a regression. - **#12 — `ReviewResponse.Summary` telescoping constructors.** Test-only, but unlike the untested pass-throughs F13 (#277) removed, these are documented, forward to the canonical constructor with explicit defaults, **and** have dedicated defaulting tests in `ReviewResponseTest`. Removing them churns 17 call sites across 7 files (and deletes those defaulting tests) for marginal benefit. - **#14 — `MaintainerReplyService.fetchDiff`.** Can't fold onto `SoftLoaders.files` without erasing its deliberate `""`-on-failure degrade (the mention path wants empty context; `SoftLoaders` would yield `"(no changes detected)"`) — a behaviour change on a non-bug the reviewer explicitly called out. - **#16 — `DiffLineResolver` variant lookups.** Unifying the four copies needs parameterizing three distinct ambiguity policies (`presentInFileOrVariant` is intentionally first-match; the others return a null/empty sentinel), a behaviour-drift risk; the code already documents the mirror. Happy to take any of the three deferrals if you'd prefer them done — flagging the tradeoffs rather than silently churning. N/A — surfaced by the second v0.3.0 ultra code-review. - [x] Unit tests - Existing `PrDescriptionGeneratorTest` / `ChangelogEntryGeneratorTest` / `CiStatusEvaluatorTest` cover the refactored paths unchanged. - Full suite green: 1390 tests, SpotBugs 0, Spotless clean, patch fully covered (merge-base). - [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 — refactor only) - [x] My changes generate no new warnings or errors **PR 4 of 4** of the second-review fixes. Pure refactor + a clarifying comment; the three deferrals are documented above with rationale.
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…ps, complete CI caption, pom 0.3.0 (#285) - [x] 🐛 Bug fix 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). N/A — from the v0.2.1→v0.3.0 release-readiness review. - [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). - [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 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.
devops-thiago
added a commit
that referenced
this pull request
Jun 30, 2026
…the best-effort summary (#286) - [x] 🐛 Bug fix The single HIGH finding from the **Sonnet 5** final pre-tag review (independent second-model pass over the whole v0.2.1→tip delta — opus had swept it twice and missed this). It's a sibling of the #285 #1 fix: same "best-effort summary failure loses disclosure" class, but in the **no-findings** path. **A truncated-but-clean first review could disclose the partial review nowhere on the PR.** When a large PR truncates the diff (`omittedFiles > 0`) with no findings, green/readable CI, and nothing unresolved, `VerdictBuilder` demotes APPROVE → COMMENT *solely* for truncation, and the truncation banner lives only in `summaryMarkdown`. `postNoIssuesReview` then early-returns ("let the summary stand alone") for a first-review COMMENT with no unresolved findings — but since #282 the summary comment is posted **best-effort** (a transient `createComment` 5xx/rate-limit is swallowed). If it fails, nothing is posted: the partial review is surfaced only on the secondary check-run caption, so a maintainer can merge believing the PR was fully reviewed. **Fix:** - `postNoIssuesReview` keeps posting the body for a **truncated** first review (`&& !result.truncated()` added to the skip). CI-only holds still skip — the failing checks are themselves visible on the PR; truncation has no other PR surface. - `noIssuesBody` discloses truncation whenever truncated (drops the `!isFirstReview` guard), so the body carries it independently of the best-effort summary. When the summary *does* post, a duplicated notice is the acceptable cost of never dropping it. N/A — Sonnet final-review finding. Relates to #282, #234. - [x] Unit tests - `ReviewOrchestratorTest.truncationOnlyHeldFirstReviewStillPostsThePartialReviewBody` — the exact bug scenario: truncated-only-held first review now posts a COMMENT body with the partial-review notice. - `truncatedFirstReviewBodyNowDisclosesPartialReview` (rewritten from the test that asserted the old "omit on first review" behavior). - Existing follow-up truncation tests unchanged. - Full suite green: 1396 tests, SpotBugs 0, Spotless clean, patch fully covered (merge-base). - [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 — v0.3.0-introduced) - [x] My changes generate no new warnings or errors Last finding before tagging. After this merges, the Sonnet pass is clean; JVM (1396 tests) + native build are green; pom is at 0.3.0-SNAPSHOT — `v0.3.0` is ready to tag.
Merged
8 tasks
devops-thiago
added a commit
that referenced
this pull request
Jul 2, 2026
…ary post fails (#338) (#348) ## What type of PR is this? - [x] 🐛 Bug fix ## Description On a truncated diff, a review **with findings** carried the truncation banner only in the PR summary comment — which is posted best-effort (#282) and only on first reviews. If that post failed or was skipped, nothing on the PR disclosed the partial coverage (dogfood finding from #286, which had fixed the same gap for the **no-findings** path). **Fix — both remaining surfaces, same dual-surface standard as #234 / #279 / #286:** - `ReviewPublisher.postReview` (with-findings paths) now appends `ReviewResult.truncationNotice()` to the formal review body whenever the diff was truncated — in the fallback branch where no finding anchored inline, and in the anchored branch, **including the case where every finding anchored and no body was posted at all** (a truncated review now always posts one). A duplicated notice when the summary does post is the acceptable cost of never dropping it, matching `noIssuesBody`. - `VerdictBuilder.checkSummaryForResult` now appends the existing truncation suffix to the findings-count caption too, so the check-run summary also discloses the omitted-file count (previously only the CI-hold and clean branches did). Non-truncated reviews are unchanged (suffix/notice are empty at `omittedFiles == 0`; the existing exact-equals caption test still passes untouched). ## Related Issues Fixes #338 ## How Has This Been Tested? - [x] Unit tests - `truncatedFirstReviewWithFindingsStillPostsBodyDisclosingPartialReview` — the exact bug scenario end-to-end through the real `VerdictBuilder` + `ReviewPublisher`: truncated first review with an anchored finding now posts a review body carrying the partial-review notice (previously no body was posted at all, so the notice's presence is independent of the best-effort summary comment). - `truncatedReviewDisclosesPartialReviewWhenNoFindingsAnchorInline` — the fallback branch (no finding anchored) lists the findings **and** the notice. - `checkSummaryForResultShouldDiscloseTruncationAlongsideFindingCounts` — the check-run findings caption discloses the omitted-file count. - Full suite green locally: 1432 tests, 0 failures; SpotBugs and 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 under Unreleased) - [x] My changes generate no new warnings or errors
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-docswalked the ignore-glob filter twice.handle()computes the reviewable list once, but
generate()then calledbuildDiffString(files), which re-ranreviewableFiles(files)internally. The precomputed list is now threaded through to the existing
reuse overload (
buildDiffStringWithStats(files, reviewable)) — oneglob walk per run, matching the F8 single-pass the review path already
does.
#7 — over-cap findings were silently dropped.
postInlineComments'sloop condition exited as soon as
postedhitmaxReviewComments, sofindings 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,
resultSurfacedis set only afterpostReview, so afailure posting the first-review summary comment (enrichment) aborted
into
handleReviewFailure— a hard FAILED check + "retry" notice — eventhough 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()fromsupplyAsynccannot interrupt the in-flight GitHub call (interruptsaren'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'sdocumented here rather than cargo-culted.
N/A — surfaced by the second v0.3.0 ultra code-review. Relates to #56,
Unit tests
ReviewOrchestratorTest:shouldRespectMaxReviewCommentsLimitnowalso asserts the over-cap finding is returned as unanchored (fix(release): unblock v0.1.0 release workflow #7);
summaryCommentFailureDoesNotAbortTheReview(chore: bump version to 0.1.1-SNAPSHOT after v0.1.0 #9).DocGenerationServiceTest:summaryReportsBothCommittableSuggestionsAndNotes,aNoteThatGitHubRejectsIsNotCounted.Full suite green: 1393 tests, SpotBugs 0, Spotless clean, patch fully
covered (merge-base).
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 (no CHANGELOG — all
v0.3.0-era paths)
My changes generate no new warnings or errors
PR 3 of 4. All fixes are within v0.3.0-introduced paths (
/add-docsthey net out under the changelog policy — no CHANGELOG lines. Stacked on