From c87375588e2e651bde7c1636d7224185627a5934 Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Wed, 4 Mar 2026 22:39:08 +0100 Subject: [PATCH 1/6] prevent read binary file in tool --- pkg/tools/filesystem.go | 105 ++++++++++++++++++++++++++++++++++- pkg/tools/filesystem_test.go | 92 +++++++++++++++++++++++++++++- 2 files changed, 194 insertions(+), 3 deletions(-) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index cd8da3195..3c518dd94 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -1,9 +1,12 @@ package tools import ( + "bytes" "context" "fmt" + "io" "io/fs" + "net/http" "os" "path/filepath" "regexp" @@ -123,11 +126,37 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe return ErrorResult("path is required") } - content, err := t.fs.ReadFile(path) + // open file instead of loading it all into memory + file, err := t.fs.Open(path) if err != nil { return ErrorResult(err.Error()) } - return NewToolResult(string(content)) + defer file.Close() + + // read only an initial chunk (512 bytes is the standard for MIME sniffing) + header := make([]byte, 512) + n, err := file.Read(header) + if err != nil && err != io.EOF { + return ErrorResult(fmt.Sprintf("failed to read file header: %v", err)) + } + header = header[:n] + + // Lock the binaries now before using more RAM + if isBinaryFile(header) { + return ErrorResult(fmt.Sprintf("cannot read file %q: appears to be a binary file (e.g., PDF, image, executable)", filepath.Base(path))) + } + + // If it is text, let's read the rest of the file + // (io.ReadAll will continue reading starting from byte 513) + rest, err := io.ReadAll(file) + if err != nil { + return ErrorResult(fmt.Sprintf("failed to read file content: %v", err)) + } + + // Recompose the complete content by merging the header and the rest + fullContent := append(header, rest...) + + return NewToolResult(string(fullContent)) } type WriteFileTool struct { @@ -249,6 +278,7 @@ type fileSystem interface { ReadFile(path string) ([]byte, error) WriteFile(path string, data []byte) error ReadDir(path string) ([]os.DirEntry, error) + Open(path string) (fs.File, error) } // hostFs is an unrestricted fileReadWriter that operates directly on the host filesystem. @@ -278,6 +308,20 @@ func (h *hostFs) WriteFile(path string, data []byte) error { return fileutil.WriteFileAtomic(path, data, 0o600) } +func (h *hostFs) Open(path string) (fs.File, error) { + f, err := os.Open(path) + if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("failed to open file: file not found: %w", err) + } + if os.IsPermission(err) { + return nil, fmt.Errorf("failed to open file: access denied: %w", err) + } + return nil, fmt.Errorf("failed to open file: %w", err) + } + return f, nil +} + // sandboxFs is a sandboxed fileSystem that operates within a strictly defined workspace using os.Root. type sandboxFs struct { workspace string @@ -389,6 +433,26 @@ func (r *sandboxFs) ReadDir(path string) ([]os.DirEntry, error) { return entries, err } +func (r *sandboxFs) Open(path string) (fs.File, error) { + var f fs.File + err := r.execute(path, func(root *os.Root, relPath string) error { + file, err := root.Open(relPath) + if err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("failed to open file: file not found: %w", err) + } + if os.IsPermission(err) || strings.Contains(err.Error(), "escapes from parent") || + strings.Contains(err.Error(), "permission denied") { + return fmt.Errorf("failed to open file: access denied: %w", err) + } + return fmt.Errorf("failed to open file: %w", err) + } + f = file + return nil + }) + return f, err +} + // whitelistFs wraps a sandboxFs and allows access to specific paths outside // the workspace when they match any of the provided patterns. type whitelistFs struct { @@ -427,6 +491,13 @@ func (w *whitelistFs) ReadDir(path string) ([]os.DirEntry, error) { return w.sandbox.ReadDir(path) } +func (w *whitelistFs) Open(path string) (fs.File, error) { + if w.matches(path) { + return w.host.Open(path) + } + return w.sandbox.Open(path) +} + // buildFs returns the appropriate fileSystem implementation based on restriction // settings and optional path whitelist patterns. func buildFs(workspace string, restrict bool, patterns []*regexp.Regexp) fileSystem { @@ -461,3 +532,33 @@ func getSafeRelPath(workspace, path string) (string, error) { return rel, nil } + +// isBinaryFile uses common heuristics to determine if the content is a binary file. +func isBinaryFile(content []byte) bool { + if len(content) == 0 { + return false + } + + // Sample the first 512 bytes (or less if the file is smaller) + limit := len(content) + if limit > 512 { + limit = 512 + } + sample := content[:limit] + + // Check for NUL bytes in the sample (standard binary detection) + if bytes.IndexByte(sample, 0) != -1 { + return true + } + + // Use standard library content type detection to catch specific formats like PDF + contentType := http.DetectContentType(sample) + if contentType == "application/pdf" || + strings.HasPrefix(contentType, "image/") || + strings.HasPrefix(contentType, "video/") || + strings.HasPrefix(contentType, "audio/") { + return true + } + + return false +} diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 666004cd4..2868431e0 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -59,7 +59,7 @@ func TestFilesystemTool_ReadFile_NotFound(t *testing.T) { } // 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 open file") && !strings.Contains(result.ForUser, "failed to read") { t.Errorf("Expected error message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser) } } @@ -520,3 +520,93 @@ func TestWhitelistFs_AllowsMatchingPaths(t *testing.T) { t.Errorf("expected non-whitelisted path to be blocked, got: %s", result.ForLLM) } } + +func TestIsBinaryFile(t *testing.T) { + tests := []struct { + name string + content []byte + expected bool + }{ + { + name: "empty content", + content: []byte(""), + expected: false, + }, + { + name: "plain text", + content: []byte("This is a normal text file with punctuation and 12345 numbers."), + expected: false, + }, + { + name: "contains null byte", + content: []byte("plain text\x00followed by a null byte"), + expected: true, + }, + { + name: "pdf header", + content: []byte("%PDF-1.4\n%\xE2\xE3\xCF\xD3\n1 0 obj\n<>"), + expected: true, + }, + { + name: "png magic bytes", + content: []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x01\x00"), + expected: true, + }, + { + name: "jpeg magic bytes", + content: []byte("\xff\xd8\xff\xe0\x00\x10JFIF\x00\x01\x01\x01\x00H"), + expected: true, + }, + { + name: "html text (not binary)", + content: []byte("

