From d6fad341534a58e776c3b2bf87228f02448cfcef Mon Sep 17 00:00:00 2001 From: sgaabdu4 Date: Sat, 28 Mar 2026 04:23:54 +0000 Subject: [PATCH 1/2] fix: prevent CancellationError from killing ask_user tool loop When ask_user was internally superseded (e.g., a new request arrived while a previous one was pending), the extension threw vscode.CancellationError. VS Code's ToolCallingLoop treats CancellationError as a hard stop at three levels: buildPrompt2 isCancelled check, _runLoop catch/break, and shouldAutoRetry exclusion. After two cancellations the LLM stopped calling ask_user entirely. Changes: - tools.ts: return a normal AskUserToolResult for superseded requests instead of throwing CancellationError. Real token cancellation (Stop button) still throws correctly via createCancellationPromise. - toolCallHandler.ts: update superseded message to instruct the LLM to re-ask the exact same question on retry. - tools.test.ts: update existing tests and add new test for real token cancellation still throwing CancellationError. --- tasksync-chat/src/tools.test.ts | 74 ++++++++++++++++---- tasksync-chat/src/tools.ts | 14 +++- tasksync-chat/src/webview/toolCallHandler.ts | 3 +- 3 files changed, 73 insertions(+), 18 deletions(-) diff --git a/tasksync-chat/src/tools.test.ts b/tasksync-chat/src/tools.test.ts index 23bfcac..0cbbcd5 100644 --- a/tasksync-chat/src/tools.test.ts +++ b/tasksync-chat/src/tools.test.ts @@ -290,40 +290,55 @@ beforeEach(() => { }); /** - * Verify that cancelled ask_user results stop the LM turn instead of becoming fake replies. + * Verify that cancelled ask_user results are returned as normal text + * (NOT CancellationError) so the LLM can see the message and call ask_user again. + * CancellationError kills the ToolCallingLoop — only real token cancellation should throw it. */ describe("askUser cancellation handling", () => { /** - * A cancelled provider result must propagate as a cancellation error. + * A superseded provider result must return as normal text, not throw. */ - it("throws CancellationError when waitForUserResponse returns a cancelled result", async () => { + it("returns cancelled message as normal result when waitForUserResponse returns a cancelled result", async () => { const { askUser } = await import("./tools"); const provider = { waitForUserResponse: vi.fn().mockResolvedValue({ - value: "[Session reset by user]", + value: + "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]", attachments: [], queue: false, cancelled: true, }), }; - await expect( - askUser({ question: "Reset?" }, provider as any, createToken() as any), - ).rejects.toBeInstanceOf(MockCancellationError); + const result = await askUser( + { question: "Reset?" }, + provider as any, + createToken() as any, + ); + expect(result.response).toBe( + "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]", + ); + expect(result.attachments).toEqual([]); + expect(result.queue).toBe(false); }); /** - * The registered LM tool must rethrow cancellation so Copilot stops the old turn cleanly. + * The registered LM tool must return a LanguageModelToolResult (not throw) + * for superseded requests so the LLM sees the message. */ - it("rethrows CancellationError from the registered tool invoke handler", async () => { + it("returns LanguageModelToolResult for superseded requests from the registered tool invoke handler", async () => { const { registerTools } = await import("./tools"); const provider = { waitForUserResponse: vi.fn().mockResolvedValue({ - value: "[Session reset by user]", + value: + "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]", attachments: [], queue: false, cancelled: true, }), + _autoAppendEnabled: false, + _autoAppendText: "", + _alwaysAppendReminder: false, }; const context = { subscriptions: [] as unknown[] }; @@ -332,12 +347,41 @@ describe("askUser cancellation handling", () => { const toolDefinition = registerToolMock.mock.calls[0]?.[1]; expect(toolDefinition).toBeTruthy(); + const result = await toolDefinition.invoke( + { input: { question: "Reset?" } }, + createToken() as any, + ); + // Should return a result, not throw + expect(result).toBeTruthy(); + expect(result.parts).toBeDefined(); + expect(result.parts.length).toBe(1); + // The response text should contain the cancelled message + const textPart = result.parts[0]; + const parsed = JSON.parse(textPart.value); + expect(parsed.response).toBe( + "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]", + ); + expect(showErrorMessageMock).not.toHaveBeenCalled(); + }); + + /** + * Real token cancellation (CancellationToken fires) must still throw CancellationError. + */ + it("throws CancellationError when token is already cancelled before starting", async () => { + const { askUser } = await import("./tools"); + const provider = { + waitForUserResponse: vi.fn(), + }; + + const cancelledToken = { + isCancellationRequested: true, + onCancellationRequested: vi.fn(() => ({ + dispose: vi.fn(), + })), + }; + await expect( - toolDefinition.invoke( - { input: { question: "Reset?" } }, - createToken() as any, - ), + askUser({ question: "Test?" }, provider as any, cancelledToken as any), ).rejects.toBeInstanceOf(MockCancellationError); - expect(showErrorMessageMock).not.toHaveBeenCalled(); }); }); diff --git a/tasksync-chat/src/tools.ts b/tasksync-chat/src/tools.ts index 2bb3a79..1b7cbc5 100644 --- a/tasksync-chat/src/tools.ts +++ b/tasksync-chat/src/tools.ts @@ -103,13 +103,23 @@ export async function askUser( cancellation.promise, ]); - // Handle case where request was superseded by another call + // Handle case where request was superseded by another call. + // IMPORTANT: Do NOT throw CancellationError here — that kills the entire + // ToolCallingLoop in VS Code's Copilot Chat (buildPrompt2 checks isCancelled + // on tool results and throws CancellationError, _runLoop catches it and breaks). + // Instead, return the cancelled message as a normal result so the LLM sees it + // and can call ask_user again. Real user cancellation (Stop button) is handled + // by the CancellationToken/createCancellationPromise path which correctly throws. if (result.cancelled) { debugLog( "[TaskSync] askUser — superseded/cancelled, response:", result.value.slice(0, 80), ); - throw new vscode.CancellationError(); + return { + response: result.value, + attachments: [], + queue: result.queue, + }; } debugLog( "[TaskSync] askUser — user responded:", diff --git a/tasksync-chat/src/webview/toolCallHandler.ts b/tasksync-chat/src/webview/toolCallHandler.ts index 3450c39..8ba0f10 100644 --- a/tasksync-chat/src/webview/toolCallHandler.ts +++ b/tasksync-chat/src/webview/toolCallHandler.ts @@ -44,7 +44,8 @@ export function cancelSupersededPendingRequest(p: P): void { const oldResolve = p._pendingRequests.get(oldToolCallId); if (oldResolve) { oldResolve({ - value: "[CANCELLED: New request superseded this one]", + value: + "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]", queue: hasQueuedItems(p), attachments: [], cancelled: true, From a5c0b76bf0dcc646fbf5690670f89702650892cc Mon Sep 17 00:00:00 2001 From: sgaabdu4 Date: Sat, 28 Mar 2026 04:35:53 +0000 Subject: [PATCH 2/2] refactor: extract ASKUSER_SUPERSEDED_MESSAGE constant, add mid-flight cancellation test Address PR review comments: - Extract superseded message to shared constant in remoteConstants.ts (DRY) - Import constant in toolCallHandler.ts and tools.test.ts - Add test for mid-flight token cancellation (createCancellationPromise race path) --- .../src/constants/remoteConstants.ts | 5 ++ tasksync-chat/src/tools.test.ts | 54 +++++++++++++++---- tasksync-chat/src/webview/toolCallHandler.ts | 4 +- 3 files changed, 50 insertions(+), 13 deletions(-) diff --git a/tasksync-chat/src/constants/remoteConstants.ts b/tasksync-chat/src/constants/remoteConstants.ts index 4de57db..5200e60 100644 --- a/tasksync-chat/src/constants/remoteConstants.ts +++ b/tasksync-chat/src/constants/remoteConstants.ts @@ -57,6 +57,11 @@ export const HUMAN_DELAY_MIN_UPPER = 30; // maximum allowed for "min delay" inpu export const HUMAN_DELAY_MAX_LOWER = 2; // minimum allowed for "max delay" input export const HUMAN_DELAY_MAX_UPPER = 60; // maximum allowed for "max delay" input +// Superseded ask_user directive — returned as normal tool result (not CancellationError) +// so the ToolCallingLoop keeps running. Instructs the LLM to re-ask the same question. +export const ASKUSER_SUPERSEDED_MESSAGE = + "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]"; + // Shared askUser prompt fragments (used by both local and remote session starts) export const ASKUSER_VISIBILITY_TEXT = "The user can ONLY see messages you send via the #askUser tool — your normal chat responses are invisible to them."; diff --git a/tasksync-chat/src/tools.test.ts b/tasksync-chat/src/tools.test.ts index 0cbbcd5..f6370b6 100644 --- a/tasksync-chat/src/tools.test.ts +++ b/tasksync-chat/src/tools.test.ts @@ -1,5 +1,8 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; -import { AUTO_APPEND_DEFAULT_TEXT } from "./constants/remoteConstants"; +import { + ASKUSER_SUPERSEDED_MESSAGE, + AUTO_APPEND_DEFAULT_TEXT, +} from "./constants/remoteConstants"; import { buildFinalResponse } from "./tools"; const { @@ -302,8 +305,7 @@ describe("askUser cancellation handling", () => { const { askUser } = await import("./tools"); const provider = { waitForUserResponse: vi.fn().mockResolvedValue({ - value: - "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]", + value: ASKUSER_SUPERSEDED_MESSAGE, attachments: [], queue: false, cancelled: true, @@ -315,9 +317,7 @@ describe("askUser cancellation handling", () => { provider as any, createToken() as any, ); - expect(result.response).toBe( - "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]", - ); + expect(result.response).toBe(ASKUSER_SUPERSEDED_MESSAGE); expect(result.attachments).toEqual([]); expect(result.queue).toBe(false); }); @@ -330,8 +330,7 @@ describe("askUser cancellation handling", () => { const { registerTools } = await import("./tools"); const provider = { waitForUserResponse: vi.fn().mockResolvedValue({ - value: - "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]", + value: ASKUSER_SUPERSEDED_MESSAGE, attachments: [], queue: false, cancelled: true, @@ -358,9 +357,7 @@ describe("askUser cancellation handling", () => { // The response text should contain the cancelled message const textPart = result.parts[0]; const parsed = JSON.parse(textPart.value); - expect(parsed.response).toBe( - "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]", - ); + expect(parsed.response).toBe(ASKUSER_SUPERSEDED_MESSAGE); expect(showErrorMessageMock).not.toHaveBeenCalled(); }); @@ -384,4 +381,39 @@ describe("askUser cancellation handling", () => { askUser({ question: "Test?" }, provider as any, cancelledToken as any), ).rejects.toBeInstanceOf(MockCancellationError); }); + + /** + * Mid-flight cancellation: token fires while waitForUserResponse is still pending. + * The createCancellationPromise race must cause askUser to reject with CancellationError. + */ + it("throws CancellationError when token fires mid-flight during waitForUserResponse", async () => { + const { askUser } = await import("./tools"); + + // waitForUserResponse never resolves — simulates the user hasn't responded yet + const provider = { + waitForUserResponse: vi.fn(() => new Promise(() => {})), + }; + + // Capture the onCancellationRequested callback so we can fire it manually + let cancelCallback: (() => void) | undefined; + const token = { + isCancellationRequested: false, + onCancellationRequested: vi.fn((cb: () => void) => { + cancelCallback = cb; + return { dispose: vi.fn() }; + }), + }; + + const promise = askUser( + { question: "Pending?" }, + provider as any, + token as any, + ); + + // Fire the cancellation callback to simulate the Stop button + expect(cancelCallback).toBeDefined(); + cancelCallback!(); + + await expect(promise).rejects.toBeInstanceOf(MockCancellationError); + }); }); diff --git a/tasksync-chat/src/webview/toolCallHandler.ts b/tasksync-chat/src/webview/toolCallHandler.ts index 8ba0f10..2b78caa 100644 --- a/tasksync-chat/src/webview/toolCallHandler.ts +++ b/tasksync-chat/src/webview/toolCallHandler.ts @@ -4,6 +4,7 @@ */ import * as vscode from "vscode"; import { + ASKUSER_SUPERSEDED_MESSAGE, CONFIG_SECTION, DEFAULT_MAX_CONSECUTIVE_AUTO_RESPONSES, } from "../constants/remoteConstants"; @@ -44,8 +45,7 @@ export function cancelSupersededPendingRequest(p: P): void { const oldResolve = p._pendingRequests.get(oldToolCallId); if (oldResolve) { oldResolve({ - value: - "[CANCELLED: This ask_user request was superseded internally. When you call ask_user again, re-ask the exact same question — do not rephrase or summarize differently.]", + value: ASKUSER_SUPERSEDED_MESSAGE, queue: hasQueuedItems(p), attachments: [], cancelled: true,