feat: add shell command approval flow

[forrestjgq]

Summary

Add a runtime-owned approval flow for protected shell commands:

• Preserve unconditional deny-list behavior for commands such as recursive deletion.
• Require one-shot user approval for recognized deletion commands in interactive TUI user turns.
• Fail closed for non-interactive origins, expired requests, disconnects, invalid responses, and shell parsing failures.
• After a recognized policy rejection or user denial, terminate the action without asking the model or a subagent to try another method.
• Stop remaining sibling tool calls and further model iterations when a terminal safety decision occurs.
• Recognize supported deletion commands across newlines, simple shell grouping, command substitution, common execution wrappers, and shell -c commands.
• Match power-control commands by executable position so argument text such as git grep shutdown is not falsely rejected.
• Use separate frontend and backend deadlines to close stale approval prompts reliably.

The shell classifier is intentionally lexical. It recognizes supported command families and common shell forms, but it does not attempt to infer arbitrary side effects performed by every program or interpreter.

Type

• Fix
• Feature
• Docs
• CI / tooling
• Refactor
• Other

Verification

• Relevant tests pass locally
• Relevant lint / type checks pass locally
• User-facing docs or screenshots are updated when needed

Commands and results:

• uv run --frozen pytest tests/test_agent_loop_approval.py tests/test_approval_broker.py tests/test_cli_tui_commands.py tests/test_sandbox_unit.py tests/test_shell_approval.py tests/test_subagent_manager.py tests/test_tool_registry_execute.py tests/test_tui_rpc_approval.py tests/test_tui_rpc_spine.py tests/test_tui_rpc_stubs.py -q: 255 passed.
• uv run --frozen ruff format --check raven/agent/tools/shell.py raven/agent/tools/shell_policy.py raven/agent/subagent/manager.py tests/test_shell_approval.py tests/test_subagent_manager.py: passed.
• uv run --frozen ruff check raven/agent/tools/shell.py raven/agent/tools/shell_policy.py raven/agent/subagent/manager.py tests/test_shell_approval.py tests/test_subagent_manager.py: passed.
• npm run type-check: passed.
• npm run lint: 0 errors, 22 warnings.
• npm test -- --run src/__tests__/approvalRoundTrip.test.ts: 7 passed.

No user-facing documentation or screenshots are required for this runtime and TUI safety-flow change.

Risk

• Security impact considered
• Backward compatibility considered
• Rollback path is clear for risky changes

The sandbox keeps its existing policy bypass because it remains the containment boundary. Direct execution preserves hard-deny precedence, and approval transport failures deny execution.

Interactive TUI user turns gain one-shot approval. Gateways and background origins without a trusted approval responder fail closed for recognized deletion commands. Workspace-boundary and deny-list violations remain terminal so the model cannot search for an alternative path after a safety rejection.

The feature can be rolled back by reverting this pull request.

Related Issues

N/A

[0xKT]

Reviewed the full range merge-base(origin/main)..HEAD (411edb7..ac7079f, 29 files). CI is green and I re-ran the 7 test files locally: 150 passed, matching the description.

The mechanism itself looks solid to me: one-shot authority only (session/always removed), the two-layer deadline, approval.closed always emitted from finally, the frontend matching ids so a late close cannot dismiss a newer request, the asymmetry where deny commits locally but allow waits for backend confirmation, and cancel_all on teardown. Those details are all in place and the comments explain the why.

Below are the points I noticed, with how I reproduced them. Happy to be corrected where I read something wrong.

Worth a look before merge

B1 - ordinary guard errors also terminate the whole turn - raven/agent/tools/shell.py:174-188

_guard_command return values now all go through _terminal_error() (retryable=False, abort_action=True), and AgentLoop ends the turn on that (loop/main.py:1661,1691). Since the deny patterns are word matches, some read-only commands get caught:

git grep -n shutdown        -> hard_deny -> turn ends
grep -rn reboot /var/log    -> hard_deny -> turn ends

With restrict_to_workspace=true, a single cat ~/.config/x takes the same path. Neither case is an approval decision. Before this PR they were ordinary tool errors (carrying [try a different approach]), so the model just picked another path and continued.

