From dce0216bcb24586fa222b0911ea35b12df800aee Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 13 Aug 2026 03:07:07 +0000 Subject: [PATCH] fix(lsp): fail diagnostics when every applicable server fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The direct diagnostics loop caught non-abort server errors without recording them, so a file whose every applicable server failed produced an empty aggregate rendered as OK with success: true — a false-negative hiding a total diagnostics failure. Track per-file and global success/failure counts: zero successful server responses now yields success: false with an explicit failure line, while partial success still surfaces diagnostics and names the servers that failed. Fixes #8377 --- packages/coding-agent/CHANGELOG.md | 4 ++ packages/coding-agent/src/lsp/tool.ts | 61 +++++++++++++++++-- .../test/tools/lsp-regressions.test.ts | 42 ++++++++++++- 3 files changed, 100 insertions(+), 7 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b387e76fd..a6bb4c55e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed LSP `diagnostics` reporting `OK`/`success: true` when every applicable language server failed; a run with zero successful server responses now fails, and partial failures surface diagnostics while naming the servers that failed ([#8377](https://github.com/can1357/oh-my-pi/issues/8377)). + ## [17.2.15] - 2026-08-12 ### Added diff --git a/packages/coding-agent/src/lsp/tool.ts b/packages/coding-agent/src/lsp/tool.ts index 75394ad1a..bf82e9736 100644 --- a/packages/coding-agent/src/lsp/tool.ts +++ b/packages/coding-agent/src/lsp/tool.ts @@ -284,6 +284,8 @@ export class LspTool implements AgentTool(); + let totalServerAttempts = 0; + let totalServerSuccesses = 0; if (truncatedGlobTargets) { results.push( `${theme.status.warning} Pattern matched more than ${MAX_GLOB_DIAGNOSTIC_TARGETS} files; showing first ${MAX_GLOB_DIAGNOSTIC_TARGETS}. Narrow the glob or use workspace diagnostics.`, @@ -302,16 +304,21 @@ export class LspTool implements AgentTool 0 + ? `OK\n${theme.status.warning} some servers failed: ${failedServers.join(", ")}` + : "OK"; + return { + content: [{ type: "text", text }], details: { action, serverName: Array.from(allServerNames).join(", "), success: true }, }; } const summary = formatDiagnosticsSummary(uniqueDiagnostics); const formatted = uniqueDiagnostics.map(d => formatDiagnostic(d, relPath)); - const output = `${summary}:\n${formatGroupedDiagnosticMessages(formatted)}`; + let output = `${summary}:\n${formatGroupedDiagnosticMessages(formatted)}`; + if (failedServers.length > 0) { + output += `\n${theme.status.warning} some servers failed: ${failedServers.join(", ")}`; + } return { content: [{ type: "text", text: output }], details: { action, serverName: Array.from(allServerNames).join(", "), success: true }, @@ -368,18 +402,33 @@ export class LspTool implements AgentTool 0) { + results.push( + `${theme.status.warning} ${relPath}: some servers failed (${failedServers.join(", ")})`, + ); + } + } } else { const summary = formatDiagnosticsSummary(uniqueDiagnostics); results.push(`${theme.status.error} ${relPath}: ${summary}`); const formatted = uniqueDiagnostics.map(d => formatDiagnostic(d, relPath)); results.push(formatGroupedDiagnosticMessages(formatted)); + if (failedServers.length > 0) { + results.push(`${theme.status.warning} ${relPath}: some servers failed (${failedServers.join(", ")})`); + } } } + const allServersFailed = totalServerAttempts > 0 && totalServerSuccesses === 0; return { content: [{ type: "text", text: results.join("\n") }], - details: { action, serverName: Array.from(allServerNames).join(", "), success: true }, + details: { action, serverName: Array.from(allServerNames).join(", "), success: !allServersFailed }, }; } diff --git a/packages/coding-agent/test/tools/lsp-regressions.test.ts b/packages/coding-agent/test/tools/lsp-regressions.test.ts index 2d905928c..310dcf292 100644 --- a/packages/coding-agent/test/tools/lsp-regressions.test.ts +++ b/packages/coding-agent/test/tools/lsp-regressions.test.ts @@ -43,7 +43,7 @@ import { resolveSymbolColumn, uriToFile, } from "@oh-my-pi/pi-coding-agent/lsp/utils"; -import { getThemeByName } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; +import { getThemeByName, initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import { ToolAbortError } from "@oh-my-pi/pi-coding-agent/tools/tool-errors"; import { clampTimeout } from "@oh-my-pi/pi-coding-agent/tools/tool-timeouts"; @@ -1597,6 +1597,46 @@ describe("lsp regressions", () => { } }); + it("reports failure when every applicable diagnostics server fails (#8377)", async () => { + const tempDir = TempDir.createSync("@omp-lsp-all-servers-fail-"); + try { + const targetFile = path.join(tempDir.path(), "target.ts"); + await Bun.write(targetFile, "export const target = 1;\n"); + // The diagnostics renderer prefixes status lines via the global theme, + // which production initializes before any tool runs. + await initTheme(); + + const serverConfig: ServerConfig = { + command: "broken-lsp", + fileTypes: ["ts"], + rootMarkers: [], + }; + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ + servers: { broken: serverConfig }, + idleTimeoutMs: undefined, + }); + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([["broken", serverConfig]]); + vi.spyOn(lspClient, "getOrCreateClient").mockRejectedValue(new Error("server exited with code 7")); + + const tool = new LspTool(makeLspSession(tempDir.path())); + const result = await tool.execute("all-servers-fail", { + action: "diagnostics", + file: targetFile, + timeout: 5, + }); + + // A total server failure must not masquerade as a clean file. + expect(result.details?.success).toBe(false); + const output = textResult(result); + expect(output).not.toBe("OK"); + expect(output).toContain("all language servers failed"); + expect(output).toContain("broken"); + } finally { + vi.restoreAllMocks(); + tempDir.removeSync(); + } + }); + it("treats a go.work-only root as a Go workspace for workspace diagnostics", async () => { const tempDir = TempDir.createSync("@omp-lsp-go-work-only-"); const spawnCalls: BunSpawnCall[] = [];