feat(gateguard): add env knobs for routine bash gate + extra destructive patterns#2161
Conversation
…ive patterns The JS port of gateguard-fact-force has two bash gates: a destructive gate (rm -rf, drop table, git push --force, etc.) that operators want to keep, and a once-per-session routine gate that fires on the very first bash invocation regardless of intent. Operators on hosts where the routine gate is friction without signal (Cursor, OpenCode, etc.) have been maintaining local patches that get clobbered on every plugin update; the Python upstream gateguard-ai already exposes equivalent config via .gateguard.yml. Adds two env vars, both off-by-default so existing behavior is preserved: - GATEGUARD_BASH_ROUTINE_DISABLED — truthy values (1, true, on, yes, enabled) skip the routine bash gate. Destructive gate is unaffected. - GATEGUARD_BASH_EXTRA_DESTRUCTIVE — regex source string for additional destructive patterns. Matches against the same quote-stripped, subshell-flattened command the built-in DESTRUCTIVE_SQL_DD regex sees, so a custom phrase inside $(...) or backticks is also caught. A malformed regex is logged once to stderr and treated as not configured rather than crashing the hook (hooks must never block tool execution unexpectedly). Twelve new tests pin both env vars (truthy aliases, falsy values, unset baseline, destructive-gate-still-fires, alternation members, malformed regex degrades safely, custom phrase inside command substitution). Existing 2619/2619 tests still pass; eslint clean. Fixes affaan-m#2078
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe hook adds two env controls: ChangesBash gate environment configuration controls
Sequence DiagramsequenceDiagram
participant Caller
participant run as run(rawInput)
participant BashCheck as tool === 'Bash'
participant Destructive as isDestructiveBash()
participant RoutineDisabled as isRoutineBashGateDisabled()
participant Session as ROUTINE_BASH_SESSION_KEY
Caller->>run: invoke hook
run->>BashCheck: check tool name
alt tool is Bash
run->>Destructive: test built-in destructive patterns
Destructive->>Destructive: apply extra destructive regex (if configured)
alt destructive match
run->>Caller: deny (destructive gate)
else no destructive match
run->>RoutineDisabled: check GATEGUARD_BASH_ROUTINE_DISABLED
alt routine disabled
run->>Caller: allow (routine gate skipped)
else routine enabled
run->>Session: check once-per-session key
alt first Bash of session
Session->>Session: mark checked
run->>Caller: deny (routine gate)
else already checked
run->>Caller: allow
end
end
end
else not Bash
run->>Caller: allow (no gate)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/hooks/gateguard-fact-force.js`:
- Around line 79-92: The warning flag extraDestructiveWarnLogged is never reset
when extraDestructiveCacheKey (the env raw) changes, so subsequent distinct
invalid regex values are not reported; update the logic where
extraDestructiveCacheKey is assigned to reset extraDestructiveWarnLogged (or
track warnings per-key) whenever raw differs from the cached key so each bad
pattern logs once, and add a same-process regression test exercising
loadDirectHook() that sets multiple different invalid
GATEGUARD_BASH_EXTRA_DESTRUCTIVE values to verify a warning is emitted for each
distinct invalid value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91fb3ddb-cb3f-4282-8f54-62aa57ddd991
📒 Files selected for processing (2)
scripts/hooks/gateguard-fact-force.jstests/hooks/gateguard-fact-force.test.js
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… changes Both reviewers (CodeRabbit + cubic) flagged that extraDestructiveWarnLogged was never reset when GATEGUARD_BASH_EXTRA_DESTRUCTIVE flipped from one invalid regex to a different invalid regex. The sticky boolean meant a long-running process saw bad-pattern-a's warning then silently swallowed bad-pattern-b's parse failure. Fix: clear extraDestructiveWarnLogged whenever the cache key changes (i.e. before the regex compile attempt). The warn-once-per-distinct- pattern invariant now matches the per-key cache invariant. Adds a same-process regression test via loadDirectHook() that spies on process.stderr.write and asserts: same bad pattern warns once across multiple invocations; switching to a different bad pattern emits a second warning; switching to a valid regex emits zero warnings.
|
Thanks @coderabbitai and @cubic-dev-ai — both findings were the same valid bug, fixed in 9e5a59b.
|
@gaurav0107 Looks good — the fix addresses exactly what both reviewers flagged. The key change in extraDestructiveCacheKey = raw;
extraDestructiveWarnLogged = false; // <— the fixThat reset right before the The rest of the diff is clean too — 102/102 passing with no eslint issues is exactly the bar this team expects before merging. No further concerns on my side. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
|
Tip For best results, initiate chat on the files or code changes.
🐇🎉 |
…ive patterns (affaan-m#2161) * feat(gateguard): add env knobs for routine bash gate + extra destructive patterns The JS port of gateguard-fact-force has two bash gates: a destructive gate (rm -rf, drop table, git push --force, etc.) that operators want to keep, and a once-per-session routine gate that fires on the very first bash invocation regardless of intent. Operators on hosts where the routine gate is friction without signal (Cursor, OpenCode, etc.) have been maintaining local patches that get clobbered on every plugin update; the Python upstream gateguard-ai already exposes equivalent config via .gateguard.yml. Adds two env vars, both off-by-default so existing behavior is preserved: - GATEGUARD_BASH_ROUTINE_DISABLED — truthy values (1, true, on, yes, enabled) skip the routine bash gate. Destructive gate is unaffected. - GATEGUARD_BASH_EXTRA_DESTRUCTIVE — regex source string for additional destructive patterns. Matches against the same quote-stripped, subshell-flattened command the built-in DESTRUCTIVE_SQL_DD regex sees, so a custom phrase inside $(...) or backticks is also caught. A malformed regex is logged once to stderr and treated as not configured rather than crashing the hook (hooks must never block tool execution unexpectedly). Twelve new tests pin both env vars (truthy aliases, falsy values, unset baseline, destructive-gate-still-fires, alternation members, malformed regex degrades safely, custom phrase inside command substitution). Existing 2619/2619 tests still pass; eslint clean. Fixes affaan-m#2078 * fix(gateguard): reset extra-destructive warn-once gate when env value changes Both reviewers (CodeRabbit + cubic) flagged that extraDestructiveWarnLogged was never reset when GATEGUARD_BASH_EXTRA_DESTRUCTIVE flipped from one invalid regex to a different invalid regex. The sticky boolean meant a long-running process saw bad-pattern-a's warning then silently swallowed bad-pattern-b's parse failure. Fix: clear extraDestructiveWarnLogged whenever the cache key changes (i.e. before the regex compile attempt). The warn-once-per-distinct- pattern invariant now matches the per-key cache invariant. Adds a same-process regression test via loadDirectHook() that spies on process.stderr.write and asserts: same bad pattern warns once across multiple invocations; switching to a different bad pattern emits a second warning; switching to a valid regex emits zero warnings.
Summary
GATEGUARD_BASH_ROUTINE_DISABLEDenv var: truthy values (1,true,on,yes,enabled) skip the once-per-session routine bash gate inscripts/hooks/gateguard-fact-force.js. Destructive gate is unaffected.GATEGUARD_BASH_EXTRA_DESTRUCTIVEenv var: regex source string for operator-supplied additional destructive patterns. Matches against the same quote-stripped, subshell-flattened command that the built-inDESTRUCTIVE_SQL_DDregex sees, so a phrase inside$(...)or backticks is also caught.GATEGUARD_BASH_EXTRA_DESTRUCTIVEregex is logged once to stderr with a[gateguard-fact-force]prefix and treated as not configured — hooks must never crash tool execution because of operator config errors..gateguard.ymlreading) is deliberately out of scope for this PR.Verification
node tests/run-all.js— 2619/2619 passed, 0 failed (12 new tests added intests/hooks/gateguard-fact-force.test.js)npx eslint scripts/**/*.js tests/**/*.js— cleanGATEGUARD_BASH_ROUTINE_DISABLED=1skips routine bash gatetrue,on,yes,enabled, mixed case) all enable opt-out0,false,off, empty, unknown string) keep current behaviorGATEGUARD_BASH_EXTRA_DESTRUCTIVEcustom phrase fires destructive gate$(...)is also caughtFixes #2078
Summary by cubic
Adds env options to the Bash gateguard to disable the once-per-session routine gate and to add custom destructive regex patterns. Defaults are unchanged; destructive protections remain on. Fixes #2078.
New Features
GATEGUARD_BASH_ROUTINE_DISABLED: truthy values (1,true,on,yes,enabled) skip the routine Bash gate; destructive gate still runs.GATEGUARD_BASH_EXTRA_DESTRUCTIVE: regex source for extra destructive phrases; matched against quote-stripped, subshell-flattened commands. Malformed regex is logged and ignored (no crash).Bug Fixes
GATEGUARD_BASH_EXTRA_DESTRUCTIVEchanges so each distinct invalid regex logs once, avoiding silent failures in long-running processes.Written for commit 9e5a59b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes