fix(claude-ops): remediate the plugins:sync audit findings (0.36.0)

[kyle-sexton]

Closes #3112

Summary

Remediates the post-use plugin-quality audit of claude-ops:plugins' sync action — all 15 findings, plus a catalog-version pre-filter worth more than the rest combined. Every harness-behaviour claim added or changed here is version-stamped against Claude Code 2.1.240, the CLI it was verified on.

Three silent-failure paths closed, all of the same shape: correct-looking output from a path that did not run.

• F1 (high) — Step 2, the self-described "primary value path", no-opped invisibly. fleet-state.sh computed currentProject as a correct tri-state, but null covers "user-scope record", "no projectPath", and "no project context resolved at all", so a run from $HOME and a run inside a repo with no in-repo installs produced an identical report. New top-level project_root carries the distinction; the report gains a fixed In-repo: row that must state a skip.
• F4 (medium-high) — sync all refreshed every marketplace, then ran install/update/enable maintenance against exactly one. Steps 2–5 are now the per-marketplace loop body, every --ids call carries --marketplace, and a new --marketplaces mode enumerates the names CR-free so the loop needs no hand-written jq.
• F2 (high) — every actionable divergence routed to a converge command that cannot execute, because -s project has no path flag and the recorded projectPath no longer exists. Those rows now get their own report section, outside the Divergences count, and converge emits them as blocked rather than runnable.

Design decision on F2, which the brief left to me: stale project records are classified and reported, never converged and never reaped. No claude plugin verb removes an install record by path, editing installed_plugins.json is outside this skill's boundary, and claude-ops must not read another plugin's userConfig to decide whose directories these were — so the wording is deliberately generic about "a tool that owns those directories' lifecycle" rather than naming one. projectPathPresent is advisory and may never filter: an unmounted volume, an offline share, and unplugged removable media are indistinguishable from a deleted worktree to a directory test, so the report says "not present on this machine", never "dead".

Fix

Catalog-version pre-filter (the performance finding). marketplace.json entries carry no version — which is why Step 3 called claude plugin update for every user-scope install and let the CLI no-op. Each plugin's version does exist in the marketplace checkout at <installLocation>/<entry.source>/.claude-plugin/plugin.json. fleet-state.sh now exposes it as catalog_versions, and --ids update-candidates-user withholds only ids it positively proved already sit at the catalog version. On this machine's already-current fleet: 66 update calls → 0.

It fails open by construction, and that is the dominant path, not an edge case. Measured across nine registered marketplaces: resolves fully for five, partially for two (13/53, 1/15), not at all for two. Any unreadable version yields a candidate, exactly as if no pre-filter existed. The test asserts the degradation as a byte-equality against installed-user, not a spot check. The pre-filter is also disabled — falling back to the unfiltered installed-user — for any marketplace whose Step 1 refresh failed, since a stale checkout cannot prove an id current; and audit mode, which never refreshes, reports its prediction as a lower bound carrying the catalog's lastUpdated.

The manifest must sit inside the checkout, enforced physically. source is third-party content and the only unsafe direction is withholding an update. A lexical ../ refusal is insufficient — a symlink inside the checkout is reached by an ordinary ./name source no string check can see — so the resolved manifest path is required to sit under the resolved checkout root with symlinks followed.

Also fixed: pluginConfigs read-path claim (user / --settings / managed only; project and local ignored since v2.1.207, while enabledPlugins still honors them); /reload-plugins --force restated as the docs' two-step with prompt-cache invalidation as the trigger; divergence count split into run-caused vs pre-existing; self-update row when the sweep updates claude-ops itself; TOCTOU wording matched to the implementation with the inert detector replaced; versionsMatch reduced to one origin plus pointers; user_scope_orphans for the structurally-invisible single-scope orphans.

setup — now load-bearing, not tidiness. #3115 regenerated this plugin's README with the corrected reconfiguration guidance while skills/setup/SKILL.md still prescribed the destructive uninstall/reinstall cycle — a contradiction live on main that #3115's own reviewer flagged and could not fix, because that file is in this change set's fence. This PR closes it, matching the landed fleet-wide wording verbatim apart from this plugin's option list, including the distinction that the write lands while the running session's behavior does not (${user_config.*} is injected at skill load and CLAUDE_PLUGIN_OPTION_* comes from an environment fixed at session start, so a same-session check still reports the old value and reading that as a failed write is wrong). Also corrects an inherited miscount: 15 options and 8 *_audit_enabled toggles, not 14 and 7.

Deferred, with reasons in the CHANGELOG: --run-log in fleet-state.sh (conflicts with the script's read-only contract; F12's cheapest tier landed instead), a fourth install_new value (needs durable state), F8's --selfcheck, F14 (the repo's own check-changed-skills.sh enforces trigger-keyword preservation, and the finding is cosmetic since the skill sets disable-model-invocation: true), and the upstream issue for the missing record-reaping verb.

