baml_language/feat: add bridge_nodejs crate with BamlRuntime and callFunctionSync#3345
Merged
baml_language/feat: add bridge_nodejs crate with BamlRuntime and callFunctionSync#3345
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a new Node.js N‑API bridge crate Changes
Sequence Diagram(s)sequenceDiagram
actor Client as Client (JS/TS)
participant Index as index.js / index.ts
participant Proto as proto.js / proto.ts
participant NativeLoader as native.js
participant Native as nativeBinding (N-API)
participant Runtime as BamlRuntime (Rust)
participant Collector as Collector (Rust)
Client->>Index: callFunctionSync(rt, name, kwargs, ctx?, collectors?)
Index->>Proto: encodeCallArgs(kwargs)
Proto-->>Index: Buffer (args)
Index->>NativeLoader: call rt.callFunctionSync(..., args)
NativeLoader->>Native: forward call
Native->>Runtime: invoke runtime with args
Runtime->>Collector: record logs / timing / usage
Runtime-->>Native: Buffer (result)
Native-->>Index: Buffer (result)
Index->>Proto: decodeCallResult(Buffer)
Proto-->>Index: decoded value
Index-->>Client: FunctionResult(result)
sequenceDiagram
actor Client as Client (JS/TS)
participant CtxMgr as CtxManager (TS)
participant ALS as AsyncLocalStorage
participant Span as HostSpanManager (N-API)
Client->>CtxMgr: traceFn(name, fn)
CtxMgr->>Span: cloneContext()
CtxMgr->>Span: enter(name, argsObj)
CtxMgr->>ALS: run(clonedSpan, () => fn(...))
ALS->>Span: execute within span context
Span-->>CtxMgr: exitOk() or exitError()
CtxMgr-->>Client: return or rethrow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcae2dfb90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Merging this PR will not alter performance
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
baml_language/crates/bridge_nodejs/tests/run_jest.rs (1)
12-17: Use a frozen lockfile for deterministic JS dependency resolution.Using plain
pnpm installcan introduce CI flakiness when registry state changes.♻️ Suggested change
- let install = Command::new("pnpm") - .arg("install") + let install = Command::new("pnpm") + .args(["install", "--frozen-lockfile"]) .current_dir(manifest_dir) .status() .expect("failed to run pnpm install");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/tests/run_jest.rs` around lines 12 - 17, The test currently runs pnpm install which can produce non-deterministic installs; update the Command invocation that builds `install` (the Command::new("pnpm") block) to pass the frozen-lockfile flag (for example replace the single "install" arg with "install" and "--frozen-lockfile" or use the equivalent CI command) so pnpm will fail if the lockfile is out of date and ensure deterministic dependency resolution; keep the same `.status().expect(...)` and `assert!(install.success(), ...)` checks.baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts (1)
63-65: Use numeric sort comparator to future-proof the assertion.
Array.prototype.sort()defaults to lexicographic ordering; numeric comparator avoids latent brittleness.♻️ Suggested change
- expect(results.sort()).toEqual([1, 2, 3]); + expect(results.sort((a, b) => a - b)).toEqual([1, 2, 3]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts` around lines 63 - 65, The test currently uses results.sort() which sorts lexicographically; change the assertion to use a numeric comparator so ordering is numeric — e.g. replace expect(results.sort()).toEqual([1,2,3]) with expect(results.sort((a,b) => a - b)).toEqual([1,2,3]) (or use a non-mutating copy like expect([...results].sort((a,b)=>a-b)).toEqual([1,2,3])) referencing the task invocations and the results array.baml_language/crates/bridge_nodejs/tests/call_function_sync.test.ts (1)
65-112: Add at least one failure-path assertion forcallFunctionSync.Happy-path coverage is great; a single unknown-function or invalid-kwargs case would harden regression detection on JS↔Rust error translation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/tests/call_function_sync.test.ts` around lines 65 - 112, Add a failing-path test that calls callFunctionSync with an unknown function name and with invalid kwargs to assert proper JS↔Rust error translation: add one or two new it() blocks that call callFunctionSync(rt, 'NonExistentFunction', {}) and expect the call to throw (or reject) and that the thrown Error message contains the function name or "not found", and another that calls callFunctionSync(rt, 'ReturnNumber', { n: 'not-a-number' }) and expects a thrown Error whose message indicates invalid/incorrect kwargs; reference callFunctionSync, BamlRuntime and the existing happy-path tests to place the new assertions consistently.baml_language/crates/bridge_nodejs/tests/test_engine.test.ts (1)
127-137: Add one integration test that aborts an active call.Current abort tests validate state flips only; they don’t verify runtime cancellation behavior end-to-end.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/tests/test_engine.test.ts` around lines 127 - 137, Add a new test under the existing describe('AbortController') suite named 'aborts an active call' that verifies runtime cancellation: create an AbortController, start an asynchronous operation that accepts an AbortSignal (e.g., a short helper longRunningOperation(signal) or the actual async API under test) so it can be cancelled, call ac.abort() while the operation is pending, then assert the operation's promise rejects with an abort-related error (e.g., throws AbortError or includes "abort") and that ac.aborted is true; use the identifiers test('aborts an active call'), AbortController, and the async operation helper (longRunningOperation or the real async call) to locate and add this test.baml_language/crates/bridge_nodejs/typescript_src/index.ts (1)
40-42: Consider makingFunctionResult.toString()more robust against edge cases.While the current
decodeCallResult()implementation converts all numeric values toNumbertype and doesn't produce circular references, defensive error handling intoString()is still reasonable to protect against unexpected future changes or edge cases.If keeping the current implementation, add basic error handling:
Alternative implementation
toString(): string { - return `FunctionResult(${JSON.stringify(this._value)})`; + try { + return `FunctionResult(${JSON.stringify(this._value)})`; + } catch { + return `FunctionResult(${String(this._value)})`; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts` around lines 40 - 42, FunctionResult.toString() currently returns JSON.stringify(this._value) which can throw or produce undesirable output if future changes introduce non-serializable or circular values; wrap the stringify in a try/catch inside FunctionResult.toString() and on error return a safe fallback (e.g. a descriptive string including typeof this._value and a short inspected representation), optionally using Node's util.inspect when available, and keep decodeCallResult() assumptions noted in a comment so the defensive handling is clearly justified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@baml_language/crates/bridge_nodejs/native.js`:
- Around line 66-73: In requireNative(), when
process.env.NAPI_RS_NATIVE_LIBRARY_PATH is present and
require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) succeeds and assigns to
nativeBinding, immediately return that binding instead of falling through;
specifically update the branch in function requireNative so that after assigning
nativeBinding = require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) you return
nativeBinding (or return the required module) to avoid later overwriting at the
end of the function.
In `@baml_language/crates/bridge_nodejs/proto`:
- Line 1: The repo currently contains a Git symlink named "proto" pointing to
"typescript_src/proto", which breaks on checkouts without symlink support;
replace this symlink by copying or generating the target directory at
build/publish time so consumers always have a real "proto" directory. Update the
packaging/build scripts (where proto is produced) to perform a recursive copy of
"typescript_src/proto" into a real "proto" directory (or regenerate the files
into "proto") before creating the package or committing artifacts, and remove
the symlink from the repository so imports like "./proto/..." resolve reliably.
In `@baml_language/crates/bridge_nodejs/proto.js`:
- Around line 13-54: The decode/encode logic must preserve BamlHandle objects
rather than turning them into plain { key, handleType } maps: update the
decoding path to construct a BamlHandle (not a plain object) when encountering a
handleValue, and update setInboundValue to special-case inputs that are
BamlHandle instances (encode them as handleValue) instead of treating them as
generic objects and serializing to mapValue; reference the functions/setters
setInboundValue, handleValue, mapValue, and the BamlHandle class and ensure
encodeCallArgs treats BamlHandle inputs via the handleValue branch.
In `@baml_language/crates/bridge_nodejs/src/errors.rs`:
- Around line 56-59: The BridgeError::Internal arm is losing the BamlClientError
marker so JS wrapNativeError can't classify it; update the napi::Error creation
in the BridgeError::Internal match arm to include the BamlClientError marker in
the error identity (e.g., prefix the message with "BamlClientError:" or
otherwise embed that marker in the formatted string) when calling
napi::Error::new(Status::GenericFailure, ...), so wrapNativeError can map it to
BamlClientError.
In `@baml_language/crates/bridge_nodejs/src/handle.rs`:
- Around line 20-27: The BamlHandle constructor is exposed to JS allowing forged
keys; remove the #[napi(constructor)] exposure and make the constructor
non-public so JS cannot instantiate BamlHandle directly (e.g., change pub fn
new(...) to fn new(...) and remove the napi constructor attribute), and ensure
only bridge-side code creates BamlHandle instances; also apply the same change
to the other constructor-like code referenced (lines 39-56) and keep
clone_handle(), finalize(), and HANDLE_TABLE usage unchanged but only called on
bridge-created BamlHandle objects.
In `@baml_language/crates/bridge_nodejs/src/types/collector.rs`:
- Around line 107-117: The getter FunctionLog::result currently swallows
conversion errors by mapping any external_to_baml_value failure to None; change
its signature to return Result<Option<Buffer>> and propagate conversion errors
so they surface to JS as exceptions: keep using
bridge_ctypes::HandleTableOptions::for_in_process() and
self.inner.result.as_ref(), but when external_to_baml_value(val,
&handle_options) returns Err, return Err(...) instead of logging and returning
None; on Ok(baml_val) wrap Buffer::from(baml_val.encode_to_vec()) in
Ok(Some(...)) and if self.inner.result is None return Ok(None) so napi-rs 3.x
will convert Rust Err into a JS exception.
In `@baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts`:
- Around line 11-19: The constructor in CtxManager currently registers
process.on('exit', ...) every time an instance is created which leads to
duplicate flushEvents() calls; fix this by registering the exit hook only
once—either move the process.on('exit') registration to module scope (so it runs
on import) or add a static/shared guard flag on the CtxManager class that
ensures the handler (which calls flushEvents()) is installed only the first
time; update references in the constructor (CtxManager(rt: unknown) and
HostSpanManager usage) to remove per-instance registration and ensure
flushEvents remains called exactly once on process exit.
In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts`:
- Around line 110-113: Replace the module-level process.on('exit', ...)
registration with process.once('exit', ...) so flushEvents is guaranteed to run
only once on process termination; specifically update the handler that calls
flushEvents in index.ts to use process.once, and apply the same change to the
exit registration inside the CtxManager constructor (ctx_manager.ts) where
process.on('exit') is currently used to avoid registering duplicate exit
handlers across module reloads or multiple CtxManager instances.
In `@baml_language/crates/bridge_nodejs/typescript_src/native.d.ts`:
- Line 1: The package's declaration entrypoint "native.d.ts" is currently a
symlink placeholder that contains just "../native.d.ts", which breaks type
resolution on Windows; replace the symlink with a real .d.ts file (or update the
build/publish step to copy the target declarations into this file) so the file
contains the actual type declarations or proper re-exports instead of a textual
path; ensure the actual declarations from the real target are included (or
copied) during prepare/publish and that "native.d.ts" exports the same symbols
as the original declarations.
In `@baml_language/crates/bridge_nodejs/typescript_src/proto.ts`:
- Around line 20-24: The encoder/decoder currently uses
Number.isInteger/Number(...) which can silently truncate 64-bit integers; update
the Value encoding/decoding logic around iv (the encoder branch that sets
iv.intValue/iv.floatValue) and the decode paths that call Number(...) to
properly handle protobuf Longs: detect Long instances (Long.isLong(value)) and
use Long.isSafeInteger(value) or Number.isSafeInteger(value) before converting
to a JS Number; if safe, use value.toNumber(), otherwise preserve the full
integer by keeping the Long (assign the Long directly if the generated types
accept it) or serialize as a string via value.toString(); replace any direct
Number(...) conversion with these safe checks (use value.toNumber(),
value.toString(), or keep the Long) so 64-bit values are not lost.
In `@tools/build`:
- Around line 431-434: The current command string ends the build with `; date`
so the `pnpm test` added when `_test_mode` is set can run even if `pnpm
build:debug` failed; update the `command` construction (the variable named
`command` where `pnpm build:debug` and `date` are composed) to use `&&` so the
date (and subsequent `pnpm test`) only run on successful build (i.e., change `;
date` to `&& date`), and when `_test_mode` is enabled append test steps guarded
by success (e.g., `&& pnpm test`). Additionally, ensure Rust tests are run for
the crate by invoking `cargo test --lib` for the bridge crate when Rust sources
changed (or unconditionally in the `_test_mode` branch) so that `cargo test
--lib` is executed after a successful build.
---
Nitpick comments:
In `@baml_language/crates/bridge_nodejs/tests/call_function_sync.test.ts`:
- Around line 65-112: Add a failing-path test that calls callFunctionSync with
an unknown function name and with invalid kwargs to assert proper JS↔Rust error
translation: add one or two new it() blocks that call callFunctionSync(rt,
'NonExistentFunction', {}) and expect the call to throw (or reject) and that the
thrown Error message contains the function name or "not found", and another that
calls callFunctionSync(rt, 'ReturnNumber', { n: 'not-a-number' }) and expects a
thrown Error whose message indicates invalid/incorrect kwargs; reference
callFunctionSync, BamlRuntime and the existing happy-path tests to place the new
assertions consistently.
In `@baml_language/crates/bridge_nodejs/tests/run_jest.rs`:
- Around line 12-17: The test currently runs pnpm install which can produce
non-deterministic installs; update the Command invocation that builds `install`
(the Command::new("pnpm") block) to pass the frozen-lockfile flag (for example
replace the single "install" arg with "install" and "--frozen-lockfile" or use
the equivalent CI command) so pnpm will fail if the lockfile is out of date and
ensure deterministic dependency resolution; keep the same
`.status().expect(...)` and `assert!(install.success(), ...)` checks.
In `@baml_language/crates/bridge_nodejs/tests/test_engine.test.ts`:
- Around line 127-137: Add a new test under the existing
describe('AbortController') suite named 'aborts an active call' that verifies
runtime cancellation: create an AbortController, start an asynchronous operation
that accepts an AbortSignal (e.g., a short helper longRunningOperation(signal)
or the actual async API under test) so it can be cancelled, call ac.abort()
while the operation is pending, then assert the operation's promise rejects with
an abort-related error (e.g., throws AbortError or includes "abort") and that
ac.aborted is true; use the identifiers test('aborts an active call'),
AbortController, and the async operation helper (longRunningOperation or the
real async call) to locate and add this test.
In `@baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts`:
- Around line 63-65: The test currently uses results.sort() which sorts
lexicographically; change the assertion to use a numeric comparator so ordering
is numeric — e.g. replace expect(results.sort()).toEqual([1,2,3]) with
expect(results.sort((a,b) => a - b)).toEqual([1,2,3]) (or use a non-mutating
copy like expect([...results].sort((a,b)=>a-b)).toEqual([1,2,3])) referencing
the task invocations and the results array.
In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts`:
- Around line 40-42: FunctionResult.toString() currently returns
JSON.stringify(this._value) which can throw or produce undesirable output if
future changes introduce non-serializable or circular values; wrap the stringify
in a try/catch inside FunctionResult.toString() and on error return a safe
fallback (e.g. a descriptive string including typeof this._value and a short
inspected representation), optionally using Node's util.inspect when available,
and keep decodeCallResult() assumptions noted in a comment so the defensive
handling is clearly justified.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 167973b9-e151-4698-92f9-428329df55c2
⛔ Files ignored due to path filters (12)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/crates/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_python/uv.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
baml_language/Cargo.tomlbaml_language/crates/bridge_nodejs/Cargo.tomlbaml_language/crates/bridge_nodejs/build.rsbaml_language/crates/bridge_nodejs/ctx_manager.d.tsbaml_language/crates/bridge_nodejs/ctx_manager.jsbaml_language/crates/bridge_nodejs/errors.d.tsbaml_language/crates/bridge_nodejs/errors.jsbaml_language/crates/bridge_nodejs/index.d.tsbaml_language/crates/bridge_nodejs/index.jsbaml_language/crates/bridge_nodejs/native.d.tsbaml_language/crates/bridge_nodejs/native.jsbaml_language/crates/bridge_nodejs/package.jsonbaml_language/crates/bridge_nodejs/protobaml_language/crates/bridge_nodejs/proto.d.tsbaml_language/crates/bridge_nodejs/proto.jsbaml_language/crates/bridge_nodejs/src/abort_controller.rsbaml_language/crates/bridge_nodejs/src/errors.rsbaml_language/crates/bridge_nodejs/src/handle.rsbaml_language/crates/bridge_nodejs/src/lib.rsbaml_language/crates/bridge_nodejs/src/runtime.rsbaml_language/crates/bridge_nodejs/src/types/collector.rsbaml_language/crates/bridge_nodejs/src/types/host_span_manager.rsbaml_language/crates/bridge_nodejs/src/types/mod.rsbaml_language/crates/bridge_nodejs/tests/call_function_sync.test.tsbaml_language/crates/bridge_nodejs/tests/run_jest.rsbaml_language/crates/bridge_nodejs/tests/test_collector.test.tsbaml_language/crates/bridge_nodejs/tests/test_engine.test.tsbaml_language/crates/bridge_nodejs/tests/test_tracing.test.tsbaml_language/crates/bridge_nodejs/tsconfig.jsonbaml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/crates/bridge_nodejs/typescript_src/errors.tsbaml_language/crates/bridge_nodejs/typescript_src/index.tsbaml_language/crates/bridge_nodejs/typescript_src/native.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.jsmise.tomltools/build
Binary size checks passed✅ 5 passed
Generated by |
sxlijin
force-pushed
the
push-rtupkwyokymy
branch
from
dcae2df to
34edda7
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
baml_language/crates/bridge_nodejs/native.js (1)
74-77:⚠️ Potential issue | 🔴 CriticalReturn the env-override binding immediately.
When
NAPI_RS_NATIVE_LIBRARY_PATHsucceeds here,requireNative()still falls through and returnsundefined, so Line 369 overwrites the loaded module. That makes the override path unusable.Minimal fix
if (process.env.NAPI_RS_NATIVE_LIBRARY_PATH) { try { - nativeBinding = require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH); + return require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) } catch (err) { loadErrors.push(err) } } else if (process.platform === 'android') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/native.js` around lines 74 - 77, The env-override path in requireNative() loads the native module into nativeBinding but doesn't return it, allowing later logic to overwrite it; update requireNative() so that when process.env.NAPI_RS_NATIVE_LIBRARY_PATH is set and require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) succeeds, the function immediately returns that nativeBinding (or the required module) instead of falling through, ensuring the environment override takes effect.baml_language/crates/bridge_nodejs/typescript_src/index.ts (1)
110-113:⚠️ Potential issue | 🟠 MajorGuard the exit hook against re-registration.
This adds a new exit listener every time the module is reloaded.
process.once(...)alone is not enough here—multipleonceregistrations made before shutdown still all run—so this needs a process-wide guard instead.ctx_manager.tsshould share the same guard.Possible fix
+const globalState = globalThis as typeof globalThis & { + __bamlFlushHookRegistered?: boolean; +}; + // Register flush on process exit -process.on('exit', () => { - try { flushEvents(); } catch (_) { /* ignore */ } -}); +if (!globalState.__bamlFlushHookRegistered) { + globalState.__bamlFlushHookRegistered = true; + process.once('exit', () => { + try { flushEvents(); } catch (_) { /* ignore */ } + }); +}In Node.js, if `process.once('exit', handler)` is called multiple times before process exit, does each registered handler still run once, or are duplicate registrations deduplicated?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts` around lines 110 - 113, The exit hook is being registered multiple times; change the registration in index.ts (where flushEvents is bound) to only register once process-wide by using a shared guard (e.g. a module/global flag named like exitFlushRegistered or a uniquified symbol on process) before calling process.once/process.on, and update ctx_manager.ts to check/set the same guard so both modules share the same single registration; ensure the guard prevents repeat registration even if the module is reloaded.baml_language/crates/bridge_nodejs/src/types/collector.rs (1)
106-117:⚠️ Potential issue | 🟠 MajorDon’t collapse serialization failures into
null.A real “no result” and an
external_to_baml_value()failure both surface as JSnullhere, which silently drops bridge errors. ReturnResult<Option<Buffer>>so napi-rs can throw conversion failures instead.Minimal fix
- pub fn result(&self) -> Option<Buffer> { + pub fn result(&self) -> Result<Option<Buffer>> { let handle_options = bridge_ctypes::HandleTableOptions::for_in_process(); - self.inner.result.as_ref().and_then(|val| { - match external_to_baml_value(val, &handle_options) { - Ok(baml_val) => Some(Buffer::from(baml_val.encode_to_vec())), - Err(e) => { - log::warn!("FunctionLog.result: failed to convert value: {e}"); - None - } - } - }) + self.inner + .result + .as_ref() + .map(|val| { + let baml_val = external_to_baml_value(val, &handle_options).map_err(|e| { + napi::Error::from_reason(format!( + "FunctionLog.result: failed to convert value: {e}" + )) + })?; + Ok(Buffer::from(baml_val.encode_to_vec())) + }) + .transpose() }Does napi-rs 3.x support `#[napi(getter)]` methods returning `Result<Option<Buffer>>`, and does returning `Err(napi::Error)` from a getter throw into JavaScript?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/src/types/collector.rs` around lines 106 - 117, The getter FunctionLog::result currently swallows conversion failures and returns Option<Buffer>; change its signature to pub fn result(&self) -> napi::Result<Option<Buffer>> (or Result<Option<Buffer>, napi::Error>) so conversion errors surface to JS, use bridge_ctypes::HandleTableOptions::for_in_process as before, call external_to_baml_value and map Ok(baml_val) to Some(Buffer::from(baml_val.encode_to_vec())), but map Err(e) to return Err(napi::Error::from_reason(format!("failed to convert value: {}", e))) instead of logging and returning None; keep the #[napi(getter)] attribute and ensure the function returns Err to let napi-rs throw into JavaScript.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@baml_language/crates/bridge_nodejs/native.js`:
- Around line 74-77: The env-override path in requireNative() loads the native
module into nativeBinding but doesn't return it, allowing later logic to
overwrite it; update requireNative() so that when
process.env.NAPI_RS_NATIVE_LIBRARY_PATH is set and
require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) succeeds, the function
immediately returns that nativeBinding (or the required module) instead of
falling through, ensuring the environment override takes effect.
In `@baml_language/crates/bridge_nodejs/src/types/collector.rs`:
- Around line 106-117: The getter FunctionLog::result currently swallows
conversion failures and returns Option<Buffer>; change its signature to pub fn
result(&self) -> napi::Result<Option<Buffer>> (or Result<Option<Buffer>,
napi::Error>) so conversion errors surface to JS, use
bridge_ctypes::HandleTableOptions::for_in_process as before, call
external_to_baml_value and map Ok(baml_val) to
Some(Buffer::from(baml_val.encode_to_vec())), but map Err(e) to return
Err(napi::Error::from_reason(format!("failed to convert value: {}", e))) instead
of logging and returning None; keep the #[napi(getter)] attribute and ensure the
function returns Err to let napi-rs throw into JavaScript.
In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts`:
- Around line 110-113: The exit hook is being registered multiple times; change
the registration in index.ts (where flushEvents is bound) to only register once
process-wide by using a shared guard (e.g. a module/global flag named like
exitFlushRegistered or a uniquified symbol on process) before calling
process.once/process.on, and update ctx_manager.ts to check/set the same guard
so both modules share the same single registration; ensure the guard prevents
repeat registration even if the module is reloaded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b1ad5aab-3075-465c-a280-e09dcd6917fd
⛔ Files ignored due to path filters (12)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/crates/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_python/uv.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
baml_language/Cargo.tomlbaml_language/crates/bridge_nodejs/Cargo.tomlbaml_language/crates/bridge_nodejs/build.rsbaml_language/crates/bridge_nodejs/ctx_manager.d.tsbaml_language/crates/bridge_nodejs/ctx_manager.jsbaml_language/crates/bridge_nodejs/errors.d.tsbaml_language/crates/bridge_nodejs/errors.jsbaml_language/crates/bridge_nodejs/index.d.tsbaml_language/crates/bridge_nodejs/index.jsbaml_language/crates/bridge_nodejs/native.d.tsbaml_language/crates/bridge_nodejs/native.jsbaml_language/crates/bridge_nodejs/package.jsonbaml_language/crates/bridge_nodejs/protobaml_language/crates/bridge_nodejs/proto.d.tsbaml_language/crates/bridge_nodejs/proto.jsbaml_language/crates/bridge_nodejs/src/abort_controller.rsbaml_language/crates/bridge_nodejs/src/errors.rsbaml_language/crates/bridge_nodejs/src/handle.rsbaml_language/crates/bridge_nodejs/src/lib.rsbaml_language/crates/bridge_nodejs/src/runtime.rsbaml_language/crates/bridge_nodejs/src/types/collector.rsbaml_language/crates/bridge_nodejs/src/types/host_span_manager.rsbaml_language/crates/bridge_nodejs/src/types/mod.rsbaml_language/crates/bridge_nodejs/tests/call_function_sync.test.tsbaml_language/crates/bridge_nodejs/tests/run_jest.rsbaml_language/crates/bridge_nodejs/tests/test_collector.test.tsbaml_language/crates/bridge_nodejs/tests/test_engine.test.tsbaml_language/crates/bridge_nodejs/tests/test_tracing.test.tsbaml_language/crates/bridge_nodejs/tsconfig.jsonbaml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/crates/bridge_nodejs/typescript_src/errors.tsbaml_language/crates/bridge_nodejs/typescript_src/generated-header.txtbaml_language/crates/bridge_nodejs/typescript_src/index.tsbaml_language/crates/bridge_nodejs/typescript_src/native.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/crates/bridge_nodejs/typescript_src/stamp-generated.jsmise.tomltools/build
✅ Files skipped from review due to trivial changes (17)
- baml_language/crates/bridge_nodejs/proto
- baml_language/crates/bridge_nodejs/typescript_src/generated-header.txt
- baml_language/crates/bridge_nodejs/typescript_src/native.d.ts
- baml_language/Cargo.toml
- mise.toml
- tools/build
- baml_language/crates/bridge_nodejs/proto.d.ts
- baml_language/crates/bridge_nodejs/tests/test_collector.test.ts
- baml_language/crates/bridge_nodejs/tests/test_engine.test.ts
- baml_language/crates/bridge_nodejs/typescript_src/errors.ts
- baml_language/crates/bridge_nodejs/errors.d.ts
- baml_language/crates/bridge_nodejs/errors.js
- baml_language/crates/bridge_nodejs/ctx_manager.d.ts
- baml_language/crates/bridge_nodejs/package.json
- baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts
- baml_language/crates/bridge_nodejs/index.d.ts
- baml_language/crates/bridge_nodejs/native.d.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- baml_language/crates/bridge_nodejs/build.rs
- baml_language/crates/bridge_nodejs/tsconfig.json
- baml_language/crates/bridge_nodejs/tests/run_jest.rs
- baml_language/crates/bridge_nodejs/src/types/mod.rs
- baml_language/crates/bridge_nodejs/tests/call_function_sync.test.ts
- baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/crates/bridge_nodejs/Cargo.toml
- baml_language/crates/bridge_nodejs/src/abort_controller.rs
- baml_language/crates/bridge_nodejs/index.js
sxlijin
force-pushed
the
push-rtupkwyokymy
branch
from
34edda7 to
351444d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
baml_language/crates/bridge_nodejs/native.js (1)
74-77:⚠️ Potential issue | 🔴 CriticalReturn the env-override binding immediately.
This branch loads the override module into
nativeBindingbut then falls through without returning it, so line 369 overwrites the successful load withundefinedand the override never takes effect.Minimal fix
if (process.env.NAPI_RS_NATIVE_LIBRARY_PATH) { try { - nativeBinding = require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH); + return require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) } catch (err) { loadErrors.push(err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/native.js` around lines 74 - 77, The override branch that requires process.env.NAPI_RS_NATIVE_LIBRARY_PATH assigns the module to nativeBinding but doesn't return it, so the later fallback overwrites it; update the try block that calls require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) to immediately return the loaded binding (or assign and module.exports/exports it) on success (i.e., after nativeBinding = require(...)) so the env-override binding is used instead of falling through to the fallback resolution in this file.baml_language/crates/bridge_nodejs/src/types/collector.rs (1)
106-118:⚠️ Potential issue | 🟠 MajorPropagate
resultconversion failures instead of returningnull.
Nonealready means “no result”. Mappingexternal_to_baml_value(...)failures toNonemakes bridge corruption indistinguishable from an actually absent result and silently drops output on the floor. ReturnResult<Option<Buffer>>so JS sees an exception here.Suggested fix
- pub fn result(&self) -> Option<Buffer> { + pub fn result(&self) -> Result<Option<Buffer>> { let handle_options = bridge_ctypes::HandleTableOptions::for_in_process(); - self.inner.result.as_ref().and_then(|val| { - match external_to_baml_value(val, &handle_options) { - Ok(baml_val) => Some(Buffer::from(baml_val.encode_to_vec())), - Err(e) => { - log::warn!("FunctionLog.result: failed to convert value: {e}"); - None - } - } - }) + self.inner + .result + .as_ref() + .map(|val| { + let baml_val = external_to_baml_value(val, &handle_options) + .map_err(|e| napi::Error::from_reason(format!( + "FunctionLog.result: failed to convert value: {e}" + )))?; + Ok(Buffer::from(baml_val.encode_to_vec())) + }) + .transpose() }Does napi-rs 3.x support `#[napi(getter)]` methods returning `Result<Option<Buffer>>`, and does returning `Err(...)` from a getter throw a JavaScript exception?baml_language/crates/bridge_nodejs/src/errors.rs (1)
56-59:⚠️ Potential issue | 🟠 MajorKeep the
BamlClientErrormarker on internal failures.This is the only
BridgeErrorarm that drops the typed marker, so JS-side wrapping will classify internal bridge failures as a genericBamlErrorinstead ofBamlClientError.Minimal fix
BridgeError::Internal(msg) => napi::Error::new( Status::GenericFailure, - format!("BamlError: Internal error: {msg}"), + format!("BamlError: BamlClientError: Internal error: {msg}"), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/src/errors.rs` around lines 56 - 59, The BridgeError::Internal arm in errors.rs currently produces a napi::Error message that omits the BamlClientError marker; update the BridgeError::Internal error construction so the napi::Error message includes the "BamlClientError" marker (e.g. prefix the formatted message with "BamlClientError:") while keeping the same Status (Status::GenericFailure) so JS-side wrapping will recognize internal bridge failures as BamlClientError.baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts (1)
11-19:⚠️ Potential issue | 🟡 MinorRegister the exit hook only once.
Registering
process.on('exit', ...)inside each constructor call stacks listeners and can trigger duplicateflushEvents()executions.♻️ Suggested fix
+let exitHookInstalled = false; + export class CtxManager { @@ constructor(rt: unknown) { this.rt = rt; this.ctx = new AsyncLocalStorage<HostSpanManager>(); this.ctx.enterWith(new HostSpanManager()); - process.on('exit', () => { - try { flushEvents(); } catch {} - }); + if (!exitHookInstalled) { + exitHookInstalled = true; + process.once('exit', () => { + try { flushEvents(); } catch {} + }); + } }#!/bin/bash set -euo pipefail rg -n "process\.(on|once)\('exit'" baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts baml_language/crates/bridge_nodejs/typescript_src/index.tsExpected after fix: no per-instance
process.on('exit')inCtxManager, and singleonce-style registration pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts` around lines 11 - 19, The constructor of CtxManager currently registers process.on('exit', ...) per instance which stacks listeners; change this to a single global once-style registration so flushEvents() is invoked only once on exit. Remove the process.on('exit', ...) call from the CtxManager constructor and instead register process.once('exit', () => { try { flushEvents(); } catch {} }); in a module-initialization scope (e.g., top-level of this module or in index.ts), ensuring the symbol flushEvents() is imported/visible and preserving the existing try/catch behavior; keep the rest of the constructor (this.rt, this.ctx, this.ctx.enterWith(new HostSpanManager())) unchanged.baml_language/crates/bridge_nodejs/typescript_src/proto.ts (2)
13-45:⚠️ Potential issue | 🟠 MajorPreserve
BamlHandleidentity in both decode and encode paths.
handleValueis decoded into a plain object and then re-encoded via the generic object branch asmapValue, which breaks handle round-tripping and drops handle lifecycle semantics.🔧 Suggested fix
import { baml } from './proto/baml_cffi'; +import { BamlHandle } from './native'; @@ +function isHandleLike(value: unknown): value is { key: number; handleType: number } { + return value instanceof BamlHandle + || (!!value + && typeof value === 'object' + && Number.isInteger((value as { key?: unknown }).key) + && Number.isInteger((value as { handleType?: unknown }).handleType)); +} + function setInboundValue(iv: baml.cffi.v1.IInboundValue, value: unknown): void { @@ + if (isHandleLike(value)) { + iv.handleValue = { key: value.key, handleType: value.handleType }; + return; + } @@ if (holder.handleValue) { - return { key: Number(holder.handleValue.key), handleType: holder.handleValue.handleType }; + return new BamlHandle( + Number(holder.handleValue.key), + holder.handleValue.handleType + ); }Also applies to: 107-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/proto.ts` around lines 13 - 45, The encode path in setInboundValue currently treats handle objects like plain JS objects and encodes them as mapValue, breaking BamlHandle identity and lifecycle; update setInboundValue to detect the runtime BamlHandle representation (the same shape used in the decode path) and write it to iv.handleValue instead of falling through to the object→mapValue branch; ensure you set iv.handleValue to the original handle identifier/structure (not a reconstructed map) and keep the existing mapValue logic only for ordinary plain objects (affecting setInboundValue and the analogous logic around the handleValue handling at lines referenced 107-110).
20-24:⚠️ Potential issue | 🟠 MajorGuard int64 conversions before coercing with
Number(...).Direct
Number(...)on protobuf int fields can silently lose precision outside JS safe integer range.🛡️ Suggested fix
+function toSafeInt(value: unknown, field: string): number { + const n = typeof value === 'number' ? value : Number(value); + if (!Number.isSafeInteger(n)) { + throw new RangeError(`${field} exceeds JS safe integer range`); + } + return n; +} @@ if (Number.isInteger(value)) { - iv.intValue = value; + iv.intValue = toSafeInt(value, 'intValue'); @@ - if (holder.intValue != null) return Number(holder.intValue); + if (holder.intValue != null) return toSafeInt(holder.intValue, 'intValue'); @@ - if (holder.literalValue.intLiteral != null) return Number(holder.literalValue.intLiteral.value); + if (holder.literalValue.intLiteral != null) return toSafeInt(holder.literalValue.intLiteral.value, 'literalValue.intLiteral.value'); @@ - return { key: Number(holder.handleValue.key), handleType: holder.handleValue.handleType }; + return { key: toSafeInt(holder.handleValue.key, 'handleValue.key'), handleType: holder.handleValue.handleType };#!/bin/bash set -euo pipefail fd -i 'baml_cffi.d.ts' baml_language/crates/bridge_nodejs -x sh -c ' echo "== {} =="; rg -n "intValue|handleValue|Long|string" "{}"; 'This verifies whether generated typings allow non-
numberint64 representations that require safe handling.Also applies to: 68-69, 83-84, 109-109
🧹 Nitpick comments (2)
baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts (1)
48-65: This doesn't actually test async-local isolation.The test only clones
HostSpanManagers and mutates them directly. It would still pass ifCtxManagerstopped usingAsyncLocalStorageentirely. Please drive the assertion throughctxMgr.get()inside a traced async function after anawaitso regressions inctx.run(...)are caught.Example of a stronger assertion
- test('provides isolated contexts', async () => { + test('provides isolated async-local contexts', async () => { const ctxMgr = new CtxManager(rt); - const results: number[] = []; - - const task = async (depth: number) => { - const mgr = ctxMgr.cloneContext(); - for (let i = 0; i < depth; i++) { - mgr.enter(`level${i}`, {}); - } - results.push(mgr.contextDepth()); - for (let i = 0; i < depth; i++) { - mgr.exitOk(); - } - }; - - await Promise.all([task(1), task(2), task(3)]); - expect(results.sort()).toEqual([1, 2, 3]); + const wrapped = ctxMgr.traceFnAsync('depth', async (depth: number) => { + const mgr = ctxMgr.get(); + for (let i = 0; i < depth; i++) { + mgr.enter(`level${i}`, {}); + } + await Promise.resolve(); + const currentDepth = ctxMgr.get().contextDepth(); + for (let i = 0; i < depth; i++) { + mgr.exitOk(); + } + return currentDepth; + }); + + const results = await Promise.all([wrapped(1), wrapped(2), wrapped(3)]); + expect(results.sort((a, b) => a - b)).toEqual([1, 2, 3]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts` around lines 48 - 65, The test currently only mutates cloned HostSpanManager instances directly, which won't detect loss of AsyncLocalStorage; change it to drive assertions through CtxManager's async-local API: for each task use ctxMgr.run(...) (or the method that creates a traced async context), inside the async function await at least one tick (e.g., await Promise.resolve()) and then call ctxMgr.get() to enter levels and read contextDepth(), then exit; collect those depths from inside the async functions and assert the sorted results equal [1,2,3]. Ensure you reference CtxManager, ctxMgr.run (or ctx.run), ctxMgr.get(), and contextDepth()/enter()/exitOk() when updating the test.baml_language/crates/bridge_nodejs/src/runtime.rs (1)
129-144: Add unit tests fordecode_argserror/valid paths.This function handles critical argument decoding with two failure modes (protobuf decode and kwargs conversion). Unit tests for both error cases and the valid path would help prevent regressions. Per coding guidelines, Rust unit tests are preferred; also run
cargo test --libafter changes to this crate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/src/runtime.rs` around lines 129 - 144, Add Rust unit tests for the function decode_args: write three tests in the crate's tests module that (1) supply invalid protobuf bytes to trigger CallFunctionArgs::decode and assert decode_args returns an invalid_argument_error, (2) supply a protobuf-encoded CallFunctionArgs that causes kwargs_to_bex_values to fail (e.g., malformed kwargs or a mocked/controlled HANDLE_TABLE state) and assert the appropriate invalid_argument_error, and (3) supply a valid CallFunctionArgs payload and assert decode_args returns the expected bex_project::BexArgs; use the decode_args function name, bridge_ctypes::baml::cffi::CallFunctionArgs and kwargs_to_bex_values/HANDLE_TABLE references to locate code, place tests in the crate's unit test module, and run cargo test --lib to verify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@baml_language/crates/bridge_nodejs/ctx_manager.js`:
- Around line 16-25: The exit hook is currently added inside
CtxManager.constructor causing a new process.on('exit', ...) listener for each
new CtxManager; change this so the exit handler (calling native_1.flushEvents())
is registered once at module scope using a module-level flag (e.g., a boolean
like exitHandlerRegistered) or an immediately-run initialization block, and
remove the process.on('exit', ...) call from the CtxManager constructor; keep
CtxManager, constructor, process.on('exit'), and native_1.flushEvents referenced
so you can locate and move the handler to module initialization.
---
Duplicate comments:
In `@baml_language/crates/bridge_nodejs/native.js`:
- Around line 74-77: The override branch that requires
process.env.NAPI_RS_NATIVE_LIBRARY_PATH assigns the module to nativeBinding but
doesn't return it, so the later fallback overwrites it; update the try block
that calls require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) to immediately
return the loaded binding (or assign and module.exports/exports it) on success
(i.e., after nativeBinding = require(...)) so the env-override binding is used
instead of falling through to the fallback resolution in this file.
In `@baml_language/crates/bridge_nodejs/src/errors.rs`:
- Around line 56-59: The BridgeError::Internal arm in errors.rs currently
produces a napi::Error message that omits the BamlClientError marker; update the
BridgeError::Internal error construction so the napi::Error message includes the
"BamlClientError" marker (e.g. prefix the formatted message with
"BamlClientError:") while keeping the same Status (Status::GenericFailure) so
JS-side wrapping will recognize internal bridge failures as BamlClientError.
In `@baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts`:
- Around line 11-19: The constructor of CtxManager currently registers
process.on('exit', ...) per instance which stacks listeners; change this to a
single global once-style registration so flushEvents() is invoked only once on
exit. Remove the process.on('exit', ...) call from the CtxManager constructor
and instead register process.once('exit', () => { try { flushEvents(); } catch
{} }); in a module-initialization scope (e.g., top-level of this module or in
index.ts), ensuring the symbol flushEvents() is imported/visible and preserving
the existing try/catch behavior; keep the rest of the constructor (this.rt,
this.ctx, this.ctx.enterWith(new HostSpanManager())) unchanged.
In `@baml_language/crates/bridge_nodejs/typescript_src/proto.ts`:
- Around line 13-45: The encode path in setInboundValue currently treats handle
objects like plain JS objects and encodes them as mapValue, breaking BamlHandle
identity and lifecycle; update setInboundValue to detect the runtime BamlHandle
representation (the same shape used in the decode path) and write it to
iv.handleValue instead of falling through to the object→mapValue branch; ensure
you set iv.handleValue to the original handle identifier/structure (not a
reconstructed map) and keep the existing mapValue logic only for ordinary plain
objects (affecting setInboundValue and the analogous logic around the
handleValue handling at lines referenced 107-110).
---
Nitpick comments:
In `@baml_language/crates/bridge_nodejs/src/runtime.rs`:
- Around line 129-144: Add Rust unit tests for the function decode_args: write
three tests in the crate's tests module that (1) supply invalid protobuf bytes
to trigger CallFunctionArgs::decode and assert decode_args returns an
invalid_argument_error, (2) supply a protobuf-encoded CallFunctionArgs that
causes kwargs_to_bex_values to fail (e.g., malformed kwargs or a
mocked/controlled HANDLE_TABLE state) and assert the appropriate
invalid_argument_error, and (3) supply a valid CallFunctionArgs payload and
assert decode_args returns the expected bex_project::BexArgs; use the
decode_args function name, bridge_ctypes::baml::cffi::CallFunctionArgs and
kwargs_to_bex_values/HANDLE_TABLE references to locate code, place tests in the
crate's unit test module, and run cargo test --lib to verify.
In `@baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts`:
- Around line 48-65: The test currently only mutates cloned HostSpanManager
instances directly, which won't detect loss of AsyncLocalStorage; change it to
drive assertions through CtxManager's async-local API: for each task use
ctxMgr.run(...) (or the method that creates a traced async context), inside the
async function await at least one tick (e.g., await Promise.resolve()) and then
call ctxMgr.get() to enter levels and read contextDepth(), then exit; collect
those depths from inside the async functions and assert the sorted results equal
[1,2,3]. Ensure you reference CtxManager, ctxMgr.run (or ctx.run), ctxMgr.get(),
and contextDepth()/enter()/exitOk() when updating the test.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d97b8800-0f15-4f87-b8dd-fc7a03b158c1
⛔ Files ignored due to path filters (12)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/crates/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_python/uv.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
baml_language/Cargo.tomlbaml_language/crates/bridge_nodejs/Cargo.tomlbaml_language/crates/bridge_nodejs/build.rsbaml_language/crates/bridge_nodejs/ctx_manager.d.tsbaml_language/crates/bridge_nodejs/ctx_manager.jsbaml_language/crates/bridge_nodejs/errors.d.tsbaml_language/crates/bridge_nodejs/errors.jsbaml_language/crates/bridge_nodejs/index.d.tsbaml_language/crates/bridge_nodejs/index.jsbaml_language/crates/bridge_nodejs/native.d.tsbaml_language/crates/bridge_nodejs/native.jsbaml_language/crates/bridge_nodejs/package.jsonbaml_language/crates/bridge_nodejs/protobaml_language/crates/bridge_nodejs/proto.d.tsbaml_language/crates/bridge_nodejs/proto.jsbaml_language/crates/bridge_nodejs/src/abort_controller.rsbaml_language/crates/bridge_nodejs/src/errors.rsbaml_language/crates/bridge_nodejs/src/handle.rsbaml_language/crates/bridge_nodejs/src/lib.rsbaml_language/crates/bridge_nodejs/src/runtime.rsbaml_language/crates/bridge_nodejs/src/types/collector.rsbaml_language/crates/bridge_nodejs/src/types/host_span_manager.rsbaml_language/crates/bridge_nodejs/src/types/mod.rsbaml_language/crates/bridge_nodejs/tests/call_function.test.tsbaml_language/crates/bridge_nodejs/tests/run_jest.rsbaml_language/crates/bridge_nodejs/tests/test_collector.test.tsbaml_language/crates/bridge_nodejs/tests/test_engine.test.tsbaml_language/crates/bridge_nodejs/tests/test_tracing.test.tsbaml_language/crates/bridge_nodejs/tsconfig.jsonbaml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/crates/bridge_nodejs/typescript_src/errors.tsbaml_language/crates/bridge_nodejs/typescript_src/generated-header.txtbaml_language/crates/bridge_nodejs/typescript_src/index.tsbaml_language/crates/bridge_nodejs/typescript_src/native.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/crates/bridge_nodejs/typescript_src/stamp-generated.jsmise.tomltools/build
✅ Files skipped from review due to trivial changes (17)
- baml_language/crates/bridge_nodejs/proto
- baml_language/crates/bridge_nodejs/build.rs
- baml_language/crates/bridge_nodejs/typescript_src/native.d.ts
- mise.toml
- baml_language/crates/bridge_nodejs/tests/test_collector.test.ts
- tools/build
- baml_language/crates/bridge_nodejs/src/types/mod.rs
- baml_language/crates/bridge_nodejs/tsconfig.json
- baml_language/Cargo.toml
- baml_language/crates/bridge_nodejs/proto.d.ts
- baml_language/crates/bridge_nodejs/typescript_src/generated-header.txt
- baml_language/crates/bridge_nodejs/src/abort_controller.rs
- baml_language/crates/bridge_nodejs/Cargo.toml
- baml_language/crates/bridge_nodejs/errors.js
- baml_language/crates/bridge_nodejs/ctx_manager.d.ts
- baml_language/crates/bridge_nodejs/package.json
- baml_language/crates/bridge_nodejs/index.d.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- baml_language/crates/bridge_nodejs/tests/run_jest.rs
- baml_language/crates/bridge_nodejs/typescript_src/stamp-generated.js
- baml_language/crates/bridge_nodejs/tests/test_engine.test.ts
- baml_language/crates/bridge_nodejs/src/lib.rs
- baml_language/crates/bridge_nodejs/errors.d.ts
- baml_language/crates/bridge_nodejs/typescript_src/errors.ts
- baml_language/crates/bridge_nodejs/src/handle.rs
- baml_language/crates/bridge_nodejs/index.js
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
baml_language/crates/bridge_nodejs/src/types/host_span_manager.rs
Dismissed
Show dismissed
Hide dismissed
sxlijin
force-pushed
the
push-rtupkwyokymy
branch
from
351444d to
dc98b4a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@baml_language/crates/bridge_nodejs/proto.js`:
- Around line 133-137: The decoder for BamlOutboundValue in proto.js currently
only returns a BamlHandle for holder.handleValue and falls back to null for
other variants, but it never decodes the uint8array_value variant so any bytes
returned become null; update the decoding branch that checks holder.handleValue
to also check for holder.uint8arrayValue (or the equivalent property produced by
the protobuf wrapper) and return the raw Uint8Array (or Buffer/ArrayBuffer as
your runtime expects) when that field is present instead of null, and make the
corresponding change in typescript_src/proto.ts (the source that generates
proto.js) so both the TS decoder and the generated JS handle uint8array_value
consistently; reference the BamlOutboundValue variant and FunctionLog.result
payload handling when updating the logic around native_1.BamlHandle and the
return value.
In `@baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts`:
- Around line 45-47: The cloneContext() method currently returns mgr.deepClone()
but does not install it as the active context so this.ctx (used by get(),
upsertTags(), and nested tracing) keeps pointing to the original
HostSpanManager; change cloneContext() to create the deep clone, set this.ctx to
that clone before returning (and ensure callers that expect to temporarily swap
contexts—e.g., traceFn* wrappers—restore the previous parent when done) OR if
you prefer a non-installing helper, rename cloneContext() to deepCloneContext()
and update all callers to either call a new installCloneContext() that sets
this.ctx = clone and returns it or to explicitly set this.ctx =
deepCloneContext() where appropriate; reference the HostSpanManager type and the
traceFn* code paths so the clone is installed and restored consistently.
In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts`:
- Around line 21-27: callFunctionSync and callFunction are currently letting raw
N-API Errors escape instead of converting them to the project’s typed errors;
catch any thrown error inside both helpers (callFunctionSync and callFunction)
and rethrow via wrapNativeError(error) so callers receive
BamlInvalidArgumentError, BamlCancelledError, or BamlClientError as intended;
locate the try/catch or promise rejection paths inside the functions and replace
direct throw/propagate with throw wrapNativeError(err) (or for async,
reject/throw the wrapped error) to ensure all native exceptions are normalized.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c442611-6601-4a6f-889e-dea912b3b6c1
⛔ Files ignored due to path filters (12)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/crates/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_python/uv.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
baml_language/Cargo.tomlbaml_language/crates/bridge_nodejs/Cargo.tomlbaml_language/crates/bridge_nodejs/build.rsbaml_language/crates/bridge_nodejs/ctx_manager.d.tsbaml_language/crates/bridge_nodejs/ctx_manager.jsbaml_language/crates/bridge_nodejs/errors.d.tsbaml_language/crates/bridge_nodejs/errors.jsbaml_language/crates/bridge_nodejs/index.d.tsbaml_language/crates/bridge_nodejs/index.jsbaml_language/crates/bridge_nodejs/native.d.tsbaml_language/crates/bridge_nodejs/native.jsbaml_language/crates/bridge_nodejs/package.jsonbaml_language/crates/bridge_nodejs/protobaml_language/crates/bridge_nodejs/proto.d.tsbaml_language/crates/bridge_nodejs/proto.jsbaml_language/crates/bridge_nodejs/src/abort_controller.rsbaml_language/crates/bridge_nodejs/src/errors.rsbaml_language/crates/bridge_nodejs/src/handle.rsbaml_language/crates/bridge_nodejs/src/lib.rsbaml_language/crates/bridge_nodejs/src/runtime.rsbaml_language/crates/bridge_nodejs/src/types/collector.rsbaml_language/crates/bridge_nodejs/src/types/host_span_manager.rsbaml_language/crates/bridge_nodejs/src/types/mod.rsbaml_language/crates/bridge_nodejs/tests/call_function.test.tsbaml_language/crates/bridge_nodejs/tests/run_jest.rsbaml_language/crates/bridge_nodejs/tests/test_collector.test.tsbaml_language/crates/bridge_nodejs/tests/test_engine.test.tsbaml_language/crates/bridge_nodejs/tests/test_tracing.test.tsbaml_language/crates/bridge_nodejs/tsconfig.jsonbaml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/crates/bridge_nodejs/typescript_src/errors.tsbaml_language/crates/bridge_nodejs/typescript_src/generated-header.txtbaml_language/crates/bridge_nodejs/typescript_src/index.tsbaml_language/crates/bridge_nodejs/typescript_src/native.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/crates/bridge_nodejs/typescript_src/stamp-generated.jsmise.tomltools/build
✅ Files skipped from review due to trivial changes (23)
- baml_language/crates/bridge_nodejs/proto
- baml_language/crates/bridge_nodejs/build.rs
- baml_language/crates/bridge_nodejs/typescript_src/generated-header.txt
- baml_language/crates/bridge_nodejs/tests/test_collector.test.ts
- baml_language/crates/bridge_nodejs/tsconfig.json
- baml_language/crates/bridge_nodejs/typescript_src/stamp-generated.js
- baml_language/Cargo.toml
- baml_language/crates/bridge_nodejs/proto.d.ts
- baml_language/crates/bridge_nodejs/src/abort_controller.rs
- baml_language/crates/bridge_nodejs/src/types/mod.rs
- baml_language/crates/bridge_nodejs/tests/call_function.test.ts
- baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/crates/bridge_nodejs/Cargo.toml
- baml_language/crates/bridge_nodejs/typescript_src/errors.ts
- baml_language/crates/bridge_nodejs/errors.d.ts
- baml_language/crates/bridge_nodejs/src/lib.rs
- baml_language/crates/bridge_nodejs/errors.js
- baml_language/crates/bridge_nodejs/ctx_manager.js
- baml_language/crates/bridge_nodejs/ctx_manager.d.ts
- baml_language/crates/bridge_nodejs/native.d.ts
- baml_language/crates/bridge_nodejs/src/types/host_span_manager.rs
- baml_language/crates/bridge_nodejs/index.d.ts
- baml_language/crates/bridge_nodejs/typescript_src/proto.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/build
- baml_language/crates/bridge_nodejs/src/errors.rs
- baml_language/crates/bridge_nodejs/src/handle.rs
- baml_language/crates/bridge_nodejs/package.json
- baml_language/crates/bridge_nodejs/native.js
sxlijin
force-pushed
the
push-rtupkwyokymy
branch
from
dc98b4a to
fea8e53
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
baml_language/crates/bridge_nodejs/native.js (1)
74-79:⚠️ Potential issue | 🔴 CriticalReturn the env-override binding immediately.
When Line 76 succeeds, this branch still falls through and
requireNative()returnsundefined, so Line 369 overwrites the loaded module. That makesNAPI_RS_NATIVE_LIBRARY_PATHunusable.Minimal fix
if (process.env.NAPI_RS_NATIVE_LIBRARY_PATH) { try { - nativeBinding = require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH); + return require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) } catch (err) { loadErrors.push(err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/native.js` around lines 74 - 79, The env-override branch in requireNative currently sets nativeBinding via require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) but doesn't stop execution, allowing later logic to overwrite it; update the branch in native.js so that after successfully assigning nativeBinding you immediately return nativeBinding (or otherwise exit the function) to prevent the subsequent module discovery logic from running and overwriting the env-provided binding; reference the nativeBinding variable and the requireNative function when making the change.baml_language/crates/bridge_nodejs/typescript_src/index.ts (1)
82-107:⚠️ Potential issue | 🟠 MajorWrap native failures before returning them to callers.
Both helpers currently let raw N-API errors escape. That bypasses the typed JS error mapping, so callers won't reliably receive
BamlInvalidArgumentError,BamlCancelledError, orBamlClientErroreven though the Rust side is already formatting errors forwrapNativeError.Minimal fix
import { encodeCallArgs, decodeCallResult } from './proto'; +import { wrapNativeError } from './errors'; @@ export function callFunctionSync( @@ ): FunctionResult { const argsProto = encodeCallArgs(kwargs); const nativeCollectors = collectors?.map(c => c._native()) ?? null; - const resultBytes = rt.callFunctionSync(functionName, argsProto, ctx ?? null, nativeCollectors, abortController ?? null); + let resultBytes; + try { + resultBytes = rt.callFunctionSync(functionName, argsProto, ctx ?? null, nativeCollectors, abortController ?? null); + } catch (err) { + throw wrapNativeError(err); + } return new FunctionResult(decodeCallResult(resultBytes)); } @@ ): Promise<FunctionResult> { const argsProto = encodeCallArgs(kwargs); const nativeCollectors = collectors?.map(c => c._native()) ?? null; - const resultBytes = await rt.callFunction(functionName, argsProto, ctx ?? null, nativeCollectors, abortController ?? null); + let resultBytes; + try { + resultBytes = await rt.callFunction(functionName, argsProto, ctx ?? null, nativeCollectors, abortController ?? null); + } catch (err) { + throw wrapNativeError(err); + } return new FunctionResult(decodeCallResult(resultBytes)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts` around lines 82 - 107, Both callFunctionSync and callFunction currently allow raw N-API exceptions from rt.callFunctionSync/rt.callFunction to escape; catch errors around the native calls and rethrow them through the existing wrapNativeError helper so callers receive the mapped Baml* JS errors. Specifically, in callFunctionSync wrap the call to rt.callFunctionSync(...) in try/catch and on error throw wrapNativeError(err), and likewise in async callFunction await the rt.callFunction(...) inside try/catch and throw wrapNativeError(err); keep using encodeCallArgs and decodeCallResult as before and preserve passing ctx, nativeCollectors, and abortController.baml_language/crates/bridge_nodejs/src/handle.rs (1)
20-27:⚠️ Potential issue | 🟠 MajorMake
BamlHandlenon-constructible from userland JS.Line 20 exposes a public constructor that accepts an arbitrary
key. Once a caller fabricates one of these objects,clone()and GC finalization will trust that forged key and can clone/release unrelatedHANDLE_TABLEentries. The bridge can still materialize handles internally fordecodeValueHolder(), but user code should not be able to instantiate them directly.Also applies to: 39-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/src/handle.rs` around lines 20 - 27, The public napi constructor BamlHandle::new must not be exposed to JS because callers can forge keys; remove the #[napi(constructor)] attribute and make the constructor non-public (e.g., change to pub(crate) fn new_internal(key: u64, handle_type: i32) -> Self or private fn new(...)) so it cannot be instantiated from userland, then update internal call sites such as decodeValueHolder() to use the internal constructor; also remove or convert any other napi-exposed constructors or factory functions in the same file (the duplicate at the 39-55 region) so no public JS-facing way exists to fabricate a BamlHandle.
🧹 Nitpick comments (1)
baml_language/crates/bridge_nodejs/src/types/collector.rs (1)
22-219: Please add Rust unit tests for wrapper behavior (not only Jest coverage).At minimum, cover
Collector::new(None)default naming andFunctionLog::result()error propagation path in Rust unit tests.As per coding guidelines "
**/*.rs: Prefer writing Rust unit tests over integration tests where possible".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/src/types/collector.rs` around lines 22 - 219, Add Rust unit tests in this module under a #[cfg(test)] mod tests: one test constructs the JS wrapper via Collector::new(None) (or Collector::new(Some(..)) as appropriate) and asserts wrapper.name() == "default" to cover default naming; another test builds a bex_events::FunctionLog with inner.result set to a value that will cause external_to_baml_value to fail (or otherwise inject a failing conversion), wraps it with the FunctionLog wrapper (FunctionLog { inner }), calls FunctionLog::result() and asserts it returns an Err and the error message contains "FunctionLog.result: failed to convert value" to verify error propagation. Use the existing constructors/types (Collector::new, FunctionLog wrapper and bex_events::FunctionLog) and run the tests with cargo test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 227-228: The git diff check that gates the workflow currently only
watches 'baml_language/crates/bridge_nodejs/**' and therefore misses upstream
proto inputs; update the git diff command that uses MERGE_BASE (the if git diff
--quiet "${MERGE_BASE}...HEAD" -- ... check) to also include the proto path(s)
read by pnpm build:debug such as 'baml_language/crates/bridge_ctypes/types/**'
(or the broader 'baml_language/crates/bridge_ctypes/**/types/**') so proto-only
changes will trigger the sync job and avoid stale generated artifacts.
- Around line 412-417: The current check uses "git diff --quiet --
baml_language/crates/bridge_nodejs" which only detects changes to tracked files
and will miss untracked/generated files created by "pnpm build:debug"; update
the CI step to also detect untracked files by replacing or augmenting that check
with a command that fails if "git status --porcelain
baml_language/crates/bridge_nodejs" (or "git ls-files --others
--exclude-standard baml_language/crates/bridge_nodejs") returns any output, and
keep the existing diagnostics (the echo and git diff --name-only) so the job
reports which files are out of sync.
- Around line 385-393: Replace the mutable action tags with immutable commit
SHAs: locate the uses of Swatinem/rust-cache@v2 and pnpm/action-setup@v4 in the
workflow and replace the `@v2` and `@v4` tags with the corresponding repository
commit SHAs (e.g., Swatinem/rust-cache@<commit-sha> and
pnpm/action-setup@<commit-sha>), ensuring you fetch the correct SHA from each
action's GitHub repo and update the workflow file accordingly so the actions are
pinned to specific commits.
---
Duplicate comments:
In `@baml_language/crates/bridge_nodejs/native.js`:
- Around line 74-79: The env-override branch in requireNative currently sets
nativeBinding via require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) but doesn't
stop execution, allowing later logic to overwrite it; update the branch in
native.js so that after successfully assigning nativeBinding you immediately
return nativeBinding (or otherwise exit the function) to prevent the subsequent
module discovery logic from running and overwriting the env-provided binding;
reference the nativeBinding variable and the requireNative function when making
the change.
In `@baml_language/crates/bridge_nodejs/src/handle.rs`:
- Around line 20-27: The public napi constructor BamlHandle::new must not be
exposed to JS because callers can forge keys; remove the #[napi(constructor)]
attribute and make the constructor non-public (e.g., change to pub(crate) fn
new_internal(key: u64, handle_type: i32) -> Self or private fn new(...)) so it
cannot be instantiated from userland, then update internal call sites such as
decodeValueHolder() to use the internal constructor; also remove or convert any
other napi-exposed constructors or factory functions in the same file (the
duplicate at the 39-55 region) so no public JS-facing way exists to fabricate a
BamlHandle.
In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts`:
- Around line 82-107: Both callFunctionSync and callFunction currently allow raw
N-API exceptions from rt.callFunctionSync/rt.callFunction to escape; catch
errors around the native calls and rethrow them through the existing
wrapNativeError helper so callers receive the mapped Baml* JS errors.
Specifically, in callFunctionSync wrap the call to rt.callFunctionSync(...) in
try/catch and on error throw wrapNativeError(err), and likewise in async
callFunction await the rt.callFunction(...) inside try/catch and throw
wrapNativeError(err); keep using encodeCallArgs and decodeCallResult as before
and preserve passing ctx, nativeCollectors, and abortController.
---
Nitpick comments:
In `@baml_language/crates/bridge_nodejs/src/types/collector.rs`:
- Around line 22-219: Add Rust unit tests in this module under a #[cfg(test)]
mod tests: one test constructs the JS wrapper via Collector::new(None) (or
Collector::new(Some(..)) as appropriate) and asserts wrapper.name() == "default"
to cover default naming; another test builds a bex_events::FunctionLog with
inner.result set to a value that will cause external_to_baml_value to fail (or
otherwise inject a failing conversion), wraps it with the FunctionLog wrapper
(FunctionLog { inner }), calls FunctionLog::result() and asserts it returns an
Err and the error message contains "FunctionLog.result: failed to convert value"
to verify error propagation. Use the existing constructors/types
(Collector::new, FunctionLog wrapper and bex_events::FunctionLog) and run the
tests with cargo test.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a96f2915-4a7f-45af-b108-ddb1de5d57a2
⛔ Files ignored due to path filters (12)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/crates/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_python/uv.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_nodejs/Cargo.tomlbaml_language/crates/bridge_nodejs/build.rsbaml_language/crates/bridge_nodejs/ctx_manager.d.tsbaml_language/crates/bridge_nodejs/ctx_manager.jsbaml_language/crates/bridge_nodejs/errors.d.tsbaml_language/crates/bridge_nodejs/errors.jsbaml_language/crates/bridge_nodejs/index.d.tsbaml_language/crates/bridge_nodejs/index.jsbaml_language/crates/bridge_nodejs/native.d.tsbaml_language/crates/bridge_nodejs/native.jsbaml_language/crates/bridge_nodejs/package.jsonbaml_language/crates/bridge_nodejs/protobaml_language/crates/bridge_nodejs/proto.d.tsbaml_language/crates/bridge_nodejs/proto.jsbaml_language/crates/bridge_nodejs/src/abort_controller.rsbaml_language/crates/bridge_nodejs/src/errors.rsbaml_language/crates/bridge_nodejs/src/handle.rsbaml_language/crates/bridge_nodejs/src/lib.rsbaml_language/crates/bridge_nodejs/src/runtime.rsbaml_language/crates/bridge_nodejs/src/types/collector.rsbaml_language/crates/bridge_nodejs/src/types/host_span_manager.rsbaml_language/crates/bridge_nodejs/src/types/mod.rsbaml_language/crates/bridge_nodejs/tests/call_function.test.tsbaml_language/crates/bridge_nodejs/tests/run_jest.rsbaml_language/crates/bridge_nodejs/tests/test_collector.test.tsbaml_language/crates/bridge_nodejs/tests/test_engine.test.tsbaml_language/crates/bridge_nodejs/tests/test_tracing.test.tsbaml_language/crates/bridge_nodejs/tsconfig.jsonbaml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/crates/bridge_nodejs/typescript_src/errors.tsbaml_language/crates/bridge_nodejs/typescript_src/generated-header.txtbaml_language/crates/bridge_nodejs/typescript_src/index.tsbaml_language/crates/bridge_nodejs/typescript_src/native.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/crates/bridge_nodejs/typescript_src/stamp-generated.jsmise.tomltools/build
✅ Files skipped from review due to trivial changes (25)
- baml_language/crates/bridge_nodejs/proto
- baml_language/crates/bridge_nodejs/build.rs
- baml_language/crates/bridge_nodejs/typescript_src/generated-header.txt
- mise.toml
- baml_language/crates/bridge_nodejs/tests/test_collector.test.ts
- baml_language/crates/bridge_nodejs/tsconfig.json
- tools/build
- baml_language/crates/bridge_nodejs/src/types/mod.rs
- baml_language/Cargo.toml
- baml_language/crates/bridge_nodejs/native.d.ts
- baml_language/crates/bridge_nodejs/proto.d.ts
- baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/crates/bridge_nodejs/typescript_src/stamp-generated.js
- baml_language/crates/bridge_nodejs/src/abort_controller.rs
- baml_language/crates/bridge_nodejs/src/lib.rs
- baml_language/crates/bridge_nodejs/typescript_src/errors.ts
- baml_language/crates/bridge_nodejs/tests/test_engine.test.ts
- baml_language/crates/bridge_nodejs/errors.js
- baml_language/crates/bridge_nodejs/errors.d.ts
- baml_language/crates/bridge_nodejs/ctx_manager.d.ts
- baml_language/crates/bridge_nodejs/tests/call_function.test.ts
- baml_language/crates/bridge_nodejs/package.json
- baml_language/crates/bridge_nodejs/Cargo.toml
- baml_language/crates/bridge_nodejs/index.d.ts
- baml_language/crates/bridge_nodejs/typescript_src/native.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/bridge_nodejs/tests/run_jest.rs
- baml_language/crates/bridge_nodejs/proto.js
- baml_language/crates/bridge_nodejs/typescript_src/proto.ts
- baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts
sxlijin
force-pushed
the
push-rtupkwyokymy
branch
from
fea8e53 to
acc72a7
Compare
sxlijin
force-pushed
the
push-rtupkwyokymy
branch
from
acc72a7 to
39ee8f2
Compare
sxlijin
force-pushed
the
push-rtupkwyokymy
branch
from
39ee8f2 to
0e292de
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
baml_language/crates/bridge_nodejs/proto.js (1)
82-138:⚠️ Potential issue | 🟠 MajorDecode
uint8arrayValuebefore falling back tonull.Lines 83-138 never inspect the outbound
uint8arrayValuevariant, so any bytes returned from Rust disappear here and surface to JS asnull. Please add the missing branch intypescript_src/proto.tsand regenerate this file.Suggested fix
if (holder.boolValue != null) return holder.boolValue; + if (holder.uint8arrayValue != null) + return holder.uint8arrayValue; if (holder.classValue) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/proto.js` around lines 82 - 138, decodeValueHolder is missing a branch for the uint8arrayValue variant so any bytes from Rust become null; update the decodeValueHolder function to check for holder.uint8arrayValue (similar placement to the existing list/map branches) and return a Uint8Array from it (e.g., return holder.uint8arrayValue or new Uint8Array(holder.uint8arrayValue) depending on runtime shape), then regenerate the TypeScript proto binding (typescript_src/proto.ts) so the change is reflected in the generated JS.
🧹 Nitpick comments (2)
baml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.js (1)
8-12: Consider filtering out directories fromreaddirSyncresults.
readdirSyncreturns both files and directories. While unlikely in this context, if a directory name matched.jsor.d.ts,readFileSyncon line 16 would throw an EISDIR error.🛡️ Defensive fix using `withFileTypes`
const files = [ - ...fs.readdirSync(path.join(__dirname, '..')).filter(f => f.endsWith('.js') || f.endsWith('.d.ts')), + ...fs.readdirSync(path.join(__dirname, '..'), { withFileTypes: true }) + .filter(e => e.isFile() && (e.name.endsWith('.js') || e.name.endsWith('.d.ts'))) + .map(e => e.name), 'typescript_src/proto/baml_cffi.js', 'typescript_src/proto/baml_cffi.d.ts', ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.js` around lines 8 - 12, The readdirSync usage building the files array can include directories, which may cause readFileSync to throw EISDIR; update the logic that constructs files (the const files array) to call fs.readdirSync with { withFileTypes: true } and filter Dirent objects via dirent.isFile(), then map to dirent.name and further filter names that endWith('.js') or '.d.ts' (so the explicit entries like 'typescript_src/proto/baml_cffi.js' remain unchanged); ensure subsequent code that reads each file (the code that uses readFileSync on entries from files) will only receive actual file paths.baml_language/crates/bridge_nodejs/src/handle.rs (1)
24-35: Add a Rust unit test forHandleKeyround-trips.Lines 25-33 are pure Rust bit-packing logic, and in the changed files they are only exercised indirectly through Jest. Please add a small
#[cfg(test)]block here for edge values like0,u32::MAX, andu64::MAX, and runcargo test --liblocally before merging.As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible" and "Always run
cargo test --libif you changed any Rust code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/src/handle.rs` around lines 24 - 35, Add a Rust unit test block that verifies HandleKey round-trip correctness for edge values by creating HandleKey instances, calling to_u64 and from_u64, and asserting equality; specifically test zero, u32::MAX (packed into low/high appropriately), and u64::MAX to ensure no sign/width loss. Place a #[cfg(test)] mod tests in the same file next to the impl and write individual #[test] functions that construct HandleKey, convert with to_u64, reconstruct with HandleKey::from_u64, and assert the original equals the reconstructed; then run cargo test --lib locally to confirm all tests pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@baml_language/crates/bridge_nodejs/ctx_manager.js`:
- Around line 17-20: The CtxManager currently eagerly constructs HostSpanManager
in the constructor which snapshots the native event sink too early; update
typescript_src/ctx_manager.ts so the constructor does not call
this.ctx.enterWith(new HostSpanManager()), leaving the AsyncLocalStorage
uninitialized, and instead have CtxManager.get() and CtxManager.reset() lazily
create and enterWith a new HostSpanManager once the runtime/sink is available
(so HostSpanManager is constructed after get_event_sink() can return the real
sink); ensure cloneContext() still clones the current store but will pick up the
lazily-created HostSpanManager, then regenerate this
bridge_nodejs/ctx_manager.js from the modified TypeScript output.
In `@baml_language/crates/bridge_nodejs/index.js`:
- Around line 58-63: The getter result conflates "no serialized result" and an
explicit null payload; change the behavior so when this._inner.result is
null/undefined the getter returns undefined (or another sentinel) while
preserving proto_1.decodeCallResult(bytes) return (which may legitimately be
null) for non-null bytes; modify the result getter in index.js to check
this._inner.result and return undefined when it's absent, otherwise return
proto_1.decodeCallResult(bytes), and apply the same change to the corresponding
TypeScript source and declaration files so the generated output and types
consistently distinguish "no result" from an explicit null.
In `@baml_language/crates/bridge_nodejs/src/errors.rs`:
- Around line 12-15: BridgeError::NotInitialized is currently converted using
napi::Error::new with Status::InvalidArg and a message telling JS to call
create_baml_runtime (which the bridge doesn’t expose); change the conversion so
it uses a node/runtime-state error status (e.g., Status::GenericFailure) and
update the error message to direct callers to the bridge's actual
initializer/export (instead of "create_baml_runtime"), keeping the error
construction in the same place where napi::Error::new is called for
BridgeError::NotInitialized.
In `@baml_language/crates/bridge_nodejs/src/runtime.rs`:
- Around line 75-77: The serialization failure from external_to_baml_value after
the BAML function ran is being misclassified as an invalid_argument_error;
change those error mappings to the internal/runtime error path instead (replace
invalid_argument_error(...) with the appropriate internal/runtime error helper
used in this crate), for the occurrences around external_to_baml_value in
runtime.rs (both the block at the baml call result and the similar occurrence at
lines ~121-123) so serialization/bridge bugs are reported as internal runtime
errors rather than caller input errors.
In `@baml_language/crates/bridge_nodejs/typescript_src/proto.ts`:
- Around line 114-117: The code currently returns null when holder.handleValue
is missing, which hides unknown/malformed outbound variants; change the fallback
to throw an explicit error instead of returning null so bridge drift surfaces
immediately—locate the branch checking holder.handleValue in proto.ts (the code
that returns new BamlHandle(holder.handleValue.key,
holder.handleValue.handleType ?? 0)) and replace the final return null with a
thrown Error (include contextual text mentioning the missing/unsupported handle
variant and any identifying data from holder) so callers see a clear failure
rather than an ambiguous null.
- Around line 94-101: The current loop that builds obj from
holder.mapValue.entries is vulnerable to prototype pollution because it uses a
plain object and assigns obj[entry.key] directly; change the creation of obj to
use a null-prototype object (e.g., Object.create(null)) and/or explicitly ignore
dangerous keys like "__proto__", "constructor", and "prototype" before calling
decodeValueHolder(entry.value) so untrusted mapValue keys cannot mutate the
object's prototype; update the code that references holder.mapValue,
mapValue.entries, obj, and decodeValueHolder accordingly.
- Around line 40-50: The current branch in setInboundValue that assigns
iv.mapValue from any typeof value === 'object' will silently encode non-plain
objects (Date, Map, Set, class instances) as empty or partial maps; update
setInboundValue to only treat plain objects as protobuf maps by checking the
object's prototype/constructor (e.g., Object.getPrototypeOf(value) ===
Object.prototype or value.constructor === Object) before building
baml.cffi.v1.IInboundMapEntry entries and assigning iv.mapValue; for any
non-plain object (Date, Map, Set, custom class instances) throw a clear error
(or return a specific failure) so callers know the value is unsupported rather
than silently serializing it. Ensure references to iv.mapValue,
baml.cffi.v1.IInboundMapEntry, and setInboundValue are used when locating where
to add the type check and error handling.
---
Duplicate comments:
In `@baml_language/crates/bridge_nodejs/proto.js`:
- Around line 82-138: decodeValueHolder is missing a branch for the
uint8arrayValue variant so any bytes from Rust become null; update the
decodeValueHolder function to check for holder.uint8arrayValue (similar
placement to the existing list/map branches) and return a Uint8Array from it
(e.g., return holder.uint8arrayValue or new Uint8Array(holder.uint8arrayValue)
depending on runtime shape), then regenerate the TypeScript proto binding
(typescript_src/proto.ts) so the change is reflected in the generated JS.
---
Nitpick comments:
In `@baml_language/crates/bridge_nodejs/src/handle.rs`:
- Around line 24-35: Add a Rust unit test block that verifies HandleKey
round-trip correctness for edge values by creating HandleKey instances, calling
to_u64 and from_u64, and asserting equality; specifically test zero, u32::MAX
(packed into low/high appropriately), and u64::MAX to ensure no sign/width loss.
Place a #[cfg(test)] mod tests in the same file next to the impl and write
individual #[test] functions that construct HandleKey, convert with to_u64,
reconstruct with HandleKey::from_u64, and assert the original equals the
reconstructed; then run cargo test --lib locally to confirm all tests pass.
In `@baml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.js`:
- Around line 8-12: The readdirSync usage building the files array can include
directories, which may cause readFileSync to throw EISDIR; update the logic that
constructs files (the const files array) to call fs.readdirSync with {
withFileTypes: true } and filter Dirent objects via dirent.isFile(), then map to
dirent.name and further filter names that endWith('.js') or '.d.ts' (so the
explicit entries like 'typescript_src/proto/baml_cffi.js' remain unchanged);
ensure subsequent code that reads each file (the code that uses readFileSync on
entries from files) will only receive actual file paths.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b688c8bf-e821-4973-8c26-c20ab864dfac
⛔ Files ignored due to path filters (12)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/crates/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_python/uv.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_nodejs/Cargo.tomlbaml_language/crates/bridge_nodejs/build.rsbaml_language/crates/bridge_nodejs/ctx_manager.d.tsbaml_language/crates/bridge_nodejs/ctx_manager.jsbaml_language/crates/bridge_nodejs/errors.d.tsbaml_language/crates/bridge_nodejs/errors.jsbaml_language/crates/bridge_nodejs/index.d.tsbaml_language/crates/bridge_nodejs/index.jsbaml_language/crates/bridge_nodejs/native.d.tsbaml_language/crates/bridge_nodejs/native.jsbaml_language/crates/bridge_nodejs/package.jsonbaml_language/crates/bridge_nodejs/protobaml_language/crates/bridge_nodejs/proto.d.tsbaml_language/crates/bridge_nodejs/proto.jsbaml_language/crates/bridge_nodejs/src/abort_controller.rsbaml_language/crates/bridge_nodejs/src/errors.rsbaml_language/crates/bridge_nodejs/src/handle.rsbaml_language/crates/bridge_nodejs/src/lib.rsbaml_language/crates/bridge_nodejs/src/runtime.rsbaml_language/crates/bridge_nodejs/src/types/collector.rsbaml_language/crates/bridge_nodejs/src/types/host_span_manager.rsbaml_language/crates/bridge_nodejs/src/types/mod.rsbaml_language/crates/bridge_nodejs/tests/call_function.test.tsbaml_language/crates/bridge_nodejs/tests/run_jest.rsbaml_language/crates/bridge_nodejs/tests/test_collector.test.tsbaml_language/crates/bridge_nodejs/tests/test_engine.test.tsbaml_language/crates/bridge_nodejs/tests/test_tracing.test.tsbaml_language/crates/bridge_nodejs/tsconfig.jsonbaml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/crates/bridge_nodejs/typescript_src/errors.tsbaml_language/crates/bridge_nodejs/typescript_src/generated-header.txtbaml_language/crates/bridge_nodejs/typescript_src/index.tsbaml_language/crates/bridge_nodejs/typescript_src/native.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.jsmise.tomltools/build
✅ Files skipped from review due to trivial changes (21)
- baml_language/crates/bridge_nodejs/proto
- baml_language/crates/bridge_nodejs/typescript_src/generated-header.txt
- mise.toml
- baml_language/crates/bridge_nodejs/tests/test_collector.test.ts
- baml_language/crates/bridge_nodejs/src/types/mod.rs
- baml_language/crates/bridge_nodejs/tsconfig.json
- baml_language/crates/bridge_nodejs/tests/run_jest.rs
- baml_language/crates/bridge_nodejs/Cargo.toml
- tools/build
- baml_language/crates/bridge_nodejs/proto.d.ts
- baml_language/Cargo.toml
- baml_language/crates/bridge_nodejs/errors.d.ts
- baml_language/crates/bridge_nodejs/errors.js
- baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/crates/bridge_nodejs/tests/call_function.test.ts
- baml_language/crates/bridge_nodejs/ctx_manager.d.ts
- baml_language/crates/bridge_nodejs/native.d.ts
- baml_language/crates/bridge_nodejs/typescript_src/errors.ts
- baml_language/crates/bridge_nodejs/native.js
- baml_language/crates/bridge_nodejs/index.d.ts
- baml_language/crates/bridge_nodejs/typescript_src/native.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/bridge_nodejs/build.rs
- baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts
- baml_language/crates/bridge_nodejs/package.json
- baml_language/crates/bridge_nodejs/typescript_src/index.ts
Comment on lines
+17
to
+20
| constructor(rt) { | ||
| this.rt = rt; | ||
| this.ctx = new node_async_hooks_1.AsyncLocalStorage(); | ||
| this.ctx.enterWith(new native_1.HostSpanManager()); |
Contributor
There was a problem hiding this comment.
Defer the first HostSpanManager allocation.
Line 20 eagerly snapshots the native event sink in HostSpanManager's constructor; the Rust side resolves that sink only once via get_event_sink(). If a CtxManager is created before the runtime is initialized, the initial store keeps sink: None, and cloneContext() preserves that disconnected manager until reset(), so trace sink writes are silently lost. Please make the change in typescript_src/ctx_manager.ts and regenerate this file.
Suggested direction
this.rt = rt;
this.ctx = new AsyncLocalStorage();
- this.ctx.enterWith(new native_1.HostSpanManager());Then let get() / reset() create the first manager once the runtime is ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baml_language/crates/bridge_nodejs/ctx_manager.js` around lines 17 - 20, The
CtxManager currently eagerly constructs HostSpanManager in the constructor which
snapshots the native event sink too early; update typescript_src/ctx_manager.ts
so the constructor does not call this.ctx.enterWith(new HostSpanManager()),
leaving the AsyncLocalStorage uninitialized, and instead have CtxManager.get()
and CtxManager.reset() lazily create and enterWith a new HostSpanManager once
the runtime/sink is available (so HostSpanManager is constructed after
get_event_sink() can return the real sink); ensure cloneContext() still clones
the current store but will pick up the lazily-created HostSpanManager, then
regenerate this bridge_nodejs/ctx_manager.js from the modified TypeScript
output.
Comment on lines
+58
to
+63
| get result() { | ||
| const bytes = this._inner.result; | ||
| if (bytes == null) | ||
| return null; | ||
| return (0, proto_1.decodeCallResult)(bytes); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep “no result” distinct from an explicit null result.
this._inner.result === null means the log has no serialized result, but decodeCallResult(bytes) can also legitimately produce null. Returning null in both cases makes collector logs ambiguous.
Suggested fix
get result() {
const bytes = this._inner.result;
if (bytes == null)
- return null;
+ return undefined;
return (0, proto_1.decodeCallResult)(bytes);
}Because this file is generated, the corresponding TypeScript source and declarations need the same change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| get result() { | |
| const bytes = this._inner.result; | |
| if (bytes == null) | |
| return null; | |
| return (0, proto_1.decodeCallResult)(bytes); | |
| } | |
| get result() { | |
| const bytes = this._inner.result; | |
| if (bytes == null) | |
| return undefined; | |
| return (0, proto_1.decodeCallResult)(bytes); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baml_language/crates/bridge_nodejs/index.js` around lines 58 - 63, The getter
result conflates "no serialized result" and an explicit null payload; change the
behavior so when this._inner.result is null/undefined the getter returns
undefined (or another sentinel) while preserving proto_1.decodeCallResult(bytes)
return (which may legitimately be null) for non-null bytes; modify the result
getter in index.js to check this._inner.result and return undefined when it's
absent, otherwise return proto_1.decodeCallResult(bytes), and apply the same
change to the corresponding TypeScript source and declaration files so the
generated output and types consistently distinguish "no result" from an explicit
null.
Comment on lines
+12
to
+15
| BridgeError::NotInitialized => napi::Error::new( | ||
| Status::InvalidArg, | ||
| "BamlError: BamlInvalidArgumentError: Engine not initialized. Call create_baml_runtime first.", | ||
| ), |
Contributor
There was a problem hiding this comment.
Use a Node-specific client error for BridgeError::NotInitialized.
Line 14 tells JS callers to invoke create_baml_runtime, which this bridge does not expose, and the Status::InvalidArg mapping will classify the failure as BamlInvalidArgumentError instead of a runtime-state error.
Suggested fix
BridgeError::NotInitialized => napi::Error::new(
- Status::InvalidArg,
- "BamlError: BamlInvalidArgumentError: Engine not initialized. Call create_baml_runtime first.",
+ Status::GenericFailure,
+ "BamlError: BamlClientError: Runtime not initialized. Create a BamlRuntime first.",
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| BridgeError::NotInitialized => napi::Error::new( | |
| Status::InvalidArg, | |
| "BamlError: BamlInvalidArgumentError: Engine not initialized. Call create_baml_runtime first.", | |
| ), | |
| BridgeError::NotInitialized => napi::Error::new( | |
| Status::GenericFailure, | |
| "BamlError: BamlClientError: Runtime not initialized. Create a BamlRuntime first.", | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baml_language/crates/bridge_nodejs/src/errors.rs` around lines 12 - 15,
BridgeError::NotInitialized is currently converted using napi::Error::new with
Status::InvalidArg and a message telling JS to call create_baml_runtime (which
the bridge doesn’t expose); change the conversion so it uses a
node/runtime-state error status (e.g., Status::GenericFailure) and update the
error message to direct callers to the bridge's actual initializer/export
(instead of "create_baml_runtime"), keeping the error construction in the same
place where napi::Error::new is called for BridgeError::NotInitialized.
Comment on lines
+75
to
+77
| let handle_options = bridge_ctypes::HandleTableOptions::for_in_process(); | ||
| let baml_value = external_to_baml_value(&result, &handle_options) | ||
| .map_err(|e| invalid_argument_error(format!("Failed to encode result: {e}")))?; |
Contributor
There was a problem hiding this comment.
Use an internal/runtime error path for result-serialization failures.
By the time external_to_baml_value(...) runs, the BAML function has already executed. Mapping that failure to invalid_argument_error(...) misclassifies a bridge serialization bug as bad caller input in both sync and async paths.
Also applies to: 121-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baml_language/crates/bridge_nodejs/src/runtime.rs` around lines 75 - 77, The
serialization failure from external_to_baml_value after the BAML function ran is
being misclassified as an invalid_argument_error; change those error mappings to
the internal/runtime error path instead (replace invalid_argument_error(...)
with the appropriate internal/runtime error helper used in this crate), for the
occurrences around external_to_baml_value in runtime.rs (both the block at the
baml call result and the similar occurrence at lines ~121-123) so
serialization/bridge bugs are reported as internal runtime errors rather than
caller input errors.
Comment on lines
+114
to
+117
| if (holder.handleValue) { | ||
| return new BamlHandle(holder.handleValue.key, holder.handleValue.handleType ?? 0); | ||
| } | ||
| return null; |
Contributor
There was a problem hiding this comment.
Don't collapse unknown outbound variants to null.
The fallback return null makes malformed or newer/unsupported protobuf payloads indistinguishable from an actual BAML null result. Throw here so bridge drift surfaces immediately.
Suggested fix
if (holder.handleValue) {
return new BamlHandle(holder.handleValue.key, holder.handleValue.handleType ?? 0);
}
- return null;
+ throw new TypeError('Cannot decode protobuf value: unsupported outbound variant');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baml_language/crates/bridge_nodejs/typescript_src/proto.ts` around lines 114
- 117, The code currently returns null when holder.handleValue is missing, which
hides unknown/malformed outbound variants; change the fallback to throw an
explicit error instead of returning null so bridge drift surfaces
immediately—locate the branch checking holder.handleValue in proto.ts (the code
that returns new BamlHandle(holder.handleValue.key,
holder.handleValue.handleType ?? 0)) and replace the final return null with a
thrown Error (include contextual text mentioning the missing/unsupported handle
variant and any identifying data from holder) so callers see a clear failure
rather than an ambiguous null.
0e292de to
d072f0d
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
.github/workflows/ci.yaml (1)
386-393:⚠️ Potential issue | 🟠 MajorPin third-party actions to immutable commit SHAs.
Swatinem/rust-cache@v2andpnpm/action-setup@v4are still mutable refs, which keeps CodeQL supply-chain warnings open.🔒 Suggested fix pattern
- - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@<resolved-commit-sha> - - name: "Install pnpm" - uses: pnpm/action-setup@v4 + - name: "Install pnpm" + uses: pnpm/action-setup@<resolved-commit-sha>Use this read-only script to resolve tag refs before pinning:
#!/bin/bash set -euo pipefail for spec in "Swatinem/rust-cache v2" "pnpm/action-setup v4"; do repo="${spec% *}" tag="${spec#* }" echo "== ${repo}@${tag} ==" git ls-remote "https://github.com/${repo}.git" "refs/tags/${tag}" "refs/tags/${tag}^{}" echo done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 386 - 393, Replace the mutable refs by pinning the third-party actions to immutable commit SHAs: run the provided script to resolve the tag refs for "Swatinem/rust-cache v2" and "pnpm/action-setup v4" to their corresponding commit SHAs, then update the two uses lines (currently "Swatinem/rust-cache@v2" and "pnpm/action-setup@v4") to the exact commit SHAs (e.g. "Swatinem/rust-cache@<SHA>" and "pnpm/action-setup@<SHA>") so the workflow references immutable commits and eliminates the supply-chain warnings.baml_language/crates/bridge_nodejs/native.js (1)
73-79:⚠️ Potential issue | 🔴 CriticalReturn the env-override binding from
requireNative().When
NAPI_RS_NATIVE_LIBRARY_PATHsucceeds, this branch assignsnativeBindingbut never returns it. The laternativeBinding = requireNative()assignment then overwrites the loaded module withundefined, so the override path is effectively broken.🐛 Minimal fix
if (process.env.NAPI_RS_NATIVE_LIBRARY_PATH) { try { - nativeBinding = require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH); + return require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) } catch (err) { loadErrors.push(err) } } else if (process.platform === 'android') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/native.js` around lines 73 - 79, The requireNative() function sets nativeBinding when process.env.NAPI_RS_NATIVE_LIBRARY_PATH is present but does not return it, causing the caller's nativeBinding = requireNative() to get undefined; update requireNative() (inside the try block where nativeBinding is assigned from require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH)) to return that binding (or return immediately after assignment) so the env-override path actually returns the loaded module; ensure any existing loadErrors handling remains unchanged.baml_language/crates/bridge_nodejs/src/runtime.rs (1)
75-77:⚠️ Potential issue | 🟡 MinorUse an internal/runtime error path for result-serialization failures.
These mappings run only after
bex.call_function(...)has already succeeded. Ifexternal_to_baml_value(...)fails here, that's a bridge serialization bug, not bad caller input, so surfacinginvalid_argument_error(...)misclassifies the failure for both sync and async callers.Also applies to: 121-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/src/runtime.rs` around lines 75 - 77, The failure to encode the returned value using external_to_baml_value after a successful bex.call_function is a bridge/runtime serialization bug and should not be classified as an invalid_argument_error; change the map_err on external_to_baml_value (both occurrences around the baml_value assignment and the similar block at lines ~121-123) to return an internal/runtime error variant instead (e.g., use a runtime/internal error helper or construct an error via a runtime_error/runtime_internal_error function), include the original error text in the message for debugging, and keep the descriptive prefix like "Failed to encode result" so callers and logs reflect a serialization/runtime failure rather than bad caller input.baml_language/crates/bridge_nodejs/typescript_src/index.ts (1)
55-58:⚠️ Potential issue | 🟡 MinorKeep “no log result” distinct from an explicit
nullpayload.
NativeFunctionLog.resultusesnullto mean “no serialized bytes”, whiledecodeCallResult(bytes)can legitimately decode tonull. Returningnullin both cases makes collector logs ambiguous for callers. Please update the source here and regenerate the JS/declaration outputs to return a distinct sentinel for the absent-bytes case.♻️ Suggested direction
- get result(): unknown { + get result(): unknown | undefined { const bytes = this._inner.result; - if (bytes == null) return null; + if (bytes == null) return undefined; return decodeCallResult(bytes); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts` around lines 55 - 58, The getter NativeFunctionLog.result currently returns null both when there are no serialized bytes and when decodeCallResult(bytes) legitimately returns null; add and export a distinct sentinel (e.g., export const NO_SERIALIZED_RESULT = Symbol("NO_SERIALIZED_RESULT")) and change the getter (get result()) to return that sentinel when this._inner.result is null/undefined, leaving decodeCallResult(bytes) behavior unchanged; after making the change update/regenerate the compiled JS and .d.ts outputs so consumers can import and check the sentinel instead of relying on null.baml_language/crates/bridge_nodejs/proto.js (1)
133-138:⚠️ Potential issue | 🟠 MajorDecode
uint8arrayValuebefore falling back tonull.
external_to_baml_value()can emitUint8arrayValue, but this decoder never handles it, so any bytes result currently disappears asnull. Please add the missing branch intypescript_src/proto.tsand regenerate this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/proto.js` around lines 133 - 138, The decoder currently returns null for bytes because external_to_baml_value()'s Uint8arrayValue case is missing; update the TypeScript source external_to_baml_value (in typescript_src/proto.ts) to handle holder.uint8arrayValue (decode/convert the Uint8Array into the corresponding BamlValue representation), ensure this new branch is placed before the final null fallback (alongside the existing holder.handleValue branch), then rebuild/regenerate proto.js so the compiled file contains the new Uint8arrayValue handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yaml:
- Around line 386-393: Replace the mutable refs by pinning the third-party
actions to immutable commit SHAs: run the provided script to resolve the tag
refs for "Swatinem/rust-cache v2" and "pnpm/action-setup v4" to their
corresponding commit SHAs, then update the two uses lines (currently
"Swatinem/rust-cache@v2" and "pnpm/action-setup@v4") to the exact commit SHAs
(e.g. "Swatinem/rust-cache@<SHA>" and "pnpm/action-setup@<SHA>") so the workflow
references immutable commits and eliminates the supply-chain warnings.
In `@baml_language/crates/bridge_nodejs/native.js`:
- Around line 73-79: The requireNative() function sets nativeBinding when
process.env.NAPI_RS_NATIVE_LIBRARY_PATH is present but does not return it,
causing the caller's nativeBinding = requireNative() to get undefined; update
requireNative() (inside the try block where nativeBinding is assigned from
require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH)) to return that binding (or
return immediately after assignment) so the env-override path actually returns
the loaded module; ensure any existing loadErrors handling remains unchanged.
In `@baml_language/crates/bridge_nodejs/proto.js`:
- Around line 133-138: The decoder currently returns null for bytes because
external_to_baml_value()'s Uint8arrayValue case is missing; update the
TypeScript source external_to_baml_value (in typescript_src/proto.ts) to handle
holder.uint8arrayValue (decode/convert the Uint8Array into the corresponding
BamlValue representation), ensure this new branch is placed before the final
null fallback (alongside the existing holder.handleValue branch), then
rebuild/regenerate proto.js so the compiled file contains the new
Uint8arrayValue handling.
In `@baml_language/crates/bridge_nodejs/src/runtime.rs`:
- Around line 75-77: The failure to encode the returned value using
external_to_baml_value after a successful bex.call_function is a bridge/runtime
serialization bug and should not be classified as an invalid_argument_error;
change the map_err on external_to_baml_value (both occurrences around the
baml_value assignment and the similar block at lines ~121-123) to return an
internal/runtime error variant instead (e.g., use a runtime/internal error
helper or construct an error via a runtime_error/runtime_internal_error
function), include the original error text in the message for debugging, and
keep the descriptive prefix like "Failed to encode result" so callers and logs
reflect a serialization/runtime failure rather than bad caller input.
In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts`:
- Around line 55-58: The getter NativeFunctionLog.result currently returns null
both when there are no serialized bytes and when decodeCallResult(bytes)
legitimately returns null; add and export a distinct sentinel (e.g., export
const NO_SERIALIZED_RESULT = Symbol("NO_SERIALIZED_RESULT")) and change the
getter (get result()) to return that sentinel when this._inner.result is
null/undefined, leaving decodeCallResult(bytes) behavior unchanged; after making
the change update/regenerate the compiled JS and .d.ts outputs so consumers can
import and check the sentinel instead of relying on null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9ce95889-96cc-4ea2-a960-df8d0f6cfbf2
⛔ Files ignored due to path filters (12)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/crates/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_python/uv.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_nodejs/Cargo.tomlbaml_language/crates/bridge_nodejs/build.rsbaml_language/crates/bridge_nodejs/ctx_manager.d.tsbaml_language/crates/bridge_nodejs/ctx_manager.jsbaml_language/crates/bridge_nodejs/errors.d.tsbaml_language/crates/bridge_nodejs/errors.jsbaml_language/crates/bridge_nodejs/index.d.tsbaml_language/crates/bridge_nodejs/index.jsbaml_language/crates/bridge_nodejs/native.d.tsbaml_language/crates/bridge_nodejs/native.jsbaml_language/crates/bridge_nodejs/package.jsonbaml_language/crates/bridge_nodejs/protobaml_language/crates/bridge_nodejs/proto.d.tsbaml_language/crates/bridge_nodejs/proto.jsbaml_language/crates/bridge_nodejs/src/abort_controller.rsbaml_language/crates/bridge_nodejs/src/errors.rsbaml_language/crates/bridge_nodejs/src/handle.rsbaml_language/crates/bridge_nodejs/src/lib.rsbaml_language/crates/bridge_nodejs/src/runtime.rsbaml_language/crates/bridge_nodejs/src/types/collector.rsbaml_language/crates/bridge_nodejs/src/types/host_span_manager.rsbaml_language/crates/bridge_nodejs/src/types/mod.rsbaml_language/crates/bridge_nodejs/tests/call_function.test.tsbaml_language/crates/bridge_nodejs/tests/run_jest.rsbaml_language/crates/bridge_nodejs/tests/test_collector.test.tsbaml_language/crates/bridge_nodejs/tests/test_engine.test.tsbaml_language/crates/bridge_nodejs/tests/test_tracing.test.tsbaml_language/crates/bridge_nodejs/tsconfig.jsonbaml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/crates/bridge_nodejs/typescript_src/errors.tsbaml_language/crates/bridge_nodejs/typescript_src/generated-header.txtbaml_language/crates/bridge_nodejs/typescript_src/index.tsbaml_language/crates/bridge_nodejs/typescript_src/native.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.jsmise.tomltools/build
✅ Files skipped from review due to trivial changes (22)
- baml_language/crates/bridge_nodejs/proto
- baml_language/crates/bridge_nodejs/typescript_src/generated-header.txt
- baml_language/crates/bridge_nodejs/tests/test_collector.test.ts
- baml_language/crates/bridge_nodejs/src/types/mod.rs
- baml_language/Cargo.toml
- baml_language/crates/bridge_nodejs/tsconfig.json
- baml_language/crates/bridge_nodejs/proto.d.ts
- baml_language/crates/bridge_nodejs/src/abort_controller.rs
- baml_language/crates/bridge_nodejs/Cargo.toml
- baml_language/crates/bridge_nodejs/errors.js
- baml_language/crates/bridge_nodejs/errors.d.ts
- baml_language/crates/bridge_nodejs/ctx_manager.d.ts
- baml_language/crates/bridge_nodejs/typescript_src/errors.ts
- baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/crates/bridge_nodejs/ctx_manager.js
- baml_language/crates/bridge_nodejs/build.rs
- baml_language/crates/bridge_nodejs/native.d.ts
- baml_language/crates/bridge_nodejs/package.json
- baml_language/crates/bridge_nodejs/index.d.ts
- baml_language/crates/bridge_nodejs/src/handle.rs
- baml_language/crates/bridge_nodejs/src/lib.rs
- baml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.js
🚧 Files skipped from review as they are similar to previous changes (7)
- mise.toml
- baml_language/crates/bridge_nodejs/tests/run_jest.rs
- baml_language/crates/bridge_nodejs/src/errors.rs
- baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts
- baml_language/crates/bridge_nodejs/src/types/host_span_manager.rs
- baml_language/crates/bridge_nodejs/typescript_src/proto.ts
- baml_language/crates/bridge_nodejs/src/types/collector.rs
sxlijin
force-pushed
the
push-rtupkwyokymy
branch
from
d072f0d to
150272d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
baml_language/crates/bridge_nodejs/typescript_src/proto.ts (3)
40-50:⚠️ Potential issue | 🟠 MajorOnly serialize plain objects as protobuf maps.
This branch currently accepts every remaining object shape.
Date,Map,Set, and arbitrary class instances get silently encoded as{}or partial maps instead of failing fast, which corrupts arguments before they reach Rust.Suggested fix
+function isPlainObject(value: unknown): value is Record<string, unknown> { + if (value == null || typeof value !== 'object') return false; + const proto = Object.getPrototypeOf(value); + return proto === Object.prototype || proto === null; +} + function setInboundValue(iv: baml.cffi.v1.IInboundValue, value: unknown): void { @@ - } else if (typeof value === 'object') { + } else if (isPlainObject(value)) { const entries: baml.cffi.v1.IInboundMapEntry[] = []; for (const [k, v] of Object.entries(value as Record<string, unknown>)) { const entry: baml.cffi.v1.IInboundMapEntry = { stringKey: k }; const childVal: baml.cffi.v1.IInboundValue = {}; setInboundValue(childVal, v); entry.value = childVal; entries.push(entry); } iv.mapValue = { entries }; } else { - throw new TypeError(`Cannot encode value of type ${typeof value} to protobuf`); + throw new TypeError( + `Cannot encode value of type ${Object.prototype.toString.call(value)} to protobuf` + ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/proto.ts` around lines 40 - 50, The code is currently treating any non-primitive object as a protobuf map; update setInboundValue (the branch that builds baml.cffi.v1.IInboundMapEntry and sets iv.mapValue) to only accept plain objects (e.g., Object.prototype.toString.call(value) === '[object Object]' or value?.constructor === Object) and reject Date, Map, Set, class instances, etc.; if the value is not a plain object, do not build entries—throw or return a clear TypeError describing the unexpected type so callers fail fast instead of silently encoding {} into iv.mapValue.
94-101:⚠️ Potential issue | 🟠 MajorBuild decoded maps with a null prototype.
obj[entry.key] = ...on a plain{}lets a__proto__key mutate the returned object's prototype. SincemapValue.entriescomes from runtime/model data, this is a prototype-pollution sink for JS consumers.Suggested fix
if (holder.mapValue) { - const obj: Record<string, unknown> = {}; + const obj: Record<string, unknown> = Object.create(null); for (const entry of holder.mapValue.entries || []) { if (entry.key != null && entry.value) { obj[entry.key] = decodeValueHolder(entry.value); } } return obj; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/proto.ts` around lines 94 - 101, The object built for mapValue in decodeValueHolder is created as a plain {} which allows a "__proto__" key to mutate its prototype — change creation of obj in the map-handling branch of decodeValueHolder to use a null-prototype object (e.g., Object.create(null)) and keep assigning obj[entry.key] = decodeValueHolder(entry.value); so the returned map cannot be used for prototype pollution while preserving lookup semantics.
114-117:⚠️ Potential issue | 🟠 MajorThrow on unsupported outbound variants instead of returning
null.The fallback currently makes malformed or newer protobuf payloads indistinguishable from a real BAML
null, so bridge drift gets silently hidden.Suggested fix
if (holder.handleValue) { return new BamlHandle(holder.handleValue.key, holder.handleValue.handleType ?? 0); } - return null; + throw new TypeError('Cannot decode protobuf value: unsupported outbound variant'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/proto.ts` around lines 114 - 117, The code currently returns null when the incoming protobuf "holder" doesn't have handleValue, hiding malformed or newer outbound variants; update the conversion logic (the branch that checks holder.handleValue and currently returns null) to throw a descriptive error instead of returning null (e.g., reference BamlHandle and holder.handleValue) so callers can detect unsupported/out-of-spec variants; ensure the thrown message names the conversion function (proto.ts handler that constructs BamlHandle) and includes the unexpected holder shape or missing variant info for debugging.baml_language/crates/bridge_nodejs/ctx_manager.js (1)
20-20:⚠️ Potential issue | 🟠 MajorAvoid eager
HostSpanManagercreation in the constructor.Pre-seeding
AsyncLocalStoragehere can snapshot tracing state too early.get()already has the lazy initialization path; removing this eagerenterWithavoids stale manager state.♻️ Proposed fix
constructor(rt) { this.rt = rt; this.ctx = new node_async_hooks_1.AsyncLocalStorage(); - this.ctx.enterWith(new native_1.HostSpanManager()); if (!exitHookInstalled) { exitHookInstalled = true; process.once('exit', () => {Since this is generated output, update
baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsand regenerate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/ctx_manager.js` at line 20, Remove the eager creation/enterWith of HostSpanManager in the constructor to avoid pre-seeding AsyncLocalStorage: delete the this.ctx.enterWith(new native_1.HostSpanManager()) call (or equivalent) in the constructor and rely on the existing lazy initialization inside get() to create and enter a HostSpanManager when first needed; update the source in baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts accordingly and regenerate the JS output so the emitted ctx.enterWith call is no longer present.baml_language/crates/bridge_nodejs/typescript_src/index.ts (1)
55-59:⚠️ Potential issue | 🟡 MinorKeep “missing result bytes” distinct from explicit
nullpayloads.
bytes == nullanddecodeCallResult(bytes) === nullcurrently collapse to the same value. Returnundefinedfor missing bytes to preserve that distinction.♻️ Proposed fix
- get result(): unknown { + get result(): unknown | undefined { const bytes = this._inner.result; - if (bytes == null) return null; + if (bytes == null) return undefined; return decodeCallResult(bytes); }After this, regenerate
baml_language/crates/bridge_nodejs/index.jsand related declarations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts` around lines 55 - 59, The getter result in index.ts currently returns null for both missing bytes and decoded-null payloads because it checks `if (bytes == null) return null;` then returns `decodeCallResult(bytes)` which may also be null; change the early-return to `undefined` for the missing-byte case so missing bytes are distinct (i.e., when `this._inner.result` is null/undefined return undefined, otherwise return `decodeCallResult(bytes)`), referencing the `get result(): unknown` getter, `this._inner.result`, and `decodeCallResult` to locate the code; after changing the return value regenerate the built output `baml_language/crates/bridge_nodejs/index.js` and associated declaration files.baml_language/crates/bridge_nodejs/proto.js (1)
133-138:⚠️ Potential issue | 🟠 MajorDecode
uint8arrayValuebefore falling back tonull.
decodeValueHolderstill drops the bytes variant, so anyuint8array_valueresult is returned asnull.♻️ Proposed fix
if (holder.streamingStateValue && holder.streamingStateValue.value) { return decodeValueHolder(holder.streamingStateValue.value); } + if (holder.uint8arrayValue != null) { + return holder.uint8arrayValue; + } // handle_value: pass the protobufjs Long directly as the key — BamlHandle's // constructor accepts { low, high } which is layout-compatible with Long. if (holder.handleValue) { return new native_1.BamlHandle(holder.handleValue.key, holder.handleValue.handleType ?? 0); }Because this file is generated, apply the same fix in
baml_language/crates/bridge_nodejs/typescript_src/proto.tsand regenerate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/proto.js` around lines 133 - 138, The decodeValueHolder path currently ignores the bytes variant and returns null; update decodeValueHolder to check for holder.uint8arrayValue before returning null and return the decoded bytes (e.g., Buffer.from(holder.uint8arrayValue) or the raw Uint8Array depending on surrounding code expectations) so consumers receive the bytes value; make the same change in baml_language/crates/bridge_nodejs/typescript_src/proto.ts (adjusting types accordingly) and then regenerate the generated JS from the TS source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@baml_language/crates/bridge_nodejs/tests/run_jest.rs`:
- Around line 11-17: The pnpm install step in the Jest test harness (the Command
built into the let install = Command::new("pnpm") ... chain in run_jest.rs) can
mutate pnpm-lock.yaml; update that Command to pass the --frozen-lockfile flag so
pnpm cannot rewrite the lockfile (i.e., add .arg("--frozen-lockfile") to the
same Command before .current_dir(manifest_dir) and keep the existing status
handling/assertion with install.success()).
---
Duplicate comments:
In `@baml_language/crates/bridge_nodejs/ctx_manager.js`:
- Line 20: Remove the eager creation/enterWith of HostSpanManager in the
constructor to avoid pre-seeding AsyncLocalStorage: delete the
this.ctx.enterWith(new native_1.HostSpanManager()) call (or equivalent) in the
constructor and rely on the existing lazy initialization inside get() to create
and enter a HostSpanManager when first needed; update the source in
baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts accordingly and
regenerate the JS output so the emitted ctx.enterWith call is no longer present.
In `@baml_language/crates/bridge_nodejs/proto.js`:
- Around line 133-138: The decodeValueHolder path currently ignores the bytes
variant and returns null; update decodeValueHolder to check for
holder.uint8arrayValue before returning null and return the decoded bytes (e.g.,
Buffer.from(holder.uint8arrayValue) or the raw Uint8Array depending on
surrounding code expectations) so consumers receive the bytes value; make the
same change in baml_language/crates/bridge_nodejs/typescript_src/proto.ts
(adjusting types accordingly) and then regenerate the generated JS from the TS
source.
In `@baml_language/crates/bridge_nodejs/typescript_src/index.ts`:
- Around line 55-59: The getter result in index.ts currently returns null for
both missing bytes and decoded-null payloads because it checks `if (bytes ==
null) return null;` then returns `decodeCallResult(bytes)` which may also be
null; change the early-return to `undefined` for the missing-byte case so
missing bytes are distinct (i.e., when `this._inner.result` is null/undefined
return undefined, otherwise return `decodeCallResult(bytes)`), referencing the
`get result(): unknown` getter, `this._inner.result`, and `decodeCallResult` to
locate the code; after changing the return value regenerate the built output
`baml_language/crates/bridge_nodejs/index.js` and associated declaration files.
In `@baml_language/crates/bridge_nodejs/typescript_src/proto.ts`:
- Around line 40-50: The code is currently treating any non-primitive object as
a protobuf map; update setInboundValue (the branch that builds
baml.cffi.v1.IInboundMapEntry and sets iv.mapValue) to only accept plain objects
(e.g., Object.prototype.toString.call(value) === '[object Object]' or
value?.constructor === Object) and reject Date, Map, Set, class instances, etc.;
if the value is not a plain object, do not build entries—throw or return a clear
TypeError describing the unexpected type so callers fail fast instead of
silently encoding {} into iv.mapValue.
- Around line 94-101: The object built for mapValue in decodeValueHolder is
created as a plain {} which allows a "__proto__" key to mutate its prototype —
change creation of obj in the map-handling branch of decodeValueHolder to use a
null-prototype object (e.g., Object.create(null)) and keep assigning
obj[entry.key] = decodeValueHolder(entry.value); so the returned map cannot be
used for prototype pollution while preserving lookup semantics.
- Around line 114-117: The code currently returns null when the incoming
protobuf "holder" doesn't have handleValue, hiding malformed or newer outbound
variants; update the conversion logic (the branch that checks holder.handleValue
and currently returns null) to throw a descriptive error instead of returning
null (e.g., reference BamlHandle and holder.handleValue) so callers can detect
unsupported/out-of-spec variants; ensure the thrown message names the conversion
function (proto.ts handler that constructs BamlHandle) and includes the
unexpected holder shape or missing variant info for debugging.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 96190462-3a54-4d44-90c2-97cbafa38217
⛔ Files ignored due to path filters (12)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/crates/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_python/uv.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_nodejs/Cargo.tomlbaml_language/crates/bridge_nodejs/build.rsbaml_language/crates/bridge_nodejs/ctx_manager.d.tsbaml_language/crates/bridge_nodejs/ctx_manager.jsbaml_language/crates/bridge_nodejs/errors.d.tsbaml_language/crates/bridge_nodejs/errors.jsbaml_language/crates/bridge_nodejs/index.d.tsbaml_language/crates/bridge_nodejs/index.jsbaml_language/crates/bridge_nodejs/native.d.tsbaml_language/crates/bridge_nodejs/native.jsbaml_language/crates/bridge_nodejs/package.jsonbaml_language/crates/bridge_nodejs/protobaml_language/crates/bridge_nodejs/proto.d.tsbaml_language/crates/bridge_nodejs/proto.jsbaml_language/crates/bridge_nodejs/src/abort_controller.rsbaml_language/crates/bridge_nodejs/src/errors.rsbaml_language/crates/bridge_nodejs/src/handle.rsbaml_language/crates/bridge_nodejs/src/lib.rsbaml_language/crates/bridge_nodejs/src/runtime.rsbaml_language/crates/bridge_nodejs/src/types/collector.rsbaml_language/crates/bridge_nodejs/src/types/host_span_manager.rsbaml_language/crates/bridge_nodejs/src/types/mod.rsbaml_language/crates/bridge_nodejs/tests/call_function.test.tsbaml_language/crates/bridge_nodejs/tests/run_jest.rsbaml_language/crates/bridge_nodejs/tests/test_collector.test.tsbaml_language/crates/bridge_nodejs/tests/test_engine.test.tsbaml_language/crates/bridge_nodejs/tests/test_tracing.test.tsbaml_language/crates/bridge_nodejs/tsconfig.jsonbaml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/crates/bridge_nodejs/typescript_src/errors.tsbaml_language/crates/bridge_nodejs/typescript_src/generated-header.txtbaml_language/crates/bridge_nodejs/typescript_src/index.tsbaml_language/crates/bridge_nodejs/typescript_src/native.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.jsmise.tomltools/build
✅ Files skipped from review due to trivial changes (22)
- baml_language/crates/bridge_nodejs/proto
- baml_language/Cargo.toml
- baml_language/crates/bridge_nodejs/typescript_src/generated-header.txt
- baml_language/crates/bridge_nodejs/tsconfig.json
- baml_language/crates/bridge_nodejs/tests/test_collector.test.ts
- baml_language/crates/bridge_nodejs/src/types/mod.rs
- baml_language/crates/bridge_nodejs/proto.d.ts
- baml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.js
- tools/build
- baml_language/crates/bridge_nodejs/src/abort_controller.rs
- baml_language/crates/bridge_nodejs/src/lib.rs
- baml_language/crates/bridge_nodejs/Cargo.toml
- baml_language/crates/bridge_nodejs/errors.d.ts
- baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/crates/bridge_nodejs/typescript_src/native.d.ts
- baml_language/crates/bridge_nodejs/src/errors.rs
- baml_language/crates/bridge_nodejs/ctx_manager.d.ts
- baml_language/crates/bridge_nodejs/package.json
- baml_language/crates/bridge_nodejs/native.js
- baml_language/crates/bridge_nodejs/src/runtime.rs
- baml_language/crates/bridge_nodejs/index.d.ts
- baml_language/crates/bridge_nodejs/native.d.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- baml_language/crates/bridge_nodejs/build.rs
- baml_language/crates/bridge_nodejs/tests/test_engine.test.ts
- baml_language/crates/bridge_nodejs/errors.js
- baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts
- baml_language/crates/bridge_nodejs/src/types/host_span_manager.rs
Comment on lines
+11
to
+17
| // Step 1: Install npm dependencies | ||
| let install = Command::new("pnpm") | ||
| .arg("install") | ||
| .current_dir(manifest_dir) | ||
| .status() | ||
| .expect("failed to run pnpm install"); | ||
| assert!(install.success(), "pnpm install failed"); |
Contributor
There was a problem hiding this comment.
Use --frozen-lockfile in the Jest harness.
pnpm install here can rewrite pnpm-lock.yaml and resolve a different tree than CI, so this ignored test is not reproducible and can dirty the checkout even when the bridge code is fine. Match the workflow step instead.
Suggested fix
- let install = Command::new("pnpm")
- .arg("install")
+ let install = Command::new("pnpm")
+ .args(["install", "--frozen-lockfile"])
.current_dir(manifest_dir)
.status()
.expect("failed to run pnpm install");
- assert!(install.success(), "pnpm install failed");
+ assert!(install.success(), "pnpm install --frozen-lockfile failed");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Step 1: Install npm dependencies | |
| let install = Command::new("pnpm") | |
| .arg("install") | |
| .current_dir(manifest_dir) | |
| .status() | |
| .expect("failed to run pnpm install"); | |
| assert!(install.success(), "pnpm install failed"); | |
| // Step 1: Install npm dependencies | |
| let install = Command::new("pnpm") | |
| .args(["install", "--frozen-lockfile"]) | |
| .current_dir(manifest_dir) | |
| .status() | |
| .expect("failed to run pnpm install"); | |
| assert!(install.success(), "pnpm install --frozen-lockfile failed"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baml_language/crates/bridge_nodejs/tests/run_jest.rs` around lines 11 - 17,
The pnpm install step in the Jest test harness (the Command built into the let
install = Command::new("pnpm") ... chain in run_jest.rs) can mutate
pnpm-lock.yaml; update that Command to pass the --frozen-lockfile flag so pnpm
cannot rewrite the lockfile (i.e., add .arg("--frozen-lockfile") to the same
Command before .current_dir(manifest_dir) and keep the existing status
handling/assertion with install.success()).
150272d to
11a9936
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
baml_language/crates/bridge_nodejs/native.js (1)
73-80:⚠️ Potential issue | 🔴 CriticalMissing
returnforNAPI_RS_NATIVE_LIBRARY_PATHoverride — binding loads but gets discarded.When the env override succeeds,
nativeBindingis assigned locally but not returned. Line 369 then receivesundefined, causing the WASI fallback path to run unnecessarily (or fail entirely if WASI isn't available). All other platform branches correctly usereturn require(...).🐛 Proposed fix
function requireNative() { if (process.env.NAPI_RS_NATIVE_LIBRARY_PATH) { try { - nativeBinding = require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH); + return require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH); } catch (err) { loadErrors.push(err) } - } else if (process.platform === 'android') { + } + if (process.platform === 'android') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/native.js` around lines 73 - 80, In requireNative(), when process.env.NAPI_RS_NATIVE_LIBRARY_PATH is set and the require succeeds you assign nativeBinding but do not return it, so the caller gets undefined; change the branch to return the loaded binding (i.e., return the result of require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) or return nativeBinding after assignment) and preserve the existing catch behavior that pushes errors into loadErrors to ensure the override path short-circuits correctly.
🧹 Nitpick comments (2)
baml_language/crates/bridge_nodejs/tests/test_engine.test.ts (2)
69-109: Consider moving runtime creation intobeforeAllfor consistency.The runtime is created at describe-scope (line 70) which executes during test collection rather than test execution. While this works, using
beforeAll(as done incall_function.test.ts) provides clearer test lifecycle semantics and better error isolation if runtime creation fails.describe('callFunctionSync', () => { let rt: BamlRuntime; beforeAll(() => { rt = makeRuntime(BAML_SOURCE); }); // ... tests });This is a minor consistency improvement—the current code functions correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/tests/test_engine.test.ts` around lines 69 - 109, The runtime is created at describe-scope via makeRuntime(BAML_SOURCE) which runs during test collection; change it to initialize rt inside a beforeAll hook so runtime creation happens during test execution: declare let rt: BamlRuntime (or appropriate type) at describe scope, add a beforeAll that assigns rt = makeRuntime(BAML_SOURCE), and update the tests to use that rt; reference symbols: makeRuntime, rt, BAML_SOURCE, beforeAll, callFunctionSync.
145-156: AbortController tests verify flag state only.These tests confirm the
abortedproperty toggles correctly but don't verify that cancellation actually interrupts an in-flight operation. Consider adding an integration test that starts a long-running async call and aborts it mid-execution if cancellation behavior is critical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/tests/test_engine.test.ts` around lines 145 - 156, Add an integration test that verifies AbortController actually cancels an in-flight async operation: create a new test (e.g., "abort cancels in-flight operation") that constructs an AbortController, starts a long-running Promise/async function which listens to signal.aborted and rejects on abort (or attaches signal.addEventListener('abort')), then call ac.abort() mid-execution and assert the started operation rejects (or throws) with an abort-related error; reference AbortController and the long-running task/promise in your test file (test_engine.test.ts) and ensure the promise cleans up timers/listeners after assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@baml_language/crates/bridge_nodejs/native.js`:
- Around line 73-80: In requireNative(), when
process.env.NAPI_RS_NATIVE_LIBRARY_PATH is set and the require succeeds you
assign nativeBinding but do not return it, so the caller gets undefined; change
the branch to return the loaded binding (i.e., return the result of
require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH) or return nativeBinding after
assignment) and preserve the existing catch behavior that pushes errors into
loadErrors to ensure the override path short-circuits correctly.
---
Nitpick comments:
In `@baml_language/crates/bridge_nodejs/tests/test_engine.test.ts`:
- Around line 69-109: The runtime is created at describe-scope via
makeRuntime(BAML_SOURCE) which runs during test collection; change it to
initialize rt inside a beforeAll hook so runtime creation happens during test
execution: declare let rt: BamlRuntime (or appropriate type) at describe scope,
add a beforeAll that assigns rt = makeRuntime(BAML_SOURCE), and update the tests
to use that rt; reference symbols: makeRuntime, rt, BAML_SOURCE, beforeAll,
callFunctionSync.
- Around line 145-156: Add an integration test that verifies AbortController
actually cancels an in-flight async operation: create a new test (e.g., "abort
cancels in-flight operation") that constructs an AbortController, starts a
long-running Promise/async function which listens to signal.aborted and rejects
on abort (or attaches signal.addEventListener('abort')), then call ac.abort()
mid-execution and assert the started operation rejects (or throws) with an
abort-related error; reference AbortController and the long-running task/promise
in your test file (test_engine.test.ts) and ensure the promise cleans up
timers/listeners after assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 62dc347d-dc87-45ef-8d74-13b17ffe3c88
⛔ Files ignored due to path filters (12)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/crates/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_python/uv.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_nodejs/Cargo.tomlbaml_language/crates/bridge_nodejs/build.rsbaml_language/crates/bridge_nodejs/ctx_manager.d.tsbaml_language/crates/bridge_nodejs/ctx_manager.jsbaml_language/crates/bridge_nodejs/errors.d.tsbaml_language/crates/bridge_nodejs/errors.jsbaml_language/crates/bridge_nodejs/index.d.tsbaml_language/crates/bridge_nodejs/index.jsbaml_language/crates/bridge_nodejs/native.d.tsbaml_language/crates/bridge_nodejs/native.jsbaml_language/crates/bridge_nodejs/package.jsonbaml_language/crates/bridge_nodejs/protobaml_language/crates/bridge_nodejs/proto.d.tsbaml_language/crates/bridge_nodejs/proto.jsbaml_language/crates/bridge_nodejs/src/abort_controller.rsbaml_language/crates/bridge_nodejs/src/errors.rsbaml_language/crates/bridge_nodejs/src/handle.rsbaml_language/crates/bridge_nodejs/src/lib.rsbaml_language/crates/bridge_nodejs/src/runtime.rsbaml_language/crates/bridge_nodejs/src/types/collector.rsbaml_language/crates/bridge_nodejs/src/types/host_span_manager.rsbaml_language/crates/bridge_nodejs/src/types/mod.rsbaml_language/crates/bridge_nodejs/tests/call_function.test.tsbaml_language/crates/bridge_nodejs/tests/run_jest.rsbaml_language/crates/bridge_nodejs/tests/test_collector.test.tsbaml_language/crates/bridge_nodejs/tests/test_engine.test.tsbaml_language/crates/bridge_nodejs/tests/test_tracing.test.tsbaml_language/crates/bridge_nodejs/tsconfig.jsonbaml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/crates/bridge_nodejs/typescript_src/errors.tsbaml_language/crates/bridge_nodejs/typescript_src/generated-header.txtbaml_language/crates/bridge_nodejs/typescript_src/index.tsbaml_language/crates/bridge_nodejs/typescript_src/native.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.jsmise.tomltools/build
✅ Files skipped from review due to trivial changes (25)
- baml_language/Cargo.toml
- baml_language/crates/bridge_nodejs/typescript_src/generated-header.txt
- baml_language/crates/bridge_nodejs/tsconfig.json
- baml_language/crates/bridge_nodejs/src/types/mod.rs
- mise.toml
- baml_language/crates/bridge_nodejs/tests/run_jest.rs
- baml_language/crates/bridge_nodejs/proto.d.ts
- baml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.js
- baml_language/crates/bridge_nodejs/src/abort_controller.rs
- baml_language/crates/bridge_nodejs/errors.d.ts
- baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/crates/bridge_nodejs/src/errors.rs
- baml_language/crates/bridge_nodejs/errors.js
- baml_language/crates/bridge_nodejs/typescript_src/errors.ts
- baml_language/crates/bridge_nodejs/proto.js
- baml_language/crates/bridge_nodejs/typescript_src/native.d.ts
- baml_language/crates/bridge_nodejs/src/handle.rs
- baml_language/crates/bridge_nodejs/Cargo.toml
- baml_language/crates/bridge_nodejs/proto
- baml_language/crates/bridge_nodejs/src/runtime.rs
- baml_language/crates/bridge_nodejs/typescript_src/proto.ts
- baml_language/crates/bridge_nodejs/tests/test_collector.test.ts
- baml_language/crates/bridge_nodejs/index.d.ts
- baml_language/crates/bridge_nodejs/ctx_manager.d.ts
- baml_language/crates/bridge_nodejs/native.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/bridge_nodejs/src/lib.rs
- baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts
- baml_language/crates/bridge_nodejs/package.json
- baml_language/crates/bridge_nodejs/build.rs
sxlijin
force-pushed
the
push-rtupkwyokymy
branch
from
11a9936 to
2653e1b
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
.github/workflows/ci.yaml (1)
386-393:⚠️ Potential issue | 🟠 MajorPin third-party Actions to immutable commit SHAs.
Line 386 and Line 392 still use mutable tags (
@v2,@v4), which leaves this job open to supply-chain drift.Resolve immutable commit SHAs for these GitHub Actions tags: - Swatinem/rust-cache@v2 - pnpm/action-setup@v4 Then update .github/workflows/ci.yaml to use owner/repo@<full-commit-sha>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 386 - 393, The workflow uses mutable action tags Swatinem/rust-cache@v2 and pnpm/action-setup@v4; replace each tag with the corresponding repository@<full-commit-sha> to pin to an immutable commit. Locate the uses entries for Swatinem/rust-cache and pnpm/action-setup in the CI job (the lines showing "uses: Swatinem/rust-cache@v2" and "uses: pnpm/action-setup@v4"), find the exact commit SHA that corresponds to the desired v2/v4 release on each repo, and update the workflow to use owner/repo@<full-commit-sha> for both actions.baml_language/crates/bridge_nodejs/proto.js (1)
144-148:⚠️ Potential issue | 🟠 MajorMissing
uint8arrayValuedecode branch.The decoder handles most
BamlOutboundValuevariants but does not decodeuint8arrayValue. Any BAML function returning bytes will collapse tonull. This should be fixed intypescript_src/proto.tsand regenerated.Suggested fix direction
Add a branch before the final
return null:if (holder.handleValue) { return new native_1.BamlHandle(holder.handleValue.key, holder.handleValue.handleType ?? 0); } + if (holder.uint8arrayValue != null) { + return holder.uint8arrayValue; + } // FIXME: Unknown/unsupported outbound variants silently collapse to null... return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/proto.js` around lines 144 - 148, The decoder for BamlOutboundValue is missing a branch for the uint8arrayValue variant so any bytes-returning BAML functions become null; update the decode function (the BamlOutboundValue decoder in proto.js and the source typescript decoder in typescript_src/proto.ts) to handle the uint8arrayValue variant by reading the length-prefixed byte buffer or appropriate field and returning a Uint8Array (or Buffer as used in Node) before the final `return null`, then regenerate the JS from the TS so proto.js includes the new branch.baml_language/crates/bridge_nodejs/native.js (1)
73-79:⚠️ Potential issue | 🔴 Critical
NAPI_RS_NATIVE_LIBRARY_PATHoverride silently fails.When the env var is set and
require()succeeds, this branch assigns tonativeBindingbut never returns it. Line 369 then assignsrequireNative()'s return value (undefined) back tonativeBinding, discarding the loaded module.Suggested fix
function requireNative() { if (process.env.NAPI_RS_NATIVE_LIBRARY_PATH) { try { - nativeBinding = require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH); + return require(process.env.NAPI_RS_NATIVE_LIBRARY_PATH); } catch (err) { loadErrors.push(err) } } else if (process.platform === 'android') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/native.js` around lines 73 - 79, In requireNative(), when process.env.NAPI_RS_NATIVE_LIBRARY_PATH is set and require(...) succeeds the loaded module is assigned to nativeBinding but the function doesn't return it, causing callers (e.g. the later assignment nativeBinding = requireNative()) to overwrite it with undefined; fix by returning the loaded module from requireNative() after successful require (and keep pushing errors to loadErrors on catch) so the caller receives the module; update the requireNative() code path that handles NAPI_RS_NATIVE_LIBRARY_PATH to return nativeBinding (the successfully required module).
🧹 Nitpick comments (1)
baml_language/crates/bridge_nodejs/index.js (1)
112-118: Duplicate exit handler withctx_manager.js.Both
index.js(line 113) andctx_manager.js(line 28) registerprocess.once('exit')handlers that callflushEvents(). Since they are different function references, both will execute, causingflushEvents()to be called twice on process exit. This is likely harmless (flush is idempotent), but wasteful.Consider consolidating the exit handler in one location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bridge_nodejs/index.js` around lines 112 - 118, There are two process.once('exit') handlers calling native_1.flushEvents (one in index.js and another in ctx_manager.js) causing flushEvents to run twice; remove the duplicate by consolidating the exit handler into a single module (e.g., keep the handler in ctx_manager.js or index.js) and have the other module call a shared registrar instead—create or expose a single function name like registerFlushOnExit or export native_1.flushEvents and call it from only one process.once('exit') handler so only one handler references process.once('exit') and invokes flushEvents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yaml:
- Around line 386-393: The workflow uses mutable action tags
Swatinem/rust-cache@v2 and pnpm/action-setup@v4; replace each tag with the
corresponding repository@<full-commit-sha> to pin to an immutable commit. Locate
the uses entries for Swatinem/rust-cache and pnpm/action-setup in the CI job
(the lines showing "uses: Swatinem/rust-cache@v2" and "uses:
pnpm/action-setup@v4"), find the exact commit SHA that corresponds to the
desired v2/v4 release on each repo, and update the workflow to use
owner/repo@<full-commit-sha> for both actions.
In `@baml_language/crates/bridge_nodejs/native.js`:
- Around line 73-79: In requireNative(), when
process.env.NAPI_RS_NATIVE_LIBRARY_PATH is set and require(...) succeeds the
loaded module is assigned to nativeBinding but the function doesn't return it,
causing callers (e.g. the later assignment nativeBinding = requireNative()) to
overwrite it with undefined; fix by returning the loaded module from
requireNative() after successful require (and keep pushing errors to loadErrors
on catch) so the caller receives the module; update the requireNative() code
path that handles NAPI_RS_NATIVE_LIBRARY_PATH to return nativeBinding (the
successfully required module).
In `@baml_language/crates/bridge_nodejs/proto.js`:
- Around line 144-148: The decoder for BamlOutboundValue is missing a branch for
the uint8arrayValue variant so any bytes-returning BAML functions become null;
update the decode function (the BamlOutboundValue decoder in proto.js and the
source typescript decoder in typescript_src/proto.ts) to handle the
uint8arrayValue variant by reading the length-prefixed byte buffer or
appropriate field and returning a Uint8Array (or Buffer as used in Node) before
the final `return null`, then regenerate the JS from the TS so proto.js includes
the new branch.
---
Nitpick comments:
In `@baml_language/crates/bridge_nodejs/index.js`:
- Around line 112-118: There are two process.once('exit') handlers calling
native_1.flushEvents (one in index.js and another in ctx_manager.js) causing
flushEvents to run twice; remove the duplicate by consolidating the exit handler
into a single module (e.g., keep the handler in ctx_manager.js or index.js) and
have the other module call a shared registrar instead—create or expose a single
function name like registerFlushOnExit or export native_1.flushEvents and call
it from only one process.once('exit') handler so only one handler references
process.once('exit') and invokes flushEvents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 775b1565-17fd-4448-b6d6-b24c5773af9e
⛔ Files ignored due to path filters (12)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/bridge_nodejs/ctx_manager.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/ctx_manager.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/errors.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/index.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbaml_language/crates/bridge_nodejs/proto.d.ts.mapis excluded by!**/*.mapbaml_language/crates/bridge_nodejs/proto.js.mapis excluded by!**/*.mapbaml_language/crates/bridge_python/uv.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
.github/workflows/ci.yamlbaml_language/Cargo.tomlbaml_language/crates/bridge_nodejs/Cargo.tomlbaml_language/crates/bridge_nodejs/build.rsbaml_language/crates/bridge_nodejs/ctx_manager.d.tsbaml_language/crates/bridge_nodejs/ctx_manager.jsbaml_language/crates/bridge_nodejs/errors.d.tsbaml_language/crates/bridge_nodejs/errors.jsbaml_language/crates/bridge_nodejs/index.d.tsbaml_language/crates/bridge_nodejs/index.jsbaml_language/crates/bridge_nodejs/native.d.tsbaml_language/crates/bridge_nodejs/native.jsbaml_language/crates/bridge_nodejs/package.jsonbaml_language/crates/bridge_nodejs/protobaml_language/crates/bridge_nodejs/proto.d.tsbaml_language/crates/bridge_nodejs/proto.jsbaml_language/crates/bridge_nodejs/src/abort_controller.rsbaml_language/crates/bridge_nodejs/src/errors.rsbaml_language/crates/bridge_nodejs/src/handle.rsbaml_language/crates/bridge_nodejs/src/lib.rsbaml_language/crates/bridge_nodejs/src/runtime.rsbaml_language/crates/bridge_nodejs/src/types/collector.rsbaml_language/crates/bridge_nodejs/src/types/host_span_manager.rsbaml_language/crates/bridge_nodejs/src/types/mod.rsbaml_language/crates/bridge_nodejs/tests/call_function.test.tsbaml_language/crates/bridge_nodejs/tests/run_jest.rsbaml_language/crates/bridge_nodejs/tests/test_collector.test.tsbaml_language/crates/bridge_nodejs/tests/test_engine.test.tsbaml_language/crates/bridge_nodejs/tests/test_tracing.test.tsbaml_language/crates/bridge_nodejs/tsconfig.jsonbaml_language/crates/bridge_nodejs/typescript_src/ctx_manager.tsbaml_language/crates/bridge_nodejs/typescript_src/errors.tsbaml_language/crates/bridge_nodejs/typescript_src/generated-header.txtbaml_language/crates/bridge_nodejs/typescript_src/index.tsbaml_language/crates/bridge_nodejs/typescript_src/native.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/crates/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.jsmise.tomlpnpm-workspace.yamltools/build
✅ Files skipped from review due to trivial changes (24)
- pnpm-workspace.yaml
- baml_language/crates/bridge_nodejs/proto
- baml_language/Cargo.toml
- baml_language/crates/bridge_nodejs/tsconfig.json
- baml_language/crates/bridge_nodejs/typescript_src/generated-header.txt
- mise.toml
- baml_language/crates/bridge_nodejs/proto.d.ts
- baml_language/crates/bridge_nodejs/src/types/mod.rs
- tools/build
- baml_language/crates/bridge_nodejs/typescript_src/tag-generated-files.js
- baml_language/crates/bridge_nodejs/tests/test_collector.test.ts
- baml_language/crates/bridge_nodejs/Cargo.toml
- baml_language/crates/bridge_nodejs/tests/call_function.test.ts
- baml_language/crates/bridge_nodejs/tests/test_tracing.test.ts
- baml_language/crates/bridge_nodejs/tests/test_engine.test.ts
- baml_language/crates/bridge_nodejs/src/lib.rs
- baml_language/crates/bridge_nodejs/errors.d.ts
- baml_language/crates/bridge_nodejs/typescript_src/errors.ts
- baml_language/crates/bridge_nodejs/errors.js
- baml_language/crates/bridge_nodejs/ctx_manager.d.ts
- baml_language/crates/bridge_nodejs/package.json
- baml_language/crates/bridge_nodejs/index.d.ts
- baml_language/crates/bridge_nodejs/src/types/collector.rs
- baml_language/crates/bridge_nodejs/native.d.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- baml_language/crates/bridge_nodejs/build.rs
- baml_language/crates/bridge_nodejs/tests/run_jest.rs
- baml_language/crates/bridge_nodejs/src/handle.rs
- baml_language/crates/bridge_nodejs/typescript_src/proto.ts
- baml_language/crates/bridge_nodejs/typescript_src/ctx_manager.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit