Skip to content

fix: [ENG-2897] route all OpenAI-compatible providers to 'openai' registry type#701

Merged
bao-byterover merged 3 commits into
mainfrom
fix/ENG-2897
May 25, 2026
Merged

fix: [ENG-2897] route all OpenAI-compatible providers to 'openai' registry type#701
bao-byterover merged 3 commits into
mainfrom
fix/ENG-2897

Conversation

@cuongdo-byterover
Copy link
Copy Markdown
Collaborator

Summary

  • Fix [BUG] openai-compatible provider rejects valid models due to hardcoded LLM_REGISTRY whitelist #691: brv providers connect openai-compatible --model <custom> no longer fires the registry-whitelist rejection; the gate now correctly bypasses for the user-facing provider ID.
  • Same fix also unblocks 10 other passthrough provider IDs (deepseek, glm, glm-coding-plan, moonshot, cerebras, cohere, deepinfra, togetherai, vercel, minimax, perplexity) that were silently misrouted to the Gemini registry.
  • Regression test cross-checks the resolver's list against PROVIDER_MODULES in infra, so adding a new OpenAI-compatible provider without updating the resolver fails the test.

Root cause

resolveRegistryProvider in src/agent/core/domain/llm/registry.ts explicitly mapped 6 user-facing provider IDs (openai, openai-compatible, openrouter, xai, groq, mistral) to the registry-internal 'openai' type. The other 11 OpenAI-compatible passthrough providers fell through to the 'gemini' default branch. Downstream effects of the wrong resolution:

  1. acceptsAnyModel('gemini') returns false → the schema validator fires "Model X is not supported for provider 'gemini'" for any model not in the Gemini registry.
  2. getEffectiveMaxInputTokens('gemini', ...) clamps to a Gemini model's window instead of honouring the user's maxInputTokens cap.
  3. Tokenizer / formatter selection in agent-llm-service.ts picks Gemini's, which is architecturally wrong for an OpenAI-compatible endpoint (works today because AiSdkContentGenerator does its own message conversion, but the mismatch is latent risk).

Issue #691 surfaced through openai-compatible because it was the most-used affected provider; the other 10 were broken in the same way without an open ticket.

Fix

Expand the explicit OpenAI-mapped list to all 17 user-facing IDs that declare providerType: 'openai' in their provider module. One file changed, plus a regression test.

Test plan

  • test/unit/agent/core/domain/llm/resolve-registry-provider.test.ts — 22 cases, including a cross-check that imports PROVIDER_MODULES from infra and asserts every module with providerType: 'openai' resolves to 'openai'. New OpenAI-compatible providers added in infra without updating the resolver fail this test.
  • Manual verification of the [BUG] openai-compatible provider rejects valid models due to hardcoded LLM_REGISTRY whitelist #691 reproduction:
    brv providers connect openai-compatible \
      --base-url <endpoint> \
      --api-key <key> \
      --model deepseek-v4-flash
    brv curate "Test curation"
    Pre-fix: Model 'deepseek-v4-flash' is not supported for provider 'openai-compatible'. Supported: gpt-4.1, gpt-4.1-mini, ...
    Post-fix: validation passes; any error originates from the downstream LLM call (endpoint unreachable, model not found at endpoint), never from the registry whitelist.
  • Full test suite: 8331 passing on Node 22.
  • Typecheck + lint clean.

Closes #691.

…istry type

resolveRegistryProvider only listed 6 of the 17 user-facing provider IDs
that talk OpenAI-compatible APIs. The other 11 (deepseek, glm,
glm-coding-plan, moonshot, cerebras, cohere, deepinfra, togetherai,
vercel, minimax, perplexity) fell through to the 'gemini' default,
breaking acceptsAnyModel bypass, context-length lookups, and tokenizer
selection — surfaced by openai-compatible users in #691 but latent for
the other ten.

Cross-checks the resolver's hardcoded list against PROVIDER_MODULES so
any future OpenAI-compatible provider added in infra fails the test
unless the resolver is updated in sync.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 25, 2026

Claude finished @cuongdo-byterover's task in 3m 39s —— View job


Claude PR Review

  • Read PR description and changed files list
  • Read src/agent/core/domain/llm/registry.ts (current change)
  • Read test/unit/agent/core/domain/llm/resolve-registry-provider.test.ts
  • Cross-check provider list against PROVIDER_MODULES / providerType: 'openai' providers in infra
  • Inspect downstream consumers (acceptsAnyModel, getEffectiveMaxInputTokens, formatter/tokenizer selection)
  • Post inline comments
  • Post summary with overall score

Summary

Overall: 4.5 / 5 — targeted, correct fix with a thoughtful regression guard. Two minor nits (one wording, one latent gap on the gemini side) and one optional test-coverage suggestion; none are blockers.

