fix(lsp): fail diagnostics when every applicable server fails
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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -284,6 +284,8 @@ export class LspTool implements AgentTool<typeof lspSchema, LspToolDetails, Them
|
||||
: Math.min(SINGLE_DIAGNOSTICS_WAIT_TIMEOUT_MS, timeoutSec * 1000);
|
||||
const results: string[] = [];
|
||||
const allServerNames = new Set<string>();
|
||||
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<typeof lspSchema, LspToolDetails, Them
|
||||
const uri = fileToUri(resolved);
|
||||
const relPath = formatPathRelativeToCwd(resolved, this.session.cwd);
|
||||
const allDiagnostics: Diagnostic[] = [];
|
||||
const failedServers: string[] = [];
|
||||
let succeededServers = 0;
|
||||
|
||||
// Query all applicable servers for this file
|
||||
for (const [serverName, serverConfig] of servers) {
|
||||
allServerNames.add(serverName);
|
||||
totalServerAttempts++;
|
||||
try {
|
||||
throwIfAborted(signal);
|
||||
if (serverConfig.createClient) {
|
||||
const linterClient = getLinterClient(serverName, serverConfig, this.session.cwd);
|
||||
const diagnostics = await linterClient.lint(resolved);
|
||||
allDiagnostics.push(...diagnostics);
|
||||
succeededServers++;
|
||||
totalServerSuccesses++;
|
||||
continue;
|
||||
}
|
||||
const client = await getOrCreateClient(serverConfig, this.session.cwd, undefined, signal);
|
||||
@@ -329,11 +336,19 @@ export class LspTool implements AgentTool<typeof lspSchema, LspToolDetails, Them
|
||||
expectedDocumentVersion,
|
||||
});
|
||||
allDiagnostics.push(...diagnostics);
|
||||
succeededServers++;
|
||||
totalServerSuccesses++;
|
||||
} catch (err) {
|
||||
if (err instanceof ToolAbortError || signal?.aborted) {
|
||||
throw err;
|
||||
}
|
||||
// Server failed, continue with others
|
||||
// Server failed; record it so a total failure is not reported as clean.
|
||||
failedServers.push(serverName);
|
||||
logger.debug("LSP diagnostics server failed", {
|
||||
server: serverName,
|
||||
file: relPath,
|
||||
error: err instanceof Error ? err.message : String(err),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@@ -351,16 +366,35 @@ export class LspTool implements AgentTool<typeof lspSchema, LspToolDetails, Them
|
||||
sortDiagnostics(uniqueDiagnostics);
|
||||
|
||||
if (!detailed && targets.length === 1) {
|
||||
if (uniqueDiagnostics.length === 0) {
|
||||
if (succeededServers === 0) {
|
||||
return {
|
||||
content: [{ type: "text", text: "OK" }],
|
||||
content: [
|
||||
{
|
||||
type: "text",
|
||||
text: `${theme.status.error} ${relPath}: all language servers failed (${failedServers.join(", ")})`,
|
||||
},
|
||||
],
|
||||
details: { action, serverName: Array.from(allServerNames).join(", "), success: false },
|
||||
};
|
||||
}
|
||||
|
||||
if (uniqueDiagnostics.length === 0) {
|
||||
const text =
|
||||
failedServers.length > 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<typeof lspSchema, LspToolDetails, Them
|
||||
}
|
||||
|
||||
if (uniqueDiagnostics.length === 0) {
|
||||
results.push(`${theme.status.success} ${relPath}: no issues`);
|
||||
if (succeededServers === 0) {
|
||||
results.push(
|
||||
`${theme.status.error} ${relPath}: all language servers failed (${failedServers.join(", ")})`,
|
||||
);
|
||||
} else {
|
||||
results.push(`${theme.status.success} ${relPath}: no issues`);
|
||||
if (failedServers.length > 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 },
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -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[] = [];
|
||||
|
||||
Reference in New Issue
Block a user