Skip to content

arg_tokenizer: shared CLI-arg tokenizer + cross-ecosystem -- boundary/flag fixes - #3681

Open
KuSh wants to merge 67 commits into
rtk-ai:developfrom
KuSh:feat/arg-tokenizer
Open

arg_tokenizer: shared CLI-arg tokenizer + cross-ecosystem -- boundary/flag fixes#3681
KuSh wants to merge 67 commits into
rtk-ai:developfrom
KuSh:feat/arg-tokenizer

Conversation

@KuSh

@KuSh KuSh commented Aug 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Introduces src/core/arg_tokenizer.rs, a shared CLI-argument tokenizer, and migrates git.rs, search.rs (grep/rg), dotnet_cmd.rs, and golangci_cmd.rs off ad hoc string-matching flag/value/---boundary detection — the repeated source of bugs across this codebase (each of those files had accumulated its own one-off -- boundary and flag-vs-value bugs independently).

The tokenizer classifies args into Long/Short/Positional/DashDash tokens, tracks attached vs. linked (separate-token) values, and models two real CLI grammars via Dialect (POSIX-ish for git/rg/golangci-lint, MSBuild-ish for dotnet). Per-tool grammar quirks beyond the base takes_value predicate are opt-in via a TokenizeOptions struct passed to tokenize_with_options, defaulting to the common case so each caller only sets what it needs: takes_separate_value for a Short flag that doesn't always accept a separate-token value (git), and claims_literal_dash_dash for a value-taking flag that claims a literal -- as its value instead of treating it as the end-of-options boundary (grep/rg — confirmed per-tool, not per-flag: git/cargo both reject -- as a pending value regardless of which flag is asking).

Real bugs found and fixed (each with a regression test verified fail-before-fix / pass-after-fix)

git.rs

  • -- boundary awareness added to run_diff, run_show, run_log, run_branch, run_stash show — a pathspec/filename literally named like a flag (--stat, -p, etc.) after -- was previously misread as the real flag.
  • run_diff/run_show now share most of run_log's raw-diff-shape flag list (--name-only, --raw, --stat, etc.) instead of a narrower 3-flag subset — except -p/-u/--patch, which stay on the compact path for diff/show: unlike log (whose default output has no diff content at all), diff/show's default output already is patch text, so passing -p explicitly is redundant with the default rather than a request for an incompatible shape. (An earlier version of this same migration wrongly folded -p/-u/--patch into the shared list too, which meant git diff -p/git show -p <rev> silently bypassed RTK's compaction — caught by /code-review high and fixed before merge.)
  • git stash show -u no longer misread as patch mode: -u means --include-untracked for stash show, unlike git log where -u is a -p synonym — the stash-show detector had copied log's mapping, so -u's real stat-only output was silently rendered as empty instead. Confirmed against real git; caught by reviewer testing, fixed before merge.
  • checkout -b/-B glued short-flag form (-bmy-branch) confirmed against real git and pinned.
  • -u's upstream value now links correctly instead of leaking as an unlinked positional.
  • has_limit_flag no longer misfires on a clustered -n with no captured value.
  • Git's own short-flag clustering rules modeled correctly: -M/-U/-C/-B are attached-value-only (never separate-token, confirmed even git log -U 3 fails against real git), -n/-l accept a separate-token value only when standalone (-n 2 works, -cn 2 fails with "ambiguous argument" against real git) — this needed a dedicated takes_separate_value predicate (git.rs's log_takes_separate_value, passed via TokenizeOptions) since it's a git-specific parser quirk, not a POSIX/GNU universal (confirmed grep -im 2 does accept it clustered).

dotnet_cmd.rs

  • MSBuild /-prefixed multi-segment paths (/abs/path/Project.csproj) no longer misclassified as a flag (confirmed via a real dotnet 9 SDK).
  • detect_test_runner_mode no longer scans flag values or post--- filter expressions as candidate project paths.
  • dotnet_takes_value expanded with --filter, -c/--configuration, -f/--framework, -r/--runtime, -a/--arch, --os (confirmed via a real dotnet 9 SDK).
  • has_nologo_arg no longer treats a broken attached-value spelling (-nologo:true) as "already present".
  • dotnet_has_flag/dotnet_has_loose_flag naming swapped so the safe, strict lookup gets the short name (the loose one previously invited exactly the kind of misuse a prior commit in this branch had to fix).

search.rs (grep/rg)

  • -L, -z, -T, and -r all made engine-aware — each means something different for grep vs. rg (confirmed via real grep --help/rg --help and direct runs); e.g. rg's -r is --replace <value> while grep's is --recursive (boolean).
  • has_format_flag computed in extract_pattern_path's own single token pass instead of re-tokenizing a second time.
  • has_short_flag's reconstructed-string ambiguity fixed: extract_pattern_path now computes show_file/show_line/context detection (a new DetectedFlags) directly from tokens in that same single pass, instead of re-scanning a reconstructed args string that could misread a value-taking flag's own value as an unrelated short flag.
  • grep -e --/rg -e -- now match real grep/rg: any value-taking flag in these tools claims a literal -- as its value (confirmed for both engines, short and long forms — -A/-m/-e/--context/--file), instead of treating it as the end-of-options boundary. rtk grep -e -- f previously searched /dev/null for pattern "f"; now matches real grep's "a -- b". Caught by reviewer testing, fixed before merge.
  • reads_piped_stdin now checks for an actual pipe/FIFO instead of merely !is_terminal(). A non-pipe stdin redirect (e.g. < /dev/null) with no explicit path was misread as "the engine reads stdin", routing into the streaming path, which can't discover "multiple files matched" the way the buffered path can — silently dropping filenames from every line. Fixes the reported rg -z foo < /dev/null case and the same pre-existing bug for plain rg/grep (not -z-specific — -z just used to dodge this path via an accidental format-flag passthrough shortcut this PR itself removed). Caught by reviewer testing, fixed before merge.

golangci_cmd.rs

  • restore_double_dash gap fixed (missing entirely).
  • has_output_flag correctly scoped to run-level flags via a dedicated golangci_run_takes_value (verified against real golangci-lint 2.13.1 via Docker), including the --out-format legacy flag it was initially missing.

Documentation

  • src/core/README.md now documents arg_tokenizer.rs and the design rule this branch's grammar-sharing bugs kept surfacing: one grammar per subcommand, never reuse a sibling's predicate wholesale (e.g. -u means -p in git log but --include-untracked in git stash show).

Known, documented limitations (deliberately not fixed — each has a concrete reason in its own doc comment)

  • MSBuild single-segment absolute paths (/app, /tmp) are structurally indistinguishable from a switch name — confirmed real MSBuild itself needs a filesystem stat() call to resolve this, which this pure-function tokenizer deliberately doesn't do.
  • dotnet_cmd.rs's detect_test_runner_mode (explicit_projects) has no filesystem backstop — it relies entirely on dotnet_takes_value's allowlist staying exhaustive for every dotnet flag that takes a separate-token value; a real value-taking flag missing from it whose value happens to end in .csproj/.fsproj/.vbproj would be misread as an explicit project path.
  • A few remaining cross-file duplications (log_takes_value/log_takes_separate_value, dotnet_takes_value's subcommand-unawareness, golangci's double-tokenize between find_subcommand_index/has_output_flag) were reviewed and left as-is — each either has no confirmed failure scenario, or a proper fix requires a larger restructuring disproportionate to the actual risk.

