Fix usage script variable replacement for custom providers#2031
Fix usage script variable replacement for custom providers#2031West-Pavilion wants to merge 2 commits intofarion1231:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes usage-script variable substitution for custom providers by improving template placeholder replacement and making usage-script test execution fall back to stored provider credentials when form fields are empty, aligning actual behavior with what the UI indicates.
Changes:
- Add forgiving template replacement (supports legacy lowercase aliases like
{{apikey}},{{baseurl}}, etc.). - Extend provider credential extraction (including Codex/OpenAI-style config parsing for base URL and API key lookup).
- Update usage-script test execution to fall back to saved provider credentials when test inputs are left blank, and add focused tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src-tauri/src/usage_script.rs |
Adds alias-aware template variable replacement and a regression test for lowercase placeholders. |
src-tauri/src/services/provider/usage.rs |
Improves credential/base URL extraction and adds provider-credential fallback during usage-script test execution, plus tests for Codex TOML parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .or_else(|| env.get("ANTHROPIC_API_KEY")) | ||
| .or_else(|| env.get("OPENROUTER_API_KEY")) | ||
| .or_else(|| env.get("GOOGLE_API_KEY")) | ||
| .or_else(|| env.get("GEMINI_API_KEY")) |
There was a problem hiding this comment.
extract_api_key_from_provider currently checks several keys under settings_config.env, but it does not consider OPENAI_API_KEY. Other parts of the codebase treat env.OPENAI_API_KEY as a valid source (e.g., Codex provider auth extraction), so usage script credential fallback can fail for providers that store the key there. Include env.get("OPENAI_API_KEY") in the env key chain (and keep the existing auth/options fallbacks).
| .or_else(|| env.get("GEMINI_API_KEY")) | |
| .or_else(|| env.get("GEMINI_API_KEY")) | |
| .or_else(|| env.get("OPENAI_API_KEY")) |
| let provider_credentials = || -> Option<(Option<String>, Option<String>)> { | ||
| let providers = state.db.get_all_providers(app_type.as_str()).ok()?; | ||
| let provider = providers.get(provider_id)?; | ||
| Some(( | ||
| extract_api_key_from_provider(provider), | ||
| extract_base_url_from_provider(provider), | ||
| )) | ||
| }; |
There was a problem hiding this comment.
In test_usage_script, the provider fallback path swallows database errors and missing-provider cases via get_all_providers(...).ok()? and providers.get(provider_id)?. When this happens, the script is executed with empty credentials, which can produce misleading user-facing errors and makes real DB/provider issues hard to diagnose. Consider returning an AppError when fallback credentials are needed but the provider lookup fails, instead of silently defaulting to empty strings.
| let providers = state.db.get_all_providers(app_type.as_str()).ok()?; | ||
| let provider = providers.get(provider_id)?; | ||
| Some(( | ||
| extract_api_key_from_provider(provider), | ||
| extract_base_url_from_provider(provider), |
There was a problem hiding this comment.
test_usage_script fetches providers using state.db.get_all_providers(...), which loads all providers (and their endpoint subqueries) just to read one provider’s credentials. This can be unnecessarily expensive if the modal’s “Test” is run frequently. Prefer using a targeted lookup like state.db.get_provider_by_id(provider_id, app_type.as_str()) for the fallback path.
| let providers = state.db.get_all_providers(app_type.as_str()).ok()?; | |
| let provider = providers.get(provider_id)?; | |
| Some(( | |
| extract_api_key_from_provider(provider), | |
| extract_base_url_from_provider(provider), | |
| let provider = state | |
| .db | |
| .get_provider_by_id(provider_id, app_type.as_str()) | |
| .ok()?; | |
| Some(( | |
| extract_api_key_from_provider(&provider), | |
| extract_base_url_from_provider(&provider), |
|
@codex review |
![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: 88e61e6911
ℹ️ 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".
| .or_else(|| env.get("ANTHROPIC_API_KEY")) | ||
| .or_else(|| env.get("OPENROUTER_API_KEY")) | ||
| .or_else(|| env.get("GOOGLE_API_KEY")) | ||
| .or_else(|| env.get("GEMINI_API_KEY")) |
There was a problem hiding this comment.
test_usage_script now relies on extract_api_key_from_provider when the usage-form API key is empty, but this extractor’s env lookup omits OPENAI_API_KEY. That means providers storing OpenAI tokens under settings_config.env.OPENAI_API_KEY (a format already supported by the Codex adapter) still resolve to an empty key during script testing, so the new fallback behavior fails for those providers. Adding this env key to the lookup chain would make the fallback consistent with existing provider auth extraction.
Useful? React with 👍 / 👎.
This fixes a pretty confusing usage-query bug in custom templates.
If a provider is using the custom usage template, the UI shows
{{apiKey}}and{{baseUrl}}as supported variables. In practice, though, they could fail to resolve unless the raw values were pasted directly into the script.What changed
OPENAI_API_KEYandbase_urlfrom their stored config{{apikey}}and{{baseurl}}in addition to the existing variable namesVerification
cargo test codex --libcargo test test_build_script_with_vars_supports_legacy_lowercase_aliases --libThis should make the actual behavior line up with what the modal tells users.