fix(core): decode per line, cover OEM code pages, and centralize on exec_capture

Addresses the inline review on #2717.

decode_process_output
- Decode a line at a time instead of reinterpreting the whole buffer at
  the first bad byte. Valid UTF-8 lines keep their bytes; only lines that
  fail UTF-8 validation go through the code page, so one stray byte no
  longer mangles output that was almost entirely UTF-8. The line is the
  unit because a byte run is not one: GB18030's four-byte sequences embed
  bytes in the ASCII digit range, so any rule that ends a run below 0x80
  splits them. \n cannot appear as a trail byte in any encoding handled
  here, and a process does not switch encoding mid-line.
- A code page result is only accepted when it decodes cleanly, so a UTF-8
  line with a corrupt byte falls back to lossy UTF-8 rather than mojibake.
- Replace the hand-written code page table with the codepage crate, as
  suggested. That also fixes 54936, which was mapped to GBK and now
  correctly resolves to gb18030.
- Add oem_cp for the legacy OEM/DOS pages (437, 850, 852, …) that plain
  cmd.exe still defaults to in many locales. encoding_rs implements only
  WHATWG encodings, so codepage alone returns None for them.
- Fall back to GetACP when GetConsoleOutputCP reports no console, which
  is the piped case rtk normally runs in, and warn once instead of
  falling back to lossy silently.
- Cache the code page lookup in a OnceLock.
- The mapping and the walk take the code page as a parameter, so they are
  compiled and unit-tested on every platform rather than only Windows.

Call sites
- Route the remaining production sites through stream::exec_capture and
  exec_capture_stdin rather than decoding at each one, so future callers
  inherit decoding. git commit keeps inherited stdin via the _stdin
  variant. Test-only sites go back to from_utf8_lossy: they assert on
  rtk's own UTF-8 output, where a console code page has no meaning.
- Decode the streamed path (read_lines_lossy) too — the OEM/ANSI lines
  its comment describes were still going straight to U+FFFD.
- curl keeps its body on from_utf8_lossy: a response body is a network
  payload whose encoding comes from the HTTP charset, not the local
  console, and non-UTF-8 bodies already take the binary passthrough for
  #1087. Only curl's own stderr is code page decoded.

git commit summary parsing
- parse_commit_output sliced from byte 1, which panics when the first
  line starts with a multi-byte character — git prints hook output before
  its summary, and a lossily decoded line starts with a multi-byte
  U+FFFD. Locate the bracket pair with find instead, so both indices are
  character boundaries.

Verified: unit tests for the walk, GBK, gb18030, CP437/850, mixed lines,
truncated input and every byte value; a test pinning that output without
a code page stays byte-identical to from_utf8_lossy; and the Windows-only
lookup cross-compiled for x86_64-pc-windows-msvc.
This commit is contained in:
guyoron1
2026-08-15 08:20:24 +03:00
parent b35ff3a374
commit f496f59b77
10 changed files with 431 additions and 162 deletions
Generated
+45 -1
View File
@@ -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]]
@@ -756,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"
@@ -780,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"
@@ -900,6 +936,7 @@ dependencies = [
"automod",
"chrono",
"clap",
"codepage",
"colored",
"dirs",
"encoding_rs",
@@ -907,6 +944,7 @@ dependencies = [
"getrandom 0.4.2",
"ignore",
"libc",
"oem_cp",
"quick-xml",
"regex",
"rusqlite",
@@ -1080,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"
+12 -2
View File
@@ -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"
@@ -38,8 +46,10 @@ automod = "1"
libc = "0.2"
[target.'cfg(windows)'.dependencies]
encoding_rs = "0.8"
windows-sys = { version = "0.59", features = ["Win32_System_Console"] }
windows-sys = { version = "0.59", features = [
"Win32_System_Console",
"Win32_Globalization",
] }
[build-dependencies]
toml = "0.8"
+32 -34
View File
@@ -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 = crate::core::utils::decode_process_output(&output.stdout);
let stderr = crate::core::utils::decode_process_output(&output.stderr);
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<CaptureResult> {
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 = crate::core::utils::decode_process_output(&output.stdout);
let stderr = crate::core::utils::decode_process_output(&output.stderr);
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<i32> {
eprintln!("Running: aws s3 ls {}", extra_args.join(" "));
}
let output = cmd.output().context("Failed to run aws s3 ls")?;
let stdout = crate::core::utils::decode_process_output(&output.stdout);
let stderr = crate::core::utils::decode_process_output(&output.stderr);
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 = crate::core::utils::decode_process_output(&output.stdout);
let stderr = crate::core::utils::decode_process_output(&output.stderr);
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 {
+9 -2
View File
@@ -43,8 +43,10 @@ pub fn run(args: &[String], verbose: u8) -> Result<i32> {
// 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() {
// 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 = crate::core::utils::decode_process_output(&output.stdout);
let stdout_str = String::from_utf8_lossy(&output.stdout);
let msg = if stderr_str.trim().is_empty() {
stdout_str.trim().to_string()
} else {
@@ -72,7 +74,12 @@ pub fn run(args: &[String], verbose: u8) -> Result<i32> {
return Ok(exit_code);
}
let raw = crate::core::utils::decode_process_output(&output.stdout);
// 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);
+64 -24
View File
@@ -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 {
@@ -1009,15 +1009,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 +1041,17 @@ fn run_commit(args: &[String], verbose: u8, global_args: &[String]) -> Result<i3
eprintln!("{}", original_cmd);
}
let output = build_commit_command(args, global_args)
.stdin(Stdio::inherit())
.output()
// stdin is inherited so an interactive editor, GPG passphrase prompt or
// credential helper still reaches the terminal.
let CaptureResult {
stdout,
stderr,
exit_code,
} = exec_capture_stdin(&mut build_commit_command(args, global_args))
.context("Failed to run git commit")?;
let stdout = crate::core::utils::decode_process_output(&output.stdout);
let stderr = crate::core::utils::decode_process_output(&output.stderr);
let exit_code = exit_code_from_output(&output, "git commit");
let raw_output = format!("{}\n{}", stdout, stderr);
match classify_commit_outcome(output.status.success(), &stdout, exit_code) {
match classify_commit_outcome(exit_code == 0, &stdout, exit_code) {
CommitOutcome::Ok(compact) => {
println!("{}", compact);
timer.track(&original_cmd, "rtk git commit", &raw_output, &compact);
@@ -2851,6 +2860,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";
@@ -2956,7 +2996,7 @@ no changes added to commit (use "git add" and/or "git commit -a")
.args(["branch", "--list", branch])
.output()
.expect("git branch --list should work");
let stdout = crate::core::utils::decode_process_output(&output.stdout);
let stdout = String::from_utf8_lossy(&output.stdout);
assert!(
stdout.contains(branch),
"Branch '{}' was not created. run_branch silently swallowed the creation.",
@@ -2977,7 +3017,7 @@ no changes added to commit (use "git add" and/or "git commit -a")
.args(["branch", "--list", branch])
.output()
.expect("git branch --list should work");
let stdout = crate::core::utils::decode_process_output(&output.stdout);
let stdout = String::from_utf8_lossy(&output.stdout);
assert!(
stdout.contains(branch),
"Branch '{}' was not created from commit.",
@@ -3080,8 +3120,8 @@ no changes added to commit (use "git add" and/or "git commit -a")
);
// Message should be on stderr, not stdout
let stderr = crate::core::utils::decode_process_output(&output.stderr);
let stdout = crate::core::utils::decode_process_output(&output.stdout);
let stderr = String::from_utf8_lossy(&output.stderr);
let stdout = String::from_utf8_lossy(&output.stdout);
assert!(
stderr.to_lowercase().contains("not a git repository"),
"Expected 'not a git repository' on stderr, got stderr={:?}, stdout={:?}",
+19 -29
View File
@@ -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<i32> {
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 = crate::core::utils::decode_process_output(&output.stdout);
let stderr = crate::core::utils::decode_process_output(&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<i32> {
&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();
let mut cmd = resolved_command("go");
cmd.arg("tool").arg("golangci-lint").arg("--version");
match output {
Ok(o) => {
let stdout = crate::core::utils::decode_process_output(&o.stdout);
let stderr = crate::core::utils::decode_process_output(&o.stderr);
let version_text = if stdout.trim().is_empty() {
&*stderr
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<i32> {
}
}
let output = cmd
.output()
.context("Failed to run go tool golangci-lint")?;
let stdout = crate::core::utils::decode_process_output(&output.stdout);
let stderr = crate::core::utils::decode_process_output(&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<i32> {
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 })
+4 -4
View File
@@ -260,7 +260,7 @@ fn main() {{
.expect("failed to run rtk read");
assert!(output.status.success());
let stdout = crate::core::utils::decode_process_output(&output.stdout);
let stdout = String::from_utf8_lossy(&output.stdout);
assert!(stdout.contains("alpha"), "first file content missing");
assert!(stdout.contains("charlie"), "second file content missing");
}
@@ -280,8 +280,8 @@ fn main() {{
.expect("failed to run rtk read");
assert!(!output.status.success(), "should exit non-zero on missing file");
let stdout = crate::core::utils::decode_process_output(&output.stdout);
let stderr = crate::core::utils::decode_process_output(&output.stderr);
let stdout = String::from_utf8_lossy(&output.stdout);
let stderr = String::from_utf8_lossy(&output.stderr);
assert!(stdout.contains("valid content"), "valid file should still be printed");
assert!(stderr.contains("rtk_nonexistent_file"), "should report missing file on stderr");
}
@@ -298,7 +298,7 @@ fn main() {{
.output()
.expect("failed to run rtk read");
let stderr = crate::core::utils::decode_process_output(&output.stderr);
let stderr = String::from_utf8_lossy(&output.stderr);
assert!(
stderr.contains("stdin specified more than once"),
"should warn about duplicate stdin, got stderr: {}",
+9 -3
View File
@@ -6,8 +6,9 @@ use std::sync::mpsc;
#[cfg(test)]
use regex::Regex;
/// Read `reader` line by line, decoding each line lossily (invalid UTF-8
/// bytes become U+FFFD) instead of erroring.
/// 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
@@ -16,6 +17,11 @@ use regex::Regex;
/// 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<Item = String> {
BufReader::new(reader).split(b'\n').filter_map(|res| {
let mut buf = match res {
@@ -28,7 +34,7 @@ fn read_lines_lossy(reader: impl Read) -> impl Iterator<Item = String> {
if buf.last() == Some(&b'\r') {
buf.pop();
}
Some(String::from_utf8_lossy(&buf).into_owned())
Some(super::utils::decode_process_output(&buf))
})
}
+236 -62
View File
@@ -498,60 +498,127 @@ pub fn human_bytes(bytes: u64) -> String {
/// Decode child process output bytes, respecting the Windows console code page.
///
/// On all platforms, tries UTF-8 first. On Windows, falls back to the console's
/// output code page (e.g., GBK for Chinese locale) via `encoding_rs`. On
/// non-Windows or unknown code pages, falls back to lossy UTF-8.
/// 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 {
if let Ok(s) = std::str::from_utf8(bytes) {
return s.to_owned();
// 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()),
}
}
#[cfg(windows)]
{
let cp = windows_console_output_cp();
if let Some(encoding) = codepage_to_encoding(cp) {
let (cow, _, _) = encoding.decode(bytes);
return cow.into_owned();
/// 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<u16>) -> 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)),
}
}
String::from_utf8_lossy(bytes).into_owned()
out
}
#[cfg(windows)]
fn windows_console_output_cp() -> u32 {
#[allow(unsafe_code)]
// nosemgrep: unsafe-block — read-only Win32 API, no memory or thread safety risk
unsafe {
windows_sys::Win32::System::Console::GetConsoleOutputCP()
/// 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<u16>) -> 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<u16> {
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 codepage_to_encoding(cp: u32) -> Option<&'static encoding_rs::Encoding> {
let label = match cp {
936 | 54936 => "gbk",
950 => "big5",
932 => "shift_jis",
949 => "euc-kr",
874 => "windows-874",
1250 => "windows-1250",
1251 => "windows-1251",
1252 => "windows-1252",
1253 => "windows-1253",
1254 => "windows-1254",
1255 => "windows-1255",
1256 => "windows-1256",
1257 => "windows-1257",
1258 => "windows-1258",
28591 => "iso-8859-1",
28592 => "iso-8859-2",
20866 => "koi8-r",
21866 => "koi8-u",
65001 => return None,
_ => return None,
};
encoding_rs::Encoding::for_label(label.as_bytes())
fn output_codepage() -> Option<u16> {
static CODEPAGE: OnceLock<Option<u16>> = 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)]
@@ -1127,34 +1194,141 @@ mod tests {
assert!(!result.is_empty());
}
#[cfg(windows)]
// `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_process_output_gbk() {
// Test the encoding path directly: codepage 936 (GBK) should decode
// GBK bytes correctly regardless of the CI runner's actual code page.
let gbk_bytes: &[u8] = &[0xB2, 0xE2, 0xCA, 0xD4];
let encoding = codepage_to_encoding(936).expect("GBK encoding should be known");
let (decoded, _, _) = encoding.decode(gbk_bytes);
assert_eq!(decoded, "测试");
fn test_decode_mixed_gbk_run() {
assert_eq!(decode_mixed(&[0xB2, 0xE2, 0xCA, 0xD4], Some(936)), "测试");
}
#[cfg(windows)]
/// GB18030 (54936) must not be treated as plain GBK: it has to keep its
/// own table so 4-byte sequences decode.
#[test]
fn test_codepage_to_encoding_known() {
assert!(codepage_to_encoding(936).is_some());
assert!(codepage_to_encoding(932).is_some());
assert!(codepage_to_encoding(949).is_some());
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")
);
}
#[cfg(windows)]
/// 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_codepage_to_encoding_utf8_returns_none() {
assert!(codepage_to_encoding(65001).is_none());
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}'));
}
#[cfg(windows)]
/// 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_codepage_to_encoding_unknown_returns_none() {
assert!(codepage_to_encoding(99999).is_none());
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<u8> = (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
);
}
}
}
+1 -1
View File
@@ -2762,7 +2762,7 @@ mod tests {
!output.status.success(),
"Should exit non-zero (no rewrite)"
);
let stderr = crate::core::utils::decode_process_output(&output.stderr);
let stderr = String::from_utf8_lossy(&output.stderr);
assert!(
stderr.contains("RTK_DISABLED=1 detected"),
"Should warn on stderr, got: {}",