diff --git a/packages/cli/src/acp-integration/session/permissionUtils.test.ts b/packages/cli/src/acp-integration/session/permissionUtils.test.ts index e23760406b9..71726b3f7d0 100644 --- a/packages/cli/src/acp-integration/session/permissionUtils.test.ts +++ b/packages/cli/src/acp-integration/session/permissionUtils.test.ts @@ -158,6 +158,39 @@ describe('permissionUtils', () => { }), ); }); + + it('keeps one-shot and always-allow options on edit approvals', () => { + const options = toPermissionOptions({ + type: 'edit', + title: 'Confirm edit', + fileName: 'a.txt', + filePath: '/tmp/a.txt', + fileDiff: 'diff', + originalContent: 'a', + newContent: 'b', + onConfirm: async () => undefined, + }); + + // Both kinds must stay present and in this wire order: the web-shell + // native Accept path selects by kind preference (allow_once first), so + // a missing allow_once would escalate a single Accept into + // "Allow All Edits". + expect(options).toEqual([ + expect.objectContaining({ + optionId: ToolConfirmationOutcome.ProceedAlways, + name: 'Allow All Edits', + kind: 'allow_always', + }), + expect.objectContaining({ + optionId: ToolConfirmationOutcome.ProceedOnce, + kind: 'allow_once', + }), + expect.objectContaining({ + optionId: ToolConfirmationOutcome.Cancel, + kind: 'reject_once', + }), + ]); + }); }); describe('interactionMetaFields', () => { diff --git a/packages/vscode-ide-companion/src/commands/index.test.ts b/packages/vscode-ide-companion/src/commands/index.test.ts index e46c5f071b1..8e99c9a994e 100644 --- a/packages/vscode-ide-companion/src/commands/index.test.ts +++ b/packages/vscode-ide-companion/src/commands/index.test.ts @@ -173,6 +173,7 @@ describe('registerNewCommands', () => { '/workspace/src/app.ts', 'old', 'new', + { readOnly: false, permissionRequestId: undefined }, ); }); @@ -196,7 +197,11 @@ describe('registerNewCommands', () => { { fsPath: '/workspace' }, 'src/foo.ts', ); - expect(closeDiff).toHaveBeenCalledWith('/workspace/src/foo.ts', true); + expect(closeDiff).toHaveBeenCalledWith( + '/workspace/src/foo.ts', + true, + undefined, + ); }); it('closeDiff keeps absolute paths unchanged', async () => { @@ -216,7 +221,11 @@ describe('registerNewCommands', () => { await getRegisteredHandler(closeDiffCommand)('/workspace/src/foo.ts'); expect(joinPath).not.toHaveBeenCalled(); - expect(closeDiff).toHaveBeenCalledWith('/workspace/src/foo.ts', true); + expect(closeDiff).toHaveBeenCalledWith( + '/workspace/src/foo.ts', + true, + undefined, + ); }); it('showDiff keeps UNC paths absolute', async () => { @@ -243,6 +252,35 @@ describe('registerNewCommands', () => { '\\\\server\\share\\app.ts', 'old', 'new', + { readOnly: false, permissionRequestId: undefined }, + ); + }); + + it('showDiff forwards the readOnly flag', 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, + }); + + expect(diffManager.showDiff).toHaveBeenCalledWith( + '/workspace/src/app.ts', + 'old', + 'new', + { readOnly: true, permissionRequestId: undefined }, ); }); }); diff --git a/packages/vscode-ide-companion/src/commands/index.ts b/packages/vscode-ide-companion/src/commands/index.ts index 09b6260d563..9f2df8dc930 100644 --- a/packages/vscode-ide-companion/src/commands/index.ts +++ b/packages/vscode-ide-companion/src/commands/index.ts @@ -78,11 +78,20 @@ export function registerNewCommands( disposables.push( vscode.commands.registerCommand( showDiffCommand, - async (args: { path: string; oldText: string; newText: string }) => { + async (args: { + path: string; + oldText: string; + newText: string; + readOnly?: boolean; + permissionRequestId?: string; + }) => { try { const absolutePath = resolveWorkspaceRelativePath(args.path); log(`[Command] Showing diff for ${absolutePath}`); - await diffManager.showDiff(absolutePath, args.oldText, args.newText); + await diffManager.showDiff(absolutePath, args.oldText, args.newText, { + readOnly: args.readOnly === true, + permissionRequestId: args.permissionRequestId, + }); } catch (error) { const errorMsg = getErrorMessage(error); log(`[Command] Error showing diff: ${errorMsg}`); @@ -95,8 +104,12 @@ export function registerNewCommands( disposables.push( vscode.commands.registerCommand( closeDiffCommand, - async (filePath: string) => - diffManager.closeDiff(resolveWorkspaceRelativePath(filePath), true), + async (filePath: string, permissionRequestId?: string) => + diffManager.closeDiff( + resolveWorkspaceRelativePath(filePath), + true, + permissionRequestId, + ), ), ); diff --git a/packages/vscode-ide-companion/src/diff-manager.test.ts b/packages/vscode-ide-companion/src/diff-manager.test.ts new file mode 100644 index 00000000000..49c929316c3 --- /dev/null +++ b/packages/vscode-ide-companion/src/diff-manager.test.ts @@ -0,0 +1,179 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const executeCommand = vi.fn().mockResolvedValue(undefined); + +vi.mock('vscode', () => { + class EventEmitter { + private listeners = new Set<(event: T) => void>(); + event = (listener: (event: T) => void) => { + this.listeners.add(listener); + return { dispose: () => this.listeners.delete(listener) }; + }; + fire(event: T): void { + for (const listener of [...this.listeners]) listener(event); + } + dispose(): void { + this.listeners.clear(); + } + } + + return { + EventEmitter, + Uri: { + file: (filePath: string) => { + const uri: { + fsPath: string; + scheme: string; + query: string; + with: (change: Record) => unknown; + toString: () => string; + } = { + fsPath: filePath, + scheme: 'file', + query: '', + with(change: Record) { + return { ...uri, ...change }; + }, + toString() { + return `${uri.scheme}://${uri.fsPath}?${uri.query}`; + }, + }; + return uri; + }, + }, + ViewColumn: { Active: -1, Beside: -2 }, + commands: { executeCommand }, + window: { + activeTextEditor: undefined, + onDidChangeActiveTextEditor: vi.fn(() => ({ dispose: vi.fn() })), + tabGroups: { all: [] }, + }, + }; +}); + +// Avoid pulling the full extension module graph; only the scheme constant is +// needed by the diff manager. +vi.mock('./extension.js', () => ({ DIFF_SCHEME: 'qwen-diff' })); + +vi.mock('@qwen-code/qwen-code-core', () => ({ + IdeDiffAcceptedNotificationSchema: { parse: (value: unknown) => value }, + IdeDiffClosedNotificationSchema: { parse: (value: unknown) => value }, +})); + +const { DiffContentProvider, DiffManager } = await import('./diff-manager.js'); + +const WRITABLE_COMMAND = + 'workbench.action.files.setActiveEditorWriteableInSession'; + +describe('DiffManager.showDiff writability', () => { + beforeEach(() => { + executeCommand.mockClear(); + }); + + function createManager(): InstanceType { + return new DiffManager(() => {}, new DiffContentProvider()); + } + + it('makes regular diffs editable so IDE-mode approvals can round-trip edits', async () => { + const manager = createManager(); + + await manager.showDiff('/workspace/foo.ts', 'old', 'new'); + + expect(executeCommand).toHaveBeenCalledWith(WRITABLE_COMMAND); + }); + + it('keeps read-only diffs locked for flows that cannot round-trip edits', async () => { + const manager = createManager(); + + await manager.showDiff('/workspace/foo.ts', 'old', 'new', { + readOnly: true, + }); + + expect(executeCommand).not.toHaveBeenCalledWith(WRITABLE_COMMAND); + // The diff itself still opens. + expect(executeCommand).toHaveBeenCalledWith( + 'vscode.diff', + expect.anything(), + expect.anything(), + expect.stringContaining('foo.ts'), + expect.anything(), + ); + }); +}); + +describe('DiffManager.showDiff reuse', () => { + beforeEach(() => { + executeCommand.mockClear(); + }); + + function createManager(): InstanceType { + return new DiffManager(() => {}, new DiffContentProvider()); + } + + function diffOpenCount(): number { + return executeCommand.mock.calls.filter( + ([command]) => command === 'vscode.diff', + ).length; + } + + it('opens a fresh diff instead of reusing a writable twin for a read-only request', async () => { + const manager = createManager(); + + // IDE-mode flow opens a writable diff for this (path, old, new) triple. + await manager.showDiff('/workspace/foo.ts', 'old', 'new'); + executeCommand.mockClear(); + + // A web-shell approval for the same triple must get its own read-only + // diff; reusing the writable one would invite hand-edits that the + // approving tool then silently discards (and inside the dedupe window + // the request would otherwise be suppressed outright). + await manager.showDiff('/workspace/foo.ts', 'old', 'new', { + readOnly: true, + }); + + expect(diffOpenCount()).toBe(1); + expect(executeCommand).not.toHaveBeenCalledWith(WRITABLE_COMMAND); + }); + + it('opens a fresh diff instead of reusing a read-only twin for a writable request', async () => { + const manager = createManager(); + + await manager.showDiff('/workspace/foo.ts', 'old', 'new', { + readOnly: true, + }); + executeCommand.mockClear(); + + // The IDE-mode flow needs an editable right side to round-trip edits; + // refocusing the locked diff would take that away. + await manager.showDiff('/workspace/foo.ts', 'old', 'new'); + + expect(diffOpenCount()).toBe(1); + expect(executeCommand).toHaveBeenCalledWith(WRITABLE_COMMAND); + }); + + it('still dedupes repeat requests with matching writability', async () => { + const manager = createManager(); + + await manager.showDiff('/workspace/foo.ts', 'old', 'new'); + executeCommand.mockClear(); + + // Same writability inside the dedupe window: suppressed entirely. + await manager.showDiff('/workspace/foo.ts', 'old', 'new'); + expect(diffOpenCount()).toBe(0); + + await manager.showDiff('/workspace/foo.ts', 'old', 'new', { + readOnly: true, + }); + executeCommand.mockClear(); + await manager.showDiff('/workspace/foo.ts', 'old', 'new', { + readOnly: true, + }); + expect(diffOpenCount()).toBe(0); + }); +}); diff --git a/packages/vscode-ide-companion/src/diff-manager.ts b/packages/vscode-ide-companion/src/diff-manager.ts index ccabe3657ea..f1fecfcadfa 100644 --- a/packages/vscode-ide-companion/src/diff-manager.ts +++ b/packages/vscode-ide-companion/src/diff-manager.ts @@ -43,6 +43,20 @@ export class DiffContentProvider implements vscode.TextDocumentContentProvider { } } +/** Options controlling how a diff editor is opened. */ +export interface ShowDiffOptions { + /** + * Open the right-hand (proposed) side as read-only. Use this when the + * approval flow cannot round-trip user edits: the approving daemon tool + * applies its own proposed content, so an editable right side would + * silently discard anything the user typed (e.g. web-shell permission + * diffs opened while IDE mode is off). + */ + readOnly?: boolean; + /** WebShell permission request represented by this native diff. */ + permissionRequestId?: string; +} + // Information about a diff view that is currently open. interface DiffInfo { originalFilePath: string; @@ -50,6 +64,13 @@ interface DiffInfo { newContent: string; leftDocUri: vscode.Uri; rightDocUri: vscode.Uri; + permissionRequestId?: string; + /** + * Whether the right-hand side was opened read-only. Reuse must match on + * this too: refocusing a writable twin for a read-only approval (or vice + * versa) would hand one flow the other flow's edit semantics. + */ + readOnly: boolean; } /** @@ -73,6 +94,38 @@ export class DiffManager { // Timed suppression window (e.g. immediately after permission allow) private suppressUntil: number | null = null; + private getTargetViewColumn( + leftDocUri?: vscode.Uri, + rightDocUri?: vscode.Uri, + ): vscode.ViewColumn { + if (leftDocUri && rightDocUri) { + const leftUri = leftDocUri.toString(); + const rightUri = rightDocUri.toString(); + for (const group of vscode.window.tabGroups.all) { + const containsDiff = group.tabs.some((tab) => { + const input = tab.input as { + original?: vscode.Uri; + modified?: vscode.Uri; + } | undefined; + return ( + input?.original?.toString() === leftUri && + input?.modified?.toString() === rightUri + ); + }); + if (containsDiff) { + return group.viewColumn; + } + } + } + + return ( + findLeftGroupOfChatWebview() ?? + findRightGroupOfChatWebview() ?? + vscode.window.activeTextEditor?.viewColumn ?? + vscode.ViewColumn.Active + ); + } + constructor( private readonly log: (message: string) => void, private readonly diffContentProvider: DiffContentProvider, @@ -100,18 +153,24 @@ export class DiffManager { * @param filePath Path to the file being diffed * @param oldContent The original content (left side) * @param newContent The modified content (right side) + * @param readOnly Writability the requester needs; only diffs with the + * same writability are reusable * @returns True if a diff view with the same content already exists, false otherwise */ private hasExistingDiff( filePath: string, oldContent: string, newContent: string, + readOnly: boolean, + permissionRequestId?: string, ): boolean { for (const diffInfo of this.diffDocuments.values()) { if ( diffInfo.originalFilePath === filePath && diffInfo.oldContent === oldContent && - diffInfo.newContent === newContent + diffInfo.newContent === newContent && + diffInfo.readOnly === readOnly && + diffInfo.permissionRequestId === permissionRequestId ) { return true; } @@ -122,12 +181,21 @@ export class DiffManager { /** * Finds an existing diff view for the given file path and focuses it * @param filePath Path to the file being diffed + * @param readOnly Only diffs opened with the same writability are eligible * @returns True if an existing diff view was found and focused, false otherwise */ - private async focusExistingDiff(filePath: string): Promise { + private async focusExistingDiff( + filePath: string, + readOnly: boolean, + permissionRequestId?: string, + ): Promise { const normalizedPath = path.normalize(filePath); for (const [, diffInfo] of this.diffDocuments.entries()) { - if (diffInfo.originalFilePath === normalizedPath) { + if ( + diffInfo.originalFilePath === normalizedPath && + diffInfo.readOnly === readOnly && + diffInfo.permissionRequestId === permissionRequestId + ) { const rightDocUri = diffInfo.rightDocUri; const leftDocUri = diffInfo.leftDocUri; @@ -140,7 +208,7 @@ export class DiffManager { rightDocUri, diffTitle, { - viewColumn: vscode.ViewColumn.Beside, + viewColumn: this.getTargetViewColumn(leftDocUri, rightDocUri), preview: false, preserveFocus: true, }, @@ -162,21 +230,43 @@ export class DiffManager { * If only newContent is provided, the old content will be read from the * filesystem (empty string when file does not exist). */ - async showDiff(filePath: string, newContent: string): Promise; + async showDiff( + filePath: string, + newContent: string, + options?: ShowDiffOptions, + ): Promise; async showDiff( filePath: string, oldContent: string, newContent: string, + options?: ShowDiffOptions, ): Promise; - async showDiff(filePath: string, a: string, b?: string): Promise { + async showDiff( + filePath: string, + a: string, + b?: string | ShowDiffOptions, + options?: ShowDiffOptions, + ): Promise { const haveOld = typeof b === 'string'; + const resolvedOptions = haveOld ? options : b; + const readOnly = resolvedOptions?.readOnly === true; const oldContent = haveOld ? a : await this.readOldContentFromFs(filePath); const newContent = haveOld ? (b as string) : a; const normalizedPath = path.normalize(filePath); const key = this.makeKey(normalizedPath, oldContent, newContent); - // Check if a diff view with the same content already exists - if (this.hasExistingDiff(normalizedPath, oldContent, newContent)) { + // Check if a diff view with the same content, writability, and permission + // owner already exists. A read-only approval must never be deduped onto a + // writable diff, and two permission requests must not share a diff. + if ( + this.hasExistingDiff( + normalizedPath, + oldContent, + newContent, + readOnly, + resolvedOptions?.permissionRequestId, + ) + ) { const last = this.recentlyShown.get(key) || 0; const now = Date.now(); if (now - last < DiffManager.DEDUPE_WINDOW_MS) { @@ -187,7 +277,11 @@ export class DiffManager { return; } // Outside the dedupe window: softly focus the existing diff - await this.focusExistingDiff(normalizedPath); + await this.focusExistingDiff( + normalizedPath, + readOnly, + resolvedOptions?.permissionRequestId, + ); this.recentlyShown.set(key, now); return; } @@ -213,6 +307,8 @@ export class DiffManager { newContent, leftDocUri, rightDocUri, + readOnly, + permissionRequestId: resolvedOptions?.permissionRequestId, }); const diffTitle = `${path.basename(normalizedPath)} (Before ↔ After)`; @@ -224,12 +320,9 @@ export class DiffManager { // Prefer opening the diff in the group to the left of the chat webview. // When that isn't available (e.g. chat is in the leftmost group), try the - // group to the right so we reuse existing layout. Only fall back to - // ViewColumn.Beside when neither neighbor exists or the webview is missing. - const targetViewColumn = - findLeftGroupOfChatWebview() ?? - findRightGroupOfChatWebview() ?? - vscode.ViewColumn.Beside; + // group to the right so we reuse existing layout. Sidebar chat has no + // editor group, so fall back to the active group rather than creating one. + const targetViewColumn = this.getTargetViewColumn(); await vscode.commands.executeCommand( 'vscode.diff', @@ -242,9 +335,15 @@ export class DiffManager { preserveFocus: true, }, ); - await vscode.commands.executeCommand( - 'workbench.action.files.setActiveEditorWriteableInSession', - ); + // The writeable-in-session flag exists so users can adjust the proposed + // content before accepting; that only round-trips when an IDE-mode + // resolver consumes the edited text. Read-only callers (web-shell + // permission approvals) would silently lose edits, so keep them locked. + if (!readOnly) { + await vscode.commands.executeCommand( + 'workbench.action.files.setActiveEditorWriteableInSession', + ); + } this.recentlyShown.set(key, Date.now()); } @@ -252,11 +351,19 @@ export class DiffManager { /** * Closes an open diff view for a specific file. */ - async closeDiff(filePath: string, suppressNotification = false) { + async closeDiff( + filePath: string, + suppressNotification = false, + permissionRequestId?: string, + ) { const normalizedPath = path.normalize(filePath); let uriToClose: vscode.Uri | undefined; for (const [, diffInfo] of this.diffDocuments.entries()) { - if (diffInfo.originalFilePath === normalizedPath) { + if ( + diffInfo.originalFilePath === normalizedPath && + (permissionRequestId === undefined || + diffInfo.permissionRequestId === permissionRequestId) + ) { uriToClose = diffInfo.rightDocUri; break; } @@ -309,6 +416,14 @@ export class DiffManager { ); } + getPermissionRequestId(rightDocUri: vscode.Uri): string | undefined { + return this.diffDocuments.get(rightDocUri.toString())?.permissionRequestId; + } + + hasDiff(rightDocUri: vscode.Uri): boolean { + return this.diffDocuments.has(rightDocUri.toString()); + } + /** * Called when a user cancels a diff view. */ diff --git a/packages/vscode-ide-companion/src/extension.test.ts b/packages/vscode-ide-companion/src/extension.test.ts index 043a1aa78f9..30a2ab1a3eb 100644 --- a/packages/vscode-ide-companion/src/extension.test.ts +++ b/packages/vscode-ide-companion/src/extension.test.ts @@ -6,7 +6,9 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import * as vscode from 'vscode'; +import { DiffManager } from './diff-manager.js'; import { activate } from './extension.js'; +import { ChatProviderRegistry } from './webview/providers/ChatProviderRegistry.js'; import { IDE_DEFINITIONS, detectIdeFromEnv } from '@qwen-code/qwen-code-core'; vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { @@ -372,4 +374,73 @@ describe('activate', () => { expect(showInformationMessageMock).not.toHaveBeenCalled(); }); }); + + describe('diff vote command gate', () => { + it('derives fromDiffEditor from the diff scheme and honors the pending gate', async () => { + vi.spyOn(global, 'fetch').mockResolvedValue({ + ok: false, + statusText: 'Internal Server Error', + } as Response); + + const provider = { + hasPendingPermission: vi.fn(() => true), + respondToPendingPermission: vi.fn(), + dispose: vi.fn(), + }; + const registrySpy = vi + .spyOn(ChatProviderRegistry.prototype, 'getPermissionAwareProviders') + .mockReturnValue([provider] as never); + + await activate(context); + + const findHandler = (name: string) => + vi + .mocked(vscode.commands.registerCommand) + .mock.calls.find(([id]) => id === name)?.[1] as + | ((uri?: unknown) => void) + | undefined; + const acceptHandler = findHandler('qwen.diff.accept'); + expect(acceptHandler).toBeDefined(); + + const diffUri = { + scheme: 'qwen-diff', + fsPath: '/workspace/src/app.ts', + toString: () => 'qwen-diff:///workspace/src/app.ts', + }; + const hasDiff = vi + .spyOn(DiffManager.prototype, 'hasDiff') + .mockReturnValue(true); + const getPermissionRequestId = vi + .spyOn(DiffManager.prototype, 'getPermissionRequestId') + .mockReturnValue('req-1'); + + await acceptHandler!(diffUri); + expect(provider.respondToPendingPermission).toHaveBeenCalledWith( + 'allow', + { fromDiffEditor: true, permissionRequestId: 'req-1' }, + ); + + provider.respondToPendingPermission.mockClear(); + const fileUri = { + scheme: 'file', + fsPath: '/workspace/src/app.ts', + toString: () => 'file:///workspace/src/app.ts', + }; + await acceptHandler!(fileUri); + expect(provider.respondToPendingPermission).not.toHaveBeenCalled(); + + getPermissionRequestId.mockReturnValue(undefined); + await acceptHandler!(diffUri); + expect(provider.respondToPendingPermission).toHaveBeenCalledWith('allow'); + + provider.respondToPendingPermission.mockClear(); + provider.hasPendingPermission.mockReturnValue(false); + await acceptHandler!(diffUri); + expect(provider.respondToPendingPermission).not.toHaveBeenCalled(); + + hasDiff.mockRestore(); + getPermissionRequestId.mockRestore(); + registrySpy.mockRestore(); + }); + }); }); diff --git a/packages/vscode-ide-companion/src/extension.ts b/packages/vscode-ide-companion/src/extension.ts index 8fe1b048d4f..af3c863f1c2 100644 --- a/packages/vscode-ide-companion/src/extension.ts +++ b/packages/vscode-ide-companion/src/extension.ts @@ -248,42 +248,70 @@ export async function activate(context: vscode.ExtensionContext) { DIFF_SCHEME, diffContentProvider, ), - (vscode.commands.registerCommand('qwen.diff.accept', (uri?: vscode.Uri) => { - const docUri = uri ?? vscode.window.activeTextEditor?.document.uri; - if (docUri && docUri.scheme === DIFF_SCHEME) { - diffManager.acceptDiff(docUri); - } - // If any chat surface is requesting permission, actively select allow (prefer once) - try { - for (const provider of chatProviderRegistry?.getPermissionAwareProviders() ?? - []) { - if (provider?.hasPendingPermission()) { - provider.respondToPendingPermission('allow'); + vscode.commands.registerCommand( + 'qwen.diff.accept', + async (uri?: vscode.Uri) => { + const docUri = uri ?? vscode.window.activeTextEditor?.document.uri; + const isManagedDiff = + docUri?.scheme === DIFF_SCHEME && diffManager.hasDiff(docUri); + const permissionRequestId = isManagedDiff + ? diffManager.getPermissionRequestId(docUri) + : undefined; + if (docUri && isManagedDiff && !permissionRequestId) { + await diffManager.acceptDiff(docUri); + } + // If any chat surface is requesting permission, actively select allow (prefer once) + try { + for (const provider of chatProviderRegistry?.getPermissionAwareProviders() ?? + []) { + if (!isManagedDiff) continue; + if (permissionRequestId) { + provider.respondToPendingPermission('allow', { + fromDiffEditor: true, + permissionRequestId, + }); + } else if (provider?.hasPendingPermission()) { + provider.respondToPendingPermission('allow'); + } } + } catch (err) { + logger.warn('[Extension] Auto-allow on diff.accept failed:', err); } - } catch (err) { - logger.warn('[Extension] Auto-allow on diff.accept failed:', err); - } - logger.log('[Extension] Diff accepted'); - }), - vscode.commands.registerCommand('qwen.diff.cancel', (uri?: vscode.Uri) => { - const docUri = uri ?? vscode.window.activeTextEditor?.document.uri; - if (docUri && docUri.scheme === DIFF_SCHEME) { - diffManager.cancelDiff(docUri); - } - // If any chat surface is requesting permission, actively select reject/cancel - try { - for (const provider of chatProviderRegistry?.getPermissionAwareProviders() ?? - []) { - if (provider?.hasPendingPermission()) { - provider.respondToPendingPermission('cancel'); + logger.log('[Extension] Diff accepted'); + }, + ), + vscode.commands.registerCommand( + 'qwen.diff.cancel', + async (uri?: vscode.Uri) => { + const docUri = uri ?? vscode.window.activeTextEditor?.document.uri; + const isManagedDiff = + docUri?.scheme === DIFF_SCHEME && diffManager.hasDiff(docUri); + const permissionRequestId = isManagedDiff + ? diffManager.getPermissionRequestId(docUri) + : undefined; + if (docUri && isManagedDiff && !permissionRequestId) { + await diffManager.cancelDiff(docUri); + } + // If any chat surface is requesting permission, actively select reject/cancel + try { + for (const provider of chatProviderRegistry?.getPermissionAwareProviders() ?? + []) { + if (!isManagedDiff) continue; + if (permissionRequestId) { + provider.respondToPendingPermission('cancel', { + fromDiffEditor: true, + permissionRequestId, + }); + } else if (provider?.hasPendingPermission()) { + provider.respondToPendingPermission('cancel'); + } } + } catch (err) { + logger.warn('[Extension] Auto-reject on diff.cancel failed:', err); } - } catch (err) { - logger.warn('[Extension] Auto-reject on diff.cancel failed:', err); - } - logger.log('[Extension] Diff cancelled'); - })), + logger.log('[Extension] Diff cancelled'); + }, + ), vscode.commands.registerCommand('qwen.diff.closeAll', async () => { try { await diffManager.closeAll(); diff --git a/packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx b/packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx index 4f92dc6594d..8d0c15ff42b 100644 --- a/packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx +++ b/packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx @@ -479,8 +479,118 @@ describe('EmbeddedApp host wiring', () => { oldText: 'header\nconst value = 1;\nfooter', newText: 'header\nconst value = 2;\nfooter', source: 'web-shell', + requestId: 'req-write', }, }); + expect(postMessagesOfType('webShellPermissionState').at(-1)).toEqual({ + type: 'webShellPermissionState', + data: { pending: true, requestId: 'req-write' }, + }); + }); + + it('keeps host permission ownership in sync while pending stays true', async () => { + const props = await renderApp(); + const onTranscriptChange = callback<(blocks: unknown[]) => void>( + props, + 'onTranscriptChange', + ); + const permissionBlock = (id: string, path: string) => ({ + id, + kind: 'permission', + requestId: id, + title: path, + options: [], + preview: { kind: 'key_value', rows: [] }, + toolCall: { + content: [{ type: 'diff', path, oldText: 'old', newText: 'new' }], + }, + }); + + await act(async () => { + onTranscriptChange([ + permissionBlock('req-a', '/workspace/a.ts'), + permissionBlock('req-b', '/workspace/b.ts'), + ]); + await Promise.resolve(); + }); + + expect(postMessagesOfType('webShellPermissionState').at(-1)).toEqual({ + type: 'webShellPermissionState', + data: { pending: true, requestId: 'req-a' }, + }); + + await act(async () => { + onTranscriptChange([ + { ...permissionBlock('req-a', '/workspace/a.ts'), resolved: true }, + permissionBlock('req-b', '/workspace/b.ts'), + ]); + await Promise.resolve(); + }); + + // Pending stays true, but ownership moves to the remaining request so a + // stale accept cannot vote on the wrong approval. + expect(postMessagesOfType('webShellPermissionState').at(-1)).toEqual({ + type: 'webShellPermissionState', + data: { pending: true, requestId: 'req-b' }, + }); + }); + + it('posts pending: false when pending permission diffs are torn down', async () => { + const props = await renderApp(); + const onTranscriptChange = callback<(blocks: unknown[]) => void>( + props, + 'onTranscriptChange', + ); + + await act(async () => { + onTranscriptChange([ + { + id: 'perm-a', + kind: 'permission', + requestId: 'req-a', + title: 'update a.ts', + options: [], + preview: { kind: 'key_value', rows: [] }, + toolCall: { + content: [ + { + type: 'diff', + path: '/workspace/a.ts', + oldText: 'old', + newText: 'new', + }, + ], + }, + }, + ]); + await Promise.resolve(); + }); + + expect(postMessagesOfType('webShellPermissionState').at(-1)).toEqual({ + type: 'webShellPermissionState', + data: { pending: true, requestId: 'req-a' }, + }); + + // Closing the host tab/view unmounts the app. The teardown must tell + // the extension the pending set is gone; otherwise the vote gate stays + // open for an approval the user can no longer see. + const { container, root } = mounted.splice(mounted.length - 1, 1)[0]; + await act(async () => { + root.unmount(); + await Promise.resolve(); + }); + container.remove(); + + expect(postMessagesOfType('webShellPermissionState').at(-1)).toEqual({ + type: 'webShellPermissionState', + data: { pending: false }, + }); + expect(postMessagesOfType('closeDiff')).toEqual([ + { + type: 'closeDiff', + data: { path: '/workspace/a.ts', requestId: 'req-a' }, + }, + ]); }); it('routes auth and session-change host actions to the extension', async () => { @@ -591,15 +701,23 @@ describe('EmbeddedApp host wiring', () => { await Promise.resolve(); }); - expect(container.textContent).toContain('Loading conversation…'); + expect( + container.querySelector( + '[role="status"][aria-label="Loading conversation…"]', + ), + ).not.toBeNull(); - // A retriable connection failure that never settles must not lock the - // panel behind the overlay forever. + // A retriable connection failure that never settles must not leave the + // header loading state active forever. await act(async () => { await vi.advanceTimersByTimeAsync(15_000); }); - expect(container.textContent).not.toContain('Loading conversation…'); + expect( + container.querySelector( + '[role="status"][aria-label="Loading conversation…"]', + ), + ).toBeNull(); expect(container.textContent).toContain( 'The conversation switch timed out. Try again.', ); @@ -608,3 +726,144 @@ describe('EmbeddedApp host wiring', () => { } }); }); + +describe('web shell permission decision messages', () => { + function installShellApi( + api: Record, + ): Record { + const props = mocks.embeddedProps.current; + expect(props).not.toBeNull(); + const shellRef = (props as CapturedProps)['shellRef'] as { + current: unknown; + }; + expect(shellRef).toBeTruthy(); + shellRef.current = api; + return api; + } + + async function setPendingPermission( + props: CapturedProps, + requestId = 'req-1', + ) { + const onTranscriptChange = callback<(blocks: unknown[]) => void>( + props, + 'onTranscriptChange', + ); + await act(async () => { + onTranscriptChange([ + { + id: 'permission-1', + kind: 'permission', + requestId, + title: 'Edit fixture.txt', + resolved: false, + options: [], + preview: { kind: 'key_value', rows: [] }, + toolCall: { + content: [ + { + type: 'diff', + path: '/workspace/fixture.txt', + oldText: 'before', + newText: 'after', + }, + ], + }, + }, + ]); + await Promise.resolve(); + }); + } + + async function dispatchDecision( + decision: string, + source: Window | null, + requestId = 'req-1', + ) { + await act(async () => { + window.dispatchEvent( + new MessageEvent('message', { + data: { + type: 'webShellPermissionDecision', + data: { decision, requestId }, + }, + source, + }), + ); + await Promise.resolve(); + }); + } + + it('forwards host-relayed decisions to the web shell', async () => { + const props = await renderApp(); + const respondToPendingPermission = vi.fn().mockResolvedValue(true); + installShellApi({ respondToPendingPermission }); + await setPendingPermission(props); + + // Extension-host messages arrive via the webview preload frame, i.e. + // with this frame's parent as their source. + await dispatchDecision('allow', window.parent); + + expect(respondToPendingPermission).toHaveBeenCalledWith('req-1', 'allow'); + }); + + it('ignores decisions posted by a nested iframe window', async () => { + const props = await renderApp(); + const respondToPendingPermission = vi.fn().mockResolvedValue(true); + installShellApi({ respondToPendingPermission }); + await setPendingPermission(props); + + // MCP apps and artifact previews run in scriptable sandboxed iframes + // inside this webview; they can postMessage to this window and must + // not be able to vote on the pending approval, even when they know the + // active request id. Their source is their own child window, not the + // preload parent frame. + const iframe = document.createElement('iframe'); + document.body.appendChild(iframe); + try { + const childWindow = iframe.contentWindow; + expect(childWindow).not.toBeNull(); + await dispatchDecision('allow', childWindow as Window); + await dispatchDecision('reject', childWindow as Window); + } finally { + iframe.remove(); + } + + expect(respondToPendingPermission).not.toHaveBeenCalled(); + }); + + it('ignores decisions delivered without a source window', async () => { + const props = await renderApp(); + const respondToPendingPermission = vi.fn().mockResolvedValue(true); + installShellApi({ respondToPendingPermission }); + await setPendingPermission(props); + + // Fail closed on synthetic deliveries: real host messages always carry + // the preload frame as their source. + await dispatchDecision('allow', null); + + expect(respondToPendingPermission).not.toHaveBeenCalled(); + }); + + it('surfaces a notice when the shell resolves the vote to false', async () => { + const props = await renderApp(); + const respondToPendingPermission = vi.fn().mockResolvedValue(false); + installShellApi({ respondToPendingPermission }); + await setPendingPermission(props); + const { container } = mounted[mounted.length - 1]; + + await dispatchDecision('allow', window.parent); + await act(async () => { + await Promise.resolve(); + }); + + expect(respondToPendingPermission).toHaveBeenCalledWith('req-1', 'allow'); + // A resolved `false` must not die silently: it covers both the benign + // race (the approval was resolved elsewhere one tick earlier) and hung + // votes (e.g. while catching up after a session switch). Notify the + // user without the hard-error state reset of `handleShellError`. + expect(container.textContent).toContain( + 'The approval decision could not be applied.', + ); + }); +}); diff --git a/packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx b/packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx index 115637842fb..f2896a9af6d 100644 --- a/packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx +++ b/packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx @@ -340,6 +340,7 @@ export function EmbeddedApp() { const currentModelIdRef = useRef(undefined); const transcriptBlocksRef = useRef([]); const openPermissionDiffsRef = useRef(new Map()); + const webShellPermissionRequestIdRef = useRef(undefined); const focusedPermissionRequestIdRef = useRef(undefined); const contextMenuRowKeyRef = useRef(null); const previousActiveFilePathRef = useRef(undefined); @@ -492,37 +493,74 @@ export function EmbeddedApp() { ); const closeOpenPermissionDiffs = useCallback(() => { - for (const path of openPermissionDiffsRef.current.values()) { - vscode.postMessage({ type: 'closeDiff', data: { path } }); + for (const [requestId, path] of openPermissionDiffsRef.current) { + vscode.postMessage({ type: 'closeDiff', data: { path, requestId } }); } openPermissionDiffsRef.current.clear(); + if (webShellPermissionRequestIdRef.current) { + webShellPermissionRequestIdRef.current = undefined; + vscode.postMessage({ + type: 'webShellPermissionState', + data: { pending: false }, + }); + } }, [vscode]); const updateTranscript = useCallback( (blocks: readonly DaemonTranscriptBlock[]) => { transcriptBlocksRef.current = blocks; const pendingIds = new Set(); - let permissionToFocus: string | undefined; - for (const block of blocks) { - if (block.kind !== 'permission' || block.resolved) { - continue; + const pendingPermission = blocks.find( + (block) => block.kind === 'permission' && !block.resolved, + ); + const permissionToFocus = + pendingPermission?.kind === 'permission' + ? pendingPermission.requestId + : undefined; + if (pendingPermission?.kind === 'permission') { + const diff = permissionDiffPreview(pendingPermission); + if (diff) { + const { path, oldText, newText } = diff; + pendingIds.add(pendingPermission.requestId); + if ( + !openPermissionDiffsRef.current.has(pendingPermission.requestId) + ) { + openPermissionDiffsRef.current.set( + pendingPermission.requestId, + path, + ); + vscode.postMessage({ + type: 'openDiff', + data: { + path, + oldText, + newText, + source: 'web-shell', + requestId: pendingPermission.requestId, + }, + }); + } } - permissionToFocus = block.requestId; - const diff = permissionDiffPreview(block); - if (!diff) continue; - const { path, oldText, newText } = diff; - pendingIds.add(block.requestId); - if (openPermissionDiffsRef.current.has(block.requestId)) continue; - openPermissionDiffsRef.current.set(block.requestId, path); - vscode.postMessage({ - type: 'openDiff', - data: { path, oldText, newText, source: 'web-shell' }, - }); } for (const [requestId, path] of openPermissionDiffsRef.current) { if (pendingIds.has(requestId)) continue; openPermissionDiffsRef.current.delete(requestId); - vscode.postMessage({ type: 'closeDiff', data: { path } }); + vscode.postMessage({ + type: 'closeDiff', + data: { path, requestId }, + }); + } + const pendingDiffRequestId = pendingIds.values().next().value as + | string + | undefined; + if (webShellPermissionRequestIdRef.current !== pendingDiffRequestId) { + webShellPermissionRequestIdRef.current = pendingDiffRequestId; + vscode.postMessage({ + type: 'webShellPermissionState', + data: pendingDiffRequestId + ? { pending: true, requestId: pendingDiffRequestId } + : { pending: false }, + }); } if ( permissionToFocus && @@ -643,6 +681,52 @@ export function EmbeddedApp() { // it, `runtime` is set and that branch is gone, so the same failure // would be invisible — show it over the transcript instead. if (runtimeRef.current) setHostNotice({ tone: 'error', text }); + } else if (message.type === 'webShellPermissionDecision') { + const decisionData = message.data as { + decision?: unknown; + requestId?: unknown; + } | null; + const decision = decisionData?.decision; + const requestId = decisionData?.requestId; + const isHostDecision = + event.source === window.parent && + requestId === webShellPermissionRequestIdRef.current; + if ( + (decision === 'allow' || decision === 'reject') && + typeof requestId === 'string' && + isHostDecision + ) { + const response = shellRef.current?.respondToPendingPermission?.( + requestId, + decision, + ); + if (!response) { + // The shell is not mounted yet, so the vote would die without a + // rejection for `.catch` to see. + if (runtimeRef.current) { + setHostNotice({ + tone: 'info', + text: t('permission.voteNotApplied'), + }); + } + } else { + void response + .then((handled) => { + // A resolved `false` drops the vote as silently as a + // rejection would — e.g. while catching up after a session + // switch — but it is also the normal result when the + // approval was resolved elsewhere one tick earlier. Notify + // without the hard-error state reset of `handleShellError`. + if (!handled) { + setHostNotice({ + tone: 'info', + text: t('permission.voteNotApplied'), + }); + } + }) + .catch(handleShellError); + } + } } else if (message.type === 'error') { const text = (message.data as { message?: unknown } | null)?.message; if (typeof text === 'string') setHostNotice({ tone: 'error', text }); @@ -810,7 +894,20 @@ export function EmbeddedApp() { window.addEventListener('message', receiveBootstrap); vscode.postMessage({ type: 'webShellReady', data: {} }); return () => window.removeEventListener('message', receiveBootstrap); - }, [clearInsight, closeOpenPermissionDiffs, t, updateTranscript, vscode]); + }, [ + clearInsight, + closeOpenPermissionDiffs, + handleShellError, + t, + updateTranscript, + vscode, + ]); + + const sessionTransitionLabel = creatingSession + ? t('session.creating') + : switchingSessionId + ? t('session.switching') + : undefined; if (!runtime) { return ( @@ -943,33 +1040,6 @@ export function EmbeddedApp() { }} /> )} - {(switchingSessionId || creatingSession) && ( -
-
- )}
{sessionTitle} -