From 46f6b95f3fc0295617c5bade644976ef368b29f5 Mon Sep 17 00:00:00 2001 From: Tom Owers Date: Mon, 7 Sep 2026 13:56:05 +0200 Subject: [PATCH] fix(wizard-ask): return an explicit cancellation outcome, not a sentinel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cancelled or timed-out `wizard_ask` came back as `{ answers: { host: "__cancelled__" } }` and nothing else. The sentinel is not named anywhere in the tool schema, and a prompt the user dismissed is byte-identical to one nobody answered before it timed out. Both facades now return a `cancelled` envelope alongside `answers`: the question ids that were not collected, whether the user dismissed the prompt or it timed out, and what to do next. The bridge reports `timedOut` — the one fact only it holds — and the shared descriptor builds the envelope, so the MCP server and the pi-native tool cannot drift. Generated-By: PostHog Desktop Task-Id: 2e6fcdd3-5969-4b4d-ab93-a70ef50ce512 --- src/lib/__tests__/wizard-ask-bridge.test.ts | 29 +++++++- src/lib/__tests__/wizard-tools.test.ts | 59 +++++++++++++++ .../runner/harness/pi/__tests__/tools.test.ts | 61 ++++++++++++++-- src/lib/agent/runner/harness/pi/tools.ts | 18 ++++- src/lib/wizard-ask-bridge.ts | 30 +++++--- src/lib/wizard-tools/mcp.ts | 15 +++- src/lib/wizard-tools/tools.ts | 72 ++++++++++++++++++- 7 files changed, 261 insertions(+), 23 deletions(-) diff --git a/src/lib/__tests__/wizard-ask-bridge.test.ts b/src/lib/__tests__/wizard-ask-bridge.test.ts index f86e31b6..1f12b9a4 100644 --- a/src/lib/__tests__/wizard-ask-bridge.test.ts +++ b/src/lib/__tests__/wizard-ask-bridge.test.ts @@ -48,7 +48,8 @@ describe('createWizardAskBridge', () => { resolveAnswers({ goal: 'Help users find the export button' }); await expect(requestPromise).resolves.toEqual({ - goal: 'Help users find the export button', + answers: { goal: 'Help users find the export button' }, + timedOut: false, }); }); @@ -167,6 +168,25 @@ describe('createWizardAskBridge', () => { }); }); + it('reports a user-dismissed ask as cancelled but not timed out', async () => { + // The two arrive identically in `answers`, so `timedOut` is the only thing + // that tells the tool facades a decline from an unattended terminal. + const bridge = createWizardAskBridge({ + getSource: () => 'product-tours', + showQuestion: () => Promise.resolve({ host: CANCELLED_SENTINEL }), + timeoutMs: 60_000, + }); + + await expect( + bridge.request({ + questions: [{ id: 'host', prompt: 'Host?', kind: 'text' }], + }), + ).resolves.toEqual({ + answers: { host: CANCELLED_SENTINEL }, + timedOut: false, + }); + }); + describe('isFullyCancelled', () => { // Gates the per-run cap refund in wizard-tools: a fully cancelled ask must // not burn a wizard_ask slot, while any real answer must still count. @@ -214,8 +234,11 @@ describe('createWizardAskBridge', () => { vi.advanceTimersByTime(1000); await expect(promise).resolves.toEqual({ - goal: CANCELLED_SENTINEL, - audience: CANCELLED_SENTINEL, + answers: { + goal: CANCELLED_SENTINEL, + audience: CANCELLED_SENTINEL, + }, + timedOut: true, }); // Without this, the host's pending-question state survives the diff --git a/src/lib/__tests__/wizard-tools.test.ts b/src/lib/__tests__/wizard-tools.test.ts index 901a1d02..97e5fbba 100644 --- a/src/lib/__tests__/wizard-tools.test.ts +++ b/src/lib/__tests__/wizard-tools.test.ts @@ -5,7 +5,9 @@ import * as path from 'path'; import { zipSync } from 'fflate'; import { ASK_BATCH_THRESHOLD, + ASK_CANCELLED_NOTE, ASK_SUBJECT_UNSPECIFIED, + ASK_TIMED_OUT_NOTE, DEFAULT_ASK_MAX_QUESTIONS, WIZARD_ASK_SUBJECT_DESCRIPTION, WIZARD_ASK_TOOL_DESCRIPTION, @@ -16,6 +18,7 @@ import { createAskAccounting, downloadSkill, ensureGitignoreCoverage, + describeAskCancellation, evaluateAskCap, fetchSkillMenu, mergeEnvValues, @@ -924,6 +927,13 @@ describe('wizard_ask shared descriptions', () => { ); }); + it('points the agent at the cancellation envelope rather than the answer values', () => { + expect(WIZARD_ASK_TOOL_DESCRIPTION).toMatch(/`cancelled` object/); + expect(WIZARD_ASK_TOOL_DESCRIPTION).toMatch( + /instead of inspecting the answer values/, + ); + }); + it('explains what a subject is and what omitting it costs', () => { expect(WIZARD_ASK_SUBJECT_DESCRIPTION).toMatch(/Postgres/); expect(WIZARD_ASK_SUBJECT_DESCRIPTION).toMatch(/consecutive calls/i); @@ -931,6 +941,55 @@ describe('wizard_ask shared descriptions', () => { }); }); +describe('describeAskCancellation', () => { + const CANCELLED = '__cancelled__'; + + it('is undefined when every question was answered', () => { + expect( + describeAskCancellation( + { host: 'db.example.com', ssl: ['require'] }, + false, + ), + ).toBeUndefined(); + }); + + it('names the uncollected questions and reads a dismissal as a decline', () => { + expect( + describeAskCancellation({ host: CANCELLED, password: CANCELLED }, false), + ).toEqual({ + reason: 'user-cancelled', + questionIds: ['host', 'password'], + note: ASK_CANCELLED_NOTE, + }); + }); + + it('separates a timed-out prompt from a dismissed one', () => { + expect(describeAskCancellation({ host: CANCELLED }, true)).toEqual({ + reason: 'timed-out', + questionIds: ['host'], + note: ASK_TIMED_OUT_NOTE, + }); + }); + + it('reports a partly answered ask, and never counts a vaulted answer as cancelled', () => { + expect( + describeAskCancellation( + { + host: 'db.example.com', + password: { secretRef: 'secret:abc' }, + tunnel: CANCELLED, + }, + false, + ), + ).toMatchObject({ reason: 'user-cancelled', questionIds: ['tunnel'] }); + }); + + it('tells a dismissal to fall back and a timeout to stop asking', () => { + expect(ASK_CANCELLED_NOTE).toMatch(/do not re-ask/i); + expect(ASK_TIMED_OUT_NOTE).toMatch(/stop asking/i); + }); +}); + describe('extractZipArchive', () => { let dest: string; diff --git a/src/lib/agent/runner/harness/pi/__tests__/tools.test.ts b/src/lib/agent/runner/harness/pi/__tests__/tools.test.ts index 1b1d88d9..c380b14d 100644 --- a/src/lib/agent/runner/harness/pi/__tests__/tools.test.ts +++ b/src/lib/agent/runner/harness/pi/__tests__/tools.test.ts @@ -17,6 +17,8 @@ import { evaluateToolCall } from '../security'; import { allowedPiCodingTools, allowedOrchestratorTools } from '../task'; import { ASK_BATCH_THRESHOLD, + ASK_CANCELLED_NOTE, + ASK_TIMED_OUT_NOTE, WIZARD_ASK_SENSITIVE_DESCRIPTION, WIZARD_ASK_SUBJECT_DESCRIPTION, WIZARD_ASK_TOOL_DESCRIPTION, @@ -27,8 +29,9 @@ const SECRET = 'phx_live_zendesk_token_123'; const makeTools = ( answers: Record, maxQuestions?: number, + timedOut = false, ) => { - const request = vi.fn().mockResolvedValue(answers); + const request = vi.fn().mockResolvedValue({ answers, timedOut }); const workingDirectory = mkdtempSync(join(tmpdir(), 'pi-tools-vault-')); const tools = createWizardPiTools({ workingDirectory, @@ -92,6 +95,54 @@ describe('pi wizard_ask — sensitive answers are vaulted', () => { expect(answers.token).toBe(CANCELLED_SENTINEL); }); + it('names the cancellation explicitly instead of leaving the sentinel to be read', async () => { + // The agent's only signal used to be the sentinel string inside `answers`, + // which says neither "this was not collected" nor who ended the prompt. + const { wizardAsk } = makeTools({ + host: CANCELLED_SENTINEL, + password: CANCELLED_SENTINEL, + }); + const result = await call(wizardAsk, { + questions: [ + { id: 'host', prompt: 'Host', kind: 'text' }, + { id: 'password', prompt: 'Password', kind: 'text', sensitive: true }, + ], + subject: 'Postgres', + }); + const { cancelled } = JSON.parse(textOf(result)) as { + cancelled: { reason: string; questionIds: string[]; note: string }; + }; + expect(cancelled.reason).toBe('user-cancelled'); + expect(cancelled.questionIds).toEqual(['host', 'password']); + expect(cancelled.note).toBe(ASK_CANCELLED_NOTE); + }); + + it('distinguishes a timed-out prompt from a dismissed one', async () => { + // A timeout means nobody is reading the terminal, so every later prompt in + // the run costs another full timeout before it fails the same way. + const { wizardAsk } = makeTools( + { host: CANCELLED_SENTINEL }, + undefined, + true, + ); + const result = await call(wizardAsk, { + questions: [{ id: 'host', prompt: 'Host', kind: 'text' }], + }); + const { cancelled } = JSON.parse(textOf(result)) as { + cancelled: { reason: string; note: string }; + }; + expect(cancelled.reason).toBe('timed-out'); + expect(cancelled.note).toBe(ASK_TIMED_OUT_NOTE); + }); + + it('carries no cancellation envelope when every question was answered', async () => { + const { wizardAsk } = makeTools({ host: 'db.example.com' }); + const result = await call(wizardAsk, { + questions: [{ id: 'host', prompt: 'Host', kind: 'text' }], + }); + expect(JSON.parse(textOf(result))).not.toHaveProperty('cancelled'); + }); + it('still rejects sensitive=true on non-text kinds', async () => { const { wizardAsk, request } = makeTools({}); const result = await call(wizardAsk, { @@ -400,9 +451,11 @@ describe('pi task wiring — wizard_ask pauses Write/Edit', () => { let release!: (answers: Record) => void; const request = vi.fn( () => - new Promise>((resolve) => { - release = resolve; - }), + new Promise<{ answers: Record; timedOut: boolean }>( + (resolve) => { + release = (answers) => resolve({ answers, timedOut: false }); + }, + ), ); const [wizardAsk] = createWizardPiTools({ workingDirectory: mkdtempSync(join(tmpdir(), 'pi-ask-pause-')), diff --git a/src/lib/agent/runner/harness/pi/tools.ts b/src/lib/agent/runner/harness/pi/tools.ts index 35b5aa8c..578d583e 100644 --- a/src/lib/agent/runner/harness/pi/tools.ts +++ b/src/lib/agent/runner/harness/pi/tools.ts @@ -26,6 +26,7 @@ import { WIZARD_TOOL_NAMES, checkEnvKeys as checkEnvKeysCore, createAskAccounting, + describeAskCancellation, fetchSkillMenu, installSkillById, mergeEnvValues, @@ -376,7 +377,7 @@ export function createWizardPiTools(ctx: PiToolsContext): ToolDefinition[] { // mutate files while it's waiting on the user's answer. onAskPendingChange?.(true); try { - const answers = await askBridge.request({ + const { answers, timedOut } = await askBridge.request({ questions: args.questions, subject: normaliseAskSubject(args.subject), }); @@ -388,12 +389,23 @@ export function createWizardPiTools(ctx: PiToolsContext): ToolDefinition[] { answers, secretVault, ); + // State an uncollected field as an outcome rather than leaving the + // agent to recognise a sentinel answer value (same as the MCP facade). + const cancelled = describeAskCancellation(sanitised, timedOut); logToFile( `[pi] wizard_ask: resolved ${ Object.keys(answers).length - } answer(s) for ${args.questions.length} question(s)`, + } answer(s) for ${args.questions.length} question(s)${ + cancelled ? `, cancelled: ${cancelled.reason}` : '' + }`, + ); + return text( + JSON.stringify( + { answers: sanitised, ...(cancelled ? { cancelled } : {}) }, + null, + 2, + ), ); - return text(JSON.stringify({ answers: sanitised }, null, 2)); } catch (err) { askAccounting.refund(args.subject); const message = err instanceof Error ? err.message : String(err); diff --git a/src/lib/wizard-ask-bridge.ts b/src/lib/wizard-ask-bridge.ts index b0d4c4a0..98cf5666 100644 --- a/src/lib/wizard-ask-bridge.ts +++ b/src/lib/wizard-ask-bridge.ts @@ -34,13 +34,25 @@ export interface WizardAskRequest { subject?: string; } +/** + * One ask's outcome. + * + * `answers` holds one answer per question id (string for `single`/`text`, + * string[] for `multi`); cancelled fields come back as the literal + * `"__cancelled__"`. `timedOut` records that the per-question timeout, rather + * than the user, ended the request — the one fact only the bridge holds, and + * the difference between "the user said no to this" and "nobody is at the + * terminal any more". Both arrive as {@link CANCELLED_SENTINEL} answers, so + * without it the two are indistinguishable to the tool facades and to the agent. + */ +export interface AskResponse { + answers: AskAnswers; + timedOut: boolean; +} + export interface WizardAskBridge { - /** - * Open the WizardAsk overlay and resolve with the user's answers. - * One answer per question id (string for `single`/`text`, string[] for - * `multi`). Cancelled fields come back as the literal `"__cancelled__"`. - */ - request(req: WizardAskRequest): Promise; + /** Open the WizardAsk overlay and resolve with the user's answers. */ + request(req: WizardAskRequest): Promise; } export interface WizardAskBridgeOptions { @@ -107,6 +119,7 @@ export function createWizardAskBridge( const startedAt = Date.now(); let timer: ReturnType | undefined; + let timedOut = false; // Race the user against the timeout. Whichever fires first wins. On // timeout we also cancel the host's overlay: resolving our side alone @@ -114,6 +127,7 @@ export function createWizardAskBridge( // wizard_ask would be rejected as a duplicate request. const timeoutPromise = new Promise((resolve) => { timer = setTimeout(() => { + timedOut = true; opts.cancelQuestion?.(); resolve(buildCancelledAnswers(questions)); }, timeoutMs); @@ -132,7 +146,7 @@ export function createWizardAskBridge( subject, question_count: questions.length, duration_ms: durationMs, - timed_out: durationMs >= timeoutMs, + timed_out: timedOut, }); } else { analytics.wizardCapture('wizard_ask answered', { @@ -143,7 +157,7 @@ export function createWizardAskBridge( }); } - return answers; + return { answers, timedOut }; } finally { if (timer) clearTimeout(timer); } diff --git a/src/lib/wizard-tools/mcp.ts b/src/lib/wizard-tools/mcp.ts index ef7a038e..2d0b4ca6 100644 --- a/src/lib/wizard-tools/mcp.ts +++ b/src/lib/wizard-tools/mcp.ts @@ -44,6 +44,7 @@ import { downloadSkill, ensureGitignoreCoverage, createAskAccounting, + describeAskCancellation, fetchSkillMenu, checkEnvKeys as checkEnvKeysCore, mergeEnvValues, @@ -738,7 +739,7 @@ export async function createWizardToolsServer(options: WizardToolsOptions) { askAccounting.record(args.subject); try { - const answers = await askBridge.request({ + const { answers, timedOut } = await askBridge.request({ questions: args.questions, subject: normaliseAskSubject(args.subject), }); @@ -759,16 +760,24 @@ export async function createWizardToolsServer(options: WizardToolsOptions) { secretVault, ); + // State an uncollected field as an outcome rather than leaving the + // agent to recognise a sentinel answer value (same as the pi facade). + const cancelled = describeAskCancellation(sanitised, timedOut); + logToFile( `wizard_ask: resolved ${Object.keys(answers).length} answer(s) for ${ args.questions.length - } question(s)`, + } question(s)${cancelled ? `, cancelled: ${cancelled.reason}` : ''}`, ); return { content: [ { type: 'text' as const, - text: JSON.stringify({ answers: sanitised }, null, 2), + text: JSON.stringify( + { answers: sanitised, ...(cancelled ? { cancelled } : {}) }, + null, + 2, + ), }, ], }; diff --git a/src/lib/wizard-tools/tools.ts b/src/lib/wizard-tools/tools.ts index 25b54bc0..b6fd661b 100644 --- a/src/lib/wizard-tools/tools.ts +++ b/src/lib/wizard-tools/tools.ts @@ -389,8 +389,76 @@ export const WIZARD_ASK_TOOL_DESCRIPTION = 'one call per data-warehouse source, one call per integration step — is ' + 'expected and is never blocked, because the batching guard counts consecutive ' + 'calls per subject. A fully cancelled or timed-out response does NOT count ' + - 'against the per-run cap — treat it as "the user declined" and fall back ' + - 'gracefully (e.g. hand over a deep link) without worrying about a wasted call.'; + 'against the per-run cap, so nothing is wasted. When a field is not ' + + 'collected the result carries a `cancelled` object naming those question ' + + 'ids, whether the user dismissed the prompt or it timed out, and what to do ' + + 'next: read that instead of inspecting the answer values, and fall back ' + + 'gracefully (e.g. hand over a deep link) rather than re-asking.'; + +/** + * Guidance returned with a `wizard_ask` result when the user dismissed the + * prompt. Shared by both harness facades, like the descriptions above. + * + * Deliberately says nothing about how the caller reports its own outcome: + * `wizard_ask` serves programs with no task queue as well as the orchestrator's + * seeded ones, so the note covers the ask and only the ask. + */ +export const ASK_CANCELLED_NOTE = + 'The user dismissed this prompt, so none of these fields were collected. ' + + 'Read it as a decline for this subject: do not re-ask the same questions. ' + + 'Fall back to a route that needs no answer from them (for example, hand ' + + 'them a link to finish it themselves) and carry on with the rest of your ' + + 'work. Asking about a different subject is still fine.'; + +/** + * Guidance returned with a `wizard_ask` result when the prompt timed out. + * + * A timeout is not one decline: it says nobody is reading the terminal, and + * every later prompt in the run will end the same way after the same wait. + * Naming that is the difference between falling back once and stopping the run + * behind one unattended prompt per remaining item. + */ +export const ASK_TIMED_OUT_NOTE = + 'This prompt timed out with no answer, so the user is most likely away from ' + + 'the terminal. Read it as a decline, and expect any further prompt in this ' + + 'run to time out the same way after the same wait: stop asking and finish ' + + 'without them — hand over links for everything still outstanding — rather ' + + 'than opening another prompt.'; + +/** + * The explicit outcome returned alongside `answers` when an ask collected + * nothing for one or more of its questions. + * + * Cancelled fields arrive inside `answers` as the {@link CANCELLED_SENTINEL} + * string, which an agent can only recognise if it already knows the sentinel, + * and which says nothing about who ended the prompt. Both facades return this + * envelope so the outcome is stated rather than encoded in an answer value. + */ +export type AskCancellation = { + reason: 'user-cancelled' | 'timed-out'; + /** Ids of the questions that came back uncollected. */ + questionIds: string[]; + /** What the agent should do next, given the reason. */ + note: string; +}; + +/** + * Describe an ask's cancelled fields, or `undefined` when every question was + * answered. `timedOut` comes from the ask bridge — it is the only thing that + * tells a dismissed prompt from an unattended one. + */ +export function describeAskCancellation( + answers: Record, + timedOut: boolean, +): AskCancellation | undefined { + const questionIds = Object.entries(answers) + .filter(([, value]) => value === CANCELLED_SENTINEL) + .map(([id]) => id); + if (questionIds.length === 0) return undefined; + return timedOut + ? { reason: 'timed-out', questionIds, note: ASK_TIMED_OUT_NOTE } + : { reason: 'user-cancelled', questionIds, note: ASK_CANCELLED_NOTE }; +} export type AskCapDecision = | { kind: 'ok' }