From 86c31d1143314316c70a1b208869924912c0cb2a Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 7 Feb 2026 12:28:22 +0100 Subject: [PATCH 1/2] fix: use .nullish() for optional tool input parameters OpenAI's Responses API normalizes tool schemas into strict mode, which forces every field into `required` and expects optional fields to accept `null`. Using `.optional()` alone produces a schema without a null type, so the model is forced to hallucinate values for fields it would normally skip. Change all optional fields in tool input schemas from `.optional()` to `.nullish()` (= `.optional().nullable()`), which satisfies strict-mode providers (OpenAI) while remaining compatible with non-strict providers (Anthropic, Google). Also update implementation handlers to use `!= null` (loose equality) instead of `!== undefined` so both `null` and `undefined` are treated as "not provided". Add convention documentation to AGENTS.md and the toolDefinitions.ts module doc comment. Change-Id: Ie3ee34b7d8e6aaf69ee29ef823b89253b2f749fc Signed-off-by: Thomas Kosiewski --- docs/AGENTS.md | 7 ++ docs/config/notifications.mdx | 2 +- .../tools/AgentSkillReadFileToolCall.tsx | 4 +- .../components/tools/FileReadToolCall.tsx | 4 +- src/browser/components/tools/TaskToolCall.tsx | 11 +-- src/common/types/tools.ts | 4 +- src/common/utils/tools/toolDefinitions.ts | 79 ++++++++++++------- .../services/system1/bashOutputFiltering.ts | 2 +- src/node/services/taskService.ts | 4 +- .../services/tools/agent_skill_read_file.ts | 10 +-- src/node/services/tools/bash_output.ts | 4 +- src/node/services/tools/code_execution.ts | 2 +- src/node/services/tools/file_edit_insert.ts | 12 +-- .../tools/file_edit_replace_shared.ts | 4 +- src/node/services/tools/file_read.ts | 6 +- src/node/services/tools/task_await.ts | 6 +- 16 files changed, 94 insertions(+), 67 deletions(-) diff --git a/docs/AGENTS.md b/docs/AGENTS.md index b0f0726e34..c2cde2697d 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -87,6 +87,13 @@ description: Agent instructions for AI assistants working on the Mux codebase - **Avoid `setTimeout` for component coordination** - racy and fragile; use callbacks or effects. - **Keyboard event propagation** - React's `e.stopPropagation()` only stops synthetic event bubbling; native `window` listeners still fire. Use `stopKeyboardPropagation(e)` from `@/browser/utils/events` to stop both React and native propagation when blocking global handlers (like stream interrupt on Escape). + +## Tool Schema Conventions + +- **Use `.nullish()` for optional tool input parameters** — never `.optional()` alone. OpenAI's Responses API normalizes tool schemas into strict mode, which forces every field into `required` and expects optional fields to accept `null`. `.nullish()` (= `.optional().nullable()`) satisfies strict-mode providers (OpenAI) while remaining compatible with non-strict providers (Anthropic, Google). See the module doc comment in `src/common/utils/tools/toolDefinitions.ts` for details. +- Implementation handlers should use `!= null` (loose equality) instead of `!== undefined` to treat both `null` and `undefined` as "not provided". +- This applies only to tool **input** schemas (parameters the model provides), not to tool **output/result** schemas (constructed by our backend). + ## Component State & Storage - Prefer **self-contained components** over utility functions + hook proliferation. A component that takes `workspaceId` and computes everything internally is better than one that requires 10 props drilled from parent hooks. diff --git a/docs/config/notifications.mdx b/docs/config/notifications.mdx index daefd3db5b..4fe62b021a 100644 --- a/docs/config/notifications.mdx +++ b/docs/config/notifications.mdx @@ -105,7 +105,7 @@ notify: { message: z .string() .max(200) - .optional() + .nullish() .describe( "Optional notification body with more details (max 200 chars). " + "Keep it brief - users may only see a preview." diff --git a/src/browser/components/tools/AgentSkillReadFileToolCall.tsx b/src/browser/components/tools/AgentSkillReadFileToolCall.tsx index 3f3b0e3840..898e1e99fe 100644 --- a/src/browser/components/tools/AgentSkillReadFileToolCall.tsx +++ b/src/browser/components/tools/AgentSkillReadFileToolCall.tsx @@ -139,13 +139,13 @@ export const AgentSkillReadFileToolCall: React.FCFile: {args.filePath} - {args.offset !== undefined && ( + {args.offset != null && (
Offset: line {args.offset}
)} - {args.limit !== undefined && ( + {args.limit != null && (
Limit: {args.limit} lines diff --git a/src/browser/components/tools/FileReadToolCall.tsx b/src/browser/components/tools/FileReadToolCall.tsx index 571dfe2cd4..31219bbf7c 100644 --- a/src/browser/components/tools/FileReadToolCall.tsx +++ b/src/browser/components/tools/FileReadToolCall.tsx @@ -104,13 +104,13 @@ export const FileReadToolCall: React.FC = ({ Path: {filePath}
- {args.offset !== undefined && ( + {args.offset != null && (
Offset: line {args.offset}
)} - {args.limit !== undefined && ( + {args.limit != null && (
Limit: {args.limit} lines diff --git a/src/browser/components/tools/TaskToolCall.tsx b/src/browser/components/tools/TaskToolCall.tsx index 4362ccee8b..0565d8667f 100644 --- a/src/browser/components/tools/TaskToolCall.tsx +++ b/src/browser/components/tools/TaskToolCall.tsx @@ -467,10 +467,7 @@ export const TaskAwaitToolCall: React.FC = ({ const suppressReportInAwaitTaskIds = taskReportLinking?.suppressReportInAwaitTaskIds; const showConfigInfo = - taskIds !== undefined || - timeoutSecs !== undefined || - args.filter !== undefined || - args.filter_exclude === true; + taskIds != null || timeoutSecs != null || args.filter != null || args.filter_exclude === true; // Summary for header const completedCount = results.filter((r) => r.status === "completed").length; @@ -562,9 +559,9 @@ export const TaskAwaitToolCall: React.FC = ({ {/* Config info */} {showConfigInfo && (
- {taskIds !== undefined && Waiting for: {taskIds.length} task(s)} - {timeoutSecs !== undefined && Timeout: {timeoutSecs}s} - {args.filter !== undefined && Filter: {args.filter}} + {taskIds != null && Waiting for: {taskIds.length} task(s)} + {timeoutSecs != null && Timeout: {timeoutSecs}s} + {args.filter != null && Filter: {args.filter}} {args.filter_exclude === true && Exclude: true}
)} diff --git a/src/common/types/tools.ts b/src/common/types/tools.ts index 8a4752f65b..2de2f0aaf0 100644 --- a/src/common/types/tools.ts +++ b/src/common/types/tools.ts @@ -113,9 +113,9 @@ export interface FileEditErrorResult extends ToolOutputUiOnlyFields { export interface FileEditInsertToolArgs { file_path: string; /** Anchor text to insert before. Content will be placed immediately before this substring. */ - insert_before?: string; + insert_before?: string | null; /** Anchor text to insert after. Content will be placed immediately after this substring. */ - insert_after?: string; + insert_after?: string | null; content: string; } diff --git a/src/common/utils/tools/toolDefinitions.ts b/src/common/utils/tools/toolDefinitions.ts index 16e6a78e70..f9bd005ec9 100644 --- a/src/common/utils/tools/toolDefinitions.ts +++ b/src/common/utils/tools/toolDefinitions.ts @@ -3,6 +3,27 @@ * * Single source of truth for all tool definitions. * Zod schemas are defined here and JSON schemas are auto-generated. + * + * ## Schema convention: `.nullish()` for optional tool parameters + * + * All optional fields in **tool input schemas** (i.e. parameters the model + * provides) MUST use `.nullish()` instead of `.optional()`. + * + * Why: OpenAI's Responses API normalizes tool schemas into strict mode, which + * forces every field into `required` and expects optional fields to accept + * `null` (via `"type": ["string", "null"]`). Using `.optional()` alone + * produces a schema without a null type, so the model is forced to hallucinate + * values for fields it would normally skip. `.nullish()` (= `.optional().nullable()`) + * emits both `null` in the type union AND keeps the field out of `required`, + * which satisfies strict-mode providers (OpenAI) while remaining compatible + * with non-strict providers (Anthropic, Google). + * + * Implementation handlers that consume these values should use `!= null` + * (loose equality) instead of `!== undefined` to correctly treat both + * `null` and `undefined` as "not provided". + * + * This does NOT apply to tool **output/result** schemas — those are constructed + * by our own backend code and always use `undefined` for absent fields. */ import { z } from "zod"; @@ -87,7 +108,7 @@ export const AskUserQuestionToolArgsSchema = z .object({ questions: z.array(AskUserQuestionQuestionSchema).min(1).max(4), // Optional prefilled answers (Claude Code supports this, though Mux typically won't use it) - answers: z.record(z.string(), z.string()).optional(), + answers: z.record(z.string(), z.string()).nullish(), }) .strict() .superRefine((args, ctx) => { @@ -137,8 +158,8 @@ const TaskAgentIdSchema = z.preprocess( const TaskToolAgentArgsSchema = z .object({ // Prefer agentId. subagent_type is a deprecated alias for backwards compatibility. - agentId: TaskAgentIdSchema.optional(), - subagent_type: SubagentTypeSchema.optional(), + agentId: TaskAgentIdSchema.nullish(), + subagent_type: SubagentTypeSchema.nullish(), prompt: z.string().min(1), title: z.string().min(1), run_in_background: z.boolean().default(false), @@ -206,13 +227,13 @@ export const TaskAwaitToolArgsSchema = z .object({ task_ids: z .array(z.string().min(1)) - .optional() + .nullish() .describe( "List of task IDs to await. When omitted, waits for all active descendant tasks of the current workspace." ), filter: z .string() - .optional() + .nullish() .describe( "Optional regex to filter bash task output lines. By default, only matching lines are returned. " + "When filter_exclude is true, matching lines are excluded instead. " + @@ -220,7 +241,7 @@ export const TaskAwaitToolArgsSchema = z ), filter_exclude: z .boolean() - .optional() + .nullish() .describe( "When true, lines matching 'filter' are excluded instead of kept. " + "Requires 'filter' to be set." @@ -228,7 +249,7 @@ export const TaskAwaitToolArgsSchema = z timeout_secs: z .number() .min(0) - .optional() + .nullish() .default(600) .describe( "Maximum time to wait in seconds for each task. " + @@ -349,14 +370,14 @@ export const TaskApplyGitPatchToolArgsSchema = z task_id: z.string().min(1).describe("Child task ID whose patch artifact should be applied"), dry_run: z .boolean() - .optional() + .nullish() .describe( "When true, attempt to apply the patch in a temporary git worktree and then discard it (does not modify the current workspace)." ), - three_way: z.boolean().optional().default(true).describe("When true, run git am with --3way"), + three_way: z.boolean().nullish().default(true).describe("When true, run git am with --3way"), force: z .boolean() - .optional() + .nullish() .describe( "When true, allow apply even if the patch was previously applied (and skip clean-tree checks)." ), @@ -466,7 +487,7 @@ export const TaskListToolArgsSchema = z .object({ statuses: z .array(TaskListStatusSchema) - .optional() + .nullish() .describe( "Task statuses to include. Defaults to active tasks: queued, running, awaiting_report." ), @@ -501,7 +522,7 @@ export const TaskListToolResultSchema = z export const AgentReportToolArgsSchema = z .object({ reportMarkdown: z.string().min(1), - title: z.string().optional(), + title: z.string().nullish(), }) .strict(); @@ -594,13 +615,13 @@ export const TOOL_DEFINITIONS = { .number() .int() .positive() - .optional() + .nullish() .describe("1-based starting line number (optional, defaults to 1)"), limit: z .number() .int() .positive() - .optional() + .nullish() .describe("Number of lines to return from offset (optional, returns all if not specified)"), }), }, @@ -650,13 +671,13 @@ export const TOOL_DEFINITIONS = { .number() .int() .positive() - .optional() + .nullish() .describe("1-based starting line number (optional, defaults to 1)"), limit: z .number() .int() .positive() - .optional() + .nullish() .describe( "Number of lines to return from offset (optional, returns all if not specified)" ), @@ -679,7 +700,7 @@ export const TOOL_DEFINITIONS = { replace_count: z .number() .int() - .optional() + .nullish() .describe( "Number of occurrences to replace (default: 1). Use -1 to replace all occurrences. If 1, old_string must be unique in the file." ), @@ -698,7 +719,7 @@ export const TOOL_DEFINITIONS = { .describe("Replacement lines. Provide an empty array to delete the specified range."), expected_lines: z .array(z.string()) - .optional() + .nullish() .describe( "Optional safety check. When provided, the current lines in the specified range must match exactly." ), @@ -718,20 +739,20 @@ export const TOOL_DEFINITIONS = { insert_before: z .string() .min(1) - .optional() + .nullish() .describe( "Anchor text to insert before. Content will be placed immediately before this substring." ), insert_after: z .string() .min(1) - .optional() + .nullish() .describe( "Anchor text to insert after. Content will be placed immediately after this substring." ), content: z.string().describe("The content to insert"), }) - .refine((data) => !(data.insert_before !== undefined && data.insert_after !== undefined), { + .refine((data) => !(data.insert_before != null && data.insert_after != null), { message: "Provide only one of insert_before or insert_after (not both).", path: ["insert_before"], }), @@ -826,11 +847,11 @@ export const TOOL_DEFINITIONS = { .finite() .min(1) .describe("1-based end line (inclusive) in the numbered output"), + // .nullish() accepts both null and undefined, so the preprocess + // hack that mapped null→undefined is no longer needed. reason: z - .preprocess( - (value) => (value === null ? undefined : value), - z.string().optional() - ) + .string() + .nullish() .describe("Optional short reason for keeping this range"), }) // Providers/models sometimes include extra keys in tool arguments; be permissive and @@ -909,7 +930,7 @@ export const TOOL_DEFINITIONS = { url: z .string() .url() - .optional() + .nullish() .describe( "Optional URL to external resource with more details (e.g., Pull Request URL). The URL persists and is displayed to the user for easy access." ), @@ -930,7 +951,7 @@ export const TOOL_DEFINITIONS = { process_id: z.string().describe("The ID of the background process to retrieve output from"), filter: z .string() - .optional() + .nullish() .describe( "Optional regex to filter output lines. By default, only matching lines are returned. " + "When filter_exclude is true, matching lines are excluded instead. " + @@ -938,7 +959,7 @@ export const TOOL_DEFINITIONS = { ), filter_exclude: z .boolean() - .optional() + .nullish() .describe( "When true, lines matching 'filter' are excluded instead of kept. " + "Key behavior: excluded lines do NOT cause early return from timeout - " + @@ -1011,7 +1032,7 @@ export const TOOL_DEFINITIONS = { message: z .string() .max(200) - .optional() + .nullish() .describe( "Optional notification body with more details (max 200 chars). " + "Keep it brief - users may only see a preview." diff --git a/src/node/services/system1/bashOutputFiltering.ts b/src/node/services/system1/bashOutputFiltering.ts index f781826bef..fee7b904c6 100644 --- a/src/node/services/system1/bashOutputFiltering.ts +++ b/src/node/services/system1/bashOutputFiltering.ts @@ -3,7 +3,7 @@ import assert from "@/common/utils/assert"; export interface System1KeepRange { start: number; end: number; - reason?: string; + reason?: string | null; } export interface ApplySystem1KeepRangesResult { diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 3d7a336fd3..eadac8afc1 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -2448,7 +2448,9 @@ export class TaskService { if (!isSuccessfulToolResult(part.output)) continue; const parsed = AgentReportToolArgsSchema.safeParse(part.input); if (!parsed.success) continue; - return parsed.data; + // Normalize null → undefined at the schema boundary so downstream + // code that expects `title?: string` doesn't need to handle null. + return { reportMarkdown: parsed.data.reportMarkdown, title: parsed.data.title ?? undefined }; } return null; } diff --git a/src/node/services/tools/agent_skill_read_file.ts b/src/node/services/tools/agent_skill_read_file.ts index afbc2bbbb6..8c81a93cc1 100644 --- a/src/node/services/tools/agent_skill_read_file.ts +++ b/src/node/services/tools/agent_skill_read_file.ts @@ -20,8 +20,8 @@ function readContentWithFileReadLimits(input: { fullContent: string; fileSize: number; modifiedTime: string; - offset?: number; - limit?: number; + offset?: number | null; + limit?: number | null; }): AgentSkillReadFileToolResult { if (input.fileSize > MAX_FILE_SIZE) { const sizeMB = (input.fileSize / (1024 * 1024)).toFixed(2); @@ -34,7 +34,7 @@ function readContentWithFileReadLimits(input: { const lines = input.fullContent === "" ? [] : input.fullContent.split("\n"); - if (input.offset !== undefined && input.offset > lines.length) { + if (input.offset != null && input.offset > lines.length) { return { success: false, error: `Offset ${input.offset} is beyond file length`, @@ -43,7 +43,7 @@ function readContentWithFileReadLimits(input: { const startLineNumber = input.offset ?? 1; const startIdx = startLineNumber - 1; - const endIdx = input.limit !== undefined ? startIdx + input.limit : lines.length; + const endIdx = input.limit != null ? startIdx + input.limit : lines.length; const numberedLines: string[] = []; let totalBytesAccumulated = 0; @@ -122,7 +122,7 @@ export const createAgentSkillReadFileTool: ToolFactory = (config: ToolConfigurat } try { - if (offset !== undefined && offset < 1) { + if (offset != null && offset < 1) { return { success: false, error: `Offset must be positive (got ${offset})`, diff --git a/src/node/services/tools/bash_output.ts b/src/node/services/tools/bash_output.ts index 94a5f15a53..12dcd93309 100644 --- a/src/node/services/tools/bash_output.ts +++ b/src/node/services/tools/bash_output.ts @@ -42,8 +42,8 @@ export const createBashOutputTool: ToolFactory = (config: ToolConfiguration) => // Pass workspaceId so getOutput can check for queued messages return await config.backgroundProcessManager.getOutput( process_id, - filter, - filter_exclude, + filter ?? undefined, + filter_exclude ?? undefined, timeout_secs, abortSignal, config.workspaceId diff --git a/src/node/services/tools/code_execution.ts b/src/node/services/tools/code_execution.ts index 7bf632ae15..569f1739ae 100644 --- a/src/node/services/tools/code_execution.ts +++ b/src/node/services/tools/code_execution.ts @@ -91,7 +91,7 @@ ${muxTypes} .number() .int() .positive() - .optional() + .nullish() .describe( "Execution timeout in seconds (default: 300, max: 3600). " + "Increase when spawning subagents that may take 5-15+ minutes." diff --git a/src/node/services/tools/file_edit_insert.ts b/src/node/services/tools/file_edit_insert.ts index f6c301bc22..583d6189c2 100644 --- a/src/node/services/tools/file_edit_insert.ts +++ b/src/node/services/tools/file_edit_insert.ts @@ -29,8 +29,8 @@ interface InsertOperationFailure { } interface InsertContentOptions { - insert_before?: string; - insert_after?: string; + insert_before?: string | null; + insert_after?: string | null; } interface GuardResolutionSuccess { @@ -147,11 +147,11 @@ function insertContent( ): InsertOperationSuccess | InsertOperationFailure { const { insert_before, insert_after } = options; - if (insert_before !== undefined && insert_after !== undefined) { + if (insert_before != null && insert_after != null) { return guardFailure("Provide only one of insert_before or insert_after (not both)."); } - if (insert_before === undefined && insert_after === undefined) { + if (insert_before == null && insert_after == null) { return guardFailure( "Provide either insert_before or insert_after guard when editing existing files." ); @@ -215,7 +215,7 @@ function resolveGuardAnchor( const fileEol = detectFileEol(originalContent); // insert_after: content goes after this anchor, so insertion point is at end of anchor - if (insert_after !== undefined) { + if (insert_after != null) { const exactResult = findUniqueSubstringIndex(originalContent, insert_after, "insert_after"); if (exactResult.success) { return { success: true, index: exactResult.index + insert_after.length }; @@ -241,7 +241,7 @@ function resolveGuardAnchor( } // insert_before: content goes before this anchor, so insertion point is at start of anchor - if (insert_before !== undefined) { + if (insert_before != null) { const exactResult = findUniqueSubstringIndex(originalContent, insert_before, "insert_before"); if (exactResult.success) { return { success: true, index: exactResult.index }; diff --git a/src/node/services/tools/file_edit_replace_shared.ts b/src/node/services/tools/file_edit_replace_shared.ts index 76876510c0..f6011ebcdf 100644 --- a/src/node/services/tools/file_edit_replace_shared.ts +++ b/src/node/services/tools/file_edit_replace_shared.ts @@ -38,7 +38,7 @@ export interface StringReplaceArgs { file_path: string; old_string: string; new_string: string; - replace_count?: number; + replace_count?: number | null; } export interface LineReplaceArgs { @@ -46,7 +46,7 @@ export interface LineReplaceArgs { start_line: number; end_line: number; new_lines: string[]; - expected_lines?: string[]; + expected_lines?: string[] | null; } /** diff --git a/src/node/services/tools/file_read.ts b/src/node/services/tools/file_read.ts index 837830981e..463ddabac5 100644 --- a/src/node/services/tools/file_read.ts +++ b/src/node/services/tools/file_read.ts @@ -81,7 +81,7 @@ export const createFileReadTool: ToolFactory = (config: ToolConfiguration) => { const startLineNumber = offset ?? 1; // Validate offset - if (offset !== undefined && offset < 1) { + if (offset != null && offset < 1) { return { success: false, error: `Offset must be positive (got ${offset})`, @@ -93,7 +93,7 @@ export const createFileReadTool: ToolFactory = (config: ToolConfiguration) => { const lines = fullContent === "" ? [] : fullContent.split("\n"); // Validate offset - if (offset !== undefined && offset > lines.length) { + if (offset != null && offset > lines.length) { return { success: false, error: `Offset ${offset} is beyond file length`, @@ -108,7 +108,7 @@ export const createFileReadTool: ToolFactory = (config: ToolConfiguration) => { // Process lines with offset and limit const startIdx = startLineNumber - 1; // Convert to 0-based index - const endIdx = limit !== undefined ? startIdx + limit : lines.length; + const endIdx = limit != null ? startIdx + limit : lines.length; for (let i = startIdx; i < Math.min(endIdx, lines.length); i++) { const line = lines[i]; diff --git a/src/node/services/tools/task_await.ts b/src/node/services/tools/task_await.ts index 927a6f63c5..82f14c2727 100644 --- a/src/node/services/tools/task_await.ts +++ b/src/node/services/tools/task_await.ts @@ -29,7 +29,7 @@ export const createTaskAwaitTool: ToolFactory = (config: ToolConfiguration) => { const taskService = requireTaskService(config, "task_await"); const timeoutMs = coerceTimeoutMs(args.timeout_secs); - const timeoutSecsForBash = args.timeout_secs; + const timeoutSecsForBash = args.timeout_secs ?? undefined; const requestedIds: string[] | null = args.task_ids && args.task_ids.length > 0 ? args.task_ids : null; @@ -115,8 +115,8 @@ export const createTaskAwaitTool: ToolFactory = (config: ToolConfiguration) => { const outputResult = await config.backgroundProcessManager.getOutput( maybeProcessId, - args.filter, - args.filter_exclude, + args.filter ?? undefined, + args.filter_exclude ?? undefined, timeoutSecsForBash, abortSignal, workspaceId, From 9659ecfdd48f34277dfaceaec2670a25c7f0bb02 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 7 Feb 2026 12:38:49 +0100 Subject: [PATCH 2/2] refactor: derive tool arg types from Zod schemas via z.infer<> Replace hand-written tool arg interfaces with z.infer<> derived types so the Zod schemas are the single source of truth. This eliminates the need to manually keep interfaces in sync when schemas change (as was needed in the prior commit for the .nullish() migration). Converted types: - BashToolArgs, FileReadToolArgs, FileEditInsertToolArgs, FileEditReplaceStringToolArgs, FileEditReplaceLinesToolArgs, TodoWriteToolArgs, TodoItem, StatusSetToolArgs, BashOutputToolArgs, BashBackgroundTerminateArgs, WebFetchToolArgs - StringReplaceArgs, LineReplaceArgs (re-exported from schema-derived types) - System1KeepRange (extracted System1KeepRangeSchema to named export) - System1KeepRangesToolArgs - InsertContentOptions (Pick from schema-derived FileEditInsertToolArgs) Change-Id: Ie7abd444135549e27479eb7047a7be4d3cb1cfa5 Signed-off-by: Thomas Kosiewski --- src/cli/toolFormatters.ts | 6 +- src/common/types/tools.ts | 91 ++++++------------- src/common/utils/tools/toolDefinitions.ts | 44 +++++---- .../services/system1/bashOutputFiltering.ts | 9 +- src/node/services/tools/bash.test.ts | 2 + src/node/services/tools/file_edit_insert.ts | 5 +- .../tools/file_edit_replace_shared.ts | 20 ++-- .../services/tools/system1_keep_ranges.ts | 6 +- src/node/services/tools/task_await.ts | 4 +- 9 files changed, 74 insertions(+), 113 deletions(-) diff --git a/src/cli/toolFormatters.ts b/src/cli/toolFormatters.ts index 0d55e65fd3..1a0b4ee2ae 100644 --- a/src/cli/toolFormatters.ts +++ b/src/cli/toolFormatters.ts @@ -120,10 +120,10 @@ function formatFileReadStart(_toolName: string, args: unknown): string | null { if (!readArgs?.file_path) return null; let suffix = ""; - if (readArgs.offset !== undefined || readArgs.limit !== undefined) { + if (readArgs.offset != null || readArgs.limit != null) { const parts: string[] = []; - if (readArgs.offset !== undefined) parts.push(`L${readArgs.offset}`); - if (readArgs.limit !== undefined) parts.push(`+${readArgs.limit}`); + if (readArgs.offset != null) parts.push(`L${readArgs.offset}`); + if (readArgs.limit != null) parts.push(`+${readArgs.limit}`); suffix = chalk.dim(` (${parts.join(", ")})`); } diff --git a/src/common/types/tools.ts b/src/common/types/tools.ts index 2de2f0aaf0..3a76e6bf1b 100644 --- a/src/common/types/tools.ts +++ b/src/common/types/tools.ts @@ -29,23 +29,14 @@ import type { WebFetchToolResultSchema, } from "@/common/utils/tools/toolDefinitions"; -// Bash Tool Types -export interface BashToolArgs { - script: string; - timeout_secs: number; // Required - defaults should be applied by producers - run_in_background?: boolean; // Run without blocking (for long-running processes) - display_name: string; // Required - used as process identifier if sent to background -} +// Bash Tool Types — derived from schema (avoid drift) +export type BashToolArgs = z.infer; // BashToolResult derived from Zod schema (single source of truth) export type BashToolResult = z.infer; -// File Read Tool Types -export interface FileReadToolArgs { - file_path: string; - offset?: number; // 1-based starting line number (optional) - limit?: number; // number of lines to return from offset (optional) -} +// File Read Tool Types — derived from schema (avoid drift) +export type FileReadToolArgs = z.infer; // Agent Skill Tool Types // Args derived from schema (avoid drift) @@ -110,35 +101,24 @@ export interface FileEditErrorResult extends ToolOutputUiOnlyFields { note?: string; // Agent-only message (not displayed in UI) } -export interface FileEditInsertToolArgs { - file_path: string; - /** Anchor text to insert before. Content will be placed immediately before this substring. */ - insert_before?: string | null; - /** Anchor text to insert after. Content will be placed immediately after this substring. */ - insert_after?: string | null; - content: string; -} +// FileEditInsertToolArgs derived from schema (avoid drift) +export type FileEditInsertToolArgs = z.infer; // FileEditInsertToolResult derived from Zod schema (single source of truth) export type FileEditInsertToolResult = z.infer; -export interface FileEditReplaceStringToolArgs { - file_path: string; - old_string: string; - new_string: string; - replace_count?: number; -} +// FileEditReplaceStringToolArgs derived from schema (avoid drift) +export type FileEditReplaceStringToolArgs = z.infer< + typeof TOOL_DEFINITIONS.file_edit_replace_string.schema +>; // FileEditReplaceStringToolResult derived from Zod schema (single source of truth) export type FileEditReplaceStringToolResult = z.infer; -export interface FileEditReplaceLinesToolArgs { - file_path: string; - start_line: number; - end_line: number; - new_lines: string[]; - expected_lines?: string[]; -} +// FileEditReplaceLinesToolArgs derived from schema (avoid drift) +export type FileEditReplaceLinesToolArgs = z.infer< + typeof TOOL_DEFINITIONS.file_edit_replace_lines.schema +>; export type FileEditReplaceLinesToolResult = | (FileEditDiffSuccessBase & { @@ -288,43 +268,28 @@ export interface LegacyProposePlanToolResult { message: string; } -// Todo Tool Types -export interface TodoItem { - content: string; - status: "pending" | "in_progress" | "completed"; -} - -export interface TodoWriteToolArgs { - todos: TodoItem[]; -} +// Todo Tool Types — derived from schema (avoid drift) +export type TodoWriteToolArgs = z.infer; +export type TodoItem = TodoWriteToolArgs["todos"][number]; export interface TodoWriteToolResult { success: true; count: number; } -// Status Set Tool Types -export interface StatusSetToolArgs { - emoji: string; - message: string; - url?: string; -} +// Status Set Tool Types — derived from schema (avoid drift) +export type StatusSetToolArgs = z.infer; -// Bash Output Tool Types (read incremental output from background processes) -export interface BashOutputToolArgs { - process_id: string; - filter?: string; - filter_exclude?: boolean; - timeout_secs: number; -} +// Bash Output Tool Types — derived from schema (avoid drift) +export type BashOutputToolArgs = z.infer; // BashOutputToolResult derived from Zod schema (single source of truth) export type BashOutputToolResult = z.infer; -// Bash Background Tool Types -export interface BashBackgroundTerminateArgs { - process_id: string; -} +// Bash Background Tool Types — derived from schema (avoid drift) +export type BashBackgroundTerminateArgs = z.infer< + typeof TOOL_DEFINITIONS.bash_background_terminate.schema +>; // BashBackgroundTerminateResult derived from Zod schema (single source of truth) export type BashBackgroundTerminateResult = z.infer; @@ -353,10 +318,8 @@ export type StatusSetToolResult = error: string; }; -// Web Fetch Tool Types -export interface WebFetchToolArgs { - url: string; -} +// Web Fetch Tool Types — derived from schema (avoid drift) +export type WebFetchToolArgs = z.infer; // WebFetchToolResult derived from Zod schema (single source of truth) export type WebFetchToolResult = z.infer; diff --git a/src/common/utils/tools/toolDefinitions.ts b/src/common/utils/tools/toolDefinitions.ts index f9bd005ec9..d83f7790f5 100644 --- a/src/common/utils/tools/toolDefinitions.ts +++ b/src/common/utils/tools/toolDefinitions.ts @@ -541,6 +541,30 @@ interface ToolSchema { }; } +/** + * Schema for a single keep-range item in the system1_keep_ranges tool. + * Extracted as a named export so internal code can derive the type via z.infer<> + * instead of maintaining a hand-written interface. + * + * Note: the tool schema applies .passthrough() on top of this to tolerate extra + * keys from models, but the inferred type is the strict shape. + */ +export const System1KeepRangeSchema = z.object({ + start: z.coerce + .number() + .finite() + .min(1) + .describe("1-based start line (inclusive) in the numbered output"), + end: z.coerce + .number() + .finite() + .min(1) + .describe("1-based end line (inclusive) in the numbered output"), + // .nullish() accepts both null and undefined, so the preprocess + // hack that mapped null→undefined is no longer needed. + reason: z.string().nullish().describe("Optional short reason for keeping this range"), +}); + /** * Tool definitions: single source of truth * Key = tool name, Value = { description, schema } @@ -835,25 +859,7 @@ export const TOOL_DEFINITIONS = { .object({ keep_ranges: z .array( - z - .object({ - start: z.coerce - .number() - .finite() - .min(1) - .describe("1-based start line (inclusive) in the numbered output"), - end: z.coerce - .number() - .finite() - .min(1) - .describe("1-based end line (inclusive) in the numbered output"), - // .nullish() accepts both null and undefined, so the preprocess - // hack that mapped null→undefined is no longer needed. - reason: z - .string() - .nullish() - .describe("Optional short reason for keeping this range"), - }) + System1KeepRangeSchema // Providers/models sometimes include extra keys in tool arguments; be permissive and // ignore them rather than failing the whole compaction call. .passthrough() diff --git a/src/node/services/system1/bashOutputFiltering.ts b/src/node/services/system1/bashOutputFiltering.ts index fee7b904c6..1a0d47cf88 100644 --- a/src/node/services/system1/bashOutputFiltering.ts +++ b/src/node/services/system1/bashOutputFiltering.ts @@ -1,10 +1,9 @@ import assert from "@/common/utils/assert"; +import type { z } from "zod"; +import type { System1KeepRangeSchema } from "@/common/utils/tools/toolDefinitions"; -export interface System1KeepRange { - start: number; - end: number; - reason?: string | null; -} +// Derived from the Zod schema (single source of truth) to avoid drift. +export type System1KeepRange = z.infer; export interface ApplySystem1KeepRangesResult { filteredOutput: string; diff --git a/src/node/services/tools/bash.test.ts b/src/node/services/tools/bash.test.ts index ae67dabf0d..bf1d13f0d6 100644 --- a/src/node/services/tools/bash.test.ts +++ b/src/node/services/tools/bash.test.ts @@ -1587,6 +1587,7 @@ describe("muxEnv environment variables", () => { const args: BashToolArgs = { script: 'echo "PROJECT:$MUX_PROJECT_PATH RUNTIME:$MUX_RUNTIME WORKSPACE:$MUX_WORKSPACE_NAME"', timeout_secs: 5, + run_in_background: false, display_name: "test", }; @@ -1616,6 +1617,7 @@ describe("muxEnv environment variables", () => { const args: BashToolArgs = { script: 'echo "MUX:$MUX_PROJECT_PATH CUSTOM:$CUSTOM_VAR"', timeout_secs: 5, + run_in_background: false, display_name: "test", }; diff --git a/src/node/services/tools/file_edit_insert.ts b/src/node/services/tools/file_edit_insert.ts index 583d6189c2..292a4babcb 100644 --- a/src/node/services/tools/file_edit_insert.ts +++ b/src/node/services/tools/file_edit_insert.ts @@ -28,10 +28,7 @@ interface InsertOperationFailure { note?: string; } -interface InsertContentOptions { - insert_before?: string | null; - insert_after?: string | null; -} +type InsertContentOptions = Pick; interface GuardResolutionSuccess { success: true; diff --git a/src/node/services/tools/file_edit_replace_shared.ts b/src/node/services/tools/file_edit_replace_shared.ts index f6011ebcdf..129196697b 100644 --- a/src/node/services/tools/file_edit_replace_shared.ts +++ b/src/node/services/tools/file_edit_replace_shared.ts @@ -10,6 +10,8 @@ import { NOTE_READ_FILE_FIRST_RETRY, NOTE_READ_FILE_RETRY, NOTE_READ_FILE_AGAIN_RETRY, + type FileEditReplaceStringToolArgs, + type FileEditReplaceLinesToolArgs, } from "@/common/types/tools"; import { convertNewlines, detectFileEol, normalizeNewlinesToLF } from "./eol"; @@ -34,20 +36,10 @@ export interface OperationError { export type OperationOutcome = OperationResult | OperationError; -export interface StringReplaceArgs { - file_path: string; - old_string: string; - new_string: string; - replace_count?: number | null; -} - -export interface LineReplaceArgs { - file_path: string; - start_line: number; - end_line: number; - new_lines: string[]; - expected_lines?: string[] | null; -} +// Re-export schema-derived types for backward compatibility. +// Local code previously imported StringReplaceArgs / LineReplaceArgs from this module. +export type StringReplaceArgs = FileEditReplaceStringToolArgs; +export type LineReplaceArgs = FileEditReplaceLinesToolArgs; /** * Handle string-based replacement diff --git a/src/node/services/tools/system1_keep_ranges.ts b/src/node/services/tools/system1_keep_ranges.ts index 42f6bfe80b..c726c5177c 100644 --- a/src/node/services/tools/system1_keep_ranges.ts +++ b/src/node/services/tools/system1_keep_ranges.ts @@ -1,14 +1,14 @@ import { tool } from "ai"; import type { Tool } from "ai"; +import type { z } from "zod"; import type { ToolConfiguration } from "@/common/utils/tools/tools"; import { TOOL_DEFINITIONS } from "@/common/utils/tools/toolDefinitions"; import type { System1KeepRange } from "@/node/services/system1/bashOutputFiltering"; -export interface System1KeepRangesToolArgs { - keep_ranges: System1KeepRange[]; -} +// Derived from the Zod schema (single source of truth) to avoid drift. +export type System1KeepRangesToolArgs = z.infer; export type System1KeepRangesToolResult = | { diff --git a/src/node/services/tools/task_await.ts b/src/node/services/tools/task_await.ts index 82f14c2727..852c502c39 100644 --- a/src/node/services/tools/task_await.ts +++ b/src/node/services/tools/task_await.ts @@ -29,7 +29,9 @@ export const createTaskAwaitTool: ToolFactory = (config: ToolConfiguration) => { const taskService = requireTaskService(config, "task_await"); const timeoutMs = coerceTimeoutMs(args.timeout_secs); - const timeoutSecsForBash = args.timeout_secs ?? undefined; + // Preserve the documented 600s default when the model sends null + // (Zod .default() only replaces undefined, not null). + const timeoutSecsForBash = args.timeout_secs ?? 600; const requestedIds: string[] | null = args.task_ids && args.task_ids.length > 0 ? args.task_ids : null;