[PR #201] [MERGED] fix(workflow-pins): drop the hard-guard resolver exemption and scan the marker line in is_guard_step (BE-8221) #209

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/201
Author: @mattmillerai
Created: 8/20/2026
Status: ✅ Merged
Merged: 8/21/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8221-guard-marker-line-resolver-exemption

📝 Commits (7)

7603faa fix(workflow-pins): drop the hard-guard resolver exemption and scan the marker line in is_guard_step (BE-8221)
c0e3689 fix(workflow-pins): normalize any list marker, and see the marker line in _skips_on_empty_output and _step_bounds (BE-8221)
caf36e4 fix(workflow-pins): anchor step bounds to steps:, gate marker-line bounds, and see the flow binding (BE-8221)
b0268c1 Merge origin/main into matt/be-8221-guard-marker-line-resolver-exemption
67261c9 Merge remote-tracking branch 'origin/main' into matt/be-8221-guard-marker-line-resolver-exemption
872fed5 Merge remote-tracking branch 'origin/main' into matt/be-8221-guard-marker-line-resolver-exemption
c7906e3 fix(workflow-pins): don't let a flow-form guard excuse a direct checkout (BE-8221)

📊 Changes

3 files changed (+616 additions, -104 deletions)

View changed files

📝 .github/workflow-pins/README.md (+21 -14)
📝 .github/workflow-pins/check_workflow_pins.py (+237 -80)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+358 -10)

📄 Description

ELI-5

Some of our workflow jobs figure out which version of our scripts to download in one step ("the resolver") and do the actual download in a later step. The lint that polices this used to say: if the resolver step checks its own input carefully, the download step doesn't need its own safety check. But checking the input tells you nothing about what the resolver actually hands over — it can validate its input and still pass along an empty value (for example, its clean-up branch blanks a malformed value, or its hand-over line was deleted). Now the download step must always carry its own "skip if empty" check. The lint also used to go blind whenever a step's first key was written on the same line as its - bullet — which let a step that can never fail masquerade as a safety guard, and, in the other direction, made the lint fail a download step that did carry the right check. That blind spot is fixed in every spelling YAML allows for that bullet.

What changed

The hard-guard resolver exemption is gone. _record_steps_output no longer accepts "the producing step is a fail-closed guard" as coverage for a step-output checkout — that proves the step rejects an empty input, nothing about the value written to $GITHUB_OUTPUT. Verified holes the old rule passed: a resolver that guards the input then sanitizes a malformed ref to '' (the exact case-pattern shape cursor-review.yml's real resolver uses), a resolver whose output write was dropped or renamed, and a consumer naming an output the step never sets. A step-output site is now covered only by the consumer's exact if: steps.<id>.outputs.<name> != ''. resolvers becomes membership-only (a set), and the remedy text, docstrings, and README now name the one accepted remedy.

The - list marker no longer hides a step's first key — anywhere. A step's first key can be written on the marker line itself, and _indent reads the marker's physical column, so every _indent(line) != key_indent scan skipped it. Four sites, three of which fail open:

is_guard_step — - continue-on-error: true (or - if: …) as the first key let a never-fail step score as a hard guard. This affected the classic direct-binding path too, not just the step-output role.
_binding_step_id / is_guard_step marker width — both stripped - as a fixed two-column slice, but YAML allows any run of separation spaces after the -. At dash+3 (- id: resolve) the slice left one space behind and the rewritten key landed one column deep, reopening the hole to anyone who types an extra space. _marker_width now measures the marker; _unmarked applies it at every site.
_skips_on_empty_output — never taught the marker line at all. With the resolver exemption gone this if: is the only coverage route, so a consumer written - if: steps.<id>.outputs.ref != '' was reported unguarded and told to add the line it already carried. (Loud, not silent — but it fails the one remedy the lint accepts.)
_step_bounds — read the step's key column off the marker's physical indent, so a step whose first key is the binding block itself (- env: with id:/run: below) resolved to no step at all: is_guard_step failed closed, but _binding_step_id never registered the resolver and its consumer's ref: ${{ steps.<id>.outputs.ref }} passed the lint unreported. It now reads the column from where the key actually starts. The step-start scan also accepts a bare - marker (every key on the lines below it) — the same fail-open in its third spelling.

end is extracted as _step_end so both _step_bounds exits share one definition.

Round 2 (caf36e4): the marker/step machinery held to the same fail-closed bar. The round-1 fixes taught four scans the marker line; the second panel round found the STEP-RESOLUTION machinery under them could still be steered by things that are not steps, plus two registration gaps that became silent passes once the resolver exemption was gone:

_step_bounds now refuses any list item that is not an entry of a steps: sequence (_in_steps_sequence): a strategy.matrix.include item, or step-shaped text inside a run: | block scalar, could carry the binding's shape plus a guard-shaped script and score as a job-wide hard guard — blessing every later unguarded checkout in the job.
The marker fast path also requires the asked line to sit INSIDE the item's bounds, and a marker-line opener (- with: {ref: …}) resolves to ITS OWN step: the shifted retry used to stop at the PRECEDING step's marker and credit a marker-first flow consumer with its neighbour's if: — the normal shape of the never-fail idiom's first checkout, so the second checkout inherited a guard it does not carry.
_env_bindings normalizes the marker, so a marker-first - env: block binds aliases too — alias discovery is the one path deciding whether a later ref: ${{ env.<name> }} is SEEN at all. Its block boundary is measured at the key column, so the step's other keys cannot bind false aliases.
_GUARD_BINDING_FLOW_RE registers a flow-form binding (env: {WORKFLOWS_REF: …}): the old "flow reads as ABSENT, which fails loudly" rationale died with the exemption — absence on the resolver path now means the consumer is never judged at all. A line that is ALSO a ref use stays reported rather than swallowed.
is_guard_step normalizes the marker line ONCE for both of its scans, so a one-line guard written marker-first (- run: [ -z "$WORKFLOWS_REF" ] && exit 1) is recognized instead of reported with the remedy the author already has.
_marker_width answers 0 for a dash with only trailing whitespace, matching its documented bare-marker contract; _consuming_step_bounds' docstring restates the retry's actual (backstop) role.

Verification

143 tests pass; 8 of round 1's new/flipped tests and 10 of round 2's 11 fail against the unmodified checker (the flipped exemption test, plus the wide-marker guard and id paths, the marker-line if:, the marker-line env: resolver, and the bare-marker resolver and consumer).
New tests also pin what must not move: the sound composition (hard guard + exact if: still passes), and that normalizing the marker line and the spacing did not relax what is matched there — an OR-widened if: stays refused in the marker-first spelling too.
python3 .github/workflow-pins/check_workflow_pins.py: OK — 10 workflows checked, every ref checkout guarded, 0 exempt.
Empirical no-op check on the real tree: ref_checkouts over the repo's own 10 workflows is byte-identical before and after — all 25 sites keep their (fallback, guarded, via_step_output) tuple. The tree's only step-output site (cursor-review.yml's ledger job) already carries the exact if: on its consumer, which is also the empirical proof that the one remedy the tightened lint accepts is practical — it is what the real workflow already does.

Coordination with #197

#197 (open) now overlaps this PR inside _skips_on_empty_output, so whichever merges second will hit a real conflict there — this is a change from this PR's original note, which predated the review round below.

Both teach _skips_on_empty_output the marker line. #197 uses the fixed line.lstrip()[2:] slice; this PR uses _unmarked, which measures the marker instead and so also covers the dash+3 spelling. Resolution: keep _unmarked.
#197 independently normalizes the spacing around != (so …outputs.ref!='' passes) and hardens steps_output_ref's flow-form walk against a quoted-comma decoy. Neither is touched here — keep both.
#197's README hunk reflows the same sentence this PR rewrites (the coverage rule in the BE-8130 section). Mechanical: keep #197's "in one of the two literal spellings below" insertion and this PR's consumer-if:-only rule.
The two suites also both add a marker-line if: test under different names; either name is fine, keeping one is enough.

Judgment calls / notes

The old README sentence "A never-fail resolver can only take the second route" was dropped rather than rewritten — with a single route left it no longer disambiguates anything, and the continue-on-error rationale it carried lives on in the new exemption-rationale sentence and in is_guard_step's own comments.
The _step_bounds hole was raised in review as pre-existing and Low, with "fail closed in _record_steps_output" as the suggested remedy. Fixed at the root instead: failing closed there would have caught the legitimate not-this-lint's-subject case its docstring describes (a git rev-parse resolver), failing workflows the lint has no claim on.
Two test comments that still justified the removed exemption are rewritten. test_the_ledger_resolver_carries_the_shape_check's continue-on-error: true assertion is kept but re-justified — the lint has no opinion on it either way now, which is precisely why the assertion has to stay.
The spike ticket this implements was produced by a read-only investigation whose findings page I could not read directly (no tracker access from this sandbox); its evidence was reproduced in full in the implementing ticket's body and every claim in it was re-verified here empirically (the failing-then-passing test run above).

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 143 passed, 0 failed (8 of round 1's new/flipped tests and 10 of round 2's 11 fail against the unmodified checker); python3 .github/workflow-pins/check_workflow_pins.py: OK — 10 workflows checked, every ref checkout guarded, 0 exempt; ref_checkouts diffed over all 10 workflows before/after: identical, 25 sites.
Deviations: none.

_{🔄 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/201 **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-8221-guard-marker-line-resolver-exemption` --- ### 📝 Commits (7) - [`7603faa`](https://github.com/Comfy-Org/github-workflows/commit/7603faae81b5303acc3ffb438f5e51ee43d59f2e) fix(workflow-pins): drop the hard-guard resolver exemption and scan the marker line in is_guard_step (BE-8221) - [`c0e3689`](https://github.com/Comfy-Org/github-workflows/commit/c0e3689a857c1a4ea301797aef929745110c8d46) fix(workflow-pins): normalize any list marker, and see the marker line in _skips_on_empty_output and _step_bounds (BE-8221) - [`caf36e4`](https://github.com/Comfy-Org/github-workflows/commit/caf36e488486effad7560ff20412d615f5763aed) fix(workflow-pins): anchor step bounds to steps:, gate marker-line bounds, and see the flow binding (BE-8221) - [`b0268c1`](https://github.com/Comfy-Org/github-workflows/commit/b0268c134432497f7c2c20511f6062486d791217) Merge origin/main into matt/be-8221-guard-marker-line-resolver-exemption - [`67261c9`](https://github.com/Comfy-Org/github-workflows/commit/67261c9eda5db376e9f78ed8dc3a611c5e59264d) Merge remote-tracking branch 'origin/main' into matt/be-8221-guard-marker-line-resolver-exemption - [`872fed5`](https://github.com/Comfy-Org/github-workflows/commit/872fed597e11264d65fab1d7cd588ba39cf42df9) Merge remote-tracking branch 'origin/main' into matt/be-8221-guard-marker-line-resolver-exemption - [`c7906e3`](https://github.com/Comfy-Org/github-workflows/commit/c7906e3c1715d87719e364623c32c3e0f58984fc) fix(workflow-pins): don't let a flow-form guard excuse a direct checkout (BE-8221) ### 📊 Changes **3 files changed** (+616 additions, -104 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+21 -14) 📝 `.github/workflow-pins/check_workflow_pins.py` (+237 -80) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+358 -10) </details> ### 📄 Description ## ELI-5 Some of our workflow jobs figure out which version of our scripts to download in one step ("the resolver") and do the actual download in a later step. The lint that polices this used to say: if the resolver step checks its own input carefully, the download step doesn't need its own safety check. But checking the input tells you nothing about what the resolver actually hands over — it can validate its input and still pass along an empty value (for example, its clean-up branch blanks a malformed value, or its hand-over line was deleted). Now the download step must always carry its own "skip if empty" check. The lint also used to go blind whenever a step's first key was written on the same line as its `- ` bullet — which let a step that can never fail masquerade as a safety guard, and, in the other direction, made the lint fail a download step that *did* carry the right check. That blind spot is fixed in every spelling YAML allows for that bullet. ## What changed **The hard-guard resolver exemption is gone.** `_record_steps_output` no longer accepts "the producing step is a fail-closed guard" as coverage for a step-output checkout — that proves the step rejects an empty input, nothing about the value written to `$GITHUB_OUTPUT`. Verified holes the old rule passed: a resolver that guards the input then sanitizes a malformed ref to `''` (the exact `case`-pattern shape `cursor-review.yml`'s real resolver uses), a resolver whose output write was dropped or renamed, and a consumer naming an output the step never sets. A step-output site is now covered only by the consumer's exact `if: steps.<id>.outputs.<name> != ''`. `resolvers` becomes membership-only (a set), and the remedy text, docstrings, and README now name the one accepted remedy. **The `- ` list marker no longer hides a step's first key — anywhere.** A step's first key can be written on the marker line itself, and `_indent` reads the marker's physical column, so every `_indent(line) != key_indent` scan skipped it. Four sites, three of which fail *open*: - `is_guard_step` — `- continue-on-error: true` (or `- if: …`) as the first key let a never-fail step score as a hard guard. This affected the classic direct-binding path too, not just the step-output role. - `_binding_step_id` / `is_guard_step` marker width — both stripped `- ` as a fixed two-column slice, but YAML allows any run of separation spaces after the `-`. At dash+3 (`- id: resolve`) the slice left one space behind and the rewritten key landed one column deep, reopening the hole to anyone who types an extra space. `_marker_width` now *measures* the marker; `_unmarked` applies it at every site. - `_skips_on_empty_output` — never taught the marker line at all. With the resolver exemption gone this `if:` is the *only* coverage route, so a consumer written `- if: steps.<id>.outputs.ref != ''` was reported unguarded and told to add the line it already carried. (Loud, not silent — but it fails the one remedy the lint accepts.) - `_step_bounds` — read the step's key column off the marker's *physical* indent, so a step whose first key is the binding block itself (`- env:` with `id:`/`run:` below) resolved to no step at all: `is_guard_step` failed closed, but `_binding_step_id` never registered the resolver and its consumer's `ref: ${{ steps.<id>.outputs.ref }}` passed the lint unreported. It now reads the column from where the key actually starts. The step-start scan also accepts a bare `-` marker (every key on the lines below it) — the same fail-open in its third spelling. `end` is extracted as `_step_end` so both `_step_bounds` exits share one definition. **Round 2 (caf36e4): the marker/step machinery held to the same fail-closed bar.** The round-1 fixes taught four scans the marker line; the second panel round found the STEP-RESOLUTION machinery under them could still be steered by things that are not steps, plus two registration gaps that became silent passes once the resolver exemption was gone: - `_step_bounds` now refuses any list item that is not an entry of a `steps:` sequence (`_in_steps_sequence`): a `strategy.matrix.include` item, or step-shaped text inside a `run: |` block scalar, could carry the binding's shape plus a guard-shaped script and score as a job-wide hard guard — blessing every later unguarded checkout in the job. - The marker fast path also requires the asked line to sit INSIDE the item's bounds, and a marker-line opener (`- with: {ref: …}`) resolves to ITS OWN step: the shifted retry used to stop at the PRECEDING step's marker and credit a marker-first flow consumer with its neighbour's `if:` — the normal shape of the never-fail idiom's first checkout, so the second checkout inherited a guard it does not carry. - `_env_bindings` normalizes the marker, so a marker-first `- env:` block binds aliases too — alias discovery is the one path deciding whether a later `ref: ${{ env.<name> }}` is SEEN at all. Its block boundary is measured at the key column, so the step's other keys cannot bind false aliases. - `_GUARD_BINDING_FLOW_RE` registers a flow-form binding (`env: {WORKFLOWS_REF: …}`): the old "flow reads as ABSENT, which fails loudly" rationale died with the exemption — absence on the resolver path now means the consumer is never judged at all. A line that is ALSO a ref use stays reported rather than swallowed. - `is_guard_step` normalizes the marker line ONCE for both of its scans, so a one-line guard written marker-first (`- run: [ -z "$WORKFLOWS_REF" ] && exit 1`) is recognized instead of reported with the remedy the author already has. - `_marker_width` answers 0 for a dash with only trailing whitespace, matching its documented bare-marker contract; `_consuming_step_bounds`' docstring restates the retry's actual (backstop) role. ## Verification - 143 tests pass; **8 of round 1's new/flipped tests and 10 of round 2's 11 fail against the unmodified checker** (the flipped exemption test, plus the wide-marker guard and id paths, the marker-line `if:`, the marker-line `env:` resolver, and the bare-marker resolver and consumer). - New tests also pin what must *not* move: the sound composition (hard guard **+** exact `if:` still passes), and that normalizing the marker line and the spacing did not relax *what* is matched there — an OR-widened `if:` stays refused in the marker-first spelling too. - `python3 .github/workflow-pins/check_workflow_pins.py`: OK — 10 workflows checked, every ref checkout guarded, 0 exempt. - Empirical no-op check on the real tree: `ref_checkouts` over the repo's own 10 workflows is **byte-identical before and after** — all 25 sites keep their `(fallback, guarded, via_step_output)` tuple. The tree's only step-output site (`cursor-review.yml`'s ledger job) already carries the exact `if:` on its consumer, which is also the empirical proof that the one remedy the tightened lint accepts is practical — it is what the real workflow already does. ## Coordination with #197 #197 (open) now **overlaps this PR inside `_skips_on_empty_output`**, so whichever merges second will hit a real conflict there — this is a change from this PR's original note, which predated the review round below. - Both teach `_skips_on_empty_output` the marker line. #197 uses the fixed `line.lstrip()[2:]` slice; this PR uses `_unmarked`, which measures the marker instead and so also covers the dash+3 spelling. **Resolution: keep `_unmarked`.** - #197 independently normalizes the spacing around `!=` (so `…outputs.ref!=''` passes) and hardens `steps_output_ref`'s flow-form walk against a quoted-comma decoy. **Neither is touched here — keep both.** - #197's README hunk reflows the same sentence this PR rewrites (the coverage rule in the BE-8130 section). Mechanical: keep #197's "in one of the two literal spellings below" insertion and this PR's consumer-`if:`-only rule. - The two suites also both add a marker-line `if:` test under different names; either name is fine, keeping one is enough. ## Judgment calls / notes - The old README sentence "A never-fail resolver can only take the second route" was dropped rather than rewritten — with a single route left it no longer disambiguates anything, and the `continue-on-error` rationale it carried lives on in the new exemption-rationale sentence and in `is_guard_step`'s own comments. - The `_step_bounds` hole was raised in review as pre-existing and Low, with "fail closed in `_record_steps_output`" as the suggested remedy. Fixed at the root instead: failing closed there would have caught the legitimate not-this-lint's-subject case its docstring describes (a `git rev-parse` resolver), failing workflows the lint has no claim on. - Two test comments that still justified the removed exemption are rewritten. `test_the_ledger_resolver_carries_the_shape_check`'s `continue-on-error: true` assertion is kept but re-justified — the lint has no opinion on it either way now, which is precisely why the assertion has to stay. - The spike ticket this implements was produced by a read-only investigation whose findings page I could not read directly (no tracker access from this sandbox); its evidence was reproduced in full in the implementing ticket's body and every claim in it was re-verified here empirically (the failing-then-passing test run above). ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'`: 143 passed, 0 failed (8 of round 1's new/flipped tests and 10 of round 2's 11 fail against the unmodified checker); `python3 .github/workflow-pins/check_workflow_pins.py`: OK — 10 workflows checked, every ref checkout guarded, 0 exempt; `ref_checkouts` diffed over all 10 workflows before/after: identical, 25 sites. - **Deviations:** none. --- <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