[PR #203] [MERGED] fix(workflow-pins): fail loud on a `ref:` expression the step-output reader cannot parse (BE-8253) #211

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-8253-workflow-pins-unparsed-ref

📝 Commits (9)

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)
91ab7fb fix(workflow-pins): fail loud on a ref: expression the step-output reader cannot parse (BE-8253)
f438b1b Merge base into matt/be-8253-workflow-pins-unparsed-ref, resolving semantic conflict
d446fc9 Merge origin/main into matt/be-8253-workflow-pins-unparsed-ref
977bf7a Merge origin/main into matt/be-8253-workflow-pins-unparsed-ref
9c9cf9e Merge origin/main into matt/be-8253-workflow-pins-unparsed-ref

📊 Changes

3 files changed (+727 additions, -154 deletions)

View changed files

📝 .github/workflow-pins/README.md (+32 -8)
📝 .github/workflow-pins/check_workflow_pins.py (+401 -146)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+294 -0)

📄 Description

Update: #202 (this PR's former base) has merged to main; GitHub retargeted this PR to main directly, so it is no longer stacked — the banner that used to be here is stale and removed. This branch was then merged with the current main tip to pick up #202/#197/#200/#196; see the Provenance section below for what that merge verified.

ELI-5

The workflow-pins lint follows a checkout's ref: back to the step that produced it, so a job that must never fail still can't silently check out mutable scripts. It read that ref: with one regex and gave one answer per line — so any spelling it couldn't chew recorded no site at all and the file passed. Now it asks two questions: "does this ref: mention a step output at all?" and "how many of those could I actually read?" If the first number is bigger than the second, the lint says so out loud instead of shrugging. And where a ref: names several step outputs, every one of them is judged, not just the first the regex happened to land on.

What was wrong

ref_checkouts's else-arm dropped any line _steps_output_match couldn't match. Five shapes escaped, all confirmed fail-open through the real CLI (see Evidence):

ref: ${{ steps.x.outputs.ref || format('refs/heads/{0}', 'main') }} — the } in {0} breaks the [^}]*? fallback stretch.
ref: ${{ (steps.x.outputs.ref) || 'main' }} — ) after the output name matches neither || nor }}.
ref: ${{ steps.x.outputs.ref && 'main' }} — only || was accepted. Runtime is wrong in both arms: truthy checks out at the mutable literal, falsey resolves to '' and actions/checkout reads '' as the default branch.
ref: "${{ steps.a.outputs.ref }}${{ steps.b.outputs.ref }}" — the block pattern's greedy .* landed on the LAST interpolation; a was never judged even though both feed the value.
ref: ${{ steps.a.outputs.ref || steps.b.outputs.ref }} — the fallback stretch swallowed b; a covered a passed the whole site.

What changed

Two-tier reader. LOOSE counts steps.<id>.outputs.<out> mentions in the comment-stripped ref: value (so ref: main # was ${{ steps.x.outputs.ref }} cannot fire it). STRICT runs the existing patterns over every interpolation and every top-level || operand, yielding a list rather than one match. Loose > strict records the site with via_step_output = 'unparsed' — truthy, so unguarded_ref_checkouts needs no change, the same trick 'dangling'/'non-leading' already use — and check_dir prints a message naming the two spellings the lint can judge. Widening the strict reader to evaluate format() / && / parenthesized expressions is deliberately not the fix: an unknown spelling asks its author for a supported one rather than growing an expression parser inside the lint.

All operands judged, strongest requirement wins. _record_steps_output takes the list; each (id, out) must independently pass (declared as an earlier step, covered by a fail-closed resolver or the exact non-empty if:, reachable by operand order). A covered sibling excuses nothing. 'non-leading' outranks 'dangling' for the reason it is judged first: no change to the id or the step order fixes operand order.

_lead_reaches_output now falls through a leading step output. An unresolved output is '', which is falsey, so A || B really can resolve to B. That second operand only became visible with the exhaustive reader, and it is judged in its own right — so nothing is excused by falling through. (Chesterton's-fence note: the check was written in #202 when only one step output per expression was ever visible, so this case could not arise; the falsey-literal behaviour and every _FALSEY_OPERANDS answer are unchanged, pinned by test_a_falsey_leading_operand_still_reaches_the_output.)

Gated by _is_ref_input, exactly as the other states are — a job-level outputs: mapping or a run: heredoc holding an unparseable expression is not a checkout. That is the false-CI-failure channel BE-8215 already had to close once; mutation-checking confirms reordering the gate breaks a named test.

Flow entry boundary is interpolation-aware. The naive [^,}] bound cut a value mid-expression and hid the rest from both tiers. ${{ … }} is atomic now; outside one, the first ,/} still ends the entry, so a sibling entry still cannot be misread as the ref (pinned by the pre-existing decoy tests — reverting it fails four of them).

Applied to all three spellings (block, flow, continuation under ref: >- / ref: |).

Evidence — the refusal path is empirically justified, not assumed

Before shipping a diff whose user-facing outcome is "this lint refuses your expression", I ran each shape through both CLIs (base = #202 head b68df60, new = this branch), against a fixture workflow with a real resolver and checkout:

`ref:` value	base exit	this branch
`${{ steps.resolve_ref.outputs.ref }}` (bare, uncovered)	1 (correctly fails)	1
`${{ steps.resolve_ref.outputs.ref \|\| format('refs/heads/{0}', 'main') }}`	0 — passed	1
`${{ (steps.resolve_ref.outputs.ref) \|\| 'main' }}`	0 — passed	1
`${{ steps.resolve_ref.outputs.ref && 'main' }}`	0 — passed	1
`"${{ steps.soft.outputs.ref }}${{ steps.resolve_ref.outputs.ref }}"` (first operand uncovered)	0 — passed	1
`${{ steps.resolve_ref.outputs.ref \|\| steps.soft.outputs.ref }}` (second operand uncovered)	0 — passed	1

So the capability being refused genuinely does not exist on any path: these expressions were never judged and approved, they were never read. There is no second lint in this repo covering ref: guards — pinact/zizmor check uses: pins, not this. The refusal redirects to the two spellings that are judged, both of which I verified still pass unchanged.

I also read the two review threads the work is built on (r3826102157, r3826102163) — public and fetchable — and they name exactly these shapes.

Tests

15 new tests (154 total in this suite, up from 139), each mutation-checked: revert the branch, a named test fails.

Mutation	Tests that fail
never return `'unparsed'`	7, incl. the three refused spellings + the error text
strict tier stops at first match	4, incl. both concatenated and both `\|\|` operands
step output does not fall through a lead	3
`'unparsed'` recorded before the `_is_ref_input` gate	1 (`…outside_a_with_input_is_not_a_checkout`)
loose tier on the un-stripped line	1 (`…commented_out_step_output_is_not_a_ref_use`)
`any(verdicts)` instead of `all(...)`	2
name the first operand, not the offending one	1
naive `[^,}]` flow entry bound	4 pre-existing tests
no dedicated `'unparsed'` message	1

Judgment calls

Error-message naming with several operands. 'non-leading' is answerable from the line alone, so it still names the exact operand. 'dangling' is a property of the job, not the line, so a multi-output value falls back to the <id>/<out> placeholders rather than pointing the author at a sibling that is fine — the same fallback the ref: >- spelling already takes. Pinned by test_a_multi_operand_error_does_not_name_the_wrong_step. Threading the offending operand through the found tuple would have changed its shape and rippled through #202's tests; not worth it for a shape that has no instance in the tree.
Two deliberate behaviour changes beyond the five shapes, both in the fail-loud direction, both tested: (a) a flow-form fallback containing a comma (with: {ref: "${{ … || 'a,b' }}"}) is now 'unparsed' rather than silently dropped — [^,}] is the flow form's real entry boundary, so that spelling genuinely cannot be read there (the block form reads it, unchanged); (b) a continuation value with a literal prefix (ref: >- then refs/heads/${{ … }}) is now a judged site, matching what the block form always did. Neither occurs in this repo's tree.
steps_output_ref is no longer called inside the module — check_dir goes through the new _reported_step_output. It stays as the module's line-level accessor and is still exercised by #202's decoy tests; deleting it would lose that coverage.

Verification

python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' — 154 passed, 0 failed (139 before).
python3 .github/workflow-pins/check_workflow_pins.py — exit 0, OK — 10 workflow(s) declare workflows_ref, none with a default, every ref checkout guarded (0 exempt). Identical to the baseline on the #202 head. The tree's only real step-output site is cursor-review.yml:456 (bare spelling); zero new failures there.
Unaffected suites re-run for safety: cursor-review 194 passed, agents-md-integrity 46 passed, groom 365 passed (1 skipped), bump-callers 424 passed + shellcheck -x clean.

Unexercised artifacts

