fix(tools): Block MSYS coreutils under Windows sandbox

[euxaristia]

Fixes #458

Summary

Git for Windows ships POSIX coreutils (head, grep, cat, and similar) as MSYS PE binaries on PATH. Under Zero's write-restricted Windows sandbox they fail at startup with ACCESS_DENIED, CreateFileMapping, signal-pipe, and cygheap errors instead of running the command. This leaves agents retrying broken shell pipelines.

Detect MSYS-prone commands before execution, surface MSYS runtime failures from command output, steer the agent toward native Zero tools or cmd/PowerShell alternatives, and exclude MSYS-prone command names from known-safe segmented-command tails on Windows.

Note: #458 may still need the issue-approved label per CONTRIBUTING.md before merge.

Changes

internal/tools/shell_runtime.go

• Add regex detection for MSYS binary paths on Windows, plus a quote-aware, segment/word-anchored scan for MSYS-prone command names (bare, .exe, or directory-prefixed).
• Export MsysProneCommandName() for shared guard logic, now backed by a single canonical name set shared by every detection path.
• Return windows_msys_sandbox issues from detectShellCommandIssue() with guidance toward native tools or require_escalated.
• Detect MSYS runtime failures in stderr via detectShellOutputIssue() (CreateFileMapping, signal pipe, cygheap).
• Expand shell guidance on Windows to warn about Git usr\bin MSYS coreutils.

internal/tools/bash.go, internal/tools/exec_command.go

• Apply output-issue hints when a command fails so MSYS startup errors get actionable suggestions.

internal/agent/command_prefix.go

• Treat MSYS-prone command names as not known-safe on Windows so segmented pipelines cannot bypass the guard.

internal/agent/system_prompt.go

• Add Windows environment guidance against piping to or invoking Git for Windows POSIX coreutils under the sandbox.

internal/tools/shell_runtime_test.go

• New tests for MSYS path detection, bare utility names, stderr failure patterns, quote-awareness, and expr/ls consistency.

internal/tools/bash_tool_test.go, internal/tools/exec_command_test.go, internal/agent/command_prefix_test.go, internal/agent/loop_test.go

• Update and extend tests for MSYS blocking, escalation bypass, and prompt guidance on Windows.

Updates addressing jatmn's review

1. Escalation path now actually bypasses the MSYS block. bash and exec_command were calling detectShellCommandIssue() before resolving sandboxPermissions into a command engine, so rerunning a blocked command with sandbox_permissions: "require_escalated" was still hard-blocked by the same preflight check it was supposed to escalate past. Both tools now resolve the command engine first and only bypass the windows_msys_sandbox issue kind when that resolves to true unsandboxed execution (an approved escalation). Any other issue kind (a real cmd.exe syntax problem) still blocks regardless of escalation, since running outside the sandbox does not fix a syntax error. Covered by TestBashToolRequireEscalatedMsysGuard and TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval.
2. One MSYS-prone command set for every detection path. windowsMsysProneNames is now the single source of truth: MsysProneCommandName(), the preflight command scan, and (transitively) the known-safe-segment guard in command_prefix.go all derive from it. This fixes two drift bugs: expr was in the name set but missing from the old regex alternations (so it was never proactively flagged), and ls hit the older windows_shell_syntax branch before ever reaching the MSYS check (so it got different metadata/guidance than every other MSYS-prone name). Covered by TestDetectShellCommandIssueFlagsExprAndLsConsistently.
3. MSYS detection is now quote-aware. The old regexes matched MSYS-prone names anywhere in the command text, so quoted argument text mentioning a coreutil name (a commit message, a PR comment body, echo "log | head is broken") could be misdetected as an actual invocation. Detection now splits the command into cmd.exe-operator-separated segments (respecting double-quote grouping) and only checks the first word of each segment, the actual command position, not the raw text anywhere. Covered by TestDetectShellCommandIssueIgnoresQuotedMsysMentions.
4. A second round applied the same command-position treatment to the explicit MSYS path check, and closed a prefix-approval gap. The binary-path detector (usr\bin\, mingw64\bin\, ...) still scanned raw command text, so a quoted path mention had the same false-positive risk item 3 fixed for coreutil names; it now runs against the same per-segment first word. The segment splitter also stopped treating ; as a separator (cmd.exe does not split on it, unlike bash) and now respects the ^ escape character, so echo ^| head is no longer misread as invoking head. Separately, command_prefix.go's proposedCommandPrefix could offer to approve a prefix (e.g. ps aux in ps aux | head -5) without checking that the remaining segments were independently safe, letting an MSYS-prone tail ride along uncovered once the whole command was escalated. It now only proposes a prefix when every other segment is known-safe on its own.
5. Command redirection is now a command-word boundary, and the post-execution heuristic no longer treats the command line as evidence. firstCommandWord split only on whitespace, so head>out.txt or cat<in.txt (cmd.exe accepts redirection with no separating space) were read as one word and missed entirely; it now also stops at </>. Separately, 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 when the real stdout/stderr was unrelated. It now takes only output (the command parameter is removed from its signature and both call sites), so the command line can never be used as evidence again. Covered by TestDetectShellCommandIssueFlagsRedirectionAttachedToCommand and TestBashToolIgnoresMsysMarkersInCommandArgumentsAfterFailure.

Test plan

• go build ./...
• go test ./internal/tools/... (pass)
• go test ./internal/agent/... (pass)
• go vet ./internal/tools/... ./internal/agent/... (clean)
• Manual: head.exe / git log | head blocked or surfaced with MSYS sandbox guidance on Windows

Summary by CodeRabbit

• Bug Fixes
  • Improved Windows safety checks for MSYS/Git-for-Windows usage by tightening command-segment and operator parsing, including safer handling of pipelines and redirection.
  • More accurately blocks and labels MSYS-related attempts, and adds clearer shell-issue hints based on command outcomes.
  • Refined command approval behavior so risky prefixes are only approved when the rest of the command is independently safe.
• Documentation
  • Expanded Windows system prompt guidance to avoid MSYS coreutils and clarify escalation usage.
• Tests
  • Extended Windows coverage for MSYS detection, quoting edge cases, escalation bypass behavior, and output-only failure classification.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 10215421-cd0c-4796-9c0d-eb1504383bd8

📥 Commits

Reviewing files that changed from the base of the PR and between 91700b8 and c55edd6.

📒 Files selected for processing (1)

• internal/tools/bash.go

💤 Files with no reviewable changes (1)

• internal/tools/bash.go

---

Walkthrough

This PR adds Windows MSYS/Cygwin sandbox detection, threads the new windows_msys_sandbox issue through bash/exec handling, updates Windows command-prefix and prompt guidance, and expands tests for detection, blocking, and require_escalated bypass behavior.

Changes

MSYS/Cygwin Sandbox Detection

Layer / File(s)	Summary
Windows MSYS/Cygwin issue detection core internal/tools/shell_runtime.go	Adds the windows_msys_sandbox issue kind, MSYS-prone command matching, and MSYS runtime-failure output detection for Windows shell checks.
Exec and bash MSYS guard wiring internal/tools/bash.go, internal/tools/exec_command.go	Moves sandbox-resolution earlier, gates bypasses through msysGuardBypassed, and records shell-output issues in exec results.
Windows command-prefix and prompt policy internal/agent/command_prefix.go, internal/agent/system_prompt.go, internal/agent/loop_test.go	Rejects MSYS-prone command names on Windows, expands Windows shell guidance, and updates Windows-facing prefix and prompt tests.
Windows MSYS detection tests internal/tools/shell_runtime_test.go, internal/tools/bash_tool_test.go, internal/tools/exec_command_test.go, internal/agent/command_prefix_test.go	Expands Windows-only coverage for MSYS-prone commands, quoted arguments, output failures, blocked results, and require_escalated bypass behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant bashTool.run
  participant detectShellCommandIssue
  participant msysGuardBypassed
  participant execToolResult

  bashTool.run->>detectShellCommandIssue: inspect command text
  detectShellCommandIssue-->>bashTool.run: windows_msys_sandbox or nil
  bashTool.run->>msysGuardBypassed: check issue + commandEngine
  msysGuardBypassed-->>bashTool.run: allow or block
  execToolResult->>execToolResult: detectShellOutputIssue on stdout + stderr

Loading

Possibly related issues

• MSYS/Cygwin coreutils (head, grep, tail, ...) fail under the Windows sandbox, separately from the pipe issue in #456 #458: Directly describes MSYS/Cygwin coreutils failing under the Windows sandbox, which this PR detects and blocks.
• Nine bugs found in a multi-agent codebase audit (sandbox, oauth, TUI, update, sessions) #480: Overlaps with the same Windows shell-command detection path and MSYS sandbox handling in internal/tools/shell_runtime.go.

Possibly related PRs

• Gitlawb/zero#322: Shares the segmented command-prefix logic and Windows-safe segment handling.
• Gitlawb/zero#412: Directly overlaps with Windows shell-runtime heuristics and shell-issue handling.
• Gitlawb/zero#49: Shares the bash tool execution path and bypass wiring used by the new MSYS guard.

Suggested reviewers: gnanam1990, Vasanthdev2004, kevincodex1

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately describes the main Windows sandbox MSYS-blocking change.
Linked Issues check	✅ Passed	The changes address #458 by detecting MSYS-prone coreutils, adding Windows sandbox guidance, and steering segmented commands away from unsafe MSYS tails.
Out of Scope Changes check	✅ Passed	No clearly unrelated code changes stand out; the additions are all tied to Windows MSYS detection, guidance, or related tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/tools/shell_runtime.go (1)

57-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

expr is MSYS-prone but never actually detected.

MsysProneCommandName (Line 61) lists expr as an MSYS-prone command name, but neither windowsMsysPosixExecutablePattern (Line 31) nor windowsPosixUtilityPattern (Line 35) include expr in their alternations. So detectShellCommandIssue will never flag a bare expr ... invocation on Windows, even though knownSafeCommandSegment (in command_prefix.go) will reject it as unsafe. The model gets no proactive guidance for this specific command even though the rest of the system treats it as MSYS-prone.

🔧 Proposed fix

-	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+|$|")`)
+	windowsMsysPosixExecutablePattern = regexp.MustCompile(`(?i)(?:^|[&|;\s"])(?:[\w .:\\-]+\\)?(head|tail|grep|cat|cut|wc|nl|paste|tr|uniq|rev|seq|stat|uname|which|id|expr|awk|sed|xargs)\.exe(?:\s+|$|")`)
 	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+|$)`)
+	windowsPosixUtilityPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)(head|tail|grep|cat|cut|wc|nl|paste|tr|uniq|rev|seq|stat|uname|which|id|expr|awk|sed|xargs)(?:\s+|$)`)

