feat(cli): persist env, global-env, and collection var changes from scripts#8387
Open
sanish-bruno wants to merge 15 commits into
Open
feat(cli): persist env, global-env, and collection var changes from scripts#8387sanish-bruno wants to merge 15 commits into
sanish-bruno wants to merge 15 commits into
Conversation
sanish-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-bruno
as code owners
Contributor
WalkthroughThe Bruno CLI gains script variable persistence: after each script phase (pre-request, post-response vars/script, tests), variable mutations are applied in-memory and written back to disk. A new ChangesVariable Persistence Across CLI Runs
Sequence Diagram(s)sequenceDiagram
participant run.js
participant run-single-request.js
participant persist-variables.js
participant disk as Disk (env/collection files)
run.js->>run.js: build persistPaths from env descriptors + envVarOverrides
run.js->>run-single-request.js: runSingleRequest(..., persistPaths, globalEnvVars)
Note over run-single-request.js: pre-request / post-response / test script runs
run-single-request.js->>persist-variables.js: syncVariableUpdates(result, request)
persist-variables.js->>persist-variables.js: applyVariableUpdates (in-memory maps)
persist-variables.js->>disk: persistVariableUpdates (env, globalEnv, collection)
disk-->>persist-variables.js: (write skipped if byte-identical)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
sanish-bruno
changed the title
![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: 7
🤖 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 `@packages/bruno-cli/src/commands/run.js`:
- Around line 369-373: The env-file format detection in resolveEnvFileFormat
currently handles .json and .yml but misses .yaml, causing YAML env files to
fall back to the BRU path; update the extension check to treat .yaml the same as
.yml, and make sure the downstream env-file parse/serialize logic that uses this
resolver also honors the restored YAML format.
In `@packages/bruno-cli/src/runner/run-single-request.js`:
- Around line 100-117: Collection variable updates are only being applied to the
current request state, so later requests in the same run still read stale values
from the shared collection object. Update the request-run flow in
run-single-request.js so `syncVariableUpdates` (and the `applyVariableUpdates`
path it uses) also mutates the in-memory collection state that `run.js` uses to
build subsequent requests, ensuring `bru.setCollectionVar()` persists for the
rest of the run as well as to disk.
In `@packages/bruno-cli/src/utils/persist-variables.js`:
- Around line 304-305: The merge path in persist-variables.js is passing
unfiltered parsed.variables into mergeScriptVarsIntoEnvList, so falsy entries
like null can still reach v.enabled and break persistence. Before calling
mergeScriptVarsIntoEnvList, normalize rawVariables the same way
parseEnvironmentJson(parsed) does by filtering out falsy entries (or otherwise
sanitizing the array) so only valid variable objects are merged. Keep the fix
localized around rawVariables and mergeScriptVarsIntoEnvList in
persist-variables.js.
- Around line 313-320: The current persist-and-compare flow in
persist-variables.js can rewrite unchanged BRU/YAML files and convert CRLF to LF
because parseEnvironment/stringifyEnvironment normalize line endings. Detect the
original EOL style from existingRaw before calling stringifyEnvironment,
preserve it for the generated content, and pass the correctly normalized text
into writeIfChanged so unchanged CRLF files are not rewritten. Keep the fix
localized around sourceForParse, stringifyEnvironment, and writeIfChanged.
In `@packages/bruno-cli/tests/integration/run-typed-persistence.spec.js`:
- Around line 41-630: Add a failure-path regression test to this CLI persistence
suite: the current cases only cover successful runs, but the runner now syncs
partialResults even when pre-request, post-response, or test scripts throw. Add
one fixture in run-typed-persistence.spec.js that calls a variable setter in a
script and then throws, run it through runCli, and assert the on-disk
env/collection file still includes the written typed value; use the existing
helpers like runCli, assertDiskState, and the script hooks in set-typed-vars.bru
/ set-typed-global-vars.bru to locate the right behavior.
- Around line 79-84: The test helper in runCli is hardcoding SHELL to /bin/sh,
which makes the integration suite fail on non-POSIX platforms. Update the spawn
environment setup in runCli so it uses a platform-appropriate shell only when
needed, or avoids overriding SHELL entirely on Windows, while keeping the
existing CLI_BIN child_process launch flow intact.
In `@packages/bruno-cli/tests/utils/persist-variables.spec.js`:
- Around line 740-761: The write-error test for persistVariableUpdates is
relying on chmodSync-based permissions, which is not deterministic across OSes
and container/root environments. Replace the filesystem-permission setup with a
mock of fs.writeFileSync after seeding the Locked.bru fixture, so the test
asserts the caller-visible EACCES/EPERM behavior directly. Update the test logic
in the persist-variable updates spec to keep the existing error expectation
while removing the Unix-only chmod dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26c7094e-7947-4087-ba4b-0fb215d78166
📒 Files selected for processing (5)
packages/bruno-cli/src/commands/run.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-cli/src/utils/persist-variables.jspackages/bruno-cli/tests/integration/run-typed-persistence.spec.jspackages/bruno-cli/tests/utils/persist-variables.spec.js
sanish-bruno
force-pushed
the
feat/persist-variables-in-cli
branch
from
babcebc to
beef4cb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/bruno-cli/src/utils/persist-variables.js`:
- Around line 172-176: The current override handling in persist-variables.js is
dropping script-authored changes whenever a CLI --env-var uses the same key.
Update the merge/persist flow around scriptVars, overrides, and the related
conflict checks so only the transient CLI-provided value is excluded from disk,
while deliberate script writes via bru.setEnvVar(), bru.deleteEnvVar(), and
bru.setGlobalEnvVar() still persist. Use the existing scriptKeys/override
comparison logic and the later conflict paths to distinguish copied override
values from real script mutations instead of deleting by key alone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9940068-de18-4fa4-b5f2-4a9a8a857774
📒 Files selected for processing (5)
packages/bruno-cli/src/commands/run.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-cli/src/utils/persist-variables.jspackages/bruno-cli/tests/integration/run-typed-persistence.spec.jspackages/bruno-cli/tests/utils/persist-variables.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-cli/src/runner/run-single-request.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/bruno-cli/tests/integration/run-typed-persistence.spec.js`:
- Around line 662-667: The regression test only verifies persisted output and
does not assert that the CLI run failed, so it can pass even if the
post-response error path is broken. In the runCli() scenario for
set-then-throw.bru, capture the result from runCli, assert a non-zero exit
status first, and only then read Test.bru and verify the persisted value. Use
the runCli helper and the existing error.partialResults regression setup to keep
the test focused on the failure path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1cd8a23-bea8-474e-b11e-1eec4dac569a
📒 Files selected for processing (4)
packages/bruno-cli/src/commands/run.jspackages/bruno-cli/src/utils/persist-variables.jspackages/bruno-cli/tests/integration/run-typed-persistence.spec.jspackages/bruno-cli/tests/utils/persist-variables.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-cli/src/utils/persist-variables.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-cli/tests/integration/run-typed-persistence.spec.js (1)
611-613: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRun the throw-path regression across both sandboxes.
This case hardcodes
'developer', but the suite already treats quickjs and nodevm as equivalent persistence surfaces. A sandbox-specificpartialResultsregression in'safe'would slip past this test, so it should use the same sandbox matrix as the happy-path cases above.As per coding guidelines, "Add tests for any new functionality or meaningful changes" and "Cover both the “happy path” and the realistically problematic paths."
🤖 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 `@packages/bruno-cli/tests/integration/run-typed-persistence.spec.js` around lines 611 - 613, The throw-path regression test in run-typed-persistence.spec.js is hardcoded to the developer sandbox, so it won’t catch sandbox-specific partialResults issues in safe. Update the set-then-throw.bru test to follow the same sandbox matrix used by the happy-path cases, and run the same assertions for both quickjs and nodevm equivalents through the existing test harness helpers. This keeps the regression coverage aligned with the persistence surfaces already exercised elsewhere in the suite.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/bruno-cli/tests/integration/run-typed-persistence.spec.js`:
- Around line 611-613: The throw-path regression test in
run-typed-persistence.spec.js is hardcoded to the developer sandbox, so it won’t
catch sandbox-specific partialResults issues in safe. Update the
set-then-throw.bru test to follow the same sandbox matrix used by the happy-path
cases, and run the same assertions for both quickjs and nodevm equivalents
through the existing test harness helpers. This keeps the regression coverage
aligned with the persistence surfaces already exercised elsewhere in the suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8db4396c-fa84-439b-8d3d-79b05030783f
📒 Files selected for processing (1)
packages/bruno-cli/tests/integration/run-typed-persistence.spec.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/bruno-cli/tests/integration/run-typed-persistence.spec.js`:
- Around line 712-728: Add a failing-path assertion for collection-var
persistence in this integration test: alongside the existing `bru.setEnvVar()`
write in the `tests {}` block, also call `bru.setCollectionVar()` before the
throw, then after verifying the non-zero `runCli()` exit, read the collection
file (for example `collection.bru`) and assert the written value is persisted.
Keep the current `set-then-throw-in-tests.bru` flow and use the existing test
helpers (`runCli`, `fs.readFileSync`, `path.join`) so this covers
`setCollectionVar()` partial-results sync as well as `setEnvVar()`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 163d8905-cb62-4c9f-a03a-87785c77efd2
📒 Files selected for processing (1)
packages/bruno-cli/tests/integration/run-typed-persistence.spec.js
…onment and collection files - Introduced new utility functions for applying and persisting variable updates, enhancing the management of environment and collection variables. - Updated the run command to include descriptors for environment files, allowing for better tracking of variable changes. - Enhanced the runSingleRequest function to synchronize variable updates and persist changes to the appropriate files. - Added comprehensive tests to ensure the correct functionality of variable merging and persistence logic across different file formats.
…ronment variables - Updated the run command to exclusively recognize '.yml' files, removing support for '.yaml' extensions. - Enhanced the persistence logic to correctly handle data type inference for newly added environment and collection variables. - Added comprehensive tests to validate the correct writing of variables to YAML and .bru files, ensuring accurate data type preservation and updates. - Improved the handling of existing data types when variables are modified, ensuring consistency across different formats.
- Introduced a new test suite to validate the persistence of typed environment and collection variables set via scripts in the CLI. - Implemented a local server to facilitate HTTP requests during tests, ensuring accurate simulation of variable management. - Enhanced the test logic to verify that the correct data type annotations are written to both .bru and YAML files after CLI execution. - Added comprehensive assertions to confirm the expected disk state for both global and collection variables across different sandboxes.
…s after post response script execution - Eliminated the syncVariableUpdates call from the runSingleRequest function, streamlining the variable handling process. - Adjusted the logic to focus on executing post response variables without unnecessary synchronization, improving performance and clarity.
… improve persistence resilience - Added support for transient environment variable overrides via the `--env-var` CLI option, ensuring these values are not persisted to disk. - Enhanced the `mergeScriptVarsIntoEnvList` function to prevent overridden variables from being written back, preserving existing entries. - Updated the `persistVariableUpdates` function to handle potential disk write errors gracefully, logging warnings instead of failing the execution. - Introduced comprehensive tests to validate the behavior of transient variables and ensure correct persistence logic under various scenarios.
- Improved the `persistEnvFile` function to preserve additional fields (uid, dataType, custom metadata) during JSON writes, ensuring no data loss for unrecognized entries. - Updated the `persistVariableUpdates` function to respect `envVarOverrides` when writing to the global environment file, preventing transient values from being persisted. - Added regression tests to validate the preservation of additional fields and the correct handling of environment variable overrides in persistence scenarios.
…istence tests - Renamed the `writeFileSyncMkdirP` function to `writeFixtureFile` for clarity, emphasizing its role in writing fixture files with parent directory creation. - Replaced multiple calls to `writeFileSyncMkdirP` with the new `writeFixtureFile` function to improve code readability and maintainability. - Added tests to verify the deletion of environment variables from disk when removed from the environment map, ensuring accurate persistence behavior. - Implemented checks to prevent runtime variables from being persisted, safeguarding against unintended data leaks.
…rsistence - Implemented a new test to verify that typed environment variables are correctly persisted to a JSON file using the --env-file option. - Ensured that existing entries retain their uid and custom fields during the persistence process, validating the shape-preservation guarantee. - Enhanced the test to check that new typed variables from the script are accurately written and that untouched entries remain unchanged.
…istence - Implemented new tests to verify the persistence of typed environment variables to both YAML and .bru files using the --env-file option. - Ensured that typed values are correctly serialized with appropriate annotations in the output files, validating the correct handling of different formats. - Enhanced coverage for the persistence behavior of environment variables, confirming that existing entries remain unchanged while new variables are accurately written.
- Added a new test to verify the preservation of native typed values in JSON environment files when unrelated keys are touched during script execution. - Ensured that typed values maintain their original types and are auto-annotated with the correct dataType, validating the integrity of the environment variable persistence process. - Expanded the typed-value inference tests to cover cross-type transitions, confirming that existing dataType annotations are ignored when new values are written.
…persistence handling - Changed the `envVarOverrides` from a Set to a Map to track injected values, enhancing the logic for distinguishing between transient and deliberate variable writes. - Updated related functions to accommodate the new Map structure, ensuring that only matching overrides are filtered out during persistence. - Enhanced documentation and tests to reflect the new behavior, confirming that deliberate script writes with different values are correctly persisted.
… and error handling
- Implemented new tests to verify that variables set within `tests {}` blocks are correctly persisted to the environment and collection files.
- Added a test to ensure that variables written before an error in a `tests {}` block are still saved, confirming the integrity of partial results during execution.
- Enhanced existing tests with detailed comments for clarity and understanding of the persistence behavior in various scenarios.
sanish-bruno
force-pushed
the
feat/persist-variables-in-cli
branch
from
27a574a to
2b22414
Compare
… handling
- Introduced a new test to verify that collection variables set before an error in a `tests {}` block are correctly persisted to the collection file.
- Enhanced existing tests to ensure that both environment and collection variables maintain their expected values during error scenarios, confirming the robustness of variable persistence.
| runtimeVariables, | ||
| collectionPath, | ||
| processEnvVars | ||
| ); |
Collaborator
There was a problem hiding this comment.
for varsRuntime.runPostResponseVars in app we are capturing the return value and processing it via sendVariableUpdates() shouldn't we be doing the same here in CLI?
There was a problem hiding this comment.
Could be a mistake if we are doing it. Most likely it is a no-op, verified the behaviour and we were not setting or deleting the post response collection variables. will check the code and remove the no-op code.
Collaborator
Author
There was a problem hiding this comment.
return value needs to be processed as the Expr technicaly can make changes to global env, collection env, collection vars. we should be processing it if it is missing on CLI
…ersistence from post-response expressions - Introduced a new test to verify that environment and collection variables mutated as a side effect of `vars:post-response` expressions are correctly persisted. - Enhanced the `runSingleRequest` function to synchronize variable updates after executing post-response scripts, ensuring that changes are reflected in the environment and collection files. - This change improves the robustness of variable handling in the CLI, aligning it with the expected behavior of the desktop application.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/bruno-cli/src/utils/persist-variables.js`:
- Around line 306-318: The JSON persistence path in persist-variables.js is
swallowing parse/validation failures by returning inside the existing try/catch
blocks, unlike the .bru/.yml flow. Update the JSON branch so malformed input and
parseEnvironmentJson validation errors are allowed to bubble out to the caller
instead of being silently ignored, keeping the behavior aligned with the
existing caller-level warning path for persistence in this module.
In `@packages/bruno-cli/tests/integration/run-typed-persistence.spec.js`:
- Around line 604-620: The post-response error regression test in
run-typed-persistence.spec.js only verifies envVariables persistence after a
thrown script and misses collectionVariables. Update the scenario around the
set-then-throw.bru run to also call bru.setCollectionVar() before the throw,
then assert the corresponding collection.bru file still contains the persisted
value after the non-zero exit, using the existing runCli and result.code checks
as the anchor points.
In `@packages/bruno-cli/tests/utils/persist-variables.spec.js`:
- Around line 331-358: The opencollection.yml persistence test only verifies
plain string values, so it does not cover typed serialization for collection
variables. Update the persistVariableUpdates/write path around
stringifyCollection for yml collections to assert number, boolean, and object
values are preserved when written, using the existing persistVariableUpdates and
stringifyCollection flow as the target. Extend this test to include typed
collection vars and verify the YAML output reflects their types, not just names
and string values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6284a59e-32a8-4bbc-b88c-7b096f5138ec
📒 Files selected for processing (5)
packages/bruno-cli/src/commands/run.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-cli/src/utils/persist-variables.jspackages/bruno-cli/tests/integration/run-typed-persistence.spec.jspackages/bruno-cli/tests/utils/persist-variables.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-cli/src/runner/run-single-request.js
Comment on lines
+306
to
+318
| try { | ||
| parsed = JSON.parse(existingRaw); | ||
| } catch { | ||
| // Bail rather than overwrite a malformed user file with our best guess. | ||
| return; | ||
| } | ||
| // Validate shape, but merge against the raw `parsed.variables` so per-entry fields the | ||
| // CLI doesn't recognize (uid, dataType, custom metadata) survive on entries the script | ||
| // didn't touch — parseEnvironmentJson's normalizer would otherwise strip them. | ||
| try { | ||
| parseEnvironmentJson(parsed); | ||
| } catch { | ||
| return; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Don't swallow malformed JSON env files.
The .bru / .yml path lets parse failures reach the caller, but this JSON branch returns silently. That makes --env-file ...json persistence fail with no warning and leaves the run looking successful even though nothing was written. Let these errors bubble so the existing caller-level warning path can report that persistence was skipped. Based on PR objectives, persistence failures are expected to warn rather than disappear silently.
🤖 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 `@packages/bruno-cli/src/utils/persist-variables.js` around lines 306 - 318,
The JSON persistence path in persist-variables.js is swallowing parse/validation
failures by returning inside the existing try/catch blocks, unlike the .bru/.yml
flow. Update the JSON branch so malformed input and parseEnvironmentJson
validation errors are allowed to bubble out to the caller instead of being
silently ignored, keeping the behavior aligned with the existing caller-level
warning path for persistence in this module.
Comment on lines
+604
to
+620
| script:post-response { | ||
| bru.setEnvVar("beforeThrow", 42); | ||
| throw new Error("boom"); | ||
| } | ||
| ` | ||
| ); | ||
|
|
||
| const result = await runCli([ | ||
| 'run', 'set-then-throw.bru', '--env', 'Test', '--sandbox', 'developer', '--noproxy' | ||
| ]); | ||
|
|
||
| // Pin failure exit first — otherwise the persistence assertion below could pass for the | ||
| // wrong reason (e.g. the script never ran). | ||
| expect(result.code).not.toBe(0); | ||
|
|
||
| const envContent = fs.readFileSync(path.join(tmpDir, 'environments', 'Test.bru'), 'utf8'); | ||
| expect(envContent).toMatch(/@number\s+beforeThrow:\s*42/); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover collection vars in the post-response error regression too.
This test only proves envVariables survive a thrown script:post-response. A regression that drops collectionVariables in that same partial-results path would still pass here. Add one bru.setCollectionVar() before the throw and assert collection.bru after the non-zero exit. As per coding guidelines, "Add tests for any new functionality or meaningful changes" and "Cover both the “happy path” and the realistically problematic paths."
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 618-618: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(tmpDir, 'environments', 'Test.bru'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 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 `@packages/bruno-cli/tests/integration/run-typed-persistence.spec.js` around
lines 604 - 620, The post-response error regression test in
run-typed-persistence.spec.js only verifies envVariables persistence after a
thrown script and misses collectionVariables. Update the scenario around the
set-then-throw.bru run to also call bru.setCollectionVar() before the throw,
then assert the corresponding collection.bru file still contains the persisted
value after the non-zero exit, using the existing runCli and result.code checks
as the anchor points.
Source: Coding guidelines
Comment on lines
+331
to
+358
| it('writes collection vars back to opencollection.yml for yml-format collections', () => { | ||
| const collectionRootPath = writeFile('opencollection.yml', | ||
| 'opencollection: 1.0.0\ninfo:\n name: yml-collection\nrequest:\n vars:\n - name: k1\n value: old\n' | ||
| ); | ||
| const collection = { | ||
| format: 'yml', | ||
| brunoConfig: { name: 'yml-collection' }, | ||
| root: { | ||
| meta: null, | ||
| request: { | ||
| headers: [], | ||
| auth: { mode: 'none' }, | ||
| script: { req: null, res: null }, | ||
| tests: null, | ||
| vars: { req: [{ uid: 'v1', name: 'k1', value: 'old', enabled: true, type: 'request' }], res: [] } | ||
| } | ||
| } | ||
| }; | ||
| persistVariableUpdates( | ||
| { collectionVariables: { k1: 'new', k2: 'fresh' } }, | ||
| { collection, collectionRootPath } | ||
| ); | ||
| const written = fs.readFileSync(collectionRootPath, 'utf8'); | ||
| expect(written).toMatch(/name:\s*k1/); | ||
| expect(written).toMatch(/value:\s*new/); | ||
| expect(written).toMatch(/name:\s*k2/); | ||
| expect(written).toMatch(/value:\s*fresh/); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert typed serialization for opencollection.yml here.
This YAML-collection case only checks plain values, but the feature contract includes typed persistence in .yml files too. If stringifyCollection(..., { format: 'yml' }) dropped number / boolean / object typing for collection vars, this suite would not catch it. As per coding guidelines, "Add tests for any new functionality or meaningful changes" and "Cover both the “happy path” and the realistically problematic paths."
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 352-352: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(collectionRootPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 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 `@packages/bruno-cli/tests/utils/persist-variables.spec.js` around lines 331 -
358, The opencollection.yml persistence test only verifies plain string values,
so it does not cover typed serialization for collection variables. Update the
persistVariableUpdates/write path around stringifyCollection for yml collections
to assert number, boolean, and object values are preserved when written, using
the existing persistVariableUpdates and stringifyCollection flow as the target.
Extend this test to include typed collection vars and verify the YAML output
reflects their types, not just names and string values.
Source: Coding guidelines
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
3 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.
JIRA
Brings the variable-persistence behavior from #8315 to the CLI. Scripts that call
bru.setEnvVar/bru.setGlobalEnvVar/bru.setCollectionVar(and theirdelete*counterparts) now write changes back to disk in addition to mutating in-memory state — matching desktop-app behavior on the same branch.Important
Depends on #8315 and must be merged after it.
Scopes persisted
--envenv file, or--env-file<workspace>/environments/<globalEnv>.ymlcollection.bru/opencollection.ymlHighlights
bru.setEnvVar('count', 42)) get the rightdataType(number/boolean/object); matching.bru/.ymlannotations land on disk.--env-vartransient overrides never reach disk; the file's existing entry (incl.secret: true) is preserved untouched.uid,dataType, and user-set custom metadata survive on entries the script doesn't echo back.Tests
tests/utils/persist-variables.spec.js— merge logic, file round-trips (.yml/.bru/.json/ collection root), typed-value inference, override regression guards, runtime-vars-NEVER-persisted design guard, content-comparison viawriteFileSyncspy, disk-error resilience.tests/integration/run-typed-persistence.spec.jsdriving the realbin/bru.jsagainst a local HTTP server, bothdeveloperandsafesandboxes — typed annotations on.bruand.ymlvia--env/--global-env,--workspace-pathexplicit flag,--env-fileround-trip across all three formats (.json/.yml/.bru) including JSONuid/ custom-field shape preservation,--env-varleak guard.Contribution Checklist
Summary by CodeRabbit
New Features
Bug Fixes
--env-varoverrides no longer overwrite saved environment files.Tests