review: line-number-annotated staged diffs #256

github-actions · 2026-07-13T19:52:38Z

suggestion (non-blocking): This PR makes snapped a required field on SampleRun (aggregate.ts:59), but the sampleRun builder in eval/aggregate.test.ts (not touched by this PR) omits it, so this snapped += run.snapped evaluates to NaN and renderAggregateMarkdown renders | Findings anchor-snapped | NaN | ... | for reports built from that fixture. Production reports are safe (parseArm defaults it to 0), so this is test-fixture-only. Add snapped: 0 to the builder's defaults. Relatedly, every fixture exercises only the zero branch, so the populated-count path (parse at aggregate.ts:164 plus this sum) is untested; a fixture with a non-empty snappedByProvenance would cover it.

Lower-confidence observations

workflows/review/lib/diff.ts:344 — - lines are annotated with the OLD-file (LEFT) number, while the "take anchor.line from the printed number — never count" instruction (review.md:1597/1701) isn't qualified for - lines. The schema line ("line is a RIGHT-side number", review.md:1662) mitigates this, but consider stating explicitly that a deletion finding anchors on the adjacent RIGHT-side context/added number, not the - line's printed OLD number — otherwise a side-less deletion anchor can be dropped by the RIGHT-side provenance gate where OLD/NEW diverge.

workflows/review/review.md:1702 — the new "strip the NNN| prefix" obligation for quoted code / suggested_patch has no deterministic backstop (unlike anchor-snap for anchors); suggested_patch is validated only as a non-empty string, so a leaked prefix would flow verbatim into a posted suggestion block. Consider a code-level strip in Phase 3.

github-actions · 2026-07-13T19:52:38Z

suggestion (non-blocking): snappedTotal and the "Findings anchor-snapped" report row are only ever exercised at zero — produceHit/produceMiss never trigger a gate snap and no test asserts a non-zero total, so a wrong-field or missing-accumulation bug would render 0 and pass. A runArm case whose candidate anchor-snaps (as live-match.test.ts already constructs) asserting perCase[0].snapped === 1 and the report row would close this.

-Original file line number
+Diff line change
@@ -0,0 +1,5 @@
+    ---
+    "review": minor
+    ---
+    Line-number-annotated staged diffs: remove the anchor mis-counting at the source. The mis-anchor pathology anchor-snap repairs downstream (reviewers counting unified-diff text lines instead of file lines) exists because the staged diff makes the model count; the staging now prints the real number on every content line so anchors are read off the page, never counted. A new deterministic `annotateDiffLineNumbers` (`lib/diff.ts`) prefixes each hunk content line with its line number (`+`/context lines carry the NEW-file RIGHT-side number, `-` lines the OLD-file LEFT-side number) while keeping the diff marker in column one, so annotated text still splits into file sections. The provenance CLI writes `full-stripped-annotated.diff` beside the raw stripped diff, and a new `annotate <in> <out>` subcommand produces `pr-annotated.diff` after Phase 1 builds `pr.diff` (the scoped and flip-gated depths refresh the annotated copies the same way). Every finding-producing reviewer (correctness, skill-auditor, conventions, the four whole-change reviewers, and all eleven specialist lenses via the shared disciplines block) now reads the annotated copy, takes `anchor.line` from the printed number, and strips the prefix when quoting code or authoring a `suggested_patch`. Everything that PARSES a diff keeps reading the raw files: provenance, re-review hunk fingerprints (whose signatures must not shift), scoped staging, and pattern-triage/claim-validator are untouched. The eval stages the annotated siblings for both arms unconditionally, and only a review.md version that names them reads them, so the A/B against a pre-annotation baseline is a pure prompt delta with no staging flag. The measurement instrument rides along: per-case anchor-snap counts (`perCase.snapped`) in the arm report and a pooled "Findings anchor-snapped" row in the aggregate, version-tolerant of older artifacts — if annotation works, candidate-arm snaps fall to zero because anchors arrive correct, with the anchor-snap gate remaining as the deterministic backstop.

-Original file line number
+Diff line change
@@ Expand Up / @@ -130,7 +130,14 @@ reviewer, was wrong). @@
       prompts carry the rule, both arms snap and the A/B is back to measuring
       prompt deltas alone. Snaps are recorded per run (`snappedByProvenance`
       in the report's runs; `out/snapped.json` in production artifacts) for
-      audit. Each record carries the original and snapped anchors, so the
+      audit, counted per case in the report (`perCase.snapped`), and pooled in
+      the aggregate ("Findings anchor-snapped"). The snap count is the direct
+      anchor-fidelity observable: the line-number-annotated staged diffs exist
+      to drive it to zero at the source, with the snap as backstop. Staging
+      writes the annotated copies (`pr-annotated.diff`,
+      `full-stripped-annotated.diff`) for both arms unconditionally; only a
+      review.md version that names them reads them, so annotation A/Bs are
+      pure prompt deltas with no staging flag. Each record carries the original and snapped anchors, so the
       window class is derivable: a from/to distance within 3 is a near-miss
       snap, anything larger is the past-EOF overflow class (the observed
       diff-text-counting pathology). Reviewing audited snaps over real PRs is
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -51,6 +51,12 @@ export type SampleRun = { @@
         missedSpecs: {specKey: string; droppedBy?: string}[];
         unmatchedPosted: number;
         posted: number;
+        /**
+         * Findings the provenance gate anchor-snapped (0 for reports predating
+         * the field). The anchor-fidelity observable: a prompt fix that anchors
+         * correctly at the source drives this to zero.
+         */
+        snapped: number;
     };
     /** One arm-run: a single pass of one arm over its cases. */
@@ Expand Down Expand Up / @@ -155,6 +161,9 @@ const parseArm = ( @@
                 missedSpecs,
                 unmatchedPosted: unmatched,
                 posted: asNumber(match["postedCount"]),
+                snapped: Array.isArray(result["snappedByProvenance"])
+                    ? result["snappedByProvenance"].length
+                    : 0,
             };
         });
         const judge = raw["judge"];
@@ Expand Down Expand Up / @@ -291,6 +300,8 @@ export type ArmAggregate = { @@
             noise: RateStat;
             trueMisses: number;
             foundButDropped: Record<string, number>;
+            /** Total anchor-snapped findings across the arm's case-runs. */
+            snapped: number;
             usd: number;
         };
         /** Mean of per-sample judge means, when any sample carried one. */
@@ Expand Down Expand Up / @@ -352,6 +363,7 @@ const aggregateArm = ( @@
         let caseRuns = 0;
         let unmatched = 0;
         let posted = 0;
+        let snapped = 0;
         let usd = 0;
         const judgeMeans: number[] = [];
@@ Expand All / @@ -374,6 +386,7 @@ const aggregateArm = ( @@
                 }
                 unmatched += run.unmatchedPosted;
                 posted += run.posted;
+                snapped += run.snapped;
                 const spec = (key: string) => {
                     const s = entry.specs.get(key) ?? {
                         caught: 0,
@@ Expand Down Expand Up / @@ -455,6 +468,7 @@ const aggregateArm = ( @@
                 noise: rateStat(unmatched, posted),
                 trueMisses,
                 foundButDropped,
+                snapped,
                 usd,
             },
             ...(judgeMeans.length > 0
@@ Expand Down Expand Up @@
             `| Misses (true / dropped) | ${dropSummary(
                 baseline,
             )} |  | ${dropSummary(candidate)} |  |`,
+            `| Findings anchor-snapped | ${baseline.pooled.snapped} |  | ${candidate.pooled.snapped} |  |`,
             ...(baseline.judgeMeanQuality !== undefined &&
             candidate.judgeMeanQuality !== undefined
                 ? [
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -44,6 +44,13 @@ export type ArmRunReport = { @@
             expected: string;
             caught: number;
             missed: string[];
+            /**
+             * Findings the provenance gate anchor-snapped this run. The direct
+             * observable for anchor fidelity: a prompt change that fixes
+             * anchoring at the source (line-number-annotated diffs) shows up
+             * here as candidate-arm snaps falling to zero.
+             */
+            snapped: number;
             /** `<agent>: <reason>` per failed agent (diagnosable from the report). */
             failedAgents: string[];
             /** Present iff the case is an open-PR (rereview) case. */
@@ Expand Down Expand Up @@
         return lines.join("\n");
     };
+    /** Total anchor-snaps across an arm's case runs (see `perCase.snapped`). */
+    const snappedTotal = (arm: ArmRunReport): number =>
+        arm.perCase.reduce((sum, c) => sum + c.snapped, 0);
     /** `caseId:specKey` -> drop bucket, for every found-but-dropped miss. */
     const dropClassByKey = (arm: ArmRunReport): Map<string, string> => {
         const map = new Map<string, string>();
@@ Expand Down Expand Up @@
                 String(dropClassByKey(baseline).size),
                 String(dropClassByKey(candidate).size),
             ),
+            row(
+                "Findings anchor-snapped",
+                String(snappedTotal(baseline)),
+                String(snappedTotal(candidate)),
+            ),
             "",
         ];
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -187,6 +187,7 @@ export const runArm = async ( @@
                 expected: corpusCase.expected.verdict,
                 caught: match.caught.length,
                 missed: match.missed,
+                snapped: result.snappedByProvenance.length,
                 failedAgents: produced.perAgent
                     .filter((a) => a.failed !== undefined)
                     .map((a) => `${a.name}: ${a.failed}`),
@@ Expand Down @@

Navigation Menu

Uh oh!

review: line-number-annotated staged diffs #256

Uh oh!

Diff view

Diff view

There are no files selected for viewing

github-actions Bot Jul 13, 2026

Uh oh!

github-actions Bot Jul 13, 2026

Uh oh!

Uh oh!

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up / @@ -82,6 +82,17 @@ describe("stageCase", () => { @@
             expect(read("/stage/context/pr.diff")).toBe(DIFF);
             expect(read("/stage/context/full-stripped.diff")).toBe(DIFF);
+            // The annotated siblings are staged unconditionally; only a
+            // review.md version that names them reads them, so an A/B against a
+            // pre-annotation baseline is a pure prompt delta.
+            const annotated = read("/stage/context/pr-annotated.diff");
+            expect(annotated).toContain("+  1| const a = 2;");
+            expect(annotated).toContain("-  1| const a = 1;");
+            expect(annotated).toContain("   2| export {a};");
+            expect(read("/stage/context/full-stripped-annotated.diff")).toBe(
+                annotated,
+            );
             const files = JSON.parse(read("/stage/context/files.json"));
             expect(files).toEqual([
                 {path: "src/a.ts", status: "modified", hasPatch: true},
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -13,6 +13,9 @@ @@
      *                                       generated files to strip)
      *   <dest>/context/pr.diff            = full.diff (no pattern-triage pass:
      *                                       every changed file is a review file)
+     *   <dest>/context/full-stripped-annotated.diff, pr-annotated.diff
+     *                                     line-number-annotated copies (read by
+     *                                     review.md versions that name them)
      *   <dest>/context/files.json         path/status/hasPatch per changed file
      *   <dest>/context/review-files.json  = files.json entries (see pr.diff)
      *   <dest>/context/provenance.json    the diff's changed-line map
@@ Expand All / @@ -32,6 +35,7 @@ import { @@
         writeFileSync,
     } from "node:fs";
+    import {annotateDiffLineNumbers} from "../lib/diff";
     import {computeDiffProvenance} from "../lib/provenance";
     import {
         buildScopedDiff,
@@ Expand Down Expand Up / @@ -192,9 +196,15 @@ const stageRereview = ( @@
         if (plan.staging === "new-hunks") {
             const scoped = buildScopedDiff(currentDiff, anchorHunks);
+            const scopedAnnotated = annotateDiffLineNumbers(scoped);
             fs.writeFileSync(`${contextDir}/scoped.diff`, scoped);
             fs.writeFileSync(`${contextDir}/full-stripped.diff`, scoped);
             fs.writeFileSync(`${contextDir}/pr.diff`, scoped);
+            fs.writeFileSync(
+                `${contextDir}/full-stripped-annotated.diff`,
+                scopedAnnotated,
+            );
+            fs.writeFileSync(`${contextDir}/pr-annotated.diff`, scopedAnnotated);
         }
         return plan;
     };
@@ Expand Down Expand Up / @@ -226,10 +236,16 @@ export const stageCase = ( @@
         // The diff surfaces. Corpus diffs carry no generated files, so the
         // stripped diff equals the full one; with no pattern-triage pass, the
-        // review diff does too.
+        // review diff does too. The annotated siblings are staged for BOTH arms
+        // unconditionally: only a review.md version that names them reads them,
+        // so an A/B between a pre-annotation baseline and an annotated candidate
+        // is a pure prompt delta with no staging flag.
+        const annotated = annotateDiffLineNumbers(diff);
         fs.writeFileSync(`${contextDir}/full.diff`, diff);
         fs.writeFileSync(`${contextDir}/full-stripped.diff`, diff);
         fs.writeFileSync(`${contextDir}/pr.diff`, diff);
+        fs.writeFileSync(`${contextDir}/full-stripped-annotated.diff`, annotated);
+        fs.writeFileSync(`${contextDir}/pr-annotated.diff`, annotated);
         // files.json + review-files.json: path/status/hasPatch. `hasPatch` is
         // whether the diff carries a section for the path (the completeness
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -1,6 +1,7 @@
     import {describe, it, expect} from "vitest";
     import {
+        annotateDiffLineNumbers,
         computeChangedLines,
         countOrphanHunkLines,
         splitUnifiedDiff,
@@ Expand Down Expand Up / @@ -188,6 +189,82 @@ describe("countOrphanHunkLines", () => { @@
         });
     });
+    describe("annotateDiffLineNumbers", () => {
+        it("prefixes added and context lines with RIGHT-side numbers, removed with LEFT-side", () => {
+            const annotated = annotateDiffLineNumbers(GIT_DIFF).split("\n");
+            expect(annotated).toContain("  10| const a = 1;");
+            expect(annotated).toContain("- 11| const b = legacy(a);");
+            expect(annotated).toContain("+ 11| const b = modern(a);");
+            expect(annotated).toContain("+ 12| const c = b + 1;");
+            // Hunk 2 resumes at the header's stated positions.
+            expect(annotated).toContain("  41| cleanup();");
+            expect(annotated).toContain("- 41| releaseLock();");
+            expect(annotated).toContain("  42| done();");
+            // The new file numbers from 1.
+            expect(annotated).toContain("+  1| export const x = 1;");
+            expect(annotated).toContain("+  2| export const y = 2;");
+        });
+        it("passes headers, hunk headers, and no-newline markers through verbatim", () => {
+            const withMarker = [
+                "diff --git a/a.ts b/a.ts",
+                "--- a/a.ts",
+                "+++ b/a.ts",
+                "@@ -1 +1 @@",
+                "-x",
+                "\\ No newline at end of file",
+                "+y",
+                "\\ No newline at end of file",
+            ].join("\n");
+            const annotated = annotateDiffLineNumbers(withMarker).split("\n");
+            expect(annotated[0]).toBe("diff --git a/a.ts b/a.ts");
+            expect(annotated[1]).toBe("--- a/a.ts");
+            expect(annotated[3]).toBe("@@ -1 +1 @@");
+            expect(annotated[4]).toBe("-  1| x");
+            expect(annotated[5]).toBe("\\ No newline at end of file");
+            expect(annotated[6]).toBe("+  1| y");
+        });
+        it("keeps the diff marker in column one, so sections still split", () => {
+            const annotated = annotateDiffLineNumbers(GIT_DIFF);
+            expect(splitUnifiedDiff(annotated).map((s) => s.path)).toEqual(
+                splitUnifiedDiff(GIT_DIFF).map((s) => s.path),
+            );
+        });
+        it("does not annotate text after a hunk's stated extent (trailing lines)", () => {
+            const trailing = [
+                "--- a/a.ts",
+                "+++ b/a.ts",
+                "@@ -1 +1 @@",
+                "-x",
+                "+y",
+                "",
+            ].join("\n");
+            const annotated = annotateDiffLineNumbers(trailing).split("\n");
+            // The trailing empty line (a split artifact, outside the hunk's
+            // counted extent) stays empty instead of gaining a phantom number.
+            expect(annotated[annotated.length - 1]).toBe("");
+        });
+        it("widens the number column for large files and is empty-safe", () => {
+            const big = [
+                "--- a/a.ts",
+                "+++ b/a.ts",
+                "@@ -9998,3 +9998,3 @@",
+                " keep;",
+                "-old;",
+                "+new;",
+                " tail;",
+            ].join("\n");
+            const annotated = annotateDiffLineNumbers(big).split("\n");
+            expect(annotated).toContain("  9998| keep;");
+            expect(annotated).toContain("- 9999| old;");
+            expect(annotated).toContain("+ 9999| new;");
+            expect(annotateDiffLineNumbers("")).toBe("");
+        });
+    });
     describe("stripDiffFiles", () => {
         it("removes the named files' sections and keeps the rest verbatim", () => {
             const stripped = stripDiffFiles(GIT_DIFF, new Set(["src/new.ts"]));
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -286,3 +286,74 @@ export const stripDiffFiles = ( @@
             .map((section) => section.text);
         return kept.join("\n");
     };
+    /**
+     * Annotate a unified diff with explicit line numbers, for the model-read
+     * staged copies (`full-stripped-annotated.diff`, `pr-annotated.diff`).
+     *
+     * The observed anchor pathology is reviewers counting diff TEXT lines
+     * instead of file lines; printing the real number on every line removes the
+     * counting entirely. Format, per hunk content line:
+     *
+     *     `+  16| added line`      RIGHT-side (new file) line number
+     *     `   17| context line`    RIGHT-side (new file) line number
+     *     `-  12| removed line`    LEFT-side (old file) line number
+     *
+     * The diff marker stays in column one, so annotated text still splits into
+     * file sections ({@link splitUnifiedDiff} recognises the same headers and
+     * the same `+`/`-`/space first columns). Headers, hunk headers, and
+     * `\ No newline` markers pass through untouched. The annotated copy is for
+     * model eyes only: every code parser (provenance, fingerprints, scoped
+     * staging) keeps reading the raw diff.
+     */
+    export const annotateDiffLineNumbers = (diff: string): string => {
+        const out: string[] = [];
+        let oldLine = 0;
+        let newLine = 0;
+        /** Remaining old/new line counts of the hunk being consumed. */
+        let hunkOld = 0;
+        let hunkNew = 0;
+        let width = 3;
+        for (const line of diff.split("\n")) {
+            const hunk = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(line);
+            if (hunk !== null) {
+                oldLine = Number(hunk[1] ?? "1");
+                newLine = Number(hunk[3] ?? "1");
+                hunkOld = Number(hunk[2] ?? "1");
+                hunkNew = Number(hunk[4] ?? "1");
+                width = Math.max(
+                    width,
+                    String(Math.max(oldLine + hunkOld, newLine + hunkNew)).length,
+                );
+                out.push(line);
+                continue;
+            }
+            if (hunkOld <= 0 && hunkNew <= 0) {
+                // Outside hunk content (file headers, preamble, trailing text):
+                // pass through verbatim. The countdown, not a flag, decides —
+                // so a `--- ` header after an exhausted hunk is never mistaken
+                // for a removed line.
+                out.push(line);
+                continue;
+            }
+            if (line.startsWith("+")) {
+                out.push(`+${String(newLine).padStart(width)}| ${line.slice(1)}`);
+                newLine++;
+                hunkNew--;
+            } else if (line.startsWith("-")) {
+                out.push(`-${String(oldLine).padStart(width)}| ${line.slice(1)}`);
+                oldLine++;
+                hunkOld--;
+            } else if (line.startsWith("\\")) {
+                out.push(line);
+            } else {
+                out.push(` ${String(newLine).padStart(width)}| ${line.slice(1)}`);
+                oldLine++;
+                newLine++;
+                hunkOld--;
+                hunkNew--;
+            }
+        }
+        return out.join("\n");
+    };

Uh oh!

review: line-number-annotated staged diffs #256

Uh oh!

review: line-number-annotated staged diffs #256

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

github-actions Bot Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!