Skip to content

fix(rewrite): strip rtk prefix from shell builtins - #2534

Closed
YOMXXX wants to merge 1 commit into
rtk-ai:developfrom
YOMXXX:fix/rewrite-rtk-shell-builtins
Closed

YOMXXX wants to merge 1 commit into
rtk-ai:developfrom
YOMXXX:fix/rewrite-rtk-shell-builtins

Conversation

@YOMXXX

@YOMXXX YOMXXX commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • strip redundant rtk prefix from shell builtins during rewrite
  • preserve already-prefixed RTK commands such as rtk git status
  • cover simple and compound shell-builtin rewrites

Fixes #2508

Tests

  • cargo +1.93.0 fmt --all -- --check
  • cargo +1.93.0 test --all
  • cargo +1.93.0 clippy --all-targets -- -D warnings

@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 — REJECT

Claim: strip a redundant rtk prefix from shell builtins during rewrite, so rtk cd /tmp && … becomes a command the shell can run.

Scope: reject. The bug is real and worse than #2508 describes, but fixing it in the rewrite engine opens a permission bypass, and it does not reach the agents that actually hit it — the guidance that generates rtk cd is the fix, tracked in #3969.

Ran: rtk rewrite over ~80 inputs on three binaries — merge-base e3f60e98, head 17b1ff22, and the head merged onto latest develop 8e9aa04c — plus rtk hook claude against isolated permission configs, real execution of rtk cd/trap/declare/eval/env cd/sudo cd/exec cd, and five source mutations against the suite. No savings figure applies: this changes the rewrite path and produces no filtered output.

CI @17b1ff22: all green — fmt, clippy, doc review, benchmark, Security Scan, semgrep, test on ubuntu/macos/windows, CLA signed. Merge with latest develop is clean; fmt, clippy --all-targets (0 warnings) and cargo test --all (3397 passed) all pass on the merge. The rejection is not about quality of execution.

Blocks merge (1)

1. src/discover/registry.rs — the emitted command is no longer the command the permission verdict was computed on, so a deny rule is bypassed and an rtk-wide allow rule auto-approves what replaces it.

Every hook computes the verdict on the raw input and emits the rewritten string — decide_from_verdict(cmd, permissions::check_command_for(cmd, host)) (src/hooks/hook_cmd.rs:274). That held only because a rewrite always went foo … → rtk foo …, so the checked string and the emitted string shared a prefix. Stripping inverts that, and command_matches_pattern (src/hooks/permissions.rs:406) is a word-boundary prefix match, so Bash(source:*) never matches rtk source x.

Isolated HOME, permissions: {"allow": ["Bash(rtk:*)"], "deny": ["Bash(source:*)", "Bash(export:*)"]}, PR merged onto latest develop:

$ echo '{"tool_name":"Bash","tool_input":{"command":"rtk source /tmp/evil.sh"}}' | rtk hook claude

# develop:   (no output — Defer; Claude Code applies its own rules to `rtk source …`)
# this PR:   {"hookSpecificOutput":{...,"updatedInput":{"command":"source /tmp/evil.sh"},
#                                   "permissionDecision":"allow"}}

An explicitly denied command, emitted with an explicit auto-allow. With no allow rule the verdict degrades to Ask, but the deny is still bypassed. Compound forms are protected by the #1213 per-segment rule, so this is the single-segment case. Control (git status → rtk git status) is identical on both binaries, so the harness is sound.

This is the mirror image of the bypass @rmsc reported on #2508: the PR closes that one for twelve names and opens this one. Both are now #3970.

Why narrowing does not rescue it

The strip is the PR, so there is no smaller version of it — and the two properties that make it unmergeable are not in the part that could be cut:

  • The fix lives in hooks/, not in registry.rs. The verdict would have to be recomputed on the rewritten command, taking the stricter of the two. #3955 is consolidating exactly that decision path, so this belongs after it, not before.
  • It would not have helped either reporter. Hookless agents (cline, kilocode, windsurf) get rtk-awareness-full.md unconditionally per hooks/README.md:18 and never call rtk rewrite. And rtk cd /tmp executed literally still exits 127 with this merged — only the hook-rewrite path changed.

Checked and correct — the work itself is sound

None of these is why it is being closed.

  • No name collision. All twelve names are unrouted (rtk rewrite "<name> x" → exit 1 for each). test, read and env are correctly absent — they are real RTK subcommands.
  • Word-bounded match. rtk cd.. and rtk .hidden keep the prefix; strip_word_prefix requires an exact match or a following space, and . does not swallow dotfiles.
  • Both registration points patched. starts_with("rtk ") appears exactly twice in registry.rs; the PR covers both, and deleting either call site fails a test (mutation-verified).
  • Prefix recursion is not a shell regression. sudo rtk cd, env rtk cd, exec rtk cd, uv run rtk cd all exited 127 on develop and still do — a different message, not a new wrong answer. noglob rtk cd → noglob cd and FOO=1 rtk cd → FOO=1 cd are outright fixed.
  • Compound, redirect and newline paths all reassemble correctly, and the second segment keeps develop's routing.
  • Quoted occurrences untouched — git commit -m "rtk cd /tmp" unchanged.
  • No conflict with the open refactors. #3681 shares no file; #3955 does not touch registry.rs; #3349 only adds a test there.

Hypotheses built and dropped

  • The strip bypasses the sudo guard and turns a safe passthrough into a broken command. Dropped — both forms exit 127 on develop too.
  • One of the twelve names shadows a routed command. Dropped — all twelve pass through bare.
  • rtk cd /tmp > out.txt loses the redirect. Dropped — a file redirect makes the top-level path decline before the strip, on both.

One gap found that is worth keeping

Deleting the redirect_suffix re-append in rewrite_segment_inner — which silently drops 2>&1 from the command the shell finally runs — passes the entire 3397-test suite, and so does replacing the word-bounded match with a bare starts_with. Not this PR's problem now that it is closing, but recorded in case any of this returns.

Next

Closing. @YOMXXX — thank you for this, and sorry it took a wrong turn on our side rather than yours: the diagnosis in #2508 was accurate, your implementation was correct for what it claimed, it covered both call sites, and CI was green on three platforms. What sank it is a coupling in hooks/ that nothing in the PR or the issue pointed at.

The fix moved to #3969, which is a wording change in three instruction files — hooks/rtk-awareness-full.md:3-6, RTK_INSTRUCTIONS (src/hooks/init.rs:241) and COPILOT_INSTRUCTIONS (src/hooks/init.rs:5925) — scoping "prefix every shell command" to programs on PATH and naming the builtins not to prefix. It reaches the hookless agents that the rewrite path cannot. That one is yours if you want it.

Rounds: 1/3, ending here. Threads: 0 open.

@KuSh KuSh closed this Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rtk rewrite "rtk <builtin>" returns unchanged (exit 3) instead of stripping prefix — cd and other builtins silently no-op

2 participants