fix(error-interception): address PR review findings and improve guidance

Author: Zoo (VP)

.gitignore

@@ -55,3 +55,12 @@ qdrant_storage/
 plans/
 
 roo-cli-*.tar.gz*
+
+# Local dev scripts (not for CI)
+/ci-fix-commit.ps1
+/commit-and-push.ps1
+/commit-message.txt
+/resolve_conflicts.py
+
+# AI session artifacts (timestamped session directories)
+/docs/*_session_*/

ci-fix-commit.ps1

@@ -38,6 +38,32 @@ type NativeArgsFor<TName extends ToolName> = TName extends keyof NativeToolArgs
  */
 export type ToolCallStreamEvent = ApiStreamToolCallStartChunk | ApiStreamToolCallDeltaChunk | ApiStreamToolCallEndChunk
 
+/**
+ * Discriminated union for parser failure kinds.
+ *
+ * - `json_syntax`: The arguments string could not be parsed as JSON.
+ * - `missing_required_arguments`: The JSON was valid but one or more required
+ *   fields were absent (including the empty-object case).
+ * - `invalid_argument_shape`: The JSON was valid and required field names were
+ *   present, but the structural shape did not match the tool schema (e.g. a
+ *   field had the wrong type or the value could not be coerced).
+ */
+export type ParserFailureKind = "json_syntax" | "missing_required_arguments" | "invalid_argument_shape"
+
+/**
+ * Typed, sanitized descriptor for a parser failure.
+ *
+ * IMPORTANT: This descriptor MUST NOT contain raw argument bodies, file paths,
+ * commands, task IDs, or secrets. It carries only structural facts needed for
+ * error classification and model guidance.
+ */
+export interface NativeToolParseFailure {
+	kind: ParserFailureKind
+	toolName?: string
+	missingParameters?: string[] // Known missing required field names from parser's tool contract
+	emptyArguments?: boolean // true if the input was {} or ""
+}
+
 /**
  * Parser for native tool calls (OpenAI-style function calling).
  * Converts native tool call format to ToolUse format for compatibility
@@ -79,12 +105,76 @@ export class NativeToolCallParser {
 	 * here so presentAssistantMessage can retrieve it and route the signal to
 	 * the INVALID_JSON_ARGUMENTS error-interception pattern instead of the
 	 * generic PARAM_MISSING path.
+	 *
+	 * @deprecated Use {@link parseFailures} and {@link consumeParseFailure} for
+	 * typed failure descriptors. This legacy string map is retained only as a
+	 * compatibility wrapper for human diagnostics.
 	 */
 	private static parseErrors = new Map<string, string>()
 