Verification

Two independent fresh-context verifier agents reviewed the final state against the sealed audit packet. Both initially returned FAIL; both sets of defects were fixed and re-verified.

Round 1 found the change had made Step 3 catalog-dependent without extending the stale-catalog deferral rule — a stale checkout could withhold an id as "already current". Fixed at the decision point, plus three unstamped claims and an over-flattering CHANGELOG figure.

Round 2 found the symlink-containment hole above, and that audit mode never triggers the stale-catalog fallback. Both fixed.

Worth recording: the symlink regression test initially failed for the wrong reason — Git Bash's ln -s silently deep-copies instead of linking, so the fixture was asserting against a real in-checkout directory and 9.9.9 was the correct answer for what was on disk. The test now creates a genuine symlink via MSYS=winsymlinks:nativestrict, gates on [[ -L ]], and skips honestly where the platform yields no real symlink. Verified against real symlinks from both ln -s and cmd mklink /D.

Test plan

• fleet-state.test.sh: 45 → 73 cases, 0 failed, covering every behaviour change — catalog_versions and all four fail-open inputs, the fail-open byte-equality proof, update-candidates-user (equal/behind/ahead/project-scope/partial), symlink containment plus an in-checkout control, the production installLocation branch (which bypasses the fixture override), projectPathPresent including the false-is-not-null guard and the never-filters guarantee, user_scope_orphans, project_root (including that every --all block carries it), --marketplaces (enumeration, empty, standalone-flag rejection order-independence, and a CR regression under a CRLF-emitting jq stub), and selector help-text drift across all six selectors.
• check-changed-skills.sh — 2 skills, 0 errors. validate-plugin-contracts.mjs — 2813 files. check-shell-portability.sh, check-skill-portability.sh, check-evals-quality.sh, check-changelog-parity.sh (--check, --check-order, --check-bump, --check-preserved), shellcheck, typos, markdownlint-cli2, check-fixture-git-isolation.sh, claude plugin validate, catalog/cheatsheet/plugin-options sync — all pass.
• Live read-only smoke independently reproduced the audit's own numbers before any test was written: user_scope_orphans returned exactly the four ids the corrected packet names, and all 45 project-scope records came back projectPathPresent: false.

Related

• Closes fix(claude-ops): remediate the plugins:sync audit findings #3112 (workstream B of the audit remediation).
• fix(claude-ops): remediate the plugins:sync audit findings (0.36.0) #3145 — a concurrent duplicate implementation of the same issue, opened by an autonomous work-items lane ~35 minutes before this PR. This PR is a strict file-superset of it (its ten files plus skills/setup/SKILL.md) and covers all 15 audit findings rather than the issue's summary of 8. Two of its mechanisms were adopted with attribution in the commit message: the --marketplaces enumeration mode, which closes a real gap here, and the snake_case project_root spelling, which matches the existing top-level key convention where this branch had used camelCase. Two were deliberately not taken — its stale-user selector name reads as an authoritative stale list rather than a candidate superset, and its plugins/<name> layout fallback guesses a path the catalog did not declare, which can withhold an update on an unverified assumption. fix(claude-ops): remediate the plugins:sync audit findings (0.36.0) #3145 also lacks the traversal and symlink containment checks.
• fix(setup): unstamped --config claim prescribes a destructive reinstall across 18 setup skills #3111 / fix(plugins): correct the unstamped --config reconfiguration claim (#3111) #3115 — workstream A, merged as 1f7525fe6. This branch is rebased on it; the predicted two-file conflict was resolved by keeping 0.36.0 and stacking both CHANGELOG entries with ## [0.35.4] unmodified (--check-preserved confirms all 107 headings survive).
• fix(source-control): reap a torn-down worktree's project-scope plugin install records #3116 — workstream C, merged as af3fea480. C makes source-control drop project-scope records at worktree teardown, the producer-side counterpart to this PR's classify-and-report handling. This PR deliberately does not name or depend on source-control.
• Audit findings are sealed in a plugin-quality evidence packet; evidence-3.md re-scopes F5 and evidence-2.md corrects a tally, both applied here.

🤖 Generated with Claude Code

[claude]

Claude finished @kyle-sexton's task in 3m 2s —— View job

---

Security review in progress

• Gather context (PR diff, changed files)
• Determine security-relevant surface
• Hunt for vulnerabilities (logic flaws, injection, trust boundaries)
• Adversarial validation of candidate findings
• Post final review

[github-actions]

Last security-reviewed head: 17a7ddcd17f8e3083a68aa4405d82402a8a508d3. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5fc18ad77a

ℹ️ 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]

Claude finished @kyle-sexton's task in 5m 7s —— View job

---

Code review complete

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Check skip gate (draft/closed/trivial/already reviewed) — none apply
• Review changed files for correctness & maintainability
• Adversarially verify candidate findings
• Post inline comments for anchorable findings
• Post final summary

This repo carries a claude-security-review.yml workflow, so security findings are scoped to that lane and out of this review.

Scope reviewed: all 11 changed files — fleet-state.sh, fleet-state.test.sh, SKILL.md, converge.md, gotchas.md, scope-semantics.md, sync.md, evals.json, setup/SKILL.md, CHANGELOG.md, plugin.json.

Findings: 1 nit, posted inline.

• 🟡 skills/setup/SKILL.md#L75-79 and CHANGELOG.md#L86 — both restate "fourteen" declared options / "seven *_audit_enabled toggles" when plugin.json's userConfig block actually declares 15 options, 8 of them *_audit_enabled (hook_failure_audit_enabled is the one omitted from both counts and from the explicit enumeration). This claim predates this PR, but the PR rewrites both paragraphs with more confidence/detail while carrying the miscount forward — flagged since it's on lines this PR touches and the PR's whole premise is precision of stamped claims.

What held up well under review (no issues found):

• fleet-state.sh's new catalog_versions fail-open pre-filter, including the two-gate symlink-containment defense (manifest_is_contained + the lexical ../ pre-check) — traced the logic against hook::physical_path/hook::normalize_path and the string-concatenation path-join (no absolute-path-swallowing risk since it's plain shell concatenation, not an os.path.join-style function).
• The new projectRoot, user_scope_orphans, and projectPathPresent fields and their --ids selectors — traced field derivation against the 68-case test suite; jq programs, // vs == null-handling (esp. projectPathPresent's deliberate avoidance of // to preserve false), and CR-stripping wrapper usage all check out.
• Cross-references between sync.md, converge.md, scope-semantics.md, and gotchas.md for the F1/F2/F4 fixes (per-marketplace loop scoping, stale-project-record classification, pluginConfigs scope correction, /reload-plugins --force two-step) are internally consistent — no contradictions found between the spoke files or against SKILL.md's hub content.
• New test cases (catalog-version fail-open equality proof, symlink escape + control, production installLocation branch, selector help-text drift) exercise the claimed behavior correctly, including the documented Git-Bash ln -s deep-copy pitfall workaround.

Not independently verified (author-claimed only, noted per this lane's rules): the live-machine numbers in the PR description/CHANGELOG (66→0 update calls, the 9-marketplace resolution breakdown, the 2.1.240 CLI probes) and the test-run/lint-tool pass counts in the Test plan — I did not have a sandboxed way to execute fleet-state.test.sh or the other listed checks in this environment, so I read the script and tests statically rather than running them.

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @kyle-sexton's task in 3m 12s —— View job

---

Security review complete

• Gather context (PR diff, custom instructions to run /review:security-review)
• Check skip gate — PR is open; not skipped (head SHA changed since last review)
• Determine security-relevant surface — fleet-state.sh (shell logic, path/symlink handling) and sync.md/converge.md (agent-facing command templates)
• Hunt for vulnerabilities (logic flaws, injection, trust boundaries)
• Adversarial validation of candidate findings
• Post final review

Per this lane's scope (/review:security-review), this repo has a dedicated claude-security-review.yml workflow — findings are limited to security issues only (logic flaws, injection, trust-boundary gaps), not style/quality (that's /review:code-review, already run on this head).

Findings: none.

Reviewed the security-relevant surface introduced/changed in this PR at 94d8869:

• fleet-state.sh's new catalog_versions symlink-containment defense (manifest_is_contained, called at L603) — this is the highest-risk new code (resolves a path built from third-party marketplace.json source content). Traced both gates: the lexical ../ pre-check (with backslash folding) and the physical containment check that resolves symlinks via hook::physical_path/hook::normalize_path and requires a strict "$root"/* prefix match. Confirmed it fails closed on containment (any unresolved path → not contained) and the caller correctly treats that as fail-open on the update decision (null version → stays an update candidate) — the one safe direction, since withholding a real update is the only unsafe outcome this pre-filter can cause. The regression test at fleet-state.test.sh:1627-1664 exercises a real symlink escaping the checkout (with an honest platform-capability skip) plus an in-checkout control, and both assert the correct outcome.
• String-built paths and jq argument passing — cv_manifest, manifest_base, and all catalog/id values reach jq exclusively via --arg/--slurpfile/<<<, never string-interpolated into a jq program or a shell command; no injection surface there.
• Command templates in sync.md/converge.md — every <id> driving a claude plugin … call is quoted ("$id"), sourced only from fleet-state.sh's own CR-stripped, TAB-delimited output (never a hand-rolled jq | while read the docs explicitly warn against for the same CRLF-corruption reason). converge.md's -s project/-s local mutations are gated on projectPathPresent before ever emitting a cd "<projectPath>" command, and target each row's own recorded path rather than the bare/current-directory form.
• No GitHub Actions, permissions, secrets, or dependency-pin changes in this PR's file set — the diff is a shell script, its test suite, and skill markdown, so none of that surface applies here.

