fix: LSP tool startup reliability, shutdown hangs, and refactor into focused files

[dgageot]

1. Server won't restart after a crash. startLocked() checked h.cmd != nil to decide if the server was running, but h.cmd stays non-nil after the process dies. A crashed server could never be restarted — every subsequent call got "LSP server already running". Fixed by adding a processDone channel that tracks actual process liveness.

2. stopLocked() hangs forever. It called sendRequestLocked("shutdown") synchronously, which blocks on reading stdout. If the server was unresponsive or already dead, this blocked indefinitely — and so did cmd.Wait() after it. Fixed by running the shutdown handshake in a goroutine with a 5-second timeout, then killing the process via context cancellation.

3. Data race on the stderr buffer. os/exec writes to a bytes.Buffer from one goroutine while readNotifications reads and resets it from another. bytes.Buffer is not thread-safe. Fixed by introducing a mutex-protected lockedBuffer.

Assisted-By: docker-agent

[docker-agent]

Review Summary

Assessment: 🟡 NEEDS ATTENTION

This PR successfully addresses the three critical issues described (server restart, shutdown hangs, and stderr data race). The refactoring into focused files improves code organization and the core fixes are well-designed.

However, the new lsp_files.go file introduces 3 medium-severity issues in the text edit handling logic:

1. Missing validation for malformed edit ranges (startLine > endLine)
2. TOCTOU race condition in file edit application
3. Silent error handling that hides LSP protocol violations

These issues could lead to file corruption when applying LSP edits. While the core concurrency fixes are solid, the file editing logic needs additional validation and error handling.

Findings

3 issues found (2 CONFIRMED, 1 LIKELY)

• 0 high severity
• 3 medium severity
• 0 low severity

[docker-agent]

Missing validation: startLine > endLine could corrupt file

The applyTextEdit function does not validate that startLine <= endLine. If the LSP server sends malformed edits with startLine > endLine, line 207 creates lines[:startLine] followed by lines[endLine+1:], which skips the region between endLine and startLine, corrupting the file.

Recommendation: Add validation before processing the edit:

if startLine > endLine || (startLine == endLine && startChar > endChar) {
    return nil, fmt.Errorf("invalid edit range: start (%d:%d) > end (%d:%d)", startLine, startChar, endLine, endChar)
}

[docker-agent]

TOCTOU race: file may change between LSP edit generation and application

applyTextEditsToFile reads the file at line 148 and writes back at line 168 without locking or versioning. If another process modifies the file between these operations, edits will be applied to the wrong base content, potentially corrupting the file.

Recommendation: Consider one of these approaches:

1. Use file locking (e.g., flock on Unix) to prevent concurrent modifications
2. Implement document versioning to detect if the file changed
3. Document this as a known limitation if single-user access is assumed

[docker-agent]

Silent clamping of out-of-bounds edits may hide bugs

Lines 183-189 silently clamp out-of-bounds edit ranges instead of returning an error. If endLine is far beyond the file end, it's clamped to len(lines)-1, causing the edit to be applied to an unintended location. The LSP protocol requires edits to be within document bounds; out-of-bounds edits indicate a server bug or protocol violation.

Recommendation: Return an error instead of silently clamping:

if endLine >= len(lines) {
    return nil, fmt.Errorf("edit range out of bounds: endLine %d >= file length %d", endLine, len(lines))
}

[aheritier]

🎯 Comprehensive LSP Tools Testing Results

We conducted extensive testing of all 13 LSP tools across Go, Python, and TypeScript/JavaScript language servers. Here are our findings and configuration:

📊 Success Rates by Language

Language Server	Success Rate	Working Tools	Status
Go (gopls)	92% (12/13)	Most comprehensive	🟢 Excellent
TypeScript/JS	85% (11/13)	Great balance	🟢 Very Good
Python (pylsp)	46% (6/13)	Basic but solid	🟡 Limited

🔧 Tool-by-Tool Compatibility Matrix

