diff --git a/assistant/src/ipc/skill-routes/registries.ts b/assistant/src/ipc/skill-routes/registries.ts index 90ad8b332f1..84738cd8827 100644 --- a/assistant/src/ipc/skill-routes/registries.ts +++ b/assistant/src/ipc/skill-routes/registries.ts @@ -160,13 +160,10 @@ async function handleRegisterTools( // Finalize before branching so both the supervisor short-circuit and // the in-memory registration path see a `Tool[]` with guaranteed - // `name`. Skills run `finalizeTool` locally before sending, so the - // `?? ""` is a defensive empty-string default — `registerSkillTools` - // will reject an empty name on the non-supervisor path with a clear - // error. The execute closure arrives as a no-op error closure from + // `name`. The execute closure arrives as a no-op error closure from // `finalizeTool`; the production (supervisor) path replaces it with // the dispatching closure installed by the manifest loader at boot. - const proxies = tools.map((tool) => finalizeTool(tool, tool.name ?? "")); + const proxies = tools.map((t) => finalizeTool(t)); // Supervisor short-circuit: when a supervisor is registered, the // manifest loader has already installed proxy tools at daemon boot. diff --git a/assistant/src/tools/ask-question/ask-question-tool.test.ts b/assistant/src/tools/ask-question/ask-question-tool.test.ts index 2dc2d290e24..b4d05f9d0c7 100644 --- a/assistant/src/tools/ask-question/ask-question-tool.test.ts +++ b/assistant/src/tools/ask-question/ask-question-tool.test.ts @@ -1,12 +1,42 @@ -import { describe, expect, test } from "bun:test"; +import { beforeEach, describe, expect, mock, test } from "bun:test"; -import type { QuestionPromptResult } from "../../permissions/question-prompter.js"; +import type { + QuestionPromptParams, + QuestionPromptResult, +} from "../../permissions/question-prompter.js"; import type { ToolContext } from "../types.js"; -import { askQuestionTool, createAskQuestionTool } from "./ask-question-tool.js"; -type PromptParams = Parameters< - import("../../permissions/question-prompter.js").QuestionPrompter["prompt"] ->[0]; +// Stub the prompter at the module level. The tool instantiates +// `new QuestionPrompter(...)` inside `execute()`, so every call goes +// through this constructor — we record `prompt()` calls into a shared +// `calls` array and rotate `nextResult` per test via `setNextResult`. +// +// `mock.module` is hoisted by bun before any static import of the tool +// runs, so the import below sees the stubbed prompter even though +// `askQuestionTool` captures the symbol at module-eval time. +const calls: QuestionPromptParams[] = []; +let nextResult: QuestionPromptResult = { + entries: [{ questionId: "q1", decision: "skipped" }], + overall: "completed", +}; +function setNextResult(result: QuestionPromptResult): void { + nextResult = result; +} + +mock.module("../../permissions/question-prompter.js", () => ({ + QuestionPrompter: class { + async prompt(params: QuestionPromptParams): Promise { + calls.push(params); + return nextResult; + } + }, +})); + +// Import after the mock so the tool's `import { QuestionPrompter }` binds +// to the stub class above. +const { askQuestionTool } = await import("./ask-question-tool.js"); + +type PromptParams = QuestionPromptParams; function makeContext(overrides: Partial = {}): ToolContext { return { @@ -18,19 +48,16 @@ function makeContext(overrides: Partial = {}): ToolContext { }; } -// Return type is inferred so the satisfies-narrowed shape of -// `createAskQuestionTool()` carries through — letting the test call -// `tool.execute(...)` without a `!` bang. -function makeToolWithStub(result: QuestionPromptResult) { - const calls: PromptParams[] = []; - const tool = createAskQuestionTool(() => ({ - async prompt(params: PromptParams) { - calls.push(params); - return result; - }, - })); - return { tool, calls }; -} +// Reset call log + default result between tests so individual cases stay +// hermetic. Each test that needs a non-default result calls +// `setNextResult()` before invoking `askQuestionTool.execute(...)`. +beforeEach(() => { + calls.length = 0; + nextResult = { + entries: [{ questionId: "q1", decision: "skipped" }], + overall: "completed", + }; +}); const validInput = { question: "Which fruit?", @@ -98,11 +125,9 @@ function singleCompleted( describe("AskQuestionTool.execute", () => { test("forwards questions array unchanged to the prompter", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); - const result = await tool.execute(validInput, makeContext()); + const result = await askQuestionTool.execute(validInput, makeContext()); expect(calls).toHaveLength(1); expect(calls[0]?.conversationId).toBe("conv-1"); @@ -122,10 +147,8 @@ describe("AskQuestionTool.execute", () => { }); test("formats option result with looked-up label", async () => { - const { tool } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "b" }), - ); - const result = await tool.execute(validInput, makeContext()); + setNextResult(singleCompleted({ decision: "option", optionId: "b" })); + const result = await askQuestionTool.execute(validInput, makeContext()); expect(result.content).toBe( `Question "${validInput.question}" → Option: b (Banana)`, ); @@ -133,10 +156,8 @@ describe("AskQuestionTool.execute", () => { }); test("falls back to '(unknown)' label when optionId is not in options", async () => { - const { tool } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "ghost" }), - ); - const result = await tool.execute(validInput, makeContext()); + setNextResult(singleCompleted({ decision: "option", optionId: "ghost" })); + const result = await askQuestionTool.execute(validInput, makeContext()); expect(result.content).toBe( `Question "${validInput.question}" → Option: ghost ((unknown))`, ); @@ -144,10 +165,8 @@ describe("AskQuestionTool.execute", () => { }); test("formats free-text result", async () => { - const { tool } = makeToolWithStub( - singleCompleted({ decision: "free_text", text: "Cherry" }), - ); - const result = await tool.execute(validInput, makeContext()); + setNextResult(singleCompleted({ decision: "free_text", text: "Cherry" })); + const result = await askQuestionTool.execute(validInput, makeContext()); expect(result.content).toBe( `Question "${validInput.question}" → Free text: Cherry`, ); @@ -155,37 +174,35 @@ describe("AskQuestionTool.execute", () => { }); test("formats skipped result", async () => { - const { tool } = makeToolWithStub(singleCompleted({ decision: "skipped" })); - const result = await tool.execute(validInput, makeContext()); + setNextResult(singleCompleted({ decision: "skipped" })); + const result = await askQuestionTool.execute(validInput, makeContext()); expect(result.content).toBe(`Question "${validInput.question}" → Skipped`); expect(result.isError).toBe(false); }); test("timeout produces tool error", async () => { - const { tool } = makeToolWithStub({ + setNextResult({ entries: [{ questionId: "q1", decision: "timed_out" }], overall: "timed_out", }); - const result = await tool.execute(validInput, makeContext()); + const result = await askQuestionTool.execute(validInput, makeContext()); expect(result.isError).toBe(true); expect(result.content).toBe("User did not respond within timeout"); }); test("aborted produces tool error", async () => { - const { tool } = makeToolWithStub({ + setNextResult({ entries: [{ questionId: "q1", decision: "skipped" }], overall: "aborted", }); - const result = await tool.execute(validInput, makeContext()); + const result = await askQuestionTool.execute(validInput, makeContext()); expect(result.isError).toBe(true); expect(result.content).toBe("Question aborted"); }); test("rejects input with fewer than 2 options", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); - const result = await tool.execute( + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); + const result = await askQuestionTool.execute( { ...validInput, options: [{ id: "a", label: "Apple" }] }, makeContext(), ); @@ -195,10 +212,8 @@ describe("AskQuestionTool.execute", () => { }); test("rejects input with more than 4 options", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); - const result = await tool.execute( + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); + const result = await askQuestionTool.execute( { ...validInput, options: [ @@ -216,10 +231,8 @@ describe("AskQuestionTool.execute", () => { }); test("rejects input with empty question", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); - const result = await tool.execute( + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); + const result = await askQuestionTool.execute( { ...validInput, question: "" }, makeContext(), ); @@ -228,11 +241,12 @@ describe("AskQuestionTool.execute", () => { }); test("propagates abort signal into the prompter", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); const ac = new AbortController(); - await tool.execute(validInput, makeContext({ signal: ac.signal })); + await askQuestionTool.execute( + validInput, + makeContext({ signal: ac.signal }), + ); expect(calls[0]?.signal).toBe(ac.signal); }); }); @@ -241,11 +255,9 @@ describe("AskQuestionTool.execute", () => { describe("AskQuestionTool batched input", () => { test("normalizes legacy flat input into a one-element batch forwarded to the prompter", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); - const result = await tool.execute(validInput, makeContext()); + const result = await askQuestionTool.execute(validInput, makeContext()); expect(calls).toHaveLength(1); expect(calls[0]?.questions).toHaveLength(1); @@ -255,11 +267,12 @@ describe("AskQuestionTool batched input", () => { }); test("accepts a single-element `questions` batch", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); - const result = await tool.execute({ questions: [singleQ] }, makeContext()); + const result = await askQuestionTool.execute( + { questions: [singleQ] }, + makeContext(), + ); expect(calls).toHaveLength(1); expect(calls[0]?.questions).toHaveLength(1); @@ -289,7 +302,7 @@ describe("AskQuestionTool batched input", () => { ], }; - const { tool, calls } = makeToolWithStub({ + setNextResult({ entries: [ { questionId: "q1", decision: "option", optionId: "a" }, { questionId: "q2", decision: "free_text", text: "noon-ish" }, @@ -298,18 +311,18 @@ describe("AskQuestionTool batched input", () => { overall: "completed", }); - const result = await tool.execute( + const result = await askQuestionTool.execute( { questions: [singleQ, q2, q3] }, makeContext(), ); expect(calls).toHaveLength(1); expect(calls[0]?.questions).toHaveLength(3); - expect(calls[0]?.questions.map((q) => q.question)).toEqual([ - singleQ.question, - q2.question, - q3.question, - ]); + expect( + calls[0]?.questions.map( + (q: PromptParams["questions"][number]) => q.question, + ), + ).toEqual([singleQ.question, q2.question, q3.question]); expect(result.isError).toBe(false); expect(result.content).toBe( @@ -336,7 +349,7 @@ describe("AskQuestionTool batched input", () => { { id: "no", label: "No" }, ], }; - const { tool } = makeToolWithStub({ + setNextResult({ entries: [ { questionId: "q1", decision: "skipped" }, { questionId: "q2", decision: "skipped" }, @@ -345,7 +358,7 @@ describe("AskQuestionTool batched input", () => { overall: "completed", }); - const result = await tool.execute( + const result = await askQuestionTool.execute( { questions: [singleQ, q2, q3] }, makeContext(), ); @@ -368,7 +381,7 @@ describe("AskQuestionTool batched input", () => { { id: "afternoon", label: "Afternoon" }, ], }; - const { tool } = makeToolWithStub({ + setNextResult({ entries: [ { questionId: "q1", decision: "skipped" }, { questionId: "q2", decision: "skipped" }, @@ -376,7 +389,7 @@ describe("AskQuestionTool batched input", () => { overall: "closed", }); - const result = await tool.execute( + const result = await askQuestionTool.execute( { questions: [singleQ, q2] }, makeContext(), ); @@ -392,7 +405,7 @@ describe("AskQuestionTool batched input", () => { }); test("accepts a 5-entry batch (max allowed)", async () => { - const { tool, calls } = makeToolWithStub({ + setNextResult({ entries: [ { questionId: "q1", decision: "skipped" }, { questionId: "q2", decision: "skipped" }, @@ -404,7 +417,10 @@ describe("AskQuestionTool batched input", () => { }); const five = [singleQ, singleQ, singleQ, singleQ, singleQ]; - const result = await tool.execute({ questions: five }, makeContext()); + const result = await askQuestionTool.execute( + { questions: five }, + makeContext(), + ); expect(result.isError).toBe(false); expect(calls).toHaveLength(1); @@ -412,12 +428,13 @@ describe("AskQuestionTool batched input", () => { }); test("rejects batches with 6+ questions", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); const six = [singleQ, singleQ, singleQ, singleQ, singleQ, singleQ]; - const result = await tool.execute({ questions: six }, makeContext()); + const result = await askQuestionTool.execute( + { questions: six }, + makeContext(), + ); expect(result.isError).toBe(true); expect(result.content.toLowerCase()).toContain("invalid input"); @@ -425,11 +442,12 @@ describe("AskQuestionTool batched input", () => { }); test("rejects empty `questions` array", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); - const result = await tool.execute({ questions: [] }, makeContext()); + const result = await askQuestionTool.execute( + { questions: [] }, + makeContext(), + ); expect(result.isError).toBe(true); expect(result.content.toLowerCase()).toContain("invalid input"); @@ -437,11 +455,9 @@ describe("AskQuestionTool batched input", () => { }); test("rejects input missing both `questions` and flat fields", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); - const result = await tool.execute({}, makeContext()); + const result = await askQuestionTool.execute({}, makeContext()); expect(result.isError).toBe(true); expect(result.content.toLowerCase()).toContain("invalid input"); @@ -449,11 +465,12 @@ describe("AskQuestionTool batched input", () => { }); test("rejects legacy `question` without `options`", async () => { - const { tool, calls } = makeToolWithStub( - singleCompleted({ decision: "option", optionId: "a" }), - ); + setNextResult(singleCompleted({ decision: "option", optionId: "a" })); - const result = await tool.execute({ question: "Hi?" }, makeContext()); + const result = await askQuestionTool.execute( + { question: "Hi?" }, + makeContext(), + ); expect(result.isError).toBe(true); expect(result.content.toLowerCase()).toContain("invalid input"); @@ -495,15 +512,13 @@ describe("askQuestionTool definition (batched schema)", () => { "freeTextPlaceholder", ]), ); + // No per-question `id` field — daemon-assigned only. expect(Object.keys(itemProps)).not.toContain("id"); - expect(questions?.items?.required).toEqual(["question", "options"]); - expect(schema.properties.question?.type).toBe("string"); - expect(schema.properties.options?.type).toBe("array"); - expect(schema.properties.options?.minItems).toBe(2); - expect(schema.properties.options?.maxItems).toBe(4); - expect(schema.properties.freeTextPlaceholder?.type).toBe("string"); + expect(questions?.items?.required).toEqual(["question", "options"]); - expect(schema.required).toBeUndefined(); + // Legacy fields still present. + expect(schema.properties.question).toBeDefined(); + expect(schema.properties.options).toBeDefined(); }); }); diff --git a/assistant/src/tools/ask-question/ask-question-tool.ts b/assistant/src/tools/ask-question/ask-question-tool.ts index 8d2056b7d96..022f7851422 100644 --- a/assistant/src/tools/ask-question/ask-question-tool.ts +++ b/assistant/src/tools/ask-question/ask-question-tool.ts @@ -137,166 +137,157 @@ const OPTION_ITEMS_SCHEMA = { // ── Tool ──────────────────────────────────────────────────────────── /** - * Build a fresh `ask_question` {@link ToolDefinition}. The default - * prompter factory wires the real `broadcastMessage` so the question - * reaches every connected client. Tests pass an override to swap in - * a stubbed prompter without monkey-patching the module — this is the - * factory-function replacement for the previous constructor injection - * point (`new AskQuestionTool(stub)`). + * `ask_question` tool definition. Tests stub the prompter by mocking + * `../../permissions/question-prompter.js` via `bun:test`'s `mock.module` + * before importing this file — see `ask-question-tool.test.ts` for the + * pattern. */ -export function createAskQuestionTool( - prompterFactory: () => Pick = () => - new QuestionPrompter({ broadcastMessage }), -) { - return { - name: "ask_question", - description: DESCRIPTION, - category: "interaction", - executionTarget: "sandbox", - defaultRiskLevel: RiskLevel.Low, - input_schema: { - type: "object", - properties: { - // ── Recommended shape ───────────────────────────────────── - questions: { - type: "array", - minItems: 1, - maxItems: MAX_QUESTIONS_PER_BATCH, - description: `Recommended shape. 1–${MAX_QUESTIONS_PER_BATCH} clarifying questions to ask in a single turn. Use a batch when several independent ambiguities block progress; ask one at a time when they're sequentially dependent. Past ${MAX_QUESTIONS_PER_BATCH} questions you should be implementing, not asking.`, - items: { - type: "object", - properties: { - question: { - type: "string", - description: "The clarifying question to display.", - }, - description: { - type: "string", - description: - "Optional one-line context shown beneath the question.", - }, - options: { - type: "array", - minItems: 2, - maxItems: 4, - description: - "2–4 structured options. The UI always appends a free-text fallback slot, so do not include a 'something else' option here.", - items: OPTION_ITEMS_SCHEMA, - }, - freeTextPlaceholder: { - type: "string", - description: - "Optional placeholder text shown inside the free-text fallback input.", - }, +export const askQuestionTool = { + name: "ask_question", + description: DESCRIPTION, + category: "interaction", + executionTarget: "sandbox", + defaultRiskLevel: RiskLevel.Low, + input_schema: { + type: "object", + properties: { + // ── Recommended shape ───────────────────────────────────── + questions: { + type: "array", + minItems: 1, + maxItems: MAX_QUESTIONS_PER_BATCH, + description: `Recommended shape. 1–${MAX_QUESTIONS_PER_BATCH} clarifying questions to ask in a single turn. Use a batch when several independent ambiguities block progress; ask one at a time when they're sequentially dependent. Past ${MAX_QUESTIONS_PER_BATCH} questions you should be implementing, not asking.`, + items: { + type: "object", + properties: { + question: { + type: "string", + description: "The clarifying question to display.", + }, + description: { + type: "string", + description: + "Optional one-line context shown beneath the question.", + }, + options: { + type: "array", + minItems: 2, + maxItems: 4, + description: + "2–4 structured options. The UI always appends a free-text fallback slot, so do not include a 'something else' option here.", + items: OPTION_ITEMS_SCHEMA, + }, + freeTextPlaceholder: { + type: "string", + description: + "Optional placeholder text shown inside the free-text fallback input.", }, - required: ["question", "options"], }, + required: ["question", "options"], }, - // ── Legacy single-question fields ───────────────────────── - // Kept optional so existing prompt caches and any single-question - // callers continue to work. New callers should use `questions`. - question: { - type: "string", - description: - "Legacy: the single clarifying question. Prefer `questions[]` for new code.", - }, - description: { - type: "string", - description: - "Legacy: optional one-line context shown beneath the question. Prefer `questions[].description`.", - }, - options: { - type: "array", - minItems: 2, - maxItems: 4, - description: - "Legacy: 2–4 structured options. Prefer `questions[].options`. The UI always appends a free-text fallback slot, so do not include a 'something else' option here.", - items: OPTION_ITEMS_SCHEMA, - }, - freeTextPlaceholder: { - type: "string", - description: - "Legacy: optional placeholder text for the free-text fallback input. Prefer `questions[].freeTextPlaceholder`.", - }, }, - // No top-level `required` — caller must supply either `questions` - // or the legacy flat trio (`question` + `options`). Enforced in Zod. + // ── Legacy single-question fields ───────────────────────── + // Kept optional so existing prompt caches and any single-question + // callers continue to work. New callers should use `questions`. + question: { + type: "string", + description: + "Legacy: the single clarifying question. Prefer `questions[]` for new code.", + }, + description: { + type: "string", + description: + "Legacy: optional one-line context shown beneath the question. Prefer `questions[].description`.", + }, + options: { + type: "array", + minItems: 2, + maxItems: 4, + description: + "Legacy: 2–4 structured options. Prefer `questions[].options`. The UI always appends a free-text fallback slot, so do not include a 'something else' option here.", + items: OPTION_ITEMS_SCHEMA, + }, + freeTextPlaceholder: { + type: "string", + description: + "Legacy: optional placeholder text for the free-text fallback input. Prefer `questions[].freeTextPlaceholder`.", + }, }, + // No top-level `required` — caller must supply either `questions` + // or the legacy flat trio (`question` + `options`). Enforced in Zod. + }, - async execute( - input: Record, - context: ToolContext, - ): Promise { - const parsed = InputSchema.safeParse(input); - if (!parsed.success) { - return { - content: `Invalid input: ${parsed.error.message}`, - isError: true, - }; - } - - // Normalize legacy flat input into a one-element `questions` batch so - // downstream code only has to deal with the batched shape. The refine - // above guarantees `question` and `options` are present whenever - // `questions` is absent. - const questions: SingleQuestion[] = parsed.data.questions ?? [ - { - question: parsed.data.question!, - description: parsed.data.description, - options: parsed.data.options!, - freeTextPlaceholder: parsed.data.freeTextPlaceholder, - }, - ]; + async execute( + input: Record, + context: ToolContext, + ): Promise { + const parsed = InputSchema.safeParse(input); + if (!parsed.success) { + return { + content: `Invalid input: ${parsed.error.message}`, + isError: true, + }; + } - const prompter = prompterFactory(); - const result = await prompter.prompt({ - conversationId: context.conversationId, - questions, - toolUseId: context.toolUseId, - signal: context.signal, - }); + // Normalize legacy flat input into a one-element `questions` batch so + // downstream code only has to deal with the batched shape. The refine + // above guarantees `question` and `options` are present whenever + // `questions` is absent. + const questions: SingleQuestion[] = parsed.data.questions ?? [ + { + question: parsed.data.question!, + description: parsed.data.description, + options: parsed.data.options!, + freeTextPlaceholder: parsed.data.freeTextPlaceholder, + }, + ]; - // Format the aggregated transcript. Each line is keyed by the original - // question text (not the daemon-assigned id) — the LLM never sees those - // ids, and human-readable labels read better in the result content. - const lines = result.entries.map((entry, i) => { - const q = questions[i]!; - const prefix = `Question "${q.question}" →`; - if (entry.decision === "option") { - const chosen = q.options.find((o) => o.id === entry.optionId); - const label = chosen?.label ?? "(unknown)"; - return `${prefix} Option: ${entry.optionId} (${label})`; - } - if (entry.decision === "free_text") { - return `${prefix} Free text: ${entry.text ?? ""}`; - } - return `${prefix} Skipped`; - }); + const prompter = new QuestionPrompter({ broadcastMessage }); + const result = await prompter.prompt({ + conversationId: context.conversationId, + questions, + toolUseId: context.toolUseId, + signal: context.signal, + }); - switch (result.overall) { - case "completed": - return { content: lines.join("\n"), isError: false }; - case "closed": { - const summary = - "User closed the question card without answering. All questions skipped."; - return { - content: [summary, ...lines].join("\n"), - isError: false, - }; - } - case "timed_out": - return { - content: "User did not respond within timeout", - isError: true, - }; - case "aborted": - return { - content: "Question aborted", - isError: true, - }; + // Format the aggregated transcript. Each line is keyed by the original + // question text (not the daemon-assigned id) — the LLM never sees those + // ids, and human-readable labels read better in the result content. + const lines = result.entries.map((entry, i) => { + const q = questions[i]!; + const prefix = `Question "${q.question}" →`; + if (entry.decision === "option") { + const chosen = q.options.find((o) => o.id === entry.optionId); + const label = chosen?.label ?? "(unknown)"; + return `${prefix} Option: ${entry.optionId} (${label})`; } - }, - } satisfies ToolDefinition; -} + if (entry.decision === "free_text") { + return `${prefix} Free text: ${entry.text ?? ""}`; + } + return `${prefix} Skipped`; + }); -export const askQuestionTool = createAskQuestionTool(); + switch (result.overall) { + case "completed": + return { content: lines.join("\n"), isError: false }; + case "closed": { + const summary = + "User closed the question card without answering. All questions skipped."; + return { + content: [summary, ...lines].join("\n"), + isError: false, + }; + } + case "timed_out": + return { + content: "User did not respond within timeout", + isError: true, + }; + case "aborted": + return { + content: "Question aborted", + isError: true, + }; + } + }, +} satisfies ToolDefinition; diff --git a/assistant/src/tools/tool-defaults.ts b/assistant/src/tools/tool-defaults.ts index 4c0df52a7a2..3a54121b818 100644 --- a/assistant/src/tools/tool-defaults.ts +++ b/assistant/src/tools/tool-defaults.ts @@ -59,15 +59,17 @@ export const TOOL_DEFAULTS = Object.freeze({ * file-derived default), and return a `Tool` that is safe to hand to a * `registerXxxTools` call. * + * `defaultName` is optional — when omitted, the tool's own `name` field + * is the source of truth and `""` is the last-ditch fallback. The + * empty-string fallback is fail-loud: `registerSkillTools` rejects any + * tool with an empty name with a clear "tool.name is required" error. + * * The default `execute` returns an error result so the model sees a clear * "this tool isn't wired up" signal at call time. The owning loader still * registers the tool cleanly — a broken individual tool must never block * the registration batch. */ -export function finalizeTool( - tool: ToolDefinition, - defaultName: string, -): Tool { +export function finalizeTool(tool: ToolDefinition, defaultName = ""): Tool { const name = tool.name ?? defaultName; const description = typeof tool.description === "string" diff --git a/assistant/src/tools/types.ts b/assistant/src/tools/types.ts index c79d14829b7..83ce6465304 100644 --- a/assistant/src/tools/types.ts +++ b/assistant/src/tools/types.ts @@ -318,15 +318,26 @@ export interface ToolContext { } /** - * Schema describing the serializable shape of a {@link ToolDefinition}. - * All fields are optional — loaders fill documented defaults for omitted - * fields via `finalizeTool` in `tool-defaults.ts`. The IPC layer parses - * incoming skill tools against this same schema and re-finalizes them - * locally, so author shape and wire shape are one schema. + * Schema describing the shape of a {@link ToolDefinition}. All fields are + * optional — loaders fill documented defaults for omitted fields via + * `finalizeTool` in `tool-defaults.ts`. The IPC layer parses incoming + * skill tools against this same schema and re-finalizes them locally, + * so author shape and wire shape are one schema. * - * `execute` is intentionally absent from the schema (closures cannot - * cross IPC). It is added back as a TypeScript overlay on - * {@link ToolDefinition} so author literals can still set it. + * `input_schema` is `z.custom(...)` rather than + * `z.record(z.string(), z.unknown())` so that authors can assign a typed + * JSON-schema literal (`{ type: "object", properties: { ... } }`) + * without `as Record<...>` gymnastics. The custom check still rejects + * `null`, primitives, and arrays at runtime. + * + * `execute` is `z.custom<(input, context) => Promise>()` + * for the same reason — the wire path drops closures (they can't cross + * IPC) and `finalizeTool` synthesizes a no-op error closure on arrival. + * The custom shape gives `ToolDefinition.execute` a fully-typed + * signature via `z.infer` without an overlay type. + * + * Result: `ToolDefinition = z.infer` — + * one declaration, both `input_schema` and `execute` typed correctly. */ export const ToolDefinitionSchema = z.object({ /** @@ -339,46 +350,49 @@ export const ToolDefinitionSchema = z.object({ /** Human-readable description shown to the model in the tool catalog. */ description: z.string().optional(), /** JSON schema describing the tool's input arguments. */ - input_schema: z.record(z.string(), z.unknown()).optional(), + input_schema: z + .custom( + (val) => val !== null && typeof val === "object" && !Array.isArray(val), + { message: "input_schema must be a plain object" }, + ) + .optional(), /** Author-asserted risk band — low / medium / high. Drives default permission gating. */ defaultRiskLevel: z.enum(RiskLevel).optional(), /** Tool category used for Slack channel `allowedToolCategories` enforcement. */ category: z.string().min(1).optional(), /** Where the tool runs — sandbox (assistant container) or host (guardian device via proxy). Resolved by `resolveExecutionTarget` if omitted. */ executionTarget: z.enum(["sandbox", "host"]).optional(), + /** + * Implementation invoked when the model calls the tool. Optional + * because some `ToolDefinition` instances are schema-only (e.g. + * {@link ../memory/graph/tools.graphRememberDefinition}, + * {@link ../messaging/style-analyzer.storeStyleAnalysisTool}, + * {@link ../memory/v2/sweep-job.SWEEP_TOOL}) — handed to providers as + * a function-calling schema without ever being registered for + * execution. Closures can't cross IPC, so the wire path drops this + * and `finalizeTool` synthesizes a no-op error closure on arrival. + * Tool sources use `satisfies ToolDefinition` (not `: ToolDefinition`) + * so the inferred export type preserves `execute` as required at + * call sites that statically import the literal. + */ + execute: z + .custom< + ( + input: Record, + context: ToolContext, + ) => Promise + >() + .optional(), }); /** * Author-facing tool spec — re-exported from `@vellumai/plugin-api`. * Loaders fill documented defaults for omitted fields via `finalizeTool` - * in `tool-defaults.ts`. Type is `z.infer` - * (serializable fields) plus overlays: - * - `input_schema` is widened from `Record` (the - * parsed wire shape) to `object`, so authors can assign a typed - * JSON-schema literal without `as Record<...>` gymnastics. - * - `execute` is optional because some `ToolDefinition` instances are - * schema-only (e.g. {@link ../memory/graph/tools.graphRememberDefinition}, - * {@link ../messaging/style-analyzer.storeStyleAnalysisTool}, - * {@link ../memory/v2/sweep-job.SWEEP_TOOL}) — handed to providers as - * a function-calling schema without ever being registered for - * execution. Closures also can't cross IPC, so the wire path drops - * it and `finalizeTool` synthesizes a no-op error closure on arrival. - * Tool sources use `satisfies ToolDefinition` (not `: ToolDefinition`) - * so the inferred export type preserves `execute` as required at - * call sites that statically import the literal. + * in `tool-defaults.ts`. The type is a direct `z.infer` of + * {@link ToolDefinitionSchema} — both `input_schema` and `execute` are + * typed correctly by the schema itself, no overlay needed. */ -export type ToolDefinition = Omit< - z.infer, - "input_schema" -> & { - /** JSON schema describing the tool's input arguments. */ - input_schema?: object; - /** Implementation invoked when the model calls the tool. */ - execute?: ( - input: Record, - context: ToolContext, - ) => Promise; -}; +export type ToolDefinition = z.infer; /** Tool after the loader has derived its name and filled defaults. */ export type Tool = Required;