Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[GH-ISSUE #172] [groom] Six actions/* refs float on major tags in files where every sibling is SHA-pinned #3

New issue

Closed

opened 2026-10-26 22:34:21 +00:00 by bsakaguchi · 3 comments

bsakaguchi commented

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

Owner

Originally created by @cloud-code-bot[bot] on GitHub (Aug 16, 2026).
Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/172

Groom auto-builder — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, or build/test config that executes in pre-review CI) — a human must author these changes), so it is filed for a human. · run

Verified exactly as claimed. cursor-review.yml has 21 uses: actions/* lines; 17 are 40-hex SHA pins carrying a version comment and 4 float: actions/checkout@v6 (:393), actions/upload-artifact@v7 (:458), actions/download-artifact@v8 (:1259 and :1472). The sharpest instance is real: :1461 pins actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c and :1472, eleven lines later in the SAME consolidate job, uses @v8. test-refresh-reviewers.yml:29 and :34 (actions/checkout@v6, actions/setup-python@v6) are the only unpinned refs across the nine test-* workflows.

This matters most in cursor-review.yml specifically: it is a reusable workflow that other repos SHA-pin precisely to obtain an immutable execution surface, which a floating tag would defeat for those steps, and the jobs involved include ones that mint a bot App token.

Fix (mechanical):

cursor-review.yml: reuse the SHAs its own siblings already carry, i.e. checkout 3d3c42e5aac5ba805825da76410c181273ba90b1 (v7.0.1), upload-artifact 043fb46d1a93c77aae656e7c1c64a875d1fc6a0a (v7.0.1), download-artifact 3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c (v8). While there, normalise the two spellings of that download-artifact version comment (v8 at :1461 and :1478 versus v8.0.1 at :1252).
test-refresh-reviewers.yml: pin checkout and setup-python to the SHAs its sibling test workflows already use.
Optional, and the step that makes it stick: .github/workflow-pins/check_workflow_pins.py already text-parses every file in .github/workflows; add an action-SHA assertion with a self-draining exemption list in the style of its own KNOWN_EXEMPT (:48-56, which fails on a STALE entry).

Steelman, and why it still lands: actions/* is first-party GitHub, so the marginal risk from four floating tags is small, and .github/dependabot.yml already records the state honestly (a handful still sit on floating major tags, and those files get the least benefit from that config until they are pinned by hand). That argues against urgency, not against the fix: it is six lines, Dependabot keeps SHA pins fresh once they exist, and the inconsistency is already visible inside a single job.

Not flagged security-adjacent, deliberately: these are first-party actions, no auth or permission predicate is involved, and the repo already treats this as pinning hygiene with a documented backlog, so it is better implemented mechanically than filed as an investigation. The supply-chain dimension is real but second-order here.

Originally created by @cloud-code-bot[bot] on GitHub (Aug 16, 2026). Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/172 **Groom auto-builder** — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, or build/test config that executes in pre-review CI) — a human must author these changes), so it is filed for a human. · [run](https://github.com/Comfy-Org/github-workflows/actions/runs/31939149292) **Verified exactly as claimed.** cursor-review.yml has 21 `uses: actions/*` lines; 17 are 40-hex SHA pins carrying a version comment and 4 float: `actions/checkout@v6` (:393), `actions/upload-artifact@v7` (:458), `actions/download-artifact@v8` (:1259 and :1472). The sharpest instance is real: :1461 pins `actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c` and :1472, eleven lines later in the SAME consolidate job, uses `@v8`. test-refresh-reviewers.yml:29 and :34 (`actions/checkout@v6`, `actions/setup-python@v6`) are the only unpinned refs across the nine test-* workflows. This matters most in cursor-review.yml specifically: it is a reusable workflow that other repos SHA-pin precisely to obtain an immutable execution surface, which a floating tag would defeat for those steps, and the jobs involved include ones that mint a bot App token. **Fix (mechanical):** 1. cursor-review.yml: reuse the SHAs its own siblings already carry, i.e. checkout `3d3c42e5aac5ba805825da76410c181273ba90b1` (v7.0.1), upload-artifact `043fb46d1a93c77aae656e7c1c64a875d1fc6a0a` (v7.0.1), download-artifact `3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c` (v8). While there, normalise the two spellings of that download-artifact version comment (v8 at :1461 and :1478 versus v8.0.1 at :1252). 2. test-refresh-reviewers.yml: pin checkout and setup-python to the SHAs its sibling test workflows already use. 3. Optional, and the step that makes it stick: `.github/workflow-pins/check_workflow_pins.py` already text-parses every file in `.github/workflows`; add an action-SHA assertion with a self-draining exemption list in the style of its own KNOWN_EXEMPT (:48-56, which fails on a STALE entry). **Steelman, and why it still lands:** actions/* is first-party GitHub, so the marginal risk from four floating tags is small, and `.github/dependabot.yml` already records the state honestly (a handful still sit on floating major tags, and those files get the least benefit from that config until they are pinned by hand). That argues against urgency, not against the fix: it is six lines, Dependabot keeps SHA pins fresh once they exist, and the inconsistency is already visible inside a single job. **Not flagged security-adjacent, deliberately:** these are first-party actions, no auth or permission predicate is involved, and the repo already treats this as pinning hygiene with a documented backlog, so it is better implemented mechanically than filed as an investigation. The supply-chain dimension is real but second-order here.

bsakaguchi

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

closed this issue
added the
groom
label

bsakaguchi commented

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

Author

Owner

@coderabbitai[bot] commented on GitHub (Aug 16, 2026):

🔗 Related PRs

Comfy-Org/github-workflows#103 - fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) [merged]
Comfy-Org/github-workflows#107 - fix(groom): pin ci-groom.yml by SHA and stop the bumper reporting unpinnable callers as converged (BE-6015) [merged]
Comfy-Org/github-workflows#139 - feat(pr-risk): require workflows_ref to be an ancestor of upstream main (BE-6492) [merged]
Comfy-Org/github-workflows#142 - feat(pr-size): opt-in exclude_tests so the cap measures production code (BE-6791) [merged]
Comfy-Org/github-workflows#169 - feat(cursor-review): announce the over-cap skip instead of a silent green run (BE-7646) [merged]

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

@coderabbitai[bot] commented on GitHub (Aug 16, 2026):  <details> <summary>🔗 Related PRs</summary> Comfy-Org/github-workflows#103 - fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) [merged] Comfy-Org/github-workflows#107 - fix(groom): pin ci-groom.yml by SHA and stop the bumper reporting unpinnable callers as converged (BE-6015) [merged] Comfy-Org/github-workflows#139 - feat(pr-risk): require workflows_ref to be an ancestor of upstream main (BE-6492) [merged] Comfy-Org/github-workflows#142 - feat(pr-size): opt-in exclude_tests so the cap measures production code (BE-6791) [merged] Comfy-Org/github-workflows#169 - feat(cursor-review): announce the over-cap skip instead of a silent green run (BE-7646) [merged] </details> --- <details> <summary> 🧪 Issue enrichment is currently in open beta.</summary> You can configure auto-planning by selecting labels in the issue_enrichment configuration. To disable automatic issue enrichment, add the following to your `.coderabbit.yaml`: ```yaml issue_enrichment: auto_enrich: enabled: false ``` </details> 💬 Have feedback or questions? Drop into our [discord](https://discord.gg/coderabbit)!

bsakaguchi commented

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

Author

Owner

@christian-byrne commented on GitHub (Aug 21, 2026):

Re-verified against origin/main @ cf308dc on 2026-08-21. 4 of the 6 refs are fixed; 2 remain, and they have moved since filing. Recommend narrowing this issue rather than closing it.

Fixed — the four `cursor-review.yml` refs

Commit f8eec04e7923f4719d5b7dbc342e5c15580e22ba (PR #193, "fix(workflows): resolve asset checkouts from job.workflow_sha…", 2026-08-20) pinned exactly the four this issue named, as a drive-by:

-        uses: actions/checkout@v7          +  @3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
-        uses: actions/upload-artifact@v7   +  @043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7
-        uses: actions/download-artifact@v8 +  @3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8
-        uses: actions/download-artifact@v8 +  @3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8

cursor-review.yml on main now has 21 uses: actions/* lines and 0 floating. The sharpest instance this issue called out — a SHA pin and a @v8 eleven lines apart in the same consolidate job — is gone.

Still open — the two in `test-refresh-reviewers.yml`

.github/workflows/test-refresh-reviewers.yml:29 and :34. Note they are no longer @v6 as the issue body says:

29:        uses: actions/checkout@v7
34:        uses: actions/setup-python@v7

git blame attributes both to 0071828 (dependabot, PR #182, 2026-08-19). They drifted a major version in the five days this issue has been open, which is the argument for the fix rather than against it.

Repo-wide count today: 136 of 138 uses: refs are 40-hex SHA pins. These two are the entire remainder — there are no floating third-party refs at all.

PR #205 is live proof of the steelman's caveat

The issue body's steelman leans on "Dependabot keeps SHA pins fresh once they exist". Open PR #205 shows what "once they exist" costs — one Dependabot run, one action, two different outcomes:

linear-ticket.yml: setup-python@ece7cb06… # v6 → @5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 — SHA in, SHA out.
test-refresh-reviewers.yml: setup-python@v7 → @v7.0.0 — tag in, tag out.

Dependabot narrowed the tag and left it mutable, exactly as .github/dependabot.yml predicts in its own header ("Dependabot will NOT convert a tag ref to a SHA, so those files get the least benefit from this config until they are pinned by hand"). These two lines will keep getting churned and will never converge on their own.

Item 3 of the proposed fix is still unimplemented

.github/workflow-pins/check_workflow_pins.py on main asserts only the workflows_ref input contract — ref:/env: shapes and the || job.workflow_sha fallback. It carries no actions/* SHA assertion, so nothing in CI fails on these two lines even though test-workflow-pins.yml already triggers on .github/workflows/**. That is why the four in cursor-review.yml needed an unrelated PR to notice them.

Recommendation

Keep open, scope narrowed to test-refresh-reviewers.yml:29,34 plus the guard. Two notes for whoever picks it up:

Coordinate with PR #205 — it touches line 34, so pinning by hand first will conflict.
The version-comment normalisation in item 1 is still outstanding and now applies to upload-artifact too: # v7 at :512/:1489 vs # v7.0.1 at :772, and # v8 at :1316/:1551/:1562/:1568 vs # v8.0.1 at :1309.

Triage sweep; no code changed. Not closing — recommending only.

@christian-byrne commented on GitHub (Aug 21, 2026): **Re-verified against `origin/main` @ `cf308dc` on 2026-08-21. 4 of the 6 refs are fixed; 2 remain, and they have moved since filing. Recommend narrowing this issue rather than closing it.** ### Fixed — the four `cursor-review.yml` refs Commit `f8eec04e7923f4719d5b7dbc342e5c15580e22ba` (PR #193, "fix(workflows): resolve asset checkouts from job.workflow_sha…", 2026-08-20) pinned exactly the four this issue named, as a drive-by: ``` - uses: actions/checkout@v7 + @3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - uses: actions/upload-artifact@v7 + @043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 - uses: actions/download-artifact@v8 + @3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 - uses: actions/download-artifact@v8 + @3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 ``` `cursor-review.yml` on main now has 21 `uses: actions/*` lines and **0** floating. The sharpest instance this issue called out — a SHA pin and a `@v8` eleven lines apart in the same consolidate job — is gone. ### Still open — the two in `test-refresh-reviewers.yml` `.github/workflows/test-refresh-reviewers.yml:29` and `:34`. Note they are **no longer `@v6`** as the issue body says: ``` 29: uses: actions/checkout@v7 34: uses: actions/setup-python@v7 ``` `git blame` attributes both to `0071828` (dependabot, PR #182, 2026-08-19). They drifted a major version in the five days this issue has been open, which is the argument for the fix rather than against it. Repo-wide count today: **136 of 138** `uses:` refs are 40-hex SHA pins. These two are the entire remainder — there are no floating third-party refs at all. ### PR #205 is live proof of the steelman's caveat The issue body's steelman leans on "Dependabot keeps SHA pins fresh once they exist". Open PR #205 shows what "once they exist" costs — one Dependabot run, one action, two different outcomes: - `linear-ticket.yml`: `setup-python@ece7cb06… # v6` → `@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0` — SHA in, SHA out. - `test-refresh-reviewers.yml`: `setup-python@v7` → `@v7.0.0` — **tag in, tag out.** Dependabot narrowed the tag and left it mutable, exactly as `.github/dependabot.yml` predicts in its own header ("Dependabot will NOT convert a tag ref to a SHA, so those files get the least benefit from this config until they are pinned by hand"). These two lines will keep getting churned and will never converge on their own. ### Item 3 of the proposed fix is still unimplemented `.github/workflow-pins/check_workflow_pins.py` on main asserts only the `workflows_ref` input contract — `ref:`/`env:` shapes and the `|| job.workflow_sha` fallback. It carries **no** `actions/*` SHA assertion, so nothing in CI fails on these two lines even though `test-workflow-pins.yml` already triggers on `.github/workflows/**`. That is why the four in `cursor-review.yml` needed an unrelated PR to notice them. ### Recommendation Keep open, scope narrowed to `test-refresh-reviewers.yml:29,34` plus the guard. Two notes for whoever picks it up: 1. Coordinate with PR #205 — it touches line 34, so pinning by hand first will conflict. 2. The version-comment normalisation in item 1 is still outstanding and now applies to `upload-artifact` too: `# v7` at `:512`/`:1489` vs `# v7.0.1` at `:772`, and `# v8` at `:1316`/`:1551`/`:1562`/`:1568` vs `# v8.0.1` at `:1309`. *Triage sweep; no code changed. Not closing — recommending only.*

bsakaguchi commented

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

Author

Owner

@mattmillerai commented on GitHub (Sep 18, 2026):

Triage: @christian-byrne's 2026-08-21 recommendation re-verified and adopted — narrowed, not closed. Leaving this open, tracked internally.

Re-checked every item against origin/main @ cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b.

Done — the four cursor-review.yml refs. Confirmed fixed by f8eec04e7923f4719d5b7dbc342e5c15580e22ba (#193), which is a genuine ancestor of main. cursor-review.yml now carries 27 uses: actions/* lines and 0 floating — it has grown since that comment counted 21, and the newer refs arrived pinned, so the drive-by fix stuck.

Still open — the two in test-refresh-reviewers.yml. They are now at :33 and :38 (the body says 29/34), and they have drifted again since the last triage:

33:        uses: actions/checkout@v7
38:        uses: actions/setup-python@v7.0.0

git blame puts :33 on 0071828 (#182, @v6→@v7) and :38 on c9a2548 — which is #205, now merged. So that PR's prediction is no longer a forecast but a demonstration on main: Dependabot narrowed the tag to a narrower tag (@v7→@v7.0.0) and did not convert it to a SHA, exactly as .github/dependabot.yml:15-18 says it will not. Two drifts in five weeks, converging on nothing.

The conflict hazard that comment flagged is gone — #205 merged, and no open PR touches this file today, so a hand-pin is now clean. The canonical SHAs are already in-repo and need no lookup: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 (73 sites) and actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 (13 sites).

Repo-wide today: 156 uses: directives, 153 SHA-pinned. Of the three that are not, one is a deliberate test fixture (test-workflow-pins.yml:105, actions/checkout@abc, inside a heredoc that writes a synthetic workflow to prove the pin checker rejects it). So these two lines are the entire remaining real surface.

Still open — item 3, and it is the part that matters. check_workflow_pins.py (3735 lines) still asserts only the workflows_ref input contract; its docstring at :17-23 enumerates exactly two checks and there is no 40-hex regex anywhere in the file. KNOWN_EXEMPT at :63 is frozenset() but is keyed on workflow filenames that still default workflows_ref — it is the style model for a self-draining list, not an action-pin exemption list. No other lint in the repo enforces action SHA pinning. That absence is why the four sibling refs needed an unrelated PR to notice them, and why these two will otherwise be re-found by the next sweep and drift a third time.

Still open — version-comment normalisation, now applying to both artifact actions as that comment predicted: upload-artifact@043fb46d… is # v7 ×12 vs # v7.0.1 ×2 (cursor-review.yml:900, pr-size.yml:618); download-artifact@3e5f45b2… is # v8 ×14 vs # v8.0.1 ×2 (cursor-review.yml:1461, pr-size.yml:662). Cosmetic. create-github-app-token's two spellings are deliberate (two different SHAs, with a dependabot.yml ignore behind them) and are not a normalisation target.

One note for whoever picks this up: any actions/* SHA assertion must scan real workflow files only and skip heredoc and fixture bodies, or it will flag test-workflow-pins.yml:105 and the ~20 @abc literals in the pins test file — and fail its own CI.

Reopen or comment if anything here reads wrong on your side.

Created by Claude Code

@mattmillerai commented on GitHub (Sep 18, 2026): **Triage: @christian-byrne's 2026-08-21 recommendation re-verified and adopted — narrowed, not closed. Leaving this open, tracked internally.** Re-checked every item against `origin/main` @ `cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b`. **Done — the four `cursor-review.yml` refs.** Confirmed fixed by `f8eec04e7923f4719d5b7dbc342e5c15580e22ba` (#193), which is a genuine ancestor of `main`. `cursor-review.yml` now carries **27** `uses: actions/*` lines and **0** floating — it has grown since that comment counted 21, and the newer refs arrived pinned, so the drive-by fix stuck. **Still open — the two in `test-refresh-reviewers.yml`.** They are now at **`:33` and `:38`** (the body says 29/34), and they have drifted *again* since the last triage: ``` 33: uses: actions/checkout@v7 38: uses: actions/setup-python@v7.0.0 ``` `git blame` puts `:33` on `0071828` (#182, `@v6`→`@v7`) and `:38` on `c9a2548` — **which is #205, now merged.** So that PR's prediction is no longer a forecast but a demonstration on `main`: Dependabot narrowed the tag to a narrower tag (`@v7`→`@v7.0.0`) and did not convert it to a SHA, exactly as `.github/dependabot.yml:15-18` says it will not. Two drifts in five weeks, converging on nothing. The conflict hazard that comment flagged is **gone** — #205 merged, and no open PR touches this file today, so a hand-pin is now clean. The canonical SHAs are already in-repo and need no lookup: `actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1` (73 sites) and `actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0` (13 sites). Repo-wide today: **156 `uses:` directives, 153 SHA-pinned.** Of the three that are not, one is a deliberate test fixture (`test-workflow-pins.yml:105`, `actions/checkout@abc`, inside a heredoc that writes a synthetic workflow to prove the pin checker rejects it). So these two lines are the entire remaining real surface. **Still open — item 3, and it is the part that matters.** `check_workflow_pins.py` (3735 lines) still asserts only the `workflows_ref` input contract; its docstring at `:17-23` enumerates exactly two checks and there is no 40-hex regex anywhere in the file. `KNOWN_EXEMPT` at `:63` is `frozenset()` but is keyed on *workflow filenames that still default `workflows_ref`* — it is the style model for a self-draining list, not an action-pin exemption list. No other lint in the repo enforces action SHA pinning. That absence is why the four sibling refs needed an unrelated PR to notice them, and why these two will otherwise be re-found by the next sweep and drift a third time. **Still open — version-comment normalisation**, now applying to both artifact actions as that comment predicted: `upload-artifact@043fb46d…` is `# v7` ×12 vs `# v7.0.1` ×2 (`cursor-review.yml:900`, `pr-size.yml:618`); `download-artifact@3e5f45b2…` is `# v8` ×14 vs `# v8.0.1` ×2 (`cursor-review.yml:1461`, `pr-size.yml:662`). Cosmetic. `create-github-app-token`'s two spellings are **deliberate** (two different SHAs, with a `dependabot.yml` ignore behind them) and are not a normalisation target. One note for whoever picks this up: any `actions/*` SHA assertion must scan real workflow files only and skip heredoc and fixture bodies, or it will flag `test-workflow-pins.yml:105` and the ~20 `@abc` literals in the pins test file — and fail its own CI. Reopen or comment if anything here reads wrong on your side. *Created by Claude Code*

bsakaguchi referenced this issue

2026-10-26 22:35:24 +00:00

[PR #3] [MERGED] chore(cursor-review): move DM email-map roster out of the public repo #11