diff --git a/Cargo.lock b/Cargo.lock index 5503fca9bd..cd7af9e28c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -217,6 +217,15 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3a822ea5bc7590f9d40f1ba12c0dc3c2760f3482c6984db1573ad11031420831" +[[package]] +name = "codepage" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "48f68d061bc2828ae826206326e61251aca94c1e4a5305cf52d9138639c918b4" +dependencies = [ + "encoding_rs", +] + [[package]] name = "colorchoice" version = "1.0.4" @@ -229,7 +238,7 @@ version = "3.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "faf9468729b8cbcea668e36183cb69d317348c2e08e994829fb56ebfdfbaac34" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -333,6 +342,15 @@ dependencies = [ "syn", ] +[[package]] +name = "encoding_rs" +version = "0.8.35" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "75030f3c4f45dafd7586dd6780965a8c7e8e285a5ecb86713e63a79c5b2766f3" +dependencies = [ + "cfg-if", +] + [[package]] name = "env_home" version = "0.1.0" @@ -747,6 +765,15 @@ dependencies = [ "autocfg", ] +[[package]] +name = "oem_cp" +version = "2.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8c7959d3349f484aec36462f98226983578bf7d33f9b034c32967489c7000e1f" +dependencies = [ + "phf", +] + [[package]] name = "once_cell" version = "1.21.3" @@ -771,6 +798,24 @@ version = "2.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" +[[package]] +name = "phf" +version = "0.11.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fd6780a80ae0c52cc120a26a1a42c1ae51b247a253e4e06113d23d2c2edd078" +dependencies = [ + "phf_shared", +] + +[[package]] +name = "phf_shared" +version = "0.11.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "67eabc2ef2a60eb7faa00097bd1ffdb5bd28e62bf39990626a582201b7a754e5" +dependencies = [ + "siphasher", +] + [[package]] name = "pkg-config" version = "0.3.32" @@ -891,12 +936,15 @@ dependencies = [ "automod", "chrono", "clap", + "codepage", "colored", "dirs", + "encoding_rs", "flate2", "getrandom 0.4.2", "ignore", "libc", + "oem_cp", "quick-xml", "regex", "rusqlite", @@ -908,6 +956,7 @@ dependencies = [ "ureq", "walkdir", "which", + "windows-sys 0.59.0", ] [[package]] @@ -1069,6 +1118,12 @@ version = "0.3.8" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e320a6c5ad31d271ad523dcf3ad13e2767ad8b1cb8f047f75a8aeaf8da139da2" +[[package]] +name = "siphasher" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8ee5873ec9cce0195efcb7a4e9507a04cd49aec9c83d0389df45b1ef7ba2e649" + [[package]] name = "smallvec" version = "1.15.1" diff --git a/Cargo.toml b/Cargo.toml index d945b7341d..89df5c6e2b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,14 @@ categories = ["command-line-utilities", "development-tools"] [dependencies] clap = { version = "4", features = ["derive"] } anyhow = "1.0" +# Console code page decoding for child-process output. Kept off the +# cfg(windows) target so the mapping and the incremental UTF-8 walk stay +# compiled — and unit-tested — on every platform; only the code page *lookup* +# is Windows-specific. codepage covers the ANSI/DBCS pages encoding_rs +# implements, oem_cp the legacy OEM/DOS pages (437, 850, …) it does not. +encoding_rs = "0.8" +codepage = "0.1" +oem_cp = "2" ignore = "0.4" walkdir = "2" regex = "1" @@ -37,6 +45,12 @@ automod = "1" [target.'cfg(unix)'.dependencies] libc = "0.2" +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.59", features = [ + "Win32_System_Console", + "Win32_Globalization", +] } + [build-dependencies] toml = "0.8" diff --git a/scripts/benchmark.sh b/scripts/benchmark.sh index dbacaa9ed3..ffc7301426 100755 --- a/scripts/benchmark.sh +++ b/scripts/benchmark.sh @@ -13,9 +13,15 @@ fi BENCH_DIR="$(pwd)/scripts/benchmark" RTK_ROOT="$(pwd)" +# Only the gitignored output subdirectories get wiped between local runs. +# `$BENCH_DIR` itself is tracked — it also holds the TypeScript VM-benchmark +# harness (run.ts, cleanup.ts, lib/) — so `rm -rf "$BENCH_DIR"` deleted that +# harness from the working tree on every local run. if [ -z "$CI" ]; then - rm -rf "$BENCH_DIR" - mkdir -p "$BENCH_DIR/unix" "$BENCH_DIR/rtk" "$BENCH_DIR/diff" + for bench_output_dir in unix rtk diff; do + rm -rf "${BENCH_DIR:?}/$bench_output_dir" + mkdir -p "$BENCH_DIR/$bench_output_dir" + done fi safe_name() { @@ -342,21 +348,98 @@ section "wc" bench "wc" "wc Cargo.toml src/main.rs" "$RTK wc Cargo.toml src/main.rs" # =================== -# curl -# =================== +# curl / wget — fully offline. mockhttp.org was both a network dependency and +# non-deterministic (its /json output is random), which made the benchmark flaky. +# Serve fixed fixtures locally instead. The JSON is pretty-printed on purpose so +# `rtk` actually exercises JSON minification (a pre-minified body leaves nothing +# to compact). +# =================== +NET_FIXTURE_DIR="$(mktemp -d)" +# Server log lives outside the served directory so it is never itself served. +NET_HTTP_LOG="$(mktemp)" +NET_HTTP_PID="" +# Every step is failure-tolerant: this runs from an EXIT trap under `set -e`, so +# a single failing command (e.g. `kill` on a server that already died) would +# otherwise abort the trap and leak the fixture dir and downloaded files. +cleanup_net_fixtures() { + if [ -n "$NET_HTTP_PID" ]; then + kill "$NET_HTTP_PID" 2>/dev/null || true + fi + rm -rf "$NET_FIXTURE_DIR" "$NET_HTTP_LOG" || true + # `rtk wget /data.json` saves to ./data.json (default basename); remove + # that download and any numbered duplicates from repeated runs. + rm -f data.json data.json.* 2>/dev/null || true +} +trap cleanup_net_fixtures EXIT + +cat > "$NET_FIXTURE_DIR/data.json" << 'JSONEOF' +{ + "message": "Hello from RTK benchmark", + "status": "success", + "code": 200, + "items": [ + { "id": 1, "name": "first" }, + { "id": 2, "name": "second" } + ] +} +JSONEOF +cat > "$NET_FIXTURE_DIR/robots.txt" << 'TXTEOF' +User-agent: * +Disallow: /private/ +Allow: / +Sitemap: https://example.com/sitemap.xml +TXTEOF + +# Bring up a loopback HTTP server once so both curl and wget get real response +# headers (`Content-Type: application/json`) — that's what lets rtk detect JSON +# and minify it. file:// carries no headers, so it can't demonstrate that path. +# +# Port 0 asks the kernel for a free port, so a busy fixed port can never make the +# benchmark fail. `python3 -u` keeps stdout unbuffered, so the "Serving HTTP on +# 127.0.0.1 port NNNNN" line — printed only once the socket is bound and +# listening — reaches the log as soon as the server is ready. +readonly NET_HTTP_EPHEMERAL_PORT=0 +readonly NET_HTTP_READY_ATTEMPTS=25 +readonly NET_HTTP_READY_DELAY=0.2 +NET_HTTP_URL="" +if command -v python3 &> /dev/null; then + ( cd "$NET_FIXTURE_DIR" \ + && exec python3 -u -m http.server "$NET_HTTP_EPHEMERAL_PORT" --bind 127.0.0.1 ) \ + > "$NET_HTTP_LOG" 2>&1 & + NET_HTTP_PID=$! + for _ in $(seq 1 "$NET_HTTP_READY_ATTEMPTS"); do + net_http_port="$(sed -n 's/.*port \([0-9][0-9]*\).*/\1/p' "$NET_HTTP_LOG" | head -1)" + if [ -n "$net_http_port" ]; then + NET_HTTP_URL="http://127.0.0.1:$net_http_port" + break + fi + sleep "$NET_HTTP_READY_DELAY" + done +fi + section "curl" if command -v curl &> /dev/null; then - bench "curl json" "curl -s https://mockhttp.org/json/1" "$RTK curl https://mockhttp.org/json/1" - bench "curl text" "curl -s https://mockhttp.org/robots.txt" "$RTK curl https://mockhttp.org/robots.txt" + if [ -n "$NET_HTTP_URL" ]; then + bench "curl json" "curl -s $NET_HTTP_URL/data.json" "$RTK curl $NET_HTTP_URL/data.json" + bench "curl text" "curl -s $NET_HTTP_URL/robots.txt" "$RTK curl $NET_HTTP_URL/robots.txt" + else + # No python3 to host a local server — fall back to file:// (no headers, so + # no JSON minification, but still fully offline and deterministic). + bench "curl json" "curl -s file://$NET_FIXTURE_DIR/data.json" "$RTK curl file://$NET_FIXTURE_DIR/data.json" + bench "curl text" "curl -s file://$NET_FIXTURE_DIR/robots.txt" "$RTK curl file://$NET_FIXTURE_DIR/robots.txt" + fi fi -# =================== -# wget -# =================== +# wget has no offline fallback: it rejects file:// outright ("Unsupported +# scheme"), so without the loopback server there is nothing local to fetch. Say +# so explicitly instead of letting the case disappear from the report. if command -v wget &> /dev/null; then section "wget" - bench "wget" "wget -qO- https://mockhttp.org/json/1" "$RTK wget https://mockhttp.org/json/1" - rm -f 1 2>/dev/null + if [ -n "$NET_HTTP_URL" ]; then + bench "wget" "wget -qO- $NET_HTTP_URL/data.json" "$RTK wget $NET_HTTP_URL/data.json" + else + echo "⏭️ wget (no local HTTP server available, skipped)" + fi fi # =================== diff --git a/src/cmds/cloud/aws_cmd.rs b/src/cmds/cloud/aws_cmd.rs index a96d2c81a9..c1250ad3b7 100644 --- a/src/cmds/cloud/aws_cmd.rs +++ b/src/cmds/cloud/aws_cmd.rs @@ -4,11 +4,12 @@ //! Specialized filters for high-frequency commands (STS, S3, EC2, ECS, RDS, CloudFormation). use crate::core::guard::never_worse; +use crate::core::stream::{exec_capture, CaptureResult}; use crate::core::tee::force_tee_hint; use crate::core::tracking; use crate::core::truncate::{CAP_INVENTORY, CAP_LIST}; use crate::core::utils::{ - exit_code_from_output, exit_code_from_status, human_bytes, join_with_overflow, + human_bytes, join_with_overflow, resolved_command, shorten_arn, truncate_iso_date, }; use crate::json_cmd; @@ -242,11 +243,13 @@ fn run_generic(subcommand: &str, args: &[String], verbose: u8, full_sub: &str) - eprintln!("Running: aws {}", full_sub); } - let output = cmd.output().context("Failed to run aws CLI")?; - let raw = String::from_utf8_lossy(&output.stdout).to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + let CaptureResult { + stdout: raw, + stderr, + exit_code, + } = exec_capture(&mut cmd).context("Failed to run aws CLI")?; - if !output.status.success() { + if exit_code != 0 { timer.track( &format!("aws {}", full_sub), &format!("rtk aws {}", full_sub), @@ -254,7 +257,7 @@ fn run_generic(subcommand: &str, args: &[String], verbose: u8, full_sub: &str) - &stderr, ); eprintln!("{}", stderr.trim()); - return Ok(crate::core::utils::exit_code_from_output(&output, "aws")); + return Ok(exit_code); } let filtered = match json_cmd::filter_json_compact(&raw, JSON_COMPRESS_DEPTH) { @@ -280,11 +283,7 @@ fn run_generic(subcommand: &str, args: &[String], verbose: u8, full_sub: &str) - Ok(0) } -fn run_aws_json( - sub_args: &[&str], - extra_args: &[String], - verbose: u8, -) -> Result<(String, String, std::process::ExitStatus)> { +fn run_aws_json(sub_args: &[&str], extra_args: &[String], verbose: u8) -> Result { let mut cmd = resolved_command("aws"); for arg in sub_args { cmd.arg(arg); @@ -313,17 +312,13 @@ fn run_aws_json( eprintln!("Running: {}", cmd_desc); } - let output = cmd - .output() - .context(format!("Failed to run {}", cmd_desc))?; - let stdout = String::from_utf8_lossy(&output.stdout).to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + let captured = exec_capture(&mut cmd).context(format!("Failed to run {}", cmd_desc))?; - if !output.status.success() { - eprintln!("{}", stderr.trim()); + if !captured.success() { + eprintln!("{}", captured.stderr.trim()); } - Ok((stdout, stderr, output.status)) + Ok(captured) } /// Shared runner for AWS commands that return JSON. @@ -338,7 +333,11 @@ fn run_aws_filtered( let rtk_label = format!("rtk {}", cmd_label); let slug = cmd_label.replace(' ', "_"); let timer = tracking::TimedExecution::start(); - let (stdout, stderr, status) = run_aws_json(sub_args, extra_args, verbose)?; + let CaptureResult { + stdout, + stderr, + exit_code, + } = run_aws_json(sub_args, extra_args, verbose)?; // Combine stdout+stderr for accurate tracking (per contract) let raw = if stderr.is_empty() { @@ -347,8 +346,7 @@ fn run_aws_filtered( format!("{}\n{}", stdout, stderr) }; - if !status.success() { - let exit_code = exit_code_from_status(&status, "aws"); + if exit_code != 0 { if let Some(hint) = crate::core::tee::tee_and_hint(&raw, &slug, exit_code) { eprintln!("{}\n{}", stderr.trim(), hint); } else { @@ -387,16 +385,17 @@ fn run_s3_ls(extra_args: &[String], verbose: u8) -> Result { eprintln!("Running: aws s3 ls {}", extra_args.join(" ")); } - let output = cmd.output().context("Failed to run aws s3 ls")?; - let stdout = String::from_utf8_lossy(&output.stdout).to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + let CaptureResult { + stdout, + stderr, + exit_code, + } = exec_capture(&mut cmd).context("Failed to run aws s3 ls")?; let raw = if stderr.is_empty() { stdout.clone() } else { format!("{}\n{}", stdout, stderr) }; - if !output.status.success() { - let exit_code = exit_code_from_output(&output, "aws"); + if exit_code != 0 { if let Some(hint) = crate::core::tee::tee_and_hint(&raw, "aws_s3_ls", exit_code) { eprintln!("{}\n{}", stderr.trim(), hint); } else { @@ -435,18 +434,17 @@ fn run_s3_transfer(operation: &str, extra_args: &[String], verbose: u8) -> Resul eprintln!("Running: {} {}", cmd_label, extra_args.join(" ")); } - let output = cmd - .output() - .context(format!("Failed to run {}", cmd_label))?; - let stdout = String::from_utf8_lossy(&output.stdout).to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + let CaptureResult { + stdout, + stderr, + exit_code, + } = exec_capture(&mut cmd).context(format!("Failed to run {}", cmd_label))?; let raw = if stderr.is_empty() { stdout.clone() } else { format!("{}\n{}", stdout, stderr) }; - if !output.status.success() { - let exit_code = exit_code_from_output(&output, "aws"); + if exit_code != 0 { if let Some(hint) = crate::core::tee::tee_and_hint(&raw, &slug, exit_code) { eprintln!("{}\n{}", stderr.trim(), hint); } else { diff --git a/src/cmds/cloud/curl_cmd.rs b/src/cmds/cloud/curl_cmd.rs index 85f242f483..8b68181fd4 100644 --- a/src/cmds/cloud/curl_cmd.rs +++ b/src/cmds/cloud/curl_cmd.rs @@ -43,7 +43,9 @@ pub fn run(args: &[String], verbose: u8) -> Result { // Skip filtering on failure: curl can return HTML error bodies that would // be misleading to summarize, and we want the real exit code surfaced. if !output.status.success() { - let stderr_str = String::from_utf8_lossy(&output.stderr); + // stderr is curl's own diagnostics, which do follow the console code + // page. The body on stdout does not — see the note below. + let stderr_str = crate::core::utils::decode_process_output(&output.stderr); let stdout_str = String::from_utf8_lossy(&output.stdout); let msg = if stderr_str.trim().is_empty() { stdout_str.trim().to_string() @@ -72,6 +74,11 @@ pub fn run(args: &[String], verbose: u8) -> Result { return Ok(exit_code); } + // Deliberately not `decode_process_output`: a response body is a network + // payload whose encoding comes from the HTTP charset, not from the local + // console code page, so decoding it as GBK/CP850 would only ever be right + // by accident. Anything that is not valid UTF-8 already took the raw + // binary passthrough above, so this conversion is lossless in practice. let raw = String::from_utf8_lossy(&output.stdout).into_owned(); let is_tty = std::io::stdout().is_terminal(); let filtered = filter_curl_output(&raw, is_tty); diff --git a/src/cmds/git/git.rs b/src/cmds/git/git.rs index 9936a061d8..6fac0b8548 100644 --- a/src/cmds/git/git.rs +++ b/src/cmds/git/git.rs @@ -4,17 +4,17 @@ use crate::core::args_utils; use crate::core::guard::never_worse; use crate::core::runner::{self, RunOptions}; use crate::core::stream::{ - self, exec_capture, CaptureResult, FilterMode, LineHandler, LineStreamFilter, StdinMode, + self, exec_capture, exec_capture_stdin, CaptureResult, FilterMode, LineHandler, + LineStreamFilter, StdinMode, }; use crate::core::tracking; use crate::core::truncate::{CAP_LIST, CAP_WARNINGS}; use crate::core::utils::{ - exit_code_from_output, exit_code_from_status, join_with_overflow, resolved_command, strip_ansi, + exit_code_from_status, join_with_overflow, resolved_command, strip_ansi, }; use anyhow::{Context, Result}; use std::ffi::OsString; use std::process::Command; -use std::process::Stdio; #[derive(Debug, Clone)] pub enum GitCommand { @@ -431,20 +431,40 @@ fn run_log( verbose: u8, global_args: &[String], ) -> Result { + // Re-insert `--` when clap's trailing_var_arg consumed it (issue #1215): + // without this, `rtk git log -- -p` loses its literal "--" and + // `requests_raw_log_output`/`log_arg_tokens` can no longer tell that + // `-p` is a pathspec, not the real patch flag. + let args = &args_utils::restore_double_dash(args); + + if requests_raw_log_output(args) { + let passthrough_args: Vec = std::iter::once(OsString::from("log")) + .chain(args.iter().map(OsString::from)) + .collect(); + return run_passthrough(&passthrough_args, global_args, verbose); + } + let timer = tracking::TimedExecution::start(); let mut cmd = git_cmd(global_args); cmd.arg("log"); + // Tokenize once and share it: flag-vs-value classification is reused + // below by both the flag-presence checks and the limit parsing, and a + // value belonging to --grep/--author/etc. (e.g. `--grep --pretty`) must + // not be misread as one of the flags below. + let tokens = log_arg_tokens(args); + let flag_args = flag_args_from_tokens(&tokens); + // Check if user provided format flags - let has_format_flag = args.iter().any(|arg| { + let has_format_flag = flag_args.iter().any(|arg| { arg.starts_with("--oneline") || arg.starts_with("--pretty") || arg.starts_with("--format") }); // Check if user provided limit flag (-N, -n N, --max-count=N, --max-count N) - let has_limit_flag = args.iter().any(|arg| { + let has_limit_flag = flag_args.iter().any(|arg| { (arg.starts_with('-') && arg.chars().nth(1).is_some_and(|c| c.is_ascii_digit())) - || arg == "-n" + || *arg == "-n" || arg.starts_with("--max-count") }); @@ -458,7 +478,7 @@ fn run_log( // Determine limit: respect user's explicit -N flag, use sensible defaults otherwise let (limit, user_set_limit) = if has_limit_flag { // User explicitly passed -N / -n N / --max-count=N → respect their choice - let n = parse_user_limit(args).unwrap_or(10); + let n = parse_limit_from_tokens(&tokens).unwrap_or(10); (n, true) } else if has_format_flag { // --oneline / --pretty without -N: user wants compact output, allow more @@ -471,9 +491,9 @@ fn run_log( }; // Only add --no-merges if user didn't explicitly request merge commits - let wants_merges = args + let wants_merges = flag_args .iter() - .any(|arg| arg == "--merges" || arg == "--min-parents=2" || arg == "--no-merges"); + .any(|arg| *arg == "--merges" || *arg == "--min-parents=2" || *arg == "--no-merges"); // Don't add --no-merges if user explicitly requested merges or an exact count (-n N / --max-count) if !wants_merges && !has_limit_flag { cmd.arg("--no-merges"); @@ -510,42 +530,186 @@ fn run_log( Ok(0) } -/// Filter git log output: truncate long messages, cap lines -/// Parse the user-specified limit from git log args. -/// Handles: -20, -n 20, --max-count=20, --max-count 20 -fn parse_user_limit(args: &[String]) -> Option { - let mut iter = args.iter(); +/// True for git log/diff options that take their value as a separate, +/// space-delimited token (e.g. `--grep -p` searches messages for the +/// literal string "-p"; it does not request patch output). Consuming +/// that value token keeps flag-lookalike values from being misread as +/// the corresponding boolean flag. +fn consumes_next_token_as_value(arg: &str) -> bool { + matches!( + arg, + "--after" + | "--anchored" + | "--author" + | "--before" + | "--color-moved-ws" + | "--committer" + | "--date" + | "--decorate-refs" + | "--decorate-refs-exclude" + | "--diff-algorithm" + | "--diff-filter" + | "--diff-merges" + | "--dst-prefix" + | "--encoding" + | "--exclude" + | "--find-object" + | "--glob" + | "--grep" + | "--grep-reflog" + | "--inter-hunk-context" + | "--line-prefix" + | "--max-depth" + | "--output" + | "--output-indicator-context" + | "--output-indicator-new" + | "--output-indicator-old" + | "--rotate-to" + | "--since" + | "--since-as-filter" + | "--skip" + | "--skip-to" + | "--src-prefix" + | "--stat-count" + | "--stat-name-width" + | "--stat-width" + | "--until" + | "--word-diff-regex" + | "--ws-error-highlight" + | "-G" + | "-I" + | "-L" + | "-O" + | "-S" + | "-l" + | "-n" + ) +} + +/// A git log argument, classified as either a flag or the value consumed +/// by the preceding flag. +enum LogArg<'a> { + Flag(&'a str), + Value { flag: &'a str, value: &'a str }, +} + +/// Tokenizes git log `args` into [`LogArg`]s, stopping at the `--` pathspec +/// separator (tokens after it are paths, never flags or their values — +/// e.g. `git log -- -5` means "history for the path literally named -5"). +/// `-n`/`--max-count`'s own count and every option in +/// [`consumes_next_token_as_value`] are paired with the flag that consumes +/// them. Shared by every git-log flag/value/limit check in [`run_log`] so +/// `--`-handling and option-value handling live in one place instead of +/// being reimplemented per check. +fn log_arg_tokens(args: &[String]) -> Vec> { + let mut tokens = Vec::with_capacity(args.len()); + let mut iter = args.iter().take_while(|arg| *arg != "--"); while let Some(arg) = iter.next() { - // -20 (combined digit form) - if arg.starts_with('-') - && arg.len() > 1 - && arg.chars().nth(1).is_some_and(|c| c.is_ascii_digit()) - { - if let Ok(n) = arg[1..].parse::() { - return Some(n); + let arg_str = arg.as_str(); + if arg_str == "--max-count" || consumes_next_token_as_value(arg_str) { + if let Some(value) = iter.next() { + tokens.push(LogArg::Value { + flag: arg_str, + value: value.as_str(), + }); + continue; } } - // -n 20 (two-token form) - if arg == "-n" { - if let Some(next) = iter.next() { - if let Ok(n) = next.parse::() { + tokens.push(LogArg::Flag(arg_str)); + } + tokens +} + +/// Filters `tokens` down to the flags themselves, dropping every value +/// consumed by the preceding option. +fn flag_args_from_tokens<'a>(tokens: &[LogArg<'a>]) -> Vec<&'a str> { + tokens + .iter() + .map(|token| match token { + LogArg::Flag(flag) | LogArg::Value { flag, .. } => *flag, + }) + .collect() +} + +/// Filters `args` down to the tokens that are actual flags, dropping every +/// token consumed as a value by the preceding option. `run_log` shares a +/// single tokenization via [`flag_args_from_tokens`] instead; this +/// convenience wrapper exists for tests that only care about the flags. +#[cfg(test)] +fn real_flag_args(args: &[String]) -> Vec<&str> { + flag_args_from_tokens(&log_arg_tokens(args)) +} + +/// True for git log/diff flags that change the *shape* of git's raw output +/// (patch text, diffstat, name lists) in a way RTK's injected +/// `--pretty=format` + `---END---` markers can't coexist with — matching +/// this must request the untouched passthrough path instead of RTK's +/// filtered one (see [`requests_raw_log_output`]). +fn requests_raw_diff_shape(flag: &str) -> bool { + matches!( + flag, + "-p" | "-u" + | "--dirstat" + | "--name-only" + | "--name-status" + | "--numstat" + | "--patch" + | "--patch-with-raw" + | "--patch-with-stat" + | "--raw" + | "--shortstat" + | "--stat" + | "--summary" + ) || flag.starts_with("--stat=") + || flag.starts_with("--dirstat=") +} + +fn requests_raw_log_output(args: &[String]) -> bool { + log_arg_tokens(args) + .iter() + .any(|token| matches!(token, LogArg::Flag(flag) if requests_raw_diff_shape(flag))) +} + +/// Parse the user-specified limit from git log args. +/// Handles: -20, -n 20, --max-count=20, --max-count 20 +/// `run_log` shares a single tokenization via [`parse_limit_from_tokens`] +/// instead; this convenience wrapper exists for tests. +#[cfg(test)] +fn parse_user_limit(args: &[String]) -> Option { + parse_limit_from_tokens(&log_arg_tokens(args)) +} + +fn parse_limit_from_tokens(tokens: &[LogArg<'_>]) -> Option { + for token in tokens { + match token { + // -20 (combined digit form) + LogArg::Flag(flag) + if flag.starts_with('-') + && flag.len() > 1 + && flag.chars().nth(1).is_some_and(|c| c.is_ascii_digit()) => + { + if let Ok(n) = flag[1..].parse::() { return Some(n); } } - } - // --max-count=20 - if let Some(rest) = arg.strip_prefix("--max-count=") { - if let Ok(n) = rest.parse::() { - return Some(n); - } - } - // --max-count 20 (two-token form) - if arg == "--max-count" { - if let Some(next) = iter.next() { - if let Ok(n) = next.parse::() { + // -n 20 / --max-count 20 (two-token form) + LogArg::Value { + flag: "-n" | "--max-count", + value, + } => { + if let Ok(n) = value.parse::() { return Some(n); } } + // --max-count=20 + LogArg::Flag(flag) => { + if let Some(rest) = flag.strip_prefix("--max-count=") { + if let Ok(n) = rest.parse::() { + return Some(n); + } + } + } + LogArg::Value { .. } => {} } } None @@ -1009,15 +1173,24 @@ fn build_commit_command(args: &[String], global_args: &[String]) -> Command { /// Handles: `[main abc1234def] message`, `[main (root-commit) abc1234def] msg`, /// localized variants, and multibyte branch names. fn parse_commit_output(line: &str) -> String { - if let Some(bracket_end) = line.find(']') { - let bracket_content = &line[1..bracket_end]; - let hash = bracket_content.split_whitespace().next_back().unwrap_or(""); - if !hash.is_empty() && hash.len() >= 7 { - let short_hash: String = hash.chars().take(7).collect(); - format!("ok {}", short_hash) - } else { - "ok".to_string() - } + // Locate the summary's own brackets rather than assuming the line starts + // with '['. git prints hook output before its summary, so the first line + // is often something else entirely; slicing from byte 1 panics outright + // when that line opens with a multi-byte character ("✅ lint passed]"), + // and a line decoded from non-UTF-8 bytes starts with a multi-byte U+FFFD. + // Both indices come from `find`, so both land on character boundaries. + let (Some(open), Some(bracket_end)) = (line.find('['), line.find(']')) else { + return "ok".to_string(); + }; + if open >= bracket_end { + return "ok".to_string(); + } + + let bracket_content = &line[open + 1..bracket_end]; + let hash = bracket_content.split_whitespace().next_back().unwrap_or(""); + if hash.chars().count() >= 7 { + let short_hash: String = hash.chars().take(7).collect(); + format!("ok {}", short_hash) } else { "ok".to_string() } @@ -1032,17 +1205,17 @@ fn run_commit(args: &[String], verbose: u8, global_args: &[String]) -> Result { println!("{}", compact); timer.track(&original_cmd, "rtk git commit", &raw_output, &compact); @@ -1117,7 +1290,11 @@ fn format_checkout_output(args: &[String], raw: &str, exit_code: i32) -> String fn format_checkout_success(args: &[String], raw: &str) -> String { if let Some(restored) = checkout_restored_count(args) { - return format!("ok {} {}", restored, pluralize(restored, "file restored", "files restored")); + return format!( + "ok {} {}", + restored, + pluralize(restored, "file restored", "files restored") + ); } if let Some(branch) = checkout_reset_branch_arg(args) { return format!("ok {}", branch); @@ -1215,7 +1392,11 @@ fn quoted_suffix<'a>(line: &'a str, prefix: &str) -> Option<&'a str> { } fn pluralize<'a>(count: usize, singular: &'a str, plural: &'a str) -> &'a str { - if count == 1 { singular } else { plural } + if count == 1 { + singular + } else { + plural + } } fn filter_checkout_failure(raw: &str) -> String { @@ -1233,8 +1414,9 @@ fn filter_checkout_failure(raw: &str) -> String { || trimmed.starts_with("CONFLICT"); if is_header { - in_file_list = - trimmed.contains("following") && trimmed.contains("files") && trimmed.ends_with(':'); + in_file_list = trimmed.contains("following") + && trimmed.contains("files") + && trimmed.ends_with(':'); important.push(trimmed.to_string()); continue; } @@ -1768,7 +1950,12 @@ fn run_stash( compact_stash_stat(&result.stdout) }; let shown = crate::core::runner::emit_guarded(&filtered, None, &result.stdout); - timer.track("git stash show", "rtk git stash show", &result.stdout, &shown); + timer.track( + "git stash show", + "rtk git stash show", + &result.stdout, + &shown, + ); } Some("apply") | Some("branch") | Some("clear") | Some("create") | Some("drop") | Some("export") | Some("import") | Some("pop") | Some("store") => { @@ -1902,7 +2089,7 @@ fn compress_stat_summary(summary: &str) -> String { .replace("deletion(-)", "-") .replace("files changed", "changed") .replace("file changed", "changed") - .replace(",", "") + .replace(",", "") } fn parse_stash_stat(stat: &str) -> (Vec, String) { @@ -2358,7 +2545,12 @@ mod tests { let (files, summary) = parse_stash_stat(raw); assert_eq!( files, - vec!["del.md 2 -", "keep.md 5 +-", "logo.bin (binary)", "new.rs 40 +"] + vec![ + "del.md 2 -", + "keep.md 5 +-", + "logo.bin (binary)", + "new.rs 40 +" + ] ); assert_eq!(summary, "4 files changed, 44 insertions(+), 3 deletions(-)"); } @@ -2372,7 +2564,10 @@ mod tests { #[test] fn test_compact_stash_stat_passthrough_numstat() { let raw = "0\t1\tdel.md\n3\t2\tkeep.md\n1\t0\tn1.rs\n"; - assert_eq!(compact_stash_stat(raw), "0\t1\tdel.md\n3\t2\tkeep.md\n1\t0\tn1.rs"); + assert_eq!( + compact_stash_stat(raw), + "0\t1\tdel.md\n3\t2\tkeep.md\n1\t0\tn1.rs" + ); } #[test] @@ -2446,7 +2641,11 @@ mod tests { let compact = format!("{}\n{}", files.join("\n"), summary); let savings = 100.0 - (estimate_tokens(&compact) as f64 / estimate_tokens(raw) as f64 * 100.0); - assert!(savings >= 40.0, "expected >=40% savings, got {:.1}%", savings); + assert!( + savings >= 40.0, + "expected >=40% savings, got {:.1}%", + savings + ); } #[test] @@ -2731,6 +2930,226 @@ A added.rs assert_eq!(parse_user_limit(&args), None); } + #[test] + fn test_patch_log_flags_request_raw_output() { + for flag in [ + "-p", + "-u", + "--patch", + "--patch-with-raw", + "--patch-with-stat", + ] { + let args = vec![flag.to_string()]; + assert!(requests_raw_log_output(&args), "{flag} should pass through"); + } + } + + #[test] + fn test_patch_flag_after_pathspec_separator_is_ignored() { + // `git log -- -p` means "show history for a path literally named -p", + // not "show patches" — the flag lookalike appears after `--`. + let args = vec!["--".to_string(), "-p".to_string()]; + assert!( + !requests_raw_log_output(&args), + "-p after -- is a pathspec, not a patch flag, and should stay on the filtered path" + ); + } + + #[test] + fn test_non_patch_log_flags_remain_filtered() { + for flag in ["--no-patch", "--oneline", "--format=%H"] { + let args = vec![flag.to_string()]; + assert!( + !requests_raw_log_output(&args), + "{flag} should remain on the filtered log path" + ); + } + } + + #[test] + fn test_diff_shape_flags_request_raw_output() { + // These change the shape of git's raw output (diffstat, name lists) + // the same way -p does — RTK's injected --pretty=format markers + // can't coexist with them, so they must stay on the raw path too. + for flag in [ + "--dirstat", + "--dirstat=files", + "--name-only", + "--name-status", + "--numstat", + "--raw", + "--shortstat", + "--stat", + "--stat=80", + "--summary", + ] { + let args = vec![flag.to_string()]; + assert!( + requests_raw_log_output(&args), + "{flag} changes output shape and should request raw output" + ); + } + } + + #[test] + fn test_diff_shape_flag_as_value_of_grep_is_not_misdetected() { + // `git log --grep --stat` searches for the literal string + // "--stat"; git consumes it as --grep's value, not the --stat flag. + let args = vec!["--grep".to_string(), "--stat".to_string()]; + assert!( + !requests_raw_log_output(&args), + "--stat as the value of --grep should stay on the filtered path" + ); + } + + #[test] + fn test_patch_flag_as_value_of_grep_is_not_misdetected() { + // `git log --grep -p` searches commit messages for the literal + // string "-p"; git does not treat it as the patch flag. + for opt in [ + "--author", + "--committer", + "--diff-algorithm", + "--diff-filter", + "--grep", + "-G", + "-S", + ] { + let args = vec![opt.to_string(), "-p".to_string()]; + assert!( + !requests_raw_log_output(&args), + "-p as the value of {opt} should stay on the filtered path" + ); + } + } + + #[test] + fn test_patch_flag_still_detected_after_value_taking_option() { + // The value-taking option consumes only its own value token; + // a genuine -p later in the args still triggers the raw path. + let args = vec!["--grep".to_string(), "fix".to_string(), "-p".to_string()]; + assert!( + requests_raw_log_output(&args), + "a real -p after --grep's value should still request raw output" + ); + } + + #[test] + fn test_optional_value_options_do_not_consume_next_token() { + // These options only take an attached value (-U3, --unified=3, + // --expand-tabs=4, --max-parents=2); a bare separate token after + // them is not their value, so it must not be swallowed. Confirmed + // against git 2.53.0: e.g. `git log --expand-tabs 4` fails with + // "fatal: ambiguous argument '4'" rather than treating 4 as the + // option's value. + for opt in [ + "-U", + "--unified", + "--expand-tabs", + "--max-parents", + "--min-parents", + ] { + let args = vec![opt.to_string(), "-p".to_string()]; + assert!( + requests_raw_log_output(&args), + "a real -p after {opt} should still request raw output" + ); + } + } + + #[test] + fn test_real_flag_args_drops_value_taking_option_values() { + // `--grep`'s value is not itself a flag and must not appear in the + // filtered set, even when it looks like -N, --pretty, or --merges. + let args = vec!["--grep".to_string(), "-5".to_string()]; + assert_eq!(real_flag_args(&args), vec!["--grep"]); + } + + #[test] + fn test_real_flag_args_keeps_limit_flag_drops_its_value() { + let args = vec!["-n".to_string(), "15".to_string()]; + assert_eq!(real_flag_args(&args), vec!["-n"]); + + let args = vec!["--max-count".to_string(), "25".to_string()]; + assert_eq!(real_flag_args(&args), vec!["--max-count"]); + } + + #[test] + fn test_real_flag_args_keeps_genuine_flags() { + let args = vec!["--grep".to_string(), "fix".to_string(), "--oneline".to_string()]; + assert_eq!(real_flag_args(&args), vec!["--grep", "--oneline"]); + } + + #[test] + fn test_grep_value_looking_like_limit_flag_is_not_misdetected() { + // `git log --grep -5` searches commit messages for the literal + // string "-5"; it is not a request to limit output to 5 commits. + let args = vec!["--grep".to_string(), "-5".to_string()]; + assert!( + !real_flag_args(&args) + .iter() + .any(|arg| arg.starts_with('-') && arg.chars().nth(1).is_some_and(|c| c.is_ascii_digit())), + "-5 as the value of --grep should not be seen as a limit flag" + ); + assert_eq!( + parse_user_limit(&args), + None, + "-5 as the value of --grep should not be parsed as a limit" + ); + } + + #[test] + fn test_grep_value_looking_like_format_flag_is_not_misdetected() { + // `git log --grep --pretty` searches for the literal string + // "--pretty"; git consumes it as --grep's value, not a format flag. + let args = vec!["--grep".to_string(), "--pretty".to_string()]; + assert!( + !real_flag_args(&args) + .iter() + .any(|arg| arg.starts_with("--pretty")), + "--pretty as the value of --grep should not be seen as a format flag" + ); + } + + #[test] + fn test_grep_value_looking_like_merges_flag_is_not_misdetected() { + // `git log --grep --merges` searches for the literal string + // "--merges"; git consumes it as --grep's value, not --merges. + let args = vec!["--grep".to_string(), "--merges".to_string()]; + assert!( + !real_flag_args(&args).contains(&"--merges"), + "--merges as the value of --grep should not be seen as the --merges flag" + ); + } + + #[test] + fn test_parse_user_limit_skips_foreign_option_values() { + // A real limit later in the args is still found after a + // value-taking option's value is skipped. + let args = vec![ + "--grep".to_string(), + "-5".to_string(), + "-20".to_string(), + ]; + assert_eq!(parse_user_limit(&args), Some(20)); + } + + #[test] + fn test_log_arg_tokens_stop_at_pathspec_separator() { + // `git log -- -5` means "history for the path literally named -5", + // not a limit flag — tokens after `--` must be ignored entirely. + let args = vec!["--".to_string(), "-5".to_string()]; + assert!( + real_flag_args(&args).is_empty(), + "-5 after -- is a pathspec, not a flag" + ); + assert_eq!( + parse_user_limit(&args), + None, + "-5 after -- should not be parsed as a limit" + ); + } + #[test] fn test_filter_log_output_token_savings() { fn count_tokens(text: &str) -> usize { @@ -2851,6 +3270,37 @@ no changes added to commit (use "git add" and/or "git commit -a") assert_eq!(parse_commit_output(line), "ok abc1234"); } + /// Regression: git prints hook output before its own summary. A first line + /// that opens with a multi-byte character and contains ']' used to panic on + /// `line[1..]` ("byte index 1 is not a char boundary"). + #[test] + fn test_parse_commit_output_multibyte_prefix_does_not_panic() { + assert_eq!(parse_commit_output("✅ lint passed]"), "ok"); + assert_eq!(parse_commit_output("→ hook] done"), "ok"); + } + + /// The same shape as above, but with a real summary after the hook text — + /// the hash must still be found via the bracket pair. + #[test] + fn test_parse_commit_output_after_multibyte_hook_prefix() { + assert_eq!( + parse_commit_output("✅ [main abc1234def] add feature"), + "ok abc1234" + ); + } + + /// A U+FFFD from lossily decoded output is itself multi-byte. + #[test] + fn test_parse_commit_output_replacement_char_prefix() { + assert_eq!(parse_commit_output("\u{FFFD}oops]"), "ok"); + } + + /// A closing bracket before any opening one must not slice backwards. + #[test] + fn test_parse_commit_output_close_before_open() { + assert_eq!(parse_commit_output("] stray [main abc1234def]"), "ok"); + } + #[test] fn test_parse_commit_output_no_bracket() { let line = "some other output"; diff --git a/src/cmds/go/go_cmd.rs b/src/cmds/go/go_cmd.rs index d35399bbcc..6d8c23a032 100644 --- a/src/cmds/go/go_cmd.rs +++ b/src/cmds/go/go_cmd.rs @@ -2,9 +2,10 @@ use crate::core::guard::never_worse; use crate::core::runner; +use crate::core::stream::{exec_capture, CaptureResult}; use crate::core::tracking; use crate::core::truncate::CAP_ERRORS; -use crate::core::utils::{exit_code_from_output, resolved_command, truncate}; +use crate::core::utils::{resolved_command, truncate}; use crate::golangci_cmd; use anyhow::{Context, Result}; use serde::Deserialize; @@ -149,16 +150,12 @@ pub fn run_other(args: &[OsString], verbose: u8) -> Result { eprintln!("Running: go {} ...", subcommand); } - let output = cmd - .output() + let captured = exec_capture(&mut cmd) .with_context(|| format!("Failed to run go {}", subcommand))?; + let raw = format!("{}\n{}", captured.stdout, captured.stderr); - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); - let raw = format!("{}\n{}", stdout, stderr); - - print!("{}", stdout); - eprint!("{}", stderr); + print!("{}", captured.stdout); + eprint!("{}", captured.stderr); timer.track( &format!("go {}", subcommand), @@ -167,26 +164,21 @@ pub fn run_other(args: &[OsString], verbose: u8) -> Result { &raw, // No filtering for unsupported commands ); - Ok(exit_code_from_output(&output, "go")) + Ok(captured.exit_code) } /// Detect golangci-lint major version when invoked via `go tool`. /// Returns 1 on any failure (safe fallback — v1 behaviour). fn detect_go_tool_golangci_version() -> u32 { - let output = resolved_command("go") - .arg("tool") - .arg("golangci-lint") - .arg("--version") - .output(); - - match output { - Ok(o) => { - let stdout = String::from_utf8_lossy(&o.stdout); - let stderr = String::from_utf8_lossy(&o.stderr); - let version_text = if stdout.trim().is_empty() { - &*stderr + let mut cmd = resolved_command("go"); + cmd.arg("tool").arg("golangci-lint").arg("--version"); + + match exec_capture(&mut cmd) { + Ok(captured) => { + let version_text = if captured.stdout.trim().is_empty() { + &captured.stderr } else { - &*stdout + &captured.stdout }; golangci_cmd::parse_major_version(version_text) } @@ -265,12 +257,11 @@ fn run_go_tool_golangci_lint(args: &[OsString], verbose: u8) -> Result { } } - let output = cmd - .output() - .context("Failed to run go tool golangci-lint")?; - - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let CaptureResult { + stdout, + stderr, + exit_code, + } = exec_capture(&mut cmd).context("Failed to run go tool golangci-lint")?; let raw = format!("{}\n{}", stdout, stderr); // v2 outputs JSON on first line + trailing text; v1 outputs just JSON @@ -295,7 +286,6 @@ fn run_go_tool_golangci_lint(args: &[OsString], verbose: u8) -> Result { shown, ); - let exit_code = exit_code_from_output(&output, "go tool golangci-lint"); // golangci-lint: exit 0 = clean, exit 1 = lint issues found (not an error), // exit 2+ = config/build error, None = killed by signal (OOM, SIGKILL) Ok(if exit_code == 1 { 0 } else { exit_code }) diff --git a/src/cmds/rust/cargo_cmd.rs b/src/cmds/rust/cargo_cmd.rs index 3fe2f3c8c1..6f0dc373a0 100644 --- a/src/cmds/rust/cargo_cmd.rs +++ b/src/cmds/rust/cargo_cmd.rs @@ -96,13 +96,14 @@ impl BlockHandler for CargoBuildHandler { !(line.trim().is_empty() && block.len() > 3) } - fn format_summary(&self, exit_code: i32, _raw: &str) -> Option { + fn format_summary(&self, exit_code: i32, raw: &str) -> Option { if self.error_count == 0 && self.warnings == 0 && exit_code == 0 { - return Some(cargo_build_success_line( + let summary = cargo_build_success_line( self.compiled, self.finished_line.as_deref(), self.label, - )); + ); + return Some(crate::core::guard::never_worse(raw, &summary).to_string()); } // The streamed path only runs for non-json build/check; error blocks are // emitted live, so the summary carries no rendered diagnostics. @@ -137,6 +138,65 @@ impl CargoTestHandler { has_compile_errors: false, } } + + /// Compacted `cargo test` summary, before the never-worse guard. + fn compute_test_summary(&self, raw: &str) -> Option { + if self.summary_lines.is_empty() { + let json = extract_json_diagnostics(raw); + if self.has_compile_errors || !json.errors.is_empty() { + // Content-based (exit 0): a real compile error yields "cargo test: N + // errors"; a bare "could not compile" leaves the raw tail fallback. + let build_filtered = filter_cargo_build_labeled(raw, "test", 0); + if build_filtered.contains("cargo test:") { + return Some(format!("{}\n", build_filtered)); + } + // Fallback: last 5 meaningful lines + let meaningful: Vec<&str> = raw + .lines() + .filter(|l| !l.trim().is_empty() && !l.trim_start().starts_with("Compiling")) + .collect(); + let last5: Vec<&str> = meaningful.iter().rev().take(5).rev().copied().collect(); + return Some(format!("{}\n", last5.join("\n"))); + } + } + + // No failures emitted — aggregate pass results + let mut aggregated: Option = None; + let mut all_parsed = true; + + for line in &self.summary_lines { + if let Some(parsed) = AggregatedTestResult::parse_line(line) { + if let Some(ref mut agg) = aggregated { + agg.merge(&parsed); + } else { + aggregated = Some(parsed); + } + } else { + all_parsed = false; + break; + } + } + + if all_parsed { + if let Some(agg) = aggregated { + if agg.suites > 0 { + return Some(format!("{}\n", agg.format_compact())); + } + } + } + + // Fallback: show raw summary lines + if !self.summary_lines.is_empty() { + let mut s = String::new(); + for line in &self.summary_lines { + s.push_str(line); + s.push('\n'); + } + return Some(s); + } + + None + } } impl BlockHandler for CargoTestHandler { @@ -195,61 +255,12 @@ impl BlockHandler for CargoTestHandler { } fn format_summary(&self, _exit_code: i32, raw: &str) -> Option { - if self.summary_lines.is_empty() { - let json = extract_json_diagnostics(raw); - if self.has_compile_errors || !json.errors.is_empty() { - // Content-based (exit 0): a real compile error yields "cargo test: N - // errors"; a bare "could not compile" leaves the raw tail fallback. - let build_filtered = filter_cargo_build_labeled(raw, "test", 0); - if build_filtered.contains("cargo test:") { - return Some(format!("{}\n", build_filtered)); - } - // Fallback: last 5 meaningful lines - let meaningful: Vec<&str> = raw - .lines() - .filter(|l| !l.trim().is_empty() && !l.trim_start().starts_with("Compiling")) - .collect(); - let last5: Vec<&str> = meaningful.iter().rev().take(5).rev().copied().collect(); - return Some(format!("{}\n", last5.join("\n"))); - } - } - - // No failures emitted — aggregate pass results - let mut aggregated: Option = None; - let mut all_parsed = true; - - for line in &self.summary_lines { - if let Some(parsed) = AggregatedTestResult::parse_line(line) { - if let Some(ref mut agg) = aggregated { - agg.merge(&parsed); - } else { - aggregated = Some(parsed); - } - } else { - all_parsed = false; - break; - } - } - - if all_parsed { - if let Some(agg) = aggregated { - if agg.suites > 0 { - return Some(format!("{}\n", agg.format_compact())); - } - } - } - - // Fallback: show raw summary lines - if !self.summary_lines.is_empty() { - let mut s = String::new(); - for line in &self.summary_lines { - s.push_str(line); - s.push('\n'); - } - return Some(s); - } - - None + // Same never-worse guard as CargoBuildHandler (#3430 review): if the + // compacted summary ends up larger than the raw output (e.g. a tiny + // `cargo test` run), keep the raw output instead of "compacting" it + // into something bigger. + let summary = self.compute_test_summary(raw)?; + Some(crate::core::guard::never_worse(raw, &summary).to_string()) } } @@ -988,7 +999,8 @@ fn filter_cargo_build_labeled(output: &str, label: &'static str, exit_code: i32) let (errors, warnings) = merge_diag_counts(handler.error_count, handler.warnings, &json); if errors == 0 && warnings == 0 && exit_code == 0 { - return cargo_build_success_line(handler.compiled, handler.finished_line.as_deref(), label); + let summary = cargo_build_success_line(handler.compiled, handler.finished_line.as_deref(), label); + return crate::core::guard::never_worse(output, &summary).to_string(); } let mut result = @@ -1437,6 +1449,9 @@ pub fn run_passthrough(args: &[OsString], verbose: u8) -> Result { #[cfg(test)] mod tests { + const TINY_BUILD_SUCCESS_OUTPUT: &str = + " Finished dev [unoptimized + debuginfo] target(s) in 0.01s\n"; + use super::*; use crate::core::args_utils::restore_double_dash_with_raw; @@ -1557,6 +1572,42 @@ mod tests { assert!(result.contains("3 crates compiled")); } + #[test] + fn test_filter_cargo_build_success_uses_raw_when_summary_is_larger() { + let output = TINY_BUILD_SUCCESS_OUTPUT; + let result = filter_cargo_build(output); + assert_eq!(result, output); + } + + #[test] + fn test_cargo_test_summary_uses_raw_when_summary_is_larger() { + // A one-line compile failure is already minimal: prefixing it with the + // "cargo test: N errors, ..." header emits more tokens than the raw + // output, so the never-worse guard must keep the raw output. + const CARGO_COMPILE_FAILURE_EXIT_CODE: i32 = 101; + let raw = "error[E0433]: failed to resolve: use of undeclared type `Foo`\n"; + + let mut handler = CargoTestHandler::new(); + for line in raw.lines() { + handler.should_skip(line); + } + + let unguarded = handler + .compute_test_summary(raw) + .expect("compile failure yields a summary"); + assert!( + unguarded.len() > raw.len(), + "expected the compacted summary to be larger than the raw output, got {:?}", + unguarded + ); + assert_eq!( + handler + .format_summary(CARGO_COMPILE_FAILURE_EXIT_CODE, raw) + .as_deref(), + Some(raw) + ); + } + #[test] fn test_filter_cargo_build_errors() { let output = r#" Compiling rtk v0.5.0 @@ -2314,6 +2365,14 @@ error: test run failed assert!(!result.contains("Compiling"), "got: {}", result); } + #[test] + fn test_cargo_build_stream_success_uses_raw_when_summary_is_larger() { + let input = TINY_BUILD_SUCCESS_OUTPUT; + let mut f = BlockStreamFilter::new(CargoBuildHandler::with_label("build")); + let result = run_block_filter(&mut f, input, 0); + assert_eq!(result, input); + } + #[test] fn test_cargo_build_stream_json_success() { let input = concat!( diff --git a/src/cmds/system/find_cmd.rs b/src/cmds/system/find_cmd.rs index 0e8d2eebb9..25f5530d7c 100644 --- a/src/cmds/system/find_cmd.rs +++ b/src/cmds/system/find_cmd.rs @@ -1,10 +1,11 @@ //! Filters find results by grouping files by directory. -use crate::core::guard::never_worse; use crate::core::tracking; +use crate::core::truncate::CAP_INVENTORY; use anyhow::{Context, Result}; use ignore::WalkBuilder; use std::collections::HashMap; +use std::io::Write; use std::path::Path; /// Match a filename against a glob pattern (supports `*` and `?`). @@ -32,6 +33,7 @@ struct FindArgs { pattern: String, path: String, max_results: usize, + max_explicit: bool, max_depth: Option, file_type: String, case_insensitive: bool, @@ -42,7 +44,8 @@ impl Default for FindArgs { Self { pattern: "*".to_string(), path: ".".to_string(), - max_results: 50, + max_results: CAP_INVENTORY, + max_explicit: false, max_depth: None, file_type: "f".to_string(), case_insensitive: false, @@ -50,151 +53,350 @@ impl Default for FindArgs { } } -/// Consume the next argument from `args` at position `i`, advancing the index. -/// Returns `None` if `i` is past the end of `args`. -fn next_arg(args: &[String], i: &mut usize) -> Option { - *i += 1; - args.get(*i).cloned() -} - -/// Check if args contain native find flags (-name, -type, -maxdepth, etc.) -fn has_native_find_flags(args: &[String]) -> bool { - args.iter() - .any(|a| a == "-name" || a == "-type" || a == "-maxdepth" || a == "-iname") -} - -/// Native find flags that RTK cannot handle correctly. -/// These involve compound predicates, actions, or semantics we don't support. -const UNSUPPORTED_FIND_FLAGS: &[&str] = &[ - "-not", "!", "-or", "-o", "-and", "-a", "-exec", "-execdir", "-delete", "-print0", "-newer", - "-perm", "-size", "-mtime", "-mmin", "-atime", "-amin", "-ctime", "-cmin", "-empty", "-link", - "-regex", "-iregex", +const VERBATIM_ACTIONS: &[&str] = &[ + "-exec", "-execdir", "-ok", "-okdir", "-delete", "-print", "-print0", "-printf", "-fprint", + "-fprint0", "-fprintf", "-ls", "-fls", ]; -fn has_unsupported_find_flags(args: &[String]) -> bool { - args.iter() - .any(|a| UNSUPPORTED_FIND_FLAGS.contains(&a.as_str())) +enum Dispatch { + Native(FindArgs), + Compress { + options: Vec, + paths: Vec, + expr: Vec, + max: Option, + file_type: Option, + }, + Verbatim(Vec), } -/// Parse arguments from raw args vec, supporting both native find and RTK syntax. -/// -/// Native find syntax: `find . -name "*.rs" -type f -maxdepth 3` -/// RTK syntax: `find *.rs [path] [-m max] [-t type]` -fn parse_find_args(args: &[String]) -> Result { - if args.is_empty() { - return Ok(FindArgs::default()); - } +fn is_expression_token(token: &str) -> bool { + token.starts_with('-') || token == "!" || token == "(" || token == ")" +} - if has_unsupported_find_flags(args) { - anyhow::bail!( - "rtk find does not support compound predicates or actions (e.g. -not, -exec). Use `find` directly." - ); - } +fn has_glob_meta(token: &str) -> bool { + token.contains('*') || token.contains('?') +} - if has_native_find_flags(args) { - parse_native_find_args(args) - } else { - parse_rtk_find_args(args) +/// Leading find options: `-H`, `-L`, `-P`, `-D debugopts`, `-Olevel`. +fn leading_options_len(args: &[String]) -> usize { + let mut i = 0; + while i < args.len() { + match args[i].as_str() { + "-H" | "-L" | "-P" => i += 1, + "-D" if i + 1 < args.len() => i += 2, + t if t.len() > 2 && (t.starts_with("-D") || t.starts_with("-O")) => i += 1, + _ => break, + } } + i.min(args.len()) } -/// Parse native find syntax: `find [path] -name "*.rs" -type f -maxdepth 3` -fn parse_native_find_args(args: &[String]) -> Result { +/// The subset rtk walks itself: one path, each of -name/-iname (globs with * and ? +/// only), -type f|d, -maxdepth N, -m/--max N, -t/--file-type f|d at most once. +fn parse_subset(paths: &[String], expr: &[String]) -> Option { + if paths.len() > 1 { + return None; + } let mut parsed = FindArgs::default(); - let mut i = 0; - - // First non-flag argument is the path (standard find behavior) - if !args[0].starts_with('-') { - parsed.path = args[0].clone(); - i = 1; + if let Some(p) = paths.first() { + parsed.path = p.clone(); } - - while i < args.len() { - match args[i].as_str() { - "-name" => { - if let Some(val) = next_arg(args, &mut i) { - parsed.pattern = val; - } - } - "-iname" => { - if let Some(val) = next_arg(args, &mut i) { - parsed.pattern = val; - parsed.case_insensitive = true; - } + let mut seen_name = false; + let mut seen_type = false; + let mut seen_depth = false; + let mut seen_max = false; + let mut i = 0; + while i < expr.len() { + let value = expr.get(i + 1)?; + match expr[i].as_str() { + "-name" | "-iname" if !seen_name && !value.contains('[') => { + parsed.pattern = value.clone(); + parsed.case_insensitive = expr[i] == "-iname"; + seen_name = true; } - "-type" => { - if let Some(val) = next_arg(args, &mut i) { - parsed.file_type = val; - } + "-type" | "-t" | "--file-type" if !seen_type && (value == "f" || value == "d") => { + parsed.file_type = value.clone(); + seen_type = true; } - "-maxdepth" => { - if let Some(val) = next_arg(args, &mut i) { - parsed.max_depth = Some(val.parse().context("invalid -maxdepth value")?); - } + "-maxdepth" if !seen_depth => { + parsed.max_depth = Some(value.parse().ok()?); + seen_depth = true; } - flag if flag.starts_with('-') => { - eprintln!("rtk find: unknown flag '{}', ignored", flag); + "-m" | "--max" if !seen_max => { + parsed.max_results = value.parse().ok()?; + parsed.max_explicit = true; + seen_max = true; } - _ => {} + _ => return None, } - i += 1; + i += 2; } - - Ok(parsed) + Some(parsed) } -/// Parse RTK syntax: `find [path] [-m max] [-t type]` -fn parse_rtk_find_args(args: &[String]) -> Result { - let mut parsed = FindArgs { - pattern: args[0].clone(), - ..FindArgs::default() +/// find syntax: `find [options] [paths...] [expression]` — paths end at the first +/// token starting with `-`, `!` or a parenthesis, exactly like find. +/// RTK syntax: `find [path] [-m max] [-t type]` — used when the first +/// token contains `*` or `?`, or is not an existing directory. +fn dispatch(original: &[String]) -> Result { + let (args, max, file_type) = peel_trailing_rtk_flags(original); + let legacy = !args.is_empty() + && !is_expression_token(&args[0]) + && (has_glob_meta(&args[0]) || !Path::new(&args[0]).is_dir()); + let args = if legacy { + legacy_to_find_syntax(&args) + } else { + args }; - let mut i = 1; - // Second positional arg (if not a flag) is the path - if i < args.len() && !args[i].starts_with('-') { - parsed.path = args[i].clone(); - i += 1; + let options = args[..leading_options_len(&args)].to_vec(); + let rest = &args[options.len()..]; + let split = rest + .iter() + .position(|t| is_expression_token(t)) + .unwrap_or(rest.len()); + let paths = rest[..split].to_vec(); + let expr = rest[split..].to_vec(); + + if expr.iter().any(|t| VERBATIM_ACTIONS.contains(&t.as_str())) { + let verbatim = if legacy { + legacy_to_find_syntax(original) + } else { + original.to_vec() + }; + return Ok(Dispatch::Verbatim(verbatim)); } - - while i < args.len() { - match args[i].as_str() { - "-m" | "--max" => { - if let Some(val) = next_arg(args, &mut i) { - parsed.max_results = val.parse().context("invalid --max value")?; + if options.is_empty() { + if let Some(mut parsed) = parse_subset(&paths, &expr) { + let repeated_max = max.is_some() && parsed.max_explicit; + let repeated_type = + file_type.is_some() && parsed.file_type != FindArgs::default().file_type; + if !repeated_max && !repeated_type { + if let Some(n) = max { + parsed.max_results = n; + parsed.max_explicit = true; } - } - "-t" | "--file-type" => { - if let Some(val) = next_arg(args, &mut i) { - parsed.file_type = val; + if let Some(t) = file_type { + parsed.file_type = t; } + return Ok(Dispatch::Native(parsed)); } - _ => {} } - i += 1; } + Ok(Dispatch::Compress { + options, + paths, + expr, + max, + file_type, + }) +} + +/// RTK syntax `find [path] ...` rewritten as find syntax +/// `[path] -name ...` so every token after the pattern is dispatched +/// like any find expression. +fn legacy_to_find_syntax(args: &[String]) -> Vec { + let mut rebuilt = Vec::with_capacity(args.len() + 1); + let mut rest = &args[1..]; + if let Some(path) = rest.first().filter(|p| !is_expression_token(p)) { + rebuilt.push(path.clone()); + rest = &rest[1..]; + } + rebuilt.push("-name".to_string()); + rebuilt.push(args[0].clone()); + rebuilt.extend(rest.iter().cloned()); + rebuilt +} - Ok(parsed) +/// rtk's own flags are only recognized at the very end of the command line, +/// where they cannot be an -exec argument or a predicate value. +fn peel_trailing_rtk_flags(args: &[String]) -> (Vec, Option, Option) { + let mut end = args.len(); + let mut max = None; + let mut file_type = None; + while end >= 2 { + let flag = args[end - 2].as_str(); + let value = &args[end - 1]; + match flag { + "-m" | "--max" if max.is_none() => match value.parse::() { + Ok(n) => max = Some(n), + Err(_) => break, + }, + "-t" | "--file-type" if file_type.is_none() && (value == "f" || value == "d") => { + file_type = Some(value.clone()) + } + _ => break, + } + end -= 2; + } + (args[..end].to_vec(), max, file_type) +} + +fn run_verbatim(args: &[String], verbose: u8) -> Result<()> { + let os_args: Vec = args.iter().map(std::ffi::OsString::from).collect(); + let exit_code = crate::core::runner::run_passthrough("find", &os_args, verbose)?; + if exit_code != 0 { + std::process::exit(exit_code); + } + Ok(()) } -/// Entry point from main.rs — parses raw args then delegates to run(). +fn run_compress( + options: &[String], + paths: &[String], + expr: &[String], + max: Option, + file_type: Option<&str>, + verbose: u8, +) -> Result<()> { + let timer = tracking::TimedExecution::start(); + if verbose > 0 { + eprintln!("find: results from find, compressed by rtk"); + } + let max_results = max.unwrap_or(CAP_INVENTORY); + let max_explicit = max.is_some(); + let mut cmd = crate::core::utils::resolved_command("find"); + cmd.args(options).args(paths); + if !expr.is_empty() { + cmd.arg("("); + cmd.args(expr); + cmd.arg(")"); + } + if let Some(t) = file_type { + cmd.arg("-type").arg(t); + } + cmd.arg("-print0").stdin(std::process::Stdio::inherit()); + let output = cmd.output().context("Failed to execute find")?; + let exit_code = crate::core::utils::exit_code_from_output(&output, "find"); + { + let mut stderr = std::io::stderr().lock(); + stderr.write_all(&output.stderr)?; + stderr.flush()?; + } + let entries: Vec<&[u8]> = output + .stdout + .split(|b| *b == 0) + .filter(|s| !s.is_empty()) + .collect(); + let track_cmd = format!( + "find {} {} {}", + options.join(" "), + paths.join(" "), + expr.join(" ") + ); + if entries.iter().any(|e| std::str::from_utf8(e).is_err()) { + let raw: Vec = entries.iter().flat_map(|e| [*e, b"\n"].concat()).collect(); + let mut stdout = std::io::stdout().lock(); + stdout.write_all(&raw)?; + stdout.flush()?; + timer.track_passthrough(&track_cmd, "rtk find (passthrough)"); + } else { + let files: Vec = entries + .iter() + .map(|e| { + let s = String::from_utf8_lossy(e); + match s.strip_prefix("./") { + Some(rest) if !rest.is_empty() => rest.to_string(), + _ => s.to_string(), + } + }) + .collect(); + let raw_output = files.join("\n"); + render( + files, + max_results, + max_explicit, + &track_cmd, + &raw_output, + &timer, + ); + } + if exit_code != 0 { + std::process::exit(exit_code); + } + Ok(()) +} + +/// Entry point from main.rs — dispatches on find's grammar then delegates. pub fn run_from_args(args: &[String], verbose: u8) -> Result<()> { - let parsed = parse_find_args(args)?; - run( - &parsed.pattern, - &parsed.path, - parsed.max_results, - parsed.max_depth, - &parsed.file_type, - parsed.case_insensitive, - verbose, - ) + match dispatch(args)? { + Dispatch::Native(parsed) => run( + &parsed.pattern, + &parsed.path, + parsed.max_results, + parsed.max_explicit, + parsed.max_depth, + &parsed.file_type, + parsed.case_insensitive, + verbose, + ), + Dispatch::Compress { + options, + paths, + expr, + max, + file_type, + } => run_compress(&options, &paths, &expr, max, file_type.as_deref(), verbose), + Dispatch::Verbatim(args) => run_verbatim(&args, verbose), + } +} + +fn group_by_dir(files: &[String]) -> HashMap> { + let mut by_dir: HashMap> = HashMap::new(); + for file in files { + let p = Path::new(file); + let dir = p + .parent() + .map(|d| d.to_string_lossy().to_string()) + .unwrap_or_else(|| ".".to_string()); + let dir = if dir.is_empty() { ".".to_string() } else { dir }; + let filename = p + .file_name() + .map(|f| f.to_string_lossy().to_string()) + .unwrap_or_else(|| file.clone()); + by_dir.entry(dir).or_default().push(filename); + } + by_dir } +fn display_ordered(files: &[String]) -> Vec { + let by_dir = group_by_dir(files); + let mut dirs: Vec<_> = by_dir.keys().cloned().collect(); + dirs.sort(); + let mut ordered = Vec::with_capacity(files.len()); + for dir in &dirs { + for filename in &by_dir[dir] { + if dir == "." { + ordered.push(filename.clone()); + } else { + ordered.push(format!("{}/{}", dir, filename)); + } + } + } + ordered +} + +fn build_capped_listing(files: &[String], max_results: usize) -> String { + let mut listing = files + .iter() + .take(max_results) + .cloned() + .collect::>() + .join("\n"); + if files.len() > max_results { + listing.push_str(&format!("\n+{} more", files.len() - max_results)); + } + listing.push('\n'); + listing +} + +#[allow(clippy::too_many_arguments)] pub fn run( pattern: &str, path: &str, max_results: usize, + max_explicit: bool, max_depth: Option, file_type: &str, case_insensitive: bool, @@ -211,6 +413,11 @@ pub fn run( let want_dirs = file_type == "d"; + if !Path::new(path).exists() { + eprintln!("find: '{}': No such file or directory", path); + std::process::exit(1); + } + // When the pattern targets dotfiles (e.g. -name ".claude.json"), we must walk hidden // entries; otherwise skip them to keep results tidy (#1101). let search_hidden = effective_pattern.starts_with('.'); @@ -262,7 +469,6 @@ pub fn run( continue; } - // Store path relative to search root let display_path = entry_path .strip_prefix(path) .unwrap_or(entry_path) @@ -271,40 +477,46 @@ pub fn run( if !display_path.is_empty() { files.push(display_path); + } else if !is_dir { + files.push(path.to_string()); } } - files.sort(); + let raw_output = { + let mut sorted = files.clone(); + sorted.sort(); + sorted.join("\n") + }; + render( + files, + max_results, + max_explicit, + &format!("find {} -name '{}'", path, effective_pattern), + &raw_output, + &timer, + ); - let raw_output = files.join("\n"); + Ok(()) +} + +fn render( + mut files: Vec, + max_results: usize, + max_explicit: bool, + track_cmd: &str, + raw_output: &str, + timer: &tracking::TimedExecution, +) { + files.sort(); if files.is_empty() { - timer.track( - &format!("find {} -name '{}'", path, effective_pattern), - "rtk find", - &raw_output, - "", - ); - return Ok(()); + timer.track(track_cmd, "rtk find", raw_output, ""); + return; } - // Group by directory - let mut by_dir: HashMap> = HashMap::new(); - - for file in &files { - let p = Path::new(file); - let dir = p - .parent() - .map(|d| d.to_string_lossy().to_string()) - .unwrap_or_else(|| ".".to_string()); - let dir = if dir.is_empty() { ".".to_string() } else { dir }; - let filename = p - .file_name() - .map(|f| f.to_string_lossy().to_string()) - .unwrap_or_default(); - by_dir.entry(dir).or_default().push(filename); - } + let ordered = display_ordered(&files); + let by_dir = group_by_dir(&files); let mut dirs: Vec<_> = by_dir.keys().cloned().collect(); dirs.sort(); let dirs_count = dirs.len(); @@ -322,8 +534,16 @@ pub fn run( } let files_in_dir = &by_dir[dir]; - let dir_display = if dir.len() > 50 { - format!("...{}", &dir[dir.len() - 47..]) + let dir_display = if dir.chars().count() > 50 { + let tail: String = dir + .chars() + .rev() + .take(47) + .collect::>() + .into_iter() + .rev() + .collect(); + format!("...{}", tail) } else { dir.clone() }; @@ -372,16 +592,20 @@ pub fn run( body.push_str(&format!("{}\n", ext_line)); } - let shown = never_worse(&raw_output, &body); - print!("{}", shown); - timer.track( - &format!("find {} -name '{}'", path, effective_pattern), - "rtk find", - &raw_output, - shown, - ); - - Ok(()) + let capped_raw = build_capped_listing(&ordered, max_results); + let hint = if displayed < total_files && !max_explicit { + crate::core::tee::force_tee_tail_hint(&ordered.join("\n"), "find", displayed + 1) + } else { + None + }; + let listing = capped_raw.trim_end_matches('\n'); + let baseline = match &hint { + Some(h) => format!("{}\n{}", listing, h), + None => listing.to_string(), + }; + let shown = + crate::core::runner::emit_guarded(body.trim_end_matches('\n'), hint.as_deref(), &baseline); + timer.track(track_cmd, "rtk find", raw_output, &shown); } #[cfg(test)] @@ -393,6 +617,258 @@ mod tests { values.iter().map(|s| s.to_string()).collect() } + fn parse_find_args(a: &[String]) -> Result { + match dispatch(a)? { + Dispatch::Native(p) => Ok(p), + Dispatch::Compress { .. } => anyhow::bail!("dispatched to compress"), + Dispatch::Verbatim(_) => anyhow::bail!("dispatched to verbatim"), + } + } + + fn class(a: &[&str]) -> &'static str { + match dispatch(&args(a)) { + Ok(Dispatch::Native(_)) => "native", + Ok(Dispatch::Compress { .. }) => "compress", + Ok(Dispatch::Verbatim(_)) => "verbatim", + Err(_) => "error", + } + } + + #[test] + fn rtk_flags_before_actions_reach_find_so_it_refuses_them() { + match dispatch(&args(&["*.rs", "src", "-delete", "-m", "1"])).unwrap() { + Dispatch::Verbatim(a) => { + assert_eq!(a, args(&["src", "-name", "*.rs", "-delete", "-m", "1"])) + } + _ => panic!("expected verbatim"), + } + match dispatch(&args(&[ + ".", "-name", "*.rs", "-exec", "echo", "{}", ";", "-t", "d", + ])) + .unwrap() + { + Dispatch::Verbatim(a) => assert!(a.ends_with(&args(&["-t", "d"]))), + _ => panic!("expected verbatim"), + } + } + + #[test] + fn bare_directory_positional_is_a_path() { + let tmp = std::env::temp_dir().to_string_lossy().into_owned(); + for a in [&["src"][..], &["src/"], &["./src"], &[tmp.as_str()], &["."]] { + let p = parse_find_args(&args(a)).unwrap(); + assert_eq!(p.path, a[0], "{a:?}"); + assert_eq!(p.pattern, "*", "{a:?}"); + } + } + + #[test] + fn literal_name_that_is_not_a_directory_stays_a_pattern() { + let p = parse_find_args(&args(&["Cargo.toml"])).unwrap(); + assert_eq!(p.pattern, "Cargo.toml"); + assert_eq!(p.path, "."); + let p = parse_find_args(&args(&["no_such_dir_xyz"])).unwrap(); + assert_eq!(p.pattern, "no_such_dir_xyz"); + let p = parse_find_args(&args(&["README.md", "src"])).unwrap(); + assert_eq!(p.pattern, "README.md"); + assert_eq!(p.path, "src"); + } + + #[test] + fn leading_glob_token_is_rtk_pattern_syntax() { + let p = parse_find_args(&args(&["*.rs", "src", "-m", "5"])).unwrap(); + assert_eq!(p.pattern, "*.rs"); + assert_eq!(p.path, "src"); + assert_eq!(p.max_results, 5); + assert!(p.max_explicit); + } + + #[test] + fn rtk_pattern_syntax_with_find_predicates_never_blocks() { + match dispatch(&args(&["*.rs", "src", "-mtime", "+7", "-m", "5"])).unwrap() { + Dispatch::Compress { + paths, expr, max, .. + } => { + assert_eq!(paths, args(&["src"])); + assert_eq!(expr, args(&["-name", "*.rs", "-mtime", "+7"])); + assert_eq!(max, Some(5)); + } + _ => panic!("expected compress"), + } + match dispatch(&args(&["*.rs", "-exec", "rm", "{}", ";"])).unwrap() { + Dispatch::Verbatim(a) => { + assert_eq!(a, args(&["-name", "*.rs", "-exec", "rm", "{}", ";"])) + } + _ => panic!("expected verbatim"), + } + } + + #[test] + fn trailing_rtk_flags_are_peeled_in_every_class() { + let (rest, max, ty) = + peel_trailing_rtk_flags(&args(&[".", "-mtime", "-7", "-t", "d", "-m", "5"])); + assert_eq!(rest, args(&[".", "-mtime", "-7"])); + assert_eq!(max, Some(5)); + assert_eq!(ty.as_deref(), Some("d")); + let (rest, max, _) = peel_trailing_rtk_flags(&args(&[".", "-name", "-m"])); + assert_eq!(rest, args(&[".", "-name", "-m"])); + assert_eq!(max, None); + let (rest, max, _) = + peel_trailing_rtk_flags(&args(&[".", "-exec", "grep", "-m", "1", "x", "{}", ";"])); + assert_eq!(rest.len(), 8); + assert_eq!(max, None); + match dispatch(&args(&[".", "-mtime", "-7", "-m", "5"])).unwrap() { + Dispatch::Compress { max, .. } => assert_eq!(max, Some(5)), + _ => panic!("expected compress"), + } + assert_eq!( + class(&[".", "-name", "*.rs", "-exec", "cat", "{}", ";", "-m", "5"]), + "verbatim" + ); + } + + #[test] + fn subset_expressions_stay_native() { + assert_eq!( + class(&[".", "-name", "*.rs", "-type", "f", "-maxdepth", "2"]), + "native" + ); + assert_eq!(class(&["-name", "*.rs"]), "native"); + assert_eq!(class(&["src", "-iname", "README*"]), "native"); + assert_eq!(class(&[".", "-type", "d"]), "native"); + assert_eq!(class(&[".", "-name", "*.rs", "-m", "10"]), "native"); + } + + #[test] + fn unmodeled_listing_predicates_compress_via_find() { + assert_eq!(class(&["-mindepth", "2"]), "compress"); + assert_eq!( + class(&[".", "-path", "*/target/*", "-name", "*.rs"]), + "compress" + ); + assert_eq!(class(&[".", "-type", "l"]), "compress"); + assert_eq!(class(&[".", "-name", "[ab]*.txt"]), "compress"); + assert_eq!(class(&[".", "-name", "a", "-o", "-name", "b"]), "compress"); + assert_eq!(class(&[".", "-not", "-name", "*.rs"]), "compress"); + assert_eq!(class(&[".", "-mtime", "-7"]), "compress"); + assert_eq!(class(&[".", "src"]), "compress"); + assert_eq!(class(&[".", "-name"]), "compress"); + assert_eq!(class(&[".", "-maxdepth", "abc"]), "compress"); + assert_eq!(class(&[".", "-prune"]), "compress"); + } + + #[test] + fn actions_and_output_formats_run_verbatim() { + for a in [ + &[".", "-name", "*.rs", "-exec", "cat", "{}", ";"][..], + &[".", "-delete"], + &[".", "-print0"], + &[".", "-printf", "%p\n"], + &[".", "-ls"], + &[".", "-ok", "rm", "{}", ";"], + ] { + assert_eq!(class(a), "verbatim", "{a:?}"); + } + } + + #[test] + fn rtk_flags_mid_expression_reach_find_untouched() { + assert_eq!( + class(&[".", "-name", "*.rs", "-m", "5", "-exec", "grep", "-m", "1", "x", "{}", ";"]), + "verbatim" + ); + match dispatch(&args(&[".", "-m", "5", "-mtime", "-7"])).unwrap() { + Dispatch::Compress { expr, max, .. } => { + assert_eq!(expr, args(&["-m", "5", "-mtime", "-7"])); + assert_eq!(max, None); + } + _ => panic!("expected compress"), + } + } + + #[test] + fn leading_find_options_are_forwarded_ahead_of_paths() { + for a in [ + &["-L", "src", "-name", "*.rs"][..], + &["-H", "src"], + &["-P", ".", "-type", "l"], + &["-D", "tree", ".", "-name", "x"], + &["-O2", ".", "-name", "x"], + &["-L"], + ] { + match dispatch(&args(a)).unwrap() { + Dispatch::Compress { options, paths, .. } => { + assert!(!options.is_empty(), "{a:?}"); + assert!(paths.iter().all(|p| !p.starts_with('-')), "{a:?}"); + } + _ => panic!("expected compress for {a:?}"), + } + } + assert_eq!( + class(&["-D", "tree", ".", "-exec", "true", ";"]), + "verbatim" + ); + } + + #[test] + fn repeated_subset_predicates_go_to_find() { + assert_eq!(class(&[".", "-name", "a", "-name", "b"]), "compress"); + assert_eq!( + class(&[".", "-iname", "*.txt", "-name", "*.TXT"]), + "compress" + ); + assert_eq!(class(&[".", "-type", "f", "-type", "d"]), "compress"); + assert_eq!( + class(&[".", "-maxdepth", "1", "-maxdepth", "2"]), + "compress" + ); + assert_eq!( + class(&[".", "-name", "*.rs", "-m", "5", "-m", "6"]), + "compress" + ); + } + + #[test] + fn subset_accepts_rtk_type_flag_and_iname() { + let p = parse_find_args(&args(&[ + "src", "-iname", "README*", "-t", "d", "--max", "3", + ])) + .unwrap(); + assert_eq!(p.path, "src"); + assert!(p.case_insensitive); + assert_eq!(p.file_type, "d"); + assert_eq!(p.max_results, 3); + assert!(p.max_explicit); + assert_eq!(class(&[".", "-type", "l"]), "compress"); + assert_eq!(class(&[".", "-t", "x"]), "compress"); + } + + #[test] + fn root_entry_keeps_its_name() { + let ordered = display_ordered(&args(&[".", "a.rs"])); + assert_eq!(ordered, args(&[".", "a.rs"])); + assert_eq!(build_capped_listing(&ordered, 10), ".\na.rs\n"); + } + + #[test] + fn long_unicode_dir_label_does_not_panic() { + let root = tempfile::TempDir::new().unwrap(); + let dir = root.path().join("é".repeat(30)); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("f.txt"), "").unwrap(); + let result = run( + "*.txt", + root.path().to_str().unwrap(), + 50, + true, + None, + "f", + false, + 0, + ); + assert!(result.is_ok()); + } + // --- glob_match unit tests --- #[test] @@ -496,17 +972,19 @@ mod tests { // --- parse_find_args: unsupported flags --- #[test] - fn parse_native_find_rejects_not() { - let result = parse_find_args(&args(&[".", "-name", "*.rs", "-not", "-name", "*_test.rs"])); - assert!(result.is_err()); - let msg = result.unwrap_err().to_string(); - assert!(msg.contains("compound predicates")); + fn parse_native_find_not_is_not_native() { + assert_eq!( + class(&[".", "-name", "*.rs", "-not", "-name", "*_test.rs"]), + "compress" + ); } #[test] - fn parse_native_find_rejects_exec() { - let result = parse_find_args(&args(&[".", "-name", "*.tmp", "-exec", "rm", "{}", ";"])); - assert!(result.is_err()); + fn parse_native_find_exec_is_not_native() { + assert_eq!( + class(&[".", "-name", "*.tmp", "-exec", "rm", "{}", ";"]), + "verbatim" + ); } // --- parse_find_args: RTK syntax --- @@ -569,14 +1047,14 @@ mod tests { #[test] fn find_dotfile_pattern_includes_hidden() { // .gitignore exists at the repo root — must be found when using a dotfile pattern - let result = run(".gitignore", ".", 50, Some(1), "f", false, 0); + let result = run(".gitignore", ".", 50, true, Some(1), "f", false, 0); assert!(result.is_ok(), "run with dotfile pattern should not error"); } #[test] fn find_regular_pattern_skips_hidden() { // Non-dot pattern should not error (hidden dirs remain skipped) - let result = run("*.rs", "src", 5, None, "f", false, 0); + let result = run("*.rs", "src", 5, true, None, "f", false, 0); assert!(result.is_ok()); } @@ -585,34 +1063,113 @@ mod tests { #[test] fn find_rs_files_in_src() { // Should find .rs files without error - let result = run("*.rs", "src", 100, None, "f", false, 0); + let result = run("*.rs", "src", 100, true, None, "f", false, 0); assert!(result.is_ok()); } #[test] fn find_dot_pattern_works() { // "." pattern should not error (was broken before) - let result = run(".", "src", 10, None, "f", false, 0); + let result = run(".", "src", 10, true, None, "f", false, 0); assert!(result.is_ok()); } #[test] fn find_no_matches() { - let result = run("*.xyz_nonexistent", "src", 50, None, "f", false, 0); + let result = run("*.xyz_nonexistent", "src", 50, true, None, "f", false, 0); assert!(result.is_ok()); } #[test] fn find_respects_max() { // With max=2, should not error - let result = run("*.rs", "src", 2, None, "f", false, 0); + let result = run("*.rs", "src", 2, true, None, "f", false, 0); assert!(result.is_ok()); } + #[test] + fn rtk_max_flag_marks_cap_explicit() { + let parsed = parse_find_args(&args(&["*.rs", "-m", "10"])).unwrap(); + assert!(parsed.max_explicit); + let parsed = parse_find_args(&args(&["*.rs", "--max", "10"])).unwrap(); + assert!(parsed.max_explicit); + } + + #[test] + fn native_syntax_cap_stays_implicit() { + let parsed = parse_find_args(&args(&[".", "-name", "*.rs", "-type", "f"])).unwrap(); + assert!(!parsed.max_explicit); + } + + #[test] + fn default_cap_stays_implicit() { + let parsed = parse_find_args(&args(&[])).unwrap(); + assert!(!parsed.max_explicit); + let parsed = parse_find_args(&args(&["*.rs", "src"])).unwrap(); + assert!(!parsed.max_explicit); + } + + #[test] + fn tee_remainder_matches_hidden_files_when_dir_sort_diverges() { + // "logs-old/f01" < "logs/f01" in flat path sort ('-' < '/'), but the + // display sorts by dir name where "logs" < "logs-old". The tee must + // follow display order or tail returns already-shown files. + let mut files: Vec = Vec::new(); + for i in 1..=30 { + files.push(format!("logs/f{:02}.txt", i)); + files.push(format!("logs-old/f{:02}.txt", i)); + } + files.sort(); + assert!(files[0].starts_with("logs-old/")); + + let ordered = display_ordered(&files); + assert!(ordered[0].starts_with("logs/")); + assert_eq!(ordered.len(), 60); + + let hidden = &ordered[50..]; + assert_eq!(hidden.first().unwrap(), "logs-old/f21.txt"); + assert_eq!(hidden.last().unwrap(), "logs-old/f30.txt"); + assert!(hidden.iter().all(|f| f.starts_with("logs-old/"))); + } + + #[test] + fn display_ordered_keeps_root_files_in_root_group() { + let files: Vec = args(&["zebra.txt", "logs/a.txt", "alpha.txt"]); + let ordered = display_ordered(&files); + assert_eq!(ordered, args(&["zebra.txt", "alpha.txt", "logs/a.txt"])); + } + + #[test] + fn capped_listing_no_truncation() { + let files = args(&["a.rs", "b.rs"]); + assert_eq!(build_capped_listing(&files, 10), "a.rs\nb.rs\n"); + } + + #[test] + fn capped_listing_truncates_with_marker() { + let files = args(&["a.rs", "b.rs", "c.rs", "d.rs"]); + assert_eq!(build_capped_listing(&files, 2), "a.rs\nb.rs\n+2 more\n"); + } + + #[test] + fn capped_listing_at_exact_max_has_no_marker() { + let files = args(&["a.rs", "b.rs"]); + assert_eq!(build_capped_listing(&files, 2), "a.rs\nb.rs\n"); + } + + #[test] + fn capped_baseline_beats_grouped_summary_on_tiny_result_sets() { + use crate::core::guard::never_worse; + let files = args(&["a.rs"]); + let capped = build_capped_listing(&files, 10); + let grouped = "1F 1D:\n\n./ a.rs\n\next: .rs(1)\n"; + assert_eq!(never_worse(&capped, grouped), capped); + } + #[test] fn find_gitignored_excluded() { // target/ is in .gitignore — files inside should not appear - let result = run("*", ".", 1000, None, "f", false, 0); + let result = run("*", ".", 1000, true, None, "f", false, 0); assert!(result.is_ok()); // We can't easily capture stdout in unit tests, but at least // verify it runs without error. The smoke tests verify content. diff --git a/src/core/stream.rs b/src/core/stream.rs index cd573c4cc9..3f24b7edcc 100644 --- a/src/core/stream.rs +++ b/src/core/stream.rs @@ -1,11 +1,43 @@ use anyhow::{Context, Result}; -use std::io::{self, BufRead, BufReader, BufWriter, Write}; +use std::io::{self, BufRead, BufReader, BufWriter, Read, Write}; use std::process::{Command, Stdio}; use std::sync::mpsc; #[cfg(test)] use regex::Regex; +/// Read `reader` line by line, decoding each line through the console code +/// page and falling back to lossy UTF-8 (invalid bytes become U+FFFD) instead +/// of erroring. +/// +/// `BufRead::lines()` returns `Err` for a non-UTF-8 line, and callers +/// commonly chain `.map_while(Result::ok)` to skip bad lines — but +/// `map_while` stops at the *first* `None`, so one invalid-UTF-8 line (e.g. +/// OEM/ANSI bytes from a non-English-locale Windows tool) silently discards +/// every line after it too, not just the bad one. This reads raw bytes and +/// never fails on the source encoding, so a garbled line still surfaces +/// instead of vanishing along with everything downstream of it. +/// +/// The OEM/ANSI lines this guards against are exactly what +/// [`decode_process_output`](super::utils::decode_process_output) exists to +/// read, so the streamed path decodes them the same way the captured path +/// does rather than going straight to U+FFFD. +fn read_lines_lossy(reader: impl Read) -> impl Iterator { + BufReader::new(reader).split(b'\n').filter_map(|res| { + let mut buf = match res { + Ok(buf) => buf, + Err(e) => { + eprintln!("rtk: stream read error: {}", e); + return None; + } + }; + if buf.last() == Some(&b'\r') { + buf.pop(); + } + Some(super::utils::decode_process_output(&buf)) + }) +} + pub trait StreamFilter { fn feed_line(&mut self, line: &str) -> Option; fn flush(&mut self) -> String; @@ -298,10 +330,7 @@ pub fn run_streaming( Some(std::thread::spawn(move || { let mut writer = BufWriter::new(child_stdin); let stdin_handle = io::stdin(); - for line in BufReader::new(stdin_handle.lock()) - .lines() - .map_while(Result::ok) - { + for line in read_lines_lossy(stdin_handle.lock()) { if let Some(out) = filter.feed_line(&line) { if writeln!(writer, "{}", out).is_err() { break; @@ -340,7 +369,7 @@ pub fn run_streaming( let (tx, rx) = mpsc::channel(); let tx_out = tx.clone(); let stdout_thread = std::thread::spawn(move || { - for line in BufReader::new(stdout).lines().map_while(Result::ok) { + for line in read_lines_lossy(stdout) { if tx_out.send(StreamLine::Stdout(line)).is_err() { break; } @@ -348,7 +377,7 @@ pub fn run_streaming( }); let tx_err = tx; let stderr_thread = std::thread::spawn(move || { - for line in BufReader::new(stderr).lines().map_while(Result::ok) { + for line in read_lines_lossy(stderr) { if tx_err.send(StreamLine::Stderr(line)).is_err() { break; } @@ -417,7 +446,7 @@ pub fn run_streaming( let stderr_thread = std::thread::spawn(move || -> String { let mut raw_err = String::new(); let mut capped = false; - for line in BufReader::new(stderr).lines().map_while(Result::ok) { + for line in read_lines_lossy(stderr) { if raw_err.len() + line.len() < RAW_CAP { raw_err.push_str(&line); raw_err.push('\n'); @@ -436,7 +465,7 @@ pub fn run_streaming( FilterMode::Passthrough => unreachable!("handled by early-return above"), FilterMode::Streaming(_) => unreachable!("handled by is_streaming branch"), FilterMode::Buffered(filter_fn) => { - for line in BufReader::new(stdout).lines().map_while(Result::ok) { + for line in read_lines_lossy(stdout) { if raw_stdout.len() + line.len() < RAW_CAP { raw_stdout.push_str(&line); raw_stdout.push('\n'); @@ -461,7 +490,7 @@ pub fn run_streaming( } } FilterMode::CaptureOnly => { - for line in BufReader::new(stdout).lines().map_while(Result::ok) { + for line in read_lines_lossy(stdout) { if raw_stdout.len() + line.len() < RAW_CAP { raw_stdout.push_str(&line); raw_stdout.push('\n'); @@ -533,22 +562,30 @@ impl CaptureResult { pub fn exec_capture(cmd: &mut Command) -> Result { cmd.stdin(Stdio::null()); - let output = cmd.output().context("Failed to execute command")?; - Ok(CaptureResult { - stdout: String::from_utf8_lossy(&output.stdout).into_owned(), - stderr: String::from_utf8_lossy(&output.stderr).into_owned(), - exit_code: status_to_exit_code(output.status), - }) + capture(cmd) } /// Like [`exec_capture`] but inherits stdin so a wrapped engine can read a piped stdin. pub fn exec_capture_stdin(cmd: &mut Command) -> Result { cmd.stdin(Stdio::inherit()); + capture(cmd) +} + +/// Run `cmd` to completion, decode what it wrote, and report the exit code. +/// +/// A process killed by a signal has no exit code of its own, and returning +/// only the synthesized `128 + signal` hides why it died. `exit_code_from_output` +/// announces that on stderr, so callers moving here from a hand-rolled +/// `.output()` keep the diagnostic instead of losing it. The program name is +/// used as the label so no call site has to pass one. +fn capture(cmd: &mut Command) -> Result { + let program = cmd.get_program().to_string_lossy().into_owned(); let output = cmd.output().context("Failed to execute command")?; + let exit_code = super::utils::exit_code_from_output(&output, &program); Ok(CaptureResult { - stdout: String::from_utf8_lossy(&output.stdout).into_owned(), - stderr: String::from_utf8_lossy(&output.stderr).into_owned(), - exit_code: status_to_exit_code(output.status), + stdout: super::utils::decode_process_output(&output.stdout), + stderr: super::utils::decode_process_output(&output.stderr), + exit_code, }) } @@ -557,6 +594,77 @@ pub(crate) mod tests { use super::*; use std::process::Command; + #[test] + fn test_read_lines_lossy_preserves_lines_after_invalid_utf8() { + // Line 2 contains a lone 0xE3 byte (the cp850 case from the bug report). + // `BufRead::lines().map_while(Result::ok)` would stop at that Err and + // silently lose line 3 as well — not just the bad byte. + let mut data = Vec::new(); + data.extend_from_slice(b"ERRO A: ascii\n"); + data.extend_from_slice(&[b'E', b'R', b'R', b'O', b' ', b'B', b':', b' ', 0xE3, b'\n']); + data.extend_from_slice(b"ERRO C: ascii again\n"); + + let lines: Vec = read_lines_lossy(data.as_slice()).collect(); + assert_eq!(lines.len(), 3, "got: {:?}", lines); + assert_eq!(lines[0], "ERRO A: ascii"); + assert!(lines[1].starts_with("ERRO B: "), "got: {:?}", lines[1]); + assert!(lines[1].contains('\u{FFFD}'), "got: {:?}", lines[1]); + assert_eq!(lines[2], "ERRO C: ascii again"); + } + + #[test] + fn test_read_lines_lossy_strips_crlf() { + let lines: Vec = read_lines_lossy(&b"a\r\nb\n"[..]).collect(); + assert_eq!(lines, vec!["a".to_string(), "b".to_string()]); + } + + #[test] + fn test_read_lines_lossy_no_trailing_newline() { + let lines: Vec = read_lines_lossy(&b"only line"[..]).collect(); + assert_eq!(lines, vec!["only line".to_string()]); + } + + #[test] + fn test_read_lines_lossy_empty_input() { + let lines: Vec = read_lines_lossy(&b""[..]).collect(); + assert!(lines.is_empty()); + } + + /// A `Read` that yields some good lines, then a genuine I/O error -- + /// distinct from clean EOF (`Ok(0)`). Before this fix, `Ok(0) | Err(_)` + /// treated both the same way, silently truncating output on a real read + /// failure instead of surfacing it. + struct FailingReader { + data: std::io::Cursor>, + failed: bool, + } + + impl Read for FailingReader { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + if self.data.position() as usize >= self.data.get_ref().len() { + if !self.failed { + self.failed = true; + return Err(io::Error::other("simulated read failure")); + } + return Ok(0); + } + std::io::Read::read(&mut self.data, buf) + } + } + + #[test] + fn test_read_lines_lossy_stops_on_io_error_without_panicking() { + let reader = FailingReader { + data: std::io::Cursor::new(b"line one\nline two\n".to_vec()), + failed: false, + }; + // The two good lines are still yielded; the simulated failure after + // them must not panic or hang -- it just ends the iterator, same as + // clean EOF would, but via the Err(_) arm instead of Ok(0). + let lines: Vec = read_lines_lossy(reader).collect(); + assert_eq!(lines, vec!["line one".to_string(), "line two".to_string()]); + } + struct LineFilter Option> { f: F, } @@ -598,6 +706,26 @@ pub(crate) mod tests { assert_eq!(status_to_exit_code(status), 137); } + #[test] + fn test_exec_capture_decodes_and_reports_exit_code() { + let captured = exec_capture(&mut Command::new("false")).expect("spawn"); + assert_eq!(captured.exit_code, 1); + assert!(!captured.success()); + } + + /// A signal-killed child keeps the `128 + signal` code that + /// `exit_code_from_output` reports, so callers that moved off a hand-rolled + /// `.output()` neither lose the code nor the stderr diagnostic with it. + #[cfg(unix)] + #[test] + fn test_exec_capture_reports_signal_exit_code() { + // `kill -TERM $$` makes the shell terminate itself by signal 15. + let mut cmd = Command::new("sh"); + cmd.arg("-c").arg("kill -TERM $$"); + let captured = exec_capture(&mut cmd).expect("spawn"); + assert_eq!(captured.exit_code, 128 + 15); + } + #[test] fn test_line_filter_passes_lines() { let mut f = LineFilter::new(|l| Some(format!("{}\n", l.to_uppercase()))); diff --git a/src/core/tee.rs b/src/core/tee.rs index 0bebe0d1e0..80f70c15b8 100644 --- a/src/core/tee.rs +++ b/src/core/tee.rs @@ -14,8 +14,10 @@ const DEFAULT_MAX_FILES: usize = 20; const DEFAULT_MAX_FILE_SIZE: usize = 1_048_576; /// Sanitize a command slug for use in filenames. -/// Replaces non-alphanumeric chars (except underscore/hyphen) with underscore, -/// truncates at 40 chars. +/// Replaces non-alphanumeric chars (except underscore/hyphen) with underscore. +/// Long slugs (usually an embedded file path that duplicates the command the LLM +/// already issued) collapse to a short readable prefix plus a short disambiguating +/// hash, keeping recovery filenames unique but compact — fewer tokens per tee hint. fn sanitize_slug(slug: &str) -> String { let sanitized: String = slug .chars() @@ -27,11 +29,22 @@ fn sanitize_slug(slug: &str) -> String { } }) .collect(); - if sanitized.len() > 40 { - sanitized[..40].to_string() - } else { - sanitized + const MAX_READABLE: usize = 24; + if sanitized.len() <= MAX_READABLE { + return sanitized; } + let prefix: String = sanitized.chars().take(8).collect(); + format!("{}_{}", prefix, short_hash(&sanitized)) +} + +/// First 6 hex chars (24 bits) of the SHA-256 of `s` — a compact tag to keep +/// shortened slugs distinct. Not collision-resistant on its own: 24 bits hits a +/// birthday collision after only a few thousand distinct slugs. It's safe here +/// because a clash also requires the identical readable prefix *and* the same +/// epoch second, which together scope tee writes exactly as before. +fn short_hash(s: &str) -> String { + use sha2::{Digest, Sha256}; + format!("{:x}", Sha256::digest(s.as_bytes()))[..6].to_string() } /// Get the tee directory, respecting config and env overrides. @@ -350,9 +363,23 @@ mod tests { assert_eq!(sanitize_slug("cargo test"), "cargo_test"); assert_eq!(sanitize_slug("cargo-test"), "cargo-test"); assert_eq!(sanitize_slug("go/test/./pkg"), "go_test___pkg"); - // Truncate at 40 - let long = "a".repeat(50); - assert_eq!(sanitize_slug(&long).len(), 40); + // Long slugs (embedded paths) collapse to a readable prefix + hash, staying short. + let long = format!("grep_0_{}", "a".repeat(50)); + let short = sanitize_slug(&long); + assert!( + short.len() < 24, + "long slug should shorten, got '{}'", + short + ); + assert!( + short.starts_with("grep_0_a"), + "keeps a readable prefix, got '{}'", + short + ); + // Deterministic, and different slugs never collide onto the same filename. + assert_eq!(sanitize_slug(&long), short); + let other = sanitize_slug(&format!("grep_1_{}", "a".repeat(50))); + assert_ne!(other, short, "distinct slugs must not collide"); } #[test] diff --git a/src/core/utils.rs b/src/core/utils.rs index d10f4b0118..c09afc32b9 100644 --- a/src/core/utils.rs +++ b/src/core/utils.rs @@ -496,6 +496,131 @@ pub fn human_bytes(bytes: u64) -> String { } } +/// Decode child process output bytes, respecting the Windows console code page. +/// +/// Valid UTF-8 is returned untouched — the overwhelmingly common case, and the +/// only one on Unix. Otherwise the buffer is decoded a line at a time: lines +/// that are valid UTF-8 keep their bytes, and only the lines that are not get +/// re-decoded through the console code page (GBK, Big5, CP850, …). Decoding +/// the whole buffer as a legacy code page at the first bad byte would mangle +/// output that was almost entirely valid UTF-8. +/// +/// Falls back to lossy UTF-8 (`U+FFFD`) when no code page applies, matching the +/// previous behavior. +pub fn decode_process_output(bytes: &[u8]) -> String { + // Fast path: fully valid UTF-8 needs no scanning and no allocation beyond + // the copy. This is every Unix run and every UTF-8 console on Windows. + match std::str::from_utf8(bytes) { + Ok(s) => s.to_owned(), + Err(_) => decode_mixed(bytes, output_codepage()), + } +} + +/// Decode `bytes` line by line, keeping valid UTF-8 lines verbatim and passing +/// the rest through code page `cp` (lossy UTF-8 when `cp` is `None`). +/// +/// The line is the decoding unit because a byte run is not one: GB18030's +/// four-byte sequences embed bytes in the ASCII digit range, so any rule that +/// stops a run at the first byte under `0x80` splits them. `\n` is unambiguous +/// in every encoding handled here — none of UTF-8, GBK, gb18030, Big5, +/// Shift_JIS, EUC-KR or the single-byte pages can produce it as a trail byte — +/// and a process does not switch encoding mid-line, so the whole line can be +/// handed to one decoder. +/// +/// Takes the code page as a parameter so the walk and the mapping are +/// unit-testable on every platform, not only Windows. +fn decode_mixed(bytes: &[u8], cp: Option) -> String { + let mut out = String::with_capacity(bytes.len()); + // `split_inclusive` keeps the terminator, so line endings survive intact + // and an empty input yields no chunks at all. + for line in bytes.split_inclusive(|&b| b == b'\n') { + match std::str::from_utf8(line) { + Ok(valid) => out.push_str(valid), + Err(_) => out.push_str(&decode_line(line, cp)), + } + } + out +} + +/// Decode one line that failed UTF-8 validation using code page `cp`. +/// +/// A code page result is only accepted when it decodes cleanly. A line that is +/// really UTF-8 with a corrupt byte usually fails the code page decoder too, +/// and lossy UTF-8 preserves its valid characters where the code page would +/// turn all of them into mojibake. +fn decode_line(line: &[u8], cp: Option) -> String { + if let Some(cp) = cp { + // encoding_rs covers the ANSI and DBCS pages (1252, GBK, gb18030, + // Shift_JIS, Big5, …); `codepage` maps the Windows page number onto it. + if let Some(encoding) = codepage::to_encoding(cp) { + let (decoded, _, had_errors) = encoding.decode(line); + if !had_errors { + return decoded.into_owned(); + } + // encoding_rs implements only WHATWG encodings, which exclude the + // legacy OEM/DOS pages (437, 850, 852, …) that plain cmd.exe still + // defaults to in many locales. `oem_cp` supplies those tables. + } else if let Some(table) = oem_cp::code_table::DECODING_TABLE_CP_MAP.get(&cp) { + if let Some(decoded) = table.decode_string_checked(line) { + return decoded; + } + } else { + warn_unmapped_codepage(cp); + } + } + String::from_utf8_lossy(line).into_owned() +} + +/// Warn once that output is being decoded lossily because the console code +/// page has no known table — previously this fell back silently. +fn warn_unmapped_codepage(cp: u16) { + static WARNED: std::sync::Once = std::sync::Once::new(); + WARNED.call_once(|| { + eprintln!( + "[rtk] warning: no decoder for console code page {}; \ + non-UTF-8 output will be shown with replacement characters", + cp + ); + }); +} + +/// The code page child output should be decoded with, or `None` when the +/// platform has no such concept (every Unix) and lossy UTF-8 should be used. +#[cfg(not(windows))] +fn output_codepage() -> Option { + None +} + +/// Windows: the console output code page, cached after the first lookup. +/// +/// `GetConsoleOutputCP` describes the console rtk is attached to, which the +/// child inherits. It returns 0 when there is no console — rtk running under a +/// hook with its output piped, the common case flagged in review — and a +/// console program writing to a pipe uses the ANSI code page instead, so +/// `GetACP` is the fallback rather than giving up and decoding lossily. +/// +/// This remains a best guess: a child is free to emit any encoding regardless +/// of either code page. It is only ever consulted for bytes that already +/// failed UTF-8 validation, so a wrong guess degrades to the same replacement +/// characters that the previous lossy conversion produced. +#[cfg(windows)] +fn output_codepage() -> Option { + static CODEPAGE: OnceLock> = OnceLock::new(); + *CODEPAGE.get_or_init(|| { + #[allow(unsafe_code)] + // nosemgrep: unsafe-block — read-only Win32 APIs, no memory or thread safety risk + let cp = unsafe { + let console = windows_sys::Win32::System::Console::GetConsoleOutputCP(); + if console != 0 { + console + } else { + windows_sys::Win32::Globalization::GetACP() + } + }; + u16::try_from(cp).ok().filter(|&cp| cp != 0) + }) +} + #[cfg(test)] mod tests { use super::*; @@ -1045,4 +1170,165 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); restrict_file(&tmp.path().join("absent.db-wal")); } + + #[test] + fn test_decode_process_output_valid_utf8() { + assert_eq!(decode_process_output(b"hello world"), "hello world"); + } + + #[test] + fn test_decode_process_output_chinese_utf8() { + let input = "测试中文".as_bytes(); + assert_eq!(decode_process_output(input), "测试中文"); + } + + #[test] + fn test_decode_process_output_empty() { + assert_eq!(decode_process_output(b""), ""); + } + + #[test] + fn test_decode_process_output_invalid_utf8_no_panic() { + let bytes: &[u8] = &[0xFF, 0xFE, 0x41, 0x42]; + let result = decode_process_output(bytes); + assert!(!result.is_empty()); + } + + // `decode_mixed` takes the code page as a parameter so these run on every + // platform, not only Windows — the CI runners that would exercise the + // Windows-gated versions do not exist. + + /// The bug this fix targets: GBK output from a Chinese-locale console. + #[test] + fn test_decode_mixed_gbk_run() { + assert_eq!(decode_mixed(&[0xB2, 0xE2, 0xCA, 0xD4], Some(936)), "测试"); + } + + /// GB18030 (54936) must not be treated as plain GBK: it has to keep its + /// own table so 4-byte sequences decode. + #[test] + fn test_decode_mixed_gb18030_is_not_gbk() { + // 0x81 0x35 0xF4 0x37 is a 4-byte GB18030 sequence. + let decoded = decode_mixed(&[0x81, 0x35, 0xF4, 0x37], Some(54936)); + assert_eq!( + decoded.chars().count(), + 1, + "expected one char: {:?}", + decoded + ); + assert!( + !decoded.contains('\u{FFFD}'), + "got replacement: {:?}", + decoded + ); + assert_eq!( + codepage::to_encoding(54936).map(|e| e.name()), + Some("gb18030") + ); + } + + /// Legacy OEM/DOS pages are cmd.exe's default in many locales and are not + /// WHATWG encodings, so they come from `oem_cp` rather than encoding_rs. + #[test] + fn test_decode_mixed_oem_codepages() { + assert_eq!(decode_mixed(&[0xB0, 0xDB], Some(437)), "░█"); + // 850 and 852 are likewise absent from encoding_rs. + assert!(codepage::to_encoding(437).is_none()); + assert!(!decode_mixed(&[0xE1], Some(850)).contains('\u{FFFD}')); + } + + /// The regression the review flagged: a buffer that is almost entirely + /// valid UTF-8 must not be reinterpreted wholesale because of one bad + /// line. Only the GBK line is re-decoded; the UTF-8 lines keep their bytes. + #[test] + fn test_decode_mixed_only_redecodes_invalid_lines() { + let mut bytes = "héllo wörld\n".as_bytes().to_vec(); + bytes.extend_from_slice(&[0xB2, 0xE2, 0xCA, 0xD4, b'\n']); // GBK 测试 + bytes.extend_from_slice("grüße\n".as_bytes()); + + assert_eq!( + decode_mixed(&bytes, Some(936)), + "héllo wörld\n测试\ngrüße\n" + ); + } + + /// A corrupt byte inside an otherwise-UTF-8 line falls back to lossy UTF-8 + /// rather than mojibake, because the code page decode does not come out + /// clean. The surrounding lines are untouched either way. + #[test] + fn test_decode_mixed_corrupt_byte_prefers_lossy_utf8() { + let mut bytes = "first\n".as_bytes().to_vec(); + bytes.extend_from_slice("naïve".as_bytes()); + bytes.push(0xC3); // dangling lead byte: invalid UTF-8 and invalid GBK + bytes.extend_from_slice(b"\nlast\n"); + + let decoded = decode_mixed(&bytes, Some(936)); + assert!(decoded.starts_with("first\n"), "{:?}", decoded); + assert!(decoded.contains("naïve"), "{:?}", decoded); + assert!(decoded.ends_with("\nlast\n"), "{:?}", decoded); + } + + /// Line endings and the final line without a terminator must survive. + #[test] + fn test_decode_mixed_preserves_line_structure() { + let mut bytes = b"a\r\n".to_vec(); + bytes.extend_from_slice(&[0xB2, 0xE2, b'\n']); // GBK 测 + bytes.extend_from_slice(b"tail"); // no trailing newline + + assert_eq!(decode_mixed(&bytes, Some(936)), "a\r\n测\ntail"); + } + + /// No code page (every Unix run) keeps the old lossy behavior. + #[test] + fn test_decode_mixed_without_codepage_is_lossy() { + let mut bytes = b"ok ".to_vec(); + bytes.push(0xFF); + assert_eq!(decode_mixed(&bytes, None), "ok \u{FFFD}"); + } + + /// An unmapped code page must degrade to lossy rather than panic. + #[test] + fn test_decode_mixed_unknown_codepage_is_lossy() { + assert_eq!(decode_mixed(&[0xFF], Some(60000)), "\u{FFFD}"); + } + + /// Truncated trailing UTF-8 must terminate rather than loop forever. + #[test] + fn test_decode_mixed_truncated_utf8_terminates() { + // First two bytes of a three-byte character, cut short. + let decoded = decode_mixed(&[b'a', 0xE6, 0xB5], None); + assert!(decoded.starts_with('a'), "{:?}", decoded); + } + + /// Every byte value must survive both paths without panicking. + #[test] + fn test_decode_mixed_all_byte_values_no_panic() { + let all: Vec = (0u8..=255).collect(); + for cp in [None, Some(936), Some(437), Some(65001), Some(1252)] { + let _ = decode_mixed(&all, cp); + } + } + + /// Without a code page — every Unix run — the result must stay identical to + /// the `String::from_utf8_lossy` this replaced. Splitting on `\n` cannot + /// change it: a newline is valid ASCII, so no ill-formed sequence spans one. + #[test] + fn test_decode_process_output_unchanged_without_codepage() { + let cases: [&[u8]; 6] = [ + b"", + b"plain ascii\n", + "utf8 ünïcödé\nsecond ライン\n".as_bytes(), + &[0xFF, 0xFE, b'\n', b'o', b'k'], + &[b'a', b'\n', 0xE6, 0xB5, b'\n', b'b'], + &[0xB2, 0xE2, 0xCA, 0xD4], + ]; + for bytes in cases { + assert_eq!( + decode_mixed(bytes, None), + String::from_utf8_lossy(bytes), + "lossy mismatch for {:?}", + bytes + ); + } + } } diff --git a/src/discover/provider.rs b/src/discover/provider.rs index d8f73c6fa6..57a15ae4d5 100644 --- a/src/discover/provider.rs +++ b/src/discover/provider.rs @@ -117,15 +117,19 @@ impl ClaudeProvider { /// Encode a filesystem path to Claude Code's directory name format. /// - /// Claude Code replaces `/`, `.`, `_`, `\`, and any non-ASCII character - /// with `-` when computing the project directory slug under `~/.claude/projects/`. + /// Claude Code replaces `/`, `.`, `_`, `\`, `:`, ` `, `[`, `]`, and any + /// non-ASCII character with `-` when computing the project directory slug + /// under `~/.claude/projects/`. /// /// `/Users/foo/bar` → `-Users-foo-bar` /// `/Users/first.last/bar` → `-Users-first-last-bar` /// `/home/chris/2_project` → `-home-chris-2-project` - /// `C:\Users\foo\bar` → `C:-Users-foo-bar` + /// `C:\Users\foo\bar` → `C--Users-foo-bar` pub fn encode_project_path(path: &str) -> String { - const SANITIZED_CHARS: &[char] = &['/', '.', '_', '\\', ' ', '[', ']']; + // The drive-letter `:` matters on Windows: every cwd carries one, and if + // it isn't sanitized the slug (`C:-...`) never matches Claude's real + // folder (`C--...`), so `rtk discover` finds zero sessions (#2919). + const SANITIZED_CHARS: &[char] = &['/', '.', '_', '\\', ' ', '[', ']', ':']; path.chars() .map(|c| { @@ -403,10 +407,13 @@ mod tests { #[test] fn test_encode_project_path_windows() { - // Windows backslashes are also replaced with '-' + // Windows backslashes AND the drive-letter colon are replaced with '-'. + // A real `C:\Users\foo\bar` dir lands in ~/.claude/projects/C--Users-foo-bar, + // so keeping the colon (C:-...) made `rtk discover` find zero sessions on + // Windows (#2919). assert_eq!( ClaudeProvider::encode_project_path(r"C:\Users\foo\bar"), - "C:-Users-foo-bar" + "C--Users-foo-bar" ); } diff --git a/src/hooks/rewrite_cmd.rs b/src/hooks/rewrite_cmd.rs index 685aa3015c..9f35d581c1 100644 --- a/src/hooks/rewrite_cmd.rs +++ b/src/hooks/rewrite_cmd.rs @@ -45,8 +45,22 @@ enum RewriteOutcome { } fn evaluate(cmd: &str, excluded: &[String], transparent_prefixes: &[String]) -> RewriteOutcome { - let verdict = check_command(cmd); + evaluate_with_verdict(cmd, check_command(cmd), excluded, transparent_prefixes) +} +/// Decision logic for [`evaluate`] with the permission verdict supplied by the +/// caller, mirroring [`check_command_with_rules`](super::permissions::check_command_with_rules). +/// +/// `check_command` reads the machine's Claude Code settings files, so tests that +/// call [`evaluate`] directly would change verdict with the developer's local +/// `settings.local.json`. Taking the verdict as a parameter keeps the rewrite +/// logic under test independent of the host configuration (#3146). +fn evaluate_with_verdict( + cmd: &str, + verdict: PermissionVerdict, + excluded: &[String], + transparent_prefixes: &[String], +) -> RewriteOutcome { if verdict == PermissionVerdict::Deny { return RewriteOutcome::Deny; } @@ -90,13 +104,46 @@ mod tests { ); } + /// The verdict still drives the outcome: an allow rule yields `Allow`. + /// Pinning both directions keeps the mapping covered without depending + /// on which rules the developer happens to have configured. + #[test] + fn test_allow_verdict_yields_allow() { + assert!(matches!( + evaluate_with_verdict("git status", PermissionVerdict::Allow, &[], &[]), + RewriteOutcome::Allow(_) + )); + } + + #[test] + fn test_deny_verdict_yields_deny() { + assert_eq!( + evaluate_with_verdict("git status", PermissionVerdict::Deny, &[], &[]), + RewriteOutcome::Deny + ); + } + + /// Commands with an unattestable construct are always a passthrough, + /// regardless of permission verdict. + /// + /// The verdict is pinned to `Default` rather than going through `evaluate`, + /// which reads the developer's own `.claude/settings.local.json`: a + /// `Bash(git *)` allow rule turns the expected `Ask` into `Allow` and these + /// tests fail on that machine only (#3146). Pinning the verdict keeps the + /// assertions about the rewrite logic and nothing about the host. mod unattestable_passthrough { - use super::super::{evaluate, RewriteOutcome}; + use super::super::{evaluate_with_verdict, RewriteOutcome}; + use crate::hooks::permissions::PermissionVerdict; #[test] fn test_backtick_substitution_passthrough() { assert_eq!( - evaluate("git status `rm -rf /tmp/x`", &[], &[]), + evaluate_with_verdict( + "git status `rm -rf /tmp/x`", + PermissionVerdict::Default, + &[], + &[] + ), RewriteOutcome::Passthrough ); } @@ -104,7 +151,12 @@ mod tests { #[test] fn test_dollar_substitution_passthrough() { assert_eq!( - evaluate("git status $(rm -rf /tmp/x)", &[], &[]), + evaluate_with_verdict( + "git status $(rm -rf /tmp/x)", + PermissionVerdict::Default, + &[], + &[] + ), RewriteOutcome::Passthrough ); } @@ -112,7 +164,12 @@ mod tests { #[test] fn test_double_quoted_substitution_passthrough() { assert_eq!( - evaluate("git log --pretty=\"$(rm -rf /tmp/x)\"", &[], &[]), + evaluate_with_verdict( + "git log --pretty=\"$(rm -rf /tmp/x)\"", + PermissionVerdict::Default, + &[], + &[] + ), RewriteOutcome::Passthrough ); } @@ -120,7 +177,12 @@ mod tests { #[test] fn test_file_redirect_passthrough() { assert_eq!( - evaluate("git log > /tmp/out.txt", &[], &[]), + evaluate_with_verdict( + "git log > /tmp/out.txt", + PermissionVerdict::Default, + &[], + &[] + ), RewriteOutcome::Passthrough ); } @@ -128,7 +190,7 @@ mod tests { #[test] fn test_fd_dup_redirect_still_rewrites() { assert!(matches!( - evaluate("git status 2>&1", &[], &[]), + evaluate_with_verdict("git status 2>&1", PermissionVerdict::Default, &[], &[]), RewriteOutcome::Ask(_) )); } @@ -136,7 +198,7 @@ mod tests { #[test] fn test_plain_command_still_rewrites() { assert!(matches!( - evaluate("git status", &[], &[]), + evaluate_with_verdict("git status", PermissionVerdict::Default, &[], &[]), RewriteOutcome::Ask(_) )); } diff --git a/src/main.rs b/src/main.rs index b29cf0769b..30ba275b38 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1344,8 +1344,8 @@ fn run_fallback(parse_error: clap::Error) -> Result { match result { Ok(output) => { let exit_code = core::utils::exit_code_from_output(&output, &raw_command); - let stdout_raw = String::from_utf8_lossy(&output.stdout); - let stderr_raw = String::from_utf8_lossy(&output.stderr); + let stdout_raw = core::utils::decode_process_output(&output.stdout); + let stderr_raw = core::utils::decode_process_output(&output.stderr); // Merge stderr into the text to filter when filter_stderr is enabled; // otherwise emit stderr directly so it is always visible. @@ -2676,8 +2676,8 @@ fn run_cli() -> Result { .join() .map_err(|_| anyhow::anyhow!("stderr streaming thread panicked"))??; - let stdout = String::from_utf8_lossy(&stdout_bytes); - let stderr = String::from_utf8_lossy(&stderr_bytes); + let stdout = core::utils::decode_process_output(&stdout_bytes); + let stderr = core::utils::decode_process_output(&stderr_bytes); let full_output = format!("{}{}", stdout, stderr); // Track usage (input = output since no filtering) diff --git a/tests/guard_integration_test.rs b/tests/guard_integration_test.rs index 1cea612b91..6c74de0f6b 100644 --- a/tests/guard_integration_test.rs +++ b/tests/guard_integration_test.rs @@ -160,6 +160,58 @@ fn git_stash_list_no_stashes_emits_empty() { assert_eq!(code, Some(0)); } +#[test] +fn git_log_patch_output_matches_raw_git() { + let dir = init_git_repo(); + std::fs::write( + dir.path().join("history.txt"), + "STRIPE_KEY=sk_live_FAKE1234567890\n", + ) + .expect("write history fixture"); + git_in_dir(dir.path(), &["add", "history.txt"]); + git_in_dir(dir.path(), &["commit", "-q", "-m", "add history fixture"]); + + let raw = Command::new("git") + .args(["log", "-p", "--all"]) + .current_dir(dir.path()) + .output() + .expect("spawn raw git log"); + assert!(raw.status.success()); + + let (rtk_stdout, rtk_stderr, rtk_code) = + rtk_output_in_dir(dir.path(), &["git", "log", "-p", "--all"]); + + assert_eq!(rtk_code, Some(0), "rtk stderr: {rtk_stderr}"); + assert_eq!(rtk_stdout.as_bytes(), raw.stdout.as_slice()); + assert!(rtk_stdout.contains("STRIPE_KEY=sk_live_FAKE1234567890")); +} + +#[test] +fn git_log_dash_p_pathspec_after_double_dash_is_not_patch_flag() { + // Regression: `rtk git log -- -p` must not be misread as the real `-p` + // patch flag. Clap's `trailing_var_arg` strips the literal "--" before + // `run_log` sees `args`, so the pathspec-separator check must restore it + // (via restore_double_dash) before deciding whether to pass through raw + // patch output; otherwise a file literally named "-p" after "--" is + // wrongly treated as a request for `git log -p`. + let dir = init_git_repo(); + std::fs::write(dir.path().join("-p"), "not a diff flag\n").expect("write -p file"); + git_in_dir(dir.path(), &["add", "--", "-p"]); + git_in_dir(dir.path(), &["commit", "-q", "-m", "add dash-p file"]); + + let (stdout, stderr, code) = rtk_output_in_dir(dir.path(), &["git", "log", "--", "-p"]); + + assert_eq!(code, Some(0), "rtk stderr: {stderr}"); + assert!( + !stdout.contains("diff --git") && !stdout.contains("@@"), + "-- -p should stay on RTK's filtered path, not raw patch output: {stdout:?}" + ); + assert!( + stdout.contains("add dash-p file"), + "expected the commit touching the -p pathspec: {stdout:?}" + ); +} + #[test] fn git_stash_show_no_stash_emits_empty_and_propagates_failure() { // Regression: previously printed "Empty stash" and returned Ok(0), masking