-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix usage script variable replacement for custom providers #2031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -85,25 +85,118 @@ pub(crate) async fn execute_and_format_usage_result( | |||||||||||||||||||||||||
| fn extract_api_key_from_provider(provider: &crate::provider::Provider) -> Option<String> { | ||||||||||||||||||||||||||
| if let Some(env) = provider.settings_config.get("env") { | ||||||||||||||||||||||||||
| // Try multiple possible API key fields | ||||||||||||||||||||||||||
| env.get("ANTHROPIC_AUTH_TOKEN") | ||||||||||||||||||||||||||
| if let Some(api_key) = env | ||||||||||||||||||||||||||
| .get("ANTHROPIC_AUTH_TOKEN") | ||||||||||||||||||||||||||
| .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")) | ||||||||||||||||||||||||||
|
Comment on lines
90
to
+93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||||||||||||||||||||||||||
| .and_then(|v| v.as_str()) | ||||||||||||||||||||||||||
| .map(|s| s.to_string()) | ||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||
| None | ||||||||||||||||||||||||||
| .filter(|s| !s.trim().is_empty()) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| return Some(api_key.to_string()); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| provider | ||||||||||||||||||||||||||
| .settings_config | ||||||||||||||||||||||||||
| .get("auth") | ||||||||||||||||||||||||||
| .and_then(|auth| auth.get("OPENAI_API_KEY")) | ||||||||||||||||||||||||||
| .or_else(|| { | ||||||||||||||||||||||||||
| provider | ||||||||||||||||||||||||||
| .settings_config | ||||||||||||||||||||||||||
| .get("options") | ||||||||||||||||||||||||||
| .and_then(|options| options.get("apiKey")) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| .or_else(|| provider.settings_config.get("apiKey")) | ||||||||||||||||||||||||||
| .or_else(|| provider.settings_config.get("api_key")) | ||||||||||||||||||||||||||
| .and_then(|v| v.as_str()) | ||||||||||||||||||||||||||
| .filter(|s| !s.trim().is_empty()) | ||||||||||||||||||||||||||
| .map(|s| s.to_string()) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// Extract base URL from provider configuration | ||||||||||||||||||||||||||
| fn extract_base_url_from_provider(provider: &crate::provider::Provider) -> Option<String> { | ||||||||||||||||||||||||||
| if let Some(env) = provider.settings_config.get("env") { | ||||||||||||||||||||||||||
| // Try multiple possible base URL fields | ||||||||||||||||||||||||||
| env.get("ANTHROPIC_BASE_URL") | ||||||||||||||||||||||||||
| if let Some(base_url) = env | ||||||||||||||||||||||||||
| .get("ANTHROPIC_BASE_URL") | ||||||||||||||||||||||||||
| .or_else(|| env.get("GOOGLE_GEMINI_BASE_URL")) | ||||||||||||||||||||||||||
| .or_else(|| env.get("OPENAI_BASE_URL")) | ||||||||||||||||||||||||||
| .and_then(|v| v.as_str()) | ||||||||||||||||||||||||||
| .map(|s| s.trim_end_matches('/').to_string()) | ||||||||||||||||||||||||||
| .filter(|s| !s.trim().is_empty()) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| return Some(base_url.trim_end_matches('/').to_string()); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| provider | ||||||||||||||||||||||||||
| .settings_config | ||||||||||||||||||||||||||
| .get("config") | ||||||||||||||||||||||||||
| .and_then(|v| v.as_str()) | ||||||||||||||||||||||||||
| .and_then(extract_codex_base_url_from_toml) | ||||||||||||||||||||||||||
| .or_else(|| { | ||||||||||||||||||||||||||
| provider | ||||||||||||||||||||||||||
| .settings_config | ||||||||||||||||||||||||||
| .get("options") | ||||||||||||||||||||||||||
| .and_then(|options| options.get("baseURL").or_else(|| options.get("baseUrl"))) | ||||||||||||||||||||||||||
| .and_then(|v| v.as_str()) | ||||||||||||||||||||||||||
| .filter(|s| !s.trim().is_empty()) | ||||||||||||||||||||||||||
| .map(|s| s.trim_end_matches('/').to_string()) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| .or_else(|| { | ||||||||||||||||||||||||||
| provider | ||||||||||||||||||||||||||
| .settings_config | ||||||||||||||||||||||||||
| .get("baseUrl") | ||||||||||||||||||||||||||
| .or_else(|| provider.settings_config.get("base_url")) | ||||||||||||||||||||||||||
| .and_then(|v| v.as_str()) | ||||||||||||||||||||||||||
| .filter(|s| !s.trim().is_empty()) | ||||||||||||||||||||||||||
| .map(|s| s.trim_end_matches('/').to_string()) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| fn extract_codex_base_url_from_toml(config_text: &str) -> Option<String> { | ||||||||||||||||||||||||||
| let parsed = config_text.parse::<toml::Value>().ok()?; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if let Some(model_provider) = parsed.get("model_provider").and_then(|v| v.as_str()) { | ||||||||||||||||||||||||||
| if let Some(base_url) = parsed | ||||||||||||||||||||||||||
| .get("model_providers") | ||||||||||||||||||||||||||
| .and_then(|providers| providers.get(model_provider)) | ||||||||||||||||||||||||||
| .and_then(|provider| provider.get("base_url")) | ||||||||||||||||||||||||||
| .and_then(|v| v.as_str()) | ||||||||||||||||||||||||||
| .filter(|s| !s.trim().is_empty()) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| return Some(base_url.trim_end_matches('/').to_string()); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if let Some(base_url) = parsed | ||||||||||||||||||||||||||
| .get("base_url") | ||||||||||||||||||||||||||
| .and_then(|v| v.as_str()) | ||||||||||||||||||||||||||
| .filter(|s| !s.trim().is_empty()) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| return Some(base_url.trim_end_matches('/').to_string()); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let base_urls: Vec<String> = parsed | ||||||||||||||||||||||||||
| .get("model_providers") | ||||||||||||||||||||||||||
| .and_then(|providers| providers.as_table()) | ||||||||||||||||||||||||||
| .map(|providers| { | ||||||||||||||||||||||||||
| providers | ||||||||||||||||||||||||||
| .values() | ||||||||||||||||||||||||||
| .filter_map(|provider| { | ||||||||||||||||||||||||||
| provider | ||||||||||||||||||||||||||
| .get("base_url") | ||||||||||||||||||||||||||
| .and_then(|v| v.as_str()) | ||||||||||||||||||||||||||
| .filter(|s| !s.trim().is_empty()) | ||||||||||||||||||||||||||
| .map(|s| s.trim_end_matches('/').to_string()) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| .collect() | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| .unwrap_or_default(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if base_urls.len() == 1 { | ||||||||||||||||||||||||||
| base_urls.into_iter().next() | ||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||
| None | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
@@ -185,9 +278,9 @@ pub async fn query_usage( | |||||||||||||||||||||||||
| /// Test usage script (using temporary script content, not saved) | ||||||||||||||||||||||||||
| #[allow(clippy::too_many_arguments)] | ||||||||||||||||||||||||||
| pub async fn test_usage_script( | ||||||||||||||||||||||||||
| _state: &AppState, | ||||||||||||||||||||||||||
| _app_type: AppType, | ||||||||||||||||||||||||||
| _provider_id: &str, | ||||||||||||||||||||||||||
| state: &AppState, | ||||||||||||||||||||||||||
| app_type: AppType, | ||||||||||||||||||||||||||
| provider_id: &str, | ||||||||||||||||||||||||||
| script_code: &str, | ||||||||||||||||||||||||||
| timeout: u64, | ||||||||||||||||||||||||||
| api_key: Option<&str>, | ||||||||||||||||||||||||||
|
|
@@ -196,11 +289,35 @@ pub async fn test_usage_script( | |||||||||||||||||||||||||
| user_id: Option<&str>, | ||||||||||||||||||||||||||
| template_type: Option<&str>, | ||||||||||||||||||||||||||
| ) -> Result<UsageResult, AppError> { | ||||||||||||||||||||||||||
| // Use provided credential parameters directly for testing | ||||||||||||||||||||||||||
| 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), | ||||||||||||||||||||||||||
|
Comment on lines
+293
to
+297
|
||||||||||||||||||||||||||
| 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), |
Copilot
AI
Apr 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract_api_key_from_providercurrently checks several keys undersettings_config.env, but it does not considerOPENAI_API_KEY. Other parts of the codebase treatenv.OPENAI_API_KEYas a valid source (e.g., Codex provider auth extraction), so usage script credential fallback can fail for providers that store the key there. Includeenv.get("OPENAI_API_KEY")in the env key chain (and keep the existing auth/options fallbacks).