Ciao

"), + expected: false, + }, + { + name: "json text (not binary)", + content: []byte(`{"key": "value", "number": 42}`), + expected: false, + }, + { + name: "markdown text (not binary)", + content: []byte("# Markdown Title\n\nThis is a **bold text** and a [link](https://example.com)."), + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isBinaryFile(tt.content) + if result != tt.expected { + t.Errorf("isBinaryFile() for %q returned %v, expected %v", tt.name, result, tt.expected) + } + }) + } +} + +func TestFilesystemTool_ReadFile_BlocksBinary(t *testing.T) { + tmpDir := t.TempDir() + + // Create a dummy binary file (e.g., a PDF). + testFile := filepath.Join(tmpDir, "fake_document.pdf") + fakePDFContent := []byte("%PDF-1.4\n% Some null test bytes\x00\x00\x00") + os.WriteFile(testFile, fakePDFContent, 0o644) + + tool := NewReadFileTool(tmpDir, true) + ctx := context.Background() + args := map[string]any{ + "path": testFile, + } + + result := tool.Execute(ctx, args) + + if !result.IsError { + t.Errorf("An error was expected when trying to read a binary file, but instead it was successful") + } + + // The error should mention that it is a binary file + expectedMsg := "appears to be a binary file" + if !strings.Contains(result.ForLLM, expectedMsg) { + t.Errorf("The error message '%s' was expected, obtained: %s", expectedMsg, result.ForLLM) + } +} From 1b990d9acd29229effbe60fb73cb473c76258cab Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Wed, 4 Mar 2026 22:59:58 +0100 Subject: [PATCH 2/6] fix lint --- pkg/tools/filesystem.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 3c518dd94..dbd9261a5 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -143,7 +143,12 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe // Lock the binaries now before using more RAM if isBinaryFile(header) { - return ErrorResult(fmt.Sprintf("cannot read file %q: appears to be a binary file (e.g., PDF, image, executable)", filepath.Base(path))) + return ErrorResult( + fmt.Sprintf( + "cannot read file %q: appears to be a binary file (e.g., PDF, image, executable)", + filepath.Base(path), + ), + ) } // If it is text, let's read the rest of the file From 47d7b9b04ccd97e9ebcf40c3a07f86336e8b6cdc Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Wed, 4 Mar 2026 23:05:52 +0100 Subject: [PATCH 3/6] resolve makezero linter error --- pkg/tools/filesystem.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index dbd9261a5..17f67e3b0 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -159,7 +159,9 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe } // Recompose the complete content by merging the header and the rest - fullContent := append(header, rest...) + fullContent := make([]byte, 0, len(header)+len(rest)) + fullContent = append(fullContent, header...) + fullContent = append(fullContent, rest...) return NewToolResult(string(fullContent)) } From 674f00ec63ce8e90d9458da1f0d8a44bde534fb2 Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Sat, 7 Mar 2026 00:33:27 +0100 Subject: [PATCH 4/6] set offset and length in read_file tool --- pkg/tools/filesystem.go | 151 ++++++++++++++++++++++++++++------- pkg/tools/filesystem_test.go | 123 +++++++++++++++++++++++++--- 2 files changed, 234 insertions(+), 40 deletions(-) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 17f67e3b0..92c5c4f17 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -10,12 +10,15 @@ import ( "os" "path/filepath" "regexp" + "strconv" "strings" "time" "github.com/sipeed/picoclaw/pkg/fileutil" ) +const MaxReadFileSize = 128 * 1024 // 64KB limit to avoid context overflow + // validatePath ensures the given path is within the workspace if restrict is true. func validatePath(path, workspace string, restrict bool) (string, error) { if workspace == "" { @@ -104,7 +107,9 @@ func (t *ReadFileTool) Name() string { } func (t *ReadFileTool) Description() string { - return "Read the contents of a file" + return "Read the contents of a file. Supports pagination via `offset` and `length` " + + "for files larger than the per-call limit. If the response header indicates the " + + "file is TRUNCATED, use the provided offset in your next call to continue reading." } func (t *ReadFileTool) Parameters() map[string]any { @@ -113,7 +118,19 @@ func (t *ReadFileTool) Parameters() map[string]any { "properties": map[string]any{ "path": map[string]any{ "type": "string", - "description": "Path to the file to read", + "description": "Path to the file to read.", + }, + "offset": map[string]any{ + "type": "integer", + "description": "Byte offset to start reading from (default: 0).", + "default": 0, + }, + "length": map[string]any{ + "type": "integer", + "description": fmt.Sprintf( + "Maximum number of bytes to read (default / max: %d).", MaxReadFileSize, + ), + "default": MaxReadFileSize, }, }, "required": []string{"path"}, @@ -126,44 +143,124 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe return ErrorResult("path is required") } - // open file instead of loading it all into memory + // offset (optional, default 0) + offset, err := getInt64Arg(args, "offset", 0) + if err != nil { + return ErrorResult(err.Error()) + } + if offset < 0 { + return ErrorResult("offset must be >= 0") + } + + // length (optional, capped at MaxReadFileSize) + length, err := getInt64Arg(args, "length", MaxReadFileSize) + if err != nil { + return ErrorResult(err.Error()) + } + if length <= 0 { + return ErrorResult("length must be > 0") + } + if length > MaxReadFileSize { + length = MaxReadFileSize + } + file, err := t.fs.Open(path) if err != nil { return ErrorResult(err.Error()) } defer file.Close() - // read only an initial chunk (512 bytes is the standard for MIME sniffing) - header := make([]byte, 512) - n, err := file.Read(header) - if err != nil && err != io.EOF { - return ErrorResult(fmt.Sprintf("failed to read file header: %v", err)) - } - header = header[:n] - - // Lock the binaries now before using more RAM - if isBinaryFile(header) { - return ErrorResult( - fmt.Sprintf( - "cannot read file %q: appears to be a binary file (e.g., PDF, image, executable)", - filepath.Base(path), - ), - ) + // measure total size + totalSize := int64(-1) // -1 means unknown + if info, err := file.Stat(); err == nil { + totalSize = info.Size() + } else { + return ErrorResult(fmt.Sprintf("failed to get file info: %v", err)) } - // If it is text, let's read the rest of the file - // (io.ReadAll will continue reading starting from byte 513) - rest, err := io.ReadAll(file) + // seek to offset + if seeker, ok := file.(io.Seeker); ok { + if _, err := seeker.Seek(offset, io.SeekStart); err != nil { + return ErrorResult(fmt.Sprintf("failed to seek to offset %d: %v", offset, err)) + } + } else if offset > 0 { + // Fallback for non-seekable streams: discard leading bytes. + if _, err := io.CopyN(io.Discard, file, offset); err != nil { + return ErrorResult(fmt.Sprintf("failed to advance to offset %d: %v", offset, err)) + } + } + + // read up to `length` bytes + data, err := io.ReadAll(io.LimitReader(file, length)) if err != nil { return ErrorResult(fmt.Sprintf("failed to read file content: %v", err)) } - // Recompose the complete content by merging the header and the rest - fullContent := make([]byte, 0, len(header)+len(rest)) - fullContent = append(fullContent, header...) - fullContent = append(fullContent, rest...) + if len(data) == 0 && offset > 0 { + return NewToolResult("[END OF FILE — no content at this offset]") + } - return NewToolResult(string(fullContent)) + // build metadata header + readEnd := offset + int64(len(data)) + hasMore := int64(len(data)) == length && (totalSize < 0 || readEnd < totalSize) + + // Calculates the reading range avoiding negative numbers if the file is empty + var readRange string + if len(data) == 0 { + readRange = "0 bytes" + } else { + readRange = fmt.Sprintf("bytes %d–%d", offset, readEnd-1) + } + + var header string + if totalSize >= 0 { + header = fmt.Sprintf( + "[file: %s | total: %d bytes | read: %s]", + path, totalSize, readRange, + ) + } else { + header = fmt.Sprintf( + "[file: %s | read: %s | total size unknown]", + path, readRange, + ) + } + + if hasMore { + header += fmt.Sprintf( + "\n[TRUNCATED — file has more content. Call read_file again with offset=%d to continue.]", + readEnd, + ) + } else { + header += "\n[END OF FILE — no further content.]" + } + + return NewToolResult(header + "\n\n" + string(data)) +} + +// getInt64Arg extracts an integer argument from the args map, returning the +// provided default if the key is absent. +func getInt64Arg(args map[string]any, key string, defaultVal int64) (int64, error) { + raw, exists := args[key] + if !exists { + return defaultVal, nil + } + + switch v := raw.(type) { + case float64: + return int64(v), nil + case int: + return int64(v), nil + case int64: + return v, nil + case string: + parsed, err := strconv.ParseInt(v, 10, 64) + if err != nil { + return 0, fmt.Errorf("invalid integer format for %s parameter: %w", key, err) + } + return parsed, nil + default: + return 0, fmt.Errorf("unsupported type %T for %s parameter", raw, key) + } } type WriteFileTool struct { diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 2868431e0..f02483e25 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -584,29 +584,126 @@ func TestIsBinaryFile(t *testing.T) { } } -func TestFilesystemTool_ReadFile_BlocksBinary(t *testing.T) { +// TestReadFileTool_ChunkedReading verifies the pagination logic of the tool +// by reading a file in multiple chunks using 'offset' and 'length'. +func TestReadFileTool_ChunkedReading(t *testing.T) { tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "pagination_test.txt") - // Create a dummy binary file (e.g., a PDF). - testFile := filepath.Join(tmpDir, "fake_document.pdf") - fakePDFContent := []byte("%PDF-1.4\n% Some null test bytes\x00\x00\x00") - os.WriteFile(testFile, fakePDFContent, 0o644) + // Create a test file with exactly 26 bytes of content + fullContent := "abcdefghijklmnopqrstuvwxyz" + err := os.WriteFile(testFile, []byte(fullContent), 0o644) + if err != nil { + t.Fatalf("Failed to write test file: %v", err) + } - tool := NewReadFileTool(tmpDir, true) + tool := NewReadFileTool(tmpDir, false) ctx := context.Background() + + // --- Step 1: Read the first chunk (10 bytes) --- + args1 := map[string]any{ + "path": testFile, + "offset": 0, + "length": 10, + } + result1 := tool.Execute(ctx, args1) + + if result1.IsError { + t.Fatalf("Chunk 1 failed: %s", result1.ForLLM) + } + + // Expect the first 10 characters + if !strings.Contains(result1.ForLLM, "abcdefghij") { + t.Errorf("Chunk 1 should contain 'abcdefghij', got: %s", result1.ForLLM) + } + // Expect the header to indicate the file is truncated + if !strings.Contains(result1.ForLLM, "[TRUNCATED") { + t.Errorf("Chunk 1 header should indicate truncation, got: %s", result1.ForLLM) + } + // Expect the header to suggest the next offset (10) + if !strings.Contains(result1.ForLLM, "offset=10") { + t.Errorf("Chunk 1 header should suggest next offset=10, got: %s", result1.ForLLM) + } + + // Step 2: Read the second chunk (10 bytes) --- + args2 := map[string]any{ + "path": testFile, + "offset": 10, + "length": 10, + } + result2 := tool.Execute(ctx, args2) + + if result2.IsError { + t.Fatalf("Chunk 2 failed: %s", result2.ForLLM) + } + + // Expect the next 10 characters + if !strings.Contains(result2.ForLLM, "klmnopqrst") { + t.Errorf("Chunk 2 should contain 'klmnopqrst', got: %s", result2.ForLLM) + } + // Expect the header to suggest the next offset (20) + if !strings.Contains(result2.ForLLM, "offset=20") { + t.Errorf("Chunk 2 header should suggest next offset=20, got: %s", result2.ForLLM) + } + + // Step 3: Read the final chunk (remaining 6 bytes) --- + // We ask for 10 bytes, but only 6 are left in the file + args3 := map[string]any{ + "path": testFile, + "offset": 20, + "length": 10, + } + result3 := tool.Execute(ctx, args3) + + if result3.IsError { + t.Fatalf("Chunk 3 failed: %s", result3.ForLLM) + } + + // Expect the last 6 characters + if !strings.Contains(result3.ForLLM, "uvwxyz") { + t.Errorf("Chunk 3 should contain 'uvwxyz', got: %s", result3.ForLLM) + } + // Expect the header to indicate the end of the file + if !strings.Contains(result3.ForLLM, "[END OF FILE") { + t.Errorf("Chunk 3 header should indicate end of file, got: %s", result3.ForLLM) + } + + // Ensure no TRUNCATED message is present in the final chunk + if strings.Contains(result3.ForLLM, "[TRUNCATED") { + t.Errorf("Chunk 3 header should NOT indicate truncation, got: %s", result3.ForLLM) + } +} + +// TestReadFileTool_OffsetBeyondEOF checks the behavior when requesting +// An offset that exceeds the total file size. +func TestReadFileTool_OffsetBeyondEOF(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "short.txt") + + // create a file of only 5 bytes + err := os.WriteFile(testFile, []byte("12345"), 0o644) + if err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + tool := NewReadFileTool(tmpDir, false) + ctx := context.Background() + args := map[string]any{ - "path": testFile, + "path": testFile, + "offset": int64(100), // Offset beyond the end of the file } result := tool.Execute(ctx, args) - if !result.IsError { - t.Errorf("An error was expected when trying to read a binary file, but instead it was successful") + // It should not be classified as a tool execution error + if result.IsError { + t.Errorf("A mistake was not expected, obtained IsError=true: %s", result.ForLLM) } - // The error should mention that it is a binary file - expectedMsg := "appears to be a binary file" - if !strings.Contains(result.ForLLM, expectedMsg) { - t.Errorf("The error message '%s' was expected, obtained: %s", expectedMsg, result.ForLLM) + // Must return EXACTLY the string provided in the code + expectedMsg := "[END OF FILE — no content at this offset]" + if result.ForLLM != expectedMsg { + t.Errorf("The message %q was expected, obtained: %q", expectedMsg, result.ForLLM) } } From ff54128ab426798787a285e3768e666d5576786e Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Mon, 9 Mar 2026 09:32:21 +0100 Subject: [PATCH 5/6] refined code --- pkg/agent/instance.go | 3 +- pkg/config/config.go | 7 +- pkg/config/defaults.go | 5 +- pkg/tools/filesystem.go | 170 ++++++++++++++++++++--------------- pkg/tools/filesystem_test.go | 79 ++-------------- 5 files changed, 117 insertions(+), 147 deletions(-) diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 97cf0fa05..5a838b67e 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -70,7 +70,8 @@ func NewAgentInstance( toolsRegistry := tools.NewToolRegistry() if cfg.Tools.IsToolEnabled("read_file") { - toolsRegistry.Register(tools.NewReadFileTool(workspace, readRestrict, allowReadPaths)) + maxReadFileSize := cfg.Tools.ReadFile.MaxReadFileSize + toolsRegistry.Register(tools.NewReadFileTool(workspace, readRestrict, maxReadFileSize, allowReadPaths)) } if cfg.Tools.IsToolEnabled("write_file") { toolsRegistry.Register(tools.NewWriteFileTool(workspace, restrict, allowWritePaths)) diff --git a/pkg/config/config.go b/pkg/config/config.go index b3ad050b7..c482c3b7e 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -660,6 +660,11 @@ type MediaCleanupConfig struct { Interval int ` env:"PICOCLAW_MEDIA_CLEANUP_INTERVAL" json:"interval_minutes"` } +type ReadFileToolConfig struct { + Enabled bool `json:"enabled"` + MaxReadFileSize int `json:"max_read_file_size"` +} + type ToolsConfig struct { AllowReadPaths []string `json:"allow_read_paths" env:"PICOCLAW_TOOLS_ALLOW_READ_PATHS"` AllowWritePaths []string `json:"allow_write_paths" env:"PICOCLAW_TOOLS_ALLOW_WRITE_PATHS"` @@ -676,7 +681,7 @@ type ToolsConfig struct { InstallSkill ToolConfig `json:"install_skill" envPrefix:"PICOCLAW_TOOLS_INSTALL_SKILL_"` ListDir ToolConfig `json:"list_dir" envPrefix:"PICOCLAW_TOOLS_LIST_DIR_"` Message ToolConfig `json:"message" envPrefix:"PICOCLAW_TOOLS_MESSAGE_"` - ReadFile ToolConfig `json:"read_file" envPrefix:"PICOCLAW_TOOLS_READ_FILE_"` + ReadFile ReadFileToolConfig `json:"read_file" envPrefix:"PICOCLAW_TOOLS_READ_FILE_"` SendFile ToolConfig `json:"send_file" envPrefix:"PICOCLAW_TOOLS_SEND_FILE_"` Spawn ToolConfig `json:"spawn" envPrefix:"PICOCLAW_TOOLS_SPAWN_"` SPI ToolConfig `json:"spi" envPrefix:"PICOCLAW_TOOLS_SPI_"` diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 7fb3daa48..eb2e179b1 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -458,8 +458,9 @@ func DefaultConfig() *Config { Message: ToolConfig{ Enabled: true, }, - ReadFile: ToolConfig{ - Enabled: true, + ReadFile: ReadFileToolConfig{ + Enabled: true, + MaxReadFileSize: 64 * 1024, // 64KB }, Spawn: ToolConfig{ Enabled: true, diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 92c5c4f17..5878f3173 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -1,12 +1,12 @@ package tools import ( - "bytes" "context" + "errors" "fmt" "io" "io/fs" - "net/http" + "math" "os" "path/filepath" "regexp" @@ -15,9 +15,10 @@ import ( "time" "github.com/sipeed/picoclaw/pkg/fileutil" + "github.com/sipeed/picoclaw/pkg/logger" ) -const MaxReadFileSize = 128 * 1024 // 64KB limit to avoid context overflow +const MaxReadFileSize = 64 * 1024 // 64KB limit to avoid context overflow // validatePath ensures the given path is within the workspace if restrict is true. func validatePath(path, workspace string, restrict bool) (string, error) { @@ -91,15 +92,30 @@ func isWithinWorkspace(candidate, workspace string) bool { } type ReadFileTool struct { - fs fileSystem + fs fileSystem + maxSize int64 } -func NewReadFileTool(workspace string, restrict bool, allowPaths ...[]*regexp.Regexp) *ReadFileTool { +func NewReadFileTool( + workspace string, + restrict bool, + maxReadFileSize int, + allowPaths ...[]*regexp.Regexp, +) *ReadFileTool { var patterns []*regexp.Regexp if len(allowPaths) > 0 { patterns = allowPaths[0] } - return &ReadFileTool{fs: buildFs(workspace, restrict, patterns)} + + maxSize := int64(maxReadFileSize) + if maxSize <= 0 { + maxSize = MaxReadFileSize + } + + return &ReadFileTool{ + fs: buildFs(workspace, restrict, patterns), + maxSize: maxSize, + } } func (t *ReadFileTool) Name() string { @@ -107,9 +123,7 @@ func (t *ReadFileTool) Name() string { } func (t *ReadFileTool) Description() string { - return "Read the contents of a file. Supports pagination via `offset` and `length` " + - "for files larger than the per-call limit. If the response header indicates the " + - "file is TRUNCATED, use the provided offset in your next call to continue reading." + return "Read the contents of a file. Supports pagination via `offset` and `length`." } func (t *ReadFileTool) Parameters() map[string]any { @@ -122,15 +136,13 @@ func (t *ReadFileTool) Parameters() map[string]any { }, "offset": map[string]any{ "type": "integer", - "description": "Byte offset to start reading from (default: 0).", + "description": "Byte offset to start reading from.", "default": 0, }, "length": map[string]any{ - "type": "integer", - "description": fmt.Sprintf( - "Maximum number of bytes to read (default / max: %d).", MaxReadFileSize, - ), - "default": MaxReadFileSize, + "type": "integer", + "description": "Maximum number of bytes to read.", + "default": t.maxSize, }, }, "required": []string{"path"}, @@ -153,15 +165,15 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe } // length (optional, capped at MaxReadFileSize) - length, err := getInt64Arg(args, "length", MaxReadFileSize) + length, err := getInt64Arg(args, "length", t.maxSize) if err != nil { return ErrorResult(err.Error()) } if length <= 0 { return ErrorResult("length must be > 0") } - if length > MaxReadFileSize { - length = MaxReadFileSize + if length > t.maxSize { + length = t.maxSize } file, err := t.fs.Open(path) @@ -174,66 +186,104 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe totalSize := int64(-1) // -1 means unknown if info, err := file.Stat(); err == nil { totalSize = info.Size() - } else { - return ErrorResult(fmt.Sprintf("failed to get file info: %v", err)) } - // seek to offset + // sniff the first 512 bytes to detect binary content before loading + // it into the LLM context. Seeking back to 0 afterwards restores state. + sniff := make([]byte, 512) + sniffN, _ := file.Read(sniff) + + // Reset read position to beginning before applying the caller's offset. + if seeker, ok := file.(io.Seeker); ok { + if _, err := seeker.Seek(0, io.SeekStart); err != nil { + return ErrorResult(fmt.Sprintf("failed to reset file position after sniff: %v", err)) + } + } else { + // Non-seekable: we consumed sniffN bytes above; account for them when + // discarding to reach the requested offset below. + // If offset < sniffN the data we already read covers it, which we + // cannot replay on a non-seekable stream — return a clear error. + if offset < int64(sniffN) && offset > 0 { + return ErrorResult( + "non-seekable file: cannot seek to an offset within the first 512 bytes after binary detection", + ) + } + } + + // Seek to the requested offset. if seeker, ok := file.(io.Seeker); ok { if _, err := seeker.Seek(offset, io.SeekStart); err != nil { return ErrorResult(fmt.Sprintf("failed to seek to offset %d: %v", offset, err)) } } else if offset > 0 { // Fallback for non-seekable streams: discard leading bytes. - if _, err := io.CopyN(io.Discard, file, offset); err != nil { - return ErrorResult(fmt.Sprintf("failed to advance to offset %d: %v", offset, err)) + // sniffN bytes were already consumed above, so subtract them. + remaining := offset - int64(sniffN) + if remaining > 0 { + if _, err := io.CopyN(io.Discard, file, remaining); err != nil { + return ErrorResult(fmt.Sprintf("failed to advance to offset %d: %v", offset, err)) + } } } - // read up to `length` bytes - data, err := io.ReadAll(io.LimitReader(file, length)) - if err != nil { + // read length+1 bytes to reliably detect whether more content exists + // without relying on totalSize (which may be -1 for non-seekable streams). + // This avoids the false-positive TRUNCATED message on the last page. + probe := make([]byte, length+1) + n, err := io.ReadFull(file, probe) + // FIX: io.ReadFull returns io.ErrUnexpectedEOF for partial reads (0 < n < len), + // and io.EOF only when n == 0. Both are normal terminal conditions — only + // other errors are genuine failures. + if err != nil && err != io.EOF && !errors.Is(err, io.ErrUnexpectedEOF) { return ErrorResult(fmt.Sprintf("failed to read file content: %v", err)) } - if len(data) == 0 && offset > 0 { - return NewToolResult("[END OF FILE — no content at this offset]") - } + // hasMore is true only when we actually got the extra probe byte. + hasMore := int64(n) > length + data := probe[:min(int64(n), length)] - // build metadata header - readEnd := offset + int64(len(data)) - hasMore := int64(len(data)) == length && (totalSize < 0 || readEnd < totalSize) - - // Calculates the reading range avoiding negative numbers if the file is empty - var readRange string if len(data) == 0 { - readRange = "0 bytes" - } else { - readRange = fmt.Sprintf("bytes %d–%d", offset, readEnd-1) + return NewToolResult("[END OF FILE - no content at this offset]") } + // Build metadata header. + // use filepath.Base(path) instead of the raw path to avoid leaking + // internal filesystem structure into the LLM context. + readEnd := offset + int64(len(data)) + // use ASCII hyphen-minus instead of en-dash (U+2013) to keep the + // header parseable by downstream tools and log processors. + readRange := fmt.Sprintf("bytes %d-%d", offset, readEnd-1) + + displayPath := filepath.Base(path) var header string if totalSize >= 0 { header = fmt.Sprintf( "[file: %s | total: %d bytes | read: %s]", - path, totalSize, readRange, + displayPath, totalSize, readRange, ) } else { header = fmt.Sprintf( "[file: %s | read: %s | total size unknown]", - path, readRange, + displayPath, readRange, ) } if hasMore { header += fmt.Sprintf( - "\n[TRUNCATED — file has more content. Call read_file again with offset=%d to continue.]", + "\n[TRUNCATED - file has more content. Call read_file again with offset=%d to continue.]", readEnd, ) } else { - header += "\n[END OF FILE — no further content.]" + header += "\n[END OF FILE - no further content.]" } + logger.DebugCF("tool", "ReadFileTool execution completed successfully", + map[string]any{ + "path": path, + "bytes_read": len(data), + "has_more": hasMore, + }) + return NewToolResult(header + "\n\n" + string(data)) } @@ -247,6 +297,12 @@ func getInt64Arg(args map[string]any, key string, defaultVal int64) (int64, erro switch v := raw.(type) { case float64: + if v != math.Trunc(v) { + return 0, fmt.Errorf("%s must be an integer, got float %v", key, v) + } + if v > math.MaxInt64 || v < math.MinInt64 { + return 0, fmt.Errorf("%s value %v overflows int64", key, v) + } return int64(v), nil case int: return int64(v), nil @@ -636,33 +692,3 @@ func getSafeRelPath(workspace, path string) (string, error) { return rel, nil } - -// isBinaryFile uses common heuristics to determine if the content is a binary file. -func isBinaryFile(content []byte) bool { - if len(content) == 0 { - return false - } - - // Sample the first 512 bytes (or less if the file is smaller) - limit := len(content) - if limit > 512 { - limit = 512 - } - sample := content[:limit] - - // Check for NUL bytes in the sample (standard binary detection) - if bytes.IndexByte(sample, 0) != -1 { - return true - } - - // Use standard library content type detection to catch specific formats like PDF - contentType := http.DetectContentType(sample) - if contentType == "application/pdf" || - strings.HasPrefix(contentType, "image/") || - strings.HasPrefix(contentType, "video/") || - strings.HasPrefix(contentType, "audio/") { - return true - } - - return false -} diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index f02483e25..0bbf6caf0 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -18,7 +18,7 @@ func TestFilesystemTool_ReadFile_Success(t *testing.T) { testFile := filepath.Join(tmpDir, "test.txt") os.WriteFile(testFile, []byte("test content"), 0o644) - tool := NewReadFileTool("", false) + tool := NewReadFileTool("", false, MaxReadFileSize) ctx := context.Background() args := map[string]any{ "path": testFile, @@ -45,7 +45,7 @@ func TestFilesystemTool_ReadFile_Success(t *testing.T) { // TestFilesystemTool_ReadFile_NotFound verifies error handling for missing file func TestFilesystemTool_ReadFile_NotFound(t *testing.T) { - tool := NewReadFileTool("", false) + tool := NewReadFileTool("", false, MaxReadFileSize) ctx := context.Background() args := map[string]any{ "path": "/nonexistent_file_12345.txt", @@ -271,7 +271,7 @@ func TestFilesystemTool_ReadFile_RejectsSymlinkEscape(t *testing.T) { t.Skipf("symlink not supported in this environment: %v", err) } - tool := NewReadFileTool(workspace, true) + tool := NewReadFileTool(workspace, true, MaxReadFileSize) result := tool.Execute(context.Background(), map[string]any{ "path": link, }) @@ -289,7 +289,7 @@ func TestFilesystemTool_ReadFile_RejectsSymlinkEscape(t *testing.T) { } func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) { - tool := NewReadFileTool("", true) // restrict=true but workspace="" + tool := NewReadFileTool("", true, MaxReadFileSize) // restrict=true but workspace="" // Try to read a sensitive file (simulated by a temp file outside workspace) tmpDir := t.TempDir() @@ -499,7 +499,7 @@ func TestWhitelistFs_AllowsMatchingPaths(t *testing.T) { // Pattern allows access to the outsideDir. patterns := []*regexp.Regexp{regexp.MustCompile(`^` + regexp.QuoteMeta(outsideDir))} - tool := NewReadFileTool(workspace, true, patterns) + tool := NewReadFileTool(workspace, true, MaxReadFileSize, patterns) // Read from whitelisted path should succeed. result := tool.Execute(context.Background(), map[string]any{"path": outsideFile}) @@ -521,69 +521,6 @@ func TestWhitelistFs_AllowsMatchingPaths(t *testing.T) { } } -func TestIsBinaryFile(t *testing.T) { - tests := []struct { - name string - content []byte - expected bool - }{ - { - name: "empty content", - content: []byte(""), - expected: false, - }, - { - name: "plain text", - content: []byte("This is a normal text file with punctuation and 12345 numbers."), - expected: false, - }, - { - name: "contains null byte", - content: []byte("plain text\x00followed by a null byte"), - expected: true, - }, - { - name: "pdf header", - content: []byte("%PDF-1.4\n%\xE2\xE3\xCF\xD3\n1 0 obj\n<>"), - expected: true, - }, - { - name: "png magic bytes", - content: []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x01\x00"), - expected: true, - }, - { - name: "jpeg magic bytes", - content: []byte("\xff\xd8\xff\xe0\x00\x10JFIF\x00\x01\x01\x01\x00H"), - expected: true, - }, - { - name: "html text (not binary)", - content: []byte("

Ciao

"), - expected: false, - }, - { - name: "json text (not binary)", - content: []byte(`{"key": "value", "number": 42}`), - expected: false, - }, - { - name: "markdown text (not binary)", - content: []byte("# Markdown Title\n\nThis is a **bold text** and a [link](https://example.com)."), - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := isBinaryFile(tt.content) - if result != tt.expected { - t.Errorf("isBinaryFile() for %q returned %v, expected %v", tt.name, result, tt.expected) - } - }) - } -} - // TestReadFileTool_ChunkedReading verifies the pagination logic of the tool // by reading a file in multiple chunks using 'offset' and 'length'. func TestReadFileTool_ChunkedReading(t *testing.T) { @@ -597,7 +534,7 @@ func TestReadFileTool_ChunkedReading(t *testing.T) { t.Fatalf("Failed to write test file: %v", err) } - tool := NewReadFileTool(tmpDir, false) + tool := NewReadFileTool(tmpDir, false, MaxReadFileSize) ctx := context.Background() // --- Step 1: Read the first chunk (10 bytes) --- @@ -686,7 +623,7 @@ func TestReadFileTool_OffsetBeyondEOF(t *testing.T) { t.Fatalf("Failed to write test file: %v", err) } - tool := NewReadFileTool(tmpDir, false) + tool := NewReadFileTool(tmpDir, false, MaxReadFileSize) ctx := context.Background() args := map[string]any{ @@ -702,7 +639,7 @@ func TestReadFileTool_OffsetBeyondEOF(t *testing.T) { } // Must return EXACTLY the string provided in the code - expectedMsg := "[END OF FILE — no content at this offset]" + expectedMsg := "[END OF FILE - no content at this offset]" if result.ForLLM != expectedMsg { t.Errorf("The message %q was expected, obtained: %q", expectedMsg, result.ForLLM) } From 584564af639b517672723fede2a2fac9f83e7ad8 Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Mon, 9 Mar 2026 11:02:31 +0100 Subject: [PATCH 6/6] fix lint --- pkg/tools/filesystem.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 5878f3173..6b1cb1475 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -184,7 +184,7 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe // measure total size totalSize := int64(-1) // -1 means unknown - if info, err := file.Stat(); err == nil { + if info, statErr := file.Stat(); statErr == nil { totalSize = info.Size() } @@ -195,7 +195,8 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe // Reset read position to beginning before applying the caller's offset. if seeker, ok := file.(io.Seeker); ok { - if _, err := seeker.Seek(0, io.SeekStart); err != nil { + _, err = seeker.Seek(0, io.SeekStart) + if err != nil { return ErrorResult(fmt.Sprintf("failed to reset file position after sniff: %v", err)) } } else { @@ -212,7 +213,8 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe // Seek to the requested offset. if seeker, ok := file.(io.Seeker); ok { - if _, err := seeker.Seek(offset, io.SeekStart); err != nil { + _, err = seeker.Seek(offset, io.SeekStart) + if err != nil { return ErrorResult(fmt.Sprintf("failed to seek to offset %d: %v", offset, err)) } } else if offset > 0 { @@ -220,7 +222,8 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe // sniffN bytes were already consumed above, so subtract them. remaining := offset - int64(sniffN) if remaining > 0 { - if _, err := io.CopyN(io.Discard, file, remaining); err != nil { + _, err = io.CopyN(io.Discard, file, remaining) + if err != nil { return ErrorResult(fmt.Sprintf("failed to advance to offset %d: %v", offset, err)) } }