The config used v1 keys under a v2 header, so golangci-lint config verify
rejected it and the Lint job failed. This change moves the exclusions to
linters.exclusions and the output order to output.sort-order.
The old linter settings never took effect. Enforcing them reports 62 issues
in the current code. Only the staticcheck setting stays, because it reports
none. The gocyclo, errcheck and govet settings need a separate cleanup.
The go.mod file requires Go 1.26.0. The release job runs Go 1.25 with
GOTOOLCHAIN=local, so it fails before it runs any test. The lint and
security jobs use the same pin. This change sets all three to 1.26.
The default limits let broad queries flood the model context. This
change sets smaller defaults. An explicit parameter still overrides each one.
- file_search: max_results 1000 to 50, context_lines 2 to 0.
- ast_query: max_results 100 to 50.
- ast_query compact output omits the node type unless verbose is set.
On a fixed seven-call suite, response size fell from 198529 to 61960 bytes.
A crashed language server stayed cached with ready==true: readLoop exited and
drained in-flight calls but never cleared the running flag, and GetServer keyed
reuse on ready alone. Every subsequent hover/definition/references then passed
the running guard, wrote to a dead stdin, and blocked the full request timeout —
permanently, since active use kept the server off the idle reaper. A single
crash wedged that language at ~10s/call with no self-heal.
- readLoop now flips running=false on exit, so Call fails fast instead of hanging.
- GetServer treats ready && client.IsRunning() as the liveness condition; a stale
entry is evicted (closed async — its process has already exited) and a fresh
server is spawned in its place.
- drainPending's send is now non-blocking, matching the dispatch path, so a
duplicate-id race can't deadlock it under c.mu.
Test (proven to fail without the fix): a client whose process exits is marked
not-running and its next Call fails fast rather than blocking.
edit.ValidateLanguage had zero callers (only its own godoc referenced it) —
removed.
yaml_json.go: contrary to the audit's claim, the file is NOT entirely dead —
ParseYAML/ParseJSON are live via the Parse() dispatch (parser.go:198-200) and
are kept with their tests. Removed only the genuinely unreachable scaffolding
that nothing constructs or calls: the YAMLNode/JSONNode types, WalkYAML,
GetYAMLNodeText, GetYAMLNodeLocation, QueryYAML, QueryJSON, ValidateYAML,
ValidateJSON, and ToSitterTree, plus their now-orphaned tests. YAML/JSON parsing
behavior is unchanged (TestParseYAML/TestParseJSON/TestRegistryParse_YAML_JSON
still pass).
Add behavior tests for the highest-risk under-tested mutation paths: findExactText
(overlapping-match index resolution + out-of-range/empty/not-found errors),
findRegexPattern (non-overlapping index resolution + out-of-range/invalid-regex/
not-found errors), and spliceContent's CRLF pullback (an edit ending between \r
and \n must not orphan a bare LF). These resolve the byte range that gets mutated,
so wrong handling means wrong-location or line-ending-corrupting writes.
References defaulted to one line per reference; compact (one line per file,
columns collapsed) existed only as opt-in. Compact preserves every line:col pair
with zero information loss while cutting roughly one line per reference for the
LLM consumer. Default it to true; compact=false and the session compact_refs
pref still select the verbose per-reference layout (explicit override wins).
The default line prefix was the padded ' 42| ' form; the compact '42|' form
existed only as opt-in (compact_line_numbers). The padding and trailing space
carry no information an LLM needs, costing ~3 chars/line — ~300 tokens on a
400-line read, the highest-volume path in the server. Flip the default to
compact; compact_line_numbers=false, mode/line_numbers=full, and the session
'full' pref all still restore the padded alignment (explicit override wins).
Updates the two tests that pinned the old default and refreshes stale comments.
edit_apply validated the target with IsPathAllowed in the handler, then later
wrote it via os.WriteFile on the raw path after reading+parsing+diffing. A
symlink swapped in during that window could redirect the write outside the
workspace root. Inject the admission check into the edit Engine
(NewEngineWithValidator) and re-run it inside a new writeValidatedFile helper
immediately before the write — IsPathAllowed resolves symlinks at write time, so
a swapped-in link now resolving outside the root is rejected. This narrows the
TOCTOU window to near-zero while still permitting legitimate in-workspace symlink
edits (unlike a blanket O_NOFOLLOW). writeValidatedFile also dedups the
perm-preserving write shared by AST and text modes.
Tests: rejected-path edit leaves the file untouched; file mode is preserved.
lsp_query printed absolute paths from URIToFile (e.g. the full
/Users/.../internal/... prefix repeated per result) while file_search and the
AST summary already relativize. On a find_references across many files that
~60-char root prefix repeats per file group — wasted tokens for the LLM
consumer. Extract a Server.relPath helper (dedups the AST summary's inline
block) and apply it to definition and references display paths; the absolute
path is kept where the file is actually re-read for previews.
Adds a relPath unit test (in-workspace relativized, out-of-workspace unchanged).
line/col are 1-indexed and converted via uint32(line-1); a caller passing 0
wrapped to 4294967295, feeding a garbage Point into tree-sitter. Reachable from
the LSP-down AST fallback with caller-supplied coordinates. Reject non-positive
positions up front. Adds tests (the function was previously 0% covered).
file_search runs ripgrep with --follow on by default, so a symlink inside the
workspace pointing outside it (e.g. link -> /etc) caused rg to traverse out and
return matches from external files — leaking their content to the caller despite
the workspace-scoping contract. parseOutput emitted match paths verbatim with no
re-validation.
Filter every match through IsPathAllowed (resolving rg's workspace-relative path
against the root first); IsPathAllowed's EvalSymlinks rejects the escape while
still permitting legitimate worktree siblings. Defense in depth that also covers
--no-ignore and any other traversal that reaches outside the root.
Adds a security regression test (proven to fail without the filter).
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.
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.
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.
ast_query walked every language-matching file under the search root, including
node_modules, vendor and bower_components — parsing third-party code and
flooding results with irrelevant matches (large token cost on every call,
worst for JS/TS node_modules). Skip these by name during the walk, alongside
the existing hidden-dir skip.
The walk root is now exempt from both skips, so targeting a dependency dir
explicitly (paths:["vendor/foo"]) still works — previously naming a hidden or
dependency dir as the root would have skipped everything. Documents the scope
in the tool description and help resource.
Adds a regression test covering default-skip and explicit-target inclusion.
extractCFunction/extractCFunctionDecl scanned for the first 'identifier' in the declarator, which for C++ methods grabbed a parameter name (e.g. method setName surfaced as 'n') or, when the name was a field_identifier with no parameter identifier, dropped the symbol entirely. cDeclaratorName now descends the declarator field-chain to the real name (identifier/field_identifier/qualified_identifier/destructor/operator); in-class definitions are kinded as methods and qualified Class::method (out-of-line Class::method definitions detected via '::'). Adds TestExtractCppMethods.
On the first page (offset==0) ripgrep capped results at exactly max_results
and set Truncated=true, but the cursor-footer guard (len>max_results) was
false, so the response said "(truncated...)" with no cursor — the caller
was told more matches exist but given no way to fetch them. Pagination only
worked from page 2. Emit a cursor on the first page using rg's total match
count for the remaining estimate.
Adds a strict regression test (asserts, not skips); the existing pagination
test was silently skipping its round-trip because page 1 never had a cursor.
firstLineOf (query) and truncateString (edit) sliced strings at a raw byte
offset, splitting a multibyte UTF-8 character into invalid bytes in
LLM-facing output (ast_query compact format and edit not-found errors are
hot paths). Extract the rune-boundary backoff already used by search's
truncateLine into util.RuneBoundary and route all three sites through it.
Adds table-driven tests for the primitive and a multibyte case for firstLineOf.
truncateLine byte-sliced s[:maxLen-3], which can split a multibyte UTF-8 character and emit an invalid sequence (mojibake) on long non-ASCII match lines. Back off to the nearest rune boundary via utf8.RuneStart. Adds TestTruncateLineRuneSafe.
Only max_file_size and max_parse_size were clamped after reading .mcp-filepuff.json; max_edit_size and max_search_results were unbounded, letting a config file request excessive memory. Clamp max_edit_size to 100MB and max_search_results to 100k (and floor negatives to 0). Adds TestLoadClampsExcessiveLimits.
FormatDocComment emitted leftover tags by ranging a map, so multi-tag comments (@example/@see/@deprecated) rendered in random order run-to-run — nondeterministic output that defeats caching/diffing. Sort the remaining tag names before emitting.
extractGoSymbols/extractJSSymbols matched var/const/let at any tree depth, so every function-local var, const (Go) and let/const/var (JS/TS) was emitted as a top-level symbol — flooding include_ast/ast_query symbol lists with locals (inaccurate + token-wasteful). Skip declarations nested inside a function/block body (isNestedInBody); module-level and 'export const' declarations are still kept. Adds TestExtractSymbolsExcludesLocals.
Locks the documented guarantee that inserting LF content into a CRLF file normalizes to CRLF, with no bare-LF leaks. Closes a test gap on a stated design goal.
extractElixirStruct hardcoded Name="defstruct", so every Elixir struct surfaced as a symbol literally called "defstruct" — useless for navigation and colliding across modules. Walk ancestors to the enclosing defmodule and name the struct %ModuleName{}. Adds TestExtractElixirStructNamedByModule.
drainStderr discarded all server stderr, so an LSP init failure (e.g. 'command not found', missing module) surfaced as an opaque 'initialization failed'. Retain a bounded tail (StderrCaptureMax=4096B, oldest dropped) under a mutex and attach it to the init-failure error context. Adds TestClientStderrCapture.
The fallback skeleton (c/cpp/elixir/html/vue) is not valid source, so the '// ' comment framing and '(line N)' wording just cost tokens. Use a compact 'kind name :line' form. ~15-25% fewer tokens on fallback skeletons; output stays accurate.
Per the project's error-handling rule, an ignored error needs a comment. Marshalling the two-scalar payload struct cannot fail; document why the error is dropped. No behavior change.
skeletonPythonNode took lines[0] of a decorated_definition, which is the first @decorator line, dropping the def/class signature entirely. Now emit the decorator line(s) then recurse into the wrapped definition so its signature is preserved. Adds skeleton_test.go (decorated + plain function).
readLoop sent the response on the pending channel while holding c.mu. A misbehaving server sending a duplicate response for the same id would block on the buffered(1) channel under the lock, deadlocking all future dispatches. Use a non-blocking send (matching the notification path).
Counter.Add applied any delta, so a negative value would decrement a counter that must be monotonic, corrupting rate()-style downstream math. Ignore delta<0. Adds TestCounterRejectsNegativeAdd.
Two file_search gaps:
- max_results defaulted to 0 (unbounded), so a broad pattern returned every match — unbounded memory + token blowup. The configured MaxSearchResults (default 1000) existed but was dead; wire it as the builtIn default. Explicit max_results still wins, and the cursor footer pages the rest.
- search.Request.IncludeHidden was a dead field with no tool param. Add an include_hidden bool (default false) so dotfiles (.github/, etc.) are searchable.
ast_query defaulted to verbose output (full code + up to 500 chars/match + captures), which is very token-heavy on multi-match queries. Default to compact (one line per match: file:line (kind) firstline) — roughly 5-10x fewer tokens on typical queries — while verbose remains available per-call (format=verbose) or via the default_format session pref. Updates tool description, help resource, and TestASTQueryFormatCompactDefault.
hunkBounds approximated one-sided start lines by reusing the opposite side's number, and re-assigned it every iteration inside the count==0 guard. For pure insertion/deletion hunks (e.g. writing into an empty file, or deleting a whole file) this produced wrong headers like '@@ -2,0 +1,2 @@' instead of '@@ -0,0 +1,2 @@'. flattenDiffLines now records both running line numbers on every line, so hunkBounds takes the start from the first line directly and applies GNU's zero-count convention (line-before-change, clamped to 0). Adds TestGenerateDiffHunkHeaderBounds.
handleASTQuery rebuilt the formatter's '[remaining: N]' placeholder string and strings.ReplaceAll'd it with the cursor line, plus a fallback append — fragile coupling to the formatter's exact output. New FormatResultsWithCursor appends the prebuilt '[cursor: ..., remaining: N]' footer in place of the bare hint when results are truncated; the handler just passes it through. FormatResultsWithOptions is unchanged (delegates with empty cursor). Adds TestFormatResultsWithCursor.
Two ast_query/lsp_query ergonomics improvements that share the tool registration and help files:
- lsp_query accepts symbol_name (+symbol_kind) and resolves the position via AST (resolveLSPSymbolPosition), so callers need not know line:col; line/column are no longer required. SymbolCandidate gains NameLine/NameColumn for the precise identifier position. Ambiguous/unknown names error with candidates / a 'did you mean' hint. Adds TestResolveLSPSymbolPosition.
- ast_query help + tool description now document that matching is keyword/kind-based, not structural: pattern arg/return constraints are not enforced; use name_exact/name_matches/kind_in to filter. Fixes a misleading 'functions returning error' example.
The goroutine parks in the blocking stderr.Read, so closing c.done does not interrupt it; termination relies on the read erroring at pipe EOF when the process is killed. The c.done select is only a best-effort fast path.
renderSymbolsOnly (fallback for c/cpp/elixir/html/vue) appended each symbol's raw start line plus a literal ' { ... }'. For brace-less languages (Elixir 'def f do', HTML) that is syntactically wrong and wastes tokens. Emit only the accurate symbol list; drop the fabricated line and unused params.
lukaszraczylo
