fix(catalog): carry bare-OpenAI catalog omit onto dig2-go (#643) - #681
Conversation
…jun#643) Fixes lidge-jun#636. Skip bare gpt-*/native OpenAI catalog rows when no enabled canonical openai provider exists; make NoEnabledOpenAiProviderError actionable.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![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: 02fe25fe74
ℹ️ 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".
| // #636: when the user only configured non-OpenAI providers (e.g. kimi), do not advertise | ||
| // bare gpt-* rows that hard-404 via NoEnabledOpenAiProviderError. Keep natives when no | ||
| // providers are configured yet (fresh install / catalog bootstrap tests). | ||
| const includeNativeOpenAi = enabledProviders.length === 0 || hasCanonicalOpenai; |
There was a problem hiding this comment.
When every configured provider is disabled—especially an existing canonical openai row—enabledProviders is empty, so this condition sets includeNativeOpenAi to true and sync preserves/backfills bare GPT models. Those entries remain selectable even though routeModel rejects them because openai.disabled === true; distinguish a genuinely empty provider configuration from a configuration with zero enabled providers, and add a focused disabled-OpenAI sync regression.
AGENTS.md reference: AGENTS.md:L112-L114
Useful? React with 👍 / 👎.
| super(`No enabled OpenAI provider for model: ${modelId}. Run 'ocx init' to configure a provider, or check that your config has an enabled 'openai' provider.`); | ||
| super( | ||
| `Model ${modelId} requires the canonical openai provider. ` | ||
| + `Run: ocx provider add openai && ocx sync && ocx restart`, |
There was a problem hiding this comment.
When the canonical openai provider exists but is disabled, this error recommends ocx provider add openai, but handleAdd in src/cli/provider.ts:163-165 exits with Provider "openai" already exists unless --force is supplied. The supported non-destructive recovery shown by the provider CLI is ocx provider edit openai --enabled on, so the error needs to distinguish disabled from missing providers rather than directing affected users to a command that immediately fails.
Useful? React with 👍 / 👎.
| const native = includeNativeOpenAi | ||
| ? catalogModels |
There was a problem hiding this comment.
For any configuration with enabled providers but no canonical openai, setting includeNativeOpenAi to false makes this ternary discard every bare catalog row, not just the supported OpenAI-native slugs. That deletes user-added entries such as user-native, whose preservation is an explicit existing invariant in tests/codex-catalog-sync-hardening.test.ts:57-90 and tests/codex-catalog-restore.test.ts:41-62; because sync overwrites the catalog, additions made after the pristine backup can be lost. Keep non-OpenAI bare rows and conditionally filter/backfill only the native OpenAI slug set.
Useful? React with 👍 / 👎.
1 participant
Summary
f19a73c3ontodev2-go(TS catalog + router + tests).Go port
Deferred: #680
Go has
catalog_sync.go/ visibility; needs omit-native-without-openai + actionable error parity.Test plan