Skip to content

chore: address Sonar scan findings#137

Merged
rapids-bot[bot] merged 1 commit into
NVIDIA:mainfrom
willkill07:wkk_fix/sonar-scan-results
May 21, 2026
Merged

chore: address Sonar scan findings#137
rapids-bot[bot] merged 1 commit into
NVIDIA:mainfrom
willkill07:wkk_fix/sonar-scan-results

Conversation

@willkill07
Copy link
Copy Markdown
Member

@willkill07 willkill07 commented May 21, 2026

Overview

Address the current Sonar scan findings by tightening the reported test and Guardrails validation code, and include the small CLI coverage-test API update needed to keep Rust validation green.

  • I confirm this contribution is my own work, or I have the right to submit it under this project's license.
  • I searched existing issues and open pull requests, and this does not duplicate existing work.

Details

  • Replaced the Node subscriber-event deep clone test with structuredClone.
  • Deduplicated the repeated Go EmitEvent failed: %v test literal.
  • Split NeMo Guardrails config-shape validation into focused mode-specific helpers backed by shared shape flags and diagnostic creation.
  • Updated the CLI coverage test conditional guardrail callback from Box to Arc to match the current registry API and unblock Rust validation.

Validation:

  • cargo test -p nemo-flow nemo_guardrails
  • just test-go
  • just test-node
  • just test-rust
  • commit hooks: cargo fmt, cargo clippy, cargo check, go fmt/vet, node prettier check

Where should the reviewer start?

Start with crates/core/src/plugins/nemo_guardrails/plugin_component.rs, where the Sonar cognitive-complexity issue is resolved without changing diagnostic messages.

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • Relates to: none

Summary by CodeRabbit

  • Tests

    • Updated test implementations across multiple components for consistency and clarity.
  • Refactor

    • Improved internal configuration validation logic with a cleaner, flag-driven approach for better maintainability.
  • Chores

    • Internal code improvements and standardization of error handling patterns.

Review Change Stack

Signed-off-by: Will Killian <wkillian@nvidia.com>
@willkill07 willkill07 requested a review from a team as a code owner May 21, 2026 14:23
@github-actions github-actions Bot added size:M PR is medium Maintenance CI or Build or general repository maintenance lang:go PR changes/introduces Go code lang:js PR changes/introduces Javascript/Typescript code lang:rust PR changes/introduces Rust code labels May 21, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: bd7b1f1f-611e-46ba-802f-03da12339184

📥 Commits

Reviewing files that changed from the base of the PR and between 78e83c0 and a32a627.

📒 Files selected for processing (4)
  • crates/cli/tests/coverage/server_tests.rs
  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
  • crates/node/tests/scope_tests.mjs
  • go/nemo_flow/scope_test.go
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Check / Run
🧰 Additional context used
📓 Path-based instructions (23)
crates/node/**

📄 CodeRabbit inference engine (.agents/skills/test-node-binding/SKILL.md)

Run just test-node for the normal dev/test loop when working on Node binding changes

Files:

  • crates/node/tests/scope_tests.mjs
crates/{python,ffi,node,wasm}/**/*

⚙️ CodeRabbit configuration file

crates/{python,ffi,node,wasm}/**/*: Treat binding changes as public API changes. Check for parity with the other language bindings, FFI ownership/lifetime safety,
callback error propagation, stable type conversion, and consistent async/stream semantics.
Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere.

Files:

  • crates/node/tests/scope_tests.mjs
{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_test.go}

⚙️ CodeRabbit configuration file

{crates/**/tests/**,python/tests/**,go/nemo_flow/**/*_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/node/tests/scope_tests.mjs
  • crates/cli/tests/coverage/server_tests.rs
  • go/nemo_flow/scope_test.go
**/*.rs

📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)

**/*.rs: Run cargo fmt --all for FFI work as it is Rust work
Run just test-rust for FFI validation
Run cargo clippy --workspace --all-targets -- -D warnings to enforce warnings-as-errors linting

**/*.rs: Run cargo fmt --all for Rust code formatting
Run cargo clippy --workspace --all-targets -- -D warnings to enforce Rust linting with no warnings
Run just test-rust as the shared-runtime build/test wrapper for Rust changes

Use Rust snake_case naming convention for Rust code

**/*.rs: Any Rust change must run just test-rust
Any Rust change must run cargo fmt --all
Any Rust change must run cargo clippy --workspace --all-targets -- -D warnings

**/*.rs: Run cargo fmt --all for Rust code formatting when Node changes touch Rust files
Run cargo clippy --workspace --all-targets -- -D warnings to enforce strict linting when Rust files changed as part of Node work

**/*.rs: Always run just test-rust when any Rust code changes
Always run cargo fmt --all when any Rust code changes
Always run cargo clippy --workspace --all-targets -- -D warnings when any Rust code changes

If any Rust files changed as part of the Python work, also run cargo fmt --all, just test-rust, and cargo clippy --workspace --all-targets -- -D warnings

Files:

  • crates/cli/tests/coverage/server_tests.rs
  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
**/*.{rs,go,js,ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Include SPDX license headers in all Rust, Go, JavaScript, and TypeScript source files using C-style comment syntax

Files:

  • crates/cli/tests/coverage/server_tests.rs
  • go/nemo_flow/scope_test.go
  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
**/*.{rs,py,go,js,ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Use SONAR_IGNORE_START / SONAR_IGNORE_END markers only for documented false positives that cannot be resolved in code; keep ignored blocks small, add explanatory comments, and require reviewer sign-off

Files:

  • crates/cli/tests/coverage/server_tests.rs
  • go/nemo_flow/scope_test.go
  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
**/*.{js,ts,tsx,jsx,py,rs,go,java,c,cpp,h,cc,cxx,cs,rb,php,swift,kt}

📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)

Changed files must be formatted with the language-native formatter

Files:

  • crates/cli/tests/coverage/server_tests.rs
  • go/nemo_flow/scope_test.go
  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
**/*.{py,js,ts,tsx,go,rs,md}

📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)

Format changed files with the language-native formatter before the final lint/test pass

Files:

  • crates/cli/tests/coverage/server_tests.rs
  • go/nemo_flow/scope_test.go
  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
**/*.{rs,py,js,ts,tsx,go}

📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)

During iteration, prefer uv run pre-commit run --files <changed files...> for targeted validation

Files:

  • crates/cli/tests/coverage/server_tests.rs
  • go/nemo_flow/scope_test.go
  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
go/nemo_flow/**/*.go

📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)

Format changed Go packages with cd go/nemo_flow && go fmt ./...

Use PascalCase naming convention for Go API implementations

For Go binding changes, run go fmt ./... within the Go module to format all Go files

Files:

  • go/nemo_flow/scope_test.go
**/*.go

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.go: Use gofmt for Go code formatting
Use go vet ./... for Go static analysis
Use PascalCase naming convention for Go code

Files:

  • go/nemo_flow/scope_test.go
**/*.{md,markdown,py,sh,bash,js,ts,java,cpp,go,rust}

📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)

Keep package names, repo references, and build commands current in documentation

Files:

  • go/nemo_flow/scope_test.go
{crates/adaptive/**,python/nemo_flow/{adaptive,plugin}.py,go/nemo_flow/{adaptive,**}/*.go,**/*.{ts,js,wasm}}

📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)

Keep adaptive config schema, plugin lifecycle, and bindings in sync across crates/adaptive, core, bindings, Python (python/nemo_flow/adaptive.py and python/nemo_flow/plugin.py), Go (go/nemo_flow/adaptive and go/nemo_flow), and Node/WebAssembly helpers

Files:

  • go/nemo_flow/scope_test.go
{crates/adaptive/**/*.rs,python/nemo_flow/adaptive.py,python/nemo_flow/plugin.py,go/nemo_flow/**/*.go,**/{helper,constructor,builder}.{ts,tsx,js}}

📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)

Ensure typed helper constructors map cleanly to the same config document without divergence

Files:

  • go/nemo_flow/scope_test.go
{crates/adaptive/**/*.rs,python/nemo_flow/plugin.py,go/nemo_flow/**/*.go,**/{plugin,lifecycle}.{ts,tsx,js}}

📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)

Maintain consistent plugin lifecycle across all language bindings (Rust, Python, Go, Node/WebAssembly)

Files:

  • go/nemo_flow/scope_test.go
{crates/adaptive/**/*.rs,python/nemo_flow/plugin.py,go/nemo_flow/**/*.go,**/{context,plugin}.{ts,tsx,js,rs}}

📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)

Keep plugin context surfaces aligned across all language implementations

Files:

  • go/nemo_flow/scope_test.go
{docs/**,examples/**,crates/adaptive/**,python/nemo_flow/**,go/nemo_flow/**,**/{example,component}.{ts,tsx,js,rs,py,go}}

📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)

Any new adaptive component kind must have documentation, examples, and binding coverage across all supported languages

Files:

  • go/nemo_flow/scope_test.go
go/**/*.go

📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)

If Go language surface changed, always run Go test target even when Rust core did not change

Files:

  • go/nemo_flow/scope_test.go
go/nemo_flow/**/*

⚙️ CodeRabbit configuration file

go/nemo_flow/**/*: Review Go binding changes for cgo memory ownership, race safety, callback cleanup, idiomatic exported APIs, and parity with Rust/FFI behavior.
Any API change should include focused Go tests and consider race-test behavior.

Files:

  • go/nemo_flow/scope_test.go
crates/{core,adaptive}/**

📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)

Changes to crates/core or crates/adaptive must run the full language matrix

Files:

  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
{crates/core,crates/adaptive}/**/*.rs

📄 CodeRabbit inference engine (.agents/skills/test-wasm-binding/SKILL.md)

If the change touched shared runtime semantics in crates/core or crates/adaptive, also use validate-change

Files:

  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
crates/core/**/*.rs

📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)

When crates/core changes, run the full validation matrix across Rust, Python, Go, Node.js, and WebAssembly

crates/core/**/*.rs: Use Json = serde_json::Value in Rust-facing runtime APIs where the existing code expects JSON payloads.
Use Result<T> with FlowError in core runtime paths. Keep errors explicit and binding-appropriate at the wrapper layer.

Files:

  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
crates/{core,adaptive}/**/*.rs

⚙️ CodeRabbit configuration file

crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.

Files:

  • crates/core/src/plugins/nemo_guardrails/plugin_component.rs
🔇 Additional comments (8)
go/nemo_flow/scope_test.go (1)

14-17: LGTM!

Also applies to: 149-151, 360-364, 536-538

crates/cli/tests/coverage/server_tests.rs (1)

534-534: LGTM!

crates/node/tests/scope_tests.mjs (1)

262-262: LGTM!

crates/core/src/plugins/nemo_guardrails/plugin_component.rs (5)

658-670: LGTM!


672-698: LGTM!


700-735: LGTM!


737-797: LGTM!


799-814: LGTM!


Walkthrough

This PR contains four independent refactoring changes across Rust, JavaScript, and Go: guardrail predicate registration uses Arc instead of Box, config validation logic is refactored into flag-driven dispatch with separate local/remote validators, test object cloning switches to structuredClone, and Go test error messages consolidate into constants.

Changes

Guardrails and test refactoring

Layer / File(s) Summary
Guardrail predicate registration update
crates/cli/tests/coverage/server_tests.rs
Guardrail predicate closure wrapped with Arc::new instead of Box::new when registering tool conditional execution guardrails in the pre_tool_hook_rejects_when_conditional_guardrail_blocks test.
Config validation refactoring with flag-driven dispatch
crates/core/src/plugins/nemo_guardrails/plugin_component.rs
validate_config_shape refactored from inline mode-specific logic to a flag-driven approach: new ConfigShapeFlags struct captures field presence/absence, validate_config_shape dispatches to dedicated validate_local_config_shape and validate_remote_config_shape helpers, and new push_config_shape_diag centralizes policy diagnostic emission.
Object cloning approach update
crates/node/tests/scope_tests.mjs
Test assertion in "subscriber event properties" updated to use structuredClone(captured) instead of JSON.parse(JSON.stringify(captured)) for deep cloning.
Go error message constant extraction
go/nemo_flow/scope_test.go
New emitEventFailed formatted string constant introduced; inline error formatting "EmitEvent failed: %v" replaced with constant references in TestEventJSONHelpers, TestEmitEvent, and the scope-type contract test.

Possibly related PRs

  • NVIDIA/NeMo-Flow#135: Integrates tool conditional-execution guardrails for agent/tool calls, directly related to the Arc-based predicate registration change.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title follows Conventional Commits format with type 'chore' and a concise imperative summary under 72 characters.
Description check ✅ Passed The PR description includes all required sections: overview with confirmations, detailed changes, reviewer starting point, and related issues status.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

@willkill07 willkill07 self-assigned this May 21, 2026
@willkill07 willkill07 added this to the 0.3 milestone May 21, 2026
Copy link
Copy Markdown
Contributor

@mnajafian-nv mnajafian-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@willkill07
Copy link
Copy Markdown
Member Author

/merge

@rapids-bot rapids-bot Bot merged commit c8407fe into NVIDIA:main May 21, 2026
68 checks passed
@willkill07 willkill07 deleted the wkk_fix/sonar-scan-results branch May 21, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lang:go PR changes/introduces Go code lang:js PR changes/introduces Javascript/Typescript code lang:rust PR changes/introduces Rust code Maintenance CI or Build or general repository maintenance size:M PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants