fix(tools): platform-specific pager suggestions, quote/caret-safe cd detection - #543
Conversation
…detection less/more/most now suggest `type` instead of cat/head/tail on Windows, since those tools don't exist there. Drop `more` from the MSYS sandbox suggestion too, since more is itself blocked as an interactive pager and would send the model straight into that guard next. detectShellCommandIssue also strips double-quoted spans and cmd.exe caret escapes before matching the bash-style `cd /foo` pattern, so a cd-shaped string inside a quoted argument (e.g. a commit message) isn't mistaken for an actual command.
WalkthroughThis PR adds Windows-specific suggestion text for interactive pager commands (less, more, most) detected by the sandbox, and improves Windows shell-command detection by stripping quoted spans and caret-escaped metacharacters before regex matching, updating suggestion wording, and expanding test coverage accordingly. ChangesWindows shell command detection and suggestions
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BashTool
participant ShellRuntime
participant SafeCommand
User->>BashTool: Submit shell command
BashTool->>ShellRuntime: detectShellCommandIssue(command)
ShellRuntime->>ShellRuntime: stripDoubleQuotedSpans(command)
ShellRuntime->>ShellRuntime: stripCmdCaretEscapes(command)
ShellRuntime->>ShellRuntime: match windowsBashStyleCDPattern
ShellRuntime-->>BashTool: issue or nil
BashTool->>SafeCommand: DetectInteractiveCommand(command)
SafeCommand->>SafeCommand: lookup interactiveProgram
SafeCommand->>SafeCommand: select suggestion (windowsSuggestion if goos=windows)
SafeCommand-->>BashTool: InteractiveCommandResult
BashTool-->>User: Block command with platform-specific suggestion
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tools/shell_runtime.go (1)
205-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHelper implementations look correct for the intended ASCII shell-metachar use case.
One nuance:
stripDoubleQuotedSpans's doc comment claims it preserves "the string's length," but for multi-byte runes inside a quoted span it emits a singleWriteByte(' ')per rune, which shrinks the byte length relative to the original (e.g. a 3-byte rune becomes 1 byte). This doesn't break the current regex-matching use case (shell metacharacters are all ASCII), so it's a doc-accuracy nit rather than a functional bug worth blocking on.🤖 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 205 - 241, The helper logic in stripDoubleQuotedSpans is fine, but its doc comment is inaccurate because it says the function preserves the string’s length even though multi-byte runes inside quoted spans are replaced with single-byte spaces, changing byte length. Update the comment to describe preserving alignment/position of unquoted text rather than exact length, and keep the implementation in sync with the documented behavior in stripDoubleQuotedSpans and stripCmdCaretEscapes.
🤖 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 205-241: The helper logic in stripDoubleQuotedSpans is fine, but
its doc comment is inaccurate because it says the function preserves the
string’s length even though multi-byte runes inside quoted spans are replaced
with single-byte spaces, changing byte length. Update the comment to describe
preserving alignment/position of unquoted text rather than exact length, and
keep the implementation in sync with the documented behavior in
stripDoubleQuotedSpans and stripCmdCaretEscapes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: decea9a2-1a1b-4cee-a349-b15a02f1a03d
📒 Files selected for processing (4)
internal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/tools/bash_tool_test.gointernal/tools/shell_runtime.go
…detection (Gitlawb#543) less/more/most now suggest `type` instead of cat/head/tail on Windows, since those tools don't exist there. Drop `more` from the MSYS sandbox suggestion too, since more is itself blocked as an interactive pager and would send the model straight into that guard next. detectShellCommandIssue also strips double-quoted spans and cmd.exe caret escapes before matching the bash-style `cd /foo` pattern, so a cd-shaped string inside a quoted argument (e.g. a commit message) isn't mistaken for an actual command.
Summary
typeinstead of cat/head/tail on Windows, since those tools don't exist there.morefrom the MSYS sandbox suggestion text, sincemoreis itself blocked as an interactive pager and would send the model straight into that guard next.detectShellCommandIssuestrips double-quoted spans and cmd.exe caret escapes before matching the bash-stylecd /foopattern, so a cd-shaped string inside a quoted argument (e.g. a commit message) isn't mistaken for a real command.This is independent of #468: it's branched from main and doesn't touch the quoting-guidance text or the MSYS/POSIX detection logic that #476 already owns.
Test plan
Summary by CodeRabbit