smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge PR #8397: fix(lsp): fail diagnostics when every applicable server fails (@roboomp)

Browse Source
This commit is contained in:
can1357
2026-08-13 05:46:31 +02:00
parent 3d9649fbfa dce0216bcb
commit c4bc106486
3 changed files with 99 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+3
View File
@@ -63,6 +63,9 @@
### Fixed
- Fixed the LSP client advertising transactional text edits despite applying multi-file workspace edits sequentially ([#8375](https://github.com/can1357/oh-my-pi/issues/8375)).
### 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
packages/coding-agent/src/lsp/tool.ts
+55 -6
View File
@@ -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 },
};
}
packages/coding-agent/test/tools/lsp-regressions.test.ts
+41 -1
View File
@@ -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";
@@ -1600,6 +1600,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[] = [];