diff --git a/changelog.d/552-pass-decided-nothing.changed.md b/changelog.d/552-pass-decided-nothing.changed.md new file mode 100644 index 000000000..c0ee3fb42 --- /dev/null +++ b/changelog.d/552-pass-decided-nothing.changed.md @@ -0,0 +1,9 @@ +- The workspace's `edit` and `edit_many` refuse a file the workspace holds + when the check of disk before them decided nothing, because the selected + branch kept changing while it ran. They store nothing, and say so, with + that reason and to make the same call again. Before, they applied to the + workspace's copy without a word, so where disk had changed they edited + text disk had moved past and left the path in conflict, and they committed + on whichever branch was selected when the check gave up. Where disk's copy + couldn't be read, an `edit` or `edit_many` still applies to the + workspace's copy and says so. diff --git a/changelog.d/552-pass-decided-nothing.fixed.md b/changelog.d/552-pass-decided-nothing.fixed.md new file mode 100644 index 000000000..e72c6a37e --- /dev/null +++ b/changelog.d/552-pass-decided-nothing.fixed.md @@ -0,0 +1,9 @@ +- The workspace's `delete`, `edit` and `edit_many`, and its `readBinary` + method, no longer answer "File not found" for a path the workspace doesn't + hold when the check of disk before them decided nothing, because the + selected branch kept changing while it ran. Each now says that disk + couldn't be checked, with that reason, and to make the same call again. + Before, they answered "File not found" even when disk held the file + (#552). `readBinary` also gives `delete`'s answer for every other path the + workspace doesn't hold, such as one whose disk copy couldn't be read, where + it said "File not found" whatever the reason. diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index ea740350e..8be71e792 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -268,6 +268,16 @@ export interface ReadFileFromDiskOptions { maxBytes?: number; } +/** + * Why a pass decided nothing, or null where it decided its paths. A pass gives + * up deciding, and reports no path, when the selected branch kept changing + * while it gathered, or when a sync's branch was no longer selected; it then + * carries no branchId, and the region it leaves incomplete says why. + */ +function undecidedOf(pass: PassResult): string | null { + return pass.branchId === undefined ? (pass.incomplete[0]?.reason ?? 'the pass decided nothing') : null; +} + function errnoOf(err: unknown): string | undefined { return typeof err === 'object' && err !== null && 'code' in err && typeof (err as { code?: unknown }).code === 'string' ? (err as { code: string }).code @@ -2284,9 +2294,9 @@ export class WorkspaceModule implements Module { if (mount.direct) return this.bytesFromDisk(mount, relativePath, mountPrefixedPath, mount.config.maxFileSize ?? DEFAULT_MAX_FILE_SIZE); const store = this.getStore(); return this.withMount(mount, async () => { - await this.ensureSynced(mount, relativePath); + const { report, undecided } = await this.ensureSynced(mount, relativePath); const entry = store.treeGet(mount.treeStateId, relativePath); - if (!entry) return { error: `File not found: ${mountPrefixedPath}` }; + if (!entry) return { error: undecided !== null ? this.notCheckedYet(mountPrefixedPath, undecided) : this.notInWorkspace(mountPrefixedPath, report) }; const blob = store.getBlob(entry.blobHash); if (!blob) return { error: `Blob not found for: ${mountPrefixedPath}` }; return { data: blob }; @@ -3676,13 +3686,24 @@ export class WorkspaceModule implements Module { * path, as a file tool names the one it acts on: a path the mount passes * over by design is observed, as a sync naming it would observe it (#276), * while a path in another mount's directory is refused before this, and - * passed over by any pass. The caller holds the mount's turn. + * passed over by any pass. Answers the pass's report on the path, and why + * the pass decided nothing where it did (undecidedOf), which leaves disk + * unchecked. The caller holds the mount's turn. */ - private async ensureSynced(mount: MountState, relativePath: string): Promise { + private async ensureSynced(mount: MountState, relativePath: string): Promise<{ report: PathReport | undefined; undecided: string | null }> { const store = this.getStore(); - if (store.treeGet(mount.treeStateId, relativePath)) return undefined; + if (store.treeGet(mount.treeStateId, relativePath)) return { report: undefined, undecided: null }; const pass = await this.passUnlocked(mount, { kind: 'paths', paths: [relativePath] }, { named: true }); - return pass.reports.get(relativePath); + return { report: pass.reports.get(relativePath), undecided: undecidedOf(pass) }; + } + + /** + * Why the workspace holds no entry at a path, after a pass over it that + * decided nothing: disk wasn't checked, so it may have a copy, and the same + * call made again checks it. + */ + private notCheckedYet(path: string, reason: string): string { + return `${this.notInWorkspace(path, { path, state: 'unverified', note: reason })} Make the same call again, which checks disk first.`; } /** Why the workspace holds no entry at a path, when disk explains it. */ @@ -3715,7 +3736,7 @@ export class WorkspaceModule implements Module { if (mount.direct) return this.bytesOnDisk(mount, relativePath, path); const store = this.getStore(); return this.withMount(mount, async () => { - const report = await this.ensureSynced(mount, relativePath); + const { report } = await this.ensureSynced(mount, relativePath); const entry = store.treeGet(mount.treeStateId, relativePath); if (!entry) return { error: this.notInWorkspace(path, report) }; const blob = store.getBlob(entry.blobHash); @@ -4086,6 +4107,8 @@ export class WorkspaceModule implements Module { // As every file tool's pass does, it names the paths it acts on, so a // file the mount passes over by design is observed (see PassOptions.named). const pass = await this.passUnlocked(mount, { kind: 'paths', paths: [...targets.values()].map((target) => target.relativePath) }, { named: true }); + // A pass that decided nothing checked no file's disk copy. + const undecided = undecidedOf(pass); // Nothing below awaits until the commit's push. const maxSize = mount.config.maxFileSize ?? DEFAULT_MAX_FILE_SIZE; const hints = hintBudget(); @@ -4109,6 +4132,20 @@ export class WorkspaceModule implements Module { // workspace didn't hold has no older copy, so taking it in needs no // word. const subject = tool === 'edit' ? 'the edit' : editList(target.edits); + // A pass that decided nothing checked no copy on disk, held or not, + // and took nothing in, so the edits would apply to text disk may + // have moved past, on whichever branch is selected once it gave up. + // They're refused, as a call made again can check. + if (undecided !== null) { + problems.push({ + kind: 'file', + edits: target.edits, + path: target.path, + text: `${target.path} couldn't be checked against disk (${undecided}), so ${subject} changed nothing. Make the same call again, which checks disk first.`, + whole: true, + }); + continue; + } const whichCopy = (verb: string): string | null => took('modified') ? `Disk's copy${tool === 'edit' ? '' : ` of ${target.path}`} had changed since the workspace last agreed with it, so it was taken in first, and ${subject} ${verb} that text.` : report?.state === 'unverified' @@ -4378,11 +4415,12 @@ export class WorkspaceModule implements Module { let committed = false; try { return await this.withMount(mount, async (): Promise => { - const report = await this.ensureSynced(mount, relativePath); + const { report, undecided } = 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 error = undecided !== null ? this.notCheckedYet(input.path, undecided) : this.notInWorkspace(input.path, report); + return { success: false, error, isError: true }; } const { refused, note } = await this.commitToolChange(mount, relativePath, { kind: 'remove', previousHash: entry.blobHash }, diff --git a/test/workspace-three-way.test.ts b/test/workspace-three-way.test.ts index 0d8de884d..d20a497c0 100644 --- a/test/workspace-three-way.test.ts +++ b/test/workspace-three-way.test.ts @@ -7028,6 +7028,163 @@ describe('on-agent-action', () => { assert.deepEqual(data.incomplete, [{ path: '', reason: 'the selected branch kept changing while the scan ran' }]); }); + /** + * Run `fn` with the selected branch changing during each of a pass's + * gathers, so the pass decides nothing, and then no more: each file a gather + * reads switches the branch, three times in all, as many as a pass gathers. + */ + const whileBranchKeepsChanging = (env: Env, fn: () => Promise): Promise => { + const main = env.store.currentBranch().name; + env.store.createBranch('other', main); + return withReadHook((n) => { if (n <= 3) env.store.switchBranch(n % 2 === 1 ? 'other' : main); }, fn); + }; + const KEPT_CHANGING = 'the selected branch kept changing while the scan ran'; + /** + * Date a file's mtime past now, so no fingerprint recorded for it vouches + * (fingerprintVouches needs the hash taken past the racy window after the + * change), and every gather of a pass reads it. + */ + const unvouched = (env: Env, path: string) => { + const later = new Date(Date.now() + 60_000); + utimesSync(env.disk(path), later, later); + }; + + test('a file the workspace doesn\'t hold, under a pass that decided nothing, is answered as unchecked by delete, readBinary, edit and edit_many, not as not found', async (t) => { + const answers: Record = {}; + for (const tool of ['delete', 'readBinary', 'edit', 'edit_many'] as const) { + const env = new Env(t); + const m = await env.open(); + env.writeDisk('new.txt', 'hello on disk\n'); + const input = tool === 'delete' ? { path: 'work/new.txt' } + : tool === 'edit' ? { path: 'work/new.txt', oldString: 'hello', newString: 'bye' } + : { edits: [{ path: 'work/new.txt', oldString: 'hello', newString: 'bye' }] }; + answers[tool] = await whileBranchKeepsChanging(env, async () => { + if (tool === 'readBinary') { + const read = await m.readBinary('work/new.txt'); + return 'error' in read ? read.error : `read ${read.data.length} bytes`; + } + const res = await m.handleToolCall({ id: 't', name: tool, input }); + assert.equal(res.success, false, JSON.stringify(res)); + return String(res.error); + }); + // Disk's file is untouched, and the same call, made again, finds it. + assert.equal(env.readDisk('new.txt'), 'hello on disk\n'); + if (tool === 'readBinary') { + const again = await m.readBinary('work/new.txt'); + assert.ok('data' in again && again.data.toString() === 'hello on disk\n', JSON.stringify(again)); + } else { + const again = await m.handleToolCall({ id: 't', name: tool, input }); + assert.equal(again.success, true, `${tool} again: ${JSON.stringify(again)}`); + } + } + const unchecked = `work/new.txt is not in the workspace, and disk could not be checked (${KEPT_CHANGING}). Make the same call again, which checks disk first.`; + assert.deepEqual(answers, { + delete: unchecked, + readBinary: unchecked, + edit: `work/new.txt couldn't be checked against disk (${KEPT_CHANGING}), so the edit changed nothing. Make the same call again, which checks disk first.`, + edit_many: 'The edit_many call stored none of its edits: 1 problem.\n\n' + + `edit 1: work/new.txt couldn't be checked against disk (${KEPT_CHANGING}), so edit 1 changed nothing. Make the same call again, which checks disk first.`, + }); + }); + + for (const autoMaterialize of [true, false]) { + test(`an edit of a file the workspace holds, under a pass that decided nothing, changes nothing on either branch or disk, and says to try again (${autoMaterialize ? 'a mount that pushes' : "one that doesn't push"})`, async (t) => { + for (const disk of ['unchanged', 'changed'] as const) { + const env = new Env(t); + const m = await env.open({ autoMaterialize }); + await seedSynced(env, m, 'a.txt', 'A line\n'); + const main = env.store.currentBranch().name; + if (disk === 'changed') env.writeDisk('a.txt', 'disk says A2\n'); + unvouched(env, 'a.txt'); + const edit = await whileBranchKeepsChanging(env, () => m.handleToolCall({ id: 't', name: 'edit', input: { path: 'work/a.txt', oldString: 'A line', newString: 'A edited' } })); + assert.equal(edit.success, false, JSON.stringify(edit)); + assert.equal(edit.error, `work/a.txt couldn't be checked against disk (${KEPT_CHANGING}), so the edit changed nothing. Make the same call again, which checks disk first.`, disk); + // So does edit_many's, the branch left on `other` by the edit's pass + // changing under its three gathers in turn. + assert.equal(env.store.currentBranch().name, 'other'); + const many = await withReadHook((n) => { if (n <= 3) env.store.switchBranch(n % 2 === 1 ? main : 'other'); }, + () => m.handleToolCall({ id: 't', name: 'edit_many', input: { edits: [{ path: 'work/a.txt', oldString: 'A line', newString: 'A edited' }] } })); + assert.equal(many.success, false, JSON.stringify(many)); + assert.equal(many.error, 'The edit_many call stored none of its edits: 1 problem.\n\n' + + `edit 1: work/a.txt couldn't be checked against disk (${KEPT_CHANGING}), so edit 1 changed nothing. Make the same call again, which checks disk first.`, disk); + // Disk is as it was, and each branch's workspace copy is the one the + // edits found, read from the store, which no pass changes. + const onDisk = disk === 'changed' ? 'disk says A2\n' : 'A line\n'; + assert.equal(env.readDisk('a.txt'), onDisk); + const treeStateId = (m as any).mounts.get('work').treeStateId as string; + for (const branch of [main, 'other']) { + env.store.switchBranch(branch); + const entry = env.store.treeGet(treeStateId, 'a.txt'); + assert.equal(entry && env.store.getBlob(entry.blobHash)?.toString(), 'A line\n', `${disk} on ${branch}`); + } + // Made again, the edit checks disk: it applies to the text disk holds, + // and where disk changed, that text no longer has the old line. + env.store.switchBranch(main); + const again = await m.handleToolCall({ id: 't', name: 'edit', input: { path: 'work/a.txt', oldString: 'A line', newString: 'A edited' } }); + if (disk === 'unchanged') assert.equal(again.success, true, JSON.stringify(again)); + else assert.match(String(again.error), /^The edit did nothing: oldString isn't in work\/a\.txt\./); + } + }); + } + + test('edit_many under a pass that decided nothing refuses every file it names, held or not, and stores nothing on any branch', async (t) => { + const env = new Env(t); + const m = await env.open({ autoMaterialize: true }); + await seedSynced(env, m, 'a.txt', 'A line\n'); + await seedSynced(env, m, 'b.txt', 'B line\n'); + env.writeDisk('new.txt', 'N line\n'); + for (const path of ['a.txt', 'b.txt', 'new.txt']) unvouched(env, path); + // Each file a gather reads selects a branch made for it, so every gather + // ends on another branch than it began on, however many files it reads. + const branches = [env.store.currentBranch().name]; + const many = await withReadHook((n) => { + const name = `churn-${n}`; + env.store.createBranch(name, env.store.currentBranch().name); + env.store.switchBranch(name); + branches.push(name); + }, () => m.handleToolCall({ id: 't', name: 'edit_many', input: { edits: [ + { path: 'work/a.txt', oldString: 'A line', newString: 'A edited' }, + { path: 'work/b.txt', oldString: 'B line', newString: 'B edited' }, + { path: 'work/new.txt', oldString: 'N line', newString: 'N edited' }, + ] } })); + assert.equal(many.success, false, JSON.stringify(many)); + const refused = (i: number, path: string) => `edit ${i}: work/${path} couldn't be checked against disk (${KEPT_CHANGING}), so edit ${i} changed nothing. Make the same call again, which checks disk first.`; + assert.equal(many.error, ['The edit_many call stored none of its edits: 3 problems.', refused(1, 'a.txt'), refused(2, 'b.txt'), refused(3, 'new.txt')].join('\n\n')); + assert.deepEqual(['a.txt', 'b.txt', 'new.txt'].map((p) => env.readDisk(p)), ['A line\n', 'B line\n', 'N line\n']); + const treeStateId = (m as any).mounts.get('work').treeStateId as string; + for (const branch of branches) { + env.store.switchBranch(branch); + const text = (path: string) => { const entry = env.store.treeGet(treeStateId, path); return entry ? env.store.getBlob(entry.blobHash)?.toString() : null; }; + assert.deepEqual([text('a.txt'), text('b.txt'), text('new.txt')], ['A line\n', 'B line\n', null], branch); + } + }); + + test('readBinary and delete answer alike for a file the workspace doesn\'t hold whose disk copy couldn\'t be read', { skip: IS_ROOT ? 'root reads every file' : false }, async (t) => { + const env = new Env(t); + const m = await env.open(); + env.writeDisk('new.txt', 'hello on disk\n'); + chmodSync(env.disk('new.txt'), 0o000); + const read = await m.readBinary('work/new.txt'); + const deleted = await m.handleToolCall({ id: 't', name: 'delete', input: { path: 'work/new.txt' } }); + assert.ok('error' in read, JSON.stringify(read)); + assert.match(read.error, /^work\/new\.txt is not in the workspace, and disk could not be checked \(.+\)\.$/); + assert.equal(read.error, deleted.error); + }); + + test("an edit under a pass that decided, with a region left incomplete, applies to the workspace's copy and says so, where one that decided nothing refuses", async (t) => { + const env = new Env(t); + const m = await env.open(); + await seedSynced(env, m, 'a.txt', 'A line\n'); + // Another directory where the mount's root was: a pass decides every + // path as disk couldn't check it, and leaves the root's region incomplete. + renameSync(env.dir, join(env.root, 'unmounted')); + mkdirSync(env.dir); + writeFileSync(join(env.dir, 'a.txt'), 'A line\n'); + const edit = await m.handleToolCall({ id: 't', name: 'edit', input: { path: 'work/a.txt', oldString: 'A line', newString: 'A edited' } }); + assert.equal(edit.success, true, JSON.stringify(edit)); + assert.match(JSON.stringify(edit.data), /couldn't be checked against disk \(the mount root is not the directory disk last agreed with/); + }); + test('a branch switched away during the root check and back during a read is decided on the branch enumerated', async (t) => { const env = new Env(t); const m = await env.open();