From 8049a1f2803ce5821d2938f075049c2a6e008a8a Mon Sep 17 00:00:00 2001 From: Antoni T Date: Mon, 3 Aug 2026 14:17:32 +0100 Subject: [PATCH 01/13] dofs: scope the read-only guard to a filesystem handle Until now the only thing that could refuse a write was a registered read-only mount root, which is a property of the stored tree. Add a second dimension that is a property of the handle instead: build a `WorkspaceFilesystem` or `SQLiteWorkspaceProvider` with `writable: false` and every mutating method on it reports EROFS. The capability is fixed at construction and cannot be changed, which is the point. A command is not one write. `rm -rf` on a populated tree issues hundreds of calls, so a decision taken per write would let the first forty land before the forty-first is refused, leaving a half-deleted tree behind. A capability that holds for as long as the handle exists cannot produce that state. It lives on the handle rather than on the `Database` because two commands can be in flight against one workspace and need not agree about write access. `resolveCache` documents that exactly one `Database` wraps each `SqlStorage`, so a second handle is not available as a place to put this, and a flag on the shared one would let a read-only command disarm a writable command running beside it. Each check now sits at the layer that owns its state. The mount mode stays next to the tree, where the sync apply path still sees it. The capability sits on the handle's own methods, where the flag is immutable and cannot be flipped between the check and the commit. Four provider methods reached SQLite without consulting any guard: `chmodSync`, `openWriteBufferSync`, `openWriteBufferForCreateSync`, and `releaseWriteBufferSync`. Under mount-root enforcement alone they were narrow. A whole-handle capability makes them the obvious way out, since a command driven through a FUSE mount reaches all four, so they are guarded too. A read-only provider also refuses to open a file for writing at all, the way opening for write on a read-only filesystem fails at the open rather than at the first write. `WorkspaceFilesystem.writeFile` becomes async. It returns a promise, so a guard that threw synchronously would escape a caller using `.catch()` rather than arriving as a rejection. --- packages/dofs/README.md | 9 + packages/dofs/src/fs/filesystem.ts | 25 +- packages/dofs/src/fs/mount-guard.ts | 61 ++++- packages/dofs/src/fs/write-capability.test.ts | 226 ++++++++++++++++++ packages/dofs/src/index.ts | 20 +- packages/dofs/src/provider.ts | 42 +++- 6 files changed, 369 insertions(+), 14 deletions(-) create mode 100644 packages/dofs/src/fs/write-capability.test.ts diff --git a/packages/dofs/README.md b/packages/dofs/README.md index 8eacbf4a..a8a3e13e 100644 --- a/packages/dofs/README.md +++ b/packages/dofs/README.md @@ -59,3 +59,12 @@ export class WorkspaceDO extends DurableObject { Repeated reads of dedup'd chunks (a file of zeroes, a re-used package payload) skip SQLite after the first fetch. - Sync protocol building blocks implemented and exported; the typed RPC surface on top of them lives in `@cloudflare/computer-rpc`. +- Two independent write guards, both reporting `EROFS`. A registered + read-only mount root is enforced next to the stored tree, so writes + arriving through the sync apply path are caught as well as direct + ones. A write capability is enforced on the handle: build a + `WorkspaceFilesystem` or `SQLiteWorkspaceProvider` with + `writable: false` and every mutating method on it refuses. The + capability is fixed for the life of the handle, which is what lets a + caller run one whole command without write access instead of + stopping it part-way through. diff --git a/packages/dofs/src/fs/filesystem.ts b/packages/dofs/src/fs/filesystem.ts index 82ea588c..ae505776 100644 --- a/packages/dofs/src/fs/filesystem.ts +++ b/packages/dofs/src/fs/filesystem.ts @@ -19,6 +19,7 @@ import { find, type WorkspaceFoundEntry } from "./find.js"; import { type GrepOptions, grep, type WorkspaceGrepMatch } from "./grep.js"; import { ls } from "./ls.js"; import { type MkdirOptions, mkdir } from "./mkdir.js"; +import { assertWritable } from "./mount-guard.js"; import { type ReaddirOptions, readdir, type WorkspaceDirentResult } from "./readdir.js"; import { type ReadFileOptions, readFile } from "./readFile.js"; import { readlink } from "./readlink.js"; @@ -31,15 +32,28 @@ export interface WorkspaceFilesystemOptions { // Clock used for mtime / last_seen. Defaults to Date.now. // Override for deterministic tests. now?: () => number; + + // Write access for this handle. Defaults to true. Pass false to + // get a handle whose mutating methods all reject with EROFS, + // which is how a single command runs without write access. + // + // The capability belongs to the handle, not to the database, so + // two handles over one database can disagree. That matters + // because commands overlap: a read-only command must not be able + // to disarm a writable one running beside it, and a writable one + // must not lend its access to a read-only one. + writable?: boolean; } export class WorkspaceFilesystem { readonly db: Database; readonly now: () => number; + readonly writable: boolean; constructor(db: Database, options: WorkspaceFilesystemOptions = {}) { this.db = db; this.now = options.now ?? Date.now; + this.writable = options.writable ?? true; } // --- Reads ------------------------------------------------------- @@ -96,19 +110,26 @@ export class WorkspaceFilesystem { // --- Mutations --------------------------------------------------- - writeFile( + // Declared async so a refusal arrives as a rejected promise. The + // guard throws, and a plain `return writeFile(...)` would let that + // throw escape synchronously out of a method whose signature + // promises otherwise, which a caller using `.catch()` would miss. + async writeFile( path: string, content: WriteFileContent, options: WriteFileOptions = {}, ): Promise { + assertWritable(this, path); return writeFile(this.db, path, content, options, this.now); } async mkdir(path: string, options: MkdirOptions = {}): Promise { + assertWritable(this, path); mkdir(this.db, path, options, this.now); } async rm(path: string, options: RmOptions = {}): Promise { + assertWritable(this, path); rm(this.db, path, options); } @@ -116,6 +137,7 @@ export class WorkspaceFilesystem { // POSIX chmod — the change lands on the target, not the link. // The supplied mode is masked to twelve bits. async chmod(path: string, mode: number): Promise { + assertWritable(this, path); chmod(this.db, path, mode, this.now); } @@ -123,6 +145,7 @@ export class WorkspaceFilesystem { // target is stored verbatim; it can be relative or absolute and // is allowed to dangle. async symlink(target: string, path: string): Promise { + assertWritable(this, path); symlink(this.db, target, path, this.now); } } diff --git a/packages/dofs/src/fs/mount-guard.ts b/packages/dofs/src/fs/mount-guard.ts index 6a2aadd3..ad32882d 100644 --- a/packages/dofs/src/fs/mount-guard.ts +++ b/packages/dofs/src/fs/mount-guard.ts @@ -1,11 +1,30 @@ -// Read-only mount guard. +// Write guard. // -// Every dofs mutating entry point (writeFile, mkdir, rm, and the -// apply path in sync/apply.ts) consults this module to reject -// writes that fall under a registered read-only mount root. The -// guard lives at the data layer so container-side writes that -// arrive via pullOnce -> applyChanges are caught too — the -// workspace-side surface wrapper alone cannot see them. +// Two separate things can refuse a write, and both report EROFS. +// +// The first is a read-only mount root. Every dofs mutating entry +// point (writeFile, mkdir, rm, and the apply path in sync/apply.ts) +// consults this module to reject writes that fall under a +// registered read-only mount root. That check lives at the data +// layer so container-side writes arriving via pullOnce -> +// applyChanges are caught too, which a surface wrapper alone +// cannot see. +// +// The second is a write capability, and it belongs to whoever holds +// the filesystem handle rather than to the data. A caller that +// builds a WorkspaceFilesystem or SQLiteWorkspaceProvider with +// `writable: false` gets a handle that refuses every mutation for +// as long as it exists. The point is to run one command without +// write access: a command is not atomic, so deciding per write +// would let the first forty writes land before the forty-first is +// refused. A capability fixed for the handle's lifetime cannot +// produce that state. +// +// Each check sits at the layer that owns its state. The mount mode +// is a property of the stored tree, so it is enforced next to the +// tree. The capability is a property of the handle, so it is +// enforced on the handle's own methods, where the flag is +// immutable and cannot be flipped part-way through a write. // // The set of read-only roots is small (one row per registered // mount per workspace, typically <10) and changes only at indexer @@ -17,6 +36,34 @@ import { createWorkspaceError } from "../errors.js"; import type { Database } from "../storage.js"; +/** + * A handle's write access. `WorkspaceFilesystem` and + * `SQLiteWorkspaceProvider` both satisfy this, so they can pass + * themselves to `assertWritable`. + * + * The field is deliberately immutable on both. A mutable flag would + * reopen the window the capability exists to close: `writeFile` + * checks once and then awaits its source stream many times before + * committing, so a flag that could change mid-call would let a write + * that was refused on entry still land. + */ +export interface WriteCapability { + readonly writable: boolean; +} + +/** + * Throws EROFS when the handle has no write access. Call this before + * any mutation on a surface that carries a capability. + * + * The message is distinct from the mount-root message below so a + * caller reading `error.message` can tell a command that was denied + * write access from a command that reached into a read-only mount. + */ +export function assertWritable(capability: WriteCapability, path: string): void { + if (capability.writable) return; + throw createWorkspaceError("EROFS", "read-only access: cannot modify", path); +} + // undefined sentinel = "not loaded yet"; an empty array means // "loaded, no read-only mounts registered". The two are not the // same: the empty case must skip the SQL lookup on every check. diff --git a/packages/dofs/src/fs/write-capability.test.ts b/packages/dofs/src/fs/write-capability.test.ts new file mode 100644 index 00000000..306f16da --- /dev/null +++ b/packages/dofs/src/fs/write-capability.test.ts @@ -0,0 +1,226 @@ +import { describe, expect, it } from "vitest"; + +import { SQLiteWorkspaceProvider } from "../provider.js"; +import { WorkspaceFilesystem } from "./filesystem.js"; +import { withDB } from "./with-db.js"; + +const clock = () => 1000; + +describe("WorkspaceFilesystem write capability", () => { + it("defaults to writable so existing callers are unaffected", async () => { + await withDB(async (db) => { + const fs = new WorkspaceFilesystem(db, { now: clock }); + + await fs.mkdir("/workspace", { recursive: true }); + await fs.writeFile("/workspace/hello.txt", "hi"); + + expect(await fs.readFile("/workspace/hello.txt", "utf8")).toBe("hi"); + }); + }); + + it("rejects writeFile with EROFS on a read-only handle", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + await writable.mkdir("/workspace", { recursive: true }); + + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + await expect(readOnly.writeFile("/workspace/hello.txt", "hi")).rejects.toMatchObject({ + code: "EROFS", + }); + + await expect(writable.readFile("/workspace/hello.txt", "utf8")).rejects.toMatchObject({ + code: "ENOENT", + }); + }); + }); + + it("rejects mkdir with EROFS on a read-only handle", async () => { + await withDB(async (db) => { + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + + await expect(readOnly.mkdir("/workspace", { recursive: true })).rejects.toMatchObject({ + code: "EROFS", + }); + }); + }); + + it("rejects rm with EROFS on a read-only handle and leaves the file in place", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + await writable.mkdir("/workspace", { recursive: true }); + await writable.writeFile("/workspace/keep.txt", "still here"); + + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + await expect(readOnly.rm("/workspace/keep.txt", {})).rejects.toMatchObject({ code: "EROFS" }); + + expect(await writable.readFile("/workspace/keep.txt", "utf8")).toBe("still here"); + }); + }); + + it("rejects chmod with EROFS on a read-only handle", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + await writable.mkdir("/workspace", { recursive: true }); + await writable.writeFile("/workspace/script.sh", "echo hi"); + + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + await expect(readOnly.chmod("/workspace/script.sh", 0o755)).rejects.toMatchObject({ + code: "EROFS", + }); + }); + }); + + it("rejects symlink with EROFS on a read-only handle", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + await writable.mkdir("/workspace", { recursive: true }); + + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + await expect(readOnly.symlink("/workspace", "/link")).rejects.toMatchObject({ + code: "EROFS", + }); + }); + }); + + it("serves every read from a read-only handle", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + await writable.mkdir("/workspace/sub", { recursive: true }); + await writable.writeFile("/workspace/a.txt", "alpha"); + await writable.writeFile("/workspace/sub/b.txt", "beta"); + + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + + expect(await readOnly.readFile("/workspace/a.txt", "utf8")).toBe("alpha"); + expect((await readOnly.stat("/workspace/a.txt")).size).toBe(5); + expect((await readOnly.readdir("/workspace")).map((e) => e.name).sort()).toEqual([ + "a.txt", + "sub", + ]); + expect(await readOnly.ls("/workspace")).toContain("/workspace/a.txt"); + expect(await readOnly.find("/workspace", "*.txt")).toEqual([ + { path: "/workspace/a.txt", type: "file" }, + ]); + expect(await readOnly.grep("beta", "/workspace")).toHaveLength(1); + }); + }); + + // Two commands can be in flight against one workspace at the same + // time, and they do not have to agree about write access. The + // capability belongs to the handle rather than to the database, so + // a read-only command cannot disarm a concurrent writable one, and + // a writable command cannot lend its access to a read-only one. + it("leaves a writable handle over the same database unaffected", async () => { + await withDB(async (db) => { + const writable = new WorkspaceFilesystem(db, { now: clock }); + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + + await writable.mkdir("/workspace", { recursive: true }); + await expect(readOnly.writeFile("/workspace/denied.txt", "no")).rejects.toMatchObject({ + code: "EROFS", + }); + + await writable.writeFile("/workspace/allowed.txt", "yes"); + expect(await readOnly.readFile("/workspace/allowed.txt", "utf8")).toBe("yes"); + }); + }); + + it("names the path it refused", async () => { + await withDB(async (db) => { + const readOnly = new WorkspaceFilesystem(db, { now: clock, writable: false }); + + await expect(readOnly.writeFile("/workspace/hello.txt", "hi")).rejects.toMatchObject({ + code: "EROFS", + path: "/workspace/hello.txt", + }); + }); + }); +}); + +describe("SQLiteWorkspaceProvider write capability", () => { + it("advertises the capability through the readonly flag", async () => { + await withDB(async (db) => { + expect(new SQLiteWorkspaceProvider(db, { now: clock }).readonly).toBe(false); + expect(new SQLiteWorkspaceProvider(db, { now: clock, writable: false }).readonly).toBe(true); + }); + }); + + it("rejects every mutating call with EROFS on a read-only provider", async () => { + await withDB(async (db) => { + const writable = new SQLiteWorkspaceProvider(db, { now: clock }); + writable.mkdirSync("/workspace", { recursive: true }); + writable.writeFileSync("/workspace/a.txt", "alpha"); + writable.mkdirSync("/workspace/dir", { recursive: true }); + + const readOnly = new SQLiteWorkspaceProvider(db, { now: clock, writable: false }); + const refused = (run: () => unknown) => + expect(run).toThrowError(expect.objectContaining({ code: "EROFS" })); + + refused(() => readOnly.writeFileSync("/workspace/b.txt", "beta")); + refused(() => readOnly.writeFileRangesSync("/workspace/a.txt", "x", [{ start: 0, end: 1 }])); + refused(() => readOnly.mkdirSync("/workspace/other", { recursive: true })); + refused(() => readOnly.rmdirSync("/workspace/dir")); + refused(() => readOnly.unlinkSync("/workspace/a.txt")); + refused(() => readOnly.renameSync("/workspace/a.txt", "/workspace/moved.txt")); + refused(() => readOnly.linkSync("/workspace/a.txt", "/workspace/hard.txt")); + refused(() => readOnly.symlinkSync("/workspace", "/link")); + refused(() => readOnly.truncateSync("/workspace/a.txt", 0)); + refused(() => readOnly.createFileSync("/workspace/c.txt")); + refused(() => readOnly.writeRangeSync("/workspace/a.txt", new Uint8Array([1]), 0)); + refused(() => readOnly.truncateFileSync("/workspace/a.txt", 0)); + refused(() => readOnly.openSync("/workspace/new.txt", "w")); + + // These four reached the database without consulting the guard + // before the capability existed. A read-only command drives all + // of them through a FUSE mount, so they are the ones worth + // naming. + refused(() => readOnly.chmodSync("/workspace/a.txt", 0o755)); + refused(() => readOnly.openWriteBufferSync("/workspace/a.txt")); + refused(() => readOnly.openWriteBufferForCreateSync("/workspace/d.txt")); + refused(() => readOnly.releaseWriteBufferSync("/workspace/a.txt")); + + // Nothing landed. + expect(writable.readFileSync("/workspace/a.txt", "utf8")).toBe("alpha"); + expect(writable.existsSync("/workspace/b.txt")).toBe(false); + expect(writable.existsSync("/workspace/dir")).toBe(true); + }); + }); + + // Opening for write on a read-only filesystem fails at the open, + // not later on the first write, so a read-only provider never hands + // out a writable descriptor. The write path is guarded too, which + // covers a descriptor obtained some other way. + it("refuses to open a file for writing", async () => { + await withDB(async (db) => { + const writable = new SQLiteWorkspaceProvider(db, { now: clock }); + writable.mkdirSync("/workspace", { recursive: true }); + writable.writeFileSync("/workspace/a.txt", "alpha"); + + const readOnly = new SQLiteWorkspaceProvider(db, { now: clock, writable: false }); + for (const flags of ["w", "a", "r+", "w+"]) { + expect(() => readOnly.openSync("/workspace/a.txt", flags)).toThrowError( + expect.objectContaining({ code: "EROFS" }), + ); + } + + expect(writable.readFileSync("/workspace/a.txt", "utf8")).toBe("alpha"); + }); + }); + + it("serves reads from a read-only provider", async () => { + await withDB(async (db) => { + const writable = new SQLiteWorkspaceProvider(db, { now: clock }); + writable.mkdirSync("/workspace", { recursive: true }); + writable.writeFileSync("/workspace/a.txt", "alpha"); + + const readOnly = new SQLiteWorkspaceProvider(db, { now: clock, writable: false }); + + expect(readOnly.readFileSync("/workspace/a.txt", "utf8")).toBe("alpha"); + expect(readOnly.statSync("/workspace/a.txt").size).toBe(5); + expect(readOnly.readdirSync("/workspace")).toEqual(["a.txt"]); + expect(readOnly.existsSync("/workspace/a.txt")).toBe(true); + const fd = readOnly.openSync("/workspace/a.txt", "r"); + readOnly.closeSync(fd); + }); + }); +}); diff --git a/packages/dofs/src/index.ts b/packages/dofs/src/index.ts index 76ef05a5..dda80a1b 100644 --- a/packages/dofs/src/index.ts +++ b/packages/dofs/src/index.ts @@ -9,16 +9,26 @@ export type { WorkspaceFoundEntry } from "./fs/find.js"; export type { GrepOptions, WorkspaceGrepMatch } from "./fs/grep.js"; export { link } from "./fs/link.js"; export type { MkdirOptions } from "./fs/mkdir.js"; -// Read-only mount enforcement. The workspace-side indexer writes -// _vfs_mounts; the helpers here let it invalidate the in-Database -// cache after a write, and let dofs callers (and tests) inspect or -// assert against the registered roots without re-implementing the -// overlap check. +// Write enforcement. Two things refuse a write, and both report +// EROFS: a registered read-only mount root, and a handle built +// without write access. +// +// For mounts, the workspace-side indexer writes _vfs_mounts; the +// helpers here let it invalidate the in-Database cache after a write, +// and let dofs callers (and tests) inspect or assert against the +// registered roots without re-implementing the overlap check. +// +// For handles, `assertWritable` is what `WorkspaceFilesystem` and +// `SQLiteWorkspaceProvider` call on every mutation, and +// `WriteCapability` is the shape any other surface can adopt to join +// in. export { assertNotReadOnly, + assertWritable, getReadOnlyMountRoots, invalidateReadOnlyMountCache, readOnlyRootFor, + type WriteCapability, } from "./fs/mount-guard.js"; export type { ReaddirOptions, WorkspaceDirentResult } from "./fs/readdir.js"; export type { ReadFileOptions } from "./fs/readFile.js"; diff --git a/packages/dofs/src/provider.ts b/packages/dofs/src/provider.ts index 5adcafc8..a44d08da 100644 --- a/packages/dofs/src/provider.ts +++ b/packages/dofs/src/provider.ts @@ -12,6 +12,7 @@ import { getBlobBytes } from "./fs/blobCache.js"; import { link as linkImpl } from "./fs/link.js"; import type { MkdirOptions } from "./fs/mkdir.js"; import { mkdir as mkdirImpl } from "./fs/mkdir.js"; +import { assertWritable } from "./fs/mount-guard.js"; import { readdir as readdirImpl } from "./fs/readdir.js"; import { readRangeSync as readRangeSyncImpl } from "./fs/readFile.js"; import { readlink as readlinkImpl } from "./fs/readlink.js"; @@ -56,6 +57,12 @@ export interface SQLiteWorkspaceProviderOptions { // to match node's fs.watch on most filesystems; tests can lower // it to keep durations short. watchIntervalMs?: number; + // Write access for this provider. Defaults to true. Pass false to + // get a provider whose mutating calls all reject with EROFS, which + // is how a single command runs without write access. The flag also + // shows up as `readonly` for @platformatic/vfs callers that ask + // before they write. + writable?: boolean; } interface VirtualStatsLike { @@ -113,8 +120,13 @@ export class SQLiteWorkspaceProvider { readonly db: Database; readonly now: () => number; + // Write access for this provider, fixed at construction. Every + // mutating method checks it, so a `writable: false` provider + // refuses the whole command rather than part of it. + readonly writable: boolean; + // Capability flags consulted by @platformatic/vfs callers. - readonly readonly = false; + readonly readonly: boolean; readonly supportsSymlinks = true; readonly supportsWatch = true; @@ -130,6 +142,8 @@ export class SQLiteWorkspaceProvider { this.db = db; this.now = options.now ?? Date.now; this.watchIntervalMs = options.watchIntervalMs ?? 100; + this.writable = options.writable ?? true; + this.readonly = !this.writable; } // -- Essential primitives ------------------------------------------ @@ -140,6 +154,10 @@ export class SQLiteWorkspaceProvider { openSync(path: string, flags: string = "r", _mode?: number): number { const { read, write, truncate, append, create, exclusive } = parseFlags(flags); + // A read-only provider refuses to hand out a writable + // descriptor at all, the way opening for write on a read-only + // filesystem fails rather than failing later on the first write. + if (write || truncate || create) assertWritable(this, path); const existing = resolveInode(this.db, path); if (existing === null) { @@ -253,6 +271,7 @@ export class SQLiteWorkspaceProvider { } mkdirSync(path: string, options?: MkdirOptions): string | undefined { + assertWritable(this, path); mkdirImpl(this.db, path, options ?? {}, this.now); return undefined; } @@ -263,6 +282,7 @@ export class SQLiteWorkspaceProvider { } rmdirSync(path: string): void { + assertWritable(this, path); rmImpl(this.db, path, {}); } @@ -272,6 +292,7 @@ export class SQLiteWorkspaceProvider { } unlinkSync(path: string): void { + assertWritable(this, path); // If a buffered create is still pending for this path, commit // it first so rm sees a real inode to unlink (and so the // resulting GC sees the orphaned blob, matching the non-buffered @@ -301,6 +322,7 @@ export class SQLiteWorkspaceProvider { } linkSync(existingPath: string, newPath: string): void { + assertWritable(this, newPath); // Commit a still-pending source before adding the second dirent, // otherwise link has nothing real to point at. Also commit a // still-pending destination: link's existence check looks at @@ -319,6 +341,8 @@ export class SQLiteWorkspaceProvider { } renameSync(oldPath: string, newPath: string): void { + assertWritable(this, oldPath); + assertWritable(this, newPath); // Commit any still-pending creates at either end before the rename // touches dirents: the source needs a real inode to move, and a // pending buffer at the destination would otherwise slip past @@ -411,6 +435,7 @@ export class SQLiteWorkspaceProvider { data: string | Buffer, options?: { encoding?: BufferEncoding; mode?: number } | BufferEncoding, ): void { + assertWritable(this, path); const mode = typeof options === "string" ? undefined : options?.mode; const bytes = typeof data === "string" @@ -425,6 +450,7 @@ export class SQLiteWorkspaceProvider { ranges: WriteFileRange[], options?: { encoding?: BufferEncoding; mode?: number } | BufferEncoding, ): void { + assertWritable(this, path); const mode = typeof options === "string" ? undefined : options?.mode; const bytes = typeof data === "string" @@ -434,6 +460,7 @@ export class SQLiteWorkspaceProvider { } createFileSync(path: string, options?: { mode?: number }): void { + assertWritable(this, path); createFileSyncImpl(this.db, path, { mode: options?.mode }, this.now); } @@ -443,6 +470,7 @@ export class SQLiteWorkspaceProvider { offset: number, options?: { encoding?: BufferEncoding; mode?: number } | BufferEncoding, ): number { + assertWritable(this, path); const mode = typeof options === "string" ? undefined : options?.mode; const bytes = typeof data === "string" @@ -452,22 +480,27 @@ export class SQLiteWorkspaceProvider { } truncateFileSync(path: string, len: number): void { + assertWritable(this, path); truncateFileSyncImpl(this.db, path, len, this.now); } openWriteBufferSync(path: string): void { + assertWritable(this, path); openWriteBufferSyncImpl(this.db, path); } openWriteBufferForCreateSync(path: string, options?: { mode?: number }): void { + assertWritable(this, path); openWriteBufferForCreateSyncImpl(this.db, path, { mode: options?.mode }, this.now); } releaseWriteBufferSync(path: string): void { + assertWritable(this, path); releaseWriteBufferSyncImpl(this.db, path, this.now); } chmodSync(path: string, mode: number): void { + assertWritable(this, path); const { path: canonical } = canonicalizePath(path); const pending = getPendingWriteBufferByPath(this.db, canonical); if (pending !== undefined) { @@ -605,6 +638,11 @@ export class SQLiteWorkspaceProvider { if (!state.writable) { throw createWorkspaceError("EBADF", `fd ${fd} is not writable`); } + // A read-only provider refuses to open for write, so it should + // never hold a writable descriptor to begin with. Check anyway: + // if some later change adds another way to mint one, the write + // still stops here rather than reaching the database. + assertWritable(this, state.path); // Append needs the current EOF, so stat only then. A non-append // write of >0 bytes doesn't need it: writeRangeSyncImpl resolves the // path and raises ENOENT/EISDIR. A zero-length write short-circuits @@ -635,6 +673,7 @@ export class SQLiteWorkspaceProvider { } truncateSync(path: string, len: number): void { + assertWritable(this, path); const node = resolveInode(this.db, path); if (node === null) { throw createWorkspaceError("ENOENT", `no such path: ${path}`, path); @@ -674,6 +713,7 @@ export class SQLiteWorkspaceProvider { } symlinkSync(target: string, path: string, _type?: string): void { + assertWritable(this, path); symlinkImpl(this.db, target, path, this.now); } From 7cceeebd5e0bbdab560d597608245366037af4ea Mon Sep 17 00:00:00 2001 From: Antoni T Date: Mon, 3 Aug 2026 14:44:45 +0100 Subject: [PATCH 02/13] dofs: reject an apply that runs without write access `applyChanges` and `applyChangesSync` take a `writable` option. With `writable: false` nothing is applied and every entry comes back in `skipped` with reason `no-write-access`. This is the second half of running one command without write access. A backend that writes this store directly is stopped before anything commits, by a filesystem handle built without the capability. A backend that keeps its own copy of the files cannot be stopped that way: it has already written to its own copy by the time the changes arrive, so they stop on the way in instead. That is after the fact rather than preventive, and the two copies diverge as a result. The skipped entries are what tells the caller so, rather than hiding it. The capability is checked ahead of the idempotence skip, so a read-only apply reports every entry it was handed instead of quietly dropping the ones that happened to match local state. It is also checked ahead of the mount-root guard, so an entry both would refuse reports the missing capability, which is the answer the caller asked for. `SkippedEntry` becomes a union discriminated on `reason`. The two refusals carry different information: a read-only mount names the mount that owns the path, and a missing capability has no mount to name because the apply had no write access wherever the entry pointed. Code that read `mountRoot` unconditionally no longer compiles, which is the intent for a field that is now conditional. --- packages/dofs/src/index.ts | 8 +- packages/dofs/src/sync/apply.test.ts | 144 ++++++++++++++++++++++++++- packages/dofs/src/sync/apply.ts | 73 ++++++++++++-- 3 files changed, 213 insertions(+), 12 deletions(-) diff --git a/packages/dofs/src/index.ts b/packages/dofs/src/index.ts index dda80a1b..1d4c148b 100644 --- a/packages/dofs/src/index.ts +++ b/packages/dofs/src/index.ts @@ -41,7 +41,13 @@ export type { SQLiteWorkspaceProviderOptions } from "./provider.js"; export { SQLiteWorkspaceProvider } from "./provider.js"; export { initializeSchema, ROOT_INODE, SCHEMA_VERSION } from "./schema/index.js"; export { Database } from "./storage.js"; -export type { ApplyOptions, ApplyResult, SkippedEntry } from "./sync/apply.js"; +export type { + ApplyOptions, + ApplyResult, + SkippedByMount, + SkippedEntry, + SkippedWithoutWriteAccess, +} from "./sync/apply.js"; // Sync protocol building blocks. The wire wiring lives in // @cloudflare/computer-rpc; these are the helpers that wiring binds // to a Database. diff --git a/packages/dofs/src/sync/apply.test.ts b/packages/dofs/src/sync/apply.test.ts index 5ced0d0e..c5bb5662 100644 --- a/packages/dofs/src/sync/apply.test.ts +++ b/packages/dofs/src/sync/apply.test.ts @@ -821,7 +821,12 @@ describe("applyChanges with read-only mount roots", () => { ); expect(result.applied).toBe(0); - expect(result.skipped[0]?.mountRoot).toBe("/workspace/r2"); + expect(result.skipped[0]).toEqual({ + path: "/workspace/r2", + mountRoot: "/workspace/r2", + op: "write", + reason: "read-only", + }); }); }); @@ -1118,3 +1123,140 @@ describe("applyChanges mtime propagation (auto_cache contract)", () => { ); }); }); + +// An apply that runs without write access is how a read-only command +// is enforced for a backend that keeps its own copy of the files. The +// command has already written to that copy by the time the changes +// reach here, so this is where they stop. Unlike the worker-shell +// lane, this is after the fact rather than preventive, and the skipped +// entries are what tells the caller so. +describe("applyChanges without write access", () => { + const fileEntry = (path: string, rev: number): ChangeEntry => ({ + kind: "file", + rev, + path, + mode: 0o644, + mtime: 1, + size: 0, + chunks: [], + }); + + it("applies nothing and reports every entry it was handed", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace", { recursive: true }, () => 0); + + const result = await applyChanges( + db, + [ + fileEntry("/workspace/a.txt", 100), + { kind: "dir", rev: 101, path: "/workspace/sub", mode: 0o755, mtime: 1 }, + { kind: "delete", rev: 102, path: "/workspace/gone.txt" }, + ], + new Map(), + { writable: false }, + ); + + expect(result.applied).toBe(0); + expect(result.skipped).toEqual([ + { path: "/workspace/a.txt", op: "write", reason: "no-write-access" }, + { path: "/workspace/sub", op: "write", reason: "no-write-access" }, + { path: "/workspace/gone.txt", op: "delete", reason: "no-write-access" }, + ]); + + expect(resolveInode(db, "/workspace/a.txt")).toBeNull(); + expect(resolveInode(db, "/workspace/sub")).toBeNull(); + }); + }); + + // A zero-chunk entry would truncate the file it lands on, so the + // original contents surviving is proof the apply did not run. + it("leaves a file the entry would have truncated untouched", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace", { recursive: true }, () => 0); + writeFileSync(db, "/workspace/a.txt", new TextEncoder().encode("original"), {}, () => 0); + const revBefore = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'"); + + const result = await applyChanges(db, [fileEntry("/workspace/a.txt", 200)], new Map(), { + writable: false, + }); + + expect(result.applied).toBe(0); + expect(result.skipped).toHaveLength(1); + expect(await readFile(db, "/workspace/a.txt", "utf8")).toBe("original"); + // Nothing was written, so the revision did not move. + expect(db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'")).toBe(revBefore); + }); + }); + + it("does not delete a file a tombstone would have removed", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace", { recursive: true }, () => 0); + writeFileSync(db, "/workspace/keep.txt", new TextEncoder().encode("hi"), {}, () => 0); + + const result = await applyChanges( + db, + [{ kind: "delete", rev: 300, path: "/workspace/keep.txt" }], + new Map(), + { writable: false }, + ); + + expect(result.applied).toBe(0); + expect(resolveInode(db, "/workspace/keep.txt")).not.toBeNull(); + }); + }); + + it("defaults to applying so existing callers are unaffected", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace", { recursive: true }, () => 0); + + const result = await applyChanges(db, [fileEntry("/workspace/a.txt", 100)], new Map()); + + expect(result.applied).toBe(1); + expect(result.skipped).toEqual([]); + }); + }); + + // Both guards would refuse this entry. The missing capability is + // reported because it is the reason the caller asked for: the + // command had no write access, whatever the mount says. + it("reports the missing capability rather than the mount when both would refuse", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace/r2", { recursive: true }, () => 0); + db.run( + "INSERT INTO _vfs_mounts (root, kind, indexed, mode) VALUES (?, ?, 1, 'read-only')", + "/workspace/r2", + "test", + ); + invalidateReadOnlyMountCache(db); + + const result = await applyChanges( + db, + [fileEntry("/workspace/r2/hello.txt", 100)], + new Map(), + { + writable: false, + }, + ); + + expect(result.skipped).toEqual([ + { path: "/workspace/r2/hello.txt", op: "write", reason: "no-write-access" }, + ]); + }); + }); + + it("applies nothing through applyChangesSync either", async () => { + await withDB(async (db) => { + mkdir(db, "/workspace", { recursive: true }, () => 0); + + const result = applyChangesSync(db, [fileEntry("/workspace/a.txt", 100)], new Map(), { + writable: false, + }); + + expect(result.applied).toBe(0); + expect(result.skipped).toEqual([ + { path: "/workspace/a.txt", op: "write", reason: "no-write-access" }, + ]); + expect(resolveInode(db, "/workspace/a.txt")).toBeNull(); + }); + }); +}); diff --git a/packages/dofs/src/sync/apply.ts b/packages/dofs/src/sync/apply.ts index 4986c98e..84d628a1 100644 --- a/packages/dofs/src/sync/apply.ts +++ b/packages/dofs/src/sync/apply.ts @@ -12,23 +12,41 @@ import type { Database } from "../storage.js"; import type { ChangeEntry } from "./changes.js"; import { computeManifestHash } from "./manifests.js"; -// One container-side change that landed under a read-only mount and -// was therefore skipped rather than applied. Callers (the workspace -// pull surface, the shell exec bracket) surface these so the user -// learns the mount stayed authoritative. -export interface SkippedEntry { +// One incoming change that was refused rather than applied. Callers +// (the workspace pull surface, the shell exec bracket) surface these +// so the user learns what did not land. +// +// Discriminated on `reason` because the two refusals carry different +// information. A read-only mount names the mount that owns the path, +// which lets a caller group entries by mount. A missing write +// capability has no mount to name: the apply itself had no write +// access, so every entry is refused wherever it pointed. +export type SkippedEntry = SkippedByMount | SkippedWithoutWriteAccess; + +// Common fields. Split out so both members stay in step. +interface SkippedEntryBase { // Absolute VFS path the change targeted. path: string; - // Mount root that owns the path (the one whose mode is - // read-only). Lets callers group skipped entries by mount. - mountRoot: string; // 'write' covers file / dir / symlink create-or-update; 'delete' // covers tombstones. The single field is enough for callers to // decide messaging. op: "write" | "delete"; - // Open shape: future skip reasons can join this union without - // breaking callers that match on 'read-only' today. +} + +// The path falls under a registered read-only mount root. +export interface SkippedByMount extends SkippedEntryBase { reason: "read-only"; + // Mount root that owns the path (the one whose mode is + // read-only). Lets callers group skipped entries by mount. + mountRoot: string; +} + +// The apply ran without write access, so nothing was applied at all. +// This is the after-the-fact half of a read-only command: the sender +// has already written to its own copy of the files, and the changes +// stop here on the way in. +export interface SkippedWithoutWriteAccess extends SkippedEntryBase { + reason: "no-write-access"; } // Return shape of applyChanges / applyChangesSync. Existing callers @@ -68,6 +86,28 @@ export interface ApplyOptions { // which is fine for the container backend the package shipped // with first. backend?: string; + // Write access for this apply. Defaults to true. Pass false and + // nothing is applied: every entry comes back in `skipped` with + // reason 'no-write-access'. + // + // This is how a command that ran without write access is enforced + // for a backend that keeps its own copy of the files. The command + // wrote to that copy already, so refusing here is after the fact + // rather than preventive — the copies diverge, and the skipped + // entries are what tells the caller so. A backend that writes this + // store directly gets the preventive version instead, through a + // filesystem handle built without write access. + writable?: boolean; +} + +// Shape one refused entry for an apply that has no write access. +// Shared by both apply variants so they cannot drift. +function refusedWithoutWriteAccess(entry: ChangeEntry): SkippedWithoutWriteAccess { + return { + path: entry.path, + op: entry.kind === "delete" ? "delete" : "write", + reason: "no-write-access", + }; } const DEFAULT_MAX_BYTES = 64 * 1024 * 1024; @@ -229,6 +269,7 @@ export async function applyChanges( ): Promise { const maxBytes = options.maxBytesPerBatch ?? DEFAULT_MAX_BYTES; const maxPaths = options.maxPathsPerBatch ?? DEFAULT_MAX_PATHS; + const writable = options.writable ?? true; let bytesInBatch = 0; let pathsInBatch = 0; @@ -240,6 +281,13 @@ export async function applyChanges( }; for await (const entry of entries) { + // Write capability. Checked ahead of everything else so a + // read-only apply reports every entry it was handed rather than + // quietly dropping the ones that happened to match local state. + if (writable === false) { + skipped.push(refusedWithoutWriteAccess(entry)); + continue; + } // Idempotent skip: if the entry already matches the local // state, drop it on the floor. The check is what stops a // pull from bumping vfs_meta.rev for entries that are @@ -358,6 +406,7 @@ export function applyChangesSync( ): ApplyResult { const maxBytes = options.maxBytesPerBatch ?? DEFAULT_MAX_BYTES; const maxPaths = options.maxPathsPerBatch ?? DEFAULT_MAX_PATHS; + const writable = options.writable ?? true; let bytesInBatch = 0; let pathsInBatch = 0; @@ -369,6 +418,10 @@ export function applyChangesSync( }; for (const entry of entries) { + if (writable === false) { + skipped.push(refusedWithoutWriteAccess(entry)); + continue; + } if (options.source === "upstream" && entry.kind !== "delete") { if (alreadyApplied(db, entry)) continue; } From 50add9c2c8f214f0f1ac8b38cf0db9f8ce1fa2f3 Mon Sep 17 00:00:00 2001 From: Antoni T Date: Mon, 3 Aug 2026 14:53:26 +0100 Subject: [PATCH 03/13] rpc: carry a write capability on exec and on pull `ShellRPC.exec` takes a `writable` flag and `pullOnce` takes a matching option that it forwards to `applyChanges`. The flag on `exec` is what a caller sets; the option on `pullOnce` is what enforces it for a runner that keeps its own copy of the files. That runner writes to its copy before we hear about the change, so the refusal happens as the change comes back rather than at the write. The interface comment says so plainly instead of implying the flag prevents the write everywhere. Two details that are easy to get wrong in the pull path: The object fetch is skipped without write access. `stageBlob` writes to the local store directly, so fetching bytes for entries about to be refused would write through the missing capability, and pay for the transfer first. The fetch cursor still advances over refused entries. Refusing is discarding, not deferring. Holding the cursor back would hand the same writes to the next pull that does have write access, which would make the refusal a delay rather than a denial. The stream is drained either way, so the caller gets a full account of what was refused rather than a bare count. --- packages/rpc/src/interface.ts | 13 +++ packages/rpc/src/sync-driver.test.ts | 113 +++++++++++++++++++++++++++ packages/rpc/src/sync-driver.ts | 32 +++++++- 3 files changed, 155 insertions(+), 3 deletions(-) diff --git a/packages/rpc/src/interface.ts b/packages/rpc/src/interface.ts index 7eb5bca0..197bc729 100644 --- a/packages/rpc/src/interface.ts +++ b/packages/rpc/src/interface.ts @@ -108,6 +108,19 @@ export interface ShellRPC { env?: Record; // Standard input bytes fed to the command. stdin?: Uint8Array; + // Whether this command may modify the workspace. Defaults to + // true. False means the caller expects the command to only read, + // and asks that any write it attempts fail rather than land. + // + // How completely that holds depends on where the files live. A + // runner working directly against the workspace store gets a + // filesystem handle without write access, so a write fails inside + // the command with EROFS and the command sees the error. A runner + // with its own copy of the files cannot be stopped that way: the + // write lands in its copy, and is refused on the way back, which + // leaves the two copies disagreeing. The refusal is reported + // either way, but only the first shape prevents the write. + writable?: boolean; }): Promise<{ id: string; events: ReadableStream; diff --git a/packages/rpc/src/sync-driver.test.ts b/packages/rpc/src/sync-driver.test.ts index 46c6e294..b1a43b86 100644 --- a/packages/rpc/src/sync-driver.test.ts +++ b/packages/rpc/src/sync-driver.test.ts @@ -1274,6 +1274,119 @@ describe("sync driver — reconcileWatermarks", () => { }); }); +describe("sync driver — pullOnce without write access", () => { + it("applies nothing and reports every entry it was handed", async () => { + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/one.txt", "one"); + providerR.writeFileSync("/two.txt", "two"); + + const result = await pullOnce(local.db, remote.rpc, undefined, { writable: false }); + + expect(result.applied).toBe(0); + expect(result.skipped).toHaveLength(2); + for (const entry of result.skipped) { + expect(entry.reason).toBe("no-write-access"); + } + expect(result.skipped.map((s) => s.path).sort()).toEqual(["/one.txt", "/two.txt"]); + expect(fileEntries(local.db)).toEqual([]); + } finally { + remote.close(); + local.close(); + } + }); + + it("stages no object bytes", async () => { + // The byte fetch is skipped rather than fetched-and-discarded. + // stageBlob writes to the local store directly, so fetching + // bytes for entries we are about to refuse would write through + // the very capability the pull is missing — and pay for the + // transfer to do it. + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/payload.txt", "some bytes worth fetching"); + + await pullOnce(local.db, remote.rpc, undefined, { writable: false }); + + expect(local.db.scalar("SELECT COUNT(*) FROM vfs_blobs")).toBe(0); + } finally { + remote.close(); + local.close(); + } + }); + + it("advances the fetch cursor so refused changes are not redelivered", async () => { + // Refusing is discarding, not deferring. If the cursor stayed + // put, the next pull that *does* have write access would apply + // the writes this one refused, which would make the refusal a + // delay rather than a denial. + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/refused.txt", "refused"); + + await pullOnce(local.db, remote.rpc, undefined, { writable: false }); + expect(readFetchCursor(local.db)).toEqual({ rev: currentRev(remote.db), path: null }); + + const second = await pullOnce(local.db, remote.rpc); + expect(second.applied).toBe(0); + expect(second.skipped).toEqual([]); + expect(fileEntries(local.db)).toEqual([]); + } finally { + remote.close(); + local.close(); + } + }); + + it("applies changes made after write access is restored", async () => { + // The refusal is scoped to the one pull that lacked the + // capability. It does not latch, and it does not poison the + // cursor for later work. + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/refused.txt", "refused"); + await pullOnce(local.db, remote.rpc, undefined, { writable: false }); + + providerR.writeFileSync("/allowed.txt", "allowed"); + const second = await pullOnce(local.db, remote.rpc); + + expect(second.applied).toBe(1); + expect(second.skipped).toEqual([]); + expect(fileEntries(local.db)).toEqual(["allowed.txt"]); + const providerL = new SQLiteWorkspaceProvider(local.db, { now: () => 1 }); + expect(providerL.readFileSync("/allowed.txt", "utf8")).toBe("allowed"); + } finally { + remote.close(); + local.close(); + } + }); + + it("pulls normally when the option is omitted or true", async () => { + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/one.txt", "one"); + + const result = await pullOnce(local.db, remote.rpc, undefined, { writable: true }); + + expect(result.applied).toBe(1); + expect(result.skipped).toEqual([]); + expect(fileEntries(local.db)).toEqual(["one.txt"]); + } finally { + remote.close(); + local.close(); + } + }); +}); + async function sha256(bytes: Uint8Array): Promise { const { createHash } = await import("node:crypto"); const hash = createHash("sha256"); diff --git a/packages/rpc/src/sync-driver.ts b/packages/rpc/src/sync-driver.ts index fba350a5..b9c49fbb 100644 --- a/packages/rpc/src/sync-driver.ts +++ b/packages/rpc/src/sync-driver.ts @@ -66,15 +66,29 @@ const PULL_BATCH_SIZE = 256; // (rev, path), so a retry can resume inside a single large rev. The // cursor is read and written per backend so concurrent backends keep // independent resume points. +// +// `writable: false` pulls the entries and refuses all of them. The +// stream is still drained so the caller learns what the remote tried +// to send, reported as skipped entries with reason no-write-access. +// This is the only enforcement point available against a remote that +// keeps its own copy of the files: it has already written to that +// copy by the time we hear about it. export async function pullOnce( db: Database, remote: SyncRPC, backend?: string, + options?: PullOptions, ): Promise { // Delegate to the inner implementation with retried=false. See // pullOnceImpl for the fetchChanges round trip, invariant check, // reset-and-retry path, and batched apply loop. - return pullOnceImpl(db, remote, backend, false); + return pullOnceImpl(db, remote, backend, false, options?.writable ?? true); +} + +export interface PullOptions { + // Whether this pull may write to the local store. Defaults to + // true. A pull without write access applies nothing. + writable?: boolean; } // Inner pullOnce that knows whether it is already a retry. The @@ -87,6 +101,7 @@ async function pullOnceImpl( remote: SyncRPC, backend: string | undefined, retried: boolean, + writable: boolean, ): Promise { const after = readFetchCursor(db, backend); const localPushRev = readWatermark(db, "pushRev", backend); @@ -164,7 +179,7 @@ async function pullOnceImpl( if (fetchDiverged) { writeFetchCursor(db, { rev: 0, path: null }, backend); } - return pullOnceImpl(db, remote, backend, true); + return pullOnceImpl(db, remote, backend, true, writable); } // After the retry path above, this assertion guards a // divergence that survived a reset. Tear down rather than loop. @@ -211,7 +226,12 @@ async function pullOnceImpl( // Probe + fetch missing chunk bytes for just this batch. Bytes // the receiver already holds (or the remote doesn't have) are // skipped, so the per-batch network cost is bounded. - if (wantedHashes.length > 0) { + // + // Skipped entirely without write access. stageBlob writes to + // the local store, so fetching bytes for entries we are about + // to refuse would write through the capability this pull does + // not have — and pay for the transfer to do it. + if (writable && wantedHashes.length > 0) { const haveSubset = await remote.hasObjects(wantedHashes); const remoteHasLocally = new Set(); for (const h of haveSubset) remoteHasLocally.add(hex(h)); @@ -241,6 +261,7 @@ async function pullOnceImpl( const batchResult = await applyChanges(db, batch, new Map(), { source: "upstream", backend, + writable, }); const last = batch[batch.length - 1]; // Cursor advancement intentionally happens after applyChanges() @@ -248,6 +269,11 @@ async function pullOnceImpl( // checkpoint re-fetches the batch; upstream apply is idempotent // because alreadyApplied() drops entries whose live state // already matches. + // + // The cursor advances over refused entries too. Refusing is + // discarding, not deferring: leaving the cursor behind would + // hand the same writes to the next pull that does have write + // access, turning a denial into a delay. writeFetchCursorIfAhead(db, { rev: last.rev, path: last.path }, backend); totalApplied += batchResult.applied; if (batchResult.skipped.length > 0) { From 27b6631ddc4e0ee7a08ad21f994f4716cbacb197 Mon Sep 17 00:00:00 2001 From: Antoni T Date: Mon, 3 Aug 2026 15:01:32 +0100 Subject: [PATCH 04/13] computer: add a gate seam and an audit seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hooks around a workspace action: `gate.check(action)` runs before it and can refuse it, `audit.record(action, outcome)` runs after and records what happened. Both default to no-ops. `WorkspaceOptions` takes them as `gate` and `audit`. These are deliberately not folded into the existing observer. That contract says an implementation must return the callback's result unchanged so the wrapping is invisible — observability that changes behaviour is a bug. A gate is the opposite, so it gets its own seam with the same shape and the opposite licence, rather than the observer contract being weakened to fit. The two hooks differ in what they can do, matching what they are for. The gate decides, runs first, and sees only the request. The audit records, runs last, and sees the outcome. An audit hook cannot deny anything and its errors are swallowed, because by the time it runs the action has already happened; the alternative is a failed log entry failing the caller's work. A gate may also allow an action with write access withdrawn, which is the answer a policy wants for a command it will run but not trust. Narrowing only: a decision cannot hand out access the action never asked for, or a read-only exec would depend on the gate behaving. A gate that throws propagates rather than being read as a refusal. A gate that could not reach a decision is not a gate that said no, and a caller that cannot distinguish them will eventually treat an outage as permission. `Workspace.fs` is gated too, via a `WorkspaceFilesystem` subclass. That surface writes to the local store without crossing the wire, so gating only `shell.exec` would leave an obvious way around it: deny the command, write the file directly. Reads stay ungated. It is a subclass rather than a wrapper so the handle is still a WorkspaceFilesystem for the mount and think surfaces that take one by type. Filesystem mutations are gated per call, where each call is the whole action. Commands are gated once for the command, and gate.ts explains why: `rm -rf` is hundreds of calls, so refusing partway leaves a half-deleted tree, and there is no safe place to suspend a running command to ask a human. --- packages/computer/src/gate.test.ts | 214 ++++++++++++++++++++++++ packages/computer/src/gate.ts | 214 ++++++++++++++++++++++++ packages/computer/src/gated-fs.ts | 100 +++++++++++ packages/computer/src/index.ts | 12 ++ packages/computer/src/workspace.test.ts | 127 ++++++++++++++ packages/computer/src/workspace.ts | 50 +++++- 6 files changed, 715 insertions(+), 2 deletions(-) create mode 100644 packages/computer/src/gate.test.ts create mode 100644 packages/computer/src/gate.ts create mode 100644 packages/computer/src/gated-fs.ts diff --git a/packages/computer/src/gate.test.ts b/packages/computer/src/gate.test.ts new file mode 100644 index 00000000..8d8db34d --- /dev/null +++ b/packages/computer/src/gate.test.ts @@ -0,0 +1,214 @@ +import { describe, expect, it, vi } from "vitest"; + +import { + ActionDeniedError, + type AuditOutcome, + noopAudit, + openGate, + type WorkspaceAction, + type WorkspaceAudit, + type WorkspaceGate, + withGate, +} from "./gate.js"; + +function execAction(writable = true): WorkspaceAction { + return { kind: "shell.exec", command: "ls", writable }; +} + +function recordingAudit(): WorkspaceAudit & { entries: [WorkspaceAction, AuditOutcome][] } { + const entries: [WorkspaceAction, AuditOutcome][] = []; + return { + entries, + record(action, outcome) { + entries.push([action, outcome]); + }, + }; +} + +function gateReturning(decision: Awaited>): WorkspaceGate { + return { check: () => decision }; +} + +describe("withGate", () => { + it("runs the action and reports it when the gate allows", async () => { + const audit = recordingAudit(); + const result = await withGate(openGate, audit, execAction(), async () => "ran"); + + expect(result).toBe("ran"); + expect(audit.entries).toEqual([[execAction(), { status: "allowed", writable: true }]]); + }); + + it("does not run the action when the gate denies", async () => { + const audit = recordingAudit(); + const run = vi.fn(async () => "ran"); + const gate = gateReturning({ allow: false, reason: "not on the allowlist" }); + + await expect(withGate(gate, audit, execAction(), run)).rejects.toThrow(ActionDeniedError); + expect(run).not.toHaveBeenCalled(); + expect(audit.entries).toEqual([ + [execAction(), { status: "denied", reason: "not on the allowlist" }], + ]); + }); + + it("carries the action and reason on the denial", async () => { + const gate = gateReturning({ allow: false, reason: "needs approval" }); + const action = execAction(); + + const error = await withGate(gate, noopAudit, action, async () => "ran").catch((e) => e); + + expect(error).toBeInstanceOf(ActionDeniedError); + expect(error.action).toEqual(action); + expect(error.reason).toBe("needs approval"); + expect(error.message).toBe("shell.exec denied: needs approval"); + }); + + it("narrows write access when the gate withdraws it", async () => { + // The useful middle answer: run the command, but not with write + // access. The callback must see the narrowed value. + const audit = recordingAudit(); + const gate = gateReturning({ allow: true, writable: false }); + const seen: boolean[] = []; + + await withGate(gate, audit, execAction(true), async (writable) => { + seen.push(writable); + }); + + expect(seen).toEqual([false]); + expect(audit.entries[0][1]).toEqual({ status: "allowed", writable: false }); + }); + + it("cannot widen write access the action did not ask for", async () => { + // A gate is a restriction, not a grant. Allowing it to hand out + // access the caller never requested would make a read-only exec + // depend on the gate's good behaviour. + const audit = recordingAudit(); + const gate = gateReturning({ allow: true, writable: true }); + const seen: boolean[] = []; + + await withGate(gate, audit, execAction(false), async (writable) => { + seen.push(writable); + }); + + expect(seen).toEqual([false]); + expect(audit.entries[0][1]).toEqual({ status: "allowed", writable: false }); + }); + + it("waits for an async gate before running the action", async () => { + const order: string[] = []; + const gate: WorkspaceGate = { + async check() { + order.push("gate-start"); + await new Promise((resolve) => setTimeout(resolve, 5)); + order.push("gate-end"); + return { allow: true }; + }, + }; + + await withGate(gate, noopAudit, execAction(), async () => { + order.push("action"); + }); + + expect(order).toEqual(["gate-start", "gate-end", "action"]); + }); + + it("propagates a gate that throws instead of treating it as a denial", async () => { + // A gate that could not reach a decision is not a gate that said + // no. Collapsing the two would let an outage read as permission + // in one direction or as a policy refusal in the other. + const failure = new Error("policy service unreachable"); + const gate: WorkspaceGate = { + check() { + throw failure; + }, + }; + const run = vi.fn(async () => "ran"); + + await expect(withGate(gate, noopAudit, execAction(), run)).rejects.toBe(failure); + expect(run).not.toHaveBeenCalled(); + }); + + it("reports a failed action and rethrows the original error", async () => { + const audit = recordingAudit(); + const failure = new Error("command blew up"); + + await expect( + withGate(openGate, audit, execAction(), async () => { + throw failure; + }), + ).rejects.toBe(failure); + expect(audit.entries).toEqual([[execAction(), { status: "failed", error: failure }]]); + }); + + it("does not let a throwing audit hook change a successful outcome", async () => { + const audit: WorkspaceAudit = { + record() { + throw new Error("log sink down"); + }, + }; + + await expect(withGate(openGate, audit, execAction(), async () => "ran")).resolves.toBe("ran"); + }); + + it("does not let a rejecting audit hook change a successful outcome", async () => { + const audit: WorkspaceAudit = { + record() { + return Promise.reject(new Error("log sink down")); + }, + }; + + await expect(withGate(openGate, audit, execAction(), async () => "ran")).resolves.toBe("ran"); + }); + + it("still denies when the audit hook throws on a denial", async () => { + const audit: WorkspaceAudit = { + record() { + throw new Error("log sink down"); + }, + }; + const gate = gateReturning({ allow: false }); + + await expect(withGate(gate, audit, execAction(), async () => "ran")).rejects.toThrow( + ActionDeniedError, + ); + }); + + it("treats filesystem actions as write actions regardless of decision shape", async () => { + const audit = recordingAudit(); + const action: WorkspaceAction = { kind: "fs.rm", path: "/workspace/a.txt" }; + + await withGate(openGate, audit, action, async (writable) => { + expect(writable).toBe(true); + }); + + expect(audit.entries).toEqual([[action, { status: "allowed", writable: true }]]); + }); + + it("gates each filesystem call separately", async () => { + // Per-call gating is safe for fs actions because each call is the + // whole action. This is the contrast with shell.exec, which is + // gated once for the command. + const seen: string[] = []; + const gate: WorkspaceGate = { + check(action) { + if (action.kind !== "shell.exec") seen.push(`${action.kind}:${action.path}`); + return { allow: true }; + }, + }; + + await withGate(gate, noopAudit, { kind: "fs.write", path: "/a", size: 1 }, async () => {}); + await withGate(gate, noopAudit, { kind: "fs.rm", path: "/b" }, async () => {}); + + expect(seen).toEqual(["fs.write:/a", "fs.rm:/b"]); + }); + + it("passes the open gate and noop audit through without allocating a decision per call", async () => { + // The default path should be cheap: the point of the no-op + // defaults is that a caller who does not opt in pays nothing + // beyond a comparison. + const first = await openGate.check(execAction()); + const second = await openGate.check(execAction()); + + expect(first).toBe(second); + expect(noopAudit.record(execAction(), { status: "allowed", writable: true })).toBeUndefined(); + }); +}); diff --git a/packages/computer/src/gate.ts b/packages/computer/src/gate.ts new file mode 100644 index 00000000..0c6c036b --- /dev/null +++ b/packages/computer/src/gate.ts @@ -0,0 +1,214 @@ +/** + * Approval seams: a gate that runs before an action and an audit hook + * that runs after it. + * + * These exist because the observer in `./observe.ts` deliberately + * cannot do this job. Its contract says an implementation "must run + * `run` synchronously and return its result unchanged so the wrapping + * is invisible to the caller" — observability that changes behaviour + * is a bug. A gate is the opposite: its whole purpose is to refuse. + * Rather than weaken the observer contract, this is a second seam with + * the same shape and the opposite licence. + * + * The split into two hooks matches what each one is for. The gate + * decides, so it runs before the action and sees only the request. The + * audit records, so it runs after and sees the outcome. An audit hook + * cannot deny anything; if it throws, the action has already happened + * and the throw is swallowed, exactly as `withSpan` treats a failing + * `finalize`. + * + * Both default to no-ops, so a caller who does not opt in pays one + * comparison per action. + * + * ## Why a command is gated once, not per write + * + * The gate fires once for a whole `shell.exec`, and the decision + * covers every write that command makes. It is tempting to instead + * check each write as it happens, but that produces worse outcomes + * than refusing the command outright: `rm -rf` issues one call per + * entry, so denying halfway leaves a half-deleted tree. A command is + * the smallest unit that can be refused and leave the workspace in a + * state the caller can reason about. + * + * The same reasoning rules out asking a human mid-command. By the time + * a write arrives the command is running, and there is nowhere to + * suspend it that does not risk a partial result. A gate that wants + * human approval must ask before the command starts, which is where + * this one runs. + * + * Filesystem actions are gated per call, because there each call is + * the whole action and refusing one leaves nothing half-done. + */ + +/** + * What is about to happen, in enough detail for a gate to decide. + * + * Discriminated on `kind`. A gate that only cares about one kind + * should switch on it and return `allowed` for the rest, so new kinds + * added later stay permitted rather than silently breaking callers who + * were never asked about them. + */ +export type WorkspaceAction = + | { + kind: "shell.exec"; + // The command line as it will be handed to the runner. + command: string; + cwd?: string; + // Whether the caller is asking for write access. A gate can + // narrow this to false in its decision; it cannot widen it. + writable: boolean; + // Which registered backend will run this, once resolved. + backend?: string; + } + | { + kind: "fs.write" | "fs.mkdir" | "fs.rm" | "fs.chmod" | "fs.symlink"; + // Absolute path inside the workspace. + path: string; + // Byte length for a write, when known. Absent for the others. + size?: number; + }; + +/** The action kinds, for callers that want to switch exhaustively. */ +export type WorkspaceActionKind = WorkspaceAction["kind"]; + +/** + * A gate's answer. + * + * `allow: true` may carry `writable: false` to permit the action with + * write access withdrawn — the useful middle answer for a command a + * policy is willing to run but not to trust with the workspace. A gate + * can only narrow write access this way. Passing `writable: true` for + * an action that did not ask for it does not grant it. + * + * `allow: false` refuses. `reason` is surfaced to the caller and + * should be phrased for whoever asked for the action, since that is + * who will read it. + */ +export type GateDecision = { allow: true; writable?: boolean } | { allow: false; reason?: string }; + +/** + * Consulted before an action runs. + * + * May be async: a gate is allowed to consult a policy service or wait + * for a human, and the action does not start until it settles. Keep in + * mind that whatever it waits on holds up the caller. + * + * A gate that throws denies the action. The error propagates to the + * caller unchanged rather than being converted into a refusal, because + * a gate that failed to reach a decision is not the same as a gate + * that decided no, and a caller who cannot tell those apart will + * eventually treat an outage as permission. + */ +export interface WorkspaceGate { + check(action: WorkspaceAction): Promise | GateDecision; +} + +/** The outcome of a gated action, as reported to an audit hook. */ +export type AuditOutcome = + | { status: "allowed"; writable: boolean } + | { status: "denied"; reason?: string } + // The action ran and threw. `error` is whatever it threw. + | { status: "failed"; error: unknown }; + +/** + * Notified after an action has been decided and, if allowed, run. + * + * Fires for denied actions too — a record of what was refused is + * usually the more interesting half. Errors thrown here are + * swallowed: the action has already happened, and failing the caller + * over a failed log entry would turn an audit hook into a gate. + */ +export interface WorkspaceAudit { + record(action: WorkspaceAction, outcome: AuditOutcome): void | Promise; +} + +/** Gate that permits everything, used when the caller passes none. */ +export const openGate: WorkspaceGate = { + check() { + return ALLOWED; + }, +}; + +const ALLOWED: GateDecision = { allow: true }; + +/** Audit hook that records nothing, used when the caller passes none. */ +export const noopAudit: WorkspaceAudit = { + record() { + // intentionally empty + }, +}; + +/** + * Thrown when a gate refuses an action. + * + * Carries the action so a caller catching it can report what was + * refused without having to remember what it asked for. + */ +export class ActionDeniedError extends Error { + readonly action: WorkspaceAction; + readonly reason?: string; + + constructor(action: WorkspaceAction, reason?: string) { + super(reason ? `${action.kind} denied: ${reason}` : `${action.kind} denied`); + this.name = "ActionDeniedError"; + this.action = action; + this.reason = reason; + } +} + +/** + * Internal helper: consult `gate`, run `action` if allowed, and report + * the outcome to `audit`. Mirrors `withSpan` in `./observe.ts` so the + * two seams read the same way at a call site that uses both. + * + * `run` receives the effective write access, which is the requested + * access narrowed by whatever the gate decided. Call sites should use + * that argument rather than the flag they passed in, since ignoring it + * is how a narrowed decision gets quietly dropped. + * + * Throws `ActionDeniedError` when the gate refuses. Nothing runs in + * that case. + */ +export async function withGate( + gate: WorkspaceGate, + audit: WorkspaceAudit, + action: WorkspaceAction, + run: (writable: boolean) => Promise, +): Promise { + const requested = action.kind === "shell.exec" ? action.writable : true; + const decision = await gate.check(action); + + if (!decision.allow) { + await reportSafely(audit, action, { status: "denied", reason: decision.reason }); + throw new ActionDeniedError(action, decision.reason); + } + + // The gate can narrow write access but not widen it, so a decision + // asking for more than the action requested is capped rather than + // honoured. + const writable = requested && (decision.writable ?? true); + + try { + const value = await run(writable); + await reportSafely(audit, action, { status: "allowed", writable }); + return value; + } catch (error) { + await reportSafely(audit, action, { status: "failed", error }); + throw error; + } +} + +// An audit hook that throws must not change the outcome of work that +// has already happened. Rejections are swallowed here for the same +// reason withSpan swallows a failing finalize. +async function reportSafely( + audit: WorkspaceAudit, + action: WorkspaceAction, + outcome: AuditOutcome, +): Promise { + try { + await audit.record(action, outcome); + } catch { + // not actionable: the action's outcome is already decided + } +} diff --git a/packages/computer/src/gated-fs.ts b/packages/computer/src/gated-fs.ts new file mode 100644 index 00000000..ba2a92f9 --- /dev/null +++ b/packages/computer/src/gated-fs.ts @@ -0,0 +1,100 @@ +/** + * A `WorkspaceFilesystem` whose mutating methods go through a gate. + * + * `Workspace.fs` hands callers a filesystem handle that writes to the + * local store directly, without crossing the wire. That makes it the + * other way into the workspace besides `shell.exec`, and a gate that + * only covered exec would be trivial to walk around — deny the + * command, write the file yourself. Gating both closes that. + * + * A subclass rather than a wrapper object, so the handle stays a + * `WorkspaceFilesystem` for every caller that already has one and for + * the mount and think surfaces that take one by type. Reads are not + * overridden and cost nothing. + * + * Each mutation is gated on its own. That is safe here in a way it is + * not for a shell command: one `writeFile` is the entire action, so + * refusing it leaves nothing half-finished. See the note in `./gate.ts` + * for why a command is gated once instead. + */ + +import type { Database } from "@cloudflare/dofs"; +import { + type MkdirOptions, + type RmOptions, + WorkspaceFilesystem, + type WorkspaceFilesystemOptions, + type WriteFileContent, + type WriteFileOptions, +} from "@cloudflare/dofs"; + +import { type WorkspaceAudit, type WorkspaceGate, withGate } from "./gate.js"; + +export interface GatedFilesystemOptions extends WorkspaceFilesystemOptions { + gate: WorkspaceGate; + audit: WorkspaceAudit; +} + +export class GatedWorkspaceFilesystem extends WorkspaceFilesystem { + readonly #gate: WorkspaceGate; + readonly #audit: WorkspaceAudit; + + constructor(db: Database, options: GatedFilesystemOptions) { + super(db, options); + this.#gate = options.gate; + this.#audit = options.audit; + } + + override async writeFile( + path: string, + content: WriteFileContent, + options: WriteFileOptions = {}, + ): Promise { + // Only reported when it is already known. Measuring a stream to + // fill in the field would consume it. + const size = byteLength(content); + return withGate(this.#gate, this.#audit, { kind: "fs.write", path, size }, () => + super.writeFile(path, content, options), + ); + } + + override async mkdir(path: string, options: MkdirOptions = {}): Promise { + return withGate(this.#gate, this.#audit, { kind: "fs.mkdir", path }, () => + super.mkdir(path, options), + ); + } + + override async rm(path: string, options: RmOptions = {}): Promise { + // One gate call for the whole tree when recursive. The gate sees + // the path the caller named, which is the decision it can + // actually make; asking per descendant would raise the same + // half-deleted-tree problem that shell commands have. + return withGate(this.#gate, this.#audit, { kind: "fs.rm", path }, () => + super.rm(path, options), + ); + } + + override async chmod(path: string, mode: number): Promise { + return withGate(this.#gate, this.#audit, { kind: "fs.chmod", path }, () => + super.chmod(path, mode), + ); + } + + override async symlink(target: string, path: string): Promise { + // Gated on the link's own path, not the target. Creating a link + // writes at `path`; the target may not exist and is allowed to + // dangle, so it is not the thing being modified. + return withGate(this.#gate, this.#audit, { kind: "fs.symlink", path }, () => + super.symlink(target, path), + ); + } +} + +// A stream has no length until it is read, and reading it here would +// consume the bytes the write needs. Those calls reach the gate with +// `size` absent rather than with a wrong number. +function byteLength(content: WriteFileContent): number | undefined { + if (typeof content === "string") return new TextEncoder().encode(content).byteLength; + if (content instanceof Uint8Array) return content.byteLength; + return undefined; +} diff --git a/packages/computer/src/index.ts b/packages/computer/src/index.ts index 2d521050..74c4070a 100644 --- a/packages/computer/src/index.ts +++ b/packages/computer/src/index.ts @@ -34,6 +34,18 @@ export { type WorkspaceRuntimeClient, } from "./client.js"; export { decodeExecEvents, encodeExecEvents } from "./exec-wire.js"; +export { + ActionDeniedError, + type AuditOutcome, + type GateDecision, + noopAudit, + openGate, + type WorkspaceAction, + type WorkspaceActionKind, + type WorkspaceAudit, + type WorkspaceGate, +} from "./gate.js"; +export { type GatedFilesystemOptions, GatedWorkspaceFilesystem } from "./gated-fs.js"; export { R2Bucket, type R2BucketBinding, type R2BucketOptions } from "./mounts/providers/r2.js"; export type { EagerMount, diff --git a/packages/computer/src/workspace.test.ts b/packages/computer/src/workspace.test.ts index 467d7fee..72540ed4 100644 --- a/packages/computer/src/workspace.test.ts +++ b/packages/computer/src/workspace.test.ts @@ -1,7 +1,14 @@ +import { WorkspaceFilesystem } from "@cloudflare/dofs"; import { SQLiteTestStorage } from "@cloudflare/dofs/testing"; import { describe, expect, it, vi } from "vitest"; import type { BackendHandle, WorkspaceBackend } from "./backend.js"; +import { + ActionDeniedError, + type WorkspaceAction, + type WorkspaceAudit, + type WorkspaceGate, +} from "./gate.js"; import { createGitClient } from "./git/index.js"; import type { WorkspaceModuleBackend } from "./runtime/types.js"; import { WorkspaceTransportError } from "./transport-failure.js"; @@ -1509,3 +1516,123 @@ describe("Workspace Think compatibility", () => { expect(ws).not.toHaveProperty("stat"); }); }); + +describe("Workspace gate — filesystem facade", () => { + function gateDenying(kinds: string[]): { + gate: WorkspaceGate; + seen: WorkspaceAction[]; + } { + const seen: WorkspaceAction[] = []; + return { + seen, + gate: { + check(action) { + seen.push(action); + return kinds.includes(action.kind) + ? { allow: false, reason: "denied by policy" } + : { allow: true }; + }, + }, + }; + } + + it("permits everything when no gate is configured", async () => { + const ws = new Workspace({ storage: makeStorage() }); + await ws.fs.writeFile("/a.txt", "hello"); + expect(await ws.fs.readFile("/a.txt", "utf8")).toBe("hello"); + }); + + it("refuses a write the gate denies, and does not write it", async () => { + // Workspace.fs writes to the local store without crossing the + // wire, so a gate that only covered shell.exec could be walked + // around by writing the file directly. + const { gate, seen } = gateDenying(["fs.write"]); + const ws = new Workspace({ storage: makeStorage(), gate }); + + await expect(ws.fs.writeFile("/blocked.txt", "nope")).rejects.toThrow(ActionDeniedError); + await expect(ws.fs.readFile("/blocked.txt", "utf8")).rejects.toThrow(); + expect(seen).toEqual([{ kind: "fs.write", path: "/blocked.txt", size: 4 }]); + }); + + it("leaves reads ungated", async () => { + const { gate, seen } = gateDenying(["fs.write", "fs.mkdir", "fs.rm"]); + const ws = new Workspace({ storage: makeStorage() }); + await ws.fs.writeFile("/readable.txt", "content"); + + const gated = new Workspace({ storage: makeStorage(), gate }); + await expect(gated.fs.readFile("/missing.txt", "utf8")).rejects.toThrow(); + // The read reached the store and failed there, not at the gate. + expect(seen).toEqual([]); + }); + + it("gates mkdir, rm, chmod and symlink", async () => { + const { gate, seen } = gateDenying([]); + const ws = new Workspace({ storage: makeStorage(), gate }); + + await ws.fs.mkdir("/dir"); + await ws.fs.writeFile("/dir/f.txt", "x"); + await ws.fs.chmod("/dir/f.txt", 0o600); + await ws.fs.symlink("/dir/f.txt", "/link"); + await ws.fs.rm("/dir", { recursive: true }); + + expect(seen.map((a) => a.kind)).toEqual([ + "fs.mkdir", + "fs.write", + "fs.chmod", + "fs.symlink", + "fs.rm", + ]); + }); + + it("gates a recursive rm once, on the path the caller named", async () => { + // Not once per descendant: a gate cannot usefully refuse half a + // tree, and asking it to would reintroduce the partial-delete + // problem the design avoids. + const { gate, seen } = gateDenying([]); + const ws = new Workspace({ storage: makeStorage(), gate }); + await ws.fs.mkdir("/tree/nested", { recursive: true }); + await ws.fs.writeFile("/tree/nested/a.txt", "a"); + await ws.fs.writeFile("/tree/nested/b.txt", "b"); + seen.length = 0; + + await ws.fs.rm("/tree", { recursive: true }); + + expect(seen).toEqual([{ kind: "fs.rm", path: "/tree" }]); + }); + + it("reports each mutation to the audit hook, including refusals", async () => { + const entries: [string, string][] = []; + const audit: WorkspaceAudit = { + record(action, outcome) { + entries.push([action.kind, outcome.status]); + }, + }; + const { gate } = gateDenying(["fs.rm"]); + const ws = new Workspace({ storage: makeStorage(), gate, audit }); + + await ws.fs.writeFile("/a.txt", "a"); + await ws.fs.rm("/a.txt").catch(() => {}); + + expect(entries).toEqual([ + ["fs.write", "allowed"], + ["fs.rm", "denied"], + ]); + }); + + it("stays a WorkspaceFilesystem so mount and think surfaces are unaffected", async () => { + const ws = new Workspace({ storage: makeStorage(), gate: gateDenying([]).gate }); + expect(ws.fs).toBeInstanceOf(WorkspaceFilesystem); + }); + + it("reports no size for a streamed write rather than a wrong one", async () => { + // Measuring the stream would consume the bytes the write needs. + const { gate, seen } = gateDenying([]); + const ws = new Workspace({ storage: makeStorage(), gate }); + const stream = new Blob(["streamed"]).stream(); + + await ws.fs.writeFile("/streamed.txt", stream); + + expect(seen).toEqual([{ kind: "fs.write", path: "/streamed.txt", size: undefined }]); + expect(await ws.fs.readFile("/streamed.txt", "utf8")).toBe("streamed"); + }); +}); diff --git a/packages/computer/src/workspace.ts b/packages/computer/src/workspace.ts index e75ada0f..32ee56eb 100644 --- a/packages/computer/src/workspace.ts +++ b/packages/computer/src/workspace.ts @@ -16,7 +16,7 @@ import { type DurableObjectStorageLike, initializeSchema, SQLiteWorkspaceProvider, - WorkspaceFilesystem, + type WorkspaceFilesystem, } from "@cloudflare/dofs"; import { @@ -27,6 +27,8 @@ import { } from "./artifacts/index.js"; import type { AssetsClient } from "./assets/index.js"; import type { BackendHandle, WorkspaceBackend } from "./backend.js"; +import { noopAudit, openGate, type WorkspaceAudit, type WorkspaceGate } from "./gate.js"; +import { GatedWorkspaceFilesystem } from "./gated-fs.js"; import type { GitClient, GitClientFactory, GitIdentity } from "./git/index.js"; import { MountIndex } from "./mounts/index.js"; import { buildMountRegistry, type MountValue } from "./mounts/registry.js"; @@ -118,6 +120,21 @@ export interface WorkspaceOptions { // adapter subpaths for the Cloudflare runtime and OpenTelemetry. observer?: WorkspaceObserver; + // Consulted before each gated action — one call per shell.exec and + // one per mutating Workspace.fs call — and able to refuse it or to + // withdraw its write access. The default permits everything, so the + // package behaves exactly as before for callers who do not opt in. + // + // This is a separate seam from `observer` on purpose: an observer + // must not change what happens, and a gate exists to. See + // `./gate.ts`. + gate?: WorkspaceGate; + + // Notified after each gated action with what was decided and how it + // turned out, including the ones that were refused. Cannot deny + // anything; errors it throws are swallowed. + audit?: WorkspaceAudit; + // Optional durable retry boundary for failed post-command pulls. // The host persists one intent per backend and wakes the Durable // Object at intent.notBefore to call retryPendingSync(backend). @@ -218,6 +235,8 @@ export class Workspace { readonly #defaultBackendId: string | undefined; readonly #defaultCommandBackendId: string | undefined; readonly #observer: WorkspaceObserver; + readonly #gate: WorkspaceGate; + readonly #audit: WorkspaceAudit; readonly #now: () => number; readonly #waitUntil: ((promise: Promise) => void) | undefined; readonly #retryScheduler: SyncRetryScheduler | undefined; @@ -299,7 +318,17 @@ export class Workspace { : createDisabledArtifactsClient(); this.#db = new Database(options.storage); initializeSchema(this.#db, this.#now); - this.#fs = new WorkspaceFilesystem(this.#db, { now: this.#now }); + this.#gate = options.gate ?? openGate; + this.#audit = options.audit ?? noopAudit; + // The gated subclass is still a WorkspaceFilesystem, so the mount + // index, the think surface, and every caller holding `fs` are + // unaffected. With the default open gate the override adds one + // comparison per mutation. + this.#fs = new GatedWorkspaceFilesystem(this.#db, { + now: this.#now, + gate: this.#gate, + audit: this.#audit, + }); const registered = (options.backends ?? []).slice(); if (registered.some((backend) => isModuleBackend(backend) && backend.requiresWaitUntil)) { if (!options.waitUntil) { @@ -376,6 +405,17 @@ export class Workspace { return this.#observer; } + // Gate and audit hook, exposed for the same reason as the observer: + // the shell facade gates its own entry point and needs the pair the + // constructor was given rather than a second set of defaults. + get gate(): WorkspaceGate { + return this.#gate; + } + + get audit(): WorkspaceAudit { + return this.#audit; + } + // Filesystem facade — the documented Workspace.fs surface from // docs/04. Available immediately; doesn't need ready() because // reads and writes hit the local store, not the wire. @@ -386,6 +426,12 @@ export class Workspace { // wrapper. The same check fires on the apply path used by // pullOnce, so container-side writes under a read-only mount are // also rejected (and surfaced via Workspace.pull's skipped[]). + // + // The handle is a GatedWorkspaceFilesystem, so mutations also pass + // the configured gate. Mount enforcement and the gate answer + // different questions — a mount root is a fixed property of the + // workspace, a gate is a decision per action — and both apply. + // Neither can be bypassed by going through the other. get fs(): WorkspaceFilesystem { return this.#fs; } From c24f027574c72a5770b219347e04318af96eab2a Mon Sep 17 00:00:00 2001 From: Antoni T Date: Mon, 3 Aug 2026 15:09:03 +0100 Subject: [PATCH 05/13] computer: thread write access from exec down to the store `ExecOptions.writable` and `WorkspaceRuntimeExecOptions.writable`, defaulting to true. The flag reaches the runner over RPC and travels with the command's own post-command pull, and `Workspace.pull` takes a matching option. The pull is the part that is easy to leave out and useless to omit. The bracket around a command pulls whatever the command produced; a read-only command whose pull still had write access would have exactly its unauthorised changes applied a moment after it finished. The flag travels with the command rather than being read from configuration so that overlapping commands cannot borrow each other's access. `shell.exec` is also where the gate is consulted, before the pre-exec push, so a refused command moves no data either. The audit hook fires on the spawn rather than on the exit. exec() returns a detached handle that the caller may never drain, so there is no later moment guaranteed to arrive; picking one would mean a command that is dropped is never audited. What the command went on to do is already on the observer's span and on ExecResult. The routed shell now forwards the backend id it resolved instead of dropping the caller's selector. A gate deciding whether to trust a command with write access wants to know which backend runs it, because that determines whether a refused write is prevented or only reported. --- packages/computer/src/runtime/runtime.ts | 1 + packages/computer/src/runtime/types.ts | 4 + packages/computer/src/shell.test.ts | 225 +++++++++++++++++++++++ packages/computer/src/shell.ts | 92 ++++++++- packages/computer/src/workspace.ts | 44 +++-- 5 files changed, 348 insertions(+), 18 deletions(-) diff --git a/packages/computer/src/runtime/runtime.ts b/packages/computer/src/runtime/runtime.ts index b426a214..db2c75f7 100644 --- a/packages/computer/src/runtime/runtime.ts +++ b/packages/computer/src/runtime/runtime.ts @@ -67,6 +67,7 @@ export class WorkspaceRuntime { timeoutMs: options.timeoutMs, env: options.env, stdin: options.stdin, + writable: options.writable, }); return wrapCommandHandle(handle, backend); } diff --git a/packages/computer/src/runtime/types.ts b/packages/computer/src/runtime/types.ts index 52d505ce..82f03004 100644 --- a/packages/computer/src/runtime/types.ts +++ b/packages/computer/src/runtime/types.ts @@ -111,6 +111,10 @@ export interface WorkspaceRuntimeExecOptions env?: Record; stdin?: Uint8Array | string; timeoutMs?: number; + // Whether this execution may modify the workspace. Defaults to + // true. Narrows the backend's own access rather than widening it: + // a backend registered read-only stays read-only regardless. + writable?: boolean; } export interface WorkspaceRuntimeGetOptions { diff --git a/packages/computer/src/shell.test.ts b/packages/computer/src/shell.test.ts index 99f937b5..f18c50c6 100644 --- a/packages/computer/src/shell.test.ts +++ b/packages/computer/src/shell.test.ts @@ -8,6 +8,13 @@ import type { ExecEvent, ShellRPC, SyncRPC, WorkspaceRPC } from "@cloudflare/computer-rpc"; import { describe, expect, it } from "vitest"; +import { + ActionDeniedError, + type AuditOutcome, + type WorkspaceAction, + type WorkspaceAudit, + type WorkspaceGate, +} from "./gate.js"; import type { KillSignal } from "./shell.js"; import { type Sync, WorkspaceShell } from "./shell.js"; @@ -46,6 +53,7 @@ interface ExecCall { id: string | undefined; cwd: string | undefined; timeoutMs: number | undefined; + writable: boolean | undefined; } interface GetExecCall { @@ -133,6 +141,7 @@ function fakeRpc(options: FakeRpcOptions = {}): FakeRpc { id: input.id, cwd: input.cwd, timeoutMs: input.timeoutMs, + writable: input.writable, }); if (options.throwOnExec !== undefined) throw options.throwOnExec; const id = input.id ?? mintedId; @@ -900,3 +909,219 @@ function liveRpc(): { }, }; } + +// --------------------------------------------------------------------------- +// exec() — write access and the gate +// --------------------------------------------------------------------------- + +describe("WorkspaceShell.exec — write access", () => { + function recordingSync(): Sync & { pulls: (boolean | undefined)[] } { + const pulls: (boolean | undefined)[] = []; + return { + pulls, + async push() { + return 0; + }, + async pull(options) { + pulls.push(options?.writable); + return applied(0); + }, + }; + } + + it("sends writable: true when the caller says nothing", async () => { + const fake = fakeRpc(); + const shell = new WorkspaceShell(fake.rpc.shell, makeSync()); + await (await shell.exec("ls")).result(); + expect(fake.calls.exec[0].writable).toBe(true); + }); + + it("forwards writable: false to the runner", async () => { + const fake = fakeRpc(); + const shell = new WorkspaceShell(fake.rpc.shell, makeSync()); + await (await shell.exec("ls", { writable: false })).result(); + expect(fake.calls.exec[0].writable).toBe(false); + }); + + it("carries the command's write access into its own post-command pull", async () => { + // Without this the bracket after the command would apply exactly + // the changes the command was not allowed to make. + const fake = fakeRpc(); + const sync = recordingSync(); + const shell = new WorkspaceShell(fake.rpc.shell, sync); + + await (await shell.exec("ls", { writable: false })).result(); + + expect(sync.pulls).toEqual([false]); + }); + + it("pulls with write access for an ordinary command", async () => { + const fake = fakeRpc(); + const sync = recordingSync(); + const shell = new WorkspaceShell(fake.rpc.shell, sync); + + await (await shell.exec("ls")).result(); + + expect(sync.pulls).toEqual([true]); + }); + + it("consults the gate once per command, before the push", async () => { + const order: string[] = []; + const fake = fakeRpc(); + const sync: Sync = { + async push() { + order.push("push"); + return 0; + }, + async pull() { + order.push("pull"); + return applied(0); + }, + }; + const gate: WorkspaceGate = { + check(action) { + order.push(`gate:${action.kind}`); + return { allow: true }; + }, + }; + const shell = new WorkspaceShell(fake.rpc.shell, sync, undefined, { gate }); + + await (await shell.exec("ls")).result(); + + expect(order).toEqual(["gate:shell.exec", "push", "pull"]); + }); + + it("does not spawn or push when the gate denies", async () => { + const fake = fakeRpc(); + const sync = recordingSync(); + const gate: WorkspaceGate = { + check: () => ({ allow: false, reason: "not allowed" }), + }; + const shell = new WorkspaceShell(fake.rpc.shell, sync, undefined, { gate }); + + await expect(shell.exec("rm -rf /")).rejects.toThrow(ActionDeniedError); + expect(fake.calls.exec).toEqual([]); + expect(sync.pulls).toEqual([]); + }); + + it("runs a command the gate allowed but withdrew write access from", async () => { + // The middle answer: the command runs, and neither the runner nor + // the pull that follows it gets write access. + const fake = fakeRpc(); + const sync = recordingSync(); + const gate: WorkspaceGate = { + check: () => ({ allow: true, writable: false }), + }; + const shell = new WorkspaceShell(fake.rpc.shell, sync, undefined, { gate }); + + await (await shell.exec("git log")).result(); + + expect(fake.calls.exec[0].writable).toBe(false); + expect(sync.pulls).toEqual([false]); + }); + + it("shows the gate the command and its requested write access", async () => { + const seen: WorkspaceAction[] = []; + const fake = fakeRpc(); + const gate: WorkspaceGate = { + check(action) { + seen.push(action); + return { allow: true }; + }, + }; + const shell = new WorkspaceShell(fake.rpc.shell, makeSync(), undefined, { gate }); + + await (await shell.exec("cat f", { cwd: "/workspace/sub", writable: false })).result(); + + expect(seen).toEqual([ + { + kind: "shell.exec", + command: "cat f", + cwd: "/workspace/sub", + writable: false, + backend: undefined, + }, + ]); + }); + + it("reports the spawn to the audit hook with the effective write access", async () => { + const entries: AuditOutcome[] = []; + const fake = fakeRpc(); + const audit: WorkspaceAudit = { + record(_action, outcome) { + entries.push(outcome); + }, + }; + const gate: WorkspaceGate = { check: () => ({ allow: true, writable: false }) }; + const shell = new WorkspaceShell(fake.rpc.shell, makeSync(), undefined, { gate, audit }); + + await (await shell.exec("ls")).result(); + + expect(entries).toEqual([{ status: "allowed", writable: false }]); + }); + + it("reports a refused command to the audit hook", async () => { + const entries: [string, AuditOutcome][] = []; + const fake = fakeRpc(); + const audit: WorkspaceAudit = { + record(action, outcome) { + entries.push([action.kind, outcome]); + }, + }; + const gate: WorkspaceGate = { check: () => ({ allow: false, reason: "no" }) }; + const shell = new WorkspaceShell(fake.rpc.shell, makeSync(), undefined, { gate, audit }); + + await shell.exec("rm -rf /").catch(() => {}); + + expect(entries).toEqual([["shell.exec", { status: "denied", reason: "no" }]]); + }); + + it("reports a failed spawn to the audit hook", async () => { + const failure = new Error("spawn failed"); + const entries: AuditOutcome[] = []; + const fake = fakeRpc({ throwOnExec: failure }); + const audit: WorkspaceAudit = { + record(_action, outcome) { + entries.push(outcome); + }, + }; + const shell = new WorkspaceShell(fake.rpc.shell, makeSync(), undefined, { audit }); + + await expect(shell.exec("ls")).rejects.toBe(failure); + expect(entries).toEqual([{ status: "failed", error: failure }]); + }); + + it("surfaces entries the read-only pull refused on the result", async () => { + // The reporting path a caller actually reads: a command that ran + // without write access, against a backend that wrote to its own + // copy anyway, comes back with those entries in skipped. + const fake = fakeRpc(); + const sync: Sync = { + async push() { + return 0; + }, + async pull(options) { + if (options?.writable === false) { + return { + applied: 0, + skipped: [{ path: "/workspace/out.txt", rev: 4, reason: "no-write-access" as const }], + }; + } + return applied(1); + }, + }; + const shell = new WorkspaceShell(fake.rpc.shell, sync); + + const result = await (await shell.exec("touch out.txt", { writable: false })).result(); + + expect(result.pulled).toBe(0); + expect(result.skipped).toEqual([ + { path: "/workspace/out.txt", rev: 4, reason: "no-write-access" }, + ]); + expect(result.sync).toEqual({ + status: "complete", + applied: 0, + skipped: [{ path: "/workspace/out.txt", rev: 4, reason: "no-write-access" }], + }); + }); +}); diff --git a/packages/computer/src/shell.ts b/packages/computer/src/shell.ts index a199281c..c682c0e0 100644 --- a/packages/computer/src/shell.ts +++ b/packages/computer/src/shell.ts @@ -29,6 +29,7 @@ import type { ExecEvent, ShellRPC } from "@cloudflare/computer-rpc"; import type { ApplyResult, SkippedEntry } from "@cloudflare/dofs"; +import { noopAudit, openGate, type WorkspaceAudit, type WorkspaceGate, withGate } from "./gate.js"; import { noopObserver, safeErrorMessage, type WorkspaceObserver, withSpan } from "./observe.js"; import { assertNotTemplate } from "./sh.js"; @@ -116,6 +117,23 @@ export interface ExecOptions { // one passed to the Workspace constructor); pass the id of // another configured backend to route this call there. backend?: string; + // Whether this command may modify the workspace. Defaults to true. + // Pass false for a command expected only to read: writes it + // attempts then fail rather than land silently. + // + // The point is the misclassified command. A caller that decides + // `git log` is read-only and is wrong about it — an alias, a shell + // function, a `;` it did not parse — gets a failure it can see + // instead of a mutation it did not intend. Which failure depends on + // the backend: one that works directly against the workspace store + // fails the write inside the command with EROFS, and one with its + // own copy of the files has the change refused on the way back, + // reported in `skipped`. + // + // A configured gate can withdraw write access even when this is + // true, so a command may end up read-only without the caller + // asking. It can never gain access this way. + writable?: boolean; } export interface GetExecOptions { @@ -138,19 +156,36 @@ export interface GetExecOptions { // skipped read-only entries on ExecResult. export interface Sync { push(): Promise; - pull(): Promise; + // The post-command pull carries the command's write access, so a + // command that ran read-only cannot have its changes applied by the + // bracket that follows it. + pull(options?: { writable?: boolean }): Promise; onPullPending?(error: unknown): Promise; } +export interface WorkspaceShellHooks { + gate?: WorkspaceGate; + audit?: WorkspaceAudit; +} + export class WorkspaceShell { readonly #shell: ShellRPC; readonly #sync: Sync; readonly #observer: WorkspaceObserver; + readonly #gate: WorkspaceGate; + readonly #audit: WorkspaceAudit; - constructor(shell: ShellRPC, sync: Sync, observer: WorkspaceObserver = noopObserver) { + constructor( + shell: ShellRPC, + sync: Sync, + observer: WorkspaceObserver = noopObserver, + hooks: WorkspaceShellHooks = {}, + ) { this.#shell = shell; this.#sync = sync; this.#observer = observer; + this.#gate = hooks.gate ?? openGate; + this.#audit = hooks.audit ?? noopAudit; } exec(command: string): Promise>; @@ -161,6 +196,34 @@ export class WorkspaceShell { options: ExecOptions = {}, ): Promise> { assertNotTemplate(command); + // The gate runs before the push, so a denied command does not + // move data. It is consulted once for the whole command; see + // ./gate.ts for why a write-by-write gate would be worse. + // + // The audit hook fires here too, on the spawn, rather than after + // the command exits. exec() returns a detached handle that the + // caller may never drain, so there is no later point that is + // guaranteed to arrive. What the command then did is on the + // observer's span and on ExecResult. + return withGate( + this.#gate, + this.#audit, + { + kind: "shell.exec", + command, + cwd: options.cwd, + writable: options.writable ?? true, + backend: options.backend, + }, + (writable) => this.#spawn(command, options, writable), + ); + } + + async #spawn( + command: string, + options: ExecOptions, + writable: boolean, + ): Promise> { // Pre-exec push: ship anything the host wrote since the last // push so the spawned command sees it. Failures non-fatal per // docs/05 — the command still runs; pushed reports 0. @@ -177,6 +240,7 @@ export class WorkspaceShell { "workspace.runtime.cwd": options.cwd, "workspace.runtime.timeout_ms": options.timeoutMs, "workspace.runtime.id": options.id, + "workspace.runtime.writable": writable, }, () => this.#shell.exec({ @@ -189,6 +253,7 @@ export class WorkspaceShell { typeof options.stdin === "string" ? new TextEncoder().encode(options.stdin) : options.stdin, + writable, }), (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.runtime.id", outcome.value.id); @@ -200,7 +265,15 @@ export class WorkspaceShell { // exec call — because we hand the inner stream off to the // caller and can't `using` the envelope ourselves. const events = disposeOnDone(envelope.events, () => maybeDispose(envelope)); - return wrapHandle(this.#shell, this.#sync, envelope.id, events, options.encoding, pushed); + return wrapHandle( + this.#shell, + this.#sync, + envelope.id, + events, + options.encoding, + pushed, + writable, + ); } get(id: string): Promise>; @@ -249,8 +322,9 @@ function wrapHandle( wireEvents: ReadableStream, encoding: E | undefined, pushed: number, + writable = true, ): ExecHandle { - const postPull = withPostPull(pipeEvents(wireEvents, encoding), sync); + const postPull = withPostPull(pipeEvents(wireEvents, encoding), sync, writable); const stream = postPull.stream; const handle = stream as ExecHandle; let resultPromise: Promise> | undefined; @@ -378,6 +452,7 @@ interface PostPullOutcome { function withPostPull( source: ReadableStream>, sync: Sync, + writable: boolean, ): { stream: ReadableStream>; outcome: Promise } { const reader = source.getReader(); let resolveOutcome!: (outcome: PostPullOutcome) => void; @@ -394,7 +469,7 @@ function withPostPull( return; } reader.releaseLock(); - const pulled = await runPostPull(sync); + const pulled = await runPostPull(sync, writable); resolveOutcome(pulled); controller.close(); } catch (error) { @@ -427,9 +502,12 @@ function withPostPull( return { stream, outcome }; } -async function runPostPull(sync: Sync): Promise { +async function runPostPull(sync: Sync, writable: boolean): Promise { try { - const result = await sync.pull(); + // The command's write access travels with its own post-command + // pull. Without this the bracket would apply exactly the changes + // the command was not allowed to make. + const result = await sync.pull({ writable }); return { applied: result.applied, skipped: result.skipped, diff --git a/packages/computer/src/workspace.ts b/packages/computer/src/workspace.ts index 32ee56eb..7bc4ef43 100644 --- a/packages/computer/src/workspace.ts +++ b/packages/computer/src/workspace.ts @@ -63,6 +63,13 @@ export interface SyncRetryScheduler { clear(backend: string): Promise; } +export interface WorkspacePullOptions { + // Whether this pull may write to the local store. Defaults to true. + // A pull without write access applies nothing and reports every + // entry it was offered as skipped. + writable?: boolean; +} + export interface SyncRetryOptions { initialDelayMs?: number; maxDelayMs?: number; @@ -626,8 +633,15 @@ export class Workspace { ); } - pull(id?: string): Promise { - return this.#serialize(id, (resolvedId) => this.#pullResolved(resolvedId)); + // `writable: false` pulls without write access: nothing is + // applied and every entry comes back in `skipped`. This is how a + // command that ran without write access has its changes refused + // when the backend keeps its own copy of the files and wrote to + // that copy before we heard about it. + pull(id?: string, options?: WorkspacePullOptions): Promise { + return this.#serialize(id, (resolvedId) => + this.#pullResolved(resolvedId, options?.writable ?? true), + ); } /** @@ -658,7 +672,7 @@ export class Workspace { }; } try { - const result = await this.#pullResolved(resolvedId); + const result = await this.#pullResolved(resolvedId, true); await scheduler.clear(resolvedId); return { status: "complete", @@ -683,11 +697,11 @@ export class Workspace { }); } - #pullResolved(resolvedId: string | undefined): Promise { + #pullResolved(resolvedId: string | undefined, writable: boolean): Promise { return withSpan( this.#observer, "workspace.sync.pull", - { "workspace.sync.backend": resolvedId }, + { "workspace.sync.backend": resolvedId, "workspace.sync.writable": writable }, async () => { if (resolvedId === undefined || this.#moduleBackendsById.has(resolvedId)) { return { applied: 0, skipped: [] }; @@ -695,7 +709,7 @@ export class Workspace { const handle = await this.#handleFor(resolvedId); if (handle.sync === "none") return { applied: 0, skipped: [] }; return this.#runWithInvalidation(resolvedId, handle, () => - pullOnce(this.#db, handle.rpc.sync, resolvedId), + pullOnce(this.#db, handle.rpc.sync, resolvedId, { writable }), ); }, (span, outcome) => { @@ -973,10 +987,11 @@ export class Workspace { handle.rpc.shell, { push: () => this.push(id), - pull: () => this.pull(id), + pull: (options) => this.pull(id, options), onPullPending: () => this.#schedulePendingSync(id), }, this.#observer, + { gate: this.#gate, audit: this.#audit }, ); this.#shells.set(id, shell); return { shell, handle }; @@ -1072,12 +1087,19 @@ class WorkspaceShellRouter { // swapped in a newer handle that we must not clobber. const { shell, handle: dispatchHandle } = await this.#shellFor(id); const { backend: _backend, ...rest } = options; + // The caller's selector is replaced with the id it resolved to, + // rather than dropped. The per-backend shell does not need it to + // route — it is already the right shell — but the gate is handed + // this and a gate deciding whether to trust a command with write + // access wants to know which backend will run it, since that is + // what determines whether a refused write is prevented or merely + // reported. + const forwarded = { ...rest, backend: id }; let execHandle: unknown; try { - execHandle = await (shell.exec as unknown as (c: string, o: typeof rest) => Promise)( - command, - rest, - ); + execHandle = await ( + shell.exec as unknown as (c: string, o: typeof forwarded) => Promise + )(command, forwarded); } catch (error) { this.#onError(id, dispatchHandle, error); throw error; From 48c086a99b6cb61dfd603fc62b7b163543701e36 Mon Sep 17 00:00:00 2001 From: Antoni T Date: Mon, 3 Aug 2026 15:16:42 +0100 Subject: [PATCH 06/13] computer: honour write access in the worker-shell backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is where `writable: false` stops being a request and starts preventing writes. The backend forwards the flag to the shell, and the shell asks the host for a workspace stub built without write access rather than checking the flag itself. Asking for a stub it cannot write through is the point. Every route into the workspace for that command — the shell's builtins, the git command, anything a subclass registers — goes through that one handle, so all of them are refused together. A check at the call site would have covered that call site and nothing else, and would have needed repeating for every path added later. `Workspace.stub({ writable: false })` is the seam, backed by `fsWithAccess`, which hands out a second filesystem handle over the same store. Two handles over one database is deliberate: commands overlap, and a read-only one must not be able to disarm a writable one running beside it. The capability cannot live on the database for that reason, and cannot be a second Database at all — dofs assumes exactly one Database wraps each SqlStorage. The stub narrows its runtime too. A read-only stub that could still spawn a writing command would not be read-only. The tests run real just-bash against a real Workspace, through the same `workspace.stub(options)` call production makes. The one that matters is `find /workspace -mindepth 1 -delete` under `writable: false`: every file survives. Its control runs the same command with access and confirms the tree does get deleted, so the first test cannot pass by the command being broken. A third asserts the failure reaches stderr as a read-only error, so neither can pass because the stub failed to load. No `denied` list on ExecResult. Here the refusal is already visible where it happened — the write fails inside the command, which sees the error and reports it — and for a backend with its own copy of the files it arrives in `skipped`. A third channel would duplicate the one and need new wire surface for the other. --- .../backends/worker-shell/entrypoint.test.ts | 159 +++++++++++++++++- .../src/backends/worker-shell/entrypoint.ts | 18 +- .../src/backends/worker-shell/worker-shell.ts | 2 + packages/computer/src/proxy.ts | 8 +- packages/computer/src/stub.ts | 71 +++++--- packages/computer/src/with-workspace.ts | 16 +- packages/computer/src/workspace.ts | 32 +++- 7 files changed, 271 insertions(+), 35 deletions(-) diff --git a/packages/computer/src/backends/worker-shell/entrypoint.test.ts b/packages/computer/src/backends/worker-shell/entrypoint.test.ts index c1fcd049..faca3625 100644 --- a/packages/computer/src/backends/worker-shell/entrypoint.test.ts +++ b/packages/computer/src/backends/worker-shell/entrypoint.test.ts @@ -81,7 +81,7 @@ interface FakeWorkspace { interface FakeEnv { HOST: { - getWorkspace(): Promise; + getWorkspace(options?: { writable?: boolean }): Promise; }; } @@ -272,6 +272,163 @@ describe("ShellWorker", () => { expect(await response.text()).toMatch(/Workers RPC/); }); + // --------------------------------------------------------------- + // write access + // --------------------------------------------------------------- + // + // Real just-bash against a real Workspace, because the claim being + // tested is about what a real command does to a real store. The env + // here calls workspace.stub(options), which is what production does + // over RPC, so the read-only path under test is the shipped one and + // not a flag this test set by hand. + describe("write access", () => { + let workspace: Workspace; + const stubs: WorkspaceStub[] = []; + + beforeEach(async () => { + workspace = new Workspace({ + storage: new SQLiteTestStorage() as never, + backends: [noopBackend()], + git: createGitClient(), + }); + await workspace.ready(); + await workspace.fs.mkdir("/workspace", { recursive: true }); + }); + + afterEach(async () => { + for (const stub of stubs.splice(0)) stub[Symbol.dispose](); + await workspace.close(); + }); + + function envForWorkspace(): FakeEnv { + return { + HOST: { + async getWorkspace(options?: { writable?: boolean }) { + const stub = workspace.stub(options); + stubs.push(stub); + return stub as unknown as FakeWorkspace; + }, + }, + }; + } + + async function run(command: string, writable?: boolean) { + const worker = new ShellWorker(undefined as never, envForWorkspace() as never); + const envelope = await worker.exec({ + command, + cwd: "/workspace", + id: `run-${Math.random().toString(36).slice(2)}`, + writable, + }); + const events = (await drain(envelope.events)) as { name: string; value: string | number }[]; + return { + stdout: events + .filter((e) => e.name === "stdout") + .map((e) => String(e.value)) + .join(""), + stderr: events + .filter((e) => e.name === "stderr") + .map((e) => String(e.value)) + .join(""), + exitCode: events.find((e) => e.name === "exit")?.value, + }; + } + + it("does not delete the workspace when a read-only command tries to", async () => { + // The case the whole feature exists for. `find -delete` is one + // command issuing a delete per entry, so a per-write refusal + // would stop partway and leave a half-deleted tree. Refusing + // the capability up front means the first delete fails and + // every file is still there. + await workspace.fs.writeFile("/workspace/keep.txt", "keep"); + await workspace.fs.mkdir("/workspace/dir", { recursive: true }); + await workspace.fs.writeFile("/workspace/dir/nested.txt", "nested"); + + const result = await run("find /workspace -mindepth 1 -delete", false); + + expect(result.exitCode).not.toBe(0); + expect(await workspace.fs.readFile("/workspace/keep.txt", "utf8")).toBe("keep"); + expect(await workspace.fs.readFile("/workspace/dir/nested.txt", "utf8")).toBe("nested"); + }); + + it("deletes the workspace when the same command is allowed to", async () => { + // The control. Without it the test above would pass just as + // well against a command that never worked. + await workspace.fs.writeFile("/workspace/keep.txt", "keep"); + + const result = await run("find /workspace -mindepth 1 -delete", true); + + expect(result.exitCode).toBe(0); + await expect(workspace.fs.readFile("/workspace/keep.txt", "utf8")).rejects.toThrow(); + }); + + it("fails a redirect into a new file and creates nothing", async () => { + const result = await run("echo written > /workspace/new.txt", false); + + expect(result.exitCode).not.toBe(0); + await expect(workspace.fs.stat("/workspace/new.txt")).rejects.toThrow(); + }); + + it("leaves an existing file untouched when a read-only command overwrites it", async () => { + await workspace.fs.writeFile("/workspace/existing.txt", "original"); + + const result = await run("echo replaced > /workspace/existing.txt", false); + + expect(result.exitCode).not.toBe(0); + expect(await workspace.fs.readFile("/workspace/existing.txt", "utf8")).toBe("original"); + }); + + it("still reads while refusing to write", async () => { + // Read-only has to mean read-only, not broken. A command that + // only reads must behave exactly as it would with access. + await workspace.fs.writeFile("/workspace/readable.txt", "contents\n"); + + const result = await run("cat /workspace/readable.txt", false); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("contents"); + }); + + it("reports the refusal to the command rather than failing silently", async () => { + // The point of failing the write inside the command: the + // command sees an error it can act on, and a caller reading + // stderr can tell what happened. + await workspace.fs.writeFile("/workspace/existing.txt", "original"); + + const result = await run("echo replaced > /workspace/existing.txt", false); + + // Pinned to the reason, not just to non-empty output: a + // non-zero exit and some stderr would also be produced by the + // stub being broken, which would make the tests above pass for + // the wrong reason. + expect(result.stderr).toMatch(/read-only|EROFS/i); + }); + + it("defaults to writable when the caller does not say", async () => { + const result = await run("echo written > /workspace/default.txt"); + + expect(result.exitCode).toBe(0); + expect(await workspace.fs.readFile("/workspace/default.txt", "utf8")).toBe("written\n"); + }); + + it("keeps concurrent commands' access independent", async () => { + // Two commands overlap in production, and the capability is + // per-handle precisely so a read-only one cannot disarm a + // writable one running beside it. + await workspace.fs.writeFile("/workspace/shared.txt", "original"); + + const [readOnly, writable] = await Promise.all([ + run("echo denied > /workspace/blocked.txt", false), + run("echo allowed > /workspace/allowed.txt", true), + ]); + + expect(readOnly.exitCode).not.toBe(0); + expect(writable.exitCode).toBe(0); + await expect(workspace.fs.stat("/workspace/blocked.txt")).rejects.toThrow(); + expect(await workspace.fs.readFile("/workspace/allowed.txt", "utf8")).toBe("allowed\n"); + }); + }); + // --------------------------------------------------------------- // `git` custom command wiring // --------------------------------------------------------------- diff --git a/packages/computer/src/backends/worker-shell/entrypoint.ts b/packages/computer/src/backends/worker-shell/entrypoint.ts index 690d6a91..a7e073b2 100644 --- a/packages/computer/src/backends/worker-shell/entrypoint.ts +++ b/packages/computer/src/backends/worker-shell/entrypoint.ts @@ -32,6 +32,14 @@ export interface ExecInput { timeoutMs?: number; env?: Record; stdin?: Uint8Array; + // Whether this command may modify the workspace. Defaults to true. + // + // Enforced by asking the host for a workspace stub without write + // access, so the shell never holds a writable capability for the + // duration of the command. A write then fails inside the command + // with EROFS, which the command sees and reports like any other + // filesystem error, and nothing is applied to undo afterwards. + writable?: boolean; } export interface ShellWorkerOptions { @@ -59,7 +67,7 @@ export interface ShellWorkerEnv { // Subset of the WorkspaceServiceProxy Fetcher the shell uses. // Declared structurally so tests can swap in a fake. export interface ShellHostFetcher { - getWorkspace(): Promise; + getWorkspace(options?: { writable?: boolean }): Promise; } // The shape getWorkspace() returns. The shell touches .fs (for @@ -154,9 +162,15 @@ export class ShellWorker< // Reach the host workspace on each call. Concurrent execs get separate // stubs; only their cancellation controllers share this entrypoint. + // The write access is requested here rather than checked later. + // Asking for a read-only stub means every path into the workspace + // for this command — the shell's own builtins, the git command, + // anything a subclass adds — is refused by the handle itself. A + // check at one call site would only cover that call site. + const writable = input.writable ?? true; let ws: HostWorkspaceStub; try { - ws = await this.env.HOST.getWorkspace(); + ws = await this.env.HOST.getWorkspace({ writable }); } catch (error) { if (timer !== undefined) clearTimeout(timer); if (this.#executions.get(id) === controller) this.#executions.delete(id); diff --git a/packages/computer/src/backends/worker-shell/worker-shell.ts b/packages/computer/src/backends/worker-shell/worker-shell.ts index b78c9abc..5464e8e0 100644 --- a/packages/computer/src/backends/worker-shell/worker-shell.ts +++ b/packages/computer/src/backends/worker-shell/worker-shell.ts @@ -40,6 +40,7 @@ export interface WorkerShellFetcher { timeoutMs?: number; env?: Record; stdin?: Uint8Array; + writable?: boolean; }): Promise<{ id: string; events: ReadableStream; @@ -177,6 +178,7 @@ export class WorkerShellBackend implements WorkspaceBackend { timeoutMs: input.timeoutMs, env: input.env, stdin: input.stdin, + writable: input.writable, }); return { id: envelope.id, events: decodeFramedEvents(envelope.events) }; }, diff --git a/packages/computer/src/proxy.ts b/packages/computer/src/proxy.ts index 861d9231..495e49a7 100644 --- a/packages/computer/src/proxy.ts +++ b/packages/computer/src/proxy.ts @@ -163,9 +163,11 @@ export class WorkspaceServiceProxy extends WorkerEntrypoint { - const stub = this.#hostStub<{ __getWorkspaceStub(): Promise }>(); - return stub.__getWorkspaceStub(); + async getWorkspace(options?: { writable?: boolean }): Promise { + const stub = this.#hostStub<{ + __getWorkspaceStub(options?: { writable?: boolean }): Promise; + }>(); + return stub.__getWorkspaceStub(options); } // Optional Artifacts CLI hook used by the worker-backend shell. diff --git a/packages/computer/src/stub.ts b/packages/computer/src/stub.ts index 844d8452..fe6d0b71 100644 --- a/packages/computer/src/stub.ts +++ b/packages/computer/src/stub.ts @@ -47,6 +47,7 @@ import type { ReadFileOptions, RmOptions, WorkspaceDirentResult, + WorkspaceFilesystem, WorkspaceFoundEntry, WorkspaceGrepMatch, WorkspaceStatResult, @@ -89,10 +90,18 @@ import type { Workspace } from "./workspace.js"; // the same reason. export class WorkspaceFilesystemStub extends RpcTarget { readonly #ws: Workspace; - - constructor(ws: Workspace) { + // The handle this stub's mutations go through. A stub made without + // write access gets a read-only handle, so a caller holding it + // cannot write no matter which method it reaches for. The handle + // carries the capability rather than this class checking a flag per + // method, which is what keeps a new method from being added without + // the check. + readonly #fs: WorkspaceFilesystem; + + constructor(ws: Workspace, writable = true) { super(); this.#ws = ws; + this.#fs = ws.fsWithAccess(writable); trackStub(this); } @@ -110,7 +119,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { optionsOrEncoding?: "utf8" | ReadFileOptions, ): Promise> { return withSpan(this.#ws.observer, "workspace.fs.readFile", { "workspace.fs.path": path }, () => - this.#ws.fs.readFile(path, optionsOrEncoding as ReadFileOptions), + this.#fs.readFile(path, optionsOrEncoding as ReadFileOptions), ); } @@ -125,7 +134,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { stat(path: string): Promise { return withSpan(this.#ws.observer, "workspace.fs.stat", { "workspace.fs.path": path }, () => - this.#ws.fs.stat(path), + this.#fs.stat(path), ); } @@ -136,7 +145,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { { "workspace.fs.path": path }, async () => { try { - return await this.#ws.fs.stat(path); + return await this.#fs.stat(path); } catch (error) { if ((error as { code?: string }).code === "ENOENT") return null; throw error; @@ -147,7 +156,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { lstat(path: string): Promise { return withSpan(this.#ws.observer, "workspace.fs.lstat", { "workspace.fs.path": path }, () => - this.#ws.fs.lstat(path), + this.#fs.lstat(path), ); } @@ -158,7 +167,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { { "workspace.fs.path": path }, async () => { try { - return await this.#ws.fs.lstat(path); + return await this.#fs.lstat(path); } catch (error) { if ((error as { code?: string }).code === "ENOENT") return null; throw error; @@ -169,7 +178,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { readlink(path: string): Promise { return withSpan(this.#ws.observer, "workspace.fs.readlink", { "workspace.fs.path": path }, () => - this.#ws.fs.readlink(path), + this.#fs.readlink(path), ); } @@ -178,7 +187,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.readdir", { "workspace.fs.path": path }, - () => this.#ws.fs.readdir(path, options), + () => this.#fs.readdir(path, options), (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.fs.entries", outcome.value.length); }, @@ -190,7 +199,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.find", { "workspace.fs.path": directory, "workspace.fs.pattern": pattern }, - () => this.#ws.fs.find(directory, pattern), + () => this.#fs.find(directory, pattern), (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.fs.matches", outcome.value.length); }, @@ -202,7 +211,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.ls", { "workspace.fs.path": prefix }, - () => this.#ws.fs.ls(prefix), + () => this.#fs.ls(prefix), (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.fs.entries", outcome.value.length); }, @@ -214,7 +223,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.grep", { "workspace.fs.path": path, "workspace.fs.pattern": pattern }, - () => this.#ws.fs.grep(pattern, path, options), + () => this.#fs.grep(pattern, path, options), (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.fs.matches", outcome.value.length); }, @@ -232,7 +241,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.writeFile", { "workspace.fs.path": path }, - () => this.#ws.fs.writeFile(path, content, options), + () => this.#fs.writeFile(path, content, options), ); } @@ -241,7 +250,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.mkdir", { "workspace.fs.path": path, "workspace.fs.recursive": options.recursive }, - () => this.#ws.fs.mkdir(path, options), + () => this.#fs.mkdir(path, options), ); } @@ -254,7 +263,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { "workspace.fs.recursive": options.recursive, "workspace.fs.force": options.force, }, - () => this.#ws.fs.rm(path, options), + () => this.#fs.rm(path, options), ); } @@ -263,7 +272,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.chmod", { "workspace.fs.path": path, "workspace.fs.mode": mode }, - () => this.#ws.fs.chmod(path, mode), + () => this.#fs.chmod(path, mode), ); } @@ -272,7 +281,7 @@ export class WorkspaceFilesystemStub extends RpcTarget { this.#ws.observer, "workspace.fs.symlink", { "workspace.fs.path": path, "workspace.fs.target": target }, - () => this.#ws.fs.symlink(target, path), + () => this.#fs.symlink(target, path), ); } } @@ -373,10 +382,15 @@ export class WorkspaceRuntimeExecHandleStub< export class WorkspaceRuntimeStub extends RpcTarget { readonly #ws: Workspace; + // Ceiling on the write access of commands spawned through this + // stub. A read-only stub narrows every exec it is asked for; it + // cannot be talked into granting more by passing writable: true. + readonly #writable: boolean; - constructor(ws: Workspace) { + constructor(ws: Workspace, writable = true) { super(); this.#ws = ws; + this.#writable = writable; trackStub(this); } @@ -403,7 +417,10 @@ export class WorkspaceRuntimeStub extends RpcTarget { source: string, options: WorkspaceRuntimeExecOptions<"utf8" | undefined>, ) => Promise> - )(source, options); + )(source, { + ...options, + writable: this.#writable && (options.writable ?? true), + }); if (options.id !== undefined && handle.id !== options.id) { throw new Error(`backend ran exec as ${handle.id}, not the requested id ${options.id}`); } @@ -580,6 +597,12 @@ export class WorkspaceGitStub extends RpcTarget { // between wsd and the DO. WorkspaceStub here is a different thing // (the Workers-RPC value carried between the DO and a Worker), so // the name doesn't clash. +export interface WorkspaceStubOptions { + // Whether the holder of this stub may modify the workspace. + // Defaults to true. + writable?: boolean; +} + export class WorkspaceStub extends RpcTarget { // Getters rather than instance properties so Workers RPC // exposes them through the stub proxy. Plain readonly fields @@ -592,10 +615,14 @@ export class WorkspaceStub extends RpcTarget { readonly #artifacts: WorkspaceArtifactsStub; readonly #useThink: boolean; - constructor(ws: Workspace) { + // `writable: false` hands out a stub that cannot write. The whole + // stub, not just its fs: a read-only stub whose runtime could still + // spawn a writing command would not be read-only. + constructor(ws: Workspace, options: WorkspaceStubOptions = {}) { super(); - this.#fs = new WorkspaceFilesystemStub(ws); - this.#runtime = new WorkspaceRuntimeStub(ws); + const writable = options.writable ?? true; + this.#fs = new WorkspaceFilesystemStub(ws, writable); + this.#runtime = new WorkspaceRuntimeStub(ws, writable); this.#git = new WorkspaceGitStub(ws); this.#assets = ws.assets === undefined ? undefined : new WorkspaceAssetsStub(ws); this.#artifacts = new WorkspaceArtifactsStub(ws.artifacts); diff --git a/packages/computer/src/with-workspace.ts b/packages/computer/src/with-workspace.ts index d3278f7e..9a3ff9b8 100644 --- a/packages/computer/src/with-workspace.ts +++ b/packages/computer/src/with-workspace.ts @@ -36,7 +36,9 @@ export const WORKSPACE = Symbol("workspace"); // The prototype method `getWorkspace(stub)` calls over RPC. Exported // so the client and its tests can name the shape. export interface WorkspaceStubHost { - __getWorkspaceStub(): Promise; + __getWorkspaceStub( + options?: import("./stub.js").WorkspaceStubOptions, + ): Promise; } // A host that carries the symbol-stashed Workspace. Used by the @@ -68,11 +70,19 @@ export function withWorkspace( (this as unknown as WorkspaceLocalHost)[WORKSPACE] = new Workspace(options(self)); } - __getWorkspaceStub(): Promise { + // `options.writable: false` returns a stub that cannot write. + // This is how a command running without write access gets a + // handle that refuses writes rather than a handle it is merely + // asked not to use: the worker-shell backend passes the flag + // here, and the shell inside the Dynamic Worker never holds a + // writable capability at all. + __getWorkspaceStub( + options?: import("./stub.js").WorkspaceStubOptions, + ): Promise { const ws = (this as unknown as WorkspaceLocalHost)[WORKSPACE]; return (async () => { await ws.ready(); - return ws.stub(); + return ws.stub(options); })(); } } diff --git a/packages/computer/src/workspace.ts b/packages/computer/src/workspace.ts index 7bc4ef43..7c425806 100644 --- a/packages/computer/src/workspace.ts +++ b/packages/computer/src/workspace.ts @@ -16,7 +16,7 @@ import { type DurableObjectStorageLike, initializeSchema, SQLiteWorkspaceProvider, - type WorkspaceFilesystem, + WorkspaceFilesystem, } from "@cloudflare/dofs"; import { @@ -42,7 +42,7 @@ import { type WorkspaceRegisteredBackend, } from "./runtime/types.js"; import { WorkspaceShell } from "./shell.js"; -import { WorkspaceStub } from "./stub.js"; +import { WorkspaceStub, type WorkspaceStubOptions } from "./stub.js"; import { isWorkspaceTransportFailure } from "./transport-failure.js"; export interface SyncRetryIntent { @@ -244,6 +244,7 @@ export class Workspace { readonly #observer: WorkspaceObserver; readonly #gate: WorkspaceGate; readonly #audit: WorkspaceAudit; + #readOnlyFs: WorkspaceFilesystem | undefined; readonly #now: () => number; readonly #waitUntil: ((promise: Promise) => void) | undefined; readonly #retryScheduler: SyncRetryScheduler | undefined; @@ -581,8 +582,31 @@ export class Workspace { // across the Workers-RPC boundary (e.g. returned from a DO RPC // method). The stub is a lazy RpcTarget — it doesn't own any // resources itself; it just delegates back to this workspace. - stub(): WorkspaceStub { - return new WorkspaceStub(this); + stub(options?: WorkspaceStubOptions): WorkspaceStub { + return new WorkspaceStub(this, options); + } + + // Filesystem handle for a given write access. The writable case is + // the workspace's own gated handle; the read-only case is a second + // handle over the same store built without the capability. + // + // Two handles over one database is the point: commands overlap, and + // a read-only one must not be able to disarm a writable one running + // beside it. The capability lives on the handle for exactly that + // reason — it cannot be a property of the database without one + // command's access leaking into another's. It also cannot be a + // second Database, because dofs assumes exactly one Database wraps + // each SqlStorage and its resolve cache depends on that. + // + // The read-only handle is built once and shared. It holds no + // per-command state, and every caller wants the same thing from it. + fsWithAccess(writable: boolean): WorkspaceFilesystem { + if (writable) return this.#fs; + this.#readOnlyFs ??= new WorkspaceFilesystem(this.#db, { + now: this.#now, + writable: false, + }); + return this.#readOnlyFs; } // Sync the local store with a configured backend. From c6b7d9a768a6c7603decef115e3ad55cadfa42ca Mon Sep 17 00:00:00 2001 From: Antoni T Date: Mon, 3 Aug 2026 15:21:54 +0100 Subject: [PATCH 07/13] computer: let the host classify a command's write access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ExecToolOptions.writable` is a host-supplied function that decides whether a command may modify the workspace. Omitting it leaves every command writable, which is the behaviour without it. It is not part of the input schema, and that is the whole design. The model must not classify its own command: the case this feature exists for is the command mislabelled as read-only, and asking the model that mislabelled it to declare the label would produce a flag that agrees with the mistake every time. The host decides from something it already trusts — an allowlist, a plan step, a human — and the model finds out the same way it finds out about any other failure, by the write failing. A test asserts the field stays out of the schema, since adding it there would look like a convenience. The resolver is allowed to be wrong, and fails safe when it is. A read command classified read-only runs normally. A write command classified read-only fails where it writes instead of writing. The effective access is reported on the tool result. Without it a read-only run looks like an arbitrary failure and the model's next move is to run the same command again. A gate denial arrives as a thrown error and is returned as a tool result like every other failure, so the model can read the refusal rather than the agent loop tearing down. `createAITools({ readonly: true })` is left alone. It drops the exec tool even when a shell is configured, and a test passes `shell` specifically to assert that, so the behaviour is a decision rather than an oversight. Letting a readonly toolset run read-only commands is worth proposing separately; it should not ride along here. --- packages/computer/src/tools/ai.test.ts | 3 + packages/computer/src/tools/exec.test.ts | 136 +++++++++++++++++++++++ packages/computer/src/tools/exec.ts | 36 +++++- 3 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 packages/computer/src/tools/exec.test.ts diff --git a/packages/computer/src/tools/ai.test.ts b/packages/computer/src/tools/ai.test.ts index 9ea35cdb..5511c658 100644 --- a/packages/computer/src/tools/ai.test.ts +++ b/packages/computer/src/tools/ai.test.ts @@ -323,6 +323,7 @@ describe("createAITools exec tool", () => { command: "npm test", cwd: "/workspace", backend: "container", + writable: true, exitCode: 2, stdout: "abc\n\n[truncated, 3 more bytes]", stderr: "uvw\n\n[truncated, 3 more bytes]", @@ -437,6 +438,7 @@ describe("createAITools exec tool", () => { command: "npm test", cwd: null, backend: "shell", + writable: true, error: "backend unavailable", }); }); @@ -465,6 +467,7 @@ describe("createAITools exec tool", () => { command: "npm test", cwd: null, backend: "shell", + writable: true, error: "transport closed", }); }); diff --git a/packages/computer/src/tools/exec.test.ts b/packages/computer/src/tools/exec.test.ts new file mode 100644 index 00000000..d66144d5 --- /dev/null +++ b/packages/computer/src/tools/exec.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, it } from "vitest"; +import { createExecTool, type ExecToolInput, type ExecWorkspaceLike } from "./exec.js"; + +interface ExecCall { + command: string; + cwd: string | undefined; + backend: string | undefined; + writable: boolean | undefined; +} + +function fakeWorkspace( + result: { exitCode: number; stdout: string; stderr: string } | Error = { + exitCode: 0, + stdout: "", + stderr: "", + }, +): ExecWorkspaceLike & { calls: ExecCall[] } { + const calls: ExecCall[] = []; + return { + calls, + runtime: { + async exec(command, options) { + calls.push({ + command, + cwd: options.cwd, + backend: options.backend, + writable: options.writable, + }); + if (result instanceof Error) throw result; + return { result: async () => result }; + }, + }, + }; +} + +function toolFor( + workspace: ExecWorkspaceLike, + writable?: (input: ExecToolInput) => boolean, +): ReturnType { + return createExecTool({ + workspace, + backends: { shell: { description: "a shell" }, container: { description: "a container" } }, + defaultBackend: "shell", + writable, + }); +} + +// The `ai` package types execute as optional and pass a second +// argument the tool ignores. +async function run( + tool: ReturnType, + input: { command: string; cwd?: string; backend?: string }, +): Promise> { + const execute = tool.execute as (i: typeof input, o: unknown) => Promise>; + return execute(input, {}); +} + +describe("createExecTool — write access", () => { + it("runs commands with write access when no resolver is configured", async () => { + const ws = fakeWorkspace(); + await run(toolFor(ws), { command: "npm test" }); + expect(ws.calls[0].writable).toBe(true); + }); + + it("asks the resolver and forwards its answer", async () => { + const ws = fakeWorkspace(); + const tool = toolFor(ws, ({ command }) => !command.startsWith("git log")); + + await run(tool, { command: "git log --oneline" }); + await run(tool, { command: "npm install" }); + + expect(ws.calls.map((c) => c.writable)).toEqual([false, true]); + }); + + it("shows the resolver the resolved backend, not the model's selector", async () => { + // The resolver's decision often depends on where the command + // runs, since that determines whether a refused write is + // prevented or only reported afterwards. + const seen: ExecToolInput[] = []; + const ws = fakeWorkspace(); + const tool = toolFor(ws, (input) => { + seen.push(input); + return true; + }); + + await run(tool, { command: "ls" }); + await run(tool, { command: "ls", backend: "container", cwd: "/workspace/sub" }); + + expect(seen).toEqual([ + { command: "ls", cwd: undefined, backend: "shell" }, + { command: "ls", cwd: "/workspace/sub", backend: "container" }, + ]); + }); + + it("does not accept write access as model input", async () => { + // The model must not classify its own command. If it could, the + // command mislabelled as read-only — the case this exists to + // catch — would arrive labelled read-only and be trusted. + const ws = fakeWorkspace(); + const tool = toolFor(ws, () => false); + + await run(tool, { command: "rm -rf /", writable: true } as never); + + expect(ws.calls[0].writable).toBe(false); + }); + + it("keeps write access out of the input schema entirely", async () => { + const tool = toolFor(fakeWorkspace()); + const schema = tool.inputSchema as { shape?: Record }; + expect(Object.keys(schema.shape ?? {})).toEqual(["command", "cwd", "backend"]); + }); + + it("reports the write access it ran with on a success", async () => { + // So the model can tell a refused write from a broken command; + // otherwise its next move is to retry the same thing. + const ws = fakeWorkspace({ exitCode: 1, stdout: "", stderr: "read-only access\n" }); + const output = await run( + toolFor(ws, () => false), + { command: "touch f" }, + ); + + expect(output.writable).toBe(false); + expect(output.exitCode).toBe(1); + expect(output.stderr).toContain("read-only access"); + }); + + it("returns a refused command as a tool result rather than throwing", async () => { + // A gate denial arrives as a thrown error. Returning it keeps the + // agent loop alive and lets the model read why it was refused. + const ws = fakeWorkspace(new Error("shell.exec denied: not on the allowlist")); + const output = await run(toolFor(ws), { command: "rm -rf /" }); + + expect(output.error).toBe("shell.exec denied: not on the allowlist"); + expect(output.writable).toBe(true); + }); +}); diff --git a/packages/computer/src/tools/exec.ts b/packages/computer/src/tools/exec.ts index efbb93c1..4e1354f4 100644 --- a/packages/computer/src/tools/exec.ts +++ b/packages/computer/src/tools/exec.ts @@ -5,7 +5,7 @@ export interface ExecWorkspaceLike { runtime: { exec( command: string, - options: { cwd?: string; encoding: "utf8"; backend?: string }, + options: { cwd?: string; encoding: "utf8"; backend?: string; writable?: boolean }, ): Promise<{ result(): Promise<{ exitCode: number; @@ -20,11 +20,33 @@ export interface ExecBackendDescription { description: string; } +export interface ExecToolInput { + command: string; + cwd?: string; + backend: string; +} + export interface ExecToolOptions { workspace: ExecWorkspaceLike; backends: Record; defaultBackend: string; maxBytes?: number; + + // Decides whether a command may modify the workspace. Omit to let + // every command write, which is the behaviour without this option. + // + // Deliberately not part of the input schema, so the model cannot + // set it. A model that classifies its own command is the failure + // this is meant to catch: the whole point is the command mislabelled + // as read-only, and asking the same model that mislabelled it to + // declare the label would make the flag agree with the mistake. The + // host decides — from an allowlist, a plan step, a human, whatever + // it already trusts — and the model finds out by the write failing. + // + // The classification is still allowed to be wrong. It fails safe in + // that direction: a read command marked read-only runs fine, and a + // write command marked read-only fails visibly instead of writing. + writable?: (input: ExecToolInput) => boolean; } const DEFAULT_MAX_BYTES = 64 * 1024; @@ -77,26 +99,38 @@ export function createExecTool( }), execute: async ({ command, cwd, backend }) => { const selectedBackend = backend ?? options.defaultBackend; + const writable = options.writable?.({ command, cwd, backend: selectedBackend }) ?? true; try { const handle = await options.workspace.runtime.exec(command, { cwd, encoding: "utf8", backend: selectedBackend, + writable, }); const result = await handle.result(); return { command, cwd: cwd ?? null, backend: selectedBackend, + // Reported so the model can tell a refused write from a + // broken command. Without it a read-only run looks like an + // arbitrary failure and the model's next move is to retry + // the same command. + writable, exitCode: result.exitCode, stdout: truncate(result.stdout, maxBytes), stderr: truncate(result.stderr, maxBytes), }; } catch (err) { + // A gate refusing the command arrives here. It is returned as + // a tool result rather than thrown, like every other failure, + // so the model reads the refusal and can respond to it instead + // of the agent loop tearing down. return { command, cwd: cwd ?? null, backend: selectedBackend, + writable, error: err instanceof Error ? err.message : String(err), }; } From a0d71ac2b650c63c60d6391595673a0fd0b14b68 Mon Sep 17 00:00:00 2001 From: Antoni T Date: Mon, 3 Aug 2026 15:29:32 +0100 Subject: [PATCH 08/13] computer: honour write access in the worker-javascript backend `ModuleExecutionInput.writable` narrows the capability handed to a module execution. Intersected with the backend's configured `access` rather than replacing it. A backend registered read-only stays read-only however the call was made, and an execution asking to be read-only gets that on a read-write backend. Neither side can widen the other, which is the same rule the gate follows. Like the shell backend, this one shares the host store, so the refusal lands where the write is attempted. The bridge answers the refused capability call with an error payload and the generated guest shim throws it inside the module, so the module sees an ordinary filesystem error rather than a silent no-op. The tests assert the file does not appear, with a control that runs the same module with access and confirms it does. A third pins the intersection by asking a read-only backend for a writable execution. --- .../worker-javascript.test.ts | 114 ++++++++++++++++++ .../worker-javascript/worker-javascript.ts | 8 +- packages/computer/src/runtime/runtime.ts | 1 + packages/computer/src/runtime/types.ts | 4 + 4 files changed, 126 insertions(+), 1 deletion(-) diff --git a/packages/computer/src/backends/worker-javascript/worker-javascript.test.ts b/packages/computer/src/backends/worker-javascript/worker-javascript.test.ts index 686070be..037929f8 100644 --- a/packages/computer/src/backends/worker-javascript/worker-javascript.test.ts +++ b/packages/computer/src/backends/worker-javascript/worker-javascript.test.ts @@ -516,6 +516,90 @@ describe("WorkerJavaScriptBackend", () => { expect(events.at(-1)).toMatchObject({ name: "exit", value: 0 }); }); + it("refuses a host write when the execution has no write access", async () => { + // The module backend shares the host store, so the refusal + // happens where the write is attempted rather than afterwards. + const db = new Database(new SQLiteTestStorage()); + initializeSchema(db, () => 0); + const fs = new WorkspaceFilesystem(db); + await fs.mkdir("/workspace", { recursive: true }); + const refusal: string[] = []; + const backend = new WorkerJavaScriptBackend({ loader: writingLoader(refusal) }); + const handle = await backend.connect({ + db, + fs, + git: undefined as never, + artifacts: undefined as never, + }); + + const execution = await handle.exec({ + id: "read-only-write", + source: "export default 1", + writable: false, + }); + for await (const _event of execution.events) { + // drain + } + + await expect(fs.stat("/workspace/output.txt")).rejects.toThrow(); + // The bridge answers a refused capability call with an error + // payload; the generated guest shim is what turns that into a + // thrown error inside the module. This asserts the payload, since + // the fake module here stands in for the shim. + expect(refusal).toHaveLength(1); + expect(JSON.parse(refusal[0]).error.message).toMatch(/write access is not available/); + }); + + it("allows the same host write when the execution has write access", async () => { + // The control: without it the test above would pass against a + // module that never managed to write in the first place. + const db = new Database(new SQLiteTestStorage()); + initializeSchema(db, () => 0); + const fs = new WorkspaceFilesystem(db); + await fs.mkdir("/workspace", { recursive: true }); + const backend = new WorkerJavaScriptBackend({ loader: writingLoader() }); + const handle = await backend.connect({ + db, + fs, + git: undefined as never, + artifacts: undefined as never, + }); + + const execution = await handle.exec({ id: "writable-write", source: "export default 1" }); + for await (const _event of execution.events) { + // drain + } + + expect(await fs.readFile("/workspace/output.txt", "utf8")).toBe("done"); + }); + + it("keeps a read-only backend read-only when an execution asks to write", async () => { + // Intersection, not replacement. A backend registered read-only + // cannot be talked into write access by the call. + const db = new Database(new SQLiteTestStorage()); + initializeSchema(db, () => 0); + const fs = new WorkspaceFilesystem(db); + await fs.mkdir("/workspace", { recursive: true }); + const backend = new WorkerJavaScriptBackend({ loader: writingLoader(), access: "read" }); + const handle = await backend.connect({ + db, + fs, + git: undefined as never, + artifacts: undefined as never, + }); + + const execution = await handle.exec({ + id: "read-backend", + source: "export default 1", + writable: true, + }); + for await (const _event of execution.events) { + // drain + } + + await expect(fs.stat("/workspace/output.txt")).rejects.toThrow(); + }); + it("streams stdout before user code returns", async () => { const db = new Database(new SQLiteTestStorage()); initializeSchema(db, () => 0); @@ -992,3 +1076,33 @@ describe("WorkerJavaScriptBackend", () => { expect(load).not.toHaveBeenCalled(); }); }); + +// A loader whose module asks the host to write one file. Used by the +// write-access tests: whether the file exists afterwards is the whole +// assertion. `payloads` collects the raw bridge responses so a test can +// check the refusal the guest shim would have thrown on. +function writingLoader(payloads: string[] = []) { + return { + load() { + return { + getEntrypoint() { + return { + async evaluate( + _input: unknown, + host: { + call(name: string, args: string): Promise; + assertResult(value: unknown): Promise; + attachOutput(readable: ReadableStream): Promise; + }, + ) { + payloads.push( + await host.call("fs.writeFile", JSON.stringify(["/workspace/output.txt", "done"])), + ); + return evaluateResult(host, 1); + }, + }; + }, + }; + }, + }; +} diff --git a/packages/computer/src/backends/worker-javascript/worker-javascript.ts b/packages/computer/src/backends/worker-javascript/worker-javascript.ts index d0bdf43a..4e753b9b 100644 --- a/packages/computer/src/backends/worker-javascript/worker-javascript.ts +++ b/packages/computer/src/backends/worker-javascript/worker-javascript.ts @@ -333,10 +333,16 @@ class JavaScriptBackendHandle implements WorkspaceModuleBackendHandle { this.#pendingIds.add(id); let admittedRecord: ExecutionRecord | undefined; try { + // Intersection, not replacement: a backend configured read-only + // stays read-only however this execution was called, and an + // execution asking to be read-only gets that even on a + // read-write backend. Neither side can widen the other. + const access: WorkspaceRuntimeAccess = + input.writable === false ? "read" : this.#options.access; const capability = new WorkspaceRuntimeCapability( this.#host.fs, this.#options.root, - this.#options.access, + access, this.#options.maxCapabilityBytes, this.#options.maxDirectoryEntries, ); diff --git a/packages/computer/src/runtime/runtime.ts b/packages/computer/src/runtime/runtime.ts index db2c75f7..cae03914 100644 --- a/packages/computer/src/runtime/runtime.ts +++ b/packages/computer/src/runtime/runtime.ts @@ -81,6 +81,7 @@ export class WorkspaceRuntime { env: options.env, stdin: options.stdin, timeoutMs: options.timeoutMs, + writable: options.writable, }); return wrapModuleHandle(runtime, backend, envelope.id, envelope.events, options.encoding); } diff --git a/packages/computer/src/runtime/types.ts b/packages/computer/src/runtime/types.ts index 82f03004..153e0daf 100644 --- a/packages/computer/src/runtime/types.ts +++ b/packages/computer/src/runtime/types.ts @@ -149,6 +149,10 @@ export interface ModuleExecutionInput { env?: Record; stdin?: Uint8Array | string; timeoutMs?: number; + // Whether this execution may modify the workspace. Defaults to + // true. Intersected with the backend's configured access, so a + // backend registered read-only stays read-only. + writable?: boolean; } export interface ModuleExecutionEnvelope { From eebd8b147a8cb9e671a27f3ac9a9e94bca797392 Mon Sep 17 00:00:00 2001 From: Antoni T Date: Mon, 3 Aug 2026 15:30:41 +0100 Subject: [PATCH 09/13] docs: specify write access and the approval seams A new chapter for per-command write access, the gate consulted before an action, and the audit hook notified after it, plus the surface changes in the runtime and tool chapters and a section in the package README. The chapter leads with why classifying commands is not the thing being attempted. Correct classification is not achievable; making a wrong classification safe is. That framing is what explains the rest of the design, including why the capability is per command rather than per write and why a gate cannot ask a human once the command is running. The per-backend enforcement table is the part worth reading twice. What `writable: false` costs a command is not the same everywhere: backends sharing the host store refuse the write where it happens, and a container writes to its own copy first and has the change refused on the way back. The second is weaker and leaves the two copies disagreeing. Documenting that plainly is better than a sentence implying the flag prevents writes everywhere. The `EROFS` row in the filesystem chapter said no code path throws it. One does now, so it describes the two cases that reach it. --- docs/04_filesystem_interface.md | 2 +- docs/05_runtime_interface.md | 5 + docs/09_tool_interface.md | 9 +- docs/20_approval.md | 218 ++++++++++++++++++++++++++++++++ docs/README.md | 1 + packages/computer/README.md | 38 ++++++ 6 files changed, 270 insertions(+), 3 deletions(-) create mode 100644 docs/20_approval.md diff --git a/docs/04_filesystem_interface.md b/docs/04_filesystem_interface.md index 1e40b11c..464d043d 100644 --- a/docs/04_filesystem_interface.md +++ b/docs/04_filesystem_interface.md @@ -282,7 +282,7 @@ so handlers from Node code port over directly. | `EPERM` | Operation is forbidden, e.g. deleting the workspace root. | | `EIO` | Backing storage failed unexpectedly. | | `EACCES` | *Reserved for future mount layer (see [06. Mount Interface](./06_mount_interface.md)).* No code path in `workspace-fs` currently throws it. | -| `EROFS` | *Reserved for future mount layer (see [06. Mount Interface](./06_mount_interface.md)).* No code path in `workspace-fs` currently throws it. | +| `EROFS` | The write was refused. Either the path is under a read-only mount root (see [06. Mount Interface](./06_mount_interface.md)), or the filesystem handle has no write access because the command holding it is running read-only (see [20. Write access and approval](./20_approval.md)). | ### Example: handle "file missing" and bubble everything else diff --git a/docs/05_runtime_interface.md b/docs/05_runtime_interface.md index 64a57485..344510d2 100644 --- a/docs/05_runtime_interface.md +++ b/docs/05_runtime_interface.md @@ -33,6 +33,7 @@ interface WorkspaceRuntimeExecOptions { timeoutMs?: number; env?: Record; stdin?: Uint8Array | string; + writable?: boolean; } interface WorkspaceRuntimeExecHandle extends ReadableStream { @@ -88,6 +89,10 @@ await workspace.runtime.exec( Omitting `backend` selects the first configured backend. Backend selection is routing, not authorization; public gateways must validate it against server-side policy. +## Write access + +`writable` defaults to true. Pass `false` for a command expected only to read, and a write it attempts fails rather than lands. What that costs the command depends on the backend: `worker-shell` and `worker-javascript` share the host store and refuse the write where it happens, while a Container writes to its own copy and has the change refused on the way back, reported in `skipped` with reason `no-write-access`. A configured gate can also withdraw write access from a command that asked for it. See [20. Write access and approval](./20_approval.md). + ## Command synchronization Command backends continue to use the existing synchronization bracket: diff --git a/docs/09_tool_interface.md b/docs/09_tool_interface.md index eeec7ca2..24364f8a 100644 --- a/docs/09_tool_interface.md +++ b/docs/09_tool_interface.md @@ -204,6 +204,7 @@ createExecTool({ backends, defaultBackend, maxBytes?, + writable?, }); ``` @@ -212,6 +213,7 @@ createExecTool({ | `backends` | required | Map of backend id to a model-facing description. | | `defaultBackend` | required | Backend used when the model omits `backend`. Must be a key in `backends`. | | `maxBytes` | 64 KiB | UTF-8 byte cap for each of stdout and stderr. | +| `writable` | every command may write | `({ command, cwd, backend }) => boolean`. Decides whether a command may modify the workspace. | Schema: @@ -223,22 +225,25 @@ Schema: } ``` -Calls `workspace.runtime.exec(command, { cwd, encoding: "utf8", backend })`, waits for `result()`, and returns: +Calls `workspace.runtime.exec(command, { cwd, encoding: "utf8", backend, writable })`, waits for `result()`, and returns: ```ts { command: string; cwd: string | null; backend: string; + writable: boolean; exitCode: number; stdout: string; stderr: string; } ``` +`writable` is resolved by the host, not by the model, and is deliberately absent from the schema. The case it defends against is the command mislabelled as read-only, so a label the model supplies would agree with the mistake. It is reported back on the result so the model can tell a refused write from a broken command instead of retrying the same thing. A gate refusal arrives as an `error` field rather than a thrown error, so the agent loop survives it. See [20. Write access and approval](./20_approval.md). + `exec` is opt-in. `createAITools()` includes it only when the caller passes `shell` options and `readonly` is not true. The backend descriptions are included in the tool description so the model can choose the cheapest backend that can run the command. -Wire this tool up carefully: it executes arbitrary shell commands inside the configured backend. Use `readonly: true` for inspection-only agents, or omit `shell` when command execution is not part of the agent's job. +Wire this tool up carefully: it executes arbitrary shell commands inside the configured backend. Use `readonly: true` for inspection-only agents, or omit `shell` when command execution is not part of the agent's job. A `writable` resolver narrows what a command can do but is not a substitute for either: it stops a command classified read-only from writing, and does nothing about one classified writable. ## `publish` diff --git a/docs/20_approval.md b/docs/20_approval.md new file mode 100644 index 00000000..b7e9fab4 --- /dev/null +++ b/docs/20_approval.md @@ -0,0 +1,218 @@ +# 20. Write access and approval + +Two related things: a per-command write capability, and a pair of hooks +for deciding and recording what a workspace is asked to do. + +## The problem + +An agent decides a command is read-only and runs it. Sometimes it is +wrong — an alias, a shell function, a `&&` it did not parse, a script +that writes a lockfile on the way to printing a version. The workspace +is modified, nothing reports it, and the mistake is found later by +whatever breaks next. + +Classifying commands correctly is not solvable. What is solvable is +making the classification safe to get wrong: a command believed to be +read-only runs without write access, so if the belief was wrong the +write fails instead of landing. + +## Per-command write access + +`writable` on an exec call. Defaults to true. + +```ts +const handle = await workspace.runtime.exec("git log --oneline", { + writable: false, +}); +``` + +The capability is per command, not per write. `rm -rf` issues one call +per entry; denying partway through leaves a half-deleted tree. A +command is the smallest unit that can be refused and leave the +workspace in a state the caller can reason about. + +It travels with the command rather than being read from configuration, +because commands overlap. A read-only command running beside a writable +one must not be able to disarm it, and must not be able to borrow its +access. + +### What "no write access" enforces, per backend + +Not the same thing everywhere, and the difference matters when +choosing a backend for work you intend to constrain. + +| Backend | Enforcement | Effect of a write | +|---|---|---| +| `worker-shell` | Preventive | Fails inside the command with `EROFS`. Nothing is written. | +| `worker-javascript` | Preventive | The capability handed to the module is narrowed, and the guest shim throws the refusal inside the module. | +| `container-shell` | After the fact | Lands in the container's own copy, then is refused on the way back and reported in `skipped`. | + +`worker-shell` shares the host store, so a command that runs there +holds a filesystem handle built without the capability, and the first +write fails where it happens. The command sees an ordinary filesystem +error and reports it like any other. + +For `worker-javascript` the per-execution flag is intersected with the +backend's own `access` option rather than replacing it. A backend +registered `access: "read"` stays read-only however the call was made, +and an execution asking to be read-only gets that on a read-write +backend. Neither side widens the other, which is the same rule the gate +follows. + +A container has its own copy of the files. By the time the host hears +about a change the container has already written it, so there is +nothing left to prevent — only to refuse. The changes are dropped on +arrival and listed in `skipped` with reason `no-write-access`: + +```ts +const result = await handle.result(); +for (const entry of result.skipped) { + if (entry.reason === "no-write-access") { + // The command wrote this. It was not applied. + } +} +``` + +Refusing is discarding, not deferring. A refused change is not +redelivered to the next pull that does have write access, or the +refusal would only be a delay. The consequence is that the container's +copy and the workspace disagree from that point on: the container still +holds the file it wrote. Treat a refused write there as a reason to +discard the container rather than keep using it. + +## The gate + +Consulted before an action, and able to refuse it. + +```ts +new Workspace({ + storage: ctx.storage, + gate: { + async check(action) { + if (action.kind !== "shell.exec") return { allow: true }; + if (isDestructive(action.command)) { + return { allow: false, reason: "destructive commands need review" }; + } + return { allow: true }; + }, + }, +}); +``` + +A refusal throws `ActionDeniedError`, which carries the action and the +reason. Nothing runs. + +A gate may also allow an action with write access withdrawn, which is +the answer for a command a policy will run but not trust: + +```ts +return { allow: true, writable: false }; +``` + +Narrowing only. A gate cannot grant access an action did not ask for, +so a read-only exec stays read-only regardless of what the gate +returns. + +`check` may be async, and the action does not start until it settles — +long enough to consult a policy service or wait for a human. Whatever +it waits on holds up the caller. + +A gate that throws propagates. A gate that could not reach a decision +is not a gate that said no, and code that cannot tell those apart will +eventually treat an outage as permission. + +### What is gated + +`shell.exec`, once per command, and the mutating methods on +`Workspace.fs` — `writeFile`, `mkdir`, `rm`, `chmod`, `symlink` — once +per call. Reads are not gated. + +The filesystem half is there because `Workspace.fs` writes to the store +without crossing the wire. A gate covering only `shell.exec` would have +an obvious way around it: deny the command, write the file directly. + +Filesystem calls are gated individually because each call is the whole +action, so refusing one leaves nothing half-finished. That is the +difference from a command, and the reason a command is gated once. + +A gate should return `{ allow: true }` for action kinds it does not +recognise, so kinds added later stay permitted rather than being +refused by a gate that was never asked about them. + +### Asking a human + +Ask before the command starts. Once it is running there is nowhere to +suspend it that does not risk a partial result, and a write-by-write +prompt would ask hundreds of times for one `rm -rf`. + +## The audit hook + +Notified after an action has been decided, and after it has run. + +```ts +new Workspace({ + storage: ctx.storage, + audit: { + record(action, outcome) { + log({ kind: action.kind, status: outcome.status }); + }, + }, +}); +``` + +`outcome.status` is `allowed`, `denied`, or `failed`. Refused actions +are reported too, and are usually the more interesting half. + +It cannot deny anything, and errors it throws are swallowed. By the +time it runs the action has already happened; failing the caller over a +failed log entry would make an audit hook into a gate. + +For `shell.exec` it fires on the spawn, not on the exit. `exec` returns +a detached handle the caller may never drain, so there is no later +moment guaranteed to arrive, and picking one would mean a command that +is dropped is never recorded at all. What the command went on to do is +on the observer's span and on the result. + +## Why this is not the observer + +The observer in +[11. Lifecycle](./11_lifecycle.md) must return its callback's result +unchanged — observability that changes behaviour is a bug. A gate +exists to change behaviour. They are separate seams with the same shape +and opposite licences, rather than one seam with a weakened contract. + +## Tool layer + +`createExecTool` takes a `writable` resolver. It is not part of the +tool's input schema, so the model cannot set it: + +```ts +createExecTool({ + workspace, + backends, + defaultBackend: "worker-shell", + writable: ({ command }) => !readOnlyCommand(command), +}); +``` + +The model must not classify its own command. The case being defended +against is the command mislabelled as read-only, and asking the model +that mislabelled it to declare the label produces a flag that agrees +with the mistake. The host decides from something it already trusts, +and the model finds out by the write failing. + +The effective access is reported on the tool result, so a model can +tell a refused write from a broken command instead of retrying the same +thing. A gate refusal comes back as a tool result too, not a thrown +error, so the agent loop survives it. + +## Related + +- [04. Filesystem interface](./04_filesystem_interface.md) — `EROFS` + and the filesystem surface. +- [05. Runtime interface](./05_runtime_interface.md) — exec options and + the synchronization bracket. +- [06. Mount interface](./06_mount_interface.md) — read-only mounts, + which are a fixed property of a path rather than a per-command + decision. Both apply, and neither is a way around the other. +- [09. Tool interface](./09_tool_interface.md) — the agent-facing tools. diff --git a/docs/README.md b/docs/README.md index ffdf2c5b..776117e0 100644 --- a/docs/README.md +++ b/docs/README.md @@ -243,6 +243,7 @@ above, then dive into the area you're working on. | [17. Isolate JavaScript runtime](./17_isolate_javascript.md) | ECMAScript modules, durable imports, configured libraries, durable `node:fs/promises`, trusted `ws:git` / `ws:artifacts`, and managed lifecycle. | | [18. Runtime migration](./18_runtime_migration.md) | Breaking preview-API mappings from public shell and script-execution surfaces to `workspace.runtime`. | | [19. Performance](./19_performance.md) | Filesystem benchmarks: `fs-bench` numbers, an `npm install` comparison, and how to reproduce them. | +| [20. Write access and approval](./20_approval.md) | Per-command write access, the gate consulted before an action, and the audit hook notified after it. | ## High-level API diff --git a/packages/computer/README.md b/packages/computer/README.md index 1075d5f7..278b074c 100644 --- a/packages/computer/README.md +++ b/packages/computer/README.md @@ -470,6 +470,44 @@ default is a zero-cost no-op, so there's no overhead unless you opt in. An adapter for the Cloudflare runtime lives at `@cloudflare/computer/observe/cloudflare`. +### Write access and approval + +Pass `writable: false` to an exec call for a command you expect to only +read. A write it attempts then fails instead of landing, which is what +makes a wrong guess about a command safe: + +```ts +await workspace.runtime.exec("git log --oneline", { writable: false }); +``` + +`worker-shell` and `worker-javascript` share the host store, so the +write fails inside the command. A container has its own copy and writes +there first, so the change is refused on the way back and reported in +`result.skipped` with reason `no-write-access`. + +Pass a `gate` to be consulted before each command and each mutating +`workspace.fs` call, with the option to refuse it or to withdraw its +write access, and an `audit` hook to be told what was decided: + +```ts +new Workspace({ + storage: ctx.storage, + gate: { + check(action) { + if (action.kind === "shell.exec" && isDestructive(action.command)) { + return { allow: false, reason: "needs review" }; + } + return { allow: true }; + }, + }, + audit: { record: (action, outcome) => log(action, outcome) }, +}); +``` + +Both default to no-ops. They are separate from `observer` because an +observer must not change what happens and a gate exists to. See +[20. Write access and approval](../../docs/20_approval.md). + ## Examples - [`examples/worker-shell`](../../examples/worker-shell) — the From 18b3e591964f8df3b09b46b100400bf04c15581c Mon Sep 17 00:00:00 2001 From: Antoni T Date: Mon, 3 Aug 2026 18:00:54 +0100 Subject: [PATCH 10/13] computer: export the exec tool's input and workspace types ExecToolOptions.writable is a function of ExecToolInput, and ExecToolOptions.workspace is an ExecWorkspaceLike, but neither type left the package. A host writing the resolver the previous commit added had no way to name its argument, which is the one thing it has to do to use the option at all. Found by writing that resolver outside the package for the first time. --- packages/computer/src/tools/index.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/computer/src/tools/index.ts b/packages/computer/src/tools/index.ts index 45dc297c..22e9dff8 100644 --- a/packages/computer/src/tools/index.ts +++ b/packages/computer/src/tools/index.ts @@ -1,5 +1,11 @@ export { type CreateAIToolsOptions, createAITools } from "./ai.js"; -export { createExecTool, type ExecBackendDescription, type ExecToolOptions } from "./exec.js"; +export { + createExecTool, + type ExecBackendDescription, + type ExecToolInput, + type ExecToolOptions, + type ExecWorkspaceLike, +} from "./exec.js"; export { createEditTool, type EditToolOptions } from "./fs/edit.js"; export { createListTool, type ListToolOptions } from "./fs/list.js"; export { createReadTool, type ReadToolOptions } from "./fs/read.js"; From 14be5e7e86c26bba78bc807f3e7030bb4c7369fa Mon Sep 17 00:00:00 2001 From: Antoni T Date: Tue, 4 Aug 2026 15:44:21 +0100 Subject: [PATCH 11/13] computerd: report a refused write as EROFS rather than EIO The FUSE driver's errno table had no entry for EROFS, so toErrno fell through to its EIO default. Two separate refusals reach it. A read-only mount root rejects writes at the dofs data layer, which is where changes arriving from a container through the sync path are caught, and a filesystem handle built without the write capability rejects every mutation for as long as it exists. Both raise EROFS, and both reached the guest as an input/output error. The distinction matters to whoever reads the message. EROFS is a fact about the workspace the caller can act on: this tree does not take writes. EIO says the daemon or the disk is broken, which invites a retry that fails the same way every time. An agent reading the error suffers most, since it will go looking for a way around a fault that has none. EACCES was missing for the same reason and is added alongside it. --- packages/computerd/src/fuse/driver.test.ts | 34 ++++++++++++++++++++++ packages/computerd/src/fuse/driver.ts | 11 +++++++ 2 files changed, 45 insertions(+) diff --git a/packages/computerd/src/fuse/driver.test.ts b/packages/computerd/src/fuse/driver.test.ts index 97f377b3..06fa9e1b 100644 --- a/packages/computerd/src/fuse/driver.test.ts +++ b/packages/computerd/src/fuse/driver.test.ts @@ -1,7 +1,11 @@ +import { invalidateReadOnlyMountCache } from "@cloudflare/dofs"; import { expect, test } from "vitest"; import { createNodeVirtualFileSystem, makeFUSEOps } from "./index.js"; +const EROFS = -30; +const EACCES = -13; + const callback = (fn: (cb: (errno: number, result: unknown) => void) => void) => new Promise<{ errno: number; result: unknown }>((resolve) => fn((errno, result) => resolve({ errno, result })), @@ -1044,3 +1048,33 @@ test("FUSE ftruncate does not depend on fuse-native binding this", async () => { expect(await status((cb) => ops.flush("/ftruncate-this.txt", fh, cb))).toBe(0); expect(Buffer.from(vfs.readFileSync("/ftruncate-this.txt")).toString("utf8")).toBe("abc"); }); + +// A write into a read-only mount root is refused by dofs with EROFS. +// The guest has to see that code: EROFS says "this tree does not take +// writes", which is a fact about the workspace the caller can act on, +// while EIO says the daemon or the disk is broken, which invites a +// retry that will fail the same way forever. +test("FUSE reports a refused write into a read-only mount as EROFS", async () => { + const { vfs, db } = await createNodeVirtualFileSystem(); + vfs.mkdirSync("/ro", { recursive: true }); + db.run("INSERT INTO _vfs_mounts (root, kind, mode) VALUES (?, ?, ?)", "/ro", "r2", "read-only"); + invalidateReadOnlyMountCache(db); + const ops = makeFUSEOps(vfs); + + expect(await status((cb) => ops.mkdir("/ro/nested", 0o755, cb))).toBe(EROFS); + expect(await status((cb) => ops.unlink("/ro/gone.txt", cb))).toBe(EROFS); +}); + +// Same reasoning for a path the caller is not permitted to touch: +// EACCES is actionable, EIO is not. +test("FUSE preserves EACCES rather than flattening it to EIO", async () => { + const { vfs } = await createNodeVirtualFileSystem(); + const denied = Object.assign(Object.create(Object.getPrototypeOf(vfs)), vfs, { + mkdirSync() { + throw Object.assign(new Error("permission denied"), { code: "EACCES" }); + }, + }); + const ops = makeFUSEOps(denied); + + expect(await status((cb) => ops.mkdir("/denied", 0o755, cb))).toBe(EACCES); +}); diff --git a/packages/computerd/src/fuse/driver.ts b/packages/computerd/src/fuse/driver.ts index afefd202..89a89a73 100644 --- a/packages/computerd/src/fuse/driver.ts +++ b/packages/computerd/src/fuse/driver.ts @@ -13,6 +13,8 @@ const ERRNO = { EISDIR: -21, EINVAL: -22, EPERM: -1, + EACCES: -13, + EROFS: -30, EFBIG: -27, ENOTEMPTY: -39, ENODATA: -61, @@ -1078,6 +1080,13 @@ function statNode(stat: { }; } +// Falling back to EIO is right for an error this layer does not +// recognise, and wrong for a refusal. A refusal is a fact about the +// workspace that the caller can act on — the tree does not take +// writes, or this caller may not make them — while EIO reads as a +// broken daemon and invites a retry that fails identically. dofs +// raises EROFS for both a read-only mount root and a handle without +// the write capability, so those have to survive the translation. function toErrno(error: unknown): number { const code = typeof error === "object" && error !== null && "code" in error ? String(error.code) : undefined; @@ -1088,5 +1097,7 @@ function toErrno(error: unknown): number { if (code === "ENOTEMPTY") return ERRNO.ENOTEMPTY; if (code === "EINVAL") return ERRNO.EINVAL; if (code === "EPERM") return ERRNO.EPERM; + if (code === "EACCES") return ERRNO.EACCES; + if (code === "EROFS") return ERRNO.EROFS; return ERRNO.EIO; } From 6a334d5c75d0650aaa0ef9df9129dab2794c8f0b Mon Sep 17 00:00:00 2001 From: Antoni T Date: Tue, 4 Aug 2026 15:49:55 +0100 Subject: [PATCH 12/13] computer: report writes the sync refused A backend holding its own copy of the files cannot be stopped from writing when the command has no write access. It writes, exits zero, and the changes are dropped when they are pulled back. ExecResult has carried those entries in skipped since the capability landed, but the exec tool never passed them on, so a caller saw a clean success and a model reported work that never happened. Silence is the wrong answer for the one backend where the refusal arrives late. The tool now returns discardedWrites when the pull refused anything: how many entries, why, and a capped sample of the paths. The reason separates a command that ran without the capability from one that reached into a read-only mount root, because the two call for different responses and one word for both would hide that. The paths are capped at ten while the count stays honest, since one refused entry per file means a recursive change can produce thousands. The key is absent rather than null when nothing was refused. Every key in a tool result is context a model pays for on every call, and nothing refused is the usual case. --- packages/computer/src/tools/exec.test.ts | 103 ++++++++++++++++++++++- packages/computer/src/tools/exec.ts | 46 ++++++++++ 2 files changed, 148 insertions(+), 1 deletion(-) diff --git a/packages/computer/src/tools/exec.test.ts b/packages/computer/src/tools/exec.test.ts index d66144d5..75deb8fa 100644 --- a/packages/computer/src/tools/exec.test.ts +++ b/packages/computer/src/tools/exec.test.ts @@ -8,8 +8,15 @@ interface ExecCall { writable: boolean | undefined; } +interface FakeResult { + exitCode: number; + stdout: string; + stderr: string; + skipped?: Array<{ path: string; op: "write" | "delete"; reason: string }>; +} + function fakeWorkspace( - result: { exitCode: number; stdout: string; stderr: string } | Error = { + result: FakeResult | Error = { exitCode: 0, stdout: "", stderr: "", @@ -134,3 +141,97 @@ describe("createExecTool — write access", () => { expect(output.writable).toBe(true); }); }); + +describe("createExecTool — refused writes", () => { + // A backend that keeps its own copy of the files does not fail the + // command when it lacks write access. The command writes, exits 0, + // and the changes are refused on the way back. Without this field + // the model reads that as a clean success and reports work it did + // not do. + it("tells the model when the sync refused the command's writes", async () => { + const ws = fakeWorkspace({ + exitCode: 0, + stdout: "", + stderr: "", + skipped: [ + { path: "/workspace/a.txt", op: "write", reason: "no-write-access" }, + { path: "/workspace/b.txt", op: "write", reason: "no-write-access" }, + ], + }); + + const result = await run( + toolFor(ws, () => false), + { command: "printf hi > /workspace/a.txt" }, + ); + + expect(result.discardedWrites).toEqual({ + count: 2, + reason: "no-write-access", + paths: ["/workspace/a.txt", "/workspace/b.txt"], + }); + }); + + it("names the read-only mount case separately, since the fix differs", async () => { + const ws = fakeWorkspace({ + exitCode: 0, + stdout: "", + stderr: "", + skipped: [{ path: "/workspace/ro/a.txt", op: "write", reason: "read-only" }], + }); + + const result = await run(toolFor(ws), { command: "printf hi > /workspace/ro/a.txt" }); + + expect(result.discardedWrites).toMatchObject({ count: 1, reason: "read-only" }); + }); + + it("reports a mixed batch as mixed rather than picking one", async () => { + const ws = fakeWorkspace({ + exitCode: 0, + stdout: "", + stderr: "", + skipped: [ + { path: "/workspace/a.txt", op: "write", reason: "no-write-access" }, + { path: "/workspace/ro/b.txt", op: "write", reason: "read-only" }, + ], + }); + + const result = await run(toolFor(ws), { command: "sh ./write-both.sh" }); + + expect(result.discardedWrites).toMatchObject({ count: 2, reason: "mixed" }); + }); + + // The field is absent rather than empty on the common path: every + // key here is context the model pays for on every single call. + it("says nothing when the command's writes all landed", async () => { + const ws = fakeWorkspace({ exitCode: 0, stdout: "ok", stderr: "", skipped: [] }); + const result = await run(toolFor(ws), { command: "printf hi > /workspace/a.txt" }); + expect(result).not.toHaveProperty("discardedWrites"); + }); + + it("says nothing when the backend reports no sync stats at all", async () => { + const ws = fakeWorkspace({ exitCode: 0, stdout: "ok", stderr: "" }); + const result = await run(toolFor(ws), { command: "ls" }); + expect(result).not.toHaveProperty("discardedWrites"); + }); + + // One refused write per file means a recursive change can produce + // thousands. The count stays honest; the list is a sample. + it("caps the path list but keeps the true count", async () => { + const skipped = Array.from({ length: 25 }, (_, i) => ({ + path: `/workspace/f${i}.txt`, + op: "write" as const, + reason: "no-write-access" as const, + })); + const ws = fakeWorkspace({ exitCode: 0, stdout: "", stderr: "", skipped }); + + const result = await run( + toolFor(ws, () => false), + { command: "sh ./many.sh" }, + ); + + const discarded = result.discardedWrites as { count: number; paths: string[] }; + expect(discarded.count).toBe(25); + expect(discarded.paths).toHaveLength(10); + expect(discarded.paths[0]).toBe("/workspace/f0.txt"); + }); +}); diff --git a/packages/computer/src/tools/exec.ts b/packages/computer/src/tools/exec.ts index 4e1354f4..f83d987f 100644 --- a/packages/computer/src/tools/exec.ts +++ b/packages/computer/src/tools/exec.ts @@ -11,11 +11,52 @@ export interface ExecWorkspaceLike { exitCode: number; stdout: string; stderr: string; + // Changes the post-command pull refused. Optional because a + // backend that shares the workspace store never produces any: + // its writes fail where they happen, so there is nothing left + // to refuse on the way back. + skipped?: ReadonlyArray<{ path: string; op: "write" | "delete"; reason: string }>; }>; }>; }; } +// Refused writes reported back to the model. +// +// A backend that keeps its own copy of the files cannot be stopped +// from writing when it has no write access. It writes, exits zero, +// and the changes are dropped when they are pulled back. Nothing in +// the exit code or the output says so, so a model that is not told +// reports work it did not do. +// +// `count` is every refused entry; `paths` is a capped sample, because +// one entry per file means a recursive change can produce thousands +// and the model is paying for each one. +export interface DiscardedWrites { + count: number; + // "no-write-access" — the command ran without the capability. + // "read-only" — it reached into a read-only mount root. + // Different fixes, so they are not collapsed into one word. + reason: "no-write-access" | "read-only" | "mixed"; + paths: string[]; +} + +const MAX_REPORTED_PATHS = 10; + +function summarizeSkipped( + skipped: ReadonlyArray<{ path: string; reason: string }> | undefined, +): DiscardedWrites | undefined { + if (skipped === undefined || skipped.length === 0) return undefined; + const reasons = new Set(skipped.map((entry) => entry.reason)); + const reason = + reasons.size === 1 ? (skipped[0].reason as DiscardedWrites["reason"]) : ("mixed" as const); + return { + count: skipped.length, + reason, + paths: skipped.slice(0, MAX_REPORTED_PATHS).map((entry) => entry.path), + }; +} + export interface ExecBackendDescription { description: string; } @@ -108,6 +149,7 @@ export function createExecTool( writable, }); const result = await handle.result(); + const discardedWrites = summarizeSkipped(result.skipped); return { command, cwd: cwd ?? null, @@ -120,6 +162,10 @@ export function createExecTool( exitCode: result.exitCode, stdout: truncate(result.stdout, maxBytes), stderr: truncate(result.stderr, maxBytes), + // Spread so the key is absent rather than null on the + // common path. Every key here is context the model reads on + // every call, and "nothing was refused" is the usual case. + ...(discardedWrites !== undefined ? { discardedWrites } : {}), }; } catch (err) { // A gate refusing the command arrives here. It is returned as From 4d41c2d70a87ad702e862a9fa0b4ad5d2215b6c1 Mon Sep 17 00:00:00 2001 From: Antoni T Date: Wed, 5 Aug 2026 13:29:23 +0100 Subject: [PATCH 13/13] computer: document the second reason a pull skips SkippedEntry became a union of two reasons when the per-command write capability landed: an entry can be refused because it targets a read-only mount root, or because the command ran without write access and the post-command pull refused everything it was offered. The comments on ExecResult.skipped and Workspace.pull still described only the mount case, and ExecResult went further and claimed the field is empty when no read-only mounts are registered. That is wrong for a container command that ran read-only. Describe both reasons, and note that a backend sharing the workspace store never fills the field because it has no pull to refuse. --- packages/computer/src/shell.ts | 11 +++++++---- packages/computer/src/workspace.ts | 3 ++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/computer/src/shell.ts b/packages/computer/src/shell.ts index c682c0e0..b1e7acf2 100644 --- a/packages/computer/src/shell.ts +++ b/packages/computer/src/shell.ts @@ -55,10 +55,13 @@ export interface ExecResult { // VFS sync stats from the docs/05 bracket. // pushed — entries shipped by the pre-exec pushOnce. // pulled — entries the post-drain pullOnce applied locally. - // skipped — entries the post-drain pullOnce did NOT apply - // because they targeted a read-only mount root. - // Empty when no read-only mounts are registered or - // the container stayed clear of them. + // skipped — entries the post-drain pullOnce did not apply, + // either because they targeted a read-only mount + // root or because the command ran without write + // access and the pull refused everything it was + // offered. Always empty for a backend that shares + // the workspace store: there is no pull to refuse, + // and a write fails inside the command instead. // pushed is observed before the stream is returned. The remaining // fields describe the post-command pull when result() is used. pushed: number; diff --git a/packages/computer/src/workspace.ts b/packages/computer/src/workspace.ts index 7c425806..3e2f3043 100644 --- a/packages/computer/src/workspace.ts +++ b/packages/computer/src/workspace.ts @@ -626,7 +626,8 @@ export class Workspace { // pull() returns the dofs ApplyResult { applied, skipped } — // `applied` is the number of entries written into the local // store, `skipped` surfaces remote-side writes the apply path - // rejected because they targeted a read-only mount root. + // rejected, either because they targeted a read-only mount root + // or because the pull ran without write access. // // Both methods emit a `workspace.sync.push` / `workspace.sync.pull` // span on the configured observer, tagged with the resolved