fix(tui): harden PowerShell invocation for safe Windows execution

[Hmbown]

Summary

Translates applicable PowerShell safe-invocation invariants into Codewhale's existing ShellDispatcher (no skill bundling/copy):

• Prefer pwsh detection order already present
• Add -NoLogo -NoProfile -NonInteractive for all PowerShell spawns
• Capture native $LASTEXITCODE after the payload
• Use temporary .ps1 + -File for multiline / heavily quoted / non-ASCII scripts
• Preserve cmd.exe raw-arg quoting (Bug Report: Git Commit Message with Spaces Causes Pathspec Parse Error #1691) and Windows Job Object process-tree cleanup

Test plan

• cargo test -p codewhale-tui --bin codewhale-tui --locked shell_dispatcher
• cargo fmt --all -- --check
• staged gitleaks clean

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @Hmbown's task in 4m 10s —— View job

---

Claude's Review — PowerShell shell hardening

Scope reviewed: crates/tui/src/shell_dispatcher.rs, crates/tui/src/sandbox/mod.rs, crates/tui/src/tools/shell/tests.rs (diff vs origin/main).

• Gather context (diff, changed files)
• Review crates/tui/src/shell_dispatcher.rs
• Review crates/tui/src/sandbox/mod.rs
• Review crates/tui/src/tools/shell/tests.rs
• Check provider/model route-safety invariant (N/A — no provider/model routing touched)
• Post review summary

Note: cargo/git fetch invocations required interactive approval that wasn't available in this run, so I reviewed statically (reading the full diff and surrounding code) rather than compiling or executing the stated test plan. The reasoning below is based on tracing the actual control flow, not a build.

Findings (posted as inline comments)

1. Correctness — crates/tui/src/shell_dispatcher.rs:116-124 (powershell_exit_aware_command): the -Command path only runs for single-line payloads (multiline goes through -File), so the appended ; if ($null -ne $LASTEXITCODE ...) { exit $LASTEXITCODE } lands on the same line as the user command. If that command contains an unquoted #, PowerShell treats the rest of the line — including the appended check — as a comment, so the exit-code capture this PR is meant to add silently becomes a no-op for that class of command. This is a regression vs. main (nothing was appended after the payload before). Suggest joining with \n instead of ;.

2. Resource leak — crates/tui/src/shell_dispatcher.rs:127-151 (write_temp_ps1): the temp .ps1 script written for multiline/heavily-quoted/non-ASCII payloads is never deleted in the production path (only the unit test cleans up after itself). Every such invocation leaks a file in %TEMP%, which also retains the command text. The crate already uses the tempfile crate elsewhere (auto-cleanup on drop) — worth reusing that here instead of std::fs::File::create.

3. Minor/simplification — crates/tui/src/sandbox/mod.rs display_command (~L180-232): since CommandSpec::shell (the only constructor used for shell-wrapped commands) now always sets requested_command = Some(...), display_command returns early before reaching the legacy -c/cmd/PowerShell-prefix-stripping fallback chain. The only other constructor, CommandSpec::program (used once, in main.rs:8103), passes plain program+args that won't match those shapes either. That whole fallback block looks dead in production now (only reachable via the manually-constructed CommandSpec { .. } literal in the test at ~L669) — a good candidate to trim in a follow-up. Not blocking.

What's solid

• requested_command on CommandSpec is a clean fix for showing users the real command instead of the internal encoding/exit-code/temp-file wrapping — good call, and display_command() is now used correctly by both changed tests.
• -NoLogo -NoProfile -NonInteractive and the pwsh-preferred detection order match the PR description.
• cmd.exe raw-arg quoting preservation (issue Bug Report: Git Commit Message with Spaces Causes Pathspec Parse Error #1691) is intact — verified the build_command branch still special-cases /C + raw_arg on Windows.
• Test updates in tools/shell/tests.rs and sandbox/mod.rs correctly stopped asserting exact wrapped-string shapes and instead assert the payload survives intact in one argv slot — more robust to the new per-shell wrapping.

No provider/model routing code is touched by this PR, so the v0.8.65 EPIC #2608 invariant doesn't apply here.

[claude]

Claude finished @Hmbown's task in 1m 50s —— View job

---

Claude's re-review — PowerShell shell hardening (post-fix)

• Gather context (new commits, diff vs branch base)
• Verify fix for exit-code #-comment swallowing
• Verify fix for temp .ps1 file leak / cleanup sweep
• Check bc7ce58 (display_command simplification) and aaaf04a (clippy)
• Post updated review summary

Note: as in the prior pass, cargo test/cargo clippy required interactive approval not available in this run, so this is a static re-review of the diff (0a741be..dbdfef2), not a compiled/executed verification of the stated test plan.

Both prior findings are fixed

1. #-comment swallowing exit capture — crates/tui/src/shell_dispatcher.rs:116-124 now joins the appended $LASTEXITCODE check with \n instead of ;, so it lands on its own line and a trailing unquoted # in the payload can no longer comment it out. New regression test powershell_trailing_comment_cannot_swallow_exit_capture (line ~592) exercises a git log --oneline -5 # recent payload and asserts the capture starts on a fresh line. Correct fix.

2. Temp .ps1 leak — write_temp_ps1 now appends TEMP_PS1_TAIL, which captures $LASTEXITCODE, self-deletes via Remove-Item -LiteralPath $MyInvocation.MyCommand.Path before the exit line (so a nonzero exit can't skip cleanup — sound, since PowerShell parses/reads the whole file before executing), then propagates the exit code. sweep_stale_temp_ps1 additionally does a best-effort removal of any codewhale-shell-*.ps1 older than one hour on each new invocation, covering killed/crashed processes. The 1-hour threshold avoids racing a concurrently-starting script. Test stale_temp_ps1_scripts_are_swept backdates a file and confirms removal. Correct fix; reasonable design given tempfile's drop-based cleanup wouldn't work here (the file must outlive build_command_parts until the child process runs it).

Follow-up commits also check out

• aaaf04a (clippy): str::is_ascii() replaces the char-iteration ASCII check (fixes needless_character_iteration), and ShellKind::needs_command_flag is gated #[cfg(test)] — confirmed it's only referenced from unit tests, so this is correct, not a coverage loss.
• bc7ce58: adds CommandSpec::requested_command (Some(command) from CommandSpec::shell, None from CommandSpec::program) and makes display_command() return it directly when present, rather than reverse-parsing dispatcher argv shape. This is the right fix for the root cause (display logic duplicating the dispatcher's wrapping contract) and resolves item 3 from the original review (the legacy fallback chain in display_command was correct-but-dead in production) by giving that code a real purpose as a fallback for non-shell()-constructed specs, rather than leaving it unreachable. Updated tests assert intent (single argv carrier, no tokenized fragments, round-trip) instead of exact argv shape, which is more robust to future dispatcher wrapping changes.

No new issues found in the fix commits. The PR looks ready from a correctness standpoint; final call on merge is Hunter's per the maintainer test-plan checklist (cargo test / cargo fmt / gitleaks already checked off in the PR description).