fix(claude-ops): widen the settings-write facts to every enabledPlugins call - #2527
Conversation
…ns call Review found two defects in the first pass, both confirmed by experiment. `scope-semantics.md` claimed converge was the only action surfacing a settings diff. False by this change's own evidence: sync's Step 5 issues `enable -s <that scope>`, and `enable -s project` writes the committed file exactly as install does. The section is now a verified table — install, uninstall, enable, disable write at project scope; update does not; `-s local` writes the gitignored settings.local.json — and sync.md Step 5 names the exposure, with the diff-surfacing remediation tracked separately rather than widened into this change. The "keys on the working directory" wording drew a correction toward "checkout root". Testing settled it against both readings: `install -s project` from `<checkout>/nested/subdir` records that subdirectory as projectPath and creates its own .claude/settings.json, so the CLI is cwd-keyed — while fleet-state.sh resolves the checkout root (fleet-state.sh:211-221). The layers disagree, and that gap is the finding: a plugin installed 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; converge.md Step 2 keeps only the worktree consequence it can support. Also recorded: `enable -s project` gates on the merged effective value, so enabling an id true only at user scope fails rather than writing a project entry. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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. |
The cherry-pick carried this change's notes inside the already-released 0.31.7 section. Restore that section to the shipped text and open 0.31.8 above it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 55s —— View job Security review
Skip gate: no security-relevant surface. All six changed files are documentation and metadata:
No executable code, scripts, or GitHub Actions workflows are touched. There is no new input handling, no command construction, no credential/token/secret handling, and no change to CI permissions or trigger patterns. The content documents existing CLI scope semantics for a human/agent operator to read; it does not itself execute anything security-sensitive. No security issues found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7847c3c68
ℹ️ 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".
|
Last security-reviewed head: |
|
Claude finished @kyle-sexton's task in 2m 34s —— View job Code review complete
This repo carries 🔴 Important — the PR's own new evidence contradicts two unrevised "converge is the only writer" claimsThis PR documents, with fresh empirical verification, that That directly contradicts two claims this PR leaves untouched, both load-bearing for how a reader trusts the rest of the skill:
Both statements are now false by this PR's own findings: OtherwiseThe rest of the diff is careful, self-consistent documentation work: the new write-behavior table, the |
…d spot Address review threads: entrypoint now names sync's enable -s project write path; gotchas narrows the subdirectory claim to currentProject-only operations. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 34s —— View job Security review
Skip gate: no security-relevant surface. Per the prior review comment, the last security-reviewed head was That commit touches three files, all documentation prose in the
These address the two Codex review findings from the prior push (version-bump/changelog parity and the subdirectory blind-spot scoping). No executable code, scripts, or GitHub Actions workflows are touched in this range — it's skill guidance prose only, describing existing CLI/tooling behavior for a human/agent operator to read. No new input handling, no command construction, no credential/secret handling, no CI permission or trigger changes. No security issues found. |
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @cursor[bot]'s task in 3m 26s —— View job Code review
Skip gate: does not apply. This is a docs-only PR, but the content is the skill's factual-claims layer that an agent executes against, and the PR is explicitly about correcting prior factual errors — a meaningful review surface, not trivial/automated churn. Both prior Codex review comments (CHANGELOG version-reuse, and the entrypoint's stale "converge is the only action" summary) already appear resolved in the current head ( Findings🔴 Important —
Per The downstream narrative (" No other correctness issues found. The rest of the rewritten claims in |
…ot null Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 33s —— View job Security review
Skip gate: no security-relevant surface. Per the prior review comment, the last security-reviewed head was That commit changes exactly one line, in one file:
No executable code, scripts, or GitHub Actions workflows are touched. No new input handling, no command construction, no credential/secret handling, no CI permission or trigger changes. Purely a one-word prose correction. No security issues found. |
|
Claude finished @cursor[bot]'s task in 2m 12s —— View job Code review complete
Skip gate: does not apply. Docs-only PR, but the content is the skill's factual-claims layer that an agent executes against — a meaningful review surface. This PR has already been through two rounds of this lane plus Codex, and both real findings from the prior round are now fixed correctly in this head (
Also verified the earlier "converge is the only action that can touch committed settings" claims (flagged by the first review round) are now reworded consistently in both No new findingsNo further correctness or maintainability issues in this diff. The write-behavior table, the |
…invariant (#2539) ## Summary `SKILL.md` states the rule the skill is built around: > `converge` is the one action that can touch a committed `.claude/settings.json`, and only after an > explicit per-plugin confirm. `sync.md` Step 5 broke it. It issued `claude plugin enable <id> -s project` for every `currentProject: true` completeness gap, and 0.31.8 verified on Claude Code 2.1.228 that this call writes exactly that file. So the default, headless-capable action silently modified a team-shared tracked file. 0.31.8 documented the exposure and asked the report to name it — a mitigation, not a fix — and in the interim the invariant itself had been softened in `SKILL.md` and `converge.md` to *describe* the violation rather than remove it. **The fix.** Step 5 now enables automatically only where the write is not team-shared state: - **`user`** — machine-scope `~/.claude/settings.json`. - **`local`** — the gitignored `.claude/settings.local.json` (verified in #2526). - **`project`** — never enabled automatically. Reported as an "Action needed" row carrying the runnable command, in the `cd`-into-its-own-`projectPath` form `converge.md` mandates and with the fully-qualified id `gotchas.md` requires, so acting on it is a copy rather than a reconstruction. **Why not confirm instead of skip.** `converge` can afford a confirm gate because it *aborts* in an autonomous session — `CLAUDE_CODE_REMOTE`, `/loop`, `/schedule`. `sync` has no such abort by design; it is the on-demand and headless maintenance action, and `/claude-ops:lanes` runs it from scheduled background sessions. A path that writes team-shared state cannot be made safe by asking when there may be nobody to answer. The write must not happen. After this change no `sync` path writes a committed settings file, so `SKILL.md` and `converge.md` state the invariant plainly again instead of carving out `sync`. ## Test plan - `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. - `scripts/affected-tests.sh --run` — every changed file is a recorded no-suite class. - `plugin.json` parses; version 0.31.9 → 0.31.10. - Consistency sweep: grepped the skill tree for any surviving claim that `sync` writes committed settings — none remain. The 0.31.8 "Known exposure" paragraph is gone, and the two softened invariant statements are restored. ## Related Closes #2535 Completes the remediation deferred in #2526 / #2527, which documented this exposure but left it unscheduled. Underlying CLI behavior is recorded upstream in `anthropics/claude-code#81058`. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. "
convergeis the only action this skill runs that surfaces a settings diff" was false.sync.mdStep 5 issuesclaude plugin enable <id> -s <that scope>, andenablesets the sameenabledPlugins[id]keyinstallwas verified to write. Tested, one call each against a cleantracked
.claude/settings.json:.claude/settings.json?install <id> -s projectuninstall <id> -s projectenable <id> -s projecttrue)disable <id> -s projectfalse)update <id> -s project-s localwrites the gitignored.claude/settings.local.jsonand never touches the tracked file —tested, not assumed, since Step 5 issues both. So
sync, the default action, can dirty ateam-shared tracked file with no diff surfaced. This change documents that exposure and tells Step 5
to name a
-s projectenable in the report; giving Step 5 converge's full diff-surfacing is abehavior change to the default action and is deliberately left to its own PR.
Also recorded:
enable -s projectgates on the merged effective value — enabling an id that istrueonly 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.shand its test. Testing settled it against both:
install -s projectfrom<checkout>/nested/subdirrecordedthat subdirectory as
projectPathand created its ownnested/subdir/.claude/settings.json.fleet-state.shresolves the checkout root (fleet-state.sh:211-221, read directly ratherthan taken from the review), and
fleet-state.test.shpins 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 adivergence row — while still loading in that subtree. Recorded in
gotchas.md, where the siblingsilent-mismatch gotchas live.
converge.mdStep 2 keeps only the worktree consequence the evidencesupports.
Test plan
scripts/affected-tests.sh --run— 3 suites selected, all pass (check-skill.test.sh,check-changed-skills.test.sh).markdownlint-cli2on all changed markdown — 0 issues.lychee --offline— 0 errors.check-skill.shonplugins/claude-ops/skills/plugins— PASS, 0 errors, 0 warnings.plugin.jsonparses; version 0.31.7 → 0.31.8.## [0.31.7]CHANGELOG section was diffed againstorigin/mainand is byte-identical— this PR adds
## [0.31.8]above it rather than editing shipped release notes.nested/tree deleted, thescratch
settings.local.jsonremoved, 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.