Explicitly out of scope

src/cmds/go/go_cmd.rs, src/cmds/rust/cargo_cmd.rs, src/discover/registry.rs, and src/cmds/system/ls.rs share the same bug class this PR fixes elsewhere but are untouched by this diff — pre-existing, not a regression from this branch, best handled as separate follow-up work. Along with the documented limitations above, these are tracked as planned follow-up work once this PR merges.

Test plan

  • cargo fmt --all --check, cargo clippy --all-targets (zero warnings), cargo test --all (2793 tests) all pass
  • Every fix has a dedicated regression test, verified to fail before the fix and pass after
  • Many rounds of /code-review high (multi-agent adversarial review) run against this branch, iterating fix → re-review until convergence, including targeted re-reviews after later fixes were pushed; remaining findings are the documented limitations above or lower-priority cleanup with no concrete failure scenario
  • A maintainer review round found 3 additional regressions (git stash show -u, grep -e --, rg -z filename-dropping) plus doc/comment-density feedback — all fixed, see above
  • Several fixes verified empirically against real tools (git 2.51, dotnet 9 SDK via Docker, golangci-lint 2.13.1 via Docker, GNU grep, ripgrep) rather than assumed

@KuSh
KuSh force-pushed the feat/arg-tokenizer branch 2 times, most recently from 15efaf3 to ed26781 Compare August 23, 2026 23:20
@KuSh KuSh mentioned this pull request Aug 25, 2026
Open
3 tasks
aeppling
aeppling requested changes Aug 26, 2026
View reviewed changes
Comment thread src/cmds/git/git.rs Outdated
@aeppling

aeppling commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Hey @KuSh

Thanks for working on this which is a pain for commands rewirtting.
Some regressions found:

repo with a stash that includes an untracked file:

rtk git stash show -u
develop: f.txt 1 + / u.txt 1 + / 2 changed 2 + exit 0
PR: (empty) exit 0
git: 2-file stat

file f containing a -- b:

rtk grep -e -- f </dev/null
develop / grep: a -- b exit 0
PR: (empty) exit 1

directory with 4 files containing foo:

rtk rg -z foo </dev/null
develop / rg: f:foo one … (filenames)
PR: foo one … (filenames dropped)

aeppling
aeppling reviewed Aug 26, 2026
View reviewed changes
Comment thread src/core/arg_tokenizer.rs
aeppling
aeppling reviewed Aug 26, 2026
View reviewed changes
Comment thread src/cmds/dotnet/dotnet_cmd.rs Outdated
@aeppling

aeppling commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Also we may want to document this system in respective README.md

