refactor: resolve core and FFI Sonar findings#430
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:XL
WalkthroughRefactors LLM annotation resolution, optimization contribution recording and summary finalization, shared OTLP/OpenInference attribute emission, ATOF sink validation, and nullable JSON parsing in the FFI intercept-outcome API. ChangesLLM optimization lifecycle
ATOF sink validation
FFI intercept JSON parsing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant finalize_optimization_summary
participant analyze_contributions
participant derive_optimization_usage
participant price_optimization_usage
participant LlmOptimizationSummary
finalize_optimization_summary->>analyze_contributions: derive limitations, tokens, and model inputs
finalize_optimization_summary->>derive_optimization_usage: derive effective and baseline usage
finalize_optimization_summary->>price_optimization_usage: compute gated cost estimates
finalize_optimization_summary->>LlmOptimizationSummary: assemble finalized summary
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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/core/src/api/optimization.rs (1)
64-139: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep reserved-attempt diagnostics atomic with finalization.
A record can reserve while open, then
close_for_finalization/finishcan run beforenote_invalid_payload_schemaor the size-limit diagnostic reacquires the lock. Because those helpers ignore closed state, the limitation is lost and the optimization summary may be omitted. Track in-flight reservations or otherwise prevent finalization from draining state before their diagnostic/commit outcome is recorded.As per path instructions, “Pay close attention to … event lifecycle regressions.”
Also applies to: 174-178
🤖 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/core/src/api/optimization.rs` around lines 64 - 139, Make the reserved-attempt lifecycle atomic with finalization: update reserve_record_attempt and the corresponding diagnostic/commit paths so an in-flight reservation cannot be drained by close_for_finalization or finish before its outcome is recorded. Track in-flight reservations or hold equivalent state through serialized_contribution_size, note_invalid_payload_schema, and commit_contribution, and ensure finalization waits for or accounts for those reservations before producing the summary.Source: Path instructions
🤖 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/core/src/observability/plugin_component.rs`:
- Around line 1978-2008: Add focused regression tests for
validate_atof_stream_sink_name covering missing, empty,
leading/trailing-whitespace, duplicate, and valid stream names. Assert both
validation outcomes and that emitted diagnostics use the expected
sinks[index].name field, while confirming valid names produce no diagnostic.
---
Outside diff comments:
In `@crates/core/src/api/optimization.rs`:
- Around line 64-139: Make the reserved-attempt lifecycle atomic with
finalization: update reserve_record_attempt and the corresponding
diagnostic/commit paths so an in-flight reservation cannot be drained by
close_for_finalization or finish before its outcome is recorded. Track in-flight
reservations or hold equivalent state through serialized_contribution_size,
note_invalid_payload_schema, and commit_contribution, and ensure finalization
waits for or accounts for those reservations before producing the summary.
🪄 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: 4c7e800c-18bb-4a50-9bc4-ebaeb62166ce
📒 Files selected for processing (7)
crates/core/src/api/llm.rscrates/core/src/api/optimization.rscrates/core/src/observability/mod.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/plugin_component.rscrates/ffi/src/api/mod.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (19)
crates/core/src/api/{tool,llm}.rs
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Wire the new middleware chain into the execute path in
crates/core/src/api/tool.rsorcrates/core/src/api/llm.rsat the appropriate pipeline stage
Files:
crates/core/src/api/llm.rs
**/*.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/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/ffi/src/api/mod.rscrates/core/src/api/optimization.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/core/src/api/optimization.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
Files:
crates/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/core/src/api/optimization.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/ffi/src/api/mod.rscrates/core/src/api/optimization.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:
crates/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/ffi/src/api/mod.rscrates/core/src/api/optimization.rs
**/*.{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:
crates/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/ffi/src/api/mod.rscrates/core/src/api/optimization.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/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/ffi/src/api/mod.rscrates/core/src/api/optimization.rs
crates/core/src/{api/**/*.rs,api/runtime/**/*.rs,codec/**/*.rs,json.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Implement the new or changed public runtime behavior first in the Rust core, especially under
crates/core/src/api/and related core modules such ascrates/core/src/api/runtime/,crates/core/src/codec/, andcrates/core/src/json.rs.
Files:
crates/core/src/api/llm.rscrates/core/src/api/optimization.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/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/ffi/src/api/mod.rscrates/core/src/api/optimization.rs
**/*
📄 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:
crates/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/ffi/src/api/mod.rscrates/core/src/api/optimization.rs
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/core/src/api/optimization.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:
crates/core/src/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/ffi/src/api/mod.rscrates/core/src/api/optimization.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/api/llm.rscrates/core/src/observability/openinference.rscrates/core/src/observability/otel.rscrates/core/src/observability/mod.rscrates/core/src/observability/plugin_component.rscrates/core/src/api/optimization.rs
crates/core/src/observability/{atif,otel,openinference}.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
When changing the core event model, emitted fields, exporter behavior, subscriber config, or lifecycle, keep the ATIF, OpenTelemetry, and OpenInference implementations in sync.
Files:
crates/core/src/observability/openinference.rscrates/core/src/observability/otel.rs
crates/ffi/**
📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/SKILL.md)
Rebuild the FFI crate in release mode so the shared library and header stay in sync when making changes to crates/ffi
Files:
crates/ffi/src/api/mod.rs
crates/ffi/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/ffi, also usetest-ffi-surfacefor validationUse C FFI export names prefixed with
nemo_relay_in the raw C FFI layer.
Files:
crates/ffi/src/api/mod.rs
{crates/ffi/src/api/*.rs,crates/ffi/nemo_relay.h}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Add or update the shared C/FFI surface in the relevant
crates/ffi/src/api/*.rsmodule, re-export it throughcrates/ffi/src/api/mod.rs, and keep the generatedcrates/ffi/nemo_relay.hheader correct.
Files:
crates/ffi/src/api/mod.rs
crates/{python,ffi,node}/**/*
⚙️ CodeRabbit configuration file
crates/{python,ffi,node}/**/*: 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/ffi/src/api/mod.rs
🔇 Additional comments (11)
crates/core/src/observability/plugin_component.rs (2)
1947-1949: LGTM!Also applies to: 1951-1976, 1978-2008
1947-2008: 📐 Maintainability & Code QualityConfirm the required Rust/core validation passed.
As per coding guidelines, Rust changes must run
just test-rust,cargo fmt --all, andcargo clippy --workspace --all-targets -- -D warnings;crates/corechanges also requirevalidate-change, the full language matrix, anduv run pre-commit run --all-files.Source: Coding guidelines
crates/core/src/api/llm.rs (1)
691-697: LGTM!Also applies to: 744-766
crates/core/src/api/optimization.rs (2)
704-763: 📐 Maintainability & Code QualityConfirm the remaining core-runtime validation gates.
The supplied validation list does not mention
cargo deny checkor the repository’svalidate-changeworkflow.As per coding guidelines, core-runtime changes must satisfy
cargo deny checkand usevalidate-change.Source: Coding guidelines
17-19: LGTM!Also applies to: 312-316, 409-468, 470-702
crates/core/src/observability/mod.rs (1)
75-221: LGTM!crates/core/src/observability/openinference.rs (1)
1044-1049: LGTM!crates/core/src/observability/otel.rs (1)
841-846: LGTM!crates/ffi/src/api/mod.rs (3)
44-44: LGTM!Also applies to: 57-57
299-351: Refactor preserves behavior correctly.Sequential parse-with-early-return,
unwrap_or_default()for the two collection fields, and passthroughOptionforannotated_requestare consistent with the prior semantics and the AI summary.
353-365: 🎯 Functional CorrectnessRe-check diagnostic parity in
parse_optional_intercept_json. Thec_str_to_jsonfailure path should set a field-specificlast_errorif it does not already, and the call needs an explicitunsafe {}ifc_str_to_jsonis unsafe under edition 2024.
Signed-off-by: Will Killian <wkillian@nvidia.com>
#### Overview This isolates an unrelated macOS CI flake observed while validating #430. A test listener is nonblocking, but its accepted socket was read as though it were blocking; macOS returned WouldBlock and the test helper panicked. - [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 Reset the accepted mock HTTP socket to blocking mode before applying its bounded read timeout. This matches the shared accept_bounded test helper and changes test infrastructure only. Validation: - Repeated foreign_and_incompatible_listeners_are_never_adopted 30 times. - cargo clippy -p nemo-relay-cli --lib -- -D warnings - just test-rust - uv run pre-commit run --all-files #### Where should the reviewer start? Review the accepted-socket setup in crates/cli/tests/coverage/shared/bootstrap_tests.rs. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to #430 ## Summary by CodeRabbit * **Tests** * Improved test server connection handling to ensure reliable blocking I/O during request processing. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Zhongxuan (Daniel) Wang (https://github.com/zhongxuanwang-nv) URL: #435
zhongxuanwang-nv
left a comment
zhongxuanwang-nv
left a comment
There was a problem hiding this comment.
A nitty thing, and everything is great!
Signed-off-by: Will Killian <wkillian@nvidia.com>
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/core/src/api/optimization.rs`:
- Around line 48-61: Rename LlmOptimizationRecorder.state to accumulator and
Accumulator.state to inner, then update every affected access and initialization
throughout the file, including the listed call sites, so references use
self.accumulator.inner.lock() while preserving existing behavior.
🪄 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: 3cda3413-5b38-42ca-ab9e-322dc7b99832
📒 Files selected for processing (2)
crates/core/src/api/optimization.rscrates/core/tests/unit/optimization_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (14)
**/*.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/core/tests/unit/optimization_tests.rscrates/core/src/api/optimization.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/tests/unit/optimization_tests.rscrates/core/src/api/optimization.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
Files:
crates/core/tests/unit/optimization_tests.rscrates/core/src/api/optimization.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/tests/unit/optimization_tests.rscrates/core/src/api/optimization.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:
crates/core/tests/unit/optimization_tests.rscrates/core/src/api/optimization.rs
**/*.{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:
crates/core/tests/unit/optimization_tests.rscrates/core/src/api/optimization.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/core/tests/unit/optimization_tests.rscrates/core/src/api/optimization.rs
**/*
📄 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:
crates/core/tests/unit/optimization_tests.rscrates/core/src/api/optimization.rs
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/tests/unit/optimization_tests.rscrates/core/src/api/optimization.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:
crates/core/tests/unit/optimization_tests.rscrates/core/src/api/optimization.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/tests/unit/optimization_tests.rscrates/core/src/api/optimization.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/core/tests/unit/optimization_tests.rs
crates/core/src/{api/**/*.rs,api/runtime/**/*.rs,codec/**/*.rs,json.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Implement the new or changed public runtime behavior first in the Rust core, especially under
crates/core/src/api/and related core modules such ascrates/core/src/api/runtime/,crates/core/src/codec/, andcrates/core/src/json.rs.
Files:
crates/core/src/api/optimization.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/core/src/api/optimization.rs
🔇 Additional comments (4)
crates/core/src/api/optimization.rs (3)
136-136: Past clone-avoidance concern is resolved.
commit_contributionnow takes&mut Option<LlmOptimizationContribution>and only.take()s onCommitted; onRetry/Rejectedthe caller keeps itsSome(...)without cloning, addressing the earlier feedback to avoidcontribution.clone()on retry.Also applies to: 167-216
7-7: LGTM!Also applies to: 34-46, 48-61, 70-125, 230-240, 262-262, 275-275, 287-287, 301-301, 315-361, 370-383
475-895: 🎯 Functional CorrectnessAnalysis/pricing pipeline could not be reviewed line-by-line.
Raw diff text for
analyze_contributions,derive_optimization_usage,derive_baseline_usage,price_optimization_usage, andfinalize_optimization_summarywasn't provided (only summaries and two bare context lines). Downstream tests inoptimization_tests.rs(combined pricing/cost snapshot, partial-status missing-cost test) appear consistent with the described behavior, but please confirmcargo clippy --workspace --all-targets -- -D warningsand the fulloptimization_tests.rssuite pass for this hunk specifically, given its complexity (flaggedcode_block_complexity_highfor 536-677, 705-747, and 768-829).crates/core/tests/unit/optimization_tests.rs (1)
8-9: LGTM!Also applies to: 735-759
|
/merge |
2 participants
Overview
Details
Resolves all seven core-runtime and FFI findings in the main-branch Sonar backlog. Optimization recording is separated into reservation, schema validation, bounded serialization, and commit phases while retaining lock release and sequence retry behavior. Optimization summary construction is decomposed into contribution, usage, pricing, limitation, and summary phases. OpenTelemetry and OpenInference now use one private common optimization-attribute builder. LLM end handling, ATOF validation, and FFI JSON parsing are similarly split into private helpers.
Validation:
cargo fmt --allcargo clippy -p nemo-relay -p nemo-relay-ffi --all-targets -- -D warningscargo test -p nemo-relay-ffijust test-rustjust test-pythonjust test-gojust test-nodeuv run pre-commit run --all-filesNo public API, ABI, serialized configuration, telemetry key/value, or error-message changes are intended.
Where should the reviewer start?
Start with
crates/core/src/api/optimization.rs, then the common attribute helpers incrates/core/src/observability/mod.rs, and finally the smaller LLM, ATOF, and FFI extractions.Related Issues:
Summary by CodeRabbit
name(non-empty, no leading/trailing whitespace, unique).