ENG-502: Add direct OpenAI provider support#388
Conversation
Farhan (fkb032)
force-pushed
the
fkb/eng-502-openai-clean
branch
from
09907c0 to
5deea3e
Compare
📋 PR Overview
🔬 Coverage
|
![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: 86002f30f3
ℹ️ 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 (@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 (@codex) address that feedback".
| }; | ||
|
|
||
| let (key, base_url) = if let Some(app) = app_handle { | ||
| crate::storage::store::get_effective_openrouter_provider_credential(app)? |
There was a problem hiding this comment.
This OpenRouter branch now only accepts an OpenRouter credential, so existing installs with summaryProvider/evolveProvider already stored as openrouter but only an OPENAI_API_KEY/stored OpenAI key stop working with “No OpenRouter API key found.” The previous code path used get_effective_openai_compatible_credential, which explicitly fell back to OpenAI credentials for the same provider value; the new auto-detection only helps when the provider is unset, and the same break applies to generate_evolution's OpenRouter branch.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,51 @@ | |||
| fn normalized_model_name(model: &str) -> String { | |||
There was a problem hiding this comment.
I think there should probably be a "ModelCapabilities" structure, and the context limit (like I saw elsewhere in this PR) should be part of that as well as temperature support and probably some other things. Longer-term we probably want to have a server-based model registry which can expose those things as JSON so we don't have to hard-code all this stuff.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41878e62e5
ℹ️ 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 (@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 (@codex) address that feedback".
| has_openai_provider_credential(app_handle)?, | ||
| has_openrouter_provider_credential(app_handle)?, |
There was a problem hiding this comment.
When a summary provider is explicitly configured as ollama, vllm, or a CLI provider, these new calls still read both OpenAI/OpenRouter credentials before the match selects the local provider. Those helpers go through get_effective_*_api_key and can return keychain errors, so a user with a locked/unavailable keychain can no longer use a configured non-cloud summary provider even though it does not need either cloud credential. Compute these booleans only when the provider is absent or when resolving legacy openai compatibility.
Useful? React with 👍 / 👎.
| let has_openai_provider_credential = | ||
| store::get_effective_openai_provider_credential(app)?.is_some(); | ||
| let has_openrouter_provider_credential = | ||
| store::get_effective_openrouter_provider_credential(app)?.is_some(); |
There was a problem hiding this comment.
This now probes both OpenAI/OpenRouter credentials before checking whether the configured evolve provider is ollama, vllm, or a CLI provider. Since get_effective_*_provider_credential can fail while reading the keychain, a user whose keychain is unavailable will fail to start evolution even when their selected provider is fully local and doesn't require cloud credentials. The credential checks should be delayed until the provider is unset or needs legacy openai resolution.
Useful? React with 👍 / 👎.
Juanpe Bolívar (arximboldi)
left a comment
Juanpe Bolívar (arximboldi)
left a comment
There was a problem hiding this comment.
Code LGTM, modulo the remark from Scott McMaster (@scottmcmaster) which I think is sensible.
I haven't tested it all the way through (I didn't have an OpenAI key at hand). I did test the preferences part. I found a, unrelated, I think, issue:
3 participants
Summary
Replaces the closed OpenAI provider PR with a focused branch rebased onto current
develop. Anthropic work is intentionally omitted per PM direction.This adds direct OpenAI support alongside OpenRouter:
openaiUI preference behavior by migrating/falling back to OpenRouter where needed.openai/prefix stripping, GPT-4o max-output limits, and no custom temperature for reasoning/GPT-5-family models.Test Plan
bunx vitest run --project=unit src/lib/ai-provider-validation.test.ts src/lib/ai-provider-migration.test.ts src/lib/api-key-verification.test.ts src/ipc/api.test.tscargo test --manifest-path apps/native/src-tauri/Cargo.toml direct_openai_cargo test --manifest-path apps/native/src-tauri/Cargo.toml model_capabilitiesbun run buildcargo test --manifest-path apps/native/src-tauri/Cargo.tomlgit diff --check$claude-reviewvia single outer Fable 5 call; accepted and fixed the blocking GPT-5 temperature-gating finding.Known unrelated local gate:
npx ultracite checkcurrently stops before file linting because the existingbiome.jsoncontains rule keys this Biome version does not recognize. This PR does not touch that config.Docs
Refs ENG-502