diff --git a/packages/lsp/lsp-local/README.md b/packages/lsp/lsp-local/README.md index f9e4b32713..1424f27835 100644 --- a/packages/lsp/lsp-local/README.md +++ b/packages/lsp/lsp-local/README.md @@ -44,6 +44,6 @@ Indirectly, through `dsh-tool-lsp`, which surfaces this provider's normalized re ## Known Limitations and Deferred Work -- **Trusted host-local only** — no sandbox confinement, no private cache/temp write contract; supporting untrusted binaries or restricted/remote/virtual workspaces requires a later process/filesystem contract and a different provider ([seam RFC](../../../docs/rfc/implemented/architecture/2026-07-15-lsp-capability-seam.md)). +- **Trusted host-local only** — no sandbox confinement, no private cache/temp write contract; supporting untrusted binaries or restricted/remote/virtual workspaces requires a later process/filesystem contract and a different provider ([seam RFC](../../../docs/rfc/implemented/architecture/2026-07-15-lsp-capability-seam.md)). Containment resolves `realpath`, then opens the source through one handle with `O_NOFOLLOW` (final-component symlink guard) and a bounded read; a concurrent mutator that swaps an *ancestor* directory for a symlink between the resolve and the open is an accepted residual TOCTOU under this trusted-deployment model, not closed with non-portable `openat` segment walks. - **Transient-open compatibility floor** — servers whose synchronization omits open/close (or advertise `None`) are unsupported even if closed-document queries would work; the pinned TypeScript e2e establishes one compatibility floor, not a cross-language claim. - **Per-instance serialization latency** — parallel agents sharing a workspace queue behind one process; long-lived workspace processes consume memory until disposal. diff --git a/packages/lsp/lsp-local/src/connection.ts b/packages/lsp/lsp-local/src/connection.ts index 272109d2c6..6eea8aec27 100644 --- a/packages/lsp/lsp-local/src/connection.ts +++ b/packages/lsp/lsp-local/src/connection.ts @@ -187,9 +187,10 @@ export class LspConnection { try { messages = this.decoder.push(chunk) } catch (error) { - // A framing/JSON failure corrupts the stream position irrecoverably: fail the instance. + // A framing/JSON failure corrupts the stream position irrecoverably: fail the instance and + // SIGKILL the whole group so helper processes don't outlive the leader. this.fail(asError(error)) - this.child.kill('SIGKILL') + this.signalGroup('SIGKILL') return } for (const message of messages) this.dispatch(message) diff --git a/packages/lsp/lsp-local/src/index.ts b/packages/lsp/lsp-local/src/index.ts index dc6e02d3c6..19d29e3618 100644 --- a/packages/lsp/lsp-local/src/index.ts +++ b/packages/lsp/lsp-local/src/index.ts @@ -114,8 +114,12 @@ export function apply(ctx: Context, config: Config): void { // nonpositive value would let a server that ignores shutdown hang disposal forever. Fail at load. assertPositiveInteger('shutdownTimeoutMs', resolved.shutdownTimeoutMs) assertPositiveInteger('killGraceMs', resolved.killGraceMs) - // A nonpositive stderr cap defeats the retained-tail bound (`slice(-0)` keeps everything). + // Byte caps must be positive: a nonpositive stderr cap defeats the retained-tail bound + // (`slice(-0)` keeps everything), `maxMessageBytes: 0` makes every response fatal, and a bad + // document cap fails later in the read path instead of at load. assertPositiveInteger('maxStderrBytes', resolved.maxStderrBytes) + assertPositiveInteger('maxMessageBytes', resolved.maxMessageBytes) + assertPositiveInteger('maxDocumentBytes', resolved.maxDocumentBytes) const childEnv = buildChildEnv(resolved.env) // Resolve the executable eagerly so a misconfigured command fails at load, not on first query. const executable = resolveExecutable(resolved.command, childEnv) diff --git a/packages/lsp/lsp-local/src/instance.ts b/packages/lsp/lsp-local/src/instance.ts index ecf2782a12..cd73d5cb8d 100644 --- a/packages/lsp/lsp-local/src/instance.ts +++ b/packages/lsp/lsp-local/src/instance.ts @@ -108,16 +108,17 @@ export class LspInstance { if (this.disposed) throw new Error('LSP instance was disposed') /* v8 ignore next -- the abortable queue wait rejects a pre-aborted signal before runQuery; this is a belt-and-suspenders guard. */ if (signal?.aborted) throw abortError(signal) - // Observe abort during the handshake wait: a server that never answers `initialize` must not - // block the tool-timeout signal here (the timeout policy awaits our quiescence, not the promise). - // If abort wins, the handshake is still pending on a live process, so tear the instance down — - // otherwise its poisoned `ready` would make every later query for this workspace re-wait. + // Observe abort during the handshake wait, and never pool a poisoned instance: if the wait ends + // in failure — an abort on a still-pending handshake, OR `initialize` rejecting (utf-8 + // negotiation, malformed result) without the process exiting — tear the instance down so a + // permanently-rejecting/pending `ready` can't make every later query for this workspace fail. try { await this.abortable(this.ready, signal) } catch (error) { - if (signal?.aborted && !this.dead) { + if (!this.dead) { this.disposed = true - await this.tearDown(abortError(signal)) + /* v8 ignore next -- ready rejects with an Error (abort reason or initialize failure); the String() fallback is defensive. */ + await this.tearDown(error instanceof Error ? error : new Error(String(error))) } throw error } diff --git a/packages/lsp/lsp-local/tests/lifecycle.spec.ts b/packages/lsp/lsp-local/tests/lifecycle.spec.ts index 66a18584a1..bf624a1c56 100644 --- a/packages/lsp/lsp-local/tests/lifecycle.spec.ts +++ b/packages/lsp/lsp-local/tests/lifecycle.spec.ts @@ -105,6 +105,16 @@ describe('lsp-local end to end over a fake server', () => { await ctx.fiber.dispose() }) + it('does not pool a poisoned instance when initialize rejects', async () => { + // A utf-8 server makes `initialize` reject; the instance must be torn down (not left with a + // permanently-rejecting `ready`) so a later query starts a fresh process rather than reusing it. + const ctx = await mount({ LSP_FAKE_ENCODING: 'utf-8', LSP_FAKE_DEF: 'null' }) + await expect(ctx.lsp.query(query('definition'))).rejects.toThrow(/unsupported position encoding/) + // A second query must also fail the same way (fresh instance), and must NOT hang on a poisoned one. + await expect(ctx.lsp.query(query('definition'))).rejects.toThrow(/unsupported position encoding/) + await ctx.fiber.dispose() + }) + it('rejects a server without transient-open sync (None)', async () => { const ctx = await mount({ LSP_FAKE_SYNC: '0', LSP_FAKE_DEF: 'null' }) await expect(ctx.lsp.query(query('definition'))).rejects.toThrow(/transient textDocument\/didOpen/) diff --git a/packages/lsp/tool-lsp/src/render.ts b/packages/lsp/tool-lsp/src/render.ts index df0913a591..0051adc719 100644 --- a/packages/lsp/tool-lsp/src/render.ts +++ b/packages/lsp/tool-lsp/src/render.ts @@ -137,7 +137,9 @@ export function renderUri(uri: string, workspaceRoot: string): string { } const rel = relative(workspaceRoot, absolute) if (rel === '') return '.' - const outside = rel.startsWith('..') || isAbsolute(rel) + // A leading `..` SEGMENT (or an absolute rel) means outside the workspace; guard against a false + // positive on an in-workspace path whose first component merely starts with dots (e.g. `..gen/x`). + const outside = rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel) return outside ? absolute : rel.split(sep).join('/') } diff --git a/packages/lsp/tool-lsp/tests/render.spec.ts b/packages/lsp/tool-lsp/tests/render.spec.ts index 53a2be5899..88b02a1fd5 100644 --- a/packages/lsp/tool-lsp/tests/render.spec.ts +++ b/packages/lsp/tool-lsp/tests/render.spec.ts @@ -60,6 +60,12 @@ describe('renderUri', () => { expect(renderUri(pathToFileURL(WS).href, WS)).toBe('.') }) + it('keeps an in-workspace path whose first segment starts with dots relative', () => { + // `..generated` is a real in-workspace dir, not a parent escape; only a `..` segment is external. + const uri = pathToFileURL(join(WS, '..generated', 'a.ts')).href + expect(renderUri(uri, WS)).toBe('..generated/a.ts') + }) + it('keeps a non-file URI verbatim', () => { expect(renderUri('untitled:Untitled-1', WS)).toBe('untitled:Untitled-1') expect(renderUri('jdt://contents/Foo.class', WS)).toBe('jdt://contents/Foo.class')