Skip to content

feat(hooks): wrap unambiguously-fish scripts as rtk run --shell fish - #4127

Merged
KuSh merged 9 commits into
rtk-ai:developfrom
pashifika:feat/fish-script-wrapping
Oct 23, 2026
Merged

KuSh merged 9 commits into
rtk-ai:developfrom
pashifika:feat/fish-script-wrapping

Conversation

@pashifika

Copy link
Copy Markdown
Contributor

Summary

PR C of the three-way split of #3911, per @KuSh's round-1 review — the part he asked to argue on its own: "should RTK infer a command's shell?". Opened as a PR rather than an issue so the behaviour is reviewable alongside the question; say the word and I will move the discussion to an issue instead.

Stacked on #4125 (A) and #4126 (B), whose commits are the first four in this diff — C's own change is the last two. It needs A for rtk run --shell fish -c, the form it emits, and B for the wrapper parser it uses to recognise a command that already delegates.

The premise, stated plainly

A host can evaluate the Bash tool's command string with a POSIX/zsh layer while the user's shell is fish. A fish script then fails to parse before RTK is consulted at all — nothing downstream can recover, because the command is already gone by the time anyone sees the error. That is an agent putting fish syntax into a tool called Bash; @KuSh is right that compensating for it is a policy choice, not a bug fix. What this PR argues is that the choice is cheap, reversible and precise:

if test -d src
  git status
