Skip to content

feat(claude-config-audit): add permission-hygiene grant audit + convention - #175

Merged
kyle-sexton merged 10 commits into
mainfrom
feat/permission-rule-hygiene-audit
Jul 14, 2026
Merged

feat(claude-config-audit): add permission-hygiene grant audit + convention#175
kyle-sexton merged 10 commits into
mainfrom
feat/permission-rule-hygiene-audit

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

What

Adds a durable guardrail against fragile Claude Code permission grants, in two parts:

  • (a) Enforceable check — a new permission-hygiene skill in the claude-config-audit plugin,
    matching the plugin's established idiom (deterministic detector script + .test.sh contract test +
    reference/criteria.md + evals/evals.json). A permission-rule-check.sh detector scans
    skill/command/agent frontmatter allowed-tools and settings.json / settings.local.json
    permissions.allow, and flags three anti-patterns.
  • (b) Convention docdocs/conventions/permission-rule-hygiene/ (README + CHANGELOG), matching
    the existing hook-telemetry / ecosystem-commands convention-directory house style, stating the
    principle, the three anti-patterns, the correct pattern, and the operator-setup boundary — each with
    official-doc citations. The skill's criteria link to it (reference, don't restate).

The three anti-patterns → the correct pattern

All three make a grant silently do nothing. Verified against current official docs (URLs below):

  1. Interpreter-wildcard / blanket allow rules are dropped in auto mode. Per permission-modes:
    "On entering auto mode, broad allow rules that grant arbitrary code execution are dropped: Blanket
    Bash(*) or PowerShell(*); Wildcarded interpreters like Bash(python*); Package-manager run
    commands; Agent allow rules. Narrow rules like Bash(npm test) carry over." So a frontmatter
    grant such as Bash(python "*helper.py":*) grants nothing under auto mode. Empirically, a guarded
    merge helper granted this way was denied even when invoked bare.
  2. Hardcoded absolute machine/user paths. Bash rules match the command string literally — no
    ~/$HOME/env expansion — so Bash(/c/Users/<name>/.../x.sh:*) breaks on other machines/usernames
    and leaks a username into source control.
  3. Assuming a skill or plugin can self-grant. Skill allowed-tools is skill-scoped and (per chore: initialize marketplace scaffold #1)
    ineffective for auto-mode-gated actions; a plugin settings.json supports only the agent and
    subagentStatusLine keys, so a permissions block there is inert; and an agent editing its own
    settings to self-grant is blocked (.claude/ is a protected path; defaultMode: auto is ignored
    from project/local settings so a repo can't grant itself auto mode).

Correct pattern: expose the guarded helper as a stable bare command on the Bash tool PATH
(pre-plugin: a PATH shim in a dir already on PATH; post-migration: the plugin's bin/), allow the bare
name narrowly (Bash(babysit_merge.sh:*) — carries over into auto mode like Bash(npm test), machine
-independent, identical before/after migration), and have the operator add that bare-name rule once
to user-global ~/.claude/settings.json.

The check (detector → criteria → evals)

permission-rule-check.sh (advisory, exits 0; --count for a count; requires jq) flags:

  • P1 interpreter-wildcard / blanket rules (Bash(*), Bash(python*), Bash(bash <path>*),
    Bash(sh -c*), package-manager runners, Bash(*.py:*)). Narrow rules (Bash(npm test),
    Bash(babysit_merge.sh:*)) are NOT flagged — a negative fixture proves this.
  • P2 hardcoded machine/user paths (/c/Users/…, /home/…, /Users/…, C:\Users\…);
    ${CLAUDE_PROJECT_DIR}/~/ forms are exempt.
  • P3 a plugin settings.json that declares an inert permissions block.

settings.local.json is parsed for its permissions.allow array only — never read or echoed wholesale
(matching the sibling settings-audit secret-handling posture). Scope vs settings-audit is explicit:
this skill owns grant portability + auto-mode durability + who adds the operative rule; file
correctness (baseline deny/ask, deprecated :*, drift) routes to settings-audit.

Detector and test fixtures assemble machine-path strings from fragments (and the docs use <name>
placeholders) so no contiguous machine-path literal sits in a committed file — the repo's own
machine-specific-path CI lane stays clean without a CI exclude.

Empirical evidence the guardrail is needed

Running the new detector against this marketplace surfaced six pre-existing interpreter/runner-led
frontmatter grants
across unrelated plugins (shapes like Bash(bash <script>:*), Bash(bash <dir>/*),
and Bash(npx:*)) — none of them the portable bare-name pattern, and the broad forms among them (a
globbed script target, a package runner) are exactly what auto mode drops. (For a grant that invokes
one fixed script through an interpreter, the detector flags the same authoring anti-pattern without
asserting the drop; the bare-name fix is identical.) Those live in other contributors' plugins and are
left for a separate follow-up (out of this PR's lane).

Tests

  • permission-rule-check.test.sh: 27 checks pass (positive + negative fixtures for all three checks,
    --count, --help, jq-missing → exit 2, and the narrow-rule-not-flagged negative case).
  • Full scripts/run-plugin-tests.sh suite green; shellcheck -x and shfmt -ci -i 2 clean;
    markdownlint clean; scripts carry the executable bit.

Sources (verified this session)

🤖 Generated with Claude Code

https://claude.ai/code/session_013gKgRoW8zkFadbE2hE4r4b


Note

Low Risk
Changes are documentation plus read-only local scanning scripts; they do not alter Claude Code behavior or auto-modify user settings.

Overview
Adds a permission-rule-hygiene marketplace convention (docs/conventions/permission-rule-hygiene/) and a fourth claude-config-audit skill, permission-hygiene, that audits whether allowed-tools and permissions.allow grants are portable and still effective in auto mode.

The skill runs a deterministic permission-rule-check.sh detector (plus permission-rule-check.test.sh, reference/criteria.md, and evals/evals.json) that flags P1 interpreter/blanket/Agent grants auto mode drops, P2 hardcoded user-home paths, and P3 inert permissions blocks in plugin settings.json. It is report-only (remediation is operator manual: bare command on PATH + user-global allow rule), with scope filters and explicit routing of file-correctness work to settings-audit.

Plugin manifest and docs move claude-config-audit to 0.3.0 and update the root catalog to describe four audit skills.

Reviewed by Cursor Bugbot for commit 867e5f2. Bugbot is set up for automated code reviews on this repo. Configure here.

…ntion

Add a durable guardrail against fragile Claude Code permission grants.

Enforceable check: a new `permission-hygiene` skill whose deterministic
`permission-rule-check.sh` detector scans skill/command/agent frontmatter
`allowed-tools` and settings.json/settings.local.json `permissions.allow`,
flagging three anti-patterns and recommending the bare-command-on-PATH pattern:

- P1 interpreter-wildcard / blanket allow rules that Claude Code drops on
  entering auto mode (Bash(*), Bash(python*), Bash(bash <path>*), package-manager
  runners, Bash(*.py:*)); narrow rules like Bash(npm test) carry over and are
  not flagged.
- P2 hardcoded absolute machine/user paths (Bash rules match literally with no
  ~/$HOME/env expansion).
- P3 a plugin settings.json that declares an inert `permissions` block (only
  agent/subagentStatusLine are supported), so the operative rule must be added
  by the operator to user-global ~/.claude/settings.json.

Ships with a 27-check contract test (positive + negative fixtures, including the
narrow-rule-not-flagged case), reference/criteria.md, and evals/evals.json.
settings.local.json is parsed for permissions.allow only, never echoed. Scope vs
settings-audit (file correctness) is explicit and routes out.

Convention: docs/conventions/permission-rule-hygiene (README + CHANGELOG) states
the principle, the three anti-patterns, the correct bare-name+PATH / plugin-bin
pattern, and the operator-setup boundary, each with official-doc citations; the
skill's criteria link to it rather than restating.

Detector and fixtures assemble machine-path strings from fragments and the docs
use <name> placeholders so no contiguous machine-path literal sits in a committed
file, keeping the repo's own machine-specific-path lane clean.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_c061249e-4bd3-400e-a4a5-08d9699183ce)

@cursor cursor 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.

Stale comment

Risk: low. Not approving because Cursor Bugbot completed with a skipped/neutral status and left no clean review comment. Human review is needed before merge; no reviewers were assigned because the repo has no non-author maintainers available.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@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: 9e38fd7eeb

ℹ️ 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".

… gate

permission-rule-check now detects a bare Bash/PowerShell grant in skill/agent
frontmatter allowed-tools, not only in settings.allow. criteria.md P1 already
lists bare Bash/PowerShell, but the frontmatter path ran only the parenthesized
P1 ERE, so a bare `allowed-tools: Bash` was missed. A single scan_bare_tool
helper now serves both the frontmatter and settings.allow paths.

Also clears two hygiene checks introduced by this branch:
- typos: drop "invokable" from the plugins-reference quote (repo standard is
  "invocable"); the ellipsis keeps the abridged citation without a misquote.
- machine-specific-paths: reword the P2 comment so no contiguous home-path
  literal trips the repo's own scanner.
@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_94b8efce-f722-43a4-9a30-74a067099b47)

@cursor cursor 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.

Stale comment

Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached) and did not produce a clean review. Human review is needed before merge; no reviewers assigned because no non-author maintainers are available in this repo.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@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: bdb2b424c5

ℹ️ 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-audit/skills/permission-hygiene/SKILL.md
permission-rule-check now flags `Agent` and scoped `Agent(...)` allow rules in
permissions.allow and skill/agent frontmatter allowed-tools. Claude Code drops
all Agent allow rules on entering auto mode — like blanket/wildcarded
interpreters — but unlike a shell helper they have no bare-command-on-PATH
analog, so the finding recommends removing/re-scoping the rule or running
outside auto mode.

A dedicated scan_agent helper covers both surfaces; a scoped Agent(...) is not
a narrow carry-over (no other detector reaches it), so both bare and
parenthesized forms flag. Adds regression cases (settings + frontmatter, bare
and scoped) and an evals case; documents the shape in the convention and P1
criteria.
@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_461a8651-2870-468a-8e51-d6d2d05a115e)

@cursor cursor 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.

Stale comment

Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached) and did not produce a clean review. Human review is needed before merge; no reviewers assigned because no non-author maintainers are available in this repo.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@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: eebecce449

ℹ️ 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".

…oken

scan_agent scanned the whole rule string for the word `Agent`, so a valid shell
grant whose payload merely contains it — `Bash(echo Agent)`, `Bash(find
*Agent*)` — was falsely reported as an Agent allow rule. Split the text into
top-level `Tool` / `Tool(...)` tokens first (the greedy parenthesized group
swallows a tool's whole payload, so an inner `Agent` never surfaces as its own
token) and flag only a token that is `Agent` or begins `Agent(`.

Adds a regression case asserting `Agent` inside a Bash payload is not flagged.
@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_1811f94e-e946-41ce-90b7-dba9bf7c2b8d)

@cursor cursor 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.

Stale comment

Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached) and did not produce a clean review. Human review is needed before merge; no reviewers assigned because no non-author maintainers are available in this repo.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@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: 1c403c00e8

ℹ️ 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".

…vendored skills

Two permission-hygiene detector refinements:

P1 now flags bare package-manager wildcard grants — Bash(npm:*), Bash(npm *),
Bash(pnpm:*), Bash(yarn:*). A bare package manager behind a wildcard permits
arbitrary execution (npm exec, npm run <anything>, lifecycle scripts), so it is
interpreter/runner-led rather than the durable bare-name pattern and auto mode
drops it. Folding npm|pnpm|yarn into the runner alternation subsumes the former
npm run|pnpm run|yarn run entries (a bare name matches its own run wildcard).
Fixed subcommands — Bash(npm test), Bash(npm run build) — carry no wildcard,
carry over into auto mode, and are not flagged.

The frontmatter scan now skips any SKILL.md/agent/command under a vendor/ path
segment. A vendored file is an upstream reference, not a loadable skill, so its
allowed-tools never take effect; flagging one (e.g. the playwright plugin's
vendored SKILL.md granting Bash(npx:*)/Bash(npm:*)) was a false positive.

Adds regression cases for both (flagged vs. carried-over package-manager forms;
real-vs-vendored SKILL.md), an evals case, and criteria/convention notes.
@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_d8105b53-a640-429a-9ecc-36386271af23)

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

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


PR Review — feat(claude-config-audit): permission-hygiene grant audit + convention

  • Gather context and read changed files
  • Review detector script (permission-rule-check.sh)
  • Review test suite (permission-rule-check.test.sh)
  • Review SKILL.md, criteria.md, evals.json
  • Review convention docs
  • Review manifest / plugin.json
  • Synthesize findings and post review

Summary

This PR adds a well-structured fourth skill to claude-config-audit. The design is sound, test coverage is thorough, and the issues raised across prior Codex reviews have been iteratively addressed across the 5 commits. The detector is advisory-only (always exits 0), scoped correctly against the sibling settings-audit skill, and handles settings.local.json safely. Below are my findings, ordered by severity.


Findings

1. No test for settings.local.json path

The test suite exercises scan_settings_allow only via .claude/settings.json fixtures. scan_settings_allow is called for both files (lines 226–227 of the detector), but there is no regression case that places fragile grants in .claude/settings.local.json and confirms they are caught. A P1/P2 fixture in settings.local.json would close this gap.

permission-rule-check.test.sh — no .local.json case between Case 8 and 9.
Fix this →

2. Convention cross-reference is a dead link in installed-from-marketplace cache (acknowledged)

SKILL.md:18 and criteria.md:9 reference ../../../../docs/conventions/permission-rule-hygiene/README.md and ../../../../../docs/conventions/... respectively — paths outside the plugin root that don't exist in a marketplace-cache install. The mechanical audit (detector + all check logic) is fully self-contained inside the plugin, so the audit outcome is unaffected. Only the "why this convention exists" prose becomes inaccessible post-install.

The author acknowledged this tradeoff on the Codex thread — the convention is intentionally marketplace-wide, not per-plugin. Flagging for awareness; not a blocker.

3. Open Codex comments — resolved in latest commit ✓

Both open items from the Codex review at commit 1c403c0 are resolved in 5b57cbb:

  • Vendor/SKILL.md false positives! -path '*/vendor/*' in the find command (line 208) correctly excludes vendored upstream SKILL.md copies (e.g. the Playwright plugin's vendored reference) while flagging the real loadable skill. Case 6b in the test suite covers this with a positive + two negative fixture assertions.

  • Bare package-manager wildcardsnpm|pnpm|yarn added to _runner (line 93) correctly flags Bash(npm:*), Bash(npm *), Bash(pnpm:*), Bash(yarn:*) as P1. The regex requires a * so fixed subcommands like Bash(npm test) and Bash(yarn build) are not matched. Case 2 and Case 3 in the test suite verify both sides.


Correctness spot-checks

P1 regex trace — key cases:

Rule Expected Verified
Bash(*) flagged (alt 1)
Bash(python*) flagged (alt 2, _interp)
Bash(sh -c*) flagged (alt 2, sh in _interp, -c as non-alnum prefix)
Bash(npm:*) flagged (alt 3, npm in _runner, : as non-alnum)
Bash(npm run *) flagged (alt 3, npm matches, run as non-alnum, *)
Bash(npm test) not flagged (no *)
Bash(npm run build) not flagged (no *)
Bash(git commit *) not flagged (git not in _interp/_runner)
Bash(*.py:*) flagged (alt 4, leading *)

Agent anchoring (scan_agent — fixed in 1c403c0): The token-split regex [A-Za-z_][A-Za-z0-9_]*(\([^)]*\))? consumes the full parenthesized payload as a single token, so Bash(echo Agent) produces only the token Bash(echo Agent)Agent is never a standalone token. Case 3 fixture with Bash(echo Agent) / Bash(find *Agent*) confirms no P1 Agent finding.

settings.local.json security posture: scan_settings_allow reads only .permissions.allow[] via jq — the file is never echoed wholesale. Double tr -d '\r' (once before jq-e validation, once around jq output) is belt-and-suspenders for Windows CRLF. ✓

P3 plugin self-grant detection: jq -e 'has("permissions")' correctly targets the top-level permissions key in a plugin settings.json. Case 7 confirms the agent-only plugin settings is not flagged. ✓


Minor notes

  • _runner includes pipx run but not bare pipx or bare uv; Bash(uv:*) would not be flagged. Not a material gap for current usage patterns but worth a note for future extension.
  • The evals (7 cases) cover all three check types plus the scope-boundary routing and the report-only constraint. Good eval quality.
  • --count mode is correctly plumbed from the advisory exit-0 contract through to the test (Cases 2b, 3, 4, 5, 6b). The jq-missing exit-2 path is tested (Case 9).
  • Version bump 0.2.0 → 0.3.0 in plugin.json is correct for a new skill addition.

Verdict

Approve with one non-blocking note. The one actionable gap (no settings.local.json test) is low-risk — the production code path for .local.json is identical to the .json path, and the existing tests exercise the shared scan_settings_allow function thoroughly. The convention-doc placement outside the plugin root is an accepted structural tradeoff. The core detection logic and test suite are solid.

@cursor cursor 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.

Stale comment

Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached) and did not produce a clean review. Human review is needed before merge; no reviewers assigned because no non-author maintainers are available in this repo.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@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: 5b57cbb6ba

ℹ️ 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".

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_8c8d8908-0637-4c27-89d4-43bfbb7d29fd)

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 40s —— View job


PR Review — feat(claude-config-audit): permission-hygiene grant audit + convention

  • Gather PR context and previous review history
  • Read detector script (permission-rule-check.sh)
  • Read test suite (permission-rule-check.test.sh)
  • Read SKILL.md, criteria.md, evals.json
  • Read convention docs and manifest
  • Synthesize findings and post review

Summary

This PR adds a well-structured, advisory-only fourth skill to claude-config-audit. The iterative review cycle (5 commits addressing Agent anchoring, bare frontmatter grants, vendor exclusion, and bare package-manager wildcards) has produced a solid implementation. One open correctness issue from the latest Codex review (commit 5b57cbb) remains unaddressed; the rest of the design is sound.


Findings

1. scan_bare_tool false positive for commands containing "Bash"/"PowerShell" as arguments [unaddressed open finding]

The latest Codex review (comment from chatgpt-codex-connector on commit 5b57cbb) flagged a false positive in scan_bare_tool that has not yet been addressed.

The grep at line 143 scans the raw text blob rather than parsed top-level tokens:

if printf '%s\n' "$text" | grep -qE "(^|[^[:alnum:]_])${tool}([^[:alnum:]_(]|\$)"; then

For a rule string like Bash(echo Bash) or Bash(grep "PowerShell" logs.txt), the inner Bash/PowerShell token matches the pattern — preceded by a space ([^[:alnum:]_]) and followed by ) ([^[:alnum:]_(]) — producing a spurious P1 "bare Bash allow rule" finding on a legitimate narrow grant.

This is exactly the same class of error that was already fixed for scan_agent in commit 1c403c0: the Agent check switched from text-scan to top-level token matching via grep -oE '[A-Za-z_][A-Za-z0-9_]*(\([^)]*\))?', where Bash(echo Bash) is consumed as a single token and the inner Bash never surfaces. scan_bare_tool should apply the same treatment: iterate over top-level tokens and flag only tokens that ARE Bash or PowerShell (no following (), exactly as scan_agent does for Agent. Fix this →

Practical risk: Low — Bash(echo Bash) or Bash(grep "PowerShell" ...) are contrived rule shapes unlikely in real repos. And the skill is advisory-only (exits 0), so a false positive is noise rather than a breakage. But the fix is well-understood (token-split, directly analogous to the scan_agent fix) and removes an inconsistency.


2. No settings.local.json regression test case [non-blocking, previously noted]

The prior Claude review noted that scan_settings_allow is called for both .claude/settings.json and .claude/settings.local.json (lines 226–227), but the test suite has no case that places a fragile grant in settings.local.json and asserts it is caught.

This is low risk — the production code path for both files is identical (same scan_settings_allow function, only the path differs). A P1 fixture in settings.local.json would close the gap. Fix this →


3. _runner gap: bare uv, pipx [minor, informational]

_runner includes uv run and uvx (and pipx run), but not bare uv or pipx. This means Bash(uv:*) or Bash(pipx:*) — which grant arbitrary execution via uv run <anything>, uv pip install, etc. — would not be flagged, while Bash(npm:*) and Bash(pnpm:*) would be. The inconsistency is minor given low prevalence of bare uv/pipx wildcard grants, but worth a comment noting this for future extension.


Correctness spot-checks

P1 regex — verified trace:

Rule Expected Verified
Bash(*) flagged (alt 1)
Bash(python*) flagged (alt 2, python in _interp)
Bash(sh -c*) flagged (alt 2, sh with non-alnum prefix -c)
Bash(npm:*) flagged (alt 3, npm in _runner, : as non-alnum)
Bash(npm run *) flagged (alt 3, npm matches, space+run as non-alnum-lead, *)
Bash(npm test) not flagged (no *)
Bash(npm run build) not flagged (no *)
Bash(pnpm dlx *) flagged (pnpm dlx before pnpm in alternation)
Bash(*.py:*) flagged (alt 4, leading *)
Bash(git add *) not flagged (git not in _interp/_runner)
Bash(babysit_merge.sh:*) not flagged (no alt matches)
Agent(code-reviewer) flagged by scan_agent
Bash(echo Agent) not flagged (Agent not a top-level token)

scan_agent token-split correctness: grep -oE '[A-Za-z_][A-Za-z0-9_]*(\([^)]*\))?' produces one token per tool; Bash(echo Agent) → single token Bash(echo Agent), inner Agent never surfaces. ✓

P2 exemptions: ${CLAUDE_PROJECT_DIR}/...$ is excluded by _seg pattern. ~/~ excluded. Doc placeholder <name>< excluded. Only concrete usernames match. ✓

P3 detection: jq -e 'has("permissions")' on the plugin settings.json (not .claude/settings.json) — correct scoping via dirname(dirname(manifest)). Agent-only plugin settings correctly not flagged. ✓

Vendor exclusion: ! -path '*/vendor/*' on the find correctly excludes both direct-child (vendor/SKILL.md) and nested (vendor/cli/SKILL.md) vendored copies while flagging real loadable skills. ✓

settings.local.json security posture: Parsed via jq -r '.permissions.allow // [] | .[]' — only the allow array is extracted, never echoed wholesale. ✓

_runner ordering: Multi-word entries (pnpm dlx, yarn dlx, pipx run, uv run) precede their bare prefixes (pnpm, yarn) in the alternation, ensuring the longer forms can match distinctly. Ordering does not affect correctness for P1 (both forms have wildcards and would flag either way), but is the right style choice. ✓


Non-issues (resolved in prior commits, confirming closed)

  • Bare frontmatter Bash/PowerShellscan_bare_tool runs on frontmatter blobs (bdb2b42). ✓
  • Agent token anchoringscan_agent uses top-level token split (1c403c0). ✓
  • Vendor SKILL.md false positives! -path '*/vendor/*' exclusion (5b57cbb). ✓
  • Bare npm:*/pnpm:*/yarn:* — added to _runner (5b57cbb). ✓

Design observations (not blockers)

  • Convention doc placement (acknowledged tradeoff): SKILL.md:18 and criteria.md:9 reference ../../../../docs/conventions/... — a path outside the plugin root that doesn't exist in a marketplace-cache install. The detector and criteria are self-contained; only the "why" prose cross-link is unavailable post-install. Author has accepted this tradeoff explicitly.
  • Scope boundary documentation is clear and well-enforced: settings-audit owns file correctness, permission-hygiene owns grant portability/durability.
  • Evals quality: 7 cases cover all three check types, the scope-boundary routing, the report-only constraint, and the carry-over vs. dropped distinction. Good coverage.
  • Version bump 0.2.0 → 0.3.0 is correct for a new skill addition.

Verdict

Approve with one actionable note. The open Codex finding (false positive in scan_bare_tool for rules containing "Bash"/"PowerShell" as command arguments) is real but low impact given advisory-only semantics and contrived trigger conditions. The fix is straightforward — switch to the same token-split approach already used by scan_agent. Everything else is solid: 27 test cases pass, the P1 regex correctly distinguishes narrow carry-over rules from dropped broad rules, the vendor exclusion is principled, and the scan_agent anchoring fix fully closes its prior false-positive thread.

@cursor cursor 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.

Stale comment

Risk: low. Not approving because Cursor Bugbot skipped (usage limit reached) and did not produce a clean review. Human review is needed before merge; no reviewers assigned because no non-author maintainers are available in this repo.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_4f8c5c3c-b025-49a8-8a6c-d51aa9ae0306)

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

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


PR Review — feat(claude-config-audit): permission-hygiene grant audit + convention

  • Read changed files and PR context
  • Review detector script (permission-rule-check.sh)
  • Review test suite (permission-rule-check.test.sh)
  • Review SKILL.md, criteria.md, evals.json
  • Review convention docs and manifest
  • Synthesize findings and post review

Summary

This is a well-structured PR that iteratively addressed prior Codex and automated review findings across 7 commits. The implementation is advisory-only (exits 0), correctly scoped against the sibling settings-audit skill, handles settings.local.json safely, and includes 27 regression tests. One open correctness issue carried forward from the previous automated review cycle remains unaddressed; everything else is solid.


Finding 1 — scan_bare_tool false positive on inner tool names [open, unresolved from prior reviews]

File: plugins/claude-config-audit/skills/permission-hygiene/scripts/permission-rule-check.sh#L143

if printf '%s\n' "$text" | grep -qE "(^|[^[:alnum:]_])${tool}([^[:alnum:]_(]|\$)"; then

This scans the full text string. For a rule like Bash(echo Bash) or Bash(grep PowerShell logs.txt), the outer Bash( is correctly excluded (followed by (), but the inner Bash or PowerShell token — preceded by a space [^[:alnum:]_] and followed by ) or end-of-string [^[:alnum:]_(] — produces a false P1 "bare Bash/PowerShell allow rule" finding on a valid narrow grant.

This is the same class of error that was already fixed in commit 1c403c0 for scan_agent: that function switched to tokenizing with grep -oE '[A-Za-z_][A-Za-z0-9_]*(\([^)]*\))?' so the greedy (\([^)]*\))? consumes a whole scoped rule as one token and an inner word never surfaces independently. scan_bare_tool should apply the same treatment: iterate over top-level tokens and flag only a token that is exactly Bash or PowerShell (no following ().

Practical risk: Low — advisory-only (exits 0), contrived trigger conditions (Bash(echo Bash) is an unusual rule shape). But the fix is well-understood, directly parallel to the scan_agent fix already present, and removes a visible inconsistency between the two functions. The test suite has a scan_agent negative fixture (Case 3: Bash(echo Agent) not flagged) but no equivalent for scan_bare_tool. Fix this →


Finding 2 — No settings.local.json regression test case [non-blocking]

scan_settings_allow is called for both .claude/settings.json (line 226) and .claude/settings.local.json (line 227). The test suite exercises the shared function extensively via the .json fixture but has no case that places a P1 or P2 grant in .local.json and asserts the detector flags it.

Practical risk: Very low — the production code for both files is identical (same function, only the path differs), and the function is well-tested via the .json path. Closing this gap would add one fixture + assert_contains. Fix this →


Closed findings from prior review cycles

Finding Status
Bash(npm:*) / Bash(pnpm:*) / Bash(yarn:*) not flagged Resolvednpm|pnpm|yarn added to _runner; Case 2 + 3 cover both sides
Agent token anchoring false positives (Bash(echo Agent)) Resolvedscan_agent uses token-split (1c403c0); Case 3 negative fixture confirms
Vendor SKILL.md false positives (e.g. Playwright plugin) Resolved! -path '*/vendor/*' in find; Case 6b covers direct and nested
Bare frontmatter Bash/PowerShell grants not checked Resolvedscan_bare_tool now runs on frontmatter blob

Correctness spot-checks

P1 regex trace — key cases:

Rule Expected Verified
Bash(*) flagged (alt 1)
Bash(python*) flagged (alt 2, python in _interp)
Bash(sh -c*) flagged (alt 2, sh + non-alnum prefix)
Bash(npm:*) flagged (alt 3, npm in _runner, : non-alnum)
Bash(npm run *) flagged (alt 3, npm matches, space+run non-alnum)
Bash(npm test) not flagged (no *)
Bash(npm run build) not flagged (no *)
Bash(git add *) not flagged (git not in _interp/_runner)
Bash(*.py:*) flagged (alt 4, leading *)
Bash(babysit_merge.sh:*) not flagged
Agent(code-reviewer) flagged by scan_agent
Bash(echo Agent) not flagged (inner token, not top-level)

scan_agent token-split correctness: grep -oE '[A-Za-z_][A-Za-z0-9_]*(\([^)]*\))?' produces one token per tool; Bash(echo Agent) → single token, inner Agent never surfaces. ✓

P2 exemptions: ${CLAUDE_PROJECT_DIR}/… ($ excluded by _seg), ~/ (~ excluded), doc placeholders <name> (< excluded). Only concrete usernames match. ✓

P3 detection: jq -e 'has("permissions")' on the plugin settings.json (not .claude/settings.json) — correct scoping via dirname(dirname(manifest)). Agent-only plugin settings correctly not flagged (Case 7). ✓

settings.local.json security posture: Parsed via jq -r '.permissions.allow // [] | .[]' — only the allow array is extracted, never echoed wholesale. ✓

Vendor exclusion: ! -path '*/vendor/*' covers both vendor/SKILL.md (direct) and vendor/cli/SKILL.md (nested). Fixture root is not named vendor to keep real-vs-vendored distinction testable. ✓


Minor notes

  • _runner includes uv run, uvx, pipx run but not bare uv or pipx. Bash(uv:*) grants arbitrary UV execution but is not flagged. Minor gap given low prevalence; worth a comment for future extension.
  • Convention doc at docs/conventions/permission-rule-hygiene/ is outside the plugin root and will not resolve in a marketplace-cache install. The detector and criteria are self-contained; only the "why" prose cross-link is unavailable post-install. Acknowledged structural tradeoff for a marketplace-wide convention.
  • Version bump 0.2.0 → 0.3.0 is correct for a new skill addition.
  • plugin.json description and keywords correctly reflect the four-skill capability set.
  • Evals: 7 cases cover all three check types, scope-boundary routing, report-only constraint, and the carry-over vs. dropped distinction.

Verdict

Approve with one actionable note. Finding 1 (scan_bare_tool false positive) is the only open correctness issue — real but low-impact given advisory-only semantics and contrived trigger conditions. The fix mirrors the scan_agent pattern already present in the same file. Finding 2 (no settings.local.json test) is very low-risk. All prior review findings are cleanly resolved. The P1 regex correctly distinguishes narrow carry-over rules from dropped broad rules, vendor exclusion is principled, and the 27-check test suite is solid.

@cursor cursor 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.

Stale comment

Risk: medium. Not approving because Cursor Bugbot skipped and did not produce a clean review. Human review is needed before merge; no reviewers assigned because no non-author maintainers are available in this repo.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@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: 5cfc6772e5

ℹ️ 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".

…only

A tool name embedded in another rule's payload (Bash(echo Bash),
Bash(grep PowerShell *)) satisfied the bare-tool ERE and produced false
P1 findings. Extract scan_agent's top-level tokenizer into
top_level_tokens() and exact-match tokens in scan_bare_tool.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Equ3hcHokPZMpjMzLgtcwm
@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_e7085530-a7f6-46f5-bd3b-eca8264164ba)

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 47s —— View job


PR Review — feat(claude-config-audit): permission-hygiene grant audit + convention

  • Gather PR context and review history
  • Read detector script (permission-rule-check.sh)
  • Read test suite (permission-rule-check.test.sh)
  • Review convention docs, SKILL.md, criteria.md, evals.json
  • Synthesize findings and post review

Summary

This is the eighth review pass across eight commits. The iterative cycle has produced a solid implementation. The one open correctness issue from the prior automated review cycle — the scan_bare_tool false positive on embedded tool names — is cleanly fixed in the latest commit (0deb1ce). One new Codex finding from commit 5cfc677 has not yet been addressed. Below is my full assessment.


Finding 1 — scan_bare_tool false positive RESOLVED ✓

Commit: 0deb1ce

The prior finding (Bash(echo Bash) / Bash(grep PowerShell *) producing spurious P1 bare-grant findings) is fully resolved. The fix extracts the existing tokenizer from scan_agent into a shared top_level_tokens() helper and rewrites scan_bare_tool to exact-match tokens rather than scan the raw string:

top_level_tokens() {
  printf '%s\n' "$1" | grep -oE '[A-Za-z_][A-Za-z0-9_]*(\([^)]*\))?' 2>/dev/null
}

The greedy (\([^)]*\))? group consumes the full parenthesized payload of each rule as one token, so an inner Bash or PowerShell never surfaces independently. Regression cases for both shapes are added to Case 3. Correct approach, clean implementation.

Trace:

Input Token(s) from top_level_tokens == "Bash"? Result
Bash(echo Bash) Bash(echo Bash) (single) NO not flagged ✓
Bash(grep PowerShell *) Bash(grep PowerShell *) (single) NO not flagged ✓
Bash Bash YES flagged ✓
PowerShell PowerShell YES flagged ✓

Finding 2 — Path-prefixed interpreter grants bypass P1 [open, unaddressed from Codex 5cfc677]

File: permission-rule-check.sh:98

The Codex review on commit 5cfc677 (posted 2026-07-14T08:07) flagged a coverage gap that has not been addressed in 0deb1ce.

P1 alt-2 allows only optional quotes/spaces before the interpreter name:

P1_ERE="${P1_ERE}|(Bash|PowerShell)\\([\"' ]*(${_interp})([^A-Za-z0-9_)][^)]*)?\\*[^)]*\\)"

A grant like Bash(.venv/bin/python *) or Bash(/usr/bin/python3 *) places a path prefix before the interpreter — starting with . or /, not in [\"' ]* — so the pattern fails to match. Both grants are wildcarded interpreter-led rules that auto mode drops, yet the detector produces a clean report for them.

This is a genuine correctness gap: criteria.md explicitly states "A grant that invokes one fixed script via an interpreter … is flagged as the same authoring anti-pattern" (the stated scope is broader than the current P1 regex covers). Notably, /usr/bin/python3 * is not caught by P2 either (P2 only matches user-home paths, not system paths).

Practical risk: Medium-low — advisory-only exits 0, virtualenv path grants (Bash(.venv/bin/python *)) are uncommon but plausible in repo-local settings.json. The fix would extend [\"' ]* to optionally allow a path prefix before the interpreter token, e.g. ([\"' ]*([^A-Za-z()\"' ]+/)?)? — which is non-trivial and best handled as a follow-up rather than blocking this PR.

Fix this →


Finding 3 — No settings.local.json regression test [non-blocking, previously noted]

scan_settings_allow is called identically for both .claude/settings.json (line 232) and .claude/settings.local.json (line 233). The test suite has no case placing a fragile grant in .local.json. Very low risk — same function, only the path argument differs. Closing this with a P1 fixture + assert_contains would complete the coverage. Fix this →


Correctness spot-checks — all passing ✓

P1 regex key cases (post-fix):

Rule Expected Verified
Bash(*) flagged (alt 1)
Bash(python*) flagged (alt 2, python in _interp)
Bash(sh -c*) flagged (alt 2, sh + non-alnum -c)
Bash(npm:*) flagged (alt 3, npm in _runner, : non-alnum)
Bash(npm run *) flagged (alt 3, npm + run)
Bash(npm test) not flagged (no *)
Bash(npm run build) not flagged (no *)
Bash(*.py:*) flagged (alt 4, leading *)
Bash(echo Bash) not flagged (embedded name, single token) ✓ NEW
Bash(grep PowerShell *) not flagged (embedded name, single token) ✓ NEW
Bash (bare) flagged via scan_bare_tool token match
Agent(code-reviewer) flagged via scan_agent
Bash(echo Agent) not flagged

top_level_tokens() correctness: grep -oE '[A-Za-z_][A-Za-z0-9_]*(\([^)]*\))?' with -o delivers non-overlapping left-to-right matches; the greedy [^)]* consumes the first occurrence of ), making inner ) unavailable for a second match start. This handles all realistic rule shapes. ✓

scan_settings_allow security posture: Only .permissions.allow[] is extracted via jq; file is never echoed wholesale. Double tr -d '\r' handles Windows CRLF on both parsing and output paths. ✓

P2 exemptions: ${CLAUDE_PROJECT_DIR}/… ($ not in _seg), ~/… (~ not in _seg), <name> placeholders (< not in _seg). Only concrete usernames in home-path shapes match. ✓

P3 scoping: jq -e 'has("permissions")' on plugin-root settings.json (path derived via dirname(dirname(manifest))). Agent-only plugin settings correctly not flagged (Case 7). ✓

Vendor exclusion: ! -path '*/vendor/*' in find covers both vendor/SKILL.md and vendor/cli/SKILL.md. Case 6b verifies both sides. ✓


Resolved from prior review cycles ✓

Finding Commit
Bare frontmatter Bash/PowerShell not checked bdb2b42
Agent token anchoring false positives 1c403c0
Vendored SKILL.md false positives 5b57cbb
Bare npm:*/pnpm:*/yarn:* not flagged 5b57cbb
scan_bare_tool false positive on embedded tool names 0deb1ce ← latest

Minor notes

  • _runner includes uv run/uvx/pipx run but not bare uv or pipx. Bash(uv:*) would not be flagged. Minor gap; low real-world prevalence.
  • Convention cross-link in SKILL.md:18 and criteria.md:9 points to docs/conventions/… outside the plugin root — unavailable in a marketplace-cache install. Acknowledged structural tradeoff for a marketplace-wide convention; mechanical audit is self-contained.
  • Version bump 0.2.0 → 0.3.0 correct for a new skill addition.
  • Evals (7 cases): complete coverage of all three check types, scope-boundary routing, report-only constraint, and carry-over vs. dropped distinction.

Verdict

Approve with one actionable note. The scan_bare_tool false positive that was open across multiple prior review cycles is cleanly resolved in 0deb1ce. The path-prefixed interpreter gap (Finding 2) is genuine but low-impact — advisory-only semantics mean it's a missed warning rather than a breakage, and the fix is a non-trivial regex extension better suited to a follow-up. Finding 3 (settings.local.json test) is very low risk. All previously flagged correctness issues are closed.

@cursor cursor 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.

Stale comment

Risk: medium. Not approving because Cursor Bugbot skipped and did not produce a clean review. Human review is needed before merge; no reviewers assigned because no non-author maintainers are available in this repo.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@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: 0deb1ce00b

ℹ️ 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".

…henated bare commands

Two P1 alt-2/alt-3 gaps from review:

- Bash(.venv/bin/python *) and Bash(/usr/bin/python3 *) scanned clean
  because only quotes/spaces were allowed before the interpreter name.
  Allow an optional path prefix (POSIX / or Windows \) before the
  interpreter basename.
- Bash(node-gyp:*) / Bash(npm-check-updates:*) false-flagged because
  any non-alphanumeric char counted as the post-name separator, so the
  hyphen matched and the audit told users to replace the very bare-name
  shape the convention recommends. Require a real separator
  (space, quote, :) after interpreter/runner names.

Also add the missing settings.local.json scan regression case.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Equ3hcHokPZMpjMzLgtcwm
@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_92502266-2b0f-46c2-be6b-eb6f11f37742)

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

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


PR Review — feat(claude-config-audit): permission-hygiene grant audit + convention

  • Read detector script and test suite
  • Verify path-prefixed interpreter fix (latest Codex finding)
  • Verify hyphenated bare-command false-positive fix (latest Codex finding)
  • Check remaining open issues (settings.local.json test gap)
  • Synthesize findings and post review

Summary

This is the ninth review pass on commit 885a94f. The two open Codex findings from commit 5cfc677 — path-prefixed interpreter grants bypassing P1, and hyphenated bare-command false positives — are both cleanly resolved in this commit. The previously noted settings.local.json test gap is also closed. All correctness issues from the iterative review cycle are now resolved.


Resolved findings — confirmed closed

1. Path-prefixed interpreter grants (Codex finding, 5cfc677) ✓ FIXED

P1_ERE alt-2 now accepts an optional path prefix before the interpreter name via ([^)\"' ]*[/\\\\])?:

P1_ERE="${P1_ERE}|(Bash|PowerShell)\\([\"' ]*([^)\"' ]*[/\\\\])?(${_interp})([\"' :][^)]*)?\\*[^)]*\\)"

Trace — Bash(.venv/bin/python *):

  • [\"' ]* → empty
  • ([^)\"' ]*[/\])? → greedy match on .venv/bin/, ending at the last / before python
  • (${_interp})python (matches python3?) ✓
  • ([\"' :][^)]*)?\\* → space separator + backtrack on * → flagged ✓

Trace — Bash(/usr/bin/python3 *):

  • Path prefix → /usr/bin/, interpreter → python3 ✓ flagged ✓

Trace — Bash(node-gyp:*) (must NOT flag):

  • Path prefix group: no / or \ before node-gyp, skips
  • (${_interp}) → matches node, leaving -gyp:*)
  • ([\"' :][^)]*)?- is not in ["' :], group skips
  • \\* must match - → fails → alt-2 does not match ✓

Case 2 adds positive fixtures and assertions for both path-prefixed forms (line 73–74, line 89–90). ✓

2. Hyphenated bare-command false positives (Codex finding, 5cfc677) ✓ FIXED

Post-name separator group tightened from ([^A-Za-z0-9_)][^)]*) to ([\"' :][^)]*) in both alt-2 (interpreter) and alt-3 (runner). A hyphen (-) is not in ["' :], so Bash(node-gyp:*), Bash(ruby-lsp:*), and Bash(npm-check-updates:*) — bare PATH commands that happen to start with an interpreter or runner name — no longer produce spurious P1 findings.

Case 3 adds negative fixtures for all three (line 116–117). ✓

3. settings.local.json test gap (prior Claude reviews) ✓ FIXED

Case 8b (line 233–239) now places a P1 grant in .claude/settings.local.json and asserts both the finding content and the file label. ✓


Correctness spot-checks — full P1 trace

Rule Expected Verified
Bash(*) flagged (alt 1)
Bash(python*) flagged (alt 2, no path prefix, python in _interp)
Bash(node *) flagged (alt 2, node in _interp, space separator)
Bash(sh -c*) flagged (alt 2, sh in _interp, space separator)
Bash(.venv/bin/python *) NEW flagged (alt 2, path prefix .venv/bin/)
Bash(/usr/bin/python3 *) NEW flagged (alt 2, path prefix /usr/bin/)
Bash(npm:*) flagged (alt 3, : separator)
Bash(npm run *) flagged (alt 3, space separator)
Bash(npm test) not flagged (no *)
Bash(npm run build) not flagged (no *)
Bash(*.py:*) flagged (alt 4, leading *)
Bash(node-gyp:*) NEW not flagged (hyphen not in ["' :], alt-2 separator fails)
Bash(ruby-lsp:*) NEW not flagged (same)
Bash(npm-check-updates:*) NEW not flagged (npm in _runner but - fails separator check)
Bash(babysit_merge.sh:*) not flagged (no match in any alt)
Bash(echo Bash) not flagged (single token, scan_bare_tool exact-match)
Bash(grep PowerShell *) not flagged (same)
Agent(code-reviewer) flagged via scan_agent
Bash(echo Agent) not flagged (inner token via top_level_tokens)

P2 exemptions: ${CLAUDE_PROJECT_DIR}/… ($ excluded by _seg), ~/… (~ excluded), <name> placeholders (< excluded). Only concrete usernames match. ✓

P3 scoping: jq -e 'has("permissions")' on the plugin-root settings.json (path via dirname(dirname(manifest))). Agent-only plugin settings correctly not flagged. ✓

settings.local.json security posture: Only .permissions.allow[] extracted via jq; file never echoed wholesale. Double tr -d '\r' for Windows CRLF. ✓

top_level_tokens() correctness: grep -oE '[A-Za-z_][A-Za-z0-9_]*(\([^)]*\))?' delivers non-overlapping left-to-right tokens; the greedy [^)]* consumes a rule's inner content as one token, preventing inner tool names from surfacing. ✓


Minor notes (non-blocking, all previously known)

  • _runner gap: Bash(uv:*) and Bash(pipx:*) are not flagged — uv and pipx are absent as bare names in _runner (only uv run, uvx, and pipx run appear). These grant arbitrary execution but are low-prevalence. Worth a follow-up addition.
  • Convention cross-link outside plugin root: SKILL.md:18 references ../../../../docs/conventions/permission-rule-hygiene/README.md, a path unavailable in a marketplace-cache install. The mechanical audit is self-contained; only the "why" prose cross-link is affected. Acknowledged structural tradeoff.
  • criteria.md doesn't mention path-prefixed interpreters by name: The new ([^)\"' ]*[/\\])? pattern catches Bash(.venv/bin/python *) but criteria.md P1 description only gives Bash(bash <path>*) as a path-prefix example. The detector comment at line 84–88 is explicit; the criteria prose is a minor doc gap, not a correctness gap.

Resolved across all prior review cycles

Finding Commit
Bare frontmatter Bash/PowerShell not checked bdb2b42
Agent token anchoring false positives 1c403c0
Vendor SKILL.md false positives 5b57cbb
Bare npm:*/pnpm:*/yarn:* not flagged 5b57cbb
scan_bare_tool false positive on embedded tool names 0deb1ce
Path-prefixed interpreter grants bypass P1 885a94f ← latest
Hyphenated bare-command false positives 885a94f ← latest
No settings.local.json regression test 885a94f ← latest

Verdict

Approve. All open correctness issues are resolved in 885a94f. The P1 regex correctly distinguishes interpreter/runner-led wildcard grants (flagged) from narrow carry-over rules (not flagged) and from hyphenated bare PATH commands (not flagged), and now also catches path-prefixed interpreter grants (Bash(.venv/bin/python *), Bash(/usr/bin/python3 *)). The settings.local.json test gap is closed. The remaining notes (uv/pipx gap, convention doc placement, minor criteria prose) are all low-impact and suitable for a follow-up.

@cursor cursor 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.

Stale comment

Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached) and did not produce a clean review. Human review is needed before merge; no reviewers assigned because no non-author maintainers are available in this repo.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Re: claude[bot] review (job):

Finding Classification Disposition
1 — scan_bare_tool false positive VALID Fixed in 0deb1ce (confirmed by this review)
2 — path-prefixed interpreter grants bypass P1 VALID Fixed in 885a94f — alt-2 now accepts an optional path prefix (POSIX / or Windows \) before the interpreter basename; positive regression cases added
3 — no settings.local.json regression test VALID Fixed in 885a94f — Case 8b flags Bash(python*) in .claude/settings.local.json and asserts the finding names the file
Minor — bare uv/pipx wildcards (Bash(uv:*)) not in _runner VALID (defer) Deferred as a follow-up: add uv|pipx to _runner with regression cases next time the detector patterns are touched — kept out of this PR to close the review cycle
Minor — convention cross-link outside plugin root Acknowledged Structural tradeoff already discussed in a prior (resolved) thread

885a94f also fixes the Codex separator finding (Bash(node-gyp:*) false positive). Suite: 48 checks pass, shellcheck clean.

@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: 885a94fbf9

ℹ️ 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".

… tokenizer

Two review gaps on the previous fix round:

- Bash(python3.11 *) / Bash(/usr/bin/python3.12:*) scanned clean:
  _interp only knew python3?. Accept version suffixes via python[0-9.]*.
- top_level_tokens() stopped a rule token at the first ), so payloads
  with command substitution or inline scripts (Bash(echo $(date) Agent),
  Bash(node -e "console.log()" PowerShell)) leaked the trailing tool
  word as a top-level token and re-created embedded-tool false
  positives. Accept one level of nested parentheses in the payload; ERE
  cannot balance arbitrary depth and rule payloads realistically nest
  at most once.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Equ3hcHokPZMpjMzLgtcwm
@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_3ae8ca81-6a1c-4933-bd0e-28567d517ba6)

@cursor cursor 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.

Risk: medium. Not approving because Cursor Bugbot skipped (usage limit reached) and did not produce a clean review. Human review is needed before merge; no reviewers assigned because no non-author maintainers are available in this repo.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@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: 867e5f229a

ℹ️ 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".

@kyle-sexton
kyle-sexton merged commit 0214a2f into main Jul 14, 2026
17 of 19 checks passed
@kyle-sexton
kyle-sexton deleted the feat/permission-rule-hygiene-audit branch July 14, 2026 08:54
kyle-sexton added a commit that referenced this pull request Jul 18, 2026
No linked issue.

Repins every ci-workflows selector and reusable-workflow reference to
`ec91c3433a8c3c0a7ebbdd239286e5a6a25eeec5` (v0.6.0), registered in the
runner policy by melodic-software/standards#175 and distributed by the
just-merged standards-sync PR. Gate callers gain `merge_group` /
same-repo `pull_request_target` selector routing; the claude-review lane
picks up per-head concurrency and the superseded-head guard.

## Verification

- Local dogfood: `GITHUB_REPOSITORY=melodic-software/claude-code-plugins
node .github/standards/runner-policy/runner-policy.mjs --root .` passes
against the #175 policy.
- No residual pre-v0.6.0 ci-workflows workflow pins (grep-verified).

## Related

- melodic-software/standards#175 (policy registration)
- melodic-software/github-iac#78 (epic — Campaign A)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant