From 968b562002de6882d546129e98afe3f713b42257 Mon Sep 17 00:00:00 2001 From: addyCooks <67361161+addyCooks@users.noreply.github.com> Date: Thu, 13 Aug 2026 02:26:53 +0530 Subject: [PATCH] fix: make the stop button end the turn it was pressed during MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AcpSession.cancel() aborted the live controller and immediately installed a fresh one. A cancel that arrived before the turn read the signal — while the agent was still resolving the prompt's file references — aborted a controller nobody would observe and handed the turn the replacement, so the run carried on. Rotate the controller when a turn begins instead, leaving cancel to abort whatever the current turn is using. The extension also never answered the agent's outstanding permission request on stop. The tool card kept its spinner and Allow/Deny buttons, and the entry stayed on the pending list, so every later prompt was refused with "approve or deny the pending tool" until the window was reloaded. Resolve those requests as cancelled on stop and on reconnect, and treat a failed tool update as terminal in the edit card. --- .changeset/fix-cancel-stops-execution.md | 5 ++++ plugins/vscode/media/chat-panel.js | 12 +++++--- plugins/vscode/src/acp-client.spec.ts | 17 +++++++++++ plugins/vscode/src/acp-client.ts | 3 +- source/acp/acp-agent.spec.ts | 38 ++++++++++++++++++++++++ source/acp/acp-agent.ts | 2 ++ source/acp/acp-session.spec.ts | 31 +++++++++++++++++-- source/acp/acp-session.ts | 4 ++- 8 files changed, 103 insertions(+), 9 deletions(-) create mode 100644 .changeset/fix-cancel-stops-execution.md 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 75b7f20d6..d6d53bff8 100644 --- a/plugins/vscode/media/chat-panel.js +++ b/plugins/vscode/media/chat-panel.js @@ -1635,7 +1635,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') { @@ -1822,10 +1822,14 @@ const statusEl = el.querySelector('.ml-auto'); if (statusEl) { if (update.status === 'success' || update.status === 'completed') statusEl.innerHTML = ICONS.success; - else if (update.status === 'error') statusEl.innerHTML = ICONS.error; - else if (update.status === 'cancelled' || update.status === 'denied') statusEl.innerHTML = ICONS.cancelled; + else if ( + update.status === 'cancelled' || + update.status === 'denied' || + (update.status === 'failed' && typeof update.rawOutput === 'string' && /aborterror|cancelled|denied/i.test(update.rawOutput)) + ) statusEl.innerHTML = ICONS.cancelled; + else if (update.status === 'error' || update.status === 'failed') statusEl.innerHTML = ICONS.error; } - if (update.status === 'success' || update.status === 'completed' || update.status === 'error' || update.status === 'cancelled' || update.status === 'denied') { + if (update.status === 'success' || update.status === 'completed' || update.status === 'error' || update.status === 'failed' || update.status === 'cancelled' || update.status === 'denied') { const actions = el.querySelector('.tool-actions'); if (actions) actions.remove(); } 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(); } }