From ff1c148c16978d1eb462aee735b009eb242c2e34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ram=C3=B3n=20Mart=C3=ADnez?= Date: Mon, 21 Sep 2026 22:44:08 +0200 Subject: [PATCH 1/5] fix(workspace): materialize refuses files whose disk copy changed since last write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit materializeToFs assumed it was the only writer: it wrote the workspace blob over every target unconditionally. Files routinely get modified by two hands — the workspace layer AND direct shell edits — so a bulk materialize could revert shell-edited files to a stale workspace snapshot (217 files in the 2026-08-11 production incident; a second single-file near-miss is in the issue thread, stopped only by an incidental branch-guard refusal). Mechanism (as restated in the issue thread): before writing, compare the disk content's hash against materializedHashes — the hash this layer last wrote. A mismatch means another writer changed the file: refuse that path loudly (listed in the tool result's skipped, with the remedy: sync to adopt the disk version, or force to overwrite), unless force: true. Symmetric with syncFromFs's existing conflict detection and needs no new state. Details: - identical disk bytes short-circuit: baseline re-pinned, file and its mtime left alone; - a refusal keeps lastMaterializedSeq back so the path stays in future incremental diffs — a loud refusal must not decay into a permanent silent one (re-attempting written files is free via the identical- bytes check); - materializeMount keeps overwrite semantics (force: true): it only runs on the framework's own _config mount after a deliberate undo/redo/branch switch, where restoring disk IS the intent; - the tool's existing force flag now also covers this guard, and its description says so. Fixes #109 Co-Authored-By: Claude Fable 5 --- .../materialize-freshness-guard.fixed.md | 1 + src/modules/workspace/index.ts | 14 +- src/modules/workspace/sync.ts | 63 +++++++- test/workspace-materialize-freshness.test.ts | 146 ++++++++++++++++++ 4 files changed, 216 insertions(+), 8 deletions(-) create mode 100644 changelog.d/materialize-freshness-guard.fixed.md create mode 100644 test/workspace-materialize-freshness.test.ts diff --git a/changelog.d/materialize-freshness-guard.fixed.md b/changelog.d/materialize-freshness-guard.fixed.md new file mode 100644 index 000000000..34d30bc96 --- /dev/null +++ b/changelog.d/materialize-freshness-guard.fixed.md @@ -0,0 +1 @@ +- **`materialize` no longer silently overwrites disk files changed by another writer** (#109). Before writing, each file's disk hash is compared against what the workspace layer last materialized; a mismatch means a shell/tool edit happened since, and the path is refused and listed in the result (`skipped`, with the remedy) instead of silently reverted — `force: true` remains the deliberate override, and refused paths stay pending rather than falling out of the incremental watermark. Files whose disk copy already matches the target bytes are left untouched (mtime preserved). The post-undo/redo `_config` restore keeps its overwrite semantics. diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 3b90f5d37..bf7dbaca8 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -1053,7 +1053,7 @@ export class WorkspaceModule implements Module { properties: { path: { type: 'string', description: 'Specific path to materialize (optional — defaults to all changed)' }, mount: { type: 'string', description: 'Specific mount (optional)' }, - force: { type: 'boolean', description: 'Materialize even if the current branch has diverged from the branch last written to disk (default false). Only needed for genuine divergence — descendant branches pass automatically.' }, + force: { type: 'boolean', description: 'Overwrite files whose disk copy changed since the last materialize (another writer), and materialize even if the current branch has diverged from the branch last written to disk (default false). Without it, divergent files are skipped and listed.' }, }, }, }, @@ -2307,12 +2307,17 @@ export class WorkspaceModule implements Module { // Suppress watcher for paths we're about to write const watcher = this.watchers.get(name); - const written = await materializeToFs(store, mount, paths); + const { written, skipped } = await materializeToFs(store, mount, paths, { force: input.force }); for (const p of written) { watcher?.suppress(p); allWritten.push({ mount: name, path: p }); } + // Freshness-guard refusals (#109) ride the same skipped list as branch + // blocks: divergence must be visible, not resolved silently either way. + for (const s of skipped) { + blocked.push({ mount: name, reason: `${s.path}: ${s.reason}` }); + } // Track which branch we materialized on. Re-pin on a clean empty // materialize too (previously-pinned mount, nothing pending): disk @@ -2349,7 +2354,10 @@ export class WorkspaceModule implements Module { mount.lastMaterializedSeq = 0; const watcher = this.watchers.get(mountName); - const written = await materializeToFs(store, mount); + // force: this path only runs after a deliberate undo/redo/branch switch + // on the framework's own _config mount — restoring disk to the branch + // state IS the operator intent, so the freshness guard yields. + const { written } = await materializeToFs(store, mount, undefined, { force: true }); for (const p of written) { watcher?.suppress(p); } diff --git a/src/modules/workspace/sync.ts b/src/modules/workspace/sync.ts index 324e72c50..520852376 100644 --- a/src/modules/workspace/sync.ts +++ b/src/modules/workspace/sync.ts @@ -193,24 +193,45 @@ export async function syncFromFs( return result; } +export interface MaterializeResult { + /** Paths actually written to disk */ + written: string[]; + /** Paths refused by the freshness guard, each with the reason — a silently + * overwritten shell edit is unrecoverable, so divergence must surface + * instead of being resolved toward the workspace copy (issue #109: a bulk + * materialize reverted 217 shell-edited files to a stale snapshot). */ + skipped: SkippedFile[]; +} + /** * Materialize Chronicle tree state to filesystem. * + * Freshness guard (#109): files get modified by two hands — this layer AND + * direct shell/tool edits. Before overwriting, the disk content's hash is + * compared against `materializedHashes` (the hash this layer last wrote): a + * mismatch means another writer changed the file since, and the path is + * refused loudly instead of silently reverted, unless `force`. Symmetric + * with syncFromFs's conflict detection, and with the same blind spot: no + * baseline (fresh process, never materialized) means no guard. + * * @param store Chronicle store * @param mount Mount state * @param paths Specific paths to materialize. If undefined, materializes all changed since last. - * @returns List of paths that were written + * @param opts force: overwrite even where the disk copy changed under us. + * @returns Paths written, and paths the freshness guard refused */ export async function materializeToFs( store: JsStore, mount: MountState, paths?: string[], -): Promise { + opts?: { force?: boolean }, +): Promise { if (mount.config.mode === 'read-only') { - return []; + return { written: [], skipped: [] }; } const written: string[] = []; + const skipped: SkippedFile[] = []; // Get changed files since last materialization const currentSeq = store.currentSequence(); @@ -262,6 +283,32 @@ export async function materializeToFs( const blob = store.getBlob(blobHash); if (!blob) continue; + let diskBuffer: Buffer | null = null; + try { + diskBuffer = await readFile(absolutePath); + } catch { + // No readable disk copy — creating a file overwrites nobody's work. + } + if (diskBuffer) { + const diskHash = hashContent(diskBuffer); + if (diskHash === blobHash) { + // Disk already holds exactly these bytes: re-pin the baseline and + // leave the file (and its mtime) alone. + mount.materializedHashes.set(relativePath, blobHash); + continue; + } + const baselineHash = mount.materializedHashes.get(relativePath); + if (!opts?.force && baselineHash !== undefined && diskHash !== baselineHash) { + skipped.push({ + path: relativePath, + reason: + 'stale copy: disk changed since last materialize (another writer) — ' + + 'sync first to adopt the disk version, or pass force to overwrite it', + }); + continue; + } + } + // Create parent directories await mkdir(dirname(absolutePath), { recursive: true }); @@ -273,9 +320,15 @@ export async function materializeToFs( mount.materializedHashes.set(relativePath, blobHash); } - mount.lastMaterializedSeq = currentSeq; + // A skipped path must stay pending: advancing the watermark past it would + // drop it from every future incremental diff, turning a loud refusal into + // a permanent silent one. Re-attempting already-written files is free — + // they short-circuit on the identical-bytes check above. + if (skipped.length === 0) { + mount.lastMaterializedSeq = currentSeq; + } - return written; + return { written, skipped }; } /** diff --git a/test/workspace-materialize-freshness.test.ts b/test/workspace-materialize-freshness.test.ts new file mode 100644 index 000000000..5600596f0 --- /dev/null +++ b/test/workspace-materialize-freshness.test.ts @@ -0,0 +1,146 @@ +/** + * Materialize freshness guard (#109): files get modified by two hands — the + * workspace layer AND direct shell/tool edits. `materialize` used to assume + * it was the only writer: a bulk materialize overwrote 217 shell-edited + * files with stale workspace copies (2026-08-11 production incident; second + * single-file occurrence in the issue thread, stopped only by an incidental + * branch-guard refusal). + * + * The guard compares the disk content's hash against `materializedHashes` + * (what the layer last wrote): a mismatch means another writer changed the + * file, and the path is refused LOUDLY — listed in the result, not resolved + * silently in either direction — unless `force: true`. + */ + +import test, { type TestContext } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { JsStore } from '@animalabs/chronicle'; +import { WorkspaceModule } from '../src/modules/workspace/index.js'; +import type { ModuleContext } from '../src/types/module.js'; + +function makeCtx(): ModuleContext { + return { + isRestart: false, + getState: () => null as T | null, + setState: () => {}, + pushEvent: () => {}, + } as unknown as ModuleContext; +} + +function setup(t: TestContext) { + const root = mkdtempSync(join(tmpdir(), 'af-ws-fresh-')); + const dir = join(root, 'work'); + mkdirSync(dir, { recursive: true }); + const store = JsStore.openOrCreate({ path: join(root, 'ws.chronicle') }); + t.after(() => { + store.close(); + rmSync(root, { recursive: true, force: true }); + }); + const module = new WorkspaceModule({ + mounts: [{ name: 'work', path: dir, mode: 'read-write' as const, watch: 'never' as const }], + }); + module.initStore(store); + return { store, dir, module }; +} + +type MaterializeData = { + materialized: Array<{ mount: string; path: string }>; + skipped?: Array<{ mount: string; reason: string }>; +}; + +async function call(module: WorkspaceModule, name: string, input: Record) { + const res = await module.handleToolCall({ id: 't', name, input }); + assert.equal(res.success, true, `${name} failed: ${res.error}`); + return res; +} + +/** Baseline: script.sh written through the workspace and materialized once. */ +async function seedBaseline(module: WorkspaceModule, dir: string): Promise { + await call(module, 'write', { path: 'work/script.sh', content: 'echo v1' }); + const res = await call(module, 'materialize', {}); + const data = res.data as MaterializeData; + assert.deepEqual(data.materialized.map((m) => m.path), ['script.sh']); + assert.equal(readFileSync(join(dir, 'script.sh'), 'utf8'), 'echo v1'); +} + +test('a shell-edited file is refused loudly, not silently reverted; force overrides', async (t) => { + const { dir, module } = setup(t); + await module.start(makeCtx()); + await seedBaseline(module, dir); + + // Another hand edits the file on disk; the agent edits the workspace copy. + writeFileSync(join(dir, 'script.sh'), 'echo edited-by-shell'); + await call(module, 'write', { path: 'work/script.sh', content: 'echo v2-stale-base' }); + + const res = await call(module, 'materialize', {}); + const data = res.data as MaterializeData; + assert.equal(readFileSync(join(dir, 'script.sh'), 'utf8'), 'echo edited-by-shell', + 'the shell edit must survive the materialize'); + assert.equal(data.materialized.length, 0, 'nothing silently written'); + const skip = (data.skipped ?? []).find((s) => s.reason.includes('script.sh')); + assert.ok(skip, `divergence must be listed in the result, got: ${JSON.stringify(data)}`); + assert.match(skip!.reason, /stale copy/, 'reason names the failure class'); + assert.match(skip!.reason, /force/, 'reason names the override'); + + // force is the deliberate override — workspace copy wins, visibly. + const forced = await call(module, 'materialize', { force: true }); + assert.deepEqual((forced.data as MaterializeData).materialized.map((m) => m.path), ['script.sh']); + assert.equal(readFileSync(join(dir, 'script.sh'), 'utf8'), 'echo v2-stale-base'); +}); + +test('a refused path stays pending: the next materialize reports it again instead of dropping it', async (t) => { + const { dir, module } = setup(t); + await module.start(makeCtx()); + await seedBaseline(module, dir); + + writeFileSync(join(dir, 'script.sh'), 'echo edited-by-shell'); + await call(module, 'write', { path: 'work/script.sh', content: 'echo v2' }); + + const first = await call(module, 'materialize', {}); + assert.ok(((first.data as MaterializeData).skipped ?? []).length > 0); + + // If the watermark advanced past the refusal, this second call would diff + // to nothing and the divergence would vanish from view — a loud refusal + // decaying into a permanent silent one. + const second = await call(module, 'materialize', {}); + const skip = ((second.data as MaterializeData).skipped ?? []).find((s) => s.reason.includes('script.sh')); + assert.ok(skip, 'the divergent path must stay visible on subsequent materializes'); + assert.equal(readFileSync(join(dir, 'script.sh'), 'utf8'), 'echo edited-by-shell'); +}); + +test('sync-then-materialize resolves the divergence in the disk direction', async (t) => { + const { dir, module } = setup(t); + await module.start(makeCtx()); + await seedBaseline(module, dir); + + writeFileSync(join(dir, 'script.sh'), 'echo edited-by-shell'); + await call(module, 'write', { path: 'work/script.sh', content: 'echo v2' }); + await call(module, 'materialize', {}); // refused, listed + + // The remedy the skip reason points at: adopt the disk version. + await call(module, 'sync', {}); + const res = await call(module, 'materialize', {}); + assert.equal(((res.data as MaterializeData).skipped ?? []).length, 0, + 'after sync the tree matches disk — nothing left to refuse'); + assert.equal(readFileSync(join(dir, 'script.sh'), 'utf8'), 'echo edited-by-shell'); +}); + +test('untouched disk copies and brand-new files materialize exactly as before', async (t) => { + const { dir, module } = setup(t); + await module.start(makeCtx()); + await seedBaseline(module, dir); + + // Agent-only edit (disk untouched since last materialize) + a new file. + await call(module, 'write', { path: 'work/script.sh', content: 'echo v2' }); + await call(module, 'write', { path: 'work/new.txt', content: 'fresh' }); + + const res = await call(module, 'materialize', {}); + const data = res.data as MaterializeData; + assert.deepEqual(data.materialized.map((m) => m.path).sort(), ['new.txt', 'script.sh']); + assert.equal(data.skipped ?? undefined, undefined, 'no spurious refusals'); + assert.equal(readFileSync(join(dir, 'script.sh'), 'utf8'), 'echo v2'); + assert.equal(readFileSync(join(dir, 'new.txt'), 'utf8'), 'fresh'); +}); From 04ec1d92a95c96573f22b9a87d56e354f1f55c80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ram=C3=B3n=20Mart=C3=ADnez?= Date: Mon, 21 Sep 2026 22:53:00 +0200 Subject: [PATCH 2/5] ci: retrigger (macOS node 20 chokidar timing flake in workspace-watcher, unrelated file) Co-Authored-By: Claude Fable 5 From bb16e5b52bb01319a5f16b3f5141a41adcab7a4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ram=C3=B3n=20Mart=C3=ADnez?= Date: Fri, 25 Sep 2026 22:56:54 +0200 Subject: [PATCH 3/5] fix(workspace): refuse materialize when the disk copy exists but cannot be read The freshness guard treated every readFile failure as "no disk copy", so a file that is writable but not readable (e.g. mode 0200) skipped the hash comparison and was overwritten silently. Only ENOENT now counts as absent; any other read error refuses the path with the filesystem error in the skip reason, unless force is passed. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../materialize-freshness-guard.fixed.md | 2 +- src/modules/workspace/sync.ts | 17 ++++++++- test/workspace-materialize-freshness.test.ts | 38 ++++++++++++++++++- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/changelog.d/materialize-freshness-guard.fixed.md b/changelog.d/materialize-freshness-guard.fixed.md index 34d30bc96..5a03e53e3 100644 --- a/changelog.d/materialize-freshness-guard.fixed.md +++ b/changelog.d/materialize-freshness-guard.fixed.md @@ -1 +1 @@ -- **`materialize` no longer silently overwrites disk files changed by another writer** (#109). Before writing, each file's disk hash is compared against what the workspace layer last materialized; a mismatch means a shell/tool edit happened since, and the path is refused and listed in the result (`skipped`, with the remedy) instead of silently reverted — `force: true` remains the deliberate override, and refused paths stay pending rather than falling out of the incremental watermark. Files whose disk copy already matches the target bytes are left untouched (mtime preserved). The post-undo/redo `_config` restore keeps its overwrite semantics. +- **`materialize` no longer silently overwrites disk files changed by another writer** (#109). Before writing, each file's disk hash is compared against what the workspace layer last materialized; a mismatch means a shell/tool edit happened since, and the path is refused and listed in the result (`skipped`, with the remedy) instead of silently reverted — A disk copy that exists but cannot be read (e.g. a write-only file) is refused the same way, with the filesystem error in the reason, rather than treated as absent. `force: true` remains the deliberate override, and refused paths stay pending rather than falling out of the incremental watermark. Files whose disk copy already matches the target bytes are left untouched (mtime preserved). The post-undo/redo `_config` restore keeps its overwrite semantics. diff --git a/src/modules/workspace/sync.ts b/src/modules/workspace/sync.ts index 520852376..93bc4c014 100644 --- a/src/modules/workspace/sync.ts +++ b/src/modules/workspace/sync.ts @@ -286,8 +286,21 @@ export async function materializeToFs( let diskBuffer: Buffer | null = null; try { diskBuffer = await readFile(absolutePath); - } catch { - // No readable disk copy — creating a file overwrites nobody's work. + } catch (err) { + // Only absence proves there is nothing to overwrite. Any other read + // failure (EACCES on a write-only file, EISDIR, EIO...) means freshness + // cannot be verified, and writeFile may still succeed — so refuse + // rather than treat "unreadable" as "not there". + const code = (err as NodeJS.ErrnoException).code; + if (code !== 'ENOENT' && !opts?.force) { + skipped.push({ + path: relativePath, + reason: + `cannot verify disk copy (${code ?? (err as Error).message}) — ` + + 'fix its permissions and materialize again, or pass force to overwrite it', + }); + continue; + } } if (diskBuffer) { const diskHash = hashContent(diskBuffer); diff --git a/test/workspace-materialize-freshness.test.ts b/test/workspace-materialize-freshness.test.ts index 5600596f0..e8410c564 100644 --- a/test/workspace-materialize-freshness.test.ts +++ b/test/workspace-materialize-freshness.test.ts @@ -14,7 +14,7 @@ import test, { type TestContext } from 'node:test'; import assert from 'node:assert/strict'; -import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { JsStore } from '@animalabs/chronicle'; @@ -128,6 +128,42 @@ test('sync-then-materialize resolves the divergence in the disk direction', asyn assert.equal(readFileSync(join(dir, 'script.sh'), 'utf8'), 'echo edited-by-shell'); }); +test('an unreadable but writable disk copy is refused, not treated as absent', async (t) => { + const { dir, module } = setup(t); + await module.start(makeCtx()); + await seedBaseline(module, dir); + + // Another hand edits the file and leaves it write-only: readFile fails + // with EACCES while writeFile would still succeed. + const file = join(dir, 'script.sh'); + writeFileSync(file, 'echo edited-by-shell'); + chmodSync(file, 0o200); + try { + readFileSync(file); + t.skip('platform/user can still read a mode-0200 file (Windows or root)'); + return; + } catch { /* unreadable, as intended */ } + + await call(module, 'write', { path: 'work/script.sh', content: 'echo v2' }); + const res = await call(module, 'materialize', {}); + const data = res.data as MaterializeData; + chmodSync(file, 0o600); + assert.equal(readFileSync(file, 'utf8'), 'echo edited-by-shell', + 'an edit we could not verify must survive the materialize'); + assert.equal(data.materialized.length, 0, 'nothing silently written'); + const skip = (data.skipped ?? []).find((s) => s.reason.includes('script.sh')); + assert.ok(skip, `the unverifiable path must be listed, got: ${JSON.stringify(data)}`); + assert.match(skip!.reason, /EACCES/, 'reason carries the filesystem error'); + assert.match(skip!.reason, /force/, 'reason names the override'); + + // force still overwrites deliberately. + chmodSync(file, 0o200); + const forced = await call(module, 'materialize', { force: true }); + assert.deepEqual((forced.data as MaterializeData).materialized.map((m) => m.path), ['script.sh']); + chmodSync(file, 0o600); + assert.equal(readFileSync(file, 'utf8'), 'echo v2'); +}); + test('untouched disk copies and brand-new files materialize exactly as before', async (t) => { const { dir, module } = setup(t); await module.start(makeCtx()); From 6093da5fb35f68fcb625fadd377bb29bb7d945bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ram=C3=B3n=20Mart=C3=ADnez?= Date: Fri, 25 Sep 2026 23:01:20 +0200 Subject: [PATCH 4/5] docs(changelog): punctuation in the freshness-guard fragment Co-Authored-By: Claude Opus 5.5 (1M context) --- changelog.d/materialize-freshness-guard.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/materialize-freshness-guard.fixed.md b/changelog.d/materialize-freshness-guard.fixed.md index 5a03e53e3..1ea052d0b 100644 --- a/changelog.d/materialize-freshness-guard.fixed.md +++ b/changelog.d/materialize-freshness-guard.fixed.md @@ -1 +1 @@ -- **`materialize` no longer silently overwrites disk files changed by another writer** (#109). Before writing, each file's disk hash is compared against what the workspace layer last materialized; a mismatch means a shell/tool edit happened since, and the path is refused and listed in the result (`skipped`, with the remedy) instead of silently reverted — A disk copy that exists but cannot be read (e.g. a write-only file) is refused the same way, with the filesystem error in the reason, rather than treated as absent. `force: true` remains the deliberate override, and refused paths stay pending rather than falling out of the incremental watermark. Files whose disk copy already matches the target bytes are left untouched (mtime preserved). The post-undo/redo `_config` restore keeps its overwrite semantics. +- **`materialize` no longer silently overwrites disk files changed by another writer** (#109). Before writing, each file's disk hash is compared against what the workspace layer last materialized; a mismatch means a shell/tool edit happened since, and the path is refused and listed in the result (`skipped`, with the remedy) instead of silently reverted. A disk copy that exists but cannot be read (e.g. a write-only file) is refused the same way, with the filesystem error in the reason, rather than treated as absent. `force: true` remains the deliberate override, and refused paths stay pending rather than falling out of the incremental watermark. Files whose disk copy already matches the target bytes are left untouched (mtime preserved). The post-undo/redo `_config` restore keeps its overwrite semantics. From e667499d8277f07a8ef570fa2890a77fb5c2d29e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ram=C3=B3n=20Mart=C3=ADnez?= Date: Tue, 6 Oct 2026 21:40:04 +0200 Subject: [PATCH 5/5] fix(workspace): close the freshness-guard gaps from review - sync re-pins the materialize baseline when it adopts a disk copy (and drops it when the file is gone), so the agent's next edit to that file is not refused forever. - Refused paths are tracked in mount.refusedPaths and retried by every later materialize; the diff watermark advances again, so it no longer understates what disk holds to the branch guard. - materializeToFs reports unchanged paths; a materialize that found every file already in place pins the branch like one that wrote. - materializedHashes and refusedPaths persist with the module state, so a restart keeps the guard. Co-Authored-By: Claude Opus 5.5 --- .../materialize-freshness-guard.fixed.md | 2 +- src/modules/workspace/index.ts | 24 +++- src/modules/workspace/sync.ts | 51 ++++++-- src/modules/workspace/types.ts | 6 + test/workspace-materialize-freshness.test.ts | 123 ++++++++++++++++-- 5 files changed, 179 insertions(+), 27 deletions(-) diff --git a/changelog.d/materialize-freshness-guard.fixed.md b/changelog.d/materialize-freshness-guard.fixed.md index 1ea052d0b..9dcc6620a 100644 --- a/changelog.d/materialize-freshness-guard.fixed.md +++ b/changelog.d/materialize-freshness-guard.fixed.md @@ -1 +1 @@ -- **`materialize` no longer silently overwrites disk files changed by another writer** (#109). Before writing, each file's disk hash is compared against what the workspace layer last materialized; a mismatch means a shell/tool edit happened since, and the path is refused and listed in the result (`skipped`, with the remedy) instead of silently reverted. A disk copy that exists but cannot be read (e.g. a write-only file) is refused the same way, with the filesystem error in the reason, rather than treated as absent. `force: true` remains the deliberate override, and refused paths stay pending rather than falling out of the incremental watermark. Files whose disk copy already matches the target bytes are left untouched (mtime preserved). The post-undo/redo `_config` restore keeps its overwrite semantics. +- **`materialize` no longer silently overwrites disk files changed by another writer** (#109). Before writing, each file's disk hash is compared against what the workspace layer last materialized; a mismatch means a shell/tool edit happened since, and the path is refused and listed in the result (`skipped`, with the remedy) instead of silently reverted. A disk copy that exists but cannot be read (e.g. a write-only file) is refused the same way, with the filesystem error in the reason, rather than treated as absent. `force: true` remains the deliberate override. Refused paths stay owed to every later materialize (and count in `status.pendingChanges`) without holding back the sequence the branch guard reads. `sync` re-pins the baseline when it adopts a disk copy, so the agent's next edit to that file is not refused. Baselines and refused paths persist across restarts, and a first materialize that finds every file already in place still pins the branch. Files whose disk copy already matches the target bytes are left untouched (mtime preserved). The post-undo/redo `_config` restore keeps its overwrite semantics. diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index bf7dbaca8..f5a6aeb4c 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -673,6 +673,7 @@ export class WorkspaceModule implements Module { initialSyncDone: false, lastMaterializedBranchId: null, materializedHashes: new Map(), + refusedPaths: new Set(), watcherReadyAt: null, watcherError: null, }; @@ -712,6 +713,12 @@ export class WorkspaceModule implements Module { if (mount) { mount.lastMaterializedSeq = meta.lastMaterializedSeq; mount.lastMaterializedBranchId = meta.lastMaterializedBranchId ?? null; + // Freshness-guard baselines: without them the first materialize after + // a restart can't tell a shell edit from our own last write (#109). + for (const [path, hash] of Object.entries(meta.materializedHashes ?? {})) { + if (!mount.materializedHashes.has(path)) mount.materializedHashes.set(path, hash); + } + for (const path of meta.refusedPaths ?? []) mount.refusedPaths.add(path); // watcherReadyAt intentionally not restored — each session must // observe its own watcher attach, otherwise a stale timestamp // would hide a new-session attach failure. @@ -900,6 +907,8 @@ export class WorkspaceModule implements Module { state.mounts[name] = { lastMaterializedSeq: mount.lastMaterializedSeq, lastMaterializedBranchId: mount.lastMaterializedBranchId ?? undefined, + materializedHashes: Object.fromEntries(mount.materializedHashes), + refusedPaths: [...mount.refusedPaths], watcherReadyAt: mount.watcherReadyAt, watcherError: mount.watcherError, }; @@ -2229,6 +2238,8 @@ export class WorkspaceModule implements Module { const changes = mount.lastMaterializedSeq > 0 ? store.treeDiff(mount.treeStateId, mount.lastMaterializedSeq, currentSeq) : []; + // Refused paths (#109) are pending too, past the watermark or not. + const pending = new Set([...changes.map((c) => c.path), ...mount.refusedPaths]); const currentBranch = store.currentBranch(); status[name] = { @@ -2238,7 +2249,7 @@ export class WorkspaceModule implements Module { fileCount: entries.length, lastMaterializedSeq: mount.lastMaterializedSeq, currentSeq, - pendingChanges: changes.length, + pendingChanges: pending.size, initialSyncDone: mount.initialSyncDone, currentBranch: currentBranch.name, lastMaterializedBranch: mount.lastMaterializedBranchId, @@ -2307,7 +2318,7 @@ export class WorkspaceModule implements Module { // Suppress watcher for paths we're about to write const watcher = this.watchers.get(name); - const { written, skipped } = await materializeToFs(store, mount, paths, { force: input.force }); + const { written, unchanged, skipped } = await materializeToFs(store, mount, paths, { force: input.force }); for (const p of written) { watcher?.suppress(p); @@ -2323,8 +2334,9 @@ export class WorkspaceModule implements Module { // materialize too (previously-pinned mount, nothing pending): disk // already reflects the current branch's tree, and leaving the old pin // would keep force required forever after a cross-branch materialize - // that happened to write nothing. - if (written.length > 0 || mount.lastMaterializedBranchId !== null) { + // that happened to write nothing. A first materialize that found every + // file already in place pins too: disk holds this branch's tree. + if (written.length > 0 || unchanged.length > 0 || mount.lastMaterializedBranchId !== null) { mount.lastMaterializedBranchId = store.currentBranch().id; } } @@ -2357,11 +2369,11 @@ export class WorkspaceModule implements Module { // force: this path only runs after a deliberate undo/redo/branch switch // on the framework's own _config mount — restoring disk to the branch // state IS the operator intent, so the freshness guard yields. - const { written } = await materializeToFs(store, mount, undefined, { force: true }); + const { written, unchanged } = await materializeToFs(store, mount, undefined, { force: true }); for (const p of written) { watcher?.suppress(p); } - if (written.length > 0) { + if (written.length > 0 || unchanged.length > 0) { mount.lastMaterializedBranchId = store.currentBranch().id; } return written; diff --git a/src/modules/workspace/sync.ts b/src/modules/workspace/sync.ts index 93bc4c014..b3d14d43d 100644 --- a/src/modules/workspace/sync.ts +++ b/src/modules/workspace/sync.ts @@ -119,6 +119,7 @@ export async function syncFromFs( store.treeRemove(mount.treeStateId, relativePath); result.synced.push({ path: relativePath, op: 'deleted' }); } + mount.materializedHashes.delete(relativePath); continue; } @@ -153,7 +154,8 @@ export async function syncFromFs( const existing = store.treeGet(mount.treeStateId, relativePath); if (existing && existing.blobHash === hash) { - // No change + // No change. Disk and tree agree, so this is the baseline now. + mount.materializedHashes.set(relativePath, hashContent(buffer)); continue; } @@ -181,6 +183,12 @@ export async function syncFromFs( } store.treeSet(mount.treeStateId, relativePath, entry); + // The tree adopted the disk copy, so disk and tree agree: re-pin the + // baseline. Left at the old hash, materialize would read the adopted + // edit as a fresh external change and refuse every later agent edit + // to this file, with no sync able to clear it. Raw disk bytes, which is + // what materializeToFs compares against. + mount.materializedHashes.set(relativePath, hashContent(buffer)); result.synced.push({ path: relativePath, op: existing ? 'modified' : 'created' }); } catch (error) { result.skipped.push({ @@ -196,6 +204,9 @@ export async function syncFromFs( export interface MaterializeResult { /** Paths actually written to disk */ written: string[]; + /** Paths whose disk copy already held exactly the tree's bytes: nothing + * written, but disk reflects the current tree for them all the same. */ + unchanged: string[]; /** Paths refused by the freshness guard, each with the reason — a silently * overwritten shell edit is unrecoverable, so divergence must surface * instead of being resolved toward the workspace copy (issue #109: a bulk @@ -212,7 +223,13 @@ export interface MaterializeResult { * mismatch means another writer changed the file since, and the path is * refused loudly instead of silently reverted, unless `force`. Symmetric * with syncFromFs's conflict detection, and with the same blind spot: no - * baseline (fresh process, never materialized) means no guard. + * baseline (never materialized, or synced) means no guard. Baselines and + * refused paths persist across restarts with the module state. + * + * Refused paths are kept in `mount.refusedPaths` and retried by every later + * materialize, so the diff watermark can still advance: holding it back + * would understate what disk holds to the branch guard, which reads the + * same sequence. * * @param store Chronicle store * @param mount Mount state @@ -227,10 +244,11 @@ export async function materializeToFs( opts?: { force?: boolean }, ): Promise { if (mount.config.mode === 'read-only') { - return { written: [], skipped: [] }; + return { written: [], unchanged: [], skipped: [] }; } const written: string[] = []; + const unchanged: string[] = []; const skipped: SkippedFile[] = []; // Get changed files since last materialization @@ -276,6 +294,18 @@ export async function materializeToFs( })); } + // Paths an earlier materialize refused are still owed. A path the tree no + // longer has is owed nothing. + if (!paths) { + const queued = new Set(filesToMaterialize.map((f) => f.path)); + for (const p of mount.refusedPaths) { + if (queued.has(p)) continue; + const entry = store.treeGet(mount.treeStateId, p); + if (entry) filesToMaterialize.push({ path: p, blobHash: entry.blobHash }); + else mount.refusedPaths.delete(p); + } + } + for (const { path: relativePath, blobHash } of filesToMaterialize) { const absolutePath = safePath(mount.config.path, relativePath); if (!absolutePath) continue; // Path traversal — skip silently @@ -299,6 +329,7 @@ export async function materializeToFs( `cannot verify disk copy (${code ?? (err as Error).message}) — ` + 'fix its permissions and materialize again, or pass force to overwrite it', }); + mount.refusedPaths.add(relativePath); continue; } } @@ -308,6 +339,8 @@ export async function materializeToFs( // Disk already holds exactly these bytes: re-pin the baseline and // leave the file (and its mtime) alone. mount.materializedHashes.set(relativePath, blobHash); + mount.refusedPaths.delete(relativePath); + unchanged.push(relativePath); continue; } const baselineHash = mount.materializedHashes.get(relativePath); @@ -318,6 +351,7 @@ export async function materializeToFs( 'stale copy: disk changed since last materialize (another writer) — ' + 'sync first to adopt the disk version, or pass force to overwrite it', }); + mount.refusedPaths.add(relativePath); continue; } } @@ -331,17 +365,12 @@ export async function materializeToFs( // Record blob hash at materialization time for conflict detection mount.materializedHashes.set(relativePath, blobHash); + mount.refusedPaths.delete(relativePath); } - // A skipped path must stay pending: advancing the watermark past it would - // drop it from every future incremental diff, turning a loud refusal into - // a permanent silent one. Re-attempting already-written files is free — - // they short-circuit on the identical-bytes check above. - if (skipped.length === 0) { - mount.lastMaterializedSeq = currentSeq; - } + mount.lastMaterializedSeq = currentSeq; - return { written, skipped }; + return { written, unchanged, skipped }; } /** diff --git a/src/modules/workspace/types.ts b/src/modules/workspace/types.ts index e0473a8ca..0ea110d3b 100644 --- a/src/modules/workspace/types.ts +++ b/src/modules/workspace/types.ts @@ -96,6 +96,9 @@ export interface MountState { lastMaterializedBranchId: string | null; /** Per-file hash at time of last materialization — baseline for conflict detection */ materializedHashes: Map; + /** Paths the materialize freshness guard refused (#109), owed to every + * later materialize until one writes them or the tree drops them. */ + refusedPaths: Set; /** * Wall-clock time chokidar emitted `ready` for this mount, or null if the * watcher hasn't finished its initial scan. null after session start = @@ -115,6 +118,9 @@ export interface WorkspaceModuleState { mounts: Record; + refusedPaths?: string[]; watcherReadyAt?: number | null; watcherError?: string | null; }>; diff --git a/test/workspace-materialize-freshness.test.ts b/test/workspace-materialize-freshness.test.ts index e8410c564..f0e7d748c 100644 --- a/test/workspace-materialize-freshness.test.ts +++ b/test/workspace-materialize-freshness.test.ts @@ -19,13 +19,17 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { JsStore } from '@animalabs/chronicle'; import { WorkspaceModule } from '../src/modules/workspace/index.js'; +import type { WorkspaceModuleState } from '../src/modules/workspace/types.js'; import type { ModuleContext } from '../src/types/module.js'; -function makeCtx(): ModuleContext { +function makeCtx(opts: { + saved?: WorkspaceModuleState; + onSave?: (state: WorkspaceModuleState) => void; +} = {}): ModuleContext { return { - isRestart: false, - getState: () => null as T | null, - setState: () => {}, + isRestart: opts.saved !== undefined, + getState: () => (opts.saved ?? null) as T | null, + setState: (state: WorkspaceModuleState) => opts.onSave?.(state), pushEvent: () => {}, } as unknown as ModuleContext; } @@ -39,11 +43,14 @@ function setup(t: TestContext) { store.close(); rmSync(root, { recursive: true, force: true }); }); - const module = new WorkspaceModule({ - mounts: [{ name: 'work', path: dir, mode: 'read-write' as const, watch: 'never' as const }], - }); - module.initStore(store); - return { store, dir, module }; + const makeModule = () => { + const m = new WorkspaceModule({ + mounts: [{ name: 'work', path: dir, mode: 'read-write' as const, watch: 'never' as const }], + }); + m.initStore(store); + return m; + }; + return { store, dir, module: makeModule(), makeModule }; } type MaterializeData = { @@ -180,3 +187,101 @@ test('untouched disk copies and brand-new files materialize exactly as before', assert.equal(readFileSync(join(dir, 'script.sh'), 'utf8'), 'echo v2'); assert.equal(readFileSync(join(dir, 'new.txt'), 'utf8'), 'fresh'); }); + +test('after sync adopts a shell edit, the next agent edit materializes (baseline re-pinned)', async (t) => { + const { dir, module } = setup(t); + await module.start(makeCtx()); + await seedBaseline(module, dir); + + writeFileSync(join(dir, 'script.sh'), 'echo edited-by-shell'); + await call(module, 'sync', {}); + await call(module, 'write', { path: 'work/script.sh', content: 'echo v3-on-top-of-shell' }); + + // A stale baseline (still 'echo v1') would read the adopted edit as a new + // external change and refuse this forever, sync or no sync. + const res = await call(module, 'materialize', {}); + const data = res.data as MaterializeData; + assert.equal(data.skipped ?? undefined, undefined, `no refusal expected, got: ${JSON.stringify(data)}`); + assert.deepEqual(data.materialized.map((m) => m.path), ['script.sh']); + assert.equal(readFileSync(join(dir, 'script.sh'), 'utf8'), 'echo v3-on-top-of-shell'); +}); + +test('a refusal does not hold back the sequence the branch guard reads', async (t) => { + const { store, dir, module } = setup(t); + await module.start(makeCtx()); + await seedBaseline(module, dir); + + // script.sh will be refused; other.txt is written in the same materialize. + writeFileSync(join(dir, 'script.sh'), 'echo edited-by-shell'); + await call(module, 'write', { path: 'work/script.sh', content: 'echo v2' }); + const forkAt = store.currentSequence(); + await call(module, 'write', { path: 'work/other.txt', content: 'from main' }); + const res = await call(module, 'materialize', {}); + assert.deepEqual((res.data as MaterializeData).materialized.map((m) => m.path), ['other.txt']); + + // A branch forked BEFORE other.txt was written never had it: disk holds + // main's write, so this is divergence and the guard must refuse. + store.createBranchAt('child', store.currentBranch().name, forkAt); + store.switchBranch('child'); + await call(module, 'write', { path: 'work/other.txt', content: 'from child' }); + const blocked = await module.handleToolCall({ id: 't', name: 'materialize', input: {} }); + assert.equal(blocked.success, false, 'divergent branch must be refused'); + assert.match(String(blocked.error), /diverged/); + assert.equal(readFileSync(join(dir, 'other.txt'), 'utf8'), 'from main'); +}); + +test('a first materialize that finds every file in place still pins the branch', async (t) => { + const { store, dir, module } = setup(t); + await module.start(makeCtx()); + + await call(module, 'write', { path: 'work/a.txt', content: 'a' }); + const forkAt = store.currentSequence(); + await call(module, 'write', { path: 'work/b.txt', content: 'b' }); + // Disk already holds the same bytes (e.g. restored from a backup). + writeFileSync(join(dir, 'a.txt'), 'a'); + writeFileSync(join(dir, 'b.txt'), 'b'); + + const first = await call(module, 'materialize', {}); + assert.equal((first.data as MaterializeData).materialized.length, 0, 'nothing needed writing'); + const status = (await call(module, 'status', {})).data as Record; + assert.equal(status.work.lastMaterializedBranch, store.currentBranch().id, 'the branch is pinned'); + + // So a branch that diverged before b.txt is refused rather than let through. + store.createBranchAt('child', store.currentBranch().name, forkAt); + store.switchBranch('child'); + await call(module, 'write', { path: 'work/a.txt', content: 'a from child' }); + const blocked = await module.handleToolCall({ id: 't', name: 'materialize', input: {} }); + assert.equal(blocked.success, false, 'divergent branch must be refused'); + assert.equal(readFileSync(join(dir, 'a.txt'), 'utf8'), 'a'); +}); + +test('baselines and refused paths survive a restart', async (t) => { + const { dir, module, makeModule } = setup(t); + let saved: WorkspaceModuleState | undefined; + await module.start(makeCtx({ onSave: (s) => { saved = s; } })); + await seedBaseline(module, dir); + + // One refusal before the restart, one divergence that happens across it. + await call(module, 'write', { path: 'work/notes.txt', content: 'n1' }); + await call(module, 'materialize', {}); + writeFileSync(join(dir, 'notes.txt'), 'notes edited by shell'); + await call(module, 'write', { path: 'work/notes.txt', content: 'n2' }); + const before = await call(module, 'materialize', {}); + assert.ok(((before.data as MaterializeData).skipped ?? []).some((s) => s.reason.includes('notes.txt'))); + await module.stop(); + assert.ok(saved, 'state persisted on stop'); + + writeFileSync(join(dir, 'script.sh'), 'echo edited-by-shell-while-down'); + const restarted = makeModule(); + await restarted.start(makeCtx({ saved })); + await call(restarted, 'write', { path: 'work/script.sh', content: 'echo v2' }); + + const res = await call(restarted, 'materialize', {}); + const data = res.data as MaterializeData; + assert.equal(data.materialized.length, 0, `nothing silently written, got: ${JSON.stringify(data)}`); + const reasons = (data.skipped ?? []).map((s) => s.reason); + assert.ok(reasons.some((r) => r.includes('script.sh') && /stale copy/.test(r)), 'the guard holds after a restart'); + assert.ok(reasons.some((r) => r.includes('notes.txt')), 'the earlier refusal is still owed'); + assert.equal(readFileSync(join(dir, 'script.sh'), 'utf8'), 'echo edited-by-shell-while-down'); + assert.equal(readFileSync(join(dir, 'notes.txt'), 'utf8'), 'notes edited by shell'); +});