KuSh and others added 22 commits August 26, 2026 22:14
@KuSh @claude
refactor(core): add shared arg_tokenizer, migrate git.rs onto it
71d48db 
Every src/cmds/** filter that needs to know "does this flag consume the
next token as its value, and where does -- end options" was reimplementing
that question independently. git.rs alone racked up 9 one-off bugfix
commits on its own LogArg/consumes_next_token_as_value/log_arg_tokens
(hardcoded matches! lists edited piecemeal, -- handling gaps), and a
codebase survey found the same shape of bug already hit or latent in
search.rs, golangci_cmd.rs, and dotnet_cmd.rs.

Adds src/core/arg_tokenizer.rs: a single tokenize() that classifies an
already-restore_double_dash'd args slice into Long/Short/Positional/
DashDash tokens, linking each value token to the flag that consumes it
and vice versa, with a caller-supplied `takes_value(kind, name)`
predicate (each wrapped CLI's value-flag list stays exactly as
data — only the token-walking around it is now shared). Digit-only
short suffixes (git log/head/tail's `-N` shorthand) are kept as one
token rather than decomposed into per-digit "flags", since no real CLI
defines boolean digit flags.

Migrates git.rs's run_log (LogArg/log_arg_tokens/consumes_next_token_as_value
-> log_takes_value + tokenize) and run_checkout's four separate hand-rolled
scanners (checkout_new_branch_arg/checkout_reset_branch_arg/
checkout_branch_arg/checkout_restored_count) onto one shared tokenization
each, keeping their existing flag lists as the predicate body. No
intended behavior change; arg_tokenizer's own unit tests encode a
regression case for every prior git.rs bugfix commit, and git.rs's
existing test suite (updated only where it asserted internal token
shape, e.g. dash-free flag text) plus the real-process integration
tests in tests/guard_integration_test.rs still pass unchanged.

Follow-up (not in this commit): migrate search.rs, dotnet_cmd.rs (which
has a latent restore_double_dash gap of its own), and golangci_cmd.rs
onto the same tokenizer.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(search): migrate grep/rg arg parsing onto arg_tokenizer
cdbc493 
search.rs's VALUE_FLAGS_SHORT/VALUE_FLAGS_LONG/ClusterResult/parse_cluster
reimplemented the same flag/value/-- classification arg_tokenizer now
centralizes. Migrates extract_pattern_path onto arg_tokenizer::tokenize,
keeping the flag lists as data (now dash-free, matching Token::text) and
the -e/--regexp-routes-to-patterns special case as extract_pattern_path's
own business logic (not something tokenize needs to know about).

Reconstructing the exact flags: Vec<String> shape (grep/rg-facing, not
just internal) needed one thing tokenize()'s per-character Short model
didn't originally have: knowing whether "-r" and "-n" were typed as one
cluster ("-rn", reconstructed glued) or two separate args ("-r" "-n"),
since existing tests pin the glued form for one-arg clusters. Added
Token::source_index (which original args slot a token came from) to
close that gap generically -- every Short token from one cluster shares
a source_index, a consumed separate-token value always has its own.
Also added Token::value() (attached-or-linked) as a small shared
convenience, used here and to simplify git.rs's now-redundant
linked_value/flag_value helpers.

Note: this is a reconstruction-format detail, not a behavior change --
grep/rg parse "-rn" and "-r -n" identically via standard getopt
clustering, and rtk's own has_short_flag() already checks via substring
so it doesn't care which form it sees either.

No intended behavior change. parse_cluster/ClusterResult's own unit
tests are removed (that internal API no longer exists); the existing
extract_pattern_path tests already exercise the same short-cluster/
value-taking/-e behavior end-to-end and all still pass unchanged.

Follow-up (not in this commit): golangci_cmd.rs, and dotnet_cmd.rs
(which has a latent restore_double_dash gap of its own).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(dotnet): restore -- before build/test/restore/format arg handling
392ea99 
dotnet_cmd.rs never called args_utils::restore_double_dash despite
Build/Test/Restore/Format all using trailing_var_arg = true in
main.rs, the same clap-strips-`--` hazard git.rs/cargo_cmd.rs/search.rs
already hit and fixed (issue rtk-ai#1215). Concretely: `rtk dotnet test --
FullyQualifiedName=MyFilter` lost its `--`, so
inject_report_trx_into_args (VsTestBridge MTP mode) couldn't find one
to reuse and appended a fresh `-- --report-trx` at the end instead —
landing the user's MTP-runtime filter expression BEFORE the separator
(misread as a dotnet-test-level arg) instead of after it, and stranding
--report-trx with nothing following it.

Added the missing restore_double_dash call to both entry points that
feed `args` into `--`-sensitive logic (run_dotnet_with_binlog for
build/test/restore, run_format). Regression test stubs `dotnet` on
PATH (no dotnet SDK in this environment) to capture the real argv rtk
would hand it; verified it fails without the fix and passes with it.

Not migrated onto arg_tokenizer: investigated during this pass and
dotnet's CLI grammar isn't POSIX/GNU-style — has_nologo_arg accepts
single-dash multi-letter flags (`-nologo`, `/nologo`) as one atomic
name rather than a short-flag cluster, and --logger:trx uses `:` as
an attach separator, both incompatible with arg_tokenizer's current
clustering model. A caller-selectable dialect (no clustering, `/`
prefix, `:`-or-`=` attach) could support this later; not attempted
here to keep this fix minimal and low-risk.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(arg_tokenizer): add Msbuild dialect, migrate dotnet_cmd.rs f…
fac0fbb 
…lag checks

dotnet's CLI grammar isn't POSIX/GNU: `-flag`, `--flag`, and `/flag` are
all one atomic flag name (no short-flag clustering, e.g. `-nologo` must
stay one token, not decompose into per-char short flags), and a value
can attach via `:` as well as `=` (`--logger:trx`). Added Dialect::{Posix,
Msbuild} to arg_tokenizer, threaded through a new tokenize_dialect();
tokenize() itself is now a thin Dialect::Posix wrapper, so git.rs/search.rs
are untouched.

Migrated dotnet_cmd.rs's flag/value helpers (has_nologo_arg,
has_trx_logger_arg, has_results_directory_arg, has_report_arg,
has_report_trx_arg, extract_report_arg, has_verify_no_changes_arg,
has_write_mode_override, extract_results_directory_arg) onto
tokenize_dialect(Msbuild), replacing ~9 hand-rolled peekable/prefix
scans with two small shared helpers (dotnet_has_flag/dotnet_flag_value).

Two things the migration had to account for, not just carry over:
- dotnet_cmd.rs's checks were never `--`-boundary aware (--report-trx is
  meaningful on either side of `--` depending on TestRunnerMode, and the
  original code just flat-scanned the whole arg list) -- unlike git/grep,
  where `--` genuinely ends option parsing. tokenize_dialect always stops
  classifying at `--` (correct for git.rs/search.rs), so dotnet_cmd.rs's
  new with_dotnet_tokens() strips `--` out before tokenizing to reproduce
  the original scan-everything behavior. Caught by
  test_vstest_bridge_respects_existing_report_trx (would otherwise
  double-inject --report-trx when the user already placed it after --).
- has_nologo_arg previously matched only "-nologo"/"/nologo" (not
  "--nologo"), and has_results_directory_arg/has_report_arg/
  has_verify_no_changes_arg/has_write_mode_override previously matched
  only the "--" form -- both incidental gaps from scanning raw strings
  per-flag rather than a unified prefix model. Since dotnet's actual
  System.CommandLine parser treats -/--/ / as interchangeable prefixes
  for every option, recognizing all three forms uniformly for every flag
  here is a correctness improvement, not a behavior risk.

inject_report_trx_into_args is untouched: it's pure `--`-relative Vec
splicing, not flag/value classification, so it doesn't need tokenizing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(arg_tokenizer): Msbuild's -- is a forwarding boundary, not end-of…
71c06a6 
…-options

Discovered this refining the previous commit: dotnet's `--` doesn't mean
what git/POSIX `--` means. Git's `--` ends option parsing -- everything
after is a literal positional/pathspec, never a flag again. dotnet's `--`
is an argument-*forwarding* boundary: what follows is still real flags,
just meant for a different receiving parser (the VSTest/MTP test host),
which can and does share flag names with dotnet test's own CLI (e.g.
--logger, --results-directory are forwarded VSTest-console options, and
--report-trx is meaningful on either side of `--` depending on
TestRunnerMode::MtpVsTestBridge).

The previous commit's dotnet_cmd.rs fix (with_dotnet_tokens stripping
`--` out before tokenizing) reproduced the right observable behavior but
for the wrong reason -- it papered over the mismatch in the caller
instead of naming it. Moved the fix to where it actually belongs:
tokenize_dialect now only stops classifying at `--` for Dialect::Posix;
Dialect::Msbuild still emits a DashDash token (so callers can find `--`'s
position) but keeps classifying flags normally past it. This also let
dotnet_cmd.rs drop the owned-copy-plus-closure workaround entirely --
dotnet_tokens() is back to a plain function borrowing straight from the
caller's args, same shape as every other tokenizer call site.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(go): migrate golangci_cmd.rs subcommand detection onto arg_t…
c433bb1 
…okenizer

find_subcommand_index/split_flag_name/golangci_flag_takes_separate_value
hand-rolled the same flag/value classification arg_tokenizer now
centralizes (GLOBAL_FLAGS_WITH_VALUE was its own one-off "which global
flags take a separate value" list). Replaced with a single
arg_tokenizer::tokenize(Dialect::Posix) call plus a small predicate
(golangci_takes_value); find_subcommand_index now just walks the
resulting tokens for the first free positional, using Token::source_index
to translate back to the position in the original `args` for the caller
(classify_invocation slices global_args/run_args off that index).

No intended behavior change: all existing classify_invocation tests
pass unchanged. Added two regression cases the prior hand-rolled scanner
didn't have tests for: `-c value` (short flag consuming a separate-token
value, previously untested) and `--` before any subcommand is found
(must fall through to Passthrough, matching golangci-lint's own parser
taking over from there).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
style: alphabetize match arms/enums/lists, prefer _ over spelled-out …
29c6d91 
…catch-alls

Consistency sweep across everything touched in this arg_tokenizer work:

- `TokenKind::X | TokenKind::Y => false` catch-alls (naming exactly the
  two variants not otherwise handled) become `_ => false`/`_ => {}`.
- match arms on TokenKind reordered alphabetically (Long before Short,
  etc.), matching the convention already used by most of them.
- matches!() flag lists reordered alphabetically where they weren't
  (dotnet_takes_value's report/results-directory, run_log's format-flag
  and wants_merges checks, parse_limit_from_tokens' arm order).
- search.rs's VALUE_FLAGS_SHORT/VALUE_FLAGS_LONG and golangci_cmd.rs's
  GOLANGCI_SUBCOMMANDS (&[&str] consts checked via .contains()) converted
  to matches!() functions (is_short_value_flag/is_long_value_flag/
  is_golangci_subcommand), consistent with every other flag-list
  predicate in this codebase.
- TokenKind and Dialect enum variants reordered alphabetically at their
  declaration site too (DashDash/Long/Positional/Short,
  Msbuild/Posix).

No behavior change; cargo fmt/clippy/test --all clean throughout.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(arg_tokenizer): make case-insensitive flag lookup a Dialect …
ab4d8ed 
…property

dotnet_cmd.rs's dotnet_flag_value/dotnet_has_flag each hardcoded their
own .eq_ignore_ascii_case() call. Case-insensitive flag names aren't a
dotnet-CLI particularity though -- it's a broader Windows/MSBuild-ecosystem
convention (classic MSBuild.exe's /nologo and /NoLogo are equally valid,
not just dotnet's System.CommandLine parser), so it belongs on Dialect
itself, not left to each caller to remember.

Token::text can't be case-folded once at tokenize time without giving up
the zero-copy &'a str contract (there's no borrowed "lowercased" view of
a str), so the fix lives in new shared lookup helpers instead:
flag_value(tokens, dialect, name) / has_flag(tokens, dialect, name),
which pick exact vs ASCII-case-insensitive comparison from the dialect
via a small flag_name_matches() (Long tokens only -- Msbuild never
produces Short, and conflating Long/Short by text alone would be wrong
for Posix). dotnet_cmd.rs's local helpers now just forward to these.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(git): stop tokenizing run_log's args twice
d94d3f4 
run_log's own comment said "tokenize once and share it," but
requests_raw_log_output(args) tokenized internally to decide on raw
passthrough, and when it returned false the code tokenized args again
right after for the shared `tokens` variable -- silently contradicting
its own stated intent. Found by /code-review high.

run_log now tokenizes once up front and checks
tokens.iter().any(requests_raw_diff_shape) directly for the raw-passthrough
decision, reusing the same tokens for everything else.
requests_raw_log_output stays as a #[cfg(test)] convenience wrapper (its
existing unit tests still call it directly with a &[String]) but is no
longer part of the real run_log path.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix: address second /code-review high pass (4 verified findings)
a8e27b9 
- dotnet_cmd.rs: has_trx_logger_arg only checked the first --logger
  occurrence via flag_value's .find(). dotnet test's --logger can
  legitimately repeat (`--logger "console;verbosity=normal" --logger
  trx`, a documented VSTest pattern); a trx logger that isn't the first
  occurrence was silently missed, causing a duplicate --logger trx
  injection. Added arg_tokenizer::flag_values (every occurrence, not
  just the first) and switched has_trx_logger_arg to it. Verified the
  new multi-logger regression test fails without the fix.

- arg_tokenizer.rs: TokenKind::DashDash's doc promised "emitted exactly
  once," but under Dialect::Msbuild seen_dash_dash was never set, so
  every literal `--` produced its own DashDash token (Posix already got
  this for free via its seen_dash_dash-gated positional catch-all).
  Added a separate emitted_dash_dash flag so only the first `--` becomes
  DashDash in either dialect; later ones fall back to plain Positional
  text "--". Latent today (no dotnet_cmd.rs caller reads DashDash
  position yet) but was a real violation of the module's own invariant.

- golangci_cmd.rs: find_subcommand_index stopped scanning (returning
  None/Passthrough) the moment it hit a bare "-" token, since
  arg_tokenizer classifies a lone "-" as Positional. The old hand-rolled
  scanner treated any "-"-prefixed token, including bare "-", as
  skippable/unrecognized and kept scanning for the real subcommand.
  Excluded "-" specifically from the stopping condition. Verified the
  new regression test fails without the fix.

- search.rs: extract_pattern_path cloned every arg into a fresh
  Vec<String> to satisfy arg_tokenizer::tokenize's &[String] signature,
  even though its one production caller already owns a Vec<String> from
  restore_double_dash. Split into extract_pattern_path_owned(&[String])
  (used by the real call site, zero-copy) and kept the generic
  T: AsRef<str> wrapper as #[cfg(test)]-only for the ~29 existing test
  call sites that pass &[&str] literals.

Also assessed and intentionally left unfixed (see conversation for
detail): run_show/run_diff's own --stat-style checks have the same
`--`-boundary gap run_log had, but touching them is new scope beyond
this session's migrated code, not a regression this diff introduced;
dotnet_cmd.rs's flag-prefix widening (-/--// treated as interchangeable
for every dotnet_cmd.rs flag) rests on an unverified assumption about
dotnet's System.CommandLine parser I can't confirm without a real SDK;
a few remaining hand-rolled string scans (has_binlog_arg,
has_verbosity_arg, has_output_flag) are consistency nitpicks, not bugs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(arg_tokenizer): genericize over AsRef<str>, simplify search.rs
54600f8 
tokenize/tokenize_dialect were hardcoded to &[String], forcing search.rs's
extract_pattern_path to clone every arg into a fresh Vec<String> just to
satisfy that signature even for its one production caller, which already
owns a Vec<String> from restore_double_dash (worked around last commit
by splitting into a test-only generic wrapper + an _owned zero-copy
variant). Genericizing tokenize/tokenize_dialect/push_atomic_flag over
T: AsRef<str> removes the need for that split entirely: String and &str
both satisfy the bound, so every existing call site (all of which pass
&[String]) needed zero changes, and search.rs collapses back to one
extract_pattern_path<T: AsRef<str>> used directly by both its production
call site and its ~29 &[&str]-literal tests, with no cloning either way.

Not extended to OsStr/OsString: unlike str, OsStr exposes almost no
string-manipulation API by design (no strip_prefix, split_once,
char-boundary slicing), so tokenizing it would mean re-deriving that
machinery byte-by-byte the way clap_lex does internally -- a much bigger
change for a case rtk doesn't hit today (its own CLI parsing already
assumes UTF-8 args for every subcommand this module serves).

Also documents why tokenize() doesn't just call restore_double_dash (or
std::env::args()) internally: restore_double_dash's result has to be an
owned Vec<String> the caller holds in its own `let`, and Token<'a>
borrows straight from `args` -- tokenizing a Vec<String> built inside
this module would tie every Token to a value dropped when the function
returns. Same root cause as why this module doesn't build on clap_lex.

Also: dotnet_cmd.rs's has_trx_logger_arg no longer needs an intermediate
`let has_trx = ...; has_trx` binding to dodge a tail-position temporary
lifetime issue -- an explicit `return` avoids it more simply. And added
a regression test + doc trail for the earlier -5x malformed-input
question: verified against real git (`git log -5x` fails with "fatal:
'5x': not an integer", exit 128) that run_log's internal limit
computation for this input is never observable either way, since
run_log bails out on !result.success() before the formatting code that
would use it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(dotnet): tokenize args once per invocation, not per helper call
bb105aa 
dotnet_tokens(args) re-tokenized from scratch inside every has_*/extract_*
helper (has_nologo_arg, has_trx_logger_arg, has_results_directory_arg,
has_report_arg, has_report_trx_arg, extract_report_arg,
has_verify_no_changes_arg, has_write_mode_override,
extract_results_directory_arg, has_binlog_arg, has_verbosity_arg) --
unlike git.rs's run_log, which already tokenizes once and shares the
result. A single `dotnet test`/`dotnet format` invocation could
re-tokenize the same args up to 5+ times across run_format's own checks
plus build_effective_dotnet_format_args, or run_dotnet_with_binlog's
should_expect_binlog plus resolve_trx_results_dir plus
build_effective_dotnet_args's several checks.

