From 2230361fcde78b265ac557aa504a7eb75eb34c06 Mon Sep 17 00:00:00 2001 From: Larry Gordon Date: Wed, 12 Aug 2026 08:08:53 -0700 Subject: [PATCH] fix(lsp): kept brokered batch content for a denied flush reread Review follow-up on #8052. A batched LSP write recorded only the destination, and the flush reread each entry from disk before post-processing. That reread tolerated `ENOENT` and rethrew everything else, so a destination a sandbox denies for reading as well as writing failed the flush after every write in the batch had already succeeded, the brokered one included. The tool call owning the flush then reported failure for bytes that were on disk, contradicting the seam's promise that a native tool continues as if its own write had worked. The pending entry now carries the content it committed. The flush still prefers a fresh read, so an external change made before the flush wins and a file deleted before it is still not recreated; the remembered bytes stand in only when the read is denied. Recovered content flows into `runLspWritethrough`, whose `writeContent` already routes through `writeFileWithFallback`, so a formatter rewrite of it is brokered too. This also fixes a shape that predates the seam: a plain write-only file (mode `0o200`) failed the same flush with nothing registered at all. Covered by a real-permission test in `test/tools/lsp-batching.test.ts` that brokers a write to a `0o000` file inside a batch and then flushes; rethrowing instead of substituting the remembered bytes fails it with `EACCES` from `flushWritethroughBatch`. --- packages/coding-agent/src/lsp/writethrough.ts | 23 +++++-- .../test/tools/lsp-batching.test.ts | 60 +++++++++++++++++++ 2 files changed, 78 insertions(+), 5 deletions(-) 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"); + }); +});