feat(groom): track the pinned Claude Code CLI version in a Dependabot-visible manifest (BE-5373) - #101
Open
mattmillerai wants to merge 4 commits into
Open
feat(groom): track the pinned Claude Code CLI version in a Dependabot-visible manifest (BE-5373)#101mattmillerai wants to merge 4 commits into
mattmillerai wants to merge 4 commits into
Conversation
…-visible manifest (BE-5373) The @anthropic-ai/claude-code version groom.yml installs was hardcoded at three separate `run:` call sites with nothing watching it. Dependabot's github-actions ecosystem only parses `uses:` refs, and an inline `npm install -g <pkg>@<ver>` is invisible to every ecosystem absent an npm manifest, so the pin silently rotted — the failure mode .github/dependabot.yml was added to fix — while being duplicated three ways, so a hand bump could update two and leave a split state. It matters more than a normal dev-tool pin because those steps run inside jobs holding a bot App token with contents / pull-requests / issues write. Move the version into .github/groom/package.json (a pin carrier, not a project — no lockfile, nothing installed from it), have groom.yml's `gate` job read and validate it once, and feed all three install steps from that job output. Add the npm ecosystem entry for /.github/groom so Dependabot opens the bump PR. - Exact pins only: the gate fails the run on a range or dist-tag, and the Dependabot entry sets `versioning-strategy: increase` so a bump stays exact. - tests/test_claude_code_pin.py guards the arrangement (exact pin, no version literal left in groom.yml, every install step wired to the gate output, npm entry present); test-groom-scripts.yml's path filter now reaches groom.yml and dependabot.yml so the guard actually runs on the changes it guards.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Comment |
mattmillerai
marked this pull request as ready for review
July 31, 2026 13:45
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 5 |
| 🟢 Low | 4 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
Addresses the cursor-review panel on #101. groom.yml — the pin resolve step: - Moved to the END of `gate` and gated on `should_run`. It was the one fail-CLOSED step in a job whose every other step fails open, running ahead of the interval/volume gates, so a bad manifest (e.g. at a stale `workflows_ref`) would red out the ~6 of 7 daily ticks that were never going to install anything. All six consumer jobs are themselves gated on `should_run`, so nothing downstream can observe the difference, and a bad pin still cannot reach main — test_claude_code_pin.py runs on any PR touching `.github/groom/**`. - `?` on the jq index, so a `dependencies` that is present but not an object reaches the legible empty-version branch instead of dying on a raw jq "cannot index" error under `set -e`. - Strip newlines from the version before it is echoed into `::error::`. It is an arbitrary JSON string at that point and had not been validated yet, so an embedded newline could emit further workflow commands into the job log. - Tighten the guard to strict SemVer. The old suffix match accepted junk like `1.2.3-a..b`, which npm resolves as a mutable dist-tag — the un-pinning the guard exists to reject. Verified against all 472 published versions of @anthropic-ai/claude-code: zero are rejected, so a real bump cannot false-fail. test_claude_code_pin.py: - `_EXACT_VERSION` mirrors the tightened shell regex and is matched with `fullmatch`, not `assertRegex`, whose `$` also accepts a trailing newline. New case-table test pins the shared contract. - Job-boundary regex widened to `[A-Za-z_][A-Za-z0-9_-]*`; `-` and uppercase are legal job IDs, and a job named e.g. `build-cli` was not seen as a boundary, folding its steps into the preceding job's block. - `needs:` is now parsed rather than substring-matched, so the block-sequence form and `needs: [audit_find, gate]` stop false-failing. - Comment lines are stripped before scanning. The `gate` job's own prose contains both `npm install -g ...` and the words `needs: gate`, so the install-job assertion was passing VACUOUSLY on a job that installs nothing; the set of installing jobs is now asserted explicitly. - Dependabot keys are checked against ONE entry instead of three independent substring matches over the whole file.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 6 |
| ⚪ Nit | 2 |
Panel: 8/8 reviewers contributed findings.
…the guards (BE-5373) Review resolution for #101. - Load `.github/groom/package.json` from a sparse checkout at `job.workflow_sha` — the commit this groom.yml was itself read from — instead of `$GROOM_ASSETS` at `inputs.workflows_ref`. `workflows_ref` defaults to the mutable `main`, so a caller that SHA-pins `uses:` but omits it had the agent CLI (executable supply chain) tracking a branch tip while the sandbox flags that depend on it stayed frozen at the pinned SHA. It also made the new fail-closed resolve step turn this repo's documented split-pin state into a total groom outage; read from the workflow's own commit that case cannot arise. NOT `github.job_workflow_sha`, which silently expands to "" in a reusable-workflow job; the step re-checks and warns if it is empty. - Reject leading zeros and unbounded numeric components in the exact-version guard, in both the shell regex and its unit-test mirror: `01.02.03` and `9007199254740992.0.0` passed while node-semver rejects them, so npm would have resolved them as mutable dist-tags — the un-pinning the guard exists to prevent. - Type-check the manifest at both levels in jq, so a top-level non-object (or a file that is not JSON) fails with the `::error::` annotation rather than a raw jq error under `set -e`. - Test guards, all of which could red on a correct file: strip inline YAML comments, accept `needs:` block sequences at the key's own indentation, compare dependabot scalars unquoted, key the hardcoded-literal scan on the absence of a `$` expansion rather than a `${`/`"` prefix allowlist (which both flagged a valid `@$VAR` and let a quoted literal through), and comment-strip the install-site count.
Contributor
Author
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-codepin, currently2.1.217(unchanged from what the workflow installed before). It is a pin carrier, not a project: no lockfile, nothing is evernpm 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— thegatejob (which already checks out this repo's groom assets) reads the pin once, validates it, and exports it as aclaude_code_versionjob output. All threeInstall Claude Codesteps (audit_find,audit_verify,build) consume that output vianeeds.gate.outputs— every one of those jobs alreadyneeds: gate, so no new job edges. No version literal remains in the workflow..github/dependabot.yml— annpmentry fordirectory: "/.github/groom"matching the existing monthly schedule,labels: ["dependencies"], and cooldown settings, plus an explicitversioning-strategy: "increase"..github/groom/tests/test_claude_code_pin.py(new) — guards the arrangement: exact pin, no hardcoded version literal anywhere ingroom.yml, every install step wired to the gate output, every CLI-installing jobneeds: gate, and the Dependabot entry still present..github/workflows/test-groom-scripts.yml— the path filter now also covers.github/workflows/groom.ymland.github/dependabot.yml. Without this the new guard would never run on the very changes it guards (a re-added literal ingroom.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:constantThe 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 anactions/checkoutof this repo atinputs.workflows_refinto$GROOM_ASSETS(that is how the briefs andledger.pyare 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.@anthropic-ai/claude-codeon 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).package.jsonmatches the.github/groom/**path filter, sotest_claude_code_pin.pyruns on that PR and fails it before merge. I confirmed the guard fails as intended by temporarily setting the pin to^2.1.217and re-adding a hardcoded literal togroom.yml— 3 tests failed — then restoring.The guard is the last step in
gateand runs only whenshould_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 staleworkflows_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 reachmainin the first place —test_claude_code_pin.pyruns on any PR touching.github/groom/**, which includespackage.jsonitself. 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.actionlintclean on every workflow. This is the substantive check on the part I could not run for real: actionlint statically resolvesneeds.<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.jqread → validation →$GITHUB_OUTPUT);shellcheckclean; confirmed the${VAR:?…}guard exits non-zero with a legible message when the variable is unset or empty.groom.yml,dependabot.yml,test-groom-scripts.yml; JSON parse ofpackage.json.Unmet acceptance criterion — please read
The ticket's step 4 ("verify a groom run still installs and executes the CLI via
workflow_dispatchonci-groom.yml") is NOT done, and is not currently doable by anyone.ci-groom.ymlonmainstill carries the literal placeholderREPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHAin both itsuses:ref and itsworkflows_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 astartup_failureand the five since are allfailure, 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_CALLERSroster pick this change up through the normalbump-groom-callersflow after merge, and the first of those runs is the real end-to-end confirmation.Judgment calls
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.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.2.1.217value. 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 adependenciesthat is present but not an object reaches the legible error instead of dying on a raw jq "cannot index" underset -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 accepted1.2.3-a..b, which npm resolves as a mutable dist-tag.Fixed in
test_claude_code_pin.py:fullmatchinstead ofassertRegex; job-boundary regex widened to allow-and uppercase (both legal job IDs);needs:parsed rather than substring-matched, so the block-sequence form andneeds: [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_gatewas passing vacuously on thegatejob itself.gate's prose contains both the stringnpm install -g <pkg>@<ver>(explaining why the pin is not inline) and the wordsneeds: 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 checkoutpath:, so the suggested literal would be the wrong path); the "nodependencieskey" jq repro does not reproduce either (jq permits string-indexingnull) though the adjacent non-object case was real and is fixed; and theworkflows_refsplit-pin concern is the repo's existing, documented trust model rather than something this PR introduces —config.py,interval.py,ledger.pyand 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,
actionlintclean across every workflow (and confirmed still load-bearing — a typo'd step id in the newif: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.ymlonmainstill carries theREPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHAplaceholder, so the end-to-end "the CLI actually installs at the resolved version" leg remains unproven by anyone.