fix(non-interactive-env): add Windows/PowerShell support #573
Conversation
Greptile SummaryAdds Windows/PowerShell support to the Key Changes:
Architecture:
Known Issue (Not Addressed): Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Hook as non-interactive-env hook
participant ShellEnv as shell-env utility
participant Bash as Bash tool
User->>Hook: Execute git command
Hook->>Hook: Check tool equals bash
Hook->>Hook: Check command contains git
Hook->>ShellEnv: detectShellType()
ShellEnv->>ShellEnv: Check PSModulePath for PowerShell
ShellEnv->>ShellEnv: Check SHELL env var for Unix
ShellEnv->>ShellEnv: Fallback to platform
ShellEnv-->>Hook: Return shell type
Hook->>ShellEnv: buildEnvPrefix with env and shellType
ShellEnv->>ShellEnv: Apply shellEscape to each value
alt Unix shell
ShellEnv-->>Hook: Return export statement
else PowerShell
ShellEnv-->>Hook: Return PowerShell env syntax
else cmd.exe
ShellEnv-->>Hook: Return cmd set syntax
end
Hook->>Hook: Prepend prefix to command
Hook->>Bash: Modified command with env vars
Bash-->>User: Execute with non-interactive environment
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
src/hooks/non-interactive-env/index.ts, line 44-50 (link)logic: Missing
isNonInteractive()check - Issue #566 specifically identified that this function exists indetector.tsbut is never called. The hook currently applies environment variables in ALL contexts (interactive and non-interactive), which contradicts the hook's name and purpose.Add environment detection before applying env vars:
5 files reviewed, 2 comments
| import type { PluginInput } from "@opencode-ai/plugin" | ||
| import { HOOK_NAME, NON_INTERACTIVE_ENV, SHELL_COMMAND_PATTERNS } from "./constants" | ||
| import { log } from "../../shared" | ||
| import { log, detectShellType, buildEnvPrefix } from "../../shared" |
There was a problem hiding this comment.
logic: Need to import isNonInteractive function that's exported from this module but never used.
| import { log, detectShellType, buildEnvPrefix } from "../../shared" | |
| import { log, detectShellType, buildEnvPrefix } from "../../shared" | |
| import { isNonInteractive } from "./detector" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/non-interactive-env/index.ts
Line: 3:3
Comment:
**logic:** Need to import `isNonInteractive` function that's exported from this module but never used.
```suggestion
import { log, detectShellType, buildEnvPrefix } from "../../shared"
import { isNonInteractive } from "./detector"
```
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 3/5
- Missing percent-sign escaping in
src/shared/shell-env.tslets%sequences expand incmd.exe, so values like100%or%PATH%will be mangled when commands run. - Because this is a concrete, user-facing quoting bug in shell env handling, there's a moderate regression risk until the escaping is fixed.
- Pay close attention to
src/shared/shell-env.ts- ensure%characters are properly escaped before invokingcmd.exe.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/shared/shell-env.ts">
<violation number="1" location="src/shared/shell-env.ts:46">
P1: cmd.exe escaping is missing percent sign handling. The `%` character triggers environment variable expansion even inside double quotes. Values containing `%` (like `100%` or `%PATH%`) will be incorrectly expanded. Escape `%` as `%%` before escaping double quotes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@sisyphus-dev-ai reflect reviews and re request to reviewers as comment |
Review ReflectionThank you @greptile-apps and @cubic-dev-ai for the thorough reviews! I've carefully analyzed both sets of feedback and identified two critical issues that need to be addressed before this PR can be merged. Issues Identified1. Missing
|
@sisyphus-dev-ai I have started the AI code review. It will take a few minutes to complete. |
sisyphus-dev-ai
removed
the
sisyphus: working
| const shellType = detectShellType() | ||
| const envPrefix = buildEnvPrefix(NON_INTERACTIVE_ENV, shellType) | ||
| output.args.command = `${envPrefix} ${command}` |
There was a problem hiding this comment.
logic: Hook applies env vars unconditionally to all git commands. The exported isNonInteractive() function from detector.ts (lines 1-19) was created to detect non-interactive environments but is never called here. This means interactive sessions also get these restrictive settings, which contradicts the hook's purpose.
Issue #566 specifically mentions this as a problem. While this PR fixes Windows compatibility, it doesn't address the core issue that the hook runs in all contexts instead of just non-interactive ones.
| const shellType = detectShellType() | |
| const envPrefix = buildEnvPrefix(NON_INTERACTIVE_ENV, shellType) | |
| output.args.command = `${envPrefix} ${command}` | |
| const shellType = detectShellType() | |
| // Only apply in non-interactive environments (CI, GitHub Actions, etc.) | |
| if (!isNonInteractive()) { | |
| return | |
| } | |
| const envPrefix = buildEnvPrefix(NON_INTERACTIVE_ENV, shellType) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/non-interactive-env/index.ts
Line: 48:50
Comment:
**logic:** Hook applies env vars unconditionally to all git commands. The exported `isNonInteractive()` function from `detector.ts` (lines 1-19) was created to detect non-interactive environments but is never called here. This means interactive sessions also get these restrictive settings, which contradicts the hook's purpose.
Issue #566 specifically mentions this as a problem. While this PR fixes Windows compatibility, it doesn't address the core issue that the hook runs in all contexts instead of just non-interactive ones.
```suggestion
const shellType = detectShellType()
// Only apply in non-interactive environments (CI, GitHub Actions, etc.)
if (!isNonInteractive()) {
return
}
const envPrefix = buildEnvPrefix(NON_INTERACTIVE_ENV, shellType)
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 3/5
- Missing percent escaping in
src/shared/shell-env.tsmeans%VAR%values will still expand inside double quotes oncmd.exe, risking unintended command execution or incorrect behavior for Windows users. - Given the concrete regression/security risk on Windows shells, merging now feels moderately risky until the escape logic is fixed.
- Pay close attention to
src/shared/shell-env.ts- ensure percent signs are doubled before invokingcmd.exe.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/shared/shell-env.ts">
<violation number="1" location="src/shared/shell-env.ts:46">
P1: Missing `%` escaping for cmd.exe - `%VAR%` patterns will be expanded even inside double quotes, causing unexpected behavior or potential security issues. The percent sign needs to be escaped by doubling it to `%%`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
you still got some feedbacks left, and let you do git pull --rebase origin dev thing. @sisyphus-dev-ai |
- Create shared shell-env utility with cross-platform shell detection - Detect shell type via PSModulePath, SHELL env vars, platform fallback - Support Unix (export), PowerShell ($env:), and cmd.exe (set) syntax - Add 41 comprehensive unit tests for shell-env utilities - Add 5 cross-platform integration tests for hook behavior - All 696 tests pass, type checking passes, build succeeds Closes #566
… % escaping - Add isNonInteractive() check to only apply env vars in CI/non-interactive contexts (Issue #566) - Fix cmd.exe percent sign escaping to prevent environment variable expansion - Update test expectations for correct % escaping behavior Resolves feedback from @greptile-apps and @cubic-dev-ai
sisyphus-dev-ai
force-pushed
the
fix/non-interactive-env-windows-support
branch
from
e301162 to
f99e7e4
Compare
✅ Review Feedback AddressedThank you @greptile-apps and @cubic-dev-ai for the thorough reviews! I've addressed all outstanding feedback: Changes Made1. Added
|
@sisyphus-dev-ai I have started the AI code review. It will take a few minutes to complete. |
sisyphus-dev-ai
removed
the
sisyphus: working
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds Windows and PowerShell support to the non-interactive-env hook by introducing a new shared utility for cross-platform shell detection and environment variable escaping.
Key Changes:
- Created
src/shared/shell-env.tswith three core functions for shell detection, value escaping, and env prefix building - Refactored hook to use shared utility, removing duplicate logic (3 lines changed in hook)
- Added 41 unit tests for shell-env utility and 5 cross-platform integration tests for the hook
- All 696 tests pass with proper environment setup/teardown
Implementation Quality:
- Follows TDD methodology (RED-GREEN-REFACTOR) as specified in AGENTS.md
- Uses Bun test framework with BDD-style comments (#given, #when, #then)
- Proper barrel export pattern in
src/shared/index.ts - Zero breaking changes - Unix behavior preserved
Issue Found:
- cmd.exe empty string handling:
set VAR=""sets variable to literal""instead of empty string. Should useset VAR=for empty values (see inline comment)
Confidence Score: 4/5
- This PR is safe to merge with one minor cmd.exe escaping issue that should be addressed
- Score reflects excellent test coverage, proper TDD methodology, and clean architecture following project conventions. Reduced from 5 due to cmd.exe empty string escaping bug that could cause unexpected behavior when env vars have empty values on Windows cmd.exe. The issue is minor and doesn't affect PowerShell or Unix shells
- Pay attention to
src/shared/shell-env.tsfor the cmd.exe empty string escaping fix
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/shared/shell-env.ts | 4/5 | New shared utility for cross-platform shell detection and environment variable escaping with proper escaping for Unix, PowerShell, and cmd.exe shells |
| src/shared/shell-env.test.ts | 5/5 | Comprehensive test coverage (41 tests) for shell detection, escaping, and env prefix building across all shell types |
| src/hooks/non-interactive-env/index.ts | 5/5 | Refactored to use shared shell-env utility, removing duplicate logic and adding cross-platform support |
| src/hooks/non-interactive-env/index.test.ts | 5/5 | Added 5 cross-platform integration tests for macOS, Linux, Windows PowerShell, and cmd.exe with proper environment setup/teardown |
Sequence Diagram
sequenceDiagram
participant Hook as non-interactive-env hook
participant ShellEnv as shell-env utility
participant Process as process.env
Hook->>Hook: tool.execute.before(bash)
Hook->>Hook: Check if command contains git
alt is git command and isNonInteractive
Hook->>ShellEnv: detectShellType()
ShellEnv->>Process: Check PSModulePath
alt PSModulePath exists
ShellEnv-->>Hook: powershell
else Check SHELL var
ShellEnv->>Process: Check SHELL
alt SHELL exists
ShellEnv-->>Hook: unix
else Check platform
ShellEnv->>Process: process.platform
alt win32
ShellEnv-->>Hook: cmd
else other
ShellEnv-->>Hook: unix
end
end
end
Hook->>ShellEnv: buildEnvPrefix(env, shellType)
loop For each env variable
ShellEnv->>ShellEnv: shellEscape(value, shellType)
end
ShellEnv-->>Hook: Env prefix string
Note over Hook: Formats depend on shell type
Hook->>Hook: Prepend prefix to git command
end
| if (value === "") { | ||
| return shellType === "cmd" ? '""' : "''" |
There was a problem hiding this comment.
In cmd.exe, set VAR="" sets the variable to a literal two-character string "" (including the quote characters), not an empty string. The correct syntax for empty string is set VAR= with nothing after =.
This might cause issues when NON_INTERACTIVE_ENV contains empty values like VISUAL: "". Consider special-casing empty strings for cmd:
| if (value === "") { | |
| return shellType === "cmd" ? '""' : "''" | |
| if (value === "") { | |
| return shellType === "cmd" ? "" : "''" | |
| } |
Then update buildEnvPrefix to handle the empty case:
// In cmd case, skip empty values or use "set VAR=" format
if (value === "" && shellType === "cmd") {
return `set ${key}=`
}
Summary
Fixes #566 by adding Windows/PowerShell support to the
non-interactive-envhook.Previously, the hook used Unix-only
export VAR=val;syntax which broke on Windows shells (PowerShell, cmd.exe). This PR implements cross-platform shell detection and appropriate syntax generation.Changes
New Shared Utility:
src/shared/shell-env.tsCreated reusable shell environment utility with three core functions:
detectShellType()- Detects shell type using environment heuristicsPSModulePath→ PowerShell,SHELL→ Unix, platform fallback"unix"|"powershell"|"cmd"shellEscape(value, shellType)- Shell-specific value escaping'\''for embedded quotes''for embedded quotes""for embedded quotesbuildEnvPrefix(env, shellType)- Builds shell-specific env var prefixexport VAR1=val1 VAR2=val2;$env:VAR1='val1'; $env:VAR2='val2';set VAR1="val1" && set VAR2="val2" &&Updated Hook:
src/hooks/non-interactive-env/index.tsshellEscape()andbuildEnvPrefix()functionsTesting
Followed TDD methodology (RED-GREEN-REFACTOR):
Tests Added
src/shared/shell-env.test.tssrc/hooks/non-interactive-env/index.test.tsTest Results
Implementation Details
Shell Detection Strategy:
PSModulePathreliably indicates PowerShell (both 5.1 and Core/pwsh)SHELLenv var indicates Unix shellswin32→ cmd, others → unixSHELL=/bin/bash)Architecture:
src/shared/conventions)Edge Cases Handled
$,%,&,|,<,>,^Benefits
Related Issues
Closes #566
Summary by cubic
Adds Windows and PowerShell support to the non-interactive-env hook by detecting the current shell and generating the right env var prefix. Applies env vars only in non-interactive contexts and fixes cmd.exe percent sign escaping; Unix behavior stays the same.
Bug Fixes
Refactors
Written for commit f99e7e4. Summary will update on new commits.