fix(acp): honor configured terminal shell

[doublecurry]

Summary

• Route ACP shell command fallbacks through the default shell selected in General Settings.
• Load the persisted selection at desktop and server startup, and update live model sessions when it changes.
• Preserve direct execution for structured command-and-argument requests.

Verification

• cargo test --offline --no-default-features --lib shell_config_tests -- --nocapture
• cargo test --offline --no-default-features --lib terminal_shell_setting_persists_and_updates_live_runtime -- --nocapture
• cargo check --offline --no-default-features --bin codeg-server
• cargo clippy --offline --no-default-features --bin codeg-server -- -D warnings

[xintaofei]

Thanks for this — the plumbing here is genuinely nice work. The shared TerminalShellRuntimeConfig handle, the read-at-create-time hot swap, and folding the desktop command + web handler into set_system_terminal_settings_core are all exactly the right shapes for this codebase. I verified the wiring end to end and it holds up:

• ConnectionManager::clone_ref() copies the Arc, and the desktop-embedded web server builds its AppState with (*app.state::<ConnectionManager>()).clone_ref() (web/mod.rs), so the Tauri command, the web handler, and live ACP connections all mutate one handle.
• State<'_, ConnectionManager> on update_system_terminal_settings is genuinely managed (lib.rs:209) — no "state not managed" panic.
• A malformed stored preference only warns and leaves the system fallback, so it can't wedge agents.
• powershell_fallback_uses_the_selected_executable is safe on the windows-latest cell: resolve_windows_program bails out when the program already carries an extension, so pwsh.exe survives normalization.

I also ran the full matrix on this branch merged into current main (which already carries the newer HostToolsPolicy work touching the same run_connection region — merge is clean, semantically too):

cargo clippy --all-targets --features test-utils -- -D warnings          -> 0
cargo clippy --no-default-features --bin codeg-server --lib -- -D warnings -> 0
cargo test --features test-utils                                          -> all green
cargo test --no-default-features --bin codeg-server --lib                 -> 2467 passed

There's one thing I'd like fixed before this lands, plus some smaller notes.

---

Blocking: the unconfigured case silently changes shell too

let shell = configured_shell.unwrap_or_else(resolve_shell);

A user who has never touched the setting stores None, so they now get resolve_shell() — which prefers $SHELL (terminal/manager.rs:28) — instead of the previous hard-coded /bin/sh. On macOS, GUI-launched apps do inherit the login shell in SHELL, so this reaches people who changed nothing.

That matters because this fallback exists specifically to evaluate the POSIX line an agent emitted — dc5057d calls it "the platform shell", and the surrounding comment is explicit that it's there so &&, pipes, $VAR and globs evaluate. Routing that through the user's interactive shell is a different contract. Measured locally:

/bin/sh  -c 'export FOO=bar; echo "FOO=$FOO"'  ->  FOO=bar
/bin/csh -c 'export FOO=bar; echo "FOO=$FOO"'  ->  export: Command not found. / FOO: Undefined variable.
/bin/sh  -c 'echo a 2>&1 | cat'                ->  a
/bin/csh -c 'echo a 2>&1 | cat'                ->  Ambiguous output redirect.

