fix(ci): an independent guard reports rather than being masked by the guard before it - #4915
Conversation
… guard before it `node-repo-validate.yml`'s `validate` job is ~36 independent guards in one step list, and GitHub's implicit step condition is `success()` — so the FIRST failing guard skips every guard behind it, and a `skipped` step publishes no failure. Measured on MeshWeaver.SocialMedia#210 (run 35431670104, job 105867277548, 2026-09-19 08:22Z): the vendored-resolver drift check failed with `32 code line(s) differ` and 16 steps reported `skipped` behind it, including `check-pr-secret-preflight.py`, the manifest-lock currency check, the module-version check, the duplicate-key guard, the pin-drift guard and no-op parity. `validate / Validate node repos` is a REQUIRED context in all five satellites, and a required context that reported `skipped` counts as SATISFIED under both classic protection and rulesets. Because `platform-ref` defaults to `main`, every satellite is drifted from the instant a canonical change merges — so for the length of each re-copy wave every pull request in that repository was unguarded by all six, with one red about an unrelated file as the only symptom. The trapdoor is made by step ORDERING, not by a `continue-on-error:` or an `if:`, so nothing in the workflow looks wrong on inspection — which is why it needs a gate and not a comment. - every one of the 36 guard steps now carries `!cancelled()` (`&&`-ed with the five conditions already there), dropping the implicit `success()` so the step runs and reports; a failure still fails the job. - `.github/scripts/check-guard-step-masking.py` holds it: every step after the declared prerequisite PREFIX must carry `!cancelled()` or `always()`; the prerequisites (checkout, its history fetch, setup-python) must BE a prefix, because one in the middle is the same defect wearing a legitimate name; and a prerequisite naming a step that was renamed is STALE and reds. 13 self-test cases, both arms. Wired into core's own guard list in dotnet-test.yml. Control: the gate run against `origin/main`'s copy of the workflow reports `36 independent guard step(s) checked, 36 violation(s)` and exits 1; against this branch's copy, `36 ... checked, 0 violation(s)` and exits 0. A guard's own FETCH is deliberately not a prerequisite: its consumers now run and fail naming the file they could not open — a second red rather than a silent skip — and the fetch's `::error::` is the root. Fixes #4784 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new checker can pass vacuously, and both the checker and prerequisite failure paths can still produce masked or misleading validation results.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (4)
What changed in this PR
Strengthens the shared CI validation lane so independent guards continue reporting after failures.
Changes:
- Adds
!cancelled()to validation guard steps. - Introduces structural validation and self-tests for guard-step masking.
- Documents the skip-trapdoor and its operational impact.
| File | Description |
|---|---|
.github/scripts/check-guard-step-masking.py |
New guard invariant checker |
.github/workflows/node-repo-validate.yml |
Makes validation steps failure-independent |
.github/workflows/dotnet-test.yml |
Runs the new checker in core CI |
src/MeshWeaver.Documentation/Data/Architecture/ReadingCiSignals.md |
Documents skipped-step behavior |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-19-one-failing-check-used-to-silence-sixteen-others.md |
Adds the fix release note |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for index in range(len(prerequisites), len(steps)): | ||
| steps_checked += 1 | ||
| condition = steps[index].get("if") | ||
| text = "" if condition is None else str(condition) | ||
| if not any(token in text for token in SURVIVES_A_FAILURE): |
There was a problem hiding this comment.
Correct, and it is the same class of defect as the one this file exists to catch — a check that cannot fail. Fixed.
check_tree now refuses a job whose step count is at or below its declared prerequisite count, naming both numbers and both readings ("either the guards were removed, which is the finding, or they moved and this declaration is stale"), and it continues so no later assertion pretends to have run.
Self-test case prefix-only-is-not-a-pass drives _PREFIX with nothing after it and is confirmed to FIRE:
self-test ok prefix-only-is-not-a-pass expected=fire got=fire
A related hole you did not name but which the same reasoning reaches: the readiness step could stop publishing what the guards read, which would skip every one of them, silently and green. readiness-publishes-nothing covers that too.
| def _matches(step_id: str, declared: str) -> bool: | ||
| if declared.startswith("uses:"): | ||
| return step_id.startswith(declared) | ||
| return step_id == declared |
There was a problem hiding this comment.
Right, and thank you — _step_id stripping the @ref is exactly what made the prefix comparison look harmless. Fixed: _matches is now exact equality for both shapes, with the reason written into its docstring rather than left to be re-derived.
def _matches(step_id: str, declared: str) -> bool:
"""Exact, both shapes. `_step_id` already strips an action's `@ref`, so a PREFIX comparison would
let `actions/checkout-foo` satisfy a declared `uses:actions/checkout` — a renamed prerequisite
passing the structural check instead of being reported stale."""
return step_id == declaredSelf-test case look-alike-action-is-not-the-prerequisite uses actions/checkout-foo@v7 as step 0 and is confirmed to FIRE:
self-test ok look-alike-action-is-not-the-prerequisite expected=fire got=fire
| - name: "An independent guard reports rather than being masked by an earlier one — the gate can fail (self-test)" | ||
| run: python3 .github/scripts/check-guard-step-masking.py --self-test | ||
| - name: "No guard in the shared validate lane is masked by the guard before it" | ||
| run: python3 .github/scripts/check-guard-step-masking.py --root . |
There was a problem hiding this comment.
Correct, and it is worse than the two steps you named: workflow-shell is 62 steps — two prerequisites and 60 independent guards, every one of them on the implicit success(). It is the job that gates main-cd.yml, the module lanes and every script a satellite fetches, so one red there silently withdrew the rest of CI's self-enforcement for that run. Adding the condition to only my two steps would have left the other 58 in the defect.
So the fix is the same treatment, applied to that job as well, and — the part that stops it coming back — dotnet-test.yml / workflow-shell is now a declared subject of the guard itself, beside the validate lane. All 60 guards carry the condition; the two prerequisites do not.
Control, on origin/main's copy of dotnet-test.yml with this branch's node-repo-validate.yml:
check-guard-step-masking: 2 job(s), 93 guard step(s) checked, 116 violation(s) → exit 1
and on this branch:
check-guard-step-masking: 2 job(s), 96 guard step(s) checked, 0 violation(s) → exit 0
The self-test also gained shell-job-checked-too, which leaves the validate lane correct and breaks only the shell job — otherwise every other case keeps dotnet-test.yml valid and none of them would notice if that subject stopped being checked.
| # previous imported commit. The guard is fetched at the lane's own scripts ref for the same | ||
| # reason as the other centralized policy below: a caller cannot silently retain an old rule. | ||
| - name: Fetch the content-CI publish-path guard at the lane's scripts ref | ||
| if: ${{ !cancelled() }} # an independent guard REPORTS; it is never masked by an earlier one (#4784) |
There was a problem hiding this comment.
This is the finding I am most glad you made, because it is the half I got wrong rather than an omission: !cancelled() drops the implicit success() for the prerequisites too, so a failed checkout let all 36 guards run against an empty workspace. Mostly that is a wall of secondary reds, but for any guard that happens to pass on an empty tree it is a vacuous pass — the thing this file exists to prevent.
Fixed by making the prerequisite line explicit rather than positional. The last prerequisite publishes one output:
- name: The workspace and the tools are present — the ONE prerequisite every guard shares
id: ready
run: echo "ok=true" >> "$GITHUB_OUTPUT"and every guard requires it:
if: ${{ !cancelled() && steps.ready.outputs.ok == 'true' }}The two failure modes are then separate, which is the property that was missing:
- a prerequisite fails ⇒
readyis skipped, its output is empty, every guard is skipped, and the prerequisite's own red is the verdict. Nothing runs against an empty workspace. - a guard fails ⇒
readyis untouched, so every other guard still reports.
I preferred this to steps.<id>.outcome == 'success' on each prerequisite because it is one expression rather than three repeated 96 times, and because it names the concept ("the prerequisite line") in the workflow instead of encoding it in a list a reader has to reconstruct.
The guard enforces both halves now — a condition carrying !cancelled() but not the readiness gate is a violation with its own message — and cancelled-without-readiness is the case that fires on exactly the shape I originally shipped:
self-test ok cancelled-without-readiness expected=fire got=fire
A guard's own fetch is still deliberately not a prerequisite: its consumers run and fail naming the file they could not open, which is a second red rather than a silent skip, and the fetch's ::error:: is the root.
…d job had the same defect
Four Copilot findings, all correct, and two of them are the half I got wrong rather than omissions.
1. `!cancelled()` drops the implicit `success()` for the PREREQUISITES too, so a failed checkout let
all 36 guards run against an empty workspace — a wall of secondary reds and, for any guard that
passes on an empty tree, a vacuous pass. The prerequisite line is now explicit rather than
positional: the last one publishes `ok=true`, and every guard carries
`!cancelled() && steps.ready.outputs.ok == 'true'`. A prerequisite failing skips every guard and
its own red is the verdict; a guard failing skips nothing. Preferred over three
`steps.<id>.outcome` terms repeated 96 times, and it names the concept in the workflow.
2. `dotnet-test.yml`'s `workflow-shell` is 62 steps — TWO prerequisites and SIXTY independent guards,
every one on the implicit `success()`. It is the job that gates main-cd.yml, the module lanes and
every script a satellite fetches, so one red there silently withdrew the rest of CI's
self-enforcement for that run. Given the same treatment, and DECLARED AS A SUBJECT of the guard so
it cannot drift back.
3. The checker passed vacuously when a job held only its prerequisites: `range(len(prerequisites),
len(steps))` is empty, so every per-step assertion was skipped and it exited 0 over a job whose 36
guards had been deleted. Now refused, naming both readings. Same reasoning reaches a readiness step
that stopped publishing `ok=true` — also refused, because that would skip every guard silently.
4. `_matches` compared `uses:` prefixes while `_step_id` already strips the `@ref`, so
`actions/checkout-foo` satisfied a declared `uses:actions/checkout`. Exact equality now.
Self-test: 19 cases, every one fired on its defect and stayed silent on its fix. The new ones are
`prefix-only-is-not-a-pass`, `readiness-publishes-nothing`, `cancelled-without-readiness`,
`look-alike-action-is-not-the-prerequisite`, `double-quoted-readiness-accepted` and
`shell-job-checked-too` — the last leaves the validate lane correct and breaks ONLY the shell job,
because every other case keeps `dotnet-test.yml` valid and none of them would notice if that subject
stopped being checked.
Controls:
origin/main's dotnet-test.yml + this branch's node-repo-validate.yml
→ 2 job(s), 93 guard step(s) checked, 116 violation(s) exit 1
this branch
→ 2 job(s), 96 guard step(s) checked, 0 violation(s) exit 0
check-workflow-{timeouts,yaml-keys,shell} and check-reusable-workflow-runners: unchanged and green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
⛔ Merge-queue steward: left dequeued — a job other than a test shard failed — a build or gate failure is never a flake.
Not a catalogued flake. Fix the failure, or — with run URLs, an issue and an assertion-message pattern — add it to |
meshweaver-cloud
Bot
added
the
queue-rejected
|
⛔ Merge-queue steward: left dequeued — a job other than a test shard failed — a build or gate failure is never a flake.
Not a catalogued flake. Fix the failure, or — with run URLs, an issue and an assertion-message pattern — add it to |
This is DIRTY on a real content collision, not on driftMerging add/add means They are NOT the same guard, so this is not simply superseded
478 lines differ. I checked whether this branch's version subsumes Why I stopped hereResolving it means deciding how the two guards relate — one file covering both defects, or two files The rest of the merge is clean apart from (No changes pushed. Reporting only, so the conflict is understood before it is resolved — |
…4949 landed the first half of the same fix Main already carries `check-guard-step-masking.py` and `!cancelled()` on every non-prologue step (#4949, merged 2026-09-20). This branch keeps the second half that #4949 does not have: the `ready` prerequisite output every guard requires, so a failed checkout, history fetch or tool setup still masks the guards (Copilot on #4915) instead of letting them run against an empty workspace. Resolution: every conflict hunk was the same step carrying the two spellings, resolved to the readiness-gated form; the three steps main added since (#4785, the node-test launcher guard) and the #4785 self-test in `workflow-shell` got the same gate; main's duplicate wiring of the guard in `workflow-shell` was dropped in favour of this branch's gated pair; the script is this branch's (a superset of main's rules, 19 self-test cases). The dated What's New this branch minted is dropped for main's existing entry, which gains one paragraph on the prerequisite half (policy `whatsnew-cadence`), and the doc page's guard counts follow the merged tree (39 and 61). Verified on the merged tree: check-guard-step-masking 2 jobs / 100 steps / 0 violations, self-test 19/19; check-workflow-timeouts 90 / 0; check-workflow-yaml-keys 37 / 0; check-reusable-workflow-runners 24 / 0; check-workflow-shell 0 live; permission-pairing 5 / 0; pr-secret-preflight 2 / 2 / 0; actionlint clean on both workflows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The skip-trapdoor is made by step ORDERING, and nothing in the file looks wrong
node-repo-validate.yml'svalidatejob runs ~36 independent guards as consecutive steps.GitHub's implicit step condition is
success(), so the first failing guard skips every guard behindit — and a
skippedstep publishes no failure.Measured on MeshWeaver.SocialMedia#210, run
35431670104, job105867277548, 2026-09-19 08:22Z:the vendored-resolver drift check failed with
32 code line(s) differand 16 steps reportedskippedbehind it, among themEvery PR-reachable secret in this repo is asserted by a preflightEvery manifest.lock is current (and carries a version)Every module's version matches its contentNo mapping in this repo's workflows writes a key twiceNo pin comment names a commit this repo no longer pinsThis repo's no-op set agrees with the platform'svalidate / Validate node reposis a required context in all five satellites, and a requiredcontext that reported
skippedcounts as satisfied under both classic protection and rulesets.Because
platform-refdefaults tomainand the drift check fetches the canonical live, everysatellite is drifted from the instant a canonical change merges — so for the length of each re-copy
wave, every pull request in that repository was unguarded by all six, with one red about an unrelated
file as the only symptom.
This is the shape AGENTS.md legislates against, except the trapdoor is
success()-by-default ratherthan a
continue-on-error:or anif:.What changed
!cancelled()(&&-ed with the five conditions thatwere already there), which drops the implicit
success()so the step runs and reports its ownverdict. A failure still fails the job.
.github/scripts/check-guard-step-masking.pykeeps it that way. For each declared(workflow, job): every step after the prerequisite prefix must carry
!cancelled()oralways(); the prerequisites — the checkout, its history fetch,setup-python— must be aprefix, because a prerequisite in the middle is the same defect wearing a legitimate name; and a
prerequisite naming a step that was renamed or removed is stale and reds, so the guard cannot
answer green having checked nothing. 13 self-test cases, both arms. Wired into core's own guard
list in
dotnet-test.yml(self-test first, then--root .).A guard's own fetch is deliberately not a prerequisite. Its consumers now run and fail naming
the file they could not open — a second red rather than a silent skip — and the fetch's
::error::is the root.
Control
The neighbouring workflow guards still pass on the edited tree:
check-workflow-timeouts90 jobs /0 violations,
check-workflow-yaml-keys37 files / 0,check-reusable-workflow-runners24 jobs / 0,check-workflow-shell0 live findings.Not in this change
#4786(fork PRs and the ARC runners) is the adjacent structural issue and is a runner-policydecision, not a defect fix; it is left open. Option 2 of #4784's acceptance — one job per guard
family — was not taken: it would turn one job into ~10, each re-checking out the repository, against
a fleet whose stated goal is zero Actions spend.
Merged with #4949, which landed the FIRST half of this fix on main
While this PR was open, #4949 fixed #4784 on main with
if: ${{ !cancelled() }}on everynon-prologue step and its own
check-guard-step-masking.py. That is the first half. What this PRstill adds is the second half, the one Copilot raised on this PR: bare
!cancelled()also stops theprerequisites from masking, so a failed checkout lets every guard run against an empty workspace,
which is a wall of secondary reds and, for any guard that passes on an empty tree, a vacuous pass.
Here the last prerequisite publishes
ok=trueand every guard requiressteps.ready.outputs.ok == 'true', so a prerequisite failing still stops everything with its own red,and a guard failing stops nothing but itself.
Resolution of the merge: every conflict hunk was the same step under the two spellings, taken in the
gated form; the guard steps main added since (#4785, in both jobs) got the same gate; main's duplicate
wiring of the guard in
workflow-shellwas dropped for this PR's gated pair; the script is this PR's(a superset of main's rules, 19 self-test cases, 0 violations over 100 guard steps on the merged
tree); the dated What's New this PR minted is dropped for main's existing entry, which gains one
paragraph on the prerequisite half (policy
whatsnew-cadence).Fixes #4784
Pairs-with: none — no
publictype or member leavessrc/; the diff is two workflow files, a new CIscript, a doc page and a What's New entry.
🤖 Generated with Claude Code