Repository navigation
Conversation
📝 WalkthroughWalkthroughWindows argument quoting now protects dollar signs while retaining the existing form for values without them. Tests cover shell quoting and verify the formatted computer action output for a worktree selector containing a dollar sign. Fixed issue severity: <fixed_issue_severity>Medium</fixed_issue_severity> Priority: ➖ Normal Merge Risk: 🔵 Low · up to Generated commands have not been verified in both Windows shells, and an uncommon app name can produce the wrong selector in cmd.exe. The remaining risk is bounded but warrants owner awareness. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The cmd quoting branch this PR adds is unreachable in normal operation, so the Windows shell detection does not actually do what the description claims. ORCA_TERMINAL_WINDOWS_SHELL / ORCA_WINDOWS_SHELL are read but assigned nowhere in the repository, so resolveCliShell always resolves to powershell on win32. A user whose terminalWindowsShell is cmd.exe therefore now gets single-quoted arguments that cmd does not treat as delimiters — a regression from the previous double-quoted output.
Reviewed changes
- PowerShell quoting for CLI follow-up commands.
quoteCliCommandArgumentnow resolves a shell family (powershell/cmd/posix) and quotes accordingly, so$in selectors is no longer expanded when the suggested command is pasted into PowerShell. - Shell detection helper.
resolveCliShellreadsORCA_TERMINAL_WINDOWS_SHELL ?? ORCA_WINDOWS_SHELLthroughresolveWindowsShellStartupFamily, defaulting topowershellwhen unset. - Follow-up command plumbing.
ComputerActionFollowUpTargetgainsshell?: AgentStartupShell, threaded throughformatComputerFollowUpCommand. - Tests.
shell-command-quote.test.tscovers the new win32 default, explicit shell selection, env-based detection, and embedded-quote doubling;computer-format.test.tsasserts the rendered--worktreeargument end to end.
ℹ️ Nitpicks
ComputerActionFollowUpTarget.shellis set by no production caller —getComputerCommandTargetreturns only{ app, worktree | session }and the handlers spread{ ...target, ...observeFlags }. It is the natural place to thread the real terminal shell, but as written only the tests exercise it and production depends entirely on the env fallback above.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| if (platform !== 'win32') { | ||
| return 'posix' | ||
| } | ||
| const configured = env.ORCA_TERMINAL_WINDOWS_SHELL ?? env.ORCA_WINDOWS_SHELL |
There was a problem hiding this comment.
ORCA_TERMINAL_WINDOWS_SHELL and ORCA_WINDOWS_SHELL are read here but assigned nowhere in the repository, so on Windows this always resolves to powershell and the cmd branch below is dead in production. A user whose terminal is cmd.exe then receives single-quoted arguments — cmd does not treat ' as a delimiter, so the selector arrives with literal quotes — a regression from the previous double-quoted output.
Technical details
# Windows shell signal is never populated
## Affected sites
- `src/cli/shell-command-quote.ts:16` — reads `ORCA_TERMINAL_WINDOWS_SHELL ?? ORCA_WINDOWS_SHELL`; neither is ever set.
- `src/cli/orchestration-mutation-recovery.ts:191` — the analogous `resolveRecoveryShell` reads the same vars but also falls back to `ComSpec ?? COMSPEC`, so it reaches `cmd` quoting; this helper deliberately omits that fallback.
- `src/main/daemon/shell-ready.ts:99` — third reader of the same var, also with no writer.
- `src/cli/computer-format.ts:201` — `ComputerActionFollowUpTarget.shell` was added but no caller populates it.
## Required outcome
- The `cmd`/`posix` branches must be reachable when the user's terminal shell is not PowerShell, or the limitation must be stated explicitly instead of implying cmd is handled.
## Suggested approach (optional)
- Populate a shell-family env var in the PTY spawn env (`buildPtyHostEnv`) from the resolved `terminalWindowsShell`/per-tab override, so the CLI child inherits the real shell; or
- Thread the resolved shell into `ComputerActionFollowUpTarget` from `getComputerCommandTarget`/the handlers.
## Open questions for the human (optional)
- Is a `ComSpec` fallback wanted here? Note `ComSpec` is set to `cmd.exe` on every Windows process, including PowerShell sessions, so it would misclassify the PowerShell default.
…follow-up commands
The Windows shell env vars were never set, so cmd.exe users got PowerShell single quotes. Emit one spelling that keeps $ literal in PowerShell without splitting cmd.exe, and drop the unused shell field.
ae2c461 to
a2fbec5
Compare
|
Pullfrog is right. Windows follow-up arguments are now one spelling both shells keep as a single argument. Worktree checks: vitest on Head: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/shell-command-quote.ts (1)
14-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Windows shell round-trip test for the existing quoting vectors.
The tests assert only the strings returned by
quoteCliCommandArgument. They do not check the arguments received by PowerShell or cmd.exe. Run the eight existing vectors through both shells and compare the argument-reporter output with the original inputs. This small matrix is proportionate to the helper’s dual-shell behavior.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
8883889a-9dc9-4b83-a556-e03e9cbd0b72
📒 Files selected for processing (3)
src/cli/computer-format.test.tssrc/cli/shell-command-quote.test.tssrc/cli/shell-command-quote.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
This branch has not been deployed
1 participant
ELI5
When Orca prints an inspect command on Windows, a folder name like
$reviewstays the folder name. The same text is safe to paste into PowerShell or cmd.exe.What Changed
$reviewin a worktree selector such aspath:C:/work/$review.path:C:/work/$"r"eview. PowerShell treats that as one argument and does not expand the dollar sign. cmd.exe joins the quotedrback onto the surrounding text, so it sees the same path. Arguments with no dollar sign stay in the old double quotes. macOS and Linux still use single quotes.quoteCliCommandArgumentno longer readsORCA_TERMINAL_WINDOWS_SHELLorORCA_WINDOWS_SHELL, andComputerActionFollowUpTargetno longer has ashellfield. Nothing in the CLI process set those variables, so the cmd.exe branch never ran and every Windows user got PowerShell single quotes. The pasted command is text for whatever shell the agent is using, so the generator emits one spelling that both PowerShell and cmd.exe accept.Why
Detecting the shell from
ComSpecwould still say cmd.exe inside PowerShell. Single quotes protect PowerShell and split cmd.exe. A quote that starts the argument ends there in PowerShell, so"path"$"review"becomes two arguments. Breaking only the first character of the variable name keeps one argument in both shells.Linked Issue
Fixes #21772
Visual Proof
N/A — CLI command text formatting change.
Testing
Local checks on this revision, from the PR worktree:
vitest run --config config/vitest.config.ts src/cli/shell-command-quote.test.ts src/cli/computer-format.test.ts src/cli/format.test.ts— 3 files, 43 passed, 0 failed. The Windows cases mockprocess.platform.node node_modules/typescript/bin/tsc --noEmit -p config/tsconfig.tc.cli.json(pnpm tc:cli) — passed.oxlinton the changed CLI files — 0 warnings, 0 errors.PowerShell and cmd.exe are not installed on this machine. The spelling was checked against the PowerShell tokenizer (
$"x"stays in one generic argument) and CommandLineToArgvW (quoted text is concatenated). It was not executed in those shells.AI Disclosure
Antigravity (Google DeepMind) drafted the first patch. This quoting revision was written with Grok.
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Ensure no issues in: Security, Cross-platform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
An argument that cannot start a PowerShell generic token (a leading space, for example) is one backtick-escaped double-quoted string. cmd.exe then keeps the backtick. A
$$(subexpression is stopped the same way. The reportedpath:C:/work/$reviewselector does not use that fallback.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Full
pnpm lint,pnpm test, andpnpm buildwere not run locally.pnpm tc:cliand oxlint on the changed files were run. CI covers the rest.