diff --git a/packages/coding-agent/src/lsp/writethrough.ts b/packages/coding-agent/src/lsp/writethrough.ts index f82d75ac0..959cafeda 100644 --- a/packages/coding-agent/src/lsp/writethrough.ts +++ b/packages/coding-agent/src/lsp/writethrough.ts @@ -1,7 +1,7 @@ import * as fs from "node:fs"; import { isEnoent, logger, once, untilAborted } from "@oh-my-pi/pi-utils"; import type { BunFile } from "bun"; -import { writeFileWithFallback } from "../tools/file-write-fallback"; +import { isPermissionDeniedError, writeFileWithFallback } from "../tools/file-write-fallback"; import { FileChangeType, notifyWorkspaceWatchedFiles } from "./client"; import { getServersForFile } from "./config"; import { @@ -75,6 +75,12 @@ interface PendingWritethrough { dst: string; file?: BunFile; changeType: FileChangeType; + /** + * The bytes this entry committed. The flush prefers a fresh read of `dst` so + * post-processing sees whatever else in the batch touched the file, and falls + * back to these when that read is denied. + */ + content: string; } interface RunLspWritethroughOptions { @@ -455,9 +461,16 @@ async function flushWritethroughBatch( try { content = await fs.promises.readFile(entry.dst, "utf8"); } catch (error) { - if (!isEnoent(error)) throw error; - bundle?.finalize(undefined); - continue; + if (isEnoent(error)) { + bundle?.finalize(undefined); + continue; + } + // A brokered write lands bytes this process may not be able to read + // back: a sandbox that denies the write commonly denies the read too. + // Failing here would fail a flush whose every write succeeded, so the + // content this entry committed stands in for the unreadable file. + if (!isPermissionDeniedError(error)) throw error; + content = entry.content; } const deferredInner = bundle && @@ -549,7 +562,7 @@ export function createLspWritethrough(cwd: string, options?: WritethroughOptions } const state = getOrCreateWritethroughBatch(batch.id, resolvedOptions); - state.entries.set(dst, { dst, file, changeType }); + state.entries.set(dst, { dst, file, changeType, content }); if (!batch.flush) return undefined; writethroughBatches.delete(batch.id); diff --git a/packages/coding-agent/test/tools/lsp-batching.test.ts b/packages/coding-agent/test/tools/lsp-batching.test.ts index 73bf14ce3..e0f44bc5b 100644 --- a/packages/coding-agent/test/tools/lsp-batching.test.ts +++ b/packages/coding-agent/test/tools/lsp-batching.test.ts @@ -1,8 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +import * as fs from "node:fs/promises"; import * as path from "node:path"; import { createLspWritethrough } from "@oh-my-pi/pi-coding-agent/lsp"; import * as lspConfig from "@oh-my-pi/pi-coding-agent/lsp/config"; import type { LinterClient, ServerConfig } from "@oh-my-pi/pi-coding-agent/lsp/types"; +import { addFileWriteFallback } from "@oh-my-pi/pi-coding-agent/tools/file-write-fallback"; import { TempDir } from "@oh-my-pi/pi-utils"; function createFormatter(format: (filePath: string, content: string) => Promise): ServerConfig { @@ -180,3 +182,61 @@ describe("createLspWritethrough batching", () => { expect(await Bun.file(filePath).text()).toBe("const single = true;\n"); }); }); + +// A privileged user is not constrained by mode bits: a 0o000 file stays both +// writable and readable, so the write would never be denied and the seam under +// test would never engage. +describe.skipIf(process.getuid?.() === 0)("createLspWritethrough batching with a brokered write", () => { + let tempDir: TempDir; + let root = ""; + const disposers: Array<() => void> = []; + + beforeEach(async () => { + tempDir = TempDir.createSync("@omp-lsp-batch-broker-"); + // The seam hands handlers a symlink-resolved path and `os.tmpdir()` sits + // under `/var` — itself a link — on macOS, so a lexical fixture root would + // differ from the brokered path for a reason unrelated to this test. + root = await fs.realpath(tempDir.path()); + }); + + afterEach(async () => { + for (const dispose of disposers.splice(0)) dispose(); + vi.restoreAllMocks(); + await fs.chmod(path.join(root, "opaque.ts"), 0o600).catch(() => {}); + tempDir.removeSync(); + }); + + it("flushes a batch whose brokered destination cannot be read back", async () => { + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([]); + const writethrough = createLspWritethrough(root, { enableFormat: true, enableDiagnostics: true }); + + // Denied for writing and for reading at once, which is what a sandbox that + // hides a path produces: the direct write fails, a privileged helper lands + // the bytes, and this process still cannot read them back. + const opaque = path.join(root, "opaque.ts"); + await Bun.write(opaque, "const before = true;\n"); + await fs.chmod(opaque, 0o000); + + const brokered: Array<{ dst: string; content: string }> = []; + disposers.push( + addFileWriteFallback(async req => { + brokered.push({ dst: req.dst, content: req.content }); + await fs.chmod(req.dst, 0o600); + await Bun.write(req.dst, req.content); + await fs.chmod(req.dst, 0o000); + return true; + }), + ); + + const sibling = path.join(root, "sibling.ts"); + const batchId = `brokered-${Date.now()}`; + await writethrough(opaque, "const after = true;\n", undefined, undefined, { id: batchId, flush: false }); + await writethrough(sibling, "const other = true;\n", undefined, undefined, { id: batchId, flush: true }); + + expect(brokered).toEqual([{ dst: opaque, content: "const after = true;\n" }]); + expect(await Bun.file(sibling).text()).toBe("const other = true;\n"); + await fs.chmod(opaque, 0o400); + expect(await Bun.file(opaque).text()).toBe("const after = true;\n"); + }); +});