Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughBoth terminal progress modules now write through explicit stderr handles. Clear and determinate progress writes add error context. Spinner write and flush errors remain ignored. The console print allowlist is empty. ChangesTerminal progress output
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The terminal progress refactor preserves CLI output and passes validation, so it is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
The console-print ratchet forbids the print macros, not console output. The two inline progress renderers were the last approved occurrences, so they now take a single `std::io::stderr()` handle, `write!` into it, and flush once, which empties the ratchet without a permanent exemption. Both copies of the renderer are fixed in place rather than collapsed into one, because the two `fit_inline_gauge_label` implementations genuinely differ on very narrow terminals and de-duplicating them would change what the CLI prints. The dedup is worth doing on its own terms, not under cover of a plumbing change. Each renderer now documents why holding a terminal handle directly is legitimate here, so a later pass that forbids direct handles in library crates has a named exception to point at.
28fe0c1 to
1a56b00
Compare
i386
left a comment
There was a problem hiding this comment.
Reviewed the synchronized exact head and its parent integration. Progress renderers use the sanctioned event/output path and the stack passes the full repository gate locally.
Closes the console-print ratchet for issue #1763. The inline progress renderers were the last two files holding approved occurrences, and they now write to
stderrthrough an explicit handle instead of a print macro. The allowlist goes from 6 approvals in 2 files to zero, andjust no-console-printpasses with an empty allowlist.Nothing the CLI prints changes.
models updates,runtime list, and thepluginsubcommands render byte-for-byte the same output on both streams, in pretty and--jsonmode alike.Stacked on #1851 (
codex/1763-runtime-events); review that first.What changed
Both
terminal_progress.rscopies take a singlestd::io::stderr()handle at each of the three write sites,write!into it, and flush once:eprint!expands tostderr().write_fmt(..), so this is the same call against the same internally-locked handle: one lock, same bytes, same ordering. The only behavioral difference is that the two?-returning sites now propagate a write error instead of panicking.Each file also gained a module doc explaining why a direct terminal handle is legitimate in this specific module: it is the console output facility's renderer, every entry point early-returns on
json_mode_enabled(), progress is a transient cursor-addressed redraw with no structured representation, and routing it back through the writer would be circular. A later pass that forbids directio::stdout()/io::stderr()handles in library crates can point its exemption rule at that comment.Dedup decision: fix both copies in place (option a)
The issue offered collapsing the two near-duplicate renderers into
mesh-llm-eventsand re-pointing the threemesh-llm-tuiconsumers. That is the better end state and it is mechanically easy — the events module is a strict superset of every symbol the tui consumers import, both crates already depend onmesh-llm-events, and the twojson_mode_enabled()gates are provably equivalent (OutputManager::init_globalsets the events sink and the tui manager together).I did not do it here, because the required proof fails.
fit_inline_gauge_labeldiffers between the copies: the tui version doesmax_label_len.saturating_sub(3)and always appends a three-character"...", while the events version emits 0/1/2 dots when the budget is smaller than the ellipsis. They agree for everymax_label_len >= 3and diverge only below that — a gauge width of 25–27 columns, reachable on a terminal of roughly 26–28 columns forplugin installandmodels updates.Worse, the byte-diff harness this PR requires cannot detect that. It redirects both streams to files,
crossterm::terminal::size()fails, and the gauge falls back toINLINE_GAUGE_WIDTH = 96. A clean diff at 96 columns would be a false proof of equivalence, so presenting one would be misleading. The dedup belongs in its own change where the truncation difference can be reviewed as the deliberate fix it is; I will file it separately.Validation
cargo check --workspace --all-targetsandcargo clippy --workspace --all-targets -- -D warningswere run workspace-wide rather than per-crate, since these two crates have many dependents.cargo check --workspace --all-targets— clean, no warnings.cargo clippy --workspace --all-targets -- -D warnings— one failure,clippy::chunks_exact_to_as_chunksincrates/skippy-model/src/gguf_writer_tests/conversion.rs:179. Pre-existing and unrelated: that crate is untouched by this branch, and a run without-D warningsshows it is the workspace's only warning. The repo pins no toolchain file; local clippy is0.1.98 (2026-08-18)against CI's pinned stable2026-08-20, which likely explains why CI does not flag it.just no-console-print— passes.tools/xtask/data/console_print_allowlist.jsonregenerated to{}. Left in place deliberately; deleting the file is the final step of task: retire the console-print ratchet — convert the remaining 1,056 allowlisted call sites and delete the allowlist #1763.just ci-validate— passes.Byte-diff, stdout and stderr captured separately
Base binary built from
700e70454in a detached worktree, new binary from this branch. Each case captures stdout and stderr to separate files and compares each stream independently withcmp -s— combining them would hide a stream migration, which is the exact bug class this refactor could introduce.models updates --all --checkmodels updates --all --check --jsonmodels updates backpack-run/Qwen2.5-0.5B-Instruct-GGUFmodels updates backpack-run/... --jsonmodels listruntime listplugin searchplugin listmodels updates --all --check 2>&1 | cat(non-tty)Both
--jsonstdouts still parse as valid JSON on the new binary. The non-tty pipe is byte-identical, so theis_terminal/json_mode_enabledgating is unchanged.The one
plugin listdifference is a harness artifact, not a rendering change. That command prints the registered plugin's own launch command, which embeds the absolute path of the binary that registered it:The converted paths were genuinely exercised. Inspecting the captured stderr shows the
LineGaugeand the ANSI redraw:That is
DeterminateProgressLine::draw_countsplus the\r\x1b[2Kclear — two of the three converted sites — producing 10,034 identical bytes across 56 repos.Adjacent finding, not addressed here
Under
--json, one-shot commands emit clean JSON on stdout but still write the human progress bar to stderr (about 10 KB formodels updates --all --check).json_mode_enabled()only returns true once anOutputSinkis installed, and one-shot commands never callOutputManager::init_global, so the early returns in these renderers cannot fire. It is byte-identical on the base binary, so it predates this work and stdout stays parseable — cosmetic, but worth its own issue. Fixing it would be a behavior change, not plumbing, so it is out of scope here.Summary by CodeRabbit
Bug Fixes
Documentation
Chores