Repository navigation
feat(rewrite): rewrite inner commands of a quoted fish -c wrapper - #4126
Conversation
📊 Automated PR Analysis
SummaryRewrites the inner script of a quoted Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
There was a problem hiding this comment.
Round 1 of 3 — APPROVED
Claim: the inner commands of a quoted fish -c '…' wrapper are rewritten, with fish's own
syntax — bare (…) substitution and control keywords at a command boundary — deferring, while
every other string RTK lexes stays bash.
Scope: accept — this is exactly the narrowing round 1 of #3911 asked for, and the
measurement that motivated it holds: the (…)/keyword rules no longer reach a bash command
string. (round 1, frozen)
Ran: merge-base 0924356b, #3828's commit alone a3fa0c05, head 9ad3061f, and
head-merged-onto-b748a5f7, release-built with one CARGO_TARGET_DIR each (md5 ec117312… /
b52f3ba4… / 711e47f0… / 126ad4d3…, all four distinct). 70 hand-built rtk rewrite rows across the
claim, the fish deferrals, #3911 blocker 1's bash regressions, an adversarial fish corpus and
every operator spelling, plus a 655-script generated corpus; a round-trip differential
under real fish 4.9.1 (debian:sid, argv-logging shims) comparing what the original and
the rewritten script actually execute — on the head, 269 of the 655 are rewritten and 35 of
those run something different, every one of them a &| script; the
permission path with deny: [Bash(rm:*)] and allow: [Bash(fish:*), Bash(git:*)], through both
rtk hook claude and rtk rewrite's verdict exit codes; and the project's own /code-review high over the commit. Invariants: every branch of the new decision (Posix-attest, fish-attest,
fish-paren defer, fish-keyword defer), under the hook (rtk rewrite first, then the returned
command run under fish), split streams n/a, no capping path in the diff. Savings the rewrite
unlocks inside the wrapper: git log -20 18179 → 5062 bytes = 72.2% vs the 20% floor
(CONTRIBUTING.md).
Matrix: never executed on this PR — running now.
CI @2dbb9d16: all green — 10/10 jobs, including test (windows-latest),
test (macos-latest), test (ubuntu-latest), fmt, clippy, benchmark, both security
scans and doc review; 11/11 checks pass on the PR. This is the first CI this branch has ever
had: run 35425334245 on 9ad3061f sat at action_required and was never built, so until now
neither of us had a Windows signal. Re-verified against current develop (f5e104e1, which
carries the v0.50.0 release merge): merges clean, cargo test --all 3968 passed / 0 failed,
exit 0.
Blocks merge (1, frozen at round 1) — fixed in 2dbb9d16, which I pushed; nothing owed by you
1. src/discover/lexer.rs — fish's &| is re-emitted as & |, and fish refuses to parse it.
&| is one operator in fish: it pipes stdout and stderr together. The shared lexer reads it as
a background & followed by a pipe, and the rewrite puts them back with a space between:
$ rtk rewrite "fish -c 'git status &| cargo test'"
merge-base: <nothing> (exit 1)
head : fish -c 'rtk git status & | cargo test' (exit 3)
Under real fish 4.9.1 those two are not the same command:
$ fish -c 'echo hi &| cat' hi exit 0
$ fish -c 'echo hi & | cat' fish: Expected a string, but found a pipe
echo hi & | cat
^ exit 127
The round-trip differential says it plainly — the original runs cargo test, the rewritten
form runs nothing at all:
git status &| cargo test -> rtk git status & | cargo test
orig (exit 0): cargo <test>
after (exit 127): fish: Expected a string, but found a pipe
On develop this command passes through untouched and works. The PR routes it into the
rewrite and hands the host a script that does not parse, so it is a regression for every input
the new reach newly covers.
Class: operator spellings where the lexer's re-emission is not fish's spelling. I ran all
nine the wrapper can carry — &&, ||, ;, |, |&, &, 2>&1, 1>&2, >file — plus
their no-space forms, on both dialects. &| is the only one that changes meaning. |& and >file defer
outright; &&, ;, &, |, 2>&1 and 1>&2 rewrite to a form real fish executes
identically (spacing is normalized around the operator, the operator itself is not re-spelled).
So the class has one member, and closing it closes the class.
This one is mine, not yours. The ShellDialect shape here is the prototype I proposed in
#3911's round 1, and it had the same hole — I enumerated fish's control flow and substitution
and never looked at its operators. So I have written and verified the fix rather than sending
it back:
contains_fish_only_construct also returns true when a & token is immediately followed by a
pipe token, which is exactly &| and nothing else. POSIX wrappers keep rewriting it, correctly:
bash parses a &| b and a & | b identically — both are syntax errors, exit 2 — so nothing
changes there.
head + fixup
fish -c 'git status &| cargo test' rtk git status & | cargo test — (defers)
fish -c 'git status&|cargo test' rtk git status & |cargo test — (defers)
fish -c 'git status & cargo test' rtk git status & rtk cargo test unchanged
fish -c 'git status | cat' rtk git status | cat unchanged
bash -c 'git status &| cargo test' rtk git status & | cargo test unchanged
git log -20 && (cd www && npm test) rtk git log -20 && (cd …) unchanged
With it, the same 655-script differential rewrites 234 and 0 run something different.
cargo fmt --all clean, cargo clippy --all-targets -- -D warnings clean, cargo test --all
3886 passed / 0 failed / 8 ignored, exit 0, with two new assertions pinning it at the lexer and
the wrapper level. A second pass by someone who had not seen my reasoning re-derived the defect,
diffed 130 further command strings between the two binaries and found no difference outside a
real unquoted &| in a fish wrapper — including the false-positive traps (&| inside single
and double quotes, \&\|, "a&"|cat) and multibyte bytes immediately before the &.
Optional — will not hold merge; I will resolve these threads
contains_fish_only_constructtokenizes withNewlineMode::Conservativeand its doc says
"Newlines are boundaries here, so a keyword opening a line of a multi-line script is seen" —
but the only caller rejectsscript.contains(['\n', '\r'])two lines earlier in the same
ifchain, so the fish check never sees a newline. Swapping the mode toNewlineMode::None
changes no test (3961 passed either way). The code is right if the newline rejection is ever
relaxed; the comment just reads as a description of live behaviour, which it is not.- Adding
fishtoSUPPORTED_SHELLSalso makesis_shell_wrapper_candidatetrue for it, so a
fish -cwrapper can never take RTK's auto-allow. Checked and it costs nothing: with
allow: [Bash(fish:*)], RTK emitted nopermissionDecisionforfish -c 'git status'on
either binary, so the host applies its own rule exactly as before.
Checked and correct — no need to re-verify
- #3911's blocker 1 is gone. All ten bash rows it measured as lost now match the merge-base
byte for byte on the head:(cd sub && git status),git log -20 && (cd www && npm test),
git status; if true; then echo x; fi,cargo build; for f in *.rs; do echo $f; done,
git status; and cargo test, both#-comment lines, and the POSIX wrapper forms. - The fish wrapper is never auto-allowed.
check_shell_wrapper_permissionsreturnsAsk
for any wrapper it parses; throughrtk hook claude,fish -c 'git status'comes back with
updatedInputand nopermissionDecision, where a baregit statuscarries
"permissionDecision":"allow". - Fish's single-quote escape cannot reach the rewrite.
find_script_endrefuses$,
backtick and\inside a double-quoted wrapper, and a single-quoted wrapper cannot contain a
'at all — so the one place fish and bash disagree about closing a quote is unreachable. - The keyword list is complete for the shapes that matter.
command,builtin,exec,
time,break,continue,returnare not in it and do not need to be: each rewrites to a
form real fish executes identically (verified in the container).break/continue/return
need an enclosing block, whose opener is already refused. - Merges clean onto current
develop(b748a5f7):cargo test --all3961 passed, 0
failed, exit 0, and everycfg(windows)path type-checks. - The docs match the code. Every claim in the README,
src/discover/README.md,
src/hooks/README.mdand TECHNICAL.md hunks was run against the binary.
Hypotheses built and dropped
-
"Adding fish to the wrapper machinery lets a command escape a deny rule." The deep
review flagged thatsplit_for_permissionsis bash-shaped, soand rm -rf …is one segment
and the deny scan misses therm. True, but not a regression and not fish-specific — at the
real entry point, withdeny: [Bash(rm:*)](rtk rewriteexit code carries the verdict:
2 = Deny, 3 = Ask, 1 = defer):merge-base head rm -rf /tmp/zz DENY DENY fish -c 'true; rm -rf /tmp/zz' defer DENY ← new fish -c 'echo (rm -rf /tmp/zz)' defer DENY ← new bash -c 'true && rm -rf /tmp/zz' defer DENY ← new (#3828) fish -c 'true; and rm -rf /tmp/zz' defer defer bash -c 'true; and rm -rf /tmp/zz' defer deferThe head adds three denials and removes none. The
androw is unchanged from the merge-base
and behaves identically underbash -c, because in bashandis just a command name and
rmis its argument — so this is a property ofsplit_for_permissions, not something fish
introduces. Same conclusion as #3911's round-1 note, and the PR body already states it. -
"The
src/hooks/README.mdhunk overstates what fish gets." It says RTK checks deny rules
"against both the outer wrapper and the parsed inner segments", which is exactly what happens;
it does not claim the inner segmentation is fish-aware, and the limitation is identical for
the POSIX wrappers the sentence already covered. Left as written. -
"Some other fish operator is mis-spelled by the rewrite too."
&&|,&||,;&|,>&|,
2>&|, a trailing&|and&|&do not all take the new deferral — but each of them is
already a fish parse error before any rewrite (fish -n, exit 127), so no runnable script is
affected either way. -
"A fish keyword after a separator the bash lexer does not split on will slip through."
Ran the no-space and unusual-separator forms —;and,;and,&& not,|| and,
| not,& and,>/dev/null; and— all defer. The tokenizer emits an operator for each of
them, so command position is reached. -
"The rebase silently re-attributes #3828's work." It does not:
07e17babis already
authored by pashifika and committed by Sindri Frostason, anda3fa0c05keeps the same author
and the sameCo-authored-bytrailer. Only the committer changes, as any rebase does. -
"A rewrite rule emits POSIX-only syntax into what is now a fish script." No rule in
rules.rsemits a shell operator; the rewrite only prefixesrtkand normalizes a few
wrapper paths. -
"
^is a fish stderr redirect and the rewrite will move it."fish -c 'git status ^err.txt'
rewrites tortk git status ^err.txt, which means the same thing under fish 2.x (redirect
applies tortk, which propagates) and under fish 3+ (a literal argument). Not reachable as a
defect. -
"
src/core/filter.rsmapssh|bash|zshtoLanguage::Shelland should learnfish."
Different subsystem (language-aware filtering of file contents), untouched by this PR, and
pre-existing. Not this PR's job. -
"Another open PR on these files conflicts." #4129–#4138 touch
lexer.rsand
registry.rsbut none of them redefine the gate,SUPPORTED_SHELLSorShellWrapper—
their only overlap with those symbols is a doc-comment context line. They are mine to rebase.
Your questions
- "[ ] CI for Linux and Windows" → Closed, and it was never your fault. No CI run had
ever been created for this branch —35425334245sat ataction_requiredon9ad3061fand
was never built, which is why the box stayed unticked through #3911 too. I approved the run on
the fixup head:test (windows-latest),test (macos-latest)andtest (ubuntu-latest)all
pass, as dofmt,clippy,benchmark, both security scans anddoc review. You now have
the Windows signal you asked for.
Next
Nothing — approving. Fixup 2dbb9d16 pushed to your branch: a fish &| now defers instead of
being re-emitted as & |, plus two assertions pinning it at the lexer and the wrapper. It is a
behaviour change rather than pure wording, so if you would rather have written it yourself, or
disagree with deferring instead of re-spelling the operator, say so and I will unwind it — it is
your PR and the defect was mine.
This merges as soon as #3828 does.
Rounds: 1/3. Threads: 1 resolved this round (the &| blocker, closed by the fixup), 0 open.
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). 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 parsed 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. Closes rtk-ai#2767 Co-authored-by: pashifika <pashifika.mkii@gmail.com>
The wrapper names the shell, so the script inside `fish -c '…'` is the one
string in the pipeline whose dialect RTK knows. The parser now reports it
(`ShellWrapper::dialect`) and the gate takes it
(`contains_unattestable_construct_in`): in fish, `(cmd)` is command
substitution rather than a subshell and `and`, `or`, `end` are control flow
rather than command names, so a script using either defers instead of being
rewritten under POSIX assumptions.
fish -c 'git status; cargo test' → fish -c 'rtk git status; rtk cargo test'
fish -c 'git status; and cargo test' unchanged
fish -c 'git status (pwd)' unchanged
fish -c 'if test -d src; git status; end' unchanged
The fish rules reach that script and nothing else. A host's command string is
still lexed as bash — it is what the agent's Bash tool ran — so
`(cd sub && git status)`, `git log -20 && (cd www && npm test)` and
`git status; if true; then echo x; fi` keep rewriting, and so does the script
of a `sh`/`bash`/`zsh` wrapper.
Refs rtk-ai#2767
`&|` is one operator in fish: it pipes stdout and stderr together. The
shared lexer reads it as a background `&` followed by a pipe, and the
rewrite re-emits the two spaced, which fish rejects outright:
fish -c 'cargo build &| tail -20'
-> fish -c 'rtk cargo build & | tail -20'
fish: Expected a string, but found a pipe
The command stops running altogether where it used to work. Treat the
pair as fish-only syntax, alongside bare `(…)` substitution and the
control keywords, so the script passes through untouched.
POSIX wrappers keep rewriting it: bash parses `a &| b` and `a & | b`
identically (both are syntax errors), so nothing changes there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ammar from an inner segment
Two defects in this PR, found by independent review of the stack built on it.
**`zsh -c '… &| …'` and `… &! …` are rewritten into a different script.**
Both are single operators in zsh — background and disown — and this lexer
reads each as a background `&` plus a pipe or a negation, so the rewrite
re-emits them spaced:
zsh -c 'cargo build &| tail -3' -> 'rtk cargo build & | tail -3'
zsh: parse error near `|'
zsh -c 'ls &! ls /nonexistent' -> 'rtk ls & ! ls /nonexistent'
original exits 1, rewrite exits 0
The second is the dangerous one: the spacing turns the following command into
a negated one, so a failing script is reported to the agent as success. `zsh`
was mapped to `ShellDialect::Posix`, which refuses neither, and the fish arm
only looked for `&|`. zsh now names its own dialect and the check covers both
pairs for every dialect that is not Posix — bash has neither operator, where
`a &| b` is a syntax error spaced or not, so nothing changes there.
**A denied command behind grammar escapes the wrapper's inner check.** The
outer gate matches each segment twice, raw and through
`strip_grammar_residue`; `check_shell_wrapper_permissions` matched only the
raw form, so with `deny: ["Bash(rm:*)"]`:
bash -c 'git status; ! rm -rf victim' rewritten (exit 3)
bash -c 'git status && { rm -rf victim; }' rewritten (exit 3)
bash -c 'git status && (rm -rf victim)' denied (exit 2)
git status; ! rm -rf victim denied (exit 2)
Inconsistent across spellings of the same thing, and on Copilot IDE the
`{ … }` form rendered as a deny whose reason told the agent to re-run
`bash -c 'rtk git status && { rm -rf victim; }'` — RTK suggesting the command
the rule forbids. The inner loop now strips the same residue the outer one
does, which is what `src/hooks/README.md` already claimed.
Rebased onto `develop`; `tests/rewrite_shell_wrapper_test.rs` goes through
`common::rtk_command()` for the test-isolation guard that landed meanwhile.
2dbb9d1 to
b969c3a
Compare
|
@KuSh — per your round-5 note on #4127, the two items you assigned here are pushed as
Both are background-and-disown in zsh and two tokens to this lexer, so the rewrite re-spaces them; the second turns the following command into a negated one, so a failing script is reported to the agent as success. The wrapper's inner deny check. It matched each inner segment raw, while the outer gate matches raw and through On Copilot IDE the Also in the rebase: #4127 is rebased on this head and carries nothing of it. |
Round 2 — APPROVEDClaim: the inner commands of a quoted Scope: accept (round 1, frozen). Ran: four release builds, one Pins: 7 decisions, 7 mutated, 1 survived — Matrix: executed this round; pin check: never before — executed this round. CI @ Blocks merge (0)The approval already on this PR stands and covers this head — GitHub kept it across your push, so
The r1 head rewrote both; this head defers both; The inner deny strip is consistent across spellings now — The class call in my round-1 review was wrong. I wrote that the operator class "has one member, and closing it closes the class" after enumerating nine spellings — but I enumerated them against Filed as follow-ups
Checked and correct — no need to re-verify
Your questions
NextNothing — approving. This no longer waits on #3828: it carries #3828's single commit with @pashifika's authorship intact, and six of #3828's own seven test cases behave on this head exactly as #3828 asserts while none of them works on Rounds: 2. Threads: 0 open, 0 resolved this round. |
#4126 made the rewrite descend through prefixes (env, assignments, timeout, nice, nohup, time, exec, command, uv run, configured transparent_prefixes) to a quoted shell wrapper, but the permission gate still looked for the wrapper at the segment start only. A denied inner command rode the outer allow: `env bash -c 'git push'` under `allow env:*` + `deny git push:*` was rewritten and auto-allowed. The gate now checks deny rules against the wrapper's script at every position the rewrite reaches (`registry::shell_wrapper_positions`, the rewrite's own prefix walk), on the text the rewrite reads: continuations joined, segments not cut at a redirect (`lexer::split_for_wrappers`). A match returns the new `PermissionVerdict::Defer`: no rewrite, no decision, the host sees the command as typed — master's behaviour, instead of #4126's Deny. Nothing else changes: a prefixed wrapper otherwise keeps the outer command's verdict, and a wrapper at the segment start keeps develop's rule. Fixes the release blocker reported on #4399. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
#4126 made the rewrite descend through prefixes (env, assignments, timeout, nice, nohup, time, exec, command, uv run, configured transparent_prefixes) to a quoted shell wrapper, but the permission gate still looked for the wrapper at the segment start only. A denied inner command rode the outer allow: `env bash -c 'git push'` under `allow env:*` + `deny git push:*` was rewritten and auto-allowed. The gate now checks deny rules against the wrapper's script at every position the rewrite reaches (`registry::shell_wrapper_positions`, the rewrite's own prefix walk), on the text the rewrite reads: continuations joined, segments not cut at a redirect (`lexer::split_for_wrappers`). Inside the script the same walk is applied before matching, so a prefix there (`exec git push`, `timeout 5 git push`, `FOO=1 git push`) reaches the rule too. A match returns the new `PermissionVerdict::Defer`: no rewrite, no decision, the host sees the command as typed — master's behaviour, instead of #4126's Deny. Nothing else changes: a prefixed wrapper otherwise keeps the outer command's verdict, and a wrapper at the segment start keeps develop's rule. Fixes the release blocker reported on #4399. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Summary
PR B of the three-way split of #3911, per @KuSh's round-1 review — the part he asked to keep: "rewrite the inner commands of a quoted
fish -c '…'wrapper, deferring on fish control flow and bare(…)inside that wrapper's script".Two commits:
develop. @SindriFr's PR has beenCONFLICTINGsincedevelopgainedRewriteContext, therewrite_pipeline_stage/rewrite_pipeline_producersplit (Tracking issue for safe pipeline rewriting #3171),hooks/decision.rsand let-chains, soshell_wrapper_depthis re-threaded through the current shape and thehead fooexpectations followdevelop's--head-lines 10default. Authorship and message are unchanged. If fix(rewrite): rewrite inner commands of quoted sh/bash/zsh -c wrappers #3828 lands first, this commit drops out of the diff; if it is easier to land this one, fix(rewrite): rewrite inner commands of quoted sh/bash/zsh -c wrappers #3828 closes with it.feat(rewrite): read a quoted fish -c script as fish— the actual subject of this PR, ~100 lines.The fish half, scoped to the wrapper
#3828 left
fishout because the shared lexer is bash-shaped and could not attest fish's syntax. The fix is not to teach that lexer a second grammar — it is to notice that the wrapper names the shell:shell_wrapper.rskeeps the matched shell (ShellWrapper::dialect()), whichsupported_shell_endpreviously discarded;lexer.rsgainsShellDialectandcontains_unattestable_construct_in(cmd, dialect)— fish adds bare(…)substitution and a control keyword (and,or,not,begin,end,if,else,switch,case,for,while,function) at a command boundary;registry.rs::rewrite_shell_wrapperpasseswrapper.dialect()for the inner script.The fish rules reach that script and nothing else. A host command string stays bash — it is what the agent's Bash tool ran, and
is_word_boundary_whitespace,advance_quote_stateandNewlineMode::Bashall model bash. Verified against this branch's binary, isolatedHOME/XDG_CONFIG_HOME:Those are exactly the lines blocker 1 measured as lost, plus blocker 2's comment cases.
test_fish_rules_do_not_reach_bash_commands(registry) andtest_posix_dialect_keeps_subshells_and_keywords_attestable(lexer) pin them so the reach cannot come back silently.What is deliberately not here
(…)/keyword deferral. Dropped, not relocated. It existed in feat(shell)!: explicit shell execution, fish-aware hook gate, and fish -c wrapper rewriting #3911 to serve the fish-script wrapping feature, which is now PR C's subject.has_unclosed_quote_or_escapein the global gate (the review's PR D, and the$'\''auto-allow hole). Not in this PR. @KuSh has an unmerged branch closing the same hole (ansi_c_quote_defeats_lexer) including the#-comment fix blocker 2 describes — that one should land on its own; nothing here depends on it.split_for_permissionsstays bash-shaped, so withdeny: ["Bash(rm:*)"],fish -c 'true; and rm -rf /tmp/zz'is not denied by RTK — but it is also not rewritten (the dialect gate refuses it), so the host evaluates the original command exactly as it does today.fish -c 'rm -rf /tmp/zz'is denied, as are thebash -cforms. No regression against the merge-base, which denied none of them.Key files
src/discover/shell_wrapper.rsSUPPORTED_SHELLScarries a dialect per shell;ShellWrapper::dialect()src/discover/lexer.rsShellDialect,contains_unattestable_construct_insrc/discover/registry.rstests/rewrite_shell_wrapper_test.rsTest plan
cargo fmt --all --check— cleancargo clippy --all-targets -- -D warnings— cleancargo test --all— 3,885 passed, 0 failed, 8 ignoredrtk rewritetranscript above, on this branch's binary vs the behaviour feat(shell)!: explicit shell execution, fish-aware hook gate, and fish -c wrapper rewriting #3911 regressedTested on macOS 26.6.2 arm64, rustc 1.97.1.
Split siblings: A — #4125, the argv/
--shellchange with the exit-127 fix (blocker 3), independent of fish. C — fish-script wrapping at the hook gate, opened separately so its premise can be argued without holding A and B.Rebased onto
develop@0924356.