feat(exec): Support custom shell path for executing commands#39413
feat(exec): Support custom shell path for executing commands#39413ZtRXR wants to merge 1 commit intoopenclaw:mainfrom
Conversation
允许在执行命令时指定自定义shell路径，增强在不同环境下的兼容性。修改包括类型定义、配置验证和执行逻辑的调整。
Greptile SummaryThis PR adds a Key observations:
Confidence Score: 4/5
|
|
|
||
| export function getShellConfig(): { shell: string; args: string[] } { | ||
| export function getShellConfig(configuredShell?: string): { shell: string; args: string[] } { | ||
| if (configuredShell) { |
There was a problem hiding this comment.
Whitespace-only shell path passes truthy check
if (configuredShell) is truthy for strings like " ", which would result in an attempt to spawn " " as the shell executable at runtime. While any invalid shell path is documented to produce a runtime error, trimming first would give a cleaner developer experience (falling back to the default logic instead of a confusing spawn failure).
| if (configuredShell) { | |
| if (configuredShell?.trim()) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/shell-utils.ts
Line: 43
Comment:
**Whitespace-only shell path passes truthy check**
`if (configuredShell)` is truthy for strings like `" "`, which would result in an attempt to spawn `" "` as the shell executable at runtime. While any invalid shell path is documented to produce a runtime error, trimming first would give a cleaner developer experience (falling back to the default logic instead of a confusing spawn failure).
```suggestion
if (configuredShell?.trim()) {
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c672c0ff6c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (opts.usePty) { | ||
| return { |
There was a problem hiding this comment.
Propagate configured shell to PTY executions
When usePty is true, this branch returns only ptyCommand, and the supervisor later chooses its own shell via getShellConfig() without the configured override, so tools.exec.shell is ignored unless PTY startup fails and falls back to child argv. In PTY-heavy workflows (interactive tools, terminal UIs), users who set a custom shell will silently run under the default shell, which can break shell-specific commands or environment setup.
Useful? React with 👍 / 👎.
| scopeKey: defaults?.scopeKey, | ||
| sessionKey: notifySessionKey, | ||
| timeoutSec: effectiveTimeout, | ||
| shell: defaults?.shell, |
There was a problem hiding this comment.
Pass configured shell into gateway approval runs
The configured shell is threaded only through this direct runExecProcess call, but approved gateway executions still invoke runExecProcess from processGatewayAllowlist without a shell argument (src/agents/bash-tools.exec-host-gateway.ts:247-263). That makes behavior inconsistent: the same command honors tools.exec.shell in the normal path but reverts to default shell once it goes through approval, which is a user-visible regression for gateway/ask flows.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as duplicate/superseded: the remaining configurable exec shell work is already tracked by the canonical open issue, while this PR is an incomplete, conflicting implementation of the same security-sensitive exec contract. So I’m closing this here and keeping the remaining discussion on the canonical linked item. Review detailsBest possible solution: Keep the canonical issue open for a maintainer-approved shell-selection contract, and close this stale branch rather than splitting design review across duplicate implementations. Do we have a high-confidence way to reproduce the issue? Yes. Static tracing shows the PR would pass Is this the best way to solve the issue? No. A simple string passthrough is not the narrow maintainable solution for this command-execution boundary; the contract needs consistent PTY, approval, node/sandbox, policy, docs, and test coverage and is already tracked at #49931. Security review: Security review needs attention: The patch changes command interpreter selection without preserving that execution context through approval-backed and PTY runs.
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 624af621e663. |
Summary
Describe the problem and fix in 2–5 bullets:
tools.exec.shellconfiguration option that allows users to specify custom shell paths in configuration files or at the Agent level. Configured paths take priority, with fallback to default logic if not configured.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
List user-visible changes (including defaults/config).
tools.exec.shell(string type, optional):tools.exec.shellagents[].tools.exec.shell$SHELLorshSecurity Impact (required)
NoNoNoNo- Only changes shell path, does not modify command execution permission checks or security policiesNoYes, explain risk + mitigation: N/ARepro + Verification
Environment
tools.exec.shellSteps
tools.exec.shellconfiguration option to config fileopenclaw agentExpected
Actual
Evidence
Attach at least one:
Build verification:
Human Verification (required)
What you personally verified (not just CI), and how:
Compatibility / Migration
Yes- Default behavior completely unchangedYes- New optional configuration optionNoFailure Recovery (if this breaks)
tools.exec.shellfrom configuration to restore default behaviorRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.Risk: Users may configure incorrect shell paths or arguments, causing command execution failures
Risk: Windows shell arguments differ from Unix (
-NoProfile -NonInteractive -Commandvs-c)Risk: Cross-platform configuration sync issues (same config behaves differently on different OS)