The spike ticket this work descends from, and its findings comment, are on Linear and unreachable from this environment. The design was reconstructed from the two public GitHub review threads above, which I did read and which carry the same five shapes — but I could not confirm the spike's comment adds nothing beyond them.

Out of scope

Teaching the strict reader to actually evaluate format() / && / parenthesized expressions. That is the point of the two-tier design, not a gap in it.

Refs rather than a closing keyword: the driving ticket's own evidence artifact went unexercised (above), so the ticket stays open for a human to confirm nothing in it was missed.

Provenance

Authored by: agent-work loop
Verified: merged current main (b92f1df, which squash-landed #202/#197/#200/#196) into this branch to clear a CONFLICTING→CLEAN merge state; the conflicts were git failing to recognize content already combined by a prior merge across a squash-merge (identical tree, different commit hash) — resolved by keeping that already-combined content, verified both BE-8220's and BE-8253's added symbols are present. Post-merge: workflow-pins unittest 160 passed / 0 failed; check_workflow_pins.py on the real tree exit 0, 10 workflows, 0 unguarded; pr-size go tests pass except one pre-existing, environment-dependent failure (TestResolveMergeBaseWithAdvancedBaseBranch) reproduced identically on an unmodified main checkout, unrelated to this change; base-vs-branch CLI comparison on 6 fixture ref: spellings (table above)
Deviations: none — every acceptance criterion in the plan is met, including the mutation-check requirement on each new branch

_{🔄 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/203 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/21/2026 **Status:** ✅ Merged **Merged:** 8/21/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8253-workflow-pins-unparsed-ref` --- ### 📝 Commits (9) - [`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) - [`91ab7fb`](https://github.com/Comfy-Org/github-workflows/commit/91ab7fb5600fd1e9587a7b8df989c412351374ab) fix(workflow-pins): fail loud on a `ref:` expression the step-output reader cannot parse (BE-8253) - [`f438b1b`](https://github.com/Comfy-Org/github-workflows/commit/f438b1bd2974e1a71ff612caf7f7027c760fccc6) Merge base into matt/be-8253-workflow-pins-unparsed-ref, resolving semantic conflict - [`d446fc9`](https://github.com/Comfy-Org/github-workflows/commit/d446fc951f4512f3fc2fefc2118ccbeaa1054215) Merge origin/main into matt/be-8253-workflow-pins-unparsed-ref - [`977bf7a`](https://github.com/Comfy-Org/github-workflows/commit/977bf7aafa3f505463075a8d400fc0da03c6d239) Merge origin/main into matt/be-8253-workflow-pins-unparsed-ref - [`9c9cf9e`](https://github.com/Comfy-Org/github-workflows/commit/9c9cf9e607ce6dd1d5a0a3b6f4f37592fc5b0643) Merge origin/main into matt/be-8253-workflow-pins-unparsed-ref ### 📊 Changes **3 files changed** (+727 additions, -154 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+32 -8) 📝 `.github/workflow-pins/check_workflow_pins.py` (+401 -146) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+294 -0) </details> ### 📄 Description _Update: #202 (this PR's former base) has merged to `main`; GitHub retargeted this PR to `main` directly, so it is no longer stacked — the banner that used to be here is stale and removed. This branch was then merged with the current `main` tip to pick up #202/#197/#200/#196; see the Provenance section below for what that merge verified._ ## ELI-5 The workflow-pins lint follows a checkout's `ref:` back to the step that produced it, so a job that must never fail still can't silently check out mutable scripts. It read that `ref:` with one regex and gave one answer per line — so any spelling it couldn't chew recorded *no site at all* and the file **passed**. Now it asks two questions: "does this `ref:` mention a step output at all?" and "how many of those could I actually read?" If the first number is bigger than the second, the lint says so out loud instead of shrugging. And where a `ref:` names several step outputs, every one of them is judged, not just the first the regex happened to land on. ## What was wrong `ref_checkouts`'s else-arm dropped any line `_steps_output_match` couldn't match. Five shapes escaped, all confirmed **fail-open through the real CLI** (see Evidence): 1. `ref: ${{ steps.x.outputs.ref || format('refs/heads/{0}', 'main') }}` — the `}` in `{0}` breaks the `[^}]*?` fallback stretch. 2. `ref: ${{ (steps.x.outputs.ref) || 'main' }}` — `)` after the output name matches neither `||` nor `}}`. 3. `ref: ${{ steps.x.outputs.ref && 'main' }}` — only `||` was accepted. Runtime is wrong in *both* arms: truthy checks out at the mutable literal, falsey resolves to `''` and `actions/checkout` reads `''` as the default branch. 4. `ref: "${{ steps.a.outputs.ref }}${{ steps.b.outputs.ref }}"` — the block pattern's greedy `.*` landed on the LAST interpolation; `a` was never judged even though both feed the value. 5. `ref: ${{ steps.a.outputs.ref || steps.b.outputs.ref }}` — the fallback stretch swallowed `b`; a covered `a` passed the whole site. ## What changed **Two-tier reader.** LOOSE counts `steps.<id>.outputs.<out>` mentions in the *comment-stripped* `ref:` value (so `ref: main # was ${{ steps.x.outputs.ref }}` cannot fire it). STRICT runs the existing patterns over **every interpolation and every top-level `||` operand**, yielding a list rather than one match. Loose > strict records the site with `via_step_output = 'unparsed'` — truthy, so `unguarded_ref_checkouts` needs no change, the same trick `'dangling'`/`'non-leading'` already use — and `check_dir` prints a message naming the two spellings the lint *can* judge. Widening the strict reader to evaluate `format()` / `&&` / parenthesized expressions is deliberately **not** the fix: an unknown spelling asks its author for a supported one rather than growing an expression parser inside the lint. **All operands judged, strongest requirement wins.** `_record_steps_output` takes the list; each `(id, out)` must independently pass (declared as an earlier step, covered by a fail-closed resolver or the exact non-empty `if:`, reachable by operand order). A covered sibling excuses nothing. `'non-leading'` outranks `'dangling'` for the reason it is judged first: no change to the id or the step order fixes operand order. **`_lead_reaches_output` now falls through a leading step output.** An unresolved output is `''`, which is falsey, so `A || B` really can resolve to `B`. That second operand only became visible with the exhaustive reader, and it is judged in its own right — so nothing is excused by falling through. (Chesterton's-fence note: the check was written in #202 when only one step output per expression was ever visible, so this case could not arise; the falsey-literal behaviour and every `_FALSEY_OPERANDS` answer are unchanged, pinned by `test_a_falsey_leading_operand_still_reaches_the_output`.) **Gated by `_is_ref_input`**, exactly as the other states are — a job-level `outputs:` mapping or a `run:` heredoc holding an unparseable expression is not a checkout. That is the false-CI-failure channel BE-8215 already had to close once; mutation-checking confirms reordering the gate breaks a named test. **Flow entry boundary is interpolation-aware.** The naive `[^,}]` bound cut a value mid-expression and hid the rest from both tiers. `${{ … }}` is atomic now; outside one, the first `,`/`}` still ends the entry, so a sibling entry still cannot be misread as the ref (pinned by the pre-existing decoy tests — reverting it fails four of them). Applied to all three spellings (block, flow, continuation under `ref: >-` / `ref: |`). ## Evidence — the refusal path is empirically justified, not assumed Before shipping a diff whose user-facing outcome is "this lint refuses your expression", I ran each shape through **both** CLIs (base = #202 head `b68df60`, new = this branch), against a fixture workflow with a real resolver and checkout: | `ref:` value | base exit | this branch | |---|---|---| | `${{ steps.resolve_ref.outputs.ref }}` (bare, uncovered) | **1** (correctly fails) | 1 | | `${{ steps.resolve_ref.outputs.ref \|\| format('refs/heads/{0}', 'main') }}` | **0 — passed** | 1 | | `${{ (steps.resolve_ref.outputs.ref) \|\| 'main' }}` | **0 — passed** | 1 | | `${{ steps.resolve_ref.outputs.ref && 'main' }}` | **0 — passed** | 1 | | `"${{ steps.soft.outputs.ref }}${{ steps.resolve_ref.outputs.ref }}"` (first operand uncovered) | **0 — passed** | 1 | | `${{ steps.resolve_ref.outputs.ref \|\| steps.soft.outputs.ref }}` (second operand uncovered) | **0 — passed** | 1 | So the capability being refused genuinely does not exist on any path: these expressions were never *judged and approved*, they were never read. There is no second lint in this repo covering `ref:` guards — `pinact`/`zizmor` check `uses:` pins, not this. The refusal redirects to the two spellings that *are* judged, both of which I verified still pass unchanged. I also read the two review threads the work is built on ([r3826102157](https://github.com/Comfy-Org/github-workflows/pull/202#discussion_r3826102157), [r3826102163](https://github.com/Comfy-Org/github-workflows/pull/202#discussion_r3826102163)) — public and fetchable — and they name exactly these shapes. ## Tests 15 new tests (154 total in this suite, up from 139), each **mutation-checked**: revert the branch, a named test fails. | Mutation | Tests that fail | |---|---| | never return `'unparsed'` | 7, incl. the three refused spellings + the error text | | strict tier stops at first match | 4, incl. both concatenated and both `\|\|` operands | | step output does not fall through a lead | 3 | | `'unparsed'` recorded before the `_is_ref_input` gate | 1 (`…outside_a_with_input_is_not_a_checkout`) | | loose tier on the un-stripped line | 1 (`…commented_out_step_output_is_not_a_ref_use`) | | `any(verdicts)` instead of `all(...)` | 2 | | name the first operand, not the offending one | 1 | | naive `[^,}]` flow entry bound | 4 **pre-existing** tests | | no dedicated `'unparsed'` message | 1 | ## Judgment calls - **Error-message naming with several operands.** `'non-leading'` is answerable from the line alone, so it still names the exact operand. `'dangling'` is a property of the *job*, not the line, so a multi-output value falls back to the `<id>`/`<out>` placeholders rather than pointing the author at a sibling that is fine — the same fallback the `ref: >-` spelling already takes. Pinned by `test_a_multi_operand_error_does_not_name_the_wrong_step`. Threading the offending operand through the `found` tuple would have changed its shape and rippled through #202's tests; not worth it for a shape that has no instance in the tree. - **Two deliberate behaviour changes beyond the five shapes**, both in the fail-loud direction, both tested: (a) a flow-form fallback containing a comma (`with: {ref: "${{ … || 'a,b' }}"}`) is now `'unparsed'` rather than silently dropped — `[^,}]` is the flow form's real entry boundary, so that spelling genuinely cannot be read there (the block form reads it, unchanged); (b) a continuation value with a literal prefix (`ref: >-` then `refs/heads/${{ … }}`) is now a judged site, matching what the block form always did. Neither occurs in this repo's tree. - **`steps_output_ref` is no longer called inside the module** — `check_dir` goes through the new `_reported_step_output`. It stays as the module's line-level accessor and is still exercised by #202's decoy tests; deleting it would lose that coverage. ## Verification - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` — **154 passed, 0 failed** (139 before). - `python3 .github/workflow-pins/check_workflow_pins.py` — exit 0, `OK — 10 workflow(s) declare workflows_ref, none with a default, every ref checkout guarded (0 exempt)`. Identical to the baseline on the #202 head. The tree's only real step-output site is `cursor-review.yml:456` (bare spelling); zero new failures there. - Unaffected suites re-run for safety: cursor-review 194 passed, agents-md-integrity 46 passed, groom 365 passed (1 skipped), bump-callers 424 passed + `shellcheck -x` clean. ## Unexercised artifacts - The spike ticket this work descends from, and its findings comment, are on Linear and unreachable from this environment. The design was reconstructed from the two public GitHub review threads above, which I *did* read and which carry the same five shapes — but I could not confirm the spike's comment adds nothing beyond them. ## Out of scope Teaching the strict reader to actually **evaluate** `format()` / `&&` / parenthesized expressions. That is the point of the two-tier design, not a gap in it. `Refs` rather than a closing keyword: the driving ticket's own evidence artifact went unexercised (above), so the ticket stays open for a human to confirm nothing in it was missed. ## Provenance - **Authored by:** agent-work loop - **Verified:** merged current `main` (`b92f1df`, which squash-landed #202/#197/#200/#196) into this branch to clear a `CONFLICTING`→`CLEAN` merge state; the conflicts were git failing to recognize content already combined by a prior merge across a squash-merge (identical tree, different commit hash) — resolved by keeping that already-combined content, verified both BE-8220's and BE-8253's added symbols are present. Post-merge: workflow-pins unittest 160 passed / 0 failed; `check_workflow_pins.py` on the real tree exit 0, 10 workflows, 0 unguarded; `pr-size` go tests pass except one pre-existing, environment-dependent failure (`TestResolveMergeBaseWithAdvancedBaseBranch`) reproduced identically on an unmodified `main` checkout, unrelated to this change; base-vs-branch CLI comparison on 6 fixture `ref:` spellings (table above) - **Deviations:** none — every acceptance criterion in the plan is met, including the mutation-check requirement on each new branch --- <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:25 +00:00