Skip to content

feat(llms): add OrcaRouter provider (supersedes #2275, with review fixes) - #2400

Open
JSv4 wants to merge 8 commits into
mainfrom
claude/pr-2275-review-ci-cd-11d96e
Open

JSv4 wants to merge 8 commits into
mainfrom
claude/pr-2275-review-ci-cd-11d96e

Conversation

@JSv4

@JSv4 JSv4 commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Carries the OrcaRouter provider from fork PR #2275 (original commit cherry-picked with its authorship intact) and fixes the blockers and smaller items from the maintainer review on that PR. The fork PR could not get CI beyond CLAAssistant: fork workflows need approval, and the CLA check fails because the commit email isn't linked to a GitHub account.

Note: #2275's CLA is still unsigned. This PR includes the contributor's original commit, so a maintainer needs to decide whether that's acceptable before merging.

Changes

  • Key leak fix. opencontractserver/llms/model_factory.py::_construct_orcarouter_model never passes api_key=None to OpenAIProvider. When it did, the OpenAI client fell back to OPENAI_API_KEY and sent the install's OpenAI secret to api.orcarouter.ai. With no key configured it now sends an inert placeholder (ORCAROUTER_API_KEY_PLACEHOLDER) and logs a static warning.
  • Context windows fetched from the gateway. opencontractserver/llms/orcarouter_context.py fetches OrcaRouter's GET {base_url}/models when the agent model is built and reads each model's context_length, context_window or max_context_length. The fetch is TTL-cached, has a 3s timeout, doesn't follow redirects, and never raises (including on httpx.InvalidURL). get_context_window_for_model serves orcarouter: specs from that cache with no I/O, and falls back to ORCAROUTER_FALLBACK_CONTEXT_WINDOW (64K) until a listing is available.
  • Picker models. supported_models is cut down to ("orcarouter/auto",). The eight speculative vendor/model names had no verified windows. Specific routed models can still be typed as a spec.
  • Build failures surface. A failure while building the OrcaRouter model is re-raised instead of degrading to the bare orcarouter: spec, which pydantic-ai can't resolve.
  • DRY. The duplicated base_url scheme check is now one helper, _is_valid_base_url().
  • Responses-API families. OrcaRouter only uses chat completions, so the factory warns when a routed model belongs to a Responses-API-only family (e.g. openai/gpt-5.6-luna).
  • Constants. ORCAROUTER_PROVIDER_KEY, ORCAROUTER_API_KEY_ENV_VAR and ORCAROUTER_API_KEY_PLACEHOLDER are in the provider module. The fallback window, cache TTL, fetch timeout and response keys are in constants/context_guardrails.py.
  • Docs. Updated the OrcaRouter notes in docs/architecture/llms/README.md, the provider count in docs/test_scripts/llm_runtime_config.md, and the changelog fragment.

Test plan

  • New opencontractserver/tests/test_orcarouter_context.py. Unit tests drive the fetch through a real httpx.Client over httpx.MockTransport. They cover parsing, sending the key only to /models, no redirects, TTL reuse and refetch, HTTP, network, malformed-URL and non-JSON failures, and fallback. An integration test builds an agent model and then checks the window lookup.
  • test_llm_model_factory.py covers the key-leak regression, DB-wins precedence, invalid base_url fallback, the Responses-family warning, build failures surfacing, the refresh being invoked with the resolved endpoint, and _is_valid_base_url.
  • Locally: the OrcaRouter, factory, runtime-config, retargeting, context-guardrail and history-processor suites give 256 passed. pre-commit reports no failures on the changed files. CI, including the full backend pytest, is green.

Checklist

  • Tests pass locally for any code this PR touches
  • pre-commit run passes on the changed files (black, isort, flake8, mypy)
  • TypeScript compiles cleanly — n/a, no frontend changes
  • A changelog fragment was added under changelog.d/
  • No new dependency — reuses pydantic-ai-slim[openai] and httpx

Contributor License Agreement

By submitting this pull request, you agree to license your contribution under the project's Contributor License Agreement.

XiaoHuo888-hue and others added 3 commits September 23, 2026 03:37
Adds an orcarouter: provider to the pipeline LLM provider registry
(opencontractserver/pipeline/llm_providers/orcarouter_provider.py)
mirroring the OpenAI provider pattern. OrcaRouter is an OpenAI-compatible
model routing gateway; model specs like orcarouter:orcarouter/auto reuse
the existing pydantic-ai OpenAI client path.

pydantic-ai has no native orcarouter: prefix, so
opencontractserver/llms/model_factory.py now always constructs a concrete
OpenAI-compatible model for this provider instead of returning a bare spec
string (which would raise 'Unknown model'). DB-configured credentials win;
otherwise ORCAROUTER_API_KEY and the default endpoint
https://api.orcarouter.ai/v1 are used.

Docs: model-spec table + API-keys section in docs/architecture/llms/README.md,
ORCAROUTER_API_KEY in the production sample env. Changelog fragment added.

Signed-off-by: XiaoHuo888-hue <jinhao.song@myflashcloud.com>
- Never pass api_key=None to OpenAIProvider for OrcaRouter: the OpenAI
  client would fall back to OPENAI_API_KEY and send the install's OpenAI
  secret to the third-party gateway. Use an inert placeholder + warning.
- Extract _is_valid_base_url() so the base_url scheme check lives in one
  place for every provider; move OrcaRouter construction to its own helper.
- Warn when a routed model belongs to a Responses-API-only family.
- Offer only orcarouter/auto in the picker, with a conservative 64K
  context-window entry (fixes test_every_supported_model_has_a_context_window).
- Update docs/test script provider counts and the changelog fragment.
Comment thread opencontractserver/llms/model_factory.py Fixed
@claude

claude Bot commented Sep 23, 2026

Copy link
Copy Markdown

Review

Solid, well-scoped PR — carries the OrcaRouter provider forward from #2275 and closes the two real blockers (API-key leak to api.orcarouter.ai, and the unverifiable-context-window issue) cleanly. The DRY extraction of _is_valid_base_url() is a faithful refactor of the existing scheme check, and the new tests exercise DB-wins/env-fallback/placeholder/Responses-family-warning paths well.

Findings

1. (Low/Medium) The module's "construction failure always degrades safely" invariant doesn't hold for OrcaRouter
model_factory.py's docstring promises: "Any failure to build a credentialed model degrades to the env-fallback string, so a misconfiguration can never take the chat path down." For every other provider that's true because the fallback string ("openai:...", "anthropic:...", etc.) is natively resolvable by pydantic-ai. For OrcaRouter it isn't — build_agent_model() computes env_spec = spec (i.e. "orcarouter:orcarouter/auto"), and if _construct_orcarouter_model() (model_factory.py:215-266) raises anything in _MODEL_BUILD_RECOVERABLE_ERRORS (e.g. a future ImportError/AttributeError from a pydantic-ai API shift touching OpenAIChatModel/OpenAIProvider), build_agent_model (line ~448-455) will happily return that bare "orcarouter:..." string — which pydantic-ai has no prefix for and will raise "Unknown model" on, per this PR's own rationale for why the always-build-concrete-model path exists in the first place. So the one provider this PR adds is also the one provider where "degrade gracefully" turns into "fail with a more confusing error than before." Given the pinned pydantic-ai-slim>=1.107.5 this is unlikely to trigger today, but it's worth either a short docstring caveat (module docstring + _construct_orcarouter_model) noting OrcaRouter is the exception to the graceful-degradation guarantee, or hardening the fallback (e.g. skip the env_spec degrade for provider_key == "orcarouter" and let the exception surface instead of silently returning an unresolvable string).

2. (Low, consistency nit) OpenAIChatModel import has no version-fallback for OrcaRouter
_construct_model's existing openai/ollama branch (model_factory.py:320-327) keeps a try/except ImportError fallback to the older OpenAIModel alias. _construct_orcarouter_model (line 234) imports OpenAIChatModel directly with no such fallback. Not reachable today given the pinned pydantic-ai version, but if that alias fallback is ever needed again, OrcaRouter would hit finding #1's failure mode instead of degrading like the other OpenAI-compatible path. Might be worth factoring the model-class resolution into one shared helper both branches call.

3. (Nit) Hardcoded requires_responses_api("openai", …) call in _construct_orcarouter_model
Line 255 always checks the routed model's tail against OPENAI_RESPONSES_ONLY_PREFIXES under the "openai" provider key, regardless of the actual vendor prefix in the routed spec (e.g. "anthropic/...", "google/..."). Harmless today since the guardrail list is OpenAI-only, but a one-line comment explaining that the check is intentionally vendor-agnostic (any routed model whose tail happens to match a Responses-only family name gets flagged) would save a future reader from wondering why "openai" is hardcoded against a possibly non-OpenAI routed name.

Other notes

  • context_guardrails.py's new "orcarouter/auto": 64_000 entry is looked up via exact match after prefix-stripping (context_guardrails.py:107-111), so it only helps the picker-offered orcarouter/auto spec — manually-typed routed models (orcarouter:openai/gpt-4.1) fall through to DEFAULT_CONTEXT_WINDOW (128K) since none of the existing keys share the vendor/model prefix shape. That matches the PR's stated intent ("routed model's window can't be known"), just flagging it's a real gap for anyone who types a specific routed model rather than using the picker's auto alias.
  • Nice catch keeping the top-level from opencontractserver.pipeline.llm_providers.orcarouter_provider import (...) import safe for early startup — verified orcarouter_provider.py's import chain (base_component.py, settings_schema.py, types/protocols.py) has no circular dependency back to model_factory and no AppRegistryNotReady risk (only django.conf.settings, not app registry access).
  • Good defensive design overall: placeholder key instead of None, _is_valid_base_url reused instead of duplicated, and the DB-wins/env-fallback/placeholder priority order all match the documented precedence and are covered by tests (test_db_base_url_wins_over_default, test_db_api_key_wins_over_env, test_unset_key_never_forwards_openai_key).

None of the findings above block merging — they're narrow edge cases in the failure path, not the happy path this PR is shipping. The CLA/authorship question flagged in the PR description is a maintainer decision, not a code issue.

JSv4 commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed in ddb3d71:

  1. Fixed. When building the OrcaRouter model fails, build_agent_model now re-raises the error. It no longer falls back to the bare orcarouter: spec, which pydantic-ai can't resolve. The module docstring and the LLM README now call out this exception, and a new test (test_construction_failure_is_raised_not_degraded) covers it.
  2. Leaving as is. Because of Bump postgres from 14.5 to 15.0 in /compose/production/postgres #1, an ImportError from a moved OpenAIChatModel now surfaces clearly instead of being hidden. With the pinned pydantic-ai version the old alias fallback can't be reached, so sharing a helper just for it isn't worth the churn in this PR.
  3. Done. Added a comment explaining that the hardcoded "openai" key is intentional and applies whatever the routed model's vendor prefix is.

On the context-window note: yes, that's intended. Only the auto alias the picker offers gets a sized entry. A routed model typed in by hand uses the default window until someone adds a verified entry for it.


Generated by Claude Code

@codecov

codecov Bot commented Sep 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment thread opencontractserver/constants/context_guardrails.py Outdated
…dels listing

Replaces the hardcoded orcarouter/auto MODEL_CONTEXT_WINDOWS entry. Agent
builds fetch GET {base_url}/models (TTL-cached, short timeout, no redirects,
never raises) and get_context_window_for_model serves orcarouter: specs from
that cache without I/O, falling back to ORCAROUTER_FALLBACK_CONTEXT_WINDOW.
Comment thread opencontractserver/llms/orcarouter_context.py Fixed
@claude

claude Bot commented Sep 24, 2026

Copy link
Copy Markdown

Review

Solid PR — this carries over the OrcaRouter provider from #2275 and fixes real problems in it (the api_key=None -> OPENAI_API_KEY leak in particular is a good catch). The DB-wins/env-fallback pattern, the _is_valid_base_url DRY-up, and the test coverage for the new provider path are all consistent with the conventions in opencontractserver/llms/model_factory.py.

One correctness concern I'd like addressed before merge, plus a couple of smaller notes.

Correctness: refresh_orcarouter_context_windows can raise httpx.InvalidURL, which isn't caught

In opencontractserver/llms/orcarouter_context.py:

```python
except (httpx.HTTPError, ValueError) as exc:
logger.warning(...)
return
```

httpx.InvalidURL (and httpx.CookieConflict) are documented by httpx as not subclassing httpx.HTTPError — they subclass Exception directly (httpx's own "Exceptions" docs page calls this out explicitly). A DB-configured base_url that passes _is_valid_base_url()'s scheme check (e.g. "http://" with no host, or a host containing a space/invalid character) can still cause httpx.get() to raise InvalidURL while it builds the request — before any network I/O happens.

That exception isn't caught here, and _construct_orcarouter_model has no try/except around refresh_orcarouter_context_windows(...) either. It would propagate through _construct_model into build_agent_model's except _MODEL_BUILD_RECOVERABLE_ERRORS (ImportError, AttributeError, ValueError, OSError, RuntimeError) — InvalidURL isn't a ValueError, so it wouldn't be caught there either, and would blow up the whole agent build for a misconfiguration that's supposed to degrade gracefully.

This runs against the module's own stated contract:

"A fetch never raises into the caller." (orcarouter_context.py docstring)

Since only superusers can set base_url, the blast radius is limited, but it's exactly the "malformed endpoint, typo'd host" scenario _is_valid_base_url() was written to guard against — the scheme check alone doesn't catch it. Suggest widening the except clause, e.g. except (httpx.HTTPError, httpx.InvalidURL, ValueError), or just except Exception given the docstring's "never raises" promise — plus a regression test with a scheme-valid-but-otherwise-malformed URL (e.g. "http://").

Minor notes (non-blocking)

  • context_guardrails.py::get_context_window_for_model changed the prefix-stripping from model_name.split(":", 1)[1] to provider, _, bare = model_name.partition(":"); lookup_name = bare if bare else model_name. For a spec that's literally "provider:" (empty model name after the colon), the old code looked up "" and the new code looks up the whole original string including the colon (since bare is falsy). Both end up hitting DEFAULT_CONTEXT_WINDOW in practice, so it's harmless, but it's a subtle behavior change worth a one-line comment if intentional — it looks incidental to the OrcaRouter branch added right after it.
  • _construct_orcarouter_model calls refresh_orcarouter_context_windows() — a blocking HTTP call (bounded by a 3s timeout) — inline on every agent build once the hourly TTL lapses. All current call sites correctly go through abuild_agent_model/sync_to_async, so this doesn't block the event loop, but the first chat request in a given worker process after each TTL expiry does pay up to a 3s stall (worse if the gateway is slow rather than down, since httpx.get waits the full timeout). Reasonable, documented tradeoff for dynamic context windows — just flagging the latency spike, not a bug.
  • Nice touch fixing the CodeQL clear-text-logging finding by making the "no api key configured" warning message static (no credential-derived interpolation).

Test coverage

Good breadth — DB-wins-over-env, env-fallback, invalid base_url fallback, the key-leak regression test, the Responses-API-family warning, and the _is_valid_base_url unit tests all look right. The one gap is the InvalidURL-shaped failure mode above; a test forcing httpx.get to raise httpx.InvalidURL (or simply passing a scheme-valid-but-hostless base_url through the real httpx.Client) would have caught it.

No security issues beyond the one above — the placeholder-key approach correctly prevents the OPENAI_API_KEY leak, follow_redirects=False correctly prevents the bearer token leaking to a redirect target, and the docs/changelog updates are accurate and consistent (docs/test_scripts/llm_runtime_config.md's "four" → "five" is applied everywhere it appears).

JSv4 commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed in 0dc554a:

  • httpx.InvalidURL (fixed). Confirmed: InvalidURL isn't an HTTPError, and http://[::1 gets past the scheme check but raises it. The fetch now catches it too. The new test_malformed_endpoint_falls_back_without_raising fails with InvalidURL without the fix and passes with it. (http:// with no host raises UnsupportedProtocol, which is an HTTPError and was already caught.)
  • Prefix stripping (reverted). get_context_window_for_model goes back to the original split(":", 1) logic, so a bare provider: spec behaves exactly as before. The OrcaRouter branch is now only an added check in front of it.
  • Latency after the hourly cache expires (leaving as is). That's the intended trade-off for fetching windows dynamically. The worst case is bounded by the 3s timeout, and it only happens off the event loop.

Generated by Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants