feat(claude-config,claude-memory): key plugin-data reports per project and contract the rule - #2335
Conversation
…t and contract the rule
Three writers under `${CLAUDE_PLUGIN_DATA}` shared one file per machine, and
`docs/conventions/` had no rule to point at.
`claude-memory:audit` is the sharp one, because it READS its report back.
`report` mode served whatever `audit/last-audit.md` currently held and `fix`
mode acted on it — so on a machine with two repositories, project B could be
shown project A's findings and offered edits derived from another repository's
memory layer. A wrong answer served, not merely a lost artifact, which is why an
append-only history would not close it: serving the newest report is not serving
this project's. All four sites now resolve one keyed path, derived in SKILL.md
and referred to by the spokes rather than restated.
`claude-config:audit-instructions` loses more than the artifact too. Its Phase D
header carries a per-surface token delta "versus the previous catalog version";
under collision that prior file belongs to a different project's surface set, so
the skill printed a number instead of declining. A silently wrong figure in a
report header is worse than a missing one.
The scheme is `audit-pass`'s, not a new one — but the third adopter would have
been the third copy of a ~40-line shell block, so it ships as `lib/state-key.sh`
with a 23-case suite, byte-identical across both plugins and registered in
`scripts/cross-plugin-source-registry.txt`. The suite pins what the prose
asserted and nothing checked, including the security property: a remote URL
becomes directory components, so `../../../etc`, an absolute local path, and a
Windows path are hashed rather than embedded, with no `..` and no backslash
surviving into a key.
Two decisions worth stating. Legacy unkeyed reports — including the pre-rename
`health/` layout `claude-memory` used to migrate — are neither served nor
adopted: they carry no project segment, so nothing records which repository
produced them, and moving one under a key would invent the attribution keying
exists to remove. And the derivation lives in `SKILL.md`, which is unambiguously
skill content, with the spokes pointing at it; whether substitution reaches
bundled spoke files is #1568's open question and this fix does not depend on the
answer.
`docs/conventions/plugin-data-report-keying/` carries the rule, separating the
two failure modes that need different fixes (collision needs keying, overwrite
needs retention) and naming the "looks scoped but isn't" case — keying on a
project-root basename — as context rather than as a defect filed against the
plugin that has it.
Closes #2276
Closes #2277
Closes #2278
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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 finished @kyle-sexton's task in 4m 26s —— View job Security review
Scope: 1. IMPORTANT — the identity validator can be bypassed via an embedded newline in the git remote URL, letting unsanitized
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4da81ee881
ℹ️ 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".
…ared state-key resolver Shipping lib/state-key.sh while leaving a hand-rolled copy of the same derivation in a sibling skill of the same plugin is exactly the drift the convention this PR adds forbids — and the CHANGELOG entry claimed a consolidation the diff had not made. The ~40-line block is replaced by the one call; its verified properties stay stated in the skill, since a reader needs to know what the key guarantees, and only the restated algorithm is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Update — migrated onto the shared resolver in this PR (39725b8). Self-review caught that the CHANGELOG entry described a consolidation the diff had not made: The block is now the one call. Its verified properties stay stated in the skill (a reader needs to know what the key guarantees); only the restated algorithm is gone. Two things checked while there, both clean:
|
|
Claude finished @kyle-sexton's task in 4m 2s —— View job Security review
Scope: 1. IMPORTANT — identity validator can be bypassed via an embedded newline in the git remote URL; still present at current HEAD, and now also reachable from
|
|
Update — Self-review caught that the CHANGELOG entry described a consolidation the diff had not made: The block is now the one call. Its verified properties stay stated in the skill — a reader needs to know what the key guarantees — and only the restated algorithm is gone. Two things checked while there, both clean:
(An earlier revision of this comment was posted with a mangled line from shell substitution — this is the corrected text.) |
|
Claude finished @kyle-sexton's task in 5m 56s —— View job Code reviewScope: correctness, error handling, test coverage, and maintainability per this repo's
Posted two inline findings on
Both are inspection-based: this review's sandbox blocks the shell mutations ( 3. 🟡 Suggestion — the PR's own new Rule 4 isn't applied to either writer it addsFile: Rule 4 states: "A component whose sole durable output lives there should say so once, near the path, rather than letting an operator discover it [that uninstalling the plugin from its last scope deletes it, absent Everything else reviewed cleanly:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| shasum -a 256 | ||
| else | ||
| echo "ERROR: no sha256sum or shasum on PATH — cannot derive a state key" >&2 | ||
| exit 2 |
There was a problem hiding this comment.
🔴 Important — error-handling
sha256()'s exit 2 on line 116 only exits the subshell that runs sha256, not the top-level script: hash12()/hash8() invoke it as the middle stage of a pipeline (printf ... | sha256 | cut -c1-12), and every element of a pipeline runs in its own subshell in bash unless it's the last stage under lastpipe (not enabled here, and irrelevant since sha256 isn't the last stage regardless). The script also never sets -e, and a failing command inside a $(...) assignment isn't otherwise checked.
Concretely, on a minimal image lacking both sha256sum and shasum: sha256 prints its error to stderr and exits 2 inside its own subshell; cut -c1-12 still runs (on empty stdin) and produces an empty string; hash12/hash8 return empty; and the caller ends up with a malformed key such as local/ or nonrepo//<8-hash-of-nothing>. The top-level script still exits 0 and prints that malformed key on stdout, contradicting the header's own documented contract at line 43 ("Exit: 0 always on a successful derivation… there is no 'cannot key' outcome. 2 on a bad argument or an unusable --root") — this is an unusable derivation, but it's reported as success. A caller (e.g. the ${CLAUDE_PLUGIN_DATA}/audit/<state-key>/... writers this PR wires up) would then read/write under that malformed path without any signal that the key is degenerate.
Same logic is byte-identical in plugins/claude-memory/lib/state-key.sh, so it needs the same fix in both (or just the canonical copy, then re-sync per the registry note).
Note: I read this from static analysis only — the sandbox for this review run blocks the shell mutations (ln, mktemp, PATH manipulation) needed to empirically reproduce a missing-sha256sum/shasum environment, so I could not execute a live repro here. The subshell/pipeline semantics and the "no -e" observation are directly verifiable by reading the script, though.
Suggested fix: have sha256() communicate failure to the caller rather than relying on exit inside a piped subshell — e.g. have hash12/hash8 check set -o pipefail (already set globally) and propagate a non-zero status with an explicit || exit 2 at each call site, or restructure so the sha256-availability check runs once at the top of the script (outside any pipeline) and the script exits 2 immediately if neither tool is present.
| # "the FIRST configured remote" — not necessarily one named `origin`. A repo | ||
| # whose only remote is `upstream` still has a remote and must not fall through | ||
| # to the local rung. | ||
| remote_name=$(git remote 2>/dev/null | tr -d '\r' | head -1) |
There was a problem hiding this comment.
🟡 Nit — documentation-accuracy
The comment ("the FIRST configured remote… not necessarily one named origin") and its restatement in docs/conventions/plugin-data-report-keying/README.md line 66 ("the first configured remote URL") describe git remote's output as configuration/insertion order. It isn't: git remote with no arguments lists remote names sorted alphabetically, not in the order they were added to .git/config. On a repo with two remotes named e.g. zzz-mirror (added first) and aaa-upstream (added later), git remote | head -1 returns aaa-upstream, not the one actually configured first.
This doesn't break the security property or determinism (the same repo state always yields the same key), so it's a documentation-precision nit rather than a functional bug — but a reader relying on the "first configured" framing to predict which remote wins on a multi-remote repo would predict wrong. Test case 3 in state-key.test.sh only exercises a single-remote (upstream-only) repo, so this alphabetical-vs-insertion-order distinction isn't covered either.
I wasn't able to run git remote against a constructed multi-remote fixture in this sandbox (repo-mutating git commands require approval I don't have in this review run), so this is based on documented/known git behavior rather than an execution I performed here.
Suggested fix: reword the comment/README to say "the alphabetically-first remote name" (or whichever is actually intended), or if insertion order is the actually-desired semantic, resolve it from .git/config's section order instead of git remote's sorted output.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
) Fixes #2282 (scoped rows A2, A3, A19). > **Scope note:** this PR takes three of #2282's five rows. **A7b** and **A12** both reproduce at HEAD and are untouched; they are now filed as **#2397**, so the closing keyword above no longer drops them. ## Summary Scoped fix for three of #2282's five rows in `audit-permission-grants`. - **A2 — direction reversed after review.** As filed, this row implies "make the `//` exemption real". The docs settle it the other way: `permissions.md` gives `//path` = "Absolute path from filesystem root" with `Read(//Users/alice/secrets/**)` → `/Users/alice/secrets/**`, and "Use `//Users/alice/file` for absolute paths." So `//Users/<name>/…` is the canonical *spelling* of a hardcoded user home, and exempting it would have made an `error`-tier username-leak check blind to the documentation's own example of the leak. **`criteria.md` moved; the detector keeps flagging `//`.** The inconsistency the row reports was real — the two shipped files disagreed — but the document was the wrong one. - **A3:** P2 findings report the full `Tool(…)` rule, not an eight-character path fragment. Two regressions this introduced are also fixed: the tool name was enumerated as `(Read|Edit|Write|Bash|PowerShell)` (silently dropping `WebFetch`/`Glob`/`NotebookEdit`/`mcp__*`/`Agent` rules — `Agent` indefensible, this script ships `scan_agent()`), and the `//` skip was a substring test that let `Read(//opt/data/../Users/kyle/secrets)` read clean. - **A19:** `Bash(npm view ctx7 version*)` is no longer treated as a bare package-manager wildcard — `*` must be preceded by a separator. Out of scope and **now tracked in #2397**: A7b (inert-grant check) and A12 (`~user` username leak). Both reproduce at HEAD, with evidence carried into that issue, so closing #2282 here drops nothing. ## Test plan A19's change verified against the shipped library at this branch, sourced verbatim — the false positive is gone and every true positive still matches: ``` $ . plugins/claude-config/lib/permission-patterns.sh $ for r in 'Bash(npm view ctx7 version*)' 'Bash(npm *)' 'Bash(npm:*)' 'Bash(npx *)' \ 'Bash(pnpm dlx *)' 'Bash(npm test)' 'Bash(npm run build *)'; do printf '%-32s -> ' "$r" out=$(printf '%s\n' "$r" | grep -oE "$CCPERM_P1_ERE"); [ -n "$out" ] && echo FLAGGED || echo clean done Bash(npm view ctx7 version*) -> clean # the false positive A19 filed Bash(npm *) -> FLAGGED Bash(npm:*) -> FLAGGED Bash(npx *) -> FLAGGED Bash(pnpm dlx *) -> FLAGGED Bash(npm test) -> clean Bash(npm run build *) -> FLAGGED ``` A2/A3's `P2_RULE_ERE` compared against the pattern it replaces, across tool names — this is where the two regressions below were found: ``` rule | old P2 | new P2 outcome Read(//Users/alice/secrets/**) | /Users/a | SKIPPED (//) Bash(/c/Users/kyle/x.sh:*) | /Users/k | FLAGGED (full rule now — A3 works) Read(/Users/kyle/.aws/credentials) | /Users/k | FLAGGED WebFetch(/Users/kyle/x) | /Users/k | NOT MATCHED <-- regression Agent(/Users/kyle/x) | /Users/k | NOT MATCHED <-- regression mcp__srv__tool(/Users/kyle/x) | /Users/k | NOT MATCHED <-- regression Read(//opt/data/../Users/kyle/secrets) | /Users/k | SKIPPED (//) <-- over-broad exemption ``` `permission-rule-check.test.sh` passes on this branch (72 + 16 new cases); none of the seven rows above is in it, which is why it stayed green. ## Related - **#2282** — the owning issue, closed here. Rows **A7b** and **A12** were split to **#2397** before merge so the auto-close drops nothing. - **#2397** — the follow-up carrying A7b + A12 with their HEAD reproductions, the A12-vs-`%USERPROFILE%` split, and A7b's branching-remedy constraint. - **#2260** — extracted the P1 rule vocabulary into `plugins/claude-config/lib/permission-patterns.sh`, which is why A19's fix lands in `lib/` rather than at the `scripts/permission-rule-check.sh:138,144` anchors #2282 names. That library now has a **second consumer** (`audit-permission-state`), so this change is no longer scoped to one detector. - **#2248 / #2249** (closed) — the previous `permission-rule-check` passes this builds on. #2248 rewrote P2's *rationale* without touching `P2_ERE`; #2249 removed the `$PWD` fallback and added the exit-2 refusal. - **#2283** — the sibling `audit-permission-grants` issue (clean bill with no denominator, `vendor/`-only exclusion, unimplemented scope filters). Not touched here. - **#2284** — the `criteria.md` staleness cluster. **Directly relevant:** the `//` exemption this PR implements comes from `criteria.md:64-65`, and #2284 is the issue about that file's doctrine being stale. See the review thread. - **#1398** (open) — faults P1's bare-name-on-PATH `Recommend`, the same remediation surface. - **#2301, #2335** — handoff-inbox batch 4 lane CC, the other `claude-config` waves. #2335 is open and bumps the same manifest, so this PR and that one will collide on `plugin.json` / `CHANGELOG.md`; whichever merges second needs a rebase. Inbox item: `20260811-024628-claude-config-audit-permission-grants-defects-and-fleet-grant-hygiene`. Ledger: `.work/handoff-inbox-batch-4/ledgers/I10-permission-grants-fleet.md` § A2, A3, A19. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
Three writers under
${CLAUDE_PLUGIN_DATA}shared one file per machine, anddocs/conventions/had no rule to point at.The harness fact, verbatim — plugins reference, § Persistent data directory, re-fetched as raw markdown 2026-08-12: "The
${CLAUDE_PLUGIN_DATA}directory resolves to~/.claude/plugins/data/{id}/, where{id}is the plugin identifier with characters outsidea-z,A-Z,0-9,_, and-replaced by-." Keyed to the plugin identifier and nothing else — no project, checkout, worktree, or session segment. A fixed filename there is one file per machine.#2277 —
claude-memory:auditis the sharp one, because it reads the file backreportmode served whateveraudit/last-audit.mdcurrently held andfixmode acted on it. On a machine with two repositories,reportin project B could present project A's findings as project B's, andfixcould propose edits derived from another repository's memory layer. A wrong answer served, not merely a lost artifact — which is why an append-only history would not close it: serving the newest report is not serving this project's.All four sites now resolve one keyed path,
audit/<state-key>/last-audit.md: the write incontext/audit.md, its restatement inreference/criteria.md, and the two reads inSKILL.mdandcontext/fix.md.#2276 —
claude-config:audit-instructionsprinted a wrong number, not just a lost fileIts Phase D header must carry a per-surface token delta "versus the previous catalog version". Under collision that prior file exists but belongs to a different project's surface set, so the skill computed and printed a figure instead of declining. A silently wrong number in a report header is the worse half of this defect. The path is now keyed, and the two absent-prior cases are separated: no report at this project's key → omit the delta with a stated reason; an unkeyed leftover → name it to the operator, never use it as a baseline.
It was the last writer in this plugin on a fixed path.
audit-passhas keyed since it shipped; #2250 movedaudit-prompting-postures.The scheme is
audit-pass's — but the third adopter would have been the third copyaudit-passspecifies<repo-identity>/<worktree-discriminator>in prose; #2250 copied a ~40-line shell block intoaudit-prompting-postures. Rather than paste it twice more, it ships aslib/state-key.shwith a 23-case suite — byte-identical across both plugins and registered inscripts/cross-plugin-source-registry.txt, following the existinglib/managed-scope.shprecedent, so the copies cannot drift silently.The suite pins what the prose asserted and nothing checked, including the security property: a remote URL becomes directory components in the resulting path, so
../../../etc, an absolute local path, and a Windows path are hashed rather than embedded, and the suite asserts no..and no backslash survives into a key.Two decisions worth stating plainly
claude-memorypreviously moved the pre-renamehealth/directory toaudit/and read it. Both older layouts carry no project segment, so nothing records which repository produced them; adopting one into a project's key invents the attribution keying exists to remove. Both read paths now decline, name the leftover file's path as something the operator may delete, and offer a fresh audit. This is a behavior change on upgrade — an operator holding a report under the old layout is told to re-run rather than shown the old one. It is whyclaude-memorytakes a minor bump.SKILL.md, and the spokes point at it.SKILL.mdis unambiguously skill content, doc-confirmed for placeholder substitution. Whether substitution reaches bundledcontext/*.mdandreference/*.mdloaded on demand is plugin-quality:audit asserts ${CLAUDE_PLUGIN_DATA} does not substitute in skill markdown; plugins-reference now says it does #1568's open question, and this fix deliberately does not depend on the answer:SKILL.mdderives the path once, the three spokes refer to it by section name and restate no token of their own.#2278 — the convention
docs/conventions/plugin-data-report-keying/(README + CHANGELOG, registered indocs/PLUGIN-PHILOSOPHY.md's registry — one row added, nothing reformatted). It separates the two failure modes that need different fixes:and states that a non-destructive history closes only the second. Rule 3 — never serve or derive from an artifact you cannot attribute — is the one that governs migration.
RKD-06 is carried as the convention's worked example, not filed as a defect.
bug-report:writekeys on the kebab-cased basename of the project root; the line already states the hazard and then picks a colliding key (two same-named checkouts share one directory). The originating item filed it explicitly as "context, not a defect to fix", and that is honored: it appears in rule 1c and in the adoption table, and nobug-reportfile is touched.The adoption table also records two things I checked rather than assumed:
claude-config:unhobblesolves the same problem a different way (basename as a label, canonical checkout identity recorded in the manifest and verified before every phase — acceptable because its artifact is never served), andmachine-health:auditis a deliberate non-adopter whose roots are passed in by the caller for a hazard that skill documents.machine-healthis cited for retention shape only.Test plan
New suite, 23 cases, all green:
Executed against this checkout, not only fixtures:
Repo gates:
All
check-skill.shwarnings are pre-existing on those skills, not introduced here.No behavioral test applies to the prose half (the four keyed sites and the convention) — those are model-facing instructions. They are covered by three eval cases instead, listed below, which is the mechanism this repo uses for that layer.
Security review note
lib/state-key.shis a new read-only surface: it runsgit remote,git config --get,git rev-parse --show-toplevel, and a hash. No write, no network, no execution of anything it reads. It adds no permission rule, noallowed-toolsentry, and no hook.The one security-relevant property is deliberate and tested: its output becomes directory components in a caller's path, so an attacker-influenced remote URL is a path-traversal vector. The identity is accepted only as lowercase segments of
[a-z0-9._-]each starting alphanumeric, and anything else is replaced by a sha256 prefix — deterministic, and inside the namespace. Cases 6, 7, 8 and 10 assert that no.., no backslash, and no absolute or space-bearing form survives into a key. This hardening is inherited from #2250, where an unvalidated version of the same derivation was caught in review normalizing a report path outside its skill's namespace.Trust-surface direction is narrowing, not widening:
claude-memory:auditpreviously read and acted on a machine-global file it could not attribute to a project, and now refuses to.Bumps:
claude-config→ 0.35.0 (newlib/, behavior change),claude-memory→ 0.9.0 (behavior change including the migration removal).docs/conventions/carries its own contract version (1.0.0) and no plugin bump.Test plan — eval coverage added
The keying property had zero eval coverage in either plugin.
claude-memory:audit—two-repos-do-not-share-one-report,legacy-unkeyed-report-is-not-served-or-adopted, andreport-without-prior-auditrewritten to assert the per-project derived path rather than "the most recent saved audit".claude-config:audit-instructions—second-project-neither-overwrites-nor-borrows-a-delta, which pins the delta-omission as well as the path.Related
Closes #2276
Closes #2277
Closes #2278
Inbox item:
2026-08-10-claude-config-report-keying-and-dispatch(RKD-01, RKD-03, RKD-04, RKD-05, RKD-06), plus20260811-020411-claude-config-audit-pass-report-path-inside-scan-set§ F7, which contributes the datum that made this an inconsistency inside one plugin rather than a fleet-wide omission.Ledger:
.work/handoff-inbox-batch-4/ledgers/I6-report-keying-dispatch.md·.../I8-audit-pass-report-path.md§ F7.Adjacent, not duplicated: #2250 (closed) is the same defect on
audit-prompting-posturesand the template followed here. #2229 / #2230 (closed) coveredaudit-pass's containment defect — containment within a run and collision across runs are different defects. #1568 owns the substitution-scope question this fix routes around rather than assumes. #1182 is hook-config-delivery adoption tracking, unrelated.