fix(electron): inherit full login shell environment instead of PATH only#62
fix(electron): inherit full login shell environment instead of PATH only#62FourWindff wants to merge 2 commits intojohannesjo:mainfrom
Conversation
…tion validation Introduced a new function to check for the existence of local branches. Enhanced the worktree creation process by validating the start-point reference and handling cases for empty repositories and non-existent branches more gracefully.
When launched from a .desktop file or AppImage, the app previously only inherited PATH from the user's login shell. Other environment variables (e.g. GOPATH, JAVA_HOME, SSH_AUTH_SOCK, NVM_DIR) were missing, causing tools that depend on them to malfunction inside spawned PTYs. Replace fixPath() with fixEnv() which runs `env -0` in an interactive login shell to capture all environment variables, then applies them to process.env via null-byte splitting. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Updates Electron startup to inherit a complete login-shell environment (not just PATH), and adds extra git-side safeguards around main-branch detection and worktree creation.
Changes:
- Replace
fixPath()withfixEnv()to import all environment variables from an interactive login shell intoprocess.env. - Enhance default-branch detection by checking local branch refs when remote-tracking refs aren’t available.
- Add pre-validation and clearer failures when creating git worktrees from missing/invalid start refs or empty repos.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
electron/main.ts |
Captures full login-shell environment via env -0 and applies it to process.env. |
electron/ipc/git.ts |
Adds local-branch fallback for main-branch detection and validates worktree start refs with improved errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // welcome messages). Login-only (-lc) would be quieter but would miss tools | ||
| // that are only added to PATH in .bashrc/.zshrc (e.g. nvm). We accept the | ||
| // side effects since the sentinel-based parsing discards all other output. | ||
| function fixPath(): void { | ||
| function fixEnv(): void { |
There was a problem hiding this comment.
The comment above this function still describes resolving only PATH and using sentinel-based parsing to isolate PATH output, but the implementation now captures and applies the full environment via env -0. Please update the comment to match the new behavior (and any updated trade-offs).
| const result = execFileSync( | ||
| loginShell, | ||
| ['-ilc', `printf '${sentinel}' && env -0 && printf '${sentinel}'`], |
There was a problem hiding this comment.
env -0 is a GNU extension and isn’t available on macOS’s default /usr/bin/env (BSD). Since fixEnv() runs on all non-Windows platforms, this will likely throw on darwin and silently skip inheriting env/PATH. Consider either gating this logic to Linux, or adding a fallback that doesn’t require env -0 (or uses a different portable strategy on macOS).
| const result = execFileSync( | |
| loginShell, | |
| ['-ilc', `printf '${sentinel}' && env -0 && printf '${sentinel}'`], | |
| const envDumpCommand = | |
| process.platform === 'darwin' | |
| ? `/usr/bin/perl -e 'while (($k, $v) = each %ENV) { print "${k}=${v}\\0" }'` | |
| : 'env -0'; | |
| const result = execFileSync( | |
| loginShell, | |
| ['-ilc', `printf '${sentinel}' && ${envDumpCommand} && printf '${sentinel}'`], |
| const result = execFileSync( | ||
| loginShell, | ||
| ['-ilc', `printf '${sentinel}' && env -0 && printf '${sentinel}'`], | ||
| { encoding: 'utf8', timeout: 5000 }, | ||
| ); |
There was a problem hiding this comment.
execFileSync defaults to a ~1MB maxBuffer. Dumping the full login-shell environment (especially with large vars like certificates, tokens, kubeconfig, etc.) can exceed this and cause the call to throw, leaving process.env unchanged. Consider setting an explicit, larger maxBuffer (similar to MAX_BUFFER used in electron/ipc/git.ts) or handling this failure mode more explicitly.
| const isEmptyRepo = await exec('git', ['rev-list', '-n1', '--all'], { cwd: repoRoot }) | ||
| .then(({ stdout }) => !stdout.trim()) | ||
| .catch(() => true); | ||
| if (isEmptyRepo) { | ||
| throw new Error( | ||
| 'Cannot create a worktree in a repository with no commits. ' + | ||
| 'Please make an initial commit first.', | ||
| ); |
There was a problem hiding this comment.
isEmptyRepo currently treats any failure of git rev-list -n1 --all as an empty repo (catch(() => true)). If repoRoot isn’t a git repo (or git fails for another reason), this will throw the “no commits” error, which is misleading and hides the real problem. Consider distinguishing “not a git repository” (or other exec errors) from a truly empty repo, e.g. by checking isGitRepo(repoRoot) or by inspecting stderr/exit status before deciding it’s empty.
| // Validate the start-point ref exists before attempting worktree creation | ||
| const startRef = baseBranch || 'HEAD'; | ||
| try { | ||
| await exec('git', ['rev-parse', '--verify', startRef], { cwd: repoRoot }); | ||
| } catch { | ||
| const isEmptyRepo = await exec('git', ['rev-list', '-n1', '--all'], { cwd: repoRoot }) | ||
| .then(({ stdout }) => !stdout.trim()) | ||
| .catch(() => true); | ||
| if (isEmptyRepo) { | ||
| throw new Error( | ||
| 'Cannot create a worktree in a repository with no commits. ' + | ||
| 'Please make an initial commit first.', | ||
| ); | ||
| } | ||
| throw new Error( | ||
| `Branch "${startRef}" does not exist. ` + | ||
| 'Please select a valid base branch or create the branch first.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
New behavior added here (validating startRef via rev-parse --verify and throwing user-facing errors for empty repos / missing base branches) isn’t covered by tests. Since electron/ipc/git.test.ts exists, it would be good to add unit tests that mock the git calls and assert the correct error is thrown for (1) empty repo and (2) nonexistent baseBranch.
| // Validate the start-point ref exists before attempting worktree creation | ||
| const startRef = baseBranch || 'HEAD'; | ||
| try { | ||
| await exec('git', ['rev-parse', '--verify', startRef], { cwd: repoRoot }); |
There was a problem hiding this comment.
The PR title/description focus on inheriting the full login-shell environment for Electron, but this file also adds new git behaviors (main branch detection changes and worktree start-ref validation with new user-facing errors). Please either update the PR description to cover these git changes (and why they’re included) or split them into a separate PR to keep scope clear.
johannesjo
left a comment
johannesjo
left a comment
There was a problem hiding this comment.
Code Review: fix(electron): inherit full login shell environment instead of PATH only
Summary
This PR replaces fixPath() (which only captured PATH from the user's login shell) with fixEnv(), which runs env -0 inside an interactive login shell to capture all environment variables. It also adds localBranchExists() and start-point ref validation to git.ts for safer worktree creation. The motivation is sound — missing env vars like ANTHROPIC_BASE_URL, GOPATH, SSH_AUTH_SOCK etc. are a real pain point when launching from .desktop files or AppImage.
Blocking Issue: env -0 breaks macOS
This is the primary reason for requesting changes.
env -0 (null-byte-separated output) is a GNU coreutils extension. macOS ships BSD env, which does not support the -0 flag. Running this on macOS will cause env to error out, which means fixEnv() will hit the catch block and silently fail — leaving process.env untouched. This is worse than the old fixPath() behavior, which worked correctly on both platforms.
Since the project explicitly targets macOS and Linux (per CLAUDE.md), this breaks one of the two supported platforms.
Recommended fix — one of:
- Gate
env -0behind a Linux-only check and keep the originalfixPath()sentinel approach for macOS:if (process.platform === 'darwin') { fixPath(); // existing sentinel-based PATH-only approach } else { fixEnv(); // full env capture via env -0 }
- Use a cross-platform alternative that works on both macOS and Linux, e.g.:
printenvwith newline parsing (but values can contain newlines, so fragile)envwithout-0combined with a known-safe delimiter approach- Spawn
node -e "process.stdout.write(JSON.stringify(process.env))"inside the login shell — this is fully cross-platform and handles all edge cases
Security / Environment Pollution Concern
fixEnv() blindly overwrites every key in process.env with whatever the login shell emits. The old fixPath() was deliberately scoped to PATH only. The new approach means:
- Any variable set by shell startup scripts (including potentially harmful ones) propagates into the Electron process and every spawned PTY
- Electron-internal variables that were set before
fixEnv()runs could be silently overwritten - If a shell startup script
exports something unexpected (e.g.,NODE_OPTIONS,ELECTRON_RUN_AS_NODE,LD_PRELOAD), it could change Electron's behavior in surprising ways
Suggestion: Consider at minimum a denylist of keys that should never be overwritten:
const PROTECTED_KEYS = new Set(['ELECTRON_RUN_AS_NODE', 'NODE_OPTIONS', 'LD_PRELOAD', 'LD_LIBRARY_PATH']);
for (const entry of envBlock.split('\0')) {
const eqIdx = entry.indexOf('=');
if (eqIdx <= 0) continue;
const key = entry.slice(0, eqIdx);
if (PROTECTED_KEYS.has(key)) continue;
process.env[key] = entry.slice(eqIdx + 1);
}Bundled Concerns: git.ts changes
The git.ts changes (local branch fallback in detectMainBranch, ref validation before createWorktree) are unrelated to the env fix. They are individually good changes (and were approved in the now-merged PR #61 which addresses the same concerns), but bundling them here makes the PR harder to review and creates potential merge conflicts.
Suggestion: Remove the git.ts changes from this PR, since PR #61 (now merged) already addresses the same issues. If this PR's git changes go beyond what #61 covers, please rebase onto main and resolve any overlap.
Minor Issues
-
Silent
catchwith no logging context: Thecatchblock logs[fixEnv] Failed to resolve login shell environment:which is good, but on macOS this will fire on every launch with a crypticenv: illegal option -- 0error. Users may not understand why. A platform-specific message or approach would help. -
printf '${sentinel}'uses single quotes inside double-quoted shell arg: This is correct (the single quotes are passed to the shell), but it's subtle. A brief comment would help future maintainers.
Overall
The core idea is valuable and addresses a real user pain point. However, the env -0 incompatibility with macOS is a blocker — it would silently regress the env-resolution behavior on half the supported platforms. Please address the macOS compatibility issue, consider the env-pollution denylist, and ideally split out the unrelated git.ts changes.
Generated by Claude Code
|
Thank you very much! <3 |
johannesjo
left a comment
johannesjo
left a comment
There was a problem hiding this comment.
No new commits have been pushed since the CHANGES_REQUESTED review on 2026-04-12. The blocking issue remains: env -0 is a GNU coreutils extension that does not exist on macOS BSD env, so fixEnv() will silently fail on every macOS launch — leaving process.env untouched. This is worse than the original fixPath() behavior which worked correctly on both platforms.
Please address the macOS compatibility issue (platform guard or cross-platform alternative) before requesting re-review.
Description
When launched from a
.desktopfile or AppImage, the app previously only inheritedPATHfrom the user's login shell viafixPath(). All other environment variables were lost, which caused two issues:GOPATH,JAVA_HOME,SSH_AUTH_SOCK, andNVM_DIRwere not available in spawned PTYs, causing tools that depend on them to fail.ANTHROPIC_BASE_URL) configured in their shell profile. Since onlyPATHwas inherited, these variables were missing and Claude could not connect at all.This PR replaces
fixPath()withfixEnv(), which runsenv -0inside an interactive login shell to capture all environment variables (null-byte separated), then applies them toprocess.env. This ensures spawned PTYs have the same complete environment as the user's terminal.Issues Resolved
.desktoplauncher only inheritedPATHfrom the login shell; all other environment variables were lost in spawned PTYs.ANTHROPIC_BASE_URL) were not passed through to the PTY.