fix(permissions): match env-prefixed shell commands against saved permission rules

[yiliang114]

TLDR

Fix shell commands with leading environment variable assignments (for example PYTHONPATH=/tmp python3 -c "...") failing to match saved permission rules. This bug caused repeated permission prompts even after the user selected "Always allow".

Core fix: Normalize commands by stripping leading VAR=value assignments before permission pattern matching and before command-root extraction, so matching stays aligned with the rule extraction phase.

Screenshots / Video Demo

N/A — no user-facing UI change; this is an internal permission matching fix.

Root Cause

There was an asymmetry between rule extraction and rule matching in the permission system:

1. Rule extraction (extractCommandRules) uses tree-sitter AST parsing, correctly treats PYTHONPATH=... as a variable assignment, extracts the real command python3, and saves a rule like python3 *.
2. Rule matching (matchesCommandPattern) previously matched against the raw command string, so the same rule failed when the command started with PYTHONPATH=.

Final Fix

This PR keeps the fix narrow and focused:

1. rule-parser.ts — Permission pattern matching

• Add stripLeadingVariableAssignments() to normalize commands before matching
• Keep matchesCommandPattern() aligned with the rule extraction behavior
• Preserve dotAll regex matching so commands with embedded newlines in arguments still match correctly

2. shell-utils.ts — Command root extraction

• Update getCommandRoot() to skip leading VAR=value assignments
• Use shell-quote parsing so quoted Windows paths with spaces are handled correctly

Files Changed

File	Change Type	Description
packages/core/src/permissions/rule-parser.ts	Modified	Add env var stripping before command-pattern matching
packages/core/src/utils/shell-utils.ts	Modified	Update command-root extraction to skip env var assignments and preserve quoted paths
packages/core/src/permissions/permission-manager.test.ts	Tests added	Cover env-prefixed commands and embedded newline matching
packages/core/src/utils/shell-utils.test.ts	Tests added	Cover env var skipping and quoted Windows paths

Reviewer Test Plan

1. Permission matching

   • Run npx vitest run packages/core/src/permissions/permission-manager.test.ts
   • Confirm env-prefixed commands still match saved rules, including commands with embedded newlines in arguments

2. Command root extraction

   • Run npx vitest run packages/core/src/utils/shell-utils.test.ts
   • Confirm env-prefixed commands return the real command root
   • Confirm quoted Windows paths with spaces still resolve to the correct binary name

3. Regression

   • Confirm existing permission matching and shell utility tests still pass

Testing Matrix

	🍏	🪟	🐧
npm run	✅	❓	❓
npx	✅	❓	❓
Docker	❓	❓	❓
Podman	❓	-	-
Seatbelt	❓	-	-

Linked issues / bugs

Fixes #2846

Related to #2669, #2723

[github-actions]

📋 Review Summary

This PR addresses a critical permission matching bug where shell commands with leading environment variable assignments (e.g., PYTHONPATH=/tmp python3) failed to match saved permission rules. The fix is well-implemented with proper test coverage and maintains backward compatibility.

🔍 General Feedback

• The root cause analysis in the PR description is excellent and clearly explains the asymmetry between rule extraction and rule matching
• The fix is minimal and focused, addressing only the specific issue without unnecessary refactoring
• Test coverage is comprehensive, covering edge cases like embedded newlines and quoted content
• The use of the shell-quote library for proper tokenization is the right approach rather than regex-based parsing
• Both files (rule-parser.ts and shell-utils.ts) now have consistent handling of environment variable prefixes

🎯 Specific Feedback

🟢 Medium

• File: packages/core/src/permissions/rule-parser.ts:686 - The ENV_ASSIGNMENT_REGEX uses /^[A-Za-z_][A-Za-z0-9_]*=/ which is correct, but consider extracting this as a named constant at module level for potential reuse and better documentation of the shell environment variable naming convention

• File: packages/core/src/permissions/rule-parser.ts:673 - The regex flag change to 's' (dotAll) is good for embedded newlines, but consider adding a comment explaining why dotAll is needed (to match . across embedded newlines in command arguments)

• File: packages/core/src/utils/shell-utils.ts:308-310 - The new getCommandRoot implementation uses a simpler regex /^[A-Za-z_]\w*=/ to detect env var assignments. This is functionally correct but differs slightly from the ENV_ASSIGNMENT_REGEX in rule-parser.ts. Consider consolidating this pattern into a shared utility to ensure consistency across both files

🔵 Low

• File: packages/core/src/permissions/rule-parser.ts:516 - Adding '\r\n' and '\n' to SHELL_OPERATORS works for splitCompoundCommand, but the order matters. Since '\r\n' is two characters, it should be listed before '\n' to ensure proper matching (longest-first principle already documented in the comment). Current order is correct, but consider adding an explicit comment noting that '\r\n' must precede '\n'

• File: packages/core/src/permissions/rule-parser.ts:716 - The stripLeadingVariableAssignments function catches all errors and returns the trimmed command. This is defensive but consider logging the error in debug mode for troubleshooting: console.debug?.('Failed to parse command with shell-quote:', error)

• File: packages/core/src/utils/shell-utils.ts:313 - The while loop for skipping env var tokens could benefit from a brief comment explaining that multiple consecutive env var assignments are valid (e.g., FOO=1 BAR=2 python3)

✅ Highlights

• Excellent PR description with clear root cause analysis, fix explanation, and testing matrix
• The shell-quote library integration is the correct approach for robust shell command parsing
• Test cases cover important edge cases: embedded newlines, quoted newlines, and multiple env var prefixes
• The fix maintains backward compatibility with existing permission rules
• Good separation of concerns: rule-parser.ts handles permission matching, shell-utils.ts handles command root extraction
• The regex-based approach in getCommandRoot is appropriately simpler than AST parsing since it only needs the first command token

[tanzhenxin]

Nice fix for a real pain point — the root cause analysis is spot on. A couple of things I noticed:

rule-parser.ts, splitCompoundCommand

Adding \n to SHELL_OPERATORS breaks heredocs. Given a rule like Bash(python *):

python - <<'PY'
print(1)
PY

splitCompoundCommand() now splits this into three separate commands: python - <<'PY', print(1), and PY. A previously allowed heredoc starts prompting, and payloads like $(rm -rf /) that are inert inside the heredoc get flagged as dangerous standalone commands.

shell-utils.ts, getCommandRoot

The rewrite switches from a quote-aware regex to .split(/\s+/), which breaks commands with quoted paths containing spaces:

"C:\Program Files\foo\bar.exe" arg1
  old: bar.exe    ✓
  new: Program    ✗

This matters on Windows where spaces in paths are common.

[yiliang114]

Took another pass after the follow-up changes. The two issues from the earlier review look addressed now: newline splitting is no longer part of the fix, and the quoted Windows path case is handled by the current shell-quote-based parsing and test coverage.

This looks good to me now.

[tanzhenxin]

Review

Fixes the asymmetry between rule extraction (tree-sitter AST correctly strips env vars) and rule matching (regex on raw command). Both matchesCommandPattern and getCommandRoot now normalize commands via shell-quote.parse() before matching. Previous review feedback (Windows path regression, heredoc breakage) has been fully addressed.

Verdict

APPROVE — Previous critical issues resolved. The fix is correct, focused, and well-tested. Minor observations (lossy quote-stripping in token reconstruction, duplicate regex constant, a few missing edge-case tests) are non-blocking. Nice work on the iterative refinement!