refactor(lexer): consolidate raw-command lexing behind shared primitives by KuSh · Pull Request #3704 · rtk-ai/rtk

KuSh · 2026-08-25T00:26:51Z

Summary

The lone-CR bug found by /code-review on #3600 appeared in two independent places (registry.rs::rewrite_multiline_block and permissions.rs::command_matches_pattern) because there was no single "parse a raw shell command string" implementation — at least five (tokenize_inner, QuoteScan, shell_split, split_token_spans, permissions.rs's ad hoc split_whitespace()), plus three compound-command segmenters on top. This consolidates layer 1 (raw string → quote/operator/newline-aware words and segments) so such a fix lands once. Layer 2 (core/arg_tokenizer.rs, argv → flags/values) is untouched — that's #3681.

What changed

Root fix: word boundaries follow bash's default $IFS (exactly space/tab/newline) via one shared is_word_boundary_whitespace, not char::is_whitespace() (Unicode White_Space, wrongly includes NBSP) or a per-caller split_whitespace(). A lone \r is no longer a word boundary, nor a command boundary on the bash-accurate path. permissions.rs::command_matches_pattern shares the predicate — it previously collapsed an embedded CR into a space, letting an allow-rule for git status auto-approve git status\rrm -rf ~.
One quote/escape rule: tokenize_inner, QuoteScan and shell_split each tracked "am I inside a quote"; all three now drive advance_quote_state.
One CRLF rule: tokenize_inner's newline guard and registry.rs's raw_breaks parity check share is_crlf_at.
One word-coalescing rule: split_token_spans (golangci-lint flags) and shell_split only need "was there a space here", not full shell-syntax tokens (*.yml is one bash word but Shellism("*") + Arg(".yml")). New coalesce_words() merges gapless adjacent tokens; both functions are now thin wrappers over tokenize() instead of bespoke scanning loops.
Newline policy is explicit, not implicit: tokenize_inner takes a NewlineMode (None/Bash/Conservative). Only split_for_permissions uses Conservative, where a lone \r is a segment boundary — keeping the gate's segmentation identical to develop's. Living inside the shared char loop, it stays quote-aware for free.
Second root fix, same shape as the first: inside double quotes bash only lets \ escape $ ` " \ or a newline — before anything else it is a literal character. shell_split treated \ as an escape everywhere outside single quotes, so a quoted Windows path lost its separators ("C:\Program Files\rtk.exe" → C:Program Filesrtk.exe). Now bash-accurate, which fixes that path for every caller at once (hooks/mod.rs hook detection, rtk proxy argv construction, registry.rs::search_uses_pattern_file) rather than per call site. Unquoted backslashes still escape, as bash does. Found while reviewing feat(hooks): add direct Codex command rewriting #3552.
Documented and pinned the three deliberately-different compound-command segmenters (permission gate / analytics / rewrite): cross-referencing doc comments, a comparison table, a segmenter_consistency test module, and a discover/README.md section — the lexer is consumed by hooks/permissions.rs, hooks/mod.rs and main.rs, so it's shared infrastructure, not a discover/ internal.

Not in scope

core/arg_tokenizer.rs and its call sites — layer 2, arg_tokenizer: shared CLI-arg tokenizer + cross-ecosystem -- boundary/flag fixes #3681.
Unifying QuoteScan onto the token stream: it needs quote state at an arbitrary byte, including inside a token (is the # in foo#bar a comment?), which Vec<ParsedToken> doesn't carry — it would re-scan every token anyway. It already shares the rule that was actually duplicated.
Unifying the three segmenters further: they already consume the same token stream and differ only in boundary policy, which is deliberate (item 7).
hooks/rewrite_cmd.rs vs hooks/hook_cmd.rs decision-flow duplication — real, but a separate follow-up at a different level.

Test plan

cargo fmt --all --check, cargo clippy --all-targets (zero warnings), cargo test --all (2724 tests)
Regression test per behavior: lone-CR word splitting (tokenize, shell_split, command_matches_pattern), NBSP splitting, permission-gate lone-CR segmentation inside and outside quotes, quoted and unquoted golangci-lint flag values, backslash handling in and out of double quotes, the segmenter-consistency table, coalesced-word shell_split

Bash's default $IFS is space/tab/newline, never a bare CR. The lexer's newline arm already conflated a lone \r with \n/\r\n as a command separator, and its generic whitespace arm independently treated \r as a word boundary too — two places that had to independently get this right and didn't. Fixes both: - tokenize_inner's newline arm now only splits on \n or the \r of a real CRLF pair; a lone \r falls through and stays glued to its word. - tokenize_inner's whitespace arm no longer treats \r as a boundary, via a new shared is_word_boundary_whitespace() predicate. - permissions.rs::command_matches_pattern now normalizes through that same shared predicate instead of str::split_whitespace() (which is \r-inclusive), so an allow rule like "git status" can no longer auto-approve "git status\rrm -rf ~" by collapsing the embedded CR into a space before matching. - registry.rs's raw_breaks parity check updated to match the corrected newline model (counts \n and CRLF's \r, never a lone \r). Regression tests added in lexer.rs and permissions.rs reproducing the exact divergence.

tokenize_inner, QuoteScan (registry.rs), and shell_split each independently tracked "am I inside a quote" - QuoteScan's own comment already admitted it was reimplementing "the same model the lexer applies" from scratch. Extract the one rule all three actually encode (an unescaped quote char opens a span if none is open, only the same character closes it) into a single advance_quote_state() primitive, and drive all three from it: - tokenize_inner's Option<char> quote tracking now calls it instead of duplicating the open/close branching inline. - QuoteScan switches its internal (in_single, in_double) bool pair to the same Option<char> model, built on the same primitive - its external (offset, byte, in_single_before, in_double_before) API and all 5 downstream consumers (comment_start, bracket/test-bracket balance, ansi_c_quote_defeats_lexer, quotes_balanced) are unchanged. - shell_split switches from its own bool-pair toggle to the same primitive, as a byproduct simplifying its match arms to one merged case. Behavior-preserving: all 2708 existing tests pass unchanged, including the ~750 lines of lexer.rs quote/escape coverage - a future quote-state correction now only needs to happen once.

split_token_spans was a from-scratch, quote-blind whitespace splitter with a single caller (golangci-lint's global-flag pre-processing). Replace it with the lexer's tokenize(), reusing its already-tracked byte offsets - genuine dedup, and a real fix: a quoted global-flag value containing a space (`--config "a path/x.yml"`) used to get mis-split at the space inside the quotes, which made parse_golangci_run_parts miss the `run` subcommand entirely and fall back to leaving the command unclassified. New regression test covers the quoted-value case; all other existing golangci-lint classification tests pass unchanged.

split_for_permissions (permission gate), split_on_operators / split_command_chain (analytics/discovery classification), and rewrite_compound's inline token walk (actual rewrite) all segment the same kind of compound-command string, but deliberately differently: pipe-stop behavior, whether background `&` or `(`/`)` grouping counts as a boundary, and whether a trailing redirect gets truncated all diverge across the three. That's intentional - the permission gate must stay the most conservative - but it was undocumented and untested as a set, so a future edit to any one of them could silently drift further from the other two without anything catching it. - Cross-referencing doc comments on all three functions, including a comparison table on split_for_permissions. - A new segmenter_consistency test module in registry.rs pinning today's actual, verified output for all three across four representative inputs (background &, subshell grouping, pipe+&&, redirect-in-segment) - including a real quirk this surfaced: in `(git status; cargo build)`, the rewritten output only prefixes "cargo build", not "git status", because the leading `(` glued to the first segment defeats rewrite_segment's own command matching while the trailing `)` on the second segment doesn't. Pinned as documented existing behavior, not changed here. No behavior change; pure documentation and test coverage.

Two follow-ups from /code-review high on this branch: 1. parse_golangci_run_parts's switch to the full shell tokenize() (Phase 3) fixed the intended quoted-value bug but introduced a real regression: tokenize() splits unquoted shell metacharacters (*, ?, `, (, ), {, }, !) into their own tokens even outside quotes, so an unquoted glob value like `--config *.yml` desynced the flag-skip loop and got the whole command misclassified as Unsupported. split_token_spans's actual job - "was there a space here", not full shell syntax - is genuinely different from tokenize()'s, so it should not have been replaced by it. Restored split_token_spans as its own function, now built on the shared advance_quote_state / is_word_boundary_whitespace primitives instead of being quote-blind, which is what actually fixes the original quoted-value bug without the metacharacter regression. New regression test for the unquoted case. 2. Extracted is_crlf_at() and used it in both tokenize_inner's newline-operator guard and registry.rs's raw_breaks parity check - this exact duplication (two independent "is this \r part of a CRLF pair" checks) was flagged as a follow-up during the PR rtk-ai#3600 review that prompted this whole consolidation, and had been reintroduced here without actually being fixed. All 2714 tests pass, clippy clean.

…s on it Even after the previous consolidation, split_token_spans and shell_split each still ran their own scanning loop over the raw command string, just now sharing the same quote/whitespace *rules* with tokenize_inner rather than the same *scan*. The gap between "one lexer token" and "one bash word" is exactly the tokens tokenize() splits apart (e.g. Shellism("*") + Arg(".yml") for an unquoted "*.yml") that sit with no gap between them in the original string - bash sees one word there. Add coalesce_words(cmd, tokens): merges directly-adjacent tokens from tokenize()'s output into single words. Rebuild split_token_spans as a one-line wrapper around tokenize() + coalesce_words, removing its own quote-aware scanning loop entirely - golangci-lint's flag parser now runs zero bespoke scanning code. Existing quoted-value and unquoted-glob golangci-lint regression tests (added when this exact call site regressed once already) pass unchanged. All 2716 tests green, clippy clean.

shell_split's per-char scanning loop duplicated the same quote/escape handling tokenize_inner already does, differing only in two genuinely distinct concerns: it splits on whitespace only (not shell operators), and it resolves quotes/escapes into argv-ready text rather than preserving them. Split that into two composable pieces instead of one bespoke loop: - resolve_word_text(): given one coalesced word's raw text, strips quote characters and resolves backslash escapes - the inverse of what tokenize_inner preserves, built on the same advance_quote_state so it can't drift from the tokenizer's own quote model. - shell_split() becomes tokenize() + coalesce_words() (from the previous commit) + resolve_word_text() per word - no scanning of its own. All 15 existing shell_split tests pass unchanged (verified byte-for- byte before writing this commit). One real, intentional behavior refinement: word boundaries now come from the shared is_word_boundary_whitespace (bash's actual default $IFS - space, tab, newline) instead of shell_split's previous bespoke ' '|'\t'-only check, so an embedded unquoted newline is now correctly treated as a word boundary. Covered by a new explicit test, since this wouldn't have been caught by the existing suite otherwise. Also added a test combining an unquoted glob directly adjacent to a quoted segment (the same token-coalescing case that mattered for split_token_spans, exercised here through shell_split's quote-stripped output). This affects 3 real call sites: hooks/mod.rs::is_claude_hook_command, registry.rs::search_uses_pattern_file, and main.rs's `rtk proxy '...'` argv construction (the highest-risk one - it directly controls what gets exec'd). All covered by the existing suite, all green. 2718 tests pass, clippy clean.

aeppling · 2026-08-26T20:39:06Z

Tested locally against develop. Rewrite output identical on CR/CRLF inputs, suite green. Two regressions and one cleanup before merge.

1. shell_split now splits on NBSP

is_word_boundary_whitespace is Unicode whitespace minus CR, not bash IFS. Old shell_split split on space/tab only. a<NBSP>b was one word on develop, now two, bash keeps it one. Breaks argv for rtk proxy '...', is_claude_hook_command, search_uses_pattern_file on pasted commands. Restrict to space/tab/newline and add a test.

2. Permission gate loosened on lone CR, description says no policy change

On develop git status<CR>rm -rf ~ with deny rm:* returns Deny. On this branch it is one segment and returns Default. Not exploitable, git just errors, but it is a policy change in the gate you document as the most conservative. Fix the description and add a test pinning the deny result.

3. Comments

Doc comments carry review history ("confirmed via /code-review high", "flagged during PR #3600 review", "the way they had before this was extracted"), and coalesce_words has a dozen lines defending an allocation. That belongs in the PR, not the source. Keep the why, drop the rest.

- Restrict is_word_boundary_whitespace to bash's actual $IFS (space/tab/newline) instead of Rust's char::is_whitespace(), which wrongly counts non-IFS Unicode whitespace like NBSP as a word boundary and broke shell_split on pasted commands containing it. - Restore the permission gate's conservative lone-CR segmentation: tokenize_inner gains a split_lone_cr parameter so split_for_permissions can opt into treating a bare \r as a command separator (matching pre-consolidation behavior) while every other caller keeps the bash-accurate "lone CR stays glued to its word" rule. Implemented inside the shared char loop (so it stays quote/escape-aware) rather than a separate raw byte scan. - Trim doc comments that referenced review tooling/history instead of explaining the code's own rationale.

- Restrict is_word_boundary_whitespace to bash's actual $IFS (space/tab/newline) instead of Rust's char::is_whitespace(), which wrongly counts non-IFS Unicode whitespace like NBSP as a word boundary and broke shell_split on pasted commands containing it. - Restore the permission gate's conservative lone-CR segmentation. tokenize_inner takes a NewlineMode (None/Bash/Conservative) instead of a bool, so split_for_permissions can opt into treating a bare \r as a command separator (matching pre-consolidation behavior) while every other caller keeps the bash-accurate "lone CR stays glued to its word" rule. The mode lives inside the shared char loop, so it stays quote/escape-aware and doesn't split inside quoted text. - Trim doc comments down to their essential rationale: dropped references to review tooling/history, and cut multi-paragraph explanations to one or two lines, letting the code carry the rest.

Per review feedback on rtk-ai#3681 (document new shared systems in their module README): lexer.rs's public functions are consumed well outside discover/ (hooks/permissions.rs, hooks/mod.rs, main.rs's rtk proxy), so document them as shared infrastructure rather than leaving them as an internal implementation detail.

KuSh · 2026-08-26T22:30:54Z

Tested locally against develop. Rewrite output identical on CR/CRLF inputs, suite green. Two regressions and one cleanup before merge.

1. shell_split now splits on NBSP

is_word_boundary_whitespace is Unicode whitespace minus CR, not bash IFS. Old shell_split split on space/tab only. a<NBSP>b was one word on develop, now two, bash keeps it one. Breaks argv for rtk proxy '...', is_claude_hook_command, search_uses_pattern_file on pasted commands. Restrict to space/tab/newline and add a test.

2. Permission gate loosened on lone CR, description says no policy change

On develop git status<CR>rm -rf ~ with deny rm:* returns Deny. On this branch it is one segment and returns Default. Not exploitable, git just errors, but it is a policy change in the gate you document as the most conservative. Fix the description and add a test pinning the deny result.

3. Comments

Doc comments carry review history ("confirmed via /code-review high", "flagged during PR #3600 review", "the way they had before this was extracted"), and coalesce_words has a dozen lines defending an allocation. That belongs in the PR, not the source. Keep the why, drop the rest.

Everything has been taken into account. Regarding point 2, I wasn’t sure whether the better approach was to get back to the old behavior or update the documentation to describe the new one. I opted for the latter, but please let me know if you’d prefer to restore the previous behavior.

aeppling · 2026-08-27T09:26:15Z

@KuSh

Your code already restores the previous gate behaviour via NewlineMode::Conservative and the new deny test pins it. That is the right call, just align the PR description and code comments with it.

KuSh · 2026-08-27T22:27:23Z

Your code already restores the previous gate behaviour via NewlineMode::Conservative and the new deny test pins it. That is the right call, just align the PR description and code comments with it.

PR description updated. Didn't found comments that contradict the code

…quotes shell_split resolved `\` as an escape everywhere outside single quotes, so every backslash in a double-quoted Windows path was eaten: `"C:\Program Files\rtk.exe"` came back as `C:Program Filesrtk.exe`. Bash only lets `\` escape `$`, `` ` ``, `"`, `\` or a newline inside double quotes; before anything else it is a literal character. Match that, which fixes quoted Windows paths for every shell_split caller at once (hooks/mod.rs's hook-install detection, rtk proxy's argv construction, registry.rs::search_uses_pattern_file) instead of per call site. Unquoted backslashes still escape, as bash does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

KuSh added 7 commits August 25, 2026 02:08

KuSh force-pushed the fix/lexer-consolidation branch from 6c07d33 to 53e237d Compare August 25, 2026 00:54

KuSh marked this pull request as ready for review August 25, 2026 00:55

KuSh force-pushed the fix/lexer-consolidation branch from 23eb019 to 627e45f Compare August 26, 2026 22:16

KuSh requested a review from aeppling August 26, 2026 22:44

esidorenko-sl mentioned this pull request Aug 27, 2026

fix(core): quote child arguments so MSYS children receive them intact #3728

Open

4 tasks

KuSh mentioned this pull request Aug 28, 2026

feat(hooks): add direct Codex command rewriting #3552

Open

5 tasks

Navigation Menu

Uh oh!

refactor(lexer): consolidate raw-command lexing behind shared primitives - #3704

refactor(lexer): consolidate raw-command lexing behind shared primitives#3704
KuSh wants to merge 10 commits into
rtk-ai:developfrom
KuSh:fix/lexer-consolidation

KuSh commented Aug 25, 2026 •

edited

Loading

Uh oh!

aeppling commented Aug 26, 2026

Uh oh!

KuSh commented Aug 26, 2026 •

edited

Loading

Uh oh!

aeppling commented Aug 27, 2026

Uh oh!

KuSh commented Aug 27, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

KuSh commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Not in scope

Test plan

Uh oh!

aeppling commented Aug 26, 2026

Uh oh!

KuSh commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeppling commented Aug 27, 2026

Uh oh!

KuSh commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KuSh commented Aug 25, 2026 •

edited

Loading

KuSh commented Aug 26, 2026 •

edited

Loading

KuSh commented Aug 27, 2026 •

edited

Loading