feat(adapter-utils): refuse a PR review whose attestation is unreachable in the target repo (BLO-32844) - #1744
Conversation
…ble in the target repo (BLO-32844)
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 <noreply@anthropic.com>
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 849ee29
Critical Issues (0)
Important Issues (1)
- [native-codex]
packages/adapter-utils/src/github-review-attestation.ts:958— the loose candidate regex does not require the attestation line to end after the captured token. A body such asReviewed head: <40-hex> extra(or a valid SHA followed by an unmatched closing delimiter and extra text) is classified aswell-formedand the commit is checked, but the consumer's strictREVIEWED_HEAD_ATTESTATION_PATTERNwill not recognize that line because it requires optional delimiters/whitespace followed by end-of-line. The guard can therefore allow a submission that still cannot satisfy the review gate.- Require the candidate line to contain only the optional Markdown delimiters and whitespace after the token, or capture any trailing non-whitespace/suffix as part of the malformed candidate so this shape is refused locally. Add regression tests for trailing prose and unmatched delimiter plus trailing prose.
Suggestions (0)
Strengths
- The guard is placed before the scrubbed subprocess is spawned, so rejected review submissions cannot reach GitHub.
- Repository reachability is checked with the direct commit endpoint and indeterminate results fail closed.
- The tests cover the cross-repository contamination case, malformed token lengths, ambiguous attestations, stdin handling, and CLI pass-through behavior.
Recommended Action
- Fix the Important issue before merge.
- Keep the existing fail-closed behavior for malformed or indeterminate attestations.
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4628bce
Prior Findings Dispositioned (1)
- prior:849ee29 important 1 — still-present —
packages/adapter-utils/src/github-review-attestation.ts:346-348— The candidate regex stops at whitespace after the captured token, soReviewed head: <40-hex> extrais classified aswell-formedand reachability is checked, but the consumer's strict attestation grammar rejects the line because it requires only delimiters/whitespace through end-of-line. The same mismatch remains for an unmatched delimiter followed by trailing prose.
Critical Issues (0)
Important Issues (1)
- [native-codex; prior:849ee29 important 1]
packages/adapter-utils/src/github-review-attestation.ts:346-348— The loose candidate regex does not require the attestation line to end after the captured token. A body such asReviewed head: <40-hex> extrais allowed through this producer guard, but the review consumer will not recognize it, so the submission can still be posted without a usable attestation.- Require the candidate line to contain only the optional Markdown delimiters and whitespace after the token, or capture trailing suffixes as malformed candidates. Add regression tests for trailing prose and unmatched delimiter plus trailing prose.
Suggestions (0)
Strengths
- The guard is placed before the scrubbed subprocess is spawned, so rejected review submissions cannot reach GitHub.
- Repository reachability is checked with the direct commit endpoint and indeterminate results fail closed.
- The test suite covers the cross-repository contamination case, malformed token lengths, ambiguous attestations, stdin handling, and CLI pass-through behavior.
Recommended Action
- Fix the Important issue before merge.
- Preserve the current fail-closed behavior for malformed or indeterminate attestations.
… token (BLO-32844) Addresses Ally's Important finding on #1744 at head 849ee29. 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 <noreply@anthropic.com>
Important issue fixed in
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c121fde
Prior Findings Dispositioned (1)
- prior:849ee29 important 1 — fixed —
packages/adapter-utils/src/github-review-attestation.ts:355-367— The candidate scanner now captures the complete remainder of the attestation line and validates it againstACCEPTED_ATTESTATION_TRAILER_PATTERN, so a 40-character SHA followed by prose or an unmatched delimiter is classified as malformed instead of being allowed through. Regression coverage is present atpackages/adapter-utils/src/github-review-attestation.test.ts:210-225.
Critical Issues (0)
Important Issues (1)
- [native-codex]
packages/adapter-utils/src/github-review-attestation.ts:253-258—gh api repos/{owner}/{repo}/pulls/{n}/reviews --method POST --input <file>is recognized as a review submission, but the input file is passed wholesale toinspectReviewAttestationrather than being decoded and scanned for its JSONbodyfield. A normal JSON request body containing an attestation therefore has no Markdown line match, returnsabsent, and bypasses the repository reachability check.- Parse
--inputas the review request JSON and inspect its stringbodyvalue (or reject unsupported input shapes). Add a regression test using a JSON file withevent,commit_id, and an attestation-bearingbody, asserting the target repository resolver is called and an unreachable SHA is refused.
- Parse
Suggestions (0)
Strengths
- The guard runs before scrubbing and before the real GitHub CLI is spawned, so refused submissions cannot reach GitHub.
- Direct commit endpoint reachability is used, and indeterminate results fail closed.
- The parser and attestation tests cover the real contamination and malformed-marker incidents, alternate flag spellings, fenced blocks, and prior-head reachability.
Recommended Action
- Fix the Important issue before merge.
- Preserve the current fail-closed behavior for malformed or indeterminate attestations.
…ning it (BLO-32844) Addresses Ally's second Important finding on #1744, at head c121fde. 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 <noreply@anthropic.com>
Important issue fixed in
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5e72e1a
Prior Findings Dispositioned (1)
- prior:c121fde important 1 — fixed —
packages/adapter-utils/src/github-review-attestation.ts:480-493—--inputreview submissions are now decoded as JSON and their stringbodymember is inspected before attestation validation; regression coverage atpackages/adapter-utils/src/github-review-attestation.test.ts:624-638verifies an unreachable cross-repository attestation is refused and the resolver is called.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The guard runs before scrubbing and before the target GitHub CLI is spawned, so refused reviews cannot reach GitHub.
- Reachability checks use the target repository and fail closed for indeterminate results while allowing valid prior commits.
- The parser distinguishes
gh pr review, field-based API submissions, and JSON--inputpayloads, with focused regression coverage for malformed, ambiguous, fenced, and cross-repository attestations. - The operator runbook documents each refusal reason and explicitly avoids bypassing the guard.
Recommended Action
- No Critical or Important issues remain from the current diff or active prior findings.
- Merge when the repository's remaining required checks are satisfied.
Thinking Path
Linked Issues or Issue Description
Path (B) — tracked in Paperclip, not GitHub Issues: BLO-32844. This PR closes its AC #2 (fail-closed guard) and AC #5 (regression coverage).
Bug report, in template terms:
Blockcast/mediamtx#33received review5146990033—APPROVED, authorallyblockcast[bot]— whose body reviewed a different PR in a different repo. Body marker81d63ac9ca72ddafc12919d3b40ae0870a901b7f(the head ofpim-multicast-gateway#2864);GET /repos/Blockcast/mediamtx/commits/81d63ac9…returns 422. The file it cites,.github/workflows/docker-e2e-tests.yml, 404s in mediamtx. The PR actually changed oneruns-on:line intest.yml.Blockcast/review-gate-action#7reviews5013561307and5013569224carry a 42-character marker (…a072+f6+f9a9…— two characters spliced into a real SHA) against a 40-charcommit_id. Both bodies byte-identical, 1m45s apart; the three other reviews on that PR are correct, so this is intermittent corrupt emission. It fails closed:REVIEWED_HEAD_ATTESTATION_PATTERNmatches nothing, so the review attests no head and can never satisfy the gate at any head, with no in-band recovery.Related PRs (searched; all consumer-side, none duplicates of this producer-side guard): Refs #1721 (BLO-32695, structured verdict block — no file overlap with this PR), Refs #1687 (BLO-32198), Refs #1667 (BLO-32044), Refs #1559, Refs #1141 (BLO-19778).
What Changed
packages/adapter-utils/src/github-review-attestation.ts. Recognises a review submission in aghargv (gh pr review, andgh api repos/{o}/{r}/pulls/{n}/reviewswith a write method), extracts theReviewed head:attestation, and classifies it as absent / well-formed / malformed / ambiguous. Argv-in, verdict-out; the only I/O is injected.github-cli-egress-runtime.ts, which is what the Helm-seededghwrapper execs (deploy/helm/paperclip/templates/statefulset.yaml:458). The guard runs before the credential scrub and before the CLI is spawned, and aborts with exit 65 and a diagnostic naming the SHA, the repo, and a stable machine-readable reason.GET /repos/{owner}/{repo}/commits/{sha}, with 404/422 read as a definite absence and everything else as indeterminate.guardIooption on the runtime options so tests can inject a resolver instead of reaching the network.runbooks/agent-review-submission-refused.md, indexed inrunbooks/README.md: what each refusal reason means, how to distinguish cross-repository contamination from a benign force-push, and why there is no bypass.github-review-attestation.test.ts, pinned to the real SHAs and the real contaminated body from both incidents rather than synthetic fixtures.Deliberate design choices, each of which was a live trap:
ally-review-detection.tsis right to ignore a 42-char marker when asking "is there a usable attestation?" — but a producer asking that same question waves the malformed review straight through, because it also sees none. So this finds any line that intends to attest, then demands exactly 40 hex.ally-review-detection.ts— otherwise a review of this guard would be refused for quoting the very line it validates.GETs the same.../reviewspath; guarding it would double the cost of every review. Note-Fmeans--body-fileongh pr reviewbut--fieldongh api, so the two forms are parsed separately.--approvewith no marker is a different defect (invariant I2d inscripts/check-ally-review-consistency.mjs); refusing it here would block every non-Ally agent's ordinarygh pr review.Verification
Exercised end-to-end against the live GitHub API with the real resolver, not mocks (replaying the actual incident):
81d63ac9…reachable inBlockcast/mediamtxunreachable81d63ac9…reachable inBlockcast/pim-multicast-gatewayreachableunreachable-attestation, exit 65, CLI never startedpim#2864)dbe4e10e…gh api repos/Blockcast/mediamtx/pulls/33/reviews(GET)The process-level path was checked too, through the module's CLI entry with a fake target that records any review attempt: exit 65, refusal on stderr, and the recorder never written — i.e. the review genuinely does not leave the pod.
Test-suite honesty: the full
packages/adapter-utilsrun shows 1 failure,mcp-isolation.integration.test.ts. It is a pre-existing parallel-load flake, not from this change — I ran the control both ways: clean master fails 2 (mcp-isolation.integrationandacpx-engine/session-store), and both pass in isolation on either tree. This branch is strictly better than master on that suite.Risks
dist/, so it takes effect on the nextadapter-utilsbuild/publish — merging this PR alone does not change fleet behaviour. Called out so nobody reads the merge as the mitigation.server/src/services/ally-review-detection.ts, intentionally:adapter-utilsships to agent pods and must not depend onserver/. Unification is tracked as BLO-32512; the loose/strict split is the property most easily lost in that merge, and the module header says so.Model Used
Claude Opus (Anthropic), model ID
claude-opus-5[1m]— the 1M-context variant, running in Claude Code with extended thinking, tool use, and shell/code execution. Used for the investigation, the implementation, the tests, and this description. All live measurements quoted above were executed as real commands against the GitHub API and the test suite, not recalled.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templaterunbooks/agent-review-submission-refused.md(triage per refusal reason, how to tell cross-repo contamination from a benign force-push, why there is no bypass) and indexed it inrunbooks/README.md. I deliberately did not touch the producer prose in.planning/ally-agent/AGENTS.md, which feat(review-gate): key the ally-comment gate on a structured verdict block, not prose regexes (BLO-32695) #1721 is currently editing.