fix(workspace): a tool whose disk check decided nothing says so and changes nothing

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.

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.

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<PathReport | undefined> {
+  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<ToolResult> => {
-        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 },

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 = <T>(env: Env, fn: () => Promise<T>): Promise<T> => {
+    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<string, string> = {};
+    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();