fix(discover): exclude_commands bypass for env-prefix, sub cmd + regex#1398
fix(discover): exclude_commands bypass for env-prefix, sub cmd + regex#1398
Conversation
Strip env prefix (sudo, VAR=val) before exclude check so PGPASSWORD=x psql matches ["psql"]. Match against full command instead of first token so ["git push"] matches "git push origin main". Compile exclude entries as regex so ["^curl"] works as pattern. Invalid regex falls back to prefix match with eprintln warning. Non-regex patterns auto-anchor with ^ and \b to prevent substring matches (e.g. "go" won't exclude "golangci-lint").
pszymkowiak
added
bug
📊 Automated PR Analysis
SummaryFixes the exclude_commands matching logic in the discover registry to handle env-prefixed commands (e.g., PGPASSWORD=x psql), multi-token subcommand patterns (e.g., "git push"), and regex patterns (e.g., "^curl"). Non-regex patterns are auto-anchored with ^ and \b to prevent substring false positives. Invalid regex patterns gracefully fall back to prefix matching with a warning. Review Checklist
Linked issues: #917, #243, #1335 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
Replace \b with ($|\s) to only match end-of-string or whitespace. Empty string and bare "^" patterns produce regexes that match all commands, silently disabling all rewrites on a config typo. Skip trivial patterns with an eprintln warning.
pszymkowiak
left a comment
pszymkowiak
left a comment
There was a problem hiding this comment.
LGTM. Clean fix for 3 real user-visible regressions (#917, #243, #1335). ExcludePattern enum with Regex/Prefix fallback is a good design, regex::escape + auto-anchoring ^{esc}($|\s) correctly prevents substring false-positives ("go" no longer excludes golangci-lint). Env-prefix stripping via existing ENV_PREFIX regex is consistent with classify_command.
Verified locally:
cargo test discover::registry→ 232/232- Rewrite still wraps env correctly:
FOO=bar git status→FOO=bar rtk git status - Docs updated in
configuration.md+hooks/README.md
Non-blocking follow-ups:
test_exclude_invalid_regex_fallbackpasses for the wrong reason."curl["is not^-prefixed, so it goes throughregex::escape→^curl\[($|\s)which is a valid regex matching literalcurl[. ThePrefixfallback path is never exercised. To actually test the fallback, use an anchored invalid pattern like"^curl["(no escape → regex compile fails →ExcludePattern::Prefixbranch).compile_exclude_patternsruns on everyrewrite_commandcall, so an invalid pattern inconfig.tomlwilleprintln!on every Bash command the hook intercepts. Consider log-once or compile-at-config-load for a follow-up.- Minor perf: regex re-compiled per call (~10-50µs for 3-5 patterns, negligible vs 10ms startup, but cache at
Configlevel would be cleaner).
2 participants
Summary
PGPASSWORD=x psql -h localhostnow correctly excluded by["psql"], env prefix is stripped before matching["git push"]now excludesgit push origin main: matches against full command, not just first token["^curl"]works as regex , patterns are compiled as regex instead of literal==comparison["curl["]) fall back to prefix match with a stderr warning^and\bto prevent substring false positives ("go"won't excludegolangci-lint)Changes
Single file:
src/discover/registry.rsExcludePatternenum , compiled regex or prefix fallback, precompiled once perrewrite_commandcallcompile_exclude_patterns(), anchoring logic +eprintln!on invalid regexis_excluded(), matches against env-stripped command using precompiled patternsrewrite_segment_inner()to strip env prefix before exclude checkTest plan
test_exclude_env_prefixed_command,PGPASSWORD=x psqlexcluded by["psql"]test_exclude_subcommand_pattern,git push origin mainexcluded by["git push"]test_exclude_regex_pattern,curl http://...excluded by["^curl"]test_exclude_invalid_regex_fallback, invalid regex doesn't crash, falls back to prefixtest_exclude_does_not_substring_match,"go"doesn't excludegolangci-lint