diff --git a/docs/extensions.md b/docs/extensions.md index e589f9c88..06d3b4ecd 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -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. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index c87e5d8f7..089640459 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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. diff --git a/packages/coding-agent/src/edit/hashline/filesystem.ts b/packages/coding-agent/src/edit/hashline/filesystem.ts index 10dbdd283..3dea49458 100644 --- a/packages/coding-agent/src/edit/hashline/filesystem.ts +++ b/packages/coding-agent/src/edit/hashline/filesystem.ts @@ -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); } diff --git a/packages/coding-agent/src/edit/modes/patch.ts b/packages/coding-agent/src/edit/modes/patch.ts index feadc35fa..a507e64b6 100644 --- a/packages/coding-agent/src/edit/modes/patch.ts +++ b/packages/coding-agent/src/edit/modes/patch.ts @@ -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 { + 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 { @@ -123,13 +149,13 @@ export const defaultFileSystem: FileSystem = { return fs.promises.readFile(path); }, async write(path: string, content: string): Promise { - await Bun.write(path, await serializeEditFileText(path, path, content)); + await writeFileWithFallback(path, await serializeEditFileText(path, path, content)); }, async delete(path: string): Promise { - await fs.promises.unlink(path); + await deleteFileWithFallback(path); }, async mkdir(path: string): Promise { - await fs.promises.mkdir(path, { recursive: true }); + await mkdirAllowingFallback(path); }, }; @@ -1753,7 +1779,7 @@ class LspFileSystem implements FileSystem { } async delete(path: string): Promise { - 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 { - await fs.promises.mkdir(path, { recursive: true }); + await mkdirAllowingFallback(path); } getDiagnostics(): FileDiagnosticsResult | undefined { diff --git a/packages/coding-agent/src/extensibility/extensions/loader.ts b/packages/coding-agent/src/extensibility/extensions/loader.ts index 4b1c04651..b0fb283d3 100644 --- a/packages/coding-agent/src/extensibility/extensions/loader.ts +++ b/packages/coding-agent/src/extensibility/extensions/loader.ts @@ -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(), diff --git a/packages/coding-agent/src/extensibility/extensions/runner.ts b/packages/coding-agent/src/extensibility/extensions/runner.ts index 9378eb494..60e8189d2 100644 --- a/packages/coding-agent/src/extensibility/extensions/runner.ts +++ b/packages/coding-agent/src/extensibility/extensions/runner.ts @@ -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 { 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: "", 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(), diff --git a/packages/coding-agent/src/extensibility/extensions/types.ts b/packages/coding-agent/src/extensibility/extensions/types.ts index 6dcf2f129..da80737d3 100644 --- a/packages/coding-agent/src/extensibility/extensions/types.ts +++ b/packages/coding-agent/src/extensibility/extensions/types.ts @@ -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(tool: ToolDefinition): 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>; toolRegistrationListeners?: Set; assistantThinkingRenderers: AssistantThinkingRenderer[]; + fileWriteFallbackHandlers: FileWriteFallbackHandler[]; + fileDeleteFallbackHandlers: FileDeleteFallbackHandler[]; messageRenderers: Map; commands: Map; flags: Map; diff --git a/packages/coding-agent/src/extensibility/extensions/wrapper.ts b/packages/coding-agent/src/extensibility/extensions/wrapper.ts index 5dd3d33e4..a4aa75dc8 100644 --- a/packages/coding-agent/src/extensibility/extensions/wrapper.ts +++ b/packages/coding-agent/src/extensibility/extensions/wrapper.ts @@ -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 + this.tool.execute(toolCallId, effectiveParams, signal, onUpdate, context), + ); } catch (err) { executionError = err instanceof Error ? err : new Error(String(err)); result = { diff --git a/packages/coding-agent/src/lsp/writethrough.ts b/packages/coding-agent/src/lsp/writethrough.ts index ec8b90785..959cafeda 100644 --- a/packages/coding-agent/src/lsp/writethrough.ts +++ b/packages/coding-agent/src/lsp/writethrough.ts @@ -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 { - 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); diff --git a/packages/coding-agent/src/tools/file-write-fallback.ts b/packages/coding-agent/src/tools/file-write-fallback.ts new file mode 100644 index 000000000..dd7b14fa9 --- /dev/null +++ b/packages/coding-agent/src/tools/file-write-fallback.ts @@ -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; + +/** A handler already bound to its owning extension's live context. */ +type BoundFileWriteFallbackHandler = (req: FileWriteFallbackRequest) => Promise; + +/** 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; + +/** A handler already bound to its owning extension's live context. */ +type BoundFileDeleteFallbackHandler = (req: FileDeleteFallbackRequest) => Promise; + +const PERMISSION_DENIED_CODES: Record = { 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(); + +/** + * 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(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 { + 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 { + 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 { + // 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; + } + } +} diff --git a/packages/coding-agent/src/tools/index.ts b/packages/coding-agent/src/tools/index.ts index 0890fe1fd..4c0c4401c 100644 --- a/packages/coding-agent/src/tools/index.ts +++ b/packages/coding-agent/src/tools/index.ts @@ -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"; diff --git a/packages/coding-agent/src/tools/path-utils.ts b/packages/coding-agent/src/tools/path-utils.ts index 696f5799a..df153ee27 100644 --- a/packages/coding-agent/src/tools/path-utils.ts +++ b/packages/coding-agent/src/tools/path-utils.ts @@ -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 { + 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 { + 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 { + try { + return !(await fs.promises.lstat(target)).isSymbolicLink(); + } catch (error) { + return isEnoent(error); + } +} + export function formatPathRelativeToCwd( filePath: string, cwd: string, diff --git a/packages/coding-agent/test/extensions-runner.test.ts b/packages/coding-agent/test/extensions-runner.test.ts index e344bcbb6..6de740abe 100644 --- a/packages/coding-agent/test/extensions-runner.test.ts +++ b/packages/coding-agent/test/extensions-runner.test.ts @@ -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(), diff --git a/packages/coding-agent/test/rpc-client.restart.test.ts b/packages/coding-agent/test/rpc-client.restart.test.ts index e6f56b9d1..eff4df35d 100644 --- a/packages/coding-agent/test/rpc-client.restart.test.ts +++ b/packages/coding-agent/test/rpc-client.restart.test.ts @@ -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({ + start(controller) { + controller.close(); + }, + }), + stdin: { write: () => 0, flush: () => 0 }, + exited: new Promise(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); }); diff --git a/packages/coding-agent/test/sdk-credential-disabled-bridge.test.ts b/packages/coding-agent/test/sdk-credential-disabled-bridge.test.ts index e3715ad18..6504aa9a3 100644 --- a/packages/coding-agent/test/sdk-credential-disabled-bridge.test.ts +++ b/packages/coding-agent/test/sdk-credential-disabled-bridge.test.ts @@ -506,6 +506,8 @@ describe("createAgentSession credential_disabled subscription", () => { ]), tools: new Map(), assistantThinkingRenderers: [], + fileWriteFallbackHandlers: [], + fileDeleteFallbackHandlers: [], messageRenderers: new Map(), commands: new Map(), flags: new Map(), diff --git a/packages/coding-agent/test/sdk-file-write-fallback-extension.test.ts b/packages/coding-agent/test/sdk-file-write-fallback-extension.test.ts new file mode 100644 index 000000000..383fc182c --- /dev/null +++ b/packages/coding-agent/test/sdk-file-write-fallback-extension.test.ts @@ -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(); + } + }, + ); +}); diff --git a/packages/coding-agent/test/tools/browser-relay-daemon.test.ts b/packages/coding-agent/test/tools/browser-relay-daemon.test.ts index 96dcf1e61..8dd8abf4f 100644 --- a/packages/coding-agent/test/tools/browser-relay-daemon.test.ts +++ b/packages/coding-agent/test/tools/browser-relay-daemon.test.ts @@ -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); }); diff --git a/packages/coding-agent/test/tools/file-write-fallback.test.ts b/packages/coding-agent/test/tools/file-write-fallback.test.ts new file mode 100644 index 000000000..50efeb016 --- /dev/null +++ b/packages/coding-agent/test/tools/file-write-fallback.test.ts @@ -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 } { + 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 = []; + 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 { + 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 { + 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); + }); + }); +}); diff --git a/packages/coding-agent/test/tools/lsp-batching.test.ts b/packages/coding-agent/test/tools/lsp-batching.test.ts index 73bf14ce3..e0f44bc5b 100644 --- a/packages/coding-agent/test/tools/lsp-batching.test.ts +++ b/packages/coding-agent/test/tools/lsp-batching.test.ts @@ -1,8 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +import * as fs from "node:fs/promises"; import * as path from "node:path"; import { createLspWritethrough } from "@oh-my-pi/pi-coding-agent/lsp"; import * as lspConfig from "@oh-my-pi/pi-coding-agent/lsp/config"; import type { LinterClient, ServerConfig } from "@oh-my-pi/pi-coding-agent/lsp/types"; +import { addFileWriteFallback } from "@oh-my-pi/pi-coding-agent/tools/file-write-fallback"; import { TempDir } from "@oh-my-pi/pi-utils"; function createFormatter(format: (filePath: string, content: string) => Promise): ServerConfig { @@ -180,3 +182,61 @@ describe("createLspWritethrough batching", () => { expect(await Bun.file(filePath).text()).toBe("const single = true;\n"); }); }); + +// A privileged user is not constrained by mode bits: a 0o000 file stays both +// writable and readable, so the write would never be denied and the seam under +// test would never engage. +describe.skipIf(process.getuid?.() === 0)("createLspWritethrough batching with a brokered write", () => { + let tempDir: TempDir; + let root = ""; + const disposers: Array<() => void> = []; + + beforeEach(async () => { + tempDir = TempDir.createSync("@omp-lsp-batch-broker-"); + // The seam hands handlers a symlink-resolved path and `os.tmpdir()` sits + // under `/var` — itself a link — on macOS, so a lexical fixture root would + // differ from the brokered path for a reason unrelated to this test. + root = await fs.realpath(tempDir.path()); + }); + + afterEach(async () => { + for (const dispose of disposers.splice(0)) dispose(); + vi.restoreAllMocks(); + await fs.chmod(path.join(root, "opaque.ts"), 0o600).catch(() => {}); + tempDir.removeSync(); + }); + + it("flushes a batch whose brokered destination cannot be read back", async () => { + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined }); + vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([]); + const writethrough = createLspWritethrough(root, { enableFormat: true, enableDiagnostics: true }); + + // Denied for writing and for reading at once, which is what a sandbox that + // hides a path produces: the direct write fails, a privileged helper lands + // the bytes, and this process still cannot read them back. + const opaque = path.join(root, "opaque.ts"); + await Bun.write(opaque, "const before = true;\n"); + await fs.chmod(opaque, 0o000); + + const brokered: Array<{ dst: string; content: string }> = []; + disposers.push( + addFileWriteFallback(async req => { + brokered.push({ dst: req.dst, content: req.content }); + await fs.chmod(req.dst, 0o600); + await Bun.write(req.dst, req.content); + await fs.chmod(req.dst, 0o000); + return true; + }), + ); + + const sibling = path.join(root, "sibling.ts"); + const batchId = `brokered-${Date.now()}`; + await writethrough(opaque, "const after = true;\n", undefined, undefined, { id: batchId, flush: false }); + await writethrough(sibling, "const other = true;\n", undefined, undefined, { id: batchId, flush: true }); + + expect(brokered).toEqual([{ dst: opaque, content: "const after = true;\n" }]); + expect(await Bun.file(sibling).text()).toBe("const other = true;\n"); + await fs.chmod(opaque, 0o400); + expect(await Bun.file(opaque).text()).toBe("const after = true;\n"); + }); +});