From ecefcc3303b65f5319faaf7f98d8d4273baa4782 Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Sun, 12 Jul 2026 12:34:35 -0700 Subject: [PATCH 1/3] fix(tools): semantic input coercion + formatZodValidationError-parity messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live incident: a model sent Grep the key `n` instead of `-n` and got the port's terse 'Grep.n: unexpected field'. The rejection is parity-correct (z.strictObject refuses unknown keys), but the incident exposed two gaps in the validation path vs the original: - Error text now mirrors formatZodValidationError (toolErrors.ts:68): 'Grep failed due to the following issue: An unexpected parameter `n` was provided' with the missing / unexpected / type-mismatch categories grouped in the original's order and JS-flavored received-type names. Uncategorized issues (enum etc.) keep the port's readable fallback (the original dumps zod's raw error.message there — deliberate deviation). - Semantic coercion (semanticBoolean.ts / semanticNumber.ts): string 'true'/'false' for boolean fields and decimal-literal strings for number fields now coerce BEFORE validation, and the coerced input is what flows to hooks/permissions/call (TS carries parsedInput.data forward). Previously the validator hard-rejected quoted scalars the original accepts, e.g. '-n': 'true' or head_limit: '30' — the most common third-party-model flake — and the tools' call-time coercion helpers never ran. Type-driven at the boundary (every boolean/number/ integer schema node), never inside anyOf/oneOf unions; copy-on-write so the assistant message's recorded tool_use input stays raw. validate_json_schema keeps its exact old rendering and strictness for generic callers (workflow structured output, advisor). Co-Authored-By: Claude Fable 5 --- src/entrypoints/mcp_serve.py | 7 +- src/services/tool_execution/tool_execution.py | 10 +- src/tool_system/registry.py | 11 +- src/tool_system/schema_validation.py | 251 +++++++++++- tests/test_tool_input_validation.py | 365 ++++++++++++++++++ 5 files changed, 617 insertions(+), 27 deletions(-) create mode 100644 tests/test_tool_input_validation.py diff --git a/src/entrypoints/mcp_serve.py b/src/entrypoints/mcp_serve.py index 55ac6d635..90e41be73 100644 --- a/src/entrypoints/mcp_serve.py +++ b/src/entrypoints/mcp_serve.py @@ -26,9 +26,10 @@ * **No output schemas:** this port's tools declare none, so none are emitted (pre-empts TS's object-rooted-only branch, mcp.ts:106-120). * **SDK input validation disabled** (``validate_input=False``): the - registry's ``validate_json_schema`` runs inside ``dispatch`` and produces - this port's error text — the analog of TS shaping its own ZodError - message (mcp.ts:231-241) rather than letting the transport layer do it. + registry's ``validate_tool_input`` runs inside ``dispatch`` and produces + ``formatZodValidationError``-parity error text — the analog of TS shaping + its own ZodError message (mcp.ts:231-241) rather than letting the + transport layer do it. * **No provider:** the registry is built with ``provider=None`` — the serve surface exposes the tool set, not the model stack; agent-spawning tools report "no provider configured" honestly at call time (divergence diff --git a/src/services/tool_execution/tool_execution.py b/src/services/tool_execution/tool_execution.py index 79dade782..aef042797 100644 --- a/src/services/tool_execution/tool_execution.py +++ b/src/services/tool_execution/tool_execution.py @@ -170,15 +170,19 @@ async def _check_permissions_and_call_tool( # Mirrors typescript/src/services/tools/toolExecution.ts schema check at # the head of checkPermissionsAndCallTool. Skipped when the tool has no # input_schema (defensive — every Tool should declare one). + # ``validate_tool_input`` returns the semantically-coerced input (string + # "true"/"30" → bool/number, the zod preprocess wrappers) and the + # coerced object is what flows onward — TS carries ``parsedInput.data`` + # forward (toolExecution.ts:669 → 821), not the raw model input. if getattr(tool, "input_schema", None): try: from src.tool_system.schema_validation import ( build_schema_not_sent_hint, - validate_json_schema, + validate_tool_input, ) - validate_json_schema( - processed_input, tool.input_schema, root_name=tool.name, + processed_input = validate_tool_input( + tool.name, processed_input, tool.input_schema, ) except Exception as schema_err: # ToolInputError or any subclass msg = str(schema_err) diff --git a/src/tool_system/registry.py b/src/tool_system/registry.py index 72d435d09..e06cc6d3e 100644 --- a/src/tool_system/registry.py +++ b/src/tool_system/registry.py @@ -7,7 +7,7 @@ from .build_tool import Tool, Tools, tool_matches_name from .context import ToolContext from .protocol import ToolCall, ToolResult -from .schema_validation import validate_json_schema +from .schema_validation import validate_tool_input from src.permissions.check import has_permissions_to_use_tool from src.permissions.handler import handle_permission_ask from src.permissions.types import ( @@ -184,7 +184,14 @@ def dispatch(self, call: ToolCall, context: ToolContext) -> ToolResult: ) context.ensure_tool_allowed(tool.name) - validate_json_schema(call.input, tool.input_schema, root_name=tool.name) + # Semantic-coerce + validate; the coerced input (string "true"/"30" → + # bool/number) replaces the raw model input for permissions and call, + # mirroring TS carrying ``parsedInput.data`` forward. + coerced_input = validate_tool_input(tool.name, call.input, tool.input_schema) + if coerced_input is not call.input: + call = ToolCall( + name=call.name, input=coerced_input, tool_use_id=call.tool_use_id, + ) if tool.validate_input is not None: validation = tool.validate_input(call.input, context) diff --git a/src/tool_system/schema_validation.py b/src/tool_system/schema_validation.py index e4993ff08..2b7726a65 100644 --- a/src/tool_system/schema_validation.py +++ b/src/tool_system/schema_validation.py @@ -1,7 +1,9 @@ from __future__ import annotations +import math +import re from dataclasses import dataclass -from typing import Any, Iterable, Mapping +from typing import Any, Mapping from .errors import ToolInputError @@ -10,6 +12,17 @@ class ValidationIssue: path: str message: str + # Structured fields consumed by the TS-parity tool-input renderer + # (``format_tool_validation_error``). ``kind`` mirrors the three issue + # categories ``formatZodValidationError`` (typescript/src/utils/ + # toolErrors.ts:68) extracts from a ZodError: "missing" (invalid_type + + # "received undefined"), "unexpected" (unrecognized_keys) and "type" + # (remaining invalid_type). Anything else stays "other" and falls back + # to the generic per-issue rendering. + kind: str = "other" + param: str = "" + expected: str = "" + received: str = "" def _type_name(value: Any) -> str: @@ -30,18 +43,190 @@ def _type_name(value: Any) -> str: return type(value).__name__ -def _as_set(values: Iterable[str] | None) -> set[str]: - return set(values or []) +def _js_type_name(value: Any) -> str: + """JS-flavored type name for TS-parity error text. + + ``formatZodValidationError`` reports the received type with JavaScript's + vocabulary ("provided as `number`"); Python's int/float split must not + leak into the message, so both map to "number". + """ + if value is None: + return "null" + if isinstance(value, bool): + return "boolean" + if isinstance(value, (int, float)): + return "number" + if isinstance(value, str): + return "string" + if isinstance(value, list): + return "array" + if isinstance(value, dict): + return "object" + return type(value).__name__ + + +def _child_key_path(path: str, key: str) -> str: + return f"{path}.{key}" if path else key + + +def _child_index_path(path: str, idx: int) -> str: + return f"{path}[{idx}]" + + +def _render_issues(issues: list[ValidationIssue]) -> str: + rendered = "; ".join(f"{i.path}: {i.message}" for i in issues[:5]) + if len(issues) > 5: + rendered += f"; (+{len(issues) - 5} more)" + return rendered def validate_json_schema(value: Any, schema: Mapping[str, Any], *, root_name: str = "input") -> None: issues: list[ValidationIssue] = [] _validate(value, schema, path=root_name, issues=issues) if issues: - rendered = "; ".join(f"{i.path}: {i.message}" for i in issues[:5]) - if len(issues) > 5: - rendered += f"; (+{len(issues) - 5} more)" - raise ToolInputError(rendered) + raise ToolInputError(_render_issues(issues)) + + +# -- Semantic coercion (TS parity) -------------------------------------------- +# +# The original wraps model-facing boolean/number fields in ``semanticBoolean`` +# / ``semanticNumber`` (typescript/src/utils/semanticBoolean.ts, +# semanticNumber.ts): zod ``preprocess`` steps that coerce the string literals +# "true"/"false" and decimal number literals BEFORE type validation, while the +# API-advertised schema stays a plain {"type": "boolean"|"number"}. Models — +# third-party ones especially — routinely quote scalars ("head_limit": "30", +# "-n": "true"); without the preprocess step the validator hard-rejects input +# the original accepts and the tool's own call-time coercion never runs. +# +# The port applies the coercion type-driven at the validation boundary rather +# than replicating the per-field wrappers: every schema node declaring type +# boolean/number/integer coerces. That covers the original's wrapped fields +# (Bash, FileEdit, FileRead, Grep, CronCreate, SendMessage, TaskOutput, …) +# exactly, and additionally tolerates quoted scalars on the few unwrapped +# fields (e.g. AgentTool.run_in_background) and on MCP tool schemas — a +# deliberate, tolerant-direction deviation. ``anyOf``/``oneOf`` nodes never +# coerce, mirroring the original's unwrapped unions (ConfigTool.value must +# keep "true" a string). + +_NUMBER_LITERAL_RE = re.compile(r"-?\d+(\.\d+)?") + + +def semantic_coerce(value: Any, schema: Mapping[str, Any]) -> Any: + """Return ``value`` with semantic string→scalar coercions applied. + + Copy-on-write: containers are rebuilt only when a nested coercion fired, + and the input is never mutated — callers may share the original dict with + the assistant message's recorded ``tool_use`` block, which must keep the + model's raw input. + """ + if not isinstance(schema, Mapping): + return value + if "anyOf" in schema or "oneOf" in schema: + return value + + schema_type = schema.get("type") + + if schema_type == "object" and isinstance(value, dict): + properties = schema.get("properties") + if not isinstance(properties, Mapping): + return value + out: dict[str, Any] | None = None + for key, item in value.items(): + prop_schema = properties.get(key) + if not isinstance(prop_schema, Mapping): + continue + coerced = semantic_coerce(item, prop_schema) + if coerced is not item: + if out is None: + out = dict(value) + out[key] = coerced + return out if out is not None else value + + if schema_type == "array" and isinstance(value, list): + item_schema = schema.get("items") + if not isinstance(item_schema, Mapping): + return value + coerced_items = [semantic_coerce(item, item_schema) for item in value] + if any(c is not o for c, o in zip(coerced_items, value)): + return coerced_items + return value + + if isinstance(value, str): + if schema_type == "boolean": + # Exactly "true"/"false" — semanticBoolean.ts coerces nothing + # else (JS-truthiness coercion would turn "false" into True). + if value == "true": + return True + if value == "false": + return False + elif schema_type in ("number", "integer"): + # Only decimal number literals, and only when finite as a JS + # number — semanticNumber.ts's /^-?\d+(\.\d+)?$/ gate plus its + # Number.isFinite check (a 400-digit literal overflows to + # Infinity in JS and is NOT coerced there; mirror that). + if _NUMBER_LITERAL_RE.fullmatch(value): + if math.isinf(float(value)): + return value + return float(value) if "." in value else int(value) + + return value + + +def format_tool_validation_error(tool_name: str, issues: list[ValidationIssue]) -> str: + """Render issues the way the original renders a failed tool-input parse. + + Mirrors ``formatZodValidationError`` (typescript/src/utils/toolErrors.ts: + 68-134): the three curated categories render grouped in fixed order — + missing, unexpected, type mismatch — under a "{tool} failed due to the + following issue(s):" header. When no issue falls in a curated category the + original falls back to zod's raw ``error.message`` (a JSON dump); the port + keeps its readable per-issue rendering instead (deliberate deviation). + """ + missing = [i for i in issues if i.kind == "missing"] + unexpected = [i for i in issues if i.kind == "unexpected"] + mismatched = [i for i in issues if i.kind == "type"] + + parts = [f"The required parameter `{i.param}` is missing" for i in missing] + parts += [f"An unexpected parameter `{i.param}` was provided" for i in unexpected] + parts += [ + f"The parameter `{i.param}` type is expected as `{i.expected}` but provided as `{i.received}`" + for i in mismatched + ] + + if parts: + noun = "issues" if len(parts) > 1 else "issue" + return f"{tool_name} failed due to the following {noun}:\n" + "\n".join(parts) + + prefixed = "; ".join( + f"{tool_name}.{i.path}: {i.message}" if i.path else f"{tool_name}: {i.message}" + for i in issues[:5] + ) + if len(issues) > 5: + prefixed += f"; (+{len(issues) - 5} more)" + return prefixed + + +def validate_tool_input(tool_name: str, value: Any, schema: Mapping[str, Any]) -> Any: + """Semantic-coerce then validate a tool input; return the input the + pipeline should carry forward. + + The tool-dispatch counterpart of ``validate_json_schema``. Mirrors the + original's ``tool.inputSchema.safeParse(normalizedInput)`` at + typescript/src/services/tools/toolExecution.ts:669: the zod schema both + coerces (semantic preprocess wrappers) and validates, and the *parsed* + output — not the raw model input — is what flows on to hooks, permission + checks and ``call()`` (``let processedInput = parsedInput.data``, + toolExecution.ts:821). On failure raises ``ToolInputError`` with + ``formatZodValidationError``-parity text. + """ + if not isinstance(schema, Mapping): + return value + coerced = semantic_coerce(value, schema) + issues: list[ValidationIssue] = [] + _validate(coerced, schema, path="", issues=issues) + if issues: + raise ToolInputError(format_tool_validation_error(tool_name, issues)) + return coerced def _validate(value: Any, schema: Mapping[str, Any], *, path: str, issues: list[ValidationIssue]) -> None: @@ -63,34 +248,34 @@ def _validate(value: Any, schema: Mapping[str, Any], *, path: str, issues: list[ if expected_type: if expected_type == "object": if not isinstance(value, dict): - issues.append(ValidationIssue(path, f"expected object, got {_type_name(value)}")) + issues.append(_type_issue(path, value, "object")) return _validate_object(value, schema, path=path, issues=issues) return if expected_type == "array": if not isinstance(value, list): - issues.append(ValidationIssue(path, f"expected array, got {_type_name(value)}")) + issues.append(_type_issue(path, value, "array")) return item_schema = schema.get("items") if isinstance(item_schema, dict): for idx, item in enumerate(value): - _validate(item, item_schema, path=f"{path}[{idx}]", issues=issues) + _validate(item, item_schema, path=_child_index_path(path, idx), issues=issues) return if expected_type == "string": if not isinstance(value, str): - issues.append(ValidationIssue(path, f"expected string, got {_type_name(value)}")) + issues.append(_type_issue(path, value, "string")) return elif expected_type == "boolean": if not isinstance(value, bool): - issues.append(ValidationIssue(path, f"expected boolean, got {_type_name(value)}")) + issues.append(_type_issue(path, value, "boolean")) return elif expected_type == "number": if not isinstance(value, (int, float)) or isinstance(value, bool): - issues.append(ValidationIssue(path, f"expected number, got {_type_name(value)}")) + issues.append(_type_issue(path, value, "number")) return elif expected_type == "integer": if not isinstance(value, int) or isinstance(value, bool): - issues.append(ValidationIssue(path, f"expected integer, got {_type_name(value)}")) + issues.append(_type_issue(path, value, "integer")) return if "enum" in schema: @@ -100,23 +285,52 @@ def _validate(value: Any, schema: Mapping[str, Any], *, path: str, issues: list[ return +def _type_issue(path: str, value: Any, expected: str) -> ValidationIssue: + return ValidationIssue( + path, + f"expected {expected}, got {_type_name(value)}", + kind="type", + param=path, + expected=expected, + received=_js_type_name(value), + ) + + def _validate_object(value: dict[str, Any], schema: Mapping[str, Any], *, path: str, issues: list[ValidationIssue]) -> None: - required = _as_set(schema.get("required")) + required = schema.get("required") or [] properties = schema.get("properties") or {} additional = schema.get("additionalProperties", True) + seen_required: set[str] = set() for req in required: + if req in seen_required: + continue + seen_required.add(req) if req not in value: - issues.append(ValidationIssue(path, f"missing required field {req!r}")) + issues.append(ValidationIssue( + path, + f"missing required field {req!r}", + kind="missing", + # The original reports the full formatted path of the missing + # key (formatValidationPath on the zod issue path). + param=_child_key_path(path, req), + )) for key, val in value.items(): prop_schema = properties.get(key) if isinstance(properties, dict) else None if prop_schema is None: if additional is False: - issues.append(ValidationIssue(f"{path}.{key}", "unexpected field")) + issues.append(ValidationIssue( + _child_key_path(path, key), + "unexpected field", + kind="unexpected", + # The original renders the BARE key for unrecognized_keys + # (it flatMaps ``err.keys``, not the issue path). + param=key, + )) continue if isinstance(prop_schema, dict): - _validate(val, prop_schema, path=f"{path}.{key}", issues=issues) + _validate(val, prop_schema, path=_child_key_path(path, key), issues=issues) def _is_valid(value: Any, schema: Mapping[str, Any]) -> bool: @@ -142,4 +356,3 @@ def build_schema_not_sent_hint(tool: Any) -> str: "is loaded on demand. Call the ToolSearchTool first with this tool's " "name to retrieve the schema, then re-issue the call." ) - diff --git a/tests/test_tool_input_validation.py b/tests/test_tool_input_validation.py new file mode 100644 index 000000000..ba8aee6f0 --- /dev/null +++ b/tests/test_tool_input_validation.py @@ -0,0 +1,365 @@ +"""Tool-input validation parity: semantic coercion + formatZodValidationError text. + +Regression suite for the live failure ``InputValidationError: Grep.n: +unexpected field`` — the model emitted ``n`` for the dash-flag ``-n``. The +rejection itself is parity-correct (the original's ``z.strictObject`` refuses +unknown keys too), but the incident exposed two gaps in the same code path: + +* Error text: the original replies with ``formatZodValidationError`` prose + ("Grep failed due to the following issue:\\nAn unexpected parameter `n` was + provided", typescript/src/utils/toolErrors.ts:68) — a materially better + recovery hint than the port's old ``Grep.n: unexpected field``. +* Semantic coercion: the original's schemas wrap boolean/number fields in + ``semanticBoolean``/``semanticNumber`` preprocess steps, so quoted scalars + (``"-n": "true"``, ``"head_limit": "30"``) validate and coerce. The port + validated the raw JSON schema first, hard-rejecting input the original + accepts. +""" + +from __future__ import annotations + +import asyncio +import tempfile +import unittest +from pathlib import Path + +import pytest + +from src.tool_system.build_tool import build_tool +from src.tool_system.context import ToolContext, ToolUseOptions +from src.tool_system.errors import ToolInputError +from src.tool_system.protocol import ToolCall, ToolResult +from src.tool_system.registry import ToolRegistry +from src.tool_system.schema_validation import ( + semantic_coerce, + validate_json_schema, + validate_tool_input, +) +from src.tool_system.tools.grep import GrepTool + + +# --------------------------------------------------------------------------- # +# semantic_coerce — mirrors semanticBoolean.ts / semanticNumber.ts +# --------------------------------------------------------------------------- # + +_BOOL = {"type": "boolean"} +_NUM = {"type": "number"} +_INT = {"type": "integer"} + + +class TestSemanticCoerce(unittest.TestCase): + def test_boolean_literals_coerce(self): + self.assertIs(semantic_coerce("true", _BOOL), True) + self.assertIs(semantic_coerce("false", _BOOL), False) + + def test_boolean_non_literals_pass_through(self): + # semanticBoolean coerces ONLY "true"/"false" — JS-truthiness would + # turn "false" into True, and "1"/"yes"/"TRUE" are rejected upstream. + for v in ("TRUE", "False", "1", "0", "yes", "no", ""): + self.assertIs(semantic_coerce(v, _BOOL), v) + + def test_number_literals_coerce(self): + self.assertEqual(semantic_coerce("30", _NUM), 30) + self.assertIsInstance(semantic_coerce("30", _NUM), int) + self.assertEqual(semantic_coerce("3.14", _NUM), 3.14) + self.assertEqual(semantic_coerce("-5", _NUM), -5) + self.assertEqual(semantic_coerce("007", _NUM), 7) + self.assertEqual(semantic_coerce("5", _INT), 5) + + def test_number_non_literals_pass_through(self): + # /^-?\d+(\.\d+)?$/ — scientific notation, bare dots, whitespace and + # garbage all fall through to the type check, exactly like the + # original's regex gate. + for v in ("1e5", "abc", ".5", "1.", "30 ", " 30", "+5", "NaN", "Infinity", ""): + self.assertIs(semantic_coerce(v, _NUM), v) + + def test_number_overflowing_js_range_passes_through(self): + # Number("9" * 400) is Infinity in JS, so semanticNumber's + # Number.isFinite gate refuses it — mirror that rather than minting + # a Python bigint the original would have rejected. + huge = "9" * 400 + self.assertIs(semantic_coerce(huge, _NUM), huge) + + def test_non_boolean_number_types_untouched(self): + self.assertIs(semantic_coerce("true", {"type": "string"}), "true") + self.assertEqual(semantic_coerce(True, _BOOL), True) + self.assertEqual(semantic_coerce(30, _NUM), 30) + + def test_union_schemas_never_coerce(self): + # ConfigTool.value is z.union([string, boolean, number]) with no + # semantic wrapper — "true" must stay a string there. + union = {"anyOf": [{"type": "string"}, {"type": "boolean"}]} + self.assertIs(semantic_coerce("true", union), "true") + one_of = {"oneOf": [{"type": "boolean"}, {"type": "number"}]} + self.assertIs(semantic_coerce("30", one_of), "30") + + def test_object_properties_coerce_copy_on_write(self): + schema = { + "type": "object", + "properties": {"flag": _BOOL, "limit": _NUM, "name": {"type": "string"}}, + } + original = {"flag": "true", "limit": "30", "name": "x"} + coerced = semantic_coerce(original, schema) + self.assertEqual(coerced, {"flag": True, "limit": 30, "name": "x"}) + # The input is never mutated — the assistant message's recorded + # tool_use block may share this dict. + self.assertEqual(original, {"flag": "true", "limit": "30", "name": "x"}) + self.assertIsNot(coerced, original) + + def test_object_without_coercions_returns_same_object(self): + schema = {"type": "object", "properties": {"flag": _BOOL}} + original = {"flag": True} + self.assertIs(semantic_coerce(original, schema), original) + + def test_unknown_keys_left_alone(self): + schema = {"type": "object", "properties": {"flag": _BOOL}, "additionalProperties": False} + original = {"stray": "true"} + self.assertIs(semantic_coerce(original, schema), original) + + def test_nested_array_items_coerce(self): + schema = { + "type": "object", + "properties": { + "todos": { + "type": "array", + "items": {"type": "object", "properties": {"done": _BOOL}}, + }, + }, + } + original = {"todos": [{"done": "true"}, {"done": False}]} + coerced = semantic_coerce(original, schema) + self.assertEqual(coerced, {"todos": [{"done": True}, {"done": False}]}) + self.assertEqual(original["todos"][0]["done"], "true") + + +# --------------------------------------------------------------------------- # +# validate_tool_input — message parity with formatZodValidationError +# --------------------------------------------------------------------------- # + +class TestValidateToolInputMessages(unittest.TestCase): + def _error(self, tool_input: dict) -> str: + with pytest.raises(ToolInputError) as exc_info: + validate_tool_input("Grep", tool_input, GrepTool.input_schema) + return str(exc_info.value) + + def test_unexpected_parameter_exact_live_repro(self): + # The live failure: the model sent ``n`` instead of ``-n``. Original + # reply for the same input, byte for byte. + msg = self._error({"pattern": "TODO|FIXME", "n": True}) + self.assertEqual( + msg, + "Grep failed due to the following issue:\n" + "An unexpected parameter `n` was provided", + ) + + def test_missing_required_parameter(self): + msg = self._error({}) + self.assertEqual( + msg, + "Grep failed due to the following issue:\n" + "The required parameter `pattern` is missing", + ) + + def test_type_mismatch_uses_js_type_names(self): + # Python's int must render as JS "number". + msg = self._error({"pattern": 5}) + self.assertEqual( + msg, + "Grep failed due to the following issue:\n" + "The parameter `pattern` type is expected as `string` but provided as `number`", + ) + + def test_multiple_issues_grouped_and_plural(self): + # Order mirrors the original: missing, then unexpected, then type + # mismatches — regardless of input key order. + msg = self._error({"-i": 3, "n": True}) + self.assertEqual( + msg, + "Grep failed due to the following issues:\n" + "The required parameter `pattern` is missing\n" + "An unexpected parameter `n` was provided\n" + "The parameter `-i` type is expected as `boolean` but provided as `number`", + ) + + def test_uncategorized_issue_falls_back_to_generic_rendering(self): + # Enum violations have no curated category (the original dumps zod's + # raw error.message there); the port keeps its readable fallback. + msg = self._error({"pattern": "x", "output_mode": "bogus"}) + self.assertIn("Grep.output_mode: expected one of", msg) + self.assertNotIn("failed due to the following", msg) + + def test_semantic_strings_validate_and_coerce(self): + # The sibling failure mode of the live repro — quoted scalars — must + # validate exactly as the original's semantic wrappers allow. + out = validate_tool_input( + "Grep", + {"pattern": "x", "-n": "true", "-i": "false", "head_limit": "30", "-B": "2"}, + GrepTool.input_schema, + ) + self.assertEqual( + out, {"pattern": "x", "-n": True, "-i": False, "head_limit": 30, "-B": 2}, + ) + + def test_valid_input_returned_unchanged_same_object(self): + tool_input = {"pattern": "x", "output_mode": "content"} + self.assertIs( + validate_tool_input("Grep", tool_input, GrepTool.input_schema), tool_input, + ) + + +# --------------------------------------------------------------------------- # +# validate_json_schema — generic callers keep the old rendering +# --------------------------------------------------------------------------- # + +class TestGenericValidationUnchanged(unittest.TestCase): + def test_unexpected_field_old_format(self): + schema = {"type": "object", "additionalProperties": False, "properties": {}} + with pytest.raises(ToolInputError) as exc_info: + validate_json_schema({"n": 1}, schema, root_name="Grep") + self.assertEqual(str(exc_info.value), "Grep.n: unexpected field") + + def test_missing_required_old_format(self): + schema = {"type": "object", "required": ["pattern"], "properties": {}} + with pytest.raises(ToolInputError) as exc_info: + validate_json_schema({}, schema, root_name="output") + self.assertEqual(str(exc_info.value), "output: missing required field 'pattern'") + + def test_no_semantic_coercion_on_generic_path(self): + # Structured-output validation (workflow/structured.py) must stay + # strict — only the tool-dispatch entrypoint coerces. + schema = {"type": "object", "properties": {"flag": {"type": "boolean"}}} + with pytest.raises(ToolInputError): + validate_json_schema({"flag": "true"}, schema) + + +# --------------------------------------------------------------------------- # +# Dispatch integration — coerced input is what the pipeline carries forward +# --------------------------------------------------------------------------- # + +def _capture_tool(received: dict): + return build_tool( + name="Stub", + description="stub", + input_schema={ + "type": "object", + "additionalProperties": False, + "properties": {"flag": {"type": "boolean"}, "limit": {"type": "number"}}, + }, + call=lambda tool_input, context: ( + received.update(tool_input) or ToolResult(name="Stub", output={"ok": True}) + ), + ) + + +class TestRegistryDispatchCoercion(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.received: dict = {} + self.registry = ToolRegistry([_capture_tool(self.received)]) + self.ctx = ToolContext(workspace_root=Path(self.tmp.name).resolve()) + self.ctx.permission_context.mode = "bypassPermissions" + + def tearDown(self): + self.tmp.cleanup() + + def test_call_receives_coerced_input_original_unmutated(self): + original = {"flag": "true", "limit": "30"} + result = self.registry.dispatch( + ToolCall(name="Stub", input=original, tool_use_id="t1"), self.ctx, + ) + self.assertFalse(result.is_error) + self.assertEqual(self.received, {"flag": True, "limit": 30}) + self.assertEqual(original, {"flag": "true", "limit": "30"}) + + def test_unknown_key_raises_ts_format_message(self): + with pytest.raises(ToolInputError) as exc_info: + self.registry.dispatch( + ToolCall(name="Stub", input={"n": True}, tool_use_id="t2"), self.ctx, + ) + self.assertEqual( + str(exc_info.value), + "Stub failed due to the following issue:\n" + "An unexpected parameter `n` was provided", + ) + + +class _ToolUse: + def __init__(self, name: str, input_: dict, id_: str = "toolu_val_1"): + self.name = name + self.input = input_ + self.id = id_ + + +class TestRunToolUseValidation(unittest.TestCase): + """The services lane (live agent loop) — the path the incident hit.""" + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.workspace = Path(self.tmp.name) + self.received: dict = {} + self.tool = _capture_tool(self.received) + + def tearDown(self): + self.tmp.cleanup() + + def _execute(self, tool_input: dict) -> list[dict]: + from src.services.tool_execution.tool_execution import run_tool_use + from src.types.messages import AssistantMessage + + ctx = ToolContext( + workspace_root=self.workspace, + options=ToolUseOptions(tools=[self.tool]), + ) + ctx.permission_context.mode = "bypassPermissions" + + async def drive(): + updates = [] + async for update in run_tool_use( + _ToolUse("Stub", tool_input), + AssistantMessage(content="using a tool"), + lambda *_a, **_k: {"behavior": "allow"}, + ctx, + ): + updates.append(update) + return updates + + updates = asyncio.run(drive()) + blocks = [] + for u in updates: + msg = getattr(u, "message", u) + content = getattr(msg, "content", None) + if isinstance(content, list): + for block in content: + if isinstance(block, dict) and block.get("type") == "tool_result": + blocks.append(block) + elif hasattr(block, "tool_use_id") and hasattr(block, "content"): + # Success results arrive as ToolResultBlock dataclass + # instances — normalize like test_tool_pipeline_round3. + blocks.append({ + "type": "tool_result", + "tool_use_id": block.tool_use_id, + "content": block.content, + "is_error": getattr(block, "is_error", False), + }) + return blocks + + def test_validation_error_wrapped_with_ts_format_text(self): + blocks = self._execute({"n": True}) + self.assertEqual(len(blocks), 1) + self.assertTrue(blocks[0]["is_error"]) + self.assertEqual( + blocks[0]["content"], + "InputValidationError: " + "Stub failed due to the following issue:\n" + "An unexpected parameter `n` was provided", + ) + + def test_coerced_input_reaches_call(self): + blocks = self._execute({"flag": "true", "limit": "30"}) + self.assertEqual(len(blocks), 1) + self.assertFalse(blocks[0].get("is_error", False)) + self.assertEqual(self.received, {"flag": True, "limit": 30}) + + +if __name__ == "__main__": + unittest.main() From 802c2207de3acc38aa10267b0a6b1cf7c5813fd1 Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Sun, 12 Jul 2026 12:38:45 -0700 Subject: [PATCH 2/3] fix(send-message): approve flag honors semanticBoolean literals inside the union MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TS wraps approve in semanticBoolean() inside the message union branches (SendMessageTool.ts:55,61) — the boundary coercion deliberately skips anyOf/oneOf, and the port's runtime bool() JS-truthied approve:"false" into an approval. Co-Authored-By: Claude Fable 5 --- src/tool_system/tools/send_message.py | 19 +++++++++++++++++-- tests/test_tool_input_validation.py | 15 +++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/tool_system/tools/send_message.py b/src/tool_system/tools/send_message.py index 06472361b..5bee5690c 100644 --- a/src/tool_system/tools/send_message.py +++ b/src/tool_system/tools/send_message.py @@ -320,6 +320,21 @@ def _broadcast( ) +def _approve_flag(message_obj: dict[str, Any]) -> bool: + """Read ``approve`` with the original's semanticBoolean tolerance. + + The original wraps ``approve`` in ``semanticBoolean()`` inside the union + branches (SendMessageTool.ts:55,61), so a quoted ``"false"`` is False — + a bare ``bool()`` would JS-truthy it into True. The port validates union + internals at runtime rather than schema level, so the coercion lives + here; other junk strings stay truthy-loose (documented divergence — the + original's z.boolean() rejects them outright). + """ + from ..schema_validation import semantic_coerce + + return bool(semantic_coerce(message_obj.get("approve"), {"type": "boolean"})) + + def _structured_message_to_envelope( *, message_obj: dict[str, Any], @@ -344,7 +359,7 @@ def _structured_message_to_envelope( "", # caller passes the original ``to`` ) if msg_type == "shutdown_response": - approve = bool(message_obj.get("approve")) + approve = _approve_flag(message_obj) if approve: return ( create_shutdown_approved_message( @@ -371,7 +386,7 @@ def _structured_message_to_envelope( raise ToolInputError( "plan_approval_response can only be sent by the team lead." ) - approve = bool(message_obj.get("approve")) + approve = _approve_flag(message_obj) permission_mode = str( message_obj.get("permission_mode") or "default" ) diff --git a/tests/test_tool_input_validation.py b/tests/test_tool_input_validation.py index ba8aee6f0..880e5750e 100644 --- a/tests/test_tool_input_validation.py +++ b/tests/test_tool_input_validation.py @@ -361,5 +361,20 @@ def test_coerced_input_reaches_call(self): self.assertEqual(self.received, {"flag": True, "limit": 30}) +class TestSendMessageApproveFlag(unittest.TestCase): + """``approve`` sits INSIDE the SendMessage union (semanticBoolean at + SendMessageTool.ts:55,61), which the boundary coercion deliberately + skips — the tool reads it with the same tolerance at runtime.""" + + def test_quoted_literals_and_bools(self): + from src.tool_system.tools.send_message import _approve_flag + + self.assertIs(_approve_flag({"approve": "false"}), False) + self.assertIs(_approve_flag({"approve": "true"}), True) + self.assertIs(_approve_flag({"approve": True}), True) + self.assertIs(_approve_flag({"approve": False}), False) + self.assertIs(_approve_flag({}), False) + + if __name__ == "__main__": unittest.main() From d66f69af9ca935ffe1e57b77d70c030d10bb8406 Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Sun, 12 Jul 2026 12:55:42 -0700 Subject: [PATCH 3/3] fix(send-message): reject non-boolean approve like z.boolean (critic R1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit approve is a required semanticBoolean inside the union — junk strings/ numbers/missing must reject the message, not JS-truthy into approving a shutdown or plan. Also: ASCII-gate the number-literal regex (JS \d) and refresh two stale 'model-original input' comments. Co-Authored-By: Claude Fable 5 --- src/services/tool_execution/tool_execution.py | 21 ++++++++++--------- src/tool_system/schema_validation.py | 4 +++- src/tool_system/tools/send_message.py | 20 +++++++++++------- tests/test_tool_input_validation.py | 19 +++++++++++++++-- 4 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/services/tool_execution/tool_execution.py b/src/services/tool_execution/tool_execution.py index aef042797..a0ff7064b 100644 --- a/src/services/tool_execution/tool_execution.py +++ b/src/services/tool_execution/tool_execution.py @@ -232,11 +232,12 @@ async def _check_permissions_and_call_tool( logger.debug("Validation error for %s: %s", tool.name, e) # ----- Step 6 — Input Backfill (clone, not mutate). - # call() receives the MODEL-ORIGINAL input: tool results embed input + # call() receives the PRE-BACKFILL input (post schema-coercion — TS's + # callInput is likewise parsedInput.data): tool results embed input # fields verbatim (e.g. "File created successfully at: {path}"), and - # changing them alters the serialized transcript. The cloned, - # backfilled input is the hooks/permissions audience only. - # Mirrors typescript/src/services/tools/toolExecution.ts:838-853. + # backfill mutations (path expansion) would alter the serialized + # transcript. The cloned, backfilled input is the hooks/permissions + # audience only. Mirrors typescript/src/services/tools/toolExecution.ts:838-853. call_input = processed_input backfilled_clone: dict[str, Any] | None = None if tool.backfill_observable_input is not None: @@ -341,12 +342,12 @@ async def _check_permissions_and_call_tool( # If processed_input still points at the backfill clone, no # hook/permission replaced it — pass the pre-backfill call_input so - # call() sees the model's original field values. Hook/permission - # flows may return a fresh object derived from the backfilled clone - # (e.g. via schema re-parse): if its file_path matches the - # backfill-expanded value, restore the model's original so the tool - # result string embeds the path the model emitted. Other - # modifications flow through unchanged. Mirrors + # call() sees the pre-expansion field values (post schema-coercion). + # Hook/permission flows may return a fresh object derived from the + # backfilled clone (e.g. via schema re-parse): if its file_path + # matches the backfill-expanded value, restore the pre-backfill one + # so the tool result string embeds the path the model emitted. + # Other modifications flow through unchanged. Mirrors # typescript/src/services/tools/toolExecution.ts:1212-1237. if ( backfilled_clone is not None diff --git a/src/tool_system/schema_validation.py b/src/tool_system/schema_validation.py index 2b7726a65..0ebad4317 100644 --- a/src/tool_system/schema_validation.py +++ b/src/tool_system/schema_validation.py @@ -108,7 +108,9 @@ def validate_json_schema(value: Any, schema: Mapping[str, Any], *, root_name: st # coerce, mirroring the original's unwrapped unions (ConfigTool.value must # keep "true" a string). -_NUMBER_LITERAL_RE = re.compile(r"-?\d+(\.\d+)?") +# re.ASCII: JS \d is ASCII-only — Python's Unicode \d would coerce +# non-ASCII digits ("٣٠") the original's regex refuses. +_NUMBER_LITERAL_RE = re.compile(r"-?\d+(\.\d+)?", re.ASCII) def semantic_coerce(value: Any, schema: Mapping[str, Any]) -> Any: diff --git a/src/tool_system/tools/send_message.py b/src/tool_system/tools/send_message.py index 5bee5690c..f01042974 100644 --- a/src/tool_system/tools/send_message.py +++ b/src/tool_system/tools/send_message.py @@ -321,18 +321,24 @@ def _broadcast( def _approve_flag(message_obj: dict[str, Any]) -> bool: - """Read ``approve`` with the original's semanticBoolean tolerance. + """Read ``approve`` with the original's semanticBoolean→z.boolean strictness. The original wraps ``approve`` in ``semanticBoolean()`` inside the union - branches (SendMessageTool.ts:55,61), so a quoted ``"false"`` is False — - a bare ``bool()`` would JS-truthy it into True. The port validates union - internals at runtime rather than schema level, so the coercion lives - here; other junk strings stay truthy-loose (documented divergence — the - original's z.boolean() rejects them outright). + branches (SendMessageTool.ts:55,61): the exact literals "true"/"false" + coerce, and anything else — junk strings, numbers, a missing field — + fails z.boolean(), rejecting the whole message. The port's loose oneOf + (no ``properties``) never type-checks ``approve``, so the strictness + must live here: shutdown/plan approvals are control-plane messages, and + a truthy junk value ("no", "0", 1) must not read as an approval. """ from ..schema_validation import semantic_coerce - return bool(semantic_coerce(message_obj.get("approve"), {"type": "boolean"})) + coerced = semantic_coerce(message_obj.get("approve"), {"type": "boolean"}) + if not isinstance(coerced, bool): + raise ToolInputError( + 'approve must be a boolean (or the string literal "true"/"false").' + ) + return coerced def _structured_message_to_envelope( diff --git a/tests/test_tool_input_validation.py b/tests/test_tool_input_validation.py index 880e5750e..e41243a24 100644 --- a/tests/test_tool_input_validation.py +++ b/tests/test_tool_input_validation.py @@ -73,6 +73,10 @@ def test_number_non_literals_pass_through(self): for v in ("1e5", "abc", ".5", "1.", "30 ", " 30", "+5", "NaN", "Infinity", ""): self.assertIs(semantic_coerce(v, _NUM), v) + def test_non_ascii_digits_pass_through(self): + # JS \d is ASCII-only; Python's Unicode \d must not widen the gate. + self.assertIs(semantic_coerce("٣٠", _NUM), "٣٠") + def test_number_overflowing_js_range_passes_through(self): # Number("9" * 400) is Infinity in JS, so semanticNumber's # Number.isFinite gate refuses it — mirror that rather than minting @@ -364,7 +368,8 @@ def test_coerced_input_reaches_call(self): class TestSendMessageApproveFlag(unittest.TestCase): """``approve`` sits INSIDE the SendMessage union (semanticBoolean at SendMessageTool.ts:55,61), which the boundary coercion deliberately - skips — the tool reads it with the same tolerance at runtime.""" + skips — the tool reads it with the same semantics at runtime: + "true"/"false" coerce, everything else fails like z.boolean().""" def test_quoted_literals_and_bools(self): from src.tool_system.tools.send_message import _approve_flag @@ -373,7 +378,17 @@ def test_quoted_literals_and_bools(self): self.assertIs(_approve_flag({"approve": "true"}), True) self.assertIs(_approve_flag({"approve": True}), True) self.assertIs(_approve_flag({"approve": False}), False) - self.assertIs(_approve_flag({}), False) + + def test_junk_and_missing_reject_like_z_boolean(self): + # A truthy junk value must NOT read as approval of a control-plane + # message ("no" would otherwise APPROVE a shutdown). + from src.tool_system.tools.send_message import _approve_flag + + for junk in ("no", "off", "0", "1", "FALSE", "False", " false", "n", 1, 0, None): + with self.assertRaises(ToolInputError): + _approve_flag({"approve": junk}) + with self.assertRaises(ToolInputError): + _approve_flag({}) if __name__ == "__main__":