Skip to content

fix(lsp): a tool call uses the session's server - #116

Merged
yusiwen merged 7 commits into
masterfrom
fix/lsp-tool-session
Oct 4, 2026
Merged

fix(lsp): a tool call uses the session's server#116
yusiwen merged 7 commits into
masterfrom
fix/lsp-tool-session

Conversation

@yusiwen

@yusiwen yusiwen commented Oct 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

What changed

  • lsp/touch.go: new Ensure(filePath) — starts the session's language server through the same
    lazyStart SyncFile already uses, under the same lock contract.
  • lsp/tools.go: with a workspace configured, a tool call promotes that server and uses it; the
    per-call server below stays the second attempt, now rooted at the project directory. The
    change is additive: maybe promote → if IsAvailable() → persistent, else → per-call.
  • lsp/tools_test.go: two gated tests (LSP_TEST=1) pin both routes.
  • tui/testdata/scenarios/lsp-tools.scenario, plus an lsp mode in the stub and a
    make test-tui-lsp that runs every lsp-*.scenario (the suite skips them by that pattern):
    the screen-level guard for [lsp] Tool calls start a server per call and never tell /diagnostics about it #114.
  • Both lsp-* scenarios wipe their throwaway HOME with chmod -R u+w …; rm -rf …, outside the
    && chain that starts the stub — fixes [tui] The LSP scenarios fail on their second run: a read-only module cache in the throwaway HOME #115.

Why

The per-call server was the only route a session without a configured workspace had, and it was
broken: gopls answers LSP error 0: no views when initialize names a file as the workspace.
Because main.go registers the four LSP tools unconditionally and only calls lsp.Init when
lsp.enabled is set, the shipped default failed every LSP tool call.

Two alternatives were rejected. Fixing only the rooting would have left /diagnostics reporting
"LSP not available (set lsp.enabled=true in config.json)" — wrong advice — and paid a server start
per call. Returning Ensure's error instead of falling through would have ended the call where the
per-call server could still answer: a workspace is configured for the session as a whole, so a file
belonging to another project, or one whose language has its own server, would lose the route that
served it before.

Verification

  • LSP_TEST=1 go test ./lsp/ -count=1 → ok, both new tests pass (gopls 0.23.0 on PATH).
  • Gate can fail — mutation 1: if Initialised() → if false makes
    TestToolCallPromotesTheSessionServer fail with "a tool call left the session without a client"
    and "server started 0 times across two calls". Re-checked against the final shape.
  • Gate can fail — mutation 2: Initialize("file://"+canonicalPath(rootDir)) → Initialize(fileURI)
    makes TestToolCallWithoutWorkspaceServesTheRequest fail with LSP error 0: no views.
  • Gate can fail — scenario: with lsp/tools.go + lsp/touch.go from before this branch and the
    binary rebuilt, lsp-tools.scenario sees lsp_symbols and then times out waiting for
    No LSP diagnostics. (exit 1).
  • Pre-fix measurement, cold session with a workspace configured: tool result 0 bytes,
    err=LSP error 0: no views, IsAvailable=false. After: 223 bytes of symbols,
    IsAvailable=true, and one LSP: gopls started for … across two calls.
  • make test-tui-lsp twice in a row → exit 0 both times, ok: 14 steps per scenario, on the
    final revision. Before this branch the second run died on connection refused ([tui] The LSP scenarios fail on their second run: a read-only module cache in the throwaway HOME #115).
  • go test ./... -count=1 → 13 packages ok. go vet ./... clean. make test-tui-scenarios →
    26 scenarios ok, 3 skipped (live-answer, lsp-diagnostics, lsp-tools).

Honest scope

  • One server: gopls 0.23.0 from this machine. The other entries in DefaultConfigs were not
    exercised locally; CI's lsp job runs the gated tests for gopls and typescript.
  • The per-call route is measured working for a file whose own directory is its project root. The
    case "persistent client asked about another project's file" was not measured, and nothing is
    claimed about it.
  • /diagnostics's listing outcome still needs real diagnostics present, and stays uncovered.

Follow-ups

yusiwen added 4 commits October 5, 2026 02:08
@yusiwen
fix(lsp): a tool call uses the session's server
1cf6edc 
lsp/tools.go started a language server per call and never told the package,
so IsAvailable() stayed false after a successful call: /diagnostics reported
"LSP not available (set lsp.enabled=true in config.json)" — advice that is
wrong when LSP is on — and every call paid another start.

That per-call server was also initialized with the *file* as its workspace
root, which gopls answers with "LSP error 0: no views". Because main.go
registers the four LSP tools unconditionally and only calls lsp.Init when
lsp.enabled is set, the shipped default reached exactly that path: all four
tools failed outright, they did not merely leave a stale status line.

- lsp/touch.go: Ensure(filePath) starts the session's server through the
  lazyStart SyncFile already uses, under the same lock contract.
- lsp/tools.go: with a workspace configured, promote that server and use it;
  without one, keep the per-call server, now rooted at the project directory.

Measured with LSP_TEST=1 and gopls 0.23.0, cold session, no file read:
  before: 0 bytes, err "LSP error 0: no views", IsAvailable=false
  after:  223 bytes of symbols, IsAvailable=true, and one "started for"
          line across two tool calls.

The two gated tests in lsp/tools_test.go pin both routes; the scenario
lsp-tools.scenario pins the screen a user sees.
@yusiwen
test(tui): keep the LSP scenario wipe working on a second run
1faca33 
The scenario runs the binary with HOME=/tmp/tinyscen-home-lsp, and gopls
resolves the workspace through that HOME: it fills $HOME/go/pkg/mod, whose
directories are read-only. `rm -rf` needs write permission on them, so the
wipe exited 1, the `&&` chain stopped before the stub was started, and the
second run of the scenario died on "connection refused" — an error about a
server nobody launched, not about LSP.

Measured after one run: 96M under the throwaway HOME, `rm -rf` exit 1,
`chmod -R u+w` then `rm -rf` exit 0.

Drop the write permission first, and keep the wipe outside the && chain so a
wipe that still fails cannot stop the stub from starting.

Refs #115
@yusiwen
test(tui): a gated scenario for the LSP tool route
78b4e72 
lsp-diagnostics starts the server with a file read, which is the route #111
fixed. This one reaches the same status line from the other side: the stub
asks for lsp_symbols (the name lsp.ToolFactory registers) and nothing reads or
writes a file, so "No LSP diagnostics." is only reachable if the tool call
promoted the session's server — the regression guard for #114.

make test-tui-lsp now runs every lsp-*.scenario instead of naming one file, and
the scenario suite skips them by that pattern: a second LSP scenario that only
the file name knew about would have been skipped in silence.

Refs #114
@yusiwen
docs(tui): record the LSP tool scenario and the wipe rule
93d415d 
docs/tui-verification.md gains the lsp-tools paragraph (what makes its
assertion about the tool route, and why the stub must name the registered
tool), and both documents gain the wipe rule: an lsp-* scenario drops the
write permission before removing its throwaway HOME, because gopls leaves a
read-only module cache under it (issue #115).

Refs #114, #115
@yusiwen yusiwen added bug Something isn't working tests Test coverage and test infrastructure labels Oct 4, 2026
yusiwen added 3 commits October 5, 2026 02:15
@yusiwen
ci(lsp): the TUI LSP step drives two scenarios now
8216fb9 
make test-tui-lsp runs every lsp-*.scenario: lsp-diagnostics starts the
server with a file read, lsp-tools with an LSP tool call. Name the step and
its comment for what it runs.

Refs #114
@yusiwen
docs(lsp): attach each session-state doc comment to its function
809bf07 
The "IsAvailable returns true if LSP is initialized and not broken." line
was left above Initialised's own doc block when #111 inserted it, so it
documented nothing while IsAvailable went undocumented.

Refs #114
@yusiwen
fix(lsp): keep the per-call server as the second attempt
cc1e728 
Returning Ensure's error ended the call where the per-call server could still
have answered: a workspace is configured for the session as a whole, so a file
belonging to another project — or one whose language has its own server — lost
the route that served it before. Promote the session's server when that works
and let the per-call server below be the second attempt, which also keeps this
change additive to the behaviour it replaces.

Refs #114
@yusiwen
yusiwen merged commit 6e1a438 into master Oct 4, 2026
8 checks passed
@yusiwen
yusiwen deleted the fix/lsp-tool-session branch October 4, 2026 18:22
This was referenced Oct 4, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Something isn't working tests Test coverage and test infrastructure

Projects

None yet

Milestone

No milestone

1 participant

@yusiwen