From ebcc8d48caee61284abdf504b16cda761dab88ac Mon Sep 17 00:00:00 2001 From: zhanghanduo Date: Wed, 2 Sep 2026 17:44:16 +0800 Subject: [PATCH] fix(providers): keep protocol types narrow and unwrap PEP 695 aliases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two type-erasure defects found while a strict downstream host consumed the provider substrate. Both are silent: the annotation reads correctly at the call site and the wrong type only shows up further downstream. `protocol_of` and `thinking_format_for_protocol` were extracted with `-> str` and `-> str | None`, widening what the host they came from declared. Their outputs feed `ModelProfile.protocol` / `.thinking_format`, which are `Literal` unions, so a value this module had just validated with `is_wire_protocol` was no longer assignable to the very field it was validated for. This file is checked at `basic` level, so nothing here caught it; a strict host did, with four errors in code it had not touched. `_schema_for_type` mapped a PEP 695 `type Alias = ...` straight to `{"type": "string"}`: a TypeAliasType is not a primitive, has no origin, and is not a Pydantic model, so it reached the `Any` floor. That floor is a deliberate choice for under-annotated parameters — an alias is the opposite, and a tool author who names a union to describe a parameter precisely got a schema claiming it was a string. Unwrap `__value__` instead. `agent_core` uses `type` statements itself (pause_check, openai_chat), so this was live. Validation: - uv run ruff check agent_core tests - uv run pyright agent_core (0 errors) - uv run pytest -q (1054 passed) - both new alias tests fail with the unwrap removed Co-Authored-By: Claude Opus 5 (1M context) --- agent_core/providers/protocol_client.py | 19 ++++++++--- agent_core/tool.py | 11 +++++++ tests/test_tool_schema_types.py | 42 +++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 4 deletions(-) diff --git a/agent_core/providers/protocol_client.py b/agent_core/providers/protocol_client.py index 0779a24..8fd6271 100644 --- a/agent_core/providers/protocol_client.py +++ b/agent_core/providers/protocol_client.py @@ -27,7 +27,11 @@ # Single source of truth for the valid ``llm.protocol`` values. Duplicating the # set here would let the two drift, which is how a protocol becomes buildable # but has no ``thinking_format`` (or vice versa). -from agent_core.runtime.loop.model_profile import WireProtocol, is_wire_protocol +from agent_core.runtime.loop.model_profile import ( + ThinkingFormat, + WireProtocol, + is_wire_protocol, +) logger = logging.getLogger(__name__) @@ -37,7 +41,7 @@ _MIN_THINKING_BUDGET = 1024 -def protocol_of(cfg: dict[str, Any]) -> str: +def protocol_of(cfg: dict[str, Any]) -> WireProtocol: """Normalised ``llm.protocol`` (default ``chat_completions``). An unusable value — wrong type, or a string that names no known protocol — @@ -84,9 +88,16 @@ def provider_label(cfg: dict[str, Any]) -> str: return "openai" -def thinking_format_for_protocol(protocol: str) -> str | None: +def thinking_format_for_protocol(protocol: str) -> ThinkingFormat | None: """``content_block`` for the reasoning protocols, else ``None`` (caller - falls back to explicit YAML / model-id inference).""" + falls back to explicit YAML / model-id inference). + + Both this and :func:`protocol_of` return the NARROW alias, not a bare + ``str``: their outputs feed ``ModelProfile.thinking_format`` / + ``ModelProfile.protocol``, which are ``Literal`` unions, so widening to + ``str`` makes a value this module just validated unassignable at the very + field it was validated for. This file is checked at ``basic`` level, so + nothing here catches that — a strict host does.""" return ( "content_block" if protocol in ("anthropic", "responses", "bedrock") diff --git a/agent_core/tool.py b/agent_core/tool.py index 580d2a2..64c3a8e 100644 --- a/agent_core/tool.py +++ b/agent_core/tool.py @@ -54,6 +54,17 @@ def to_openai_schema(self) -> dict[str, Any]: def _schema_for_type(t: Any) -> dict[str, Any]: """Build a JSON-schema fragment for a Python type hint.""" + # PEP 695 (``type Alias = ...``) produces a TypeAliasType, which is not a + # primitive, has no ``get_origin``, and is not a Pydantic model — so without + # this it reaches the final fallback and becomes ``{"type": "string"}``. That + # is the WORST failure mode available here: a tool author who names a union + # to describe a parameter precisely gets a schema claiming the parameter is + # a string, silently, with the annotation looking correct in the source. + # This module documents ``Any -> string`` as a deliberate floor for + # under-annotated parameters; an alias is the opposite of under-annotated. + if isinstance(t, typing.TypeAliasType): + return _schema_for_type(t.__value__) + if t in _PRIMITIVE_TYPES: return {"type": _PRIMITIVE_TYPES[t]} diff --git a/tests/test_tool_schema_types.py b/tests/test_tool_schema_types.py index 1785231..1c62671 100644 --- a/tests/test_tool_schema_types.py +++ b/tests/test_tool_schema_types.py @@ -66,3 +66,45 @@ async def probe(items: list[dict[str, Any]]) -> str: return "" assert _properties(probe)["items"]["items"] == {"type": "object"} + + +# ── PEP 695 aliases, the spelling used to name a union ──────────────────────── + + +def test_a_pep695_alias_resolves_to_what_it_aliases() -> None: + """``type Alias = ...`` must not degrade to ``string``. + + A ``TypeAliasType`` is not a primitive, has no origin, and is not a Pydantic + model, so before it was unwrapped it fell through to the ``Any`` floor. The + result was a parameter documented as a union and SERVED as a string, with + nothing in the source or the schema to show the annotation had been dropped. + """ + type JsonValue = dict[str, Any] | list[str] | str | float | bool + + async def fn(value): # annotation injected below + return "" + fn.__annotations__ = {"value": JsonValue, "return": str} + t = tool(fn) + + arms = _properties(t)["value"]["anyOf"] + assert {arm.get("type") for arm in arms} == { + "object", "array", "string", "number", "boolean", + } + + +def test_a_pep695_alias_survives_being_an_items_type() -> None: + """The case that motivated this: ``list[Alias]``, where the alias describes + what one element may be. Losing it here types every element ``string``.""" + type Cell = str | float | bool + + async def fn(rows): # annotation injected below + return "" + fn.__annotations__ = {"rows": list[Cell], "return": str} + t = tool(fn) + + schema = _properties(t)["rows"] + assert schema["type"] == "array" + assert {arm.get("type") for arm in schema["items"]["anyOf"]} == { + "string", "number", "boolean", + } + assert not _walk(schema, "rows")