For a fix I would lean toward returning a plain str for the workspace-restriction branch and keeping abort for approval denial only. Whether a deny-pattern hit should abort I am less sure about -- you may well think it should and that the matching just needs tightening. Your call on that one.

Verify: assert abort_action is False after a workspace violation, and that git grep -n shutdown does not end the turn.

(Side note: the allow_patterns branch has no production caller anywhere in the repo, so it is probably not worth spending effort on.)

B2 - the subagent loop does not read abort_action - raven/agent/subagent/manager.py:213-221

The inner loop appends the tools.execute() result and keeps iterating (up to 15 rounds) with no abort check, and the subagent builds its own ExecTool (manager.py:163) so it never receives a responder. What I measured: after rm x is refused, bash -c "rm x" in the same loop executes normally.

So on the subagent path, "no equivalent retry" currently rests only on the model-facing text instruction. Suggest mirroring the main loop: on abort_action, skip the remaining sibling calls and end the subtask.

Verify: a subagent test asserting no tool call runs after the first denial.

B3 - suggest softening "cannot retry through an equivalent command or interpreter" in the description - raven/agent/tools/shell_policy.py:63-95

The module docstring already says this is lexical best-effort, so gaps in the implementation make sense to me. It is just that the sentence in the PR description reads like a runtime guarantee, and someone picking this up later may trust it more than intended. Forms I measured as allowed:

cd /tmp\nrm x        echo hi\nunlink x    (rm foo)        { rm foo; }
echo $(rm foo)       echo `rm foo`        bash -c "rm foo"   sh -c "rm foo"
nohup rm foo &       ls | xargs rm        for f in *; do rm $f; done
git clean -fdx       truncate -s 0 f      > f             python -c "import os; os.remove('x')"

npm rm / git rm / docker rm are correctly passed through -- that part reads right to me.

Root cause is that _command_segments uses punctuation_chars=";&|" which excludes \n (the newline is treated as plain whitespace and swallowed into the same segment, so segment[0] becomes cd/echo), plus (/{ sticking to rm as "(rm". Multi-line commands are a fairly common model output shape -- the PR's own demo fixture is three lines (ui-tui/src/demo/gallery.tsx:39) -- so that shape may be the one worth covering first.

Suggest only the description wording before merge; the implementation side (adding \n to segmentation, stripping (/{, extending the wrapper table with nohup/bash -c/sh -c/xargs) is fine as follow-up.

One thing I would like to confirm

D1 - deletion capability on non-interactive paths - raven/tui_rpc/spine.py:92-95

Default sandbox.backend="none" gives DirectExecutor, so the policy applies, and the responder is bound only for Origin.USER. With default config that means:

Path	rm foo.txt
TUI user message	prompt, allow once
raven agent REPL / -p	refused + turn ends
channels	refused + turn ends
cron / sentinel / heartbeat	refused + turn ends
subagent	refused, loop continues (B2)

I read "Fail closed for non-interactive origins" in the description as deliberate; I mainly want to confirm the scope is what you intended, since it means the agent can no longer delete files on channels and cron. The new tests pin this behavior, so if an opt-in is wanted (a config flag, or wiring the broker into the CLI REPL), deciding now is cheaper than changing it later.

Follow-up

• F1 _ABORTED_ACTION_REPLY (loop/main.py:54) is a hardcoded English user-facing string. Every other final reply comes from the model, so it stands out in a non-English conversation.
• F2 ExecTool now waits on a human but is not marked blocking_interaction = True (ask_user.py:27 sets it, with a comment about not being timer-killed). The budget works today but is not wide: registry ceiling 660s vs 35s + 600s = 635s, 25s of headroom. Raising either limit would let the registry timer interrupt it.
• F3 A shlex parse failure (unbalanced quote such as echo don't, or a trailing backslash such as echo foo\) is absorbed into HARD_DENY at shell_policy.py:151-155, so the user sees "blocked by safety guard (policy evaluation failed)". Fail-closed makes sense; it would just read better if "could not parse" and "policy refused" were distinguishable. Related: the deny patterns are evaluated twice (once in _guard_command, once in policy.evaluate), and the deny branch in the latter is unreachable as a result.
• F4 The prompt description is fixed to "Delete files using a shell command" (shell.py:231), while register_approval_matcher is a public extension point, so a future matcher would show the wrong description.
• F5 The 30s visible deadline auto-denies and ends the turn (prompts.tsx). Stepping away briefly means the task is over and cannot be resumed, which feels different from ask_user (no timeout) -- not sure whether that is intended. Separately, denied_digests looks unreachable on the main path (the first denial aborts and skips siblings), so test_denied_command_is_not_prompted_again_in_same_turn may be covering a scenario that cannot occur in the product.

Minor

• N1 The ContextVar is created per instance in ExecTool.__init__ (shell.py:89); the Python docs suggest module level, and building AgentLoop per session would accumulate context slots.
• N2 ShellCommandPolicy.__init__ (shell_policy.py:135) compiles extra_deny_patterns up front, so an invalid regex moves from a per-call tool error to a construction-time failure. Fail-fast is better, but the behavior changed, so a line in the description would help.
• N3 Type has both Fix and Feature checked; the repo convention is a single box mirroring the commit type (feat -> Feature).

[forrestjgq]

B1: Fixed the false positives for power-control command names. shutdown, reboot, and poweroff are now classified by executable position after wrapper normalization, so read-only commands such as git grep -n shutdown and grep -rn reboot /var/log are allowed. Actual commands such as shutdown, sudo -n reboot, and bash -c "poweroff" remain hard-denied.

I intentionally kept workspace-boundary and genuine deny-list violations terminal. The intended behavior is that a safety-policy rejection must stop the current action rather than invite the model to find another path.

B2: Fixed. The subagent loop now observes abort_action, stops executing remaining sibling tool calls, does not start another model iteration, and reports the subtask as stopped. A regression test covers a denied rm followed by an already-proposed bash -c "rm ..." sibling and a possible subsequent model retry.

B3: Fixed the common lexical gaps identified in the review. Command classification now preserves LF and CRLF as command boundaries and handles simple parentheses/braces, command substitution, nohup, and recognized shell -c wrappers. The PR description has also been softened to describe this as lexical classification rather than a guarantee that every possible program side effect can be detected.

D1: Confirmed as intentional. Only trusted interactive TUI user turns receive an approval responder. CLI prompt mode, channels, cron, sentinel, heartbeat, and subagent paths fail closed when no responder is available. Sandboxed execution retains its existing policy bypass because the sandbox remains the containment boundary.

F1, F3 message refinement, F4, N1, and N2 remain suitable follow-up items. Parse failures are still explicitly pinned as fail-closed by a regression test.

F2 was not changed in this patch. Marking the entire ExecTool as a blocking interaction would also remove the registry backstop for a wedged executor. The current 660-second ceiling still covers the 35-second approval ceiling plus the 600-second command limit. This should be revisited together if either timeout changes.

F5 is intentional: expiry is a denial and terminates the current action. denied_digests remains as defense in depth for direct or future callers even though the main loop normally terminates after the first denial.

The PR type is Feature only.

Verification after these changes:

• Ruff format passed.
• Ruff lint passed.
• 255 relevant Python tests passed.

[0xKT]

Follow-up review

Re-checked the latest head (72292cf, new merge-base 16bdca7). The B1/B2/B3 fixes all verify as working:

• power false positives: git grep -n shutdown, grep -rn reboot /var/log and man shutdown are allowed now.
• B2: I walked the nested-break logic in manager.py for false positives and negatives, and the new test drives the real inner loop while asserting executor.commands == []. Solidly done.
• B3: newlines (LF and CRLF), ()/{}, $(...), nohup and shell -c (including -lc and sudo bash -c) all reach REQUIRE_APPROVAL. I specifically checked whether the new (){} boundary characters cause collateral damage: cp file{,.bak}, echo {a,b}, quoted awk '{print $1}' and curl -d '{"a":1}' are all unaffected.

Your calls on D1, F2 and F5 make sense to me, not pursuing those. Each item below notes how I verified it.

Suggest addressing before merge

P1 - moving power detection to argv[0] dropped the systemctl <subcommand> family (measured)

Context first: switching to executable-position matching was my suggestion last round, and I did not measure what it narrowed. So this gap comes from that suggestion rather than from your judgement.

command	main (word regex)	current head
systemctl poweroff	denied	allow
systemctl reboot	denied	allow
sudo systemctl reboot	denied	allow
busybox poweroff	denied	allow
loginctl poweroff	denied	allow
systemctl -i poweroff	denied	allow
shutdown -h now	denied	hard_deny (ok)

systemctl poweroff is the standard way to power off a modern Linux box and succeeds without sudo inside a logind session, so it is probably the most common current form in this HARD_DENY category.

Suggested fix (I ran both directions of this locally):

_SYSTEM_POWER_COMMANDS = frozenset({"halt", "poweroff", "reboot", "shutdown"})
_POWER_MULTIPLEXERS = frozenset({"busybox", "init", "loginctl", "systemctl", "telinit"})

# in _matches_system_power_command, after the argv[0] check:
if executable in _POWER_MULTIPLEXERS and any(
    arg in _SYSTEM_POWER_COMMANDS or arg in {"0", "6"} for arg in segment[1:]
):
    return True

Coverage result: all 14 commands that should be denied are denied, including halt, init 0 and telinit 0 which main also misses. All 12 that should pass do pass, including systemctl show reboot.target, grep -rn 'systemctl poweroff' docs/ and git grep -n shutdown, which main flags as false positives. So this direction is strictly better than both main and the current head, without narrowing anything again.

Follow-up (packaged, no need to answer individually)

All four are edges of the area you just touched.

1 - backtick command substitution (measured)

echo `rm foo`      -> allow
echo $(rm foo)     -> require_approval

The backtick is not in punctuation_chars, so the token stays as one piece and PurePath reports the name with the backtick attached, which never equals rm.

Fix: add a backtick to punctuation_chars. I checked the collateral surface across 8 cases: backticks inside quotes are unaffected (tokenization of echo 'ab'andgrep "" f does not change), awk and ls do not change. The only difference is that an assignment such as x=`date` echo $x gets split, but _ASSIGNMENT still matches the x= token and pops it, so the classification result is unchanged.

2 - -c with a directly attached quote (measured)

bash -c'rm foo' and bash -c"rm foo" are allowed. The token is actually ['bash', '-crm foo'], and _embedded_shell_command reads segment[index + 1], which does not exist for the attached form.

Fix: look at where c sits inside the option token; if characters follow it, that is the command, otherwise take the next token.

pos = token.find("c", 1)
if pos != -1:
    if pos + 1 < len(token):        # -c'rm foo' -> '-crm foo'
        return token[pos + 1:]
    if index + 1 < len(segment):    # -c / -lc -> next token
        return segment[index + 1]

Verified: -c'...', -c"...", -c ... and -lc ... all match, while tar -cf a.tar b, bash -lc 'ls', docker -c ctx ps and bash --version do not.

3 - find -exec rm (measured)

find . -name "*.log" -exec rm {} \; is allowed while find -delete is covered. The tokens are ['find', '.', '-name', '*.log', '-exec', 'rm', '{}', ';'], so rm sits inside the find segment but not at position 0 and the executable is still find. _matches_delete_command only recognizes -delete for find; suggest also recognizing -exec / -execdir followed by rm or unlink.

4 - heredoc bodies are classified as commands (measured)

A cat <<'EOF' block whose body contains rm x reaches REQUIRE_APPROVAL. The direction is fail-safe and in an interactive turn it is only an extra prompt, but on a non-interactive origin it terminates the turn, and writing a script that contains an rm line through a heredoc is a fairly ordinary thing for the agent to do. How to weigh this is up to you; recognizing heredoc boundaries is not cheap, so parking it is reasonable.

One observation (no fix proposed)

rm -f file.txt hits the deny regex and becomes hard_deny, while rm --force file.txt goes to require_approval. That means the most common deletion forms, rm -rf and rm -f, never reach the approval flow this PR is built around, and tests/test_shell_approval.py:44 and :63 pin that pair of outcomes as expected.

I understand keeping deny-list hits terminal is deliberate and I am not trying to reopen that. This particular interaction just looks like it may not be the intended design. I am deliberately not proposing a fix: turning rm -rf into position-based matching is the same class of change that produced the P1 gap, and I have not measured its coverage, so I am only leaving the observation here for you to judge.

Nothing else blocking from my side beyond P1. With P1 addressed I think this is good to go.