Judge the whole PR on every push, not just the newest commit

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).

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.`;
 

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";
@@ -25,7 +25,7 @@
 import { encodeState, encodeStateRef, extractState, isTrivialPatch, WalkthroughState } from "./walkthrough-state.js";
 import { assessRisk, renderRiskBlock, assessCoverage, renderCoverageBlock, shouldSuggestSplit, renderSplitSuggestion, renderConfidenceAggregate, computeReviewerDeltas, renderReviewerDeltaBlock, calibrateSeverities, resolveSeverityCalibration, renderSeverityCalibrationBlock, type CalibrationResult } from "./insights.js";
 import { suggestReviewersFromBlame, renderSuggestedReviewers, combineReviewers, renderCombinedReviewers } from "./blame-reviewers.js";
 import { loadCodeowners, ownersForFiles, renderCodeownersBlock } from "./codeowners.js";
 import { findPriorBotThreadsForPaths, renderPriorDiscussionsBlock, diffWithOtherPR, renderDiffPRReply } from "./cross-pr.js";
 import { renderStickyStatus, STICKY_MARKER } from "./sticky-status.js";
 import { recordRepo, recordPR, recordReview, recordFindings, recordPatternHits, recordIssue, recordIssueAction, getSuppressedFingerprints, listCustomRulesForRepo, deleteReviewJob, getWalkthroughState, saveWalkthroughState } from "./storage/dao.js";
@@ -41,7 +41,7 @@
 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 @@
   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 = "<!-- DiffSentry Walkthrough -->";
 const WALKTHROUGH_START = "<!-- walkthrough_start -->";
 const WALKTHROUGH_END = "<!-- walkthrough_end -->";
@@ -779,6 +840,37 @@
         );
       }
 
+      // 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)
@@ -791,8 +883,8 @@
       // (Guidelines and issues are injected via the prompt builder's learnings param)
       const knowledgeLearnings = [...relevantLearnings];
       // Add guideline content as synthetic learnings
       const guidelinesPrompt = formatGuidelinesForPrompt(relevantGuidelines);
       const issuesPrompt = formatIssuesForPrompt(linkedIssues);
 
       // Inject language preference
       if (repoConfig.language) {
@@ -847,10 +939,27 @@
       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 @@
 
       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 @@
         }
       }
 
-      // 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 @@
       // catch below would swallow drift entirely).
       let driftFindings: Awaited<ReturnType<typeof detectDescriptionDrift>> = [];
       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 @@
         );
       }
       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 @@
 
         // 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,
           })
         ) {

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 ─────────────────────────────────────