diff --git a/openspec/changes/openai-compatible-native-tool-return/.openspec.yaml b/openspec/changes/openai-compatible-native-tool-return/.openspec.yaml new file mode 100644 index 000000000..dd9a1d92e --- /dev/null +++ b/openspec/changes/openai-compatible-native-tool-return/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-06 diff --git a/openspec/changes/openai-compatible-native-tool-return/design.md b/openspec/changes/openai-compatible-native-tool-return/design.md new file mode 100644 index 000000000..0f1ac30fd --- /dev/null +++ b/openspec/changes/openai-compatible-native-tool-return/design.md @@ -0,0 +1,315 @@ +# Design: OpenAI-Compatible Native Tool Return + +## Context + +pydantic-ai's `OpenAIChatModel._map_user_message()` (openai.py:1481) handles `ToolReturnPart` by calling `part.model_response_str_and_user_content()`, which returns `tuple[str, list[UserContent]]`. The string is always a JSON-serialized representation of the tool return content. This works for official OpenAI models but is suboptimal for OpenAI-compatible models (GLM-5, vLLM) whose chat templates natively handle list-type content. + +### Current Flow + +``` +Tool returns ["result1", "result2"] + → ToolReturnPart(content=["result1", "result2"]) + → _map_user_message() calls model_response_str_and_user_content() + → model_response_str() calls tool_return_ta.dump_json(["result1", "result2"]).decode() + → content = '["result1", "result2"]' (JSON string) + → ChatCompletionToolMessageParam(role='tool', content='["result1", "result2"]') +``` + +### Desired Flow (with flag enabled) + +``` +Tool returns ["result1", "result2"] + → ToolReturnPart(content=["result1", "result2"]) + → _map_user_message() overridden: detect list content, no files + → content = [{"type": "text", "text": "result1"}, {"type": "text", "text": "result2"}] + → ChatCompletionToolMessageParam(role='tool', content=[...]) +``` + +## Design Decisions + +### Decision 1: Subclass `OpenAIChatModel` (not monkey-patch or fork) + +**Choice:** Create `OpenAICompatibleModel(OpenAIChatModel)` in agentpool. + +**Rationale:** +- `OpenAIChatModel` already has 3 subclasses (`OllamaModel`, `OpenRouterModel`, `CerebrasModel`) — subclassing is the established pattern. +- `OpenRouterModel` overrides `_map_messages()` with super-call + post-processing, proving internal methods are overridable. +- No pydantic-ai upstream changes required — agentpool controls the subclass. +- Clean separation: the override only affects agents that explicitly use this model class. + +**Rejected alternatives:** +- **Monkey-patch `OpenAIChatModel`** — Global side effects, breaks if pydantic-ai changes internals. +- **Fork pydantic-ai** — Maintenance burden, diverges from upstream. +- **Contribute to pydantic-ai upstream** — Issue #3888 is open but not merged; can't wait for upstream. + +### Decision 2: Override `_map_user_message()` with full method duplication + +**Choice:** Override `_map_user_message()` entirely. When the profile flag is `False` (default), delegate entirely to `super()._map_user_message(message)`. When the flag is `True`, duplicate the parent's method body, replacing only the `ToolReturnPart` branch. + +**Rationale:** +- `_map_user_message()` (line 1481) takes a `ModelRequest` and iterates over `message.parts`, yielding `ChatCompletionMessageParam` items. It handles `SystemPromptPart`, `UserPromptPart`, `ToolReturnPart`, and `RetryPromptPart` in a single loop, and also accumulates `file_content` (a `list[UserContent]`) that is yielded as a final `UserPromptPart` when non-empty. +- Because the method processes all parts in a single loop and maintains state (`file_content`) across iterations, it is not possible to delegate individual parts to `super()` — calling `super()._map_user_message(message)` would process ALL parts including `ToolReturnPart`. +- Therefore, when the flag is `True`, the entire method body must be duplicated with only the `ToolReturnPart` branch modified. When the flag is `False`, a simple `super()` delegation avoids any duplication. + +**Implementation approach:** +```python +async def _map_user_message(self, message: ModelRequest) -> AsyncIterator[ChatCompletionMessageParam]: + if not self._resolved_profile.get('openai_chat_tool_return_as_list', False): + # Flag disabled: delegate entirely to parent + async for item in super()._map_user_message(message): + yield item + return + + # Flag enabled: duplicate parent logic, replacing ToolReturnPart branch + file_content: list[UserContent] = [] + for part in message.parts: + if isinstance(part, SystemPromptPart): + # ... same as parent + elif isinstance(part, UserPromptPart): + # ... same as parent + elif isinstance(part, ToolReturnPart): + if isinstance(part.content, list) and not part.files and part.content: + # NEW: construct list[ChatCompletionContentPartTextParam] + content_parts = [] + for item in part.content_items(mode='str'): + if isinstance(item, str): + content_parts.append(ChatCompletionContentPartTextParam(type='text', text=item)) + yield chat.ChatCompletionToolMessageParam( + role='tool', + tool_call_id=_guard_tool_call_id(t=part), + content=content_parts, + ) + else: + # String content, empty list, or files: use parent behavior + tool_text, tool_file_content = part.model_response_str_and_user_content() + file_content.extend(tool_file_content) + yield chat.ChatCompletionToolMessageParam( + role='tool', + tool_call_id=_guard_tool_call_id(t=part), + content=tool_text, + ) + elif isinstance(part, RetryPromptPart): + # ... same as parent + else: + assert_never(part) + if file_content: + yield await self._map_user_prompt(UserPromptPart(content=file_content)) +``` + +**Key details preserved from parent:** +- `file_content` accumulation across all parts +- Final `UserPromptPart` yield when `file_content` is non-empty +- `_guard_tool_call_id()` for tool call ID extraction +- `assert_never(part)` for exhaustive matching + +This is acceptable because: +1. The parent method is stable (well-established API). +2. We only diverge in one branch (`ToolReturnPart` with non-empty list content, no files). +3. `OpenRouterModel` already overrides at a similar level (`_map_messages()`). +4. When flag is `False`, zero code duplication — pure super delegation. + +### Decision 3: Custom profile TypedDict for type safety + +**Choice:** Define a custom `OpenAICompatibleModelProfile(OpenAIModelProfile)` TypedDict in agentpool that adds the `openai_chat_tool_return_as_list: bool` key. + +**Rationale:** +- `OpenAIModelProfile` in pydantic-ai is a `TypedDict(total=False)`. `total=False` means all *declared* keys are optional — it does NOT mean arbitrary keys are accepted. Static type checkers (pyright strict, mypy strict) will flag access to undeclared keys. +- The original design's claim that "TypedDict(total=False) allows custom keys" was incorrect for type checking. +- Defining a custom TypedDict in agentpool is the type-safe approach: + +```python +from pydantic_ai.profiles.openai import OpenAIModelProfile + +class OpenAICompatibleModelProfile(OpenAIModelProfile, total=False): + """Profile for OpenAI-compatible models with extended flags.""" + openai_chat_tool_return_as_list: bool +``` + +- The model class accesses the profile via a `_resolved_profile` property (following the `OpenRouterModel` pattern at `openrouter.py:706-707`), which casts `self.profile` to `OpenAICompatibleModelProfile`: + +```python +@property +def _resolved_profile(self) -> OpenAICompatibleModelProfile: + return cast(OpenAICompatibleModelProfile, self.profile) +``` + +- This mirrors the parent's own pattern: `OpenAIChatModel.profile` (line 806) uses `cast(OpenAIModelProfile, _profile)`, and `OpenRouterModel._resolved_profile` (line 707) uses `cast(OpenRouterModelProfile, self.profile)`. Using `cast()` here is consistent with the established pydantic-ai pattern for TypedDict profile subclasses. +- At runtime, profile data from YAML (plain dict) is compatible with this TypedDict. + +**Alternatives rejected:** +- **Access via `dict(self.profile).get(...)`** — Works but loses type safety; ugly workaround. +- **Override the `profile` property itself** — The parent's `profile` is a `@cached_property` with complex logic (line 796-806); overriding it risks breaking that logic. A separate `_resolved_profile` property is safer. +- **Modify pydantic-ai's `OpenAIModelProfile` upstream** — Out of scope; requires upstream PR. + +### Decision 4: Manifest configuration via `ImportModelConfig` with `openai_*` kwarg passthrough + +**Choice:** Add explicit constructor parameters (`base_url`, `api_key`, `tool_return_as_list`) plus `**profile_overrides` to capture arbitrary `openai_*` prefixed kwargs from YAML. All `openai_*` keys in `kw_args` are automatically merged into the profile dict with type coercion. + +**Rationale:** +- `ImportModelConfig.kw_args` is typed as `dict[str, str]` (verified in `llmling_models_config`), so nested dicts like `profile: {openai_chat_tool_return_as_list: true}` cannot be passed via YAML. +- The parent `OpenAIChatModel.__init__` (line 739) takes `provider: Provider[AsyncOpenAI]`, not `base_url`/`api_key` directly. Adding these as convenience params on the subclass simplifies YAML configuration. +- The `tool_return_as_list` flag is passed as a string (`"true"`/`"false"`) from YAML and coerced to `bool` in the constructor. +- `OpenAIModelProfile` has 15+ `openai_*` fields (e.g. `openai_system_prompt_role`, `openai_supports_strict_tool_definition`, `openai_chat_supports_web_search`, `openai_chat_thinking_field`, etc.). Exposing each as a separate constructor param would be unwieldy. Instead, `**profile_overrides` captures any `openai_*` kwarg and merges it into the profile with automatic type coercion. +- This follows the `OpenRouterModel` pattern (line 687-703) which adds convenience params, but extends it with flexible profile passthrough. + +**Type coercion:** Known boolean profile keys have their string values (`"true"`, `"false"`, `"1"`, `"0"`, `"yes"`, `"no"`) coerced to `bool`. Non-boolean keys keep string values. The set of known boolean keys is: + +```python +_OPENAI_BOOL_PROFILE_KEYS = frozenset({ + 'openai_chat_tool_return_as_list', + 'openai_supports_strict_tool_definition', + 'openai_supports_tool_choice_required', + 'openai_chat_supports_multiple_system_messages', + 'openai_chat_supports_web_search', + 'openai_chat_supports_file_urls', + 'openai_supports_encrypted_reasoning_content', + 'openai_supports_reasoning', + 'openai_supports_reasoning_effort_none', + 'openai_chat_supports_document_input', + 'openai_chat_supports_max_completion_tokens', + 'openai_responses_requires_function_call_status_none', + 'openai_supports_phase', +}) +``` + +**Constructor signature:** +```python +def __init__( + self, + model_name: str, + *, + base_url: str | None = None, + api_key: str | None = None, + provider: Provider[AsyncOpenAI] | None = None, + tool_return_as_list: str | bool = False, + profile: ModelProfileSpec | None = None, + settings: ModelSettings | None = None, + **profile_overrides: str, +): + if provider is None: + provider = OpenAIProvider(base_url=base_url, api_key=api_key) + if isinstance(tool_return_as_list, str): + tool_return_as_list = tool_return_as_list.lower() in ('true', '1', 'yes') + + # Collect all openai_* overrides (from profile_overrides + tool_return_as_list) + overrides: dict[str, Any] = {} + if tool_return_as_list: + overrides['openai_chat_tool_return_as_list'] = True + for key, value in profile_overrides.items(): + if key.startswith('openai_'): + overrides[key] = _coerce_profile_value(key, value) + + # Merge overrides into profile dict + if overrides: + if profile is None: + profile = cast(ModelProfileSpec, overrides) + elif isinstance(profile, dict): + profile = {**profile, **overrides} + else: + # profile is a callable — wrap to inject overrides post-call + original_profile = profile + def _wrapped_profile(base: ModelProfile) -> ModelProfile: + result = original_profile(base) + return {**result, **overrides} # type: ignore[typeddict-item] + profile = _wrapped_profile + super().__init__(model_name, provider=provider, profile=profile, settings=settings) +``` + +**Coercion helper:** +```python +def _coerce_profile_value(key: str, value: str) -> str | bool: + """Coerce string values to bool for known boolean profile keys.""" + if key in _OPENAI_BOOL_PROFILE_KEYS: + return value.lower() in ('true', '1', 'yes') + return value +``` + +**Note on `provider` shorthand:** The parent `OpenAIChatModel.__init__` accepts `provider` as `OpenAIChatCompatibleProvider | Literal['openai', 'openai-chat', 'gateway'] | Provider[AsyncOpenAI]`. This subclass narrows it to `Provider[AsyncOpenAI] | None` because the primary use case is YAML-driven configuration where `base_url`/`api_key` are passed as strings. Programmatic users who need string-based provider inference should use `OpenAIChatModel` directly. + +**Example YAML (with profile overrides):** +```yaml +model_variants: + glm-5: + type: import + model: agentpool.models.openai_compatible.OpenAICompatibleModel + kw_args: + model_name: "glm-5" + base_url: "https://open.bigmodel.cn/api/paas/v4/" + api_key: "${OPENAI_API_KEY}" + tool_return_as_list: "true" + # Any openai_* key is auto-merged into the profile: + openai_system_prompt_role: "developer" + openai_supports_strict_tool_definition: "false" + openai_chat_supports_web_search: "true" + openai_chat_thinking_field: "reasoning" +``` + +**Non-`openai_*` kwargs:** Keys that don't start with `openai_` and aren't named constructor params will raise `TypeError` (standard Python behavior for unexpected kwargs). This prevents silent typos. + +### Decision 5: Content serialization strategy for list items + +**Choice:** Use `part.content_items(mode='str')` to get serialized string items, then wrap each in `ChatCompletionContentPartTextParam(type='text', text=item)`. + +**Rationale:** +- `content_items(mode='str')` already handles the serialization of non-string items via `tool_return_ta.dump_json(item).decode()`, and passes through string items as-is. This is the same serialization the parent uses, ensuring consistency. +- Each item is wrapped as `{"type": "text", "text": }` — the format that `ChatCompletionContentPartTextParam` expects and that GLM-5's chat template handles natively. +- Multimodal content (files) is excluded by the `not part.files` check and the `content_items` method's handling of `MultiModalContent` items (they pass through as non-string, but we filter them via the `isinstance(item, str)` check in the loop since `MultiModalContent` is not a `str`). + +### Decision 6: File handling unchanged + +**Choice:** When `ToolReturnPart` has files (`part.files` is non-empty), always use parent behavior. + +**Rationale:** +- The parent already handles file extraction correctly (files → user message, text → tool message). +- Mixing native list content with file extraction adds complexity without clear benefit. +- File-containing tool returns are rare and already work via the existing path. + +### Decision 7: Empty list handling + +**Choice:** When `part.content` is an empty list (`[]`), fall back to parent behavior (sends `content=''`). + +**Rationale:** +- An empty list has no items to wrap into `ChatCompletionContentPartTextParam` — sending `content=[]` to the OpenAI API may be rejected. +- The parent's `model_response_str()` handles empty lists correctly by returning `''` (via `_unwrap_data()` → `None` → `''`). +- The condition `isinstance(part.content, list) and not part.files and part.content` ensures empty lists fall through to the parent branch. + +## Architecture + +``` +agentpool/src/agentpool/models/ +├── __init__.py +├── openai_compatible.py # NEW: OpenAICompatibleModel(OpenAIChatModel) + OpenAICompatibleModelProfile +└── ... + +agentpool/tests/models/ +├── __init__.py +├── test_openai_compatible.py # NEW: unit + integration tests +└── ... +``` + +### Class diagram + +``` +OpenAIModelProfile (pydantic-ai, TypedDict) + └── OpenAICompatibleModelProfile (agentpool, adds openai_chat_tool_return_as_list) + +OpenAIChatModel (pydantic-ai) + └── OpenAICompatibleModel (agentpool) + - profile type: OpenAICompatibleModelProfile (via _resolved_profile property) + - Overrides _map_user_message() + - Reads profile flag via self._resolved_profile.get('openai_chat_tool_return_as_list', False) + - Constructor: base_url, api_key, tool_return_as_list + **profile_overrides (openai_* passthrough) + - _OPENAI_BOOL_PROFILE_KEYS: frozenset of known boolean profile keys for type coercion + - _coerce_profile_value(key, value): string→bool for known bool keys +``` + +## Risks & Mitigations + +| Risk | Impact | Mitigation | +|------|--------|------------| +| pydantic-ai changes `_map_user_message()` signature or internals | Override breaks | Pin pydantic-ai version; add integration test that detects signature changes | +| Model doesn't support list content in tool messages | API error at runtime | Profile flag defaults to `False`; only enable for known-compatible models | +| List items contain complex nested objects | Unexpected serialization | Use `content_items(mode='str')` — same serialization as parent | +| Backward compatibility for existing agents | No impact — opt-in via profile flag | Flag defaults to `False`; when `False`, pure super delegation (zero duplication) | +| `file_content` accumulation lost in override | Multimodal files not sent | Explicitly preserved in duplicated method body; covered by test 3.7 | +| Empty list content causes API error | `content=[]` rejected by API | Empty lists fall back to parent behavior (`content=''`) | diff --git a/openspec/changes/openai-compatible-native-tool-return/proposal.md b/openspec/changes/openai-compatible-native-tool-return/proposal.md new file mode 100644 index 000000000..84e54438d --- /dev/null +++ b/openspec/changes/openai-compatible-native-tool-return/proposal.md @@ -0,0 +1,54 @@ +# Proposal: OpenAI-Compatible Native Tool Return + +## Summary + +Subclass `OpenAIChatModel` in agentpool to override tool return handling for OpenAI-compatible models (e.g., GLM-5, vLLM-hosted models) whose chat templates natively render list-type tool message content. Currently pydantic-ai always serializes list-type tool return values to JSON strings, causing unnecessary escape characters and loss of structural semantics in models that support native list content. + +## Motivation + +When a tool returns a `list` value (e.g., `["result1", "result2"]`), pydantic-ai's `OpenAIChatModel` serializes it to a JSON string `'["result1", "result2"]'` via `tool_return_ta.dump_json(value).decode()`. This string is then placed as the `content` field of a `ChatCompletionToolMessageParam`. + +The OpenAI SDK's `ChatCompletionToolMessageParam.content` type actually supports `str | Iterable[ChatCompletionContentPartTextParam]`, but pydantic-ai only uses the `str` branch. + +Models like GLM-5 have chat templates that explicitly branch on `m.content is string` vs list: + +```jinja +{%- if m.content is string -%} + {{- '<|tool_return|>' }} {{- m.content }} {{- '<|/tool_return|>' }} +{%- else -%} + {% for tr in m.content %} + <|tool_return|>{{ tr.output if tr.output is defined else tr }}<|/tool_return|> + {% endfor -%} +{% endif -%} +``` + +When pydantic-ai sends `'["result1", "result2"]'` (a string), the template renders a single `<|tool_return|>` block containing escaped JSON text. If the content were a native list, the template would iterate and create multiple `<|tool_return|>` blocks — the intended behavior for multi-item tool results. + +### Problems Caused + +1. **Unnecessary escaping** — List data like `[{"name": "Alice"}, {"name": "Bob"}]` becomes `'[{"name": "Alice"}, {"name": "Bob"}]'` — the model sees literal JSON with quotes/brackets as text tokens rather than structured content. +2. **Chat template mismatch** — Models with list-aware templates (GLM-5, some vLLM deployments) cannot exercise their native multi-block rendering path. +3. **No escape hatch** — There is no way to configure pydantic-ai to send native list content for OpenAI-compatible endpoints. Issue [#3888](https://github.com/pydantic/pydantic-ai/issues/3888) proposes a `model_response_str` protocol but is still open. + +## Proposal + +Create an `OpenAICompatibleModel` subclass of `OpenAIChatModel` in agentpool that: + +1. **Overrides `_map_user_message()`** to intercept `ToolReturnPart` handling — when the tool return content is a list with no multimodal files, construct `list[ChatCompletionContentPartTextParam]` (i.e., `[{"type": "text", "text": item}, ...]`) instead of a JSON-serialized string. +2. **Uses a profile flag** (`openai_chat_tool_return_as_list: bool`) to control whether native list content is emitted, defaulting to `False` for backward compatibility. +3. **Is configurable via manifest YAML** using `ImportModelConfig` or a dedicated config type, allowing agents to opt into this behavior per-model. + +### Non-Goals + +- Modifying pydantic-ai upstream (this is an agentpool-level override) +- Supporting the Responses API (which already handles list content for multimodal returns) +- Changing behavior for non-OpenAI providers (Anthropic, Google, etc.) +- Auto-detecting whether a model supports list tool content (explicit configuration only) + +## References + +- pydantic-ai Issue #3888: https://github.com/pydantic/pydantic-ai/issues/3888 +- pydantic-ai Issue #2034: https://github.com/pydantic/pydantic-ai/issues/2034 +- pydantic-ai PR #3826: https://github.com/pydantic/pydantic-ai/pull/3826 +- agentpool Issue #112: https://github.com/Leoyzen/agentpool/issues/112 +- GLM-5 chat template: https://www.modelscope.cn/models/ZhipuAI/GLM-5/resolve/master/chat_template.jinja diff --git a/openspec/changes/openai-compatible-native-tool-return/specs/openai-compatible-model/spec.md b/openspec/changes/openai-compatible-native-tool-return/specs/openai-compatible-model/spec.md new file mode 100644 index 000000000..f939b7ed5 --- /dev/null +++ b/openspec/changes/openai-compatible-native-tool-return/specs/openai-compatible-model/spec.md @@ -0,0 +1,84 @@ +## ADDED Requirements + +### Requirement: OpenAICompatibleModel subclass +The system SHALL provide an `OpenAICompatibleModel` class that subclasses `OpenAIChatModel` from pydantic-ai, located at `agentpool/models/openai_compatible.py`. This class SHALL inherit all functionality from `OpenAIChatModel` and override `_map_user_message()` to optionally emit native list content for tool return messages. + +#### Scenario: Class inherits from OpenAIChatModel +- **WHEN** `OpenAICompatibleModel` is instantiated +- **THEN** it SHALL be a subclass of `OpenAIChatModel` and accept constructor parameters (`model_name`, `base_url`, `api_key`, `provider`, `tool_return_as_list`, `profile`, `settings`) + +#### Scenario: Default behavior matches parent +- **WHEN** `OpenAICompatibleModel` is instantiated without the `openai_chat_tool_return_as_list` profile flag (or with it set to `False`) +- **THEN** `_map_user_message()` SHALL delegate entirely to `super()._map_user_message(message)` — producing identical output to `OpenAIChatModel`, with tool return content JSON-serialized to a string + +### Requirement: Custom profile TypedDict for type safety +The system SHALL define an `OpenAICompatibleModelProfile` TypedDict in `agentpool/models/openai_compatible.py` that extends `OpenAIModelProfile` from pydantic-ai with the additional `openai_chat_tool_return_as_list: bool` key. This ensures type-safe access to the profile flag without modifying pydantic-ai upstream. + +#### Scenario: Profile flag is type-safe +- **WHEN** `OpenAICompatibleModel` accesses `self._resolved_profile.get('openai_chat_tool_return_as_list', False)` +- **THEN** the access SHALL be type-checked by pyright/mypy without errors, because `OpenAICompatibleModelProfile` declares the key and `_resolved_profile` casts to it (following the `OpenRouterModel._resolved_profile` pattern) + +### Requirement: Native list tool return when profile flag enabled +When the `openai_chat_tool_return_as_list` profile flag is `True`, and a `ToolReturnPart` has non-empty list content with no multimodal files, the model SHALL emit `ChatCompletionToolMessageParam.content` as `list[ChatCompletionContentPartTextParam]` (i.e., `[{"type": "text", "text": ...}, ...]`) instead of a JSON-serialized string. + +#### Scenario: List content with string items +- **WHEN** `openai_chat_tool_return_as_list` is `True` and a `ToolReturnPart` has `content=["result1", "result2"]` and no files +- **THEN** the tool message `content` SHALL be `[{"type": "text", "text": "result1"}, {"type": "text", "text": "result2"}]` + +#### Scenario: List content with non-string items +- **WHEN** `openai_chat_tool_return_as_list` is `True` and a `ToolReturnPart` has `content=[{"key": "value"}, 42]` and no files +- **THEN** each non-string item SHALL be serialized via `content_items(mode='str')` (which uses `tool_return_ta.dump_json(item).decode()`) and wrapped as `{"type": "text", "text": }` + +#### Scenario: String content unaffected +- **WHEN** `openai_chat_tool_return_as_list` is `True` and a `ToolReturnPart` has `content="plain string"` and no files +- **THEN** the tool message `content` SHALL remain a plain string `"plain string"` (not wrapped in a list) + +#### Scenario: Empty list falls back to parent +- **WHEN** `openai_chat_tool_return_as_list` is `True` and a `ToolReturnPart` has `content=[]` (empty list) and no files +- **THEN** the tool message `content` SHALL be `''` (empty string), matching parent behavior via `model_response_str_and_user_content()` + +#### Scenario: Multimodal content unaffected +- **WHEN** `openai_chat_tool_return_as_list` is `True` and a `ToolReturnPart` has files (multimodal content) +- **THEN** the parent's behavior SHALL be used (files extracted to user message, text serialized to string) + +#### Scenario: Flag disabled falls back to parent +- **WHEN** `openai_chat_tool_return_as_list` is `False` (or unset) +- **THEN** `_map_user_message()` SHALL delegate entirely to `super()._map_user_message(message)` — all `ToolReturnPart` content is JSON-serialized to string + +### Requirement: Non-ToolReturnPart messages handled identically to parent +When the profile flag is `True`, all non-`ToolReturnPart` message parts (`SystemPromptPart`, `UserPromptPart`, `RetryPromptPart`) SHALL be handled identically to the parent `OpenAIChatModel._map_user_message()`, including `file_content` accumulation and the final `UserPromptPart` yield for multimodal files. + +#### Scenario: UserPromptPart handling +- **WHEN** a `ModelRequest` contains a `UserPromptPart` and the flag is `True` +- **THEN** the part SHALL be mapped identically to the parent's implementation + +#### Scenario: SystemPromptPart handling +- **WHEN** a `ModelRequest` contains a `SystemPromptPart` and the flag is `True` +- **THEN** the part SHALL be mapped identically to the parent's implementation + +#### Scenario: RetryPromptPart handling +- **WHEN** a `ModelRequest` contains a `RetryPromptPart` and the flag is `True` +- **THEN** the part SHALL be mapped identically to the parent's implementation (using `part.model_response()`, not affected by the list-content override) + +#### Scenario: file_content accumulation preserved +- **WHEN** the flag is `True` and a `ToolReturnPart` with files is processed +- **THEN** the `file_content` list SHALL be accumulated across all parts and a final `UserPromptPart` with the file content SHALL be yielded, identical to parent behavior + +### Requirement: Manifest configuration via ImportModelConfig with `openai_*` kwarg passthrough +The system SHALL support configuring `OpenAICompatibleModel` through the existing `ImportModelConfig` mechanism in agent manifests. Because `ImportModelConfig.kw_args` is typed as `dict[str, str]`, the `tool_return_as_list` flag SHALL be passed as a string (`"true"`/`"false"`) and coerced to `bool` by the constructor. The constructor SHALL accept `base_url` and `api_key` as convenience parameters and construct an `OpenAIProvider` internally when `provider` is not explicitly given. Additionally, any `openai_*` prefixed key in `kw_args` SHALL be automatically captured via `**profile_overrides` and merged into the profile dict, with known boolean keys coerced from string to `bool`. Non-`openai_*` keys that are not named constructor params SHALL raise `TypeError`. + +#### Scenario: ImportModelConfig with tool_return_as_list flag +- **WHEN** a manifest defines a model variant with `type: import`, `model: agentpool.models.openai_compatible.OpenAICompatibleModel`, and `kw_args` containing `tool_return_as_list: "true"`, `base_url`, `api_key`, and `model_name` +- **THEN** the resolved model SHALL be an instance of `OpenAICompatibleModel` with the `openai_chat_tool_return_as_list` profile flag set to `True` + +#### Scenario: openai_* profile overrides in kw_args +- **WHEN** a manifest defines `kw_args` with `openai_system_prompt_role: "developer"` and `openai_supports_strict_tool_definition: "false"` +- **THEN** the resolved model's profile SHALL contain `openai_system_prompt_role` set to `"developer"` (string) and `openai_supports_strict_tool_definition` set to `False` (bool, coerced from string `"false"`) + +#### Scenario: Non-openai_* unknown kwarg raises TypeError +- **WHEN** a manifest defines `kw_args` with a key that does not start with `openai_` and is not a named constructor parameter (e.g. `foo_bar: "baz"`) +- **THEN** the constructor SHALL raise `TypeError` (standard Python behavior for unexpected kwargs) + +#### Scenario: Model variant referencing import config +- **WHEN** an agent references a model variant name that resolves to an `ImportModelConfig` for `OpenAICompatibleModel` +- **THEN** the agent SHALL use the `OpenAICompatibleModel` instance with the configured profile diff --git a/openspec/changes/openai-compatible-native-tool-return/tasks.md b/openspec/changes/openai-compatible-native-tool-return/tasks.md new file mode 100644 index 000000000..1c5cfdf54 --- /dev/null +++ b/openspec/changes/openai-compatible-native-tool-return/tasks.md @@ -0,0 +1,41 @@ +# Tasks: OpenAI-Compatible Native Tool Return + +## 1. Create `OpenAICompatibleModel` class and profile + +- [ ] 1.1 Create `packages/agentpool/src/agentpool/models/openai_compatible.py` +- [ ] 1.2 Define `OpenAICompatibleModelProfile(OpenAIModelProfile)` TypedDict with `openai_chat_tool_return_as_list: bool` key +- [ ] 1.3 Define `OpenAICompatibleModel(OpenAIChatModel)` with `@dataclass(init=False)` and custom `__init__` accepting `model_name`, `base_url`, `api_key`, `provider` (`Provider[AsyncOpenAI] | None`, narrower than parent's string-literal union — documented as intentional for YAML-driven config), `tool_return_as_list` (str|bool, coerced to bool), `profile` (`ModelProfileSpec | None`), `settings`, and `**profile_overrides: str` for arbitrary `openai_*` profile overrides; construct `OpenAIProvider` from `base_url`/`api_key` when `provider` is None; merge `tool_return_as_list` + `openai_*` overrides into profile — handle three cases: `profile is None` → create new dict with `cast(ModelProfileSpec, {...})`; `isinstance(profile, dict)` → spread-merge `{**profile, **overrides}`; callable profile → wrap in lambda that injects overrides post-call +- [ ] 1.4 Define `_OPENAI_BOOL_PROFILE_KEYS` frozenset and `_coerce_profile_value(key, value)` helper that coerces string → bool for known boolean profile keys, returns string for others +- [ ] 1.5 Add `_resolved_profile` property returning `cast(OpenAICompatibleModelProfile, self.profile)` (following `OpenRouterModel._resolved_profile` pattern) +- [ ] 1.6 Override `_map_user_message()`: + - [ ] 1.6.1 When flag is `False` (default): delegate entirely to `super()._map_user_message(message)` — zero code duplication + - [ ] 1.6.2 When flag is `True`: duplicate the parent's method body, replacing only the `ToolReturnPart` branch: + - For `ToolReturnPart` with non-empty list content (`isinstance(part.content, list) and part.content`) and no files (`not part.files`): use `part.content_items(mode='str')` to get serialized items, wrap each string item as `ChatCompletionContentPartTextParam(type='text', text=item)`, yield `ChatCompletionToolMessageParam` with `content=list[...]` + - For `ToolReturnPart` with string content, empty list, or files: use parent behavior (`model_response_str_and_user_content()`) + - For all other part types (`SystemPromptPart`, `UserPromptPart`, `RetryPromptPart`): identical to parent implementation + - Preserve `file_content` accumulation and final `UserPromptPart` yield + - End with `assert_never(part)` for exhaustive matching +- [ ] 1.7 Export `OpenAICompatibleModel` and `OpenAICompatibleModelProfile` from `packages/agentpool/src/agentpool/models/__init__.py` + +## 2. Tests + +- [ ] 2.1 Create `packages/agentpool/tests/models/test_openai_compatible.py` +- [ ] 2.2 Unit test: `OpenAICompatibleModel` is a subclass of `OpenAIChatModel` +- [ ] 2.3 Unit test: Default behavior (flag `False`/unset) delegates to super — tool return content is JSON string +- [ ] 2.4 Unit test: Flag `True` with list string items → `content` is `list[ChatCompletionContentPartTextParam]` +- [ ] 2.5 Unit test: Flag `True` with list non-string items → each item JSON-serialized and wrapped in `{"type": "text", "text": ...}` +- [ ] 2.6 Unit test: Flag `True` with string content → `content` remains plain string (not list-wrapped) +- [ ] 2.7 Unit test: Flag `True` with empty list `[]` → `content` is `''` (falls back to parent) +- [ ] 2.8 Unit test: Flag `True` with multimodal content (files) → delegates to parent behavior (file extraction + user message) +- [ ] 2.9 Unit test: Flag `True` with mixed message (UserPromptPart + ToolReturnPart + RetryPromptPart) → only ToolReturnPart with list content is modified, others match parent output +- [ ] 2.10 Unit test: `file_content` accumulation preserved when flag is `True` and ToolReturnPart has files +- [ ] 2.11 Integration test: `ImportModelConfig` resolves `OpenAICompatibleModel` with `tool_return_as_list: "true"` from YAML (string coercion to bool) +- [ ] 2.12 Integration test: `openai_*` profile overrides in `kw_args` are correctly merged into profile (e.g. `openai_system_prompt_role: "developer"`, `openai_supports_strict_tool_definition: "false"`) +- [ ] 2.13 Integration test: Non-`openai_*` unknown kwarg raises `TypeError` +- [ ] 2.14 Integration test: End-to-end agent run with `TestModel` or mock verifying the tool message content shape + +## 3. Documentation + +- [ ] 3.1 Add module docstring to `openai_compatible.py` explaining purpose and usage +- [ ] 3.2 Add example YAML manifest snippet in docstring showing `ImportModelConfig` usage with `tool_return_as_list: "true"`, `base_url`, and `api_key` +- [ ] 3.3 Update `packages/agentpool/src/agentpool/models/__init__.py` `__all__` if applicable diff --git a/src/agentpool/models/__init__.py b/src/agentpool/models/__init__.py index 60ea571cf..093e79024 100644 --- a/src/agentpool/models/__init__.py +++ b/src/agentpool/models/__init__.py @@ -5,6 +5,10 @@ from agentpool.models.acp_agents import ACPAgentConfig, ACPAgentConfigTypes, BaseACPAgentConfig from agentpool.models.agents import AnyToolConfig, NativeAgentConfig # noqa: F401 from agentpool.models.manifest import AgentsManifest, AnyAgentConfig +from agentpool.models.openai_compatible import ( + OpenAICompatibleModel, + OpenAICompatibleModelProfile, +) from agentpool.models.pending_interaction import PendingPermission, PendingQuestion @@ -15,6 +19,8 @@ "AnyAgentConfig", "BaseACPAgentConfig", "NativeAgentConfig", + "OpenAICompatibleModel", + "OpenAICompatibleModelProfile", "PendingPermission", "PendingQuestion", ] diff --git a/src/agentpool/models/openai_compatible.py b/src/agentpool/models/openai_compatible.py new file mode 100644 index 000000000..9e09bf1f2 --- /dev/null +++ b/src/agentpool/models/openai_compatible.py @@ -0,0 +1,323 @@ +"""OpenAI-compatible model with native list tool return support. + +Subclass of pydantic-ai's ``OpenAIChatModel`` that optionally emits native list +content for tool return messages instead of JSON-serialized strings. This is +useful for OpenAI-compatible models (GLM-5, vLLM, etc.) whose chat templates +natively render list-type tool message content. + +Example YAML manifest usage: + +```yaml +model_variants: + glm-5: + type: import + model: agentpool.models.openai_compatible.OpenAICompatibleModel + kw_args: + model_name: "glm-5" + base_url: "https://open.bigmodel.cn/api/paas/v4/" + api_key: "${OPENAI_API_KEY}" + tool_return_as_list: "true" + openai_system_prompt_role: "developer" + openai_supports_strict_tool_definition: "false" +``` +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any, TypedDict, cast, override + +from pydantic_ai.messages import ( + ModelRequest, + RetryPromptPart, + SystemPromptPart, + ToolReturnPart, + UserPromptPart, +) +from pydantic_ai.models.openai import ( # type: ignore[attr-defined] + OpenAIChatModel, + _guard_tool_call_id, +) +from pydantic_ai.profiles import ModelProfile, ModelProfileSpec +from pydantic_ai.profiles.openai import OpenAIModelProfile +from pydantic_ai.providers.openai import OpenAIProvider + + +try: + from openai.types import chat + from openai.types.chat import ChatCompletionContentPartTextParam +except ImportError: # pragma: no cover + pass + +if TYPE_CHECKING: + from collections.abc import AsyncIterator + + from openai import AsyncOpenAI + from pydantic_ai.providers import Provider + from pydantic_ai.settings import ModelSettings + + +class OpenAICompatibleModelProfile(TypedDict, total=False): + """Profile dict for :class:`OpenAICompatibleModel`. + + Extends ``OpenAIModelProfile`` fields with the additional + ``openai_chat_tool_return_as_list`` key. + """ + + openai_chat_tool_return_as_list: bool + + +_OPENAI_BOOL_PROFILE_KEYS: frozenset[str] = frozenset({ + "openai_chat_tool_return_as_list", + "openai_supports_strict_tool_definition", + "openai_supports_sampling_settings", + "openai_supports_tool_choice_required", + "openai_chat_supports_multiple_system_messages", + "openai_chat_supports_web_search", + "openai_chat_supports_file_urls", + "openai_supports_encrypted_reasoning_content", + "openai_supports_reasoning", + "openai_supports_reasoning_effort_none", + "openai_responses_requires_function_call_status_none", + "openai_supports_phase", + "supports_inline_system_prompts", +}) +"""Profile keys whose values should be coerced from string to bool.""" + +_OPENAI_PROFILE_FIELDS: frozenset[str] = frozenset({ + "openai_chat_thinking_field", + "openai_chat_send_back_thinking_parts", + "openai_supports_strict_tool_definition", + "openai_supports_sampling_settings", + "openai_unsupported_model_settings", + "openai_supports_tool_choice_required", + "openai_system_prompt_role", + "supports_inline_system_prompts", + "openai_chat_supports_multiple_system_messages", + "openai_chat_supports_web_search", + "openai_chat_audio_input_encoding", + "openai_chat_supports_file_urls", + "openai_supports_encrypted_reasoning_content", + "openai_supports_reasoning", + "openai_supports_reasoning_effort_none", + "openai_responses_requires_function_call_status_none", + "openai_supports_phase", +}) + + +def _coerce_profile_value(key: str, value: Any) -> Any: + """Coerce string values to bool for known boolean profile keys. + + Args: + key: The profile key name. + value: The raw value (typically a string from YAML kw_args). + + Returns: + The coerced value — ``bool`` for known boolean keys, original value otherwise. + """ + if key in _OPENAI_BOOL_PROFILE_KEYS and isinstance(value, str): + return value.lower() in ("true", "1", "yes") + return value + + +def _profile_to_dict(profile: ModelProfile) -> dict[str, Any]: + """Extract non-default OpenAI profile fields into a dict.""" + result: dict[str, Any] = {} + for field_name in _OPENAI_PROFILE_FIELDS: + val = getattr(profile, field_name, None) + if val is not None: + result[field_name] = val + return result + + +class OpenAICompatibleModel(OpenAIChatModel): + """An ``OpenAIChatModel`` subclass that can emit native list tool return content. + + When the ``openai_chat_tool_return_as_list`` profile flag is ``True`` and a + ``ToolReturnPart`` has non-empty list content with no multimodal files, the + tool message ``content`` is emitted as + ``list[ChatCompletionContentPartTextParam]`` instead of a JSON-serialized + string. This matches the expectation of chat templates that branch on + ``m.content is string`` vs list (e.g. GLM-5). + + All other behavior is inherited from :class:`OpenAIChatModel`. + """ + + def __init__( + self, + model_name: str, + *, + base_url: str | None = None, + api_key: str | None = None, + provider: Provider[AsyncOpenAI] | None = None, + tool_return_as_list: str | bool = False, + profile: ModelProfileSpec | None = None, + settings: ModelSettings | None = None, + **profile_overrides: str, + ) -> None: + """Initialize an OpenAI-compatible model. + + Args: + model_name: The name of the model to use. + base_url: Base URL for the OpenAI-compatible API. Ignored if + ``provider`` is given. + api_key: API key for authentication. Ignored if ``provider`` is + given. + provider: A pre-built ``Provider[AsyncOpenAI]``. When ``None``, an + ``OpenAIProvider`` is constructed from ``base_url`` and + ``api_key``. + tool_return_as_list: Whether to emit native list tool return + content. Accepts ``str`` (``"true"``/``"false"``) or ``bool`` + for YAML convenience. + profile: The model profile spec to use. + settings: Default model settings for this model instance. + **profile_overrides: Arbitrary ``openai_*`` prefixed keys merged + into the profile. Known boolean keys are coerced from + string to ``bool``. + """ + # Validate openai_* kwargs before constructing OpenAIProvider, + # so invalid kwargs raise TypeError before any network/auth setup. + real_overrides: dict[str, Any] = {} + for key, value in profile_overrides.items(): + if key.startswith("openai_"): + coerced = _coerce_profile_value(key, value) + if key in _OPENAI_PROFILE_FIELDS: + real_overrides[key] = coerced + else: + msg = ( + f"Unexpected keyword argument '{key}'. " + f"Only 'openai_*' prefixed keys are accepted as profile overrides." + ) + raise TypeError(msg) + + if provider is None: + provider = OpenAIProvider(base_url=base_url, api_key=api_key) + + # Coerce tool_return_as_list to bool and store on instance + self._tool_return_as_list_enabled: bool = ( + tool_return_as_list + if isinstance(tool_return_as_list, bool) + else isinstance(tool_return_as_list, str) + and tool_return_as_list.lower() in ("true", "1", "yes") + ) + + # Build the merged profile spec + merged_profile = self._build_merged_profile(profile, real_overrides) + + super().__init__( + model_name=model_name, + provider=provider, + profile=merged_profile, + settings=settings, + ) + + @staticmethod + def _build_merged_profile( + profile: ModelProfileSpec | None, + real_overrides: dict[str, Any], + ) -> ModelProfileSpec | None: + """Merge openai_* overrides into the profile spec.""" + if not real_overrides: + return profile + + if profile is None: + return OpenAIModelProfile(**real_overrides) + + if isinstance(profile, ModelProfile): + base_dict = _profile_to_dict(profile) + base_dict.update(real_overrides) + return OpenAIModelProfile(**base_dict) + + # Callable profile: wrap to inject overrides post-call + original_fn = profile + + def _wrapped(model_name: str) -> ModelProfile | None: + result = original_fn(model_name) + if result is None: + return None + result_dict = _profile_to_dict(result) if isinstance(result, ModelProfile) else {} + result_dict.update(real_overrides) + return OpenAIModelProfile(**result_dict) + + return _wrapped + + @property + def _resolved_profile(self) -> OpenAICompatibleModelProfile: + """Return the resolved profile as a typed dict for flag access.""" + result: dict[str, Any] = {} + profile = OpenAIModelProfile.from_profile(self.profile) + for field_name in ( + "openai_supports_strict_tool_definition", + "openai_system_prompt_role", + ): + val = getattr(profile, field_name, None) + if val is not None: + result[field_name] = val + result["openai_chat_tool_return_as_list"] = self._tool_return_as_list_enabled + return cast(OpenAICompatibleModelProfile, result) + + @override + async def _map_user_message( + self, message: ModelRequest + ) -> AsyncIterator[chat.ChatCompletionMessageParam]: + if not self._tool_return_as_list_enabled: + # Flag disabled: delegate entirely to parent + async for item in super()._map_user_message(message): + yield item + return + + # Flag enabled: duplicate parent logic, replacing ToolReturnPart branch + file_content: list[Any] = [] + for part in message.parts: + if isinstance(part, SystemPromptPart): + system_prompt_role = OpenAIModelProfile.from_profile( + self.profile + ).openai_system_prompt_role + if system_prompt_role == "developer": + yield chat.ChatCompletionDeveloperMessageParam( + role="developer", content=part.content + ) + elif system_prompt_role == "user": + yield chat.ChatCompletionUserMessageParam(role="user", content=part.content) + else: + yield chat.ChatCompletionSystemMessageParam(role="system", content=part.content) + elif isinstance(part, UserPromptPart): + yield await self._map_user_prompt(part) + elif isinstance(part, ToolReturnPart): + if isinstance(part.content, list) and part.content and not part.files: + # Native list content: emit list[ChatCompletionContentPartTextParam] + content_parts: list[ChatCompletionContentPartTextParam] = [ + ChatCompletionContentPartTextParam(type="text", text=item) + for item in part.content_items(mode="str") + if isinstance(item, str) + ] + yield chat.ChatCompletionToolMessageParam( + role="tool", + tool_call_id=_guard_tool_call_id(t=part), + content=content_parts, + ) + else: + # String content, empty list, or files: use parent behavior + tool_text, tool_file_content = part.model_response_str_and_user_content() + file_content.extend(tool_file_content) + yield chat.ChatCompletionToolMessageParam( + role="tool", + tool_call_id=_guard_tool_call_id(t=part), + content=tool_text, + ) + elif isinstance(part, RetryPromptPart): + if part.tool_name is None: + yield chat.ChatCompletionUserMessageParam( + role="user", content=part.model_response() + ) + else: + yield chat.ChatCompletionToolMessageParam( + role="tool", + tool_call_id=_guard_tool_call_id(t=part), + content=part.model_response(), + ) + else: + from typing import assert_never + + assert_never(part) + if file_content: + yield await self._map_user_prompt(UserPromptPart(content=file_content)) diff --git a/tests/models/__init__.py b/tests/models/__init__.py new file mode 100644 index 000000000..744975a84 --- /dev/null +++ b/tests/models/__init__.py @@ -0,0 +1 @@ +"""Tests for model classes.""" diff --git a/tests/models/test_openai_compatible.py b/tests/models/test_openai_compatible.py new file mode 100644 index 000000000..aeccfc1d9 --- /dev/null +++ b/tests/models/test_openai_compatible.py @@ -0,0 +1,490 @@ +"""Tests for OpenAICompatibleModel.""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock, patch + +from pydantic_ai.messages import ( + ModelRequest, + RetryPromptPart, + SystemPromptPart, + ToolReturnPart, + UserPromptPart, +) +from pydantic_ai.profiles.openai import OpenAIModelProfile +import pytest + +from agentpool.models.openai_compatible import ( + OpenAICompatibleModel, + _coerce_profile_value, +) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _mock_provider() -> MagicMock: + """Create a mock provider with a real OpenAIModelProfile.""" + provider = MagicMock() + provider.model_profile = OpenAIModelProfile() + return provider + + +async def _collect(async_iter: Any) -> list[Any]: + """Collect all items from an async iterator into a list.""" + return [item async for item in async_iter] + + +# --------------------------------------------------------------------------- +# 1. Subclass relationship +# --------------------------------------------------------------------------- + + +def test_is_subclass_of_openai_chat_model() -> None: + """OpenAICompatibleModel should be a subclass of OpenAIChatModel.""" + from pydantic_ai.models.openai import OpenAIChatModel + + assert issubclass(OpenAICompatibleModel, OpenAIChatModel) + + +# --------------------------------------------------------------------------- +# 2. Constructor and profile handling +# --------------------------------------------------------------------------- + + +def test_constructor_accepts_base_url_and_api_key() -> None: + """Constructor should accept base_url, api_key, and tool_return_as_list.""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_provider_cls: + mock_provider_cls.return_value = _mock_provider() + model = OpenAICompatibleModel( + model_name="test-model", + base_url="https://api.example.com/v1", + api_key="test-key", + tool_return_as_list="true", + ) + mock_provider_cls.assert_called_once_with( + base_url="https://api.example.com/v1", api_key="test-key" + ) + assert model.model_name == "test-model" + + +def test_tool_return_as_list_string_true() -> None: + """tool_return_as_list='true' should set the flag to True.""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_cls: + mock_cls.return_value = _mock_provider() + model = OpenAICompatibleModel( + model_name="test", + tool_return_as_list="true", + ) + assert model._resolved_profile.get("openai_chat_tool_return_as_list") is True + + +def test_tool_return_as_list_string_false() -> None: + """tool_return_as_list='false' should set the flag to False.""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_cls: + mock_cls.return_value = _mock_provider() + model = OpenAICompatibleModel( + model_name="test", + tool_return_as_list="false", + ) + assert model._resolved_profile.get("openai_chat_tool_return_as_list") is False + + +def test_tool_return_as_list_bool() -> None: + """tool_return_as_list=True (bool) should set the flag to True.""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_cls: + mock_cls.return_value = _mock_provider() + model = OpenAICompatibleModel( + model_name="test", + tool_return_as_list=True, + ) + assert model._resolved_profile.get("openai_chat_tool_return_as_list") is True + + +def test_tool_return_as_list_default_false() -> None: + """Default (no flag) should be False.""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_cls: + mock_cls.return_value = _mock_provider() + model = OpenAICompatibleModel(model_name="test") + assert model._resolved_profile.get("openai_chat_tool_return_as_list") is False + + +def test_openai_profile_overrides_passed_through() -> None: + """openai_* prefixed kwargs should be merged into the profile.""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_cls: + mock_cls.return_value = _mock_provider() + model = OpenAICompatibleModel( + model_name="test", + tool_return_as_list="true", + openai_system_prompt_role="developer", + openai_supports_strict_tool_definition="false", + ) + profile = OpenAIModelProfile.from_profile(model.profile) + assert profile.openai_system_prompt_role == "developer" + assert profile.openai_supports_strict_tool_definition is False + + +def test_non_openai_kwarg_raises_type_error() -> None: + """Non-openai_* unknown kwargs should raise TypeError.""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_cls: + mock_cls.return_value = _mock_provider() + with pytest.raises(TypeError, match="Unexpected keyword argument"): + OpenAICompatibleModel( + model_name="test", + foo_bar="baz", # type: ignore[call-arg] + ) + + +def test_profile_dict_merged_with_overrides() -> None: + """When profile is a ModelProfile, overrides should be merged in.""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_cls: + mock_cls.return_value = _mock_provider() + model = OpenAICompatibleModel( + model_name="test", + tool_return_as_list="true", + profile=OpenAIModelProfile(openai_system_prompt_role="developer"), + ) + profile = OpenAIModelProfile.from_profile(model.profile) + assert profile.openai_system_prompt_role == "developer" + assert model._resolved_profile.get("openai_chat_tool_return_as_list") is True + + +# --------------------------------------------------------------------------- +# 3. _coerce_profile_value helper +# --------------------------------------------------------------------------- + + +def test_coerce_bool_true() -> None: + """String 'true' should coerce to True for boolean keys.""" + assert _coerce_profile_value("openai_supports_strict_tool_definition", "true") is True + + +def test_coerce_bool_false() -> None: + """String 'false' should coerce to False for boolean keys.""" + assert _coerce_profile_value("openai_supports_strict_tool_definition", "false") is False + + +def test_coerce_non_bool_key_unchanged() -> None: + """Non-boolean keys should return the value unchanged.""" + assert _coerce_profile_value("openai_system_prompt_role", "developer") == "developer" + + +def test_coerce_non_string_value_unchanged() -> None: + """Non-string values should be returned unchanged even for boolean keys.""" + assert _coerce_profile_value("openai_supports_strict_tool_definition", True) is True + assert _coerce_profile_value("openai_supports_strict_tool_definition", 1) == 1 + + +# --------------------------------------------------------------------------- +# 4. _map_user_message behavior +# --------------------------------------------------------------------------- + + +@pytest.fixture +def model_flag_disabled() -> OpenAICompatibleModel: + """Model with tool_return_as_list disabled (default).""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_cls: + mock_cls.return_value = _mock_provider() + return OpenAICompatibleModel(model_name="test", tool_return_as_list="false") + + +@pytest.fixture +def model_flag_enabled() -> OpenAICompatibleModel: + """Model with tool_return_as_list enabled.""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_cls: + mock_cls.return_value = _mock_provider() + return OpenAICompatibleModel(model_name="test", tool_return_as_list="true") + + +async def test_flag_disabled_delegates_to_super( + model_flag_disabled: OpenAICompatibleModel, +) -> None: + """When flag is False, _map_user_message should delegate to parent.""" + part = ToolReturnPart( + tool_name="test_tool", + content=["item1", "item2"], + tool_call_id="call_123", + ) + message = ModelRequest(parts=[part]) + + # Mock the parent's _map_user_message to verify delegation + with ( + patch.object( + OpenAIChatModel, + "_map_user_message", + return_value=AsyncIteratorMock([MagicMock()]), + ) as mock_super, + ): + await _collect(model_flag_disabled._map_user_message(message)) + mock_super.assert_called_once_with(message) + + +async def test_flag_enabled_list_string_items( + model_flag_enabled: OpenAICompatibleModel, +) -> None: + """Flag True + list of strings -> content is list of text parts.""" + part = ToolReturnPart( + tool_name="test_tool", + content=["result1", "result2"], + tool_call_id="call_123", + ) + message = ModelRequest(parts=[part]) + + results = await _collect(model_flag_enabled._map_user_message(message)) + + assert len(results) == 1 + tool_msg = results[0] + assert tool_msg["role"] == "tool" + assert tool_msg["tool_call_id"] == "call_123" + content = tool_msg["content"] + assert isinstance(content, list) + assert len(content) == 2 + assert content[0]["type"] == "text" + assert content[0]["text"] == "result1" + assert content[1]["type"] == "text" + assert content[1]["text"] == "result2" + + +async def test_flag_enabled_list_non_string_items( + model_flag_enabled: OpenAICompatibleModel, +) -> None: + """Flag True + list with non-string items -> each JSON-serialized and wrapped.""" + part = ToolReturnPart( + tool_name="test_tool", + content=[{"key": "value"}, 42], + tool_call_id="call_123", + ) + message = ModelRequest(parts=[part]) + + results = await _collect(model_flag_enabled._map_user_message(message)) + + assert len(results) == 1 + tool_msg = results[0] + content = tool_msg["content"] + assert isinstance(content, list) + assert len(content) == 2 + assert content[0]["type"] == "text" + # Non-string items are JSON-serialized via content_items(mode='str') + assert '"key"' in content[0]["text"] + assert "value" in content[0]["text"] + assert content[1]["type"] == "text" + assert content[1]["text"] == "42" + + +async def test_flag_enabled_string_content( + model_flag_enabled: OpenAICompatibleModel, +) -> None: + """Flag True + string content -> content remains plain string.""" + part = ToolReturnPart( + tool_name="test_tool", + content="plain string", + tool_call_id="call_123", + ) + message = ModelRequest(parts=[part]) + + results = await _collect(model_flag_enabled._map_user_message(message)) + + assert len(results) == 1 + tool_msg = results[0] + content = tool_msg["content"] + assert isinstance(content, str) + assert content == "plain string" + + +async def test_flag_enabled_empty_list( + model_flag_enabled: OpenAICompatibleModel, +) -> None: + """Flag True + empty list -> falls back to parent (empty string).""" + part = ToolReturnPart( + tool_name="test_tool", + content=[], + tool_call_id="call_123", + ) + message = ModelRequest(parts=[part]) + + results = await _collect(model_flag_enabled._map_user_message(message)) + + assert len(results) == 1 + tool_msg = results[0] + content = tool_msg["content"] + # Empty list falls back to parent behavior which serializes to '' + assert isinstance(content, str) + + +async def test_flag_enabled_system_prompt_part( + model_flag_enabled: OpenAICompatibleModel, +) -> None: + """Flag True + SystemPromptPart -> mapped identically to parent.""" + part = SystemPromptPart(content="You are a helpful assistant.") + message = ModelRequest(parts=[part]) + + results = await _collect(model_flag_enabled._map_user_message(message)) + + assert len(results) == 1 + sys_msg = results[0] + assert sys_msg["role"] == "system" + assert sys_msg["content"] == "You are a helpful assistant." + + +async def test_flag_enabled_system_prompt_developer_role( + model_flag_enabled: OpenAICompatibleModel, +) -> None: + """Flag True + SystemPromptPart with developer role -> developer message.""" + with patch("agentpool.models.openai_compatible.OpenAIProvider") as mock_cls: + mock_cls.return_value = _mock_provider() + model = OpenAICompatibleModel( + model_name="test", + tool_return_as_list="true", + openai_system_prompt_role="developer", + ) + part = SystemPromptPart(content="You are a developer.") + message = ModelRequest(parts=[part]) + + results = await _collect(model._map_user_message(message)) + + assert len(results) == 1 + assert results[0]["role"] == "developer" + + +async def test_flag_enabled_retry_prompt_with_tool_name( + model_flag_enabled: OpenAICompatibleModel, +) -> None: + """Flag True + RetryPromptPart with tool_name -> tool message.""" + part = RetryPromptPart( + tool_name="test_tool", + content="Retry this", + tool_call_id="call_123", + ) + message = ModelRequest(parts=[part]) + + results = await _collect(model_flag_enabled._map_user_message(message)) + + assert len(results) == 1 + tool_msg = results[0] + assert tool_msg["role"] == "tool" + + +async def test_flag_enabled_retry_prompt_without_tool_name( + model_flag_enabled: OpenAICompatibleModel, +) -> None: + """Flag True + RetryPromptPart without tool_name -> user message.""" + part = RetryPromptPart( + tool_name=None, + content="Retry this", + ) + message = ModelRequest(parts=[part]) + + results = await _collect(model_flag_enabled._map_user_message(message)) + + assert len(results) == 1 + user_msg = results[0] + assert user_msg["role"] == "user" + + +async def test_flag_enabled_mixed_message( + model_flag_enabled: OpenAICompatibleModel, +) -> None: + """Flag True + mixed message parts -> only ToolReturnPart with list is modified.""" + from pydantic_ai.messages import ModelRequestPart + + parts: list[ModelRequestPart] = [ + UserPromptPart(content="Run the tool"), + ToolReturnPart( + tool_name="test_tool", + content=["result1", "result2"], + tool_call_id="call_1", + ), + ToolReturnPart( + tool_name="other_tool", + content="string result", + tool_call_id="call_2", + ), + ] + message = ModelRequest(parts=parts) + + results = await _collect(model_flag_enabled._map_user_message(message)) + + # Should have: user prompt, tool msg 1 (list), tool msg 2 (string) + assert len(results) == 3 + # First: user message + assert results[0]["role"] == "user" + # Second: tool message with list content + assert results[1]["role"] == "tool" + assert isinstance(results[1]["content"], list) + assert len(results[1]["content"]) == 2 + # Third: tool message with string content + assert results[2]["role"] == "tool" + assert isinstance(results[2]["content"], str) + + +# --------------------------------------------------------------------------- +# 5. Integration: ImportModelConfig resolution +# --------------------------------------------------------------------------- + + +def test_import_model_config_resolves_model() -> None: + """ImportModelConfig should resolve OpenAICompatibleModel from YAML.""" + from llmling_models_config import ImportModelConfig + + config = ImportModelConfig( + model="agentpool.models.openai_compatible.OpenAICompatibleModel", + kw_args={ + "model_name": "glm-5", + "base_url": "https://open.bigmodel.cn/api/paas/v4/", + "api_key": "test-key", + "tool_return_as_list": "true", + "openai_system_prompt_role": "developer", + "openai_supports_strict_tool_definition": "false", + }, + ) + model = config.get_model() + assert isinstance(model, OpenAICompatibleModel) + assert model._resolved_profile.get("openai_chat_tool_return_as_list") is True + profile = OpenAIModelProfile.from_profile(model.profile) + assert profile.openai_system_prompt_role == "developer" + assert profile.openai_supports_strict_tool_definition is False + + +def test_import_model_config_non_openai_kwarg_raises() -> None: + """ImportModelConfig with non-openai_* kwarg should raise TypeError.""" + from llmling_models_config import ImportModelConfig + + config = ImportModelConfig( + model="agentpool.models.openai_compatible.OpenAICompatibleModel", + kw_args={ + "model_name": "test", + "foo_bar": "baz", + }, + ) + with pytest.raises(TypeError, match="Unexpected keyword argument"): + config.get_model() + + +# --------------------------------------------------------------------------- +# Helper: AsyncIteratorMock +# --------------------------------------------------------------------------- + + +# Import at module level for patch target +from pydantic_ai.models.openai import OpenAIChatModel # noqa: E402 + + +class AsyncIteratorMock: + """Mock that acts as an async iterator yielding provided items.""" + + def __init__(self, items: list[Any]) -> None: + self._items = items + self._index = 0 + + def __aiter__(self) -> AsyncIteratorMock: + return self + + async def __anext__(self) -> Any: + if self._index >= len(self._items): + raise StopAsyncIteration + item = self._items[self._index] + self._index += 1 + return item diff --git a/tests/orchestrator/test_child_done_events.py b/tests/orchestrator/test_child_done_events.py index 560def575..87933391c 100644 --- a/tests/orchestrator/test_child_done_events.py +++ b/tests/orchestrator/test_child_done_events.py @@ -224,7 +224,8 @@ async def _consume() -> None: consumer_task = asyncio.create_task(_consume()) # Wait for: first turn (instant) + 50ms timeout + second turn (instant). - await asyncio.sleep(0.15) + # Use generous sleep to avoid flakiness on slow CI runners. + await asyncio.sleep(0.3) # Close to unblock idle. handle.close() diff --git a/tests/orchestrator/test_subagent_events.py b/tests/orchestrator/test_subagent_events.py index 266d7ee93..71b91e678 100644 --- a/tests/orchestrator/test_subagent_events.py +++ b/tests/orchestrator/test_subagent_events.py @@ -564,6 +564,12 @@ async def test_nested_subagents_create_recursive_toolparts() -> None: # But actually, the parent consumer subscribes with descendants scope to parent_id, # and the spawn event for child would be published on parent_id by the agent runtime. # Let's publish on parent_id. + # Wait for parent consumer to initialize before publishing depth=2 event. + for _ in range(50): + if parent_id in integration._contexts: + break + await asyncio.sleep(0.01) + await asyncio.sleep(0.05) await _publish_spawn_event( session_pool, parent_id, child_id, source_name="worker2", depth=2 )