Skip to content

fix(rewrite): rewrite inner commands of quoted sh/bash/zsh -c wrappers - #3828

Closed
SindriFr wants to merge 1 commit into
rtk-ai:developfrom
SindriFr:feat/rewrite-quoted-shell-wrappers
Closed

SindriFr wants to merge 1 commit into
rtk-ai:developfrom
SindriFr:feat/rewrite-quoted-shell-wrappers

Conversation

@SindriFr

@SindriFr SindriFr commented Sep 2, 2026 •

Copy link
Copy Markdown

Summary

Rewrites the portable commands inside an exact quoted sh -c, bash -c, or zsh -c wrapper, leaving the wrapper's own bytes untouched.

bash -c "head foo && grep -R bar ."
  → bash -c "rtk read foo --head-lines 10 && rtk grep -R bar ."

Agents that reach a project toolchain through a wrapper (direnv exec . bash -c '…', devenv shell -- bash -c '…', container and CI runners) send every command as a quoted script, so the whole invocation was opaque and no inner command was rewritten.

shell_wrapper.rs and most tests are @pashifika's, from #3057, rebuilt against current develop — the rewrite engine has since gained RewriteContext, PipeKind, analyze_pipeline, pipeline_final_safe, multi-line block handling, decision.rs and the #3171 producer fallback. Credited via Co-authored-by.

Refs #2767. That issue also asks for fish -c, which this deliberately leaves out (see Passthrough cases), so it does not close it.

Eligibility

Deliberately narrow. The shell must be exactly sh, bash, or zsh (any path, e.g. /bin/bash), the option exactly -c, and the script one balanced single- or double-quoted argument.

Only the script's byte span is replaced, so shell path, quote delimiters, horizontal spacing, suffix arguments and fd redirects come back byte-identical:

/bin/bash -c 'git status' command-name 'a b'
  → /bin/bash -c 'rtk git status' command-name 'a b'

bash -c 'git status' 2>&1
  → bash -c 'rtk git status' 2>&1

A wrapper is found at any prefix depth, so one behind a transparent prefix, a strippable command, or a process wrapper is reached too:

direnv exec . bash -c 'git status'
  → direnv exec . bash -c 'rtk git status'

timeout 30 bash -c 'git status'
  → timeout 30 bash -c 'rtk git status'

Passthrough cases

These return to the host unchanged:

  • a leading sudo — the inner rtk would run as root, where it is off secure_path, the same reason a bare sudo docker ps is left alone (Feature Request: Add sudo prefix support for command rewriting #146). Pinned by test_shell_wrapper_behind_sudo_is_not_rewritten.
  • anything exclude_commands matches — exclude_commands = ["bash"] exempts a wrapper the same way it exempts any other command, and exempts it from the permission surface too
  • a script containing a newline, heredoc, or $((
  • active outer $ expansion, backticks, or backslash escapes in a double-quoted script
  • anything contains_unattestable_construct rejects — command/process substitution, file-target redirects
  • extra shell options: bash -lc, bash -e -c, bash -o pipefail -c, fish --command
  • an unquoted or unbalanced script
  • a wrapper nested inside a wrapper — recursion is capped at one level
  • fish, whose control keywords (and, or, if) at a command boundary need a fish-aware lexer the shared one does not model

Permission handling

A host's allow rule matched the outer command — it never parsed the quoted script. Rewriting inside that script therefore must not widen what the rule approved, so:

  • deny rules are also evaluated against the inner commands, and an inner deny outranks an outer allow
  • a wrapper rewrite is never auto-allowed; its strongest verdict is Ask, so a * allow rule cannot approve a script the host never saw
  • Ask-capable hosts prompt, others keep their native permission flow

The gate does not look for the wrapper itself. rewrite_segment_inner probes for one at every prefix-stripping level, and records each script it enters on a WrapperScan. registry::scan_shell_wrappers replays that same walk and hands the result to permissions::check_with_wrapper_scan, which judges exactly those scripts. So the gate and the rewriter cannot disagree about where a wrapper may begin — behind an env assignment, a shell keyword, a routable or process wrapper (#2375), or any entry in the user's own unbounded transparent_prefixes, both reach it or neither does. A word that merely looks like a wrapper is never such a position, so echo bash -c 'rm -rf /tmp/x' is untouched.

A command-string wrapper the strict parser refuses (bash -lc, bash -o pipefail -c) is never rewritten, but it is recorded too: the host did not decompose that script either, so it must not ride an outer allow.

Agent protocol coverage: Claude, Cursor, Gemini, Copilot CLI, Droid.

Test plan

  • cargo fmt --all --check
  • cargo clippy --all-targets -- -D warnings
  • cargo test --all — 3,958 passed, 0 failed, 8 ignored, all suites green
  • unit tests across parser, registry, permissions and the agent protocols, plus 6 CLI integration tests in tests/rewrite_shell_wrapper_test.rs
  • rtk rewrite and rtk hook claude verified end-to-end against an isolated HOME/XDG_CONFIG_HOME with real .claude/settings.json and rtk/config.toml, over every prefix family in the round-1 matrix
  • CI to validate Linux and Windows

Tested on Linux x86_64 (NixOS).

@CLAassistant

CLAassistant commented Sep 2, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@SindriFr
SindriFr force-pushed the feat/rewrite-quoted-shell-wrappers branch 2 times, most recently from 29e56ff to 3317930 Compare September 7, 2026 09:50
@SindriFr

SindriFr commented Sep 7, 2026

Copy link
Copy Markdown
Author

Rebased onto current develop (was conflicting, now mergeable). Head is 3317930.

Two notes while rebasing:

A sudo behaviour change on develop caught this branch. #146/#3569 hardened the sudo passthrough after my branch point, so sudo bash -c 'git status' now returns None rather than rewriting — the sudo guard fires before the wrapper path. That is the right call (the inner rtk would run as root, where it is off secure_path), so I flipped my assertion to pin the passthrough as intentional instead of restoring the old behaviour, and corrected the PR body, which had claimed sudo was reached.

Overlap with #3057. shell_wrapper.rs and most of the tests here are @pashifika's work from that PR, rebuilt against current develop — the rewrite engine has since gained RewriteContext, PipeKind, analyze_pipeline, pipeline_final_safe and multi-line block handling, so it no longer applies as written. Credit is carried in the commit's Co-authored-by. #3057 is still open and untouched since July; if this lands, that one should close with it. Happy to defer entirely if @pashifika would rather refresh theirs.

Gates locally, on Linux x86_64 (NixOS):

cargo fmt --all --check                 clean
cargo clippy --all-targets -D warnings  clean
cargo test --all                        3,137 passed, 0 failed, 8 ignored, 12 suites green

Only license/cla has run on this PR — no build or test check has been triggered. Anything needed from me to get CI to run?

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 1 of 3 — CHANGES REQUESTED

Claim: rewrite the inner commands of an exact quoted sh|bash|zsh -c wrapper, byte-preserving the wrapper itself, without letting a host allow rule approve a script the host never parsed.
Scope: accept — the mechanism is right, the subset is honestly narrow, and the byte-span approach is the correct way to do this. #3057 is closed; #3911 is stacked on this branch, so this lands first. (round 1, frozen)
Ran: merge-base 5e10aa64, head 7518b894 and head-merged-onto-develop built into three separate CARGO_TARGET_DIRs (md5 distinct). rtk rewrite and all six rtk hook <agent> entry points, differentially, against isolated HOME + XDG_CONFIG_HOME with real .claude/settings.json and rtk/config.toml; 611 wrapped cases generated from 321 real commands harvested from the repo's own rewrite tests; raw-vs-rewritten execution of wrapped pipeline stages. No filter output changes, so the 20% floor (CONTRIBUTING.md) does not apply.
CI @7518b894: only license/cla. Run 34109043724 is action_required — see "Your questions".

Blocks merge (2, frozen at round 1)

1. src/hooks/permissions.rs:140 — the permission path and the rewrite path disagree about where a wrapper can start, so a prefixed wrapper is rewritten and auto-allowed with its inner deny rule skipped.

check_shell_wrapper_permissions calls parse_shell_wrapper(segment) / is_shell_wrapper_candidate(segment) on the raw segment, and both require the shell to be the segment's first word. rewrite_segment_inner does not: it reaches the wrapper after stripping env assignments, SHELL_KEYWORD_PREFIXES, ROUTABLE_WRAPPER_PREFIXES, PROCESS_WRAPPERS and user transparent_prefixes. So contains_shell_wrapper stays false, the function returns None, and the ask/allow loop hands back Allow.

With ~/.claude/settings.json = {"permissions":{"allow":["Bash(*)"],"deny":["Bash(rm:*)"]}}, neutral cwd, rtk rewrite (0=Allow, 1=passthrough, 2=Deny, 3=Ask):

                                                    base   head
bash -c 'git status; rm -rf /tmp/x'                   1      2   ✓ inner deny fires
command bash -c 'git status; rm -rf /tmp/x'           1      0   ✗
env     bash -c 'git status; rm -rf /tmp/x'           1      0   ✗
exec    bash -c 'git status; rm -rf /tmp/x'           1      0   ✗
builtin bash -c 'git status; rm -rf /tmp/x'           1      0   ✗
noglob  bash -c 'git status; rm -rf /tmp/x'           1      0   ✗
uv run  bash -c 'git status; rm -rf /tmp/x'           1      0   ✗
FOO=bar bash -c 'git status; rm -rf /tmp/x'           1      0   ✗
nice     bash -c 'git status; rm -rf /tmp/x'          1      0   ✗   (post-rebase only, #2375)
nohup    bash -c 'git status; rm -rf /tmp/x'          1      0   ✗   (post-rebase only, #2375)
timeout 30 bash -c 'git status; rm -rf /tmp/x'        1      0   ✗   (post-rebase only, #2375)
direnv exec . bash -c 'git status; rm -rf /tmp/x'     1      0   ✗   (transparent_prefixes)

End to end through the Claude hook, same settings:

$ echo '{"tool_name":"Bash","tool_input":{"command":"command bash -c '\''git status; rm -rf /tmp/x'\''"}}' | rtk hook claude
develop:  (no output — RTK stays out of it, the host evaluates the original command)
head:     {"hookSpecificOutput":{...,"updatedInput":{"command":"command bash -c 'rtk git status; rm -rf /tmp/x'"},
                                 "permissionDecision":"allow"}}

This is a regression against develop, which auto-allowed nothing on these inputs, and it inverts the three invariants the PR states: "deny rules are also evaluated against the inner commands", "a wrapper rewrite is never auto-allowed", and "One parser backs both rewriting and permission evaluation, so the two paths agree on the boundary." direnv exec . bash -c '…' is the PR's own headline example. Every host that honours AllowRewrite is affected.

Class, not instance: the covered set is exactly one shape (bare wrapper) out of every prefix family rewrite_segment_inner strips — ROUTABLE_WRAPPER_PREFIXES (7 entries), PROCESS_WRAPPERS (4), the env-assignment prefix, and an unbounded user-configured transparent_prefixes list. Patching the four I named would leave the config-driven one open, so please fix it at the boundary rather than by enumeration.

I am not sending a patch for this one, because I built the obvious one and it is wrong. Scanning every word start for a wrapper does close all twelve rows above (verified), but it also makes echo bash -c 'rm -rf /tmp/x' return Deny — a command that only mentions a wrapper. Two directions that do not have that failure mode, your call which:

  • have the permission check consume the wrapper spans the rewriter actually used, rather than re-deriving them (this is what "one parser backs both" would mean literally, and it needs no config in permissions.rs); or
  • thread transparent_prefixes into check_command_with_rules and reuse rewrite_segment_inner's prefix-stripping before parse_shell_wrapper.

2. The branch does not build on current develop, and one conflict hunk silently reverts #3171 if resolved toward this PR.

develop has moved 304 commits since the 09-07 rebase; mergeStateStatus is DIRTY. Three conflict hunks in src/discover/registry.rs and one in src/hooks/README.md. The one that matters is the third: develop added the .or_else(|| rewrite_pipeline_producer(...)) fallback (#3171), and this branch's side of that hunk does not have it — taking "theirs" drops producer-stage rewriting with nothing failing to say so.

I resolved it and ran the gates so the rebase is transcription rather than discovery. Nine items, in order:

  1. registry.rs import block — take develop's ordering, add contains_unattestable_construct and use super::shell_wrapper::parse_shell_wrapper;.
  2. rewrite_pipeline_stage (new on develop) — add shell_wrapper_depth: usize, thread into rewrite_segment_inner.
  3. rewrite_pipeline_final_stage — develop reshaped the signature; pass shell_wrapper_depth to rewrite_pipeline_stage.
  4. rewrite_pipeline_producer — add the parameter and keep develop's .or_else(...) fallback.
  5. strip_process_wrapper_prefix call site (#2375, ~registry.rs:1740) — a new rewrite_segment_inner call that needs the sixth argument. This is the one that fails to compile: error[E0061]: this function takes 6 arguments but 5 arguments were supplied.
  6. registry.rs:1373 — if shell_wrapper_depth < MAX_SHELL_WRAPPER_DEPTH { if let Some(...) } must become a let-chain, or edition 2024 fails clippy: error: this 'if' statement can be collapsed.
  7. cargo fmt --all — two assert! blocks in shell_wrapper.rs reflow under style edition 2024.
  8. Four assertion strings: rtk read foo → rtk read foo --head-lines 10 (develop #3668 changed how head rewrites). In registry.rs:7699, hook_cmd.rs:2859, hook_cmd.rs:2884, tests/rewrite_shell_wrapper_test.rs:22. Without this, 3 unit tests fail.
  9. src/hooks/README.md — keep develop's hook_cmd.rs line ("maps decisions to each agent's JSON protocol, including Codex updatedInput"), then add the discover/shell_wrapper.rs line.

After those nine: cargo fmt --all --check clean, cargo clippy --all-targets -- -D warnings clean, cargo test --all 3,843 passed, 0 failed. The wrapper matrix and develop's producer path (git log | head -20 → rtk git log | head -20) both still behave.

Optional — will not hold merge; I will resolve these threads myself

  • exclude_commands cannot exempt a wrapper. The wrapper branch sits above every is_excluded call in rewrite_segment_inner, so with exclude_commands = ["bash"], ["bash -c"], ["^bash"] or ["bash -c .*"], bash -c 'git status' is still rewritten to bash -c 'rtk git status' (control: ["git"] correctly suppresses it). A user who hits any sharp edge has no escape hatch. if is_excluded(command, excluded) { return None; } at the top of rewrite_shell_wrapper covers it.
  • is_shell_wrapper_candidate stops scanning at the first non-- token, so any shell option taking a separate argument hides the -c behind it. bash -lc, bash -e -c and bash --norc … -c correctly force Ask; bash -o pipefail -c, bash -O extglob -c and bash --rcfile F -c reach Allow. Not a regression — develop allows all of them — but it is the same claim as blocking item 1, so worth closing in the same pass.
  • registry.rs:1322 — the doc comment says sh|bash|zsh|fish -c; SUPPORTED_SHELLS deliberately omits fish and a test pins that. Drop |fish.
  • The commit message says Closes #2767 while the PR body says "it does not close it" (fish is out of scope). The trailer wins when develop reaches the default branch. Suggest Refs #2767.

Checked and correct — no need to re-verify

  • Byte preservation: shell path, quote delimiters, tabs/spacing, suffix arguments and fd redirects all come back identical (sh\t-c\t'…'\tname, /bin/bash -c '…' command-name 'a b', … 2>&1).
  • Inner deny across ;, &&, ||, |, &, both quote styles, and sh / zsh / /bin/bash — all 10 bare forms return Deny.
  • All six agent entry points end to end with real payloads, not test helpers: claude, cursor, gemini (ask_user + rewritten tool_input), copilot, droid, vibe. (codex is not a rtk hook subcommand.)
  • Recursion cap: bash -c 'git status && bash -c "cargo test"' → bash -c 'rtk git status && bash -c "cargo test"'; bash -c 'bash -c "git status"' → passthrough.
  • sudo, sudo -E, sudo -u nobody, doas all pass through.
  • Your gates on the head: cargo fmt --all --check clean, cargo clippy --all-targets -- -D warnings clean, cargo test --all 3,243 passed / 0 failed.

Hypotheses built and dropped

  • An inner rewrite emitting a shell metacharacter that changes the wrapper's meaning. 321 real commands harvested from the repo's rewrite tests → 611 single- and double-quoted wrapped cases → 379 rewritten. Zero introduced ', ", $, ` or \; no rtk_cmd in RULES contains one. Every inner script also matched the bare rewrite of the same command.
  • UTF-8 span safety. Every boundary (script_start, script_end, supported_shell_end) lands on an ASCII ', ", space or tab, which cannot occur inside a multibyte sequence, and replace_script uses .get() throughout. Your truncation corpus test already covers it.
  • Pipeline-final guards bypassed inside a wrapper. The bypass is real — develop refuses cat pat.txt | grep -f - data.txt (exit 1, the -f/--file deferral in TECHNICAL.md), and this PR rewrites the same thing inside bash -c because rewrite_shell_wrapper re-enters through rewrite_compound, which resets RewriteContext to Normal. But I could not turn it into a defect: grep -f -, jq ., wc -l and grep b as wrapped final stages all produced byte-identical stdout and exit codes raw vs rewritten. Recording it as a lost safety margin, not a finding — if you want to restore it, skipping the wrapper branch when context == RewriteContext::PipelineFinal is the one-line version.
  • Allow → Ask for wrappers a user had explicitly allowlisted. Real but deliberate and documented, and it only reaches a host when RTK actually rewrote; the hooks emit nothing on a non-rewrite, so nobody gets a new prompt for a wrapper RTK left alone.

Your questions

  • "Only license/cla has run on this PR — no build or test check has been triggered. Anything needed from me to get CI to run?" → Nothing from you. This is a fork PR, so the CI workflow parks at action_required until a maintainer approves the run — run 34109043724 on 7518b894, and three earlier ones on older heads, are all sitting in that state. I'll approve the run on the rebased head; approving it on this one would only fail at checkout while the PR conflicts.
  • "#3057 is still open and untouched since July; if this lands, that one should close with it. Happy to defer entirely if @pashifika would rather refresh theirs." → No need to defer. #3057 is already closed, and #3911 is explicitly stacked on this branch, so this is the one that lands first. Keep the Co-authored-by.

Next

Fix blocking item 1 at the boundary (your call which of the two directions), then rebase with the nine steps above — I have those verified green, so they should not cost you a debugging pass. The four Optional items are cheap enough to fold into the same push. Then ping me and I'll approve the CI run.

Rounds: 1/3. Threads: 2 open (blocking items 1 and 2), 0 resolved this round.

Comment thread src/hooks/permissions.rs Outdated
An agent that reaches a project toolchain through a wrapper writes every
command as `direnv exec . bash -c '...'` or `devenv shell -- bash -c '...'`.
The script is one quoted argument, so the whole invocation was opaque and no
inner command was ever rewritten.

Parse the conservative subset of quoted `-c` wrappers and rewrite the script
in place, replacing only its byte span. The shell path, quote delimiters,
horizontal spacing, suffix arguments and fd redirects stay byte-identical.
Recursion is capped at one level, so a wrapper inside a wrapper stays raw.

A wrapper is reached at any prefix depth, so one sitting behind a transparent
prefix or a strippable `command` is still seen. A leading `sudo` keeps the
passthrough it has for bare commands: the inner `rtk` would run as root,
where it is off secure_path (rtk-ai#146). `exclude_commands` exempts a wrapper the
same way it exempts any other command.

Deferred unchanged: a script with a newline, heredoc, `$((`, active outer `$`
expansion, or any construct the shared lexer cannot attest; extra shell
options (`-lc`, `-e -c`); an unquoted or unbalanced script; and `fish`, whose
control keywords (`and`, `or`, `if`) at a command boundary need a fish-aware
lexer this does not model.

Permissions are evaluated on both sides of the new boundary: deny rules run
against the inner segments, an inner deny outranks an outer allow, and a
wrapper rewrite is never auto-allowed — its strongest verdict is Ask, so a
`*` allow rule cannot approve a script the host never parsed.

The gate does not look for that boundary itself. The rewrite walk records the
wrapper scripts it enters, and `scan_shell_wrappers` replays that walk so
`check_with_wrapper_scan` can judge exactly those. The two therefore cannot
disagree about where a wrapper may begin — which matters because the rewriter
reaches one behind an env assignment, a shell keyword, a process wrapper
(rtk-ai#2375) or any entry in the user's own unbounded `transparent_prefixes`, while
enumerating those in the gate would leave the config-driven ones open. A word
that merely looks like a wrapper is never such a position, so
`echo bash -c '...'` stays untouched.

Refs rtk-ai#2767

Co-authored-by: pashifika <pashifika.mkii@gmail.com>
@SindriFr
SindriFr force-pushed the feat/rewrite-quoted-shell-wrappers branch from 7518b89 to 07e17ba Compare September 21, 2026 17:00
@SindriFr

Copy link
Copy Markdown
Author

Round 1 addressed. Head is 07e17ba, rebased onto develop (b748a5f); mergeable is now MERGEABLE.

Blocking 1 — the boundary

Took your first direction: the gate consumes the spans the rewriter used rather than re-deriving them. permissions.rs no longer mentions parse_shell_wrapper, is_shell_wrapper_candidate, or any prefix — check_shell_wrapper_permissions is gone.

rewrite_segment_inner already probes for a wrapper at every recursion level, which is exactly the set of positions one can start at. It now records what it finds on a WrapperScan carried through the walk, registry::scan_shell_wrappers replays that walk and returns the scan, and permissions::check_with_wrapper_scan judges those scripts. One walk, so there is nothing left to drift — and nothing enumerated, so a transparent_prefixes entry is covered by construction rather than by being listed.

Same settings as your matrix ({"permissions":{"allow":["Bash(*)"],"deny":["Bash(rm:*)"]}}, transparent_prefixes = ["direnv exec ."]), rtk rewrite exit codes:

                                                    develop   3828(old)   head
bash -c 'git status; rm -rf /tmp/x'                     1          2        2
command bash -c 'git status; rm -rf /tmp/x'             1          0        2
env     bash -c 'git status; rm -rf /tmp/x'             1          0        2
exec    bash -c 'git status; rm -rf /tmp/x'             1          0        2
builtin bash -c 'git status; rm -rf /tmp/x'             1          0        2
noglob  bash -c 'git status; rm -rf /tmp/x'             1          0        2
uv run  bash -c 'git status; rm -rf /tmp/x'             1          0        2
FOO=bar bash -c 'git status; rm -rf /tmp/x'             1          0        2
nice    bash -c 'git status; rm -rf /tmp/x'             1          0        2
nohup   bash -c 'git status; rm -rf /tmp/x'             1          0        2
timeout 30 bash -c 'git status; rm -rf /tmp/x'          1          0        2
direnv exec . bash -c 'git status; rm -rf /tmp/x'       1          0        2

echo bash -c 'rm -rf /tmp/x'                            1          1        1

That last row is the reason this shape rather than a word-start scan: echo is not a position the rewriter can strip to, so the walk never offers it.

End to end, same settings:

$ echo '{"tool_name":"Bash","tool_input":{"command":"command bash -c '\''git status; rm -rf /tmp/x'\''"}}' | rtk hook claude
(no output — RTK stays out of it, as on develop)

$ echo '{"tool_name":"Bash","tool_input":{"command":"direnv exec . bash -c '\''git status; cargo test'\''"}}' | rtk hook claude
{"hookSpecificOutput":{...,"updatedInput":{"command":"direnv exec . bash -c 'rtk git status; rtk cargo test'"}}}
                                                                  ^ no permissionDecision

Pinned by test_prefixed_shell_wrapper_inner_deny_wins (all eleven prefixes), test_prefixed_shell_wrapper_never_auto_allowed, and test_wrapper_shaped_argument_is_not_a_wrapper.

One shape note: the scan is &RefCell<WrapperScan> bundled with the depth into a Copy WrapperWalk, because rewrite_pipeline_final_stage(...).or_else(|| rewrite_pipeline_producer(...)) — your item 2 step 4 — is two branches a &mut could not span, and I would rather keep that call site identical to develop's than reshape it. It also keeps both pipeline functions at 7 arguments, under clippy's ceiling.

Blocking 2 — the rebase

Transcribed your nine steps; all nine were exactly as described, including the E0061 at the strip_process_wrapper_prefix call site and the let-chain at the wrapper probe. develop had moved 338 commits by the time I got to it rather than 304, but nothing beyond your list came out of it. develop's .or_else(|| rewrite_pipeline_producer(...)) is kept — git log | head -20 → rtk git log | head -20 still holds, pinned by the existing suite.

Thank you for the recipe; it cost no debugging pass.

Optional — all four folded in

  • exclude_commands now sits above the wrapper branch. exclude_commands = ["bash"] leaves bash -c 'git status' alone (exit 1). It is above the scan as well as the rewrite, so an excluded wrapper raises no permission surface either — test_excluded_shell_wrapper_raises_no_permission_surface.
  • is_shell_wrapper_candidate now skips the value of an option that takes a separate argument (-o, +o, -O, +O, --rcfile, --init-file), so bash -o pipefail -c, bash -O extglob -c and bash --rcfile F -c are recognised and reach Ask. bash -o pipefail script.sh is still not a wrapper. Since it is driven by the same walk it also closes behind a prefix: command bash -lc '…' asks.
  • registry.rs doc comment — |fish dropped.
  • Commit trailer — now Refs #2767.

Gates

cargo fmt --all --check                    clean
cargo clippy --all-targets -- -D warnings  clean
cargo test --all                           3,958 passed, 0 failed, 8 ignored

Linux x86_64 (NixOS). Ready for the CI run whenever suits you.

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2 of 3 — CHANGES REQUESTED

Claim: rewrite the inner commands of an exact quoted sh|bash|zsh -c wrapper, byte-preserving the wrapper itself, without letting a host allow rule approve a script the host never parsed.
Scope: accept (round 1, frozen).
Ran: develop @2bf3eb76 and head @07e17bab built into separate CARGO_TARGET_DIRs (md5 distinct). Both round-1 blockers' repros re-executed, plus the full round-1 matrix: 13 prefix families × inner-deny, 8 unsupported-option spellings × compound, all 6 agent hooks each against its own rule source, a 7,200-shape byte-preservation fuzz (4,692 rewritten), under-the-hook execution (rewrite, then run what it returns, diffed against raw), and two mutations of the new invariants. Invariants: every arm of the new decision driven — inner Deny, unparsed→Ask, Allow→Ask downgrade, excluded→no surface, non-wrapper→untouched — plus the pipeline positions, which round 1 did not drive and which is where both new blockers live. No filter output changes, so the 20% floor does not apply.
Matrix: executed round 1 @7518b894; head moved, so re-executed in full this round @07e17bab.
CI @07e17bab: run 35629235291 still action_required. Deliberately not approving it this round — the fix below moves the head, and a run on a head that is about to be replaced tells neither of us anything. I will approve the run on your next push.

Both round-1 blockers are fixed. Verified by execution, not by reading.

1. The boundary. Taking the scan off the rewrite walk instead of re-deriving it is the right call, and it is what makes the config-driven prefixes correct by construction. I re-ran the round-1 table and added devenv shell --, a second config-driven prefix that was in neither of our tables:

                                                   develop  @7518b894  @07e17bab
bash -c 'git status; rm -rf /tmp/x'                    1         2          2
command / env / exec / builtin / noglob  bash -c …     1         0          2
uv run / FOO=bar / nice / nohup / timeout 30  … -c …   1         0          2
direnv exec . bash -c 'git status; rm -rf /tmp/x'      1         0          2
devenv shell -- bash -c 'git status; rm -rf /tmp/x'    1         0          2   ← added
echo bash -c 'rm -rf /tmp/x'                           1         1          1

2. The rebase. Merges clean onto develop @2bf3eb76, nine commits past your base; .or_else(|| rewrite_pipeline_producer(...)) intact. Gates on the merged tree: fmt clean, clippy clean, cargo test --all 3,975 passed, 0 failed.

All four Optional items are folded in and verified, including that the widened option scan does not over-fire (bash -o pipefail script.sh is still not a wrapper).

Blocks merge (1 class, 2 faces — new in this round)

src/discover/registry.rs:1805 — the wrapper hook is spliced into rewrite_segment_inner above the pipeline machinery, so it neither inherits the pipeline context nor participates in the pipeline's branch order. One cause, two faces. Both are in RewriteContext/pipeline territory, which round 1 never drove — see the note at the end, this is my miss.

Face A — PipelineSafety is bypassed inside a wrapper, and a downstream consumer gets compressed bytes. rewrite_shell_wrapper re-enters through rewrite_compound, which restarts at RewriteContext::Normal, so every PipelineSafety::None rule becomes rewritable again as soon as it is wrapped:

                                     develop            @07e17bab
curl https://example.com | cat        (unchanged)        (unchanged)
bash -c 'curl https://example.com'|cat(unchanged)   →   bash -c 'rtk curl …' | cat
gh pr list | cat                      (unchanged)        (unchanged)
bash -c 'gh pr list' | cat            (unchanged)   →   bash -c 'rtk gh pr list' | cat
bash -c 'git log' | cat               (unchanged)   →   bash -c 'rtk git log' | cat
echo x | bash -c 'git log'            (unchanged)   →   echo x | bash -c 'rtk git log'

Executed, real repo, what cat actually receives:

$ bash -c 'git log' | cat                 # develop leaves this alone
commit 925c44f6e99e7664e6e459a510208eb924199108
Author: t <a@b>
Date:   Fri Sep 25 00:54:05 2026 +0200
    commit number 8 with a reasonably long subject line
…                                                         [47 lines, 1287 bytes]

$ bash -c 'rtk git log' | cat             # what the hook now hands the agent
925c44f commit number 8 with a reasonably long subject line (il y a 0 seconde…
…                                                         [8 lines, 648 bytes]

Full hashes, author and date are gone. That is the corruption PipelineSafety was added for (#3171), and it is a regression against develop, which refuses all six commands above.

Face B — the inner-deny gate is unreachable whenever the pipeline's final stage is itself rewritable. rewrite_pipeline_final_stage(...).or_else(|| rewrite_pipeline_producer(...)) short-circuits, so when the final stage rewrites, the producer is never walked and a wrapper sitting there is never recorded on the scan. Appending a final-safe consumer is enough to turn the deny off:

# deny = ["Bash(rm:*)"], allow = ["Bash(*)"]          develop  @07e17bab
bash -c 'rm -rf /tmp/x' | cat                            1         2   ← Deny, correct
bash -c 'rm -rf /tmp/x' | tail -5                        1         2   ← Deny, correct
bash -c 'rm -rf /tmp/x' | head -5                        1         2   ← Deny, correct
bash -c 'rm -rf /tmp/x' | grep foo                       0         0   ← Allow
bash -c 'rm -rf /tmp/x' | rg foo                         0         0   ← Allow
bash -c 'git status; rm -rf /tmp/x' | grep foo           0         0   ← Allow
command bash -c 'rm -rf /tmp/x' | grep foo               0         0   ← Allow
direnv exec . bash -c 'rm -rf /tmp/x' | grep foo         0         0   ← Allow

| cat denies and | grep foo auto-approves the identical inner rm -rf. develop allowed both, so this is not a regression — but it is the gate's own Deny arm being unreachable for a whole shape, which test_shell_wrapper_inner_deny_wins cannot see because it never puts the wrapper in a pipeline.

On the fix — I built one and it only covers face A, so I am not sending it as a patch. Scanning in any context but rewriting only in RewriteContext::Normal is about eight lines, and I measured it: all six face-A commands return to develop's passthrough, bash -c 'rm -rf /tmp/x' | cat still denies, plain and prefixed wrappers still rewrite (3/2 unchanged), and git log | head -20 / ls -la | grep foo are untouched. But face B is unchanged by it (| grep foo still 0), because the short-circuit is in the branch order, not in the rewrite decision — the scan has to walk the producer branch even when the final stage already succeeded. Your call whether that is one change or two; if you would rather I push the face-A half as a fixup while you take face B, say so.

Checked and correct — no need to re-verify

  • Byte preservation: 7,200 shapes (12 prefixes × 5 shells × 2 quote styles × 10 scripts × 6 pipeline positions), 4,692 rewritten, 0 anomalies — quote counts, prefix, shell token and -c preserved; nothing introduced into a single-quoted script; no stderr; no exit code outside {0,1,2,3}.
  • All six agent hooks, each against its own rule source — claude/copilot/droid/vibe via .claude/settings.json, cursor via .cursor/cli-config.json, gemini. Deny → passthrough, wrapper rewrite → ask + updatedInput, never permissionDecision: allow.
  • Under the hook: rewrite, then execute what it returns, against a real repo — six wrapper forms, exit codes identical to raw, differences are RTK's own compression.
  • Mutation testing. Making the inner-deny loop unable to fire fails three named tests; removing the Allow→Ask downgrade fails seven across permissions and all four host protocols. Both new properties are genuinely pinned.
  • Cost: hyperfine -N 6.7 ms ± 0.5 on the wrapper path, identical to an ordinary git status rewrite and inside the 10 ms target. hook_rewrite_params() goes through the process-wide cached_config(), so check_command_for adds one load per process, and discover correctly takes the _precompiled form.
  • Recursion cap, sudo/sudo -E/doas passthrough, and the RefCell sites (two statement temporaries, no borrow live across the recursive call; no panic in 7,200 cases).

Hypotheses built and dropped

  • A phantom script recorded from the abandoned .or_else branch. The direction exists — a final-stage wrapper that parses and then fails to rewrite stays recorded — but every recorded script is a real substring of a command that really runs, so there is no phantom. Recording on the parse is deliberate and can only make the verdict stricter.
  • Cursor, Droid, Gemini and Vibe emitting a rewrite for a denied command. Reproduced, then traced to my own harness: those hosts read their own rule files, not .claude/settings.json. Re-run with {"deny":["Shell(rm)"],"allow":["Shell(*)"]} in .cursor/cli-config.json, all three denied forms pass through correctly. Not a defect.
  • Inner deny at nesting depth 2 (bash -c 'git status && bash -c "rm -rf …"'). The nested script is not recorded, so the deny does not reach it; the verdict is Ask where develop returns Allow, so the new path is stricter than what it replaces. Not worth a second level — noting it so it is not re-derived.
  • The Allow→Ask cap not reaching the host when RTK produces no rewrite (bash -lc 'git status' → Defer). Real, but identical to develop, and the cap does bite in every case where RTK speaks at all — bash -lc 'git status' && cargo test is Ask on head against Allow on develop, for all eight option spellings. The README's "its strongest RTK verdict is Ask" is accurate; only the test's name is broader than what it pins.

Your questions

  • "Ready for the CI run whenever suits you." → Holding it until your next push, for the reason in the CI line above. Nothing is needed from you to make it runnable; it is mine to approve and I will.
  • The &RefCell<WrapperScan> in a Copy WrapperWalk for the two-branch call site → Right call, and keeping develop's .or_else shape rather than reshaping it for a &mut is the correct trade. I probed the failure mode that choice invites and it holds. Face B is the order of those two branches, not the sharing.

Next

Decide whether the wrapper may be entered at all outside RewriteContext::Normal (face A), and make the scan walk the producer branch even when the final stage already rewrote (face B). Everything else on this PR is verified and done — the blocking set is this one class and nothing more.

One thing I owe you: I raised this exact class at round 1 and dropped it. I had probed grep -f -, jq ., wc -l and grep b, which all happen to be near-identity filters, concluded "bypass real, breakage not reproducible", and moved on. curl, gh, docker and git log are the same class and are not near-identity, and I did not try them. Round 1 should have caught this and the second round is on me, not on you.

Rounds: 2/3. Threads: 1 open (the class above), 2 resolved this round (both round-1 blockers), 4 optional resolved.

Comment thread src/discover/registry.rs
// (`direnv exec . bash -c '…'`) is still seen, and the permission gate reads
// the same positions back off `wrappers.scan`. The wrapper depth is the only
// recursion bound that matters here.
if wrappers.may_enter()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocks merge — face A of 1 class. The wrapper hook sits here, above the RewriteContext checks further down, and rewrite_shell_wrapper re-enters through rewrite_compound, which restarts the inner walk at RewriteContext::Normal. Every PipelineSafety::None rule therefore becomes rewritable again as soon as it is inside bash -c, and a downstream consumer receives rtk-compressed bytes:

                                        develop        @07e17bab
curl https://example.com | cat          (unchanged)    (unchanged)
bash -c 'curl https://example.com'|cat  (unchanged)  → bash -c 'rtk curl ...' | cat
gh pr list | cat                        (unchanged)    (unchanged)
bash -c 'gh pr list' | cat              (unchanged)  → bash -c 'rtk gh pr list' | cat
bash -c 'git log' | cat                 (unchanged)  → bash -c 'rtk git log' | cat
echo x | bash -c 'git log'              (unchanged)  → echo x | bash -c 'rtk git log'

Executed against a real repo, what cat actually receives:

$ bash -c 'git log' | cat            # develop leaves this alone
commit 925c44f6e99e7664e6e459a510208eb924199108
Author: t <a@b>
Date:   Fri Sep 25 00:54:05 2026 +0200
    commit number 8 with a reasonably long subject line
...                                              [47 lines, 1287 bytes]

$ bash -c 'rtk git log' | cat        # what the hook now hands the agent
925c44f commit number 8 with a reasonably long subject line (il y a 0 seconde...
...                                              [8 lines, 648 bytes]

Full hashes, author and date gone — the corruption PipelineSafety was added for (#3171), and a regression against develop, which refuses all six commands above.

I prototyped the obvious fix — scan in any context, rewrite only when context == RewriteContext::Normal — and measured it: all six commands return to develop's passthrough, bash -c 'rm -rf /tmp/x' | cat still denies, plain and prefixed wrappers are unchanged, and git log | head -20 / ls -la | grep foo are untouched. It does not fix face B (see the other comment), so I am offering it as a direction rather than a patch. Happy to push just this half as a fixup if you would rather split it.

Comment thread src/discover/registry.rs
analysis,
excluded,
transparent_prefixes,
wrappers,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocks merge — face B of the same class. This .or_else short-circuits: when rewrite_pipeline_final_stage succeeds, rewrite_pipeline_producer is never called, so a wrapper sitting in the producer position is never walked and never recorded on the scan. The inner-deny gate is then unreachable, and appending any final-safe consumer is enough to switch it off:

# deny = ["Bash(rm:*)"], allow = ["Bash(*)"]       develop   @07e17bab
bash -c 'rm -rf /tmp/x' | cat                          1         2   <- Deny, correct
bash -c 'rm -rf /tmp/x' | tail -5                      1         2   <- Deny, correct
bash -c 'rm -rf /tmp/x' | head -5                      1         2   <- Deny, correct
bash -c 'rm -rf /tmp/x' | grep foo                     0         0   <- Allow
bash -c 'rm -rf /tmp/x' | rg foo                       0         0   <- Allow
bash -c 'git status; rm -rf /tmp/x' | grep foo         0         0   <- Allow
command bash -c 'rm -rf /tmp/x' | grep foo             0         0   <- Allow
direnv exec . bash -c 'rm -rf /tmp/x' | grep foo       0         0   <- Allow

| cat denies and | grep foo auto-approves the identical inner rm -rf. develop allowed both, so this is not a regression — but it is the gate's own Deny arm being unreachable for a whole shape, which test_shell_wrapper_inner_deny_wins cannot see because it never puts the wrapper in a pipeline.

The scan needs to walk the producer branch even when the final stage already rewrote. My face-A prototype does not do this — I verified | grep foo still returns 0 under it — so this is the half that genuinely needs your call on shape.

@KuSh

KuSh commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator

@SindriFr are you available and willing to work on requested changes on your PR?

@KuSh

KuSh commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator

This landed in develop via #4126 (merge 27a659f56), which carried this PR's commit 07e17bab rebased as 7cc2699a3 — same change, same ten files, with its Co-authored-by: pashifika trailer intact. Thank you for carrying it, @SindriFr.

Verified on the merged result, before → after:

bash -c 'git status; cargo test'       —  ->  bash -c 'rtk git status; rtk cargo test'
zsh  -c 'git status; cargo test'       —  ->  zsh  -c 'rtk git status; rtk cargo test'
sh   -c 'git status; cargo test'       —  ->  sh   -c 'rtk git status; rtk cargo test'
nice bash -c 'git status; cargo test'  —  ->  nice bash -c 'rtk git status; rtk cargo test'
bash -c 'git status $(whoami)'         —  ->  —   (still refused)
echo bash -c 'git status'              —  ->  —   (still not a wrapper)

Six of this PR's seven test cases behave on develop exactly as they assert here. The seventh is
fish -c 'git status; cargo test', which this PR pinned as pass-through: #4126's own subject was to
read a fish -c script as fish, so that one is deliberately superseded rather than broken — a
portable fish script now rewrites, while fish's own syntax ((cmd) substitution, and/or/end,
and the &| pipe) still defers.

Two defects were found and fixed on top of it while the stack was in review, both in code this PR
introduced, so they are worth knowing about:

  • zsh was mapped to the POSIX dialect, so zsh -c 'cargo build &| tail -3' and
    zsh -c 'ls &! ls /nonexistent' were rewritten with the pair re-spaced. Both are single
    operators in zsh — background and disown — and the second is the dangerous one: false & ! false
    exits 0 where false &! false exits 1, so a failing script would have been reported as success.
  • the wrapper's inner deny scan matched each segment only raw, so with deny: ["Bash(rm:*)"],
    bash -c 'git status; ! rm -rf …' was rewritten where the unwrapped form was denied.

Closing as landed rather than merged, since the branch here has been CONFLICTING against the
current develop and the commit is already in. Nothing further is needed from you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants