[PR #221] [MERGED] fix(workflow-pins): scope the step-id pre-scan to the step key column (BE-8254) #226

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/221
Author: @mattmillerai
Created: 8/24/2026
Status: ✅ Merged
Merged: 8/25/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8254-workflow-pins-step-id-prescan

📝 Commits (4)

3da470e fix(workflow-pins): scope the step-id pre-scan to the step key column (BE-8254)
a91ec91 fix(workflow-pins): read every YAML spelling of a step item in the pre-scan (BE-8254)
0062a73 fix(workflow-pins): refuse a quoted comma as a flow step boundary (BE-8254)
be0b1d0 fix(workflow-pins): open a quoted scalar only where a YAML node can start (BE-8254)

📊 Changes

3 files changed (+1036 additions, -58 deletions)

View changed files

📝 .github/workflow-pins/README.md (+74 -0)
📝 .github/workflow-pins/check_workflow_pins.py (+424 -58)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+538 -0)

📄 Description

ELI-5

The pin checker keeps a list of "step ids that exist in this job" so it can tell a ref: ${{ steps.x.outputs.ref }} that points at a real earlier step (fine, not our business) from one that points at nothing (broken — at runtime it's '', and actions/checkout reads '' as the default branch, so the checkout silently runs at a mutable ref). It built that list by grepping every line of the job for anything shaped like id:. So a line that merely looks like a step id — an action input literally named id, or a line of YAML a shell script prints into a heredoc — got added to the list, and a genuinely broken ref then resolved against that phantom and was reported as nothing at all. Now the checker walks the job's actual step items and reads each id at the column a step declares one, so lines that aren't step declarations can't get in.

What changed

_job_step_ids is rewritten from a flat line scan into a walk of the job's steps: items:

The id is read exactly where _binding_step_id reads it — at the item's own key column, with a key written on the - marker line normalized back into that column. This excludes both silencing shapes structurally rather than by pattern: YAML puts a with: member and a block scalar's body deeper than the step's own keys either way.
Block scalars (run: |, script: >-) are additionally skipped outright — belt-and-braces for odd indentation-indicator edges.
Flow spellings are read only where a flow mapping actually opens: a marker line whose value begins { (- {id: x, run: …}), and the continuation lines of one spanning several physical lines (- {uses: x, / id: y}), tracked by unbalanced braces.
Fail-loud escape. A steps: the walk cannot stand behind — flow-style steps: [ … ] on the key line, or opening no - item — returns None ("unknown", not "empty"). _record_steps_output then drops that job's sites whose id names no tracked resolver, exactly as this reader did before the dangling check existed. That is every verdict resting on "no step of that id exists": dangling, and the non-leading one an absent id also reaches — the same pair a real earlier out-of-scope step drops today. Resolver and guard verdicts are untouched, in that job and every other.

Also updated: .github/workflow-pins/README.md, which documents this subsystem's contract and now describes both new behaviors.

Verification

python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' — 239 passed, 0 failed (216 before; 23 new).
python3 .github/workflow-pins/check_workflow_pins.py — green on the real tree, exit 0, 11 workflows checked, 0 exempt.
Falsification of the new tests: 8 of the 12 fail against the pre-fix check_workflow_pins.py from main — precisely the silencing shapes. The other 4 are regression pins on behavior that had to be preserved (the two existing flow fixtures, the marker-line id, the caller-job shape).
Fifteen-way mutation battery, each reverting one branch of the change: the seven of the first round (the key-column gate, the escape sentinel, the caller's handling of it, the marker-line normalization, the flow-continuation read, the flow-mapping-only gate, the caller-job {} answer) and the eight added in review (below). Each fails exactly the test(s) that name it and nothing else — so no branch here is unpinned.
Other suites (unaffected by this path-filtered diff) re-run anyway: agents-md-integrity 46, cursor-review 194, groom 365, public-repo-hygiene 165, refresh-reviewers 48 — all green.

Corpus sweep — including the half this does not change

Swept all 48 workflow-shaped YAML files / 69 jobs in the tree, running the old flat scan and the new item walk side by side and diffing their id sets per job:

	count
phantom ids no longer registered	0
real ids lost by the new walk	0
jobs hitting the fail-loud escape	0

So this is a verified no-op on today's tree — a latent robustness fix, which matches the read-only investigation it came from ("no phantom shape in today's tree"). The heredoc-heavy workflows the silencing shape would most plausibly hit are covered by that sweep and land identically: groom.yml (3,308 lines, 7 jobs) and cursor-review.yml (2,133 lines, 9 jobs) produce byte-identical id sets before and after.

Review round — every YAML spelling of a step item

The four-model panel found that walking items instead of lines had traded the two phantom shapes for several shapes the walk could no longer read. Every one is the costly direction (a missed id is a false dangling FAILURE on a compliant workflow, or an escaped job that drops a real finding), so all were reproduced first, then fixed:

finding	shape	was	now
High	indentless sequence — `steps:` and `- uses: …` both at column 4 (valid YAML; Actions accepts it)	no item found strictly deeper than the key → whole job escaped, a third fail-open shape the docs did not enumerate	sequence looked for at or deeper than the key's indent, and closed by the first non-item line back at the marker column (in that style `steps:`'s siblings share it)
Medium	braces inside quoted scalars — `- {run: "echo {"}` / `- {name: "}"}`	raw `text.count("{")` wedged flow mode open (or closed it early); while open, a heredoc's `- id: x` registered the exact phantom this PR removes, and a real `- id: resolve_ref` matched neither pattern applied there and was lost	new `_flow_brace_delta`, quote-aware on the same rules `_strip_comment` uses, at both update sites
Low	comment-only marker line — `- # set up`	marker width measured on the raw line locked `key_column` to the comment's column; the item's real `id:` was never read	width measured on the comment-stripped line, so it reduces to the bare-`-` path and the members below name the column
Low	bare `-` whose item is a flow mapping on the next line	only the block pattern applied; a line opening `{` matched nothing	that branch makes the same "begins `{`" test the marker-line branch makes and enters flow tracking
Low	flow continuation dedented past the dash (legal — `{ … }` constrains no indentation)	ended the entire pre-scan; every id below it lost	the sequence-closing break is suppressed while a mapping is open — but never past the job, which bounds a `flow_depth` a residual miscount could still leave stuck

Two findings were answered without changing behavior:

Docs, not code. The escape continues ahead of the leading test, so it costs the non-leading verdict as well as dangling — and three places said "only the dangling verdict". Judging leading first would be worse: with a readable steps:, a site whose id names a real earlier out-of-scope step is dropped regardless of leading, so printing non-leading under the escape trades a dropped site for a false failure. The call-site comment, both docstrings and the README now state the escape's real reach.
Residual pinned, not narrowed. - {uses: x, with: {id: y}} still registers y. Scoping the flat pattern to the outermost mapping's depth is the under-collection direction; a test now names the shape so any future change here is a deliberate one. 0 occurrences in the tree.

Deferred to a follow-up (noted in the README): making the fail-open observable. Reformatting a job's steps: is an in-band way to switch its dangling check off with nothing in the output saying so — but every path in check_dir appends to errors, and routing the escape there turns a deliberate fail-open into a fail-closed. It needs a non-fatal notices channel, which changes check_dir's return contract, the CLI and the test job; none are in this diff's scope.

Re-verified after the review round: 239 tests pass; an eight-mutation battery over the new branches (indentless marker_column, the sibling-column terminator, the quote-aware brace count at each of its two sites, the comment-stripped marker width, the bare-dash flow entry, the flow-dedent suppression, and its job bound) each fails exactly the test(s) naming it — two of them (the job bound, and the continuation-line brace count) came back green on the first pass, which is how the two tests covering them were found to be too weak and tightened; the corpus sweep re-run against the pre-review implementation shows 0 differences across 50 files / 69 jobs and 0 escapes either way — still a verified no-op on today's tree; check_workflow_pins.py exit 0; and both CI smoke tests still reject, including the unguarded fixture rewritten in the indentless style.

Review round two — the quote scanner itself

The panel came back on the reader that the first review round added. Five findings; four fixed, one deferred with a ticket.

finding	shape	was	now
Medium (5/8 reviewers)	a quote inside a PLAIN scalar — `- {name: don't, id: real}`, legal YAML (a quote is forbidden only as a plain scalar's FIRST character)	the apostrophe opened string state, so the real entry comma before `id:` read as content and `real` was never registered, while the closing `}` went uncounted and wedged `flow_depth` open — the whole job's id set came back `{}`, making every site in it a false `dangling`	`_opens_quoted_scalar` gates every opener on a position a node can actually begin at (start of text, or after `{`, `[`, `,`, a `:` separator, or a `-` marker)
Low (3/8)	a quoted scalar spanning physical lines, both directions	`- {name: "a` / `id: phantom", …}` registered a phantom (silencing a real verdict); `- {name: "two` / `lines", id: real}` lost the real id	both fixed, not just pinned — the lost-id half falls out of the scalar-start rule; the phantom half is refused from the VALUE side (`_step_id_value`: an Actions step id is `[A-Za-z0-9_-]`, so a stray quote marks the tail of a scalar opened above)
Low (2/8)	a node property before the mapping — `- &resolver {id: x, …}`	the `{`-immediately-after-the-marker gate missed it, the item fell to a block pattern that cannot match a line beginning `&resolver {`, and its id was lost	`_after_node_properties` skips leading `&anchor`/`!tag` at both flow-entry sites
Low (2/8)	`_outside_quotes` rescanned from column 0 per match	O(matches x line length) on one long flow line	one `_quote_mask` pass per line, shared by all three readers — 3200 candidates: 1009 ms → 1.7 ms, same ids out

A regression the fix itself introduced, caught before it shipped. Applying the scalar-start rule to _strip_comment too — which shares the scan — changed 11 of the tree's 16,359 physical lines, all run: | script text like echo "Updated PR #${existing}": strictly, that " follows the plain word echo and opens nothing, so the # reads as a comment opener and truncates a line whose tail may carry the very ref: this lint looks for. _strip_comment is asked about every physical line and cannot tell YAML from shell, so it deliberately keeps the weaker reading and is pinned to it; only the structural readers, never asked about script text, take the strict rule. _strip_comment is now byte-identical to before across the whole tree.

One finding deferred, with a ticket rather than a reply. _in_steps_sequence was never taught the indentless spelling the pre-scan learned in round one, so in such a job every step has no bounds and a step consuming its OWN output is silently dropped — reproduced: the identical job reports [(False, False, 'dangling')] indented and [] indentless. Not fixed here because _in_steps_sequence is the step-BOUNDS subsystem gating resolver registration and is_guard_step, and this PR's stated invariant is that resolver and guard verdicts are untouched in every job. The obvious fix also opens a fail-open hole (a run: | heredoc emitting steps: / - id: x at one column would be credited as a real sequence); the follow-up carries the reproduction, that trap, the alternative of keying the boundary off the pre-scan's own item boundaries, and acceptance criteria.

Residue that survives, pinned not claimed closed: a flow continuation line whose id value carries no quote at all (id: phantom, more":) still over-collects. Closing it needs real cross-line quote state, which is a module-wide contract — every reader here is per-line, _strip_comment included, and it runs first — so it stays out of scope rather than being half-done in one walk. 0 occurrences in the tree.

Re-verified after round two: 255 tests pass (239 before; 16 new, of which 9 fail against the pre-round-two implementation and the rest pin preserved behaviour). Seven-way mutation battery over the new branches — the scalar-start rule, its list-marker case, the node-property skip at each of its two sites, the step-id quote refusal, its matched-quote unwrap, and _strip_comment's deliberate weaker reading — each mutation failing exactly the test(s) naming it, no survivors (one, the matched-quote unwrap, survived the first pass, which is how the test covering it was found missing and added). Full-pipeline differential across all 50 workflow-shaped files / 69 jobs, run three ways — origin/main, this PR's previous tip, and this commit all produce identical ref_checkouts and find_unguarded_ref_checkouts output, 0 diffs, 0 escapes — so this is still a verified no-op on today's tree. check_workflow_pins.py exit 0, 11 workflows checked. Every other suite green (agents-md-integrity 46, cursor-review 194, groom 365, public-repo-hygiene 165, refresh-reviewers 48).

Judgment calls and residuals

marker column + 2 was specified; I used the measured marker width instead. The repo's own WIDE_MARKER_RESOLVER fixture pins - id: resolve_ref (dash + 3), which a hardcoded + 2 reads one column shallow — the exact hole _marker_width was extracted to close. Using _marker_width keeps this reader agreeing with _binding_step_id and _step_bounds; a new test pins both separation widths.
The flow pattern is applied more narrowly than specified — only to marker lines whose value begins {, not to every marker line. Applied to every marker line it also matches - with: {id: x}, which is the flow spelling of silencing shape 1; the narrowing is pinned by a test covering both the member and marker spellings.
Residual over-collection inside a flow mapping. - {with: {id: x}, uses: y} still registers x, because _STEP_ID_FLOW_RE is a flat pattern that cannot see nesting. This is the tolerated direction by design (over-collection reproduces the old fail-open drop for one site; under-collection manufactures a false CI failure) and it is the contract the rewrite was specified to keep — but it is a residual on _job_step_ids / _STEP_ID_FLOW_RE, so this PR does not claim the pre-scan is exhaustively scoped, and the tracking ticket stays open rather than auto-closing. No occurrence exists in the tree today (0 of 69 jobs).
The explicit block-scalar skip is redundant on every shape I could construct, and I say so rather than implying it is load-bearing: deleting it alone keeps all 228 tests green, because YAML already forces a scalar's body deeper than its key, where the key-column gate drops it. It earns its place only as the second of two independent defenses — deleting the column gate alone leaves the heredoc tests passing on the strength of this skip. I kept it deliberately, per the specified design.
~~Brace counting for the multi-line flow form is not quote-aware.~~ Fixed in review round one — and the claim that bounded it was wrong: while flow mode is stuck open a real block item - id: resolve_ref matches neither pattern applied there, so it can lose a real id and manufacture a failure. _flow_brace_delta now counts outside quoted scalars. ~~A quote spanning physical lines is still out of scope~~ — also closed in round two, from the value side: _step_id_value refuses an id still carrying a quote after its closing punctuation and one matched wrapper are stripped, and the mirror direction falls out of the scalar-start rule. The walk's job bound on a stuck flow_depth remains explicit and test-pinned. One narrower residue survives and is pinned (see round two above).
Capability-denial check. The escape path's user-visible outcome is "no dangling verdict for this job", so I falsified it rather than asserting it: it is a fail-open drop restoring this lint's pre-existing coverage, not a new refusal, and I confirmed empirically via the product's own entry point (check_workflow_pins.py over the real tree, exit 0) and the 69-job sweep that 0 jobs reach it — no workflow loses a verdict it has today.
Pre-existing, untouched, not caused by this diff: check_agents_md.py --root . fails on AGENTS.md being 305 lines (ceiling 200) — identical on origin/main, and this diff does not touch that file. .github/coderabbit-config/tests errors locally on a missing jsonschema dependency — also identical on origin/main, an environment gap, and outside this diff's path filter.

Unexercised artifacts

The source ticket's Provenance block links a Linear issue and a run dashboard/transcript on an internal host; this worker holds no Linear access and those are unreachable from it, so neither was read. The ticket's description carried the full mechanism and the recommended implementation, and every shape it names was reproduced as a failing test before it was fixed, so nothing in those links was needed to establish acceptance. The linked GitHub review-thread comment was likewise not fetched; its content is restated in the ticket body I worked from.

Refs BE-8254

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 255 passed, 0 failed (239 before review round two; 16 new); python3 .github/workflow-pins/check_workflow_pins.py: exit 0, 11 workflows checked, 0 exempt; all five round-two findings reproduced as failures BEFORE being fixed (the plain-scalar quote collapsing a job's id set to {}; both cross-line-quote directions; the &anchor item losing its id; the indentless job dropping a self-consuming step's dangling verdict); 9 of the 16 new tests confirmed failing against the pre-round-two implementation, the other 7 pinning preserved behaviour; seven-way mutation battery over the round-two branches, each mutation failing exactly the test(s) naming it and none surviving — one survived the first pass, which is how the missing matched-quote-unwrap test was found and added; full-pipeline differential across all 50 workflow-shaped files / 69 jobs run three ways (origin/main, the PR's previous tip, this commit) — identical ref_checkouts and find_unguarded_ref_checkouts output, 0 diffs, 0 escapes, still a verified no-op on today's tree; _strip_comment differential over all 16,359 physical lines in the tree — 0 differ, which is how the 11-line run: | script-text regression an over-broad first attempt introduced was caught and reverted; quote-scan hoist measured at 200/800/3200 flow id: candidates (3.4/59.8/1009 ms → 0.09/0.42/1.7 ms) with identical ids out; every other suite in the repo re-run green (agents-md-integrity 46, cursor-review 194, groom 365, public-repo-hygiene 165, refresh-reviewers 48). Note the earlier review commit message says "ten-mutation battery", which miscounts the eight of that round.
Deviations: four, all narrowing and all test-pinned. Three from the first rounds: measured marker width instead of the specified marker column + 2 (the repo's own dash+3 fixture proves +2 wrong); the flow pattern applied only where a flow mapping actually opens rather than to all marker lines (the wider form re-admits the - with: {id: x} silencing shape); and one finding answered in documentation rather than code (the escape costs the non-leading verdict too — judging leading ahead of it would trade a dropped site for a false failure). New in round two: _strip_comment deliberately does NOT take the scalar-start rule the other readers take, because it is asked about run: | script bodies where the strict reading truncates real content — the split is pinned by its own test and by a mutation. Two items are deferred rather than done: making the fail-open observable (needs a non-fatal notices channel check_dir does not have), and teaching _in_steps_sequence the indentless spelling (it is the step-bounds subsystem this PR pledges to leave untouched, and the naive fix opens a heredoc fail-open) — both carry a written plan. No acceptance criterion was left unmet.

_{🔄 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/221 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/24/2026 **Status:** ✅ Merged **Merged:** 8/25/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8254-workflow-pins-step-id-prescan` --- ### 📝 Commits (4) - [`3da470e`](https://github.com/Comfy-Org/github-workflows/commit/3da470ef676f91b94be0f8d21573aaa1d65fccde) fix(workflow-pins): scope the step-id pre-scan to the step key column (BE-8254) - [`a91ec91`](https://github.com/Comfy-Org/github-workflows/commit/a91ec9118f9d24192afdb635080dbe2a64260e18) fix(workflow-pins): read every YAML spelling of a step item in the pre-scan (BE-8254) - [`0062a73`](https://github.com/Comfy-Org/github-workflows/commit/0062a73af5bd74d4e9bb384bc54b8532870934f9) fix(workflow-pins): refuse a quoted comma as a flow step boundary (BE-8254) - [`be0b1d0`](https://github.com/Comfy-Org/github-workflows/commit/be0b1d03727cf0a32e83549d391cf65c806cc055) fix(workflow-pins): open a quoted scalar only where a YAML node can start (BE-8254) ### 📊 Changes **3 files changed** (+1036 additions, -58 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+74 -0) 📝 `.github/workflow-pins/check_workflow_pins.py` (+424 -58) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+538 -0) </details> ### 📄 Description ## ELI-5 The pin checker keeps a list of "step ids that exist in this job" so it can tell a `ref: ${{ steps.x.outputs.ref }}` that points at a real earlier step (fine, not our business) from one that points at nothing (broken — at runtime it's `''`, and `actions/checkout` reads `''` as *the default branch*, so the checkout silently runs at a mutable ref). It built that list by grepping every line of the job for anything shaped like `id:`. So a line that merely *looks* like a step id — an action input literally named `id`, or a line of YAML a shell script prints into a heredoc — got added to the list, and a genuinely broken ref then resolved against that phantom and was reported as nothing at all. Now the checker walks the job's actual step items and reads each id at the column a step declares one, so lines that aren't step declarations can't get in. ## What changed `_job_step_ids` is rewritten from a flat line scan into a walk of the job's `steps:` **items**: - The id is read exactly where `_binding_step_id` reads it — at the item's own key column, with a key written on the `- ` marker line normalized back into that column. This excludes both silencing shapes *structurally* rather than by pattern: YAML puts a `with:` member and a block scalar's body deeper than the step's own keys either way. - Block scalars (`run: |`, `script: >-`) are additionally skipped outright — belt-and-braces for odd indentation-indicator edges. - Flow spellings are read only where a flow mapping actually opens: a marker line whose value begins `{` (`- {id: x, run: …}`), and the continuation lines of one spanning several physical lines (`- {uses: x,` / `id: y}`), tracked by unbalanced braces. - **Fail-loud escape.** A `steps:` the walk cannot stand behind — flow-style `steps: [ … ]` on the key line, or opening no `- ` item — returns `None` ("unknown", not "empty"). `_record_steps_output` then drops that job's sites whose id names no tracked resolver, exactly as this reader did before the dangling check existed. That is every verdict resting on "no step of that id exists": `dangling`, **and** the `non-leading` one an absent id also reaches — the same pair a real earlier out-of-scope step drops today. Resolver and guard verdicts are untouched, in that job and every other. Also updated: `.github/workflow-pins/README.md`, which documents this subsystem's contract and now describes both new behaviors. ## Verification - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` — **239 passed, 0 failed** (216 before; 23 new). - `python3 .github/workflow-pins/check_workflow_pins.py` — green on the real tree, **exit 0**, 11 workflows checked, 0 exempt. - **Falsification of the new tests:** 8 of the 12 fail against the pre-fix `check_workflow_pins.py` from `main` — precisely the silencing shapes. The other 4 are regression pins on behavior that had to be preserved (the two existing flow fixtures, the marker-line id, the caller-job shape). - **Fifteen-way mutation battery**, each reverting one branch of the change: the seven of the first round (the key-column gate, the escape sentinel, the caller's handling of it, the marker-line normalization, the flow-continuation read, the flow-mapping-only gate, the caller-job `{}` answer) and the **eight** added in review (below). Each fails exactly the test(s) that name it and nothing else — so no branch here is unpinned. - Other suites (unaffected by this path-filtered diff) re-run anyway: agents-md-integrity 46, cursor-review 194, groom 365, public-repo-hygiene 165, refresh-reviewers 48 — all green. ## Corpus sweep — including the half this does not change Swept **all 48 workflow-shaped YAML files / 69 jobs** in the tree, running the old flat scan and the new item walk side by side and diffing their id sets per job: | | count | |---|---| | phantom ids no longer registered | **0** | | real ids lost by the new walk | **0** | | jobs hitting the fail-loud escape | **0** | So this is a **verified no-op on today's tree** — a latent robustness fix, which matches the read-only investigation it came from ("no phantom shape in today's tree"). The heredoc-heavy workflows the silencing shape would most plausibly hit are covered by that sweep and land identically: `groom.yml` (3,308 lines, 7 jobs) and `cursor-review.yml` (2,133 lines, 9 jobs) produce byte-identical id sets before and after. ## Review round — every YAML spelling of a step item The four-model panel found that walking *items* instead of *lines* had traded the two phantom shapes for several shapes the walk could no longer read. Every one is the **costly** direction (a missed id is a false `dangling` FAILURE on a compliant workflow, or an escaped job that drops a real finding), so all were reproduced first, then fixed: | finding | shape | was | now | |---|---|---|---| | High | **indentless sequence** — `steps:` and `- uses: …` both at column 4 (valid YAML; Actions accepts it) | no item found strictly deeper than the key → whole job escaped, a third fail-open shape the docs did not enumerate | sequence looked for at or **deeper than** the key's indent, and closed by the first non-item line back at the marker column (in that style `steps:`'s siblings share it) | | Medium | **braces inside quoted scalars** — `- {run: "echo {"}` / `- {name: "}"}` | raw `text.count("{")` wedged flow mode open (or closed it early); while open, a heredoc's `- id: x` registered the exact phantom this PR removes, and a real `- id: resolve_ref` matched *neither* pattern applied there and was lost | new `_flow_brace_delta`, quote-aware on the same rules `_strip_comment` uses, at both update sites | | Low | **comment-only marker line** — `- # set up` | marker width measured on the raw line locked `key_column` to the comment's column; the item's real `id:` was never read | width measured on the comment-stripped line, so it reduces to the bare-`-` path and the members below name the column | | Low | **bare `-` whose item is a flow mapping on the next line** | only the block pattern applied; a line opening `{` matched nothing | that branch makes the same "begins `{`" test the marker-line branch makes and enters flow tracking | | Low | **flow continuation dedented past the dash** (legal — `{ … }` constrains no indentation) | ended the entire pre-scan; every id below it lost | the sequence-closing break is suppressed while a mapping is open — but never past the **job**, which bounds a `flow_depth` a residual miscount could still leave stuck | Two findings were answered without changing behavior: - **Docs, not code.** The escape `continue`s ahead of the `leading` test, so it costs the `non-leading` verdict as well as `dangling` — and three places said "only the dangling verdict". Judging `leading` first would be worse: with a readable `steps:`, a site whose id names a real earlier out-of-scope step is dropped *regardless* of `leading`, so printing `non-leading` under the escape trades a dropped site for a false failure. The call-site comment, both docstrings and the README now state the escape's real reach. - **Residual pinned, not narrowed.** `- {uses: x, with: {id: y}}` still registers `y`. Scoping the flat pattern to the outermost mapping's depth is the under-collection direction; a test now names the shape so any future change here is a deliberate one. 0 occurrences in the tree. Deferred to a follow-up (noted in the README): making the fail-open **observable**. Reformatting a job's `steps:` is an in-band way to switch its dangling check off with nothing in the output saying so — but every path in `check_dir` appends to `errors`, and routing the escape there turns a deliberate fail-open into a fail-closed. It needs a non-fatal `notices` channel, which changes `check_dir`'s return contract, the CLI and the test job; none are in this diff's scope. **Re-verified after the review round:** 239 tests pass; an **eight**-mutation battery over the new branches (indentless `marker_column`, the sibling-column terminator, the quote-aware brace count at each of its two sites, the comment-stripped marker width, the bare-dash flow entry, the flow-dedent suppression, and its job bound) each fails exactly the test(s) naming it — two of them (the job bound, and the continuation-line brace count) came back green on the first pass, which is how the two tests covering them were found to be too weak and tightened; the corpus sweep re-run against the pre-review implementation shows **0 differences across 50 files / 69 jobs and 0 escapes either way** — still a verified no-op on today's tree; `check_workflow_pins.py` exit 0; and both CI smoke tests still reject, including the unguarded fixture rewritten in the indentless style. ## Review round two — the quote scanner itself The panel came back on the reader that the first review round added. Five findings; four fixed, one deferred with a ticket. | finding | shape | was | now | |---|---|---|---| | Medium (5/8 reviewers) | **a quote inside a PLAIN scalar** — `- {name: don't, id: real}`, legal YAML (a quote is forbidden only as a plain scalar's FIRST character) | the apostrophe opened string state, so the real entry comma before `id:` read as content and `real` was never registered, while the closing `}` went uncounted and wedged `flow_depth` open — the whole job's id set came back **`{}`**, making every site in it a false `dangling` | `_opens_quoted_scalar` gates every opener on a position a node can actually begin at (start of text, or after `{`, `[`, `,`, a `:` separator, or a `- ` marker) | | Low (3/8) | **a quoted scalar spanning physical lines**, both directions | `- {name: "a` / `id: phantom", …}` registered a phantom (silencing a real verdict); `- {name: "two` / `lines", id: real}` lost the real id | both **fixed**, not just pinned — the lost-id half falls out of the scalar-start rule; the phantom half is refused from the VALUE side (`_step_id_value`: an Actions step id is `[A-Za-z0-9_-]`, so a stray quote marks the tail of a scalar opened above) | | Low (2/8) | **a node property before the mapping** — `- &resolver {id: x, …}` | the `{`-immediately-after-the-marker gate missed it, the item fell to a block pattern that cannot match a line beginning `&resolver {`, and its id was lost | `_after_node_properties` skips leading `&anchor`/`!tag` at both flow-entry sites | | Low (2/8) | **`_outside_quotes` rescanned from column 0 per match** | O(matches x line length) on one long flow line | one `_quote_mask` pass per line, shared by all three readers — 3200 candidates: **1009 ms → 1.7 ms**, same ids out | **A regression the fix itself introduced, caught before it shipped.** Applying the scalar-start rule to `_strip_comment` too — which shares the scan — changed **11 of the tree's 16,359 physical lines**, all `run: |` script text like `echo "Updated PR #${existing}"`: strictly, that `"` follows the plain word `echo` and opens nothing, so the ` #` reads as a comment opener and truncates a line whose tail may carry the very `ref:` this lint looks for. `_strip_comment` is asked about every physical line and cannot tell YAML from shell, so it deliberately keeps the weaker reading and is **pinned to it**; only the structural readers, never asked about script text, take the strict rule. `_strip_comment` is now byte-identical to before across the whole tree. **One finding deferred, with a ticket rather than a reply.** `_in_steps_sequence` was never taught the indentless spelling the pre-scan learned in round one, so in such a job every step has no bounds and a step consuming its OWN output is silently dropped — reproduced: the identical job reports `[(False, False, 'dangling')]` indented and `[]` indentless. Not fixed here because `_in_steps_sequence` is the step-BOUNDS subsystem gating resolver registration and `is_guard_step`, and this PR's stated invariant is that resolver and guard verdicts are untouched in every job. The obvious fix also opens a fail-open hole (a `run: |` heredoc emitting `steps:` / `- id: x` at one column would be credited as a real sequence); the follow-up carries the reproduction, that trap, the alternative of keying the boundary off the pre-scan's own item boundaries, and acceptance criteria. **Residue that survives, pinned not claimed closed:** a flow continuation line whose id value carries no quote at all (`id: phantom, more":`) still over-collects. Closing it needs real cross-line quote state, which is a module-wide contract — every reader here is per-line, `_strip_comment` included, and it runs *first* — so it stays out of scope rather than being half-done in one walk. 0 occurrences in the tree. **Re-verified after round two:** 255 tests pass (239 before; 16 new, of which 9 fail against the pre-round-two implementation and the rest pin preserved behaviour). Seven-way mutation battery over the new branches — the scalar-start rule, its list-marker case, the node-property skip at each of its two sites, the step-id quote refusal, its matched-quote unwrap, and `_strip_comment`'s deliberate weaker reading — each mutation failing exactly the test(s) naming it, **no survivors** (one, the matched-quote unwrap, survived the first pass, which is how the test covering it was found missing and added). Full-pipeline differential across all 50 workflow-shaped files / 69 jobs, run three ways — `origin/main`, this PR's previous tip, and this commit all produce identical `ref_checkouts` and `find_unguarded_ref_checkouts` output, 0 diffs, 0 escapes — so this is still a **verified no-op on today's tree**. `check_workflow_pins.py` exit 0, 11 workflows checked. Every other suite green (agents-md-integrity 46, cursor-review 194, groom 365, public-repo-hygiene 165, refresh-reviewers 48). ## Judgment calls and residuals - **`marker column + 2` was specified; I used the measured marker width instead.** The repo's own `WIDE_MARKER_RESOLVER` fixture pins `- id: resolve_ref` (dash + 3), which a hardcoded `+ 2` reads one column shallow — the exact hole `_marker_width` was extracted to close. Using `_marker_width` keeps this reader agreeing with `_binding_step_id` and `_step_bounds`; a new test pins both separation widths. - **The flow pattern is applied more narrowly than specified** — only to marker lines whose value *begins* `{`, not to every marker line. Applied to every marker line it also matches `- with: {id: x}`, which is the flow spelling of silencing shape 1; the narrowing is pinned by a test covering both the member and marker spellings. - **Residual over-collection inside a flow mapping.** `- {with: {id: x}, uses: y}` still registers `x`, because `_STEP_ID_FLOW_RE` is a flat pattern that cannot see nesting. This is the tolerated direction by design (over-collection reproduces the old fail-open drop for one site; under-collection manufactures a false CI failure) and it is the contract the rewrite was specified to keep — but it *is* a residual on `_job_step_ids` / `_STEP_ID_FLOW_RE`, so this PR does not claim the pre-scan is exhaustively scoped, and the tracking ticket stays open rather than auto-closing. No occurrence exists in the tree today (0 of 69 jobs). - **The explicit block-scalar skip is redundant on every shape I could construct**, and I say so rather than implying it is load-bearing: deleting it alone keeps all 228 tests green, because YAML already forces a scalar's body deeper than its key, where the key-column gate drops it. It earns its place only as the second of two independent defenses — deleting the *column gate* alone leaves the heredoc tests passing on the strength of this skip. I kept it deliberately, per the specified design. - ~~**Brace counting for the multi-line flow form is not quote-aware.**~~ **Fixed in review round one** — and the claim that bounded it was wrong: while flow mode is stuck open a real block item `- id: resolve_ref` matches neither pattern applied there, so it *can* lose a real id and manufacture a failure. `_flow_brace_delta` now counts outside quoted scalars. ~~A quote spanning physical lines is still out of scope~~ — **also closed in round two**, from the value side: `_step_id_value` refuses an id still carrying a quote after its closing punctuation and one matched wrapper are stripped, and the mirror direction falls out of the scalar-start rule. The walk's job bound on a stuck `flow_depth` remains explicit and test-pinned. One narrower residue survives and is pinned (see round two above). - **Capability-denial check.** The escape path's user-visible outcome is "no dangling verdict for this job", so I falsified it rather than asserting it: it is a *fail-open drop* restoring this lint's pre-existing coverage, not a new refusal, and I confirmed empirically via the product's own entry point (`check_workflow_pins.py` over the real tree, exit 0) and the 69-job sweep that **0 jobs reach it** — no workflow loses a verdict it has today. - **Pre-existing, untouched, not caused by this diff:** `check_agents_md.py --root .` fails on `AGENTS.md` being 305 lines (ceiling 200) — identical on `origin/main`, and this diff does not touch that file. `.github/coderabbit-config/tests` errors locally on a missing `jsonschema` dependency — also identical on `origin/main`, an environment gap, and outside this diff's path filter. ## Unexercised artifacts The source ticket's Provenance block links a Linear issue and a run dashboard/transcript on an internal host; this worker holds no Linear access and those are unreachable from it, so neither was read. The ticket's description carried the full mechanism and the recommended implementation, and every shape it names was reproduced as a failing test before it was fixed, so nothing in those links was needed to establish acceptance. The linked GitHub review-thread comment was likewise not fetched; its content is restated in the ticket body I worked from. Refs BE-8254 ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'`: 255 passed, 0 failed (239 before review round two; 16 new); `python3 .github/workflow-pins/check_workflow_pins.py`: exit 0, 11 workflows checked, 0 exempt; all five round-two findings reproduced as failures BEFORE being fixed (the plain-scalar quote collapsing a job's id set to `{}`; both cross-line-quote directions; the `&anchor` item losing its id; the indentless job dropping a self-consuming step's `dangling` verdict); 9 of the 16 new tests confirmed failing against the pre-round-two implementation, the other 7 pinning preserved behaviour; seven-way mutation battery over the round-two branches, each mutation failing exactly the test(s) naming it and none surviving — one survived the first pass, which is how the missing matched-quote-unwrap test was found and added; full-pipeline differential across all 50 workflow-shaped files / 69 jobs run three ways (`origin/main`, the PR's previous tip, this commit) — identical `ref_checkouts` and `find_unguarded_ref_checkouts` output, 0 diffs, 0 escapes, still a verified no-op on today's tree; `_strip_comment` differential over all 16,359 physical lines in the tree — 0 differ, which is how the 11-line `run: |` script-text regression an over-broad first attempt introduced was caught and reverted; quote-scan hoist measured at 200/800/3200 flow `id:` candidates (3.4/59.8/1009 ms → 0.09/0.42/1.7 ms) with identical ids out; every other suite in the repo re-run green (agents-md-integrity 46, cursor-review 194, groom 365, public-repo-hygiene 165, refresh-reviewers 48). Note the earlier review commit message says "ten-mutation battery", which miscounts the eight of that round. - **Deviations:** four, all narrowing and all test-pinned. Three from the first rounds: measured marker width instead of the specified `marker column + 2` (the repo's own dash+3 fixture proves `+2` wrong); the flow pattern applied only where a flow mapping actually opens rather than to all marker lines (the wider form re-admits the `- with: {id: x}` silencing shape); and one finding answered in documentation rather than code (the escape costs the `non-leading` verdict too — judging `leading` ahead of it would trade a dropped site for a false failure). New in round two: `_strip_comment` deliberately does NOT take the scalar-start rule the other readers take, because it is asked about `run: |` script bodies where the strict reading truncates real content — the split is pinned by its own test and by a mutation. Two items are deferred rather than done: making the fail-open observable (needs a non-fatal `notices` channel `check_dir` does not have), and teaching `_in_steps_sequence` the indentless spelling (it is the step-bounds subsystem this PR pledges to leave untouched, and the naive fix opens a heredoc fail-open) — both carry a written plan. No acceptance criterion was left unmet. --- <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