refactor(providers): run Claude through Pydantic AI - #355
Conversation
Implement Phase 5 event-mapping bridge for the Pydantic AI provider:
- map_pydantic_event() translates pydantic-ai streaming events into
Conductor payloads: agent_message, agent_reasoning, agent_tool_start,
agent_tool_complete.
- emit_pydantic_event() forwards mapped events through the Conductor
event callback, swallowing subscriber errors to protect the agent loop.
- emit_agent_turn_start() synthesizes agent_turn_start events with
{turn: N} for agentic-loop iterations and {turn: awaiting_model}
before each model API call, matching ClaudeProvider parity.
- maybe_emit_tool_truncation() detects MCPManager truncation markers on
tool results and emits agent_tool_output_truncated with the same
keys used by ClaudeProvider.
Add 23 unit tests covering text/reasoning/tool-call/tool-result mapping,
unknown events, callback error swallowing, turn synthesis, truncation
passthrough, and unnamed-agent fallback.
Verification:
- uv run pytest tests/test_providers/test_pydantic_ai_events.py -v: 23 passed
- uv run ruff check src/conductor/providers/_pydantic_ai/events.py tests/test_providers/test_pydantic_ai_events.py: passed
- uv run ty check src/conductor/providers/_pydantic_ai/events.py tests/test_providers/test_pydantic_ai_events.py: passed
Post-process the pydantic-ai result: dump the BaseModel to a dict and re-validate via Conductor's validate_output(), mirroring ClaudeProvider behavior. Adds a text fallback to JSON parsing. - 8 unit tests cover ToolOutput default, round-trip, validation errors and fallback - All 87 Pydantic AI provider tests pass
Add reusable execute_with_retry wrapper in _pydantic_ai/retry.py that mirrors ClaudeProvider retry semantics: error classification, backoff, retry-after header, retry_on filter, and agent_retry event payload. Add tests pinning Pydantic AI retries=0 and covering retryable/fatal/exhausted/validation-error cases.
…run helper Extend run_with_interrupt() with keyword-only usage_limits and max_session_seconds parameters, forwarding usage_limits to agent.iter() and agent.run(). - max_session_seconds is enforced at the start of each agentic iteration, matching the legacy ClaudeProvider loop semantics; on expiry a non-retryable ProviderError is raised. - UsageLimitExceeded from pydantic-ai is mapped to a non-retryable ProviderError with the legacy max-iterations message and suggestion. - Add tests for usage_limits, max_session_seconds, and None/None regression.
… seams Re-point legacy Claude provider tests away from AsyncAnthropic to the Pydantic AI build_agent/TestModel seam. Preserve public-contract assertions (system prompt forwarding, raw mode wrapping, tool exclusion, concurrency, memory, coexistence, edge cases). Remove obsolete parse-recovery / in-flight-interrupt tests that covered deleted legacy loop internals; add one-line justification comments where dropped. No source changes; all mocks are hermetic and require no network.
- test_claude.py: redirect public-contract tests (BasicExecution, StructuredOutput, ErrorHandling, ConcurrentExecution, DialogTurn, GetMaxPromptTokens, GetModelCapabilities, MCPManagerPool) to the build_agent / TestModel seam. Delete obsolete internal tests covering removed helpers: _build_tools_for_structured_output, _build_json_schema_properties, _execute_api_call, _process_response_content_blocks, _extract_token_usage, multi-turn parse recovery, retry delay/backoff/header math, and reasoning effort internals. - test_claude_parameters.py: keep empty prompt / special characters / null context public-behavior tests and convert to the build_agent + TestModel seam. Delete parse-recovery and retryable-error classification tests. - test_claude_parameter_passing.py: keep factory parameter-forwarding test and convert SDK-call assertions to build_agent default assertions. No source files modified. Category C regressions: none.
Remove unused TextPart, UserPromptPart, and ReasoningConfig imports. Reformat four long method signatures in get_max_prompt_tokens and get_model_capabilities tests to stay within the 100-character limit.
Replace legacy AsyncAnthropic/messages.create mocks with the Pydantic AI TestModel seam by patching build_agent. Keep WorkflowEngine-level coverage; assert runtime temperature and max_tokens reach Agent.model_settings via build_agent kwargs. Drive retry/auth errors through mock Agent.run exceptions recognized by the retry classifier.
…n Pydantic AI pipeline - Construct AsyncAnthropic explicitly with max_retries=0 so Conductor-level execute_with_retry is the only retry layer. - Forward auth_token and timeout through build_agent/_resolve_anthropic_model to the Anthropic SDK client. - Relax credential check: accept auth_token-only authentication (gateway / LiteLLM path), while still rejecting completely missing credentials. - Remove non-existent pydantic-ai[anthropic] extra; depend on pydantic-ai>=1.44.0. - Add tests pinning max_retries=0, auth_token/timeout forwarding, and no-credentials ValidationError.
The retry.max_parse_recovery_attempts YAML field (and provider-level default) is still accepted for configuration compatibility but no longer has any effect on the Claude provider. Structured output is enforced via pydantic-ai ToolOutput and the in-prose JSON fallback is single-shot. Copilot still honors the field.
…overy Passing retries=0 as a bare int zeroed both the tool and the output retry budgets in pydantic-ai v2. With the output budget at zero, a plain-text answer to a ToolOutput schema raised UnexpectedModelBehavior on the first attempt: the OutputValidator call (and any structured agent on a text-prone model) failed before Conductor could see a result, and the validator silently fell back to fail-open pass. Split the budgets in build_agent: tools=0 keeps execute_with_retry as the sole API/tool retry layer, while output=2 restores the legacy parse-recovery semantics (RetryConfig.max_parse_recovery_attempts=2): pydantic-ai reprompts the model to call the output tool instead of failing immediately. Add regression tests covering recovery via output retry (text answer, then tool call) and the immediate UnexpectedModelBehavior when the output budget is zero.
…tract The fixture constructed ClaudeProvider via __new__ and hand-populated a partial attribute list, which drifted from the rewritten provider: execute() reads self._auth_token and self._timeout, neither of which the fixture set, so all three TestMcpToolsReachApiInWorkflow tests failed with AttributeError. Construct the provider through its real __init__ (no network calls, the AsyncAnthropic client is immediately replaced with a MagicMock) and override only the seams the tests need (_client, the pre-wired MCP manager pool, _tool_output_config). The fixture now follows the provider's attribute contract automatically when the implementation changes.
Exhausting Pydantic AI's structured-output recovery budget raised UnexpectedModelBehavior, which was classified as fatal, so Conductor-level retries never fired and the workflow died with a misleading "Check API key" suggestion; now it is retryable and the exhausted-attempts suggestion points at the structured output cause.
93c37c2 to
672b2bc
Compare
jrob5756
left a comment
There was a problem hiding this comment.
Reviewed the Claude/Pydantic AI refactor. Overall this is a clean split of responsibilities across the new _pydantic_ai package, and the test suite is much better organized than the old monolithic test_claude.py. I found three issues worth blocking on (mainly the structured-output recovery contract from issue #343 no longer holding), plus some smaller cleanup items inline. Happy to discuss any of these.
| else: | ||
| suggestion = f"Check API connectivity and rate limits. Last error: {last_error}" | ||
|
|
||
| raise ProviderError( |
There was a problem hiding this comment.
This breaks the parity contract from issue #343 (see AGENTS.md): once the retry budget is exhausted on a structured-output failure, we're supposed to re-raise the original ValidationError (it names the field and expected type) and reserve ProviderError for syntax failures.
Here UnexpectedModelBehavior is treated as retryable and, once exhausted, gets wrapped in a generic ProviderError with a vague "output tool not called" message. But pydantic-ai actually preserves the real ValidationError as __cause__ on that exception - we're just not looking at it. A workflow author with a schema mismatch now gets "retry later or simplify the schema" instead of the actual field/type error, and it's misleading since retrying won't fix a shape mismatch.
Roughly:
from pydantic import ValidationError as PydanticValidationError
cause = getattr(last_error, "__cause__", None)
if isinstance(cause, PydanticValidationError):
raise ValidationError(str(cause)) from causebefore falling through to the existing ProviderError branch for everything else.
There was a problem hiding this comment.
Fixed. On retry-budget exhaustion, when UnexpectedModelBehavior carries a pydantic ValidationError as __cause__, we now re-raise it as Conductor ValidationError (naming the field and expected type) via raise ... from cause, so the original error stays available for introspection. ProviderError with the "output tool not called" suggestion is kept only for the case where no schema-validation cause is preserved. A new test asserts both the message content and __cause__ is schema_error.
| interrupt_message = _make_interrupt_message(has_output_schema) | ||
| partial_history = list(history) | ||
| partial_history.append(ModelRequest(parts=[interrupt_message])) | ||
| result = await agent.run(user_prompt=None, message_history=partial_history) |
There was a problem hiding this comment.
The docstring above says "tools are not available on this run so the model must answer directly," but agent.run here reuses the same agent built with toolsets= and a ToolOutput output type - nothing overrides either for this call. Pydantic AI's run() supports passing toolsets=[] / output_type=str per-call, which would actually enforce what the docstring claims.
As written, an interrupted structured-output agent can still try to call tools or get stuck on ToolOutput validation retries on this "final" call, and could raise UnexpectedModelBehavior instead of returning a partial result - which somewhat defeats the point of this code path.
There was a problem hiding this comment.
Good catch — enforced rather than documented. The partial-result call now runs inside agent.override(toolsets=[], tools=[]) with output_type=str. One pydantic-ai subtlety worth noting: the toolsets= run kwarg is additive in 2.18 (it never clears construction-time toolsets — verified empirically that an MCP-style toolset stays callable through run(toolsets=[])), so agent.override is the only mechanism that actually removes them. Since output_type=str removes the output tool from the request, the structured interrupt prompt was also reworded from "call the final_result tool" to "respond with plain text; do not call any tools" — asking for a tool that no longer exists would just produce a failed tool call instead of a partial result. Trade-off rationale: this path is an escape hatch whose contract is best-effort delivery after Esc; partial outputs are never schema-validated downstream (_build_partial_content handles both str with lenient JSON extraction and BaseModel), so guaranteed delivery beats shape fidelity here. Covered by a semantic test where the scripted model attempts a tool call on the partial run and the tool's side effect must not happen.
There was a problem hiding this comment.
To be honest, I'm not sure if this is the right solution, but I don't have any other suggestions yet.
| if output_type is None: | ||
| output_type = str | ||
|
|
||
| model_settings = _build_model_settings( |
There was a problem hiding this comment.
_build_model_settings never gets default_model here, even though _resolve_anthropic_model a few lines up does. Inside _build_model_settings, model_name = (agent.model or None) or None (line 269) means the thinking-support check falls back to the hardcoded DEFAULT_ANTHROPIC_MODEL whenever agent.model is unset - not the workflow/provider's actual configured default.
That means a workflow relying on runtime.provider's default model together with default_reasoning_effort gets its thinking-budget validated against the wrong model, which can produce spurious ValidationErrors (or miss a real one) depending on which models support extended thinking.
| model_settings = _build_model_settings( | |
| model_settings = _build_model_settings( | |
| agent, | |
| default_temperature, | |
| default_max_tokens, | |
| default_reasoning_effort, | |
| default_model=default_model, | |
| timeout=timeout, | |
| ) |
There was a problem hiding this comment.
Fixed exactly as suggested: _build_model_settings now takes default_model and resolves (agent.model or default_model) or DEFAULT_ANTHROPIC_MODEL, with build_agent forwarding it. Two new tests cover both directions — a thinking-capable default model is accepted when agent.model is unset, and a non-thinking default raises ValidationError instead of silently validating against the library default.
| ValidationError: If extracted JSON fails schema validation. | ||
| """ | ||
| try: | ||
| content = parse_json_output(text) |
There was a problem hiding this comment.
This path parses and validates directly, but skips unwrap_scalar_wrappers/normalize_agent_output from providers/_output_shape.py, which copilot.py and hermes.py both apply before validating. That's the step that recovers single-field wrapper shapes like {"value": ...} or {"result": ...}.
Might be low-impact in practice since _build_output_type wraps schemas in ToolOutput, so this fallback may rarely get hit - but worth confirming, and if it can be reached, it should go through the same normalization the other providers use for parity.
There was a problem hiding this comment.
Fixed — applied normalize_agent_output before validate_output in both places where provider output is validated: parse_text_fallback (your comment target) and the BaseModel path in extract_content, which had the same gap for test-constructed/edge outputs. On reachability: you're right that ToolOutput rejects wrapper shapes upstream most of the time, but the fallback is reachable whenever the model answers a tool-output schema with plain text containing JSON, so parity is worth having. Added tests for both paths (wrapper object unwrapped to the scalar in each).
| return None | ||
| return {"result": partial_output} | ||
|
|
||
| def _coerce_for_thinking( |
There was a problem hiding this comment.
This method looks unused - execute() and execute_dialog_turn() call the module-level _coerce_for_thinking imported from agent_builder.py instead (see line 699), not self._coerce_for_thinking. Looks like leftover from the refactor. If it's truly dead, removing it would also let you drop the now-unnecessary CLAUDE_ANSWER_HEADROOM_TOKENS/CLAUDE_EXTENDED_THINKING_OUTPUT_CAP imports at the top of the file (lines 42-43).
There was a problem hiding this comment.
Confirmed dead — removed the method along with the CLAUDE_ANSWER_HEADROOM_TOKENS/CLAUDE_EXTENDED_THINKING_OUTPUT_CAP imports (verified no remaining references; the live call site at the dialog-turn path uses the module-level _coerce_for_thinking from agent_builder.py).
| event_callback=intercepting_callback, | ||
| agent_name=agent.name, | ||
| ) | ||
| finally: |
There was a problem hiding this comment.
| finally: |
No-op finally: pass - looks like refactor debris, safe to remove.
There was a problem hiding this comment.
Removed — the try/finally: pass is gone and execute_with_retry is awaited directly (the finally block was empty, so no cleanup semantics changed).
| logger = logging.getLogger(__name__) | ||
|
|
||
| EventCallback = Callable[[str, dict[str, Any]], None] | ||
|
|
There was a problem hiding this comment.
EventCallback is defined twice in a row here (identical to line 30). Probably a merge artifact - one can be dropped.
There was a problem hiding this comment.
Dropped the duplicate definition.
| try: | ||
| result = await self._manager.call_tool(name, tool_args) | ||
| except Exception as e: | ||
| raise ToolFailed(f"Error executing tool {name!r}: {e}") from e |
There was a problem hiding this comment.
This re-raises as ToolFailed so the model sees the error, which matches the old provider's behavior - but there's no Conductor-side log here. A genuine adapter bug (vs. a legitimate tool-level failure) will only ever show up in the model's transcript, which most subscribers/dashboards don't surface. Worth a logger.warning before re-raising so operators can tell the two apart without digging through transcripts.
There was a problem hiding this comment.
Added logger.warning("MCP tool call %r failed: %s: %s", name, type(e).__name__, e) immediately before the ToolFailed raise, so operator logs distinguish adapter/tool failures from model behavior without digging through transcripts.
| its own default. | ||
| toolsets: Optional Pydantic AI toolsets to register. Used by Phase 4 to | ||
| inject MCP toolsets without changing this signature. | ||
| tools: Optional plain Pydantic AI tools to register. Also reserved for |
There was a problem hiding this comment.
"Reserved for Phase 4" is already inaccurate - toolsets are wired up in this same PR (claude.py passes MCPManagerToolset into build_agent(toolsets=...)). Also worth dropping the internal phase reference and just describing the current contract, e.g. "Pydantic AI toolsets to register (e.g. the MCP tool bridge)."
There was a problem hiding this comment.
Reworded — no more phase references: "Optional Pydantic AI toolsets to register (e.g. the MCP tool bridge)" / "Optional plain Pydantic AI tools to register."
…vider - retry: re-raise the original pydantic ValidationError preserved as __cause__ on UnexpectedModelBehavior once the retry budget is exhausted on a structured-output failure, keeping ProviderError for syntax failures (issue microsoft#343 parity contract); drop the duplicated EventCallback definition. - interrupt: enforce the documented hard cut on the partial-result call via agent.override(toolsets=[], tools=[]) plus output_type=str; the toolsets= run kwarg is additive in pydantic-ai and cannot clear construction-time toolsets. Reword the structured interrupt prompt to ask for plain text instead of the final_result tool, which no longer exists on that run. - agent_builder: forward default_model into _build_model_settings so the extended-thinking support check validates against the model the run will actually use instead of the hardcoded library default; remove stale "Phase 4" docstring references. - structured_output: run normalize_agent_output before validate_output in both extract_content and parse_text_fallback, matching the copilot/hermes issue microsoft#343 normalization contract. - mcp_toolset: log a warning before re-raising MCP call failures as ToolFailed so adapter bugs are visible outside model transcripts. - claude: remove the dead _coerce_for_thinking method, its now-unused imports, and the no-op finally: pass.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #355 +/- ##
=======================================
Coverage ? 90.74%
=======================================
Files ? 93
Lines ? 14570
Branches ? 0
=======================================
Hits ? 13222
Misses ? 1348
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
This PR replaces the hand-written inner agent loop in
ClaudeProviderwith a Pydantic AI-based runtime while preserving Conductor's public provider contract and workflow-level behavior.It is primarily intended as a concrete way to move the architectural discussion in #315 forward. It is not presented with an expectation that it should be merged as-is. Whether this direction belongs in Conductor, and whether this implementation should be taken further, is entirely up to the maintainers.
What changed
AgentProvider,AgentOutput, capability declarations, Conductor events, retries, interrupts, usage accounting, MCP policy, and output validation at the Conductor boundary;Practical validation
I have spent several days using builds from this branch on real Conductor workflows and fixed the defects I encountered along the way. Apart from the issue described below, it has worked without problems for my tasks. Those tasks do not cover Conductor's complete feature surface, so this should not be read as exhaustive production validation.
Known issue
One remaining defect should be fixed separately: #353. MCP connections can be opened in a worker task and later closed from the root task, which conflicts with AnyIO cancel-scope task affinity. The problem is in the shared
MCPManagerlifecycle and is not specific to Pydantic AI, so I deliberately did not expand this PR to address it.Skills follow-up
I also noticed that skills capabilities were added recently in #215. The Pydantic AI harness offers an opportunity to support this somewhat better than the current eager-preamble implementation. I did not expand the scope again to redesign that integration, but I can follow up with a separate PR if the maintainers are interested.
Verification
make checkmake test: 4556 passed; 27 environment/optional-integration tests skipped; 8 install-script tests deselectedRelated: #315, #353, #215