fix(claude-ops): correct converge's settings-write claim and scope-keying gap - #2519
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 1m 4s —— View job Security Review
Skipped — no security-relevant surface. All five changed files are documentation and metadata:
No code, scripts, GitHub Actions workflows, permissions, credential handling, or executable logic is touched — the change corrects factual claims in skill guidance text about existing Claude Code CLI behavior. Nothing here introduces an injection surface, authorization gap, or supply-chain/trust-boundary risk, so per the security-review skill's skip gate this lane has nothing to report. |
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e6b212e51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Claude finished @kyle-sexton's task in 4m 40s —— View job Claude Code Review
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ying gap
converge.md Step 5 said `uninstall -s project` "can remove an enabledPlugins
entry" from committed settings, which reads as "a clean tree means nothing was
written" — so a run that predicted no diff had grounds to skip the check. It
always writes. Verified on Claude Code 2.1.228 with single calls against a clean
tracked .claude/settings.json: it empties the map to `"enabledPlugins": {}`
rather than deleting the key, writes the key even into a file that never had
one, and rewrites the file in Claude Code's key order so unrelated sibling keys
move. Step 5 now checks every touched project unconditionally and classifies the
diff as inert or substantive, because the two warrant opposite advice.
scope-semantics.md gains that as a section beside the update exemption, which
was re-verified on the same version under the hardest available conditions and
still holds.
Also recorded: project scope keys on the working directory, not the repository —
two checkouts of one repo pin independently, so converge must keep them as
separate rows with separate cd targets. And one observation on
installed_plugins.json write timing, logged as a data point that does not retire
sync.md's <new> fallback.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
4e6b212 to
0d8cec4
Compare
|
Claude finished @kyle-sexton's task in 1m 20s —— View job Security Review
Skipped — no security-relevant surface, and unchanged since the last review. The head SHA moved from This PR's actual changes remain:
No code, scripts, GitHub Actions workflows, permissions, or executable logic is touched. The three |
|
Claude finished @kyle-sexton's task in 3m 1s —— View job Claude Code Review
|
…ns call (#2527) ## Summary Follow-up to #2519, which merged before its review findings were addressed. Both findings were real; both are now confirmed by experiment on Claude Code 2.1.228 rather than by wording judgment. **1. "`converge` is the only action this skill runs that surfaces a settings diff" was false.** `sync.md` Step 5 issues `claude plugin enable <id> -s <that scope>`, and `enable` sets the same `enabledPlugins[id]` key `install` was verified to write. Tested, one call each against a clean tracked `.claude/settings.json`: | Call | Writes `.claude/settings.json`? | |---|---| | `install <id> -s project` | yes | | `uninstall <id> -s project` | yes | | `enable <id> -s project` | yes (adds the id, `true`) | | `disable <id> -s project` | yes (adds the id, `false`) | | `update <id> -s project` | no | `-s local` writes the gitignored `.claude/settings.local.json` and never touches the tracked file — tested, not assumed, since Step 5 issues both. So `sync`, the default action, can dirty a team-shared tracked file with no diff surfaced. This change documents that exposure and tells Step 5 to name a `-s project` enable in the report; **giving Step 5 converge's full diff-surfacing is a behavior change to the default action and is deliberately left to its own PR.** Also recorded: `enable -s project` gates on the *merged effective* value — enabling an id that is `true` only at user scope fails rather than writing a project entry. **2. Project-scope keying: both the original wording and the proposed correction were wrong.** #2519 said "keys on the working directory"; review proposed "checkout root", citing `fleet-state.sh` and its test. Testing settled it against both: - The **CLI** keys on the literal cwd — `install -s project` from `<checkout>/nested/subdir` recorded that subdirectory as `projectPath` and created its own `nested/subdir/.claude/settings.json`. - **`fleet-state.sh`** resolves the checkout root (`fleet-state.sh:211-221`, read directly rather than taken from the review), and `fleet-state.test.sh` pins the nested-subdirectory case. The layers disagree, and the gap is the finding, not either wording: a project-scope install from below the checkout root never matches `currentProject`, never updates, and never appears in a divergence row — while still loading in that subtree. Recorded in `gotchas.md`, where the sibling silent-mismatch gotchas live. `converge.md` Step 2 keeps only the worktree consequence the evidence supports. ## Test plan - `scripts/affected-tests.sh --run` — 3 suites selected, all pass (`check-skill.test.sh`, `check-changed-skills.test.sh`). - `markdownlint-cli2` on all changed markdown — 0 issues. - `lychee --offline` — 0 errors. - `check-skill.sh` on `plugins/claude-ops/skills/plugins` — **PASS**, 0 errors, 0 warnings. - `plugin.json` parses; version 0.31.7 → 0.31.8. - The released `## [0.31.7]` CHANGELOG section was diffed against `origin/main` and is byte-identical — this PR adds `## [0.31.8]` above it rather than editing shipped release notes. - Every experiment torn down: scratch project-scope installs removed, the `nested/` tree deleted, the scratch `settings.local.json` removed, and both checkouts left clean. ## Related Closes #2526 Follows #2519. Upstream context for the underlying CLI behavior is in `anthropics/claude-code#81058`, where this session's 2.1.228 findings are posted as a comment. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
1 participant
Summary
/claude-ops:plugins convergepredicted thatclaude plugin uninstall -s projectwould leave aproject's committed
.claude/settings.jsonuntouched when that file carried noenabledPluginskey. It does not. Running converge against 18 project-scope pins dirtied the tracked settings file
in both affected checkouts, and
converge.mdStep 5's wording ("can remove anenabledPluginsentry") gives a run grounds to skip the diff check on exactly that wrong prediction.
Verified on Claude Code 2.1.228 with single calls against a clean tracked
.claude/settings.json, git-diffed after every step:install -s projectenabledPlugins(true)update -s projectuninstall -s project"enabledPlugins": {}Both writing calls rewrite the whole file in Claude Code's key order, so unrelated sibling keys
move. The resulting blob was byte-identical to what the original converge run produced.
Changes:
converge.mdStep 5 — check every touched project unconditionally (a clean tree after anuninstall is the surprising outcome, not a dirty one), and classify the diff before showing it:
inert (empty map plus key reorder — recommend discarding, so a team-shared file carries no
churn) or substantive (an entry actually removed — the user decides).
scope-semantics.md— new verified-facts section for install/uninstall, beside the updateexemption, which is re-verified on the same version and left intact.
converge.mdStep 2 — project scope keys on the working directory, not the repository.Verified by uninstalling one id in a repo's main checkout and watching its
git worktree's recordfor the same id survive. Two checkouts sharing one
.gitand one tracked.claude/settings.jsonpin independently, so each needs its own
cd.sync.md— one observation oninstalled_plugins.jsonwrite timing (21/21 CLI-reportedupdates already visible to a post-sweep re-read). Logged explicitly as a single data point that
does not retire the
<new>fallback, since it shows only that the write landed before there-read on that run.
Scoped to what was observed: no claim that the behavior holds on other Claude Code versions.
Test plan
scripts/affected-tests.sh --explain— every changed file is a recorded no-suite class; no shellsuite selected.
markdownlint-cli2on all four changed markdown files — 0 issues.lychee --offlineon the changed markdown — 0 errors (covers the newconverge.mdrelative link).check-skill.shonplugins/claude-ops/skills/plugins— PASS, 0 errors, 0 warnings; runs theskill's own
fleet-state.test.sh, which passed.biome checkandeditorconfig-checkeron the changed files — clean.checkouts left clean, and
fleet-state.shreports 0 actionable divergences.Related
Closes #2520
Found while running
/claude-ops:plugins syncthenconvergeon a real fleet — the converge runproduced an unexpected settings diff in two checkouts, which is what surfaced the wrong prediction.
Upstream
anthropics/claude-code#81058reports a related but more destructive variant on 2.1.220(the
enabledPluginskey deleted from user settings, disabling a surviving user-scope install).That did not reproduce on 2.1.228 — the user-scope entries survived the converge run intact.