fix(desktop): wrap zsh and bash to enable claude and codex proxies instead of modifying users' paths#187
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdded shell-aware wrapper scaffolding and APIs: zsh/bash wrapper creators, getShellEnv, getShellArgs, and getSupersetBinDir; replaced the former getSupersetPath-based PATH injection with shell-specific env/args and updated terminal startup to merge shell envs and use shell args. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Desktop App
participant AgentSetup as agent-setup module
participant Terminal as Terminal Manager
participant Shell as User Shell (zsh/bash)
Note over App,AgentSetup: Initialization step
App->>AgentSetup: setupAgentHooks()
AgentSetup->>AgentSetup: create ZSH_DIR / BASH_DIR
AgentSetup->>AgentSetup: create zsh/bash wrapper scripts
AgentSetup-->>App: expose getSupersetBinDir, getShellEnv, getShellArgs
Note over App,Terminal: When opening a terminal
App->>Terminal: spawn shell (shell)
Terminal->>AgentSetup: getShellEnv(shell)
AgentSetup-->>Terminal: shell-specific env map
Terminal->>AgentSetup: getShellArgs(shell)
AgentSetup-->>Terminal: shell startup args
Terminal->>Shell: launch process with merged env and args
Shell-->>Terminal: shell runs with wrappers intercepting PATH/bin resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (4)apps/desktop/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
apps/desktop/**/*.{ts,tsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/desktop/src/main/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (4)📚 Learning: 2025-11-28T01:03:47.951ZApplied to files:
📚 Learning: 2025-11-24T21:32:21.713ZApplied to files:
📚 Learning: 2025-11-28T01:03:47.951ZApplied to files:
📚 Learning: 2025-11-28T01:03:47.951ZApplied to files:
🧬 Code graph analysis (2)apps/desktop/src/main/lib/terminal-manager.ts (1)
apps/desktop/src/main/lib/agent-setup.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (7)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/agent-setup.ts (1)
42-91: Consider extracting common wrapper logic to reduce duplication.The Claude and Codex wrapper functions share ~80% of their logic (runtime resolution, error handling, exec pattern). Consider extracting a helper function to reduce maintenance burden:
function createAgentWrapper( binaryName: string, settingsOrFlags: string ): void { const wrapperPath = path.join(BIN_DIR, binaryName); const script = `#!/bin/bash # Superset wrapper for ${binaryName} # Injects notification hook settings # Find the real ${binaryName} binary at runtime, excluding our wrapper REAL_BINARY=$(which -a ${binaryName} 2>/dev/null | grep -F -v "${BIN_DIR}" | head -1) if [ -z "$REAL_BINARY" ]; then echo "Error: ${binaryName} not found in PATH" >&2 exit 1 fi exec "$REAL_BINARY" ${settingsOrFlags} "$@" `; fs.writeFileSync(wrapperPath, script, { mode: 0o755 }); console.log(\`[agent-setup] Created \${binaryName} wrapper\`); }Then call it with agent-specific settings:
createAgentWrapper("claude", '--settings "$SUPERSET_CLAUDE_SETTINGS"'); createAgentWrapper("codex", '-c \'notify=["bash","~/\${SUPERSET_DIR_NAME}/hooks/notify.sh"]\'');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/main/lib/agent-setup.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/lib/agent-setup.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/lib/agent-setup.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code
Files:
apps/desktop/src/main/lib/agent-setup.ts
apps/desktop/src/main/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Node.js modules (fs, path, os, net, etc.) can be used in main process code only
Files:
apps/desktop/src/main/lib/agent-setup.ts
🧠 Learnings (2)
📚 Learning: 2025-11-24T21:32:21.713Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/CLAUDE.md:0-0
Timestamp: 2025-11-24T21:32:21.713Z
Learning: Applies to apps/desktop/**/AGENTS.md : Document agent responsibilities, capabilities, and interaction patterns in AGENTS.md
Applied to files:
apps/desktop/src/main/lib/agent-setup.ts
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to apps/desktop/src/main/**/*.{ts,tsx} : Node.js modules (fs, path, os, net, etc.) can be used in main process code only
Applied to files:
apps/desktop/src/main/lib/agent-setup.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/agent-setup.ts (1)
apps/desktop/src/shared/constants.ts (1)
SUPERSET_DIR_NAME(21-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| # Find the real claude binary at runtime, excluding our wrapper | ||
| REAL_CLAUDE=$(which -a claude 2>/dev/null | grep -v "${BIN_DIR}" | head -1) |
There was a problem hiding this comment.
Use grep -F for fixed-string matching to avoid regex interpretation.
The BIN_DIR path is used as a grep pattern, but it contains characters like . that are regex metacharacters. While typical paths are unlikely to cause issues, paths with special regex characters (e.g., [, +, *) could lead to incorrect filtering.
Apply this diff:
-# Find the real claude binary at runtime, excluding our wrapper
-REAL_CLAUDE=$(which -a claude 2>/dev/null | grep -v "${BIN_DIR}" | head -1)
+# Find the real claude binary at runtime, excluding our wrapper
+REAL_CLAUDE=$(which -a claude 2>/dev/null | grep -F -v "${BIN_DIR}" | head -1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Find the real claude binary at runtime, excluding our wrapper | |
| REAL_CLAUDE=$(which -a claude 2>/dev/null | grep -v "${BIN_DIR}" | head -1) | |
| # Find the real claude binary at runtime, excluding our wrapper | |
| REAL_CLAUDE=$(which -a claude 2>/dev/null | grep -F -v "${BIN_DIR}" | head -1) |
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/agent-setup.ts around lines 51 to 52, the grep used
to filter out our wrapper path treats BIN_DIR as a regex which can misfilter
paths containing regex metacharacters; change the grep invocation to use
fixed-string matching (grep -F) when excluding BIN_DIR (i.e., use grep -F -v
with the BIN_DIR variable) so the path is treated literally and filtering is
reliable.
| # Find the real codex binary at runtime, excluding our wrapper | ||
| REAL_CODEX=$(which -a codex 2>/dev/null | grep -v "${BIN_DIR}" | head -1) |
There was a problem hiding this comment.
Use grep -F for fixed-string matching to avoid regex interpretation.
Same issue as in the Claude wrapper: BIN_DIR should be matched as a fixed string, not a regex pattern.
Apply this diff:
-# Find the real codex binary at runtime, excluding our wrapper
-REAL_CODEX=$(which -a codex 2>/dev/null | grep -v "${BIN_DIR}" | head -1)
+# Find the real codex binary at runtime, excluding our wrapper
+REAL_CODEX=$(which -a codex 2>/dev/null | grep -F -v "${BIN_DIR}" | head -1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Find the real codex binary at runtime, excluding our wrapper | |
| REAL_CODEX=$(which -a codex 2>/dev/null | grep -v "${BIN_DIR}" | head -1) | |
| # Find the real codex binary at runtime, excluding our wrapper | |
| REAL_CODEX=$(which -a codex 2>/dev/null | grep -F -v "${BIN_DIR}" | head -1) |
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/agent-setup.ts around lines 79 to 80, the current
grep call uses the default (regex) mode to exclude BIN_DIR which can
misinterpret special characters; change the grep invocation to use fixed-string
matching (e.g., add the -F flag) when excluding "${BIN_DIR}" so the exclusion is
treated as a literal string, ensuring the real codex binary is found reliably.
saddlepaddle
force-pushed
the
golden-ocean-48
branch
from
3d5927b to
4ec3117
Compare
saddlepaddle
changed the title
1 participant
…time
The wrapper scripts for Claude Code and Codex were hardcoding absolute paths to binaries when the app first launched. This caused notifications to fail for other users since paths like /Users/satyapatel/.nvm/... don't exist on their machines.
Now the wrappers use
which -aat runtime to find the real binary, making them portable across different machines and Node.js installations.🤖 Generated with Claude Code
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.