fix(terminal): inherit full parent env for Unix PTY to fix zsh#840
fix(terminal): inherit full parent env for Unix PTY to fix zsh#840pedramamini merged 2 commits intoRunMaestro:rcfrom
Conversation
The Unix PTY terminal env was a minimal 6-variable set (HOME, USER, SHELL, TERM, LANG, PATH). While bash tolerates this, zsh depends on inherited variables (FPATH, XDG_*, TMPDIR, etc.) for ZLE and plugin initialization. Without them, zsh input becomes invisible and navigation breaks. Inherit full parent env (matching Windows behavior), strip Electron/IDE vars, and override TERM/LANG/PATH with controlled values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughPTY environment construction now inherits the full parent Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Greptile SummaryThis PR fixes zsh breakage in Unix PTY terminal sessions by switching from a minimal 6-variable environment to inheriting the full parent Confidence Score: 5/5Safe to merge — the logic change is well-scoped, Windows behaviour is untouched, and only two minor doc-comment inconsistencies remain. All findings are P2 documentation-only issues. The runtime behaviour is correct: full parent env is inherited on Unix, Electron/IDE vars are stripped, and TERM/LANG/PATH are overridden with controlled values. Platform detection reads process.platform at call time, so test mocking is reliable. Existing 41/41 tests pass plus two new targeted tests. No files require special attention Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[buildPtyTerminalEnv called] --> B{isWindows?}
B -- Yes --> C["env = { ...process.env, TERM: 'xterm-256color' }"]
B -- No --> D["env = { ...process.env, TERM, LANG, PATH: buildUnixBasePath() }"]
D --> E[Delete each key in STRIPPED_ENV_VARS\nELECTRON_*, CLAUDECODE, NODE_ENV…]
C --> F{env.VIMINIT set?}
E --> F
F -- No --> G["env.VIMINIT = process.env.VIMINIT || 'set nocompatible | set esckeys'"]
F -- Yes --> H[Keep inherited VIMINIT]
G --> I{shellEnvVars provided?}
H --> I
I -- Yes --> J[Merge shellEnvVars with ~/ expansion]
I -- No --> K[Return env]
J --> K
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/process-manager/utils/__tests__/envBuilder.test.ts (1)
481-500: Consider testing allSTRIPPED_ENV_VARSfor completeness.This test verifies 4 of 8 variables in
STRIPPED_ENV_VARS. For consistency with the comprehensive test at lines 196-215 (which testsbuildChildProcessEnv), consider adding the missing variables:ELECTRON_EXTRA_LAUNCH_ARGS,CLAUDE_CODE_ENTRYPOINT,CLAUDE_AGENT_SDK_VERSION, andCLAUDE_CODE_ENABLE_SDK_FILE_CHECKPOINTING.📝 Proposed test enhancement
it('should strip Electron/IDE variables from PTY environment on Unix', () => { const originalPlatform = process.platform; Object.defineProperty(process, 'platform', { value: 'linux' }); try { process.env.ELECTRON_RUN_AS_NODE = '1'; process.env.ELECTRON_NO_ASAR = '1'; + process.env.ELECTRON_EXTRA_LAUNCH_ARGS = '--enable-features=something'; process.env.CLAUDECODE = 'true'; + process.env.CLAUDE_CODE_ENTRYPOINT = '/path/to/entrypoint'; + process.env.CLAUDE_AGENT_SDK_VERSION = '1.0.0'; + process.env.CLAUDE_CODE_ENABLE_SDK_FILE_CHECKPOINTING = 'true'; process.env.NODE_ENV = 'test'; const env = buildPtyTerminalEnv({}); expect(env.ELECTRON_RUN_AS_NODE).toBeUndefined(); expect(env.ELECTRON_NO_ASAR).toBeUndefined(); + expect(env.ELECTRON_EXTRA_LAUNCH_ARGS).toBeUndefined(); expect(env.CLAUDECODE).toBeUndefined(); + expect(env.CLAUDE_CODE_ENTRYPOINT).toBeUndefined(); + expect(env.CLAUDE_AGENT_SDK_VERSION).toBeUndefined(); + expect(env.CLAUDE_CODE_ENABLE_SDK_FILE_CHECKPOINTING).toBeUndefined(); expect(env.NODE_ENV).toBeUndefined(); } finally { Object.defineProperty(process, 'platform', { value: originalPlatform }); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/utils/__tests__/envBuilder.test.ts` around lines 481 - 500, The test for buildPtyTerminalEnv currently asserts only half of the entries from STRIPPED_ENV_VARS; update the test in envBuilder.test.ts to include assertions for the remaining variables ELECTRON_EXTRA_LAUNCH_ARGS, CLAUDE_CODE_ENTRYPOINT, CLAUDE_AGENT_SDK_VERSION, and CLAUDE_CODE_ENABLE_SDK_FILE_CHECKPOINTING (set them on process.env before calling buildPtyTerminalEnv and assert they are undefined on the returned env). Ensure you still toggle the platform to 'linux' and restore it in the finally block, and reference the STRIPPED_ENV_VARS concept to match the comprehensive coverage done for buildChildProcessEnv.src/main/process-manager/utils/envBuilder.ts (1)
62-62: Docstring is now inconsistent with Unix behavior.Line 62 states "Terminal sessions do NOT strip Electron/IDE variables" but the Unix code path at lines 80-82 now strips
STRIPPED_ENV_VARS. This docstring should be updated to reflect the platform-specific difference.📝 Proposed docstring update
- * `@note` Terminal sessions do NOT strip Electron/IDE variables (full environment inherited on Windows) + * `@note` On Windows, the full environment is inherited without stripping. On Unix/Linux/macOS, Electron/IDE variables are stripped to avoid zsh/plugin issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/utils/envBuilder.ts` at line 62, Update the top docstring to reflect platform-specific behavior: state that on Windows terminal sessions inherit the full environment (Electron/IDE vars are not stripped) but on Unix-like systems the code path removes variables listed in STRIPPED_ENV_VARS; mention the symbol STRIPPED_ENV_VARS and the env-building function (e.g., envBuilder / the function that performs the Unix code path) so readers know the docstring matches the actual behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/process-manager/utils/__tests__/envBuilder.test.ts`:
- Around line 481-500: The test for buildPtyTerminalEnv currently asserts only
half of the entries from STRIPPED_ENV_VARS; update the test in
envBuilder.test.ts to include assertions for the remaining variables
ELECTRON_EXTRA_LAUNCH_ARGS, CLAUDE_CODE_ENTRYPOINT, CLAUDE_AGENT_SDK_VERSION,
and CLAUDE_CODE_ENABLE_SDK_FILE_CHECKPOINTING (set them on process.env before
calling buildPtyTerminalEnv and assert they are undefined on the returned env).
Ensure you still toggle the platform to 'linux' and restore it in the finally
block, and reference the STRIPPED_ENV_VARS concept to match the comprehensive
coverage done for buildChildProcessEnv.
In `@src/main/process-manager/utils/envBuilder.ts`:
- Line 62: Update the top docstring to reflect platform-specific behavior: state
that on Windows terminal sessions inherit the full environment (Electron/IDE
vars are not stripped) but on Unix-like systems the code path removes variables
listed in STRIPPED_ENV_VARS; mention the symbol STRIPPED_ENV_VARS and the
env-building function (e.g., envBuilder / the function that performs the Unix
code path) so readers know the docstring matches the actual behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fdb3c6e-3ead-457b-8593-ea43bbdd3e7a
📒 Files selected for processing (2)
src/main/process-manager/utils/__tests__/envBuilder.test.tssrc/main/process-manager/utils/envBuilder.ts
…ge, fix stale docstring Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the contribution, @chr1syy! This is a well-scoped and well-motivated fix. Review notes:
One truly minor nit (not blocking): the Looks good to merge! 🎉 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/process-manager/utils/envBuilder.ts (1)
81-83: Re-apply stripping after custom env merge (or narrow the doc contract).At Line 81-83, stripping happens before
shellEnvVarsare applied (Line 94+), so stripped keys can be reintroduced. That conflicts with the wording at Line 62-63 unless the intent is “strip inherited parent vars only.”♻️ Proposed adjustment
} else { const basePath = buildUnixBasePath(); env = { ...process.env, TERM: 'xterm-256color', LANG: process.env.LANG || 'en_US.UTF-8', PATH: basePath, }; - for (const key of STRIPPED_ENV_VARS) { - delete env[key]; - } } @@ if (shellEnvVars && Object.keys(shellEnvVars).length > 0) { const homeDir = os.homedir(); for (const [key, value] of Object.entries(shellEnvVars)) { env[key] = value.startsWith('~/') ? path.join(homeDir, value.slice(2)) : value; } } + + // Ensure stripped vars never leak into Unix PTY sessions, even via custom merges. + if (!isWindows()) { + for (const key of STRIPPED_ENV_VARS) { + delete env[key]; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/utils/envBuilder.ts` around lines 81 - 83, Stripping of inherited vars is performed too early: STRIPPED_ENV_VARS are deleted from env before shellEnvVars are merged, so those keys can be reintroduced; either move the deletion loop that iterates STRIPPED_ENV_VARS to after the merge with shellEnvVars (or delete those keys from shellEnvVars before merging) so they cannot be reintroduced, and update the doc/comment if the intent is to only strip parent vars; look for the env variable and the merge with shellEnvVars in envBuilder.ts and apply the deletion after that merge (or adjust the contract).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/process-manager/utils/envBuilder.ts`:
- Around line 81-83: Stripping of inherited vars is performed too early:
STRIPPED_ENV_VARS are deleted from env before shellEnvVars are merged, so those
keys can be reintroduced; either move the deletion loop that iterates
STRIPPED_ENV_VARS to after the merge with shellEnvVars (or delete those keys
from shellEnvVars before merging) so they cannot be reintroduced, and update the
doc/comment if the intent is to only strip parent vars; look for the env
variable and the merge with shellEnvVars in envBuilder.ts and apply the deletion
after that merge (or adjust the contract).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25038c1a-f7d1-426f-9b2f-2097654c161f
📒 Files selected for processing (2)
src/main/process-manager/utils/__tests__/envBuilder.test.tssrc/main/process-manager/utils/envBuilder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/process-manager/utils/tests/envBuilder.test.ts
3 participants
Summary
Test plan
:qrenders correctly🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Bug Fixes