From 51882f6adc02638d127ec949511db0d9d8224ee6 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Fri, 3 Jul 2026 21:56:30 -0400 Subject: [PATCH 1/7] fix(tools): block MSYS coreutils under Windows sandbox Git for Windows usr\bin coreutils (head, grep, cat, ...) are MSYS PE binaries that fail with ACCESS_DENIED and cygheap/signal-pipe errors when Zero runs them in the write-restricted Windows sandbox. Detect and block these commands before execution, surface MSYS runtime failures from stderr, steer agents toward native Zero tools or require_escalated, and exclude MSYS-prone names from known-safe command tails. Refs #458 --- internal/agent/command_prefix.go | 7 ++- internal/agent/command_prefix_test.go | 26 ++++++++- internal/agent/loop_test.go | 14 +++-- internal/agent/system_prompt.go | 2 +- internal/tools/bash_tool_test.go | 8 ++- internal/tools/exec_command.go | 6 +++ internal/tools/shell_runtime.go | 78 +++++++++++++++++++++------ internal/tools/shell_runtime_test.go | 72 +++++++++++++++++++++++++ 8 files changed, 188 insertions(+), 25 deletions(-) create mode 100644 internal/tools/shell_runtime_test.go diff --git a/internal/agent/command_prefix.go b/internal/agent/command_prefix.go index 43e666f7..6a55610c 100644 --- a/internal/agent/command_prefix.go +++ b/internal/agent/command_prefix.go @@ -2,6 +2,7 @@ package agent import ( "fmt" + "runtime" "strings" "github.com/Gitlawb/zero/internal/sandbox" @@ -275,7 +276,11 @@ func knownSafeCommandSegment(command []string) bool { if len(command) == 0 { return false } - switch commandName(command[0]) { + name := commandName(command[0]) + if runtime.GOOS == "windows" && tools.MsysProneCommandName(name) { + return false + } + switch name { case "cat", "cd", "cut", "echo", "expr", "false", "grep", "head", "id", "ls", "nl", "paste", "pwd", "rev", "seq", "stat", "tail", "tr", "true", "uname", "uniq", "wc", "which", "whoami": diff --git a/internal/agent/command_prefix_test.go b/internal/agent/command_prefix_test.go index eec799e7..0db9a98b 100644 --- a/internal/agent/command_prefix_test.go +++ b/internal/agent/command_prefix_test.go @@ -1,9 +1,11 @@ package agent import ( + "runtime" "testing" "github.com/Gitlawb/zero/internal/sandbox" + "github.com/Gitlawb/zero/internal/tools" ) func TestProposedCommandPrefixUsesSafeSimpleCommands(t *testing.T) { @@ -120,13 +122,35 @@ func TestProposedCommandPrefixRejectsUnsafeLaunchers(t *testing.T) { func TestMatchCommandPrefixCoversSegmentedCommandWithSafeTail(t *testing.T) { engine := sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: t.TempDir()}) engine.GrantCommandPrefixForSession("bash", []string{"ps", "aux"}) + // head is MSYS-prone on Windows (#458) and must not count as a known-safe tail. + command := "ps aux | head -5" + if runtime.GOOS == "windows" { + command = "ps aux | echo ok" + } - grant, ok, session := matchCommandPrefix("bash", map[string]any{"command": "ps aux | head -5"}, Options{Sandbox: engine}) + grant, ok, session := matchCommandPrefix("bash", map[string]any{"command": command}, Options{Sandbox: engine}) if !ok || !session || !equalStringSlices(grant.Prefix, []string{"ps", "aux"}) { t.Fatalf("match = %#v ok=%v session=%v, want session ps aux prefix", grant, ok, session) } } +func TestKnownSafeCommandSegmentRejectsMsysProneOnWindows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("windows-only known-safe MSYS guard") + } + for _, command := range [][]string{{"head", "-5"}, {"cat", "file.txt"}, {"grep", "pat"}} { + if knownSafeCommandSegment(command) { + t.Fatalf("expected %q to be unsafe on Windows, got known-safe", command) + } + } + if !knownSafeCommandSegment([]string{"echo", "ok"}) { + t.Fatal("expected echo to remain known-safe on Windows") + } + if !tools.MsysProneCommandName("head") { + t.Fatal("expected head to be MSYS-prone") + } +} + func TestMatchCommandPrefixRejectsUncoveredSegment(t *testing.T) { engine := sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: t.TempDir()}) engine.GrantCommandPrefixForSession("bash", []string{"ps", "aux"}) diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index 88052d06..06be5328 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "os" "path/filepath" "runtime" @@ -1945,6 +1946,11 @@ func TestRunCommandPrefixApprovalBypassesSandboxForMatchingShellCalls(t *testing func TestRunCommandPrefixApprovalCoversSegmentedShellWithSafeTail(t *testing.T) { root := t.TempDir() + segmentedCommand := `ps aux | head -5` + if runtime.GOOS == "windows" { + // head is MSYS-prone on Windows (#458) and no longer counts as a known-safe tail. + segmentedCommand = `ps aux | echo ok` + } retryTool := &sandboxDeniedRetryTool{} registry := tools.NewRegistry() registry.Register(retryTool) @@ -1958,7 +1964,7 @@ func TestRunCommandPrefixApprovalCoversSegmentedShellWithSafeTail(t *testing.T) }, { {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-2", ToolName: "bash"}, - {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-2", ArgumentsFragment: `{"command":"ps aux | head -5"}`}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-2", ArgumentsFragment: fmt.Sprintf(`{"command":%q}`, segmentedCommand)}, {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-2"}, {Type: zeroruntime.StreamEventDone}, }, @@ -2907,8 +2913,10 @@ func TestBuildSystemPromptInjectsHostShellContext(t *testing.T) { t.Fatalf("expected operating system in environment block, got %q", prompt) } if runtime.GOOS == "windows" { - if !strings.Contains(prompt, "Windows cmd.exe syntax") || !strings.Contains(prompt, "cwd argument") { - t.Fatalf("expected Windows cmd.exe shell guidance in prompt, got %q", prompt) + for _, want := range []string{"Windows cmd.exe syntax", "cwd argument", "MSYS binaries", "grep", "require_escalated"} { + if !strings.Contains(prompt, want) { + t.Fatalf("expected Windows shell guidance to mention %q, got %q", want, prompt) + } } } else if !strings.Contains(prompt, "/bin/sh syntax") { t.Fatalf("expected POSIX shell guidance in prompt, got %q", prompt) diff --git a/internal/agent/system_prompt.go b/internal/agent/system_prompt.go index 1a492695..46c51b62 100644 --- a/internal/agent/system_prompt.go +++ b/internal/agent/system_prompt.go @@ -286,7 +286,7 @@ func workspaceContext(cwd string) string { b.WriteString("Working directory: " + cwd + "\n") b.WriteString("Operating system: " + runtime.GOOS + "\n") if runtime.GOOS == "windows" { - b.WriteString("Shell syntax: Windows cmd.exe syntax for exec_command/bash tools; prefer the workdir/cwd argument instead of cd when changing directories.\n") + b.WriteString("Shell syntax: Windows cmd.exe syntax for exec_command/bash tools; prefer the workdir/cwd argument instead of cd when changing directories. Do not pipe to or invoke POSIX coreutils from Git for Windows (usr\\bin head/grep/tail/cat/...): they are MSYS binaries and fail under the write-restricted sandbox; use native Zero tools (grep, read_file, list_directory, glob) or cmd.exe findstr/more instead, or sandbox_permissions require_escalated only when host-level execution is truly required.\n") } else { b.WriteString("Shell syntax: /bin/sh syntax for exec_command/bash tools; prefer the workdir/cwd argument instead of cd when changing directories.\n") } diff --git a/internal/tools/bash_tool_test.go b/internal/tools/bash_tool_test.go index a3270ee6..839bc663 100644 --- a/internal/tools/bash_tool_test.go +++ b/internal/tools/bash_tool_test.go @@ -181,9 +181,13 @@ func TestDetectShellCommandIssueFlagsPipedPosixUtilities(t *testing.T) { `cat file.txt | wc -l`, `some_command | tail -n 20`, } { - if issue := detectShellCommandIssue(command, "windows"); issue == nil { + issue := detectShellCommandIssue(command, "windows") + if issue == nil { t.Fatalf("expected POSIX utility pipeline to be flagged for %q", command) } + if issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected windows_msys_sandbox for %q, got %q", command, issue.Kind) + } } } @@ -195,7 +199,7 @@ func TestDetectShellCommandIssueAllowsUnrelatedCommands(t *testing.T) { `echo header text`, `tail-log.sh`, `grep-cli --version`, - `sed.exe --help`, + `mytool.exe --help`, } { if issue := detectShellCommandIssue(command, "windows"); issue != nil { t.Fatalf("expected unrelated command to pass for %q, got %#v", command, issue) diff --git a/internal/tools/exec_command.go b/internal/tools/exec_command.go index 0feed7e1..e0495d49 100644 --- a/internal/tools/exec_command.go +++ b/internal/tools/exec_command.go @@ -860,6 +860,12 @@ func execToolResult(input execToolResultInput) Result { status = StatusError } body := formatExecCommandOutput(output, input.sessionID, input.exited, input.exitCode, input.interrupted) + if status == StatusError && input.exited && !input.interrupted { + if issue := detectShellOutputIssue(input.commandText, output, runtimeGOOS()); issue != nil { + meta["shell_issue"] = issue.Kind + body = appendShellIssueHint(body, *issue) + } + } if input.outputBufferTruncated { // Appended after truncateExecOutput's own head/tail slicing, not // embedded in the text that goes through it — a marker inside that diff --git a/internal/tools/shell_runtime.go b/internal/tools/shell_runtime.go index 9d5a0198..9bedbc91 100644 --- a/internal/tools/shell_runtime.go +++ b/internal/tools/shell_runtime.go @@ -17,14 +17,22 @@ type shellIssue struct { Suggestion string } +const windowsMsysSandboxSuggestion = "MSYS/Cygwin coreutils from Git for Windows cannot run under Zero's write-restricted Windows sandbox. Prefer Zero native tools (grep, read_file with offset/limit, list_directory, glob), cmd.exe findstr/more, or PowerShell Select-Object -First/-Last. If host-level execution is truly required, rerun with sandbox_permissions: \"require_escalated\" and a narrow justification." + var ( windowsBashStyleCDPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)cd\s+/(?:[a-ce-z0-9_./~-]|d[a-z0-9_./~-])[a-z0-9_./~-]*`) windowsLSCommandPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)ls\b(?:\s+|$)`) - // windowsPosixUtilityPattern catches the other common POSIX coreutils/grep - // family commands cmd.exe has no equivalent for (unlike `dir`/`findstr`, - // which sound similar enough that a model might reach for the POSIX name - // instead) — most often seen piped in, e.g. `git log ... | head`. - windowsPosixUtilityPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)(head|tail|grep|wc|awk|sed|cut|xargs|tr)(?:\s+|$)`) + // windowsMsysBinaryPathPattern catches explicit Git-for-Windows / MSYS usr\bin + // paths. These executables are valid Windows PE files but fail under the + // write-restricted sandbox with CreateFileMapping ACCESS_DENIED (#458). + windowsMsysBinaryPathPattern = regexp.MustCompile(`(?i)(?:\\usr\\bin\\|\\mingw64\\bin\\|msys-2\.0\.dll|cygwin1\.dll)`) + // windowsMsysPosixExecutablePattern catches quoted or unquoted invocations of + // MSYS coreutils by executable name (e.g. head.exe), including full paths. + windowsMsysPosixExecutablePattern = regexp.MustCompile(`(?i)(?:^|[&|;\s"])(?:[\w .:\\-]+\\)?(head|tail|grep|cat|cut|wc|nl|paste|tr|uniq|rev|seq|stat|uname|which|id|awk|sed|xargs)\.exe(?:\s+|$|")`) + // windowsPosixUtilityPattern catches POSIX coreutils invoked as bare command + // names (usually via PATH to Git usr\bin). Most often piped, e.g. + // `git log ... | head`, but also standalone `cat file.txt`. + windowsPosixUtilityPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)(head|tail|grep|cat|cut|wc|nl|paste|tr|uniq|rev|seq|stat|uname|which|id|awk|sed|xargs)(?:\s+|$)`) ) func detectShellRuntime(goos string) shellRuntime { @@ -37,23 +45,44 @@ func detectShellRuntime(goos string) shellRuntime { func shellGuidanceForGOOS(goos string) string { runtime := detectShellRuntime(goos) if goos == "windows" { - return "Uses " + runtime.Syntax + " syntax on Windows; prefer cwd over cd when changing directories." + return "Uses " + runtime.Syntax + " syntax on Windows; prefer cwd over cd when changing directories. MSYS/Cygwin coreutils on PATH (Git for Windows usr\\bin) are not sandbox-compatible; prefer native Zero file tools." } guidance := "Uses " + runtime.Syntax + " syntax." if goos == "darwin" { - // `ps` is setuid root and cannot run under the macOS sandbox; `pgrep` needs a - // blocked system service. Point the model at the tools that DO work so it - // doesn't waste turns: lsof to find a process, kill to stop it. guidance += " To find or stop a process, use `lsof -i :PORT` (or `lsof -nP -iTCP -sTCP:LISTEN`) for the PID then `kill `; `ps` and `pgrep` do not work under the sandbox." } return guidance } +// MsysProneCommandName reports whether a bare command name commonly resolves to +// a Git-for-Windows MSYS binary that fails under the Windows restricted sandbox. +func MsysProneCommandName(name string) bool { + switch strings.ToLower(strings.TrimSpace(name)) { + case "cat", "cut", "expr", "grep", "head", "id", "ls", "nl", "paste", "rev", + "seq", "stat", "tail", "tr", "uname", "uniq", "wc", "which", "awk", "sed", "xargs": + return true + default: + return false + } +} + +func windowsMsysSandboxIssue(message string) *shellIssue { + return &shellIssue{ + Kind: "windows_msys_sandbox", + Message: message, + Suggestion: windowsMsysSandboxSuggestion, + } +} + func detectShellCommandIssue(command string, goos string) *shellIssue { if goos != "windows" { return nil } trimmed := strings.TrimSpace(command) + if windowsMsysBinaryPathPattern.MatchString(trimmed) || + windowsMsysPosixExecutablePattern.MatchString(trimmed) { + return windowsMsysSandboxIssue("Command invokes an MSYS/Cygwin binary path that cannot run under Zero's Windows sandbox.") + } if windowsBashStyleCDPattern.MatchString(trimmed) || windowsLSCommandPattern.MatchString(trimmed) { return &shellIssue{ @@ -63,11 +92,7 @@ func detectShellCommandIssue(command string, goos string) *shellIssue { } } if windowsPosixUtilityPattern.MatchString(trimmed) { - return &shellIssue{ - Kind: "windows_shell_syntax", - Message: "Command uses a POSIX utility (head/tail/grep/wc/awk/sed/cut/xargs/tr) that Windows cmd.exe does not have.", - Suggestion: "Use cmd.exe equivalents (e.g. `findstr` for grep, `more` to page output) or Zero's native tools (grep, read_file with offset/limit) instead of piping to a POSIX utility.", - } + return windowsMsysSandboxIssue("Command uses a POSIX coreutil (head/tail/grep/cat/...) that commonly resolves to Git-for-Windows MSYS binaries incompatible with the Windows sandbox.") } return nil } @@ -76,9 +101,12 @@ func detectShellOutputIssue(command string, output string, goos string) *shellIs if goos != "windows" { return nil } - haystack := strings.ToLower(command + "\n" + output) - if strings.Contains(haystack, "the syntax of the command is incorrect") || - strings.Contains(haystack, "is not recognized as an internal or external command") { + lower := strings.ToLower(command + "\n" + output) + if msysRuntimeFailedInOutput(lower) { + return windowsMsysSandboxIssue("An MSYS/Cygwin runtime failed under Zero's Windows sandbox (ACCESS_DENIED during MSYS startup).") + } + if strings.Contains(lower, "the syntax of the command is incorrect") || + strings.Contains(lower, "is not recognized as an internal or external command") { return &shellIssue{ Kind: "windows_shell_syntax", Message: "Windows cmd.exe rejected the command syntax.", @@ -88,6 +116,22 @@ func detectShellOutputIssue(command string, output string, goos string) *shellIs return nil } +func msysRuntimeFailedInOutput(lower string) bool { + if strings.Contains(lower, "fatal error - createfilemapping") { + return true + } + if strings.Contains(lower, "couldn't create signal pipe") && strings.Contains(lower, "win32 error 5") { + return true + } + if strings.Contains(lower, "cygheap_user::init") && strings.Contains(lower, "fatal error") { + return true + } + if strings.Contains(lower, "usr\\bin\\") && strings.Contains(lower, "fatal error") { + return true + } + return strings.Contains(lower, "win32 error 5") && strings.Contains(lower, "terminating") +} + func appendShellIssueHint(output string, issue shellIssue) string { output = strings.TrimRight(output, "\r\n") hint := "[zero] shell issue: " + issue.Message diff --git a/internal/tools/shell_runtime_test.go b/internal/tools/shell_runtime_test.go new file mode 100644 index 00000000..e002fc97 --- /dev/null +++ b/internal/tools/shell_runtime_test.go @@ -0,0 +1,72 @@ +package tools + +import ( + "strings" + "testing" +) + +func TestDetectShellCommandIssueFlagsMsysBinaryPaths(t *testing.T) { + for _, command := range []string{ + `for /F %i in ('whoami') do echo %i | "C:\Program Files\Git\usr\bin\head.exe" -1`, + `C:\Git\usr\bin\grep.exe pattern file.txt`, + } { + issue := detectShellCommandIssue(command, "windows") + if issue == nil { + t.Fatalf("expected MSYS path issue for %q", command) + } + if issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected windows_msys_sandbox kind, got %q", issue.Kind) + } + if !strings.Contains(issue.Suggestion, "require_escalated") { + t.Fatalf("expected escalation guidance, got %#v", issue) + } + } +} + +func TestDetectShellCommandIssueFlagsStandaloneCat(t *testing.T) { + issue := detectShellCommandIssue(`cat README.md`, "windows") + if issue == nil || issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected MSYS sandbox issue for cat, got %#v", issue) + } +} + +func TestDetectShellOutputIssueFlagsMsysCreateFileMappingError(t *testing.T) { + output := `0 [main] head (3568) C:\Program Files\Git\usr\bin\head.exe: *** fatal error - CreateFileMapping S-1-5-21-3149109338-1484423945-518236903-1001.1, Win32 error 5. Terminating.` + issue := detectShellOutputIssue(`git log | head -5`, output, "windows") + if issue == nil || issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected MSYS output issue, got %#v", issue) + } + if !strings.Contains(issue.Suggestion, "require_escalated") { + t.Fatalf("expected escalation guidance, got %#v", issue) + } +} + +func TestDetectShellOutputIssueFlagsMsysSignalPipeError(t *testing.T) { + output := `0 [main] head (39684) cygheap_user::init: NtSetInformationToken (TokenDefaultDacl), 0xC0000022 +648 [main] head (39684) C:\Program Files\Git\usr\bin\head.exe: *** fatal error - couldn't create signal pipe, Win32 error 5` + issue := detectShellOutputIssue(`"C:\Program Files\Git\usr\bin\head.exe" --version`, output, "windows") + if issue == nil || issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected MSYS output issue, got %#v", issue) + } +} + +func TestShellIssueBlockResultMsysCommand(t *testing.T) { + result := shellIssueBlockResult(*detectShellCommandIssue(`cat README.md`, "windows")) + if result.Status != StatusError { + t.Fatalf("status = %q, want error", result.Status) + } + for _, want := range []string{"[zero] shell issue:", "MSYS/Cygwin", "grep", "read_file", "require_escalated"} { + if !strings.Contains(result.Output, want) { + t.Fatalf("expected %q in blocked output, got %q", want, result.Output) + } + } + if result.Meta["shell_issue"] != "windows_msys_sandbox" { + t.Fatalf("meta shell_issue = %q", result.Meta["shell_issue"]) + } +} + +func TestMsysProneCommandName(t *testing.T) { + if !MsysProneCommandName("HEAD") || MsysProneCommandName("echo") { + t.Fatalf("unexpected MsysProneCommandName results") + } +} From d40049a8bb211a8f9c7c34d4bc24cdfb10f016cd Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Sat, 4 Jul 2026 09:40:33 -0400 Subject: [PATCH 2/7] fix(tools): anchor the last MSYS failure heuristic to MSYS markers CodeRabbit review feedback: the win32-error-5 plus terminating check had no MSYS-specific anchor, so unrelated access-denied failures could be mislabeled as windows_msys_sandbox. Require an MSYS marker like the neighboring checks do. Co-Authored-By: Claude Fable 5 --- internal/tools/shell_runtime.go | 12 +++++++++++- internal/tools/shell_runtime_test.go | 16 ++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/internal/tools/shell_runtime.go b/internal/tools/shell_runtime.go index 9bedbc91..02b1230e 100644 --- a/internal/tools/shell_runtime.go +++ b/internal/tools/shell_runtime.go @@ -129,7 +129,17 @@ func msysRuntimeFailedInOutput(lower string) bool { if strings.Contains(lower, "usr\\bin\\") && strings.Contains(lower, "fatal error") { return true } - return strings.Contains(lower, "win32 error 5") && strings.Contains(lower, "terminating") + if !strings.Contains(lower, "win32 error 5") || !strings.Contains(lower, "terminating") { + return false + } + // Anchor the broad win32-error-5 fallback to an MSYS-specific marker so + // unrelated access-denied failures are not mislabeled as MSYS sandbox + // incompatibilities. + return strings.Contains(lower, `usr\bin\`) || + strings.Contains(lower, "cygheap") || + strings.Contains(lower, "msys-2.0.dll") || + strings.Contains(lower, "cygwin1.dll") || + strings.Contains(lower, "[main]") } func appendShellIssueHint(output string, issue shellIssue) string { diff --git a/internal/tools/shell_runtime_test.go b/internal/tools/shell_runtime_test.go index e002fc97..4764d7ff 100644 --- a/internal/tools/shell_runtime_test.go +++ b/internal/tools/shell_runtime_test.go @@ -50,6 +50,22 @@ func TestDetectShellOutputIssueFlagsMsysSignalPipeError(t *testing.T) { } } +func TestDetectShellOutputIssueFlagsMsysTerminatingWithMsysMarker(t *testing.T) { + output := `1 [main] tail (4321) tail: *** MapViewOfFileEx failed, Win32 error 5. Terminating.` + issue := detectShellOutputIssue(`git log | tail -5`, output, "windows") + if issue == nil || issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected MSYS output issue, got %#v", issue) + } +} + +func TestDetectShellOutputIssueIgnoresNonMsysWin32Error5(t *testing.T) { + output := `myapp.exe: unable to open service handle, Win32 error 5 (access denied). Terminating worker.` + issue := detectShellOutputIssue(`myapp.exe run`, output, "windows") + if issue != nil { + t.Fatalf("expected no issue for non-MSYS access-denied output, got %#v", issue) + } +} + func TestShellIssueBlockResultMsysCommand(t *testing.T) { result := shellIssueBlockResult(*detectShellCommandIssue(`cat README.md`, "windows")) if result.Status != StatusError { From 2b22c8006e2d0930184a79f19e0227945e21e29b Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Sat, 4 Jul 2026 18:32:40 -0400 Subject: [PATCH 3/7] fix(tools): let escalation bypass the MSYS guard and make detection quote-aware Addresses jatmn's review on PR #476: - bash and exec_command called detectShellCommandIssue before resolving sandbox_permissions into a command engine, so a require_escalated retry of a blocked command was still hard-blocked by the same preflight check. Resolve the command engine first and only bypass the windows_msys_sandbox issue kind when that resolves to true unsandboxed execution; other issue kinds (real cmd.exe syntax problems) still block regardless of escalation. - Unify all MSYS-prone command name checks (MsysProneCommandName, the preflight scan, and transitively command_prefix.go's known-safe guard) behind one windowsMsysProneNames set. This fixes expr (in the name set but missing from the old regex alternations, so never proactively flagged) and ls (hit the older windows_shell_syntax branch first, so it got different metadata than every other MSYS-prone name). - Make the preflight scan quote-aware: split the command into cmd.exe- operator-separated segments respecting double-quote grouping, and only check the first word of each segment instead of scanning raw text anywhere. Fixes false positives on quoted text like commit messages or PR comment bodies that merely mention a coreutil name. --- internal/tools/bash.go | 20 ++++- internal/tools/bash_tool_test.go | 64 ++++++++++++++ internal/tools/exec_command.go | 7 +- internal/tools/exec_command_test.go | 34 ++++++++ internal/tools/shell_runtime.go | 119 ++++++++++++++++++++++----- internal/tools/shell_runtime_test.go | 42 ++++++++++ 6 files changed, 261 insertions(+), 25 deletions(-) diff --git a/internal/tools/bash.go b/internal/tools/bash.go index 7361c346..2bf26b42 100644 --- a/internal/tools/bash.go +++ b/internal/tools/bash.go @@ -83,7 +83,12 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS if err != nil { return errorResult("Error: Invalid arguments for bash: " + err.Error()) } - if issue := detectShellCommandIssue(commandText, runtime.GOOS); issue != nil { + // Resolve the command engine before the MSYS preflight check so an + // approved require_escalated call (commandEngine == nil, truly + // unsandboxed) can actually bypass the MSYS guard instead of being + // hard-blocked by the same check it was meant to escalate past. + commandEngine := commandEngineForSandboxPermissions(engine, sandboxPermissions) + if issue := detectShellCommandIssue(commandText, runtime.GOOS); issue != nil && !msysGuardBypassed(issue, commandEngine) { return shellIssueBlockResult(*issue) } @@ -106,7 +111,6 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS "cwd": relativeCwd, "timeout_ms": strconv.Itoa(timeoutMS), } - commandEngine := commandEngineForSandboxPermissions(engine, sandboxPermissions) if commandEngine == nil && sandboxPermissions == SandboxPermissionsRequireEscalated { meta["sandbox_permissions"] = string(SandboxPermissionsRequireEscalated) } @@ -198,6 +202,18 @@ func commandEngineForSandboxPermissions(engine *zeroSandbox.Engine, sandboxPermi return engine } +// msysGuardBypassed reports whether a windows_msys_sandbox preflight issue no +// longer applies because sandbox_permissions: require_escalated actually +// resolved to unsandboxed, host-level execution (commandEngine is nil only +// then, per commandEngineForSandboxPermissions). MSYS coreutils fail because +// of the sandbox's restricted token/handles, so running outside it removes +// the failure mode this guard exists for. Any other issue kind (e.g. a real +// cmd.exe syntax problem) still blocks regardless of escalation, since +// running outside the sandbox does not fix a syntax error. +func msysGuardBypassed(issue *shellIssue, commandEngine *zeroSandbox.Engine) bool { + return issue.Kind == windowsMsysSandboxKind && commandEngine == nil +} + // appendSandboxBlocks appends a block listing the denials // the sandbox log monitor captured, so the model can see what was blocked. With no // blocks the stderr is returned unchanged. diff --git a/internal/tools/bash_tool_test.go b/internal/tools/bash_tool_test.go index 839bc663..f0c8e96d 100644 --- a/internal/tools/bash_tool_test.go +++ b/internal/tools/bash_tool_test.go @@ -438,6 +438,70 @@ func TestRegistryRunsWithDegradedUnavailableNativeSandbox(t *testing.T) { } } +// TestBashToolRequireEscalatedMsysGuard covers the boundary of the fix for an +// escalation-vs-preflight ordering bug: the MSYS sandbox guard exists only +// because MSYS/Cygwin coreutils fail under the write-restricted sandbox, so +// once sandbox_permissions: require_escalated is actually approved (running +// the command unsandboxed), the guard must get out of the way, but an +// unrelated windows_shell_syntax issue (a real cmd.exe syntax problem, not a +// sandbox interaction) must still block regardless of escalation. +func TestBashToolRequireEscalatedMsysGuard(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("windows-only MSYS sandbox guard") + } + root := t.TempDir() + newEngine := func() *sandbox.Engine { + return sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: root, + Policy: sandbox.DefaultPolicy(), + Backend: sandbox.Backend{Name: sandbox.BackendUnavailable, Message: "native sandbox unavailable"}, + }) + } + registry := NewRegistry() + registry.Register(NewBashTool(root)) + + t.Run("default sandboxing still blocks an MSYS-prone command", func(t *testing.T) { + result := registry.RunWithOptions(context.Background(), "bash", map[string]any{ + "command": "cat somefile.txt", + }, RunOptions{ + PermissionGranted: true, + Sandbox: newEngine(), + PermissionMode: string(sandbox.PermissionModeAsk), + }) + if result.Meta["shell_issue"] != "windows_msys_sandbox" { + t.Fatalf("expected windows_msys_sandbox block without escalation, got %#v", result) + } + }) + + t.Run("approved require_escalated bypasses the MSYS guard", func(t *testing.T) { + result := registry.RunWithOptions(context.Background(), "bash", map[string]any{ + "command": "cat somefile.txt", + "sandbox_permissions": string(SandboxPermissionsRequireEscalated), + }, RunOptions{ + PermissionGranted: true, + Sandbox: newEngine(), + PermissionMode: string(sandbox.PermissionModeAsk), + }) + if result.Meta["shell_issue"] != "" { + t.Fatalf("expected approved require_escalated to bypass the MSYS guard, got blocked: %#v", result) + } + }) + + t.Run("approved require_escalated still blocks an unrelated syntax issue", func(t *testing.T) { + result := registry.RunWithOptions(context.Background(), "bash", map[string]any{ + "command": `cd /d/tmp/zero-pr-158 && dir`, + "sandbox_permissions": string(SandboxPermissionsRequireEscalated), + }, RunOptions{ + PermissionGranted: true, + Sandbox: newEngine(), + PermissionMode: string(sandbox.PermissionModeAsk), + }) + if result.Meta["shell_issue"] != "windows_shell_syntax" { + t.Fatalf("expected windows_shell_syntax block to still apply under require_escalated, got %#v", result) + } + }) +} + func TestBashToolRunsWithDegradedUnavailableNativeSandbox(t *testing.T) { root := t.TempDir() policy := sandbox.DefaultPolicy() diff --git a/internal/tools/exec_command.go b/internal/tools/exec_command.go index e0495d49..41a9f8f4 100644 --- a/internal/tools/exec_command.go +++ b/internal/tools/exec_command.go @@ -517,7 +517,12 @@ func (tool execCommandTool) run(ctx context.Context, args map[string]any, engine if err != nil { return errorResult("Error: Invalid arguments for exec_command: " + err.Error()) } - if issue := detectShellCommandIssue(commandText, runtimeGOOS()); issue != nil { + // Resolve the command engine before the MSYS preflight check so an + // approved require_escalated call (commandEngine == nil, truly + // unsandboxed) can actually bypass the MSYS guard instead of being + // hard-blocked by the same check it was meant to escalate past. + commandEngine := commandEngineForSandboxPermissions(engine, sandboxPermissions) + if issue := detectShellCommandIssue(commandText, runtimeGOOS()); issue != nil && !msysGuardBypassed(issue, commandEngine) { return shellIssueBlockResult(*issue) } if interactive := zeroSandbox.DetectInteractiveCommand(commandText, runtimeGOOS()); interactive.Interactive { diff --git a/internal/tools/exec_command_test.go b/internal/tools/exec_command_test.go index b7743f50..5deb9c37 100644 --- a/internal/tools/exec_command_test.go +++ b/internal/tools/exec_command_test.go @@ -141,6 +141,40 @@ func TestExecCommandRequireEscalatedBypassesNativeSandboxAfterApproval(t *testin } } +// TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval mirrors +// TestBashToolRequireEscalatedMsysGuard for exec_command: the MSYS sandbox +// guard exists only because MSYS/Cygwin coreutils fail under the +// write-restricted sandbox, so once require_escalated is actually approved +// (unsandboxed execution), the guard must not block the same command it was +// meant to let escalate past. +func TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("windows-only MSYS sandbox guard") + } + root := t.TempDir() + manager := newExecSessionManager() + registry := NewRegistry() + registry.Register(NewScopedExecCommandTool(root, nil, manager)) + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: root, + Policy: sandbox.DefaultPolicy(), + Backend: sandbox.Backend{Name: sandbox.BackendUnavailable, Message: "native sandbox unavailable"}, + }) + + result := registry.RunWithOptions(context.Background(), ExecCommandToolName, map[string]any{ + "cmd": "cat somefile.txt", + "sandbox_permissions": string(SandboxPermissionsRequireEscalated), + }, RunOptions{ + PermissionGranted: true, + Sandbox: engine, + PermissionMode: string(sandbox.PermissionModeAsk), + }) + + if result.Meta["shell_issue"] != "" { + t.Fatalf("expected approved require_escalated to bypass the MSYS guard, got blocked: %#v", result) + } +} + func TestExecCommandReturnsExitCodeWhenCommandCompletesDuringInitialYield(t *testing.T) { root := t.TempDir() manager := newExecSessionManager() diff --git a/internal/tools/shell_runtime.go b/internal/tools/shell_runtime.go index 02b1230e..1ceaf1a4 100644 --- a/internal/tools/shell_runtime.go +++ b/internal/tools/shell_runtime.go @@ -17,22 +17,31 @@ type shellIssue struct { Suggestion string } +const windowsMsysSandboxKind = "windows_msys_sandbox" + const windowsMsysSandboxSuggestion = "MSYS/Cygwin coreutils from Git for Windows cannot run under Zero's write-restricted Windows sandbox. Prefer Zero native tools (grep, read_file with offset/limit, list_directory, glob), cmd.exe findstr/more, or PowerShell Select-Object -First/-Last. If host-level execution is truly required, rerun with sandbox_permissions: \"require_escalated\" and a narrow justification." +// windowsMsysProneNames is the single source of truth for POSIX coreutil names +// that commonly resolve to a Git-for-Windows MSYS/Cygwin binary rather than a +// cmd.exe-native command, and so fail under the write-restricted Windows +// sandbox (#458). Every Windows MSYS-detection path (the preflight command +// scan below, the exported MsysProneCommandName, and the known-safe-segment +// guard in internal/agent/command_prefix.go) derives from this one set, so +// they cannot drift out of sync with each other. +var windowsMsysProneNames = map[string]bool{ + "cat": true, "cut": true, "expr": true, "grep": true, "head": true, + "id": true, "ls": true, "nl": true, "paste": true, "rev": true, + "seq": true, "stat": true, "tail": true, "tr": true, "uname": true, + "uniq": true, "wc": true, "which": true, "awk": true, "sed": true, + "xargs": true, +} + var ( windowsBashStyleCDPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)cd\s+/(?:[a-ce-z0-9_./~-]|d[a-z0-9_./~-])[a-z0-9_./~-]*`) - windowsLSCommandPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)ls\b(?:\s+|$)`) // windowsMsysBinaryPathPattern catches explicit Git-for-Windows / MSYS usr\bin // paths. These executables are valid Windows PE files but fail under the // write-restricted sandbox with CreateFileMapping ACCESS_DENIED (#458). windowsMsysBinaryPathPattern = regexp.MustCompile(`(?i)(?:\\usr\\bin\\|\\mingw64\\bin\\|msys-2\.0\.dll|cygwin1\.dll)`) - // windowsMsysPosixExecutablePattern catches quoted or unquoted invocations of - // MSYS coreutils by executable name (e.g. head.exe), including full paths. - windowsMsysPosixExecutablePattern = regexp.MustCompile(`(?i)(?:^|[&|;\s"])(?:[\w .:\\-]+\\)?(head|tail|grep|cat|cut|wc|nl|paste|tr|uniq|rev|seq|stat|uname|which|id|awk|sed|xargs)\.exe(?:\s+|$|")`) - // windowsPosixUtilityPattern catches POSIX coreutils invoked as bare command - // names (usually via PATH to Git usr\bin). Most often piped, e.g. - // `git log ... | head`, but also standalone `cat file.txt`. - windowsPosixUtilityPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)(head|tail|grep|cat|cut|wc|nl|paste|tr|uniq|rev|seq|stat|uname|which|id|awk|sed|xargs)(?:\s+|$)`) ) func detectShellRuntime(goos string) shellRuntime { @@ -57,42 +66,108 @@ func shellGuidanceForGOOS(goos string) string { // MsysProneCommandName reports whether a bare command name commonly resolves to // a Git-for-Windows MSYS binary that fails under the Windows restricted sandbox. func MsysProneCommandName(name string) bool { - switch strings.ToLower(strings.TrimSpace(name)) { - case "cat", "cut", "expr", "grep", "head", "id", "ls", "nl", "paste", "rev", - "seq", "stat", "tail", "tr", "uname", "uniq", "wc", "which", "awk", "sed", "xargs": - return true - default: - return false - } + return windowsMsysProneNames[strings.ToLower(strings.TrimSpace(name))] } func windowsMsysSandboxIssue(message string) *shellIssue { return &shellIssue{ - Kind: "windows_msys_sandbox", + Kind: windowsMsysSandboxKind, Message: message, Suggestion: windowsMsysSandboxSuggestion, } } +// windowsCommandSegments splits a command into cmd.exe-operator-separated +// segments (&, |, ;, and their doubled forms &&/||), respecting double quotes +// (cmd.exe's grouping construct), so an operator or command name mentioned +// inside a quoted argument (e.g. a commit message or PR comment body) is not +// mistaken for a real segment boundary or invocation. +func windowsCommandSegments(command string) []string { + var segments []string + var current strings.Builder + inQuotes := false + for _, c := range command { + if c == '"' { + inQuotes = !inQuotes + current.WriteRune(c) + continue + } + if !inQuotes && (c == '&' || c == '|' || c == ';') { + if seg := strings.TrimSpace(current.String()); seg != "" { + segments = append(segments, seg) + } + current.Reset() + continue + } + current.WriteRune(c) + } + if seg := strings.TrimSpace(current.String()); seg != "" { + segments = append(segments, seg) + } + return segments +} + +// firstCommandWord returns the first token of a command segment. A leading +// double-quoted span counts as one token with its quotes stripped, since +// cmd.exe treats a quoted path as a single argument: the command invoked by +// `"C:\Program Files\Git\usr\bin\head.exe" file.txt` is the quoted path, not +// "C:\Program. +func firstCommandWord(segment string) string { + trimmed := strings.TrimSpace(segment) + if trimmed == "" { + return "" + } + if trimmed[0] == '"' { + if end := strings.IndexByte(trimmed[1:], '"'); end >= 0 { + return trimmed[1 : end+1] + } + return trimmed[1:] + } + fields := strings.Fields(trimmed) + if len(fields) == 0 { + return "" + } + return fields[0] +} + +// msysProneCommandWord reports whether word (the first token of a command +// segment, as returned by firstCommandWord) names an MSYS-prone coreutil, +// bare or with a directory prefix and/or .exe suffix (head, head.exe, +// C:\...\usr\bin\head.exe, ...). +func msysProneCommandWord(word string) bool { + word = strings.Trim(word, `"`) + if i := strings.LastIndexAny(word, `\/`); i >= 0 { + word = word[i+1:] + } + word = strings.TrimSuffix(strings.ToLower(word), ".exe") + return MsysProneCommandName(word) +} + func detectShellCommandIssue(command string, goos string) *shellIssue { if goos != "windows" { return nil } trimmed := strings.TrimSpace(command) - if windowsMsysBinaryPathPattern.MatchString(trimmed) || - windowsMsysPosixExecutablePattern.MatchString(trimmed) { + if windowsMsysBinaryPathPattern.MatchString(trimmed) { return windowsMsysSandboxIssue("Command invokes an MSYS/Cygwin binary path that cannot run under Zero's Windows sandbox.") } - if windowsBashStyleCDPattern.MatchString(trimmed) || - windowsLSCommandPattern.MatchString(trimmed) { + if windowsBashStyleCDPattern.MatchString(trimmed) { return &shellIssue{ Kind: "windows_shell_syntax", Message: "Command looks like POSIX/Bash syntax, but Zero runs bash tool commands through Windows cmd.exe on this host.", Suggestion: "Use the cwd argument instead of cd, use Windows cmd.exe syntax, or use native tools such as list_directory, read_file, grep, and glob.", } } - if windowsPosixUtilityPattern.MatchString(trimmed) { - return windowsMsysSandboxIssue("Command uses a POSIX coreutil (head/tail/grep/cat/...) that commonly resolves to Git-for-Windows MSYS binaries incompatible with the Windows sandbox.") + // Check the first word of each operator-separated segment (not the raw + // text anywhere in the command) against the single MSYS-prone name set, + // covering bare names (head), .exe names (head.exe), and directory- + // prefixed forms (C:\...\head.exe) uniformly. Being segment/word anchored + // rather than a whole-string regex, it never matches text that only + // appears inside a quoted argument. + for _, segment := range windowsCommandSegments(trimmed) { + if msysProneCommandWord(firstCommandWord(segment)) { + return windowsMsysSandboxIssue("Command uses a POSIX coreutil (head/tail/grep/cat/...) that commonly resolves to Git-for-Windows MSYS binaries incompatible with the Windows sandbox.") + } } return nil } diff --git a/internal/tools/shell_runtime_test.go b/internal/tools/shell_runtime_test.go index 4764d7ff..16e88c43 100644 --- a/internal/tools/shell_runtime_test.go +++ b/internal/tools/shell_runtime_test.go @@ -86,3 +86,45 @@ func TestMsysProneCommandName(t *testing.T) { t.Fatalf("unexpected MsysProneCommandName results") } } + +// TestDetectShellCommandIssueFlagsExprAndLsConsistently guards against the +// preflight regex list drifting from the canonical windowsMsysProneNames set +// (both listed expr and ls as MSYS-prone, but the old regex alternations +// omitted expr entirely and let ls hit the older windows_shell_syntax branch +// first, so it never got MSYS-kind guidance). +func TestDetectShellCommandIssueFlagsExprAndLsConsistently(t *testing.T) { + for _, command := range []string{ + `expr 1 + 1`, + `expr.exe 1 + 1`, + `ls -la`, + `ls`, + } { + issue := detectShellCommandIssue(command, "windows") + if issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected windows_msys_sandbox for %q, got %#v", command, issue) + } + } +} + +// TestDetectShellCommandIssueIgnoresQuotedMsysMentions guards against +// treating an MSYS-prone name that only appears inside a quoted argument +// (e.g. a commit message, a PR comment body, or a doc string discussing the +// command) as an actual invocation. The preflight check must anchor on the +// first word of each command segment, not scan the raw text anywhere. +func TestDetectShellCommandIssueIgnoresQuotedMsysMentions(t *testing.T) { + for _, command := range []string{ + `git commit -m "fix head.exe crash"`, + `gh pr comment --body "grep.exe fails under MSYS"`, + `echo "log | head is broken on windows"`, + `git commit -m "note: | head does not work here"`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue != nil { + t.Fatalf("expected quoted MSYS mention to pass for %q, got %#v", command, issue) + } + } + + // A real invocation alongside quoted text must still be caught. + if issue := detectShellCommandIssue(`echo "not a real command" && head file.txt`, "windows"); issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected real head invocation to still be flagged, got %#v", issue) + } +} From cb8fb69ac49607e7c558e3906c58058b14d4f947 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Sat, 4 Jul 2026 22:16:03 -0400 Subject: [PATCH 4/7] test(tools): decouple MSYS-guard bypass tests from real command output TestBashToolRequireEscalatedMsysGuard and TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval asserted shell_issue == "" to prove the require_escalated bypass worked. Once bypassed, "cat somefile.txt" actually runs unsandboxed, and its real, PATH-dependent output could trip the unrelated post-execution detectShellOutputIssue heuristic, failing the assertion for reasons unrelated to the preflight guard under test. Assert on the exit_code "-1" sentinel (set only by shellIssueBlockResult) instead, which directly reflects whether the preflight guard blocked the command. --- internal/tools/bash_tool_test.go | 8 +++++++- internal/tools/exec_command_test.go | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/internal/tools/bash_tool_test.go b/internal/tools/bash_tool_test.go index f0c8e96d..806a4a61 100644 --- a/internal/tools/bash_tool_test.go +++ b/internal/tools/bash_tool_test.go @@ -482,7 +482,13 @@ func TestBashToolRequireEscalatedMsysGuard(t *testing.T) { Sandbox: newEngine(), PermissionMode: string(sandbox.PermissionModeAsk), }) - if result.Meta["shell_issue"] != "" { + // Assert on the preflight block sentinel (exit_code "-1", set only by + // shellIssueBlockResult) rather than shell_issue: once the guard is + // bypassed, "cat somefile.txt" actually runs, and its real, + // PATH-dependent output could otherwise trip the unrelated + // post-execution detectShellOutputIssue heuristic and make this + // assertion flaky for reasons unrelated to the guard under test. + if result.Meta["exit_code"] == "-1" { t.Fatalf("expected approved require_escalated to bypass the MSYS guard, got blocked: %#v", result) } }) diff --git a/internal/tools/exec_command_test.go b/internal/tools/exec_command_test.go index 5deb9c37..92728526 100644 --- a/internal/tools/exec_command_test.go +++ b/internal/tools/exec_command_test.go @@ -170,7 +170,13 @@ func TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval(t *testing.T) PermissionMode: string(sandbox.PermissionModeAsk), }) - if result.Meta["shell_issue"] != "" { + // Assert on the preflight block sentinel (exit_code "-1", set only by + // shellIssueBlockResult) rather than shell_issue: once the guard is + // bypassed, "cat somefile.txt" actually runs, and its real, + // PATH-dependent output could otherwise trip the unrelated + // post-execution detectShellOutputIssue heuristic and make this + // assertion flaky for reasons unrelated to the guard under test. + if result.Meta["exit_code"] == "-1" { t.Fatalf("expected approved require_escalated to bypass the MSYS guard, got blocked: %#v", result) } } From 685543e8f3ca37cdd7916355387a1842c7b26824 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Sun, 5 Jul 2026 00:34:06 -0400 Subject: [PATCH 5/7] fix(tools): anchor MSYS path detection to command position, fix prefix approval gap Address jatmn's latest review on PR #476: - shell_runtime.go: the explicit MSYS binary-path check (usr\bin\, mingw64\bin\, msys-2.0.dll, cygwin1.dll) ran against the raw command string, so a quoted mention (e.g. a commit message describing a usr\bin\head.exe failure) was still hard-blocked. It now runs against the first word of each operator-separated segment, same as the coreutil-name check. windowsCommandSegments also stopped treating ; as a command separator (cmd.exe does not split on ;, unlike bash) and now respects cmd.exe's ^ escape character, so escaped operators like echo ^| head are no longer misread as a real head invocation. - command_prefix.go: proposedCommandPrefix could offer to approve a single segment (e.g. "ps aux" in "ps aux | head -5") without checking that every other segment was independently safe. Since approving a prefix escalates the whole command to bypass the sandbox, an MSYS-prone or otherwise unsafe segment could ride along uncovered. It now only proposes a segment as the prefix when every other segment in the command is known-safe on its own. Added regression tests for quoted MSYS path mentions, caret-escaped operators, semicolon text, and the prefix-approval gap. Co-Authored-By: Claude Sonnet 5 --- internal/agent/command_prefix.go | 30 ++++++++++++++++-- internal/agent/command_prefix_test.go | 25 +++++++++++++++ internal/tools/shell_runtime.go | 44 +++++++++++++++++--------- internal/tools/shell_runtime_test.go | 45 +++++++++++++++++++++++++++ 4 files changed, 126 insertions(+), 18 deletions(-) diff --git a/internal/agent/command_prefix.go b/internal/agent/command_prefix.go index 6a55610c..c577f6de 100644 --- a/internal/agent/command_prefix.go +++ b/internal/agent/command_prefix.go @@ -28,10 +28,20 @@ func proposedCommandPrefix(toolName string, args map[string]any) []string { } return nil } - for _, tokens := range segments { - if !knownSafeCommandSegment(tokens) && sandbox.ValidCommandPrefix(tokens) { - return append([]string(nil), tokens...) + // Only propose approving a prefix of one segment when every other segment + // in the command is independently known-safe. Once a prefix is approved, + // shellExecutionArgsForApproval escalates the whole command (every + // segment, not just the approved one) to bypass the sandbox, so offering a + // prefix that leaves an MSYS-prone (or otherwise unsafe) segment uncovered + // would let that segment run unsandboxed without ever being reviewed. + for index, tokens := range segments { + if knownSafeCommandSegment(tokens) { + continue } + if !sandbox.ValidCommandPrefix(tokens) || !otherSegmentsKnownSafe(segments, index) { + return nil + } + return append([]string(nil), tokens...) } if len(segments) == 0 || !sandbox.ValidCommandPrefix(segments[0]) { return nil @@ -39,6 +49,20 @@ func proposedCommandPrefix(toolName string, args map[string]any) []string { return append([]string(nil), segments[0]...) } +// otherSegmentsKnownSafe reports whether every segment other than the one at +// skip is known-safe on its own. +func otherSegmentsKnownSafe(segments [][]string, skip int) bool { + for index, tokens := range segments { + if index == skip { + continue + } + if !knownSafeCommandSegment(tokens) { + return false + } + } + return true +} + func matchCommandPrefix(toolName string, args map[string]any, options Options) (sandbox.CommandPrefixGrant, bool, bool) { if !isShellCommandTool(toolName) || options.Sandbox == nil { return sandbox.CommandPrefixGrant{}, false, false diff --git a/internal/agent/command_prefix_test.go b/internal/agent/command_prefix_test.go index 0db9a98b..69508a67 100644 --- a/internal/agent/command_prefix_test.go +++ b/internal/agent/command_prefix_test.go @@ -37,12 +37,37 @@ func TestProposedCommandPrefixHonorsValidatedRequestedPrefix(t *testing.T) { func TestProposedCommandPrefixSupportsSegmentedCommands(t *testing.T) { got := proposedCommandPrefix("bash", map[string]any{"command": "ps aux | head -5"}) + if runtime.GOOS == "windows" { + // head is MSYS-prone on Windows (#458), so proposedCommandPrefix must + // not offer "ps aux" as a reusable prefix here: approving it would + // escalate the whole command, including the uncovered head segment, + // to bypass the sandbox unreviewed. See + // TestProposedCommandPrefixRejectsPrefixLeavingUnsafeTailUncovered for + // the platform-independent regression coverage of this behavior. + if got != nil { + t.Fatalf("expected no prefix on Windows because head is MSYS-prone, got %#v", got) + } + return + } want := []string{"ps", "aux"} if !equalStringSlices(got, want) { t.Fatalf("prefix = %#v, want %#v", got, want) } } +// TestProposedCommandPrefixRejectsPrefixLeavingUnsafeTailUncovered guards +// against proposedCommandPrefix offering to approve one segment of a +// multi-segment command (e.g. "ps aux") while a different segment (e.g. "npm +// install") is not known-safe. shellExecutionArgsForApproval escalates the +// entire command once any prefix is approved, so an uncovered unsafe segment +// would bypass the sandbox unreviewed. Uses npm, which is never known-safe on +// any platform, so the assertion does not depend on runtime.GOOS. +func TestProposedCommandPrefixRejectsPrefixLeavingUnsafeTailUncovered(t *testing.T) { + if got := proposedCommandPrefix("bash", map[string]any{"command": "ps aux && npm install"}); got != nil { + t.Fatalf("expected no prefix because npm segment is not known-safe, got %#v", got) + } +} + func TestProposedCommandPrefixHonorsRequestedPrefixAcrossSegments(t *testing.T) { got := proposedCommandPrefix("bash", map[string]any{ "command": "git status --short && git status --branch", diff --git a/internal/tools/shell_runtime.go b/internal/tools/shell_runtime.go index 1ceaf1a4..853b9d12 100644 --- a/internal/tools/shell_runtime.go +++ b/internal/tools/shell_runtime.go @@ -78,21 +78,33 @@ func windowsMsysSandboxIssue(message string) *shellIssue { } // windowsCommandSegments splits a command into cmd.exe-operator-separated -// segments (&, |, ;, and their doubled forms &&/||), respecting double quotes -// (cmd.exe's grouping construct), so an operator or command name mentioned -// inside a quoted argument (e.g. a commit message or PR comment body) is not -// mistaken for a real segment boundary or invocation. +// segments (&, |, and their doubled forms &&/||), respecting double quotes +// (cmd.exe's grouping construct) and the caret (^) escape character, so an +// operator or command name mentioned inside a quoted argument (e.g. a commit +// message or PR comment body), or an operator escaped with ^ (cmd.exe prints +// `echo ^| head` as literal text instead of piping to head), is not mistaken +// for a real segment boundary or invocation. Unlike bash, cmd.exe does not +// treat ; as a statement separator, so it is left as ordinary argument text +// (e.g. `echo foo; head` is a single `echo` invocation with literal args). func windowsCommandSegments(command string) []string { var segments []string var current strings.Builder inQuotes := false - for _, c := range command { + runes := []rune(command) + for i := 0; i < len(runes); i++ { + c := runes[i] + if !inQuotes && c == '^' && i+1 < len(runes) { + current.WriteRune(c) + i++ + current.WriteRune(runes[i]) + continue + } if c == '"' { inQuotes = !inQuotes current.WriteRune(c) continue } - if !inQuotes && (c == '&' || c == '|' || c == ';') { + if !inQuotes && (c == '&' || c == '|') { if seg := strings.TrimSpace(current.String()); seg != "" { segments = append(segments, seg) } @@ -148,9 +160,6 @@ func detectShellCommandIssue(command string, goos string) *shellIssue { return nil } trimmed := strings.TrimSpace(command) - if windowsMsysBinaryPathPattern.MatchString(trimmed) { - return windowsMsysSandboxIssue("Command invokes an MSYS/Cygwin binary path that cannot run under Zero's Windows sandbox.") - } if windowsBashStyleCDPattern.MatchString(trimmed) { return &shellIssue{ Kind: "windows_shell_syntax", @@ -159,13 +168,18 @@ func detectShellCommandIssue(command string, goos string) *shellIssue { } } // Check the first word of each operator-separated segment (not the raw - // text anywhere in the command) against the single MSYS-prone name set, - // covering bare names (head), .exe names (head.exe), and directory- - // prefixed forms (C:\...\head.exe) uniformly. Being segment/word anchored - // rather than a whole-string regex, it never matches text that only - // appears inside a quoted argument. + // text anywhere in the command) against the MSYS binary-path pattern and + // the single MSYS-prone name set, covering bare names (head), .exe names + // (head.exe), and directory-prefixed forms (C:\...\head.exe) uniformly. + // Being segment/word anchored rather than a whole-string regex or scan, + // neither check matches text that only appears inside a quoted argument + // (e.g. a commit message or PR comment body discussing head.exe). for _, segment := range windowsCommandSegments(trimmed) { - if msysProneCommandWord(firstCommandWord(segment)) { + word := firstCommandWord(segment) + if windowsMsysBinaryPathPattern.MatchString(word) { + return windowsMsysSandboxIssue("Command invokes an MSYS/Cygwin binary path that cannot run under Zero's Windows sandbox.") + } + if msysProneCommandWord(word) { return windowsMsysSandboxIssue("Command uses a POSIX coreutil (head/tail/grep/cat/...) that commonly resolves to Git-for-Windows MSYS binaries incompatible with the Windows sandbox.") } } diff --git a/internal/tools/shell_runtime_test.go b/internal/tools/shell_runtime_test.go index 16e88c43..38c8179a 100644 --- a/internal/tools/shell_runtime_test.go +++ b/internal/tools/shell_runtime_test.go @@ -128,3 +128,48 @@ func TestDetectShellCommandIssueIgnoresQuotedMsysMentions(t *testing.T) { t.Fatalf("expected real head invocation to still be flagged, got %#v", issue) } } + +// TestDetectShellCommandIssueIgnoresQuotedMsysPathMentions guards the +// explicit MSYS-binary-path check the same way as the coreutil-name check: +// a full usr\bin\ path that only appears inside a quoted argument (e.g. a +// commit message describing the failure) must not be treated as an +// invocation, since the path check is now anchored to the first word of each +// command segment rather than scanned across the raw command text. +func TestDetectShellCommandIssueIgnoresQuotedMsysPathMentions(t *testing.T) { + for _, command := range []string{ + `git commit -m "C:\Program Files\Git\usr\bin\head.exe fails"`, + `gh pr comment --body "C:\Git\usr\bin\grep.exe is blocked"`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue != nil { + t.Fatalf("expected quoted MSYS path mention to pass for %q, got %#v", command, issue) + } + } + + // A real invocation by full path must still be caught. + if issue := detectShellCommandIssue(`C:\Git\usr\bin\grep.exe pattern file.txt`, "windows"); issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected real MSYS path invocation to still be flagged, got %#v", issue) + } +} + +// TestDetectShellCommandIssueRespectsCaretEscapedOperators guards against +// misreading cmd.exe's ^ escape character: `echo ^| head` prints the pipe and +// "head" as literal text (the caret escapes the pipe so it never splits into +// a separate `head` invocation), and `echo foo; head` is a single `echo` +// command with literal arguments since cmd.exe (unlike bash) does not treat +// ; as a statement separator. +func TestDetectShellCommandIssueRespectsCaretEscapedOperators(t *testing.T) { + for _, command := range []string{ + `echo ^| head`, + `echo ^& head`, + `echo foo; head`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue != nil { + t.Fatalf("expected no issue for %q, got %#v", command, issue) + } + } + + // An unescaped pipe must still split into a real head invocation. + if issue := detectShellCommandIssue(`echo foo | head`, "windows"); issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected real head invocation to still be flagged, got %#v", issue) + } +} From 91700b8d6c25ef688e8897d5821b3ed08f317d80 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Sun, 5 Jul 2026 02:52:31 -0400 Subject: [PATCH 6/7] fix(tools): treat redirection as a command boundary, stop using command text as output evidence Addresses jatmn's latest review on PR #476: - firstCommandWord only split on whitespace, so cmd.exe redirection attached directly to a command name (head>out.txt, catmatches.txt) was read as one word and never matched against the MSYS-prone name set. It now also stops at < and >. - detectShellOutputIssue scanned command+output together, so a command whose own argument text merely quoted an MSYS failure string (e.g. a gh pr comment --body describing the bug) could be mislabeled after running even though the real stdout/stderr was unrelated. Removed the command parameter entirely from detectShellOutputIssue and both call sites (bash.go, exec_command.go), so the command line can never be used as post-execution evidence again. --- internal/tools/bash.go | 8 +++--- internal/tools/bash_tool_test.go | 31 ++++++++++++++++++++- internal/tools/exec_command.go | 2 +- internal/tools/shell_runtime.go | 29 +++++++++++++++----- internal/tools/shell_runtime_test.go | 41 +++++++++++++++++++++++++--- 5 files changed, 94 insertions(+), 17 deletions(-) diff --git a/internal/tools/bash.go b/internal/tools/bash.go index 2bf26b42..5b1afe3c 100644 --- a/internal/tools/bash.go +++ b/internal/tools/bash.go @@ -171,7 +171,7 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS outText, errText, truncated := budgetBashCapture(stdoutText, stdout.total, stderrText, stderrTotal, meta) return Result{ Status: StatusError, - Output: formatBashOutputWithShellHint(commandText, outText, errText, exitCode, meta), + Output: formatBashOutputWithShellHint(outText, errText, exitCode, meta), Truncated: truncated, Meta: meta, } @@ -182,7 +182,7 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS if meta[SandboxLikelyDeniedMeta] == "true" { return Result{ Status: StatusError, - Output: formatBashOutputWithShellHint(commandText, outText, errText, exitCode, meta), + Output: formatBashOutputWithShellHint(outText, errText, exitCode, meta), Truncated: truncated, Meta: meta, } @@ -563,9 +563,9 @@ func truncateHeadTailWithTotal(value string, total, maxBytes int) (string, int, return utf8Prefix(value, head) + marker + utf8Suffix(value, tail), total, true } -func formatBashOutputWithShellHint(command string, stdout string, stderr string, exitCode int, meta map[string]string) string { +func formatBashOutputWithShellHint(stdout string, stderr string, exitCode int, meta map[string]string) string { output := formatBashOutput(stdout, stderr, exitCode) - if issue := detectShellOutputIssue(command, stdout+"\n"+stderr, runtime.GOOS); issue != nil { + if issue := detectShellOutputIssue(stdout+"\n"+stderr, runtime.GOOS); issue != nil { meta["shell_issue"] = issue.Kind output = appendShellIssueHint(output, *issue) } diff --git a/internal/tools/bash_tool_test.go b/internal/tools/bash_tool_test.go index 806a4a61..5fa7dfe4 100644 --- a/internal/tools/bash_tool_test.go +++ b/internal/tools/bash_tool_test.go @@ -208,7 +208,7 @@ func TestDetectShellCommandIssueAllowsUnrelatedCommands(t *testing.T) { } func TestDetectShellOutputIssueAddsWindowsSyntaxHint(t *testing.T) { - issue := detectShellOutputIssue(`cd /d/tmp/zero-pr-158 && ls -la`, "The syntax of the command is incorrect.", "windows") + issue := detectShellOutputIssue("The syntax of the command is incorrect.", "windows") if issue == nil { t.Fatal("expected Windows syntax error to get shell guidance") } @@ -508,6 +508,35 @@ func TestBashToolRequireEscalatedMsysGuard(t *testing.T) { }) } +// TestBashToolIgnoresMsysMarkersInCommandArgumentsAfterFailure guards the +// post-execution MSYS heuristic against treating the command line itself as +// evidence. The helper command below fails for a reason unrelated to MSYS +// (a plain non-zero exit), but its own argument text quotes an MSYS crash +// marker the way a PR-comment or commit-message argument might. Before the +// fix, detectShellOutputIssue scanned command+output together and would have +// misdiagnosed this as an MSYS sandbox failure even though the real +// stdout/stderr never mentions MSYS at all. +func TestBashToolIgnoresMsysMarkersInCommandArgumentsAfterFailure(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("windows-only MSYS output guard") + } + root := t.TempDir() + // The helper only reads argv[2] ("fail"); this trailing quoted argument is + // otherwise unused by the helper but still part of the command line text. + command := helperCommand("fail") + ` "fatal error - CreateFileMapping S-1-5-21, Win32 error 5. Terminating. cygheap_user::init"` + + result := NewBashTool(root).Run(context.Background(), map[string]any{ + "command": command, + }) + + if result.Status != StatusError || result.Meta["exit_code"] != "7" { + t.Fatalf("expected the helper's real exit 7 failure, got %s: %#v", result.Status, result) + } + if result.Meta["shell_issue"] == "windows_msys_sandbox" { + t.Fatalf("expected the MSYS marker in the command's own argument text to be ignored, got %#v", result) + } +} + func TestBashToolRunsWithDegradedUnavailableNativeSandbox(t *testing.T) { root := t.TempDir() policy := sandbox.DefaultPolicy() diff --git a/internal/tools/exec_command.go b/internal/tools/exec_command.go index 41a9f8f4..027eca89 100644 --- a/internal/tools/exec_command.go +++ b/internal/tools/exec_command.go @@ -866,7 +866,7 @@ func execToolResult(input execToolResultInput) Result { } body := formatExecCommandOutput(output, input.sessionID, input.exited, input.exitCode, input.interrupted) if status == StatusError && input.exited && !input.interrupted { - if issue := detectShellOutputIssue(input.commandText, output, runtimeGOOS()); issue != nil { + if issue := detectShellOutputIssue(output, runtimeGOOS()); issue != nil { meta["shell_issue"] = issue.Kind body = appendShellIssueHint(body, *issue) } diff --git a/internal/tools/shell_runtime.go b/internal/tools/shell_runtime.go index 853b9d12..72f9c7cc 100644 --- a/internal/tools/shell_runtime.go +++ b/internal/tools/shell_runtime.go @@ -3,6 +3,7 @@ package tools import ( "regexp" "strings" + "unicode" ) type shellRuntime struct { @@ -123,7 +124,11 @@ func windowsCommandSegments(command string) []string { // double-quoted span counts as one token with its quotes stripped, since // cmd.exe treats a quoted path as a single argument: the command invoked by // `"C:\Program Files\Git\usr\bin\head.exe" file.txt` is the quoted path, not -// "C:\Program. +// "C:\Program. For an unquoted command, the token ends at whitespace or a +// redirection operator (<, >): cmd.exe accepts redirection attached directly +// to the command name with no separating space (head>out.txt, catout.txt" as one word and +// miss the invoked command. func firstCommandWord(segment string) string { trimmed := strings.TrimSpace(segment) if trimmed == "" { @@ -135,11 +140,14 @@ func firstCommandWord(segment string) string { } return trimmed[1:] } - fields := strings.Fields(trimmed) - if len(fields) == 0 { - return "" + if end := strings.IndexFunc(trimmed, isCommandWordBoundary); end >= 0 { + return trimmed[:end] } - return fields[0] + return trimmed +} + +func isCommandWordBoundary(r rune) bool { + return unicode.IsSpace(r) || r == '<' || r == '>' } // msysProneCommandWord reports whether word (the first token of a command @@ -186,11 +194,18 @@ func detectShellCommandIssue(command string, goos string) *shellIssue { return nil } -func detectShellOutputIssue(command string, output string, goos string) *shellIssue { +// detectShellOutputIssue looks for MSYS runtime crash markers and cmd.exe +// syntax-error text in output only, never in the command that was run. The +// command line is attacker/user-controlled argument text (e.g. a `gh pr +// comment --body` quoting a sample failure), not something the shell +// produced, so treating it as evidence would reintroduce the same +// quoted-text false positives the preflight command-position check exists to +// avoid, just after execution instead of before it. +func detectShellOutputIssue(output string, goos string) *shellIssue { if goos != "windows" { return nil } - lower := strings.ToLower(command + "\n" + output) + lower := strings.ToLower(output) if msysRuntimeFailedInOutput(lower) { return windowsMsysSandboxIssue("An MSYS/Cygwin runtime failed under Zero's Windows sandbox (ACCESS_DENIED during MSYS startup).") } diff --git a/internal/tools/shell_runtime_test.go b/internal/tools/shell_runtime_test.go index 38c8179a..6c3ac5da 100644 --- a/internal/tools/shell_runtime_test.go +++ b/internal/tools/shell_runtime_test.go @@ -32,7 +32,7 @@ func TestDetectShellCommandIssueFlagsStandaloneCat(t *testing.T) { func TestDetectShellOutputIssueFlagsMsysCreateFileMappingError(t *testing.T) { output := `0 [main] head (3568) C:\Program Files\Git\usr\bin\head.exe: *** fatal error - CreateFileMapping S-1-5-21-3149109338-1484423945-518236903-1001.1, Win32 error 5. Terminating.` - issue := detectShellOutputIssue(`git log | head -5`, output, "windows") + issue := detectShellOutputIssue(output, "windows") if issue == nil || issue.Kind != "windows_msys_sandbox" { t.Fatalf("expected MSYS output issue, got %#v", issue) } @@ -44,7 +44,7 @@ func TestDetectShellOutputIssueFlagsMsysCreateFileMappingError(t *testing.T) { func TestDetectShellOutputIssueFlagsMsysSignalPipeError(t *testing.T) { output := `0 [main] head (39684) cygheap_user::init: NtSetInformationToken (TokenDefaultDacl), 0xC0000022 648 [main] head (39684) C:\Program Files\Git\usr\bin\head.exe: *** fatal error - couldn't create signal pipe, Win32 error 5` - issue := detectShellOutputIssue(`"C:\Program Files\Git\usr\bin\head.exe" --version`, output, "windows") + issue := detectShellOutputIssue(output, "windows") if issue == nil || issue.Kind != "windows_msys_sandbox" { t.Fatalf("expected MSYS output issue, got %#v", issue) } @@ -52,7 +52,7 @@ func TestDetectShellOutputIssueFlagsMsysSignalPipeError(t *testing.T) { func TestDetectShellOutputIssueFlagsMsysTerminatingWithMsysMarker(t *testing.T) { output := `1 [main] tail (4321) tail: *** MapViewOfFileEx failed, Win32 error 5. Terminating.` - issue := detectShellOutputIssue(`git log | tail -5`, output, "windows") + issue := detectShellOutputIssue(output, "windows") if issue == nil || issue.Kind != "windows_msys_sandbox" { t.Fatalf("expected MSYS output issue, got %#v", issue) } @@ -60,7 +60,7 @@ func TestDetectShellOutputIssueFlagsMsysTerminatingWithMsysMarker(t *testing.T) func TestDetectShellOutputIssueIgnoresNonMsysWin32Error5(t *testing.T) { output := `myapp.exe: unable to open service handle, Win32 error 5 (access denied). Terminating worker.` - issue := detectShellOutputIssue(`myapp.exe run`, output, "windows") + issue := detectShellOutputIssue(output, "windows") if issue != nil { t.Fatalf("expected no issue for non-MSYS access-denied output, got %#v", issue) } @@ -173,3 +173,36 @@ func TestDetectShellCommandIssueRespectsCaretEscapedOperators(t *testing.T) { t.Fatalf("expected real head invocation to still be flagged, got %#v", issue) } } + +// TestDetectShellCommandIssueFlagsRedirectionAttachedToCommand guards against +// firstCommandWord treating cmd.exe redirection operators as part of the +// command name. cmd.exe accepts redirection with no separating space +// (head>out.txt, catout.txt" as one word and miss the invoked command entirely. +func TestDetectShellCommandIssueFlagsRedirectionAttachedToCommand(t *testing.T) { + for _, command := range []string{ + `some-command | head>out.txt`, + `catmatches.txt pattern`, + } { + issue := detectShellCommandIssue(command, "windows") + if issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected windows_msys_sandbox for %q, got %#v", command, issue) + } + } +} + +// TestDetectShellOutputIssueSignatureOmitsCommandText documents, at the type +// level, that detectShellOutputIssue can no longer take the command line as +// evidence: it only accepts the real output. Harmless output must not be +// flagged, and output that genuinely carries the MSYS failure markers must +// still be flagged. +func TestDetectShellOutputIssueSignatureOmitsCommandText(t *testing.T) { + if issue := detectShellOutputIssue("hello from bash", "windows"); issue != nil { + t.Fatalf("expected no issue for harmless output, got %#v", issue) + } + output := `0 [main] head (3568) C:\Program Files\Git\usr\bin\head.exe: *** fatal error - CreateFileMapping ..., Win32 error 5. Terminating.` + if issue := detectShellOutputIssue(output, "windows"); issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected real MSYS output to still be flagged, got %#v", issue) + } +} From c55edd62dc3a6a729ce5a7038b134daa09cc13cb Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Sun, 5 Jul 2026 17:13:25 -0400 Subject: [PATCH 7/7] fix(tools): remove unused truncateHeadTail wrapper CodeRabbit nitpick: truncateHeadTailWithTotal is called directly everywhere, so this wrapper is dead code that trips golangci-lint's unused check and fails CI. --- internal/tools/bash.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/internal/tools/bash.go b/internal/tools/bash.go index 5b1afe3c..50489809 100644 --- a/internal/tools/bash.go +++ b/internal/tools/bash.go @@ -536,14 +536,6 @@ func (b *boundedBuffer) retained() string { return string(b.head) + string(b.tail) } -// truncateHeadTail keeps the first and last halves of value when it exceeds -// maxBytes, dropping the middle behind a marker. Returns the possibly-truncated -// text, the raw byte length, and whether truncation happened. -func truncateHeadTail(value string, maxBytes int) (string, int, bool) { - // value holds the whole stream, so its length is the true raw size. - return truncateHeadTailWithTotal(value, len(value), maxBytes) -} - // truncateHeadTailWithTotal head+tail-truncates value to maxBytes, using total — // the full original byte count — for the "N bytes omitted" marker and the raw // count. total may exceed len(value) when the middle was already discarded during