fix(node): make callback failures catchable#523
Conversation
Closes RELAY-544 Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:M
WalkthroughNode tool and LLM execution APIs now accept JavaScript callbacks directly, validate synchronous and promise results as JSON-safe values, unwrap callback error envelopes, and preserve process usability after callback failures. ChangesNode callback safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NodeApi
participant JsCallback
participant callback_factory
participant ExecutionApi
NodeApi->>callback_factory: wrap JsFunction
callback_factory->>JsCallback: invoke callback
JsCallback-->>callback_factory: value or exception
callback_factory-->>ExecutionApi: JSON result envelope
ExecutionApi-->>NodeApi: unwrapped JSON value or FlowError
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/node/src/promise_call.rs (1)
185-236: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSame
jsonValueduplication and per-call script compilation ascallable.rs.
create_promise_wrapperembeds the identical recursivejsonValuescript assafe_execution_callbackincrates/node/src/callable.rs(lines 122-182), and also recompiles the whole factory string viaenv.run_scripteach time a promise-aware wrapper is created.See consolidated comment for the shared fix across both files.
🤖 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/node/src/promise_call.rs` around lines 185 - 236, Refactor create_promise_wrapper and safe_execution_callback to share one reusable JSON-conversion helper and one cached wrapper factory instead of embedding duplicate jsonValue implementations and calling env.run_script for every wrapper creation. Preserve the existing conversion validation and promise callback behavior while reusing the shared initialized factory.
🤖 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/node/src/callable.rs`:
- Around line 122-182: Extract the shared recursive jsonValue validator,
including its finite-number checks, circular detection, array/object traversal,
and error messages, into one reusable JavaScript source constant or builder.
Update both safe_execution_callback and create_promise_wrapper to use that
shared definition, removing their duplicated implementations while preserving
current behavior.
- Around line 127-182: Update safe_execution_callback so the JavaScript wrapper
factory is compiled once per Env and reused across invocations, using an
appropriate Env-keyed cache or lazy initialization of the resulting
JsFunction/Ref. Keep the existing wrapper behavior unchanged, and have each call
invoke the cached factory with func instead of calling env.run_script on the
full source.
---
Outside diff comments:
In `@crates/node/src/promise_call.rs`:
- Around line 185-236: Refactor create_promise_wrapper and
safe_execution_callback to share one reusable JSON-conversion helper and one
cached wrapper factory instead of embedding duplicate jsonValue implementations
and calling env.run_script for every wrapper creation. Preserve the existing
conversion validation and promise callback behavior while reusing the shared
initialized factory.
🪄 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: 1bd4d79c-1d21-445f-acfa-f13ff7d77fc1
📒 Files selected for processing (5)
crates/node/src/api/mod.rscrates/node/src/callable.rscrates/node/src/promise_call.rscrates/node/tests/llm_tests.mjscrates/node/tests/tools_tests.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (13)
crates/node/**/*.{js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
camelCasefor Node.js public APIs.
Files:
crates/node/tests/llm_tests.mjscrates/node/tests/tools_tests.mjs
**/*.{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/node/tests/llm_tests.mjscrates/node/src/promise_call.rscrates/node/src/api/mod.rscrates/node/tests/tools_tests.mjscrates/node/src/callable.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/node/tests/llm_tests.mjscrates/node/src/promise_call.rscrates/node/src/api/mod.rscrates/node/tests/tools_tests.mjscrates/node/src/callable.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/node/tests/llm_tests.mjscrates/node/src/promise_call.rscrates/node/src/api/mod.rscrates/node/tests/tools_tests.mjscrates/node/src/callable.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/node/tests/llm_tests.mjscrates/node/tests/tools_tests.mjs
**/*.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/node/src/promise_call.rscrates/node/src/api/mod.rscrates/node/src/callable.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/node/src/promise_call.rscrates/node/src/api/mod.rscrates/node/src/callable.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/node/src/promise_call.rscrates/node/src/api/mod.rscrates/node/src/callable.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/node/src/promise_call.rscrates/node/src/api/mod.rscrates/node/src/callable.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/node/src/promise_call.rscrates/node/src/api/mod.rscrates/node/src/callable.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/node/src/promise_call.rscrates/node/src/api/mod.rscrates/node/src/callable.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/node/src/promise_call.rscrates/node/src/api/mod.rscrates/node/src/callable.rs
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
crates/node/src/api/mod.rs
🔇 Additional comments (5)
crates/node/src/callable.rs (1)
379-380: LGTM!Also applies to: 573-574
crates/node/src/promise_call.rs (1)
159-164: LGTM!crates/node/tests/llm_tests.mjs (1)
187-222: LGTM!crates/node/tests/tools_tests.mjs (1)
241-316: LGTM!crates/node/src/api/mod.rs (1)
1923-1964: 🗄️ Data Integrity & IntegrationNo Python/Go follow-up needed The Python and Go
tool_call_execute/llm_call_executebindings already surface callback failures as catchable errors or takejson.RawMessagedirectly, so this Node-only crash fix does not need matching changes elsewhere.> Likely an incorrect or invalid review comment.
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/node/src/callback_factory.rs`:
- Around line 11-44: Update jsonValue to accept only plain objects in the
generic object branch, while preserving the existing Array handling. Detect and
reject non-plain-object built-ins such as Date, Map, Set, RegExp, Promise, boxed
primitives, and Error with a TypeError instead of passing them to Object.keys
and returning {}. Ensure circular-value detection and cleanup remain correct for
accepted objects.
🪄 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: 01fc1bb8-f78d-4117-add9-1aaec7abaf8e
📒 Files selected for processing (4)
crates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/lib.rscrates/node/src/promise_call.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 (10)
**/*.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/node/src/lib.rscrates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/promise_call.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/node/src/lib.rscrates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/promise_call.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/node/src/lib.rscrates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/promise_call.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/node/src/lib.rscrates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/promise_call.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/node/src/lib.rscrates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/promise_call.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/node/src/lib.rscrates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/promise_call.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/node/src/lib.rscrates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/promise_call.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/node/src/lib.rscrates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/promise_call.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/node/src/lib.rscrates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/promise_call.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/node/src/lib.rscrates/node/src/callable.rscrates/node/src/callback_factory.rscrates/node/src/promise_call.rs
🔇 Additional comments (6)
crates/node/src/callback_factory.rs (1)
46-74: 🩺 Stability & AvailabilityConfirm parity with Python/FFI binding callback-safety handling.
This introduces the canonical JSON-safety/callback-failure-isolation mechanism for Node callbacks (thrown exceptions,
BigInt, circular refs, non-finite numbers no longer abort the process). Per path instructions forcrates/{python,ffi,node}/**/*, binding changes should be checked for parity across the other language bindings. Do Python/FFI callback paths have an equivalent guard against callback exceptions or non-serializable results aborting the process, or is that tracked as separate follow-up work?As per path instructions, "Treat binding changes as public API changes. Check for parity with the other language bindings... Flag changes that update one binding without corresponding tests or documentation for the same surface elsewhere."
Source: Path instructions
crates/node/src/lib.rs (1)
19-19: LGTM!crates/node/src/callable.rs (3)
128-130: Delegation resolves prior duplication/recompilation concerns.
safe_execution_callbacknow delegates tocallback_factory::wrap_execution_callback, resolving the previously flagged duplicatedjsonValuelogic and per-callenv.run_scriptrecompilation.
327-328: Correctly unwraps the new{ok, value/error}envelope for both tool and LLM execution.Both
wrap_js_tool_exec_fn(Line 327-328) andwrap_js_llm_exec_fn(Line 521-522) now pass the raw callback result throughunwrap_middleware_result, consistent with the envelope shape produced bycallback_factory'sexecutionwrapper.Also applies to: 521-522
103-121: 🩺 Stability & Availability
safe_middleware_callbackdoesn't get the new JSON-safety validation.Unlike
safe_execution_callback/the promise wrapper (now backed bycallback_factory'sjsonValuevalidation), this middleware wrapper only normalizes top-levelundefinedand otherwise passesvaluethrough untouched. If a JS middleware callback returns aBigInt, circular structure, or non-finite number, is that still safely rejected downstream (via theJson/serde_json::Valueconversion), or can it still crash/abort the process the way execution/promise callbacks used to? Worth confirming whether this wrapper needs the same treatment as part of this effort or a tracked follow-up.crates/node/src/promise_call.rs (1)
161-166: Conversion failures now surface as errors instead of silently defaulting tonull, and the wrapper is shared viacallback_factory.Line 161-166 correctly maps a failed
ctx.get::<Json>(0)conversion into an explicitFlowError::Internalsent through the completion channel instead of swallowing it asnull. Line 197-198 delegates wrapper construction to the sharedcallback_factory::wrap_promise_callback, resolving the prior duplicatedjsonValueimplementation.Also applies to: 197-198
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/node/src/callback_factory.rs`:
- Around line 38-42: Update the callback result validation around the prototype
check to accept JSON-safe plain objects from other realms, such as objects
created by node:vm, using a realm-independent plain-object test instead of
direct Object.prototype identity checks. Add a regression test covering a
cross-realm plain object while preserving rejection of unsupported object
values.
🪄 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: b35b8028-2fff-470f-a795-d5996424b70a
📒 Files selected for processing (2)
crates/node/src/callback_factory.rscrates/node/tests/tools_tests.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
crates/node/**/*.{js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
camelCasefor Node.js public APIs.
Files:
crates/node/tests/tools_tests.mjs
**/*.{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/node/tests/tools_tests.mjscrates/node/src/callback_factory.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/node/tests/tools_tests.mjscrates/node/src/callback_factory.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/node/tests/tools_tests.mjscrates/node/src/callback_factory.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/node/tests/tools_tests.mjs
**/*.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/node/src/callback_factory.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/node/src/callback_factory.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/node/src/callback_factory.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/node/src/callback_factory.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/node/src/callback_factory.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/node/src/callback_factory.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/node/src/callback_factory.rs
🔇 Additional comments (1)
crates/node/tests/tools_tests.mjs (1)
262-263: LGTM!
| const prototype = Object.getPrototypeOf(value); | ||
| if (prototype !== Object.prototype && prototype !== null) { | ||
| seen.delete(value); | ||
| throw new TypeError('JavaScript callback returned an unsupported object value that cannot be converted to JSON'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
node - <<'NODE'
const vm = require('node:vm');
const value = vm.runInNewContext('({ value: 1 })');
console.log(Object.getPrototypeOf(value) === Object.prototype); // false
console.log(JSON.stringify(value)); // {"value":1}
NODERepository: NVIDIA/NeMo-Relay
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby context.
wc -l crates/node/src/callback_factory.rs
sed -n '1,140p' crates/node/src/callback_factory.rs
# Search for related object-conversion logic and tests.
rg -n "unsupported object value|Object.getPrototypeOf|callback returned|node:vm|cross-realm|realm" crates/node -SRepository: NVIDIA/NeMo-Relay
Length of output: 5462
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Node tests that mention vm and any docs or code that mention realm/restrictions.
sed -n '1,220p' crates/node/tests/index_loader_tests.mjs
printf '\n--- search docs/tests ---\n'
rg -n "same-realm|cross-realm|vm\\.|node:vm|Object.getPrototypeOf\\(|unsupported object value|JSON-safe|JSON safe|callback results" \
crates/node crates -SRepository: NVIDIA/NeMo-Relay
Length of output: 7834
Accept cross-realm plain objects or document the restriction
This prototype identity check rejects plain objects created in node:vm even though they are JSON-safe. If callback results may come from other realms, switch to a realm-independent plain-object check and add a regression test; otherwise document the same-realm requirement.
🤖 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/node/src/callback_factory.rs` around lines 38 - 42, Update the
callback result validation around the prototype check to accept JSON-safe plain
objects from other realms, such as objects created by node:vm, using a
realm-independent plain-object test instead of direct Object.prototype identity
checks. Add a regression test covering a cross-realm plain object while
preserving rejection of unsupported object values.
2 participants
Overview
Make Node tool and LLM callback failures reject their returned promises instead of terminating the Node process.
Details
Validation:
just build-nodejust test-nodecargo fmt --alljust test-rustcargo clippy --workspace --all-targets -- -D warningsuv run pre-commit run --all-filesWhere should the reviewer start?
Start with
safe_execution_callbackincrates/node/src/callable.rs, then compare the promise-aware validation incrates/node/src/promise_call.rsand the process-survival regressions incrates/node/tests/tools_tests.mjs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Bug Fixes
Tests