fix(tools): flag piped POSIX utilities and improve Windows quoting guidance#468
fix(tools): flag piped POSIX utilities and improve Windows quoting guidance#468euxaristia wants to merge 15 commits into
Conversation
…SIX flagging with local guidance/quoting improvements)
…ent memory errors - Align detection with upstream landed version while keeping 'cat' and extra safety. - Strengthen shell guidance with explicit double-quote advice for args containing '|' (gh --jq, test -run, etc.). - Update suggestions to mention type for cat and quoting. - Tests pass. This addresses repeated 'Not enough memory resources are available to process this command' when agent used | cat / | head on Windows cmd.exe (and unquoted | in filters).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWindows shell guidance and detection now cover ChangesWindows shell command handling
Clipboard execution and test tolerance fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/tools/shell_runtime.go (2)
66-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMessage text drifts from the regex it describes.
Line 69's
Messagestill enumerateshead/tail/grep/wc/awk/sed/cut/xargs/trbutwindowsPosixUtilityPatternnow also matchescat(line 28), and theSuggestionon line 70 explicitly calls out "typefor cat". An agent hittingcat file.txtwill see a message that doesn't mention the very utility it used, which undercuts the guidance's clarity.✏️ Proposed fix
- Message: "Command uses a POSIX utility (head/tail/grep/wc/awk/sed/cut/xargs/tr) that Windows cmd.exe does not have.", + Message: "Command uses a POSIX utility (head/tail/grep/wc/awk/sed/cut/xargs/tr/cat) that Windows cmd.exe does not have.",🤖 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 66 - 72, The shell syntax warning text is out of sync with windowsPosixUtilityPattern because it omits cat even though the matcher now flags it. Update the shellIssue message in the shell runtime path that builds the windows_shell_syntax warning so it lists cat alongside the other POSIX utilities, and keep the wording aligned with the existing Suggestion that references type for cat.
38-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWindows guidance string duplicated with
workspaceContextin system_prompt.go.This string is nearly identical to the one added at
internal/agent/system_prompt.goLine 200 (metachar quoting rules, avoid POSIX-only commands, prefer cwd/native tools). Maintaining two independently-worded copies of the same guidance risks them drifting out of sync over time (as already evidenced by minor wording differences like "cwd" vs "workdir/cwd").Consider extracting a shared constant/function (e.g. in a common package or via
shellRuntime) that both call sites consume.🤖 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 38 - 51, The Windows shell guidance in shellGuidanceForGOOS is duplicated with the similar guidance in system_prompt.go, which risks the two copies drifting apart. Extract the shared Windows guidance into a single reusable constant or helper (for example alongside detectShellRuntime or in a common package) and have both shellGuidanceForGOOS and workspaceContext consume it. Keep the metachar quoting, POSIX-only command warnings, and cwd/native-tool preferences sourced from that one place so future edits stay consistent.
🤖 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/shell_runtime.go`:
- Around line 66-72: The shell syntax warning text is out of sync with
windowsPosixUtilityPattern because it omits cat even though the matcher now
flags it. Update the shellIssue message in the shell runtime path that builds
the windows_shell_syntax warning so it lists cat alongside the other POSIX
utilities, and keep the wording aligned with the existing Suggestion that
references type for cat.
- Around line 38-51: The Windows shell guidance in shellGuidanceForGOOS is
duplicated with the similar guidance in system_prompt.go, which risks the two
copies drifting apart. Extract the shared Windows guidance into a single
reusable constant or helper (for example alongside detectShellRuntime or in a
common package) and have both shellGuidanceForGOOS and workspaceContext consume
it. Keep the metachar quoting, POSIX-only command warnings, and cwd/native-tool
preferences sourced from that one place so future edits stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df8f365d-1aaf-4fc3-bdf5-2a0bf31ab9a6
📒 Files selected for processing (5)
internal/agent/system_prompt.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/tools/bash_tool_test.gointernal/tools/shell_runtime.go
…rors, clipboard safety, and bash output cap
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tools/bash.go (1)
129-153: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftCap stdout/stderr during capture
bytes.Bufferstill grows without bound whilecommand.Run()is executing, so the post-runTruncatecalls don’t prevent an OOM on a large producer. Use a capped writer (or other streaming sink that stops accepting bytes pastmaxBashOutputBytes) forcommand.Stdout/command.Stderrinstead of truncating afterward.🤖 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 129 - 153, The current stdout/stderr handling in the bash execution path still allows unbounded growth because command.Stdout and command.Stderr are wired directly to bytes.Buffer and only truncated after command.Run() completes. Update the Run/command setup in bash.go to use a capped writer or other streaming sink for stdout and stderr capture so output stops accumulating once maxBashOutputBytes is reached, while keeping the existing exitCode, appendSandboxBlocks, and stderr/stdout collection flow intact.
🧹 Nitpick comments (1)
internal/imageinput/clipboard.go (1)
78-92: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePath values are interpolated into the PowerShell script via raw string concatenation.
tmpPathis spliced directly into a single-quoted PowerShell string literal (Line 88).os.CreateTempoutput normally won't contain a single quote, but this is fragile if the OS temp dir ever contains one (e.g. certain corporate-managed profiles/usernames with apostrophes). Consider escaping embedded single quotes ('') before interpolation for defense in depth.Escape single quotes defensively
- script := `Add-Type -AssemblyName System.Windows.Forms; Add-Type -AssemblyName System.Drawing; $img = [System.Windows.Forms.Clipboard]::GetImage(); if ($img -ne $null) { $img.Save('` + tmpPath + `', [System.Drawing.Imaging.ImageFormat]::Png) }` + escapedTmpPath := strings.ReplaceAll(tmpPath, "'", "''") + script := `Add-Type -AssemblyName System.Windows.Forms; Add-Type -AssemblyName System.Drawing; $img = [System.Windows.Forms.Clipboard]::GetImage(); if ($img -ne $null) { $img.Save('` + escapedTmpPath + `', [System.Drawing.Imaging.ImageFormat]::Png) }`🤖 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/imageinput/clipboard.go` around lines 78 - 92, The PowerShell script built in clipboard.go’s temp-script flow interpolates tmpPath directly into a single-quoted string, which is fragile if the path contains apostrophes. Update the script construction to defensively escape any single quotes in tmpPath before concatenation, and keep the fix localized to the script assembly around os.CreateTemp, os.WriteFile, and the exec.Command("powershell", "-File", scriptPath) flow.
🤖 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/acp/agent.go`:
- Around line 390-409: The warning paths in Agent.persistTurn currently write to
stdout, which can corrupt ACP JSON-RPC output. Update the failure logging in
persistTurn to send the messages to stderr instead, using fmt.Fprintf(os.Stderr,
...) or the existing logger, and keep the user/assistant persistence branches
unchanged apart from the output destination.
In `@internal/tools/bash.go`:
- Around line 146-153: The Bash output truncation in the command execution flow
is silent, so update the truncation path in internal/tools/bash.go to track
whether stdout and stderr were cut off. Thread those truncation flags through
formatBashOutput and formatBashOutputWithShellHint, and append a clear
truncation notice like “[zero] output truncated at N bytes” to the returned
result when maxBashOutputBytes is exceeded.
---
Outside diff comments:
In `@internal/tools/bash.go`:
- Around line 129-153: The current stdout/stderr handling in the bash execution
path still allows unbounded growth because command.Stdout and command.Stderr are
wired directly to bytes.Buffer and only truncated after command.Run() completes.
Update the Run/command setup in bash.go to use a capped writer or other
streaming sink for stdout and stderr capture so output stops accumulating once
maxBashOutputBytes is reached, while keeping the existing exitCode,
appendSandboxBlocks, and stderr/stdout collection flow intact.
---
Nitpick comments:
In `@internal/imageinput/clipboard.go`:
- Around line 78-92: The PowerShell script built in clipboard.go’s temp-script
flow interpolates tmpPath directly into a single-quoted string, which is fragile
if the path contains apostrophes. Update the script construction to defensively
escape any single quotes in tmpPath before concatenation, and keep the fix
localized to the script assembly around os.CreateTemp, os.WriteFile, and the
exec.Command("powershell", "-File", scriptPath) flow.
🪄 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: 78a31a77-1b9d-49cb-96d7-4d99e504cf76
📒 Files selected for processing (4)
internal/acp/agent.gointernal/imageinput/clipboard.gointernal/specialist/exec.gointernal/tools/bash.go
- Add cat to windowsPosixUtilityPattern message in shell_runtime.go - Switch persistTurn warnings to stderr in acp/agent.go - Escape tmpPath in clipboard PowerShell script - Implement cappedWriter for bash output to prevent OOM during run, add truncation notice Addresses unaddressed items from CodeRabbit reviews.
|
Addressed the remaining CodeRabbit review comments from #468:
Addresses unaddressed items from CodeRabbit reviews. |
…ctsDeadlineUnderContinuousOutput
The system prompt now documents double-quote quoting for cmd.exe metachars instead of the older cwd argument phrasing. Update TestBuildSystemPromptInjectsHostShellContext so Windows CI matches the injected text.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Request changes — real, valuable work, but a Windows regression + grab-bag scope
Worth merging: yes, after a split and one revert — this is not slop. The core (Windows shell-quoting guidance, the POSIX cat flag, interactive-command suggestions) is a correct additive refinement on top of merged #412 — I confirmed #412 is an ancestor of your base, so you build on it rather than re-doing it — and the OOM cappedWriter, the specialist depth-cap (--depth propagation), and the stderr logging fix are each individually sound. Thanks @euxaristia.
Two things block it:
🔴 Hard blocker — the clipboard change breaks image paste on default Windows. clipboard.go switches PowerShell from -Command to -File with no -ExecutionPolicy Bypass. -Command is not gated by script-file execution policy; -File is, and the Windows client default is Restricted — so on an unconfigured machine, image paste silently stops working. Two more regressions ride in the same switch: the BOM-less UTF-8 .ps1 is decoded as ANSI by Windows PowerShell 5.1, so a non-ASCII %TEMP% path (accented username) mis-resolves; and writing-then-executing a temp script adds a TOCTOU window the inline form doesn't have. Fix: revert to -Command, keeping only the single-quote escaping — that already fixes the bug you were targeting, with none of the downside.
🟡 Scope — split the grab-bag. This bundles shell guidance + OOM cap + specialist depth + persistTurn logging + clipboard under a "Windows shell guidance" title. Each is fine on its own, but they need separate issue-linked PRs for reviewability and to avoid re-churning files #412 just touched (CodeRabbit's out-of-scope warning flagged the same).
One rebase note: once #476 lands (it expands the same windowsPosixUtilityPattern and already adds cat), your cat addition becomes redundant — drop it, keeping the quoting-guidance rewrite + interactive suggestions as the unique value. Credit where due: the additive shell slice is a legitimate improvement; just unbundle it and revert the clipboard call.
Review feedback: -File is gated by script execution policy (default Restricted on Windows clients), Windows PowerShell 5.1 decodes BOM-less UTF-8 .ps1 files as ANSI so non-ASCII temp paths mis-resolve, and the write-then-execute temp script adds a TOCTOU window. Revert to the inline -Command form and keep only the single-quote escaping, which is the actual quoting fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Vasanthdev2004 asked for this PR to be split since it bundled several independent static-analysis fixes. Remove the specialist depth cap, bash output OOM cap, and persistence error logging: each now has its own PR (Gitlawb#491, Gitlawb#492, Gitlawb#493). Also drop the "cat" addition to the Windows POSIX-utility detection in shell_runtime.go. PR Gitlawb#476 already covers cat detection comprehensively under the MSYS/sandbox angle, so keeping it here would duplicate that work. What remains: the Windows cmd.exe quoting-guidance rewrite, the Windows-specific interactive-command suggestions (steering away from Unix head/tail/ps toward native or PowerShell alternatives), the clipboard PowerShell -Command fix, and the CI test-slack change in exec_command_test.go. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a few issues that need to be addressed before this is ready.
Findings
-
[P2] Keep
| catcovered before closing #467
internal/tools/shell_runtime.go:28
The PR description says this closes #467, but the current Windows preflight regex still does not includecat. That means the exact issue examplegh ... 2>&1 | catis not blocked before execution, while only cases that also contain another listed utility such as| headare caught. Please either keepcatin this PR's detector/tests, or stop this PR from closing #467 until thecatcoverage has landed elsewhere and the issue-closing wording is updated. -
[P2] Do not suggest another follow-forever command for
tail -f
internal/sandbox/safe_command.go:641
The Windows replacement for a blockedtail -fisGet-Content -Path file -Tail N -Wait, but-Waitfollows the file indefinitely too. The detector is supposed to steer the agent away from commands that hang a non-interactive run, so this can send it straight into the same class of problem with a Windows-native command. Please make the primary suggestion bounded, such asGet-Content -Tail Nwithout-Wait, and tighten the test so it rejects follow/-Waitsuggestions. -
[P2] Coordinate or drop the duplicate #413 fix from this PR
internal/sandbox/safe_command.go:618
The Windows interactive-suggestion slice in this PR overlaps the older open PR #414, which already targets #413 and changes the samesafe_command.go,safe_command_test.go, andbash_tool_test.gopaths. #414 has already been through review for this exact behavior, including the Windows sandbox test fallout, while this PR neither links #413 nor explains why it should replace that earlier PR. Please either coordinate this slice with #414 and make this PR the chosen replacement, or drop the interactive-suggestion changes here so the review queue does not carry two competing implementations for the same issue. -
[P3] Do not auto-close #473 from this PR
PR description
The body saysCloses #473, but the current patch does not replace, augment, simplify, or materially test the hand-written parser risk described in that issue; it only adds Windows-specific suggestion text on top of the existing parser. Merging as-is would close the parser-risk issue even though the risk remains intentionally deferred. Please remove #473 from the closing keywords or include the parser-risk fix here.
…ion slice Addresses jatmn's review on PR Gitlawb#468: - Add `cat` back to windowsPosixUtilityPattern so the exact issue Gitlawb#467 reproduction (`... 2>&1 | cat`) is flagged even without another POSIX utility in the pipeline. - Revert the Windows-specific interactive-command suggestion overrides in safe_command.go (less/more/top/tail -f, etc.): they duplicated the already-open PR Gitlawb#414, which covers the same ground without the tail -f `-Wait` bug this PR introduced.
There was a problem hiding this comment.
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/bash_tool_test.go`:
- Around line 540-548: The pager-block guidance is hardcoded to Unix-style
commands, so update the pager guard in safe_command.go to be OS-aware instead of
always suggesting cat. Adjust the logic that emits the interactive/pager-block
message for less/more/most so it branches on GOOS (or equivalent platform
detection) and uses Windows-appropriate guidance such as type/more when running
under cmd.exe. Keep the existing pager detection behavior in the command
handling path, but make the suggestion text come from the platform-specific
branch.
🪄 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: 446698ac-8c66-45b5-bd85-f2c6b988a1f1
📒 Files selected for processing (2)
internal/tools/bash_tool_test.gointernal/tools/shell_runtime.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tools/shell_runtime.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/bash_tool_test.go`:
- Around line 540-548: The pager-block guidance is hardcoded to Unix-style
commands, so update the pager guard in safe_command.go to be OS-aware instead of
always suggesting cat. Adjust the logic that emits the interactive/pager-block
message for less/more/most so it branches on GOOS (or equivalent platform
detection) and uses Windows-appropriate guidance such as type/more when running
under cmd.exe. Keep the existing pager detection behavior in the command
handling path, but make the suggestion text come from the platform-specific
branch.
🪄 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: 446698ac-8c66-45b5-bd85-f2c6b988a1f1
📒 Files selected for processing (2)
internal/tools/bash_tool_test.gointernal/tools/shell_runtime.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tools/shell_runtime.go
🛑 Comments failed to post (1)
internal/tools/bash_tool_test.go (1)
540-548: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Locate where the "interactive" + "cat" suggestion message originates rg -nP '"interactive"' --type=go -C5 rg -nP 'less|more|pager' --type=go -g '*sandbox*' -C5Repository: Gitlawb/zero
Length of output: 150
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant test file and nearby production code. git ls-files 'internal/tools/*' | sed -n '1,200p' ast-grep outline internal/tools/bash_tool_test.go --view expanded || true # Find pager / interactive guard messages and any OS-specific branching. rg -n --hidden --glob '!**/vendor/**' --glob '!**/node_modules/**' \ 'interactive|pager|cat suggestion|type suggestion|cmd\.exe|windows|runtime\.GOOS|GOOS' \ internal/tools -C 3 # Find the exact test expectation and the implementation that emits it. rg -n --hidden --glob '!**/vendor/**' --glob '!**/node_modules/**' \ 'strings\.Contains\(result\.Output, "interactive"\)|strings\.Contains\(result\.Output, "cat"\)|less /etc/hosts' \ internal/tools -C 3Repository: Gitlawb/zero
Length of output: 40378
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the interactive-command detector and the pager-block message source. rg -n --hidden --glob '!**/vendor/**' --glob '!**/node_modules/**' \ 'DetectInteractiveCommand|interactiveBlockResult|cat suggestion|pager|less|more|type for cat|findstr|Windows cmd\.exe|interactive command' \ . -C 4 # Show the implementation of the interactive-command detector and its tests, if present. fd -a 'interactive' . || true rg -n --hidden --glob '!**/vendor/**' --glob '!**/node_modules/**' \ 'func .*DetectInteractiveCommand|type .*InteractiveCommandResult|Suggestion:' \ . -C 3Repository: Gitlawb/zero
Length of output: 50369
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the zeroSandbox interactive-command detector and any OS-specific suggestion text. rg -n --hidden --glob '!**/vendor/**' --glob '!**/node_modules/**' \ 'DetectInteractiveCommand|InteractiveCommandResult|cat|type|more|less|pager|interactive' \ internal zerocommands sandbox -C 3 # If the detector lives outside these paths, locate its definition by filename hints. fd -a 'DetectInteractiveCommand|interactive' internal zerocommands internal sandbox . || trueRepository: Gitlawb/zero
Length of output: 50369
🏁 Script executed:
#!/bin/bash set -euo pipefail # Narrow to the exact detector implementation and its tests. rg -n --hidden --glob '!**/vendor/**' --glob '!**/node_modules/**' \ 'func .*DetectInteractiveCommand|type InteractiveCommandResult|Suggestion:|Reason:' \ . -g '*.go' -C 4 | sed -n '1,220p'Repository: Gitlawb/zero
Length of output: 15731
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find the actual definition of the interactive detector and print the relevant slice. grep -RIn --include='*.go' 'func DetectInteractiveCommand\|type InteractiveCommandResult' . | sed -n '1,80p' # Once found, print the surrounding lines from the defining file(s). python3 - <<'PY' import os, re, glob, pathlib for path in glob.glob('**/*.go', recursive=True): try: txt = pathlib.Path(path).read_text() except Exception: continue if 'func DetectInteractiveCommand' in txt or 'type InteractiveCommandResult' in txt: print(path) PYRepository: Gitlawb/zero
Length of output: 385
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the zerocommands package, then inspect files likely to contain interactive detection. git ls-files 'internal/zerocommands/*.go' | sed -n '1,120p' ast-grep outline internal/zerocommands --view expanded || trueRepository: Gitlawb/zero
Length of output: 19352
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search only the package most likely to own DetectInteractiveCommand. rg -n --hidden --glob '!**/vendor/**' --glob '!**/node_modules/**' \ 'DetectInteractiveCommand|InteractiveCommandResult|interactive|less|more|cat|type' \ internal/zerocommands -C 4Repository: Gitlawb/zero
Length of output: 50368
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the interactive detector implementation in the repository and show just that file. fd -a 'interactive' internal/zerocommands internal tools . | sed -n '1,120p' || true rg -n --hidden --glob '!**/vendor/**' --glob '!**/node_modules/**' \ 'func DetectInteractiveCommand|type InteractiveCommandResult|interactive command|pager|cat suggestion|Windows' \ internal/zerocommands internal/tools -C 3Repository: Gitlawb/zero
Length of output: 18872
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the detector implementation and the tests that cover its suggestion text. sed -n '1,240p' internal/sandbox/safe_command.go printf '\n---- tests ----\n' rg -n --hidden --glob '!**/vendor/**' --glob '!**/node_modules/**' \ 'DetectInteractiveCommand|interactive|cat|type|more|less' internal/sandbox -C 3Repository: Gitlawb/zero
Length of output: 50369
Make the pager-block suggestion OS-aware
internal/sandbox/safe_command.gohardcodescatfor pager blocks (less/more/most). On Windows, this should switch totype/more(or branch onGOOS) so the guidance matchescmd.exe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/bash_tool_test.go` around lines 540 - 548, The pager-block guidance is hardcoded to Unix-style commands, so update the pager guard in safe_command.go to be OS-aware instead of always suggesting cat. Adjust the logic that emits the interactive/pager-block message for less/more/most so it branches on GOOS (or equivalent platform detection) and uses Windows-appropriate guidance such as type/more when running under cmd.exe. Keep the existing pager detection behavior in the command handling path, but make the suggestion text come from the platform-specific branch.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
A Task specialist can recursively spawn further specialists via --depth. Without a ceiling, a runaway or malicious chain of Task calls can recurse indefinitely, exhausting process and memory resources. Reject any run whose depth exceeds a hard cap of 8. Related to Gitlawb#470, not closing it: today's normal specialist/swarm path cannot reach this recursively, since child sessions are launched with --tag specialist and shouldRegisterExecSpecialistTools suppresses the Task tool for that tag (see PR description for details). This is a defense-in-depth guard, not a fix for an active exploit. Split out of Gitlawb#468 at reviewer request. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The interactive-command guard's suggestion for a blocked less/more/most was "use cat, head, or tail -n N", none of which are native cmd.exe commands, so a model blocked on Windows got advice it couldn't follow. Add windowsSuggestion to interactiveProgram so Windows gets its own guidance (type, or the read_file tool with offset/limit) instead. Fix the existing test that asserted "cat" unconditionally, and add coverage that the linux and windows suggestions differ and stay POSIX-tool-free on Windows.
|
Status update on the open findings: Already fixed before this push:
Fixed in 6e62c9f (this push): CodeRabbit's remaining finding, that the pager guard's suggestion for a blocked less/more/most ("use cat, head, or tail -n N") names commands that don't exist on cmd.exe. Added a windowsSuggestion override so Windows gets "use type, or read_file with offset/limit" instead. Fixed the existing test that hardcoded the cat suggestion regardless of OS, and added dedicated coverage that the two platforms' suggestions differ. go build/vet clean on linux and windows targets, go test ./internal/sandbox/... ./internal/tools/... ./internal/agent/... green. Ready for re-review. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sandbox/safe_command.go (1)
46-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting duplicated pager suggestion strings.
less,more, andmostshare byte-identicalsuggestion/windowsSuggestiontext. Pulling these into named constants would avoid future drift if only one entry is updated later.♻️ Optional refactor
+const ( + pagerSuggestion = "Use `cat`, `head`, or `tail -n N` to print file contents non-interactively." + pagerWindowsSuggestion = "Use `type` to print file contents non-interactively, or the read_file tool with offset/limit for a partial view." +) + var interactivePrograms = map[string]interactiveProgram{ ... "less": { reason: "less is a pager that waits for navigation keys", - suggestion: "Use `cat`, `head`, or `tail -n N` to print file contents non-interactively.", - windowsSuggestion: "Use `type` to print file contents non-interactively, or the read_file tool with offset/limit for a partial view.", + suggestion: pagerSuggestion, + windowsSuggestion: pagerWindowsSuggestion, }, "more": { reason: "more is a pager that waits for navigation keys", - suggestion: "Use `cat`, `head`, or `tail -n N` to print file contents non-interactively.", - windowsSuggestion: "Use `type` to print file contents non-interactively, or the read_file tool with offset/limit for a partial view.", + suggestion: pagerSuggestion, + windowsSuggestion: pagerWindowsSuggestion, }, "most": { reason: "most is a pager that waits for navigation keys", - suggestion: "Use `cat`, `head`, or `tail -n N` to print file contents non-interactively.", - windowsSuggestion: "Use `type` to print file contents non-interactively, or the read_file tool with offset/limit for a partial view.", + suggestion: pagerSuggestion, + windowsSuggestion: pagerWindowsSuggestion, },🤖 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/sandbox/safe_command.go` around lines 46 - 60, The pager entries for less, more, and most in safe_command.go duplicate the same suggestion text, so extract the shared suggestion and windowsSuggestion strings into named constants and reuse them in the command map. Keep the existing behavior in the safe command definitions, but reference those constants from the pager-related entries so future edits stay in sync.
🤖 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/sandbox/safe_command.go`:
- Around line 46-60: The pager entries for less, more, and most in
safe_command.go duplicate the same suggestion text, so extract the shared
suggestion and windowsSuggestion strings into named constants and reuse them in
the command map. Keep the existing behavior in the safe command definitions, but
reference those constants from the pager-related entries so future edits stay in
sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80ea73f3-f6d1-4596-a568-522083c9135a
📒 Files selected for processing (3)
internal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/tools/bash_tool_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tools/bash_tool_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
-
[P2] Do not flag quoted
| cattext as an executable pipeline
internal/tools/shell_runtime.go:29
The new Windows POSIX-utility guard runs this regex over the raw command string beforebash/exec_commandexecutes, so it also matches utility names that appear inside double-quoted argument values. For example, a valid Windows command such asgh pr comment --body "please do not use | cat here"is blocked because the quoted body contains| cat, even thoughcmd.exewould treat that pipe as literal text. This regresses the same review/comment workflows the PR is trying to improve whenever the comment body mentions the forbidden pipeline. Please make this detection segment/quote-aware, or otherwise only match actual command positions, and add a regression test for a quoted argument containing| cat/| grep. -
[P2] Do not suggest
morewhen the next guard blocks it
internal/tools/shell_runtime.go:71
The Windows POSIX-utility block tells the model to use cmd.exe equivalents includingmoreto page output, but both shell tools immediately runDetectInteractiveCommandafter this check, andsafe_command.goclassifiesmoreas an interactive pager on Windows. A retry such assome-cmd | moretherefore gets blocked by the interactive-command guard with a different suggestion (typeorread_file), so the remediation can lead directly into another preflight failure. Please removemorefrom this suggestion or make the two guards agree on which Windows paging alternatives are actually allowed.
…ocked more suggestion detectShellCommandIssue matched its cd/ls/POSIX-utility regexes against the raw command string, so an operator or utility name appearing only inside a double-quoted argument value (e.g. `gh pr comment --body "please do not use | cat here"`) was mistaken for an actual pipeline cmd.exe would interpret. Add stripDoubleQuotedSpans, which blanks quoted spans (quotes included) before matching, so only text cmd.exe would actually treat as command syntax can trigger a match. Also drop `more` from the POSIX-utility suggestion: it's itself blocked as an interactive pager on Windows (safe_command.go), so recommending it sent the model straight into that guard's own suggestion instead.
|
Addressed both new findings from the latest review:
go build/vet clean on linux and windows targets, go test ./internal/tools/... ./internal/agent/... green. @jatmn ready for another look. |
There was a problem hiding this comment.
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)
54-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle cmd caret escapes before this heuristic.
foo^|catis literal text in cmd.exe, butwindowsPosixUtilityPatternstill matches|catand raises a false positive. Strip cmd escape sequences in the same pre-pass, or accept the over-flagging here.🤖 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 54 - 109, The Windows shell heuristic in detectShellCommandIssue is still flagging cmd.exe caret-escaped literals like foo^|cat as POSIX utilities because only double-quoted spans are stripped today. Update the pre-processing in detectShellCommandIssue by extending stripDoubleQuotedSpans or adding a similar normalization step to remove/neutralize cmd caret escape sequences before running windowsPosixUtilityPattern and the other Windows regex checks, so escaped operators and commands are not mistaken for real shell syntax.
🤖 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.
Outside diff comments:
In `@internal/tools/shell_runtime.go`:
- Around line 54-109: The Windows shell heuristic in detectShellCommandIssue is
still flagging cmd.exe caret-escaped literals like foo^|cat as POSIX utilities
because only double-quoted spans are stripped today. Update the pre-processing
in detectShellCommandIssue by extending stripDoubleQuotedSpans or adding a
similar normalization step to remove/neutralize cmd caret escape sequences
before running windowsPosixUtilityPattern and the other Windows regex checks, so
escaped operators and commands are not mistaken for real shell syntax.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b43c383-c976-4d8b-9415-787126422e2d
📒 Files selected for processing (2)
internal/tools/bash_tool_test.gointernal/tools/shell_runtime.go
foo^|cat is a literal token in cmd.exe (^ escapes the pipe), but detectShellCommandIssue only stripped double-quoted spans before matching, so the escaped | still read as a real segment boundary and falsely flagged cat as a piped-to POSIX utility. Add stripCmdCaretEscapes, run after quote-stripping, to blank out caret escapes of cmd.exe's own metacharacters (^&, ^|, ^;, ^^, ^<, ^>) before the Windows command-issue regexes run.
|
Addressed: cmd.exe caret escapes (^&, ^|, ^;, ^^, ^<, ^>) are now neutralized in the same pre-pass as the double-quote stripping, so foo^|cat reads as the literal token it is in cmd.exe instead of a pipe into cat. Added regression coverage, including confirming an actual unescaped pipe is still flagged. go build/vet/tests green on linux and windows targets. |
* fix(specialist): cap max specialist nesting depth A Task specialist can recursively spawn further specialists via --depth. Without a ceiling, a runaway or malicious chain of Task calls can recurse indefinitely, exhausting process and memory resources. Reject any run whose depth exceeds a hard cap of 8. Related to #470, not closing it: today's normal specialist/swarm path cannot reach this recursively, since child sessions are launched with --tag specialist and shouldRegisterExecSpecialistTools suppresses the Task tool for that tag (see PR description for details). This is a defense-in-depth guard, not a fix for an active exploit. Split out of #468 at reviewer request. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(specialist): reject depth cap at the boundary, not past it Addresses jatmn's review on PR #491: - Executor.Run checked `CurrentDepth > maxSpecialistDepth`, but this call is what launches the child at CurrentDepth+1. A parent already AT the cap passed the check and started a child one level past it, only getting rejected on that child's own next call. Change to `>=` so the guard fires before the over-cap child is launched. - Add TestRunRejectsDepthAtMax to cover the boundary explicitly. * test(specialist): cover resume boundary in depth-cap guard The pre-branch depth guard in Run sits before the fresh/resume split, but the boundary test only exercised the fresh path, leaving a future refactor free to move the guard without any test catching a resume regression. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that need to be addressed before this is ready.
Findings
-
[P1] Rebase on main and resolve the merge conflicts first
GitHub mergeability
GitHub currently reportsmergeable=CONFLICTINGwithmergeStateStatus=DIRTY, so this branch cannot be merged as-is. Please rebase on the latestmainbranch and resolve the conflicts before addressing any remaining review items, so the final fixes apply to the code that will actually be merged. -
[P2] Keep flagging POSIX utilities when cmd redirection/operators follow the name
internal/tools/shell_runtime.go:29
The new Windows POSIX-utility regex only matchescat/head/grep/etc. when the utility name is followed by whitespace or the end of the command.cmd.exealso accepts operators and redirections immediately after a command name, so an actual pipeline such assome-cmd|cat>out.txtorsome-cmd|head&&dirdoes not match this preflight even though it still invokes the POSIX utility that this PR is trying to block. Please treat cmd metacharacters such as|,&,<, and>as valid command-name boundaries here, and add regression coverage for a piped utility followed immediately by redirection or another operator.
…tion windowsPosixUtilityPattern only matched a POSIX utility name when it was followed by whitespace or end of string, so a pipeline like some-cmd|cat>out.txt or some-cmd|head&&dir slipped past the Windows preflight check even though cmd.exe still runs the utility. Treat |, &, <, and > as valid boundaries too, and add regression coverage for a piped utility immediately followed by a redirection or another operator.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that need to be addressed before this is ready.
Findings
-
[P1] Rebase on main and resolve the merge conflicts first
GitHub mergeability
GitHub currently reportsmergeable=CONFLICTINGwithmergeStateStatus=DIRTY, so this branch cannot be merged as-is. Please rebase on the latestmainbranch and resolve the conflicts before addressing any remaining review items, so the final fixes apply to the code that will actually be merged. -
[P2] Resolve the remaining #414/#413 overlap or update the PR body
internal/sandbox/safe_command.go:46
The PR body says the Windows-specific interactive-command suggestions forless/more/top/tail -fwere dropped because they duplicated the already-open #414, but the current patch still addswindowsSuggestionbehavior for the pager entries (less,more, andmost) and addsTestDetectInteractiveCommandPagerSuggestionIsPlatformSpecificfor that same interactive-command path. That leaves the earlier duplicate-scope request only partially resolved and makes the description misleading for reviewers trying to decide whether #414 or this PR owns #413's pager fix. Please either drop this remainingsafe_command.goslice from this PR, or explicitly coordinate/describe that this PR is taking the pager subset from #414 so the overlapping PRs and issue-closing path are unambiguous.
# Conflicts: # internal/tools/bash_tool_test.go
Addresses jatmn's latest review on PR Gitlawb#468 (after merging main to resolve mergeable=CONFLICTING): A later commit reintroduced windowsSuggestion overrides for the less/more/most pager entries in safe_command.go, contradicting this PR's own body, which says that slice was dropped because it duplicates the already-open PR Gitlawb#414. Drop it again along with its test coverage, and revert the bash_tool_test.go assertion this reintroduction had changed back to the unconditional cat suggestion check.
|
@Vasanthdev2004 Same as on #475, I can't re-request your review directly. Your findings here (the |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The rebase and the #414 pager slice are both sorted now — thanks. But holding here, because this collides with #476, which is already approved, and I don't think both can land as-is.
Both PRs rework the same Windows detection in shell_runtime.go, in incompatible ways:
- #476 removes
windowsPosixUtilityPatternandwindowsLSCommandPatternentirely and replaces them with segment/word-anchored detection (windowsCommandSegments+firstCommandWord+ a singlewindowsMsysProneNamesset) — that's what avoids the quoted-argument false positives. - This PR keeps and extends
windowsPosixUtilityPattern(addscat+ operator/redirection boundaries) and addsstripDoubleQuotedSpans+stripCmdCaretEscapesto solve that same quoted-arg problem a different way.
They can't coexist. Once #476 merges, this branch still references windowsLSCommandPattern, which #476 deletes, so it won't compile until it's reworked — and even after a rebase the two are duplicate, competing solutions to the same detection. It's also why CI is green here but would break against #476.
The parts of this PR that don't overlap #476 are worth keeping: the Windows quoting guidance in system_prompt.go (wrap metachar args in double quotes, single quotes don't protect in cmd.exe) and the clipboard escaping fix are both genuinely useful and unique to this branch.
Suggestion: let #476 own the shell_runtime.go POSIX/MSYS detection, and narrow this PR down to the quoting guidance + clipboard fix, rebased after #476 lands. If you intended these two to be sequential and I'm missing the ordering, tell me and I'll re-look — but as the branches stand they're on a collision course nothing's flagged yet since neither is merged.
(Issue side is fine — #467/#472 are real; I'll mark them issue-approved once the scope here settles so the gate isn't the thing holding it.)
3 participants
Closes #467 #472
Windows shell quoting guidance and a clipboard escaping fix.
Changes
shell_runtime.go) to explain double-quote escaping for metacharacters in cmd.exe, and to flag piped POSIX utilities (cat/head/tail/grep/wc/awk/sed/cut/xargs/tr) that cmd.exe lacks.-Commandscript (clipboard.go) to escape embedded single quotes in the temp file path instead of concatenating it unescaped. Closes clipboard: risky string concatenation into PowerShell -Command #472.safe_command.gohand-written shell parsing risk (sandbox: complex hand-written shell parsing in safe_command risks bypasses #473); a deeper refactor is left for follow-up, per reviewer discussion. Not closed by this PR since the risk remains.TestCollectRespectsDeadlineUnderContinuousOutputfor Windows CI jitter.Split from this PR
Per reviewer request (Vasanthdev2004), the bash output OOM cap, specialist depth cap, and persistence error logging that were originally bundled here have been split into their own PRs, since they are independent fixes:
This PR no longer touches
internal/tools/bash.go's output buffering,internal/specialist/exec.go's depth check, orinternal/acp/agent.go's persistence logging.Per jatmn's review,
catis back in the Windows POSIX-utility pattern inshell_runtime.go(issue #467's exact reported case is a bare| cat, which needs coverage here regardless of #476's broader MSYS angle), and the Windows-specific interactive-command suggestions (less/more/top/tail -falternatives insafe_command.go) have been dropped from this PR since they duplicated the already-open #414.Rebased onto latest
mainto resolve a merge conflict (internal/tools/bash_tool_test.go'secho-argargument-count check; keptmain's more lenient< 4version since it's a superset of this branch's check and both pass this PR's own tests). ThewindowsSuggestionpager overrides insafe_command.go(less/more/most) were reintroduced by a later commit; dropped them again for the same #414-duplication reason.See individual issues for details and full analysis.
Summary by CodeRabbit
Bug Fixes
more).Tests
less,more,most) and updated related Windows prompt assertions.