review: the re-review mode dial (full | scoped | flip-gated | fast)#246
review: the re-review mode dial (full | scoped | flip-gated | fast)#246jwbron wants to merge 9 commits into
Conversation
🦋 Changeset detectedLatest commit: 73b55f4 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 |
jwbron
force-pushed
the
jwbron/rereview-mode-dial
branch
from
be4202d to
6cee4a1
Compare
jwbron
changed the base branch from
jwbron/review-out-of-lane
to
jwbron/review-dispatch-tax
khan-actions-bot
requested review from
a team,
jaredly and
jeresig
and removed request for
a team
jwbron
force-pushed
the
jwbron/review-dispatch-tax
branch
from
8e015ff to
813767c
Compare
|
Heads-up: jwbron/review-dispatch-tax (this PR's base) was rebuilt on the refreshed chain per the topology note on #243; its history was rewritten (old tip 8e015ff, new tip 813767c, content unchanged apart from prettier formatting). Reset or rebase local state onto origin/jwbron/review-dispatch-tax before continuing. The prettier/lint CI failures on the old chain are fixed on the new one. |
…parsing, deterministic depth lib, lifecycle evals The runs-per-PR cost lever. A per-repo 're-review' line in ROUTING (full | scoped | flip-gated | fast, default full) dials how much of the roster a repeat review runs. lib/rereview-mode.ts owns the deterministic half: content-hashed hunk signatures over the generated-stripped diff, the hidden review-body fingerprint stamp (survives cache eviction, dismiss-stale-approvals, and COMMENTED-only histories), the divergence tripwire that re-arms full review at unreviewed share >= 0.4, and the plan/stamp CLI. Adversarial lifecycle cases (rewrite-after-approval, sparse-PR-then-payload) ship as data in eval/lifecycle/ with a deterministic replay harness; counters gain costByRereviewDepth so the live A/B prices scoped against full. The review.md orchestrator wiring lands in a follow-up commit, on top of fold-in batch two's re-review accountability surface.
… accountability renderer The re-review mode dial's flip gate needs 'did the reconciler keep any blocking thread' as a number it can read from rereview.json, not a label judgment re-made at verdict time. Additive: the section rendering is unchanged.
… the orchestrator Step 1 stages the bot's prior reviews (every state, dismissed and comment-only included) for the stamp reader. Step 3 runs the rereview-mode plan CLI after the provenance CLI and maps each depth to its dispatch and staging: scoped overwrites full-stripped.diff with scoped.diff so the full roster reads only unseen hunks; flip-gated dispatches reconcile plus correctness over the scoped diff; fast is reconcile-only. Step 4 gains the flip rule: a reduced-depth run may flip a stamped REQUEST_CHANGES to APPROVE only at keptBlockingCount 0, and in flip-gated depth a validated blocking finding posts and blocks, vetoing the flip. Step 6 appends the hidden fingerprint stamp as the last line of every submitted review body; Steps 7 and 8 skip on reduced depths; Step 9 notes the stamp, not the cache, is the tripwire's authoritative fingerprint. The executed plan is copied into out/ so the run artifact records the depth the counters price.
…es for the newly-linted workflows tree The dispatch-tax trim brought workflows/review into eslint+prettier scope; format the mode-dial files and the two pre-existing violations in disciplines.test.ts and finding-schema.ts so CI lints clean.
jwbron
force-pushed
the
jwbron/rereview-mode-dial
branch
from
b2308dd to
976b925
Compare
Review live A/BBaseline:
Improvements (candidate caught, baseline missed)
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. |
|
Eval pointer: the real (roster-level) evals for the mode dial live in #251, stacked on this PR. This PR carries the mechanism plus its deterministic layer only: the depth/tripwire replays in What actually measures reviewer behavior under each dial setting is in #251:
Reviewers judging whether the dial is measured before any repo flips a mode line: that evidence is #251's; both merge before any consumer can set |
… into jwbron/rereview-mode-dial
… into jwbron/rereview-mode-dial
Review Guidancegithub-actions (5 files)
Common patterns2 files: 2 files: New 3 files: Three Excluded from review (3 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| input.currentSignature, | ||
| input.priorStamp.anchorHunks, | ||
| ); | ||
| if (divergence.unreviewedShare >= threshold) { |
There was a problem hiding this comment.
suggestion (non-blocking): The inclusive >= boundary is the whole point of the tripwire, but share === 0.4 exactly is never tested — the decideReReviewDepth cases cover 1.0/0.5 above and 1/6, 1/3 below the wire. A refactor to > would slip a diff sitting precisely on the threshold through as a cheap re-review with no test failing. Add a case with 2 of 5 unreviewed hunks (share 0.4) asserting depth === 'full' and tripwireRearmed === true.
Low-confidence (1)
workflows/review/lib/counters.ts:291—costByRereviewDepth: a depth group mixing cost-bearing and cost-less runs dividesusdSumby allruns, understating per-run cost; only uniform groups are tested (may be intentional, mirroringtokensPerRun).
| * `full` is the default everywhere: a repo pays for a cheaper mode only by | ||
| * writing a `re-review` line in its ROUTING file. | ||
| */ | ||
| export const RE_REVIEW_MODES = [ |
There was a problem hiding this comment.
thought (non-blocking): The changeset's own evidence ("a fresh seeded defect was caught on a re-review push that a reconcile-only path would have missed; the case for scoped as the recommended reduced depth") argues for full|scoped, yet fast and flip-gated — both reconcile-only for fresh code — ship now as permanent surface before any consumer has priced even scoped. Consider shipping full|scoped and adding the reconcile-only modes in a follow-up if the live A/B shows scoped doesn't capture enough of the savings. (Counter-argument acknowledged: the A/B needs the modes to exist to price them.)
| * pushed onto a sparse PR (share near 1.0) always re-arms. Exported so the | ||
| * eval suite and the live A/B can price other settings. | ||
| */ | ||
| export const DEFAULT_TRIPWIRE_THRESHOLD = 0.4; |
There was a problem hiding this comment.
thought (non-blocking): computeDivergence uses an unweighted hunk-count share — no weighting by changed lines or ROUTING risk tier — so under fast a single hunk pushed onto a 6-hunk reviewed PR sits at ~0.17 and never trips the 0.4 wire, getting zero finding-producing review. Both adversarial lifecycle cases use payloads that dominate the count, so this small-payload weak spot isn't exercised. A floor rule (any unreviewed hunk in a high-tier path re-arms) or a line-weighted share would close it. Not a regression today since full is the default.
| a category call. Count the blocking labels in your final comment set; zero blocking | ||
| labels means APPROVE. | ||
|
|
||
| **The re-review flip rule (reduced depths only).** One addition to the rule above |
There was a problem hiding this comment.
question (non-blocking): The flip gate is computed as data (flipGate in the plan, keptBlockingCount in rereview.json), but I don't see any code comparing the submitted verdict against it — computeVerdict keys only off posted labels, and the veto lives entirely in this Step 4 prose. Is a code chokepoint (e.g. the stamp CLI refusing an APPROVE stamp when depth is flip-gated/fast, the prior stamp verdict was REQUEST_CHANGES, and keptBlockingCount > 0) intended as a follow-up, or is prompt-following the deliberate enforcement for this guard?
| const verdict = flagIndex >= 0 ? rest[flagIndex + 1] : undefined; | ||
| if (verdict === undefined || !/^[A-Z_]+$/.test(verdict)) { | ||
| // eslint-disable-next-line no-console | ||
| console.error("usage: rereview.ts stamp --verdict <EVENT>"); |
There was a problem hiding this comment.
nitpick (non-blocking): The usage string prints rereview.ts, but this file is rereview-mode.ts and a separate rereview.ts actually exists in the same directory, so the printed name points at the wrong (real) file. The sibling investigation-cap.ts names itself correctly.
| console.error("usage: rereview.ts stamp --verdict <EVENT>"); | |
| console.error("usage: rereview-mode.ts stamp --verdict <EVENT>"); |
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
# Conflicts: # workflows/review/eval/corpus/live.ts
| * world (its cwd, with no network), so the agent WILL often try to read | ||
| * an imported module or caller that is not there; that read returns an | ||
| * ordinary not-found tool error the agent tolerates and works around, | ||
| * not a run failure. The cost of a missing file is realism, not a crash: |
There was a problem hiding this comment.
thought (non-blocking): The "not a run failure" guarantee for a missing tolerated read is enforced elsewhere (live-runner.ts absorbs per-tool read errors and throws only on run-level subtype !== "success"), and no test near this comment pins it. The corpus-side promise can silently drift from the harness if the staging/runner behavior later tightens. Consider cross-referencing the runner or adding a small assertion so the two stay coupled.
1 participant
The re-review mode dial: the runs-per-PR cost lever from the plan of record's "Re-review economics" section. Stacked on the eval stack (#232-#238) and fold-in batch two (#243-#245); the base should track the top of the batch-two stack as it grows.
Why
Two measured lifecycles of the same seeded PR paid the full 11-agent fan-out on every push, with per-run cost rising as threads accumulate (v1.3.1: $7.19 / $8.77 / $9.14; v1.4.0: $8.31 / $9.87 / $11.29, runs 29044383251 / 29045581242 / 29046928431 in Khan/webapp). The approval run is the most expensive while emitting the least. Both lifecycles also caught a fresh seeded defect on a re-review push that a reconcile-only fast path would have missed, which argues for
scopedas the default reduced depth.What
A per-repo mode line in
.github/aw/review/ROUTING:full: today's behavior, and the default. No consumer changes behavior until its repo sets a mode line.scoped: full roster, staged only the hunks new since the last fully-reviewed fingerprint; comments scoped to those hunks.flip-gated: reconcile plus the correctness pass over new hunks; a REQUEST_CHANGES to APPROVE flip is vetoed by that pass's validated blocking findings (the findings gate the flip instead of being discarded).fast: reconcile-only.Three guards, all deterministic (
workflows/review/lib/rereview-mode.ts):Named interactions handled by construction:
state, so a dismissed approval still anchors and still reads as an approval.Eval and measurement
The roster-level evals live in #251 (stacked on this PR): open-PR corpus cases with reconciler dispatch and flip-gate/duplicate scoring, the per-mode pricing sweep (
eval/rereview-sweep.ts), and the ported seeded lifecycle. This PR carries the deterministic layer:eval/lifecycle/):rewrite-after-approvalandsparse-pr-then-payload, each scored as "the tripwire re-armed and the payload got a full review", plus an economy control (fix-push-stays-cheap) scored as "the reduced path actually ran". Replayed deterministically ineval/lifecycle.test.tsunder all three reduced modes.out/rereview-plan.json;lib/counters.tsaggregatescostByRereviewDepth; the review-trial skill gains a ROUTING-override arms pattern (two arms at the same ref differing only in the mode line) and scores adversarial lifecycle pushes.enablelines are.Composition with batch two
Based on the top of the batch-two stack (#243 -> #244 -> #245 -> #247) so the mode dial's re-review paths compose with the re-review accountability rendering (#244) rather than race it: the accountability section stays the visible body surface; the fingerprint stamp is appended last as a hidden comment; and the flip gate reads
keptBlockingCount, a small additive field this PR adds to the accountability renderer'srereview.json, so "did the reconciler keep any blocking thread" is a number read at verdict time, not a label judgment re-made there. The orchestrator wiring (Step 1 prior-review staging, the Step 3 depth plan and per-depth dispatch/staging, the Step 4 flip rule, the Step 6 stamp, reduced-depth skips in Steps 7/8) is the last commit on the branch.Supersedes
Khan/frontend#13420 (frontend's consumer-side re-review stopgap, held on the old pin) is superseded by this: the dial lives in the shared workflow and frontend opts in with a single
re-reviewline in its ROUTING file once priced. Frontend is deliberately not touched here.