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: |
lidge-jun#967 found two real defects in my own lidge-jun#955 code and both verify at runtime: a Team account with a monthly window could never recover because the predicate picked its window by plan name while the parser picks by window duration, and the probe's own token refresh was mistaken for an external credential replacement. lidge-jun#963 and lidge-jun#965 both claim lidge-jun#962; lidge-jun#965 wins because it inherits from the row it actually replaces rather than recomputing config hints, and because lidge-jun#963 rewrites an existing regression contract to justify a broader change. lidge-jun#966 is a fifth design for lidge-jun#914 that survives two of the four prior falsifications but not all: mixed 5xx-then-rejection still loses the attributable failure, and five newly-classified sidecar paths keep default redirects, so a credential-visible 307 to a dead host still reads as neutral.
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