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`.
This commit is contained in:
Larry Gordon
2026-08-12 08:08:53 -07:00
parent 2c5a9c43aa
commit 2230361fcd
2 changed files with 78 additions and 5 deletions
+18 -5
View File
@@ -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);
@@ -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<string>): 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");
});
});