Skip to content

review: re-review fingerprint anchors on cache memory (body stamp never survives gh-aw ingest) #287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jwbron wants to merge 1 commit into from
Closed

review: re-review fingerprint anchors on cache memory (body stamp never survives gh-aw ingest) #287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/stamp-carrier-cache-memory.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"review": patch

@github-actions github-actions Bot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): This ships as "review": patch, but the fix changes production re-review depth behavior (re-reviews that escalated to full will now anchor and run scoped/flip-gated/fast per the ROUTING dial). The two prior behavior-changing changesets (the payload seam and NOTIFIED) both shipped minor, while patch has been used for internal/eval-only fixes. Is patch intended here, or should this be minor?

---

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.
153 changes: 153 additions & 0 deletions workflows/review/lib/rereview-mode.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
runRereviewPlanCli,
runRereviewStampCli,
STAMP_SCHEMA_VERSION,
stampFromCacheMemory,
} from "./rereview-mode";
import type {HunkSignature, ReReviewStamp} from "./rereview-mode";

Expand Down Expand Up @@ -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, unknown> = {}): 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());
Expand Down
137 changes: 123 additions & 14 deletions workflows/review/lib/rereview-mode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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-<n>.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:
*
Expand Down Expand Up @@ -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") {

@github-actions github-actions Bot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): No test asserts stampFromCacheMemory accepts a REQUEST_CHANGES record and carries that verdict — the reconstruct test only uses APPROVE, and the REQUEST_CHANGES record seeded elsewhere is the body-wins case where the cache is never consumed. Fails toward full so it's low severity, but a one-line case would cover the accept-branch:

it("reconstructs a REQUEST_CHANGES anchor", () => {
    const stamp = stampFromCacheMemory(
        JSON.parse(cacheRecord({verdict: "REQUEST_CHANGES"})),
    );
    expect(stamp?.verdict).toBe("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);

@github-actions github-actions Bot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): The reviewedHunks fallback anchors on a regime-mismatched signature, so on a pre-upgrade cache record (has reviewedHunks, no stampHunks) the run posts a false "divergence tripwire re-armed" note. In production reviewedHunks is Step 1's 64-char SHA over added lines only, but the CLI hashes 16-char truncations over added+removed lines (HUNK_HASH_CHARS), so they can never string-match. validSignature does no length/regime check, so the anchor validates, computeDivergence matches nothing (share 1.0), and decideReReviewDepth returns tripwire-divergence/tripwireRearmed: true — posting divergence tripwire re-armed a full review (unreviewed share 1.00) when nothing diverged, plus a phantom re-arm in the cost metric. Review coverage stays correct (still full), so this is observability, not blocking.

Two supporting notes: the code comment here claims a "scripted-mode staging layer" writes reviewedHunks in the CLI regime, but no such writer exists in this repo; and the new test seeds reviewedHunks: CURRENT (the CLI-computed signature), so the production mismatch is never exercised. Accepting only the CLI regime here also subsumes the stampHunks: "overflow" case (both degrade to the honest no-prior-fingerprint):

if (
    !Array.isArray(hashes) ||
    hashes.some(
        (hash) => typeof hash !== "string" || !/^[0-9a-f]{16}$/.test(hash),
    )
) {
    return null;
}

if (signature === null) {
return null;
}
return {
schemaVersion: STAMP_SCHEMA_VERSION,
depth: "full",
verdict: record.verdict,
anchorDraft: record.wasDraft,

@github-actions github-actions Bot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note (non-blocking): anchorDraft is reconstructed from wasDraft, which is the last run's draft status, whereas anchorDraft means the draft status when the anchor fingerprint was taken. Tracing both directions this is conservative-only (it can only add a full review, never miss one), but the docstring documents the depth: "full" approximation and not this one — worth a line so a future reader doesn't "fix" it by carrying stampAnchorDraft into the cache record without re-deriving the analysis.

anchorHunks: signature,
};
};

/* -------------------------------------------------------------------------- */
/* The depth decision */
/* -------------------------------------------------------------------------- */
Expand Down Expand Up @@ -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-<number>.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
Expand All @@ -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`;

Expand All @@ -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;
};

/**
Expand All @@ -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") {
Expand Down Expand Up @@ -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,
Expand All @@ -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 (
Expand All @@ -657,7 +765,7 @@ export const runRereviewPlanCli = (
);
}

return {plan, warnings};
return {plan, warnings, stampSource};
};

/**
Expand Down Expand Up @@ -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,
}),
);
Expand Down
Loading
Loading