This commit is contained in:
afjcjsbx 2026-03-27 21:54:45 +01:00
parent 8733fa2935
commit ebe70bbb3b
3 changed files with 62 additions and 25 deletions

View file

@ -295,8 +295,8 @@ Even with `restrict_to_workspace: false`, the `exec` tool blocks these dangerous
|------------|------|---------|-------------| |------------|------|---------|-------------|
| `tools.read_file.enabled` | bool | `true` | Enables the `read_file` tool | | `tools.read_file.enabled` | bool | `true` | Enables the `read_file` tool |
| `tools.read_file.mode` | string | `bytes` | Selects the `read_file` implementation: `bytes` or `lines` | | `tools.read_file.mode` | string | `bytes` | Selects the `read_file` implementation: `bytes` or `lines` |
| `tools.read_file.max_read_file_size` | int | `65536` | Maximum bytes returned by `read_file` and byte budget used by `read_file_lines` | | `tools.read_file.max_read_file_size` | int | `65536` | Maximum bytes returned by `read_file` and byte budget used by `read_file` |
| `tools.read_file_lines.enabled` | bool | `false` | Enables the separate line-oriented `read_file_lines` tool | | `tools.read_file.enabled` | bool | `false` | Enables the separate line-oriented `read_file` tool |
#### Mode: `bytes` #### Mode: `bytes`
@ -328,9 +328,8 @@ Behavior notes:
* Binary-looking files are rejected with guidance to use `read_file` * Binary-looking files are rejected with guidance to use `read_file`
* Extremely long single lines are truncated rather than skipped * Extremely long single lines are truncated rather than skipped
* If truncation happens mid-line, the tool explicitly suggests falling back to `read_file` for byte-wise inspection
Use `read_file_lines` when: Use `mode = lines` when:
* The agent mostly reads text files * The agent mostly reads text files
* You want line-based pagination in prompts and tool calls * You want line-based pagination in prompts and tool calls

View file

@ -24,7 +24,11 @@ import (
const MaxReadFileSize = 64 * 1024 // 64KB limit to avoid context overflow const MaxReadFileSize = 64 * 1024 // 64KB limit to avoid context overflow
func validatePathWithAllowPaths(path, workspace string, restrict bool, patterns []*regexp.Regexp) (string, error) { func validatePathWithAllowPaths(
path, workspace string,
restrict bool,
patterns []*regexp.Regexp,
) (string, error) {
if workspace == "" { if workspace == "" {
return path, fmt.Errorf("workspace is not defined") return path, fmt.Errorf("workspace is not defined")
} }
@ -328,7 +332,7 @@ func (t *ReadFileTool) Description() string {
} }
func (t *ReadFileLinesTool) Description() string { func (t *ReadFileLinesTool) Description() string {
return "Read a UTF-8 text file from a specific line range. Uses 1-indexed line offsets and stops when the configured byte budget is reached." return "Read a UTF-8 text file from the filesystem. Output always includes line numbers in the format `LINE_NUMBER|LINE_CONTENT` (1-indexed). Supports partial reads via `start_line` and `max_lines` for large text files."
} }
func (t *ReadFileTool) Parameters() map[string]any { func (t *ReadFileTool) Parameters() map[string]any {
@ -638,18 +642,18 @@ func (t *ReadFileLinesTool) Execute(ctx context.Context, args map[string]any) *T
switch { switch {
case lineTruncated: case lineTruncated:
header += fmt.Sprintf( header += fmt.Sprintf(
"\n[TRUNCATED - line %d exceeded the %d byte read budget and was cut mid-line. Use read_file for byte-wise inspection of the remaining content.]", "\n[TRUNCATED - line %d exceeded the %d byte read budget and was cut mid-line.",
endLine, endLine,
t.maxSize, t.maxSize,
) )
case byteBudgetTruncated: case byteBudgetTruncated:
header += fmt.Sprintf( header += fmt.Sprintf(
"\n[TRUNCATED - byte budget reached. Call read_file_lines again with offset=%d to continue at the next line.]", "\n[TRUNCATED - byte budget reached. Call read_file again with offset=%d to continue at the next line.]",
startLine+linesRead, startLine+linesRead,
) )
case !reachedEOF && limit > 0 && linesRead >= limit: case !reachedEOF && limit > 0 && linesRead >= limit:
header += fmt.Sprintf( header += fmt.Sprintf(
"\n[PARTIAL - more content remains. Call read_file_lines again with offset=%d to continue.]", "\n[PARTIAL - more content remains. Call read_file again with offset=%d to continue.]",
startLine+linesRead, startLine+linesRead,
) )
default: default:
@ -822,7 +826,11 @@ type WriteFileTool struct {
fs fileSystem fs fileSystem
} }
func NewWriteFileTool(workspace string, restrict bool, allowPaths ...[]*regexp.Regexp) *WriteFileTool { func NewWriteFileTool(
workspace string,
restrict bool,
allowPaths ...[]*regexp.Regexp,
) *WriteFileTool {
var patterns []*regexp.Regexp var patterns []*regexp.Regexp
if len(allowPaths) > 0 { if len(allowPaths) > 0 {
patterns = allowPaths[0] patterns = allowPaths[0]
@ -875,7 +883,9 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *ToolR
if !overwrite { if !overwrite {
if _, err := t.fs.Open(path); err == nil { if _, err := t.fs.Open(path); err == nil {
return ErrorResult(fmt.Sprintf("file: %s already exists. Set overwrite=true to replace.", path)) return ErrorResult(
fmt.Sprintf("file: %s already exists. Set overwrite=true to replace.", path),
)
} }
} }

View file

@ -59,8 +59,13 @@ func TestFilesystemTool_ReadFile_NotFound(t *testing.T) {
} }
// Should contain error message // Should contain error message
if !strings.Contains(result.ForLLM, "failed to open file") && !strings.Contains(result.ForUser, "failed to open") { if !strings.Contains(result.ForLLM, "failed to open file") &&
t.Errorf("Expected error message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser) !strings.Contains(result.ForUser, "failed to open") {
t.Errorf(
"Expected error message, got ForLLM: %s, ForUser: %s",
result.ForLLM,
result.ForUser,
)
} }
} }
@ -78,7 +83,8 @@ func TestFilesystemTool_ReadFile_MissingPath(t *testing.T) {
} }
// Should mention required parameter // Should mention required parameter
if !strings.Contains(result.ForLLM, "path is required") && !strings.Contains(result.ForUser, "path is required") { if !strings.Contains(result.ForLLM, "path is required") &&
!strings.Contains(result.ForUser, "path is required") {
t.Errorf("Expected 'path is required' message, got ForLLM: %s", result.ForLLM) t.Errorf("Expected 'path is required' message, got ForLLM: %s", result.ForLLM)
} }
} }
@ -297,7 +303,12 @@ func TestFilesystemTool_WriteFile_OverwriteSandboxed(t *testing.T) {
"content": "replaced in sandbox", "content": "replaced in sandbox",
"overwrite": true, "overwrite": true,
}) })
assert.False(t, result.IsError, "expected success in sandbox mode with overwrite=true, got: %s", result.ForLLM) assert.False(
t,
result.IsError,
"expected success in sandbox mode with overwrite=true, got: %s",
result.ForLLM,
)
data, err := os.ReadFile(filepath.Join(workspace, testFile)) data, err := os.ReadFile(filepath.Join(workspace, testFile))
assert.NoError(t, err) assert.NoError(t, err)
@ -325,7 +336,8 @@ func TestFilesystemTool_ListDir_Success(t *testing.T) {
} }
// Should list files and directories // Should list files and directories
if !strings.Contains(result.ForLLM, "file1.txt") || !strings.Contains(result.ForLLM, "file2.txt") { if !strings.Contains(result.ForLLM, "file1.txt") ||
!strings.Contains(result.ForLLM, "file2.txt") {
t.Errorf("Expected files in listing, got: %s", result.ForLLM) t.Errorf("Expected files in listing, got: %s", result.ForLLM)
} }
if !strings.Contains(result.ForLLM, "subdir") { if !strings.Contains(result.ForLLM, "subdir") {
@ -349,8 +361,13 @@ func TestFilesystemTool_ListDir_NotFound(t *testing.T) {
} }
// Should contain error message // Should contain error message
if !strings.Contains(result.ForLLM, "failed to read") && !strings.Contains(result.ForUser, "failed to read") { if !strings.Contains(result.ForLLM, "failed to read") &&
t.Errorf("Expected error message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser) !strings.Contains(result.ForUser, "failed to read") {
t.Errorf(
"Expected error message, got ForLLM: %s, ForUser: %s",
result.ForLLM,
result.ForUser,
)
} }
} }
@ -397,7 +414,8 @@ func TestFilesystemTool_ReadFile_RejectsSymlinkEscape(t *testing.T) {
// os.Root might return different errors depending on platform/implementation // os.Root might return different errors depending on platform/implementation
// but it definitely should error. // but it definitely should error.
// Our wrapper returns "access denied or file not found" // Our wrapper returns "access denied or file not found"
if !strings.Contains(result.ForLLM, "access denied") && !strings.Contains(result.ForLLM, "file not found") && if !strings.Contains(result.ForLLM, "access denied") &&
!strings.Contains(result.ForLLM, "file not found") &&
!strings.Contains(result.ForLLM, "no such file") { !strings.Contains(result.ForLLM, "no such file") {
t.Fatalf("expected symlink escape error, got: %s", result.ForLLM) t.Fatalf("expected symlink escape error, got: %s", result.ForLLM)
} }
@ -416,10 +434,20 @@ func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) {
}) })
// We EXPECT IsError=true (access blocked due to empty workspace) // We EXPECT IsError=true (access blocked due to empty workspace)
assert.True(t, result.IsError, "Security Regression: Empty workspace allowed access! content: %s", result.ForLLM) assert.True(
t,
result.IsError,
"Security Regression: Empty workspace allowed access! content: %s",
result.ForLLM,
)
// Verify it failed for the right reason // Verify it failed for the right reason
assert.Contains(t, result.ForLLM, "workspace is not defined", "Expected 'workspace is not defined' error") assert.Contains(
t,
result.ForLLM,
"workspace is not defined",
"Expected 'workspace is not defined' error",
)
} }
// TestRootMkdirAll verifies that root.MkdirAll (used by atomicWriteFileInRoot) handles all cases: // TestRootMkdirAll verifies that root.MkdirAll (used by atomicWriteFileInRoot) handles all cases:
@ -653,7 +681,10 @@ func TestWhitelistFs_BlocksSymlinkEscapeInAllowedDir(t *testing.T) {
patterns := []*regexp.Regexp{regexp.MustCompile(`^` + regexp.QuoteMeta(allowedDir))} patterns := []*regexp.Regexp{regexp.MustCompile(`^` + regexp.QuoteMeta(allowedDir))}
tool := NewReadFileTool(workspace, true, MaxReadFileSize, patterns) tool := NewReadFileTool(workspace, true, MaxReadFileSize, patterns)
result := tool.Execute(context.Background(), map[string]any{"path": filepath.Join(linkPath, "secret.txt")}) result := tool.Execute(
context.Background(),
map[string]any{"path": filepath.Join(linkPath, "secret.txt")},
)
if !result.IsError { if !result.IsError {
t.Fatalf("expected symlink escape from allowed dir to be blocked, got: %s", result.ForLLM) t.Fatalf("expected symlink escape from allowed dir to be blocked, got: %s", result.ForLLM)
} }
@ -1012,9 +1043,6 @@ func TestReadFileLinesTool_TruncatesSingleLongLineAtByteBudget(t *testing.T) {
if !strings.Contains(result.ForLLM, "was cut mid-line") { if !strings.Contains(result.ForLLM, "was cut mid-line") {
t.Fatalf("expected explicit mid-line truncation warning, got: %s", result.ForLLM) t.Fatalf("expected explicit mid-line truncation warning, got: %s", result.ForLLM)
} }
if !strings.Contains(result.ForLLM, "Use read_file for byte-wise inspection") {
t.Fatalf("expected byte-tool guidance for long line, got: %s", result.ForLLM)
}
if strings.Contains(result.ForLLM, "second line") { if strings.Contains(result.ForLLM, "second line") {
t.Fatalf("did not expect second line after truncation, got: %s", result.ForLLM) t.Fatalf("did not expect second line after truncation, got: %s", result.ForLLM)
} }