🐛 fix: answer server-initiated requests and frame by byte length (unblocks TypeScript 7's native LSP) - #52
Open
sudhirj wants to merge 2 commits into
Open
Conversation
Content-Length counts bytes, but the transport accumulated stdout into a JS string and sliced it by that count. The first message carrying a multi-byte character reads short, so the next frame is parsed from the wrong offset and every message after it fails — the parser never resynchronises, and the session dies with a run of "Failed to parse LSP message" errors. This is reachable with any server whose output is not pure ASCII. TypeScript 7's native LSP hits it immediately: its own timing logs are of the form "handled method 'textDocument/hover' (11) in 680.392µs". Buffer the raw bytes and index into them, so declared and consumed lengths agree. Also accept a lowercase/spaced Content-Length header, and stop treating a chunk boundary that falls inside a multi-byte character as a parse error.
LSP requires a response to every request, and a server is entitled to block until it arrives. cclsp routed server-initiated messages to handleMessage, which only understood notifications, so a request that no adapter claimed was silently dropped. TypeScript 7's native LSP (`tsc --lsp --stdio`) sends client/registerCapability immediately after `initialized` and waits for the reply. Nothing answers it, so every later request — hover, definition, references — sits until cclsp's 30s timeout. typescript-language-server never sends one, which is why this went unnoticed. Answer the capability requests cclsp understands (client/registerCapability, client/unregisterCapability, window/workDoneProgress/create, workspace/configuration). These are always answered: a server that sends one is already waiting, so a reply can only unblock it. Unrecognised requests keep today's behaviour and are ignored, unless the new per-server `rejectUnhandledRequests` option asks for a MethodNotFound reply. Existing configurations are therefore unaffected. Also widen LSPMessage.id to `number | string`, since servers pick their own request ids (TS 7 uses "ts1"), and correlate responses on an explicit number check so that id 0 is not mistaken for absent.
There was a problem hiding this comment.
Pull request overview
Fixes two LSP/JSON-RPC protocol correctness issues that prevented cclsp from working with servers that (a) send server→client requests that must be answered and/or (b) emit non-ASCII content (notably TypeScript 7’s native LSP). This keeps existing behavior stable by always answering a small set of safe capability/configuration requests, while leaving “reject unknown requests” as an opt-in per server.
Changes:
- Add server→client request handling (with an opt-in
MethodNotFoundrejection mode for otherwise-unhandled requests). - Fix incoming JSON-RPC framing to slice by byte length (Buffer-based), not JS string length.
- Expand request id support to
number | string, and add targeted tests for multi-byte framing and server-initiated request replies.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds rejectUnhandledRequests config option to control opt-in request rejection behavior. |
| src/lsp/types.ts | Widens LSPMessage.id to number | string for server-generated request ids. |
| src/lsp/server-manager.ts | Routes server-initiated requests to a responder (buildClientResponse) and fixes id=0 truthiness handling. |
| src/lsp/json-rpc.ts | Reworks receive-side framing to buffer bytes and parse by Content-Length byte counts; fixes response correlation for id=0. |
| src/lsp/json-rpc.test.ts | Adds regression tests for multi-byte payload framing and split-chunk mid-character parsing. |
| src/lsp/client-requests.ts | Implements “client-side” responses for key server-initiated LSP requests + optional MethodNotFound rejection. |
| src/lsp/client-requests.test.ts | Adds unit tests covering known request handling, opt-in rejection, and string/id=0 cases. |
| README.md | Documents the new rejectUnhandledRequests configuration option. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two protocol bugs that make cclsp unusable with TypeScript 7's native LSP (
tsc --lsp --stdio, the Go port — same server astsgo --lsp). Both are general correctness fixes rather than TS-specific workarounds; TS 7 just happens to trip both immediately.Symptom: every tool call fails with
LSP request timeout: … (30000ms), and once past that, a run ofFailed to parse LSP message.The bugs
1. Server → client requests were never answered (
src/lsp/server-manager.ts)LSP requires a response to every request, and a server may block until it arrives. Server-initiated messages went to
handleMessage, which only understood notifications, so any request no adapter claimed was silently dropped.TS 7 sends
client/registerCapabilityimmediately afterinitializedand waits for the reply. Nothing answers, so every subsequent request — hover, definition, references — sits until the 30s timeout.typescript-language-servernever sends one, which is why this has gone unnoticed.2. Incoming frames were sliced by string length, not byte length (
src/lsp/json-rpc.ts)Content-Lengthcounts bytes, but stdout was accumulated into a JS string and sliced by that count. The first message carrying a multi-byte character reads short, the next frame is then parsed from the wrong offset, and the parser never resynchronises.Reachable with any server whose output is not pure ASCII. TS 7 hits it at once — its own timing logs read
handled method 'textDocument/hover' (11) in 680.392µs.Note
sendMessagealready usedBuffer.byteLength, so outgoing framing was correct; only the receive path was wrong.Approach — existing setups are unaffected
I deliberately split "always safe" from "changes behaviour":
client/registerCapability,client/unregisterCapability,window/workDoneProgress/create,workspace/configuration. A server that sends one of these is already blocked waiting for the reply, so answering can only unblock it. There is no configuration in which silence was the better outcome.MethodNotFoundto an unrecognised request is the one genuine behaviour change, so it sits behind a new per-serverrejectUnhandledRequestsoption, defaultfalse. Without it, unrecognised requests are ignored exactly as today.Also in the second commit:
LSPMessage.idwidened tonumber | string(servers pick their own request ids — TS 7 uses"ts1"), and response correlation now checks for a number explicitly rather than relying on truthiness, so an id of0is no longer treated as absent.Testing
src/lsp/client-requests.test.ts(new, 9 cases) and 4 new cases insrc/lsp/json-rpc.test.ts, including a frame split mid-character across two chunks.I checked the framing tests fail against the unpatched transport — all 4 fail before, pass after — so they genuinely pin the regression.
The 19 failures are pre-existing on
mainat 93414a1 (verified by running the suite on a clean checkout:203 pass / 5 skip / 19 failbefore, same 19 after). Happy to look at them separately if useful.Verified against a real server
Driven end-to-end through cclsp over MCP against TypeScript 7.0.2's
tsc --lsp --stdioon a 14-package monorepo:find_definition,find_referencesandrename_symbolall return correct results where they previously timed out.Possibly related
#18 reports
vue-language-servertiming out ondocumentSymbolafter 30s. That is the same shape as bug 1 — a request the client never answers — though the Vue adapter coverstsserver/requestspecifically, so I would not claim this fixes it without a Vue project to test on.