Correctness

  • praise: I cross-checked the resolver's hardcoded openai list (17 IDs) against every src/agent/infra/llm/providers/*.ts module declaring providerType: 'openai'. Both sets match exactly: cerebras, cohere, deepinfra, deepseek, glm, glm-coding-plan, groq, minimax, mistral, moonshot, openai, openai-compatible, openrouter, perplexity, togetherai, vercel, xai. The 11 newly-mapped providers (deepseek, glm, glm-coding-plan, moonshot, cerebras, cohere, deepinfra, togetherai, vercel, minimax, perplexity) were indeed falling through to the 'gemini' default branch pre-fix — confirmed by diffing against HEAD~1 (old list was just 6 entries).
  • praise: Root-cause analysis in the PR body matches the call sites — resolveRegistryProvider is consumed by agent-llm-service.ts:810 (tokenizer/formatter selection) and cipher-agent.ts:1271 (getEffectiveMaxInputTokens for map tools). Both paths benefit from the fix.

Issues

  • issue (latent, out of scope): The byterover provider (providerType: 'gemini') is not in step-1 explicit handling. It currently resolves correctly only because the terminal fallback is return 'gemini' — i.e. by accident, not by design. The new test doesn't cover this because it only iterates openai-compatible providers. See inline comment on registry.ts:711 for two cheap follow-up options.
  • nit: Comment on registry.ts:706-708 says "at build time" but the cross-check is a runtime mocha test. Tighten to "at test time" — see inline comment for suggestion.
  • nit (PR description): Body says "10 other passthrough provider IDs" then lists 11. Off-by-one in the description only, not the code.

Test design

  • praise: Cross-layer import in the test (domain test pulling from infra) is justified — that's literally the regression guard. The sanity-floor assertion (length > 0 + explicit include('openai-compatible')) is a great defense against a future refactor that quietly empties the dynamically-generated it() block. See inline comment on the test for an optional extension covering all three providerType buckets.

Security / Performance

  • No security implications: provider-ID set is closed, hardcoded in code, not user-controlled.
  • No performance impact: trivial Array.includes on a 17-element list, called per-task.

Verdict

LGTM. The latent byterover gap is worth a follow-up issue but does not block this PR — current behavior is unchanged for that provider.
• Branch: fix/ENG-2897

@cuongdo-byterover cuongdo-byterover changed the title fix: [ENG-2897] route all OpenAI-compatible providers to 'openai' reg… fix: [ENG-2897] route all OpenAI-compatible providers to 'openai' registry type May 25, 2026
Comment thread src/agent/core/domain/llm/registry.ts Outdated
Comment thread src/agent/core/domain/llm/registry.ts Outdated
Comment thread test/unit/agent/core/domain/llm/resolve-registry-provider.test.ts
… all-bucket cross-check

Reviewer flagged that `byterover` (declared providerType: 'gemini')
resolved correctly only via the terminal `return 'gemini'` fallback —
by accident, not by design. A future refactor of the default branch
would silently regress it on the same path that bit openai-compatible.

- Map `byterover` explicitly in the gemini branch alongside `google` /
  `google-vertex`, so resolution is intent-driven rather than fallback-
  driven.
- Generalise the regression test to iterate every provider module and
  assert it resolves to its declared `providerType`. Now covers all
  three buckets (claude / gemini / openai = 24 cases vs 22 before),
  so drift on the claude/gemini side fails loudly instead of slipping
  through the same gap.
- Pin `google-vertex` separately (no infra module, only in
  PROVIDER_REGISTRY) so it stays covered by an explicit test.
- Tighten the registry comment: "at build time" -> "at test time" to
  match the actual mocha-runtime check.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 25, 2026

Claude finished @cuongdo-byterover's task in 2m 19s —— View job


Claude PR Review (synchronize on d96ae86)

  • Read updated src/agent/core/domain/llm/registry.ts
  • Read updated test/unit/agent/core/domain/llm/resolve-registry-provider.test.ts
  • Cross-check that d96ae86 addresses prior review feedback
  • Verify byterover is now explicitly mapped
  • Verify cross-check test now covers all three providerType buckets
  • Post inline comments + summary

Summary

Overall: 4.8 / 5 — every actionable item from the prior review was addressed cleanly. One small comment-accuracy nit on the test side; no functional issues.

