fix: make Node middleware callback errors catchable#478
Conversation
github-actions
Bot
added
size:L
Signed-off-by: Will Killian <wkillian@nvidia.com>
32eb785 to
0661bcd
Compare
|
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 (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (12)crates/node/**/*.{js,mjs,cjs,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,js,mjs,cjs,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
crates/{python,ffi,node}/**/*⚙️ CodeRabbit configuration file
Files:
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}⚙️ CodeRabbit configuration file
Files:
**/*.rs📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
**/*.{rs,py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,go,js,ts,c,h}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,go,js,ts}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
{crates/**/src/**/*.rs,python/**/*.py}📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Files:
**/*.{rs,py,go,js,ts}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Files:
🔇 Additional comments (2)
WalkthroughThe Node bindings route JavaScript guardrails, intercepts, and sanitizers through middleware callbacks that capture exceptions, preserve sanitizer payloads on failure, and expose catchable errors. Registration APIs now accept ChangesNode middleware callback handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JavaScript
participant NodeMiddleware
participant CallbackWrapper
participant CoreExecution
JavaScript->>NodeMiddleware: invoke registered callback
NodeMiddleware->>CallbackWrapper: return success or error envelope
CallbackWrapper->>CoreExecution: provide value, fallback payload, or managed error
🚥 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
🤖 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 108-109: Make the catch handler around the callable invocation
exception-safe by wrapping error formatting in a nested fallback that cannot
rethrow when accessing error.message or converting the value to a string; always
return the { ok: false, error: ... } result. Add a regression test for an object
whose message getter or string conversion throws, verifying the wrapper still
returns a failure result without propagating the exception.
In `@crates/node/tests/llm_tests.mjs`:
- Around line 494-521: Extend the LLM sanitizer adapter tests for
wrap_js_llm_sanitize_request_fn and wrap_js_llm_response_fn with throwing
callbacks. Assert each failure preserves the original request or response,
records the thrown error through getLastCallbackError(), and allows a subsequent
successful call to complete normally.
🪄 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: d4a9f43e-555e-4215-9dd3-dd8507a77873
📒 Files selected for processing (11)
crates/node/src/api/mod.rscrates/node/src/callable.rscrates/node/src/types/mod.rscrates/node/tests/adaptive_tests.mjscrates/node/tests/callback_error_tests.mjscrates/node/tests/event_sanitizers_tests.mjscrates/node/tests/llm_tests.mjscrates/node/tests/scope_local_tests.mjscrates/node/tests/tools_tests.mjsdocs/instrument-applications/advanced-guide.mdxdocs/reference/event-sanitizers.mdx
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (19)
**/*.mdx
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
MDX top-of-file SPDX comments must use {/* ... */} delimiters instead of HTML comment delimiters (Must-Fix)
In MDX files, top-of-file comments must use JSX comment delimiters (
{/*to open and*/}to close); do not use HTML comments for MDX SPDX headers
Files:
docs/instrument-applications/advanced-guide.mdxdocs/reference/event-sanitizers.mdx
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Update
README.md,fern/, package READMEs, and binding-support notes when public behavior, package names, examples, or supported bindings change.
**/*.{md,mdx}: Prefer the documented public API, not internal shortcuts
Keep package names, repo references, and build commands current
Keep release-process and release-notes guidance in repo-maintainer docs such asRELEASING.md, not as user-facing docs pages orCHANGELOG.md
Keep stable user-facing wrappers atscripts/root in docs and examples; only point at namespaced helper paths when documenting internal maintenance work
When detailed dynamic plugin guides exist, keep Rust native plugin examples, Python worker plugin examples, andgrpc-v1protocol details on separate pagesIf links in documentation change, run
just docs-linkcheck.
Files:
docs/instrument-applications/advanced-guide.mdxdocs/reference/event-sanitizers.mdx
**/*.{md,markdown,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Markdown/MDX documentation files using the HTML comment block form.
Files:
docs/instrument-applications/advanced-guide.mdxdocs/reference/event-sanitizers.mdx
{docs,examples}/**/*
📄 CodeRabbit inference engine (.agents/skills/rename-surfaces/SKILL.md)
Update docs and examples.
Files:
docs/instrument-applications/advanced-guide.mdxdocs/reference/event-sanitizers.mdx
**/*
📄 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:
docs/instrument-applications/advanced-guide.mdxdocs/reference/event-sanitizers.mdxcrates/node/tests/event_sanitizers_tests.mjscrates/node/tests/callback_error_tests.mjscrates/node/tests/scope_local_tests.mjscrates/node/tests/llm_tests.mjscrates/node/tests/adaptive_tests.mjscrates/node/tests/tools_tests.mjscrates/node/src/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.rs
docs/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If documentation examples or commands under
docs/change, run the targeted docs checks appropriate to the change.
Files:
docs/instrument-applications/advanced-guide.mdxdocs/reference/event-sanitizers.mdx
{docs/**,README.md,CONTRIBUTING.md,RELEASING.md,SECURITY.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,RELEASING.md,SECURITY.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency across language bindings.
Flag stale examples, missing SPDX headers where required, and instructions that no longer match CI or pre-commit behavior.
Files:
docs/instrument-applications/advanced-guide.mdxdocs/reference/event-sanitizers.mdx
crates/node/**/*.{js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
camelCasefor Node.js public APIs.
Files:
crates/node/tests/event_sanitizers_tests.mjscrates/node/tests/callback_error_tests.mjscrates/node/tests/scope_local_tests.mjscrates/node/tests/llm_tests.mjscrates/node/tests/adaptive_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/event_sanitizers_tests.mjscrates/node/tests/callback_error_tests.mjscrates/node/tests/scope_local_tests.mjscrates/node/tests/llm_tests.mjscrates/node/tests/adaptive_tests.mjscrates/node/tests/tools_tests.mjscrates/node/src/types/mod.rscrates/node/src/callable.rscrates/node/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/node/tests/event_sanitizers_tests.mjscrates/node/tests/callback_error_tests.mjscrates/node/tests/scope_local_tests.mjscrates/node/tests/llm_tests.mjscrates/node/tests/adaptive_tests.mjscrates/node/tests/tools_tests.mjscrates/node/src/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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/event_sanitizers_tests.mjscrates/node/tests/callback_error_tests.mjscrates/node/tests/scope_local_tests.mjscrates/node/tests/llm_tests.mjscrates/node/tests/adaptive_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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/node/src/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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/types/mod.rscrates/node/src/callable.rscrates/node/src/api/mod.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 (12)
crates/node/src/callable.rs (1)
17-17: LGTM!Also applies to: 41-41, 88-107, 110-182, 213-235, 252-263, 274-288, 350-361, 409-424, 445-452, 467-478, 614-658
crates/node/src/types/mod.rs (1)
312-328: LGTM!crates/node/src/api/mod.rs (2)
721-725: 📐 Maintainability & Code QualityConfirm the required Rust, Node, and dynamic-plugin validation.
Please provide CI or local-run confirmation for:
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningsjust test-rust- The repository’s Node test target
- The
maintain-dynamic-pluginsvalidation planuv run pre-commit run --all-filesAs per coding guidelines, Rust changes require the listed Rust checks, language-surface changes require that language’s tests, dynamic-plugin changes require
maintain-dynamic-plugins, and handoff requires the all-files pre-commit pass.Source: Coding guidelines
81-81: LGTM!Also applies to: 567-600, 765-811, 853-947, 987-987, 1115-1116, 1273-1304, 1334-1386, 1526-1527, 2383-2386, 2434-2440, 2456-2457, 2473-2474, 2489-2501, 2525-2532, 2548-2548, 2612-2685, 2707-2724, 2864-2867, 2925-2934, 2953-2954, 2971-2972, 2987-3002, 3032-3042, 3061-3061, 3138-3154, 3178-3194, 3217-3232, 3260-3281
crates/node/tests/adaptive_tests.mjs (1)
9-9: LGTM!Also applies to: 170-202
crates/node/tests/callback_error_tests.mjs (1)
70-81: LGTM!crates/node/tests/event_sanitizers_tests.mjs (1)
167-189: LGTM!Also applies to: 245-270
crates/node/tests/llm_tests.mjs (1)
607-633: LGTM!crates/node/tests/scope_local_tests.mjs (1)
208-223: LGTM!Also applies to: 714-729
crates/node/tests/tools_tests.mjs (1)
30-31: LGTM!Also applies to: 525-590, 677-701
docs/instrument-applications/advanced-guide.mdx (1)
195-202: LGTM!docs/reference/event-sanitizers.mdx (1)
52-54: LGTM!
Signed-off-by: Will Killian <wkillian@nvidia.com>
1 participant
Overview
Catch synchronous exceptions from registered Node middleware callbacks instead of allowing N-API to terminate the process.
Details
getLastCallbackError().Where should the reviewer start?
Start with
crates/node/src/callable.rsfor the callback envelope, thencrates/node/src/api/mod.rsfor the registration surfaces and the Node regression tests.Related Issues: (use one of the action keywords, "Resolves", "Fixes", "Closes")
Checklist
Summary by CodeRabbit
getLastCallbackError().