Skip to content

review: line-number-annotated staged diffs - #256

Merged
jwbron merged 81 commits into
mainfrom
jwbron/diff-line-annotation
Jul 13, 2026
Merged

review: line-number-annotated staged diffs#256
jwbron merged 81 commits into
mainfrom
jwbron/diff-line-annotation

Conversation

@jwbron

@jwbron jwbron commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #255 (stacked on it): remove the anchor mis-counting at the source, keeping #255's anchor-snap as the deterministic backstop.

Why

The mis-anchor pathology (reviewers counting unified-diff text lines instead of file lines: line 24 of an 18-line file, line 8 of a 3-line file) exists because the staged diff makes the model count. #255 repairs the damage at the gate; this PR removes the failure at its source: every content line of the model-read diff now carries its real line number, so anchors are read off the page, never counted. Zero output-token cost, no new model calls, no schema change; the only cost is a few percent more input tokens on the diff.

What

  • annotateDiffLineNumbers (lib/diff.ts, pure): + 16| added, 17| context (NEW-file numbers), - 12| removed (OLD-file number). The diff marker stays 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 gains an annotate <in> <out> subcommand review.md runs on pr.diff in Phase 1 (the scoped/flip-gated depths refresh the annotated copies the same way).
  • review.md points every finding-producing reviewer at the annotated copy: correctness-reviewer, skill-auditor, conventions (their own staged-input blocks), the four whole-change reviewers, and all eleven specialist lenses via one edit to the shared disciplines block. Each carries the rule: take anchor.line from the printed number, never count, strip the NNN| prefix when quoting code or authoring a suggested_patch.
  • Nothing that parses a diff sees annotations. Provenance, re-review hunk fingerprints (whose signatures must not shift, or the divergence tripwire would force full reviews fleet-wide), scoped staging, pattern-triage, and the claim-validator all keep reading the raw files.
  • Arm isolation with no flags: eval staging writes the annotated siblings unconditionally; only a review.md version that names them reads them. The A/B between the pre-annotation baseline (review: anchor-snap fallback in the change-provenance gate #255) and this candidate is a pure prompt delta, exactly what the instrument measures.
  • The instrument rides along: per-case anchor-snap counts (perCase.snapped) in arm reports and a pooled "Findings anchor-snapped" row in the aggregate (version-tolerant: older artifacts read as 0). This is the outcome variable: if annotation works, candidate-arm snaps fall to ~0 because anchors arrive correct.

Eval gate

Same targeted-repeats recipe as #255, baseline = #255's branch (snap, no annotation):

gh workflow run review-eval-ab.yml -R Khan/actions --ref jwbron/diff-line-annotation \
  -f base_ref=origin/jwbron/anchor-snap-provenance \
  -f cases=adversarial-injection-approve,golden-request-changes-authz -f repeats=10 -f max_usd=45

Success criteria: candidate-arm "Findings anchor-snapped" ~0 with catch/verdict rates at least as good as baseline (the baseline arm should show the snaps doing the rescuing); noise not worse (a failure mode to watch: annotation prefixes leaking into comment prose or suggestion blocks, which the judge/noise rows would surface). Dispatching after #255's in-flight powered run completes, so the two measurements stay attributable.

Tests

742 pass: annotation unit tests (per-hunk numbering, old-side numbers on removals, headers/no-newline passthrough, section-splittability of annotated text, hunk-extent boundary so trailing text is never annotated, wide-file padding, empty diff), the CLI sibling + subcommand, staged annotated siblings in the eval layout, and typecheck.

jwbron added 30 commits July 9, 2026 12:47
…live cases

Phase 1 of the live A/B eval plan (#232). The corpus gains
an opt-in live block carrying what a real model run needs and the
deterministic replay does not:

- prContext (PR title/description/author/base; the description is
  untrusted author text, so an adversarial case can carry its payload
  there or in the diff),
- a post-change file tree on disk next to the case, via a new
  <id>/case.json + <id>/tree/ layout that coexists with flat <id>.json
  (a directory containing case.json is one case; its tree is never
  parsed as corpus JSON), and
- labeled defect specs (mustCatchSpecs / mustNotFlagSpecs: path, line
  window, mechanism keyword alternates). Live runs choose their own
  finding ids, so ground truth matches on anchor window + mechanism
  rather than id.

The loader enforces the invariants: the live tag and the live block
imply each other, a live case needs a cleanly-parseable diff, spec
paths must appear in changedFiles and the diff, and every non-removed
changed file must exist in the tree. loadLiveCorpus() returns the
subset. The live half lives in corpus/live.ts; loader.ts re-exports it
as the single public surface.

Ten cases are converted with hand-authored real diffs and trees: five
smoke incidents, both clean cases, one adversarial injection whose
payload is a code comment in the diff, one golden holdout, and one
synthetic mutation. Their recorded line anchors are rewritten to the
authored defect lines (natural files beat content padded to synthetic
line numbers), which activates the provenance gate on these cases in
the deterministic suite; all expectations hold unchanged.
…action and case staging

Phases 2a/2b of the live A/B eval plan (#232), stacked on the Phase 1
corpus format (#233).

agent-extract.ts turns review.md's '## agent:' sections into data
(name, description, pinned model, prompt body). It takes the markdown
as a string with no fs access, because the baseline arm of an A/B
reads the merge-base review.md via git show. Parsing is strict; a
malformed or model-less section throws listing every problem, since a
silently dropped agent would skew an eval arm without failing it. An
integration test extracts the real review.md (21 agents) and pins the
staging-root reference so a future rename fails a test instead of
silently staging nothing.

live-stage.ts materializes the production staging layout for one
live-enabled corpus case: pr-context.json (review.md Step 1's shape,
synthetic identity fields), full.diff / pr.diff / full-stripped.diff
(all the case diff; corpus diffs carry no generated files and no
pattern-triage pass runs), files.json + review-files.json with the
hasPatch cross-check derived from the diff parse, provenance.json,
routing.json from the deterministic router, an out/ directory, and
the post-change checkout copied from the case tree.
rewriteAgentPrompt swaps the production staging root for the staged
context dir. Everything sits behind an injected-fs seam and is
memfs-tested.

Model dispatch (phase 2c) is deliberately absent; it needs the Agent
SDK dependency decision and arrives separately.
… runner (phase 2c)

live-producer.ts runs the live roster over one staged case behind an
injected LiveAgentRunner seam (the judge.ts pattern), so its logic is
stub-tested with zero model calls. Roster: the default finders
(correctness-reviewer, skill-auditor) plus the router's lensesToSpawn;
no pattern-triage or thread-reconciler in eval. It maps all three
sub-agent output contracts into the shapes the deterministic runner
consumes: label-shape findings (correctness lens, or conventions for
the skill-auditor so labelForFinding reproduces the best-practice
variants; confidence defaulted to 0.7 per the production claims rule),
structured-schema lens findings validated as-is, and the validator's
{claims: [...]} verdicts into CaseVerification[]. It stages
claims.json for the validator, resolves {{#runtime-import}} directives
against the case tree (a case opts into a skills index by carrying the
file), retries once on malformed output with the rejection fed back,
keeps partial results when an agent fails twice, prefixes colliding
finding ids, and reports per-agent cost/turns/wall-clock.

live-runner.ts is the one module that touches a real model runtime:
an Agent SDK query() per dispatch with Read/Grep/Glob only, cwd pinned
to the staged checkout, the agent's pinned model, and hard turn and
wall-clock caps; plus the CLI smoke entry
(tsx live-runner.ts --case <id>, requires ANTHROPIC_API_KEY). The
investigation-cap CLI the prompts reference is not staged; its own
denied-budget fallback applies. Adds @anthropic-ai/claude-agent-sdk
as a dev dependency.
live-match.ts scores a live run against a case's labeled defect
specs: a posted candidate satisfies a spec when its anchor agrees
with the spec's path (and line window when both carry one) and any
mechanism alternate matches the finding's failure_scenario or prose;
each candidate satisfies at most one spec and vice versa. An injected
fallback arbiter (hard-capped, same-file only, recorded as
via: fallback for audit) can rescue recall on vague prose; false
flags are decided by the deterministic rule alone. computeLiveMetrics
aggregates recall, verdict agreement, clean false-flag (including a
clean case that blocks), and noise.

live-ab.ts is the arm orchestrator and CLI: baseline review.md from
git show <merge-base>, candidate from the working tree, both arms
over the same live corpus with everything else (corpus, lib, runner,
metrics, judge) from the candidate, per the plan's settled decision
to isolate the model seam. Each arm runs under half the --max-usd
budget with sticky exhaustion: once spend plus the running per-case
average crosses the cap, remaining cases are recorded skipped and
the report still emits. Spec-level regressions are diffed only over
cases both arms scored. Judge scoring reuses the pinned judge
(quality aggregates only; judge-vs-ground-truth disagreement keys on
recorded ids a live arm does not use); the fetch model moves to
judge-live-model.ts and live-judge.ts now imports it. runner.ts
gains an optional RunOptions.validation override so a live
validator's output replaces the recorded block.

Report-only except the standing rule: adversarial-injection failures
on the candidate arm exit non-zero.
Review Eval A/B runs on every non-draft PR touching
workflows/review/** (and on workflow_dispatch): both review.md arms
over the live corpus via live-ab.ts, with the delta report posted as
a sticky PR comment (hidden-marker upsert), appended to the job
summary, and uploaded as an artifact even on partial or gate-failing
runs. Per-PR scope is the smoke-tagged live subset; the full-eval
label or dispatch input lifts it, skip-live-eval opts out, drafts
wait until ready, the changeset-release branch is excluded (it
matches the path filter via package.json but changes no behavior),
secretless runs skip green so fork PRs never fail, and per-PR
concurrency cancels superseded runs. The workflow name is distinct
from every gh-aw workflow per the operational-floor rule about shared
concurrency groups. live-ab.ts gains --smoke-only and writes
out/live-ab-report.md alongside the JSON for the comment step.
… and vitest

Tree directories are byte-exact case fixtures paired with each case's
diff: prettier auto-formatting one would silently desync it from the
diff the provenance gate parses, and a tree may carry a *.test.ts
whose tests fail by design (test-adequacy cases), so vitest must not
execute them either. CI's lint job caught the first drift (prettier
wanting to rewrap a fixture ternary).
…rpus' into jwbron/live-eval-producer-staging
…s with the case id

Live agents choose their own finding ids, so every case's first
correctness finding was live-correctness-reviewer-1; ids were unique
within a case but collided across cases, and judge.ts's score join
requires arm-global uniqueness. Caught by the first real A/B run
(both arms completed, then judge aggregation threw). Ids are now
<caseId>:<id> from the moment of parsing, so claims.json, the
validator round-trip, the matcher, and the judge all see the same
namespaced id.
…eport instead of killing it

The first real A/B run spent both arms' budgets and then died in
judge aggregation, writing no report: the exact
everything-spent-nothing-posted failure mode the plan forbids. Judge
scoring is additive, so a per-arm failure is now caught, recorded as
judgeError on the arm, rendered as a degradation note in the report,
and the run proceeds to write JSON + markdown and evaluate the
adversarial gate as usual.
…A/B phase 5)

Packages the seeded-defect live-trial pattern (Khan/webapp#40678) as
a Claude Code skill so a trial costs an operator an afternoon instead
of a week of hand choreography. The skill collects required inputs
(seeded branch, human-authored defect table, arms, budget approval)
and refuses to improvise ground truth; sets up one isolated PR per
arm with per-arm trigger recipes and the distinct-workflow-name rule
(same-named gh-aw workflows share a per-PR concurrency group and
cancel each other); collects reviews, artifacts, and costs per run
with the known gh-aw artifact-bug tolerance; drives optional
lifecycle pushes; scores defect by defect with the deterministic
rule mirroring eval/live-match.ts plus audited manual judgment;
exports live-enabled corpus case skeletons with a sanitization gate
for private-repo content landing in this public repo; and cleans up
trial PRs, branches, and temporary workflows. Trials remain the
architecture-bet instrument; per-change evals belong to the corpus
A/B.

.gitignore narrows from .claude to .claude/* with a !.claude/skills/
carve-out: local agent state (settings, worktrees) stays untracked,
project skills are shared tooling and are committed.
…ity, code-rendered from the reconciler keep-list
…oad (allowed-paths must match staging-relative paths)
…ive case

The first live acceptance runs missed
incident-sql-missing-index:dm-missing-index-1 in all four arms: the
recorded finding is a data-migrations LENS finding, but live cases
carried no routerConfig lens rules, so the live roster never spawned
the specialist that catches it. Every live case whose recorded
finding belongs to a specialist lens now routes that lens on the
finding's file (seven cases across five lenses), mirroring how a
consumer ROUTING file would route the same paths in production.
…rpus' into jwbron/live-eval-producer-staging
… the A/B report

The acceptance runs surfaced a claim-validator failure that the
report could only name, not explain (perCase carried agent names
only, and the PerAgentReport.failed detail never reached the
markdown). perCase failedAgents entries are now '<agent>: <reason>',
so the next failure is diagnosable from the sticky comment alone.
jwbron added 16 commits July 9, 2026 19:13
…powered runs, drift watch, sql-index case fix

The eval instrument's percentage deltas sit below the one-case noise floor;
this lands the memo's measurement items. eval/aggregate.ts pools N report
artifacts into per-case pass rates with Wilson intervals and pooled rows
(reproduces the 07-10 cumulative numbers exactly); live-ab.ts --repeats runs
one powered dispatch with a strict-majority adversarial gate; the weekly
review-eval-drift.yml workflow runs full corpus x3 on main and publishes the
aggregate; identical-arm pools render noise-floor bands as data.

incident-sql-missing-index diagnosed against all 28 recorded arm-runs: the
reviewer found the missing index every single time; the 8/16 catch rate was
the spec accepting only the migration-file anchor while the reviewer anchored
the same blocking finding at the hot query (11 runs) or had it provenance-
dropped on a mis-anchor (2 runs). Specs gain altLocations; the case accepts
the query-site anchor and its residual misses now classify found-but-dropped
(the anchor-snap defect class), not recall.
…rbiter on the pinned Haiku snapshot

Fills the MatchFallback seam in live-match.ts: when a spec stays unmatched
after the deterministic pass and a posted candidate shares its file, a
pinned claude-haiku-4-5-20251001 answers one yes/no question (same defect,
same root cause?), biased to no since a false yes inflates recall. All seam
guard rails inherited: capped per case, same-file only, matches recorded
via 'fallback' for audit. API failures degrade to non-matches through
onError; both arms share the matcher so the A/B delta stays unbiased.
On by default in live-ab.ts; --no-match-arbiter restores deterministic-only
matching. This targets the 54-69% 'unmatched posted' readings, implausibly
high as true noise.
…nderers into live-ab-report.ts

CI's max-lines (1000) caught live-ab.ts at 1011 after the repeats and
arbiter work; this worktree cannot run eslint locally (dot-dir ignore).
The report types (ArmRunReport, AbReport, MultiAbReport, gate types) and
both markdown renderers move to a leaf module with no runner dependency;
live-ab.ts re-exports them so the import surface is unchanged.
…t after every repeat

A multi-repeat run carries tens of dollars of spend but wrote its artifact
only at the end, so a crash or cancellation on repeat n forfeited repeats
1..n-1 (the exact dies-with-nothing-emitted failure mode the plan forbids;
the workflow's always() artifact upload had nothing to grab). Each completed
repeat now overwrites the out path with the accumulated partial payload,
which aggregate.ts already pools via its repeats-field handling.
…or in the report footer

Bought per the memo's item 6: run 29069228968 (--force-arms --repeats 3,
full 14-case corpus, $58.71 under the $60 clamp) gives 6 arm-samples of
one review.md. Measured bands: must-catch recall 54-86%, verdict agreement
75-100%, noise 50-60%, judge quality 0.82-0.86. Every single-run report now
renders these as data with provenance, replacing prose guesswork; a delta
whose arms both sit inside a band is wobble, and the footer points at
--repeats for smaller effects. Pre-arbiter measurement; the weekly drift
run re-measures with the arbiter active.
…budget to $85

The noise-floor dispatch showed the live corpus is now 14 cases (the #251
golden re-review set), and the $60 default trimmed case-runs on both arms
(38 and 36 of 42; $58.71 spent at the clamp). $85 covers 6 full arm-runs
at the measured ~$10/arm-run with landing headroom, so the weekly aggregate
never carries budget-skip asymmetry.
…port opens a visibility PR

workflows/review/eval/README.md is the guide an agent or engineer needs to
run the eval system cold: the three tiers, every live-ab flag and CI entry
point, powered-run recipes with real costs, corpus-growth rules (target the
20-80% band), report-reading guidance anchored on the measured noise floor,
model pins, and the harness's historical limits (nothing before the
2026-07-08 structured-agent architecture runs under it).

The weekly drift run now also opens a PR committing the report markdown and
compact aggregate under .github/review-eval/drift/ (changeset-gate-excluded,
outside the A/B path filter): a job summary nobody opens is not a drift
watch, and merged report PRs accumulate an in-repo time series.
…-floor statistics, rolling repeat budgets

Findings from an adversarial pass over the instrument's own claims:

Reports now stamp their ruler (matcher configuration + a content hash of
the loaded corpus); the aggregate prints it, warns when a pool mixes
rulers, and the drift series stays interpretable across instrument
upgrades like the arbiter default or corpus growth, which move every rate
without the reviewer changing.

Noise-floor bands gain SD (min/max are extreme-value statistics that only
widen as samples accumulate; mean +/- sd is the stable band) and a loud
case-asymmetry warning: the 2026-07-10 measurement had budget skips, so
its published v1 bands fold case-mix variance in on top of wobble; the
footer constants and provenance string now say so.

Repeat budgets roll: each arm-run's slice is the remaining budget over the
remaining arm-runs, so cheap early arms donate headroom forward instead of
stranding it (fixed slices caused the 38/36-of-42 skip asymmetry that
contaminated the noise-floor run). README gains a statistical-honesty
section: clustered-interval optimism, the repeats-gate relaxation, and the
arbiter's uncalibrated refuse bias.
…hange-provenance gate

The reviewer produces right-file, right-mechanism findings at wrong line
numbers (it appears to sometimes count unified-diff text lines instead of
file lines: observed anchors at line 24 of an 18-line file and line 8 of
a 3-line file), and the provenance gate then drops them; since only
surviving blocking labels feed the computed verdict, a correct blocking
finding dying at the gate can flip REQUEST_CHANGES to APPROVE. Observed
on adversarial-injection-approve, golden-request-changes-authz, the
sql-index replay, and production main's own baseline arm.

Before setting a line-anchored finding aside, the gate now snaps it to
the nearest changed line in the same file under two windows: a 3-line
near-miss window (the unified diff's context width) and an overflow
window for anchors past every shown line by no more than the file's
diff-text overhead (exactly the counting mis-anchor's overshoot bound).
Ties break toward the lower line. Snapped findings keep their severity,
post at the snapped anchor, and are recorded for audit (out/snapped.json
in production, snappedByProvenance in eval results); findings outside
both windows keep today's set-aside behavior. provenance.json carries a
precomputed per-file snap lookup so review.md's gate stays a dictionary
lookup. The live A/B emulates each arm's own review.md gate version,
keyed on the anchor-snap marker, so the baseline arm replays the
pre-snap gate and the powered run prices this change.
…istic smoke case

provenance-anchor-snap-rescued replays the observed production anatomy
in the per-push CI gate: a correct blocking finding anchored at the
diff-text line (past the end of a short file, at the exact overflow
edge) snaps to the nearest changed line, posts there, and drives
REQUEST_CHANGES, while a far-anchored pre-existing observation in the
same run is still set aside unposted. Deterministic and smoke-tagged
only, so the live ruler (corpus stamp over live-tagged cases) is
untouched and the in-flight powered run stays comparable. Also
documents how to audit snap records (from/to distance separates the
near-miss and overflow classes).
…ffs, anchors read not counted

The mis-anchor pathology anchor-snap repairs downstream exists because
the staged diff makes the model count lines. annotateDiffLineNumbers
prefixes every hunk content line with its real line number (+/context
lines carry the NEW-file number, - lines the OLD-file number) while
keeping the diff marker in column one, so annotated text still splits
into sections. The provenance CLI writes full-stripped-annotated.diff
beside the raw stripped diff and gains an 'annotate <in> <out>'
subcommand for pr-annotated.diff (Phase 1) and the scoped-depth
refreshes. Every finding-producing reviewer reads the annotated copy
and takes anchor.line from the printed number; everything that parses
a diff (provenance, re-review fingerprints, scoped staging,
pattern-triage, claim-validator) keeps reading the raw files, so hunk
signatures never shift. Eval staging writes the annotated siblings for
both arms unconditionally; only a review.md that names them reads
them, so the A/B prices this as a pure prompt delta. Reports gain the
anchor-fidelity observable: per-case snap counts and a pooled
'Findings anchor-snapped' row (version-tolerant of older artifacts);
if annotation works, candidate-arm snaps fall to zero with the
anchor-snap gate remaining as backstop.
@changeset-bot

changeset-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 6b8e3c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
review Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@khan-actions-bot
khan-actions-bot requested review from a team, jeresig and kevinb-khan and removed request for a team July 10, 2026 20:22
@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review live A/B

Baseline: origin/main (review.md ce260c7c40a9); candidate: working tree (review.md 1a385edab866).

Ruler: matcher deterministic+arbiter; corpus 0c1680e66eae (9 cases).

Metric Baseline Candidate Delta
Must-catch recall 100% 100% +0%
Verdict agreement 89% 89% +0%
Noise (unmatched posted) 50% 53% +3%
Clean false flags 0 0
Judge mean quality 0.87 0.87 +0.00
Cost $7.26 $7.26
Wall clock 1239s 1154s
Cases run / skipped 9 / 0 9 / 0
Misses found-but-dropped 0 0
Findings anchor-snapped 0 0

Adversarial hard gate: PASSED on the candidate arm.

Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric.

Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use --repeats to resolve smaller effects.

@jwbron
jwbron changed the base branch from jwbron/anchor-snap-provenance to main July 13, 2026 19:28
…in' into jwbron/diff-line-annotation

# Conflicts:
#	.changeset/review-anchor-snap.md
#	.github/workflows/review-eval-ab.yml
#	workflows/review/eval/README.md
#	workflows/review/eval/aggregate.test.ts
#	workflows/review/eval/aggregate.ts
#	workflows/review/eval/corpus/live.ts
#	workflows/review/eval/corpus/loader.test.ts
#	workflows/review/eval/corpus/loader.ts
#	workflows/review/eval/corpus/smoke/provenance-anchor-snap-rescued.json
#	workflows/review/eval/live-ab-report.ts
#	workflows/review/eval/live-ab.ts
#	workflows/review/eval/live-match.ts
#	workflows/review/eval/live-stage.test.ts
#	workflows/review/eval/live-stage.ts
#	workflows/review/lib/disciplines.test.ts
#	workflows/review/lib/provenance.test.ts
#	workflows/review/lib/provenance.ts
#	workflows/review/lib/rereview-mode.ts
#	workflows/review/review.md
@jwbron
jwbron merged commit 618de86 into main Jul 13, 2026
5 of 6 checks passed
@jwbron
jwbron deleted the jwbron/diff-line-annotation branch July 13, 2026 19:32
@github-actions

Copy link
Copy Markdown
Contributor

Review Guidance

github-actions (7 files)
File Reason
diff.ts Adds annotateDiffLineNumbers, the pure function that produces the model-facing annotated diff for every consumer PR; a numbering error would mis-anchor findings fleet-wide.
provenance.ts Adds the annotated-sibling write and the annotate subcommand that review.md invokes on every run; parsers still read the raw diff.
review.md This file is the reviewer's behavior in every consuming repo; it repoints all finding-producing reviewers at the annotated copies and updates the re-review depths.
live-stage.ts Stages annotated siblings for both eval arms so an A/B is a pure prompt delta.
aggregate.ts Threads the new required snapped metric through arm aggregation and the pooled report row.
live-ab.ts Populates perCase.snapped from the gate output.
live-ab-report.ts Adds snappedTotal and the per-run anchor-snap report row.

Common patterns

1 file (review.md): Five reviewer prompt blocks all swap their diff source from the raw file to the annotated sibling (full-stripped-annotated.diff / pr-annotated.diff) with the same boilerplate on reading anchor.line off the NNN| prefix.

3 files: A required snapped: number field threaded from the per-case run shape through arm aggregation into the "Findings anchor-snapped" report row (aggregate.ts, live-ab-report.ts, live-ab.ts).

2 files: annotateDiffLineNumbers(...) called and its result written as a *-annotated.diff sibling alongside each existing raw diff write (provenance.ts, live-stage.ts).

Excluded from review (2 files)

Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:

  • .changeset/review-annotated-diffs.md — formatting-only
  • workflows/review/eval/README.md — formatting-only

pr-reviewer v1.4.1 · schema=2

}
unmatched += run.unmatchedPosted;
posted += run.posted;
snapped += run.snapped;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

/** Total anchor-snaps across an arm's case runs (see `perCase.snapped`). */
const snappedTotal = (arm: ArmRunReport): number =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants