ci: a skipped matrix job creates no per-leg contexts, so #351 made docs-only PRs unmergeable - #357
Merged
Merged
Conversation
…cs PRs unmergeable #351 gated the acceptance matrix with a job-level `if:` on the reasoning that a skipped job still reports and a skip satisfies a required check. That is true of an ordinary job and false of a matrix one: a skipped matrix never expands, so the per-leg contexts are not skipped, they are never created. #349 is the proof. Fifteen checks green, zero failures, one entry in the checks list named literally `acceptance: ${{ matrix.suite }}`, and all twelve required `acceptance: acceptance-*` contexts reported MISSING. A documentation-only pull request -- the exact case #351 was built for -- could not be merged at all. The #351 PR body named this trap ("if a required check is skipped by a path filter, GitHub can leave a PR unmergeable forever") and then walked into it, because the mitigation it chose was the same bug in a different costume. The matrix now always expands and every leg reports. What is conditional is the work: each step carries the gate, a documentation-only run costs a runner allocation instead of a suite, and an explicit notice step says why a green check did nothing rather than leaving a reader to guess. Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
There was a problem hiding this comment.
Pull request overview
This PR fixes a GitHub Actions branch-protection regression where a job-level if: on a matrix job prevented matrix expansion on docs-only PRs, resulting in missing (not skipped) required per-leg check contexts and therefore unmergeable pull requests. The workflow is adjusted so the matrix always expands (ensuring all required contexts exist) while the actual suite work is gated at the step level based on the changes job output.
Changes:
- Removes the job-level
if:from theacceptancematrix job to ensure all matrix legs (and thus all required contexts) are created. - Adds step-level gating (
if: needs.changes.outputs.code == 'true') to prevent suite work from running on documentation-only changes. - Adds an explicit
::noticestep for docs-only runs and avoids log artifact upload when suites did not run.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…d today it hung for 24 minutes `go build, vet, test` on #357 sat 24 minutes in "internal/api coverage measures the tests, not the preflight" against a step measured at 98-114s across the last five runs on main, until the job's own timeout-minutes: 25 cancelled it and named nothing. TWO GAPS, AND THE SECOND IS BROKEN ARITHMETIC WITH OR WITHOUT A HANG. The probes carried no -timeout, so Go's default of TEN MINUTES PER PROBE applied to three of them: 30 minutes of worst case behind a job ceiling of 25. The guard could never have reported; it could only ever have been cancelled. This is the error the acceptance job already documents about its watchdog, where the arithmetic "quietly assumes the suite starts when the job does". And the step was the one exception to a discipline this file states out loud at :340 -- "THE FOUR SCRIPT STEPS IN THIS JOB CARRY STEP TIMEOUTS ... a job timeout names nothing, a step timeout names the step". Four got one. The fifth is the one that hung. WHY GO'S TIMEOUT CANNOT BE THE BACKSTOP HERE, though it is still worth having. The guard captures each probe with `out="$(go test ...)"`, and command substitution blocks until the pipe has no writers left rather than until go test exits. A test leaking a child that holds inherited stdout hangs the capture after the binary is gone and after Go's timeout has already fired -- the mechanism the note at :360 already gives for the watchdog, one step below the step this happened to. So: -timeout 4m per probe (~3x the slowest real one, 12m worst case) and timeout-minutes: 14 on the step, above that 12 so Go still wins the race and prints the goroutine dump naming the test, and below the job's 25 either way. Verified: passes in 103s locally, matching CI's 98-114s. Mutating the flag to 1ms makes the first probe fail with `panic: test timed out after 1ms` naming TestLedgerPreflight, so the bound is wired to the probes rather than decorative. Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
|
This was referenced Aug 14, 2026
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.
The bug I shipped
#351 gated the acceptance matrix with a job-level
if:, on the reasoning that a skipped job still reports and a skip satisfies a required check.That is true of an ordinary job. It is false of a matrix job: a skipped matrix never expands, so the per-leg contexts are not skipped — they are never created.
#349 is the proof, and it is unambiguous:
A documentation-only pull request — the exact case #351 was built for — could not be merged at all. #351's own PR body named this trap and then walked into it, because the mitigation it chose was the same bug in a different costume.
The fix
The matrix always expands, so all twelve contexts report. What is conditional is the work: each step carries
if: needs.changes.outputs.code == 'true'.A step cannot end a job early without failing it, so the gate is spelled out on each of the six working steps rather than checked once.
Upload logsmoves fromalways()toalways() && …code == 'true'— no artifact exists on a docs run, only a warning about its absence.Added a
Documentation-only change, so this suite did not runstep that emits a::notice, so a reader of the checks list is never left guessing whether a green check ran the suite or no-opped.Why not the container job's shape
container-suitessolves this by emitting a suite list and usingif: needs.container-suites.outputs.suites != '[]'— the same job-level skip. It gets away with it because its per-suite contexts are not required; onlywhich container suitesis. That option is unavailable here.Verification
actionlint(with shellcheck): 7 findings before, 7 after — all pre-existing, none introduced.if:onacceptance, 12 matrix legs, and every one of the 9 steps carrying the intended condition.ci.ymland so is code by the workflow's own rule. Once this merges, docs: fold the coverage census into the note that already existed #349 is the docs-only run that must show 12 greenacceptance: acceptance-*contexts. If it does not, this fix is wrong and the remaining option is dropping those twelve from branch protection.Closes the regression from #351.
https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX