fix(claude-ops): correct inventory field bleed, manifest paths, and scope coverage

[kyle-sexton]

Summary

Follow-up to #2313, which merged before these fixes landed — auto-merge fired when its checks went green and squashed only the first two commits, so main currently carries the defects below.

Six findings, all from the automated review on #2313 and all real:

Correctness

• Bundled-skill fields could bleed from the next registration. The extractor read each registration through a fixed 4000-character window — the exact failure mode build_brace_map exists to prevent for commands, and the one reference/extraction.md names as the thing not to do. A registration omitting a description adopted the following one's. Fields are now bound to their own literal via the brace map; an unmatched brace is counted and surfaced rather than silently skipped.
• Manifest-declared component paths were ignored. PLUGIN_COMPONENTS carried a manifest key per component and a comment claiming the manifest is read before the tree; nothing read it. A declared path replaces the default directory, so scanning defaults regardless reported components a plugin does not ship. Dotted keys resolve the experimental block.
• --self-check lost its diagnostic when no binary was found. pick_binary stores its explanation under reason; the self-check path read only error and printed a generic message.
• An unreadable CLI version passed silently. It is itself a drift signal, so it now degrades the verdict rather than skipping the comparison.
• Degraded and argparse's usage error both exited 2. A CI gate treating 2 as "degraded, warn" would silently swallow a mistyped flag. Degraded is now 3, leaving 2 to argparse: 0 ok, 1 broken, 2 usage error, 3 degraded.

Coverage

• Installed plugins and project scope were never read. Only marketplace catalogs were scanned, so a plugin installed from a marketplace that is no longer cached was invisible. disk.installed_plugins now walks the plugin cache, and catalog / installed / enabled are reported as three distinct sets. A project's .claude tree contributes skills, agents, and wired hook events that no machine-scope scan sees; --project-dir defaults to cwd. Wired hook events are reported, never hook scripts on disk — listing an unreferenced script as a hook would repeat the present-versus-active error the skill exists to avoid.

Test plan

• test_inventory.py — 31 hermetic tests (was 25), adding field-binding non-bleed, manifest path resolution incl. dotted and array forms, and the version-unknown advisory
• One existing fixture corrected: it asserted ok against a build the new version check correctly calls degraded
• Exit codes verified individually against the live build: 0 ok, 1 broken (missing binary, and a non-binary file), 2 usage error, 3 degraded
• Negative paths: nonexistent --binary (reason preserved in output), mutually exclusive flags, empty config dir, non-binary file
• Live run on 2.1.228: 104 built-in commands, 33 bundled skills, installed_plugins 67 across 4 marketplaces vs 121 catalog entries — the distinction this PR adds
• check-skill.sh PASS 0 errors · check-evals-quality.sh PASS · portability clean · validate-plugins.sh clean · cheat sheet in sync

Related

No linked issue. Direct follow-up to #2313 (merged), addressing that PR's own review comments, which arrived after auto-merge had already squashed and merged it.

• claude-ops 0.30.0 → 0.30.1.
• Still deliberately out of scope, unchanged from feat(claude-ops): add inventory skill enumerating the invocable surface #2313: a generic binary-exploration plugin.

🤖 Generated with Claude Code

[cursor]

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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ba60302902

ℹ️ 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 4m 20s —— View job

---

Security review

• Gather context (PR diff, changed files)
• Review inventory.py changes for security issues
• Review SKILL.md, CHANGELOG.md, plugin.json for security issues
• Review test_inventory.py for security issues
• Post findings

Reviewed the diff (git diff via gh pr diff 2350) against the five changed files. No GitHub Actions workflows are touched by this PR, so there's nothing in zizmor's lane (unpinned actions, dangerous triggers, permission widening, template injection) to comment on. SKILL.md, CHANGELOG.md, and plugin.json are docs/metadata only — no findings there. test_inventory.py is test-only.

One real finding in inventory.py, the file that gained the new manifest-path-resolution logic this PR is built around.

---

IMPORTANT — Manifest-declared component paths allow traversal outside the plugin directory (path traversal / arbitrary directory & file-existence disclosure)

Confidence: High (straightforward to trace, no mitigating check anywhere in the file)

_manifest_paths() returns the raw string(s) a plugin's plugin.json declares for a component (skills, commands, agents, workflows, outputStyles, experimental.themes, experimental.monitors, hooks, mcpServers, lspServers) with no sanitization — no rejection of absolute paths, no stripping of .. segments.

_scan_component() then joins that string directly onto the plugin's own directory:

• line 811: return [d for d in declared if (root / d).is_file()]
• line 817: targets = [root / d for d in declared]

Python's Path.__truediv__ discards the left operand entirely when the right side is an absolute path (Path("/plugin/root") / "/etc" → Path("/etc")), and it does not block embedded .. components either. So a plugin.json can declare, e.g., "commands": "/home/user/.ssh" or "agents": ["../../../../etc"], and the scanner will:

