Files
oh-my-pi/packages/coding-agent/test/tools/file-write-fallback.test.ts
T
Larry Gordon 6e4334c003 feat(extensions): broker denied file writes and deletes
A host that runs omp inside an OS sandbox can grant a path mid-session but cannot
apply that grant to an in-process write: `write` and `edit` do their I/O in the
agent process, so an out-of-workspace write fails and stays failed until the
process restarts under a wider profile.

Nothing available today closes that. A `tool_call` handler can block and a
`tool_result` handler can rewrite content, but neither can re-run a tool.
`ctx.invokeTool` delegates execution, but the delegated native tool runs in the
same process under the same restrictions. And the failure lands AT the write
syscall - after the tool computed the final content, before it returned - so the
bytes are gone with the throw, and reconstructing them means reimplementing
`edit`'s hashline protocol and the snapshot bookkeeping.

The byte-write that `write`, `edit` and `apply_patch` perform on an ordinary file
path already funnels through one two-line primitive
(`file ? file.write(content) : Bun.write(dst, content)`) at four call sites.
Routing that primitive through `writeFileWithFallback` gives an embedder a single
seam to intercept a permission-denied write: the native tool still records its own
snapshot under the real destination path once a handler reports success, so a
follow-up hashline `edit` on that path keeps working.

Only a permission boundary diverts - `EPERM`/`EACCES`/`EROFS`. Two cases needed
more than that:

- `Bun.write` creates missing parents itself, and when that `mkdir` is the denied
  operation it reports the subsequent `open()`'s `ENOENT` instead of the denial -
  making a sandboxed write into a new out-of-tree directory indistinguishable from
  an ordinary bad path. Redoing the `mkdir` explicitly recovers the real errno, and
  because it runs through the same enforcement path as the write it also sees
  kernel-level denials (Seatbelt, LSM) that a `stat`/`access` probe reports as
  writable. If no handler takes the write, the original `ENOENT` is still what
  propagates, with the recovered denial attached as its `cause`.
- `apply_patch` creates the parent as a separate step before writing, so a denial
  there threw before the seam was ever reached. That `mkdir` now tolerates a
  permission denial when a fallback is registered, letting the write report it.

A denial reached through a SYMLINK is never brokered. The in-process write follows
the link, so the kernel denied the link's TARGET, but a handler receives `dst` and
a privileged helper opening it with ordinary follow semantics would land the bytes
wherever the link points. That also defeats the obvious helper-side defence, since
a prefix allowlist passes when the link sits inside the allowed root while its
target does not. omp cannot vouch for the destination, so it refuses rather than
hand the ambiguity to a privileged writer - the same answer `confineToWorkspace`
already gives an unresolvable link.

Removing a file is a different primitive, so it gets its own seam
(`deleteFileWithFallback`, `registerFileDeleteFallback`) covering `edit`'s `REM`,
a hashline `MV`'s source unlink, and `apply_patch`'s delete op. Two differences
from the write path: `ENOENT` is never diverted, since nothing is created on the
way to an unlink and `REM` needs it to become a not-found error; and the seam
refuses a target it can confirm is a directory, because `unlink` on a directory
reports `EPERM` on Darwin and is otherwise indistinguishable from a sandbox
denial. That check cannot always run - a sandbox denying the unlink usually denies
the target's metadata too - so the request carries `confirmedFile`, and a handler
is required to use a plain unlink rather than resolving or recursing.

The two registries are deliberately separate. A write handler brokers `content` to
`dst`, so a delete request reaching it with no content invites brokering an empty
write and truncating the file it was asked to remove.

With nothing registered both seams are inert: the primitives run exactly as
before, a failure rethrows from the same place, and no extra syscalls are
performed.

Scope is deliberately narrow. Archive-member and SQLite writes are unchanged -
neither is a byte-write to a path, so brokering them needs a different request
shape - along with the ACP bridge's `writeTextFile`, the `lsp` tool's own
workspace-edit and formatter writes, and directory removal.
2026-08-14 08:52:34 -07:00

605 lines
21 KiB
TypeScript

import { afterEach, beforeEach, describe, expect, it } from "bun:test";
import * as fs from "node:fs/promises";
import * as os from "node:os";
import * as path from "node:path";
import { applyPatch } from "@oh-my-pi/pi-coding-agent/edit/modes/patch";
import {
addFileDeleteFallback,
addFileWriteFallback,
deleteFileWithFallback,
isPermissionDeniedError,
writeFileWithFallback,
} from "@oh-my-pi/pi-coding-agent/tools/file-write-fallback";
/** Mimics a Node/Bun filesystem error with a structured `code`, without touching a real fs. */
function fsError(code: string, message = `${code}: simulated`): NodeJS.ErrnoException {
const error = new Error(message) as NodeJS.ErrnoException;
error.code = code;
return error;
}
describe("isPermissionDeniedError", () => {
it("is true for EPERM, EACCES, and EROFS error codes", () => {
expect(isPermissionDeniedError(fsError("EPERM"))).toBe(true);
expect(isPermissionDeniedError(fsError("EACCES"))).toBe(true);
expect(isPermissionDeniedError(fsError("EROFS"))).toBe(true);
});
it("is false for unrelated error codes", () => {
expect(isPermissionDeniedError(fsError("ENOENT"))).toBe(false);
expect(isPermissionDeniedError(fsError("EISDIR"))).toBe(false);
expect(isPermissionDeniedError(fsError("ENOSPC"))).toBe(false);
});
it("is false for a plain error with no code or matching message", () => {
expect(isPermissionDeniedError(new Error("something else went wrong"))).toBe(false);
expect(isPermissionDeniedError("not an error")).toBe(false);
expect(isPermissionDeniedError(undefined)).toBe(false);
});
it("defensively matches a permission code embedded only in the message", () => {
// A bridged/transport write can surface a denial as a plain Error with no code.
expect(isPermissionDeniedError(new Error("write failed: EACCES permission denied"))).toBe(true);
});
it("trusts a structured code over a permission name appearing in the path", () => {
// Bun embeds the full path in fs error messages, so a directory literally named
// EACCES would otherwise make an ordinary missing-path ENOENT look like a denial
// and divert a write that should just fail.
expect(isPermissionDeniedError(fsError("ENOENT", "ENOENT: no such file, open '/repo/EACCES/x.txt'"))).toBe(false);
});
});
describe("writeFileWithFallback", () => {
const disposers: Array<() => void> = [];
afterEach(() => {
for (const dispose of disposers.splice(0)) dispose();
});
/** A `BunFile`-shaped stub whose `.write()` always fails with `error`. */
function denyingFile(error: unknown): { write: (content: string) => Promise<number> } {
return {
write: async () => {
throw error;
},
};
}
it("diverts a permission-denied write to a registered handler", async () => {
const seen: Array<{ dst: string; content: string }> = [];
disposers.push(
addFileWriteFallback(async req => {
seen.push({ dst: req.dst, content: req.content });
return true;
}),
);
await writeFileWithFallback("/denied/path.txt", "payload", denyingFile(fsError("EACCES")) as never);
expect(seen).toEqual([{ dst: "/denied/path.txt", content: "payload" }]);
});
it("rethrows a non-permission error without consulting any handler", async () => {
let called = false;
disposers.push(
addFileWriteFallback(async () => {
called = true;
return true;
}),
);
await expect(
writeFileWithFallback("/denied/path.txt", "payload", denyingFile(fsError("EISDIR")) as never),
).rejects.toMatchObject({ code: "EISDIR" });
expect(called).toBe(false);
});
it("retries an ENOENT at most once when the parent turns out to be creatable", async () => {
// A creatable parent means the ENOENT was a race, not a boundary: the helper
// creates the directory and repeats the write. This stub keeps failing, which
// pins the retry at exactly one extra attempt instead of spinning.
const root = await fs.mkdtemp(path.join(os.tmpdir(), "fallback-race-"));
let attempts = 0;
let handlerCalled = false;
disposers.push(
addFileWriteFallback(async () => {
handlerCalled = true;
return true;
}),
);
const file = {
write: async () => {
attempts += 1;
throw fsError("ENOENT");
},
};
await expect(
writeFileWithFallback(path.join(root, "fresh", "path.txt"), "payload", file as never),
).rejects.toMatchObject({ code: "ENOENT" });
expect(attempts).toBe(2);
expect(handlerCalled).toBe(false);
// The repair is the reason the retry happened, so it must be observable.
expect((await fs.stat(path.join(root, "fresh"))).isDirectory()).toBe(true);
await fs.rm(root, { recursive: true, force: true });
});
it("rethrows the ORIGINAL error when the handler returns false", async () => {
const cause = fsError("EACCES");
disposers.push(addFileWriteFallback(async () => false));
await expect(writeFileWithFallback("/denied/path.txt", "payload", denyingFile(cause) as never)).rejects.toBe(
cause,
);
});
it("rethrows the ORIGINAL error when every handler throws", async () => {
const cause = fsError("EACCES");
disposers.push(
addFileWriteFallback(async () => {
throw new Error("handler blew up");
}),
);
await expect(writeFileWithFallback("/denied/path.txt", "payload", denyingFile(cause) as never)).rejects.toBe(
cause,
);
});
it("falls through a throwing handler to the next registered handler", async () => {
disposers.push(
addFileWriteFallback(async () => {
throw new Error("first handler blew up");
}),
);
let secondCalled = false;
disposers.push(
addFileWriteFallback(async () => {
secondCalled = true;
return true;
}),
);
await writeFileWithFallback("/denied/path.txt", "payload", denyingFile(fsError("EACCES")) as never);
expect(secondCalled).toBe(true);
});
it("invokes handlers in registration order and stops at the first success", async () => {
const order: string[] = [];
disposers.push(
addFileWriteFallback(async () => {
order.push("first");
return false;
}),
);
disposers.push(
addFileWriteFallback(async () => {
order.push("second");
return true;
}),
);
disposers.push(
addFileWriteFallback(async () => {
order.push("third");
return true;
}),
);
await writeFileWithFallback("/denied/path.txt", "payload", denyingFile(fsError("EACCES")) as never);
expect(order).toEqual(["first", "second"]);
});
it("stops receiving writes once its disposer runs", async () => {
let calls = 0;
// Registered through `disposers` as well: if an assertion below throws, afterEach
// still removes the handler. A leaked registration is process-global and would
// silently swallow denied writes in every later test file.
const dispose = addFileWriteFallback(async () => {
calls += 1;
return true;
});
disposers.push(dispose);
await writeFileWithFallback("/denied/path.txt", "payload", denyingFile(fsError("EACCES")) as never);
expect(calls).toBe(1);
dispose();
await expect(
writeFileWithFallback("/denied/path.txt", "payload", denyingFile(fsError("EACCES")) as never),
).rejects.toMatchObject({ code: "EACCES" });
expect(calls).toBe(1);
});
// A privileged user is not constrained by mode bits, so `chmod 0o500` denies
// nothing and every expectation here would fail for a reason unrelated to this
// seam. Root is real for a Docker-based local run and for a self-hosted runner.
describe.skipIf(process.getuid?.() === 0)("against real kernel permissions", () => {
let root = "";
beforeEach(async () => {
root = await fs.mkdtemp(path.join(os.tmpdir(), "fallback-kernel-"));
});
afterEach(async () => {
// Restore the mode first: a 0o500 directory cannot be emptied.
await fs.chmod(path.join(root, "locked"), 0o700).catch(() => {});
await fs.rm(root, { recursive: true, force: true });
});
/** A directory the current user may traverse and read, but not create inside. */
async function lockedDir(): Promise<string> {
const dir = path.join(root, "locked");
await fs.mkdir(dir);
await fs.chmod(dir, 0o500);
return dir;
}
it("diverts a real EACCES from creating a file in an unwritable directory", async () => {
const dst = path.join(await lockedDir(), "new.txt");
const seen: Array<{ dst: string; content: string; code: unknown }> = [];
disposers.push(
addFileWriteFallback(async req => {
seen.push({ dst: req.dst, content: req.content, code: (req.cause as NodeJS.ErrnoException).code });
return true;
}),
);
await writeFileWithFallback(dst, "payload");
expect(seen).toEqual([{ dst, content: "payload", code: "EACCES" }]);
});
it("unmasks a denied parent mkdir that Bun reports as ENOENT", async () => {
// Bun's write creates missing parents itself and, when that mkdir is denied,
// surfaces the open()'s ENOENT instead of the denial. Without unmasking, a
// sandboxed write into a new out-of-tree directory never reaches a handler.
const dst = path.join(await lockedDir(), "sub", "new.txt");
const seen: Array<{ dst: string; content: string; code: unknown }> = [];
disposers.push(
addFileWriteFallback(async req => {
seen.push({ dst: req.dst, content: req.content, code: (req.cause as NodeJS.ErrnoException).code });
return true;
}),
);
await writeFileWithFallback(dst, "payload");
expect(seen).toEqual([{ dst, content: "payload", code: "EACCES" }]);
});
it("attaches the recovered denial as `cause` when no handler takes the write", async () => {
// The thrown error stays the ENOENT Bun reported, so behaviour matches a host
// with no fallback registered. But this code has already proven the real
// boundary is EACCES, and discarding that would hand the caller back exactly
// the misleading errno this module exists to see through.
const dst = path.join(await lockedDir(), "sub", "new.txt");
disposers.push(addFileWriteFallback(async () => false));
await expect(writeFileWithFallback(dst, "payload")).rejects.toMatchObject({
code: "ENOENT",
cause: { code: "EACCES" },
});
});
it("leaves an ENOENT alone when a path component is a file rather than a directory", async () => {
const blocker = path.join(root, "blocker");
await Bun.write(blocker, "not a directory");
let called = false;
disposers.push(
addFileWriteFallback(async () => {
called = true;
return true;
}),
);
await expect(writeFileWithFallback(path.join(blocker, "child.txt"), "payload")).rejects.toMatchObject({
code: expect.stringMatching(/^(ENOTDIR|ENOENT)$/),
});
expect(called).toBe(false);
});
it("refuses to broker a denied write reached through a symlink", async () => {
// The escape this blocks: the agent creates a link inside a directory the
// sandbox permits, pointing at a target it does not. The in-process write
// follows the link and the kernel denies the TARGET, so the failure looks
// like any other denial — but a privileged helper handed `dst` would follow
// the same link and land the bytes outside the sandbox. A helper-side prefix
// allowlist cannot catch it, because the link is inside the allowed root.
const secretDir = path.join(root, "off-limits");
await fs.mkdir(secretDir);
const secret = path.join(secretDir, "authorized_keys");
await Bun.write(secret, "original\n");
await fs.chmod(secret, 0o400);
await fs.chmod(secretDir, 0o500);
const link = path.join(root, "innocent-link");
await fs.symlink(secret, link);
let called = false;
disposers.push(
addFileWriteFallback(async () => {
called = true;
return true;
}),
);
try {
await expect(writeFileWithFallback(link, "pwned\n")).rejects.toMatchObject({
code: expect.stringMatching(/^(EACCES|EPERM)$/),
});
expect(called).toBe(false);
expect(await Bun.file(secret).text()).toBe("original\n");
} finally {
await fs.chmod(secretDir, 0o700);
await fs.chmod(secret, 0o600);
}
});
});
// `apply_patch` creates a missing parent before writing, so a denial there used
// to throw before the write — and therefore before the seam — was ever reached.
describe.skipIf(process.getuid?.() === 0)("apply_patch into a denied new directory", () => {
let root = "";
let locked = "";
beforeEach(async () => {
root = await fs.mkdtemp(path.join(os.tmpdir(), "fallback-patch-"));
locked = path.join(root, "locked");
await fs.mkdir(locked);
await fs.chmod(locked, 0o500);
});
afterEach(async () => {
await fs.chmod(locked, 0o700).catch(() => {});
await fs.rm(root, { recursive: true, force: true });
});
it("reaches a registered handler with the bytes for the created file", async () => {
const brokered: Array<{ dst: string; content: string }> = [];
disposers.push(
addFileWriteFallback(async req => {
brokered.push({ dst: req.dst, content: req.content });
return true;
}),
);
const target = path.join(locked, "sub", "new.txt");
const result = await applyPatch({ path: target, op: "create", diff: "hello\n" }, { cwd: root });
expect(result.change).toMatchObject({ type: "create", path: target });
expect(brokered).toEqual([{ dst: target, content: "hello\n" }]);
});
it("still fails when no handler is registered", async () => {
const target = path.join(locked, "sub", "new.txt");
await expect(applyPatch({ path: target, op: "create", diff: "hello\n" }, { cwd: root })).rejects.toMatchObject(
{
code: expect.stringMatching(/^(EACCES|EPERM)$/),
},
);
});
});
});
describe("deleteFileWithFallback", () => {
const disposers: Array<() => void> = [];
afterEach(() => {
for (const dispose of disposers.splice(0)) dispose();
});
it("rethrows ENOENT without consulting a handler", async () => {
// `edit`'s REM turns this into a NotFoundError, so it must not be diverted.
let called = false;
disposers.push(
addFileDeleteFallback(async () => {
called = true;
return true;
}),
);
await expect(deleteFileWithFallback("/nonexistent/nope.txt")).rejects.toMatchObject({ code: "ENOENT" });
expect(called).toBe(false);
});
it("does not consult a registered WRITE handler", async () => {
// A write handler brokers `content` to `dst`. If a delete reached it, brokering
// a request with no content would truncate the file instead of removing it.
let writeCalled = false;
disposers.push(
addFileWriteFallback(async () => {
writeCalled = true;
return true;
}),
);
await expect(deleteFileWithFallback("/nonexistent/nope.txt")).rejects.toMatchObject({ code: "ENOENT" });
expect(writeCalled).toBe(false);
});
describe.skipIf(process.getuid?.() === 0)("against real kernel permissions", () => {
let root = "";
let locked = "";
beforeEach(async () => {
root = await fs.mkdtemp(path.join(os.tmpdir(), "fallback-del-"));
locked = path.join(root, "locked");
await fs.mkdir(locked);
});
afterEach(async () => {
await fs.chmod(locked, 0o700).catch(() => {});
await fs.rm(root, { recursive: true, force: true });
});
/** A file whose containing directory denies the unlink. */
async function lockedFile(name = "victim.txt"): Promise<string> {
const target = path.join(locked, name);
await Bun.write(target, "payload");
await fs.chmod(locked, 0o500);
return target;
}
it("diverts a real denied unlink to a registered handler", async () => {
const target = await lockedFile();
const seen: Array<{ dst: string; code: unknown }> = [];
disposers.push(
addFileDeleteFallback(async req => {
seen.push({ dst: req.dst, code: (req.cause as NodeJS.ErrnoException).code });
return true;
}),
);
await deleteFileWithFallback(target);
expect(seen).toEqual([{ dst: target, code: expect.stringMatching(/^(EACCES|EPERM)$/) }]);
});
it("rethrows the ORIGINAL error when the handler declines", async () => {
const target = await lockedFile();
disposers.push(addFileDeleteFallback(async () => false));
await expect(deleteFileWithFallback(target)).rejects.toMatchObject({
code: expect.stringMatching(/^(EACCES|EPERM)$/),
});
});
it("refuses to divert a directory it can confirm, reporting confirmedFile on files", async () => {
// On Darwin `unlink` on a directory fails EPERM, which by code alone looks
// exactly like a sandbox denial. Brokering it would ask a privileged deleter
// to remove a whole directory for a tool that only ever removes one file.
const dir = path.join(root, "a-directory");
await fs.mkdir(dir);
const seen: boolean[] = [];
disposers.push(
addFileDeleteFallback(async req => {
seen.push(req.confirmedFile);
return true;
}),
);
await expect(deleteFileWithFallback(dir)).rejects.toMatchObject({
code: expect.stringMatching(/^(EPERM|EISDIR)$/),
});
expect(seen).toEqual([]);
expect((await fs.lstat(dir)).isDirectory()).toBe(true);
// A file under a directory that denies the unlink but still permits lstat
// resolves the check, so the handler is told the target is a real file.
const target = await lockedFile("confirmed.txt");
await deleteFileWithFallback(target);
expect(seen).toEqual([true]);
});
it("still diverts, unresolved, when the target's own metadata is denied", async () => {
// A sandbox that denies the unlink usually denies the metadata too, so the
// directory check cannot run. The write must still reach a handler — that is
// the whole point of the seam — but the handler has to be TOLD the check was
// unresolved, or it may recursively remove a path that is really a directory.
const opaque = path.join(root, "opaque");
await fs.mkdir(opaque);
const victim = path.join(opaque, "buried.txt");
await Bun.write(victim, "payload");
await fs.chmod(opaque, 0o000);
const seen: Array<{ dst: string; confirmedFile: boolean }> = [];
disposers.push(
addFileDeleteFallback(async req => {
seen.push({ dst: req.dst, confirmedFile: req.confirmedFile });
return true;
}),
);
try {
await deleteFileWithFallback(victim);
expect(seen).toEqual([{ dst: victim, confirmedFile: false }]);
} finally {
await fs.chmod(opaque, 0o700);
}
});
it("rethrows a non-permission lstat failure rather than diverting", async () => {
// `ENOTDIR` from a path component that is a file is a genuinely bad path, not
// a boundary, so the seam must not paper over it by consulting a handler.
const blocker = path.join(root, "not-a-dir");
await Bun.write(blocker, "payload");
let called = false;
disposers.push(
addFileDeleteFallback(async () => {
called = true;
return true;
}),
);
await expect(deleteFileWithFallback(path.join(blocker, "child.txt"))).rejects.toMatchObject({
code: "ENOTDIR",
});
expect(called).toBe(false);
});
it("reports confirmedFile false for a symlink, so a handler cannot safely resolve it", async () => {
// Unlinking a symlink is a legitimate file removal, so this still diverts. But
// a handler that realpaths `dst` for auditing, or removes it recursively,
// would act on the link's TARGET — a directory tree, here. `confirmedFile`
// must therefore be false even though `lstat` succeeded.
const targetDir = path.join(root, "link-target-dir");
await fs.mkdir(targetDir);
await Bun.write(path.join(targetDir, "keep.txt"), "keep me");
const link = path.join(locked, "dir-link");
await fs.symlink(targetDir, link);
await fs.chmod(locked, 0o500);
const seen: boolean[] = [];
disposers.push(
addFileDeleteFallback(async req => {
seen.push(req.confirmedFile);
return true;
}),
);
await deleteFileWithFallback(link);
expect(seen).toEqual([false]);
// The target must be untouched: the seam only ever asked for the link.
expect(await Bun.file(path.join(targetDir, "keep.txt")).text()).toBe("keep me");
});
it("diverts a denied unlink issued through a BunFile handle", async () => {
// `LspFileSystem.delete` is the only caller that passes a `BunFile`, and it is
// covered only transitively, so the `file.unlink()` branch would otherwise
// never be exercised directly.
const target = await lockedFile("via-handle.txt");
const seen: string[] = [];
disposers.push(
addFileDeleteFallback(async req => {
seen.push(req.dst);
return true;
}),
);
await deleteFileWithFallback(target, Bun.file(target));
expect(seen).toEqual([target]);
});
it("routes an apply_patch delete op through the seam", async () => {
const target = await lockedFile("doomed.txt");
const removed: string[] = [];
disposers.push(
addFileDeleteFallback(async req => {
await fs.chmod(locked, 0o700);
await fs.unlink(req.dst);
removed.push(req.dst);
return true;
}),
);
const result = await applyPatch({ path: target, op: "delete" }, { cwd: root });
expect(result.change).toMatchObject({ type: "delete", path: target });
expect(removed).toEqual([target]);
expect(await Bun.file(target).exists()).toBe(false);
});
});
});