review: enforce the change-provenance gate in code; stage generated-stripped diffs - #227
Merged
Conversation
Every finding (not just blocking ones) now carries a failure_scenario: the specific inputs or state and the wrong outcome they produce. The field is required in the structured finding schema (FINDING_SCHEMA_VERSION bumped to 2), emitted by every producer (label-shape reviewers and all 11 specialist lenses), and carried verbatim into claims.json. The claim-validator attacks exactly that stated scenario: confirmed means the named scenario occurs on the cited lines, and a scenario too vague to check caps at plausible. Every corpus fixture is migrated with a hand-written failure scenario, per the schema module's rule that a required field invalidates previously serialized findings.
…tripped diffs New lib/diff.ts (unified-diff parsing) and lib/provenance.ts (gate + CLI) compute a per-file changed-line map (provenance.json). A finding whose anchor is not an added or modified line of the diff cannot carry a blocking label, and pre-existing observations collapse into at most one non-blocking collapsed note (renderPreExistingNote). Deletion findings pass via removedAdjacent lines. The gate fails open (gates nothing, with a review-body note) whenever the parsed map cannot be trusted: an unparseable diff, hunk headers not attributable to any file section, or a changed file whose patch is missing from the parse (files.json now carries a hasPatch flag so binary/too-large files do not false-positive the check). The provenance CLI also stages full-stripped.diff (the full diff minus files the router classifies linguist-generated; routing.json now exposes generatedFiles). Every whole-change reviewer and specialist lens reads it instead of the full diff; pattern-triage keeps the full diff since classifying every changed file is its job. Wired through the no-post eval runner and covered by unit tests plus a new smoke corpus case (provenance-pre-existing-note).
🦋 Changeset detectedLatest commit: 756ea71 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
July 9, 2026 03:24
jeresig
approved these changes
Jul 9, 2026
jeresig
left a comment
jeresig
left a comment
Member
There was a problem hiding this comment.
Great - hope this helps to clean up some of the extraneous comments!
| * Returns `null` when there is nothing to note; the caller then posts no | ||
| * note at all. | ||
| */ | ||
| export const renderPreExistingNote = ( |
Member
There was a problem hiding this comment.
I'm curious if we want to render this at all?
Contributor
Author
There was a problem hiding this comment.
Good call. By the time a finding lands in this bucket it has already slipped past two layers of instruction telling the model not to produce it, so what collects here is mostly drift, and a comment whose body says "no action required in this PR" is exactly the extraneous-comment problem. It would also repeat on re-reviews (the gate runs before the new-scope dedup).
Updated: the note is gone entirely. The gate still sets these findings aside, but they now go to the run artifact (out/pre-existing.json) instead of the PR, so we can still inspect what the gate is catching while posting nothing. A genuinely important pre-existing issue still has a path in: the materially-amplifies rule anchors it on changed lines and it passes the gate as a normal finding.
jwbron
added a commit
that referenced
this pull request
Jul 9, 2026
## Summary `actions/get-changed-files` crashes with `core.warn is not a function` on push events when the pushed commit is associated with more than one open PR. The github-script `core` API method is `core.warning`; `core.warn` does not exist, and the test mock stubbed `warn` so the suite never caught it. The path fires exactly for stacked PRs (a commit that is the head of one PR and part of another), which is how #227 surfaced it: its "Testing with 'push' events" check dies on this line instead of warning and proceeding with the first associated PR. One-line fix plus the matching test-mock rename. cc @jeresig ## Testing - `npx vitest run actions/get-changed-files`: 40 tests green, including the multi-PR warn path. - `pnpm build` (tsc) compiles clean. Author: jwbron Reviewers: jeresig, somewhatabstract, kevinb-khan Required Reviewers: Approved By: jeresig Checks: ✅ 3 checks were successful Pull Request URL: #229
…ote; set-asides are artifact-only Review feedback from jeresig: don't render the collapsed pre-existing observations note at all. Anything the change-provenance gate sets aside now posts nothing to the PR; the collected observations are written to the run artifact (out/pre-existing.json) instead, so the gate stays inspectable for tuning without adding a comment whose body says no action is required. A pre-existing issue important enough to surface still has its path: the materially-amplifies rule anchors it on changed lines and it passes the gate as a normal finding. Removes renderPreExistingNote and its tests, simplifies the eval runner and gates (postedCommentCount is now exactly the posted candidates), updates the prompt and README, and reworks the smoke case to pin that the set-aside finding is absent from the planned review rather than present in a note.
jwbron
added a commit
that referenced
this pull request
Jul 9, 2026
## Summary First of three PRs split out of #224 (stack: this PR, then #227 provenance gate, then #228 prompt disciplines). Each lands independently revertable; the three together are byte-identical to #224. Every finding (not just blocking ones) now carries a concrete `failure_scenario`: the specific inputs or state and the wrong outcome they produce. The field is required in the structured finding schema (`FINDING_SCHEMA_VERSION` bumped to 2), emitted by every producer (label-shape reviewers and all 11 specialist lenses), and carried verbatim into `claims.json`. The claim-validator now attacks exactly that stated scenario: confirmed means the named scenario occurs on the cited lines, and a scenario too vague to check caps at plausible. `FINDING_SCHEMA_VERSION` is now 2 (a required field invalidates previously serialized findings, per the schema module's own rule); every corpus fixture is migrated with a hand-written failure scenario. cc @jeresig ## Testing - `npx vitest run workflows/review`: 410 tests green, including the smoke set. - `npx tsc --noEmit` clean. Author: jwbron Reviewers: jeresig, jaredly Required Reviewers: Approved By: jeresig Checks: ✅ 3 checks were successful Pull Request URL: #226
This was referenced Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second of three PRs split out of #224; stacked on #226 (retarget to main when it merges). The follow-up prompt-disciplines PR stacks on this one.
Change-provenance gate, enforced in code. New
lib/diff.ts(unified-diff parsing) andlib/provenance.ts(gate + CLI) compute a per-file changed-line map (provenance.json). A finding whose anchor is not an added or modified line of the diff cannot carry a blocking label, and pre-existing observations collapse into at most one non-blocking collapsed note (renderPreExistingNote). Deletion findings pass viaremovedAdjacentlines. The gate fails open (gates nothing, with a review-body note) whenever the parsed map cannot be trusted: an unparseable diff, hunk headers not attributable to any file section, or a changed file whose patch is missing from the parse (files.jsonnow carries ahasPatchflag so binary/too-large files do not false-positive the check). Wired through the no-post eval runner and covered by unit tests plus a new smoke corpus case (provenance-pre-existing-note).Generated-stripped whole-change diffs (measured cost fix). The provenance CLI also stages
full-stripped.diff(the full diff minus files the router classifieslinguist-generated;routing.jsonnow exposesgeneratedFiles). Every whole-change reviewer and specialist lens reads it instead of the full diff;pattern-triagekeeps the full diff since classification is its job. These ship together because the same CLI writes both artifacts.No trigger config or consumer files touched.
cc @jeresig
Testing
npx vitest run workflows/review: 452 tests green, including the new end-to-end smoke case for the provenance gate.lib/diff.test.ts,lib/provenance.test.ts, plusrenderPreExistingNoterendering tests and routergeneratedFilescoverage.npx tsc --noEmitclean under the repo's strict compiler options.