[PR #104] [MERGED] docs(groom): document the CLI pin's accepted boundary and guard its dependency shape (BE-5582) #114

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: matt/be-5373-claude-code-version-pin ← Head: matt/be-5582-pin-boundary-doc-guard

📝 Commits (2)

927c377 docs(groom): document the CLI pin's accepted boundary and guard its dependency shape (BE-5582)
fb2c69f fix(groom): close the dependency-shape guard's blind spots (BE-5582)

📊 Changes

4 files changed (+586 additions, -2 deletions)

View changed files

📝 .github/groom/README.md (+97 -0)
📝 .github/groom/tests/test_claude_code_pin.py (+471 -0)
📝 .github/workflows/groom.yml (+4 -2)
📝 .github/workflows/test-groom-scripts.yml (+14 -0)

📄 Description

STACKED — merging lands on matt/be-5373-claude-code-version-pin (owned by @mattmillerai, PR #101), NOT main.
Every file this PR touches exists only on that branch. Review the net diff (3 files); GitHub retargets this to main once #101 merges.

ELI-5

The robot's tool gets installed fresh from the internet each run. We already pin which version we install; someone worried the tool's own ingredients could still change under us. We checked: this tool has no outside ingredients — only its own maker's parts, locked to the same version. So instead of rebuilding how we install it, we write down why the current pin is enough, and add an automatic check that yells if a future version ever starts pulling in outside ingredients again.

What & why

Review thread discussion_r3691084270 on #101 noted the top-level CLI pin does not pin transitive deps, since npm install -g has no lockfile. Spike BE-5580 established the tree is already closed at 2.1.x, so the decision is to accept the top-level pin as the full boundary, document why, and guard the one real residual risk.

Verified against the live registry while writing this:

$ npm view "@anthropic-ai/claude-code@2.1.217" dependencies optionalDependencies
{ "dependencies": {},
  "optionalDependencies": { 8 × "@anthropic-ai/claude-code-<platform>": "2.1.217" } }

$ npm view "@anthropic-ai/claude-code-linux-x64@2.1.217" dependencies optionalDependencies scripts
(all three empty)

Three changes, zero runtime change:

.github/groom/tests/test_claude_code_pin.py — new TestPinnedDependencyShape. Reads the pin via the existing _MANIFEST / _EXACT_VERSION constants and fails if dependencies is non-empty, if an optionalDependencies key leaves the @anthropic-ai/ scope, or if any value is not the exact pinned version string (equality, not range-satisfaction — ^2.1.217 satisfies 2.1.217 today and floats tomorrow).
.github/groom/README.md — new ### Scope: why the top-level pin is the whole pin: the closed-tree argument, the two accepted residual risks, and the note that 1.x–2.0.0 genuinely did carry floating @img/sharp-*: ^0.33.5.
.github/workflows/groom.yml — the "Install Claude Code" comment's last clause retargeted at the decision. Comment-only; no run: or step touched.

Verification

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v → 182 passed, in both CI=true and local modes.
Guard proven against the real historical regression: temporarily pinning the manifest to 2.0.0 makes it fail with
@anthropic-ai/claude-code@2.0.0 declares @img/sharp-darwin-arm64@^0.33.5: the dependency tree is no longer closed under the exact top-level pin … Re-open the transitive-pinning decision (spike BE-5580) before bumping.
Reverted; the manifest is untouched in this diff.
Degradation paths exercised with a stub npm on PATH: registry error → fail under CI=true, skip locally; no npm at all → skip in both.

Judgment calls

One field per npm view call, not the two-field form the ticket sketched. Empirically, npm view is-odd@3.0.1 dependencies optionalDependencies --json prints {"is-number": "^6.0.0"} — bare and unlabelled, because npm keys by field name only when both fields are present. A keyed reader therefore cannot distinguish that from a two-field reply with dependencies absent, so a future version declaring real dependencies and no optionalDependencies would parse as "both empty" and pass — exactly the regression being guarded. One field per call has no such ambiguity. Results are cached per (spec, field), so the three test methods cost two registry hits total.
No setup-node added to test-groom-scripts.yml, per the ticket. ubuntu-latest ships npm, and no-npm skips rather than fails, so a future image change degrades quietly instead of reddening unrelated PRs.
New network dependency, flagged deliberately: this suite was previously hermetic. TestPinnedDependencyShape is the only class that reaches the network, and a registry error hard-fails CI on any PR touching .github/groom/**, groom.yml, dependabot.yml, or the test workflow. That fail-in-CI behaviour is ticket-mandated (an unverifiable guard is an absent guard) and is the deliberate cost of the guard; the 60s timeout is generous for a cold runner. If it proves flaky in practice, the narrow knob is to relax _in_ci() handling rather than delete the guard.

Not done

Nothing from the ticket's scope is unimplemented. Per the ticket, no groom workflow_dispatch re-validation was run — the install steps and all runtime behaviour are untouched (README, test, and one comment string).

_{🔄 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/104 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/1/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-5373-claude-code-version-pin` ← **Head:** `matt/be-5582-pin-boundary-doc-guard` --- ### 📝 Commits (2) - [`927c377`](https://github.com/Comfy-Org/github-workflows/commit/927c37796e7a3019d26b248c3e8ada61478b0939) docs(groom): document the CLI pin's accepted boundary and guard its dependency shape (BE-5582) - [`fb2c69f`](https://github.com/Comfy-Org/github-workflows/commit/fb2c69f91434c01fbfc35899a5fa539a0b987d92) fix(groom): close the dependency-shape guard's blind spots (BE-5582) ### 📊 Changes **4 files changed** (+586 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+97 -0) 📝 `.github/groom/tests/test_claude_code_pin.py` (+471 -0) 📝 `.github/workflows/groom.yml` (+4 -2) 📝 `.github/workflows/test-groom-scripts.yml` (+14 -0) </details> ### 📄 Description > **STACKED — merging lands on `matt/be-5373-claude-code-version-pin` (owned by @mattmillerai, PR #101), NOT `main`.** > Every file this PR touches exists only on that branch. Review the net diff (3 files); GitHub retargets this to `main` once #101 merges. ## ELI-5 The robot's tool gets installed fresh from the internet each run. We already pin which version we install; someone worried the tool's own ingredients could still change under us. We checked: this tool has no outside ingredients — only its own maker's parts, locked to the same version. So instead of rebuilding how we install it, we write down why the current pin is enough, and add an automatic check that yells if a future version ever starts pulling in outside ingredients again. ## What & why Review thread `discussion_r3691084270` on #101 noted the top-level CLI pin does not pin transitive deps, since `npm install -g` has no lockfile. Spike BE-5580 established the tree is already closed at 2.1.x, so the decision is to **accept the top-level pin as the full boundary, document why, and guard the one real residual risk**. Verified against the live registry while writing this: ``` $ npm view "@anthropic-ai/claude-code@2.1.217" dependencies optionalDependencies { "dependencies": {}, "optionalDependencies": { 8 × "@anthropic-ai/claude-code-<platform>": "2.1.217" } } $ npm view "@anthropic-ai/claude-code-linux-x64@2.1.217" dependencies optionalDependencies scripts (all three empty) ``` Three changes, **zero runtime change**: 1. **`.github/groom/tests/test_claude_code_pin.py`** — new `TestPinnedDependencyShape`. Reads the pin via the existing `_MANIFEST` / `_EXACT_VERSION` constants and fails if `dependencies` is non-empty, if an `optionalDependencies` key leaves the `@anthropic-ai/` scope, or if any value is not the exact pinned version **string** (equality, not range-satisfaction — `^2.1.217` satisfies 2.1.217 today and floats tomorrow). 2. **`.github/groom/README.md`** — new `### Scope: why the top-level pin is the whole pin`: the closed-tree argument, the two accepted residual risks, and the note that 1.x–2.0.0 genuinely did carry floating `@img/sharp-*: ^0.33.5`. 3. **`.github/workflows/groom.yml`** — the "Install Claude Code" comment's last clause retargeted at the decision. Comment-only; no `run:` or step touched. ## Verification - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v` → **182 passed**, in both `CI=true` and local modes. - **Guard proven against the real historical regression:** temporarily pinning the manifest to `2.0.0` makes it fail with `@anthropic-ai/claude-code@2.0.0 declares @img/sharp-darwin-arm64@^0.33.5: the dependency tree is no longer closed under the exact top-level pin … Re-open the transitive-pinning decision (spike BE-5580) before bumping.` Reverted; the manifest is untouched in this diff. - Degradation paths exercised with a stub `npm` on `PATH`: registry error → **fail** under `CI=true`, **skip** locally; no `npm` at all → **skip** in both. ## Judgment calls - **One field per `npm view` call, not the two-field form the ticket sketched.** Empirically, `npm view is-odd@3.0.1 dependencies optionalDependencies --json` prints `{"is-number": "^6.0.0"}` — bare and unlabelled, because npm keys by field name only when *both* fields are present. A keyed reader therefore cannot distinguish that from a two-field reply with `dependencies` absent, so a future version declaring real `dependencies` and no `optionalDependencies` would parse as "both empty" and **pass** — exactly the regression being guarded. One field per call has no such ambiguity. Results are cached per `(spec, field)`, so the three test methods cost two registry hits total. - **No `setup-node` added to `test-groom-scripts.yml`**, per the ticket. `ubuntu-latest` ships npm, and no-npm skips rather than fails, so a future image change degrades quietly instead of reddening unrelated PRs. - **New network dependency, flagged deliberately:** this suite was previously hermetic. `TestPinnedDependencyShape` is the only class that reaches the network, and a registry error hard-fails CI on any PR touching `.github/groom/**`, `groom.yml`, `dependabot.yml`, or the test workflow. That fail-in-CI behaviour is ticket-mandated (an unverifiable guard is an absent guard) and is the deliberate cost of the guard; the 60s timeout is generous for a cold runner. If it proves flaky in practice, the narrow knob is to relax `_in_ci()` handling rather than delete the guard. ## Not done Nothing from the ticket's scope is unimplemented. Per the ticket, **no groom `workflow_dispatch` re-validation** was run — the install steps and all runtime behaviour are untouched (README, test, and one comment string). --- <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:23 +00:00