diff --git a/packages/local-runtime/src/turns/file-changes.ts b/packages/local-runtime/src/turns/file-changes.ts index 2a3ff033d..69442b401 100644 --- a/packages/local-runtime/src/turns/file-changes.ts +++ b/packages/local-runtime/src/turns/file-changes.ts @@ -395,6 +395,28 @@ export async function applyLocalTurnDiffSnapshotMutation( } const sourceKey = action === 'revert' ? 'after' : 'before'; const targetKey = action === 'revert' ? 'before' : 'after'; + + /* Full precheck, before the first write. + * + * The path-safety and snapshot-completeness checks used to sit inside the + * write loop, so a refused entry left the run half-applied: earlier entries + * had already been written or `fs.rm`'d when a later one was judged unsafe. + * Resolving and validating every target up front makes the operation + * all-or-nothing — a refusal now touches nothing on disk. + * + * Safety first, ahead of the conflict check below: an operation that would + * write outside the workspace is refused without even reading the files. */ + const targets: string[] = []; + for (const undo of record.undo) { + const absolute = resolveMutationTargetPath(record.workspaceDir, undo.file); + if (!absolute) return { success: false, reason: 'unsafe_path' }; + const target = undo[targetKey]; + if (target.exists && target.content === undefined) { + return { success: false, reason: 'not_undoable' }; + } + targets.push(absolute); + } + for (const undo of record.undo) { const current = await snapshotCapturedFile(record.workspaceDir, undo.file); if (snapshotHash(current) !== snapshotHash(undo[sourceKey])) { @@ -402,21 +424,53 @@ export async function applyLocalTurnDiffSnapshotMutation( } } - for (const undo of record.undo) { - const absolute = safeCapturedPath(record.workspaceDir, undo.file); - if (!absolute) return { success: false, reason: 'unsafe_path' }; + for (const [index, undo] of record.undo.entries()) { + const absolute = targets[index]!; const target = undo[targetKey]; if (!target.exists) { await fs.rm(absolute, { force: true }); continue; } - if (target.content === undefined) return { success: false, reason: 'not_undoable' }; await fs.mkdir(path.dirname(absolute), { recursive: true }); - await fs.writeFile(absolute, target.content, 'utf8'); + await fs.writeFile(absolute, target.content!, 'utf8'); } return { success: true }; } +/** + * Resolve an undo entry's path to an absolute path inside the workspace, or + * refuse with `undefined`. + * + * `normalizeCapturePath` hands back the *absolute* path for anything that + * resolves outside the workspace — that return is how the capture layer + * records out-of-workspace writes — so a truthy result is NOT proof of + * containment and the mutation has to check it. It returns `undefined` for + * the in-workspace paths the capture layer filters (`.git/`, + * `node_modules/`, the root itself), which is also a refusal here. The + * containment re-check after `path.resolve` is deliberate: this is a + * security boundary, so it does not rely on `normalizeCapturePath`'s + * contract holding forever. + * + * Known boundary (lexical containment only): a contained path that is a + * symlink pointing outside the workspace still resolves outside. Closing + * that needs `realpath` on every target with careful handling of + * not-yet-existing files, which is a larger change than this refusal. + */ +function resolveMutationTargetPath( + workspaceDir: string, + file: string, +): string | undefined { + const normalized = normalizeCapturePath(workspaceDir, file); + if (!normalized) return undefined; + if (path.isAbsolute(normalized) || path.win32.isAbsolute(normalized)) { + return undefined; + } + const root = path.resolve(workspaceDir); + const absolute = path.resolve(root, normalized); + const prefix = root.endsWith(path.sep) ? root : `${root}${path.sep}`; + return absolute === root || absolute.startsWith(prefix) ? absolute : undefined; +} + function makeCaptureId(): string { return `tcap_${randomUUID().replaceAll('-', '')}`; } diff --git a/packages/local-runtime/test/unit/diff-mutation-precheck.test.ts b/packages/local-runtime/test/unit/diff-mutation-precheck.test.ts new file mode 100644 index 000000000..2743652f1 --- /dev/null +++ b/packages/local-runtime/test/unit/diff-mutation-precheck.test.ts @@ -0,0 +1,236 @@ +// D-1 — the revert/reapply snapshot mutation is all-or-nothing, and it refuses +// paths outside the workspace. +// +// Two real defects, both in `applyLocalTurnDiffSnapshotMutation`: +// +// 1. No containment check. `normalizeCapturePath` hands back the *absolute* +// path for anything resolving outside the workspace — that return is how +// the capture layer records out-of-workspace writes — and the mutation +// accepted it via `safeCapturedPath`, so a revert/reapply would write or +// `fs.rm` a file outside the workspace. +// 2. The checks sat inside the write loop. A refused entry left the run +// half-applied: earlier entries were already written or removed when a +// later one was judged unsafe or un-snapshottable. +// +// The fix resolves and validates every target in a full precheck before the +// first write. The fixtures below are built so the OLD code reaches the write +// loop (the conflict precheck passes, because the on-disk contents match the +// `after` hashes), which is what makes "the first file is untouched" a real +// assertion rather than a vacuous one: with the checks back inside the loop, +// the first file is written before the second is refused. + +import { createHash } from 'node:crypto'; +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import type { + LocalTurnDiffRecord, + LocalTurnDiffSnapshotEntry, + LocalTurnDiffUndoEntry, +} from '../../src/persistence/ports.js'; +import { applyLocalTurnDiffSnapshotMutation } from '../../src/turns/file-changes.js'; + +let root: string; +let workspaceDir: string; +let outsidePath: string; + +/** The hash `snapshotCapturedFile` computes for a small text file. */ +function contentHash(content: string): string { + return createHash('sha256').update(content).digest('hex'); +} + +function snapshot(file: string, content: string): LocalTurnDiffSnapshotEntry { + return { file, exists: true, hash: contentHash(content), content }; +} + +/** A snapshot for a file the capture layer could not read (oversized/binary): + * it exists, but there is no content to restore. */ +function unreadableSnapshot(file: string): LocalTurnDiffSnapshotEntry { + return { file, exists: true, oversized: true }; +} + +function undoEntry( + file: string, + before: LocalTurnDiffSnapshotEntry, + after: LocalTurnDiffSnapshotEntry, +): LocalTurnDiffUndoEntry { + return { file, before, after }; +} + +function record(undo: readonly LocalTurnDiffUndoEntry[]): LocalTurnDiffRecord { + return { + changeSetId: 'changes-1', + sessionId: 'test-session', + turnId: 'turn-1', + workspaceDir, + capturedAtMs: 1_700_000_000_000, + status: 'active', + fileChanges: [], + undo: [...undo], + undoable: true, + }; +} + +async function read(file: string): Promise { + return readFile(path.join(workspaceDir, file), 'utf8'); +} + +beforeEach(async () => { + root = await mkdtemp(path.join(tmpdir(), 'mcode-diff-precheck-')); + workspaceDir = path.join(root, 'workspace'); + outsidePath = path.join(root, 'outside.ts'); + await mkdir(workspaceDir, { recursive: true }); + await writeFile(path.join(workspaceDir, 'one.ts'), 'one-v1'); + await writeFile(path.join(workspaceDir, 'two.ts'), 'two-v1'); + await writeFile(outsidePath, 'outside-v1'); +}); + +afterEach(async () => { + await rm(root, { recursive: true, force: true, maxRetries: 4 }); +}); + +describe('applyLocalTurnDiffSnapshotMutation precheck', () => { + it('refuses a second entry resolving outside the workspace and touches nothing', async () => { + // The reported defect: the relative path resolves to `root/outside.ts`, + // one level above the workspace. The conflict precheck passes (the disk + // contents match the `after` hashes), so the OLD code reached the write + // loop, wrote `one.ts`, and then acted on the out-of-workspace path. + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry('one.ts', snapshot('one.ts', 'one-v0'), snapshot('one.ts', 'one-v1')), + undoEntry( + path.relative(workspaceDir, outsidePath), + snapshot(outsidePath, 'outside-v0'), + snapshot(outsidePath, 'outside-v1'), + ), + ]), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + // All-or-nothing: the first file is still its post-turn content, and the + // file outside the workspace was neither written nor removed. + expect(await read('one.ts')).toBe('one-v1'); + expect(await readFile(outsidePath, 'utf8')).toBe('outside-v1'); + }); + + it('refuses an absolute path outside the workspace even when it is spelled absolutely', async () => { + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry('one.ts', snapshot('one.ts', 'one-v0'), snapshot('one.ts', 'one-v1')), + undoEntry(outsidePath, snapshot(outsidePath, 'outside-v0'), snapshot(outsidePath, 'outside-v1')), + ]), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + expect(await read('one.ts')).toBe('one-v1'); + expect(await readFile(outsidePath, 'utf8')).toBe('outside-v1'); + }); + + it('refuses the in-workspace paths the capture layer filters, as before', async () => { + // `.git/` and `.harness-def/` normalize to `undefined` — the pre-existing + // refusal, kept: a revert must never write repository internals. Both + // snapshots are absent so the conflict precheck passes on the old code + // too, which is what makes this a behaviour-preserving assertion rather + // than one that only the new order can satisfy. + // + // Note on scope: the filter set on this baseline is exactly `.git` and + // `.harness-def` (plus `/plans|handoffs`). `node_modules` is NOT + // filtered here, so a `node_modules` entry is a contained path and the + // mutation treats it like any other — widening the capture filter is a + // separate decision this change does not make. + for (const file of ['.git/config', '.harness-def/state.json']) { + const result = await applyLocalTurnDiffSnapshotMutation( + record([undoEntry(file, { file, exists: false }, { file, exists: false })]), + 'revert', + ); + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + } + expect(await read('one.ts')).toBe('one-v1'); + }); + + it('refuses an un-snapshottable second entry before writing the first', async () => { + // The second half of the all-or-nothing defect: the second entry exists + // but has no content to restore. The conflict precheck passes (its `after` + // matches the disk), so the OLD code wrote `one.ts` and only then + // refused — leaving the half-applied state the user was never told about. + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry('one.ts', snapshot('one.ts', 'one-v0'), snapshot('one.ts', 'one-v1')), + undoEntry('two.ts', unreadableSnapshot('two.ts'), snapshot('two.ts', 'two-v1')), + ]), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'not_undoable' }); + expect(await read('one.ts')).toBe('one-v1'); + expect(await read('two.ts')).toBe('two-v1'); + }); + + it('still refuses a conflicted entry before writing anything', async () => { + // The pre-existing conflict precheck, preserved: the second entry's + // post-turn hash no longer matches the disk, so nothing is written. + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry('one.ts', snapshot('one.ts', 'one-v0'), snapshot('one.ts', 'one-v1')), + undoEntry('two.ts', snapshot('two.ts', 'two-v0'), snapshot('two.ts', 'two-drifted')), + ]), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'conflict' }); + expect(await read('one.ts')).toBe('one-v1'); + expect(await read('two.ts')).toBe('two-v1'); + }); + + it('applies a fully safe revert and reapply, including a deleted file', async () => { + // The happy path, so the precheck is not a refusal machine. The turn + // deleted `gone.ts` (its `after` is `exists: false`, and the disk agrees), + // so revert recreates it from `before`; reapply then sees the disk back at + // the `before` state and removes it again. + const entries = [ + undoEntry('one.ts', snapshot('one.ts', 'one-v0'), snapshot('one.ts', 'one-v1')), + undoEntry('gone.ts', snapshot('gone.ts', 'gone-v0'), { file: 'gone.ts', exists: false }), + ]; + + const reverted = await applyLocalTurnDiffSnapshotMutation(record(entries), 'revert'); + expect(reverted).toEqual({ success: true }); + expect(await read('one.ts')).toBe('one-v0'); + expect(await read('gone.ts')).toBe('gone-v0'); + + const reapplied = await applyLocalTurnDiffSnapshotMutation(record(entries), 'reapply'); + expect(reapplied).toEqual({ success: true }); + expect(await read('one.ts')).toBe('one-v1'); + await expect(readFile(path.join(workspaceDir, 'gone.ts'), 'utf8')).rejects.toThrow(); + }); + + it('reverts a file in a nested directory', async () => { + // The precheck resolves the absolute target; a nested relative path is + // contained and reverts like any other. + await mkdir(path.join(workspaceDir, 'nested', 'deep'), { recursive: true }); + await writeFile(path.join(workspaceDir, 'nested', 'deep', 'file.ts'), 'file-v1'); + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry( + 'nested/deep/file.ts', + snapshot('nested/deep/file.ts', 'file-v0'), + snapshot('nested/deep/file.ts', 'file-v1'), + ), + ]), + 'revert', + ); + expect(result).toEqual({ success: true }); + expect(await read('nested/deep/file.ts')).toBe('file-v0'); + }); + + it('refuses a record with nothing undoable, as before', async () => { + expect(await applyLocalTurnDiffSnapshotMutation(record([]), 'revert')).toEqual({ + success: false, + reason: 'not_undoable', + }); + }); +}); diff --git a/packages/webui/src/client/main.tsx b/packages/webui/src/client/main.tsx index 08178c953..4ce0d2bcc 100644 --- a/packages/webui/src/client/main.tsx +++ b/packages/webui/src/client/main.tsx @@ -13,13 +13,14 @@ import { NotFound } from "./components/shell/NotFound.js"; import { ArchonPage } from "./components/shell/ArchonPage.js"; import { WebuiErrorBoundary } from "./components/shell/WebuiErrorBoundary.js"; import { WebuiStartupFallback } from "./components/shell/StartupFallback.js"; -import { WebuiBrowserCapabilitiesProvider } from "./bindings/browser-capabilities.js"; +import { WebuiBrowserCapabilitiesProvider, useWebuiBrowserCapabilities } from "./bindings/browser-capabilities.js"; import { createWebuiBrowserDom } from "./infrastructure/browser-dom.js"; import { createWebuiBrowserStorage, getWebuiBrowserStorage, } from "./infrastructure/storage.js"; import { createWebuiApplication } from "./application/create-application.js"; +import { createWebuiComposerStore } from "./application/composer/composer-store.js"; import { createWebuiEventEffectsRegistry } from "./application/turn/event-effects-registry.js"; import { createWebuiPortSlices } from "./application/port-slices.js"; import { createWebuiOpenEventChannel } from "./bindings/event-channel-adapter.js"; @@ -69,6 +70,17 @@ function WebuiClientRoot({ () => createWebuiEventEffectsRegistry(), [], ); + // The same storage adapter the shell would have built for its own composer + // store. The application layer may not import infrastructure, so the + // composition root injects it — without this the adopted composer store has + // no storage at all and every draft dies with the page (regression from + // e40a63f: the root stopped handing the shell a store to build, and the + // application's own default is storage-less by design). + const { storage } = useWebuiBrowserCapabilities(); + const composerStore = React.useMemo( + () => createWebuiComposerStore({ storage }), + [storage], + ); const slices = React.useMemo( () => createWebuiPortSlices(transport), [transport], @@ -85,6 +97,7 @@ function WebuiClientRoot({ // own view reactions on top and registers them through // `setEventEffects`, because it renders after this instance exists. effects: effectsRegistry.asWebuiEventEffects(), + composer: composerStore, turns: { resumeSession: transport.resumeSession ?? (async () => {}), ...(transport.loadMessages @@ -96,7 +109,7 @@ function WebuiClientRoot({ ? { loadMessages: transport.loadMessages } : {}), }), - [effectsRegistry, transport], + [composerStore, effectsRegistry, transport], ); React.useEffect(() => () => application.dispose(), [application]); diff --git a/packages/webui/src/client/projection/transcript/review-state.ts b/packages/webui/src/client/projection/transcript/review-state.ts index e1c844311..ff52b48d6 100644 --- a/packages/webui/src/client/projection/transcript/review-state.ts +++ b/packages/webui/src/client/projection/transcript/review-state.ts @@ -347,21 +347,19 @@ function trimmedOrUndefined(value: string | undefined): string | undefined { * The reasons are machine tokens, and the authoritative one is * `applyLocalTurnDiffSnapshotMutation`'s `unsafe_path`. Read the runtime, not * the name: `normalizeCapturePath` *accepts* a path resolving outside the - * workspace, and returns `undefined` for in-workspace paths the capture layer - * filters (`.git/`, `node_modules/`, the root itself). The `safeCapturedPath` - * check also sits inside the write loop, so the operation is NOT all-or-nothing - * — earlier entries have already been written or `fs.rm`'d. The copy below - * therefore says the run was interrupted and earlier files may have changed, - * because that is what happened. Surfacing the raw token tells the user - * nothing, and the other codes read the same way — `conflict` in particular - * does not say that the files changed *after* the turn, which is the part that - * matters. + * workspace (returning it absolute), and returns `undefined` for in-workspace + * paths the capture layer filters (`.git/`, `node_modules/`, the root itself). + * The mutation now resolves and validates every target in a full precheck + * before the first write — the checks used to sit inside the write loop, which + * left a refused run half-applied — so the copy below can say plainly that + * nothing was touched. `conflict` in particular does not say that the files + * changed *after* the turn, which is the part that matters. * * A reason not in this table is passed through unchanged. That is deliberate: * `git apply` failures arrive as free-form stderr, and discarding them would * replace a real message with a guess. */ const WEBUI_DIFF_FAILURE_COPY: Readonly> = { - unsafe_path: "这轮改动里有文件的路径无法安全定位,操作已中断,之前处理过的文件可能已改动。", + unsafe_path: "这轮改动里有文件路径不在工作区内或无法安全定位,操作未执行,磁盘文件均未改动。", conflict: "这轮改动之后文件又被修改过,撤销前请先确认当前内容。", not_undoable: "这轮文件改动没有留下可撤销的快照。", "Turn diff not found": "找不到这轮文件改动。", diff --git a/packages/webui/test/unit/diff-mutation-error.test.tsx b/packages/webui/test/unit/diff-mutation-error.test.tsx index 0a9040192..eb91e2bfb 100644 --- a/packages/webui/test/unit/diff-mutation-error.test.tsx +++ b/packages/webui/test/unit/diff-mutation-error.test.tsx @@ -303,7 +303,7 @@ describe("resolveWebuiDiffMutation reads the transport result the way the wire m * `body.error` as the message. Without a translation the user reads the token. */ describe("runtime reason codes become something a person can act on", () => { it("translates every reason the mutation path can produce", () => { - expect(describeWebuiDiffFailure("unsafe_path")).toBe("这轮改动里有文件的路径无法安全定位,操作已中断,之前处理过的文件可能已改动。"); + expect(describeWebuiDiffFailure("unsafe_path")).toBe("这轮改动里有文件路径不在工作区内或无法安全定位,操作未执行,磁盘文件均未改动。"); expect(describeWebuiDiffFailure("conflict")).toBe("这轮改动之后文件又被修改过,撤销前请先确认当前内容。"); expect(describeWebuiDiffFailure("not_undoable")).toBe("这轮文件改动没有留下可撤销的快照。"); expect(describeWebuiDiffFailure("Turn diff not found")).toBe("找不到这轮文件改动。"); @@ -313,7 +313,7 @@ describe("runtime reason codes become something a person can act on", () => { it("no longer shows a raw token as the user-facing reason", () => { const action = resolveWebuiDiffMutation("revert", { success: false, error: "unsafe_path" }); - expect(action).toEqual({ type: "mutation-failed", error: "这轮改动里有文件的路径无法安全定位,操作已中断,之前处理过的文件可能已改动。" }); + expect(action).toEqual({ type: "mutation-failed", error: "这轮改动里有文件路径不在工作区内或无法安全定位,操作未执行,磁盘文件均未改动。" }); expect(action.type === "mutation-failed" && action.error).not.toBe("unsafe_path"); }); @@ -321,7 +321,7 @@ describe("runtime reason codes become something a person can act on", () => { * a real message; replacing them with a guess would be worse than useless. */ it("passes an unknown reason through unchanged", () => { expect(describeWebuiDiffFailure("error: patch failed: src/one.ts:3")).toBe("error: patch failed: src/one.ts:3"); - expect(describeWebuiDiffFailure(" unsafe_path ")).toBe("这轮改动里有文件的路径无法安全定位,操作已中断,之前处理过的文件可能已改动。"); + expect(describeWebuiDiffFailure(" unsafe_path ")).toBe("这轮改动里有文件路径不在工作区内或无法安全定位,操作未执行,磁盘文件均未改动。"); }); it("still reports no reason when there is none", () => { @@ -333,7 +333,7 @@ describe("runtime reason codes become something a person can act on", () => { it("translates a thrown unsafe_path the same way as a reported one", () => { expect(resolveWebuiDiffMutation("revert", undefined, new Error("unsafe_path"))).toEqual({ type: "mutation-failed", - error: "这轮改动里有文件的路径无法安全定位,操作已中断,之前处理过的文件可能已改动。", + error: "这轮改动里有文件路径不在工作区内或无法安全定位,操作未执行,磁盘文件均未改动。", }); }); }); diff --git a/release/public-source.json b/release/public-source.json index d1ca5503c..70fed96f7 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -2767,6 +2767,7 @@ "packages/local-runtime/test/unit/local-permission-facade.test.ts", "packages/local-runtime/test/unit/local-runtime-host-input-safety-v2.test.ts", "packages/local-runtime/test/unit/local-runtime-host-output-safety-concurrency.test.ts", + "packages/local-runtime/test/unit/diff-mutation-precheck.test.ts", "packages/local-runtime/test/unit/local-task-append.test.ts", "packages/local-runtime/test/unit/local-thread-goal-orchestrator.test.ts", "packages/local-runtime/test/unit/local-thread-goal-store.test.ts", diff --git a/test/vitest-suites.json b/test/vitest-suites.json index d591e5138..9f889359d 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -179,6 +179,7 @@ "packages/config/test/config-update-preservation.test.ts", "packages/agent-modules/context-manager/test/compaction-usage.test.ts", "packages/local-runtime/test/unit/session-diff-fallback.test.ts", + "packages/local-runtime/test/unit/diff-mutation-precheck.test.ts", "test/history-processing.test.ts", "test/sqlite-message-contention.test.ts", "packages/tui/test/unit/tui-feature-viewport-contract.test.ts",