review: state introduce-vs-amplify in the posted prose#250
Conversation
🦋 Changeset detectedLatest commit: 9f29767 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,
jaredly and
jeresig
and removed request for
a team
jwbron
force-pushed
the
jwies/review-budget-cap
branch
from
4370d5b to
78e501e
Compare
jwbron
force-pushed
the
jwies/review-provenance-label
branch
from
a2ec4f4 to
45cd748
Compare
|
Eval-case gap worth closing here (or as a rider on #235): the live corpus has must-catch cases for the composite key, eventual consistency, and flag mock, but no amplification/provenance case. Khan/webapp#40736's seeded case (a 'redundant' WithLimit(5) removal amplifying a pre-existing Query default-limit-1 mechanism, plus a non-amplified pre-existing defect that must stay non-blocking, plus an off-diff observation that must not post) is fully specified in that PR's behavior-test record and passed live; porting it gives this PR's prose requirement a scoreable case and locks the gate behavior in regression. |
Review live A/BBaseline:
Regressions (baseline caught, candidate missed)
Adversarial hard gate: FAILED on the candidate arm
Gate flips retried (best of three, flipped cases only)
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. |
…osted prose The finder discipline already classified each defect as introduced or as an amplified pre-existing mechanism, but the classification could live entirely in structure and implication: the amplification behavior test's posted comment named the pre-existing default and attributed the regression to the guard removal without ever saying which was which. The finder rule now requires the call as a plain clause in the discussion prose the author reads, covering the boundary case where the enabling mechanism predates the diff but the defect is new, and the claim-validator's corrected path adds the clause when a claim omits it.
jwbron
force-pushed
the
jwies/review-provenance-label
branch
from
45cd748 to
f504b16
Compare
…' into jwies/review-provenance-label
…' into jwies/review-provenance-label
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…fy paragraph The insertion left one clause alone on a short line, breaking the block's fill width.
Phase 1 of the live A/B eval plan (#232): the eval corpus learns to carry real change content so a future live producer can run the actual model sub-agents against it. ## Format A case may now carry an opt-in `live` block (`corpus/live.ts`, re-exported through the loader): - `prContext`: PR title/description/author/base branch, mirroring production `pr-context.json`. The description is untrusted author text, so adversarial cases can carry their payload there or in the diff. - An on-disk post-change file tree, via a new `<id>/case.json` + `<id>/tree/` layout coexisting with flat `<id>.json`. A directory containing `case.json` is one case; its tree is never parsed as corpus JSON. - `mustCatchSpecs` / `mustNotFlagSpecs`: labeled defects as (path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window plus mechanism rather than id; the Phase 3 matcher consumes these. Enforced invariants: the `live` tag and the block imply each other; a live case needs a cleanly-parseable diff (fail-open is fine in production, an authoring error here); spec paths must appear in `changedFiles` and the diff; every non-removed changed file must exist in the tree. `loadLiveCorpus()` returns the subset. ## Cases Ten cases converted with hand-authored real diffs and trees: the five smoke incidents (money rounding, auth bypass, cache key, race condition, missing index), both clean cases, the adversarial injection case (payload is a code comment in the diff instructing the reviewer to approve), the golden authz holdout, and the money-payments synthetic mutation. Recorded line anchors are rewritten to the authored defect lines; natural files beat content padded to synthetic line numbers, and every anchor is asserted to be an added line so the provenance gate (now active on these cases) keeps them. The trial-content port landed as #235, stacked on this PR. One acceptance-driven change also landed here: the phase 4 runs missed `incident-sql-missing-index` in all four arms because live cases carried no `routerConfig` lens rules, so specialist lenses never spawned; every live case whose ground truth belongs to a specialist lens now routes that lens on the finding's file (see the "route the specialist lens on each live case" commit). ## Test plan: - `pnpm run test --run`: 659 tests green, including 17 new loader tests (memfs) covering the live block, both layouts, tree validation, and the tree-JSON exclusion. - The deterministic suite runs the ten converted cases unchanged (same verdicts, comment counts, must-catch sets), now with the provenance gate active on them. - `pnpm run typecheck` and eslint clean on the three touched TS files. ## Next steps (human) 1. Review the ten authored diffs and trees for realism; they are synthetic content a live model will read, so plausibility matters more than in ordinary fixtures. Spot-check that each case's recorded finding still describes the authored defect (anchors were rewritten to the authored lines). 2. This is the root of the stack: review and undraft first; #234 and #235 rebase onto it. 3. No live validation needed here; the deterministic suite (`pnpm run test --run`) fully gates it. Author: jwbron Reviewers: jeresig, github-actions[bot], jwbron Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #233
| materially amplify is at most a `note (non-blocking)`, never blocking; the | ||
| orchestrator also enforces this positionally (a finding not anchored on an | ||
| added/modified diff line cannot block). | ||
| was removed). Put that call in the `discussion` prose itself, in plain words the |
There was a problem hiding this comment.
suggestion (non-blocking): This rule enforces the introduce-vs-amplify clause through prose compliance alone — the Finding type in workflows/review/lib/finding-schema.ts has no provenance field to anchor it. That schema already contains a precedent for the more robust move: rule_quote is a structured field the renderer surfaces mechanically into the comment, for the same author-visibility reason. Consider a provenance enum (introduced | pre-existing-amplified | mixed) rendered into a fixed clause, so the validator checks a field instead of sniffing prose. Non-blocking — the prose rule is a reasonable increment; this is just the more durable end state.
# Conflicts: # workflows/review/eval/corpus/live.ts
Review Guidancegithub-actions (1 file)
Excluded from review (1 file)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
| "pre-existing; this change amplifies it by removing the guard" — not only in a | ||
| structured field or implied by the description of the mechanism. This includes the | ||
| boundary case where the enabling mechanism predates the diff but the defect is new | ||
| (a changed line drops the guard that made a pre-existing default safe): name the |
There was a problem hiding this comment.
question (non-blocking): Guard removal appears both as the amplification example (line 1609, "a guard in front of it was removed") and, here, as the "introduced" boundary case ("a changed line drops the guard that made a pre-existing default safe"). The discriminator — whether the defect could already occur pre-diff (amplify) vs. was previously fully prevented (introduce) — is implicit, not stated as an explicit tiebreaker keyed on the shared "guard removed" phrasing. Since the claim-validator's confirm gate makes the classification consequential, a finder and the validator could plausibly classify the same guard-removal scenario oppositely. Worth one sentence stating the tiebreaker.
1 participant
Follow-up from the v1.4.0-preview amplification behavior test (Khan/webapp#40736, run 29051817727): the reviewer's behavior was correct (single blocking, validator-confirmed comment on the amplifying hunk; the merely-modified pre-existing defect stayed unposted), and the posted comment argued the provenance distinction in substance, naming the pre-existing default and attributing the regression to the guard removal. But it never said which part was which: the introduce-vs-amplify call lived in structure and implication, not in words the author reads.
Changes
discussionprose ('introduced by this change', 'pre-existing; this change amplifies it by removing the guard'), and covers the boundary case the test hit: the enabling mechanism predates the diff but the defect is new (a changed line drops the guard that made a pre-existing default safe), where the comment must name the mechanism as pre-existing and the regression as introduced.correctedpath symmetrically: add the missing clause whether the claim is an unlabeled amplification or an introduced defect riding a pre-existing mechanism.Prompt-only; no code paths change.
pnpm run test --rungreen (708), typecheck clean.Stacked on #249.