[PR #222] [MERGED] feat(workflow-pins): warn when a job's `steps:` escapes the step-id pre-scan (BE-9064) #229

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-9064-workflow-pins-escaped-steps-warning

📝 Commits (2)

daab3c3 feat(workflow-pins): warn when a job's steps: escapes the step-id pre-scan (BE-9064)
952d1b9 fix(workflow-pins): tighten the escaped-steps: warning after review (BE-9064)

📊 Changes

4 files changed (+545 additions, -49 deletions)

View changed files

📝 .github/workflow-pins/README.md (+21 -2)
📝 .github/workflow-pins/check_workflow_pins.py (+223 -19)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+263 -28)
📝 .github/workflows/test-workflow-pins.yml (+38 -0)

📄 Description

ELI-5

The pin lint has one job: make sure no reusable workflow here can check out at a mutable ref. One part of it — the dangling-ref check — quietly gives up on a job whose steps: is written in a shape it can't read (steps: [ … ] on the key line, an anchor, or no - item below it). Giving up is the right call: guessing there would fail perfectly good workflows. The problem was that it gave up silently — the lint printed "every ref checkout guarded" and exited 0, so rewriting a job's steps: into flow style was an in-band way to switch that job's check off with nothing in the output to say so. This PR makes the lint say it out loud, in a ::warning that shows up in the PR annotations and does not fail the run.

What changed

check_dir now returns a fourth value, notices — annotation-ready strings that never touch the exit status — and main prints them after the checked … lines and before any errors. One ::warning per job, naming the job, pointing at its steps: line, and counting the ref: sites that were passed unjudged. When any warning fired, the OK summary drops its "every ref checkout guarded" claim for "every JUDGED ref checkout guarded (…); N job(s) skipped the dangling-ref check". return 1 if errors else 0 is unchanged, and the drop itself is unchanged — a fail-closed escape would manufacture false CI failures out of a pre-scan that could not run, which is the failure mode this deliberately does not trade for.

It fires only where the escape actually cost a site its verdict. A flow-steps: job that never reads a steps.<id>.outputs.<name> never reaches the drop branch and stays silent, and neither does a site that still came away with a verdict because a sibling operand named a tracked resolver — so one ref: line can never carry both an ::error and a ::warning saying it went unjudged. A warning marks lost coverage rather than a shape, and says so: the message reads "this is lost coverage, not a finding", because with the pre-scan defeated a dangling id and a real earlier out-of-scope step are indistinguishable and the readable path drops the second one silently too.

Collection rides on the walk check_dir already makes for the self-pin carve-out, as an optional dropped= out-parameter on ref_checkouts — no third walk, and no return-shape change to ref_checkouts / unguarded_ref_checkouts / find_unguarded_ref_checkouts (49 call sites read those). _record_steps_output notes the escape at the existing if step_ids is None: branch and appends (job_start, job_indent, idx) once, after the loop, only for a site it leaves with no found entry; nothing else in the verdict logic moved. The out-parameter is one bundle, drop=(job_start, job_indent, out_list), so the list cannot travel without the job coordinates the annotation needs.

Every annotation check_dir builds — the nine pre-existing ::error lines as well as the new ::warning — now runs its path/name through _ann_prop / _ann_msg. Those values come from a directory listing, and git permits newlines and commas in a filename; unescaped, a crafted name walks out of file= or starts a log line with ::stop-commands:: and suppresses every annotation after it — which, now that notices print ahead of the errors, means the error annotations. A no-op for every name a real workflow file has.

.github/workflows/test-workflow-pins.yml gains a third smoke step alongside the two that already pin the failing paths, because either half of this is a silent regression on its own: a warning that starts failing the run, or a drop that goes quiet again. It asserts exit 0 and ::warning in the captured stdout.

Judgment calls / deviations from the plan

The count's unit. The plan's message said "%d checkout(s)". The counter's actual unit is the ref: line — _record_steps_output appends at most one found entry per call, keyed on the line — so two flow-style checkouts written on one physical line count once, which is exactly the shape that triggers this warning. Rather than print a number a reader can falsify against the job's step count, the message says "%d `ref:` site(s) in this job — one per `ref:` LINE, this lint's unit throughout — and the checkout(s) they feed were passed unjudged". Documented in the README and pinned by test_a_warning_counts_the_sites_the_job_lost (a flow sequence broken over two physical lines counts 2). This is a documented property of the annotation, not an unfixed defect: no site is lost, only merged in the count.
Job-name extraction. The plan said lines[job_start].strip().rstrip(':'). Two things are wrong with that trim. build: # the mapper is legal YAML and it prints the comment inside the job's name; and build: &common — an anchor on the key line, which is one of the three escape shapes this very message enumerates, so a routine way to reach the annotation — prints "job `build: &common`". The name now goes through the module's quote-aware _strip_comment and is then cut at the key's own mapping colon, found on the shared _quote_mask scan so a quoted key that contains one ("deploy: prod":) is cut at its delimiter rather than inside its name. Output is identical for every ordinary key.
De-duplication. The collector records once per site — a ref: line, the unit _record_steps_output itself works in — and only for a site left with no found entry; check_dir still groups by job and counts distinct idx, so the count stays a count of lines if a line is ever walked twice.
One extra test call site beyond the 28. test_this_repos_own_workflows_pass now binds notices and asserts == [], turning "zero warnings on the real tree" from a one-off manual check into a regression gate.
Five new tests, not four — the four named in the plan plus the site-count one above.
The smoke step captures stdout to "$dir/out.txt" rather than out.txt in the checkout root.

Verification

python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' → 265 passed, 0 failed (255 before this change, +10). The other three suites in this repo (cursor-review 194, groom 365, agents-md-integrity) pass unchanged, and shellcheck -x on the bump-callers scripts is clean.
python3 .github/workflow-pins/check_workflow_pins.py on the real tree → exit 0, zero warnings, summary unchanged verbatim. Swept every steps: key in .github/workflows/: 68 total, 67 bare block sequences, 1 value-on-key-line — and that one is the fixture inside the new smoke step's heredoc, not a real job. So nothing in this tree is currently escaping the pre-scan, and the not-fixed portion of that sweep is zero.
Repro, both directions. The fixture from the report (steps: [{id: resolve, run: echo hi}, {uses: actions/checkout@abc, with: {ref: "${{ steps.typo.outputs.ref }}"}}]) now prints the ::warning and still exits 0. The identical typo'd id under a block-sequence steps: still produces the existing dangling error and no warning (test_a_readable_steps_job_emits_no_notice) — so the message's claim "this job was not judged" is falsifiable, and it checks out in the direction that proves the message right rather than merely asserting it.
Mutation check. Four independent mutations, each caught by exactly the intended test and nothing else: reverting the job-name cut to the trailing-: strip; making it a naive first-colon cut (caught by the quoted-key test); making the drop per-operand and unconditional again; removing the annotation escaping.
The new smoke step was extracted from the workflow and run locally end to end (exit 0, ::warning present); the workflow file parses as YAML.

Not exercised

The investigation ticket this cites (BE-9045) and its findings comment live in Linear, which this run has no access to. The reproduction above was rebuilt from the description alone and confirmed here against the real checker, so the behavior is verified first-hand; the original evidence comment is not. The precondition — that #221 (fix(workflow-pins): scope the step-id pre-scan to the step key column) is merged — was exercised: it is on main as 9efb339, this branch is based on it, and the None escape it introduces is what the new tests fire on.

AGENTS.md fails check_agents_md.py --root . at 305 lines against a 200-line ceiling. That is pre-existing on main (identical there), this repo is not self-enrolled in that check, and nothing in this diff touches it.

Review round (post-panel)

Four of the five panel findings are fixed in 952d1b9 (job-name cut at the mapping colon; record once per unjudged site rather than per operand; bundle the out-parameter so it cannot travel without its job coordinates; escape annotation values). The fifth — a steps: line carrying both a WORKFLOWS_REF binding and a step-output ref: — reproduced as an upstream site-detection gap, not a dodge of this notice: the site is recorded neither in found nor in dropped even under a fully readable block steps:, so the notice is silent because nothing was dropped, because nothing was ever found. Closing it changes verdicts, which this PR's contract excludes; deferred to a follow-up with the reproduction.

One thing a human should settle: the branch, PR title and commit subjects say BE-9064, while every in-code and README reference — including the user-facing "See BE-9045." in the warning itself — says BE-9045. One of the two is wrong and this run holds no Linear access to tell which, so nothing was renamed.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 265 passed, 0 failed; the repo's other suites (cursor-review 194, groom 365, agents-md-integrity) pass and shellcheck -x on the bump-callers scripts is clean; python3 .github/workflow-pins/check_workflow_pins.py on the real tree: exit 0, 11 workflows checked, 0 warnings, 0 errors, summary line unchanged verbatim; four mutation checks each caught by exactly the intended test and no other; the CI smoke step run locally: exit 0 with ::warning present.
Deviations: the judgment calls listed under "Judgment calls / deviations from the plan" above; every acceptance criterion in the plan is met. The panel's fifth finding is deferred rather than fixed, for the reason given under "Review round" above.