🤖 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/shell_runtime.go` around lines 57 - 96, The Windows shell
detection is missing bare expr even though MsysProneCommandName already treats
it as MSYS-prone. Update the Windows command-issue detection in
detectShellCommandIssue by adding expr to the same matching path used for other
POSIX utilities, likely via the windowsPosixUtilityPattern or the MSYS-prone
executable handling alongside windowsMsysPosixExecutablePattern. Keep the
existing behavior and make sure a bare expr invocation now returns a
windows_msys_sandbox issue with the same kind of guidance as the other
MSYS-prone commands.

🤖 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 119-134: The final fallback in msysRuntimeFailedInOutput is too
broad and can misclassify non-MSYS failures as windows_msys_sandbox. Tighten the
last return condition in msysRuntimeFailedInOutput by adding a stronger
MSYS-specific anchor, such as requiring usr\\bin, cygheap, or an MSYS-style log
prefix like [main], alongside the existing win32 error 5 and terminating checks.
Keep the earlier more-specific branches unchanged and make the fallback only
match genuine MSYS runtime failures.

---

Outside diff comments:
In `@internal/tools/shell_runtime.go`:
- Around line 57-96: The Windows shell detection is missing bare expr even
though MsysProneCommandName already treats it as MSYS-prone. Update the Windows
command-issue detection in detectShellCommandIssue by adding expr to the same
matching path used for other POSIX utilities, likely via the
windowsPosixUtilityPattern or the MSYS-prone executable handling alongside
windowsMsysPosixExecutablePattern. Keep the existing behavior and make sure a
bare expr invocation now returns a windows_msys_sandbox issue with the same kind
of guidance as the other MSYS-prone commands.

🪄 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: e6d23b5c-35e6-428c-a314-98b418ed13a3

📥 Commits

Reviewing files that changed from the base of the PR and between 949ee43 and 365296f.

📒 Files selected for processing (8)

• internal/agent/command_prefix.go
• internal/agent/command_prefix_test.go
• internal/agent/loop_test.go
• internal/agent/system_prompt.go
• internal/tools/bash_tool_test.go
• internal/tools/exec_command.go
• internal/tools/shell_runtime.go
• internal/tools/shell_runtime_test.go

[Vasanthdev2004]

Approve — genuine Windows-sandbox hardening; two edge-case false-positives worth a fast-follow

Worth merging: yes. This fixes a real, documented failure (#458): Git-for-Windows MSYS coreutils fail under Zero's write-restricted sandbox token and strand the agent in retry loops. The layered approach — pre-exec block + reactive stderr detection + allowlist tightening + prompt guidance — is well-tested and correct. Thanks @euxaristia.

Reviewed as part of the 6-PR Windows-sandbox cluster, with the detection logic ground-truthed against the actual code and independently re-verified. Note the CHANGES_REQUESTED here is the CodeRabbit bot, not a human — not a merge gate.

Two real but minor edge cases I'd fold in as a fast-follow (non-blocking):

1. The require_escalated escape hatch it advertises is self-blocked. detectShellCommandIssue fires unconditionally (bash.go:87 / exec_command.go:520) before sandboxPermissions is consulted, so re-running the same command with require_escalated — exactly what the new message suggests — gets hard-blocked identically and never reaches the escalated path that would actually succeed. The advertised recovery isn't actionable through the same tool.
2. The .exe regex over-matches. windowsMsysPosixExecutablePattern's leading class [&|;\s"] matches a coreutil .exe token appearing anywhere as an argument or inside a quoted string — so git commit -m "fix head.exe crash" is hard-blocked as a false MSYS invocation. Uncommon, but plausible in a repo that literally discusses head.exe/grep.exe. Anchor it to the invoked-program position the way the bare windowsPosixUtilityPattern already is. (Also: expr is missing from the util set.)

Neither blocks — both are recoverable via native tools and the core fix is correct. Merge as-is and file one polish issue, or fold them in.

Sequencing: land after #465 — both touch bash_tool_test.go with different test functions, so keep a single copy of the echo-arg helper on rebase.

[jatmn]

I found a few issues that need to be addressed before this is ready.

Findings

• [P2] Make the advertised escalation path actually bypass the MSYS block
  internal/tools/bash.go:83
  The new MSYS hint tells the model to rerun with sandbox_permissions: "require_escalated" when host-level execution is truly required, but both bash and exec_command call detectShellCommandIssue before the parsed sandboxPermissions is used to choose the unsandboxed command engine. That means rerunning the exact blocked command with require_escalated is still hard-blocked by the same preflight result and never reaches the escalation path. Please either let approved require_escalated calls bypass this MSYS preflight check or remove the escalation suggestion so the recovery guidance matches the actual behavior.

• [P2] Use one MSYS-prone command set for every detection path
  internal/tools/shell_runtime.go:31
  CodeRabbit's outside-diff request is still valid, and it points at a broader drift problem: this PR now has a central MsysProneCommandName list, but detectShellCommandIssue still maintains separate regex alternations. expr is listed as MSYS-prone and knownSafeCommandSegment rejects it on Windows, yet bare expr ... and expr.exe miss the proactive windows_msys_sandbox guidance because the regexes omit it. ls has the opposite inconsistency: it is listed as MSYS-prone, but it hits the old windows_shell_syntax branch before the MSYS branch, so it gets different metadata and recovery guidance from the rest of the shared set. Please make the preflight detection derive from the same MSYS-prone command set, or otherwise keep the lists and result kinds explicitly synchronized, and add regression coverage for expr and ls.

• [P2] Make the MSYS command detection quote-aware
  internal/tools/shell_runtime.go:31
  The new MSYS preflight regexes do not understand shell quoting, so they can treat harmless argument text as an executable command. windowsMsysPosixExecutablePattern matches head.exe/grep.exe after any whitespace or quote, which hard-blocks commands like git commit -m "fix head.exe crash" or gh pr comment --body "grep.exe fails under MSYS". The bare utility regex has the same boundary problem around quoted operators, so echo "try git log | head" can be blocked as if head were actually being invoked. This repo now explicitly discusses those command names, so the false positive is likely during follow-up commits, PR comments, or docs edits. Please anchor detection to parsed command positions or explicit MSYS paths, and add regression tests for quoted argument text.

[coderabbitai]

Actionable comments posted: 1

🤖 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/exec_command_test.go`:
- Around line 164-176: The Windows-only exec command test is using a
PATH-dependent command in the RunWithOptions call, so the result can be polluted
by a shell_issue even when the sandbox bypass works. Update the test in
exec_command_test.go to use a guaranteed native command instead of the current
cat invocation, or assert the require_escalated bypass directly from
RunWithOptions/ExecCommandToolName behavior so the check does not depend on
shell availability.

🪄 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: 88ad9884-a6d4-4623-9219-de8285ddd2ba

📥 Commits

Reviewing files that changed from the base of the PR and between 96afeec and b4e58a6.

📒 Files selected for processing (6)

• internal/tools/bash.go
• internal/tools/bash_tool_test.go
• internal/tools/exec_command.go
• internal/tools/exec_command_test.go
• internal/tools/shell_runtime.go
• internal/tools/shell_runtime_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/tools/shell_runtime_test.go

[euxaristia]

Status update on the open findings:

Already fixed on this branch before this push:

• jatmn's escalation-bypass finding (bash.go/exec_command.go): commandEngine is now resolved before the MSYS preflight check, and msysGuardBypassed lets an approved require_escalated call through (b4e58a6).
• jatmn's inconsistent-detection-set finding and CodeRabbit's missing-expr finding: shell_runtime.go now has one windowsMsysProneNames map as the single source of truth, used by both the preflight scan and MsysProneCommandName, so expr/ls can't drift out of sync (b4e58a6/365296f).
• jatmn's quote-awareness finding: windowsCommandSegments now respects double quotes and firstCommandWord handles quoted first tokens, so text inside a quoted argument (e.g. a commit message mentioning head.exe) is no longer mistaken for an invocation (b4e58a6).
• CodeRabbit's msysRuntimeFailedInOutput fallback finding: the win32-error-5 fallback now requires an MSYS-specific anchor (usr\bin, cygheap, msys-2.0.dll, cygwin1.dll, or [main]) (96afeec).

Fixed in 304a5a5 (this push): CodeRabbit's last finding, that TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval asserted shell_issue == "" after running the real cat somefile.txt unsandboxed, which is PATH-dependent and could spuriously fail if that command's real output happened to trip the unrelated post-execution detectShellOutputIssue heuristic. Same latent issue existed in the sibling TestBashToolRequireEscalatedMsysGuard, so fixed both: both now assert on the exit_code "-1" preflight-block sentinel instead, which is what they actually mean to test.

go build/vet clean on both linux and windows targets, go test ./internal/tools/... ./internal/agent/... green.

@jatmn ready for re-review.

[jatmn]

Thanks for the update. I rechecked the changed paths and found a couple of issues that still need to be addressed.

Findings

• [P2] Apply cmd.exe command-position parsing to every MSYS preflight path
  internal/tools/shell_runtime.go:151
  The quote-aware fix only covers the bare-name scan, but the explicit MSYS path detector still runs against the entire raw command before windowsCommandSegments/firstCommandWord is used. That means commands such as git commit -m "C:\Program Files\Git\usr\bin\head.exe fails" or gh pr comment --body "C:\Git\usr\bin\grep.exe is blocked" are still hard-blocked even though the MSYS path is just quoted argument text, which is the same class of false positive the latest parser was meant to avoid. The segment splitter also does not match cmd.exe tokenization: it treats caret-escaped operators and ; as command separators, so echo ^| head and echo foo; head are classified as actual head invocations even though cmd.exe prints those as argument text. Please route the explicit path check through a real command-position parser, or otherwise make the parser handle quoted/escaped argument text consistently, and add regressions for quoted MSYS paths, caret-escaped operators, and semicolon text.

• [P2] Do not offer a prefix approval that leaves an MSYS-prone tail uncovered
  internal/agent/command_prefix.go:31
  This PR removes MSYS-prone commands from knownSafeCommandSegment on Windows, but proposedCommandPrefix still returns the first non-safe valid segment before proving the remaining segments are safe. For a command like ps aux | head -5, the prompt can still offer only ps aux as the reusable prefix; if the user chooses that option, shellExecutionArgsForApproval adds sandbox_permissions: "require_escalated" to the entire command and the first execution runs the uncovered head segment unsandboxed. That undercuts the new guard because the current command can still bypass the sandbox through a prefix that does not cover the MSYS-prone segment. Please only offer prefix approval when every other segment is genuinely known-safe for the current OS, or include the unsafe segment in the approval flow instead of escalating the whole command under the earlier prefix.

[jatmn]

Thanks for the update. I rechecked the changed paths and found a couple of issues that still need to be addressed.

Findings

• [P2] Treat cmd redirection as a command-name boundary
  internal/tools/shell_runtime.go:177
  The new command-position scan only checks the first whitespace-delimited word of each cmd.exe segment. That still misses real MSYS-prone invocations when cmd redirection is attached directly to the command name, which cmd.exe accepts. For example, some-command | head>out.txt, cat<in.txt, and grep>matches.txt all invoke the same MSYS-prone utilities, but firstCommandWord returns head>out.txt/cat<in.txt/grep>matches.txt, so msysProneCommandWord never matches the bare command name. Please strip or split cmd redirection operators when extracting the invoked command word, and add regressions for attached </> redirection.

• [P2] Keep post-execution MSYS detection from matching quoted command text
  internal/tools/shell_runtime.go:193
  The preflight path now avoids quoted MSYS-path mentions, but the output heuristic still lowercases and scans command + "\n" + output. That means a failed command whose arguments merely contain an MSYS failure string, such as a gh pr comment --body or commit command quoting C:\Git\usr\bin\head.exe: *** fatal error ..., can be mislabeled as windows_msys_sandbox even when the actual failure output is unrelated. This reintroduces the same false-positive class the command-position parser just fixed, only after execution. Please base the MSYS runtime heuristic on the command's actual output/stderr, or otherwise apply the same command-position/quote-aware treatment before using command text as evidence.

Process note: the linked issue #458 still does not have the issue-approved label, while CONTRIBUTING.md says community PRs require an approved parent issue. Please wait for maintainer confirmation on whether this PR can proceed under that policy.

