fix(shell): parse arguments in defaultShell so flagged shells don't hang

[JosephDoUrden]

Fixes #448. getShellSpawnArgs treated the whole defaultShell value as one executable path, so a value like pwsh.exe -NoProfile -NoLogo or /bin/bash --norc tried to spawn a binary named after the entire string, hit ENOENT and the call just hung until the client timeout. Now the value is split into executable + args (quote-aware, so an executable path with spaces still survives), matched on the executable name, and the extra args are kept ahead of the usual login/-c flags. Single-token values behave exactly as before.

Added test/test-default-shell-args.js covering the split, the existing test-default-shell.js still passes, and I checked a multi-token shell actually runs a command end to end on macOS.

[coderabbitai]

📝 Walkthrough

Walkthrough

defaultShell values now support embedded arguments and quoted executable paths. Shell spawn construction preserves those arguments across supported shells, and new tests cover parsing, quoting, and Windows cmd.exe behavior.

Changes

Shell argument support

Layer / File(s)	Summary
Shell configuration parsing and spawn construction src/config-field-definitions.ts, src/terminal-manager.ts	The shell configuration description documents argument usage, while splitShellConfig and getShellSpawnArgs separate and preserve shell arguments.
Shell argument parsing tests test/test-default-shell-args.js	Tests cover single-token shells, flags, quoted executable paths, cmd.exe, exported helper usage, and direct test execution.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Config
  participant TerminalManager
  participant ShellProcess
  Config->>TerminalManager: Provide shell path with optional arguments
  TerminalManager->>TerminalManager: Split executable and arguments
  TerminalManager->>ShellProcess: Spawn with preserved arguments and command flags

Loading

Possibly related PRs

• wonderwhy-er/DesktopCommanderMCP#131: Changes how terminal-manager.ts honors configured shells during command execution.
• wonderwhy-er/DesktopCommanderMCP#270: Updates shell spawn argument construction and quoting behavior.
• wonderwhy-er/DesktopCommanderMCP#495: Adjusts cmd.exe verbatim argument handling.

Suggested labels: size:XL

Suggested reviewers: serg33v, wonderwhy-er, edgarsskore

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes implement quote-aware splitting of defaultShell into executable and args, matching the requested support in #448.
Out of Scope Changes check	✅ Passed	The added test and help text updates are directly related to the defaultShell parsing fix and do not appear out of scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: parsing defaultShell arguments to avoid hangs with flagged shells.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

test/test-default-shell-args.js (1)

17-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Good test coverage for the primary use cases.

The tests correctly validate single-token non-regression, pwsh with flags, bash with flags, quoted executable paths with spaces, and cmd.exe verbatim handling. Consider adding a test for the unknown-shell fallback to verify that shellArgs are dropped and useShellOption is set to the extracted executable — this is the one code path in getShellSpawnArgs not covered by the current suite.

🧪 Suggested test for the unknown-shell fallback

   console.log('✓ cmd.exe /q splits correctly and stays verbatim');

+  // 6. Unknown shell with args: executable extracted, args dropped, useShellOption set
+  cfg = getShellSpawnArgs('myshell --norc', 'echo hi');
+  assert.strictEqual(cfg.executable, 'echo hi');
+  assert.deepStrictEqual(cfg.args, []);
+  assert.strictEqual(cfg.useShellOption, 'myshell');
+  console.log('✓ unknown shell fallback extracts executable and drops args');
+
   console.log('\n✅ All defaultShell argument-parsing tests passed');

🤖 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 `@test/test-default-shell-args.js` around lines 17 - 55, Add coverage in
runDefaultShellArgsTests for an unrecognized shell passed with flags, using
getShellSpawnArgs. Assert that shellArgs are omitted and useShellOption is set
to the extracted executable, preserving the expected fallback behavior.

🤖 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.

Nitpick comments:
In `@test/test-default-shell-args.js`:
- Around line 17-55: Add coverage in runDefaultShellArgsTests for an
unrecognized shell passed with flags, using getShellSpawnArgs. Assert that
shellArgs are omitted and useShellOption is set to the extracted executable,
preserving the expected fallback behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9909f5e4-c66c-4917-859b-21f3f29f84e7

📥 Commits

Reviewing files that changed from the base of the PR and between 78100e6 and 4dae66f.

📒 Files selected for processing (3)

• src/config-field-definitions.ts
• src/terminal-manager.ts
• test/test-default-shell-args.js

[lAiasu]

Please someone take a look at this, it solves the issue quite nicely. You can get around a little right now with a shim, but that still is slower than native noProfile.