From 5cdc450fa63e3eb005f96a94708e18891d41ffad Mon Sep 17 00:00:00 2001 From: oldschoola Date: Mon, 25 May 2026 23:36:08 -0700 Subject: [PATCH] =?UTF-8?q?test(coding-agent/lsp):=20cover=20$-prefixed=20?= =?UTF-8?q?identifiers,=20create=E2=86=92edit=20ordering,=20folder-delete?= =?UTF-8?q?=20subtree=20flush?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the three regression tests the PR body claimed but #1389 had not actually shipped: - $-prefixed identifier resolution: asserts resolveSymbolColumn("$store") on a line with bar$store + $store returns the standalone column (16), not the substring inside the compound identifier (7). - create→edit ordering on the same URI: the motivating LSP §3.16.2 case ("Extract to new file" code actions emitting [CreateFile, TextDocumentEdit] for the same URI). Pre-fix this threw ENOENT. - Folder-delete subtree flush: mirror of the existing folder-rename test for the delete arm — child-file edits must land before the parent folder goes away. Also adds a CHANGELOG sentence noting that when a WorkspaceEdit supplies both `changes` and `documentChanges`, the new code uses documentChanges exclusively per LSP §3.16.2 (previously the two were merged). --- packages/coding-agent/CHANGELOG.md | 2 +- .../test/tools/lsp-regressions.test.ts | 125 ++++++++++++++++++ 2 files changed, 126 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 74489b89e..6a53fa26c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -28,7 +28,7 @@ - Updated Tavily missing-credential feedback to prompt users to configure an API-key provider setting instead of referencing `agent.db` directly - Refreshed expired OpenAI Codex OAuth tokens during `web_search` execution and persisted the updated credentials so searches continue working after token expiry - Fixed the LSP symbol-resolver `BARE_IDENTIFIER_RE` (`/^[A-Za-z_][\w]*$/`) rejecting `$`-prefixed identifiers (`$store`, `$count`, RxJS observables, Svelte stores, Angular signals). Without the word-boundary check, searching for `$store` on a line containing `bar$store` returned the offset inside the compound identifier rather than the standalone occurrence, feeding a wrong column to the LSP server. Pattern now `/^[$A-Za-z_][\w$]*$/`; the companion `IDENTIFIER_CHAR_RE` already contained `$`. -- Fixed `applyWorkspaceEdit` writing all text edits before walking `documentChanges` for resource operations. LSP §3.16.2 requires clients to apply `documentChanges` in declared order, so any server emitting `{kind: "create", uri: X}` followed by a `TextDocumentEdit` for `X` (e.g. "Extract to new file" code actions, some rename responses) broke: the edit ran against a non-existent file, then the create happened. `applyWorkspaceEdit` now walks `documentChanges` once in declared order; per-URI text edits are coalesced into a pending Map and flushed immediately before any subsequent resource op for the same URI. Legacy `changes`-map-only payloads are unchanged. Folder-level `rename`/`delete` ops now flush every pending URI under the affected subtree (not just the exact target) so child-file edits queued before a parent-folder move land at the original location instead of dangling against a non-existent path on the final flush. Rename ops additionally flush pending edits queued against `renameOp.newUri` (and its descendants) **before** `fs.rename` runs, so edits intended for the pre-rename target file are applied before the rename clobbers or replaces it (relevant under `options.overwrite`/`options.ignoreIfExists`). +- Fixed `applyWorkspaceEdit` writing all text edits before walking `documentChanges` for resource operations. LSP §3.16.2 requires clients to apply `documentChanges` in declared order, so any server emitting `{kind: "create", uri: X}` followed by a `TextDocumentEdit` for `X` (e.g. "Extract to new file" code actions, some rename responses) broke: the edit ran against a non-existent file, then the create happened. `applyWorkspaceEdit` now walks `documentChanges` once in declared order; per-URI text edits are coalesced into a pending Map and flushed immediately before any subsequent resource op for the same URI. Legacy `changes`-map-only payloads are unchanged. Folder-level `rename`/`delete` ops now flush every pending URI under the affected subtree (not just the exact target) so child-file edits queued before a parent-folder move land at the original location instead of dangling against a non-existent path on the final flush. Rename ops additionally flush pending edits queued against `renameOp.newUri` (and its descendants) **before** `fs.rename` runs, so edits intended for the pre-rename target file are applied before the rename clobbers or replaces it (relevant under `options.overwrite`/`options.ignoreIfExists`). When a `WorkspaceEdit` payload supplies both `changes` and `documentChanges`, the `documentChanges` arm is now used exclusively per LSP §3.16.2 ("if documentChanges are supplied … servers should use them in preference to changes"); previously the two were merged. ### Fixed diff --git a/packages/coding-agent/test/tools/lsp-regressions.test.ts b/packages/coding-agent/test/tools/lsp-regressions.test.ts index f199f98c8..447dc7594 100644 --- a/packages/coding-agent/test/tools/lsp-regressions.test.ts +++ b/packages/coding-agent/test/tools/lsp-regressions.test.ts @@ -11,6 +11,8 @@ import { applyWorkspaceEdit } from "@oh-my-pi/pi-coding-agent/lsp/edits"; import { renderCall, renderResult } from "@oh-my-pi/pi-coding-agent/lsp/render"; import type { CodeAction, + CreateFile, + DeleteFile, Diagnostic, LspClient, RenameFile, @@ -1044,4 +1046,127 @@ describe("lsp regressions", () => { tempDir.removeSync(); } }); + it("resolves $-prefixed identifiers past compound matches", async () => { + // Pre-fix, BARE_IDENTIFIER_RE rejected leading `$`, so requireWordBoundary + // was false and `resolveSymbolColumn(_, _, "$store")` returned the column + // inside `bar$store` rather than the standalone occurrence, feeding the + // LSP server the wrong column. The new regex `/^[$A-Za-z_][\w$]*$/` plus + // IDENTIFIER_CHAR_RE's existing `$` membership enforces the boundary. + const tempDir = TempDir.createSync("@omp-lsp-dollar-identifier-"); + try { + const filePath = path.join(tempDir.path(), "store.ts"); + // Standalone `$store` starts at column 16; compound `bar$store` + // contains the substring at column 7. Old code returned 7; new code + // returns 16. + await Bun.write(filePath, "let bar$store = $store + 1;\n"); + + const column = await resolveSymbolColumn(filePath, 1, "$store"); + expect(column).toBe(16); + + // `bar$store` is itself a valid `$`-bearing identifier and resolves + // to its own start, not into either fragment. + const compoundColumn = await resolveSymbolColumn(filePath, 1, "bar$store"); + expect(compoundColumn).toBe(4); + } finally { + tempDir.removeSync(); + } + }); + + it("applies a create op followed by a text edit for the same URI in declared order", async () => { + // LSP §3.16.2 motivating case for the rewrite: "Extract to new file" + // code actions emit `[CreateFile(newUri), TextDocumentEdit(newUri, ...)]`. + // Pre-fix, all text edits flushed first → applyTextEdits opened a + // not-yet-created file → ENOENT. The new walk processes each entry in + // order, so the create lands first and the edit reads the empty file + // the create just wrote. + const tempDir = TempDir.createSync("@omp-lsp-create-then-edit-"); + try { + const newFilePath = path.join(tempDir.path(), "extracted.ts"); + expect(fs.existsSync(newFilePath)).toBe(false); + + const newUri = fileToUri(newFilePath); + const createOp: CreateFile = { + kind: "create", + uri: newUri, + }; + const textEdit: TextDocumentEdit = { + textDocument: { uri: newUri, version: null }, + edits: [ + { + range: { + start: { line: 0, character: 0 }, + end: { line: 0, character: 0 }, + }, + newText: "export const extracted = 42;\n", + }, + ], + }; + const workspaceEdit: WorkspaceEdit = { + documentChanges: [createOp, textEdit], + }; + + const applied = await applyWorkspaceEdit(workspaceEdit, tempDir.path()); + + expect(fs.existsSync(newFilePath)).toBe(true); + expect(fs.readFileSync(newFilePath, "utf8")).toBe("export const extracted = 42;\n"); + + // Declared order observable in the applied log: create first, then edit. + expect(applied).toHaveLength(2); + expect(applied[0]).toContain("Created"); + expect(applied[0]).toContain("extracted.ts"); + expect(applied[1]).toContain("Applied 1 edit(s)"); + expect(applied[1]).toContain("extracted.ts"); + } finally { + tempDir.removeSync(); + } + }); + + it("flushes pending descendant text edits before a folder delete", async () => { + // Mirror of the folder-rename subtree-flush test for the `delete` arm: + // edits queued against a child URI must land at the original path + // BEFORE the parent folder is removed, otherwise the flush at end of + // walk would target a non-existent path and throw. + const tempDir = TempDir.createSync("@omp-lsp-folder-delete-"); + try { + const srcDir = path.join(tempDir.path(), "src"); + fs.mkdirSync(srcDir, { recursive: true }); + const childPath = path.join(srcDir, "a.ts"); + await Bun.write(childPath, "export const a = 1;\n"); + + const childUri = fileToUri(childPath); + const folderUri = fileToUri(srcDir); + + const childEdit: TextDocumentEdit = { + textDocument: { uri: childUri, version: null }, + edits: [ + { + range: { + start: { line: 0, character: 18 }, + end: { line: 0, character: 19 }, + }, + newText: "999", + }, + ], + }; + const folderDelete: DeleteFile = { + kind: "delete", + uri: folderUri, + }; + const workspaceEdit: WorkspaceEdit = { + documentChanges: [childEdit, folderDelete], + }; + + const applied = await applyWorkspaceEdit(workspaceEdit, tempDir.path()); + + // Folder is gone; "Applied" message proves the flush ran before delete. + expect(fs.existsSync(srcDir)).toBe(false); + expect(applied).toHaveLength(2); + expect(applied[0]).toContain("Applied 1 edit(s)"); + expect(applied[0]).toContain("src/a.ts"); + expect(applied[1]).toContain("Deleted"); + expect(applied[1]).toContain("src"); + } finally { + tempDir.removeSync(); + } + }); });