[PR #242] [MERGED] fix(workflow-pins): judge the folded ref value for the job.workflow_sha fallback #249

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-9648-workflow-pins-folded-fallback

📝 Commits (1)

7b6c92d fix(workflow-pins): judge the FOLDED ref value for the job.workflow_sha fallback

📊 Changes

3 files changed (+257 additions, -2 deletions)

View changed files

📝 .github/workflow-pins/README.md (+11 -2)
📝 .github/workflow-pins/check_workflow_pins.py (+71 -0)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+175 -0)

📄 Description

ELI-5

A workflow can write its checkout ref across several lines. GitHub glues them back into one string before running, but this lint was reading them one line at a time — so ${{ inputs.workflows_ref || on one line and job.workflow_sha }} on the next did not look like the self-pin it actually is. The lint then told a perfectly correct workflow to fix two things that were not broken. Now the lint glues the lines back together first, exactly like GitHub does.

What was wrong

ref_checkouts' multi-line ref: continuation arm judged uses_fallback from ONE physical line (fallback = _pins_to_job_workflow_sha(line)), while the continuation form of _FALLBACK_RES is anchored and needs that text to BE the whole ${{ inputs.workflows_ref || job.workflow_sha }}. So a block scalar folded as ref: >- / ${{ inputs.workflows_ref || / job.workflow_sha }} recorded uses_fallback=False, and two things went wrong at once: the fallback-strength guard in that job no longer covered the site, so it was reported UNGUARDED with the wrong (bare-input) remedy; and where every site in a file folded that way, check_dir also lost the default: '' carve-out and added BE-5546's "delete the default:" on top. A file that self-pins and guards correctly got two contradictory errors.

Fail-closed and latent — no workflow on main folds the fallback today, so this changes no verdict on the real tree (measured below).

The fix, and why not the obvious one

The arm records at the FIRST line that mentions the input. For the trailing split that line is one short of the whole expression, so joining only the lines already seen still answers False — appending-then-judging " ".join(pending_parts) does not fix this spelling. The fold has to read FORWARD. _folded_scalar_from does that: from the recording line it walks down while the interpolation is still open and the scalar has not closed, bounded by the ref: key's own column, skipping blank/comment lines the way the walk that feeds it does.

Three properties keep it safe. The verdict stays at its own site, so there is no deferred flush and the last field of the last step can never be dropped — the hazard the sibling UNPARSED branch guards separately. The single-line answer is tried first and the join runs only when it said False, so the one-line spelling behaves byte for byte as it did and the fold can only ever ADD the fallback, never clear one. And the folded text goes to the SAME anchored matcher, so a third operand or a leading operand is rejected in the folded spelling exactly as it is on one line.

Judgment call worth flagging: the helper takes a plain flag and picks _strip_comment vs. a raw .strip() the same way the caller's own continuation branch does, because a # ends a plain scalar's value but is CONTENT inside a |/> body. It changes nothing for today's single consumer (_pins_to_job_workflow_sha strips its own argument regardless) — it matters only for a caller that reads the fold un-stripped, which is why the helper was extracted rather than inlined.

Verified

Full suite green (316, up from 308). The five new assertions were run against the unmodified parser first and all five fail there, so they are red→green rather than self-confirming. test_the_forward_fold_stops_at_the_end_of_the_scalar and the two operand negative controls pass both ways by design — they are regression guards on the bound and the anchor, not on the fix.

End-to-end on the artifact the lint exists for: a copy of groom.yml with all seven fallback ref: lines rewritten as ref: >- / ${{ inputs.workflows_ref || / job.workflow_sha }} passes --workflows-dir <tmp> cleanly on this branch; on main the same file emits 8 problems — the seven unguarded-checkout errors with the bare-input remedy, plus the line-361 default: error.