What changed since the prior review (23b8342d96ae86)

  1. byterover is now explicitly mapped to 'gemini' (registry.ts:714-720). Previously it resolved correctly only via the terminal return 'gemini' fallback — now it is intentional, not accidental. ✅
  2. Comment wording fixed — "Sync is locked … at build time" → "Sync is verified by a regression test … at test time" (registry.ts:707-711). ✅
  3. Cross-check expanded to all three providerType buckets (resolve-registry-provider.test.ts:25-46). The test now iterates every listProviderModules() entry and asserts resolveRegistryProvider(<any-model>, module.id) === module.providerType. The sanity-floor block additionally asserts coverage of claude, gemini, AND openai buckets, so a future drift on any side fails loudly instead of silently no-op'ing. ✅
  4. google-vertex pinned via an explicit test case (resolve-registry-provider.test.ts:48-55), since it has no agent-side ProviderModule. ✅

Correctness re-verification

Re-ran the per-module cross-check by hand against src/agent/infra/llm/providers/*.ts:

Bucket Modules declaring this providerType Resolver-mapped IDs Match?
claude anthropic anthropic
gemini google, byterover google, byterover (+ google-vertex defensive)
openai 17 modules (cerebras, cohere, deepinfra, deepseek, glm, glm-coding-plan, groq, minimax, mistral, moonshot, openai, openai-compatible, openrouter, perplexity, togetherai, vercel, xai) identical 17 IDs

No misroutes. The terminal-fallback return 'gemini' is no longer load-bearing for any real provider — byterover was the only one that needed it.

Issues / nits

  • nit (documentation only, posted inline): The new test's block comment at lines 48–51 says google-vertex is "exposed in PROVIDER_REGISTRY (user-facing)". Verified by grep -rn "google-vertex" src/: it only appears inside registry.ts itself — no entry in src/server/core/domain/entities/provider-registry.ts, and no caller passes it as explicitProvider. The mapping is future-proofing for Vertex support, not a current user-facing path. Suggested rewording inline; cosmetic only.

Security / performance

  • No change since the prior review. Closed hardcoded set, not user-controlled. Trivial Array.includes cost.

Verdict

LGTM. Ship it. The remaining nit is documentation-only.

• Branch: fix/ENG-2897

Comment thread test/unit/agent/core/domain/llm/resolve-registry-provider.test.ts
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 25, 2026

Claude finished @bao-byterover's task in 1m 54s —— View job


Claude PR Review (synchronize on f711c71)

  • Read PR description and changed files list
  • Read updated src/agent/core/domain/llm/registry.ts
  • Read updated test/unit/agent/core/domain/llm/resolve-registry-provider.test.ts
  • Check what changed since prior review (d96ae86f711c71)
  • Cross-check resolver list against PROVIDER_MODULES in infra
  • Post summary

Summary

Overall: 4.8 / 5 — no functional change since the prior review; the synchronize is a clean merge from main. Prior LGTM still stands.

What changed in f711c71

  • f711c71 is a merge commit pulling origin/main into the branch (picking up ded2802 Proj/setting auto update (#696) and surrounding work).
  • git diff d96ae86..HEAD -- src/agent/core/domain/llm/registry.ts test/unit/agent/core/domain/llm/resolve-registry-provider.test.ts is empty — the two ENG-2897 files are byte-for-byte identical to d96ae86.
  • No merge conflicts introduced into the fix files. The unrelated 90+ files surfaced by the merge belong to other shipped/landed work and are not part of this PR's scope.

Re-verification on the merged tree

Re-ran the per-module cross-check against src/agent/infra/llm/providers/*.ts on the current tree:

Bucket Modules in infra Resolver-mapped IDs Match?
claude anthropic anthropic
gemini google, byterover google, byterover (+ google-vertex defensive)
openai 17 modules (cerebras, cohere, deepinfra, deepseek, glm, glm-coding-plan, groq, minimax, mistral, moonshot, openai, openai-compatible, openrouter, perplexity, togetherai, vercel, xai) identical 17 IDs

No drift. Regression test still asserts all three buckets (registry.ts:707-711, resolve-registry-provider.test.ts:34-45).

Outstanding (carried over, optional)

  • nit (doc-only, from prior review, still unaddressed): resolve-registry-provider.test.ts:49 still says google-vertex is "exposed in PROVIDER_REGISTRY (user-facing)". Verified again by grep -rn "google-vertex" src/ — only registry.ts references it; it is not in src/server/core/domain/entities/provider-registry.ts and no caller passes it as explicitProvider. Cosmetic only; the resolver mapping itself is correct as forward-looking pinning. Not a blocker.

Verdict

LGTM — ship it. The synchronize introduces no risk to the fix; the merge is clean and the regression-guard test is intact. Only carry-over is the documentation-accuracy nit on the google-vertex block comment, which is optional.
• Branch: fix/ENG-2897

@bao-byterover bao-byterover merged commit 62ddfd6 into main May 25, 2026
7 checks passed
@bao-byterover bao-byterover deleted the fix/ENG-2897 branch May 25, 2026 04:06
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.

[BUG] openai-compatible provider rejects valid models due to hardcoded LLM_REGISTRY whitelist

2 participants