Compare commits

..
Author SHA1 Message Date
Vasanthdev2004 55d7cc37ef fix(mcp): do not promise a retry returns the same content
The dropped-block note said "Retrying will return the same thing".
registryTool.Run makes a fresh remote call on every retry, so the server
may well answer differently; that is a promise this code is in no
position to make.

What cannot change is that Zero still has nowhere to put a non-text
block, so the note now says retrying cannot RECOVER the payload. Same
guidance, a claim that is actually true.

The test pins both directions: the new wording must be present, and the
absolute claim must not come back.

Raised by CodeRabbit on #874.
2026-08-07 21:03:51 +05:30
Vasanthdev2004 af45b1a0d5 fix(mcp): name the non-text blocks a tool result drops
TextContent keeps only blocks whose type is "text", so a server that
returns an image, audio, or an embedded resource produced "(empty MCP
tool result)". A screenshot or browser server would complete a call
successfully, hand back a perfectly good image, and the model would be
told nothing came back. It then usually retries, which spends another
call to reach the same empty answer, and the user never learns an image
existed at all.

Carrying the payload is the larger half of #823 and is not this change.
Dropping silently is the part that causes the retry loop, and it can be
fixed on its own.

Decode mimeType on Content, and add DroppedContentSummary, which
describes what TextContent discarded ("1 image/png block", "2 resource
blocks, 1 audio/wav block"), grouping by mime type and ordering by first
appearance so the same result always reads the same way. Servers that
omit mimeType fall back to the block type rather than printing an empty
label.

registryTool.Run appends that as a note, and only when something was
actually dropped, so a text-only result is byte-for-byte unchanged. The
note also says retrying will return the same thing, since telling the
model what happened without telling it not to retry only fixes half the
loop.

Refs #823.
2026-08-06 21:57:53 +05:30
21 changed files with 307 additions and 741 deletions

No files matched your search

-36
View File
@@ -67,39 +67,3 @@ requirements. If a related check fails, fix it. If a required check cannot run
because of the environment or fails for an unrelated external reason, report
the exact failure and obtain maintainer direction rather than silently ignoring
it.
## 4. Common Review Blockers
These classes drive multi-round reviews. Fix them before requesting review:
- **Fresh base:** Rebase onto the current PR base (`main` or stacked target)
before review. A stale head that rolls back mainline commits is a hard
blocker, not a merge-time detail. Resolve conflicts by keeping upstream
behavior unless the PR intentionally changes it.
- **Platform truth:** Code and tests must pass on Linux, macOS, and Windows.
Compare paths after canonicalization (physical/`EvalSymlinks` form); never
assert raw `t.TempDir()` spellings (`/var` vs `/private/var` on macOS, short
paths on Windows). Prefer one cross-platform function with small `GOOS`
checks over duplicated helpers that drift.
- **Security edges:** Keep secrets out of argv, env dumps, and logs. Redact
success and error paths (including stderr). Fail closed on ownership, lease,
and permission checks. Do not rely on pre-open path resolution
(`EvalSymlinks` then open) for containment: that is a check-to-use race.
Bind containment at open/use time with rooted or handle-relative,
traversal-resistant APIs. If a no-follow API is used, apply it to every
traversed component and enforce the platform's reparse-point protections;
final-component-only no-follow is insufficient. On multi-step setup, roll back
only what this run created; never destroy pre-existing resources you did
not create; never report success when cleanup or unlock failed.
- **Atomic shared state:** Write a complete temporary file, then atomically
replace the destination so concurrent readers never see a partial write.
Exclusive create or a write lock alone is not enough for readers. Serialize
the full read-modify-write sequence for lockfiles and shared stores.
- **Tests match the claim:** Every behavior or security-boundary change needs a
regression test, including the failure path. Path-sensitive logic needs at
least one non-Linux path case (or a hermetic fake that exercises the same
normalization).
- **Honest scope:** PR description, help text, and comments must match what
shipped. Wire advertised entry points or shrink the claim. Do not bundle
unrelated fixes. Do not widen security allowlists by name alone; check
classification and side effects too.
+7 -13
View File
@@ -30,13 +30,11 @@ work.
read-before-edit discipline: inspect the target file and nearby callers,
tests, or config before you modify behavior. Never edit a file you have not
read.
2. **Plan.** Use update_plan for implementation or investigation that has at
least three meaningful dependent steps, then keep it current at milestones.
Mark completed work and the next in-progress step together when practical;
do not spend separate turns updating it after every file or command. Keep at
most one item in_progress. Skip plans for simple lookups, explanations, code
navigation, and other short tasks. Never create a plan after the work is
already complete merely to describe what you did.
2. **Plan.** For multi-step work, call update_plan with an ordered checklist and
keep it current at meaningful milestones. Mark completed work and the next
in-progress step together when practical; do not spend separate model turns
updating the plan after every file or command. Keep at most one item
in_progress. Skip the plan for trivial one-step tasks.
3. **Implement.** Make focused changes that match the surrounding code's style,
naming, and conventions. Prefer the smallest change that fully solves the
problem. Avoid broad refactors, unrelated rewrites, dependency churn, and
@@ -51,13 +49,9 @@ work.
## Editing discipline
- Choose the narrowest tool that safely accomplishes the step. Prefer native
file tools - read_file, read_minified_file, list_directory, glob, grep,
write_file, edit_file, apply_patch - over shelling out to
cat/sed/awk/python for file operations.
file tools - read_file, list_directory, glob, grep, write_file, edit_file,
apply_patch - over shelling out to cat/sed/awk/python for file operations.
They are safer, reviewable, and produce clean diffs.
- Prefer read_minified_file when initially exploring source code; it preserves
code while removing comments and redundant whitespace. Use read_file when
exact text, comments, or line numbers are needed.
- Keep edits focused and reviewable. A single patch may update several related
files when they form one coherent change; do not hide unrelated edits in a
bulk shell or script rewrite.
+1 -2
View File
@@ -71,6 +71,5 @@ const geminiPromptAddendum = `<model_guidance>
equivalent shell commands; they are safer and produce cleaner diffs.
- Be concise and concrete. When you run a shell command with side effects, state
in one short clause why it is needed.
- Use update_plan only for work with at least three meaningful dependent steps;
skip it for simple lookups and never create one after the work is complete.
- Use update_plan for any multi-step task and keep it current.
</model_guidance>`
+2 -2
View File
@@ -79,8 +79,8 @@ func TestRegisterToolSearchIfEligibleSkipsWhenThresholdZero(t *testing.T) {
func TestDeferredEligibleCountIncludesOptionalBuiltins(t *testing.T) {
registry := newCoreRegistry(t.TempDir())
if got := deferredEligibleCount(registry, agent.PermissionModeAuto, nil, nil); got < 2 {
t.Fatalf("deferredEligibleCount(core) = %d, want at least 2 optional built-ins", got)
if got := deferredEligibleCount(registry, agent.PermissionModeAuto, nil, nil); got < 3 {
t.Fatalf("deferredEligibleCount(core) = %d, want at least 3 optional built-ins", got)
}
}
+53
View File
@@ -26,6 +26,10 @@ type RemoteTool struct {
type Content struct {
Type string `json:"type"`
Text string `json:"text,omitempty"`
// MimeType names what a non-text block holds. Decoded but not yet forwarded:
// it is what lets a dropped block be described to the model instead of
// vanishing (#823). Servers that omit it still decode fine.
MimeType string `json:"mimeType,omitempty"`
}
type CallToolResult struct {
@@ -495,3 +499,52 @@ func TextContent(content []Content) string {
}
return strings.TrimSpace(strings.Join(parts, "\n"))
}
// DroppedContentSummary describes the blocks TextContent discards, e.g.
// "1 image/png block" or "2 resource blocks, 1 audio/wav block". It returns ""
// when a result is entirely text, so a caller adds nothing to the ordinary case.
//
// This exists because dropping silently is the worst available behaviour. A
// screenshot server returns a valid image, TextContent keeps nothing, and the
// call is reported as "(empty MCP tool result)" — so the model concludes the
// tool produced nothing and usually retries, burning another call on the same
// empty answer. Naming what came back costs nothing and ends that loop even
// though the payload still cannot be forwarded.
//
// Counts are grouped by mime type and ordered by first appearance, so the same
// result always produces the same sentence.
func DroppedContentSummary(content []Content) string {
labels := make([]string, 0, len(content))
counts := make(map[string]int, len(content))
for _, item := range content {
if item.Type == "text" {
continue
}
// Prefer the mime type: "image/png" tells the reader more than "image".
// A server may omit it, so fall back to the block type rather than
// printing an empty label.
label := strings.TrimSpace(item.MimeType)
if label == "" {
label = strings.TrimSpace(item.Type)
}
if label == "" {
label = "unknown"
}
if _, seen := counts[label]; !seen {
labels = append(labels, label)
}
counts[label]++
}
if len(labels) == 0 {
return ""
}
parts := make([]string, 0, len(labels))
for _, label := range labels {
part := fmt.Sprintf("%d %s block", counts[label], label)
if counts[label] != 1 {
part += "s"
}
parts = append(parts, part)
}
return strings.Join(parts, ", ")
}
+158
View File
@@ -0,0 +1,158 @@
package mcp
import (
"context"
"strings"
"testing"
"github.com/Gitlawb/zero/internal/tools"
)
// A server that returns only an image currently reports "(empty MCP tool
// result)": TextContent keeps text blocks and drops the rest, so a successful
// call looks like it produced nothing. The model then usually retries, which is
// the worst outcome, and the user is never told an image existed (#823).
//
// Carrying the payload is a separate change. Naming what was dropped is what
// stops the retry loop, and it has to be true of the DELIVERED result, so this
// drives registryTool.Run rather than the helper alone.
func TestAnImageOnlyResultSaysWhatItReturned(t *testing.T) {
tool := registryTool{
client: &nonTextClient{content: []Content{
{Type: "image", MimeType: "image/png"},
}},
server: Server{Name: "shots"},
remote: RemoteTool{Name: "screenshot"},
}
result := tool.Run(context.Background(), map[string]any{})
if strings.Contains(result.Output, "(empty MCP tool result)") {
t.Fatalf("an image-only result still reports empty, so the model will retry:\n%s", result.Output)
}
if !strings.Contains(result.Output, "image/png") {
t.Errorf("the output does not name what the server returned:\n%s", result.Output)
}
// Naming the block is only half of it. Without the guidance the model still
// retries, which is the expensive symptom, so the wording is pinned too.
//
// "cannot recover this payload" and not "will return the same thing": every
// retry is a fresh call and the server may answer differently. What cannot
// change is that Zero has nowhere to put a non-text block.
if !strings.Contains(result.Output, "Retrying cannot recover this payload.") {
t.Errorf("the output does not tell the model retrying is pointless:\n%s", result.Output)
}
if strings.Contains(result.Output, "will return the same thing") {
t.Errorf("the output promises an identical response, which a fresh call cannot guarantee:\n%s", result.Output)
}
if result.Status != tools.StatusOK {
t.Errorf("status = %v, want OK: the call succeeded, we just cannot forward the payload", result.Status)
}
}
// The quieter half of the same bug: when a result carries text AND an image,
// the text arrives and the image vanishes with no mention at all.
func TestTextAlongsideAnImageStillReportsTheImage(t *testing.T) {
tool := registryTool{
client: &nonTextClient{content: []Content{
{Type: "text", Text: "captured the page"},
{Type: "image", MimeType: "image/png"},
}},
server: Server{Name: "shots"},
remote: RemoteTool{Name: "screenshot"},
}
result := tool.Run(context.Background(), map[string]any{})
if !strings.Contains(result.Output, "captured the page") {
t.Errorf("the text block was lost:\n%s", result.Output)
}
if !strings.Contains(result.Output, "image/png") {
t.Errorf("the dropped image was not mentioned:\n%s", result.Output)
}
}
// A text-only result must be byte-for-byte what it was before: this change adds
// a line only when something was actually dropped.
func TestATextOnlyResultIsUnchanged(t *testing.T) {
tool := registryTool{
client: &nonTextClient{content: []Content{{Type: "text", Text: "plain answer"}}},
server: Server{Name: "shots"},
remote: RemoteTool{Name: "lookup"},
}
if got := tool.Run(context.Background(), map[string]any{}).Output; got != "plain answer" {
t.Fatalf("output = %q, want exactly %q", got, "plain answer")
}
}
func TestDroppedContentSummaryNamesTheBlocks(t *testing.T) {
tests := []struct {
name string
content []Content
want string
}{
{
name: "nothing dropped",
content: []Content{{Type: "text", Text: "hi"}},
want: "",
},
{
name: "no content at all",
content: nil,
want: "",
},
{
name: "one image with a mime type",
content: []Content{{Type: "image", MimeType: "image/png"}},
want: "1 image/png block",
},
{
// A server may omit mimeType. Fall back to the block type rather than
// inventing one or printing an empty pair of slashes.
name: "one image without a mime type",
content: []Content{{Type: "image"}},
want: "1 image block",
},
{
name: "several of the same kind are counted, not repeated",
content: []Content{
{Type: "image", MimeType: "image/png"},
{Type: "image", MimeType: "image/png"},
},
want: "2 image/png blocks",
},
{
// Order follows first appearance so the message is stable to read and
// to assert on.
name: "mixed kinds",
content: []Content{
{Type: "text", Text: "ignored here"},
{Type: "resource"},
{Type: "audio", MimeType: "audio/wav"},
{Type: "resource"},
},
want: "2 resource blocks, 1 audio/wav block",
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
if got := DroppedContentSummary(test.content); got != test.want {
t.Fatalf("DroppedContentSummary() = %q, want %q", got, test.want)
}
})
}
}
type nonTextClient struct {
content []Content
}
func (client *nonTextClient) ListTools(context.Context) ([]RemoteTool, error) { return nil, nil }
func (client *nonTextClient) CallTool(context.Context, string, map[string]any) (CallToolResult, error) {
return CallToolResult{Content: client.content}, nil
}
func (client *nonTextClient) Close() error { return nil }
+18
View File
@@ -325,6 +325,24 @@ func (tool registryTool) Run(ctx context.Context, args map[string]any) tools.Res
status = tools.StatusError
}
output := TextContent(result.Content)
// Say what was thrown away. Without this an image-only result reads as
// "(empty MCP tool result)", the model concludes the call produced nothing
// and retries, and the user never learns an image came back (#823). The note
// is appended only when something was actually dropped, so a text-only
// result is byte-for-byte what it was before.
//
// It says retrying cannot RECOVER the payload rather than that a retry
// returns the same thing. Each retry is a fresh call, so the server may well
// answer differently; what cannot change is that Zero still has nowhere to
// put a non-text block. Claiming the response would be identical would be a
// promise this code is in no position to make.
if dropped := DroppedContentSummary(result.Content); dropped != "" {
note := "[zero] this server also returned " + dropped + ", which Zero cannot forward yet. Retrying cannot recover this payload."
if output == "" {
note = "[zero] this server returned " + dropped + ", which Zero cannot forward yet. Retrying cannot recover this payload."
}
output = strings.TrimSpace(output + "\n\n" + note)
}
if output == "" {
output = "(empty MCP tool result)"
}
+2 -134
View File
@@ -13,10 +13,8 @@ package minify
import (
"bytes"
"go/ast"
"go/parser"
"go/printer"
"go/scanner"
"go/token"
"path/filepath"
"strings"
@@ -36,11 +34,8 @@ func File(path string, content []byte) Result {
if out, ok := minifyGo(content); ok {
return Result{Content: out, Language: "go", Applied: true}
}
if out, ok := minifyGoFragment(content); ok {
return Result{Content: out, Language: "go-fragment", Applied: true}
}
// An incomplete fragment that cannot be wrapped safely falls through to
// whitespace-only normalization; no source text is guessed or discarded.
// Unparsable Go (a snippet, generics edge, or syntax error): fall through
// to the safe generic path rather than risk a partial AST reprint.
} else if style, ok := commentStyles[ext]; ok {
// Strip comments with a string-aware lexer, then collapse the whitespace the
// removed comments left behind. The stripper only handles languages whose
@@ -51,133 +46,6 @@ func File(path string, content []byte) Result {
return Result{Content: minifyGeneric(content), Language: "text", Applied: false}
}
// Fragment minifies a bounded source range without assuming lexical state from
// text outside the range. Go remains parser-validated; other languages use the
// conservative whitespace-only transform because a range may begin inside a
// multiline string or block comment.
func Fragment(path string, content []byte) Result {
if strings.EqualFold(filepath.Ext(path), ".go") {
if out, ok := minifyGo(content); ok {
return Result{Content: out, Language: "go", Applied: true}
}
if out, ok := minifyGoFragment(content); ok {
return Result{Content: out, Language: "go-fragment", Applied: true}
}
}
return Result{Content: minifyGeneric(content), Language: "text", Applied: false}
}
// ContextualFragment minifies a bounded fragment after checking its starting
// position against the complete source. A Go range that begins inside a string
// or comment must remain verbatim apart from whitespace normalization: parsing
// that fragment by itself would mistake literal text for Go syntax.
func ContextualFragment(path string, source, content []byte, startOffset int) Result {
if strings.EqualFold(filepath.Ext(path), ".go") && goFragmentStartsInsideToken(source, startOffset) {
return Result{Content: minifyGeneric(content), Language: "text", Applied: false}
}
return Fragment(path, content)
}
func goFragmentStartsInsideToken(source []byte, startOffset int) bool {
if startOffset <= 0 || startOffset >= len(source) {
return false
}
fset := token.NewFileSet()
file := fset.AddFile("", fset.Base(), len(source))
var lexer scanner.Scanner
lexer.Init(file, source, nil, scanner.ScanComments)
for {
position, kind, literal := lexer.Scan()
if kind == token.EOF {
return false
}
if kind != token.STRING && kind != token.COMMENT {
continue
}
start := file.Offset(position)
if startOffset > start && startOffset < start+len(literal) {
return true
}
}
}
// minifyGoFragment handles bounded reads that do not contain a package clause.
// It finds the longest complete declaration or statement prefix that the real
// Go parser accepts, prints that prefix without comments, and preserves any
// incomplete tail conservatively. Ranges are intentionally capped: repeatedly
// parsing a large broken file would otherwise turn a cheap read into O(n²) work.
func minifyGoFragment(content []byte) (string, bool) {
lines := strings.Split(strings.ReplaceAll(string(content), "\r\n", "\n"), "\n")
if len(lines) > 1 && lines[len(lines)-1] == "" {
lines = lines[:len(lines)-1]
}
if len(lines) > 512 {
return "", false
}
for end := len(lines); end > 0; end-- {
prefix := strings.Join(lines[:end], "\n")
if compact, ok := parseGoDeclarations(prefix); ok {
return joinCompactPrefix(compact, lines[end:]), true
}
if compact, ok := parseGoStatements(prefix); ok {
return joinCompactPrefix(compact, lines[end:]), true
}
}
return "", false
}
func parseGoDeclarations(fragment string) (string, bool) {
fset := token.NewFileSet()
file, err := parser.ParseFile(fset, "", "package compact\n"+fragment, parser.SkipObjectResolution)
if err != nil || len(file.Decls) == 0 {
return "", false
}
nodes := make([]ast.Node, len(file.Decls))
for i, declaration := range file.Decls {
nodes[i] = declaration
}
return printGoNodes(fset, nodes)
}
func parseGoStatements(fragment string) (string, bool) {
fset := token.NewFileSet()
file, err := parser.ParseFile(fset, "", "package compact\nfunc _(){\n"+fragment+"\n}", parser.SkipObjectResolution)
if err != nil || len(file.Decls) != 1 {
return "", false
}
function, ok := file.Decls[0].(*ast.FuncDecl)
if !ok || function.Body == nil || len(function.Body.List) == 0 {
return "", false
}
nodes := make([]ast.Node, len(function.Body.List))
for i, statement := range function.Body.List {
nodes[i] = statement
}
return printGoNodes(fset, nodes)
}
func printGoNodes(fset *token.FileSet, nodes []ast.Node) (string, bool) {
var buf bytes.Buffer
cfg := printer.Config{Mode: printer.TabIndent, Tabwidth: 1}
for i, node := range nodes {
if i > 0 {
buf.WriteByte('\n')
}
if err := cfg.Fprint(&buf, fset, node); err != nil {
return "", false
}
}
return strings.TrimSpace(buf.String()), true
}
func joinCompactPrefix(compact string, remainder []string) string {
tail := minifyGeneric([]byte(strings.Join(remainder, "\n")))
if tail == "" {
return compact
}
return compact + "\n" + tail
}
// minifyGo parses Go WITHOUT comments (omitting parser.ParseComments leaves them
// unattached) and reprints the AST, yielding valid, comment-free, gofmt-shaped
// source with tab indentation (1 char per level). It returns ok=false on any
+7 -48
View File
@@ -39,58 +39,17 @@ func Greet(name string) string {
}
}
func TestMinifyGoStatementFragment(t *testing.T) {
func TestMinifyGoFallsBackOnUnparsableGo(t *testing.T) {
// A snippet (no package clause) cannot parse as a file -> safe generic path.
r := File("snippet.go", []byte("x := 1 \n\n\n// keep me\ny := 2\n"))
if !r.Applied || r.Language != "go-fragment" {
t.Fatalf("expected parsed Go fragment, got %+v", r)
if r.Applied {
t.Fatalf("expected fallback for unparsable Go, got %+v", r)
}
if strings.Contains(r.Content, "\n\n\n") {
t.Errorf("fragment should collapse blank runs:\n%q", r.Content)
t.Errorf("generic should collapse blank runs:\n%q", r.Content)
}
if strings.Contains(r.Content, "keep me") || !strings.Contains(r.Content, "y := 2") {
t.Errorf("fragment was not compacted safely: %q", r.Content)
}
}
func TestMinifyGoFragmentAccepts512LinesWithFinalNewline(t *testing.T) {
src := strings.Repeat("x++\n", 512)
r := File("snippet.go", []byte(src))
if !r.Applied || r.Language != "go-fragment" {
t.Fatalf("expected 512-line fragment to remain eligible, got %+v", r)
}
if got := strings.Count(r.Content, "x++"); got != 512 {
t.Fatalf("compacted statement count = %d, want 512", got)
}
}
func TestMinifyGoDeclarationPrefixPreservesIncompleteTail(t *testing.T) {
src := `func Handle737(n int) (int, error) {
if n%2 == 1 {
return 0, errBad
}
return n + 737, nil
}
// Handle738 begins outside the requested range.
func Handle738(n int) (int, error) {
if n%2 == 1 {`
r := File("range.go", []byte(src))
if !r.Applied || r.Language != "go-fragment" {
t.Fatalf("expected declaration-prefix compaction, got %+v", r)
}
if strings.Contains(r.Content, "Handle737 returns") || !strings.Contains(r.Content, "return n + 737") {
t.Fatalf("complete declaration was not compacted correctly:\n%s", r.Content)
}
if !strings.Contains(r.Content, "func Handle738") || !strings.Contains(r.Content, "if n%2 == 1 {") {
t.Fatalf("incomplete tail was not preserved:\n%s", r.Content)
}
}
func TestMinifyGoLargeInvalidInputFallsBackConservatively(t *testing.T) {
src := strings.Repeat("x := 1 // keep\n", 513)
r := File("broken.go", []byte(src))
if r.Applied || !strings.Contains(r.Content, "// keep") {
t.Fatalf("large invalid input must retain conservative fallback: %+v", r)
if !strings.Contains(r.Content, "// keep me") {
t.Errorf("generic must NOT strip comments (unsafe without a parser): %q", r.Content)
}
}
-35
View File
@@ -4,7 +4,6 @@ import (
"context"
"os"
"path/filepath"
"strconv"
"strings"
"testing"
)
@@ -296,40 +295,6 @@ func TestEditFileAllowsSeenRangeAndRejectsUnseenRange(t *testing.T) {
}
}
func TestCanonicalLimitedReadAuthorizesEdit(t *testing.T) {
dir := t.TempDir()
var source strings.Builder
for line := 1; line <= 1000; line++ {
source.WriteString("line ")
source.WriteString(strconv.Itoa(line))
source.WriteByte('\n')
}
path := filepath.Join(dir, "f.txt")
if err := os.WriteFile(path, []byte(source.String()), 0o644); err != nil {
t.Fatal(err)
}
registry := safetyRegistry(t, dir)
tracker := NewFileTracker()
opts := grantedOpts(tracker)
read := registry.RunWithOptions(context.Background(), "read_file", map[string]any{
"path": "f.txt", "offset": 500, "limit": 10,
}, opts)
if read.Status != StatusOK || read.Truncated {
t.Fatalf("bounded canonical read must be exact: status=%s truncated=%v meta=%#v", read.Status, read.Truncated, read.Meta)
}
if read.Meta["seen_lines"] != "500-509" {
t.Fatalf("bounded canonical read did not receive exact-read credit: %#v", read.Meta)
}
edit := registry.RunWithOptions(context.Background(), "edit_file", map[string]any{
"path": "f.txt", "old_string": "line 505", "new_string": "updated 505",
}, opts)
if edit.Status != StatusOK {
t.Fatalf("edit inside canonical read range was refused: %s", edit.Output)
}
}
func TestEditFileReplaceAllRecordsEveryShiftedReplacementSpan(t *testing.T) {
dir := t.TempDir()
path := filepath.Join(dir, "f.txt")
+2 -104
View File
@@ -38,108 +38,6 @@ func TestReadFileToolReadsLineRanges(t *testing.T) {
}
}
func TestReadFileToolReadsCanonicalLineRange(t *testing.T) {
root := t.TempDir()
writeTestFile(t, filepath.Join(root, "notes.txt"), "alpha\nbeta\ngamma\ndelta")
result := NewScopedReadFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "notes.txt",
"offset": 2,
"limit": 2,
})
if result.Status != StatusOK {
t.Fatalf("expected ok status, got %s: %s", result.Status, result.Output)
}
if result.Truncated {
t.Fatalf("canonical limit defines the requested range and must not mark it truncated: %#v", result.Meta)
}
if !strings.Contains(result.Output, "2 | beta") || !strings.Contains(result.Output, "3 | gamma") {
t.Fatalf("canonical range returned the wrong lines: %q", result.Output)
}
if strings.Contains(result.Output, "alpha") || strings.Contains(result.Output, "delta") {
t.Fatalf("canonical range leaked outside requested slice: %q", result.Output)
}
}
func TestReadFileToolCanonicalOffsetPastEnd(t *testing.T) {
root := t.TempDir()
writeTestFile(t, filepath.Join(root, "notes.txt"), "alpha\nbeta")
result := NewScopedReadFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "notes.txt", "offset": 10,
})
if result.Status != StatusOK || !strings.Contains(result.Output, "offset 10 is past the end") {
t.Fatalf("expected canonical out-of-range message, got status=%s output=%q", result.Status, result.Output)
}
if strings.Contains(result.Output, "start_line") {
t.Fatalf("out-of-range message exposed legacy argument: %q", result.Output)
}
}
func TestReadFileToolMixedLegacyRangesPreferLines(t *testing.T) {
root := t.TempDir()
writeTestFile(t, filepath.Join(root, "notes.txt"), "alpha\nbeta\ngamma\ndelta")
result := NewScopedReadFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "notes.txt",
"start_line": 2,
"end_line": 3,
"max_lines": 2,
"byte_offset": 0,
"byte_limit": readFileByteChunkMax,
})
if result.Status != StatusOK {
t.Fatalf("mixed legacy range should recover, got %s: %s", result.Status, result.Output)
}
if !strings.Contains(result.Output, "2 | beta") || !strings.Contains(result.Output, "3 | gamma") {
t.Fatalf("mixed legacy range did not prefer lines: %q", result.Output)
}
}
func TestReadFileToolCombinesLegacyStartWithCanonicalLimit(t *testing.T) {
root := t.TempDir()
writeTestFile(t, filepath.Join(root, "notes.txt"), "alpha\nbeta\ngamma\ndelta")
result := NewScopedReadFileTool(root, nil).Run(context.Background(), map[string]any{
"path": "notes.txt", "start_line": 2, "limit": 2,
})
if result.Status != StatusOK || result.Truncated {
t.Fatalf("mixed compatible range should be exact, got status=%s truncated=%v: %s", result.Status, result.Truncated, result.Output)
}
if !strings.Contains(result.Output, "2 | beta") || !strings.Contains(result.Output, "3 | gamma") {
t.Fatalf("mixed compatible range returned the wrong lines: %q", result.Output)
}
if strings.Contains(result.Output, "alpha") || strings.Contains(result.Output, "delta") {
t.Fatalf("mixed compatible range leaked outside requested slice: %q", result.Output)
}
}
func TestFileToolSchemasExposeOnlyCanonicalArguments(t *testing.T) {
readProperties := NewScopedReadFileTool(t.TempDir(), nil).Parameters().Properties
for _, want := range []string{"path", "offset", "limit", "byte_offset", "byte_limit"} {
if _, ok := readProperties[want]; !ok {
t.Fatalf("read_file schema missing %q", want)
}
}
for _, legacy := range []string{"start_line", "end_line", "max_lines"} {
if _, ok := readProperties[legacy]; ok {
t.Fatalf("read_file schema must not expose legacy argument %q", legacy)
}
}
grepProperties := NewScopedGrepTool(t.TempDir(), nil).Parameters().Properties
if _, ok := grepProperties["case_insensitive"]; !ok {
t.Fatal("grep schema missing case_insensitive")
}
if _, ok := grepProperties["-i"]; ok {
t.Fatal("grep schema must not expose the -i alias")
}
}
func TestReadFileToolMarksTruncation(t *testing.T) {
root := t.TempDir()
writeTestFile(t, filepath.Join(root, "notes.txt"), "a\nb\nc\nd\ne")
@@ -154,7 +52,7 @@ func TestReadFileToolMarksTruncation(t *testing.T) {
}
// The cut must be visible in the rendered output, not just the Truncated flag,
// and must tell the model where to continue.
if !strings.Contains(result.Output, "[truncated:") || !strings.Contains(result.Output, "offset=3") {
if !strings.Contains(result.Output, "[truncated:") || !strings.Contains(result.Output, "start_line=3") {
t.Fatalf("expected truncation marker pointing to the next line, got %q", result.Output)
}
}
@@ -170,7 +68,7 @@ func TestReadFileToolAppliesByteBudget(t *testing.T) {
if result.Status != StatusOK || !result.Truncated {
t.Fatalf("expected ok+truncated, got status=%s truncated=%v", result.Status, result.Truncated)
}
if !strings.Contains(result.Output, "output exceeded") || !strings.Contains(result.Output, "offset/limit") {
if !strings.Contains(result.Output, "output exceeded") || !strings.Contains(result.Output, "start_line") {
t.Fatalf("expected byte-budget continuation hint, got %q", result.Output[len(result.Output)-200:])
}
if result.Meta["raw_bytes"] == "" || result.Meta["emitted_bytes"] == "" {
+1 -1
View File
@@ -377,5 +377,5 @@ func fileConflictMessage(relativePath string) string {
}
func fileUnseenMessage(relativePath string) string {
return "Error writing " + relativePath + ": the intended change depends on content that has not been read exactly in this session. Read the affected lines with read_file using offset/limit (or byte_offset/byte_limit for a very long line), then retry the edit."
return "Error writing " + relativePath + ": the intended change depends on content that has not been read exactly in this session. Read the affected lines with read_file, or use byte_offset/byte_limit for an oversized single line, then retry the edit."
}
+1
View File
@@ -48,6 +48,7 @@ func NewScopedGrepTool(workspaceRoot string, scope PathScope) Tool {
"path": {Type: "string", Description: "Directory or file to search. Relative paths stay in the workspace; use an absolute path to search a granted extra root. Defaults to workspace root.", Default: "."},
"glob": {Type: "string", Description: `Optional glob filter, for example "**/*.go".`},
"output_mode": {Type: "string", Description: "Output mode.", Enum: []string{"content", "files_with_matches", "count"}, Default: "content"},
"-i": {Type: "boolean", Description: "Case insensitive search.", Default: false},
"case_insensitive": {Type: "boolean", Description: "Case insensitive search.", Default: false},
"head_limit": {Type: "integer", Description: "Maximum content results to return.", Default: 50, Minimum: intPtr(1)},
},
+25 -50
View File
@@ -31,15 +31,16 @@ func NewScopedReadFileTool(workspaceRoot string, scope PathScope) Tool {
return readFileTool{
baseTool: baseTool{
name: "read_file",
description: "Read exact file text with line numbers. Use for comments, formatting, or edits; prefer read_minified_file for initial code understanding. Use offset and limit for a line range.",
description: "Read a file by 1-based inclusive line range, or by exact byte chunks for oversized single-line files.",
parameters: Schema{
Type: "object",
Properties: map[string]PropertySchema{
"path": {Type: "string", Description: "File path."},
"offset": {Type: "integer", Description: "Optional 1-based source line to start from.", Minimum: intPtr(1)},
"limit": {Type: "integer", Description: "Optional number of source lines to return.", Minimum: intPtr(1)},
"byte_offset": {Type: "integer", Description: "Optional zero-based offset for exact byte reads.", Minimum: intPtr(0)},
"byte_limit": {Type: "integer", Description: "Optional byte count for an exact byte read; use with byte_offset.", Minimum: intPtr(1), Maximum: intPtr(readFileByteChunkMax)},
"path": {Type: "string", Description: "Path of the file to read."},
"start_line": {Type: "integer", Description: "1-based inclusive line number to start reading from.", Minimum: intPtr(1)},
"end_line": {Type: "integer", Description: "1-based inclusive line number to stop reading at.", Minimum: intPtr(1)},
"max_lines": {Type: "integer", Description: "Maximum number of lines to return.", Minimum: intPtr(1)},
"byte_offset": {Type: "integer", Description: "Zero-based byte offset for an exact chunk read. Use with byte_limit for oversized single-line files; cannot be combined with line ranges.", Minimum: intPtr(0)},
"byte_limit": {Type: "integer", Description: "Maximum source bytes returned in exact byte mode. Defaults to 65536.", Default: readFileByteChunkMax, Minimum: intPtr(1), Maximum: intPtr(readFileByteChunkMax)},
},
Required: []string{"path"},
AdditionalProperties: false,
@@ -68,27 +69,7 @@ func (tool readFileTool) run(args map[string]any, options RunOptions, directBudg
if err != nil {
return errorResult("Error: Invalid arguments for read_file: " + err.Error())
}
_, hasOffset := args["offset"]
_, hasLimit := args["limit"]
_, hasStartLine := args["start_line"]
_, hasEndLine := args["end_line"]
_, hasMaxLines := args["max_lines"]
if hasOffset && hasStartLine {
return errorResult("Error: Invalid arguments for read_file: use either offset or start_line, not both")
}
if hasLimit && (hasEndLine || hasMaxLines) {
return errorResult("Error: Invalid arguments for read_file: use limit instead of end_line or max_lines, not both")
}
startKey := "offset"
if !hasOffset && hasStartLine {
startKey = "start_line"
}
startLine, err := intArg(args, startKey, 1, 1, 0)
if err != nil {
return errorResult("Error: Invalid arguments for read_file: " + err.Error())
}
limit, err := intArg(args, "limit", 0, 1, 0)
startLine, err := intArg(args, "start_line", 1, 1, 0)
if err != nil {
return errorResult("Error: Invalid arguments for read_file: " + err.Error())
}
@@ -100,25 +81,19 @@ func (tool readFileTool) run(args map[string]any, options RunOptions, directBudg
if err != nil {
return errorResult("Error: Invalid arguments for read_file: " + err.Error())
}
if hasLimit {
const maxInt = int(^uint(0) >> 1)
endLine = startLine + min(limit-1, maxInt-startLine)
}
hasLineRange := hasOffset || hasLimit || hasStartLine || hasEndLine || hasMaxLines
_, hasByteOffset := args["byte_offset"]
_, hasByteLimit := args["byte_limit"]
byteMode := (hasByteOffset || hasByteLimit) && !hasLineRange
byteOffset := 0
byteLimit := readFileByteChunkMax
if byteMode {
byteOffset, err = intArg(args, "byte_offset", 0, 0, 0)
if err != nil {
return errorResult("Error: Invalid arguments for read_file: " + err.Error())
}
byteLimit, err = intArg(args, "byte_limit", readFileByteChunkMax, 1, readFileByteChunkMax)
if err != nil {
return errorResult("Error: Invalid arguments for read_file: " + err.Error())
}
byteMode := hasByteOffset || hasByteLimit
byteOffset, err := intArg(args, "byte_offset", 0, 0, 0)
if err != nil {
return errorResult("Error: Invalid arguments for read_file: " + err.Error())
}
byteLimit, err := intArg(args, "byte_limit", readFileByteChunkMax, 1, readFileByteChunkMax)
if err != nil {
return errorResult("Error: Invalid arguments for read_file: " + err.Error())
}
if byteMode && (args["start_line"] != nil || args["end_line"] != nil || args["max_lines"] != nil) {
return errorResult("Error: Invalid arguments for read_file: byte_offset/byte_limit cannot be combined with line ranges")
}
absolutePath, relativePath, err := resolveScopedReadPath(tool.workspaceRoot, tool.scope, requestedPath)
@@ -194,7 +169,7 @@ func renderedReadRange(total, start, end, maxLines int) (int, int) {
func renderReadFileRange(absolutePath string, relativePath string, total int, startLine int, endLine int, maxLines int, maxBytes int) Result {
if startLine > total {
return okResult(fmt.Sprintf("File: %s\n(offset %d is past the end of the file, which has %d lines)", relativePath, startLine, total))
return okResult(fmt.Sprintf("File: %s\n(start_line %d is past the end of the file, which has %d lines)", relativePath, startLine, total))
}
if endLine == 0 || endLine > total {
endLine = total
@@ -233,9 +208,9 @@ func renderReadFileRange(absolutePath string, relativePath string, total int, st
// Tool.Run calls preserve the legacy prefix-only byte budget.
var budgetedOutput *outputBudgetBuilder
if maxBytes > 0 {
budgetedOutput = newOutputBudgetBuilder(maxBytes, "use offset/limit to request a smaller line range")
budgetedOutput = newOutputBudgetBuilder(maxBytes, "use start_line/end_line or max_lines for normal files; use byte_offset/byte_limit for an oversized single line")
} else {
budgetedOutput = newHeadTailOutputBudgetBuilder(readOutputBudgetBytes, "use offset/limit to request a smaller line range")
budgetedOutput = newHeadTailOutputBudgetBuilder(readOutputBudgetBytes, "use start_line/end_line or max_lines for normal files; use byte_offset/byte_limit for an oversized single line")
}
budgetedOutput.WriteString(header)
budgetedOutput.WriteString("\n")
@@ -249,9 +224,9 @@ func renderReadFileRange(absolutePath string, relativePath string, total int, st
}
if truncated {
// The Truncated flag alone is invisible to the model in the rendered
// output, so it cannot tell a limit cut from a complete read. Make the
// output, so it cannot tell a max_lines cut from a complete read. Make the
// cut explicit and tell it how to continue.
budgetedOutput.WriteString(fmt.Sprintf("\n\n[truncated: %d more line(s) in the requested range not shown; set offset=%d to continue]", endLine-lastLine, lastLine+1))
budgetedOutput.WriteString(fmt.Sprintf("\n\n[truncated: %d more line(s) in the requested range not shown; set start_line=%d to continue]", endLine-lastLine, lastLine+1))
}
budgeted := budgetedOutput.Result()
@@ -266,7 +241,7 @@ func renderReadFileRange(absolutePath string, relativePath string, total int, st
if budgeted.Truncated {
meta["truncation_reason"] = "byte_budget"
} else {
meta["truncation_reason"] = "limit"
meta["truncation_reason"] = "max_lines"
}
}
return Result{
+8 -71
View File
@@ -28,13 +28,12 @@ func NewScopedReadMinifiedFileTool(workspaceRoot string, scope PathScope) Tool {
return readMinifiedFileTool{
baseTool: baseTool{
name: "read_minified_file",
description: "Read source code in a token-efficient, language-aware form. Prefer this for initial understanding; use read_file for exact text or line numbers.",
description: "Read a source file in a dense, token-cheap form: comments and redundant whitespace removed, no line numbers. Use it to scan or understand code for far fewer tokens than read_file. For exact text, comments, line numbers, or before editing, use read_file instead.",
deferred: true,
parameters: Schema{
Type: "object",
Properties: map[string]PropertySchema{
"path": {Type: "string", Description: "Source file path."},
"offset": {Type: "integer", Description: "Optional 1-based source line to start from.", Minimum: intPtr(1)},
"limit": {Type: "integer", Description: "Optional maximum number of source lines to minify.", Minimum: intPtr(1)},
"path": {Type: "string", Description: "Path of the file to read in minified form."},
},
Required: []string{"path"},
AdditionalProperties: false,
@@ -60,14 +59,6 @@ func (tool readMinifiedFileTool) run(args map[string]any, options RunOptions, di
if err != nil {
return errorResult("Error: Invalid arguments for read_minified_file: " + err.Error())
}
offset, err := intArg(args, "offset", 1, 1, 0)
if err != nil {
return errorResult("Error: Invalid arguments for read_minified_file: " + err.Error())
}
limit, err := intArg(args, "limit", 0, 1, 0)
if err != nil {
return errorResult("Error: Invalid arguments for read_minified_file: " + err.Error())
}
absolutePath, relativePath, err := resolveScopedReadPath(tool.workspaceRoot, tool.scope, requestedPath)
if err != nil {
@@ -84,19 +75,11 @@ func (tool readMinifiedFileTool) run(args map[string]any, options RunOptions, di
info, _ := os.Stat(absolutePath)
options.FileTracker.Record(absolutePath, content, info)
selected := selectSourceLines(content, offset, limit)
if selected.pastEnd {
return okResult(fmt.Sprintf("File: %s\n(offset %d is past the end of the file, which has %d lines)", relativePath, offset, selected.totalLines))
}
ranged := offset > 1 || limit > 0
result := minify.File(relativePath, selected.content)
if ranged {
result = minify.ContextualFragment(relativePath, content, selected.content, selected.startByte)
}
rawLines := lineCount(string(selected.content))
result := minify.File(relativePath, content)
rawLines := lineCount(string(content))
minLines := lineCount(result.Content)
pct := 0
if rawBytes := len(selected.content); rawBytes > 0 {
if rawBytes := len(content); rawBytes > 0 {
if saved := rawBytes - len(result.Content); saved > 0 {
pct = saved * 100 / rawBytes
}
@@ -106,15 +89,12 @@ func (tool readMinifiedFileTool) run(args map[string]any, options RunOptions, di
if result.Applied {
header = fmt.Sprintf("File: %s — minified %s view (comments stripped, no line numbers; %d→%d lines, ~%d%% fewer bytes). For exact text/comments or before editing, use read_file.",
relativePath, result.Language, rawLines, minLines, pct)
} else if ranged {
header = fmt.Sprintf("File: %s — safe ranged view (whitespace normalized, no line numbers; %d→%d lines; context-sensitive stripping disabled because the range may begin inside a multiline construct). For exact text, use read_file.",
relativePath, rawLines, minLines)
} else {
header = fmt.Sprintf("File: %s — whitespace-normalized view (no line numbers; %d→%d lines; full minification not available for this file type). For exact text, use read_file.",
relativePath, rawLines, minLines)
}
rawBytes := len(selected.content)
rawBytes := len(content)
compactBytes := len(result.Content)
savedTokens := 0
if savedBytes := rawBytes - compactBytes; savedBytes > 0 {
@@ -133,54 +113,11 @@ func (tool readMinifiedFileTool) run(args map[string]any, options RunOptions, di
meta["estimated_tokens_saved"] = strconv.Itoa(savedTokens)
toolResult := Result{Status: StatusOK, Output: output, Meta: meta}
if directBudget {
return applyLegacyByteBudgetToResult(toolResult, readOutputBudgetBytes, "use offset/limit to request a smaller source range, or read_file when exact text is required")
return applyLegacyByteBudgetToResult(toolResult, readOutputBudgetBytes, "use read_file with start_line/end_line or max_lines for normal files, or byte_offset/byte_limit for an oversized single line")
}
return toolResult
}
type sourceSelection struct {
content []byte
startByte int
totalLines int
pastEnd bool
}
func selectSourceLines(content []byte, offset, limit int) sourceSelection {
if offset <= 1 && limit == 0 {
return sourceSelection{content: content, totalLines: sourceLineCount(content)}
}
lines := strings.Split(string(content), "\n")
totalLines := sourceLineCount(content)
start := offset - 1
if start >= totalLines {
return sourceSelection{startByte: len(content), totalLines: totalLines, pastEnd: true}
}
startByte := 0
for index := 0; index < start; index++ {
startByte += len(lines[index]) + 1
}
end := totalLines
if limit > 0 && limit < end-start {
end = start + limit
}
return sourceSelection{
content: []byte(strings.Join(lines[start:end], "\n")),
startByte: startByte,
totalLines: totalLines,
}
}
func sourceLineCount(content []byte) int {
if len(content) == 0 {
return 1
}
lines := strings.Count(string(content), "\n")
if content[len(content)-1] != '\n' {
lines++
}
return lines
}
// lineCount reports the number of newline-separated lines in s (an empty string
// counts as 0 lines, matching how a reader perceives an empty file).
func lineCount(s string) int {
-117
View File
@@ -45,123 +45,6 @@ func TestReadMinifiedFileRejectsTraversal(t *testing.T) {
}
}
func TestReadMinifiedFileSelectsSourceLineRangeBeforeMinifying(t *testing.T) {
dir := t.TempDir()
src := "package demo\n\nfunc One() int { return 1 }\n\nfunc Two() int { return 2 }\n"
if err := os.WriteFile(filepath.Join(dir, "f.go"), []byte(src), 0o644); err != nil {
t.Fatal(err)
}
res := NewScopedReadMinifiedFileTool(dir, nil).Run(context.Background(), map[string]any{
"path": "f.go", "offset": 5, "limit": 1,
})
if res.Status != StatusOK || !strings.Contains(res.Output, "func Two") || strings.Contains(res.Output, "func One") {
t.Fatalf("unexpected ranged compact read: status=%s\n%s", res.Status, res.Output)
}
}
func TestReadMinifiedFileRangesPreserveUnknownLexicalContext(t *testing.T) {
tests := []struct {
name string
path string
src string
want []string
}{
{
name: "multiline string",
path: "snippet.py",
src: "value = \"\"\"\n# literal text\nstill literal\n\"\"\"\nprint(value)\n",
want: []string{"# literal text", "still literal"},
},
{
name: "template literal",
path: "snippet.js",
src: "const value = `\n// literal text\nstill literal\n`;\n",
want: []string{"// literal text", "still literal"},
},
{
name: "block comment",
path: "snippet.c",
src: "/* open\ncomment body\n*/\nint live;\n",
want: []string{"comment body", "*/"},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
dir := t.TempDir()
if err := os.WriteFile(filepath.Join(dir, tc.path), []byte(tc.src), 0o644); err != nil {
t.Fatal(err)
}
res := NewScopedReadMinifiedFileTool(dir, nil).Run(context.Background(), map[string]any{
"path": tc.path, "offset": 2, "limit": 2,
})
if res.Status != StatusOK || res.Meta["compacted"] != "false" {
t.Fatalf("ranged read must use conservative normalization: status=%s meta=%#v\n%s", res.Status, res.Meta, res.Output)
}
for _, want := range tc.want {
if !strings.Contains(res.Output, want) {
t.Fatalf("ranged read lost lexical content %q:\n%s", want, res.Output)
}
}
})
}
}
func TestReadMinifiedFileRangeInsideGoRawStringIsConservative(t *testing.T) {
dir := t.TempDir()
src := "package demo\nvar value = `first\nSECRET-MARKER-ONE\n// literal line\nlast`\n"
if err := os.WriteFile(filepath.Join(dir, "f.go"), []byte(src), 0o644); err != nil {
t.Fatal(err)
}
res := NewScopedReadMinifiedFileTool(dir, nil).Run(context.Background(), map[string]any{
"path": "f.go", "offset": 3, "limit": 2,
})
if res.Status != StatusOK || res.Meta["compacted"] != "false" {
t.Fatalf("raw-string range must use conservative normalization: status=%s meta=%#v\n%s", res.Status, res.Meta, res.Output)
}
for _, want := range []string{"SECRET-MARKER-ONE", "// literal line"} {
if !strings.Contains(res.Output, want) {
t.Fatalf("raw-string range lost or rewrote %q:\n%s", want, res.Output)
}
}
if strings.Contains(res.Output, "SECRET - MARKER - ONE") {
t.Fatalf("raw-string contents were parsed as Go code:\n%s", res.Output)
}
}
func TestReadMinifiedFileCanonicalOffsetPastEnd(t *testing.T) {
dir := t.TempDir()
if err := os.WriteFile(filepath.Join(dir, "f.go"), []byte("package demo\nvar value = 1\n"), 0o644); err != nil {
t.Fatal(err)
}
res := NewScopedReadMinifiedFileTool(dir, nil).Run(context.Background(), map[string]any{
"path": "f.go", "offset": 10,
})
if res.Status != StatusOK || !strings.Contains(res.Output, "offset 10 is past the end") {
t.Fatalf("expected canonical out-of-range response, got status=%s output=%q", res.Status, res.Output)
}
}
func TestReadMinifiedFileMaximumLimitDoesNotOverflow(t *testing.T) {
dir := t.TempDir()
if err := os.WriteFile(filepath.Join(dir, "notes.txt"), []byte("alpha\nbeta\ngamma\n"), 0o644); err != nil {
t.Fatal(err)
}
const maxInt = int(^uint(0) >> 1)
res := NewScopedReadMinifiedFileTool(dir, nil).Run(context.Background(), map[string]any{
"path": "notes.txt", "offset": 2, "limit": maxInt,
})
if res.Status != StatusOK {
t.Fatalf("maximum limit should return the remaining range without panicking: status=%s output=%q", res.Status, res.Output)
}
if !strings.Contains(res.Output, "beta") || !strings.Contains(res.Output, "gamma") || strings.Contains(res.Output, "alpha") {
t.Fatalf("maximum limit returned the wrong range: %q", res.Output)
}
}
func TestReadMinifiedFileAppliesByteBudget(t *testing.T) {
dir := t.TempDir()
if err := os.WriteFile(filepath.Join(dir, "large.txt"), []byte(strings.Repeat("0123456789abcdef\n", 9000)), 0o644); err != nil {
+1 -1
View File
@@ -297,8 +297,8 @@ func scrubResultSecrets(res Result) Result {
func CoreReadOnlyToolsScoped(workspaceRoot string, scope PathScope) []Tool {
return []Tool{
NewScopedReadMinifiedFileTool(workspaceRoot, scope),
NewScopedReadFileTool(workspaceRoot, scope),
NewScopedReadMinifiedFileTool(workspaceRoot, scope),
// view_image is read-only and shares read_file's path scoping, so it
// belongs with the other readers rather than behind a gate.
NewScopedViewImageTool(workspaceRoot, scope),
+20 -19
View File
@@ -523,24 +523,25 @@ func TestIsDeferralEligibleDecouplesFromDeferred(t *testing.T) {
func TestOptionalBuiltinsUseDeferredDiscovery(t *testing.T) {
wantDeferred := map[string]bool{
"bash": true,
"browser_action": true,
"browser_click": true,
"browser_connect": true,
"browser_install": true,
"browser_launch": true,
"browser_open": true,
"browser_press": true,
"browser_snapshot": true,
"browser_type": true,
"capture_artifact": true,
"desktop_action": true,
"desktop_snapshot": true,
"desktop_windows": true,
"lsp_navigate": true,
"terminal_session": true,
"web_fetch": true,
"web_search": true,
"bash": true,
"browser_action": true,
"browser_click": true,
"browser_connect": true,
"browser_install": true,
"browser_launch": true,
"browser_open": true,
"browser_press": true,
"browser_snapshot": true,
"browser_type": true,
"capture_artifact": true,
"desktop_action": true,
"desktop_snapshot": true,
"desktop_windows": true,
"lsp_navigate": true,
"read_minified_file": true,
"terminal_session": true,
"web_fetch": true,
"web_search": true,
}
for _, tool := range BuiltinCatalog(t.TempDir()) {
if _, listed := wantDeferred[tool.Name()]; listed {
@@ -548,7 +549,7 @@ func TestOptionalBuiltinsUseDeferredDiscovery(t *testing.T) {
t.Errorf("%s should be deferred", tool.Name())
}
delete(wantDeferred, tool.Name())
} else if tool.Name() == "read_file" || tool.Name() == "read_minified_file" || tool.Name() == "grep" || tool.Name() == ExecCommandToolName || tool.Name() == "edit_file" {
} else if tool.Name() == "read_file" || tool.Name() == "grep" || tool.Name() == ExecCommandToolName || tool.Name() == "edit_file" {
if IsDeferred(tool) {
t.Errorf("essential tool %s must stay eager", tool.Name())
}
+1 -1
View File
@@ -26,7 +26,7 @@ func NewUpdatePlanTool() *updatePlanTool {
return &updatePlanTool{
baseTool: baseTool{
name: "update_plan",
description: "Create or update the in-memory plan for implementation or investigation with at least three meaningful dependent steps. Do not use it for simple lookups, explanations, or code navigation. " +
description: "Create or update the in-memory plan for the current task. " +
"Pass the full ordered list of steps each call; it replaces the previous plan. " +
"Each item needs a `content` string; `status` defaults to \"pending\" and `id` is " +
"auto-numbered, so you only need to supply `content` (and `status` as the task progresses). " +
-60
View File
@@ -1,60 +0,0 @@
package tui
import (
"strings"
"testing"
"github.com/charmbracelet/x/ansi"
)
// A COPIED NUL KILLED THE WHOLE PROGRAM ON WINDOWS.
//
// github.com/atotto/clipboard's Windows path calls syscall.StringToUTF16, which
// PANICS rather than returning an error when the string contains a NUL — so
// selecting transcript text that carried one took the TUI down with "program
// experienced a panic" and a stack ending in clipboard.writeAll. A teammate hit
// it on a real Windows machine.
//
// The NUL reaches the clipboard because ansi.Strip removes escape SEQUENCES and
// leaves C0 bytes alone, and the transcript carries them from two directions:
// tool output (binary reads, git's -z listings) and this package's own card
// protocol, whose prefixes are literally "\x00command-card\x00".
func TestNulNeverReachesTheClipboard(t *testing.T) {
// The card prefix is a real, in-repo source: ansi.Strip leaves it intact.
if !strings.ContainsRune(ansi.Strip(commandCardTranscriptPrefix+"Profile"), 0) {
t.Fatal("setup: the card prefix no longer carries a NUL; this test guards the wrong thing")
}
for _, tc := range []struct{ name, in string }{
{"card prefix", commandCardTranscriptPrefix + "Profile\nstatus: ok"},
{"plan card prefix", planCardTranscriptPrefix + "plan"},
{"binary tool output", "before\x00after"},
{"git -z listing", "a.go\x00b.go\x00c.go"},
} {
t.Run(tc.name, func(t *testing.T) {
got := clipboardSafeText(tc.in)
if strings.ContainsRune(got, 0) {
t.Fatalf("a NUL survived to the clipboard write: %q — Windows would panic here", got)
}
// The readable content must survive: this sanitises, it does not truncate.
for _, want := range strings.Split(strings.ReplaceAll(tc.in, "\x00", "\n"), "\n") {
if want = strings.TrimSpace(want); want != "" && !strings.Contains(got, want) {
t.Fatalf("sanitising dropped real content %q from %q", want, got)
}
}
})
}
}
// Newlines and tabs are legal clipboard content and must survive — a multi-line
// selection is the normal case, not the exception.
func TestClipboardSanitisingKeepsLayout(t *testing.T) {
in := "line one\n\tindented\nline three"
if got := clipboardSafeText(in); got != in {
t.Fatalf("clean multi-line text was altered:\n got %q\nwant %q", got, in)
}
// Other C0 bytes go: they would terminate the OSC52 escape early and
// corrupt the copy even where they do not panic.
if got := clipboardSafeText("a\x07b\x1bc"); got != "abc" {
t.Fatalf("control bytes survived: %q", got)
}
}
-47
View File
@@ -1499,54 +1499,7 @@ func (m model) selectedTranscriptText() string {
return strings.Join(parts, "\n")
}
// clipboardSafeText removes the control characters a clipboard write cannot
// carry, keeping newlines and tabs.
//
// A NUL PANICS THE WHOLE PROGRAM ON WINDOWS, and that is not a theoretical
// concern: syscall.StringToUTF16 — which github.com/atotto/clipboard calls on
// Windows — PANICS rather than erroring when the string contains one, so a
// single copied NUL killed the TUI with "program experienced a panic". It
// reached the clipboard because ansi.Strip removes escape SEQUENCES and leaves
// C0 bytes untouched, and the transcript legitimately carries them from two
// directions: tool output (a read of a binary file, git's -z NUL-separated
// listings) and this package's own card protocol, whose row prefixes are
// literally "\x00command-card\x00".
//
// Stripped at the WRITE, not at each source, because the sources are unbounded:
// any tool result a user can select from is a source, and a fix per source is
// one that the next tool re-breaks. The same pass protects the OSC52 fallback,
// where a stray control byte would terminate the escape sequence early and
// corrupt the copy rather than crash.
//
// \n and \t survive: a multi-line selection is the normal case, and both are
// legal clipboard content.
func clipboardSafeText(text string) string {
if !strings.ContainsFunc(text, unsafeClipboardRune) {
return text
}
var out strings.Builder
out.Grow(len(text))
for _, r := range text {
if unsafeClipboardRune(r) {
continue
}
out.WriteRune(r)
}
return out.String()
}
func unsafeClipboardRune(r rune) bool {
if r == '\n' || r == '\t' {
return false
}
return r < 0x20 || r == 0x7f
}
func copyTranscriptSelectionCmd(text string) tea.Cmd {
// SANITISED ONCE, HERE, before either destination sees it — the native
// clipboard would panic on a NUL and the OSC52 fallback would be corrupted
// by one.
text = clipboardSafeText(text)
return func() tea.Msg {
// Prefer the native OS clipboard (pbcopy / clip.exe / xclip): it works on
// local terminals — including macOS Terminal.app, which has no OSC52 support