fix(host-service): don't crash startup when shell env probe fails#4150
Conversation
Mirrors v1's posture from apps/desktop shell-env.ts: when the login-shell snapshot fails (parse error, non-zero exit, exotic shell, rc files that redirect stdout), log a warning and fall back to process.env + augmentPathForMacOS instead of bringing down host-service startup. Reported by a user who could open v1 workspaces but never v2 — desktop main logged "Failed to start: Failed to parse shell env output - delimiter not found" and process.exit(1)'d before the HTTP server came up. v1 silently falls back via shellEnv()'s catch, which is why only v2 broke. Also pin the snapshot shell's cwd to \$HOME (backport of 4feaa3d, issue #4025) and include captured stdout/stderr in the thrown error so the next failure surfaces a diagnosable signal instead of a blind "delimiter not found".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR improves resilience and diagnostics in host-service shell environment handling. It exports a macOS PATH augmentation utility, adds diagnostic truncation to bound error output size, refactors error construction to include truncated stdout/stderr, and adds try/catch fallback in environment resolution to gracefully degrade to process.env when shell derivation fails. ChangesShell Environment Resilience & Diagnostics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR fixes a v2-only startup crash where a failed login-shell environment probe (e.g. brew aborting due to an unreadable cwd) propagated to
Confidence Score: 4/5Safe to merge; the fallback path is narrow, well-guarded, and mirrors established v1 behaviour. The two changes are independent and targeted: the cwd anchor addresses a known shell-tool abort pattern, and the try/catch fallback prevents a hard startup failure. The only gap found is that when a shell is killed by a signal before writing the delimiters, the signal value is available in scope but not included in the parse-error message, making that specific failure mode slightly harder to diagnose from logs. No files require special attention; clean-shell-env.ts has the minor diagnostic gap noted in the inline comment.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/terminal/clean-shell-env.ts | Adds HOME-anchored cwd to spawned shell (fixes brew #4025), enriches thrown errors with truncated stdout/stderr for diagnostics, and exports augmentPathForMacOS for the new fallback path. Logic is sound; one minor gap in signal reporting on the parse-failure branch. |
| packages/host-service/src/terminal/env.ts | Wraps getStrictShellEnvironment() in try/catch and falls back to a process.env snapshot + augmentPathForMacOS on failure; mirrors the v1 shell-env.ts fallback pattern and fixes the startup crash. Clean change. |
Sequence Diagram
sequenceDiagram
participant HS as host-service startup
participant RTE as resolveTerminalBaseEnv
participant GSE as getStrictShellEnvironment
participant SH as login shell
HS->>RTE: "await resolveTerminalBaseEnv()"
RTE->>GSE: "await getStrictShellEnvironment()"
GSE->>SH: "spawn(shell, [-i,-l,-c,...], cwd=HOME)"
alt probe succeeds
SH-->>GSE: "stdout with delimiters"
GSE-->>RTE: "parsed env Record"
RTE-->>HS: "shell-derived env"
else probe fails
SH-->>GSE: "error (enriched with stdout/stderr)"
GSE-->>RTE: "throws Error"
RTE->>RTE: "catch: warn + snapshotStringEnv(process.env) + augmentPathForMacOS"
RTE-->>HS: "process.env fallback"
end
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/host-service/src/terminal/clean-shell-env.ts:174-186
**Signal not surfaced in parse-error path**
When a shell is killed by a signal (e.g. an external `SIGTERM` arrives before the delimiters are written), `code` is `null` so the non-zero-exit branch is skipped, and the subsequent parse error message is built without mentioning `signal`. The user sees something like `Failed to parse shell env output - delimiter not found (shell=/bin/zsh stdout= stderr=)` with no indication the process was terminated by a signal. Since `signal` is already in scope in the `close` callback, appending `(signal ? \` signal=${signal}\` : "")` to the error string in the `catch` block would surface it, matching the treatment in the non-zero-exit branch above.
Reviews (1): Last reviewed commit: "fix(host-service): don't crash startup w..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| resolve(parseEnvOutput(Buffer.concat(stdoutBuffers).toString("utf8"))); | ||
| resolve(parseEnvOutput(stdout)); | ||
| } catch (error) { | ||
| reject(error); | ||
| const detail = error instanceof Error ? error.message : String(error); | ||
| reject( | ||
| new Error( | ||
| `${detail} (shell=${shell}` + | ||
| ` stdout=${truncateForDiagnostics(stdout)}` + | ||
| (stderr ? ` stderr=${truncateForDiagnostics(stderr)}` : "") + | ||
| ")", | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
When a shell is killed by a signal (e.g. an external SIGTERM arrives before the delimiters are written), code is null so the non-zero-exit branch is skipped, and the subsequent parse error message is built without mentioning signal. The user sees something like Failed to parse shell env output - delimiter not found (shell=/bin/zsh stdout= stderr=) with no indication the process was terminated by a signal. Since signal is already in scope in the close callback, appending (signal ? \ signal=${signal}` : "")to the error string in thecatch` block would surface it, matching the treatment in the non-zero-exit branch above.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/terminal/clean-shell-env.ts
Line: 174-186
Comment:
**Signal not surfaced in parse-error path**
When a shell is killed by a signal (e.g. an external `SIGTERM` arrives before the delimiters are written), `code` is `null` so the non-zero-exit branch is skipped, and the subsequent parse error message is built without mentioning `signal`. The user sees something like `Failed to parse shell env output - delimiter not found (shell=/bin/zsh stdout= stderr=)` with no indication the process was terminated by a signal. Since `signal` is already in scope in the `close` callback, appending `(signal ? \` signal=${signal}\` : "")` to the error string in the `catch` block would surface it, matching the treatment in the non-zero-exit branch above.
How can I resolve this? If you propose a fix, please make it concise.Changes since v0.2.10: - host-service: gh CLI is first-class for PR/issue search; failures surface in the UI instead of silently degrading. (#4140) - host-service: respawn lost terminal sessions on attach instead of dead-ending the pane after laptop sleep / daemon restart. (#4149) - host-service: don't crash startup when the shell env probe fails (e.g. user's .zshrc errors). (#4150) Also drop --draft from the CLI release workflow so the per-version release auto-publishes alongside the cli-latest rolling pointer. --prerelease stays (still required to keep desktop's auto-updater from picking up CLI releases on /releases/latest/). Push cli-v0.2.11 after this lands to fire the release pipeline.
Changes since v0.2.10: - host-service: gh CLI is first-class for PR/issue search; failures surface in the UI instead of silently degrading. (#4140) - host-service: respawn lost terminal sessions on attach instead of dead-ending the pane after laptop sleep / daemon restart. (#4149) - host-service: don't crash startup when the shell env probe fails (e.g. user's .zshrc errors). (#4150) Also drop --draft from the CLI release workflow so the per-version release auto-publishes alongside the cli-latest rolling pointer. --prerelease stays (still required to keep desktop's auto-updater from picking up CLI releases on /releases/latest/). Push cli-v0.2.11 after this lands to fire the release pipeline.
1 participant
Summary
User report (john@benomadic.co): can open v1 workspaces but every v2 attempt errors at startup with
v1 desktop main has had a
process.envfallback inapps/desktop/src/lib/.../shell-env.ts:76-95since launch — when its login-shell probe fails, it logs a warning and keeps going. v2 host-service uses its own probe inpackages/host-service/src/terminal/clean-shell-env.tsand any failure propagates toprocess.exit(1)inapps/desktop/src/main/host-service/index.ts. That asymmetry is the v1-works/v2-doesn't symptom.Changes
terminal/env.ts:resolveTerminalBaseEnv— try/catch aroundgetStrictShellEnvironment; on failure, log a warning with the underlying error and fall back to aprocess.envsnapshot +augmentPathForMacOS. Same shape as v1.terminal/clean-shell-env.ts— pin the snapshot shell'scwdto$HOME(backport of4feaa3d0, issue [bug] the title of bug report #4025; the brew "current working directory must be readable" failure mode), and include truncated stdout/stderr in the thrown error so the next user report surfaces which shell behaviour broke instead of a blind "delimiter not found".augmentPathForMacOSfromclean-shell-env.tsso the fallback path can reuse it.exec-ghalready wrapsgetStrictShellEnvironment().catch(...), so making the strict probe throw richer errors doesn't regress that consumer.Test plan
bun run lintcleantsc --noEmitclean (host-service)bun test src/terminal/— 37 pass, 0 fail[host-service] Shell env snapshot failed, falling back to process.env: <details>line so we can see his actual shell outputSummary by cubic
Stop host-service from crashing at startup when the shell env probe fails by falling back to a
process.envsnapshot (same as v1). Also improves diagnostics and stability of the shell probe.packages/host-service/src/terminal/env.ts, wrapgetStrictShellEnvironmentwith try/catch; on failure log a warning and fall back toprocess.env+augmentPathForMacOS(now exported).packages/host-service/src/terminal/clean-shell-env.ts, run the probe shell withcwdset to$HOMEto avoid unreadable-cwd issues (e.g., brew).Written for commit 4641abe. Summary will update on new commits.
Summary by CodeRabbit