diff --git a/docs/configuration.md b/docs/configuration.md index a771f8174..75fde0074 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -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.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_lines.enabled` | bool | `false` | Enables the separate line-oriented `read_file_lines` tool | +| `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.enabled` | bool | `false` | Enables the separate line-oriented `read_file` tool | #### Mode: `bytes` @@ -328,9 +328,8 @@ Behavior notes: * Binary-looking files are rejected with guidance to use `read_file` * 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 * You want line-based pagination in prompts and tool calls diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index cb5921f4b..d40c44c8f 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -24,7 +24,11 @@ import ( 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 == "" { return path, fmt.Errorf("workspace is not defined") } @@ -328,7 +332,7 @@ func (t *ReadFileTool) 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 { @@ -638,18 +642,18 @@ func (t *ReadFileLinesTool) Execute(ctx context.Context, args map[string]any) *T switch { case lineTruncated: 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, t.maxSize, ) case byteBudgetTruncated: 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, ) case !reachedEOF && limit > 0 && linesRead >= limit: 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, ) default: @@ -822,7 +826,11 @@ type WriteFileTool struct { 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 if len(allowPaths) > 0 { patterns = allowPaths[0] @@ -875,7 +883,9 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *ToolR if !overwrite { 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), + ) } } diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 289b44c7c..3290ce1d8 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -59,8 +59,13 @@ func TestFilesystemTool_ReadFile_NotFound(t *testing.T) { } // Should contain error message - if !strings.Contains(result.ForLLM, "failed to open file") && !strings.Contains(result.ForUser, "failed to open") { - t.Errorf("Expected error message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser) + if !strings.Contains(result.ForLLM, "failed to open file") && + !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 - 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) } } @@ -297,7 +303,12 @@ func TestFilesystemTool_WriteFile_OverwriteSandboxed(t *testing.T) { "content": "replaced in sandbox", "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)) assert.NoError(t, err) @@ -325,7 +336,8 @@ func TestFilesystemTool_ListDir_Success(t *testing.T) { } // 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) } if !strings.Contains(result.ForLLM, "subdir") { @@ -349,8 +361,13 @@ func TestFilesystemTool_ListDir_NotFound(t *testing.T) { } // Should contain error message - if !strings.Contains(result.ForLLM, "failed to read") && !strings.Contains(result.ForUser, "failed to read") { - t.Errorf("Expected error message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser) + if !strings.Contains(result.ForLLM, "failed to read") && + !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 // but it definitely should error. // 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") { 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) - 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 - 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: @@ -653,7 +681,10 @@ func TestWhitelistFs_BlocksSymlinkEscapeInAllowedDir(t *testing.T) { patterns := []*regexp.Regexp{regexp.MustCompile(`^` + regexp.QuoteMeta(allowedDir))} 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 { 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") { 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") { t.Fatalf("did not expect second line after truncation, got: %s", result.ForLLM) }