diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 8edbc03ba..39fae7d7c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -22,6 +22,7 @@ ### Fixed +- Fixed `write` blocking for the full 3-second LSP diagnostics poll in main-agent sessions by wiring it into the deferred late-diagnostics channel; slow diagnostics now return after the short inline window and arrive as an aside. - Fixed `tab.fill`/`tab.click` (and every puppeteer Locator action) timing out after 15s on all pages: the stealth patch routes default `Frame.evaluate`/`waitForFunction` through the isolated world, but `waitForSelector`/Locator results were still transferred to the main world, so Locator's enabled-precondition (`handle.frame.waitForFunction(pred, opts, handle)`) and `page.evaluate(fn, handle)` threw a cross-context handle error that Locators retried silently until timeout. `QueryHandler.waitFor` now returns its result in the isolated world, matching the patched default realm; explicit `//!world=main` evaluation still adopts handles via ElementHandle - Fixed browser runs silently dropping `display("string")`, `console.log`, and `print` output: the runtime emits those as stream text, which the browser embedders (worker and cmux) routed to the debug log only, so the tool result showed a bare "Ran code on tab". Stream text is now buffered and surfaced as ordered display entries alongside `display()` payloads and screenshots. - Fixed `input.fill is not a function` on element handles from `tab.id()`/`tab.ref()`/`tab.waitFor()`: raw puppeteer ElementHandles expose `type()` but not the `fill()` the tool docs promise. Handles handed to user code now carry a `fill()` matching `tab.fill()` semantics (focus, clear, retype). diff --git a/packages/coding-agent/src/edit/index.ts b/packages/coding-agent/src/edit/index.ts index 6c004bb1b..bc1c13e82 100644 --- a/packages/coding-agent/src/edit/index.ts +++ b/packages/coding-agent/src/edit/index.ts @@ -4,19 +4,13 @@ import hashlineDescription from "@oh-my-pi/hashline/prompt.md" with { type: "tex import type { AgentTool, AgentToolContext, AgentToolResult, AgentToolUpdateCallback } from "@oh-my-pi/pi-agent-core"; import type { ToolExample } from "@oh-my-pi/pi-ai"; import { prompt } from "@oh-my-pi/pi-utils"; -import { - createLspWritethrough, - type FileDiagnosticsResult, - flushLspWritethroughBatch, - type WritethroughCallback, - type WritethroughDeferredHandle, - writethroughNoop, -} from "../lsp"; +import { createLspWritethrough, flushLspWritethroughBatch, type WritethroughCallback, writethroughNoop } from "../lsp"; +import { DeferredDiagnostics } from "../lsp/deferred-diagnostics"; import { getDiagnosticsLedger } from "../lsp/diagnostics-ledger"; import applyPatchDescription from "../prompts/tools/apply-patch.md" with { type: "text" }; import patchDescription from "../prompts/tools/patch.md" with { type: "text" }; import replaceDescription from "../prompts/tools/replace.md" with { type: "text" }; -import type { DeferredDiagnosticsEntry, ToolSession } from "../tools"; +import type { ToolSession } from "../tools"; import { truncateForPrompt } from "../tools/approval"; import { isInternalUrlPath } from "../tools/path-utils"; import { type EditMode, normalizeEditMode, resolveEditMode } from "../utils/edit-mode"; @@ -384,12 +378,7 @@ export class EditTool implements AgentTool { readonly #fuzzyThreshold: number; readonly #writethrough: WritethroughCallback; readonly #editMode?: EditMode; - readonly #dedupDiagnostics: boolean; - readonly #pendingDeferredFetches = new Map(); - /** Fallback per-path mutation counter used only when the session does not expose - * a shared one. Prefer `session.bumpFileMutationVersion` so write (and any other - * tool) mutating the same file also invalidates pending late-diagnostics. */ - readonly #editVersionByPath = new Map(); + readonly #deferredDiagnostics: DeferredDiagnostics; constructor(private readonly session: ToolSession) { const { @@ -401,10 +390,11 @@ export class EditTool implements AgentTool { this.#editMode = resolveConfiguredEditMode(envEditVariant); this.#allowFuzzy = resolveAllowFuzzy(session, editFuzzy); this.#fuzzyThreshold = resolveFuzzyThreshold(session, editFuzzyThreshold); - this.#dedupDiagnostics = + const deduplicateDiagnostics = (session.enableLsp ?? true) && session.settings.get("lsp.diagnosticsOnEdit") && session.settings.get("lsp.diagnosticsDeduplicate"); + this.#deferredDiagnostics = new DeferredDiagnostics(session, deduplicateDiagnostics); this.#writethrough = createEditWritethrough(session); } @@ -549,7 +539,7 @@ export class EditTool implements AgentTool { // doubles as the documented full-file overwrite (patch.md ). allowCreateOverwrite: true, writethrough: tool.#writethrough, - beginDeferredDiagnosticsForPath: p => tool.#beginDeferredDiagnosticsForPath(p), + beginDeferredDiagnosticsForPath: p => tool.#deferredDiagnostics.begin(p), }), ); return executeSinglePathEntries(path, runs, batchRequest, onUpdate, tool.session.cwd, signal); @@ -588,7 +578,7 @@ export class EditTool implements AgentTool { allowFuzzy: tool.#allowFuzzy, fuzzyThreshold: tool.#fuzzyThreshold, writethrough: tool.#writethrough, - beginDeferredDiagnosticsForPath: p => tool.#beginDeferredDiagnosticsForPath(p), + beginDeferredDiagnosticsForPath: p => tool.#deferredDiagnostics.begin(p), }), }; }); @@ -612,7 +602,7 @@ export class EditTool implements AgentTool { signal, batchRequest, writethrough: tool.#writethrough, - beginDeferredDiagnosticsForPath: p => tool.#beginDeferredDiagnosticsForPath(p), + beginDeferredDiagnosticsForPath: p => tool.#deferredDiagnostics.begin(p), }); }, }, @@ -638,7 +628,7 @@ export class EditTool implements AgentTool { allowFuzzy: tool.#allowFuzzy, fuzzyThreshold: tool.#fuzzyThreshold, writethrough: tool.#writethrough, - beginDeferredDiagnosticsForPath: p => tool.#beginDeferredDiagnosticsForPath(p), + beginDeferredDiagnosticsForPath: p => tool.#deferredDiagnostics.begin(p), }), ); return executeSinglePathEntries(path, runs, batchRequest, onUpdate, tool.session.cwd, signal); @@ -646,60 +636,4 @@ export class EditTool implements AgentTool { }, }[this.mode]; } - - #beginDeferredDiagnosticsForPath(path: string): WritethroughDeferredHandle { - const existingDeferred = this.#pendingDeferredFetches.get(path); - if (existingDeferred) { - existingDeferred.abort(); - this.#pendingDeferredFetches.delete(path); - } - - const deferredController = new AbortController(); - const editVersion = this.#bumpFileVersion(path); - return { - onDeferredDiagnostics: (lateDiagnostics: FileDiagnosticsResult) => { - this.#pendingDeferredFetches.delete(path); - this.#injectLateDiagnostics(path, lateDiagnostics, editVersion); - }, - signal: deferredController.signal, - finalize: (diagnostics: FileDiagnosticsResult | undefined) => { - if (!diagnostics) { - this.#pendingDeferredFetches.set(path, deferredController); - } else { - deferredController.abort(); - } - }, - }; - } - - #injectLateDiagnostics(path: string, diagnostics: FileDiagnosticsResult, editVersion: number): void { - const effective = this.#dedupDiagnostics - ? getDiagnosticsLedger(this.session).reduce(path, diagnostics) - : diagnostics; - if (this.#dedupDiagnostics && effective.messages.length === 0) return; - - const entry: DeferredDiagnosticsEntry = { - path, - summary: effective.summary ?? "", - messages: effective.messages ?? [], - errored: effective.errored, - // Drop at flush time if a later edit to the same file superseded this fetch. - isStale: () => this.#fileVersion(path) !== editVersion, - }; - this.session.queueDeferredDiagnostics?.(entry); - } - - /** Bump the file's mutation counter (session-global when available). */ - #bumpFileVersion(path: string): number { - if (this.session.bumpFileMutationVersion) return this.session.bumpFileMutationVersion(path); - const next = (this.#editVersionByPath.get(path) ?? 0) + 1; - this.#editVersionByPath.set(path, next); - return next; - } - - /** Read the file's current mutation counter (session-global when available). */ - #fileVersion(path: string): number { - if (this.session.getFileMutationVersion) return this.session.getFileMutationVersion(path); - return this.#editVersionByPath.get(path) ?? 0; - } } diff --git a/packages/coding-agent/src/lsp/deferred-diagnostics.ts b/packages/coding-agent/src/lsp/deferred-diagnostics.ts new file mode 100644 index 000000000..148edd1ce --- /dev/null +++ b/packages/coding-agent/src/lsp/deferred-diagnostics.ts @@ -0,0 +1,66 @@ +import type { DeferredDiagnosticsEntry, ToolSession } from "../tools"; +import { getDiagnosticsLedger } from "./diagnostics-ledger"; +import type { FileDiagnosticsResult, WritethroughDeferredHandle } from "./index"; + +/** Coordinates late LSP diagnostics for one mutation tool instance. */ +export class DeferredDiagnostics { + readonly #pendingFetches = new Map(); + readonly #fallbackVersions = new Map(); + + constructor( + private readonly session: ToolSession, + private readonly deduplicate: boolean, + ) {} + + /** Begin a file mutation and return the handle consumed by LSP writethrough. */ + begin(path: string): WritethroughDeferredHandle { + const existing = this.#pendingFetches.get(path); + if (existing) { + existing.abort(); + this.#pendingFetches.delete(path); + } + + const controller = new AbortController(); + const mutationVersion = this.#bumpVersion(path); + return { + onDeferredDiagnostics: diagnostics => { + this.#pendingFetches.delete(path); + this.#inject(path, diagnostics, mutationVersion); + }, + signal: controller.signal, + finalize: diagnostics => { + if (!diagnostics) { + this.#pendingFetches.set(path, controller); + } else { + controller.abort(); + } + }, + }; + } + + #inject(path: string, diagnostics: FileDiagnosticsResult, mutationVersion: number): void { + const effective = this.deduplicate ? getDiagnosticsLedger(this.session).reduce(path, diagnostics) : diagnostics; + if (this.deduplicate && effective.messages.length === 0) return; + + const entry: DeferredDiagnosticsEntry = { + path, + summary: effective.summary ?? "", + messages: effective.messages ?? [], + errored: effective.errored, + isStale: () => this.#version(path) !== mutationVersion, + }; + this.session.queueDeferredDiagnostics?.(entry); + } + + #bumpVersion(path: string): number { + if (this.session.bumpFileMutationVersion) return this.session.bumpFileMutationVersion(path); + const next = (this.#fallbackVersions.get(path) ?? 0) + 1; + this.#fallbackVersions.set(path, next); + return next; + } + + #version(path: string): number { + if (this.session.getFileMutationVersion) return this.session.getFileMutationVersion(path); + return this.#fallbackVersions.get(path) ?? 0; + } +} diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index a7356d166..348384eed 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -20,6 +20,7 @@ import type { RenderResultOptions } from "../extensibility/custom-tools/types"; import { InternalUrlRouter } from "../internal-urls"; import { parseInternalUrl } from "../internal-urls/parse"; import { createLspWritethrough, type FileDiagnosticsResult, type WritethroughCallback, writethroughNoop } from "../lsp"; +import { DeferredDiagnostics } from "../lsp/deferred-diagnostics"; import { getDiagnosticsLedger } from "../lsp/diagnostics-ledger"; import { getLanguageFromPath, highlightCode, type Theme } from "../modes/theme/theme"; import writeDescription from "../prompts/tools/write.md" with { type: "text" }; @@ -323,12 +324,15 @@ export class WriteTool implements AgentTool this.#deferredDiagnostics?.begin(dst), + ); invalidateFsScanAfterWrite(absolutePath); - this.session.bumpFileMutationVersion?.(absolutePath); + if (!this.#deferredDiagnostics || batchRequest?.flush === false) { + this.session.bumpFileMutationVersion?.(absolutePath); + } const madeExecutable = await maybeMarkExecutableForShebang(absolutePath, cleanContent); const header = maybeWriteSnapshotHeader(this.session, absolutePath, cleanContent); diff --git a/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts b/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts index e091f445e..6702f0c98 100644 --- a/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts +++ b/packages/coding-agent/test/tools/lsp-diagnostics-freshness.test.ts @@ -1,10 +1,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; import * as path from "node:path"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { createLspWritethrough, type FileDiagnosticsResult } from "@oh-my-pi/pi-coding-agent/lsp"; import * as lspClient from "@oh-my-pi/pi-coding-agent/lsp/client"; import * as lspConfig from "@oh-my-pi/pi-coding-agent/lsp/config"; import type { Diagnostic, LspClient, ServerConfig } from "@oh-my-pi/pi-coding-agent/lsp/types"; import { fileToUri } from "@oh-my-pi/pi-coding-agent/lsp/utils"; +import type { DeferredDiagnosticsEntry, ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { WriteTool } from "@oh-my-pi/pi-coding-agent/tools/write"; import { type ptree, TempDir } from "@oh-my-pi/pi-utils"; const TEST_SERVER: ServerConfig = { @@ -352,6 +355,61 @@ describe("LSP diagnostics freshness", () => { expect(await Bun.file(filePath).text()).toBe("export const value: number = 'x';\n"); }); + it("returns the write tool result before slow diagnostics and queues them for the agent", async () => { + const filePath = path.join(tempDir.path(), "write-tool.ts"); + const uri = fileToUri(filePath); + const client = createClient(tempDir.path(), TEST_SERVER); + const clock = new VirtualClock(Date.now()); + installVirtualTime(clock); + + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([["test-lsp", TEST_SERVER]]); + vi.spyOn(lspClient, "getOrCreateClient").mockResolvedValue(client); + vi.spyOn(lspClient, "syncContent").mockImplementation(async (mockClient, syncedFilePath) => { + const syncedUri = fileToUri(syncedFilePath); + mockClient.openFiles.set(syncedUri, { version: 1, languageId: "typescript" }); + }); + vi.spyOn(lspClient, "notifySaved").mockImplementation(async mockClient => { + clock.in(2000, () => { + publishDiagnostics(mockClient, uri, [createDiagnostic("write tool deferred error")], null); + }); + }); + + const queued = Promise.withResolvers(); + const mutationVersions = new Map(); + const session: ToolSession = { + cwd: tempDir.path(), + hasUI: false, + getSessionFile: () => null, + getSessionSpawns: () => "*", + settings: Settings.isolated({ + "lsp.formatOnWrite": false, + "lsp.diagnosticsOnWrite": true, + "lsp.diagnosticsDeduplicate": true, + }), + enableLsp: true, + queueDeferredDiagnostics: entry => queued.resolve(entry), + bumpFileMutationVersion: target => { + const version = (mutationVersions.get(target) ?? 0) + 1; + mutationVersions.set(target, version); + return version; + }, + getFileMutationVersion: target => mutationVersions.get(target) ?? 0, + }; + + const result = await new WriteTool(session).execute("write-deferred", { + path: filePath, + content: "export const value: number = 'x';\n", + }); + + expect(result.details?.diagnostics).toBeUndefined(); + const late = await queued.promise; + expect(late.isStale()).toBe(false); + expect(late.errored).toBe(true); + expect(late.messages.some(message => message.includes("write tool deferred error"))).toBe(true); + expect(await Bun.file(filePath).text()).toBe("export const value: number = 'x';\n"); + }); + it("suppresses TypeScript project diagnostics for orphan files but keeps syntax errors", async () => { const server: ServerConfig = { ...TEST_SERVER,