fix(automerge): close the pr-classify TOCTOU race — classify against base-branch risk-paths.yml at decision time#121
Merged
Conversation
…base-branch risk-paths.yml at decision time The reusable trusted the live risk:* label as its canonical manual-merge signal, with a documented mitigation: the caller wires 'labeled' into trigger types so a late label re-runs the workflow and revokes. That re-run is structurally unreachable — pr-classify labels via GITHUB_TOKEN, and GITHUB_TOKEN-attributed events never trigger workflows. whois-api-llc/wxa-secrets#27 (2026-07-17): automerge read labels 4s before risk:blocked landed, armed, and the PR merged when checks went green. Every repo-specific blocked:/sensitive: path not covered by the global regex had this race. The verdict step now computes the classification itself at decision time: it reads .github/risk-paths.yml from the PR's BASE branch (a PR cannot edit its own risk file to escape; 404 = no repo policy, regex still gates; any other read failure fails CLOSED) and runs the same classify.mjs pr-classify fetches — one matcher, zero glob drift. The risk:* label stays honored as belt-and-suspenders (labels only ever ADD gating). No checks:read required (wxa-secrets deliberately omits it). selftest/test_automerge_riskfile_gate.sh extracts the shipped step and pins: the #27 race shape, sensitive, clean, label-only, base-ref pinning (incl. non-default base), 404 fall-through, fail-closed on API error and the 3000-file cap, and nocase parity via the real classify.mjs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Bind the arm to the event head SHA (gh pr merge --auto --match-head-commit): GraphQL rejects the enable atomically if the head moved between event and arm, so a mid-run push can never inherit an arm decided on older files. Head-moved failures exit 0 with a notice (the newer push's own run re-arms); other failures stay red. - 404 on the base ref now falls back to the DEFAULT branch's risk-paths.yml — a release/legacy base predating the policy file can't dodge the repo policy. Absent on both = policy-free repo (automerge-only callers stay supported). - Closed enum on classifier output: anything outside the documented class set fails closed instead of reading as benign. - Deliberately NOT pinning classify.mjs/deps beyond what pr-classify itself does — a one-sided pin would reintroduce matcher drift. Selftest: +default-branch-fallback case, +enum-violation case, +structural asserts for --match-head-commit and per-ref reads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… P1) A hand-applied risk:blocked/risk:sensitive label is an independent signal the direct file classification cannot reconstruct — a human may know context no path glob carries. The previous '|| echo ""' degraded a failed label read to 'no label', which Signal 2 masked on blocked-path PRs but silently lost hand-applied labels on benign-path PRs. Now: retry x3, then refuse to arm (fail closed). Selftest: the old case 10 asserted the masked behavior; replaced with two fail-closed cases (blocked files + the load-bearing clean-files variant). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ad (codex round-4) - 'Refusing to arm' is not fail-closed for a PR a previous run already armed: a gating step that errors kills the job before the revoke steps, leaving the stale arm to merge once checks pass. New terminal always() step revokes any existing arm whenever classifier_verdict or the risk scan errored; the job still ends red from the failed gate. - Label read now paginates (per_page=100 --paginate) — a hand-applied risk label past the 30-label default page must not read as absent. - Documented the deliberate no-revoke in the enable step's head-moved path: a stale run revoking could disarm the newer head's valid arm and strand clean PRs on rapid fixup pushes; a blocked post-arm push is revoked by its own synchronize run, which only loses if every required check beats ~1 min — not a real shape in this fleet. Selftest: structural pins for the always() error-revoke wiring and label pagination. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dex round-5 P1) A checkout/detect/setup-node error leaves the gate steps 'skipped', not 'failure' — the round-4 error-revoke condition never fired and a stale arm survived a red run. The cleanup now watches those outcomes as well, and when checkout/detect died before authorship detection it falls back to the payload-level claude/* branch prefix (trailer-only Claude PRs with a failed checkout remain uncovered — accepted, rare- squared, and the step itself needs no workspace so it runs even when checkout failed). Selftest: structural pin extended to the prerequisite outcomes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-6 P2) An older run erroring late could revoke unconditionally AFTER a newer-head run validly armed — disarming a clean PR with no event left to re-arm it (the same ordering hazard the enable step's head-moved path documents). The cleanup now skips only on a POSITIVE head-moved read (the newer head's run owns gating); a failed head read still revokes, failing toward safety. Codex round 6 also flagged (deferred, documented in the PR): a stale automated risk label can strand a de-risked PR until the next event — pure liveness, pre-existing behavior, needs the timeline-actor distinction (bot vs hand-applied) as its own change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Coverage Floor — mode:
|
topcoder1
marked this pull request as ready for review
July 17, 2026 05:08
|
Risk class: This PR touches one of the blocked path categories from Auto-merge is refused by (This is a policy notice, not a code-quality failure. The classify job itself does not fail — required CI checks remain authoritative for "is the code green.") |
|
No issues found. Verdict step's class enum matches classify.mjs, invocation contract (cwd rules + stdin files) is correct, all fail-closed branches propagate nonzero under set -e, and the enable/error-revoke gating is sound. Selftest exercises the shipped bash against the real classifier. |
topcoder1
added a commit
that referenced
this pull request
Jul 17, 2026
…×3 + cross-page timeline aggregation (#122) > [!NOTE] > Fast-follow to #121 (merged): the P2 deferred from its review — the manual-hold step's own fail-open reads. Builds directly on #121's `retry()` pattern and always() error-revoke step. Authored stacked on the #121 branch; rebased onto main (single commit, clean replay) after #121 click-merged mid-flight. Draft per repo policy: PRs to `ci-workflows` are always manual click-merge. ## What The **Check manual hold** step is the techrecon#405 defense: it keeps a human's explicit "do not auto-merge" decision (hold label, or a human `gh pr merge --disable-auto`) in force across workflow re-runs. Both of its reads were fail-open — `… 2>/dev/null || echo ""` degraded any API failure to "no hold" — so an API blip could defeat the human's hold under exactly the conditions (API misbehavior) where the defense matters most. The step's header even promised this as a feature ("degrades gracefully… can never crash the run"). The sibling classifier-verdict step already fails closed on an unreadable label set after 3 retries (#121, codex round-3 P1). This PR applies the identical contract to both hold signals: - **Label read**: `retry()` ×3 (5s/10s backoff) → on persistent failure, `exit 1` with a `::error::` explaining re-run/manual-merge. Also now paginated (`per_page=100 --paginate`, same as the classifier read). - **Timeline read**: same retry×3 → fail closed. The jq lost its `try` — an unparseable body is an *unknown* hold state and fails closed, never open. - **Already-armed PRs**: `steps.hold.outcome == 'failure'` is wired into #121's always() **error-revoke** step, so a failed hold read doesn't just refuse to arm — it disarms an arm a previous run already enabled. Without this, "fail closed" would only be half true. - The stale header-comment promise is rewritten with the fail-closed rationale (red run = one re-run or a manual click; silently dropped hold = unwanted merge of exactly the PR a human flagged). ## Two silent-defeat defects found while hardening (both fixed + pinned) 1. **Unpaginated label read** — default 30 labels/page; a hold label past position 30 read as absent. 2. **Per-page timeline aggregation** — `gh api --paginate` emits each page as its *own* top-level JSON array (`gh api --help`: "Each page is a separate JSON array or object"), and the old single-pass jq computed a **per-page max**, printing one verdict line per page. Multi-line output can never equal `"yes"`, so any PR whose timeline crossed one page (30 events pre-fix — routine for busy PRs, which are the ones humans hold) silently reported "no hold" **deterministically**, blips aside. Reproduced with the exact production jq: newest event a human `auto_merge_disabled` on page 2 ⇒ output `no\nyes` ⇒ hold missed; same events in one array ⇒ `yes`. Now one jq pass over all pages (`-s` + `add`). Also: both reads switched from `${{ github.repository }}` interpolation to `$GITHUB_REPOSITORY` — behavior-identical in Actions, makes the run block extractable for the selftest, and removes a template-interpolation-in-script pattern. ## Not changed Hold semantics are untouched: label exact-match, actor filtering (bot self-revokes don't hold), chronology + same-second tie toward the human disable, label short-circuits timeline, timeline holds lift only by manual re-enable. All pinned by the new test against the shipped script. ## Tests New `selftest/test_automerge_hold_gate.sh` (24 checks) — awk-extracts the shipped run block (same pattern as `test_automerge_riskfile_gate.sh`) and drives it through a stubbed `gh` with failure-injection counters: label/timeline hold detection, near-miss label names, bot-vs-human actor, re-enable-lifts, same-second tie, **multi-page aggregation in both directions**, retry recovery (exactly 3 attempts, recovered value used), all fail-closed paths (label ×3, timeline ×3, 200-with-garbage), `hold_label`-disabled, empty timeline, plus structural pins (error-revoke wiring, pagination, no `${{ }}` in the run block, stale promise gone). Wired into `test_workflow_guards.py` → enforced by tests-runner. - Red run against the unmodified step: structural pins + every fail-closed/pagination case fail (the old block isn't even executable standalone due to `${{ }}`). - Green: all 24 pass. Full suite: `uv run pytest -q` → **10 passed**. `actionlint` clean. (Prettier drift on this file pre-exists on the base branch and workflow yml isn't in CI's prettier scope — left untouched to keep the diff surgical.) ## Residual A persistent API outage now ends the run red with auto-merge disarmed — the PR waits for a re-run or a human click instead of merging past a hold. That's the intended direction of failure. Auto-merge rationale: manual-merge category — PRs to `ci-workflows` are always click-merged, and this touches `.github/workflows/**` (production CI infra). Codex rounds: 2 (both clean — zero findings; round 1: "No discrete regression introduced by this patch was identified", round 2 concurred) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem — fleet-wide TOCTOU race between pr-classify and claude-author-automerge
Evidence (whois-api-llc/wxa-secrets#27, 2026-07-17 UTC): both reusables fired on the same
ready_for_reviewevent at 00:40:37. The automerge run queried live labels at 00:40:43 (none yet), armed auto-merge at 00:40:46; pr-classify appliedrisk:blockedat 00:40:50 — 4 seconds late — and the PR merged at 00:42:30 when Claude Review went green, despitesrc/wxa_secrets/**being in the repo'sblocked:list.The documented mitigation ("caller wires
labeledinto trigger types so a re-run revokes") is structurally unreachable: pr-classify labels withGITHUB_TOKEN, and GITHUB_TOKEN-attributed events never trigger workflows. Every repo-specificblocked:/sensitive:path not covered by the central regex had this race.Fix — compute the classifier verdict at decision time, race-free
The
classifier_verdictstep no longer depends on the label round-trip:risk:blocked/risk:sensitivelabel still gates (hand-applied labels are an independent signal). The label read now paginates and fails closed after 3 retries — it can no longer silently degrade to "no label"..github/risk-paths.ymlfrom the PR's base branch (a PR cannot edit its own risk file to escape; on base-ref 404 it falls back to the default branch's file so legacy/release bases can't dodge policy; absent on both = policy-free repo, global regex still gates) and classifies the changed files with the sameclassify.mjspr-classify fetches — one matcher, zero glob-semantics drift, nocase/bracket/negation guards inherited.blocked/sensitive⇒ refuse + revoke + sticky comment.Fail-closed everywhere it matters: non-404 rules read, failed label read, failed file listing, ≥3000-file truncation, classifier parse/guard errors, and output outside the class enum all refuse to arm. A terminal
always()step revokes any pre-existing arm when a gating or prerequisite (checkout/detect/setup-node) step errors — a red run must not leave a stale arm alive — with a head-ownership guard so a stale run never disarms a newer head's valid arm. Arming itself is bound to the event head SHA (--match-head-commit→ GraphQLexpectedHeadOid, verified live), so a mid-run push can never inherit an arm decided on older files.No
checks: readrequired anywhere (wxa-secrets deliberately omits it). No caller changes needed; the reusable floats@main, so this lands fleet-wide on merge. Cost: ~10–20s per Claude-PR run (setup-node + npm + classify).Alternatives rejected: polling for the classify check needs
checks:readin every caller; PAT-labeling still leaves an arm→revoke window that loses when required checks are already green.Selftest (
selftest/test_automerge_riskfile_gate.sh, CI-enforced via pytest)Extracts the shipped step from the YAML and executes it against a stubbed
gh+ the realclassify.mjs: the #27 race shape, sensitive, clean, label-only, base-ref pinning (every case; incl. non-default base), default-branch fallback, 404×2 fall-through, fail-closed on API error / 3000-file cap / unreadable labels (blocked and clean files) / enum violation, nocase parity. Structural pins: shared classifier source + dep pins in both workflows,--match-head-commit,always()error-revoke wiring incl. prerequisites + head guard, label pagination.Verification
actionlint: clean (both workflows)bash selftest/test_automerge_riskfile_gate.sh: 21/21uv run pytest -q: 9 passed (new selftest registered intest_workflow_guards.py)gh pr merge --auto --match-head-commit <wrong-sha>probed live: rejected atomically, combo confirmedCodex rounds: 6 (hard cap)
r1 clean → r2 3 findings (2 fixed: head-SHA binding, 404 fallback + enum guard; pin/integrity sub-point rejected: one-sided pinning would reintroduce matcher drift vs pr-classify) → r3 1 fixed (fail-closed label read) → r4 3 (fixed: error-revoke step, label pagination; rejected: revoke-on-head-moved in enable path — would strand clean PRs on rapid fixup pushes; documented instead) → r5 1 fixed (prerequisite failures skip gates → covered) → r6 0 P1 / 2 P2 (fixed: head-ownership guard on error-revoke; deferred: stale automated label can strand a de-risked PR until the next event — pure liveness, pre-existing, self-heals; clean fix is distinguishing bot- vs hand-applied labels via timeline actor, follow-up).
Known residuals (documented in-file): post-arm blocked push relies on its synchronize run's revoke (~1 min), which loses only if every required check beats it — not a real shape in this fleet; trailer-only Claude PRs with a failed checkout aren't covered by the error-revoke authorship fallback. Related follow-up chip spawned: the manual-hold step's own fail-open label read (pre-existing, same class).
wxa-secrets already shipped a repo-side race-proof gate (CODEOWNERS + require_code_owner_review, whois-api-llc/wxa-secrets#29); this central fix covers the rest of the fleet.
Auto-merge rationale: manual merge — production infrastructure (
.github/workflows/**), and PRs to topcoder1/ci-workflows are always manual. Opened as draft per fleet policy.🤖 Generated with Claude Code