Repository navigation
Conversation
…sessions encode_project_path mirrors Claude Code's cwd -> ~/.claude/projects slug, but never replaced the drive-letter ':' with '-'. Every Windows cwd carries one, so the encoded slug (C:-Users-me) never matched Claude's real folder (C--Users-me) and the default `rtk discover` scanned 0 sessions on Windows. Add ':' to SANITIZED_CHARS; the shared encoder also fixes `rtk learn`. Refs #2919
BufRead::lines().map_while(Result::ok) returns Err on a non-UTF-8 line (e.g. OEM/ANSI bytes from a non-English-locale Windows tool), and map_while stops at the first None — silently discarding every line after the bad one too, not just the bad one. A failing build command whose error text contains a single non-UTF-8 byte produces output that looks like a clean success. Add read_lines_lossy(), which reads raw bytes and decodes each line with String::from_utf8_lossy (invalid bytes become U+FFFD) instead of erroring, and use it at all 6 call sites in run_streaming instead of the truncating lines()/map_while(Result::ok) pattern. Fixes #2994
…rten hints The tee recovery filename embeds a command-derived slug (often a file path that duplicates the command the LLM already issued), costing ~12 tokens in every truncation hint. Long slugs (>24 chars) now collapse to a short readable prefix plus a 6-hex SHA-256 tag. This also fixes a latent collision in the old 40-char truncation: sibling paths sharing a long common prefix (e.g. several `git show` blobs from the same directory in the same second) truncated to an identical filename and overwrote each other's recovery file. Hashing the full slug makes distinct commands produce distinct filenames (~1-in-16M collision).
KuSh's review on this PR flagged two real issues in read_lines_lossy: - Ok(0) | Err(_) => None treated a genuine I/O error the same as clean EOF, silently truncating output on a real read failure -- exactly the "failing build looks clean" class of bug this PR exists to fix, just relocated rather than removed. - A fresh Vec was allocated every iteration on a hot streaming path instead of reusing std's own buffered split. Replaced the hand-rolled read_until loop with BufReader::split(b'\n'), per the reviewer's suggested implementation: reuses std instead of reimplementing it (except CR-stripping, which split() doesn't do), and now surfaces a genuine I/O error to stderr instead of silently conflating it with EOF. Added a test with a Read impl that yields good lines then a real error, confirming the error path doesn't panic or hang and the already-read lines are still preserved.
…-in-stream-filters fix(stream): decode lossily instead of dropping lines on invalid UTF-8
…-colon fix(discover): sanitize drive-letter colon so Windows discover finds sessions
On Windows with non-UTF-8 console code pages (e.g., GBK for Chinese locale), child process output is mis-decoded by String::from_utf8_lossy, producing mojibake. Add decode_process_output() that detects the console output code page via GetConsoleOutputCP() and decodes with encoding_rs. Replaces from_utf8_lossy in the core capture paths (exec_capture, exec_capture_stdin, TOML filter path, proxy streaming path). Module- specific call sites left for follow-up. Fixes #2452
Replace hand-written FFI extern block with windows-sys crate binding for GetConsoleOutputCP to satisfy semgrep unsafe-block rule. Rewrite test_decode_process_output_gbk to test encoding logic directly via codepage_to_encoding(936) instead of relying on the CI runner's actual console code page (which is not GBK on GitHub Actions).
…cess_output Review feedback (KuSh): the helper existed but most filter modules still called String::from_utf8_lossy directly, so non-UTF-8 console output (e.g. GBK on Chinese-locale Windows) was still mangled for most commands. Wire decode_process_output through the remaining child-output call sites: git, go, aws, curl (non-binary paths only — raw binary passthrough for #1087 is untouched), system read, and discover registry. File-content decoding (dotnet TRX XML, hook trust snippets) intentionally keeps from_utf8_lossy since console code pages don't apply there.
requests_raw_log_output() scanned the full args slice for patch flags like -p, so `git log -- -p` (a literal pathspec named -p) was misdetected as a raw patch request and skipped RTK's normal filtering. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…h-2944 fix(git): preserve patch output from log commands
… flag requests_raw_log_output() matched bare -p/-u/--patch tokens anywhere in the args, without checking whether the previous token was an option like --grep or -S that consumes the next token as its value. `git log --grep -p` searches commit messages for the literal string "-p" (verified against real git 2.53.0: no diff output, identical to --grep=-p) but RTK treated it as a patch request and skipped its own filtering/limit, dumping raw uncapped git log output for what is actually a plain grep search. Skip the value token after any known value-taking git log/diff option before checking for the patch flags.
…xec_capture Addresses the inline review on #2717. decode_process_output - Decode a line at a time instead of reinterpreting the whole buffer at the first bad byte. Valid UTF-8 lines keep their bytes; only lines that fail UTF-8 validation go through the code page, so one stray byte no longer mangles output that was almost entirely UTF-8. The line is the unit because a byte run is not one: GB18030's four-byte sequences embed bytes in the ASCII digit range, so any rule that ends a run below 0x80 splits them. \n cannot appear as a trail byte in any encoding handled here, and a process does not switch encoding mid-line. - A code page result is only accepted when it decodes cleanly, so a UTF-8 line with a corrupt byte falls back to lossy UTF-8 rather than mojibake. - Replace the hand-written code page table with the codepage crate, as suggested. That also fixes 54936, which was mapped to GBK and now correctly resolves to gb18030. - Add oem_cp for the legacy OEM/DOS pages (437, 850, 852, …) that plain cmd.exe still defaults to in many locales. encoding_rs implements only WHATWG encodings, so codepage alone returns None for them. - Fall back to GetACP when GetConsoleOutputCP reports no console, which is the piped case rtk normally runs in, and warn once instead of falling back to lossy silently. - Cache the code page lookup in a OnceLock. - The mapping and the walk take the code page as a parameter, so they are compiled and unit-tested on every platform rather than only Windows. Call sites - Route the remaining production sites through stream::exec_capture and exec_capture_stdin rather than decoding at each one, so future callers inherit decoding. git commit keeps inherited stdin via the _stdin variant. Test-only sites go back to from_utf8_lossy: they assert on rtk's own UTF-8 output, where a console code page has no meaning. - Decode the streamed path (read_lines_lossy) too — the OEM/ANSI lines its comment describes were still going straight to U+FFFD. - curl keeps its body on from_utf8_lossy: a response body is a network payload whose encoding comes from the HTTP charset, not the local console, and non-UTF-8 bodies already take the binary passthrough for #1087. Only curl's own stderr is code page decoded. git commit summary parsing - parse_commit_output sliced from byte 1, which panics when the first line starts with a multi-byte character — git prints hook output before its summary, and a lossily decoded line starts with a multi-byte U+FFFD. Locate the bracket pair with find instead, so both indices are character boundaries. Verified: unit tests for the walk, GBK, gb18030, CP437/850, mixed lines, truncated input and every byte value; a test pinning that output without a code page stays byte-identical to from_utf8_lossy; and the Windows-only lookup cross-compiled for x86_64-pc-windows-msvc.
Moving call sites onto exec_capture swapped exit_code_from_output for status_to_exit_code, which returns the same 128 + signal but drops the stderr line explaining that the child was killed rather than exiting. A command that dies to an OOM kill would just report 137 with no reason. Route both capture helpers through one function that calls exit_code_from_output, using the program name as the label so no call site has to pass one. Covered by a test that terminates a child with SIGTERM and asserts the 128 + signal code survives.
The unattestable_passthrough tests called evaluate(), which calls check_command() and reads the developer's Claude Code settings files (.claude/settings.local.json, ~/.claude/settings.json). A local `Bash(git *)` allow rule turned the expected Ask into Allow, so the two rewrite assertions failed on that machine and passed everywhere else; deny rules would likewise have broken the four Passthrough assertions. Give evaluate() the same verdict-injection seam that permissions.rs already exposes via check_command_with_rules: evaluate_with_verdict() holds the decision logic and takes the verdict as a parameter, while evaluate() stays the thin wrapper that looks it up. The tests pin PermissionVerdict::Default, so they read no settings at all and assert the exact outcome again rather than accepting either verdict. Added coverage for the Allow and Deny verdicts so the mapping stays tested in both directions without depending on host configuration. Verified by running the test binary with HOME pointed at settings files containing deny, ask, allow, and no rules: 8/8 pass in all four, and with the repo's own settings.local.json in place. Closes #3146
fix(core): decode process output using Windows console code page
Inline evaluate_with_verdict() calls directly in unattestable_passthrough tests instead of through a locally-named eval() alias, and move the two verdict-to-outcome tests (allow/deny) up into the parent tests module since they aren't about unattestable-construct passthrough. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fix(test): accept both Ask and Allow verdicts in rewrite tests
….org The remaining online calls (curl robots.txt + wget /json on mockhttp.org) were both a network dependency and non-deterministic. Serve fixed local fixtures over a loopback http.server so curl and wget get real Content-Type headers (exercising JSON minification), fully offline. curl falls back to file:// when python3 is unavailable. Clean up the server, temp fixtures, and the ./data.json download on exit.
CargoTestHandler.format_summary could emit a compacted summary larger than the raw output on tiny runs, same failure mode CargoBuildHandler already guards. Wrap all return paths with never_worse so the raw output wins when the summary would be bigger.
…et -e safe Review follow-up on #3430: - Bind `python3 -m http.server 0` so the kernel picks a free port and read the chosen one from the (unbuffered) server log, instead of hardcoding 8899 which fails needlessly when that port is already in use. - Make `cleanup_net_fixtures` failure-tolerant: it runs from an EXIT trap under `set -e`, so `[ -n "$PID" ] && kill ...` aborted the whole handler whenever `kill` failed (server already dead), leaking the fixture dir and downloads. - Give the wget case an explicit skip line instead of silently disappearing. wget rejects `file://` ("Unsupported scheme"), so there is no offline URL to fall back to when the loopback server is unavailable. RED (PR HEAD): fixture dir NOT removed when kill fails; server unusable with 8899 taken. GREEN: fixture dir removed; server up on an ephemeral port.
…se guard Review follow-up on #3430: - Replace the IIFE closure in `CargoTestHandler::format_summary` with a private `compute_test_summary` on the existing `impl CargoTestHandler`, and apply `never_worse` at the call site, as suggested. - Add `test_cargo_test_summary_uses_raw_when_summary_is_larger`: a one-line compile failure whose "cargo test: N errors, ..." header is larger than the raw output, so the guard must return the raw output. RED→GREEN: the new test fails on upstream develop (returns the 105-byte header form instead of the 62-byte raw), passes here. Full suite: 2612 unit + 78 integration tests pass; benchmark 0 negative (develop baseline: 1).
…curl-cargo fix(benchmark): avoid negative curl/cargo cases that fail the benchmark job
…arness `BENCH_DIR="$(pwd)/scripts/benchmark"` is a tracked directory: besides the gitignored `unix/`, `rtk/` and `diff/` output dirs it holds the TypeScript VM-benchmark harness (`run.ts`, `cleanup.ts`, `rebuild.ts`, `lib/*.ts`, `cloud-init.yaml`). `rm -rf "$BENCH_DIR"` therefore wiped all 7 tracked files from the working tree on every local run (`$CI` unset), which is exactly when a contributor runs the benchmark before pushing. Wipe only the three gitignored output subdirectories instead. Stale output is still cleared between runs; the harness survives. Follow-up to #3430 (#3430 review).
The guard compared the grouped summary to the full match list, so a --max-capped run could emit more tokens than plain paths (benchmark 'find --max 10' negative case). Baseline is now the plain listing truncated at max_results with a +N more marker.
Per src/cmds/README.md truncation-recovery rule: when the cap comes from the default (agent never asked to truncate), tee the full flat listing and emit force_tee_tail_hint so hidden items are recoverable without re-running. An explicit -m/--max keeps the bare +N more marker: the agent asked for less.
The hook rewrites any find command, but rtk find bailed on compound predicates and actions (-not, -exec, ...) with advice to 'use find directly' that the hook makes impossible to follow: the retry gets rewritten again. Now such commands execute the real find from PATH with the original args, output and exit code untouched, tracked at 0% savings.
These options only accept an attached value (-U3, --unified=3, --expand-tabs=4) — verified against git 2.53.0, where `git log --expand-tabs 4` fails with "fatal: ambiguous argument '4'" instead of treating 4 as the value. consumes_next_token_as_value() was swallowing the next token for them anyway, so a real -p right after one of these was misread as their value and the raw patch request went undetected. Addresses review feedback from @aeppling on #3575. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Audited every option in consumes_next_token_as_value() against real git 2.53.0 behavior (git log <opt> <token> -1, checking whether the token gets swallowed as the option's value or leaks through as a positional arg). --max-parents and --min-parents behave like -U: `git log --max-parents 2` fails with "ambiguous argument '2'", so a real -p right after them was being misread as their value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cross-checked git-log(1)'s full option list against consumes_next_token_as_value() (using git log <opt> <token> -1 against real git 2.53.0 to see whether <token> gets swallowed as the option's value or leaks through as a positional arg). --diff-algorithm and --diff-filter both take a required, separate-token value (`git log --diff-algorithm -p` rejects -p as an invalid algorithm rather than treating it as the patch flag) but were missing from the list, so a genuine -p right after either was misdetected as the option's own value and the raw-patch request went undetected. Every other bracket/optional-value option checked (--format, --pretty, --stat, -M, -C, -B, etc.) is correctly attached-value-only and stays out of the list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…values as flags has_limit_flag, has_format_flag, wants_merges, and parse_user_limit scanned args positionally with no awareness that a value belonging to --grep, --author, etc. can itself look like a flag (-5, --pretty, --merges), reproducing the same misdetection class this branch already fixed for -p. Unify these into a single log_arg_tokens tokenizer shared by requests_raw_log_output, real_flag_args, and parse_user_limit, which also stops at the -- pathspec separator so a literal path like -5 after -- isn't misread as a flag.
…rough requests_raw_log_output only recognized -p/-u/--patch variants as needing the raw path. --stat, --numstat, --name-only, --name-status, --raw, --shortstat, --dirstat, and --summary change git's raw output shape the same way -p does, but weren't listed — RTK's injected --pretty=format + ---END--- markers can't coexist with them, so the diffstat/name-list block got misparsed as the start of the next commit, mangling output silently instead of just leaving it unfiltered. Also share one log_arg_tokens() pass across run_log's flag/limit checks instead of retokenizing per check, and drop a redundant "-n" match arm already covered by consumes_next_token_as_value.
…lision fix(tee): hash long recovery-file slugs to prevent collisions and shorten hints
…-tracked-harness fix(cicd): stop benchmark.sh deleting the tracked scripts/benchmark harness
Contributor
|
#3603 for failing CI because of find cmd never worse guard |
Every other run_* function that inspects args for "--" (run_diff, run_checkout) calls args_utils::restore_double_dash() first to re-insert the "--" that clap's trailing_var_arg strips out. run_log never did, so requests_raw_log_output()/log_arg_tokens()'s own take_while(|arg| *arg != "--") check was dead code in production: by the time run_log saw args, the literal "--" was already gone. Concretely, `rtk git log -- -p` lost its "--" and -p (a file literally named -p, a valid pathspec) was misdetected as the real patch flag, routing to raw passthrough instead of RTK's filtered log. Added an end-to-end regression test (real rtk binary, real git repo) alongside the existing git_log_patch_output_matches_raw_git test, since restore_double_dash reads live process args and can't be exercised by calling run_log's internals directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ed-as-patch-flag fix(git): don't misdetect a value-taking option's argument as a patch flag
track() with stdout as both input and output recorded real token counts at 0% savings, diluting the global gain percentage for commands rtk never tried to filter. track_passthrough records 0/0, neutral by design.
Flat path sort and display dir sort diverge ('logs-old/f' < 'logs/f'
as paths, 'logs' < 'logs-old' as dirs), so tail on the flat-sorted
tee could return files already shown while the hidden ones stayed
buried. One display-ordered list now feeds the summary, the capped
listing, and the tee.
The passthrough decoded find's output through the lossy text decoder, so non-UTF-8 filenames came out with U+FFFD, including under -print0 where the bytes feed xargs -0. Write the captured stdout/stderr bytes verbatim instead; exit code and signal diagnostic unchanged.
…deled predicates Paths are the tokens before the first expression token, as in find, so 'find src' lists src instead of matching a file named src. rtk walks only the subset it models (-name/-iname globs, -type f|d, -maxdepth); any other listing predicate runs the real find and its results go through the same compressed renderer; actions (-exec, -delete, -print*, ...) stay verbatim. Also: nonexistent path exits 1, long unicode dir labels no longer panic, rtk -m/-t mixed with actions is a clear error. Known gaps from fuzzing, to fix before merge: leading -H/-L/-P/-O/-D options, rtk flag scan reaching into -exec args, repeated subset predicates, -t precedence under -o, root '.' entry rendered empty.
…ions forwarded Leading -H/-L/-P/-D/-O options are recognized and forwarded ahead of the paths. rtk's -m/-t are interpreted only when the whole expression is the modeled subset, each predicate at most once; everything else reaches find untouched, so -exec arguments and predicate values are never misread and -t can no longer change precedence under -o. The root '.' entry keeps its name in compressed output.
Explicit remove_dir_all trips the filesystem-deletion semgrep rule; TempDir cleans up on drop like the other test modules.
…ck on legacy syntax Verbatim runs go through runner::run_passthrough (inherited stdio, live output). The tee hint is emitted through runner::emit_guarded so body plus hint never exceed the plain listing. rtk's -m/-t are read only as trailing tokens and apply to native and compressed runs (( expr ) -type T keeps precedence); combined with an action they are refused rather than dropped. Legacy 'find <pattern> ...' is rewritten to -name form and dispatched. Default cap is CAP_INVENTORY.
…efore actions; paths like find The never-worse baseline now carries the tee hint too, so a truncated listing always keeps its recovery path. Trailing -m/-t in front of an action are forwarded untouched and find rejects them itself, no rtk error string. The native walker prints paths from the search root as find does. File formatted with rustfmt directly: automod hides src/cmds from cargo fmt.
Truncating labels over 50 chars to ...tail left grouped output unresolvable whenever the search root was absolute; a label is printed once per directory, so the saving was negligible.
Restores the pre-PR output shape: paths relative to the search root and long directory labels shortened, both cheaper in tokens. A bare positional is a path only when it is an existing directory, so 'find src' lists src while 'rtk find Cargo.toml' still searches by name as it always did.
fix(find): never-worse guard, recovery hint, and dispatch on find's grammar
aeppling
approved these changes
Aug 26, 2026
The bare-positional test asserted /tmp is a path; on Windows it does not exist, so the directory rule treated it as a name pattern.
test(find): use the platform temp dir instead of /tmp
pszymkowiak
approved these changes
Aug 26, 2026
pszymkowiak
left a comment
pszymkowiak
left a comment
Collaborator
There was a problem hiding this comment.
Deep review of the full bundle (11 aggregated PRs) done. 3 minor correctness issues found (-type f -t d silently overwritten, -m with invalid value gives a confusing find-side error instead of a clean rtk error, git diff/show still exposed to the value-taking-option bug this release fixes for git log), plus a small perf regression in the streaming line reader and some cleanup opportunities — none blocking. The red 'benchmark' CI check is a pre-existing golangci-lint-on-this-runner issue, unrelated to this PR (same failure on develop since before these commits). LGTM to ship.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feats
Fix
Other