Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe Claude Code driver now bounds stderr diagnostics at a fixed byte cap while preserving UTF-8 boundaries. Stderr draining uses the new helper. Unit tests cover multibyte input, repeated chunks, below-cap accumulation, and exact ASCII caps. ChangesStderr diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change prevents stderr truncation from panicking on multibyte UTF-8 characters while preserving bounded output; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Your free Security trial is over. An organization admin can activate billing to continue. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
How this change flows1 changed behaviour across 8 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 42 further behaviours left out to keep the diagram readable. flowchart LR
n0["run_turn<br/>changed"]:::changed
n1["join"]:::impacted
n2["append_system_prompt_args"]:::impacted
n3["build_stdin"]:::impacted
n4["feed_bytes"]:::impacted
n5["vec"]:::impacted
n6["expect"]:::impacted
n0 -->|calls| n1
n0 -->|calls| n2
n0 -->|calls| n3
n0 -->|calls| n4
n0 -->|calls| n5
n0 -->|calls| n6
n2 -->|calls| n1
n2 -->|calls| n5
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
130ce02 to
fa044d3
Compare
|
@ntdatt812 — my mistake, and I'm sorry: I closed this PR by accident and I cannot undo it from my side. Here is exactly what happened and exactly how to get it back. What happened. I was rebasing this branch onto current Nothing is lost. Your work, rebased onto To restore, one of: # from your clone
git remote add m3ga https://github.com/M3gA-Mind/openhuman.git
git fetch m3ga recover/5719-stderr-utf8
git checkout fix/claude-code-stderr-utf8
git reset --hard m3ga/recover/5719-stderr-utf8
git push --force-with-leaseThat puts commits back on the branch, after which this PR can be reopened (the Reopen button should light up once the branch is non-empty). If it will not reopen, open a fresh PR from the same branch and I will re-link it. What the rebase actually changed — one conflict, in
#[cfg(test)]
#[path = "driver_tests.rs"]
mod tests;and moves your The fix itself is right and worth landing: |
|
No harm done — thank you for writing down exactly what happened and where the work was. That is a much better outcome than a silent force-push. Branch restored. GitHub still refused to reopen this one — its recorded head is I did not take
Your recovery branch was cut against Verification on the rebased head: the three One thing worth flagging for whoever else you are rebasing for: the auto-close revoked maintainer-can-modify, so the same accident on any other PR of mine will need the same recovery path. Happy to do the rebases myself if that is easier — for the other conflicting ones you reviewed today, I am working through them. |
Sibling of #5718, same file family, different failure mode. Independent of it — either can land first.
The defect
The stderr drain caps its accumulator with a raw byte index (
driver.rs:417-427):String::truncatetakes a byte index and panics when it is not a character boundary. Proved withrustcon the exact shape:So any stderr over 16 KiB whose 16384th byte lands inside a multi-byte character aborts the drain task.
Why it is invisible
The panic happens inside
tokio::spawn, and the join isstderr_task.await.unwrap_or_default()(driver.rs:483). TheJoinErroris swallowed into an empty string, sodriver.rs:486reportsThe operator loses the entire error output for the turn, at exactly the moment they need it — a failing turn with a lot of stderr is the case most likely to exceed 16 KiB.
The fix
The repo already owns the right helper:
util::text::utf8_safe_prefix_at_byte_boundary, whose doc comment describes this exact problem, and whichtool_result_artifacts/mod.rs:58and:432already use correctly. This call site simply bypassed it — I checked the other two rawString::truncatecalls insrc/(mcp/server/tools/params.rs:496,sandbox/cwd_jail/windows.rs:425) and both operate on provably ASCII input, so this was the only one.Extracted as a pure
push_bounded(&mut String, &str, usize)so it can be asserted without spawning a process, matching how every other testable piece of this file is factored.Verification
Mutation-checked, after confirming the edit applied: restoring
acc.truncate(max_bytes)reproduces the original panic inside the test —— so the test pins the panic itself, not merely the length.
Tests
run_turnis untested (it spawns a process), which is why the bound was never exercised. Three cases on the extracted helper:é, which must back up rather than panic;Plus a below-cap case and an ASCII exact-cap case.
Summary by CodeRabbit