[PR #202] [MERGED] fix(workflow-pins): report a step-output `ref:` whose id cannot resolve, and read the `||` fallback spelling (BE-8220) #210

New issue

Closed

opened 2026-10-26 22:37:23 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:23 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/202
Author: @mattmillerai
Created: 8/20/2026
Status: ✅ Merged
Merged: 8/21/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8220-dangling-step-id-and-or-fallback

📝 Commits (5)

f481ca5 fix(workflow-pins): stop the step-output reader failing compliant spellings (BE-8130)
61cbc05 fix(workflow-pins): report dangling step-output refs and read the || fallback spelling (BE-8220)
b68df60 fix(workflow-pins): keep the dangling check to real checkouts, and judge any leading operand (BE-8220)
1c66f7f fix(workflow-pins): keep the scope gate ahead of operand order, and read a with: the YAML nests differently (BE-8220)
793bfae Merge branch main into matt/be-8220-dangling-step-id-and-or-fallback

📊 Changes

3 files changed (+852 additions, -62 deletions)

View changed files

📝 .github/workflow-pins/README.md (+51 -3)
📝 .github/workflow-pins/check_workflow_pins.py (+417 -41)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+384 -18)

📄 Description

STACKED — merging lands on matt/be-8130-step-output-reader-spellings (owned by #197's author, mattmillerai), NOT main. GitHub retargets this PR to main automatically when #197 merges; do not merge it ahead of #197.

ELI-5

check_workflow_pins.py follows a ref: through a step output (BE-8130, #195/#197) so the resolve-then-consume checkout idiom is linted. But two spellings still fell out of the lint entirely, and both were verified empirically by the BE-8215 spike: a ref: naming a step id that does not exist before the checkout (a typo, a step in another job, or a resolver declared below its consumer) was silently dropped — even though at runtime that expression is '' and actions/checkout reads '' as the default branch, running the checkout unconditionally at a mutable ref — and ref: ${{ steps.x.outputs.ref || 'main' }} matched none of the step-output patterns, recording nothing. This closes both drops: the first becomes an error in its own right with its own message, and the second is read and judged by operand order exactly as the input side already is.

What changed

Per-job step-id pre-scan (_job_step_ids). The walk-order resolvers map cannot tell "an earlier step this lint has no claim on" from "no such step at all", and the two demand opposite answers. The pre-scan collects every step id in the job — block form with the same - id: list-marker normalization _binding_step_id makes, plus a flow-form pattern so - {id: x, …} steps are seen — because an id the scan misses becomes a false FAILURE under the new check, while over-collection merely reproduces the old fail-open drop for that one site.
The early return in _record_steps_output is split. An id that exists and is declared before the consuming step stays out of scope (an unrelated git rev-parse, a release lookup). An id that is absent, in another job, or declared at/after the consumer is recorded unguarded with via_step_output='dangling' — whose truthiness keeps unguarded_ref_checkouts unchanged. The boundary is the consuming STEP's first line, not the ref: line, so a step's own id: cannot excuse a ref consuming it (its output does not exist during its own with: evaluation).
Only a real with: INPUT is judged (_is_ref_input). The walk asks every line of the job about step-output refs, so ref: keys that are not action inputs reach the same code path: a job-level outputs: mapping (which conventionally sits above steps:, and therefore above every step id) and a ref: line inside a run: heredoc. Both used to be dropped harmlessly; under the dangling check they instead hard-failed a compliant workflow with an error naming a mutable checkout ref where there is no checkout at all. The gate is the one thing every real site has and neither of those does — a checkout's ref: is an entry of its step's with: mapping — restoring the old drop for the rest.
A distinct error message for the dangling case in check_dir, referencing BE-8215, naming the actual id and output rather than <id>/<out> placeholders. The BE-8130 remedies are deliberately not reused — neither fixes a nonexistent id; the message says to fix the id or move the resolving step above the checkout, and a test asserts the BE-8130 remedy text is absent from it.
_STEPS_OUTPUT_BODY reads || continuations, before and after the output name, keeping the named id/out groups and #197's _outside_quotes decoy loop. The block and continuation bodies are bounded by [^}]; [^,}] is the FLOW form's entry boundary and is kept only there, since carrying it into the other two left a fallback containing a comma (|| 'a,b') matching nothing at all. [^}] still cannot cross a }}, so a match can never span two interpolations.
Operand order is judged like the input side's _leading_operand_reaches_input. When the expression STARTS with the step output, the site is covered by the usual routes (under the exact if: the fallback arm is unreachable dead code, so steps.x.outputs.ref || 'main' passes). Otherwise it is a 'non-leading' site with its own message: 'main' || steps.x.outputs.ref resolves to the literal on every runner, and A && steps.x.outputs.ref resolves to the output or to '', so no if: and no resolver change can cover either — the message is worded for any leading operator, not just ||. A leading operand || falls through (false, '', "", 0, null) still reaches the output and is judged normally, rather than being failed with an error no edit can satisfy. Judged from the lead group of the same match rather than the line's first interpolation, so a quoted decoy sibling cannot skew the verdict, and judged before step existence, since a leading operand that wins makes "no such step" a red herring.
Tests: the two #197-pinned vacuous-pass tests are flipped to assert the site is now REPORTED (below-consumer and cross-job); new tests cover the typo'd id, the dangling error text and its interpolated id, the self-id edge, both operand orders in block, flow, and ref: >- continuation forms, a flow-style - {id: …} resolver's consumer not being falsely reported dangling, the job-level outputs: and run:-heredoc non-checkouts, a comma-bearing fallback, every falsey leading operand, and the && wording. The earlier-unrelated-step and repo-real-workflows tests still pass unchanged.
README: documents that a dangling id is an error in its own right, that || fallbacks are read here with leading-operand judgment, which ref: lines are in scope at all — and keeps (narrowed to the BINDING precondition) the warning that an unrecognized resolver is silent, plus the ref spellings this reader still does not recognize.

Verification

python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' — 139 passed, 0 failed (123 on the #197 base).
The other three suites are unaffected and green: cursor-review 194, groom 365, agents-md-integrity 46. shellcheck -x clean.
python3 .github/workflow-pins/check_workflow_pins.py — green on the repo's real workflows: 10 workflows, every ref checkout guarded, the 16-site count unchanged, 0 exempt.
The four spike repro cases each produce exactly one error (typo'd id, below-consumer, cross-job, 'main' || leading), and steps.x.outputs.ref || 'main' under the exact if: produces none — each pinned by a test.
Eleven mutation checks across both rounds — reverting the dangling report, the body widening, the operand-order judgment, the flow-form id pattern, the consumer-start boundary, the with: gate, the [^}] widening, the falsey-lead judgment, the non-leading state, the id interpolation, and the operator-agnostic wording — each fails exactly the tests that name it and nothing else.

Review round

The panel's High finding was real and reproduced before fixing: a compliant job declaring outputs: {ref: ${{ steps.pick.outputs.ref }}} hard-failed with the dangling error, on a workflow containing no checkout. Fixed by _is_ref_input above, gating all of _record_steps_output rather than only the dangling verdict — _consuming_step_bounds is not None (the suggested gate) does not separate the two over-matches, since a heredoc line resolves its - list item perfectly well. Also fixed from the panel: the unactionable message on a non-leading operand, the falsey leading operand, the block form's inherited [^,}] bound, the placeholder-only dangling text, and the over-deleted README warning.

Not fixed here (pre-existing or deferred, with follow-ups filed)

The two BE-8130 residuals #197 lists are untouched and still open under BE-8130: the resolver is registered only on a literal _GUARD_BINDING_RE binding match, and a hard-guard resolver's consumer exemption never proves the value written to $GITHUB_OUTPUT is non-empty. The panel raised the second again in its new combination (steps.r.outputs.ref || 'main' passing via a hard-guard resolver that writes a differently-named output); the outcome there is unchanged from before this branch — that spelling previously matched nothing and passed silently — so nothing regressed, and the fix belongs with the rest of that residual.
Unrecognized ref expressions still record no site — a fallback containing a brace (|| format('refs/heads/{0}', 'main')), a parenthesized operand, && rather than ||, and any line holding more than one interpolation or more than one step-output operand (only one match is judged). Documented in the README; a follow-up proposes a two-tier reader so an unknown spelling fails LOUD rather than vanishing. Widening the regex further inside this PR is the wrong trade — a bad widening turns a silent miss into a false CI failure in every consumer repo, which this PR already had to fix once.
The step-id pre-scan over-collects — an id: under with:, or echo "id: x" in a run:, registers as a declared step and silences the dangling check. Deliberate here (an id the scan MISSES is a false failure; one it over-collects is the old drop), with a follow-up to scope it to the step key column paired with a fail-loud escape.

Refs BE-8220 — kept open rather than closed only because this body names unfixed (BE-8130-tracked) residuals in the same file the ticket names; BE-8220's own two reported drops are both fixed and verified, so it can be closed once a human confirms the residuals stay BE-8130's.

Unexercised artifact: the BE-8215 spike ticket itself (unreachable from this sandbox) — its verified findings are reproduced in BE-8220's body, which is what this change was built and verified against. The precondition on PR #197 was handled by stacking on its branch rather than waiting for it to merge.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 139 passed, 0 failed; cursor-review 194, groom 365, agents-md-integrity 46, all green; shellcheck -x clean; python3 .github/workflow-pins/check_workflow_pins.py: green (10 workflows, every ref checkout guarded, 16 sites, 0 exempt); 11 mutation checks, each failing exactly the tests that name it
Deviations: branched from #197's head (stacked PR) instead of the ticket's "branch from main AFTER #197 merges" — standing stack-don't-wait policy; the flipped assertion the ticket describes for the post-merge case is handled identically on the stack. The dangling error message originally used generic steps.<id>.outputs.<out> placeholders (as the ticket's own wording does); the review round replaced them with the actual id and output. Two panel findings were deferred to follow-ups rather than fixed here (unrecognized ref expressions; step-id pre-scan scope), both because widening this reader carries a false-CI-failure risk across every consumer repo that a separate change can prove out.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/202 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/20/2026 **Status:** ✅ Merged **Merged:** 8/21/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8220-dangling-step-id-and-or-fallback` --- ### 📝 Commits (5) - [`f481ca5`](https://github.com/Comfy-Org/github-workflows/commit/f481ca5d7bc5756cacac475c25ed75ddda4ed847) fix(workflow-pins): stop the step-output reader failing compliant spellings (BE-8130) - [`61cbc05`](https://github.com/Comfy-Org/github-workflows/commit/61cbc05fd4bd10fa5b12d1dc63177128bc1293fd) fix(workflow-pins): report dangling step-output refs and read the `||` fallback spelling (BE-8220) - [`b68df60`](https://github.com/Comfy-Org/github-workflows/commit/b68df60c415a332ba73fb8f4fe819a621298e0fa) fix(workflow-pins): keep the dangling check to real checkouts, and judge any leading operand (BE-8220) - [`1c66f7f`](https://github.com/Comfy-Org/github-workflows/commit/1c66f7f9ecb4406d0fbfd1e13f31fb882e9e95b4) fix(workflow-pins): keep the scope gate ahead of operand order, and read a `with:` the YAML nests differently (BE-8220) - [`793bfae`](https://github.com/Comfy-Org/github-workflows/commit/793bfae3d8bcbe9f1381d4d30826857d1a6cac53) Merge branch main into matt/be-8220-dangling-step-id-and-or-fallback ### 📊 Changes **3 files changed** (+852 additions, -62 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+51 -3) 📝 `.github/workflow-pins/check_workflow_pins.py` (+417 -41) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+384 -18) </details> ### 📄 Description **STACKED — merging lands on `matt/be-8130-step-output-reader-spellings` (owned by #197's author, mattmillerai), NOT `main`.** GitHub retargets this PR to `main` automatically when #197 merges; do not merge it ahead of #197. ## ELI-5 `check_workflow_pins.py` follows a `ref:` through a step output (BE-8130, #195/#197) so the resolve-then-consume checkout idiom is linted. But two spellings still fell out of the lint entirely, and both were verified empirically by the BE-8215 spike: a `ref:` naming a step id that does not exist before the checkout (a typo, a step in another job, or a resolver declared below its consumer) was silently dropped — even though at runtime that expression is `''` and `actions/checkout` reads `''` as the default branch, running the checkout unconditionally at a mutable ref — and `ref: ${{ steps.x.outputs.ref || 'main' }}` matched none of the step-output patterns, recording nothing. This closes both drops: the first becomes an error in its own right with its own message, and the second is read and judged by operand order exactly as the input side already is. ## What changed - **Per-job step-id pre-scan (`_job_step_ids`).** The walk-order `resolvers` map cannot tell "an earlier step this lint has no claim on" from "no such step at all", and the two demand opposite answers. The pre-scan collects every step id in the job — block form with the same `- id:` list-marker normalization `_binding_step_id` makes, plus a flow-form pattern so `- {id: x, …}` steps are seen — because an id the scan misses becomes a false FAILURE under the new check, while over-collection merely reproduces the old fail-open drop for that one site. - **The early return in `_record_steps_output` is split.** An id that exists and is declared before the consuming step stays out of scope (an unrelated `git rev-parse`, a release lookup). An id that is absent, in another job, or declared at/after the consumer is recorded unguarded with `via_step_output='dangling'` — whose truthiness keeps `unguarded_ref_checkouts` unchanged. The boundary is the consuming STEP's first line, not the `ref:` line, so a step's own `id:` cannot excuse a ref consuming it (its output does not exist during its own `with:` evaluation). - **Only a real `with:` INPUT is judged (`_is_ref_input`).** The walk asks every line of the job about step-output refs, so `ref:` keys that are not action inputs reach the same code path: a job-level `outputs:` mapping (which conventionally sits above `steps:`, and therefore above every step id) and a `ref:` line inside a `run:` heredoc. Both used to be dropped harmlessly; under the dangling check they instead hard-failed a compliant workflow with an error naming a mutable checkout ref where there is no checkout at all. The gate is the one thing every real site has and neither of those does — a checkout's `ref:` is an entry of its step's `with:` mapping — restoring the old drop for the rest. - **A distinct error message for the dangling case** in `check_dir`, referencing BE-8215, naming the actual id and output rather than `<id>`/`<out>` placeholders. The BE-8130 remedies are deliberately not reused — neither fixes a nonexistent id; the message says to fix the id or move the resolving step above the checkout, and a test asserts the BE-8130 remedy text is absent from it. - **`_STEPS_OUTPUT_BODY` reads `||` continuations**, before and after the output name, keeping the named `id`/`out` groups and #197's `_outside_quotes` decoy loop. The block and continuation bodies are bounded by `[^}]`; `[^,}]` is the FLOW form's entry boundary and is kept only there, since carrying it into the other two left a fallback containing a comma (`|| 'a,b'`) matching nothing at all. `[^}]` still cannot cross a `}}`, so a match can never span two interpolations. - **Operand order is judged like the input side's `_leading_operand_reaches_input`.** When the expression STARTS with the step output, the site is covered by the usual routes (under the exact `if:` the fallback arm is unreachable dead code, so `steps.x.outputs.ref || 'main'` passes). Otherwise it is a `'non-leading'` site with its own message: `'main' || steps.x.outputs.ref` resolves to the literal on every runner, and `A && steps.x.outputs.ref` resolves to the output or to `''`, so no `if:` and no resolver change can cover either — the message is worded for any leading operator, not just `||`. A leading operand `||` falls *through* (`false`, `''`, `""`, `0`, `null`) still reaches the output and is judged normally, rather than being failed with an error no edit can satisfy. Judged from the `lead` group of the same match rather than the line's first interpolation, so a quoted decoy sibling cannot skew the verdict, and judged before step existence, since a leading operand that wins makes "no such step" a red herring. - **Tests**: the two #197-pinned vacuous-pass tests are flipped to assert the site is now REPORTED (below-consumer and cross-job); new tests cover the typo'd id, the dangling error text and its interpolated id, the self-id edge, both operand orders in block, flow, and `ref: >-` continuation forms, a flow-style `- {id: …}` resolver's consumer not being falsely reported dangling, the job-level `outputs:` and `run:`-heredoc non-checkouts, a comma-bearing fallback, every falsey leading operand, and the `&&` wording. The earlier-unrelated-step and repo-real-workflows tests still pass unchanged. - **README**: documents that a dangling id is an error in its own right, that `||` fallbacks are read here with leading-operand judgment, which `ref:` lines are in scope at all — and keeps (narrowed to the BINDING precondition) the warning that an unrecognized resolver is silent, plus the ref spellings this reader still does not recognize. ## Verification - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` — **139 passed, 0 failed** (123 on the #197 base). - The other three suites are unaffected and green: cursor-review 194, groom 365, agents-md-integrity 46. `shellcheck -x` clean. - `python3 .github/workflow-pins/check_workflow_pins.py` — green on the repo's real workflows: 10 workflows, every ref checkout guarded, the 16-site count unchanged, 0 exempt. - The four spike repro cases each produce exactly one error (typo'd id, below-consumer, cross-job, `'main' ||` leading), and `steps.x.outputs.ref || 'main'` under the exact `if:` produces none — each pinned by a test. - **Eleven mutation checks** across both rounds — reverting the dangling report, the body widening, the operand-order judgment, the flow-form id pattern, the consumer-start boundary, the `with:` gate, the `[^}]` widening, the falsey-lead judgment, the non-leading state, the id interpolation, and the operator-agnostic wording — each fails exactly the tests that name it and nothing else. ## Review round The panel's High finding was real and reproduced before fixing: a compliant job declaring `outputs: {ref: ${{ steps.pick.outputs.ref }}}` hard-failed with the dangling error, on a workflow containing no checkout. Fixed by `_is_ref_input` above, gating all of `_record_steps_output` rather than only the dangling verdict — `_consuming_step_bounds is not None` (the suggested gate) does not separate the two over-matches, since a heredoc line resolves its `- ` list item perfectly well. Also fixed from the panel: the unactionable message on a non-leading operand, the falsey leading operand, the block form's inherited `[^,}]` bound, the placeholder-only dangling text, and the over-deleted README warning. ## Not fixed here (pre-existing or deferred, with follow-ups filed) - The two BE-8130 residuals #197 lists are untouched and still open under BE-8130: the resolver is registered only on a literal `_GUARD_BINDING_RE` binding match, and a hard-guard resolver's consumer exemption never proves the value written to `$GITHUB_OUTPUT` is non-empty. The panel raised the second again in its new combination (`steps.r.outputs.ref || 'main'` passing via a hard-guard resolver that writes a differently-named output); the outcome there is unchanged from before this branch — that spelling previously matched nothing and passed silently — so nothing regressed, and the fix belongs with the rest of that residual. - **Unrecognized ref expressions still record no site** — a fallback containing a brace (`|| format('refs/heads/{0}', 'main')`), a parenthesized operand, `&&` rather than `||`, and any line holding more than one interpolation or more than one step-output operand (only one match is judged). Documented in the README; a follow-up proposes a two-tier reader so an unknown spelling fails LOUD rather than vanishing. Widening the regex further inside this PR is the wrong trade — a bad widening turns a silent miss into a false CI failure in every consumer repo, which this PR already had to fix once. - **The step-id pre-scan over-collects** — an `id:` under `with:`, or `echo "id: x"` in a `run:`, registers as a declared step and silences the dangling check. Deliberate here (an id the scan MISSES is a false failure; one it over-collects is the old drop), with a follow-up to scope it to the step key column paired with a fail-loud escape. Refs BE-8220 — kept open rather than closed only because this body names unfixed (BE-8130-tracked) residuals in the same file the ticket names; BE-8220's own two reported drops are both fixed and verified, so it can be closed once a human confirms the residuals stay BE-8130's. Unexercised artifact: the BE-8215 spike ticket itself (unreachable from this sandbox) — its verified findings are reproduced in BE-8220's body, which is what this change was built and verified against. The precondition on PR #197 was handled by stacking on its branch rather than waiting for it to merge. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'`: 139 passed, 0 failed; cursor-review 194, groom 365, agents-md-integrity 46, all green; `shellcheck -x` clean; `python3 .github/workflow-pins/check_workflow_pins.py`: green (10 workflows, every ref checkout guarded, 16 sites, 0 exempt); 11 mutation checks, each failing exactly the tests that name it - **Deviations:** branched from #197's head (stacked PR) instead of the ticket's "branch from main AFTER #197 merges" — standing stack-don't-wait policy; the flipped assertion the ticket describes for the post-merge case is handled identically on the stack. The dangling error message originally used generic `steps.<id>.outputs.<out>` placeholders (as the ticket's own wording does); the review round replaced them with the actual id and output. Two panel findings were deferred to follow-ups rather than fixed here (unrecognized ref expressions; step-id pre-scan scope), both because widening this reader carries a false-CI-failure risk across every consumer repo that a separate change can prove out. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:23 +00:00