fish is the more likely real-world case (it's literally the placeholder in terminalShellCustomPlaceholder), and it has no heredocs at all — which is the exact shape the InvalidFilename/ENAMETOOLONG branch was added for, since grok crams multi-KB heredoc scripts into command. The failure surfaces as a confusing error inside the agent's tool output, not as an obvious settings problem, so it'd be rough to diagnose from a bug report.

Suggested minimal fix, which keeps the PR's stated goal intact and shrinks the blast radius to explicit opt-in:

let shell = configured_shell.unwrap_or_else(default_platform_shell); // "/bin/sh" / $COMSPEC

i.e. Some(shell) → honor the user's pick; None → keep the old platform default. Worth a small regression test pinning the unset case, since nothing currently asserts it (I ran the 24 acp::terminal_runtime tests under both SHELL=/bin/zsh and SHELL=/bin/csh — green in both, so the default is unpinned today).

---

Non-blocking notes

Windows PowerShell 5.1 can't do &&. powershell.exe is an offered option in build_available_terminal_shells, and PS 5.1 doesn't support &&/|| — the very operators this fallback exists to evaluate. Only pwsh (7+) does. Probably worth a note in the picker, or at least awareness that selecting it degrades agent shell lines.

Consider the env-var indirection for the PowerShell branch. The built-in terminal deliberately goes through $env:CODEG_CMD + Invoke-Expression ("Indirection via env var avoids quoting/escaping bugs for arbitrary commands"). I couldn't demonstrate an actual mangling on the new -Command <line> argv path, so I'm not calling this a defect — just that the interactive path already decided this was worth hardening, and agent-generated lines are at least as arbitrary. Same place you'd want [Console]::OutputEncoding = UTF8: the ACP path captures stdout through a pipe and decodes it as UTF-8, while the built-in PowerShell path sets the console encoding explicitly. Untested on Windows by me.

Shell classification is now duplicated. shell_wrapped_command's pwsh/powershell/cmd detection is a near-copy of detect_windows_shell_flavor (same lowercased file_name() trick). The different fallthrough is intentional, but the classification half could be one shared helper so the two don't drift.

can_retry_through_shell keys off configured_shell.is_some(). That flag doesn't actually say "this is a builtin" — and the motivating case still isn't covered once the agent passes args (Get-ChildItem -Path . → non-empty args → no fallback). Understood why: reconstructing argv as shell text needs shell-specific quoting. Side effect worth knowing: on POSIX hosts with any configured shell, a missing bare command now becomes exit 127 from the shell instead of an RPC spawn error. No extra authority granted (the agent already has arbitrary shell execution via the whitespace path), so this is informational.

Settings copy is now understated. GeneralSettings.terminalDescription still says "the shell used when opening new terminal tabs from the terminal bar or file tree" in all 10 locales, but the setting now also governs how agents run shell commands. Users deserve to know that before they point it at fish.

Startup ordering (pre-existing pattern). ccm_ref.start_background(...) is spawned around lib.rs:467, while apply_persisted_terminal_shell_config runs in the block_on at ~lib.rs:601. So on desktop there's a very small window where a chat-channel-triggered agent could create a terminal before the preference is seeded. The live-read handle makes this nearly unreachable, and every other apply_persisted_* call has the same shape, so it's not on you — just noting it. Server mode seeds before binding, which is correct.

Nit: the cfg-gated use crate::acp::manager::ConnectionManager; should sort before TerminalShellRuntimeConfig in commands/system_settings.rs. No cargo fmt gate in CI, so purely cosmetic.

---

Happy to merge as soon as the unset-default case is preserved — everything else on the list is optional. Nice change overall. 🙏

[doublecurry]

Fixed the blocking case in c2acace: when General Settings has no explicit shell, ACP now preserves its legacy platform fallback (/bin/sh on Unix; COMSPEC or cmd.exe on Windows). Explicitly configured shells still apply to model terminal requests. Added regression coverage for the unset Unix default and Windows fallback normalization. Verified locally with the shell-config tests, server cargo check, server clippy, and git diff --check.

[xintaofei]

c2acace9 is exactly the fix — thanks for turning that around so fast. None now lands on default_platform_shell(), so an unconfigured install keeps /bin/sh / $COMSPEC and only an explicit selection changes anything. Blocking issue resolved. 🎉

I re-ran the full matrix on this branch merged into current main:

cargo clippy --all-targets --features test-utils -- -D warnings           -> 0
cargo clippy --no-default-features --bin codeg-server --lib -- -D warnings -> 0
cargo test --features test-utils                    -> 2532 lib + all integration bins, 0 failures
cargo test --no-default-features --bin codeg-server --lib -> 2508 passed, 0 failed

I also wrote a throwaway call-site test to confirm the fix works end to end and not just at the helper — unconfigured runtime, SHELL=/bin/csh, command export FOO=bar; echo "FOO=$FOO"; echo redir 2>&1 | cat. It passes on c2acace9, and when I reverted just the one-line call site back to resolve_shell it failed with csh's Ambiguous output redirect. — i.e. it reproduces the exact regression and your fix closes it.

I'll merge this shortly. Everything below is optional — recording it so it isn't lost, not asking for another round.

• The unset Windows path preserves the binary but not the flags (/C → /D /S /C). /D is hardening and matches the built-in terminal. /S does change quote handling after /C in an input-dependent way, so it isn't a strict no-op for lines with embedded quotes — worth knowing, though I don't think it's worth chasing without a Windows repro.
• The two shell classifiers have inverted fallthrough: detect_windows_shell_flavor sends an unrecognized Windows shell to Cmd, shell_wrapped_command sends it to POSIX -c. That's now mildly load-bearing, since $COMSPEC is accepted generically — a cmd-compatible replacement like TCC would get -c where it used to get /C. Rare enough to ignore; mostly an argument for sharing one classifier eventually.
• unset_shell_keeps_the_legacy_posix_default pins the helper rather than the selection at the call site, so a future edit to create_terminal could reintroduce this without failing anything. The scratch test above shows a discriminating version is cheap if you ever want one.
• Still open from the first pass and still fine to defer: the settings copy across the 10 locales, the classifier duplication, PowerShell 5.1's missing &&, the can_retry_through_shell keying, the desktop startup-order window, and the Windows UTF-8 console-encoding gap.

Nice work on this one — the shared hot-swappable handle is a clean piece of design. 🙏

[xintaofei]

Pushed cbb3d458 — this clears the follow-up list from the last two rounds so nothing is left dangling.

Shell classification is now shared. detect_windows_shell_flavor and the ACP wrapper's inline copy had inverted fallthrough — an unrecognized Windows shell got cmd's /C from one and POSIX -c from the other — so a COMSPEC pointing at a cmd-compatible replacement was invoked the wrong way. Both now go through a new terminal::shell_flavor. detect_posix_shell_flavor keeps its own list, since "does this shell accept -l -i" is a different question from "what's its -c convention".

The retry gate no longer keys off "is something configured". Explicitly selecting /bin/sh used to behave differently from leaving the setting on its default — same shell, different retry policy. It now keys off whether the fallback shell resolves names the OS can't: PowerShell's Get-ChildItem, cmd's dir.

The unset default is pinned at the call site, not just the helper. unset_shell_keeps_the_legacy_posix_default couldn't catch a future edit to create_terminal. The new test runs echo "ran-under=$0" through an unconfigured runtime — $0 names the shell that actually interpreted the line, so reverting the call site to resolve_shell fails it with ran-under=/bin/zsh. I verified that discrimination by actually reverting it.

Desktop startup seeds the shell before the chat-channel dispatcher spawns, matching server startup, which seeds before it binds.

Settings copy updated in all 10 locales — it now says agents use this shell too, with a hint that they emit POSIX syntax which fish, nushell, and Windows PowerShell 5.1 may reject, and that System default keeps agent lines on the platform shell. Plus the import-order nit.

One item I deliberately did not fix

The Windows OEM-encoding gap. I had implemented it — chcp 65001 for cmd, [Console]::OutputEncoding for PowerShell — and then backed it out, because both need a console and crate::process::tokio_command spawns these children with CREATE_NO_WINDOW (process.rs:38). They'd have failed and added a diagnostic to the agent's captured output without changing the encoding, and for cmd the failed ERRORLEVEL could have leaked into the final status. The built-in terminal gets away with it only because it runs its shell under a PTY. So the residual is unchanged, pre-existing, Windows-only, non-ASCII-only, and decode_available_utf8 already degrades lossily rather than failing. The reasoning is recorded in a doc comment on shell_wrapped_command so nobody re-adds the preamble. A real fix needs a redirected-stream-aware approach and deserves its own change.

Also worth flagging as a conscious tradeoff: unrecognized Windows shells now get the cmd convention rather than POSIX -c. That's what fixes the COMSPEC case, but it means a nu.exe/xonsh.exe custom path on Windows won't get -c. Given the picker's Windows options are pwsh/Windows PowerShell/cmd, that seemed the right way to lean.

Verification

cargo clippy --all-targets --features test-utils -- -D warnings           -> 0
cargo clippy --no-default-features --bin codeg-server --lib -- -D warnings -> 0
cargo test --features test-utils                     -> 2444 lib + all integration bins, 0 failures
cargo test --no-default-features --bin codeg-server --lib -> 2420 passed, 0 failed
pnpm eslint .  -> 0      npx tsc --noEmit -> 0
pnpm test      -> 286 files / 3766 tests passed
pnpm build     -> 0

I don't have a Windows host, so the Windows arms are argv-asserted by unit tests but never executed on Windows — same as before this commit, since the wrapper now passes the line verbatim exactly as it did.

Ready to merge from my side. Thanks again for the quick turnarounds on this one. 🙏

[xintaofei]

Follow-up: main moved under us (#432 landed and refactored general-settings.tsx into the new SettingsSection/SettingCard shell, plus touched all 10 locale files), so the branch went CONFLICTING. Resolved in f5921d00 — took main's structure and re-applied the agent-shell hint as a third block inside the section's description, which fits the new layout better than the standalone paragraph I'd added. The locale files were regenerated on top of main's versions rather than hand-merged.

Re-ran everything on the merged result, since #432 brought a fair amount of Rust with it:

cargo clippy --all-targets --features test-utils -- -D warnings           -> 0
cargo clippy --no-default-features --bin codeg-server --lib -- -D warnings -> 0
cargo test --features test-utils                          -> 0 failures
cargo test --no-default-features --bin codeg-server --lib -> 0 failures
pnpm eslint . -> 0    npx tsc --noEmit -> 0
pnpm test     -> 292 files / 3829 tests passed
pnpm build    -> 0

Back to MERGEABLE.