fix(tools): flag piped POSIX utilities before running on Windows#412
Open
PierrunoYT wants to merge 2 commits into
Open
fix(tools): flag piped POSIX utilities before running on Windows#412PierrunoYT wants to merge 2 commits into
PierrunoYT wants to merge 2 commits into
Conversation
…nning on Windows Zero always executes the bash tool through native cmd.exe on Windows (shellExecutable() in bash.go), but the pre-flight Windows-syntax detector only caught bash-style `cd /...` and bare `ls`. Anything piped into head/tail/grep/wc/awk/sed/cut/xargs/tr — very common in model-generated commands like `git log ... | head` — ran straight through to cmd.exe, which has no such builtins, wasting a turn on a guaranteed failure. Reported in the wild via `git log ... | head` and a `git blame ... | grep` pipeline. detectShellCommandIssue now also flags these before the command runs, with the same "translate to cmd.exe / use Zero's native tools" guidance already used for the cd/ls cases. Note: the existing detectShellOutputIssue path (matching cmd.exe's error text post-execution) only recognizes English strings and will miss this on non-English Windows locales — the pre-flight check above is locale-independent since it inspects the model-generated command text rather than cmd.exe's translated error output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 2, 2026
Open
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a Windows-only pre-flight regex check for piped POSIX utilities in shell commands and returns a ChangesWindows POSIX Utility Detection
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant BashTool
participant detectShellCommandIssue
participant windowsPosixUtilityPattern
BashTool->>detectShellCommandIssue: command, target="windows"
detectShellCommandIssue->>windowsPosixUtilityPattern: match(command)
windowsPosixUtilityPattern-->>detectShellCommandIssue: match found
detectShellCommandIssue-->>BashTool: shellIssue(windows_shell_syntax, suggestion)
Related issues: Suggested reviewers: none identified from provided context. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/tools/bash_tool_test.go (1)
177-188: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression case for utility-prefixed script names.
Once the
windowsPosixUtilityPatternfalse-positive on names liketail-log.sh/grep-cliis fixed (see companion comment inshell_runtime.go), add a case here to lock in the fix, e.g.tail-log.sh,grep-cli --version.🤖 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 `@internal/tools/bash_tool_test.go` around lines 177 - 188, Add a regression case in TestDetectShellCommandIssueAllowsUnrelatedCommands to cover utility-prefixed script names that should not trigger windowsPosixUtilityPattern false positives. Extend the existing command list with examples like tail-log.sh and grep-cli --version, and verify detectShellCommandIssue continues to return nil for them on windows, matching the fix in detectShellCommandIssue and the related shell_runtime.go pattern.
🤖 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.
Inline comments:
In `@internal/tools/shell_runtime.go`:
- Around line 23-27: The Windows POSIX utility matcher is too broad because the
current boundary after the command name still matches script or executable names
like tail-log.sh, grep-cli, and sed.exe. Update windowsPosixUtilityPattern in
shell_runtime.go to require whitespace or end-of-string after the utility token,
matching the same guard used by the ls pattern, so the regex only flags
standalone commands in the shell command parsing path.
---
Nitpick comments:
In `@internal/tools/bash_tool_test.go`:
- Around line 177-188: Add a regression case in
TestDetectShellCommandIssueAllowsUnrelatedCommands to cover utility-prefixed
script names that should not trigger windowsPosixUtilityPattern false positives.
Extend the existing command list with examples like tail-log.sh and grep-cli
--version, and verify detectShellCommandIssue continues to return nil for them
on windows, matching the fix in detectShellCommandIssue and the related
shell_runtime.go pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd6c5f24-df45-47d3-b8eb-d0a6791c2a6b
📒 Files selected for processing (2)
internal/tools/bash_tool_test.gointernal/tools/shell_runtime.go
CodeRabbit review on PR Gitlawb#412: windowsPosixUtilityPattern used \b after the utility alternation, but \b only requires a non-word character next — so tail-log.sh, grep-cli, and sed.exe would all false-positive (the hyphen/dot satisfies \b without being a real command boundary). Match windowsLSCommandPattern's existing convention: require whitespace or end-of-string after the utility name instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Zero always runs the bash tool through native
cmd.exeon Windows. The existing pre-flight Windows-syntax detector (detectShellCommandIssue) only caught bash-stylecd /...and barels, not the broader family of common POSIX pipe utilities (head,tail,grep,wc,awk,sed,cut,xargs,tr) that cmd.exe has no equivalent for. Piping into any of these (e.g.git log ... | head, agit blame ... | greppipeline) always failed, wasting a turn — reported in the wild.This adds a
windowsPosixUtilityPatterncheck alongside the existingcd/lsdetectors ininternal/tools/shell_runtime.go, blocking these pre-flight with the same "translate to cmd.exe / use Zero's native tools" guidance already used forcd/ls.Scoped narrowly to the pre-flight command-text check (locale-independent, since it inspects the model-generated command rather than cmd.exe's own translated error output) rather than trying to also expand the post-hoc output-string matcher, which can never be locale-complete.
Linked issue
Fixes #411
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Verification notes
go build ./.../go vet ./...clean.TestDetectShellCommandIssueFlagsPipedPosixUtilitiesandTestDetectShellCommandIssueAllowsUnrelatedCommands(internal/tools/bash_tool_test.go); both pass alongside the existingcd/lsdetector tests.go test ./internal/tools/...passes in full.gofmt -lon the touched files: pre-existing repo-wide CRLF flag (core.autocrlf=trueon Windows), not introduced by this change — same as prior PRs against this repo.git grep foo(real git subcommand, not a pipe target) andfindstr /r "..."are unaffected since the pattern only matches at the start of a command or immediately after a|/&/;separator, not as a positional argument.Summary by CodeRabbit
head,grep,wc,tail) where they aren’t supported.git,dir,findstr, andecho) to pass through unflagged.