Refactor sandbox env passthrough #1073
Merged
Conversation
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
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
WalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 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 |
Contributor
|
I'll analyze this pull request that refactors the sandbox environment variable handling. Let me first understand the current implementation and the changes being made. Now I have a good understanding of the changes made in this PR. Let me analyze them for the review. The PR refactors the sandbox environment variable passthrough to:
This addresses issue #1016 which suggested avoiding the indirect approach of creating CLI args and then parsing them back. LLxprt PR Review – PR #1073Issue Alignment
Side Effects
Code Quality
Tests & Coverage
Verdict |
![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
🤖 Fix all issues with AI agents
In @packages/cli/src/utils/sandbox.test.ts:
- Around line 138-226: The test mistakenly compares the returned array from
buildSandboxEnvArgs to the input env object, which is always false; replace that
assertion by calling buildSandboxEnvArgs twice (e.g., result1 =
buildSandboxEnvArgs(env); result2 = buildSandboxEnvArgs(env);) and assert
result2 is not the same reference as result1 using not.toBe to ensure the
function returns a fresh array each call; update the test referencing
buildSandboxEnvArgs and the local env variable accordingly.
🧹 Nitpick comments (2)
packages/cli/src/utils/sandbox.ts (2)
56-76: Avoid per-iteration object spreads ingetPassthroughEnvVars(unnecessary allocations).
Right now Line 66-75 creates a new object on every passthrough var; a simple loop is clearer and avoids churn. Also consider hoisting the passthrough list so it isn’t recreated each call.Proposed refactor
+const PASSTHROUGH_VARIABLES = [ + 'LLXPRT_CODE_IDE_SERVER_PORT', + 'LLXPRT_CODE_IDE_WORKSPACE_PATH', + 'LLXPRT_CODE_WELCOME_CONFIG_PATH', + 'TERM_PROGRAM', +] as const; + export function getPassthroughEnvVars( env: NodeJS.ProcessEnv, ): Record<string, string> { - const passthroughVariables = [ - 'LLXPRT_CODE_IDE_SERVER_PORT', - 'LLXPRT_CODE_IDE_WORKSPACE_PATH', - 'LLXPRT_CODE_WELCOME_CONFIG_PATH', - 'TERM_PROGRAM', - ]; - - return passthroughVariables.reduce<Record<string, string>>( - (result, envVar) => { - const value = env[envVar]; - if (!value) { - return result; - } - return { ...result, [envVar]: value }; - }, - {}, - ); + const result: Record<string, string> = {}; + for (const envVar of PASSTHROUGH_VARIABLES) { + const value = env[envVar]; + if (typeof value === 'string' && value.length > 0) { + result[envVar] = value; + } + } + return result; }
343-346:sandboxEnvspread ofprocess.env+ passthrough-from-process.envis redundant as written.
At Line 343-346,getPassthroughEnvVars(process.env)can’t add anything that isn’t already in...process.env(it only re-selects from it). If this is meant as type-narrowing / “curated override” for specific keys, consider adding a short comment or usingObject.assignto make the intent explicit.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/utils/sandbox.test.tspackages/cli/src/utils/sandbox.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/cli/src/utils/sandbox.test.ts (1)
packages/cli/src/utils/sandbox.ts (2)
getPassthroughEnvVars(56-76)buildSandboxEnvArgs(78-83)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
🔇 Additional comments (2)
packages/cli/src/utils/sandbox.ts (1)
78-83:buildSandboxEnvArgsdelegation/formatting looks good.
The--env,KEY=VALUEflattening (Line 79-82) is straightforward and keeps quoting concerns correctly delegated to the process boundary (argv array).packages/cli/src/utils/sandbox.test.ts (1)
7-136: Good coverage forgetPassthroughEnvVars(empty/undefined/special chars + immutability).
No functional concerns. If you want to reduce repetition later, these could be table-driven, but current clarity is fine.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/cli/src/utils/sandbox.ts:
- Around line 343-347: The sandboxEnv currently spreads all of process.env then
assigns getPassthroughEnvVars(process.env), which is redundant and exposes
unwanted vars; change it to initialize sandboxEnv from
getPassthroughEnvVars(process.env) (i.e., const sandboxEnv =
getPassthroughEnvVars(process.env)) and then add any proxy/control variables
(e.g., HTTPS_PROXY, HTTP_PROXY, NO_PROXY, or other vars set when proxyCommand is
present) conditionally to sandboxEnv so only curated environment variables are
passed into the sandbox.
🧹 Nitpick comments (4)
packages/cli/src/utils/sandbox.ts (2)
56-76: Nice extraction; consider tightening the return type (and optionally trimming).
getPassthroughEnvVars()is clean and non-mutating. If you want stronger typing (and better autocomplete for callers), you can type the keys to thePASSTHROUGH_VARIABLESunion instead ofRecord<string, string>. Also consider whether whitespace-only values should be treated as “set” (currently they are).Proposed typing tweak
const PASSTHROUGH_VARIABLES = [ 'LLXPRT_CODE_IDE_SERVER_PORT', 'LLXPRT_CODE_IDE_WORKSPACE_PATH', 'LLXPRT_CODE_WELCOME_CONFIG_PATH', 'TERM_PROGRAM', ] as const; +type PassthroughVar = (typeof PASSTHROUGH_VARIABLES)[number]; + export function getPassthroughEnvVars( env: NodeJS.ProcessEnv, -): Record<string, string> { - const result: Record<string, string> = {}; +): Partial<Record<PassthroughVar, string>> { + const result: Partial<Record<PassthroughVar, string>> = {}; for (const envVar of PASSTHROUGH_VARIABLES) { const value = env[envVar]; if (typeof value === 'string' && value.length > 0) { result[envVar] = value; } } return result; }
78-83: MakebuildSandboxEnvArgs()order explicit (avoid relying onObject.entries()ordering).Not a functional issue, but the current ordering is indirectly determined by insertion order in
getPassthroughEnvVars(). If you want the order to be defined solely byPASSTHROUGH_VARIABLES, you can build args by iterating that list (also avoids anObject.entries()allocation).Alternative implementation
export function buildSandboxEnvArgs(env: NodeJS.ProcessEnv): string[] { - return Object.entries(getPassthroughEnvVars(env)).flatMap(([key, value]) => [ - '--env', - `${key}=${value}`, - ]); + const passthrough = getPassthroughEnvVars(env); + return PASSTHROUGH_VARIABLES.flatMap((key) => { + const value = passthrough[key]; + return value ? ['--env', `${key}=${value}`] : []; + }); }packages/cli/src/utils/sandbox.test.ts (2)
10-136: Tests cover the contract well; optional: collapse the one-var-per-test cases.The suite hits the important behaviors (filtering, special chars, non-mutation). If you want to reduce maintenance, you could table-drive the single-variable assertions.
138-227: Ordering is implicitly part of the contract here—confirm that’s intentional.
buildSandboxEnvArgstests assert a specific--envordering. That’s fine if order is meant to followPASSTHROUGH_VARIABLES, but it will make refactors (e.g., usingObject.entries()differently) harder. Consider either (a) explicitly documenting/encoding that order, or (b) relaxing tests to assert set-equivalence if order isn’t important.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/utils/sandbox.test.tspackages/cli/src/utils/sandbox.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/cli/src/utils/sandbox.test.ts (2)
packages/cli/src/utils/sandbox.ts (2)
getPassthroughEnvVars(63-76)buildSandboxEnvArgs(78-83)scripts/tests/get-release-version.test.js (1)
originalEnv(28-28)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
Comment on lines
+343
to
347
| const sandboxEnv = { | ||
| ...process.env, | ||
| }; | ||
| Object.assign(sandboxEnv, getPassthroughEnvVars(process.env)); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Run the verification queries from the review comment
echo "=== Searching for sandbox-exec usage ==="
rg -n "sandbox-exec" -C3
echo -e "\n=== Searching for sandboxEnv assignment ==="
rg -n "sandboxEnv\s*=\s*\{" -C3
echo -e "\n=== Searching for getPassthroughEnvVars calls ==="
rg -n "getPassthroughEnvVars\(" -C3Repository: vybestack/llxprt-code
Length of output: 28763
🏁 Script executed:
#!/bin/bash
# Get the full context of the function containing lines 343-347
echo "=== Full context around lines 343-347 in sandbox.ts ==="
sed -n '320,370p' packages/cli/src/utils/sandbox.tsRepository: vybestack/llxprt-code
Length of output: 1788
🏁 Script executed:
#!/bin/bash
# Find and inspect the getPassthroughEnvVars function
echo "=== Finding getPassthroughEnvVars implementation ==="
rg -n "function getPassthroughEnvVars|const getPassthroughEnvVars|export.*getPassthroughEnvVars" -A 10Repository: vybestack/llxprt-code
Length of output: 839
Object.assign with getPassthroughEnvVars is functionally redundant; consider building sandboxEnv from curated vars only.
Starting sandboxEnv with { ...process.env } includes all environment variables. The subsequent Object.assign(sandboxEnv, getPassthroughEnvVars(process.env)) overwrites already-present values with identical ones, but doesn't filter out unwanted vars from the spread. If the goal is to pass only curated variables to sandbox-exec (as the helper's name suggests), construct sandboxEnv from getPassthroughEnvVars() first, then add proxy/control vars conditionally:
const sandboxEnv = getPassthroughEnvVars(process.env);
if (proxyCommand) {
sandboxEnv['HTTPS_PROXY'] = proxy;
// ... rest of proxy vars
}🤖 Prompt for AI Agents
In @packages/cli/src/utils/sandbox.ts around lines 343 - 347, The sandboxEnv
currently spreads all of process.env then assigns
getPassthroughEnvVars(process.env), which is redundant and exposes unwanted
vars; change it to initialize sandboxEnv from getPassthroughEnvVars(process.env)
(i.e., const sandboxEnv = getPassthroughEnvVars(process.env)) and then add any
proxy/control variables (e.g., HTTPS_PROXY, HTTP_PROXY, NO_PROXY, or other vars
set when proxyCommand is present) conditionally to sandboxEnv so only curated
environment variables are passed into the sandbox.
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.
TLDR
Refactor sandbox env passthrough to use a shared helper and avoid re-parsing CLI args, with unit tests.
Summary
Dive Deeper
The sandbox utilities now share a single passthrough map builder. buildSandboxEnvArgs formats that map as --env args, while sandbox execution merges the passthrough keys explicitly into the sandbox env without re-parsing CLI args. Tests cover edge cases and immutability expectations.
Reviewer Test Plan
Testing Matrix
Linked Issues
Fixes #1016