end
# → rtk run --shell fish -c 'if test -d src\n  git status\nend'
  • Precise: a script is wrapped only when it is provably fish — a fish-only keyword (end, begin, switch, and, or, not) at a command boundary, and nothing a POSIX shell needs anywhere: then, fi, do, done, esac, elif, [[, a heredoc, or a backtick (added this round, from the review's optional list: fish has no backtick substitution, so echo `hostname`; and echo ok no longer classifies). @KuSh's own measurement on 494 real command lines from this repo's docs: 0 wrapped.
  • Never a promotion: the wrap's strongest verdict is Ask, never AllowRewrite, because the script's content is not attested. A deny rule outranks it. fish_wrap_is_never_auto_allowed and deny_outranks_the_wrap pin both.
  • Reversible: wrap_fish_scripts = false under [hooks], and it is skipped entirely on Windows or without a resolvable fish binary.
  • Byte-exact: the script travels inside one quoted argument; \\ and \' — the only two sequences where fish single-quote semantics diverge from POSIX — are refused, so every wrapped script round-trips identically under sh, zsh and fish. That round-trip is tested against the real shells (wrapped_quoting_round_trips_under_posix_and_fish_hosts); @KuSh independently verified it in debian:sid.

What changed since #3911

The global (…)/control-keyword deferral is gone, not relocated. In #3911 it was what made a bare fish script reach the wrap, and it cost every ordinary bash line containing a subshell or a keyword block its rewrite (blocker 1). The trigger is now the classifier itself, asked first in hooks::decision::decide, so:

if test -d src\n  git status\nend      → rtk run --shell fish -c '…'        (exit 3)
test -d src; and git status            → rtk run --shell fish -c '…'        (exit 3)
echo `hostname`; and echo ok             unchanged                          (exit 1)
git status; if true; then echo x; fi   → rtk git status; if true; …         (exit 3)
(cd sub && git status)                 → (cd sub && rtk git status)         (exit 3)

The last two are the regressions blocker 1 measured; they are gone because nothing in this PR touches the shared gate any more.

develop has since merged HookDecision and RewriteOutcome into hooks/decision.rs, so the two copies of the wrap-at-gate from #3911 collapse to one site (decide_with_wrap), injected the same way the verdict and rewrite parameters already are (#3146) — tests never depend on the machine's fish or config.

Key files

File Purpose
src/discover/fish_script.rs Classification, POSIX/backtick vetoes, divergent-backslash veto, gate ordering, wrap assembly
src/hooks/decision.rs decide_with_wrap — one site, ahead of the rewrite rules, Ask-only
src/core/config.rs hooks.wrap_fish_scripts (defaults to enabled)
tests/fish_wrap_test.rs E2E wrap, opt-out, backslash veto, cross-host quoting round-trip, real-fish execution (#[ignore])

Test plan

  • cargo fmt --all --check — clean
  • cargo clippy --all-targets -- -D warnings — clean
  • cargo test --all — 3,947 passed, 0 failed, 9 ignored
  • rtk rewrite transcript above on this branch's binary, isolated HOME/XDG_CONFIG_HOME
  • rtk run --shell fish -c 'if test -d src; echo yes; end' executes and exits 0
  • CI for Linux and Windows

Tested on macOS 26.6.2 arm64, rustc 1.97.1, fish 4.9.0.


Split siblings: A — #4125 (argv/--shell, exit-127 fix), B — #4126 (fish -c wrapper rewriting, dialect-scoped). #3911 is closed in favour of the three.

@rtk-wshm-sync-bot

Copy link
Copy Markdown

wshm · Automated triage by AI

📊 Automated PR Analysis

✨ Type feature
🟡 Risk medium

Summary

Adds a policy to detect unambiguously-fish shell scripts (using fish-only keywords/operators with no POSIX markers) and rewrite them to explicit rtk run --shell fish -c execution, preventing parse failures when a host evaluates fish syntax with a POSIX/zsh layer. It is the third of a three-way split of a prior PR, stacked on two sibling PRs, is opt-out via config, restricted to non-Windows with a resolvable fish binary, and only ever produces an 'ask' rewrite verdict.

Review Checklist

  • Tests present
  • Breaking change
  • Docs updated

Analyzed automatically by wshm · This is an automated analysis, not a human review.

@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: a command string that is provably fish — a fish-only keyword at command position, no POSIX disambiguator anywhere — is handed back as rtk run --shell fish -c '<script>' ahead of the rewrite rules, Ask-only, opt-out via hooks.wrap_fish_scripts.

Scope: accept. This is exactly the PR C the #3911 split asked for, and opening it as a PR rather than an issue was the right call — the measurements below only exist because there was a branch to run. #3911's blocker 1 is verifiably gone: the global (…)/control-keyword deferral is absent rather than relocated, and every bash row it used to cost a rewrite matches plain develop byte for byte. Merge order: #4125 and #4126 first (this is stacked on both); #3909 also rewrites src/hooks/decision.rs (+220/−11) around the same verdict handling and is not blocked on anything, so whichever of the two goes second will rebase.

Ran: rtk rewrite and rtk hook <host> on three binaries — plain develop b748a5f, the #4125+#4126 stack 5b547c9, this head f3347cf — across 16 matrix rows, 19,870 real agent-emitted Bash commands, 3,733 repo command lines and 5,818 adversarial fuzz cases; end-to-end execution against real fish 4.9.1 under sh/bash/dash/zsh/fish host layers; 7 mutations of the classifier's guards. Invariants: every branch of the new decision (Default/Allow/Deny × wrap on/off × fish present/absent), under the hook (rtk rewrite first, then the form it returns), split streams, over-cap input (2.7 MB and binary stdout), non-UTF-8, BOM and CRLF payloads. No filter is added, so the 20% floor (CONTRIBUTING.md) has no filter to measure — but blocker 2 is a savings regression against develop.

CI @f3347cf8: never ran. Run 35425641688 sits at action_required; only license/cla is green and check-target is SKIPPED. It needs approving before any of this can be signed off.

Blocks merge (3, frozen at round 1)

  1. src/discover/fish_script.rs:139 — comment text is classified as code (1 site). echo $((1+2)) # x; and y → bash prints 3, head wraps it, fish exits 127 and nothing runs. A working command becomes a hard failure. Fix: strip unquoted word-initial # to newline before tokenizing.
  2. src/hooks/decision.rs:110 — the wrap costs the rewrite develop performs (27 of 28 leading commands; 10 of 10 three-way). git diff HEAD~3 HEAD; and true: 76.9% saved on develop → 0% here.
  3. src/hooks/decision.rs:111 — the wrap is the only path that rewrites a command RTK could not attest; on Codex that renders as a protocol-level allow. Deny itself still wins — this is not a deny bypass.

Details and transcripts are in the three line comments.

Optional — will not hold merge

  • hooks.exclude_commands and transparent_prefixes are bypassed, and decide's numbered gate-order doc still lists 1. Deny / 2. unattestable / 3. rewrite with no mention of the new gate between 1 and 2. Both fields are read by rewrite_command, which the wrap precedes: test -d src; and curl https://x wraps even with exclude_commands = ["curl"]. wrap_fish_scripts is the dedicated switch, so this may well be intended — one line in that doc list either way.
  • tests/fish_wrap_test.rs asserts nothing on any runner without fish: all five tests return early on which::which("fish").is_err(), the sixth is #[ignore]. Forcing try_wrap to return None leaves 5/5 green. The classification logic itself is properly covered — mutating each of the seven guards (backtick, heredoc, POSIX keyword, [[, \\/\', unclosed quote, Ask-not-Allow) fails a named unit test — so this is an end-to-end gap rather than a coverage hole. A fish step in CI, or an assertion on the fish-absent branch, would stop the file being inert.

Checked and correct — no need to re-verify

  • Byte-exact round trip. 9 scripts (multiline if/for/switch/begin, ; and, ; or, not, embedded single quotes, printf '%s\n') wrapped and executed under sh, bash, dash, zsh and fish: stdout identical to fish -c '<script>' in every cell.
  • IO fidelity through rtk run --shell fish -c. 2 688 900 B / 400 001 lines identical; 2 000 B of /dev/urandom identical; large stderr identical; exit codes 0/1/7 propagated; stdin forwarded. No cap, no truncation, nothing to lose.
  • Precision on real input. 0 wraps over 19,870 agent-emitted Bash commands from 195 Claude Code transcripts, and 0 real commands over 3,733 repo command lines (the 2 hits there are doc prose and a bare end in a fish snippet). Blocker 1 is a mechanism defect, not something this corpus happens to trigger.
  • Gates. cargo fmt --all --check and cargo clippy --all-targets -- -D warnings clean; cargo test --all on the head merged onto latest develop (merge is clean): 4 023 passed, 0 failed; cargo check --target x86_64-pc-windows-gnu --all-targets clean.
  • Config. Missing field defaults true; wrap_fish_scripts = false disables the wrap and survives a load-mutate-save round trip.
  • Decision branches. Deny → Deny; Allow → AskRewrite (never promoted); Default → AskRewrite; wrap off → prior path; fish absent from PATH → prior path. The wrap is idempotent — the emitted form re-enters as itself and suppress_identity defers it.
  • Perf. No regression: 6.81 / 6.72 / 1.35 ms against develop's 7.14 / 6.77 / 1.40 ms, 200 runs each. The config read and the fish probe are FnOnce behind the classifier, so non-fish commands pay nothing.
  • Host payloads. claude / copilot / cursor carry the wrap in updatedInput without asserting a permission decision; BOM, CRLF and multiline scripts all render correctly.

Hypotheses built and dropped

  • Injected-marker false positives. 5,818 fuzz cases, 5,258 wrapped, 246 where bash exits 0 and fish diverges — every one needs a bare marker keyword as a standalone command (command -v git || end), which bash reaches only as dead code after a successful left side. No real command has that shape. (Blocker 1 is a different mechanism and is real.)
  • gemini / codex get no benefit from the wrap. True, but {"decision":"allow"} and empty output are what both develop and this head emit for a plain git status as well — pre-existing per-host rendering. The Codex point that does survive is in blocker 3.
  • has_unclosed_quote_or_escape disagreeing with the lexer. Escape-before-quote ordering and the quote != Some('\'') rule are identical in both scans; no input found where they disagree. A quote inside a comment suppresses classification (ls # don't; and rm -rf / → unclosed → no wrap), which is the safe direction.
  • HooksConfig's hand-written Default drifting from the struct. It is an exhaustive struct literal; a new field is a compile error, not a silent false.

Your questions

  • "say the word and I will move the discussion to an issue instead" → keep it here. The behaviour is what made the question answerable.
  • "should RTK infer a command's shell?" → on this evidence, yes. The classifier is precise on real input, the wrap is reversible, Ask-only, byte-exact across five host layers, and costs nothing when it does not fire. None of the three blockers is an argument against inferring the shell; they are all about where the inference is placed — ahead of the comment stripper it does not have, ahead of the rewrite rules, and ahead of the attestation gate.

Next

Strip # comments before classifying (blocker 1) — a correctness bug and the smallest of the three. Then decide blocker 2 (nest the rewrite, or document the loss) and blocker 3 (gate behind contains_unattestable_construct, or document the exemption). Everything else is green; CI still has to be approved and pass on the resulting head.

Rounds: 1/3. Threads: 3 open, 0 resolved — no prior threads on this PR.

Comment thread src/discover/fish_script.rs
Comment thread src/hooks/decision.rs Outdated
Comment thread src/hooks/decision.rs Outdated
pashifika added a commit to pashifika/rtk that referenced this pull request Sep 22, 2026
…e refuses

Round-1 review of rtk-ai#4127, three defects, one mechanism each.

**Comment text was read as code.** Neither the classifier nor the shared lexer
models `#`, so a separator inside a trailing comment reset command position and
the next English word was matched against the fish markers:
`echo $((1+2))  # x; and y` is a working bash command that was wrapped and then
failed under fish with nothing run at all. Every shell in play agrees where a
comment starts and ends, so the span is dropped before anything is classified.
A `#` inside quotes is still an argument, and a fish script may carry a comment
of its own.

**The wrap cost the rewrite.** It preceded `rewrite_command`, so a fish chain
whose leading command was rewritable lost the savings `develop` delivers —
`git diff HEAD~3 HEAD; and true` went from 76.9% to 0%. The script's own
commands are rewritten first and the wrap carries the result, honouring
`exclude_commands` and `transparent_prefixes` like any other rewrite.

**It was the only path emitting a command RTK could not attest.** It now
refuses everything the unattestable gate refuses — command and process
substitution, file-target redirects — plus fish's own `(cmd)` substitution,
which the bash-shaped lexer reads as a subshell and lets past. What still
reaches the wrap is a script whose only unattestable property is fish control
flow, which is the case the feature exists for, and `src/hooks/README.md` now
records why that one is exempt, what bounds it, and that Codex renders `Ask` as
a protocol-level `allow`.

Also from the review: `tests/fish_wrap_test.rs` asserts on the fish-absent
branch instead of returning early, so the file is no longer inert on a runner
without `fish`, and `decide`'s gate-order doc lists the wrap.
@pashifika
pashifika force-pushed the feat/fish-script-wrapping branch from f3347cf to d87d12d Compare September 22, 2026 02:13
@pashifika

Copy link
Copy Markdown
Contributor Author

@KuSh — three blockers fixed on d87d12d, and the branch is rebased on the round-2 heads of #4125 and #4126.

Blocker 1 — comment text is no longer code. is_unambiguous_fish drops every unquoted, word-initial # through the end of its line before tokenizing, exactly as you suggested; sh, bash, zsh and fish all agree on that span. Your three cases, on this head:

echo $((1+2))  # x; and y                  unchanged                       (exit 1)
echo "${HOME}"  # note; and more           unchanged                       (exit 1)
ls -la  # long listing; and hidden files → rtk ls -la  # long listing; …   (exit 3)

The third is the one that shows the fix is not just "refuse more": it rewrites as bash, byte-identical to develop. test_comment_text_is_never_a_marker pins all four (yours plus ls # don't; and rm -rf /), and test_comment_stripping_leaves_code_alone pins the two directions a naive strip would break — rg '#end' src/ stays an argument, and a fish script with a comment of its own still classifies.

Blocker 2 — the wrap keeps the rewrite. The script's own commands are rewritten before it is wrapped, so the savings you measured come back:

git diff HEAD~3 HEAD; and true
  → rtk run --shell fish -c 'rtk git diff HEAD~3 HEAD; and true'    (exit 3)

It runs through rewrite_command with the same excluded / transparent_prefixes the unwrapped path gets, which also closes the exclude_commands bypass from your optional list — the_nested_rewrite_honours_exclusions pins that, and rewrite_keeps_the_inner_rewrite_inside_the_wrap covers it end to end.

Blocker 3 — the wrap now refuses what the gate refuses. Rather than move it behind contains_unattestable_construct (which would kill the feature: ; and and if … end are exactly what the gate cannot decompose), it applies that gate's own rules to the script itself — command and process substitution, file-target redirects — plus fish's (cmd) substitution, which the bash-shaped lexer reads as a subshell and lets through:

test -d src; and cat /etc/passwd > /tmp/leak     unchanged   (exit 1)   ← your case
test -d src; and echo $(whoami)                  unchanged   (exit 1)
for f in (ls)\n  echo $f\nend                    unchanged   (exit 1)
test -d src; and git status                    → wrapped     (exit 3)
if test -d src\n  git status\nend              → wrapped     (exit 3)

What still reaches the wrap is a script whose only unattestable property is fish control flow — the case the feature exists for. src/hooks/README.md now states why that one is exempt, in your terms: deferring loses the command outright rather than leaving it unrewritten, the script travels byte-identical, the verdict is Ask and deny wins first, and the residual exposure is a host that treats an rtk-prefixed command as pre-approved — naming Codex, its protocol-level allow, and hooks.wrap_fish_scripts = false as the switch for anyone who does not want that trade. test_unattestable_scripts_are_not_wrapped and rewrite_defers_unattestable_fish_scripts pin it at both levels.

Also from the optional list: decide's numbered gate-order doc lists the wrap as gate 2 and says where the reasoning lives; and tests/fish_wrap_test.rs asserts on the fish-absent branch (defer, exit 1, no stdout) instead of returning early, so the file is no longer inert on a runner without fish. A CI fish step would still be better than either branch — happy to add one if you want it in this PR rather than separately.

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

(macOS 26.6.2 arm64, rustc 1.97.1, fish 4.9.0.) Keeping the discussion here as you asked. CI has still never been approved on this branch — that is the last thing outstanding that I cannot do myself.

@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: a command string that is provably fish is handed back as rtk run --shell fish -c '<script>' ahead of the rewrite rules, Ask-only, opt-out via hooks.wrap_fish_scripts.

Scope: accept (round 1, frozen).

Ran: rtk rewrite and rtk hook <host> on four binaries — plain develop feac25d, the #4125+#4126 stack 9df8621, the round-1 head f3347cf, this head 3de8bed — across the round-1 matrix, the three blockers' own repros, 19,870 real agent-emitted Bash commands, and 1,825 new comment/quote fuzz cases built as region × separator × marker; end-to-end execution against real fish 4.9.1 under sh/bash/dash/zsh/fish. Invariants: every branch of the decision (Default/Allow/Deny × wrap on/off × fish present/absent), under the hook, split streams, over-cap input (2.7 MB, 400 001 lines, binary), idempotence, line continuations. 3de8beda touches only tests/run_shell_test.rs, and the release binaries at d87d12d8 and 3de8beda are byte-identical (separate target dirs, both built from scratch), so every rtk rewrite transcript below holds at the head under review.

Matrix: executed in full this round.

CI @3de8beda: still never run — three runs sit at action_required (36131883246, 35678701971, 35425641688) and only license/cla is green. That approval is owed from my side and I will give it; separately, the head is red on any runner without fish — blocker 3.

Blocks merge (3 — round-1 blocker 2 cleared; round-1 blocker 1 partly survives as 2; +1 new regression in d87d12d8)

  1. src/discover/fish_script.rs:87 — gate/classify asymmetry, new in d87d12d8 (1 site; the three gates read the raw script while classification reads the stripped code). A quote in a comment blinds them: test -d src; and git status # don't + cat /etc/passwd > /tmp/rtk-leak defers on develop, on the A+B stack and on the round-1 head, wraps here, and writing it out produced 839 bytes of /etc/passwd. Re-opens round-1 blocker 3.
  2. src/discover/fish_script.rs:183 — round-1 blocker 1 survives for operator-adjacent comments (1 site; ;#, |#, &#). echo ${HOME:-x};# note; and more works in bash and dies under fish with nothing run. The space-separated spelling is fixed.
  3. tests/fish_wrap_test.rs:70 — the suite is red on every runner without fish, so CI cannot go green. The assertion's cause is round 1's own Optional, so that part is mine; the one-screen change is in the thread, built and verified.

Transcripts and the fix for each are in the three line comments.

Cleared this round — verified, not re-opened

  • Round-1 blocker 2 (the wrap cost the rewrite) is fixed, and further than asked: multi-line blocks get their inner lines rewritten too (begin\n git status\n ls -la\nend → begin\n rtk git status\n rtk ls -la\nend). All 10 leading commands that lost the rewrite at round 1 keep it; git diff HEAD~3 HEAD; and true is back to 76.9% saved. Executed under all five host layers — the differences against raw fish are exactly the rtk filters doing their job.
  • Round-1 blocker 3's plain case is fixed: all five unattestable shapes now defer, matching develop. Only the quote-blinded path in blocker 1 gets through.
  • Round-1 blocker 1's space-separated case is fixed, and the comment rows now match develop byte for byte where develop rewrote.

Optional — will not hold merge

  • Docs carry a claim this push made false. src/hooks/README.md, README.md:426 and docs/usage/FEATURES.md all still say the script "travels byte-identical inside one quoted argument", while the same README paragraph now also says the inner commands are rewritten first — and collapse_line_continuations folds \<newline> away. Semantics are preserved; bytes are not. The same three files still advertise for/switch blocks without noting that every unquoted (/) and every file-target redirect now defers, so the canonical for f in (ls) … end no longer wraps (test_for_loop_with_fish_substitution_is_fish still asserts it classifies, which is true but no longer means it is wrapped).

Checked and correct — no need to re-verify

  • Precision holds after the comment change: 0 wraps over 19,870 real agent Bash commands. Of the 63 commands whose rewrite differs from develop, all 63 are identical between the #4125+#4126 stack and this head — C changes nothing for any real command in that corpus.
  • Comment fuzz: 1,825 cases across region × separator × marker (comment, double-quoted, single-quoted, apostrophe-bearing comment), 138 wrapped, 0 that work in bash and diverge under fish. Blockers 1 and 2 came from shapes outside that grid.
  • IO fidelity with the nested wrap: 400 001 lines / 2 688 900 B identical, binary output identical, large stderr identical, exit 0/1/7 propagated, stdin forwarded.
  • Idempotence: feeding the emitted form back returns defer, three commands over.
  • Gates: cargo fmt --all --check and cargo clippy --all-targets -- -D warnings clean; cargo check --target x86_64-pc-windows-gnu --all-targets clean; merge onto latest develop (feac25d, 20 commits on from round 1) is clean.
  • Decision branches: Deny → Deny; Allow → AskRewrite (never promoted); wrap_fish_scripts = false → prior path; fish absent from PATH → prior path. Host payload shapes unchanged from develop.
  • The missing newline in rtk git status --short; and echo TAIL is byte-identical on develop — the pre-existing trailing-newline class, not this PR.

Hypotheses built and dropped

  • rewrite_command injecting something the gates would have refused. The gates answer for the pre-rewrite script, so this was the first place I looked. Across 19,869 corpus commands wrapped as test -d src; and <cmd>, 0 emitted inner scripts contained more $(, `, (, ), >, \\ or \' than their input. The asymmetry is real but it is reached through the comment, not the rewrite — blocker 1.
  • contains_fish_substitution over-refusing. Correctly refuses (whoami), x(whoami), --author=(whoami); correctly passes '(hi)', "(hi)", grep -E 'a(b|c)', \(hi\).
  • exclude_commands still bypassed. Partly: an excluded command inside a fish script is still wrapped (test -d src; and curl https://x wraps with exclude_commands = ["curl"]). But the wrap only execs fish — it routes nothing through an rtk filter — so the_nested_rewrite_honours_exclusions is accurately named and nothing in the commit overclaims. Dropped.
  • Other strip_comments edges: # at position 0, inside $'…', after an escaped space, inside a double-quoted string containing an apostrophe, and ># are all handled correctly. Only the operator-adjacent word-start reset is missing.

Your questions

  • "A CI fish step would still be better than either branch — happy to add one if you want it in this PR rather than separately." → in this PR, please. Without fish on the runner the wrap never fires, so CI currently cannot exercise the feature's only code path at all; both new blockers live in code a fish-present runner would reach.
  • "CI has still never been approved on this branch — that is the last thing outstanding that I cannot do myself." → acknowledged, and it is on me. I will approve a run; expect it red on fish_wrap_test until blocker 3 lands.

Next

Three changes, all in files this PR adds: gate on the stripped code (1), treat unquoted operators as word boundaries in strip_comments (2), correct the no-fish assertion (3, suggestion inline). Add the CI fish step with them and this is done — everything else on this head is verified.

Rounds: 2/3. Threads: 3 open (blockers 1–3), 3 resolved this round (round-1 blockers 1 partly, 2 and 3 — see "Cleared this round").

Comment thread src/discover/fish_script.rs Outdated
Comment thread src/discover/fish_script.rs Outdated
Comment thread tests/fish_wrap_test.rs Outdated
pashifika added a commit to pashifika/rtk that referenced this pull request Sep 25, 2026
…e refuses

Round-1 review of rtk-ai#4127, three defects, one mechanism each.

**Comment text was read as code.** Neither the classifier nor the shared lexer
models `#`, so a separator inside a trailing comment reset command position and
the next English word was matched against the fish markers:
`echo $((1+2))  # x; and y` is a working bash command that was wrapped and then
failed under fish with nothing run at all. Every shell in play agrees where a
comment starts and ends, so the span is dropped before anything is classified.
A `#` inside quotes is still an argument, and a fish script may carry a comment
of its own.

**The wrap cost the rewrite.** It preceded `rewrite_command`, so a fish chain
whose leading command was rewritable lost the savings `develop` delivers —
`git diff HEAD~3 HEAD; and true` went from 76.9% to 0%. The script's own
commands are rewritten first and the wrap carries the result, honouring
`exclude_commands` and `transparent_prefixes` like any other rewrite.

**It was the only path emitting a command RTK could not attest.** It now
refuses everything the unattestable gate refuses — command and process
substitution, file-target redirects — plus fish's own `(cmd)` substitution,
which the bash-shaped lexer reads as a subshell and lets past. What still
reaches the wrap is a script whose only unattestable property is fish control
flow, which is the case the feature exists for, and `src/hooks/README.md` now
records why that one is exempt, what bounds it, and that Codex renders `Ask` as
a protocol-level `allow`.

Also from the review: `tests/fish_wrap_test.rs` asserts on the fish-absent
branch instead of returning early, so the file is no longer inert on a runner
without `fish`, and `decide`'s gate-order doc lists the wrap.
@pashifika
pashifika force-pushed the feat/fish-script-wrapping branch from 3de8bed to 64ce837 Compare September 25, 2026 16:58
pashifika added a commit to pashifika/rtk that referenced this pull request Sep 25, 2026
Round-2 review of rtk-ai#4127.

**A quote in a comment blinded the gates** (new in the previous push). The
classification moved to the comment-stripped text but the three gates still
read the raw script, and an apostrophe there opens the shared lexer's quote
state — everything after it collapses into one argument, so no redirect and no
substitution was ever seen while the classifier saw clean fish:

    test -d src; and git status # don't
    cat secrets.env > /tmp/leak

wrapped, and running the emitted form wrote the file out. The gates now read
the same stripped code the classification does, so the second line is seen and
the script defers, as it does on `develop`.

**A comment glued to an operator was still read as code.** `strip_comments`
started a word only after a space, tab or newline, so `;#`, `|#` and `&#` kept
round-1's defect: `echo ${HOME:-x};# note; and more` runs in bash and dies
under fish with nothing executed. Both shells start a word after an unquoted
operator; so does this now.

**The suite was red on any runner without `fish`.** The no-fish branch of
`rewrite_keeps_the_inner_rewrite_inside_the_wrap` asserted a defer, but the
script's leading command is rewritable on its own, so the answer there is the
ordinary rewrite — asserting that exact string pins the wrap as genuinely off
rather than merely absent. `rewrite_defers_fish_script_with_divergent_backslash`
no longer returns early either.

CI installs `fish` on the Linux and macOS test runners, since without it the
wrap never fires and the feature's only code path is never exercised; Windows
has none to exercise, where the wrap is disabled by design. The fixtures no
longer name `/etc/passwd`, which the repo's own semgrep rule flags.

Docs: the script no longer travels byte-identical — its commands are rewritten
first — and `README.md`, `src/hooks/README.md` and the French FEATURES guide
now say so, along with the substitution and file-redirect refusals that make
`for f in (ls) … end` defer.
@pashifika

Copy link
Copy Markdown
Contributor Author

@KuSh — all three blockers fixed on 64ce837, rebased on #4125's round-3 head (309c4f0), and the CI fish step is in as you asked.

Blocker 1 — the gates read the stripped code. You are right about the mechanism and it was mine: moving classification onto strip_comments while leaving the gates on the raw script is exactly the asymmetry an apostrophe exploits. try_wrap_with_probes now computes code once and runs contains_unattestable_construct and contains_fish_substitution on it. Your repro, and the two variants:

test -d src; and git status # don't\ncat secrets.env > /tmp/leak     unchanged (exit 1)
test -d src; and git status # don't\necho SUBST=(whoami)             unchanged (exit 1)
test -d src; and git status # 5" wide\ncat secrets.env > /tmp/leak   unchanged (exit 1)

test_a_comment_quote_cannot_blind_the_gates pins all three, and rewrite_defers_unattestable_fish_scripts gained the comment-bearing spelling end to end. The \\/\' veto deliberately stays on the raw script: it answers for the bytes that get emitted, comment included, so reading the stripped text there would be the looser choice.

Blocker 2 — a word starts after an unquoted operator. ;, | and & join space/tab/newline in the at_word_start reset:

echo ${HOME:-x};# note; and more     unchanged (exit 1)
ls -la|# note; and more              unchanged (exit 1)
sleep 1&# note; and more             unchanged (exit 1)

>/< are deliberately not in that set — >#foo is a redirect to a file named #foo, which your dropped hypothesis already confirmed we handle correctly. The three spellings are in test_comment_text_is_never_a_marker beside the space-separated ones.

Blocker 3 — the no-fish branch asserts the right thing. Took your suggestion verbatim: the leading command is rewritable on its own, so without fish the answer is the ordinary rewrite, and asserting that exact string pins the wrap as genuinely off. rewrite_defers_fish_script_with_divergent_backslash no longer returns early either — it defers on both branches, for different reasons. Verified by running the suite with fish removed from PATH:

PATH=/usr/bin:/bin:/usr/sbin:/sbin  fish_wrap_test   7 passed, 0 failed, 1 ignored
with fish on PATH                   cargo test --all 3,966 passed, 0 failed, 9 ignored

CI: the test job installs fish on Linux (apt-get) and macOS (brew) before cargo test --all. Windows is left alone — the wrap is disabled there by design, so there is nothing to exercise.

Also on this head: semgrep failed on 482d67d's successor for a reason unrelated to the code — sensitive-path-reference matched the literal /etc/passwd in my new fixtures. They now name secrets.env, which exercises the same file-target redirect without tripping the rule.

Docs (your Optional): the "travels byte-identical" claim is gone from README.md, src/hooks/README.md and the French FEATURES guide — the script carries the same commands, not the same bytes, since the rewrite rules run on it first, and the README example now shows the inner rtk git status. All three also record the refusals that make for f in (ls) … end defer.

cargo fmt --all --check                          clean
cargo clippy --all-targets -D warnings           clean
cargo test --all                                 3,966 passed, 0 failed, 9 ignored
cargo check --target x86_64-pc-windows-gnu       clean

(macOS 26.6.2 arm64, rustc 1.97.1, fish 4.9.0.) Ready for a CI run whenever you can approve one — with the fish step it will be the first time the wrap itself runs on a runner.

pashifika added a commit to pashifika/rtk that referenced this pull request Sep 26, 2026
…e refuses

Round-1 review of rtk-ai#4127, three defects, one mechanism each.

**Comment text was read as code.** Neither the classifier nor the shared lexer
models `#`, so a separator inside a trailing comment reset command position and
the next English word was matched against the fish markers:
`echo $((1+2))  # x; and y` is a working bash command that was wrapped and then
failed under fish with nothing run at all. Every shell in play agrees where a
comment starts and ends, so the span is dropped before anything is classified.
A `#` inside quotes is still an argument, and a fish script may carry a comment
of its own.

**The wrap cost the rewrite.** It preceded `rewrite_command`, so a fish chain
whose leading command was rewritable lost the savings `develop` delivers —
`git diff HEAD~3 HEAD; and true` went from 76.9% to 0%. The script's own
commands are rewritten first and the wrap carries the result, honouring
`exclude_commands` and `transparent_prefixes` like any other rewrite.

**It was the only path emitting a command RTK could not attest.** It now
refuses everything the unattestable gate refuses — command and process
substitution, file-target redirects — plus fish's own `(cmd)` substitution,
which the bash-shaped lexer reads as a subshell and lets past. What still
reaches the wrap is a script whose only unattestable property is fish control
flow, which is the case the feature exists for, and `src/hooks/README.md` now
records why that one is exempt, what bounds it, and that Codex renders `Ask` as
a protocol-level `allow`.

Also from the review: `tests/fish_wrap_test.rs` asserts on the fish-absent
branch instead of returning early, so the file is no longer inert on a runner
without `fish`, and `decide`'s gate-order doc lists the wrap.
pashifika added a commit to pashifika/rtk that referenced this pull request Sep 26, 2026
Round-2 review of rtk-ai#4127.

**A quote in a comment blinded the gates** (new in the previous push). The
classification moved to the comment-stripped text but the three gates still
read the raw script, and an apostrophe there opens the shared lexer's quote
state — everything after it collapses into one argument, so no redirect and no
substitution was ever seen while the classifier saw clean fish:

    test -d src; and git status # don't
    cat secrets.env > /tmp/leak

wrapped, and running the emitted form wrote the file out. The gates now read
the same stripped code the classification does, so the second line is seen and
the script defers, as it does on `develop`.

**A comment glued to an operator was still read as code.** `strip_comments`
started a word only after a space, tab or newline, so `;#`, `|#` and `&#` kept
round-1's defect: `echo ${HOME:-x};# note; and more` runs in bash and dies
under fish with nothing executed. Both shells start a word after an unquoted
operator; so does this now.

**The suite was red on any runner without `fish`.** The no-fish branch of
`rewrite_keeps_the_inner_rewrite_inside_the_wrap` asserted a defer, but the
script's leading command is rewritable on its own, so the answer there is the
ordinary rewrite — asserting that exact string pins the wrap as genuinely off
rather than merely absent. `rewrite_defers_fish_script_with_divergent_backslash`
no longer returns early either.

CI installs `fish` on the Linux and macOS test runners, since without it the
wrap never fires and the feature's only code path is never exercised; Windows
has none to exercise, where the wrap is disabled by design. The fixtures no
longer name `/etc/passwd`, which the repo's own semgrep rule flags.

Docs: the script no longer travels byte-identical — its commands are rewritten
first — and `README.md`, `src/hooks/README.md` and the French FEATURES guide
now say so, along with the substitution and file-redirect refusals that make
`for f in (ls) … end` defer.
@pashifika
pashifika force-pushed the feat/fish-script-wrapping branch from 64ce837 to 813d8fd Compare September 26, 2026 05:27
@pashifika

Copy link
Copy Markdown
Contributor Author

Rebased onto develop @ c75f159 — 813d8fd — after #4125's own rebase, since this one is stacked on it. It also picks up #4126's 2dbb9d1 (&| deferral).

The only structural conflict was in #4125's half: src/hooks/init.rs was split into src/hooks/init/ on develop, so the awareness-body edits moved to src/hooks/init/mod.rs. Nothing in this PR's own files conflicted.

cargo fmt --all --check                  clean
cargo clippy --all-targets -D warnings   clean
cargo test --all                         4,101 passed, 0 failed, 9 ignored
fish_wrap_test with fish off PATH        7 passed, 0 failed, 1 ignored

Round-2 blockers re-verified on the rebased binary: git diff HEAD~3 HEAD; and true → rtk run --shell fish -c 'rtk git diff HEAD~3 HEAD; and true'; echo \${HOME:-x};# note; and more and the apostrophe-comment + redirect script both defer.

pashifika added a commit to pashifika/rtk that referenced this pull request Sep 27, 2026
…e refuses

Round-1 review of rtk-ai#4127, three defects, one mechanism each.

**Comment text was read as code.** Neither the classifier nor the shared lexer
models `#`, so a separator inside a trailing comment reset command position and
the next English word was matched against the fish markers:
`echo $((1+2))  # x; and y` is a working bash command that was wrapped and then
failed under fish with nothing run at all. Every shell in play agrees where a
comment starts and ends, so the span is dropped before anything is classified.
A `#` inside quotes is still an argument, and a fish script may carry a comment
of its own.

**The wrap cost the rewrite.** It preceded `rewrite_command`, so a fish chain
whose leading command was rewritable lost the savings `develop` delivers —
`git diff HEAD~3 HEAD; and true` went from 76.9% to 0%. The script's own
commands are rewritten first and the wrap carries the result, honouring
`exclude_commands` and `transparent_prefixes` like any other rewrite.

**It was the only path emitting a command RTK could not attest.** It now
refuses everything the unattestable gate refuses — command and process
substitution, file-target redirects — plus fish's own `(cmd)` substitution,
which the bash-shaped lexer reads as a subshell and lets past. What still
reaches the wrap is a script whose only unattestable property is fish control
flow, which is the case the feature exists for, and `src/hooks/README.md` now
records why that one is exempt, what bounds it, and that Codex renders `Ask` as
a protocol-level `allow`.

Also from the review: `tests/fish_wrap_test.rs` asserts on the fish-absent
branch instead of returning early, so the file is no longer inert on a runner
without `fish`, and `decide`'s gate-order doc lists the wrap.
pashifika added a commit to pashifika/rtk that referenced this pull request Sep 27, 2026
Round-2 review of rtk-ai#4127.

**A quote in a comment blinded the gates** (new in the previous push). The
classification moved to the comment-stripped text but the three gates still
read the raw script, and an apostrophe there opens the shared lexer's quote
state — everything after it collapses into one argument, so no redirect and no
substitution was ever seen while the classifier saw clean fish:

    test -d src; and git status # don't
    cat secrets.env > /tmp/leak

wrapped, and running the emitted form wrote the file out. The gates now read
the same stripped code the classification does, so the second line is seen and
the script defers, as it does on `develop`.

**A comment glued to an operator was still read as code.** `strip_comments`
started a word only after a space, tab or newline, so `;#`, `|#` and `&#` kept
round-1's defect: `echo ${HOME:-x};# note; and more` runs in bash and dies
under fish with nothing executed. Both shells start a word after an unquoted
operator; so does this now.

**The suite was red on any runner without `fish`.** The no-fish branch of
`rewrite_keeps_the_inner_rewrite_inside_the_wrap` asserted a defer, but the
script's leading command is rewritable on its own, so the answer there is the
ordinary rewrite — asserting that exact string pins the wrap as genuinely off
rather than merely absent. `rewrite_defers_fish_script_with_divergent_backslash`
no longer returns early either.

CI installs `fish` on the Linux and macOS test runners, since without it the
wrap never fires and the feature's only code path is never exercised; Windows
has none to exercise, where the wrap is disabled by design. The fixtures no
longer name `/etc/passwd`, which the repo's own semgrep rule flags.

Docs: the script no longer travels byte-identical — its commands are rewritten
first — and `README.md`, `src/hooks/README.md` and the French FEATURES guide
now say so, along with the substitution and file-redirect refusals that make
`for f in (ls) … end` defer.
@pashifika
pashifika force-pushed the feat/fish-script-wrapping branch from 813d8fd to c3de622 Compare September 27, 2026 01:23
@pashifika

Copy link
Copy Markdown
Contributor Author

Rebased onto develop @ 45b0b4c now that #4125 has landed — c3de622. The conflict was this branch still carrying #4125's six commits; they are gone from the diff, which is now #4126's three (sh|bash|zsh -c wrapper, the fish dialect, 2dbb9d1's &| deferral) plus this PR's own four.

cargo fmt --all --check                    clean
cargo clippy --all-targets -D warnings     clean
cargo test --all                           4,124 passed, 0 failed, 9 ignored
cargo check --target x86_64-pc-windows-gnu clean
fish_wrap_test with fish off PATH          7 passed, 0 failed, 1 ignored

Round-2 blockers re-verified on the rebased binary:

git diff HEAD~3 HEAD; and true           → rtk run --shell fish -c 'rtk git diff HEAD~3 HEAD; and true'  (exit 3)
test -d src; and git status              → rtk run --shell fish -c 'test -d src; and git status'         (exit 3)
echo \${HOME:-x};# note; and more         unchanged                                                      (exit 1)
… # don't\ncat secrets.env > /tmp/leak    unchanged                                                      (exit 1)
fish -c 'git status; cargo test'         → fish -c 'rtk git status; rtk cargo test'                      (exit 3)
(cd sub && git status)                   → (cd sub && rtk git status)                                    (exit 3)
git status; if true; then echo x; fi     → rtk git status; if true; then echo x; fi                      (exit 3)

CI needs approving on the new head — with the fish step it will be the first run that exercises the wrap itself.

@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 3 of 3 — CHANGES REQUESTED

Claim: a command string that is provably fish is handed back as rtk run --shell fish -c '<script>' ahead of the rewrite rules, Ask-only, opt-out via hooks.wrap_fish_scripts.

Scope: accept (round 1, frozen). Blocker 2 below offers a narrowing as one of its two ways out, but the claim as written is still the right one.

Ran: rtk rewrite and rtk hook <host> on four binaries — develop 45b0b4c, #4126's head ff75313, this head c3de622, and a mutant with & removed from the comment scanner — over all six prior blockers' repros, the round-1 matrix, 19,870 real agent Bash commands, 6,552 comment/boundary fuzz cases, and a 34-character glue sweep using execution as the discriminator; tests/fish_wrap_test.rs executed against real fish 4.9.1 and fish 3.7.0 (CI's version), with and without a user config.fish. Invariants: every branch of the decision (Default/Allow/Deny × wrap on/off × fish present/absent), under the hook, a live deny rule, and the wrap's own execution end to end.

Matrix: executed in full this round.

CI @c3de6229: all green, 11/11, and for the first time it exercises the feature — the new Install fish step put fish 3.7.0 on the Linux runner and brew's fish on macOS, correctly skipped on Windows. Both blockers below are invisible to it, which is the point of the matrix.

Blocks merge (2 — all six prior blockers cleared; +2 in c3de6229)

  1. src/discover/fish_script.rs:192 — & is the one member of the new reset set where bash and fish disagree (1 site; & alone of 34 characters tried). test -d src; and echo HIT&#y > /tmp/LEAK1 defers on develop and on #4126's head, wraps here, and executing the emitted form writes 7 bytes through a redirect the gate never saw; the if … end spelling does the same. Removing & is not the fix — it reinstates round 1's defect. Rows a fix must pass are in the thread.
  2. src/hooks/decision.rs:199 — the wrap executes commands the permission gate structurally cannot see. Under deny: ["Bash(rm:*)"], test -d src; and rm -rf victim and not rm -rf victim are wrapped and run under fish, where the file is deleted; bash leaves it alone and develop defers. The block forms correctly Deny. src/hooks/README.md:114 asserts the opposite. I under-called this at round 2 — I checked the bare and block spellings, not the chain spelling with a rule live.

Optional — will not hold merge; threads resolved by me

  • README.md:429 and the "costs no savings" claim. c3de6229 changed the example to rtk git status inside a multi-line if … end, but that shape gets no inner rewrite: every multi-line if/while/for/function … end block wraps unprefixed, and in a ; and chain only a rewritable leading command is rewritten. begin … end blocks and single-line if true; …; end do get it. So the example as printed is not what the code emits, and src/hooks/README.md:90, decision.rs:128 and the French guide carry the same claim. The codebase's own fish_script_is_wrapped_as_ask pins the unprefixed form, so the docs contradict the tests.

  • CI now has no runner for the no-fish branches. With fish installed on both unix legs and the file #[cfg(unix)], the disabled-path assertions that c3de6229 just corrected are exercised nowhere, so that regression could return silently. One leg without the install step, or a PATH-stripped run of that one test, would keep both sides covered.

  • tests/fish_wrap_test.rs inherits the developer's HOME, and it costs a real failure. fish -c sources ~/.config/fish/config.fish, and anything it prints lands on the stdout wrapped_quoting_round_trips_under_posix_and_fish_hosts compares byte for byte. With a one-line config.fish that echoes, that test fails on fish 4.9.1 and on 3.7.0 — i.e. for a fish user contributing to rtk, which is this PR's own audience. The sibling tests avoid it by going through rewrite_isolated; these two do not. Separately, wrapped_multiline_fish_script_executes_and_propagates_exit_code is still #[ignore]d, so the one case that actually runs a wrapped script through rtk run --shell fish is skipped by cargo test --all — the new fish install step is paying for coverage it does not get. I confirmed the enabled test passes on both fish versions.

Filed as follow-ups

None filed this round; both items above are inside this PR's own hunks, so they stay as threads rather than issues.

Checked and correct — no need to re-verify

  • All six prior blockers are fixed, re-verified by execution three-way. Comment text is no longer code, including the ;#/|#/&&#/||# spellings; the wrap keeps the rewrite; the apostrophe-comment scripts that reached the wrap at round 2 now defer; the no-fish assertion is right and rewrite_defers_fish_script_with_divergent_backslash no longer returns early.
  • Precision. 0 wraps over 19,870 real agent Bash commands; 0 rewrite differences between #4126's head and this head across that corpus — the feature is inert for every real command in it.
  • Comment/boundary fuzz. 6,552 cases, 420 wrapped, 0 that work in bash and diverge under fish. Blocker 1 needed execution rather than a bash/fish output diff to surface, which is why it took the 34-character sweep.
  • The \\/\' veto reading the raw script while the gates read the stripped code is the safe direction — strip_comments only ever removes characters, so the raw read is strictly stricter, and continuation collapse cannot manufacture either sequence.
  • Gates. cargo fmt --all --check, cargo clippy --all-targets -- -D warnings, cargo check --target x86_64-pc-windows-gnu --all-targets clean; merge onto develop (now c529013e) clean, cargo test --all on the merged tree 4 124 passed, 0 failed.
  • The wrap executed, not just emitted. All 8 fish_wrap_test cases pass against fish 4.9.1 and 3.7.0.
  • Docs. The "travels byte-identical" claim is gone from the user-facing docs; the two remaining internal mentions are about the single-quote escape, which is still true. The README now names for f in (ls) … end as deferring.

Your questions

  • "The \\/\' veto deliberately stays on the raw script … reading the stripped text there would be the looser choice." → agreed, keep it, and I verified the reasoning rather than taking it: strip_comments is subtractive, so raw ⊇ stripped and the raw read can only ever refuse more.
  • ">/< are deliberately not in that set — >#foo is a redirect to a file named #foo." → agreed on the outcome, though it is moot either way: a file-target redirect makes the script unattestable, so echo x >#foo; and more and cat <#foo; and more defer regardless. The character that needed this scrutiny was &, where the two shells genuinely disagree — blocker 1.
  • "Windows is left alone — the wrap is disabled there by design." → agreed, and CI confirms both install steps skip there. See the second Optional for the side effect on the no-fish branches.
  • "CI needs approving on the new head." → done, and green.

Next

Two changes, both in src/discover/fish_script.rs and src/hooks/decision.rs: defer when bash and fish disagree about an unquoted # preceded by an unquoted & (1), and either give the verdict a fish-aware split or drop and/or/not chains from the wrap (2). Worth keeping this push to exactly those two: each of the last two revisions fixed what it was asked to and moved a new defect into the same scanner, so the narrower the change, the better the odds it is the last one. If the fish-aware split turns out to be more than contained, take the narrowing on 2 and I will file the chains as a follow-up.

Rounds: 3/3. Threads: 2 open (blockers 1–2), 6 resolved this round.

Comment thread src/discover/fish_script.rs Outdated
Comment thread src/hooks/decision.rs Outdated
pashifika added a commit to pashifika/rtk that referenced this pull request Sep 28, 2026
…n keyword

Round-3 review of rtk-ai#4127, both defects introduced by round 2's own fixes.

**`&` was the one boundary character bash and fish read differently.** Since
fish 3.0 `&` is job control only at the end of a job, so a `#` glued to it
opens a comment in bash and continues the word in fish:

    bash -c 'echo A&#y'   ->  A        (comment)
    fish -c 'echo A&#y'   ->  A&#y     (one literal word)

`strip_comments` took bash's reading, which hid everything after the `#` from
the gates while the emitted wrap carried it and fish ran it —
`test -d src; and echo HIT&#y > /tmp/LEAK` wrapped and wrote the file. Taking
fish's reading instead re-admits comment text as code, which is the defect
round 1 fixed. Neither is safe, so the scanner refuses: `strip_comments`
returns `None` when a comment start is glued to a lone unquoted `&`, and the
script keeps the host's own behaviour. `&&#` and `||#` still strip — both
shells agree there — and the run is counted only over syntactic ampersands, so
an escaped `\&` before a lone `&` refuses too, and a run never crosses the
newline that closes a comment.

**A fish chain keyword hid a denied command from the permission gate.**
`split_for_permissions` models bash, where `test -d src; and rm -rf victim` is
a command called `and`, so a `Bash(rm:*)` rule never matched it. Bash would
not run that command at all; fish does, and the wrap hands the script to a
fish. The deny loop now also matches the segment with leading chain keywords
(`and`, `or`, `not`, `command`, `builtin`) peeled, folding the line
continuations both shells fold. Deny only: peeling for an allow rule would
approve a command the rule never named.

Reviewed independently before pushing; that pass found the escaped-`&` and
line-continuation bypasses above, which are fixed and pinned here.
@pashifika

Copy link
Copy Markdown
Contributor Author

@KuSh — both blockers fixed on 5478386. I put this push through an independent review before sending it, precisely because of your note that each of the last two revisions moved a new defect into the same scanner; it found two bypasses in my first attempt, and those are fixed and pinned here.

Blocker 1 — the scanner refuses instead of picking a reading. You are right that neither reading is safe, so strip_comments now returns Option<String> and answers None when a comment start is glued to a lone unquoted &. The caller treats that as "not classifiable": is_unambiguous_fish is false and try_wrap_with_probes returns None, so the script keeps the host's behaviour. Your rows, on this head:

test -d src; and echo HIT&#y > /tmp/LEAK        unchanged (exit 1)
test -d src; and echo HIT&#y (whoami)           unchanged (exit 1)
if test -d src\n  echo A&#y > /tmp/LEAK\nend    unchanged (exit 1)
sleep 1&# note; and more                        unchanged (exit 1)
echo ${HOME:-x};# note; and more                unchanged (exit 1)
ls -la|# note; and more                         unchanged (exit 1)
ls -la  # long listing; and hidden files      → rtk ls -la  # …            (exit 3, = develop)
cargo test 2>&1 | tail -20  # errors only; …  → rtk cargo test 2>&1 | …    (exit 3, = develop)

Two refinements the review pass forced, both from shapes I had not considered:

  • the run is counted over syntactic ampersands only, so an escaped \& does not pair with the & after it — true; and echo \&&#y > /tmp/LEAK was wrapping under a two-character lookback and now defers;
  • the run resets when a comment closes, so true; and echo ok &&# comment\n&#y > /tmp/LEAK cannot borrow the previous line's &&.

test_operator_pairs_still_end_a_word is now load-bearing in the other direction too: it asserts &&#, ||# and a spaced & # still classify, so a refusal that widens to them fails the test — which is the failure mode you flagged on the old version of that test.

Blocker 2 — the fix, not the narrowing. The verdict now sees through the chain keyword: the deny loop matches each segment and the segment with leading and/or/not/command/builtin peeled. Deny only — peeling for an allow rule would approve a command the rule never named. With deny: ["Bash(rm:*)"]:

rm -rf victim                          exit 2
if test -d src\n  rm -rf victim\nend   exit 2
test -d src; and rm -rf victim         exit 2   ← was 3 + wrap
not rm -rf victim                      exit 2   ← was 3 + wrap
test -d src; and \<newline> rm -rf victim   exit 2   ← the review pass found this one
test -d src; and git status            exit 3, wrapped (unchanged)

The continuation spelling is why the peel goes through a skip_blanks that folds \<newline> and \<CRLF>: without it the peeled segment starts with a backslash and no rule matches. deny_outranks_the_wrap_in_every_spelling in tests/fish_wrap_test.rs runs all four spellings through rtk rewrite under a real settings.json, so src/hooks/README.md:114's claim is now pinned by execution rather than asserted.

I kept the push to exactly these two, as you asked — no docs or test-harness changes rode along.

cargo fmt --all --check                    clean
cargo clippy --all-targets -D warnings     clean
cargo test --all                           4,130 passed, 0 failed, 9 ignored
cargo check --target x86_64-pc-windows-gnu clean
fish_wrap_test with fish on PATH           8 passed, 1 ignored

(macOS 26.6.2 arm64, rustc 1.97.1, fish 4.9.0.)

On the three Optionals, so the threads are not left silent: the multi-line if … end example in README.md:429 versus what the code emits, the CI legs both having fish now, and wrapped_quoting_round_trips_under_posix_and_fish_hosts inheriting the developer's HOME — all three are real and none is in this push. Say which you want in this PR and which you would rather have as follow-ups, and I will do exactly that next.

pashifika added a commit to pashifika/rtk that referenced this pull request Sep 28, 2026
…e refuses

Round-1 review of rtk-ai#4127, three defects, one mechanism each.

**Comment text was read as code.** Neither the classifier nor the shared lexer
models `#`, so a separator inside a trailing comment reset command position and
the next English word was matched against the fish markers:
`echo $((1+2))  # x; and y` is a working bash command that was wrapped and then
failed under fish with nothing run at all. Every shell in play agrees where a
comment starts and ends, so the span is dropped before anything is classified.
A `#` inside quotes is still an argument, and a fish script may carry a comment
of its own.

**The wrap cost the rewrite.** It preceded `rewrite_command`, so a fish chain
whose leading command was rewritable lost the savings `develop` delivers —
`git diff HEAD~3 HEAD; and true` went from 76.9% to 0%. The script's own
commands are rewritten first and the wrap carries the result, honouring
`exclude_commands` and `transparent_prefixes` like any other rewrite.

**It was the only path emitting a command RTK could not attest.** It now
refuses everything the unattestable gate refuses — command and process
substitution, file-target redirects — plus fish's own `(cmd)` substitution,
which the bash-shaped lexer reads as a subshell and lets past. What still
reaches the wrap is a script whose only unattestable property is fish control
flow, which is the case the feature exists for, and `src/hooks/README.md` now
records why that one is exempt, what bounds it, and that Codex renders `Ask` as
a protocol-level `allow`.

Also from the review: `tests/fish_wrap_test.rs` asserts on the fish-absent
branch instead of returning early, so the file is no longer inert on a runner
without `fish`, and `decide`'s gate-order doc lists the wrap.
pashifika added a commit to pashifika/rtk that referenced this pull request Sep 28, 2026
Round-2 review of rtk-ai#4127.

**A quote in a comment blinded the gates** (new in the previous push). The
classification moved to the comment-stripped text but the three gates still
read the raw script, and an apostrophe there opens the shared lexer's quote
state — everything after it collapses into one argument, so no redirect and no
substitution was ever seen while the classifier saw clean fish:

    test -d src; and git status # don't
    cat secrets.env > /tmp/leak

wrapped, and running the emitted form wrote the file out. The gates now read
the same stripped code the classification does, so the second line is seen and
the script defers, as it does on `develop`.

**A comment glued to an operator was still read as code.** `strip_comments`
started a word only after a space, tab or newline, so `;#`, `|#` and `&#` kept
round-1's defect: `echo ${HOME:-x};# note; and more` runs in bash and dies
under fish with nothing executed. Both shells start a word after an unquoted
operator; so does this now.

**The suite was red on any runner without `fish`.** The no-fish branch of
`rewrite_keeps_the_inner_rewrite_inside_the_wrap` asserted a defer, but the
script's leading command is rewritable on its own, so the answer there is the
ordinary rewrite — asserting that exact string pins the wrap as genuinely off
rather than merely absent. `rewrite_defers_fish_script_with_divergent_backslash`
no longer returns early either.

CI installs `fish` on the Linux and macOS test runners, since without it the
wrap never fires and the feature's only code path is never exercised; Windows
has none to exercise, where the wrap is disabled by design. The fixtures no
longer name `/etc/passwd`, which the repo's own semgrep rule flags.

Docs: the script no longer travels byte-identical — its commands are rewritten
first — and `README.md`, `src/hooks/README.md` and the French FEATURES guide
now say so, along with the substitution and file-redirect refusals that make
`for f in (ls) … end` defer.
pashifika added a commit to pashifika/rtk that referenced this pull request Sep 28, 2026
…n keyword

Round-3 review of rtk-ai#4127, both defects introduced by round 2's own fixes.

**`&` was the one boundary character bash and fish read differently.** Since
fish 3.0 `&` is job control only at the end of a job, so a `#` glued to it
opens a comment in bash and continues the word in fish:

    bash -c 'echo A&#y'   ->  A        (comment)
    fish -c 'echo A&#y'   ->  A&#y     (one literal word)

`strip_comments` took bash's reading, which hid everything after the `#` from
the gates while the emitted wrap carried it and fish ran it —
`test -d src; and echo HIT&#y > /tmp/LEAK` wrapped and wrote the file. Taking
fish's reading instead re-admits comment text as code, which is the defect
round 1 fixed. Neither is safe, so the scanner refuses: `strip_comments`
returns `None` when a comment start is glued to a lone unquoted `&`, and the
script keeps the host's own behaviour. `&&#` and `||#` still strip — both
shells agree there — and the run is counted only over syntactic ampersands, so
an escaped `\&` before a lone `&` refuses too, and a run never crosses the
newline that closes a comment.

**A fish chain keyword hid a denied command from the permission gate.**
`split_for_permissions` models bash, where `test -d src; and rm -rf victim` is
a command called `and`, so a `Bash(rm:*)` rule never matched it. Bash would
not run that command at all; fish does, and the wrap hands the script to a
fish. The deny loop now also matches the segment with leading chain keywords
(`and`, `or`, `not`, `command`, `builtin`) peeled, folding the line
continuations both shells fold. Deny only: peeling for an allow rule would
approve a command the rule never named.

Reviewed independently before pushing; that pass found the escaped-`&` and
line-continuation bypasses above, which are fixed and pinned here.
@pashifika

Copy link
Copy Markdown
Contributor Author

@KuSh — narrowed and rebased: dae6a11 on develop @ 7bff768. No apology needed; the round-6 list was a reasonable reading of the loaders, and the round-7 one is the correct one.

Item 1 — only rtk hook claude wraps

DenyRules is now NoneConfigured | Unattested, and only claude_hook_deny_rules() returns the first. AgentPath::ClaudeHook separates rtk hook claude from Copilot, since Host cannot — exactly as you said.

Your rows, measured on this head (release build, isolated HOMEs):

row result
rtk hook claude, no deny rule, every file parsed block, chain, begin, for, switch shapes wrap; git diff HEAD~3 HEAD; and true → rtk run --shell fish -c 'rtk git diff HEAD~3 HEAD; and true'
rtk hook claude, a deny rule in any file RTK reads no wrap
rtk hook claude, a settings file it cannot parse no wrap
rtk rewrite, no host and each delegate name never a wrap
copilot|cursor|gemini|droid|codex|trae|vibe|antigravity|opencode, with and without rule files never a wrap
rtk hook check --agent <each>, rules-only included agrees with that agent's hook

Pins. Every mutation you named turns a test red, and I checked each one in a scratch worktree rather than reasoning about it:

mutation test that fails
Gemini / Droid given Claude's attestation (diagnostic) only_the_claude_hook_wraps
Gemini / Droid / Cursor given it in the real hook path only_the_claude_hook_wraps
AgentPath::ViaRewrite given it the_delegate_diagnostic_never_shows_a_wrap
rtk rewrite given it rtk_rewrite_never_wraps
attestation ignoring the parse status a_settings_file_rtk_could_not_read_keeps_the_wrap_off

only_the_claude_hook_wraps now drives each host's own payload hook as well as hook check — an earlier draft pinned only the diagnostic, and a mutation of the real Cursor/Droid/Gemini path survived the whole suite.

Two gaps in my own attestation, found before pushing:

  • A deny entry this gate does not parse was read as "no deny rule". append_bash_rules keeps only the Bash(…) forms, so {"permissions":{"deny":["Bash"]}} — which denies every Bash call, and is a documented rule — looked unconfigured and wrapped. "No deny rule" now means no entry under permissions.deny at all, whatever its tool.
  • A file RTK could not read did not clear attestation. Only the JSON parse error did. settings.json whose bytes are not UTF-8, or which RTK may not open, went through silently. Both now clear it, like an unparsable file.

Both have their own row with a parsed-and-empty control.

Item 2 — the rebase

Five conflicts resolved as you described; the three call sites were exactly the three you named. The three silent-merge resolutions:

  • rtk rewrite's verdict is permissions::check_command(cmd) again. With transparent_prefixes configured, a prefixed shell wrapper whose script hits a deny rule gives the same exit code as develop (both exit 1, empty output), and the control still rewrites on both.
  • The pipeline Swap gate passes Unattested. With it, decide_for_agent has no wrap branch to take, so the call reduces to develop's two-argument form. I could not make a swap rule fire locally to see it end to end — the rule never matched on either binary — so that one rests on construction, same as your own evidence.
  • docs/usage/FEATURES.md carries the fish section in English. The README paragraph is in docs/guide/getting-started/quick-start.md, right after "nested wrappers pass through unchanged". develop's PermissionVerdict::Defer check and its gate-1 doc sentence are kept.

rtk rewrite output is byte-identical to develop across 51 command × profile combinations, and 10 hosts × 18 non-fish inputs × 2 profiles match on exit code, stdout and stderr.

Optional

  • TECHNICAL.md's word-scan sentence and the src/hooks/README.md narration are gone; both describe the code now, with no reference to develop.
  • The settings are still read twice per hook call. Left alone, as you measured no cost and it keeps the verdict path exactly as develop has it.
  • ApprovalOwner::apply is now simpler than when you said "Drop". The prefix sniff was dead weight once the wrap is confined to rtk hook claude: no wrap can reach a delegate, so it only ever fired on a command the user typed in the wrap form — RTK_REWRITE_HOST=openclaw rtk rewrite "rtk run --shell fish -c 'echo ok'" exited 3 where develop exits 0. Removing it restores that and deletes fish_script::is_wrapped. Tell me if you would rather I had left it.

The two test items you own — environment isolation in wrapped_quoting_round_trips_under_posix_and_fish_hosts, and the #[ignore]d execution test — are untouched.

Gates

cargo fmt --all --check                    clean
cargo clippy --all-targets -D warnings     clean
cargo test --all                           5,019 passed, 0 failed, 9 ignored
cargo check --target x86_64-pc-windows-gnu clean
fish_wrap_test with fish off PATH          passes

(macOS 26.6.2 arm64, rustc 1.97.1, fish 4.9.3.)

pashifika added a commit to pashifika/rtk that referenced this pull request Oct 17, 2026
…e refuses

Round-1 review of rtk-ai#4127, three defects, one mechanism each.

**Comment text was read as code.** Neither the classifier nor the shared lexer
models `#`, so a separator inside a trailing comment reset command position and
the next English word was matched against the fish markers:
`echo $((1+2))  # x; and y` is a working bash command that was wrapped and then
failed under fish with nothing run at all. Every shell in play agrees where a
comment starts and ends, so the span is dropped before anything is classified.
A `#` inside quotes is still an argument, and a fish script may carry a comment
of its own.

**The wrap cost the rewrite.** It preceded `rewrite_command`, so a fish chain
whose leading command was rewritable lost the savings `develop` delivers —
`git diff HEAD~3 HEAD; and true` went from 76.9% to 0%. The script's own
commands are rewritten first and the wrap carries the result, honouring
`exclude_commands` and `transparent_prefixes` like any other rewrite.

**It was the only path emitting a command RTK could not attest.** It now
refuses everything the unattestable gate refuses — command and process
substitution, file-target redirects — plus fish's own `(cmd)` substitution,
which the bash-shaped lexer reads as a subshell and lets past. What still
reaches the wrap is a script whose only unattestable property is fish control
flow, which is the case the feature exists for, and `src/hooks/README.md` now
records why that one is exempt, what bounds it, and that Codex renders `Ask` as
a protocol-level `allow`.

Also from the review: `tests/fish_wrap_test.rs` asserts on the fish-absent
branch instead of returning early, so the file is no longer inert on a runner
without `fish`, and `decide`'s gate-order doc lists the wrap.
pashifika added a commit to pashifika/rtk that referenced this pull request Oct 17, 2026
Round-2 review of rtk-ai#4127.

**A quote in a comment blinded the gates** (new in the previous push). The
classification moved to the comment-stripped text but the three gates still
read the raw script, and an apostrophe there opens the shared lexer's quote
state — everything after it collapses into one argument, so no redirect and no
substitution was ever seen while the classifier saw clean fish:

    test -d src; and git status # don't
    cat secrets.env > /tmp/leak

wrapped, and running the emitted form wrote the file out. The gates now read
the same stripped code the classification does, so the second line is seen and
the script defers, as it does on `develop`.

**A comment glued to an operator was still read as code.** `strip_comments`
started a word only after a space, tab or newline, so `;#`, `|#` and `&#` kept
round-1's defect: `echo ${HOME:-x};# note; and more` runs in bash and dies
under fish with nothing executed. Both shells start a word after an unquoted
operator; so does this now.

**The suite was red on any runner without `fish`.** The no-fish branch of
`rewrite_keeps_the_inner_rewrite_inside_the_wrap` asserted a defer, but the
script's leading command is rewritable on its own, so the answer there is the
ordinary rewrite — asserting that exact string pins the wrap as genuinely off
rather than merely absent. `rewrite_defers_fish_script_with_divergent_backslash`
no longer returns early either.

CI installs `fish` on the Linux and macOS test runners, since without it the
wrap never fires and the feature's only code path is never exercised; Windows
has none to exercise, where the wrap is disabled by design. The fixtures no
longer name `/etc/passwd`, which the repo's own semgrep rule flags.

Docs: the script no longer travels byte-identical — its commands are rewritten
first — and `README.md`, `src/hooks/README.md` and the French FEATURES guide
now say so, along with the substitution and file-redirect refusals that make
`for f in (ls) … end` defer.
pashifika added a commit to pashifika/rtk that referenced this pull request Oct 17, 2026
Round-3 review of rtk-ai#4127.

`&` is the one boundary character bash and fish read differently. Since fish
3.0 `&` is job control only at the end of a job, so a `#` glued to it opens a
comment in bash and continues the word in fish:

    bash -c 'echo A&#y'   ->  A        (comment)
    fish -c 'echo A&#y'   ->  A&#y     (one literal word)

`strip_comments` took bash's reading, which hid everything after the `#` from
the gates while the emitted wrap carried it and fish ran it —
`test -d src; and echo HIT&#y > /tmp/LEAK` wrapped and wrote the file. Taking
fish's reading instead re-admits comment text as code, which is the defect
round 1 fixed. Neither is safe, so the scanner refuses: `strip_comments`
returns `None` when a comment start is glued to a lone unquoted `&`, and the
script keeps the host's own behaviour. `&&#` and `||#` still strip — both
shells agree there — and the run is counted only over syntactic ampersands, so
an escaped `\&` before a lone `&` refuses too, and a run never crosses the
newline that closes a comment.
pashifika added a commit to pashifika/rtk that referenced this pull request Oct 17, 2026
Round-4 review of rtk-ai#4127. The round-3 answer peeled `and`/`or`/`not` off the
front of a segment, which was both too wide — it changed how every command is
matched against deny rules, not only fish ones — and too narrow: it only ever
looked at the start of a segment. A denied command in the condition of an `if`
or a `while` went straight through, and the wrap handed it to a fish that runs
it where bash refuses the syntax outright:

    deny: ["Bash(rm:*)"]
    if not rm -rf victim\n  echo x\nend      wrapped, and fish deleted the file
    while rm -rf victim\n  break\nend        the same

Peeling keywords chases one placement at a time, and fish starts commands in
more places than a keyword list holds. So the criterion is no longer "peel the
keyword" but **never wrap a script a deny rule could match anywhere**: every
word is treated as a possible command start and the permission gate's own
`command_matches_pattern` answers for each run of words
(`permissions::deny_matches_any_word_run`). The words come from the same
comment-stripped code the classification and the gates read, dequoted through
the shared `lexer::shell_split`, so an apostrophe in a comment cannot hide
them and `'rm'`, `r'm'` and `"rm"` all read as `rm`.

The rules are the host's own, the ones its verdict was judged against:
`decide` takes them, `decide_hook_action` loads them once per host instead of
twice, and `AgentPath` supplies the set its own `verdict` reads.
`check_command` goes with that — `rewrite_cmd` was its only caller and now
loads Claude's rules explicitly, which is what it documented already.

The keyword peel leaves this PR entirely; with the wrap deciding for itself,
nothing here needs it.

Conservative by construction: a denied program named as a plain argument keeps
the wrap off too. That costs a rewrite, never a guarantee. With no deny rules
nothing changes, and a rule naming something else leaves the wrap alone.
`deny_keeps_the_wrap_off_in_every_placement` pins all twelve spellings through
`rtk rewrite` under a real `settings.json`, asserting *never wrapped* rather
than a particular exit code.
@pashifika
pashifika force-pushed the feat/fish-script-wrapping branch from dae6a11 to a8d6c31 Compare October 17, 2026 13:36
@pashifika

Copy link
Copy Markdown
Contributor Author

Follow-up: develop moved three commits while I was writing the above (#3879, the Claude Code worktree git exclusion), and they conflict with this branch in decision.rs and hook_cmd.rs. Rebased again — the head is now a8d6c31 on develop @ e542dca, and BLOCKED rather than DIRTY.

The resolution threads both parameters: decide_for_agent_at(cmd, verdict, deny_rules, cwd) and decide_from_verdict(cmd, verdict, deny_rules, cwd), so the worktree exclusion and the wrap gate are independent. decide_claude_hook_action takes the payload's cwd like the path it replaces.

Re-ran everything on the new head: the whole item-1 table, cargo fmt --check, clippy -D warnings, cargo test --all (5,023 passed, 0 failed, 9 ignored) and the Windows cross-check. Added row: inside a .claude/worktrees/… cwd, git status && cargo build still comes back as git status && rtk cargo build — #3864's guard is intact alongside the wrap.

pashifika added a commit to pashifika/rtk that referenced this pull request Oct 17, 2026
…e refuses

Round-1 review of rtk-ai#4127, three defects, one mechanism each.

**Comment text was read as code.** Neither the classifier nor the shared lexer
models `#`, so a separator inside a trailing comment reset command position and
the next English word was matched against the fish markers:
`echo $((1+2))  # x; and y` is a working bash command that was wrapped and then
failed under fish with nothing run at all. Every shell in play agrees where a
comment starts and ends, so the span is dropped before anything is classified.
A `#` inside quotes is still an argument, and a fish script may carry a comment
of its own.

**The wrap cost the rewrite.** It preceded `rewrite_command`, so a fish chain
whose leading command was rewritable lost the savings `develop` delivers —
`git diff HEAD~3 HEAD; and true` went from 76.9% to 0%. The script's own
commands are rewritten first and the wrap carries the result, honouring
`exclude_commands` and `transparent_prefixes` like any other rewrite.

**It was the only path emitting a command RTK could not attest.** It now
refuses everything the unattestable gate refuses — command and process
substitution, file-target redirects — plus fish's own `(cmd)` substitution,
which the bash-shaped lexer reads as a subshell and lets past. What still
reaches the wrap is a script whose only unattestable property is fish control
flow, which is the case the feature exists for, and `src/hooks/README.md` now
records why that one is exempt, what bounds it, and that Codex renders `Ask` as
a protocol-level `allow`.

Also from the review: `tests/fish_wrap_test.rs` asserts on the fish-absent
branch instead of returning early, so the file is no longer inert on a runner
without `fish`, and `decide`'s gate-order doc lists the wrap.
pashifika added a commit to pashifika/rtk that referenced this pull request Oct 17, 2026
Round-2 review of rtk-ai#4127.

**A quote in a comment blinded the gates** (new in the previous push). The
classification moved to the comment-stripped text but the three gates still
read the raw script, and an apostrophe there opens the shared lexer's quote
state — everything after it collapses into one argument, so no redirect and no
substitution was ever seen while the classifier saw clean fish:

    test -d src; and git status # don't
    cat secrets.env > /tmp/leak

wrapped, and running the emitted form wrote the file out. The gates now read
the same stripped code the classification does, so the second line is seen and
the script defers, as it does on `develop`.

**A comment glued to an operator was still read as code.** `strip_comments`
started a word only after a space, tab or newline, so `;#`, `|#` and `&#` kept
round-1's defect: `echo ${HOME:-x};# note; and more` runs in bash and dies
under fish with nothing executed. Both shells start a word after an unquoted
operator; so does this now.

**The suite was red on any runner without `fish`.** The no-fish branch of
`rewrite_keeps_the_inner_rewrite_inside_the_wrap` asserted a defer, but the
script's leading command is rewritable on its own, so the answer there is the
ordinary rewrite — asserting that exact string pins the wrap as genuinely off
rather than merely absent. `rewrite_defers_fish_script_with_divergent_backslash`
no longer returns early either.

CI installs `fish` on the Linux and macOS test runners, since without it the
wrap never fires and the feature's only code path is never exercised; Windows
has none to exercise, where the wrap is disabled by design. The fixtures no
longer name `/etc/passwd`, which the repo's own semgrep rule flags.

Docs: the script no longer travels byte-identical — its commands are rewritten
first — and `README.md`, `src/hooks/README.md` and the French FEATURES guide
now say so, along with the substitution and file-redirect refusals that make
`for f in (ls) … end` defer.
pashifika added a commit to pashifika/rtk that referenced this pull request Oct 17, 2026
Round-3 review of rtk-ai#4127.

`&` is the one boundary character bash and fish read differently. Since fish
3.0 `&` is job control only at the end of a job, so a `#` glued to it opens a
comment in bash and continues the word in fish:

    bash -c 'echo A&#y'   ->  A        (comment)
    fish -c 'echo A&#y'   ->  A&#y     (one literal word)

`strip_comments` took bash's reading, which hid everything after the `#` from
the gates while the emitted wrap carried it and fish ran it —
`test -d src; and echo HIT&#y > /tmp/LEAK` wrapped and wrote the file. Taking
fish's reading instead re-admits comment text as code, which is the defect
round 1 fixed. Neither is safe, so the scanner refuses: `strip_comments`
returns `None` when a comment start is glued to a lone unquoted `&`, and the
script keeps the host's own behaviour. `&&#` and `||#` still strip — both
shells agree there — and the run is counted only over syntactic ampersands, so
an escaped `\&` before a lone `&` refuses too, and a run never crosses the
newline that closes a comment.
pashifika added a commit to pashifika/rtk that referenced this pull request Oct 17, 2026
Round-4 review of rtk-ai#4127. The round-3 answer peeled `and`/`or`/`not` off the
front of a segment, which was both too wide — it changed how every command is
matched against deny rules, not only fish ones — and too narrow: it only ever
looked at the start of a segment. A denied command in the condition of an `if`
or a `while` went straight through, and the wrap handed it to a fish that runs
it where bash refuses the syntax outright:

    deny: ["Bash(rm:*)"]
    if not rm -rf victim\n  echo x\nend      wrapped, and fish deleted the file
    while rm -rf victim\n  break\nend        the same

Peeling keywords chases one placement at a time, and fish starts commands in
more places than a keyword list holds. So the criterion is no longer "peel the
keyword" but **never wrap a script a deny rule could match anywhere**: every
word is treated as a possible command start and the permission gate's own
`command_matches_pattern` answers for each run of words
(`permissions::deny_matches_any_word_run`). The words come from the same
comment-stripped code the classification and the gates read, dequoted through
the shared `lexer::shell_split`, so an apostrophe in a comment cannot hide
them and `'rm'`, `r'm'` and `"rm"` all read as `rm`.

The rules are the host's own, the ones its verdict was judged against:
`decide` takes them, `decide_hook_action` loads them once per host instead of
twice, and `AgentPath` supplies the set its own `verdict` reads.
`check_command` goes with that — `rewrite_cmd` was its only caller and now
loads Claude's rules explicitly, which is what it documented already.

The keyword peel leaves this PR entirely; with the wrap deciding for itself,
nothing here needs it.

Conservative by construction: a denied program named as a plain argument keeps
the wrap off too. That costs a rewrite, never a guarantee. With no deny rules
nothing changes, and a rule naming something else leaves the wrap alone.
`deny_keeps_the_wrap_off_in_every_placement` pins all twelve spellings through
`rtk rewrite` under a real `settings.json`, asserting *never wrapped* rather
than a particular exit code.
@pashifika
pashifika force-pushed the feat/fish-script-wrapping branch from a8d6c31 to 3e9fbef Compare October 17, 2026 13:54

@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 8 (landing) — CHANGES REQUESTED

Claim: on Claude Code's own hook, a command string that is provably fish is handed back as rtk run --shell fish -c '<script>', with its own commands rewritten first, instead of failing in a POSIX host layer.
Scope: accept if narrowed to "opt-in (hooks.wrap_fish_scripts defaults to false), and every wrap is an explicit ask". This changes the round-1 verdict, and the reason is a claim of mine that turned out wrong; see item 1.
Ran: Claude Code 2.1.305 against a mock API, with rtk hook claude as its PreToolUse hook, × 8 rule setups × develop/3e9fbeff/merged; rtk hook claude × 18 settings states × 6 commands × 4 binaries (develop 0e596d1, 59fbc04d, 3e9fbeff, 3e9fbeff merged onto 0e596d138) in a fish 4.9.1 container; 11 host payload paths, each through its own rtk hook <agent> entry; rtk hook check × 15 agents; rtk rewrite × 5 host settings × 3 commands; transparent_prefixes; a real pipeline swap rule; the flag off. The classifier is unchanged since 59fbc04d, so its round-7 rows stand. Invariants: under the hook, every decision. Savings: n/a (no filter).
Pins: 26 decisions this push adds, 26 mutated, 6 survived (the Codex, Vibe, Trae, VS Code and Copilot CLI hook paths, and the pipeline Swap gate).
Matrix: executed this round. Pin check: executed this round.
CI @3e9fbeff: all green (15/15).

Every row I asked for in round 7 passes on 3e9fbeff except the pins. Each one is listed under "Checked and correct". Your rebase onto e542dca was right on every row, and so were both gaps you found in your own attestation: the bare Bash deny entry, and the file RTK cannot read.

Blocks merge (3)

  1. src/hooks/permissions.rs:107: Claude Code judges the wrapped string with rules this gate cannot see. An allow rule matching rtk runs the script without approval. A deny rule from --settings, from --disallowedTools, or from a project the agent has cd'd away from stops applying. My round-7 claim, which you implemented exactly, was wrong. The narrowed claim, measured, is in the thread.
  2. tests/fish_wrap_test.rs:342: unpinned behaviour (which host may wrap). Six decisions pass the suite when flipped, and each changes real output. The exact edits and payloads are in the thread.
  3. src/hooks/rewrite_cmd.rs thread: the rebase. develop moved again, and e27b47d1c conflicts in permissions.rs. The resolution and its rows are in the reply there.

Optional — will not hold merge

  • claude_hook_deny_rules() runs on every rtk hook claude call, fish or not, so the settings files are walked twice. When a file is broken, its parse warning is also printed twice as often (4 times where develop prints 2). Computing it only after wrap_fish matched, or having one walk return both, removes both. Round 7 measured no time cost.
  • Four doc passages describe paths this head cannot take:
    • rewrite_cmd.rs:87 and TECHNICAL.md:130-131 describe a fish wrap through rtk rewrite, which never wraps now;
    • decision.rs:222 says ApprovalOwner::apply leaves a wrap alone, but it no longer checks for one, and no wrap reaches it;
    • permissions.rs:166 says the wrap reads its rules through load_rules_for, but it no longer does.

Checked and correct — no need to re-verify

  • Hosts. Copilot, Cursor (global deny and project-only deny), Gemini (exclude lists), Droid (no settings, and a denylist), Codex, Antigravity and OpenCode never wrap, through their own payload hooks.
  • rtk hook check. For 15 agents it shows a wrap only for claude.
  • rtk rewrite. With no host and with each delegate name, it never wraps, and it matches develop's output and exit codes.
  • rtk hook claude. It does not wrap with a deny entry in any of the four files or in CLAUDE_CONFIG_DIR. Nor with a bare Bash or a Read(…) deny, or with a settings file that is unparsable, not UTF-8, unreadable, or a directory. It wraps, keeping the inner rewrite, for deny: [], allow-only and ask-only settings.
  • transparent_prefixes = ["nice"]. nice bash -c '<denied command>' exits 1, as on develop.
  • The Swap gate. Driven with a real swap rule, its own outcome matches develop for a fish chain, a begin block and a plain command.
  • The flag and the delegate path. wrap_fish_scripts = false turns the wrap off. A wrap typed by the user through RTK_REWRITE_HOST=openclaw exits 0, as on develop.
  • Docs. FEATURES.md is English throughout, and the fish paragraph sits in quick-start.md.
  • CI installs fish on Linux (apt) and macOS (brew), and is green on this head. Locally, fish 4.9.1 passes fish_wrap_test and rewrite_shell_wrapper_test.

Your questions

  • "ApprovalOwner::apply is now simpler … Tell me if you would rather I had left it." → Keep it. A user-typed wrap through OpenClaw is back to develop's exit 0. Under the narrowed claim, a wrap's ask travels as permissionDecision on the Claude hook, not through ApprovalOwner, so is_wrapped would have no job.
  • "I could not make a swap rule fire locally" → the recipe is in the thread on fish_wrap_test.rs. rtk pipeline validate warns (W023) that only a digest task runs a swap.

Next

Three changes, then this merges:

  • the opt-in default and the explicit ask, with the docs saying what turning it on costs;
  • the six tests;
  • the rebase onto develop with the one conflict resolved as described.

Once that is on the branch, I push the round-3 test fixup on top: both fish tests isolated through common::native_command, and the execution test no longer #[ignore]d. I approve when CI is green.
Rounds: landing. Threads: 3 open (waiting on you), 1 resolved this round.

Comment thread src/hooks/permissions.rs Outdated
Comment thread tests/fish_wrap_test.rs Outdated
A host can evaluate the Bash tool's command string with a POSIX/zsh layer
while the user's shell is fish, so a fish script (`if … end`, `; and`, `; or`)
fails to parse before RTK is ever consulted. Nothing downstream can recover
from that: the command is already gone by the time a hook would see the error.

Classify the command instead. A script is *provably* fish when a fish-only
keyword (`end`, `begin`, `switch`, `and`, `or`, `not`) sits at a command
boundary and nothing a POSIX shell needs appears anywhere — `then`, `fi`,
`do`, `done`, `esac`, `elif`, `[[`, a heredoc, or a backtick, none of which
fish has. Such a script is handed back as

    rtk run --shell fish -c '<script>'

one quoted argument every host layer parses as a single command, with the
script byte-identical inside it.

The classifier is this module's own; it does not ask the shared lexer to treat
fish keywords as unattestable, because that lexer reads the host's command
string as bash.

Gates, in order, with the pure ones first so a non-fish command reaching this
path does no I/O: not Windows, not already delegating (`rtk …` or an explicit
`-c` wrapper), classified fish, no `\\` or `\'` — the only two sequences where
fish single-quote semantics diverge from POSIX, so every wrapped script
round-trips byte-identically under sh/bash/zsh *and* fish — then
`hooks.wrap_fish_scripts` (default on) and a resolvable `fish` binary.

The wrap is never auto-allowed: its strongest verdict is `Ask`, because the
script's content is not attested. A deny rule still outranks it.
README auto-rewrite section, the hooks permission-model README, and the
French FEATURES guide (behaviour, gates, the `wrap_fish_scripts` opt-out and
the config sample).
…e refuses

Round-1 review of rtk-ai#4127, three defects, one mechanism each.

**Comment text was read as code.** Neither the classifier nor the shared lexer
models `#`, so a separator inside a trailing comment reset command position and
the next English word was matched against the fish markers:
`echo $((1+2))  # x; and y` is a working bash command that was wrapped and then
failed under fish with nothing run at all. Every shell in play agrees where a
comment starts and ends, so the span is dropped before anything is classified.
A `#` inside quotes is still an argument, and a fish script may carry a comment
of its own.

**The wrap cost the rewrite.** It preceded `rewrite_command`, so a fish chain
whose leading command was rewritable lost the savings `develop` delivers —
`git diff HEAD~3 HEAD; and true` went from 76.9% to 0%. The script's own
commands are rewritten first and the wrap carries the result, honouring
`exclude_commands` and `transparent_prefixes` like any other rewrite.

**It was the only path emitting a command RTK could not attest.** It now
refuses everything the unattestable gate refuses — command and process
substitution, file-target redirects — plus fish's own `(cmd)` substitution,
which the bash-shaped lexer reads as a subshell and lets past. What still
reaches the wrap is a script whose only unattestable property is fish control
flow, which is the case the feature exists for, and `src/hooks/README.md` now
records why that one is exempt, what bounds it, and that Codex renders `Ask` as
a protocol-level `allow`.

Also from the review: `tests/fish_wrap_test.rs` asserts on the fish-absent
branch instead of returning early, so the file is no longer inert on a runner
without `fish`, and `decide`'s gate-order doc lists the wrap.
Round-2 review of rtk-ai#4127.

**A quote in a comment blinded the gates** (new in the previous push). The
classification moved to the comment-stripped text but the three gates still
read the raw script, and an apostrophe there opens the shared lexer's quote
state — everything after it collapses into one argument, so no redirect and no
substitution was ever seen while the classifier saw clean fish:

    test -d src; and git status # don't
    cat secrets.env > /tmp/leak

wrapped, and running the emitted form wrote the file out. The gates now read
the same stripped code the classification does, so the second line is seen and
the script defers, as it does on `develop`.

**A comment glued to an operator was still read as code.** `strip_comments`
started a word only after a space, tab or newline, so `;#`, `|#` and `&#` kept
round-1's defect: `echo ${HOME:-x};# note; and more` runs in bash and dies
under fish with nothing executed. Both shells start a word after an unquoted
operator; so does this now.

**The suite was red on any runner without `fish`.** The no-fish branch of
`rewrite_keeps_the_inner_rewrite_inside_the_wrap` asserted a defer, but the
script's leading command is rewritable on its own, so the answer there is the
ordinary rewrite — asserting that exact string pins the wrap as genuinely off
rather than merely absent. `rewrite_defers_fish_script_with_divergent_backslash`
no longer returns early either.

CI installs `fish` on the Linux and macOS test runners, since without it the
wrap never fires and the feature's only code path is never exercised; Windows
has none to exercise, where the wrap is disabled by design. The fixtures no
longer name `/etc/passwd`, which the repo's own semgrep rule flags.

Docs: the script no longer travels byte-identical — its commands are rewritten
first — and `README.md`, `src/hooks/README.md` and the French FEATURES guide
now say so, along with the substitution and file-redirect refusals that make
`for f in (ls) … end` defer.
Round-3 review of rtk-ai#4127.

`&` is the one boundary character bash and fish read differently. Since fish
3.0 `&` is job control only at the end of a job, so a `#` glued to it opens a
comment in bash and continues the word in fish:

    bash -c 'echo A&#y'   ->  A        (comment)
    fish -c 'echo A&#y'   ->  A&#y     (one literal word)

`strip_comments` took bash's reading, which hid everything after the `#` from
the gates while the emitted wrap carried it and fish ran it —
`test -d src; and echo HIT&#y > /tmp/LEAK` wrapped and wrote the file. Taking
fish's reading instead re-admits comment text as code, which is the defect
round 1 fixed. Neither is safe, so the scanner refuses: `strip_comments`
returns `None` when a comment start is glued to a lone unquoted `&`, and the
script keeps the host's own behaviour. `&&#` and `||#` still strip — both
shells agree there — and the run is counted only over syntactic ampersands, so
an escaped `\&` before a lone `&` refuses too, and a run never crosses the
newline that closes a comment.
Round-4 review of rtk-ai#4127. The round-3 answer peeled `and`/`or`/`not` off the
front of a segment, which was both too wide — it changed how every command is
matched against deny rules, not only fish ones — and too narrow: it only ever
looked at the start of a segment. A denied command in the condition of an `if`
or a `while` went straight through, and the wrap handed it to a fish that runs
it where bash refuses the syntax outright:

    deny: ["Bash(rm:*)"]
    if not rm -rf victim\n  echo x\nend      wrapped, and fish deleted the file
    while rm -rf victim\n  break\nend        the same

Peeling keywords chases one placement at a time, and fish starts commands in
more places than a keyword list holds. So the criterion is no longer "peel the
keyword" but **never wrap a script a deny rule could match anywhere**: every
word is treated as a possible command start and the permission gate's own
`command_matches_pattern` answers for each run of words
(`permissions::deny_matches_any_word_run`). The words come from the same
comment-stripped code the classification and the gates read, dequoted through
the shared `lexer::shell_split`, so an apostrophe in a comment cannot hide
them and `'rm'`, `r'm'` and `"rm"` all read as `rm`.

The rules are the host's own, the ones its verdict was judged against:
`decide` takes them, `decide_hook_action` loads them once per host instead of
twice, and `AgentPath` supplies the set its own `verdict` reads.
`check_command` goes with that — `rewrite_cmd` was its only caller and now
loads Claude's rules explicitly, which is what it documented already.

The keyword peel leaves this PR entirely; with the wrap deciding for itself,
nothing here needs it.

Conservative by construction: a denied program named as a plain argument keeps
the wrap off too. That costs a rewrite, never a guarantee. With no deny rules
nothing changes, and a rule naming something else leaves the wrap alone.
`deny_keeps_the_wrap_off_in_every_placement` pins all twelve spellings through
`rtk rewrite` under a real `settings.json`, asserting *never wrapped* rather
than a particular exit code.
Round-8 review. Claude Code does not take the hook's answer as final for a
rewrite: with `updatedInput` and no `permissionDecision`, it runs its own
permission check on what came back. For a wrap, what it checks is one
`rtk run --shell fish -c '…'` command, with the script's own commands invisible
to every rule it has. Measured against Claude Code 2.1.305: an
`allow: ["Bash(rtk:*)"]` ran the whole script unprompted, and a deny from
`--settings`, from `--disallowedTools`, or from a project the agent had `cd`'d
away from stopped applying.

Two holes, and RTK can close only one of them. The gate reads deny entries, so
an allow rule matching `rtk` and an ask rule naming a script command were never
considered — both are closed by the hook stating its own decision. The second
is the rule sources that never reach a hook at all; nothing here can see them.

So the wrap becomes a thing a user asks for, and says so when it happens:

- `hooks.wrap_fish_scripts` defaults to `false`. Without it the Claude hook
  answers exactly as `develop` does.
- Every wrap the hook emits carries `"permissionDecision": "ask"`. An ordinary
  rewrite is untouched: an allow rule still renders as `allow`.
- The user docs say what turning it on costs — the script reaches the prompt as
  one `rtk run` command, and a rule RTK cannot read becomes that prompt rather
  than a refusal.

The deny-entry and read/parse attestation stays as it was: with the flag on, a
rule RTK *can* see still keeps the wrap off entirely.

Six decisions this branch added still passed the suite when flipped, each
changing real output. `only_the_claude_hook_wraps` now drives Codex, Vibe,
Trae, the VS Code hook and the Copilot CLI through their own payload shapes —
the Codex row sent `"tool_name":"shell"`, which that path ignores, so it passed
whatever Codex decided — and each row carries a control proving the payload
reaches the decision at all. `the_pipeline_swap_gate_never_sees_a_wrap` drives
a real swap rule, with the swap itself as the control.

Rebased onto `develop`, whose loader now fails closed: a settings file that
exists but cannot be read or parsed warns and pushes `ask *`. Both behaviours
are kept in `load_permission_rules_tracking`, with `every_file_read` cleared
beside each push, and `load_permission_rules_from(paths)` kept as the wrapper
`develop`'s tests call. One behaviour moves with it, correctly: an empty
settings file counts as read, because `develop` skips it — it holds no rule.

Also from the review's optional list: the attestation is computed only for a
command that classifies as fish, so an ordinary command no longer walks the
settings twice, and four doc passages that described paths this head cannot
take are corrected.
@pashifika
pashifika force-pushed the feat/fish-script-wrapping branch from 3e9fbef to 62cf6ba Compare October 21, 2026 00:29
@pashifika

Copy link
Copy Markdown
Contributor Author

@KuSh — all three on 62cf6ba, rebased onto develop @ 5ab6291. No apology needed on item 1: the hook contract you measured is not something I could have read off the code, and the measurement is what settles it.

Item 1 — opt-in, and the ask is explicit

Your reading is right, and it changes the shape of the feature rather than a detail of it: with updatedInput and no permissionDecision, Claude Code judges rtk run --shell fish -c '…' with every rule source it has, and the script's own commands are not in that string.

  • hooks.wrap_fish_scripts now defaults to false. Both the Default impl and serde's default, so a config file that omits the field is off too.
  • Every wrap carries "permissionDecision": "ask". An ordinary rewrite is untouched — an allow rule still renders as allow.
  • The deny-entry and read/parse attestation is unchanged. With the flag on, a rule RTK can see still keeps the wrap off entirely.

Your rows, on this head:

row result
default config, test -d src; and git status nothing, as on develop
wrap_fish_scripts = true, no rules wrap with "permissionDecision":"ask", inner rewrite kept (rtk git diff … inside)
the same with allow: ["Bash(rtk:*)"] same output, "ask" asserted on the JSON field
the same with allow: ["Bash(rtk:*)"] + ask: ["Bash(git:*)"] same
any deny entry, or a file RTK cannot read or parse no wrap

Pinned by the_hook_does_not_wrap_unless_asked_to (no config, false, and a config that exists without the field) and a_wrap_carries_an_explicit_ask. Flipping either default, or dropping the permissionDecision, turns one of them red — I ran each mutation rather than reasoning about it.

The docs say what turning it on costs, in quick-start.md, FEATURES.md and src/hooks/README.md: the script reaches the prompt as one rtk run command, and a rule RTK cannot read — --settings, --disallowedTools, managed settings, a project the agent has cd'd away from — becomes that prompt instead of a refusal.

Item 2 — the six pins

All six turn a test red now, each verified by building the mutant:

mutation test
Codex / Vibe / Trae hook given the Claude answer only_the_claude_hook_wraps
the VS Code hook, and the Copilot CLI only_the_claude_hook_wraps
Swap gate Unattested → NoneConfigured the_pipeline_swap_gate_never_sees_a_wrap

The Codex row was exactly as you diagnosed — it sent "tool_name":"shell", which process_codex_payload ignores, so it passed whatever that path decided. It now sends Bash with permission_mode. Every row in that table also carries a control: the same payload with git status must come back as rtk git status, so a row cannot pass because the payload never reached the decision. Copilot gets three rows, one per shape (VS Code hook, IDE tool call, CLI toolArgs).

Thank you for the swap recipe — it fires from $XDG_CONFIG_HOME/rtk/pipeline.toml exactly as you wrote it. The new test uses the swap of a plain git status as its control and then asserts the fish chain is not swapped into sh -c.

Item 3 — the rebase

Resolved as you specified: develop's NotFound skip, both warnings and both ask_rules.push("*") kept, scan.every_file_read = false beside each, the empty-file skip kept, and load_permission_rules_from(paths) kept as the wrapper over the tracking walk with claude_hook_deny_rules passing &get_settings_paths(). The empty-file behaviour moves with it, as you said it should.

Optional, both done

  • The attestation is now computed only when the command classifies as fish — a pure check, no I/O — so an ordinary command walks the settings once, as on develop, and a broken file warns the same number of times.
  • The four stale passages are corrected: rewrite_cmd.rs and TECHNICAL.md no longer describe a wrap through rtk rewrite, decision.rs points at the hook's own permissionDecision instead of ApprovalOwner, and permissions.rs no longer claims the wrap reads its rules through load_rules_for.

Gates

cargo fmt --all --check                    clean
cargo clippy --all-targets -D warnings     clean
cargo test --all                           5,093 passed, 0 failed, 9 ignored
cargo check --target x86_64-pc-windows-gnu clean

(macOS 26.6.2 arm64, rustc 1.97.1, fish 4.9.3.) Ready for your round-3 test fixup on top.

KuSh and others added 2 commits October 23, 2026 16:28
`wrapped_multiline_fish_script_executes_and_propagates_exit_code` was
`#[ignore]`d, so the one case that executes a wrapped script through
`rtk run --shell fish` never ran: `cargo test --all` skips ignored tests, and
CI now installs `fish` on Linux and macOS. Enable it, and assert on both
branches the way the tests above it do: without a resolvable `fish` the
command must report exit 127 rather than appear to have run.

The direct `sh`/`zsh`/`fish -c` calls in both tests now go through
`common::native_command`, so they get the same isolated home as the rtk child:
`fish -c` sources the user's `config.fish`, and anything it prints landed on
the stdout `wrapped_quoting_round_trips_under_posix_and_fish_hosts` compares
byte for byte, failing the test for any fish user whose config greets them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ere one can be

The Claude hook decided a wrap's explicit `ask` by the shape of the rewritten
string alone. So an `rtk run --shell fish -c '…' && git status` the agent typed
itself, which the rules only rewrote around, went from `allow` to `ask`. That
happened even with `hooks.wrap_fish_scripts` off. The explicit ask now needs the
flag on, and a command that did not already start in the wrap form. `try_wrap`
never wraps a command whose first word is `rtk`, so such a command is one the
agent typed: it keeps its ordinary decision. With the flag off, no wrap exists,
and the hook answers exactly as it does without the feature.

The deny-rule attestation walks the settings a second time. It now runs only
where a wrap is possible: the flag is on and the command is provably fish. One
gate, `claude_hook_deny_rules_for`, serves both the hook and
`rtk hook check --agent claude`, so the two read the settings alike. Before,
with the flag off, both walked the settings twice for a fish command and
repeated every loader warning. With the flag on, `rtk hook check` did the same
for every command.

Several comments still said Claude Code takes RTK's decision as final, or that
the files RTK reads are all of its rules. They now say what holds: RTK reads
those files, and the hook asks explicitly for every wrap it builds.
`hooks.wrap_fish_scripts` turns the wrap on, not off.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@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 9 (landing) — APPROVED, follow-ups filed

Claim: with hooks.wrap_fish_scripts = true, Claude Code's own hook hands a command string that is provably fish back as rtk run --shell fish -c '<script>'. The script's own commands are rewritten first, and the decision is an explicit ask.
Scope: accept if narrowed to "opt-in, and every wrap is an explicit ask" (round 8). Met.
Ran: Claude Code 2.1.305 against a mock API, with rtk hook claude as its PreToolUse hook: 8 rule setups × develop/62cf6ba7/fixup × flag off and on. Also, in a fish 4.9.1 container, the 172-row hook matrix (rtk hook claude × 18 settings states, 6 hosts' payload hooks, rtk hook check × 15 agents, rtk rewrite × 5 hosts, transparent_prefixes, a real swap rule) × flag off and on × develop 0ac602b, 3e9fbeff, 62cf6ba7, merged onto develop, and the fixup. Separately, the fixup's flag-off parity was re-run over 1428 runs. Invariants: under the hook, every decision. Savings: n/a (no filter).
Pins: 32 decisions on 62cf6ba7, 32 mutated, 0 survived. The fixup's own 6 decisions: 6 mutated, 0 survived.
Matrix: executed this round. Pin check: executed this round.
CI @5c1bf7056: all green.

Your round-8 rows all hold on 62cf6ba7:

  • Default config. All 172 matrix rows are identical to develop, and so are all 8 Claude Code setups.
  • Flag on. Every wrap carries the explicit ask. In Claude Code the script never runs unprompted: not with Bash(rtk:*) allowed, not with an ask rule, not with a --settings or --disallowedTools deny, and not in a project the agent cd'd away from. A deny RTK can see still denies.
  • Pins. All six turn a test red.
  • Rebase. Resolved exactly as described.

Blocks merge — none

Fixup pushed and code-reviewed: 205a7d44e, 5c1bf7056

  • 205a7d44e is the round-3 test items I owed you. Both fish tests now call sh, zsh and fish -c through common::native_command, so a greeting in the user's config.fish no longer lands on the compared stdout. The wrapped-script execution test is no longer #[ignore]d; it asserts exit 127 when fish is absent.
  • 5c1bf7056 removes two places where this head still differed from develop with the flag off. Both were measured.
    • An rtk run --shell fish -c '…' && git status the agent typed itself, which the rules only rewrote around, went from allow to ask. The explicit ask matched the rewritten string alone. It now needs the flag on, and a command that did not already start in that form; try_wrap never wraps a command whose first word is rtk.
    • For a fish command, the hook walked the settings a second time (the attestation), and so did rtk hook check, so a broken file warned 4 times instead of 2. One gate, claude_hook_deny_rules_for, now serves both, and it attests only when the flag is on and the command is provably fish.
    • Comments that still said Claude Code takes RTK's decision as final now say what holds. decision.rs said the flag turns the wrap off; it turns it on.
    • Pinned by a_typed_wrap_form_keeps_its_decision and a_fish_script_reads_the_settings_no_more_often_when_the_wrap_is_off, with each new condition mutated.

Filed as follow-ups

  • #4762 hook claude: mark a fish wrap in the decision instead of matching the rewritten string. With the flag on, a typed form behind a leading line continuation still gets an ask. That is a prompt only, because a settings deny still wins over a hook's ask. A fish command that try_wrap refuses anyway still walks the settings twice.

Optional — will not hold merge

  • try_wrap classifies the trimmed command. Take a command led by a form feed or a no-break space, which bash refuses (exit 127) and fish reads as a blank. The hook correctly defers it, but rtk hook check --agent claude shows a wrap.
  • fish -c in wrapped_quoting_round_trips_under_posix_and_fish_hosts still sources the system-wide conf.d. fish --no-config would make the comparison independent of the machine.

Your questions

  • "Ready for your round-3 test fixup on top." → Done in 205a7d44e, rewritten for the current test-isolation helpers.

Next

Nothing — approving.
Rounds: landing. Threads: 0 open, 3 resolved this round.

@KuSh
KuSh merged commit 32b2ec4 into rtk-ai:develop Oct 23, 2026
14 checks passed
@rtk-release-bot rtk-release-bot Bot mentioned this pull request Oct 23, 2026
@pashifika
pashifika deleted the feat/fish-script-wrapping branch October 24, 2026 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants