Skip to content

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

Closed
kyle-sexton wants to merge 4 commits into
mainfrom
claude/work-items-integration-72n0ja
Closed

fix(claude-ops): remediate the plugins:sync audit findings (0.36.0)#3145
kyle-sexton wants to merge 4 commits into
mainfrom
claude/work-items-integration-72n0ja

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Closes #3112

Summary

Remediates the 8 actionable findings from the plugin-quality audit of claude-ops:plugins (sync action) recorded in #3112: the Step 2 silent no-op (F1), converge/enable commands routed into deleted directories (F2), sync all covering one marketplace (F4), the unreported mid-run self-update (F6), the misstated pluginConfigs read scopes (F7), the non-durable version capture (F12), the 61-call no-op update sweep (catalog-version pre-filter), and the versionsMatch filter rule hand-maintained in four files (recurring-concerns §4). Bumps claude-ops 0.35.3 → 0.36.0.

The remaining audit findings (3 low-medium, 5 low) are sealed in the off-repo evidence packet (audit-notes.md, memory-tier on the audited machine) and are not enumerated in the issue body, so they are deliberately not addressed here.

Fix

Two commits, script mechanics first, then the doc surfaces that consume them:

  • 294ef439fleet-state.sh gains four capabilities (each covered in fleet-state.test.sh): a top-level project_root field (string | null) on every successful marketplace block, disambiguating "no project context" from "project context, zero in-repo installs" (F1's mechanism); projectPathExists (true/false/null) on installed[] records and divergences[].scopes[] entries via a directory-existence probe (F2's mechanism); a standalone --marketplaces flag emitting CR-free marketplace names for per-marketplace iteration (F4's mechanism); and a fail-open --ids stale-user selector comparing user-scope installed versions against the local marketplace checkout's per-plugin manifests — an id is omitted only when its catalog version was read successfully and matches (the pre-filter's mechanism).
  • c8ceeb0c — the doc surfaces consume them: sync.md Step 2 branches on project_root and SKILL.md's report gains an always-present In-repo: row (F1); every (cd "<projectPath>" && …) site gates on projectPathExists, with orphaned install records as a named report-only category and all-orphaned divergences marked non-convergeable (F2); sync all iterates Steps 2–5 per marketplace via --marketplaces (F4); a conditional Note: row states a self-updated run executed the pre-update algorithm (F6); the pluginConfigs sentence now names user/--settings/managed as the read scopes since CC v2.1.207, contrasted with enabledPlugins (F7); version capture mandates a mktemp ledger appended per mutating call (F12); Step 3 sweeps --ids stale-user with the fail-open contract documented (48/61 exact prediction on the audited CC 2.1.240 run); and the versionsMatch filter rule is normative only in scope-semantics.md, with SKILL.md/converge.md/gotchas.md pointing at it.

All new or changed harness claims carry CLI version stamps attributed to the audited CC 2.1.240 run, per the issue's constraints; no call site hand-writes a jq id extraction.

Verification

  • fleet-state.test.sh: 45 → 56 cases, 0 failed (run main-side after each phase). New cases cover the projectPathExists tri-state, project_root string/null/--all shapes, --marketplaces enumeration + CRLF regression + combination rejections, and every stale-user fail-open edge (missing installLocation, missing/invalid manifest, non-string version, object source, current-id omission, project-scope exclusion). No pre-existing case modified.
  • shellcheck and shfmt -d: clean on both scripts. markdownlint-cli2: 0 issues across the six changed markdown files. typos, editorconfig-checker: clean on all changed files.
  • Live smoke in this container: --marketplaces emits melodic-software (exit 0) and rejects --all (exit 2); the block carries project_root and projectPathExists; --ids stale-user emits 0 ids on this fresh, all-current install.
  • Two independent fresh-context verifiers checked each phase's acceptance criteria against the diffs: phase 1 PASS 8/8, phase 2 PASS 11/11 (including doc-claims-vs-script spot checks).

Related

  • plugins/plugin-quality/skills/audit/references/recurring-concerns.md §4 (SSOT/drift) — the concern that drove the versionsMatch consolidation.
  • Upstream limitation left in place, now documented rather than silently hit: no claude CLI verb reaps an install record whose projectPath no longer exists (observed on CC 2.1.240; prune -s project shares the no-path-flag limitation).

Generated by Claude Code

claude added 2 commits August 23, 2026 03:04
Extend the plugins skill's read-only fleet inspection script so the sync
doc surfaces can consume real state instead of collapsed signals:

- project_root (string|null) on every emitted marketplace block, incl.
  each per-marketplace block under --all: disambiguates "no project
  context at all" from "project context with zero in-repo installs",
  which --ids current-project alone projects identically to zero ids.
- projectPathExists (true/false/null) on installed[] records and
  divergences[].scopes[] entries: converge/sync must not emit
  (cd "<projectPath>" && claude plugin ...) against directories that no
  longer exist; no CLI verb reaps such records on the audited CC 2.1.240
  run, so they are report-only.
- --marketplaces: standalone CR-free per-line enumeration of
  known_marketplaces.json names so sync's all mode can iterate
  --marketplace <name> --ids <selector> without hand-writing the
  jq -r | while read that corrupts ids under native-Windows jq.
  Combining it with --marketplace/--all/--ids is a usage error (exit 2).
- --ids stale-user: user-scope installed ids NOT confirmed current with
  the local marketplace checkout's catalog version (manifest at
  <installLocation>/<source-path>/.claude-plugin/plugin.json), failing
  open on every unresolvable edge so a wrong emission costs one no-op
  update while a wrong omission can never skip a real one. On the
  audited CC 2.1.240 run this comparison predicted exactly the 48 of 61
  plugins that updated with 0 false positives/negatives, all catalog
  manifests resolving locally with zero network.

Test suite grows from 45 to 56 cases covering the projectPathExists
tri-state, project_root string/null/--all shapes, --marketplaces
enumeration + CRLF regression + combination rejections, and every
stale-user fail-open edge; no pre-existing case was modified.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RJjit7ErqbTpZ94KbVMUs
…dit fixes (0.36.0)

Phase 2 of the plugins-skill sync audit remediation: the doc surfaces now
consume the four fleet-state.sh capabilities Phase 1 shipped. sync.md Step 2
branches on project_root for the zero-record tri-state and SKILL.md's report
gains the always-present In-repo row; every (cd "<projectPath>" && ...) site
gates on projectPathExists with orphaned install records as a named
report-only category; sync all iterates Steps 2-5 per marketplace via
--marketplaces; a conditional self-update Note row states the run executed
the pre-update algorithm; the pluginConfigs read-scope claim is corrected
(user/--settings/managed only since CC v2.1.207, contrasted with
enabledPlugins); version capture mandates a mktemp ledger with per-call
appends; Step 3 consumes --ids stale-user (fail-open, 48/61 validated on the
audited CC 2.1.240 run); and the versionsMatch filter rule is normative only
in scope-semantics.md with every other site pointing at it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RJjit7ErqbTpZ94KbVMUs
@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 26s —— View job


Claude finished the security review

Task: Run /review:security-review against PR #3145 (HEAD c8ceeb0c).

  • Gather context (gh pr diff 3145, full diff read — origin/main was disjoint history in this checkout so git diff had no merge base; used gh pr diff per the skill's documented CI workaround)
  • Invoke /review:security-review skill and follow its procedure
  • Post findings / summary

Security review — no issues found

Reviewed all 9 changed files against the skip-gate criteria (logic flaws, authz/access-control gaps, injection surfaces, unsafe secret/token handling, dangerous Actions patterns). No .github/workflows files are touched by this PR.

Most of the diff is documentation (SKILL.md, converge.md, gotchas.md, scope-semantics.md, sync.md, CHANGELOG.md) — no executable surface there.

The one executable change worth scrutinizing is fleet-state.sh's new emit_stale_user_ids() (adds the --ids stale-user selector) and the projectPathExists directory-probe loop in emit_marketplace():

  • Both new jq invocations pass untrusted-ish values (rec_path, pname, id) via --arg, never string-interpolated into a jq filter — no jq/command injection.
  • manifest="$install_location/$rel/.claude-plugin/plugin.json" builds a path from a catalog source string, which is read from the already-installed marketplace's own marketplace.json. A marketplace catalog is a trust boundary the user crosses explicitly via claude plugin marketplace add, unchanged by this PR — and even a crafted source path here can only cause a .version field to be read for a same/no-match comparison that decides whether an already-known-installed id gets included in a claude plugin update <id> -s user candidate list. No write, no code execution, no path escape into a mutating action.
  • The [[ -d "$rec_path" ]] / [[ -f "$manifest" ]] bash tests use quoted variables throughout — no word-splitting/glob injection.
  • fleet-state.sh itself remains read-only/reporting-only; it does not execute claude plugin update/enable/marketplace update — those stay in the doc-driven sync procedure, unchanged by this PR's script.

The doc changes actually reduce risk versus the prior version: they gate every (cd "<projectPath>" && claude plugin …) construction in converge.md / sync.md / SKILL.md on the new projectPathExists field so an agent following the skill no longer builds commands against directories that may no longer exist.

No CRITICAL, IMPORTANT, or SUGGESTION-level security findings to report.

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown

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

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

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


CI Code Review

Todo

  • Gather context (skip-gate check, gh pr diff, read changed files)
  • Review fleet-state.sh script changes
  • Review fleet-state.test.sh coverage
  • Review doc surfaces (SKILL.md, converge.md, gotchas.md, scope-semantics.md, sync.md)
  • Adversarially verify candidate findings
  • Post inline findings and final summary

This is an unusually well-engineered doc/script pair — extensive inline rationale, deliberate fail-open semantics, and a real fixture-based test suite. I read the full diff plus the whole current fleet-state.sh, fleet-state.test.sh, and all five changed context docs, and traced the emit_stale_user_ids/projectPathExists/project_root/--marketplaces logic against their consumers. Two findings survived scrutiny; both are cases of the PR fixing one instance of script/doc drift while introducing a smaller instance of the same class:

  1. fleet-state.sh:60 — the new stale-user selector correctly claims (Step 3 update), but the pre-existing installed-user selector (line 49) still carries that same tag even though sync.md Step 3 and the gotchas.md example were both repointed to stale-user exclusively. The header table frames itself as authoritative for "which step consumes which selector," so two selectors now falsely claim Step 3.
  2. SKILL.md:148 — the new "orphaned install record" category (dead projectPath directory) collides in name with this same file's pre-existing "unknown/orphaned plugin id" (line 159, also used in evals.json) — a different concept (catalog-mismatched id). A single report can now contain both failure modes at once with no textual way to tell them apart.

Both are posted as inline comments with more detail. Nothing else rose to the "a careful senior reviewer would block" bar — the emit_stale_user_ids fail-open logic, the projectPathExists/project_root plumbing, the --marketplaces standalone-mode rejection, and the new test fixtures (including the fail-open edge-case matrix) all checked out against their stated contracts.

Not independently verified: I could not execute fleet-state.test.sh or shellcheck/shfmt myself — this environment's tool policy blocked running the test script directly (only specific gh subcommands were permitted). The PR body's test-count, shellcheck/shfmt/markdownlint/typos results, and the "48/61" audited-run numbers are author-claimed and unverified by me.

Branch: claude/work-items-integration-72n0ja

Comment thread plugins/claude-ops/skills/plugins/scripts/fleet-state.sh
Comment thread plugins/claude-ops/skills/plugins/SKILL.md
@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown

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

…ctor

Two review findings on the plugins skill:

- fleet-state.sh selector table: `installed-user` still claimed
  `(Step 3 update)` after sync.md Step 3 was retargeted exclusively to
  `stale-user`. Retag it as the full user-scope set whose
  not-confirmed-current subset (stale-user) Step 3 consumes. Comment-only.
- SKILL.md used "unknown/orphaned plugin id" for the catalog-mismatch
  concept, colliding with the new "orphaned install record" (dead
  projectPath) category. Drop "orphaned" from the old phrase and define
  it inline; "orphaned" now appears only in the dead-path sense. Update
  the three matching catalog-mismatch phrasings in evals.json to keep
  the graded expectations consistent.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RJjit7ErqbTpZ94KbVMUs
@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

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


Security review — no issues found

Re-reviewed following the synchronize trigger. The only change since the last security pass (head c8ceeb0c) is commit 848e519, which resolves the two prior non-security review comments (stale installed-user Step-3 tag; overloaded "orphaned" terminology):

  • fleet-state.sh — comment-only edit to the header selector table (re-tags installed-user's step attribution). No executable line touched.
  • SKILL.md and evals.json — prose-only rewording ("unknown/orphaned" → "unknown … present in enabledPlugins with no matching catalog entry") to remove a naming collision with the new "orphaned install record" category. No logic, no data flow.

Neither touches .github/workflows, executable script logic, jq invocations, path construction, or any trust boundary. This carries forward the conclusion from the earlier pass on c8ceeb0c: emit_stale_user_ids() and the projectPathExists probe both build jq filters via --arg (no injection), quote all bash path variables, and remain read-only/reporting — fleet-state.sh never itself invokes mutating claude plugin commands.

No CRITICAL, IMPORTANT, or SUGGESTION-level security findings to report.

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

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


CI Code Review

Todo

  • Gather context (gh pr diff 3145, read fix commit 848e519 in full)
  • Verify the two prior inline findings (installed-user step-tag drift, orphaned naming collision)
  • Review fleet-state.sh script changes for correctness
  • Review fleet-state.test.sh coverage for the new fixture/case
  • Review doc surfaces (SKILL.md, converge.md, gotchas.md, scope-semantics.md, sync.md)
  • Post summary

This lane already reviewed this PR twice at c8ceeb0c and posted two inline findings. This run is triggered by the follow-up commit 848e519 ("de-overload 'orphaned' and retag installed-user selector"), so I focused on verifying that commit resolves both findings correctly, then re-swept the full diff for anything new.

Both prior findings are correctly resolved:

  • fleet-state.sh:49-51installed-user's selector-table comment no longer claims (Step 3 update); it's now described as the full user-scope set whose not-confirmed-current subset (stale-user) Step 3 consumes. The selector itself is untouched/still valid — only the stale step-attribution comment changed, so nothing that referenced installed-user functionally broke.
  • SKILL.md:159-162 — "unknown/orphaned plugin id" is now "unknown plugin id (present in enabledPlugins with no matching catalog entry)", defined inline. orphaned now appears only in the dead-projectPath sense across SKILL.md/converge.md/sync.md. The three matching evals.json phrasings were updated in lockstep (grep -rn orphaned confirms no leftover collision anywhere in the skill).

No new issues found in the rest of the diff. Specifically checked and found sound:

  • emit_stale_user_ids's fail-open resolution chain (missing installLocation → missing/invalid manifest → non-string version → object source) against the new fixture matrix in fleet-state.test.sh (alpha/beta/curr/custom/ghost/broken/notstr/objsrc/projonly) — the asserted emitted/omitted set matches the code path for every edge, including the custom case that proves catalog source overrides the plugins/<name> default.
  • projectPathExists's path→bool map construction (fleet-state.sh:520-530) is computed once per unique path (not per record) and correctly folds back to null for user-scope records with no projectPath.
  • project_root's null-vs-string surfacing (fleet-state.sh:608-624) and its presence on every block under --all — the new tri-state test cases exercise both the resolved and unresolved paths correctly.
  • Doc consistency: sync.md Step 3 and the gotchas.md example both point at stale-user exclusively now; scope-semantics.md's "Divergence is not automatically actionable" section reads as the single normative statement, and SKILL.md/converge.md/gotchas.md all just reference it rather than restating it.

Not independently verified: as in the prior round, this environment's tool policy blocks running fleet-state.test.sh and shellcheck/shfmt directly (only specific gh subcommands are permitted) — I read the script and test diffs by hand rather than executing them. The PR body's test-count (56 cases, 0 failed), shellcheck/shfmt/markdownlint/typos results, and the "48/61" audited-run numbers remain author-claimed and unverified by me.

No CRITICAL/IMPORTANT findings to report on this pass.

Branch: claude/work-items-integration-72n0ja

…hangelog interleave)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RJjit7ErqbTpZ94KbVMUs
@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 6s —— View job


CI Code Review

Todo

Skip gate hit: this head already has a successful review from this lane that still applies.

This lane reviewed the PR twice already — at c8ceeb0c (2 findings posted) and again at 848e519 (both findings verified resolved, 0 new findings). The current HEAD 4b18b58 is a merge commit ("bring main into claude/work-items-integration-72n0ja") whose only diff versus 848e519 inside plugins/claude-ops/ is:

Both are pre-existing main content pulled in by the merge, not new authorship in this PR — confirmed via git diff 848e519..4b18b58 -- plugins/claude-ops/. None of the 10 files this PR actually changes (fleet-state.sh, fleet-state.test.sh, SKILL.md, converge.md, gotchas.md, scope-semantics.md, sync.md, evals.json, plugin.json, plugins/claude-ops/CHANGELOG.md's own 0.36.0 entry) differ from the already-reviewed 848e519 state.

Per this lane's skip gate, no further review is warranted — nothing new to post.

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

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


Security review — no issues found

  • Gather context (git diff origin/main...HEAD, confirmed diff scope matches PR's 10 changed files)
  • Invoke /review:security-review and follow its procedure
  • Post findings / summary

Re-reviewed following the synchronize trigger to HEAD 4b18b58. Per the skill's skip gate (criterion 3: "this head already has a successful security review that still applies"): the only change since the last security-reviewed head (848e519) is the merge commit 4b18b58 ("bring main into claude/work-items-integration-72n0ja"), which merges in unrelated upstream main commits (fleet-wide README/CHANGELOG regeneration from #3115, #3111). None of that lands inside this PR's actual diff — git diff origin/main...HEAD (three-dot, against the merge-base) is byte-identical in scope to the prior review: the same 10 files (fleet-state.sh, fleet-state.test.sh, SKILL.md, converge.md, gotchas.md, scope-semantics.md, sync.md, evals.json, CHANGELOG.md, plugin.json), and a direct diff of plugins/claude-ops/skills/ and plugins/claude-ops/.claude-plugin/ between 848e519 and 4b18b58 is empty.

This carries forward the conclusion from the prior passes: emit_stale_user_ids() and the projectPathExists probe in fleet-state.sh both build jq filters via --arg (no injection), quote all bash path variables, and the script remains read-only/reporting — it never itself invokes mutating claude plugin commands. No .github/workflows files are touched by this PR.

No CRITICAL, IMPORTANT, or SUGGESTION-level security findings to report.

kyle-sexton added a commit that referenced this pull request Aug 23, 2026
Closes the post-use audit of `claude-ops:plugins`' `sync` action. Every
harness-behaviour claim added or changed here was verified on Claude Code
2.1.240.

Three silent-failure paths closed:

- Step 2 no-opped invisibly. `fleet-state.sh` computes `currentProject` as a
  correct tri-state, but `null` covers "user-scope record", "no projectPath",
  and "no project context resolved at all" alike, so a run from $HOME and a run
  inside a repo with no in-repo installs produced an identical report. A new
  top-level `project_root` field carries the distinction and the report gains a
  fixed `In-repo:` row that must state a skip.
- `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.
- Actionable divergences 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 as runnable commands.

Adds a catalog-version pre-filter for the Step 3 sweep. Each plugin's version
lives in the marketplace checkout even though the catalog entry carries none, so
`--ids update-candidates-user` can withhold ids it proved are already current --
0 update calls instead of 66 on an already-current fleet. It fails open by
construction: an unreadable catalog version yields a candidate, and the test
suite proves the degradation as a byte-equality against `installed-user`.

The manifest it reads must sit inside the marketplace checkout, enforced
physically rather than lexically: `source` is third-party content, withholding
an update is the only unsafe direction, and a symlink inside the checkout is
reached by an ordinary `./name` source no string check can see.

Also corrects the `pluginConfigs` scope claim (user/--settings/managed only;
project and local ignored since v2.1.207, while `enabledPlugins` still honors
them), restates the `/reload-plugins --force` trigger as prompt-cache
invalidation with the docs' two-step recovery, splits the divergence count into
run-caused vs pre-existing, and reduces the `versionsMatch` rule to one origin.

`setup`: the headless `--config` route no longer prescribes an
uninstall/reinstall cycle, and separates the write from the running session's
behavior (a same-session `check` still reports the old value). Wording matches
the fleet-wide correction landed in #3115 verbatim apart from this plugin's own
option list, closing a live contradiction on main where the regenerated README
carried the corrected guidance while this skill still prescribed the
destructive cycle.

fleet-state.test.sh: 45 -> 73 cases, covering each behaviour change.

Two mechanisms adopted from the concurrent duplicate implementation in #3145,
which an autonomous work-items lane opened against the same issue: the
`--marketplaces` enumeration mode, which closes a real gap here (this branch
told the model to loop per marketplace without giving it a CR-safe way to list
them), and the snake_case `project_root` spelling, which matches the existing
top-level key convention where this branch had used camelCase. Its `stale-user`
selector name and `plugins/<name>` layout fallback were deliberately not taken:
the former reads as an authoritative stale list rather than a candidate
superset, and the latter guesses a layout the catalog did not declare, which can
withhold on an unverified assumption.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

No description provided.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Closing as a duplicate of #3148 (same title/scope: "remediate the plugins:sync audit findings (0.36.0)").

Evidence:

Recommend continuing work on #3148 (currently has unresolved bot-review threads + a behind-main blocker being worked) and abandoning this one. Reopen if #3148 turns out to be missing something #3145 covered — diff the two file lists above.

kyle-sexton added a commit that referenced this pull request Aug 23, 2026
Closes the post-use audit of `claude-ops:plugins`' `sync` action. Every
harness-behaviour claim added or changed here was verified on Claude Code
2.1.240.

Three silent-failure paths closed:

- Step 2 no-opped invisibly. `fleet-state.sh` computes `currentProject` as a
  correct tri-state, but `null` covers "user-scope record", "no projectPath",
  and "no project context resolved at all" alike, so a run from $HOME and a run
  inside a repo with no in-repo installs produced an identical report. A new
  top-level `project_root` field carries the distinction and the report gains a
  fixed `In-repo:` row that must state a skip.
- `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.
- Actionable divergences 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 as runnable commands.

Adds a catalog-version pre-filter for the Step 3 sweep. Each plugin's version
lives in the marketplace checkout even though the catalog entry carries none, so
`--ids update-candidates-user` can withhold ids it proved are already current --
0 update calls instead of 66 on an already-current fleet. It fails open by
construction: an unreadable catalog version yields a candidate, and the test
suite proves the degradation as a byte-equality against `installed-user`.

The manifest it reads must sit inside the marketplace checkout, enforced
physically rather than lexically: `source` is third-party content, withholding
an update is the only unsafe direction, and a symlink inside the checkout is
reached by an ordinary `./name` source no string check can see.

Also corrects the `pluginConfigs` scope claim (user/--settings/managed only;
project and local ignored since v2.1.207, while `enabledPlugins` still honors
them), restates the `/reload-plugins --force` trigger as prompt-cache
invalidation with the docs' two-step recovery, splits the divergence count into
run-caused vs pre-existing, and reduces the `versionsMatch` rule to one origin.

`setup`: the headless `--config` route no longer prescribes an
uninstall/reinstall cycle, and separates the write from the running session's
behavior (a same-session `check` still reports the old value). Wording matches
the fleet-wide correction landed in #3115 verbatim apart from this plugin's own
option list, closing a live contradiction on main where the regenerated README
carried the corrected guidance while this skill still prescribed the
destructive cycle.

fleet-state.test.sh: 45 -> 73 cases, covering each behaviour change.

Two mechanisms adopted from the concurrent duplicate implementation in #3145,
which an autonomous work-items lane opened against the same issue: the
`--marketplaces` enumeration mode, which closes a real gap here (this branch
told the model to loop per marketplace without giving it a CR-safe way to list
them), and the snake_case `project_root` spelling, which matches the existing
top-level key convention where this branch had used camelCase. Its `stale-user`
selector name and `plugins/<name>` layout fallback were deliberately not taken:
the former reads as an authoritative stale list rather than a candidate
superset, and the latter guesses a layout the catalog did not declare, which can
withhold on an unverified assumption.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #3148, after a file-by-file comparison rather than a size judgement.

This PR is a real implementation, not a duplicate to be discarded lightly. It covers the same core findings, bumps to the same 0.36.0, and takes fleet-state.test.sh from 45 to 56 cases. Two of its decisions were better than #3148's and have been taken into #3148 with attribution:

  • --marketplaces — a genuine gap in fix(claude-ops): remediate the plugins:sync audit findings (0.36.0) #3148. That branch was told to loop Steps 2–5 per marketplace without being given a CR-safe way to enumerate marketplace names, which would have forced exactly the hand-written jq | while read that the skill's own context/gotchas.md forbids on Windows. This PR's standalone flag, its rejection of invalid flag combinations, and its CRLF regression test were all adopted.
  • project_root — snake_case is correct here. Top-level keys in this output are snake_case (missing_from_*), and fix(claude-ops): remediate the plugins:sync audit findings (0.36.0) #3148's projectRoot was inconsistent with both the convention and its own sibling keys.

Why #3148 lands instead:

  1. Scope. This PR implements the 8 findings summarised in fix(claude-ops): remediate the plugins:sync audit findings #3112's issue body. fix(claude-ops): remediate the plugins:sync audit findings (0.36.0) #3148 implements all 15 from the audit's sealed evidence packet — the issue was a public summary, not the specification.
  2. It carries plugins/claude-ops/skills/setup/SKILL.md, which this PR does not touch. That file is now load-bearing rather than cosmetic: since fix(plugins): correct the unstamped --config reconfiguration claim (#3111) #3115 merged, plugins/claude-ops/README.md publishes the corrected reconfiguration guidance while the setup skill beside it still asserts the old false claim. A codex reviewer flagged that contradiction as P1 on fix(plugins): correct the unstamped --config reconfiguration claim (#3111) #3115, where it was resolved with the ordering constraint recorded and the window declared bounded on the explicit expectation that this work would close it. fix(claude-ops): remediate the plugins:sync audit findings (0.36.0) #3148 closes it; this PR would leave it open.
  3. Containment. fix(claude-ops): remediate the plugins:sync audit findings (0.36.0) #3148's pre-filter rejects a ../-traversing source and a symlink inside the checkout. Without those, a foreign manifest can falsely prove an id current and cause a real update to be withheld — the pre-filter's only unsafe direction, since the safe failure mode is an unnecessary update call.

Two of this PR's choices were deliberately not taken, for the record: the stale-user selector name, which reads as an authoritative stale list when it is a candidate superset; and the plugins/<name> layout fallback, which guesses a path the catalog never declares and can therefore withhold an update on an unverified assumption.

On why this happened at all — filing #3112 as a tracking issue to satisfy the pr-issue-linkage gate put it into this lane's queue while a separately-orchestrated workstream was already implementing it from the audit packet. That is an orchestration error on the filing side, not a defect in this lane's work, and the duplicated effort was real. Recorded so the next campaign opens the PR first and files the issue against it.

Thanks to whatever ran this — the --marketplaces gap would have shipped without it.

kyle-sexton added a commit that referenced this pull request Aug 23, 2026
…3148)

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 #3112 (workstream B of the audit remediation).
- **#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. #3145 also lacks the traversal and symlink
containment checks.
- #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).
- #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](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kyle-sexton
kyle-sexton deleted the claude/work-items-integration-72n0ja branch August 24, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants