From 849ee2949483a0fd76b3d3c3e45b466c8fafad1b Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Thu, 10 Sep 2026 17:43:13 +0000 Subject: [PATCH 1/4] feat(adapter-utils): refuse a PR review whose attestation is unreachable in the target repo (BLO-32844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review computed against Blockcast/pim-multicast-gateway#2864 was submitted to Blockcast/mediamtx#33 as APPROVED. GitHub stamps `commit_id` from the *target* PR at submission time, so it read mediamtx#33's real head and every consumer checking `state === "APPROVED" && commit_id === head` saw a correctly approved head. Only the body's `Reviewed head:` marker disagreed — it named 81d63ac9, which resolves nowhere in mediamtx (HTTP 422). That is a fail-open: in a repo where an Ally approval gates merge it admits entirely unreviewed code. The control plane never posts a review, so there is no server/ code path to hook. Agents shell out to `gh`, and the Helm-seeded launcher in front of /usr/bin/gh (statefulset.yaml:458) is the only interposition point. The guard goes there, beside the existing stdin fail-closed check. Detection is deliberately looser than the consumer's grammar and validation stricter. ally-review-detection.ts answers "is there a usable attestation?" and correctly ignores a malformed one; a producer asking that question would wave the malformed review through. So this finds any line that *intends* to attest, then demands exactly 40 hex — which also closes the opposite, fail-CLOSED defect: review-gate-action#7 reviews 5013561307/5013569224 carry a 42-character marker, so REVIEWED_HEAD_ATTESTATION_PATTERN matches nothing, the review attests no head, and it can never satisfy the gate at any head. Reachability rather than head equality is the test: reviewing a prior head is legitimate, and only "does this commit exist in this repository" separates that from a review of another repo. Probed via GET /repos/{o}/{r}/commits/{sha} rather than commit search, which is index-backed and empty for a fresh push. Every indeterminate outcome refuses, because the costs are asymmetric — a refused review is recoverable by re-running, an admitted false approval is merge-visible immediately and only a human dismissal removes it. Verified against the live API: 81d63ac9 reads unreachable in mediamtx and reachable in pim-multicast-gateway; replaying the real contaminated body at mediamtx#33 refuses with exit 65 and never starts the CLI, while the genuine mediamtx review at dbe4e10e and the same body against its correct repo both pass. Ally's idempotency GET of the reviews path is untouched. Co-Authored-By: Claude --- .../src/github-cli-egress-runtime.ts | 97 +++- .../src/github-review-attestation.test.ts | 480 ++++++++++++++++++ .../src/github-review-attestation.ts | 475 +++++++++++++++++ 3 files changed, 1051 insertions(+), 1 deletion(-) create mode 100644 packages/adapter-utils/src/github-review-attestation.test.ts create mode 100644 packages/adapter-utils/src/github-review-attestation.ts diff --git a/packages/adapter-utils/src/github-cli-egress-runtime.ts b/packages/adapter-utils/src/github-cli-egress-runtime.ts index 15cab4f2fc0b..892f35810941 100644 --- a/packages/adapter-utils/src/github-cli-egress-runtime.ts +++ b/packages/adapter-utils/src/github-cli-egress-runtime.ts @@ -17,10 +17,18 @@ import { hasGitHubCliStdinTextFile, scrubGitHubCliInvocation, } from "./github-cli-egress-shim.js"; +import { + type CommitReachability, + evaluateReviewSubmission, + type ReviewAttestationGuardIo, +} from "./github-review-attestation.js"; export interface GitHubCliEgressRuntimeOptions { target: string; argv: string[]; + /** Override the review-attestation guard's I/O. Tests inject a resolver so + * the guard can be exercised without a network or a real repository. */ + guardIo?: ReviewAttestationGuardIo; } export class GitHubCliEgressRuntimeError extends Error { @@ -68,9 +76,96 @@ export function prepareGitHubCliInvocation(options: GitHubCliEgressRuntimeOption return { argv: result.argv, temporaryDirectory }; } -export function runGitHubCliEgressRuntime( +/** + * Run the real GitHub CLI and capture its output. + * + * `target` is the binary this wrapper fronts (/usr/bin/gh in the pod), never + * the wrapper itself, so the guard's own probe calls cannot recurse back + * through this runtime. + */ +function captureTarget( + target: string, + argv: readonly string[], +): Promise<{ code: number | null; stdout: string; stderr: string }> { + return new Promise((resolve) => { + const child = spawn(target, [...argv], { stdio: ["ignore", "pipe", "pipe"] }); + let stdout = ""; + let stderr = ""; + child.stdout?.setEncoding("utf8").on("data", (chunk: string) => { + stdout += chunk; + }); + child.stderr?.setEncoding("utf8").on("data", (chunk: string) => { + stderr += chunk; + }); + child.once("error", (error: NodeJS.ErrnoException) => { + resolve({ code: null, stdout, stderr: `${stderr}${error.code ?? "spawn failed"}` }); + }); + child.once("close", (code) => { + resolve({ code, stdout, stderr }); + }); + }); +} + +// A definite "this commit is not in this repository". `gh api` reports a +// nonexistent SHA as 422 ("No commit found for SHA") and a missing or +// invisible repository as 404. Anything else — a 5xx, a DNS failure, an auth +// problem — is deliberately NOT matched here, so it falls through to +// "indeterminate" and is refused rather than mistaken for a clean negative. +const DEFINITE_ABSENCE_PATTERN = /HTTP 404|HTTP 422|Not Found|No commit found/i; + +/** + * Build the guard's I/O against the real GitHub CLI. + * + * Reachability is asked via `GET /repos/{owner}/{repo}/commits/{sha}` rather + * than commit search: search is index-backed and returns an empty result for a + * commit pushed moments earlier, which would refuse legitimate reviews of a + * fresh head. The direct endpoint is authoritative. + */ +export function createReviewAttestationGuardIo(target: string): ReviewAttestationGuardIo { + return { + readText: (filePath) => readFileSync(filePath, "utf8"), + resolveCommitReachability: async (repo, sha): Promise => { + const result = await captureTarget(target, [ + "api", + `repos/${repo}/commits/${sha}`, + "--jq", + ".sha", + ]); + if (result.code === 0 && result.stdout.trim().length > 0) return "reachable"; + if (DEFINITE_ABSENCE_PATTERN.test(result.stderr)) return "unreachable"; + return "indeterminate"; + }, + resolveDefaultRepo: async () => { + const result = await captureTarget(target, [ + "repo", + "view", + "--json", + "nameWithOwner", + "--jq", + ".nameWithOwner", + ]); + if (result.code !== 0) return null; + const name = result.stdout.trim(); + return name.length > 0 ? name : null; + }, + }; +} + +export async function runGitHubCliEgressRuntime( options: GitHubCliEgressRuntimeOptions, ): Promise { + // BLO-32844: refuse an incoherent review before anything is scrubbed or + // spawned. This runs first because it is the only check whose failure means + // the call must not happen at all — the scrub rewrites a call that is going + // to proceed, whereas this one cancels it. + const refusal = await evaluateReviewSubmission( + options.argv, + options.guardIo ?? createReviewAttestationGuardIo(options.target), + ); + if (refusal) { + throw new GitHubCliEgressRuntimeError(`${refusal.message} [${refusal.reason}]`, 65); + } + const invocation = prepareGitHubCliInvocation(options); return new Promise((resolve, reject) => { const child = spawn(options.target, invocation.argv, { stdio: "inherit" }); diff --git a/packages/adapter-utils/src/github-review-attestation.test.ts b/packages/adapter-utils/src/github-review-attestation.test.ts new file mode 100644 index 000000000000..df8408f6d213 --- /dev/null +++ b/packages/adapter-utils/src/github-review-attestation.test.ts @@ -0,0 +1,480 @@ +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + evaluateReviewSubmission, + inspectReviewAttestation, + parseReviewSubmission, + type CommitReachability, + type ReviewAttestationGuardIo, +} from "./github-review-attestation.js"; +import { runGitHubCliEgressRuntime } from "./github-cli-egress-runtime.js"; + +// Real values from the two incidents this guard exists for, so the regression +// is pinned to what actually happened rather than to a synthetic shape. +// +// BLO-32844 — cross-repository contamination (fails OPEN): +const MEDIAMTX_HEAD = "dbe4e10e1068fc43383b59ea5a52ea1bb6134fce"; +const PIM_2864_HEAD = "81d63ac9ca72ddafc12919d3b40ae0870a901b7f"; +// The malformed-marker defect on review-gate-action#7 (fails CLOSED): the real +// SHA with "f6" spliced in at offset 25, giving 42 characters. +const MALFORMED_MARKER = "160a571d83f8e0bcd6dc23a072f6f9a9bda1983556"; +const REVIEW_GATE_ACTION_HEAD = "160a571d83f8e0bcd6dc23a072f9a9bda1983556"; + +/** The contaminated review body as posted to mediamtx#33, trimmed. */ +function bodyAttesting(sha: string): string { + return [ + "## Ally — Consolidated PR Review", + "", + "_Lenses: pr-review-toolkit + gstack/review._", + `Reviewed head: ${sha}`, + "", + "Looks good. The change pins the cached wasm-pack toolchain.", + "", + "### Critical Issues (0)", + "", + "None.", + ].join("\n"); +} + +function makeIo( + overrides: Partial = {}, +): ReviewAttestationGuardIo & { + reachabilityCalls: Array<{ repo: string; sha: string }>; +} { + const reachabilityCalls: Array<{ repo: string; sha: string }> = []; + return { + reachabilityCalls, + readText: overrides.readText ?? (() => bodyAttesting(MEDIAMTX_HEAD)), + resolveCommitReachability: + overrides.resolveCommitReachability ?? + (async (repo, sha) => { + reachabilityCalls.push({ repo, sha }); + return "reachable" as CommitReachability; + }), + resolveDefaultRepo: overrides.resolveDefaultRepo ?? (async () => null), + }; +} + +describe("parseReviewSubmission", () => { + it("recognises the review form Ally actually posts", () => { + const submission = parseReviewSubmission([ + "pr", + "review", + "33", + "--repo", + "Blockcast/mediamtx", + "--approve", + "--body-file", + "/tmp/ally-review.md", + ]); + + expect(submission).toEqual({ + repo: "Blockcast/mediamtx", + repoSource: "argv-flag", + pullNumber: 33, + body: { kind: "file", path: "/tmp/ally-review.md" }, + }); + }); + + it("accepts fused long and short spellings of the same flags", () => { + expect( + parseReviewSubmission([ + "pr", + "review", + "33", + "--repo=Blockcast/mediamtx", + "--body-file=/tmp/b.md", + ]), + ).toMatchObject({ repo: "Blockcast/mediamtx", body: { kind: "file", path: "/tmp/b.md" } }); + + expect( + parseReviewSubmission(["pr", "review", "33", "-RBlockcast/mediamtx", "-F/tmp/b.md"]), + ).toMatchObject({ repo: "Blockcast/mediamtx", body: { kind: "file", path: "/tmp/b.md" } }); + }); + + // `gh pr review` spells --body-file as -F, which is --field on `gh api`. A + // single shared flag table would read the body path as a field expression + // and silently stop guarding this form. + it("treats -F on pr review as the body file, not as a typed api field", () => { + expect(parseReviewSubmission(["pr", "review", "7", "-F", "/tmp/body.md"])).toMatchObject({ + body: { kind: "file", path: "/tmp/body.md" }, + }); + }); + + it("reads repo and number out of a pull request URL", () => { + expect( + parseReviewSubmission([ + "pr", + "review", + "https://github.com/Blockcast/mediamtx/pull/33", + "--approve", + "--body", + "text", + ]), + ).toMatchObject({ + repo: "Blockcast/mediamtx", + repoSource: "argv-url", + pullNumber: 33, + body: { kind: "inline", text: "text" }, + }); + }); + + it("does not mistake a flag value for the positional PR argument", () => { + expect( + parseReviewSubmission(["pr", "review", "--repo", "Blockcast/mediamtx", "33", "--comment"]), + ).toMatchObject({ repo: "Blockcast/mediamtx", pullNumber: 33 }); + }); + + it("recognises the raw api submission form", () => { + expect( + parseReviewSubmission([ + "api", + "repos/Blockcast/mediamtx/pulls/33/reviews", + "-X", + "POST", + "-f", + "event=COMMENT", + "-F", + "body=@/tmp/ally-review.md", + ]), + ).toEqual({ + repo: "Blockcast/mediamtx", + repoSource: "argv-api-path", + pullNumber: 33, + body: { kind: "file", path: "/tmp/ally-review.md" }, + }); + }); + + // Ally's own idempotency step GETs this exact path before deciding whether to + // review. Guarding a read would double the cost of every review for nothing. + it("ignores a GET against the reviews path", () => { + expect(parseReviewSubmission(["api", "repos/Blockcast/mediamtx/pulls/33/reviews"])).toBeNull(); + expect( + parseReviewSubmission(["api", "repos/Blockcast/mediamtx/pulls/33/reviews", "-X", "GET"]), + ).toBeNull(); + }); + + it("ignores invocations that are not review submissions", () => { + expect(parseReviewSubmission(["pr", "comment", "33", "--body", "hi"])).toBeNull(); + expect(parseReviewSubmission(["pr", "view", "33"])).toBeNull(); + expect(parseReviewSubmission(["api", "repos/Blockcast/mediamtx/issues/33/comments"])).toBeNull(); + expect(parseReviewSubmission([])).toBeNull(); + }); +}); + +describe("inspectReviewAttestation", () => { + it("extracts a well-formed attestation", () => { + expect(inspectReviewAttestation(bodyAttesting(PIM_2864_HEAD))).toEqual({ + kind: "well-formed", + sha: PIM_2864_HEAD, + }); + }); + + it("accepts the backticked and upper-case spellings the consumer accepts", () => { + expect( + inspectReviewAttestation(`## Ally — Consolidated PR Review\nReviewed head: \`${PIM_2864_HEAD}\``), + ).toEqual({ kind: "well-formed", sha: PIM_2864_HEAD }); + + expect( + inspectReviewAttestation(`Reviewed head: ${PIM_2864_HEAD.toUpperCase()}`), + ).toEqual({ kind: "well-formed", sha: PIM_2864_HEAD }); + }); + + it("reports no attestation when the body carries none", () => { + expect(inspectReviewAttestation("## Ally — Consolidated PR Review\n\nLooks good.")).toEqual({ + kind: "absent", + }); + }); + + // The defect the consumer cannot see: extractAllyReviewedHeadSha requires + // exactly 40 hex followed by end-of-line, so a 42-character token matches + // nothing and reads as "no attestation" rather than as a broken one. + it("classifies the real 42-character marker as malformed, not absent", () => { + const attestation = inspectReviewAttestation(bodyAttesting(MALFORMED_MARKER)); + expect(attestation).toEqual({ kind: "malformed", raw: MALFORMED_MARKER }); + expect(MALFORMED_MARKER).toHaveLength(42); + }); + + it("treats several attestations as ambiguous", () => { + const body = `Reviewed head: ${PIM_2864_HEAD}\nReviewed head: ${MEDIAMTX_HEAD}`; + expect(inspectReviewAttestation(body)).toEqual({ + kind: "ambiguous", + raw: [PIM_2864_HEAD, MEDIAMTX_HEAD], + }); + }); + + // A review *of this guard* quotes `Reviewed head:` lines in fenced examples. + // Counting those would refuse honest reviews of the refusing code itself. + it("ignores an attestation quoted inside a fenced block", () => { + const body = [ + "## Ally — Consolidated PR Review", + `Reviewed head: ${MEDIAMTX_HEAD}`, + "", + "The docs show:", + "```", + `Reviewed head: ${PIM_2864_HEAD}`, + "```", + ].join("\n"); + + expect(inspectReviewAttestation(body)).toEqual({ kind: "well-formed", sha: MEDIAMTX_HEAD }); + }); + + it("ignores an attestation indented into a code block", () => { + expect(inspectReviewAttestation(`text\n Reviewed head: ${PIM_2864_HEAD}`)).toEqual({ + kind: "absent", + }); + }); +}); + +describe("evaluateReviewSubmission", () => { + const crossRepoArgv = [ + "pr", + "review", + "33", + "--repo", + "Blockcast/mediamtx", + "--approve", + "--body-file", + "/tmp/ally-review.md", + ]; + + // THE REGRESSION. Review 5146990033: computed against pim#2864, submitted to + // mediamtx#33. GitHub stamped commit_id with mediamtx#33's real head, so + // every commit_id-based check passed; only the body marker disagreed. + it("refuses a review whose attestation names a commit absent from the target repo", async () => { + const io = makeIo({ + readText: () => bodyAttesting(PIM_2864_HEAD), + resolveCommitReachability: async () => "unreachable", + }); + + const refusal = await evaluateReviewSubmission(crossRepoArgv, io); + + expect(refusal).not.toBeNull(); + expect(refusal?.reason).toBe("unreachable-attestation"); + expect(refusal?.message).toContain(PIM_2864_HEAD); + expect(refusal?.message).toContain("Blockcast/mediamtx"); + }); + + it("asks about the attested SHA in the argv-named repository", async () => { + const io = makeIo({ readText: () => bodyAttesting(PIM_2864_HEAD) }); + + await evaluateReviewSubmission(crossRepoArgv, io); + + expect(io.reachabilityCalls).toEqual([ + { repo: "Blockcast/mediamtx", sha: PIM_2864_HEAD }, + ]); + }); + + // Rejected locally: a 42-character token cannot be a commit anywhere, so + // spending an API call to discover that is pure latency. + it("refuses a malformed attestation without consulting the network", async () => { + const resolveCommitReachability = vi.fn(async () => "reachable" as CommitReachability); + const io = makeIo({ + readText: () => bodyAttesting(MALFORMED_MARKER), + resolveCommitReachability, + }); + + const refusal = await evaluateReviewSubmission(crossRepoArgv, io); + + expect(refusal?.reason).toBe("malformed-attestation"); + expect(refusal?.message).toContain("42 characters"); + expect(resolveCommitReachability).not.toHaveBeenCalled(); + }); + + it("refuses an ambiguous attestation", async () => { + const io = makeIo({ + readText: () => `Reviewed head: ${PIM_2864_HEAD}\nReviewed head: ${MEDIAMTX_HEAD}`, + }); + + expect((await evaluateReviewSubmission(crossRepoArgv, io))?.reason).toBe( + "ambiguous-attestation", + ); + }); + + // Reviewing a prior head is legitimate and common; the gate's own staleness + // rules handle it. Refusing it here would break normal review traffic, so + // reachability — not head equality — is the test. + it("allows an attestation for a prior head that still exists in the repo", async () => { + const io = makeIo({ + readText: () => bodyAttesting(REVIEW_GATE_ACTION_HEAD), + resolveCommitReachability: async () => "reachable", + }); + + expect(await evaluateReviewSubmission(crossRepoArgv, io)).toBeNull(); + }); + + it("refuses when reachability cannot be determined", async () => { + const io = makeIo({ + readText: () => bodyAttesting(PIM_2864_HEAD), + resolveCommitReachability: async () => "indeterminate", + }); + + const refusal = await evaluateReviewSubmission(crossRepoArgv, io); + expect(refusal?.reason).toBe("unreachable-attestation"); + expect(refusal?.message).toContain("could not confirm"); + }); + + it("passes through a review that attests nothing", async () => { + const resolveCommitReachability = vi.fn(async () => "reachable" as CommitReachability); + const io = makeIo({ readText: () => "## Ally — Consolidated PR Review\n\nLooks good.", resolveCommitReachability }); + + expect(await evaluateReviewSubmission(crossRepoArgv, io)).toBeNull(); + expect(resolveCommitReachability).not.toHaveBeenCalled(); + }); + + it("passes through invocations that are not review submissions", async () => { + const resolveCommitReachability = vi.fn(async () => "reachable" as CommitReachability); + const io = makeIo({ resolveCommitReachability }); + + expect( + await evaluateReviewSubmission(["pr", "comment", "33", "--body", "hi"], io), + ).toBeNull(); + expect(resolveCommitReachability).not.toHaveBeenCalled(); + }); + + it("falls back to gh's default repository when argv names none", async () => { + const io = makeIo({ + readText: () => bodyAttesting(PIM_2864_HEAD), + resolveDefaultRepo: async () => "Blockcast/pim-multicast-gateway", + }); + + expect( + await evaluateReviewSubmission(["pr", "review", "2864", "--body-file", "/tmp/b.md"], io), + ).toBeNull(); + expect(io.reachabilityCalls).toEqual([ + { repo: "Blockcast/pim-multicast-gateway", sha: PIM_2864_HEAD }, + ]); + }); + + it("refuses when neither argv nor the checkout names a target repository", async () => { + const io = makeIo({ + readText: () => bodyAttesting(PIM_2864_HEAD), + resolveDefaultRepo: async () => null, + }); + + expect( + (await evaluateReviewSubmission(["pr", "review", "33", "--body-file", "/tmp/b.md"], io)) + ?.reason, + ).toBe("unresolved-target"); + }); + + it("refuses when the body file cannot be read", async () => { + const io = makeIo({ + readText: () => { + throw new Error("ENOENT"); + }, + }); + + expect((await evaluateReviewSubmission(crossRepoArgv, io))?.reason).toBe("unreadable-body"); + }); + + it("guards an inline body too", async () => { + const io = makeIo({ resolveCommitReachability: async () => "unreachable" }); + + const refusal = await evaluateReviewSubmission( + [ + "pr", + "review", + "33", + "--repo", + "Blockcast/mediamtx", + "--approve", + "--body", + bodyAttesting(PIM_2864_HEAD), + ], + io, + ); + + expect(refusal?.reason).toBe("unreachable-attestation"); + }); +}); + +describe("runGitHubCliEgressRuntime review guard", () => { + const temporaryDirectories: string[] = []; + + afterEach(() => { + while (temporaryDirectories.length > 0) { + const directory = temporaryDirectories.pop(); + if (directory) rmSync(directory, { recursive: true, force: true }); + } + }); + + function makeRecordingTarget(): { target: string; record: string; bodyPath: string } { + const directory = mkdtempSync(path.join(os.tmpdir(), "paperclip-review-guard-")); + temporaryDirectories.push(directory); + const target = path.join(directory, "gh-target.mjs"); + const record = path.join(directory, "target-record.json"); + const bodyPath = path.join(directory, "review.md"); + + writeFileSync( + target, + [ + "#!/usr/bin/env node", + 'import { writeFileSync } from "node:fs";', + `writeFileSync(${JSON.stringify(record)}, JSON.stringify({ argv: process.argv.slice(2) }));`, + "", + ].join("\n"), + { mode: 0o755 }, + ); + writeFileSync(bodyPath, bodyAttesting(PIM_2864_HEAD)); + + return { target, record, bodyPath }; + } + + // The whole point of the guard: the review must never reach GitHub. A + // refusal that still spawned the CLI would have already posted the review. + it("never starts the GitHub CLI when the attestation is unreachable", async () => { + const fixture = makeRecordingTarget(); + + await expect( + runGitHubCliEgressRuntime({ + target: fixture.target, + argv: [ + "pr", + "review", + "33", + "--repo", + "Blockcast/mediamtx", + "--approve", + "--body-file", + fixture.bodyPath, + ], + guardIo: makeIo({ + readText: (filePath) => readFileSync(filePath, "utf8"), + resolveCommitReachability: async () => "unreachable", + }), + }), + ).rejects.toThrow("unreachable-attestation"); + + expect(() => readFileSync(fixture.record, "utf8")).toThrow(); + }); + + it("runs the GitHub CLI when the attestation resolves in the target repo", async () => { + const fixture = makeRecordingTarget(); + + const exitCode = await runGitHubCliEgressRuntime({ + target: fixture.target, + argv: [ + "pr", + "review", + "2864", + "--repo", + "Blockcast/pim-multicast-gateway", + "--approve", + "--body-file", + fixture.bodyPath, + ], + guardIo: makeIo({ + readText: (filePath) => readFileSync(filePath, "utf8"), + resolveCommitReachability: async () => "reachable", + }), + }); + + expect(exitCode).toBe(0); + const recorded = JSON.parse(readFileSync(fixture.record, "utf8")) as { argv: string[] }; + expect(recorded.argv).toContain("--approve"); + }); +}); diff --git a/packages/adapter-utils/src/github-review-attestation.ts b/packages/adapter-utils/src/github-review-attestation.ts new file mode 100644 index 000000000000..4ed1ed31af0f --- /dev/null +++ b/packages/adapter-utils/src/github-review-attestation.ts @@ -0,0 +1,475 @@ +// BLO-32844: bind an agent-authored PR review to the pull request it is being +// submitted to, at the last moment before it leaves the pod. +// +// Where this lives, and why it cannot live in server/. +// +// The control plane never posts a review: there is no POST /pulls/{n}/reviews +// anywhere in server/. Agents compose a body and shell out to `gh pr review` +// or `gh api repos/{o}/{r}/pulls/{n}/reviews`, so the egress launcher described +// in github-cli-egress-shim.ts is the only code of ours in front of the call. +// There is no in-process "the post succeeded" moment to hook. +// +// What went wrong, and why GitHub's own metadata cannot detect it. +// +// A review computed against Blockcast/pim-multicast-gateway#2864 was submitted +// to Blockcast/mediamtx#33 (review 5146990033, 2026-09-08T21:04:48Z). GitHub +// stamps `commit_id` from the *target* PR at submission time, so it read +// mediamtx#33's real head: every consumer checking +// `state === "APPROVED" && commit_id === head` saw a correctly approved head. +// The body's `Reviewed head:` marker named 81d63ac9…, which is pim#2864's head +// and resolves nowhere in mediamtx (422). The marker is the only field that +// travels *with the review text*, so it is the only one that can disagree with +// the target — which makes it the sole signal for this class, and makes +// `commit_id` worthless for it. +// +// Two failure directions, one guard. +// +// - Unreachable marker — fails OPEN. The case above: an APPROVED review of +// code nobody looked at, carrying a green signal. In a repo where an Ally +// approval gates merge this admits entirely unreviewed code. +// - Malformed marker — fails CLOSED. review-gate-action#7 reviews +// 5013561307 and 5013569224 carry a 42-character marker (two characters +// spliced into a real SHA, `…a072` + `f6` + `f9a9…`). The consumer's +// REVIEWED_HEAD_ATTESTATION_PATTERN requires exactly [0-9a-f]{40} followed +// by end of line, so it matches nothing and extractAllyReviewedHeadSha +// returns null. The review attests no head and can never satisfy the gate +// at any head, with no in-band recovery. +// +// Both are refused here, before the call leaves the pod, because the costs are +// not symmetric. A refused submission is recoverable: the agent re-reviews and +// posts again. An admitted false approval is not — it is merge-visible the +// instant it lands and only a human dismissal removes it. That asymmetry is +// why every indeterminate outcome below refuses rather than passes. +// +// Detection must be LOOSER than the consumer's grammar; validation STRICTER. +// +// server/src/services/ally-review-detection.ts answers "is there a usable +// attestation?", and it is right to ignore a 42-char marker. A producer asking +// that same question would wave the malformed review straight through, because +// it also sees no attestation. So this module first finds any line that +// *intends* to attest, then demands the token be exactly 40 hex. Intent and +// validity are separate questions on the producing side, and only here. +// +// The two grammars are deliberately not shared: adapter-utils ships to agent +// pods and must not depend on server/. Unifying the Ally review grammar across +// scripts/, server/ and here is tracked as BLO-32512; if that lands, the +// candidate scanner below is the piece to fold in — keeping the loose/strict +// split, which is the part that is easy to lose in a merge. + +/** How the target repository was determined, for diagnostics in the refusal. */ +export type ReviewTargetSource = "argv-flag" | "argv-url" | "argv-api-path" | "resolved-default"; + +export interface ReviewSubmission { + /** "owner/name", or null when argv does not name it (`gh pr review` with no + * --repo relies on the checkout's remote, which argv cannot tell us). */ + repo: string | null; + repoSource: ReviewTargetSource | null; + /** The pull request number, when argv carries it. */ + pullNumber: number | null; + /** Inline body text, or a path to read it from. */ + body: { kind: "inline"; text: string } | { kind: "file"; path: string } | null; +} + +export type ReviewAttestation = + /** No line intends to attest. Nothing for this guard to check. */ + | { kind: "absent" } + /** Exactly one attesting line, token is exactly 40 lowercase hex. */ + | { kind: "well-formed"; sha: string } + /** Exactly one attesting line, token is not a SHA. The fail-closed defect. */ + | { kind: "malformed"; raw: string } + /** Several attesting lines. The consumer requires exactly one and returns + * null otherwise, so this is the malformed case by another route. */ + | { kind: "ambiguous"; raw: string[] }; + +/** Whether a SHA names a commit in a given repository. `indeterminate` covers + * every outcome that is neither a definite yes nor a definite no — a network + * failure, a 5xx, an auth problem. It is treated as a refusal by the caller; + * see the asymmetry note in the module header. */ +export type CommitReachability = "reachable" | "unreachable" | "indeterminate"; + +export interface ReviewAttestationGuardIo { + /** Read an authored body file. May throw; the caller turns that into a refusal. */ + readText(path: string): string; + resolveCommitReachability(repo: string, sha: string): Promise; + /** The repo `gh` itself would target when argv names none. */ + resolveDefaultRepo(): Promise; +} + +export interface ReviewAttestationRefusal { + /** Stable machine-readable cause, for logs and tests. */ + reason: + | "malformed-attestation" + | "ambiguous-attestation" + | "unreachable-attestation" + | "unresolved-target" + | "unreadable-body"; + message: string; +} + +// -- argv parsing ------------------------------------------------------------ + +/** `gh pr review` spells --body-file as -F. That collides with `gh api`'s + * typed --field, so the two forms must be parsed separately rather than by one + * shared flag table. */ +const PR_REVIEW_BODY_FILE_FLAGS = new Set(["--body-file", "-F"]); +const PR_REVIEW_BODY_INLINE_FLAGS = new Set(["--body", "-b"]); +const REPO_FLAGS = new Set(["--repo", "-R"]); + +/** Flags on `gh pr review` that consume the following argv element. Needed so + * a flag's value is never mistaken for the positional PR argument. */ +const PR_REVIEW_VALUE_FLAGS = new Set([ + "--body-file", + "-F", + "--body", + "-b", + "--repo", + "-R", +]); + +const PR_URL_PATTERN = /^https?:\/\/[^/]+\/([^/]+\/[^/]+)\/pull\/(\d+)(?:[/?#].*)?$/; +const API_REVIEWS_PATH_PATTERN = /^\/?repos\/([^/]+\/[^/]+)\/pulls\/(\d+)\/reviews\/?$/; + +function splitFused(arg: string): { flag: string; value: string } | null { + const long = /^(--[a-z-]+)=([\s\S]*)$/.exec(arg); + if (long) return { flag: long[1]!, value: long[2]! }; + // Short flags accept a fused value: -Rowner/name, -F/tmp/body.md. + const short = /^(-[A-Za-z])(.+)$/.exec(arg); + if (short) { + const value = short[2]!.startsWith("=") ? short[2]!.slice(1) : short[2]!; + return { flag: short[1]!, value }; + } + return null; +} + +/** + * Recognise a pull-request review submission in a `gh` argv. + * + * Returns null for everything else, including `gh pr comment` and read-only + * calls, so a non-review invocation is never delayed by this guard. + */ +export function parseReviewSubmission(argv: readonly string[]): ReviewSubmission | null { + if (argv[0] === "pr" && argv[1] === "review") return parsePrReview(argv.slice(2)); + if (argv[0] === "api") return parseApiReview(argv.slice(1)); + return null; +} + +function parsePrReview(rest: readonly string[]): ReviewSubmission { + const submission: ReviewSubmission = { + repo: null, + repoSource: null, + pullNumber: null, + body: null, + }; + + for (let i = 0; i < rest.length; i += 1) { + const arg = rest[i]!; + + const fused = splitFused(arg); + if (fused) { + if (REPO_FLAGS.has(fused.flag)) { + submission.repo = fused.value; + submission.repoSource = "argv-flag"; + } else if (PR_REVIEW_BODY_FILE_FLAGS.has(fused.flag)) { + submission.body = { kind: "file", path: fused.value }; + } else if (PR_REVIEW_BODY_INLINE_FLAGS.has(fused.flag)) { + submission.body = { kind: "inline", text: fused.value }; + } + continue; + } + + if (PR_REVIEW_VALUE_FLAGS.has(arg)) { + const value = rest[i + 1]; + if (value !== undefined) { + if (REPO_FLAGS.has(arg)) { + submission.repo = value; + submission.repoSource = "argv-flag"; + } else if (PR_REVIEW_BODY_FILE_FLAGS.has(arg)) { + submission.body = { kind: "file", path: value }; + } else { + submission.body = { kind: "inline", text: value }; + } + i += 1; + } + continue; + } + + if (arg.startsWith("-")) continue; + + // First bare positional is the PR selector: a number, a URL, or a branch. + // A branch name carries no number and is left null — the caller then falls + // back to gh's own default-repo resolution for reachability. + if (submission.pullNumber === null && submission.repoSource !== "argv-url") { + const url = PR_URL_PATTERN.exec(arg); + if (url) { + submission.repo = url[1]!; + submission.repoSource = "argv-url"; + submission.pullNumber = Number(url[2]!); + continue; + } + if (/^\d+$/.test(arg)) submission.pullNumber = Number(arg); + } + } + + return submission; +} + +/** + * Recognise `gh api repos/{o}/{r}/pulls/{n}/reviews -X POST`. + * + * A GET against the same path is a read and must pass through untouched — the + * idempotency check in Ally's own workflow issues exactly that call, so + * treating every `.../reviews` path as a submission would guard the read and + * double the cost of every review. + */ +function parseApiReview(rest: readonly string[]): ReviewSubmission | null { + let path: string | null = null; + let method: string | null = null; + let body: ReviewSubmission["body"] = null; + + for (let i = 0; i < rest.length; i += 1) { + const arg = rest[i]!; + + const fused = splitFused(arg); + if (fused) { + if (fused.flag === "--method" || fused.flag === "-X") method = fused.value; + else if (fused.flag === "--input") body = { kind: "file", path: fused.value }; + else if (isFieldFlag(fused.flag)) { + const parsed = parseBodyField(fused.value); + if (parsed) body = parsed; + } + continue; + } + + if (arg === "--method" || arg === "-X") { + const value = rest[i + 1]; + if (value !== undefined) { + method = value; + i += 1; + } + continue; + } + if (arg === "--input") { + const value = rest[i + 1]; + if (value !== undefined) { + body = { kind: "file", path: value }; + i += 1; + } + continue; + } + if (isFieldFlag(arg)) { + const value = rest[i + 1]; + if (value !== undefined) { + const parsed = parseBodyField(value); + if (parsed) body = parsed; + i += 1; + } + continue; + } + + if (!arg.startsWith("-") && path === null) path = arg; + } + + if (path === null) return null; + const match = API_REVIEWS_PATH_PATTERN.exec(path); + if (!match) return null; + // `gh api` defaults to GET; a body-bearing field flag implies POST the same + // way gh itself infers it. + const isWrite = method !== null ? method.toUpperCase() !== "GET" : body !== null; + if (!isWrite) return null; + + return { + repo: match[1]!, + repoSource: "argv-api-path", + pullNumber: Number(match[2]!), + body, + }; +} + +function isFieldFlag(flag: string): boolean { + return flag === "-f" || flag === "-F" || flag === "--field" || flag === "--raw-field"; +} + +/** Pull the review text out of a `key=value` field expression. Only the `body` + * key carries the authored review; `event` and `commit_id` are metadata. */ +function parseBodyField(expression: string): ReviewSubmission["body"] { + const equals = expression.indexOf("="); + if (equals < 0) return null; + if (expression.slice(0, equals) !== "body") return null; + const value = expression.slice(equals + 1); + if (value.startsWith("@") && value.length > 1) return { kind: "file", path: value.slice(1) }; + return { kind: "inline", text: value }; +} + +// -- attestation scanning ---------------------------------------------------- + +// Mirrors ally-review-detection.ts. A fenced span is quoted content, not this +// review's own attestation: a review *of this guard* quotes `Reviewed head:` +// lines in prose, and flagging those would refuse honest reviews of exactly +// the code that does the refusing. Lines are blanked rather than removed so +// the line anchors below keep pointing at the same text. +const FENCE_DELIMITER_PATTERN = /^ {0,3}(`{3,}|~{3,})(.*)$/; +const FENCE_CLOSE_PATTERN = /^ {0,3}(`{3,}|~{3,})[ \t]*$/; + +function withoutFencedCodeBlocks(body: string): string { + if (!body.includes("```") && !body.includes("~~~")) return body; + const lines = body.split("\n"); + let open: { char: string; length: number } | null = null; + for (let i = 0; i < lines.length; i += 1) { + const line = lines[i]!; + if (open) { + const close = FENCE_CLOSE_PATTERN.exec(line); + const closes = close && close[1]![0] === open.char && close[1]!.length >= open.length; + lines[i] = ""; + if (closes) open = null; + continue; + } + const fence = FENCE_DELIMITER_PATTERN.exec(line); + if (fence && !(fence[1]![0] === "`" && fence[2]!.includes("`"))) { + open = { char: fence[1]![0]!, length: fence[1]!.length }; + lines[i] = ""; + } + } + return lines.join("\n"); +} + +// CommonMark starts an indented code block at four columns, and a tab always +// reaches column four. Both are excluded, matching the consumer. +const NOT_INDENTED_CODE = String.raw`(?! *\t)(?! {4})`; +const MARKDOWN_EMPHASIS_RUN = "[*_`]{0,3}"; + +// The loose half of the loose/strict split. This captures whatever token +// follows the label — 40 hex, 42 hex, a short SHA, a placeholder — because the +// producer's question is "did this body try to attest?", not "did it succeed?". +// The token run excludes emphasis characters so a backticked SHA yields the +// SHA rather than the delimiters. +const ATTESTATION_CANDIDATE_PATTERN = new RegExp( + `(?:^|\\n)${NOT_INDENTED_CODE} {0,3}${MARKDOWN_EMPHASIS_RUN}[ \\t]{0,3}reviewed head:[ \\t]*` + + `${MARKDOWN_EMPHASIS_RUN}([^\\s*_\`]*)`, + "gi", +); + +/** Exactly 40 lowercase hex, and nothing else. */ +const WELL_FORMED_SHA_PATTERN = /^[0-9a-f]{40}$/; + +/** + * Classify a review body's `Reviewed head:` attestation. + * + * Case-insensitive on the label and on the SHA, matching the consumer, which + * lowercases what it extracts. + */ +export function inspectReviewAttestation(body: string): ReviewAttestation { + const emitted = withoutFencedCodeBlocks(body); + const candidates = Array.from( + emitted.matchAll(ATTESTATION_CANDIDATE_PATTERN), + (match) => match[1] ?? "", + ); + + if (candidates.length === 0) return { kind: "absent" }; + if (candidates.length > 1) return { kind: "ambiguous", raw: candidates }; + + const raw = candidates[0]!; + const normalized = raw.toLowerCase(); + if (!WELL_FORMED_SHA_PATTERN.test(normalized)) return { kind: "malformed", raw }; + return { kind: "well-formed", sha: normalized }; +} + +// -- the guard --------------------------------------------------------------- + +/** + * Decide whether a `gh` invocation may post its review. + * + * Returns null to allow. A non-null refusal must abort the invocation: the + * point of the guard is that the review never reaches GitHub. + * + * Only fires when argv is a review submission *and* the body carries an + * attestation. A review with no `Reviewed head:` line is passed through — an + * approval carrying no attestation is a different defect, already reported by + * invariant I2d in scripts/check-ally-review-consistency.mjs, and refusing it + * here would block every non-Ally agent's ordinary `gh pr review` too. + */ +export async function evaluateReviewSubmission( + argv: readonly string[], + io: ReviewAttestationGuardIo, +): Promise { + const submission = parseReviewSubmission(argv); + if (!submission || !submission.body) return null; + + let text: string; + if (submission.body.kind === "inline") { + text = submission.body.text; + } else { + // `-` is stdin, which the runtime rejects before reaching here. + if (submission.body.path === "-") return null; + try { + text = io.readText(submission.body.path); + } catch { + return { + reason: "unreadable-body", + message: `cannot read review body ${submission.body.path}; refusing to post a review whose attestation cannot be checked`, + }; + } + } + + const attestation = inspectReviewAttestation(text); + if (attestation.kind === "absent") return null; + + if (attestation.kind === "malformed") { + return { + reason: "malformed-attestation", + message: attestationRefusalMessage( + `"Reviewed head: ${attestation.raw}" is not a 40-character hex commit SHA ` + + `(${attestation.raw.length} characters)`, + "A malformed attestation can never match any head, so the review would be " + + "permanently unable to satisfy the review gate.", + ), + }; + } + + if (attestation.kind === "ambiguous") { + return { + reason: "ambiguous-attestation", + message: attestationRefusalMessage( + `the body carries ${attestation.raw.length} "Reviewed head:" attestations (${attestation.raw.join(", ")})`, + "Consumers require exactly one attestation and treat several as none, so " + + "the review would attest no head at all.", + ), + }; + } + + const repo = submission.repo ?? (await io.resolveDefaultRepo()); + if (!repo) { + return { + reason: "unresolved-target", + message: attestationRefusalMessage( + "the target repository could not be determined from argv or from the checkout", + "The attestation cannot be checked against a repository that is not known.", + ), + }; + } + + const reachability = await io.resolveCommitReachability(repo, attestation.sha); + if (reachability === "reachable") return null; + + // Reachable-but-not-head is deliberately allowed: reviewing a prior head is + // legitimate and normal, and the gate's own staleness rules handle it. The + // question here is only whether this review belongs to this repository. + const detail = + reachability === "unreachable" + ? `commit ${attestation.sha} does not exist in ${repo}` + : `could not confirm commit ${attestation.sha} exists in ${repo}`; + return { + reason: "unreachable-attestation", + message: attestationRefusalMessage( + detail, + reachability === "unreachable" + ? "The review was computed against a different repository or a commit that " + + "has since been rewritten, so it does not describe this pull request." + : "Refusing rather than guessing: an unverifiable approval is not recoverable, " + + "a refused one is — re-run the review.", + ), + }; +} + +function attestationRefusalMessage(detail: string, why: string): string { + return `refusing to submit PR review: ${detail}. ${why}`; +} From 4628bce0e8165873af4ae53a8d9909754db789f3 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Thu, 10 Sep 2026 18:00:30 +0000 Subject: [PATCH 2/4] docs(runbooks): triage an egress-refused agent PR review (BLO-32844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard added in the previous commit refuses a review before it leaves the pod, with exit 65 and a stable bracketed reason. Without a runbook the obvious operator reactions are both wrong: disable the guard, or hand-post the refused body under a human seat — which re-creates the exact defect with a human identity attached. Documents what each reason means and, for `unreachable-attestation`, how to tell the three causes apart: cross-repository contamination (the BLO-32844 defect, worth capturing because AC #1's transport is still open), a commit force-pushed away (benign, and the refusal is still correct because the review describes code that is gone), and GitHub being unreachable (`could not confirm` rather than `does not exist`). Also carries the marker-resolves-in-its-own-repo check for auditing reviews that predate the guard, and notes that a prior-head marker which still resolves is normal staleness rather than contamination. Co-Authored-By: Claude --- runbooks/README.md | 7 + runbooks/agent-review-submission-refused.md | 135 ++++++++++++++++++++ 2 files changed, 142 insertions(+) create mode 100644 runbooks/agent-review-submission-refused.md diff --git a/runbooks/README.md b/runbooks/README.md index 41b3b538275b..5235a73ac8e0 100644 --- a/runbooks/README.md +++ b/runbooks/README.md @@ -18,6 +18,13 @@ platform cannot resolve automatically. Each runbook should be: which nothing re-drives: decide re-review vs accept without double-posting a review. Trigger: alert `PaperclipPrReviewWakeTerminalFailed`, or `max by (error_code, scope) (paperclip_agent_wakeup_terminal_failed_unresolved{scope="pr_review"}) > 0`. +- [`agent-review-submission-refused.md`](agent-review-submission-refused.md) — + an agent's PR review was refused by the `gh` egress guard and never posted, + because its `Reviewed head:` attestation is malformed or names a commit that + does not exist in the target repository. Covers how to tell cross-repository + contamination from a benign force-push, and why there is deliberately no + bypass. Trigger: agent stderr or run log carrying + `paperclip-github-egress: refusing to submit PR review:` with exit 65. - [`clear-polluted-ssh-workspace.md`](clear-polluted-ssh-workspace.md) — recover a stranded SSH-driven run whose workspace import is failing on a sibling task's leftover scratch state. Trigger: blocked issue auto-comment diff --git a/runbooks/agent-review-submission-refused.md b/runbooks/agent-review-submission-refused.md new file mode 100644 index 000000000000..9ce67869749b --- /dev/null +++ b/runbooks/agent-review-submission-refused.md @@ -0,0 +1,135 @@ +# Agent review submission refused (`paperclip-github-egress`) + +An agent tried to post a GitHub pull-request review and the `gh` egress wrapper +refused it. The review **was not posted** — the GitHub CLI was never started — +and the agent's command exited **65**. + +**Trigger.** A run log or agent stderr carrying: + +``` +paperclip-github-egress: refusing to submit PR review: [] +``` + +This is a deliberate fail-closed refusal, not a crash. It is emitted by +`packages/adapter-utils/src/github-review-attestation.ts`, called from the +Helm-seeded `gh` wrapper (`deploy/helm/paperclip/templates/statefulset.yaml`) +before the real CLI runs. + +## Why the guard exists + +A review body computed against one pull request can be submitted to a +different one. GitHub stamps a review's `commit_id` from the **target** PR at +submission time, so `commit_id` reads the target's real head and every +consumer checking `state == "APPROVED" && commit_id == head` is satisfied. The +body's `Reviewed head:` marker is the only field that travels with the review +text, so it is the only one able to disagree. + +That happened once, measured: `Blockcast/mediamtx#33` received an **APPROVED** +review (`5146990033`) whose body reviewed `pim-multicast-gateway#2864`. It +fails **open** — in a repo where an Ally approval gates merge it admits +entirely unreviewed code under a green signal. + +## Triage by reason + +The bracketed `reason` is stable and tells you what to do. + +### `unreachable-attestation` + +The body attests a SHA that does not exist in the target repository, or its +existence could not be confirmed. Two very different causes — check which: + +```bash +# Does the attested SHA exist anywhere else in the org? +gh api "repos///commits/" --jq .sha # 422/404 = absent here +gh search commits --hash --owner # slow to index; not authoritative +``` + +- **Cross-repository contamination** — the SHA is the head of a PR in a + *different* repo. This is the BLO-32844 defect. The refusal is correct and + valuable: capture the target repo, PR number, attested SHA and the run id, + then attach them to BLO-32844 (AC #1, the transport, is still open). Have the + agent re-review the intended PR. +- **The commit was rewritten** — a force-push removed the attested head + between composing and submitting. Benign, and the refusal is still correct: + the review describes code that is no longer there. Re-review at the live + head. +- **GitHub was unreachable** — the message reads `could not confirm` rather + than `does not exist`. Transient; re-run the agent. If it persists, check + GitHub status and the pod's token before anything else. + +### `malformed-attestation` + +The `Reviewed head:` token is not 40 hex characters. Refused locally with no +API call. + +This is corrupt marker emission by the reviewer, not staleness. Measured on +`Blockcast/review-gate-action#7` (reviews `5013561307`, `5013569224`): a +42-character marker, the real SHA with two characters spliced in, both bodies +byte-identical 1m45s apart. Note the failure direction is the **opposite** of +the case above — a malformed marker can never equal any head, so the +attestation is permanently unsatisfiable and the PR could never pass the gate. +Refusing at emission is what keeps that out of the repo. Re-run the agent; +report a recurring pattern against BLO-32844. + +### `ambiguous-attestation` + +Several `Reviewed head:` lines outside fenced blocks. Consumers require exactly +one and treat several as none, so the review would attest nothing. Usually the +agent quoted a prior review without fencing it. Re-run; fenced quotes are +ignored by design. + +### `unresolved-target` + +Neither the argv nor the checkout named a target repository, so the attestation +could not be checked against anything. Have the agent pass `--repo /` +explicitly. + +### `unreadable-body` + +The `--body-file` path could not be read. Almost always a workspace problem +(wrong cwd, cleaned temp dir), not a review problem. + +## Do not + +- **Do not disable or bypass the guard to get a review posted.** There is no + env escape hatch on purpose. The asymmetry is the whole point: a refused + review is recoverable by re-running, whereas an admitted false approval is + merge-visible immediately and only a human dismissal removes it. +- **Do not hand-post the refused review body under a human seat.** You would be + re-creating the exact defect — an attestation that does not describe the PR it + is attached to — with a human identity attached to it. +- **Do not read a refusal as the reviewer being broken.** Every refusal so far + has been the guard working. Diagnose the reason first. + +## Verifying an already-posted review + +To check reviews that predate the guard, the marker must name a commit that +exists in *that* repo: + +```bash +R=/; N= +gh api "repos/$R/pulls/$N/reviews" --paginate \ + --jq '.[]|select(.user.login=="allyblockcast[bot]")|.body' \ + | grep -oE 'Reviewed head: [0-9a-f]{40}' | awk '{print $3}' | while read -r m; do + gh api "repos/$R/commits/$m" --jq .sha >/dev/null 2>&1 \ + && echo "$m exists in $R" || echo "$m ABSENT from $R <-- contamination" + done +``` + +A prior-head marker that still resolves is normal staleness, not contamination. +The distinguishing signal is a SHA absent from the repository entirely. + +## Sources + +- [BLO-32844](https://paperclip.blockcast.net/BLO/issues/BLO-32844) — the + cross-repository APPROVED review, this guard, and the still-open question of + how the body reached the wrong PR. +- [BLO-32512](https://paperclip.blockcast.net/BLO/issues/BLO-32512) — the + attestation grammar is intentionally duplicated between `adapter-utils` + (ships to agent pods, must not import `server/`) and + `server/src/services/ally-review-detection.ts`. Detection there is + deliberately *looser* and validation *stricter* than the consumer's, which is + what catches a malformed marker the consumer reads as simply absent. +- [BLO-31730](https://paperclip.blockcast.net/BLO/issues/BLO-31730) — earlier + attestation-parsing defect, for contrast: a backticked marker made a real + review invisible. From c121fdec65d7151447c1d65e87fc65fabc5b782a Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Thu, 10 Sep 2026 18:14:06 +0000 Subject: [PATCH 3/4] fix(adapter-utils): validate the whole attestation line, not just its token (BLO-32844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Ally's Important finding on #1744 at head 849ee294. The finding is correct and the hole was in the same failure family the guard was written to close, which is worth stating plainly. The loose candidate regex captured the token after `Reviewed head:` but did not require the line to end there. So `Reviewed head: <40-hex> and some prose` yielded a perfectly good token, was classified `well-formed`, resolved fine against the target repo, and was ALLOWED — while the consumer's REVIEWED_HEAD_ATTESTATION_PATTERN requires optional delimiters and whitespace followed by end-of-line, so it matches that line nowhere. The review would attest no head and the gate would be permanently unsatisfiable: exactly the fail-closed livelock this guard exists to refuse, waved through by the guard. The loose/strict split was only half implemented. Detection was loose, but validation checked the token rather than the line. Now the pattern captures the rest of the line too and the trailer must match what the consumer tolerates — an unbalanced emphasis run and trailing whitespace, nothing else. Malformed now carries `detail` so the refusal says whether the token is not a SHA or the line does not end after it. Both are refused locally, with no API call. Verified by cross-checking against the real extractAllyReviewedHeadSha rather than by re-reading its regex: the two now agree on all 8 forms exercised (plain, trailing whitespace, backticked, bold SHA, underscored SHA, upper-case, 3-space indent, bolded label). That cross-check also corrected a test I had written from assumption — a bolded *label* is rejected by the consumer as well, because the emphasis run cannot span `**` plus the space before the SHA, so refusing it is agreement rather than added strictness. Pinned with a test that will fail if the consumer's grammar is ever widened to accept it, so the two grammars are re-aligned deliberately instead of drifting (cf. BLO-32512). Re-ran the live replay after the change: the real contaminated body at mediamtx#33 still refuses, the same body against pim-multicast-gateway and the genuine dbe4e10e review still pass, and trailing prose on a genuinely reachable head is now refused where it previously would have been posted. Co-Authored-By: Claude --- .../src/github-review-attestation.test.ts | 78 ++++++++++++++++++- .../src/github-review-attestation.ts | 55 +++++++++---- 2 files changed, 118 insertions(+), 15 deletions(-) diff --git a/packages/adapter-utils/src/github-review-attestation.test.ts b/packages/adapter-utils/src/github-review-attestation.test.ts index df8408f6d213..e52a77133cee 100644 --- a/packages/adapter-utils/src/github-review-attestation.test.ts +++ b/packages/adapter-utils/src/github-review-attestation.test.ts @@ -194,10 +194,69 @@ describe("inspectReviewAttestation", () => { // nothing and reads as "no attestation" rather than as a broken one. it("classifies the real 42-character marker as malformed, not absent", () => { const attestation = inspectReviewAttestation(bodyAttesting(MALFORMED_MARKER)); - expect(attestation).toEqual({ kind: "malformed", raw: MALFORMED_MARKER }); + expect(attestation).toEqual({ + kind: "malformed", + raw: MALFORMED_MARKER, + detail: "not-a-sha", + }); expect(MALFORMED_MARKER).toHaveLength(42); }); + // A good token is not a good attestation. The consumer requires the line to + // end after optional delimiters and whitespace, so trailing prose makes the + // line match its pattern nowhere — the review attests no head and the gate + // becomes permanently unsatisfiable, which is the fail-closed shape this + // guard exists to refuse. Validating only the token would allow it. + it("refuses a valid SHA followed by trailing prose", () => { + const attestation = inspectReviewAttestation( + `## Ally — Consolidated PR Review\nReviewed head: ${PIM_2864_HEAD} (rebased onto master)`, + ); + expect(attestation).toEqual({ + kind: "malformed", + raw: `${PIM_2864_HEAD} (rebased onto master)`, + detail: "trailing-content", + }); + }); + + it("refuses an unmatched closing delimiter followed by trailing prose", () => { + expect( + inspectReviewAttestation(`Reviewed head: \`${PIM_2864_HEAD}\` see below`), + ).toMatchObject({ kind: "malformed", detail: "trailing-content" }); + }); + + // The trailers the consumer does tolerate must still pass, or the guard would + // refuse the emphasis styles Ally actually emits. This list was derived by + // running each form through the real extractAllyReviewedHeadSha rather than + // read off its regex — the two agree on every case here. + it("accepts the forms the consumer tolerates", () => { + for (const line of [ + `Reviewed head: ${PIM_2864_HEAD}`, + `Reviewed head: ${PIM_2864_HEAD} `, + `Reviewed head: \`${PIM_2864_HEAD}\``, + `Reviewed head: **${PIM_2864_HEAD}**`, + `Reviewed head: _${PIM_2864_HEAD}_`, + ` Reviewed head: ${PIM_2864_HEAD}`, + ]) { + expect(inspectReviewAttestation(line), line).toEqual({ + kind: "well-formed", + sha: PIM_2864_HEAD, + }); + } + }); + + // A bolded *label* (`**Reviewed head:** \`sha\``) is rejected by the consumer + // too — the emphasis run cannot span `**` plus the space before the SHA. So + // refusing it here is agreement, not added strictness: posting it would + // produce an attestation the gate cannot read. Pinned so that if the + // consumer's grammar is ever widened to accept it (cf. BLO-31730, which + // widened for backticks), this test fails and the two are re-aligned + // deliberately rather than drifting apart. + it("refuses a bolded label, matching the consumer's own rejection", () => { + expect( + inspectReviewAttestation(`**Reviewed head:** \`${PIM_2864_HEAD}\``), + ).toMatchObject({ kind: "malformed" }); + }); + it("treats several attestations as ambiguous", () => { const body = `Reviewed head: ${PIM_2864_HEAD}\nReviewed head: ${MEDIAMTX_HEAD}`; expect(inspectReviewAttestation(body)).toEqual({ @@ -294,6 +353,23 @@ describe("evaluateReviewSubmission", () => { ); }); + // Same local-refusal property as the 42-char case: the token resolves fine, + // so a reachability check would pass and the review would be posted with an + // attestation the gate can never read. + it("refuses trailing content after a resolvable SHA without consulting the network", async () => { + const resolveCommitReachability = vi.fn(async () => "reachable" as CommitReachability); + const io = makeIo({ + readText: () => `Reviewed head: ${MEDIAMTX_HEAD} (after rebase)`, + resolveCommitReachability, + }); + + const refusal = await evaluateReviewSubmission(crossRepoArgv, io); + + expect(refusal?.reason).toBe("malformed-attestation"); + expect(refusal?.message).toContain("does not end after the SHA"); + expect(resolveCommitReachability).not.toHaveBeenCalled(); + }); + // Reviewing a prior head is legitimate and common; the gate's own staleness // rules handle it. Refusing it here would break normal review traffic, so // reachability — not head equality — is the test. diff --git a/packages/adapter-utils/src/github-review-attestation.ts b/packages/adapter-utils/src/github-review-attestation.ts index 4ed1ed31af0f..2e6f5149f792 100644 --- a/packages/adapter-utils/src/github-review-attestation.ts +++ b/packages/adapter-utils/src/github-review-attestation.ts @@ -73,10 +73,12 @@ export interface ReviewSubmission { export type ReviewAttestation = /** No line intends to attest. Nothing for this guard to check. */ | { kind: "absent" } - /** Exactly one attesting line, token is exactly 40 lowercase hex. */ + /** Exactly one attesting line, token is exactly 40 lowercase hex, and the + * line ends after it. */ | { kind: "well-formed"; sha: string } - /** Exactly one attesting line, token is not a SHA. The fail-closed defect. */ - | { kind: "malformed"; raw: string } + /** Exactly one attesting line that the consumer will not accept. The + * fail-closed defect: `detail` says which way it is broken. */ + | { kind: "malformed"; raw: string; detail: "not-a-sha" | "trailing-content" } /** Several attesting lines. The consumer requires exactly one and returns * null otherwise, so this is the malformed case by another route. */ | { kind: "ambiguous"; raw: string[] }; @@ -342,15 +344,28 @@ const MARKDOWN_EMPHASIS_RUN = "[*_`]{0,3}"; // producer's question is "did this body try to attest?", not "did it succeed?". // The token run excludes emphasis characters so a backticked SHA yields the // SHA rather than the delimiters. +// +// Group 2 deliberately captures the REST OF THE LINE. Validating only the +// token is not enough: the consumer requires the line to end after optional +// delimiters and whitespace, so `Reviewed head: <40-hex> and some prose` has a +// perfectly good token and still matches the consumer's pattern nowhere. That +// shape would attest no head and livelock the gate, which is the exact defect +// this guard exists to refuse — so the whole line has to be checked, not just +// the token it contains. const ATTESTATION_CANDIDATE_PATTERN = new RegExp( `(?:^|\\n)${NOT_INDENTED_CODE} {0,3}${MARKDOWN_EMPHASIS_RUN}[ \\t]{0,3}reviewed head:[ \\t]*` + - `${MARKDOWN_EMPHASIS_RUN}([^\\s*_\`]*)`, + `${MARKDOWN_EMPHASIS_RUN}([^\\s*_\`]*)([^\\n]*)`, "gi", ); /** Exactly 40 lowercase hex, and nothing else. */ const WELL_FORMED_SHA_PATTERN = /^[0-9a-f]{40}$/; +// What the consumer tolerates after the SHA, and nothing more: an unbalanced +// emphasis run and trailing whitespace, then end of line. Mirrors the tail of +// REVIEWED_HEAD_ATTESTATION_PATTERN in ally-review-detection.ts. +const ACCEPTED_ATTESTATION_TRAILER_PATTERN = /^[*_`]{0,3}[ \t]*[*_`]{0,3}[ \t]*$/; + /** * Classify a review body's `Reviewed head:` attestation. * @@ -359,17 +374,24 @@ const WELL_FORMED_SHA_PATTERN = /^[0-9a-f]{40}$/; */ export function inspectReviewAttestation(body: string): ReviewAttestation { const emitted = withoutFencedCodeBlocks(body); - const candidates = Array.from( - emitted.matchAll(ATTESTATION_CANDIDATE_PATTERN), - (match) => match[1] ?? "", - ); + const candidates = Array.from(emitted.matchAll(ATTESTATION_CANDIDATE_PATTERN), (match) => ({ + token: match[1] ?? "", + trailer: match[2] ?? "", + })); if (candidates.length === 0) return { kind: "absent" }; - if (candidates.length > 1) return { kind: "ambiguous", raw: candidates }; + if (candidates.length > 1) { + return { kind: "ambiguous", raw: candidates.map((candidate) => candidate.token) }; + } - const raw = candidates[0]!; - const normalized = raw.toLowerCase(); - if (!WELL_FORMED_SHA_PATTERN.test(normalized)) return { kind: "malformed", raw }; + const { token, trailer } = candidates[0]!; + const normalized = token.toLowerCase(); + if (!WELL_FORMED_SHA_PATTERN.test(normalized)) { + return { kind: "malformed", raw: token, detail: "not-a-sha" }; + } + if (!ACCEPTED_ATTESTATION_TRAILER_PATTERN.test(trailer)) { + return { kind: "malformed", raw: `${token}${trailer}`, detail: "trailing-content" }; + } return { kind: "well-formed", sha: normalized }; } @@ -414,11 +436,16 @@ export async function evaluateReviewSubmission( if (attestation.kind === "absent") return null; if (attestation.kind === "malformed") { + const detail = + attestation.detail === "not-a-sha" + ? `"Reviewed head: ${attestation.raw}" is not a 40-character hex commit SHA ` + + `(${attestation.raw.length} characters)` + : `the attestation line does not end after the SHA: ` + + `"Reviewed head: ${attestation.raw}"`; return { reason: "malformed-attestation", message: attestationRefusalMessage( - `"Reviewed head: ${attestation.raw}" is not a 40-character hex commit SHA ` + - `(${attestation.raw.length} characters)`, + detail, "A malformed attestation can never match any head, so the review would be " + "permanently unable to satisfy the review gate.", ), From 5e72e1a747d3ce4934a058ec247750e918a0e3a7 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Thu, 10 Sep 2026 18:35:44 +0000 Subject: [PATCH 4/4] fix(adapter-utils): decode a --input JSON request payload before scanning it (BLO-32844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Ally's second Important finding on #1744, at head c121fdec. Like the first, it was the guard's own defect rather than a gap around it — and this one re-opened the original fail-open hole in a second shape. `gh api repos/{o}/{r}/pulls/{n}/reviews --method POST --input req.json` was recognised as a review submission, but the file was handed to inspectReviewAttestation as if it were Markdown. It is not: it is the whole JSON request payload, and its newlines are `\n` escapes rather than real ones. Every pattern here is line-anchored, so nothing matched, the attestation read `absent`, and the reachability check was skipped entirely. Reproduced before fixing: an APPROVE payload whose `body` attests 81d63ac9 (pim#2864's head) targeted at mediamtx#33 was ALLOWED with zero reachability calls. That is the exact fail-open this guard exists to close, reached through the JSON door. `--input` now yields a distinct `json-request-file` body source, decoded before scanning. A payload with no `body` member is allowed rather than refused — `{"event":"APPROVE"}` is a valid review with no comment, so it attests nothing and there is nothing to check. A file that is not a JSON object, or whose `body` is present but not a string, is refused as `unparsable-request-body`: the request would fail at GitHub anyway, so refusing costs nothing, while guessing could pass an unverified attestation in an unanticipated shape. Re-verified against the live API after the change: the cross-repo JSON payload now refuses, the correct-attestation and no-body payloads are allowed, and the Markdown `--body-file` path still refuses cross-repo. `gh api --input` against a non-review endpoint is untouched, with a test pinning that. Also corrected a defect in my own test helper found by these tests: overriding `resolveCommitReachability` replaced the call recorder, so two assertions that the resolver had been consulted were vacuously reading an empty array. The verdict is now set via a `reachability` option that keeps the recorder, which is what lets those tests assert the resolver actually ran — the property whose absence was the bug being fixed here. Runbook updated with the new refusal reason. Co-Authored-By: Claude --- .../src/github-review-attestation.test.ts | 115 +++++++++++++++++- .../src/github-review-attestation.ts | 78 ++++++++++-- runbooks/agent-review-submission-refused.md | 12 ++ 3 files changed, 195 insertions(+), 10 deletions(-) diff --git a/packages/adapter-utils/src/github-review-attestation.test.ts b/packages/adapter-utils/src/github-review-attestation.test.ts index e52a77133cee..0d173e85af57 100644 --- a/packages/adapter-utils/src/github-review-attestation.test.ts +++ b/packages/adapter-utils/src/github-review-attestation.test.ts @@ -40,7 +40,7 @@ function bodyAttesting(sha: string): string { } function makeIo( - overrides: Partial = {}, + overrides: Partial & { reachability?: CommitReachability } = {}, ): ReviewAttestationGuardIo & { reachabilityCalls: Array<{ repo: string; sha: string }>; } { @@ -48,11 +48,14 @@ function makeIo( return { reachabilityCalls, readText: overrides.readText ?? (() => bodyAttesting(MEDIAMTX_HEAD)), + // `reachability` sets the verdict while keeping the recorder, so a test can + // assert both the outcome and that the resolver was actually consulted. + // Pass `resolveCommitReachability` only to assert it was NOT called. resolveCommitReachability: overrides.resolveCommitReachability ?? (async (repo, sha) => { reachabilityCalls.push({ repo, sha }); - return "reachable" as CommitReachability; + return overrides.reachability ?? "reachable"; }), resolveDefaultRepo: overrides.resolveDefaultRepo ?? (async () => null), }; @@ -318,7 +321,7 @@ describe("evaluateReviewSubmission", () => { }); it("asks about the attested SHA in the argv-named repository", async () => { - const io = makeIo({ readText: () => bodyAttesting(PIM_2864_HEAD) }); + const io = makeIo({ readText: () => bodyAttesting(PIM_2864_HEAD), reachability: "reachable" }); await evaluateReviewSubmission(crossRepoArgv, io); @@ -468,6 +471,112 @@ describe("evaluateReviewSubmission", () => { }); }); +// `gh api --input` points at a whole JSON request payload, not Markdown. Its +// newlines are `\n` escapes, so scanning the file as Markdown finds no +// line-anchored attestation, reports `absent`, and skips the reachability +// check — which let the original fail-open defect through in a second shape. +describe("evaluateReviewSubmission with a --input JSON request payload", () => { + const apiArgv = [ + "api", + "repos/Blockcast/mediamtx/pulls/33/reviews", + "--method", + "POST", + "--input", + "/tmp/review-request.json", + ]; + + function requestPayload(body?: unknown): string { + const payload: Record = { + event: "APPROVE", + commit_id: MEDIAMTX_HEAD, + }; + if (body !== undefined) payload.body = body; + return JSON.stringify(payload); + } + + it("parses --input as a JSON request payload rather than Markdown", () => { + expect(parseReviewSubmission(apiArgv)).toEqual({ + repo: "Blockcast/mediamtx", + repoSource: "argv-api-path", + pullNumber: 33, + body: { kind: "json-request-file", path: "/tmp/review-request.json" }, + }); + }); + + it("decodes the JSON body and refuses a cross-repository attestation", async () => { + const io = makeIo({ + readText: () => requestPayload(bodyAttesting(PIM_2864_HEAD)), + reachability: "unreachable", + }); + + const refusal = await evaluateReviewSubmission(apiArgv, io); + + expect(refusal?.reason).toBe("unreachable-attestation"); + // The resolver must actually have been consulted — the bug was that it + // never was, so the submission sailed through. + expect(io.reachabilityCalls).toEqual([ + { repo: "Blockcast/mediamtx", sha: PIM_2864_HEAD }, + ]); + }); + + it("allows a JSON body whose attestation resolves in the target repo", async () => { + const io = makeIo({ + readText: () => requestPayload(bodyAttesting(MEDIAMTX_HEAD)), + reachability: "reachable", + }); + + expect(await evaluateReviewSubmission(apiArgv, io)).toBeNull(); + expect(io.reachabilityCalls).toEqual([ + { repo: "Blockcast/mediamtx", sha: MEDIAMTX_HEAD }, + ]); + }); + + it("refuses a malformed attestation carried inside the JSON body", async () => { + const resolveCommitReachability = vi.fn(async () => "reachable" as CommitReachability); + const io = makeIo({ + readText: () => requestPayload(bodyAttesting(MALFORMED_MARKER)), + resolveCommitReachability, + }); + + expect((await evaluateReviewSubmission(apiArgv, io))?.reason).toBe("malformed-attestation"); + expect(resolveCommitReachability).not.toHaveBeenCalled(); + }); + + // `{"event":"APPROVE"}` is a valid review with no comment. It attests + // nothing, so there is nothing to check — not an error. + it("allows a request payload with no body member", async () => { + const resolveCommitReachability = vi.fn(async () => "reachable" as CommitReachability); + const io = makeIo({ readText: () => requestPayload(), resolveCommitReachability }); + + expect(await evaluateReviewSubmission(apiArgv, io)).toBeNull(); + expect(resolveCommitReachability).not.toHaveBeenCalled(); + }); + + it("refuses a payload that is not a JSON object with a string body", async () => { + for (const raw of ["not json at all", "[1,2,3]", '{"body":42}', '"just a string"']) { + const io = makeIo({ readText: () => raw }); + expect((await evaluateReviewSubmission(apiArgv, io))?.reason, raw).toBe( + "unparsable-request-body", + ); + } + }); + + // A non-review endpoint must stay untouched, or every `gh api --input` call + // in the fleet would start being parsed as a review payload. + it("ignores --input against a non-review endpoint", async () => { + const resolveCommitReachability = vi.fn(async () => "reachable" as CommitReachability); + const io = makeIo({ resolveCommitReachability }); + + expect( + await evaluateReviewSubmission( + ["api", "repos/Blockcast/mediamtx/issues/33/comments", "--input", "/tmp/x.json"], + io, + ), + ).toBeNull(); + expect(resolveCommitReachability).not.toHaveBeenCalled(); + }); +}); + describe("runGitHubCliEgressRuntime review guard", () => { const temporaryDirectories: string[] = []; diff --git a/packages/adapter-utils/src/github-review-attestation.ts b/packages/adapter-utils/src/github-review-attestation.ts index 2e6f5149f792..141c653bed91 100644 --- a/packages/adapter-utils/src/github-review-attestation.ts +++ b/packages/adapter-utils/src/github-review-attestation.ts @@ -59,6 +59,22 @@ /** How the target repository was determined, for diagnostics in the refusal. */ export type ReviewTargetSource = "argv-flag" | "argv-url" | "argv-api-path" | "resolved-default"; +/** + * Where the authored review text came from. + * + * The distinction between `file` and `json-request-file` is load-bearing. + * `gh pr review --body-file x.md` and `gh api ... -F body=@x.md` both point at + * raw Markdown, but `gh api ... --input x.json` points at a whole JSON request + * payload whose `body` member holds the Markdown — with newlines encoded as + * `\n` escapes. Scanning that file as Markdown finds no line-anchored + * attestation at all, reports `absent`, and skips the reachability check, so + * the JSON form silently bypassed the guard entirely. + */ +export type ReviewBodySource = + | { kind: "inline"; text: string } + | { kind: "file"; path: string } + | { kind: "json-request-file"; path: string }; + export interface ReviewSubmission { /** "owner/name", or null when argv does not name it (`gh pr review` with no * --repo relies on the checkout's remote, which argv cannot tell us). */ @@ -67,7 +83,7 @@ export interface ReviewSubmission { /** The pull request number, when argv carries it. */ pullNumber: number | null; /** Inline body text, or a path to read it from. */ - body: { kind: "inline"; text: string } | { kind: "file"; path: string } | null; + body: ReviewBodySource | null; } export type ReviewAttestation = @@ -104,7 +120,8 @@ export interface ReviewAttestationRefusal { | "ambiguous-attestation" | "unreachable-attestation" | "unresolved-target" - | "unreadable-body"; + | "unreadable-body" + | "unparsable-request-body"; message: string; } @@ -226,7 +243,7 @@ function parsePrReview(rest: readonly string[]): ReviewSubmission { function parseApiReview(rest: readonly string[]): ReviewSubmission | null { let path: string | null = null; let method: string | null = null; - let body: ReviewSubmission["body"] = null; + let body: ReviewBodySource | null = null; for (let i = 0; i < rest.length; i += 1) { const arg = rest[i]!; @@ -234,7 +251,7 @@ function parseApiReview(rest: readonly string[]): ReviewSubmission | null { const fused = splitFused(arg); if (fused) { if (fused.flag === "--method" || fused.flag === "-X") method = fused.value; - else if (fused.flag === "--input") body = { kind: "file", path: fused.value }; + else if (fused.flag === "--input") body = { kind: "json-request-file", path: fused.value }; else if (isFieldFlag(fused.flag)) { const parsed = parseBodyField(fused.value); if (parsed) body = parsed; @@ -253,7 +270,7 @@ function parseApiReview(rest: readonly string[]): ReviewSubmission | null { if (arg === "--input") { const value = rest[i + 1]; if (value !== undefined) { - body = { kind: "file", path: value }; + body = { kind: "json-request-file", path: value }; i += 1; } continue; @@ -293,7 +310,7 @@ function isFieldFlag(flag: string): boolean { /** Pull the review text out of a `key=value` field expression. Only the `body` * key carries the authored review; `event` and `commit_id` are metadata. */ -function parseBodyField(expression: string): ReviewSubmission["body"] { +function parseBodyField(expression: string): ReviewBodySource | null { const equals = expression.indexOf("="); if (equals < 0) return null; if (expression.slice(0, equals) !== "body") return null; @@ -397,6 +414,34 @@ export function inspectReviewAttestation(body: string): ReviewAttestation { // -- the guard --------------------------------------------------------------- +/** + * Pull the authored Markdown out of a `gh api --input` request payload. + * + * `no-body` is a legitimate shape, not an error: `{"event":"APPROVE"}` is a + * valid review submission that carries no comment and therefore attests + * nothing. `unparsable` covers a file that is not a JSON object, or whose + * `body` member is present but not a string — the request would be rejected by + * GitHub anyway, and refusing costs nothing while guessing could let an + * unverified attestation through in a shape nobody anticipated. + */ +function decodeJsonRequestBody( + raw: string, +): { kind: "text"; text: string } | { kind: "no-body" } | { kind: "unparsable" } { + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return { kind: "unparsable" }; + } + if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { + return { kind: "unparsable" }; + } + const body = (parsed as Record).body; + if (typeof body === "string") return { kind: "text", text: body }; + if (body === undefined) return { kind: "no-body" }; + return { kind: "unparsable" }; +} + /** * Decide whether a `gh` invocation may post its review. * @@ -422,14 +467,33 @@ export async function evaluateReviewSubmission( } else { // `-` is stdin, which the runtime rejects before reaching here. if (submission.body.path === "-") return null; + let raw: string; try { - text = io.readText(submission.body.path); + raw = io.readText(submission.body.path); } catch { return { reason: "unreadable-body", message: `cannot read review body ${submission.body.path}; refusing to post a review whose attestation cannot be checked`, }; } + + if (submission.body.kind === "json-request-file") { + const decoded = decodeJsonRequestBody(raw); + if (decoded.kind === "unparsable") { + return { + reason: "unparsable-request-body", + message: attestationRefusalMessage( + `${submission.body.path} is not a JSON object with a string "body"`, + "The review text cannot be located, so its attestation cannot be checked.", + ), + }; + } + // A review with no comment body attests nothing; nothing to check. + if (decoded.kind === "no-body") return null; + text = decoded.text; + } else { + text = raw; + } } const attestation = inspectReviewAttestation(text); diff --git a/runbooks/agent-review-submission-refused.md b/runbooks/agent-review-submission-refused.md index 9ce67869749b..6c10ea01ce49 100644 --- a/runbooks/agent-review-submission-refused.md +++ b/runbooks/agent-review-submission-refused.md @@ -89,6 +89,18 @@ explicitly. The `--body-file` path could not be read. Almost always a workspace problem (wrong cwd, cleaned temp dir), not a review problem. +### `unparsable-request-body` + +Only reachable via `gh api .../pulls/{n}/reviews --input `, where the file +is the whole JSON request payload rather than raw Markdown. It means the file is +not a JSON object, or its `body` member exists but is not a string, so the +review text cannot be located and its attestation cannot be checked. + +A payload with **no** `body` member at all is fine and is allowed — +`{"event":"APPROVE"}` is a valid review that carries no comment and therefore +attests nothing. Have the agent emit a well-formed payload, or use +`--body-file` with Markdown. + ## Do not - **Do not disable or bypass the guard to get a review posted.** There is no