refactor(cli): resolve Sonar findings - #1018
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:S
|
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 (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used📓 Path-based instructions (24)Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.⚙️ CodeRabbit configuration file Files:
If a language surface changed, always run that language's test target even when Rust core did not change.📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md) Files:
Keep async behavior on the existing tokio-based model.📄 CodeRabbit inference engine (AGENTS.md) Files:
[ ] Do all bindings expose the same logical knobs and semantics?📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md) Files:
If any Rust code changed, always run `just test-rust`.📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md) Files:
**Formatting**: `cargo fmt` (rustfmt defaults) **Linting**: `cargo clippy -- -D warnings` -- all warnings are treated as errors📄 CodeRabbit inference engine (CONTRIBUTING.md) Files:
If any Rust code changed, also run `cargo fmt --all`.📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md) Files:
Use `Json = serde_json::Value` in Rust-facing runtime APIs where the existing code expects JSON payloads.📄 CodeRabbit inference engine (AGENTS.md) Files:
[ ] Branch scope is coherent and reviewable [ ] Relevant tests passed under `validate-change` [ ] Docs and examples updated for any public behavior changes [ ] Pull request title follows Conventional Commit style and uses the correct type U...📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md) Files:
Format changed files with the language-native formatter before the final lint/test pass.📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md) Files:
Keep NeMo Relay optional Use stable, documented framework or plugin APIs Wrap tool and LLM paths at the correct framework boundary Preserve the framework's original behavior when NeMo Relay is absent Integration uses public framework or plu...📄 CodeRabbit inference engine (.agents/skills/contribute-integration/SKILL.md) Files:
Tool execution callbacks and each execution-intercept `next` continuation return the canonical `ToolExecutionResult { result, annotation }`.📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md) Files:
Keep SPDX headers on source, docs, scripts, and configuration files.📄 CodeRabbit inference engine (AGENTS.md) Files:
**Validation** Run the validation matrix from the `validate-change` skill for the affected surfaces.📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md) Files:
Use `test-node-binding`.📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md) Files:
Use `test-ffi-surface`.📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md) Files:
[ ] Any Rust change ran `just test-rust` [ ] Any Rust change ran `cargo fmt --all` [ ] Any Rust change ran `cargo clippy --workspace --all-targets -- -D warnings`📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md) Files:
| Node.js | `camelCase` | `toolCall` |📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md) Files:
Follow binding naming conventions: Rust and Python `snake_case`, C FFI exports prefixed `nemo_relay_`, Go `PascalCase` for public APIs, Node.js `camelCase`.📄 CodeRabbit inference engine (AGENTS.md) Files:
[ ] SPDX license header on any new files📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md) Files:
Update docs and examples in the same branch.📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md) Files:
Run `cargo fmt --all` for all FFI work since it is Rust work Run `just test-rust` to validate FFI changes Run `cargo clippy --workspace --all-targets -- -D warnings` to enforce strict linting on FFI work📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md) Files:
Run `cargo fmt --all` when Rust files are changed as part of Node work Run `cargo clippy --workspace --all-targets -- -D warnings` when Rust files are changed as part of Node work Run `just test-rust` when Rust files are changed as part of...📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md) Files:
When Rust files changed as part of Go work, also run `cargo fmt --all`, `just test-rust`, and `cargo clippy --workspace --all-targets -- -D warnings`📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md) Files:
🔇 Additional comments (4)
WalkthroughThe changes refactor managed extension validation and timeout checks, update tool-result and Unicode handling, adjust MCP child readiness detection, and reformat the managed dispatcher test fixture. ChangesRuntime behavior updates
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change cleans up managed extension validation, result formatting, Unicode truncation, dispatcher fixtures, and MCP route timeout handling without identified behavior regressions. The current change set is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Will Killian <wkillian@nvidia.com>
|
/merge |
2 participants
Overview
Resolve all 17 open SonarQube findings reported for the current
mainbranch across the managed Pi extension and MCP daemon.Details
codePointAt().String.rawfor the managed Pi dispatcher test fixture.Validation:
just test-pi(passed: TypeScript checks, 114 Pi tests, and 5 managed Pi tests)cargo test -p nemo-relay-cli daemon::mcp --features __test-cli-port-override,__skip-implicit-config --locked(passed: 23 MCP tests)cargo fmt --all -- --check(passed)git diff --check(passed)just test-rust, workspace Cargo Check, and workspace Clippy are blocked by six unrelated existing compile errors incrates/pii-redactiontests, whereinitialize_pluginsandclear_plugin_configurationare unavailable.Where should the reviewer start?
Start with
shapeViolationand its extracted helpers incrates/cli/src/daemon/managed/pi_extension/index.ts, then reviewroute_activation_timed_outincrates/cli/src/daemon/mcp/mod.rs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit