Repository navigation
feat: wire cancellation in bridges - #3720
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR threads explicit nonzero u64 call IDs through CallFunctionArgs, bridge CFFI, engine active-call tracking (including pre-cancel reservations), adds cancel/new call-id APIs, updates Node.js and Python bindings/SDKs to use call IDs for cancellation, adjusts proto and generated artifacts, and adds cross-language cancellation tests and docs. ChangesCall ID & Engine
Node (Rust + TS SDK)
Python (Rust bindings + SDK)
Tests, Docs & Misc
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as SDK (TS/Py)
participant Runtime as BamlRuntime
participant CFFI as bridge_cffi
participant Engine as BexEngine
Client->>SDK: call_function_sync/async (optionally with BamlCallContext)
SDK->>CFFI: new_function_call_id() -> call_id
SDK->>SDK: attachCallContext(callCtx?, call_id)
SDK->>Runtime: call_function(args_proto including call_id)
Runtime->>CFFI: function_call_context_builder(call_id)
CFFI->>Engine: execute(call_ctx(call_id))
alt user aborts before/during execution
Client->>SDK: callCtx.abort() or task cancel
SDK->>CFFI: cancel_function_call_by_id(call_id)
CFFI->>Engine: runtime.cancel_function_call(call_id) or reserve pre-cancel
Engine-->>Runtime: Cancelled panic/error
Runtime-->>SDK: outbound Cancelled class
SDK-->>Client: throw BamlAbortError / asyncio.CancelledError (with reason)
else success
Engine-->>Runtime: return result
Runtime-->>SDK: result bytes
SDK-->>Client: decoded return value
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
![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: 8e04c7dfe0
ℹ️ 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".
| @@ -1,136 +0,0 @@ | |||
| /** | |||
There was a problem hiding this comment.
Deleting the built dist tree leaves the Node package with no resolvable entrypoint: package.json still points exports['.'].import and types at ./dist/index.js / ./dist/index.d.ts, and the bridge tests import ../dist/index.js. In a checkout or published package that has not run the build first, @boundaryml/baml-core-node will fail to import before any cancellation code can run.
Useful? React with 👍 / 👎.
Comment on lines
225
to
226
| * later kwarg fails, the engine never sees (and so never releases) the keys we | ||
| * already registered, so we release them here. |
There was a problem hiding this comment.
This exported helper now requires an options object and immediately reads options.callId; existing callers that still use the previous public form encodeCallArgs(kwargs) now throw before encoding. For example, baml_language/sdks/nodejs/bridge_nodejs/tests/test_stream.test.ts still calls encodeCallArgs({ self: stream }), and consumers importing the helper from dist/index.js have the same failure unless every call site is migrated or a default call id is allocated here.
Useful? React with 👍 / 👎.
|
No description provided. |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts (1)
229-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject all invalid
callIdvalues, not just0n.Line 230 only rejects
0n, so this public API still accepts negative IDs and values above theuint64ceiling even though the contract here is “nonzero uint64”. Please fail fast before serializing so invalid IDs never cross the bridge.Suggested fix
export function encodeCallArgs(kwargs: Record<string, unknown>, options: EncodeCallArgsOptions): Buffer { const callId = options.callId; - if (callId === 0n) { + const MAX_CALL_ID = (1n << 64n) - 1n; + if (callId <= 0n || callId > MAX_CALL_ID) { throw new TypeError('callId must be a nonzero uint64'); } const ctx: EncodeCtx = { syncMode: options.syncMode ?? false, registered: [] };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts` around lines 229 - 243, The current check only rejects callId === 0n; instead validate that callId is a BigInt and within the uint64 (1..2**64-1) range before serializing: ensure callId is of type bigint, greater than 0n, and less than or equal to 18446744073709551615n (max uint64); if not, throw TypeError('callId must be a nonzero uint64'). Update the validation near the callId variable (the logic right before constructing CallFunctionArgs.fromObject) so invalid negative or overflow values are rejected early.baml_language/sdks/python/rust/bridge_python/src/abort_controller.rs (1)
10-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh the
AbortControllerdocs to match the new call-id flow.The example still tells users to pass
abort_controller=intocall_function/call_function_sync, but those parameters were removed in this slice. It also describes a single associated call even though the controller now tracks multiple active call IDs. The generated.pyiinherits the same stale example, so this is best fixed here at the source.Also applies to: 36-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/python/rust/bridge_python/src/abort_controller.rs` around lines 10 - 16, The doc example for AbortController is stale: remove the old "abort_controller=" usage and replace it with the new call-id flow showing how to create an AbortController, register one or more call IDs with the controller (e.g. call controller.register_call(call_id) — use the actual register/unregister method names defined on AbortController in this file), show how those call IDs are associated with calls via the runtime's new call-id parameter (use the runtime's "call_id" / call option name), and demonstrate that controller.abort() cancels all tracked call IDs; update the comment block for AbortController and regenerate the .pyi so the new example appears in the published typing stub.
🧹 Nitpick comments (6)
baml_language/sdk_tests/DEVELOPMENT.md (1)
240-240: 💤 Low valueConsider adding a trailing newline.
The file ends without a newline character. While not required, many tools and style guides prefer files to end with a newline for better diff behavior and POSIX compliance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/DEVELOPMENT.md` at line 240, Add a single trailing newline character at the end of the DEVELOPMENT.md file so the file ends with a newline (POSIX-compliant EOF); open the file, place a newline after the last line, save, and commit the change so it appears in the PR (no other modifications required).baml_language/crates/bridge_cffi/src/lib.rs (4)
66-73: ⚖️ Poor tradeoffConsider adding cleanup for stale pending cancellations.
The
PENDING_CANCELSset retains cancelled call IDs indefinitely until they're consumed byfunction_call_context_builder. If a host cancels a call ID but never submits the call, that ID remains in the set forever. The 4096 cap bounds the leak, but when the cap is reached, new pending cancellations are silently dropped (line 304-307).Consider one of:
- TTL-based eviction for stale entries
- Cleanup hook when calls complete to remove IDs that were never pending
- Documentation explaining the tradeoff and the 4096 limit's rationale
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bridge_cffi/src/lib.rs` around lines 66 - 73, PENDING_CANCELS currently stores cancelled call IDs forever and can silently drop new IDs once the ~4096 cap is hit; update the implementation so pending cancellations are garbage-collected: either switch PENDING_CANCELS from HashSet<u64> to a timestamped map (e.g. HashMap<u64, Instant>) and prune entries older than a TTL on insert/lookup, or add explicit removal of IDs from PENDING_CANCELS in the call completion/teardown path (the same place that finalizes calls in function_call_context_builder) so IDs cancelled but never submitted are cleared; also add a brief comment/doc explaining the 4096 cap and chosen eviction strategy.
303-307: ⚡ Quick winDocument the behavior when
PENDING_CANCELSreaches capacity.When the pending-cancellation set reaches 4096 entries, new cancellations are silently dropped (the
pending.len() < 4096check on line 304 prevents insertion). This means:
- Pre-cancellation requests beyond the cap are lost
- No error is returned to the caller (they get
false, same as other failure modes)- The limit is hardcoded
Consider:
- Documenting this limit and behavior in the
PENDING_CANCELScomment- Adding a const for the 4096 limit
- Logging when the cap is reached to aid debugging
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bridge_cffi/src/lib.rs` around lines 303 - 307, The pending-cancellation set currently drops new cancellations silently when it reaches 4096 entries; add a const like MAX_PENDING_CANCELS = 4096 and replace the hardcoded 4096 in the `pending.len() < 4096` check with that constant, add/update the comment that declares `PENDING_CANCELS` to document the cap and the fact that excess cancellations are dropped (and any semantic implications), and emit a diagnostic (e.g., log.warn or tracing::warn) right where the insertion would be skipped (the branch surrounding `PENDING_CANCELS.lock()` / `pending.len() < ...` and `pending.insert(id)`) so callers/operators can observe when the cap is reached.
286-309: ⚡ Quick winClarify the return value semantics of
cancel_function_call_by_id.The function returns
falsewhen successfully adding a call ID toPENDING_CANCELS(line 308), which might confuse callers. Current behavior:
- Returns
true→ runtime immediately cancelled an active call- Returns
false→ call not active, but may be queued for pre-cancellationThe documentation (lines 288-289) says "Returns true on success, false if the call ID is unknown, completed, or the runtime is not initialized," but doesn't mention the pending-cancellation case.
Consider:
- Updating the documentation to explicitly cover the pending-cancellation scenario
- Or, returning
truewhen successfully queuing toPENDING_CANCELSto indicate "cancellation recorded"- Or, using a tri-state return (e.g., enum:
Cancelled,Pending,NotFound)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bridge_cffi/src/lib.rs` around lines 286 - 309, The current cancel_function_call_by_id returns false when an ID is queued in PENDING_CANCELS which contradicts the docstring; change the behavior to return true when you successfully insert into PENDING_CANCELS and update the function docstring to state that true means either the runtime cancelled the active call (via runtime.cancel_function_call) or the ID was recorded for pending pre-cancellation, while false means the ID was invalid, completed, or the runtime is unavailable; update references to get_runtime, runtime.cancel_function_call, and PENDING_CANCELS in the function to reflect this new semantic.
265-284: 💤 Low valueConsider logging or handling lock poison in
PENDING_CANCELSaccess.Line 276's
unwrap_or(false)treats a poisoned lock as "not pending", allowing the call to proceed without the pre-cancelled token. This is a safe failure mode (the call runs instead of panicking), but:
- A pre-cancelled call might not receive its cancellation signal
- The poisoned state is silently ignored
- Debugging lock corruption becomes harder
Consider logging when the lock is poisoned, or using
.expect()with a clear message if lock poison should be treated as a critical failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bridge_cffi/src/lib.rs` around lines 265 - 284, The access to PENDING_CANCELS in function_call_context_builder currently swallows lock poison via unwrap_or(false); change it to explicitly handle the Result from PENDING_CANCELS.lock() (match or if let) so you can detect PoisonError and either log the poison (using the crate's logger or eprintln!) and treat it conservatively (e.g., assume pending cancellation or propagate a failure) or call .expect() with a clear message if poison should be fatal; update the code around the was_pending assignment (the block that calls pending.remove(&call_id.0)) to handle Ok(mut guard) and Err(poison) separately and perform the chosen logging/behavior.baml_language/sdks/python/src/baml_core/__init__.py (1)
191-193: ⚡ Quick winDefensive handling for cancel_function_call in exception path.
If
cancel_function_call(call_id)raises an exception at line 192, it would replace theasyncio.CancelledErrorbeing handled, potentially masking the original cancellation. Consider wrapping it in a try/except to ensure the CancelledError is always re-raised.The same pattern appears in the async
define_functionat lines 304-306.🛡️ Proposed defensive wrapper
except asyncio.CancelledError: - cancel_function_call(call_id) + try: + cancel_function_call(call_id) + except Exception: + pass # best-effort; never mask the cancellation raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/python/src/baml_core/__init__.py` around lines 191 - 193, Wrap calls to cancel_function_call(call_id) in a defensive try/except inside the asyncio.CancelledError handler so any exception raised by cancel_function_call does not replace the original CancelledError; catch Exception (or BaseException if you need to be extra safe), optionally log it, and then always re-raise the original CancelledError. Apply this change both where cancel_function_call(call_id) is called in the current CancelledError handler and in the async define_function cancellation handler (the block around the cancel_function_call invocation) so CancelledError is never masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/sdk_tests/README.md`:
- Line 21: Remove the stray ">" in the README sentence fragment so the line
reads "SDK tests are designed to be run using `cargo nextest run` and will fail
in" (i.e., change `will > fail in` to `will fail in`) to restore proper
blockquote/markdown formatting; update the README.md sentence accordingly and
verify the surrounding blockquote markers still render as intended.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.ts`:
- Around line 148-156: Generated call paths call the native runtime
(rt.callFunctionSync / rt.callFunctionAsync / rt.callFunction... sites) and
currently let native N-API throws escape without conversion; wrap the native
calls so any thrown error is passed through wrapNativeError before rethrowing
(preserve the existing abortBinding.detach/finally logic and then call
decodeCallResult and throwWithCapturedAbortReason as before), i.e., catch errors
thrown by rt.callFunction* calls, call wrapNativeError(err) and rethrow the
result so all generated functions produce Baml*Error subclasses like index.ts.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.ts`:
- Around line 81-83: wrapNativeError currently converts any native Error into a
plain BamlError, which strips subclass types like BamlInvalidCallError expected
by callers; change wrapNativeError to preserve and rethrow native errors that
are instances of BamlError or any of its subclasses (e.g., BamlInvalidCallError)
instead of always wrapping them, while still converting non-Error values to new
BamlError(String(err)). Locate the wrapNativeError function and modify its logic
to return the original error if err instanceof BamlError (or otherwise preserve
subclass identity), and only construct new BamlError for non-Error/unknown
values so index.ts call sites receive the correct typed Baml*Error subclasses.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.ts`:
- Around line 57-67: The native bridge calls in _callSync and _callAsync can
throw raw Errors that bypass wrapNativeError; wrap the calls to
rt.callFunctionSync (in _callSync) and await rt.callFunction (in _callAsync) in
try/catch blocks, and on any caught error call wrapNativeError(err) and rethrow
the wrapped error before proceeding to decodeCallResult; keep the surrounding
logic (getRuntime, encodeCallArgs, decodeCallResult) unchanged so only the
native-call invocation is translated to use wrapNativeError.
In `@baml_language/sdks/python/src/baml_core/_stream.py`:
- Around line 85-93: The async helper _call_async currently creates a function
call proto inline and awaits rt.call_function without retaining the generated
call_id, so cancellations never invoke cancel_function_call; change _call_async
to extract the call_id from new_function_call() (or the encoded args) before
awaiting, then wrap the await in try/finally (or catch asyncio.CancelledError)
and on cancellation call rt.cancel_function_call(call_id); ensure you still
decode the result with decode_call_result and preserve uses of get_runtime,
encode_call_args and call_function.
---
Outside diff comments:
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 229-243: The current check only rejects callId === 0n; instead
validate that callId is a BigInt and within the uint64 (1..2**64-1) range before
serializing: ensure callId is of type bigint, greater than 0n, and less than or
equal to 18446744073709551615n (max uint64); if not, throw TypeError('callId
must be a nonzero uint64'). Update the validation near the callId variable (the
logic right before constructing CallFunctionArgs.fromObject) so invalid negative
or overflow values are rejected early.
In `@baml_language/sdks/python/rust/bridge_python/src/abort_controller.rs`:
- Around line 10-16: The doc example for AbortController is stale: remove the
old "abort_controller=" usage and replace it with the new call-id flow showing
how to create an AbortController, register one or more call IDs with the
controller (e.g. call controller.register_call(call_id) — use the actual
register/unregister method names defined on AbortController in this file), show
how those call IDs are associated with calls via the runtime's new call-id
parameter (use the runtime's "call_id" / call option name), and demonstrate that
controller.abort() cancels all tracked call IDs; update the comment block for
AbortController and regenerate the .pyi so the new example appears in the
published typing stub.
---
Nitpick comments:
In `@baml_language/crates/bridge_cffi/src/lib.rs`:
- Around line 66-73: PENDING_CANCELS currently stores cancelled call IDs forever
and can silently drop new IDs once the ~4096 cap is hit; update the
implementation so pending cancellations are garbage-collected: either switch
PENDING_CANCELS from HashSet<u64> to a timestamped map (e.g. HashMap<u64,
Instant>) and prune entries older than a TTL on insert/lookup, or add explicit
removal of IDs from PENDING_CANCELS in the call completion/teardown path (the
same place that finalizes calls in function_call_context_builder) so IDs
cancelled but never submitted are cleared; also add a brief comment/doc
explaining the 4096 cap and chosen eviction strategy.
- Around line 303-307: The pending-cancellation set currently drops new
cancellations silently when it reaches 4096 entries; add a const like
MAX_PENDING_CANCELS = 4096 and replace the hardcoded 4096 in the `pending.len()
< 4096` check with that constant, add/update the comment that declares
`PENDING_CANCELS` to document the cap and the fact that excess cancellations are
dropped (and any semantic implications), and emit a diagnostic (e.g., log.warn
or tracing::warn) right where the insertion would be skipped (the branch
surrounding `PENDING_CANCELS.lock()` / `pending.len() < ...` and
`pending.insert(id)`) so callers/operators can observe when the cap is reached.
- Around line 286-309: The current cancel_function_call_by_id returns false when
an ID is queued in PENDING_CANCELS which contradicts the docstring; change the
behavior to return true when you successfully insert into PENDING_CANCELS and
update the function docstring to state that true means either the runtime
cancelled the active call (via runtime.cancel_function_call) or the ID was
recorded for pending pre-cancellation, while false means the ID was invalid,
completed, or the runtime is unavailable; update references to get_runtime,
runtime.cancel_function_call, and PENDING_CANCELS in the function to reflect
this new semantic.
- Around line 265-284: The access to PENDING_CANCELS in
function_call_context_builder currently swallows lock poison via
unwrap_or(false); change it to explicitly handle the Result from
PENDING_CANCELS.lock() (match or if let) so you can detect PoisonError and
either log the poison (using the crate's logger or eprintln!) and treat it
conservatively (e.g., assume pending cancellation or propagate a failure) or
call .expect() with a clear message if poison should be fatal; update the code
around the was_pending assignment (the block that calls
pending.remove(&call_id.0)) to handle Ok(mut guard) and Err(poison) separately
and perform the chosen logging/behavior.
In `@baml_language/sdk_tests/DEVELOPMENT.md`:
- Line 240: Add a single trailing newline character at the end of the
DEVELOPMENT.md file so the file ends with a newline (POSIX-compliant EOF); open
the file, place a newline after the last line, save, and commit the change so it
appears in the PR (no other modifications required).
In `@baml_language/sdks/python/src/baml_core/__init__.py`:
- Around line 191-193: Wrap calls to cancel_function_call(call_id) in a
defensive try/except inside the asyncio.CancelledError handler so any exception
raised by cancel_function_call does not replace the original CancelledError;
catch Exception (or BaseException if you need to be extra safe), optionally log
it, and then always re-raise the original CancelledError. Apply this change both
where cancel_function_call(call_id) is called in the current CancelledError
handler and in the async define_function cancellation handler (the block around
the cancel_function_call invocation) so CancelledError is never masked.
🪄 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: 904e8433-9640-43ef-948d-daef2a0ddcce
⛔ Files ignored due to path filters (41)
baml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/errors.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/errors.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/errors.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/errors.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/native.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/native.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/stream.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/typemap.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/typemap.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/typemap.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/typemap.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (33)
baml_language/crates/bridge_cffi/src/baml_to_host.rsbaml_language/crates/bridge_cffi/src/error.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.protobaml_language/crates/bridge_wasm/tests/host_callable.rsbaml_language/sdk_tests/DEVELOPMENT.mdbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_cancellation.pybaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_errors.pybaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/cancellation.test.tsbaml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rsbaml_language/sdks/nodejs/bridge_nodejs/src/errors.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.tsbaml_language/sdks/python/rust/bridge_python/src/abort_controller.rsbaml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/__init__.pyibaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/src/baml_core/__init__.pybaml_language/sdks/python/src/baml_core/_stream.pybaml_language/sdks/python/src/baml_core/baml_py.pyibaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pyibaml_language/sdks/python/src/baml_core/errors.pybaml_language/sdks/python/src/baml_core/proto.py
|
|
||
| The shared infrastructure is split into two crates so the heavy | ||
| codegen + project-loading deps only land where they're needed: | ||
| > SDK tests are designed to be run using `cargo nextest run` and will > fail in |
Contributor
There was a problem hiding this comment.
Fix blockquote formatting error.
Line 21 contains an extraneous > character in the middle of the sentence: will > fail in. This should be will fail in. The extra > breaks the blockquote formatting and reads as if it's comparing values rather than describing test behavior.
📝 Proposed fix
-> SDK tests are designed to be run using `cargo nextest run` and will > fail in
+> SDK tests are designed to be run using `cargo nextest run` and will fail in
> surprising ways if run using `cargo test`. Specifically, `cargo nextest run`📝 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
| > SDK tests are designed to be run using `cargo nextest run` and will > fail in | |
| > SDK tests are designed to be run using `cargo nextest run` and will fail in | |
| > surprising ways if run using `cargo test`. Specifically, `cargo nextest run` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdk_tests/README.md` at line 21, Remove the stray ">" in the
README sentence fragment so the line reads "SDK tests are designed to be run
using `cargo nextest run` and will fail in" (i.e., change `will > fail in` to
`will fail in`) to restore proper blockquote/markdown formatting; update the
README.md sentence accordingly and verify the surrounding blockquote markers
still render as intended.
Comment on lines
+148
to
+156
| try { | ||
| resultBytes = rt.callFunctionSync(bamlFqn, argsProto, null, null); | ||
| } finally { | ||
| abortBinding.detach(); | ||
| } | ||
| try { | ||
| return decodeCallResult(resultBytes); | ||
| } catch (err) { | ||
| throwWithCapturedAbortReason(err, abortBinding.reason()); |
Contributor
There was a problem hiding this comment.
Wrap native bridge throws in all generated call paths.
These four rt.callFunction* sites bypass wrapNativeError, so any N-API failure here leaks as a raw native Error instead of the SDK's Baml*Error subclasses. That makes generated functions behave differently from callFunction* in index.ts.
Suggested fix
-import { BamlAbortError } from './errors.js';
+import { BamlAbortError, wrapNativeError } from './errors.js';
...
let resultBytes: Buffer;
try {
resultBytes = rt.callFunctionSync(bamlFqn, argsProto, null, null);
+ } catch (err) {
+ throw wrapNativeError(err);
} finally {
abortBinding.detach();
}
...
let resultBytes: Buffer;
try {
resultBytes = await rt.callFunction(bamlFqn, argsProto, null, null);
+ } catch (err) {
+ throw wrapNativeError(err);
} finally {
abortBinding.detach();
}
...
let resultBytes: Buffer;
try {
resultBytes = rt.callFunctionSync(bamlFqn, argsProto, null, null);
+ } catch (err) {
+ throw wrapNativeError(err);
} finally {
abortBinding.detach();
}
...
let resultBytes: Buffer;
try {
resultBytes = await rt.callFunction(bamlFqn, argsProto, null, null);
+ } catch (err) {
+ throw wrapNativeError(err);
} finally {
abortBinding.detach();
}Also applies to: 168-176, 217-226, 237-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.ts`
around lines 148 - 156, Generated call paths call the native runtime
(rt.callFunctionSync / rt.callFunctionAsync / rt.callFunction... sites) and
currently let native N-API throws escape without conversion; wrap the native
calls so any thrown error is passed through wrapNativeError before rethrowing
(preserve the existing abortBinding.detach/finally logic and then call
decodeCallResult and throwWithCapturedAbortReason as before), i.e., catch errors
thrown by rt.callFunction* calls, call wrapNativeError(err) and rethrow the
result so all generated functions produce Baml*Error subclasses like index.ts.
Comment on lines
81
to
+83
| export function wrapNativeError(err: unknown): BamlError { | ||
| if (!(err instanceof Error)) return new BamlError(String(err)); | ||
| const msg = err.message; | ||
| for (const [prefix, Ctor] of PREFIX_MAP) { | ||
| if (msg.startsWith(prefix)) return new Ctor(msg); | ||
| } | ||
| return new BamlError(msg); | ||
| return new BamlError(err.message); |
Contributor
There was a problem hiding this comment.
Restore typed native-error translation in wrapNativeError.
This helper now collapses every bridge Error into plain BamlError, but the call sites in index.ts still rely on it to surface specific Baml*Error subclasses. Invalid call IDs and other native bridge failures will lose their public error type.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.ts` around
lines 81 - 83, wrapNativeError currently converts any native Error into a plain
BamlError, which strips subclass types like BamlInvalidCallError expected by
callers; change wrapNativeError to preserve and rethrow native errors that are
instances of BamlError or any of its subclasses (e.g., BamlInvalidCallError)
instead of always wrapping them, while still converting non-Error values to new
BamlError(String(err)). Locate the wrapNativeError function and modify its logic
to return the original error if err instanceof BamlError (or otherwise preserve
subclass identity), and only construct new BamlError for non-Error/unknown
values so index.ts call sites receive the correct typed Baml*Error subclasses.
Comment on lines
57
to
67
| private _callSync(fqn: string): unknown { | ||
| const rt = getRuntime(); | ||
| const argsProto = encodeCallArgs({ self: this }, /* syncMode */ true); | ||
| const resultBytes = rt.callFunctionSync(fqn, argsProto, null, null, null); | ||
| const argsProto = encodeCallArgs({ self: this }, { syncMode: true, callId: newFunctionCall() }); | ||
| const resultBytes = rt.callFunctionSync(fqn, argsProto, null, null); | ||
| return decodeCallResult(resultBytes); | ||
| } | ||
| private async _callAsync(fqn: string): Promise<unknown> { | ||
| const rt = getRuntime(); | ||
| const argsProto = encodeCallArgs({ self: this }); | ||
| const resultBytes = await rt.callFunction(fqn, argsProto, null, null, null); | ||
| const argsProto = encodeCallArgs({ self: this }, { callId: newFunctionCall() }); | ||
| const resultBytes = await rt.callFunction(fqn, argsProto, null, null); | ||
| return decodeCallResult(resultBytes); |
Contributor
There was a problem hiding this comment.
Translate native stream-call failures before decoding.
BamlStream.next() and final() still call the bridge directly, so native failures bypass wrapNativeError and escape as raw Errors. That leaves stream consumers with different error types than the rest of the Node SDK.
Suggested fix
import { BamlHandle, getRuntime, newFunctionCall as nativeNewFunctionCall } from './native.js';
+import { wrapNativeError } from './errors.js';
import { encodeCallArgs, decodeCallResult } from './proto.js';
...
private _callSync(fqn: string): unknown {
const rt = getRuntime();
const argsProto = encodeCallArgs({ self: this }, { syncMode: true, callId: newFunctionCall() });
- const resultBytes = rt.callFunctionSync(fqn, argsProto, null, null);
+ let resultBytes: Buffer;
+ try {
+ resultBytes = rt.callFunctionSync(fqn, argsProto, null, null);
+ } catch (err) {
+ throw wrapNativeError(err);
+ }
return decodeCallResult(resultBytes);
}
private async _callAsync(fqn: string): Promise<unknown> {
const rt = getRuntime();
const argsProto = encodeCallArgs({ self: this }, { callId: newFunctionCall() });
- const resultBytes = await rt.callFunction(fqn, argsProto, null, null);
+ let resultBytes: Buffer;
+ try {
+ resultBytes = await rt.callFunction(fqn, argsProto, null, null);
+ } catch (err) {
+ throw wrapNativeError(err);
+ }
return decodeCallResult(resultBytes);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.ts` around
lines 57 - 67, The native bridge calls in _callSync and _callAsync can throw raw
Errors that bypass wrapNativeError; wrap the calls to rt.callFunctionSync (in
_callSync) and await rt.callFunction (in _callAsync) in try/catch blocks, and on
any caught error call wrapNativeError(err) and rethrow the wrapped error before
proceeding to decodeCallResult; keep the surrounding logic (getRuntime,
encodeCallArgs, decodeCallResult) unchanged so only the native-call invocation
is translated to use wrapNativeError.
Comment on lines
85
to
93
| async def _call_async(self, fqn: str) -> Any: | ||
| from . import get_runtime | ||
| from .baml_py import new_function_call | ||
| from .proto import decode_call_result, encode_call_args | ||
|
|
||
| rt = get_runtime() | ||
| args_proto = encode_call_args({"self": self}) | ||
| result_bytes = await rt.call_function(fqn, args_proto, None, None, None) | ||
| args_proto = encode_call_args({"self": self}, new_function_call()) | ||
| result_bytes = await rt.call_function(fqn, args_proto, None, None) | ||
| return decode_call_result(result_bytes) |
Contributor
There was a problem hiding this comment.
Propagate task cancellation for async stream calls.
_call_async() allocates a call_id inline and then discards it before awaiting rt.call_function(...). If next_async() / final_async() is cancelled, nothing calls cancel_function_call(call_id), so the bridge keeps the stream step running after the Python task is gone.
Suggested fix
async def _call_async(self, fqn: str) -> Any:
from . import get_runtime
- from .baml_py import new_function_call
+ from .baml_py import cancel_function_call, new_function_call
from .proto import decode_call_result, encode_call_args
+ import asyncio
rt = get_runtime()
- args_proto = encode_call_args({"self": self}, new_function_call())
- result_bytes = await rt.call_function(fqn, args_proto, None, None)
+ call_id = new_function_call()
+ args_proto = encode_call_args({"self": self}, call_id)
+ try:
+ result_bytes = await rt.call_function(fqn, args_proto, None, None)
+ except asyncio.CancelledError:
+ cancel_function_call(call_id)
+ raise
return decode_call_result(result_bytes)📝 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
| async def _call_async(self, fqn: str) -> Any: | |
| from . import get_runtime | |
| from .baml_py import new_function_call | |
| from .proto import decode_call_result, encode_call_args | |
| rt = get_runtime() | |
| args_proto = encode_call_args({"self": self}) | |
| result_bytes = await rt.call_function(fqn, args_proto, None, None, None) | |
| args_proto = encode_call_args({"self": self}, new_function_call()) | |
| result_bytes = await rt.call_function(fqn, args_proto, None, None) | |
| return decode_call_result(result_bytes) | |
| async def _call_async(self, fqn: str) -> Any: | |
| from . import get_runtime | |
| from .baml_py import cancel_function_call, new_function_call | |
| from .proto import decode_call_result, encode_call_args | |
| import asyncio | |
| rt = get_runtime() | |
| call_id = new_function_call() | |
| args_proto = encode_call_args({"self": self}, call_id) | |
| try: | |
| result_bytes = await rt.call_function(fqn, args_proto, None, None) | |
| except asyncio.CancelledError: | |
| cancel_function_call(call_id) | |
| raise | |
| return decode_call_result(result_bytes) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/python/src/baml_core/_stream.py` around lines 85 - 93, The
async helper _call_async currently creates a function call proto inline and
awaits rt.call_function without retaining the generated call_id, so
cancellations never invoke cancel_function_call; change _call_async to extract
the call_id from new_function_call() (or the encoded args) before awaiting, then
wrap the await in try/finally (or catch asyncio.CancelledError) and on
cancellation call rt.cancel_function_call(call_id); ensure you still decode the
result with decode_call_result and preserve uses of get_runtime,
encode_call_args and call_function.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/sdks/nodejs/bridge_nodejs/src/runtime.rs`:
- Around line 117-150: The current code constructs the async call context (via
bridge_cffi::function_call_context_builder(decoded.call_id) and
call_ctx.build()) before calling env.spawn_future, which consumes
PENDING_CANCELS too early and can miss cancels that arrive after call_function()
returns; move the creation of the call context (the
function_call_context_builder, optional with_host_ctx, and call_ctx.build())
into the async closure inside env.spawn_future so that pending cancels arriving
between call_function() and the spawned task start are observed before calling
bridge_cffi::call_and_encode; keep decode_args and runtime acquisition outside
if desired but ensure call_ctx is built inside the async move block where
call_and_encode is awaited.
🪄 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: 18e000e3-b63b-49a8-b82e-7d9fb68b5ec7
⛔ Files ignored due to path filters (41)
baml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/ctx_manager.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/errors.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/errors.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/errors.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/errors.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/exit_hook.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/native.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/native.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/stream.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/typemap.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/typemap.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/typemap.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/typemap.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (31)
baml_language/crates/bridge_cffi/src/baml_to_host.rsbaml_language/crates/bridge_cffi/src/error.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.protobaml_language/crates/bridge_wasm/tests/host_callable.rsbaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_cancellation.pybaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_errors.pybaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/cancellation.test.tsbaml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rsbaml_language/sdks/nodejs/bridge_nodejs/src/errors.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.tsbaml_language/sdks/python/rust/bridge_python/src/abort_controller.rsbaml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/__init__.pyibaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/src/baml_core/__init__.pybaml_language/sdks/python/src/baml_core/_stream.pybaml_language/sdks/python/src/baml_core/baml_py.pyibaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pyibaml_language/sdks/python/src/baml_core/errors.pybaml_language/sdks/python/src/baml_core/proto.py
✅ Files skipped from review due to trivial changes (3)
- baml_language/crates/bridge_cffi/src/baml_to_host.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.ts
- baml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.py
🚧 Files skipped from review as they are similar to previous changes (25)
- baml_language/crates/bridge_wasm/tests/host_callable.rs
- baml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.proto
- baml_language/sdks/nodejs/bridge_nodejs/src/lib.rs
- baml_language/sdks/python/src/baml_core/_stream.py
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.ts
- baml_language/sdks/python/rust/bridge_python/src/lib.rs
- baml_language/crates/bridge_cffi/src/error.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
- baml_language/sdks/python/src/baml_core/proto.py
- baml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pyi
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.ts
- baml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/init.pyi
- baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_errors.py
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.ts
- baml_language/sdks/python/src/baml_core/baml_py.pyi
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.ts
- baml_language/sdks/python/rust/bridge_python/src/abort_controller.rs
- baml_language/sdk_tests/crates/typescript_node/function_calls/customizable/cancellation.test.ts
- baml_language/crates/bridge_cffi/src/lib.rs
- baml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rs
- baml_language/sdks/python/src/baml_core/init.py
- baml_language/sdks/python/rust/bridge_python/src/runtime.rs
- baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_cancellation.py
- baml_language/sdks/python/src/baml_core/errors.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_errors.py (1)
87-89: ⚡ Quick winRename this test to match the new cancellation contract.
The implementation now verifies
asyncio.CancelledError, but the test name still says “as_baml_panic”, which makes failure output confusing.Suggested patch
-async def test_cancellation_surfaces_as_baml_panic(): +async def test_cancellation_surfaces_as_cancelled_error():🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_errors.py` around lines 87 - 89, Rename the test function test_cancellation_surfaces_as_baml_panic to reflect the new contract (e.g. test_cancellation_surfaces_as_asyncio_cancellederror) so the name matches the implementation that asserts asyncio.CancelledError; update the function definition and any direct references to that function (e.g. in test discovery or markers) to use the new name, leaving the test body (rt = get_runtime(), the cancellation/assertion logic) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 222-239: reserve_cancelled currently inserts a permanent "pending"
tombstone into BexEngine.active_calls for any unknown CallId, causing unbounded
growth; change reserve_cancelled (and related ActiveCall usage) to not create a
persistent pending entry for unknown IDs — either (A) only mark pending if the
CallId is known to have been allocated (track allocated IDs or check existing
registration before inserting), or (B) replace the boolean pending with a
timestamped tombstone and add bounded cleanup (periodic sweeper or TTL check
when accessing active_calls) so orphaned pending entries expire; update
references to ActiveCall, reserve_cancelled, and any consumers of active_calls
to handle the absence of a tombstone or the expiry semantics.
In `@baml_language/crates/bridge_cffi/src/lib.rs`:
- Around line 285-290: Update the function comment for cancel_function_call to
reflect the new semantics: state that the function returns 0 on success and 1 on
failure (e.g., when id == 0 or another error occurs), and explicitly document
that once the runtime is initialized, cancellations for any nonzero id will be
accepted (late-cancel / pre-cancel behavior) even if the call was not previously
known or is already completed; reference the exported function name
cancel_function_call and the helper cancel_function_call_by_id so the updated
contract is adjacent to their definitions.
In
`@baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_cancellation.py`:
- Around line 22-32: Replace the hard-coded _MAX_CANCELLATION_SECONDS = 0.5 with
a configurable threshold read from an environment variable (e.g.,
BAMl_FAST_CANCELLATION_SECONDS) and a sensible higher default (e.g., 2.0s);
update the constant definition to parse the env var to float with a fallback
default and adjust _assert_fast_cancellation to continue using that symbol so
tests use the configurable deadline instead of a fixed 0.5s.
In
`@baml_language/sdk_tests/crates/typescript_node/function_calls/customizable/cancellation.test.ts`:
- Around line 12-26: The fixed 500ms SLA in cancellation.test.ts
(MAX_CANCELLATION_MS and expectFastCancellation) is too strict for CI; make the
threshold configurable and raise the safe default to reduce flakiness by
changing MAX_CANCELLATION_MS to read from an environment variable (e.g.,
process.env.TEST_CANCELLATION_MS) or a test-config with a higher default (e.g.,
2000ms), and update expectFastCancellation to use that configurable value (or
accept an optional threshold parameter) so tests use the env/config value
instead of the hard-coded 500ms.
In `@baml_language/sdks/nodejs/bridge_nodejs/src/baml_call_context.rs`:
- Around line 45-60: In attach_call_id, reject a parsed call_id of 0 instead of
storing it: in the function attach_call_id(parse of call_id) check if call_id ==
0 and return a napi::Error (e.g. Status::InvalidArg with a message like "callId
must be a non-zero decimal uint64 string"); only proceed to lock and push into
active_call_ids when call_id != 0, and keep the existing aborted() branch that
calls bridge_cffi::cancel_function_call_by_id(call_id) (which will never be
invoked for the rejected zero), ensuring no zero IDs are stored or passed to
cancel_function_call_by_id.
In `@baml_language/sdks/python/rust/bridge_python/src/baml_call_context.rs`:
- Around line 13-15: The doc example for call_function / call_function_sync
passes the wrong keyword parameter; update the example so the call-context is
bound via the _ctx keyword (e.g., use _ctx=ctx) instead of ctx=ctx in the
comment block that shows "result = await call_function(rt, "MyFunc", args,
ctx=ctx)"; ensure both asynchronous and sync examples within the same comment
(the lines around "call_function" and "call_function_sync") use _ctx
consistently so users see the correct parameter name.
- Around line 1-80: The tests failed because you ran `cargo test --lib` from the
repo root instead of the crate workspace; re-run the tests from the
bridge_python crate workspace (the directory that contains that crate’s
Cargo.toml) so Cargo can find the manifest and execute the unit tests for
BamlCallContext and related items; alternatively, invoke cargo with
--manifest-path pointing at that crate’s Cargo.toml to run `cargo test --lib`
for the bridge_python crate and verify tests pass for functions like
BamlCallContext::new, abort, _attach_call_id, and _detach_call_id.
---
Nitpick comments:
In
`@baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_errors.py`:
- Around line 87-89: Rename the test function
test_cancellation_surfaces_as_baml_panic to reflect the new contract (e.g.
test_cancellation_surfaces_as_asyncio_cancellederror) so the name matches the
implementation that asserts asyncio.CancelledError; update the function
definition and any direct references to that function (e.g. in test discovery or
markers) to use the new name, leaving the test body (rt = get_runtime(), the
cancellation/assertion logic) unchanged.
🪄 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: e52d040c-cc77-4909-aba2-0d1f6b06ca1a
⛔ Files ignored due to path filters (23)
baml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/errors.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/errors.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/errors.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/errors.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/native.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/native.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/stream.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (37)
.gitignorebaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/cancellation.rsbaml_language/crates/bridge_cffi/src/baml_to_host.rsbaml_language/crates/bridge_cffi/src/error.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.protobaml_language/crates/bridge_wasm/tests/host_callable.rsbaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_cancellation.pybaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_errors.pybaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/cancellation.test.tsbaml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rsbaml_language/sdks/nodejs/bridge_nodejs/src/baml_call_context.rsbaml_language/sdks/nodejs/bridge_nodejs/src/errors.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.tsbaml_language/sdks/python/rust/bridge_python/src/abort_controller.rsbaml_language/sdks/python/rust/bridge_python/src/baml_call_context.rsbaml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/__init__.pyibaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/src/baml_core/__init__.pybaml_language/sdks/python/src/baml_core/_stream.pybaml_language/sdks/python/src/baml_core/baml_py.pyibaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pyibaml_language/sdks/python/src/baml_core/errors.pybaml_language/sdks/python/src/baml_core/proto.py
💤 Files with no reviewable changes (2)
- baml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rs
- baml_language/sdks/python/rust/bridge_python/src/abort_controller.rs
✅ Files skipped from review due to trivial changes (1)
- baml_language/crates/bridge_cffi/src/baml_to_host.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- baml_language/crates/bridge_wasm/tests/host_callable.rs
- baml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.proto
- baml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.py
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.ts
- baml_language/sdks/nodejs/bridge_nodejs/src/lib.rs
- baml_language/sdks/python/src/baml_core/_stream.py
- baml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pyi
- .gitignore
- baml_language/sdks/python/src/baml_core/proto.py
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.ts
- baml_language/sdks/python/src/baml_core/errors.py
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.ts
- baml_language/sdks/nodejs/bridge_nodejs/src/runtime.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
- baml_language/sdks/python/rust/bridge_python/src/runtime.rs
Comment on lines
+222
to
+239
| fn reserve_cancelled(engine: &BexEngine, call_id: CallId) { | ||
| let mut map = engine | ||
| .active_calls | ||
| .lock() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| match map.get(&call_id) { | ||
| Some(existing) => existing.cancel.cancel(), | ||
| None => { | ||
| let cancel = CancellationToken::new(); | ||
| cancel.cancel(); | ||
| map.insert( | ||
| call_id, | ||
| ActiveCall { | ||
| cancel, | ||
| pending: true, | ||
| }, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Pending-cancel tombstones never expire.
reserve_cancelled inserts a pending entry for any unknown call_id, and nothing removes that entry unless a future call reuses the same ID. With the new bridge/SDK flow generating fresh IDs, and Line 1694 treating late cancels as success, every cancel on a completed or guessed ID can leave a permanent active_calls entry behind. Repeated stale cancels will grow this map without bound.
Reserve only IDs that were actually allocated-but-not-yet-registered, or add bounded cleanup for orphaned pending entries instead of storing them indefinitely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/crates/bex_engine/src/lib.rs` around lines 222 - 239,
reserve_cancelled currently inserts a permanent "pending" tombstone into
BexEngine.active_calls for any unknown CallId, causing unbounded growth; change
reserve_cancelled (and related ActiveCall usage) to not create a persistent
pending entry for unknown IDs — either (A) only mark pending if the CallId is
known to have been allocated (track allocated IDs or check existing registration
before inserting), or (B) replace the boolean pending with a timestamped
tombstone and add bounded cleanup (periodic sweeper or TTL check when accessing
active_calls) so orphaned pending entries expire; update references to
ActiveCall, reserve_cancelled, and any consumers of active_calls to handle the
absence of a tombstone or the expiry semantics.
Comment on lines
285
to
+290
| /// Cancel an in-flight function call. | ||
| /// | ||
| /// Returns 0 on success, 1 if the call ID is unknown or already completed. | ||
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn cancel_function_call(id: u32) -> i32 { | ||
| let runtime = match get_runtime() { | ||
| Ok(rt) => rt, | ||
| Err(_) => return 1, | ||
| }; | ||
| match runtime.cancel_function_call(sys_types::CallId(id.into())) { | ||
| Ok(()) => 0, | ||
| Err(_) => 1, | ||
| } | ||
| pub extern "C" fn cancel_function_call(id: u64) -> i32 { | ||
| if cancel_function_call_by_id(id) { 0 } else { 1 } |
Contributor
There was a problem hiding this comment.
Update the exported cancel contract comment.
This comment still says cancel_function_call returns 1 when the call ID is unknown or already completed, but the new implementation now succeeds for any nonzero ID once the runtime is initialized. External callers reading this C-ABI contract will branch on the wrong result.
Please update the comment here to describe the new late-cancel / pre-cancel behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/crates/bridge_cffi/src/lib.rs` around lines 285 - 290, Update
the function comment for cancel_function_call to reflect the new semantics:
state that the function returns 0 on success and 1 on failure (e.g., when id ==
0 or another error occurs), and explicitly document that once the runtime is
initialized, cancellations for any nonzero id will be accepted (late-cancel /
pre-cancel behavior) even if the call was not previously known or is already
completed; reference the exported function name cancel_function_call and the
helper cancel_function_call_by_id so the updated contract is adjacent to their
definitions.
Comment on lines
+22
to
+32
| _MAX_CANCELLATION_SECONDS = 0.5 | ||
|
|
||
|
|
||
| def _assert_cancelled_panic(exc: BamlPanic) -> None: | ||
| assert isinstance(exc.value, Cancelled) | ||
|
|
||
|
|
||
| def _assert_fast_cancellation(start: float) -> None: | ||
| elapsed = time.monotonic() - start | ||
| assert elapsed < _MAX_CANCELLATION_SECONDS, f"cancellation took {elapsed:.3f}s" | ||
|
|
Contributor
There was a problem hiding this comment.
Use a configurable cancellation deadline instead of a fixed 0.5s bound.
A strict global timing cap can make these tests flaky on noisy CI hosts. Prefer an environment-overridable threshold with a higher default.
Suggested patch
import asyncio
+import os
import threading
import time
@@
-_MAX_CANCELLATION_SECONDS = 0.5
+_MAX_CANCELLATION_SECONDS = float(
+ os.getenv("BAML_TEST_MAX_CANCELLATION_SECONDS", "1.5")
+)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_cancellation.py`
around lines 22 - 32, Replace the hard-coded _MAX_CANCELLATION_SECONDS = 0.5
with a configurable threshold read from an environment variable (e.g.,
BAMl_FAST_CANCELLATION_SECONDS) and a sensible higher default (e.g., 2.0s);
update the constant definition to parse the env var to float with a fallback
default and adjust _assert_fast_cancellation to continue using that symbol so
tests use the configurable deadline instead of a fixed 0.5s.
Comment on lines
+12
to
+26
| const MAX_CANCELLATION_MS = 500; | ||
|
|
||
| function expectAbortError(error: unknown): void { | ||
| expect(error).toBeInstanceOf(Error); | ||
| expect((error as Error).name).toBe("AbortError"); | ||
| } | ||
|
|
||
| function expectBamlCancelledReason(error: unknown): void { | ||
| expectAbortError(error); | ||
| expect((error as { reason?: unknown }).reason).toBeInstanceOf(BamlCancelledError); | ||
| } | ||
|
|
||
| function expectFastCancellation(start: number): void { | ||
| expect(performance.now() - start).toBeLessThan(MAX_CANCELLATION_MS); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid a fixed 500ms cancellation SLA in tests to reduce CI flakiness.
The strict wall-clock cutoff can fail on slower runners even when cancellation behavior is correct. Make the threshold configurable (with a safer default) so the suite remains stable across environments.
Suggested patch
+const DEFAULT_MAX_CANCELLATION_MS = 1500;
const SLEEP_FQN = "user.throws_test.SleepMs";
-const MAX_CANCELLATION_MS = 500;
+const MAX_CANCELLATION_MS = Number(
+ process.env.BAML_TEST_MAX_CANCELLATION_MS ?? DEFAULT_MAX_CANCELLATION_MS,
+);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@baml_language/sdk_tests/crates/typescript_node/function_calls/customizable/cancellation.test.ts`
around lines 12 - 26, The fixed 500ms SLA in cancellation.test.ts
(MAX_CANCELLATION_MS and expectFastCancellation) is too strict for CI; make the
threshold configurable and raise the safe default to reduce flakiness by
changing MAX_CANCELLATION_MS to read from an environment variable (e.g.,
process.env.TEST_CANCELLATION_MS) or a test-config with a higher default (e.g.,
2000ms), and update expectFastCancellation to use that configurable value (or
accept an optional threshold parameter) so tests use the env/config value
instead of the hard-coded 500ms.
Comment on lines
+45
to
+60
| pub fn attach_call_id(&self, call_id: String) -> napi::Result<()> { | ||
| let call_id = call_id.parse::<u64>().map_err(|_| { | ||
| napi::Error::new( | ||
| napi::Status::InvalidArg, | ||
| "callId must be a decimal uint64 string", | ||
| ) | ||
| })?; | ||
| if let Ok(mut ids) = self.active_call_ids.lock() | ||
| && !ids.contains(&call_id) | ||
| { | ||
| ids.push(call_id); | ||
| } | ||
| if self.aborted() { | ||
| bridge_cffi::cancel_function_call_by_id(call_id); | ||
| } | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
Reject callId == "0" at attach time.
_attachCallId currently accepts "0" and stores it in active_call_ids, but every downstream bridge path treats 0 as invalid and cancel_function_call_by_id(0) is a no-op. That makes abort() silently fail for an attached ID that can never be cancelled.
Suggested fix
let call_id = call_id.parse::<u64>().map_err(|_| {
napi::Error::new(
napi::Status::InvalidArg,
"callId must be a decimal uint64 string",
)
})?;
+ if call_id == 0 {
+ return Err(napi::Error::new(
+ napi::Status::InvalidArg,
+ "callId must be a nonzero uint64 string",
+ ));
+ }
if let Ok(mut ids) = self.active_call_ids.lock()
&& !ids.contains(&call_id)
{📝 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
| pub fn attach_call_id(&self, call_id: String) -> napi::Result<()> { | |
| let call_id = call_id.parse::<u64>().map_err(|_| { | |
| napi::Error::new( | |
| napi::Status::InvalidArg, | |
| "callId must be a decimal uint64 string", | |
| ) | |
| })?; | |
| if let Ok(mut ids) = self.active_call_ids.lock() | |
| && !ids.contains(&call_id) | |
| { | |
| ids.push(call_id); | |
| } | |
| if self.aborted() { | |
| bridge_cffi::cancel_function_call_by_id(call_id); | |
| } | |
| Ok(()) | |
| pub fn attach_call_id(&self, call_id: String) -> napi::Result<()> { | |
| let call_id = call_id.parse::<u64>().map_err(|_| { | |
| napi::Error::new( | |
| napi::Status::InvalidArg, | |
| "callId must be a decimal uint64 string", | |
| ) | |
| })?; | |
| if call_id == 0 { | |
| return Err(napi::Error::new( | |
| napi::Status::InvalidArg, | |
| "callId must be a nonzero uint64 string", | |
| )); | |
| } | |
| if let Ok(mut ids) = self.active_call_ids.lock() | |
| && !ids.contains(&call_id) | |
| { | |
| ids.push(call_id); | |
| } | |
| if self.aborted() { | |
| bridge_cffi::cancel_function_call_by_id(call_id); | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/nodejs/bridge_nodejs/src/baml_call_context.rs` around
lines 45 - 60, In attach_call_id, reject a parsed call_id of 0 instead of
storing it: in the function attach_call_id(parse of call_id) check if call_id ==
0 and return a napi::Error (e.g. Status::InvalidArg with a message like "callId
must be a non-zero decimal uint64 string"); only proceed to lock and push into
active_call_ids when call_id != 0, and keep the existing aborted() branch that
calls bridge_cffi::cancel_function_call_by_id(call_id) (which will never be
invoked for the rejected zero), ensuring no zero IDs are stored or passed to
cancel_function_call_by_id.
Comment on lines
+1
to
+80
| //! Python `BamlCallContext` class for cancelling in-flight BAML function calls. | ||
|
|
||
| use std::sync::Mutex; | ||
|
|
||
| use pyo3::{prelude::pymethods, pyclass}; | ||
| use pyo3_stub_gen::derive::{gen_stub_pyclass, gen_stub_pymethods}; | ||
|
|
||
| /// A call context for cancelling BAML function calls. | ||
| /// | ||
| /// Usage from Python: | ||
| /// ```python | ||
| /// ctx = BamlCallContext() | ||
| /// # Pass to call_function / call_function_sync: | ||
| /// result = await call_function(rt, "MyFunc", args, ctx=ctx) | ||
| /// # Cancel from another task: | ||
| /// ctx.abort() | ||
| /// ``` | ||
| #[gen_stub_pyclass] | ||
| #[pyclass] | ||
| pub struct BamlCallContext { | ||
| aborted: Mutex<bool>, | ||
| active_call_ids: Mutex<Vec<u64>>, | ||
| } | ||
|
|
||
| #[gen_stub_pymethods] | ||
| #[pymethods] | ||
| impl BamlCallContext { | ||
| #[new] | ||
| fn new() -> Self { | ||
| Self { | ||
| aborted: Mutex::new(false), | ||
| active_call_ids: Mutex::new(Vec::new()), | ||
| } | ||
| } | ||
|
|
||
| /// Cancel the associated function call. | ||
| /// | ||
| /// If the function is still running, it will be interrupted at the next | ||
| /// cancellation check point (before HTTP calls, between retries, etc.). | ||
| /// Calling `abort()` multiple times is harmless. | ||
| fn abort(&self) { | ||
| if let Ok(mut aborted) = self.aborted.lock() { | ||
| *aborted = true; | ||
| } | ||
| let call_ids = self | ||
| .active_call_ids | ||
| .lock() | ||
| .map(|ids| ids.clone()) | ||
| .unwrap_or_default(); | ||
| for call_id in call_ids { | ||
| bridge_cffi::cancel_function_call_by_id(call_id); | ||
| } | ||
| } | ||
|
|
||
| /// Whether `abort()` has been called. | ||
| #[getter] | ||
| fn aborted(&self) -> bool { | ||
| self.aborted.lock().map(|aborted| *aborted).unwrap_or(true) | ||
| } | ||
|
|
||
| /// Bind this controller to an in-flight CFFI call id for the duration of | ||
| /// one host call. Private runtime hook used by `baml_core`. | ||
| fn _attach_call_id(&self, call_id: u64) { | ||
| if let Ok(mut ids) = self.active_call_ids.lock() | ||
| && !ids.contains(&call_id) | ||
| { | ||
| ids.push(call_id); | ||
| } | ||
| if self.aborted() { | ||
| bridge_cffi::cancel_function_call_by_id(call_id); | ||
| } | ||
| } | ||
|
|
||
| /// Remove a call-id binding installed by `_attach_call_id`. | ||
| fn _detach_call_id(&self, call_id: u64) { | ||
| if let Ok(mut ids) = self.active_call_ids.lock() { | ||
| ids.retain(|id| *id != call_id); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cargo test --libRepository: BoundaryML/baml
Length of output: 318
Run cargo test --lib from the correct Rust workspace (tests didn’t start)
The required cargo test --lib command fails immediately because Cargo.toml isn’t found in the current working directory (repo root). Run cargo test --lib from the directory/workspace that contains the Cargo.toml for baml_language/sdks/python/rust/bridge_python so the Rust unit tests actually execute and pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/python/rust/bridge_python/src/baml_call_context.rs` around
lines 1 - 80, The tests failed because you ran `cargo test --lib` from the repo
root instead of the crate workspace; re-run the tests from the bridge_python
crate workspace (the directory that contains that crate’s Cargo.toml) so Cargo
can find the manifest and execute the unit tests for BamlCallContext and related
items; alternatively, invoke cargo with --manifest-path pointing at that crate’s
Cargo.toml to run `cargo test --lib` for the bridge_python crate and verify
tests pass for functions like BamlCallContext::new, abort, _attach_call_id, and
_detach_call_id.
Source: Coding guidelines
Comment on lines
+13
to
+15
| /// # Pass to call_function / call_function_sync: | ||
| /// result = await call_function(rt, "MyFunc", args, ctx=ctx) | ||
| /// # Cancel from another task: |
Contributor
There was a problem hiding this comment.
Fix the doc example keyword for call context.
Line 14 uses ctx=ctx, but call-context binding is wired through _ctx; this example currently points users to the wrong parameter.
Suggested doc fix
-/// result = await call_function(rt, "MyFunc", args, ctx=ctx)
+/// result = await call_function(rt, "MyFunc", args, _ctx=ctx)📝 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
| /// # Pass to call_function / call_function_sync: | |
| /// result = await call_function(rt, "MyFunc", args, ctx=ctx) | |
| /// # Cancel from another task: | |
| /// # Pass to call_function / call_function_sync: | |
| /// result = await call_function(rt, "MyFunc", args, _ctx=ctx) | |
| /// # Cancel from another task: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/python/rust/bridge_python/src/baml_call_context.rs` around
lines 13 - 15, The doc example for call_function / call_function_sync passes the
wrong keyword parameter; update the example so the call-context is bound via the
_ctx keyword (e.g., use _ctx=ctx) instead of ctx=ctx in the comment block that
shows "result = await call_function(rt, "MyFunc", args, ctx=ctx)"; ensure both
asynchronous and sync examples within the same comment (the lines around
"call_function" and "call_function_sync") use _ctx consistently so users see the
correct parameter name.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Around line 191-193: The .gitignore currently unignores the entire dist tree
with "!baml_language/sdks/nodejs/bridge_nodejs/dist/**" but only ignores
top-level binaries with "baml_language/sdks/nodejs/bridge_nodejs/dist/*.node",
so nested .node files can still be committed; update the ignore rules by adding
a pattern that re-ignores nested native addons (e.g., add
"baml_language/sdks/nodejs/bridge_nodejs/dist/**/*.node" after the unignore) so
all .node files under the dist tree are ignored while keeping the rest of
dist/** unignored.
In `@baml_language/sdks/nodejs/bridge_nodejs/src/lib.rs`:
- Around line 47-55: The function cancel_function_call currently accepts callId
"0" and returns Ok(false); update cancel_function_call to treat id == 0 as
invalid input: after parsing call_id to u64 (in cancel_function_call) add a
check for id == 0 and return a napi::Error with Status::InvalidArg and a clear
message like "callId must be a non-zero decimal uint64 string" instead of
calling bridge_cffi::cancel_function_call_by_id for zero; keep the existing
parse error handling for non-numeric input.
🪄 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: ad3ccf43-e4ff-46a8-821d-7e0441ecf602
⛔ Files ignored due to path filters (23)
baml_language/sdks/go/bridge_go/cffi/proto/baml_core/cffi/v1/baml_inbound.pb.gois excluded by!**/*.pb.gobaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/errors.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/errors.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/errors.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/errors.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/native.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/native.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/stream.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/stream.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (37)
.gitignorebaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/cancellation.rsbaml_language/crates/bridge_cffi/src/baml_to_host.rsbaml_language/crates/bridge_cffi/src/error.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.protobaml_language/crates/bridge_wasm/tests/host_callable.rsbaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_cancellation.pybaml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_errors.pybaml_language/sdk_tests/crates/typescript_node/function_calls/customizable/cancellation.test.tsbaml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rsbaml_language/sdks/nodejs/bridge_nodejs/src/baml_call_context.rsbaml_language/sdks/nodejs/bridge_nodejs/src/errors.rsbaml_language/sdks/nodejs/bridge_nodejs/src/lib.rsbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.tsbaml_language/sdks/python/rust/bridge_python/src/abort_controller.rsbaml_language/sdks/python/rust/bridge_python/src/baml_call_context.rsbaml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/__init__.pyibaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/src/baml_core/__init__.pybaml_language/sdks/python/src/baml_core/_stream.pybaml_language/sdks/python/src/baml_core/baml_py.pyibaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pyibaml_language/sdks/python/src/baml_core/errors.pybaml_language/sdks/python/src/baml_core/proto.py
💤 Files with no reviewable changes (2)
- baml_language/sdks/python/rust/bridge_python/src/abort_controller.rs
- baml_language/sdks/nodejs/bridge_nodejs/src/abort_controller.rs
✅ Files skipped from review due to trivial changes (3)
- baml_language/crates/bridge_wasm/tests/host_callable.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.ts
- baml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.py
🚧 Files skipped from review as they are similar to previous changes (28)
- baml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pyi
- baml_language/crates/bridge_cffi/src/error.rs
- baml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.proto
- baml_language/sdk_tests/crates/typescript_node/function_calls/customizable/cancellation.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/src/baml_call_context.rs
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_engine.test.ts
- baml_language/sdks/python/src/baml_core/_stream.py
- baml_language/crates/bex_engine/tests/cancellation.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/native.d.ts
- baml_language/sdks/python/src/baml_core/errors.py
- baml_language/sdks/nodejs/bridge_nodejs/src/errors.rs
- baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_errors.py
- baml_language/crates/bridge_cffi/src/lib.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.js
- baml_language/sdks/nodejs/bridge_nodejs/src/runtime.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/stream.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/errors.ts
- baml_language/sdks/python/rust/bridge_python/src/baml_call_context.rs
- baml_language/sdks/python/rust/bridge_python/src/lib.rs
- baml_language/sdks/python/rust/bridge_python/src/baml_core/baml_py/init.pyi
- baml_language/sdks/python/src/baml_core/proto.py
- baml_language/sdks/python/src/baml_core/baml_py.pyi
- baml_language/crates/bex_engine/src/lib.rs
- baml_language/sdks/python/src/baml_core/init.py
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.ts
- baml_language/sdk_tests/crates/python_pydantic2/function_calls/customizable/test_cancellation.py
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.ts
Comment on lines
+191
to
+193
| !baml_language/sdks/nodejs/bridge_nodejs/dist/ | ||
| !baml_language/sdks/nodejs/bridge_nodejs/dist/** | ||
| baml_language/sdks/nodejs/bridge_nodejs/dist/*.node |
Contributor
There was a problem hiding this comment.
Re-ignore nested .node binaries under the unignored dist tree.
After !.../dist/**, only top-level dist/*.node is ignored; nested native addons can be accidentally committed.
Suggested fix
!baml_language/sdks/nodejs/bridge_nodejs/dist/
!baml_language/sdks/nodejs/bridge_nodejs/dist/**
-baml_language/sdks/nodejs/bridge_nodejs/dist/*.node
+baml_language/sdks/nodejs/bridge_nodejs/dist/**/*.node🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.gitignore around lines 191 - 193, The .gitignore currently unignores the
entire dist tree with "!baml_language/sdks/nodejs/bridge_nodejs/dist/**" but
only ignores top-level binaries with
"baml_language/sdks/nodejs/bridge_nodejs/dist/*.node", so nested .node files can
still be committed; update the ignore rules by adding a pattern that re-ignores
nested native addons (e.g., add
"baml_language/sdks/nodejs/bridge_nodejs/dist/**/*.node" after the unignore) so
all .node files under the dist tree are ignored while keeping the rest of
dist/** unignored.
Comment on lines
+47
to
+55
| pub fn cancel_function_call(call_id: String) -> napi::Result<bool> { | ||
| let id = call_id.parse::<u64>().map_err(|_| { | ||
| napi::Error::new( | ||
| napi::Status::InvalidArg, | ||
| "callId must be a decimal uint64 string", | ||
| ) | ||
| })?; | ||
| Ok(bridge_cffi::cancel_function_call_by_id(id)) | ||
| } |
Contributor
There was a problem hiding this comment.
Reject callId = "0" as invalid input.
0 is outside the call-id contract, but this path currently returns Ok(false), which masks a caller error as a normal “not cancelled” outcome.
Suggested fix
#[napi(js_name = "cancelFunctionCall")]
pub fn cancel_function_call(call_id: String) -> napi::Result<bool> {
let id = call_id.parse::<u64>().map_err(|_| {
napi::Error::new(
napi::Status::InvalidArg,
"callId must be a decimal uint64 string",
)
})?;
+ if id == 0 {
+ return Err(napi::Error::new(
+ napi::Status::InvalidArg,
+ "callId must be a nonzero uint64 string",
+ ));
+ }
Ok(bridge_cffi::cancel_function_call_by_id(id))
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/nodejs/bridge_nodejs/src/lib.rs` around lines 47 - 55, The
function cancel_function_call currently accepts callId "0" and returns
Ok(false); update cancel_function_call to treat id == 0 as invalid input: after
parsing call_id to u64 (in cancel_function_call) add a check for id == 0 and
return a napi::Error with Status::InvalidArg and a clear message like "callId
must be a non-zero decimal uint64 string" instead of calling
bridge_cffi::cancel_function_call_by_id for zero; keep the existing parse error
handling for non-numeric input.
This comment has been minimized.
This comment has been minimized.
This was referenced Jul 13, 2026
This branch was previously deployed
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
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
New Features
Bug Fixes
Documentation
Tests