diff --git a/model_tools.py b/model_tools.py index e07f6da68fe..c3c3a11a66f 100644 --- a/model_tools.py +++ b/model_tools.py @@ -718,16 +718,128 @@ def coerce_tool_args(tool_name: str, args: Dict[str, Any]) -> Dict[str, Any]: continue if not isinstance(value, str): + # Recurse into already-native containers so JSON-encoded + # *elements* (array items) and *sub-fields* (nested object + # properties) get normalized too — e.g. ``todos: ['{"id":...}']`` + # or ``tasks: [{"goal": "..."}]`` where an element was emitted as + # a JSON string. The top-level coercion above only repairs the + # outermost value. + if expected == "array" and isinstance(value, (list, tuple)): + args[key] = _normalize_json_strings_for_schema(value, prop_schema) + elif expected == "object" and isinstance(value, dict): + args[key] = _normalize_json_strings_for_schema(value, prop_schema) continue if not expected and not _schema_allows_null(prop_schema): continue coerced = _coerce_value(value, expected, schema=prop_schema) if coerced is not value: args[key] = coerced + # If we just JSON-parsed a string into a container, recurse so + # nested JSON-encoded elements/fields get normalized as well. + if isinstance(coerced, (list, tuple, dict)): + args[key] = _normalize_json_strings_for_schema(coerced, prop_schema) return args +def _schema_accepts_kind(schema: Any, kind: str) -> bool: + """Return True when *schema* permits a value of JSON type *kind*. + + Looks at ``type`` (string or list) and recurses through + ``anyOf``/``oneOf``/``allOf`` branches — matching the JSON-Schema shapes + open-weight models emit against. ``kind`` is ``"array"`` or ``"object"``. + """ + if not isinstance(schema, dict): + return False + t = schema.get("type") + if t == kind or (isinstance(t, list) and kind in t): + return True + for union_key in ("anyOf", "oneOf", "allOf"): + branches = schema.get(union_key) + if isinstance(branches, list) and any( + _schema_accepts_kind(b, kind) for b in branches + ): + return True + return False + + +def _normalize_json_strings_for_schema(value: Any, schema: Any) -> Any: + """Recursively parse JSON-encoded string values that a schema expects to + be arrays or objects, including nested array items and object properties. + + Open-weight models (DeepSeek, Qwen, GLM, and others) sometimes emit a + structured field — or an *element* of a structured field — as a + JSON-encoded string instead of a native value. The top-level + :func:`coerce_tool_args` pass repairs the outermost value; this helper + walks the rest of the tree so cases like:: + + {"todos": ["{\\"id\\": \\"1\\", \\"content\\": \\"x\\"}"]} + + (a list whose elements are JSON strings) and nested object sub-fields are + repaired too. Parsing is schema-guided: a string is only parsed when the + matching schema position actually expects an array or object, so + legitimate JSON-looking string fields (``type: string``) are preserved. + + Ported from cline/cline#11803, adapted to hermes-agent's coercion layer. + Returns the original value object when nothing changed (identity preserved + so callers can cheaply detect no-ops). + """ + if not isinstance(schema, dict): + return value + + # Parse a JSON-encoded string into the container the schema expects. + if isinstance(value, str): + trimmed = value.strip() + expects_array = _schema_accepts_kind(schema, "array") + expects_object = _schema_accepts_kind(schema, "object") + if (expects_array and trimmed.startswith("[")) or ( + expects_object and trimmed.startswith("{") + ): + try: + parsed = json.loads(trimmed) + except (ValueError, TypeError): + return value + if isinstance(parsed, list) and expects_array: + value = parsed + elif isinstance(parsed, dict) and expects_object: + value = parsed + else: + return value + else: + return value + + # Recurse into list items using the ``items`` schema. + if isinstance(value, list): + items_schema = schema.get("items") + if not isinstance(items_schema, dict): + return value + changed = False + out = [] + for item in value: + nxt = _normalize_json_strings_for_schema(item, items_schema) + changed = changed or (nxt is not item) + out.append(nxt) + return out if changed else value + + # Recurse into object properties using each property's schema. + if isinstance(value, dict): + props = schema.get("properties") + if not isinstance(props, dict): + return value + changed = False + out = dict(value) + for k, prop_schema in props.items(): + if k not in value or not isinstance(prop_schema, dict): + continue + nxt = _normalize_json_strings_for_schema(value[k], prop_schema) + if nxt is not value[k]: + out[k] = nxt + changed = True + return out if changed else value + + return value + + def _coerce_value(value: str, expected_type, schema: dict | None = None): """Attempt to coerce a string *value* to *expected_type*. diff --git a/tests/run_agent/test_tool_arg_coercion.py b/tests/run_agent/test_tool_arg_coercion.py index e5bbdd93d80..5b80fea4065 100644 --- a/tests/run_agent/test_tool_arg_coercion.py +++ b/tests/run_agent/test_tool_arg_coercion.py @@ -13,6 +13,8 @@ from model_tools import ( _coerce_value, _coerce_number, _coerce_boolean, + _schema_accepts_kind, + _normalize_json_strings_for_schema, ) @@ -408,3 +410,138 @@ class TestCoerceToolArgs: assert isinstance(result["offset"], int) assert result["limit"] == 100 assert isinstance(result["limit"], int) + + +# ── Schema-guided nested JSON-string normalization (cline/cline#11803) ───── + + +class TestSchemaAcceptsKind: + """Unit tests for _schema_accepts_kind.""" + + def test_plain_type(self): + assert _schema_accepts_kind({"type": "array"}, "array") is True + assert _schema_accepts_kind({"type": "object"}, "object") is True + assert _schema_accepts_kind({"type": "string"}, "array") is False + + def test_type_list(self): + assert _schema_accepts_kind({"type": ["array", "null"]}, "array") is True + assert _schema_accepts_kind({"type": ["string", "null"]}, "array") is False + + def test_union_branches(self): + schema = {"anyOf": [{"type": "string"}, {"type": "array"}]} + assert _schema_accepts_kind(schema, "array") is True + assert _schema_accepts_kind(schema, "object") is False + + def test_non_dict(self): + assert _schema_accepts_kind(None, "array") is False + + +class TestNormalizeJsonStringsForSchema: + """Unit tests for _normalize_json_strings_for_schema (the recursive pass).""" + + def test_parses_json_string_array_when_schema_expects_array(self): + schema = {"type": "array", "items": {"type": "string"}} + out = _normalize_json_strings_for_schema('["git status", "bun test"]', schema) + assert out == ["git status", "bun test"] + + def test_preserves_json_looking_string_when_schema_expects_string(self): + schema = {"type": "string"} + text = '{"keep": "as text"}' + assert _normalize_json_strings_for_schema(text, schema) == text + + def test_normalizes_array_item_json_strings(self): + schema = { + "type": "array", + "items": {"type": "object", "properties": {"id": {"type": "string"}}}, + } + out = _normalize_json_strings_for_schema(['{"id": "1"}', '{"id": "2"}'], schema) + assert out == [{"id": "1"}, {"id": "2"}] + + def test_normalizes_nested_object_field(self): + schema = { + "type": "object", + "properties": {"cfg": {"type": "object", "properties": {"k": {"type": "string"}}}}, + } + out = _normalize_json_strings_for_schema({"cfg": '{"k": "v"}'}, schema) + assert out == {"cfg": {"k": "v"}} + + def test_native_list_preserved_identity(self): + schema = {"type": "array", "items": {"type": "object", "properties": {}}} + value = [{"id": "1"}] + # Nothing to change — same object back (no-op identity preserved). + assert _normalize_json_strings_for_schema(value, schema) is value + + def test_non_dict_schema_returns_value(self): + assert _normalize_json_strings_for_schema("x", None) == "x" + + +class TestCoerceToolArgsNested: + """Integration: nested JSON-string elements/fields are normalized via the + registry schema, while legitimate string fields are preserved.""" + + def _array_of_objects_schema(self): + return { + "name": "test_tool", + "description": "test", + "parameters": { + "type": "object", + "properties": { + "items": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": {"type": "string"}, + "content": {"type": "string"}, + }, + }, + }, + }, + }, + } + + def test_array_elements_as_json_strings_are_parsed(self): + schema = self._array_of_objects_schema() + with patch("model_tools.registry.get_schema", return_value=schema): + args = {"items": ['{"id": "1", "content": "x"}']} + result = coerce_tool_args("test_tool", args) + assert result["items"] == [{"id": "1", "content": "x"}] + + def test_mixed_native_and_string_elements(self): + schema = self._array_of_objects_schema() + with patch("model_tools.registry.get_schema", return_value=schema): + args = {"items": [{"id": "1", "content": "a"}, '{"id": "2", "content": "b"}']} + result = coerce_tool_args("test_tool", args) + assert result["items"] == [ + {"id": "1", "content": "a"}, + {"id": "2", "content": "b"}, + ] + + def test_string_subfield_with_json_content_preserved(self): + """A string-typed sub-field whose value looks like JSON must NOT be parsed.""" + schema = self._array_of_objects_schema() + with patch("model_tools.registry.get_schema", return_value=schema): + args = {"items": [{"id": "1", "content": '{"not": "parsed"}'}]} + result = coerce_tool_args("test_tool", args) + assert result["items"][0]["content"] == '{"not": "parsed"}' + + def test_whole_array_string_still_works(self): + schema = self._array_of_objects_schema() + with patch("model_tools.registry.get_schema", return_value=schema): + args = {"items": '[{"id": "1", "content": "x"}]'} + result = coerce_tool_args("test_tool", args) + assert result["items"] == [{"id": "1", "content": "x"}] + + def test_native_array_preserved(self): + schema = self._array_of_objects_schema() + with patch("model_tools.registry.get_schema", return_value=schema): + args = {"items": [{"id": "1", "content": "keep"}]} + result = coerce_tool_args("test_tool", args) + assert result["items"] == [{"id": "1", "content": "keep"}] + + def test_real_todo_schema_element_strings(self): + """Against the real todo schema from the registry.""" + import json as _json + args = {"todos": [_json.dumps({"id": "1", "content": "x", "status": "pending"})]} + result = coerce_tool_args("todo", args) + assert result["todos"][0] == {"id": "1", "content": "x", "status": "pending"}