fix(non-interactive-env): detect shell type for csh/tcsh env var syntax (fixes #2089)#2769
Conversation
|
I have read the CLA Document and I hereby sign the CLA |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92509d8cfb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Detect the current shell type for proper env var syntax (export for bash/zsh, setenv for csh/tcsh, etc.) | ||
| const shellType = detectShellType() | ||
| const envPrefix = buildEnvPrefix(NON_INTERACTIVE_ENV, shellType) |
There was a problem hiding this comment.
This hook only runs for input.tool === "bash", and the existing behavior in src/hooks/non-interactive-env/index.test.ts:209-303 explicitly documents that the Bash tool executes in a Unix-like shell even when the parent OpenCode process was launched from PowerShell or Windows cmd. Switching to detectShellType() here keys off the host process environment (PSModulePath, $SHELL) instead of the tool that will actually parse the command, so users who start OpenCode from PowerShell, cmd, or tcsh will now prepend $env:..., set ..., or setenv ... to a Bash command line. Bash treats those prefixes as syntax errors, so every git command in those environments regresses from “works with non-interactive vars” to “fails before git starts.”
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Confidence score: 2/5
- High-confidence, user-impacting bug risk: in
src/hooks/non-interactive-env/non-interactive-env-hook.ts,detectShellType()can returncshand producesetenvoutput even though this hook is for thebashtool, so generated commands may fail to execute. src/shared/shell-env.tsuses substring matching on fullSHELLpaths, which can misclassify shells (e.g., paths containingcsh) and propagate incorrect env syntax decisions.- Given the two medium-high severity findings (7/10 and 8/10) with strong confidence, this is not yet safe to merge without fixes.
- Pay close attention to
src/hooks/non-interactive-env/non-interactive-env-hook.ts,src/shared/shell-env.ts- shell misdetection can generate incompatible environment export commands.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/shared/shell-env.ts">
<violation number="1" location="src/shared/shell-env.ts:18">
P1: Shell detection uses unsafe substring matching on the full SHELL path that generates false positives for paths containing 'csh'.</violation>
</file>
<file name="src/hooks/non-interactive-env/non-interactive-env-hook.ts">
<violation number="1" location="src/hooks/non-interactive-env/non-interactive-env-hook.ts:56">
P1: The `detectShellType()` call can return "csh" for csh/tcsh users, generating `setenv` syntax that bash cannot execute. Since this hook only processes the `bash` tool, it must always use POSIX `export` syntax compatible with bash/sh, not the host's login shell syntax.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if (process.env.SHELL) { | ||
| const shell = process.env.SHELL | ||
| if (shell.includes("csh") || shell.includes("tcsh")) { |
There was a problem hiding this comment.
P1: Shell detection uses unsafe substring matching on the full SHELL path that generates false positives for paths containing 'csh'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/shared/shell-env.ts, line 18:
<comment>Shell detection uses unsafe substring matching on the full SHELL path that generates false positives for paths containing 'csh'.</comment>
<file context>
@@ -14,6 +14,10 @@ export function detectShellType(): ShellType {
if (process.env.SHELL) {
+ const shell = process.env.SHELL
+ if (shell.includes("csh") || shell.includes("tcsh")) {
+ return "csh"
+ }
</file context>
| // (via Git Bash, WSL, etc.), so always use unix export syntax. | ||
| const envPrefix = buildEnvPrefix(NON_INTERACTIVE_ENV, "unix") | ||
| // Detect the current shell type for proper env var syntax (export for bash/zsh, setenv for csh/tcsh, etc.) | ||
| const shellType = detectShellType() |
There was a problem hiding this comment.
P1: The detectShellType() call can return "csh" for csh/tcsh users, generating setenv syntax that bash cannot execute. Since this hook only processes the bash tool, it must always use POSIX export syntax compatible with bash/sh, not the host's login shell syntax.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/non-interactive-env/non-interactive-env-hook.ts, line 56:
<comment>The `detectShellType()` call can return "csh" for csh/tcsh users, generating `setenv` syntax that bash cannot execute. Since this hook only processes the `bash` tool, it must always use POSIX `export` syntax compatible with bash/sh, not the host's login shell syntax.</comment>
<file context>
@@ -52,9 +52,9 @@ export function createNonInteractiveEnvHook(_ctx: PluginInput) {
- // (via Git Bash, WSL, etc.), so always use unix export syntax.
- const envPrefix = buildEnvPrefix(NON_INTERACTIVE_ENV, "unix")
+ // Detect the current shell type for proper env var syntax (export for bash/zsh, setenv for csh/tcsh, etc.)
+ const shellType = detectShellType()
+ const envPrefix = buildEnvPrefix(NON_INTERACTIVE_ENV, shellType)
</file context>
|
Re: issues identified by cubic. P1 (detectShellType returns csh for bash tool): Acknowledged concern. The bash tool in OpenCode spawns a shell using the system's If the maintainer confirms that the bash tool always spawns P2 (substring matching on SHELL paths): The check is |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
LGTM. Clean fix for csh/tcsh shell detection with proper setenv syntax. All CI passing. Fixes #2089.
2 participants
Summary
setenvsyntax to the non-interactive-env hookProblem
The non-interactive-env hook hardcodes
"unix"as the shell type when building environment variable prefixes for git commands. This generatesexport VAR=valuesyntax which is incompatible with csh/tcsh shells, wheresetenv VAR valueis required. Users running csh/tcsh see syntax errors when the hook prepends env vars to git commands.Fix
"csh"to theShellTypeunion inshell-env.tsdetectShellType()to check$SHELLfor csh/tcsh patternssetenv VAR value;syntax generation inbuildEnvPrefix()for csh"unix"in the non-interactive-env hook withdetectShellType()callChanges
src/shared/shell-env.tssrc/hooks/non-interactive-env/non-interactive-env-hook.tsFixes #2089
Summary by cubic
Detects csh/tcsh shells and uses
setenvsyntax in the non-interactive env hook, preventing syntax errors in git commands. Restores non-interactive behavior for csh/tcsh users.$SHELLand adds"csh"toShellType.setenv VAR value;inbuildEnvPrefix()for csh; reuses unix escaping."unix"withdetectShellType()in the non-interactive-env hook.Written for commit 92509d8. Summary will update on new commits.