Compare commits

..
3 Commits
Author SHA1 Message Date
lukaszraczylo 99de8a93b9 test(search): cover cross-file context attribution in parseOutput
parseOutput's pendingFile guard attaches before-context only when it belongs to the same file as the following match. Add tests for both the cross-file leak guard (fileA context must not attach to a fileB match) and the same-file positive case. Test-only; no behavior change.
2026-06-21 00:07:08 +01:00
lukaszraczylo a11623cb0d perf(edit_apply): drop redundant Diff: label; docs(file_read): note strip renumbering
edit_apply response=diff prefixed the fenced ```diff block with a 'Diff:'
label that the fence and the diff's own ---/+++ headers already convey — pure
token cost per diff response. Extract fencedDiff() (also dedups the diff and
default branches) and drop the label. Adds a test guard.

Also document that strip renumbers lines from 1 over the stripped content, so
the model doesn't cite original-file line numbers after a stripped read.
2026-06-20 02:12:46 +01:00
lukaszraczylo 5f0bb90e28 fix(ast_query): reliable first-page pagination cursor
The page-1 walk was capped at max_results via SkipAll, but buildASTCursorFooter
derives the remaining count from the total number of matches seen. Capping
truncated that total, so on page 1 the cursor fired only when the last parsed
file happened to overshoot the cap — nondeterministic — and reported a wrong
remaining count when it did. Same failure class as the file_search first-page
cursor bug.

Always walk the full tree (pages 2+ already did). The common case where matches
fit in one page already visited everything (SkipAll never triggered), and parse
results are LRU-cached, so the extra cost falls only on multi-page queries and is
mostly file I/O.

Adds a strict regression test asserting page 1 emits a cursor with an accurate
remaining count; the existing pagination test silently skipped when no cursor.
2026-06-20 01:44:47 +01:00
6 changed files with 119 additions and 21 deletions
+45
View File
@@ -13,6 +13,51 @@ import (
"github.com/lukaszraczylo/mcp-filepuff/internal/config"
)
// parseOutput must not attach one file's pending before-context to a match in a
// different file (cross-file leak guard in the JSON stream parser).
func TestParseOutputCrossFileContextNotLeaked(t *testing.T) {
s := newTestSearcher(t)
var buf bytes.Buffer
// fileA emits a before-context with no following match in fileA; then fileB
// has a match. fileA's pending context must NOT attach to fileB's match.
buf.WriteString(`{"type":"context","data":{"path":{"text":"fileA.go"},"lines":{"text":"orphan a ctx\n"},"line_number":1}}` + "\n")
buf.WriteString(`{"type":"match","data":{"path":{"text":"fileB.go"},"lines":{"text":"hit B\n"},"line_number":5,"submatches":[{"match":{"text":"hit"},"start":0,"end":3}]}}` + "\n")
res, err := s.parseOutput(&buf, 0)
if err != nil {
t.Fatalf("parseOutput: %v", err)
}
if len(res.Results) != 1 {
t.Fatalf("want 1 result, got %d", len(res.Results))
}
if res.Results[0].File != "fileB.go" {
t.Errorf("result file = %q, want fileB.go", res.Results[0].File)
}
if len(res.Results[0].Context.Before) != 0 {
t.Errorf("fileA context leaked into fileB match: %v", res.Results[0].Context.Before)
}
}
// parseOutput must attach a same-file before-context to the following match.
func TestParseOutputSameFileBeforeContext(t *testing.T) {
s := newTestSearcher(t)
var buf bytes.Buffer
buf.WriteString(`{"type":"context","data":{"path":{"text":"f.go"},"lines":{"text":"before line\n"},"line_number":4}}` + "\n")
buf.WriteString(`{"type":"match","data":{"path":{"text":"f.go"},"lines":{"text":"the match\n"},"line_number":5,"submatches":[{"match":{"text":"match"},"start":4,"end":9}]}}` + "\n")
res, err := s.parseOutput(&buf, 0)
if err != nil {
t.Fatalf("parseOutput: %v", err)
}
if len(res.Results) != 1 {
t.Fatalf("want 1 result, got %d", len(res.Results))
}
got := res.Results[0].Context.Before
if len(got) != 1 || got[0] != "before line" {
t.Errorf("same-file before-context not attached: %v", got)
}
}
// truncateLine must back off to a rune boundary so multibyte characters are
// never split into invalid UTF-8.
func TestTruncateLineRuneSafe(t *testing.T) {
+30
View File
@@ -215,6 +215,36 @@ func TestASTQuerySkipsDependencyDirs(t *testing.T) {
// ---- Feature 3 (ast_query): pagination cursor ----
// TestASTQueryFirstPageCursor is a strict regression guard: page 1 must emit a
// cursor with an accurate remaining count when more matches exist. Previously
// the page-1 walk was capped at max_results, so the total was truncated and the
// cursor fired nondeterministically (and with a wrong remaining count).
func TestASTQueryFirstPageCursor(t *testing.T) {
srv, tmpDir := newFeaturesServer(t)
ctx := context.Background()
// Fixture has 5 functions (Alpha/Beta/Gamma/Delta/Epsilon); cap at 2.
req := mcp.CallToolRequest{}
req.Params.Arguments = map[string]interface{}{
"pattern": "func $NAME() $RET",
"language": "go",
"paths": []interface{}{tmpDir},
"max_results": float64(2),
}
res, err := srv.handleASTQuery(ctx, req)
if err != nil {
t.Fatalf("page1 error: %v", err)
}
text := res.Content[0].(mcp.TextContent).Text
if !strings.Contains(text, "[cursor:") {
t.Fatalf("page1 with 5 matches capped at 2 must emit a cursor, got:\n%s", text)
}
// 5 total - 2 shown = 3 remaining.
if !strings.Contains(text, "remaining: 3") {
t.Errorf("expected accurate 'remaining: 3', got:\n%s", text)
}
}
func TestASTQueryPaginationCursor(t *testing.T) {
srv, tmpDir := newFeaturesServer(t)
ctx := context.Background()
+7 -10
View File
@@ -109,12 +109,13 @@ func (s *Server) runASTQueryWalk(ctx context.Context, p *astQueryParams, exts []
},
}
// Collect limit for early-exit: when paginating we need all results first.
collectLimit := p.maxResults
if p.offset > 0 {
collectLimit = 0
}
// A full walk is required on every page: the cursor's "remaining" count is
// derived from the total match count (see buildASTCursorFooter), so the page-1
// response can only emit a correct cursor if it has seen every match. Capping
// page 1 at max_results made the cursor fire nondeterministically (only when
// the last parsed file overshot the cap) with a wrong remaining count. Parse
// results are LRU-cached, so re-walking on later pages is mostly file I/O, and
// matches below max_results already visit the whole tree regardless.
var allResults []query.MatchResult
for _, searchPath := range p.paths {
if !s.cfg.IsPathAllowed(searchPath) {
@@ -162,10 +163,6 @@ func (s *Server) runASTQueryWalk(ctx context.Context, p *astQueryParams, exts []
return nil
}
allResults = append(allResults, matches...)
if collectLimit > 0 && len(allResults) >= collectLimit {
return filepath.SkipAll
}
return nil
})
if walkErr != nil {
+9 -10
View File
@@ -92,22 +92,21 @@ func (s *Server) handleEdit(ctx context.Context, request mcp.CallToolRequest) (*
return mcp.NewToolResultText(fmt.Sprintf("+%d -%d", added, removed)), nil
case "diff":
var output strings.Builder
output.WriteString("Diff:\n```diff\n")
output.WriteString(result.Diff)
output.WriteString("```\n")
return mcp.NewToolResultText(output.String()), nil
return mcp.NewToolResultText(fencedDiff(result.Diff)), nil
default:
// Fallback: treat unknown values as "diff" for safety.
var output strings.Builder
output.WriteString("Diff:\n```diff\n")
output.WriteString(result.Diff)
output.WriteString("```\n")
return mcp.NewToolResultText(output.String()), nil
return mcp.NewToolResultText(fencedDiff(result.Diff)), nil
}
}
// fencedDiff wraps a unified diff in a ```diff code fence. No "Diff:" label is
// added — the fence and the diff's own ---/+++ headers already identify it, so
// the label would only cost tokens.
func fencedDiff(diff string) string {
return "```diff\n" + diff + "```\n"
}
// countDiffLines counts added (+) and removed (-) content lines in a unified diff.
// Only lines inside hunks are counted (everything after the first "@@" header), so the
// "---"/"+++" file headers are skipped structurally — and content whose own text starts
+27
View File
@@ -59,6 +59,33 @@ func TestEditApplyPreservesBackslashSequencesText(t *testing.T) {
}
}
// TestEditApplyDiffResponseFencedNoLabel verifies response=diff emits the unified
// diff in a ```diff fence with no redundant "Diff:" label (the fence and the diff's
// own headers already identify it; the label would only cost tokens).
func TestEditApplyDiffResponseFencedNoLabel(t *testing.T) {
tmpDir := t.TempDir()
srv := newTestServer(t, tmpDir)
f := writeFile(t, tmpDir, "note.txt", "before\n")
res := callEdit(t, srv, map[string]any{
"file": f,
"operation": "replace",
"selector_text": "before",
"new_content": "after",
"response": "diff",
})
if res.IsError {
t.Fatalf("edit returned error: %+v", res.Content)
}
out := res.Content[0].(mcp.TextContent).Text
if !strings.HasPrefix(out, "```diff\n") {
t.Errorf("diff response should start with the ```diff fence, got:\n%s", out)
}
if strings.Contains(out, "Diff:") {
t.Errorf("diff response should not carry a redundant 'Diff:' label, got:\n%s", out)
}
}
// TestCountDiffLinesHunkAware verifies the line counter skips file headers structurally and
// counts content lines whose own text begins with + or - (previously dropped as "+++"/"---").
func TestCountDiffLinesHunkAware(t *testing.T) {
+1 -1
View File
@@ -193,7 +193,7 @@ func (s *Server) registerTools() {
mcp.Description("Output mode: 'full' (default, full file), 'skeleton' (signatures + { ... } stubs, bodies elided), 'symbols_only' (symbol list only, alias for symbols_only=true)."),
),
mcp.WithArray("strip",
mcp.Description("Strip content classes before line-numbering. Values: 'imports' (remove import blocks), 'license' (remove leading license comment), 'block_comments' (remove /* */ and Python triple-quoted strings). Emits [stripped: ...] footer."),
mcp.Description("Strip content classes before line-numbering. Values: 'imports' (remove import blocks), 'license' (remove leading license comment), 'block_comments' (remove /* */ and Python triple-quoted strings). Emits [stripped: ...] footer. Note: line numbers are renumbered from 1 over the stripped content, so they no longer match the file's original line numbers."),
mcp.WithStringItems(),
),
mcp.WithNumber("max_lines",