diff --git a/.changeset/review-rereview-mode-dial.md b/.changeset/review-rereview-mode-dial.md new file mode 100644 index 00000000..3257d807 --- /dev/null +++ b/.changeset/review-rereview-mode-dial.md @@ -0,0 +1,5 @@ +--- +"review": minor +--- + +The re-review mode dial, the runs-per-PR cost lever: a per-repo `re-review` line in `.github/aw/review/ROUTING` (`full` | `scoped` | `flip-gated` | `fast`, default `full`) decides how much of the roster a repeat review of the same PR runs. `scoped` runs the whole roster staged only the hunks new since the last fully-reviewed fingerprint; `flip-gated` runs reconciliation plus the correctness pass, whose validated blocking findings veto a REQUEST_CHANGES→APPROVE flip; `fast` is reconcile-only. Three deterministic guards (`lib/rereview-mode.ts`): the anchoring full review is taken at ready-for-review (never on a draft skeleton); the flip gate; and a divergence tripwire that stamps a content-hashed hunk signature into the review body as a hidden comment (surviving cache eviction, dismiss-stale-approvals, and COMMENTED-only histories) and re-arms full review when the unreviewed share reaches 0.4. Adversarial lifecycle cases (rewrite-after-approval, sparse-PR-then-payload) live in `eval/lifecycle/` and are replayed deterministically; run cost now aggregates per executed depth (`costByRereviewDepth`) so the live A/B can price `scoped` against `full` on recall and dollars. Ships default-`full`: no consumer's behavior changes until its repo adds a mode line. Motivated by measured lifecycle costs on the seeded trial (v1.3.1: $7.19/$8.77/$9.14; v1.4.0: $8.31/$9.87/$11.29 per push), where 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. Supersedes the consumer-side stopgap in Khan/frontend#13420. diff --git a/.claude/skills/review-trial/SKILL.md b/.claude/skills/review-trial/SKILL.md index f4c96182..08c33dd8 100644 --- a/.claude/skills/review-trial/SKILL.md +++ b/.claude/skills/review-trial/SKILL.md @@ -31,6 +31,13 @@ missing, stop and ask; do not improvise a defect table from the branch diff. - `workflow @ `: the shared review workflow pinned to a tag/branch (a candidate build or a prior release). - `hosted`: the Claude Code GitHub app review (`@claude review`). + + A `workflow` arm may also carry a **ROUTING override**: extra or changed + lines for the arm branch's `.github/aw/review/ROUTING`. This is how the + re-review mode dial is priced; two arms at the SAME ref, one with + `re-review full` (control) and one with `re-review scoped` (or + `flip-gated`/`fast`), differ only in the mode line, so the lifecycle + tables isolate the dial's recall and dollar deltas. 4. **Lifecycle plan** (optional): the push-2 content (fixes mixed with fresh seeds, plus their defect-table rows) and the push-3 content (everything fixed). @@ -85,6 +92,15 @@ let each arm re-review, and collect again; repeat for push-3. Score each push separately: re-review behavior (thread resolution, duplicate suppression, scoping to new hunks) is half the point of the lifecycle. +When an arm runs a reduced re-review mode (`re-review` in ROUTING), also +record per push: the executed depth and tripwire fields from the run's +`out/rereview-plan.json` artifact, and the billed cost, so the report can +price depth against recall. If the lifecycle plan includes an adversarial +push (a rewrite-after-approval or a payload onto a sparse PR; the +`eval/lifecycle/` cases), score it as: the tripwire re-armed AND the payload +got a full review; a reduced-mode arm that approves such a push without +re-arming has failed the trial whatever it cost. + ## Step 4: score Match each arm's posted comments against the defect table, per push: diff --git a/workflows/review/README.md b/workflows/review/README.md index 93794214..4db549cd 100644 --- a/workflows/review/README.md +++ b/workflows/review/README.md @@ -153,12 +153,14 @@ rule per line: ``` # [lens=,…] [tier=trivial|low|medium|high] [direction-dependent] # enable [,…] +# re-review full|scoped|flip-gated|fast services/**/migrations/** tier=high lens=data-migrations **/*.graphql lens=api-federation-compat pkg/auth/** tier=high direction-dependent lens=security-auth services/**/testdata/** tier=trivial docs/** tier=trivial enable holistic,test-adequacy +re-review scoped ``` - `lens=` names the specialist lenses to spawn when the pattern is touched; when @@ -176,6 +178,9 @@ enable holistic,test-adequacy (tightening vs. loosening a check); the router emits the file as a pending risk question instead of guessing, and it applies only when its own rule is the winning tier rule for the path. +- `re-review` sets the repo's re-review mode (see the next section). Default + `full`; when several lines set it, the last one wins with a warning. An + unknown mode degrades to `full`: toward more review, never less. Glob semantics are a practical subset of gitignore/CODEOWNERS: `**` crosses directories, `*` and `?` stay within a segment, a trailing `/` matches everything @@ -187,6 +192,49 @@ and are skipped; routing degrades to fewer lenses, never to a crashed review. stays the model-facing prose about file *contents*; team ownership stays in `.github/REVIEWERS`, unchanged. +### Re-review modes (the runs-per-PR cost lever) + +The workflow reviews every push, so a PR's lifetime cost is runs-per-PR times +cost-per-run, and measured lifecycles showed cost *rising* per run as review +threads accumulate, with the final approval run the most expensive while +emitting the least. The `re-review` line in `ROUTING` dials how much of the +roster a *repeat* review runs; the first full review of a ready PR always runs +everything, whatever the mode: + +| Mode | Repeat review runs | When to use | +| --- | --- | --- | +| `full` | The whole roster over the whole diff (today's behavior). **Default.** | Until the live A/B has priced a cheaper mode for the repo. | +| `scoped` | The whole roster, staged only the hunks that are new since the last fully-reviewed fingerprint (`scoped.diff`); comments stay scoped to those hunks. | The recommended first step down: measured lifecycles caught fresh seeded defects on re-review pushes, which a reconcile-only path would miss. | +| `flip-gated` | Thread reconciliation plus the correctness pass over the new hunks. A REQUEST_CHANGES→APPROVE flip is vetoed by any validated blocking finding from that pass; the pass gates the flip instead of being discarded. | Cheap re-reviews that still cannot flip to approval over a fresh validated defect. | +| `fast` | Thread reconciliation only. | Maximum savings; fresh code on a re-push is guarded only by the tripwire below. | + +Three guards keep the cheaper modes honest (`lib/rereview-mode.ts`, deterministic): + +- **Ready-for-review anchor.** A fingerprint taken while the PR was a draft + never anchors cheap re-reviews of the ready PR: the ready PR gets the one + full review the cheaper modes lean on. +- **Flip gate.** In `flip-gated` mode the dispatched correctness pass's + validated blocking findings veto the approval flip. +- **Divergence tripwire.** Every full-depth review stamps a content-hashed + hunk signature into its review body as a hidden comment (it survives cache + eviction and branch protection's dismiss-stale-approvals, and it (not the + review state) is what marks a full review as having happened, so a + COMMENTED-only or dismissed history never wedges the dial). Each push is + compared against that last fully-reviewed fingerprint; when the unreviewed + share reaches the threshold (default 0.4), full-review mode re-arms and the + divergent push gets the whole roster. This is what defeats + rewrite-after-approval and sparse-PR-then-payload + (`eval/lifecycle/`, replayed in `eval/lifecycle.test.ts`). + +The dial is a measured change, not a default change: it ships `full` +everywhere, so no consumer's behavior moves until its repo adds a `re-review` +line, and that line should be earned the way `enable` lines are, through the +live A/B (arms identical except the ROUTING mode line, priced on recall and +dollars; each run's executed depth lands in `out/rereview-plan.json`, and the +counters aggregate `costByRereviewDepth`). Lifecycle-class changes like this +one are trialed with the seeded-defect skill +(`.claude/skills/review-trial/SKILL.md`). + ### Models and effort per role Each sub-agent pins its model in its own definition inside `review.md` (with a diff --git a/workflows/review/eval/corpus/live.ts b/workflows/review/eval/corpus/live.ts index 9ca12eaf..890e4a9e 100644 --- a/workflows/review/eval/corpus/live.ts +++ b/workflows/review/eval/corpus/live.ts @@ -72,7 +72,19 @@ export type LiveDefectSpec = { */ export type CaseLive = { prContext: LivePrContext; - /** Case-dir-relative path to the post-change tree (default `tree`). */ + /** + * Case-dir-relative path to the post-change tree (default `tree`). + * + * Trees are deliberately minimal: validation requires only the changed + * files to exist. The staged copy of this tree is the sub-agent's whole + * 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: + * include the context files whose absence would change what a reviewer + * concludes (e.g. the decorator module a sibling handler imports), and + * nothing more. + */ tree: string; /** Labeled defects a live run must catch. */ mustCatchSpecs?: LiveDefectSpec[]; diff --git a/workflows/review/eval/lifecycle.test.ts b/workflows/review/eval/lifecycle.test.ts new file mode 100644 index 00000000..92b0bcd9 --- /dev/null +++ b/workflows/review/eval/lifecycle.test.ts @@ -0,0 +1,162 @@ +/** + * Lifecycle replay for the re-review mode dial: the adversarial cases the + * dial must survive, as data (`eval/lifecycle/*.json`), replayed + * deterministically through the same lib functions the production CLI runs. + * + * These cases do not fit the corpus format (a corpus case is one PR state; + * these are push *sequences*), so they live in their own dataset. Each case + * lists the modes it must hold under, a sequence of pushes (each a unified + * diff and the verdict that run submits), and per-push expectations. The + * harness threads the review-body stamp from push to push exactly as + * production does: compute the signature, decide the depth, then stamp the + * simulated review the next push reads. + * + * The two adversarial cases are scored as "the tripwire re-armed and the + * payload got a full review"; the economy control is scored as "the reduced + * path actually ran"; a dial that trips on every fix push is a no-op cost + * lever, so both directions are load-bearing. A live-PR trial + * (`.claude/skills/review-trial/SKILL.md`) exercises the same sequences + * against the real workflow when a lifecycle change ships. + */ + +import {readdirSync, readFileSync} from "node:fs"; +import {join} from "node:path"; + +import {describe, it, expect} from "vitest"; + +import { + computeHunkSignature, + decideReReviewDepth, + findLatestStamp, + renderRereviewStamp, + STAMP_SCHEMA_VERSION, +} from "../lib/rereview-mode"; +import type {PriorReview, ReReviewPlan} from "../lib/rereview-mode"; +import type {ReReviewMode} from "../lib/routing-config"; + +/* -------------------------------------------------------------------------- */ +/* Dataset shape and loading */ +/* -------------------------------------------------------------------------- */ + +type LifecyclePush = { + name: string; + isDraft: boolean; + /** The verdict this run submits (rides into the stamp the next push reads). */ + verdict: string; + /** The push's full unified diff, one line per array entry. */ + diff: string[]; + expected: { + /** A depth, or `same-as-mode`: the configured mode's own cheap path. */ + depth: string; + tripwireRearmed: boolean; + }; +}; + +type LifecycleCase = { + id: string; + tags: string[]; + description: string; + /** The reduced modes the expectations must hold under, each replayed. */ + modes: ReReviewMode[]; + scoring: "payload-full-review" | "reduced-path-taken"; + pushes: LifecyclePush[]; +}; + +const LIFECYCLE_DIR = join(__dirname, "lifecycle"); + +const loadLifecycleCases = (): LifecycleCase[] => + readdirSync(LIFECYCLE_DIR) + .filter((name) => name.endsWith(".json")) + .sort() + .map( + (name) => + JSON.parse( + readFileSync(join(LIFECYCLE_DIR, name), "utf8"), + ) as LifecycleCase, + ); + +/* -------------------------------------------------------------------------- */ +/* Replay */ +/* -------------------------------------------------------------------------- */ + +/** Replay one case under one mode; returns the per-push plans in order. */ +const replay = ( + lifecycleCase: LifecycleCase, + mode: ReReviewMode, +): ReReviewPlan[] => { + const priorReviews: PriorReview[] = []; + const plans: ReReviewPlan[] = []; + for (const [index, push] of lifecycleCase.pushes.entries()) { + const plan = decideReReviewDepth({ + mode, + isDraft: push.isDraft, + priorStamp: findLatestStamp(priorReviews), + currentSignature: computeHunkSignature(push.diff.join("\n")), + }); + plans.push(plan); + priorReviews.push({ + body: renderRereviewStamp({ + schemaVersion: STAMP_SCHEMA_VERSION, + depth: plan.depth, + verdict: push.verdict, + anchorDraft: plan.stampAnchorDraft, + anchorHunks: plan.stampHunks, + }), + submittedAt: `2026-07-0${index + 1}T00:00:00Z`, + }); + } + return plans; +}; + +/* -------------------------------------------------------------------------- */ +/* The suite */ +/* -------------------------------------------------------------------------- */ + +const CASES = loadLifecycleCases(); + +describe("re-review lifecycle dataset", () => { + it("loads the adversarial pair and at least one economy control", () => { + const ids = CASES.map((c) => c.id); + expect(ids).toContain("rereview-rewrite-after-approval"); + expect(ids).toContain("rereview-sparse-pr-then-payload"); + expect(CASES.some((c) => c.scoring === "reduced-path-taken")).toBe( + true, + ); + }); +}); + +describe.each(CASES)("$id", (lifecycleCase) => { + describe.each(lifecycleCase.modes)("under mode %s", (mode) => { + const plans = replay(lifecycleCase, mode); + + it.each( + lifecycleCase.pushes.map((push, index) => ({ + push, + plan: plans[index], + })), + )("push $push.name meets its expected depth", ({push, plan}) => { + const expectedDepth = + push.expected.depth === "same-as-mode" + ? mode + : push.expected.depth; + expect(plan.depth).toBe(expectedDepth); + expect(plan.tripwireRearmed).toBe(push.expected.tripwireRearmed); + }); + + if (lifecycleCase.scoring === "payload-full-review") { + it("scores: the tripwire re-armed and the payload got a full review", () => { + const payload = plans[plans.length - 1]; + expect(payload.tripwireRearmed).toBe(true); + expect(payload.depth).toBe("full"); + expect(payload.dispatch).toBe("all"); + expect(payload.staging).toBe("whole-diff"); + }); + } else { + it("scores: the reduced path actually ran on the follow-up push", () => { + const followUp = plans[plans.length - 1]; + expect(followUp.depth).toBe(mode); + expect(followUp.tripwireRearmed).toBe(false); + }); + } + }); +}); diff --git a/workflows/review/eval/lifecycle/fix-push-stays-cheap.json b/workflows/review/eval/lifecycle/fix-push-stays-cheap.json new file mode 100644 index 00000000..031b075b --- /dev/null +++ b/workflows/review/eval/lifecycle/fix-push-stays-cheap.json @@ -0,0 +1,85 @@ +{ + "id": "rereview-fix-push-stays-cheap", + "tags": ["rereview", "economy"], + "description": "The economics control: a reviewed six-hunk PR gets a routine one-hunk fix push. The unreviewed share (1/6) stays under the tripwire threshold, so each reduced mode actually runs its cheap path; this is the case the dial exists for, and the case the live A/B prices against full. A dial that trips on every fix push would be a no-op cost lever.", + "modes": ["scoped", "flip-gated", "fast"], + "scoring": "reduced-path-taken", + "pushes": [ + { + "name": "initial", + "isDraft": false, + "verdict": "REQUEST_CHANGES", + "diff": [ + "diff --git a/src/models/order.ts b/src/models/order.ts", + "--- a/src/models/order.ts", + "+++ b/src/models/order.ts", + "@@ -5,2 +5,3 @@", + " export type Order = {", + "+ status: OrderStatus;", + " id: string;", + "@@ -20,2 +21,3 @@", + " export const load = (id: string) => {", + "+ metrics.count(\"order.load\");", + " return db.get(id);", + "@@ -40,2 +42,3 @@", + " export const save = (order: Order) => {", + "+ metrics.count(\"order.save\");", + " return db.put(order);", + "diff --git a/src/models/order.test.ts b/src/models/order.test.ts", + "--- a/src/models/order.test.ts", + "+++ b/src/models/order.test.ts", + "@@ -8,2 +8,3 @@", + " describe(\"order\", () => {", + "+ it(\"carries a status\", () => run(statusCase));", + " it(\"loads\", () => run(loadCase));", + "@@ -30,2 +31,3 @@", + " const loadCase = {", + "+ status: \"open\",", + " id: \"o-1\",", + "@@ -50,2 +52,3 @@", + " const saveCase = {", + "+ status: \"open\",", + " id: \"o-2\"," + ], + "expected": {"depth": "full", "tripwireRearmed": false} + }, + { + "name": "review-fix", + "isDraft": false, + "verdict": "APPROVE", + "diff": [ + "diff --git a/src/models/order.ts b/src/models/order.ts", + "--- a/src/models/order.ts", + "+++ b/src/models/order.ts", + "@@ -5,2 +5,3 @@", + " export type Order = {", + "+ status: OrderStatus;", + " id: string;", + "@@ -20,2 +21,3 @@", + " export const load = (id: string) => {", + "+ metrics.count(\"order.load\");", + " return db.get(id);", + "@@ -40,2 +42,3 @@", + " export const save = (order: Order) => {", + "+ metrics.count(\"order.save\", {status: order.status});", + " return db.put(order);", + "diff --git a/src/models/order.test.ts b/src/models/order.test.ts", + "--- a/src/models/order.test.ts", + "+++ b/src/models/order.test.ts", + "@@ -8,2 +8,3 @@", + " describe(\"order\", () => {", + "+ it(\"carries a status\", () => run(statusCase));", + " it(\"loads\", () => run(loadCase));", + "@@ -30,2 +31,3 @@", + " const loadCase = {", + "+ status: \"open\",", + " id: \"o-1\",", + "@@ -50,2 +52,3 @@", + " const saveCase = {", + "+ status: \"open\",", + " id: \"o-2\"," + ], + "expected": {"depth": "same-as-mode", "tripwireRearmed": false} + } + ] +} diff --git a/workflows/review/eval/lifecycle/rewrite-after-approval.json b/workflows/review/eval/lifecycle/rewrite-after-approval.json new file mode 100644 index 00000000..162c4d39 --- /dev/null +++ b/workflows/review/eval/lifecycle/rewrite-after-approval.json @@ -0,0 +1,63 @@ +{ + "id": "rereview-rewrite-after-approval", + "tags": ["rereview", "adversarial"], + "description": "A PR is fully reviewed and approved, then force-pushed as a near-total rewrite. Under every reduced re-review mode the divergence tripwire must re-arm and the rewrite must get a full review; an approval must never ride forward onto content no full review has seen. (Branch protection may also dismiss the stale approval; the fingerprint lives in the review body's hidden stamp, which survives dismissal, so the comparison still runs.)", + "modes": ["scoped", "flip-gated", "fast"], + "scoring": "payload-full-review", + "pushes": [ + { + "name": "initial", + "isDraft": false, + "verdict": "APPROVE", + "diff": [ + "diff --git a/src/service/handler.ts b/src/service/handler.ts", + "--- a/src/service/handler.ts", + "+++ b/src/service/handler.ts", + "@@ -10,2 +10,4 @@", + " export const handle = (req: Request) => {", + "+ const user = requireUser(req);", + "+ audit(user.id, \"handle\");", + " return respond(req);", + "@@ -40,2 +42,3 @@", + " const respond = (req: Request) => {", + "+ validate(req.body);", + " return ok();", + "diff --git a/src/service/config.ts b/src/service/config.ts", + "--- a/src/service/config.ts", + "+++ b/src/service/config.ts", + "@@ -1,2 +1,3 @@", + " export const config = {", + "+ retries: 3,", + " };" + ], + "expected": {"depth": "full", "tripwireRearmed": false} + }, + { + "name": "rewrite-payload", + "isDraft": false, + "verdict": "APPROVE", + "diff": [ + "diff --git a/src/service/handler.ts b/src/service/handler.ts", + "--- a/src/service/handler.ts", + "+++ b/src/service/handler.ts", + "@@ -10,2 +10,4 @@", + " export const handle = (req: Request) => {", + "+ const user = maybeUser(req);", + "+ if (user) audit(user.id, \"handle\");", + " return respond(req);", + "@@ -40,2 +42,3 @@", + " const respond = (req: Request) => {", + "+ validate(req.body, {lenient: true});", + " return ok();", + "diff --git a/src/service/config.ts b/src/service/config.ts", + "--- a/src/service/config.ts", + "+++ b/src/service/config.ts", + "@@ -1,2 +1,3 @@", + " export const config = {", + "+ retries: 30, allowAnonymous: true,", + " };" + ], + "expected": {"depth": "full", "tripwireRearmed": true} + } + ] +} diff --git a/workflows/review/eval/lifecycle/sparse-pr-then-payload.json b/workflows/review/eval/lifecycle/sparse-pr-then-payload.json new file mode 100644 index 00000000..55fde2b5 --- /dev/null +++ b/workflows/review/eval/lifecycle/sparse-pr-then-payload.json @@ -0,0 +1,52 @@ +{ + "id": "rereview-sparse-pr-then-payload", + "tags": ["rereview", "adversarial"], + "description": "An attacker (or an unlucky workflow) opens a sparse, innocuous PR, gets the cheap full review over with, and only then pushes the real payload. The payload dominates the hunk count, so the unreviewed share crosses the tripwire threshold and the payload push must get a full review under every reduced mode; the reduced path never reviews the payload with only a reconcile.", + "modes": ["scoped", "flip-gated", "fast"], + "scoring": "payload-full-review", + "pushes": [ + { + "name": "sparse-decoy", + "isDraft": false, + "verdict": "APPROVE", + "diff": [ + "diff --git a/README.md b/README.md", + "--- a/README.md", + "+++ b/README.md", + "@@ -3,2 +3,3 @@", + " ## Usage", + "+Fixed a typo in the usage section.", + " Run the tool with defaults." + ], + "expected": {"depth": "full", "tripwireRearmed": false} + }, + { + "name": "payload", + "isDraft": false, + "verdict": "APPROVE", + "diff": [ + "diff --git a/README.md b/README.md", + "--- a/README.md", + "+++ b/README.md", + "@@ -3,2 +3,3 @@", + " ## Usage", + "+Fixed a typo in the usage section.", + " Run the tool with defaults.", + "diff --git a/src/billing/charge.ts b/src/billing/charge.ts", + "--- a/src/billing/charge.ts", + "+++ b/src/billing/charge.ts", + "@@ -12,3 +12,3 @@", + " export const charge = (amountCents: number, customer: Customer) => {", + "- assertPositive(amountCents);", + "+ // assertPositive(amountCents);", + " return gateway.charge(amountCents, customer.token);", + "@@ -30,2 +30,4 @@", + " export const refund = (chargeId: string) => {", + "+ const target = lookup(chargeId);", + "+ return gateway.refund(target.id, target.amountCents * 2);", + " };" + ], + "expected": {"depth": "full", "tripwireRearmed": true} + } + ] +} diff --git a/workflows/review/lib/counters.test.ts b/workflows/review/lib/counters.test.ts new file mode 100644 index 00000000..43a98339 --- /dev/null +++ b/workflows/review/lib/counters.test.ts @@ -0,0 +1,71 @@ +import {describe, it, expect} from "vitest"; + +import {computeRunCounters, normalizeRunArtifacts} from "./counters"; +import type {RunArtifacts} from "./counters"; + +const runOf = (over: Partial): RunArtifacts => ({ + runId: "run", + verdict: "APPROVE", + postedCommentCount: 0, + validatorDecisions: [], + ...over, +}); + +describe("computeRunCounters: cost by re-review depth", () => { + it("groups run cost by executed depth; the mode-dial pricing surface", () => { + const counters = computeRunCounters([ + runOf({runId: "a", rereviewDepth: "full", cost: {usd: 9.14}}), + runOf({runId: "b", rereviewDepth: "full", cost: {usd: 8.77}}), + runOf({runId: "c", rereviewDepth: "scoped", cost: {usd: 3.1}}), + runOf({runId: "d", rereviewDepth: "fast", cost: {usd: 0.9}}), + ]); + expect(counters.costByRereviewDepth).toEqual([ + {depth: "fast", runs: 1, totalUsd: 0.9, usdPerRun: 0.9}, + {depth: "full", runs: 2, totalUsd: 17.91, usdPerRun: 8.955}, + {depth: "scoped", runs: 1, totalUsd: 3.1, usdPerRun: 3.1}, + ]); + }); + + it("groups depth-less runs (older reviewer versions) under unrecorded, cost null when unknown", () => { + const counters = computeRunCounters([runOf({runId: "old"})]); + expect(counters.costByRereviewDepth).toEqual([ + {depth: "unrecorded", runs: 1, totalUsd: null, usdPerRun: null}, + ]); + }); + + it("is empty over an empty window", () => { + expect(computeRunCounters([]).costByRereviewDepth).toEqual([]); + }); +}); + +describe("normalizeRunArtifacts: re-review depth", () => { + it("reads the depth from the staged plan artifact", () => { + const run = normalizeRunArtifacts( + { + summary: {verdict: "APPROVE"}, + rereviewPlan: { + depth: "flip-gated", + dispatch: "reconcile+correctness", + }, + }, + "fallback", + ); + expect(run.rereviewDepth).toBe("flip-gated"); + }); + + it("falls back to a summary-recorded depth when no plan is uploaded", () => { + const run = normalizeRunArtifacts( + {summary: {verdict: "APPROVE", rereviewDepth: "scoped"}}, + "fallback", + ); + expect(run.rereviewDepth).toBe("scoped"); + }); + + it("leaves the field absent when neither source records one", () => { + const run = normalizeRunArtifacts( + {summary: {verdict: "APPROVE"}}, + "fallback", + ); + expect(run.rereviewDepth).toBeUndefined(); + }); +}); diff --git a/workflows/review/lib/counters.ts b/workflows/review/lib/counters.ts index 0a5ed351..ce9435de 100644 --- a/workflows/review/lib/counters.ts +++ b/workflows/review/lib/counters.ts @@ -78,6 +78,13 @@ export type RunArtifacts = { thumbs?: ThumbsTally; /** Model cost/usage for the run (absent when the log has none). */ cost?: RunCost; + /** + * The re-review depth the run executed (`rereview-plan.json`: + * `full`/`scoped`/`flip-gated`/`fast`; absent for a run that predates the + * mode dial). This is what lets the cost counters price the dial: the + * runs-per-PR lever is judged in dollars per depth, not per run. + */ + rereviewDepth?: string; }; /* -------------------------------------------------------------------------- */ @@ -114,6 +121,19 @@ export type CostCounter = { tokensPerRun: number | null; }; +/** + * Cost per executed re-review depth: the measurement surface for the + * re-review mode dial (price `scoped` against `full` in dollars; recall is + * the eval suite's half). Runs that recorded no depth (older reviewer + * versions) are grouped under `unrecorded` so the window still adds up. + */ +export type DepthCostCounter = { + depth: string; + runs: number; + totalUsd: number | null; + usdPerRun: number | null; +}; + /** The full set of live counters over a window of runs. */ export type RunCounters = { /** Number of runs aggregated. */ @@ -132,6 +152,8 @@ export type RunCounters = { thumbs: ThumbsCounter; /** Cost across the window. */ cost: CostCounter; + /** Cost grouped by executed re-review depth, sorted by `depth`. */ + costByRereviewDepth: DepthCostCounter[]; }; /** A fresh verdict histogram with every event key initialised to `0`. */ @@ -241,6 +263,37 @@ export const computeRunCounters = ( tokensPerRun: tokensSeen && runCount > 0 ? tokensSum / runCount : null, }; + // Cost per executed re-review depth (the mode-dial pricing surface). + const perDepth = new Map< + string, + {runs: number; usdSum: number; usdSeen: boolean} + >(); + for (const run of runs) { + const depth = run.rereviewDepth ?? "unrecorded"; + const entry = perDepth.get(depth) ?? { + runs: 0, + usdSum: 0, + usdSeen: false, + }; + entry.runs += 1; + if (run.cost?.usd !== undefined) { + entry.usdSum += run.cost.usd; + entry.usdSeen = true; + } + perDepth.set(depth, entry); + } + const costByRereviewDepth: DepthCostCounter[] = [...perDepth.entries()] + .map(([depth, entry]) => ({ + depth, + runs: entry.runs, + totalUsd: entry.usdSeen ? entry.usdSum : null, + usdPerRun: + entry.usdSeen && entry.runs > 0 + ? entry.usdSum / entry.runs + : null, + })) + .sort((a, b) => (a.depth < b.depth ? -1 : a.depth > b.depth ? 1 : 0)); + return { runCount, validatorDropBySource, @@ -250,6 +303,7 @@ export const computeRunCounters = ( verdictMix, thumbs, cost, + costByRereviewDepth, }; }; @@ -285,6 +339,8 @@ export type RawRunArtifacts = { validator?: unknown; /** The run summary: verdict, posted-comment count, thumbs, cost. */ summary?: unknown; + /** `out/rereview-plan.json`: the executed re-review plan (mode dial). */ + rereviewPlan?: unknown; }; const extractValidatorEntries = (validator: unknown): unknown[] => { @@ -421,6 +477,21 @@ export const normalizeRunArtifacts = ( const thumbs = normalizeThumbs(summary); const cost = normalizeCost(summary); + // The executed re-review depth: the staged plan artifact is + // authoritative; a summary-recorded depth is the fallback for arms that + // upload no plan. Any non-string (or missing) value leaves the field + // absent, which the counters group as `unrecorded`. + const planDepth = isRecord(raw.rereviewPlan) + ? raw.rereviewPlan["depth"] + : undefined; + const summaryDepth = summary["rereviewDepth"]; + const rereviewDepth = + typeof planDepth === "string" && planDepth.length > 0 + ? planDepth + : typeof summaryDepth === "string" && summaryDepth.length > 0 + ? summaryDepth + : undefined; + const run: RunArtifacts = { runId, verdict, @@ -433,6 +504,9 @@ export const normalizeRunArtifacts = ( if (cost !== undefined) { run.cost = cost; } + if (rereviewDepth !== undefined) { + run.rereviewDepth = rereviewDepth; + } return run; }; @@ -448,6 +522,8 @@ export type RunArtifactLayout = { validator: string; /** The run summary (verdict, comments, thumbs, cost), relative to the run dir. */ summary: string; + /** The executed re-review plan (mode dial), relative to the run dir. */ + rereviewPlan: string; }; /** @@ -461,6 +537,7 @@ export const DEFAULT_RUN_ARTIFACT_LAYOUT: RunArtifactLayout = { claims: "claims.json", validator: "out/claim-validator.json", summary: "summary.json", + rereviewPlan: "out/rereview-plan.json", }; const readJsonIfPresent = (path: string): unknown => { @@ -490,6 +567,7 @@ export const readRunArtifactsFromDir = ( claims: readJsonIfPresent(join(dir, layout.claims)), validator: readJsonIfPresent(join(dir, layout.validator)), summary: readJsonIfPresent(join(dir, layout.summary)), + rereviewPlan: readJsonIfPresent(join(dir, layout.rereviewPlan)), }; return normalizeRunArtifacts(raw, dir); }; diff --git a/workflows/review/lib/rereview-mode.test.ts b/workflows/review/lib/rereview-mode.test.ts new file mode 100644 index 00000000..2523d137 --- /dev/null +++ b/workflows/review/lib/rereview-mode.test.ts @@ -0,0 +1,594 @@ +import {describe, it, expect} from "vitest"; + +import { + buildScopedDiff, + computeDivergence, + computeHunkSignature, + decideReReviewDepth, + DEFAULT_TRIPWIRE_THRESHOLD, + findLatestStamp, + MAX_STAMP_HUNKS_B64_CHARS, + parseRereviewStamp, + renderRereviewStamp, + runRereviewPlanCli, + runRereviewStampCli, + STAMP_SCHEMA_VERSION, +} from "./rereview-mode"; +import type {HunkSignature, ReReviewStamp} from "./rereview-mode"; + +/* -------------------------------------------------------------------------- */ +/* Diff fixtures */ +/* -------------------------------------------------------------------------- */ + +/** A one-file, one-hunk diff with the given added line, at the given offset. */ +const diffFor = (path: string, addedLine: string, startLine = 1): string => + [ + `diff --git a/${path} b/${path}`, + `--- a/${path}`, + `+++ b/${path}`, + `@@ -${startLine},2 +${startLine},3 @@`, + " context", + `+${addedLine}`, + " more", + ].join("\n"); + +const TWO_HUNK_DIFF = [ + "diff --git a/src/handler.ts b/src/handler.ts", + "--- a/src/handler.ts", + "+++ b/src/handler.ts", + "@@ -1,2 +1,3 @@", + " a", + "+first hunk line", + " b", + "@@ -10,2 +11,3 @@", + " c", + "+second hunk line", + " d", +].join("\n"); + +/* -------------------------------------------------------------------------- */ +/* Hunk signatures */ +/* -------------------------------------------------------------------------- */ + +describe("computeHunkSignature", () => { + it("hashes one entry per hunk, per file", () => { + const signature = computeHunkSignature(TWO_HUNK_DIFF); + expect(Object.keys(signature)).toEqual(["src/handler.ts"]); + expect(signature["src/handler.ts"]).toHaveLength(2); + expect(signature["src/handler.ts"][0]).not.toBe( + signature["src/handler.ts"][1], + ); + }); + + it("is stable across rebases: shifted line numbers and context do not move the hash", () => { + const before = diffFor("src/a.ts", "the same added line", 1); + const rebased = [ + "diff --git a/src/a.ts b/src/a.ts", + "--- a/src/a.ts", + "+++ b/src/a.ts", + "@@ -41,2 +41,3 @@", + " completely different context", + "+the same added line", + " other surroundings", + ].join("\n"); + expect(computeHunkSignature(before)).toEqual( + computeHunkSignature(rebased), + ); + }); + + it("moves when added content changes", () => { + expect(computeHunkSignature(diffFor("src/a.ts", "one"))).not.toEqual( + computeHunkSignature(diffFor("src/a.ts", "two")), + ); + }); + + it("moves on a deletion-only change (removed lines are hashed too)", () => { + const removalA = [ + "diff --git a/src/a.ts b/src/a.ts", + "--- a/src/a.ts", + "+++ b/src/a.ts", + "@@ -1,3 +1,2 @@", + " keep", + "-drop the auth guard", + " keep too", + ].join("\n"); + const removalB = removalA.replace( + "-drop the auth guard", + "-drop a comment", + ); + expect(computeHunkSignature(removalA)).not.toEqual( + computeHunkSignature(removalB), + ); + }); + + it("trims trailing whitespace so CRLF or trailing-space churn does not move the hash", () => { + const withTrailing = diffFor("src/a.ts", "line\t "); + const clean = diffFor("src/a.ts", "line"); + expect(computeHunkSignature(withTrailing)).toEqual( + computeHunkSignature(clean), + ); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* Divergence */ +/* -------------------------------------------------------------------------- */ + +describe("computeDivergence", () => { + const current = computeHunkSignature(TWO_HUNK_DIFF); + + it("reports zero unreviewed when the fingerprint matches", () => { + const d = computeDivergence(current, current); + expect(d).toEqual({ + totalHunks: 2, + unreviewedHunks: 0, + unreviewedShare: 0, + }); + }); + + it("counts a path absent from the fingerprint as entirely unreviewed", () => { + const d = computeDivergence(current, {}); + expect(d.unreviewedHunks).toBe(2); + expect(d.unreviewedShare).toBe(1); + }); + + it("ignores fingerprint hunks that are gone from the current diff", () => { + const reviewed: HunkSignature = { + "src/handler.ts": [ + ...current["src/handler.ts"], + "deadbeef00000000", + ], + "src/deleted.ts": ["cafecafe00000000"], + }; + const d = computeDivergence(current, reviewed); + expect(d).toEqual({ + totalHunks: 2, + unreviewedHunks: 0, + unreviewedShare: 0, + }); + }); + + it("returns share 0 for an empty current diff", () => { + expect(computeDivergence({}, current).unreviewedShare).toBe(0); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* The stamp */ +/* -------------------------------------------------------------------------- */ + +const stampOf = (over: Partial = {}): ReReviewStamp => ({ + schemaVersion: STAMP_SCHEMA_VERSION, + depth: "full", + verdict: "APPROVE", + anchorDraft: false, + anchorHunks: computeHunkSignature(TWO_HUNK_DIFF), + ...over, +}); + +describe("stamp render/parse", () => { + it("round-trips through a review body", () => { + const stamp = stampOf(); + const body = `Approved — no blocking issues found.\n\n${renderRereviewStamp( + stamp, + )}`; + expect(parseRereviewStamp(body)).toEqual(stamp); + }); + + it("round-trips a REQUEST_CHANGES verdict and a draft anchor", () => { + const stamp = stampOf({ + depth: "scoped", + verdict: "REQUEST_CHANGES", + anchorDraft: true, + }); + expect(parseRereviewStamp(renderRereviewStamp(stamp))).toEqual(stamp); + }); + + it("returns null on a body with no stamp", () => { + expect(parseRereviewStamp("just an ordinary review body")).toBeNull(); + }); + + it("returns null on an unknown schema version (future writer)", () => { + const body = renderRereviewStamp(stampOf()).replace(" v=1 ", " v=2 "); + expect(parseRereviewStamp(body)).toBeNull(); + }); + + it("returns null when the fingerprint payload does not decode", () => { + const body = renderRereviewStamp(stampOf()).replace( + /hunks=\S+/, + "hunks=!!!not-base64!!!", + ); + expect(parseRereviewStamp(body)).toBeNull(); + }); + + it("renders overflow when the signature exceeds the cap, and parses it back", () => { + const huge: HunkSignature = {}; + for (let i = 0; i < 3000; i++) { + huge[`src/file-${i}.ts`] = ["0123456789abcdef"]; + } + const rendered = renderRereviewStamp(stampOf({anchorHunks: huge})); + expect(rendered).toContain("hunks=overflow"); + expect(rendered.length).toBeLessThan(MAX_STAMP_HUNKS_B64_CHARS); + expect(parseRereviewStamp(rendered)?.anchorHunks).toBe("overflow"); + }); + + it("takes the last stamp when a body carries more than one", () => { + const first = renderRereviewStamp(stampOf({depth: "full"})); + const second = renderRereviewStamp(stampOf({depth: "fast"})); + expect(parseRereviewStamp(`${first}\n${second}`)?.depth).toBe("fast"); + }); +}); + +describe("findLatestStamp", () => { + it("prefers the newest stamped review by submittedAt", () => { + const older = renderRereviewStamp( + stampOf({verdict: "REQUEST_CHANGES"}), + ); + const newer = renderRereviewStamp(stampOf({verdict: "APPROVE"})); + const found = findLatestStamp([ + {body: newer, submittedAt: "2026-07-09T10:00:00Z"}, + {body: older, submittedAt: "2026-07-01T10:00:00Z"}, + ]); + expect(found?.verdict).toBe("APPROVE"); + }); + + it("skips stampless reviews (pre-dial history) and unstamped chatter", () => { + const stamped = renderRereviewStamp(stampOf()); + const found = findLatestStamp([ + {body: stamped, submittedAt: "2026-07-01T10:00:00Z"}, + {body: "LGTM from a human", submittedAt: "2026-07-09T10:00:00Z"}, + ]); + expect(found).not.toBeNull(); + }); + + it("returns null when no review carries a stamp", () => { + expect(findLatestStamp([{body: "Approved."}])).toBeNull(); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* The depth decision */ +/* -------------------------------------------------------------------------- */ + +const CURRENT = computeHunkSignature(TWO_HUNK_DIFF); + +describe("decideReReviewDepth", () => { + it("mode full always runs full", () => { + const plan = decideReReviewDepth({ + mode: "full", + isDraft: false, + priorStamp: stampOf(), + currentSignature: CURRENT, + }); + expect(plan.depth).toBe("full"); + expect(plan.reasons).toEqual(["mode-full"]); + expect(plan.dispatch).toBe("all"); + expect(plan.staging).toBe("whole-diff"); + }); + + it("no prior fingerprint runs full (first review, or pre-dial history)", () => { + const plan = decideReReviewDepth({ + mode: "fast", + isDraft: false, + priorStamp: null, + currentSignature: CURRENT, + }); + expect(plan.depth).toBe("full"); + expect(plan.reasons).toEqual(["no-prior-fingerprint"]); + expect(plan.stampHunks).toEqual(CURRENT); + }); + + it("a stamped COMMENTED review anchors normally; no full-forever loop", () => { + const plan = decideReReviewDepth({ + mode: "fast", + isDraft: false, + priorStamp: stampOf({verdict: "COMMENT", anchorHunks: CURRENT}), + currentSignature: CURRENT, + }); + expect(plan.depth).toBe("fast"); + }); + + it("ready-for-review anchor: a draft-taken fingerprint forces one full ready review", () => { + const plan = decideReReviewDepth({ + mode: "scoped", + isDraft: false, + priorStamp: stampOf({anchorDraft: true, anchorHunks: CURRENT}), + currentSignature: CURRENT, + }); + expect(plan.depth).toBe("full"); + expect(plan.reasons).toEqual(["ready-for-review-anchor"]); + // The fresh stamp anchors on the ready PR. + expect(plan.stampAnchorDraft).toBe(false); + }); + + it("draft pushes may stay cheap against a draft-taken fingerprint", () => { + const plan = decideReReviewDepth({ + mode: "fast", + isDraft: true, + priorStamp: stampOf({anchorDraft: true, anchorHunks: CURRENT}), + currentSignature: CURRENT, + }); + expect(plan.depth).toBe("fast"); + expect(plan.stampAnchorDraft).toBe(true); + }); + + it("an overflowed fingerprint forces full", () => { + const plan = decideReReviewDepth({ + mode: "fast", + isDraft: false, + priorStamp: stampOf({anchorHunks: "overflow"}), + currentSignature: CURRENT, + }); + expect(plan.depth).toBe("full"); + expect(plan.reasons).toEqual(["fingerprint-overflow"]); + }); + + it("re-arms full when the unreviewed share crosses the threshold", () => { + const plan = decideReReviewDepth({ + mode: "fast", + isDraft: false, + priorStamp: stampOf({anchorHunks: {}}), + currentSignature: CURRENT, + }); + expect(plan.depth).toBe("full"); + expect(plan.tripwireRearmed).toBe(true); + expect(plan.divergence?.unreviewedShare).toBe(1); + // The tripped run stamps the current signature: the loop re-converges. + expect(plan.stampHunks).toEqual(CURRENT); + }); + + it("stays cheap under the threshold and maps each mode to its shape", () => { + const anchored = stampOf({anchorHunks: CURRENT}); + const base = { + isDraft: false, + priorStamp: anchored, + currentSignature: CURRENT, + } as const; + + const scoped = decideReReviewDepth({...base, mode: "scoped"}); + expect(scoped.depth).toBe("scoped"); + expect(scoped.dispatch).toBe("all"); + expect(scoped.staging).toBe("new-hunks"); + expect(scoped.flipGate).toBe(false); + expect(scoped.stampHunks).toEqual(CURRENT); + + const flipGated = decideReReviewDepth({...base, mode: "flip-gated"}); + expect(flipGated.depth).toBe("flip-gated"); + expect(flipGated.dispatch).toBe("reconcile+correctness"); + expect(flipGated.staging).toBe("new-hunks"); + expect(flipGated.flipGate).toBe(true); + + const fast = decideReReviewDepth({...base, mode: "fast"}); + expect(fast.depth).toBe("fast"); + expect(fast.dispatch).toBe("reconcile-only"); + expect(fast.staging).toBe("none"); + expect(fast.flipGate).toBe(false); + }); + + it("fast and flip-gated carry the anchor fingerprint forward, so drift accumulates", () => { + // Push 2: one of the two reviewed hunks still present, one new hunk. + const drifted: HunkSignature = { + "src/handler.ts": [ + CURRENT["src/handler.ts"][0], + "aaaaaaaaaaaaaaaa", + ], + }; + const push2 = decideReReviewDepth({ + mode: "fast", + isDraft: false, + priorStamp: stampOf({anchorHunks: CURRENT}), + currentSignature: drifted, + }); + expect(push2.divergence?.unreviewedShare).toBe(0.5); + expect(push2.depth).toBe("full"); // 0.5 >= 0.4: already re-armed + expect(DEFAULT_TRIPWIRE_THRESHOLD).toBeLessThanOrEqual(0.5); + + // With a higher threshold the fast path carries the OLD anchor + // forward; it does not launder the drifted push into the fingerprint. + const carried = decideReReviewDepth({ + mode: "fast", + isDraft: false, + priorStamp: stampOf({anchorHunks: CURRENT}), + currentSignature: drifted, + tripwireThreshold: 0.9, + }); + expect(carried.depth).toBe("fast"); + expect(carried.stampHunks).toEqual(CURRENT); + }); + + it("a dismissed approval's stamp still anchors (verdict lives in the stamp, not the state)", () => { + // dismiss-stale-approvals rewrites state to DISMISSED; the reader + // never sees state, only the body, so the fingerprint and the prior + // verdict both survive. + const dismissedBody = `Approved — no blocking issues found.\n${renderRereviewStamp( + stampOf({verdict: "APPROVE", anchorHunks: CURRENT}), + )}`; + const stamp = findLatestStamp([{body: dismissedBody}]); + expect(stamp?.verdict).toBe("APPROVE"); + const plan = decideReReviewDepth({ + mode: "flip-gated", + isDraft: false, + priorStamp: stamp, + currentSignature: CURRENT, + }); + expect(plan.depth).toBe("flip-gated"); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* Scoped-diff staging */ +/* -------------------------------------------------------------------------- */ + +describe("buildScopedDiff", () => { + it("keeps only the hunks the fingerprint has not seen", () => { + const reviewed: HunkSignature = { + "src/handler.ts": [CURRENT["src/handler.ts"][0]], + }; + const scoped = buildScopedDiff(TWO_HUNK_DIFF, reviewed); + expect(scoped).toContain("second hunk line"); + expect(scoped).not.toContain("first hunk line"); + expect(scoped).toContain( + "diff --git a/src/handler.ts b/src/handler.ts", + ); + }); + + it("drops files that are fully reviewed", () => { + expect(buildScopedDiff(TWO_HUNK_DIFF, CURRENT)).toBe(""); + }); + + it("keeps whole new files", () => { + const scoped = buildScopedDiff(TWO_HUNK_DIFF, {}); + expect(scoped).toBe(TWO_HUNK_DIFF); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* CLI staging */ +/* -------------------------------------------------------------------------- */ + +type FakeFs = { + files: Map; + readFileSync: (p: string, enc: "utf8") => string; + writeFileSync: (p: string, data: string) => void; + existsSync: (p: string) => boolean; + mkdirSync: (p: string, opts: {recursive: boolean}) => void; +}; + +const fakeFs = (seed: Record): FakeFs => { + const files = new Map(Object.entries(seed)); + return { + files, + readFileSync: (p) => { + const content = files.get(p); + if (content === undefined) { + throw new Error(`ENOENT: ${p}`); + } + return content; + }, + writeFileSync: (p, data) => void files.set(p, data), + existsSync: (p) => files.has(p), + mkdirSync: () => undefined, + }; +}; + +const REVIEW_DIR = "/tmp/gh-aw/review"; + +const stagedInputs = ( + over: Record = {}, +): Record => ({ + [`${REVIEW_DIR}/full.diff`]: TWO_HUNK_DIFF, + [`${REVIEW_DIR}/routing.json`]: JSON.stringify({reReviewMode: "fast"}), + [`${REVIEW_DIR}/pr-context.json`]: JSON.stringify({isDraft: false}), + [`${REVIEW_DIR}/prior-reviews.json`]: JSON.stringify([ + { + body: renderRereviewStamp(stampOf({anchorHunks: CURRENT})), + submittedAt: "2026-07-01T00:00:00Z", + }, + ]), + ...over, +}); + +describe("runRereviewPlanCli", () => { + it("stages a fast plan when the fingerprint matches", () => { + const fs = fakeFs(stagedInputs()); + const {plan, warnings} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("fast"); + expect(warnings).toEqual([]); + expect(fs.files.has(`${REVIEW_DIR}/rereview-plan.json`)).toBe(true); + expect(fs.files.has(`${REVIEW_DIR}/scoped.diff`)).toBe(false); + }); + + it("writes scoped.diff for a new-hunks plan", () => { + // Three hunks, one of them new since the fingerprint: share 1/3 stays + // under the tripwire threshold, so the scoped path actually runs. + const threeHunkDiff = [ + TWO_HUNK_DIFF, + "@@ -20,2 +22,3 @@", + " e", + "+third hunk line", + " f", + ].join("\n"); + const reviewedFirstTwo: HunkSignature = { + "src/handler.ts": CURRENT["src/handler.ts"], + }; + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/full.diff`]: threeHunkDiff, + [`${REVIEW_DIR}/routing.json`]: JSON.stringify({ + reReviewMode: "scoped", + }), + [`${REVIEW_DIR}/prior-reviews.json`]: JSON.stringify([ + { + body: renderRereviewStamp( + stampOf({anchorHunks: reviewedFirstTwo}), + ), + }, + ]), + }), + ); + const {plan} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("scoped"); + expect(plan.divergence?.unreviewedShare).toBeCloseTo(1 / 3); + const scoped = fs.files.get(`${REVIEW_DIR}/scoped.diff`); + expect(scoped).toContain("third hunk line"); + expect(scoped).not.toContain("first hunk line"); + expect(scoped).not.toContain("second hunk line"); + }); + + it("prefers the generated-stripped diff so lockfile churn never counts as divergence", () => { + // full.diff carries an extra (generated) file's hunk; the stripped + // diff does not. The fingerprint must come from the stripped diff: + // against an anchor over the same two source hunks, divergence is 0. + const lockfileHunk = [ + "diff --git a/package-lock.json b/package-lock.json", + "--- a/package-lock.json", + "+++ b/package-lock.json", + "@@ -1,2 +1,3 @@", + ' "packages": {', + '+ "left-pad": "2.0.0",', + " }", + ].join("\n"); + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/full.diff`]: `${TWO_HUNK_DIFF}\n${lockfileHunk}`, + [`${REVIEW_DIR}/full-stripped.diff`]: TWO_HUNK_DIFF, + }), + ); + const {plan} = runRereviewPlanCli(fs); + expect(plan.divergence?.unreviewedShare).toBe(0); + expect(plan.depth).toBe("fast"); + }); + + it("degrades every missing input to a full plan with a warning, never a crash", () => { + const fs = fakeFs({}); + const {plan, warnings} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("full"); + expect(warnings.length).toBeGreaterThan(0); + }); + + it("degrades an unparseable routing.json to full", () => { + const fs = fakeFs( + stagedInputs({[`${REVIEW_DIR}/routing.json`]: "{not json"}), + ); + const {plan} = runRereviewPlanCli(fs); + expect(plan.mode).toBe("full"); + }); +}); + +describe("runRereviewStampCli", () => { + it("renders this run's stamp from the staged plan and the decided verdict", () => { + const fs = fakeFs(stagedInputs()); + runRereviewPlanCli(fs); + const stamp = runRereviewStampCli(fs, "APPROVE"); + expect(stamp).not.toBeNull(); + const parsed = parseRereviewStamp(stamp ?? ""); + expect(parsed?.depth).toBe("fast"); + expect(parsed?.verdict).toBe("APPROVE"); + // The fast run carried the anchor fingerprint forward verbatim. + expect(parsed?.anchorHunks).toEqual(CURRENT); + }); + + it("returns null when no plan is staged", () => { + expect(runRereviewStampCli(fakeFs({}), "APPROVE")).toBeNull(); + }); +}); diff --git a/workflows/review/lib/rereview-mode.ts b/workflows/review/lib/rereview-mode.ts new file mode 100644 index 00000000..ce1f6f24 --- /dev/null +++ b/workflows/review/lib/rereview-mode.ts @@ -0,0 +1,721 @@ +/** + * The re-review mode dial: deterministic depth selection for repeat reviews + * of the same PR. + * + * Measured lifecycles of one seeded PR paid the full 11-agent fan-out on + * every push, with per-run cost rising as threads accumulate ($7.19 → $9.14 + * across three runs of v1.3.1; $8.31 → $11.29 on v1.4.0), and the approval + * run (the one that emits the least) was the most expensive. This module is + * the runs-per-PR cost lever: a per-repo `re-review` mode in the ROUTING file + * (`full` | `scoped` | `flip-gated` | `fast`, default `full`) decides how + * much of the roster a repeat review runs. See {@link ReReviewMode} in + * `routing-config.ts` for what each mode does. + * + * Three guards keep the cheaper modes honest: + * + * 1. **Ready-for-review anchor.** The one full review the cheaper modes + * lean on must have reviewed the PR *ready*, never a draft skeleton: a + * fingerprint stamped on a draft forces one more full review when the + * PR leaves draft. + * 2. **Flip gate.** `flip-gated` dispatches the correctness pass alongside + * reconciliation, and a REQUEST_CHANGES→APPROVE flip is vetoed by any + * validated blocking finding from that pass; the findings gate the + * flip instead of being discarded. + * 3. **Divergence tripwire.** Every full-depth review stamps a + * content-hashed hunk signature into its review body as a hidden + * comment (so it survives cache eviction AND branch protection's + * dismiss-stale-approvals; a dismissed review keeps its body). Each + * later push compares its current signature against that last + * fully-reviewed fingerprint; when the unreviewed share crosses + * {@link DEFAULT_TRIPWIRE_THRESHOLD}, full-review mode re-arms and the + * divergent push gets the whole roster. + * + * Two interactions are handled by construction: + * + * - **dismiss-stale-approvals**: the prior verdict is read from the stamp, + * not the review's `state`, so a dismissed approval still anchors the + * fingerprint and still reads as an approval to flip logic. + * - **COMMENTED-only history**: the marker of "a full review happened" is + * the stamp itself, whatever the review state, so a PR whose only prior + * bot review is COMMENTED (e.g. a comment-only A/B arm) reduces normally + * after its first stamped run instead of re-running full forever. + * + * The hunk signature hashes each hunk's added AND removed lines (markers + * kept, trailing whitespace trimmed), so it is stable across rebases, + * squashes, and base merges (which rewrite SHAs and shift line numbers but + * not the change's content), while a deletion-only payload still moves the + * fingerprint. (Step 1's older added-lines-only prompt definition is + * superseded by this CLI; the one-time mismatch on upgrade degrades to a + * full review, never a skipped one.) + * + * Every failure degrades toward `full`: toward more review, never less. + * + * Determinism boundary: pure functions of the diff text, the ROUTING mode, + * and the stamped fingerprint; no model call, no clock, no prose about the + * code under review. + */ + +import {createHash} from "node:crypto"; + +import {splitUnifiedDiff} from "./diff"; +import {DEFAULT_RE_REVIEW_MODE, RE_REVIEW_MODES} from "./routing-config"; +import type {ReReviewMode} from "./routing-config"; + +/* -------------------------------------------------------------------------- */ +/* Hunk signatures */ +/* -------------------------------------------------------------------------- */ + +/** + * Per-file content-hashed hunk signature: `path → [hunkHash, …]`, one + * truncated SHA-256 per hunk over its `+`/`-` lines (markers kept, trailing + * whitespace trimmed). Content-based, so force-pushes and rebases that do not + * change what the diff adds or removes do not move the signature. + */ +export type HunkSignature = Record; + +/** Truncation keeps the stamp compact; 16 hex chars ≈ 64 bits per hunk. */ +const HUNK_HASH_CHARS = 16; + +const HUNK_HEADER_RE = /^@@ -\d+(?:,\d+)? \+\d+(?:,\d+)? @@/; + +/** Split one file section's text into its hunk bodies (headers included). */ +const splitHunks = (sectionText: string): string[] => { + const hunks: string[] = []; + let current: string[] | null = null; + for (const line of sectionText.split("\n")) { + if (HUNK_HEADER_RE.test(line)) { + if (current !== null) { + hunks.push(current.join("\n")); + } + current = [line]; + } else if (current !== null) { + current.push(line); + } + } + if (current !== null) { + hunks.push(current.join("\n")); + } + return hunks; +}; + +const hashHunk = (hunkText: string): string => { + const content = hunkText + .split("\n") + .filter((line) => line.startsWith("+") || line.startsWith("-")) + .map((line) => line.replace(/\s+$/, "")) + .join("\n"); + return createHash("sha256") + .update(content) + .digest("hex") + .slice(0, HUNK_HASH_CHARS); +}; + +/** + * Compute the hunk signature of a unified diff. Pure: same diff text, same + * signature. Files whose section carries no hunks (e.g. a binary file's + * header-only section) get an empty list. + */ +export const computeHunkSignature = (diffText: string): HunkSignature => { + const signature: HunkSignature = {}; + for (const section of splitUnifiedDiff(diffText)) { + signature[section.path] = splitHunks(section.text).map(hashHunk); + } + return signature; +}; + +/* -------------------------------------------------------------------------- */ +/* Divergence */ +/* -------------------------------------------------------------------------- */ + +/** How far the current diff has drifted from the anchoring fingerprint. */ +export type Divergence = { + /** Hunks in the current diff. */ + totalHunks: number; + /** Current hunks whose hash the fingerprint does not contain. */ + unreviewedHunks: number; + /** `unreviewedHunks / totalHunks` (0 when the diff has no hunks). */ + unreviewedShare: number; +}; + +/** + * Unreviewed share at or above which the tripwire re-arms a full review. + * Sized so a routine fix push on a reviewed PR (a small fraction of its + * hunks) stays cheap while a rewrite-after-approval (share 1.0) or a payload + * 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; + +/** + * Compare the current signature against the last fully-reviewed fingerprint. + * A hunk is unreviewed when its hash is absent from the fingerprint's set + * for that path (a path absent from the fingerprint is entirely unreviewed). + * Hunks that existed at the last full review and are gone now do not count: + * share measures what the current diff contains that no full review saw. + */ +export const computeDivergence = ( + current: HunkSignature, + reviewed: HunkSignature, +): Divergence => { + let totalHunks = 0; + let unreviewedHunks = 0; + for (const [path, hashes] of Object.entries(current)) { + const seen = new Set(reviewed[path] ?? []); + for (const hash of hashes) { + totalHunks++; + if (!seen.has(hash)) { + unreviewedHunks++; + } + } + } + return { + totalHunks, + unreviewedHunks, + unreviewedShare: totalHunks === 0 ? 0 : unreviewedHunks / totalHunks, + }; +}; + +/* -------------------------------------------------------------------------- */ +/* The review-body stamp */ +/* -------------------------------------------------------------------------- */ + +/** The depth a run actually executed (a mode may be overridden by a guard). */ +export type ReReviewDepth = ReReviewMode; + +/** + * The hidden-comment stamp a review body carries. `anchorHunks` is the last + * fully-reviewed fingerprint (refreshed by a `full`/`scoped` run, carried + * forward verbatim by `flip-gated`/`fast`), or `"overflow"` when the + * signature was too large to stamp; `anchorDraft` is whether the PR was a + * draft when that fingerprint was taken. `verdict` is the submitted event of + * THIS review; kept in the stamp because branch protection's + * dismiss-stale-approvals rewrites the review's `state` to DISMISSED and the + * original verdict becomes unreadable from the API listing. + */ +export type ReReviewStamp = { + schemaVersion: number; + depth: ReReviewDepth; + verdict: string; + anchorDraft: boolean; + anchorHunks: HunkSignature | "overflow"; +}; + +export const STAMP_SCHEMA_VERSION = 1; + +const STAMP_MARKER = "pr-reviewer:rereview"; + +/** + * Cap on the base64 fingerprint payload. A review body holds 65536 chars and + * must also carry the verdict text, note lines, and any accountability + * rendering; past the cap the stamp records `hunks=overflow`, which readers + * treat as "fingerprint unavailable" and answer with a full review. + */ +export const MAX_STAMP_HUNKS_B64_CHARS = 20000; + +/** Stable serialization: sorted paths, so equal signatures encode equally. */ +const encodeSignature = (signature: HunkSignature): string => { + const sorted: HunkSignature = {}; + for (const path of Object.keys(signature).sort()) { + sorted[path] = signature[path]; + } + return Buffer.from(JSON.stringify(sorted), "utf8").toString("base64"); +}; + +const decodeSignature = (encoded: string): HunkSignature | null => { + let parsed: unknown; + try { + parsed = JSON.parse(Buffer.from(encoded, "base64").toString("utf8")); + } catch { + return null; + } + if ( + parsed === null || + typeof parsed !== "object" || + Array.isArray(parsed) + ) { + return null; + } + const signature: HunkSignature = {}; + for (const [path, hashes] of Object.entries(parsed)) { + if ( + !Array.isArray(hashes) || + !hashes.every((hash) => typeof hash === "string") + ) { + return null; + } + signature[path] = hashes as string[]; + } + return signature; +}; + +/** Render the stamp as the hidden HTML comment the review body carries. */ +export const renderRereviewStamp = (stamp: ReReviewStamp): string => { + let hunksField: string; + if (stamp.anchorHunks === "overflow") { + hunksField = "overflow"; + } else { + const encoded = encodeSignature(stamp.anchorHunks); + hunksField = + encoded.length > MAX_STAMP_HUNKS_B64_CHARS ? "overflow" : encoded; + } + return ( + `` + ); +}; + +const STAMP_RE = new RegExp( + ``, + "g", +); + +/** + * Parse the LAST stamp in a review body (a body carries at most one, but + * last-wins keeps the reader deterministic if a future writer appends). + * Returns null when there is no stamp, the schema version is unknown, the + * depth is not a known mode, or the fingerprint payload does not decode; + * all of which the caller must treat as "no usable fingerprint" (→ full). + */ +export const parseRereviewStamp = (body: string): ReReviewStamp | null => { + let match: RegExpExecArray | null = null; + STAMP_RE.lastIndex = 0; + for (let m = STAMP_RE.exec(body); m !== null; m = STAMP_RE.exec(body)) { + match = m; + } + if (match === null) { + return null; + } + const [, version, depth, verdict, anchorDraft, hunksField] = match; + if (Number(version) !== STAMP_SCHEMA_VERSION) { + return null; + } + if (!(RE_REVIEW_MODES as readonly string[]).includes(depth)) { + return null; + } + let anchorHunks: HunkSignature | "overflow"; + if (hunksField === "overflow") { + anchorHunks = "overflow"; + } else { + const decoded = decodeSignature(hunksField); + if (decoded === null) { + return null; + } + anchorHunks = decoded; + } + return { + schemaVersion: STAMP_SCHEMA_VERSION, + depth: depth as ReReviewDepth, + verdict, + anchorDraft: anchorDraft === "true", + anchorHunks, + }; +}; + +/** A prior review of the PR, as the orchestrator stages it. */ +export type PriorReview = { + body: string; + /** ISO timestamp when known; entries without one keep their input order. */ + submittedAt?: string; +}; + +/** + * Find the most recent stamped review. The stamp (not the review `state`) + * is the marker that a review carrying a fingerprint happened, so a + * DISMISSED or COMMENTED review counts the same as an APPROVED one. + */ +export const findLatestStamp = ( + reviews: readonly PriorReview[], +): ReReviewStamp | null => { + const ordered = [...reviews].sort((a, b) => { + if (a.submittedAt === undefined || b.submittedAt === undefined) { + return 0; + } + return a.submittedAt < b.submittedAt ? -1 : 1; + }); + for (let i = ordered.length - 1; i >= 0; i--) { + const stamp = parseRereviewStamp(ordered[i].body); + if (stamp !== null) { + return stamp; + } + } + return null; +}; + +/* -------------------------------------------------------------------------- */ +/* The depth decision */ +/* -------------------------------------------------------------------------- */ + +export type ReReviewDecisionInput = { + /** The repo's configured mode (`routing.json` `reReviewMode`). */ + mode: ReReviewMode; + /** Whether the PR is currently a draft. */ + isDraft: boolean; + /** The most recent stamped fingerprint, or null when none is readable. */ + priorStamp: ReReviewStamp | null; + /** The hunk signature of the diff under review now. */ + currentSignature: HunkSignature; + /** Tripwire re-arm threshold; {@link DEFAULT_TRIPWIRE_THRESHOLD}. */ + tripwireThreshold?: number; +}; + +/** Which sub-agents a depth dispatches (the prompt maps this to the roster). */ +export type ReReviewDispatch = + | "all" + | "reconcile+correctness" + | "reconcile-only"; + +/** Which diff the finding-producing reviewers are staged. */ +export type ReReviewStaging = "whole-diff" | "new-hunks" | "none"; + +export type ReReviewPlan = { + mode: ReReviewMode; + depth: ReReviewDepth; + dispatch: ReReviewDispatch; + staging: ReReviewStaging; + /** + * When true, a REQUEST_CHANGES→APPROVE flip requires zero validated + * blocking findings from the dispatched correctness pass (`flip-gated`). + */ + flipGate: boolean; + /** Fixed-format decision codes, most significant first (never prose). */ + reasons: string[]; + /** Divergence vs. the anchor; null when no anchor fingerprint exists. */ + divergence: Divergence | null; + /** True when the divergence tripwire re-armed a full review. */ + tripwireRearmed: boolean; + /** + * The fingerprint this run's stamp must carry: the current signature + * after a full-depth (`full`/`scoped`) run, the anchor carried forward + * verbatim otherwise. + */ + stampHunks: HunkSignature | "overflow"; + /** Draft flag for the stamp (see {@link ReReviewStamp.anchorDraft}). */ + stampAnchorDraft: boolean; +}; + +const DEPTH_SHAPE: Record< + ReReviewDepth, + {dispatch: ReReviewDispatch; staging: ReReviewStaging; flipGate: boolean} +> = { + full: {dispatch: "all", staging: "whole-diff", flipGate: false}, + scoped: {dispatch: "all", staging: "new-hunks", flipGate: false}, + "flip-gated": { + dispatch: "reconcile+correctness", + staging: "new-hunks", + flipGate: true, + }, + fast: {dispatch: "reconcile-only", staging: "none", flipGate: false}, +}; + +const fullPlan = ( + input: ReReviewDecisionInput, + reasons: string[], + divergence: Divergence | null, + tripwireRearmed: boolean, +): ReReviewPlan => ({ + mode: input.mode, + depth: "full", + ...DEPTH_SHAPE.full, + reasons, + divergence, + tripwireRearmed, + stampHunks: input.currentSignature, + stampAnchorDraft: input.isDraft, +}); + +/** + * Decide how deep this run reviews. Pure. Every guard resolves toward + * `full`; a cheaper depth requires a configured mode AND a usable, ready + * anchor fingerprint AND divergence under the tripwire threshold. + */ +export const decideReReviewDepth = ( + input: ReReviewDecisionInput, +): ReReviewPlan => { + const threshold = input.tripwireThreshold ?? DEFAULT_TRIPWIRE_THRESHOLD; + + if (input.mode === "full") { + return fullPlan(input, ["mode-full"], null, false); + } + if (input.priorStamp === null) { + return fullPlan(input, ["no-prior-fingerprint"], null, false); + } + // Ready-for-review anchor: a fingerprint taken on a draft skeleton must + // not anchor cheap re-reviews of the ready PR; the ready PR gets the + // one full review the cheaper modes lean on. + if (!input.isDraft && input.priorStamp.anchorDraft) { + return fullPlan(input, ["ready-for-review-anchor"], null, false); + } + if (input.priorStamp.anchorHunks === "overflow") { + return fullPlan(input, ["fingerprint-overflow"], null, false); + } + + const divergence = computeDivergence( + input.currentSignature, + input.priorStamp.anchorHunks, + ); + if (divergence.unreviewedShare >= threshold) { + return fullPlan(input, ["tripwire-divergence"], divergence, true); + } + + const depth = input.mode; + return { + mode: input.mode, + depth, + ...DEPTH_SHAPE[depth], + reasons: [`mode-${depth}`], + divergence, + tripwireRearmed: false, + stampHunks: + depth === "scoped" + ? input.currentSignature + : input.priorStamp.anchorHunks, + stampAnchorDraft: + depth === "scoped" ? input.isDraft : input.priorStamp.anchorDraft, + }; +}; + +/* -------------------------------------------------------------------------- */ +/* Scoped-diff staging */ +/* -------------------------------------------------------------------------- */ + +/** + * Rebuild the diff keeping only the hunks whose hash the fingerprint does + * not contain; the diff a `scoped`/`flip-gated` run stages to its + * finding-producing reviewers. Each kept file keeps its section header lines + * and its in-scope hunks verbatim (original hunk headers included); files + * with no in-scope hunk are dropped entirely. + */ +export const buildScopedDiff = ( + diffText: string, + reviewed: HunkSignature, +): string => { + const kept: string[] = []; + for (const section of splitUnifiedDiff(diffText)) { + const seen = new Set(reviewed[section.path] ?? []); + const hunks = splitHunks(section.text); + const inScope = hunks.filter((hunk) => !seen.has(hashHunk(hunk))); + if (inScope.length === 0) { + continue; + } + const firstHunkAt = section.text.search(/^@@ /m); + const header = + firstHunkAt === -1 + ? section.text + : section.text.slice(0, firstHunkAt).replace(/\n$/, ""); + kept.push([header, ...inScope].join("\n")); + } + return kept.join("\n"); +}; + +/* -------------------------------------------------------------------------- */ +/* CLI entrypoint (review.md invokes this after the router) */ +/* -------------------------------------------------------------------------- */ + +/** + * On-disk contract, extending the run's staging convention. + * + * `plan` (the default subcommand) reads the staged diff, preferring + * `full-stripped.diff` (the provenance CLI's generated-stripped diff) over + * `full.diff`, so generated churn (a lockfile push) neither enters the + * fingerprint nor counts as divergence. It also reads `routing.json` (for + * `reReviewMode`), `pr-context.json` (for `isDraft`), and + * `prior-reviews.json` (the bot's prior reviews of this PR, each + * `{body, submittedAt?}`, every state included, DISMISSED and COMMENTED + * too), and writes `rereview-plan.json` (the {@link ReReviewPlan}). When the + * plan stages `new-hunks` it also writes `scoped.diff` (generated-stripped + * whenever the stripped diff was the input). A missing or unreadable input + * degrades the plan to `full` with a fixed-format reason, never to a crash + * and never to a cheaper depth. + * + * `stamp --verdict ` runs after the verdict is decided: it reads + * `rereview-plan.json` back and prints the hidden-comment stamp line the + * orchestrator appends to the review body it submits. + */ +const REVIEW_DIR = "/tmp/gh-aw/review"; +const FULL_DIFF_PATH = `${REVIEW_DIR}/full.diff`; +const STRIPPED_DIFF_PATH = `${REVIEW_DIR}/full-stripped.diff`; +const ROUTING_PATH = `${REVIEW_DIR}/routing.json`; +const PR_CONTEXT_PATH = `${REVIEW_DIR}/pr-context.json`; +const PRIOR_REVIEWS_PATH = `${REVIEW_DIR}/prior-reviews.json`; +const PLAN_OUT = `${REVIEW_DIR}/rereview-plan.json`; +const SCOPED_DIFF_OUT = `${REVIEW_DIR}/scoped.diff`; + +type RereviewCliFs = { + readFileSync: (p: string, enc: "utf8") => string; + writeFileSync: (p: string, data: string) => void; + existsSync: (p: string) => boolean; + mkdirSync: (p: string, opts: {recursive: boolean}) => void; +}; + +const readJsonIfPresent = (fs: RereviewCliFs, path: string): unknown => { + if (!fs.existsSync(path)) { + return undefined; + } + try { + return JSON.parse(fs.readFileSync(path, "utf8")); + } catch { + return undefined; + } +}; + +export type RereviewPlanCliResult = { + plan: ReReviewPlan; + /** Fixed-format staging problems (each also forced the plan to full). */ + warnings: string[]; +}; + +/** + * Stage the re-review plan. Factored out (fs injected) so it is testable + * without touching the real filesystem. Returns what was written. + */ +export const runRereviewPlanCli = ( + fs: RereviewCliFs, +): RereviewPlanCliResult => { + const warnings: string[] = []; + + const routing = readJsonIfPresent(fs, ROUTING_PATH) as + | {reReviewMode?: unknown} + | undefined; + let mode = DEFAULT_RE_REVIEW_MODE; + if ( + routing !== undefined && + typeof routing.reReviewMode === "string" && + (RE_REVIEW_MODES as readonly string[]).includes(routing.reReviewMode) + ) { + mode = routing.reReviewMode as ReReviewMode; + } else if (routing === undefined) { + warnings.push(`routing not staged (${ROUTING_PATH}): mode is full`); + } + + const prContext = readJsonIfPresent(fs, PR_CONTEXT_PATH) as + | {isDraft?: unknown} + | undefined; + let isDraft = false; + if (prContext !== undefined && typeof prContext.isDraft === "boolean") { + isDraft = prContext.isDraft; + } else { + warnings.push( + `pr context not staged (${PR_CONTEXT_PATH}): mode is full`, + ); + mode = "full"; + } + + // Prefer the generated-stripped diff: reviewers never review generated + // content, so it belongs in neither the fingerprint nor the divergence + // share (a lockfile push must not trip the wire). + const diffPath = fs.existsSync(STRIPPED_DIFF_PATH) + ? STRIPPED_DIFF_PATH + : FULL_DIFF_PATH; + let diffText: string | null = null; + let currentSignature: HunkSignature = {}; + if (fs.existsSync(diffPath)) { + diffText = fs.readFileSync(diffPath, "utf8"); + currentSignature = computeHunkSignature(diffText); + } else { + warnings.push(`no diff staged (${diffPath}): mode is full`); + mode = "full"; + } + + const rawReviews = readJsonIfPresent(fs, PRIOR_REVIEWS_PATH); + const priorReviews: PriorReview[] = Array.isArray(rawReviews) + ? rawReviews + .filter( + (entry): entry is {body: string; submittedAt?: string} => + typeof (entry as {body?: unknown}).body === "string", + ) + .map((entry) => ({ + body: entry.body, + ...(typeof entry.submittedAt === "string" + ? {submittedAt: entry.submittedAt} + : {}), + })) + : []; + + const priorStamp = findLatestStamp(priorReviews); + const plan = decideReReviewDepth({ + mode, + isDraft, + priorStamp, + currentSignature, + }); + + fs.mkdirSync(REVIEW_DIR, {recursive: true}); + fs.writeFileSync(PLAN_OUT, JSON.stringify(plan, null, 2)); + // A `new-hunks` plan implies a usable anchor (every guard that loses the + // anchor resolves to full, whose staging is the whole diff). + if ( + plan.staging === "new-hunks" && + priorStamp !== null && + priorStamp.anchorHunks !== "overflow" && + diffText !== null + ) { + fs.writeFileSync( + SCOPED_DIFF_OUT, + buildScopedDiff(diffText, priorStamp.anchorHunks), + ); + } + + return {plan, warnings}; +}; + +/** + * Render this run's stamp from the staged plan and the decided verdict. + * Returns null when the plan is not staged (the caller then omits the stamp; + * the next run degrades to full, never crashes). + */ +export const runRereviewStampCli = ( + fs: RereviewCliFs, + verdict: string, +): string | null => { + const plan = readJsonIfPresent(fs, PLAN_OUT) as ReReviewPlan | undefined; + if (plan === undefined) { + return null; + } + return renderRereviewStamp({ + schemaVersion: STAMP_SCHEMA_VERSION, + depth: plan.depth, + verdict, + anchorDraft: plan.stampAnchorDraft, + anchorHunks: plan.stampHunks, + }); +}; + +// Run only when executed directly (review.md), never on import (tests). +if (typeof require !== "undefined" && require.main === module) { + const fs = require("node:fs") as RereviewCliFs; + const [subcommand = "plan", ...rest] = process.argv.slice(2); + if (subcommand === "stamp") { + const flagIndex = rest.indexOf("--verdict"); + 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 "); + process.exit(2); + } + const stamp = runRereviewStampCli(fs, verdict); + if (stamp === null) { + // eslint-disable-next-line no-console + console.error(`plan not staged (${PLAN_OUT})`); + process.exit(1); + } + // eslint-disable-next-line no-console + console.log(stamp); + } else { + const result = runRereviewPlanCli(fs); + // eslint-disable-next-line no-console + console.log( + JSON.stringify({ + depth: result.plan.depth, + dispatch: result.plan.dispatch, + staging: result.plan.staging, + reasons: result.plan.reasons, + tripwireRearmed: result.plan.tripwireRearmed, + unreviewedShare: + result.plan.divergence?.unreviewedShare ?? null, + warnings: result.warnings, + }), + ); + } +} diff --git a/workflows/review/lib/rereview.test.ts b/workflows/review/lib/rereview.test.ts index e5ab9f77..fc5d00f2 100644 --- a/workflows/review/lib/rereview.test.ts +++ b/workflows/review/lib/rereview.test.ts @@ -286,7 +286,12 @@ describe("runRereviewCli", () => { [THREADS]: JSON.stringify([]), }); const result = runRereviewCli(fs); - expect(result).toEqual({section: "", keptCount: 0, resolvedCount: 0}); + expect(result).toEqual({ + section: "", + keptCount: 0, + resolvedCount: 0, + keptBlockingCount: 0, + }); expect(JSON.parse(written[RESULT])).toEqual(result); }); @@ -311,3 +316,41 @@ describe("runRereviewCli", () => { expect(result.section).toContain("still unaddressed"); }); }); + +describe("keptBlockingCount (the mode dial's flip-gate input)", () => { + const thread = (id: string, label: string, line: number): StagedThread => ({ + thread_id: id, + path: "src/handler.ts", + line, + comments: [{author: "github-actions[bot]", body: `**${label}:** x`}], + }); + + it("counts only kept threads whose opening label blocks", () => { + const result = renderRereviewSection({ + threads: [ + thread("t1", "issue (blocking)", 10), + thread("t2", "suggestion (non-blocking)", 20), + thread("t3", "todo (blocking)", 30), + ], + reconciler: {resolve: ["t2"], keep: ["t1", "t3"]}, + }); + expect(result.keptCount).toBe(2); + expect(result.keptBlockingCount).toBe(2); + }); + + it("is zero when every kept thread is non-blocking (flip may proceed)", () => { + const result = renderRereviewSection({ + threads: [thread("t1", "nitpick (non-blocking)", 5)], + reconciler: {resolve: [], keep: ["t1"]}, + }); + expect(result.keptBlockingCount).toBe(0); + }); + + it("is zero when everything is resolved", () => { + const result = renderRereviewSection({ + threads: [thread("t1", "issue (blocking)", 5)], + reconciler: {resolve: ["t1"], keep: []}, + }); + expect(result.keptBlockingCount).toBe(0); + }); +}); diff --git a/workflows/review/lib/rereview.ts b/workflows/review/lib/rereview.ts index 7ac17d2b..6c8af050 100644 --- a/workflows/review/lib/rereview.ts +++ b/workflows/review/lib/rereview.ts @@ -54,6 +54,14 @@ export type RereviewSection = { section: string; keptCount: number; resolvedCount: number; + /** + * How many kept threads carry a blocking opening label. The re-review + * mode dial's flip gate reads this: a reduced-depth run may flip a prior + * REQUEST_CHANGES to APPROVE only when it is zero (and, in `flip-gated` + * mode, no validated blocking finding survived), so the check is a number + * comparison, not a label judgment re-made at verdict time. + */ + keptBlockingCount: number; }; /** @@ -173,7 +181,7 @@ export const renderRereviewSection = ( const total = resolvedCount + keptCount; if (total === 0) { - return {section: "", keptCount, resolvedCount}; + return {section: "", keptCount, resolvedCount, keptBlockingCount: 0}; } if (keptCount === 0) { @@ -181,7 +189,7 @@ export const renderRereviewSection = ( resolvedCount === 1 ? "The 1 prior review thread is resolved." : `All ${resolvedCount} prior review threads are resolved.`; - return {section, keptCount, resolvedCount}; + return {section, keptCount, resolvedCount, keptBlockingCount: 0}; } const asOf = @@ -204,6 +212,7 @@ export const renderRereviewSection = ( section: [header, ...entries.map(renderKeptLine)].join("\n"), keptCount, resolvedCount, + keptBlockingCount: entries.filter((entry) => entry.blocking).length, }; }; @@ -296,7 +305,12 @@ export const runRereviewCli = (fs: RereviewCliFs): RereviewSection => { let result: RereviewSection; if (reconciler === undefined) { - result = {section: "", keptCount: 0, resolvedCount: 0}; + result = { + section: "", + keptCount: 0, + resolvedCount: 0, + keptBlockingCount: 0, + }; } else { const prContext = readJson(fs, PR_CONTEXT_PATH); const headSha = diff --git a/workflows/review/lib/router.test.ts b/workflows/review/lib/router.test.ts index cb316637..0070aef8 100644 --- a/workflows/review/lib/router.test.ts +++ b/workflows/review/lib/router.test.ts @@ -812,3 +812,60 @@ describe("runCli: enabled reviewers", () => { expect(runCli(fs).enabledReviewers).toEqual([]); }); }); + +describe("parseRoutingConfig: re-review directive", () => { + it("defaults to full", () => { + expect(parseRoutingConfig("docs/** tier=trivial").reReviewMode).toBe( + "full", + ); + }); + + it("parses each known mode", () => { + for (const mode of ["full", "scoped", "flip-gated", "fast"] as const) { + const config = parseRoutingConfig(`re-review ${mode}`); + expect(config.reReviewMode).toBe(mode); + expect(config.warnings).toEqual([]); + } + }); + + it("degrades an unknown mode to full with a warning; toward more review", () => { + const config = parseRoutingConfig("re-review turbo"); + expect(config.reReviewMode).toBe("full"); + expect(config.warnings.join("\n")).toContain( + 'unknown re-review mode "turbo"', + ); + }); + + it("skips a re-review line with the wrong arity", () => { + const config = parseRoutingConfig("re-review scoped fast"); + expect(config.reReviewMode).toBe("full"); + expect(config.warnings.join("\n")).toContain("exactly one mode"); + }); + + it("lets the last of duplicate lines win, with a warning", () => { + const config = parseRoutingConfig("re-review scoped\nre-review fast"); + expect(config.reReviewMode).toBe("fast"); + expect(config.warnings.join("\n")).toContain("duplicate re-review"); + }); +}); + +describe("runCli: re-review mode", () => { + it("surfaces the configured mode in routing.json", () => { + const {fs} = fakeFs({ + ["/tmp/gh-aw/review/files.json"]: JSON.stringify([ + {path: "a.ts", status: "modified"}, + ]), + [ROUTING_CONFIG_PATH]: "re-review scoped", + }); + expect(runCli(fs).reReviewMode).toBe("scoped"); + }); + + it("defaults to full without a ROUTING config", () => { + const {fs} = fakeFs({ + ["/tmp/gh-aw/review/files.json"]: JSON.stringify([ + {path: "a.ts", status: "modified"}, + ]), + }); + expect(runCli(fs).reReviewMode).toBe("full"); + }); +}); diff --git a/workflows/review/lib/router.ts b/workflows/review/lib/router.ts index 28996187..61a1281f 100644 --- a/workflows/review/lib/router.ts +++ b/workflows/review/lib/router.ts @@ -33,14 +33,17 @@ import {KNOWN_LENSES} from "./finding-schema"; import type {Lens} from "./finding-schema"; import { + DEFAULT_RE_REVIEW_MODE, ENABLEABLE_REVIEWERS, parseRoutingConfig, + RE_REVIEW_MODES, RISK_TIERS, ROUTING_CONFIG_PATH, } from "./routing-config"; import type { EnableableReviewer, LensRule, + ReReviewMode, RiskRule, RiskTier, RoutingFileConfig, @@ -49,14 +52,17 @@ import type { // Re-exported so consumers (and the tests) can treat the router as the single // entry point for routing vocabulary and the ROUTING parser. export { + DEFAULT_RE_REVIEW_MODE, ENABLEABLE_REVIEWERS, parseRoutingConfig, + RE_REVIEW_MODES, RISK_TIERS, ROUTING_CONFIG_PATH, }; export type { EnableableReviewer, LensRule, + ReReviewMode, RiskRule, RiskTier, RoutingFileConfig, @@ -790,6 +796,12 @@ export type RoutingJson = { * (`enable ` lines). Empty means the default roster only. */ enabledReviewers: EnableableReviewer[]; + /** + * The repo's re-review mode (`re-review` line in `ROUTING`; `full` when + * absent or when the ROUTING file is missing). The re-review CLI + * (`rereview.ts`) reads this to decide how deep a repeat review runs. + */ + reReviewMode: ReReviewMode; /** * Whether the consumer `ROUTING` file was found, plus any parse warnings * (or the missing-file warning). The orchestrator surfaces these in the @@ -811,6 +823,7 @@ export const toRoutingJson = ( result: RoutingResult, routingConfig: RoutingJson["routingConfig"] = {present: true, warnings: []}, enabledReviewers: EnableableReviewer[] = [], + reReviewMode: ReReviewMode = DEFAULT_RE_REVIEW_MODE, ): RoutingJson => { const owners: Record = {}; const generatedFiles: string[] = []; @@ -835,6 +848,7 @@ export const toRoutingJson = ( runBudget: result.runBudget, pendingRiskQuestions: result.pendingRiskQuestions, enabledReviewers, + reReviewMode, routingConfig, }; }; @@ -914,6 +928,7 @@ export const runCli = (fs: FsLike, repoRoot = "."): RoutingJson => { lensRules: [], riskRules: [], enabledReviewers: [], + reReviewMode: DEFAULT_RE_REVIEW_MODE, warnings: [ `routing config missing (${ROUTING_CONFIG_PATH}): no ` + `specialist lenses will run; always-on reviewers only ` + @@ -946,6 +961,7 @@ export const runCli = (fs: FsLike, repoRoot = "."): RoutingJson => { warnings: routingFileConfig.warnings, }, routingFileConfig.enabledReviewers, + routingFileConfig.reReviewMode, ); fs.mkdirSync(REVIEW_DIR, {recursive: true}); diff --git a/workflows/review/lib/routing-config.ts b/workflows/review/lib/routing-config.ts index 11800eb3..292302b9 100644 --- a/workflows/review/lib/routing-config.ts +++ b/workflows/review/lib/routing-config.ts @@ -61,12 +61,47 @@ export const ENABLEABLE_REVIEWERS = [ export type EnableableReviewer = typeof ENABLEABLE_REVIEWERS[number]; +/** + * The re-review mode dial: how much of the roster a *repeat* review of the + * same PR runs. The first full review of a ready-for-review PR always runs + * the whole roster whatever the mode; the dial governs the pushes after it. + * Ordered cheapest-last: + * + * - `full`: every push re-runs the whole roster (today's behavior). + * - `scoped`: every push re-runs the whole roster, but reviewers are + * staged only the hunks that are new since the last + * fully-reviewed fingerprint, and comments stay scoped to + * those hunks. Catches fresh defects in new code at a + * fraction of the input cost. + * - `flip-gated`: reconcile-only fast path, plus the correctness pass over + * the new hunks; a REQUEST_CHANGES→APPROVE flip is vetoed + * by any validated blocking finding from that pass. + * - `fast`: reconcile-only: threads are verified and resolved, + * nothing new is reviewed (the divergence tripwire is the + * only fresh-code guard). + * + * `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 = [ + "full", + "scoped", + "flip-gated", + "fast", +] as const; + +export type ReReviewMode = typeof RE_REVIEW_MODES[number]; + +export const DEFAULT_RE_REVIEW_MODE: ReReviewMode = "full"; + /** Parsed `.github/aw/review/ROUTING` config. */ export type RoutingFileConfig = { lensRules: LensRule[]; riskRules: RiskRule[]; /** Opt-in whole-change reviewers this repo enables (canonical order). */ enabledReviewers: EnableableReviewer[]; + /** The repo's re-review mode (`re-review` line; default `full`). */ + reReviewMode: ReReviewMode; /** Fixed-format parse warnings (unknown lens/tier, no-op rule). */ warnings: string[]; }; @@ -79,6 +114,7 @@ const KNOWN_LENS_SET: ReadonlySet = new Set(KNOWN_LENSES); * * [lens=[,…]] [tier=trivial|low|medium|high] [direction-dependent] * enable [,…] + * re-review full|scoped|flip-gated|fast * * `lens=` names specialist lenses to spawn when the pattern is touched (multiple * matching rules union their lenses). `tier=` assigns a risk tier; when several @@ -90,15 +126,21 @@ const KNOWN_LENS_SET: ReadonlySet = new Set(KNOWN_LENSES); * {@link RiskRule.diffDirectionDependent}) and requires `tier=`. * `enable` turns on an opt-in whole-change reviewer * ({@link ENABLEABLE_REVIEWERS}) for every review in this repo. + * `re-review` sets the repo's re-review mode ({@link RE_REVIEW_MODES}); when + * several lines set it the LAST one wins (with a warning), matching the + * file's last-rule-wins convention. * * Malformed fields and unknown lens/reviewer names produce a warning and skip * the lens or line rather than aborting the run: routing degrades to fewer - * reviewers, never to a crashed review. + * reviewers, never to a crashed review. An unknown `re-review` mode degrades + * to `full`: toward more review, never less. */ export const parseRoutingConfig = (content: string): RoutingFileConfig => { const lensRules: LensRule[] = []; const riskRules: RiskRule[] = []; const enabled = new Set(); + let reReviewMode: ReReviewMode = DEFAULT_RE_REVIEW_MODE; + let reReviewLineSeen = false; const warnings: string[] = []; const lines = content.split(/\r?\n/); @@ -135,6 +177,33 @@ export const parseRoutingConfig = (content: string): RoutingFileConfig => { continue; } + if (pattern === "re-review") { + if (fields.length !== 1) { + warnings.push( + `ROUTING line ${lineNo}: re-review takes exactly one ` + + `mode (line skipped)`, + ); + continue; + } + const mode = fields[0]; + if (!(RE_REVIEW_MODES as readonly string[]).includes(mode)) { + warnings.push( + `ROUTING line ${lineNo}: unknown re-review mode ` + + `"${mode}" (kept ${reReviewMode})`, + ); + continue; + } + if (reReviewLineSeen) { + warnings.push( + `ROUTING line ${lineNo}: duplicate re-review line ` + + `(last one wins)`, + ); + } + reReviewMode = mode as ReReviewMode; + reReviewLineSeen = true; + continue; + } + const lenses = new Set(); let tier: RiskTier | undefined; let directionDependent = false; @@ -208,6 +277,7 @@ export const parseRoutingConfig = (content: string): RoutingFileConfig => { enabledReviewers: ENABLEABLE_REVIEWERS.filter((reviewer) => enabled.has(reviewer), ), + reReviewMode, warnings, }; }; diff --git a/workflows/review/review.md b/workflows/review/review.md index 394b8045..097d7687 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -359,6 +359,15 @@ Write the result to `/tmp/gh-aw/review/new-scope.json` as RIGHT-side line numbers of the added lines inside in-scope hunks. Step 3 uses this to filter candidate comments. +**Stage the bot's prior reviews.** Fetch the PR's reviews (`pull_requests` +`get_pull_request_reviews`) and write `/tmp/gh-aw/review/prior-reviews.json`: every +review authored by `github-actions[bot]`, **whatever its state** (APPROVED, +CHANGES_REQUESTED, COMMENTED, DISMISSED), each `{"body": "...", +"submittedAt": ""}`. The re-review plan CLI (Step 3) reads the hidden +fingerprint stamp from these bodies; a review that branch protection dismissed, or +that was submitted comment-only, still carries its stamp, which is exactly why the +state is ignored here. Do not filter or truncate the bodies. + ## Step 2: Early-Exit Check This workflow runs on every push. Decide here — using the context gathered in Step 1 — @@ -526,6 +535,53 @@ two files: so a lock-file-heavy PR cannot balloon their context; `pattern-triage` still reads `full.diff` because classifying every changed file is its job. +**Decide the re-review depth (deterministic code).** After the provenance CLI, run +the re-review mode CLI from the shared lib checkout, once: +``` +cd gh-aw-review-lib && npx -y tsx workflows/review/lib/rereview-mode.ts +``` +It reads `routing.json` (the repo's `re-review` mode line, default `full`), +`pr-context.json`, the staged diff (preferring `full-stripped.diff`), and +`prior-reviews.json` (Step 1), and writes `/tmp/gh-aw/review/rereview-plan.json`: +`{"depth": "full|scoped|flip-gated|fast", "dispatch", "staging", "flipGate", +"reasons", "divergence", "tripwireRearmed", …}`, plus `/tmp/gh-aw/review/scoped.diff` +(the hunks no fully-reviewed fingerprint has seen) when `staging` is `new-hunks`. +Copy `rereview-plan.json` to `/tmp/gh-aw/review/out/rereview-plan.json` now, so the +run artifact records the executed depth (the cost counters price the mode dial from +it). The plan is deterministic and final: never deepen or shallow it yourself, and +never re-run the CLI later in the review. Its three guards are code, not your +judgment: the one anchoring full review is taken at ready-for-review, a fingerprint +overflow or a missing input forces `full`, and the divergence tripwire re-arms +`full` when too much of the diff is unreviewed. What each depth means for the phases +below: + +- **`depth: full`**: proceed exactly as written below; nothing changes. +- **`depth: scoped`**: the full roster runs, but over only the unseen hunks. Before + Phase 1, overwrite `/tmp/gh-aw/review/full-stripped.diff` with the contents of + `scoped.diff`, and in Phase 1 build `pr.diff` from the `scoped.diff` sections of + the triage `reviewFiles` (a `reviewFiles` entry absent from `scoped.diff` is + already reviewed; leave it out of `pr.diff`). Everything else, the provenance + gate, the scope filter, threads, and validation, runs as written. +- **`depth: flip-gated`**: skip `pattern-triage` and dispatch in Phase 2 only + `thread-reconciler` and `correctness-reviewer` (no enabled reviewers, no lenses). + Stage `pr.diff` as a copy of `scoped.diff` and `review-files.json` as the files + appearing in it. The correctness candidates still flow through the provenance + gate, the scope filter, and Phase 3 validation exactly as written; the flip rule + in Step 4 is what makes their validated blocking findings veto an approval flip. +- **`depth: fast`**: skip `pattern-triage` and dispatch in Phase 2 only + `thread-reconciler`. There are no finding-producing reviewers, so Phase 3 is + skipped; Steps 4 to 6 run on the reconciler's result and the flip rule (Step 4). + +On a reduced depth (`scoped`, `flip-gated`, `fast`), Step 7 posts no new +risks/patterns comment and Step 9 carries `risksPatternsKey` forward unchanged (the +reduced run computed no triage or risk data to compare), and Step 8 requests no new +reviewers when `correctness-reviewer` did not run. Also queue one note line for the +review body (Step 6), exactly: +`Note: re-review ran at depth (re-review mode ).` +When the plan's `tripwireRearmed` is true, queue instead, exactly: +`Note: divergence tripwire re-armed a full review (unreviewed share ).` + **Phase 1 — triage (first, alone).** Dispatch **`pattern-triage`**. It returns `patterns[]` (common cross-file change patterns; on approval they go in the risk/patterns comment, Step 7) and `reviewFiles` (the files that need a real review — @@ -837,6 +893,22 @@ label (see below), but the verdict follows the labels on the actual posted comme 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 +when `rereview-plan.json` (Step 3) says `depth` is `flip-gated` or `fast` and the +latest fingerprint stamp's `verdict` was `REQUEST_CHANGES`: read the stamp, not the +review state, since branch protection may have dismissed that review. A reduced-depth +run reviews little or nothing new, so its APPROVE would mean "the prior objections +are resolved"; it may flip to APPROVE only when the code-rendered accountability +result (`/tmp/gh-aw/review/rereview.json`, Step 6) has `keptBlockingCount: 0`, that +is, the reconciler resolved every blocking thread. If `keptBlockingCount` is greater +than zero, the verdict is REQUEST_CHANGES even though this run posted no new blocking +comment; the accountability section lists the surviving threads, so the author sees +exactly what still blocks. In `flip-gated` depth the dispatched correctness pass adds +the second half of the gate mechanically: any validated blocking finding it produced +posts and blocks under the rule above, so a fresh defect vetoes the flip instead of +being discarded. This rule never applies to `full` or `scoped` depth, where the whole +roster re-reviews and the plain rule above stands alone. + ### What should carry a blocking label **Blocking requires a concrete failing scenario.** A finding may carry a blocking @@ -1026,10 +1098,14 @@ section rather than dropping them. Within the cap the ranking order is: Before submitting, check whether this review would be a no-op repeat of the PR's current state: the verdict (Step 4) is APPROVE, you left **no** inline comments in -Step 5, there are **no** skipped-dimension notes to add (below), and the code-rendered +Step 5, there are **no** skipped-dimension notes to add (below), **no** re-review +depth or tripwire note was queued (Step 3), and the code-rendered re-review accountability section (below) is empty — i.e. the review body would be exactly the plain `Approved — no blocking issues found.` text with nothing -else. Only when all of those hold, fetch the PR's existing reviews +else. (The hidden fingerprint stamp below is invisible and does not count as text for +this check; when the skip applies, no review is submitted, so the stamp is simply not +refreshed and the prior one stays authoritative, which can only make the next run more +thorough.) Only when all of those hold, fetch the PR's existing reviews (`pull_requests` `get_pull_request_reviews`) and find the most recent one authored by `github-actions[bot]`. If its `state` is `APPROVED`, the PR is already sitting at an approved, no-comment state and posting an identical approval again adds nothing — @@ -1082,7 +1158,9 @@ cd gh-aw-review-lib && npx -y tsx workflows/review/lib/rereview.ts ``` It reads `threads.json`, the reconciler's `out/thread-reconciler.json`, and `pr-context.json`, and writes `/tmp/gh-aw/review/rereview.json`: -`{"section": "", "keptCount": , "resolvedCount": }`. Append +`{"section": "", "keptCount": , "resolvedCount": , +"keptBlockingCount": }` (`keptBlockingCount` also feeds the re-review flip rule, +Step 4). Append `section` **verbatim** to the review body, after any verdict-specific text above — it enumerates each still-unaddressed prior thread as a link to its prior comment (blocking first) and states the resolved count, and on a run that resolved the last @@ -1099,18 +1177,37 @@ per skipped dimension, exactly: change-provenance gate was skipped because `provenance.json` was missing or carried warnings (Step 3), also append exactly: `Note: change-provenance gate skipped this run (diff staging unparseable).` -These note lines and the code-rendered re-review accountability section are the +Also append here the re-review depth or tripwire note queued in Step 3, when there +is one. These note lines, the code-rendered re-review accountability section, and +the hidden fingerprint stamp below are the only text permitted beyond the verdict bodies above, and they apply to both APPROVE and REQUEST_CHANGES, including the empty-body cases: when the body is otherwise empty, they are the entire body. +**The re-review fingerprint stamp (every submitted review; code-rendered).** Last, +render this run's stamp with the verdict event you are about to submit: +``` +cd gh-aw-review-lib && npx -y tsx workflows/review/lib/rereview-mode.ts stamp \ + --verdict +``` +Append its single output line **verbatim** as the final line of the review body. It +is a hidden HTML comment and renders as nothing; it is how the next run finds the +last fully-reviewed fingerprint and the prior verdict, surviving cache eviction, +branch protection's dismiss-stale-approvals, and comment-only submissions. Every +submitted review carries it, whatever the depth and verdict, on first reviews and +re-reviews alike. If the CLI prints nothing (the plan was not staged), submit +without it; the next run then degrades to a full review, never to a cheaper one. + Do NOT put the risk summary or common patterns in the review body. On approval they go in a separate PR comment (Step 7). ## Step 7: On Approval — Post Risk and Patterns as a PR Comment **Only run this step when the verdict is APPROVE.** When requesting changes, skip -it entirely and post no comment. +it entirely and post no comment. Also skip it entirely on a reduced re-review +depth (`scoped`, `flip-gated`, `fast`; Step 3): the reduced run computed no triage +or risk data to compare, so the existing comment stands and `risksPatternsKey` +carries forward unchanged (Step 9). When this PR has moderate- or high-risk files **or** common patterns (both from Step 3), post a single standalone PR comment — separate from the review and @@ -1255,7 +1352,10 @@ fully explained by a common pattern above: ## Step 8: On Approval — Request the Owning Teams as Reviewers **Only run this step when the verdict is APPROVE.** Skip it entirely when -requesting changes. +requesting changes. Also skip it entirely when `correctness-reviewer` did not run +this run (a `flip-gated` or `fast` re-review depth, Step 3): there are no fresh +risk classifications to route on, and the anchoring full review already requested +the owning teams. **Only request reviewers when the PR is not a draft** — that is, when the PR's `draft` field (from the PR details you fetched in Step 1) is `false`. Drafts are @@ -1340,7 +1440,10 @@ Save to `/tmp/gh-aw/cache-memory/pr-${{ github.event.pull_request.number || gith lines only). Always record this, on every review, so the next run can scope its comments to hunks whose content is new since this review (Step 1 → Step 3). Record the full current signature, not just the hunks you commented on — "already reviewed" - means every hunk you looked at this run. + means every hunk you looked at this run. (This cache entry serves comment scoping + only; the divergence tripwire's authoritative fingerprint is the hidden stamp in + the review body, Step 6, which is exactly why the stamp exists: cache memory can + be evicted, the review body cannot.) - `wasDraft`: whether the PR was a draft at this review (its `draft` field). Record it on every review so Step 2 can compare it against the current draft status to detect the draft→ready transition and bypass the early-exit check