LSP Tool	Go	Python	TypeScript	Notes
lsp_workspace	⚠️ Limited	✅ Full	✅ Full	Go: server already running limitation
lsp_document_symbols	✅ Full	✅ Full	✅ Full	Universally supported
lsp_workspace_symbols	✅ Full	❌ Not supported	✅ Full	pylsp limitation
lsp_hover	✅ Full	⚠️ Partial	✅ Full	Python: limited for imports
lsp_definition	✅ Full	⚠️ Partial	✅ Full	Python: local symbols only
lsp_references	✅ Full	✅ Full	✅ Full	Universally supported
lsp_diagnostics	✅ Full	✅ Full	✅ Full	Universally supported
lsp_code_actions	❌ Column error	⚠️ No actions	❌ Server bug	Implementation issues
lsp_call_hierarchy	✅ Full	❌ Not supported	✅ Full	pylsp limitation
lsp_type_hierarchy	✅ Full	❌ Not supported	❌ Not supported	Advanced feature
lsp_implementations	✅ Full	❌ Not supported	✅ Full	pylsp limitation
lsp_signature_help	✅ Full	❌ No help	✅ Full	Python: positioning issues
lsp_inlay_hints	✅ Works*	❌ Not supported	✅ Works*	*No hints available by default

🌟 Key Discoveries

Go (gopls) - Most Capable

• Outstanding performance: Found 480+ error implementations, 300+ io.Closer implementations
• Deep ecosystem integration: lsp_implementations reveals incredible Go ecosystem depth
• Only major issue: lsp_code_actions has persistent "column beyond end of line" errors
• Best for: Complex codebases requiring extensive type system exploration

TypeScript/JS (typescript-language-server) - Excellent Developer Experience

• Rich documentation: Hover info includes MDN links and comprehensive type information
• Strong workspace analysis: Excellent cross-file symbol and reference finding
• Modern tooling: Great integration with web development workflows
• Limitations: Code actions server bug, type hierarchy not supported for JavaScript files

Python (pylsp) - Reliable Basics

• Solid core features: Essential navigation (symbols, hover, definition, references) work consistently
• Major gaps: Missing 5 advanced LSP features (hierarchies, workspace symbols, implementations)
• Recommendation: Consider Pyright/Pylance for advanced Python development
• Best for: Simple Python projects with basic code intelligence needs

🚨 Common Issues Found

1. LSP Server State Management: All agents encountered connection issues requiring server restarts
2. Code Actions Bugs: Both Go and TypeScript have implementation issues in this feature
3. Path Resolution: Absolute paths work more reliably than relative paths
4. Server Capability Variations: Significant differences in what each LSP server supports

⚙️ Complete Multi-Language Agent Configuration

Here's our full working configuration from examples/lsp-code-reviewer.yaml:

Click to expand full configuration

#!/usr/bin/env docker agent run

# Multi-language code development with LSP intelligence.
#
# An orchestrator (planner) has read-only LSP access to Go, TypeScript, and
# Python so it can understand the entire codebase before delegating work.
# Language-specialist developer agents have full LSP + filesystem + shell
# access to implement changes.

models:
  default:
    provider: anthropic
    model: claude-sonnet-4-0
    max_tokens: 64000

agents:
  root:
    model: default
    description: Technical lead that reads the codebase across all languages, plans tasks, and delegates to language-specialist developers.
    instruction: |
      You are a technical lead and planner. You have read-only access to the
      entire codebase via LSP tools for Go, TypeScript, and Python.

      ## Your Role
      You PLAN and DELEGATE. You never write code yourself.

      ## Workflow
      1. **Understand the request** — use LSP and filesystem tools to explore
         the codebase. Start with lsp_workspace on each language server to
         discover capabilities.
      2. **Analyse** — use lsp_workspace_symbols, lsp_references,
         lsp_call_hierarchy, lsp_diagnostics, etc. to build a complete picture
         of what needs to change and where.
      3. **Plan** — use the think tool to reason about the approach, break the
         work into discrete tasks per language, and identify dependencies
         between them.
      4. **Delegate** — dispatch tasks to the right developer agent:
         - **go-developer**: anything touching .go files
         - **ts-developer**: anything touching .ts/.tsx/.js/.jsx files  
         - **py-developer**: anything touching .py files
         Run independent tasks in parallel with background_agents.
      5. **Verify & synthesise** — review the developers' results, run
         additional read-only checks if needed, and present a summary.

    sub_agents: [go-developer, ts-developer, py-developer]
    toolsets:
      - type: think
      - type: background_agents
      - type: filesystem
        tools: [read_file, read_multiple_files, list_directory, directory_tree, search_files_content]

      # Go LSP — read-only tools only
      - type: lsp
        command: gopls
        version: "golang/tools@v0.21.0"
        file_types: [".go"]
        tools: [lsp_workspace, lsp_hover, lsp_definition, lsp_references, lsp_document_symbols, lsp_workspace_symbols, lsp_diagnostics, lsp_code_actions, lsp_call_hierarchy, lsp_type_hierarchy, lsp_implementations, lsp_signature_help, lsp_inlay_hints]

      # TypeScript/JavaScript LSP — read-only tools only  
      - type: lsp
        command: typescript-language-server
        args: ["--stdio"]
        file_types: [".ts", ".tsx", ".js", ".jsx"]
        tools: [lsp_workspace, lsp_hover, lsp_definition, lsp_references, lsp_document_symbols, lsp_workspace_symbols, lsp_diagnostics, lsp_code_actions, lsp_call_hierarchy, lsp_type_hierarchy, lsp_implementations, lsp_signature_help, lsp_inlay_hints]

      # Python LSP — read-only tools only
      - type: lsp  
        command: pylsp
        file_types: [".py"]
        tools: [lsp_workspace, lsp_hover, lsp_definition, lsp_references, lsp_document_symbols, lsp_workspace_symbols, lsp_diagnostics, lsp_code_actions, lsp_call_hierarchy, lsp_type_hierarchy, lsp_implementations, lsp_signature_help, lsp_inlay_hints]

  go-developer:
    model: default
    description: Expert Go developer with full LSP code intelligence, filesystem, and shell access.
    toolsets:
      - type: lsp
        command: gopls
        version: "golang/tools@v0.21.0"
        file_types: [".go"]
      - type: filesystem
      - type: shell
      - type: todo

  ts-developer:
    model: default
    description: Expert TypeScript/JavaScript developer with full LSP code intelligence, filesystem, and shell access.
    toolsets:
      - type: lsp
        command: typescript-language-server
        args: ["--stdio"]
        file_types: [".ts", ".tsx", ".js", ".jsx"]
      - type: filesystem
      - type: shell  
      - type: todo

  py-developer:
    model: default
    description: Expert Python developer with full LSP code intelligence, filesystem, and shell access.
    toolsets:
      - type: lsp
        command: pylsp
        file_types: [".py"]
      - type: filesystem
      - type: shell
      - type: todo

🎯 Recommendations

For Go Development

• Use everything except lsp_code_actions (due to column positioning bug)
• Power tools: lsp_implementations and lsp_type_hierarchy provide incredible ecosystem insights
• Core workflow: hover, definition, references, workspace_symbols for navigation

For TypeScript/JavaScript Development

• Excellent overall experience with rich tooling support
• Standout features: hover with MDN docs, signature_help, comprehensive workspace symbols
• Great for: Modern web development with excellent project navigation

For Python Development

• Stick to the 6 working core tools for reliable functionality
• Essential tools: document_symbols, hover, definition, references, diagnostics
• Upgrade path: Consider Pyright for advanced features in larger Python projects

📋 Testing Methodology

Our testing was conducted by three specialized developer agents, each systematically testing all 13 LSP tools:

• ✅ Comprehensive coverage: All tools tested with real code examples
• ✅ Detailed documentation: Commands, results, and limitations recorded
• ✅ Server management: Complex LSP connection issues resolved and documented
• ✅ Practical recommendations: Based on real-world usage patterns

This provides a definitive reference for LSP tool capabilities across the three major language ecosystems in Docker Agent! 🚀

[aheritier]

Bug: Self-Deadlock in lspHandler.format() → Frozen Process

Root Cause

format() acquires h.mu and then calls NotifyFileChange(), which attempts to acquire the same h.mu. Since Go's sync.Mutex is not reentrant, the goroutine blocks forever waiting on itself.

