feat!: add canonical tool execution results - #575
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
📜 Recent review details⏰ Context from checks skipped due to timeout. (41)
🧰 Additional context used📓 Path-based instructions (11)**/*.rs📄 CodeRabbit inference engine (.agents/skills/test-ffi-surface/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:
**/*.{rs,py,js,mjs,ts,go,c,h}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,js,mjs,ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,go,js,ts,html,md,mdx,toml}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,c,h}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,toml}📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
**/*.{rs,py,pyi,go,js,ts}📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Files:
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (16)📓 Common learnings📚 Learning: 2026-08-07T22:42:49.565ZApplied to files:
📚 Learning: 2026-08-04T01:50:51.021ZApplied to files:
📚 Learning: 2026-08-04T01:51:02.652ZApplied to files:
📚 Learning: 2026-08-04T01:51:02.652ZApplied to files:
📚 Learning: 2026-08-07T22:42:49.565ZApplied to files:
📚 Learning: 2026-08-07T22:42:49.565ZApplied to files:
📚 Learning: 2026-08-07T22:42:49.565ZApplied to files:
📚 Learning: 2026-08-12T18:42:21.566ZApplied to files:
📚 Learning: 2026-08-07T22:42:49.565ZApplied to files:
📚 Learning: 2026-08-07T22:42:49.565ZApplied to files:
📚 Learning: 2026-08-12T18:42:21.566ZApplied to files:
📚 Learning: 2026-08-03T19:55:03.931ZApplied to files:
📚 Learning: 2026-08-12T18:42:21.566ZApplied to files:
📚 Learning: 2026-07-02T18:33:14.361ZApplied to files:
📚 Learning: 2026-05-26T21:03:12.012ZApplied to files:
🔇 Additional comments (8)
WalkthroughThe pull request introduces canonical ChangesCanonical execution result contract
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🟡 Moderate · up to The PR introduces canonical tool-result propagation and changes observability sanitization, but sanitizer callback failures can still drop telemetry contrary to the required fail-open behavior, and a modified plugin README lacks the required license header. The observability issue should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant ToolCallback
participant InterceptChain
participant ToolLifecycle
participant Sanitizer
participant Exporters
ToolCallback->>InterceptChain: return ToolExecutionResult
InterceptChain->>InterceptChain: preserve or modify result and annotation
InterceptChain->>ToolLifecycle: return result and pending marks
ToolLifecycle->>Sanitizer: sanitize result and annotation at separate stages
ToolLifecycle->>Exporters: project sanitized tool result annotation
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
1b1ebb7 to
6ea55f1
Compare
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
6ea55f1 to
78931ea
Compare
78931ea to
09e7362
Compare
6b8b633 to
4002875
Compare
b49ce6c to
e0e438d
Compare
e0e438d to
3e37def
Compare
3e37def to
d09b621
Compare
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
d09b621 to
695cc60
Compare
mnajafian-nv
left a comment
There was a problem hiding this comment.
Great work, I don’t have anything new beyond Will’s open review comments here. I’m good to approve once those are addressed.
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
/merge |
1 similar comment
|
/merge |
Overview
This PR makes one canonical tool-result contract apply across the runtime, bindings, plugins, and observability surfaces.
Warning
BREAKING CHANGE: [Tool execution APIs and plugin wire contracts] Tool producers, execution-intercept continuations, managed execution returns, and manual completion no longer exchange raw JSON; they now use the canonical
ToolExecutionResult { result, annotation }contract. Update forwarding intercepts to preserve or deliberately replace/removeannotation, and update Rust, Python, Node.js, C/FFI, and Go call sites to the new result shape. Relay 0.8 resets the tool-result semantics undernative_api = "1"andworker_protocol = "grpc-v1". Rebuild every native and worker plugin and declarecompat.relayso it excludes versions before 0.8 (recommended:>=0.8.0,<1.0). Native ABI v4 layouts remain unchanged. Workers retain thegrpc-v1identifier andnemo.relay.worker.v1package, service names, and RPC method names, but the protobuf contract changes:RelayHostRuntime.ToolNextnow returnsToolExecutionResultResponse, andToolExecutionInterceptResult.outcomeis a typedToolExecutionInterceptOutcomeinstead of aJsonEnvelope. Regenerate custom protobuf bindings before rebuilding workers.Tool producers, execution continuations, managed execution, and manual completion now exchange:
Tool execution intercepts return the flat companion contract:
Details
ToolExecutionResultin Rust, Python, Node.js, C/FFI, Go, native plugins, and the Rust/Python worker SDKs.pending_marksRelay-owned while allowing each execution intercept to explicitly preserve, replace, remove, or short-circuit the opaque annotation.resultorannotationvalue.content,structuredContent,_meta, andisError—insideresult; Relay annotations remain adjacent to, and independent from, MCP_meta.result; they cannot inspect or rewriteannotation. Relay then attaches each non-null annotation atcategory_profile.tool_result_annotationbefore the general scope-end event-sanitizer chain, which can replace or removedata,category_profile(including the annotation), and eventmetadatabefore subscribers and exporters receive the event.tool_outputtoresultand the projected annotation as independent JSON documents.target_pathsare relative to each document; root removal omits the annotation key, while other typed tool profile fields remain unchanged.ToolExecutionResultto the application.grpc-v1protocol identifier andnemo.relay.worker.v1package, service names, and RPC method names, while structurally defining theToolExecutionResultandToolExecutionInterceptOutcomewrappers in protobuf. Their application-owned result, annotation, and pending-mark array values use losslessJsonValuebytes rather thangoogle.protobuf.Valueso arbitrary JSON and integer precision are preserved.RelayHostRuntime.ToolNextfromJsonResulttoToolExecutionResultResponseandToolExecutionInterceptResult.outcomefromJsonEnvelopeto typedToolExecutionInterceptOutcome. All workers must regenerate bindings and rebuild for the Relay 0.8 baseline.compat.relayrange that excludes pre-0.8 Relay releases. Plugin loading separately retains the existing check that the range matches the running host.tool_call_idbehavior unchanged in this PR; [Enhancement]: Add MCP and tool ID for tool-call provenance and correlation #446 remains a focused follow-up.Breaking surfaces include callback return values, execution-intercept
next, managed execute return values, manual tool completion input, and the two worker protobuf type seams above. Native plugin ABI v4 itself does not change.Focused validation completed on the current implementation:
cargo check -p nemo-relay-worker-proto -p nemo-relay-worker -p nemo-relay --features worker-grpc --lockedcargo clippy -p nemo-relay-worker-proto -p nemo-relay-worker -p nemo-relay --all-targets --features worker-grpc -- -D warningscargo fmt --all -- --checkcargo test -p nemo-relay-worker-proto;cargo test -p nemo-relay-worker --test worker_sdk_tests; and focused host-worker continuation/error tests passed, including structural wrappers, lossless JSON, null normalization, malformed payloads, and pending marksToolNext, result mutation, annotation preservation, and pending marks. The test compiled locally and runs end to end when the lifecycle-managed Python environment is provisionedgrpcio-toolsgeneration verified theToolNext, tool-result, and outcome descriptorsgit diff --checkThe local fixture rebuild and full matrix were intentionally deferred because the Mac data partition had only 2.7 GiB free. CI runs the complete generated-binding, fixture, and cross-language matrix for this head.
Where should the reviewer start?
Start with
crates/types/src/api/tool.rsfor the canonical application contracts andcrates/worker-proto/proto/nemo/relay/worker/v1/plugin_worker.protofor their structural worker representation. Then reviewcrates/core/src/plugin/dynamic/worker.rs,crates/worker/src/lib.rs, andpython/plugin/src/nemo_relay_plugin/_api.pyfor the local host/SDK conversions and enforcement.crates/worker-proto/src/lib.rsintentionally remains a generic generated-protocol and JSON-value helper crate.For runtime semantics, review
crates/core/src/api/runtime/state.rsandcrates/core/src/api/tool.rs.crates/core/tests/integration/middleware_tests.rscaptures annotation propagation, repeated/concurrent continuation behavior, pending-mark ownership, sanitization order, error behavior, and MCP-shaped result regressions.For dynamic compatibility, review
crates/core/src/plugin/dynamic.rs,crates/core/src/plugin/dynamic/native.rs,crates/core/src/plugin/dynamic/worker.rs, anddocs/reference/migration-guides.mdxtogether. The key design choice is a Relay 0.8 cutover under the existing v1 identifiers: every plugin must rebuild and exclude pre-0.8 Relay versions. Native ABI v4 remains unchanged; workers retain the v1 identifier/package/API names but must regenerate bindings for the new structural tool-result protobuf types.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
resultand optional opaqueannotation.Breaking Changes
Updates
grpc-v1, with a Relay 0.8 rebuild requirement and updated migration guidance.