fix(discover): require token boundaries for rewrite rules by axelray-dev · Pull Request #4014 · rtk-ai/rtk

axelray-dev · 2026-09-13T05:22:48Z

Summary

Fix 22 rewrite rules end in a bare subcommand alternation with no word boundary (git branchless → rtk git branchless) #4009 by requiring a token or shell delimiter after supported subcommands in native and TOML-backed rewrite rules.
Add table-driven coverage for longer command names while preserving valid subcommand rewrites.

Test plan

cargo fmt --all && cargo clippy --all-targets && cargo test --all (formatting and clippy passed; the full suite has one Git-version assertion failure because Git 2.43 reports not a non-negative integer while the test expects not an integer)
cargo test discover::registry::tests
Manual testing: target/debug/rtk rewrite leaves git branchless status, cargo builder, and terraform planner unchanged and rewrites git branch status, cargo build --release, and terraform plan

rtk-wshm-sync-bot · 2026-09-13T05:28:38Z

wshm · Automated triage by AI

📊 Automated PR Analysis


🐛 Type	`bug-fix`
🟡 Risk	`medium`

Summary

Fixes false-positive rewrite matches in the discover rule engine by requiring a token or shell delimiter after supported subcommands (e.g. 'git branch' no longer wrongly matches 'git branchless'). Adds regex boundary anchors across many rules in rules.rs and a table-driven test covering false-positive and valid subcommand cases.

Review Checklist

Tests present
Breaking change
Docs updated

Linked issues: #4009

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

KuSh

Round 1 of 3 — CHANGES REQUESTED

Claim: fix #4009 by requiring a token or shell delimiter after the subcommand alternation in 21 native rewrite rules and 5 TOML match_command patterns.

Scope: accept (frozen for later rounds). The claim is worth merging as stated; one regression to fix. #3676 fixes the 22nd arm (pnpm) and touches the same two files but not the same lines — complementary, either order merges.

Ran: 0f8b26c7 (merge-base) vs 98f8fffc, one CARGO_TARGET_DIR each, md5-distinct binaries. rtk rewrite over the issue's 22 repros, a valid-command set, a boundary-character matrix (every bash metacharacter plus NBSP / CR / VT / FF / EN SPACE), and three source mutations. Savings floor is not applicable — this changes routing, not filter output.

CI @98f8fffc: never ran. Only license/cla is green, check-target skipped, run 34740154437 is action_required — the fork-PR state waiting on a maintainer, so this PR has never actually been built by CI. Local gate on this PR merged into latest develop (6d104308, clean merge) is green: fmt clean, clippy 0 warnings, cargo test --all 3640 passed / 0 failed.

Blocks merge (1, frozen at round 1)

src/discover/rules.rs:577 — namespaced rake/rails test tasks lose their rewrite. rake test:unit, rake test:models, rails test:system and bundle exec rake test:unit all rewrite on develop and stop on this head. Details and a verified one-character fix are in the line comment.

Optional — will not hold merge

\s is not the boundary this repo defines. lexer.rs:60-65 states it outright: "Bash's default $IFS is exactly space/tab/newline — not Rust's char::is_whitespace(), which wrongly includes non-IFS Unicode whitespace like NBSP". The regex crate's \s is \p{White_Space}, i.e. exactly what that comment forbids. Still false-positive on this head: git status<NBSP>foo, git status<EN SPACE>foo, git status<VT>foo, git status<FF>foo all rewrite. Not a regression — develop does it too (uv run<NBSP>x rewrites via the untouched ^uv\s+run(?:\s|$)), so this is a pre-existing class the PR replicates into 21 more sites while its stated purpose is to model token boundaries. (?:[ \t\r]|$|[;|&()<>]) is suite-green as-is; [ \t] additionally needs test_lone_cr_line_gets_a_single_prefix updated, which is arguably correct — by that test's own comment ("bash keeps git log glued to the preceding word") the command's argv[1] is status\rgit, so under #4009's logic it shouldn't be rewritten at all.
The new test doesn't close the class. test_subcommand_rules_require_token_boundaries is a hardcoded 24-string list. Inject a new unanchored rule — ^nomad\s+(status|job|alloc) — and nomad status-all → rtk nomad status-all, the same defect, while the test passes. #4009 asked for the iterating form (for (i, rule) in RULES.iter().enumerate()), and that form fails and names the offending pattern. In fairness the current test does catch a regression of the rules it names: reverting the go boundary fails it on go vetting. I have the iterating version written and verified locally and will bring it along rather than spend a round on it.
The TOML half's metacharacter alternatives are unreachable. match_command is matched against argv joined by spaces (main.rs:1555-1564), where a metacharacter can never be a separator; and all five commands have a RULES entry, so classify_command returns Supported before the TOML branch is consulted. Only (\s|$) can ever fire — which is what the 13 sibling filters already use (tofu-*, pulumi-*, mix-*, df, ps, ssh, liquibase). The added group is also capturing where the Rust side is (?:…).
Precedence widening on version-pinned npx. REGEX_SET.matches(...).last() wins, so when a tool rule stops matching, the earlier generic ^npx\s+ takes over: npx prettier@3 --check . → rtk npx prettier@3 --check ., where develop emitted nothing (same for playwright@1.40, prisma@5). Not a regression, and consistent with how bunx prettier@3 already behaved — but it routes these into npm_cmd's npx path, which by its own comment doesn't forward piped stdin.

