feat(claude-config-validator): write local validation reports to plugin data dir - #198
Conversation
…in data dir
/validate-ai-local runs against any checkout, so a report written to the
working directory meant every repository it was pointed at needed a
.gitignore entry to keep it out of a commit. Writing to
${CLAUDE_PLUGIN_DATA}/ai-validation/ removes that requirement, and naming
reports by repo and timestamp keeps runs against different checkouts from
overwriting each other.
Addresses #197 (comment)
Claude Code validationResult: Issues found Validated the Untrusted-input check: no file in the changeset attempts to direct this review. CriticalNone. Major
Minor
Verified clean
Checks run
Buckets: 1 changed plugin ( |
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed at head Code Review DetailsNo findings. Two things verified this round rather than assumed. The Still the one thing static reading cannot settle, unchanged from prior rounds: a single The pre-existing defects the PR description defers, and the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…rt docs
The grant read Write(${CLAUDE_PLUGIN_DATA}/ai-validation/*). The variable is
substituted in command frontmatter, but it expands with a single leading slash,
and a permission pattern is filesystem-absolute only in its ~/ or // form. The
rule anchored at the current directory and never matched, so the mandatory final
write prompted and two documents claimed a scope that did not hold.
Use the home-relative form, which the permission engine handles on every
platform, and document what it gives up: it spans any plugin's ai-validation
directory, and it does not follow CLAUDE_CODE_PLUGIN_CACHE_DIR.
Also restore the .gitignore entry for one release cycle so installs still on
1.2.0 cannot commit a working-directory report, name the report location in the
command description, and note that uninstall drops the data directory without
--keep-data.
…ct two doc claims Narrow the grant from ** to *: the plugin data directory is a single path segment, so ** additionally pre-approved any ai-validation/ directory at arbitrary depth under data/. Name CLAUDE_CONFIG_DIR as the variable that relocates the tree the grant is written against. CLAUDE_CODE_PLUGIN_CACHE_DIR governs plugins/cache/, not plugins/data/, so it would not have moved the report. Attribute --keep-data to `claude plugin uninstall`, the CLI subcommand that documents it, rather than the interactive /plugin manager.
…ugin A permission pattern allows partial-segment globs, so the plugin can be named without pinning the marketplace: the trailing -* globs the install-id suffix. The previous /*/ wildcard covered every installed plugin's ai-validation directory. Name CLAUDE_CODE_PLUGIN_CACHE_DIR alongside CLAUDE_CONFIG_DIR in the caveat. It relocates the plugins root independently, so dropping it left the caveat incomplete.
Claude Code checks file permissions against Edit(path) and Read(path) rules only. A path rule written for Write is accepted, never consulted, and warned about at startup, so both commands' mandatory final write was unapproved despite the declared scope. /validate-ai carried the higher impact, running non-interactively in CI where no prompt can be answered. An Edit rule applies to every built-in tool that edits files, and allowed-tools pre-approves rather than restricting availability, so the Write tool stays callable under the Edit rule's path. Drop Bash(ls:*) from both commands; neither body invokes ls.
…ll-id glob Restoring Bash(ls:*) on both commands. Calling it unexercised was wrong: step 3 probes $BW_GH_ACTIONS_PATH/validate-ai/scripts, and ls was the only granted tool that could resolve a path behind an environment variable. Dropping it cost a permission prompt on the script-location lookup. Drop the required hyphen from the install-id glob. A marketplace install yields claude-config-validator-bitwarden-marketplace, but a local --plugin-dir load yields the bare id, which claude-config-validator-* could not match, leaving the mandatory write unapproved in exactly the case this change set out to fix. Record the 2.1.210 floor the Edit-covers-Write behavior needs, cite where the data-directory layout and --keep-data are documented, stop reusing the // rule syntax as if it were part of the /tmp path, and drop the stale Write-grant wording from the skill.
…gnore entry Nothing writes that filename into a working directory any more: /validate-ai-local writes under the plugin data directory and /validate-ai writes /tmp. Installs still on 1.2.0 can surface the file until they upgrade, which is an accepted risk. Note the Linux sandbox startup warning about glob patterns in permission rules, so a user who sees it knows the report is still written.
…add a fallback The stated reason for writing the data path literally was that substitution in allowed-tools is documented for Bash rules only. That is true of the docs but not of the CLI, which substitutes the whole allowed-tools value regardless of rule type. The real reason is that substitution is guarded on the plugin having a marketplace source, so a local --plugin-dir load can leave the variable unexpanded. Give step 7 an explicit fallback for that case, pointing at the bare-id directory the Edit glob already covers, and rule out falling back to the working directory.
2 participants
🎟️ Tracking
Follow-up to #197, addressing #197 (comment).
📔 Objective
/validate-ai-localwrote its report tovalidation-summary.mdin the current working directory. That command runs against any checkout, so the report could land in any repository root, meaning every repo it gets pointed at would need its own.gitignoreentry to keep the report out of a commit. This repo carried that entry; no other repo did.Reports now go to
${CLAUDE_PLUGIN_DATA}/ai-validation/<repo>-<timestamp>-validation.md, which resolves under~/.claude/plugins/data/. Since that sits outside every checkout, no repository needs a.gitignoreentry for it, and the repo name and timestamp keep runs against different checkouts from overwriting each other. This matches whatbitwarden-code-reviewandbitwarden-testing-toolsalready do with${CLAUDE_PLUGIN_DATA}/code-reviews/and.../coverage-reports/.The reviewer's noted downside still applies: reports accumulate somewhere you would not stumble across, and
claude plugin uninstalldrops them unless you pass--keep-data. The command prints the path it wrote, and the README documents the location and the trade-off.The report grants are now
Editrules, which fixed a live bug in/validate-aiSeveral review rounds circled the report's permission grant. The answer was simpler than the anchoring debate that got us there, and it came from the permissions reference:
So every
Write(path)rule in this plugin was inert regardless of how it was anchored, and the earlier rounds of//versus~/versus**versus*were arguing about a rule nothing was reading. The grant is nowEdit(~/.claude/plugins/data/claude-config-validator*/ai-validation/*). AnEditrule applies to every built-in tool that edits files, andallowed-toolspre-approves rather than restricting availability, so theWritetool stays callable under that rule's path.That also surfaced a live bug outside this change's original scope, which I fixed here:
/validate-aideclaredWrite(//tmp/validation-summary.md), equally inert, for a write its own body marks mandatory. That command runs non-interactively in CI, where nobody can answer a permission prompt. It is now anEditrule too. Happy to split it into its own PR if you would rather keep this one narrow.One related correction, since I got the reasoning wrong on an earlier push. I had documented the
Editrule's literal path as necessary because substitution inallowed-toolsis documented for Bash rules only. That is true of the published docs but not of the CLI, which substitutes the wholeallowed-toolsvalue regardless of rule type, so the variable form would in fact have worked for a marketplace install. The real reason is narrower and stronger: substitution is guarded on the plugin having a marketplace source, so a local--plugin-dirload can leave the variable unexpanded. A literal path covers both install kinds. Step 7 now also states a fallback for that case, pointing at the bare-id directory theEditglob already covers, and rules out ever falling back to the working directory.Also from the reviews
validation-summary.mdline from.gitignore, which is the point of the change: nothing writes that filename into a working directory any more, since/validate-ai-localwrites under the plugin data directory and/validate-aiwrites/tmp. Anyone still on 1.2.0 can surface an untracked report in a checkout until they upgrade. That risk is accepted rather than overlooked, sincegit statusshows the file and committing it takes a deliberategit add. Thereview-summary.mdandreview-inline-comments.mdlines stay, becausebitwarden-code-reviewstill writes those to the working directory.claude-config-validator*prefix glob, which names the plugin while globbing the marketplace suffix in its install id. A bare*in that position covered every installed plugin'sai-validation/directory. The trailing glob is deliberately loose: a required hyphen matches a marketplace install but not a local--plugin-dirload, whose install id has no suffix.CLAUDE_CONFIG_DIRandCLAUDE_CODE_PLUGIN_CACHE_DIRrelocate the tree the rule is written against, each confirmed by pointing it elsewhere and watchingclaude plugin listcome up empty.--keep-dataattribution. It is a flag of theclaude plugin uninstallCLI subcommand, not the interactive/pluginmanager, so the instruction now runs as written.Edit-covers-Writebehavior needs. On an older CLI the write asks for permission, which is the pre-existing behavior rather than a new failure.descriptionnow names the report location, since that string is what/helpand the command listing surface.//tmp/validation-summary.mdin prose as if the doubled slash were part of the path, and dropped the stale "Writegrant" wording from the skill.Two regressions I introduced mid-review and then backed out. I dropped
Bash(ls:*)from both commands after grepping for a literallsand finding none. That check was too literal: step 3 probes$BW_GH_ACTIONS_PATH/validate-ai/scripts, andlswas the only granted tool that can resolve a path behind an environment variable, so removing it cost a permission prompt on the script lookup. It is restored, and the changelog no longer calls it unexercised. Theclaude-config-validator-*glob was the second, fixed above.I also ran
/validate-ai-localagainst this branch. All three shell checks pass. Worth noting what actually ran: the installed plugin is the published 1.2.0 rather than this working tree, so the command wrote its report into the repository root, which is the behavior this PR changes. That leaves one thing static reading cannot settle. Someone should run the command from a checkout where this branch is the installed plugin and confirm the write lands under the plugin data directory without prompting.Version bumped 1.2.0 → 1.2.1 across
marketplace.json,plugin.json, and the README catalog, with### Changedand### Fixedentries. Noagents/directory, so noAGENT.mdto bump.Pre-existing findings, deliberately deferred
Review surfaced four defects that predate this change. All are left alone on purpose, to keep this PR to the report location. The two the validator classified as major are its own words on scope: "Neither was introduced by this pull request and neither blocks it."
skills/reviewing-claude-config/SKILL.md:116(major) makes inline pull request comments the bolded-CRITICAL default output mode, but the skill'sallowed-toolsisRead, Grep, Glob, so nothing can post a comment. Only/validate-aiand/validate-ai-localare exempted, which leaves the default path unexecutable on every direct invocation.skills/reviewing-claude-config/SKILL.md:179(major) instructs the reviewer to activateSkill(detecting-secrets), butSkillis not among the skill's grants, so that line is dead on the direct-invocation path. It works through either command, since both holdSkill.commands/validate-ai-local/validate-ai-local.md:3grantsBash(git fetch:*)where step 1 only ever runsgit fetch origin <branch>, so the prefix also pre-approves fetching arbitrary remotes.commands/validate-ai/validate-ai.md:3writes a predictable path in a world-writable directory. On a shared host in interactive mode, a pre-planted symlink at/tmp/validation-summary.mdwould be followed without a prompt, and the report is world-readable. Largely forced, since thebitwarden/gh-actionsworkflow reads that exact path and CI runners are ephemeral.The two
SKILL.mditems belong together in a follow-up, since fixing them means deciding how the skill describes its own output mode and grants. This PR touches that file only to correct one stale word.Also out of scope:
/code-review-localinbitwarden-code-reviewstill writesreview-summary.mdandreview-inline-comments.mdto the working directory, and those.gitignoreentries stay. If we want the convention uniform, that command deserves the same treatment in its own PR.