fix(shell): avoid interactive job control in environment probes - #11275
LouisDeconinck wants to merge 2 commits into
Conversation
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughLogin-shell probes now choose arguments by shell type. zsh uses ChangesLogin shell probe
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant DesktopShellEnvironment
participant ShellProbe
participant LoginShell
participant Environment
DesktopShellEnvironment->>ShellProbe: select arguments by shell
ShellProbe->>LoginShell: run zsh with +m -ilc or other shells with -lc
LoginShell->>Environment: load login startup files
Environment-->>LoginShell: set PATH
LoginShell-->>DesktopShellEnvironment: return captured PATH
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The shell probe changes preserve the intended zsh and non-zsh invocation forms, with no remaining actionable merge risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — This is a narrowly scoped shell-probe bug fix that removes interactive job control while preserving the existing environment-capture and merge flow, with coverage for both affected call sites. A separate high-severity correctness finding flags that interactive-only PATH configuration such as zsh Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
-lc skipped ~/.zshrc, dropping PATH entries from version managers and custom bin dirs. zsh keeps -i but gets +m, which clears MONITOR before startup so the probe still never calls tcsetpgrp() on a foreign controlling TTY. bash stays -lc (login shells never read ~/.bashrc and bash ignores +m at startup), fish reads config.fish for login shells.
|
Follow-up on the review finding: |
What Changed
Both login-shell environment probes now run
shell -lcinstead ofshell -ilc:readEnvironmentFromLoginShellinpackages/shared/src/shell.ts(used by the server'sfixPath, which runs on the remote duringt3 serveboot over SSH)login-shellprobe inapps/desktop/src/shell/DesktopShellEnvironment.ts(the local desktop twin)Both call sites changed together so local and remote behavior stay consistent. Marker-delimited capture, merge behavior, timeouts, and Windows probing are unchanged.
Why
-imakes the shell initialize job control, which callstcsetpgrp()on the session's controlling TTY. When the remote SSH session inherits a controlling TTY owned by a different process group (e.g.tailscaled's built-in SSH server started from an interactive terminal), the kernel stops the probe with SIGTTOU before it runs anything.desktop:ensure-ssh-environmentthen hangs until the 90s launch timeout, and the stopped remote process group is leaked.Environment capture only needs login-shell semantics (
/etc/profile,~/.profile,~/.zprofile, fish login config), not interactive job control.-lcis also what the SSH bootstrap itself already uses (sh -l -s, #7213). Verified on the affected host in the issue report:bash -lcreturns instantly wherebash -ilchangs.Tests assert the probe args are
-lc(no-i) and that PATH/environment capture still works in both packages.Tests run:
vp test run packages/shared/src/shell.test.ts apps/desktop/src/shell/DesktopShellEnvironment.test.ts— 48 passedvp run --filter @t3tools/shared --filter @t3tools/desktop --filter t3 typecheck— passedvp lint/vp fmt --checkon the touched files — cleanOut of scope per the triage: remote process-group cleanup (SIGCONT/kill on launch timeout) and the offline-environment retry cadence (#4144) are follow-ups.
Fixes #11001
Checklist
Made with SWE-2 via Devin.
Summary by CodeRabbit
Bug Fixes
PATHdetection for zsh while preventing terminal job-control interruptions during shell checks.Tests
PATHapplication.