On the boundary itself

Worth recording, because the shape is easy to "simplify" later and two of these were nearly dropped in review:

The metacharacter half is not redundant with the segmenter. For rewrite_segment it is — ;, |, & are already gone by then, and dropping them changes not one rewrite result. But classify_command matches the whole unsegmented line, so without them git status;ls, go test& and cargo build;make classify Unsupported while the rewrite still fires, and rtk discover / rtk session under-count every compound command.
( and ) are load-bearing in both paths. lexer.rs:170 classifies them as Shellism, not Operator, and split_on_operators splits only on Operator/Pipe — so parens survive segmentation glued to the segment, and (git status; cargo build) yields the literal segment cargo build). Drop () and segmenter_consistency::subshell_grouping fails. The suite demands [()] and nothing else.
No quantifier is needed. Only the first character after the subcommand is examined, so git status now matches identically under [ \t] and [ \t]+.
Lookahead isn't an option — the regex crate rejects look-around, and these patterns go through .expect(...), so it would panic at first use rather than degrade. Consuming is safe here regardless: the only reader is registry.rs:231 captures(...) → caps.get(1), and nothing reads the match end.
No POSIX/Windows split. The lexer is bash-modelled throughout and the hook is fed whatever the agent's Bash tool ran. cmd.exe's extra separators (, = ^) and PowerShell's (, { }) would be a lexer-wide decision, not a per-rule regex one. This head correctly stops rewriting cargo build,test under the bash model.

That said — this is a fix at the end of the chain, and I'd rather say so than let the shape harden. segmenter_consistency::subshell_grouping already pins three different answers for one input: split_for_permissions strips the parens, split_command_chain glues them, and the rewrite loses the first command entirely ((git status; cargo build) → (git status; rtk cargo build); (git status) isn't rewritten at all). Its own comment calls that "a real, existing quirk … pinned here, not fixed here", and #4100 independently concludes the two sides should share one notion of the boundary. ( and ) are control operators in the shell grammar, not shellisms. I'm taking that convergence on directly as a follow-up refactor — one lexer pass shared by classify, rewrite and the permission gate — which should also absorb #1053, #1252 and #4100. When it lands these 21 suffixes become dead weight and delete cleanly. None of that blocks this PR: it removes 22 verified false positives today, including git branchless, which is a real installed tool being silently misrouted.

Checked and correct — no need to re-verify

All 22 repros in #4009 fixed. prettierish, playwrighting, prismax, next builder, sbt tester, shopify themepark were never broken on develop (the sbt rule was already anchored) — harmless to keep in the table.
Coverage is complete: across all 93 rules the only remaining unanchored pattern is ^pnpm\s+(exec|i|…), which is #3676's.
Capture-group numbering intact — the suffix is (?:…) everywhere, so caps.get(1) and the subcmd_status / subcmd_savings lookups are untouched; 441 registry tests green.
Valid commands preserved: git -C /tmp status, docker compose ps, uv pip install flask, oc get pods, go test ./..., terraform plan -out=x, (git status; cargo build), git status|head, git status;ls, git status&.
Newly stopping rewrites of git status{a,b}, git status!x, git status* is correct — those aren't metacharacters in bash, so each is a single word.

Hypotheses built and dropped

helm statuses / gradle tests are not false positives. ^helm\b and the gradlew rule (optional subcommand group + (\s|$)) are deliberate catch-alls; routing every subcommand is the design.
"The subshell case regresses" — an artifact of a contaminated binary on my side. A fresh worktree and target dir show this head handling (git status; cargo build) exactly as develop does.
"The metacharacters are untested" — they are, by subshell_grouping, just not by this PR's own test.

Your questions

