diff --git a/.changeset/stamp-carrier-cache-memory.md b/.changeset/stamp-carrier-cache-memory.md new file mode 100644 index 00000000..703450dc --- /dev/null +++ b/.changeset/stamp-carrier-cache-memory.md @@ -0,0 +1,7 @@ +--- +"review": patch +--- + +review: the re-review fingerprint anchors on cache memory; the body stamp never survives gh-aw ingest + +gh-aw's safe-output sanitizer strips all XML/HTML comments (`removeXmlComments`), so the hidden fingerprint stamp a review body carries never reaches the PR: every production re-review planned `no-prior-fingerprint` and silently escalated to full depth, making the `re-review` ROUTING dial (scoped/flip-gated/fast) inert. The plan CLI now falls back to the Step 9 cache-memory record (`verdict`, `stampHunks`/`reviewedHunks`, `wasDraft`) when no prior-review body carries a stamp, and records which carrier anchored the plan as `stampSource` in `rereview-plan.json`. Step 9 gains a `stampHunks` field copied verbatim from the plan CLI's own hash computation so hash regimes are never mixed. Cache eviction still degrades to a full review, never a cheaper one. diff --git a/workflows/review/lib/rereview-mode.test.ts b/workflows/review/lib/rereview-mode.test.ts index 2523d137..72608eb4 100644 --- a/workflows/review/lib/rereview-mode.test.ts +++ b/workflows/review/lib/rereview-mode.test.ts @@ -13,6 +13,7 @@ import { runRereviewPlanCli, runRereviewStampCli, STAMP_SCHEMA_VERSION, + stampFromCacheMemory, } from "./rereview-mode"; import type {HunkSignature, ReReviewStamp} from "./rereview-mode"; @@ -575,6 +576,158 @@ describe("runRereviewPlanCli", () => { }); }); +/* -------------------------------------------------------------------------- */ +/* The cache-memory fingerprint carrier */ +/* -------------------------------------------------------------------------- */ + +/** A Step 9 cache record whose fields reconstruct a usable stamp. */ +const cacheRecord = (over: Record = {}): string => + JSON.stringify({ + verdict: "APPROVE", + reviewedHunks: CURRENT, + wasDraft: false, + ...over, + }); + +describe("stampFromCacheMemory", () => { + it("reconstructs a stamp from a valid Step 9 record", () => { + const stamp = stampFromCacheMemory(JSON.parse(cacheRecord())); + expect(stamp).toEqual({ + schemaVersion: STAMP_SCHEMA_VERSION, + depth: "full", + verdict: "APPROVE", + anchorDraft: false, + anchorHunks: CURRENT, + }); + }); + + it.each([ + ["missing verdict", {verdict: undefined}], + ["unknown verdict", {verdict: "COMMENTED"}], + ["missing wasDraft", {wasDraft: undefined}], + ["non-boolean wasDraft", {wasDraft: "false"}], + ["missing hunks", {reviewedHunks: undefined}], + ["array hunks", {reviewedHunks: ["abc"]}], + ["non-string hash", {reviewedHunks: {"a.ts": [42]}}], + ["empty hash", {reviewedHunks: {"a.ts": [""]}}], + ["empty hunk map", {reviewedHunks: {}}], + ])("returns null on %s (fail toward full)", (_label, over) => { + expect(stampFromCacheMemory(JSON.parse(cacheRecord(over)))).toBeNull(); + }); + + it("returns null on a non-object record", () => { + expect(stampFromCacheMemory(null)).toBeNull(); + expect(stampFromCacheMemory("{}")).toBeNull(); + expect(stampFromCacheMemory([])).toBeNull(); + }); + + it("prefers stampHunks (the plan CLI's own hash regime) over reviewedHunks", () => { + const other: HunkSignature = {"other.ts": ["deadbeef"]}; + const stamp = stampFromCacheMemory( + JSON.parse(cacheRecord({stampHunks: other})), + ); + expect(stamp?.anchorHunks).toEqual(other); + }); + + it("falls back to reviewedHunks when stampHunks is invalid", () => { + const stamp = stampFromCacheMemory( + JSON.parse(cacheRecord({stampHunks: {"a.ts": [42]}})), + ); + expect(stamp?.anchorHunks).toEqual(CURRENT); + }); +}); + +describe("runRereviewPlanCli cache-memory fallback", () => { + const CACHE_PATH = "/tmp/gh-aw/cache-memory/pr-41007.json"; + const contextWithNumber = JSON.stringify({isDraft: false, number: 41007}); + + it("anchors on the cache record when no prior-review body carries a stamp (the production shape: the ingest sanitizer strips the body stamp)", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/pr-context.json`]: contextWithNumber, + // What production prior reviews actually look like: bodies + // present, stamps sanitized away. + [`${REVIEW_DIR}/prior-reviews.json`]: JSON.stringify([ + {body: "Changes requested — see inline comments."}, + ]), + [CACHE_PATH]: cacheRecord(), + }), + ); + const {plan, stampSource} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("fast"); + expect(plan.reasons).toEqual(["mode-fast"]); + expect(stampSource).toBe("cache-memory"); + const written = JSON.parse( + fs.files.get(`${REVIEW_DIR}/rereview-plan.json`) ?? "{}", + ); + expect(written.stampSource).toBe("cache-memory"); + }); + + it("prefers a review-body stamp over the cache record", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/pr-context.json`]: contextWithNumber, + [CACHE_PATH]: cacheRecord({verdict: "REQUEST_CHANGES"}), + }), + ); + const {stampSource} = runRereviewPlanCli(fs); + expect(stampSource).toBe("review-body"); + }); + + it("plans full with no stamp in either carrier", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/pr-context.json`]: contextWithNumber, + [`${REVIEW_DIR}/prior-reviews.json`]: JSON.stringify([ + {body: "no stamp here"}, + ]), + }), + ); + const {plan, stampSource} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("full"); + expect(plan.reasons).toEqual(["no-prior-fingerprint"]); + expect(stampSource).toBeNull(); + }); + + it("applies the ready-for-review guard to a cache anchor taken on a draft", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/pr-context.json`]: contextWithNumber, + [`${REVIEW_DIR}/prior-reviews.json`]: "[]", + [CACHE_PATH]: cacheRecord({wasDraft: true}), + }), + ); + const {plan} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("full"); + expect(plan.reasons).toEqual(["ready-for-review-anchor"]); + }); + + it("ignores the cache when pr-context carries no number", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/prior-reviews.json`]: "[]", + [CACHE_PATH]: cacheRecord(), + }), + ); + const {plan, stampSource} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("full"); + expect(stampSource).toBeNull(); + }); + + it("treats an unparseable cache record as no anchor", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/pr-context.json`]: contextWithNumber, + [`${REVIEW_DIR}/prior-reviews.json`]: "[]", + [CACHE_PATH]: "{not json", + }), + ); + const {plan, stampSource} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("full"); + expect(stampSource).toBeNull(); + }); +}); + describe("runRereviewStampCli", () => { it("renders this run's stamp from the staged plan and the decided verdict", () => { const fs = fakeFs(stagedInputs()); diff --git a/workflows/review/lib/rereview-mode.ts b/workflows/review/lib/rereview-mode.ts index 4e61e9da..7e9f7408 100644 --- a/workflows/review/lib/rereview-mode.ts +++ b/workflows/review/lib/rereview-mode.ts @@ -21,14 +21,34 @@ * 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. + * 3. **Divergence tripwire.** Every full-depth review records a + * content-hashed hunk signature. 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. + * + * **Fingerprint carriers.** The signature is written to two places and read + * back in priority order: + * + * 1. The hidden-comment stamp in the review body. This was designed as the + * durable carrier (it would survive cache eviction and branch + * protection's dismiss-stale-approvals), but gh-aw's safe-output ingest + * sanitizer strips ALL XML/HTML comments (`removeXmlComments` in + * gh-aw-actions `sanitize_content_core.cjs`), so a stamp posted through + * `submit_pull_request_review` never reaches the PR. Measured in + * production 2026-07-21 (Khan/webapp#40996: every re-review planned + * `no-prior-fingerprint` and escalated to full). The stamp is still + * emitted and still parsed first: it costs nothing, it documents the + * run, and it becomes load-bearing again the day the sanitizer allows + * it through or another submission path posts it verbatim. + * 2. The cache-memory record (`/tmp/gh-aw/cache-memory/pr-.json`), + * whose Step 9 fields (`verdict`, `stampHunks` — falling back to + * `reviewedHunks` where a consumer's Step 9 wrote the code-computed + * signature there — and `wasDraft`) carry the same information. This + * is the carrier that works today. Cache eviction degrades to `full` + * (more review, never less), which is exactly the pre-fix steady + * state. * * Two interactions are handled by construction: * @@ -343,6 +363,71 @@ export const findLatestStamp = ( return null; }; +/** + * Reconstruct a stamp from the Step 9 cache-memory record (the fallback + * fingerprint carrier; see the module header). The record is model-written + * in task mode, so every field is validated and any gap returns null: a + * fingerprint we cannot trust anchors nothing, and the depth decision + * degrades to `full`. The executed depth is not recorded there, so the + * reconstructed stamp carries `full` (the field is informational; no + * consumer branches on it). + */ +export const stampFromCacheMemory = (raw: unknown): ReReviewStamp | null => { + if (typeof raw !== "object" || raw === null || Array.isArray(raw)) { + return null; + } + const record = raw as { + verdict?: unknown; + stampHunks?: unknown; + reviewedHunks?: unknown; + wasDraft?: unknown; + }; + if (record.verdict !== "APPROVE" && record.verdict !== "REQUEST_CHANGES") { + return null; + } + if (typeof record.wasDraft !== "boolean") { + return null; + } + const validSignature = (hunks: unknown): HunkSignature | null => { + if ( + typeof hunks !== "object" || + hunks === null || + Array.isArray(hunks) + ) { + return null; + } + const signature: HunkSignature = {}; + for (const [path, hashes] of Object.entries(hunks)) { + if ( + !Array.isArray(hashes) || + hashes.some((hash) => typeof hash !== "string" || hash === "") + ) { + return null; + } + signature[path] = hashes as string[]; + } + return Object.keys(signature).length === 0 ? null : signature; + }; + // `stampHunks` is the field Step 9 copies verbatim from the plan CLI's + // own computation; `reviewedHunks` is accepted for consumers whose + // Step 9 wrote the code-computed signature there (the scripted-mode + // staging layer does). A hash-regime mismatch inside either one cannot + // be detected here; it surfaces as full divergence, i.e. a full review. + const signature = + validSignature(record.stampHunks) ?? + validSignature(record.reviewedHunks); + if (signature === null) { + return null; + } + return { + schemaVersion: STAMP_SCHEMA_VERSION, + depth: "full", + verdict: record.verdict, + anchorDraft: record.wasDraft, + anchorHunks: signature, + }; +}; + /* -------------------------------------------------------------------------- */ /* The depth decision */ /* -------------------------------------------------------------------------- */ @@ -520,10 +605,16 @@ export const buildScopedDiff = ( * `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 + * `reReviewMode`), `pr-context.json` (for `isDraft` and `number`), 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 + * too). When no prior-review body carries a stamp (in production none ever + * does; the ingest sanitizer strips it, see the module header), the anchor + * falls back to `/tmp/gh-aw/cache-memory/pr-.json` via + * {@link stampFromCacheMemory}. It writes `rereview-plan.json` (the + * {@link ReReviewPlan}, plus `stampSource`: + * `"review-body" | "cache-memory" | null`, recording which carrier + * anchored the plan). 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 @@ -539,6 +630,7 @@ 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 CACHE_MEMORY_DIR = "/tmp/gh-aw/cache-memory"; const PLAN_OUT = `${REVIEW_DIR}/rereview-plan.json`; const SCOPED_DIFF_OUT = `${REVIEW_DIR}/scoped.diff`; @@ -560,10 +652,14 @@ const readJsonIfPresent = (fs: RereviewCliFs, path: string): unknown => { } }; +/** Which carrier anchored the plan's prior fingerprint. */ +export type StampSource = "review-body" | "cache-memory" | null; + export type RereviewPlanCliResult = { plan: ReReviewPlan; /** Fixed-format staging problems (each also forced the plan to full). */ warnings: string[]; + stampSource: StampSource; }; /** @@ -590,7 +686,7 @@ export const runRereviewPlanCli = ( } const prContext = readJsonIfPresent(fs, PR_CONTEXT_PATH) as - | {isDraft?: unknown} + | {isDraft?: unknown; number?: unknown} | undefined; let isDraft = false; if (prContext !== undefined && typeof prContext.isDraft === "boolean") { @@ -633,7 +729,19 @@ export const runRereviewPlanCli = ( })) : []; - const priorStamp = findLatestStamp(priorReviews); + let priorStamp = findLatestStamp(priorReviews); + let stampSource: StampSource = priorStamp === null ? null : "review-body"; + if (priorStamp === null && typeof prContext?.number === "number") { + priorStamp = stampFromCacheMemory( + readJsonIfPresent( + fs, + `${CACHE_MEMORY_DIR}/pr-${prContext.number}.json`, + ), + ); + if (priorStamp !== null) { + stampSource = "cache-memory"; + } + } const plan = decideReReviewDepth({ mode, isDraft, @@ -642,7 +750,7 @@ export const runRereviewPlanCli = ( }); fs.mkdirSync(REVIEW_DIR, {recursive: true}); - fs.writeFileSync(PLAN_OUT, JSON.stringify(plan, null, 2)); + fs.writeFileSync(PLAN_OUT, JSON.stringify({...plan, stampSource}, 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 ( @@ -657,7 +765,7 @@ export const runRereviewPlanCli = ( ); } - return {plan, warnings}; + return {plan, warnings, stampSource}; }; /** @@ -714,6 +822,7 @@ if (typeof require !== "undefined" && require.main === module) { tripwireRearmed: result.plan.tripwireRearmed, unreviewedShare: result.plan.divergence?.unreviewedShare ?? null, + stampSource: result.stampSource, warnings: result.warnings, }), ); diff --git a/workflows/review/review.md b/workflows/review/review.md index 02bce5f9..507118a2 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -380,7 +380,10 @@ 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. +state is ignored here. Do not filter or truncate the bodies. (In practice gh-aw's +safe-output sanitizer strips the stamp comment before the review posts, so these +bodies usually carry none; the CLI then falls back to the Step 9 cache-memory +record. Stage them anyway: the body stamp is read first whenever it exists.) ## Step 2: Early-Exit Check @@ -1613,9 +1616,17 @@ Save to `/tmp/gh-aw/cache-memory/pr-${{ github.event.pull_request.number || gith 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. (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.) + only; both sides of that comparison are Step 1's own added-lines hash.) +- `stampHunks`: copy **verbatim** from `rereview-plan.json`'s `stampHunks` field (the + plan CLI wrote it in Step 3). This, with `verdict` and `wasDraft`, is the divergence + tripwire's working fingerprint carrier: gh-aw's safe-output sanitizer strips the + hidden body stamp before the review posts, so the Step 6 stamp (still emitted, and + still read first if ever present) never survives to the PR today, and the next + run's plan CLI anchors on this cache record instead. Never hand-compute it: the + CLI compares it hash-for-hash against its own computation, which hashes added AND + removed lines (Step 1's added-lines hash is a different regime and must not be + mixed in). Cache eviction degrades the next run to a full review, never a cheaper + one. - `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