Uh oh!
There was an error while loading. Please reload this page.
feat(providers): add native OpenAI provider on Pydantic AI - #421
Conversation
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Thanks for another great contribution!
The shared-runner extraction itself checks out. I diffed the pre-PR Claude execute() against claude.py plus runner.py line by line and the behaviour is preserved, the factory drops nothing, and event parity, last_call_input_tokens, cache-inclusive tokens and structured-output recovery are all inherited rather than reimplemented. That is the payoff #315 predicted, and it is the right shape.
Four things stand out as blocking:
- Ambient
OPENAI_API_KEYreaches a custombase_url, and the guard meant to stop it is unreachable. reasoning.effortis never checked against the model, which an explicit parity rule requires.agent_reasoning_events=Truecannot be honoured on Chat Completions.runtime.temperatureabove 1.0 is no longer rejected on therunpath.
Two more that are not inline anywhere:
AGENTS.md lost about a thousand characters of unrelated content. The Run/Resume Parity note now stops at "hard-stops a healthy workflow." Both the merge base and origin/main still carry the tail covering the dialog_message rationale, the replayMode latching in Header.tsx and use-replay.ts, and the EventLogSubscriber append-mode note. It disappears inside this branch's own history, so it reads as a rebase artifact, but merging as-is silently reverts documentation for behaviour this PR does not touch.
tier="stable" has no real-API test. The repo's own promotion criteria ask for one, and there is a real_api marker plus a Claude precedent to mirror. Shipping as experimental first would also be a fair option, given the descriptor issues above.
On naming: openai is the right choice and matches claude in naming the model API rather than the vendor. The thing to fix is openai-agents sitting next to it. See the comment on config/schema.py.
| effective_api_key = self._api_key | ||
| if effective_api_key is None: | ||
| effective_api_key = os.environ.get("OPENAI_API_KEY") | ||
| if not effective_api_key: | ||
| raise ValidationError( | ||
| "OPENAI_API_KEY environment variable is not set and no api_key was provided", | ||
| suggestion="Set OPENAI_API_KEY or pass api_key to the provider.", | ||
| ) |
There was a problem hiding this comment.
_initialize_client reads the ambient OPENAI_API_KEY even when a custom base_url is configured, so a user's real OpenAI key is sent to whatever endpoint the workflow points at. Exporting OPENAI_BASE_URL alone is enough to trigger it, with no YAML opt-in.
Two knock-on effects. The docstring at line 170 promises the opposite behaviour. And because line 254 writes the resolved key back to self._api_key, the guard in agent_builder.py:110-114 can never fire — the test covering it calls build_agent directly, so it passes while the shipped provider does the reverse.
copilot.py refuses this fallback on purpose, and docs/configuration.md:164 explains why.
| effective_api_key=self._api_key | |
| ifeffective_api_keyisNone: | |
| effective_api_key=os.environ.get("OPENAI_API_KEY") | |
| ifnoteffective_api_key: | |
| raiseValidationError( | |
| "OPENAI_API_KEY environment variable is not set and no api_key was provided", | |
| suggestion="Set OPENAI_API_KEY or pass api_key to the provider.", | |
| ) | |
| effective_api_key=self._api_key | |
| ifeffective_api_keyisNoneandself._base_urlisNone: | |
| effective_api_key=os.environ.get("OPENAI_API_KEY") | |
| ifnoteffective_api_key: | |
| ifself._base_urlisnotNone: | |
| raiseValidationError( | |
| "A custom base_url requires an explicit api_key", | |
| suggestion=( | |
| "Set runtime.provider.api_key in YAML. Conductor will not " | |
| "forward an ambient OPENAI_API_KEY to a non-OpenAI endpoint." | |
| ), | |
| ) | |
| raiseValidationError( | |
| "OPENAI_API_KEY environment variable is not set and no api_key was provided", | |
| suggestion="Set OPENAI_API_KEY or pass api_key to the provider.", | |
| ) |
There was a problem hiding this comment.
Fixed. _initialize_client no longer falls back to the ambient OPENAI_API_KEY when a custom base_url is set — that combination now raises ValidationError at construction, and the resolved key is never written back to self._api_key, so the agent_builder.py guard is reachable in production, not just in its direct unit test. When no custom base_url is configured the ambient key is still honored (the zero-YAML default-endpoint path). The __init__ docstring now states the rule instead of contradicting it. Covered by new tests in test_openai.py (custom base_url + ambient key raises; explicit key + base_url works; ambient-only keeps _api_key is None).
| streaming_events=True, | ||
| # Reasoning content is surfaced as ``agent_reasoning`` events when the model | ||
| # returns it. | ||
| agent_reasoning_events=True, |
There was a problem hiding this comment.
OpenAIChatModel speaks Chat Completions, and pydantic-ai only builds a ThinkingPart from reasoning / reasoning_content, which its own source notes is a DeepSeek and Moonshot field. OpenAI returns reasoning summaries through the Responses API only, so against api.openai.com this event never fires. The claim holds only for third-party proxies that echo reasoning_content.
capabilities.py:67 asks for the weaker value when a claim cannot be honoured under all conditions, and tier="stable" makes that stricter rather than looser.
| agent_reasoning_events=True, | |
| # Chat Completions never returns reasoning content. This becomes True | |
| # only on an OpenAIResponsesModel backend. | |
| agent_reasoning_events=False, |
There was a problem hiding this comment.
Fixed — agent_reasoning_events=False with a comment explaining that Chat Completions never returns reasoning content from api.openai.com (only third-party proxies echoing reasoning_content would surface it), and that this flips to True only on an OpenAIResponsesModel backend. Test updated to assert the weaker value.
| # returns it. | ||
| agent_reasoning_events=True, | ||
| # OpenAI supports reasoning_effort on its reasoning models (o-series, etc.). | ||
| reasoning_effort=("low", "medium", "high", "xhigh"), |
There was a problem hiding this comment.
o1, o1-mini, o3, o3-mini and o4-mini accept low, medium and high only. xhigh arrived with the GPT-5.1-Codex-Max generation, so the two models named in docs/providers/openai.md:206 and AGENTS.md:370 are precisely the ones that will 400 on it.
hermes.py handles the same uncertainty well: it declares the narrower tuple and spends a comment explaining that upstream support is unverified.
There was a problem hiding this comment.
Fixed — the tuple is now ("low", "medium", "high") with a hermes-style comment explaining that xhigh arrived with the GPT-5.1-Codex-Max generation and that o1/o3-mini/o4-mini accept only low/medium/high. docs/providers/openai.md's effort matrix now marks xhigh as rejected, and the provider-level tuple tests assert the narrower set.
| if timeout is not None: | ||
| settings["timeout"] = timeout | ||
| if effort is not None: | ||
| settings["openai_reasoning_effort"] = effort |
There was a problem hiding this comment.
AGENTS.md requires every provider to validate the requested effort against the selected model and raise ValidationError when the model does not support it. execute() only checks membership in CAPABILITIES.reasoning_effort, which is a provider-level claim, so openai_reasoning_effort gets set for any model at all.
With gpt-4o this reaches the API, returns a 400, is classified non-retryable in retry.py:133, and lands on the user mid-run as Pydantic AI provider error: ... rather than a config error caught before anything ran. _resolve_anthropic_thinking a few hundred lines up is the shape to copy. Sharing the model check with get_model_capabilities would stop the two drifting.
There was a problem hiding this comment.
Fixed — _build_openai_model_settings now validates the resolved effort against the model using pydantic_ai.profiles.openai.openai_model_profile(...).openai_supports_reasoning via a shared _openai_model_supports_reasoning helper (returns None on older pydantic-ai so unknown means skip, never guess). A non-reasoning model (e.g. gpt-4o) with an effort configured raises ValidationError at agent-build time, mirroring _resolve_anthropic_thinking. The same helper is reused by get_model_capabilities and execute_dialog_turn. Tests cover gpt-4o rejection, o3-mini acceptance, and the default-model resolution path.
| if max_tokens is not None: | ||
| self._validate_max_tokens(max_tokens) | ||
| self._default_max_tokens = max_tokens or 4096 |
There was a problem hiding this comment.
pydantic-ai maps max_tokens onto max_completion_tokens, which on reasoning models counts reasoning tokens as well as output. The default model here is gpt-5-mini, so an unconfigured workflow can spend the whole 4096 on reasoning and come back with empty content. That surfaces as UnexpectedModelBehavior, which the retry layer treats as retryable, so it exhausts the budget and finishes on the misleading "Model repeatedly failed to produce valid structured output".
Anthropic requires an output cap. OpenAI does not, so there is no need to invent one.
| self._default_max_tokens=max_tokensor4096 | |
| self._default_max_tokens=max_tokens |
There was a problem hiding this comment.
Fixed — self._default_max_tokens = max_tokens (no invented 4096 cap). The docstring now states that None leaves the parameter unset so the server applies its own default. The dialog path keeps its explicit 4096 cap, which is intentional: dialog turns are short text replies.
| ProviderType = Literal[ | ||
| "copilot", | ||
| "openai", | ||
| "openai-agents", |
There was a problem hiding this comment.
Same removal as config/schema.py:2547. While the unions are being touched, they have drifted from each other: AgentDef.provider and ProviderSettings.name differ by {aca, openai-agents}, and this PR widens registry.ProviderType further. One shared alias would keep them honest.
If the member has to stay for now, the branch at line 143 should stop recommending copilot, since the accurate answer is the provider this PR adds.
There was a problem hiding this comment.
Done — all four provider-name Literals now share one alias, ProviderName, defined in config/schema.py and imported by factory.py and registry.py (schema imports nothing from providers, so no cycle). AgentDef.provider and ProviderSettings.name use the same alias, which also fixes the drift you noted (AgentDef.provider was missing aca). The factory's case _ suggestion no longer recommends copilot for a removed name; it lists the actual set. Tests asserting the old not yet implemented behavior now assert the schema rejection.
| "anthropic>=0.77.0,<1.0.0", | ||
| "pydantic-ai>=1.44.0", | ||
| "pydantic-ai-slim[anthropic,openai]>=1.44.0", | ||
| "openai>=2.48.0,<2.49.0", |
There was a problem hiding this comment.
A one-minor window is far tighter than every sibling pin here (anthropic>=0.77.0,<1.0.0, mcp>=1.28.1), and it will block patch releases and conflict with anything else in the environment wanting a newer openai. Nothing in the code requires <2.49; 2.48 is only the floor for xhigh in ReasoningEffort.
The line above is a package swap rather than an added extra: pydantic-ai to pydantic-ai-slim drops pydantic_ai.mcp. That is harmless because Conductor ships its own toolset, but it belongs in the CHANGELOG.
| "openai>=2.48.0,<2.49.0", | |
| "openai>=2.48.0,<3.0.0", |
There was a problem hiding this comment.
Done — openai>=2.48.0,<3.0.0, and uv.lock regenerated. The pydantic-ai → pydantic-ai-slim[anthropic,openai] swap is now documented in the CHANGELOG entry.
| "authenticates via `claude login`; ANTHROPIC_API_KEY is an optional override" | ||
| ), | ||
| ), | ||
| "openai": _CredentialSpec(env_vars=("OPENAI_API_KEY", "OPENAI_BASE_URL")), |
There was a problem hiding this comment.
With optional_auth_note left at None, the dataclass docstring says the provider genuinely requires one of these env vars, so cli/doctor.py:262 renders an unset OPENAI_BASE_URL as a red cross for every user on the standard endpoint. The claude entry above lists only real credentials and deliberately leaves ANTHROPIC_BASE_URL out.
| "openai": _CredentialSpec(env_vars=("OPENAI_API_KEY","OPENAI_BASE_URL")), | |
| "openai": _CredentialSpec(env_vars=("OPENAI_API_KEY",)), |
There was a problem hiding this comment.
Fixed — "openai": _CredentialSpec(env_vars=("OPENAI_API_KEY",)). An unset OPENAI_BASE_URL no longer renders as a red cross for standard-endpoint users. The diagnostics test asserting the old pair is updated.
| if isinstance(exception, ProviderError): | ||
| return exception.is_retryable | ||
| if ModelHTTPError is not None and isinstance(exception, ModelHTTPError): |
There was a problem hiding this comment.
pydantic-ai wraps every Anthropic APIStatusError at 400 or above into ModelHTTPError, so this branch is on Claude's hot path too. Against the merge base, 429 and 5xx flip from fatal to retryable. That is a genuine fix, but it changes an existing provider's behaviour with no CHANGELOG entry and no note in the parity section.
The deny-list shape also makes 409, 413, 422 and 451 retryable, and none of those recover, so they now burn the full budget with backoff. An allow-list of 5xx plus 429 and 408 says what you mean.
Worth pairing with _get_retry_after, which returns None for ModelHTTPError, so a 429 Retry-After header is ignored on both providers.
There was a problem hiding this comment.
Mostly addressed by the rebase — while this PR was in flight, origin/main landed #458 which rewrote _is_retryable_error to the allow-list you describe (429 or 500 <= code < 600 for ModelHTTPError). Our branch's deny-list hunk was dropped in the conflict resolution. On top of that we added 408 to the allow-list and an explicit openai.APIStatusError isinstance arm, since the OpenAI SDK's errors reach the shared runner untranslated on this path. CHANGELOG now notes the retry-classification change. Fair point on _get_retry_after returning None for ModelHTTPError — left as-is since that's pre-existing on main and deserves its own issue.
| ```bash | ||
| # Using uv (recommended) | ||
| uv add 'openai>=1.0.0' |
There was a problem hiding this comment.
openai is a core dependency pinned at >=2.48.0,<2.49.0 in pyproject.toml, so this asks for a version two majors below the project's own floor and installs something the reader already has.
The matching troubleshooting entry at line 245 describes a state that cannot occur either: agent_builder.py imports openai and pydantic_ai.models.openai at module scope, so a missing SDK raises ImportError well before the friendly ProviderError can run. Contrast claude-agent-sdk and aca, which are real extras where that guard earns its place.
There was a problem hiding this comment.
Fixed — the "Install the OpenAI SDK" quick-start step and the matching troubleshooting entry are deleted; openai is a core dependency pinned at >=2.48.0, and the module-scope import in agent_builder.py means a missing SDK raises ImportError long before the friendly ProviderError. Quick Start is renumbered.
Drive the real OpenAIChatModel + AsyncOpenAI through run_agent_pipeline with httpx.MockTransport. Covers success usage mapping, 400 non-retryable (one request), and 429 retryable then success (two requests).
…gration Add todo-11 test coverage for the openai provider: - factory: string + structured construction, YAML api_key precedence over env, missing API key raises ValidationError naming OPENAI_API_KEY - validate-level: runtime/provider: openai, per-agent provider: openai, reasoning.effort: max static rejection, tools allowlist, mcp_servers, skills - retry: existing openai.RateLimitError/BadRequestError tests already cover the requirement; no new code needed - SecretStr redaction for openai ProviderSettings - temperature: schema-level 1.5 valid / 2.5 invalid, validator-level mixed default=openai + per-agent/for_each inline claude override fails naming temperature and claude Also exclude openai-named example files from the copilot backward-compatibility helper so the existing copilot-only assertion does not trip over examples/openai-compatible.yaml. Verification: uv run pytest tests/test_config tests/test_providers/test_factory.py tests/test_providers/test_pydantic_ai_retry.py -x (1253 passed, 9 skipped); make check clean.
… OPENAI_BASE_URL env fallback
Address every review comment on microsoft#421: - openai.py: never forward an ambient OPENAI_API_KEY to a custom base_url; the env-resolved key no longer overwrites self._api_key, so the agent_builder guard against custom endpoints without an explicit key is reachable in production, not just in direct unit tests. - openai.py: declare agent_reasoning_events=False — Chat Completions never returns reasoning content from api.openai.com; only DeepSeek/Moonshot-style proxies echo reasoning_content. - openai.py: narrow reasoning_effort to low/medium/high; xhigh arrived with the GPT-5.1-Codex-Max generation and the o-series models named in the docs (o1, o3-mini, o4-mini) 400 on it. - agent_builder.py: validate reasoning effort against the resolved model via pydantic-ai's model profile (shared helper _openai_model_supports_reasoning), mirroring the Anthropic thinking check, instead of trusting the provider-level tuple for every model. - openai.py: drop the invented default max_tokens=4096; an output cap on a reasoning model can be entirely consumed by reasoning tokens and surfaces as a misleading structured-output retry failure. - openai.py: validate_connection now mirrors ClaudeProvider's inconclusive-probe pattern (401/403/connection errors fail, other HTTP statuses warn and proceed with _connection_probe_note), and reports a mistyped default model against the fetched model list. - openai.py: get_model_capabilities no longer uses the startswith("o") heuristic (it misclassified the provider's own default gpt-5-mini and any "openai/..."-prefixed OpenRouter id); it consults the pydantic-ai model profile and returns None for unknowns. - schema/factory/registry: remove the never-implemented openai-agents provider name and unify the drifted provider-name Literals behind a single ProviderName alias in config/schema.py. - Temperature ceilings are now capability-driven: ProviderCapabilities gains max_temperature (1.0 for copilot/claude/hermes/aca/ claude-agent-sdk), the static validator reads it instead of hardcoding provider names, and create_provider enforces it so run/resume are covered too, restoring the 1.1-for-claude rejection test. - diagnostics.py: drop OPENAI_BASE_URL from openai's credential spec — an unset base URL must not render as a missing credential. - pyproject.toml: widen the openai pin to >=2.48.0,<3.0.0. - docs/providers/openai.md: remove the unreachable "install the SDK" instructions and troubleshooting entry, correct the reasoning-effort matrix (xhigh rejected), and renumber Quick Start. - CHANGELOG: note the pydantic-ai -> pydantic-ai-slim dependency swap, the openai-agents removal, and the temperature enforcement move. - AGENTS.md: restore the Run/Resume Parity tail truncated in a rebase, and document the new temperature capability. - tests: new real-API integration test (tests/test_integration/ test_openai_real_api.py, pytest.mark.real_api) exercised against an OpenAI-compatible endpoint; satisfies the stable-tier real-API bar. Co-Authored-By: Sisyphus (OhMyOpenCode) <noreply@ohmyopencode.com>
0eea251 to
a0aa16bComparehertznsk
commented
Aug 19, 2026
Thanks for the thorough review — every concern checked out against the code. All 14 inline threads are addressed in the replies above and in commit On the two non-inline items: AGENTS.md truncation — confirmed and fixed. The Run/Resume Parity bullet now ends with the full tail again (
One note on the retry comment: I did not extend |
… lint
Follow-up to the review round on the native OpenAI provider. No behaviour
changes; the provider code is untouched apart from two suggestion strings.
- Run `ruff format` on the three files CI flagged. The formatter check was
failing, which skipped every downstream job, so no test run had executed
against this branch head.
- Move the OpenAI provider CHANGELOG entry out of the released `[0.1.28]`
section and into `[Unreleased]`, where it belongs. A rebase had carried it
down as 0.1.29-0.1.33 were inserted above it, so it was editing the notes of
a shipped release while the version that will actually carry the feature had
no entry at all. Also record the retry-classification change (408/429/5xx are
now retried on Claude as well) and the `default_reasoning_effort` forwarding
fix, both of which alter existing provider behaviour.
- Describe the credential rule the provider actually implements. The docs
promised `OPENAI_API_KEY` and `OPENAI_BASE_URL` both work as environment
fallbacks, but once a custom `base_url` is in effect the ambient key is
refused and construction raises. That is the correct behaviour, and it is the
configuration the recipes on the same page recommend, so the page needed to
say so rather than send readers into a `ValidationError`.
- Drop the remaining `xhigh` claims left over from narrowing the capability
tuple to `("low", "medium", "high")`, including an example comment that
contradicted the matrix ten lines above it.
- Correct the comparison table's context-window cell: `get_max_prompt_tokens`
returns `None` unconditionally, so the provider reports no window.
- Point the "install the SDK" suggestions at `openai>=2.48.0` instead of
`>=1.0.0`, two majors below the project's own floor.
- Rewrite the AGENTS.md provider notes so the env-resolution, reasoning-effort
and temperature bullets match the implementation, and record why
`agent_reasoning_events` is `False` on a Chat Completions backend.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>Resolves the CHANGELOG conflict: microsoft#420's MCP 2.0 entry and this branch's OpenAI provider entries both landed in the previously-empty [Unreleased] section, so git could not merge them. Both are kept, with the two Fixed items combined under a single heading. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
commented
Aug 22, 2026
Went back through all fourteen comments against The temperature work is the standout. Moving the ceiling onto The remaining items were small and almost entirely documentation, so rather than send it back around again I pushed them myself in
On the one open question, the
Locally on the merge result: Thanks for working through a long list carefully. Nothing further from me beyond the |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #421 +/- ##
=======================================
Coverage ? 91.73% =======================================
Files ? 146 Lines ? 23640 Branches ? 0 =======================================
Hits ? 21686 Misses ? 1954 Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM. Approved!
3d9b071
into
microsoft:mainUh oh!
There was an error while loading. Please reload this page.
Summary
This PR adds a native OpenAI-compatible provider backed by the shared Pydantic AI runtime introduced for Claude in #355. It also extracts the common Pydantic AI execution pipeline so the Claude and OpenAI adapters share Conductor-owned retry, interrupt, event, MCP, structured-output, usage, and partial-output behavior.
Together with the already merged #355, this closes#315 by demonstrating and shipping Pydantic AI as the shared inner runtime for direct Anthropic and OpenAI providers.
What changed
openaiprovider using Pydantic AI and the OpenAI Chat Completions API;OPENAI_API_KEY/OPENAI_BASE_URL;Practical validation
Builds from this branch have been used for an extended period on real working workflows. No shortcomings were identified during that use beyond the separately reported MCP 2.0 compatibility issue in #419. The issue is not specific to this provider implementation and is intentionally tracked separately.
Verification
origin/main;origin/main;make check;make test: 6295 passed, 39 skipped, 8 deselected;conductor --help;examples/openai-compatible.yamland a missing workflow path.Closes#315.
Related: #355, #419.