fix(cli): fix gpu command to restore stderr output - #844
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates GPU CLI parsing tests; adds CLI integration and benchmark-subprocess tests; extracts TUI fatal emission into a new fatal module with emergency fallback and tests; fixes build-release feature assembly with validation tests; updates docs and makes a CI timeout configurable. ChangesError Visibility and CLI Integration
Build Release GPU Benchmark Feature Flags
Docs and CI Smoke Script
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh-llm-tui/src/output/mod.rs`:
- Around line 8498-8501: The current branch only checks
GLOBAL_OUTPUT_MANAGER.get() and calls emit_event but doesn't handle emit_event
failures, so fatal output can be dropped if the manager exists but its
worker/channel is down; change the call site to attempt emit_event and if it
returns or throws an error (or otherwise indicates failure) fall back to
write_emergency_event(&event); specifically, wrap the emit_event invocation in
error handling (or check its Result/Option) and call write_emergency_event when
emit_event fails, ensuring GLOBAL_OUTPUT_MANAGER, emit_event, and
write_emergency_event are the referenced symbols you update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a82697e-d3e4-4937-99e2-6f5b7e961a7f
📒 Files selected for processing (7)
crates/mesh-llm-cli/src/parser.rscrates/mesh-llm-system/src/benchmark.rscrates/mesh-llm-tui/src/output/mod.rscrates/mesh-llm/tests/cli_errors.rsscripts/build-release.shscripts/tests/test_build_release.pywebsite/src/docs/pages/testing.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh-llm-tui/src/output/mod.rs`:
- Around line 8496-8510: emit_event_or_write_emergency currently treats
emit(event) returning Ok(()) as success even when GLOBAL_OUTPUT_MANAGER was
never initialized, so change emit_event_or_write_emergency to explicitly detect
the "no global manager" path and fall back to write_emergency in that case: call
emit(event) and if it returns Err use write_emergency as today, but if it
returns Ok then verify the global manager is actually present (e.g., check
GLOBAL_OUTPUT_MANAGER or expose/consume an is_initialized() accessor from the
output manager module) and if not present call write_emergency(&event) and
return its result; update emit_fatal_error to continue using
build_fatal_error_event, emit_event and write_emergency_event unchanged. Also
add a regression test next to the existing BrokenPipe test that ensures
emit_fatal_error(...) produces the emergency/stderr output when
GLOBAL_OUTPUT_MANAGER has not been initialized (i.e., arrange no global manager,
call emit_fatal_error, assert emergency output seen).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 14db0c87-28a6-45e9-8695-0cc595ba0b47
📒 Files selected for processing (2)
crates/mesh-llm-tui/src/output/mod.rsscripts/skippy-ci-smoke.sh
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh-llm-tui/src/output/mod.rs (1)
8496-8519: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract fatal/emergency output routing into its own named module.
This change adds a distinct responsibility around fatal-event fallback handling inside an already oversized
output/mod.rs. Moving this slice into a focused module such asfatal_outputwould keep ownership clearer and stop growing a file that is already far past the repo threshold. As per coding guidelines, "When touching a source file that is already over 1,000 lines, first check whether the change adds or exposes a separable responsibility. If it does, split that responsibility into a semantically named module as part of the change, and keep the new file under 1,000 lines."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-tui/src/output/mod.rs` around lines 8496 - 8519, Split the fatal/emergency output routing into a dedicated module (e.g., fatal_output) by moving the functions emit_fatal_error, global_output_manager_initialized, and emit_event_or_write_emergency (and any helpers like build_fatal_error_event, emit_event, write_emergency_event references) into that new module, mark APIs as pub(crate) or pub as needed, update imports/uses so callers still call fatal_output::emit_fatal_error, and keep the implementation identical while ensuring the new module is declared in the parent with mod fatal_output so the file size of the original output/mod.rs is reduced and ownership is clear.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh-llm-tui/src/output/mod.rs`:
- Around line 8509-8518: The helper emit_event_or_write_emergency currently
allows a race where emit(event) returns Ok(()) before the output manager is
initialized; fix by evaluating output_manager_initialized() before deciding the
success branch: call let initialized = output_manager_initialized() first, then
call emit(event.clone()) and only treat Ok(()) as success if initialized is
true; otherwise call write_emergency(&event). Update the logic in
emit_event_or_write_emergency to use the precomputed initialized flag when
matching emit's result so the no-op emit cannot falsely return Ok(()).
---
Outside diff comments:
In `@crates/mesh-llm-tui/src/output/mod.rs`:
- Around line 8496-8519: Split the fatal/emergency output routing into a
dedicated module (e.g., fatal_output) by moving the functions emit_fatal_error,
global_output_manager_initialized, and emit_event_or_write_emergency (and any
helpers like build_fatal_error_event, emit_event, write_emergency_event
references) into that new module, mark APIs as pub(crate) or pub as needed,
update imports/uses so callers still call fatal_output::emit_fatal_error, and
keep the implementation identical while ensuring the new module is declared in
the parent with mod fatal_output so the file size of the original output/mod.rs
is reduced and ownership is clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65e3b7d7-08d5-4b69-8dd5-5d4371e3b2de
📒 Files selected for processing (1)
crates/mesh-llm-tui/src/output/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/skippy-ci-smoke.sh (1)
47-47: ⚡ Quick winRemove the duplicate
DENSE_CHAIN_STARTUP_TIMEOUT_SECSassignment.
DENSE_CHAIN_STARTUP_TIMEOUT_SECSis already defined at Line 39; redefining it at Line 47 adds config drift risk without functional benefit.Suggested cleanup
DENSE_SMOKE_SPLIT_1="${DENSE_SMOKE_SPLIT_1:-}" DENSE_SMOKE_SPLIT_2="${DENSE_SMOKE_SPLIT_2:-}" -DENSE_CHAIN_STARTUP_TIMEOUT_SECS="${DENSE_CHAIN_STARTUP_TIMEOUT_SECS:-180}" STAGE_SERVER_BIN="${STAGE_SERVER_BIN:-target/debug/skippy-server}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/skippy-ci-smoke.sh` at line 47, There is a duplicate assignment of the environment variable DENSE_CHAIN_STARTUP_TIMEOUT_SECS; remove the redundant line that reassigns DENSE_CHAIN_STARTUP_TIMEOUT_SECS (leave the original definition intact) so the script only defines this variable once and avoids config drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/skippy-ci-smoke.sh`:
- Line 47: There is a duplicate assignment of the environment variable
DENSE_CHAIN_STARTUP_TIMEOUT_SECS; remove the redundant line that reassigns
DENSE_CHAIN_STARTUP_TIMEOUT_SECS (leave the original definition intact) so the
script only defines this variable once and avoids config drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4367368-6649-4626-8c33-e077961ed9fa
📒 Files selected for processing (1)
scripts/skippy-ci-smoke.sh
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
ndizazzo
force-pushed
the
fix/gpu-command-alias-output
branch
from
June 14, 2026 07:03
cadd8a0 to
539e7dc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GPU inspection and benchmark-refresh failures now tell operators what went wrong instead of exiting silently. The
gpuandgpuscommand spellings are locked as exact synonyms, the hidden GPU benchmark path reports backend/build problems on stderr, and CUDA/ROCm release builds now include the GPU benchmark backend features they advertise at runtime.This also updates the testing docs to use the supported benchmark-refresh command:
mesh-llm gpus detect --json.What changed
mesh-llm gpu detectandmesh-llm gpus detectare explicitly covered as the same command path.mesh-llm gpu benchmarkcommand.Before / After
Invalid old GPU benchmark command
Before, this was part of the confusion because stale docs referenced it, and related command failures could be hard to interpret.
$ mesh-llm gpu benchmark # non-zero exit, often no useful operator-facing output in nearby failure pathsAfter,
benchmarkremains unsupported undergpu/gpus, but the CLI prints a normal error and usage text.$ mesh-llm gpu benchmark error: unrecognized subcommand 'benchmark' Usage: mesh-llm gpus [OPTIONS] [COMMAND]Supported GPU benchmark refresh
Before, users could reasonably try the stale documented command and miss the supported refresh path.
After, docs and tests use the supported command.
Both spellings remain valid synonyms:
Missing CUDA benchmark support
Before, Jetson/aarch64 hosts could select CUDA for fingerprinting, then the hidden benchmark command could exit 1 with no stdout or stderr.
$ mesh-llm benchmark run-gpu --backend cuda # exit 1, no outputAfter, the same failure is visible to the operator.
Empty-stderr benchmark child failures
Before, a failing benchmark child with no stderr made the parent refresh path difficult to diagnose.
After, the parent reports the child process status so the failure is actionable.
Validation
cargo test -p mesh-llm-cli gpu_and_gpus_spellings_are_synonymouscargo test -p mesh-llm --test cli_errorscargo test -p mesh-llm-system benchmark::tests::test_run_and_savepython3 -m unittest scripts.tests.test_build_releasejust website-buildcargo fmt --all -- --checkcargo check -p mesh-llmcargo clippy -p mesh-llm-system --all-targets -- -D warningscargo clippy -p mesh-llm --all-targets -- -D warningsSummary by CodeRabbit
Bug Fixes
Refactor
Documentation
Tests
Chores