[PR #198] [MERGED] fix(workflow-pins): recognize flow-form env, bracket access and env chains as ref uses (BE-8146) #207

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-8146-workflow-pins-ref-uses

📝 Commits (4)

4d50e1b fix(workflow-pins): recognize flow-form env, bracket access and env chains as ref uses (BE-8146)
0e239d7 fix(workflow-pins): close review-flagged gaps in the BE-8146 ref-use widening
27e4655 merge main into matt/be-8146-workflow-pins-ref-uses
aca730f Merge remote-tracking branch 'origin/main' into matt/be-8146-workflow-pins-ref-uses

📊 Changes

3 files changed (+901 additions, -42 deletions)

View changed files

📝 .github/workflow-pins/README.md (+42 -1)
📝 .github/workflow-pins/check_workflow_pins.py (+429 -41)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+430 -0)

📄 Description

ELI-5

check_workflow_pins.py makes sure every job that checks this repo out at workflows_ref first proves that ref isn't empty — otherwise actions/checkout silently takes the default branch and the job runs mutable scripts under write permissions. The lint found that checkout by pattern-matching the text, and it only knew one way to spell it. Three other perfectly legal spellings made the checkout invisible, so the lint went green over an unguarded checkout. This teaches it those three spellings.

The three spellings, and why each failed silently

Each was verified failing open against main before the fix — the fixture reports unguarded=[] (a clean lint) while carrying an unguarded checkout:

Flow-form env: — env: {WORKFLOWS_REF: ${{ inputs.workflows_ref }}} followed by ref: ${{ env.WORKFLOWS_REF }}. _ENV_KEY_RE required env: to end its line, so nothing bound the alias and the ref: read as no ref use at all.
Index access — ref: ${{ inputs['workflows_ref'] }} and ref: ${{ env['WORKFLOWS_REF'] }}. Every mention regex was dot-access only, though inputs['x'] is documented Actions expression syntax and interchangeable with inputs.x.
An alias chain — BASE: ${{ inputs.workflows_ref }} then REF: ${{ env.BASE }}, consumed as ref: ${{ env.REF }}. The alias scan followed exactly one hop.

The _CONSUMES_* backstop did not cover any of them, and the module's own comment claimed it did ("a flow-style env: {…} binds no alias here, which loses nothing the _CONSUMES_* backstop does not already catch"). That comment was false and is corrected in the diff: the backstop only runs when defaults is None — when the input declaration itself was unparseable — so for a well-formed workflow it never ran at all.

What changed

One shared _INPUT_MENTION_BODY now defines how a workflow can name the input, and every "does this reach the input" pattern is built from it: _REF_USE_BLOCK_RE / _REF_USE_FLOW_RE, _mention_alt, the env: alias patterns, and the _CONSUMES_* trio (so a bracket-only file with a lost declaration still trips the loud "NOT covering this file" error rather than being a silent skip). The dead, never-referenced _INPUT_MENTION_RE is deleted rather than left as a second, narrower spelling of the same concept.
_env_bindings learned the flow form. Entries on an env: {…} line are bounded at the entry boundary ([^,}]) exactly as _REF_USE_FLOW_RE bounds its own value, and each entry's start is checked with _outside_quotes, so a , planted inside a quoted scalar cannot bind a decoy name.
env_aliases is a fixpoint instead of a single hop, bounded by the number of names bound anywhere in the file (each pass either adds a name or stops, and every addable name is one of those). An empty alias set short-circuits, because an empty alternation would degenerate to env\.(?:)\b and bind every name in the file.

Deliberately asymmetric, and this is the point: all three widen only what counts as a ref use, which can only ever demand a guard, never excuse a missing one. The guard-recognition side (_GUARD_BINDING_RE) stays block-form and dot-access only, and _FALLBACK_RES — an exemption — stays literal. An unrecognized guard and an unclaimed exemption both fail closed, which is loud. Two tests pin that asymmetry directly (test_the_guard_side_is_NOT_widened_to_the_flow_form, test_bracket_access_NEVER_earns_the_self_pin_exemption).

Testing

