[PR #197] [MERGED] fix(workflow-pins): stop the step-output reader failing compliant `if:` spellings (BE-8130) #204

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-8130-step-output-reader-spellings

📝 Commits (1)

f481ca5 fix(workflow-pins): stop the step-output reader failing compliant spellings (BE-8130)

📊 Changes

3 files changed (+107 additions, -6 deletions)

View changed files

📝 .github/workflow-pins/README.md (+3 -2)
📝 .github/workflow-pins/check_workflow_pins.py (+30 -4)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+74 -0)

📄 Description

ELI-5

check_workflow_pins.py now follows a ref: through a step output (#195, BE-8130), so cursor-review.yml's never-fail ledger checkout is linted instead of invisible. The multi-model review panel on that PR found the reader is narrower than the shapes it has to read: three ways of writing a checkout that IS compliant were reported as violations, and one way of writing an evasive one slipped past. A lint that fails the remedy it just recommended is a lint people learn to route around, so this widens the reader — and only the reader. Every coverage rule is unchanged.

What changed

Four fixes, each mirroring a precedent already in this file rather than inventing a rule.

- if: on the list-marker line was never seen. _skips_on_empty_output scanned only lines at the step's key column, so a step written - if: steps.x.outputs.ref != '' had its guard ignored and a correctly guarded checkout was reported unguarded. Normalize the marker exactly as _binding_step_id already does to read an id: off it.
!= had to be surrounded by spaces. Actions accepts …outputs.ref!=''; the lint failed it. Spacing around != is now normalized to one space either side. Nothing else is normalized, so an OR-widened condition and an if: on a different output stay refused however they are spaced — both pinned by tests.
A quoted sibling could plant a decoy step output. _STEPS_OUTPUT_FLOW_RE.search took the leftmost [{,] entry boundary, and a comma inside a quoted scalar meets it: with: {path: "x, ref: ${{ steps.other.outputs.ref }}", ref: "${{ steps.resolve_ref.outputs.ref }}"} resolved to other, which holds no resolvers entry, so the real checkout was dropped out of coverage entirely. It now keeps looking past a match inside quotes, via the _outside_quotes helper _pins_to_job_workflow_sha already applies to its own flow match. That function stops at a quoted match; stopping here would lose the site, so this one iterates.
README claimed coverage whenever the producing step's env: "binds workflows_ref", which is wider than what the code detects. It now says "in one of the two literal spellings below" — the spellings the preconditions block already lists.

Two tests that passed vacuously now assert what they name:

test_a_resolver_below_its_consumer_is_never_credited filtered on guarded, which an empty site list satisfies, so it could not distinguish "reported as unguarded" from "dropped and never seen". It now pins the site list and documents that today the site is dropped, though at runtime that output is guaranteed '' and the checkout takes the default branch. Left as-is deliberately — see below.
_binding_step_id's own list-marker normalization had no test at all; deleting it kept the suite green. Found by mutation while checking the fix above.

Verification

python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' — 123 passed, 0 failed (118 on main).
python3 .github/workflow-pins/check_workflow_pins.py — green: 10 workflows, every ref checkout guarded, 0 exempt.
Both CI smoke tests run by hand: reintroduced default: → exit 1, unguarded checkout → exit 1.
Every new test mutation-checked. Reverting each fix individually fails exactly the test that names it and nothing else: list-marker if: → test_the_if_on_the_list_marker_line_is_seen; != spacing → test_a_tightly_spaced_if_passes; the decoy fix → test_a_quoted_sibling_cannot_plant_a_decoy_step_output; _binding_step_id's marker rewrite → test_the_id_on_the_list_marker_line_registers_the_resolver.

Deliberately NOT fixed here

The panel's other findings are coverage-rule changes, not reader changes, and each needs a judgment this PR does not make. They are recorded for follow-up rather than folded in:

The resolver is registered only on a literal _GUARD_BINDING_RE match (8/8 reviewers), so a step reaching the input any other way — a different env name, || 'main', a job-level env: — registers nothing and its consumer drops out of coverage. This is the "Known residual" #195 named when it landed as Refs BE-8130 rather than Closes; BE-8130 stays open for it. Fixing it fail-closed would demand an empty-ref if: of checkouts whose ref is a mutable branch by construction, which is the different judgment #195 declined to make.
An unresolvable <id> — a typo, another job, or declared below its consumer — is dropped rather than reported, though the runtime value is then guaranteed ''. Distinguishing it from the legitimately out-of-scope case (an earlier step that exists and never touches workflows_ref) needs a per-job pre-scan of step ids. The test above now makes the gap visible in the suite.
A hard-guard resolver exempts its consumer from carrying any if:, but is_guard_step proves only that the step rejects an empty input — never that the value it writes to $GITHUB_OUTPUT is non-empty. The BE-8130 error text actively recommends that route.
ref: ${{ steps.x.outputs.ref || 'main' }} matches none of the three step-output patterns and names no input either, so it records nothing — a mutable ref with no coverage.

Refs BE-8130

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 123 passed, 0 failed; python3 .github/workflow-pins/check_workflow_pins.py: green (10 workflows, every ref checkout guarded, 0 exempt); both CI smoke tests run by hand (reintroduced default → exit 1, unguarded checkout → exit 1); 4 mutation checks, one per new test, each failing exactly the test that names it
Deviations: none — this is a reader-only widening; every coverage rule is unchanged, and the four coverage-rule findings the panel raised are listed under "Deliberately NOT fixed here" rather than folded in

_{🔄 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/197 **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-8130-step-output-reader-spellings` --- ### 📝 Commits (1) - [`f481ca5`](https://github.com/Comfy-Org/github-workflows/commit/f481ca5d7bc5756cacac475c25ed75ddda4ed847) fix(workflow-pins): stop the step-output reader failing compliant spellings (BE-8130) ### 📊 Changes **3 files changed** (+107 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+3 -2) 📝 `.github/workflow-pins/check_workflow_pins.py` (+30 -4) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+74 -0) </details> ### 📄 Description ## ELI-5 `check_workflow_pins.py` now follows a `ref:` through a step output (#195, BE-8130), so `cursor-review.yml`'s never-fail ledger checkout is linted instead of invisible. The multi-model review panel on that PR found the *reader* is narrower than the shapes it has to read: three ways of writing a checkout that IS compliant were reported as violations, and one way of writing an evasive one slipped past. A lint that fails the remedy it just recommended is a lint people learn to route around, so this widens the reader — and only the reader. Every coverage rule is unchanged. ## What changed Four fixes, each mirroring a precedent already in this file rather than inventing a rule. - **`- if:` on the list-marker line was never seen.** `_skips_on_empty_output` scanned only lines at the step's key column, so a step written `- if: steps.x.outputs.ref != ''` had its guard ignored and a correctly guarded checkout was reported unguarded. Normalize the marker exactly as `_binding_step_id` already does to read an `id:` off it. - **`!=` had to be surrounded by spaces.** Actions accepts `…outputs.ref!=''`; the lint failed it. Spacing around `!=` is now normalized to one space either side. Nothing else is normalized, so an OR-widened condition and an `if:` on a different output stay refused however they are spaced — both pinned by tests. - **A quoted sibling could plant a decoy step output.** `_STEPS_OUTPUT_FLOW_RE.search` took the leftmost `[{,]` entry boundary, and a comma *inside a quoted scalar* meets it: `with: {path: "x, ref: ${{ steps.other.outputs.ref }}", ref: "${{ steps.resolve_ref.outputs.ref }}"}` resolved to `other`, which holds no `resolvers` entry, so the real checkout was dropped out of coverage entirely. It now keeps looking past a match inside quotes, via the `_outside_quotes` helper `_pins_to_job_workflow_sha` already applies to its own flow match. That function *stops* at a quoted match; stopping here would lose the site, so this one iterates. - **README** claimed coverage whenever the producing step's `env:` "binds `workflows_ref`", which is wider than what the code detects. It now says "in one of the two literal spellings below" — the spellings the preconditions block already lists. Two tests that passed vacuously now assert what they name: - `test_a_resolver_below_its_consumer_is_never_credited` filtered on `guarded`, which an empty site list satisfies, so it could not distinguish "reported as unguarded" from "dropped and never seen". It now pins the site list and documents that today the site is **dropped**, though at runtime that output is guaranteed `''` and the checkout takes the default branch. Left as-is deliberately — see below. - `_binding_step_id`'s own list-marker normalization had no test at all; deleting it kept the suite green. Found by mutation while checking the fix above. ## Verification - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` — **123 passed, 0 failed** (118 on `main`). - `python3 .github/workflow-pins/check_workflow_pins.py` — green: 10 workflows, every ref checkout guarded, 0 exempt. - Both CI smoke tests run by hand: reintroduced `default:` → exit 1, unguarded checkout → exit 1. - **Every new test mutation-checked.** Reverting each fix individually fails exactly the test that names it and nothing else: list-marker `if:` → `test_the_if_on_the_list_marker_line_is_seen`; `!=` spacing → `test_a_tightly_spaced_if_passes`; the decoy fix → `test_a_quoted_sibling_cannot_plant_a_decoy_step_output`; `_binding_step_id`'s marker rewrite → `test_the_id_on_the_list_marker_line_registers_the_resolver`. ## Deliberately NOT fixed here The panel's other findings are coverage-rule changes, not reader changes, and each needs a judgment this PR does not make. They are recorded for follow-up rather than folded in: - **The resolver is registered only on a literal `_GUARD_BINDING_RE` match** (8/8 reviewers), so a step reaching the input any other way — a different env name, `|| 'main'`, a job-level `env:` — registers nothing and its consumer drops out of coverage. This is the "Known residual" #195 named when it landed as `Refs BE-8130` rather than `Closes`; **BE-8130 stays open for it.** Fixing it fail-closed would demand an empty-ref `if:` of checkouts whose ref is a mutable branch by construction, which is the different judgment #195 declined to make. - **An unresolvable `<id>`** — a typo, another job, or declared below its consumer — **is dropped rather than reported**, though the runtime value is then guaranteed `''`. Distinguishing it from the legitimately out-of-scope case (an earlier step that exists and never touches `workflows_ref`) needs a per-job pre-scan of step ids. The test above now makes the gap visible in the suite. - **A hard-guard resolver exempts its consumer from carrying any `if:`**, but `is_guard_step` proves only that the step rejects an empty *input* — never that the value it writes to `$GITHUB_OUTPUT` is non-empty. The BE-8130 error text actively recommends that route. - **`ref: ${{ steps.x.outputs.ref || 'main' }}`** matches none of the three step-output patterns and names no input either, so it records nothing — a mutable ref with no coverage. Refs BE-8130 ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'`: 123 passed, 0 failed; `python3 .github/workflow-pins/check_workflow_pins.py`: green (10 workflows, every ref checkout guarded, 0 exempt); both CI smoke tests run by hand (reintroduced default → exit 1, unguarded checkout → exit 1); 4 mutation checks, one per new test, each failing exactly the test that names it - **Deviations:** none — this is a reader-only widening; every coverage rule is unchanged, and the four coverage-rule findings the panel raised are listed under "Deliberately NOT fixed here" rather than folded in --- <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:20 +00:00