Skip to content

fix(stream): decode lossily instead of dropping lines on invalid UTF-8 - #2997

Merged
KuSh merged 2 commits into
rtk-ai:developfrom
albatrossflyon-coder:fix/utf8-line-drop-in-stream-filters
Aug 12, 2026
Merged

KuSh merged 2 commits into
rtk-ai:developfrom
albatrossflyon-coder:fix/utf8-line-drop-in-stream-filters

Conversation

@albatrossflyon-coder

Copy link
Copy Markdown
Contributor

Summary

Fixes #2994.

run_streaming's six call sites all read child stdout/stderr with:

BufReader::new(stdout).lines().map_while(Result::ok)

BufRead::lines() returns Err for a line that isn't valid UTF-8 (OEM/ANSI
bytes from a non-English-locale Windows tool, per the issue's cp850 repro).
map_while stops iteration at the first None — so one non-UTF-8 line
doesn't just get dropped, it silently discards every line emitted after
it too. A failing build whose error text happens to contain one non-UTF-8
byte can end up looking like a clean, silent success even though the exit
code is non-zero.

Fix

Added read_lines_lossy(), which reads raw bytes via BufRead::read_until
and decodes each line with String::from_utf8_lossy (invalid bytes become
U+FFFD) instead of erroring out of the iterator. Replaced all 6
.lines().map_while(Result::ok) call sites in run_streaming with it —
this is the single shared path every filter routes through, so the fix
covers all callers at once.

rtk pipe (cmds/system/pipe_cmd.rs) already fails loudly on invalid
UTF-8 stdin rather than silently, per the issue's own note — left as-is,
out of scope here.

Testing

  • cargo test: 2448 passed, 8 ignored, 0 failed (no regressions)
  • 4 new unit tests on read_lines_lossy directly, including one that
    reproduces the exact failure mode from the issue: an ASCII line, a line
    with an embedded invalid byte, then another ASCII line — asserts all 3
    lines survive instead of the third silently vanishing
  • cargo clippy --all-targets -- -D warnings: clean
  • cargo fmt --check: clean

BufRead::lines().map_while(Result::ok) returns Err on a non-UTF-8 line
(e.g. OEM/ANSI bytes from a non-English-locale Windows tool), and
map_while stops at the first None — silently discarding every line
after the bad one too, not just the bad one. A failing build command
whose error text contains a single non-UTF-8 byte produces output that
looks like a clean success.

Add read_lines_lossy(), which reads raw bytes and decodes each line
with String::from_utf8_lossy (invalid bytes become U+FFFD) instead of
erroring, and use it at all 6 call sites in run_streaming instead of
the truncating lines()/map_while(Result::ok) pattern.

Fixes rtk-ai#2994
@CLAassistant

CLAassistant commented Jul 14, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@albatrossflyon-coder

Copy link
Copy Markdown
Contributor Author

Hi — just checking in on this one, it's been open about 9 days with no review yet. Happy to make any changes if there's feedback, or let me know if anything's blocking it. Thanks for maintaining rtk!

Comment thread src/core/stream.rs Outdated
Comment thread src/core/stream.rs Outdated
Comment thread src/core/stream.rs
@KuSh KuSh self-assigned this Aug 11, 2026
KuSh's review on this PR flagged two real issues in read_lines_lossy:
- Ok(0) | Err(_) => None treated a genuine I/O error the same as clean
  EOF, silently truncating output on a real read failure -- exactly
  the "failing build looks clean" class of bug this PR exists to fix,
  just relocated rather than removed.
- A fresh Vec was allocated every iteration on a hot streaming path
  instead of reusing std's own buffered split.

Replaced the hand-rolled read_until loop with BufReader::split(b'\n'),
per the reviewer's suggested implementation: reuses std instead of
reimplementing it (except CR-stripping, which split() doesn't do),
and now surfaces a genuine I/O error to stderr instead of silently
conflating it with EOF.

Added a test with a Read impl that yields good lines then a real
error, confirming the error path doesn't panic or hang and the
already-read lines are still preserved.
@albatrossflyon-coder

Copy link
Copy Markdown
Contributor Author

Thanks for the review — pushed a fix addressing both issues in ae5d1ae:

  • Ok(0) | Err(_) => None was treating a genuine I/O error the same as clean EOF, silently truncating output on a real read failure. Fixed by switching to your suggested BufReader::split(b'\n') approach, which surfaces the error via eprintln! instead of conflating it with EOF.
  • The per-iteration Vec::new() allocation is gone with the same change — now reusing std's own buffered split instead of hand-rolling the read loop.

Added a test with a custom Read impl that yields good lines then a real I/O error, confirming the already-read lines are preserved and the error path doesn't panic or hang.

Verified clean against cargo fmt --all -- --check, cargo clippy --all-targets, and the full cargo test --all suite (2,449 tests, 0 failed) before pushing.

@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.

Thanks LGTM!

@KuSh
KuSh merged commit 1989899 into rtk-ai:develop Aug 12, 2026
11 checks passed
@rtk-release-bot rtk-release-bot Bot mentioned this pull request Aug 12, 2026
social4hyq pushed a commit to social4hyq/homebrew-core that referenced this pull request Sep 20, 2026
rtk 0.46.0

Created-by: HarmonybrewBot
Commit-by: HarmonybrewBot
Merged-by: HarmonybrewBot
Description: Created by `brew bump`

---

Created with `brew bump-formula-pr`.<details>
  <summary>release notes</summary>
  <pre>## [0.46.0](rtk-ai/rtk@v0.45.0...v0.46.0) (2026-08-26)


### Features

- find: dispatch on find's grammar; compress find output for unmodeled predicates ([#3603](rtk-ai/rtk#3603))
- find: tee tail hint when rtk imposes the result cap ([#3603](rtk-ai/rtk#3603))

### Bug Fixes

- find: never-worse guard, recovery hint, and dispatch on find's grammar ([#3603](rtk-ai/rtk#3603))
- git: don't misdetect a value-taking option's argument as a patch flag ([#3575](rtk-ai/rtk#3575))
- cicd: stop benchmark.sh deleting the tracked scripts/benchmark harness ([#3595](rtk-ai/rtk#3595))
- tee: hash long recovery-file slugs to prevent collisions and shorten hints ([#3266](rtk-ai/rtk#3266))
- benchmark: avoid negative curl/cargo cases that fail the benchmark job ([#3430](rtk-ai/rtk#3430))
- test: accept both Ask and Allow verdicts in rewrite tests ([#3147](rtk-ai/rtk#3147)) — Closes [#3146](rtk-ai/rtk#3146)
- core: decode process output using Windows console code page ([#2717](rtk-ai/rtk#2717)) — Closes [#2452](rtk-ai/rtk#2452)
- git: preserve patch output from log commands ([#2951](rtk-ai/rtk#2951)) — Closes [#2944](rtk-ai/rtk#2944)
- discover: sanitize drive-letter colon so Windows discover finds sessions ([#2952](rtk-ai/rtk#2952)) — Closes [#2919](rtk-ai/rtk#2919)
- stream: decode lossily instead of dropping lines on invalid UTF-8 ([#2997](rtk-ai/rtk#2997)) — Closes [#2994](rtk-ai/rtk#2994)

### Other

- test(find): use the platform temp dir instead of /tmp ([#3717](https://github.com/rtk-ai/rtk/pull/3717))</pre>
  <p>View the full release notes at <a href="https://github.com/rtk-ai/rtk/releases/tag/v0.46.0">https://github.com/rtk-ai/rtk/releases/tag/v0.46.0</a>.</p>
</details>
<hr>

See merge request: Harmonybrew/homebrew-core!17826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Filters silently drop non-UTF-8 lines: shell/build errors vanish while exit code stays non-zero (non-English Windows; likely root cause of #1936)

3 participants