_{🔄 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/222 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/25/2026 **Status:** ✅ Merged **Merged:** 8/25/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9064-workflow-pins-escaped-steps-warning` --- ### 📝 Commits (2) - [`daab3c3`](https://github.com/Comfy-Org/github-workflows/commit/daab3c305a56f0497def201e7d32d6cf029c4762) feat(workflow-pins): warn when a job's `steps:` escapes the step-id pre-scan (BE-9064) - [`952d1b9`](https://github.com/Comfy-Org/github-workflows/commit/952d1b9b11c2337770e1fdc20621bf6aea2a56bc) fix(workflow-pins): tighten the escaped-`steps:` warning after review (BE-9064) ### 📊 Changes **4 files changed** (+545 additions, -49 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+21 -2) 📝 `.github/workflow-pins/check_workflow_pins.py` (+223 -19) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+263 -28) 📝 `.github/workflows/test-workflow-pins.yml` (+38 -0) </details> ### 📄 Description ## ELI-5 The pin lint has one job: make sure no reusable workflow here can check out at a mutable ref. One part of it — the dangling-ref check — quietly gives up on a job whose `steps:` is written in a shape it can't read (`steps: [ … ]` on the key line, an anchor, or no `- ` item below it). Giving up is the right call: guessing there would fail perfectly good workflows. The problem was that it gave up **silently** — the lint printed "every ref checkout guarded" and exited 0, so rewriting a job's `steps:` into flow style was an in-band way to switch that job's check off with nothing in the output to say so. This PR makes the lint say it out loud, in a `::warning` that shows up in the PR annotations and does not fail the run. ## What changed `check_dir` now returns a fourth value, `notices` — annotation-ready strings that never touch the exit status — and `main` prints them after the `checked …` lines and before any errors. One `::warning` per job, naming the job, pointing at its `steps:` line, and counting the `ref:` sites that were passed unjudged. When any warning fired, the OK summary drops its "every ref checkout guarded" claim for "every JUDGED ref checkout guarded (…); N job(s) skipped the dangling-ref check". `return 1 if errors else 0` is unchanged, and the drop itself is unchanged — a fail-closed escape would manufacture false CI failures out of a pre-scan that could not run, which is the failure mode this deliberately does not trade for. It fires **only where the escape actually cost a site its verdict**. A flow-`steps:` job that never reads a `steps.<id>.outputs.<name>` never reaches the drop branch and stays silent, and neither does a site that still came away with a verdict because a sibling operand named a tracked resolver — so one `ref:` line can never carry both an `::error` and a `::warning` saying it went unjudged. A warning marks lost coverage rather than a shape, and says so: the message reads "this is lost coverage, not a finding", because with the pre-scan defeated a dangling id and a real earlier out-of-scope step are indistinguishable and the readable path drops the second one silently too. Collection rides on the walk `check_dir` already makes for the self-pin carve-out, as an optional `dropped=` out-parameter on `ref_checkouts` — no third walk, and no return-shape change to `ref_checkouts` / `unguarded_ref_checkouts` / `find_unguarded_ref_checkouts` (49 call sites read those). `_record_steps_output` notes the escape at the existing `if step_ids is None:` branch and appends `(job_start, job_indent, idx)` once, after the loop, only for a site it leaves with **no** `found` entry; nothing else in the verdict logic moved. The out-parameter is one bundle, `drop=(job_start, job_indent, out_list)`, so the list cannot travel without the job coordinates the annotation needs. Every annotation `check_dir` builds — the nine pre-existing `::error` lines as well as the new `::warning` — now runs its `path`/`name` through `_ann_prop` / `_ann_msg`. Those values come from a directory listing, and git permits newlines and commas in a filename; unescaped, a crafted name walks out of `file=` or starts a log line with `::stop-commands::` and suppresses every annotation after it — which, now that notices print ahead of the errors, means the error annotations. A no-op for every name a real workflow file has. `.github/workflows/test-workflow-pins.yml` gains a third smoke step alongside the two that already pin the failing paths, because either half of this is a silent regression on its own: a warning that starts failing the run, or a drop that goes quiet again. It asserts exit 0 **and** `::warning` in the captured stdout. ## Judgment calls / deviations from the plan - **The count's unit.** The plan's message said "%d checkout(s)". The counter's actual unit is the `ref:` **line** — `_record_steps_output` appends at most one `found` entry per call, keyed on the line — so two flow-style checkouts written on one physical line count once, which is exactly the shape that triggers this warning. Rather than print a number a reader can falsify against the job's step count, the message says "%d \`ref:\` site(s) in this job — one per \`ref:\` LINE, this lint's unit throughout — and the checkout(s) they feed were passed unjudged". Documented in the README and pinned by `test_a_warning_counts_the_sites_the_job_lost` (a flow sequence broken over two physical lines counts 2). This is a documented property of the annotation, not an unfixed defect: no site is lost, only merged in the count. - **Job-name extraction.** The plan said `lines[job_start].strip().rstrip(':')`. Two things are wrong with that trim. ` build: # the mapper` is legal YAML and it prints the comment inside the job's name; and ` build: &common` — an anchor on the key line, which is *one of the three escape shapes this very message enumerates*, so a routine way to reach the annotation — prints "job \`build: &common\`". The name now goes through the module's quote-aware `_strip_comment` and is then cut at the key's **own** mapping colon, found on the shared `_quote_mask` scan so a quoted key that contains one (`"deploy: prod":`) is cut at its delimiter rather than inside its name. Output is identical for every ordinary key. - **De-duplication.** The collector records once per **site** — a `ref:` line, the unit `_record_steps_output` itself works in — and only for a site left with no `found` entry; `check_dir` still groups by job and counts distinct `idx`, so the count stays a count of lines if a line is ever walked twice. - **One extra test call site beyond the 28.** `test_this_repos_own_workflows_pass` now binds `notices` and asserts `== []`, turning "zero warnings on the real tree" from a one-off manual check into a regression gate. - **Five new tests, not four** — the four named in the plan plus the site-count one above. - The smoke step captures stdout to `"$dir/out.txt"` rather than `out.txt` in the checkout root. ## Verification - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` → **265 passed, 0 failed** (255 before this change, +10). The other three suites in this repo (`cursor-review` 194, `groom` 365, `agents-md-integrity`) pass unchanged, and `shellcheck -x` on the bump-callers scripts is clean. - `python3 .github/workflow-pins/check_workflow_pins.py` on the real tree → exit 0, **zero warnings**, summary unchanged verbatim. Swept every `steps:` key in `.github/workflows/`: **68 total, 67 bare block sequences, 1 value-on-key-line** — and that one is the fixture inside the new smoke step's heredoc, not a real job. So nothing in this tree is currently escaping the pre-scan, and the not-fixed portion of that sweep is zero. - **Repro, both directions.** The fixture from the report (`steps: [{id: resolve, run: echo hi}, {uses: actions/checkout@abc, with: {ref: "${{ steps.typo.outputs.ref }}"}}]`) now prints the `::warning` and still exits 0. The identical typo'd id under a block-sequence `steps:` still produces the existing dangling error and **no** warning (`test_a_readable_steps_job_emits_no_notice`) — so the message's claim "this job was not judged" is falsifiable, and it checks out in the direction that proves the message right rather than merely asserting it. - **Mutation check.** Four independent mutations, each caught by exactly the intended test and nothing else: reverting the job-name cut to the trailing-`:` strip; making it a naive first-colon cut (caught by the quoted-key test); making the drop per-operand and unconditional again; removing the annotation escaping. - The new smoke step was extracted from the workflow and run locally end to end (exit 0, `::warning` present); the workflow file parses as YAML. ## Not exercised The investigation ticket this cites (BE-9045) and its findings comment live in Linear, which this run has no access to. The reproduction above was rebuilt from the description alone and confirmed here against the real checker, so the behavior is verified first-hand; the original evidence comment is not. The precondition — that #221 (`fix(workflow-pins): scope the step-id pre-scan to the step key column`) is merged — **was** exercised: it is on `main` as `9efb339`, this branch is based on it, and the `None` escape it introduces is what the new tests fire on. `AGENTS.md` fails `check_agents_md.py --root .` at 305 lines against a 200-line ceiling. That is pre-existing on `main` (identical there), this repo is not self-enrolled in that check, and nothing in this diff touches it. ## Review round (post-panel) Four of the five panel findings are fixed in `952d1b9` (job-name cut at the mapping colon; record once per unjudged site rather than per operand; bundle the out-parameter so it cannot travel without its job coordinates; escape annotation values). The fifth — a `steps:` line carrying both a `WORKFLOWS_REF` binding and a step-output `ref:` — reproduced as an **upstream site-detection** gap, not a dodge of this notice: the site is recorded neither in `found` nor in `dropped` even under a fully readable block `steps:`, so the notice is silent because nothing was dropped, because nothing was ever found. Closing it changes verdicts, which this PR's contract excludes; deferred to a follow-up with the reproduction. **One thing a human should settle:** the branch, PR title and commit subjects say **BE-9064**, while every in-code and README reference — including the user-facing "See BE-9045." in the warning itself — says **BE-9045**. One of the two is wrong and this run holds no Linear access to tell which, so nothing was renamed. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'`: 265 passed, 0 failed; the repo's other suites (`cursor-review` 194, `groom` 365, `agents-md-integrity`) pass and `shellcheck -x` on the bump-callers scripts is clean; `python3 .github/workflow-pins/check_workflow_pins.py` on the real tree: exit 0, 11 workflows checked, 0 warnings, 0 errors, summary line unchanged verbatim; four mutation checks each caught by exactly the intended test and no other; the CI smoke step run locally: exit 0 with `::warning` present. - **Deviations:** the judgment calls listed under "Judgment calls / deviations from the plan" above; every acceptance criterion in the plan is met. The panel's fifth finding is deferred rather than fixed, for the reason given under "Review round" above. --- <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:34 +00:00