feat: Implement isSafeMode() API (#5760)#6662
Conversation
sid-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes add a new Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Runtime as Runtime (this.runtime)
participant Bru as Bru instance
participant QuickJS as QuickJS VM Shim
Runner->>Runtime: configure sandbox mode (developer | safe)
Runner->>Bru: instantiate Bru(runtime, ...)
Bru-->>Runtime: store runtime on instance
QuickJS->>Bru: invoke bru.isSafeMode() via shim
Bru-->>QuickJS: return (this.runtime === 'quickjs')
Runner->>Tests: assertions evaluate bru.isSafeMode() result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This PR implements a new isSafeMode() API that allows scripts to detect whether they're running in QuickJS (safe mode) or Node.js VM runtime. The implementation adds a runtime parameter to the Bru class constructor and propagates this parameter across all runtime instantiations.
Key Changes
- Added
isSafeMode()method to the Bru class that returnstruewhen runtime is 'quickjs' - Updated Bru constructor to accept a
runtimeparameter as the first argument - Propagated runtime parameter to all Bru instantiations across test, script, vars, and assert runtimes
- Added QuickJS shim for the new API method
- Updated autocomplete suggestions to include the new API
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/bruno-js/src/bru.js | Added runtime parameter to constructor and implemented isSafeMode() method |
| packages/bruno-js/src/runtime/test-runtime.js | Updated Bru instantiation to pass runtime parameter |
| packages/bruno-js/src/runtime/script-runtime.js | Updated two Bru instantiations to pass runtime parameter |
| packages/bruno-js/src/runtime/vars-runtime.js | Updated Bru instantiation to pass runtime parameter |
| packages/bruno-js/src/runtime/assert-runtime.js | Updated Bru instantiation to pass runtime parameter |
| packages/bruno-js/src/sandbox/quickjs/shims/bru.js | Added QuickJS shim for isSafeMode() function |
| packages/bruno-js/tests/setEnvVar.spec.js | Updated test to pass runtime parameter to Bru constructor |
| packages/bruno-app/src/utils/codemirror/autocomplete.js | Added bru.isSafeMode() to autocomplete suggestions |
Comments suppressed due to low confidence (2)
packages/bruno-js/src/bru.js:21
- The
runtimeparameter is accepted in the constructor but is never stored as an instance variable. This will cause theisSafeMode()method to always returnfalse(sincethis.runtimewill beundefined). Addthis.runtime = runtime;in the constructor to store the runtime parameter.
constructor(runtime, envVariables, runtimeVariables, processEnvVars, collectionPath, collectionVariables, folderVariables, requestVariables, globalEnvironmentVariables, oauth2CredentialVariables, collectionName, promptVariables) {
this.envVariables = envVariables || {};
this.runtimeVariables = runtimeVariables || {};
this.promptVariables = promptVariables || {};
this.processEnvVars = cloneDeep(processEnvVars || {});
this.collectionVariables = collectionVariables || {};
this.folderVariables = folderVariables || {};
this.requestVariables = requestVariables || {};
this.globalEnvironmentVariables = globalEnvironmentVariables || {};
this.oauth2CredentialVariables = oauth2CredentialVariables || {};
this.collectionPath = collectionPath;
this.collectionName = collectionName;
packages/bruno-js/tests/setEnvVar.spec.js:21
- The parameter comments in the test do not match the actual constructor signature. The comments reference parameters like
historyLogger,setVisualizations,secretVariables, anditerationDetailsthat don't exist in the Bru constructor. The actual signature is: runtime, envVariables, runtimeVariables, processEnvVars, collectionPath, collectionVariables, folderVariables, requestVariables, globalEnvironmentVariables, oauth2CredentialVariables, collectionName, promptVariables. Please update the comments to match the actual parameters being passed.
new Bru(
/* runtime */ 'quickjs',
/* envVariables */ {},
/* runtimeVariables */ {},
/* processEnvVars */ {},
/* collectionPath */ '/',
/* historyLogger */ undefined,
/* setVisualizations */ undefined,
/* secretVariables */ {},
/* collectionVariables */ {},
/* folderVariables */ {},
/* requestVariables */ {},
/* globalEnvironmentVariables */ {},
/* oauth2CredentialVariables */ {},
/* iterationDetails */ {},
/* collectionName */ 'Test'
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| isSafeMode() { | ||
| return this.runtime === 'quickjs'; | ||
| } |
There was a problem hiding this comment.
The new isSafeMode() API lacks test coverage. Consider adding tests to verify that it returns true when runtime is 'quickjs' and false for other runtime values like 'nodevm'.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @packages/bruno-js/src/bru.js:
- Line 10: The constructor currently accepts runtime but doesn't assign it to
the instance; add an assignment in the constructor (e.g., set this.runtime =
runtime) so that methods using this.runtime—specifically isSafeMode()—work
correctly; update the constructor function in bru.js to store the runtime
parameter on the instance before any other logic that may rely on it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/bruno-app/src/utils/codemirror/autocomplete.jspackages/bruno-js/src/bru.jspackages/bruno-js/src/runtime/assert-runtime.jspackages/bruno-js/src/runtime/script-runtime.jspackages/bruno-js/src/runtime/test-runtime.jspackages/bruno-js/src/runtime/vars-runtime.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.jspackages/bruno-js/tests/setEnvVar.spec.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-js/src/bru.jspackages/bruno-js/src/runtime/script-runtime.jspackages/bruno-js/src/runtime/vars-runtime.jspackages/bruno-js/src/runtime/assert-runtime.jspackages/bruno-js/tests/setEnvVar.spec.jspackages/bruno-js/src/runtime/test-runtime.jspackages/bruno-app/src/utils/codemirror/autocomplete.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: packages/bruno-js/src/runtime/hooks-runtime.js:8-135
Timestamp: 2025-12-03T06:32:11.738Z
Learning: vm2 runtime is being removed from the Bruno application and QuickJS will become the default runtime for script execution.
📚 Learning: 2025-12-03T06:32:11.738Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: packages/bruno-js/src/runtime/hooks-runtime.js:8-135
Timestamp: 2025-12-03T06:32:11.738Z
Learning: vm2 runtime is being removed from the Bruno application and QuickJS will become the default runtime for script execution.
Applied to files:
packages/bruno-js/src/bru.jspackages/bruno-js/src/runtime/script-runtime.jspackages/bruno-js/src/runtime/vars-runtime.jspackages/bruno-js/tests/setEnvVar.spec.jspackages/bruno-js/src/runtime/test-runtime.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.js
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-js/src/bru.jspackages/bruno-js/src/runtime/script-runtime.jspackages/bruno-js/src/runtime/vars-runtime.jspackages/bruno-js/src/runtime/assert-runtime.jspackages/bruno-js/tests/setEnvVar.spec.jspackages/bruno-js/src/runtime/test-runtime.jspackages/bruno-app/src/utils/codemirror/autocomplete.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.js
📚 Learning: 2025-12-16T07:16:23.647Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:23.647Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.
Applied to files:
packages/bruno-js/src/runtime/test-runtime.js
🧬 Code graph analysis (1)
packages/bruno-js/src/sandbox/quickjs/shims/bru.js (2)
packages/bruno-js/src/sandbox/quickjs/index.js (2)
vm(59-59)vm(100-100)packages/bruno-js/src/sandbox/quickjs/utils/index.js (1)
marshallToVm(1-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (9)
packages/bruno-app/src/utils/codemirror/autocomplete.js (1)
79-79: LGTM!The autocomplete hint for the new
isSafeMode()API is correctly formatted and appropriately placed in the hints array.packages/bruno-js/src/runtime/vars-runtime.js (1)
39-39: LGTM!The Bru constructor invocation correctly passes
this.runtimeas the first argument, aligning with the updated constructor signature.packages/bruno-js/src/sandbox/quickjs/shims/bru.js (1)
26-30: LGTM!The
isSafeModeshim correctly follows the established pattern: creates a VM function, marshalls the return value, binds it to the bru object, and properly disposes of the handle.packages/bruno-js/tests/setEnvVar.spec.js (1)
6-6: LGTM!The test correctly instantiates Bru with the new
runtimeparameter as the first argument. The inline comment helpfully documents the parameter's purpose.packages/bruno-js/src/bru.js (1)
283-285: LGTM, assuming runtime is stored.The
isSafeMode()method correctly returnstruewhen the runtime is 'quickjs'. However, this depends on fixing the constructor to storethis.runtime(see comment on line 10).packages/bruno-js/src/runtime/assert-runtime.js (1)
260-260: LGTM! Runtime parameter correctly passed to Bru constructor.The addition of
this.runtimeas the first argument aligns with the updated Bru constructor signature to support the newisSafeMode()API.packages/bruno-js/src/runtime/test-runtime.js (1)
35-35: LGTM! Runtime parameter correctly passed to Bru constructor.The change is consistent with the constructor signature update across all runtime modules.
packages/bruno-js/src/runtime/script-runtime.js (2)
36-36: LGTM! Runtime parameter correctly passed to Bru constructor in runRequestScript.The change maintains consistency with the updated Bru constructor signature.
131-131: LGTM! Runtime parameter correctly passed to Bru constructor in runResponseScript.The change mirrors the update in
runRequestScriptand maintains consistency across both methods.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/bruno-tests/collection/scripting/api/bru/isSafeMode.bru`:
- Around line 13-17: Update the test so the assertion matches its name: in the
test with the description "bru.isSafeMode() returns true in safe mode" change
the expectation from expect(bru.isSafeMode()).to.be.false to
expect(bru.isSafeMode()).to.be.true; also normalize indentation to 2 spaces for
the test block (the test(...) line, the function body and the expect line) to
follow project formatting.
🧹 Nitpick comments (2)
tests/scripting/bru-api/isSafeMode/isSafeMode.spec.ts (2)
5-22: Consider usingtest.stepfor improved report readability.Per coding guidelines, using
test.stepmakes generated reports easier to read. The test logic is sound and well-commented.Suggested refactor
test('returns false when running in developer mode', async ({ pageWithUserData: page }) => { - // Set up developer mode - await setSandboxMode(page, 'is-safe-mode-test', 'developer'); + await test.step('Set up developer mode', async () => { + await setSandboxMode(page, 'is-safe-mode-test', 'developer'); + }); - // Run the collection - await runCollection(page, 'is-safe-mode-test'); + await test.step('Run the collection', async () => { + await runCollection(page, 'is-safe-mode-test'); + }); - // Validate test results - // In developer mode: - // - test-safe-mode-false should PASS (expects false, gets false) - // - test-safe-mode-true should FAIL (expects true, gets false) - await validateRunnerResults(page, { - totalRequests: 2, - passed: 1, - failed: 1, - skipped: 0 + await test.step('Validate test results', async () => { + // In developer mode: + // - test-safe-mode-false should PASS (expects false, gets false) + // - test-safe-mode-true should FAIL (expects true, gets false) + await validateRunnerResults(page, { + totalRequests: 2, + passed: 1, + failed: 1, + skipped: 0 + }); }); });
24-41: Sametest.stepsuggestion applies here.Apply the same refactor for consistency. Otherwise, the test structure and assertions are correct.
| script:pre-request { | ||
| test("bru.isSafeMode() returns true in safe mode", function() { | ||
| expect(bru.isSafeMode()).to.be.false; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Test name contradicts assertion.
The test name states "bru.isSafeMode() returns true in safe mode" but the assertion expects false. This mismatch will cause confusion when tests fail. Update either the test name or the assertion to be consistent.
Additionally, the indentation is inconsistent (3 spaces on line 14, 6 spaces on line 15). Per coding guidelines, use 2 spaces.
Suggested fix
script:pre-request {
- test("bru.isSafeMode() returns true in safe mode", function() {
- expect(bru.isSafeMode()).to.be.false;
+ test("bru.isSafeMode() returns false in developer mode", function() {
+ expect(bru.isSafeMode()).to.be.false;
});
}📝 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.
| script:pre-request { | |
| test("bru.isSafeMode() returns true in safe mode", function() { | |
| expect(bru.isSafeMode()).to.be.false; | |
| }); | |
| } | |
| script:pre-request { | |
| test("bru.isSafeMode() returns false in developer mode", function() { | |
| expect(bru.isSafeMode()).to.be.false; | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@packages/bruno-tests/collection/scripting/api/bru/isSafeMode.bru` around
lines 13 - 17, Update the test so the assertion matches its name: in the test
with the description "bru.isSafeMode() returns true in safe mode" change the
expectation from expect(bru.isSafeMode()).to.be.false to
expect(bru.isSafeMode()).to.be.true; also normalize indentation to 2 spaces for
the test block (the test(...) line, the function body and the expect line) to
follow project formatting.
Add `isSafeMode()` to Bru API that returns `true` in
case the runtime is a sandbox.
This allows for scripts to test for and handle whether
running in sandbox or not:
```javascript
if (bru.isSafeMode()) {
throw new Error('This script requires Developer mode')
}
```
Co-authored-by: Anoop M D <[email protected]>
* fix bru safe mode and add tests * rm: settimeout fix: isSafe mode test (#6844)
sid-bruno
force-pushed
the
feature/bru-safemode-5760
branch
from
df58690 to
a724f01
Compare
3 participants
Description
Extends #5760
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
✏️ Tip: You can customize this high-level summary in your review settings.