PR #32631 followup: ToolDefinition = z.infer<>, kill double factory

[vellum-apollo-bot]

Follow-up to #32631 — addresses all 4 review comments left on the APPROVED review.

Changes

1. types.ts — ToolDefinition = z.infer<typeof ToolDefinitionSchema> (review thread)

ToolDefinitionSchema now uses z.custom<...>() for both input_schema and execute:

input_schema: z
  .custom<object>(
    (val) => val !== null && typeof val === "object" && !Array.isArray(val),
    { message: "input_schema must be a plain object" },
  )
  .optional(),

execute: z
  .custom<
    (
      input: Record<string, unknown>,
      context: ToolContext,
    ) => Promise<ToolExecutionResult>
  >()
  .optional(),

Then:

export type ToolDefinition = z.infer<typeof ToolDefinitionSchema>;

Single declaration. No Omit<> overlay. Both fields are typed correctly by the schema itself.

z.custom on execute is a runtime no-op — but it doesn't need a runtime guard because the wire path drops closures (they can't cross IPC) and finalizeTool synthesizes a no-op error closure on arrival.

2. tool-defaults.ts — finalizeTool second arg optional (review thread)

-export function finalizeTool(tool: ToolDefinition, defaultName: string): Tool {
+export function finalizeTool(tool: ToolDefinition, defaultName = ""): Tool {

The "" fallback is fail-loud — registerSkillTools already rejects empty names with a clear error.

3. registries.ts — one-liner (review thread)

-const proxies = tools.map((tool) => finalizeTool(tool, tool.name ?? ""));
+const proxies = tools.map((t) => finalizeTool(t));

Defensive default lives inside finalizeTool now, not at every IPC call site.

4. ask-question-tool.ts — kill the double factory (review thread)

The createAskQuestionTool(prompterFactory = ...) factory existed only to inject a stub QuestionPrompter from tests. That's two layers of indirection for one mock seam. Replaced with a plain literal matching every other tool:

export const askQuestionTool = {
  // ...
  async execute(input, context) {
    const prompter = new QuestionPrompter({ broadcastMessage });
    // ...
  },
} satisfies ToolDefinition;

Test rewrite uses mock.module() + a dynamic await import() of the tool, which collapses both layers into one mock seam at the module boundary.

Verification

• 55/55 tests pass across: ask-question-tool.test.ts (23), registries.test.ts (16), plugin-api-tool-definition.test.ts (4), plugin-tool-contribution.test.ts (12).
• Lint clean on all touched files.
• Spot tsc on changed files: no errors.

Stack

Builds on #32631 (merged). No interaction with any open PRs.

[dvargasfuertes]

QuestionPrompter can remove its broadcastMessage constructor args and just import it directly