From ca64624c88808afd6cbf2e75ba3ddcfec37cea02 Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Tue, 24 Mar 2026 22:54:13 +0100 Subject: [PATCH] feat(tool): read_file tool by lines --- config/config.example.json | 3 +- docs/configuration.md | 62 +++++ docs/it/configuration.md | 62 +++++ pkg/agent/instance.go | 7 +- pkg/agent/instance_test.go | 35 +++ pkg/config/config.go | 21 +- pkg/config/config_test.go | 7 + pkg/config/defaults.go | 1 + pkg/routing/features.go | 6 +- pkg/tools/filesystem.go | 487 ++++++++++++++++++++++++++++------- pkg/tools/filesystem_test.go | 188 +++++++++++--- 11 files changed, 731 insertions(+), 148 deletions(-) diff --git a/config/config.example.json b/config/config.example.json index ff2969dcb..e82599065 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -412,7 +412,8 @@ "enabled": true }, "read_file": { - "enabled": true + "enabled": true, + "mode": "bytes" }, "spawn": { "enabled": true diff --git a/docs/configuration.md b/docs/configuration.md index 9360d3897..9a016420c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -287,6 +287,68 @@ Even with `restrict_to_workspace: false`, the `exec` tool blocks these dangerous | `tools.allow_read_paths` | string[] | `[]` | Additional paths allowed for reading outside workspace | | `tools.allow_write_paths` | string[] | `[]` | Additional paths allowed for writing outside workspace | +### Read File Mode + +`read_file` has two mutually exclusive implementations selected by config. PicoClaw registers exactly one of them at startup: + +| Config Key | Type | Default | Description | +|------------|------|---------|-------------| +| `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 in `bytes` mode and the output budget source for `lines` mode truncation | + +#### Mode: `bytes` + +Legacy behavior, optimized for arbitrary files and binary-safe pagination. + +Parameters: + +* `path` (required): File path +* `offset` (optional): Starting byte offset, default `0` +* `length` (optional): Maximum number of bytes to read, default `max_read_file_size` + +Use `bytes` when: + +* You need backward compatibility with older prompts and agents +* You may read binary files +* You want deterministic byte-range pagination + +#### Mode: `lines` + +Text-oriented behavior, optimized for source files, markdown, logs, and configs. + +Parameters: + +* `path` (required): File path +* `offset` (optional): Starting line number, 1-indexed, default `1` +* `limit` (optional): Maximum number of lines to read, default = all remaining lines + +When content exceeds the estimated token budget, PicoClaw truncates it intelligently: + +* Estimates the token count heuristically +* Keeps the head and tail of the selected text +* Cuts on newline boundaries when possible +* Inserts an explicit truncation notice in the middle + +Use `lines` when: + +* The agent mostly reads text files +* You want line-based pagination in prompts and tool calls +* You want cleaner chunks for code review, logs, and documentation + +#### Example + +```json +{ + "tools": { + "read_file": { + "enabled": true, + "mode": "lines", + "max_read_file_size": 65536 + } + } +} +``` + ### Exec Security | Config Key | Type | Default | Description | diff --git a/docs/it/configuration.md b/docs/it/configuration.md index 6a79a9543..f2eb7969b 100644 --- a/docs/it/configuration.md +++ b/docs/it/configuration.md @@ -125,6 +125,68 @@ Anche con `restrict_to_workspace: false`, lo strumento `exec` blocca questi coma | `tools.allow_read_paths` | string[] | `[]` | Percorsi aggiuntivi consentiti per la lettura al di fuori del workspace | | `tools.allow_write_paths` | string[] | `[]` | Percorsi aggiuntivi consentiti per la scrittura al di fuori del workspace | +### Modalità Read File + +`read_file` ha due implementazioni mutuamente esclusive, selezionate via configurazione. PicoClaw ne registra esattamente una all'avvio: + +| Chiave di configurazione | Tipo | Predefinito | Descrizione | +|--------------------------|------|-------------|-------------| +| `tools.read_file.mode` | string | `bytes` | Seleziona l'implementazione di `read_file`: `bytes` oppure `lines` | +| `tools.read_file.max_read_file_size` | int | `65536` | Numero massimo di byte restituiti in modalità `bytes` e base del budget di output per la truncation in modalità `lines` | + +#### Modalità: `bytes` + +Comportamento legacy, ottimizzato per file arbitrari e paginazione byte-safe. + +Parametri: + +* `path` (obbligatorio): percorso del file +* `offset` (opzionale): offset iniziale in byte, default `0` +* `length` (opzionale): numero massimo di byte da leggere, default `max_read_file_size` + +Usa `bytes` quando: + +* Ti serve retrocompatibilità con prompt o agent esistenti +* Potresti leggere file binari +* Vuoi una paginazione deterministica a byte + +#### Modalità: `lines` + +Comportamento orientato al testo, ottimizzato per sorgenti, markdown, log e file di configurazione. + +Parametri: + +* `path` (obbligatorio): percorso del file +* `offset` (opzionale): numero di riga iniziale, 1-indexed, default `1` +* `limit` (opzionale): numero massimo di righe da leggere, default = tutte le righe rimanenti + +Quando il contenuto supera il budget token stimato, PicoClaw applica una truncation intelligente: + +* Stima il numero di token in modo euristico +* Mantiene testa e coda del testo selezionato +* Tronca, quando possibile, sui boundary di newline +* Inserisce al centro un avviso esplicito di contenuto troncato + +Usa `lines` quando: + +* L'agent legge soprattutto file testuali +* Vuoi paginazione a righe nei prompt e nei tool call +* Vuoi chunk più leggibili per code review, log e documentazione + +#### Esempio + +```json +{ + "tools": { + "read_file": { + "enabled": true, + "mode": "lines", + "max_read_file_size": 65536 + } + } +} +``` + ### Sicurezza Exec | Chiave di configurazione | Tipo | Predefinito | Descrizione | diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 880725660..bacfa49c5 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -77,7 +77,12 @@ func NewAgentInstance( if cfg.Tools.IsToolEnabled("read_file") { maxReadFileSize := cfg.Tools.ReadFile.MaxReadFileSize - toolsRegistry.Register(tools.NewReadFileTool(workspace, readRestrict, maxReadFileSize, allowReadPaths)) + switch cfg.Tools.ReadFile.EffectiveMode() { + case config.ReadFileModeLines: + toolsRegistry.Register(tools.NewReadFileLinesTool(workspace, readRestrict, maxReadFileSize, allowReadPaths)) + default: + toolsRegistry.Register(tools.NewReadFileBytesTool(workspace, readRestrict, maxReadFileSize, allowReadPaths)) + } } if cfg.Tools.IsToolEnabled("write_file") { toolsRegistry.Register(tools.NewWriteFileTool(workspace, restrict, allowWritePaths)) diff --git a/pkg/agent/instance_test.go b/pkg/agent/instance_test.go index e296a18cb..7f0406836 100644 --- a/pkg/agent/instance_test.go +++ b/pkg/agent/instance_test.go @@ -248,6 +248,41 @@ func TestNewAgentInstance_AllowsMediaTempDirForReadListAndExec(t *testing.T) { } } +func TestNewAgentInstance_ReadFileModeSelectsSchema(t *testing.T) { + workspace := t.TempDir() + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: workspace, + ModelName: "test-model", + }, + }, + Tools: config.ToolsConfig{ + ReadFile: config.ReadFileToolConfig{ + Enabled: true, + Mode: config.ReadFileModeLines, + MaxReadFileSize: 4096, + }, + }, + } + + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, &mockProvider{}) + readTool, ok := agent.Tools.Get("read_file") + if !ok { + t.Fatal("read_file tool not registered") + } + + params := readTool.Parameters() + props, _ := params["properties"].(map[string]any) + if _, ok := props["limit"]; !ok { + t.Fatalf("expected line-mode schema to expose limit, got %#v", props) + } + if _, ok := props["length"]; ok { + t.Fatalf("did not expect line-mode schema to expose length, got %#v", props) + } +} + func TestNewAgentInstance_InvalidExecConfigDoesNotExit(t *testing.T) { workspace := t.TempDir() diff --git a/pkg/config/config.go b/pkg/config/config.go index 533f45a44..3daa6e879 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -882,8 +882,25 @@ type MediaCleanupConfig struct { } type ReadFileToolConfig struct { - Enabled bool `json:"enabled"` - MaxReadFileSize int `json:"max_read_file_size"` + Enabled bool `json:"enabled"` + Mode string `json:"mode"` + MaxReadFileSize int `json:"max_read_file_size"` +} + +const ( + ReadFileModeBytes = "bytes" + ReadFileModeLines = "lines" +) + +func (c ReadFileToolConfig) EffectiveMode() string { + switch strings.ToLower(strings.TrimSpace(c.Mode)) { + case ReadFileModeLines: + return ReadFileModeLines + case "", ReadFileModeBytes: + return ReadFileModeBytes + default: + return ReadFileModeBytes + } } type ToolsConfig struct { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 75eb458b8..1e48c1371 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -317,6 +317,13 @@ func TestDefaultConfig_WebTools(t *testing.T) { } } +func TestDefaultConfig_ReadFileMode(t *testing.T) { + cfg := DefaultConfig() + if cfg.Tools.ReadFile.EffectiveMode() != ReadFileModeBytes { + t.Fatalf("expected default read_file mode %q, got %q", ReadFileModeBytes, cfg.Tools.ReadFile.EffectiveMode()) + } +} + func TestSaveConfig_FilePermissions(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("file permission bits are not enforced on Windows") diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index bc4ab0649..6bd17687c 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -480,6 +480,7 @@ func DefaultConfig() *Config { }, ReadFile: ReadFileToolConfig{ Enabled: true, + Mode: ReadFileModeBytes, MaxReadFileSize: 64 * 1024, // 64KB }, Spawn: ToolConfig{ diff --git a/pkg/routing/features.go b/pkg/routing/features.go index c371e21aa..650296b65 100644 --- a/pkg/routing/features.go +++ b/pkg/routing/features.go @@ -42,7 +42,7 @@ type Features struct { // the returned struct. func ExtractFeatures(msg string, history []providers.Message) Features { return Features{ - TokenEstimate: estimateTokens(msg), + TokenEstimate: EstimateTokens(msg), CodeBlockCount: countCodeBlocks(msg), RecentToolCalls: countRecentToolCalls(history), ConversationDepth: len(history), @@ -50,12 +50,12 @@ func ExtractFeatures(msg string, history []providers.Message) Features { } } -// estimateTokens returns a token count proxy that handles both CJK and Latin text. +// EstimateTokens returns a token count proxy that handles both CJK and Latin text. // CJK runes (U+2E80–U+9FFF, U+F900–U+FAFF, U+AC00–U+D7AF) map to roughly one // token each, while non-CJK runes average ~0.25 tokens/rune (≈4 chars per token // for English). Splitting the count this way avoids the 3x underestimation that a // flat rune_count/3 would produce for Chinese, Japanese, and Korean text. -func estimateTokens(msg string) int { +func EstimateTokens(msg string) int { total := utf8.RuneCountInString(msg) if total == 0 { return 0 diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 39d45013d..c3aef4436 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -1,25 +1,30 @@ package tools import ( + "bufio" + "bytes" "context" - "errors" "fmt" - "io" "io/fs" "math" + "net/http" "os" "path/filepath" "regexp" "strconv" "strings" "time" + "unicode/utf8" "github.com/sipeed/picoclaw/pkg/fileutil" "github.com/sipeed/picoclaw/pkg/logger" + "github.com/sipeed/picoclaw/pkg/routing" ) const MaxReadFileSize = 64 * 1024 // 64KB limit to avoid context overflow +const readFileTruncationSafetyMargin = 0.95 + func validatePathWithAllowPaths(path, workspace string, restrict bool, patterns []*regexp.Regexp) (string, error) { if workspace == "" { return path, fmt.Errorf("workspace is not defined") @@ -249,6 +254,14 @@ func isWithinWorkspace(candidate, workspace string) bool { } type ReadFileTool struct { + readFileBase +} + +type ReadFileLinesTool struct { + readFileBase +} + +type readFileBase struct { fs fileSystem maxSize int64 } @@ -258,6 +271,15 @@ func NewReadFileTool( restrict bool, maxReadFileSize int, allowPaths ...[]*regexp.Regexp, +) *ReadFileTool { + return NewReadFileBytesTool(workspace, restrict, maxReadFileSize, allowPaths...) +} + +func NewReadFileBytesTool( + workspace string, + restrict bool, + maxReadFileSize int, + allowPaths ...[]*regexp.Regexp, ) *ReadFileTool { var patterns []*regexp.Regexp if len(allowPaths) > 0 { @@ -270,8 +292,34 @@ func NewReadFileTool( } return &ReadFileTool{ - fs: buildFs(workspace, restrict, patterns), - maxSize: maxSize, + readFileBase: readFileBase{ + fs: buildFs(workspace, restrict, patterns), + maxSize: maxSize, + }, + } +} + +func NewReadFileLinesTool( + workspace string, + restrict bool, + maxReadFileSize int, + allowPaths ...[]*regexp.Regexp, +) *ReadFileLinesTool { + var patterns []*regexp.Regexp + if len(allowPaths) > 0 { + patterns = allowPaths[0] + } + + maxSize := int64(maxReadFileSize) + if maxSize <= 0 { + maxSize = MaxReadFileSize + } + + return &ReadFileLinesTool{ + readFileBase: readFileBase{ + fs: buildFs(workspace, restrict, patterns), + maxSize: maxSize, + }, } } @@ -279,8 +327,16 @@ func (t *ReadFileTool) Name() string { return "read_file" } +func (t *ReadFileLinesTool) Name() string { + return "read_file" +} + func (t *ReadFileTool) Description() string { - return "Read the contents of a file. Supports pagination via `offset` and `length`." + return "Read the contents of a file using byte-based pagination via `offset` and `length`." +} + +func (t *ReadFileLinesTool) Description() string { + return "Read file contents from the filesystem. Output always includes line numbers in the format `LINE_NUMBER|LINE_CONTENT` (1-indexed). Supports partial reads via `offset` and `limit` for large text files." } func (t *ReadFileTool) Parameters() map[string]any { @@ -306,13 +362,142 @@ func (t *ReadFileTool) Parameters() map[string]any { } } +func (t *ReadFileLinesTool) Parameters() map[string]any { + return map[string]any{ + "type": "object", + "properties": map[string]any{ + "path": map[string]any{ + "type": "string", + "description": "Path to the file to read.", + }, + "offset": map[string]any{ + "type": "integer", + "description": "Starting line number (1-indexed). Use for large files to read from a specific line.", + "default": 1, + }, + "limit": map[string]any{ + "type": "integer", + "description": "Number of lines to read. Use with offset for large files to read in chunks.", + }, + }, + "required": []string{"path"}, + } +} + func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolResult { path, ok := args["path"].(string) if !ok { return ErrorResult("path is required") } - // offset (optional, default 0) + data, err := t.fs.ReadFile(path) + if err != nil { + return ErrorResult(err.Error()) + } + + return t.executeByteRead(path, data, args) +} + +func (t *ReadFileLinesTool) Execute(ctx context.Context, args map[string]any) *ToolResult { + path, ok := args["path"].(string) + if !ok { + return ErrorResult("path is required") + } + + data, err := t.fs.ReadFile(path) + if err != nil { + return ErrorResult(err.Error()) + } + if isBinaryReadFileData(data) { + return ErrorResult(`file appears to be binary`) + } + + return t.executeLineRead(path, data, args) +} + +func (t *readFileBase) executeLineRead(path string, data []byte, args map[string]any) *ToolResult { + offset, err := getInt64Arg(args, "offset", 1) + if err != nil { + return ErrorResult(err.Error()) + } + if offset < 1 { + return ErrorResult("offset must be >= 1") + } + + limit := int64(-1) + if _, exists := args["limit"]; exists { + limit, err = getInt64Arg(args, "limit", -1) + if err != nil { + return ErrorResult(err.Error()) + } + if limit <= 0 { + return ErrorResult("limit must be > 0") + } + } + + lines := splitLinesPreserveNewlines(string(data)) + totalLines := int64(len(lines)) + if totalLines == 0 { + return NewToolResult("[END OF FILE - no content at this offset]") + } + + startIdx := offset - 1 + if startIdx >= totalLines { + return NewToolResult("[END OF FILE - no content at this offset]") + } + + endExclusive := totalLines + if limit > 0 { + endExclusive = startIdx + limit + if endExclusive > totalLines { + endExclusive = totalLines + } + } + + selectedLines := lines[startIdx:endExclusive] + if len(selectedLines) == 0 { + return NewToolResult("[END OF FILE - no content at this offset]") + } + + formatted := make([]string, 0, len(lines)) + for _, line := range lines { + // Remove only final carriage returns to normalize + lineContent := strings.TrimRight(line, "\r\n") + formatted = append(formatted, lineContent) + } + + content := strings.Join(formatted, "\n") + maxTokens := t.maxTokenBudget() + content = truncateReadFileContent(content, maxTokens) + + readEndLine := startIdx + int64(len(selectedLines)) + readRange := fmt.Sprintf("lines %d-%d", offset, readEndLine) + + displayPath := filepath.Base(path) + header := fmt.Sprintf( + "[file: %s | total: %d lines | read: %s]", + displayPath, totalLines, readRange, + ) + + hasMore := endExclusive < totalLines + if hasMore { + header += fmt.Sprintf( + "\n[TRUNCATED - file has more content. Call read_file again with offset=%d to continue.]", + readEndLine+1, + ) + } + + logger.DebugCF("tool", "ReadFileTool execution completed successfully", + map[string]any{ + "path": path, + "lines_read": len(selectedLines), + "has_more": hasMore, + }) + + return NewToolResult(header + "\n\n" + content) +} + +func (t *readFileBase) executeByteRead(path string, data []byte, args map[string]any) *ToolResult { offset, err := getInt64Arg(args, "offset", 0) if err != nil { return ErrorResult(err.Error()) @@ -321,7 +506,6 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe return ErrorResult("offset must be >= 0") } - // length (optional, capped at MaxReadFileSize) length, err := getInt64Arg(args, "length", t.maxSize) if err != nil { return ErrorResult(err.Error()) @@ -333,105 +517,28 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe length = t.maxSize } - file, err := t.fs.Open(path) - if err != nil { - return ErrorResult(err.Error()) - } - defer file.Close() - - // measure total size - totalSize := int64(-1) // -1 means unknown - if info, statErr := file.Stat(); statErr == nil { - totalSize = info.Size() - } - - // 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 { - _, err = seeker.Seek(0, io.SeekStart) - if 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 { - _, 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 { - // Fallback for non-seekable streams: discard leading bytes. - // sniffN bytes were already consumed above, so subtract them. - remaining := offset - int64(sniffN) - if remaining > 0 { - _, err = io.CopyN(io.Discard, file, remaining) - if err != nil { - return ErrorResult(fmt.Sprintf("failed to advance to offset %d: %v", offset, err)) - } - } - } - - // 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)) - } - - // hasMore is true only when we actually got the extra probe byte. - hasMore := int64(n) > length - data := probe[:min(int64(n), length)] - - if len(data) == 0 { + totalSize := int64(len(data)) + if offset >= totalSize { 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]", - displayPath, totalSize, readRange, - ) - } else { - header = fmt.Sprintf( - "[file: %s | read: %s | total size unknown]", - displayPath, readRange, - ) + end := offset + length + if end > totalSize { + end = totalSize } + chunk := data[offset:end] + readRange := fmt.Sprintf("bytes %d-%d", offset, end-1) + displayPath := filepath.Base(path) + header := fmt.Sprintf( + "[file: %s | total: %d bytes | read: %s]", + displayPath, totalSize, readRange, + ) + hasMore := end < totalSize if hasMore { header += fmt.Sprintf( "\n[TRUNCATED - file has more content. Call read_file again with offset=%d to continue.]", - readEnd, + end, ) } else { header += "\n[END OF FILE - no further content.]" @@ -440,11 +547,191 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe logger.DebugCF("tool", "ReadFileTool execution completed successfully", map[string]any{ "path": path, - "bytes_read": len(data), + "bytes_read": len(chunk), "has_more": hasMore, + "mode": "bytes", }) - return NewToolResult(header + "\n\n" + string(data)) + return NewToolResult(header + "\n\n" + string(chunk)) +} + +func (t *readFileBase) maxTokenBudget() int { + if t.maxSize <= 0 { + return MaxReadFileSize / 4 + } + + budget := int(t.maxSize / 4) + if budget <= 0 { + return 1 + } + return budget +} + +func splitLinesPreserveNewlines(text string) []string { + if text == "" { + return nil + } + + lines := strings.SplitAfter(text, "\n") + if len(lines) > 0 && lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] + } + return lines +} + +func isBinaryReadFileData(data []byte) bool { + if len(data) == 0 { + return false + } + + sample := data + if len(sample) > 512 { + sample = sample[:512] + } + + if bytes.IndexByte(sample, 0) >= 0 { + return true + } + + contentType := http.DetectContentType(sample) + if strings.HasPrefix(contentType, "text/") { + return false + } + if strings.HasSuffix(contentType, "/json") || + strings.HasSuffix(contentType, "+json") || + strings.HasSuffix(contentType, "/xml") || + strings.HasSuffix(contentType, "+xml") || + strings.Contains(contentType, "javascript") { + return false + } + + if !utf8.Valid(sample) { + return true + } + + controlChars := 0 + for _, b := range sample { + if b < 0x20 && b != '\n' && b != '\r' && b != '\t' && b != '\f' && b != '\b' { + controlChars++ + } + } + + return float64(controlChars)/float64(len(sample)) > 0.1 +} + +func truncateReadFileContent(text string, maxTokens int) string { + if strings.TrimSpace(text) == "" || maxTokens <= 0 { + return text + } + + tokenCount := routing.EstimateTokens(text) + if tokenCount <= maxTokens { + return text + } + + totalChars := utf8.RuneCountInString(text) + if totalChars == 0 { + return text + } + + tokenPerChar := float64(tokenCount) / float64(totalChars) + if tokenPerChar <= 0 { + return text + } + + allowedChars := int(float64(maxTokens)/tokenPerChar*readFileTruncationSafetyMargin + 0.5) + if allowedChars < 2 { + allowedChars = 2 + } + if allowedChars >= totalChars { + return text + } + + headChars := allowedChars / 2 + tailChars := allowedChars - headChars + + headRaw := firstNRunes(text, headChars) + tailRaw := lastNRunes(text, tailChars) + + head := trimHeadToLineBoundary(headRaw) + tail := trimTailToLineBoundary(tailRaw) + if head == "" { + head = strings.TrimRight(headRaw, "\n") + } + if tail == "" { + tail = strings.TrimLeft(tailRaw, "\n") + } + + notice := fmt.Sprintf( + "\n\n... [Content truncated: %d tokens -> ~%d tokens limit] ...\n\n", + tokenCount, + maxTokens, + ) + + switch { + case head == "" && tail == "": + return notice + case head == "": + return notice + tail + case tail == "": + return head + notice + default: + return head + notice + tail + } +} + +func firstNRunes(text string, n int) string { + if n <= 0 { + return "" + } + if utf8.RuneCountInString(text) <= n { + return text + } + + var builder strings.Builder + builder.Grow(n) + + count := 0 + reader := bufio.NewReader(strings.NewReader(text)) + for count < n { + r, _, err := reader.ReadRune() + if err != nil { + break + } + builder.WriteRune(r) + count++ + } + + return builder.String() +} + +func lastNRunes(text string, n int) string { + if n <= 0 { + return "" + } + + runes := []rune(text) + if len(runes) <= n { + return text + } + + return string(runes[len(runes)-n:]) +} + +func trimHeadToLineBoundary(text string) string { + idx := strings.LastIndex(text, "\n") + if idx < 0 { + return strings.TrimRight(text, "\n") + } + return strings.TrimRight(text[:idx], "\n") +} + +func trimTailToLineBoundary(text string) string { + idx := strings.Index(text, "\n") + if idx < 0 { + return strings.TrimLeft(text, "\n") + } + return strings.TrimLeft(text[idx+1:], "\n") } // getInt64Arg extracts an integer argument from the args map, returning the diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 0b4dd310b..aec99e50a 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "regexp" + "strconv" "strings" "testing" @@ -18,7 +19,7 @@ func TestFilesystemTool_ReadFile_Success(t *testing.T) { testFile := filepath.Join(tmpDir, "test.txt") os.WriteFile(testFile, []byte("test content"), 0o644) - tool := NewReadFileTool("", false, MaxReadFileSize) + tool := NewReadFileBytesTool("", false, MaxReadFileSize) ctx := context.Background() args := map[string]any{ "path": testFile, @@ -45,7 +46,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, MaxReadFileSize) + tool := NewReadFileBytesTool("", false, MaxReadFileSize) ctx := context.Background() args := map[string]any{ "path": "/nonexistent_file_12345.txt", @@ -59,7 +60,7 @@ 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 read") { + if !strings.Contains(result.ForLLM, "failed to read file") && !strings.Contains(result.ForUser, "failed to read") { t.Errorf("Expected error message, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser) } } @@ -721,26 +722,32 @@ func TestWhitelistFs_AllowsResolvedAllowedRootAlias(t *testing.T) { } // TestReadFileTool_ChunkedReading verifies the pagination logic of the tool -// by reading a file in multiple chunks using 'offset' and 'length'. +// by reading a file in multiple chunks using 1-indexed line offset and limit. func TestReadFileTool_ChunkedReading(t *testing.T) { tmpDir := t.TempDir() testFile := filepath.Join(tmpDir, "pagination_test.txt") - // Create a test file with exactly 26 bytes of content - fullContent := "abcdefghijklmnopqrstuvwxyz" + fullContent := strings.Join([]string{ + "line 1", + "line 2", + "line 3", + "line 4", + "line 5", + "line 6", + }, "\n") + "\n" err := os.WriteFile(testFile, []byte(fullContent), 0o644) if err != nil { t.Fatalf("Failed to write test file: %v", err) } - tool := NewReadFileTool(tmpDir, false, MaxReadFileSize) + tool := NewReadFileLinesTool(tmpDir, false, MaxReadFileSize) ctx := context.Background() - // --- Step 1: Read the first chunk (10 bytes) --- + // --- Step 1: Read the first chunk (2 lines) --- args1 := map[string]any{ "path": testFile, - "offset": 0, - "length": 10, + "offset": 1, + "limit": 2, } result1 := tool.Execute(ctx, args1) @@ -748,24 +755,24 @@ func TestReadFileTool_ChunkedReading(t *testing.T) { 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) + if !strings.Contains(result1.ForLLM, " 1|line 1\n 2|line 2") { + t.Errorf("Chunk 1 should contain numbered first two lines, 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) + if !strings.Contains(result1.ForLLM, "offset=3") { + t.Errorf("Chunk 1 header should suggest next offset=3, got: %s", result1.ForLLM) + } + if !strings.Contains(result1.ForLLM, "read: lines 1-2") { + t.Errorf("Chunk 1 header should report line range 1-2, got: %s", result1.ForLLM) } - // Step 2: Read the second chunk (10 bytes) --- + // Step 2: Read the second chunk (2 lines) --- args2 := map[string]any{ "path": testFile, - "offset": 10, - "length": 10, + "offset": 3, + "limit": 2, } result2 := tool.Execute(ctx, args2) @@ -773,21 +780,18 @@ func TestReadFileTool_ChunkedReading(t *testing.T) { 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) + if !strings.Contains(result2.ForLLM, " 3|line 3\n 4|line 4") { + t.Errorf("Chunk 2 should contain numbered lines 3-4, 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) + if !strings.Contains(result2.ForLLM, "offset=5") { + t.Errorf("Chunk 2 header should suggest next offset=5, 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 + // Step 3: Read the final chunk (remaining 2 lines) --- args3 := map[string]any{ "path": testFile, - "offset": 20, - "length": 10, + "offset": 5, + "limit": 2, } result3 := tool.Execute(ctx, args3) @@ -795,39 +799,34 @@ func TestReadFileTool_ChunkedReading(t *testing.T) { 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) + if !strings.Contains(result3.ForLLM, " 5|line 5\n 6|line 6") { + t.Errorf("Chunk 3 should contain numbered lines 5-6, 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. +// a starting line that exceeds the total number of lines. 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) + err := os.WriteFile(testFile, []byte("line 1\nline 2\n"), 0o644) if err != nil { t.Fatalf("Failed to write test file: %v", err) } - tool := NewReadFileTool(tmpDir, false, MaxReadFileSize) + tool := NewReadFileLinesTool(tmpDir, false, MaxReadFileSize) ctx := context.Background() args := map[string]any{ "path": testFile, - "offset": int64(100), // Offset beyond the end of the file + "offset": int64(100), // Line offset beyond the end of the file } result := tool.Execute(ctx, args) @@ -843,3 +842,110 @@ func TestReadFileTool_OffsetBeyondEOF(t *testing.T) { t.Errorf("The message %q was expected, obtained: %q", expectedMsg, result.ForLLM) } } + +func TestReadFileTool_DefaultOffsetAndRemainingLines(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "default_lines.txt") + + err := os.WriteFile(testFile, []byte("line 1\nline 2\nline 3\n"), 0o644) + if err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + tool := NewReadFileLinesTool(tmpDir, false, MaxReadFileSize) + result := tool.Execute(context.Background(), map[string]any{"path": testFile}) + if result.IsError { + t.Fatalf("Execute() error = %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, " 1|line 1\n 2|line 2\n 3|line 3") { + t.Fatalf("expected numbered remaining lines by default, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "read: lines 1-3") { + t.Fatalf("expected line range 1-3, got: %s", result.ForLLM) + } +} + +func TestReadFileTool_LegacyLengthUsesByteModeForText(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "legacy_bytes.txt") + + err := os.WriteFile(testFile, []byte("abcdefghijklmnopqrstuvwxyz"), 0o644) + if err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + tool := NewReadFileBytesTool(tmpDir, false, MaxReadFileSize) + result := tool.Execute(context.Background(), map[string]any{ + "path": testFile, + "offset": 10, + "length": 5, + }) + if result.IsError { + t.Fatalf("Execute() error = %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "read: bytes 10-14") { + t.Fatalf("expected byte-based header, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "klmno") { + t.Fatalf("expected byte chunk content, got: %s", result.ForLLM) + } + if strings.Contains(result.ForLLM, "lines ") { + t.Fatalf("expected legacy byte mode, got line-based header: %s", result.ForLLM) + } +} + +func TestReadFileLinesTool_BinaryFileRejected(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "binary.dat") + + data := []byte{0x00, 0x01, 'A', 'B', 'C', 'D', 'E', 'F'} + err := os.WriteFile(testFile, data, 0o644) + if err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + tool := NewReadFileLinesTool(tmpDir, false, MaxReadFileSize) + result := tool.Execute(context.Background(), map[string]any{"path": testFile}) + if !result.IsError { + t.Fatalf("expected binary file rejection in line mode, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, `switch tools.read_file.mode to "bytes"`) { + t.Fatalf("expected mode-switch guidance, got: %s", result.ForLLM) + } +} + +func TestReadFileTool_TruncatesLargeContentByEstimatedTokens(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "long.txt") + + var builder strings.Builder + for i := 1; i <= 40; i++ { + builder.WriteString(strings.Repeat("line-content-", 8)) + builder.WriteString(strconv.Itoa(i)) + builder.WriteString("\n") + } + + err := os.WriteFile(testFile, []byte(builder.String()), 0o644) + if err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + tool := NewReadFileLinesTool(tmpDir, false, 160) // ~40 token output budget + result := tool.Execute(context.Background(), map[string]any{"path": testFile}) + if result.IsError { + t.Fatalf("Execute() error = %s", result.ForLLM) + } + + if !strings.Contains(result.ForLLM, "... [Content truncated:") { + t.Fatalf("expected truncation notice, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "-> ~40 tokens limit") { + t.Fatalf("expected token limit notice derived from config, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, " 1|line-content-line-content-") { + t.Fatalf("expected head of content to remain visible, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "line-content-line-content-40") { + t.Fatalf("expected tail of content to remain visible, got: %s", result.ForLLM) + } +}