Call Chain

(*LocalRuntime).RunStream.func1()          loop.go:353
  (*codeModeTool).Tools.func1()            codemode.go:82
    (*codeModeTool).runJavascript()        exec.go:66
      (*lspHandler).format()              lsp.go:499  ← acquires h.mu
        (*lspHandler).NotifyFileChange()  lsp_files.go:105  ← tries to re-acquire h.mu → DEADLOCK

Relevant Code

lsp.go (format):

func (h *lspHandler) format(ctx context.Context, args FileArgs) (*tools.ToolCallResult, error) {
    // ...
    h.mu.Lock()
    defer h.mu.Unlock()   // ← holds lock for entire function body

    h.sendRequestLocked("textDocument/formatting", ...)
    // ...
    applyTextEditsToFile(args.File, edits)

    h.NotifyFileChange(ctx, uri)  // ← line 527, called while h.mu is still held
}

lsp_files.go (NotifyFileChange):

func (h *lspHandler) NotifyFileChange(_ context.Context, uri string) error {
    // ...
    h.openFilesMu.Lock()
    h.openFiles[uri]++
    h.openFilesMu.Unlock()

    h.mu.Lock()   // ← line 105: BLOCKS — same goroutine already holds h.mu
    defer h.mu.Unlock()
    // never reached
}

Goroutine Dump Evidence

The SIGQUIT dump showed goroutine 24314 blocked for 189 minutes:

goroutine 24314 [sync.Mutex.Lock, 189 minutes]:
internal/sync.(*Mutex).lockSlow(0x317d2dbf3560)
sync.(*Mutex).Lock(...)
github.com/docker/docker-agent/pkg/tools/builtin.(*lspHandler).NotifyFileChange(0x317d2dbf3560, ...)
    lsp_files.go:105
github.com/docker/docker-agent/pkg/tools/builtin.(*lspHandler).format(0x317d2dbf3560, ...)
    lsp.go:527

Fix

Release h.mu immediately after sendRequestLocked returns, before the disk write and re-notification. Neither applyTextEditsToFile nor NotifyFileChange need the lock at call time — NotifyFileChange acquires it internally.

func (h *lspHandler) format(ctx context.Context, args FileArgs) (*tools.ToolCallResult, error) {
    // ...
    h.mu.Lock()
    result, err := h.sendRequestLocked("textDocument/formatting", map[string]any{...})
    h.mu.Unlock()  // release before disk I/O and re-notification

    if err != nil { ... }
    // ... parse edits, applyTextEditsToFile, NotifyFileChange as before ...
}

This narrows the critical section to just the LSP wire request, which is the only part that actually needs mutual exclusion.

[aheritier]

Bug Reports from Multi-Agent LSP Testing (jdtls + typescript-language-server)

We tested all 15 LSP tools across 4 agents (each with different LSP configs) in a multi-agent YAML setup with jdtls (Java) and typescript-language-server (TypeScript). Found 2 bugs — both present on this PR branch.

Testing Setup

# 6 agents, 4 with LSP:
root:                 jdtls (-data data/jdtls-cfp-cr)  + typescript-language-server --stdio
spring-boot-engineer: jdtls (-data data/jdtls-cfp-sbe)
angular-engineer:     typescript-language-server --stdio
code-reviewer:        jdtls (-data data/jdtls-cfp-cr)  + typescript-language-server --stdio
test-automator:       jdtls (-data data/jdtls-cfp-cr)  + typescript-language-server --stdio

Results: Every agent gets "LSP initialization failed: failed to start LSP server: LSP server already running" on ALL tool calls — except angular-engineer whose TypeScript LSP works (9/13 tools ✅).

---

Bug 1 (P1): ensureInitialized() fails when process was eagerly started by StartableToolSet.Start()

Symptom: "LSP initialization failed: failed to start LSP server: LSP server already running" on every tool call.

Root cause: State mismatch between startLocked() and ensureInitialized():