+	/**
+	 * Stores typed parser failure descriptors keyed by tool call ID.
+	 * When parseToolCall() catches any failure (JSON syntax, missing required
+	 * arguments, or invalid argument shape), it records a typed descriptor here
+	 * so downstream consumers can classify the failure precisely instead of
+	 * relying on raw error strings.
+	 */
+	private static parseFailures = new Map<string, NativeToolParseFailure>()
+
+	/**
+	 * Required parameter names for each native tool, derived from
+	 * {@link NativeToolArgs}. Used to classify missing-required-arguments
+	 * failures with precise field names.
+	 */
+	private static readonly REQUIRED_PARAMETERS: Record<string, string[]> = {
+		access_mcp_resource: ["server_name", "uri"],
+		read_file: ["path"],
+		read_command_output: ["artifact_id"],
+		attempt_completion: ["result"],
+		execute_command: ["command"],
+		apply_diff: ["path", "diff"],
+		edit: ["file_path", "old_string", "new_string"],
+		search_and_replace: ["file_path", "old_string", "new_string"],
+		search_replace: ["file_path", "old_string", "new_string"],
+		edit_file: ["file_path", "old_string", "new_string"],
+		apply_patch: ["patch"],
+		list_files: ["path"],
+		new_task: ["mode", "message"],
+		ask_followup_question: ["question", "follow_up"],
+		codebase_search: ["query"],
+		generate_image: ["prompt", "path"],
+		run_slash_command: ["command"],
+		skill: ["skill"],
+		search_files: ["path", "regex"],
+		switch_mode: ["mode_slug", "reason"],
+		update_todo_list: ["todos"],
+		use_mcp_tool: ["server_name", "tool_name"],
+		write_to_file: ["path", "content"],
+	}
+
+	/**
+	 * Retrieve and remove the typed parse failure descriptor for a given tool
+	 * call ID. Returns undefined if no failure was recorded or if it was
+	 * already consumed.
+	 *
+	 * Atomic consume-and-delete, matching the lifecycle of the legacy
+	 * {@link consumeParseError} string side channel.
+	 */
+	public static consumeParseFailure(toolCallId: string): NativeToolParseFailure | undefined {
+		const failure = NativeToolCallParser.parseFailures.get(toolCallId)
+		if (failure !== undefined) {
+			NativeToolCallParser.parseFailures.delete(toolCallId)
+		}
+		return failure
+	}
+
 	/**
 	 * Retrieve and remove the parse error for a given tool call ID.
 	 * Returns undefined if no parse error was recorded.
+	 *
+	 * @deprecated Compatibility wrapper. New production code should use
+	 * {@link consumeParseFailure} for typed failure descriptors. This method
+	 * returns the string representation for human diagnostics only.
 	 */
 	public static consumeParseError(toolCallId: string): string | undefined {
 		const error = NativeToolCallParser.parseErrors.get(toolCallId)
@@ -1032,11 +1122,43 @@ export class NativeToolCallParser {
 			// Native-only: core tools must always have typed nativeArgs.
 			// If we couldn't construct it, the model produced an invalid tool call payload.
 			if (!nativeArgs && !customToolRegistry.has(resolvedName)) {
-				throw new Error(
-					`[NativeToolCallParser] Invalid arguments for tool '${resolvedName}'. ` +
-						`Native tool calls require a valid JSON payload matching the tool schema. ` +
-						`Received: ${JSON.stringify(args)}`,
-				)
+				// Classify the failure precisely so the catch block can store a
+				// typed descriptor instead of a raw error string.
+				//
+				// If args is not a plain object (e.g. a primitive, array, or null),
+				// the structural shape is fundamentally wrong.
+				const isPlainObject = typeof args === "object" && args !== null && !Array.isArray(args)
+
+				if (!isPlainObject) {
+					throw {
+						__parserFailureKind: "invalid_argument_shape" as const,
+						toolName: resolvedName as string,
+						missingParameters: [],
+						emptyArguments: false,
+					}
+				}
+
+				const required = NativeToolCallParser.REQUIRED_PARAMETERS[resolvedName as string] ?? []
+				const missing = required.filter((p) => args[p] === undefined)
+				const isEmpty = Object.keys(args).length === 0
+
+				if (missing.length > 0) {
+					throw {
+						__parserFailureKind: "missing_required_arguments" as const,
+						toolName: resolvedName as string,
+						missingParameters: missing,
+						emptyArguments: isEmpty,
+					}
+				}
+
+				// Required fields are present but the structural shape didn't match
+				// any known pattern in the switch above.
+				throw {
+					__parserFailureKind: "invalid_argument_shape" as const,
+					toolName: resolvedName as string,
+					missingParameters: [],
+					emptyArguments: isEmpty,
+				}
 			}
 
 			const result: ToolUse<TName> = {
@@ -1059,23 +1181,67 @@ export class NativeToolCallParser {
 
 			return result
 		} catch (error) {
-				const errorMessage = error instanceof Error ? error.message : String(error)
-	
-				console.error(
-					`Failed to parse tool call arguments: ${errorMessage}`,
-				)
-	
-				console.error(`Tool call: ${JSON.stringify(toolCall, null, 2)}`)
-	
-				// Store the parse error so presentAssistantMessage can route it
-				// to the INVALID_JSON_ARGUMENTS error-interception pattern
-				// instead of the generic PARAM_MISSING path.
-				NativeToolCallParser.parseErrors.set(toolCall.id, errorMessage)
-	
-				return null
+			// Determine whether this is a JSON.parse syntax failure or a
+			// post-parse structural failure (missing required arguments or
+			// invalid argument shape). The structural failures are thrown as
+			// tagged objects with __parserFailureKind; JSON.parse failures are
+			// standard SyntaxError instances.
+			const failure = NativeToolCallParser.classifyParseFailure(error, resolvedName as string)
+
+			const errorMessage = error instanceof Error ? error.message : String(error)
+
+			console.error(`Failed to parse tool call arguments: ${errorMessage}`)
+
+			console.error(`Tool call: ${JSON.stringify(toolCall, null, 2)}`)
+
+			// Store the legacy string error for backward compatibility with
+			// existing callers of consumeParseError().
+			NativeToolCallParser.parseErrors.set(toolCall.id, errorMessage)
+
+			// Store the typed failure descriptor for new callers that use
+			// consumeParseFailure().
+			NativeToolCallParser.parseFailures.set(toolCall.id, failure)
+
+			return null
+		}
+	}
+
+	/**
+	 * Classify a caught error from parseToolCall() into a typed
+	 * {@link NativeToolParseFailure} descriptor.
+	 *
+	 * - If the error is a tagged object with `__parserFailureKind`, it was
+	 *   thrown by the structural validation logic and carries precise metadata.
+	 * - Otherwise, the error originated from JSON.parse (a SyntaxError) and is
+	 *   classified as `json_syntax`.
+	 */
+	private static classifyParseFailure(error: unknown, toolName: string): NativeToolParseFailure {
+		// Check for tagged structural failure objects thrown by the validation
+		// logic above. These are not Error instances — they are plain objects
+		// with a __parserFailureKind discriminator.
+		if (typeof error === "object" && error !== null && "__parserFailureKind" in error) {
+			const tagged = error as {
+				__parserFailureKind: ParserFailureKind
+				toolName?: string
+				missingParameters?: string[]
+				emptyArguments?: boolean
+			}
+			return {
+				kind: tagged.__parserFailureKind,
+				toolName: tagged.toolName ?? toolName,
+				missingParameters: tagged.missingParameters,
+				emptyArguments: tagged.emptyArguments,
 			}
 		}
 
+		// Any other error (SyntaxError from JSON.parse, or unexpected runtime
+		// error) is classified as a JSON syntax failure.
+		return {
+			kind: "json_syntax",
+			toolName,
+		}
+	}
+
 	/**
 	 * Parse dynamic MCP tools (named mcp--serverName--toolName).
 	 * These are generated dynamically by getMcpServerTools() and are returned