Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-cancel-stops-execution.md
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 1 addition & 1 deletion plugins/vscode/media/chat-panel.js
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand Down
17 changes: 17 additions & 0 deletions plugins/vscode/src/acp-client.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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());
});
3 changes: 2 additions & 1 deletion plugins/vscode/src/acp-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<unknown> {
Expand Down Expand Up @@ -337,10 +338,10 @@ export class NanocoderAcpClient {
}

async cancel(): Promise<void> {
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
Expand Down
38 changes: 38 additions & 0 deletions source/acp/acp-agent.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
// ============================================================================
Expand Down
2 changes: 2 additions & 0 deletions source/acp/acp-agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ export class AcpAgent implements Agent {
);
}

session.beginTurn();

const {text: userText, images} = await acpContentToUserMessage(
params.prompt,
{
Expand Down
31 changes: 28 additions & 3 deletions source/acp/acp-session.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,18 +104,43 @@ 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',
conn: createMockConn(),
});
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',
Expand All @@ -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);
});

// ============================================================================
Expand Down
4 changes: 3 additions & 1 deletion source/acp/acp-session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Loading