feat(gate): gate on new CI failures instead of absolute green - #694
Conversation
`--gate-review` required absolute green CI, so in a repo whose base
branch is already red it could never pass and every ship needed a human
to reproduce the comparison by hand. The question a merge gate needs to
answer is not "is CI green?" but "does this change ADD a failure?".
- src/pr.js: getPullRequestStatus returns failedNames; checkOutcomes()
reads {failing,total} for a ref (check-runs + legacy statuses);
baselineFailures() unions the base branch HEAD with the head of the
last merged PR into that base.
- src/finish/review-gate.js: waitForGreenCi takes baselineFailures. When
non-empty, failures already red on base stop blocking, UNSTABLE joins
the trusted merge states, and the no-verdict fallback accounts every
check as success-or-cleared-failure.
- New --gate-baseline / --no-gate-baseline, wired through both finish
paths.
The baseline unions two sources because neither alone works: ci.yml
triggers on pull_request only, so test (node 20) never runs on main --
yet main carries one unrelated check-run, which defeats any "does the
base have checks?" heuristic. Verified live: the union reports
["test (node 20)"] for main; the base-only read returned [].
Nothing GitHub itself refuses is relaxed: pending still waits, ambiguous
states still block, BLOCKED/DIRTY/BEHIND still block, an unnameable
failure still blocks, and an unreachable API yields an empty baseline
(stricter, never looser).
Verified: npm test failing set byte-identical to main (41 pre-existing),
biome 1.9.4 clean, openspec change + specs valid.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- blockedStates/trustedStates are loop-invariant; build them once. - Coerce every counter through one helper so a partial snapshot cannot make an accounting sum NaN (NaN comparisons are false, which would read as 'nothing failing'). - Order the merged-PR lookup by updated desc; the created-desc default ranks a long-lived PR by when it was opened, not when it landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NagyVikt
left a comment
NagyVikt
left a comment
There was a problem hiding this comment.
Code review — PR #694
Decision: APPROVE with comments — 0 CRITICAL, 0 HIGH, 2 MEDIUM, 1 LOW.
Validation
| Check | Result |
|---|---|
| Lint (biome 1.9.4, pinned) | Pass — 188 files |
Tests (npm test) |
802/844 pass, 41 fail — failing set byte-identical to main |
node --check bin/multiagent-safety.js |
Pass |
bash scripts/check-script-symlinks.sh |
Pass |
openspec validate <change> --type change --strict |
Pass |
openspec validate --specs |
Pass — 133/133 |
| Live baseline read against this repo | branch 'main' + last merged PR (1f4fc50) → ["test (node 20)"] |
A real bug this change caught in itself
The first implementation computed c.failed + c.cancelled. On a snapshot with no
cancelled key that is NaN, and NaN > 0 is false — so a failing check would
have been waved straight through. The pre-existing test
waitForGreenCi fails closed on failed checks caught it, which is exactly what
the base-diff comparison is for. Now every counter goes through one coercing
helper, with a regression test for the partial-snapshot case.
Correctness properties verified
- Off by default. With no
--gate-baseline,baselineFailuresis an empty
Set,baselineModeis false, and every branch reduces to the original logic.
Two tests pin the unchanged behavior explicitly. - Nothing GitHub refuses is relaxed. Baseline mode moves exactly one value
(UNSTABLE) from blocked to trusted.BLOCKED/DIRTY/BEHINDstill block,
pending still waits,otherstates still block. Tested per state. - Unnameable failures block. If the failing-check count exceeds the number of
resolvable names, the gate blocks — an unnamed failure cannot be proven
pre-existing. - Unknown never reads as green. An unreachable API yields
total: 0and
contributes nothing, so the baseline shrinks and the comparison gets stricter.
MEDIUM
-
Whitelisting is per check NAME, not per failure cause. If
test (node 20)
is red on base for reason X and red on the PR for a new reason Y, baseline
mode merges it. Check-level granularity is all the GitHub API exposes here, so
this is inherent rather than fixable in this layer — but it is the real limit
of the guarantee, and anyone enabling the flag should know the gate proves
"no new failing check", not "no new failing test". -
lastMergedPrHeadsamples 10 PRs ordered byupdateddesc. GitHub cannot
sort by merge time, so a PR merged long ago but touched recently could outrank
a newer merge. Impact is bounded — a slightly staler baseline, which is the
safe direction — andupdatedis a much better proxy than thecreated
default it replaced.
LOW
checkOutcomessumstotalacross the check-runs and commit-status queries,
so a name reported by both would count twice.totalis only used as a
"did anything run here" signal, so this cannot change a verdict.
Note on scope
This fixes the CI half of the manual gate. The local agent-preflight.sh
gate still requires npm test to exit 0 and therefore still needs
--no-preflight in a baseline-red repo. Unattended ship in this repo also
remains blocked on the separate non-TTY provider-launch issue (cue launch: no profile resolved and stdin is not a TTY), which another lane owns.
1 participant
Summary
Test plan