Skip to content

fix: Filter permission gaps against configured subcommand patterns#38

Merged
evansenter merged 2 commits into
mainfrom
fix/permission-gaps-filter-subcommands
Jan 2, 2026
Merged

fix: Filter permission gaps against configured subcommand patterns#38
evansenter merged 2 commits into
mainfrom
fix/permission-gaps-filter-subcommands

Conversation

@evansenter

Copy link
Copy Markdown
Owner

Summary

  • Fixes bug where get_permission_gaps() reported commands like gh as gaps even when settings.json had subcommand patterns like Bash(gh pr view:*)
  • Updated load_allowed_commands() to extract base commands (first word) from patterns
  • Added tests for the new behavior

Fixes #37

Test plan

  • All 253 tests pass
  • New tests verify base command extraction from subcommand patterns
  • New tests verify permission gaps are filtered correctly

🤖 Generated with Claude Code

Previously, get_permission_gaps() would report commands like `gh` as
gaps even when settings.json had patterns like `Bash(gh pr view:*)`.
This happened because load_allowed_commands() extracted the full
pattern content ("gh pr view") rather than the base command ("gh").

The fix extracts just the base command (first word) from each pattern,
so any configured subcommand pattern covers the base command.

Fixes #37

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

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude

claude Bot commented Jan 2, 2026

Copy link
Copy Markdown

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

This PR fixes a bug where get_permission_gaps() incorrectly reported commands like gh as permission gaps when settings.json already had subcommand patterns like Bash(gh pr view:*). The fix updates load_allowed_commands() to extract base commands (the first word) from subcommand patterns.

Issues Found

Critical

None

Important

None

Suggestions

  • src/session_analytics/patterns.py:581 - The pattern matching uses :) in pattern which could theoretically match malformed patterns that have :) appearing elsewhere in the string. Consider using pattern.endswith(:*)) in combination with finding the content, or using a regex for more robust extraction. This is unlikely to occur in practice but would make the parsing more defensive.

Verdict

REQUEST_CHANGES - One minor suggestion for more defensive pattern parsing.


Automated review by Claude Code

@evansenter

Copy link
Copy Markdown
Owner Author

Feedback Addressed

Skipped

  • [Suggestion] patterns.py:581 - More defensive pattern parsing with endswith() or regex - Current implementation already handles edge cases correctly by using find() to get first occurrence. The suggested change adds complexity for a nearly impossible malformed pattern scenario.

@claude

claude Bot commented Jan 2, 2026

Copy link
Copy Markdown

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

This PR fixes a bug where get_permission_gaps() reported commands like gh as permission gaps even when settings.json had subcommand patterns like Bash(gh pr view:*). The fix updates load_allowed_commands() to extract base commands (first word) from subcommand patterns, ensuring that any configured pattern for a command prevents it from being reported as a gap.

Issues Found

Critical

None

Important

None

Suggestions

None

Notes

The linked issue #37 suggested a filter_configured parameter for backwards compatibility, but this PR chose to always filter configured patterns instead. This is a sound design choice—the original problem was false positives (suggesting permissions that already exist), so always filtering is the correct behavior. No one would want false positive recommendations.

The previous review's suggestion about defensive pattern parsing was appropriately skipped per the "Feedback Addressed" comment.

Verdict

APPROVE - Code is correct, well-tested, and addresses the linked issue. The implementation choice to always filter (rather than requiring opt-in) is a better UX for the problem being solved.


Automated review by Claude Code

@evansenter evansenter merged commit 915b1c7 into main Jan 2, 2026
3 checks passed
@evansenter evansenter deleted the fix/permission-gaps-filter-subcommands branch January 2, 2026 03:46
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.

Filter permission gaps against configured settings.json patterns

1 participant