fix(catalog): custom model rows inherit provider reasoning metadata - #965
fix(catalog): custom model rows inherit provider reasoning metadata#965Yuxin-Qiao wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughCustom catalog rows now inherit missing metadata from matching provider-derived rows before deduplication. Explicit custom values remain unchanged. A regression test verifies reasoning, tool capability, modalities, context, and generated catalog output. ChangesCatalog metadata handling
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 |
![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
🤖 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/codex-catalog.test.ts`:
- Around line 851-900: Add a second provider/custom model fixture in the
existing test to cover inherited non-empty reasoning metadata, configuring
provider reasoning efforts and modelDefaultReasoningEfforts. Assert the replaced
custom CatalogModel preserves defaultReasoningEffort and that
buildCatalogEntries emits the corresponding default_reasoning_level, while
retaining the current empty-reasoning assertions for issue `#962`.
🪄 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: c718f5e9-51bd-46bf-8ce6-39f79dbc91cd
📒 Files selected for processing (2)
src/codex/catalog/provider-fetch.tstests/codex-catalog.test.ts
| test("a custom row inherits provider reasoning metadata from the provider-derived row it replaces (#962)", async () => { | ||
| clearModelCache("ollama"); | ||
| const originalFetch = globalThis.fetch; | ||
| globalThis.fetch = (() => { throw new Error("fetch should not be called"); }) as typeof fetch; | ||
| try { | ||
| const models = await gatherRoutedModels({ | ||
| port: 10100, | ||
| defaultProvider: "ollama", | ||
| providers: { | ||
| ollama: { | ||
| baseUrl: "http://localhost:11434/v1", | ||
| adapter: "openai-chat", | ||
| authMode: "key", | ||
| liveModels: false, | ||
| models: ["qwen-coder-3b"], | ||
| selectedModels: ["qwen-coder-3b"], | ||
| noReasoningModels: ["qwen-coder-3b"], | ||
| modelReasoningEfforts: { "qwen-coder-3b": [] }, | ||
| }, | ||
| }, | ||
| customModels: [ | ||
| { | ||
| id: "cm-962", | ||
| provider: "ollama", | ||
| modelId: "qwen-coder-3b", | ||
| displayName: "Qwen Coder 3B (local)", | ||
| contextWindow: 32768, | ||
| inputModalities: ["text"], | ||
| addedAt: "2026-01-01T00:00:00.000Z", | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| // Explicit custom fields stay verbatim; provider capability metadata is inherited from the | ||
| // replaced provider-derived row (noReasoningModels -> empty reasoning ladder, openai-chat | ||
| // adapter -> parallel tool calls). | ||
| const custom = models.find(m => m.provider === "ollama" && m.id === "qwen-coder-3b"); | ||
| expect(custom?.displayName).toBe("Qwen Coder 3B (local)"); | ||
| expect(custom?.contextWindow).toBe(32768); | ||
| expect(custom?.inputModalities).toEqual(["text"]); | ||
| expect(custom?.reasoningEfforts).toEqual([]); | ||
| expect(custom?.parallelToolCalls).toBe(true); | ||
|
|
||
| const entries = buildCatalogEntries(nativeTemplate(), [], models); | ||
| const row = entries.find(e => e.slug === "ollama/qwen-coder-3b"); | ||
| expect(row?.display_name).toBe("Qwen Coder 3B (local)"); | ||
| // The catalog must expose no reasoning levels and no default reasoning level for this model; | ||
| // the generic low..ultra ladder and the medium default must not be synthesized. | ||
| expect(row?.supported_reasoning_levels).toEqual([]); | ||
| expect(row?.default_reasoning_level).toBeUndefined(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for an inherited non-empty default reasoning effort.
This fixture gives the provider-derived row an empty reasoningEfforts list and no defaultReasoningEffort. Therefore, the test cannot detect a regression in src/codex/catalog/provider-fetch.ts line 813.
Add a second provider/custom pair with configured reasoning efforts and modelDefaultReasoningEfforts. Assert both CatalogModel.defaultReasoningEffort and the generated default_reasoning_level. Keep the current no-reasoning case for issue #962.
🤖 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/codex-catalog.test.ts` around lines 851 - 900, Add a second
provider/custom model fixture in the existing test to cover inherited non-empty
reasoning metadata, configuring provider reasoning efforts and
modelDefaultReasoningEfforts. Assert the replaced custom CatalogModel preserves
defaultReasoningEffort and that buildCatalogEntries emits the corresponding
default_reasoning_level, while retaining the current empty-reasoning assertions
for issue `#962`.
![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: c5565d055c
ℹ️ 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".
| const enrichedByName = new Map(activeProviders); | ||
| // Provider-derived rows keyed by their Codex-facing slug: a custom override replaces the row | ||
| // with the same slug below, so that row's provider capability metadata is the inheritance source. | ||
| const replacedByRoutedSlug = new Map(all.map(model => [routedSlug(model.provider, model.id), model])); |
There was a problem hiding this comment.
When a provider exposes colliding native IDs such as a-b and a/b, both produce the same routed slug, and the preceding sort causes this map to retain the slash row. A custom override for the plain a-b model therefore inherits the other model's reasoning ladder, context, and capabilities, even though resolveSlugAliasCollisions() deliberately catalogs the plain-hyphen model as the winner. Index the provider-derived rows by exact provider/native ID first, and use a routed-slug fallback only when that slug is unambiguous.
Useful? React with 👍 / 👎.
| const replaced = replacedByRoutedSlug.get(routedSlug(cm.provider, cm.modelId)); | ||
| const merged: CatalogModel = replaced ? { | ||
| ...base, | ||
| ...(base.contextWindow === undefined && replaced.contextWindow !== undefined ? { contextWindow: replaced.contextWindow } : {}), | ||
| ...(base.maxInputTokens === undefined && replaced.maxInputTokens !== undefined ? { maxInputTokens: replaced.maxInputTokens } : {}), | ||
| ...(base.inputModalities === undefined && replaced.inputModalities !== undefined ? { inputModalities: replaced.inputModalities } : {}), | ||
| ...(base.reasoningEfforts === undefined && replaced.reasoningEfforts !== undefined ? { reasoningEfforts: replaced.reasoningEfforts } : {}), | ||
| ...(base.defaultReasoningEffort === undefined && replaced.defaultReasoningEffort !== undefined ? { defaultReasoningEffort: replaced.defaultReasoningEffort } : {}), | ||
| ...(base.parallelToolCalls === undefined && replaced.parallelToolCalls !== undefined ? { parallelToolCalls: replaced.parallelToolCalls } : {}), | ||
| ...(base.supportsVerbosity === undefined && replaced.supportsVerbosity !== undefined ? { supportsVerbosity: replaced.supportsVerbosity } : {}), | ||
| ...(base.supportsReasoningSummaries === undefined && replaced.supportsReasoningSummaries !== undefined ? { supportsReasoningSummaries: replaced.supportsReasoningSummaries } : {}), | ||
| ...(base.capabilities === undefined && replaced.capabilities !== undefined ? { capabilities: replaced.capabilities } : {}), | ||
| } : base; |
There was a problem hiding this comment.
When a custom model is intentionally added outside the provider's discovered/static list—a supported case already exercised by the renamed-model custom-model test—this lookup returns undefined, so the row still ignores noReasoningModels, modelReasoningEfforts, defaults, and adapter capabilities and buildCatalogEntries() synthesizes the generic reasoning ladder. This leaves the reported spawn-agent failure unfixed for the normal use case where customModels supplies an otherwise unlisted model; derive the missing capability fields through the provider hint flow even when there is no replaced row, while retaining explicit custom context/modalities.
AGENTS.md reference: src/AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
|
Please put your Pull-Request on Ready for Review, once you are finished. |
|
Carried into #973 (stack 6/6), with authorship preserved ( #963 landed the same fix for #962 about an hour apart. Yours won on evidence: it inherits from the provider row actually being replaced, so it also retains live Your regression reproduces the reporter's real configuration, including both Verified on the stack: |
3 participants
Summary
Fixes #962. When a provider model also has a dashboard-created
customModelsrow,gatherRoutedModels()replaced the provider-derived catalog row without retaining its provider capability metadata. The custom row reachedbuildCatalogEntries()withoutreasoningEfforts, soapplyReasoningLevels()synthesized the genericlow..ultraladder plus amediumdefault. For a local Ollama model configured withnoReasoningModelsand an emptymodelReasoningEfforts, Codex then rejected an ephemeralspawn_agentcall during client-side preflight: "Reasoning effort 'none' is not supported".Fix
In
src/codex/catalog/provider-fetch.ts, a custom row now inherits provider capability metadata from the provider-derived row it replaces (same routedprovider/modelslug):reasoningEfforts) and default effort (defaultReasoningEffort)Explicit custom fields (display name, context window, input modalities) still win verbatim, preserving the existing user-override contract from the vision-sidecar work (#349/#344). A
noReasoningModelscustom model now yields a catalog entry with no reasoning levels and no default reasoning level.Reproduction
ollama/qwen-coder-3bwithnoReasoningModels+ emptymodelReasoningEffortsand a matching dashboard custom row.ocx syncpreviously advertisedsupported_reasoning_levels: low..ultraanddefault_reasoning_level: medium; after the fix the entry has an empty reasoning ladder and no default level.Tests
tests/codex-catalog.test.tscovering the issue end-to-end throughgatherRoutedModels+buildCatalogEntries: empty reasoning ladder, no default level, explicit custom fields preserved.bun run typecheckpasses.codex-catalog,catalog-vision-sidecar-modalities,vertex-catalog,catalog-input-modality-enum,codex-catalog-sync-hardening): 157 tests, 0 fail.bun run test: no catalog failures; the only failures reproduce on cleandevin this local environment (management provider-validation HTTP tests and a crash-guard timing test), and upstream CI is green at the samedevbase commit.Summary by CodeRabbit