Every one of these helpers now takes `tokens: &[Token<'_>]` directly
instead of `args: &[String]`, with the tokenization itself hoisted to a
single `dotnet_tokens(args)` call at the top of run_format and
run_dotnet_with_binlog, threaded down through resolve_trx_results_dir,
resolve_format_report_path, build_effective_dotnet_args, and
build_effective_dotnet_format_args. `args` itself stays a parameter
alongside `tokens` where still needed for non-classification purposes
(raw forwarding to the dotnet subprocess, detect_test_runner_mode's
filename-extension scan, inject_report_trx_into_args's positional
Vec-splice).

Also migrated has_binlog_arg/has_verbosity_arg (previously hand-rolled
starts_with string scans) onto the tokenizer as part of this pass, since
they were two of the remaining "cleanup opportunity" holdouts sitting a
few lines above the already-migrated helpers.

No intended behavior change; full suite (2731 tests, including the
dotnet_double_dash_test.rs real-process regression test) passes
unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(go): give has_output_flag -- boundary awareness
9972d04 
has_output_flag was the last hand-rolled string scan left in
golangci_cmd.rs (find_subcommand_index/golangci_takes_value were already
migrated), and had no `--` awareness: `golangci-lint run -- --out-format`
(a package path literally named "--out-format" passed to the linted
target after the separator) would be misdetected as the user requesting
a custom output format, silently skipping RTK's own default
--output.json.path/--out-format=json injection.

