Skip to content

fix(claude-config): add scan denominator to permission-rule-check - #2409

Closed
kyle-sexton wants to merge 1 commit into
mainfrom
cursor/permission-grants-denominator-63e1
Closed

fix(claude-config): add scan denominator to permission-rule-check#2409
kyle-sexton wants to merge 1 commit into
mainfrom
cursor/permission-grants-denominator-63e1

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Fixes #2283 (row A5).

The detector's clean-bill line now reports how many frontmatter files with allowed-tools and how many permissions.allow rules across how many settings files were scanned, so a zero-finding run after scanning nothing is distinguishable from a zero-finding run after a real scan.

Verification

  • permission-rule-check.test.sh: 91 checks passed

Related

…an bill

Fixes #2283 (row A5).

Clean output now states how many frontmatter files and allow rules were
scanned; finding runs emit the same counts on stderr.

Tests: permission-rule-check.test.sh (91 checks).

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@cursor

cursor Bot commented Aug 12, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 54s —— View job


Security Review

  • Gather context (diff via gh pr diff 2409)
  • Review shell script changes for injection/logic issues
  • Review skill/config changes for permission-widening
  • Post final findings

No security issues found.

This PR only adds scan counters (stats_frontmatter, stats_allow_rules, stats_settings_files) to permission-rule-check.sh and surfaces them in the clean-bill/finding output strings, plus corresponding doc/changelog/version and test updates. Specifically checked:

  • No injection surface introduced. The new printf 'No fragile permission grants found after scanning %d ... %d ... %d ...\n' "$stats_frontmatter" "$stats_allow_rules" "$stats_settings_files" (L350–351) and the >&2 variant (L354–355) use a static format string with %d placeholders bound only to counters produced via $(( ... + 1 )) arithmetic on stats_* variables — never interpolating file contents, filenames, or scanned rule text into the format string itself. No new eval, command substitution of untrusted data, or path handling was added.
  • No trust-boundary or authorization change. The counters are purely observational (increment-and-report); they don't alter which files are scanned, how findings are classified, or the script's exit-code semantics — a clean bill is still exit 0, findings still don't fail the run.
  • No CI/workflow, permissions, or dependency-pin changes. This PR touches only plugin script/doc/test/version files; no .github/workflows/*, settings.json permission grants, or action/dependency pins are modified, so there's nothing in scope for the dangerous-workflow-pattern or supply-chain categories.
  • Test additions (permission-rule-check.test.sh cases 8f) don't touch untrusted input — they exercise the new denominator strings against fixture directories the test harness controls.

Nothing further to report.

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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


Reviewing PR #2409

  • Gather context (read changed files, CLAUDE.md/REVIEW.md)
  • Understand the request (code review only)
  • Review permission-rule-check.sh changes
  • Review permission-rule-check.test.sh changes
  • Review SKILL.md / CHANGELOG.md / plugin.json changes
  • Post inline findings
  • Post summary review

Summary

This is a small, well-scoped fix: permission-rule-check.sh now accumulates stats_frontmatter, stats_allow_rules, and stats_settings_files counters through the existing scan loops and prints them in the clean-bill line (and as an stderr NOTE: when there are findings), so a genuine zero-finding scan is distinguishable from a scan that silently touched nothing. The change matches issue #2283 row A5 as described in the PR body.

Static review of the diff (gh pr diff 2409) and the full script/test file:

  • Counter logic is correct. stats_frontmatter increments once per frontmatter file with a non-empty allowed-tools block; stats_settings_files/stats_allow_rules increment inside scan_settings_allow only after the file parses as valid JSON, and only for the three settings files actually scanned (project .claude/settings.json, .claude/settings.local.json, and user-global, with the user-global skipped when it canonically resolves to the same file as project settings — preventing double-count). No double-counting or off-by-one issues found.
  • Test coverage (Case 8f) is appropriate: it covers both the "scanned nothing" (0 frontmatter file(s), 0 allow rule(s)) and "scanned something, still clean" (1 allow rule(s), 1 settings file(s)) paths, and confirms the clean-bill phrase itself is still present. This directly targets the ambiguity the PR is fixing.
  • Version/changelog bump (0.35.40.35.6, with 0.35.5 for the already-merged audit-pass fix) is consistent between plugin.json and CHANGELOG.md.

Two minor, non-blocking maintainability nits posted as inline comments:

  1. permission-rule-check.test.sh (L308) — the explanatory comment above Case 8e was removed rather than updated, losing some rationale for why the case exists.
  2. SKILL.md (L88) — the doc summary of the new clean-bill line omits the settings-file count, one of the three quantities the script actually reports.

Verification note: the PR body's "91 checks passed" test-run result is author-claimed; my sandboxed environment here didn't have approval to execute bash permission-rule-check.test.sh or gh issue view, so I was unable to independently re-run the test suite or pull the full #2283 issue text — this review is based on static reading of the diff and full source files only.

No correctness bugs or security concerns found in the scan-counting logic itself.

@@ -306,12 +306,24 @@ assert_eq "relocated config root produces exactly one finding" "1" \
"$(run_with_config_dir "$D8C" "$RELOCATED" "$FAKE_HOME" --count)"

# --- Case 8e: an unresolvable user scope is announced, never silently skipped --

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR drops the three-line comment that used to sit above Case 8e explaining why the test exists ("A silent skip would let \"No fragile permission grants found.\" rest on a scope that was never opened."). That's understandable since the quoted string is now stale (the clean-bill line carries a denominator suffix), but deleting the rationale entirely rather than updating the quoted string loses context for future readers about why this case matters. Consider re-adding a version of the comment with the quote refreshed (or paraphrased) instead of removing it outright.

Not a functional issue — flagging for maintainability only.

| warning | Interpreter/runner-led grant whose broad forms auto mode drops, or an inert self-grant (P1, P3) |

A clean scan ("No fragile permission grants found.") is a valid outcome — report it as such.
A clean scan (the detector's "No fragile permission grants found …" line, which now includes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This summary says the clean-bill line "now includes how many frontmatter files and allow rules were scanned," but the actual detector message (permission-rule-check.sh:350) also reports a third quantity — the number of settings files scanned (... across %d settings file(s).). Worth naming all three here so this doc stays a complete description of what the line reports.

@github-actions

Copy link
Copy Markdown

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Superseded by #2408, which carries the full #2281/#2283 contract stack.

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.

fix(claude-config): audit-permission-grants reports a clean bill with no denominator, and four amplifiers around it

2 participants