diff --git a/packages/runtime/src/__tests__/filesystem-authority-contract.test.ts b/packages/runtime/src/__tests__/filesystem-authority-contract.test.ts new file mode 100644 index 0000000000..24cb591caf --- /dev/null +++ b/packages/runtime/src/__tests__/filesystem-authority-contract.test.ts @@ -0,0 +1,68 @@ +// Direct unit tests for the filesystem-authority contract module. The +// integration path (executor → ToolOutcomeUnknownError) is covered in +// filesystem-mutation-outcome.test.ts; these tests pin the pure classifier +// and its types at the contract seam. +import assert from 'node:assert/strict'; +import { describe, test } from 'node:test'; + +import { FilesystemWorkerClientError } from '../filesystem-worker/client.js'; +import { classifyFailedMutationOutcome, UNKNOWN_OUTCOME_REASONS } from '../filesystem-authority.js'; + +function workerError( + reason: ConstructorParameters[0]['reason'], + dispatched?: boolean, +): FilesystemWorkerClientError { + return new FilesystemWorkerClientError({ + reason, + stage: 'launch', + requestId: 'test', + ...(dispatched !== undefined ? { dispatched } : {}), + }); +} + +describe('filesystem-authority contract: classifyFailedMutationOutcome', () => { + test('a non-worker error is not classifiable', () => { + assert.equal(classifyFailedMutationOutcome(new Error('boom')), undefined); + assert.equal(classifyFailedMutationOutcome(undefined), undefined); + }); + + test('an aborted error is unknown only after dispatch', () => { + assert.equal(classifyFailedMutationOutcome(workerError('aborted', false)), undefined); + assert.equal(classifyFailedMutationOutcome(workerError('aborted', true)), 'unknown'); + // An aborted error with no flag (the question does not apply) is not unknown. + assert.equal(classifyFailedMutationOutcome(workerError('aborted')), undefined); + }); + + test('every UNKNOWN_OUTCOME_REASONS member maps to unknown', () => { + for (const reason of UNKNOWN_OUTCOME_REASONS) { + // These reasons are semantically dispatched, so the flag is irrelevant. + assert.equal( + classifyFailedMutationOutcome(workerError(reason, true)), + 'unknown', + `${reason} (dispatched=true) should be unknown`, + ); + assert.equal( + classifyFailedMutationOutcome(workerError(reason, undefined)), + 'unknown', + `${reason} (dispatched unset) should still be unknown`, + ); + } + }); + + test('a never-dispatched spawn failure is not unknown', () => { + assert.equal(classifyFailedMutationOutcome(workerError('spawn_failed', false)), undefined); + }); + + test('pre-flight validation failures are not unknown', () => { + assert.equal( + classifyFailedMutationOutcome( + new FilesystemWorkerClientError({ + reason: 'invalid_request', + stage: 'validation', + requestId: 'test', + }), + ), + undefined, + ); + }); +}); diff --git a/packages/runtime/src/__tests__/filesystem-authority.test.ts b/packages/runtime/src/__tests__/filesystem-authority.test.ts index 8184f808da..166384543a 100644 --- a/packages/runtime/src/__tests__/filesystem-authority.test.ts +++ b/packages/runtime/src/__tests__/filesystem-authority.test.ts @@ -273,6 +273,10 @@ describe('file tools follow the execution boundary', () => { const secondKeyResolvedPromise = new Promise((resolve) => { secondKeyResolved = resolve; }); + const pinnedReadModifyWrite = host.readModifyWrite; + if (!pinnedReadModifyWrite) { + throw new Error('LocalWorkspaceExecutor must provide readModifyWrite'); + } const tools = toolsFor({ executor: Object.assign(Object.create(host) as typeof host, { writeLockKey: async (input: Parameters[0]) => { @@ -295,6 +299,23 @@ describe('file tools follow the execution boundary', () => { active -= 1; } }, + readModifyWrite: async (input: Parameters[0]) => { + // The pinned read-modify-write is the mutation's read step now + // (#2600); the causal barrier lives here for the same reason it + // lived on readFile before. + active += 1; + overlapped ||= active > 1; + reads += 1; + if (reads === 1) { + firstReadStarted(); + await secondKeyResolvedPromise; + } + try { + return await pinnedReadModifyWrite(input); + } finally { + active -= 1; + } + }, }), }); const edit = toolNamed(tools, 'Edit'); diff --git a/packages/runtime/src/__tests__/filesystem-mutation-outcome.test.ts b/packages/runtime/src/__tests__/filesystem-mutation-outcome.test.ts new file mode 100644 index 0000000000..b5d2b92746 --- /dev/null +++ b/packages/runtime/src/__tests__/filesystem-mutation-outcome.test.ts @@ -0,0 +1,269 @@ +// Verify that a filesystem mutation whose worker fails after dispatch is +// surfaced as an unknown outcome, while pre-flight failures and read failures +// pass through as ordinary errors. This is the host-side half of issue #2600's +// "post-dispatch unknown outcomes"; the worker-side dispatch flag is exercised +// separately in filesystem-worker-client.test.ts. +import assert from 'node:assert/strict'; +import { mkdtemp, realpath, rename, rm, stat, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, test } from 'node:test'; + +import { ToolOutcomeUnknownError } from '@maka/core/events'; + +import { createBoundaryFilesystemExecutor } from '../filesystem-executor.js'; +import { + FilesystemWorkerClientError, + type FilesystemWorkerClient, + type FilesystemWorkerClientErrorReason, + type FilesystemWorkerExecuteInput, +} from '../filesystem-worker/client.js'; +import type { FilesystemWorkerResult } from '../filesystem-worker/protocol.js'; +import { createLocalWorkspaceExecutor } from '../workspace-executor.js'; + +const cleanup: string[] = []; +afterEach(async () => { + await Promise.all(cleanup.splice(0).map((path) => rm(path, { recursive: true, force: true }))); +}); + +/** A worker whose execute always rejects with a configured client error. */ +function failingWorker( + reason: FilesystemWorkerClientErrorReason, + dispatched?: boolean, +): { execute: () => Promise } { + return { + async execute(): Promise { + throw new FilesystemWorkerClientError({ + reason, + stage: 'launch', + requestId: 'test', + ...(dispatched !== undefined ? { dispatched } : {}), + }); + }, + }; +} + +function executorWith(worker: { + execute: (input: FilesystemWorkerExecuteInput) => Promise; +}) { + return createBoundaryFilesystemExecutor({ + workspace: createLocalWorkspaceExecutor(), + worker: worker as Pick, + }); +} + +describe('filesystem mutation unknown-outcome classification', () => { + // Launch-stage reasons: the child ran. These carry dispatched:true on the + // real error object (the process-runner / client set it), and the filter + // treats membership in UNKNOWN_OUTCOME_REASONS as sufficient. + const launchStageReasons: FilesystemWorkerClientErrorReason[] = [ + 'worker_crashed', + 'worker_io_incomplete', + 'timeout', + 'response_overflow', + ]; + for (const reason of launchStageReasons) { + test(`Write failing with ${reason} (dispatched) becomes an unknown outcome`, async () => { + const fs = executorWith(failingWorker(reason, true)); + await assert.rejects( + fs.execute({ + operation: { kind: 'write', path: '/tmp/maka-outcome-write.txt', content: 'x' }, + cwd: '/tmp', + }), + (error: unknown) => { + assert.ok(error instanceof ToolOutcomeUnknownError, `${reason} should convert`); + assert.ok( + error.cause instanceof FilesystemWorkerClientError, + 'cause should be the original worker error', + ); + return true; + }, + ); + }); + } + + // Protocol-stage reasons: these arrive from the worker-response branch in + // client.ts, which sets dispatched:true explicitly. The filter must still + // convert them even if some future change leaves dispatched unset, because + // these reasons are *semantically* dispatched (the child answered). We do + // NOT pass dispatched in the fixture to prove membership alone suffices. + const protocolStageReasons: FilesystemWorkerClientErrorReason[] = [ + 'invalid_response', + 'response_id_mismatch', + 'response_kind_mismatch', + 'outcome_unknown', + ]; + for (const reason of protocolStageReasons) { + test(`Write failing with ${reason} converts regardless of dispatched flag`, async () => { + // Real path: the worker-response branch sets dispatched:true. Here we + // exercise the stronger guarantee — the reason alone is sufficient. + const fs = executorWith(failingWorker(reason, undefined)); + await assert.rejects( + fs.execute({ + operation: { kind: 'write', path: `/tmp/maka-outcome-${reason}.txt`, content: 'x' }, + cwd: '/tmp', + }), + (error: unknown) => error instanceof ToolOutcomeUnknownError, + ); + }); + } + + test('Write failing with spawn_failed (never dispatched) is NOT an unknown outcome', async () => { + const fs = executorWith(failingWorker('spawn_failed', false)); + await assert.rejects( + fs.execute({ + operation: { kind: 'write', path: '/tmp/maka-outcome-spawn.txt', content: 'x' }, + cwd: '/tmp', + }), + (error: unknown) => { + // spawn_failed means the child never started: nothing could have been + // written, so it must surface as a plain error, not unknown outcome. + assert.ok(!(error instanceof ToolOutcomeUnknownError)); + assert.ok(error instanceof FilesystemWorkerClientError); + return true; + }, + ); + }); + + test('an aborted mutation before dispatch is a clean cancel, not unknown', async () => { + const fs = executorWith(failingWorker('aborted', false)); + await assert.rejects( + fs.execute({ + operation: { kind: 'write', path: '/tmp/maka-outcome-abort-pre.txt', content: 'x' }, + cwd: '/tmp', + }), + (error: unknown) => { + assert.ok(!(error instanceof ToolOutcomeUnknownError)); + assert.ok(error instanceof FilesystemWorkerClientError); + return true; + }, + ); + }); + + test('an aborted mutation after dispatch is an unknown outcome', async () => { + const fs = executorWith(failingWorker('aborted', true)); + await assert.rejects( + fs.execute({ + operation: { kind: 'write', path: '/tmp/maka-outcome-abort-post.txt', content: 'x' }, + cwd: '/tmp', + }), + (error: unknown) => { + assert.ok(error instanceof ToolOutcomeUnknownError); + return true; + }, + ); + }); + + test('apply_patch (delete) failing after dispatch becomes an unknown outcome', async () => { + const fs = executorWith(failingWorker('worker_crashed', true)); + await assert.rejects( + fs.applyPatch({ + operation: { type: 'delete_file', path: '/tmp/maka-outcome-delete.txt' }, + cwd: '/tmp', + }), + (error: unknown) => error instanceof ToolOutcomeUnknownError, + ); + }); + + test('a validation-stage failure is NOT an unknown outcome', async () => { + // Pre-flight validation failures (e.g. invalid_request) happen before any + // dispatch, so they can never have mutated the file. + const worker = { + async execute(): Promise { + throw new FilesystemWorkerClientError({ + reason: 'invalid_request', + stage: 'validation', + requestId: 'test', + }); + }, + }; + const fs = executorWith(worker); + await assert.rejects( + fs.execute({ + operation: { kind: 'write', path: '/tmp/maka-outcome-validation.txt', content: 'x' }, + cwd: '/tmp', + }), + (error: unknown) => { + assert.ok(!(error instanceof ToolOutcomeUnknownError)); + return true; + }, + ); + }); +}); + +describe('filesystem mutation T0 identity capture (queue-window closure)', () => { + // This is the red-line test for issue #2600 concern #1. The identity must be + // captured at lock acquisition (T0), BEFORE waiting for the write lock — not + // re-derived after the lock is granted (T1). To prove that, the test must + // exercise a REAL lock wait: a first mutation blocks inside the worker while + // holding the path's lock, a second mutation queues behind it, and the path + // is replaced while the second one waits. A regression that captures the + // identity at T1 (after the lock is granted) then samples the replacement's + // inode and this test fails; a T0 capture still sees the original inode. + test('a queued mutation receives the pre-replacement identity captured at lock acquisition', async () => { + const cwd = await realpath(await mkdtemp(join(tmpdir(), 'maka-t0-lockwait-'))); + cleanup.push(cwd); + const target = join(cwd, 'file.txt'); + const replacement = join(cwd, 'replacement.txt'); + await writeFile(target, 'original', 'utf8'); + await writeFile(replacement, 'replacement-body', 'utf8'); + + const original = await stat(target, { bigint: true }); + + // The first mutation blocks inside the worker, holding the write lock. + let releaseFirst!: () => void; + const firstGate = new Promise((resolve) => { + releaseFirst = resolve; + }); + let calls = 0; + let queuedIdentity: { dev: string; ino: string } | undefined; + const gatedWorker: { + execute: (input: FilesystemWorkerExecuteInput) => Promise; + } = { + async execute(input) { + calls += 1; + if (calls === 1) { + await firstGate; // hold the path's lock until the swap has happened + return { kind: 'write', ok: true, path: target, bytes: 5 }; + } + // The queued (second) call: record the identity it was handed. + queuedIdentity = input.expectedIdentity; + return { kind: 'write', ok: true, path: target, bytes: 6 }; + }, + }; + const fs = executorWith(gatedWorker); + + // First mutation: acquires the lock and blocks inside the worker. + const first = fs.execute({ + operation: { kind: 'write', path: target, content: 'first' }, + cwd, + }); + await sleep(50); // let the first call reach the worker and hold the lock + + // Second mutation: captures its identity (T0) and queues on the lock. + const second = fs.execute({ + operation: { kind: 'write', path: target, content: 'second' }, + cwd, + }); + await sleep(50); // let the second call finish its T0 capture and queue + + // Replace the path WHILE the second mutation is still waiting for the lock. + await rename(replacement, target); + + // Release the first mutation; the second acquires the lock and dispatches + // with whatever identity its capture step sampled. + releaseFirst(); + await Promise.all([first, second]); + + assert.ok(queuedIdentity, 'the queued mutation should have dispatched to the worker'); + assert.equal( + queuedIdentity.ino, + String(original.ino), + 'identity must be the inode captured at lock acquisition (before the replacement); a T1 capture would sample the replacement', + ); + }); +}); + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} diff --git a/packages/runtime/src/__tests__/filesystem-stable-write.test.ts b/packages/runtime/src/__tests__/filesystem-stable-write.test.ts new file mode 100644 index 0000000000..e6b93645dc --- /dev/null +++ b/packages/runtime/src/__tests__/filesystem-stable-write.test.ts @@ -0,0 +1,300 @@ +// Deterministic race tests for the fd-pinned mutation primitive (#2600). +// The pinning is the load-bearing defence: once the approved object is open +// and validated on the descriptor, a path swap cannot redirect the write — +// these tests prove it by swapping the path between validation and the write +// and asserting the bytes landed on the original inode, never the replacement. +import assert from 'node:assert/strict'; +import { mkdtemp, readFile, realpath, rename, rm, symlink, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, test } from 'node:test'; + +import { + compareAndDeleteEntry, + deleteCapturedTombstone, + hostVisibilityAfterWrite, + openStableTarget, + restoreTombstoneNoReplace, + writeThroughHandle, + type StableWriteFailure, +} from '../file-stable-write.js'; + +const cleanup: string[] = []; +afterEach(async () => { + await Promise.all(cleanup.splice(0).map((path) => rm(path, { recursive: true, force: true }))); +}); + +async function temporaryDirectory(prefix: string): Promise { + const path = await realpath(await mkdtemp(join(tmpdir(), prefix))); + cleanup.push(path); + return path; +} + +async function captureIdentity(path: string): Promise<{ dev: string; ino: string }> { + const { stat } = await import('node:fs/promises'); + const s = await stat(path, { bigint: true }); + return { dev: String(s.dev), ino: String(s.ino) }; +} + +describe('fd-pinned mutation primitive', () => { + test('a write survives a path swap between validation and the write', async () => { + const cwd = await temporaryDirectory('maka-pin-swap-'); + const target = join(cwd, 'file.txt'); + const replacement = join(cwd, 'replacement.txt'); + await writeFile(target, 'original', 'utf8'); + await writeFile(replacement, 'replacement-body', 'utf8'); + const identity = await captureIdentity(target); + + // The deterministic race: open + validate on the descriptor, THEN swap + // the path, THEN write. A path-based write would land on the replacement; + // the pinned write cannot. + const handle = await openStableTarget({ path: target, approvedIdentity: identity }); + try { + await rename(replacement, target); // the swap happens here + await writeThroughHandle(handle, 'pinned-content'); + + // The bytes went to the pinned inode: reading back through the same + // descriptor shows the new content. + assert.equal(await handle.readFile('utf8'), 'pinned-content'); + // The replacement file (now at the path) is untouched. + assert.equal(await readFile(target, 'utf8'), 'replacement-body'); + // Host visibility honestly reports the orphaned write. + const visibility = await hostVisibilityAfterWrite(target, handle); + assert.equal(visibility?.code, 'outcome_unknown'); + } finally { + await handle.close(); + } + }); + + test('a failed validation leaves the file byte-for-byte intact', async () => { + const cwd = await temporaryDirectory('maka-pin-reject-'); + const target = join(cwd, 'file.txt'); + const replacement = join(cwd, 'replacement.txt'); + await writeFile(target, 'original', 'utf8'); + await writeFile(replacement, 'replacement-body', 'utf8'); + const identity = await captureIdentity(target); + + // Swap before the open: the descriptor's inode will not match the approved + // identity, and — unlike a 'w' open — the rejected 'r+' never truncated it. + await rename(replacement, target); + await assert.rejects( + openStableTarget({ path: target, approvedIdentity: identity }), + (error: StableWriteFailure) => error.code === 'path_changed', + ); + assert.equal(await readFile(target, 'utf8'), 'replacement-body'); + }); + + test('an approved-missing target rejects a file that appeared in the gap', async () => { + const cwd = await temporaryDirectory('maka-pin-wx-'); + const target = join(cwd, 'file.txt'); + // The gap: the target was approved as missing, then something created it. + await writeFile(target, 'external-content', 'utf8'); + + await assert.rejects( + openStableTarget({ path: target, approvedIdentity: undefined }), + (error: StableWriteFailure) => error.code === 'path_changed', + ); + // The interloper's content was never truncated. + assert.equal(await readFile(target, 'utf8'), 'external-content'); + }); + + test('an approved-missing target is created exclusively', async () => { + const cwd = await temporaryDirectory('maka-pin-create-'); + const target = join(cwd, 'file.txt'); + + const handle = await openStableTarget({ path: target, approvedIdentity: undefined }); + try { + await writeThroughHandle(handle, 'created'); + } finally { + await handle.close(); + } + assert.equal(await readFile(target, 'utf8'), 'created'); + }); +}); + +describe('compare-and-delete (tombstone verification)', () => { + // POSIX has no atomic compare-and-unlink, so delete cannot PREVENT a + // same-directory swap — but the tombstone rename makes it atomic to CAPTURE + // whatever the path names, verify it, and RESTORE a replacement instead of + // silently deleting it (#2600 review: "delete can remove replacement"). + test('removes the approved entry', async () => { + const cwd = await temporaryDirectory('maka-delete-plain-'); + const target = join(cwd, 'file.txt'); + await writeFile(target, 'bye', 'utf8'); + const identity = await captureIdentity(target); + + await compareAndDeleteEntry({ path: target, approvedIdentity: identity }); + + const { readdir } = await import('node:fs/promises'); + assert.deepEqual(await readdir(cwd), []); // gone, and no tombstone leaked + }); + + test('restores a replacement installed after the check instead of deleting it', async () => { + const cwd = await temporaryDirectory('maka-delete-swap-'); + const target = join(cwd, 'file.txt'); + const replacement = join(cwd, 'replacement.txt'); + await writeFile(target, 'approved', 'utf8'); + await writeFile(replacement, 'replacement-body', 'utf8'); + const identity = await captureIdentity(target); + + // The race: after the check captured the approved identity, an external + // process renames a replacement over the path. + await rename(replacement, target); + + await assert.rejects( + compareAndDeleteEntry({ path: target, approvedIdentity: identity }), + (error: StableWriteFailure) => error.code === 'path_changed', + ); + // The replacement was restored, not deleted — content intact at the path. + assert.equal(await readFile(target, 'utf8'), 'replacement-body'); + const { readdir } = await import('node:fs/promises'); + assert.deepEqual(await readdir(cwd), ['file.txt']); // no tombstone leaked + }); + + test('deletes a symlink entry by its own identity without following it', async () => { + const cwd = await temporaryDirectory('maka-delete-symlink-'); + const pointed = join(cwd, 'pointed.txt'); + const link = join(cwd, 'link.txt'); + await writeFile(pointed, 'keep', 'utf8'); + await symlink(pointed, link); + const { lstat } = await import('node:fs/promises'); + const meta = await lstat(link, { bigint: true }); + const identity = { dev: String(meta.dev), ino: String(meta.ino) }; + + await compareAndDeleteEntry({ path: link, approvedIdentity: identity }); + + await assert.rejects(readFile(link, 'utf8'), { code: 'ENOENT' }); // link gone + assert.equal(await readFile(pointed, 'utf8'), 'keep'); // target untouched + }); + + // #2600 review P1: after the tombstone captured replacement C, another + // process created B at the original path; a rename-based restore would + // atomically overwrite and DELETE B — the exact loss class this module + // prevents. The no-replace restore must preserve both. + test('a no-replace restore preserves a path reoccupied during the capture', async () => { + const cwd = await temporaryDirectory('maka-delete-reoccupied-'); + const target = join(cwd, 'file.txt'); + const { writeFile: wf } = await import('node:fs/promises'); + // C sits on the tombstone; B has reoccupied the original path. + const tombstone = join(cwd, 'tombstone'); + await wf(tombstone, 'captured-C', 'utf8'); + await wf(target, 'newcomer-B', 'utf8'); + + await assert.rejects( + restoreTombstoneNoReplace(tombstone, target), + (error: StableWriteFailure) => error.code === 'outcome_unknown', + ); + // Both survive, byte-for-byte, at their own names. + assert.equal(await readFile(target, 'utf8'), 'newcomer-B'); + assert.equal(await readFile(tombstone, 'utf8'), 'captured-C'); + }); + + test('a no-replace restore moves the captured entry back when the path is free', async () => { + const cwd = await temporaryDirectory('maka-delete-restore-'); + const target = join(cwd, 'file.txt'); + const { writeFile: wf, readdir } = await import('node:fs/promises'); + const tombstone = join(cwd, 'tombstone'); + await wf(tombstone, 'captured', 'utf8'); + + await restoreTombstoneNoReplace(tombstone, target); + + assert.equal(await readFile(target, 'utf8'), 'captured'); + assert.deepEqual(await readdir(cwd), ['file.txt']); // no tombstone left + }); + + // #2600 review P2: renaming a directory into the tombstone succeeds while + // the tombstone unlink cannot (EISDIR/EPERM) — the directory would vanish + // from its path and hide under the tombstone name. Reject up front instead. + test('rejects a directory before touching it', async () => { + const cwd = await temporaryDirectory('maka-delete-directory-'); + const dir = join(cwd, 'subdir'); + const { mkdir, readdir } = await import('node:fs/promises'); + await mkdir(dir); + const { stat: st } = await import('node:fs/promises'); + const meta = await st(dir, { bigint: true }); + + await assert.rejects( + compareAndDeleteEntry({ + path: dir, + approvedIdentity: { dev: String(meta.dev), ino: String(meta.ino) }, + }), + (error: StableWriteFailure) => error.code === 'is_directory', + ); + // The directory is untouched at its original path — not hidden anywhere. + assert.deepEqual(await readdir(cwd), ['subdir']); + }); + + // #2600 review: the pre-check is not an enforcement point — a directory can + // race into the window between it and the capture. The enforcement lives + // after the capture: the tombstone's type is checked and a directory is + // renamed straight back BEFORE any identity comparison (link() cannot + // restore a directory, so the mismatch path would strand it on the tombstone + // with the path left empty). Force the directory into the tombstone directly. + test('a directory that raced into the capture is renamed back, not stranded', async () => { + const cwd = await temporaryDirectory('maka-delete-dirrace-'); + const dir = join(cwd, 'raced-in'); + const { + mkdir, + readdir, + rename: mv, + rm, + stat: st, + writeFile: wf, + } = await import('node:fs/promises'); + // The delete was approved against a regular file at this path. + await wf(dir, 'x', 'utf8'); + const meta = await st(dir, { bigint: true }); + const approvedIdentity = { dev: String(meta.dev), ino: String(meta.ino) }; + // The race: an external process removes the file and puts a directory at + // the path — after the caller's pre-check, before the capture. + await rm(dir); + await mkdir(dir); + // The production caller's capture rename grabs whatever is at the path, + // which is now the directory, onto a fresh tombstone name. + const tombstone = join(cwd, 'captured'); + await mv(dir, tombstone); + + await assert.rejects( + deleteCapturedTombstone(tombstone, dir, approvedIdentity), + (error: StableWriteFailure) => error.code === 'is_directory', + ); + // The directory is back at its original path; nothing is stranded. + assert.deepEqual(await readdir(cwd), ['raced-in']); + }); + + // #2600 review: on darwin, link() dereferences a symlink source, so a + // link-based restore of a captured symlink would plant a regular-file alias + // of the TARGET at the path — a foreign entry later reads/writes silently + // edit. The restore must recreate the symlink itself; the path may never be + // left holding a foreign regular-file entry. + test('restores a swapped-in symlink as a symlink, never a foreign regular file', async () => { + const cwd = await temporaryDirectory('maka-delete-linkswap-'); + const target = join(cwd, 'link.txt'); + const pointedA = join(cwd, 'pointed-a.txt'); + const pointedC = join(cwd, 'pointed-c.txt'); + const { lstat: lst, readdir, symlink: lnsym, writeFile: wf } = await import('node:fs/promises'); + await wf(pointedA, 'approved-target', 'utf8'); + await wf(pointedC, 'replacement-target', 'utf8'); + await lnsym(pointedA, target); // the approved entry A + const meta = await lst(target, { bigint: true }); + const approvedIdentity = { dev: String(meta.dev), ino: String(meta.ino) }; + // The race: a different symlink C is swapped over the path. + const replacement = join(cwd, 'replacement-link'); + await lnsym(pointedC, replacement); + await rename(replacement, target); + + await assert.rejects( + compareAndDeleteEntry({ path: target, approvedIdentity }), + (error: StableWriteFailure) => error.code === 'path_changed', + ); + // The path holds a SYMLINK pointing where C pointed — not a regular-file + // alias of C's target, and not the approved A either. + const restored = await lst(target); + assert.equal(restored.isSymbolicLink(), true, 'path must hold a symlink, not a regular file'); + const { readlink } = await import('node:fs/promises'); + assert.equal(await readlink(target), pointedC); + // No tombstone leaked; the approved target's content is untouched. + assert.deepEqual((await readdir(cwd)).sort(), ['link.txt', 'pointed-a.txt', 'pointed-c.txt']); + assert.equal(await readFile(pointedA, 'utf8'), 'approved-target'); + }); +}); diff --git a/packages/runtime/src/__tests__/filesystem-target-identity.test.ts b/packages/runtime/src/__tests__/filesystem-target-identity.test.ts new file mode 100644 index 0000000000..48d59c4c0f --- /dev/null +++ b/packages/runtime/src/__tests__/filesystem-target-identity.test.ts @@ -0,0 +1,255 @@ +// Red-line tests for the T0 target identity CAS (issue #2600 concern #1). +// A mutation whose target was replaced while the call waited for the write +// lock must be detected and rejected, not silently written to the replacement. +// These tests exercise the worker's assertTargetUnchanged identity check +// directly against a real filesystem. +import assert from 'node:assert/strict'; +import { mkdtemp, realpath, rename, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, test } from 'node:test'; + +import { executeFilesystemWorkerRequest } from '../filesystem-worker/operations.js'; +import { + FILESYSTEM_WORKER_PROTOCOL_VERSION, + type FilesystemWorkerRequest, + type FilesystemWorkerTarget, +} from '../filesystem-worker/protocol.js'; + +const cleanup: string[] = []; +afterEach(async () => { + await Promise.all(cleanup.splice(0).map((path) => rm(path, { recursive: true, force: true }))); +}); + +async function temporaryDirectory(prefix: string): Promise { + const path = await realpath(await mkdtemp(join(tmpdir(), prefix))); + cleanup.push(path); + return path; +} + +function requestFor( + operation: FilesystemWorkerRequest['operation'], + expectedTarget: FilesystemWorkerTarget, +): FilesystemWorkerRequest { + return { + version: FILESYSTEM_WORKER_PROTOCOL_VERSION, + requestId: 'test', + operation, + operationBoundary: { + filesystem: { + entries: [{ path: expectedTarget.enforcementPath, access: 'write', scope: 'exact' }], + }, + }, + expectedTarget, + }; +} + +async function captureIdentity(path: string): Promise<{ dev: string; ino: string }> { + const { stat } = await import('node:fs/promises'); + const s = await stat(path, { bigint: true }); + return { dev: String(s.dev), ino: String(s.ino) }; +} + +describe('filesystem worker target identity CAS', () => { + test('rejects a write when the target inode changed after authorisation', async () => { + const cwd = await temporaryDirectory('maka-identity-replace-'); + const target = join(cwd, 'file.txt'); + const replacement = join(cwd, 'replacement.txt'); + await writeFile(target, 'original', 'utf8'); + await writeFile(replacement, 'replacement', 'utf8'); + + // Capture the identity of the original target (T0). + const identity = await captureIdentity(target); + + // Swap the path to a different inode while "queued" (before the worker runs). + await rename(replacement, target); + + const response = await executeFilesystemWorkerRequest( + requestFor( + { kind: 'write', cwd, path: target, content: 'new' }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file', identity }, + ), + ); + + assert.equal(response.ok, false); + assert.equal(response.error?.code, 'path_changed'); + // The write must NOT have landed on the replacement file. + assert.equal( + await import('node:fs/promises').then((fs) => fs.readFile(target, 'utf8')), + 'replacement', + ); + }); + + test('allows a write when the target inode is unchanged', async () => { + const cwd = await temporaryDirectory('maka-identity-same-'); + const target = join(cwd, 'file.txt'); + await writeFile(target, 'original', 'utf8'); + + const identity = await captureIdentity(target); + + const response = await executeFilesystemWorkerRequest( + requestFor( + { kind: 'write', cwd, path: target, content: 'updated' }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file', identity }, + ), + ); + + assert.equal(response.ok, true); + assert.equal( + await import('node:fs/promises').then((fs) => fs.readFile(target, 'utf8')), + 'updated', + ); + }); + + test('rejects a delete when the target inode changed after authorisation', async () => { + const cwd = await temporaryDirectory('maka-identity-delete-'); + const target = join(cwd, 'delete-me.txt'); + const replacement = join(cwd, 'replacement.txt'); + await writeFile(target, 'original', 'utf8'); + await writeFile(replacement, 'replacement', 'utf8'); + + // Capture identity of the original, then swap to a different inode. + const identity = await captureIdentity(target); + await rename(replacement, target); + + const response = await executeFilesystemWorkerRequest( + requestFor( + { kind: 'apply_patch', cwd, path: target, action: 'delete' }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file', identity }, + ), + ); + + assert.equal(response.ok, false); + assert.equal(response.error?.code, 'path_changed'); + // The replacement must NOT have been deleted. + const { readFile } = await import('node:fs/promises'); + assert.equal(await readFile(target, 'utf8'), 'replacement'); + }); + + test('rejects an edit when the target inode changed after authorisation', async () => { + const cwd = await temporaryDirectory('maka-identity-edit-'); + const target = join(cwd, 'edit.txt'); + const replacement = join(cwd, 'replacement.txt'); + await writeFile(target, 'line\nold\n', 'utf8'); + await writeFile(replacement, 'replacement\nold\n', 'utf8'); + + const identity = await captureIdentity(target); + await rename(replacement, target); + + const response = await executeFilesystemWorkerRequest( + requestFor( + { kind: 'edit', cwd, path: target, oldString: 'old', newString: 'new' }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file', identity }, + ), + ); + + assert.equal(response.ok, false); + assert.equal(response.error?.code, 'path_changed'); + // The replacement content must be untouched (no edit applied). + const { readFile } = await import('node:fs/promises'); + assert.equal(await readFile(target, 'utf8'), 'replacement\nold\n'); + }); + + test('creates a missing target without requiring an identity', async () => { + const cwd = await temporaryDirectory('maka-identity-missing-'); + const target = join(cwd, 'brand-new.txt'); + + // A missing target carries no identity (there is no inode to pin); the + // create must succeed on it. Assert success and the created content so a + // regression that wrongly demands an identity for missing write targets + // (e.g. an over-broad mandatory-identity check) fails loudly here. + const response = await executeFilesystemWorkerRequest( + requestFor( + { kind: 'apply_patch', cwd, path: target, action: 'create', diff: '+created\n' }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'missing' }, + ), + ); + + assert.equal(response.ok, true); + const { readFile } = await import('node:fs/promises'); + assert.equal(await readFile(target, 'utf8'), 'created'); + }); + + test('reports path_changed when the target is replaced before the write (pre-write CAS)', async () => { + const cwd = await temporaryDirectory('maka-identity-prewrite-'); + const target = join(cwd, 'file.txt'); + const replacement = join(cwd, 'replacement.txt'); + await writeFile(target, 'original', 'utf8'); + await writeFile(replacement, 'replacement-body', 'utf8'); + + const identity = await captureIdentity(target); + + // Swap the path to a different inode BEFORE the request (simulates a + // replacement during the lock-wait window). The pre-write CAS catches it. + await rename(replacement, target); + + const response = await executeFilesystemWorkerRequest( + requestFor( + { kind: 'write', cwd, path: target, content: 'new' }, + { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file', identity }, + ), + ); + + assert.equal(response.ok, false); + assert.equal(response.error?.code, 'path_changed'); + }); +}); + +describe('filesystem worker post-write host-visibility check', () => { + // Direct unit tests for hostVisibilityAfterWrite — the check that runs AFTER + // writing through a pinned descriptor, describing whether the path still + // resolves to the inode we wrote. The fd-pinned write itself (tested in + // filesystem-stable-write.test.ts) is the primary defence; this describes + // host visibility when the path was swapped despite the pinning. + test('passes when the path still matches the written inode', async () => { + const { openStableTarget, hostVisibilityAfterWrite } = await import('../file-stable-write.js'); + const cwd = await temporaryDirectory('maka-orphan-same-'); + const target = join(cwd, 'file.txt'); + await writeFile(target, 'content', 'utf8'); + const identity = await captureIdentity(target); + const handle = await openStableTarget({ path: target, approvedIdentity: identity }); + try { + assert.equal(await hostVisibilityAfterWrite(target, handle), undefined); + } finally { + await handle.close(); + } + }); + + test('reports outcome_unknown when the path was replaced after the write', async () => { + const { openStableTarget, hostVisibilityAfterWrite } = await import('../file-stable-write.js'); + const cwd = await temporaryDirectory('maka-orphan-swapped-'); + const target = join(cwd, 'file.txt'); + const replacement = join(cwd, 'replacement.txt'); + await writeFile(target, 'original', 'utf8'); + await writeFile(replacement, 'replacement-body', 'utf8'); + const identity = await captureIdentity(target); + + const handle = await openStableTarget({ path: target, approvedIdentity: identity }); + try { + // Swap the path to a different inode after the write went to the pin. + await rename(replacement, target); + const visibility = await hostVisibilityAfterWrite(target, handle); + assert.equal(visibility?.code, 'outcome_unknown'); + } finally { + await handle.close(); + } + }); + + test('reports outcome_unknown when the path disappeared after the write', async () => { + const { openStableTarget, hostVisibilityAfterWrite } = await import('../file-stable-write.js'); + const cwd = await temporaryDirectory('maka-orphan-gone-'); + const target = join(cwd, 'file.txt'); + await writeFile(target, 'content', 'utf8'); + const identity = await captureIdentity(target); + + const handle = await openStableTarget({ path: target, approvedIdentity: identity }); + try { + // Remove the path entirely (the pinned inode is orphaned). + await rm(target); + const visibility = await hostVisibilityAfterWrite(target, handle); + assert.equal(visibility?.code, 'outcome_unknown'); + } finally { + await handle.close(); + } + }); +}); diff --git a/packages/runtime/src/__tests__/filesystem-worker-client.test.ts b/packages/runtime/src/__tests__/filesystem-worker-client.test.ts index ff12ce1c49..8ea0ed0296 100644 --- a/packages/runtime/src/__tests__/filesystem-worker-client.test.ts +++ b/packages/runtime/src/__tests__/filesystem-worker-client.test.ts @@ -1,6 +1,15 @@ import assert from 'node:assert/strict'; import { rmSync } from 'node:fs'; -import { mkdtemp, mkdir, realpath, rm, symlink, writeFile } from 'node:fs/promises'; +import { + lstat, + mkdtemp, + mkdir, + readFile, + realpath, + rm, + symlink, + writeFile, +} from 'node:fs/promises'; import { homedir, tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, describe, test } from 'node:test'; @@ -56,21 +65,29 @@ describe('filesystem worker client permission snapshots', () => { const link = join(workspace, 'link.txt'); await writeFile(target, 'keep', 'utf8'); await symlink(target, link); + // Capture the symlink entry's own identity (lstat, no follow) as the + // boundary executor does at T0; a write mutation on an existing target + // without it is rejected as path_changed. + const linkMeta = await lstat(link, { bigint: true }); const { client, requests } = fakeClient(); await client.execute({ operation: { kind: 'apply_patch', path: link, action: 'delete' }, cwd: workspace, mode: 'ask', + expectedIdentity: { dev: String(linkMeta.dev), ino: String(linkMeta.ino) }, }); assert.equal(requests[0]?.operation.path, link); - assert.deepEqual(requests[0]?.expectedTarget, { - enforcementPath: link, - access: 'write', - scope: 'exact', - targetType: 'symlink', - }); + const expectedTarget = requests[0]?.expectedTarget; + assert.equal(expectedTarget?.enforcementPath, link); + assert.equal(expectedTarget?.access, 'write'); + assert.equal(expectedTarget?.scope, 'exact'); + assert.equal(expectedTarget?.targetType, 'symlink'); + // The symlink entry's own identity (lstat, no follow) is captured at T0 and + // forwarded; only its shape is stable, not its value. + assert.equal(typeof expectedTarget?.identity?.dev, 'string'); + assert.equal(typeof expectedTarget?.identity?.ino, 'string'); }); for (const kind of ['bypass', 'external'] as const) { @@ -508,6 +525,7 @@ function fakeClient( timedOut: false, aborted: false, responseOverflow: false, + dispatched: true, }; }, }); @@ -560,6 +578,197 @@ function hasArgTriple( ); } +describe('filesystem worker client dispatch classification', () => { + // The process-runner attaches a `dispatched` flag to its rejection so the + // client can tell a never-started spawn from a ran-but-result-lost failure. + // Only the latter can have written anything, so it gets a distinct reason. + function clientWithRejectingRunProcess( + reject: (input: FilesystemWorkerProcessRunInput) => Error, + ): FilesystemWorkerClient { + const sandboxManager = new SandboxManager([new MacosSeatbeltBackend()]); + return new FilesystemWorkerClient({ + sandboxManager, + platform: 'darwin', + newId: () => 'request-1', + getLaunchSpec: async () => ({ + ok: true, + spec: { + program: '/usr/bin/node', + args: ['/runtime/filesystem-worker.js', '--grep-executable', '/usr/bin/rg'], + env: {}, + runtimeReadableRoots: ['/runtime/filesystem-worker.js'], + executableRoots: ['/usr/bin/node', '/usr/bin/rg'], + }, + }), + runProcess: async (input) => { + throw reject(input); + }, + }); + } + + function dispatchedError(message: string, dispatched: boolean): Error { + const error = new Error(message); + Object.defineProperty(error, 'dispatched', { value: dispatched, enumerable: true }); + return error; + } + + test('classifies a post-dispatch runProcess rejection as worker_io_incomplete', async () => { + const client = clientWithRejectingRunProcess(() => + dispatchedError('Filesystem worker output did not drain before lifecycle deadline', true), + ); + await assert.rejects( + client.execute({ + operation: { kind: 'write', path: '/tmp/maka-dispatch-incomplete.txt', content: 'x' }, + cwd: '/tmp', + mode: 'ask', + }), + (error: unknown) => { + assert.ok(error instanceof FilesystemWorkerClientError); + assert.equal(error.reason, 'worker_io_incomplete'); + assert.equal(error.dispatched, true); + return true; + }, + ); + }); + + test('classifies a never-dispatched runProcess rejection as spawn_failed', async () => { + const client = clientWithRejectingRunProcess(() => dispatchedError('spawn ENOENT', false)); + await assert.rejects( + client.execute({ + operation: { kind: 'write', path: '/tmp/maka-dispatch-spawn.txt', content: 'x' }, + cwd: '/tmp', + mode: 'ask', + }), + (error: unknown) => { + assert.ok(error instanceof FilesystemWorkerClientError); + assert.equal(error.reason, 'spawn_failed'); + assert.equal(error.dispatched, false); + return true; + }, + ); + }); + + test('a rejection without a dispatched flag is treated as never-dispatched', async () => { + // spawn() itself throwing (before any 'spawn' event) carries no flag. + const client = clientWithRejectingRunProcess(() => new Error('spawn EACCES')); + await assert.rejects( + client.execute({ + operation: { kind: 'write', path: '/tmp/maka-dispatch-noflag.txt', content: 'x' }, + cwd: '/tmp', + mode: 'ask', + }), + (error: unknown) => { + assert.ok(error instanceof FilesystemWorkerClientError); + assert.equal(error.reason, 'spawn_failed'); + return true; + }, + ); + }); + + // The missing↔existing transitions while queued (#2600 P2-1): the client + // reconciles the T0 identity against the T1 reality the normaliser derived, + // so cooperative lock-ordered changes never surface as invalid_request. + test('drops a stale identity when the target vanished while queued (delete-then-rewrite)', async () => { + const workspace = await temporaryDirectory('maka-client-stale-identity-'); + const target = join(workspace, 'file.txt'); + await writeFile(target, 'original', 'utf8'); + const stale = await lstat(target, { bigint: true }); + // The cooperative delete already ran: the target is gone by T1. + await rm(target); + + const requests: FilesystemWorkerRequest[] = []; + const sandboxManager = new SandboxManager([new MacosSeatbeltBackend()]); + const client = new FilesystemWorkerClient({ + sandboxManager, + platform: 'darwin', + newId: () => 'request-1', + getLaunchSpec: async () => ({ + ok: true, + spec: { + program: '/usr/bin/node', + args: ['/runtime/filesystem-worker.js', '--grep-executable', '/usr/bin/rg'], + env: {}, + runtimeReadableRoots: ['/runtime/filesystem-worker.js'], + executableRoots: ['/usr/bin/node', '/usr/bin/rg'], + }, + }), + runProcess: async (input) => { + const request = FilesystemWorkerRequestSchema.parse(JSON.parse(input.stdin)); + requests.push(request); + return { + exitCode: 0, + stdout: JSON.stringify({ + version: FILESYSTEM_WORKER_PROTOCOL_VERSION, + requestId: request.requestId, + ok: true, + result: { kind: 'write', ok: true, path: request.operation.path, bytes: 3 }, + }), + stderrTail: '', + timedOut: false, + aborted: false, + responseOverflow: false, + dispatched: true, + }; + }, + }); + + // T0 captured an identity; by T1 the target is missing. The stale identity + // must be dropped — never sent on a missing target — so the write proceeds + // as a fresh exclusive create instead of failing invalid_request. + await client.execute({ + operation: { kind: 'write', path: target, content: 'new' }, + cwd: workspace, + mode: 'ask', + expectedIdentity: { dev: String(stale.dev), ino: String(stale.ino) }, + }); + + assert.equal(requests[0]?.expectedTarget.targetType, 'missing'); + assert.equal(requests[0]?.expectedTarget.identity, undefined); + }); + + test('rejects a write whose target was created while queued (never invalid_request)', async () => { + const workspace = await temporaryDirectory('maka-client-created-'); + const target = join(workspace, 'file.txt'); + // The target was approved as missing (no identity) and appeared by T1. + await writeFile(target, 'external-content', 'utf8'); + + const sandboxManager = new SandboxManager([new MacosSeatbeltBackend()]); + const client = new FilesystemWorkerClient({ + sandboxManager, + platform: 'darwin', + newId: () => 'request-1', + getLaunchSpec: async () => ({ + ok: true, + spec: { + program: '/usr/bin/node', + args: ['/runtime/filesystem-worker.js', '--grep-executable', '/usr/bin/rg'], + env: {}, + runtimeReadableRoots: ['/runtime/filesystem-worker.js'], + executableRoots: ['/usr/bin/node', '/usr/bin/rg'], + }, + }), + runProcess: async () => { + throw new Error('must not dispatch'); + }, + }); + + await assert.rejects( + client.execute({ + operation: { kind: 'write', path: target, content: 'new' }, + cwd: workspace, + mode: 'ask', + }), + (error: unknown) => { + assert.ok(error instanceof FilesystemWorkerClientError); + assert.equal(error.reason, 'path_changed'); + return true; + }, + ); + // The interloper's content was never touched. + assert.equal(await readFile(target, 'utf8'), 'external-content'); + }); +}); + function isPathDenied(error: unknown): boolean { return error instanceof FilesystemWorkerClientError && error.reason === 'path_denied'; } diff --git a/packages/runtime/src/__tests__/filesystem-worker-linux-smoke.test.ts b/packages/runtime/src/__tests__/filesystem-worker-linux-smoke.test.ts index 21696be080..f5dfbf8bb3 100644 --- a/packages/runtime/src/__tests__/filesystem-worker-linux-smoke.test.ts +++ b/packages/runtime/src/__tests__/filesystem-worker-linux-smoke.test.ts @@ -1,5 +1,5 @@ import assert from 'node:assert/strict'; -import { mkdtemp, mkdir, readFile, realpath, rm } from 'node:fs/promises'; +import { mkdtemp, mkdir, readFile, realpath, rm, stat } from 'node:fs/promises'; import { homedir, tmpdir } from 'node:os'; import { join } from 'node:path'; import { after, before, describe, test } from 'node:test'; @@ -74,6 +74,10 @@ describe('Linux filesystem worker smoke', { skip }, () => { content: 'export const healthSignal = true;\n', }); + // Capture the identity at T0 (the boundary executor does this in + // production): a write mutation on an existing target must carry the + // inode, or the worker refuses to skip CAS. + const editMeta = await stat(sourceFile, { bigint: true }); const edit = await client.execute({ operation: { kind: 'edit', @@ -83,6 +87,7 @@ describe('Linux filesystem worker smoke', { skip }, () => { }, cwd: workspace, mode: 'ask', + expectedIdentity: { dev: String(editMeta.dev), ino: String(editMeta.ino) }, }); assert.equal(edit.kind, 'edit'); assert.equal(await readFile(sourceFile, 'utf8'), 'export const healthSignal = "healthy";\n'); diff --git a/packages/runtime/src/__tests__/filesystem-worker-smoke.test.ts b/packages/runtime/src/__tests__/filesystem-worker-smoke.test.ts index 6295b2a042..1e5d5fd6d0 100644 --- a/packages/runtime/src/__tests__/filesystem-worker-smoke.test.ts +++ b/packages/runtime/src/__tests__/filesystem-worker-smoke.test.ts @@ -82,11 +82,16 @@ describe('macOS filesystem worker smoke', { skip: process.platform !== 'darwin' const aliasedTarget = target.replace(/^\/private(?=\/)/, ''); assert.notEqual(aliasedTarget, target); await writeFile(target, 'delete me', 'utf8'); + // Capture the identity at T0 (the boundary executor does this in production). + const { lstat } = await import('node:fs/promises'); + const meta = await lstat(target, { bigint: true }); + const expectedIdentity = { dev: String(meta.dev), ino: String(meta.ino) }; await client.execute({ operation: { kind: 'apply_patch', path: aliasedTarget, action: 'delete' }, cwd: workspace, mode: 'ask', + expectedIdentity, }); await assert.rejects(readFile(target, 'utf8'), { code: 'ENOENT' }); diff --git a/packages/runtime/src/__tests__/filesystem-worker-windows-smoke.test.ts b/packages/runtime/src/__tests__/filesystem-worker-windows-smoke.test.ts index 5852d87c14..967779d4fa 100644 --- a/packages/runtime/src/__tests__/filesystem-worker-windows-smoke.test.ts +++ b/packages/runtime/src/__tests__/filesystem-worker-windows-smoke.test.ts @@ -86,10 +86,16 @@ describe('Windows filesystem worker smoke', { skip: !enabled }, () => { // grant covers only this file object and never its parent directory. const target = join(workspace, 'inside.txt'); await writeFile(target, 'seeded'); + // A write mutation on an existing target must carry the identity captured + // at lock acquisition (#2600): lstat the file and supply { dev, ino }, + // exactly as the boundary executor does in production. + const { stat } = await import('node:fs/promises'); + const meta = await stat(target, { bigint: true }); await client.execute({ operation: { kind: 'write', path: target, content: 'windows-relay-ok' }, cwd: workspace, mode: 'ask', + expectedIdentity: { dev: String(meta.dev), ino: String(meta.ino) }, }); assert.equal(await readFile(target, 'utf8'), 'windows-relay-ok'); diff --git a/packages/runtime/src/__tests__/filesystem-worker.test.ts b/packages/runtime/src/__tests__/filesystem-worker.test.ts index a7e077e307..f4ebb2a44d 100644 --- a/packages/runtime/src/__tests__/filesystem-worker.test.ts +++ b/packages/runtime/src/__tests__/filesystem-worker.test.ts @@ -1,5 +1,15 @@ import { strict as assert } from 'node:assert'; -import { mkdtemp, mkdir, readFile, realpath, rm, symlink, writeFile } from 'node:fs/promises'; +import { + lstat, + mkdtemp, + mkdir, + readFile, + realpath, + rm, + stat, + symlink, + writeFile, +} from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join, parse } from 'node:path'; import { afterEach, describe, test } from 'node:test'; @@ -35,7 +45,7 @@ describe('filesystem worker operations', () => { }; const created = await executeFilesystemWorkerRequest( - requestFor(operation, { + await requestFor(operation, { enforcementPath: target, access: 'write', scope: 'exact', @@ -46,7 +56,7 @@ describe('filesystem worker operations', () => { assert.equal(await readFile(target, 'utf8'), 'created'); const conflict = await executeFilesystemWorkerRequest( - requestFor(operation, { + await requestFor(operation, { enforcementPath: target, access: 'write', scope: 'exact', @@ -63,7 +73,7 @@ describe('filesystem worker operations', () => { await writeFile(target, 'before\n', 'utf8'); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'apply_patch', cwd: root, @@ -96,7 +106,7 @@ describe('filesystem worker operations', () => { await symlink(target, link); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'apply_patch', cwd: root, path: link, action: 'delete' }, { enforcementPath: link, @@ -112,13 +122,42 @@ describe('filesystem worker operations', () => { await assert.rejects(readFile(link, 'utf8'), { code: 'ENOENT' }); }); + test('refuses a directory delete with the structured is_directory code', async () => { + const root = await temporaryDirectory('maka-worker-delete-dir-'); + const dir = join(root, 'subdir'); + await mkdir(dir); + + const response = await executeFilesystemWorkerRequest( + await requestFor( + { kind: 'apply_patch', cwd: root, path: dir, action: 'delete' }, + { + enforcementPath: dir, + access: 'write', + scope: 'exact', + targetType: 'directory', + }, + ), + ); + + // The structured code survives classification — the model learns a + // directory was refused, not a generic filesystem failure (#2600). + assert.equal(response.ok, false); + if (!response.ok) { + assert.equal(response.error.code, 'is_directory'); + assert.match(response.error.message, /directory/i); + } + // The directory is untouched at its original path. + const entries = await (await import('node:fs/promises')).readdir(root); + assert.deepEqual(entries, ['subdir']); + }); + test('fails Grep closed inside the Windows sandbox instead of approximating its contract', async () => { const root = await temporaryDirectory('maka-worker-grep-sandboxed-'); const target = join(root, 'file.ts'); await writeFile(target, 'const healthSignal = true;', 'utf8'); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'grep', cwd: root, @@ -155,7 +194,7 @@ describe('filesystem worker operations', () => { let grepCwd: string | undefined; const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'grep', cwd: root, @@ -198,7 +237,7 @@ describe('filesystem worker operations', () => { limit: 200, timeoutMs: 1_000, }; - const request = requestFor(operation, { + const request = await requestFor(operation, { enforcementPath: target, access: 'read', scope: 'exact', @@ -245,7 +284,7 @@ describe('filesystem worker operations', () => { await writeFile(target, ONE_PIXEL_PNG); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'read', cwd: root, path: target, offset: 1, limit: 1 }, { enforcementPath: target, access: 'read', scope: 'exact', targetType: 'file' }, ), @@ -272,7 +311,7 @@ describe('filesystem worker operations', () => { await symlink(text, textLink); const imageResponse = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'read', cwd: root, path: imageLink }, { enforcementPath: image, access: 'read', scope: 'exact', targetType: 'file' }, image, @@ -282,7 +321,7 @@ describe('filesystem worker operations', () => { if (imageResponse.ok) assert.equal(imageResponse.result.kind, 'read_image'); const textResponse = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'read', cwd: root, path: textLink }, { enforcementPath: text, access: 'read', scope: 'exact', targetType: 'file' }, text, @@ -300,7 +339,7 @@ describe('filesystem worker operations', () => { await writeFile(insidePath, 'inside', 'utf8'); const readResponse = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'read', cwd: root, path: insidePath }, { enforcementPath: insidePath, access: 'read', scope: 'exact', targetType: 'file' }, ), @@ -309,7 +348,7 @@ describe('filesystem worker operations', () => { if (readResponse.ok) assert.deepEqual(readResponse.result, { kind: 'read', content: 'inside' }); const denied = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'write', cwd: root, path: outsidePath, content: 'blocked' }, { enforcementPath: outsidePath, access: 'write', scope: 'exact', targetType: 'missing' }, insidePath, @@ -332,7 +371,7 @@ describe('filesystem worker operations', () => { await symlink(target, link); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'write', cwd: root, path: link, content: 'blocked' }, { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'missing' }, link, @@ -350,7 +389,7 @@ describe('filesystem worker operations', () => { await mkdir(target); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'read', cwd: root, path: target }, { enforcementPath: target, access: 'read', scope: 'exact', targetType: 'file' }, ), @@ -370,7 +409,7 @@ describe('filesystem worker operations', () => { await symlink(replacement, link); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'read', cwd: root, path: link }, { enforcementPath: approved, access: 'read', scope: 'exact', targetType: 'file' }, approved, @@ -387,7 +426,7 @@ describe('filesystem worker operations', () => { await writeFile(target, before, 'utf8'); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'edit', cwd: root, @@ -411,7 +450,7 @@ describe('filesystem worker operations', () => { await writeFile(target, '{\n "a": 1\n}', 'utf8'); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'format_json', cwd: root, path: target, sortKeys: false }, { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file' }, ), @@ -430,7 +469,7 @@ describe('filesystem worker operations', () => { await writeFile(target, 'secret\n', { mode: 0o222 }); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'write', cwd: root, path: target, content: 'replacement\n' }, { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file' }, ), @@ -450,7 +489,7 @@ describe('filesystem worker operations', () => { await writeFile(target, ONE_PIXEL_PNG); const response = await executeFilesystemWorkerRequest( - requestFor( + await requestFor( { kind: 'write', cwd: root, path: target, content: 'not an image anymore\n' }, { enforcementPath: target, access: 'write', scope: 'exact', targetType: 'file' }, ), @@ -463,11 +502,11 @@ describe('filesystem worker operations', () => { }); }); -function requestFor( +async function requestFor( operation: FilesystemWorkerOperation, expectedTarget: FilesystemWorkerTarget, permissionPath = operation.path, -): FilesystemWorkerRequest { +): Promise { const operationBoundary: FilesystemWorkerRequest['operationBoundary'] = { filesystem: { entries: [ @@ -479,12 +518,30 @@ function requestFor( ], }, }; + // The real caller captures the target identity at T0; mirror that here so + // the worker's mandatory-identity check is satisfied for non-missing targets. + let resolvedTarget = expectedTarget; + if (expectedTarget.targetType !== 'missing' && !expectedTarget.identity) { + const follow = expectedTarget.targetType !== 'symlink'; + try { + const metadata = follow + ? await stat(expectedTarget.enforcementPath, { bigint: true }) + : await lstat(expectedTarget.enforcementPath, { bigint: true }); + resolvedTarget = { + ...expectedTarget, + identity: { dev: String(metadata.dev), ino: String(metadata.ino) }, + }; + } catch { + // Target may not exist at request construction time (the test sets it up + // differently); leave identity absent and let the worker surface it. + } + } return { version: FILESYSTEM_WORKER_PROTOCOL_VERSION, requestId: 'request-1', operation, operationBoundary, - expectedTarget, + expectedTarget: resolvedTarget, }; } diff --git a/packages/runtime/src/apply-patch-file.ts b/packages/runtime/src/apply-patch-file.ts index 156a77ed71..29863886d6 100644 --- a/packages/runtime/src/apply-patch-file.ts +++ b/packages/runtime/src/apply-patch-file.ts @@ -28,6 +28,15 @@ export async function updatePatchedFile(path: string, diff: string): Promise { + const noFollow = process.platform === 'win32' ? 0 : constants.O_NOFOLLOW; + if (input.approvedIdentity) { + if (process.platform === 'win32') { + const entry = await lstat(input.path).catch(() => null); + if (entry?.isSymbolicLink()) { + throw pathChanged( + 'The approved filesystem target is a symbolic link; refusing to follow it.', + ); + } + } + let handle: FileHandle; + try { + handle = await open(input.path, constants.O_RDWR | noFollow); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ELOOP' || code === 'ENOTDIR' || code === 'ENOENT') { + throw pathChanged('The approved filesystem target changed before execution.'); + } + if (code === 'EACCES' || code === 'EPERM') { + // A write-only target (e.g. mode 0o222) refuses 'r+' but is still a + // legitimate mutation target — unlink semantics need no read + // permission. Retry write-only (still no truncate: identity is + // validated on the descriptor before writeThroughHandle truncates). + // The pinned read of the previous content will fail and the caller + // reports 'unknown' (no diff), which is the pre-existing behaviour. + try { + handle = await open(input.path, constants.O_WRONLY | noFollow); + } catch (retry) { + const retryCode = (retry as NodeJS.ErrnoException).code; + if (retryCode === 'ELOOP' || retryCode === 'ENOTDIR' || retryCode === 'ENOENT') { + throw pathChanged('The approved filesystem target changed before execution.'); + } + throw retry; + } + } else { + throw error; + } + } + // The compare in compare-and-update, performed on the descriptor itself. + const metadata = await handle.stat({ bigint: true }); + if ( + String(metadata.dev) !== input.approvedIdentity.dev || + String(metadata.ino) !== input.approvedIdentity.ino + ) { + await handle.close(); + throw pathChanged('The approved filesystem target changed before execution.'); + } + return handle; + } + try { + return await open(input.path, constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'EEXIST') { + throw pathChanged('A file appeared at the approved missing target; re-read before writing.'); + } + throw error; + } +} + +/** + * Write `content` through the pinned descriptor. The truncation happens here, + * only after the identity was validated on the fd. Write-step failures + * (ENOSPC/EIO/EDQUOT/EFBIG) can leave the file truncated or half-written, so + * they surface as `outcome_unknown` — the file's state is genuinely unknown. + */ +export async function writeThroughHandle(handle: FileHandle, content: string): Promise { + try { + await handle.truncate(0); + // Position 0 explicitly: a prior readFile leaves the fd position at EOF, + // and a positionless write would create a NUL-prefixed sparse file. + await handle.write(content, 0, 'utf8'); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ENOSPC' || code === 'EIO' || code === 'EDQUOT' || code === 'EFBIG') { + throw new StableWriteFailure( + 'outcome_unknown', + 'The write started but failed partway; the file may be truncated. ' + + 'Re-read the file before writing to it again.', + ); + } + throw error; + } +} + +/** + * Read-modify-write entirely through one descriptor. Transform errors happen + * before any truncation and propagate unchanged (nothing was written). An + * unchanged result skips the write entirely, preserving mtime for no-ops. + */ +export async function readModifyWriteThroughHandle( + handle: FileHandle, + transform: (existing: string) => string, +): Promise { + const existing = await handle.readFile('utf8'); + const replacement = transform(existing); + if (replacement !== existing) { + await writeThroughHandle(handle, replacement); + } + return replacement; +} + +/** + * After writing through the handle, describe the host-visible outcome: if the + * path no longer resolves to the descriptor's inode, the write went to an + * orphaned inode and the visible file is the replacement — an unknown outcome. + * `undefined` means the path still matches and the write is host-visible. + */ +export async function hostVisibilityAfterWrite( + path: string, + handle: FileHandle, +): Promise { + const written = await handle.stat({ bigint: true }); + let current: { dev: bigint; ino: bigint }; + try { + current = await stat(path, { bigint: true }); + } catch { + return new StableWriteFailure( + 'outcome_unknown', + 'The target disappeared after the write; the outcome on disk is unknown.', + ); + } + if (String(current.dev) !== String(written.dev) || String(current.ino) !== String(written.ino)) { + return new StableWriteFailure( + 'outcome_unknown', + 'The target was replaced during the write; the outcome on disk is unknown.', + ); + } + return undefined; +} + +/** + * Compare-and-delete for directory entries (#2600). POSIX has no atomic + * compare-and-unlink, so a bare `unlink(path)` can remove a replacement + * installed between the identity check and the unlink — and report success. + * Instead, atomically grab whatever the path currently names by renaming it to + * a private tombstone in the same directory, verify the tombstone carries the + * approved identity, and only then unlink the tombstone. A mismatch means a + * replacement was installed in the window: it is moved back to the path + * WITHOUT clobbering anything that reoccupied it (no-replace restore) and the + * operation reports `path_changed`. + * + * rename(2) moves the directory entry itself, so this works for regular files + * and symlinks alike, needs no permission on the file (only write+execute on + * the parent directory, exactly like unlink), and never follows a symlink. + * Directories are refused — up front when visible, and again after the + * capture when a directory races into the window — because a directory cannot + * be unlinked, only recursively removed: a different operation entirely. + */ +export async function compareAndDeleteEntry(input: { + path: string; + approvedIdentity: FilesystemTargetIdentity | undefined; +}): Promise { + // Fast refusal when the directory is visible up front (#2600 review): a + // rename into the tombstone succeeds for directories, but the tombstone + // unlink cannot (EISDIR/EPERM), which would hide the directory under a + // stray name instead of failing cleanly. The post-capture check below is + // the enforcement; this pre-check just avoids moving anything. + const entryBefore = await lstat(input.path); + if (entryBefore.isDirectory()) { + throw new StableWriteFailure( + 'is_directory', + 'Refusing to delete a directory through the entry-delete path.', + ); + } + const tombstone = join(dirname(input.path), `.maka-pending-delete-${randomUUID()}`); + // Atomically capture whatever entry the path names right now. + await rename(input.path, tombstone); + await deleteCapturedTombstone(tombstone, input.path, input.approvedIdentity); +} + +/** + * Verify and delete the entry held on the tombstone, restoring it on any + * mismatch. The capture rename in the caller is the atomic step, so this is + * where enforcement lives (#2600 review): the captured entry's TYPE is checked + * before anything else — a directory that raced in after the caller's pre-check + * is renamed straight back rather than identity-compared (link() cannot restore + * a directory, so the mismatch path would strand it on the tombstone). + * + * @internal Exported for the forced-directory-in-tombstone regression test. + */ +export async function deleteCapturedTombstone( + tombstone: string, + path: string, + approvedIdentity: FilesystemTargetIdentity | undefined, +): Promise { + const captured = await lstat(tombstone).catch(() => null); + if (captured?.isDirectory()) { + // A directory raced into the window between the caller's pre-check and + // the capture. Restore it by rename — the only mechanism that moves a + // directory — before any identity comparison. POSIX self-protects most + // reoccupations (dir → existing non-directory fails ENOTDIR); if even + // that fails, preserve the tombstone and report the location. + try { + await rename(tombstone, path); + } catch { + throw new StableWriteFailure( + 'outcome_unknown', + `A directory was captured and could not be restored; it is preserved at ${tombstone}.`, + ); + } + throw new StableWriteFailure( + 'is_directory', + 'Refusing to delete a directory through the entry-delete path; it was restored to its original location.', + ); + } + if (approvedIdentity) { + const entry = await lstat(tombstone, { bigint: true }).catch(() => null); + const matches = + entry !== null && + String(entry.dev) === approvedIdentity.dev && + String(entry.ino) === approvedIdentity.ino; + if (!matches) { + // A replacement was installed after the check. Restore it without + // clobbering: if the path was reoccupied in the window, the restore + // itself throws outcome_unknown and the tombstone is preserved. + await restoreTombstoneNoReplace(tombstone, path); + throw pathChanged( + 'The approved filesystem target changed before the delete; the replacement was restored. Re-check the directory before deleting again.', + ); + } + } + // The tombstone is a private unpredictable name: nothing else contends + // with this unlink. + try { + await unlink(tombstone); + } catch { + // The approved entry was moved but not removed: the delete's outcome on + // disk is genuinely unknown, and the tombstone preserves the content. + throw new StableWriteFailure( + 'outcome_unknown', + `The delete may not have completed; the entry is preserved at ${tombstone}.`, + ); + } +} + +/** + * Move the tombstone entry back to its original path WITHOUT overwriting + * whatever may have reoccupied the path while the entry was captured (#2600 + * review: a plain rename would atomically delete the new occupant — the exact + * class of replacement loss this module exists to prevent). Node exposes no + * RENAME_NOREPLACE, so the no-replace mechanism depends on the entry type: + * + * - Regular file: `link()` is natively no-replace (EEXIST when the destination + * exists) and links the very inode the tombstone holds. + * - Symlink: `link()` is unusable — POSIX leaves its treatment of a symlink + * source implementation-defined, and darwin dereferences it, which would + * plant a regular-file alias of the TARGET at the path (a foreign entry any + * later read/write silently edits). The link is instead recreated with + * `symlink(readlink(...))`, which is also natively no-replace and round-trips + * the target string exactly; the recreated link is a new inode, which is + * immaterial for a delete this call is refusing anyway. + * + * On EEXIST — or any creation failure — the tombstone is preserved and the + * failure reported as `outcome_unknown` with the location, so nothing is ever + * lost. + * + * @internal Exported for the no-replace restore regression tests. + */ +export async function restoreTombstoneNoReplace(tombstone: string, path: string): Promise { + const captured = await lstat(tombstone).catch(() => null); + if (captured === null) { + throw new StableWriteFailure( + 'outcome_unknown', + `The captured entry could not be read; it is preserved at ${tombstone}.`, + ); + } + if (captured.isSymbolicLink()) { + const target = await readlink(tombstone); + try { + await symlink(target, path); + } catch { + // EEXIST: the path was reoccupied after the capture (or symlink creation + // failed). Preserving the tombstone is the only non-destructive option. + throw new StableWriteFailure( + 'outcome_unknown', + `The original path was reoccupied; the captured symlink is preserved at ${tombstone}.`, + ); + } + // The link semantics are restored at the path; dropping the old link name + // is best-effort — a failure leaves a stray name, never data loss. + await unlink(tombstone).catch(() => {}); + return; + } + try { + await link(tombstone, path); + } catch { + // EEXIST: the path was reoccupied after the capture. Any other failure + // (entry type / filesystem without hardlink support): preserving the + // tombstone is the only non-destructive option. Either way nothing is + // lost — the captured entry survives at the tombstone. + throw new StableWriteFailure( + 'outcome_unknown', + `The original path was reoccupied; the captured entry is preserved at ${tombstone}.`, + ); + } + // The restored entry must be the very inode the tombstone holds. A mismatch + // means the path no longer carries what this call created — most plausibly a + // concurrent rename placed a foreign entry there — and unlinking it would + // destroy third-party data (the exact loss class this module prevents). + // Touch nothing and report both locations. + const restored = await lstat(path, { bigint: true }).catch(() => null); + const held = await lstat(tombstone, { bigint: true }).catch(() => null); + if ( + restored === null || + held === null || + restored.ino !== held.ino || + restored.dev !== held.dev + ) { + throw new StableWriteFailure( + 'outcome_unknown', + `The entry could not be restored reliably; it is preserved at ${tombstone} (path now holds ${path}).`, + ); + } + // The entry is verified at the path; dropping the tombstone name is + // best-effort — a failure here leaves a stray name, never data loss, and + // must not turn a successful restore into a reported failure. + await unlink(tombstone).catch(() => {}); +} diff --git a/packages/runtime/src/filesystem-authority.ts b/packages/runtime/src/filesystem-authority.ts new file mode 100644 index 0000000000..641fdf24da --- /dev/null +++ b/packages/runtime/src/filesystem-authority.ts @@ -0,0 +1,102 @@ +// packages/runtime/src/filesystem-authority.ts +// The shared contract for who a filesystem mutation may touch and how its +// outcome is reported. Issue #2600 asks for this to live in one place, kept +// separate from the individual editing tools: the executor, the worker, and +// the workspace executor all import from here rather than each restating what +// "the target I approved" and "the outcome on disk" mean. +// +// This module is types and pure classification only — no I/O. The fd-pinned +// read-modify-write that enforces these contracts lives in file-stable-write.ts +// and is consumed by both the worker and the local workspace executor; the +// contract itself is what every backend agrees on. + +import { FilesystemWorkerClientError } from './filesystem-worker/client.js'; + +/** + * A stable identity for a filesystem target, captured the moment a mutation is + * authorised. `dev` and `ino` are carried as opaque decimal strings rather + * than bigint: bigint cannot cross the worker's JSON protocol boundary + * (`JSON.stringify` throws on a BigInt), and identity is only ever compared + * for equality, never used to build a path. Stringifying `stats.dev` / + * `stats.ino` on the capturing side and comparing the strings on the checking + * side is the whole round-trip. + */ +export interface FilesystemTargetIdentity { + readonly dev: string; + readonly ino: string; +} + +/** + * The full target descriptor captured at lock acquisition (the earliest point a + * mutation commits to a path). Modelled as a discriminated union so that + * "the target had no identity to compare" is an explicit `missing` case, + * never an accidentally-absent optional field. A later "skip the identity + * check" change cannot compile without handling the `missing` arm, which is + * what closes the "no identity → CAS passes" regression class. + * + * - `missing`: the path did not exist at capture (a create, or a write to a + * brand-new file). There is no inode to pin; the missing→existing transition + * is detected by `O_EXCL` / `wx` instead. + * - the other variants carry the captured inode, which the worker compares + * against the on-disk inode immediately before mutating. + */ +export type FilesystemTargetDescriptor = + | { readonly enforcementPath: string; readonly targetType: 'missing' } + | { + readonly enforcementPath: string; + readonly targetType: 'file' | 'directory' | 'symlink' | 'other'; + readonly identity: FilesystemTargetIdentity; + }; + +/** + * The outcome a mutation can report. Distinct from "did the tool call succeed" + * — a tool that fails to apply is `rejected`; a tool that may have applied + * before losing the ability to confirm is `unknown`. Only `applied` leaves the + * file in a known state. + */ +export type FilesystemMutationOutcome = 'applied' | 'rejected' | 'unknown'; + +/** + * Reasons that, when a *mutating* operation fails with them, mean the file's + * state on disk is genuinely unknown. Each is semantically dispatched: the + * launch-stage reasons are produced only after the child ran (timeout / + * worker_crashed / response_overflow observe a real exit; worker_io_incomplete + * is defined as "ran but the result was lost"), and the protocol-stage reasons + * are produced while parsing the child's own response. So membership here is a + * sufficient signal — `dispatched` is not re-checked for them. Only `aborted` + * straddles the pre-flight and post-dispatch worlds, so it alone is gated on + * the worker error's `dispatched` flag. + * + * Pre-flight failures (validation, transform, the bundle being unavailable, a + * never-started spawn) are deliberately absent: nothing could have been + * written there. + */ +export const UNKNOWN_OUTCOME_REASONS: ReadonlySet = new Set< + FilesystemWorkerClientError['reason'] +>([ + 'timeout', + 'worker_io_incomplete', + 'response_overflow', + 'worker_crashed', + 'invalid_response', + 'response_id_mismatch', + 'response_kind_mismatch', + 'outcome_unknown', +]); + +/** + * Classify the outcome of a failed mutation. Returns `undefined` for errors + * that are not a worker client failure (the caller lets those propagate) and + * for worker failures that cannot have touched the disk (reads, pre-flight + * validation, a never-dispatched spawn, a pre-dispatch abort). Returns + * `'unknown'` when the worker had the request and may have applied it, so the + * caller surfaces a ToolOutcomeUnknownError and the model re-reads instead of + * assuming the call did nothing. + */ +export function classifyFailedMutationOutcome( + error: unknown, +): FilesystemMutationOutcome | undefined { + if (!(error instanceof FilesystemWorkerClientError)) return undefined; + if (error.reason === 'aborted') return error.dispatched === true ? 'unknown' : undefined; + return UNKNOWN_OUTCOME_REASONS.has(error.reason) ? 'unknown' : undefined; +} diff --git a/packages/runtime/src/filesystem-executor.ts b/packages/runtime/src/filesystem-executor.ts index a4769dea75..2773626f7a 100644 --- a/packages/runtime/src/filesystem-executor.ts +++ b/packages/runtime/src/filesystem-executor.ts @@ -10,19 +10,26 @@ // "full access" stricter than ask mode, which grants :slash_tmp outright (#2083). import { Buffer } from 'node:buffer'; -import { realpath } from 'node:fs/promises'; +import { lstat, realpath, stat } from 'node:fs/promises'; import { isAbsolute } from 'node:path'; import type { ExecutionBoundary } from '@maka/core/sandbox-boundary'; import type { PermissionMode } from '@maka/core/permission'; import type { PermissionProfile } from '@maka/core/permission-profile'; +import { ToolOutcomeUnknownError } from '@maka/core/events'; import { computeEditedSource } from './edit-replace.js'; import { createUnifiedDiff } from './unified-diff.js'; +import { + classifyFailedMutationOutcome, + type FilesystemTargetIdentity, +} from './filesystem-authority.js'; +import { StableWriteFailure } from './file-stable-write.js'; +import { applyUpdateToContent } from './apply-patch-file.js'; import { withFileWriteLock } from './file-write-lock.js'; import type { FilesystemWorkerClient, FilesystemWorkerClientOperation, } from './filesystem-worker/client.js'; -import type { ImageMimeType } from './image-file.js'; +import { isSupportedImagePath, type ImageMimeType } from './image-file.js'; import type { FilesystemWorkerResult } from './filesystem-worker/protocol.js'; import { resolveCanonicalDirectoryEntryTarget } from './path-containment.js'; import { normalizeSandboxBoundaryPath } from './sandbox-boundary-path.js'; @@ -31,6 +38,7 @@ import type { WorkspaceEditExecutor, WorkspaceApplyPatchExecutor, WorkspacePathScope, + WorkspaceReadModifyWriteExecutor, WorkspaceSearchExecutor, WorkspaceWriteExecutor, } from './workspace-executor.js'; @@ -91,6 +99,7 @@ export interface FilesystemExecutor { export type FilesystemWorkspaceExecutor = WorkspaceWriteExecutor & WorkspaceEditExecutor & Partial & + Partial & WorkspaceSearchExecutor; export interface BoundaryFilesystemExecutorInput { @@ -119,6 +128,34 @@ function mutates(operation: FilesystemOperation): boolean { ); } +/** + * Capture the target's stable identity at lock acquisition (T0) — *before* + * waiting for the write lock. This is the inode the worker compare-and-swaps + * against, so a path replaced while the call is queued for the lock is detected + * rather than silently written. Returns undefined when the target does not yet + * exist (a create), since there is no inode to pin. + * + * `follow` must match how the worker derives the targetType: content operations + * follow the final symlink (stat), create/delete pin the directory entry (lstat) + * so a swapped link is detected against the entry's own inode. + */ +async function captureIdentityAtLockAcquisition( + canonicalPath: string, + follow: boolean, +): Promise { + try { + const metadata = follow + ? await stat(canonicalPath, { bigint: true }) + : await lstat(canonicalPath, { bigint: true }); + return { dev: String(metadata.dev), ino: String(metadata.ino) }; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + // A missing target (create / new file) has no inode to pin. + if (code === 'ENOENT' || code === 'ENOTDIR') return undefined; + throw error; + } +} + /** * Compose the backends behind one boundary-driven decision. * @@ -150,9 +187,22 @@ export function createBoundaryFilesystemExecutor( 'Managed filesystem execution is unavailable because the sandboxed worker cannot be enforced.', }); }; - async function run(call: FilesystemBackendExecuteInput): Promise { + async function run( + call: FilesystemBackendExecuteInput, + expectedIdentity?: FilesystemTargetIdentity, + ): Promise { const worker = workerFor(call.executionBoundary); - if (!worker) return await local.execute(call, pathScopeForBoundary(call.executionBoundary)); + if (!worker) { + // The local backend consumes the same identity authority as the worker + // (#2600): the pinned read-modify-write validates the T0 identity on the + // descriptor. Remote/isolated workspaces without readModifyWrite stay on + // the path-based fallback, documented as unprotected by the authority. + return await local.execute( + call, + pathScopeForBoundary(call.executionBoundary), + expectedIdentity, + ); + } const result = await worker.execute({ operation: call.operation, // The worker is host-local by definition, so a session opened through a @@ -165,6 +215,7 @@ export function createBoundaryFilesystemExecutor( mode: call.permissionMode ?? 'ask', ...(input.permissionProfile ? { permissionProfile: input.permissionProfile } : {}), ...(call.abortSignal ? { abortSignal: call.abortSignal } : {}), + ...(expectedIdentity ? { expectedIdentity } : {}), }); if (result.kind === 'read_image') { return { @@ -179,27 +230,29 @@ export function createBoundaryFilesystemExecutor( call: Omit, path: string, semantics: 'target' | 'entry' = 'target', - ): Promise { + ): Promise<{ key: string; canonicalPath: string }> { const worker = workerFor(call.executionBoundary); - if (!worker) - return ( + if (!worker) { + const key = ( await input.workspace.writeLockKey({ cwd: call.cwd, path, semantics, }) ).key; + return { key, canonicalPath: key }; + } if (semantics === 'entry') { - return (await resolveCanonicalDirectoryEntryTarget(call.cwd, path)).path; + const resolved = await resolveCanonicalDirectoryEntryTarget(call.cwd, path); + return { key: resolved.path, canonicalPath: resolved.path }; } - return ( - await normalizeSandboxBoundaryPath({ - path, - access: 'write', - scope: 'exact', - cwd: await canonicalExistingPath(call.cwd), - }) - ).enforcementPath; + const normalized = await normalizeSandboxBoundaryPath({ + path, + access: 'write', + scope: 'exact', + cwd: await canonicalExistingPath(call.cwd), + }); + return { key: normalized.enforcementPath, canonicalPath: normalized.enforcementPath }; } return { async execute(call) { @@ -208,37 +261,82 @@ export function createBoundaryFilesystemExecutor( // goes on to reject still takes the same lock as its other spellings. The // key is derived from the same canonicalisation the backend will resolve // with, or the lock-key space and the resolved-path space drift apart. - const key = await writeLockTarget(call, call.operation.path); - return await withFileWriteLock(key, () => run(call)); + const { key, canonicalPath } = await writeLockTarget(call, call.operation.path); + // Capture the target identity at lock acquisition (T0), BEFORE waiting + // for the lock, for BOTH backends — the worker CAS and the local pinned + // read-modify-write compare against this inode. Content operations follow + // the final symlink (stat); apply_patch create/delete use 'entry' + // semantics but execute() only handles write/edit/format_json here. + const expectedIdentity = await captureIdentityAtLockAcquisition(canonicalPath, true); + try { + return await withFileWriteLock(key, () => run(call, expectedIdentity)); + } catch (error) { + throw settleMutationFailure(error); + } }, async applyPatch(call) { const { operation, ...common } = call; const semantics = operation.type === 'update_file' ? 'target' : 'entry'; - const key = await writeLockTarget(common, operation.path, semantics); - return await withFileWriteLock(key, async () => { - const backendOperation: FilesystemWorkerClientOperation = - operation.type === 'delete_file' - ? { kind: 'apply_patch', path: operation.path, action: 'delete' } - : { - kind: 'apply_patch', - path: operation.path, - action: operation.type === 'create_file' ? 'create' : 'update', - diff: operation.diff, - }; - const result = await run({ ...common, operation: backendOperation }); - if (result.kind !== 'apply_patch') { - throw new Error(`ApplyPatch backend returned ${JSON.stringify(result.kind)}.`); - } - return { status: 'completed' }; - }); + const { key, canonicalPath } = await writeLockTarget(common, operation.path, semantics); + // Capture identity at T0 (before the lock wait), for both backends. + // update_file follows the target (stat); create/delete pin the directory + // entry (lstat). + const expectedIdentity = await captureIdentityAtLockAcquisition( + canonicalPath, + semantics === 'target', + ); + try { + return await withFileWriteLock(key, async () => { + const backendOperation: FilesystemWorkerClientOperation = + operation.type === 'delete_file' + ? { kind: 'apply_patch', path: operation.path, action: 'delete' } + : { + kind: 'apply_patch', + path: operation.path, + action: operation.type === 'create_file' ? 'create' : 'update', + diff: operation.diff, + }; + const result = await run({ ...common, operation: backendOperation }, expectedIdentity); + if (result.kind !== 'apply_patch') { + throw new Error(`ApplyPatch backend returned ${JSON.stringify(result.kind)}.`); + } + return { status: 'completed' }; + }); + } catch (error) { + throw settleMutationFailure(error); + } }, }; } +/** + * Settle a failed mutation into its caller-facing error. A pinned-primitive + * failure maps by code: `outcome_unknown` (the write may have partially + * applied) becomes ToolOutcomeUnknownError, `path_changed` becomes a plain + * error with the primitive's actionable message. Worker failures keep the + * post-dispatch classification from the authority contract. + */ +function settleMutationFailure(error: unknown): unknown { + if (error instanceof StableWriteFailure) { + if (error.code === 'outcome_unknown') { + return new ToolOutcomeUnknownError(error.message, { cause: error }); + } + return new Error(error.message, { cause: error }); + } + if (classifyFailedMutationOutcome(error) === 'unknown') { + return new ToolOutcomeUnknownError( + 'Filesystem mutation may have been applied before the worker failed.', + { cause: error }, + ); + } + return error; +} + interface WorkspaceFilesystemBackend { execute( input: FilesystemBackendExecuteInput, scope: WorkspacePathScope, + expectedIdentity?: FilesystemTargetIdentity, ): Promise; } @@ -251,7 +349,7 @@ function createWorkspaceFilesystemExecutor( workspace: FilesystemWorkspaceExecutor, ): WorkspaceFilesystemBackend { return { - async execute({ operation, cwd, abortSignal }, scope) { + async execute({ operation, cwd, abortSignal }, scope, expectedIdentity) { switch (operation.kind) { case 'read': { const { path } = await workspace.resolveExistingPath({ @@ -278,11 +376,35 @@ function createWorkspaceFilesystemExecutor( label: 'Write', scope, }); - // Read-before-write: an overwrite's diff is what tells the reader - // what was lost. Only a missing file means the whole content is - // new — an unreadable or binary existing file leaves the previous - // state unknown, and claiming `--- /dev/null` would report the - // file as created. + if (workspace.readModifyWrite) { + // Pinned RMW (#2600): open once, validate the T0 identity on the + // descriptor, write through it. previous feeds the diff below. + const result = await workspace.readModifyWrite({ + cwd, + path, + label: 'Write', + scope, + approvedIdentity: expectedIdentity, + transform: () => operation.content, + }); + const diff = + result.previous === 'unknown' + ? undefined + : createUnifiedDiff( + path, + result.previous === 'new' ? undefined : result.previous, + operation.content, + ); + return { + kind: 'write', + ok: true, + path, + bytes: Buffer.byteLength(operation.content, 'utf8'), + ...(diff !== undefined ? { diff } : {}), + }; + } + // Fallback (remote/isolated workspace): path-based, unprotected by + // the identity authority. let previous: 'new' | 'unknown' | string; try { const read = await workspace.readFile({ cwd, path }); @@ -311,9 +433,31 @@ function createWorkspaceFilesystemExecutor( case 'apply_patch': { if (!workspace.applyPatch) throw new Error('Workspace does not support ApplyPatch'); const common = { cwd, path: operation.path, label: 'ApplyPatch', scope }; + if (operation.action === 'update' && workspace.readModifyWrite) { + // update requires an existing target: resolve first (ENOENT guard, + // matching the worker's resolveExistingAllowed) so a missing target + // is rejected without the exclusive create ever running. + const { path } = await workspace.resolveExistingPath({ + cwd, + path: operation.path, + label: 'ApplyPatch', + scope, + }); + await workspace.readModifyWrite({ + ...common, + path, + approvedIdentity: expectedIdentity, + transform: (ctx) => applyUpdateToContent(ctx.content ?? '', operation.diff), + }); + return { kind: 'apply_patch', ok: true, path }; + } const patched = await workspace.applyPatch( operation.action === 'delete' - ? { ...common, action: 'delete' } + ? { + ...common, + action: 'delete' as const, + ...(expectedIdentity ? { approvedIdentity: expectedIdentity } : {}), + } : { ...common, action: operation.action, diff: operation.diff }, ); return { kind: 'apply_patch', ok: true, path: patched.path }; @@ -325,6 +469,41 @@ function createWorkspaceFilesystemExecutor( label: 'Edit', scope, }); + if (isSupportedImagePath(path)) throw new Error('Edit does not support image files.'); + if (workspace.readModifyWrite) { + let edited!: ReturnType; + const result = await workspace.readModifyWrite({ + cwd, + path, + label: 'Edit', + scope, + approvedIdentity: expectedIdentity, + transform: (ctx) => { + edited = computeEditedSource( + ctx.content ?? '', + operation.oldString, + operation.newString, + operation.path, + ); + return edited.content; + }, + }); + const diff = createUnifiedDiff( + path, + result.previous === 'unknown' || result.previous === 'new' ? '' : result.previous, + result.finalContent ?? '', + ); + return { + kind: 'edit', + ok: true, + path, + replacements: 1, + matchedVia: edited.matchedVia, + startLine: edited.startLine, + endLine: edited.endLine, + ...(diff !== undefined ? { diff } : {}), + }; + } const read = await workspace.readFile({ cwd, path }); if ('bytes' in read) throw new Error('Edit does not support image files.'); const edited = computeEditedSource( @@ -353,6 +532,60 @@ function createWorkspaceFilesystemExecutor( label: 'FormatJson', scope, }); + if (isSupportedImagePath(path)) { + throw new Error('FormatJson does not support image files.'); + } + if (workspace.readModifyWrite) { + let parseError: string | undefined; + let original = ''; + const result = await workspace.readModifyWrite({ + cwd, + path, + label: 'FormatJson', + scope, + approvedIdentity: expectedIdentity, + transform: (ctx) => { + original = ctx.content ?? ''; + try { + const value = operation.sortKeys + ? sortKeysDeep(JSON.parse(original)) + : JSON.parse(original); + return JSON.stringify(value, null, 2); + } catch (error) { + parseError = error instanceof Error ? error.message : 'parse failed'; + return null; + } + }, + }); + const bytesBefore = Buffer.byteLength(original, 'utf8'); + if (parseError !== undefined || result.finalContent === null) { + return { + kind: 'format_json', + ok: false, + valid: false, + error: `FormatJson: invalid JSON: ${parseError ?? 'parse failed'}`, + path, + bytesBefore, + byteDelta: 0, + changed: false, + }; + } + const formatted = result.finalContent; + const bytesAfter = Buffer.byteLength(formatted, 'utf8'); + const diff = + formatted === original ? undefined : createUnifiedDiff(path, original, formatted); + return { + kind: 'format_json', + ok: true, + valid: true, + path, + bytesBefore, + bytesAfter, + byteDelta: bytesAfter - bytesBefore, + changed: formatted !== original, + ...(diff !== undefined ? { diff } : {}), + }; + } const read = await workspace.readFile({ cwd, path }); if ('bytes' in read) throw new Error('FormatJson does not support image files.'); const original = read.content; diff --git a/packages/runtime/src/filesystem-worker/client.ts b/packages/runtime/src/filesystem-worker/client.ts index 14db96891d..3e3a23a6aa 100644 --- a/packages/runtime/src/filesystem-worker/client.ts +++ b/packages/runtime/src/filesystem-worker/client.ts @@ -58,6 +58,13 @@ export interface FilesystemWorkerExecuteInput { /** Explicit embedding policy. Mode-based defaults are compiled only when omitted. */ permissionProfile?: PermissionProfile; abortSignal?: AbortSignal; + /** + * The target identity captured at lock acquisition (T0), passed in by the + * caller so the client does not re-derive it after acquiring the lock (T1). + * The worker compare-and-swaps this against the on-disk inode. Undefined for + * a missing target (create) or when the host-local backend is in use. + */ + expectedIdentity?: { dev: string; ino: string }; } export type FilesystemWorkerClientErrorReason = @@ -67,6 +74,7 @@ export type FilesystemWorkerClientErrorReason = | 'worker_bundle_unavailable' | 'runtime_executable_unavailable' | 'spawn_failed' + | 'worker_io_incomplete' | 'timeout' | 'aborted' | 'response_overflow' @@ -91,6 +99,14 @@ export class FilesystemWorkerClientError extends Error { readonly backend?: 'none' | 'macos-seatbelt' | 'linux' | 'windows'; readonly profileName?: string; readonly requiredExpansion?: SandboxBoundaryExpansion; + /** + * Whether the request had been dispatched to the worker before this failure. + * Only meaningful for `'launch'`/`'protocol'` failures: `undefined` means + * "the question does not apply" (pre-flight validation). When `false` the + * child never ran, so nothing on disk could have changed; when `true` the + * child ran and the outcome on disk is genuinely unknown. + */ + readonly dispatched?: boolean; constructor(input: { reason: FilesystemWorkerClientErrorReason; @@ -101,6 +117,7 @@ export class FilesystemWorkerClientError extends Error { backend?: 'none' | 'macos-seatbelt' | 'linux' | 'windows'; profileName?: string; requiredExpansion?: SandboxBoundaryExpansion; + dispatched?: boolean; }) { super(input.message ?? `Filesystem worker failed: ${input.reason}.`); this.name = 'FilesystemWorkerClientError'; @@ -111,6 +128,7 @@ export class FilesystemWorkerClientError extends Error { this.backend = input.backend; this.profileName = input.profileName; this.requiredExpansion = input.requiredExpansion; + this.dispatched = input.dispatched; } } @@ -127,7 +145,12 @@ export class FilesystemWorkerClient { async execute(input: FilesystemWorkerExecuteInput): Promise { const requestId = this.newId(); - if (input.abortSignal?.aborted) throw clientError('aborted', 'launch', requestId); + if (input.abortSignal?.aborted) { + // Pre-flight cancel: nothing has been dispatched, so this is a clean + // cancellation, not an unknown outcome. Carrying dispatched:false lets + // the host tell the two apart instead of blaming every queued tool. + throw clientError('aborted', 'launch', requestId, undefined, false, {}, false); + } if (input.executionBoundary && input.executionBoundary.kind !== 'managed') { throw clientError( 'invalid_request', @@ -167,6 +190,34 @@ export class FilesystemWorkerClient { ).catch(() => { throw clientError('invalid_operation', 'validation', requestId); }); + // The identity was captured by the caller at lock acquisition (T0) and + // passed in as expectedIdentity. Do NOT re-derive it here: re-deriving at + // this point (after the lock is held) would sample the post-queue inode, + // making the CAS self-fulfilling and re-opening the queue window. + // + // Missing↔existing transitions while queued are reconciled here, against + // the T1 reality the target normaliser just derived: + // - T0 existing (identity present) but T1 missing: the target was removed + // while this call waited — typically a cooperative Maka delete that ran + // first under the same write lock. Drop the stale identity and let the + // mutation proceed as a fresh exclusive create ("delete then rewrite" + // stays a clean apply; a rename-swap is NOT this case — it leaves an + // existing inode and is caught by the identity comparison instead). + // - T0 missing (no identity) but T1 existing: the target was created while + // this call waited. Writing would clobber content this call never saw, + // so fail with a meaningful path_changed (never invalid_request). + const identity = + input.expectedIdentity && target.targetType !== 'missing' + ? input.expectedIdentity + : undefined; + if (!identity && target.targetType !== 'missing' && access === 'write') { + throw clientError( + 'path_changed', + 'validation', + requestId, + 'The target was created while this call waited for the lock; re-read before writing.', + ); + } const compiled = input.executionBoundary?.kind === 'managed' ? { @@ -246,6 +297,7 @@ export class FilesystemWorkerClient { access, scope: target.scope, targetType: target.targetType, + ...(identity ? { identity } : {}), }, } as const; const requestJson = JSON.stringify(request); @@ -386,15 +438,63 @@ export class FilesystemWorkerClient { timeoutMs: this.timeoutMs, ...(input.abortSignal ? { abortSignal: input.abortSignal } : {}), }); - } catch { - throw clientError('spawn_failed', 'launch', requestId); + } catch (error) { + // The process-runner attaches a `dispatched` flag to the rejection so we + // can tell "the child never started" (spawn_failed — clean) from "the + // child ran but its result was lost" (worker_io_incomplete — the outcome + // on disk is unknown). A thrown error without the flag (e.g. spawn() + // itself raising before any 'spawn' event could fire) is treated as + // never-dispatched. + const dispatched = (error as { dispatched?: boolean } | null)?.dispatched === true; + throw clientError( + dispatched ? 'worker_io_incomplete' : 'spawn_failed', + 'launch', + requestId, + undefined, + false, + {}, + dispatched, + ); } finally { pinnedTarget?.releaseSource(); pinnedRuntimeWritableRoot?.releaseSource(); } - if (processResult.timedOut) throw clientError('timeout', 'launch', requestId); - if (processResult.aborted) throw clientError('aborted', 'launch', requestId); - if (processResult.responseOverflow) throw clientError('response_overflow', 'launch', requestId); + if (processResult.timedOut) { + throw clientError( + 'timeout', + 'launch', + requestId, + undefined, + false, + {}, + processResult.dispatched, + ); + } + if (processResult.aborted) { + // A post-dispatch abort means the child had the request and may have + // acted on it before being killed; carry dispatched so the host can + // classify it as an unknown outcome rather than a clean cancel. + throw clientError( + 'aborted', + 'launch', + requestId, + undefined, + false, + {}, + processResult.dispatched, + ); + } + if (processResult.responseOverflow) { + throw clientError( + 'response_overflow', + 'launch', + requestId, + undefined, + false, + {}, + processResult.dispatched, + ); + } if (processResult.exitCode !== 0) { const brokerFailure = transformed.sandboxType === 'windows' @@ -415,6 +515,9 @@ export class FilesystemWorkerClient { 'launch', requestId, processResult.stderrTail || undefined, + false, + {}, + processResult.dispatched, ); } @@ -422,11 +525,18 @@ export class FilesystemWorkerClient { try { response = parseFilesystemWorkerResponse(JSON.parse(processResult.stdout)); } catch { - throw clientError('invalid_response', 'protocol', requestId); + // The child produced output we could not parse, but it ran and emitted + // something, so the request was dispatched (the on-disk outcome is + // unknown for a mutation). + throw clientError('invalid_response', 'protocol', requestId, undefined, false, {}, true); } if (response.requestId !== requestId) - throw clientError('response_id_mismatch', 'protocol', requestId); + throw clientError('response_id_mismatch', 'protocol', requestId, undefined, false, {}, true); if (!response.ok) { + // A well-formed worker error means the child ran and answered: dispatch + // had happened. Carry dispatched:true so a mutating op that fails here + // (e.g. the worker reports `outcome_unknown` after a partial write) is + // classified as an unknown outcome rather than slipping through. throw clientError( response.error.code, 'operation', @@ -437,13 +547,22 @@ export class FilesystemWorkerClient { backend: transformed.exec.sandboxType, profileName: effectiveProfile.name ?? effectiveProfile.type, }, + true, ); } if ( response.result.kind !== operation.kind && !(operation.kind === 'read' && response.result.kind === 'read_image') ) { - throw clientError('response_kind_mismatch', 'protocol', requestId); + throw clientError( + 'response_kind_mismatch', + 'protocol', + requestId, + undefined, + false, + {}, + true, + ); } return response.result; } @@ -559,6 +678,7 @@ function clientError( profileName?: string; requiredExpansion?: SandboxBoundaryExpansion; } = {}, + dispatched?: boolean, ): FilesystemWorkerClientError { return new FilesystemWorkerClientError({ reason, @@ -567,5 +687,6 @@ function clientError( message, recoverable, ...metadata, + ...(dispatched !== undefined ? { dispatched } : {}), }); } diff --git a/packages/runtime/src/filesystem-worker/operations.ts b/packages/runtime/src/filesystem-worker/operations.ts index 2bf5c56255..5dad9dd871 100644 --- a/packages/runtime/src/filesystem-worker/operations.ts +++ b/packages/runtime/src/filesystem-worker/operations.ts @@ -7,12 +7,20 @@ import { sandboxPathApi } from './sandbox-paths.js'; import { sandboxBoundaryExpansionAllowsPath } from '@maka/core/sandbox-boundary'; import { ApplyPatchRejectedError, + applyUpdateToContent, createPatchedFile, - updatePatchedFile, } from '../apply-patch-file.js'; import { computeEditedSource } from '../edit-replace.js'; import { createUnifiedDiff } from '../unified-diff.js'; +import { + compareAndDeleteEntry, + hostVisibilityAfterWrite, + openStableTarget, + readModifyWriteThroughHandle, + StableWriteFailure, + writeThroughHandle, +} from '../file-stable-write.js'; import { isSupportedImagePath, readWorkspaceImage } from '../image-file.js'; import { FILESYSTEM_WORKER_PROTOCOL_VERSION, @@ -59,6 +67,12 @@ export type FilesystemWorkerGrepRunner = ( input: FilesystemWorkerGrepRunInput, ) => Promise; +function operationAccess(kind: FilesystemWorkerOperation['kind']): 'read' | 'write' { + return kind === 'write' || kind === 'apply_patch' || kind === 'edit' || kind === 'format_json' + ? 'write' + : 'read'; +} + export async function executeFilesystemWorkerRequest( request: FilesystemWorkerRequest, dependencies: FilesystemWorkerOperationDependencies = {}, @@ -69,6 +83,7 @@ export async function executeFilesystemWorkerRequest( request.operation.path, request.expectedTarget, operationUsesDirectoryEntry(request.operation), + operationAccess(request.operation.kind), ); return { version: FILESYSTEM_WORKER_PROTOCOL_VERSION, @@ -78,6 +93,7 @@ export async function executeFilesystemWorkerRequest( request.operation, request.operationBoundary, dependencies, + request.expectedTarget, ), }; } catch (error) { @@ -95,6 +111,7 @@ export async function executeFilesystemOperation( operation: FilesystemWorkerOperation, operationBoundary: FilesystemWorkerRequest['operationBoundary'], dependencies: FilesystemWorkerOperationDependencies = {}, + expectedTarget?: FilesystemWorkerTarget, ): Promise { switch (operation.kind) { case 'read': { @@ -135,29 +152,47 @@ export async function executeFilesystemOperation( 'Write', operationBoundary, ); - // Read-before-write: the diff of an overwrite is what tells the reader - // what was lost. Only a missing file means the whole content is new — - // any other read failure leaves the previous state unknown, and - // claiming `--- /dev/null` would report the file as created. - let previous: 'new' | 'unknown' | string; + // Pin the approved object (#2600): open once, validate the identity on + // the descriptor, and write through that descriptor — a path swap between + // validation and the write cannot divert the bytes onto the replacement. + // An approved-missing target is created exclusively; anything that + // appeared in the gap is `path_changed`, never truncated. + const handle = await openStableTarget({ + path, + approvedIdentity: expectedTarget?.identity, + }); try { - previous = await fs.readFile(path, 'utf8'); - } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - previous = code === 'ENOENT' || code === 'ENOTDIR' ? 'new' : 'unknown'; + // Read-before-write (for the diff): only through the pinned descriptor. + // An approved-missing target was just created by 'wx', so it is new. + let previous: 'new' | 'unknown' | string; + if (!expectedTarget?.identity) { + previous = 'new'; + } else { + try { + previous = await handle.readFile('utf8'); + } catch { + previous = 'unknown'; + } + } + await writeThroughHandle(handle, operation.content); + // Host visibility: if the path no longer resolves to the pinned inode, + // the bytes went to an orphan and the visible file is the replacement. + const visibility = await hostVisibilityAfterWrite(path, handle); + if (visibility) throw visibility; + const diff = + previous === 'unknown' + ? undefined + : createUnifiedDiff(path, previous === 'new' ? undefined : previous, operation.content); + return { + kind: 'write', + ok: true, + path, + bytes: Buffer.byteLength(operation.content, 'utf8'), + ...(diff !== undefined ? { diff } : {}), + }; + } finally { + await handle.close(); } - await fs.writeFile(path, operation.content, 'utf8'); - const diff = - previous === 'unknown' - ? undefined - : createUnifiedDiff(path, previous === 'new' ? undefined : previous, operation.content); - return { - kind: 'write', - ok: true, - path, - bytes: Buffer.byteLength(operation.content, 'utf8'), - ...(diff !== undefined ? { diff } : {}), - }; } case 'apply_patch': { if (operation.action !== 'update') { @@ -168,7 +203,15 @@ export async function executeFilesystemOperation( operationBoundary, ); if (operation.action === 'create') await createPatchedFile(path, operation.diff); - else await fs.unlink(path); + // Compare-and-delete (#2600): rename the entry to a tombstone, + // verify the approved identity, then unlink — a replacement swapped + // in after the check is restored and reported, never silently + // deleted. + else + await compareAndDeleteEntry({ + path, + approvedIdentity: expectedTarget?.identity, + }); return { kind: 'apply_patch', ok: true, path }; } const path = await resolveExistingAllowed( @@ -178,7 +221,21 @@ export async function executeFilesystemOperation( 'write', operationBoundary, ); - await updatePatchedFile(path, operation.diff); + // Pin the target and apply the diff through one descriptor (#2600); a + // rejected patch propagates before any truncation, so the file is intact. + const handle = await openStableTarget({ + path, + approvedIdentity: expectedTarget?.identity, + }); + try { + await readModifyWriteThroughHandle(handle, (existing) => + applyUpdateToContent(existing, operation.diff), + ); + const visibility = await hostVisibilityAfterWrite(path, handle); + if (visibility) throw visibility; + } finally { + await handle.close(); + } return { kind: 'apply_patch', ok: true, path }; } case 'edit': { @@ -189,33 +246,43 @@ export async function executeFilesystemOperation( 'write', operationBoundary, ); - const content = await fs.readFile(path, 'utf8'); - let edited: ReturnType; + const handle = await openStableTarget({ + path, + approvedIdentity: expectedTarget?.identity, + }); try { - edited = computeEditedSource( - content, - operation.oldString, - operation.newString, - operation.path, - ); - } catch (error) { - throw operationError( - 'edit_conflict', - error instanceof Error ? error.message : 'Edit could not be applied.', - ); + const content = await handle.readFile('utf8'); + let source: ReturnType; + try { + source = computeEditedSource( + content, + operation.oldString, + operation.newString, + operation.path, + ); + } catch (error) { + throw operationError( + 'edit_conflict', + error instanceof Error ? error.message : 'Edit could not be applied.', + ); + } + await writeThroughHandle(handle, source.content); + const visibility = await hostVisibilityAfterWrite(path, handle); + if (visibility) throw visibility; + const diff = createUnifiedDiff(path, content, source.content); + return { + kind: 'edit', + ok: true, + path, + replacements: 1, + matchedVia: source.matchedVia, + startLine: source.startLine, + endLine: source.endLine, + ...(diff !== undefined ? { diff } : {}), + }; + } finally { + await handle.close(); } - await fs.writeFile(path, edited.content, 'utf8'); - const diff = createUnifiedDiff(path, content, edited.content); - return { - kind: 'edit', - ok: true, - path, - replacements: 1, - matchedVia: edited.matchedVia, - startLine: edited.startLine, - endLine: edited.endLine, - ...(diff !== undefined ? { diff } : {}), - }; } case 'format_json': { const path = await resolveExistingAllowed( @@ -225,39 +292,56 @@ export async function executeFilesystemOperation( 'write', operationBoundary, ); - const original = await fs.readFile(path, 'utf8'); - const bytesBefore = Buffer.byteLength(original, 'utf8'); - let parsed: unknown; + const handle = await openStableTarget({ + path, + approvedIdentity: expectedTarget?.identity, + }); try { - parsed = JSON.parse(original); - } catch (error) { + const original = await handle.readFile('utf8'); + const bytesBefore = Buffer.byteLength(original, 'utf8'); + let parsed: unknown; + try { + parsed = JSON.parse(original); + } catch (error) { + // Invalid JSON: return the structured failure without writing. + return { + kind: 'format_json', + ok: false, + valid: false, + path, + error: `FormatJson: invalid JSON: ${error instanceof Error ? error.message : 'parse failed'}`, + bytesBefore, + byteDelta: 0, + changed: false, + }; + } + const formatted = JSON.stringify( + operation.sortKeys ? sortKeysDeep(parsed) : parsed, + null, + 2, + ); + if (formatted !== original) { + await writeThroughHandle(handle, formatted); + } + const visibility = await hostVisibilityAfterWrite(path, handle); + if (visibility) throw visibility; + const bytesAfter = Buffer.byteLength(formatted, 'utf8'); + const diff = + formatted === original ? undefined : createUnifiedDiff(path, original, formatted); return { kind: 'format_json', - ok: false, - valid: false, + ok: true, + valid: true, path, - error: `FormatJson: invalid JSON: ${error instanceof Error ? error.message : 'parse failed'}`, bytesBefore, - byteDelta: 0, - changed: false, + bytesAfter, + byteDelta: bytesAfter - bytesBefore, + changed: formatted !== original, + ...(diff !== undefined ? { diff } : {}), }; + } finally { + await handle.close(); } - const formatted = JSON.stringify(operation.sortKeys ? sortKeysDeep(parsed) : parsed, null, 2); - await fs.writeFile(path, formatted, 'utf8'); - const bytesAfter = Buffer.byteLength(formatted, 'utf8'); - const diff = - formatted === original ? undefined : createUnifiedDiff(path, original, formatted); - return { - kind: 'format_json', - ok: true, - valid: true, - path, - bytesBefore, - bytesAfter, - byteDelta: bytesAfter - bytesBefore, - changed: formatted !== original, - ...(diff !== undefined ? { diff } : {}), - }; } case 'glob': { assertContainedGlobPattern(operation.pattern); @@ -363,6 +447,9 @@ function sortKeysDeep(value: unknown): unknown { function normalizeOperationError(error: unknown): FilesystemOperationError { if (error instanceof FilesystemOperationError) return error; + if (error instanceof StableWriteFailure) { + return operationError(error.code, error.message); + } if (error instanceof ApplyPatchRejectedError) { return operationError('edit_conflict', error.message); } @@ -379,6 +466,7 @@ async function assertTargetUnchanged( path: string, expected: FilesystemWorkerTarget, noFollowFinalSymlink = false, + access: 'read' | 'write' = 'read', ): Promise { const enforcementPath = noFollowFinalSymlink ? (await resolveCanonicalDirectoryEntryTarget(cwd, path)).path @@ -392,6 +480,36 @@ async function assertTargetUnchanged( 'The approved filesystem target changed before execution.', ); } + // Compare the on-disk identity against the one captured at authorisation + // time. This is the load-bearing check for the queue window: a path swapped + // while the call waited for the lock has a different inode even when its + // canonical path and type still match. + // + // A non-missing WRITE target MUST carry an identity — if it does not, the + // CAS is silently skipped and the entire defence collapses. Fail loudly + // rather than degrading to "no check", so a buggy caller that omits the + // identity is caught immediately instead of leaving the window open. Reads + // are exempt: they do not mutate, so there is no queue window to close. + if (access === 'write' && expected.targetType !== 'missing' && !expected.identity) { + throw operationError( + 'invalid_request', + 'A non-missing filesystem target must carry an identity for CAS.', + ); + } + if (expected.identity) { + const metadata = noFollowFinalSymlink + ? await fs.lstat(enforcementPath, { bigint: true }) + : await fs.stat(enforcementPath, { bigint: true }); + if ( + String(metadata.dev) !== expected.identity.dev || + String(metadata.ino) !== expected.identity.ino + ) { + throw operationError( + 'path_changed', + 'The approved filesystem target changed before execution.', + ); + } + } } async function resolveWritableAllowed( diff --git a/packages/runtime/src/filesystem-worker/process-runner.ts b/packages/runtime/src/filesystem-worker/process-runner.ts index 63a8eeb79b..3409844dff 100644 --- a/packages/runtime/src/filesystem-worker/process-runner.ts +++ b/packages/runtime/src/filesystem-worker/process-runner.ts @@ -38,6 +38,15 @@ export interface FilesystemWorkerProcessRunResult { timedOut: boolean; aborted: boolean; responseOverflow: boolean; + /** + * Whether the request was dispatched to the child process. True once Node's + * `'spawn'` event fires (the process actually started) AND stdin was fully + * written; false when the process never started or stdin was not yet + * delivered. Callers use this to tell a clean "never ran" failure from a + * "ran but the result was lost" failure — only the latter can have mutated + * anything on disk. + */ + dispatched: boolean; } export type FilesystemWorkerProcessRunner = ( @@ -60,6 +69,7 @@ export async function runFilesystemWorkerProcess( timedOut: false, aborted: true, responseOverflow: false, + dispatched: false, }; } let child: WorkerChildProcess; @@ -93,6 +103,20 @@ async function observeWorker( let responseOverflow = false; let termination: 'timeout' | 'abort' | 'overflow' | undefined; let settled = false; + // Dispatched becomes true only after the child has actually started (the + // 'spawn' event fired) and the request has been written to its stdin. + // Before that point no filesystem mutation can have happened; afterwards a + // failure means the outcome on disk is genuinely unknown. + let dispatched = false; + let dispatchedSpawned = false; + let dispatchedStdin = false; + const maybeDispatched = (): void => { + if (dispatchedSpawned && dispatchedStdin) dispatched = true; + }; + child.once('spawn', () => { + dispatchedSpawned = true; + maybeDispatched(); + }); child.stdout.on('data', (chunk: Buffer) => { if (responseOverflow) return; stdoutBytes += chunk.length; @@ -118,23 +142,29 @@ async function observeWorker( ioDrainTimeoutMs, }, ); - void lifecycle.completion.then((outcome) => { - if (settled) return; - if (!outcome.ioDrained) { - rejectOnce(new Error('Filesystem worker output did not drain before lifecycle deadline')); - return; - } - settled = true; - cleanup(); - resolvePromise({ - exitCode: outcome.exitCode ?? 1, - stdout: responseOverflow ? '' : Buffer.concat(stdoutChunks).toString('utf8'), - stderrTail: stderrTail.toString('utf8'), - timedOut: termination === 'timeout', - aborted: termination === 'abort', - responseOverflow, - }); - }, rejectOnce); + void lifecycle.completion.then( + (outcome) => { + if (settled) return; + if (!outcome.ioDrained) { + rejectOnce( + workerError('Filesystem worker output did not drain before lifecycle deadline'), + ); + return; + } + settled = true; + cleanup(); + resolvePromise({ + exitCode: outcome.exitCode ?? 1, + stdout: responseOverflow ? '' : Buffer.concat(stdoutChunks).toString('utf8'), + stderrTail: stderrTail.toString('utf8'), + timedOut: termination === 'timeout', + aborted: termination === 'abort', + responseOverflow, + dispatched, + }); + }, + (error: unknown) => rejectOnce(workerError(error)), + ); const timeout = setTimeout(() => terminate('timeout'), timeoutMs); const abort = () => terminate('abort'); if (input.abortSignal) { @@ -148,11 +178,24 @@ async function observeWorker( settled = true; cleanup(); lifecycle.forceKill(); - reject(error); + reject(workerError(error)); return; } + // The request has been queued to the child's stdin. If the 'spawn' event + // has already fired this completes dispatch; if it fires later the spawn + // listener flips the flag. Either way, after this line a subsequent + // failure means "the child may have run". + dispatchedStdin = true; + maybeDispatched(); child.stdin.end(input.stdin); + /** Attach the current `dispatched` flag to an error so callers can classify it. */ + function workerError(error: unknown): Error { + const cause = error instanceof Error ? error : new Error(String(error)); + Object.defineProperty(cause, 'dispatched', { value: dispatched, enumerable: true }); + return cause; + } + function terminate(reason: 'timeout' | 'abort' | 'overflow'): void { if (termination || settled) return; termination = reason; diff --git a/packages/runtime/src/filesystem-worker/protocol.ts b/packages/runtime/src/filesystem-worker/protocol.ts index e42460f346..00f09015cd 100644 --- a/packages/runtime/src/filesystem-worker/protocol.ts +++ b/packages/runtime/src/filesystem-worker/protocol.ts @@ -1,12 +1,23 @@ import { z } from 'zod'; import { validateSandboxBoundaryExpansion } from '@maka/core/sandbox-boundary'; -// v5 adds the provider-native single-file ApplyPatch operation. -export const FILESYSTEM_WORKER_PROTOCOL_VERSION = 5 as const; +// v6 adds the captured target identity (opaque decimal-string dev/ino) to +// FilesystemWorkerTarget, so the worker can compare-and-swap against the +// inode that was authorised at lock acquisition instead of only the path +// string. The identity is carried as strings because bigint cannot cross the +// JSON protocol boundary. +export const FILESYSTEM_WORKER_PROTOCOL_VERSION = 6 as const; const path = z.string().min(1).max(4096); const cwd = z.string().min(1).max(4096); +// Opaque identity strings: `String(stats.dev)` / `String(stats.ino)`. Decimal +// only so they survive JSON round-trips; compared for equality on the worker. +const decimalString = z.string().regex(/^\d+$/); +const FilesystemTargetIdentitySchema = z + .object({ dev: decimalString, ino: decimalString }) + .strict(); + const OperationBoundarySchema = z .object({ filesystem: z @@ -42,8 +53,22 @@ export const FilesystemWorkerTargetSchema = z access: z.enum(['read', 'write']), scope: z.enum(['exact', 'subtree']), targetType: z.enum(['file', 'directory', 'symlink', 'other', 'missing']), + // Captured at lock acquisition (T0). Present (and required) for every + // targetType except 'missing'. The contract module (FilesystemTargetDescriptor) + // models this as a discriminated union; the wire schema keeps the field + // optional so it can omit it for missing targets, and the client that + // builds the request enforces the invariant. + identity: FilesystemTargetIdentitySchema.optional(), }) - .strict(); + .strict() + .superRefine((target, context) => { + if (target.targetType === 'missing' && target.identity !== undefined) { + context.addIssue({ + code: 'custom', + message: 'A missing target cannot carry an identity.', + }); + } + }); export const FilesystemWorkerOperationSchema = z.union([ z @@ -175,6 +200,14 @@ export const FilesystemWorkerErrorCodeSchema = z.enum([ 'sandbox_denied', 'filesystem_denied', 'filesystem_error', + // The worker may have applied the mutation before it lost the ability to + // report back (e.g. it wrote the file then the post-write identity check + // found the on-path inode no longer matches the one it wrote). The host + // treats this as an unknown outcome on disk, not a clean failure. + 'outcome_unknown', + // The entry-delete path refuses directories outright (#2600): a directory + // cannot be unlinked, only recursively removed — a different operation. + 'is_directory', ]); export const FilesystemWorkerResponseSchema = z.discriminatedUnion('ok', [ diff --git a/packages/runtime/src/workspace-executor.ts b/packages/runtime/src/workspace-executor.ts index 2408eb8255..3d41dc04a6 100644 --- a/packages/runtime/src/workspace-executor.ts +++ b/packages/runtime/src/workspace-executor.ts @@ -8,6 +8,12 @@ import { resolveCanonicalDirectoryEntryTarget, } from './path-containment.js'; import { createPatchedFile, updatePatchedFile } from './apply-patch-file.js'; +import { + compareAndDeleteEntry, + hostVisibilityAfterWrite, + openStableTarget, + writeThroughHandle, +} from './file-stable-write.js'; import { promisify } from 'node:util'; import type { ToolExecutionFacts } from '@maka/core/permission'; import { runProcessWithBoundedTail, runShellWithBoundedTail } from './shell-exec.js'; @@ -88,13 +94,55 @@ export interface WorkspaceWriteFileResult { } export type WorkspaceApplyPatchInput = WorkspaceResolvePathInput & - ({ action: 'create' | 'update'; diff: string } | { action: 'delete' }); + ({ action: 'create' | 'update'; diff: string } | { action: 'delete' }) & { + /** + * Captured at lock acquisition; carried through to the compare-and-delete + * guard for delete (#2600). Optional so external callers are unaffected. + */ + approvedIdentity?: { dev: string; ino: string }; + }; export interface WorkspaceApplyPatchResult { ok: true; path: string; } +/** + * A read-modify-write pinned to one file descriptor, enforcing the + * filesystem-authority contract (#2600): the approved object is opened once, + * its identity validated on the descriptor, and the read/transform/write all + * run through that descriptor. The local executor implements this; a remote or + * isolated workspace cannot pin host descriptors and stays on the path-based + * readFile/writeFile fallback (documented as unprotected by the identity + * authority). + */ +export interface WorkspaceReadModifyWriteInput { + cwd: string; + path: string; + label: string; + scope: WorkspacePathScope; + /** Captured at lock acquisition; undefined for an approved-missing target. */ + approvedIdentity?: { dev: string; ino: string }; + /** + * Compute the new content from the pinned read. Return null to not write + * (e.g. invalid JSON in FormatJson) — nothing is modified. + */ + transform: (existing: { content: string | null; existed: boolean }) => string | null; +} + +export interface WorkspaceReadModifyWriteResult { + path: string; + /** What the pinned read saw, for the caller's diff. */ + previous: 'new' | 'unknown' | string; + /** The content that was (or would have been) written. */ + finalContent: string | null; + written: boolean; +} + +export interface WorkspaceReadModifyWriteExecutor { + readModifyWrite(input: WorkspaceReadModifyWriteInput): Promise; +} + /** * Which path space a resolution may land in. * @@ -226,7 +274,8 @@ export interface WorkspaceExecutor WorkspaceEditExecutor, WorkspaceGlobExecutor, WorkspaceGrepExecutor, - Partial {} + Partial, + Partial {} export class LocalWorkspaceExecutor implements WorkspaceExecutor { readonly facts = LOCAL_WORKSPACE_EXECUTOR_FACTS; @@ -276,6 +325,53 @@ export class LocalWorkspaceExecutor implements WorkspaceExecutor { }; } + async readModifyWrite( + input: WorkspaceReadModifyWriteInput, + ): Promise { + // Pin the approved object (#2600): open once, validate the identity on the + // descriptor, read/transform/write through it. A path swap mid-operation + // cannot divert the bytes; a failed validation leaves the file untouched. + // Resolve into the same canonical path space as every other operation so + // the approved identity (captured on the canonical path) matches what we + // open; callers may pass either a raw or an already-resolved path. + const path = input.approvedIdentity + ? await resolveExistingPathInScope(input.cwd, input.path, input.label, input.scope) + : (await canonicalPathInScope(input.cwd, input.path, input.label, input.scope)).path; + const handle = await openStableTarget({ + path, + approvedIdentity: input.approvedIdentity, + }); + try { + const existed = input.approvedIdentity !== undefined; + let previous: 'new' | 'unknown' | string; + let content: string | null = null; + if (!existed) { + previous = 'new'; // just created by the exclusive open + } else if (isSupportedImagePath(path)) { + // Binary/image targets are never read as text: the previous state is + // unknown and no diff may claim /dev/null. + previous = 'unknown'; + } else { + try { + content = await handle.readFile('utf8'); + previous = content; + } catch { + previous = 'unknown'; + } + } + const replacement = input.transform({ content, existed }); + if (replacement === null) { + return { path, previous, finalContent: null, written: false }; + } + await writeThroughHandle(handle, replacement); + const visibility = await hostVisibilityAfterWrite(path, handle); + if (visibility) throw visibility; + return { path, previous, finalContent: replacement, written: true }; + } finally { + await handle.close(); + } + } + async applyPatch(input: WorkspaceApplyPatchInput): Promise { if (input.action !== 'update') { const path = await resolveDirectoryEntryPathInScope( @@ -285,7 +381,13 @@ export class LocalWorkspaceExecutor implements WorkspaceExecutor { input.scope, ); if (input.action === 'create') await createPatchedFile(path, input.diff); - else await fs.unlink(path); + // Compare-and-delete (#2600): a replacement swapped in after the check + // is restored and reported, never silently deleted. + else + await compareAndDeleteEntry({ + path, + approvedIdentity: input.approvedIdentity, + }); return { ok: true, path }; } const path = await resolveExistingPathInScope(input.cwd, input.path, input.label, input.scope);