Migrated onto arg_tokenizer::tokenize + has_flag, consistent with the
rest of the file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(dotnet): most dotnet_cmd.rs flags are double-dash-only, not -/--/…
bb9c5d0 
…/ interchangeable

Verified against a real dotnet 9 SDK (Docker, mcr.microsoft.com/dotnet/sdk:9.0)
that the earlier claim behind the Msbuild dialect's uniform prefix widening
("dotnet's actual System.CommandLine parser treats -/--// as interchangeable
prefixes for every option") was wrong. Only a handful of options are genuine
legacy MSBuild.exe passthrough switches valid via all three prefixes
(-nologo/--nologo/nologo, -bl, -v/-verbosity) -- confirmed working. Every
other dotnet_cmd.rs flag (--verify-no-changes, --report for `dotnet format`;
--logger, --results-directory for `dotnet test`) is double-dash-only, and a
single-dash or slash spelling doesn't just get rejected -- it gets
*misparsed* as an unrelated MSBuild switch:

  $ dotnet format -verify-no-changes
  Argument 'erify-no-changes' not recognized. Must be one of: 'q' 'quiet' ...
  (parsed as -v with attached value "erify-no-changes")

  $ dotnet test -results-directory /tmp/out
  MSBUILD : error MSB1001: Unknown switch.

  $ dotnet test -logger trx
  MSBUILD : error MSB1007: Specify a logger.
  (collides with MSBuild's own -logger switch, which wants an assembly spec)

  $ dotnet format /verify-no-changes
  Unhandled exception: FileNotFoundException: 'verify-no-changes' does not
  appear to be a valid project or solution file.
  (positional argument fallback, same as -write / --write's actual behavior)

Added Token::double_dash (true only for a literal `--` prefix; -flag/­/flag
under Dialect::Msbuild are false) plus has_double_dash_flag/
double_dash_flag_value/double_dash_flag_values, and switched
has_trx_logger_arg/has_results_directory_arg/has_report_arg/
has_report_trx_arg/extract_report_arg/has_verify_no_changes_arg/
extract_results_directory_arg/has_write_mode_override to the strict variants
-- has_nologo_arg/has_binlog_arg/has_verbosity_arg keep the loose has_flag,
since those three are the actual legacy-switch exceptions. --write is RTK's
own pseudo-flag (stripped before forwarding to real dotnet, never validated
by it) but kept double-dash-only too for consistency with its dotnet-format
siblings rather than inventing a laxer convention with no real reference
behavior to match.

Removed the now-unused loose flag_value/flag_values (nothing needs
loose-prefix *value* extraction, only loose presence checks for the three
legacy switches) and dotnet_cmd.rs's now-dead dotnet_flag_value wrapper.

Regression test (test_double_dash_only_flags_reject_single_dash_and_slash_spellings)
verified to fail against the prior (loose) implementation before restoring
the fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(arg_tokenizer): never let a value-taking flag swallow the boundar…
90aa5fc 
…y --

Critical finding from the third /code-review high pass, independently
verified against real git before fixing: both push_atomic_flag
(Long/Msbuild-atomic flags) and the Short-cluster value-consumption branch
consumed the next raw arg as a value-taking flag's value via args.get(...)
unconditionally, with no check for the literal "--" separator. So
`rtk git log --grep -- pattern` or `rtk git checkout -b -- file.txt`
silently swallowed "--" as the flag's value and never emitted a DashDash
token at all -- breaking anything downstream that depends on finding that
boundary (e.g. git.rs's checkout_restored_count/checkout_branch_arg).

Verified against real git 2.51 that this exact swallowing is wrong:

  $ git log --grep -- fix
  fatal: Option '--grep' requires a value
  (git refuses to let --grep claim "--", even though --grep needs a value)

  $ git log --grep -- -- foo
  fatal: Option '--grep' requires a value
  (still refuses -- only the FIRST/boundary "--" is protected)

  $ git log -- -- fix
  (succeeds -- once past the boundary, a later "--" is ordinary text and
  fair game as a value)

So the fix is precise, not a blanket "never consume --": guard value
consumption on the tokenizer's own emitted_dash_dash state (already
tracked for the Msbuild-forwarding-boundary fix) -- refuse to link the
still-unseen boundary "--" as a value, but still allow a later "--" once
the boundary was already emitted. Threaded emitted_dash_dash into
push_atomic_flag as a parameter; the short-cluster branch already had it
in scope.

Also extracted the shared link_next_value() helper both consumption
sites now call, addressing a related cleanup finding from the same
review: the duplicated consume-next-token-as-value logic is exactly
what let the original bug exist in two places at once instead of one.

Three new regression tests (verified to fail against the prior
implementation before restoring the fix): a value-taking Long flag
before --, a value-taking Short flag before --, and a value-taking flag
after the boundary was already emitted (which must still consume normally).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix: address lower-severity findings from third /code-review high pass
c5c46db 
- dotnet_cmd.rs: has_write_mode_override matched an attached-value
  spelling ("--write=true") as a force-write override via
  dotnet_has_double_dash_flag (which only checks the flag name, not
  whether it has a value), but build_effective_dotnet_format_args's
  strip filter only ever removed the exact bare "--write" token. So RTK
  would skip its own --verify-no-changes injection *and* still forward
  the unrecognized "--write=true" to real dotnet, which has no --write
  option at all (confirmed via Docker) and would reject it. --write was
  never meant to take a value, so detection now requires attached ==
  None too -- an attached-value spelling is simply not recognized as
  the --write pseudo-flag, matching how any other unrecognized argument
  passes through untouched (consistent with, not silently swallowed by,
  the strip filter).

- golangci_cmd.rs: run_filtered called build_filtered_args (which
  tokenizes invocation.run_args) twice -- once to build the real
  command, once again just to render the verbose debug log line.
  Compute it once and reuse it for both.

Investigated but found NOT to be real bugs (verified against a real
dotnet 9 SDK via Docker before concluding, rather than assuming):
- arg_tokenizer.rs's split_attached splitting on both ':' and '=' for
  every Dialect::Msbuild Long token, including double-dash-only options
  like --results-directory/--report: confirmed `--results-directory:val`
  and `--report:val` both work correctly against real dotnet test/format,
  so the existing behavior is correct, not over-permissive.

Not changed, judged intentional/established rather than problems:
dotnet_cmd.rs's per-flag wrapper functions (has_nologo_arg etc.) --
named predicates over hardcoded string literals, matching the
pre-existing convention from before this session's migration; Token::
double_dash being meaningful only under Dialect::Msbuild -- normal for
a field on a struct shared across dialects with different needs;
git.rs's #[cfg(test)]-only wrappers (real_flag_args, parse_user_limit,
requests_raw_log_output) -- a deliberate tradeoff from an earlier commit
in this branch to keep production on the single-tokenization path
without rewriting ~15 existing test call sites.

Also noted but out of scope for this branch (per user instruction):
run_show/run_diff's own `--`-boundary gap, already tracked as a known
follow-up since commit 7049983.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(golangci): restore -- boundary before classifying invocation args
3577391 
run() never called restore_double_dash despite clap's trailing_var_arg
stripping the literal "--", so both classify_invocation's -- detection
and the raw args forwarded to the real binary were missing the user's
own --.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(git): restore -- boundary in run_add and migrate run_show to arg_…
1386faf 
…tokenizer

run_add never called restore_double_dash, so `rtk git add -- -weird-filename`
lost its -- before reaching git, which rejected the flag-like filename
instead of staging it.

run_show's wants_stat_only/wants_format checks were raw, --boundary-unaware
string scans (arg == "--stat", arg.starts_with("--pretty")) -- a pathspec
literally named "--stat" or "--pretty" after -- was misdetected as the real
flag. Migrated onto arg_tokenizer (sharing git log's value-taking-flag
predicate) with restore_double_dash up front, matching the fix already
applied to run_log/run_diff/run_checkout. wants_blob_show/is_blob_show_arg
is left as-is: its own dash-prefix guard already makes it boundary-safe.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
test(git): add real-process regression for run_show's -- boundary fix
973ea3e 
Mirrors the existing git_log_dash_p_pathspec_after_double_dash test:
stages a file literally named "--stat" past the -- separator and asserts
`rtk git show -- --stat` stays on RTK's compacted-diff path instead of
being misdetected as the --stat summary flag.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(dotnet): has_nologo_arg must not match an attached-value spelling
7cf5409 
-nologo is a pure boolean MSBuild.exe switch (confirmed via a real
dotnet 9 SDK, Docker: "-nologo:true" fails with "MSB1002: This switch
does not take any parameters"). has_nologo_arg used the loose
dotnet_has_flag lookup, which ignores Token::attached entirely, so it
treated the broken "-nologo:true" spelling as "user already passed
-nologo" -- RTK skipped its own -nologo injection while still
forwarding the invalid flag straight through to real dotnet.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(git): restore -- boundary and migrate run_branch to arg_tokenizer
c856d83 
run_branch never called restore_double_dash despite git branch using
trailing_var_arg, and has_action_flag/has_list_flag/has_positional_arg
were raw, --boundary-unaware string scans (has_positional_arg in
particular: `!a.starts_with('-')`). A branch name starting with '-'
after -- was misclassified as a flag rather than the positional name to
create, so rtk silently ran a harmless empty `git branch -a` list
instead of attempting the creation.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(dotnet): reuse the already-tokenized -- position in inject_r…
a30df3c 
…eport_trx_into_args

inject_report_trx_into_args hand-rolled its own args.iter().position(|a|
a == "--") scan instead of reusing the DashDash token already computed by
dotnet_tokens for this same invocation. Pure refactor, no behavior change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
KuSh and others added 27 commits August 26, 2026 22:14
@KuSh @claude
fix(search): make -T value-taking detection engine-aware
ef810ef 
is_short_value_flag treated -T as value-taking for both grep and rg,
but confirmed via real grep --help/rg --help that -T means unrelated
things per engine: GNU grep's -T is --initial-tab (boolean, no value),
ripgrep's -T is --type-not (takes a value). `rtk grep -T pattern
file.txt` consumed "pattern" as -T's (nonexistent) value, leaving
"file.txt" misread as the search pattern and dropping the real pattern
entirely. Threaded Engine through is_short_value_flag/search_takes_value,
matching the pattern already used for is_format_flag_token's -L/-z fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(git): has_limit_flag must require -n to have actually captured a …
3875b50 
…value

has_limit_flag matched any Short "n" token by text alone, even one
clustered with another short flag where log_takes_separate_value's
is_solo gate correctly refuses to link a value (e.g. "-cn 2" leaves "n"
with no attached/linked value at all, per the earlier clustering fix).
RTK would then believe the user explicitly set a commit-count limit
(skipping its own --no-merges default) even though no value was ever
captured. Extracted has_limit_flag into its own testable function
(matching parse_limit_from_tokens's convention) and required t.value()
to be Some for the "n" case.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(search): make -r value-taking detection engine-aware
1eba55e 
is_short_value_flag treated -r as unconditionally boolean, but
confirmed via real rg (`rg -rn hello file` prints "n", `rg -rXXX hello
file` prints "XXX" -- proof -r consumes the cluster remainder as its
--replace value) that ripgrep's -r takes a value (grep's -r/-R is
--recursive, boolean, with no equivalent to rg's -r at all).
`rtk rg -rREPLACEMENT pattern src` was misread as a boolean cluster
instead of -r's attached replacement value, corrupting the actual
command RTK executes -- extract_pattern_path's reconstructed
patterns/paths/flags feed directly into building the invoked command,
not just filtering, so this was a real command-substitution bug.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(git): run_stash's patch-mode check needs -- boundary awareness too
4c54b3d 
run_stash's "show" branch used a raw arg == "-p" scan for patch-mode
detection, the same bug class already fixed for sibling run_log/
run_diff/run_show -- a pathspec literally named "-p" or "--patch" after
`--` was wrongly treated as the real patch flag, routing to compact_diff
instead of RTK's compact_stash_stat summary. Extracted into a testable
stash_show_wants_patch(args) function using tokenize_git +
log_takes_value/log_takes_separate_value, matching only the patch-mode
flags (not the full requests_raw_diff_shape list, since --stat et al.
correctly stay on the compact_stash_stat branch here).

An end-to-end integration test proved impractical: real git's own
`git stash show -- <pathspec>` unconditionally shows full patch content
whenever limited by any pathspec, regardless of -p, so the final git
output can't distinguish flag-vs-pathspec interpretation -- verified
this with a direct git session before settling on a unit-level test
against the extracted predicate instead.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(dotnet): expand dotnet_takes_value with more confirmed value-taki…
088f588 
…ng flags

dotnet_takes_value's allowlist (logger/report/results-directory) was
narrower than dotnet's real value-taking flags. Confirmed via a real
dotnet 9 SDK (Docker, dotnet test --help/dotnet build --help) and added
--filter, -c/--configuration, -f/--framework, -r/--runtime, -a/--arch,
--os. Without these, e.g. `dotnet test --filter
FullyQualifiedName~Foo.csproj` tokenized the filter value as an
unlinked Positional, misread by detect_test_runner_mode_in_dir's
explicit_projects filter as an explicit project reference (since it
coincidentally ends in .csproj), skipping the working-directory scan.

The short forms (-c/-f/-r/-a) are matched under TokenKind::Long, not
Short: under Dialect::Msbuild, tokenize_dialect routes every
single-dash arg through push_atomic_flag, which always produces a Long
token (Msbuild has no character-clustering concept the way Dialect::Posix
does) -- also confirmed dotnet has no glued short form like -cRelease
either way (MSB1001: Unknown switch; only "-c Release" or "-c=Release"
work).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(golangci): add missing out-format to golangci_run_takes_value
ae267a4 
golangci_run_takes_value's allowlist omitted "out-format" even though
has_output_flag explicitly checks for it -- its separate-token value
(e.g. `--out-format json`) tokenized independently as an unlinked
Positional instead of being linked, the same bug class already fixed
for --path-prefix and other run-level flags in this same file.
out-format is a v1-only legacy flag (golangci-lint 2.x's --help no
longer lists it), kept for v1 installs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(arg_tokenizer): dedupe double-dash-flag predicate; derive se…
e07f60f 
…en_dash_dash

- has_double_dash_flag/double_dash_flag_value/double_dash_flag_values
  shared the same 3-part match condition inline three times; factored
  into is_double_dash_flag(). double_dash_flag_value keeps its own
  find() (not double_dash_flag_values().next()) since that would subtly
  change semantics: "first match regardless of whether it has a value"
  vs "first match that has a value".
- seen_dash_dash was always exactly emitted_dash_dash && dialect ==
  Posix (only ever set true immediately after emitted_dash_dash inside
  the Posix branch); removed the redundant variable and derived the
  condition directly. Pure refactor, no behavior change either way.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(arg_tokenizer): add Token::is_free_positional(), dedupe 4 ca…
199de43 
…ll sites

"Positional kind, and not consumed as some preceding flag's value" was
hand-spelled identically in git.rs (x2), golangci_cmd.rs, search.rs,
and dotnet_cmd.rs. Added Token::is_free_positional() to the shared type
and switched all 5 call sites to it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(arg_tokenizer): add dashdash_index()/has_dashdash(), dedupe …
2b6f8bd 
…4 call sites

Finding the `--` boundary token's position was hand-rolled
independently at dotnet_cmd.rs:713,864 and git.rs:1328,1352. Added
arg_tokenizer::dashdash_index (index into the tokens slice;
tokens[i].source_index recovers the original-args position) and
has_dashdash (presence check), and switched all 4 call sites to them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(git): add git_log_tokens() helper, dedupe 9 tokenize_git cal…
9339df1 
…l sites

log_takes_value/log_takes_separate_value were passed to tokenize_git
at 9 separate call sites across run_log/run_diff/run_show/run_stash and
their test helpers. Added git_log_tokens(args), mirroring dotnet_cmd.rs's
dotnet_tokens() consolidation, and switched every call site to it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(dotnet): dedupe has_nologo_arg/has_write_mode_override's sha…
68602e1 
…red predicate

Both hand-rolled the identical "Long flag, no attached value,
case-insensitive name match" shape (differing only in whether
double_dash is required), because neither arg_tokenizer.rs primitive
checks `attached` at all. Factored into dotnet_has_bare_loose_flag/
dotnet_has_bare_double_dash_flag, matching the existing
dotnet_has_loose_flag/dotnet_has_flag naming split.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(golangci): golangci_run_takes_value composes golangci_takes_…
2bbb3b2 
…value

golangci_run_takes_value hand-duplicated golangci_takes_value's 5 flag
names (color/config/cpu-profile-path/mem-profile-path/trace-path)
instead of building on it, even though golangci_takes_value's set is a
strict subset (every pre-run flag is also valid after run). That
duplication is exactly how the "out-format" gap (fixed earlier this
branch) came about -- one list got a new entry, the other didn't. Pure
refactor, behavior-preserving (confirmed by the full existing test
suite, unchanged).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(arg_tokenizer): replace push_atomic_flag's 8 params with a S…
e1d9f81 
…canner struct

push_atomic_flag/link_next_value threaded 7-8 positional parameters
(tokens, args, i, dialect, takes_value, takes_separate_value,
emitted_dash_dash) by hand, carrying #[allow(clippy::too_many_arguments)].
Introduced a Scanner<'a, 'p, T> struct grouping this shared scan state,
with push_atomic_flag/link_next_value as &mut self methods.
tokenize_dialect_ex now owns one Scanner instance instead of five loose
local variables threaded through free functions -- a future addition
to the scan state (this module already had to add one,
takes_separate_value) means one new field instead of a parameter
threaded through every helper and call site.

Pure refactor, no behavior change: full test suite (2762 tests) passes
unchanged, clippy is clean with no allow needed anymore. Verified the
one pre-existing --ignored test failure (test_git_status_not_a_repo_exits_nonzero)
is an unrelated, pre-existing locale-dependent flake by reproducing it
identically on the pre-refactor commit.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
docs(dotnet): document explicit_projects' filesystem-backstop limitation
760587b 
explicit_projects has no filesystem backstop and relies entirely on
dotnet_takes_value's allowlist being exhaustive; a real dotnet flag
missing from it (e.g. --property/-p, --collect, --diag, --settings,
--blame, --output/-o, --source, --sln) whose value ends in a project
extension would still be misread as an explicit project path, the same
class of bug three earlier commits in this branch each closed one flag
at a time. Same root cause as arg_tokenizer.rs's Msbuild
single-segment-path limitation (real MSBuild needs a stat() call to
resolve the analogous ambiguity) -- documented as a known, deliberate
tradeoff rather than fixed, since teaching this function to stat()
every candidate would be a materially different, slower design.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(search): compute context/show_file/show_line from tokens, not rec…
857714d 
…onstructed strings

has_short_flag scanned the reconstructed flags: Vec<String>, which also
contains value-taking flags' own separate-token values as bare,
unmarked strings alongside real flags -- a value starting with '-' and
containing the queried letter was misread as the real flag (e.g.
`rtk rg --replace '-Chart' pattern src` misdetected a -C context flag
from --replace's own value).

Added DetectedFlags (show_file/show_line/context), computed directly
from token kind/linked in extract_pattern_path's own walk -- the same
technique already used for has_format_flag, extended to cover
has_context_flag/show_file/show_line's letters (H/r/R/n/N/A/B/C) too.
A consumed value token is never examined as a candidate flag at all,
regardless of its text, since it's classified Positional+linked rather
than re-derived from a reconstructed string.

show_file's -r/-R check is engine-aware (only counts for Grep: rg's -r
is --replace, an unrelated value-taking flag, not recursive-descent;
rg has no -R at all), matching the is_format_flag_token/
is_short_value_flag precedent already established for -L/-z/-T/-r
elsewhere in this file.

show_file/show_line/has_context_flag/has_short_flag become
#[cfg(test)]-only wrappers preserving the existing extensive test
suite unchanged; the now-fully-dead show_file (paths+flags combined)
production wrapper is replaced by wants_show_file (the paths-only half,
combined with DetectedFlags::show_file at each call site) since it had
no direct test callers of its own.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(git): stop git diff -p / git show -p from bypassing RTK's compaction
b51e6ae 
requests_raw_diff_shape's -p/-u/--patch match was correct for run_log (whose
default output has no diff content at all, so an explicit -p is a genuine
request for something the filtered path never produces) but was wrong when
run_diff/run_show started sharing that same predicate this branch: diff/show's
default output already IS patch text, so an explicit -p/-u/--patch is
redundant with the default, not a request for an incompatible shape. Sharing
the full list meant `git diff -p` / `git show -p <rev>` silently skipped
RTK's stat + compacted-diff pipeline and returned full raw output instead --
a real token-savings regression for a very common invocation, caught by
/code-review high with zero test coverage.

Add requests_diff_show_raw_shape, the same shape-flag list minus patch/p/u,
and point run_diff/run_show at it while leaving run_log on the original
requests_raw_diff_shape. --patch-with-raw/--patch-with-stat still trigger the
raw path for diff/show since they mix patch text with a raw/stat format
RTK's own extra --stat step would double up against.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(arg_tokenizer): share one token() constructor across constru…
498175f 
…ction sites

The DashDash, digit-run Short, and cluster-loop Short token pushes each
hand-built a full Token{...} struct literal instead of reusing the
positional()-style constructor already used elsewhere in the same function
(and push_atomic_flag's Long push did the same). Token gaining a new field
later -- a growth pattern this module's own docs already anticipate, citing
takes_separate_value as precedent -- meant updating 4 literals instead of 1
call site, with a forgotten field at one of them silently defaulting wrong
instead of failing to compile.

Generalize positional() into token(kind, text, source_index, double_dash),
have positional() call it, and update every other construction site to build
on it via struct-update syntax the same way link_next_value already does on
top of positional(). Pure refactor, no behavior change: cargo test --all
passes the same 2765 tests before and after.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(dotnet): fix Windows CI unused-import/dead-code in dotnet_double_…
d08fff4 
…dash_test.rs

The test itself is #[cfg(unix)] (it spawns a /bin/sh stub using unix file
permissions), but its Command import and shell_quote helper weren't gated
the same way -- on Windows the test compiled out entirely, leaving both
genuinely unused and tripping -D unused-imports / -D dead-code.

Nest both inside the test function's body instead of gating them separately:
since the whole function is already #[cfg(unix)], that one attribute now
covers everything, and Linux/macOS behavior is unaffected (cargo test --all
still passes the same 2765 tests).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(arg_tokenizer): rename tokenize_git to tokenize_with_separat…
2de7ef6 
…e_value

tokenize_git had exactly one caller in the whole codebase (git.rs's own
git_log_tokens()), and zero git-specific code -- it's a thin wrapper fixing
Dialect::Posix while leaving takes_separate_value customizable, reusable by
any POSIX-ish tool with the same short-flag clustering quirk. Naming a
shared, mechanism-generic function after its only current caller was
misleading: either it should generalize (drop the git-specific name) or it
should live in git-specific code. Since the git-specific rationale (the
-n/-M-family rules) was already documented at git.rs's own
log_takes_separate_value, not lost by renaming, generalizing was the smaller
change.

Trimmed the moved function's doc comment down to the general mechanism,
pointing to log_takes_separate_value for the git-specific quirks it
originally listed inline. Pure rename, no behavior change: cargo test --all
passes the same 2765 tests before and after.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(dotnet): reject attached-value spellings in has_report_trx_arg
43d7a7e 
--report-trx is a pure boolean flag, same class as --write/--nologo
(already fixed earlier in this branch). The non-bare dotnet_has_flag
lookup let a broken spelling like --report-trx:true count as "already
present," skipping RTK's own injection while still forwarding the
unrecognized flag straight through to real dotnet.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor(git): delegate requests_diff_show_raw_shape to requests_raw_…
ed9a99e 
…diff_shape

Was duplicating requests_raw_diff_shape's Long-flag list verbatim
(minus "patch") instead of delegating and excluding the patch-only
cases. Same list-drift pattern that already caused the git diff -p/
git show -p regression fixed by 7533879 -- delegating rules out the
two lists silently diverging again.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(git): stop stash_show_wants_patch from swallowing -p as a log-onl…
b42f70a 
…y option's value

stash_show_wants_patch reused git log's separate-value grammar
(log_takes_value/log_takes_separate_value), under which --author is a
value-taking Long flag. git stash show's real grammar is much
narrower ([-u] <diff-options> [<stash>]) and doesn't recognize
--author at all -- confirmed against real git 2.53.0 that
`git stash show --author -p` produces full patch output, since
--author consumes no value there. The old code swallowed the
following -p as --author's value, causing patch content to render
through the stat-only compact_stash_stat path instead of compact_diff.

Now tokenizes with a predicate that never treats any flag as taking a
separate value, since this detector only needs to spot a bare
-p/-u/--patch token -- that can only ever false-positive, never
silently swallow a real one.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(git): stop stash_show_wants_patch from treating -u as patch mode
12d8971 
-u means --include-untracked for git stash show, not -p (unlike git
log, where -u is a -p synonym). Conflating them routed -u's stat-only
output through compact_diff, which only renders patch content,
producing silently empty output where a stat summary was expected.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(search): let grep/rg value-taking flags claim a literal -- as the…
47688a0 
…ir value

grep -e -- f (and any other value-taking grep/rg flag followed by a
literal --) was misparsed: the tokenizer's -- boundary guard is correct
for git/cargo, which always reject -- as a pending value, but grep/rg
both consume -- unconditionally as any value-taking flag's value
(confirmed for -A/-m/-e/--context/--file across both engines). RTK
treated -- as the end-of-options boundary instead, so `rtk grep -e --
f` searched for pattern "f" in /dev/null instead of "--" in file "f".

Adds Scanner::claims_literal_dash_dash, exposed as an opt-in field on
a new TokenizeOptions passed to tokenize_with_options -- defaults to
false everywhere except search.rs, which reuses its own takes_value
predicate for it. Folds the now-redundant tokenize_dialect and
tokenize_with_separate_value into the same options struct so the
module doesn't keep growing one wrapper function per axis.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
fix(search): detect real pipes on stdin instead of just non-terminal
2dd75d8 
reads_piped_stdin treated any non-terminal stdin as "the engine will
read stdin," including a non-pipe redirect like `< /dev/null`. Real
grep/rg still search the cwd/default path in that case, not stdin.
This routed rtk rg -z foo < /dev/null (and, pre-existing, any rtk rg/
grep invocation with redirected-but-not-piped stdin and no explicit
path) into the streaming path, which can't discover "multiple files
matched" the way the buffered/grouped path can, so it silently
dropped filenames from every line.

stdin_is_pipe() checks the actual file type (FIFO) on Unix instead of
just is_terminal(), falling back to the prior heuristic elsewhere.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
docs(core): document arg_tokenizer.rs and the per-subcommand grammar …
6b22642 
…rule

Reviewer asked that this system be documented in the relevant
READMEs. Adds an Argument Tokenizer section to src/core/README.md
covering what the module provides and the design rule two confirmed
bugs this round came from: never reuse a sibling command's grammar
predicate wholesale (stash_show_wants_patch's -u/-p conflation,
requests_diff_show_raw_shape's list-drift). Cross-links from
src/cmds/git/README.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh @claude
refactor: trim comment bloat across arg_tokenizer.rs and its callers
be0c3d7 
Per review feedback: many doc comments in this branch had grown longer
than the code they documented, narrating investigation history
("confirmed via Docker/real git X.Y.Z", commit hashes, issue numbers)
instead of stating the non-obvious fact a reader actually needs, or
simply restating what a well-named function/type already says.

Trimmed every multi-paragraph comment across the 5 files this branch
touches down to 1-2 lines, keeping only the load-bearing why. Also:

- dropped comments that only restated a well-named call
  (restore_double_dash, dotnet_takes_value's Dialect::Msbuild) or a
  callee's own doc (run_diff/run_show's tokenize call sites duplicated
  requests_diff_show_raw_shape's rationale)
- renamed git_log_tokens/dotnet_tokens to tokenize_git_log_args/
  tokenize_dotnet_args so the call site reads as an action without
  needing a comment
- trimmed several "Regression: X used to..." test comments down to
  the bare invariant under test
- documented the tokenizer's design in src/core/README.md, kept light
  (no consumer list, no bug writeup, just the rule and one example)

Comment-line counts in the diff against develop: from roughly 18% of
added lines down to ~16%. No logic changes anywhere in this commit:
comments, blank lines, and two function renames only, verified via
cargo fmt/clippy/test after every step.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KuSh
KuSh force-pushed the feat/arg-tokenizer branch from 46c3b51 to be0c3d7 Compare August 26, 2026 21:34
@KuSh

KuSh commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

@aeppling took all of this into account

@KuSh
KuSh requested a review from aeppling August 26, 2026 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aeppling aeppling Awaiting requested review from aeppling

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@KuSh @aeppling