python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' — 138 passed, 0 failed (was 118; 20 added).
Red→green proof: checked out main's check_workflow_pins.py under the new test file — 9 of the new tests fail, one per behavior this diff adds. The remaining new tests are the "same checkout behind the canonical guard still passes" and negative-boundary halves, which must pass in both directions.
python3 .github/workflow-pins/check_workflow_pins.py against this repo's own .github/workflows/ — green, 10 workflows, 25 ref checkouts judged, 0 errors (no new errors, as expected: a sweep of all 45 workflow files found 0 using any of the three spellings, so any new error would have meant a regex over-matched).
Both CLI smoke tests from test-workflow-pins.yml run locally — a reintroduced default: and an unguarded checkout each still exit non-zero.
Unrelated suites re-run to confirm no collateral: cursor-review 194 passed, agents-md-integrity 46 passed, groom 365 passed, 1 skipped.
Performance: check_dir over the real .github/workflows/ is 0.081s; env_aliases on the 3,308-line groom.yml converges in one extra pass (~9ms).

Residual — the part I did NOT fix

The same corpus sweep, pointed at the shapes this diff does not cover, found 0 of 45 workflow files using any of them today. Two remain verified-failing-open, and both are the module's pre-existing line-orientation rather than something this diff introduces (_REF_USE_FLOW_RE, _STEPS_OUTPUT_FLOW_RE and _FALLBACK_RES' flow arm all share it):

