Repository navigation
refactor: share the codespan stderr renderer - #1550
Conversation
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 2 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 2 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 38 minutes for your next included review. Limit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe common crate now provides a shared, color-aware stderr reporting function. The decode, graph, and mctx crates use this function instead of creating and locking ChangesDiagnostic output
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to The refactor preserves diagnostic output behavior and is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
A rabbit found one stderr door Comment |
decode, graph, and mctx each carried an identical report_to_stderr that built a StandardStream::stderr(ColorChoice::Auto). Move that terminal setup into common::report_to_stderr, which takes the formatting as a closure, and have the three methods delegate. No output changes: the per-type report_to bodies and the exact stderr invocation are untouched.
c3a6e04 to
8eeae28
Compare
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — APPROVE: all personas completed and nothing survives above severity 'nit' (0 finding(s) kept).
Scope matches the title: the diff is exactly the 7 files in pr.json, and the only thing it does is move the StandardStream::stderr(ColorChoice::Auto).lock() terminal setup out of three byte-identical report_to_stderr bodies into common::report_to_stderr, plus the codespan-reporting Cargo dependency it requires. Clobber none: git log on the changed files shows the prior commits are unrelated (sandbox2 network merge #1422; #1413 which added a report_to AttrTooDeep test, untouched). Correctness verified by reading: the helper reproduces the exact original setup — same stream, same ColorChoice::Auto, same lock lifetime — and each delegation (common::report_to_stderr(|w| self.report_to(w))) hands the same StandardStreamLock-as-dyn WriteColor to the existing report_to, whose signature (&mut dyn codespan_reporting::term::termcolor::WriteColor) matches the closure argument exactly, so no rendered-byte change is possible. The dependency change is minimal: codespan-reporting 0.13.1 was already in the lockfile (used by nickel-lang-core and others); the Cargo.lock delta only appends it to common's dep list, no resolution change. All three consumers already depend on common, so the claimed 'no new crate edge' holds. The PR-chosen home in common over diagnostics is defensible given the edge analysis. The only other terminal setup in the workspace, mip/src/cmd_status.rs, is a stdout status renderer — correctly out of scope. Not checked: compilation (cargo build/clippy/test are out of bounds); the change is type-checked from the source only, and the types line up (StandardStreamLock implements WriteColor), so compile risk looks minimal. Minor observation, not a finding: the pre-existing "writes to standard out" doc comments on the three report_to_stderr methods were wrong before this PR (they write to stderr) and this PR does not touch them; out of scope for a no-behavior-change refactor.
What ran
6 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. git show 28cbda2d:crates/{decode/src/error.rs,graph/src/lib.rs,mctx/src/error.rs} | sed body — exit 0 · self-reported
All three pre-PR bodies were byte-identical
self.report_to(&mut StandardStream::stderr(ColorChoice::Auto).lock()); the new helper reproduces that setup and the closure argument is already&mut dyn WriteColor, exactly what eachreport_totakes, so behavior is preserved.
2. rg -n 'StandardStream::(stderr|stdout)|ColorChoice::Auto' crates — exit 0 · self-reported
After the change only
crates/common/src/report.rs:12usesStandardStream::stderr(ColorChoice::Auto); the sole other terminal setup ismip/src/cmd_status.rs:17, a stdout status renderer, correctly out of scope. The three copies were the only instances of the stderr pattern.
3. grep common|codespan in crates/{graph,decode,mctx}/Cargo.toml — exit 0 · self-reported
graph, decode, and mctx all already declare
common.workspace = trueandcodespan-reporting.workspace = true, confirming the helper adds no new crate edge and no consumer dependency change.
4. git log -8 -- changed-files; git diff cb384e31 HEAD --stat — exit 0 · self-reported
Head commit 8eeae28 touches exactly the 7 files in pr.json; prior commits on these paths are the unrelated sandbox2 network merge and a decode report_to test addition. No recent fix is reverted.
5. grep codespan-reporting Cargo.lock; sed Cargo.lock common entry — exit 0 · self-reported
codespan-reporting 0.13.1 already present in the lockfile (shared with nickel-lang-core et al.); the diff only adds it to common's dependency list, so no version resolution change.
6. rg 'fn report_to_stderr|fn report_to\b' crates — exit 0 · self-reported
Exactly three
report_to_stderrimplementations exist (decode, graph, mctx), all refactored; mip's own error reporting flows through mctx::Error::report_to_stderr, so no caller is left on an old copy.
minimal-review · schema minimal-review/v1 · persona generalist · model deepseek/deepseek-v4-flash-0731:nitro via claude-code · prompt review-prompt/v2 · head 8eeae286dbf2 · verdict APPROVE · 0 finding(s), 0 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — APPROVE: all personas completed and nothing survives above severity 'nit' (0 finding(s) kept).
Scope matches the title exactly: 7 files, 21 insertions / 6 deletions, every hunk traces to sharing the codespan stderr renderer. common/src/report.rs is the only place the terminal setup now lives, and the three report_to_stderr methods are one-line delegations. Clobber none: git log on the touched files shows the last substantive change to these bodies was this refactor itself (8eeae28) plus a merge of main; nothing reverted. Correctness verified by structure (build tools are out of bounds, so nothing was compiled): the helper body is byte-identical to each of the three replaced bodies, and all three report_to signatures take &mut dyn WriteColor, so the closure delegation is type-correct and output is unchanged. Dependency claims hold — decode, graph, mctx already depend on common and on codespan-reporting 0.13 (same workspace pin, no version bump; the Cargo.lock change is exactly one added edge). rg across the repo finds no other copy of the stderr setup left behind: every remaining report_to_stderr() call site (orchestrator, minimald, mip) routes through the three refactored types. Two nits not raised as findings because they are out of the diff's reach or sub-actionable: the unchanged doc comments on report_to_stderr in decode and graph still say "standard out" (they write to stderr), and mip/src/cmd_status.rs:17 has a separate StandardStream::stdout(ColorChoice::Auto) setup that could share a sibling helper but is a single instance with a different purpose. Change is good as-is.
What ran
5 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. rg -n 'StandardStream::stderr' --type rust — exit 0 · self-reported
Only match is the new crates/common/src/report.rs:12; all three copies of the inline setup are gone, and no fourth copy of the stderr pattern exists anywhere in the repo.
2. rg -n 'ColorChoice::Auto|report_to_stderr' --type rust — exit 0 · self-reported
Every remaining report_to_stderr() call site (orchestrator state.rs, minimald composables.rs, mip main.rs, decode/graph/mctx internals) invokes one of the three refactored methods; no missed duplicate. Only other ColorChoice::Auto use is mip/src/cmd_status.rs:17 which opens stdout, not stderr.
3. git diff --stat 8052fe8 10da5e0c — exit 0 · self-reported
Base..head diff is 7 files / +21 -6 and matches the staged pr.diff exactly; working tree is at head_sha 10da5e0.
4. Read Cargo.toml of common/decode/graph/mctx and root Cargo.toml — self-reported
decode, graph, mctx each declare common.workspace = true and codespan-reporting.workspace = true; root pins codespan-reporting 0.13 with serialization; Cargo.lock change adds only the codespan-reporting edge to common's package entry.
5. Read report_to signatures in the three changed files — self-reported
decode/src/error.rs:200, graph/src/lib.rs:47, mctx/src/error.rs:34 all take &mut dyn codespan_reporting::term::termcolor::WriteColor, so common::report_to_stderr(|w| self.report_to(w)) is type-correct and renders identically to the replaced inline bodies.
minimal-review · schema minimal-review/v1 · persona generalist · model claude-sonnet-4-5 via claude-code · prompt review-prompt/v2 · head 10da5e0c21ca · verdict APPROVE · 0 finding(s), 0 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
Summary
decode,graph, andmctxeach carried a byte-identicalreport_to_stderrthat opened a
StandardStream::stderr(ColorChoice::Auto)and handed it to thetype's own
report_to. That is three copies of the same terminal setup, all ofwhich have to stay in sync.
This moves the setup to
common::report_to_stderr, a free function that takesthe output formatting as a closure. Each type keeps its own
report_toand itsreport_to_stderrmethod, which is now a one-line delegation, so no caller andno rendered byte changes.
commonwas picked overdiagnosticsbecausedecode,graph, andmctxalready depend on it, so the helper adds no new crate edge. Putting it in
diagnosticswould have needed three new edges.Testing
Clippy is clean. The tests pass (70 + 84 + 28), including the
decode,graph,and
mctxrenderer tests that capturereport_tooutput into a buffer.The rest of the workspace was not rebuilt, since only these four crates changed
and the helper adds a module rather than altering an existing API.
Checklist
BREAKING CHANGE:footer present if this is a breaking change (not breaking)Summary by CodeRabbit