Skip to content

refactor(disk-hygiene): share the unreferenced plugin-cache-version logic with claude-ops - #5632

Merged
kyle-sexton merged 7 commits into
mainfrom
refactor/5221-share-plugin-cache-versions
Sep 30, 2026
Merged

kyle-sexton merged 7 commits into
mainfrom
refactor/5221-share-plugin-cache-versions

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Closes #5221

Summary

Items 1, 2, 4, 5 and 7 of #5221 landed in #5585. This PR completes item 3 (reuse the unreferenced-cache-version logic from the claude-ops install-state audit) and item 6 (coordination with the related deep-inventory work).

disk-hygiene's deep inventory carried its own plugin-cache-version logic, a divergent copy of the claude-ops audit-install-state rule: it read installed_plugins.json without the guarded read the audit uses.

Fix

Verification

  • bash scripts/check-changelog-parity.sh --check: pass
  • bash scripts/check-changelog-parity.sh --check-order: pass (102 changelogs)
  • bash scripts/check-changelog-parity.sh --check-bump origin/main: pass
  • bash scripts/validate-plugins.sh: all manifests and the catalog validated
  • bash scripts/check-changed-skills.sh origin/main: 2 skills checked, 0 failed
  • bash scripts/check-cross-plugin-source-drift.sh: exit 0; lib/plugin_cache_versions.py IDENTICAL [registered]
  • python3 -m unittest on test_deep_inventory.py (52 tests) and test_install_state.py (91 tests): OK

Related

🤖 Generated with Claude Code

kyle-sexton and others added 3 commits September 30, 2026 18:08
…eader with claude-ops

Refs: #5221

Move registry parsing, installPath resolution and the .orphaned_at marker age
into one lib/plugin_cache_versions.py, carried byte-identical by claude-ops
and disk-hygiene and registered in the cross-plugin source registry. Each
caller keeps its own row shape and passes its own reader; disk-hygiene now
reads through a guard that refuses a symlinked marker, as claude-ops does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e-version module

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 30, 2026 22:14
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-30T22:18:04.428068Z f0eb4fe Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Claude code review complete

  • Gather context (PR diff, changed files)
  • Review shared plugin_cache_versions.py module
  • Review install_state.py integration
  • Review deep_inventory.py integration + tests
  • Review registry/changelog/version bump files
  • Post findings

No blocking findings. This is a clean extract of the unreferenced-plugin-cache-version logic into one shared module, consumed identically by both engines.

Notes from the review:

  • Confirmed the two copies are byte-identical (diff plugins/claude-ops/lib/plugin_cache_versions.py plugins/disk-hygiene/lib/plugin_cache_versions.py), matching the cross-plugin-source-registry.txt entry and the drift check's premise.
  • Registry / load_registry faithfully reproduces the prior per-file logic in both install_state.py and deep_inventory.py (registry-unreadable, no-plugins-object, and "no installPath under cache" doubt cases all map 1:1 to the old inline code), and orphan_marker's ISO formatting matches the pre-existing iso() helper in install_state.py exactly.
  • The refactor of deep_inventory.py's registry/marker reads through a new _read_guarded (deep_inventory.py#L349-L358) is a genuine improvement, not just a move: previously _orphan_marker read .orphaned_at markers with no symlink guard at all, while install_state.py already guarded these. Now both engines refuse a symlinked .orphaned_at, and the new test_a_symlinked_marker_is_not_followed test exercises exactly that case.
  • No remaining references to the removed local json import, _install_paths, _resolve, or _orphan_marker in deep_inventory.py; no name collisions between the imported resolve/install_paths and existing module-level names (e.g. install_state.py's unrelated resolve_root).
  • sys.path insertion (parents[3] / "lib") resolves correctly for both callers' directory depths, and both plugins already ship other shared lib/*.py modules loaded the same way, so this isn't a new packaging pattern.

Verification status: I could not execute the test suite or the pinned ruff wrapper in this session (python3 -m unittest ..., bash scripts/run-ruff.sh ..., and even python3 -c ... were all denied by the sandbox here) — the PR description's test/lint results (unittest, check-cross-plugin-source-drift.sh, check-changelog-parity.sh, etc.) are author-claimed and unverified by me. If this matters, the workflow's --allowedTools/permission config would need to allow those commands for an independent run. Everything above is from static reading of the diff.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review: no issues found

  • Gather context (PR diff, custom instructions)
  • Read changed files for security-relevant surface
  • Check for injection / unsafe file handling / credential exposure
  • Check instruction-surface deletions against exception register
  • Report findings

Scope reviewed: origin/main...HEAD (10 files, all under plugins/claude-ops/, plugins/disk-hygiene/, and scripts/). No CLAUDE.md, AGENTS.md, rules file, hook allow/deny list, or other standing-instruction surface is touched by this diff, so the instruction-exception-register lens does not apply here.

This PR is a pure refactor that extracts the "which plugin cache versions are unreferenced" logic into one byte-identical module (lib/plugin_cache_versions.py) shared by claude-ops's install_state.py and disk-hygiene's deep_inventory.py. It is read-only tooling over local filesystem state (installed_plugins.json and .orphaned_at markers under the plugin cache), pure standard library, with no shell/subprocess invocation, no network calls, no deserialization of untrusted formats (json.loads only), and no secrets handling.

Notably, the refactor improves the security posture rather than regressing it:

  • plugins/disk-hygiene/skills/clean/scripts/deep_inventory.py's old _orphan_marker() read .orphaned_at directly via (version / ".orphaned_at").read_text(), with no symlink guard — see the pre-image at bd07e43:.../deep_inventory.py#L358. The new shared path routes every read through _read_guarded() (f0eb4fe:.../deep_inventory.py#L349-L358), which now refuses to follow a symlink placed at a .orphaned_at marker path before reading it. The new regression test test_a_symlinked_marker_is_not_followed in test_deep_inventory.py#L418-L425 confirms this closes a real (if low-severity, local-attacker-only) TOCTOU/symlink-following gap.
  • claude-ops's install_state.py already had the stronger guard (read_text_guarded, including the never-read/secret-content checks) and the shared module now flows through that same guard via its caller-supplied read closure, so no secret-adjacent path gains a new read route.
  • Both copies are registered in scripts/cross-plugin-source-registry.txt and enforced identical by scripts/check-cross-plugin-source-drift.sh, so the two audits can no longer silently diverge on this logic (which was the actual bug motivating disk-hygiene and repo-hygiene: default to a deep, justified inventory instead of category skips #5221's item 3).

No CRITICAL, IMPORTANT, or SUGGESTION findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f0eb4fe5e1

ℹ️ 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".

Comment thread plugins/disk-hygiene/skills/clean/scripts/deep_inventory.py Outdated
kyle-sexton and others added 2 commits September 30, 2026 18:25
…sweep-window notes at the shared module

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


CI security review (/review:security-review)

  • Gather context (diff origin/main...HEAD at 8f3ea42, changed files, prior review comments)
  • Check skip gate — head changed since the last security review (new commits 7802885, 8f3ea42 landed after it), so re-reviewed fresh
  • Review shared module plugin_cache_versions.py (byte-identical in both plugins)
  • Review install_state.py and deep_inventory.py integration (the read closures / guards each caller supplies)
  • Check prior Codex findings against current head
  • Check for instruction-surface deletions (none apply — no CLAUDE.md/AGENTS.md/rules/skill-body guardrail is removed or narrowed by this diff)
  • Report findings

No security issues found.

This diff extracts the "which plugin cache version is unreferenced" logic into one byte-identical module (plugins/claude-ops/lib/plugin_cache_versions.py / plugins/disk-hygiene/lib/plugin_cache_versions.py), consumed by install_state.py and deep_inventory.py through caller-supplied read closures. It's pure read-only local-filesystem logic (json.loads only, no subprocess, no network, no secret handling), and each caller keeps its own read guard — the security-relevant behavior lives in the callers, not the shared module.

Both P2 findings from Codex's earlier review round on this PR are already fixed at the current head (commit 7802885):

  • Strict registry decoding. deep_inventory.py's _read_guarded now opens installed_plugins.json with errors="strict" (only the .orphaned_at marker read still uses errors="replace"), so invalid UTF-8 in an installPath raises instead of silently producing a modified path that could mark an installed version CANDIDATE. See _read_guarded, and the new regression test test_registry_with_invalid_utf8_leaves_every_version_unknown.
  • Sweep-window pointer. SKILL.md and scan-flags.md now point maintenance at lib/plugin_cache_versions.py (and note the byte-identical copy in the other plugin) instead of the old per-script constant location. See SKILL.md#L178-L182.

Other notes from re-reading the current head:

  • The pre-existing generic read_text_guarded in install_state.py still decodes with errors="replace" for every path, including the registry — unchanged by this PR and out of scope as a pre-existing condition, not something this diff introduces or touches.
  • sys.path.insert(0, str(_LIB_DIR)) in both consumers (install_state.py, deep_inventory.py) prepends the plugin's own lib/ dir, matching the existing pattern other shared lib/*.py modules in this repo already use; no new shadowing/injection surface.
  • resolve() / load_registry() in the shared module reproduce the prior per-file logic 1:1 (registry-unreadable, no-plugins-object, and "foreign registry" doubt cases), and orphan_marker()'s symlink/regular-file guard is enforced by each caller's own read closure before the module ever sees the bytes.
  • No standing instruction (CLAUDE.md, AGENTS.md, a rules file, a skill/agent body) is deleted, narrowed, or softened by this diff — the SKILL.md/scan-flags.md edits only repoint a maintenance pointer to the new shared-module location, they don't remove a guardrail.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review complete

  • Gather context (PR diff at 8f3ea42, changed files)
  • Review shared lib/plugin_cache_versions.py and its two byte-identical copies
  • Review install_state.py and deep_inventory.py integration
  • Review the new commit (7802885) that responds to Codex's two P2 findings
  • Review registry/changelog/version-bump files
  • Report findings

No blocking findings. The latest commit (7802885, "decode the plugin registry strictly and point the sweep-window notes at the shared module") resolves both outstanding Codex review comments on this PR, and both are backed by a new regression test:

  • deep_inventory.py's _read_guarded now decodes installed_plugins.json with errors="strict" (only the registry; .orphaned_at markers still use "replace"), so a corrupted installPath can no longer slip through as a CANDIDATE. Covered by test_registry_with_invalid_utf8_leaves_every_version_unknown in test_deep_inventory.py:375-381.
  • SKILL.md and scan-flags.md now point ORPHAN_SWEEP_DAYS maintenance at lib/plugin_cache_versions.py (the claude-ops copy also says to sync the disk-hygiene copy), matching where the constant actually lives now.

Also verified independently://

  • plugins/claude-ops/lib/plugin_cache_versions.py and plugins/disk-hygiene/lib/plugin_cache_versions.py are byte-identical (diff confirms), matching the cross-plugin-source-registry.txt entry.
  • No leftover references to the removed _install_paths / _resolve / _orphan_marker / local json import in deep_inventory.py; no name collisions between the newly-imported resolve and each caller's own module-level names.

One non-blocking observation, not a regression from this PR so not filed as a blocking finding: install_state.py's read_text_guarded (untouched by this diff) still decodes every read — including the registry — with errors="replace":

with path.open(encoding="utf-8", errors="replace") as fh:

That's pre-existing (unchanged before/after this PR), so the claude-ops engine doesn't get the same fail-closed protection against a corrupted installPath that deep_inventory.py now has. It's outside this PR's diff, but worth a follow-up given the PR's stated goal is "one rule for which cache versions are unreferenced" — right now the two engines still diverge on registry-decoding strictness, just via each caller's own guard rather than the shared module (which correctly leaves that choice to the caller).

Verification status: test execution and script execution (python3 -m unittest, bash scripts/check-cross-plugin-source-drift.sh --check, bash scripts/run-ruff.sh) were denied by the sandbox in this session — the PR description's test/lint results are author-claimed and unverified by me. Everything above is from static reading of the diff plus the diff-based byte-identity check, which the sandbox did allow.

kyle-sexton and others added 2 commits September 30, 2026 18:37
… heading

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


CI security review (/review:security-review)

  • Gather context (PR diff origin/main...HEAD, custom instructions)
  • Check skip gate — head SHA changed (9ed595b) since the last security review (8f3ea42), but git diff 8f3ea42...9ed595b on every file this PR touches is empty; the two intervening commits (6b99754, 9ed595b) only merge unrelated upstream main changes (source-control, disk-hygiene destructive-guard work) into the branch, none of which appear in this PR's changed-files list
  • Report result

Skip gate hit — no re-review needed. This PR's actual diff (origin/main...HEAD, 12 files, +327/-108) is byte-for-byte identical to what was already reviewed and cleared at 8f3ea42:

  • The CI security review at 8f3ea42 found no security issues, after Codex's two P2 findings (registry UTF-8 strictness, sweep-window doc pointers) were fixed in commit 7802885.
  • The prior full review (at f0eb4fe) likewise found no issues, and separately noted this refactor improves security posture: deep_inventory.py's new _read_guarded now refuses to follow a symlink at a .orphaned_at marker path, closing a gap the old _orphan_marker() had (regression test test_a_symlinked_marker_is_not_followed).
  • Re-verified independently at current HEAD 9ed595b: plugins/claude-ops/lib/plugin_cache_versions.py and plugins/disk-hygiene/lib/plugin_cache_versions.py are still byte-identical.
  • No CLAUDE.md, AGENTS.md, rules file, or skill/agent-body guardrail is deleted or narrowed by this diff — the SKILL.md / scan-flags.md edits only repoint a maintenance note to the new shared-module location.

No security issues found. Nothing new to review since the last pass; standing findings remain resolved.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review (/review:code-review)

  • Gather context (PR diff origin/main...HEAD at 9ed595b, prior reviews on this thread)
  • Check skip gate
  • Report

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

The actual PR diff (origin/main...HEAD) touches the same 12 files described in the PR body, and
is byte-for-byte unchanged since commit 8f3ea42 — I diffed each of this PR's files between
8f3ea42 and the current head 9ed595b and got no output. The commits added since 8f3ea42
(efb9f4b, 6b99754, 9ed595b) are all merges of origin/main plus unrelated disk-hygiene
changelog/engine-gate fixes that landed on main from other PRs; none of them touch this PR's
files.

The code-review lane already ran against 8f3ea42 in this thread ("Code review complete",
2026-09-30T22:26:56Z) after the author's fix-up commit 7802885 addressed both of Codex's P2
findings (strict UTF-8 decoding in deep_inventory.py's _read_guarded, and the sweep-window
maintenance pointers in SKILL.md/scan-flags.md). That review found no blocking issues and one
non-blocking, pre-existing observation (install_state.py's read_text_guarded still decodes
with errors="replace", unchanged by this PR —

with path.open(encoding="utf-8", errors="replace") as fh:
).

Since the reviewable content hasn't changed, there's nothing new to add. No inline comments filed.

@kyle-sexton
kyle-sexton merged commit db1c68b into main Sep 30, 2026
20 checks passed
@kyle-sexton
kyle-sexton deleted the refactor/5221-share-plugin-cache-versions branch September 30, 2026 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant