fix(catalog): accept Together top-level /models arrays - #639
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughProvider model discovery now normalizes top-level arrays and ChangesProvider model discovery
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review |
|
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/management/provider-routes.ts (1)
343-349: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep connectivity validation aligned with catalog discovery.
Line 346 accepts any array, so responses such as
[{}]or[{"error":"invalid key"}]returnok: trueeven thoughfetchProviderModelsrejects those entries throughisProviderModelsApiItems. The dashboard can therefore report “Connected” while discovery still fails.Reuse the catalog validator before reporting success:
Proposed fix
-import { providerModelsListFromResponse } from "../../codex/catalog/provider-fetch"; +import { + isProviderModelsApiItems, + providerModelsListFromResponse, +} from "../../codex/catalog/provider-fetch"; - if (!Array.isArray(list)) { + if (!isProviderModelsApiItems(list)) { return jsonResponse({ ok: false, latencyMs, error: "upstream /models returned an unexpected shape" }); }As per path instructions, provider/adapter contract drift should be prevented across the catalog and connectivity paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/management/provider-routes.ts` around lines 343 - 349, Update the connectivity validation around providerModelsListFromResponse in the management route to reuse the same isProviderModelsApiItems validator used by fetchProviderModels. Reject arrays containing invalid entries, such as error objects, with the existing unexpected-shape response; only report success when the catalog entries pass the shared validator.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/provider-connection-test.test.ts`:
- Around line 138-149: Add focused coverage for fetchProviderModels using the
Together-style top-level array response, reusing the existing mock payload and
Together configuration. Assert the discovered catalog includes both “meta/llama”
and “Qwen/Qwen”, alongside—not instead of—the existing probe test.
---
Outside diff comments:
In `@src/server/management/provider-routes.ts`:
- Around line 343-349: Update the connectivity validation around
providerModelsListFromResponse in the management route to reuse the same
isProviderModelsApiItems validator used by fetchProviderModels. Reject arrays
containing invalid entries, such as error objects, with the existing
unexpected-shape response; only report success when the catalog entries pass the
shared validator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 42ab0570-c897-444b-a549-1c1c0bd3e2e3
📒 Files selected for processing (3)
src/codex/catalog/provider-fetch.tssrc/server/management/provider-routes.tstests/provider-connection-test.test.ts
| test("Together-style top-level /models array is accepted (#617)", async () => { | ||
| globalThis.fetch = (async () => new Response(JSON.stringify([{ id: "meta/llama" }, { id: "Qwen/Qwen" }]), { | ||
| status: 200, | ||
| headers: { "content-type": "application/json" }, | ||
| })) as typeof fetch; | ||
| const config = baseConfig({ | ||
| together: { adapter: "openai-chat", baseUrl: "https://api.together.xyz/v1", apiKey: "tg-key" }, | ||
| }); | ||
| const { body } = await probe(config, "together"); | ||
| expect(body.ok).toBe(true); | ||
| expect(body.models).toBe(2); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add catalog-discovery coverage for the Together response.
This test verifies only /api/providers/test; it never exercises fetchProviderModels, which is the primary path fixed by the PR. A future regression in catalog normalization could therefore leave this test green while Together model discovery fails.
Add a focused catalog test using the same top-level array and assert that the discovered catalog contains meta/llama and Qwen/Qwen.
As per path instructions, a behavior change in src/ should have a focused regression test near the existing tests for that subsystem; the current test covers only the connectivity probe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/provider-connection-test.test.ts` around lines 138 - 149, Add focused
coverage for fetchProviderModels using the Together-style top-level array
response, reusing the existing mock payload and Together configuration. Assert
the discovered catalog includes both “meta/llama” and “Qwen/Qwen”, alongside—not
instead of—the existing probe test.
Source: Path instructions
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a29621972a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (json !== null && typeof json === "object") { | ||
| const obj = json as { data?: unknown; models?: unknown }; | ||
| if (Array.isArray(obj.data)) return obj.data; | ||
| if (Array.isArray(obj.models)) return obj.models; |
There was a problem hiding this comment.
models
For an OpenAI-chat provider returning { models: [] }, this branch now yields an empty array; isProviderModelsApiItems accepts it vacuously, so discovery becomes authoritative-empty and drops the configured fallback. That directly contradicts the unchanged missing-data case in tests/codex-catalog.test.ts:1888-1917, which expects static-fallback and an invalid_response status, so the catalog suite will fail. Either restrict this envelope to provider-aware Google handling or update the catalog contract and add a focused top-level-array catalog regression, rather than testing only the management probe.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
|
@codex @coderabbitai: addressed in a2b8458 — catalog discovery accepts top-level arrays + |
|
You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ ✏️ Learnings added
|
|
Transition complete for #639:
|
Align FetchProviderModels with the TS oracle: accept top-level arrays
and {data:[...]}, reject stray {models} for catalog discovery.
fix(catalog): carry Together /models array fix onto dev2-go (#639)
1 participant
Summary
/modelspayloads that return a top-level JSON array (Together AI).Test plan
bun test tests/provider-connection-test.test.tsCloses #617
Summary by CodeRabbit
/modelsresponse formats, including top-level arrays and alternate object structures.