fix(api): provider endpoint mismatches preventing copilot save#511
fix(api): provider endpoint mismatches preventing copilot save#511ibhagwan wants to merge 1 commit intospacedriveapp:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEnsure GitHub Copilot is auto-registered in test LLM configs when missing; add pre-removal checks that detect provider keys in TOML, secrets, or environment and prevent TOML deletion if an env var is active; expose Copilot base URL and helper at crate scope and add a unit test validating registration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review SummaryP2 Suggestion (non-blocking)The Copilot special case in P3 Architecture NoteThe sandbox refactoring (deleting |
This PR does not modify detection.rs at all, probably a local leftover during the review? |
ibhagwan
force-pushed
the
fix/copilot-provider-endpoints
branch
2 times, most recently
from
0904a72 to
80a69de
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/providers.rs`:
- Around line 497-503: The tuple elements for the LLM provider flags are passed
in the wrong order—swap the values so they match the declaration order
(github_copilot then azure) instead of passing azure first and
copilot_configured second; update the tuple construction that currently chains
doc.get("llm")... .is_some_and(|url| !url.trim().is_empty()), copilot_configured
to use github_copilot, azure (or the matching variable names you declared) so
github_copilot aligns with its declared variable and azure aligns with its
declared variable.
- Around line 210-224: The ProviderConfig literal for "github-copilot" is
missing required fields api_version and deployment; update the struct literal in
the providers.insert block to include those fields (matching how other
ProviderConfig instances are created) — e.g. add api_version: None and
deployment: None (or the empty/default values used elsewhere in the codebase) so
the ProviderConfig for GitHub Copilot compiles alongside the existing fields
(api_type, base_url, api_key, name, use_bearer_auth, extra_headers).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc77f7ac-8040-4688-9697-eb7bd4e271f8
📒 Files selected for processing (3)
src/api/providers.rssrc/config.rssrc/config/providers.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config.rs
ibhagwan
force-pushed
the
fix/copilot-provider-endpoints
branch
3 times, most recently
from
6359cee to
a4d7f6c
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/providers.rs (1)
847-856:⚠️ Potential issue | 🔴 CriticalBackend/frontend HTTP method mismatch will cause save to fail with 405 Method Not Allowed.
The backend annotation now expects PUT, but the frontend
updateProviderfunction (interface/src/api/client.ts:1596-1606) still usesmethod: "POST":updateProvider: async (...) => { const response = await fetch(`${getApiBase()}/providers`, { method: "POST", // ← Frontend uses POST ... }); }This mismatch will break the provider update functionality. Either update the frontend to use PUT in this PR or revert the backend to POST to maintain consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/providers.rs` around lines 847 - 856, The route annotation for Provider updates in the backend (the #[utoipa::path(... put ...)] on the providers endpoint) expects HTTP PUT, but the frontend function updateProvider in interface/src/api/client.ts still sends method: "POST", causing 405 errors; fix by making the client use PUT to match the backend (or alternatively change the backend annotation back to POST), i.e., update the request method in updateProvider to "PUT" so it aligns with the utoipa #[put] path for the /providers endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/api/providers.rs`:
- Around line 847-856: The route annotation for Provider updates in the backend
(the #[utoipa::path(... put ...)] on the providers endpoint) expects HTTP PUT,
but the frontend function updateProvider in interface/src/api/client.ts still
sends method: "POST", causing 405 errors; fix by making the client use PUT to
match the backend (or alternatively change the backend annotation back to POST),
i.e., update the request method in updateProvider to "PUT" so it aligns with the
utoipa #[put] path for the /providers endpoint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de4633a5-6c01-49e6-855e-3d5f06ae9d1d
📒 Files selected for processing (3)
src/api/providers.rssrc/config.rssrc/config/providers.rs
ibhagwan
force-pushed
the
fix/copilot-provider-endpoints
branch
from
a4d7f6c to
9c40bf2
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/providers.rs (1)
210-226: Consider centralizing Copilot provider bootstrap config.This literal can drift from other Copilot config construction paths. A small shared helper for the common fields (
api_type,name, auth mode, default base URL) would reduce divergence risk over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/providers.rs` around lines 210 - 226, The GitHub Copilot provider construction in the providers.rs branch (provider == "github-copilot") duplicates common fields and may drift; extract a shared helper (e.g., a function named make_copilot_provider_config or an associated constructor like ProviderConfig::copilot_default) that returns a ProviderConfig populated with the common values (api_type = ApiType::OpenAiChatCompletions, base_url = GITHUB_COPILOT_DEFAULT_BASE_URL, name = "GitHub Copilot", use_bearer_auth = true, and other defaults), then replace the inline ProviderConfig creation here to call that helper and only override instance-specific fields such as api_key (set to credential) and any api_version/deployment if needed; ensure LlmManager::get_copilot_token usage remains compatible with the new helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/providers.rs`:
- Around line 1624-1637: The current env-var removal check only looks for a
single env_var computed as env_var = format!("{}_API_KEY",
provider.to_uppercase().replace("-", "_")) and sets env_configured accordingly;
for the "ollama" provider you must also consider OLLAMA_BASE_URL, otherwise the
handler can report removal while Ollama remains configured. Update the logic
around env_var and env_configured (and the early return that constructs
ProviderUpdateResponse) to check a list of environment variables: always check
the computed "<PROVIDER>_API_KEY", and if provider == "ollama" also check
"OLLAMA_BASE_URL"; set env_configured to true if any of these are
present/non-empty, and update the response message to mention all relevant env
var names when returning early.
---
Nitpick comments:
In `@src/api/providers.rs`:
- Around line 210-226: The GitHub Copilot provider construction in the
providers.rs branch (provider == "github-copilot") duplicates common fields and
may drift; extract a shared helper (e.g., a function named
make_copilot_provider_config or an associated constructor like
ProviderConfig::copilot_default) that returns a ProviderConfig populated with
the common values (api_type = ApiType::OpenAiChatCompletions, base_url =
GITHUB_COPILOT_DEFAULT_BASE_URL, name = "GitHub Copilot", use_bearer_auth =
true, and other defaults), then replace the inline ProviderConfig creation here
to call that helper and only override instance-specific fields such as api_key
(set to credential) and any api_version/deployment if needed; ensure
LlmManager::get_copilot_token usage remains compatible with the new helper.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ec80f32-dc5d-4249-8619-22972a087b2e
📒 Files selected for processing (3)
src/api/providers.rssrc/config.rssrc/config/providers.rs
ibhagwan
force-pushed
the
fix/copilot-provider-endpoints
branch
8 times, most recently
from
e8a76df to
a3ef45d
Compare
ibhagwan
force-pushed
the
fix/copilot-provider-endpoints
branch
from
a3ef45d to
0a9730f
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/providers.rs (2)
1525-1537:⚠️ Potential issue | 🟠 MajorDelay Copilot token cleanup until removal is actually allowed.
This clears the cached token and in-memory token before the later TOML/env checks can reject the delete. On env-backed or no-config-file setups, the endpoint returns
success: falseafter already mutating provider state. Move the Copilot cleanup onto the successful removal path.Also applies to: 1589-1659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/providers.rs` around lines 1525 - 1537, The Copilot token removal (both the filesystem removal using crate::github_copilot_auth::credentials_path(...) and the in-memory clear via llm_manager.clear_copilot_token()) is happening before the provider delete is validated; move both cleanup actions so they only run after the code path that confirms the provider was successfully removed (i.e., after the successful TOML/env checks and the branch that returns success). Update the handler logic to defer calling tokio::fs::remove_file(&token_path).await and manager.clear_copilot_token().await until inside the post-success block, and apply the same change to the other similar block around the second occurrence (the code region referenced by lines 1589-1659). Ensure you reference and modify the token_path creation and manager.clear_copilot_token() calls together so no state is mutated on failed deletes.
1589-1659:⚠️ Potential issue | 🟠 MajorThe env-backed removal fix still misses the cases from
#415.This new guard only runs after a config file has been found, so fresh installs configured purely via env vars still short-circuit with
"No config file found". And even when it does run,get_providers()still falls back toGITHUB_COPILOT_API_KEYwhenconfig.tomlexists, so Copilot remains visible after deleting the TOML key. You'll need to handle the no-config-file path here and special-case Copilot in the config-exists branch ofget_providers().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/providers.rs` around lines 1589 - 1659, The removal flow currently returns "No config file found" before checking env-backed providers and get_providers() still falls back to GITHUB_COPILOT_API_KEY when a config exists but the Copilot TOML key was removed; move or duplicate the env-var detection logic so that when config_path doesn't exist you still compute env_vars (using the same provider.as_str() branch) and check std::env::var to return the environment-variable warning instead of "No config file found", and separately update get_providers() to special-case Copilot so it does not unconditionally fall back to GITHUB_COPILOT_API_KEY when a config.toml exists but the Copilot key was removed (ensure the TOML key presence check used in has_toml_key and the secrets_store logic remains authoritative for Copilot visibility).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/api/providers.rs`:
- Around line 1525-1537: The Copilot token removal (both the filesystem removal
using crate::github_copilot_auth::credentials_path(...) and the in-memory clear
via llm_manager.clear_copilot_token()) is happening before the provider delete
is validated; move both cleanup actions so they only run after the code path
that confirms the provider was successfully removed (i.e., after the successful
TOML/env checks and the branch that returns success). Update the handler logic
to defer calling tokio::fs::remove_file(&token_path).await and
manager.clear_copilot_token().await until inside the post-success block, and
apply the same change to the other similar block around the second occurrence
(the code region referenced by lines 1589-1659). Ensure you reference and modify
the token_path creation and manager.clear_copilot_token() calls together so no
state is mutated on failed deletes.
- Around line 1589-1659: The removal flow currently returns "No config file
found" before checking env-backed providers and get_providers() still falls back
to GITHUB_COPILOT_API_KEY when a config exists but the Copilot TOML key was
removed; move or duplicate the env-var detection logic so that when config_path
doesn't exist you still compute env_vars (using the same provider.as_str()
branch) and check std::env::var to return the environment-variable warning
instead of "No config file found", and separately update get_providers() to
special-case Copilot so it does not unconditionally fall back to
GITHUB_COPILOT_API_KEY when a config.toml exists but the Copilot key was removed
(ensure the TOML key presence check used in has_toml_key and the secrets_store
logic remains authoritative for Copilot visibility).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bb9da64-cd5f-4845-987b-3e97817ff1fc
📒 Files selected for processing (3)
src/api/providers.rssrc/config.rssrc/config/providers.rs
… and remove Fix spacedriveapp#415 Three endpoint mismatches between frontend and backend caused the GitHub Copilot provider's save, test, and remove buttons to fail with 405 Method Not Allowed. These affected all providers for save. - change update_provider annotation from post to put to match frontend - fix test button URL from /providers/test to /providers/test-model - add github-copilot entry in build_test_llm_config since default_provider_config returns None for providers that require token exchange - widen GITHUB_COPILOT_DEFAULT_BASE_URL visibility to pub(crate) - add unit test for build_test_llm_config with github-copilot fix(api): Copilot provider shows as available after remove when env var is set get_providers fell back to the GITHUB_COPILOT_API_KEY env var when the TOML key was absent, so the provider stayed visible in settings after a remove — the env var can't be unset from a running process. Only check the TOML key for Copilot status in the config-exists path. The env var fallback remains for the no-config-file case (fresh install). fix(api): make Copilot provider visibility consistent with other providers - Change Copilot from TOML-only check to has_value() pattern, consistent with all other providers. Now env var GITHUB_COPILOT_API_KEY works like other provider env vars in get_providers(). - Add helpful error message in delete_provider() when attempting to remove a provider that is only configured via environment variable. The message explains which env var to unset and that a restart is required. For Ollama, checks both OLLAMA_BASE_URL and OLLAMA_API_KEY env vars. - Add comment in test explaining why github_copilot_key is excluded from all_shorthand_keys_register_providers_via_toml test (uses token exchange, not standard shorthand registration). - Extract copilot_default_provider_config() helper to reduce duplication in Copilot provider construction.
ibhagwan
force-pushed
the
fix/copilot-provider-endpoints
branch
from
0a9730f to
80333f7
Compare
2 participants
Fix #415
And remove three endpoint mismatches between frontend and backend caused the GitHub Copilot provider's save, test, and remove buttons to fail with 405 Method Not Allowed. These affected all providers for save.
fix(api): Copilot provider shows as available after remove when env var is set
get_providers fell back to the GITHUB_COPILOT_API_KEY env var when the TOML key was absent, so the provider stayed visible in settings after a remove — the env var can't be unset from a running process.
Only check the TOML key for Copilot status in the config-exists path. The env var fallback remains for the no-config-file case (fresh install).