Repository navigation
fix(ci): an EMPTY check set must never arm auto-merge (#248) - #249
Merged
Merged
Conversation
The head-settle step's snapshot guard is on the BODY (`[[ -n "$body" ]]`). A
well-formed API answer that reports ZERO check runs is not empty, so it
passes that guard; the filter yields {pending:[],bad:[],all:[],total:0}; every
count reads 0; and the step fell through to "all 0 check run(s) on this head
passed - safe to arm auto-merge" with exit 0. The gate was then satisfied by
observing NOTHING - the same fail-open shape #247 removed for the empty body,
one level down.
The guard now also covers the SET. A total of 0 is its own class (exit 4,
EMPTY) and refuses to arm, because `arm` has `needs: [validate, lint]`: the
`validate` job - the required context - has COMPLETED by the time this step
reads the head, and it left a check run on that head. Only this job's own run
is excluded, so the non-self set is never empty by construction. A total of 0
means the head's check runs are not VISIBLE here - not that the head is green.
MEASURED over 48 merged PRs in 6 repos: the non-self set was never empty
(minimum 2), with the required `validate / validate` present 48/48. Unobserved
is not unreachable, so the gate refuses.
scripts/head-settle_test.sh covers it. The shipped text must not reach the
arming line on an empty set; a control built by removing EXACTLY the class-4
block from the shipped text must, so the coverage can fire.
The gate is STRENGTHENED, not loosened: one more refusal, on a state the old
shape could not distinguish from a green head.
Assisted-by: Claude Code minimax minimax-m3.1-flash:cloud (analysed on a live system)
Contributor
Review — PASSWhat the change does, re-derived from the diff
The block reads
Correctness / honesty checks
CommentsThere are no comments in the thread, so there is no sign-off to evaluate and nothing to disposition; the change is judged on its own evidence, which is internally consistent and reproduced by the diff. Verdict: PASS ollama-cloud/deepseek-v4.1-flash — action-review. |
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
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.
Summary
The
Head check-runsstep of the reusablecharly/pr-validatorworkflow — the merge guardrail that arms native auto-merge — could be satisfied by observing nothing. Its snapshot's guard is on the body:[[ -n "$body" ]]. A well-formed API answer that reports ZERO check runs is not an empty body, so it passes that guard; the jq filter yields{pending:[],bad:[],all:[],total:0}; every count reads 0; and the step fell through towith exit 0. The next step, "Enable auto-merge", then ran on a head whose checks the gate had never observed. This is the same fail-open shape #247 removed for the empty body, one level down.
The fix — the guard now also covers the SET. A total of 0 is its own class, and it refuses to arm. The exit code is the class, as it is for the four classes #247 introduced:
And the empty set is not a legitimate state for this job.
armhasneeds: [validate, lint], so thevalidatejob — the required context — has COMPLETED by the time this step reads the head, and it left a check run on that head. Thelintjob is unconditional today and left one too. Only this job's own run is excluded, so the non-self set is never empty by construction. A total of 0 therefore means the head's check runs are not visible here — a wrong head SHA, an API anomaly, or a lookup that does not cover them — and NOT that the head is green. Measured over 48 merged PRs in 6 org repos: the non-self set was never empty (minimum 2), with the requiredvalidate / validatepresent 48/48. Unobserved is not unreachable, so the gate refuses.The coverage can fire.
scripts/head-settle_test.shalready extracts the shipped step'srun:text and runs THAT against fixtures, and it already restores the pre-#229 shape as its negative control. This change adds a second control, isolated to this one guard: the shipped text with exactly the class-4 block removed. The shipped text must NOT reach the arming line on an empty set (exit 4); the control MUST (exit 0). If the guard is removed again, the control proves the coverage can fail.Files:
.github/workflows/pr-validator.yml— the class-4 guard, the class table, and the annotation that names the class, says it is not a finding about the change, and names the cheap remedy.scripts/head-settle_test.sh— the coverage: the empty-set fixture, the class-4 assertions, the isolated pre-pr-validator: a valid but EMPTY check set still reaches 'all 0 check run(s) passed' and arms auto-merge on a head the gate observed nothing on #248 control, and the class-table assertion.Head
845afd1229cf5fdc27cf8bc89c5dc2a146279f84— 2 files changed, 144 insertions, 5 deletions.Change classification
hook/workflow— one step of the reusable validator workflow, plus the coverage that exercises it. Thevalidatejob, the review itself, the requiredvalidate / validatecontext, and the auto-merge mutation are untouched.validatejob, the review, the required context, the pre-push gate, the branch-protection config, or the rulebook's landing/attribution/no-force-push/no-direct-push rules. The precedent in this repo is exact: ci(pr-validator): bound theEnsure charlystep with a 15-minute timeout (#178) #218 changed this same workflow file and the validator wrote "the change is fail-closed … so T4 sign-off is not triggered"; fix(ci): arm against the mutation, not against a proxy merge state #242 changed this samearmjob's arming step and merged on the same ground, against fix(validator): grant theissuesscope so a PRIVATE repo can be validated (#173) #174 (widened scope → T4 BLOCK).How tested
.github/workflows/workflow-checks.yml(theworkflow-checksjob). The change class is hook/workflow; this repo ships no candy or box, nocharlybinary and nodisposable: truedeploy, and the changed code runs in the CALLER's repository — so this repo's CI is the only place its own copy of the step executes, and the owner-script test is the only place its logic can be exercised.charly check run <bed>beds: it is the org's GitHub-configuration repo (workflows, shell and Python owner scripts), not a deployable. There is nodisposable: truetarget in it, so no bed applies to this change class; the gate above is the class's gate.charlybinary, socharly versiondoes not apply to it.armjob — this repo is the org gate's own first consumer. The PRE-change behaviour was measured, by running the pre-pr-validator: a valid but EMPTY check set still reaches 'all 0 check run(s) passed' and arms auto-merge on a head the gate observed nothing on #248 step text itself against a mocked GitHub API and a mocked clock (block 2): an empty check set reached the arming line with exit 0. The NEW branch (EMPTY = 4) is exercised against the SHIPPED step text with the same mocked API and clock, because a live head with ZERO observable check runs cannot be staged on demand. That is why the tier below isanalysed on a live systemand notfully tested and validated.Block 1 — the change-class gate, run on the committed tree (
git status --porcelainempty; the commands are the ones.github/workflows/workflow-checks.ymlruns, minus the twouses:runtime steps, which need GitHub's own runners and run in this PR's CI):Block 2 — each changed code path executing. The hole, measured on the pre-#248 shipped text (the step text extracted from
opencharly/.github@e716f7b, run against the empty-set fixture with a mocked API and a mocked clock), against the shipped text of this commit:and the empty-set assertions inside the gate itself, from block 1:
Block 3 — the R5 sweep, claim-keyed. The claim is "the gate never arms on a head it observed nothing on". Every surviving occurrence of the old arming phrase, and its classification:
Every hit is a comment describing the OLD behaviour, plus the one assertion that PROVES the old text produced it. No live path produces it: the phrase is emitted by the class-4 guard's absence, and the guard is present. The class table itself is enumerated in exactly two files — the workflow and its coverage — and both carry class 4. No legacy path, alias or transitional dual-mode survives: the change is purely additive, and no identifier is removed or renamed.
Block 4 — the live measurement behind "the empty set is not legitimate". 48 merged PRs across 6 org repos, the non-self check set of each merged head:
Rulebook compliance
/charly-internals:repo-setupand/charly-internals:git-workflow(this repo's ownAGENTS.mdnames both as its R0 set), and the dispatcher rows for a GitHub workflow change and for a failing gate.origin/main; one commit; the push is the first.git status --porcelainwas empty before the commit and after it.armjob.charlybinary, so there is no binary to rebuild andcharly versiondoes not apply to it..github/workflows/workflow-checks.yml, run on the committed tree (block 1): every offline step returned rc=0 and produced no warning. There is nodisposable: truetarget in this repo, so no bed applies.plan:. SDD: N/A — no schema and no.cue.setup-gopin, same file, different lines) does not overlap this diff.Agent:FIRST andAssisted-by:LAST on the issue and the PR.workflow-checks.yml) is unchanged — the test file was already wired into it, so no second copy of the logic and no new gate step.Attribution tier
analysed on a live system. The change-class gate passed on the final tree with zero warnings, and the change was analysed against live systems — the real GitHub API, the org's real check-run history across 48 merged PRs, and the org gate's own first consumer. It is notfully tested and validatedbecause the discriminating branch (EMPTY = 4) is exercised against the extracted shipped text with a mocked API and a mocked clock: a live head with ZERO observable check runs cannot be staged on demand, and a test that could not fail would be invalid under R7.Agent: charly
Assisted-by: Claude Code minimax minimax-m3.1-flash:cloud (analysed on a live system)