diff --git a/.changeset/fix-cancel-stops-execution.md b/.changeset/fix-cancel-stops-execution.md new file mode 100644 index 000000000..ad77ca111 --- /dev/null +++ b/.changeset/fix-cancel-stops-execution.md @@ -0,0 +1,5 @@ +--- +"@nanocollective/nanocoder": patch +--- + +Fixed the VS Code extension's stop button leaving a request running. Two holes: `AcpSession.cancel()` aborted the current controller and immediately replaced it with a fresh one, so a cancel that landed before the turn read the signal — the window while the agent is still resolving the prompt's file references — handed the turn an unaborted controller and the stop was lost. The controller is now rotated when a turn begins instead. Separately, the extension never answered the agent's pending permission request when you hit stop: the tool card kept its spinner and Allow/Deny buttons, and because the request stayed on the pending list every later message was refused with "Please approve or deny the pending tool before sending a new message" until the window was reloaded. Stopping (or reconnecting after the agent process restarts) now resolves those requests as cancelled. Thanks to @akramcodez. Closes #864. diff --git a/plugins/vscode/media/chat-panel.js b/plugins/vscode/media/chat-panel.js index 80d7dcc07..95acb1950 100644 --- a/plugins/vscode/media/chat-panel.js +++ b/plugins/vscode/media/chat-panel.js @@ -2160,7 +2160,7 @@ update.status === 'denied' || // ACP has no 'cancelled' status, so a cancel arrives as failed with // 'Cancelled by user'. Case-insensitive, or the capital C misses. - (update.status === 'failed' && update.rawOutput && typeof update.rawOutput === 'string' && /aborterror|cancelled/i.test(update.rawOutput)) + (update.status === 'failed' && update.rawOutput && typeof update.rawOutput === 'string' && /aborterror|cancelled|denied/i.test(update.rawOutput)) ) { statusEl.innerHTML = ICONS.cancelled; } else if (update.status === 'error' || update.status === 'failed') { diff --git a/plugins/vscode/src/acp-client.spec.ts b/plugins/vscode/src/acp-client.spec.ts index 01cf95be4..51ef880b3 100644 --- a/plugins/vscode/src/acp-client.spec.ts +++ b/plugins/vscode/src/acp-client.spec.ts @@ -50,6 +50,7 @@ test('NanocoderAcpClient - cancel resolves and clears pending permissions', asyn const requestPromise = client.handlePermissionRequest({ toolCall: { toolCallId: 'call_123', name: 'write_file', arguments: {} }, }); + t.true(client.hasPendingPermissions()); await client.cancel(); @@ -111,3 +112,19 @@ test('NanocoderAcpClient - a cancelled prompt does not raise an error toast', as t.regex(shownError ?? '', /RequestError/, 'A genuine failure must still surface'); }); + +test('NanocoderAcpClient - reconnecting clears permissions left by the dead process', async (t) => { + const outputChannel = { appendLine: () => {} } as any; + const stateManager = new AcpStateManager(); + const client = new NanocoderAcpClient(outputChannel, stateManager); + + const requestPromise = client.handlePermissionRequest({ + toolCall: { toolCallId: 'call_456', name: 'test_tool', arguments: {} }, + }); + + client.setConnection({} as any); + + const result = await requestPromise; + t.is((result as any).outcome.outcome, 'cancelled'); + t.false(client.hasPendingPermissions()); +}); diff --git a/plugins/vscode/src/acp-client.ts b/plugins/vscode/src/acp-client.ts index 028eb2928..0354c5160 100644 --- a/plugins/vscode/src/acp-client.ts +++ b/plugins/vscode/src/acp-client.ts @@ -92,6 +92,7 @@ export class NanocoderAcpClient { setConnection(connection: ClientSideConnection): void { this.connection = connection; this._sessionId = undefined; // Clear any stale session to force re-creation + this._clearPendingPermissions(); } async handlePermissionRequest(params: any): Promise { @@ -337,10 +338,10 @@ export class NanocoderAcpClient { } async cancel(): Promise { - if (!this.connection || !this._sessionId) return; this.cancelRequested = true; // Before the notification, so the map is emptied even if cancel() throws. this._clearPendingPermissions(); + if (!this.connection || !this._sessionId) return; try { await this.connection.cancel({ sessionId: this._sessionId diff --git a/source/acp/acp-agent.spec.ts b/source/acp/acp-agent.spec.ts index 576843758..9ee80143a 100644 --- a/source/acp/acp-agent.spec.ts +++ b/source/acp/acp-agent.spec.ts @@ -467,6 +467,44 @@ test('AcpAgent.cancel - aborts session for known session', async t => { t.pass(); }); +test('AcpAgent.cancel - stops a turn cancelled before the loop reads the signal', async t => { + const context = createMockInitContext(); + let chatCalls = 0; + (context.client as any).chat = async () => { + chatCalls++; + return {choices: [{message: {content: 'Test response'}}]}; + }; + const agent = new AcpAgent(context, createMockConn()); + const session = await agent.newSession({cwd: '/tmp'}); + + const turn = agent.prompt({ + sessionId: session.sessionId, + prompt: [{type: 'text', text: 'hi'}], + }); + await agent.cancel({sessionId: session.sessionId}); + + t.is((await turn).stopReason, 'cancelled'); + t.is(chatCalls, 0); +}); + +test('AcpAgent.prompt - a cancelled turn does not block the next prompt', async t => { + const {agent} = createAgent(); + const session = await agent.newSession({cwd: '/tmp'}); + + const cancelled = agent.prompt({ + sessionId: session.sessionId, + prompt: [{type: 'text', text: 'first'}], + }); + await agent.cancel({sessionId: session.sessionId}); + t.is((await cancelled).stopReason, 'cancelled'); + + const next = await agent.prompt({ + sessionId: session.sessionId, + prompt: [{type: 'text', text: 'second'}], + }); + t.is(next.stopReason, 'end_turn'); +}); + // ============================================================================ // setSessionMode() // ============================================================================ diff --git a/source/acp/acp-agent.ts b/source/acp/acp-agent.ts index be5c273f6..39d27ada3 100644 --- a/source/acp/acp-agent.ts +++ b/source/acp/acp-agent.ts @@ -164,6 +164,8 @@ export class AcpAgent implements Agent { ); } + session.beginTurn(); + const {text: userText, images} = await acpContentToUserMessage( params.prompt, { diff --git a/source/acp/acp-session.spec.ts b/source/acp/acp-session.spec.ts index 1b62e6651..28be5e4a6 100644 --- a/source/acp/acp-session.spec.ts +++ b/source/acp/acp-session.spec.ts @@ -104,7 +104,7 @@ test('AcpSession - cancel aborts the old controller', t => { t.true(oldController.signal.aborted); }); -test('AcpSession - cancel creates fresh abort controller', t => { +test('AcpSession - cancel leaves the session signal aborted', t => { const session = new AcpSession({ sessionId: 'test-id', cwd: '/tmp', @@ -112,10 +112,35 @@ test('AcpSession - cancel creates fresh abort controller', t => { }); const original = session.abortController; session.cancel(); - t.not(session.abortController, original); + t.is(session.abortController, original); + t.true(session.abortController.signal.aborted); +}); + +test('AcpSession - beginTurn creates fresh abort controller', t => { + const session = new AcpSession({ + sessionId: 'test-id', + cwd: '/tmp', + conn: createMockConn(), + }); + session.cancel(); + const cancelled = session.abortController; + session.beginTurn(); + t.not(session.abortController, cancelled); t.false(session.abortController.signal.aborted); }); +test('AcpSession - cancel after beginTurn aborts the turn controller', t => { + const session = new AcpSession({ + sessionId: 'test-id', + cwd: '/tmp', + conn: createMockConn(), + }); + session.beginTurn(); + const turnController = session.abortController; + session.cancel(); + t.true(turnController.signal.aborted); +}); + test('AcpSession - cancel can be called multiple times safely', t => { const session = new AcpSession({ sessionId: 'test-id', @@ -125,7 +150,7 @@ test('AcpSession - cancel can be called multiple times safely', t => { session.cancel(); session.cancel(); session.cancel(); - t.false(session.abortController.signal.aborted); + t.true(session.abortController.signal.aborted); }); // ============================================================================ diff --git a/source/acp/acp-session.ts b/source/acp/acp-session.ts index 46b88218f..7b6c310d3 100644 --- a/source/acp/acp-session.ts +++ b/source/acp/acp-session.ts @@ -35,7 +35,9 @@ export class AcpSession { cancel(): void { this.abortController.abort(); - // Create a fresh controller for potential subsequent prompts + } + + beginTurn(): void { this.abortController = new AbortController(); } }