fix: support response query filtering in safe mode#7441
Merged
bijin-bruno merged 4 commits intousebruno:mainfrom Apr 2, 2026
Merged
fix: support response query filtering in safe mode#7441bijin-bruno merged 4 commits intousebruno:mainfrom
bijin-bruno merged 4 commits intousebruno:mainfrom
Conversation
abhishek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe pull request adds support for passing callback functions as query arguments to the Changes
Sequence DiagramsequenceDiagram
participant VM as QuickJS VM
participant Shim as res() Shim
participant Host as Host Runtime
VM->>Shim: res(exprStr, callbackFunc)
Shim->>Shim: toHostQueryArg(vm, callbackFunc)
Note over Shim: Convert JS function to<br/>native callback
Shim->>Host: res(dumpedExpr, nativeCallback)
Host->>nativeCallback: invoke(item)
nativeCallback->>VM: callFunction(callbackFunc, item)
VM-->>nativeCallback: result
nativeCallback->>Host: return filtered result
Host-->>Shim: hostResult
Shim->>VM: marshallToVm(hostResult)
Shim-->>VM: response value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 1
🧹 Nitpick comments (1)
packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js (1)
76-77: Consider adding error propagation test.The
toHostQueryArgerror handling path (when the sandbox callback throws) isn't exercised. A test verifying error propagation would strengthen coverage.💡 Example test case
it('propagates errors from sandbox callbacks', () => { const response = Object.assign( () => { throw new Error('should not reach'); }, { status: 200, statusText: 'OK', headers: {}, body: {} } ); addBrunoResponseShimToContext(vm, response); const result = vm.evalCode(` res('items[?]', () => { throw new Error('callback error'); }) `); expect(result.error).toBeDefined(); const error = vm.dump(result.error); result.error.dispose(); expect(error.message).toBe('callback error'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js` around lines 76 - 77, Add a test in bruno-response.spec.js that exercises the toHostQueryArg error path by creating a Bruno response shim with a callback that throws and calling addBrunoResponseShimToContext(vm, response); evaluate code via vm.evalCode using a query like res('items[?]', () => { throw new Error('callback error'); }) and assert that result.error is defined, dump the error with vm.dump(result.error), dispose it, and expect the dumped error.message to equal the thrown message; this will verify error propagation from the sandbox callback (use the existing patterns around vm, addBrunoResponseShimToContext, vm.evalCode, vm.dump, and result.error.dispose()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js`:
- Around line 16-22: The tests currently only null out vm and module which leaks
QuickJS WASM resources; update the teardown handlers to explicitly free
resources before nulling: in afterEach, call vm.dispose() (or
vm.destroy()/vm.free() if that’s the API) guarded by a typeof check (e.g., if
(vm && typeof vm.dispose === 'function') vm.dispose()), then set vm = null;
similarly in afterAll call module.dispose()/module.destroy()/module.free()
guarded by a check before setting module = null; adjust the handlers that
reference vm and module to perform disposal first.
---
Nitpick comments:
In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js`:
- Around line 76-77: Add a test in bruno-response.spec.js that exercises the
toHostQueryArg error path by creating a Bruno response shim with a callback that
throws and calling addBrunoResponseShimToContext(vm, response); evaluate code
via vm.evalCode using a query like res('items[?]', () => { throw new
Error('callback error'); }) and assert that result.error is defined, dump the
error with vm.dump(result.error), dispose it, and expect the dumped
error.message to equal the thrown message; this will verify error propagation
from the sandbox callback (use the existing patterns around vm,
addBrunoResponseShimToContext, vm.evalCode, vm.dump, and
result.error.dispose()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5c91273-0f45-43f0-9102-5b2ad712877c
📒 Files selected for processing (3)
.gitignorepackages/bruno-js/src/sandbox/quickjs/shims/bruno-response.jspackages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js
…ndling - Introduced a new test suite for the bruno-response shim to validate query filtering and function callback behavior. - Refactored the `res` function to enhance argument handling, allowing for better marshalling of QuickJS function handles and other values. - Improved error handling within the response processing to ensure robust behavior during query execution.
abhishek-bruno
force-pushed
the
fix/safe-mode-response-query
branch
from
f50fe9a to
b731106
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js (1)
16-22:⚠️ Potential issue | 🟡 MinorDispose the QuickJS VM and module in teardown.
Lines 16-22 only clear the JS references. The QuickJS context and WASM module stay allocated until they are explicitly disposed, so repeated test runs will leak memory.
♻️ Suggested teardown change
afterEach(() => { - vm = null; + if (vm && typeof vm.dispose === 'function') { + vm.dispose(); + } + vm = null; }); afterAll(() => { - module = null; + if (module && typeof module.dispose === 'function') { + module.dispose(); + } + module = null; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js` around lines 16 - 22, The teardown currently just nulls vm and module, leaking the QuickJS context and WASM module; update the teardown so you call the QuickJS disposal methods (e.g., vm.dispose() in afterEach and module.dispose() in afterAll) with null-checks before setting vm = null and module = null; locate the vm and module variables in bruno-response.spec.js and replace the simple null assignments with guarded dispose calls to free the VM and WASM module resources.
🧹 Nitpick comments (2)
packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.js (1)
3-7: Use JSDoc fortoHostQueryArg().This helper carries a non-obvious borrowing contract, so please promote the banner comment to JSDoc on the function itself and include the synchronous-lifetime caveat there. As per coding guidelines, "Add JSDoc comments to abstractions for additional details".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.js` around lines 3 - 7, Convert the leading banner comment into a JSDoc block placed immediately above the toHostQueryArg function: document what the function does (marshal a QuickJS query argument to a host-compatible value), note that function handles are wrapped as native callbacks and other values are returned as-is, and explicitly state the borrowing contract and synchronous-lifetime caveat ("Safe because `@usebruno/query`'s get() invokes filters synchronously, so the borrowed arg handle is still valid"). Keep the existing details but format them as JSDoc on the toHostQueryArg declaration to satisfy the coding guideline.packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js (1)
24-76: Add a thrown-predicate regression test.The new shim's hardest branch is the error path when the sandbox predicate throws, but the suite only exercises successful filtering. A case that asserts
res(...)surfaces that failure would lock down the cross-VM degraded path that is easiest to regress. Based on learnings, "Cover both the 'happy path' and the realistically problematic paths. Validate expected success behaviour, but also validate error handling, edge cases, and degraded-mode behaviour".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js` around lines 24 - 76, Add a new spec in bruno-response.spec.js that exercises the shim's error path by making the sandbox predicate throw and asserting that res(...) surfaces that error; specifically, reuse the pattern around addBrunoResponseShimToContext(vm, response) and vm.evalCode('res(...)') but define the response function (the callable assigned via Object.assign) so its second argument (predicate/filter) throws (or invokes the predicate which throws) and then assert the vm.unwrapResult/throwing behavior or that the evalCode result represents the error as expected; locate the existing tests using addBrunoResponseShimToContext, vm.evalCode, vm.unwrapResult, vm.dump and mirror their setup while replacing the predicate with one that throws to lock down the degraded/error path handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.js`:
- Around line 9-12: The predicate is being invoked with vm.global as the thisArg
which changes semantics; update the callback returned by the function using
marshallToVm and vm.callFunction to pass vm.undefined instead of vm.global so
predicates run as bare functions (this === undefined) matching the host
filterOrMap behavior; locate the call site where vm.callFunction(arg, vm.global,
itemHandle) is used and replace the second argument with vm.undefined (keep
marshallToVm and dispose logic intact).
---
Duplicate comments:
In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js`:
- Around line 16-22: The teardown currently just nulls vm and module, leaking
the QuickJS context and WASM module; update the teardown so you call the QuickJS
disposal methods (e.g., vm.dispose() in afterEach and module.dispose() in
afterAll) with null-checks before setting vm = null and module = null; locate
the vm and module variables in bruno-response.spec.js and replace the simple
null assignments with guarded dispose calls to free the VM and WASM module
resources.
---
Nitpick comments:
In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.js`:
- Around line 3-7: Convert the leading banner comment into a JSDoc block placed
immediately above the toHostQueryArg function: document what the function does
(marshal a QuickJS query argument to a host-compatible value), note that
function handles are wrapped as native callbacks and other values are returned
as-is, and explicitly state the borrowing contract and synchronous-lifetime
caveat ("Safe because `@usebruno/query`'s get() invokes filters synchronously, so
the borrowed arg handle is still valid"). Keep the existing details but format
them as JSDoc on the toHostQueryArg declaration to satisfy the coding guideline.
In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js`:
- Around line 24-76: Add a new spec in bruno-response.spec.js that exercises the
shim's error path by making the sandbox predicate throw and asserting that
res(...) surfaces that error; specifically, reuse the pattern around
addBrunoResponseShimToContext(vm, response) and vm.evalCode('res(...)') but
define the response function (the callable assigned via Object.assign) so its
second argument (predicate/filter) throws (or invokes the predicate which
throws) and then assert the vm.unwrapResult/throwing behavior or that the
evalCode result represents the error as expected; locate the existing tests
using addBrunoResponseShimToContext, vm.evalCode, vm.unwrapResult, vm.dump and
mirror their setup while replacing the predicate with one that throws to lock
down the degraded/error path handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a02fb93f-b53b-40ea-8b34-19e47d90e62f
📒 Files selected for processing (3)
.gitignorepackages/bruno-js/src/sandbox/quickjs/shims/bruno-response.jspackages/bruno-js/src/sandbox/quickjs/shims/bruno-response.spec.js
✅ Files skipped from review due to trivial changes (1)
- .gitignore
…llbacks - Updated the `toHostQueryArg` function to use `vm.undefined` instead of `vm.global` when calling the provided function argument. This change ensures proper context handling during function execution, improving the reliability of query argument processing.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.js (1)
3-7: Use JSDoc for thetoHostQueryArgabstraction.Please convert this block comment to a
/** ... */JSDoc comment so the contract/assumptions are captured in the project’s standard abstraction docs style.As per coding guidelines "Add JSDoc comments to abstractions for additional details".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.js` around lines 3 - 7, Convert the existing block comment above the toHostQueryArg function into a JSDoc-style comment (/** ... */) preserving the same description and assumptions (that it marshals a QuickJS query argument to a host-compatible value, that function handles are wrapped as native callbacks, other values are dumped as-is, and the safety note about `@usebruno/query`'s get() invoking filters synchronously so the borrowed arg handle remains valid); attach the JSDoc directly above the toHostQueryArg declaration and keep the wording clear and concise to match project abstraction docs style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.js`:
- Around line 3-7: Convert the existing block comment above the toHostQueryArg
function into a JSDoc-style comment (/** ... */) preserving the same description
and assumptions (that it marshals a QuickJS query argument to a host-compatible
value, that function handles are wrapped as native callbacks, other values are
dumped as-is, and the safety note about `@usebruno/query`'s get() invoking filters
synchronously so the borrowed arg handle remains valid); attach the JSDoc
directly above the toHostQueryArg declaration and keep the wording clear and
concise to match project abstraction docs style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7457a268-9349-4ea0-b29f-6c7ef878de94
📒 Files selected for processing (1)
packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.js
Collaborator
sanish-bruno
left a comment
sanish-bruno
left a comment
There was a problem hiding this comment.
PR Review
Found 2 issue(s): 0 major, 0 minor, 2 nits.
Summary
- The core fix in
toHostQueryArgis well-implemented — correctly marshals QuickJS function handles into native callbacks for synchronousget()invocations, and properly handles both function and object-predicate arguments. - Good error handling with proper disposal of VM handles on both success and error paths.
- The
.gitignorechange introduces duplicate entries. - Tests cover both callback and object-predicate paths but have a minor cleanup gap.
…se tests - Removed redundant entries from .gitignore to streamline ignored files. - Improved error handling in the `afterEach` and `afterAll` hooks of the bruno-response tests to ensure proper disposal of resources, preventing potential memory leaks.
bijin-bruno
approved these changes
Apr 2, 2026
5 tasks
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
res()shim to the host query engine, fixingres('path[?].prop', filterFn)in safe modetoHostQueryArghelper that marshals QuickJS function handles into native callbacks across the VM boundary, while passing object predicates as-istry/finallyargument disposal that was inconsistent with other shims in the codebaseCloses #7387
Built on top of #7417 by @cryst-hq
JIRA
How it works
The
res()function in the QuickJS sandbox previously only forwarded the expression string to the host, silently dropping any filter/predicate arguments. This caused[?]array filtering to fail with "missing function for resources".The fix wraps each QuickJS function argument into a native callback that marshals data across the VM boundary (host → sandbox → host), while keeping the filter function safely inside the sandbox. Object predicates (e.g.
{ id: 'test' }) are simply dumped to plain JS objects.Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Tests