diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 0a5979728..e8510834b 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -99,6 +99,9 @@ ### Fixed - Fixed Portkey/gateway custom models whose ids start with `@` (e.g. `@modal/GLM-5-2-FP8`) being rewritten to unrelated bundled wire ids (e.g. `glm-5-2`), which caused `400` responses requiring `x-portkey-config` or `x-portkey-provider`. +### Changed + +- Reduced format-on-write latency by avoiding cold language-server startup when diagnostics are disabled. ## [17.0.6] - 2026-07-20 diff --git a/packages/coding-agent/src/lsp/client.ts b/packages/coding-agent/src/lsp/client.ts index 60bb0d9ff..d5f0f0c34 100644 --- a/packages/coding-agent/src/lsp/client.ts +++ b/packages/coding-agent/src/lsp/client.ts @@ -846,6 +846,29 @@ export async function getOrCreateClient( return clientPromise; } +/** Return an active or already-starting client without starting a language server. */ +export async function getActiveOrPendingClient( + config: ServerConfig, + cwd: string, + signal?: AbortSignal, +): Promise { + throwIfAborted(signal); + const client = clients.get(`${config.command}:${cwd}`); + if (client) { + client.lastActivity = Date.now(); + return client; + } + + const pending = clientLocks.get(`${config.command}:${cwd}`); + if (!pending) return undefined; + try { + return await untilAborted(signal, pending); + } catch { + throwIfAborted(signal); + return undefined; + } +} + /** * Ensure a file is opened in the LSP client. * Sends didOpen notification if the file is not already tracked. diff --git a/packages/coding-agent/src/lsp/clients/biome-client.ts b/packages/coding-agent/src/lsp/clients/biome-client.ts index 82bd497a6..2e1cfbbf6 100644 --- a/packages/coding-agent/src/lsp/clients/biome-client.ts +++ b/packages/coding-agent/src/lsp/clients/biome-client.ts @@ -2,7 +2,7 @@ * Biome CLI-based linter client. * Uses Biome's CLI with JSON output instead of LSP (which has stale diagnostics issues). */ -import path from "node:path"; +import * as path from "node:path"; import { logger } from "@oh-my-pi/pi-utils"; import type { Diagnostic, DiagnosticSeverity, LinterClient, ServerConfig } from "../../lsp/types"; @@ -141,14 +141,13 @@ export class BiomeClient implements LinterClient { ) {} async format(filePath: string, content: string): Promise { - // Write content to file first + // Keep the standalone LinterClient contract: callers supply the content to + // format, regardless of what is currently on disk. await Bun.write(filePath, content); - // Run biome format --write const result = await runBiome(["format", "--write", filePath], this.cwd, this.config.resolvedCommand); if (result.success) { - // Read back formatted content return await Bun.file(filePath).text(); } diff --git a/packages/coding-agent/src/lsp/index.ts b/packages/coding-agent/src/lsp/index.ts index a33733475..12dc7dd9b 100644 --- a/packages/coding-agent/src/lsp/index.ts +++ b/packages/coding-agent/src/lsp/index.ts @@ -20,6 +20,7 @@ import { ensureFileOpen, FileChangeType, getActiveClients, + getActiveOrPendingClient, getOrCreateClient, type LspServerStatus, notifySaved, @@ -211,6 +212,7 @@ async function syncFileContent( cwd: string, servers: Array<[string, ServerConfig]>, signal?: AbortSignal, + createMissing = true, ): Promise { throwIfAborted(signal); await Promise.allSettled( @@ -219,11 +221,15 @@ async function syncFileContent( if (serverConfig.createClient) { return; } - const client = await getOrCreateClient(serverConfig, cwd, undefined, signal); + const client = createMissing + ? await getOrCreateClient(serverConfig, cwd, undefined, signal) + : await getActiveOrPendingClient(serverConfig, cwd, signal); + if (!client) return; throwIfAborted(signal); await syncContent(client, absolutePath, content, signal); }), ); + throwIfAborted(signal); } /** @@ -239,6 +245,7 @@ async function notifyFileSaved( cwd: string, servers: Array<[string, ServerConfig]>, signal?: AbortSignal, + createMissing = true, ): Promise { throwIfAborted(signal); await Promise.allSettled( @@ -247,10 +254,14 @@ async function notifyFileSaved( if (serverConfig.createClient) { return; } - const client = await getOrCreateClient(serverConfig, cwd, undefined, signal); + const client = createMissing + ? await getOrCreateClient(serverConfig, cwd, undefined, signal) + : await getActiveOrPendingClient(serverConfig, cwd, signal); + if (!client) return; await notifySaved(client, absolutePath, signal); }), ); + throwIfAborted(signal); } // Cache config per cwd to avoid repeated file I/O @@ -1333,7 +1344,8 @@ async function runLspWritethrough( // Capture diagnostic versions BEFORE syncing to detect stale diagnostics // Bound client creation by the writethrough budget: a hung/broken server // must not add its full init wait (30s default) to every edit. - const minVersions = enableDiagnostics ? await captureDiagnosticVersions(cwd, servers, 5_000, signal) : undefined; + const minVersionsPromise = enableDiagnostics ? captureDiagnosticVersions(cwd, servers, 5_000, signal) : undefined; + let minVersions = useCustomFormatter ? undefined : await minVersionsPromise; let expectedDocumentVersions: ServerVersionMap | undefined; let formatter: FileFormatResult | undefined; @@ -1353,13 +1365,19 @@ async function runLspWritethrough( operationSignal = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal; await untilAborted(operationSignal, async () => { if (useCustomFormatter) { - // Custom linters (e.g. Biome CLI) require on-disk input. + // Custom linters operate on on-disk input; the shared pre-write also + // supports implementations that inspect the file before formatting. await writeContent(content); - finalContent = await formatContent(dst, content, cwd, customLinterServers, operationSignal); + const [formattedContent, capturedVersions] = await Promise.all([ + formatContent(dst, content, cwd, customLinterServers, operationSignal), + minVersionsPromise, + ]); + finalContent = formattedContent; + minVersions = capturedVersions; formatter = finalContent !== content ? FileFormatResult.FORMATTED : FileFormatResult.UNCHANGED; await writeContent(finalContent); await notifyWriteCommitted(operationSignal); - await syncFileContent(dst, finalContent, cwd, lspServers, operationSignal); + await syncFileContent(dst, finalContent, cwd, lspServers, operationSignal, enableDiagnostics); } else { // 1. Sync original content to LSP servers await syncFileContent(dst, content, cwd, lspServers, operationSignal); @@ -1385,7 +1403,7 @@ async function runLspWritethrough( } // 5. Notify saved to LSP servers - await notifyFileSaved(dst, cwd, lspServers, operationSignal); + await notifyFileSaved(dst, cwd, lspServers, operationSignal, !useCustomFormatter || enableDiagnostics); }); synced = true; } catch { diff --git a/packages/coding-agent/test/biome-client.test.ts b/packages/coding-agent/test/biome-client.test.ts new file mode 100644 index 000000000..b97b6a0db --- /dev/null +++ b/packages/coding-agent/test/biome-client.test.ts @@ -0,0 +1,132 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { BiomeClient } from "../src/lsp/clients/biome-client"; +import type { ServerConfig } from "../src/lsp/types"; + +const tempDirs: string[] = []; +const tempRoots: string[] = []; +const repoRoot = path.resolve(import.meta.dir, "../../.."); + +function resolveRepoBiome(): string { + const platformPackages: Partial>>> = { + darwin: { arm64: ["cli-darwin-arm64"], x64: ["cli-darwin-x64"] }, + linux: { + arm64: ["cli-linux-arm64", "cli-linux-arm64-musl"], + x64: ["cli-linux-x64", "cli-linux-x64-musl"], + }, + win32: { arm64: ["cli-win32-arm64"], x64: ["cli-win32-x64"] }, + }; + const executable = process.platform === "win32" ? "biome.exe" : "biome"; + for (const packageName of platformPackages[process.platform]?.[process.arch] ?? []) { + try { + return Bun.resolveSync(`@biomejs/${packageName}/${executable}`, repoRoot); + } catch {} + } + throw new Error(`No repository Biome binary for ${process.platform}/${process.arch}`); +} + +const repoBiome = resolveRepoBiome(); + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map(dir => fs.rm(dir, { force: true, recursive: true }))); + await Promise.all(tempRoots.splice(0).map(dir => fs.rm(dir, { force: true, recursive: true }))); +}); + +async function makeTempDir(): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-biome-client-test-")); + tempDirs.push(dir); + return dir; +} + +async function createFakeBiomeCommand( + tempDir: string, + expectedInput: string, + formattedOutput: string, +): Promise { + const command = path.join(tempDir, "biome"); + const expectedInputPath = path.join(tempDir, "expected-input.ts"); + const formattedOutputPath = path.join(tempDir, "formatted-output.ts"); + await Bun.write(expectedInputPath, expectedInput); + await Bun.write(formattedOutputPath, formattedOutput); + await Bun.write( + command, + `#!/bin/sh +test "$1" = "format" || exit 7 +test "$2" = "--write" || exit 8 +test "$3" = "${path.join(tempDir, "example.ts")}" || exit 10 +cmp -s "$3" "${expectedInputPath}" || exit 9 +cp "${formattedOutputPath}" "$3" +exit 0 +`, + ); + await fs.chmod(command, 0o755); + return command; +} + +function biomeConfig(command: string): ServerConfig { + return { + command: "biome", + fileTypes: [".ts"], + rootMarkers: [], + resolvedCommand: command, + }; +} + +describe("BiomeClient format", () => { + test("formats the supplied content instead of stale on-disk content", async () => { + const tempDir = await makeTempDir(); + const targetFile = path.join(tempDir, "example.ts"); + const unformatted = "export const value:number=1\n"; + const formatted = "export const value: number = 1;\n"; + await Bun.write(targetFile, "export const stale = true;\n"); + const command = await createFakeBiomeCommand(tempDir, unformatted, formatted); + const result = await new BiomeClient(biomeConfig(command), tempDir).format(targetFile, unformatted); + + expect(result).toBe(formatted); + expect(await Bun.file(targetFile).text()).toBe(formatted); + }); + + test("formats configured TypeScript with the repository Biome", async () => { + const scratchDir = await fs.mkdtemp( + path.join(repoRoot, "packages", "coding-agent", "src", "__biome_client_test__-"), + ); + tempDirs.push(scratchDir); + const targetFile = path.join(scratchDir, "configured.ts"); + const unformatted = "export const configured:number=1\n"; + await Bun.write(targetFile, unformatted); + + const result = await new BiomeClient(biomeConfig(repoBiome), repoRoot).format(targetFile, unformatted); + + expect(result).toBe("export const configured: number = 1;\n"); + }); + + test("leaves config-excluded content unchanged with the repository Biome", async () => { + const excludedRoot = path.join(repoRoot, ".perf"); + const createdRoot = await fs.mkdir(excludedRoot, { recursive: true }); + if (createdRoot) tempRoots.push(excludedRoot); + const scratchDir = await fs.mkdtemp(path.join(excludedRoot, "biome-client-test-")); + tempDirs.push(scratchDir); + const targetFile = path.join(scratchDir, "excluded.ts"); + const unformatted = "export const excluded:number=1\n"; + await Bun.write(targetFile, unformatted); + + const result = await new BiomeClient(biomeConfig(repoBiome), repoRoot).format(targetFile, unformatted); + + expect(result).toBe(unformatted); + }); + + test("returns the original content when Biome fails", async () => { + const tempDir = await makeTempDir(); + const command = path.join(tempDir, "biome-failure"); + await Bun.write(command, "#!/bin/sh\ncat >/dev/null\nexit 1\n"); + await fs.chmod(command, 0o755); + const targetFile = path.join(tempDir, "example.ts"); + const content = "export const value = 1;\n"; + + const result = await new BiomeClient(biomeConfig(command), tempDir).format(targetFile, content); + + expect(result).toBe(content); + }); +}); diff --git a/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts b/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts index 1504b38c7..13ed0a57c 100644 --- a/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts +++ b/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts @@ -1,10 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; import * as path from "node:path"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; -import { createLspWritethrough, type FileDiagnosticsResult } from "@oh-my-pi/pi-coding-agent/lsp"; +import { createLspWritethrough, type FileDiagnosticsResult, FileFormatResult } from "@oh-my-pi/pi-coding-agent/lsp"; import * as lspClient from "@oh-my-pi/pi-coding-agent/lsp/client"; import * as lspConfig from "@oh-my-pi/pi-coding-agent/lsp/config"; -import type { Diagnostic, LspClient, ServerConfig } from "@oh-my-pi/pi-coding-agent/lsp/types"; +import type { Diagnostic, LinterClient, LspClient, ServerConfig } from "@oh-my-pi/pi-coding-agent/lsp/types"; import { fileToUri } from "@oh-my-pi/pi-coding-agent/lsp/utils"; import type { DeferredDiagnosticsEntry, ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import { WriteTool } from "@oh-my-pi/pi-coding-agent/tools/write"; @@ -16,6 +16,19 @@ const TEST_SERVER: ServerConfig = { rootMarkers: [], }; +function createFormatter(format: (filePath: string, content: string) => Promise): ServerConfig { + return { + command: "test-formatter", + fileTypes: ["ts"], + rootMarkers: [], + createClient: () => + ({ + format, + lint: async () => [], + }) satisfies LinterClient, + }; +} + function createDiagnostic(message: string): Diagnostic { return { message, @@ -163,6 +176,137 @@ describe("LSP diagnostics freshness", () => { expect(getOrCreate).not.toHaveBeenCalled(); }); + it("does not cold-start an LSP server for custom formatting when diagnostics are disabled", async () => { + const filePath = path.join(tempDir.path(), "formatted.ts"); + const formatter = createFormatter(async () => "export const value = 1;\n"); + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([ + ["test-lsp", TEST_SERVER], + ["formatter", formatter], + ]); + const getOrCreate = vi + .spyOn(lspClient, "getOrCreateClient") + .mockRejectedValue(new Error("format-only writes must not cold-start an LSP server")); + vi.spyOn(lspClient, "getActiveOrPendingClient").mockResolvedValue(undefined); + const sync = vi.spyOn(lspClient, "syncContent").mockResolvedValue(); + const notifySaved = vi.spyOn(lspClient, "notifySaved").mockResolvedValue(); + vi.spyOn(lspClient, "notifyWorkspaceWatchedFiles").mockResolvedValue(); + + const writethrough = createLspWritethrough(tempDir.path(), { + enableFormat: true, + enableDiagnostics: false, + }); + const result = await writethrough(filePath, "export const value=1\n"); + + expect(result?.formatter).toBe(FileFormatResult.FORMATTED); + expect(await Bun.file(filePath).text()).toBe("export const value = 1;\n"); + expect(getOrCreate).not.toHaveBeenCalled(); + expect(sync).not.toHaveBeenCalled(); + expect(notifySaved).not.toHaveBeenCalled(); + }); + + it("keeps an already-running LSP client synchronized after custom formatting", async () => { + const filePath = path.join(tempDir.path(), "formatted.ts"); + const client = createClient(tempDir.path(), TEST_SERVER); + const formatter = createFormatter(async () => "export const value = 1;\n"); + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([ + ["test-lsp", TEST_SERVER], + ["formatter", formatter], + ]); + const getOrCreate = vi.spyOn(lspClient, "getOrCreateClient"); + vi.spyOn(lspClient, "getActiveOrPendingClient").mockResolvedValue(client); + const sync = vi.spyOn(lspClient, "syncContent").mockResolvedValue(); + const notifySaved = vi.spyOn(lspClient, "notifySaved").mockResolvedValue(); + vi.spyOn(lspClient, "notifyWorkspaceWatchedFiles").mockResolvedValue(); + + const writethrough = createLspWritethrough(tempDir.path(), { + enableFormat: true, + enableDiagnostics: false, + }); + await writethrough(filePath, "export const value=1\n"); + + expect(getOrCreate).not.toHaveBeenCalled(); + expect(sync).toHaveBeenCalledWith(client, filePath, "export const value = 1;\n", expect.any(AbortSignal)); + expect(notifySaved).toHaveBeenCalledWith(client, filePath, expect.any(AbortSignal)); + }); + + it("waits for an already-starting LSP client without cold-starting another one", async () => { + const filePath = path.join(tempDir.path(), "formatted.ts"); + const client = createClient(tempDir.path(), TEST_SERVER); + const pendingClient = Promise.withResolvers(); + const formatter = createFormatter(async () => "export const value = 1;\n"); + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([ + ["test-lsp", TEST_SERVER], + ["formatter", formatter], + ]); + const getOrCreate = vi.spyOn(lspClient, "getOrCreateClient"); + const getActiveOrPending = vi + .spyOn(lspClient, "getActiveOrPendingClient") + .mockImplementation(async () => pendingClient.promise); + const sync = vi.spyOn(lspClient, "syncContent").mockResolvedValue(); + const notifySaved = vi.spyOn(lspClient, "notifySaved").mockResolvedValue(); + vi.spyOn(lspClient, "notifyWorkspaceWatchedFiles").mockResolvedValue(); + + const writethrough = createLspWritethrough(tempDir.path(), { + enableFormat: true, + enableDiagnostics: false, + }); + const resultPromise = writethrough(filePath, "export const value=1\n"); + await Bun.sleep(0); + expect(sync).not.toHaveBeenCalled(); + pendingClient.resolve(client); + await resultPromise; + + expect(getOrCreate).not.toHaveBeenCalled(); + expect(getActiveOrPending).toHaveBeenNthCalledWith(1, TEST_SERVER, tempDir.path(), expect.any(AbortSignal)); + expect(getActiveOrPending).toHaveBeenNthCalledWith(2, TEST_SERVER, tempDir.path(), expect.any(AbortSignal)); + expect(sync).toHaveBeenCalledWith(client, filePath, "export const value = 1;\n", expect.any(AbortSignal)); + expect(notifySaved).toHaveBeenCalledWith(client, filePath, expect.any(AbortSignal)); + }); + + it("starts cold diagnostic initialization before custom formatting completes", async () => { + const filePath = path.join(tempDir.path(), "formatted.ts"); + const uri = fileToUri(filePath); + const client = createClient(tempDir.path(), TEST_SERVER); + const init = Promise.withResolvers(); + let initStarted = false; + const formatter = createFormatter(async () => { + expect(initStarted).toBe(true); + init.resolve(client); + return "export const value = 1;\n"; + }); + const clock = new VirtualClock(Date.now()); + installVirtualTime(clock); + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([ + ["test-lsp", TEST_SERVER], + ["formatter", formatter], + ]); + vi.spyOn(lspClient, "getOrCreateClient").mockImplementation(() => { + initStarted = true; + return init.promise; + }); + vi.spyOn(lspClient, "syncContent").mockImplementation(async mockClient => { + mockClient.openFiles.set(uri, { version: 1, languageId: "typescript" }); + }); + vi.spyOn(lspClient, "notifySaved").mockImplementation(async mockClient => { + publishDiagnostics(mockClient, uri, [], 1); + }); + vi.spyOn(lspClient, "notifyWorkspaceWatchedFiles").mockResolvedValue(); + + const writethrough = createLspWritethrough(tempDir.path(), { + enableFormat: true, + enableDiagnostics: true, + }); + const result = await writethrough(filePath, "export const value=1\n"); + + expect(result?.formatter).toBe(FileFormatResult.FORMATTED); + expect(result?.messages).toEqual([]); + expect(await Bun.file(filePath).text()).toBe("export const value = 1;\n"); + }); + it("announces batched sibling writes before syncing the diagnostic target", async () => { const stylesPath = path.join(tempDir.path(), "probe.module.scss"); const tsPath = path.join(tempDir.path(), "probe.tsx"); diff --git a/packages/coding-agent/test/tools/lsp-regressions.test.ts b/packages/coding-agent/test/tools/lsp-regressions.test.ts index c2da5ed90..bf06804f4 100644 --- a/packages/coding-agent/test/tools/lsp-regressions.test.ts +++ b/packages/coding-agent/test/tools/lsp-regressions.test.ts @@ -315,6 +315,83 @@ describe("lsp regressions", () => { } }); + it("returns an already-starting client without creating a second client", async () => { + const tempDir = TempDir.createSync("@omp-lsp-pending-client-"); + const initialize = Promise.withResolvers(); + try { + const server = installFakeLsp(async (message, srv) => { + if (message.method === "initialize") { + await initialize.promise; + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + }); + const config: ServerConfig = { + command: "fake-pending-lsp", + fileTypes: ["ts"], + rootMarkers: [], + }; + + const startingClient = lspClient.getOrCreateClient(config, tempDir.path(), 1_000); + await server.waitFor(message => message.method === "initialize"); + const existingClient = lspClient.getActiveOrPendingClient(config, tempDir.path()); + let settled = false; + void existingClient.then(() => { + settled = true; + }); + await Bun.sleep(0); + expect(settled).toBe(false); + + initialize.resolve(); + expect(await existingClient).toBe(await startingClient); + expect(server.received.filter(message => message.method === "initialize")).toHaveLength(1); + } finally { + initialize.resolve(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + + it("stops waiting for a pending client on caller abort without cancelling its initialization", async () => { + const tempDir = TempDir.createSync("@omp-lsp-pending-abort-"); + const initialize = Promise.withResolvers(); + try { + const server = installFakeLsp(async (message, srv) => { + if (message.method === "initialize") { + await initialize.promise; + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + }); + const config: ServerConfig = { + command: "fake-abort-pending-lsp", + fileTypes: ["ts"], + rootMarkers: [], + }; + + const startingClient = lspClient.getOrCreateClient(config, tempDir.path(), 1_000); + await server.waitFor(message => message.method === "initialize"); + const controller = new AbortController(); + const waitingClient = lspClient.getActiveOrPendingClient(config, tempDir.path(), controller.signal); + controller.abort(); + await expect(waitingClient).rejects.toThrow(); + + initialize.resolve(); + expect(await startingClient).toBeDefined(); + expect(server.received.filter(message => message.method === "initialize")).toHaveLength(1); + } finally { + initialize.resolve(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + it("advertises workspace folder support during LSP initialization", async () => { const tempDir = TempDir.createSync("@omp-lsp-workspace-folders-"); try {