review: anchor-snap fallback in the change-provenance gate#255
Conversation
…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.
🦋 Changeset detectedLatest commit: 3f96569 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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
requested review from
a team,
kevinb-khan and
somewhatabstract
and removed request for
a team
…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).
|
Sha provenance note: the powered run (29119112371, |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| side !== "LEFT" && | ||
| last !== undefined && | ||
| line > entry.lastShownLine && | ||
| line - last <= entry.textOverhead |
There was a problem hiding this comment.
issue (blocking): The overflow snap window can rewrite a genuine finding about untouched code into a posted blocking comment. Its premise — an anchor past lastShownLine by ≤ textOverhead can only be a diff-text-counting mis-anchor — breaks for a small hunk in a longer file: @@ -1,7 +1,4 @@ gives lastShownLine=4, textOverhead=7, so the window admits real file lines 5–9, which are genuinely unshown code below the hunk. A blocking finding anchored at its true line 8 there snaps onto the last changed line, keeps blocking severity, and posts — the exact "no blocking review of code the PR didn't touch" guarantee the gate exists to enforce, and it can flip the verdict on untouched code. The near-miss window doesn't share this (its targets are lines actually shown). Consider gating the overflow rule on the hunk reaching EOF (a fully-added file has no unshown lines below — the case that motivated it), or demoting overflow-snapped findings to advisory so a mis-anchor is still surfaced without blocking on unverified untouched code.
Low-confidence notes (2)
.changeset/review-anchor-snap.md:2— this behavior change ships as aminorbump; the README semver contract (a release that changes reviewer behavior bumps major) arguably wantsmajor, though prior review changesets have usedminorfor behavior changes too.workflows/review/lib/provenance.ts:125— the overflow window uses whole-filetextOverheadsummed across all hunks, so in a multi-hunk file the window past the last hunk can extend farther than that hunk's own overshoot.
| export const ANCHOR_SNAP_MARKER = "anchor-snap"; | ||
|
|
||
| /** Whether a review.md version documents the anchor-snap gate rule. */ | ||
| export const reviewMdHasAnchorSnap = (markdown: string): boolean => |
There was a problem hiding this comment.
suggestion (non-blocking): reviewMdHasAnchorSnap keys each arm's gate emulation on markdown.includes("anchor-snap") over the whole prompt, so a future review.md that merely mentions the term (a changelog line, a "removed anchor-snap" note) would read as the rule being present and silently flip an arm's gate emulation — mis-pricing the very change this mechanism exists to measure. Consider keying on a structured single-purpose marker (a gate-version: line or a reserved HTML comment) and adding a negative test for a prompt that names the term outside the gate step.
| ); | ||
| expect(reviewMdHasAnchorSnap(reviewMd)).toBe(true); | ||
| expect(reviewMd).toContain(ANCHOR_SNAP_MARKER); | ||
| }); |
There was a problem hiding this comment.
suggestion (non-blocking): The new marker tests here are thorough, but the runProvenanceCli test just below asserts only .files/.warnings on the written provenance.json, never .snap — the entire consumer contract review.md's gate reads as a pure dictionary lookup. A regression that stopped serializing snap would silently no-op the whole anchor-snap rule in production while every in-memory unit test still passed. The CLI's DIFF fixture already yields a non-empty snap table, so a positive assertion on provJson.snap is cheap and would guard that serialization boundary.
| console.log( | ||
| JSON.stringify({ | ||
| files: Object.keys(result.provenance.files).length, | ||
| snapLines: Object.values(result.provenance.snap).reduce( |
There was a problem hiding this comment.
nitpick (non-blocking): This snapLines reducer runs only in the require.main === module CLI block, which the runProvenanceCli tests never execute, so the operator-facing count is untested. A small unit asserting the count for a known diff would close it.
| entry: FileChangedLines, | ||
| side: Side = "RIGHT", | ||
| ): number | null => { | ||
| const targets = side === "LEFT" ? entry.removed : rightTargets(entry); |
There was a problem hiding this comment.
note (non-blocking): snapLineToChanged snaps LEFT-side anchors against entry.removed and applyProvenanceGate calls it for every out-of-provenance anchor, but the precomputed snap table review.md ships is RIGHT-side only (buildFileSnapTable calls snapLineToChanged(..., "RIGHT") exclusively) and the gate step looks it up side-blind. So a LEFT-side near-miss mis-anchor is snapped-and-kept in the eval runner but has no table entry in production — the A/B prices a gate the shipped rule doesn't have. Either drop LEFT-side snapping (the observed pathology is RIGHT-only per the module doc) or key the table by side so the two implementations stay coextensive.
| * width, so the window never reaches past the context block the reviewer was | ||
| * actually reading when it anchored. | ||
| */ | ||
| export const ANCHOR_SNAP_WINDOW = 3; |
There was a problem hiding this comment.
thought (non-blocking): The overflow window is proven by arithmetic, but the ±3 near-miss window admits lines by distance alone with no check that the finding is actually about the snapped line — and the production incidents cited in the description were all overflow-class (past-EOF), not interior near-misses. A cheap content cross-check (snap only when the finding's evidence/prose matches text on the target line) would both justify this window and point at the root fix: content-resolved anchoring would eliminate the numeric-counting mis-anchor class outright rather than bounding it.
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus f281ede095eb (7 cases).
Adversarial hard gate: PASSED on the candidate arm. Agent failures
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 |
Eval gate: passed on all success criteriaPowered runs complete, both halves 10 repeats/arm against
Against the stated criteria:
One honest footnote: eliminating the drops raises measured noise on the golden case (14%→50%, back into the corpus-normal 50-60% band). The pre-snap gate was suppressing comments, correct ones included; the low baseline noise was an artifact of dropping findings. Total powered spend $24.68 plus smoke tripwires. Instrument-side record: the eval memo, rev 5. |
1 participant
Round three's top reviewer bet, eval-gated per the roadmap. Stacked on #252 (needs its powered-run machinery, the drop-bucket taxonomy, and the two anchor-fragile corpus cases as the eval gate).
The defect
The reviewer produces right-file, right-mechanism findings anchored at wrong line numbers; it appears to sometimes count unified-diff text lines instead of file lines. Observed live: the
adversarial-injection-approveblocking auth finding anchored at line 24 of an 18-line file (its exact position in the diff text: 4 header lines + 4 removed lines above it), and a finding at line 8 of a 3-line file in the sql-index replay. The change-provenance gate (in code since #227) then drops the finding, and since only surviving blocking labels feed the computed verdict, a correct blocking finding dying at the gate flips REQUEST_CHANGES to APPROVE. Evidence spans both corpus cases, the cumulative run's both arms, and production main's own baseline arm: a live production defect.The fix
Before setting a line-anchored finding aside,
applyProvenanceGatesnaps it to the nearest changed line in the same file, under two windows:lastShownLine), by no more than the file's diff-text overhead (textOverhead= file headers + hunk headers + removed lines), is the counting mis-anchor: that overhead is exactly the amount diff-text counting overshoots by, so the intended line can only be a shown line, and the anchor snaps to the last changed line. This is what repairs both observed cases (offsets 8 and 5, each within its file's overhead bound and both past EOF), while a genuine observation about an untouched region stays outside it: on theprovenance-pre-existing-droppedsmoke case the legacy finding at line 45 (23 lines past the shown range, overhead 5) still drops.A snapped finding keeps its severity, continues through the pipeline, and posts at the snapped anchor; every snap is recorded for audit (
out/snapped.jsonin the production artifact,snappedByProvenancein eval run results). A finding outside both windows keeps today's behavior exactly: demoted to advisory, artifact-only, never posted. The gate's purpose is unchanged; snapping is for near-miss anchors, not provenance laundering.Mechanically,
provenance.jsonnow carries a precomputed per-filesnaplookup (every admissible near-miss/overflow line, finite by construction, built by the same function the code gate calls), so review.md's gate step stays a pure dictionary lookup with no model-side line arithmetic.Pricing the change through the A/B
The deterministic pipeline is shared by both arms, so the gate emulates each arm's OWN review.md gate version, keyed on the literal
anchor-snapmarker the gate step now carries (reviewMdHasAnchorSnap): the baseline arm (base ref, pre-snap prompt) replays the pre-snap gate, the candidate arm snaps. Once both prompts carry the rule, both arms snap and the A/B returns to measuring prompt deltas alone.Eval gate, per the memo's targeted-repeats recipe (baseline = this PR's base branch, no snap):
Success criteria: found-but-dropped provenance counts on the two cases fall to ~0 on the candidate arm, per-case catch and verdict rates rise with separated intervals, and the strict-majority adversarial gate passes. Results will be posted here when the dispatch completes.
Tests
Unit tests cover the window edges (3 in, 4 out), the overflow bound edges (at and one past), interior lines excluded from the overflow rule, equidistant-tie direction, LEFT-side snapping over removed lines (window only, no overflow), range anchors (first admitted line, collapsed to single-line), file/pr/unknown-path anchors (never snapped),
anchorSnap: false(pre-snap baseline emulation), fail-open (no snaps), the precomputed lookup's exact contents, and both observed production mis-anchors replayed byte-for-byte from the corpus diffs. An end-to-end runner test pins the verdict flip: the mis-anchored blocking finding posts at the snapped line and drives REQUEST_CHANGES; under the pre-snap gate it drops and the verdict flips to APPROVE, classified found-but-dropped provenance.