refactor: resolve CLI and script Sonar findings#428
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (8)**/*.rs📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
**/*.{rs,py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,js,mjs,cjs,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,go,js,ts,c,h}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,go,js,ts}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
{crates/**/src/**/*.rs,python/**/*.py}📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Files:
**/*📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{rs,py,go,js,ts}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
🔇 Additional comments (6)
WalkthroughThe pull request reorganizes CLI diagnostics, marketplace installation, plugin lifecycle, process supervision, server shutdown, session cleanup, gateway tests, and test scripts by extracting internal helper functions while preserving existing public interfaces and reported outcomes. ChangesCLI diagnostics and runtime control flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cli/src/server/mod.rs (1)
364-389: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReport teardown errors before returning success
close_result?short-circuits on the success path, so subscriber flush and plugin teardown failures are dropped when session shutdown fails first. Surface all three teardown outcomes consistently here.🤖 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/cli/src/server/mod.rs` around lines 364 - 389, The success path in finish_server_shutdown must report all teardown failures instead of short-circuiting at close_result?. Collect and surface the outcomes of close_result, flush_result, and clear_result consistently, while preserving the existing server-error handling and returning success only when all teardown operations succeed.
🤖 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/cli/src/installation/marketplace/mod.rs`:
- Around line 411-422: Replace the #[allow(clippy::too_many_arguments)]
suppressions on install_marketplace_content, write_install_state, and
finish_install_registration by introducing a small InstallTransactionState that
groups the mutable force_snapshot and replacement_generation_lock fields, and
pass it through these functions. Update each call site and field access
consistently, while preserving the existing transaction behavior; group runner
and setup_runner as well if needed to satisfy the argument-count lint.
---
Outside diff comments:
In `@crates/cli/src/server/mod.rs`:
- Around line 364-389: The success path in finish_server_shutdown must report
all teardown failures instead of short-circuiting at close_result?. Collect and
surface the outcomes of close_result, flush_result, and clear_result
consistently, while preserving the existing server-error handling and returning
success only when all teardown operations succeed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 19e76d3b-089f-4b91-98a3-43a3dcff9c65
📒 Files selected for processing (17)
crates/cli/src/commands/diagnostics.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/hooks/delivery.rscrates/cli/src/installation/marketplace/mod.rscrates/cli/src/mcp_environment.rscrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/plugins/schema/secrets.rscrates/cli/src/process/detached.rscrates/cli/src/process/supervision/unix.rscrates/cli/src/process/supervision/windows.rscrates/cli/src/server/mod.rscrates/cli/src/sessions/idle.rscrates/cli/tests/cli_tests.rsscripts/test-claude-plugin-e2e.shscripts/test-hermes-mcp-e2e.shscripts/test-support/codex_mock_provider.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
scripts/test-support/codex_mock_provider.pycrates/cli/src/plugins/schema/secrets.rscrates/cli/src/process/supervision/unix.rscrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/mcp_environment.rscrates/cli/src/hooks/delivery.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/sessions/idle.rscrates/cli/src/process/detached.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/process/supervision/windows.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/server/mod.rscrates/cli/src/installation/marketplace/mod.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
scripts/test-support/codex_mock_provider.pycrates/cli/src/plugins/schema/secrets.rscrates/cli/src/process/supervision/unix.rscrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/mcp_environment.rscrates/cli/src/hooks/delivery.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/sessions/idle.rscrates/cli/src/process/detached.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/process/supervision/windows.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/server/mod.rscrates/cli/src/installation/marketplace/mod.rs
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: When changing the Python wrapper package, tests, or docs tooling, lint with Ruff (E,F,W,I), format with Ruff formatter (120-character lines, double quotes), and passtytype checking.
Add the SPDX license header to all Python source files using the#comment form.
Files:
scripts/test-support/codex_mock_provider.py
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
scripts/test-support/codex_mock_provider.pycrates/cli/src/plugins/schema/secrets.rscrates/cli/src/process/supervision/unix.rscrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/mcp_environment.rscrates/cli/src/hooks/delivery.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/sessions/idle.rscrates/cli/src/process/detached.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/process/supervision/windows.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/server/mod.rscrates/cli/src/installation/marketplace/mod.rs
**/*.{py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
Keep Python, Go, and Node.js config objects and subscriber/exporter methods aligned so all bindings expose the same logical knobs and semantics.
Files:
scripts/test-support/codex_mock_provider.py
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
scripts/test-support/codex_mock_provider.pycrates/cli/src/plugins/schema/secrets.rscrates/cli/src/process/supervision/unix.rsscripts/test-hermes-mcp-e2e.shcrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/mcp_environment.rscrates/cli/src/hooks/delivery.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/sessions/idle.rsscripts/test-claude-plugin-e2e.shcrates/cli/src/process/detached.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/process/supervision/windows.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/server/mod.rscrates/cli/src/installation/marketplace/mod.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
scripts/test-support/codex_mock_provider.pycrates/cli/src/plugins/schema/secrets.rscrates/cli/src/process/supervision/unix.rscrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/mcp_environment.rscrates/cli/src/hooks/delivery.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/sessions/idle.rscrates/cli/src/process/detached.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/process/supervision/windows.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/server/mod.rscrates/cli/src/installation/marketplace/mod.rs
{.github/**,.gitlab-ci.yml,.pre-commit-config.yaml,justfile,scripts/**}
⚙️ CodeRabbit configuration file
{.github/**,.gitlab-ci.yml,.pre-commit-config.yaml,justfile,scripts/**}: Review automation changes for reproducibility, pinned versions where appropriate, secret handling, and consistency with the documented validation matrix.
Pay attention to commands that need generated native artifacts, FFI libraries, or platform-specific environment variables.
Files:
scripts/test-support/codex_mock_provider.pyscripts/test-hermes-mcp-e2e.shscripts/test-claude-plugin-e2e.sh
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/cli/src/plugins/schema/secrets.rscrates/cli/src/process/supervision/unix.rscrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/mcp_environment.rscrates/cli/src/hooks/delivery.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/sessions/idle.rscrates/cli/src/process/detached.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/process/supervision/windows.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/server/mod.rscrates/cli/src/installation/marketplace/mod.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/cli/src/plugins/schema/secrets.rscrates/cli/src/process/supervision/unix.rscrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/mcp_environment.rscrates/cli/src/hooks/delivery.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/sessions/idle.rscrates/cli/src/process/detached.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/process/supervision/windows.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/server/mod.rscrates/cli/src/installation/marketplace/mod.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/cli/src/plugins/schema/secrets.rscrates/cli/src/process/supervision/unix.rscrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/mcp_environment.rscrates/cli/src/hooks/delivery.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/sessions/idle.rscrates/cli/src/process/detached.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/process/supervision/windows.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/server/mod.rscrates/cli/src/installation/marketplace/mod.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/cli_tests.rs
🔇 Additional comments (18)
crates/cli/src/mcp_environment.rs (1)
227-227: LGTM!Also applies to: 238-263
crates/cli/src/plugins/lifecycle/environment.rs (1)
432-444: LGTM!Also applies to: 446-494, 496-500, 502-512
crates/cli/src/plugins/lifecycle/mod.rs (1)
673-680: LGTM!Also applies to: 772-814, 998-1119
crates/cli/src/plugins/schema/secrets.rs (1)
120-133: LGTM!Also applies to: 136-205
crates/cli/src/process/detached.rs (1)
17-87: LGTM!Also applies to: 157-166
crates/cli/src/process/supervision/unix.rs (1)
73-74: LGTM!Also applies to: 95-112
crates/cli/src/process/supervision/windows.rs (1)
138-138: LGTM!Also applies to: 154-157, 169-203
crates/cli/src/commands/diagnostics.rs (1)
34-37: LGTM!Also applies to: 39-63, 65-91
crates/cli/src/diagnostics/mod.rs (1)
302-305: LGTM!Also applies to: 307-349, 351-372, 374-402
crates/cli/src/hooks/delivery.rs (1)
11-11: LGTM!Also applies to: 32-38, 47-50, 101-133
crates/cli/src/installation/marketplace/mod.rs (2)
276-328: LGTM!Also applies to: 330-409, 1862-2025, 2051-2078
411-499: 🩺 Stability & AvailabilityFresh-install generation lock is already cleaned up on failure.
ReplacementGenerationLockremoves the lock inDropwhen it is unreferenced, so an error inwrite_install_stateorfinish_install_registrationwill not strandlayout.generation_lockon disk.> Likely an incorrect or invalid review comment.crates/cli/src/server/mod.rs (1)
244-322: LGTM!Also applies to: 324-344, 346-362
crates/cli/src/sessions/idle.rs (1)
36-53: LGTM!Also applies to: 55-76, 78-117, 119-138, 140-153
crates/cli/tests/cli_tests.rs (1)
774-788: LGTM!Also applies to: 833-947, 3383-3469, 3559-3569, 3596-3596, 3641-3713
scripts/test-claude-plugin-e2e.sh (2)
38-38: LGTM!
181-182: 🎯 Functional CorrectnessNo issue here. The helpers are only called as bare statements under
set -euo pipefail, so the trailingreturn 0does not mask a failing Python heredoc.> Likely an incorrect or invalid review comment.scripts/test-support/codex_mock_provider.py (1)
369-370: LGTM!
Signed-off-by: Will Killian <wkillian@nvidia.com>
#### Overview This isolates two unrelated ARM CI failures observed while validating #428: a sidecar-publication timing flake and transient Ubuntu ports package-download failures. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Allow up to 30 seconds for test sidecars to publish ownership state on slower ARM runners. - Use HTTPS for Ubuntu ports package sources on ARM. - Retry apt metadata and package downloads up to five times. No production runtime behavior changes. Validation: - Repeated the previously flaky CLI test 10 times. - cargo clippy -p nemo-relay-cli --test cli_tests -- -D warnings - just test-rust - uv run pre-commit run --all-files #### Where should the reviewer start? Start with the Install musl tools step in .github/workflows/ci_rust.yml, then review the test-only timeout constant in crates/cli/tests/cli_tests.rs. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to #428 ## Summary by CodeRabbit - **Chores** - Improved build reliability for ARM64 and musl-based environments by adding package-manager retries and platform-specific setup handling. - **Tests** - Increased the sidecar publication wait period to 30 seconds, reducing failures caused by slower startup or publishing conditions. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Zhongxuan (Daniel) Wang (https://github.com/zhongxuanwang-nv) URL: #433
zhongxuanwang-nv
left a comment
zhongxuanwang-nv
left a comment
There was a problem hiding this comment.
Used powerful AIs to help review, I think everything looks great!
|
/merge |
2 participants
Overview
Details
Resolves the CLI and shell-script portion of the main-branch Sonar backlog without changing public APIs or command behavior. Complex orchestration is split into private phase helpers across marketplace transactions, diagnostics, configuration traversal, lifecycle filesystem work, process supervision, session cleanup, server startup, and the affected integration tests. The mock provider is documented as test-only loopback HTTP, and flagged shell helpers now return success explicitly.
Validation:
cargo fmt --allcargo test -p nemo-relay-clijust test-rustcargo clippy --workspace --all-targets -- -D warningsbash -n scripts/test-claude-plugin-e2e.sh scripts/test-hermes-mcp-e2e.shuv run pre-commit run --all-filesNo public API, ABI, configuration, telemetry, error-message, or CLI behavior changes are intended.
Where should the reviewer start?
Start with
install_host_lockedand its transaction phase helpers incrates/cli/src/installation/marketplace/mod.rs, then review the smaller private-helper extractions and test decomposition.Related Issues:
Summary by CodeRabbit