diff --git a/CHANGELOG.md b/CHANGELOG.md index 10e6c1f..c43d1da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Reviews after a push now judge the whole PR, not just the newest commit: the + review prompt carries the already-reviewed files as read-only context, and the + walkthrough, risk score, coverage signal, and split suggestion are computed + from the full branch. Stops incremental reviews reporting earlier commits' + work as missing. - Docker build: the builder stage now compiles the server only, not the SPA (#43). - Self-heal when a model rejects our chosen `reasoning_effort` (#17). - Stop GPT-5+ reasoning models from starving review output of tokens (#16). diff --git a/src/ai/prompt.ts b/src/ai/prompt.ts index 9d99724..d12b556 100644 --- a/src/ai/prompt.ts +++ b/src/ai/prompt.ts @@ -1,4 +1,4 @@ -import { PRContext, RepoConfig, Learning, IssueContext } from "../types.js"; +import { PRContext, PriorReviewContext, RepoConfig, Learning, IssueContext } from "../types.js"; // ─── Review Prompts ──────────────────────────────────────────── @@ -37,6 +37,7 @@ You MUST respond with valid JSON matching this schema: Rules for the JSON response: - "line" must be a line number that appears in the diff (from the + side of the patch). - "prLevelComments" is for findings that are NOT tied to one specific changed line and therefore cannot be an inline comment. Use it — do NOT invent a line number or bury the finding only in the summary — for: the diff contradicting the PR description (a claimed change is missing, or the code does something the description doesn't mention), issues spanning many files, or concerns about the change as a whole. Each entry has NO "path"/"line". Same title/body/type/severity/aiAgentPrompt/confidence fields as inline comments. Omit the field or use [] when there are none. +- NEVER raise a description-vs-diff finding ("the description claims X but the diff doesn't do it") when the prompt tells you the diff you were shown is partial — an incremental review, a truncated patch, or omitted files. The code backing the claim may simply not have been shown to you, and a separate whole-PR check handles that comparison with the complete diff. A file's absence from what you were given is never evidence that it is unchanged or that its work is missing. - Reserve "prLevelComments" for findings that genuinely have no home in the diff. Unlike an inline comment, a reader cannot resolve one, reply to it, or collapse it — it stays in the review body permanently — so a high-confidence entry here is the most expensive thing you can emit. If a finding CAN be pinned to a changed line, always prefer "comments". Rate "confidence" honestly: a "medium" entry still reaches the reviewer, just without claiming the top of the review. - A REQUEST_CHANGES verdict MUST be backed by at least one concrete finding — an inline "comments" entry or a "prLevelComments" entry. Never request changes while leaving both arrays empty and describing the problem only in "summary". - "path" must exactly match a filename from the changed files. @@ -116,6 +117,54 @@ How to apply: return REVIEW_SYSTEM_BASE + instructions + tone + learningsBlock; } +/** + * Render the "already reviewed earlier in this PR" block for an incremental + * review, or "" when there is nothing prior (every full review, and the first + * review of a PR). + * + * The rules matter more than the diffs. A model handed a one-file delta plus a + * description of a ten-file feature will confidently report the other nine as + * unimplemented — the failure mode this block exists to prevent — so it states + * outright that the diff is partial, that the description covers the earlier + * commits too, and that absence proves nothing. + */ +function buildPriorReviewSection(prior: PriorReviewContext | undefined): string { + if (!prior || prior.files.length === 0) return ""; + + const budget = prior.budget; + const shown = prior.namesOnly + ? [] + : prior.files.filter((f) => !budget?.byFile[f.filename]?.omitted); + const shownNames = new Set(shown.map((f) => f.filename)); + const notShown = prior.files.filter((f) => !shownNames.has(f.filename)); + + const blocks = shown + .map((f) => { + const budgeted = budget?.byFile[f.filename]; + const patch = budgeted ? budgeted.patch : f.patch; + const truncNote = budgeted?.truncated ? " _(patch truncated to fit the size budget)_" : ""; + return `### ${f.filename}${truncNote}\n\`\`\`diff\n${patch}\n\`\`\``; + }) + .join("\n\n"); + + const notShownNote = + notShown.length > 0 + ? `\n\nAlso already reviewed, diffs not shown here to stay within the size budget: ${notShown + .map((f) => `\`${f.filename}\``) + .join(", ")}. They are part of this pull request and their changes are in place.` + : ""; + + return ` + +## Already reviewed earlier in this PR (context only — do NOT comment on these) + +This is an incremental review. The "Changed Files" section above contains ONLY what changed since the last review of this pull request. The files below belong to the same pull request and were reviewed on an earlier commit; they are shown so you can judge the new changes against the complete change set. + +- The PR title and description describe the ENTIRE pull request, including this earlier work. Treat what the description promises as already delivered by it. +- Do NOT report that something the description mentions is missing, unimplemented, or not wired up. You are looking at a partial diff, and absence from the "Changed Files" section is not evidence that code does not exist. +- Post findings ONLY against files in "Changed Files". These files were already reviewed — do not re-report issues in them.${blocks ? "\n\n" + blocks : ""}${notShownNote}`; +} + export function buildReviewPrompt( context: PRContext, repoConfig?: RepoConfig, @@ -155,6 +204,13 @@ export function buildReviewPrompt( .join(", ")}. These were not shown to you — do not assume they are correct or approve them.` : ""; + // On an incremental run "Changed Files" above is only the delta since the + // last review, while the title/description below cover the whole branch. Show + // the already-reviewed remainder of the PR so the model can judge the new + // commit against the complete change set — and, critically, so it stops + // reporting the earlier commits' work as missing. + const priorSection = buildPriorReviewSection(context.priorReview); + // Bounded graph-backed context (whole-function bodies, cross-file // dependents/dependencies, high-fan-in flags). Already token-budgeted by the // builder; injected only when present so diff-only behaviour is preserved. @@ -172,7 +228,7 @@ ${context.description || "(no description provided)"} ## Changed Files -${filesSection}${omittedNote}${relatedSection} +${filesSection}${omittedNote}${priorSection}${relatedSection} Review this pull request and respond with JSON. Remember to obey the Repository Learnings in the system prompt — they override your default flagging heuristics.`; diff --git a/src/reviewer.ts b/src/reviewer.ts index 2e8e04f..a6d225f 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -1,5 +1,5 @@ import { randomUUID } from "node:crypto"; -import { Config, AIProvider, FileChange, PRContext, RepoConfig, ReviewComment } from "./types.js"; +import { Config, AIProvider, DiffBudgetConfig, FileChange, PRContext, PriorReviewContext, RepoConfig, ReviewComment } from "./types.js"; import { buildProvider, ProviderSpec } from "./ai/provider-factory.js"; import { FailoverProvider } from "./ai/failover.js"; import { isAiTimeoutError } from "./ai/timeout.js"; @@ -41,7 +41,7 @@ import { runWithCostContext, getCostContext, setCostReviewId, flushCostEvents } import { isPauseAll, resolveProfileOverride, resolveMaxFilesOverride } from "./settings/overrides.js"; import { reviewQueue, type ReviewHandle } from "./realtime/queue.js"; import { buildGraphContext } from "./graph-context.js"; -import { applyDiffBudget } from "./ai/diff-budget.js"; +import { applyDiffBudget, resolveDiffBudget } from "./ai/diff-budget.js"; /** Map a PR chat command to the cost-attribution kind for its AI calls. */ function costKindForCommand(type: string): string { @@ -129,6 +129,67 @@ export function partitionFilesForReview( return { allFiles: files, filesToReview, currentFileShas, filesSkippedSimilar, filesSkippedTrivial }; } +/** + * Package the already-reviewed part of an incremental review's PR as read-only + * prompt context, sized to whatever review budget the delta left unspent. + * + * `usedChars` is what the prompt has already committed to (the budgeted delta + * plus the graph related-context section). Prior context is strictly secondary: + * it never displaces the diff actually under review, and when there isn't room + * for even one more file it degrades to naming the files rather than pushing + * the prompt past `per_review_chars`. Naming alone is most of the value — the + * false "you never implemented this" finding comes from the model believing + * those files aren't in the PR at all. + */ +export function buildPriorReviewContext(params: { + /** Files in this PR already reviewed on an earlier commit. */ + files: FileChange[]; + cfg: DiffBudgetConfig | undefined; + usedChars: number; +}): PriorReviewContext | undefined { + const { files, cfg, usedChars } = params; + if (files.length === 0) return undefined; + + const resolved = resolveDiffBudget(cfg); + // Budgeting off ⇒ the caller has opted out of size limits entirely; send the + // patches as-is, exactly as the primary diff is sent. + if (!resolved.enabled) return { files }; + + const headroom = resolved.perReviewChars - usedChars; + // applyDiffBudget floors its per-review budget at per_file_chars (it never + // sends an empty diff), so anything below that floor would overshoot rather + // than trim. Name the files instead. + if (headroom < resolved.perFileChars) return { files, namesOnly: true }; + + return { + files, + budget: applyDiffBudget( + files.map((f) => ({ filename: f.filename, patch: f.patch })), + { ...cfg, per_review_chars: headroom }, + ), + }; +} + +/** + * Drop inline findings the model raised against a context-only file. + * + * The prior-review files are in the prompt to be READ, not reviewed — they were + * reviewed on an earlier commit and their threads already exist. The prompt says + * so; this is the deterministic backstop, so a model that comments on them + * anyway can't turn every push into a re-litigation of the whole branch. + * PR-level findings (no path) are never touched — those are whole-PR by + * definition, and reasoning about the whole PR is exactly why the context is + * there. + */ +export function dropContextOnlyFindings( + comments: ReviewComment[], + prior: PriorReviewContext | undefined, +): ReviewComment[] { + if (!prior || prior.files.length === 0) return comments; + const contextOnly = new Set(prior.files.map((f) => f.filename)); + return comments.filter((c) => !c.path || !contextOnly.has(c.path)); +} + const WALKTHROUGH_MARKER = ""; const WALKTHROUGH_START = ""; const WALKTHROUGH_END = ""; @@ -779,6 +840,37 @@ export class Reviewer { ); } + // Incremental runs: carry the rest of the PR into the review prompt as + // read-only context. context.files is the delta, but the title and + // description below it describe the whole branch — a model given only the + // slice reads every earlier commit's work as never written and reports + // the feature as unimplemented (mk7luke/atlas-timeclock#89: a docs-only + // follow-up commit flipped an approved branch to REQUEST_CHANGES over + // four files that had been reviewed a week earlier). PR #81 fixed this + // for the drift pass; the main review call had the same blind spot. + // Sized against what the delta + related context already spent, so it + // never displaces the diff actually under review. + const alreadyReviewedFiles = allReviewableFiles.filter((f) => + filesSkippedSimilar.includes(f.filename), + ); + const priorReview = buildPriorReviewContext({ + files: alreadyReviewedFiles, + cfg: repoConfig.reviews?.diff_budget, + usedChars: diffBudget.totalSentChars + graphContext.relatedContextMarkdown.length, + }); + if (priorReview) { + context.priorReview = priorReview; + log.info( + { + priorFiles: priorReview.files.length, + namesOnly: priorReview.namesOnly === true, + omitted: priorReview.budget?.filesOmitted.length ?? 0, + sentChars: priorReview.budget?.totalSentChars ?? 0, + }, + "Attached already-reviewed PR files to the review prompt as context", + ); + } + const relevantGuidelines = getRelevantGuidelines(allGuidelines, filenames); const linkedIssues = issueNumbers.length > 0 ? await fetchLinkedIssues(octokit, owner, repo, issueNumbers) @@ -847,10 +939,27 @@ export class Reviewer { const walkthroughEnabled = repoConfig.reviews?.walkthrough?.enabled !== false; const summaryEnabled = repoConfig.reviews?.high_level_summary !== false; + // The walkthrough describes the pull request AS A WHOLE — it upserts one + // comment and rewrites the PR-description summary table on every push, so + // generating it from the incremental slice replaces a whole-PR summary + // with one covering only the newest commit. Same reason drift gets the + // full set. Full reviews are untouched: their context already is the + // whole PR, so they keep the exact prompt (and budget) they had. + const wholePRBudget = + alreadyReviewedFiles.length > 0 + ? applyDiffBudget( + allReviewableFiles.map((f) => ({ filename: f.filename, patch: f.patch })), + repoConfig.reviews?.diff_budget, + ) + : null; + const walkthroughContext: PRContext = wholePRBudget + ? { ...context, files: allReviewableFiles, diffBudget: wholePRBudget } + : context; + const [reviewResult, walkthroughResult] = await Promise.all([ this.ai.review(context, repoConfig, knowledgeLearnings), walkthroughEnabled || summaryEnabled - ? this.ai.generateWalkthrough(context, repoConfig) + ? this.ai.generateWalkthrough(walkthroughContext, repoConfig) : Promise.resolve(null), ]); @@ -864,6 +973,19 @@ export class Reviewer { if (signal.aborted) return; + // Enforce the "context only" contract on the prior-review files before + // anything else consumes the findings. + if (context.priorReview) { + const kept = dropContextOnlyFindings(reviewResult.comments, context.priorReview); + if (kept.length !== reviewResult.comments.length) { + log.info( + { dropped: reviewResult.comments.length - kept.length }, + "Dropped findings raised against already-reviewed context files", + ); + reviewResult.comments = kept; + } + } + // Second, cheap verification pass over the AI's OWN findings: ask the // model to cite the diff line(s) that substantiate each finding and drop // the ones it can't. One extra batched call; skipped entirely when the @@ -993,8 +1115,18 @@ export class Reviewer { } } - // Compute insights (risk, coverage, split suggestion) before posting - const coverage = assessCoverage(context.files); + // Compute insights (risk, coverage, split suggestion) before posting. + // + // These describe the PULL REQUEST, not this review pass: they render + // inside the whole-PR walkthrough and feed the sticky status, and every + // push replaces them. Computing them from the incremental delta reports + // the branch wrong — "🔴 production code added with no test changes" for + // a PR whose tests landed two commits ago, a risk score that resets to + // near-zero on a one-line follow-up. Same reasoning as the walkthrough + // and drift: whole-PR questions get the whole PR. Identical to + // context.files on a full review. + const wholePRFiles = walkthroughContext.files; + const coverage = assessCoverage(wholePRFiles); // Context-aware severity calibration: nudge each finding's severity to // reflect real risk — escalate in high-fan-in (blast-radius) files and in @@ -1066,10 +1198,14 @@ export class Reviewer { // catch below would swallow drift entirely). let driftFindings: Awaited> = []; try { - const driftBudget = applyDiffBudget( - allReviewableFiles.map((f) => ({ filename: f.filename, patch: f.patch })), - repoConfig.reviews?.diff_budget, - ); + // Same full-set budget the walkthrough used when this is an incremental + // run (identical inputs, so reusing it just avoids recomputing it). + const driftBudget = + wholePRBudget ?? + applyDiffBudget( + allReviewableFiles.map((f) => ({ filename: f.filename, patch: f.patch })), + repoConfig.reviews?.diff_budget, + ); driftFindings = await detectDescriptionDrift({ ai: this.ai, context: { ...context, files: allReviewableFiles, diffBudget: driftBudget }, @@ -1131,7 +1267,9 @@ export class Reviewer { ); } const risk = assessRisk({ - files: context.files, + // Size / high-risk-path factors describe the whole PR; the findings + // factors come from this pass, which is what the sticky status reports. + files: wholePRFiles, review: reviewResult, effortEstimate: walkthroughResult?.effortEstimate, hasNewTests: coverage.testAdditions > 0, @@ -1274,13 +1412,15 @@ export class Reviewer { // PR splitting heuristic const cohorts = walkthroughResult.cohorts ?? []; - const totalLines = context.files.reduce((s, f) => s + f.additions + f.deletions, 0); + // "Is this PR too big to review in one go?" is a question about the PR, + // so it counts every file on the branch — not just this push's. + const totalLines = wholePRFiles.reduce((s, f) => s + f.additions + f.deletions, 0); if ( cohorts.length > 0 && shouldSuggestSplit({ cohortCount: cohorts.length, effortEstimate: walkthroughResult.effortEstimate, - fileCount: context.files.length, + fileCount: wholePRFiles.length, totalChangedLines: totalLines, }) ) { diff --git a/src/types.ts b/src/types.ts index 8e16bd3..2eaa435 100644 --- a/src/types.ts +++ b/src/types.ts @@ -453,6 +453,38 @@ export interface PRContext { * See src/ai/diff-budget.ts. */ diffBudget?: DiffBudgetResult; + /** + * Incremental reviews only: the part of THIS PR that was already reviewed on + * an earlier commit, handed to the review prompt as read-only context. See + * PriorReviewContext. + */ + priorReview?: PriorReviewContext; +} + +/** + * The already-reviewed remainder of an incremental review's pull request. + * + * On a synchronize push `PRContext.files` is trimmed to the delta, but the + * title and description still describe the whole branch — so a model shown the + * slice alone reliably concludes that the feature the description promises was + * never implemented. This carries the rest of the PR into the prompt as + * context: never commented on, only used to judge the new commit against the + * complete change set. + */ +export interface PriorReviewContext { + /** Files in this PR reviewed on an earlier commit, with their full patches. */ + files: FileChange[]; + /** + * Budget applied to those patches. Entries marked `omitted` are named in the + * prompt but not shown. Absent ⇒ send every patch in full (budgeting off). + */ + budget?: DiffBudgetResult; + /** + * Set when the delta and related-context sections already consumed the review + * budget: name the earlier files so the model knows they exist, but spend no + * budget on their diffs. + */ + namesOnly?: boolean; } // ─── AI Provider Interface ───────────────────────────────────── diff --git a/tests/e2e/runner.ts b/tests/e2e/runner.ts index 6273213..3c9d410 100644 --- a/tests/e2e/runner.ts +++ b/tests/e2e/runner.ts @@ -152,6 +152,20 @@ function evalExpectations( } } + if (exp.reviewBodyNotContains) { + const botBodies = fromBot(data.reviews) + .map((r) => r.body ?? "") + .filter(Boolean); + for (const needle of exp.reviewBodyNotContains) { + const offender = botBodies.find((b) => b.includes(needle)); + results.push({ + name: `no review body contains "${needle}"`, + passed: !offender, + detail: offender ? `found in a review body` : `absent from ${botBodies.length} review body/bodies`, + }); + } + } + if (exp.walkthroughContains) { for (const needle of exp.walkthroughContains) { const ok = !!data.walkthrough && data.walkthrough.includes(needle); diff --git a/tests/e2e/scenarios/incremental-full-pr.ts b/tests/e2e/scenarios/incremental-full-pr.ts new file mode 100644 index 0000000..1e10c18 --- /dev/null +++ b/tests/e2e/scenarios/incremental-full-pr.ts @@ -0,0 +1,61 @@ +import type { Scenario } from "../types.js"; + +// Regression: mk7luke/atlas-timeclock#89. A feature branch is reviewed, then a +// small follow-up commit lands that only touches a doc file. The incremental +// re-review used to see just that file while still reading a description of the +// whole feature — and reported the already-merged-into-the-branch backend work +// as never implemented, flipping an approved PR to CHANGES_REQUESTED. +// +// The follow-up commit here is deliberately docs-only, exactly as in #89: the +// bot must reconcile the description against the whole branch, not the delta. +export const scenario: Scenario = { + name: "incremental-full-pr", + description: + "Feature PR, then a docs-only follow-up commit. The incremental re-review must not claim the feature described in the PR body is missing from the diff.", + prTitle: "Add a retry helper with backoff", + prBody: + "Adds `withRetry()` in `src/util/retry.ts` — exponential backoff with a configurable attempt cap — " + + "and wires it into the fetch helper in `src/util/fetch-json.ts` so transient 5xx responses are retried. " + + "Includes the default backoff table and the caller-facing options type.", + files: [ + { + path: "src/util/retry.ts", + content: `export interface RetryOptions {\n attempts?: number;\n baseMs?: number;\n}\n\nexport async function withRetry(fn: () => Promise, opts: RetryOptions = {}): Promise {\n const attempts = opts.attempts ?? 3;\n const baseMs = opts.baseMs ?? 100;\n let lastErr: unknown;\n for (let i = 0; i < attempts; i++) {\n try {\n return await fn();\n } catch (err) {\n lastErr = err;\n await new Promise((r) => setTimeout(r, baseMs * 2 ** i));\n }\n }\n throw lastErr;\n}\n`, + }, + { + path: "src/util/fetch-json.ts", + content: `import { withRetry } from "./retry.js";\n\nexport async function fetchJson(url: string): Promise {\n return withRetry(async () => {\n const res = await fetch(url);\n if (res.status >= 500) throw new Error("transient " + res.status);\n return (await res.json()) as T;\n });\n}\n`, + }, + ], + postPrActions: [ + // Let the first review land and embed its state blob. + { type: "wait", ms: 60_000 }, + { + type: "push", + commitMessage: "Document the retry helper", + files: [ + { + path: "docs/retry.md", + content: `# Retry helper\n\n\`withRetry()\` retries a promise-returning function with exponential backoff.\n\n- \`attempts\` — total tries, default 3.\n- \`baseMs\` — first delay in ms, doubling per attempt, default 100.\n`, + }, + ], + }, + ], + waitFor: { + walkthrough: true, + review: true, + botIssueCommentsAtLeast: 4, + timeoutMs: 360_000, + }, + expect: { + // The incremental review sees only docs/retry.md as changed, so it must be + // told the rest of the PR is already in place. + reviewBodyContains: ["Reviewing files that changed from"], + // The exact phrasing the old bug produced. None of these may appear. + reviewBodyNotContains: [ + "missing from the diff", + "is not implemented", + "does not exist in the diff", + ], + }, +}; diff --git a/tests/e2e/scenarios/index.ts b/tests/e2e/scenarios/index.ts index fde99ed..7b501d6 100644 --- a/tests/e2e/scenarios/index.ts +++ b/tests/e2e/scenarios/index.ts @@ -12,6 +12,7 @@ import { scenario as pathFilter } from "./path-filter.js"; import { scenario as chatQuestion } from "./chat-question.js"; import { scenario as linkedIssue } from "./linked-issue.js"; import { scenario as incrementalReview } from "./incremental-review.js"; +import { scenario as incrementalFullPr } from "./incremental-full-pr.js"; import { scenario as trivialSkip } from "./trivial-skip.js"; import { scenario as riskAndCoverage } from "./risk-and-coverage.js"; import { scenario as chatTldr } from "./chat-tldr.js"; @@ -59,6 +60,7 @@ export const ALL_SCENARIOS: Scenario[] = [ linkedIssue, trivialSkip, incrementalReview, + incrementalFullPr, riskAndCoverage, secretScanner, mergeMarker, diff --git a/tests/e2e/types.ts b/tests/e2e/types.ts index 4302ba2..80fc9d3 100644 --- a/tests/e2e/types.ts +++ b/tests/e2e/types.ts @@ -33,6 +33,12 @@ export type Scenario = { expect?: { reviewState?: "CHANGES_REQUESTED" | "COMMENTED" | "APPROVED"; reviewBodyContains?: string[]; + /** + * Needles that must appear in NO bot review body. For regressions whose + * symptom is the bot saying something it shouldn't (e.g. reporting an + * earlier commit's work as missing on an incremental re-review). + */ + reviewBodyNotContains?: string[]; inlineCommentsContain?: Array<{ pathContains?: string; bodyContains: string[] }>; walkthroughContains?: string[]; issueCommentContains?: string[]; diff --git a/tests/unit/incremental-scope.test.ts b/tests/unit/incremental-scope.test.ts new file mode 100644 index 0000000..dfb8652 --- /dev/null +++ b/tests/unit/incremental-scope.test.ts @@ -0,0 +1,220 @@ +import { describe, it, expect } from "vitest"; +import { buildReviewPrompt } from "../../src/ai/prompt.js"; +import { + buildPriorReviewContext, + dropContextOnlyFindings, + partitionFilesForReview, +} from "../../src/reviewer.js"; +import type { FileChange, PRContext, ReviewComment } from "../../src/types.js"; + +// Regression coverage for "DiffSentry ignores the rest of the PR once you push +// another commit". Companion to drift-scope.test.ts, which fixed the same class +// of false positive for the *drift* pass only. +// +// On a synchronize push the reviewer trims context.files to the delta, then +// hands the model that slice together with the WHOLE-PR title and description — +// and a system prompt that explicitly asks for a PR-level finding when "a +// claimed change is missing". The model duly reports that the feature the +// description promises was never implemented. Observed on +// mk7luke/atlas-timeclock#89: a docs-only follow-up commit turned an approved +// feature branch into "Claimed overlay implementation is missing from the +// diff." + REQUEST_CHANGES, naming the four files that had been reviewed and +// accepted a week earlier. + +function file(filename: string, marker = "changed"): FileChange { + return { + filename, + status: "modified", + patch: `@@ -1,2 +1,3 @@\n context\n+${marker} ${filename}\n`, + additions: 1, + deletions: 0, + }; +} + +function ctx(files: FileChange[], over: Partial = {}): PRContext { + return { + owner: "o", + repo: "r", + pullNumber: 89, + title: "Stamp date/time onto served selfies", + description: + "Adds config.selfie_timestamp_overlay, images.render_timestamp_overlay(), " + + "stamped responses in the selfies router, and tests.", + baseBranch: "main", + headBranch: "feat", + headSha: "deadbee", + files, + ...over, + }; +} + +describe("incremental review — the model sees the rest of the PR", () => { + it("shows the already-reviewed files' diffs as read-only context", () => { + const prompt = buildReviewPrompt( + ctx([file(".env.example")], { + priorReview: { files: [file("backend/app/images.py", "render_timestamp_overlay")] }, + }), + ); + + // The earlier commit's actual diff reaches the model... + expect(prompt.user).toContain("render_timestamp_overlay backend/app/images.py"); + // ...labelled so it is never mistaken for something to comment on again. + expect(prompt.user).toContain("Already reviewed earlier in this PR"); + expect(prompt.user).toMatch(/do NOT comment on these/i); + }); + + it("tells the model the diff is partial and absence proves nothing", () => { + const prompt = buildReviewPrompt( + ctx([file(".env.example")], { + priorReview: { files: [file("backend/app/config.py")] }, + }), + ); + + expect(prompt.user).toMatch(/incremental review/i); + expect(prompt.user).toMatch(/not evidence/i); + // The description covers the whole branch, not just the newest commit. + expect(prompt.user).toMatch(/ENTIRE pull request/i); + }); + + it("still names the earlier files when there is no budget headroom for their diffs", () => { + const prompt = buildReviewPrompt( + ctx([file(".env.example")], { + priorReview: { + files: [file("backend/app/images.py", "render_timestamp_overlay")], + namesOnly: true, + }, + }), + ); + + expect(prompt.user).toContain("backend/app/images.py"); + // Named, not shown — the patch body must not be spent. + expect(prompt.user).not.toContain("render_timestamp_overlay backend/app/images.py"); + expect(prompt.user).toMatch(/not evidence/i); + }); + + it("honors the prior-context budget: truncated patches and omitted files are labelled", () => { + const prior = [file("a.py", "kept"), file("b.py", "dropped")]; + const budget = buildPriorReviewContext({ + files: prior, + cfg: { per_file_chars: 60, per_review_chars: 200 }, + usedChars: 0, + }); + + const prompt = buildReviewPrompt(ctx([file("c.py")], { priorReview: budget })); + + // Whatever the budget dropped is named rather than silently vanishing — + // a file the model can't see must never read as a file that doesn't exist. + for (const f of prior) expect(prompt.user).toContain(f.filename); + }); + + it("adds nothing to a full review's prompt", () => { + const prompt = buildReviewPrompt(ctx([file("src/a.ts")])); + + expect(prompt.user).not.toContain("Already reviewed earlier in this PR"); + expect(prompt.user).not.toMatch(/incremental review/i); + }); + + it("forbids description-vs-diff findings when the diff shown is partial", () => { + const prompt = buildReviewPrompt(ctx([file("src/a.ts")])); + + // System-prompt guard: the "claimed change is missing" rule must carve out + // the case where the model was only shown part of the change set. + expect(prompt.system).toMatch(/partial/i); + }); +}); + +describe("context files are read, not re-reviewed", () => { + function finding(path: string, line = 3): ReviewComment { + return { path, line, side: "RIGHT", body: "b", title: "t", type: "issue", severity: "major" }; + } + + const prior = { files: [file("old.ts"), file("older.ts")] }; + + it("drops inline findings raised against an already-reviewed file", () => { + const kept = dropContextOnlyFindings( + [finding("new.ts"), finding("old.ts"), finding("older.ts")], + prior, + ); + + expect(kept.map((c) => c.path)).toEqual(["new.ts"]); + }); + + it("keeps PR-level findings — whole-PR reasoning is the point of the context", () => { + const kept = dropContextOnlyFindings([finding("", 0), finding("old.ts")], prior); + + expect(kept).toHaveLength(1); + expect(kept[0].path).toBe(""); + }); + + it("is a no-op on a full review", () => { + const comments = [finding("a.ts"), finding("b.ts")]; + + expect(dropContextOnlyFindings(comments, undefined)).toBe(comments); + }); +}); + +describe("buildPriorReviewContext", () => { + const cfg = { per_file_chars: 1000, per_review_chars: 10_000 }; + + it("returns nothing when every file in the PR is in this review", () => { + expect(buildPriorReviewContext({ files: [], cfg, usedChars: 0 })).toBeUndefined(); + }); + + it("budgets the earlier patches against the headroom the delta left", () => { + const result = buildPriorReviewContext({ + files: [file("a.ts"), file("b.ts")], + cfg, + usedChars: 500, + }); + + expect(result?.files.map((f) => f.filename)).toEqual(["a.ts", "b.ts"]); + expect(result?.namesOnly).toBeFalsy(); + expect(result?.budget).toBeDefined(); + }); + + it("degrades to names-only rather than overrunning the review budget", () => { + // The delta plus related context already consumed nearly the whole budget: + // there isn't room for even one more file, so send names, not patches. + const result = buildPriorReviewContext({ + files: [file("a.ts")], + cfg, + usedChars: 9_500, + }); + + expect(result?.namesOnly).toBe(true); + expect(result?.budget).toBeUndefined(); + }); + + it("sends full patches when diff budgeting is disabled", () => { + const result = buildPriorReviewContext({ + files: [file("a.ts")], + cfg: { enabled: false }, + usedChars: 1_000_000, + }); + + expect(result?.namesOnly).toBeFalsy(); + expect(result?.budget).toBeUndefined(); + }); + + it("pairs with partitionFilesForReview: prior files are exactly the skipped-similar set", () => { + const files = [file("backend/app/images.py"), file("backend/app/config.py"), file(".env.example")]; + const first = partitionFilesForReview(files, "full", undefined); + const priorShas = { + "backend/app/images.py": first.currentFileShas["backend/app/images.py"], + "backend/app/config.py": first.currentFileShas["backend/app/config.py"], + }; + + const second = partitionFilesForReview(files, "incremental", priorShas); + const prior = buildPriorReviewContext({ + files: second.allFiles.filter((f) => second.filesSkippedSimilar.includes(f.filename)), + cfg, + usedChars: 0, + }); + + expect(second.filesToReview.map((f) => f.filename)).toEqual([".env.example"]); + expect(prior?.files.map((f) => f.filename)).toEqual([ + "backend/app/images.py", + "backend/app/config.py", + ]); + }); +});