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..0d173e85af57 --- /dev/null +++ b/packages/adapter-utils/src/github-review-attestation.test.ts @@ -0,0 +1,665 @@ +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 & { reachability?: CommitReachability } = {}, +): ReviewAttestationGuardIo & { + reachabilityCalls: Array<{ repo: string; sha: string }>; +} { + const reachabilityCalls: Array<{ repo: string; sha: string }> = []; + 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 overrides.reachability ?? "reachable"; + }), + 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, + 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({ + 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), reachability: "reachable" }); + + 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", + ); + }); + + // 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. + 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"); + }); +}); + +// `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[] = []; + + 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..141c653bed91 --- /dev/null +++ b/packages/adapter-utils/src/github-review-attestation.ts @@ -0,0 +1,566 @@ +// 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"; + +/** + * 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). */ + 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: ReviewBodySource | 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, and the + * line ends after it. */ + | { kind: "well-formed"; sha: 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[] }; + +/** 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" + | "unparsable-request-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: ReviewBodySource | null = 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: "json-request-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: "json-request-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): ReviewBodySource | null { + 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. +// +// 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*_\`]*)([^\\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. + * + * 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) => ({ + token: match[1] ?? "", + trailer: match[2] ?? "", + })); + + if (candidates.length === 0) return { kind: "absent" }; + if (candidates.length > 1) { + return { kind: "ambiguous", raw: candidates.map((candidate) => candidate.token) }; + } + + 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 }; +} + +// -- 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. + * + * 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; + let raw: string; + try { + 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); + 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( + detail, + "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}`; +} 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..6c10ea01ce49 --- /dev/null +++ b/runbooks/agent-review-submission-refused.md @@ -0,0 +1,147 @@ +# 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. + +### `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 + 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.