From 10bf0655798e75c7cfbfb5724814c6d2b11bcd7b Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 11:04:59 -0400 Subject: [PATCH 01/13] fix(workspace): the file tools write what they're given or refuse, say when a change is stored but not on disk, and number the lines a file has The design for nissa (Ada-1017, room-203 #129173; Oscar-1808's rulings #129183), its fix PR: parts 1 and 5, the phantom line, the ranges, and the edit half of the disk check. Evidence: Phoebe-2419's account (#128784). Part 1, exactly what was given or a refusal: - edit replaces through a replacer function, so `$&`, `$$`, `` $` `` and `$'` in newString go in as given (E1). - edit refuses a file that isn't valid UTF-8 (isUtf8), which decoding and writing back changed outside the edit (E7); write still replaces it whole. - write refuses a lone surrogate in content, edit in oldString or newString (/\p{Cs}/u), and edit refuses an empty oldString, each before reading (W3, E2). - handleToolCall checks every call's input against its tool's own schema (inputProblem): required fields, strings, booleans, whole numbers with the minimums in INPUT_MINIMUMS, since membrane's ToolParameter has no `minimum`. read's offset/limit and grep's context fields are typed integer. Booleans are checked too, beyond the design's text and numbers: replaceAll "false" counted as true, and materialize's force would too. - A write, edit or delete stored but unfinished (push refused, or a throw after the tree entry) answers storedButUnfinished: an error that leads with "stored, don't send it again", data { inChronicle: true, path }. commitToolChange now calls onCommitted for a removal too. Part 2: - linesOf: no empty line after a final line end, an empty file has none; grep uses it too (a `^$` matched past the last line). - An offset past the end (or an empty file) answers empty content with totalLines, a note, and no range. - edit settles its path with a pass first (the rule listings use): a disk change the store hasn't taken comes in, the edit applies to it and says so; a miss says the text was disk's. Where the path is in conflict, the edit applies to the draft (Ada's call (a), #129263): on a pushing mount the refused push's answer names the conflict, elsewhere a note does. write doesn't settle. Part 5: grep's invalid pattern gives the parser's message; the catch-all drops "Error: "; reconcile's inTheWay names a directory at the path or a file where the path needs a directory, and the way out, for the unverified refusal (when the store has the file) and the write effect's failure. Tests: test/workspace-file-tools.test.ts (17), and the delete refusal's text in workspace-three-way.test.ts. Focused run of workspace-*.test.js on the VPS before this commit: 287 tests, 286 pass, the one failure the old delete text this commit updates. Co-Authored-By: Claude Opus 5.5 --- changelog.d/fix-workspace-file-tools.fixed.md | 9 + src/modules/workspace/index.ts | 346 +++++++++++++----- src/modules/workspace/reconcile.ts | 27 +- test/workspace-file-tools.test.ts | 284 ++++++++++++++ test/workspace-three-way.test.ts | 2 +- 5 files changed, 570 insertions(+), 98 deletions(-) create mode 100644 changelog.d/fix-workspace-file-tools.fixed.md create mode 100644 test/workspace-file-tools.test.ts diff --git a/changelog.d/fix-workspace-file-tools.fixed.md b/changelog.d/fix-workspace-file-tools.fixed.md new file mode 100644 index 000000000..64380407c --- /dev/null +++ b/changelog.d/fix-workspace-file-tools.fixed.md @@ -0,0 +1,9 @@ +- The workspace's `edit` writes its replacement as given. It read `$&`, `$$`, `` $` `` and `$'` in `newString` as replacement patterns. It now refuses, changing nothing, three edits it used to report as successes: + - an edit of a file that isn't valid UTF-8, where decoding and writing it back turned every invalid byte sequence in the file into U+FFFD; + - an empty `oldString`, which matched between every character; + - a lone UTF-16 surrogate in `oldString` or `newString`, which `write` now refuses in `content` too. +- Every workspace tool checks its input against its own schema before it does anything: required fields, strings, booleans, and whole numbers, with `read`'s `offset` and `limit` at least 1 and `grep`'s context lines at least 0. An edit missing `newString` wrote the word "undefined", and `replaceAll: "false"` counted as true. `read`'s `offset` and `limit`, and `grep`'s `contextBefore` and `contextAfter`, are typed `integer` in their schemas. +- A `write`, `edit` or `delete` that the workspace stored but couldn't finish still answers as an error. That covers one whose push to disk was refused, and one that failed after its change was stored. The answer now leads with the fact that the change is stored and mustn't be sent again, and its `data` carries `inChronicle: true`, as `writeBinary`'s does, with the `path`. Before, a refused push's answer began "Wrote to Chronicle", "Edited Chronicle" or "Removed from Chronicle", and a failure after the change was stored gave only the error. Either way, an edit sent again could apply twice. +- `read` counts the lines a file has: a final line end no longer starts an empty extra line, so "a\nb\n" is two lines, and an empty file has none. `grep` numbers lines the same way. An `offset` past the last line answers empty `content` with the file's `totalLines` and a note, and no `fromLine` or `toLine`, where before it claimed a range that ended before it began. +- An `edit` first takes in a change on disk the workspace hasn't seen, as a listing does, applies to that text, and says so in its receipt. Before, it edited the older stored copy, and on a mount that pushes, its push then failed as stale. Where the path's stored copy is a draft and disk changed too, the edit applies to the draft as before, and its answer names the conflict and the two ways out. `write` doesn't do this, since it replaces the whole file. +- `grep`'s refusal of an invalid pattern gives the parser's reason, and the workspace's refusals no longer begin with "Error: ". A push that disk's shape refuses says what's in the way and the way out: a directory at the path, or a file where the path needs a directory. Before, a directory's refusal offered `force`, which can't write there either, and a file in the way named no way out. diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 479000375..292216fdf 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -5,6 +5,7 @@ * auto-sync between real filesystem and Chronicle, and manual materialization. */ +import { isUtf8 } from 'node:buffer'; import { constants as fsConstants } from 'node:fs'; import type { Stats } from 'node:fs'; import { open, readFile, stat, lstat, realpath } from 'node:fs/promises'; @@ -1093,8 +1094,8 @@ export class WorkspaceModule implements Module { type: 'object' as const, properties: { path: { type: 'string', description: 'File path (mount-prefixed, e.g., "project/src/main.ts")' }, - offset: { type: 'number', description: 'Starting line number (1-indexed)' }, - limit: { type: 'number', description: 'Maximum number of lines to return (default 2000)' }, + offset: { type: 'integer', description: 'Starting line number (1-indexed)' }, + limit: { type: 'integer', description: 'Maximum number of lines to return (default 2000)' }, offsetChars: { type: 'integer', description: 'Character paging: zero-based UTF-16 offset (default 0); use nextOffsetChars to continue' }, limitChars: { type: 'integer', description: 'Character paging: code units to return (default 2000), plus at most one to keep a surrogate pair intact. Use the limit suggested by a spill notice.' }, }, @@ -1194,8 +1195,8 @@ export class WorkspaceModule implements Module { pattern: { type: 'string', description: 'Regular expression pattern' }, path: { type: 'string', description: 'File or directory to search (mount-prefixed, optional)' }, glob: { type: 'string', description: 'Glob pattern to filter files' }, - contextBefore: { type: 'number', description: 'Context lines before match' }, - contextAfter: { type: 'number', description: 'Context lines after match' }, + contextBefore: { type: 'integer', description: 'Context lines before match' }, + contextAfter: { type: 'integer', description: 'Context lines after match' }, }, required: ['pattern'], }, @@ -1308,6 +1309,13 @@ export class WorkspaceModule implements Module { async handleToolCall(call: ToolCall): Promise { if (!this.running()) return { success: false, isError: true, error: NOT_RUNNING }; try { + // Every handler reads its input as its schema describes it; a call + // that sends something else is refused here, before anything is read. + const schema = this.getTools().find((tool) => tool.name === call.name)?.inputSchema; + const problem = schema ? inputProblem(schema, call.input, INPUT_MINIMUMS[call.name]) : null; + if (problem !== null) { + return { success: false, isError: true, error: `The ${call.name} call did nothing: ${problem}.` }; + } const input = call.input as Record; const generated = typeof input?.path === 'string' ? this.findGeneratedTextFile(input.path) : undefined; if (generated) { @@ -1332,7 +1340,7 @@ export class WorkspaceModule implements Module { return { success: false, error: `Unknown tool: ${call.name}`, isError: true }; } } catch (err) { - return { success: false, error: String(err), isError: true }; + return { success: false, error: err instanceof Error ? err.message : String(err), isError: true }; } } @@ -1977,6 +1985,7 @@ export class WorkspaceModule implements Module { }); } else { store.treeRemove(mount.treeStateId, relativePath); + onCommitted?.(); intents.update(relativePath, (cur) => { const next = { ...cur, tombstone: { hash: change.previousHash } }; delete next.origin; @@ -2060,15 +2069,10 @@ export class WorkspaceModule implements Module { const characterPaging = input.offsetChars !== undefined || input.limitChars !== undefined; const offsetChars = input.offsetChars ?? 0; const limitChars = input.limitChars ?? 2000; - if (characterPaging) { - if (input.offset !== undefined || input.limit !== undefined) { - return { success: false, isError: true, error: 'Use either offset/limit (lines) or offsetChars/limitChars (characters), not both.' }; - } - if (!Number.isSafeInteger(offsetChars) || offsetChars < 0 - || !Number.isSafeInteger(limitChars) || limitChars < 1 - || input.offsetChars === null || input.limitChars === null) { - return { success: false, isError: true, error: 'offsetChars must be a non-negative safe integer and limitChars a positive safe integer.' }; - } + // Each number is a whole number of at least its schema's minimum by now + // (inputProblem). + if (characterPaging && (input.offset !== undefined || input.limit !== undefined)) { + return { success: false, isError: true, error: 'Use either offset/limit (lines) or offsetChars/limitChars (characters), not both.' }; } let content = generatedContent; if (content === undefined) { @@ -2107,7 +2111,7 @@ export class WorkspaceModule implements Module { }, }; } - const lines = content.split('\n'); + const lines = linesOf(content); // Apply offset/limit. An unlimited read of a large file would inject the // whole thing into the turn (a 2MB file ≈ 600k tokens → the request blows @@ -2115,6 +2119,21 @@ export class WorkspaceModule implements Module { // default cap; the result reports total/from/to so the agent can page. const DEFAULT_READ_LINE_LIMIT = 2000; const startLine = (input.offset ?? 1) - 1; // Convert to 0-indexed + if (startLine >= lines.length) { + // Nothing to show: an empty file, or an offset past the last line. No + // range is claimed, and the line count says where the file ends. + return { + success: true, + data: { + path: input.path, + totalLines: lines.length, + ...(lines.length > 0 + ? { note: `Line ${startLine + 1} is past the end: the file has ${lines.length} line${lines.length === 1 ? '' : 's'}.` } + : {}), + content: '', + }, + }; + } const effectiveLimit = input.limit ?? DEFAULT_READ_LINE_LIMIT; const endLine = startLine + effectiveLimit; const slice = lines.slice(startLine, endLine); @@ -2213,6 +2232,9 @@ export class WorkspaceModule implements Module { } private async handleWrite(input: WriteInput): Promise { + if (LONE_SURROGATE.test(input.content)) { + return { success: false, isError: true, error: 'The write did nothing: "content" holds a lone UTF-16 surrogate, which UTF-8 can\'t store.' }; + } const { mount, relativePath } = this.parsePath(input.path); if (mount.config.mode === 'read-only') { return { success: false, error: `Mount "${mount.config.name}" is read-only`, isError: true }; @@ -2225,18 +2247,20 @@ export class WorkspaceModule implements Module { } const buffer = Buffer.from(input.content, 'utf-8'); - const materializeError = await this.withMount(mount, async () => { - await this.settleBeforeMutation(mount, relativePath); - const blobHash = store.storeBlob(buffer, 'text/plain'); - return this.commitToolChange(mount, relativePath, { kind: 'set', blobHash, size: buffer.byteLength }); - }); - if (materializeError) { - return { - success: false, - error: `Wrote to Chronicle but failed to materialize "${input.path}" to disk: ${materializeError}. Downstream agents will not see this change until the next materialize call.`, - isError: true, - }; + let committed = false; + let refused: string | null; + try { + refused = await this.withMount(mount, async () => { + await this.settleBeforeMutation(mount, relativePath); + const blobHash = store.storeBlob(buffer, 'text/plain'); + return this.commitToolChange(mount, relativePath, { kind: 'set', blobHash, size: buffer.byteLength }, + () => { committed = true; }); + }); + } catch (error) { + if (!committed) throw error; + return storedButUnfinished('write', input.path, { threw: error instanceof Error ? error.message : String(error) }); } + if (refused !== null) return storedButUnfinished('write', input.path, { refused }); return { success: true, @@ -2249,71 +2273,110 @@ export class WorkspaceModule implements Module { } private async handleEdit(input: EditInput): Promise { + // Refused before anything is read: an empty oldString matches between + // every character, and UTF-8 can't store a lone surrogate. + if (input.oldString === '') { + return { success: false, isError: true, error: 'The edit did nothing: "oldString" is empty, and an empty string matches between every character. Give the text to replace.' }; + } + for (const field of ['oldString', 'newString'] as const) { + if (LONE_SURROGATE.test(input[field])) { + return { success: false, isError: true, error: `The edit did nothing: "${field}" holds a lone UTF-16 surrogate, which UTF-8 can't store.` }; + } + } const { mount, relativePath } = this.parsePath(input.path); if (mount.config.mode === 'read-only') { return { success: false, error: `Mount "${mount.config.name}" is read-only`, isError: true }; } const store = this.getStore(); - return this.withMount(mount, async (): Promise => { - const report = await this.ensureSynced(mount, relativePath); - await this.settleBeforeMutation(mount, relativePath); // before the edit reads the entry - - const entry = store.treeGet(mount.treeStateId, relativePath); - if (!entry) { - return { success: false, error: this.notInWorkspace(input.path, report), isError: true }; - } - - const blob = store.getBlob(entry.blobHash); - if (!blob) { - return { success: false, error: `Blob not found for: ${input.path}`, isError: true }; - } + let committed = false; + try { + return await this.withMount(mount, async (): Promise => { + // The edit applies to the path as disk holds it now: a change on disk + // the store hasn't taken is taken in first, by the rule a listing + // uses. A write doesn't do this, since it replaces the whole file. + const pass = await this.passUnlocked(mount, { kind: 'paths', paths: [relativePath] }); + const report = pass.reports.get(relativePath); + const tookDisk = pass.ops.some((op) => op.path === relativePath && op.op === 'modified'); + const fromDisk = tookDisk + ? ' Disk\'s copy had changed since the workspace last agreed with it, so it was taken in first, and the edit looked in that text.' + : ''; + await this.settleBeforeMutation(mount, relativePath); // before the edit reads the entry - let content = blob.toString('utf-8'); + const entry = store.treeGet(mount.treeStateId, relativePath); + if (!entry) { + return { success: false, error: this.notInWorkspace(input.path, report), isError: true }; + } - // Validate uniqueness - if (!input.replaceAll) { - const count = content.split(input.oldString).length - 1; - if (count === 0) { - return { success: false, error: `String not found in ${input.path}`, isError: true }; + const blob = store.getBlob(entry.blobHash); + if (!blob) { + return { success: false, error: `Blob not found for: ${input.path}`, isError: true }; } - if (count > 1) { + // Decoding would turn each invalid sequence into U+FFFD, and writing + // the text back would change bytes the edit never named. + if (!isUtf8(blob)) { return { success: false, - error: `String found ${count} times in ${input.path}. Use replaceAll: true or provide more context.`, isError: true, + error: `The edit did nothing: ${input.path} isn't valid UTF-8, so editing it would change bytes outside the text it replaces. A write replaces the whole file, as UTF-8, if that's what you want.${fromDisk}`, }; } - } - content = input.replaceAll - ? content.replaceAll(input.oldString, input.newString) - : content.replace(input.oldString, input.newString); + let content = blob.toString('utf-8'); - const newBuffer = Buffer.from(content, 'utf-8'); - const newBlobHash = store.storeBlob(newBuffer, 'text/plain'); - const materializeError = await this.commitToolChange(mount, relativePath, { - kind: 'set', - blobHash: newBlobHash, - size: newBuffer.byteLength, - mode: entry.mode, - }); - if (materializeError) { - return { - success: false, - error: `Edited Chronicle but failed to materialize "${input.path}" to disk: ${materializeError}. Downstream agents will not see this change until the next materialize call.`, - isError: true, - }; - } + // Validate uniqueness + if (!input.replaceAll) { + const count = content.split(input.oldString).length - 1; + if (count === 0) { + return { success: false, error: `String not found in ${input.path}.${fromDisk}`, isError: true }; + } + if (count > 1) { + return { + success: false, + error: `String found ${count} times in ${input.path}. Use replaceAll: true or provide more context.${fromDisk}`, + isError: true, + }; + } + } - return { - success: true, - data: { - path: input.path, + // A replacer function, so that the new text goes in as given: in a + // replacement string, $&, $$, $' and $` are patterns. + content = input.replaceAll + ? content.replaceAll(input.oldString, () => input.newString) + : content.replace(input.oldString, () => input.newString); + + const newBuffer = Buffer.from(content, 'utf-8'); + const newBlobHash = store.storeBlob(newBuffer, 'text/plain'); + const refused = await this.commitToolChange(mount, relativePath, { + kind: 'set', + blobHash: newBlobHash, size: newBuffer.byteLength, - }, - }; - }); + mode: entry.mode, + }, () => { committed = true; }); + if (refused !== null) return storedButUnfinished('edit', input.path, { refused }); + + // A mount that doesn't push keeps a conflict until materialize or + // sync settles it, so the edit says so. + const conflict = report?.state === 'conflict' ? report.conflict?.kind ?? 'both-changed' : null; + const notes = [ + ...(tookDisk ? ['Disk\'s copy had changed since the workspace last agreed with it, so it was taken in first, and the edit applied to that text.'] : []), + ...(conflict !== null + ? [`The path is in conflict with disk (${conflict}): the edit is in the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's.`] + : []), + ]; + return { + success: true, + data: { + path: input.path, + size: newBuffer.byteLength, + ...(notes.length > 0 ? { note: notes.join(' ') } : {}), + }, + }; + }); + } catch (error) { + if (!committed) throw error; + return storedButUnfinished('edit', input.path, { threw: error instanceof Error ? error.message : String(error) }); + } } private async handleDelete(input: DeleteInput): Promise { @@ -2323,28 +2386,28 @@ export class WorkspaceModule implements Module { } const store = this.getStore(); - return this.withMount(mount, async (): Promise => { - const report = await this.ensureSynced(mount, relativePath); - await this.settleBeforeMutation(mount, relativePath); - const entry = store.treeGet(mount.treeStateId, relativePath); - if (!entry) { - return { success: false, error: this.notInWorkspace(input.path, report), isError: true }; - } + let committed = false; + try { + return await this.withMount(mount, async (): Promise => { + const report = await this.ensureSynced(mount, relativePath); + await this.settleBeforeMutation(mount, relativePath); + const entry = store.treeGet(mount.treeStateId, relativePath); + if (!entry) { + return { success: false, error: this.notInWorkspace(input.path, report), isError: true }; + } - const materializeError = await this.commitToolChange(mount, relativePath, { kind: 'remove', previousHash: entry.blobHash }); - if (materializeError) { - return { - success: false, - error: `Removed from Chronicle but did not unlink "${input.path}" from disk: ${materializeError}. ` + - 'The disk copy stays (ls shows its state) until materialize with applyDeletions removes it.', - isError: true, - }; - } + const refused = await this.commitToolChange(mount, relativePath, { kind: 'remove', previousHash: entry.blobHash }, + () => { committed = true; }); + if (refused !== null) return storedButUnfinished('delete', input.path, { refused }); - // Its whole receipt is what it changed: kept in the stub if the guard - // withholds the result (agent-framework #277). - return { success: true, data: { path: input.path, deleted: true }, keepWhenWithheld: ['path', 'deleted'] }; - }); + // Its whole receipt is what it changed: kept in the stub if the guard + // withholds the result (agent-framework #277). + return { success: true, data: { path: input.path, deleted: true }, keepWhenWithheld: ['path', 'deleted'] }; + }); + } catch (error) { + if (!committed) throw error; + return storedButUnfinished('delete', input.path, { threw: error instanceof Error ? error.message : String(error) }); + } } private async handleLs(input: LsInput): Promise { @@ -2447,7 +2510,8 @@ export class WorkspaceModule implements Module { try { regex = new RegExp(input.pattern); } catch (e) { - return { success: false, error: `Invalid regex: ${input.pattern}`, isError: true }; + // The parser's reason says where the pattern goes wrong. + return { success: false, error: `The grep did nothing: ${e instanceof Error ? e.message : String(e)}.`, isError: true }; } const fileGlob = input.glob ? globToRegex(input.glob) : null; @@ -2464,7 +2528,7 @@ export class WorkspaceModule implements Module { const incomplete: Array<{ path: string; reason: string }> = []; const search = (content: string): GrepMatch[] => { - const lines = content.split('\n'); + const lines = linesOf(content); const found: GrepMatch[] = []; for (let i = 0; i < lines.length; i++) { if (regex.test(lines[i])) { @@ -3181,6 +3245,98 @@ function stateView(report: PathReport): Record { // Utilities // ========================================================================== +/** A lone UTF-16 surrogate, which UTF-8 can't store: in `u` mode, a pair is one code point. */ +const LONE_SURROGATE = /\p{Cs}/u; + +/** + * The answer to a write, edit or delete that the workspace stored but + * couldn't finish: its push to disk was refused, or what follows the tree + * entry threw. It stays an error, since part of the call failed, but it leads + * with what the caller must know before acting on it: the change is stored, + * and sending it again could apply it twice. `inChronicle` lets a script tell + * it from a call that changed nothing, as writeBinary's answer does. + */ +function storedButUnfinished( + tool: 'write' | 'edit' | 'delete', + path: string, + outcome: { refused: string } | { threw: string }, +): ToolResult { + const change = tool === 'delete' ? 'deletion' : tool; + const stored = `The ${change} of "${path}" is stored in the workspace, so don't send it again` + + (tool === 'edit' ? ': a second edit can apply the change twice' : ''); + const rest = 'refused' in outcome + ? `. Its push to disk was refused: ${outcome.refused}. ` + (tool === 'delete' + ? 'Until materialize with applyDeletions removes it, disk keeps its copy (ls shows its state).' + : 'Until materialize puts it there, disk doesn\'t have it.') + : `. The rest of the call failed: ${outcome.threw}.`; + return { success: false, isError: true, error: stored + rest, data: { inChronicle: true, path } }; +} + +/** + * A text's lines, as read and grep number them: a final line end ends the + * last line rather than starting an empty one, so "a\nb\n" is two lines, + * and an empty text has none. + */ +function linesOf(content: string): string[] { + const lines = content.split('\n'); + if (lines.at(-1) === '') lines.pop(); + return lines; +} + +/** + * The least each tool's whole-number fields may be, beside their schemas, + * whose type (membrane's ToolParameter) has no `minimum`. + */ +const INPUT_MINIMUMS: Record> = { + read: { offset: 1, limit: 1, offsetChars: 0, limitChars: 1 }, + grep: { contextBefore: 0, contextAfter: 0 }, +}; + +/** + * Why a tool call's input doesn't fit its tool's schema, or null when it does: + * an object, each required field present, and each field its schema calls a + * string, a boolean or an integer of that type, an integer at least its + * minimum. The workspace's schemas use no other types. Without it, an edit + * missing `newString` wrote the word "undefined", and an offset of 1.5 + * numbered lines 1.5, 2.5 and on. A field the schema doesn't describe is left + * to its handler. + */ +function inputProblem( + schema: { properties?: Record; required?: string[] }, + input: unknown, + minimums: Record = {}, +): string | null { + if (input === null || typeof input !== 'object' || Array.isArray(input)) { + return 'its input must be an object of the fields its schema lists'; + } + const fields = input as Record; + for (const name of schema.required ?? []) { + if (fields[name] === undefined) return `"${name}" is required`; + } + for (const [name, value] of Object.entries(fields)) { + const type = (schema.properties?.[name] as { type?: string } | undefined)?.type; + if (value === undefined || type === undefined) continue; + const minimum = minimums[name]; + const fits = type === 'string' ? typeof value === 'string' + : type === 'boolean' ? typeof value === 'boolean' + : type === 'integer' + ? Number.isSafeInteger(value) && (minimum === undefined || (value as number) >= minimum) + : true; + if (!fits) { + const wanted = type === 'string' ? 'a string' + : type === 'boolean' ? 'true or false' + : minimum === undefined ? 'a whole number' : `a whole number of at least ${minimum}`; + const got = value === null ? 'null' + : typeof value === 'number' ? String(value) + : Array.isArray(value) ? 'an array' + : typeof value === 'object' ? 'an object' + : `a ${typeof value}`; + return `"${name}" must be ${wanted}, not ${got}`; + } + } + return null; +} + /** * Convert a glob pattern to a RegExp. */ diff --git a/src/modules/workspace/reconcile.ts b/src/modules/workspace/reconcile.ts index d2b0a4b13..72f70577c 100644 --- a/src/modules/workspace/reconcile.ts +++ b/src/modules/workspace/reconcile.ts @@ -340,6 +340,24 @@ function settleAdoption(p: Pending, s: { hash: string; size: number } | null): A return p.prior ?? 'forget'; } +/** + * What's in the way, and the way out, when disk's shape keeps a file from + * being written at its path: no push writes a file over a directory, or below + * a path that's a file on disk, forced or not (`writeContained`). Null for any + * other reason, which says its own way out. + */ +function inTheWay(reason: string | undefined): string | null { + if (reason === 'a directory') { + return 'disk has a directory at this path, and no push writes a file over one, forced or not — ' + + 'move or remove that directory on disk and materialize again, or write the file under another path'; + } + if (reason === 'a parent path is not a directory') { + return 'disk has a file where this path needs a directory, and no push writes below one, forced or not — ' + + 'move or remove that file on disk and materialize again, or write the file under another path'; + } + return null; +} + /** * The synchronous precondition of every workspace mutation: evidence owed a * barrier is made durable first, and an adoption left pending on the @@ -1065,7 +1083,11 @@ export async function pushPaths( } if (v.state === 'unverified') { if (!opts.force) { - result.skipped.push({ path, reason: `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and materialize again, or pass force to overwrite it` }); + result.skipped.push({ + path, + reason: (s ? inTheWay(v.note) : null) + ?? `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and materialize again, or pass force to overwrite it`, + }); continue; } } @@ -1167,7 +1189,8 @@ export async function pushPaths( // exactly what it was, whatever kind that was. agreement.restore(mount.name, step.path, step.before); recorded = true; - result.skipped.push({ path: step.path, reason: `not ${step.kind === 'write' ? 'written' : 'unlinked'}: ${failure.message}` }); + const why = (step.kind === 'write' ? inTheWay(failure.message) : null) ?? failure.message; + result.skipped.push({ path: step.path, reason: `not ${step.kind === 'write' ? 'written' : 'unlinked'}: ${why}` }); } else { // No completion: the intent stays pending, and the next observation // resolves it from whatever disk then holds. diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts new file mode 100644 index 000000000..b9e133507 --- /dev/null +++ b/test/workspace-file-tools.test.ts @@ -0,0 +1,284 @@ +// The workspace's file tools write exactly what they're given or refuse, +// say when a change is stored but not on disk, number a file's lines as it +// has them, and edit the text disk holds now. +import test, { type TestContext } from 'node:test'; +import assert from 'node:assert/strict'; +import { existsSync, linkSync, 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 { ToolResult } from '../src/types/events.js'; + +function setup(t: TestContext, opts: { autoMaterialize?: boolean } = {}) { + const root = mkdtempSync(join(tmpdir(), 'workspace-file-tools-')); + const dir = join(root, 'mount'); + mkdirSync(dir); + const store = JsStore.openOrCreate({ path: join(root, 'store') }); + const workspace = new WorkspaceModule({ + mounts: [{ name: 'work', path: dir, mode: 'read-write', watch: 'never', autoMaterialize: opts.autoMaterialize ?? false }], + }); + workspace.initStore(store); + t.after(() => { store.close(); rmSync(root, { recursive: true, force: true }); }); + const call = (name: string, input: unknown): Promise => + workspace.handleToolCall({ id: name, name, input } as Parameters[0]); + const disk = (rel: string): Buffer | null => (existsSync(join(dir, rel)) ? readFileSync(join(dir, rel)) : null); + const stored = async (rel: string): Promise => { + const result = await call('read', { path: `work/${rel}`, offsetChars: 0, limitChars: 100_000 }); + assert.equal(result.success, true, result.error); + return (result.data as { content: string }).content; + }; + return { workspace, dir, call, disk, stored }; +} + +const errorOf = (result: ToolResult): string => { + assert.equal(result.success, false, 'the call was refused'); + assert.equal(result.isError, true); + return result.error ?? ''; +}; + +// --------------------------------------------------------------------------- +// Part 1: exactly what was given, or a refusal +// --------------------------------------------------------------------------- + +test('an edit writes its replacement as given, $ patterns and all', async (t) => { + const { dir, call, disk } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'price.txt'), 'price: 10\n'); + const one = await call('edit', { path: 'work/price.txt', oldString: '10', newString: "$$5, $&, $', $`, $1" }); + assert.equal(one.success, true, one.error); + assert.equal(disk('price.txt')!.toString(), "price: $$5, $&, $', $`, $1\n"); + + writeFileSync(join(dir, 'pair.txt'), 'a-a\n'); + const all = await call('edit', { path: 'work/pair.txt', oldString: 'a', newString: '$&$&', replaceAll: true }); + assert.equal(all.success, true, all.error); + assert.equal(disk('pair.txt')!.toString(), '$&$&-$&$&\n'); +}); + +test("an edit of a file that isn't valid UTF-8 is refused and changes nothing, and a write still replaces it", async (t) => { + const { dir, call, disk } = setup(t, { autoMaterialize: true }); + const latin1 = Buffer.from('caf\xe9 au lait\n', 'latin1'); + writeFileSync(join(dir, 'latin1.txt'), latin1); + const refused = errorOf(await call('edit', { path: 'work/latin1.txt', oldString: 'au', newString: 'with' })); + assert.match(refused, /^The edit did nothing: work\/latin1\.txt isn't valid UTF-8, so editing it would change bytes outside the text it replaces\./); + assert.deepEqual(disk('latin1.txt'), latin1, 'disk keeps every byte'); + + const written = await call('write', { path: 'work/latin1.txt', content: 'café with milk\n' }); + assert.equal(written.success, true, written.error); + assert.equal(disk('latin1.txt')!.toString('utf8'), 'café with milk\n'); +}); + +test('a lone surrogate is refused in a write and in either of an edit\'s strings, and nothing changes', async (t) => { + const { dir, call, disk } = setup(t, { autoMaterialize: true }); + assert.match(errorOf(await call('write', { path: 'work/s.txt', content: 'a\ud800b' })), + /^The write did nothing: "content" holds a lone UTF-16 surrogate/); + assert.equal(disk('s.txt'), null); + + writeFileSync(join(dir, 'e.txt'), 'pair \ud83d\ude00 here\n'); + assert.match(errorOf(await call('edit', { path: 'work/e.txt', oldString: 'here', newString: 'x\udc00' })), + /^The edit did nothing: "newString" holds a lone UTF-16 surrogate/); + assert.match(errorOf(await call('edit', { path: 'work/e.txt', oldString: '\ud83d', newString: 'x' })), + /^The edit did nothing: "oldString" holds a lone UTF-16 surrogate/); + assert.equal(disk('e.txt')!.toString(), 'pair \ud83d\ude00 here\n'); + // A pair is one character, not a lone half. + const paired = await call('edit', { path: 'work/e.txt', oldString: 'here', newString: '\ud83c\udf89' }); + assert.equal(paired.success, true, paired.error); +}); + +test('an empty oldString is refused, with replaceAll or without', async (t) => { + const { dir, call, disk } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'e.txt'), 'abc\n'); + for (const replaceAll of [false, true]) { + assert.match(errorOf(await call('edit', { path: 'work/e.txt', oldString: '', newString: 'X', replaceAll })), + /^The edit did nothing: "oldString" is empty/); + } + assert.equal(disk('e.txt')!.toString(), 'abc\n'); +}); + +test("a call whose input doesn't fit its tool's schema does nothing", async (t) => { + const { dir, call, disk } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'u.txt'), 'keep a\n'); + const cases: Array<[string, unknown, RegExp]> = [ + ['edit', { path: 'work/u.txt', oldString: 'a' }, /^The edit call did nothing: "newString" is required\.$/], + ['edit', { path: 'work/u.txt', oldString: 'a', newString: 5 }, /"newString" must be a string, not 5\.$/], + ['edit', { path: 'work/u.txt', oldString: 'a', newString: null }, /"newString" must be a string, not null\.$/], + ['edit', { path: 'work/u.txt', oldString: 'a', newString: 'b', replaceAll: 'false' }, /"replaceAll" must be true or false, not a string\.$/], + ['write', { path: 'work/u.txt', content: ['x'] }, /^The write call did nothing: "content" must be a string, not an array\.$/], + ['write', { path: 3, content: 'x' }, /"path" must be a string, not 3\.$/], + ['read', { path: 'work/u.txt', offset: '1' }, /"offset" must be a whole number of at least 1, not a string\.$/], + ['grep', { pattern: 'a', contextBefore: -1 }, /"contextBefore" must be a whole number of at least 0, not -1\.$/], + ['delete', null, /^The delete call did nothing: its input must be an object/], + ]; + for (const [name, input, expected] of cases) { + assert.match(errorOf(await call(name, input)), expected, `${name} ${JSON.stringify(input)}`); + } + assert.equal(disk('u.txt')!.toString(), 'keep a\n'); +}); + +test('a write, edit or delete that is stored but refused by disk says so, with inChronicle', async (t) => { + const { dir, call, disk, stored } = setup(t, { autoMaterialize: true }); + + // A write over a disk change the workspace hasn't seen. + writeFileSync(join(dir, 'w.txt'), 'v1\n'); + assert.equal((await call('read', { path: 'work/w.txt' })).success, true); + writeFileSync(join(dir, 'w.txt'), 'v2 from another writer\n'); + const write = await call('write', { path: 'work/w.txt', content: 'v3 agent\n' }); + assert.match(errorOf(write), /^The write of "work\/w\.txt" is stored in the workspace, so don't send it again\. Its push to disk was refused: stale copy: .*Until materialize puts it there, disk doesn't have it\.$/); + assert.deepEqual(write.data, { inChronicle: true, path: 'work/w.txt' }); + assert.equal(await stored('w.txt'), 'v3 agent\n'); + assert.equal(disk('w.txt')!.toString(), 'v2 from another writer\n'); + + // An edit of a file with another name: the push writes in place, which + // would change the other name's file too. + writeFileSync(join(dir, 'h.txt'), 'shared\n'); + linkSync(join(dir, 'h.txt'), join(dir, 'h2.txt')); + const edit = await call('edit', { path: 'work/h.txt', oldString: 'shared', newString: 'shared, edited' }); + assert.match(errorOf(edit), /^The edit of "work\/h\.txt" is stored in the workspace, so don't send it again: a second edit can apply the change twice\. Its push to disk was refused: not written: a file with other hard links/); + assert.deepEqual(edit.data, { inChronicle: true, path: 'work/h.txt' }); + assert.equal(await stored('h.txt'), 'shared, edited\n'); + assert.equal(disk('h2.txt')!.toString(), 'shared\n'); + + // A deletion whose disk copy changed since the workspace read it. + writeFileSync(join(dir, 'd.txt'), 'doomed\n'); + assert.equal((await call('read', { path: 'work/d.txt' })).success, true); + writeFileSync(join(dir, 'd.txt'), 'changed on disk\n'); + const del = await call('delete', { path: 'work/d.txt' }); + assert.match(errorOf(del), /^The deletion of "work\/d\.txt" is stored in the workspace, so don't send it again\. Its push to disk was refused: .*Until materialize with applyDeletions removes it, disk keeps its copy \(ls shows its state\)\.$/); + assert.deepEqual(del.data, { inChronicle: true, path: 'work/d.txt' }); + assert.equal(disk('d.txt')!.toString(), 'changed on disk\n'); +}); + +test('a write, edit or delete that throws after its tree entry is set says the change is stored', async (t) => { + const { workspace, dir, call, stored } = setup(t); + const ws = workspace as unknown as { intents: Map unknown }> }; + const intents = ws.intents.get('work')!; + const update = intents.update; + writeFileSync(join(dir, 'e.txt'), 'line\n'); + writeFileSync(join(dir, 'd.txt'), 'doomed\n'); + assert.equal((await call('read', { path: 'work/e.txt' })).success, true); + assert.equal((await call('read', { path: 'work/d.txt' })).success, true); + intents.update = () => { throw new Error('injected intent exception'); }; + try { + const write = await call('write', { path: 'work/w.txt', content: 'new\n' }); + assert.equal(errorOf(write), 'The write of "work/w.txt" is stored in the workspace, so don\'t send it again. The rest of the call failed: injected intent exception.'); + assert.deepEqual(write.data, { inChronicle: true, path: 'work/w.txt' }); + const edit = await call('edit', { path: 'work/e.txt', oldString: 'line', newString: 'line, edited' }); + assert.match(errorOf(edit), /^The edit of "work\/e\.txt" is stored in the workspace, so don't send it again: a second edit can apply the change twice\. The rest of the call failed: injected intent exception\.$/); + assert.deepEqual(edit.data, { inChronicle: true, path: 'work/e.txt' }); + const del = await call('delete', { path: 'work/d.txt' }); + assert.match(errorOf(del), /^The deletion of "work\/d\.txt" is stored in the workspace, so don't send it again\. The rest of the call failed: injected intent exception\.$/); + assert.deepEqual(del.data, { inChronicle: true, path: 'work/d.txt' }); + } finally { + intents.update = update; + } + assert.equal(await stored('w.txt'), 'new\n'); + assert.equal(await stored('e.txt'), 'line, edited\n'); +}); + +// --------------------------------------------------------------------------- +// Part 2: a read numbers the lines the file has +// --------------------------------------------------------------------------- + +test('a final line end ends the last line, and an empty file reads as empty', async (t) => { + const { dir, call } = setup(t); + writeFileSync(join(dir, 'nl.txt'), 'a\nb\n'); + assert.deepEqual((await call('read', { path: 'work/nl.txt' })).data, { + path: 'work/nl.txt', totalLines: 2, fromLine: 1, toLine: 2, content: ' 1\ta\n 2\tb', + }); + writeFileSync(join(dir, 'blank.txt'), 'a\n\n'); + assert.equal(((await call('read', { path: 'work/blank.txt' })).data as { totalLines: number }).totalLines, 2, + 'an empty line before the final line end is a line'); + writeFileSync(join(dir, 'empty.txt'), ''); + assert.deepEqual((await call('read', { path: 'work/empty.txt' })).data, { path: 'work/empty.txt', totalLines: 0, content: '' }); +}); + +test('grep numbers lines as read does: no match on an empty line after the last', async (t) => { + const { dir, call } = setup(t); + writeFileSync(join(dir, 'g.txt'), 'x\n'); + const result = await call('grep', { pattern: '^$' }); + assert.equal(result.success, true, result.error); + assert.deepEqual((result.data as { results: unknown[] }).results, []); +}); + +test('an offset or limit that is not a whole number of at least 1 is refused', async (t) => { + const { dir, call } = setup(t); + writeFileSync(join(dir, 'nl.txt'), 'a\nb\n'); + for (const input of [{ offset: 0 }, { offset: -1 }, { offset: 1.5 }, { offset: null }, { limit: 0 }, { limit: 2.5 }]) { + assert.match(errorOf(await call('read', { path: 'work/nl.txt', ...input })), + /^The read call did nothing: "(offset|limit)" must be a whole number of at least 1, not /, JSON.stringify(input)); + } +}); + +test('an offset past the last line answers empty, with the line count and no range', async (t) => { + const { dir, call } = setup(t); + writeFileSync(join(dir, 'nl.txt'), 'a\nb\n'); + for (const offset of [3, 10]) { + assert.deepEqual((await call('read', { path: 'work/nl.txt', offset })).data, { + path: 'work/nl.txt', totalLines: 2, note: `Line ${offset} is past the end: the file has 2 lines.`, content: '', + }); + } +}); + +// --------------------------------------------------------------------------- +// Part 2: an edit applies to the text disk holds now +// --------------------------------------------------------------------------- + +test("an edit applies to disk's text when disk changed and the store didn't, and says so", async (t) => { + const { dir, call, disk, stored } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 's.txt'), 'v1 line\n'); + assert.equal((await call('read', { path: 'work/s.txt' })).success, true); + writeFileSync(join(dir, 's.txt'), 'v2 line from another writer\n'); + + const edit = await call('edit', { path: 'work/s.txt', oldString: 'line', newString: 'LINE' }); + assert.equal(edit.success, true, edit.error); + assert.match((edit.data as { note: string }).note, /^Disk's copy had changed since the workspace last agreed with it, so it was taken in first, and the edit applied to that text\.$/); + assert.equal(disk('s.txt')!.toString(), 'v2 LINE from another writer\n'); + assert.equal(await stored('s.txt'), 'v2 LINE from another writer\n'); + + // Text only the older copy held isn't there to replace, and the refusal says why. + writeFileSync(join(dir, 's.txt'), 'v3 from another writer\n'); + assert.match(errorOf(await call('edit', { path: 'work/s.txt', oldString: 'LINE', newString: 'x' })), + /^String not found in work\/s\.txt\. Disk's copy had changed since the workspace last agreed with it, so it was taken in first, and the edit looked in that text\.$/); +}); + +test('an edit of a draft whose disk copy also changed applies to the draft and names the conflict', async (t) => { + const { dir, call, disk, stored } = setup(t); + writeFileSync(join(dir, 'k.txt'), 'base\n'); + assert.equal((await call('read', { path: 'work/k.txt' })).success, true); + assert.equal((await call('write', { path: 'work/k.txt', content: 'agent draft\n' })).success, true); + writeFileSync(join(dir, 'k.txt'), 'another writer\n'); + + const edit = await call('edit', { path: 'work/k.txt', oldString: 'draft', newString: 'draft, edited' }); + assert.equal(edit.success, true, edit.error); + assert.equal((edit.data as { note: string }).note, + "The path is in conflict with disk (both-changed): the edit is in the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's."); + assert.equal(await stored('k.txt'), 'agent draft, edited\n'); + assert.equal(disk('k.txt')!.toString(), 'another writer\n'); +}); + +// --------------------------------------------------------------------------- +// Part 5: smaller texts +// --------------------------------------------------------------------------- + +test("grep's refusal of an invalid pattern gives the parser's reason", async (t) => { + const { dir, call } = setup(t); + writeFileSync(join(dir, 'g.txt'), 'x\n'); + assert.match(errorOf(await call('grep', { pattern: '(' })), /^The grep did nothing: Invalid regular expression: \/\(\/: Unterminated group\.$/); +}); + +test('a refusal raised inside a tool reads without an "Error: " prefix', async (t) => { + const { call } = setup(t); + assert.equal(errorOf(await call('read', { path: 'zzz/a.txt' })), 'Unknown mount: "zzz". Available: work'); +}); + +test("a push that disk's shape refuses names what's in the way and the way out", async (t) => { + const { dir, call } = setup(t, { autoMaterialize: true }); + mkdirSync(join(dir, 'sub')); + assert.match(errorOf(await call('write', { path: 'work/sub', content: 'x\n' })), + /Its push to disk was refused: disk has a directory at this path, and no push writes a file over one, forced or not — move or remove that directory on disk and materialize again, or write the file under another path\./); + const forced = await call('materialize', { path: 'work/sub', force: true }); + assert.match(JSON.stringify(forced.data), /not written: disk has a directory at this path, and no push writes a file over one, forced or not/); + + writeFileSync(join(dir, 'f.txt'), 'file\n'); + assert.match(errorOf(await call('write', { path: 'work/f.txt/x.txt', content: 'x\n' })), + /Its push to disk was refused: not written: disk has a file where this path needs a directory, and no push writes below one, forced or not — move or remove that file on disk and materialize again, or write the file under another path\./); +}); diff --git a/test/workspace-three-way.test.ts b/test/workspace-three-way.test.ts index 250e929d8..a9e9e6fb6 100644 --- a/test/workspace-three-way.test.ts +++ b/test/workspace-three-way.test.ts @@ -499,7 +499,7 @@ describe('workspace deletes', () => { env.writeDisk('b.txt', 'edited in the shell'); const error = await refused(m, 'delete', { path: 'work/b.txt' }); - assert.match(error, /did not unlink/); + assert.match(error, /^The deletion of "work\/b\.txt" is stored in the workspace, so don't send it again\. Its push to disk was refused: /); assert.equal(env.readDisk('b.txt'), 'edited in the shell', 'the shell edit survives'); }); }); From 006b5329b716cc499108f4aa42598878603330de Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 11:12:56 -0400 Subject: [PATCH 02/13] test(workspace): an edit stored but refused by disk applies twice when sent again, which its answer warns of The second edit of a hard-linked file, whose push disk refuses, matches the text the first left and stores the change again: the reason the answer says not to send it again. Co-Authored-By: Claude Opus 5.5 --- test/workspace-file-tools.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts index b9e133507..a8ea87156 100644 --- a/test/workspace-file-tools.test.ts +++ b/test/workspace-file-tools.test.ts @@ -136,6 +136,10 @@ test('a write, edit or delete that is stored but refused by disk says so, with i assert.deepEqual(edit.data, { inChronicle: true, path: 'work/h.txt' }); assert.equal(await stored('h.txt'), 'shared, edited\n'); assert.equal(disk('h2.txt')!.toString(), 'shared\n'); + // Why the answer says not to send it again: the edit's text is still there + // to match, so a second one applies the change twice. + await call('edit', { path: 'work/h.txt', oldString: 'shared', newString: 'shared, edited' }); + assert.equal(await stored('h.txt'), 'shared, edited, edited\n'); // A deletion whose disk copy changed since the workspace read it. writeFileSync(join(dir, 'd.txt'), 'doomed\n'); @@ -282,3 +286,14 @@ test("a push that disk's shape refuses names what's in the way and the way out", assert.match(errorOf(await call('write', { path: 'work/f.txt/x.txt', content: 'x\n' })), /Its push to disk was refused: not written: disk has a file where this path needs a directory, and no push writes below one, forced or not — move or remove that file on disk and materialize again, or write the file under another path\./); }); + +test('a deletion whose path disk has made a directory is not told to write the file elsewhere', async (t) => { + const { dir, call } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'gone.txt'), 'v1\n'); + assert.equal((await call('read', { path: 'work/gone.txt' })).success, true); + rmSync(join(dir, 'gone.txt')); + mkdirSync(join(dir, 'gone.txt')); + const refused = errorOf(await call('delete', { path: 'work/gone.txt' })); + assert.match(refused, /^The deletion of "work\/gone\.txt" is stored in the workspace/); + assert.doesNotMatch(refused, /write the file under another path/, 'nothing is being written'); +}); From a1676bc87c3f9ba8eba3e80abf2fb998bc222ace Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 11:56:53 -0400 Subject: [PATCH 03/13] fix(workspace): the first review's fixes: stored answers quote the push's reason, reasons name materialize's way out, and an edit says which copy it used The fix round of af-ws-fix (wf-2284, judged at 006b5329): every fix-now issue. - storedButUnfinished: "Its push to disk didn't complete: ." with no suffix of its own, which claimed disk lacked the change even when the push had touched disk or failed only its fsync, and named a plain materialize that stale and conflict refusals refuse (I3, I4, I15). The thrown answer adds "ls shows the path's state." (I28). An edit's answer carries its note on which copy it applied to (I10). - reconcile.ts reasons name the tool: "materialize it with force", with applyDeletions for a deletion, where they said "pass force", which write, edit and delete don't take (I16, I4). noEvidence keeps main's text; the interrupted one says materialize. - inTheWay(reason, deletion): a deletion blocked by a directory, or by a file or symlink where its directory should be, says so with no force remedy, for the unverified refusal and a failed unlink alike (I9); the parent text says "a file or a symlink", which the reason covers (I26); its comment is true (I18). - handleEdit: one pass over the path (it settles a pending intent as settleBeforeMutation's would, and the commit enforces the rest), so the notes come from the only pass (I20). Notes say when disk's copy had changed, when it couldn't be checked and the workspace's copy was used (I8), and a not-found answer says when disk had deleted it (I6). A replaceAll that matches nothing is refused (I5). The non-UTF-8 refusal says read shows invalid bytes as U+FFFD, so a write of what read shows puts U+FFFD there (I11). - parsePath refuses a lone surrogate in a path, for every tool (I25). - read: a note whenever the offset given is past the last line, other than offset 1 on an empty file (I7). - handleWrite builds its receipt before the change (I21). - Changelog: null and numeric strings refused for typed fields (I2), the note's scope (I1), write settles only a pending intent (I22), inChronicle for a deletion (I27), the new refusals and reasons. Tests: test/workspace-file-tools.test.ts, now 24 (seven new for the round: a replaceAll miss, a taken-in deletion, an unverified disk copy on both mount kinds, a failure before the entry, a pushing-mount conflict, a thrown push after a take-in, a FIFO deletion); updated expectations for the new texts there and in workspace-three-way (the interrupted reason, a refused deletion's lead). Co-Authored-By: Claude Opus 5.5 --- changelog.d/fix-workspace-file-tools.fixed.md | 16 +- src/modules/workspace/index.ts | 103 ++++++------ src/modules/workspace/reconcile.ts | 39 +++-- test/workspace-file-tools.test.ts | 146 ++++++++++++++++-- test/workspace-three-way.test.ts | 4 +- 5 files changed, 232 insertions(+), 76 deletions(-) diff --git a/changelog.d/fix-workspace-file-tools.fixed.md b/changelog.d/fix-workspace-file-tools.fixed.md index 64380407c..e4409431c 100644 --- a/changelog.d/fix-workspace-file-tools.fixed.md +++ b/changelog.d/fix-workspace-file-tools.fixed.md @@ -1,9 +1,11 @@ -- The workspace's `edit` writes its replacement as given. It read `$&`, `$$`, `` $` `` and `$'` in `newString` as replacement patterns. It now refuses, changing nothing, three edits it used to report as successes: +- The workspace's `edit` writes its replacement as given. It read `$&`, `$$`, `` $` `` and `$'` in `newString` as replacement patterns. It now refuses, changing nothing, four edits it used to report as successes: - an edit of a file that isn't valid UTF-8, where decoding and writing it back turned every invalid byte sequence in the file into U+FFFD; - an empty `oldString`, which matched between every character; - - a lone UTF-16 surrogate in `oldString` or `newString`, which `write` now refuses in `content` too. -- Every workspace tool checks its input against its own schema before it does anything: required fields, strings, booleans, and whole numbers, with `read`'s `offset` and `limit` at least 1 and `grep`'s context lines at least 0. An edit missing `newString` wrote the word "undefined", and `replaceAll: "false"` counted as true. `read`'s `offset` and `limit`, and `grep`'s `contextBefore` and `contextAfter`, are typed `integer` in their schemas. -- A `write`, `edit` or `delete` that the workspace stored but couldn't finish still answers as an error. That covers one whose push to disk was refused, and one that failed after its change was stored. The answer now leads with the fact that the change is stored and mustn't be sent again, and its `data` carries `inChronicle: true`, as `writeBinary`'s does, with the `path`. Before, a refused push's answer began "Wrote to Chronicle", "Edited Chronicle" or "Removed from Chronicle", and a failure after the change was stored gave only the error. Either way, an edit sent again could apply twice. -- `read` counts the lines a file has: a final line end no longer starts an empty extra line, so "a\nb\n" is two lines, and an empty file has none. `grep` numbers lines the same way. An `offset` past the last line answers empty `content` with the file's `totalLines` and a note, and no `fromLine` or `toLine`, where before it claimed a range that ended before it began. -- An `edit` first takes in a change on disk the workspace hasn't seen, as a listing does, applies to that text, and says so in its receipt. Before, it edited the older stored copy, and on a mount that pushes, its push then failed as stale. Where the path's stored copy is a draft and disk changed too, the edit applies to the draft as before, and its answer names the conflict and the two ways out. `write` doesn't do this, since it replaces the whole file. -- `grep`'s refusal of an invalid pattern gives the parser's reason, and the workspace's refusals no longer begin with "Error: ". A push that disk's shape refuses says what's in the way and the way out: a directory at the path, or a file where the path needs a directory. Before, a directory's refusal offered `force`, which can't write there either, and a file in the way named no way out. + - a `replaceAll` whose `oldString` isn't in the file, which stored the file unchanged; + - a lone UTF-16 surrogate in `oldString` or `newString`, which `write` now refuses in `content` too, as every tool does in a path. +- Every workspace tool checks its input against its own schema before it does anything: required fields, strings, booleans, and whole numbers, with `read`'s `offset` and `limit` at least 1 and `grep`'s context lines at least 0. An edit missing `newString` wrote the word "undefined", and `replaceAll: "false"` counted as true. A typed field given as `null`, or a number given as a string, is refused too, as `read` already refused them for `offsetChars` and `limitChars`. Before, some handlers read `null` as the field's default and did arithmetic on a numeric string. `read`'s `offset` and `limit`, and `grep`'s `contextBefore` and `contextAfter`, are typed `integer` in their schemas. +- A `write`, `edit` or `delete` that the workspace stored but couldn't finish still answers as an error. That covers one whose push to disk didn't complete, and one that failed after its change was stored. The answer now leads with the fact that the change is stored and mustn't be sent again, then gives the push's reason, which says what disk holds and the way out. Its `data` carries `inChronicle: true` and the `path`, as `writeBinary`'s does: the call's change is in Chronicle, and for a deletion that change is the file's removal. Before, a refused push's answer began "Wrote to Chronicle", "Edited Chronicle" or "Removed from Chronicle", and a failure after the change was stored gave only the error. Either way, an edit sent again could apply twice. +- A refused push's way out names the tool that takes it, `materialize it with force`, where it said `pass force`. For a deletion it adds `applyDeletions`, which materialize needs to unlink anything. +- `read` counts the lines a file has: a final line end no longer starts an empty extra line, so "a\nb\n" is two lines, and an empty file has none. `grep` numbers lines the same way. An `offset` past the last line answers empty `content` with the file's `totalLines`, no `fromLine` or `toLine`, and a note saying where the file ends. Before, it claimed a range that ended before it began. Reading an empty file from line 1 answers `totalLines: 0` with no note. +- An `edit` first takes in a change on disk to a file the workspace holds, as a listing does, and applies to that text. Its answer says so when disk's copy had changed since the workspace last agreed with it, when disk had deleted the file, or when disk's copy couldn't be checked and it used the workspace's copy. Before, it edited the older stored copy, and on a mount that pushes, its push then failed as stale. Where the path's stored copy is a draft and disk changed too, the edit applies to the draft as before, and its answer names the conflict and the two ways out. `write` doesn't look at disk first, since it replaces the whole file; as before, it only settles a pending intent. +- `grep`'s refusal of an invalid pattern gives the parser's reason, and the workspace's refusals no longer begin with "Error: ". A push that disk's shape refuses says what's in the way and the way out: a directory at the path, or a file or a symlink where the path needs a directory. Before, it offered `force`, which can't write or unlink there either, and a file in the way named no way out. diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 292216fdf..4f445d770 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -1589,6 +1589,11 @@ export class WorkspaceModule implements Module { * Parse a mount-prefixed path into (mountName, relativePath). */ private parsePath(path: string): { mount: MountState; relativePath: string } { + // A file's name on disk is UTF-8, which can't hold a lone surrogate: the + // store would keep one name and disk another. + if (LONE_SURROGATE.test(path)) { + throw new Error(`The path ${JSON.stringify(path)} holds a lone UTF-16 surrogate, which UTF-8 can't store, so no file on disk can have that name. Nothing was done.`); + } const slashIdx = path.indexOf('/'); const mountName = slashIdx >= 0 ? path.slice(0, slashIdx) : path; const relativePath = slashIdx >= 0 ? path.slice(slashIdx + 1) : ''; @@ -2127,7 +2132,8 @@ export class WorkspaceModule implements Module { data: { path: input.path, totalLines: lines.length, - ...(lines.length > 0 + // Reading an empty file from its start is reading it whole. + ...(startLine > 0 ? { note: `Line ${startLine + 1} is past the end: the file has ${lines.length} line${lines.length === 1 ? '' : 's'}.` } : {}), content: '', @@ -2247,6 +2253,7 @@ export class WorkspaceModule implements Module { } const buffer = Buffer.from(input.content, 'utf-8'); + const receipt = { path: input.path, size: buffer.byteLength, hash: hashContent(input.content) }; let committed = false; let refused: string | null; try { @@ -2262,14 +2269,7 @@ export class WorkspaceModule implements Module { } if (refused !== null) return storedButUnfinished('write', input.path, { refused }); - return { - success: true, - data: { - path: input.path, - size: Buffer.byteLength(input.content), - hash: hashContent(input.content), - }, - }; + return { success: true, data: receipt }; } private async handleEdit(input: EditInput): Promise { @@ -2290,22 +2290,37 @@ export class WorkspaceModule implements Module { const store = this.getStore(); let committed = false; + let appliedNote: string | null = null; try { return await this.withMount(mount, async (): Promise => { // The edit applies to the path as disk holds it now: a change on disk - // the store hasn't taken is taken in first, by the rule a listing - // uses. A write doesn't do this, since it replaces the whole file. + // to a file the workspace holds is taken in first, by the rule a + // listing uses. The pass settles a pending intent on the path too, as + // settleBeforeMutation's would, and the commit enforces the rest + // (commitToolChange). A write passes over its path only to settle a + // pending intent, since it replaces the whole file. const pass = await this.passUnlocked(mount, { kind: 'paths', paths: [relativePath] }); const report = pass.reports.get(relativePath); - const tookDisk = pass.ops.some((op) => op.path === relativePath && op.op === 'modified'); - const fromDisk = tookDisk - ? ' Disk\'s copy had changed since the workspace last agreed with it, so it was taken in first, and the edit looked in that text.' - : ''; - await this.settleBeforeMutation(mount, relativePath); // before the edit reads the entry + const took = (op: TreeOp) => pass.ops.some((o) => o.path === relativePath && o.op === op); + // Which copy the edit used, when it isn't the one the workspace last + // agreed on with disk: disk's newer text, or the workspace's copy + // because disk's couldn't be read. A file the workspace didn't hold + // has no older copy, so taking it in needs no word. + const whichCopy = (verb: string): string | null => took('modified') + ? `Disk's copy had changed since the workspace last agreed with it, so it was taken in first, and the edit ${verb} that text.` + : report?.state === 'unverified' + ? `Disk's copy couldn't be checked (${report.note ?? 'not observed'}), so the edit ${verb} the workspace's copy.` + : null; + const lookedIn = whichCopy('looked in'); + const suffix = lookedIn !== null ? ` ${lookedIn}` : ''; + appliedNote = whichCopy('applied to'); const entry = store.treeGet(mount.treeStateId, relativePath); if (!entry) { - return { success: false, error: this.notInWorkspace(input.path, report), isError: true }; + const error = took('deleted') + ? `File not found: ${input.path}. Disk's copy was deleted since the workspace last agreed with it, and that was taken in first.` + : this.notInWorkspace(input.path, report); + return { success: false, error, isError: true }; } const blob = store.getBlob(entry.blobHash); @@ -2318,25 +2333,25 @@ export class WorkspaceModule implements Module { return { success: false, isError: true, - error: `The edit did nothing: ${input.path} isn't valid UTF-8, so editing it would change bytes outside the text it replaces. A write replaces the whole file, as UTF-8, if that's what you want.${fromDisk}`, + error: `The edit did nothing: ${input.path} isn't valid UTF-8, so editing it would change bytes outside the text it replaces. ` + + `Read shows each invalid byte sequence as U+FFFD, and a write replaces the whole file as UTF-8, so writing back what read shows puts U+FFFD in their place.${suffix}`, }; } let content = blob.toString('utf-8'); - // Validate uniqueness - if (!input.replaceAll) { - const count = content.split(input.oldString).length - 1; - if (count === 0) { - return { success: false, error: `String not found in ${input.path}.${fromDisk}`, isError: true }; - } - if (count > 1) { - return { - success: false, - error: `String found ${count} times in ${input.path}. Use replaceAll: true or provide more context.${fromDisk}`, - isError: true, - }; - } + // An edit that would replace nothing is refused, replaceAll or not; + // without replaceAll, the text must occur once. + const count = content.split(input.oldString).length - 1; + if (count === 0) { + return { success: false, error: `String not found in ${input.path}.${suffix}`, isError: true }; + } + if (count > 1 && !input.replaceAll) { + return { + success: false, + error: `String found ${count} times in ${input.path}. Use replaceAll: true or provide more context.${suffix}`, + isError: true, + }; } // A replacer function, so that the new text goes in as given: in a @@ -2353,13 +2368,13 @@ export class WorkspaceModule implements Module { size: newBuffer.byteLength, mode: entry.mode, }, () => { committed = true; }); - if (refused !== null) return storedButUnfinished('edit', input.path, { refused }); + if (refused !== null) return storedButUnfinished('edit', input.path, { refused }, appliedNote); // A mount that doesn't push keeps a conflict until materialize or // sync settles it, so the edit says so. const conflict = report?.state === 'conflict' ? report.conflict?.kind ?? 'both-changed' : null; const notes = [ - ...(tookDisk ? ['Disk\'s copy had changed since the workspace last agreed with it, so it was taken in first, and the edit applied to that text.'] : []), + ...(appliedNote !== null ? [appliedNote] : []), ...(conflict !== null ? [`The path is in conflict with disk (${conflict}): the edit is in the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's.`] : []), @@ -2375,7 +2390,7 @@ export class WorkspaceModule implements Module { }); } catch (error) { if (!committed) throw error; - return storedButUnfinished('edit', input.path, { threw: error instanceof Error ? error.message : String(error) }); + return storedButUnfinished('edit', input.path, { threw: error instanceof Error ? error.message : String(error) }, appliedNote); } } @@ -3250,26 +3265,28 @@ const LONE_SURROGATE = /\p{Cs}/u; /** * The answer to a write, edit or delete that the workspace stored but - * couldn't finish: its push to disk was refused, or what follows the tree + * couldn't finish: its push to disk didn't complete, or what follows the tree * entry threw. It stays an error, since part of the call failed, but it leads * with what the caller must know before acting on it: the change is stored, - * and sending it again could apply it twice. `inChronicle` lets a script tell - * it from a call that changed nothing, as writeBinary's answer does. + * and sending it again could apply it twice. The push's reason says what disk + * holds and the way out, as materialize takes it. `inChronicle` says the + * call's change is in Chronicle (for a deletion, the file's removal), so a + * script can tell it from a call that changed nothing, as writeBinary's + * answer does. An edit's note on which copy it applied to comes last. */ function storedButUnfinished( tool: 'write' | 'edit' | 'delete', path: string, outcome: { refused: string } | { threw: string }, + note: string | null = null, ): ToolResult { const change = tool === 'delete' ? 'deletion' : tool; const stored = `The ${change} of "${path}" is stored in the workspace, so don't send it again` - + (tool === 'edit' ? ': a second edit can apply the change twice' : ''); + + (tool === 'edit' ? ': a second edit can apply the change twice.' : '.'); const rest = 'refused' in outcome - ? `. Its push to disk was refused: ${outcome.refused}. ` + (tool === 'delete' - ? 'Until materialize with applyDeletions removes it, disk keeps its copy (ls shows its state).' - : 'Until materialize puts it there, disk doesn\'t have it.') - : `. The rest of the call failed: ${outcome.threw}.`; - return { success: false, isError: true, error: stored + rest, data: { inChronicle: true, path } }; + ? ` Its push to disk didn't complete: ${outcome.refused}.` + : ` The rest of the call failed: ${outcome.threw}. ls shows the path's state.`; + return { success: false, isError: true, error: stored + rest + (note !== null ? ` ${note}` : ''), data: { inChronicle: true, path } }; } /** diff --git a/src/modules/workspace/reconcile.ts b/src/modules/workspace/reconcile.ts index 72f70577c..5a6cf4ac9 100644 --- a/src/modules/workspace/reconcile.ts +++ b/src/modules/workspace/reconcile.ts @@ -341,19 +341,24 @@ function settleAdoption(p: Pending, s: { hash: string; size: number } | null): A } /** - * What's in the way, and the way out, when disk's shape keeps a file from - * being written at its path: no push writes a file over a directory, or below - * a path that's a file on disk, forced or not (`writeContained`). Null for any - * other reason, which says its own way out. + * What's in the way, and the way out, when disk's shape keeps a push from a + * path: no push writes a file over a directory or unlinks one, or reaches a + * path below a file or a symlink the walk won't take (`writeContained`, + * `unlinkContained`), forced or not. Null for any other reason, which is + * passed on as it is. */ -function inTheWay(reason: string | undefined): string | null { +function inTheWay(reason: string | undefined, deletion: boolean): string | null { if (reason === 'a directory') { - return 'disk has a directory at this path, and no push writes a file over one, forced or not — ' + - 'move or remove that directory on disk and materialize again, or write the file under another path'; + return deletion + ? 'disk has a directory at this path, and no push unlinks one, forced or not — if it should go, move or remove it on disk' + : 'disk has a directory at this path, and no push writes a file over one, forced or not — ' + + 'move or remove that directory on disk and materialize again, or write the file under another path'; } if (reason === 'a parent path is not a directory') { - return 'disk has a file where this path needs a directory, and no push writes below one, forced or not — ' + - 'move or remove that file on disk and materialize again, or write the file under another path'; + return deletion + ? 'disk has a file or a symlink where this path needs a directory, so disk holds no file here for a push to unlink' + : 'disk has a file or a symlink where this path needs a directory, and no push writes below one, forced or not — ' + + 'move or remove it on disk and materialize again, or write the file under another path'; } return null; } @@ -1038,6 +1043,10 @@ export async function pushPaths( const p = agreement.get(mount.name, path); const bi = mount.intents.get(path); const v = decide(d, s, p, bi, branchId, false); + // The ways out a refusal names, as materialize takes them: a deletion + // reaches disk only with applyDeletions. + const again = s ? 'materialize again' : 'materialize again with applyDeletions'; + const forced = s ? 'materialize it with force to overwrite it' : 'materialize it with force and applyDeletions to delete it'; // What the effect may find: what this observation saw, unless forced. const expect: Expect = opts.force || d.kind === 'unobserved' || d.kind === 'unhashed' ? { kind: 'any' } @@ -1051,7 +1060,7 @@ export async function pushPaths( result.skipped.push({ path, reason: 'stale copy: disk changed since it last agreed with the workspace, which did not — ' + - 'sync this path to adopt the disk version, or pass force to overwrite it', + `sync this path to adopt the disk version, or ${forced}`, }); continue; } @@ -1085,8 +1094,8 @@ export async function pushPaths( if (!opts.force) { result.skipped.push({ path, - reason: (s ? inTheWay(v.note) : null) - ?? `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and materialize again, or pass force to overwrite it`, + reason: inTheWay(v.note, !s) + ?? `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and ${again}, or ${forced}`, }); continue; } @@ -1112,9 +1121,9 @@ export async function pushPaths( : kind === 'interrupted' ? 'disk holds bytes that match neither what was being written there nor what disk last agreed with ' + '(conflict: interrupted), as a write that failed or was cut off partway leaves — check the file, then ' + - 'pass force to write the workspace copy over it, or sync this path to adopt the disk copy as it is' + 'materialize it with force to write the workspace copy over it, or sync this path to adopt the disk copy as it is' : `stale copy: disk changed since it last agreed with the workspace (conflict: ${kind}) — ` + - 'sync this path to adopt the disk version, or pass force to overwrite it', + `sync this path to adopt the disk version, or ${forced}`, }); continue; } @@ -1189,7 +1198,7 @@ export async function pushPaths( // exactly what it was, whatever kind that was. agreement.restore(mount.name, step.path, step.before); recorded = true; - const why = (step.kind === 'write' ? inTheWay(failure.message) : null) ?? failure.message; + const why = inTheWay(failure.message, step.kind === 'unlink') ?? failure.message; result.skipped.push({ path: step.path, reason: `not ${step.kind === 'write' ? 'written' : 'unlinked'}: ${why}` }); } else { // No completion: the intent stays pending, and the next observation diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts index a8ea87156..3855b69d7 100644 --- a/test/workspace-file-tools.test.ts +++ b/test/workspace-file-tools.test.ts @@ -1,6 +1,7 @@ // The workspace's file tools write exactly what they're given or refuse, // say when a change is stored but not on disk, number a file's lines as it // has them, and edit the text disk holds now. +import { spawnSync } from 'node:child_process'; import test, { type TestContext } from 'node:test'; import assert from 'node:assert/strict'; import { existsSync, linkSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; @@ -28,7 +29,7 @@ function setup(t: TestContext, opts: { autoMaterialize?: boolean } = {}) { assert.equal(result.success, true, result.error); return (result.data as { content: string }).content; }; - return { workspace, dir, call, disk, stored }; + return { workspace, store, dir, call, disk, stored }; } const errorOf = (result: ToolResult): string => { @@ -72,6 +73,8 @@ test('a lone surrogate is refused in a write and in either of an edit\'s strings assert.match(errorOf(await call('write', { path: 'work/s.txt', content: 'a\ud800b' })), /^The write did nothing: "content" holds a lone UTF-16 surrogate/); assert.equal(disk('s.txt'), null); + assert.match(errorOf(await call('write', { path: 'work/s\ud800.txt', content: 'x' })), + /^The path "work\/s\\ud800\.txt" holds a lone UTF-16 surrogate, which UTF-8 can't store, so no file on disk can have that name\. Nothing was done\.$/); writeFileSync(join(dir, 'e.txt'), 'pair \ud83d\ude00 here\n'); assert.match(errorOf(await call('edit', { path: 'work/e.txt', oldString: 'here', newString: 'x\udc00' })), @@ -102,6 +105,7 @@ test("a call whose input doesn't fit its tool's schema does nothing", async (t) ['edit', { path: 'work/u.txt', oldString: 'a', newString: 5 }, /"newString" must be a string, not 5\.$/], ['edit', { path: 'work/u.txt', oldString: 'a', newString: null }, /"newString" must be a string, not null\.$/], ['edit', { path: 'work/u.txt', oldString: 'a', newString: 'b', replaceAll: 'false' }, /"replaceAll" must be true or false, not a string\.$/], + ['edit', { path: 'work/u.txt', oldString: 'a', newString: 'b', replaceAll: null }, /"replaceAll" must be true or false, not null\.$/], ['write', { path: 'work/u.txt', content: ['x'] }, /^The write call did nothing: "content" must be a string, not an array\.$/], ['write', { path: 3, content: 'x' }, /"path" must be a string, not 3\.$/], ['read', { path: 'work/u.txt', offset: '1' }, /"offset" must be a whole number of at least 1, not a string\.$/], @@ -122,7 +126,7 @@ test('a write, edit or delete that is stored but refused by disk says so, with i assert.equal((await call('read', { path: 'work/w.txt' })).success, true); writeFileSync(join(dir, 'w.txt'), 'v2 from another writer\n'); const write = await call('write', { path: 'work/w.txt', content: 'v3 agent\n' }); - assert.match(errorOf(write), /^The write of "work\/w\.txt" is stored in the workspace, so don't send it again\. Its push to disk was refused: stale copy: .*Until materialize puts it there, disk doesn't have it\.$/); + assert.match(errorOf(write), /^The write of "work\/w\.txt" is stored in the workspace, so don't send it again\. Its push to disk didn't complete: stale copy: disk changed since it last agreed with the workspace \(conflict: both-changed\) — sync this path to adopt the disk version, or materialize it with force to overwrite it\.$/); assert.deepEqual(write.data, { inChronicle: true, path: 'work/w.txt' }); assert.equal(await stored('w.txt'), 'v3 agent\n'); assert.equal(disk('w.txt')!.toString(), 'v2 from another writer\n'); @@ -132,7 +136,7 @@ test('a write, edit or delete that is stored but refused by disk says so, with i writeFileSync(join(dir, 'h.txt'), 'shared\n'); linkSync(join(dir, 'h.txt'), join(dir, 'h2.txt')); const edit = await call('edit', { path: 'work/h.txt', oldString: 'shared', newString: 'shared, edited' }); - assert.match(errorOf(edit), /^The edit of "work\/h\.txt" is stored in the workspace, so don't send it again: a second edit can apply the change twice\. Its push to disk was refused: not written: a file with other hard links/); + assert.match(errorOf(edit), /^The edit of "work\/h\.txt" is stored in the workspace, so don't send it again: a second edit can apply the change twice\. Its push to disk didn't complete: not written: a file with other hard links/); assert.deepEqual(edit.data, { inChronicle: true, path: 'work/h.txt' }); assert.equal(await stored('h.txt'), 'shared, edited\n'); assert.equal(disk('h2.txt')!.toString(), 'shared\n'); @@ -146,7 +150,7 @@ test('a write, edit or delete that is stored but refused by disk says so, with i assert.equal((await call('read', { path: 'work/d.txt' })).success, true); writeFileSync(join(dir, 'd.txt'), 'changed on disk\n'); const del = await call('delete', { path: 'work/d.txt' }); - assert.match(errorOf(del), /^The deletion of "work\/d\.txt" is stored in the workspace, so don't send it again\. Its push to disk was refused: .*Until materialize with applyDeletions removes it, disk keeps its copy \(ls shows its state\)\.$/); + assert.match(errorOf(del), /^The deletion of "work\/d\.txt" is stored in the workspace, so don't send it again\. Its push to disk didn't complete: stale copy: disk changed since it last agreed with the workspace \(conflict: [a-z-]+\) — sync this path to adopt the disk version, or materialize it with force and applyDeletions to delete it\.$/); assert.deepEqual(del.data, { inChronicle: true, path: 'work/d.txt' }); assert.equal(disk('d.txt')!.toString(), 'changed on disk\n'); }); @@ -163,13 +167,13 @@ test('a write, edit or delete that throws after its tree entry is set says the c intents.update = () => { throw new Error('injected intent exception'); }; try { const write = await call('write', { path: 'work/w.txt', content: 'new\n' }); - assert.equal(errorOf(write), 'The write of "work/w.txt" is stored in the workspace, so don\'t send it again. The rest of the call failed: injected intent exception.'); + assert.equal(errorOf(write), 'The write of "work/w.txt" is stored in the workspace, so don\'t send it again. The rest of the call failed: injected intent exception. ls shows the path\'s state.'); assert.deepEqual(write.data, { inChronicle: true, path: 'work/w.txt' }); const edit = await call('edit', { path: 'work/e.txt', oldString: 'line', newString: 'line, edited' }); - assert.match(errorOf(edit), /^The edit of "work\/e\.txt" is stored in the workspace, so don't send it again: a second edit can apply the change twice\. The rest of the call failed: injected intent exception\.$/); + assert.match(errorOf(edit), /^The edit of "work\/e\.txt" is stored in the workspace, so don't send it again: a second edit can apply the change twice\. The rest of the call failed: injected intent exception\. ls shows the path's state\.$/); assert.deepEqual(edit.data, { inChronicle: true, path: 'work/e.txt' }); const del = await call('delete', { path: 'work/d.txt' }); - assert.match(errorOf(del), /^The deletion of "work\/d\.txt" is stored in the workspace, so don't send it again\. The rest of the call failed: injected intent exception\.$/); + assert.match(errorOf(del), /^The deletion of "work\/d\.txt" is stored in the workspace, so don't send it again\. The rest of the call failed: injected intent exception\. ls shows the path's state\.$/); assert.deepEqual(del.data, { inChronicle: true, path: 'work/d.txt' }); } finally { intents.update = update; @@ -215,6 +219,10 @@ test('an offset or limit that is not a whole number of at least 1 is refused', a test('an offset past the last line answers empty, with the line count and no range', async (t) => { const { dir, call } = setup(t); writeFileSync(join(dir, 'nl.txt'), 'a\nb\n'); + writeFileSync(join(dir, 'empty.txt'), ''); + assert.deepEqual((await call('read', { path: 'work/empty.txt', offset: 2 })).data, { + path: 'work/empty.txt', totalLines: 0, note: 'Line 2 is past the end: the file has 0 lines.', content: '', + }); for (const offset of [3, 10]) { assert.deepEqual((await call('read', { path: 'work/nl.txt', offset })).data, { path: 'work/nl.txt', totalLines: 2, note: `Line ${offset} is past the end: the file has 2 lines.`, content: '', @@ -278,13 +286,13 @@ test("a push that disk's shape refuses names what's in the way and the way out", const { dir, call } = setup(t, { autoMaterialize: true }); mkdirSync(join(dir, 'sub')); assert.match(errorOf(await call('write', { path: 'work/sub', content: 'x\n' })), - /Its push to disk was refused: disk has a directory at this path, and no push writes a file over one, forced or not — move or remove that directory on disk and materialize again, or write the file under another path\./); + /Its push to disk didn't complete: disk has a directory at this path, and no push writes a file over one, forced or not — move or remove that directory on disk and materialize again, or write the file under another path\./); const forced = await call('materialize', { path: 'work/sub', force: true }); assert.match(JSON.stringify(forced.data), /not written: disk has a directory at this path, and no push writes a file over one, forced or not/); writeFileSync(join(dir, 'f.txt'), 'file\n'); assert.match(errorOf(await call('write', { path: 'work/f.txt/x.txt', content: 'x\n' })), - /Its push to disk was refused: not written: disk has a file where this path needs a directory, and no push writes below one, forced or not — move or remove that file on disk and materialize again, or write the file under another path\./); + /Its push to disk didn't complete: not written: disk has a file or a symlink where this path needs a directory, and no push writes below one, forced or not — move or remove it on disk and materialize again, or write the file under another path\./); }); test('a deletion whose path disk has made a directory is not told to write the file elsewhere', async (t) => { @@ -296,4 +304,124 @@ test('a deletion whose path disk has made a directory is not told to write the f const refused = errorOf(await call('delete', { path: 'work/gone.txt' })); assert.match(refused, /^The deletion of "work\/gone\.txt" is stored in the workspace/); assert.doesNotMatch(refused, /write the file under another path/, 'nothing is being written'); + assert.match(refused, /Its push to disk didn't complete: disk has a directory at this path, and no push unlinks one, forced or not — if it should go, move or remove it on disk\.$/); +}); + +// --------------------------------------------------------------------------- +// The fix round of the first review (af-ws-fix) +// --------------------------------------------------------------------------- + +test('an edit that would replace nothing is refused, with replaceAll too', async (t) => { + const { dir, call, disk } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'r.txt'), 'abc\n'); + assert.equal(errorOf(await call('edit', { path: 'work/r.txt', oldString: 'zzz', newString: 'y', replaceAll: true })), 'String not found in work/r.txt.'); + assert.equal(disk('r.txt')!.toString(), 'abc\n'); +}); + +test('an edit of a file disk deleted since the workspace read it says so', async (t) => { + const { dir, call } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'g.txt'), 'here\n'); + assert.equal((await call('read', { path: 'work/g.txt' })).success, true); + rmSync(join(dir, 'g.txt')); + assert.equal(errorOf(await call('edit', { path: 'work/g.txt', oldString: 'here', newString: 'x' })), + "File not found: work/g.txt. Disk's copy was deleted since the workspace last agreed with it, and that was taken in first."); + assert.equal(existsSync(join(dir, 'g.txt')), false, 'the edit put nothing back'); +}); + +test("an edit whose disk copy can't be checked says it used the workspace's copy, stored or not", async (t) => { + for (const autoMaterialize of [false, true]) { + const { dir, call, stored } = setup(t, { autoMaterialize }); + writeFileSync(join(dir, 'u.txt'), 'held\n'); + assert.equal((await call('read', { path: 'work/u.txt' })).success, true); + rmSync(join(dir, 'u.txt')); + mkdirSync(join(dir, 'u.txt')); + const used = "Disk's copy couldn't be checked (a directory), so the edit applied to the workspace's copy."; + const edit = await call('edit', { path: 'work/u.txt', oldString: 'held', newString: 'held, edited' }); + if (autoMaterialize) { + // Its push meets the directory, and the stored answer carries the note. + assert.equal(errorOf(edit), 'The edit of "work/u.txt" is stored in the workspace, so don\'t send it again: a second edit can apply the change twice. ' + + "Its push to disk didn't complete: disk has a directory at this path, and no push writes a file over one, forced or not — " + + `move or remove that directory on disk and materialize again, or write the file under another path. ${used}`); + } else { + assert.equal(edit.success, true, edit.error); + assert.equal((edit.data as { note: string }).note, used); + } + assert.equal(await stored('u.txt'), 'held, edited\n'); + assert.equal(errorOf(await call('edit', { path: 'work/u.txt', oldString: 'zzz', newString: 'x' })), + "String not found in work/u.txt. Disk's copy couldn't be checked (a directory), so the edit looked in the workspace's copy."); + } +}); + +test('a write, edit or delete that fails before its change is stored answers only the failure', async (t) => { + const { workspace, dir, call, stored } = setup(t); + writeFileSync(join(dir, 'e.txt'), 'line\n'); + assert.equal((await call('read', { path: 'work/e.txt' })).success, true); + // Every write, edit and delete takes the mount's turn before anything else. + const ws = workspace as unknown as { withMount: (...args: unknown[]) => Promise }; + const withMount = ws.withMount; + ws.withMount = async () => { throw new Error('injected before the entry'); }; + try { + const calls: Array<[string, Record]> = [ + ['write', { path: 'work/w.txt', content: 'new\n' }], + ['edit', { path: 'work/e.txt', oldString: 'line', newString: 'x' }], + ['delete', { path: 'work/e.txt' }], + ]; + for (const [name, input] of calls) { + const result = await call(name, input); + assert.equal(errorOf(result), 'injected before the entry', name); + assert.equal((result.data as { inChronicle?: boolean } | undefined)?.inChronicle, undefined, name); + } + } finally { + ws.withMount = withMount; + } + assert.equal(await stored('e.txt'), 'line\n'); + assert.equal((await call('read', { path: 'work/w.txt' })).success, false, 'nothing was stored'); +}); + +test('an edit of a draft whose disk copy also changed, on a mount that pushes, names the conflict its push met', async (t) => { + const { dir, call, disk, stored } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'k.txt'), 'base\n'); + assert.equal((await call('read', { path: 'work/k.txt' })).success, true); + writeFileSync(join(dir, 'k.txt'), 'another writer\n'); + // The write's push is refused, so the workspace keeps it as a draft. + assert.match(errorOf(await call('write', { path: 'work/k.txt', content: 'agent draft\n' })), /^The write of "work\/k\.txt" is stored in the workspace/); + const edit = await call('edit', { path: 'work/k.txt', oldString: 'draft', newString: 'draft, edited' }); + assert.equal(errorOf(edit), 'The edit of "work/k.txt" is stored in the workspace, so don\'t send it again: a second edit can apply the change twice. ' + + "Its push to disk didn't complete: stale copy: disk changed since it last agreed with the workspace (conflict: both-changed) — " + + 'sync this path to adopt the disk version, or materialize it with force to overwrite it.'); + assert.equal(await stored('k.txt'), 'agent draft, edited\n'); + assert.equal(disk('k.txt')!.toString(), 'another writer\n'); +}); + +test('an edit that fails after its change is stored, having taken disk\'s change in, says both', async (t) => { + const { workspace, dir, call, stored } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'e.txt'), 'v1 line\n'); + assert.equal((await call('read', { path: 'work/e.txt' })).success, true); + writeFileSync(join(dir, 'e.txt'), 'v2 line from another writer\n'); + const ws = workspace as unknown as { pushUnlocked: (...args: unknown[]) => Promise }; + const push = ws.pushUnlocked; + ws.pushUnlocked = async () => { throw new Error('injected push exception'); }; + try { + const edit = await call('edit', { path: 'work/e.txt', oldString: 'line', newString: 'LINE' }); + assert.equal(errorOf(edit), 'The edit of "work/e.txt" is stored in the workspace, so don\'t send it again: a second edit can apply the change twice. ' + + "The rest of the call failed: injected push exception. ls shows the path's state. " + + "Disk's copy had changed since the workspace last agreed with it, so it was taken in first, and the edit applied to that text."); + } finally { + ws.pushUnlocked = push; + } + assert.equal(await stored('e.txt'), 'v2 LINE from another writer\n'); +}); + +test('a deletion whose disk copy is not a regular file names the materialize that unlinks it', async (t) => { + const { dir, call } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'q.txt'), 'v1\n'); + assert.equal((await call('read', { path: 'work/q.txt' })).success, true); + rmSync(join(dir, 'q.txt')); + assert.equal(spawnSync('mkfifo', [join(dir, 'q.txt')]).status, 0, 'mkfifo'); + assert.equal(errorOf(await call('delete', { path: 'work/q.txt' })), 'The deletion of "work/q.txt" is stored in the workspace, so don\'t send it again. ' + + "Its push to disk didn't complete: cannot verify disk copy (not a regular file) — " + + 'fix it and materialize again with applyDeletions, or materialize it with force and applyDeletions to delete it.'); + const forced = await call('materialize', { path: 'work/q.txt', force: true, applyDeletions: true }); + assert.equal(forced.success, true, forced.error); + assert.equal(existsSync(join(dir, 'q.txt')), false, 'the forced materialize unlinked it'); }); diff --git a/test/workspace-three-way.test.ts b/test/workspace-three-way.test.ts index a9e9e6fb6..964fda444 100644 --- a/test/workspace-three-way.test.ts +++ b/test/workspace-three-way.test.ts @@ -499,7 +499,7 @@ describe('workspace deletes', () => { env.writeDisk('b.txt', 'edited in the shell'); const error = await refused(m, 'delete', { path: 'work/b.txt' }); - assert.match(error, /^The deletion of "work\/b\.txt" is stored in the workspace, so don't send it again\. Its push to disk was refused: /); + assert.match(error, /^The deletion of "work\/b\.txt" is stored in the workspace, so don't send it again\. Its push to disk didn't complete: /); assert.equal(env.readDisk('b.txt'), 'edited in the shell', 'the shell edit survives'); }); }); @@ -3223,7 +3223,7 @@ describe('the mount boundary', () => { const data = await call(m, 'materialize', {}); const skip = JSON.stringify(data); assert.match(skip, /match neither what was being written there nor what disk last agreed with/); - assert.match(skip, /pass force to write the workspace copy over it/); + assert.match(skip, /materialize it with force to write the workspace copy over it/); assert.equal(env.readDisk('a.txt'), 'v2, cut off'); await call(m, 'materialize', { force: true }); assert.equal(env.readDisk('a.txt'), 'v2'); From bac4e74bef38befb8cc77d4036b0a8dcea22e12c Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 12:28:50 -0400 Subject: [PATCH 04/13] fix(workspace): the second review's fixes: a deletion's every reason names applyDeletions, the unverified fallback drops force, and the edit's notes say only what its pass saw The fix round of af-ws-fix-r1 (wf-2303, judged at a1676bc8): every fix-now issue. - reconcile.ts: the no-evidence and interrupted conflicts take the deletion's wording too (decide() returns a recorded conflict and an interrupted one before its tombstone branch); the interrupted one uses `forced` (I1). The unverified fallback says only "fix it and materialize again" (with applyDeletions for a deletion): force can't pass a symlink the mount doesn't follow, or a FIFO a write meets (I4, I7). A deletion below a file or symlink says no push unlinks below one, since a file can still be there behind a symlink (I10). - handleEdit: the deletion note needs an adopted removal (state synced), so a copy disk made binary or too large gets the disk-only answer (I2). The unverified note says the path couldn't be checked against disk, whatever the cause (I11). - commitToolChange's comment names the edit's own pass as a way of settling (I19). - Changelog: a lone surrogate in oldString was a miss, not a success (I20); the unverified fallback's text. Tests: test/workspace-file-tools.test.ts, now 29: the non-UTF-8 refusal asserted whole and a many-matches refusal after a take-in (I3); a disk copy made binary (I2); a deletion of a colliding draft (I1); a forced deletion over a directory (I5); a deletion whose parent became a file (I6, I10); a write over a FIFO (I7). workspace-three-way's interrupted expectation takes the new wording. Co-Authored-By: Claude Opus 5.5 --- changelog.d/fix-workspace-file-tools.fixed.md | 4 +- src/modules/workspace/index.ts | 10 ++- src/modules/workspace/reconcile.ts | 13 ++-- test/workspace-file-tools.test.ts | 66 +++++++++++++++++-- test/workspace-three-way.test.ts | 2 +- 5 files changed, 80 insertions(+), 15 deletions(-) diff --git a/changelog.d/fix-workspace-file-tools.fixed.md b/changelog.d/fix-workspace-file-tools.fixed.md index e4409431c..c131f923a 100644 --- a/changelog.d/fix-workspace-file-tools.fixed.md +++ b/changelog.d/fix-workspace-file-tools.fixed.md @@ -2,10 +2,10 @@ - an edit of a file that isn't valid UTF-8, where decoding and writing it back turned every invalid byte sequence in the file into U+FFFD; - an empty `oldString`, which matched between every character; - a `replaceAll` whose `oldString` isn't in the file, which stored the file unchanged; - - a lone UTF-16 surrogate in `oldString` or `newString`, which `write` now refuses in `content` too, as every tool does in a path. + - a lone UTF-16 surrogate in `newString`, which was written as U+FFFD. One in `oldString`, which used to miss, is refused the same way. `write` now refuses one in `content` too, as every tool does in a path. - Every workspace tool checks its input against its own schema before it does anything: required fields, strings, booleans, and whole numbers, with `read`'s `offset` and `limit` at least 1 and `grep`'s context lines at least 0. An edit missing `newString` wrote the word "undefined", and `replaceAll: "false"` counted as true. A typed field given as `null`, or a number given as a string, is refused too, as `read` already refused them for `offsetChars` and `limitChars`. Before, some handlers read `null` as the field's default and did arithmetic on a numeric string. `read`'s `offset` and `limit`, and `grep`'s `contextBefore` and `contextAfter`, are typed `integer` in their schemas. - A `write`, `edit` or `delete` that the workspace stored but couldn't finish still answers as an error. That covers one whose push to disk didn't complete, and one that failed after its change was stored. The answer now leads with the fact that the change is stored and mustn't be sent again, then gives the push's reason, which says what disk holds and the way out. Its `data` carries `inChronicle: true` and the `path`, as `writeBinary`'s does: the call's change is in Chronicle, and for a deletion that change is the file's removal. Before, a refused push's answer began "Wrote to Chronicle", "Edited Chronicle" or "Removed from Chronicle", and a failure after the change was stored gave only the error. Either way, an edit sent again could apply twice. -- A refused push's way out names the tool that takes it, `materialize it with force`, where it said `pass force`. For a deletion it adds `applyDeletions`, which materialize needs to unlink anything. +- A refused push's way out names the tool that takes it, `materialize it with force`, where it said `pass force`. For a deletion it adds `applyDeletions`, which materialize needs to unlink anything. A disk copy that can't be verified is told only to fix it and materialize again, since force can't pass a symlink the mount doesn't follow, or a FIFO a write meets. - `read` counts the lines a file has: a final line end no longer starts an empty extra line, so "a\nb\n" is two lines, and an empty file has none. `grep` numbers lines the same way. An `offset` past the last line answers empty `content` with the file's `totalLines`, no `fromLine` or `toLine`, and a note saying where the file ends. Before, it claimed a range that ended before it began. Reading an empty file from line 1 answers `totalLines: 0` with no note. - An `edit` first takes in a change on disk to a file the workspace holds, as a listing does, and applies to that text. Its answer says so when disk's copy had changed since the workspace last agreed with it, when disk had deleted the file, or when disk's copy couldn't be checked and it used the workspace's copy. Before, it edited the older stored copy, and on a mount that pushes, its push then failed as stale. Where the path's stored copy is a draft and disk changed too, the edit applies to the draft as before, and its answer names the conflict and the two ways out. `write` doesn't look at disk first, since it replaces the whole file; as before, it only settles a pending intent. - `grep`'s refusal of an invalid pattern gives the parser's reason, and the workspace's refusals no longer begin with "Error: ". A push that disk's shape refuses says what's in the way and the way out: a directory at the path, or a file or a symlink where the path needs a directory. Before, it offered `force`, which can't write or unlink there either, and a file in the way named no way out. diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 4f445d770..5fd90ee51 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -1959,7 +1959,8 @@ export class WorkspaceModule implements Module { * Returns the reason when the change reached the workspace but not disk, so * callers surface it in the tool result: the autoMaterialize contract is * that disk is the source of truth for downstream agents. The caller holds - * the mount's turn and has settled the path first (settleBeforeMutation). + * the mount's turn and has settled the path first (settleBeforeMutation, or + * a pass over the path, as an edit makes). */ private async commitToolChange( mount: MountState, @@ -2309,7 +2310,7 @@ export class WorkspaceModule implements Module { const whichCopy = (verb: string): string | null => took('modified') ? `Disk's copy had changed since the workspace last agreed with it, so it was taken in first, and the edit ${verb} that text.` : report?.state === 'unverified' - ? `Disk's copy couldn't be checked (${report.note ?? 'not observed'}), so the edit ${verb} the workspace's copy.` + ? `The path couldn't be checked against disk (${report.note ?? 'not observed'}), so the edit ${verb} the workspace's copy.` : null; const lookedIn = whichCopy('looked in'); const suffix = lookedIn !== null ? ` ${lookedIn}` : ''; @@ -2317,7 +2318,10 @@ export class WorkspaceModule implements Module { const entry = store.treeGet(mount.treeStateId, relativePath); if (!entry) { - const error = took('deleted') + // A pass also drops a copy disk made binary or too large, which + // the disk-only answer describes; only an adopted removal is a + // deletion. + const error = took('deleted') && report?.state === 'synced' ? `File not found: ${input.path}. Disk's copy was deleted since the workspace last agreed with it, and that was taken in first.` : this.notInWorkspace(input.path, report); return { success: false, error, isError: true }; diff --git a/src/modules/workspace/reconcile.ts b/src/modules/workspace/reconcile.ts index 5a6cf4ac9..52ebdcd51 100644 --- a/src/modules/workspace/reconcile.ts +++ b/src/modules/workspace/reconcile.ts @@ -356,7 +356,8 @@ function inTheWay(reason: string | undefined, deletion: boolean): string | null } if (reason === 'a parent path is not a directory') { return deletion - ? 'disk has a file or a symlink where this path needs a directory, so disk holds no file here for a push to unlink' + ? 'disk has a file or a symlink where this path needs a directory, and no push unlinks below one, forced or not — ' + + 'if something there should go, move or remove it on disk' : 'disk has a file or a symlink where this path needs a directory, and no push writes below one, forced or not — ' + 'move or remove it on disk and materialize again, or write the file under another path'; } @@ -1094,8 +1095,10 @@ export async function pushPaths( if (!opts.force) { result.skipped.push({ path, - reason: inTheWay(v.note, !s) - ?? `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and ${again}, or ${forced}`, + // Force can't pass most of what keeps disk from being verified (a + // symlink the mount doesn't follow, a FIFO for a write), so the + // fallback names only the way out that holds for every note. + reason: inTheWay(v.note, !s) ?? `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and ${again}`, }); continue; } @@ -1117,11 +1120,11 @@ export async function pushPaths( path, reason: noEvidence ? `disk holds a different copy, and nothing records which is newer (conflict: ${kind}) — ` + - 'sync this path to adopt the disk version, or materialize this path with force to overwrite it' + `sync this path to adopt the disk version, or materialize this path with force ${s ? 'to overwrite it' : 'and applyDeletions to delete it'}` : kind === 'interrupted' ? 'disk holds bytes that match neither what was being written there nor what disk last agreed with ' + '(conflict: interrupted), as a write that failed or was cut off partway leaves — check the file, then ' + - 'materialize it with force to write the workspace copy over it, or sync this path to adopt the disk copy as it is' + `${forced}, or sync this path to adopt the disk copy as it is` : `stale copy: disk changed since it last agreed with the workspace (conflict: ${kind}) — ` + `sync this path to adopt the disk version, or ${forced}`, }); diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts index 3855b69d7..7ec05553c 100644 --- a/test/workspace-file-tools.test.ts +++ b/test/workspace-file-tools.test.ts @@ -60,7 +60,8 @@ test("an edit of a file that isn't valid UTF-8 is refused and changes nothing, a const latin1 = Buffer.from('caf\xe9 au lait\n', 'latin1'); writeFileSync(join(dir, 'latin1.txt'), latin1); const refused = errorOf(await call('edit', { path: 'work/latin1.txt', oldString: 'au', newString: 'with' })); - assert.match(refused, /^The edit did nothing: work\/latin1\.txt isn't valid UTF-8, so editing it would change bytes outside the text it replaces\./); + assert.equal(refused, "The edit did nothing: work/latin1.txt isn't valid UTF-8, so editing it would change bytes outside the text it replaces. " + + 'Read shows each invalid byte sequence as U+FFFD, and a write replaces the whole file as UTF-8, so writing back what read shows puts U+FFFD in their place.'); assert.deepEqual(disk('latin1.txt'), latin1, 'disk keeps every byte'); const written = await call('write', { path: 'work/latin1.txt', content: 'café with milk\n' }); @@ -250,6 +251,9 @@ test("an edit applies to disk's text when disk changed and the store didn't, and writeFileSync(join(dir, 's.txt'), 'v3 from another writer\n'); assert.match(errorOf(await call('edit', { path: 'work/s.txt', oldString: 'LINE', newString: 'x' })), /^String not found in work\/s\.txt\. Disk's copy had changed since the workspace last agreed with it, so it was taken in first, and the edit looked in that text\.$/); + writeFileSync(join(dir, 's.txt'), 'v4 twice twice\n'); + assert.equal(errorOf(await call('edit', { path: 'work/s.txt', oldString: 'twice', newString: 'x' })), + "String found 2 times in work/s.txt. Use replaceAll: true or provide more context. Disk's copy had changed since the workspace last agreed with it, so it was taken in first, and the edit looked in that text."); }); test('an edit of a draft whose disk copy also changed applies to the draft and names the conflict', async (t) => { @@ -335,7 +339,7 @@ test("an edit whose disk copy can't be checked says it used the workspace's copy assert.equal((await call('read', { path: 'work/u.txt' })).success, true); rmSync(join(dir, 'u.txt')); mkdirSync(join(dir, 'u.txt')); - const used = "Disk's copy couldn't be checked (a directory), so the edit applied to the workspace's copy."; + const used = "The path couldn't be checked against disk (a directory), so the edit applied to the workspace's copy."; const edit = await call('edit', { path: 'work/u.txt', oldString: 'held', newString: 'held, edited' }); if (autoMaterialize) { // Its push meets the directory, and the stored answer carries the note. @@ -348,7 +352,7 @@ test("an edit whose disk copy can't be checked says it used the workspace's copy } assert.equal(await stored('u.txt'), 'held, edited\n'); assert.equal(errorOf(await call('edit', { path: 'work/u.txt', oldString: 'zzz', newString: 'x' })), - "String not found in work/u.txt. Disk's copy couldn't be checked (a directory), so the edit looked in the workspace's copy."); + "String not found in work/u.txt. The path couldn't be checked against disk (a directory), so the edit looked in the workspace's copy."); } }); @@ -420,8 +424,62 @@ test('a deletion whose disk copy is not a regular file names the materialize tha assert.equal(spawnSync('mkfifo', [join(dir, 'q.txt')]).status, 0, 'mkfifo'); assert.equal(errorOf(await call('delete', { path: 'work/q.txt' })), 'The deletion of "work/q.txt" is stored in the workspace, so don\'t send it again. ' + "Its push to disk didn't complete: cannot verify disk copy (not a regular file) — " - + 'fix it and materialize again with applyDeletions, or materialize it with force and applyDeletions to delete it.'); + + 'fix it and materialize again with applyDeletions.'); const forced = await call('materialize', { path: 'work/q.txt', force: true, applyDeletions: true }); assert.equal(forced.success, true, forced.error); assert.equal(existsSync(join(dir, 'q.txt')), false, 'the forced materialize unlinked it'); }); + +// --------------------------------------------------------------------------- +// The fix round of the second review (af-ws-fix-r1) +// --------------------------------------------------------------------------- + +test("an edit of a file disk made binary answers the disk-only text, not a deletion", async (t) => { + const { dir, call } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'b.txt'), 'text\n'); + assert.equal((await call('read', { path: 'work/b.txt' })).success, true); + writeFileSync(join(dir, 'b.txt'), Buffer.from([0, 1, 2, 0, 255, 0])); + assert.equal(errorOf(await call('edit', { path: 'work/b.txt', oldString: 'text', newString: 'x' })), + 'work/b.txt is on disk but not stored in the workspace (binary or over the size limit).'); +}); + +test('a deletion of a draft that met a disk file the workspace never saw names applyDeletions', async (t) => { + const { dir, call, disk } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'c.txt'), 'disk text\n'); + // The workspace never read c.txt, so its write is a draft that collides. + assert.match(errorOf(await call('write', { path: 'work/c.txt', content: 'agent text\n' })), /conflict: store-origin-collision/); + assert.equal(errorOf(await call('delete', { path: 'work/c.txt' })), 'The deletion of "work/c.txt" is stored in the workspace, so don\'t send it again. ' + + "Its push to disk didn't complete: disk holds a different copy, and nothing records which is newer (conflict: store-origin-collision) — " + + 'sync this path to adopt the disk version, or materialize this path with force and applyDeletions to delete it.'); + assert.equal(disk('c.txt')!.toString(), 'disk text\n'); +}); + +test("a forced deletion over a directory says no push unlinks one", async (t) => { + const { dir, call } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'gone.txt'), 'v1\n'); + assert.equal((await call('read', { path: 'work/gone.txt' })).success, true); + rmSync(join(dir, 'gone.txt')); + mkdirSync(join(dir, 'gone.txt')); + assert.match(errorOf(await call('delete', { path: 'work/gone.txt' })), /^The deletion of "work\/gone\.txt" is stored in the workspace/); + const forced = await call('materialize', { path: 'work/gone.txt', force: true, applyDeletions: true }); + assert.match(JSON.stringify(forced.data), /not unlinked: disk has a directory at this path, and no push unlinks one, forced or not — if it should go, move or remove it on disk/); +}); + +test("a deletion whose parent disk made a file says no push unlinks below it", async (t) => { + const { dir, call } = setup(t); + mkdirSync(join(dir, 'sub')); + writeFileSync(join(dir, 'sub', 'x.txt'), 'v1\n'); + assert.equal((await call('read', { path: 'work/sub/x.txt' })).success, true); + assert.equal((await call('delete', { path: 'work/sub/x.txt' })).success, true); + rmSync(join(dir, 'sub'), { recursive: true }); + writeFileSync(join(dir, 'sub'), 'now a file\n'); + const pushed = await call('materialize', { path: 'work/sub/x.txt', applyDeletions: true }); + assert.match(JSON.stringify(pushed.data), /disk has a file or a symlink where this path needs a directory, and no push unlinks below one, forced or not — if something there should go, move or remove it on disk/); +}); + +test("a write over a FIFO is told to fix it and materialize again, with no force", async (t) => { + const { dir, call } = setup(t, { autoMaterialize: true }); + assert.equal(spawnSync('mkfifo', [join(dir, 'p.txt')]).status, 0, 'mkfifo'); + assert.equal(errorOf(await call('write', { path: 'work/p.txt', content: 'x\n' })), 'The write of "work/p.txt" is stored in the workspace, so don\'t send it again. ' + + "Its push to disk didn't complete: cannot verify disk copy (not a regular file) — fix it and materialize again."); +}); diff --git a/test/workspace-three-way.test.ts b/test/workspace-three-way.test.ts index 964fda444..b78234bdf 100644 --- a/test/workspace-three-way.test.ts +++ b/test/workspace-three-way.test.ts @@ -3223,7 +3223,7 @@ describe('the mount boundary', () => { const data = await call(m, 'materialize', {}); const skip = JSON.stringify(data); assert.match(skip, /match neither what was being written there nor what disk last agreed with/); - assert.match(skip, /materialize it with force to write the workspace copy over it/); + assert.match(skip, /check the file, then materialize it with force to overwrite it, or sync this path/); assert.equal(env.readDisk('a.txt'), 'v2, cut off'); await call(m, 'materialize', { force: true }); assert.equal(env.readDisk('a.txt'), 'v2'); From aef072197d2236089774ba1e2bd0067d0cb9f40c Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 12:47:35 -0400 Subject: [PATCH 05/13] fix(workspace): a taken-in deletion is any drop but a disk-only one, and a deletion below a file gets no text of its own Two of the round's own tests failed at bac4e74b: - The deletion note required the report's state to be synced, but an adopted removal leaves no such report; the gate is now any 'deleted' op but a disk-only drop, which the disk-only answer describes (I2). - A deletion below a file finds nothing to unlink (observe reads the path as absent), so inTheWay's deletion arm for 'a parent path is not a directory' had no way to be reached, and its test failed. The arm and the test are cut; a deletion that ever met that reason would get the unverified fallback, which holds (I6, I10). Probing it showed a defect on main this change doesn't touch: on a mount that doesn't follow symlinks, a deletion's push unlinked a file behind a symlinked parent that resolves inside the mount (probe-symlink-unlink.ts at 3adbdb86), reported apart. Tests: test/workspace-file-tools.test.ts, now 28. Co-Authored-By: Claude Opus 5.5 --- src/modules/workspace/index.ts | 5 ++--- src/modules/workspace/reconcile.ts | 15 +++++++-------- test/workspace-file-tools.test.ts | 12 ------------ 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 5fd90ee51..5dffccf41 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -2319,9 +2319,8 @@ export class WorkspaceModule implements Module { const entry = store.treeGet(mount.treeStateId, relativePath); if (!entry) { // A pass also drops a copy disk made binary or too large, which - // the disk-only answer describes; only an adopted removal is a - // deletion. - const error = took('deleted') && report?.state === 'synced' + // the disk-only answer describes; any other drop is disk's deletion. + const error = took('deleted') && report?.state !== 'disk-only' ? `File not found: ${input.path}. Disk's copy was deleted since the workspace last agreed with it, and that was taken in first.` : this.notInWorkspace(input.path, report); return { success: false, error, isError: true }; diff --git a/src/modules/workspace/reconcile.ts b/src/modules/workspace/reconcile.ts index 52ebdcd51..3dff5ce95 100644 --- a/src/modules/workspace/reconcile.ts +++ b/src/modules/workspace/reconcile.ts @@ -342,8 +342,8 @@ function settleAdoption(p: Pending, s: { hash: string; size: number } | null): A /** * What's in the way, and the way out, when disk's shape keeps a push from a - * path: no push writes a file over a directory or unlinks one, or reaches a - * path below a file or a symlink the walk won't take (`writeContained`, + * path: no push writes a file over a directory or unlinks one, or writes + * below a file or a symlink the walk won't take (`writeContained`, * `unlinkContained`), forced or not. Null for any other reason, which is * passed on as it is. */ @@ -354,12 +354,11 @@ function inTheWay(reason: string | undefined, deletion: boolean): string | null : 'disk has a directory at this path, and no push writes a file over one, forced or not — ' + 'move or remove that directory on disk and materialize again, or write the file under another path'; } - if (reason === 'a parent path is not a directory') { - return deletion - ? 'disk has a file or a symlink where this path needs a directory, and no push unlinks below one, forced or not — ' + - 'if something there should go, move or remove it on disk' - : 'disk has a file or a symlink where this path needs a directory, and no push writes below one, forced or not — ' + - 'move or remove it on disk and materialize again, or write the file under another path'; + // A deletion below a file finds nothing there to unlink, so only a write + // meets this one. + if (reason === 'a parent path is not a directory' && !deletion) { + return 'disk has a file or a symlink where this path needs a directory, and no push writes below one, forced or not — ' + + 'move or remove it on disk and materialize again, or write the file under another path'; } return null; } diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts index 7ec05553c..90cf24a12 100644 --- a/test/workspace-file-tools.test.ts +++ b/test/workspace-file-tools.test.ts @@ -465,18 +465,6 @@ test("a forced deletion over a directory says no push unlinks one", async (t) => assert.match(JSON.stringify(forced.data), /not unlinked: disk has a directory at this path, and no push unlinks one, forced or not — if it should go, move or remove it on disk/); }); -test("a deletion whose parent disk made a file says no push unlinks below it", async (t) => { - const { dir, call } = setup(t); - mkdirSync(join(dir, 'sub')); - writeFileSync(join(dir, 'sub', 'x.txt'), 'v1\n'); - assert.equal((await call('read', { path: 'work/sub/x.txt' })).success, true); - assert.equal((await call('delete', { path: 'work/sub/x.txt' })).success, true); - rmSync(join(dir, 'sub'), { recursive: true }); - writeFileSync(join(dir, 'sub'), 'now a file\n'); - const pushed = await call('materialize', { path: 'work/sub/x.txt', applyDeletions: true }); - assert.match(JSON.stringify(pushed.data), /disk has a file or a symlink where this path needs a directory, and no push unlinks below one, forced or not — if something there should go, move or remove it on disk/); -}); - test("a write over a FIFO is told to fix it and materialize again, with no force", async (t) => { const { dir, call } = setup(t, { autoMaterialize: true }); assert.equal(spawnSync('mkfifo', [join(dir, 'p.txt')]).status, 0, 'mkfifo'); From 80c432cd660cfc24f014c08b79de2e40aabe08f4 Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 13:10:06 -0400 Subject: [PATCH 06/13] fix(workspace): an unverified refusal names force where a forced push gets past its note The second review's fix (bac4e74b, for af-ws-fix-r1's I4 and I7) dropped force from the fallback for every disk copy that can't be verified, since a forced push stops at a symlink the mount doesn't follow and at a FIFO for a write. That also dropped it where force works: the full check at aef07219 failed main's own pin, workspace-materialize-freshness's "an unreadable but writable disk copy is refused, not treated as absent", whose reason must name the override, and a forced deletion unlinks what isn't a regular file, as this change's own FIFO test shows. forcePasses names force for a file the module can't read (cannot open (EACCES)), which a forced write overwrites where disk lets it be written and a forced deletion unlinks, and, for a deletion, for what isn't a regular file. Every other note keeps the way out that holds for all: fix it and materialize again. A note a forced push stops at, or one added later, such as F145's for a parent link, gets no false force. The review store's notes for af-ws-fix-r1 I4 and I7 describe the round-2 approach this replaces. --- src/modules/workspace/reconcile.ts | 21 +++++++++++++++++---- test/workspace-file-tools.test.ts | 2 +- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/modules/workspace/reconcile.ts b/src/modules/workspace/reconcile.ts index 3dff5ce95..bee0eb5be 100644 --- a/src/modules/workspace/reconcile.ts +++ b/src/modules/workspace/reconcile.ts @@ -363,6 +363,19 @@ function inTheWay(reason: string | undefined, deletion: boolean): string | null return null; } +/** + * Whether a forced push gets past what kept a disk copy from being verified: + * a file the module can't read, which a forced write overwrites where disk + * lets it be written and a forced deletion unlinks, and, for a deletion, + * what isn't a regular file, which unlinking removes. A forced push stops at + * the rest too (a symlink the mount doesn't follow, a FIFO for a write, a + * parent it can't use), or isn't known to pass them, so their refusal names + * only fixing it. + */ +function forcePasses(note: string | undefined, deletion: boolean): boolean { + return note === 'cannot open (EACCES)' || (deletion && note === 'not a regular file'); +} + /** * The synchronous precondition of every workspace mutation: evidence owed a * barrier is made durable first, and an adoption left pending on the @@ -1094,10 +1107,10 @@ export async function pushPaths( if (!opts.force) { result.skipped.push({ path, - // Force can't pass most of what keeps disk from being verified (a - // symlink the mount doesn't follow, a FIFO for a write), so the - // fallback names only the way out that holds for every note. - reason: inTheWay(v.note, !s) ?? `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and ${again}`, + // Fixing it holds for every note; force is named only where a + // forced push gets past what the note names. + reason: inTheWay(v.note, !s) ?? + `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and ${again}${forcePasses(v.note, !s) ? `, or ${forced}` : ''}`, }); continue; } diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts index 90cf24a12..dda90a268 100644 --- a/test/workspace-file-tools.test.ts +++ b/test/workspace-file-tools.test.ts @@ -424,7 +424,7 @@ test('a deletion whose disk copy is not a regular file names the materialize tha assert.equal(spawnSync('mkfifo', [join(dir, 'q.txt')]).status, 0, 'mkfifo'); assert.equal(errorOf(await call('delete', { path: 'work/q.txt' })), 'The deletion of "work/q.txt" is stored in the workspace, so don\'t send it again. ' + "Its push to disk didn't complete: cannot verify disk copy (not a regular file) — " - + 'fix it and materialize again with applyDeletions.'); + + 'fix it and materialize again with applyDeletions, or materialize it with force and applyDeletions to delete it.'); const forced = await call('materialize', { path: 'work/q.txt', force: true, applyDeletions: true }); assert.equal(forced.success, true, forced.error); assert.equal(existsSync(join(dir, 'q.txt')), false, 'the forced materialize unlinked it'); From d10f4ae36d9c6c57d2199d3a6eba7c6627bb39e5 Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 13:13:23 -0400 Subject: [PATCH 07/13] docs(changelog): an unverified disk copy is offered force only where a forced push gets past it The fragment still said such a copy is told only to fix it and materialize again, which 80c432cd changed: force is named for a file its permissions keep the workspace from reading, and, for a deletion, for what isn't a regular file. Main's forced push leaving a FIFO (for a write) and a symlink the mount doesn't follow as they were is from a probe at 119a8625 (ws-fix-notes/runs/probe-force-main-119a.txt): 'not written: not a regular file' and 'not written: a symlink, which this mount does not follow'. --- changelog.d/fix-workspace-file-tools.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/fix-workspace-file-tools.fixed.md b/changelog.d/fix-workspace-file-tools.fixed.md index c131f923a..43766c9a4 100644 --- a/changelog.d/fix-workspace-file-tools.fixed.md +++ b/changelog.d/fix-workspace-file-tools.fixed.md @@ -5,7 +5,7 @@ - a lone UTF-16 surrogate in `newString`, which was written as U+FFFD. One in `oldString`, which used to miss, is refused the same way. `write` now refuses one in `content` too, as every tool does in a path. - Every workspace tool checks its input against its own schema before it does anything: required fields, strings, booleans, and whole numbers, with `read`'s `offset` and `limit` at least 1 and `grep`'s context lines at least 0. An edit missing `newString` wrote the word "undefined", and `replaceAll: "false"` counted as true. A typed field given as `null`, or a number given as a string, is refused too, as `read` already refused them for `offsetChars` and `limitChars`. Before, some handlers read `null` as the field's default and did arithmetic on a numeric string. `read`'s `offset` and `limit`, and `grep`'s `contextBefore` and `contextAfter`, are typed `integer` in their schemas. - A `write`, `edit` or `delete` that the workspace stored but couldn't finish still answers as an error. That covers one whose push to disk didn't complete, and one that failed after its change was stored. The answer now leads with the fact that the change is stored and mustn't be sent again, then gives the push's reason, which says what disk holds and the way out. Its `data` carries `inChronicle: true` and the `path`, as `writeBinary`'s does: the call's change is in Chronicle, and for a deletion that change is the file's removal. Before, a refused push's answer began "Wrote to Chronicle", "Edited Chronicle" or "Removed from Chronicle", and a failure after the change was stored gave only the error. Either way, an edit sent again could apply twice. -- A refused push's way out names the tool that takes it, `materialize it with force`, where it said `pass force`. For a deletion it adds `applyDeletions`, which materialize needs to unlink anything. A disk copy that can't be verified is told only to fix it and materialize again, since force can't pass a symlink the mount doesn't follow, or a FIFO a write meets. +- A refused push's way out names the tool that takes it, `materialize it with force`, where it said `pass force`. For a deletion it adds `applyDeletions`, which materialize needs to unlink anything. A disk copy that can't be verified is told to fix it and materialize again. `force` is named beside that only where a forced push gets past what kept the copy from being verified: a file its permissions keep the workspace from reading, and, for a deletion, something that isn't a regular file. Before, force was named for every such copy, though a forced push leaves a symlink the mount doesn't follow, or a FIFO a write meets, as it is. - `read` counts the lines a file has: a final line end no longer starts an empty extra line, so "a\nb\n" is two lines, and an empty file has none. `grep` numbers lines the same way. An `offset` past the last line answers empty `content` with the file's `totalLines`, no `fromLine` or `toLine`, and a note saying where the file ends. Before, it claimed a range that ended before it began. Reading an empty file from line 1 answers `totalLines: 0` with no note. - An `edit` first takes in a change on disk to a file the workspace holds, as a listing does, and applies to that text. Its answer says so when disk's copy had changed since the workspace last agreed with it, when disk had deleted the file, or when disk's copy couldn't be checked and it used the workspace's copy. Before, it edited the older stored copy, and on a mount that pushes, its push then failed as stale. Where the path's stored copy is a draft and disk changed too, the edit applies to the draft as before, and its answer names the conflict and the two ways out. `write` doesn't look at disk first, since it replaces the whole file; as before, it only settles a pending intent. - `grep`'s refusal of an invalid pattern gives the parser's reason, and the workspace's refusals no longer begin with "Error: ". A push that disk's shape refuses says what's in the way and the way out: a directory at the path, or a file or a symlink where the path needs a directory. Before, it offered `force`, which can't write or unlink there either, and a file in the way named no way out. From 650487e6d3effb6fce288885dd96fa0236452217 Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 14:56:34 -0400 Subject: [PATCH 08/13] fix(workspace): the third review's fixes: no data on a stored answer, force named on its condition, a sync for an unreadable intent, and the edit's notes for a missing disk copy and a conflicted miss Answers af-ws-fix-r2 (wf-2366, Ogden-2688's review of a1676bc8..d10f4ae3, 15 issues judged in the review store) and Cuthbert-2453's finding (room-203 #132418), with Ada-1017's call on it (#132588). - storedButUnfinished's answer carries no data. An error's data reaches neither the model, which stores the error's text (buildStoredToolResultContent), nor a code_execution script, which gets "Error: " and the text (handleScriptToolCall), and nothing reads it from a tool's answer in process. The design's "for a script to test" didn't hold. writeBinary keeps inChronicle, which three in-process readers use. - forcePasses becomes forcedWay, which returns the clause. For "cannot open (EACCES)", force is named on the condition a forced push needs: "if disk lets the workspace write the file", or for a deletion "unlink the file", since a read's EACCES shows neither (I1). For a deletion, "not a regular file" is offered force as before. - A path whose branch intent can't be read is told to sync the path, which replaces the intent. decide() decides nothing over it until a sync does, so "fix it and materialize again" was refused the same way (I3). - handleEdit says when disk has no copy and nothing records whether it was deleted there (disk-missing-provenance-unknown): the edit uses the workspace's copy, and since commitToolChange marks the path the store's, the file goes back to disk when the path is materialized (I4). The conflict note now comes on a miss or a refusal too, not only on a success (Ogden's optional point). - ls's description names applyDeletions beside force for a path the workspace deleted (I6). - inTheWay's "&& !deletion" guard goes: no deletion meets "a parent path is not a directory" (I8). - Tests: stored answers carry no data; the EACCES texts for a write and a deletion, a forced materialize that writes past a 0200 file and is refused at a 000 one, and a forced unlink of an unreadable file; an unreadable intent's reason and the sync that clears it; the missing-copy note on both mount kinds, with the file back on disk; a conflicted miss's note; no force for a write over a symlink the mount doesn't follow (I14); refusals leave disk as it was (I9). Not taken: I5, I10, I12 and I13 (not-worth), I7 and I15 (not-real), as the verdicts say. Runs, J16731 (on-vps 20261017T185431Z-e1a837bc), on this tree before the commit: the ten workspace test files pass, workspace-file-tools 33/33, workspace-three-way 210/210 and workspace-materialize-freshness 9/9 among them; this round's 17 removals (R28 to R30, R33, R36, R37, R50 to R60) are each caught; and red-first, all 33 cases fail against main's index.ts and reconcile.ts. --- changelog.d/fix-workspace-file-tools.fixed.md | 6 +- src/modules/workspace/index.ts | 45 +++---- src/modules/workspace/reconcile.ts | 37 +++--- test/workspace-file-tools.test.ts | 112 ++++++++++++++++-- 4 files changed, 153 insertions(+), 47 deletions(-) diff --git a/changelog.d/fix-workspace-file-tools.fixed.md b/changelog.d/fix-workspace-file-tools.fixed.md index 43766c9a4..439748583 100644 --- a/changelog.d/fix-workspace-file-tools.fixed.md +++ b/changelog.d/fix-workspace-file-tools.fixed.md @@ -4,8 +4,8 @@ - a `replaceAll` whose `oldString` isn't in the file, which stored the file unchanged; - a lone UTF-16 surrogate in `newString`, which was written as U+FFFD. One in `oldString`, which used to miss, is refused the same way. `write` now refuses one in `content` too, as every tool does in a path. - Every workspace tool checks its input against its own schema before it does anything: required fields, strings, booleans, and whole numbers, with `read`'s `offset` and `limit` at least 1 and `grep`'s context lines at least 0. An edit missing `newString` wrote the word "undefined", and `replaceAll: "false"` counted as true. A typed field given as `null`, or a number given as a string, is refused too, as `read` already refused them for `offsetChars` and `limitChars`. Before, some handlers read `null` as the field's default and did arithmetic on a numeric string. `read`'s `offset` and `limit`, and `grep`'s `contextBefore` and `contextAfter`, are typed `integer` in their schemas. -- A `write`, `edit` or `delete` that the workspace stored but couldn't finish still answers as an error. That covers one whose push to disk didn't complete, and one that failed after its change was stored. The answer now leads with the fact that the change is stored and mustn't be sent again, then gives the push's reason, which says what disk holds and the way out. Its `data` carries `inChronicle: true` and the `path`, as `writeBinary`'s does: the call's change is in Chronicle, and for a deletion that change is the file's removal. Before, a refused push's answer began "Wrote to Chronicle", "Edited Chronicle" or "Removed from Chronicle", and a failure after the change was stored gave only the error. Either way, an edit sent again could apply twice. -- A refused push's way out names the tool that takes it, `materialize it with force`, where it said `pass force`. For a deletion it adds `applyDeletions`, which materialize needs to unlink anything. A disk copy that can't be verified is told to fix it and materialize again. `force` is named beside that only where a forced push gets past what kept the copy from being verified: a file its permissions keep the workspace from reading, and, for a deletion, something that isn't a regular file. Before, force was named for every such copy, though a forced push leaves a symlink the mount doesn't follow, or a FIFO a write meets, as it is. +- A `write`, `edit` or `delete` that the workspace stored but couldn't finish still answers as an error. That covers one whose push to disk didn't complete, and one that failed after its change was stored. The answer now leads with the fact that the change is stored and mustn't be sent again, then gives the push's reason, which says what disk holds and the way out. All of that is in its text, which is what the model and a `code_execution` script receive of an error. Before, a refused push's answer began "Wrote to Chronicle", "Edited Chronicle" or "Removed from Chronicle", and a failure after the change was stored gave only the error. Either way, an edit sent again could apply twice. +- A refused push's way out names the tool that takes it, `materialize it with force`, where it said `pass force`. For a deletion it adds `applyDeletions`, which materialize needs to unlink anything. A disk copy that can't be verified is told to fix it and materialize again. `force` is named beside that only where a forced push gets past what kept the copy from being verified. A file its permissions keep the workspace from reading is offered force if disk lets the workspace write it, or, for a deletion, unlink it, since a read's refusal doesn't show either. For a deletion, something that isn't a regular file is offered force as it is. Before, force was named for every such copy, though a forced push leaves a symlink the mount doesn't follow, or a FIFO a write meets, as it is. A path whose branch intent can't be read is told to sync the path, which replaces the intent. Before, it was told to fix it and materialize again, which is refused the same way until a sync does. `ls`'s description names `applyDeletions` beside `force` for a path the workspace deleted. - `read` counts the lines a file has: a final line end no longer starts an empty extra line, so "a\nb\n" is two lines, and an empty file has none. `grep` numbers lines the same way. An `offset` past the last line answers empty `content` with the file's `totalLines`, no `fromLine` or `toLine`, and a note saying where the file ends. Before, it claimed a range that ended before it began. Reading an empty file from line 1 answers `totalLines: 0` with no note. -- An `edit` first takes in a change on disk to a file the workspace holds, as a listing does, and applies to that text. Its answer says so when disk's copy had changed since the workspace last agreed with it, when disk had deleted the file, or when disk's copy couldn't be checked and it used the workspace's copy. Before, it edited the older stored copy, and on a mount that pushes, its push then failed as stale. Where the path's stored copy is a draft and disk changed too, the edit applies to the draft as before, and its answer names the conflict and the two ways out. `write` doesn't look at disk first, since it replaces the whole file; as before, it only settles a pending intent. +- An `edit` first takes in a change on disk to a file the workspace holds, as a listing does, and applies to that text. Its answer says so when disk's copy had changed since the workspace last agreed with it, when disk had deleted the file, or when disk's copy couldn't be checked and it used the workspace's copy. Before, it edited the older stored copy, and on a mount that pushes, its push then failed as stale. When disk has no copy and nothing records whether it was deleted there, the edit uses the workspace's copy, which goes back to disk when the path is materialized, as before, and its answer now says so. Where the path's stored copy is a draft and disk changed too, the edit applies to the draft as before, and its answer names the conflict and the two ways out, as does an edit that misses there. `write` doesn't look at disk first, since it replaces the whole file; as before, it only settles a pending intent. - `grep`'s refusal of an invalid pattern gives the parser's reason, and the workspace's refusals no longer begin with "Error: ". A push that disk's shape refuses says what's in the way and the way out: a directory at the path, or a file or a symlink where the path needs a directory. Before, it offered `force`, which can't write or unlink there either, and a file in the way named no way out. diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 5dffccf41..6d54d0163 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -1159,7 +1159,8 @@ export class WorkspaceModule implements Module { + 'still on disk); not-in-branch (on disk, absent from this branch\'s workspace); disk-only (binary or over ' + 'the size limit, never stored — shown with size and image type); conflict, with its kind (disk and the ' + 'workspace both changed; disk holds a different copy and nothing records which is newer; or a write was ' - + 'interrupted) — sync the path to take disk, or materialize that path with force to take the workspace; ' + + 'interrupted) — sync the path to take disk, or materialize that path with force to take the workspace, ' + + 'with applyDeletions too where the workspace deleted it; ' + 'disk-missing-provenance-unknown; unverified (disk could not be checked). `incomplete` names regions ' + 'the listing could not see.', inputSchema: { @@ -2305,16 +2306,27 @@ export class WorkspaceModule implements Module { const took = (op: TreeOp) => pass.ops.some((o) => o.path === relativePath && o.op === op); // Which copy the edit used, when it isn't the one the workspace last // agreed on with disk: disk's newer text, or the workspace's copy - // because disk's couldn't be read. A file the workspace didn't hold - // has no older copy, so taking it in needs no word. + // because disk's couldn't be read or disk has none. A file the + // workspace didn't hold has no older copy, so taking it in needs no + // word. const whichCopy = (verb: string): string | null => took('modified') ? `Disk's copy had changed since the workspace last agreed with it, so it was taken in first, and the edit ${verb} that text.` : report?.state === 'unverified' ? `The path couldn't be checked against disk (${report.note ?? 'not observed'}), so the edit ${verb} the workspace's copy.` - : null; - const lookedIn = whichCopy('looked in'); - const suffix = lookedIn !== null ? ` ${lookedIn}` : ''; - appliedNote = whichCopy('applied to'); + : report?.state === 'disk-missing-provenance-unknown' + ? `Disk has no copy of this file, and nothing records whether it was deleted there, so the edit ${verb} the workspace's copy.` + : null; + // A mount that doesn't push keeps a conflict until materialize or + // sync settles it, so the edit says so, whether or not it applied. + const conflict = report?.state === 'conflict' ? report.conflict?.kind ?? 'both-changed' : null; + const conflictNote = (where: string): string | null => conflict === null ? null + : `The path is in conflict with disk (${conflict}): the edit ${where} the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's.`; + const suffix = [whichCopy('looked in'), conflictNote('looked in')].filter((n) => n !== null).map((n) => ` ${n}`).join(''); + // The edit makes the path the workspace's own change (commitToolChange), + // so a file disk lacks goes back to disk with the next push of it. + appliedNote = report?.state === 'disk-missing-provenance-unknown' + ? `${whichCopy('applied to')} It goes back to disk when the path is materialized; if it was deleted there on purpose, delete it here too.` + : whichCopy('applied to'); const entry = store.treeGet(mount.treeStateId, relativePath); if (!entry) { @@ -2373,15 +2385,7 @@ export class WorkspaceModule implements Module { }, () => { committed = true; }); if (refused !== null) return storedButUnfinished('edit', input.path, { refused }, appliedNote); - // A mount that doesn't push keeps a conflict until materialize or - // sync settles it, so the edit says so. - const conflict = report?.state === 'conflict' ? report.conflict?.kind ?? 'both-changed' : null; - const notes = [ - ...(appliedNote !== null ? [appliedNote] : []), - ...(conflict !== null - ? [`The path is in conflict with disk (${conflict}): the edit is in the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's.`] - : []), - ]; + const notes = [appliedNote, conflictNote('is in')].filter((n): n is string => n !== null); return { success: true, data: { @@ -3272,10 +3276,9 @@ const LONE_SURROGATE = /\p{Cs}/u; * entry threw. It stays an error, since part of the call failed, but it leads * with what the caller must know before acting on it: the change is stored, * and sending it again could apply it twice. The push's reason says what disk - * holds and the way out, as materialize takes it. `inChronicle` says the - * call's change is in Chronicle (for a deletion, the file's removal), so a - * script can tell it from a call that changed nothing, as writeBinary's - * answer does. An edit's note on which copy it applied to comes last. + * holds and the way out, as materialize takes it. The text carries all of it, + * since an error's data reaches neither the model nor a code_execution + * script. An edit's note on which copy it applied to comes last. */ function storedButUnfinished( tool: 'write' | 'edit' | 'delete', @@ -3289,7 +3292,7 @@ function storedButUnfinished( const rest = 'refused' in outcome ? ` Its push to disk didn't complete: ${outcome.refused}.` : ` The rest of the call failed: ${outcome.threw}. ls shows the path's state.`; - return { success: false, isError: true, error: stored + rest + (note !== null ? ` ${note}` : ''), data: { inChronicle: true, path } }; + return { success: false, isError: true, error: stored + rest + (note !== null ? ` ${note}` : '') }; } /** diff --git a/src/modules/workspace/reconcile.ts b/src/modules/workspace/reconcile.ts index bee0eb5be..217d5225b 100644 --- a/src/modules/workspace/reconcile.ts +++ b/src/modules/workspace/reconcile.ts @@ -356,7 +356,7 @@ function inTheWay(reason: string | undefined, deletion: boolean): string | null } // A deletion below a file finds nothing there to unlink, so only a write // meets this one. - if (reason === 'a parent path is not a directory' && !deletion) { + if (reason === 'a parent path is not a directory') { return 'disk has a file or a symlink where this path needs a directory, and no push writes below one, forced or not — ' + 'move or remove it on disk and materialize again, or write the file under another path'; } @@ -364,16 +364,21 @@ function inTheWay(reason: string | undefined, deletion: boolean): string | null } /** - * Whether a forced push gets past what kept a disk copy from being verified: - * a file the module can't read, which a forced write overwrites where disk - * lets it be written and a forced deletion unlinks, and, for a deletion, - * what isn't a regular file, which unlinking removes. A forced push stops at - * the rest too (a symlink the mount doesn't follow, a FIFO for a write, a - * parent it can't use), or isn't known to pass them, so their refusal names - * only fixing it. + * The forced way out the refusal of a disk copy that couldn't be verified + * names, if any. A forced push skips the verification, so it gets past the + * note only where its effect can go on. A file the module can't read (EACCES) + * is overwritten, or unlinked, where disk lets the workspace write it, or + * unlink it, which a read's EACCES doesn't show: force is named on that + * condition. What isn't a regular file is unlinked by a forced deletion. No + * other note names force: a forced push stops at some (a symlink the mount + * doesn't follow, a FIFO for a write, a parent it can't use); another writer + * still at the file ("the file changed while it was read") is answered by + * materializing again; and the rest aren't known to pass. */ -function forcePasses(note: string | undefined, deletion: boolean): boolean { - return note === 'cannot open (EACCES)' || (deletion && note === 'not a regular file'); +function forcedWay(note: string | undefined, deletion: boolean, forced: string): string { + if (note === 'cannot open (EACCES)') return `, or, if disk lets the workspace ${deletion ? 'unlink' : 'write'} the file, ${forced}`; + if (deletion && note === 'not a regular file') return `, or ${forced}`; + return ''; } /** @@ -1107,10 +1112,14 @@ export async function pushPaths( if (!opts.force) { result.skipped.push({ path, - // Fixing it holds for every note; force is named only where a - // forced push gets past what the note names. - reason: inTheWay(v.note, !s) ?? - `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and ${again}${forcePasses(v.note, !s) ? `, or ${forced}` : ''}`, + // An intent that can't be read is the one note fixing doesn't + // answer: nothing is decided over it until a sync of the path + // replaces it (decide()). For the rest, fixing what the note names + // holds, and forcedWay says where force does too. + reason: bi?.unreadable !== undefined + ? `${v.note} — sync this path to adopt the disk version, which replaces the intent` + : inTheWay(v.note, !s) ?? + `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and ${again}${forcedWay(v.note, !s, forced)}`, }); continue; } diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts index dda90a268..ae388c40c 100644 --- a/test/workspace-file-tools.test.ts +++ b/test/workspace-file-tools.test.ts @@ -4,7 +4,7 @@ import { spawnSync } from 'node:child_process'; import test, { type TestContext } from 'node:test'; import assert from 'node:assert/strict'; -import { existsSync, linkSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { JsStore } from '@animalabs/chronicle'; @@ -119,7 +119,7 @@ test("a call whose input doesn't fit its tool's schema does nothing", async (t) assert.equal(disk('u.txt')!.toString(), 'keep a\n'); }); -test('a write, edit or delete that is stored but refused by disk says so, with inChronicle', async (t) => { +test('a write, edit or delete that is stored but refused by disk says so, in its text alone', async (t) => { const { dir, call, disk, stored } = setup(t, { autoMaterialize: true }); // A write over a disk change the workspace hasn't seen. @@ -128,7 +128,7 @@ test('a write, edit or delete that is stored but refused by disk says so, with i writeFileSync(join(dir, 'w.txt'), 'v2 from another writer\n'); const write = await call('write', { path: 'work/w.txt', content: 'v3 agent\n' }); assert.match(errorOf(write), /^The write of "work\/w\.txt" is stored in the workspace, so don't send it again\. Its push to disk didn't complete: stale copy: disk changed since it last agreed with the workspace \(conflict: both-changed\) — sync this path to adopt the disk version, or materialize it with force to overwrite it\.$/); - assert.deepEqual(write.data, { inChronicle: true, path: 'work/w.txt' }); + assert.equal(write.data, undefined, "an error's data reaches neither the model nor a script"); assert.equal(await stored('w.txt'), 'v3 agent\n'); assert.equal(disk('w.txt')!.toString(), 'v2 from another writer\n'); @@ -138,7 +138,7 @@ test('a write, edit or delete that is stored but refused by disk says so, with i linkSync(join(dir, 'h.txt'), join(dir, 'h2.txt')); const edit = await call('edit', { path: 'work/h.txt', oldString: 'shared', newString: 'shared, edited' }); assert.match(errorOf(edit), /^The edit of "work\/h\.txt" is stored in the workspace, so don't send it again: a second edit can apply the change twice\. Its push to disk didn't complete: not written: a file with other hard links/); - assert.deepEqual(edit.data, { inChronicle: true, path: 'work/h.txt' }); + assert.equal(edit.data, undefined); assert.equal(await stored('h.txt'), 'shared, edited\n'); assert.equal(disk('h2.txt')!.toString(), 'shared\n'); // Why the answer says not to send it again: the edit's text is still there @@ -152,7 +152,7 @@ test('a write, edit or delete that is stored but refused by disk says so, with i writeFileSync(join(dir, 'd.txt'), 'changed on disk\n'); const del = await call('delete', { path: 'work/d.txt' }); assert.match(errorOf(del), /^The deletion of "work\/d\.txt" is stored in the workspace, so don't send it again\. Its push to disk didn't complete: stale copy: disk changed since it last agreed with the workspace \(conflict: [a-z-]+\) — sync this path to adopt the disk version, or materialize it with force and applyDeletions to delete it\.$/); - assert.deepEqual(del.data, { inChronicle: true, path: 'work/d.txt' }); + assert.equal(del.data, undefined); assert.equal(disk('d.txt')!.toString(), 'changed on disk\n'); }); @@ -169,13 +169,13 @@ test('a write, edit or delete that throws after its tree entry is set says the c try { const write = await call('write', { path: 'work/w.txt', content: 'new\n' }); assert.equal(errorOf(write), 'The write of "work/w.txt" is stored in the workspace, so don\'t send it again. The rest of the call failed: injected intent exception. ls shows the path\'s state.'); - assert.deepEqual(write.data, { inChronicle: true, path: 'work/w.txt' }); + assert.equal(write.data, undefined); const edit = await call('edit', { path: 'work/e.txt', oldString: 'line', newString: 'line, edited' }); assert.match(errorOf(edit), /^The edit of "work\/e\.txt" is stored in the workspace, so don't send it again: a second edit can apply the change twice\. The rest of the call failed: injected intent exception\. ls shows the path's state\.$/); - assert.deepEqual(edit.data, { inChronicle: true, path: 'work/e.txt' }); + assert.equal(edit.data, undefined); const del = await call('delete', { path: 'work/d.txt' }); assert.match(errorOf(del), /^The deletion of "work\/d\.txt" is stored in the workspace, so don't send it again\. The rest of the call failed: injected intent exception\. ls shows the path's state\.$/); - assert.deepEqual(del.data, { inChronicle: true, path: 'work/d.txt' }); + assert.equal(del.data, undefined); } finally { intents.update = update; } @@ -263,6 +263,9 @@ test('an edit of a draft whose disk copy also changed applies to the draft and n assert.equal((await call('write', { path: 'work/k.txt', content: 'agent draft\n' })).success, true); writeFileSync(join(dir, 'k.txt'), 'another writer\n'); + // A miss says so too: the text the agent sought may be disk's. + assert.equal(errorOf(await call('edit', { path: 'work/k.txt', oldString: 'another', newString: 'x' })), + "String not found in work/k.txt. The path is in conflict with disk (both-changed): the edit looked in the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's."); const edit = await call('edit', { path: 'work/k.txt', oldString: 'draft', newString: 'draft, edited' }); assert.equal(edit.success, true, edit.error); assert.equal((edit.data as { note: string }).note, @@ -373,7 +376,6 @@ test('a write, edit or delete that fails before its change is stored answers onl for (const [name, input] of calls) { const result = await call(name, input); assert.equal(errorOf(result), 'injected before the entry', name); - assert.equal((result.data as { inChronicle?: boolean } | undefined)?.inChronicle, undefined, name); } } finally { ws.withMount = withMount; @@ -441,6 +443,7 @@ test("an edit of a file disk made binary answers the disk-only text, not a delet writeFileSync(join(dir, 'b.txt'), Buffer.from([0, 1, 2, 0, 255, 0])); assert.equal(errorOf(await call('edit', { path: 'work/b.txt', oldString: 'text', newString: 'x' })), 'work/b.txt is on disk but not stored in the workspace (binary or over the size limit).'); + assert.deepEqual([...readFileSync(join(dir, 'b.txt'))], [0, 1, 2, 0, 255, 0], 'disk keeps its bytes'); }); test('a deletion of a draft that met a disk file the workspace never saw names applyDeletions', async (t) => { @@ -463,6 +466,7 @@ test("a forced deletion over a directory says no push unlinks one", async (t) => assert.match(errorOf(await call('delete', { path: 'work/gone.txt' })), /^The deletion of "work\/gone\.txt" is stored in the workspace/); const forced = await call('materialize', { path: 'work/gone.txt', force: true, applyDeletions: true }); assert.match(JSON.stringify(forced.data), /not unlinked: disk has a directory at this path, and no push unlinks one, forced or not — if it should go, move or remove it on disk/); + assert.equal(statSync(join(dir, 'gone.txt')).isDirectory(), true, 'the directory stays'); }); test("a write over a FIFO is told to fix it and materialize again, with no force", async (t) => { @@ -470,4 +474,94 @@ test("a write over a FIFO is told to fix it and materialize again, with no force assert.equal(spawnSync('mkfifo', [join(dir, 'p.txt')]).status, 0, 'mkfifo'); assert.equal(errorOf(await call('write', { path: 'work/p.txt', content: 'x\n' })), 'The write of "work/p.txt" is stored in the workspace, so don\'t send it again. ' + "Its push to disk didn't complete: cannot verify disk copy (not a regular file) — fix it and materialize again."); + assert.equal(statSync(join(dir, 'p.txt')).isFIFO(), true, 'the FIFO stays'); }); + +// --------------------------------------------------------------------------- +// The fix round of the third review (af-ws-fix-r2) +// --------------------------------------------------------------------------- + +test("a write over a symlink the mount doesn't follow is told to fix it, with no force", async (t) => { + const { dir, call } = setup(t, { autoMaterialize: true }); + writeFileSync(join(dir, 'target.txt'), 'target\n'); + symlinkSync('target.txt', join(dir, 'l.txt')); + assert.equal(errorOf(await call('write', { path: 'work/l.txt', content: 'x\n' })), 'The write of "work/l.txt" is stored in the workspace, so don\'t send it again. ' + + "Its push to disk didn't complete: cannot verify disk copy (a symlink, which this mount does not follow) — fix it and materialize again."); + assert.equal(lstatSync(join(dir, 'l.txt')).isSymbolicLink(), true, 'the link stays'); + assert.equal(readFileSync(join(dir, 'target.txt'), 'utf8'), 'target\n'); +}); + +test('a disk copy the workspace cannot read is offered force on the condition force needs', async (t) => { + const { dir, call } = setup(t, { autoMaterialize: true }); + for (const name of ['r.txt', 'n.txt', 's.txt']) writeFileSync(join(dir, name), 'v1\n'); + for (const name of ['r.txt', 'n.txt', 's.txt']) assert.equal((await call('read', { path: `work/${name}` })).success, true); + chmodSync(join(dir, 'r.txt'), 0o200); + chmodSync(join(dir, 'n.txt'), 0o000); + chmodSync(join(dir, 's.txt'), 0o200); + t.after(() => { for (const name of ['r.txt', 'n.txt']) if (existsSync(join(dir, name))) chmodSync(join(dir, name), 0o600); }); + try { + readFileSync(join(dir, 'r.txt')); + t.skip('this user can read a mode-0200 file (Windows or root)'); + return; + } catch { /* unreadable, as intended */ } + + const writeRefusal = (path: string): string => `The write of "work/${path}" is stored in the workspace, so don't send it again. ` + + "Its push to disk didn't complete: cannot verify disk copy (cannot open (EACCES)) — fix it and materialize again, " + + 'or, if disk lets the workspace write the file, materialize it with force to overwrite it.'; + assert.equal(errorOf(await call('write', { path: 'work/r.txt', content: 'v2\n' })), writeRefusal('r.txt')); + assert.equal(errorOf(await call('write', { path: 'work/n.txt', content: 'v2\n' })), writeRefusal('n.txt')); + // Force gets past the read where disk lets the file be written, and not + // where it doesn't, which is why the offer carries its condition. + const forced = await call('materialize', { force: true }); + assert.equal(forced.success, true, forced.error); + assert.match(JSON.stringify(forced.data), /n\.txt: not written: [^"]*EACCES/); + chmodSync(join(dir, 'r.txt'), 0o600); + chmodSync(join(dir, 'n.txt'), 0o600); + assert.equal(readFileSync(join(dir, 'r.txt'), 'utf8'), 'v2\n'); + assert.equal(readFileSync(join(dir, 'n.txt'), 'utf8'), 'v1\n'); + + // A deletion's forced unlink needs its directory, not the file's own + // permissions: this one unlinks it. + assert.equal(errorOf(await call('delete', { path: 'work/s.txt' })), 'The deletion of "work/s.txt" is stored in the workspace, so don\'t send it again. ' + + "Its push to disk didn't complete: cannot verify disk copy (cannot open (EACCES)) — fix it and materialize again with applyDeletions, " + + 'or, if disk lets the workspace unlink the file, materialize it with force and applyDeletions to delete it.'); + const unlinked = await call('materialize', { path: 'work/s.txt', force: true, applyDeletions: true }); + assert.equal(unlinked.success, true, unlinked.error); + assert.equal(existsSync(join(dir, 's.txt')), false, 'the forced materialize unlinked it'); +}); + +test("a path whose branch intent can't be read is told to sync the path, which replaces the intent", async (t) => { + const { store, dir, call } = setup(t); + writeFileSync(join(dir, 'u.txt'), 'v1\n'); + assert.equal((await call('read', { path: 'work/u.txt' })).success, true); + const junk = Buffer.from('not an intent'); + store.treeSet('workspace/work/intent', 'u.txt', { blobHash: store.storeBlob(junk, 'text/plain'), size: junk.byteLength, mode: 0o644 }); + const refused = await call('materialize', { path: 'work/u.txt' }); + assert.match(JSON.stringify(refused.data), + /u\.txt: its branch intent can't be read: its record is not a branch intent — sync this path to adopt the disk version, which replaces the intent/); + const synced = await call('sync', { path: 'work/u.txt' }); + assert.equal(synced.success, true, synced.error); + const again = await call('materialize', { path: 'work/u.txt' }); + assert.equal(again.success, true, again.error); + assert.doesNotMatch(JSON.stringify(again.data), /branch intent/, 'the sync replaced it'); +}); + +for (const autoMaterialize of [false, true]) { + test(`an edit of a file disk lacks, with nothing to say whether disk deleted it, says so, ${autoMaterialize ? 'and its push writes it back' : 'and a materialize writes it back'}`, async (t) => { + const { store, call, disk } = setup(t, { autoMaterialize }); + // An entry from before the evidence (as three-way's legacyEntry makes it). + const bytes = Buffer.from('legacy line\n'); + store.treeSet('workspace/work/tree', 'l.txt', { blobHash: store.storeBlob(bytes, 'text/plain'), size: bytes.byteLength, mode: 0o644 }); + const edit = await call('edit', { path: 'work/l.txt', oldString: 'legacy', newString: 'edited' }); + assert.equal(edit.success, true, edit.error); + assert.equal((edit.data as { note?: string }).note, + "Disk has no copy of this file, and nothing records whether it was deleted there, so the edit applied to the workspace's copy. " + + 'It goes back to disk when the path is materialized; if it was deleted there on purpose, delete it here too.'); + if (!autoMaterialize) { + assert.equal(disk('l.txt'), null, "a mount that doesn't push leaves disk as it was"); + const materialized = await call('materialize', {}); + assert.equal(materialized.success, true, materialized.error); + } + assert.equal(disk('l.txt')!.toString(), 'edited line\n'); + }); +} From 3e8536e3cbb32016aeafeb7618e24046218770c7 Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 15:21:21 -0400 Subject: [PATCH 09/13] fix(workspace): the fourth review's fixes: the missing-copy note per mount, an unreadable intent's sync named for when it can see disk, and the conflict note pinned on each refusal Answers af-ws-fix-r3 (wf-2386, Ogden-2688's review of d10f4ae3..650487e6, 9 issues judged in the review store) and Ogden's own point on the missing-copy note (room-203 #132989). - On a mount that pushes, an edit of a file disk lacks, with nothing recording whether it was deleted there, now says "Its push put the file back on disk": the edit's own push had already done so when the answer came. On one that doesn't push it says "A materialize of the path puts it back on disk". A stored edit whose push didn't complete leaves it to the push's reason. The changelog says which, per mount (I1). - A path whose branch intent can't be read is told to sync the path only where disk's copy was observed. With a disk copy a sync can't see, decide(..., adopt=true) returns unverified with no intent, so the sync left the intent and the same refusal came back. The reason now says disk's copy can't be verified, with the observation's reason, and that a sync replaces the intent once it can be. That names a directory or a symlink at the path first (I5, Ogden's reading). - The conflict note on the refusal of a string found more than once, and on the refusal for a workspace copy that isn't valid UTF-8, is now pinned by tests, as the miss's was (I9). - The comment on the conflict note says what the answers do: an edit's answer names the conflict, applied or refused, and a stored edit that couldn't finish leaves it to the push's reason, which names the conflict, or to ls, which its answer points to (I6's comment; the answers stay as they are). - Tests: the missing-copy note in each mount's own words, with disk holding the file once the push ran; an unreadable intent beside a symlink the mount doesn't follow, which a sync leaves until the link is gone; the found-twice refusal's conflict note; and the refusal for a copy that isn't valid UTF-8, on an entry from before the evidence beside a different disk copy (unknown-provenance). Not taken: I2 (not-real: only a write meets "a parent path is not a directory"), I3 and I8 (intended), I4, I6's change to the answers and I7 (not-worth), as the verdicts say. Runs, J16806 (on-vps 20261017T191929Z-bddf661a), on this tree before the commit: the ten workspace test files pass, workspace-file-tools 34/34 with none skipped, workspace-three-way 210/210 and workspace-materialize-freshness 9/9 among them; this round's 7 removals (R56 to R58, R61 to R64) are each caught; and red-first, all 34 cases fail against main's index.ts and reconcile.ts. --- changelog.d/fix-workspace-file-tools.fixed.md | 4 +-- src/modules/workspace/index.ts | 20 +++++++---- src/modules/workspace/reconcile.ts | 15 +++++--- test/workspace-file-tools.test.ts | 35 +++++++++++++++++-- 4 files changed, 58 insertions(+), 16 deletions(-) diff --git a/changelog.d/fix-workspace-file-tools.fixed.md b/changelog.d/fix-workspace-file-tools.fixed.md index 439748583..52e51cfea 100644 --- a/changelog.d/fix-workspace-file-tools.fixed.md +++ b/changelog.d/fix-workspace-file-tools.fixed.md @@ -5,7 +5,7 @@ - a lone UTF-16 surrogate in `newString`, which was written as U+FFFD. One in `oldString`, which used to miss, is refused the same way. `write` now refuses one in `content` too, as every tool does in a path. - Every workspace tool checks its input against its own schema before it does anything: required fields, strings, booleans, and whole numbers, with `read`'s `offset` and `limit` at least 1 and `grep`'s context lines at least 0. An edit missing `newString` wrote the word "undefined", and `replaceAll: "false"` counted as true. A typed field given as `null`, or a number given as a string, is refused too, as `read` already refused them for `offsetChars` and `limitChars`. Before, some handlers read `null` as the field's default and did arithmetic on a numeric string. `read`'s `offset` and `limit`, and `grep`'s `contextBefore` and `contextAfter`, are typed `integer` in their schemas. - A `write`, `edit` or `delete` that the workspace stored but couldn't finish still answers as an error. That covers one whose push to disk didn't complete, and one that failed after its change was stored. The answer now leads with the fact that the change is stored and mustn't be sent again, then gives the push's reason, which says what disk holds and the way out. All of that is in its text, which is what the model and a `code_execution` script receive of an error. Before, a refused push's answer began "Wrote to Chronicle", "Edited Chronicle" or "Removed from Chronicle", and a failure after the change was stored gave only the error. Either way, an edit sent again could apply twice. -- A refused push's way out names the tool that takes it, `materialize it with force`, where it said `pass force`. For a deletion it adds `applyDeletions`, which materialize needs to unlink anything. A disk copy that can't be verified is told to fix it and materialize again. `force` is named beside that only where a forced push gets past what kept the copy from being verified. A file its permissions keep the workspace from reading is offered force if disk lets the workspace write it, or, for a deletion, unlink it, since a read's refusal doesn't show either. For a deletion, something that isn't a regular file is offered force as it is. Before, force was named for every such copy, though a forced push leaves a symlink the mount doesn't follow, or a FIFO a write meets, as it is. A path whose branch intent can't be read is told to sync the path, which replaces the intent. Before, it was told to fix it and materialize again, which is refused the same way until a sync does. `ls`'s description names `applyDeletions` beside `force` for a path the workspace deleted. +- A refused push's way out names the tool that takes it, `materialize it with force`, where it said `pass force`. For a deletion it adds `applyDeletions`, which materialize needs to unlink anything. A disk copy that can't be verified is told to fix it and materialize again. `force` is named beside that only where a forced push gets past what kept the copy from being verified. A file its permissions keep the workspace from reading is offered force if disk lets the workspace write it, or, for a deletion, unlink it, since a read's refusal doesn't show either. For a deletion, something that isn't a regular file is offered force as it is. Before, force was named for every such copy, though a forced push leaves a symlink the mount doesn't follow, or a FIFO a write meets, as it is. A path whose branch intent can't be read is told to sync the path, which replaces the intent. Where disk's copy can't be verified either, it's told that first, since a sync replaces the intent only once it can see disk's copy. Before, it was told to fix it and materialize again, which is refused the same way until a sync does. `ls`'s description names `applyDeletions` beside `force` for a path the workspace deleted. - `read` counts the lines a file has: a final line end no longer starts an empty extra line, so "a\nb\n" is two lines, and an empty file has none. `grep` numbers lines the same way. An `offset` past the last line answers empty `content` with the file's `totalLines`, no `fromLine` or `toLine`, and a note saying where the file ends. Before, it claimed a range that ended before it began. Reading an empty file from line 1 answers `totalLines: 0` with no note. -- An `edit` first takes in a change on disk to a file the workspace holds, as a listing does, and applies to that text. Its answer says so when disk's copy had changed since the workspace last agreed with it, when disk had deleted the file, or when disk's copy couldn't be checked and it used the workspace's copy. Before, it edited the older stored copy, and on a mount that pushes, its push then failed as stale. When disk has no copy and nothing records whether it was deleted there, the edit uses the workspace's copy, which goes back to disk when the path is materialized, as before, and its answer now says so. Where the path's stored copy is a draft and disk changed too, the edit applies to the draft as before, and its answer names the conflict and the two ways out, as does an edit that misses there. `write` doesn't look at disk first, since it replaces the whole file; as before, it only settles a pending intent. +- An `edit` first takes in a change on disk to a file the workspace holds, as a listing does, and applies to that text. Its answer says so when disk's copy had changed since the workspace last agreed with it, when disk had deleted the file, or when disk's copy couldn't be checked and it used the workspace's copy. Before, it edited the older stored copy, and on a mount that pushes, its push then failed as stale. When disk has no copy and nothing records whether it was deleted there, the edit uses the workspace's copy, as before. That copy goes back to disk with the edit's own push on a mount that pushes, or when the path is materialized on one that doesn't, and the edit's answer now says which. Where the path's stored copy is a draft and disk changed too, the edit applies to the draft as before, and its answer names the conflict and the two ways out, as does an edit refused there for what it found. `write` doesn't look at disk first, since it replaces the whole file; as before, it only settles a pending intent. - `grep`'s refusal of an invalid pattern gives the parser's reason, and the workspace's refusals no longer begin with "Error: ". A push that disk's shape refuses says what's in the way and the way out: a directory at the path, or a file or a symlink where the path needs a directory. Before, it offered `force`, which can't write or unlink there either, and a file in the way named no way out. diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 6d54d0163..0317bceec 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -2317,16 +2317,14 @@ export class WorkspaceModule implements Module { ? `Disk has no copy of this file, and nothing records whether it was deleted there, so the edit ${verb} the workspace's copy.` : null; // A mount that doesn't push keeps a conflict until materialize or - // sync settles it, so the edit says so, whether or not it applied. + // sync settles it, so an edit's answer says so, applied or refused. A + // stored edit that couldn't finish leaves it to the push's reason, + // which names the conflict, or to ls, which its answer points to. const conflict = report?.state === 'conflict' ? report.conflict?.kind ?? 'both-changed' : null; const conflictNote = (where: string): string | null => conflict === null ? null : `The path is in conflict with disk (${conflict}): the edit ${where} the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's.`; const suffix = [whichCopy('looked in'), conflictNote('looked in')].filter((n) => n !== null).map((n) => ` ${n}`).join(''); - // The edit makes the path the workspace's own change (commitToolChange), - // so a file disk lacks goes back to disk with the next push of it. - appliedNote = report?.state === 'disk-missing-provenance-unknown' - ? `${whichCopy('applied to')} It goes back to disk when the path is materialized; if it was deleted there on purpose, delete it here too.` - : whichCopy('applied to'); + appliedNote = whichCopy('applied to'); const entry = store.treeGet(mount.treeStateId, relativePath); if (!entry) { @@ -2385,7 +2383,15 @@ export class WorkspaceModule implements Module { }, () => { committed = true; }); if (refused !== null) return storedButUnfinished('edit', input.path, { refused }, appliedNote); - const notes = [appliedNote, conflictNote('is in')].filter((n): n is string => n !== null); + // The edit makes the path the workspace's own change (commitToolChange), + // so a file disk lacked goes back with this edit's push, or with the + // next materialize where the mount doesn't push. A push that didn't + // complete answers above, with its reason. + const pushes = mount.config.autoMaterialize && mount.config.mode !== 'read-only'; + const backOnDisk = report?.state === 'disk-missing-provenance-unknown' + ? `${pushes ? 'Its push put the file back on disk' : 'A materialize of the path puts it back on disk'}; if it was deleted there on purpose, delete it here too.` + : null; + const notes = [appliedNote, backOnDisk, conflictNote('is in')].filter((n): n is string => n !== null); return { success: true, data: { diff --git a/src/modules/workspace/reconcile.ts b/src/modules/workspace/reconcile.ts index 217d5225b..8f2ab7d79 100644 --- a/src/modules/workspace/reconcile.ts +++ b/src/modules/workspace/reconcile.ts @@ -1110,14 +1110,19 @@ export async function pushPaths( } if (v.state === 'unverified') { if (!opts.force) { + // An intent that can't be read is the one note fixing doesn't + // answer: nothing is decided over it until a sync of the path + // replaces it (decide()), and a sync replaces it only where it sees + // disk's copy, so a copy it can't see is named first. For the rest, + // fixing what the note names holds, and forcedWay says where force + // does too. + const sync = 'sync this path to adopt the disk version, which replaces the intent'; result.skipped.push({ path, - // An intent that can't be read is the one note fixing doesn't - // answer: nothing is decided over it until a sync of the path - // replaces it (decide()). For the rest, fixing what the note names - // holds, and forcedWay says where force does too. reason: bi?.unreadable !== undefined - ? `${v.note} — sync this path to adopt the disk version, which replaces the intent` + ? d.kind === 'absent' || d.kind === 'file' + ? `${v.note} — ${sync}` + : `${v.note}, and disk's copy can't be verified (${d.kind === 'unobserved' ? d.reason : 'not hashed'}) — once it can be, ${sync}` : inTheWay(v.note, !s) ?? `cannot verify disk copy (${v.note ?? 'not observed'}) — fix it and ${again}${forcedWay(v.note, !s, forced)}`, }); diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts index ae388c40c..943a03126 100644 --- a/test/workspace-file-tools.test.ts +++ b/test/workspace-file-tools.test.ts @@ -266,6 +266,8 @@ test('an edit of a draft whose disk copy also changed applies to the draft and n // A miss says so too: the text the agent sought may be disk's. assert.equal(errorOf(await call('edit', { path: 'work/k.txt', oldString: 'another', newString: 'x' })), "String not found in work/k.txt. The path is in conflict with disk (both-changed): the edit looked in the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's."); + assert.equal(errorOf(await call('edit', { path: 'work/k.txt', oldString: 'a', newString: 'x' })), + "String found 2 times in work/k.txt. Use replaceAll: true or provide more context. The path is in conflict with disk (both-changed): the edit looked in the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's."); const edit = await call('edit', { path: 'work/k.txt', oldString: 'draft', newString: 'draft, edited' }); assert.equal(edit.success, true, edit.error); assert.equal((edit.data as { note: string }).note, @@ -274,6 +276,17 @@ test('an edit of a draft whose disk copy also changed applies to the draft and n assert.equal(disk('k.txt')!.toString(), 'another writer\n'); }); +test("an edit refused for a workspace copy that isn't valid UTF-8 names the path's conflict too", async (t) => { + const { store, dir, call } = setup(t); + // An entry from before the evidence, beside a disk copy that differs: + // nothing records which is newer. + const latin1 = Buffer.from('caf\xe9\n', 'latin1'); + store.treeSet('workspace/work/tree', 'c.txt', { blobHash: store.storeBlob(latin1, 'text/plain'), size: latin1.byteLength, mode: 0o644 }); + writeFileSync(join(dir, 'c.txt'), 'disk copy\n'); + assert.match(errorOf(await call('edit', { path: 'work/c.txt', oldString: 'caf', newString: 'tea' })), + /^The edit did nothing: work\/c\.txt isn't valid UTF-8, .* The path is in conflict with disk \(unknown-provenance\): the edit looked in the workspace's copy\. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's\.$/); +}); + // --------------------------------------------------------------------------- // Part 5: smaller texts // --------------------------------------------------------------------------- @@ -530,7 +543,7 @@ test('a disk copy the workspace cannot read is offered force on the condition fo assert.equal(existsSync(join(dir, 's.txt')), false, 'the forced materialize unlinked it'); }); -test("a path whose branch intent can't be read is told to sync the path, which replaces the intent", async (t) => { +test("a path whose branch intent can't be read is told to sync the path, which replaces the intent once it sees disk's copy", async (t) => { const { store, dir, call } = setup(t); writeFileSync(join(dir, 'u.txt'), 'v1\n'); assert.equal((await call('read', { path: 'work/u.txt' })).success, true); @@ -544,6 +557,22 @@ test("a path whose branch intent can't be read is told to sync the path, which r const again = await call('materialize', { path: 'work/u.txt' }); assert.equal(again.success, true, again.error); assert.doesNotMatch(JSON.stringify(again.data), /branch intent/, 'the sync replaced it'); + + // While disk holds what the mount can't see, a sync leaves the intent, so + // the refusal names what disk holds first. + writeFileSync(join(dir, 'w.txt'), 'v1\n'); + assert.equal((await call('read', { path: 'work/w.txt' })).success, true); + store.treeSet('workspace/work/intent', 'w.txt', { blobHash: store.storeBlob(junk, 'text/plain'), size: junk.byteLength, mode: 0o644 }); + rmSync(join(dir, 'w.txt')); + symlinkSync('u.txt', join(dir, 'w.txt')); + const unseen = /w\.txt: its branch intent can't be read: its record is not a branch intent, and disk's copy can't be verified \(a symlink, which this mount does not follow\) — once it can be, sync this path to adopt the disk version, which replaces the intent/; + assert.match(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), unseen); + await call('sync', { path: 'work/w.txt' }); + assert.match(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), unseen, "a sync that can't see disk's copy leaves the intent"); + rmSync(join(dir, 'w.txt')); + const seen = await call('sync', { path: 'work/w.txt' }); + assert.equal(seen.success, true, seen.error); + assert.doesNotMatch(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), /branch intent/, 'once it sees disk, the sync replaces it'); }); for (const autoMaterialize of [false, true]) { @@ -556,7 +585,9 @@ for (const autoMaterialize of [false, true]) { assert.equal(edit.success, true, edit.error); assert.equal((edit.data as { note?: string }).note, "Disk has no copy of this file, and nothing records whether it was deleted there, so the edit applied to the workspace's copy. " - + 'It goes back to disk when the path is materialized; if it was deleted there on purpose, delete it here too.'); + + (autoMaterialize ? 'Its push put the file back on disk' : 'A materialize of the path puts it back on disk') + + '; if it was deleted there on purpose, delete it here too.'); + if (autoMaterialize) assert.equal(disk('l.txt')!.toString(), 'edited line\n', 'the push put it back before the answer'); if (!autoMaterialize) { assert.equal(disk('l.txt'), null, "a mount that doesn't push leaves disk as it was"); const materialized = await call('materialize', {}); From 99f8c57759ea2829e80b176bcf54d89784ab0a54 Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 15:38:48 -0400 Subject: [PATCH 10/13] test(workspace): pin the fifth review's untested claims, and read the edit's push note from the push's own predicate Answers af-ws-fix-r4 (wf-2399, Ogden-2688's review of 650487e6..3e8536e3, 11 issues judged in the review store), taking the tests and the shared predicate Ogden named (room-203 #133385). - pushesOnCommit is the one predicate for whether a tool's change goes to disk as it commits: commitToolChange's push and the edit's "Its push put the file back on disk" both read it, so the note can't name a push that didn't run (I6). - Tests: an edit of a file disk lacks whose push is refused answers the push's reason and the copy note, with no word of the file going back (I4); an unreadable intent beside an absent disk copy gets the plain sync wording (I2); and the sync that can't see disk's copy is shown reaching its adoption, which met the symlink rather than the intent (I3). - The comment on the conflict note says which refusals carry it: those for the text the edit found (I9). Not taken: I1, I5, I8, I10 and I11 (not-worth or intended), and I7 (not-real), as the verdicts say. Runs, J16841 (on-vps 20261017T193747Z-c8fe9689), on this tree before the commit: the build is clean; the ten workspace test files pass, workspace-file-tools 35/35, workspace-three-way 210/210 and workspace-materialize-freshness 9/9 among them; removals R65 (a refused edit's answer says the file went back) and R66 (an absent copy gets the can't-be-verified wording) are each caught, and R61 and R62, on the note's mount condition and the unreadable intent's branch, still are; and red-first, all 35 cases fail against main's index.ts and reconcile.ts. --- src/modules/workspace/index.ts | 19 ++++++++++++++----- test/workspace-file-tools.test.ts | 23 ++++++++++++++++++++++- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 0317bceec..3b6f8cff9 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -1948,6 +1948,14 @@ export class WorkspaceModule implements Module { } } + /** + * Whether a tool's change on this mount goes to disk as it commits: the + * push commitToolChange makes, and what an edit's answer says of it. + */ + private pushesOnCommit(mount: MountState): boolean { + return mount.config.autoMaterialize === true && mount.config.mode !== 'read-only'; + } + /** * Commit a tool's write or delete to the workspace tree, with the branch * intent it implies (store origin for a path disk never agreed with; a @@ -1999,7 +2007,7 @@ export class WorkspaceModule implements Module { return next; }); } - if (!mount.config.autoMaterialize || mount.config.mode === 'read-only') return null; + if (!this.pushesOnCommit(mount)) return null; const pushed = await this.pushUnlocked(mount, [relativePath], { applyDeletions: change.kind === 'remove', @@ -2317,9 +2325,10 @@ export class WorkspaceModule implements Module { ? `Disk has no copy of this file, and nothing records whether it was deleted there, so the edit ${verb} the workspace's copy.` : null; // A mount that doesn't push keeps a conflict until materialize or - // sync settles it, so an edit's answer says so, applied or refused. A - // stored edit that couldn't finish leaves it to the push's reason, - // which names the conflict, or to ls, which its answer points to. + // sync settles it, so an edit's answer says so, whether it applied or + // was refused for the text it found. A stored edit that couldn't + // finish leaves it to the push's reason, which names the conflict, or + // to ls, which its answer points to. const conflict = report?.state === 'conflict' ? report.conflict?.kind ?? 'both-changed' : null; const conflictNote = (where: string): string | null => conflict === null ? null : `The path is in conflict with disk (${conflict}): the edit ${where} the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's.`; @@ -2387,7 +2396,7 @@ export class WorkspaceModule implements Module { // so a file disk lacked goes back with this edit's push, or with the // next materialize where the mount doesn't push. A push that didn't // complete answers above, with its reason. - const pushes = mount.config.autoMaterialize && mount.config.mode !== 'read-only'; + const pushes = this.pushesOnCommit(mount); const backOnDisk = report?.state === 'disk-missing-provenance-unknown' ? `${pushes ? 'Its push put the file back on disk' : 'A materialize of the path puts it back on disk'}; if it was deleted there on purpose, delete it here too.` : null; diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts index 943a03126..847bef464 100644 --- a/test/workspace-file-tools.test.ts +++ b/test/workspace-file-tools.test.ts @@ -567,9 +567,16 @@ test("a path whose branch intent can't be read is told to sync the path, which r symlinkSync('u.txt', join(dir, 'w.txt')); const unseen = /w\.txt: its branch intent can't be read: its record is not a branch intent, and disk's copy can't be verified \(a symlink, which this mount does not follow\) — once it can be, sync this path to adopt the disk version, which replaces the intent/; assert.match(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), unseen); - await call('sync', { path: 'work/w.txt' }); + const blind = await call('sync', { path: 'work/w.txt' }); + assert.equal(blind.success, true, blind.error); + // Its adoption met the symlink, not the intent: the sync reached the verdict + // that would replace the intent, and couldn't. + assert.match(JSON.stringify(blind.data), /"path":"w\.txt","reason":"disk could not be checked: a symlink, which this mount does not follow"/); assert.match(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), unseen, "a sync that can't see disk's copy leaves the intent"); rmSync(join(dir, 'w.txt')); + // Absent is a copy a sync sees, so the reason names the sync alone. + assert.match(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), + /w\.txt: its branch intent can't be read: its record is not a branch intent — sync this path to adopt the disk version, which replaces the intent/); const seen = await call('sync', { path: 'work/w.txt' }); assert.equal(seen.success, true, seen.error); assert.doesNotMatch(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), /branch intent/, 'once it sees disk, the sync replaces it'); @@ -596,3 +603,17 @@ for (const autoMaterialize of [false, true]) { assert.equal(disk('l.txt')!.toString(), 'edited line\n'); }); } + +test("an edit of a file disk lacks whose push is refused doesn't say the file went back", async (t) => { + const { store, dir, call, stored } = setup(t, { autoMaterialize: true }); + // An entry from before the evidence, below a path disk holds as a file, so + // the edit's push can't write it. + const bytes = Buffer.from('legacy line\n'); + store.treeSet('workspace/work/tree', 'f.txt/l.txt', { blobHash: store.storeBlob(bytes, 'text/plain'), size: bytes.byteLength, mode: 0o644 }); + writeFileSync(join(dir, 'f.txt'), 'file\n'); + assert.equal(errorOf(await call('edit', { path: 'work/f.txt/l.txt', oldString: 'legacy', newString: 'edited' })), + 'The edit of "work/f.txt/l.txt" is stored in the workspace, so don\'t send it again: a second edit can apply the change twice. ' + + "Its push to disk didn't complete: not written: disk has a file or a symlink where this path needs a directory, and no push writes below one, forced or not — move or remove it on disk and materialize again, or write the file under another path. " + + "Disk has no copy of this file, and nothing records whether it was deleted there, so the edit applied to the workspace's copy."); + assert.equal(await stored('f.txt/l.txt'), 'edited line\n'); +}); From 2c8c9d7df897d28326aa63123d984344edb31969 Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 15:58:38 -0400 Subject: [PATCH 11/13] test(workspace): end the unreadable intent's assertions at the reason's end, check disk after a refused push, and say what the blind sync shows Answers af-ws-fix-r5 (wf-2410, Ogden-2688's review of 3e8536e3..99f8c577, 9 issues judged in the review store), taking the cheap tests Ogden named (room-203 #133681). - The three unreadable-intent assertions end at the reason's closing quote, so a way out added after the sync, such as force, fails them (I2). - The comment on the blind sync says what its assertion shows: a sync asks for an adoption, which gets past the intent's check and stops at the symlink before anything is adopted, so its note is the symlink's where a pass that adopts nothing answers the intent's (I5). - The refused-push test checks that disk's file is as it was (I8). Not taken: I1 and I7 (not-worth), I3, I4 and I9 (intended), and I6 (not-real), as the verdicts say. What I3 and I4 change for a reader goes into the description: pushesOnCommit's read-only clause is main's guard, which nothing reaches, and autoMaterialize now pushes only when it is true itself, as connectome-host reads it. Runs, J16898 (on-vps 20261017T195715Z-f871fc0f), on this tree before the commit: the build is clean; the ten workspace test files pass, workspace-file-tools 35/35, workspace-three-way 210/210 and workspace-materialize-freshness 9/9 among them; removals R67 and R68, which add force after the sync in each arm of the unreadable intent's reason, are each caught, and R65 and R66 still are; and red-first, all 35 cases fail against main's index.ts and reconcile.ts. --- test/workspace-file-tools.test.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts index 847bef464..a39f7eaf7 100644 --- a/test/workspace-file-tools.test.ts +++ b/test/workspace-file-tools.test.ts @@ -551,7 +551,7 @@ test("a path whose branch intent can't be read is told to sync the path, which r store.treeSet('workspace/work/intent', 'u.txt', { blobHash: store.storeBlob(junk, 'text/plain'), size: junk.byteLength, mode: 0o644 }); const refused = await call('materialize', { path: 'work/u.txt' }); assert.match(JSON.stringify(refused.data), - /u\.txt: its branch intent can't be read: its record is not a branch intent — sync this path to adopt the disk version, which replaces the intent/); + /u\.txt: its branch intent can't be read: its record is not a branch intent — sync this path to adopt the disk version, which replaces the intent"/); const synced = await call('sync', { path: 'work/u.txt' }); assert.equal(synced.success, true, synced.error); const again = await call('materialize', { path: 'work/u.txt' }); @@ -565,18 +565,19 @@ test("a path whose branch intent can't be read is told to sync the path, which r store.treeSet('workspace/work/intent', 'w.txt', { blobHash: store.storeBlob(junk, 'text/plain'), size: junk.byteLength, mode: 0o644 }); rmSync(join(dir, 'w.txt')); symlinkSync('u.txt', join(dir, 'w.txt')); - const unseen = /w\.txt: its branch intent can't be read: its record is not a branch intent, and disk's copy can't be verified \(a symlink, which this mount does not follow\) — once it can be, sync this path to adopt the disk version, which replaces the intent/; + const unseen = /w\.txt: its branch intent can't be read: its record is not a branch intent, and disk's copy can't be verified \(a symlink, which this mount does not follow\) — once it can be, sync this path to adopt the disk version, which replaces the intent"/; assert.match(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), unseen); const blind = await call('sync', { path: 'work/w.txt' }); assert.equal(blind.success, true, blind.error); - // Its adoption met the symlink, not the intent: the sync reached the verdict - // that would replace the intent, and couldn't. + // A sync asks for an adoption, which gets past the intent's check and stops + // at the symlink, before anything is adopted: its note is the symlink's, + // where a pass that adopts nothing answers the intent's. assert.match(JSON.stringify(blind.data), /"path":"w\.txt","reason":"disk could not be checked: a symlink, which this mount does not follow"/); assert.match(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), unseen, "a sync that can't see disk's copy leaves the intent"); rmSync(join(dir, 'w.txt')); // Absent is a copy a sync sees, so the reason names the sync alone. assert.match(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), - /w\.txt: its branch intent can't be read: its record is not a branch intent — sync this path to adopt the disk version, which replaces the intent/); + /w\.txt: its branch intent can't be read: its record is not a branch intent — sync this path to adopt the disk version, which replaces the intent"/); const seen = await call('sync', { path: 'work/w.txt' }); assert.equal(seen.success, true, seen.error); assert.doesNotMatch(JSON.stringify((await call('materialize', { path: 'work/w.txt' })).data), /branch intent/, 'once it sees disk, the sync replaces it'); @@ -605,7 +606,7 @@ for (const autoMaterialize of [false, true]) { } test("an edit of a file disk lacks whose push is refused doesn't say the file went back", async (t) => { - const { store, dir, call, stored } = setup(t, { autoMaterialize: true }); + const { store, dir, call, disk, stored } = setup(t, { autoMaterialize: true }); // An entry from before the evidence, below a path disk holds as a file, so // the edit's push can't write it. const bytes = Buffer.from('legacy line\n'); @@ -616,4 +617,5 @@ test("an edit of a file disk lacks whose push is refused doesn't say the file we + "Its push to disk didn't complete: not written: disk has a file or a symlink where this path needs a directory, and no push writes below one, forced or not — move or remove it on disk and materialize again, or write the file under another path. " + "Disk has no copy of this file, and nothing records whether it was deleted there, so the edit applied to the workspace's copy."); assert.equal(await stored('f.txt/l.txt'), 'edited line\n'); + assert.equal(disk('f.txt')!.toString(), 'file\n', "the refused push left disk's file as it was"); }); From 83cc65ea65874a7acddd68398b02b9a1156a409d Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 16:32:34 -0400 Subject: [PATCH 12/13] fix(workspace): a pushing mount's edit names no conflict its push settled, replaceAll checks for its text without splitting, and the FIFO tests skip on Windows Answers Greptile's review of #456 (three findings, all taken). - On a mount that pushes, an edit whose push completed met no conflict, since the push isn't forced and stops at one. If the edit made the workspace's copy agree with disk, the push settles the conflict, and the success answer no longer names it with its two ways out. A mount that doesn't push still names the conflict the pass found. The pushing-mount conflict test now makes the draft agree with disk and checks that the answer has no note and that a materialize meets no conflict. - With replaceAll, the edit learns whether its text occurs with includes. Without replaceAll it still splits to count, since its refusal says how often the text occurs. - The two FIFO tests skip on Windows with "no FIFOs", as workspace-three-way's FIFO test does. Runs on this tree before the commit, Linux, Node 24.22.0: the eleven workspace test files pass, workspace-file-tools 35/35; removing the mount condition from the success note fails one case, and so does letting a replaceAll that matches nothing succeed; and all 35 cases fail against main 3af13bf4's index.ts and reconcile.ts. --- src/modules/workspace/index.ts | 14 ++++++++++---- test/workspace-file-tools.test.ts | 15 +++++++++++++-- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 6f2068bd8..a4410f522 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -2380,8 +2380,11 @@ export class WorkspaceModule implements Module { let content = blob.toString('utf-8'); // An edit that would replace nothing is refused, replaceAll or not; - // without replaceAll, the text must occur once. - const count = content.split(input.oldString).length - 1; + // without replaceAll, the text must occur once, and the refusal says + // how often it does. With replaceAll, whether it occurs is enough. + const count = input.replaceAll + ? Number(content.includes(input.oldString)) + : content.split(input.oldString).length - 1; if (count === 0) { return { success: false, error: `String not found in ${input.path}.${suffix}`, isError: true }; } @@ -2412,12 +2415,15 @@ export class WorkspaceModule implements Module { // The edit makes the path the workspace's own change (commitToolChange), // so a file disk lacked goes back with this edit's push, or with the // next materialize where the mount doesn't push. A push that didn't - // complete answers above, with its reason. + // complete answers above, with its reason. One that completed met no + // conflict, since this push, unforced, stops at one: the edit made the + // two copies agree, which settles it, or there was none. So only a + // mount that doesn't push still holds the conflict the pass found. const pushes = this.pushesOnCommit(mount); const backOnDisk = report?.state === 'disk-missing-provenance-unknown' ? `${pushes ? 'Its push put the file back on disk' : 'A materialize of the path puts it back on disk'}; if it was deleted there on purpose, delete it here too.` : null; - const notes = [appliedNote, backOnDisk, conflictNote('is in')].filter((n): n is string => n !== null); + const notes = [appliedNote, backOnDisk, pushes ? null : conflictNote('is in')].filter((n): n is string => n !== null); return { success: true, data: { diff --git a/test/workspace-file-tools.test.ts b/test/workspace-file-tools.test.ts index a39f7eaf7..381e6a103 100644 --- a/test/workspace-file-tools.test.ts +++ b/test/workspace-file-tools.test.ts @@ -410,6 +410,17 @@ test('an edit of a draft whose disk copy also changed, on a mount that pushes, n + 'sync this path to adopt the disk version, or materialize it with force to overwrite it.'); assert.equal(await stored('k.txt'), 'agent draft, edited\n'); assert.equal(disk('k.txt')!.toString(), 'another writer\n'); + + // An edit that makes the draft agree with disk settles the conflict as its + // push meets it, so its answer names none. + const agreed = await call('edit', { path: 'work/k.txt', oldString: 'agent draft, edited', newString: 'another writer' }); + assert.equal(agreed.success, true, agreed.error); + assert.equal((agreed.data as { note?: string }).note, undefined, 'no conflict remains to name'); + assert.equal(await stored('k.txt'), 'another writer\n'); + assert.equal(disk('k.txt')!.toString(), 'another writer\n'); + const after = await call('materialize', { path: 'work/k.txt' }); + assert.equal(after.success, true, after.error); + assert.doesNotMatch(JSON.stringify(after.data), /conflict/, 'and a materialize meets none'); }); test('an edit that fails after its change is stored, having taken disk\'s change in, says both', async (t) => { @@ -431,7 +442,7 @@ test('an edit that fails after its change is stored, having taken disk\'s change assert.equal(await stored('e.txt'), 'v2 LINE from another writer\n'); }); -test('a deletion whose disk copy is not a regular file names the materialize that unlinks it', async (t) => { +test('a deletion whose disk copy is not a regular file names the materialize that unlinks it', { skip: process.platform === 'win32' ? 'no FIFOs' : false }, async (t) => { const { dir, call } = setup(t, { autoMaterialize: true }); writeFileSync(join(dir, 'q.txt'), 'v1\n'); assert.equal((await call('read', { path: 'work/q.txt' })).success, true); @@ -482,7 +493,7 @@ test("a forced deletion over a directory says no push unlinks one", async (t) => assert.equal(statSync(join(dir, 'gone.txt')).isDirectory(), true, 'the directory stays'); }); -test("a write over a FIFO is told to fix it and materialize again, with no force", async (t) => { +test("a write over a FIFO is told to fix it and materialize again, with no force", { skip: process.platform === 'win32' ? 'no FIFOs' : false }, async (t) => { const { dir, call } = setup(t, { autoMaterialize: true }); assert.equal(spawnSync('mkfifo', [join(dir, 'p.txt')]).status, 0, 'mkfifo'); assert.equal(errorOf(await call('write', { path: 'work/p.txt', content: 'x\n' })), 'The write of "work/p.txt" is stored in the workspace, so don\'t send it again. ' From e57e48a9f344b1e35b0e201f048368d5b58747d8 Mon Sep 17 00:00:00 2001 From: Phoebe-2419 Date: Sat, 17 Oct 2026 16:34:43 -0400 Subject: [PATCH 13/13] docs(workspace): the comment on the edit's conflict note says where the note goes A refusal for the text the edit found names the conflict on either kind of mount, and a success names it only where the mount doesn't push. The comment had read as if the note were for a mount that doesn't push. Ogden-2688 pointed it out in his read of 83cc65ea. Comment only. --- src/modules/workspace/index.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index a4410f522..64880421d 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -2341,11 +2341,12 @@ export class WorkspaceModule implements Module { : report?.state === 'disk-missing-provenance-unknown' ? `Disk has no copy of this file, and nothing records whether it was deleted there, so the edit ${verb} the workspace's copy.` : null; - // A mount that doesn't push keeps a conflict until materialize or - // sync settles it, so an edit's answer says so, whether it applied or - // was refused for the text it found. A stored edit that couldn't - // finish leaves it to the push's reason, which names the conflict, or - // to ls, which its answer points to. + // A conflict the pass found is named when the edit is refused for the + // text it found, on either kind of mount, and on a success where the + // mount doesn't push, since such a mount keeps the conflict until + // materialize or sync settles it. A pushing mount's success met none + // (below). A stored edit that couldn't finish leaves it to the push's + // reason, which names the conflict, or to ls, which its answer points to. const conflict = report?.state === 'conflict' ? report.conflict?.kind ?? 'both-changed' : null; const conflictNote = (where: string): string | null => conflict === null ? null : `The path is in conflict with disk (${conflict}): the edit ${where} the workspace's copy. Sync the path to take disk's copy instead, or materialize it with force to write the workspace's over disk's.`;