Add Language Server Protocol support with five code intelligence tools - #342
Conversation
…nce tools ## Summary Adds a Language Server Protocol (LSP) client and manager that enables code intelligence tools by launching language servers as subprocesses. ## New Features - **lsp_goto_definition**: jump to the definition of a symbol - **lsp_find_references**: find all usages of a symbol across the workspace - **lsp_hover**: get type info and documentation at a cursor position - **lsp_document_symbol**: list all symbols in a file (hierarchical) - **lsp_workspace_symbol**: search for a symbol by name across all files All five tools are read-only and guarded by the lsp.read capability. When a language server is still indexing or unavailable, tools return friendly errors suggesting retry or fallback to grep + read_file. ## Configuration Language servers are configured via lsp.json in the data directory. The file is optional; if absent, built-in defaults are used for Go, Python, TypeScript, Rust, C/C++, HTML/CSS/JSON, YAML, and Vue. Server discovery order: PATH lookup, known install directories (Go bin, Cargo bin, npm global, LLVM), and VS Code extension directories. ## Architecture - internal/lsp/: JSON-RPC 2.0 client, protocol types, server discovery, and manager that coordinates per-language server lifecycles - internal/tools/catalog_lsp.go: tool registration and handlers that delegate to the LSP manager - Language servers start lazily on first invocation and warm up in the background when matching workspace files are detected - ead_file full-read results include a symbol_outline field when LSP is available ## Testing 40 unit tests covering: - Tool registration and capability checks - Error message classification (no views, pipe closed, timeout, etc.) - Result formatting for locations, hover, and document symbols - Parameter schema validation - Input validation (invalid JSON, negative line/character, path escape) - All 5 tool handlers: success, no-provider, not-ready, and LSP-error paths - Workspace symbol multi-language aggregation - Truncation for results exceeding the display limit Test mocks (lspClient, lspToolProvider) are injected via the unexported lspOverride field on Toolset, avoiding the need for real language servers in tests. ## Documentation + 'go doc ./internal/lsp' + ' provides full package documentation covering configuration format with example, server discovery, lifecycle, and tool exposure. Type-level docs are present for Manager, Client, LSPConfig, and ServerConfig. Method-level docs cover Warmup, ReadyClientForFile, ClientForLanguage, and other key methods. ## Changed Files Modified: - internal/app/app_tools_init.go: load lsp.json, create Manager, warmup - internal/tools/catalog.go: register lspTools in the tool list - internal/tools/view.go: inject symbol_outline into read_file results - internal/tools/toolset.go: add LSP-related fields and setters New: - internal/lsp/: LSP client, manager, config, discovery, protocol types - internal/tools/catalog_lsp.go: tool registration and handlers - internal/tools/catalog_lsp_test.go: 40 unit tests
|
Hi @OliverZou. Thanks for the PR. Will take a look ASAP. |
| client, serverName, ready := p.readyClientForFile(abs) | ||
| if !ready { | ||
| return marshalToolError(call, "lsp_not_ready", fmt.Sprintf("language server %q is still starting up; wait a few seconds and retry, or use grep + read_file in the meantime", serverName)), nil | ||
| } |
There was a problem hiding this comment.
[P1] Start a server when an LSP tool is invoked
For a workspace whose only files for a language are below the two-level warmup scan limit, ReadyClientForFile remains false forever: these handlers only check readiness and never call ClientForFile to start the matching server. This makes all file-based LSP tools return lsp_not_ready permanently for common nested layouts rather than performing the documented lazy startup.
There was a problem hiding this comment.
确实存在,runLSPPositionOp 和 lspDocumentSymbol 应该改为在 readyClientForFile 返回 false 时 fallback 到 clientForFile（懒启动），这样才能实现文档所述的 "started lazily on first tool invocation"。会修复。
| type HoverContents struct { | ||
| Kind string `json:"kind"` // "markdown" or "plaintext" | ||
| Value string `json:"value"` | ||
| } |
There was a problem hiding this comment.
[P1] Decode all valid hover content shapes
LSP permits Hover.contents to be a MarkupContent, a legacy string, or an array of marked strings, but this concrete struct only decodes the first form. Servers returning either valid legacy representation cause json.Unmarshal in sendRequest to fail, so lsp_hover reports an LSP call failure instead of returning hover information.
There was a problem hiding this comment.
实现自定义 UnmarshalJSON，兼容 MarkupContent、string 和 MarkedString[] 三种 LSP 规范格式,会修复
| Position: Position{Line: line, Character: character}, | ||
| }, | ||
| Context: ReferenceContext{IncludeDeclaration: true}, | ||
| }, &result) |
There was a problem hiding this comment.
[P2] Honor the include_declaration option
The lsp_find_references schema exposes include_declaration, including a documented false value, but the handler never parses it and this request always sends IncludeDeclaration: true. Calls that request references excluding the declaration therefore return an incorrect extra location.
shayne-snap
left a comment
shayne-snap
left a comment
There was a problem hiding this comment.
Automated review of the LSP subsystem. Overall a well-structured, well-tested, opt-in feature. Main concerns are one functional gap (position tools never lazily start a server) and a few concurrency correctness issues worth verifying under go test -race before merge. Individual notes are inline.
| // returns nil and false. The returned string is the server name for error | ||
| // messages. Use this from LSP tool handlers that need fast checks without | ||
| // blocking on startup. | ||
| func (m *Manager) ReadyClientForFile(filePath string) (*Client, string, bool) { |
There was a problem hiding this comment.
Functional gap (blocker-worthy): ReadyClientForFile only returns clients already present in m.clients — it never lazily starts one. The position handlers (runLSPPositionOp, lspDocumentSymbol) go through this path, so a server only ever becomes usable if Warmup() started it. Combined with the depth-2 scan cap in scanDir (see below), any source file nested more than two directories below the workspace root returns lsp_not_ready permanently — the "retry in a few seconds" hint never resolves because nothing ever starts the server for that file.
Consider having this path fall back to a lazy clientForServer start, or route the position handlers through ClientForFile.
There was a problem hiding this comment.
和#1关联，有一点需要说明，就是warmup本身的作用仅仅是大致检查项目所涉及的文件类型，然后加载对应lsp服务，但即使没有warmup，也会在第一次访问时加载lsp服务，到lsp加载需要时间，这个过程中只能告知大模型还没准备好，先用缺省工具。这是设计行为。这个策略暂时这样，除非能找到更好的策略。
| } | ||
|
|
||
| func scanDir(dir string, depth int, result map[string]bool) { | ||
| if depth > 2 { |
There was a problem hiding this comment.
scanDir bails at depth > 2, so Warmup only discovers languages within two directories of the workspace root. Given that the position tools only work for servers Warmup started (see comment on ReadyClientForFile), deeper files silently never get code intelligence. Reconsider this cap, or make startup lazy so scan depth doesn't gate availability.
| return nil, err | ||
| } | ||
| var result []Location | ||
| err := c.conn.sendRequest(ctx,"textDocument/definition", TextDocumentPositionParams{ |
There was a problem hiding this comment.
Data race: the operation methods read c.conn without holding c.mu, while Close() writes c.conn = nil under the lock. A tool call concurrent with shutdown is a race and can nil-panic. Snapshot c.conn under the mutex (as ensureDocumentOpen starts to do) before using it. Recommend running the suite with -race.
| default: | ||
| } | ||
| // Check if process has exited | ||
| if c.cmd.ProcessState != nil && c.cmd.ProcessState.Exited() { |
There was a problem hiding this comment.
Data race: c.cmd.ProcessState is read here while a background goroutine runs cmd.Wait(). ProcessState is only valid after Wait() returns; reading it concurrently with Wait() is a documented race. Prefer keying liveness off the done channel (already checked above) or an atomic set when Wait completes.
| // initialize handshake happens outside the lock. | ||
| func (c *Client) Start(ctx context.Context) error { | ||
| c.mu.Lock() | ||
| if c.conn != nil { |
There was a problem hiding this comment.
This early return keys on c.conn != nil, but on an init failure below (initialize/initialized), c.conn is left set while ready stays false and the process is killed. A subsequent Start() then short-circuits here reporting "already started and ready" on a dead client. Gate this on c.ready.Load() and clear c.conn on the failure paths.
There was a problem hiding this comment.
确认。失败路径上应该 c.conn = nil（在持锁状态下），且 line 57 的判断条件应改为 c.conn != nil && c.ready.Load()。
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(raw) > 0 && raw[0] == '[' && bytes.Contains(raw, []byte(`"location"`)) { |
There was a problem hiding this comment.
Format detection is a substring heuristic (raw[0] == '[' && bytes.Contains(raw, "location")). A hierarchical DocumentSymbol whose name/detail legitimately contains location would be misparsed as SymbolInformation. Low probability, but a shape-based check (does the first array element have a location key) would be sturdier.
| } | ||
|
|
||
| // shutdown drains pending channels and sends nil to unblock callers. | ||
| func (c *rpcConn) shutdown() { |
There was a problem hiding this comment.
shutdown() is never called, so when readLoop exits (server died) pending requests are not unblocked — callers wait out their full context timeout instead of failing fast, and the msg == nil "connection closed" branch in sendRequest is unreachable. Wire shutdown() into readLoop's exit path.
| }, | ||
| } | ||
| for name, srv := range defaults { | ||
| cfg.RegisterServer(name, srv) |
There was a problem hiding this comment.
RegisterServer's error is swallowed here (and via continue in the user-server loop). An extension collision or invalid default disappears with no log, so a user editing lsp.json gets no feedback about why their server was ignored. Consider logging.
| return nil | ||
| } | ||
|
|
||
| func (c *LSPConfig) ForExtension(ext string) (*ServerConfig, string, bool) { |
There was a problem hiding this comment.
Minor: ForExtension is O(servers × extensions) and runs on every read_file/tool call. The claimed map already holds ext→server name; using it would make this O(1).
| webFetchClient *webfetch.Client | ||
| ddgSearchURL string | ||
| bingSearchURL string | ||
| bingSearchURL string |
There was a problem hiding this comment.
bingSearchURL string looks mis-aligned here, and this hunk also drops two explanatory comments (// Test hooks…, // Deferred tool search (MCP)). Please run gofmt/goimports and restore the comments if the removal was unintentional.
There was a problem hiding this comment.
确认。缩进问题是 gofmt 运行不完整导致的，会修正。
…nce tools - Add JSON-RPC 2.0 LSP client with Content-Length framing transport - Implement Manager for per-language server lifecycle coordination - Support 10 built-in language defaults (Go, Rust, Python, TypeScript, C/C++, HTML, CSS, JSON, YAML, Vue) with auto-discovery via PATH, known install directories, and VS Code extension directories - Expose 9 tools: goto_definition, find_references, hover, document_symbol, workspace_symbol, go_to_implementation, prepare_call_hierarchy, incoming_calls, outgoing_calls - Lazy-start design: first call waits up to 3s for server startup, subsequent calls fail fast while server warms in background - Crash restart monitor with configurable maxRestarts - Hover contents decode all three LSP formats (MarkupContent, string, MarkedString array) - Configurable lsp.json with env, initializationOptions, settings, startup/shutdown timeouts, restartOnCrash, maxRestarts, diagnostics - read_file results include symbol_outline when LSP is available - 50 unit tests covering tool registration, error classification, result formatting, parameter validation, and handler paths
- Extend languageIDFromPath with missing mappings: mjs/cjs to javascript, scss/less to css, jsonc to json - Log readLoop errors instead of silently discarding them - Add exponential backoff (1s-30s) and hard cap (100) to crash restart monitor to prevent infinite CPU spin when MaxRestarts is 0 - Expand SymbolKindName with all 26 LSP symbol kinds - Add unit tests for JSON-RPC framing, protocol types, config loading, server discovery, and client lifecycle - Restore default server config when user override fails validation - Log config parse errors in app_tools_init for troubleshooting - Document scanDir depth trade-off in code comments
P0 — Process leak: store lspManager in App struct and call Close() on shutdown to gracefully terminate gopls/rust-analyzer subprocesses. P0 — Transport: distinguish server-initiated requests from responses in readLoop by checking msg.Method. Reply MethodNotFound (-32601) to server requests (workspace/configuration, client/registerCapability) instead of misrouting them to pending callers. P1 — clientForServer race: build the replacement Client before acquiring the write lock, then atomically swap dead→new under the lock to prevent concurrent goroutines from creating orphan processes. P1 — Stale file content: remove openDocs cache from ensureDocumentOpen. Always read the file from disk and send textDocument/didOpen with fresh content, ensuring LSP results reflect the latest edits. P1 — Handshake timeout: apply c.startupTimeout inside Start() via a dedicated context.WithTimeout for the initialize request, so the handshake has a bounded deadline even when the caller passes context.Background(). P1 — Restart backoff: exponential backoff (1s-30s) with hard cap of 100 total restarts to prevent CPU spin on repeated crashes. Also: add missing languageIDFromPath mappings, expand SymbolKindName, add internal/lsp unit tests, and fix jsonrpc sendResponse routing.
… interference Go's default log.Logger writes timestamped messages to stderr, which can corrupt TUI rendering when Bubble Tea is in alternate screen mode. Replace all log.Printf calls in the LSP subsystem with fmt.Fprintf to os.Stderr matching the established project convention (whale: prefix), and remove the unused log import. Also finally remove the openDocs cache from ensureDocumentOpen — always read file content from disk to keep LSP results fresh.
The fmt.Fprintf to os.Stderr in the readLoop goroutine fires on every server exit, spamming the TUI with repetitive error messages like 'read header: EOF' or 'file already closed'. These are normal shutdown/restart signals, not actionable errors. Remove the stderr output entirely — errors during initialize handshake are still returned via Start()'s error value.
Re-add the openDocs map (map[string]time.Time) to avoid redundant textDocument/didOpen on every LSP call. ensureDocumentOpen now stats the file first and skips didOpen if the modTime is unchanged from the last open. This eliminates unnecessary file reads and JSON-RPC round trips for unchanged files while still sending fresh content when the file has been modified.
The ch <- &msg send in readLoop had no timeout protection. If a caller cancelled its context and the response arrived simultaneously, the channel could theoretically block readLoop. Add select+default to drop the message harmlessly in this edge case.
Instead of discarding the process exit error with _, append it to the existing stderrBuf (crash diagnostics buffer). This makes the exit reason available for debugging without writing to os.Stderr, which would interfere with TUI rendering.
…ssage - Replace hardcoded clangd_19.1.2 path with clangdInstallDir() that glob-matches any clangd_* version directory - Make Close() wait up to 5s for an in-progress Start() to set c.conn before checking it, preventing a process leak when Close races with Start between the lock-release and conn-assignment windows - Improve the Start() spin-loop error message to indicate the concurrent attempt completed without the server becoming ready
Increase max scan depth from 2 to 5 and add a file-count cap of 200. The scan stops when either limit is reached, providing better coverage for deep monorepos while bounding the cost of large directories. Files beyond the limits are still covered by lazy start.
c.startupTimeout was set by newClient() but never actually applied. Create a context.WithTimeout wrapping the callers context so the initialize handshake has a bounded deadline even when called with context.Background().
lsp_workspace_symbol only queried readyLanguages(), missing languages whose server binary was installed but not yet started. Call ensureAllAsync() before iterating ready servers so the next workspace symbol search covers all configured languages.
Replace the 50ms polling loop in Start()'s concurrent-wait path with a channel-based approach. Start() creates readyCh before releasing the initial lock and closes it when the initialize handshake succeeds. Waiting goroutines block on <-readyCh with a startupTimeout fallback. Also add a //go:build integration smoke test that starts gopls, initializes a workspace, and calls DocumentSymbols to verify the end-to-end lifecycle.
…, timeout wrapper - Actually add readyCh field and replace Start() spin-loop with channel-based blocking (the previous commit's message was incorrect) - Clear openDocs map in Close() to prevent unbounded memory growth - Filter EnsureAllAsync to skip servers whose binary was not found or that have no matching workspace files - Wrap clientForServer's Start() call with startupTimeout when the caller's context deadline is shorter than the configured timeout
backgroundStart already creates its own context with srv.StartupTimeout(), so the deadline guard added by the previous commit is a no-op. The guard is only needed in clientForServer where the caller's context may be shorter.
|
本次 push 包含自初版 20fadbd 以来 13 个修复/改进提交，主要内容： 核心架构改进
并发与稳定性
传输与协议
代码质量
测试
|
|
CI aside. Overall LGTM. Please fix the CI failure. |
|
我是windows下开发的，问题查了很久，这些test的问题应该是主干版本带来的。go run ./cmd/dev test 是linux的入口，windows应该是go run ./cmd/dev test-windows。如果ci在windows下执行前者必然错误，但在linux这些问题应该是解决的。 主干为什么 CI 能过：Windows CI 跑的是白名单子集（不含那 5 个测试），Linux CI 跑全量（那 5 个在 Linux 上本来就过）。两个口径下 main 都绿——但"Windows 全量"这个口径从来没人跑，我理解也不应该跑。我现在能做的是把 internal/lsp 加进 test-windows 白名单。 |
|
搞清楚了，lsp的测试在linux上没过。稍晚提交修复。 |
- Gate TestPathToURI_Windows and TestURIToPath_Windows behind isWindows(), mirroring the existing TestURIToPath_Unix skip - TestLoadLSPConfig_FileError: use a directory instead of a Windows-only invalid path; reading a directory fails with a non-IsNotExist error on all platforms - TestFindServerForConfig_KnownInstallDir: write the fake executable with 0o755 so exec.LookPath matches it on Unix - TestIsRustupProxy_ValidBinary: skip when the discovered rust-analyzer is a rustup proxy stub whose --version fails (the case on CI runners)
|
Fixed the Linux CI failures — the 5 failing
Also added Verified on both Windows (full |
|
在做linux的功能充分验证，先不要合并。 |
Correctness: - Default python server used the pyright type-check CLI instead of pyright-langserver, so Python LSP never worked when installed via npm (the VSCode Pylance fallback masked this on Windows); the fallback also duplicated --stdio args - isRustupProxy only guarded the install-dir fallback while rustup puts its proxy shim on PATH, so the most common case bypassed the check - RegisterServer left dangling extension claims on conflict, breaking the default-restore path and enabling a nil-deref in FindServerForConfig - ensureDocumentOpen re-sent didOpen for changed files (LSP forbids this; servers kept stale buffers and returned outdated positions); documents are now closed before re-open, bounded by an LRU (didClose on evict), and cleared on Close so a restarted server gets fresh didOpen - languageIDFromPath ignored the configured extensionToLanguage mapping, was case-sensitive, and mis-parsed dotted directories - EnsureAllAsync was a no-op before Warmup populated extCache, and the workspace scan let build output (build/dist/target) exhaust the file quota before reaching sources - A symbol outline could push a full read_file result over the envelope limit and degrade it to outline mode; the outline is now dropped first Responsiveness: - ensureAsync/backgroundStart were synchronous despite their names: a plain read_file could block up to 30s on a server handshake. Starts now run in the background behind an exclusive claim (tryClaimStart), and ClientForFileQuick waits event-driven on the attempt channel - Discovery results are memoized (foundCache) with a retry TTL for negative entries, replacing a full PATH/install-dir rescan per call - All LSP requests now carry timeouts (10s tools, 3s read_file outline); previously only lsp_document_symbol had one and a ready-but-indexing server could hang tools indefinitely - Start failures now close readyCh so concurrent waiters are released immediately instead of sitting out the full startup timeout Process lifecycle: - All start paths (background, synchronous lazy-start, crash-restart) go through the closed/startCtx/startWG gate: Close cancels in-flight handshakes, waits them out, and refuses new spawns - Client shutdown kills the whole process tree (AttachCommandCleanup was previously attached before cmd.Start, making the job object a no-op, and Cleanup was never called) and waits until the process is reaped — an unreaped process still holds directory handles on Windows - Manager.Close closes servers concurrently outside the manager lock Tests follow TDD (failing test first) and are hermetic: process tests impersonate servers via the test binary (WHALE_LSP_FAKE_MODE), verified on Windows and Linux (WSL).
Same skip-if-absent pattern as the gopls smoke test: each starts a real server and exercises initialize -> didOpen -> documentSymbol -> shutdown. Verified on Linux (WSL Ubuntu) against gopls v0.23.0, pyright 1.1.411, and rust-analyzer 0.3.2971, alongside a fully green unit suite with all three servers on PATH.
go mod tidy: the package is imported directly, not transitively.
Make LSP opt-in: the former always-on startup leaves users exposed to background server discovery and warmup on every session regardless of whether they need code intelligence. Default Config.LSPEnabled to false; enable via /lsp on (persisted to whale.toml) or the config file key [lsp] enabled = true. - Config field LSPEnabled (bool, default false) mirrors WorkflowsEnabled - File schema: [lsp] enabled = true/false in whale.toml (FileLSPConfig) - app_tools_init gate: skip manager creation and tool registration when disabled, producing zero background work - /lsp command with sub-commands status / on / off / sample (prototype restored from 8d590776, which was lost during the July 15 rewrite) - LSPStatusInfo, SetLSPEnabledPersist, LSPWriteSampleConfig accessors - Fix dangling lsp_status reference in error messages → /lsp - isRustupProxy timeout raised from 3s to 15s (a fresh 20MB binary under AV scan was misclassified as a proxy stub) - Test flake: loosen release-of-concurrent-waiter threshold to 6s
|
New push after a 12-phase code review and fix campaign:
Verified locally with the same dual-platform CI matrix (LF clean checkout on |
|
Linux and Windows verification is complete — the local CI replication (LF clean checkout on WSL, full |
|
I changed the pull request workflow to Will run the workflow automatically. |
clangdInstallDir returns "" when no clangd installation exists, and the empty string was appended unconditionally — triggering a test failure on CI runners that lack clangd.
- Filter empty clangdInstallDir so knownInstallDirs never contains empty strings (breaks on CI runners without clangd installed) - Relax TestClientForFileQuickFailsFastOnStartFailure threshold from 2.5s to 4.5s to tolerate load-induced jitter
|
The Windows CI job surfaced two issues that never reproduced locally because the developer machine differs from the GitHub runner:
Verified both fixes with a fresh local |
2 participants
Summary
Adds a Language Server Protocol (LSP) client and manager that enables
code intelligence tools by launching language servers as subprocesses.
New Features
a language server is still indexing or unavailable, tools return friendly
errors suggesting retry or fallback to grep + read_file.
Configuration
Language servers are configured via
lsp.jsonin the data directory.The file is optional; if absent, built-in defaults are used for Go,
Python, TypeScript, Rust, C/C++, HTML/CSS/JSON, YAML, and Vue.
Server discovery order: PATH lookup, known install directories (Go
bin, Cargo bin, npm global, LLVM), and VS Code extension directories.
Architecture
internal/lsp/: JSON-RPC 2.0 client, protocol types, server discovery,and manager that coordinates per-language server lifecycles
internal/tools/catalog_lsp.go: tool registration and handlers thatdelegate to the LSP manager
background when matching workspace files are detected
read_filefull-read results include asymbol_outlinefield whenLSP is available
Testing
40 unit tests covering:
Test mocks (
lspClient,lspToolProvider) are injected via theunexported
lspOverridefield on Toolset, avoiding the need for reallanguage servers in tests.
Documentation
go doc ./internal/lspprovides full package documentation coveringconfiguration format with an example, server discovery, lifecycle, and
tool exposure. Type-level docs are present for Manager, Client,
LSPConfig, and ServerConfig. Method-level docs cover Warmup,
ReadyClientForFile, ClientForLanguage, and other key methods.
Changed Files
Modified:
internal/app/app_tools_init.go: load lsp.json, create Manager, warmupinternal/tools/catalog.go: register lspTools in the tool listinternal/tools/view.go: inject symbol_outline into read_file resultsinternal/tools/toolset.go: add LSP-related fields and settersNew:
internal/lsp/: LSP client, manager, config, discovery, protocol typesinternal/tools/catalog_lsp.go: tool registration and handlersinternal/tools/catalog_lsp_test.go: 40 unit testsValidation
go vet ./internal/lsp/... ./internal/tools/...— cleango test ./internal/tools/ -run "TestLSP|TestFormatLSPResult|TestWriteDocumentSymbol|TestRunLSPPositionOp" -count=1— 40/40 PASSUser-visible impact
read_fileresults now include asymbol_outlinefield when LSP isavailable for the file's language
one language server is configured and available
Breaking changes
None. LSP is opt-in: if no
lsp.jsonexists and no language serversare installed, the feature has zero effect on existing behavior.
Notes
To test manually, ensure at least one supported language server is
installed (e.g.,
goplsfor Go) and open a workspace with matchingsource files. The LSP manager warms up in the background and the tools
become available automatically.