From 0dfc99daf2e370c6dd51e0e1899888e5debfdfab Mon Sep 17 00:00:00 2001 From: Kaiyi Date: Wed, 8 Jul 2026 18:13:47 +0800 Subject: [PATCH] fix(agent-core-v2): align AskUserQuestion tool chain with v1 - translate wire ids back to question text / option labels when resolving a question over REST, joining multi-select labels with ', ' - enforce unique question texts / option labels and non-empty strings at both the schema and the execution path - cancel pending questions on turn abort or background task stop by dismissing the parked entry (resolves null, v1 broker semantics) - restore the unsupported-client fallback and dismissed-error handling - pass empty header / option description through verbatim and align the model-facing tool description byte-for-byte with v1 - drop the synthetic expires_at field from the question wire shape --- .changeset/ask-user-question-v2-parity.md | 6 + .../src/agent/questionTools/tools/ask-user.md | 4 +- .../src/agent/questionTools/tools/ask-user.ts | 137 +++-- .../src/session/question/question.ts | 16 +- .../src/session/question/questionService.ts | 28 +- .../test/question/question.test.ts | 46 ++ .../test/questionTools/ask-user.test.ts | 574 ++++++++++++++++++ packages/kap-server/src/routes/questions.ts | 83 ++- packages/kap-server/test/questions.test.ts | 102 +++- 9 files changed, 925 insertions(+), 71 deletions(-) create mode 100644 .changeset/ask-user-question-v2-parity.md create mode 100644 packages/agent-core-v2/test/questionTools/ask-user.test.ts diff --git a/.changeset/ask-user-question-v2-parity.md b/.changeset/ask-user-question-v2-parity.md new file mode 100644 index 000000000..98d563b8a --- /dev/null +++ b/.changeset/ask-user-question-v2-parity.md @@ -0,0 +1,6 @@ +--- +"@moonshot-ai/agent-core-v2": patch +"@moonshot-ai/kap-server": patch +--- + +Fix the v2 AskUserQuestion flow: answers now come back keyed by question text with option labels as values, aborting a turn or stopping a background question dismisses the pending question instead of leaking it, and duplicate question texts or option labels are rejected before the question is shown. The pending-question wire shape no longer carries a synthetic expires_at field. diff --git a/packages/agent-core-v2/src/agent/questionTools/tools/ask-user.md b/packages/agent-core-v2/src/agent/questionTools/tools/ask-user.md index 68e394aec..7e9f0f7a8 100644 --- a/packages/agent-core-v2/src/agent/questionTools/tools/ask-user.md +++ b/packages/agent-core-v2/src/agent/questionTools/tools/ask-user.md @@ -15,6 +15,8 @@ Overusing this tool interrupts the user's flow. Only use it when the user's inpu - Use multi_select to allow multiple answers to be selected for a question - Keep option labels concise (1-5 words), use descriptions for trade-offs and details - Each question should have 2-4 meaningful, distinct options +- Question texts must be unique across the call, and option labels must be unique within each question - You can ask 1-4 questions at a time; group related questions to minimize interruptions - If you recommend a specific option, list it first and append "(Recommended)" to its label -- Set background=true when you can keep working without the answer. This starts a background question task and returns a task_id immediately. The answer arrives automatically in a later turn — you do not need to poll, sleep, or check on it. Continue with other work; never fabricate or predict the answer. +- The result is JSON with an `answers` object keyed by question text; each value is the chosen option's label (comma-separated labels for multi_select, or the user's own words if they picked "Other"); if `answers` is empty and a `note` says the user dismissed it, they declined to answer — proceed with your best judgment and do not re-ask the same question +- Set background=true when you can keep working without the answer. This starts a background question task and returns a task_id immediately. The answer arrives automatically in a later turn — you do not need to poll, sleep, or check on it. Continue with other work; never fabricate or predict the answer. \ No newline at end of file diff --git a/packages/agent-core-v2/src/agent/questionTools/tools/ask-user.ts b/packages/agent-core-v2/src/agent/questionTools/tools/ask-user.ts index 2736ea3ce..f340d09ff 100644 --- a/packages/agent-core-v2/src/agent/questionTools/tools/ask-user.ts +++ b/packages/agent-core-v2/src/agent/questionTools/tools/ask-user.ts @@ -10,7 +10,10 @@ import { z } from 'zod'; +import { CoreErrors } from '#/_base/errors/codes'; +import { KimiError } from '#/_base/errors/errors'; import { toInputJsonSchema } from '#/_base/tools/support/input-schema'; +import { errorMessage, isAbortError } from '#/agent/loop/errors'; import { IAgentTaskService } from '#/agent/task/task'; import { ITelemetryService } from '#/app/telemetry/telemetry'; import type { TelemetryProperties } from '#/app/telemetry/telemetry'; @@ -37,12 +40,13 @@ import { QuestionTask } from './question-task'; const QuestionOptionSchema = z.object({ label: z .string() + .min(1) .describe("Concise display text (1-5 words). If recommended, append '(Recommended)'."), description: z.string().default('').describe('Brief explanation of trade-offs or implications.'), }); const QuestionItemSchema = z.object({ - question: z.string().describe("A specific, actionable question. End with '?'."), + question: z.string().min(1).describe("A specific, actionable question. End with '?'."), header: z .string() .default('') @@ -70,6 +74,36 @@ export interface AskUserQuestionInput { }>; } +const QUESTION_UNIQUENESS_MESSAGE = + 'Question texts must be unique across questions, and option labels must be unique within each question.'; + +/** + * Answers are keyed by question text with option labels as values, so both + * must be unambiguous: question texts unique across the call, option labels + * unique within their question. Runtime tool-arg validation is AJV against + * the JSON Schema (where zod refinements are unrepresentable), so the + * execution path re-runs this check itself. + */ +function questionUniquenessError( + questions: AskUserQuestionInput['questions'], +): string | null { + const texts = new Set(); + for (const q of questions) { + if (texts.has(q.question)) { + return `Invalid questions: duplicate question text ${JSON.stringify(q.question)}. ${QUESTION_UNIQUENESS_MESSAGE} Rephrase the duplicates and call the tool again.`; + } + texts.add(q.question); + const labels = new Set(); + for (const option of q.options) { + if (labels.has(option.label)) { + return `Invalid questions: duplicate option label ${JSON.stringify(option.label)} in question ${JSON.stringify(q.question)}. ${QUESTION_UNIQUENESS_MESSAGE} Rephrase the duplicates and call the tool again.`; + } + labels.add(option.label); + } + } + return null; +} + const AskUserQuestionInputBaseSchema = z.object({ questions: z .array(QuestionItemSchema) @@ -83,15 +117,23 @@ const AskUserQuestionInputSchemaWithBackground = AskUserQuestionInputBaseSchema. .boolean() .default(false) .describe( - 'Set true to ask in the background and return immediately with a background task_id. Use TaskOutput to read the answer later.', + 'Set true to ask in the background and return immediately with a background task_id; you are notified automatically when the user answers — do not poll with TaskOutput while the question is pending.', ), +}).refine((data) => questionUniquenessError(data.questions) === null, { + message: QUESTION_UNIQUENESS_MESSAGE, }); export const AskUserQuestionInputSchema: z.ZodType = - AskUserQuestionInputSchemaWithBackground; + AskUserQuestionInputBaseSchema.refine( + (data) => questionUniquenessError(data.questions) === null, + { message: QUESTION_UNIQUENESS_MESSAGE }, + ); const QUESTION_DISMISSED_MESSAGE = 'User dismissed the question without answering.'; +const QUESTION_UNSUPPORTED_FAILURE_MESSAGE = + 'The connected client does not support interactive questions. Do NOT call this tool again. Ask the user directly in your text response instead.'; + // ── Implementation ─────────────────────────────────────────────────── export class AskUserQuestionTool implements BuiltinTool { @@ -122,45 +164,72 @@ export class AskUserQuestionTool implements BuiltinTool { args: AskUserQuestionInput, { toolCallId, signal, turnId }: ExecutableToolContext, ): Promise { + // AJV (the runtime arg validator) cannot express the uniqueness refine, + // so enforce it here before any UI interaction or task registration. + const uniquenessError = questionUniquenessError(args.questions); + if (uniquenessError !== null) { + return { isError: true, output: uniquenessError }; + } + if (args.background === true) { return this.executeInBackground(args, { toolCallId, turnId, signal }); } - return this.executeQuestion(args, { toolCallId, turnId }); + return this.executeQuestion(args, { toolCallId, turnId, signal }); } private async executeQuestion( args: AskUserQuestionInput, - { toolCallId, turnId }: Pick, - ): Promise { - const result = await this.question.request({ - turnId, + { toolCallId, - questions: args.questions.map((q) => ({ - question: q.question, - header: q.header.length > 0 ? q.header : undefined, - options: q.options.map((o) => ({ - label: o.label, - description: o.description.length > 0 ? o.description : undefined, - })), - multiSelect: q.multi_select, - })), - }); + signal, + turnId, + }: Pick, + ): Promise { + try { + const result = await this.question.request( + { + turnId, + toolCallId, + questions: args.questions.map((q) => ({ + question: q.question, + header: q.header, + options: q.options.map((o) => ({ + label: o.label, + description: o.description, + })), + multiSelect: q.multi_select, + })), + }, + { signal }, + ); + + const normalized = normalizeQuestionResult(result); + if (normalized === null || Object.keys(normalized.answers).length === 0) { + this.telemetry.track('question_dismissed'); + return dismissedQuestionResult(); + } + + const properties: TelemetryProperties = + normalized.method !== undefined + ? { answered: Object.keys(normalized.answers).length, method: normalized.method } + : { answered: Object.keys(normalized.answers).length }; + this.telemetry.track('question_answered', properties); + return { + isError: false, + output: JSON.stringify({ answers: normalized.answers }), + }; + } catch (error) { + if (isAbortError(error) || signal.aborted) throw error; + + if (error instanceof KimiError && error.code === CoreErrors.codes.NOT_IMPLEMENTED) { + return { + isError: true, + output: QUESTION_UNSUPPORTED_FAILURE_MESSAGE, + }; + } - const normalized = normalizeQuestionResult(result); - if (normalized === null || Object.keys(normalized.answers).length === 0) { - this.telemetry.track('question_dismissed'); return dismissedQuestionResult(); } - - const properties: TelemetryProperties = - normalized.method !== undefined - ? { answered: Object.keys(normalized.answers).length, method: normalized.method } - : { answered: Object.keys(normalized.answers).length }; - this.telemetry.track('question_answered', properties); - return { - isError: false, - output: JSON.stringify({ answers: normalized.answers }), - }; } private executeInBackground( @@ -180,7 +249,7 @@ export class AskUserQuestionTool implements BuiltinTool { try { taskId = this.tasks.registerTask( new QuestionTask( - () => this.executeQuestion(args, { toolCallId, turnId }), + (taskSignal) => this.executeQuestion(args, { toolCallId, turnId, signal: taskSignal }), description, { questionCount: args.questions.length, @@ -250,7 +319,3 @@ function isQuestionResponse(result: Exclude): result is Qu const answers = (result as { readonly answers?: unknown }).answers; return typeof answers === 'object' && answers !== null && !Array.isArray(answers); } - -function errorMessage(error: unknown): string { - return error instanceof Error ? error.message : String(error); -} diff --git a/packages/agent-core-v2/src/session/question/question.ts b/packages/agent-core-v2/src/session/question/question.ts index 0a0bff561..90ebe5f8e 100644 --- a/packages/agent-core-v2/src/session/question/question.ts +++ b/packages/agent-core-v2/src/session/question/question.ts @@ -8,7 +8,7 @@ * * The model is the **in-process** representation (camelCase, options carry no * ids). The protocol wire shape (snake_case, synthesized item/option ids, - * `expires_at`, 5-kind answer union) is produced at the edge — see the + * 5-kind answer union) is produced at the edge — see the * `server-v2` questions route, which is the single protocol↔in-process * adapter for this domain. Session-scoped — one instance per session. */ @@ -32,7 +32,11 @@ export interface QuestionItem { export type QuestionAnswerMethod = 'enter' | 'space' | 'number_key'; -/** Flattened answers keyed by item id — `true` marks a selected flag-style option. */ +/** + * Flattened answers keyed by question text; values are the chosen option + * label(s) (comma-joined for multi-select) or free-form "Other" text. + * `true` marks a question as answered without echoing a concrete value. + */ export type QuestionAnswers = Record; export interface QuestionResponse { @@ -54,7 +58,13 @@ export interface QuestionRequest { export interface ISessionQuestionService { readonly _serviceBrand: undefined; - request(req: QuestionRequest): Promise; + /** + * Post a question and block on the answer. When `options.signal` aborts + * while the question is parked (or was already aborted), the pending entry + * is dismissed and the promise resolves with `null` — the same dismissed + * result as an explicit dismiss (v1 broker semantics). + */ + request(req: QuestionRequest, options?: { signal?: AbortSignal }): Promise; /** * Post a question without blocking on the answer. Returns the request with * its resolved `id`; the answer is delivered through the interaction diff --git a/packages/agent-core-v2/src/session/question/questionService.ts b/packages/agent-core-v2/src/session/question/questionService.ts index 14a27ac7a..6e677b65b 100644 --- a/packages/agent-core-v2/src/session/question/questionService.ts +++ b/packages/agent-core-v2/src/session/question/questionService.ts @@ -20,13 +20,35 @@ export class SessionQuestionService implements ISessionQuestionService { constructor(@ISessionInteractionService private readonly interaction: ISessionInteractionService) {} - request(req: QuestionRequest): Promise { - return this.interaction.request({ - id: requestId(req), + request(req: QuestionRequest, options?: { signal?: AbortSignal }): Promise { + const id = requestId(req); + const pending = this.interaction.request({ + id, kind: 'question', payload: req, origin: { turnId: req.turnId }, }); + + // Mirrors the v1 broker: when the caller aborts (turn interrupted, + // background task killed) — or was aborted before parking — the entry is + // dismissed so listPending()/session status don't stay stuck in + // awaiting_question, and the caller receives the same `null` (dismissed) + // result as an explicit dismiss. + const signal = options?.signal; + if (signal !== undefined) { + if (signal.aborted) { + this.dismiss(id); + } else { + const onAbort = (): void => { + this.dismiss(id); + }; + signal.addEventListener('abort', onAbort, { once: true }); + void pending.finally(() => { + signal.removeEventListener('abort', onAbort); + }); + } + } + return pending; } enqueue(req: QuestionRequest): QuestionRequest & { readonly id: string } { diff --git a/packages/agent-core-v2/test/question/question.test.ts b/packages/agent-core-v2/test/question/question.test.ts index 1c349c747..a3c3e1c9c 100644 --- a/packages/agent-core-v2/test/question/question.test.ts +++ b/packages/agent-core-v2/test/question/question.test.ts @@ -97,6 +97,52 @@ describe('ISessionQuestionService (Session scope facade over the interaction ker }); }); + it('request with a pre-aborted signal resolves null and parks nothing', async () => { + const questions = session.accessor.get(ISessionQuestionService); + const controller = new AbortController(); + controller.abort(); + + await expect( + questions.request(makeRequest('q1'), { signal: controller.signal }), + ).resolves.toBeNull(); + expect(questions.listPending()).toEqual([]); + }); + + it('aborting a parked request dismisses it and resolves the caller with null', async () => { + const interaction = session.accessor.get(ISessionInteractionService); + const questions = session.accessor.get(ISessionQuestionService); + + const resolved: { id: string; response: unknown }[] = []; + disposables.add(interaction.onDidResolve((r) => resolved.push(r))); + + const controller = new AbortController(); + const pending = questions.request(makeRequest('q1'), { signal: controller.signal }); + expect(questions.listPending().map((r) => r.id)).toEqual(['q1']); + + controller.abort(); + + // v1 broker semantics: the abort settles the entry as a dismissal, so the + // caller sees the same `null` result (→ `event.question.dismissed`) as an + // explicit dismiss instead of a rejection. + await expect(pending).resolves.toBeNull(); + expect(questions.listPending()).toEqual([]); + expect(resolved).toEqual([{ id: 'q1', response: null }]); + expect(interaction.isRecentlyResolved('q1')).toBe(true); + }); + + it('an answer that arrives before the abort still wins', async () => { + const questions = session.accessor.get(ISessionQuestionService); + + const controller = new AbortController(); + const pending = questions.request(makeRequest('q1'), { signal: controller.signal }); + questions.answer('q1', { answers: { q_0: 'Yes' } }); + + await expect(pending).resolves.toEqual({ answers: { q_0: 'Yes' } }); + // A late abort is a no-op: the entry is already settled. + controller.abort(); + expect(questions.listPending()).toEqual([]); + }); + it('Session scope isolates brokers: a question parked in A is invisible to B', () => { const sessionB = host.child(LifecycleScope.Session, 'session-b'); const questionsA = session.accessor.get(ISessionQuestionService); diff --git a/packages/agent-core-v2/test/questionTools/ask-user.test.ts b/packages/agent-core-v2/test/questionTools/ask-user.test.ts new file mode 100644 index 000000000..38d4e1f18 --- /dev/null +++ b/packages/agent-core-v2/test/questionTools/ask-user.test.ts @@ -0,0 +1,574 @@ +/** + * AskUserQuestionTool unit tests — ported from v1 + * `packages/agent-core/test/tools/ask-user.test.ts` and adapted to the v2 DI + * constructor (`ISessionQuestionService` / `IAgentTaskService` / + * `ITelemetryService` stubs instead of a fake `Agent`). + */ + +import { describe, expect, it, vi } from 'vitest'; + +import { CoreErrors } from '#/_base/errors/codes'; +import { KimiError } from '#/_base/errors/errors'; +import { IAgentTaskService } from '#/agent/task/task'; +import type { AgentTask, AgentTaskInfoBase, AgentTaskSink, AgentTaskStatus } from '#/agent/task/types'; +import { + AskUserQuestionInputSchema, + AskUserQuestionTool, + type AskUserQuestionInput, +} from '#/agent/questionTools/tools/ask-user'; +import { ITelemetryService } from '#/app/telemetry/telemetry'; +import type { + ISessionQuestionService, + QuestionRequest, + QuestionResult, +} from '#/session/question/question'; +import { executeTool } from '../tools/fixtures/execute-tool'; + +const signal = new AbortController().signal; + +function input( + overrides: Partial = {}, +): AskUserQuestionInput { + return { + questions: [ + { + question: 'Which database?', + header: 'Storage', + options: [ + { label: 'Postgres', description: 'Relational storage' }, + { label: 'SQLite', description: 'Embedded storage' }, + ], + multi_select: false, + ...overrides, + }, + ], + }; +} + +interface FakeTaskEntry { + readonly task: AgentTask; + readonly controller: AbortController; + output: string; + status: AgentTaskStatus; + stopReason?: string; + readonly settled: Promise; +} + +/** + * Minimal in-memory stand-in for `IAgentTaskService`: runs a registered task + * immediately with a real sink so the QuestionTask wiring (including the + * task-signal passthrough) is exercised end to end. + */ +function createFakeTaskService(): { + readonly tasks: IAgentTaskService; + readonly registerTask: ReturnType; + entry(taskId: string): FakeTaskEntry; + kill(taskId: string): void; + wait(taskId: string): Promise; +} { + const entries = new Map(); + let counter = 0; + const registerTask = vi.fn((task: AgentTask): string => { + counter += 1; + const taskId = `${task.idPrefix}-${String(counter).padStart(8, '0')}`; + const controller = new AbortController(); + let settleResolve!: () => void; + const settled = new Promise((resolve) => { + settleResolve = resolve; + }); + const entry: FakeTaskEntry = { task, controller, output: '', status: 'running', settled }; + const sink: AgentTaskSink = { + signal: controller.signal, + appendOutput: (chunk) => { + entry.output += chunk; + }, + settle: (settlement) => { + entry.status = settlement.status; + entry.stopReason = settlement.stopReason; + settleResolve(); + return Promise.resolve(true); + }, + }; + entries.set(taskId, entry); + void Promise.resolve(task.start(sink)).catch(() => {}); + return taskId; + }); + const tasks = { + registerTask, + getTask: (taskId: string) => { + const entry = entries.get(taskId); + if (entry === undefined) return undefined; + const base: AgentTaskInfoBase = { + taskId, + description: entry.task.description, + status: entry.status, + startedAt: 0, + endedAt: null, + stopReason: entry.stopReason, + }; + return entry.task.toInfo(base); + }, + } as unknown as IAgentTaskService; + return { + tasks, + registerTask, + entry: (taskId) => { + const entry = entries.get(taskId); + if (entry === undefined) throw new Error(`unknown task ${taskId}`); + return entry; + }, + kill: (taskId) => { + entries.get(taskId)?.controller.abort(); + }, + wait: async (taskId) => entries.get(taskId)?.settled, + }; +} + +function makeTool( + options: { + readonly request?: ( + req: QuestionRequest, + requestOptions?: { readonly signal?: AbortSignal }, + ) => Promise; + } = {}, +): { + readonly tool: AskUserQuestionTool; + readonly request: ReturnType; + readonly telemetryTrack: ReturnType; + readonly taskService: ReturnType; +} { + const request = vi.fn(options.request ?? (async () => ({ Postgres: true }) as QuestionResult)); + const telemetryTrack = vi.fn(); + const taskService = createFakeTaskService(); + const question = { request } as unknown as ISessionQuestionService; + const telemetry = { track: telemetryTrack } as unknown as ITelemetryService; + const tool = new AskUserQuestionTool(question, taskService.tasks, telemetry); + return { tool, request, telemetryTrack, taskService }; +} + +describe('AskUserQuestionTool', () => { + it('exposes current metadata and schema', () => { + const { tool } = makeTool(); + + expect(tool.name).toBe('AskUserQuestion'); + expect(tool.description).toContain('structured options'); + expect(tool.parameters).toMatchObject({ + type: 'object', + properties: { questions: { type: 'array' } }, + }); + expect(AskUserQuestionInputSchema.safeParse(input()).success).toBe(true); + expect(AskUserQuestionInputSchema.safeParse({ questions: [] }).success).toBe(false); + expect( + AskUserQuestionInputSchema.safeParse( + input({ + options: [{ label: 'Only one', description: 'Not enough choices' }], + }), + ).success, + ).toBe(false); + }); + + it('documents the answers shape and the uniqueness requirement to the model', () => { + const { tool } = makeTool(); + + expect(tool.description).toContain('must be unique across the call'); + expect(tool.description).toContain('keyed by question text'); + }); + + it('tells the model not to poll a pending background question', () => { + const { tool } = makeTool(); + const paramsJson = JSON.stringify(tool.parameters); + + expect(paramsJson).toContain('do not poll with TaskOutput'); + expect(paramsJson).not.toContain('Use TaskOutput to read the answer later'); + }); + + it('rejects empty question text and empty option labels at the schema layer', () => { + expect( + AskUserQuestionInputSchema.safeParse(input({ question: '' })).success, + ).toBe(false); + expect( + AskUserQuestionInputSchema.safeParse( + input({ + options: [ + { label: '', description: 'Empty label' }, + { label: 'B', description: '' }, + ], + }), + ).success, + ).toBe(false); + }); + + it('rejects duplicate question texts across questions (schema + execution)', async () => { + const duplicated: AskUserQuestionInput = { + questions: [input().questions[0]!, input().questions[0]!], + }; + expect(AskUserQuestionInputSchema.safeParse(duplicated).success).toBe(false); + + const { tool, request } = makeTool(); + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'call_dup_question', + args: duplicated, + signal, + }); + expect(result.isError).toBe(true); + expect(result.output).toContain('unique'); + expect(request).not.toHaveBeenCalled(); + }); + + it('rejects duplicate option labels within one question (schema + execution)', async () => { + const duplicated = input({ + options: [ + { label: 'Postgres', description: 'Relational storage' }, + { label: 'Postgres', description: 'Same label again' }, + ], + }); + expect(AskUserQuestionInputSchema.safeParse(duplicated).success).toBe(false); + + const { tool, request } = makeTool(); + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'call_dup_label', + args: duplicated, + signal, + }); + expect(result.isError).toBe(true); + expect(result.output).toContain('unique'); + expect(request).not.toHaveBeenCalled(); + }); + + it('allows the same option label to appear in different questions', async () => { + const args: AskUserQuestionInput = { + questions: [ + input().questions[0]!, + input({ question: 'Which cache?' }).questions[0]!, + ], + }; + expect(AskUserQuestionInputSchema.safeParse(args).success).toBe(true); + + const { tool, request } = makeTool(); + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'call_cross_label', + args, + signal, + }); + expect(result.isError).toBe(false); + expect(request).toHaveBeenCalledOnce(); + }); + + it('rejects duplicate questions on the background path before starting a task', async () => { + const { tool, request, taskService } = makeTool(); + + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'call_bg_dup', + args: { + questions: [input().questions[0]!, input().questions[0]!], + background: true, + }, + signal, + }); + expect(result.isError).toBe(true); + expect(result.output).toContain('unique'); + expect(result.output).not.toContain('task_id:'); + expect(request).not.toHaveBeenCalled(); + expect(taskService.registerTask).not.toHaveBeenCalled(); + }); + + it('describes the no-Other rule on options and the Recommended hint on label', () => { + const { tool } = makeTool(); + const params = tool.parameters as { + properties: { + questions: { + items: { + properties: { + options: { + description?: string; + items: { properties: { label: { description?: string } } }; + }; + }; + }; + }; + }; + }; + + const optionsSchema = params.properties.questions.items.properties.options; + expect(optionsSchema.description).toContain("Do NOT include an 'Other' option"); + expect(optionsSchema.description).toContain('the system adds one automatically'); + + const labelSchema = optionsSchema.items.properties.label; + expect(labelSchema.description).toContain("append '(Recommended)'"); + }); + + it('always builds the background-question schema', () => { + const { tool } = makeTool(); + + expect(tool.description).toContain('Set background=true'); + expect(JSON.stringify(tool.parameters)).toContain('background'); + }); + + it('dispatches questions through the session question service', async () => { + const { tool, request, telemetryTrack } = makeTool(); + + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'call_question', + args: input({ multi_select: true }), + signal, + }); + + expect(result.isError).toBe(false); + expect(result.output).toBe(JSON.stringify({ answers: { Postgres: true } })); + expect(request).toHaveBeenCalledWith( + { + turnId: 0, + toolCallId: 'call_question', + questions: [ + { + question: 'Which database?', + header: 'Storage', + options: [ + { label: 'Postgres', description: 'Relational storage' }, + { label: 'SQLite', description: 'Embedded storage' }, + ], + multiSelect: true, + }, + ], + }, + { signal }, + ); + expect(telemetryTrack).toHaveBeenCalledWith('question_answered', { + answered: 1, + }); + }); + + it('passes empty headers and option descriptions through verbatim (v1 wire parity)', async () => { + const { tool, request } = makeTool(); + + await executeTool(tool, { + turnId: 0, + toolCallId: 'call_empty_fields', + args: input({ + header: '', + options: [ + { label: 'Postgres', description: '' }, + { label: 'SQLite', description: '' }, + ], + }), + signal, + }); + + expect(request).toHaveBeenCalledWith( + expect.objectContaining({ + questions: [ + expect.objectContaining({ + header: '', + options: [ + { label: 'Postgres', description: '' }, + { label: 'SQLite', description: '' }, + ], + }), + ], + }), + { signal }, + ); + }); + + it('tracks the structured question answer method without leaking it into output', async () => { + const { tool, telemetryTrack } = makeTool({ + request: async () => ({ + answers: { 'Which database?': 'SQLite' }, + method: 'number_key', + }), + }); + + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'call_question', + args: input(), + signal, + }); + + expect(result).toMatchObject({ isError: false }); + expect(result.output).toBe(JSON.stringify({ answers: { 'Which database?': 'SQLite' } })); + expect(telemetryTrack).toHaveBeenCalledWith('question_answered', { + answered: 1, + method: 'number_key', + }); + }); + + it('starts a background question task and stores the eventual answer in task output', async () => { + let resolveQuestion!: (result: QuestionResult) => void; + const questionResult = new Promise((resolve) => { + resolveQuestion = resolve; + }); + const { tool, taskService, telemetryTrack } = makeTool({ + request: async () => questionResult, + }); + + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'call_background_question', + args: { ...input(), background: true }, + signal, + }); + + expect(result.isError).toBe(false); + expect(result.output).toContain('task_id: question-'); + const outputText = typeof result.output === 'string' ? result.output : ''; + const taskId = /task_id: (?question-[0-9a-z]{8})/.exec(outputText)?.groups?.['taskId']; + expect(taskId).toBeDefined(); + expect(taskService.tasks.getTask(taskId!)).toMatchObject({ + kind: 'question', + status: 'running', + questionCount: 1, + toolCallId: 'call_background_question', + }); + + resolveQuestion({ answers: { 'Which database?': 'SQLite' }, method: 'enter' }); + await taskService.wait(taskId!); + + expect(taskService.tasks.getTask(taskId!)).toMatchObject({ status: 'completed' }); + expect(taskService.entry(taskId!).output).toBe( + JSON.stringify({ answers: { 'Which database?': 'SQLite' } }), + ); + expect(telemetryTrack).toHaveBeenCalledWith('question_answered', { + answered: 1, + method: 'enter', + }); + }); + + it('dismisses the underlying question when a background task is killed (v1 broker semantics)', async () => { + const { tool, request, taskService, telemetryTrack } = makeTool({ + // Emulates the real question service: an abort dismisses the parked + // entry and resolves with `null` instead of rejecting. + request: async (_req, requestOptions) => + new Promise((resolve) => { + requestOptions?.signal?.addEventListener('abort', () => resolve(null), { + once: true, + }); + }), + }); + + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'call_bg_kill', + args: { ...input(), background: true }, + signal, + }); + const outputText = typeof result.output === 'string' ? result.output : ''; + const taskId = /task_id: (?question-[0-9a-z]{8})/.exec(outputText)?.groups?.['taskId']; + expect(taskId).toBeDefined(); + + // The task signal must reach the question service so a TaskStop actually + // cancels the pending question instead of leaking it. + const requestOptions = request.mock.calls[0]?.[1] as { signal?: AbortSignal } | undefined; + expect(requestOptions?.signal).toBeDefined(); + + taskService.kill(taskId!); + await taskService.wait(taskId!); + // The dismissal resolves the question thunk with the dismissed answers + // payload, so the task itself completes — matching the v1 runtime. + expect(taskService.entry(taskId!).status).toBe('completed'); + expect(taskService.entry(taskId!).output).toBe( + JSON.stringify({ answers: {}, note: 'User dismissed the question without answering.' }), + ); + expect(telemetryTrack).toHaveBeenCalledWith('question_dismissed'); + }); + + it('returns a dismissed message when every question is dismissed', async () => { + const { tool, telemetryTrack } = makeTool({ request: async () => null }); + + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'call_question', + args: { + questions: [input().questions[0]!, input({ question: 'Which cache?' }).questions[0]!], + }, + signal, + }); + + expect(result).toMatchObject({ isError: false }); + expect(result.output).toContain('dismissed'); + expect(result.output).toContain('answers'); + expect(telemetryTrack).toHaveBeenCalledWith('question_dismissed'); + }); + + it('resolves question service error responses as dismissed answers', async () => { + const { tool } = makeTool({ + request: async () => { + throw new KimiError(CoreErrors.codes.INTERNAL, 'question broker error'); + }, + }); + + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'call_question', + args: input(), + signal, + }); + + expect(result).toMatchObject({ isError: false }); + expect(result.output).toContain('dismissed'); + expect(typeof result.output).toBe('string'); + const output = typeof result.output === 'string' ? result.output : ''; + expect(JSON.parse(output)).toEqual({ + answers: {}, + note: 'User dismissed the question without answering.', + }); + expect(result.output).not.toContain('Do NOT call this tool again'); + }); + + it('propagates aborts while waiting for the question service', async () => { + const controller = new AbortController(); + const { tool } = makeTool({ + request: async (_req, requestOptions) => + new Promise((_resolve, reject) => { + requestOptions?.signal?.addEventListener( + 'abort', + () => { + const error = new Error('Aborted'); + error.name = 'AbortError'; + reject(error); + }, + { once: true }, + ); + }), + }); + + const result = executeTool(tool, { + turnId: 0, + toolCallId: 'call_question', + args: input(), + signal: controller.signal, + }); + controller.abort(); + + await expect(result).rejects.toHaveProperty('name', 'AbortError'); + }); + + it('returns a distinct hard error when the host signals unsupported', async () => { + const { tool } = makeTool({ + request: async () => { + throw new KimiError( + CoreErrors.codes.NOT_IMPLEMENTED, + 'Client does not support questions', + ); + }, + }); + + const result = await executeTool(tool, { + turnId: 0, + toolCallId: 'tc-ask-unsupported', + args: input(), + signal, + }); + + expect(result).toMatchObject({ isError: true }); + expect(result.output).toContain('connected client'); + expect(result.output).toContain('does not support interactive questions'); + expect(result.output).toContain('Do NOT call this tool again'); + expect(result.output).toContain('Ask the user directly in your text response instead'); + }); +}); diff --git a/packages/kap-server/src/routes/questions.ts b/packages/kap-server/src/routes/questions.ts index 9d4b6e924..5be404885 100644 --- a/packages/kap-server/src/routes/questions.ts +++ b/packages/kap-server/src/routes/questions.ts @@ -35,8 +35,10 @@ * **Anti-corruption**: this is the single protocol↔in-process adapter for * questions. The v2 domain stores the in-process `QuestionRequest` (camelCase, * options without ids); the wire shape (snake_case, synthesized item/option - * ids, `expires_at`, 5-kind answer union) is derived here. No `agent-core` - * (v1) imports. + * ids, 5-kind answer union) is derived here. On resolve, wire + * ids are translated back to question text / option labels (reading the + * pending request before it settles) so the flattened record the model sees + * is self-explanatory. No `agent-core` (v1) imports. */ import { @@ -100,13 +102,6 @@ const tailParamsSchema = z.object({ const detailsSchema = z.array(z.object({ path: z.string(), message: z.string() })); -/** - * Wire `expires_at` horizon: v2 interactions never expire, so we emit the v1 - * broker's default timeout (60s) as a stable derived value, because the wire - * schema requires an `expires_at`. - */ -const QUESTION_EXPIRY_MS = 60_000; - export function registerQuestionsRoutes(app: QuestionRouteHost, core: Scope): void { const listRoute = defineRoute( { @@ -182,9 +177,11 @@ export function registerQuestionsRoutes(app: QuestionRouteHost, core: Scope): vo } const interaction = handle.accessor.get(ISessionInteractionService); - const isPending = interaction.listPending('question').some((i) => i.id === questionId); + const pendingInteraction = interaction + .listPending('question') + .find((i) => i.id === questionId); - if (!isPending) { + if (pendingInteraction === undefined) { if (interaction.isRecentlyResolved(questionId)) { reply.send({ code: ErrorCode.APPROVAL_ALREADY_RESOLVED, // 40902 — shared "already_resolved" @@ -238,7 +235,13 @@ export function registerQuestionsRoutes(app: QuestionRouteHost, core: Scope): vo return; } - const result = toInProcessResponse(bodyParse.data); + // The pending request must be projected BEFORE answer() settles (and + // thereby drops) the kernel entry — its synthesized wire ids are the + // lookup table for the id → text translation below. + const result = toInProcessResponse( + bodyParse.data, + toWireQuestion(pendingInteraction, session_id), + ); questions.answer(questionId, result); reply.send( okEnvelope({ resolved: true as const, resolved_at: new Date().toISOString() }, req.id), @@ -286,15 +289,14 @@ function buildItem(item: QuestionItem, itemIdx: number): ProtocolQuestionItem { export function toWireQuestion( interaction: Interaction, sessionId: string, -): ProtocolQuestionRequest & { expires_at: string } { +): ProtocolQuestionRequest { const req = interaction.payload as QuestionRequest; const createdAt = new Date(interaction.createdAt).toISOString(); - const out: ProtocolQuestionRequest & { expires_at: string } = { + const out: ProtocolQuestionRequest = { question_id: interaction.id, session_id: sessionId, questions: req.questions.map((q, i) => buildItem(q, i)), created_at: createdAt, - expires_at: new Date(interaction.createdAt + QUESTION_EXPIRY_MS).toISOString(), }; if (req.turnId !== undefined) out.turn_id = req.turnId; if (req.toolCallId !== undefined) out.tool_call_id = req.toolCallId; @@ -302,29 +304,60 @@ export function toWireQuestion( } /** - * Protocol REST response body → in-process `QuestionResponse`. Normalization - * rules (SCHEMAS §6.4): - * - single → option_id - * - multi → option_ids.join(',') + * Protocol REST response body → in-process `QuestionResponse`. + * + * The wire keeps synthesized ids (`q_` / `opt__`) so clients can + * answer unambiguously, but the flattened record is what the ask-user tool + * feeds back to the model — so ids are translated back to text here using + * the pending wire `request` (ported from v1's `toAgentCoreResponse`): + * - key → the question's text (falls back to the raw qid + * when the request is unavailable or the qid is + * unknown — stale client, defensive) + * - single → option label + * - multi → labels.join(', ') * - other → text - * - multi_with_other → [...option_ids, other_text].join(',') + * - multi_with_other → [...labels, other_text].join(', ') * - skipped → OMIT entry + * + * Multi-select joins use `', '` to match what the TUI reverse-RPC path + * already emits, so the model sees one format regardless of which client + * answered. + * + * Unknown qids and option ids — including ids that belong to a DIFFERENT + * question than the one being answered — are kept verbatim rather than + * resolved or dropped: translating a cross-question id would hand the model + * a plausible-looking label that was never offered for that question, while + * the raw id stays diagnosable. */ -function toInProcessResponse(resp: ProtocolQuestionResponse): QuestionResult { +function toInProcessResponse( + resp: ProtocolQuestionResponse, + request?: ProtocolQuestionRequest, +): QuestionResult { + const itemsById = new Map(); + for (const item of request?.questions ?? []) { + itemsById.set(item.id, item); + } + const flattened: QuestionAnswers = {}; for (const [qid, ans] of Object.entries(resp.answers)) { + const item = itemsById.get(qid); + const key = item?.question ?? qid; + // Resolve option ids only within the answered question's own options + // (at most 4, so a linear scan is fine). + const optionText = (id: string): string => + item?.options.find((o) => o.id === id)?.label ?? id; switch (ans.kind) { case 'single': - flattened[qid] = ans.option_id; + flattened[key] = optionText(ans.option_id); break; case 'multi': - flattened[qid] = ans.option_ids.join(','); + flattened[key] = ans.option_ids.map(optionText).join(', '); break; case 'other': - flattened[qid] = ans.text; + flattened[key] = ans.text; break; case 'multi_with_other': - flattened[qid] = [...ans.option_ids, ans.other_text].join(','); + flattened[key] = [...ans.option_ids.map(optionText), ans.other_text].join(', '); break; case 'skipped': // Omitted from the record — matches SCHEMAS §6.4. diff --git a/packages/kap-server/test/questions.test.ts b/packages/kap-server/test/questions.test.ts index b96a51a35..c9d9bf71b 100644 --- a/packages/kap-server/test/questions.test.ts +++ b/packages/kap-server/test/questions.test.ts @@ -46,7 +46,6 @@ interface QuestionWire { tool_call_id?: string; questions: QuestionItemWire[]; created_at: string; - expires_at: string; } interface ListWire { @@ -163,7 +162,8 @@ describe('server-v2 /api/v1/sessions/{sid}/questions', () => { }, ]); expect(Number.isNaN(Date.parse(item.created_at))).toBe(false); - expect(Number.isNaN(Date.parse(item.expires_at))).toBe(false); + // v1 parity: the question wire shape carries no synthetic expiry. + expect(item).not.toHaveProperty('expires_at'); }); it('resolves a pending question', async () => { @@ -193,11 +193,107 @@ describe('server-v2 /api/v1/sessions/{sid}/questions', () => { method: 'click', // protocol-only method; dropped on the in-process side }); + // Wire ids are translated back to question text / option labels so the + // record the model sees is self-explanatory (v1 parity: multi joins with + // ', ' to match the TUI reverse-RPC path). await expect(resultPromise).resolves.toEqual({ - answers: { q_0: 'opt_0_0,opt_0_1' }, + answers: { 'Pick one': 'Yes, No' }, }); }); + function makeTwoQuestionRequest(id: string): QuestionRequest { + return { + id, + toolCallId: `tc-${id}`, + questions: [ + { + question: 'Which animal?', + options: [{ label: 'Cat' }, { label: 'Dog' }], + }, + { + question: 'Which colors?', + options: [{ label: 'Red' }, { label: 'Green' }, { label: 'Blue' }], + multiSelect: true, + }, + ], + }; + } + + it('translates ids to text across single / other / multi_with_other kinds', async () => { + const sid = await createSession(); + const single: Promise = questionService(sid).request( + makeTwoQuestionRequest('q-t1'), + ); + await postJson(`/api/v1/sessions/${sid}/questions/q-t1`, { + answers: { + q_0: { kind: 'single', option_id: 'opt_0_1' }, + q_1: { + kind: 'multi_with_other', + option_ids: ['opt_1_0', 'opt_1_1'], + other_text: 'Custom', + }, + }, + }); + await expect(single).resolves.toEqual({ + answers: { 'Which animal?': 'Dog', 'Which colors?': 'Red, Green, Custom' }, + }); + + const other: Promise = questionService(sid).request( + makeTwoQuestionRequest('q-t2'), + ); + await postJson(`/api/v1/sessions/${sid}/questions/q-t2`, { + answers: { + q_0: { kind: 'other', text: 'Hippopotamus' }, + q_1: { kind: 'skipped' }, + }, + }); + await expect(other).resolves.toEqual({ + answers: { 'Which animal?': 'Hippopotamus' }, + }); + }); + + it('keeps unknown and cross-question option ids verbatim (stale client)', async () => { + const sid = await createSession(); + const resultPromise: Promise = questionService(sid).request( + makeTwoQuestionRequest('q-t3'), + ); + + await postJson(`/api/v1/sessions/${sid}/questions/q-t3`, { + answers: { + // opt_0_9 does not exist; q_9 is an unknown question id. + q_0: { kind: 'single', option_id: 'opt_0_9' }, + q_9: { kind: 'single', option_id: 'opt_9_0' }, + // opt_0_0 belongs to question 0 — never offered for question 1, so it + // must NOT be resolved to 'Cat'. + q_1: { kind: 'multi', option_ids: ['opt_1_0', 'opt_0_0'] }, + }, + }); + + await expect(resultPromise).resolves.toEqual({ + answers: { + 'Which animal?': 'opt_0_9', + q_9: 'opt_9_0', + 'Which colors?': 'Red, opt_0_0', + }, + }); + }); + + it('produces an empty answers record when all questions are skipped (not a dismissal)', async () => { + const sid = await createSession(); + const resultPromise: Promise = questionService(sid).request( + makeTwoQuestionRequest('q-t4'), + ); + + await postJson(`/api/v1/sessions/${sid}/questions/q-t4`, { + answers: { + q_0: { kind: 'skipped' }, + q_1: { kind: 'skipped' }, + }, + }); + + await expect(resultPromise).resolves.toEqual({ answers: {} }); + }); + it('dismisses a pending question', async () => { const sid = await createSession(); const resultPromise: Promise = questionService(sid).request(makeRequest('q-4'));