1. At boot, emitToolsProgressively() → StartableToolSet.Start() → LSPTool.Start() → startLocked() starts the OS process: sets h.cmd != nil, but h.initialized stays false (the LSP initialize handshake is not done in startLocked()).
2. On first tool call (e.g., lsp_hover), ensureInitialized() checks h.initialized.Load() && h.isProcessAlive() → false && true → falls through.
3. ensureInitialized() unconditionally calls startLocked() (line 262), which finds h.cmd != nil with a live process and returns "LSP server already running" (line 91).
4. The error propagates: "failed to start LSP server: LSP server already running".

Code trace (all in pkg/tools/builtin/lsp_handler.go on this branch):

// Line 248-262: ensureInitialized() — the missing guard
func (h *lspHandler) ensureInitialized() error {
    if h.initialized.Load() && h.isProcessAlive() { return nil }  // line 249: false && true → miss
    h.mu.Lock()
    defer h.mu.Unlock()
    if h.initialized.Load() && h.isProcessAlive() { return nil }  // line 257: same
    // ❌ BUG: unconditionally calls startLocked() even when process is alive but not yet initialized
    if err := h.startLocked(); err != nil {                       // line 262
        return fmt.Errorf("failed to start LSP server: %w", err)
    }
    // ... LSP handshake never reached ...
}

// Line 84-91: startLocked() — the guard that fires
func (h *lspHandler) startLocked() error {
    if h.cmd != nil {
        select {
        case <-h.processDone: h.cleanupLocked()     // process dead → cleanup ok
        default: return errors.New("LSP server already running") // process alive → ERROR
        }
    }
    // ...
}

Why angular-engineer works: It has only 1 LSP backend (typescript-language-server). Sub-agents may not trigger EmitStartupInfo, so Start() is never called eagerly — ensureInitialized() finds h.cmd == nil on first call and correctly starts + initializes.

Proposed fix — ensureInitialized() line 262:

// Before (line 261-263):
// (Re)start the server. startLocked handles cleaning up a dead process.
if err := h.startLocked(); err != nil {
    return fmt.Errorf("failed to start LSP server: %w", err)
}

// After:
// Only (re)start if the process is not running. It may have been
// pre-started by Start() (via StartableToolSet during EmitStartupInfo)
// without completing the LSP initialize handshake.
if !h.isProcessAlive() {
    if err := h.startLocked(); err != nil {
        return fmt.Errorf("failed to start LSP server: %w", err)
    }
}

---

Bug 2 (P2): codeActions() sends null diagnostics, crashes TypeScript Language Server

Symptom: lsp_code_actions returns LSP error -32603: Cannot read properties of null (reading 'filter') from TypeScript Language Server.

Root cause: In pkg/tools/builtin/lsp.go line 464, rangeDiags is declared as a nil slice:

var rangeDiags []lspDiagnostic  // nil when no diagnostics match

When no diagnostics are in the requested line range, rangeDiags stays nil. Go's json.Marshal(nil slice) produces null, so the request is sent as:

{ "context": { "diagnostics": null } }

The LSP spec requires diagnostics to be Diagnostic[] (an array). TypeScript Language Server's JavaScript code calls .filter() on this value, which crashes on null.

Proposed fix — lsp.go line 464:

// Before:
var rangeDiags []lspDiagnostic

// After:
rangeDiags := make([]lspDiagnostic, 0)  // empty slice → JSON [] instead of null

---

Full Test Results Matrix

Agent	Java (jdtls)	TypeScript (tsserver)
root (coordinator)	❌ Bug 1	❌ Bug 1
spring-boot-engineer	❌ Bug 1	N/A
angular-engineer	N/A	✅ 9/13 tools (Bug 2 affects lsp_code_actions)
code-reviewer	❌ Bug 1	❌ Bug 1
test-automator	❌ Bug 1	❌ Bug 1

Tools verified working on angular-engineer (TypeScript): lsp_workspace, lsp_document_symbols, lsp_workspace_symbols, lsp_diagnostics, lsp_hover, lsp_definition, lsp_references, lsp_call_hierarchy, lsp_implementations, lsp_signature_help.

Tools broken: lsp_code_actions (Bug 2), lsp_type_hierarchy (tsserver doesn't implement textDocument/prepareTypeHierarchy).