Skip to content

fix(cli): handle non-UTF-8 argv in raw-execution fallback - #3027

Closed
georgyia wants to merge 1 commit into
rtk-ai:developfrom
georgyia:fix/non-utf8-argv-panic
Closed

georgyia wants to merge 1 commit into
rtk-ai:developfrom
georgyia:fix/non-utf8-argv-panic

Conversation

@georgyia

Copy link
Copy Markdown

Fixes #3025

Problem

Any argument containing non-UTF-8 bytes aborted rtk before the wrapped command ran:

$ rtk grep -c $'\xc3\xa2\xe2\x80' mojibake.txt
thread 'main' panicked at library/std/src/env.rs:876:51:
called `Result::unwrap()` on an `Err` value: "â\xE2\x80"

Exit 134 (SIGABRT) under the release profile's panic="abort", 101 under a debug build. Native grep handles the same bytes fine and reports 1, so rtk turned a working command into a crash.

Root cause

Not clap. Clap reads argv via args_os() and correctly reports InvalidUtf8 — it never panics. That error routes into run_fallback(), which then collected argv with std::env::args(), panicking on exactly the input that sent it there. The non-UTF-8 error path was itself non-UTF-8-unsafe:

14: rtk::run_fallback
13: <Skip<std::env::Args> as Iterator>::collect::<Vec<String>>
 6: <std::env::Args as Iterator>::next          // per-argument unwrap

std::env::args() unwraps each argument, so it aborts the moment argv holds bytes that are not valid UTF-8.

Fix

  • Read argv with std::env::args_os() and keep the OsString values for execution, so the wrapped tool receives the bytes the user actually typed.
  • Derive lossy String copies for matching, display and tracking (RTK_META_COMMANDS, raw_command, TOML lookup_cmd, record_parse_failure_silent), where text is all that is needed.

Forwarding the raw bytes rather than lossily converting them is the deliberate part: to_string_lossy alone would stop the abort, but the wrapped tool would then search for replacement characters and silently report zero matches — trading a loud crash for a wrong answer.

Scoped to the one reachable site. core::args_utils::restore_double_dash has the same std::env::args() call, but every non-UTF-8 argv is rejected by clap and funnelled into run_fallback before those callers run — verified by backtrace on git diff --, cargo test -- and grep -c. It is a latent hazard rather than a live bug, so it is left for a follow-up rather than changed without a test that can fail.

Known remaining edge: a command name (argv[0]) that is itself non-UTF-8 still degrades to a clean 127 instead of resolving, because resolved_command() takes &str. Widening that signature ripples through many callers, and the outcome is graceful rather than an abort, so it is out of scope here.

Verification

  • Original panic reproduced against a pre-fix binary (env.rs:876:51, matching the issue), then confirmed gone.
  • Post-fix rtk grep -c returns 1 — identical to LC_ALL=C grep -c on the same bytes.
  • All 3 new tests in tests/non_utf8_argv_test.rs fail on unfixed code and pass with the fix: no abort, byte-forwarding vs native grep, and a clean 127 on the spawn-failure path.
  • No regression: old vs new binary output is byte-identical on ASCII fallback paths (echo, ls, missing command, gain --badtypo, git status).
  • cargo fmt --all --check clean, cargo clippy --all-targets zero warnings, cargo test --all 2496 passed / 0 failed.

Verified on Linux (rust:latest); the tests are #![cfg(unix)] since they build argv from raw bytes via OsStringExt::from_vec.

Test plan

  • cargo fmt --all && cargo clippy --all-targets && cargo test --all
  • Manual testing: rtk grep -c $'\xc3\xa2\xe2\x80' <file> inspected against native grep
  • Regression tests added, confirmed failing before the fix
  • Token savings: N/A (crash fix, no filter change)

Happy to contribute here — rtk is a genuinely interesting project, and I'm glad to adjust anything in this PR if you'd prefer a different scope or approach.

rtk aborted (SIGABRT, exit 134) when any argument contained bytes that
are not valid UTF-8, before the wrapped command ran.

Clap is not the culprit: it reads argv via args_os() and correctly reports
InvalidUtf8. That error routes into run_fallback(), which then collected
argv with std::env::args() — panicking on exactly the input that sent it
there. The non-UTF-8 error path was itself non-UTF-8-unsafe.

Read argv with args_os() and keep the OsString values for execution, so the
wrapped tool receives the bytes the user typed. Lossy copies still drive
matching, display and tracking, where text is all that is needed.

Fixes rtk-ai#3025
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

kylehgc added a commit to kylehgc/rtk that referenced this pull request Jul 17, 2026
… tests

Amendment to the adopted upstream rtk-ai#3027 fix, per the repo test rule
(rust-patterns.md: tests use expect("reason"), never bare unwrap).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kylehgc added a commit to kylehgc/rtk that referenced this pull request Jul 17, 2026
fix(cli): adopt upstream rtk-ai#3027 — handle non-UTF-8 argv in raw-execution fallback

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @georgyia thanks for your contribution. You'll have to sign the CLA before your PR can be merged. Also minor nitpicks


#[test]
fn non_utf8_pattern_does_not_abort() {
let d = tempfile::tempdir().unwrap();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated in rust-patterns.md:

tests use expect("reason"), never bare unwrap

@KuSh KuSh self-assigned this Jul 25, 2026
@KuSh

KuSh commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Ping @georgyia. Aside from the nitpick, and once you’ve signed the CLA, this one can be merged

@KuSh

KuSh commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Closing this one in favour of #3224, which makes the same args_os fix in run_fallback, also covers the matching env::args call in args_utils.rs, and has its CLA signed. The root-cause write-up here was genuinely useful; further comments are welcome on #3224.

@KuSh KuSh closed this Sep 6, 2026
@KuSh KuSh added the duplicate This issue or pull request already exists label Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic (SIGABRT, exit 134) on non-UTF-8 bytes in argv — std::env::args() unwrap

3 participants