an env: { whose entries continue on later lines, and
an env: nested inside a larger flow mapping (- {uses: …, env: {…}}), which _ENV_FLOW_KEY_RE's line-start anchor does not reach.

Because that is an unfixed gap on the very file this change is about, no closing keyword is written here and the driving work item stays open to carry it. Teaching the module multi-line flow mappings is a parser-shape change across every flow matcher, not a widening of this one — worth doing on its own, not as a rider here.

One further near-miss was cheap enough to close and is included: Actions permits whitespace inside an index expression, so inputs[ 'workflows_ref' ] is the same access; the brackets now tolerate it.

Judgment calls / deviations from the plan as written

_ENV_ALIAS_RE became _ENV_ALIAS_RES, a (block, flow) pair produced by a small _env_alias_res(mention) builder. The fixpoint has to build the same pair per pass against a different "reaches" test (env.<known>), so parameterizing it was the alternative to a third hand-written copy of the pattern. Nothing outside the module referenced the old name.
_REF_USE_BLOCK_RE / _REF_USE_FLOW_RE were widened too, which the plan did not list. They are is_ref_use's defaults when no res is passed; leaving them dot-only would have recreated exactly the "second, narrower spelling of the same concept" the plan objects to for _INPUT_MENTION_RE. In-module behavior is unchanged — ref_checkouts always passes _ref_use_res(aliases).
Capability-denial check: the trigger did not fire — this diff adds no deny/dead-end path and no "not supported"-family string; it only widens what an existing lint reports, and the error messages are pre-existing. The nearest thing, test_the_guard_side_is_NOT_widened_to_the_flow_form, asserts the plan's explicit fail-closed design (an unrecognized guard reports its checkout), and that behavior was exercised rather than assumed.
Unexercised artifacts: the investigation this plan came from lives in an internal tracker I have no access to from this sandbox, so its evidence was re-derived here from scratch rather than read — every claim above about a spelling failing open is from a fixture I ran against main, not from that write-up. Separately, AGENTS.md is 281 lines and already fails the repo's own 200-line integrity ceiling on main, untouched by this diff and with no CI caller wired up in this repo; flagging it, not fixing it here.

Provenance

Authored by: agent-work loop
Verified: workflow-pins unittest 138 passed / 0 failed (9 of the new tests confirmed red against main's checker); check_workflow_pins.py on this repo's workflows green, 0 errors; both test-workflow-pins.yml CLI smoke tests exit non-zero as expected; cursor-review 194 passed, agents-md-integrity 46 passed, groom 365 passed / 1 skipped
Deviations: _ENV_ALIAS_RE refactored into a (block, flow) pair and _REF_USE_* widened alongside it (neither listed in the plan, both reasoned above); two multi-line/nested flow-mapping spellings verified still failing open and deliberately left out of scope

_{🔄 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/198 **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-8146-workflow-pins-ref-uses` --- ### 📝 Commits (4) - [`4d50e1b`](https://github.com/Comfy-Org/github-workflows/commit/4d50e1bbfa5fa14ce6502f98497ceecf71e2964d) fix(workflow-pins): recognize flow-form env, bracket access and env chains as ref uses (BE-8146) - [`0e239d7`](https://github.com/Comfy-Org/github-workflows/commit/0e239d75df29b653af73b81f6be3dac3ca8403ac) fix(workflow-pins): close review-flagged gaps in the BE-8146 ref-use widening - [`27e4655`](https://github.com/Comfy-Org/github-workflows/commit/27e4655d5a867ba17c0df9874df79ee635239002) merge main into matt/be-8146-workflow-pins-ref-uses - [`aca730f`](https://github.com/Comfy-Org/github-workflows/commit/aca730f5d246670180b45a9036a0b2f4c550f2e1) Merge remote-tracking branch 'origin/main' into matt/be-8146-workflow-pins-ref-uses ### 📊 Changes **3 files changed** (+901 additions, -42 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+42 -1) 📝 `.github/workflow-pins/check_workflow_pins.py` (+429 -41) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+430 -0) </details> ### 📄 Description ## ELI-5 `check_workflow_pins.py` makes sure every job that checks this repo out at `workflows_ref` first proves that ref isn't empty — otherwise `actions/checkout` silently takes the default branch and the job runs *mutable* scripts under write permissions. The lint found that checkout by pattern-matching the text, and it only knew one way to spell it. Three other perfectly legal spellings made the checkout invisible, so the lint went green over an unguarded checkout. This teaches it those three spellings. ## The three spellings, and why each failed silently Each was verified failing open against `main` before the fix — the fixture reports `unguarded=[]` (a clean lint) while carrying an unguarded checkout: 1. **Flow-form `env:`** — `env: {WORKFLOWS_REF: ${{ inputs.workflows_ref }}}` followed by `ref: ${{ env.WORKFLOWS_REF }}`. `_ENV_KEY_RE` required `env:` to end its line, so nothing bound the alias and the `ref:` read as no ref use at all. 2. **Index access** — `ref: ${{ inputs['workflows_ref'] }}` and `ref: ${{ env['WORKFLOWS_REF'] }}`. Every mention regex was dot-access only, though `inputs['x']` is documented Actions expression syntax and interchangeable with `inputs.x`. 3. **An alias chain** — `BASE: ${{ inputs.workflows_ref }}` then `REF: ${{ env.BASE }}`, consumed as `ref: ${{ env.REF }}`. The alias scan followed exactly one hop. The `_CONSUMES_*` backstop did not cover any of them, and the module's own comment claimed it did ("a flow-style `env: {…}` binds no alias here, which loses nothing the `_CONSUMES_*` backstop does not already catch"). That comment was false and is corrected in the diff: the backstop only runs when `defaults is None` — when the input *declaration* itself was unparseable — so for a well-formed workflow it never ran at all. ## What changed - **One shared `_INPUT_MENTION_BODY`** now defines how a workflow can name the input, and every "does this reach the input" pattern is built from it: `_REF_USE_BLOCK_RE` / `_REF_USE_FLOW_RE`, `_mention_alt`, the `env:` alias patterns, and the `_CONSUMES_*` trio (so a bracket-only file with a lost declaration still trips the loud "NOT covering this file" error rather than being a silent skip). The dead, never-referenced `_INPUT_MENTION_RE` is deleted rather than left as a second, narrower spelling of the same concept. - **`_env_bindings` learned the flow form.** Entries on an `env: {…}` line are bounded at the entry boundary (`[^,}]`) exactly as `_REF_USE_FLOW_RE` bounds its own value, and each entry's start is checked with `_outside_quotes`, so a `,` planted inside a quoted scalar cannot bind a decoy name. - **`env_aliases` is a fixpoint** instead of a single hop, bounded by the number of names bound anywhere in the file (each pass either adds a name or stops, and every addable name is one of those). An empty alias set short-circuits, because an empty alternation would degenerate to `env\.(?:)\b` and bind every name in the file. **Deliberately asymmetric, and this is the point:** all three widen only what counts as a ref **use**, which can only ever *demand* a guard, never excuse a missing one. The guard-recognition side (`_GUARD_BINDING_RE`) stays block-form and dot-access only, and `_FALLBACK_RES` — an *exemption* — stays literal. An unrecognized guard and an unclaimed exemption both fail **closed**, which is loud. Two tests pin that asymmetry directly (`test_the_guard_side_is_NOT_widened_to_the_flow_form`, `test_bracket_access_NEVER_earns_the_self_pin_exemption`). ## Testing - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` — **138 passed, 0 failed** (was 118; 20 added). - **Red→green proof:** checked out `main`'s `check_workflow_pins.py` under the new test file — **9 of the new tests fail**, one per behavior this diff adds. The remaining new tests are the "same checkout behind the canonical guard still passes" and negative-boundary halves, which must pass in both directions. - `python3 .github/workflow-pins/check_workflow_pins.py` against this repo's own `.github/workflows/` — **green, 10 workflows, 25 ref checkouts judged, 0 errors** (no new errors, as expected: a sweep of all 45 workflow files found **0** using any of the three spellings, so any new error would have meant a regex over-matched). - Both CLI smoke tests from `test-workflow-pins.yml` run locally — a reintroduced `default:` and an unguarded checkout each still exit non-zero. - Unrelated suites re-run to confirm no collateral: cursor-review **194 passed**, agents-md-integrity **46 passed**, groom **365 passed, 1 skipped**. - Performance: `check_dir` over the real `.github/workflows/` is 0.081s; `env_aliases` on the 3,308-line `groom.yml` converges in one extra pass (~9ms). ## Residual — the part I did NOT fix The same corpus sweep, pointed at the shapes this diff does **not** cover, found **0 of 45** workflow files using any of them today. Two remain verified-failing-open, and both are the module's pre-existing line-orientation rather than something this diff introduces (`_REF_USE_FLOW_RE`, `_STEPS_OUTPUT_FLOW_RE` and `_FALLBACK_RES`' flow arm all share it): - an `env: {` whose entries continue on **later lines**, and - an `env:` **nested inside** a larger flow mapping (`- {uses: …, env: {…}}`), which `_ENV_FLOW_KEY_RE`'s line-start anchor does not reach. Because that is an unfixed gap on the very file this change is about, no closing keyword is written here and the driving work item stays open to carry it. Teaching the module multi-line flow mappings is a parser-shape change across every flow matcher, not a widening of this one — worth doing on its own, not as a rider here. One further near-miss **was** cheap enough to close and is included: Actions permits whitespace inside an index expression, so `inputs[ 'workflows_ref' ]` is the same access; the brackets now tolerate it. ## Judgment calls / deviations from the plan as written - **`_ENV_ALIAS_RE` became `_ENV_ALIAS_RES`**, a `(block, flow)` pair produced by a small `_env_alias_res(mention)` builder. The fixpoint has to build the same pair per pass against a *different* "reaches" test (`env.<known>`), so parameterizing it was the alternative to a third hand-written copy of the pattern. Nothing outside the module referenced the old name. - **`_REF_USE_BLOCK_RE` / `_REF_USE_FLOW_RE` were widened too**, which the plan did not list. They are `is_ref_use`'s defaults when no `res` is passed; leaving them dot-only would have recreated exactly the "second, narrower spelling of the same concept" the plan objects to for `_INPUT_MENTION_RE`. In-module behavior is unchanged — `ref_checkouts` always passes `_ref_use_res(aliases)`. - **Capability-denial check:** the trigger did not fire — this diff adds no deny/dead-end path and no "not supported"-family string; it only widens what an existing lint reports, and the error messages are pre-existing. The nearest thing, `test_the_guard_side_is_NOT_widened_to_the_flow_form`, asserts the plan's explicit fail-*closed* design (an unrecognized guard reports its checkout), and that behavior was exercised rather than assumed. - **Unexercised artifacts:** the investigation this plan came from lives in an internal tracker I have no access to from this sandbox, so its evidence was re-derived here from scratch rather than read — every claim above about a spelling failing open is from a fixture I ran against `main`, not from that write-up. Separately, `AGENTS.md` is 281 lines and already fails the repo's own 200-line integrity ceiling on `main`, untouched by this diff and with no CI caller wired up in this repo; flagging it, not fixing it here. ## Provenance - **Authored by:** agent-work loop - **Verified:** workflow-pins unittest 138 passed / 0 failed (9 of the new tests confirmed red against main's checker); `check_workflow_pins.py` on this repo's workflows green, 0 errors; both `test-workflow-pins.yml` CLI smoke tests exit non-zero as expected; cursor-review 194 passed, agents-md-integrity 46 passed, groom 365 passed / 1 skipped - **Deviations:** `_ENV_ALIAS_RE` refactored into a `(block, flow)` pair and `_REF_USE_*` widened alongside it (neither listed in the plan, both reasoned above); two multi-line/nested flow-mapping spellings verified still failing open and deliberately left out of scope --- <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:22 +00:00