• list filenames inside that arbitrary directory into the JSON report (dir-kind components), or
• use .is_file() as a file-existence oracle for any absolute path on the machine, echoing the literal declared path into the report if it exists (file-kind components: mcp-servers → .mcp.json, lsp-servers → .lsp.json).

Notably this isn't limited to plugins the user has installed and enabled: scan_marketplace() calls scan_plugin() (which calls _scan_component()) on every catalog entry in a cached marketplace, not just installed ones. So merely having a marketplace added/refreshed is enough for a listed-but-never-installed plugin's manifest to drive this — no install/enable consent required.

For context, the code this replaced (_manifest_component_path, removed in this diff) did declared.strip().lstrip("./") before joining, which at least neutralized a bare leading / or ../ prefix. That partial mitigation is gone in the new _manifest_paths/_scan_component pair, and no equivalent containment check (e.g. resolve() + is_relative_to()) was added in its place — I confirmed no resolve(, relative_to(, or is_relative_to( call exists anywhere else in the file that would constrain these joins.

Impact is filename/path disclosure and a file-existence oracle (not file-content read or code execution), and the precondition (a malicious/compromised plugin manifest) is somewhat overlapping with the fact that an installed plugin can typically already run code via its own hooks/commands — but the marketplace-catalog path above means this is reachable even for plugins never installed, which is a meaningfully lower trust bar than "the user chose to run this plugin's code."

Suggested fix: reject absolute declared paths, and after joining, resolve both root and the candidate and verify containment before using it, e.g.:

root_resolved = root.resolve()
candidate = (root / d).resolve()
if not candidate.is_relative_to(root_resolved):
    continue  # or skip this declared entry

Fix this →

---

No other security-relevant issues found in this diff: no subprocess/shell=True/eval/exec was introduced, the exit-code and self-check reason-string changes are behavior-only, and the field-binding fix (brace-map-bound extraction replacing the fixed 4000-char window) operates only on the locally-installed claude binary — not attacker-reachable input in this trust model.

[claude]

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

---

Claude's review

Reviewed the 5 changed files against origin/main (via gh pr diff, since this PR is already merged into main). This repo has a claude-security-review workflow, so per REVIEW.md this pass stays in the code-review lane's scope: correctness, error handling, test coverage, maintainability — no security findings reported.

The five stated fixes (field-bleed via brace map, --self-check reason/exit-code fixes, version-unknown advisory) check out on inline reading — well-scoped, well-tested (TestBundledSkillFieldBinding, TestManifestComponentPaths, TestSelfCheckDiagnostic), and the exit-code separation (0/1/2/3) is correctly threaded through main(). Posted 3 inline findings on real gaps in the less-tested parts of the diff:

1. inventory.py:922 — the new project-scope block reuses scan_config_scope instead of the deleted, project-aware scan_project_scope, and in doing so silently drops settings.local.json-sourced hooks/enabledPlugins, and reads .mcp.json from <project>/.claude/.mcp.json instead of the correct <project>/.mcp.json. This is the most significant finding — it's a coverage regression in exactly the feature this PR adds.
2. inventory.py:806 — _manifest_paths returning [] for an explicit empty-array declaration is indistinguishable from "not declared" under _scan_component's truthiness checks, so an explicit empty declaration silently falls back to scanning the default directory (narrow edge case).
3. inventory.py:562 — the new unbounded_registrations counter isn't wired into check_integrity's advisory text (which still always says "used a computed name"), and has no dedicated test.

Test-coverage gap not anchorable to a single line: none of the rewritten disk-scanning functions — scan_disk, the new scan_installed (inventory.py:725, including its one-level version-directory nesting heuristic), scan_marketplace, scan_plugin, or the new project block in build_report — have any unit tests in test_inventory.py. That's the actual mechanism behind this PR's headline "Coverage" claims (installed vs. catalog vs. enabled, project scope), yet it's the one part with no hermetic regression protection — everything there was validated only by the described live run against a real ~/.claude, which isn't reproducible in CI. The manifest-path-resolution logic, by contrast, is thoroughly unit-tested (TestManifestComponentPaths).

Minor: the PR description says claude-ops 0.30.0 → 0.30.1, but the actual diff bumps plugin.json from 0.31.0 to 0.31.1. Doesn't affect the code, just a stale note worth a heads-up for anyone using the description as a changelog reference.

[claude]

Project-scope report silently drops settings.local.json and reads .mcp.json from the wrong path.

This reuses scan_config_scope (designed for the machine-scope config dir) for the new project-scope block, but it doesn't have the fidelity the function it replaces (scan_project_scope, deleted in this same diff) had:

1. settings.local.json is never read. The deleted scan_project_scope explicitly merged hooks/enabledPlugins from both settings.json and settings.local.json, labeling them project vs local. scan_config_scope only reads root / "settings.json" (line 850). A hook wired only in .claude/settings.local.json — a very common place for personal/gitignored overrides — is now invisible to the project-scope scan, even though it's genuinely active. That's exactly the "wired vs. present" gap this PR's own SKILL.md language (A hook script on disk is not a wired hook) warns against, just from the opposite direction: a wired hook now goes unreported.
2. enabledPlugins is dropped entirely for project scope. The old code reported per-scope enabled/disabled plugin lists for project/local; scan_config_scope has no enabledPlugins handling at all, so this information no longer appears anywhere in the project section.
3. .mcp.json is read from the wrong location. scan_config_scope's mcp = _load_json(root / ".mcp.json") (line 853) is called here with root = project_claude (i.e. <project>/.claude), so it looks for <project>/.claude/.mcp.json. The deleted code correctly read project_root / ".mcp.json" — Claude Code's project MCP config lives at the project root, not inside .claude/. As written, project-scope mcp_servers will essentially never be populated.

Given the PR's whole point is "no machine-scope scan sees project wiring" and "listing it wrong repeats the present-versus-active error," these are worth fixing rather than accepting as scope trims — nothing in the SKILL.md update says local-scope hooks or .mcp.json were deliberately dropped.

Fix this →

[claude]

An explicit empty-array manifest declaration is indistinguishable from "not declared," silently falling back to the default directory.

_manifest_paths returns [] (not None) when a manifest key resolves to an empty list (isinstance(node, list) branch two functions up). Both the kind == "file" branch (line 810: if declared:) and the directory branch (line 816: if declared:) treat [] the same as None because both are falsy in Python — so a plugin that explicitly declares e.g. "commands": [] (meaning "I ship zero commands, don't scan the default commands/ dir either") still gets the default directory scanned instead.

This contradicts the stated contract in _manifest_paths's own docstring: "A declared path replaces the default directory rather than adding to it." For a non-empty declaration that's true; for an explicit empty declaration it silently reverts to "adds to it" (falls back to defaults), which is the one case the docstring says can't happen.

Narrow edge case, but worth a is not None check instead of truthiness if the empty-list case matters in practice: if declared is not None:.

[claude]

unbounded_registrations is surfaced in the raw notes but never wired into check_integrity's human-readable advisory, and has no test covering the self-check path.

check_integrity (further down, around line 651-657) already has logic that fires when registrations_seen > resolved:

advisories.append(
    f"{seen - resolved} bundled-skill registration(s) used a computed name and "
    "were not resolved; ..."
)

That gap can now come from either cause — an unresolved computed name (pre-existing) or a brace-map desync counted here as unbounded (new) — but the advisory text unconditionally attributes it to "used a computed name," which is inaccurate when the real cause is unbounded_registrations. The PR description says an unmatched brace "is counted and surfaced rather than silently skipped," which is true for the raw JSON (bundled_skill_notes.unbounded_registrations), but the summarized --self-check advisory a CI gate would actually alert on still mislabels it.

Also: test_inventory.py has no test constructing a source with an actual unmatched/desynced brace to exercise unbounded_registrations end-to-end (the existing TestBundledSkillFieldBinding/TestManifestComponentPaths tests cover the other two fixes thoroughly, but this one is untested).

[claude]

Project-scope report silently drops settings.local.json and reads .mcp.json from the wrong path.

This reuses scan_config_scope (designed for the machine-scope config dir) for the new project-scope block, but it doesn't have the fidelity the function it replaces (scan_project_scope, deleted in this same diff) had:

1. settings.local.json is never read. The deleted scan_project_scope explicitly merged hooks/enabledPlugins from both settings.json and settings.local.json, labeling them project vs local. scan_config_scope only reads root / "settings.json" (line 850 below). A hook wired only in .claude/settings.local.json — a very common place for personal/gitignored overrides — is now invisible to the project-scope scan, even though it's genuinely active. That's exactly the "wired vs. present" gap this PR's own SKILL.md language (A hook script on disk is not a wired hook) warns against, just from the opposite direction: a wired hook now goes unreported.
2. enabledPlugins is dropped entirely for project scope. The old code reported per-scope enabled/disabled plugin lists for project/local; scan_config_scope has no enabledPlugins handling at all, so this information no longer appears anywhere in the project section.
3. .mcp.json is read from the wrong location. scan_config_scope's mcp = _load_json(root / ".mcp.json") (line 853) is called here with root = project_claude (i.e. <project>/.claude), so it looks for <project>/.claude/.mcp.json. The deleted code correctly read project_root / ".mcp.json" — Claude Code's project MCP config lives at the project root, not inside .claude/. As written, project-scope mcp_servers will essentially never be populated.

Given the PR's whole point is "no machine-scope scan sees project wiring" and "listing it wrong repeats the present-versus-active error," these are worth fixing rather than accepting as scope trims — nothing in the SKILL.md update says local-scope hooks or .mcp.json were deliberately dropped.

Fix this →

[github-actions]

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