[PR #101] [MERGED] feat(groom): track the pinned Claude Code CLI version in a Dependabot-visible manifest (BE-5373) #111

New issue

Closed

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

bsakaguchi commented

2026-10-26 22:36:22 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/101
Author: @mattmillerai
Created: 7/31/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-5373-claude-code-version-pin

📝 Commits (6)

8550dcc feat(groom): track the pinned Claude Code CLI version in a Dependabot-visible manifest (BE-5373)
c714a84 fix(groom): harden the CLI-pin gate and its guards (BE-5373)
36289df fix(groom): read the CLI pin from the workflow's own commit, tighten the guards (BE-5373)
78331ba docs(groom): scope the install-step pin comment to the top-level version (BE-5373)
b50da95 docs(groom): document the CLI pin's accepted boundary and guard its dependency shape (BE-5582) (#104)
efe5212 Merge remote-tracking branch 'origin/main' into matt/be-5373-claude-code-version-pin

📊 Changes

7 files changed (+1281 additions, -16 deletions)

View changed files

📝 .github/dependabot.yml (+35 -0)
📝 .github/groom/README.md (+180 -0)
➕ .github/groom/package.json (+8 -0)
➕ .github/groom/tests/test_claude_code_pin.py (+835 -0)
📝 .github/workflows/groom.yml (+177 -5)
📝 .github/workflows/test-groom-scripts.yml (+41 -10)
📝 AGENTS.md (+5 -1)

📄 Description

ELI-5

The groom workflow installs the Claude Code CLI at a specific version, and that version number was typed into the workflow file three separate times. Nothing anywhere checked whether it was out of date, so it just quietly aged — and if someone bumped it by hand they could easily update two of the three and leave the workflow in a split state. This moves the version into a tiny package.json, which is a file format Dependabot actually knows how to read, so it will now open a PR when a newer CLI ships. The workflow reads the version out of that one file at run time and hands it to all three install steps, so there is only one number to change and it can never disagree with itself.

What changed

.github/groom/package.json (new) — the single source of truth for the @anthropic-ai/claude-code pin, currently 2.1.217 (unchanged from what the workflow installed before). It is a pin carrier, not a project: no lockfile, nothing is ever npm installed from that directory, and no CI job runs there. It exists purely so Dependabot's npm ecosystem can see the version.
.github/workflows/groom.yml — the gate job (which already checks out this repo's groom assets) reads the pin once, validates it, and exports it as a claude_code_version job output. All three Install Claude Code steps (audit_find, audit_verify, build) consume that output via needs.gate.outputs — every one of those jobs already needs: gate, so no new job edges. No version literal remains in the workflow.
.github/dependabot.yml — an npm entry for directory: "/.github/groom" matching the existing monthly schedule, labels: ["dependencies"], and cooldown settings, plus an explicit versioning-strategy: "increase".
.github/groom/tests/test_claude_code_pin.py (new) — guards the arrangement: exact pin, no hardcoded version literal anywhere in groom.yml, every install step wired to the gate output, every CLI-installing job needs: gate, and the Dependabot entry still present.
.github/workflows/test-groom-scripts.yml — the path filter now also covers .github/workflows/groom.yml and .github/dependabot.yml. Without this the new guard would never run on the very changes it guards (a re-added literal in groom.yml, or the npm entry being dropped).
AGENTS.md / .github/groom/README.md — document the manifest and the two rules around it.

Why a manifest instead of a workflow `env:` constant

The ticket allowed either. A manifest wins because the env:-constant variant needs a second file for Dependabot to bump plus a CI assertion that the two agree — i.e. it keeps two copies of the version and adds machinery to keep them equal. Reading the manifest at run time was straightforward here because all three install jobs already do an actions/checkout of this repo at inputs.workflows_ref into $GROOM_ASSETS (that is how the briefs and ledger.py are loaded), so the manifest is already on disk before the install step runs. One file, one number.

The exact-pin guard, and why it fails loudly

The gate rejects anything that is not an exact X.Y.Z (optionally with a prerelease/build suffix). The agent CLI is executable supply chain for steps that read untrusted repo content, so a ^/~/wildcard/dist-tag requirement would silently un-pin the thing the pin exists to protect.

This is the riskiest line in the diff, because a false rejection would break groom for every live caller at once. Three things bound that:

versioning-strategy: "increase" is set explicitly, so Dependabot rewrites an exact requirement to a new exact requirement rather than widening it into a range.
The regex accepts every shape npm publishes as an exact version. Not asserted — measured: all 472 published versions of @anthropic-ai/claude-code on the registry match it, zero rejected. Spot cases: 2.1.217, 2.1.218-beta.1, 1.0.0+b5, 1.0.0-rc.1+b5 (accepted); ^2.1.217, ~2.1.0, *, latest, 2.1.x, >=2.0.0, "", 1.2.3-a..b (rejected).
It cannot reach a groom run in the first place: a Dependabot PR editing package.json matches the .github/groom/** path filter, so test_claude_code_pin.py runs on that PR and fails it before merge. I confirmed the guard fails as intended by temporarily setting the pin to ^2.1.217 and re-adding a hardcoded literal to groom.yml — 3 tests failed — then restoring.

The guard is the last step in gate and runs only when should_run == 'true' — i.e. only on ticks that would actually install the CLI. It first ran eagerly, ahead of the interval/volume gates; the review panel pushed back and was right. It is the one fail-closed step in a job whose every other step fails open, and a scheduled caller skips ~6 of 7 daily ticks, so running it eagerly let a broken manifest (say, at a stale workflows_ref) red out ticks that were never going to install anything.

Nothing is lost by deferring it: all six consumer jobs are themselves if: needs.gate.outputs.should_run == 'true', so the empty output on a skipped tick is unobservable, and a bad pin cannot reach main in the first place — test_claude_code_pin.py runs on any PR touching .github/groom/**, which includes package.json itself. This step is the backstop for a stale ref, not the primary guard.

Verification

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' — 178 passed (8 new).
python3 -m unittest discover -s .github/cursor-review/tests … and … agents-md-integrity/tests … — 40 and 18 passed.
python3 .github/agents-md-integrity/check_agents_md.py --root . — passed (1 pre-existing CODEOWNERS warning). AGENTS.md is 146 lines.
actionlint clean on every workflow. This is the substantive check on the part I could not run for real: actionlint statically resolves needs.<job>.outputs.<name> against declared outputs. I proved it is a real check by injecting a typo'd output reference and a bad step id — it flagged both at the exact lines — then restoring.
Simulated the gate step's shell against the real manifest (jq read → validation → $GITHUB_OUTPUT); shellcheck clean; confirmed the ${VAR:?…} guard exits non-zero with a legible message when the variable is unset or empty.
YAML parse of groom.yml, dependabot.yml, test-groom-scripts.yml; JSON parse of package.json.

Unmet acceptance criterion — please read

The ticket's step 4 ("verify a groom run still installs and executes the CLI via workflow_dispatch on ci-groom.yml") is NOT done, and is not currently doable by anyone. ci-groom.yml on main still carries the literal placeholder REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA in both its uses: ref and its workflows_ref: input — the BE-4004 bump-at-merge step was never performed. Every scheduled run has failed since the workflow was added: the oldest run in history (2026-07-21) is a startup_failure and the five since are all failure, including today's. So this repo's own groom caller has never had a green run, before or after this change.

I did not fix that here. Bumping those two refs means choosing a SHA and thereby switching on a live daily agent sweep that files issues on a public repo — a deliberate operator action with real spend, not a side effect of a pin refactor. It is also a bump-at-merge by nature: the correct value is this PR's squash SHA, which does not exist yet.

Consequence for review: the end-to-end "the CLI actually installs at the resolved version" leg is unproven. What is proven is the wiring (actionlint), the resolution logic (simulated against the real manifest), and the guards (unit tests, demonstrated failing on regression). The other four callers on the GROOM_CALLERS roster pick this change up through the normal bump-groom-callers flow after merge, and the first of those runs is the real end-to-end confirmation.

Judgment calls

No package-lock.json. Dependabot's npm ecosystem resolves a manifest-only directory fine, and nothing installs from it, so a lockfile would be a second artifact to keep in sync for no benefit. The ticket allowed one "if the npm ecosystem needs one" — it does not.
Left groom.yml's "Verified empirically against the pinned 2.1.217" comment alone. That is a historical record of which CLI version the agent sandbox rules were validated against (the rule surface is version-specific), not a pin. It is useful to whoever bumps next, so removing it would delete information.
Did not touch the existing 2.1.217 value. This PR changes where the version lives, not what it is.

Review round 1 — resolved (`c714a84`)

All ten panel findings addressed; every thread replied to and resolved. Seven produced code changes, three were answered without one.

Fixed in groom.yml's gate step: the eager-fail ordering above; ? on the jq index so a dependencies that is present but not an object reaches the legible error instead of dying on a raw jq "cannot index" under set -e; newlines stripped from the version before it is echoed into a pre-validation ::error:: (it is unvalidated JSON at that point, and an embedded newline could emit further workflow commands into the log); and the guard tightened to strict SemVer, since the old suffix match accepted 1.2.3-a..b, which npm resolves as a mutable dist-tag.

Fixed in test_claude_code_pin.py: fullmatch instead of assertRegex; job-boundary regex widened to allow - and uppercase (both legal job IDs); needs: parsed rather than substring-matched, so the block-sequence form and needs: [audit_find, gate] stop false-failing; and the Dependabot keys checked against one parsed entry instead of three independent substring matches over the whole file.

Found while verifying that last set, not raised by the panel: test_every_install_job_depends_on_the_gate was passing vacuously on the gate job itself. gate's prose contains both the string npm install -g <pkg>@<ver> (explaining why the pin is not inline) and the words needs: gate (explaining why it resolves there), so the substring scan saw a phantom install step and then "proved" the job depends on itself. Comment lines are now stripped before scanning, and the set of installing jobs is asserted explicitly so a silent drop to zero matches cannot pass.

Answered without a code change: the $GROOM_ASSETS-unbound finding does not reproduce (it is defined at workflow level, groom.yml:371, and points one level deeper than the checkout path:, so the suggested literal would be the wrong path); the "no dependencies key" jq repro does not reproduce either (jq permits string-indexing null) though the adjacent non-object case was real and is fixed; and the workflows_ref split-pin concern is the repo's existing, documented trust model rather than something this PR introduces — config.py, interval.py, ledger.py and the three agent briefs already travel at that same ref, so a version number read from it is strictly less powerful than the prompts and Python already loaded from it. No follow-up tickets filed.

Re-verified after the changes: 178 groom / 40 cursor-review / 18 agents-md tests pass, actionlint clean across every workflow (and confirmed still load-bearing — a typo'd step id in the new if: is flagged at the exact line), AGENTS.md integrity passes, and the gate shell was re-simulated against the real manifest plus the hostile ones from the findings.

Unchanged: the "Unmet acceptance criterion" section above still stands. ci-groom.yml on main still carries the REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA placeholder, so the end-to-end "the CLI actually installs at the resolved version" leg remains unproven by anyone.

_{🔄 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/101 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/31/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-5373-claude-code-version-pin` --- ### 📝 Commits (6) - [`8550dcc`](https://github.com/Comfy-Org/github-workflows/commit/8550dcc40b84e50204b3f980cfab46edc239bd52) feat(groom): track the pinned Claude Code CLI version in a Dependabot-visible manifest (BE-5373) - [`c714a84`](https://github.com/Comfy-Org/github-workflows/commit/c714a846aa3fe1b77355977c4e73957990a55334) fix(groom): harden the CLI-pin gate and its guards (BE-5373) - [`36289df`](https://github.com/Comfy-Org/github-workflows/commit/36289dfe566c45f36be5c1a757fdf51d0cdc9468) fix(groom): read the CLI pin from the workflow's own commit, tighten the guards (BE-5373) - [`78331ba`](https://github.com/Comfy-Org/github-workflows/commit/78331bace2761e3b424c559b6ef0c21c482ab0e8) docs(groom): scope the install-step pin comment to the top-level version (BE-5373) - [`b50da95`](https://github.com/Comfy-Org/github-workflows/commit/b50da95b115dc6353df144f687447cb73cc41cd8) docs(groom): document the CLI pin's accepted boundary and guard its dependency shape (BE-5582) (#104) - [`efe5212`](https://github.com/Comfy-Org/github-workflows/commit/efe5212dc2729fe5993a1cf048c84ef47e915ace) Merge remote-tracking branch 'origin/main' into matt/be-5373-claude-code-version-pin ### 📊 Changes **7 files changed** (+1281 additions, -16 deletions) <details> <summary>View changed files</summary> 📝 `.github/dependabot.yml` (+35 -0) 📝 `.github/groom/README.md` (+180 -0) ➕ `.github/groom/package.json` (+8 -0) ➕ `.github/groom/tests/test_claude_code_pin.py` (+835 -0) 📝 `.github/workflows/groom.yml` (+177 -5) 📝 `.github/workflows/test-groom-scripts.yml` (+41 -10) 📝 `AGENTS.md` (+5 -1) </details> ### 📄 Description ## ELI-5 The groom workflow installs the Claude Code CLI at a specific version, and that version number was typed into the workflow file three separate times. Nothing anywhere checked whether it was out of date, so it just quietly aged — and if someone bumped it by hand they could easily update two of the three and leave the workflow in a split state. This moves the version into a tiny `package.json`, which is a file format Dependabot actually knows how to read, so it will now open a PR when a newer CLI ships. The workflow reads the version out of that one file at run time and hands it to all three install steps, so there is only one number to change and it can never disagree with itself. ## What changed - **`.github/groom/package.json` (new)** — the single source of truth for the `@anthropic-ai/claude-code` pin, currently `2.1.217` (unchanged from what the workflow installed before). It is a *pin carrier, not a project*: no lockfile, nothing is ever `npm install`ed from that directory, and no CI job runs there. It exists purely so Dependabot's npm ecosystem can see the version. - **`.github/workflows/groom.yml`** — the `gate` job (which already checks out this repo's groom assets) reads the pin once, validates it, and exports it as a `claude_code_version` job output. All three `Install Claude Code` steps (`audit_find`, `audit_verify`, `build`) consume that output via `needs.gate.outputs` — every one of those jobs already `needs: gate`, so no new job edges. No version literal remains in the workflow. - **`.github/dependabot.yml`** — an `npm` entry for `directory: "/.github/groom"` matching the existing monthly schedule, `labels: ["dependencies"]`, and cooldown settings, plus an explicit `versioning-strategy: "increase"`. - **`.github/groom/tests/test_claude_code_pin.py` (new)** — guards the arrangement: exact pin, no hardcoded version literal anywhere in `groom.yml`, every install step wired to the gate output, every CLI-installing job `needs: gate`, and the Dependabot entry still present. - **`.github/workflows/test-groom-scripts.yml`** — the path filter now also covers `.github/workflows/groom.yml` and `.github/dependabot.yml`. Without this the new guard would never run on the very changes it guards (a re-added literal in `groom.yml`, or the npm entry being dropped). - **`AGENTS.md` / `.github/groom/README.md`** — document the manifest and the two rules around it. ## Why a manifest instead of a workflow `env:` constant The ticket allowed either. A manifest wins because the `env:`-constant variant needs a *second* file for Dependabot to bump plus a CI assertion that the two agree — i.e. it keeps two copies of the version and adds machinery to keep them equal. Reading the manifest at run time was straightforward here because all three install jobs already do an `actions/checkout` of this repo at `inputs.workflows_ref` into `$GROOM_ASSETS` (that is how the briefs and `ledger.py` are loaded), so the manifest is already on disk before the install step runs. One file, one number. ## The exact-pin guard, and why it fails loudly The gate rejects anything that is not an exact `X.Y.Z` (optionally with a prerelease/build suffix). The agent CLI is executable supply chain for steps that read untrusted repo content, so a `^`/`~`/wildcard/dist-tag requirement would silently un-pin the thing the pin exists to protect. This is the riskiest line in the diff, because a false rejection would break groom for every live caller at once. Three things bound that: 1. `versioning-strategy: "increase"` is set explicitly, so Dependabot rewrites an exact requirement to a new exact requirement rather than widening it into a range. 2. The regex accepts every shape npm publishes as an exact version. Not asserted — measured: all **472** published versions of `@anthropic-ai/claude-code` on the registry match it, zero rejected. Spot cases: `2.1.217`, `2.1.218-beta.1`, `1.0.0+b5`, `1.0.0-rc.1+b5` (accepted); `^2.1.217`, `~2.1.0`, `*`, `latest`, `2.1.x`, `>=2.0.0`, `""`, `1.2.3-a..b` (rejected). 3. It cannot reach a groom run in the first place: a Dependabot PR editing `package.json` matches the `.github/groom/**` path filter, so `test_claude_code_pin.py` runs on that PR and fails it before merge. I confirmed the guard fails as intended by temporarily setting the pin to `^2.1.217` and re-adding a hardcoded literal to `groom.yml` — 3 tests failed — then restoring. The guard is the **last** step in `gate` and runs only when `should_run == 'true'` — i.e. only on ticks that would actually install the CLI. It first ran eagerly, ahead of the interval/volume gates; the review panel pushed back and was right. It is the one fail-*closed* step in a job whose every other step fails open, and a scheduled caller skips ~6 of 7 daily ticks, so running it eagerly let a broken manifest (say, at a stale `workflows_ref`) red out ticks that were never going to install anything. Nothing is lost by deferring it: all six consumer jobs are themselves `if: needs.gate.outputs.should_run == 'true'`, so the empty output on a skipped tick is unobservable, and a bad pin cannot reach `main` in the first place — `test_claude_code_pin.py` runs on any PR touching `.github/groom/**`, which includes `package.json` itself. This step is the backstop for a stale ref, not the primary guard. ## Verification - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'` — 178 passed (8 new). - `python3 -m unittest discover -s .github/cursor-review/tests …` and `… agents-md-integrity/tests …` — 40 and 18 passed. - `python3 .github/agents-md-integrity/check_agents_md.py --root .` — passed (1 pre-existing CODEOWNERS warning). AGENTS.md is 146 lines. - `actionlint` clean on every workflow. This is the substantive check on the part I could not run for real: actionlint statically resolves `needs.<job>.outputs.<name>` against declared outputs. I proved it is a real check by injecting a typo'd output reference and a bad step id — it flagged both at the exact lines — then restoring. - Simulated the gate step's shell against the real manifest (`jq` read → validation → `$GITHUB_OUTPUT`); `shellcheck` clean; confirmed the `${VAR:?…}` guard exits non-zero with a legible message when the variable is unset or empty. - YAML parse of `groom.yml`, `dependabot.yml`, `test-groom-scripts.yml`; JSON parse of `package.json`. ## Unmet acceptance criterion — please read **The ticket's step 4 ("verify a groom run still installs and executes the CLI via `workflow_dispatch` on `ci-groom.yml`") is NOT done, and is not currently doable by anyone.** `ci-groom.yml` on `main` still carries the literal placeholder `REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA` in both its `uses:` ref and its `workflows_ref:` input — the BE-4004 bump-at-merge step was never performed. Every scheduled run has failed since the workflow was added: the oldest run in history (2026-07-21) is a `startup_failure` and the five since are all `failure`, including today's. So this repo's own groom caller has never had a green run, before or after this change. I did not fix that here. Bumping those two refs means choosing a SHA and thereby switching on a live daily agent sweep that files issues on a public repo — a deliberate operator action with real spend, not a side effect of a pin refactor. It is also a bump-at-merge by nature: the correct value is *this* PR's squash SHA, which does not exist yet. Consequence for review: the end-to-end "the CLI actually installs at the resolved version" leg is unproven. What is proven is the wiring (actionlint), the resolution logic (simulated against the real manifest), and the guards (unit tests, demonstrated failing on regression). The other four callers on the `GROOM_CALLERS` roster pick this change up through the normal `bump-groom-callers` flow after merge, and the first of those runs is the real end-to-end confirmation. ## Judgment calls - **No `package-lock.json`.** Dependabot's npm ecosystem resolves a manifest-only directory fine, and nothing installs from it, so a lockfile would be a second artifact to keep in sync for no benefit. The ticket allowed one "if the npm ecosystem needs one" — it does not. - **Left `groom.yml`'s "Verified empirically against the pinned 2.1.217" comment alone.** That is a historical record of which CLI version the agent sandbox rules were validated against (the rule surface is version-specific), not a pin. It is useful to whoever bumps next, so removing it would delete information. - **Did not touch the existing `2.1.217` value.** This PR changes where the version lives, not what it is. --- ## Review round 1 — resolved (c714a84) All ten panel findings addressed; every thread replied to and resolved. Seven produced code changes, three were answered without one. **Fixed in `groom.yml`'s gate step:** the eager-fail ordering above; `?` on the jq index so a `dependencies` that is present but not an object reaches the legible error instead of dying on a raw jq "cannot index" under `set -e`; newlines stripped from the version *before* it is echoed into a pre-validation `::error::` (it is unvalidated JSON at that point, and an embedded newline could emit further workflow commands into the log); and the guard tightened to strict SemVer, since the old suffix match accepted `1.2.3-a..b`, which npm resolves as a mutable dist-tag. **Fixed in `test_claude_code_pin.py`:** `fullmatch` instead of `assertRegex`; job-boundary regex widened to allow `-` and uppercase (both legal job IDs); `needs:` parsed rather than substring-matched, so the block-sequence form and `needs: [audit_find, gate]` stop false-failing; and the Dependabot keys checked against one parsed entry instead of three independent substring matches over the whole file. **Found while verifying that last set, not raised by the panel:** `test_every_install_job_depends_on_the_gate` was passing *vacuously* on the `gate` job itself. `gate`'s prose contains both the string `npm install -g <pkg>@<ver>` (explaining why the pin is not inline) and the words `needs: gate` (explaining why it resolves there), so the substring scan saw a phantom install step and then "proved" the job depends on itself. Comment lines are now stripped before scanning, and the set of installing jobs is asserted explicitly so a silent drop to zero matches cannot pass. **Answered without a code change:** the `$GROOM_ASSETS`-unbound finding does not reproduce (it is defined at workflow level, `groom.yml:371`, and points one level deeper than the checkout `path:`, so the suggested literal would be the wrong path); the "no `dependencies` key" jq repro does not reproduce either (jq permits string-indexing `null`) though the adjacent non-object case was real and is fixed; and the `workflows_ref` split-pin concern is the repo's existing, documented trust model rather than something this PR introduces — `config.py`, `interval.py`, `ledger.py` and the three agent briefs already travel at that same ref, so a version *number* read from it is strictly less powerful than the prompts and Python already loaded from it. No follow-up tickets filed. Re-verified after the changes: 178 groom / 40 cursor-review / 18 agents-md tests pass, `actionlint` clean across every workflow (and confirmed still load-bearing — a typo'd step id in the new `if:` is flagged at the exact line), AGENTS.md integrity passes, and the gate shell was re-simulated against the real manifest plus the hostile ones from the findings. **Unchanged: the "Unmet acceptance criterion" section above still stands.** `ci-groom.yml` on `main` still carries the `REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA` placeholder, so the end-to-end "the CLI actually installs at the resolved version" leg remains unproven by anyone. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:22 +00:00