fix: preserve structured ATIF tool results#223
Conversation
Signed-off-by: dnandakumar-nv <dnandakumar@nvidia.com>
github-actions
Bot
added
size:M
WalkthroughUpdated ATIF exporter refactors observation handling to extract structured tool results into observation extras via new ChangesATIF Observation Result Structure
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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.
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/observability/atif.rs (1)
2330-2335:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStructured tool results can be dropped by later pruning.
handle_tool_endnow correctly emits structured outputs inresult.extra.tool_resultwithcontent = None, butprune_subagent_refscurrently retains results only whencontentorsubagent_trajectory_refexists (Line 2731). That drops valid extra-only tool observations and loses data.Suggested fix
- result.content.is_some() || result.subagent_trajectory_ref.is_some() + result.content.is_some() + || result.subagent_trajectory_ref.is_some() + || result.extra.is_some()🤖 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/observability/atif.rs` around lines 2330 - 2335, prune_subagent_refs is dropping valid observations that only have structured tool results in result.extra.tool_result (emitted by handle_tool_end) because it currently retains results only when content or subagent_trajectory_ref exist; update the retention condition in prune_subagent_refs to also keep AtifObservationResult entries when extra is Some and extra.tool_result is Some (i.e., check result.extra.as_ref().and_then(|e| e.tool_result.as_ref()).is_some()), so observations with content = None but a structured tool_result are preserved.
🤖 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.
Outside diff comments:
In `@crates/core/src/observability/atif.rs`:
- Around line 2330-2335: prune_subagent_refs is dropping valid observations that
only have structured tool results in result.extra.tool_result (emitted by
handle_tool_end) because it currently retains results only when content or
subagent_trajectory_ref exist; update the retention condition in
prune_subagent_refs to also keep AtifObservationResult entries when extra is
Some and extra.tool_result is Some (i.e., check
result.extra.as_ref().and_then(|e| e.tool_result.as_ref()).is_some()), so
observations with content = None but a structured tool_result are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: dba4bb98-b9d1-4d4d-b779-0148d8a2345e
📒 Files selected for processing (2)
crates/core/src/observability/atif.rscrates/core/tests/unit/atif_tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (14)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Use
snake_casenaming convention for Rust identifiers (e.g.,nemo_relay_tool_call)
**/*.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 work
**/*.rs: Runcargo fmt --allto format all Rust code
Runcargo clippy --workspace --all-targets -- -D warningsto enforce all clippy lints as errors
**/*.rs: Runcargo fmt --allwhen Rust files changed as part of WebAssembly work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files changed as part of WebAssembly work
**/*.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
Run Rust formatting withcargo fmt --all
Run Rust linting withcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Usecargo fmtfor Rust code formatting
Runcargo clippy -- -D warningsto lint Rust code and treat all warnings as errors
Use Rust snake_case naming convention for Rust identifiers
Include SPDX license header in all Rust source files using double-slash comment syntax
Validate Rust code withuv run pre-commit run --all-filesto enforce cargo fmt formatting check, cargo clippy lints, and cargo deny aud...
Files:
crates/core/src/observability/atif.rscrates/core/tests/unit/atif_tests.rs
crates/core/src/observability/{atif,otel,openinference}.rs
📄 CodeRabbit inference engine (.agents/skills/maintain-observability/SKILL.md)
When changing event fields in ATIF, OpenTelemetry, or OpenInference observability surfaces, keep the core event model in
crates/core/src/observability/atif.rs,crates/core/src/observability/otel.rs, andcrates/core/src/observability/openinference.rsin sync
Files:
crates/core/src/observability/atif.rs
**/{Cargo.toml,**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Maintain consistency between Rust package names in
Cargo.tomland their actual usage across the codebase
Files:
crates/core/src/observability/atif.rscrates/core/tests/unit/atif_tests.rs
**/*.{h,hpp,c,cpp,rs}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Ensure FFI header and library naming follows consistent conventions across platform-specific builds
Files:
crates/core/src/observability/atif.rscrates/core/tests/unit/atif_tests.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/observability/atif.rscrates/core/tests/unit/atif_tests.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (.agents/skills/rename-surfaces/SKILL.md)
Update Rust crate names and module prefixes during coordinated rename operations
Files:
crates/core/src/observability/atif.rscrates/core/tests/unit/atif_tests.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
crates/core/**/*.rs: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths. Keep errors explicit and binding-appropriate at the wrapper layer.
Files:
crates/core/src/observability/atif.rscrates/core/tests/unit/atif_tests.rs
crates/{core,adaptive}/**
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full matrix across Rust, Python, Go, Node.js, and WebAssembly
Files:
crates/core/src/observability/atif.rscrates/core/tests/unit/atif_tests.rs
**/*.{rs,py,js,ts,tsx,jsx,go,sh,toml,yaml,yml,md}
📄 CodeRabbit inference engine (AGENTS.md)
Keep SPDX headers on source, docs, scripts, and configuration files. The project is Apache-2.0.
Files:
crates/core/src/observability/atif.rscrates/core/tests/unit/atif_tests.rs
**/*.{rs,py,go,js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions: Rust and Python use
snake_case, C FFI exports prefixednemo_relay_, Go usesPascalCasefor public APIs, Node.js usescamelCase.
Files:
crates/core/src/observability/atif.rscrates/core/tests/unit/atif_tests.rs
crates/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/**/*.rs: Keep async behavior on the existing tokio-based model. Bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
UseJson = serde_json::Valuein Rust-facing runtime APIs for JSON payload handling.
Files:
crates/core/src/observability/atif.rscrates/core/tests/unit/atif_tests.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/observability/atif.rscrates/core/tests/unit/atif_tests.rs
{crates/adaptive/**/*.rs,**/*test*.{rs,py,go,ts,js},**/*adaptive*test*.{rs,py,go,ts,js},docs/plugins/adaptive/**}
📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Maintain documented and tested validation and report behavior for adaptive surfaces
Files:
crates/core/tests/unit/atif_tests.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/atif_tests.rs
🔇 Additional comments (2)
crates/core/src/observability/atif.rs (1)
522-546: LGTM!Also applies to: 2677-2698
crates/core/tests/unit/atif_tests.rs (1)
388-397: LGTM!Also applies to: 399-405, 491-491, 528-556, 558-585, 966-969, 1212-1212, 1373-1376, 1751-1751, 1864-1864, 3054-3054
|
/ok to test c727dbe |
|
/merge |
2 participants
Overview
Fix ATIF v1.7 export so tool observation result
contentis never serialized as an arbitrary JSON object. Structured tool outputs are now preservedunder
extra.tool_result, while valid string and content-part array outputs remain incontent.Details
AtifObservationResult.contentto ATIF-compatible values:{"status":"closed_by_turn_end"}, inAtifObservationResult.extra.tool_result.extrametadata when tool observations and subagent references are combined.observation.results[].contentis rejected.Validation run:
cargo fmt --allcargo test -p nemo-relay observability::atif::testsjust test-rustcargo clippy --workspace --all-targets -- -D warningsjust test-pythonjust test-gojust test-nodejust test-wasmuv run pre-commit run --all-filesgit diff --checkWhere should the reviewer start?
Start with
crates/core/src/observability/atif.rs, especially the observation content/extra helpers and observation merge path. The focusedregression coverage is in
crates/core/tests/unit/atif_tests.rs.Summary by CodeRabbit