Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #195] [MERGED] feat(workflow-pins): follow a `ref:` through a step output so the never-fail idiom is linted (BE-8130) #203

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-8130-workflow-pins-steps-output-ref

📝 Commits (2)

12eb051 feat(workflow-pins): follow a ref: through a step output (BE-8130)
a8017d8 fix(workflow-pins): accept a quoted if: scalar, pin the resolver's never-fail (BE-8130)

📊 Changes

3 files changed (+558 additions, -56 deletions)

View changed files

📝 .github/workflow-pins/README.md (+31 -0)
📝 .github/workflow-pins/check_workflow_pins.py (+279 -9)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+248 -47)

📄 Description

ELI-5

The pin lint asks every actions/checkout in this repo's reusable workflows: "are you checking out at workflows_ref, and if so, did something make sure that ref isn't empty first?" It answered that by reading the ref: line and looking for the words inputs.workflows_ref. cursor-review.yml's ledger job (a job that must never fail, so it can't run the usual fail-closed guard) resolves the ref one step earlier and writes ref: ${{ steps.resolve_ref.outputs.ref }} — no inputs.workflows_ref on the line, so the lint stopped seeing that checkout at all. This teaches the lint to follow the ref: back one step: if the producing step is the one holding workflows_ref, the checkout is in scope again, and it passes only when the resolver itself rejects an empty value or the consuming step carries exactly the if: … != '' that skips it.

What changed

ref_checkouts now walks a job carrying a small resolvers map alongside the existing guard flags: a step whose env: binds the input (_GUARD_BINDING_RE) and that carries an id: at its own key column is recorded, along with whether it is a fail-closed guard. A later ref: reading steps.<id>.outputs.<name> (block, flow, or ref: >- continuation) is then a ref use, and it is covered when either that resolver is a hard guard, or the consuming step carries exactly if: steps.<id>.outputs.<name> != '' — bare or ${{ … }}-wrapped, either spelling optionally double-quoted, nothing else. An OR-widened ... != '' || always() runs the checkout precisely when the output is empty, so it is refused; so is an if: on a different output, and a job-level if: (which skips the resolver too, so it can never tell the two states apart). A ref: whose producing step never touches workflows_ref is not this lint's subject and is left alone. Sites reached this way are recorded as non-fallback: the if: tests the resolved value, so guard strength (bare vs || job.workflow_sha) is moot on this path.

check_dir gained a third error message for the shape, because "copy the Require a pinned workflows_ref step in ahead of it" is advice a never-fail job cannot take — the missing piece there is the consumer's own if:. That required ref_checkouts to carry a fourth field (via_step_output); five existing test unpacks moved from for _, fb, _ to for _, fb, _, _.

cursor-review.yml is not touched by this PR — the fix is entirely in the lint, and the ledger job already satisfies it.

Verification

python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' — 118 passed, 0 failed (102 on main before this change; the ticket's "90" baseline predates two other merges to main).
python3 .github/workflow-pins/check_workflow_pins.py — green: 10 workflows checked, every ref checkout guarded.
Mutation checks, run by hand against the real cursor-review.yml (the ticket's acceptance): (1) delete the ledger consumer's if: line → check_workflow_pins.py itself exits 1 with the new BE-8130 annotation; (2) widen it to ... != '' || always() → exits 1 the same way; (3) restore → exits 0. The suite goes red for both mutations too. cursor-review.yml was restored byte-for-byte afterwards (git status clean for it).
Corpus sweep, including the half not being fixed: across all 45 workflow files in .github/workflows, there is exactly 1 ref: line reading a step output — the ledger checkout — and it is now covered. 0 step-output ref: sites are left outside coverage today. The population this lint deliberately does not claim (a ref: resolved from a step unrelated to workflows_ref) is currently empty here, which is why the new tests carry a synthetic git rev-parse fixture for it.
Site count across the four tracked workflows: 15 → 16, the ledger checkout re-entering coverage. The count is now taken from ref_checkouts rather than a per-line is_ref_use scan — a step-output site cannot be judged from a single line, since the answer depends on what the job walked past.

Judgment calls

resolvers[id] stores a bool, not the {"fallback": …, "is_guard": …} dict the plan sketched. The fallback half would have been written and never read: the plan's own next point makes guard strength irrelevant on this path, and a lint carrying a field nothing consults is a comment pretending to be data. The rationale is written where the value is recorded.
The retained ledger test is named test_the_ledger_resolver_carries_the_shape_check, mirroring its groom twin test_every_groom_guard_carries_the_shape_check, rather than the ..._rejects_rather_than_sanitizes the plan offered — the plan asked for a name consistent with the twin.
The flow form needed one extra helper. _step_bounds answers None at a step's own key column by design, which is exactly where with: {…, ref: …} puts the ref:. _consuming_step_bounds re-asks it from a copy of that line pushed one column deeper, rather than growing a second copy of the step-boundary logic that could drift from the first. Worst case it still returns None, and the site is then reported rather than credited.

Known residual — why this is `Refs`, not `Closes`

The resolver is recognized through _GUARD_BINDING_RE, which matches only WORKFLOWS_REF: ${{ inputs.workflows_ref }} and the || job.workflow_sha fallback. A copy of the idiom whose resolver binds ${{ inputs.workflows_ref || 'main' }} therefore registers as no resolver at all, and its consuming checkout drops back out of coverage as "not this lint's subject" — the same "copies inherit the blind spot" concern, one spelling over. For cursor-review.yml itself that is still covered, by the hand assertion this PR deliberately keeps (WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} must be the resolver's binding); for a copy in another workflow it is not. Widening the resolver test to _ENV_ALIAS_RE would pull those in, but it would also demand an empty-ref if: of checkouts whose ref is a mutable branch by construction — a different judgment than this change makes, so it is not folded in here. Refs BE-8130 rather than Closes: the lint now covers the shape the ticket names, and this named gap in the same file stays open.

Unexercised artifacts

The spike this ticket was written from (its findings comment) is on the internal tracker and was not reachable from this environment; its three probes were re-derived here instead — is_ref_use is False on cursor-review.yml's ledger ref: line, _GUARD_BINDING_RE matches the resolver's binding with fallback=True, and is_guard_step is False there because of continue-on-error: true. All three reproduced.
The PR #193 review comment the ticket cites was read (public API) and its ask — "teach find_unguarded_ref_checkouts to follow a ref: through a step output" — is what this implements.

Review follow-ups (`a8017d8`)

Three CodeRabbit findings on the first commit, all addressed:

A quoted if: scalar was failed. cond kept the YAML quotes, so if: "steps.x.outputs.ref != ''" — a consumer with the remedy already applied — was rejected. One matching double-quote pair is now unwrapped before the ${{ … }} unwrap. The comparison after it is still character-exact, so the only string the strip can newly accept is the wanted condition wearing quotes; an OR-widened condition stays refused, quoted or not, and a test pins that. Single-quoted scalars stay out on purpose: YAML escapes their inner ' as '', and that spelling fails LOUDLY with the remedy naming the accepted form rather than passing something unguarded.
The resolver's never-fail property is now pinned. The lint takes a fail-closed resolver as an alternative to the consumer's if:, so deleting continue-on-error: true from resolve_ref and exiting non-zero would have kept the lint green while making the ledger job able to fail — and a failing ledger job takes the whole review matrix with it. test_the_ledger_resolver_carries_the_shape_check now asserts it, verified load-bearing by mutation.
The two silent preconditions are documented. The README now states that the binding is matched literally (a third operand such as || 'main' registers as no resolver — the "Known residual" above, now written where the idiom is copied from) and that the resolver must be an earlier step in the same job. Both were verified against the code: _GUARD_BINDING_RE matches only the two bindings, and resolvers is reset per job and populated as the walk goes.

Refs BE-8130

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 118 passed, 0 failed; python3 .github/workflow-pins/check_workflow_pins.py --workflows-dir .github/workflows: green (10 workflows, every ref checkout guarded, 0 exempt); both CI smoke tests run by hand (reintroduced default → exit 1, unguarded checkout → exit 1); 3 hand mutation checks on cursor-review.yml: delete-if: → lint exit 1, || always() → lint exit 1, restore → exit 0; 2 further mutations for a8017d8: reverting the quote unwrap fails exactly the 2 new positive tests, deleting continue-on-error: true from resolve_ref fails exactly test_the_ledger_resolver_carries_the_shape_check. cursor-review.yml restored byte-for-byte after each (git status clean).
Deviations: none on the acceptance criteria; three implementation judgment calls are listed under "Judgment calls" above, and one in-scope gap is named under "Known residual" — that gap is unchanged by a8017d8, which documents it rather than closing it. The single-quoted YAML spelling of the skip if: is knowingly not accepted, for the reason given under "Review follow-ups".

_{🔄 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/195 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/20/2026 **Status:** ✅ Merged **Merged:** 8/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8130-workflow-pins-steps-output-ref` --- ### 📝 Commits (2) - [`12eb051`](https://github.com/Comfy-Org/github-workflows/commit/12eb051dd7b1a9db0af97051028e5607a123e4ee) feat(workflow-pins): follow a `ref:` through a step output (BE-8130) - [`a8017d8`](https://github.com/Comfy-Org/github-workflows/commit/a8017d8b41c710c1e5df793ccf8c00111721549d) fix(workflow-pins): accept a quoted `if:` scalar, pin the resolver's never-fail (BE-8130) ### 📊 Changes **3 files changed** (+558 additions, -56 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+31 -0) 📝 `.github/workflow-pins/check_workflow_pins.py` (+279 -9) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+248 -47) </details> ### 📄 Description ## ELI-5 The pin lint asks every `actions/checkout` in this repo's reusable workflows: "are you checking out at `workflows_ref`, and if so, did something make sure that ref isn't empty first?" It answered that by reading the `ref:` line and looking for the words `inputs.workflows_ref`. `cursor-review.yml`'s ledger job (a job that must never fail, so it can't run the usual fail-closed guard) resolves the ref one step earlier and writes `ref: ${{ steps.resolve_ref.outputs.ref }}` — no `inputs.workflows_ref` on the line, so the lint stopped seeing that checkout at all. This teaches the lint to follow the `ref:` back one step: if the producing step is the one holding `workflows_ref`, the checkout is in scope again, and it passes only when the resolver itself rejects an empty value or the consuming step carries exactly the `if: … != ''` that skips it. ## What changed `ref_checkouts` now walks a job carrying a small `resolvers` map alongside the existing guard flags: a step whose `env:` binds the input (`_GUARD_BINDING_RE`) and that carries an `id:` at its own key column is recorded, along with whether it is a fail-closed guard. A later `ref:` reading `steps.<id>.outputs.<name>` (block, flow, or `ref: >-` continuation) is then a ref use, and it is covered when **either** that resolver is a hard guard, **or** the consuming step carries exactly `if: steps.<id>.outputs.<name> != ''` — bare or `${{ … }}`-wrapped, either spelling optionally double-quoted, nothing else. An OR-widened `... != '' || always()` runs the checkout precisely when the output is empty, so it is refused; so is an `if:` on a different output, and a job-level `if:` (which skips the resolver too, so it can never tell the two states apart). A `ref:` whose producing step never touches `workflows_ref` is not this lint's subject and is left alone. Sites reached this way are recorded as non-fallback: the `if:` tests the resolved *value*, so guard strength (bare vs `|| job.workflow_sha`) is moot on this path. `check_dir` gained a third error message for the shape, because "copy the `Require a pinned workflows_ref` step in ahead of it" is advice a never-fail job cannot take — the missing piece there is the consumer's own `if:`. That required `ref_checkouts` to carry a fourth field (`via_step_output`); five existing test unpacks moved from `for _, fb, _` to `for _, fb, _, _`. `cursor-review.yml` is **not** touched by this PR — the fix is entirely in the lint, and the ledger job already satisfies it. ## Verification - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` — **118 passed, 0 failed** (102 on `main` before this change; the ticket's "90" baseline predates two other merges to `main`). - `python3 .github/workflow-pins/check_workflow_pins.py` — green: 10 workflows checked, every ref checkout guarded. - **Mutation checks, run by hand against the real `cursor-review.yml`** (the ticket's acceptance): (1) delete the ledger consumer's `if:` line → `check_workflow_pins.py` itself exits 1 with the new BE-8130 annotation; (2) widen it to `... != '' || always()` → exits 1 the same way; (3) restore → exits 0. The suite goes red for both mutations too. `cursor-review.yml` was restored byte-for-byte afterwards (`git status` clean for it). - **Corpus sweep, including the half not being fixed:** across all **45** workflow files in `.github/workflows`, there is exactly **1** `ref:` line reading a step output — the ledger checkout — and it is now covered. **0** step-output `ref:` sites are left outside coverage today. The population this lint deliberately does *not* claim (a `ref:` resolved from a step unrelated to `workflows_ref`) is currently empty here, which is why the new tests carry a synthetic `git rev-parse` fixture for it. - Site count across the four tracked workflows: **15 → 16**, the ledger checkout re-entering coverage. The count is now taken from `ref_checkouts` rather than a per-line `is_ref_use` scan — a step-output site cannot be judged from a single line, since the answer depends on what the job walked past. ## Judgment calls - **`resolvers[id]` stores a bool, not the `{"fallback": …, "is_guard": …}` dict the plan sketched.** The `fallback` half would have been written and never read: the plan's own next point makes guard strength irrelevant on this path, and a lint carrying a field nothing consults is a comment pretending to be data. The rationale is written where the value is recorded. - **The retained ledger test is named `test_the_ledger_resolver_carries_the_shape_check`**, mirroring its groom twin `test_every_groom_guard_carries_the_shape_check`, rather than the `..._rejects_rather_than_sanitizes` the plan offered — the plan asked for a name consistent with the twin. - **The flow form needed one extra helper.** `_step_bounds` answers `None` at a step's own key column by design, which is exactly where `with: {…, ref: …}` puts the `ref:`. `_consuming_step_bounds` re-asks it from a copy of that line pushed one column deeper, rather than growing a second copy of the step-boundary logic that could drift from the first. Worst case it still returns `None`, and the site is then reported rather than credited. ## Known residual — why this is `Refs`, not `Closes` The resolver is recognized through `_GUARD_BINDING_RE`, which matches only `WORKFLOWS_REF: ${{ inputs.workflows_ref }}` and the `|| job.workflow_sha` fallback. A copy of the idiom whose resolver binds `${{ inputs.workflows_ref || 'main' }}` therefore registers as *no resolver at all*, and its consuming checkout drops back out of coverage as "not this lint's subject" — the same "copies inherit the blind spot" concern, one spelling over. For `cursor-review.yml` itself that is still covered, by the hand assertion this PR deliberately keeps (`WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }}` must be the resolver's binding); for a copy in another workflow it is not. Widening the resolver test to `_ENV_ALIAS_RE` would pull those in, but it would also demand an empty-ref `if:` of checkouts whose ref is a mutable branch by construction — a different judgment than this change makes, so it is not folded in here. **`Refs BE-8130` rather than `Closes`: the lint now covers the shape the ticket names, and this named gap in the same file stays open.** ## Unexercised artifacts - The spike this ticket was written from (its findings comment) is on the internal tracker and was not reachable from this environment; its three probes were re-derived here instead — `is_ref_use` is `False` on `cursor-review.yml`'s ledger `ref:` line, `_GUARD_BINDING_RE` matches the resolver's binding with `fallback=True`, and `is_guard_step` is `False` there because of `continue-on-error: true`. All three reproduced. - The PR #193 review comment the ticket cites *was* read (public API) and its ask — "teach `find_unguarded_ref_checkouts` to follow a `ref:` through a step output" — is what this implements. ## Review follow-ups (a8017d8) Three CodeRabbit findings on the first commit, all addressed: - **A quoted `if:` scalar was failed.** `cond` kept the YAML quotes, so `if: "steps.x.outputs.ref != ''"` — a consumer with the remedy already applied — was rejected. One matching double-quote pair is now unwrapped before the `${{ … }}` unwrap. The comparison after it is still character-exact, so the only string the strip can newly accept is the wanted condition wearing quotes; an OR-widened condition stays refused, quoted or not, and a test pins that. Single-quoted scalars stay out on purpose: YAML escapes their inner `'` as `''`, and that spelling fails LOUDLY with the remedy naming the accepted form rather than passing something unguarded. - **The resolver's never-fail property is now pinned.** The lint takes a fail-closed resolver as an *alternative* to the consumer's `if:`, so deleting `continue-on-error: true` from `resolve_ref` and exiting non-zero would have kept the lint green while making the ledger job able to fail — and a failing ledger job takes the whole review matrix with it. `test_the_ledger_resolver_carries_the_shape_check` now asserts it, verified load-bearing by mutation. - **The two silent preconditions are documented.** The README now states that the binding is matched literally (a third operand such as `|| 'main'` registers as no resolver — the "Known residual" above, now written where the idiom is copied from) and that the resolver must be an earlier step in the same job. Both were verified against the code: `_GUARD_BINDING_RE` matches only the two bindings, and `resolvers` is reset per job and populated as the walk goes. Refs BE-8130 ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'`: 118 passed, 0 failed; `python3 .github/workflow-pins/check_workflow_pins.py --workflows-dir .github/workflows`: green (10 workflows, every ref checkout guarded, 0 exempt); both CI smoke tests run by hand (reintroduced default → exit 1, unguarded checkout → exit 1); 3 hand mutation checks on `cursor-review.yml`: delete-`if:` → lint exit 1, `|| always()` → lint exit 1, restore → exit 0; 2 further mutations for a8017d8: reverting the quote unwrap fails exactly the 2 new positive tests, deleting `continue-on-error: true` from `resolve_ref` fails exactly `test_the_ledger_resolver_carries_the_shape_check`. `cursor-review.yml` restored byte-for-byte after each (`git status` clean). - **Deviations:** none on the acceptance criteria; three implementation judgment calls are listed under "Judgment calls" above, and one in-scope gap is named under "Known residual" — that gap is unchanged by a8017d8, which documents it rather than closing it. The single-quoted YAML spelling of the skip `if:` is knowingly not accepted, for the reason given under "Review follow-ups". --- <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:19 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

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

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

No labels

cursor-review-catalog-drift

groom

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/github-workflows#203

No description provided.

Rows
Columns

[PR #195] [MERGED] feat(workflow-pins): follow a ref: through a step output so the never-fail idiom is linted (BE-8130) #203