Repository navigation
fix: route mip trace output to stderr instead of stdout - #1315
Conversation
The mip tracing subscriber was initialised with ot::StdoutWriter, so any trace event fired during a command's execution was written to stdout. For `mip dump --packages`, whose stdout carries a machine-readable JSON array, a trace line emitted ahead of the payload corrupted the stream and broke any consumer parsing it through a pipe. The lines also carried ANSI escapes unconditionally, because tracing-subscriber's default `ansi` feature stays enabled here. Add ot::StderrWriter, the stderr counterpart of StdoutWriter that keeps the same MultiProgress suspension so logs and progress bars don't clobber each other, and point the mip subscriber at it. Gate ANSI colouring on stderr being a terminal so redirected diagnostics stay plain text.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe CLI now sends tracing output to stderr and enables ANSI formatting only when stderr is a terminal. The output crate adds and re-exports a progress-aware ChangesStderr output integration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change keeps machine-readable command output on stdout and routes diagnostics to stderr, with plain text when redirected. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the problem, implementation, and verification results. It includes summary and testing information, although it uses a Verification heading instead of Testing and omits the checklist.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABNw930g
mip dump --packagesprints a machine-readable JSON array on stdout, but the CLI's tracing subscriber was initialised withot::StdoutWriter, so any trace event fired while a command ran was written to stdout as well. A trace line emitted ahead of the JSON corrupted the stream and broke any consumer parsing the dump through a pipe. Those lines also carried ANSI escapes unconditionally, becausetracing-subscriber's defaultansifeature stays enabled, so colour codes appeared even on a non-terminal pipe.This adds
ot::StderrWriter, the stderr counterpart ofStdoutWriterthat keeps the sameMultiProgresssuspension so trace logs and progress bars don't clobber each other, and points themipsubscriber at it. ANSI colouring is now gated on stderr being a terminal, so redirected diagnostics stay plain text. stdout now carries only the command's payload.No test was added: the change is subscriber wiring in
main()plus a delegating writer mirroring the (untested)StdoutWriter, and the harness has no natural unit surface for assertingmain's output stream.Verification
cargo fmt --all --check— cleancargo clippy --workspace --locked -- -D warnings— clean, no warningscargo build --workspace --locked— Finished, okcargo test --workspace --locked— ok, all suites passed (exit 0)Summary by CodeRabbit