fix(api): provider endpoint mismatches preventing copilot save#511
fix(api): provider endpoint mismatches preventing copilot save#511ibhagwan wants to merge 2 commits 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 registered in test LLM configs when missing; add pre-removal checks that detect copilot API key presence in TOML or environment before deleting provider; expose Copilot base URL at crate scope and add a unit test verifying registration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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
… 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).
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
5 times, most recently
from
7838f0d to
0a5ea4f
Compare
…iders - 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
0a5ea4f to
36ace52
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).