Interactive command gate: Yes / No / Yes-Always for foreground chats - #328
Merged
Conversation
Add a unified command gate (utils/auth/command_gate.py) that runs the existing signature match, org allow/deny policy, and LLM safety judge layers behind a single entry point, and prompts the user in foreground chats when any layer blocks. Background chats and RCAs keep the pre-existing hard-block behavior. - Yes: approve for this invocation (silent to the agent). - No: abort with code USER_DECLINED, distinct from static policy codes so the agent sees an explicit user rejection. - Yes-Always: only offered on org-policy blocks; mutates the policy (disables the hit deny rule and/or adds an editable allow pattern) so future runs including background RCAs inherit the decision. Extend CommandVerdict with deny_rule_id and allowlist_exhausted so the gate can plan the right mutation, add plan_yes_always / apply_yes_always and a pattern-derivation helper in command_policy. Extend the existing WebSocket confirmation protocol with block_layer, yes_always_effect, an execute_always option, and edited_patterns handling. Thread the new fields through useMessageHandler and render them in ToolExecutionWidget with an editable regex input for proposed allow rules. Idempotency: two contextvars short-circuit duplicate prompts and duplicate guardrail LLM calls when one shell tool routes into another (terminal_exec -> cloud_exec) and when terminal_run._check_guardrails would otherwise re-evaluate a command the gate just approved. No new feature flag: the gate is the interactive surface of the existing guardrails, so GUARDRAILS_ENABLED and per-org list toggles continue to govern whether anything blocks (and therefore prompts) at all. Made-with: Cursor
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughConsolidates command authorization into a unified command gate with HITL prompts and Yes‑Always policy mutations, adds durable pending-turn persistence and session rehydration, extends client types/UI to surface confirmation and Yes‑Always effects, and replaces many per-tool confirmation flows with Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User/Client
participant UI as ToolExecutionWidget
participant Gate as command_gate
participant Policy as command_policy
participant Safety as evaluate_command
participant DB as chat_sessions
participant WS as WebSocket (HITL)
Client->>UI: request execute(command)
UI->>Gate: gate_command(user_id, tool, command)
Gate->>Safety: evaluate_command(command)
Safety-->>Gate: safety_result
Gate->>Policy: evaluate_compound_command(command)
Policy-->>Gate: policy_result
alt allowed
Gate->>DB: (optionally) record approved hash
Gate-->>UI: allowed=true
UI->>Client: run & stream output
else blocked (foreground)
Gate->>DB: mark_session_tainted(session_id)
Gate->>WS: send confirmation prompt (includes yes_always_effect / PolicyChange)
WS->>Client: show ConfirmationPanel (Deny / Allow / Always)
alt User chooses "Always"
Client->>WS: execute_always + edited_patterns
WS->>Policy: apply_yes_always(org_id, changes)
Policy-->>Gate: persisted
Gate-->>UI: allowed=true
UI->>Client: run & stream output
else User allows
Client->>WS: confirmation_response(execute)
WS-->>Gate: allowed=true
Gate-->>UI: allowed=true
UI->>Client: run & stream output
else User denies or timeout
WS-->>Gate: allowed=false (USER_DECLINED)
Gate-->>UI: allowed=false
UI->>Client: show denial
end
else blocked (background)
Gate-->>UI: allowed=false
UI->>Client: show denial (no prompt)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
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. Review rate limit: 0/1 reviews remaining, refill in 20 minutes and 17 seconds.Comment |
damianloch
changed the title
damianloch
marked this pull request as ready for review
April 28, 2026 14:37
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/components/tool-calls/ToolExecutionWidget.tsx`:
- Around line 596-613: The respond handler updates UI state before ensuring the
confirmation payload was actually sent; change respond (in
ToolExecutionWidget.tsx) to check the boolean return of
sendRaw(JSON.stringify(payload)) and only call onToolUpdate to set status to
'running' or 'completed' when sendRaw returns true; if sendRaw returns false (or
sendRaw is undefined) keep the card in pending state and surface an error to the
user (e.g., set an error output or retry prompt) so the UI does not show a
resolved state the server never received—apply this logic around the existing
payload/decision/edited usage and the confirmation_id/userId/sessionId checks.
In `@server/chat/backend/agent/tools/cloud_exec_tool.py`:
- Around line 1412-1421: The gate is applied to a fabricated command string
(gated_cmd) before CLI resolution, causing mismatches with org-policy and hash
checks; fix by moving the gate_command call (gate = gate_command(...)) to after
the final resolved command is produced (i.e., use the same
CLI-selection/normalization logic used by the actual execution path) or rebuild
gated_cmd using that exact CLI-selection logic used by the executor so the
command passed to gate_command matches the real exec string (references:
_CLI_PREFIX, provider, gated_cmd, gate_command, and
terminal_run._check_guardrails).
In `@server/chat/backend/agent/tools/terminal_exec_tool.py`:
- Around line 259-269: The current gate_command check (gate =
gate_command(user_id=user_id, tool_name="terminal_exec", command=command) and
its early return) is run before routing, causing aws/kubectl/terraform
invocations to be evaluated as terminal_exec; move this gating logic to after
you determine the destination tool (i.e., after the routing logic that decides
to call cloud_exec or iac_tool) so that you call gate_command with the actual
destination tool_name (e.g., "cloud_exec" or "iac_tool") and then enforce
gate.allowed; update references to gate, user_id and command accordingly so
denials and Yes-Always mutations are recorded against the final tool.
In `@server/chat/backend/agent/workflow.py`:
- Around line 987-994: The foreground branch currently only taints the session
and lets the original blocked prompt continue to agentic_tool_flow, so modify
the logic in workflow.py around the input_state check to short-circuit
foreground blocked inputs: when getattr(input_state, "is_background", False) is
False (foreground), immediately yield the blocked response ("Your message was
blocked by our safety system. Please rephrase your request.") and return (or set
a clear "requires_replay" flag that prevents entering agentic_tool_flow until an
explicit replay occurs), and keep mark_session_tainted(...) for both branches;
ensure check_input() / agentic_tool_flow invocation is skipped for
foreground-blocked inputs so the model never receives the blocked prompt.
In `@server/utils/auth/command_gate.py`:
- Around line 138-160: is_session_tainted currently uses get_user_connection()
and queries chat_sessions by id without setting an RLS context, so reads can
silently miss when RLS is required; update is_session_tainted to mirror
mark_session_tainted by calling set_rls_context(user_id) (or otherwise establish
the same RLS context) before performing the SELECT using
db_pool.get_user_connection(), and ensure callers now pass the user_id into
is_session_tainted (update call sites) so the RLS context can be set; keep the
same error handling and return semantics.
In `@server/utils/auth/command_policy.py`:
- Around line 318-370: derive_pattern_from_command is dropping the actual
subcommand and _ENV_ASSIGN_RE fails to strip quoted env assignments; update
_ENV_ASSIGN_RE to also match quoted values (e.g. allow VAR="..." and VAR='...')
so leading assignments like KUBECONFIG="/tmp/my cfg" are popped, and change the
stop condition in derive_pattern_from_command's parts-building loop (the
len(parts) check) so it allows one more token (include two subcommand tokens for
CLIs like "aws ec2 terminate-instances") — i.e. stop after three parts instead
of two — while keeping the existing behavior to break on flag-like tokens and
referencing the symbols _ENV_ASSIGN_RE and
derive_pattern_from_command/parts/tokens in your patch.
In `@server/utils/cloud/infrastructure_confirmation.py`:
- Around line 117-132: The code clears the durable pending turn unconditionally
even when confirmation_id wasn't found; change it so
_clear_pending_turn(session_id, user_id) is only called when the confirmation
was actually resolved (i.e., when confirmation_id exists in
_pending_confirmations and you've set confirmation_data['result'] etc.). Locate
the block handling confirmation_id and move or guard the call to
_clear_pending_turn so it runs inside the same if branch that resolves
confirmation_data (leave the else branch to only log the missing waiter).
- Around line 293-299: The race condition: register the waiter in
_pending_confirmations before calling _send_ws so a fast client response won't
be dropped; specifically, create and assign the dict entry for confirmation_id
(with keys 'result', 'user_id', 'timestamp') prior to invoking _send_ws(payload,
tool_name), ensuring handle_websocket_confirmation_response can find the pending
entry; also preserve any behavior that clears pending_turn only when no matching
confirmation exists, and keep confirmation_id, _pending_confirmations, and
pending_turn names unchanged.
In `@server/utils/db/db_utils.py`:
- Around line 1481-1496: The migration that adds the security_tainted column
must not silently fail: inside the ALTER TABLE try/except (the block calling
cursor.execute for "ADD COLUMN IF NOT EXISTS security_tainted"), after
conn.rollback() re-raise the exception (or explicitly abort boot) so startup
fails fast; alternatively, catch the error and explicitly disable the feature
that uses that column (the command_gate path that reads/writes security_tainted)
by setting a clear runtime flag—do not leave the warning-only behavior. Ensure
you reference the same cursor.execute/conn.commit/conn.rollback block and the
command_gate usage so the change prevents running with an absent column.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffee2b97-8942-4717-ab55-a534dc97275b
📒 Files selected for processing (17)
client/src/app/chat/types.tsclient/src/components/SecuritySettings.tsxclient/src/components/tool-calls/ToolExecutionWidget.tsxclient/src/hooks/useChatHistory.tsclient/src/hooks/useMessageHandler.tsserver/chat/backend/agent/tools/cloud_exec_tool.pyserver/chat/backend/agent/tools/kubectl_onprem_tool.pyserver/chat/backend/agent/tools/tailscale_ssh_tool.pyserver/chat/backend/agent/tools/terminal_exec_tool.pyserver/chat/backend/agent/workflow.pyserver/routes/chat_routes.pyserver/routes/command_policies.pyserver/utils/auth/command_gate.pyserver/utils/auth/command_policy.pyserver/utils/cloud/infrastructure_confirmation.pyserver/utils/db/db_utils.pyserver/utils/terminal/terminal_run.py
- ToolExecutionWidget: only transition card after sendRaw succeeds - is_session_tainted: set RLS context + scope by user_id (mirrors mark_session_tainted) - derive_pattern_from_command: keep up to 3 tokens so "aws ec2 terminate-instances" matches docstring - command_gate: require org admin (Casbin admin/access) before offering/applying Yes-Always - infrastructure_confirmation: only clear pending_turn when confirmation id matched - infrastructure_confirmation: register waiter before WS send to avoid fast-response race Made-with: Cursor
The pre-gate codebase had two parallel confirmation paths: the new command gate (shell commands, policy-aware, with Yes/No/Yes-Always) and a legacy wait_for_user_confirmation helper used by structured actions (Terraform apply/destroy, Bitbucket merges/deletes, Notion column deletes, Spinnaker pipeline triggers, destructive MCP tools, multi- account cloud_exec). Two code paths, two UIs, two pieces of plumbing for the same "are you sure?" question. This commit collapses them into one: - Add gate_action() to command_gate.py. Same _prompt_user surface as gate_command() but Yes/No only (no regex means no Yes-Always), denies in background (matches the previous legacy-helper behavior). - Delete the two cloud_exec legacy prompts and the now-orphaned summarize_cloud_command helper and its verb sets. Org policy is the source of truth for which cloud CLI verbs need approval; the unified gate at the top of cloud_exec already enforces that. Gate the multi-account fan-out once up front to keep the single-prompt UX. - Migrate iac_commands_tool (apply/destroy), bitbucket confirm_or_cancel, notion_export_postmortem, notion_update_database_properties (deletes only), spinnaker trigger_pipeline, and mcp_tools destructive-tool path to gate_action. - Delete wait_for_user_confirmation from infrastructure_confirmation.py now that nothing calls it. Net result: one gate, one confirmation UI, one WS protocol, one taint model. No behavior change for users beyond the visual unification (same Yes/No buttons everywhere, same cancel semantics, same background-chat deny behavior). Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/chat/backend/agent/tools/mcp_tools.py (1)
1261-1278:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFail closed for destructive MCP actions when confirmation cannot be evaluated.
This block currently executes destructive tools if user context is missing or
gate_actionraises, which bypasses HITL protection.Suggested fix
- if user_id: - summary_msg = summarize_mcp_tool_action(original_tool_name, kwargs) - if not gate_action( - user_id=user_id, - tool_name=tool_name, - summary=summary_msg, - ).allowed: - cancellation_result = f"MCP tool {original_tool_name} cancelled by user." - try: - if send_tool_completion: - send_tool_completion(tool_name, cancellation_result, "cancelled", tool_call_id) - except Exception: - pass - return cancellation_result + if not user_id: + return f"MCP tool {original_tool_name} blocked: missing user context for confirmation." + summary_msg = summarize_mcp_tool_action(original_tool_name, kwargs) + if not gate_action( + user_id=user_id, + tool_name=tool_name, + summary=summary_msg, + ).allowed: + cancellation_result = f"MCP tool {original_tool_name} cancelled by user." + try: + if send_tool_completion: + send_tool_completion(tool_name, cancellation_result, "cancelled", tool_call_id) + except Exception: + pass + return cancellation_result except Exception as confirm_err: logging.warning(f"Failed to get confirmation for {tool_name}: {confirm_err}") - # Continue without confirmation if there's an error + return f"MCP tool {original_tool_name} blocked: confirmation failed."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/chat/backend/agent/tools/mcp_tools.py` around lines 1261 - 1278, The confirmation/gating logic for potentially destructive MCP tools should fail closed: if user_id is missing or the gate_action/confirmation check raises an exception, cancel the tool instead of proceeding. Modify the block around summarize_mcp_tool_action and gate_action so that (1) if not user_id you construct the cancellation_result and call send_tool_completion(tool_name, cancellation_result, "cancelled", tool_call_id) if send_tool_completion is set, then return cancellation_result; and (2) wrap the gate_action call so any exception from gate_action or related confirmation is treated as a denial (i.e., create the same cancellation_result, call send_tool_completion if present, and return it). Use the existing symbols summarize_mcp_tool_action, gate_action, send_tool_completion, tool_name, original_tool_name, and tool_call_id to locate and implement this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/chat/backend/agent/tools/bitbucket/utils.py`:
- Around line 161-165: The gate_action(...) result is discarded and the specific
rejection signal (e.g., USER_DECLINED) is lost; capture the return value from
gate_action (e.g., gate_result = gate_action(user_id=user_id,
tool_name=tool_name, summary=message)), check gate_result.allowed and return
None when allowed, but when not allowed call build_cancelled_response while
preserving the gate decision field from gate_result (pass the
decision/code/signal field returned by gate_action so the USER_DECLINED signal
propagates back to the agent).
In `@server/chat/backend/agent/tools/cloud_exec_tool.py`:
- Around line 1159-1173: The block that calls gate_command again (_gated, _gate)
inside cloud_exec_tool.py duplicate-runs the approval flow already performed in
cloud_exec(), causing repeated prompts during AWS multi-account fan-out; remove
or bypass this second gate so _cloud_exec_aws_multi_account() is not re-gated.
Locate the gate_command usage (symbols: gate_command, _gated, _gate) and either
delete the entire re-gating block or add a conditional to skip it when the
caller has already performed gating (e.g., when invoking the multi-account
path), preserving the existing JSON error shape if gating is needed elsewhere.
- Around line 1339-1344: The gating logic omits the Tailscale CLI prefix because
"_CLI_PREFIX" lacks a "tailscale" entry, causing gated_cmd to remain unprefixed
and breaking org-policy regexes; add "tailscale": "tailscale" to the _CLI_PREFIX
mapping used in the provider lookup and keep the existing prefix application
logic in the gated_cmd construction so that when provider == "tailscale"
subcommands like "device list" or "auth-key create" are prefixed before calling
gate_command(user_id=user_id, tool_name="cloud_exec", command=gated_cmd).
In `@server/utils/auth/command_gate.py`:
- Around line 283-299: The block_layer and block_reason logic currently only
adds "session_taint" when it's the sole blocker; change it so that if tainted is
true you always append "session_taint" to layers (i.e., after building layers
from safety_layer and policy_layer, if tainted do
layers.append("session_taint")), then recompute block_layer = "+".join(layers);
likewise always append the taint reason to reasons when tainted (in addition to
safety/policy reasons) so block_reason reflects the combined state; update the
code references safety_layer, policy_layer, tainted, layers, block_layer,
reasons, and block_reason accordingly.
- Around line 167-173: The current early returns use the USER_DECLINED
hard-block code for non-user reasons; update the two places that call _block
when user_id is falsy and when not _is_foreground() to return a non-user
hard-block code (e.g., BACKGROUND_DENIED or TOOL_NOT_ALLOWED_BACKGROUND) instead
of "USER_DECLINED", and similarly change the invalid-regex validation branch
(the branch that currently returns a policy/validation code around line
~425-428) to use a distinct validation error code (e.g., "INVALID_INPUT" or
"VALIDATION_FAILED") rather than "USER_DECLINED"; ensure you update the _block
calls in the user_id check, the _is_foreground() check, and the invalid-regex
branch so that USER_DECLINED is reserved only for explicit user rejections.
In `@server/utils/cloud/infrastructure_confirmation.py`:
- Around line 217-221: The _pending_confirmations entries are missing the
session_id, causing cancel_pending_confirmations_for_session(session_id) to
cancel all unresolved confirmations; update the code that creates the entry
(where _pending_confirmations[confirmation_id] is set) to include 'session_id':
session_id, and then modify cancel_pending_confirmations_for_session to only
mark entries cancelled when entry.get('session_id') == session_id (in addition
to existing unresolved checks), ensuring you reference the dict key
_pending_confirmations, the confirmation_id assignment site, and the
cancel_pending_confirmations_for_session function when making the change.
---
Outside diff comments:
In `@server/chat/backend/agent/tools/mcp_tools.py`:
- Around line 1261-1278: The confirmation/gating logic for potentially
destructive MCP tools should fail closed: if user_id is missing or the
gate_action/confirmation check raises an exception, cancel the tool instead of
proceeding. Modify the block around summarize_mcp_tool_action and gate_action so
that (1) if not user_id you construct the cancellation_result and call
send_tool_completion(tool_name, cancellation_result, "cancelled", tool_call_id)
if send_tool_completion is set, then return cancellation_result; and (2) wrap
the gate_action call so any exception from gate_action or related confirmation
is treated as a denial (i.e., create the same cancellation_result, call
send_tool_completion if present, and return it). Use the existing symbols
summarize_mcp_tool_action, gate_action, send_tool_completion, tool_name,
original_tool_name, and tool_call_id to locate and implement this behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: da01fbb0-fed7-40fa-b1a4-10cbd4896ff8
📒 Files selected for processing (11)
client/src/components/tool-calls/ToolExecutionWidget.tsxserver/chat/backend/agent/tools/bitbucket/utils.pyserver/chat/backend/agent/tools/cloud_exec_tool.pyserver/chat/backend/agent/tools/iac/iac_commands_tool.pyserver/chat/backend/agent/tools/mcp_tools.pyserver/chat/backend/agent/tools/notion/postmortem.pyserver/chat/backend/agent/tools/notion/structured.pyserver/chat/backend/agent/tools/spinnaker_rca_tool.pyserver/utils/auth/command_gate.pyserver/utils/auth/command_policy.pyserver/utils/cloud/infrastructure_confirmation.py
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
OlivierTrudeau
left a comment
OlivierTrudeau
left a comment
Contributor
There was a problem hiding this comment.
Review comments on security, correctness, and scalability. Skipping cosmetic nits.
Contributor
|
Minor: Policy cache in
Suggestion: Use if len(_cache) > 500:
_cache.clear() |
OlivierTrudeau
requested changes
Apr 30, 2026
Every new org now gets the Observability Only template applied automatically at registration time: both the allowlist and denylist are enabled and populated with the full read-only rule set (AWS/GCP/Azure/k8s read operations, git, system diagnostics, etc.) plus the 13 universal deny rules (rm -rf /, LD_PRELOAD, reverse shells, SUID, etc.). The seeding is idempotent (skips if org already has rows), non-fatal (org creation succeeds even if policy seeding fails), and uses its own admin connection (the org INSERT has already committed). Previously both lists were OFF by default, leaving new orgs completely ungated at the policy layer until an admin manually configured Security settings. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/auth/command_policy.py (1)
126-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound the org-policy cache.
TTL only controls freshness here; entries for orgs that are never looked up again stay resident forever. In a long-lived multi-tenant worker, this dict will grow monotonically with org churn unless you add an eviction policy or size cap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/auth/command_policy.py` around lines 126 - 135, The _get_cached implementation leaves _cache unbounded causing memory growth; replace or wrap the current _cache usage with a bounded LRU-style eviction policy so old org entries are dropped when capacity is exceeded. Specifically, change the backing store referenced by _get_cached/_fetch/_cache to a size-capped structure (e.g., collections.OrderedDict used as an LRU or a cachetools.LRUCache) and on insertion ensure you evict the least-recently-used entry when len(_cache) >= MAX_CACHE_SIZE; keep existing TTL logic (_CACHE_TTL) and update access paths in _get_cached to move touched keys to the front so LRU eviction works correctly. Ensure any concurrency considerations around _cache are preserved (wrap mutations with the existing lock or add one if absent).
♻️ Duplicate comments (1)
server/utils/auth/command_policy.py (1)
360-374:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip
sudooptions before deriving the anchor.After removing only the literal
sudo, commands likesudo -u root aws ec2 describe-instancesstill derive^-u\s+root\s+aws\b, so the stored Yes-Always rule will not match the next real invocation. Keep skippingsudoflags (and flag values when applicable) until you reach the actual CLI token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/auth/command_policy.py` around lines 360 - 374, The current token-stripping loop only removes a literal "sudo" token but not its flags/flag-values, causing incorrect anchor derivation; update the loop in command_policy.py that uses tokens and _ENV_ASSIGN_RE to also handle "sudo" with its options by: when encountering a "sudo" token, pop it then repeatedly pop any following tokens that start with "-" (treating tokens with "=" as single flags) and also pop a following token as the flag value when appropriate (i.e., the flag does not contain "=" and the next token exists and does not start with "-"), continuing until you reach the real CLI token; keep the existing checks against _ENV_ASSIGN_RE and _SHELL_NON_CLI_LEADERS and preserve the rest of the logic that builds parts from tokens[0] and up to two subcommands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/auth_routes.py`:
- Around line 143-147: The current except blocks around the
seed_default_command_policy import/call only log str(policy_err) and drop the
traceback; update both call sites where seed_default_command_policy(org_id,
user_id) is invoked to use logging.exception(...) (or logging.warning(...,
exc_info=policy_err)) so the full stack trace is preserved in the logs when
import or seeding fails, keeping the same message text but including exc_info
for diagnostics.
In `@server/utils/auth/command_policy.py`:
- Line 806: The code currently picks the seed template by position using
get_policy_templates()[0]; change it to resolve the template by its id (e.g.,
"observability_only") so ordering changes won't break seeding: call
get_policy_templates(), search for the template whose id equals
"observability_only" (or the correct id string used in your templates), assign
that result to tpl, and add a defensive check that tpl is not None (log or
raise) before using it; update any references to tpl accordingly (the lookup
occurs where tpl = get_policy_templates()[0] is used).
---
Outside diff comments:
In `@server/utils/auth/command_policy.py`:
- Around line 126-135: The _get_cached implementation leaves _cache unbounded
causing memory growth; replace or wrap the current _cache usage with a bounded
LRU-style eviction policy so old org entries are dropped when capacity is
exceeded. Specifically, change the backing store referenced by
_get_cached/_fetch/_cache to a size-capped structure (e.g.,
collections.OrderedDict used as an LRU or a cachetools.LRUCache) and on
insertion ensure you evict the least-recently-used entry when len(_cache) >=
MAX_CACHE_SIZE; keep existing TTL logic (_CACHE_TTL) and update access paths in
_get_cached to move touched keys to the front so LRU eviction works correctly.
Ensure any concurrency considerations around _cache are preserved (wrap
mutations with the existing lock or add one if absent).
---
Duplicate comments:
In `@server/utils/auth/command_policy.py`:
- Around line 360-374: The current token-stripping loop only removes a literal
"sudo" token but not its flags/flag-values, causing incorrect anchor derivation;
update the loop in command_policy.py that uses tokens and _ENV_ASSIGN_RE to also
handle "sudo" with its options by: when encountering a "sudo" token, pop it then
repeatedly pop any following tokens that start with "-" (treating tokens with
"=" as single flags) and also pop a following token as the flag value when
appropriate (i.e., the flag does not contain "=" and the next token exists and
does not start with "-"), continuing until you reach the real CLI token; keep
the existing checks against _ENV_ASSIGN_RE and _SHELL_NON_CLI_LEADERS and
preserve the rest of the logic that builds parts from tokens[0] and up to two
subcommands.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49f3ac42-d8c2-4d2a-8aa8-2357d3ed4bd3
📒 Files selected for processing (2)
server/routes/auth_routes.pyserver/utils/auth/command_policy.py
- Remove duplicate gate_command call in _cloud_exec_aws_multi_account - Add tailscale to _CLI_PREFIX so policy patterns match correctly - Use TOOL_NOT_ALLOWED / BACKGROUND_DENIED instead of USER_DECLINED for non-user-initiated blocks in gate_action - Always include session_taint in block_layer/block_reason when tainted - Merge _is_foreground/_session_id into single _get_context() helper - Reset _guardrails_approved_command in gate_command finally block to prevent stale hash reuse across retried commands on the same thread - Fix cancel_pending_confirmations_for_session to actually filter by session_id (was cancelling all pending confirmations across all sessions) - Guard pending_turn rendering by is_own so non-owners don't see interactive confirmation cards - Add ReDoS protection to validate_pattern: length cap + nested quantifier check before compiling client-supplied regex - Use logging.warning with exc_info for seed_default_command_policy errors - Resolve observability_only seed template by id not list position - Cap policy cache at 512 entries with LRU eviction via OrderedDict - Strip sudo flags/values in derive_pattern_from_command so patterns like sudo -u root aws ec2 ... anchor on aws not -u Made-with: Cursor
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
_REDOS_RE ran a complex regex on user-supplied input, which is exactly the polynomial-regex-on-uncontrolled-data pattern CodeQL flags. Replace it with _has_nested_quantifiers(), a simple character walk that detects (X+)+ style nesting without executing any regex against user data. Made-with: Cursor
An org admin explicitly adding a pattern to the allowlist is a trust signal that should not be overridden by the LLM safety judge. Previously, safety_evaluate ran unconditionally so allowlisted commands (e.g. gcloud instances delete) still triggered a Yes/No prompt if the judge flagged them as dangerous. Now: if evaluate_compound_command returns allowed=True with a non-default rule_description (i.e. an explicit allow rule matched, not just "lists disabled"), skip the safety judge entirely. Default-allow (lists off) and no-rule-match paths still run the full judge. Made-with: Cursor
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…rule" This reverts commit c97572f.
|
OlivierTrudeau
approved these changes
May 1, 2026
This was referenced May 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Our security layers (NeMo signature match, org allow/deny policy, LLM safety judge) block destructive commands outright. That's the right call for background RCAs and scheduled runs, but in a foreground chat the user is typing the request themselves -- a hard block feels broken, and they have no way to say "I know, run it anyway" or "stop asking me about this command."
What this PR does
Adds a single command gate that runs the existing security layers behind one entry point and, for foreground chats only, prompts the user before a blocked command runs:
USER_DECLINEDcode so the agent sees an explicit user rejection rather than a static policy failure.Background chats and RCAs keep the existing hard-block behavior; no prompt, no bypass.
No new feature flag. The gate is just the interactive surface of the existing guardrails, so
GUARDRAILS_ENABLEDand the per-org list toggles continue to govern whether anything blocks (and therefore prompts) at all.How it's wired
Backend
server/utils/auth/command_gate.py(new) -- unified gate. One call per tool (terminal_exec,cloud_exec,kubectl_onprem,tailscale_ssh) replaces the scattered per-layer checks.server/utils/auth/command_policy.py-- extendedCommandVerdictwithdeny_rule_idandallowlist_exhausted; addedplan_yes_always/apply_yes_alwaysandderive_pattern_from_commandso the gate can propose conservative regex mutations (falls back to a fully escaped literal for shell compound statements).contextvarsshort-circuit duplicate prompts and duplicate guardrail LLM calls when one shell tool routes into another (terminal_exec->cloud_exec) or whenterminal_run._check_guardrailswould re-evaluate a command the gate just approved.server/utils/cloud/infrastructure_confirmation.py--wait_for_user_confirmation_exreturns the full decision payload (execute/execute_always/cancel+ edited patterns). The WS protocol is extended withblock_layer,yes_always_effect, theexecute_alwaysoption, andedited_patterns.Frontend
ToolExecutionWidget.tsx-- newConfirmationPanelrow with three visually distinct buttons (muted Deny, plain Allow, filled-blue Always). Always opens a popover with the proposed pattern editable before commit. Shows the real command in the header pre-approval, including forcloud_exec.useMessageHandler.tsthreads the new fields through;types.tsgetsPolicyChange/YesAlwaysEffect/ richerToolCall.SecuritySettings.tsx+routes/command_policies.py-- rules now require a non-empty description on both sides.Reload state model
The gate introduces a live HITL state that needs to survive a page reload. We keep
chat_sessions.messagesappend-only (history, never rewritten) and add a separatepending_turnJSONB column for the in-flight confirmation. The gate sets it before sending the prompt, clears it on any response or timeout, and the frontend renders it as a synthetic awaiting-confirmation tail card on load. This is what fixed the duplicate-tool-call / stuck-pending-card / missing-user-prompt issues that appeared during testing.Test plan
USER_DECLINED, not a policy code.Summary by CodeRabbit
New Features
Improvements
Bug Fixes