feat(infra): T-226 shell detector for $SHELL / pwsh / cmd.exe (closes #121)#133
feat(infra): T-226 shell detector for $SHELL / pwsh / cmd.exe (closes #121)#133mpiton wants to merge 1 commit into
Conversation
Implement `infrastructure/pty/shell_detector.rs` per ARCHI §11.2.
`detect()` returns a `ShellSpec { program, args }` that `PtyPool::spawn`
will hand to `portable_pty::CommandBuilder`:
- Unix: honours `$SHELL`, falls back to `/bin/bash` when unset or empty.
- Windows: prefers `pwsh` (PowerShell 7) over `powershell.exe` over
`%COMSPEC%` / `cmd.exe`. `-NoLogo` on PowerShell variants.
`ShellSpec` derives `Serialize + Deserialize + TS` (exported to
`src/shared/types/ShellSpec.ts`) so it can flow through debug IPC later.
The Windows priority chain lives in a pure `resolve_windows_shell`
helper that is unit-tested on every platform; Linux CI runners exercise
the full pwsh > powershell > COMSPEC > cmd.exe fallback ladder without
needing real PowerShell binaries. Three `temp_env::with_var` tests cover
the Unix env-var fallback (set, unset, empty).
`which = "8.0.2"` added via `cargo add --target 'cfg(windows)'` so the
Unix build keeps the dependency surface lean.
Closes #121
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThis PR implements platform-aware shell detection for PTY spawning. It introduces ChangesShell Detection Infrastructure
Possibly related issues
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/infrastructure/pty/shell_detector.rs`:
- Around line 24-27: The env var handling for program resolution (e.g., the
std::env::var("SHELL") block that assigns program) should treat whitespace-only
values as empty; change the chain to trim the retrieved string before checking
emptiness (for example, map or filter using value.trim()) so SHELL=" " won't
be accepted and the fallback ("/bin/bash") will be used; apply the same
trim-before-empty-check fix to the COMSPEC handling at the other occurrence
(lines referenced around the COMSPEC resolution) so both std::env::var("SHELL")
and std::env::var("COMSPEC") treat whitespace-only values as empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4517983d-7209-4ddb-bbf4-b0bef15e1ea8
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.mdsrc-tauri/Cargo.tomlsrc-tauri/src/infrastructure/pty/mod.rssrc-tauri/src/infrastructure/pty/shell_detector.rssrc/shared/types/ShellSpec.ts
| let program = std::env::var("SHELL") | ||
| .ok() | ||
| .filter(|value| !value.is_empty()) | ||
| .unwrap_or_else(|| "/bin/bash".into()); |
There was a problem hiding this comment.
Treat whitespace-only env vars as empty before fallback resolution.
SHELL=" " or COMSPEC=" " currently bypasses the empty check and can return an unusable program value. Trim first, then apply the emptiness fallback.
💡 Suggested patch
pub fn detect() -> ShellSpec {
let program = std::env::var("SHELL")
.ok()
- .filter(|value| !value.is_empty())
+ .map(|value| value.trim().to_string())
+ .filter(|value| !value.is_empty())
.unwrap_or_else(|| "/bin/bash".into());
ShellSpec {
program,
args: Vec::new(),
@@
let program = comspec
- .filter(|value| !value.is_empty())
+ .map(str::trim)
+ .filter(|value| !value.is_empty())
.map(String::from)
.unwrap_or_else(|| "cmd.exe".into());Also applies to: 67-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src-tauri/src/infrastructure/pty/shell_detector.rs` around lines 24 - 27, The
env var handling for program resolution (e.g., the std::env::var("SHELL") block
that assigns program) should treat whitespace-only values as empty; change the
chain to trim the retrieved string before checking emptiness (for example, map
or filter using value.trim()) so SHELL=" " won't be accepted and the fallback
("/bin/bash") will be used; apply the same trim-before-empty-check fix to the
COMSPEC handling at the other occurrence (lines referenced around the COMSPEC
resolution) so both std::env::var("SHELL") and std::env::var("COMSPEC") treat
whitespace-only values as empty.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06c65e161f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * system shell (so `.bashrc`, `.zshrc`, aliases and `PATH` are honoured) and | ||
| * then injects the Claude CLI command through stdin. | ||
| */ | ||
| export type ShellSpec = { program: string, args: Array<string>, }; |
There was a problem hiding this comment.
Adding ShellSpec.ts without updating src/shared/types/index.ts introduces deterministic codegen drift: scripts/codegen-types.ts rebuilds the barrel from all *.ts files in that directory, so it will add export type * from "./ShellSpec"; on the next pnpm run codegen. The CI codegen-diff gate in .github/workflows/ci.yml checks for exactly this drift and will fail even if runtime code is unchanged.
Useful? React with 👍 / 👎.
1 participant
Summary
Implements
infrastructure/pty/shell_detector.rsper ARCHI §11.2 so T-227 (PtyPool) has a ready-madeShellSpec { program, args }to feed toportable_pty::CommandBuilder. No agent code, no Tauri code — pure platform detection.Closes #121.
Why
Forgent never spawns
claudedirectly. It always spawns the user's system shell (so.bashrc/.zshrc, aliases, andPATHare honoured) and then injects the Claude CLI command via stdin (ADR-014). The shell detector is the first brick of theinfrastructure/pty/module, dependency-free of any agent / Tauri code, runnable in parallel with T-225.Changes
src-tauri/src/infrastructure/pty/shell_detector.rsShellSpecvalue object +detect()per platform + pureresolve_windows_shellhelpersrc-tauri/src/infrastructure/pty/mod.rspub mod shell_detector;src-tauri/Cargo.tomlcargo add which --target 'cfg(windows)'→which = "8.0.2"under[target.'cfg(windows)'.dependencies]src/shared/types/ShellSpec.tsCHANGELOG.md[Unreleased] → AddedentryPlatform logic
std::env::var("SHELL").ok().filter(non-empty).unwrap_or("/bin/bash")→ShellSpec { program, args: [] }.which("pwsh")first (→-NoLogo), thenwhich("powershell")(→-NoLogo), then%COMSPEC%(filtered for empty), finallycmd.exe.Testability
The Windows priority chain is extracted into a pure
resolve_windows_shell(pwsh_on_path, powershell_on_path, comspec)helper gated with#[cfg(any(test, windows))]. Five branch tests run on every CI platform (Linux included), so the pwsh → powershell → COMSPEC → cmd.exe ladder cannot regress silently. Realwhich::whichcalls only happen inside the platform-#[cfg(windows)]detect()wrapper.Acceptance criteria
pub fn detect() -> ShellSpecreturns the right program per platformstd::env::var("SHELL"), falls back to/bin/bashwhen unset or emptywhich("pwsh")→which("powershell")→$COMSPEC(or empty) →cmd.exe-NoLogoonpwsh/powershell, none elsewhereShellSpecderivesSerialize + Deserialize + TS, exported tosrc/shared/types/ShellSpec.tstemp_env::with_var)#[cfg(windows)], file still compiles on Linux CITesting
cargo fmt --checkcargo clippy --all-targets -- -D warningsserde(transparent)newtypes — unrelated)cargo test --workspaceresolve_windows_shellcases)cargo deny checkpnpm exec tsc -bShellSpec.tsintegrates cleanly)Tests added
windows_resolver::resolve_prefers_pwsh_when_availablewindows_resolver::resolve_falls_back_to_powershell_when_pwsh_missingwindows_resolver::resolve_uses_comspec_when_no_powershell_variantswindows_resolver::resolve_falls_back_to_cmd_when_comspec_unsetwindows_resolver::resolve_falls_back_to_cmd_when_comspec_emptyunix::detect_uses_shell_env_var_when_setunix::detect_falls_back_to_bin_bash_when_shell_unsetunix::detect_falls_back_to_bin_bash_when_shell_emptywindows::detect_returns_non_empty_program_on_windows_runner(integration smoke, runs only on Windows CI)Adversarial review notes
Three parallel reviewers (security / logic / clean-code) ran on the diff. Resolved:
/// Detect the appropriate shelldocstring on both platform branches → replaced with platform-specific contracts.resolve_windows_shellhelper with full branch coverage on every runner..to_string()everywhere →.into()for&'static str → Stringconversions, matchingerror.rs.temp_env::with_var(..., detect)function-pointer form → closure form|| { ... }, matchingconfig.rs/sentry_init.rs.Deserializederive vs the canonicaldomain/tasks/model.rspattern → added.Accepted as out of scope (consistent with ADR-014 trust model):
which("pwsh")resolving a user-writable directory (LOW security) — Forgent explicitly inherits the user's$PATHso.bashrc/ aliases work; same risk surface as the user typingpwshin their own terminal.which()reporting a binary that fails to execute (LOW logic) — out of scope for T-226; spawn-time error handling lives in T-227PtyPool.Dependencies
None — runs in parallel with T-225 per the original ticket.
Generated by APEX workflow
Summary by CodeRabbit
New Features