[jatmn]

@Vasanthdev2004 LGTM

[Vasanthdev2004]

Re-approving. My earlier approval predates the hardening you and jatmn worked through, so I read the delta since then rather than let a stale approval ride on a sandbox PR.

The part that matters most is the escalation hole in proposedCommandPrefix, and it's closed correctly: since approving a prefix escalates the whole command past the sandbox, it now refuses to propose one unless every other segment is independently known-safe, and knownSafeCommandSegment no longer treats MSYS-prone coreutils as safe on Windows. That's the actual security boundary and it holds.

The rest is advisory-hint quality, and it's careful too — detection is anchored to the first word of each cmd.exe segment (so head.exe quoted in a commit message or PR body doesn't false-trigger), the output scan reads output only instead of the command line, the win32-error-5 fallback is anchored to MSYS markers so an unrelated ACCESS_DENIED isn't mislabeled, and the prone-name set is one source of truth shared by every path so they can't drift. Test coverage across all of it is good.

Solid. Over to kevin.

[kevincodex1]

please rebase from main and fix conflicts @euxaristia

[euxaristia]

@kevincodex1 Done, rebased onto main and resolved the conflict in bash.go (main had already merged a stdout/stderr budget-capture rewrite that overlapped with this PR's changes). Force-pushed. GitHub now shows this as mergeable with no conflicts.

[coderabbitai]

🧹 Nitpick comments (1)

internal/tools/bash.go (1)

539-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Dead code: truncateHeadTail is unused and trips the linter.

golangci-lint flags this as an unused error, so it'll fail CI. Nothing calls the wrapper — truncateHeadTailWithTotal is used directly. Delete it.

🧹 Proposed removal

-// 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)
-}
-

Confirm it's truly unreferenced (incl. tests / exec_command.go) before removing:

#!/bin/bash
rg -nP '\btruncateHeadTail\b\s*\(' --type=go

🤖 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.go` around lines 539 - 545, `truncateHeadTail` is dead
code and only wraps `truncateHeadTailWithTotal`, so it should be removed if it
is truly unreferenced. Confirm there are no callers in the Go tree, including
tests and `exec_command.go` (use the `truncateHeadTail` symbol to verify), then
delete the unused `truncateHeadTail` function and keep
`truncateHeadTailWithTotal` as the single implementation.

Source: Linters/SAST tools

🤖 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.

Nitpick comments:
In `@internal/tools/bash.go`:
- Around line 539-545: `truncateHeadTail` is dead code and only wraps
`truncateHeadTailWithTotal`, so it should be removed if it is truly
unreferenced. Confirm there are no callers in the Go tree, including tests and
`exec_command.go` (use the `truncateHeadTail` symbol to verify), then delete the
unused `truncateHeadTail` function and keep `truncateHeadTailWithTotal` as the
single implementation.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 34b4ec18-8d1a-4f5e-90e0-ed7b864925ff

📥 Commits

Reviewing files that changed from the base of the PR and between 136d720 and 91700b8.

📒 Files selected for processing (10)

• internal/agent/command_prefix.go
• internal/agent/command_prefix_test.go
• internal/agent/loop_test.go
• internal/agent/system_prompt.go
• internal/tools/bash.go
• internal/tools/bash_tool_test.go
• internal/tools/exec_command.go
• internal/tools/exec_command_test.go
• internal/tools/shell_runtime.go
• internal/tools/shell_runtime_test.go

✅ Files skipped from review due to trivial changes (1)

• internal/agent/system_prompt.go

🚧 Files skipped from review as they are similar to previous changes (8)

• internal/tools/exec_command_test.go
• internal/tools/exec_command.go
• internal/agent/loop_test.go
• internal/tools/shell_runtime_test.go
• internal/tools/bash_tool_test.go
• internal/agent/command_prefix_test.go
• internal/tools/shell_runtime.go
• internal/agent/command_prefix.go

[kevincodex1]

LGTM