"the full suite has one Git-version assertion failure because Git 2.43 reports not a non-negative integer while the test expects not an integer" → not yours. tests/guard_integration_test.rs:345 carries the identical assertion on develop; it's pre-existing and git-version-dependent, and it passes here on git 2.53. Don't let it hold your test plan — I'll deal with it separately.

Addressed the reviewer request by treating the colon in namespaced rake and Rails test commands as a valid boundary and adding regressions for rake test:unit, rails test:system, bundle exec rake test:models, and bin/rails test:integration. The targeted token-boundary test passed. The full suite reached one unrelated existing git-log error-message assertion; no touched code was involved. Please re-review commit e5518dc.

…4100) Several places decided where a word ends, and none of them used the definition bash uses. `strip_word_prefix` accepted exactly one byte as the boundary between a prefix and the command behind it: an ASCII space. Bash separates words on space, tab and newline alike, and the rule patterns match `\s+`, so a tab-separated command failed to rewrite: ls\t-la no rewrite cargo\tbuild no rewrite noglob\tls -la no rewrite The first two also classified as Supported, so `rtk discover` counted coverage the hook never delivered — the two sides disagreeing about what separates a command from its prefix. The third did not: no rule names `noglob`, so `classify_command` calls it Unsupported either way. Its rewrite is repaired here too, but nothing was over-reported for it. `strip_absolute_path` split on a literal space, so `cat\t/etc/passwd` normalised to `passwd`. `extract_base_command` and `normalize_php_tool_command_with_dirs` used `char::is_whitespace`, which is Unicode and counts NBSP — the lexer rejects exactly that in `is_word_boundary_whitespace`, whose comment says why. All of them now use it, so `cat\u{a0}/etc/passwd` stays one word as bash reads it, and the separator is carried through rather than replaced, so `noglob\tls -la` keeps its tab. The remaining `char::is_whitespace` in `join_redirect_suffix` tests whether a suffix is already separated rather than splitting a word, so it stays. Two table-wide invariants the `RULES` table has never had come with it. **Anchoring, both ends.** A pattern matches against a whole command line, so one that does not start with `^` fires on a path component, and one that ends in a bare alternation matches any command whose subcommand merely *starts* with a listed one — `git branchless` routes into `rtk git` (#4009). The 22 patterns in that state are #4014's and #3676's to fix and are named in a `PENDING` list, not exempted: an entry that no longer matches a rule fails the test, so the list can only shrink. **`rtk_cmd` does not identify a rule.** `classify_command` picks a rule by index and `rewrite_segment_inner` then looks one up again by `rtk_cmd` with `find`, whose comment claims the value is unique. It is not — `rtk php` and `rtk uv` are two rules each, so that lookup already returns a sibling. It is harmless only because every field it reads matches across each pair, and that is now asserted rather than assumed. Forcing uniqueness is the better end state but not a test-only change: the siblings are worded differently (`\b` against `(?:\s|$)`), so merging means choosing one terminator and re-checking every command each pattern currently claims. The mechanical fix is elsewhere anyway — `classify_command` already holds the matched rule's index and could pass it along instead of having the rewrite re-derive a rule from `rtk_cmd`. Left as a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

axelray-dev · 2026-09-21T19:52:24Z

Checked the current head (�5518dc) against the requested review change. The Ruby rule already includes the : boundary, and the regression matrix covers
ake test:unit,
ake test:models,
ails test:system, �undle exec rake test:models, and �in/rails test:integration. On the VPS, cargo fmt --all --check passed and the discover test suite passed: 606 passed, 0 failed. No additional code change was necessary; please re-review the current head.

axelray-dev · 2026-09-23T08:15:53Z

Addressed the remaining Round 1 regression in the current head: the rake/rails rule now accepts a colon after test, so namespaced tasks such as rake test:unit, rails test:system, and bundle exec rake test:models keep their rewrite while rake tester and rake test-helper remain untouched. The focused boundary test passes. The full local suite reached 3538 passing tests and exposed one unrelated pre-existing failure in git_log_malformed_digit_run_propagates_real_git_error; it is outside this discover-rule change. Please re-review the current head.

KuSh

Round 2 of 3 — APPROVED

Claim: fix #4009 by requiring a token or shell delimiter after the subcommand alternation in 21 native rewrite rules and 5 TOML match_command patterns.

Scope: accept (frozen at round 1).

Ran: rtk rewrite × 64 commands × before/after, on current develop (feac25d1) and on the PR merged into it, one CARGO_TARGET_DIR each, md5-distinct binaries. Re-verified after the release: develop moved 20 commits since my first pass on this head, several of them in this PR's area, so the whole matrix was re-run against the new base rather than carried over. Invariants: under the hook — took the rewrite's own output and ran it in a pinned ruby:3.3-slim container (ruby 3.3.12, rake 13.1.0) against a real Rakefile with a namespaced test:unit task and a deliberately failing test: rake test:unit → rtk rake test:unit → real rake, summary line and failure detail both preserved, exit 1 propagated, byte-identical to develop. Plus every decision branch of the changed rule. Savings floor not applicable — this changes routing, not filter output.

Matrix: executed round 1 @98f8fffc, and again this round @e5518dcc — the second time against the post-release develop.

CI @e5518dcc: 11/11 green, including test (windows-latest).

Blocks merge (0 — the round-1 item is fixed)

e5518dcc resolves the single blocking item. Measured against current develop, which is the baseline that matters:

command	develop `feac25d1`	with this PR
`rake test:unit`	`rtk rake test:unit`	`rtk rake test:unit`
`rake test:models`	`rtk rake test:models`	`rtk rake test:models`
`rails test:system`	`rtk rake test:system`	`rtk rake test:system`
`bundle exec rake test:models`	`rtk rake test:models`	`rtk rake test:models`
`bin/rails test:integration`	`rtk rake test:integration`	`rtk rake test:integration`

— unchanged, so the round-1 regression is gone. Meanwhile the false positives the : could have re-opened stay closed: rake tester, rails tester, rake test-helper, rake testing:unit all stop rewriting, as do the 22 in #4009. rake test:unit:fast and rake test::unit do rewrite, which is right — both are boundary-terminated task names that rtk rake forwards unchanged.

Across all 64 cases the only differences against develop are the 26 intended false-positive removals and the pre-existing npx <tool>@version note below. Nothing else moved.

Optional — unchanged from round 1, will not hold merge

Still true, still not blocking, and mine to carry rather than yours to fix:

\s in the boundary is Unicode \p{White_Space}, so NBSP / EN SPACE / VT / FF count as word boundaries where bash's $IFS does not. Pre-existing on develop (uv run<NBSP>x rewrites there too, via a rule this PR does not touch), so not a regression.
The [;|&()<>] half and the TOML half, as described in round 1.
npx <tool>@version precedence widening — verified unchanged on this head and on the new base.

The class-closing test I offered in round 1 is withdrawn: a better version exists in the lexer work below, checking both ends of every pattern rather than just the tail.

Checked and correct — no need to re-verify

The four added rows in test_subcommand_rules_require_token_boundaries pin the fix, confirmed by mutation: reverting [\s:] to \s fails on the first added row with rake test:unit classified as Unsupported.
Admitting : on this one rule disturbs nothing else. Every other rule whose arguments can carry a colon is identical with and without the PR: gradle :module:test, ./gradlew :app:testDebug, npm run test:unit, docker run alpine:3.19, sbt testOnly com.foo.BarSpec, mvn surefire:test. Rake/Rails is the only rule in the table whose matched word doubles as a colon-namespace prefix in the tool's own grammar, so scoping the fix there is right rather than partial.
The rewritten command was executed, not just produced, in a pinned container — see Ran.
Merged into current develop (feac25d1, clean merge): cargo fmt --all --check clean, cargo clippy --all-targets 0 warnings, cargo test --all exit 0, 26 suites, 0 failures.

Your questions

"the full local suite … exposed one unrelated pre-existing failure in git_log_malformed_digit_run_propagates_real_git_error; it is outside this discover-rule change" → Correct, and it is not yours. tests/guard_integration_test.rs:345 asserts git's own wording (not an integer); git 2.43 says not a non-negative integer. The identical assertion is on develop, and it passes here on git 2.53 and on all three CI runners. Nothing for you to do.
"No additional code change was necessary; please re-review the current head." → Agreed. e5518dcc is complete, and re-running the full matrix against the post-release develop found nothing new.

…4100) Several places decided where a word ends, and none of them used the definition bash uses. `strip_word_prefix` accepted exactly one byte as the boundary between a prefix and the command behind it: an ASCII space. Bash separates words on space, tab and newline alike, and the rule patterns match `\s+`, so a tab-separated command failed to rewrite: ls\t-la no rewrite cargo\tbuild no rewrite noglob\tls -la no rewrite The first two also classified as Supported, so `rtk discover` counted coverage the hook never delivered — the two sides disagreeing about what separates a command from its prefix. The third did not: no rule names `noglob`, so `classify_command` calls it Unsupported either way. Its rewrite is repaired here too, but nothing was over-reported for it. `strip_absolute_path` split on a literal space, so `cat\t/etc/passwd` normalised to `passwd`. `extract_base_command` and `normalize_php_tool_command_with_dirs` used `char::is_whitespace`, which is Unicode and counts NBSP — the lexer rejects exactly that in `is_word_boundary_whitespace`, whose comment says why. All of them now use it, so `cat\u{a0}/etc/passwd` stays one word as bash reads it, and the separator is carried through rather than replaced, so `noglob\tls -la` keeps its tab. The remaining `char::is_whitespace` in `join_redirect_suffix` tests whether a suffix is already separated rather than splitting a word, so it stays. Two table-wide invariants the `RULES` table has never had come with it. **Anchoring, both ends.** A pattern matches against a whole command line, so one that does not start with `^` fires on a path component, and one that ends in a bare alternation matches any command whose subcommand merely *starts* with a listed one — `git branchless` routes into `rtk git` (#4009). The 22 patterns in that state are #4014's and #3676's to fix and are named in a `PENDING` list, not exempted: an entry that no longer matches a rule fails the test, so the list can only shrink. **`rtk_cmd` does not identify a rule.** `classify_command` picks a rule by index and `rewrite_segment_inner` then looks one up again by `rtk_cmd` with `find`, whose comment claims the value is unique. It is not — `rtk php` and `rtk uv` are two rules each, so that lookup already returns a sibling. It is harmless only because every field it reads matches across each pair, and that is now asserted rather than assumed. Forcing uniqueness is the better end state but not a test-only change: the siblings are worded differently (`\b` against `(?:\s|$)`), so merging means choosing one terminator and re-checking every command each pattern currently claims. The mechanical fix is elsewhere anyway — `classify_command` already holds the matched rule's index and could pass it along instead of having the rewrite re-derive a rule from `rtk_cmd`. Left as a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(discover): require token boundaries for rewrite rules

98f8fff

rtk-wshm-sync-bot Bot added bug Something isn't working discover rewrite-rules labels Sep 13, 2026

axelray-dev marked this pull request as ready for review September 15, 2026 14:36

KuSh self-assigned this Sep 18, 2026

KuSh requested changes Sep 18, 2026

View reviewed changes

Comment thread src/discover/rules.rs Outdated

KuSh mentioned this pull request Sep 19, 2026

fix(discover): split words the way bash does, and pin the rule table (#4100) #4133

Open

2 tasks done

fix(discover): preserve namespaced rake test rewrites

e5518dc

KuSh approved these changes Sep 25, 2026

View reviewed changes

KuSh merged commit 60446ff into rtk-ai:develop Sep 25, 2026
11 checks passed

rtk-release-bot Bot mentioned this pull request Sep 25, 2026

Next Release #4228

Merged

Navigation Menu

Repository navigation

fix(discover): require token boundaries for rewrite rules - #4014

fix(discover): require token boundaries for rewrite rules#4014
KuSh merged 2 commits into
rtk-ai:developfrom
axelray-dev:fix/discover-token-boundaries

axelray-dev commented Sep 13, 2026

Uh oh!

rtk-wshm-sync-bot Bot commented Sep 13, 2026

Uh oh!

KuSh left a comment

Uh oh!

Uh oh!

axelray-dev commented Sep 19, 2026

Uh oh!

axelray-dev commented Sep 21, 2026

Uh oh!

axelray-dev commented Sep 23, 2026

Uh oh!

KuSh left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

axelray-dev commented Sep 13, 2026

Summary

Test plan

Uh oh!

rtk-wshm-sync-bot Bot commented Sep 13, 2026

📊 Automated PR Analysis

Summary

Review Checklist

Uh oh!

KuSh left a comment

Choose a reason for hiding this comment

Round 1 of 3 — CHANGES REQUESTED

Blocks merge (1, frozen at round 1)

Optional — will not hold merge

On the boundary itself

Checked and correct — no need to re-verify

Hypotheses built and dropped

Your questions

Next

Uh oh!

Uh oh!

axelray-dev commented Sep 19, 2026

Uh oh!

axelray-dev commented Sep 21, 2026

Uh oh!

axelray-dev commented Sep 23, 2026

Uh oh!

KuSh left a comment

Choose a reason for hiding this comment

Round 2 of 3 — APPROVED

Blocks merge (0 — the round-1 item is fixed)

Optional — unchanged from round 1, will not hold merge

Checked and correct — no need to re-verify

Your questions

Next

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants