fix(vscode): give the edit back when a permission diff is closed

packages/vscode-ide-companion/src/commands/index.test.ts

@@ -256,6 +256,62 @@ describe('registerNewCommands', () => {
     );
   });
 
+  // R3-2: every existing showDiff case leaves permissionRequestId undefined, so
+  // the hop that binds a diff to a pending approval was never exercised.
+  it('showDiff forwards a defined permission request id', async () => {
+    workspaceMock.workspaceFolders = [
+      { uri: { fsPath: '/workspace' }, name: 'workspace', index: 0 },
+    ];
+
+    registerNewCommands(
+      context as never,
+      log,
+      diffManager as never,
+      () => [],
+      vi.fn() as never,
+    );
+
+    await getRegisteredHandler(showDiffCommand)({
+      path: '/workspace/src/app.ts',
+      oldText: 'old',
+      newText: 'new',
+      readOnly: true,
+      permissionRequestId: 'req-42',
+    });
+
+    expect(diffManager.showDiff).toHaveBeenCalledWith(
+      '/workspace/src/app.ts',
+      'old',
+      'new',
+      { readOnly: true, permissionRequestId: 'req-42' },
+    );
+  });
+
+  // R3-3: the close side of the same binding. Without the id, closeDiff would
+  // close any diff for that path, including one owned by another approval.
+  it('closeDiff forwards a defined permission request id', async () => {
+    workspaceMock.workspaceFolders = [
+      { uri: { fsPath: '/workspace' }, name: 'workspace', index: 0 },
+    ];
+    const closeDiff = vi.fn().mockResolvedValue(undefined);
+
+    registerNewCommands(
+      context as never,
+      log,
+      { showDiff: vi.fn(), closeDiff } as never,
+      () => [],
+      vi.fn() as never,
+    );
+
+    await getRegisteredHandler(closeDiffCommand)('src/foo.ts', 'req-42');
+
+    expect(closeDiff).toHaveBeenCalledWith(
+      '/workspace/src/foo.ts',
+      true,
+      'req-42',
+    );
+  });
+
   it('showDiff forwards the readOnly flag', async () => {
     workspaceMock.workspaceFolders = [
       { uri: { fsPath: '/workspace' }, name: 'workspace', index: 0 },

packages/vscode-ide-companion/src/diff-manager.test.ts

@@ -63,6 +63,9 @@ vi.mock('vscode', () => ({
     fire = (e: unknown) => {
       for (const listener of this.listeners) listener(e);
     };
+    dispose = () => {
+      this.listeners.length = 0;
+    };
   },
 }));
 
@@ -217,6 +220,25 @@ describe('DiffManager path resolution', () => {
 const WRITABLE_COMMAND =
   'workbench.action.files.setActiveEditorWriteableInSession';
 
+// `vscode.diff` is called as (command, left, right, title, options): the left
+// side is the read-only old document, the right side the writable one the vote
+// commands and the dismissal keying both resolve through.
+function openedDiffCall(): unknown[] {
+  const call = executeCommand.mock.calls.find(
+    ([command]) => command === 'vscode.diff',
+  );
+  if (!call) throw new Error('no diff was opened');
+  return call;
+}
+
+function lastOpenedLeftUri(): { toString(): string } {
+  return openedDiffCall()[1] as { toString(): string };
+}
+
+function lastOpenedRightUri(): { toString(): string } {
+  return openedDiffCall()[2] as { toString(): string };
+}
+
 describe('DiffManager.showDiff writability', () => {
   beforeEach(() => {
     executeCommand.mockClear();
@@ -323,3 +345,234 @@ describe('DiffManager.showDiff reuse', () => {
     expect(diffOpenCount()).toBe(0);
   });
 });
+
+describe('DiffManager permission diff dismissal', () => {
+  beforeEach(() => {
+    executeCommand.mockClear();
+    openTextDocument.mockResolvedValue({ getText: () => 'new content' });
+  });
+
+  function createManager(): InstanceType<typeof DiffManager> {
+    return new DiffManager(() => {}, new DiffContentProvider());
+  }
+
+  it('reports a permission diff the user closed without voting', async () => {
+    const manager = createManager();
+    const closed = vi.fn();
+    manager.onDidClosePermissionDiff(closed);
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new', {
+      readOnly: true,
+      permissionRequestId: 'req-1',
+    });
+    await manager.cancelDiff(lastOpenedRightUri() as never);
+
+    // Deep equality on purpose: the fan-out in extension.ts consumes only the
+    // request id, so a field added here without a reader would be dead weight
+    // and this assertion is what stops one creeping back in.
+    expect(closed).toHaveBeenCalledWith({ permissionRequestId: 'req-1' });
+  });
+
+  it('stays quiet for a diff that no approval is waiting on', async () => {
+    const manager = createManager();
+    const closed = vi.fn();
+    manager.onDidClosePermissionDiff(closed);
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new');
+    await manager.cancelDiff(lastOpenedRightUri() as never);
+
+    expect(closed).not.toHaveBeenCalled();
+  });
+
+  it('does not echo a close the chat surface asked for', async () => {
+    const manager = createManager();
+    const closed = vi.fn();
+    manager.onDidClosePermissionDiff(closed);
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new', {
+      readOnly: true,
+      permissionRequestId: 'req-1',
+    });
+    const rightUri = lastOpenedRightUri();
+    // closeDiff() drops the entry before the tab closes, so the
+    // onDidCloseTextDocument -> cancelDiff hop that follows finds nothing.
+    await manager.closeDiff('/workspace/foo.ts', false, 'req-1');
+    await manager.cancelDiff(rightUri as never);
+
+    expect(closed).not.toHaveBeenCalled();
+  });
+
+  // R4-1: the IDE-mode MCP tool closes by path alone, so its close lands on a
+  // diff an approval owns while the caller knows nothing about that request.
+  it('reports a permission diff an id-less close took away', async () => {
+    const manager = createManager();
+    const closed = vi.fn();
+    manager.onDidClosePermissionDiff(closed);
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new', {
+      readOnly: true,
+      permissionRequestId: 'req-1',
+    });
+    const rightUri = lastOpenedRightUri();
+
+    await manager.closeDiff('/workspace/foo.ts', true);
+    // The entry is already gone, so this hop cannot be what reports it.
+    await manager.cancelDiff(rightUri as never);
+
+    expect(closed).toHaveBeenCalledWith({ permissionRequestId: 'req-1' });
+    expect(closed).toHaveBeenCalledTimes(1);
+  });
+
+  // R5-2: the two cases above both pass `suppressNotification = true`, and the
+  // one case that passes `false` also passes a request id, so it never reaches
+  // the fire. The default arm is the one production actually sends —
+  // IdeClient.disconnect() calls closeDiff(filePath) with no options and the MCP
+  // closeDiff tool forwards `suppressNotification: undefined` — so gating the
+  // fire on the flag instead of on the missing id went unnoticed.
+  it('reports an id-less close that leaves the notification flag at its default', async () => {
+    const manager = createManager();
+    const closed = vi.fn();
+    manager.onDidClosePermissionDiff(closed);
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new', {
+      readOnly: true,
+      permissionRequestId: 'req-1',
+    });
+
+    await manager.closeDiff('/workspace/foo.ts');
+
+    expect(closed).toHaveBeenCalledWith({ permissionRequestId: 'req-1' });
+    expect(closed).toHaveBeenCalledTimes(1);
+  });
+
+  it('stays quiet when an id-less close matches a diff no approval owns', async () => {
+    const manager = createManager();
+    const closed = vi.fn();
+    manager.onDidClosePermissionDiff(closed);
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new');
+
+    await manager.closeDiff('/workspace/foo.ts', true);
+
+    expect(closed).not.toHaveBeenCalled();
+  });
+
+  it('stops notifying once the manager is disposed', async () => {
+    const manager = createManager();
+    const closed = vi.fn();
+    manager.onDidClosePermissionDiff(closed);
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new', {
+      readOnly: true,
+      permissionRequestId: 'req-1',
+    });
+    const rightUri = lastOpenedRightUri();
+
+    // An extension-host reload activates a second manager while the previous
+    // one's listener closure still holds the torn-down provider registry.
+    manager.dispose();
+    await manager.cancelDiff(rightUri as never);
+
+    expect(closed).not.toHaveBeenCalled();
+  });
+});
+
+// R3-9: the request-id binding was landed without a witness for any of its three
+// halves — stored by showDiff, read back by getPermissionRequestId/hasDiff, and
+// used by closeDiff to refuse a diff owned by a different approval.
+describe('DiffManager permission request id binding', () => {
+  beforeEach(() => {
+    executeCommand.mockClear();
+    openTextDocument.mockResolvedValue({ getText: () => 'new content' });
+  });
+
+  function createManager(): InstanceType<typeof DiffManager> {
+    return new DiffManager(() => {}, new DiffContentProvider());
+  }
+
+  it('reads back the request id the diff was opened for', async () => {
+    const manager = createManager();
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new', {
+      readOnly: true,
+      permissionRequestId: 'req-1',
+    });
+    const rightUri = lastOpenedRightUri();
+
+    expect(manager.hasDiff(rightUri as never)).toBe(true);
+    expect(manager.getPermissionRequestId(rightUri as never)).toBe('req-1');
+  });
+
+  // R5-1: the entry is keyed on the writable right side only. `qwen.diff.accept`
+  // and `qwen.diff.cancel` resolve the vote through the active editor's uri,
+  // which is the modified side, so keying the map on the left document would
+  // silently break both — and while the uri mock rendered a `with()` copy with
+  // the original's scheme and query, both sides shared one key and nothing here
+  // could tell the two apart.
+  it('keys the diff on the writable side, not the read-only one', async () => {
+    const manager = createManager();
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new', {
+      readOnly: true,
+      permissionRequestId: 'req-1',
+    });
+    const leftUri = lastOpenedLeftUri();
+
+    expect(manager.hasDiff(leftUri as never)).toBe(false);
+    expect(manager.getPermissionRequestId(leftUri as never)).toBeUndefined();
+  });
+
+  it('leaves the request id undefined for a diff no approval owns', async () => {
+    const manager = createManager();
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new');
+    const rightUri = lastOpenedRightUri();
+
+    expect(manager.hasDiff(rightUri as never)).toBe(true);
+    expect(manager.getPermissionRequestId(rightUri as never)).toBeUndefined();
+  });
+
+  it('refuses to close a diff owned by a different approval', async () => {
+    const manager = createManager();
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new', {
+      readOnly: true,
+      permissionRequestId: 'req-1',
+    });
+    const rightUri = lastOpenedRightUri();
+
+    await manager.closeDiff('/workspace/foo.ts', true, 'req-other');
+
+    // Same path, different owner: the diff the other approval is waiting on
+    // must survive.
+    expect(manager.hasDiff(rightUri as never)).toBe(true);
+  });
+
+  it('closes the bound diff when the ids match', async () => {
+    const manager = createManager();
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new', {
+      readOnly: true,
+      permissionRequestId: 'req-1',
+    });
+    const rightUri = lastOpenedRightUri();
+
+    await manager.closeDiff('/workspace/foo.ts', true, 'req-1');
+
+    expect(manager.hasDiff(rightUri as never)).toBe(false);
+  });
+
+  it('closes by path alone when no request id is given', async () => {
+    const manager = createManager();
+
+    await manager.showDiff('/workspace/foo.ts', 'old', 'new', {
+      readOnly: true,
+      permissionRequestId: 'req-1',
+    });
+    const rightUri = lastOpenedRightUri();
+
+    await manager.closeDiff('/workspace/foo.ts', true);
+
+    expect(manager.hasDiff(rightUri as never)).toBe(false);
+  });
+});

packages/vscode-ide-companion/src/diff-manager.ts

@@ -90,6 +90,24 @@ export class DiffManager {
   private readonly onDidChangeEmitter =
     new vscode.EventEmitter<JSONRPCNotification>();
   readonly onDidChange = this.onDidChangeEmitter.event;
+
+  private readonly onDidClosePermissionDiffEmitter = new vscode.EventEmitter<{
+    permissionRequestId: string;
+  }>();
+  /**
+   * Fires when a diff opened for a pending permission is closed without a vote.
+   *
+   * `onDidChange` carries `ide/diffClosed` to IDE-mode MCP transports, which no
+   * web-shell host is; a chat surface that opened the diff itself would
+   * otherwise never learn the user closed it, and would keep waiting for a
+   * decision on an edit the user can no longer see (#10557). `qwen.diff.accept`
+   * and `qwen.diff.cancel` do not reach here for a request-bound diff — they
+   * route the vote through `respondToPendingPermission` instead — and
+   * `closeDiffEditor` drops the entry before the tab closes, so a close the
+   * chat surface asked for does not echo back as a dismissal.
+   */
+  readonly onDidClosePermissionDiff =
+    this.onDidClosePermissionDiffEmitter.event;
   private diffDocuments = new Map<string, DiffInfo>();
   private readonly subscriptions: vscode.Disposable[] = [];
   // Dedupe: remember recent showDiff calls keyed by (file+content)
@@ -158,6 +176,7 @@ export class DiffManager {
     for (const subscription of this.subscriptions) {
       subscription.dispose();
     }
+    this.onDidClosePermissionDiffEmitter.dispose();
   }
 
   /**
@@ -406,6 +425,19 @@ export class DiffManager {
       const rightDoc = await vscode.workspace.openTextDocument(uriToClose);
       const modifiedContent = rightDoc.getText();
       await this.closeDiffEditor(uriToClose);
+      // An id-less close matches by path alone: that caller does not know an
+      // approval owns this diff, so it cannot tell the surface holding the
+      // request. Fire the dismissal here, because closeDiffEditor already
+      // dropped the entry and the onDidCloseTextDocument -> cancelDiff hop that
+      // follows finds nothing (#10557 through a second door: an IDE-mode CLI
+      // closing the tab leaves the shell locked on a diff that is gone). A
+      // caller that passed the id *is* that surface and has cleared its own
+      // state, so its close must not echo back.
+      if (permissionRequestId === undefined && openDiff.permissionRequestId) {
+        this.onDidClosePermissionDiffEmitter.fire({
+          permissionRequestId: openDiff.permissionRequestId,
+        });
+      }
       if (!suppressNotification) {
         this.onDidChangeEmitter.fire(
           IdeDiffClosedNotificationSchema.parse({
@@ -486,6 +518,12 @@ export class DiffManager {
         },
       }),
     );
+
+    if (diffInfo.permissionRequestId) {
+      this.onDidClosePermissionDiffEmitter.fire({
+        permissionRequestId: diffInfo.permissionRequestId,
+      });
+    }
   }
 
   private async onActiveEditorChange(editor: vscode.TextEditor | undefined) {