From fc37f20dea52f369b45c2afd73340be3b4f6b5ab Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 13 Aug 2026 03:06:34 +0000 Subject: [PATCH] fix(lsp): abort rename_file when willRenameFiles fails on a supporting server The willRenameFiles loop caught every non-abort, non-method-not-found error into serverNotes and fell through to fs.rename, so a genuine failure from a server that supports the request moved the path without its semantic edits, leaving references dangling. Split client acquisition from the request, track hard failures, and abort before any mutation when a supporting server errors. Servers replying method-not-found are still skipped without blocking. Fixes #8380 --- packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/src/lsp/tool.ts | 45 +++++- .../test/tools/lsp-regressions.test.ts | 134 ++++++++++++++++++ 3 files changed, 182 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b387e76fd..99c0b4633 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `lsp` `rename_file` moving the path even when a server that supports `workspace/willRenameFiles` failed the request, leaving references dangling; the rename now aborts before any filesystem mutation and reports the failure ([#8380](https://github.com/can1357/oh-my-pi/issues/8380)). + ## [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..19aec07f7 100644 --- a/packages/coding-agent/src/lsp/tool.ts +++ b/packages/coding-agent/src/lsp/tool.ts @@ -65,6 +65,7 @@ import { type Hover, type Location, type LocationLink, + type LspClient, type LspParams, type LspToolDetails, lspSchema, @@ -484,14 +485,32 @@ export class LspTool implements AgentTool(); const perServerEdits: Array<{ serverName: string; edit: WorkspaceEdit }> = []; const serverNotes: string[] = []; + // Servers that support workspace/willRenameFiles (i.e. did not reply + // method-not-found) but failed the request. Their semantic edits are + // owed but missing, so on apply the rename MUST NOT mutate the workspace + // — moving the path without those edits leaves dangling references + // (issue #8380). + const hardFailures: string[] = []; for (const [serverName, serverConfig] of servers) { throwIfAborted(signal); + let client: LspClient; try { - const client = await getOrCreateClient(serverConfig, this.session.cwd, undefined, signal); + client = await getOrCreateClient(serverConfig, this.session.cwd, undefined, signal); if (isProjectAwareLspServer(serverConfig)) { await waitForProjectLoaded(client, signal); } + } catch (err) { + if (err instanceof ToolAbortError || signal?.aborted) { + throw err; + } + // Could not reach the server at all; note it but don't block — + // this is not a willRenameFiles failure. + const msg = err instanceof Error ? err.message : String(err); + serverNotes.push(` ${serverName}: ${msg}`); + continue; + } + try { const result = (await sendRequest( client, "workspace/willRenameFiles", @@ -506,9 +525,13 @@ export class LspTool implements AgentTool 0) { + const lines: string[] = [ + `Error: aborted rename; workspace/willRenameFiles failed on ${hardFailures.join(", ")}, so semantic references would not be updated. No files were moved.`, + ]; + lines.push(" Server notes:"); + lines.push(...serverNotes); + return { + content: [{ type: "text", text: lines.join("\n") }], + details: { + action, + serverName: Array.from(respondingServers).join(", "), + success: false, + request: params, + }, + }; + } + const summary: string[] = []; // Coalesce per-URI edits across servers before applying. Each server diff --git a/packages/coding-agent/test/tools/lsp-regressions.test.ts b/packages/coding-agent/test/tools/lsp-regressions.test.ts index 2d905928c..1020479a7 100644 --- a/packages/coding-agent/test/tools/lsp-regressions.test.ts +++ b/packages/coding-agent/test/tools/lsp-regressions.test.ts @@ -2012,6 +2012,140 @@ describe("lsp regressions", () => { } }); + it("rename_file aborts before mutation when willRenameFiles fails on a supporting server", async () => { + const tempDir = TempDir.createSync("@omp-lsp-rename-file-fail-"); + try { + const sourceFile = path.join(tempDir.path(), "src", "old.ts"); + const destFile = path.join(tempDir.path(), "src", "new.ts"); + await Bun.write(sourceFile, "export const value = 42;\n"); + + const server: ServerConfig = { command: "test-lsp", fileTypes: ["ts"], rootMarkers: [] }; + const client: LspClient = { + name: "test-lsp", + cwd: tempDir.path(), + config: server, + proc: { + stdin: { write() {}, flush: async () => {} }, + } as unknown as LspClient["proc"], + requestId: 0, + diagnostics: new Map(), + diagnosticsVersion: 0, + openFiles: new Map(), + pendingRequests: new Map(), + messageBuffer: new Uint8Array(), + isReading: false, + status: "ready", + lastActivity: Date.now(), + writeQueue: Promise.resolve(), + activeProgressTokens: new Set(), + projectLoaded: Promise.resolve(), + resolveProjectLoaded: () => {}, + }; + + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ + servers: { "test-lsp": server }, + idleTimeoutMs: undefined, + }); + vi.spyOn(lspClient, "getOrCreateClient").mockResolvedValue(client); + + // A server that supports willRenameFiles but fails with a real error + // (not method-not-found). The rename must not proceed. + vi.spyOn(lspClient, "sendRequest").mockImplementation(async (_client, method) => { + if (method === "workspace/willRenameFiles") { + throw new Error("internal error: index not ready"); + } + return null; + }); + const notifySpy = vi.spyOn(lspClient, "sendNotification").mockResolvedValue(); + + const tool = new LspTool(makeLspSession(tempDir.path())); + const result = await tool.execute("rename-file-fail", { + action: "rename_file", + file: sourceFile, + new_name: destFile, + timeout: 5, + }); + + // No filesystem mutation. + expect(fs.existsSync(sourceFile)).toBe(true); + expect(fs.existsSync(destFile)).toBe(false); + // No didRenameFiles notification. + expect(notifySpy).not.toHaveBeenCalledWith(expect.anything(), "workspace/didRenameFiles", expect.anything()); + + expect(result.details).toMatchObject({ action: "rename_file", success: false }); + const output = result.content + .filter(block => block.type === "text") + .map(block => block.text) + .join("\n"); + expect(output).toContain("aborted rename"); + expect(output).toContain("index not ready"); + } finally { + vi.restoreAllMocks(); + tempDir.removeSync(); + } + }); + + it("rename_file skips a server that replies method-not-found and still renames", async () => { + const tempDir = TempDir.createSync("@omp-lsp-rename-file-mnf-"); + try { + const sourceFile = path.join(tempDir.path(), "src", "old.ts"); + const destFile = path.join(tempDir.path(), "src", "new.ts"); + await Bun.write(sourceFile, "export const value = 42;\n"); + + const server: ServerConfig = { command: "test-lsp", fileTypes: ["ts"], rootMarkers: [] }; + const client: LspClient = { + name: "test-lsp", + cwd: tempDir.path(), + config: server, + proc: { + stdin: { write() {}, flush: async () => {} }, + } as unknown as LspClient["proc"], + requestId: 0, + diagnostics: new Map(), + diagnosticsVersion: 0, + openFiles: new Map(), + pendingRequests: new Map(), + messageBuffer: new Uint8Array(), + isReading: false, + status: "ready", + lastActivity: Date.now(), + writeQueue: Promise.resolve(), + activeProgressTokens: new Set(), + projectLoaded: Promise.resolve(), + resolveProjectLoaded: () => {}, + }; + + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ + servers: { "test-lsp": server }, + idleTimeoutMs: undefined, + }); + vi.spyOn(lspClient, "getOrCreateClient").mockResolvedValue(client); + vi.spyOn(lspClient, "sendRequest").mockImplementation(async (_client, method) => { + if (method === "workspace/willRenameFiles") { + throw new Error("Method not found: -32601"); + } + return null; + }); + vi.spyOn(lspClient, "sendNotification").mockResolvedValue(); + + const tool = new LspTool(makeLspSession(tempDir.path())); + const result = await tool.execute("rename-file-mnf", { + action: "rename_file", + file: sourceFile, + new_name: destFile, + timeout: 5, + }); + + // Unsupported server does not block: the path still moves. + expect(fs.existsSync(sourceFile)).toBe(false); + expect(fs.existsSync(destFile)).toBe(true); + expect(result.details).toMatchObject({ action: "rename_file", success: true }); + } finally { + vi.restoreAllMocks(); + tempDir.removeSync(); + } + }); + it("rename_file with apply:false previews edits without filesystem changes", async () => { const tempDir = TempDir.createSync("@omp-lsp-rename-file-preview-"); try {