fix(automerge): fail closed on unreadable manual-hold signals — retry×3 + cross-page timeline aggregation#122
Conversation
…×3 + cross-page timeline aggregation The Check-manual-hold step (the techrecon#405 defense: keep a human's explicit one-click hold in force across workflow re-runs) read both of its signals fail-open: `... 2>/dev/null || echo ""` degraded any label or timeline read failure to "no hold", so an API blip could defeat the human's hold under exactly the conditions where the defense matters. The sibling classifier-verdict step already fails closed on an unreadable label set (codex round-3 P1); this applies the same retry×3 then-fail-closed contract to both hold signals, and wires steps.hold.outcome == 'failure' into the always() error-revoke step so a failed read also disarms an arm a previous run already enabled. Two additional silent-defeat defects fixed and pinned: * The label read was unpaginated (30-label default page) — a hold label past position 30 read as absent. Now per_page=100 --paginate, same as the classifier read. * The timeline read piped `gh api --paginate` (one top-level JSON array PER PAGE) into an aggregating per-input jq — a per-page max printing one verdict line per page, which the `= "yes"` comparison can never match. Reproduced: newest event a human disable on page 2 ⇒ "no\nyes" ⇒ hold missed. Now one jq pass over all pages (-s, add), with no `try`: an unparseable body is an unknown hold state and fails closed. New selftest/test_automerge_hold_gate.sh (24 checks) extracts the shipped run block and pins label/timeline holds, actor + chronology + tie semantics, multi-page aggregation both directions, retry recovery, all fail-closed paths, and the error-revoke wiring; wired into test_workflow_guards.py so tests-runner enforces it. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Coverage Floor — mode:
|
|
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. Fail-closed hold logic is sound (retry×3 → exit 1, blocked by implicit success() on enable + always() error-revoke wiring), cross-page jq aggregation is correct, and new branches are well-covered by the 24-check selftest. |
1 participant
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 toci-workflowsare 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:
retry()×3 (5s/10s backoff) → on persistent failure,exit 1with a::error::explaining re-run/manual-merge. Also now paginated (per_page=100 --paginate, same as the classifier read).try— an unparseable body is an unknown hold state and fails closed, never open.steps.hold.outcome == 'failure'is wired into fix(automerge): close the pr-classify TOCTOU race — classify against base-branch risk-paths.yml at decision time #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.Two silent-defeat defects found while hardening (both fixed + pinned)
gh api --paginateemits 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 humanauto_merge_disabledon page 2 ⇒ outputno\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 astest_automerge_riskfile_gate.sh) and drives it through a stubbedghwith 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 intotest_workflow_guards.py→ enforced by tests-runner.${{ }}).uv run pytest -q→ 10 passed.actionlintclean. (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-workflowsare 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