fix(claude-config-validator): match the skill's documented behavior to its tool grants - #199
Open
withinfocus wants to merge 16 commits into
Open
fix(claude-config-validator): match the skill's documented behavior to its tool grants#199withinfocus wants to merge 16 commits into
withinfocus wants to merge 16 commits into
Conversation
…tch its grants The skill's allowed-tools is Read, Grep, Glob, but its output section made inline pull request comments the required mode and exempted only the two commands, so the default path could not be carried out on a direct invocation. Findings are now per-issue text by default; comment posting and the single-document report contract are what an invoking context can supply. The cross-plugin secret-detection enrichment called Skill(detecting-secrets) without holding Skill. Conditioned on the grant rather than widening allowed-tools, since a validator that preaches least privilege should not grant itself the ability to invoke arbitrary skills to reach one optional enrichment. Narrow Bash(git fetch:*) to origin, which is all step 1 fetches, and record the shared-host caveat for the fixed /tmp report path.
…der the output cases The first pass left the same capability claim in five more places: the skill README's process step and Review Output Format section, and the plugin README's feedback heading and two output descriptions. Order the three output cases specific to general with a first-match rule. /validate-ai holds a comment-posting grant, so it matched two bullets in an unordered list.
Claude Code validationResult: Issues found Validated the CriticalNone. No hardcoded credentials, no Major
Minor
Security screening
Version bumpComplete and consistent at Checks run
Not run: |
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Documentation-only change to |
…n the routing path checklists/agents.md carried the retired instruction under its Process heading, and SKILL.md step 3 loads checklists into context, so a direct invocation still received it. The occurrences in checklists/prompts.md and under examples/ are sample content rather than reviewer instructions and are left alone. Finish the skill README: the always-first security list claimed the git-tracking check runs unconditionally and dropped the finish-remaining-checks half of the rule, the feature list and tree still advertised executable automation, and Requirements listed Git and Bash as the skill's rather than the helper's. Also fold in the duplicated priority-level block, which had drifted to contradict its own earlier copy, point Troubleshooting at /plugin list for plugin installs, drop an unsourced statistic, and repair a sentence that inverted its own subject. Thin the 1.2.2 changelog entries to one line each.
… in the output templates SKILL.md step 5 loads examples/ as the format template for the file type under review, so the retired instruction in example-prompts-review.md, the two other examples, and the prompts checklist exemplar still propagated. Reworded all four. Correct the shared-host caveat: an allow rule applies only when the symlink and its target both match, so Edit(//tmp/validation-summary.md) prompts rather than following a hostile symlink. World-readability is the real exposure. Mirror the git-tracking correction into the plugin README, replace SKILL.md's "mental checks" with the Grep mechanism the same step prescribes, and drop the remaining unsourced statistics from the checklists, examples, and plugin README.
…ment branch The branch carried "Create new comments rather than updating existing ones" verbatim from the inline mode this PR retires, and it contradicts the plugin's only comment-posting caller: /validate-ai upserts a single sticky comment keyed on its marker. Defer to the invoking context's convention instead. State the precedence ordering in the skill README too, which presented the three destinations as co-equal.
withinfocus
force-pushed
the
fix-config-validator-skill-grant-coherence
branch
from
August 17, 2026 21:18
cba668f to
130bb0e
Compare
…f posting them The comment-posting branch had no caller that wanted it. bitwarden-code-reviewer holds an inline-comment tool but tells the skill to "fold its findings into your own classification and validation", and the multi-agent config subagent expects findings per its Finding Shape schema. Posting directly would bypass both pipelines, plus the false-positive gate and the low-severity cap. Step 5 is now two cases: the validate-ai report contract, or return the findings as text. State the grant-composition rule once at the top. The file previously reasoned from the skill's own allowed-tools in one place and the caller's grants in another, which cannot both be the ceiling. Carry the settings.local.json qualification up into both Features lists and the frontmatter description, scope the CRITICAL CHECKS line to the pattern checks it actually covers, give the direct-invocation path a defined scope source, note that only origin is fetched, record that the Edit grant covers the Write tool, and finish the vocabulary rename across the examples and priority framework.
…ith step 5 Round-2 validation caught the skill README still documenting the three-case routing order, including the posting case, and citing the SKILL.md section that now forbids it. checklists/settings.md told the reviewer to stop the review on a security finding, reversing a fix 1.2.0 already shipped, and stated the settings.local.json check as a git-tracking fact ten lines before stating it correctly. Harmonize the wording everywhere on "appears in the changeset", and scope the /validate-ai description to what it does in CI.
…ound-3 validation The direct-invocation list still opened with detecting recently modified files, which needs a changed-files list the skill has no way to obtain. Check 1 of the security patterns reference still framed the settings.local.json check as git tracking with no changeset alternative, and two headings kept the old vocabulary.
… broken references The output format still ended in APPROVE / REQUEST CHANGES, which are GitHub review-event values from the posting mode this release retires, and which the report contract cannot consume. The verdict is now Pass or Issues found, with the caller mapping to its own vocabulary, updated across the README and four examples. Nine relative references were broken: six checklists cited SKILL.md as a sibling, and two examples cited reference/ and checklists/ as if they sat alongside. This is the plugin's own broken-file-reference finding class. Scope the marketplace use case to the component files the skill can review and point manifest checks at the command that delegates them. Align the skills pass list with the checklist's five passes, restate the strategy list as the five actual steps, add checklists/ to the disclosure criteria both commands pass to the skill reviewer, and finish the changeset wording in the settings and skills checklists. Also from the notes: hooks.md licensed abandoning the review on a finding, and a heading's globs were being eaten by markdown emphasis. Trim both command descriptions, which are what /help renders.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ep 2 The settings example kept a third verdict spelling, BLOCK, so the earlier sweep for APPROVE and REQUEST CHANGES missed it. All seven verdict lines now read Pass or Issues found, which is what the report contract consumes. Step 2 listed the same credential check twice and omitted the dangerous auto-approval check that both the README and the security reference advertise as always run. Step 5 stated the per-issue rule twice; the Per-Issue Rules block is the more complete of the two. Note that a changeset touching only skill support files lands in no component bucket, so the configuration and security row does not fire for it. Widening the pattern would put the reference out of step with the action it mirrors. State the 2.1.210 floor in the plugin README's Installation section, convert the new grant section to imperative bullets to match the rest of the file, and drop a step number that collided with this skill's own numbering.
…rity-scan.sh The skill-bucket gloss was written for plugin-hosted skills and inverted for a repo-level one: .claude/skills/foo/reference/bar.md matches the config bucket, so components-changed is true and the configuration row does fire, while changed-plugins is empty so plugin validation does not. A reviewer following it would have skipped the one review that applied. Qualified both paths, mirroring the hook-files bullet. security-scan.sh had two silent-pass paths and one false positive. git ls-files ran against whatever repository the shell was in, and outside a repository the error was swallowed and the check printed a pass it never performed; it now runs with -C and reports a non-repository as skipped. grep -v "example" discarded any line containing that substring, including real credentials; the anchored path exclusions already cover the fixtures. checklists/ was missing from those exclusions, so the script reported the plugin's own teaching material as a critical finding. Verified all three by running it before and after. security-patterns.md embedded a copy of the script that had drifted 134 lines from the shipped file, including a different directory resolution that would silently find nothing; it now points at the file. Also finish the verdict and finish-remaining-checks sweeps, document allowed-tools in the frontmatter reference, bridge the two meanings of a changed skill, and record the omitted changes in the changelog.
withinfocus
force-pushed
the
fix-config-validator-skill-grant-coherence
branch
from
August 18, 2026 16:25
c2fa2fd to
d7f1b14
Compare
…n check 1 Under pipefail, `git ls-files | grep -q` fails when grep matches early: grep exits, git dies of SIGPIPE with 141, and pipefail makes that the pipeline's status, so the branch went false and the script printed "OK: settings.local.json not in git" for a file that was committed. Reproduced at 20k tracked files with a match that sorts early; a small .claude/ never shows it. Ask git directly instead of piping. Carry the skipped state into the summary. The branch added in 6017574 left ISSUES_FOUND at 0, so a non-repository still printed "All security checks passed" and "No committed local settings" for a check that never ran.
withinfocus
force-pushed
the
fix-config-validator-skill-grant-coherence
branch
from
August 18, 2026 16:36
d7f1b14 to
e0f8912
Compare
withinfocus
changed the title
Pass and Issues found were defined as the two values without saying which findings flip one to the other, so two reviewers with identical findings could return opposite verdicts and a SUGGESTED-only review was ambiguous.
…t of scope Those fixes belong in a change where shell behaviour is the subject and can carry regression coverage. Three consecutive review rounds found the same false-negative class in that file, each time in code a previous fix was meant to close, and the script has no tests. They are preserved on fix-security-scan-false-negatives. Restore the troubleshooting note that described the reverted exclusions, and drop the changelog bullet that claimed them. The security-patterns.md changes stay: pointing at the shipped script instead of a copy that had drifted 134 lines is true regardless of what that script contains. Attribute the report to the caller in the security-scan step, the last place the skill still spoke as though it owned one.
withinfocus
changed the title
…own edits The reworded settings.local.json check fired CRITICAL on any changeset where the file appeared, including the one that deletes it, which is the remediation the same section prescribes. Scope it to additions and modifications. The enrichment section said a direct invocation "cannot invoke another skill", which contradicts the allowed-tools note added to the requirements reference in this same change: the field pre-approves rather than restricts. The call prompts, which makes it unavailable in a non-interactive run, and that is what matters. The settings example closed by counting three CRITICAL findings where it has two CRITICAL and one IMPORTANT.
… severity The CRITICAL heading covered all three bullets, one of which maps to the IMPORTANT finding at line 62, so the section claimed three CRITICAL findings twelve lines above a closing line that correctly says two. Regrouped on the shape the skill example already uses, one sub-heading per severity, which also folds the separate risk list into the bullets it describes.
withinfocus
marked this pull request as ready for review
August 18, 2026 20:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
Follow-up to #198, which deferred these findings.
📔 Objective
Documentation only. Fixes places where
reviewing-claude-configtold a reviewer to do something itsRead, Grep, Globgrant cannot deliver.The skill required inline pull request comments it cannot post. It now returns per-issue findings for the caller to route, which is what both real callers already expect:
bitwarden-code-reviewerfolds them into its own classification, and the multi-agent config subagent wants them per its Finding Shape schema. Thevalidate-aireport contract is the one alternative. The same instruction lived in the checklists and examples thatSKILL.mdloads as templates, so those match now, as does the verdict vocabulary, which used GitHub review-event values the report contract cannot consume. The verdict also has a threshold: any CRITICAL or IMPORTANT finding makes itIssues found.The skill reasoned from its own
allowed-toolsin one place and the caller's grants in another. One rule sits at the top now and the rest derives from it, including theSkill(detecting-secrets)enrichment, which is conditioned on the grant rather than wideningallowed-tools.The
settings.local.jsoncheck needs a changed-files list that only the two commands supply, and every surface says so now, including both Features lists and the frontmatter description. Ten broken relative references are repaired,security-patterns.mdpoints at the shippedsecurity-scan.shinstead of a copy that had drifted 134 lines from it, and the marketplace use case is scoped to what the skill can actually review.Two command-level items:
Bash(git fetch:*)narrows toBash(git fetch origin:*), which is all step 1 runs, and/validate-ai's README records that its fixed/tmpreport path is world-readable on a shared host.Version
1.2.1 to 1.2.2, PATCH.
Not included
security-scan.shhas three false-negative paths, including apipefail/SIGPIPE one that makes the committed-settings.local.jsoncheck report a pass under load. Fixes are ready onfix-security-scan-false-negativesand will land separately, where shell behaviour is the subject and can carry regression coverage. They are pre-existing onmain, so nothing here regresses by waiting.Also left alone:
reference/claude-code-requirements.md:77presents a closed tool-name list thatchecklists/agents.md:55enforces as CRITICAL, so it misreports any config grantingTask,Skill, orNotebookEdit, including this plugin's own commands. And whether per-commandREADME.mdfiles register as descriptionless commands is a repo-wide question rather than one local to this plugin.