diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java index d0aed110..aadd3b09 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java @@ -19,6 +19,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import dev.thiagogonzaga.thrillhousebot.config.BotIdentity; import dev.thiagogonzaga.thrillhousebot.dashboard.ReviewSession; +import dev.thiagogonzaga.thrillhousebot.github.GitHubPullRequestClient; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; import dev.thiagogonzaga.thrillhousebot.review.ai.AiReviewService; import dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService; @@ -50,6 +51,9 @@ @ApplicationScoped public class FindingPipeline { + /** Directory rows listed in the scope header before the remainder is rolled up by count. */ + private static final int MAX_SCOPE_DIRECTORIES = 10; + private record BatchOutcome( int index, List findings, @@ -551,6 +555,8 @@ private static String changedFilesOverview( if (!pureRenames.isEmpty()) { sb.append(ReviewDiffFormatter.formatPureRenameRollup(pureRenames)); } + // Scope totals next, ahead of the per-file rows, for the same reason: clamping drops the tail. + sb.append(changeScopeSummary(ctx)); var omitted = Set.copyOf(plan.omittedFiles()); var clipped = Set.copyOf(plan.clippedFiles()); for (var file : ctx.reviewableFiles()) { @@ -576,6 +582,93 @@ private static String changedFilesOverview( return sb.toString(); } + /** + * PR-level scope header for the summary call: the authoritative file/line totals (GitHub's, the + * same numbers the rendered Changes Overview reports — #298) plus how the change is spread across + * directories. The summary call never sees the diff, so without these the only cue for how big + * the change is is a file list the input budget may have clamped — which is how a multi-file + * decompose got described as one extracted class (#335). Rendered as data, ahead of the per-file + * rows, so clamping can only take the tail. + */ + private static String changeScopeSummary(ReviewContextLoader.ReviewContext ctx) { + var files = VerdictBuilder.overviewFiles(ctx); + var additions = 0; + var deletions = 0; + for (var file : files) { + additions += file.additions(); + deletions += file.deletions(); + } + var totals = ctx.prTotals(); + // A non-positive file count means the totals carry nothing usable: fall back to the + // diff-derived counts rather than announcing a zero-file PR over a non-empty file list. + var authoritative = totals != null && totals.filesChanged() > 0; + var filesChanged = authoritative ? totals.filesChanged() : files.size(); + if (authoritative) { + additions = totals.additions(); + deletions = totals.deletions(); + } + if (filesChanged <= 0) { + return ""; + } + var sb = new StringBuilder(); + sb.append("PR scope (whole pull request): ") + .append(filesChanged) + .append(filesChanged == 1 ? " file changed, +" : " files changed, +") + .append(additions) + .append(" -") + .append(deletions) + .append("\n"); + appendDirectoryBreakdown(sb, files); + return sb.toString(); + } + + /** + * "directory: N files (+a -d)" rows, most files first, so a change spread over several packages + * cannot read as a single-file edit. Bounded so a wide PR cannot crowd out the file list. + */ + private static void appendDirectoryBreakdown( + StringBuilder sb, List files) { + if (files.isEmpty()) { + return; + } + var byDirectory = new LinkedHashMap(); + for (var file : files) { + var stats = byDirectory.computeIfAbsent(directoryOf(file.filename()), key -> new int[3]); + stats[0]++; + stats[1] += file.additions(); + stats[2] += file.deletions(); + } + var rows = new ArrayList<>(byDirectory.entrySet()); + rows.sort( + Comparator.>comparingInt(e -> -e.getValue()[0]) + .thenComparing(Map.Entry::getKey)); + sb.append("Directories touched: ").append(byDirectory.size()).append("\n"); + for (var row : rows.subList(0, Math.min(rows.size(), MAX_SCOPE_DIRECTORIES))) { + sb.append("- ") + .append(row.getKey()) + .append(": ") + .append(row.getValue()[0]) + .append(row.getValue()[0] == 1 ? " file (+" : " files (+") + .append(row.getValue()[1]) + .append(" -") + .append(row.getValue()[2]) + .append(")\n"); + } + if (rows.size() > MAX_SCOPE_DIRECTORIES) { + sb.append("- (+").append(rows.size() - MAX_SCOPE_DIRECTORIES).append(" more directories)\n"); + } + } + + /** + * The file's parent directory, or a stable label for a path carrying no directory component — + * which also covers a blank one. A null path is not guarded here: the per-file loop above already + * dereferences the same name against an immutable set, so it could never reach this point. + */ + private static String directoryOf(String path) { + var slash = path.lastIndexOf('/'); + return slash <= 0 ? "(repository root)" : path.substring(0, slash); + } + /** * Runs the raw model response through the full post-AI chain and persists it. The {@code * lineResolver} is shared with the caller's verdict backstop, so it is passed in rather than diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java index bc3de5c0..66a157ee 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java @@ -357,16 +357,25 @@ mismatches between what the author claims and what the code does (claimed change - Base summary.total_findings and the per-severity counts on the findings provided below — unless a "(+N more findings not shown …)" note follows the array; use that note's stated true totals then, since the list was truncated to fit your input. - - overall_assessment and pr_purpose must be consistent with those findings and the - changed-files list; do not contradict them. + - overall_assessment and pr_purpose must be consistent with those findings, with the PR + scope totals, and with the changed-files list; do not contradict them. A summary whose + scope is narrower than the stated PR scope is wrong: few or no findings means the + reviewed code looked fine, never that the change was small or touched one file. The "summary" object must include: - total_findings, critical, high, medium, low: counts of the findings provided below - overall_assessment: one-sentence verdict on the change - - pr_purpose: 1-3 sentences on what this change does, derived from the changed files and - findings — describe behavior, not file names + - pr_purpose: 1-3 sentences on what the WHOLE change set does. You do not see the diff, + so ground it in the PR title and description (the author's stated intent) together + with the PR scope totals and the changed-file list below, which are computed from the + diff and are authoritative. Cover the change set as a whole: when it spans many files + or several directories, say so and name the main areas it touches. Never present one + extracted class, one file, or the one component that happens to carry findings as if + it were the whole pull request. Describe behavior, not a file listing. - description_gaps: when a PR description is provided, an array of concrete mismatches - between what the author claims and what the change does. Empty array otherwise. + between what the author claims and what the change does — including a description + whose scope is narrower than the change itself (it covers one component, or far fewer + files than the PR scope totals report). Empty array otherwise. - file_summaries: an array of { path, summary } objects giving a file-by-file walkthrough. "path" must match a changed-file path exactly; "summary" is a single line (max ~100 chars) on what changed in that file. Most impactful first, cap at 15. @@ -396,7 +405,9 @@ mismatches between what the author claims and what the code does (claimed change ## Findings already computed for this PR (final — summarize, do not change) {{findings}} - ## Changed files + ## PR scope and changed files (computed from the diff — authoritative) + Everything listed here belongs to this pull request; the purpose you write must + account for all of it, not just the entries with findings. {{changedFiles}} {{#if previousFindings}} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java index 592e222b..23f82b17 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java @@ -42,6 +42,7 @@ import dev.thiagogonzaga.thrillhousebot.review.ai.PrReviewPrompts; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; import dev.thiagogonzaga.thrillhousebot.review.ai.TokenCounter; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.concurrent.CompletionException; @@ -113,6 +114,17 @@ private static ReviewContextLoader.ReviewContext reviewContext() { /** Same context with a caller-supplied changed-file list, so rename disclosure can be driven. */ private static ReviewContextLoader.ReviewContext reviewContext(List files) { + return reviewContext( + files, + List.of( + new FileDiff("a.java", "modified", 3, 0, 3, ""), + new FileDiff("b.java", "modified", 2, 0, 2, "")), + null); + } + + /** Same context with an explicit reviewable-file set and GitHub's PR totals (may be null). */ + private static ReviewContextLoader.ReviewContext reviewContext( + List files, List reviewableFiles, ReviewContextLoader.PrTotals prTotals) { return new ReviewContextLoader.ReviewContext( files, "raw legacy diff", @@ -130,11 +142,9 @@ private static ReviewContextLoader.ReviewContext reviewContext(List fi List.of(), "", "", - List.of( - new FileDiff("a.java", "modified", 3, 0, 3, ""), - new FileDiff("b.java", "modified", 2, 0, 2, "")), + reviewableFiles, () -> new DiffLineResolver(Map.of()), - null); + prTotals); } private static DiffBudgetPlanner.BudgetPlan multiBatchPlan() { @@ -275,6 +285,150 @@ void summaryOverviewLeadsWithThePureRenameRollup() { assertTrue(captor.getValue().changedFiles().contains("pkg/A.java → pkg/B.java")); } + /** Runs the multi-call path and returns the changed-files section the summary call received. */ + private String captureSummaryChangedFiles( + ReviewSession session, ReviewContextLoader.ReviewContext ctx) { + var template = new AiReviewService.PromptInputs("d", "ctx", "base", "stack", "tests", "", ""); + when(aiReviewService.reviewBatch(eq(session), any(), anyInt(), anyInt())) + .thenReturn(new ReviewResponse(List.of(), List.of(), null)); + var captor = ArgumentCaptor.forClass(AiReviewService.SummaryInputs.class); + when(aiReviewService.summarize(eq(session), captor.capture())) + .thenReturn(new ReviewResponse(List.of(), List.of(), null)); + + pipeline.run(session, template, ctx, multiBatchPlan(), new DiffLineResolver(Map.of())); + + return captor.getValue().changedFiles(); + } + + @Test + void summaryOverviewStatesTheWholePrScopeForAMultiFileRefactor() { + // The #335 fixture: a decompose whose title/body announce the full scope, but whose summary + // call sees no diff — without these totals nothing tells it the change is more than one class. + var session = ReviewSession.create("owner/repo", 1, "Decompose the orchestrator", "sha"); + var files = + List.of( + new FileDiff( + "src/main/java/app/review/Orchestrator.java", "modified", 40, 900, 940, ""), + new FileDiff( + "src/main/java/app/review/CiStatusEvaluator.java", "added", 120, 0, 120, ""), + new FileDiff("src/main/java/app/review/FindingPipeline.java", "added", 300, 0, 300, ""), + new FileDiff("src/test/java/app/review/PipelineTest.java", "added", 200, 0, 200, ""), + new FileDiff("README.md", "modified", 4, 2, 6, "")); + var overview = captureSummaryChangedFiles(session, reviewContext(files, files, null)); + + assertTrue( + overview.contains("PR scope (whole pull request): 5 files changed, +664 -902"), overview); + assertTrue(overview.contains("Directories touched: 3"), overview); + assertTrue(overview.contains("- src/main/java/app/review: 3 files (+460 -900)"), overview); + assertTrue(overview.contains("- src/test/java/app/review: 1 file (+200 -0)"), overview); + assertTrue(overview.contains("- (repository root): 1 file (+4 -2)"), overview); + // Ahead of the per-file rows, so clamping a long overview can only drop the tail. + assertTrue( + overview.indexOf("PR scope (whole pull request)") < overview.indexOf("README.md (modified"), + overview); + } + + @Test + void summaryOverviewScopeUsesGitHubsAuthoritativeTotalsWhenAvailable() { + // Same totals the rendered Changes Overview reports (#298), so the prose cannot contradict it. + var session = ReviewSession.create("owner/repo", 1, "Big PR", "sha"); + var reviewable = + List.of( + new FileDiff("a.java", "modified", 3, 0, 3, ""), + new FileDiff("b.java", "modified", 2, 0, 2, "")); + var ctx = reviewContext(List.of(), reviewable, new ReviewContextLoader.PrTotals(23, 1612, 240)); + + var overview = captureSummaryChangedFiles(session, ctx); + + assertTrue( + overview.contains("PR scope (whole pull request): 23 files changed, +1612 -240"), overview); + } + + @Test + void summaryOverviewScopeStaysSingularForASingleFilePr() { + // No regression on small single-purpose PRs: no multi-file or multi-directory language. + var session = ReviewSession.create("owner/repo", 1, "Fix a typo", "sha"); + var files = List.of(new FileDiff("src/main/java/app/Tiny.java", "modified", 3, 1, 4, "")); + var overview = captureSummaryChangedFiles(session, reviewContext(files, files, null)); + + assertTrue(overview.contains("PR scope (whole pull request): 1 file changed, +3 -1"), overview); + assertTrue(overview.contains("Directories touched: 1"), overview); + assertTrue(overview.contains("- src/main/java/app: 1 file (+3 -1)"), overview); + assertFalse(overview.contains("files changed"), overview); + assertFalse(overview.contains("more directories"), overview); + } + + @Test + void summaryOverviewScopeFallsBackToDiffCountsWhenPrTotalsAreEmpty() { + // A zero file count means the totals carry nothing usable; announcing a zero-file PR over a + // non-empty file list would contradict the very list printed below it. + var session = ReviewSession.create("owner/repo", 1, "Totals unavailable", "sha"); + var reviewable = + List.of( + new FileDiff("a.java", "modified", 3, 0, 3, ""), + new FileDiff("b.java", "modified", 2, 0, 2, "")); + var ctx = reviewContext(List.of(), reviewable, new ReviewContextLoader.PrTotals(0, 0, 0)); + + var overview = captureSummaryChangedFiles(session, ctx); + + assertTrue( + overview.contains("PR scope (whole pull request): 2 files changed, +5 -0"), overview); + } + + @Test + void summaryOverviewOmitsTheScopeBlockWhenNothingIsInTheChangeSet() { + // Nothing reviewable and no PR totals: emit no scope header at all rather than "0 files". + var session = ReviewSession.create("owner/repo", 1, "Everything ignored", "sha"); + + var overview = captureSummaryChangedFiles(session, reviewContext(List.of(), List.of(), null)); + + assertEquals("", overview); + } + + @Test + void summaryOverviewScopeKeepsTotalsWhenNoFileSurvivesTheIgnoreGlob() { + // GitHub still reports the PR's real size when the ignore-glob drops every changed file, so + // the header stands on its own — with no directory breakdown, which would have to be empty. + var session = ReviewSession.create("owner/repo", 1, "All ignored", "sha"); + var ctx = reviewContext(List.of(), List.of(), new ReviewContextLoader.PrTotals(23, 1612, 240)); + + var overview = captureSummaryChangedFiles(session, ctx); + + assertEquals("PR scope (whole pull request): 23 files changed, +1612 -240\n", overview); + } + + @Test + void summaryOverviewScopeBucketsAPathWithNoDirectoryAtTheRoot() { + // A name carrying no directory component — a blank one included — buckets at the root instead + // of opening a directory row named after it. + var session = ReviewSession.create("owner/repo", 1, "Odd payload", "sha"); + var files = + List.of( + new FileDiff(" ", "modified", 2, 0, 2, ""), + new FileDiff("src/app/A.java", "modified", 4, 1, 5, "")); + + var overview = captureSummaryChangedFiles(session, reviewContext(files, files, null)); + + assertTrue( + overview.contains("PR scope (whole pull request): 2 files changed, +6 -1"), overview); + assertTrue(overview.contains("Directories touched: 2"), overview); + assertTrue(overview.contains("- (repository root): 1 file (+2 -0)"), overview); + assertTrue(overview.contains("- src/app: 1 file (+4 -1)"), overview); + } + + @Test + void summaryOverviewRollsUpDirectoriesBeyondTheCap() { + var session = ReviewSession.create("owner/repo", 1, "Wide PR", "sha"); + var files = new ArrayList(); + for (var i = 0; i < 12; i++) { + files.add(new FileDiff("pkg" + i + "/File.java", "modified", 1, 0, 1, "")); + } + var overview = captureSummaryChangedFiles(session, reviewContext(files, files, null)); + + assertTrue(overview.contains("Directories touched: 12"), overview); + assertTrue(overview.contains("- (+2 more directories)"), overview); + } + @Test void budgetedSingleBatchSendsThePlannedTextNotTheRawDiff() { var session = ReviewSession.create("owner/repo", 1, "One big file", "sha"); @@ -500,13 +654,26 @@ void summaryFindingsJsonIsClampedToThePerCallBudget() throws Exception { var session = ReviewSession.create("owner/repo", 1, "Big PR", "sha"); var ctx = reviewContext(); var template = new AiReviewService.PromptInputs("d", "d", "", "", "", "", ""); - var high = new ReviewResponse.Finding("high", "high", "a.java", 1, "H", "d", "o", "n"); - var medium = new ReviewResponse.Finding("medium", "high", "a.java", 3, "M", "d", "o", "n"); - var nullRisk = new ReviewResponse.Finding(null, "high", "a.java", 2, "N", "d", "o", "n"); - var critical = new ReviewResponse.Finding("critical", "high", "b.java", 1, "C", "d", "o", "n"); + // Descriptions long enough that one finding outweighs the changed-files overview: the budget + // below leaves room for exactly one, and the overview keeps its own (larger) share unclamped. + var desc = + "this description exists to make one serialized finding the dominant cost ".repeat(3); + var high = new ReviewResponse.Finding("high", "high", "a.java", 1, "H", desc, "o", "n"); + var medium = new ReviewResponse.Finding("medium", "high", "a.java", 3, "M", desc, "o", "n"); + var nullRisk = new ReviewResponse.Finding(null, "high", "a.java", 2, "N", desc, "o", "n"); + var critical = new ReviewResponse.Finding("critical", "high", "b.java", 1, "C", desc, "o", "n"); var tokenCounter = new TokenCounter(); - var overview = "a.java (modified, +3 -0)\nb.java (modified, +2 -0)\n"; + // The overview the pipeline will build (scope header + per-file rows), so the budget below is + // calibrated against the same fixed sections the production code measures. + var overview = + """ + PR scope (whole pull request): 2 files changed, +5 -0 + Directories touched: 1 + - (repository root): 2 files (+5 -0) + a.java (modified, +3 -0) + b.java (modified, +2 -0) + """; var fixedSections = PrReviewPrompts.SUMMARY_SYSTEM + PrReviewPrompts.SUMMARY_USER + "d" + overview; var criticalJson = new ObjectMapper().writeValueAsString(List.of(critical)); diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPromptsContentTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPromptsContentTest.java index db366aff..9bd1f502 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPromptsContentTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPromptsContentTest.java @@ -24,9 +24,10 @@ * config/IaC severity recalibration (so declarative findings are not suppressed), the * parameter-nullability / unseen-caller guard (#107), the in-diff-test exercise gate (a green test * must demonstrably hit the claimed path before it can invalidate a finding — #116), the symmetric - * exact-arithmetic / "test fails" cap (#97), and the heuristic failure-mode characterization pass - * with its verifier exemption (#123). These assertions are intentionally coarse — they check intent - * survives, not exact wording; an intentional rewording should update the matching anchor. + * exact-arithmetic / "test fails" cap (#97), the heuristic failure-mode characterization pass with + * its verifier exemption (#123), and the summary call's whole-change-set grounding (#335). These + * assertions are intentionally coarse — they check intent survives, not exact wording; an + * intentional rewording should update the matching anchor. * *

The automated LLM eval that checks the model actually acts on this guidance is * tracked separately ({@code evalcorpus/}); this is the cheap deterministic guard. @@ -361,4 +362,56 @@ void diagramRequestGivesSequenceDiagramItsOwnSyntaxRules() { "participant O as ReviewOrchestrator", "the diagram prompt must include a correct sequence-diagram example"); } + + @Test + void summaryPromptGroundsThePurposeInTheWholeChangeSet() { + String sys = PrReviewPrompts.SUMMARY_SYSTEM; + assertContains( + sys, + "what the WHOLE change set does", + "pr_purpose must be scoped to the whole change set, not one file (#335)"); + assertContains( + sys, + "PR title and description (the author's stated intent)", + "pr_purpose must be grounded in the PR title/description"); + assertContains( + sys, + "PR scope totals and the changed-file list", + "pr_purpose must be grounded in the authoritative scope totals"); + assertContains( + sys, + "extracted class, one file, or the one component that happens to carry findings", + "the summary must not present one extracted class as the whole PR (#335)"); + } + + @Test + void summaryPromptRejectsASummaryNarrowerThanThePrScope() { + String sys = PrReviewPrompts.SUMMARY_SYSTEM; + assertContains( + sys, + "scope is narrower than the stated PR scope is wrong", + "a summary whose scope is a subset of the diff's must be called out as wrong (#335)"); + assertContains( + sys, + "never that the change was small or touched one file", + "zero findings must not be read as a small change"); + assertContains( + sys, + "whose scope is narrower than the change itself", + "description_gaps must cover a description that covers only part of the change (#335)"); + } + + @Test + void summaryUserPromptFramesTheFileListAsAuthoritativeScope() { + String user = PrReviewPrompts.SUMMARY_USER; + assertContains( + user, + "## PR scope and changed files (computed from the diff — authoritative)", + "the summary user prompt must present the scope block as authoritative"); + assertContains( + user, + "account for all of it", + "the summary user prompt must require the purpose to cover every listed entry"); + assertContains(user, "{{changedFiles}}", "the changed-files slot must survive the rewording"); + } }