Adversarial sweep of spellings the fold must NOT bless, each compared against main and against its own one-line twin: a third operand (|| job.workflow_sha || 'main' }}), a concatenation prefix (refs/heads/${{ … ), a bare input, and || 'main' all stay non-fallback and stay reported. A blank line inside the scalar is the one case the fold newly gets right (main missed it); it is locked in by a test.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 316 passed, 0 failed (308 on the base commit; 5 of the 8 new assertions verified red against the unpatched parser). python3 .github/workflow-pins/check_workflow_pins.py: OK — 11 workflow(s) declare workflows_ref, none with a default, every ref checkout guarded (0 exempt). Pre/post ref_checkouts sweep over all 50 files in .github/workflows/: 0 differing files, groom.yml unchanged at sites 683, 1185, 1660, 2067, 2270, 2514, 3009, all (…, True, True, False). Folded-groom.yml copy via --workflows-dir: exit 0 here, exit 1 with 8 problems on the base commit. python3 .github/agents-md-integrity/check_agents_md.py --root .: passed (2 pre-existing warnings, unrelated to this diff). bash .github/bump-callers/tests/test_paths_contract.sh: 70 passed, 0 failed.
Deviations: none against the plan's acceptance criteria; two mechanical adaptations, both because the file moved since the plan was written. parts = pending_parts + [cont] became parts = list(pending_parts) — the current loop already appends cont before this point, so re-adding it would have duplicated the line in the fold. And the plan's sibling leading-operand work is already on main (the arm reads joined for _leading_operand_reaches_input today), so the helper is extracted for reuse but that call site is left untouched.

Residual

The same defect on a different arm: the PLAIN multi-line spelling is still misjudged. ref: ${{ inputs.workflows_ref || / job.workflow_sha }} — no >- header, just a plain scalar folding onto the line below — is valid YAML that the runtime folds identically, and it still records (…, False, False, False) under a fallback guard. This fix does not reach it and was not meant to: the root cause is different. is_ref_use returns True on that first physical line even though its ${{ is unclosed, so the ONE-LINE elif ref_use: arm fires and the continuation window never opens at all. Fixing it means deferring or forward-folding that arm's verdict too, which also moves its _leading_operand_reaches_input(line, …) read — a wider change than the one scoped here, and one whose blast radius is every non-folded checkout rather than only the folded ones. Repro: put that two-line ref: under the GUARD_WITH_FALLBACK fixture in GuardCoverageTests and compare against the one-line spelling. Same severity as the bug fixed here — latent, fail-closed, no workflow on main writes it.

A folded leading operand can still pass where its one-line spelling is reported. ref: >- / ${{ 'main' || inputs.workflows_ref || / job.workflow_sha }} under a BARE guard yields (…, False, True, False) — guarded — while the byte-identical one-line expression correctly yields (…, False, False, False). Cause: _leading_operand_reaches_input is handed the pre-fold joined, whose ${{ is still unclosed, so _INTERPOLATION_RE finds nothing and the check returns True without narrowing. Verified present on the base commit and UNCHANGED by this diff — it is neither introduced nor fixed here. _folded_scalar_from returns the completed text precisely so this is a one-line follow-up: pass the folded value to that check instead. Left out because it widens an existing guard verdict, which needs its own history read rather than a drive-by.

Unexercised artifacts. The plan this PR implements cites an upstream investigation's findings comment as the evidence it was built on, plus an internal dashboard URL and a worker transcript log on the build host (documented as best-effort and truncated by the next run). None were readable from this session — no tracker access, and the other two are internal-network/local paths — so the reproduction here was rebuilt from first principles against the parser rather than inherited: the failing spellings were derived from _FALLBACK_RES' anchoring and confirmed red against the unpatched code before the fix was written. Two figures the plan quoted have also drifted with the file: it expected a 272-test suite and 48 workflow files; the base commit has 308 tests and 50 files. The site-level expectations it named for groom.yml still hold exactly.

_{🔄 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/242 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/27/2026 **Status:** ✅ Merged **Merged:** 8/28/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9648-workflow-pins-folded-fallback` --- ### 📝 Commits (1) - [`7b6c92d`](https://github.com/Comfy-Org/github-workflows/commit/7b6c92dd3e5fabc532aeceeec1371e326953acdd) fix(workflow-pins): judge the FOLDED ref value for the job.workflow_sha fallback ### 📊 Changes **3 files changed** (+257 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+11 -2) 📝 `.github/workflow-pins/check_workflow_pins.py` (+71 -0) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+175 -0) </details> ### 📄 Description ## ELI-5 A workflow can write its checkout ref across several lines. GitHub glues them back into one string before running, but this lint was reading them one line at a time — so `${{ inputs.workflows_ref ||` on one line and `job.workflow_sha }}` on the next did not look like the self-pin it actually is. The lint then told a perfectly correct workflow to fix two things that were not broken. Now the lint glues the lines back together first, exactly like GitHub does. ## What was wrong `ref_checkouts`' multi-line `ref:` continuation arm judged `uses_fallback` from ONE physical line (`fallback = _pins_to_job_workflow_sha(line)`), while the continuation form of `_FALLBACK_RES` is anchored and needs that text to BE the whole `${{ inputs.workflows_ref || job.workflow_sha }}`. So a block scalar folded as `ref: >-` / `${{ inputs.workflows_ref ||` / `job.workflow_sha }}` recorded `uses_fallback=False`, and two things went wrong at once: the fallback-strength guard in that job no longer covered the site, so it was reported UNGUARDED with the wrong (bare-input) remedy; and where every site in a file folded that way, `check_dir` also lost the `default: ''` carve-out and added BE-5546's "delete the `default:`" on top. A file that self-pins and guards correctly got two contradictory errors. Fail-closed and latent — no workflow on `main` folds the fallback today, so this changes no verdict on the real tree (measured below). ## The fix, and why not the obvious one The arm records at the FIRST line that mentions the input. For the trailing split that line is one short of the whole expression, so joining only the lines already seen still answers `False` — appending-then-judging `" ".join(pending_parts)` does not fix this spelling. The fold has to read FORWARD. `_folded_scalar_from` does that: from the recording line it walks down while the interpolation is still open and the scalar has not closed, bounded by the `ref:` key's own column, skipping blank/comment lines the way the walk that feeds it does. Three properties keep it safe. The verdict stays at its own site, so there is no deferred flush and the last field of the last step can never be dropped — the hazard the sibling UNPARSED branch guards separately. The single-line answer is tried first and the join runs only when it said `False`, so the one-line spelling behaves byte for byte as it did and the fold can only ever ADD the fallback, never clear one. And the folded text goes to the SAME anchored matcher, so a third operand or a leading operand is rejected in the folded spelling exactly as it is on one line. Judgment call worth flagging: the helper takes a `plain` flag and picks `_strip_comment` vs. a raw `.strip()` the same way the caller's own continuation branch does, because a ` #` ends a plain scalar's value but is CONTENT inside a `|`/`>` body. It changes nothing for today's single consumer (`_pins_to_job_workflow_sha` strips its own argument regardless) — it matters only for a caller that reads the fold un-stripped, which is why the helper was extracted rather than inlined. ## Verified Full suite green (316, up from 308). The five new assertions were run against the unmodified parser first and all five fail there, so they are red→green rather than self-confirming. `test_the_forward_fold_stops_at_the_end_of_the_scalar` and the two operand negative controls pass both ways by design — they are regression guards on the bound and the anchor, not on the fix. End-to-end on the artifact the lint exists for: a copy of `groom.yml` with all seven fallback `ref:` lines rewritten as `ref: >-` / `${{ inputs.workflows_ref ||` / `job.workflow_sha }}` passes `--workflows-dir <tmp>` cleanly on this branch; on `main` the same file emits 8 problems — the seven unguarded-checkout errors with the bare-input remedy, plus the line-361 `default:` error. Adversarial sweep of spellings the fold must NOT bless, each compared against `main` and against its own one-line twin: a third operand (`|| job.workflow_sha || 'main' }}`), a concatenation prefix (`refs/heads/${{ … `), a bare input, and `|| 'main'` all stay non-fallback and stay reported. A blank line inside the scalar is the one case the fold newly gets right (`main` missed it); it is locked in by a test. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'`: 316 passed, 0 failed (308 on the base commit; 5 of the 8 new assertions verified red against the unpatched parser). `python3 .github/workflow-pins/check_workflow_pins.py`: `OK — 11 workflow(s) declare workflows_ref, none with a default, every ref checkout guarded (0 exempt)`. Pre/post `ref_checkouts` sweep over all 50 files in `.github/workflows/`: 0 differing files, `groom.yml` unchanged at sites 683, 1185, 1660, 2067, 2270, 2514, 3009, all `(…, True, True, False)`. Folded-`groom.yml` copy via `--workflows-dir`: exit 0 here, exit 1 with 8 problems on the base commit. `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed (2 pre-existing warnings, unrelated to this diff). `bash .github/bump-callers/tests/test_paths_contract.sh`: 70 passed, 0 failed. - **Deviations:** none against the plan's acceptance criteria; two mechanical adaptations, both because the file moved since the plan was written. `parts = pending_parts + [cont]` became `parts = list(pending_parts)` — the current loop already appends `cont` before this point, so re-adding it would have duplicated the line in the fold. And the plan's sibling leading-operand work is already on `main` (the arm reads `joined` for `_leading_operand_reaches_input` today), so the helper is extracted for reuse but that call site is left untouched. ## Residual **The same defect on a different arm: the PLAIN multi-line spelling is still misjudged.** `ref: ${{ inputs.workflows_ref ||` / `job.workflow_sha }}` — no `>-` header, just a plain scalar folding onto the line below — is valid YAML that the runtime folds identically, and it still records `(…, False, False, False)` under a fallback guard. This fix does not reach it and was not meant to: the root cause is different. `is_ref_use` returns True on that first physical line even though its `${{` is unclosed, so the ONE-LINE `elif ref_use:` arm fires and the continuation window never opens at all. Fixing it means deferring or forward-folding that arm's verdict too, which also moves its `_leading_operand_reaches_input(line, …)` read — a wider change than the one scoped here, and one whose blast radius is every non-folded checkout rather than only the folded ones. Repro: put that two-line `ref:` under the `GUARD_WITH_FALLBACK` fixture in `GuardCoverageTests` and compare against the one-line spelling. Same severity as the bug fixed here — latent, fail-closed, no workflow on `main` writes it. **A folded leading operand can still pass where its one-line spelling is reported.** `ref: >-` / `${{ 'main' || inputs.workflows_ref ||` / `job.workflow_sha }}` under a BARE guard yields `(…, False, True, False)` — guarded — while the byte-identical one-line expression correctly yields `(…, False, False, False)`. Cause: `_leading_operand_reaches_input` is handed the pre-fold `joined`, whose `${{` is still unclosed, so `_INTERPOLATION_RE` finds nothing and the check returns True without narrowing. Verified present on the base commit and UNCHANGED by this diff — it is neither introduced nor fixed here. `_folded_scalar_from` returns the completed text precisely so this is a one-line follow-up: pass the folded value to that check instead. Left out because it widens an existing guard verdict, which needs its own history read rather than a drive-by. **Unexercised artifacts.** The plan this PR implements cites an upstream investigation's findings comment as the evidence it was built on, plus an internal dashboard URL and a worker transcript log on the build host (documented as best-effort and truncated by the next run). None were readable from this session — no tracker access, and the other two are internal-network/local paths — so the reproduction here was rebuilt from first principles against the parser rather than inherited: the failing spellings were derived from `_FALLBACK_RES`' anchoring and confirmed red against the unpatched code before the fix was written. Two figures the plan quoted have also drifted with the file: it expected a 272-test suite and 48 workflow files; the base commit has 308 tests and 50 files. The site-level expectations it named for `groom.yml` still hold exactly. --- <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:45 +00:00