Consistent with the prior code-review pass on this head and the security scan at the prior head 5fc18ad, which reached the same conclusion on this same logic.

[claude]

Code review in progress

• Gather context (PR diff, CLAUDE.md, skill instructions)
• Check skip gate (draft/closed/trivial/already reviewed) — none apply; head advanced since the last review
• Review changed files for correctness & maintainability
• Adversarially verify candidate findings (3 independent verifier agents, each CONFIRMED)
• Post inline comments for anchorable findings
• Post final summary

View job run

[claude]

Claude finished @kyle-sexton's task in 3m 2s —— View job

---

Security review complete

• Gather context (PR diff via gh pr diff, custom instructions to run /review:security-review)
• Check skip gate — PR is open; head advanced to 17a7ddc since the last completed security pass (the most recent review attempt at a later SHA hit an infrastructure failure and never completed) — not skipped
• Determine security-relevant surface — fleet-state.sh (shell logic, path/symlink handling) and sync.md/converge.md (agent-facing command templates)
• Hunt for vulnerabilities (logic flaws, injection, trust boundaries)
• Adversarial validation of candidate findings
• Post final review

Per this lane's scope (/review:security-review), this repo has a dedicated claude-security-review.yml workflow — findings are limited to security issues only (logic flaws, injection, trust-boundary gaps), not style/quality (that's /review:code-review, already run separately on this head).

Findings: none.

Reviewed the full PR diff (origin/main → 17a7ddc) with focus on the security-relevant surface:

• fleet-state.sh's catalog_versions symlink-containment defense (manifest_is_contained, called at L606) — this remains the highest-risk new code, since it resolves a filesystem path built from third-party marketplace.json source content and uses the result to decide whether to withhold a plugin update. Confirmed:
  • The lexical ../ pre-check folds backslashes before matching, so both ../foo and ..\foo spellings are caught.
  • The physical gate resolves the candidate manifest path with symlinks followed (hook::physical_path) and requires a strict "$root"/* prefix match against the resolved, normalized checkout root — closing the gap a lexical-only check would miss (a symlink inside the checkout pointing outside it).
  • manifest_is_contained fails closed on containment (any unresolved path → not contained), and the caller correctly treats that as fail-open on the update decision (unresolved/null version → id stays an update candidate). The only unsafe direction for this pre-filter is wrongly withholding a real update, and both gates land on the safe side of that.
  • The regression suite (fleet-state.test.sh) exercises this with a real symlink escaping the checkout (created via MSYS=winsymlinks:nativestrict / gated on [[ -L ]] to avoid Git Bash's silent copy-instead-of-link behavior), a Windows-backslash traversal spelling, and an in-checkout control — all assert the correct outcome.
• jq argument passing — all values that flow from marketplace/catalog content into jq (cv_manifest, manifest_base, ids, etc.) go through --arg/--slurpfile/<<<, never interpolated into a jq program string or a shell command — no injection surface.
• Command templates in sync.md/converge.md — every id driving a claude plugin … invocation is quoted and sourced only from fleet-state.sh's own CR-stripped, tab-delimited --ids output (the docs explicitly warn against a hand-rolled jq | while read, which would corrupt ids with embedded \r on Windows). converge.md's new blocked-row handling for absent projectPath rows means a (cd "<projectPath>" && …) command is never emitted for a path that can't be cd'd into.
• No GitHub Actions, permissions, secrets, or dependency-pin changes in this PR's file set — the diff is a shell script, its test suite, and skill/changelog markdown, so that surface doesn't apply here.

This is consistent with the prior security passes on earlier heads of this PR, which reached the same conclusion on this same logic; the code in the security-relevant sections (manifest_is_contained and its call site) is unchanged in substance since then.

[claude]

Claude finished @kyle-sexton's task in 37s —— View job

---

Code review skipped

• Gather context (PR diff, CLAUDE.md, /review:code-review skill instructions)
• Check skip gate — PR is merged (closed, not open); skip gate condition 1 applies
• Post final summary

/review:code-review's skip gate says to stop early — without posting further review content — when "PR is closed or not open." PR #3148 was already merged into main (as 6e8c97f, matching this PR's title) before this run started, so there's no open PR state left to review against, and no further findings are being posted for this trigger.