Skip to content

fix(desktop): source shell profile in teardown via wrappers#1416

Merged
Kitenite merged 2 commits into
mainfrom
kitenite/teardown-source
Feb 12, 2026
Merged

fix(desktop): source shell profile in teardown via wrappers#1416
Kitenite merged 2 commits into
mainfrom
kitenite/teardown-source

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Feb 12, 2026

Summary

  • Teardown scripts bypassed the shell wrapper system (ZDOTDIR/rcfile), so they didn't get ~/.superset/bin in PATH or source profiles the same way the terminal does
  • Now uses buildSafeEnv + getShellEnv + new getCommandShellArgs to match the terminal's profile sourcing

Changes

  • shell-wrappers.ts: Added getCommandShellArgs(shell, command) for non-interactive command execution that sources wrapper profiles inline (needed because --rcfile is ignored with -c in bash, and .zshrc is skipped in non-interactive zsh)
  • teardown.ts: Replaced getShellEnvironment() (spawned a separate login shell) with the shell wrapper approach — buildSafeEnv + getShellEnv for env, getCommandShellArgs for args
  • index.ts: Re-exports getCommandShellArgs

Test Plan

  • All 11 teardown tests pass
  • No type errors
  • Verify teardown scripts can access tools in ~/.superset/bin (e.g. agent wrappers)
  • Verify teardown works on both zsh and bash shells

Summary by CodeRabbit

  • Improvements
    • More reliable shell environment handling and command argument construction for workspace operations.
    • Better RC-file sourcing behavior so user shell commands run with expected initialization.
    • Refined process event handling to accommodate background processes that may keep resources open.

Teardown scripts used getShellEnvironment() which spawned a separate
login shell to capture env, bypassing the shell wrapper system that
terminal/setup use. This meant teardown didn't get ~/.superset/bin
in PATH or go through the same profile sourcing as the terminal.

Switch to buildSafeEnv + getShellEnv (ZDOTDIR) + a new
getCommandShellArgs helper that explicitly sources wrapper profiles
for non-interactive command execution.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 12, 2026

📝 Walkthrough

Walkthrough

Refactors workspace teardown to use new shell wrapper helpers and sanitized environment composition. Adds and exports getCommandShellArgs; teardown now computes shell-specific args, merges sanitized base env with wrapper env, and spawns the child process, listening for "exit" instead of "close".

Changes

Cohort / File(s) Summary
Shell wrapper utilities
apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts, apps/desktop/src/main/lib/agent-setup/index.ts
Add getCommandShellArgs(shell, command) and RC-file constants; update getShellArgs to reference new constant; export new helper from the module index.
Workspace teardown
apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts
Remove prior getShellEnvironment usage; build baseEnv via sanitizeEnv/buildSafeEnv, compute wrapperEnv, get shell-specific args via getCommandShellArgs, and spawn using merged env; change listener to "exit".

Sequence Diagram

sequenceDiagram
    participant Teardown as Teardown
    participant ShellWrapper as ShellWrapper
    participant Sanitizer as EnvSanitizer
    participant Spawner as ChildProcess

    Teardown->>Sanitizer: sanitizeEnv(process.env)
    Sanitizer-->>Teardown: baseEnv
    Teardown->>ShellWrapper: getCommandShellArgs(shell, command)
    ShellWrapper-->>Teardown: computedArgs
    Teardown->>Teardown: mergedEnv = baseEnv + wrapperEnv + SUPSERSET vars
    Teardown->>Spawner: spawn(shell, computedArgs, { env: mergedEnv, stdio: 'inherit' })
    Spawner-->>Teardown: process started
    Teardown->>Spawner: listen "exit"
    Spawner-->>Teardown: emit "exit"
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly Related PRs

Poem

🐇 I hopped through RCs and sanitized air,
I stitched envs together with careful care,
Commands get wrapped, args set just right,
Spawned little processes dancing into night,
A rabbit's cheer for shells now polite.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly summarizes the main change: fixing shell profile sourcing in teardown via wrappers system.
Description check ✅ Passed Description provides context, detailed changes, and test plan; follows template structure with all key sections covered.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch kitenite/teardown-source

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts (2)

88-96: Use an object parameter instead of positional arguments.

Per coding guidelines, functions with 2+ parameters should use object parameters.

Suggested refactor
-export function getCommandShellArgs(shell: string, command: string): string[] {
+export function getCommandShellArgs({ shell, command }: { shell: string; command: string }): string[] {

Update the call site in teardown.ts accordingly:

-const args = getCommandShellArgs(shell, command);
+const args = getCommandShellArgs({ shell, command });

As per coding guidelines, "Use object parameters for functions with 2 or more parameters instead of positional arguments".


89-93: Inconsistent use of -l flag between zsh and bash branches.

The zsh branch uses "-lc" (login + command) while the bash branch uses just "-c". This appears intentional since the bash rcfile wrapper already sources /etc/profile and user login profiles inline, making -l redundant for bash. However, a brief inline comment would help future readers understand this asymmetry.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Kitenite Kitenite merged commit e85037f into main Feb 12, 2026
5 of 6 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ⚠️ Neon database branch
  • ⚠️ Electric Fly.io app
  • ⚠️ Streams Fly.io app

Thank you for your contribution! 🎉

@Kitenite Kitenite deleted the kitenite/teardown-source branch February 12, 2026 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant