Merge PR #8052: feat(extensions): broker denied file writes and deletes (@psyrendust)

This commit is contained in:
can1357
2026-08-17 11:16:08 +03:00
19 changed files with 2429 additions and 24 deletions
+132
View File
@@ -123,6 +123,7 @@ Core methods:
- `setModel`, `getThinkingLevel`, `setThinkingLevel`
- `getServiceTiers`, `setServiceTier`
- `registerProvider`
- `registerFileWriteFallback`, `registerFileDeleteFallback`
- `events` (shared event bus)
`getServiceTiers()` returns a detached snapshot of the session's live per-family tier map. `setServiceTier(family, tier)` changes one family for subsequent requests; pass `undefined` to clear that session override. OpenAI accepts `auto`, `default`, `flex`, `scale`, or `priority`; Anthropic accepts `priority`; Google accepts `flex` or `priority`. Changes made while a response is streaming do not alter that in-flight request.
@@ -370,6 +371,137 @@ pi.registerTool({
`tool_call`/`tool_result` intercept all tools once the registry is wrapped in `sdk.ts`, including built-ins and extension/custom tools. `ToolDefinition` also supports optional `hidden`, `defaultInactive`, `loadMode` (`"discoverable"` by default, or `"essential"`), `deferrable`, `approval` (`"exec"` by default), `strict`, `mcpServerName`, `mcpToolName`, `renderCall`, and `renderResult` fields.
### File write fallback (`registerFileWriteFallback`)
`write`, `edit` and `apply_patch` perform the real byte-write to an ordinary file
path through one shared primitive
(`file ? file.write(content) : Bun.write(dst, content)`). When that primitive fails
with a permission error (`EPERM`/`EACCES`/`EROFS` — every other error, such as
`EISDIR`, is unaffected), the coding agent consults handlers registered
via `pi.registerFileWriteFallback` before giving up:
```ts
import type { FileWriteFallbackHandler } from "@oh-my-pi/pi-coding-agent";
const writeThroughBroker: FileWriteFallbackHandler = async (req, ctx) => {
// req: { dst: string; content: string; cause: unknown }
const ok = await myPrivilegedWriter.write(req.dst, req.content);
return ok;
};
pi.registerFileWriteFallback(writeThroughBroker);
```
Handlers run in registration order; the first one to resolve `true` counts as the
bytes being durably on disk, and the native tool continues exactly as if its own
write had succeeded — including recording its file snapshot under the real
destination path, so a later hashline `edit` on that path keeps working. A
throwing handler is logged and skipped in favor of the next one — per handler, so a
later handler registered by the same extension still runs; if every handler
returns `false` (or none are registered), the original error is rethrown
unchanged. Intended for a host that embeds the agent inside a sandbox denying
direct filesystem writes but exposing a privileged write channel.
`req.dst` is the **symlink-resolved** destination, not the path the tool was given.
The kernel follows every component above the last, so `ws/link/file` under a
`ws/link -> /elsewhere` link lands outside `ws` while still looking in-workspace, and
a prefix allowlist in your handler would pass on that innocent-looking path. For a
write the final component is followed too, so it is resolved as well; for a delete it
is not, because `unlink` removes a link rather than what it points at (so a delete
`req.dst` may itself name a link). Treat `req.dst` as authoritative and do not
re-derive the target from anything else. When the real destination cannot be
established — a dangling final link, or an ancestor this process may not resolve — no
handler is consulted at all and the original error is rethrown, because there is no
destination to hand a privileged writer.
Two details matter when the destination is outside what the host allows:
- **A missing parent directory.** `Bun.write` creates missing parents itself, and
when that `mkdir` is the operation being denied it reports the subsequent
`open()`'s `ENOENT` rather than the denial. The agent redoes the `mkdir`
explicitly to recover the real errno, so this still reaches a handler — with
`req.cause` set to the `mkdir` denial. In that case `req.dst`'s parent does not
exist yet and the handler is responsible for creating it. An `ENOENT` with a
genuinely creatable or invalid parent is not diverted. (`apply_patch` creates the
parent as a separate step before writing; that `mkdir` tolerates a denial when a
fallback is registered, so the write still reaches the handler.)
- **A hashline `MV`.** `edit`'s move writes its destination directly rather than
through the LSP writethrough. It is routed to the same handlers, and the source
unlink goes to the delete seam below, so a move out of a directory you cannot
write completes too.
This is deliberately not an interception of every write the agent can make. A
permission error from these surfaces as it does today, with no handler consulted:
- `write` to an archive member (`foo.zip:entry`) or to a SQLite row. Neither is a
byte-write to `dst`: an archive rewrite reads the whole archive, replaces one
entry, writes a temp file and renames over the original, so what lands is a whole
binary container rather than the string the tool was handed; a SQLite write is a
row operation inside the database engine with no byte payload at all. Brokering
either needs a different request shape than "these bytes belong at this path".
- The ACP bridge's `writeTextFile`, which hands the write to a remote client.
- The `lsp` tool's own writes: applying a workspace edit or code action, and the
Biome formatter, which writes the buffer and then shells out to `biome format
--write` — a subprocess write no in-process seam can reach.
### File delete fallback (`registerFileDeleteFallback`)
Removing a file is a different primitive from writing one, and it has its own seam:
```ts
pi.registerFileDeleteFallback(async (req, ctx) => {
// req: { dst; cause; confirmedFile; sessionId } — no `content`.
return await myPrivilegedWriter.unlink(req.dst);
});
```
It covers `edit`'s `REM`, the source side of a hashline `MV`, and `apply_patch`'s
delete op, and follows the same rules as the write seam: same permission codes, first
`true` wins, a throwing handler is skipped, the original error is rethrown if none
succeed, and nothing happens at all when no handler is registered. Two differences:
- **`ENOENT` is never diverted.** Nothing is created on the way to an unlink, so a
missing file genuinely is missing — `REM` turns it into a not-found error.
- **A handler must unlink, never remove recursively.** `unlink` on a directory reports
`EPERM` on macOS, which is indistinguishable from a sandbox denial by error code
alone, so the seam `lstat`s the target and refuses to divert a directory. But when
the target's own metadata sits behind the same boundary that denied the unlink —
the common sandbox case — that check cannot be resolved, and `req.dst` may then be a
directory. `req.confirmedFile` is `true` only when the seam positively established
the target is a plain regular file; a symlink reports `false` too, since unlinking a
link is fine but resolving it acts on something else entirely. A privileged helper
that recursively removes `req.dst`, or realpaths it first, would act far outside
what a tool that only ever removes one file asked for.
**Registering for deletes is deliberately separate from registering for writes.** A
write handler brokers `req.content` to `req.dst`; if a delete request reached it, the
missing content invites brokering an empty write and *truncating* the file that was
meant to be removed. A write-only handler therefore never sees a delete.
Two lifecycle constraints, which apply to both seams:
- **Register during extension load** (from the default factory), like other
`register*` calls. Handlers are installed when `ExtensionRunner.initialize` runs;
an extension that registered nothing by then is skipped entirely, so a first
registration made later never takes effect. The `ctx` a handler receives is built
per invocation, not captured at install time, so `ctx.cwd` and `ctx.hasUI` describe
the session as it is when the mutation is denied — a workspace change (`/move`) is
reflected in the next request rather than pinned to load time.
- **The registries are process-wide.** A process can host several sessions (a subagent
gets its own runner), so a handler may be consulted for a denied write or delete
from any session in the process — not only the one whose extension registered it.
This is deliberate: a subagent spawned with restricted tools loads no extensions of
its own, and a host that registers once in its top-level session still expects its
subagents' writes brokered. `req.sessionId` names the session that issued the
mutation (`undefined` when it did not come from a tool call), and
`ctx.sessionManager.getSessionId()` names the handler's own — compare them to make
the decision per session. It matters most before prompting: `ctx.ui` belongs to the
handler's session, not necessarily to the one being asked about. Handlers are
removed on `session_shutdown`.
With nothing registered none of this engages: the primitive runs exactly as it did
before and performs no extra syscalls.
## UI integration points
`ctx.ui` implements the `ExtensionUIContext` interface. Support differs by mode.
+3
View File
@@ -2,6 +2,9 @@
## [Unreleased]
### Added
- Added `ExtensionAPI.registerFileWriteFallback(handler)` and `ExtensionAPI.registerFileDeleteFallback(handler)`, letting an extension supply a fallback writer or deleter that is consulted when a native `write`, `edit`, or `apply_patch` byte-write or unlink is denied with a permission error (`EPERM`/`EACCES`/`EROFS`) — for hosts that embed the agent inside a sandbox that denies direct filesystem access but exposes a privileged channel. The brokered path is symlink-resolved so a handler's allowlist sees the real destination, a destination that cannot be resolved is not brokered at all, and `req.sessionId` names the session that issued the mutation so a handler sharing the process-wide registry can enforce policy per session. See [`docs/extensions.md`](../../docs/extensions.md).
### Fixed
- Fixed `omp stats` and `/stats` dashboards being unreachable from container hosts by accepting an explicit `--host` bind address while preserving the `127.0.0.1` default.
@@ -25,6 +25,7 @@ import { FileChangeType, notifyWorkspaceWatchedFiles } from "../../lsp/client";
import type { ToolSession } from "../../tools";
import { routeWriteThroughBridge } from "../../tools/acp-bridge";
import { assertEditableFileContent } from "../../tools/auto-generated-guard";
import { deleteFileWithFallback, writeFileWithFallback } from "../../tools/file-write-fallback";
import { invalidateFsScanAfterWrite } from "../../tools/fs-cache-invalidation";
import { isInternalUrlPath } from "../../tools/path-utils";
import { enforcePlanModeWrite, resolvePlanPath, targetsLocalSandbox } from "../../tools/plan-mode-guard";
@@ -153,7 +154,7 @@ export class HashlineFilesystem extends Filesystem {
enforcePlanModeWrite(this.session, relativePath, { op: "delete" });
const absolutePath = this.resolveAbsolute(relativePath);
try {
await fs.rm(absolutePath);
await deleteFileWithFallback(absolutePath);
} catch (error) {
if (isEnoent(error)) throw new NotFoundError(relativePath, error);
throw error;
@@ -173,8 +174,13 @@ export class HashlineFilesystem extends Filesystem {
const fromAbsolute = this.resolveAbsolute(fromRelative);
const toAbsolute = this.resolveAbsolute(toRelative);
if (content !== undefined) {
await Bun.write(toAbsolute, content);
await fs.rm(fromAbsolute);
// The one `edit` write that does not pass through the writethrough, so it
// routes to the fallback seam directly. `patcher.ts` always supplies
// `content` for a hashline `MV`, making this the live branch. The source
// unlink is a separate primitive with its own seam, so a move out of the
// workspace and a move out of denied territory both complete.
await writeFileWithFallback(toAbsolute, content);
await deleteFileWithFallback(fromAbsolute);
} else {
await fs.rename(fromAbsolute, toAbsolute);
}
+31 -5
View File
@@ -20,6 +20,12 @@ import { FileChangeType, notifyWorkspaceWatchedFiles } from "../../lsp/client";
import type { ToolSession } from "../../tools";
import { routeWriteThroughBridge } from "../../tools/acp-bridge";
import { assertEditableFile } from "../../tools/auto-generated-guard";
import {
deleteFileWithFallback,
hasFileWriteFallback,
isPermissionDeniedError,
writeFileWithFallback,
} from "../../tools/file-write-fallback";
import {
invalidateFsScanAfterDelete,
invalidateFsScanAfterRename,
@@ -111,6 +117,26 @@ export interface ApplyPatchOptions {
// Default File System
// ═══════════════════════════════════════════════════════════════════════════
/**
* Create a patch target's parent directory, tolerating a permission denial when a
* file-write fallback is registered.
*
* `apply_patch` mkdirs the parent before writing, so under a sandbox that denies
* the out-of-tree path this throws before the write — and therefore before
* {@link writeFileWithFallback} — is ever reached, leaving the fallback unable to
* broker a `create` or a rename-move into a new directory. Swallowing only a
* permission denial, and only with a handler installed, hands control to the write,
* which reports the denial through the seam. Without a handler the error propagates
* exactly as before.
*/
async function mkdirAllowingFallback(dir: string): Promise<void> {
try {
await fs.promises.mkdir(dir, { recursive: true });
} catch (error) {
if (!hasFileWriteFallback() || !isPermissionDeniedError(error)) throw error;
}
}
/** Default filesystem implementation using Bun APIs */
export const defaultFileSystem: FileSystem = {
async exists(path: string): Promise<boolean> {
@@ -123,13 +149,13 @@ export const defaultFileSystem: FileSystem = {
return fs.promises.readFile(path);
},
async write(path: string, content: string): Promise<void> {
await Bun.write(path, await serializeEditFileText(path, path, content));
await writeFileWithFallback(path, await serializeEditFileText(path, path, content));
},
async delete(path: string): Promise<void> {
await fs.promises.unlink(path);
await deleteFileWithFallback(path);
},
async mkdir(path: string): Promise<void> {
await fs.promises.mkdir(path, { recursive: true });
await mkdirAllowingFallback(path);
},
};
@@ -1753,7 +1779,7 @@ class LspFileSystem implements FileSystem {
}
async delete(path: string): Promise<void> {
await this.#getFile(path).unlink();
await deleteFileWithFallback(path, this.#getFile(path));
if (this.session.enableLsp ?? true) {
await notifyWorkspaceWatchedFiles(
this.session.cwd,
@@ -1764,7 +1790,7 @@ class LspFileSystem implements FileSystem {
}
async mkdir(path: string): Promise<void> {
await fs.promises.mkdir(path, { recursive: true });
await mkdirAllowingFallback(path);
}
getDiagnostics(): FileDiagnosticsResult | undefined {
@@ -28,6 +28,7 @@ import { execCommand } from "../../exec/exec";
// Runtime self-reference: dereference this namespace only inside loader functions to keep the index.ts cycle safe.
import * as PiCodingAgent from "../../index";
import type { CustomMessagePayload } from "../../session/messages";
import type { FileDeleteFallbackHandler, FileWriteFallbackHandler } from "../../tools/file-write-fallback";
import { EventBus } from "../../utils/event-bus";
import * as TypeBox from "../legacy-typebox";
import { installLegacyPiSpecifierShim, loadLegacyPiModule } from "../plugins/legacy-pi-compat";
@@ -183,6 +184,14 @@ class ConcreteExtensionAPI implements ExtensionAPI, IExtensionRuntime {
for (const listener of this.extension.toolRegistrationListeners ?? []) listener(tool.name);
}
registerFileWriteFallback(handler: FileWriteFallbackHandler): void {
this.extension.fileWriteFallbackHandlers.push(handler);
}
registerFileDeleteFallback(handler: FileDeleteFallbackHandler): void {
this.extension.fileDeleteFallbackHandlers.push(handler);
}
registerCommand(
name: string,
options: {
@@ -320,6 +329,8 @@ function createExtension(extensionPath: string, resolvedPath: string): Extension
tools: new Map(),
toolRegistrationListeners: new Set(),
assistantThinkingRenderers: [],
fileWriteFallbackHandlers: [],
fileDeleteFallbackHandlers: [],
messageRenderers: new Map(),
commands: new Map(),
flags: new Map(),
@@ -19,6 +19,7 @@ import type { MemoryRuntimeContext } from "../../memory-backend";
import { type Theme, theme } from "../../modes/theme/theme";
import type { AsyncJobSnapshot } from "../../session/agent-session";
import type { SessionManager } from "../../session/session-manager";
import { addFileDeleteFallback, addFileWriteFallback } from "../../tools/file-write-fallback";
import type { BranchHandler, NavigateTreeHandler, NewSessionHandler } from "../session-handler-types";
import { ManagedTimers } from "./managed-timers";
import { createExtensionModelQuery } from "./model-api";
@@ -375,10 +376,12 @@ export type SwitchSessionHandler = (sessionPath: string) => Promise<{ cancelled:
export type ShutdownHandler = () => void;
/**
* Emit `session_shutdown` and clear timers owned by an extension runner.
* Emit `session_shutdown`, dispose file-write-fallback registrations, and clear
* timers owned by an extension runner.
*
* Returns whether any shutdown handlers were present. Timer cleanup runs even
* when a handler fails so extension background work cannot outlive its host.
* Returns whether any shutdown handlers were present. Fallback disposal and timer
* cleanup run even when a handler fails so extension background work — and a
* fallback bound to this session's context — cannot outlive its host.
*/
export async function emitSessionShutdownEvent(extensionRunner: ExtensionRunner | undefined): Promise<boolean> {
if (!extensionRunner) return false;
@@ -389,6 +392,7 @@ export async function emitSessionShutdownEvent(extensionRunner: ExtensionRunner
});
return true;
} finally {
extensionRunner.disposeFileFallbacks();
extensionRunner.clearManagedTimers();
}
}
@@ -486,6 +490,23 @@ export class ExtensionRunner {
#managedTimers = new ManagedTimers((event, error, stack) =>
this.emitError({ extensionPath: "<timer>", event, error, stack }),
);
/**
* Disposers for the trampolines installed via {@link addFileWriteFallback} and
* {@link addFileDeleteFallback} — one per extension per seam it registered for.
* Installed during {@link initialize} (after the UI/runtime context is live, so
* the bound handler sees a working `ctx.ui`) and drained by
* {@link disposeFileFallbacks} on session shutdown so a handler from a
* torn-down session can never fire for a later one sharing the same process.
*
* Each trampoline re-reads its extension's handler list at call time rather than
* closing over a snapshot, matching how `ext.handlers` is re-read on every emit,
* so an extension that already had a handler for that seam at `initialize` picks
* up later additions to it. A seam the extension registered NOTHING for gets no
* trampoline at all, which keeps the registry empty for a host with no fallbacks;
* the cost is that a first registration for that seam after `initialize` never
* takes effect, which is why the API documents load-time registration.
*/
#fileFallbackDisposers: Array<() => void> = [];
/**
* Dedup markers for `tool_call` emission, keyed `${toolCallId}:${toolName}`.
* The agent loop emits `tool_call` at arg-prep time (before scheduling and
@@ -609,6 +630,18 @@ export class ExtensionRunner {
return this.sessionManager.getCwd();
}
/**
* Stable id of the session this runner serves. Read through `sessionManager`
* for the same reason as {@link cwd}: it is this session's own, never a
* process-global, so a subagent runner reports itself and not its parent.
*
* Used to attribute a denied file write or delete to the session that issued
* it, since the fallback registry those handlers live in is process-wide.
*/
get sessionId(): string {
return this.sessionManager.getSessionId();
}
initialize(
actions: ExtensionActions,
contextActions: ExtensionContextActions,
@@ -666,6 +699,75 @@ export class ExtensionRunner {
this.#mode = mode;
this.#initialized = true;
// Re-initialize (e.g. a mode switch rewiring UI/runtime actions) must not
// accumulate duplicate global registrations — drop the prior generation before
// installing this one's trampolines.
this.disposeFileFallbacks();
for (const ext of this.extensions) {
// Nothing registered by this extension means no trampoline, so a host with
// no fallback-registering extension leaves the seam genuinely empty and
// `hasFileWriteFallback()`/`hasFileDeleteFallback()` false — the invariant
// the whole feature rests on. Each seam is checked separately, so an
// extension that only brokers writes never appears in the delete registry.
if (ext.fileWriteFallbackHandlers.length === 0 && ext.fileDeleteFallbackHandlers.length === 0) continue;
// One trampoline per extension per seam, not per handler: the list is walked
// at mutation time so a handler this extension adds later still takes effect,
// and `createContext()` takes no extension argument, so within one invocation
// a single context is all any of this extension's handlers would have
// received anyway.
//
// The context is built PER INVOCATION rather than captured here, matching
// every other dispatch site. `createContext()` materializes `cwd` and
// `hasUI` as values, so a trampoline holding one context for the life of the
// session would keep handing handlers the workspace this runner initialized
// in — wrong the moment `SessionManager.moveTo()` relocates the session
// (`/move`), and a handler that scopes or prompts against `ctx.cwd` would
// then allow the old workspace and deny the new one. A denied mutation is a
// rare path, so the extra object costs nothing that matters.
//
// Isolation is per HANDLER, not per extension. The registry only sees one
// trampoline per extension, so a throw escaping this loop would advance the
// registry to the NEXT extension and skip every later handler this one
// registered — breaking both the documented "a throwing handler is skipped"
// contract and registration order for a backup-handler setup.
if (ext.fileWriteFallbackHandlers.length > 0) {
this.#fileFallbackDisposers.push(
addFileWriteFallback(async req => {
const ctx = this.createContext();
for (const handler of ext.fileWriteFallbackHandlers) {
try {
if (await handler(req, ctx)) return true;
} catch (error) {
logger.warn("Extension file write fallback handler threw; trying next handler", {
extension: ext.path,
error: error instanceof Error ? error.message : String(error),
});
}
}
return false;
}),
);
}
if (ext.fileDeleteFallbackHandlers.length > 0) {
this.#fileFallbackDisposers.push(
addFileDeleteFallback(async req => {
const ctx = this.createContext();
for (const handler of ext.fileDeleteFallbackHandlers) {
try {
if (await handler(req, ctx)) return true;
} catch (error) {
logger.warn("Extension file delete fallback handler threw; trying next handler", {
extension: ext.path,
error: error instanceof Error ? error.message : String(error),
});
}
}
return false;
}),
);
}
}
// Drain events buffered by emitCredentialDisabled() before initialize ran. The
// spread adds the `type` discriminator — `event` is the pi-ai shape (no `type`).
// Deferred by one microtask so callers that register an onError listener
@@ -1098,6 +1200,16 @@ export class ExtensionRunner {
this.#managedTimers.clearAll();
}
/**
* Remove every file write and delete fallback this runner installed into the
* process-wide registries. Called on session shutdown (and before reinstalling
* on a re-{@link initialize}) so a handler bound to a torn-down session's
* context can never fire for another session sharing this process.
*/
disposeFileFallbacks(): void {
for (const dispose of this.#fileFallbackDisposers.splice(0)) dispose();
}
createCommandContext(): ExtensionCommandContext {
return {
...this.createContext(),
@@ -76,6 +76,7 @@ import type {
WriteToolInput,
} from "../../tools";
import type { ApprovalMode } from "../../tools/approval";
import type { FileDeleteFallbackHandler, FileWriteFallbackHandler } from "../../tools/file-write-fallback";
import type { EventBus } from "../../utils/event-bus";
import type {
AgentEndEvent,
@@ -1256,6 +1257,63 @@ export interface ExtensionAPI {
/** Register a tool that the LLM can call. */
registerTool<TParams extends TSchema = TSchema, TDetails = unknown>(tool: ToolDefinition<TParams, TDetails>): void;
/**
* Register a fallback writer consulted when a native `write`/`edit` byte-write is
* denied with a permission error (`EPERM`/`EACCES`/`EROFS`). Every other write
* error is unaffected. Handlers run in registration order; the first one to
* resolve `true` counts as the bytes being durably on disk, and the native tool
* continues as if its own write had succeeded — including recording its file
* snapshot under the real destination path, so a later hashline `edit` on that
* path keeps working. Intended for a host embedding the agent inside a sandbox
* that denies direct filesystem writes but exposes a privileged write channel.
*
* A denial that `Bun.write` masks as `ENOENT` — a write into a directory the host
* may not create — also diverts here, with `req.dst`'s parent absent and the
* handler responsible for creating it.
*
* `req.dst` is symlink-RESOLVED: the path the failed write itself acted on, not
* the one the tool was given. A link anywhere in a lexical path redirects the
* bytes while still passing a prefix allowlist, so treat `req.dst` as
* authoritative. A destination that cannot be resolved is never brokered.
*
* Call this during extension load, like the other `register*` methods: handlers
* are installed when the runner initializes, so an extension that has registered
* none by then is skipped and a first registration made later never takes effect.
*
* The underlying registry is process-wide, so a handler may be consulted for a
* denied write from any session in the process, not only its own.
* `req.sessionId` names the session that issued the write and
* `ctx.sessionManager.getSessionId()` names the handler's own; compare them
* before prompting, because `ctx.ui` belongs to the latter. See
* `docs/extensions.md`.
*/
registerFileWriteFallback(handler: FileWriteFallbackHandler): void;
/**
* Register a fallback deleter consulted when a native `edit`/`apply_patch` unlink is
* denied with a permission error (`EPERM`/`EACCES`/`EROFS`). Covers `edit`'s `REM`,
* the source side of a hashline `MV`, and `apply_patch`'s delete op. Return `true`
* once `dst` is gone from disk.
*
* A handler MUST remove `dst` with a plain unlink and MUST NOT fall back to a
* recursive removal. `unlink` on a directory reports `EPERM` on Darwin, so the seam
* checks the target before diverting — but when the target's own metadata is behind
* the same boundary that denied the unlink, which is the common sandbox case, that
* check cannot be resolved and `dst` may be a directory. `req.confirmedFile` says
* which situation the handler is in.
*
* `req.dst` resolves every component ABOVE the last, for the same reason the
* write seam resolves all of them; the last is left alone because `unlink`
* removes a link rather than its target, so `req.dst` may name a link.
*
* Separate from {@link registerFileWriteFallback} on purpose. A write handler
* brokers `req.content` to `req.dst`, so a delete request reaching it with no
* content invites brokering an empty write and truncating the file instead of
* removing it. Registering for deletes is therefore an explicit opt-in, and the
* same load-time and process-wide notes above apply.
*/
registerFileDeleteFallback(handler: FileDeleteFallbackHandler): void;
// =========================================================================
// Command, Shortcut, Flag Registration
// =========================================================================
@@ -1632,6 +1690,8 @@ export interface Extension {
tools: Map<string, RegisteredTool<any, any>>;
toolRegistrationListeners?: Set<ToolRegistrationListener>;
assistantThinkingRenderers: AssistantThinkingRenderer[];
fileWriteFallbackHandlers: FileWriteFallbackHandler[];
fileDeleteFallbackHandlers: FileDeleteFallbackHandler[];
messageRenderers: Map<string, MessageRenderer>;
commands: Map<string, RegisteredCommand>;
flags: Map<string, ExtensionFlag>;
@@ -14,6 +14,7 @@ import type { Settings } from "../../config/settings";
import type { Theme } from "../../modes/theme/theme";
import { type ApprovalMode, formatApprovalPrompt, resolveApproval, truncateForPrompt } from "../../tools/approval";
import { defaultLoadModeForToolName } from "../../tools/essential-tools";
import { withFileMutationSession } from "../../tools/file-write-fallback";
import { normalizeToolEventInput, resolveToolEventInput } from "../tool-event-input";
import { applyToolProxy } from "../tool-proxy";
import type { ExtensionRunner } from "./runner";
@@ -349,7 +350,15 @@ export class ExtensionToolWrapper<TParameters extends TSchema = TSchema, TDetail
let executionError: Error | undefined;
try {
result = await this.tool.execute(toolCallId, effectiveParams, signal, onUpdate, context);
// A denied file write or delete inside this tool can be brokered to an
// extension handler, and that registry is PROCESS-WIDE — so the session is
// named here, the one place where every tool's execution and the runner
// that owns the handlers are both in scope (`sdk.ts` wraps the whole tool
// registry with this class whenever a runner exists). Inert with no
// fallback registered: no scope is entered.
result = await withFileMutationSession(this.runner.sessionId, () =>
this.tool.execute(toolCallId, effectiveParams, signal, onUpdate, context),
);
} catch (err) {
executionError = err instanceof Error ? err : new Error(String(err));
result = {
+20 -10
View File
@@ -1,6 +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 { isPermissionDeniedError, writeFileWithFallback } from "../tools/file-write-fallback";
import { FileChangeType, notifyWorkspaceWatchedFiles } from "./client";
import { getServersForFile } from "./config";
import {
@@ -66,11 +67,7 @@ export async function writethroughNoop(
_batch?: LspWritethroughBatchRequest,
_getDeferred?: (dst: string) => WritethroughDeferredHandle | undefined,
): Promise<FileDiagnosticsResult | undefined> {
if (file) {
await file.write(content);
} else {
await Bun.write(dst, content);
}
await writeFileWithFallback(dst, content, file);
return undefined;
}
@@ -78,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 {
@@ -288,7 +291,7 @@ async function runLspWritethrough(
const contentAlreadyWritten = runOptions?.contentAlreadyWritten ?? false;
let finalContent = content;
const writeContent = async (value: string) => (file ? file.write(value) : Bun.write(dst, value));
const writeContent = async (value: string) => writeFileWithFallback(dst, value, file);
const getWritePromise = once(() =>
contentAlreadyWritten && finalContent === content ? Promise.resolve() : writeContent(finalContent),
);
@@ -458,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 &&
@@ -552,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);
@@ -0,0 +1,467 @@
/**
* In-session fallbacks for permission-denied file writes and deletes.
*
* A host that embeds the agent inside an OS sandbox can grant a path mid-session
* but cannot apply that grant to an in-process write, because the write happens
* in the agent process under a profile fixed at launch. This module gives such a
* host a seam to intercept a denied mutation and perform it through a privileged
* channel, without reimplementing `write`/`edit` semantics: the native tool still
* records its own snapshot under the real destination path once the fallback
* reports success, so a follow-up hashline `edit` on that path keeps working.
*
* Writes and deletes have SEPARATE registries. 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. Opting into deletes
* is therefore explicit; see {@link addFileDeleteFallback}.
*
* ## What is routed
*
* The byte-write that `write`, `edit` and `apply_patch` perform on an ordinary
* file path goes through the same two-line primitive
* (`file ? file.write(content) : Bun.write(dst, content)`). It has four call
* sites, and all of them route here:
*
* - `writethroughNoop` and `runLspWritethrough`'s `writeContent` (`lsp/writethrough.ts`),
* the `WritethroughCallback` that `write` and `edit` both write through.
* `apply_patch` reaches it too: `LspFileSystem.write` (`edit/modes/patch.ts`),
* which it always injects, delegates to the same callback.
* - `HashlineFilesystem.move` (`edit/hashline/filesystem.ts`) — a hashline `MV`
* destination, the one `edit` write that does not pass through the writethrough.
* - `defaultFileSystem.write` (`edit/modes/patch.ts`), only the default parameter
* for external `applyPatch` callers and tests.
*
* `apply_patch` also creates a missing parent directory before writing, via its
* filesystem's `mkdir`. That `mkdir` consults {@link hasFileWriteFallback} so a
* denial there falls through to the write and reaches a handler, instead of
* throwing before the seam is ever consulted.
*
* The unlink that `edit` and `apply_patch` perform routes to the separate delete
* seam ({@link deleteFileWithFallback}) at four sites: `HashlineFilesystem.delete`
* (`edit`'s `REM`) and `HashlineFilesystem.move`'s source unlink, plus
* `LspFileSystem.delete` and `defaultFileSystem.delete` for `apply_patch`.
*
* ## What is NOT routed
*
* This is deliberately not an exhaustive interception of every syscall the tools
* can make. A permission error from any of these surfaces as it does today:
*
* - `write` to an archive member (`foo.zip:entry`) or a SQLite row. Neither is a
* byte-write to `dst`: an archive member rewrite reads the whole archive, sets
* one entry, writes a temp file and renames over the original, so the bytes on
* disk are a whole binary container rather than the string the tool was given;
* a SQLite write is a row operation inside the database engine with no byte
* payload at all. Brokering either needs a different request shape than
* "these exact bytes belong at this path".
* - `acp-bridge.ts`'s `bridge.writeTextFile` — a remote-client transport.
* - Removing a DIRECTORY is never the intent: the delete seam refuses to divert a
* target it can confirm is one, and reports `confirmedFile: false` when the
* target's metadata is behind the same boundary and the check cannot be resolved.
* - The `lsp` tool's own writes: applying a workspace edit or a code action
* (`lsp/edits.ts`), and the Biome formatter, which writes the buffer and then
* shells out to `biome format --write` (`lsp/clients/biome-client.ts`) — a
* subprocess write no in-process seam can reach anyway.
*
* ## Diverting
*
* Only a permission boundary diverts — `EPERM`, `EACCES`, `EROFS`, plus the one
* case where Bun hides such a denial behind an `ENOENT` (see
* {@link classifyWriteFailure}). Every other error rethrows untouched.
*
* With no handler registered this module is inert: the primitive runs exactly as
* it did before, a failure rethrows from the same place, and no extra syscalls
* are performed.
*
* ## The path a handler is given
*
* A handler is more privileged than the syscall that just failed, so it is never
* handed the lexical path the tool used. A lexical path is not a destination: the
* kernel follows every component above the last, so `ws/link/file` under a
* `ws/link -> /elsewhere` link lands outside `ws` while still looking
* in-workspace. That defeats the defence a helper author reaches for first, since
* a prefix allowlist passes on the link's own path — and for writes the final
* component is followed too, so a plain `ws/link` is enough.
*
* `req.dst` is therefore resolved through {@link resolveSyscallTarget} to the path
* the failed syscall itself acted on: fully for a write, and up to the last
* component for a delete, since `unlink` removes a link rather than following it.
* Resolving rather than refusing also closes the TOCTOU window, because the
* handler no longer traverses a link the agent could re-point after the check.
*
* A path that cannot be canonicalized — a dangling final link, or an ancestor
* whose own resolution is denied — is not brokered at all. "Where would this
* land" has no answer there, and a privileged writer is the wrong place to guess.
* That narrows the seam for a sandbox that also hides the ancestors of a denied
* path, which is the honest cost of not handing over an unverifiable target.
*
* That refusal is load-bearing for more than symlink safety, and relaxing it needs
* care. `apply_patch`'s `create` and rename-destination refuse to overwrite, and
* they decide that with `Bun.file(dst).exists()`, which reports `false` when the
* parent hides the target's metadata rather than distinguishing "absent" from
* "unknown". The non-overwrite contract holds today only because the same denied
* `lstat` that fools that check also stops this seam from brokering — a privileged
* writer, the one party that could enforce exclusivity itself, is never handed the
* path. Broker an unverifiable destination and a `create` starts clobbering a
* protected file it was told not to touch; a request field carrying explicit
* exclusive-create intent would be the prerequisite for that change.
*
* ## Scope of the registry
*
* Handlers live in one process-wide list, and a process can host several sessions
* (a subagent gets its own `ExtensionRunner`). A handler is therefore consulted
* for denied mutations from ANY session in the process, not only the one whose
* extension registered it. Filtering by session here would be wrong: a subagent
* spawned with `restrictToolNames` loads no extensions of its own, so scoping
* would leave its denied writes with nothing to broker them, and a host that
* registers once in its top-level session expects subagent writes covered.
*
* So the request names its origin instead, and the policy stays with the party
* that owns it. `req.sessionId` is the session that issued the mutation (see
* {@link withFileMutationSession}); a handler compares it with
* `ctx.sessionManager.getSessionId()` to decide. That matters most for a handler
* that prompts: `ctx.ui` belongs to the session whose extension registered the
* handler, which is not necessarily the session being asked about.
*
* Each list is iterated over a snapshot, because a concurrent session shutdown
* splices the live array and a `for` over it would skip whichever handler shifted
* into the hole.
*/
import { AsyncLocalStorage } from "node:async_hooks";
import * as fs from "node:fs/promises";
import * as path from "node:path";
import { isEnoent, isFsError, logger } from "@oh-my-pi/pi-utils";
import type { BunFile } from "bun";
import type { ExtensionContext } from "../extensibility/extensions/types";
import { resolveSyscallTarget } from "./path-utils";
/** A denied write, captured for a registered fallback to retry through a privileged channel. */
export interface FileWriteFallbackRequest {
/**
* Absolute, symlink-resolved path to write the bytes to.
*
* This is where the failed in-process write would itself have landed, which is
* not necessarily the path the tool was given: `open` follows every component,
* so a link anywhere in that path redirects the bytes. Resolving it here is
* what lets a handler's allowlist see the real destination instead of a
* lexically innocent path, so a handler MUST treat this as authoritative and
* MUST NOT re-derive the target from anything else.
*/
dst: string;
/**
* Session the denied write was issued from, or `undefined` when the mutation
* did not happen inside a tool call (an external `applyPatch` caller, a test).
*
* The registry is process-wide, so a handler can be consulted for a write from
* a session other than the one whose extension registered it. Compare this with
* `ctx.sessionManager.getSessionId()` to tell the two apart — a handler that
* prompts through `ctx.ui` needs to, since that UI belongs to ITS session and
* not necessarily to the one being asked about.
*/
sessionId: string | undefined;
/** The exact bytes the tool intended to write. */
content: string;
/**
* The error that proves the write hit a permission boundary. Usually the write's
* own `EPERM`/`EACCES`/`EROFS`; for a write into a directory the host may not
* create, the denial raised by creating that directory, in which case `dst`'s
* parent may not exist yet and the handler is responsible for creating it.
*/
cause: unknown;
}
/** Extension-authored handler. Return `true` once `content` is durably on disk at `dst`. */
export type FileWriteFallbackHandler = (req: FileWriteFallbackRequest, ctx: ExtensionContext) => Promise<boolean>;
/** A handler already bound to its owning extension's live context. */
type BoundFileWriteFallbackHandler = (req: FileWriteFallbackRequest) => Promise<boolean>;
/** A denied unlink, captured for a registered fallback to perform through a privileged channel. */
export interface FileDeleteFallbackRequest {
/**
* Absolute, symlink-resolved path the unlink was denied for.
*
* Every component ABOVE the last is resolved, so a handler cannot be walked
* outside its allowed roots through a link in the path. The last component is
* deliberately NOT resolved, because `unlink` removes a link itself rather
* than its target — which is also why this may still name a symlink.
*/
dst: string;
/** The `EPERM`/`EACCES`/`EROFS` that proves the unlink hit a permission boundary. */
cause: unknown;
/**
* Whether `dst` was confirmed to be a plain regular file before diverting.
*
* `false` means the seam could not establish that, either because the target's
* own metadata is behind the same boundary that denied the unlink — the common
* sandbox case, since `unlink` on a directory also reports `EPERM` on Darwin —
* or because `dst` is a symlink.
*
* A handler MUST remove `dst` with a plain unlink. It MUST NOT remove it
* recursively, and MUST NOT resolve the path first: when this is `false` the
* target may be a directory, and resolving a symlink would delete whatever it
* points at instead of the link.
*/
confirmedFile: boolean;
/** See {@link FileWriteFallbackRequest.sessionId}. */
sessionId: string | undefined;
}
/** Extension-authored handler. Return `true` once `dst` is gone from disk. */
export type FileDeleteFallbackHandler = (req: FileDeleteFallbackRequest, ctx: ExtensionContext) => Promise<boolean>;
/** A handler already bound to its owning extension's live context. */
type BoundFileDeleteFallbackHandler = (req: FileDeleteFallbackRequest) => Promise<boolean>;
const PERMISSION_DENIED_CODES: Record<string, true> = { EPERM: true, EACCES: true, EROFS: true };
const PERMISSION_DENIED_MESSAGE = /\b(EPERM|EACCES|EROFS)\b/;
/** True for `EPERM`, `EACCES`, and `EROFS` — the sandbox-boundary write failures this seam exists for. */
export function isPermissionDeniedError(error: unknown): boolean {
// A structured `code` is authoritative. Checking the message as well would
// misclassify any error whose path contains one of these names, and Bun embeds
// the full path in its fs error messages (`ENOENT: ..., open '/x/EACCES/y'`).
if (isFsError(error)) return PERMISSION_DENIED_CODES[error.code] === true;
// Some write paths (e.g. a bridged transport) surface the denial as a plain
// Error with no structured `code`, leaving only the message to go on.
return error instanceof Error && PERMISSION_DENIED_MESSAGE.test(error.message);
}
const fallbackHandlers: BoundFileWriteFallbackHandler[] = [];
/** Whether any fallback is registered. Lets a caller skip work that only this seam needs. */
export function hasFileWriteFallback(): boolean {
return fallbackHandlers.length > 0;
}
/**
* Append a fallback writer, consulted in registration order when a direct write is
* permission-denied. Returns a disposer that removes this exact registration; the
* runner calls it on session shutdown so no handler outlives its session.
*/
export function addFileWriteFallback(handler: BoundFileWriteFallbackHandler): () => void {
fallbackHandlers.push(handler);
return () => {
const index = fallbackHandlers.indexOf(handler);
if (index !== -1) fallbackHandlers.splice(index, 1);
};
}
const deleteFallbackHandlers: BoundFileDeleteFallbackHandler[] = [];
/** Whether any delete fallback is registered. */
export function hasFileDeleteFallback(): boolean {
return deleteFallbackHandlers.length > 0;
}
/**
* Append a fallback deleter, consulted in registration order when a direct unlink is
* permission-denied. Deliberately a separate registry from
* {@link addFileWriteFallback}: a write handler brokers `content` to `dst`, and
* handing it a request with no content would let it "broker" an empty write and
* truncate the file it was asked to remove. Opting in is explicit for that reason.
*/
export function addFileDeleteFallback(handler: BoundFileDeleteFallbackHandler): () => void {
deleteFallbackHandlers.push(handler);
return () => {
const index = deleteFallbackHandlers.indexOf(handler);
if (index !== -1) deleteFallbackHandlers.splice(index, 1);
};
}
const mutationSessionStorage = new AsyncLocalStorage<string>();
/**
* Name the session whose tool call is about to run, so a denied mutation inside it
* can tell a handler where the request came from.
*
* Entered once per tool call by `ExtensionToolWrapper` (`extensibility/extensions/
* wrapper.ts`), which `sdk.ts` puts around the whole tool registry whenever an
* `ExtensionRunner` exists — so the component that owns the handlers is the one
* naming its own session, and no caller has to thread an `AgentToolContext`
* through for attribution to work.
*
* That covers the deferred LSP write batch too: a batch id belongs to one
* assistant turn of one session, and its flush is awaited inside a tool call of
* that same session, so a write performed during a later call of the group is
* still attributed to the session that issued it.
*
* Deliberately NOT a general "current session" accessor: nothing else enters this
* scope, so outside a tool call it is empty by design — an external `applyPatch`
* caller reports `undefined` rather than borrowing someone else's identity.
*/
export function withFileMutationSession<T>(sessionId: string | undefined, fn: () => T): T {
// With nothing registered no scope is entered, keeping the seam's inertness
// promise: a stock host pays one length check per tool call and no more.
if (sessionId === undefined || (fallbackHandlers.length === 0 && deleteFallbackHandlers.length === 0)) return fn();
return mutationSessionStorage.run(sessionId, fn);
}
/**
* Remove a file, consulting registered delete fallbacks when the unlink is denied.
*
* Unlike the write path there is no masked-`ENOENT` case to see through: nothing is
* created on the way, so an `ENOENT` here means the file genuinely is not there and
* must propagate — `edit`'s `REM` turns it into a `NotFoundError`.
*/
export async function deleteFileWithFallback(dst: string, file?: BunFile): Promise<void> {
try {
if (file) {
await file.unlink();
} else {
await fs.unlink(dst);
}
} catch (error) {
if (deleteFallbackHandlers.length === 0 || !isPermissionDeniedError(error)) throw error;
// A handler is more privileged than the unlink that just failed, so it is told
// which path really gets removed, not the lexical one the tool used. `unlink`
// follows every component ABOVE the last, so `ws/link/victim` under a
// `ws/link -> /elsewhere` link removes a file outside `ws` while a helper's
// prefix allowlist still passes. The final component is deliberately left
// unresolved: `unlink` removes the link itself, never its target.
const target = await resolveSyscallTarget(dst, false);
if (target === null) throw error;
// `unlink` on a directory reports EPERM on Darwin (EISDIR on Linux), which is
// indistinguishable from a sandbox denial by code alone, so check the target
// before diverting: asking a privileged deleter to remove a DIRECTORY on
// behalf of a tool that only ever removes one file would far exceed the
// intent. `lstat` rather than `stat`, so the link itself is judged — removing
// a symlink is a legitimate file removal, and following it here would ask the
// wrong question.
const stat = await fs.lstat(target).catch((statError: unknown) => {
// A sandbox that denies the unlink usually denies the target's metadata
// too, so a denied `lstat` is expected here and must still divert — it
// just leaves the question unresolved, which `confirmedFile` reports.
// Any OTHER `lstat` failure is not something this seam should paper over.
if (isPermissionDeniedError(statError)) return null;
throw error;
});
if (stat?.isDirectory()) throw error;
// A symlink is safe to unlink but NOT safe to resolve: a helper that
// realpaths `dst` for auditing, or removes it recursively, would act on the
// link's target instead. Only a plain regular file is a confirmed file.
const confirmedFile = stat?.isFile() ?? false;
// The process-wide registry can hand this to a handler from another session,
// so the request names the one that issued it.
const sessionId = mutationSessionStorage.getStore();
// Snapshot: a concurrent session shutdown splices the live array, and
// iterating it directly would skip whichever handler shifted into the hole.
for (const handler of [...deleteFallbackHandlers]) {
try {
if (await handler({ dst: target, cause: error, confirmedFile, sessionId })) return;
} catch (handlerError) {
logger.warn("File delete fallback handler threw; trying next handler", {
dst: target,
error: handlerError instanceof Error ? handlerError.message : String(handlerError),
});
}
}
// Always the ORIGINAL error, never a handler's, so behaviour matches a host
// with no fallback registered.
throw error;
}
}
/**
* Outcome of inspecting a failed primitive write. `denied` diverts to the
* registered handlers, `retry` repeats the write because this call repaired the
* cause, and `rethrow` leaves the original error alone.
*/
type WriteFailureKind = { kind: "denied"; cause: unknown } | { kind: "retry" } | { kind: "rethrow" };
/**
* Decide whether a failed write hit a permission boundary.
*
* `Bun.write` and `BunFile.write` create missing parent directories themselves,
* but when that `mkdir` is the thing being denied they report the subsequent
* `open()`'s `ENOENT` rather than the denial — so a sandboxed write into a new
* out-of-tree directory is indistinguishable from an ordinary missing path.
* Redoing the `mkdir` explicitly recovers the real errno, and because it runs
* through the same enforcement path as the write it sees kernel-level denials
* (Seatbelt, LSM) that a `stat`/`access` probe would report as writable.
*
* Only called with at least one handler registered, so a stock host never pays
* for this.
*/
async function classifyWriteFailure(dst: string, error: unknown): Promise<WriteFailureKind> {
if (isPermissionDeniedError(error)) return { kind: "denied", cause: error };
if (!isEnoent(error)) return { kind: "rethrow" };
try {
await fs.mkdir(path.dirname(dst), { recursive: true });
} catch (mkdirError) {
// A denied `mkdir` is the boundary the write hid; anything else (`ENOTDIR`
// for a file used as a directory, ...) is a genuine bad path.
if (isPermissionDeniedError(mkdirError)) return { kind: "denied", cause: mkdirError };
return { kind: "rethrow" };
}
// The parent exists now, so the `ENOENT` was a lost race rather than a
// boundary. Any directory just created stays, matching what a permitted
// `Bun.write` would have left behind; removing it could race a concurrent
// writer that legitimately needs it.
return { kind: "retry" };
}
export async function writeFileWithFallback(dst: string, content: string, file?: BunFile): Promise<void> {
// Attempt 0 is the plain write. The single retry is reachable only when the
// first failure turned out to be a parent-directory race this call repaired,
// which bounds the loop at two writes.
for (let attempt = 0; ; attempt++) {
try {
if (file) {
await file.write(content);
} else {
await Bun.write(dst, content);
}
return;
} catch (error) {
if (fallbackHandlers.length === 0) throw error;
// On the second attempt a `retry` verdict can no longer change the
// outcome, so skip the probe and let the error stand unless it is a
// denial the handlers should see.
const failure =
attempt === 0
? await classifyWriteFailure(dst, error)
: isPermissionDeniedError(error)
? ({ kind: "denied", cause: error } as const)
: ({ kind: "rethrow" } as const);
if (failure.kind === "retry") continue;
if (failure.kind === "denied") {
// A handler is more privileged than the write that just failed, so it is
// told where the bytes would REALLY have landed rather than the lexical
// path the tool used. `open` follows EVERY component, so `ws/link/file`
// under a `ws/link -> /elsewhere` link writes outside `ws` while still
// looking in-workspace — which defeats the defence a helper author
// reaches for first, since a prefix allowlist passes on the link's own
// path. Resolving closes that, and closes the TOCTOU window with it: the
// helper no longer traverses a link the agent could re-point after the
// check. A path that cannot be canonicalized is not brokered at all,
// because "where would this land" then has no answer to hand over.
const target = await resolveSyscallTarget(dst, true);
// Snapshot: a concurrent session shutdown splices the live array, and
// iterating it directly would skip whichever handler shifted into the hole.
if (target !== null) {
// The process-wide registry can hand this to a handler from another
// session, so the request names the one that issued it.
const sessionId = mutationSessionStorage.getStore();
for (const handler of [...fallbackHandlers]) {
try {
if (await handler({ dst: target, content, cause: failure.cause, sessionId })) return;
} catch (handlerError) {
logger.warn("File write fallback handler threw; trying next handler", {
dst: target,
error: handlerError instanceof Error ? handlerError.message : String(handlerError),
});
}
}
}
}
// Always the ORIGINAL error, never a handler's, so behaviour matches a
// host with no fallback registered. When the real boundary was recovered
// from behind a masked `ENOENT`, attach it so the denial is not lost:
// without this the caller is told `ENOENT` for a path this code has
// already proven is `EACCES`.
if (failure.kind === "denied" && failure.cause !== error && error instanceof Error && error.cause == null) {
error.cause = failure.cause;
}
throw error;
}
}
}
+1
View File
@@ -86,6 +86,7 @@ export * from "./debug";
export * from "./essential-tools";
export * from "./eval";
export * from "./eval-backends";
export * from "./file-write-fallback";
export * from "./gh";
export * from "./glob";
export * from "./grep";
@@ -618,6 +618,85 @@ function isSymlink(target: string): boolean {
}
}
/**
* Resolve the path a syscall on `filePath` would really act on, or `null` when
* that cannot be established.
*
* A lexical path is not a destination. The kernel follows every component above
* the last, so `ws/link/file` under a `ws/link -> /elsewhere` link lands outside
* `ws` while still looking relative and `..`-free. Handing such a path to a
* privileged helper defeats the defence a helper author reaches for first — a
* prefix allowlist passes, because the link sits inside the allowed root while
* its target does not. Callers that hand a path to something more privileged
* than the syscall that just failed resolve it here first.
*
* Rejecting symlinked components outright is not an option: `/var` and `/tmp`
* are links on macOS, so every path under `os.tmpdir()` traverses one. They are
* resolved instead, and only a path whose real destination cannot be established
* is refused, because "where would this land" then has no answer to hand over.
* {@link confineToWorkspace} refuses an unresolvable link for the same reason.
*
* @param followFinal `true` for a syscall that follows a link at the final
* component (`open`, so every write), `false` for one that acts on the link
* itself (`unlink`) and therefore needs it left alone.
*/
export async function resolveSyscallTarget(filePath: string, followFinal: boolean): Promise<string | null> {
const target = path.resolve(filePath);
if (followFinal) {
const real = await tryRealpathAsync(target);
if (real !== null) return real;
// `realpath` also fails on a DANGLING link, which a write follows to a place
// this cannot name, and on a path whose ancestor may not be searched. Neither
// is proof the final component is a plain name, and only proof continues.
if (!(await isProvenNotSymlink(target))) return null;
}
// Walk up to the deepest ancestor that does resolve, then re-apply the
// components below it. A resolved ancestor vouches for the ones above it, so
// re-applying them lexically matches what the kernel would have done.
const tail: string[] = [path.basename(target)];
let ancestor = path.dirname(target);
for (;;) {
const real = await tryRealpathAsync(ancestor);
if (real !== null) return path.join(real, ...tail.reverse());
// This component is about to be re-applied lexically without a resolved
// ancestor vouching for it, which is exactly the escape being closed — so it
// has to prove itself. `realpath` fails here for a component that does not
// exist yet AND for one inside a directory the caller may not search (the
// usual shape when a sandbox hides a denied path), and the second still
// permits `lstat`.
if (!(await isProvenNotSymlink(ancestor))) return null;
const parent = path.dirname(ancestor);
// Ran past the filesystem root: `realpath("/")` cannot fail, so only a
// filesystem disappearing mid-walk gets here.
if (parent === ancestor) return null;
tail.push(path.basename(ancestor));
ancestor = parent;
}
}
async function tryRealpathAsync(target: string): Promise<string | null> {
try {
// `fs.promises.realpath` has no `.native` variant under Bun, unlike its sync
// counterpart; the JS implementation resolves links identically.
return await fs.promises.realpath(target);
} catch {
return null;
}
}
/**
* Whether `target` is known NOT to redirect. A path that does not exist cannot
* redirect anything, and nothing below it exists either; any other `lstat`
* failure leaves the question unanswered, which is not proof.
*/
async function isProvenNotSymlink(target: string): Promise<boolean> {
try {
return !(await fs.promises.lstat(target)).isSymbolicLink();
} catch (error) {
return isEnoent(error);
}
}
export function formatPathRelativeToCwd(
filePath: string,
cwd: string,
@@ -3739,6 +3739,8 @@ describe("ExtensionRunner", () => {
handlers: new Map([["input", [async (...args: unknown[]) => handler(args[0] as InputEvent)]]]),
tools: new Map(),
assistantThinkingRenderers: [],
fileWriteFallbackHandlers: [],
fileDeleteFallbackHandlers: [],
messageRenderers: new Map(),
commands: new Map(),
flags: new Map(),
@@ -1,7 +1,7 @@
import { describe, expect, spyOn, test } from "bun:test";
import * as path from "node:path";
import { RpcClient } from "@oh-my-pi/pi-coding-agent/modes/rpc/rpc-client";
import { ptree, TempDir } from "@oh-my-pi/pi-utils";
import { type ChildProcess, ptree, TempDir } from "@oh-my-pi/pi-utils";
const MOCK_AGENT = path.join(import.meta.dir, "fixtures", "mock-rpc-agent.ts");
@@ -243,4 +243,41 @@ describe("RpcClient lifecycle (issue #4079 B)", () => {
"Agent process exited with code 23. Stderr: fixture worker failed",
);
});
test("start() rejects instead of hanging when a pre-ready worker closes stdout and never exits", async () => {
// The worker outlives its own stdout, so start() cannot learn an exit code
// and must still fail: it waits a bounded time for the exit, then reports
// the stream end. A regression stalls until the 30s ready timeout, which
// this test's own timeout catches.
let resolveExit: ((exitCode: number) => void) | undefined;
let killCalls = 0;
const fakeChild = {
stdout: new ReadableStream<Uint8Array>({
start(controller) {
controller.close();
},
}),
stdin: { write: () => 0, flush: () => 0 },
exited: new Promise<number>(resolve => {
resolveExit = resolve;
}),
peekStderr: () => "worker went quiet",
kill() {
killCalls += 1;
resolveExit?.(0);
},
};
const spawn = spyOn(ptree, "spawn").mockImplementation(() => fakeChild as unknown as ChildProcess);
try {
using client = new RpcClient({ cliPath: MOCK_AGENT, terminationGraceMs: 10 });
await expect(client.start()).rejects.toThrow(
"Agent output stream ended before ready. Stderr: worker went quiet",
);
// The failed start must also reap the orphan rather than leak it.
expect(killCalls).toBe(1);
} finally {
spawn.mockRestore();
}
}, 5_000);
});
@@ -506,6 +506,8 @@ describe("createAgentSession credential_disabled subscription", () => {
]),
tools: new Map(),
assistantThinkingRenderers: [],
fileWriteFallbackHandlers: [],
fileDeleteFallbackHandlers: [],
messageRenderers: new Map(),
commands: new Map(),
flags: new Map(),
@@ -0,0 +1,586 @@
/**
* End-to-end proof of the `registerFileWriteFallback` seam: a REAL extension,
* loaded through the REAL loader/runner pipeline, registered on a REAL
* `createAgentSession` session, intercepting a REAL EACCES raised by the
* kernel for a genuinely-unwritable destination — not a fake resolver
* standing in for the extension path.
*
* Permission denial is simulated without a sandbox, since real permission
* bits behave differently depending on whether the destination already
* exists:
* - `write` targets a NEW file inside a directory chmod'd `0o500` (no write
* bit). Creating a file needs write permission on the *directory*, so this
* raises a real EACCES on `Bun.write`.
* - `edit` overwrites an EXISTING file chmod'd `0o400` (no write bit). Bun
* opens the existing inode directly for the rewrite, so a locked
* *directory* alone does NOT block it (verified empirically) — only a
* locked *file* does.
*/
import { afterAll, afterEach, beforeAll, describe, expect, it, spyOn } from "bun:test";
import * as fs from "node:fs";
import * as os from "node:os";
import * as path from "node:path";
import type { AgentTool } from "@oh-my-pi/pi-agent-core";
import { getBundledModel } from "@oh-my-pi/pi-catalog/models";
import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry";
import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import type {
ExtensionActions,
ExtensionContextActions,
ExtensionFactory,
ExtensionRunner,
} from "@oh-my-pi/pi-coding-agent/extensibility/extensions";
import { type CreateAgentSessionOptions, createAgentSession, discoverAuthStorage } from "@oh-my-pi/pi-coding-agent/sdk";
import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager";
import type { FileWriteFallbackRequest } from "@oh-my-pi/pi-coding-agent/tools/file-write-fallback";
import { removeSyncWithRetries, Snowflake } from "@oh-my-pi/pi-utils";
/**
* Drives `ExtensionRunner.initialize` with no-op stubs, mirroring what a mode
* controller (interactive/RPC/ACP/print/subagent) does after
* `createAgentSession` returns. Without this, `registerFileWriteFallback`
* handlers never install: `ExtensionRunner` binds them to a live `ctx` inside
* `initialize`, not at extension-load time.
*/
function initializeRunnerForTest(runner: ExtensionRunner | undefined): void {
if (!runner) return;
const actions: ExtensionActions = {
sendMessage: () => {},
sendUserMessage: () => {},
appendEntry: () => {},
setLabel: () => {},
getActiveTools: () => [],
getAllTools: () => [],
setActiveTools: async () => {},
getCommands: () => [],
setModel: async () => false,
getThinkingLevel: () => undefined,
setThinkingLevel: () => {},
getSessionName: () => undefined,
setSessionName: async () => {},
};
const contextActions: ExtensionContextActions = {
getModel: () => undefined,
isIdle: () => true,
abort: () => {},
hasPendingMessages: () => false,
shutdown: () => {},
getContextUsage: () => undefined,
compact: async () => {},
getSystemPrompt: () => [],
};
runner.initialize(actions, contextActions);
}
function resultText(result: { content: Array<{ type: string; text?: string }> }): string {
return result.content
.filter((b): b is { type: "text"; text: string } => b.type === "text" && typeof b.text === "string")
.map(b => b.text)
.join("\n");
}
const HASHLINE_HEADER_LINE = /^\[([^#\r\n]+)#([0-9A-F]{4})\]$/;
describe("registerFileWriteFallback end-to-end (real extension, real session)", () => {
const tempDirs: string[] = [];
const lockedDirs: string[] = [];
let modelRegistry!: ModelRegistry;
let registryAuthDir: string;
const makeTempDir = (): string => {
const created = path.join(os.tmpdir(), `pi-file-write-fallback-e2e-${Snowflake.next()}`);
fs.mkdirSync(created, { recursive: true });
// The seam brokers a symlink-RESOLVED path, and `os.tmpdir()` sits under `/var`
// — itself a link — on macOS. Canonicalizing the fixture up front keeps a
// handler's `req.dst` comparable to the path a test built.
const tempDir = fs.realpathSync.native(created);
tempDirs.push(tempDir);
return tempDir;
};
/** Tighten a mode and register it for restoration in `afterEach`, not in the test body. */
const lock = (target: string, mode: number): void => {
lockedDirs.push(target);
fs.chmodSync(target, mode);
};
// Mode bits do not constrain a privileged user, so `chmod` denies nothing as root
// and every expectation that depends on a real denial would fail for a reason
// unrelated to the seam. Root is real for a Docker-based local run and for a
// self-hosted CI runner. `getuid` is undefined on Windows, where these modes are
// not enforced either.
const itDenied = it.skipIf(process.platform === "win32" || process.getuid?.() === 0);
const baseOptions = (tempDir: string, extensions: ExtensionFactory[]): CreateAgentSessionOptions => ({
cwd: tempDir,
agentDir: tempDir,
modelRegistry,
sessionManager: SessionManager.inMemory(),
settings: Settings.isolated(),
model: getBundledModel("openai", "gpt-4o-mini"),
disableExtensionDiscovery: true,
extensions,
skills: [],
contextFiles: [],
promptTemplates: [],
slashCommands: [],
enableMCP: false,
enableLsp: false,
rules: [],
workspaceTree: { rootPath: tempDir, rendered: "", truncated: false, totalLines: 0, agentsMdFiles: [] },
});
beforeAll(async () => {
registryAuthDir = path.join(os.tmpdir(), `pi-file-write-fallback-e2e-auth-${Snowflake.next()}`);
fs.mkdirSync(registryAuthDir, { recursive: true });
modelRegistry = new ModelRegistry(await discoverAuthStorage(registryAuthDir));
});
afterAll(() => {
removeSyncWithRetries(registryAuthDir);
});
// Restore every mode this file tightened BEFORE removing the trees. A test that
// throws before its own `finally` would otherwise leave a 0o500 directory behind,
// and `removeSyncWithRetries` only retries on Windows — on macOS/Linux it throws
// EACCES, aborting this loop after `splice(0)` already emptied the list, which
// strands every remaining temp dir for the rest of the run.
afterEach(() => {
for (const dir of lockedDirs.splice(0)) {
try {
fs.chmodSync(dir, 0o700);
} catch {
// Already gone, or never created: nothing to restore.
}
}
for (const tempDir of tempDirs.splice(0)) {
removeSyncWithRetries(tempDir);
}
});
itDenied(
"write: a permission-denied create succeeds through a registered fallback, and a follow-up hashline edit on the real path works",
async () => {
const tempDir = makeTempDir();
const lockedDir = path.join(tempDir, "locked-write");
fs.mkdirSync(lockedDir, { recursive: true });
lock(lockedDir, 0o500); // no write bit: creating a file here needs dir-write
const received: FileWriteFallbackRequest[] = [];
const ownSessionIds: string[] = [];
const factory: ExtensionFactory = pi => {
pi.registerFileWriteFallback(async (req, ctx) => {
received.push(req);
ownSessionIds.push(ctx.sessionManager.getSessionId());
// Stand-in for an out-of-process privileged broker: this test's own
// user cannot write into `lockedDir`, so relax the permission bit
// just long enough to place the exact bytes the tool intended, then
// restore it — proving the handler alone determined success, not
// some ambient permission the tool already had.
fs.chmodSync(lockedDir, 0o700);
try {
fs.writeFileSync(req.dst, req.content);
} finally {
fs.chmodSync(lockedDir, 0o500);
}
return true;
});
};
const { session } = await createAgentSession(baseOptions(tempDir, [factory]));
initializeRunnerForTest(session.extensionRunner);
try {
const writeTool = session.getToolByName("write") as AgentTool | undefined;
expect(writeTool).toBeDefined();
const targetPath = path.join(lockedDir, "new-file.txt");
const content = "export const value = 42;\n";
const writeResult = await writeTool!.execute("call-write-1", { path: targetPath, content });
// (i) the tool call succeeds
expect(writeResult.isError).not.toBe(true);
// (ii) the fallback received the exact intended bytes and the real destination path
expect(received).toHaveLength(1);
expect(received[0]?.dst).toBe(targetPath);
expect(received[0]?.content).toBe(content);
// (ii-b) and it can tell WHOSE write it was: the registry is process-wide, so
// the request names the issuing session and `ctx` names the handler's own.
// Both defined and equal here, which only holds if the tool-execution scope
// that carries the session id is actually entered.
expect(ownSessionIds[0]).toMatch(/./);
expect(received[0]?.sessionId).toBe(ownSessionIds[0]);
expect(fs.readFileSync(targetPath, "utf8")).toBe(content);
const headerLine = resultText(writeResult).split("\n")[0] ?? "";
expect(HASHLINE_HEADER_LINE.test(headerLine)).toBe(true);
// (iii) a subsequent hashline edit on the SAME real path works — this only
// holds if the write tool recorded its snapshot under `targetPath` itself
// (not a temp path the fallback happened to route through).
const editTool = session.getToolByName("edit") as AgentTool | undefined;
expect(editTool).toBeDefined();
const editInput = `${headerLine}\nPUT 1-1:\n+export const value = 43;\n`;
const editResult = await editTool!.execute("call-edit-1", { input: editInput });
expect(editResult.isError).not.toBe(true);
expect(fs.readFileSync(targetPath, "utf8")).toBe("export const value = 43;\n");
} finally {
fs.chmodSync(lockedDir, 0o700);
await session.dispose();
}
},
);
itDenied(
"edit: a permission-denied overwrite of an existing file succeeds through a registered fallback",
async () => {
const tempDir = makeTempDir();
const targetPath = path.join(tempDir, "existing.txt");
const originalContent = "export const enabled = false;\n";
fs.writeFileSync(targetPath, originalContent);
lock(targetPath, 0o400); // no write bit on the file itself
const received: FileWriteFallbackRequest[] = [];
const factory: ExtensionFactory = pi => {
pi.registerFileWriteFallback(async req => {
received.push(req);
fs.chmodSync(targetPath, 0o600);
try {
fs.writeFileSync(req.dst, req.content);
} finally {
fs.chmodSync(targetPath, 0o400);
}
return true;
});
};
const { session } = await createAgentSession(baseOptions(tempDir, [factory]));
initializeRunnerForTest(session.extensionRunner);
try {
// A prior `read` is required to seed the file-snapshot tag the hashline
// edit addresses, mirroring how the model would discover an existing
// file's current tag before patching it.
const readTool = session.getToolByName("read") as AgentTool | undefined;
expect(readTool).toBeDefined();
const readResult = await readTool!.execute("call-read-1", { path: targetPath });
const readHeaderLine = resultText(readResult).split("\n")[0] ?? "";
expect(HASHLINE_HEADER_LINE.test(readHeaderLine)).toBe(true);
const editTool = session.getToolByName("edit") as AgentTool | undefined;
expect(editTool).toBeDefined();
const editInput = `${readHeaderLine}\nPUT 1-1:\n+export const enabled = true;\n`;
const editResult = await editTool!.execute("call-edit-2", { input: editInput });
expect(editResult.isError).not.toBe(true);
expect(received).toHaveLength(1);
expect(received[0]?.dst).toBe(targetPath);
expect(received[0]?.content).toBe("export const enabled = true;\n");
expect(fs.readFileSync(targetPath, "utf8")).toBe("export const enabled = true;\n");
} finally {
fs.chmodSync(targetPath, 0o600);
await session.dispose();
}
},
);
itDenied("edit: a hashline MV into an unwritable directory succeeds through a registered fallback", async () => {
// `MV` is the one `edit` write that never passes through the LSP writethrough
// (`HashlineFilesystem.move` writes the destination directly), so it needs its
// own end-to-end proof that the seam covers it.
const tempDir = makeTempDir();
const sourcePath = path.join(tempDir, "source.txt");
fs.writeFileSync(sourcePath, "export const stage = 1;\n");
const lockedDir = path.join(tempDir, "locked-move");
fs.mkdirSync(lockedDir, { recursive: true });
lock(lockedDir, 0o500);
const destPath = path.join(lockedDir, "moved.txt");
const received: FileWriteFallbackRequest[] = [];
const factory: ExtensionFactory = pi => {
pi.registerFileWriteFallback(async req => {
received.push(req);
fs.chmodSync(lockedDir, 0o700);
try {
fs.writeFileSync(req.dst, req.content);
} finally {
fs.chmodSync(lockedDir, 0o500);
}
return true;
});
};
const { session } = await createAgentSession(baseOptions(tempDir, [factory]));
initializeRunnerForTest(session.extensionRunner);
try {
const readTool = session.getToolByName("read") as AgentTool | undefined;
expect(readTool).toBeDefined();
const readResult = await readTool!.execute("call-read-mv", { path: sourcePath });
const readHeaderLine = resultText(readResult).split("\n")[0] ?? "";
expect(HASHLINE_HEADER_LINE.test(readHeaderLine)).toBe(true);
const editTool = session.getToolByName("edit") as AgentTool | undefined;
expect(editTool).toBeDefined();
const editInput = [readHeaderLine, "PUT 1-1:", "+export const stage = 2;", `MV ${destPath}`, ""].join("\n");
const editResult = await editTool!.execute("call-edit-mv", { input: editInput });
expect(editResult.isError).not.toBe(true);
expect(received).toHaveLength(1);
expect(received[0]?.dst).toBe(destPath);
expect(received[0]?.content).toBe("export const stage = 2;\n");
expect(fs.readFileSync(destPath, "utf8")).toBe("export const stage = 2;\n");
// `move` unlinks the source after the destination lands.
expect(fs.existsSync(sourcePath)).toBe(false);
} finally {
fs.chmodSync(lockedDir, 0o700);
await session.dispose();
}
});
it("edit: a hashline MV with no handler registered behaves exactly as before", async () => {
// Guards the seam's inertness claim on the one site that now reaches it
// outside the writethrough: with nothing registered, a plain MV must still
// move the file and this package has no other coverage for that path.
const tempDir = makeTempDir();
const sourcePath = path.join(tempDir, "plain-source.txt");
fs.writeFileSync(sourcePath, "export const stage = 1;\n");
const destPath = path.join(tempDir, "nested", "plain-dest.txt");
const { session } = await createAgentSession(baseOptions(tempDir, []));
initializeRunnerForTest(session.extensionRunner);
try {
const readTool = session.getToolByName("read") as AgentTool | undefined;
const readResult = await readTool!.execute("call-read-plain", { path: sourcePath });
const readHeaderLine = resultText(readResult).split("\n")[0] ?? "";
expect(HASHLINE_HEADER_LINE.test(readHeaderLine)).toBe(true);
const editTool = session.getToolByName("edit") as AgentTool | undefined;
const editInput = [readHeaderLine, "PUT 1-1:", "+export const stage = 2;", `MV ${destPath}`, ""].join("\n");
const editResult = await editTool!.execute("call-edit-plain-mv", { input: editInput });
expect(editResult.isError).not.toBe(true);
expect(fs.readFileSync(destPath, "utf8")).toBe("export const stage = 2;\n");
expect(fs.existsSync(sourcePath)).toBe(false);
} finally {
await session.dispose();
}
});
itDenied("edit: a permission-denied REM succeeds through a registered delete fallback", async () => {
// `REM` unlinks the file, which is a different primitive from the byte-write and
// has its own seam. A write fallback must NOT be consulted for it: a write
// handler brokers `content` to `dst`, so a delete arriving there would truncate
// the file instead of removing it.
const tempDir = makeTempDir();
const lockedDir = path.join(tempDir, "locked-rem");
fs.mkdirSync(lockedDir, { recursive: true });
const targetPath = path.join(lockedDir, "doomed.txt");
fs.writeFileSync(targetPath, "export const stage = 1;\n");
lock(lockedDir, 0o500);
const deleted: string[] = [];
const writeCalls: string[] = [];
const factory: ExtensionFactory = pi => {
pi.registerFileWriteFallback(async req => {
writeCalls.push(req.dst);
return false;
});
pi.registerFileDeleteFallback(async req => {
deleted.push(req.dst);
fs.chmodSync(lockedDir, 0o700);
try {
fs.rmSync(req.dst);
} finally {
fs.chmodSync(lockedDir, 0o500);
}
return true;
});
};
const { session } = await createAgentSession(baseOptions(tempDir, [factory]));
initializeRunnerForTest(session.extensionRunner);
try {
const readTool = session.getToolByName("read") as AgentTool | undefined;
const readResult = await readTool!.execute("call-read-rem", { path: targetPath });
const readHeaderLine = resultText(readResult).split("\n")[0] ?? "";
expect(HASHLINE_HEADER_LINE.test(readHeaderLine)).toBe(true);
const editTool = session.getToolByName("edit") as AgentTool | undefined;
const editResult = await editTool!.execute("call-edit-rem", {
input: [readHeaderLine, "REM", ""].join("\n"),
});
expect(editResult.isError).not.toBe(true);
expect(deleted).toEqual([targetPath]);
expect(writeCalls).toEqual([]);
expect(fs.existsSync(targetPath)).toBe(false);
} finally {
fs.chmodSync(lockedDir, 0o700);
await session.dispose();
}
});
itDenied("write: a throwing handler does not skip later handlers from the SAME extension", async () => {
// The registry sees ONE trampoline per extension, so per-handler isolation has to
// live inside that trampoline. Without it, a throw from the first handler escapes
// to the registry, which advances to the next EXTENSION — so every later handler
// this extension registered is skipped, breaking both the documented "a throwing
// handler is skipped" rule and registration order for a backup-handler setup.
const tempDir = makeTempDir();
const lockedDir = path.join(tempDir, "locked-order");
fs.mkdirSync(lockedDir, { recursive: true });
lock(lockedDir, 0o500);
const order: string[] = [];
const factory: ExtensionFactory = pi => {
pi.registerFileWriteFallback(async () => {
order.push("throws");
throw new Error("first handler blew up");
});
pi.registerFileWriteFallback(async () => {
order.push("declines");
return false;
});
pi.registerFileWriteFallback(async req => {
order.push("brokers");
fs.chmodSync(lockedDir, 0o700);
try {
fs.writeFileSync(req.dst, req.content);
} finally {
fs.chmodSync(lockedDir, 0o500);
}
return true;
});
};
const { session } = await createAgentSession(baseOptions(tempDir, [factory]));
initializeRunnerForTest(session.extensionRunner);
try {
const targetPath = path.join(lockedDir, "ordered.txt");
const content = "export const value = 3;\n";
const writeTool = session.getToolByName("write") as AgentTool | undefined;
const writeResult = await writeTool!.execute("call-write-order", { path: targetPath, content });
expect(writeResult.isError).not.toBe(true);
expect(order).toEqual(["throws", "declines", "brokers"]);
expect(fs.readFileSync(targetPath, "utf8")).toBe(content);
} finally {
fs.chmodSync(lockedDir, 0o700);
await session.dispose();
}
});
itDenied("write: a handler sees the session's CURRENT cwd, not the one captured at init", async () => {
// Handlers are installed once, at `ExtensionRunner.initialize`, but every other
// extension dispatch builds its `ExtensionContext` per call — and `createContext`
// materializes `cwd` as a value. A trampoline holding one context for the life of
// the session would keep reporting the workspace it initialized in, so a handler
// that scopes or prompts against `ctx.cwd` would allow the old workspace and deny
// the new one after a `/move`.
const tempDir = makeTempDir();
const lockedDir = path.join(tempDir, "locked-cwd");
fs.mkdirSync(lockedDir, { recursive: true });
lock(lockedDir, 0o500);
const seenCwds: string[] = [];
const factory: ExtensionFactory = pi => {
pi.registerFileWriteFallback(async (req, ctx) => {
seenCwds.push(ctx.cwd);
fs.chmodSync(lockedDir, 0o700);
try {
fs.writeFileSync(req.dst, req.content);
} finally {
fs.chmodSync(lockedDir, 0o500);
}
return true;
});
};
const { session } = await createAgentSession(baseOptions(tempDir, [factory]));
initializeRunnerForTest(session.extensionRunner);
// Stands in for `SessionManager.moveTo()` without relocating real session files:
// `getCwd()` is the session's own source of truth for its workspace, and it is
// what `ExtensionRunner.cwd` reads.
const moved = path.join(tempDir, "moved-workspace");
fs.mkdirSync(moved, { recursive: true });
const cwdSpy = spyOn(session.sessionManager, "getCwd").mockReturnValue(moved);
try {
const writeTool = session.getToolByName("write") as AgentTool | undefined;
const writeResult = await writeTool!.execute("call-write-cwd", {
path: path.join(lockedDir, "after-move.txt"),
content: "export const value = 4;\n",
});
expect(writeResult.isError).not.toBe(true);
expect(seenCwds).toEqual([moved]);
} finally {
cwdSpy.mockRestore();
fs.chmodSync(lockedDir, 0o700);
await session.dispose();
}
});
itDenied(
"edit: a hashline MV OUT of an undeletable directory removes the source through the delete seam",
async () => {
// `HashlineFilesystem.move` writes the destination and then unlinks the source as
// two separate primitives. Moving INTO a locked directory only exercises the
// write seam, because the source sits in the writable workspace. This is the
// mirror case, and the only end-to-end cover for the source-unlink site: before
// the delete seam the destination landed and the unlink threw, so the move failed
// with the original left behind.
const tempDir = makeTempDir();
const lockedDir = path.join(tempDir, "locked-source");
fs.mkdirSync(lockedDir, { recursive: true });
const sourcePath = path.join(lockedDir, "escaping.txt");
fs.writeFileSync(sourcePath, "export const stage = 1;\n");
const destPath = path.join(tempDir, "escaped.txt");
lock(lockedDir, 0o500);
const deleted: string[] = [];
const factory: ExtensionFactory = pi => {
pi.registerFileDeleteFallback(async req => {
deleted.push(req.dst);
fs.chmodSync(lockedDir, 0o700);
try {
fs.rmSync(req.dst);
} finally {
fs.chmodSync(lockedDir, 0o500);
}
return true;
});
};
const { session } = await createAgentSession(baseOptions(tempDir, [factory]));
initializeRunnerForTest(session.extensionRunner);
try {
const readTool = session.getToolByName("read") as AgentTool | undefined;
const readResult = await readTool!.execute("call-read-mv-out", { path: sourcePath });
const readHeaderLine = resultText(readResult).split("\n")[0] ?? "";
expect(HASHLINE_HEADER_LINE.test(readHeaderLine)).toBe(true);
const editTool = session.getToolByName("edit") as AgentTool | undefined;
const editResult = await editTool!.execute("call-edit-mv-out", {
input: [readHeaderLine, "PUT 1-1:", "+export const stage = 2;", `MV ${destPath}`, ""].join("\n"),
});
expect(editResult.isError).not.toBe(true);
expect(deleted).toEqual([sourcePath]);
expect(fs.readFileSync(destPath, "utf8")).toBe("export const stage = 2;\n");
expect(fs.existsSync(sourcePath)).toBe(false);
} finally {
fs.chmodSync(lockedDir, 0o700);
await session.dispose();
}
},
);
});
@@ -163,5 +163,11 @@ try {
rescue.close();
await fs.rm(home, { recursive: true, force: true });
}
}, 30_000);
// Budget must exceed the sum of the bounds inside the test: two 15s marker waits
// plus the 5s shutdown probe are 35s of legitimate waiting, so a 30s cap let a
// loaded runner kill the test mid-`waitUntil` and report only "timed out after
// 30000ms" instead of the marker assertion that actually failed. Each consumer is
// a cold `bun` process importing the daemon module graph, so the spawns are slow
// exactly when the machine is busy.
}, 60_000);
});
@@ -0,0 +1,796 @@
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,
withFileMutationSession,
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("names the session that issued the write, and reports none outside a tool call", async () => {
// The registry is process-wide, so a handler can be asked about a write from a
// session other than its own. Without this it cannot tell the difference, which
// is what makes a per-session decision (or a prompt through the right session's
// UI) impossible.
const seen: Array<string | undefined> = [];
disposers.push(
addFileWriteFallback(async req => {
seen.push(req.sessionId);
return true;
}),
);
await withFileMutationSession("session-a", () =>
writeFileWithFallback("/denied/path.txt", "payload", denyingFile(fsError("EACCES")) as never),
);
// No scope: an external `applyPatch` caller is not attributable to a session,
// and inventing one would be worse than saying so.
await writeFileWithFallback("/denied/path.txt", "payload", denyingFile(fsError("EACCES")) as never);
expect(seen).toEqual(["session-a", undefined]);
});
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 () => {
// Canonical from the start: the seam hands handlers a symlink-resolved path,
// and `os.tmpdir()` is under `/var` — itself a link — on macOS, so a lexical
// fixture path would differ from the brokered one for a reason unrelated to
// what these tests are about.
root = await fs.realpath(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("brokers the RESOLVED target for a write through a symlink", async () => {
// The escape this closes: 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, so the kernel denied the TARGET — but a handler given
// the LINK would pass its own prefix allowlist, because the link sits inside
// the allowed root while its target does not. The handler is told where the
// bytes would really land, so its allowlist judges the real destination.
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);
const seen: string[] = [];
disposers.push(
addFileWriteFallback(async req => {
seen.push(req.dst);
// Declining stands in for the allowlist refusal a real helper makes.
return false;
}),
);
try {
await expect(writeFileWithFallback(link, "pwned\n")).rejects.toMatchObject({
code: expect.stringMatching(/^(EACCES|EPERM)$/),
});
expect(seen).toEqual([secret]);
expect(await Bun.file(secret).text()).toBe("original\n");
} finally {
await fs.chmod(secretDir, 0o700);
await fs.chmod(secret, 0o600);
}
});
it("resolves a symlinked ANCESTOR, not just a link at the last component", async () => {
// `lstat(dst)` alone judges only the final component, so `ws/link/file` under
// a `ws/link -> /outside` link is a lexically innocent path whose bytes land
// outside. Every component above the last is followed by the kernel, so the
// handler has to be told the resolved path for this shape too.
const outside = path.join(root, "off-limits");
await fs.mkdir(outside);
const victim = path.join(outside, "secret.txt");
await Bun.write(victim, "original\n");
await fs.chmod(victim, 0o400);
await fs.chmod(outside, 0o500);
const linkDir = path.join(root, "innocent-dir");
await fs.symlink(outside, linkDir);
const seen: string[] = [];
disposers.push(
addFileWriteFallback(async req => {
seen.push(req.dst);
return false;
}),
);
try {
await expect(writeFileWithFallback(path.join(linkDir, "secret.txt"), "pwned\n")).rejects.toMatchObject({
code: expect.stringMatching(/^(EACCES|EPERM)$/),
});
expect(seen).toEqual([victim]);
expect(await Bun.file(victim).text()).toBe("original\n");
} finally {
await fs.chmod(outside, 0o700);
await fs.chmod(victim, 0o600);
}
});
it("refuses to broker a write through a dangling symlink", async () => {
// `realpath` cannot name where a dangling link points, and the write follows
// it, so there is no destination to hand a privileged writer. Refusing is the
// only honest answer, and it is the one `confineToWorkspace` already gives.
const dir = path.join(root, "locked");
await fs.mkdir(dir);
const dangling = path.join(dir, "dangling");
await fs.symlink(path.join(dir, "nowhere"), dangling);
await fs.chmod(dir, 0o500);
let called = false;
disposers.push(
addFileWriteFallback(async () => {
called = true;
return true;
}),
);
await expect(writeFileWithFallback(dangling, "payload")).rejects.toMatchObject({
code: expect.stringMatching(/^(EACCES|EPERM)$/),
});
expect(called).toBe(false);
});
it("refuses to broker a write whose own metadata is behind the boundary", async () => {
// A sandbox that denies the write often hides the target's metadata too, so
// the final component cannot be shown to be a plain name rather than a link —
// and `open` follows a link there. The delete seam keeps working in this shape
// because `unlink` never follows the last component; a write cannot.
const opaque = path.join(root, "opaque");
await fs.mkdir(opaque);
await fs.chmod(opaque, 0o000);
let called = false;
disposers.push(
addFileWriteFallback(async () => {
called = true;
return true;
}),
);
try {
await expect(writeFileWithFallback(path.join(opaque, "new.txt"), "payload")).rejects.toMatchObject({
code: expect.stringMatching(/^(EACCES|EPERM)$/),
});
expect(called).toBe(false);
} finally {
await fs.chmod(opaque, 0o700);
}
});
});
// `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.realpath(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)$/),
},
);
});
it("never brokers an exclusive create whose destination cannot be proven absent", async () => {
// `apply_patch`'s `create` refuses to overwrite, and it decides that with
// `Bun.file(dst).exists()`, which reports `false` when the parent hides the
// target's metadata instead of distinguishing "absent" from "unknown". The
// non-overwrite contract survives regardless, because the same denied `lstat`
// that fools the existence check also stops the seam from brokering: a
// privileged writer is never handed a destination whose identity is unproven,
// and it is the only party that could have enforced exclusivity itself.
//
// Those are two independent guards in two files, so this pins the pair. If the
// seam is ever relaxed to broker an unverifiable path, a `create` would start
// silently clobbering a protected file it was told not to touch.
const opaque = path.join(root, "opaque");
await fs.mkdir(opaque);
const victim = path.join(opaque, "victim.txt");
await Bun.write(victim, "original\n");
await fs.chmod(opaque, 0o000);
let called = false;
disposers.push(
addFileWriteFallback(async () => {
called = true;
return true;
}),
);
try {
// The premise: the existence check cannot see the file it must not clobber.
expect(await Bun.file(victim).exists()).toBe(false);
await expect(
applyPatch({ path: victim, op: "create", diff: "clobbered\n" }, { cwd: root }),
).rejects.toMatchObject({ code: expect.stringMatching(/^(EACCES|EPERM)$/) });
expect(called).toBe(false);
} finally {
await fs.chmod(opaque, 0o700);
}
expect(await Bun.file(victim).text()).toBe("original\n");
});
});
});
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 () => {
// Canonical from the start; see the write-side note above.
root = await fs.realpath(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, naming its session", async () => {
const target = await lockedFile();
const seen: Array<{ dst: string; code: unknown; sessionId: string | undefined }> = [];
disposers.push(
addFileDeleteFallback(async req => {
seen.push({ dst: req.dst, code: (req.cause as NodeJS.ErrnoException).code, sessionId: req.sessionId });
return true;
}),
);
await withFileMutationSession("session-del", () => deleteFileWithFallback(target));
expect(seen).toEqual([
{ dst: target, code: expect.stringMatching(/^(EACCES|EPERM)$/), sessionId: "session-del" },
]);
});
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("resolves a symlinked ANCESTOR before brokering a delete", async () => {
// `unlink` follows every component above the last, so a link in the path
// removes a file outside the allowed root while the lexical path still looks
// contained. The handler must be told which file actually disappears.
const outside = path.join(root, "off-limits");
await fs.mkdir(outside);
const victim = path.join(outside, "keep.txt");
await Bun.write(victim, "keep me");
await fs.chmod(outside, 0o500);
const linkDir = path.join(root, "innocent-dir");
await fs.symlink(outside, linkDir);
const seen: Array<{ dst: string; confirmedFile: boolean }> = [];
disposers.push(
addFileDeleteFallback(async req => {
seen.push({ dst: req.dst, confirmedFile: req.confirmedFile });
return false;
}),
);
try {
await expect(deleteFileWithFallback(path.join(linkDir, "keep.txt"))).rejects.toMatchObject({
code: expect.stringMatching(/^(EACCES|EPERM)$/),
});
expect(seen).toEqual([{ dst: victim, confirmedFile: true }]);
expect(await Bun.file(victim).text()).toBe("keep me");
} finally {
await fs.chmod(outside, 0o700);
}
});
it("leaves the LAST component unresolved, reporting confirmedFile false for a link", async () => {
// `unlink` removes the link itself, so resolving the final component would
// name the wrong file. Diverting is still right — unlinking a link is a
// legitimate file removal — but a handler that realpaths `dst` for auditing,
// or removes it recursively, would act on the link's TARGET, a directory tree
// here. So the link is brokered as itself, and `confirmedFile` is 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: Array<{ dst: string; confirmedFile: boolean }> = [];
disposers.push(
addFileDeleteFallback(async req => {
seen.push({ dst: req.dst, confirmedFile: req.confirmedFile });
return true;
}),
);
await deleteFileWithFallback(link);
expect(seen).toEqual([{ dst: link, confirmedFile: 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);
});
});
});
@@ -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");
});
});