Skip to content

fix(claude-config,claude-memory): gate audit-pass on report containment not the --report-to flag, refuse non-git targets, and audit user-global memory surfaces - #2234

Merged
kyle-sexton merged 4 commits into
mainfrom
fix/2228-audit-pass-report-containment
Aug 11, 2026
Merged

fix(claude-config,claude-memory): gate audit-pass on report containment not the --report-to flag, refuse non-git targets, and audit user-global memory surfaces#2234
kyle-sexton merged 4 commits into
mainfrom
fix/2228-audit-pass-report-containment

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Five findings across two plugins. Three are about audit-pass's report path, one is about the target
class it admits, one is about a surface claude-memory:audit could not reach. They are independent
see "what does not subsume what" below.

The false premise, and the machinery gated on the wrong thing (#2229, #2230)

reference/report-location-and-schema.md:13 said the report goes under ${CLAUDE_PLUGIN_DATA}, "which
resolves outside any target repository". False by construction: that directory resolves to
~/.claude/plugins/data/{id}/ (plugins reference,
verified 2026-08-11) and no documented setting relocates it. The sentence is true for an ordinary repo
under C:/Projects/…, and it was stated as a universal while a whole reachable target class — anything
at or above ~, a dotfiles repo being the obvious one — falsifies it.

The consequence was not cosmetic. Every obligation that exists for a report landing inside the target
was written under the --report-to bullet: name-based refusal, the pre-write exclusion entry, the
operator disclosure, the digest exclusion, assertion 2.4. The flag is one way the condition arises; it
was never the definition. So a run against a target at or above ~ wrote its report into its own scan
set with no exclusion entry and no disclosure, and then reported indeterminate on its own
determinism gate — every time, purely for doing what it was asked.

Both are re-gated on the predicate report_path ⊆ target_root, evaluated against the resolved path
on every run:

  • exclusion-set.md Class 4 is stated as that predicate rather than as a two-item list, so a member
    added later inherits the rule instead of needing someone to remember to extend a list.
  • report-location-and-schema.md §2, SKILL.md's argument list, SKILL.md's "never writes into its own
    scan set without --report-to" absolute, and determinism-tiers.md's digest exclusion all follow.
  • Assertions 2.1–2.4 re-scoped on containment. 2.1 ("git status --porcelain is empty") was
    falsified by the same class as 2.4, not just 2.4 — that is the folded F14 residual. 2.6 added as
    the default-path twin of the redirect disclosure, so a contained write is never silent.
  • README.md:184 carried the same false claim and is corrected (found by sweeping the plugin for the
    string, not by the ledger).

The target class the contract cannot describe (#2231)

SKILL.md:42 calls target "the git repository to audit"; the gate at :46 required only that it
"resolve to the active project root". A non-git directory satisfied the second and not the first, and
nothing refused it — after which the contract went quiet in five places: no no-git state-key fallback
(only a no-remote one), a scan baseline defined as HEAD plus the state digest, a Class 3 exclusion
derived from git worktree list with no fallback (Class 1 has one), assertion 2.1 stated over
git status --porcelain, and — the one that is a permanent capability loss rather than a missing
derivation — suppression enacted only by the team layer, which is the tracked layer. An operator could
accept a finding on such a target and have the acceptance silently fail to persist, forever.

The gate now enforces both halves. This closes a target class deliberately, and the issue records
that the operator settled the refuse-vs-specify fork toward refuse — the alternative obliges the contract
to promise a capability it cannot deliver there. The refusal reuses the existing channel shape
(non-zero, before Phase 0, naming the resolved path and the reason) rather than minting an exit code, and
it states the cost out loud, naming the suppression consequence, so it does not read as arbitrary.

Verified before writing: there is no existing non-git refusal in audit-pass to narrow — grep for
exit 4, non-git, not a git returns nothing. The exit-4 non-git diagnostic that exists in this repo
is source-control's scripts/worktree-create.sh:258-261, a different plugin and a different finding.
This is net-new machinery.

What does not subsume what

Refusing non-git targets does not retire the containment work, and the changelog says so explicitly.
The class that falsifies the containment claim is a git repository rooted at or above $HOME.
report_path ⊆ target_root holds there by construction, independent of gitignore, and that target shape
is entirely ordinary.

The eval suite was grading the defect (#2228)

This row had to land in the same change, not after it. Eval 1 expectation 3 graded "Writes the report
under CLAUDE_PLUGIN_DATA rather than into the scan set", and eval 20 graded the digest exclusion as
a --report-to property. Both encode the premise the other fixes remove, so the suite passed a
defect-exhibiting run and would have failed the corrected one.

The discriminator is not the report location — it is the pre-write exclusion record plus the
disclosure
, which the old contract performs on neither the default path nor at all. Rewritten so:

Case Now grades
1 bare-invocation-is-read-only that the containment predicate is evaluated, and that where it holds the path is recorded in this run's own exclusion set before the write and disclosed in output
20 dirty-file-edited-without-changing-the-count that the digest exclusion is keyed on containment, not on --report-to, so a contained default path is excluded too
30 non-git-target-is-refused-not-half-specified (new) the refusal: non-zero before Phase 0, naming path and reason, naming the suppression cost, and not inventing a non-git state key or Class 3 fallback to proceed

All 29 shipped cases were read at HEAD to discharge the packet's own "only eval 1 was audited" blindspot.
No others carry the premise.

The surface neither delegate audited (#2232, claude-memory)

claude-memory:audit Step 1 discovery was two find commands rooted at the cwd, so it saw project scope
only. Meanwhile claude-config's audit-instructions partitions memory-layer hygiene to this skill and
names ~/.claude/rules/ in the handoff explicitly
(audit-instructions/reference/criteria.md:96). One skill delegated a user-global surface by name; the
receiving skill's discovery could not reach it. ~/.claude/CLAUDE.md loads in every session of every
project and was audited by nothing — and under-coverage reads as a clean report.

Corrected rationale, recorded so it is not shipped wrong. The filed report argued from
claude-memory's reference/criteria.md:224, the C9 carve-out. Read in context, that line excludes
personal files from C9 as "not repo-scoped", which cuts against the argument. The seam above is the
load-bearing mechanism and needs no interpretation. (Two different files are named criteria.md here, in
two different plugins.)

Discovery becomes a script because the fix has a second half inline find cannot carry: every file is
tagged with the scope it loads from.
Widening without that would fire project-scoped C9 on
~/.claude/CLAUDE.md and FAIL it for not stating a repo's build and test commands — trading
under-coverage for a manufactured false positive. Step 2 now routes on the tag and carves C9 out for user
scope; R-checks apply at both scopes, because a user rule loads every session in every project; Step 3
gains a cross-scope conflict pass, since both layers load together and the report must say which scope
each side came from (only one is yours to edit on behalf of the repo). The config-root resolution reuses
what resolve-memory-dir.sh already carries rather than re-deriving it.

What review added

Eight threads across three rounds, all resolved, six follow-up commits. Four were defects in my own
work and are worth naming, because three of them were the same shape — a claim in a comment that the
code did not implement:

  1. The dotfiles case ate its own tail. When the project root is ~, .claude/rules and
    ~/.claude/rules are one directory, so every rule was emitted twice under two path spellings — a
    duplicate finding each, and a Step 3 comparison of a file against itself. Landing on the exact target
    shape this PR's other half calls "entirely ordinary". Fixed with canonical comparison and a third
    scope value, both, emitted once and satisfying either --scope filter.
  2. My comment claimed CLAUDE.md could not collide, and used that to justify not guarding it. It
    can: ~/.claude tracked as the repo makes the depth-1 CLAUDE.md and <config_root>/CLAUDE.md one
    file. Guarded, with a canon_file that resolves the containing directory and re-appends the basename
    rather than depending on readlink -f.
  3. Then the corrected comment over-claimed in the other direction. It said layout 2 collides the
    rules dirs too. It does not — project rules there resolve to <config_root>/.claude/rules, not
    <config_root>/rules — and my own fixture already agreed with the code rather than the comment. The
    truth is a clean asymmetry now stated as such: each layout collides exactly one surface, which is
    why the two comparisons are computed independently rather than from one flag. Pinned in both
    directions, including a layout-2 fixture carrying both a rules/ and a .claude/rules/.
  4. That asymmetry then broke the R1 rule I had just written. "A both-scoped rule against whichever
    CLAUDE.md shares its scope" pointed at nothing: both-scoped rules arise only in the ~-rooted
    layout, which is exactly where the two CLAUDE.md files stay distinct. Such a rule now compares
    against every CLAUDE.md in scope, attributing each finding to the scope of the one it overlapped.

Two more, both genuine over-claims in the prose:

  • "A user rule loads every session" is false for a paths:-scoped rule — it is absent until a
    matching file is read, the same distinction orphan-rule-check.sh already draws. Left unfixed it
    would have manufactured repo-relative currency, redundancy and contradiction findings for rules that
    cannot apply here. Step 2 and Step 3 now require co-residency to be established from the rule's own
    frontmatter.
  • The non-git refusal could not name what it refused in the case it exists for: on a bare invocation
    with no ${CLAUDE_PROJECT_DIR}, the documented default is git rev-parse --show-toplevel, which
    fails outside a repository, so there was no resolved root for the message. It falls back to the current
    directory for the diagnostic only — never as a target — and eval 30 grades that.

And R1 became ambiguous the moment this PR put two CLAUDE.md files in scope, which was a hole this
PR opened, so reference/criteria.md R1 is updated rather than only the workflow: R1 pairs within a
scope, and cross-scope overlap stays with Step 3, which owns it and names each side.

Trust surface

No new grant, no new hook, no new network read. One new local script, read-only, advisory (always exits
0), which reads ${CLAUDE_CONFIG_DIR:-$HOME/.claude} — a path this plugin's resolve-memory-dir.sh
already reads. Its test suite pins CLAUDE_CONFIG_DIR to a fixture in every case, so running the tests
never reads or reports on the real ~/.claude. The audit-pass changes narrow what the skill accepts
and add disclosure; nothing widens.

Test plan

Real fail-before / pass-after for F6. The new test's user-scope assertions fail against the shipped
inline discovery and pass against the script. Demonstrated against the same fixture:

$ bash old-discovery-repro.sh   # runs the SHIPPED context/audit.md:11-15 commands verbatim
Fixture user-scope surfaces that load in every session:
  /tmp/tmp.w8zd4xJhzc/conf/CLAUDE.md
  /tmp/tmp.w8zd4xJhzc/conf/rules/user-rule.md

=== OLD Step 1 discovery, verbatim from context/audit.md:11-15 at the parent commit ===
$ find . -maxdepth 1 -name "CLAUDE.md" -o -name "CLAUDE.local.md" 2>/dev/null
./CLAUDE.md
$ find .claude/rules -name "*.md" -type f 2>/dev/null
.claude/rules/project-rule.md

Neither user-scope surface appears above. That is the defect.

=== NEW discovery, same fixture ===
project	claude-md	CLAUDE.md
project	rule	.claude/rules/project-rule.md
user	claude-md	/tmp/tmp.w8zd4xJhzc/conf/CLAUDE.md
user	rule	/tmp/tmp.w8zd4xJhzc/conf/rules/user-rule.md

The suite — 44 checks, including the Git Bash drive-letter case and both dotfiles layouts:

$ bash plugins/claude-memory/skills/audit/scripts/discover-instruction-surfaces.test.sh
PASS: exits 0
PASS: user-scope CLAUDE.md is discovered
PASS: user-scope CLAUDE.md carries the user tag
PASS: user-scope rule is discovered
PASS: user-scope rule carries the user tag
PASS: project CLAUDE.md is tagged project
PASS: CLAUDE.local.md gets its own kind
PASS: project rule is tagged project
PASS: user CLAUDE.md is never tagged project
PASS: non-.md under rules is not discovered
PASS: subtree CLAUDE.md is out of scope (depth 1 by design)
PASS: --scope user emits user surfaces
PASS: --scope user suppresses project surfaces
PASS: --scope project emits project surfaces
PASS: --scope project suppresses user surfaces
PASS: exits 0 with nothing to report
PASS: emits nothing when no surface exists
PASS: exits 0 when the config root is absent
PASS: still discovers project surfaces without a config root
PASS: emits no user rows without a config root
PASS: discovers user CLAUDE.md under a Windows-form config root
PASS: tags it user under a Windows-form config root
PASS: overlapping rules dir emits each rule exactly once
PASS: the overlapping rule is tagged both
PASS: the overlapping rule is not also tagged project
PASS: the overlapping rule is not also tagged user
PASS: repo-root CLAUDE.md is still project
PASS: config-root CLAUDE.md is still user
PASS: repo rooted at the config root emits CLAUDE.md exactly once
PASS: the collided CLAUDE.md is tagged both
PASS: it is not also emitted as a user row
PASS: it is not also emitted as a project row
PASS: --scope user shows the both-tagged CLAUDE.md
PASS: distinct roots never produce a both-tagged CLAUDE.md
PASS: layout 1 collides rules
PASS: layout 1 leaves CLAUDE.md distinct
PASS: layout 2 collides CLAUDE.md
PASS: layout 2 leaves the rules dirs distinct
PASS: layout 2 emits the project-layer rule as project
PASS: layout 2 emits the user-layer rule as user
PASS: --scope user still shows the both-tagged rule
PASS: --scope project still shows the both-tagged rule
PASS: distinct rules dirs never produce a both tag
PASS: unknown argument still exits 0 (advisory contract)

44 checks run, 0 failed
RESULT: PASS — all 44 checks passed.

Static gates on the rest. The audit-pass half is contract prose plus evals.json and has no
executable test; these are the honest gates, run on the final tree:

$ jq empty plugins/claude-config/skills/audit-pass/evals/evals.json
$ jq empty plugins/claude-config/.claude-plugin/plugin.json plugins/claude-memory/.claude-plugin/plugin.json
(clean)

$ bash plugins/skill-quality/scripts/check-evals-quality.sh plugins/claude-config/skills/audit-pass/evals/evals.json
WARN: cases 1, 7 share an identical prompt and files — confirm the duplication is intentional (Q6)
check-evals-quality: PASS (1 warning(s) across 1 file(s))
   ^ pre-existing: the identical warning is produced by the unmodified file at origin/main (verified via git stash)

$ shellcheck plugins/claude-memory/skills/audit/scripts/discover-instruction-surfaces{,.test}.sh
(clean)

$ CHECK_SKILL_SKILLS_ROOT=.../claude-config/skills bash plugins/skill-quality/scripts/check-skill.sh audit-pass
INFO: all 7 base-ref trigger phrase(s) preserved
INFO: markdownlint clean
WARN: SKILL.md is 388 lines (soft target 200)
WARN: stale fresh-eyes-exempt directive (SKILL.md:340)
CHECK-SKILL audit-pass: PASS — 0 errors, 2 warning(s)

$ CHECK_SKILL_SKILLS_ROOT=.../claude-memory/skills bash plugins/skill-quality/scripts/check-skill.sh audit
INFO: all 5 base-ref trigger phrase(s) preserved
INFO: markdownlint clean
INFO: script test passed: scripts/discover-instruction-surfaces.test.sh
INFO: script test passed: scripts/memory-dir-stats.test.sh
INFO: script test passed: scripts/memory-index-refs-check.test.sh
INFO: script test passed: scripts/orphan-rule-check.test.sh
WARN: no Gotchas surface
CHECK-SKILL audit: PASS — 0 errors, 1 warning(s)

$ npx markdownlint-cli2 --config .markdownlint-cli2.jsonc <all 20 changed/adjacent md files>
Summary: 0 issues in 0 files

Both audit-pass warnings pre-date this change: the fresh-eyes-exempt directive at what is now :340
is unmodified from origin/main (it sat at :307 before this diff shifted it), and the line-count
warning was already firing at 353 lines.

Related

Closes #2228
Closes #2229
Closes #2230
Closes #2231
Closes #2232

Inbox item: 20260811-020411-claude-config-audit-pass-report-path-inside-scan-set.md (batch-4 ledger
I8, rows F16, F1, F2, F4, F6; row F14 folded into F2, no separate issue). F4 was held in
the batch decision queue as DQ-1 and released by the operator toward the refuse branch.

Two bumps, because two plugins changed: claude-config 0.30.0 → 0.31.0 (refusal and re-gated
machinery are behavior), claude-memory 0.7.1 → 0.8.0 (two surfaces newly in scope). Not shared.

I8 rows left filed-not-fixed: F3 (no scripts, MED), F5 (lease, MED/SELF_REPORTED), F7 (I6 owns
it
— same defect, do not file twice), F8/F9/F10/F13 (DOC_ONLY), F11 (I6-adjacent), F12,
G1 and S1 (routed to item 20260811-021645, other plugins), H1 (UPSTREAM). Issue filing for those
is held pending the operator's call on batch-4 volume (RECONCILE OR-4).

…a flag, refuse non-git targets, and let claude-memory see user-global surfaces

audit-pass said its report goes under ${CLAUDE_PLUGIN_DATA}, "which resolves
outside any target repository". That is false by construction: the directory
resolves to ~/.claude/plugins/data/{id}/ and no documented setting relocates it,
so it is inside any target at or above ~. True for a repo under C:/Projects/,
stated as a universal, and a dotfiles repo falsifies it.

Worse, the machinery that exists for exactly that situation was gated on the
--report-to flag rather than on the condition the flag happens to create. Name-
based refusal, the pre-write exclusion entry, the operator disclosure and the
digest exclusion were all written under that bullet, so a run against a target
at or above ~ wrote into its own scan set with no exclusion and no disclosure,
then reported indeterminate about itself every time. The governing condition is
now report_path ⊆ target_root, evaluated on every run, with --report-to demoted
to one instance. Class 4 is stated as that predicate rather than a two-item list
so a later member inherits the rule instead of needing someone to extend a list.
Assertions 2.1-2.4 are re-scoped on containment — 2.1 was falsified by the same
class as 2.4, not just 2.4 — and 2.6 adds the default-path twin of the redirect
disclosure.

Separately, SKILL.md called target "the git repository to audit" while the gate
required only "resolves to the active project root", so a non-git directory
passed into a contract with no branch for it: no no-git state-key fallback, a
scan baseline defined as HEAD plus digest, a Class 3 exclusion over git worktree
list with no fallback, an assertion stated over git status --porcelain, and —
the one that is a capability loss rather than a missing derivation — suppression
enacted only by the tracked layer, so no acceptance would ever persist. It is
refused, using the existing non-zero channel rather than a new exit code, and
the refusal states that cost. The operator settled this fork toward refuse;
closing the class is deliberate.

These are independent: refusing non-git targets does not retire the containment
work, because the class that falsifies the containment claim is a git repository
rooted at or above $HOME.

evals 1 and 20 graded the false premise as a pass condition, so a corrected run
would have failed the suite. Both rewritten onto the predicate; eval 30 grades
the refusal. All 29 shipped cases were read; no others carry the premise.

claude-memory's audit could not see the surfaces claude-config hands it. Step 1
discovery was two finds rooted at the cwd, while audit-instructions' surface
partition delegates ~/.claude/rules/ here by name — so ~/.claude/CLAUDE.md, which
loads in every session of every project, was audited by nothing. Discovery moves
into a script so it can do the half inline find cannot: tag every file with its
scope. Without that, widening would fire project-scoped C9 on ~/.claude/CLAUDE.md
and FAIL it for not stating a repo's build commands — under-coverage traded for a
false positive. Step 2 routes on the tag, R-checks apply at both scopes, and
Step 3 gains a cross-scope conflict pass since both layers load together.

Recorded because the filed report argued from the wrong line: claude-memory's
criteria.md:224 excludes personal files from C9 as not repo-scoped, which cuts
against the argument. The load-bearing seam is audit-instructions' criteria.md:96.

Closes #2228
Closes #2229
Closes #2230
Closes #2231
Closes #2232

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

cursor Bot commented Aug 11, 2026

Copy link
Copy Markdown

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.

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

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


Security Review — PR #2234

  • Gather context (PR diff via gh pr diff, all 15 changed files)
  • Review shell scripts (discover-instruction-surfaces.sh, .test.sh) for injection/path issues
  • Review markdown/skill contract changes (SKILL.md, report-location-and-schema.md, exclusion-set.md, determinism-tiers.md, context/audit.md) for authorization/logic gaps
  • Review plugin.json / evals.json / CHANGELOG.md / README.md changes for permission widening or leaked secrets
  • Post final findings

Findings

No security issues found.

Scope reviewed: all 15 files changed in this PR (2 new shell scripts, 5 audit-pass prose contract files, evals.json, 2 plugin.json version bumps, 2 CHANGELOG.md, 2 README.md, claude-memory's SKILL.md/context/audit.md).

Notes from the review (informational, not findings):

  • discover-instruction-surfaces.sh (new) is read-only, always exits 0, does no eval/command substitution on untrusted input, and only ever passes literal path strings to find/printf. CLAUDE_CONFIG_DIR is read the same way the existing resolve-memory-dir.sh already reads it — no new trust boundary is crossed. Its .test.sh companion pins CLAUDE_CONFIG_DIR to a mktemp -d fixture in every case (with an EXIT trap cleanup), so the suite never reads or discloses the real ~/.claude.
  • The audit-pass changes (containment predicate report_path ⊆ target_root, the non-git refusal, the new disclosure assertions 2.6/2.1/2.2/2.3/2.4) are all natural-language contract prose that an agent follows, not executable code — but the direction is uniformly a tightening: a class of runs that previously wrote a report into the target with no exclusion entry and no disclosure now gets both, and a target class that previously produced an undocumented, permanently-non-enactable suppression state (non-git target) is now refused outright before any work happens. No case was found where the new logic widens what gets written or removes an existing safeguard.
  • plugin.json diffs are version-number-only bumps; no permissions, hooks, or trust-surface fields changed.
  • No secrets/tokens/credentials introduced (checked via pattern scan across the full diff).
  • No GitHub Actions workflow files are touched by this PR, so the pull_request_target/workflow_run/permissions-widening/script-injection lane doesn't apply here.

@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: 643ec2272c

ℹ️ 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/claude-config/skills/audit-pass/SKILL.md Outdated
Comment thread plugins/claude-memory/skills/audit/context/audit.md Outdated
Comment thread plugins/claude-memory/skills/audit/scripts/discover-instruction-surfaces.sh Outdated
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

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


Review complete

  • Gather context (diff, CLAUDE.md, REVIEW.md)
  • Review claude-config changes (audit-pass containment gating, non-git refusal, evals)
  • Review claude-memory changes (discovery script, tests, audit.md)
  • Post findings as inline comments
  • Post summary

Summary

This PR is prose-and-spec-as-code (SKILL.md / reference docs / evals.json) plus one new bash script. I read all 15 changed files against origin/main, cross-checked the PR's factual claims against the current tree, and independently ran what I could execute.

Independently verified (not just author-claimed):

  • jq empty on evals/evals.json and both plugin.json files — valid JSON.
  • All 30 eval ids in evals.json are unique (count: 30, unique: 30, max: 30) — matches the PR's "29 shipped + eval 30" claim.
  • ~/.claude/rules/ is in fact named explicitly in plugins/claude-config/skills/audit-instructions/reference/criteria.md (surface-partition paragraph) — substantiates the PR's central premise that this surface was delegated but unreachable.
  • plugins/claude-memory/skills/audit/reference/criteria.md:224-225 does exclude personal files from C9 as "not repo-scoped" — confirms the PR's own correction of its originating rationale.
  • Cross-file consistency of the containment predicate (report_path ⊆ target_root) across SKILL.md, report-location-and-schema.md §2/assertions, exclusion-set.md Class 4, and determinism-tiers.md — all four now state the same rule and none is left keyed on --report-to.

Could not execute: the sandbox in this environment declined to run (even bash -n/shellcheck on) discover-instruction-surfaces.sh/.test.sh, so the 23-case suite and the fail-before/pass-after repro in the Test Plan are author-claimed and unverified by me — I reviewed the script and test file by reading only.

Finding posted inline (plugins/claude-memory/skills/audit/scripts/discover-instruction-surfaces.sh:86-108): when the audited project root is $HOME itself — a home-directory dotfiles repo, which this PR's own audit-pass fix calls "entirely ordinary" and reachable — project-scope rule discovery (.claude/rules relative to cwd) and user-scope rule discovery (${CLAUDE_CONFIG_DIR:-$HOME/.claude}/rules) resolve to the same directory, so every rule under it is emitted twice with different scope tags and different path strings for one physical file. context/audit.md:41 runs R1-R4 at both scopes, so this produces a duplicate finding per rule in exactly the target class the sibling fix treats as ordinary. The 23-case test suite always uses two distinct tmpdirs for project and config root, so this gap isn't covered by it.

Everything else held up under review:

  • The non-git refusal (SKILL.md:42-84) is genuinely net-new — I confirmed no prior exit 4/non-git/not a git refusal existed for audit-pass by inspecting the file, matching the PR's own stated verification method.
  • Assertions 2.1-2.4 and the new 2.6 in report-location-and-schema.md are correctly re-scoped on containment rather than on "no redirect"/--report-to.
  • evals.json cases 1, 20, and the new case 30 grade the corrected behavior (containment evaluation + pre-write exclusion + disclosure) rather than the old flag-gated premise.
  • The claude-memory scope-tagging rationale (why C9 must skip user-scope files) is accurate against the current criteria file, and the fix's second half (tagging, not just widening discovery) is the right shape for avoiding a manufactured false positive.

Branch: fix/2228-audit-pass-report-containment

Comment thread plugins/claude-memory/skills/audit/scripts/discover-instruction-surfaces.sh Outdated
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown

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

…respect path-scoped rules, and name the directory a non-git refusal refused

Three review findings, all real.

The dotfiles case eats its own tail. When the project root IS the home directory
— the same target shape this PR's audit-pass half calls ordinary — `.claude/rules`
relative to the cwd and ~/.claude/rules are the SAME directory, so discovery
emitted every rule twice under two path spellings: a duplicate finding per rule,
and a Step 3 comparison of a file against itself. The two roots are now compared
canonically and, where they coincide, each file is emitted once with scope
`both`. It satisfies either --scope filter, because the file really is reachable
by each layer. CLAUDE.md cannot collide this way — project discovery is depth-1
at the cwd while the user copy sits inside the config root — and a case pins that.

Path-scoped rules are not always-loaded. A user rule carrying `paths:` is absent
until a matching file is read, so "a user rule loads every session" over-claimed
and would have manufactured repo-relative currency, redundancy and contradiction
findings for rules that cannot apply here. Step 2 and the Step 3 comparison now
say to establish co-residency from the rule's own frontmatter first.

And the non-git refusal could not name what it refused in the case it exists for.
With no explicit target and no CLAUDE_PROJECT_DIR the documented default is
git rev-parse --show-toplevel, which fails outside a repository — so there was no
resolved root to put in the message. The diagnostic falls back to the current
directory. That fallback is for the message only and never becomes a target.

Suite grows 23 -> 32 checks.

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

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

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


Security Review — PR #2234 (re-run at d0eee42)

  • Gather context (gh pr diff against origin/main, all 15 changed files)
  • Review shell scripts (discover-instruction-surfaces.sh, .test.sh) for injection/path/trust issues
  • Review markdown/skill contract changes (SKILL.md, report-location-and-schema.md, exclusion-set.md, determinism-tiers.md, context/audit.md) for authorization/logic gaps
  • Review plugin.json / evals.json / CHANGELOG.md / README.md for permission widening or leaked secrets
  • Post final findings

Findings

No security issues found.

This is a re-review at the latest commit (d0eee42), which is the fix-up commit addressing the correctness feedback from the prior two review rounds (the dotfiles double-discovery dedup, the non-git refusal's diagnostic-path fallback, and path-scoped-rule handling). Those were correctness/coverage issues, not security ones, and are out of scope for this lane; I focused solely on injection, authz/trust-boundary, secret-handling, and GitHub Actions risk across all 15 changed files.

  • discover-instruction-surfaces.sh (full file at d0eee42) — still read-only, always exits 0, no eval/curl/wget/command substitution on untrusted content. All file paths from find are passed through printf '%s\t%s\t%s\n' as literal data, never interpreted. CLAUDE_CONFIG_DIR is read via the same pattern resolve-memory-dir.sh already uses — no new trust boundary. The new canon_dir() helper (cd "$1" && pwd -P, L58-63) only canonicalizes directories already reached by the pre-existing project/user resolution logic; it doesn't introduce a new file-read path.
  • SKILL.md's non-git refusal now falls back to the current directory for the diagnostic message when git rev-parse --show-toplevel produces nothing (addressing the earlier Codex P2 comment) — this is prose an agent follows, not code, and the fallback is explicitly scoped to the message only ("never becomes a target"), so it doesn't widen what gets scanned or written.
  • The containment re-gating (report_path ⊆ target_root across SKILL.md, report-location-and-schema.md, exclusion-set.md Class 4, determinism-tiers.md) remains a uniform tightening — a previously-silent write-into-target-with-no-disclosure case now gets both an exclusion entry and disclosure. No case widens what gets written or removes an existing safeguard.
  • plugin.json diffs in both plugins are version-number-only bumps — no permissions, hooks, or trust-surface fields changed.
  • No secrets/tokens/credentials in the diff.
  • No GitHub Actions workflow files touched — the pull_request_target/workflow_run/permissions-widening/script-injection lane doesn't apply.

Nothing new to flag beyond what the two prior review rounds already covered (both of which were correctness findings, since fixed in this commit).
· Branch: fix/2228-audit-pass-report-containment

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

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


Code Review — PR #2234

  • Gather context (diff, CLAUDE.md, REVIEW.md)
  • Review claude-config/audit-pass changes (containment gating, non-git refusal, evals)
  • Review claude-memory/audit changes (discovery script, tests, audit.md)
  • Post inline findings
  • Post summary

Scope note

Per REVIEW.md, this lane (correctness/design/conventions/tests) leaves security to the dedicated security lane, which has already run twice on this PR. Two prior review rounds (Codex, and this lane's earlier security pass) already flagged issues on commit 643ec2272c — the non-git-target cwd fallback, the path-scoped-rules assumption, and the dotfiles rules-directory double-discovery. I checked all three against the current HEAD (d0eee42): all three are fixed in the second commit, which is exactly what that commit's message ("dedupe the dotfiles scope overlap, respect path-scoped rules, and name the directory a non-git refusal refused") claims. I read the relevant diffs directly rather than trusting the message alone.

New findings (posted inline)

  1. discover-instruction-surfaces.sh:108-110 — the dotfiles-overlap fix canonicalizes and dedupes the two rules directories, but not the two CLAUDE.md paths. When the project root is the config root itself (e.g. a dotfiles repo tracked at ~/.claude rather than ~), CLAUDE.md (project) and $config_root/CLAUDE.md (user) are the same physical file and get emitted twice under different scope tags — the same class of bug this PR just fixed for rules, in an adjacent, narrower case the code comment incorrectly rules out. No test fixture covers project-root-equals-config-root either.
  2. context/audit.md:41 — R1 ("Duplication with CLAUDE.md") is now ambiguous once a rule can be user-scoped: its definition in reference/criteria.md (untouched by this PR) still assumes one CLAUDE.md in scope, and this bullet doesn't say whether a user-scope rule's R1 check compares against the user's own CLAUDE.md or the project's. If it's read as project's, it duplicates the new Step 3 item 4 cross-scope pass under a different check name.

Verified independently (ran myself, not just author-claimed)

  • plugins/claude-config/skills/audit-instructions/reference/criteria.md:96 does name ~/.claude/rules/ explicitly in the surface-partition list — substantiates the PR's central premise.
  • plugins/claude-memory/skills/audit/reference/criteria.md:224-225 (C9) does exclude ~/.claude/CLAUDE.md as "not repo-scoped" — confirms the PR's own correction of its originating rationale.
  • Confirmed via git show on commit 643ec22 that the "fall back to cwd for the diagnostic" text was genuinely absent at the point Codex reviewed it, and is present now — the fix is real, not just claimed.
  • reference/criteria.md (both plugins) is untouched by this PR's diff (gh pr diff --name-only) — the basis for finding fix: correct plugin-system inaccuracies and harden references #2 above.

Could not execute

Same sandbox restriction the earlier security-lane pass hit: any bash <script> invocation here requires approval I can't grant, so I could not run discover-instruction-surfaces.test.sh, shellcheck, or jq empty myself. I reviewed the script and its test file by reading only; the Test Plan's claimed pass counts and command outputs are author-claimed and unverified by me. (Minor, non-blocking aside: the Test Plan's pasted output claims "23 checks run, 0 failed," but the test file at HEAD has visibly more assertion call-sites than that — consistent with the dotfiles-overlap test cases the second commit added after that output was likely captured. Doesn't affect correctness of the shipped tests, just a stale paste.)

Everything else

The containment re-gating (report_path ⊆ target_root) is applied consistently across SKILL.md, report-location-and-schema.md, exclusion-set.md, and determinism-tiers.md — no file left keyed on --report-to. The non-git refusal is genuinely net-new (no prior exit 4/non-git handling existed for audit-pass). Evals 1, 20, and the new 30 grade the corrected behavior rather than the old premise. plugin.json bumps are version-only and match the stated semver reasoning (both minors, for behavior changes on two independent target classes).
· Branch: fix/2228-audit-pass-report-containment

Comment thread plugins/claude-memory/skills/audit/scripts/discover-instruction-surfaces.sh Outdated
Comment thread plugins/claude-memory/skills/audit/context/audit.md
…md R1 compares against

My own comment asserted CLAUDE.md could not collide across scopes, and it was
wrong. It covered only the repo-rooted-at-~ layout, where the depth-1 CLAUDE.md
and <config_root>/CLAUDE.md really are distinct. In the other common dotfiles
layout — ~/.claude itself tracked as the repo — they are the same physical file,
and the rules-only overlap guard missed it, so that file was emitted twice.

Both roots are now compared canonically, files as well as directories, via a
canon_file that resolves the containing dir and re-appends the basename rather
than depending on readlink -f. Six cases pin the layout, including that a normal
distinct-root project never regresses into a both-tagged CLAUDE.md.

Separately, R1 asks whether a rule duplicates "CLAUDE.md" — unambiguous while
only one could ever be in scope, ambiguous the moment this PR put two there. R1
now pairs within a scope: a user rule against the user CLAUDE.md, a project rule
against the project one, because R1 is a redundancy the owner of that layer fixes
by deleting one of the two and only a same-scope pair is theirs to fix.
Cross-scope overlap stays with Step 3, which reports it against the pair and
names each side; routing it through R1 as well would report one overlap twice and
address it to the wrong person.

Suite 32 -> 38 checks.

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

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

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


Security Review — PR #2234 (re-run at 57fd122)

  • Gather context (gh pr diff against origin/main, all changed files at HEAD 57fd122)
  • Review shell scripts (discover-instruction-surfaces.sh, .test.sh) for injection/path/trust issues, focused on the new canon_dir/dedup logic added since the last security pass
  • Review markdown/skill contract changes for authorization/logic gaps
  • Review plugin.json / evals.json / CHANGELOG.md / README.md for permission widening or leaked secrets
  • Post final findings

Findings

No security issues found.

This is a re-review at the latest commit (57fd122), which addresses the CLAUDE.md-collision correctness finding from the prior review round (the "canonicalize CLAUDE.md too" fix) plus a doc clarification on which CLAUDE.md R1 compares against. Both are correctness/design fixes, not security-relevant, so — consistent with this lane's scope — I focused only on injection, authz/trust-boundary, secret-handling, and GitHub Actions risk in the delta since the last security pass (d0eee4257fd122).

  • discover-instruction-surfaces.sh — the new canon_file() helper (L101-110) mirrors the existing canon_dir() pattern: it only calls dirname/basename/cd/pwd -P on paths already reached by the pre-existing project/config-root resolution, does no eval/command substitution on file contents, and all values still flow through printf '%s\t%s\t%s\n' as literal data. No new trust boundary is crossed — the CLAUDE.md overlap check (L137-143) is read-only comparison logic, same shape as the rules-dir overlap check it's modeled on.
  • The script remains always-exit-0, read-only, and reads CLAUDE_CONFIG_DIR the same way the pre-existing resolve-memory-dir.sh already does.
  • context/audit.md and reference/criteria.md changes are natural-language contract prose (which CLAUDE.md a rule's R1 check pairs against) — no code, no permission or trust-surface implication.
  • CHANGELOG.md diff in this commit is documentation only; no plugin.json, hook, or permissions changes in this commit.
  • No secrets/tokens/credentials introduced.
  • No GitHub Actions workflow files touched — the pull_request_target/workflow_run/permissions-widening/script-injection lane doesn't apply.

Nothing new to flag beyond the two prior review rounds (both were correctness findings, already fixed across d0eee42 and 57fd122).

Branch: fix/2228-audit-pass-report-containment

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 17s —— View job


Code Review — PR #2234 (re-run at 57fd122)

  • Gather context (diff of latest commit 57fd122 vs prior HEAD d0eee42)
  • Verify the CLAUDE.md-collision fix and R1 scope clarification actually address the two findings this lane raised last round
  • Check for new issues introduced by 57fd122
  • Post findings / summary

Scope note

This is a re-review focused on what changed since the last code-review pass on this PR (commit d0eee4257fd122, i.e. the "canonicalize CLAUDE.md too, and say which CLAUDE.md R1 compares against" fixup). Per REVIEW.md, security is the dedicated security lane's job, which has already run twice on this PR — I did not re-check that lane's territory.

The two prior findings this lane raised are substantively fixed:

  • The discover-instruction-surfaces.sh:108-110 CLAUDE.md-collision gap (project root == config root, e.g. ~/.claude tracked as the repo itself) is fixed: canon_file() now canonicalizes both CLAUDE.md paths the same way canon_dir() already did for the rules dirs, and emits a single both-tagged record when they coincide. The new CONFREPO fixture in the test file exercises exactly this layout and asserts one row, tagged both, never duplicated as project+user.
  • The R1-ambiguity finding (which CLAUDE.md does a rule's duplication check compare against, now that two can be in scope) got a real answer: reference/criteria.md's new "Which CLAUDE.md" note and context/audit.md's new "R1 pairs within a scope" bullet both say same-scope-only, and explicitly hand cross-scope overlap to Step 3 item 4 instead — which prevents the double-reporting the earlier finding was about.

New findings (posted inline)

  1. reference/criteria.md:269-276 — the new "Which CLAUDE.md" note's answer for a both-scoped rule ("compare against whichever CLAUDE.md shares its scope") has no target in the ~-rooted dotfiles case — the layout this PR's own audit-pass half calls "entirely ordinary." In that case the rule is tagged both but the two CLAUDE.md files stay distinct (project/user), confirmed by the PR's own HOMEREPO test fixture (discover-instruction-surfaces.test.sh:162 vs :168-169). Only the other, rarer dotfiles layout (project root == config root) actually produces a both-tagged CLAUDE.md to pair against. So the fix for the R1-ambiguity finding is correct for one collision case and silently inapplicable for the other, more common one.
  2. discover-instruction-surfaces.sh:121-123 — comment/doc accuracy only, not a functional bug: claims case 2 (project root == config root) makes "the two rules dirs" coincide too, but they generally don't — the code computes rules_overlap correctly, and the PR's own CONFREPO test-fixture comment (discover-instruction-surfaces.test.sh:180) already says as much (".claude/rules does not exist here, so only the CLAUDE.md collision is in play"). The comment (and its mirrors in the header block and --help text) should be corrected to match.

Verified independently (read, not executed — see below)

  • Traced canon_file()/canon_dir() path resolution by hand for both dotfiles layouts (project root = $HOME, and project root = config root) against the emit logic at lines 149-181 — the emitted-record behavior itself (dedup, both tagging, no double-emission) is correct for both cases.
  • Cross-checked the new reference/criteria.md R1 text, the mirrored context/audit.md bullet, and the CHANGELOG.md entry for consistency — the changelog's description of the R1 fix also only mentions the same-scope project/user pairing and never mentions the both-scoped-rule case, consistent with finding 1 being a real gap rather than something documented elsewhere.

Could not execute

Same sandbox restriction prior rounds on this PR hit: I could not run discover-instruction-surfaces.test.sh or shellcheck myself. The test file's "38 checks" claim (up from 32) is author-claimed and unverified by me; my findings above are based on reading the script and test file logic directly, including tracing the specific fixtures (HOMEREPO, CONFREPO) the test author added.

Branch: fix/2228-audit-pass-report-containment

View job

Comment thread plugins/claude-memory/skills/audit/reference/criteria.md Outdated
Comment thread plugins/claude-memory/skills/audit/scripts/discover-instruction-surfaces.sh Outdated
…a both-scoped rule an R1 partner

Two comment/doc errors, both mine, both caught by review reading the code
against the prose rather than trusting it.

The header claimed a repo rooted at ~/.claude collides CLAUDE.md "and so are the
two rules dirs". It does not: project rules there resolve to
<config_root>/.claude/rules, which is not <config_root>/rules. My own fixture
already agreed with the code and not the comment.

The truth is a clean asymmetry — each layout collides exactly ONE surface. A repo
rooted at ~ collides the rules dir and leaves the two CLAUDE.md files distinct; a
repo rooted at ~/.claude collides CLAUDE.md and leaves the rules dirs distinct.
That is precisely why the two comparisons are computed independently rather than
from one flag, and it is now stated that way and pinned in both directions
instead of being asserted in a comment.

The asymmetry also broke R1's new pairing rule. "A both-scoped rule against
whichever CLAUDE.md shares its scope" has no target: both-scoped RULES arise only
in the ~-rooted layout, and that is exactly the layout where no both-scoped
CLAUDE.md exists. Such a rule now compares against every CLAUDE.md in scope, with
each finding attributed to the scope of the one it overlapped — not
double-reporting, because the rule really is loaded alongside both.

Suite 38 -> 44 checks.

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

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

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


Code Review — PR #2234 (re-run at 02ea9cf)

  • Gather context (diff of latest commit 02ea9cf vs prior HEAD 57fd122)
  • Verify the two 23:16 findings (R1 "both"-scope target gap, case-2 comment inaccuracy) are actually fixed
  • Check for new issues introduced by 02ea9cf
  • Post findings / summary

Scope note

Per REVIEW.md, security is the dedicated security lane's job (already run 3 times on this PR). This pass covers only what changed since my last code-review round: commit 02ea9cf ("correct the per-layout collision claim, and give a both-scoped rule an R1 partner"), which touches plugins/claude-memory/{CHANGELOG.md, skills/audit/reference/criteria.md, skills/audit/scripts/discover-instruction-surfaces.{sh,test.sh}}. No claude-config/audit-pass files changed in this delta, so I didn't re-review those.

Both prior findings from this lane are genuinely fixed

  1. R1 "both"-scoped-rule target gap (was: reference/criteria.md had no target for "whichever CLAUDE.md shares its scope" in the ~-rooted case) — fixed at reference/criteria.md:275-280. The new text states plainly that a both-scoped rule arises only from the ~-rooted layout, where the two CLAUDE.md files stay distinct, so it has no same-scope partner — and directs comparing it against every CLAUDE.md in scope, attributing each finding to whichever side it overlapped. This is a real target, not a dead end. I traced the script's md_overlap/rules_overlap computation (discover-instruction-surfaces.sh:131-147) to confirm the two collisions are independent as claimed: layout 1 (root at $HOME) only ever sets rules_overlap, layout 2 (root at $config_root) only ever sets md_overlap (project rules there resolve to $config_root/.claude/rules, not $config_root/rules) — so "a both-scoped rule never has a both-scoped CLAUDE.md to pair against" holds by construction, not just by example.
  2. Case-2 comment inaccuracy (claimed layout 2 collides "the two rules dirs" too) — fixed at discover-instruction-surfaces.sh:118-130, including the header block and --help text. The comment now correctly states each layout collides exactly one surface (layout 1 → rules only, layout 2 → CLAUDE.md only) and spells out why they're independent.

New test coverage added, logic checked against the fixture (not executed — see below)

The new CONFREPO2 fixture (discover-instruction-surfaces.test.sh:204-221) pins the asymmetry in both directions: layout 1's rule collides but its CLAUDE.md doesn't, layout 2's CLAUDE.md collides but its rules don't. I traced the fixture by hand against the script logic (canon_dir/canon_file on the exact paths the fixture creates) and the assertions match what the code actually does — e.g. .claude/rules under $CONFREPO2 (project) resolves to $CONFREPO2/.claude/rules, while $config_root/rules resolves to $CONFREPO2/rules — genuinely distinct directories, so assert_not_contains "layout 2 leaves the rules dirs distinct" is correct. CHANGELOG.md's "44 checks" claim matches the assertion count in the test file (44, counted via grep).

No new issues found in this delta

I did not find any new correctness problems introduced by 02ea9cf. The fix is scoped precisely to the two gaps raised last round, doesn't touch audit.md (whose "R1 pairs within a scope" bullet already deferred to criteria.md's note, so it didn't need updating), and I found no stale references to the old "whichever CLAUDE.md shares its scope" text anywhere else in the plugin.

Could not execute

Same sandbox restriction every round of this PR has hit: bash <script> and shellcheck both require approval I can't grant in this lane, so I could not run discover-instruction-surfaces.test.sh myself — the above is based on reading the script and test file and tracing the logic by hand, not on running it. I was able to run jq empty on evals/evals.json and both plugin.json files — both valid JSON (unrelated to this delta, unchanged since the last round).

Branch: fix/2228-audit-pass-report-containment

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

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


Security Review — PR #2234 (re-run at 02ea9cf)

  • Gather context (gh pr diff against origin/main, all 16 changed files at HEAD 02ea9cf)
  • Review shell scripts (discover-instruction-surfaces.sh, .test.sh) for injection/path/trust issues
  • Review markdown/skill contract changes (SKILL.md, report-location-and-schema.md, exclusion-set.md, determinism-tiers.md, context/audit.md, criteria.md) for authorization/logic gaps
  • Review plugin.json / evals.json / CHANGELOG.md / README.md for permission widening or leaked secrets
  • Post final findings

Findings

No security issues found.

This is a re-review at the latest commit (02ea9cf), which fixes a correctness issue from the prior code-review round (per-layout CLAUDE.md/rules collision claim, and the R1 pairing note for a both-scoped rule when no both-scoped CLAUDE.md exists). That's a correctness/documentation fix, not security-relevant, so — consistent with this lane's scope of injection / authz / trust-boundary / secrets / GitHub Actions risk only — I focused on whether anything in the delta since the last security pass (57fd12202ea9cf) changes that picture. It doesn't.

  • discover-instruction-surfaces.sh — unchanged in this commit's diff (the fix landed in reference/criteria.md and context/audit.md, prose only). It remains read-only, always exits 0, does no eval/command substitution on untrusted content, and passes all resolved paths through printf '%s\t%s\t%s\n' as literal data. CLAUDE_CONFIG_DIR is read the same way the pre-existing resolve-memory-dir.sh already does — no new trust boundary. All find invocations use -type f without -L, so a symlink placed under .claude/rules or $config_root/rules is not dereferenced/followed into as a file match, which rules out a symlink-based read-outside-scope vector.
  • reference/criteria.md / context/audit.md changes in this commit are natural-language contract prose only (which CLAUDE.md a both-scoped rule's R1 check pairs against) — no code, no permission or trust-surface implication.
  • No plugin.json, hook, or permissions changes in this commit.
  • No secrets/tokens/credentials introduced anywhere in the 16-file diff.
  • No GitHub Actions workflow files touched — the pull_request_target/workflow_run/permissions-widening/script-injection lane doesn't apply to this PR.

Across the full PR (all 16 files vs. origin/main): the audit-pass containment re-gating (report_path ⊆ target_root) and non-git refusal are uniformly tightening — no case widens what gets written, grants new access, or removes an existing safeguard. The claude-memory discovery script is net-new but stays inside the same read-only, always-exit-0, advisory contract the plugin already uses elsewhere, with CLAUDE_CONFIG_DIR handled the same way the existing resolver handles it. Nothing new to flag beyond the four prior review rounds on this PR (all correctness findings, already fixed across 643ec22, d0eee42, 57fd122, and 02ea9cf).

Branch: fix/2228-audit-pass-report-containment

@kyle-sexton
kyle-sexton merged commit 96d578d into main Aug 11, 2026
36 checks passed
@kyle-sexton
kyle-sexton deleted the fix/2228-audit-pass-report-containment branch August 11, 2026 23:28
kyle-sexton added a commit that referenced this pull request Aug 12, 2026
…an unresolvable scan root, and key the postures report per project (#2259)

## Summary

Three findings, one plugin. Every claim below was **executed or grepped
this pass**, not derived from a
model of the code — the standard this batch adopted after PR 2's review
found three defects of the shape
"a comment asserting something the code does not do".

### P2's rationale is unsupported by the docs, and false on rule classes
it fires on (#2248)

`reference/criteria.md:63` read *"Bash rules match literally with no
`~`/`$HOME`/env expansion"*.

**Grepped, not assumed.** Both pages pulled with `curl` to a file
(`skills.md` 87,211 bytes;
`permissions.md` 61,351 bytes) and searched:
`grep -in "no ~/\$HOME|match literally|literally with no|no
expansion|does not expand"` returns **zero
hits across both**. The Bash section (`permissions.md:162-176`)
specifies wildcard glob matching and
states no no-expansion rule. So the claim is **unsupported**, not merely
over-broad.

Two documented behaviors contradict it:

- `skills.md:333` — *"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."* The
canonical example at `:339` is `allowed-tools:
Bash(${CLAUDE_SKILL_DIR}/scripts/render.sh *)`. The
skill was telling authors to remove the documented zero-prompt pattern.
- `permissions.md:190` — known-safe leading env-assignment stripping,
and **scoped**: an allow rule
won't match past an assignment of any other variable, while deny/ask
match past any. Stated flatly it
  would over-generalize, so the new text carries the scoping.

**And the message is emitted on rule classes where it is false twice
over.** The single string at
`permission-rule-check.sh:139` serves every class. Probed against the
shipped script:

| Rule | Result |
|---|---|
| `Bash(/c/Users/kyle/x.sh:*)` | flagged P2 |
| `Read(/c/Users/kyle/notes.md)` | **flagged, with the "Bash rules match
literally" message** |
| `Edit(/Users/alice/src/**)` | **flagged, same message** |
| `Read(~/Documents/notes.md)` | not flagged (correct) |
| `Bash(${CLAUDE_SKILL_DIR}/scripts/x.sh *)` | not flagged (correct) |
| `Bash(${CLAUDE_PROJECT_DIR}/scripts/lint.sh *)` | not flagged
(correct) |

`Read`/`Edit` rules use gitignore pattern syntax and **do** resolve `~/`
(`permissions.md:280`:
`Read(~/Documents/*.pdf)` → `/Users/alice/Documents/*.pdf`). So on a
`Read` finding the old message named
the wrong rule class *and* asserted a mechanism false for that class.

The emitted message now carries only what is true of every class — the
portability break — and names the
portable form per class. The mechanism detail moves into `criteria.md`
as a per-rule-class table, syncing
**down** from
`docs/conventions/permission-rule-hygiene/README.md:106-134`, which
already held the
corrected doctrine and two limits the ledger's sketch omitted:
`${CLAUDE_PROJECT_DIR}` substitution
requires **v2.1.196+**, and `${CLAUDE_PLUGIN_ROOT}` is **not**
substituted at all, so a rule using it is
inert. No convention edit needed — this is the sync direction.

### The scan fell through to `$PWD` and swept the user profile, exiting
0 (#2249)

`permission-rule-check.sh:71-76` ended
`ROOT="${CLAUDE_PROJECT_DIR:-$PWD}"`. Outside a repository `$PWD`
is whatever directory the session stands in — on a developer machine,
usually the user profile — and
both scans walk it with `find`, no `-maxdepth`, no `-prune`, stderr
discarded, then `exit 0`. A timeout
or a swallowed permission error was indistinguishable from a clean bill,
on a skill that is
model-invocable (`disable-model-invocation: false` at `SKILL.md:5`).

The ladder now ends at `${CLAUDE_PROJECT_DIR}`; an unresolvable root
**exits 2**, reusing the
environment-gap channel the contract already documents for a missing
`jq` rather than minting a code, so
the advisory exit-0-for-findings contract is untouched. **`--count`
refuses too** — a `0` from a scan
that never resolved a root reads exactly like a clean bill, and that
path exited 0 separately at `:264`.

**Ledger correction, verified:** the ledger lists `SKILL.md:56` as a
third "always exits 0" site to
update. `grep -n "exits 0\|exit 0"` over `SKILL.md` returns **no
match**; `:56` is the jq-exits-2 line.
`SKILL.md` carries no always-exits-0 claim. The real count is **five**,
in two files:
`reference/criteria.md:20` plus the script's header, usage block, and
`--help` text. All moved together.

### The postures report had no project dimension (#2250)

`audit-prompting-postures/SKILL.md:78` persisted to one fixed
`${CLAUDE_PLUGIN_DATA}/audit-prompting-postures/last-audit.md`.
`${CLAUDE_PLUGIN_DATA}` resolves to
`~/.claude/plugins/data/{id}/` where `{id}` is the **plugin**
identifier, never the project — so the
skill's only durable deliverable was silently overwritten by the next
run from any other root.

**The filed fix sketch is not what shipped, deliberately.** It proposed
`${CLAUDE_PROJECT_DIR}` with a
"when set, else" fallback. That placeholder substitutes **inline in
skill content**, so the model never
sees the literal token and cannot evaluate "when set" — the defect filed
separately against `audit-pass`
as `F9`. Implementing it as sketched would have introduced that defect
while removing this one. The
derivation is therefore written as **commands to run**.

**The scheme is `audit-pass`'s, reused rather than reinvented** —
`<repo-identity>/<worktree-discriminator>` from
`run-state-and-resumability.md` §3 — because a second
scheme for one concern is the drift this batch exists to remove. **One
rung added:** that ladder has
git-with-remote and git-without-remote and no non-repo rung.
`audit-pass` does not need one (PR #2234
made it refuse non-git targets); this skill is report-only and
legitimately audits them — the run that
produced the finding was rooted at a non-repo home directory.

Plus the mandated three-line header (resolved root, scope filter, UTC
timestamp), so a surviving report
is self-describing rather than merely un-overwritten.

### What review caught — including the same defect class, in my own work

Five threads, all resolved, four follow-up commits. Two of the findings
were the exact shape this batch
keeps hitting, and both were mine:

- **The snippet read `remote.origin.url` while the sentence above it
claimed verbatim reuse of a scheme
that says "the first configured remote URL".** Reproduced: a repo whose
only remote is `upstream`
returns empty and fell to the `local/` rung *despite having a remote*,
so one repository keyed
differently depending on what someone named their remote. Now `git
remote | head -1`.
- **A remote URL is arbitrary text that becomes directory components,
and I did not validate it.**
Reproduced with `git remote add origin ../central.git`: state key
`../central/2a8fd283`, and the
report path normalizes to `/…/central/…` — **outside this skill's
namespace entirely**. Absolute-local
and Windows-path remotes break the same way. The identity is now
accepted only in the shape the scheme
  means, and anything else keys by hash, still deterministically.

Plus three smaller ones: `root` now strips CRLF to match the sibling
script this same PR touches; the
new `-d "$ROOT"` guard shipped untested next to four assertions for its
sibling branch, and now has five
cases; and CI's machine-path detector caught four literals I added,
where the test file already had a
runtime-assembly idiom I had failed to follow.

Verified after the remote fix across seven shapes — relative,
absolute-local, Windows, https, scp-style
ssh, no-remote, non-repo:

```
  CONTAINED  remote/feb98d0c6fe2/5bdf8482       relative filesystem remote
  CONTAINED  remote/9025af3e59ae/72f53c5b       absolute local remote
  CONTAINED  remote/ee5e188160e5/f80a5d9b       windows local remote
  CONTAINED  github.com/acme/widget/5fb34de0    normal https remote
  CONTAINED  github.com/acme/widget/31e38d12    scp-style ssh remote
  CONTAINED  local/aa0eeb6c3f8f/aa0eeb6c        no remote at all
  CONTAINED  nonrepo/862f1fe5638e/862f1fe5      not a repo

  remote name: upstream   key: github.com/acme/widget/51e207e2   (was local/… before the fix)
  stable across runs; with several remotes the first wins
```

### Trust surface

Narrows. The scan refuses an unresolved root instead of walking the
user's home, and the report path can
no longer escape the skill's own directory via a crafted remote. Nothing
widens. No new grant, hook, or
network read.

## Test plan

**Fail-before / pass-after, both rows.** The new assertions run against
the **pre-fix** script
(`git checkout origin/main -- permission-rule-check.sh`, test file
kept):

```
$ bash plugins/claude-config/skills/audit-permission-grants/scripts/permission-rule-check.test.sh
FAIL: P2 detail does not assert a blanket no-expansion rule
FAIL: P2 detail does not scope its rationale to Bash rules
FAIL: Read-rule finding does not claim Bash semantics
FAIL: Read-rule finding does not deny ~ expansion
FAIL: unresolvable root exits 2, not 0
FAIL: refusal names the failure
FAIL: refusal is not a clean bill
FAIL: --count also refuses rather than printing 0
14/68 checks failed.
```

Restored, and after the fix:

```
$ bash plugins/claude-config/skills/audit-permission-grants/scripts/permission-rule-check.test.sh
All 68 checks passed.
```

Baseline for reference was `All 50 checks passed.` at the merge-base.

Two of the new assertions are **guards, not failing tests**, and are
presented as such: the
`${CLAUDE_SKILL_DIR}` positive case and "explicit fixture root still
scans" both pass before the change
too. The A4 refusal test unsets `CLAUDE_PROJECT_DIR` as well as
`PERMISSION_HYGIENE_FIXTURE_DIR` and sets
`GIT_CEILING_DIRECTORIES`, so it fails for the intended reason rather
than inheriting a root from the
outer session.

**The CC-F1 derivation was executed, not just written.** All three
identity rungs, in real contexts:

```
--- context 1: this repo (has a remote)
github.com/melodic-software/claude-code-plugins/8163d6b9
--- context 2: git repo with NO remote
local/c508785eede5/c508785e
--- context 3: not a repo at all
nonrepo/fa5107130e4e/fa510713
```

And the worktree discriminator does what it exists for — two worktrees
of this repository:

```
C:/Projects/melodic/worktrees/batch4-laneA-audit -> 8163d6b9
C:/Projects/melodic/claude-code-plugins         -> 009628cd
```

Portability caught by executing rather than assuming: `sha256sum` is
absent on stock macOS, so the
snippet carries a `shasum -a 256` fallback (both verified present here).

**Static gates:**

```
$ shellcheck permission-rule-check.sh permission-rule-check.test.sh     (clean)
$ jq empty audit-prompting-postures/evals/evals.json plugin.json        (clean)
$ bash check-evals-quality.sh audit-prompting-postures/evals/evals.json
check-evals-quality: PASS (0 warning(s) across 1 file(s))
$ CHECK_SKILL_SKILLS_ROOT=... check-skill.sh audit-permission-grants
CHECK-SKILL audit-permission-grants: PASS — 0 errors, 1 warning(s)   (pre-existing: no Gotchas surface)
$ CHECK_SKILL_SKILLS_ROOT=... check-skill.sh audit-prompting-postures
CHECK-SKILL audit-prompting-postures: PASS — 0 errors, 0 warning(s)
$ npx markdownlint-cli2 <5 changed md files>                            Summary: 0 issues in 0 files
```

CHANGELOG verified as additions only: `git diff --cached CHANGELOG.md |
grep -c '^-[^-]'` = **0**, so the
shipped `0.30.0` and `0.31.0` entries are untouched.

## Related

Closes #2248
Closes #2249
Closes #2250

Inbox items: batch-4 ledger `I10` rows `A1` and `A4`; ledger `I9` row
`CC-F1`, whose mechanism was
replaced per reconciliation `OR-3` (`rationale_falsified` in effect,
caught cross-ledger).

Bump: `claude-config` 0.31.0 → **0.32.0** — minor, because two graded
behaviors move (an unresolvable
root now refuses; the report path changes).

**Reproduced but deliberately not fixed:** `criteria.md:60-61` claims
`//…` forms are not flagged, and
`Read(//Users/alice/secrets/**)` — the docs' own literal example at
`permissions.md:278` — **is** flagged.
That is ledger row `A2` (MED, `implement_now: false`), a different hunk
in the same file. The
reproduction is recorded on #2248 as corroborating evidence for it; the
new text asserts nothing about
`//` exemption. Also left: `A3` (P2 prints an 8-char fragment), `A11`,
`A14`, and `CC-F2`–`CC-F11`.
Filing for those is held pending the operator's call on batch-4 volume
(`OR-4`).

---------

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

Copy link
Copy Markdown
Contributor Author

Batch-4 filing note — three follow-ons, and one collision warning.

#2280 files what this PR left standing in audit-pass: no scripts/ directory at all behind 1,547 lines of lease/heartbeat/owner_epoch/partial specification (F3, F5, F12, F13), the unstated {id} mangling rule (F8), and the ${CLAUDE_PROJECT_DIR}-"when set" conditional at SKILL.md:42-43 (F9) — which now contradicts the explicit prohibition #2250 landed in a sibling skill of the same plugin. Note that F8 is partly discharged by this PR: report-location-and-schema.md:19-23 now states the resolution and cites the source; only the mangling rule and the not-in-Bash-env fact remain.

#2279 files the degradation posture: Phase 5's apply-verify names a fresh-context subagent as "the stated fallback" with no fallback of its own, and the report schema's section table (report-location-and-schema.md:112-119) has no per-lane verification-mode field — while skipped exists in that same table precisely because "a silent exclusion reads as coverage".

Collision warning for claude-memory: #2277 edits plugins/claude-memory/skills/audit/context/audit.md:104 (the unkeyed report path). This PR edited :8-40 of the same file. Different hunks — rebase rather than assume.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment