Skip to content

feat(llm): add GitHub Copilot as a first-class LLM provider#394

Merged
jamiepine merged 2 commits intospacedriveapp:mainfrom
ibhagwan:spacebot-copilot
Mar 12, 2026
Merged

feat(llm): add GitHub Copilot as a first-class LLM provider#394
jamiepine merged 2 commits intospacedriveapp:mainfrom
ibhagwan:spacebot-copilot

Conversation

@ibhagwan
Copy link
Contributor

@ibhagwan ibhagwan commented Mar 11, 2026

Two-step auth: exchanges a GitHub PAT for a short-lived Copilot API
token, caches it to disk, and auto-refreshes when expired. The Copilot
endpoint is OpenAI-compatible so it routes through ApiType::OpenAiCompletions.

Backend:

  • New github_copilot_auth module: token exchange, disk caching (0600),
    base URL derivation from proxy-ep field, expiry handling
  • Config: github_copilot_key in [llm] section (TOML + env fallback chain:
    GITHUB_COPILOT_API_KEY, COPILOT_GITHUB_TOKEN, GH_TOKEN, GITHUB_TOKEN)
  • LlmManager: copilot_token RwLock, lazy exchange/refresh, provider
    config generation with derived base URL and bearer auth
  • Routing: defaults (channel=claude-sonnet-4, worker=gpt-4.1-mini),
    prefix mapping, priority ordering
  • API: provider status, TOML key mapping, delete handler (clears cached
    token file + in-memory state)

Frontend:

  • Settings page: add/update/remove provider entry
  • ModelSelect: provider label and ordering
  • Provider icons: GithubCopilot from @lobehub/icons

Note

This PR implements GitHub Copilot as a first-class LLM provider with automatic token exchange and refresh. The backend handles GitHub PAT → Copilot API token conversion, caches tokens to disk (restricted 0600 permissions), and auto-refreshes expired tokens. Configuration via GITHUB_COPILOT_API_KEY environment variable with TOML fallback. Frontend adds provider settings UI, model labeling, and icon support. The Copilot endpoint (OpenAI-compatible) derives its base URL from the token's proxy-ep field with fallback to default endpoint. Latest improvements: simplified env var handling, atomic file creation on Unix, proper error logging, and hardcoded editor version headers for Copilot API compliance.

Written by Tembo for commit 771504e.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 11, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds GitHub Copilot as a new LLM provider across frontend and backend: API/schema additions, UI labels/icons/settings, config/TOML/env support, provider routing, a new auth module for PAT→Copilot token exchange and caching, and LlmManager integration for token lifecycle and provider construction.

Changes

Cohort / File(s) Summary
Frontend Provider Integration
interface/src/api/client.ts, interface/src/components/ModelSelect.tsx, interface/src/lib/providerIcons.tsx, interface/src/routes/Settings.tsx
Expose github_copilot in ProviderStatus, add "github-copilot" label/order, import/map GithubCopilot icon, and add GitHub Copilot entry to Settings providers.
Backend Provider Status & API
src/api/providers.rs
Add github_copilot: bool to ProviderStatus, map github-copilotgithub_copilot_key, include it in test-config builders/get_providers/has_any, and remove cached Copilot token on provider deletion.
Config Types & TOML Schema
src/config/toml_schema.rs, src/config/types.rs
Add github_copilot_key: Option<String> to TOML and LlmConfig, redact it in Debug, include in has_any_key, and register secret mapping to GITHUB_COPILOT_API_KEY.
Config Loading & Providers
src/config/load.rs, src/config/providers.rs
Add GITHUB_COPILOT_DEFAULT_BASE_URL, resolve github_copilot_key from env/TOML, register github-copilot shorthand provider in env and TOML load paths, and include it in routing priorities.
GitHub Copilot Auth Module
src/github_copilot_auth.rs, src/lib.rs
New exported module implementing PAT hashing, PAT→Copilot token exchange (HTTP), token parsing/base-URL derivation, on-disk cache (save/load with secure perms), expiry logic, and CopilotToken type with helpers and tests.
LLM Manager & Model Routing
src/llm/manager.rs, src/llm/model.rs, src/llm/routing.rs
Add copilot_token cache to LlmManager, load/save cached token, add get_copilot_token/get_github_copilot_provider/clear_copilot_token, route github-copilot models to the provider, and add provider-specific routing defaults and prefix.
Tooling Refactor (unrelated)
src/tools/send_message_to_another_channel.rs
Consolidate Signal handling conditionals using combined pattern matching, preserving behavior while simplifying nested branches.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • jamiepine
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(llm): add GitHub Copilot as a first-class LLM provider' accurately summarizes the main change across the entire changeset, which adds comprehensive GitHub Copilot support as a complete LLM provider.
Description check ✅ Passed The description provides detailed information about the implementation including backend token exchange, caching, routing configuration, and frontend UI changes that directly correspond to the changeset modifications.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ibhagwan ibhagwan force-pushed the spacebot-copilot branch 2 times, most recently from a198f35 to d84fe73 Compare March 11, 2026 06:44
@ibhagwan
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 11, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (5)
src/github_copilot_auth.rs (2)

63-75: Creates new HTTP client per token exchange.

exchange_github_token creates a new reqwest::Client on each call. While this works, it doesn't benefit from connection pooling. Consider accepting the LlmManager's HTTP client as a parameter, or document why a fresh client is preferred here.

♻️ Proposed signature change to reuse HTTP client
-pub async fn exchange_github_token(github_pat: &str, pat_hash: String) -> Result<CopilotToken> {
-    let client = reqwest::Client::new();
+pub async fn exchange_github_token(
+    client: &reqwest::Client,
+    github_pat: &str,
+    pat_hash: String,
+) -> Result<CopilotToken> {

Then update the caller in src/llm/manager.rs to pass self.http_client().

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/github_copilot_auth.rs` around lines 63 - 75, The exchange_github_token
function currently constructs a new reqwest::Client on each call (in
exchange_github_token), causing missed connection pooling; change the signature
of exchange_github_token to accept an existing HTTP client (e.g.,
&reqwest::Client or &Client) instead of creating one inside the function, remove
the Client::new() call, and use the passed client for the request; then update
callers (notably in LlmManager or src/llm/manager.rs) to forward
self.http_client() when invoking exchange_github_token so the shared client is
reused.

176-191: File write before permission set creates brief window of exposure.

save_cached_token writes the file first, then sets restrictive permissions. On Unix systems, there's a brief window where the file may be readable by other users (depending on umask). Consider using OpenOptions with mode set at creation time.

🔒 Proposed fix for atomic secure file creation on Unix
 pub fn save_cached_token(instance_dir: &Path, token: &CopilotToken) -> Result<()> {
     let path = credentials_path(instance_dir);
     let data = serde_json::to_string_pretty(token).context("failed to serialize Copilot token")?;

-    std::fs::write(&path, &data).with_context(|| format!("failed to write {}", path.display()))?;
-
     #[cfg(unix)]
     {
-        use std::os::unix::fs::PermissionsExt;
-        std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600))
-            .with_context(|| format!("failed to set permissions on {}", path.display()))?;
+        use std::fs::OpenOptions;
+        use std::io::Write;
+        use std::os::unix::fs::OpenOptionsExt;
+        let mut file = OpenOptions::new()
+            .write(true)
+            .create(true)
+            .truncate(true)
+            .mode(0o600)
+            .open(&path)
+            .with_context(|| format!("failed to create {}", path.display()))?;
+        file.write_all(data.as_bytes())
+            .with_context(|| format!("failed to write {}", path.display()))?;
     }
+
+    #[cfg(not(unix))]
+    {
+        std::fs::write(&path, &data).with_context(|| format!("failed to write {}", path.display()))?;
+    }

     Ok(())
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/github_copilot_auth.rs` around lines 176 - 191, The current
save_cached_token writes the file then sets permissions, leaving a brief
exposure window; modify save_cached_token to create-and-write the file with
restrictive mode atomically on Unix by using std::fs::OpenOptions with
std::os::unix::fs::OpenOptionsExt::mode(0o600) (use
create(true)|truncate(true)|write(true)), write the serde_json data into that
open file (and optionally flush/fsync), and remove the separate
std::fs::set_permissions call; keep the existing serialize to data and the
credentials_path lookup, and retain the non-Unix path behavior (or fall back to
current write+set_permissions only on non-Unix platforms).
src/llm/manager.rs (2)

70-73: Inconsistent error handling between set_instance_dir and with_instance_dir.

set_instance_dir silently discards load errors via if let Ok(Some(...)), while with_instance_dir (lines 109-119) logs warnings. For consistency and debuggability, consider logging failures here as well.

♻️ Proposed fix for consistent error handling
-        if let Ok(Some(token)) = crate::github_copilot_auth::load_cached_token(&instance_dir) {
-            tracing::info!("loaded GitHub Copilot token from github_copilot_token.json");
-            *self.copilot_token.write().await = Some(token);
-        }
+        match crate::github_copilot_auth::load_cached_token(&instance_dir) {
+            Ok(Some(token)) => {
+                tracing::info!("loaded GitHub Copilot token from github_copilot_token.json");
+                *self.copilot_token.write().await = Some(token);
+            }
+            Ok(None) => {}
+            Err(error) => {
+                tracing::warn!(%error, "failed to load GitHub Copilot token");
+            }
+        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/llm/manager.rs` around lines 70 - 73, The current set_instance_dir code
silently ignores errors when calling
crate::github_copilot_auth::load_cached_token(&instance_dir); update it to
mirror with_instance_dir's behavior by matching the Result and logging warnings
on Err(e) and when Ok(None) to aid debugging, while still setting
*self.copilot_token.write().await = Some(token) on Ok(Some(token)); reference
the load_cached_token call and self.copilot_token mutation to locate and modify
the logic.

369-378: Hardcoded editor version strings may become stale.

The editor-version and editor-plugin-version headers use hardcoded values (vscode/1.96.2, copilot-chat/0.26.7). These may need periodic updates as GitHub Copilot evolves its API requirements. Consider extracting these as constants or making them configurable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/llm/manager.rs` around lines 369 - 378, The extra_headers block currently
hardcodes "editor-version" and "editor-plugin-version" values (inside the
extra_headers vector) which will become stale; refactor by replacing those
hardcoded strings with constants or configuration-driven values (e.g.,
EDITOR_VERSION and EDITOR_PLUGIN_VERSION constants or env/config fields) and
update the code that builds extra_headers to read those constants/config values
so they can be updated without editing the header literal in extra_headers.
src/config/load.rs (1)

434-438: Environment variable fallback chain may unintentionally use general-purpose GitHub tokens.

The fallback chain includes GH_TOKEN and GITHUB_TOKEN, which are commonly set for general GitHub CLI/API operations. These tokens may not have Copilot access, leading to confusing errors at runtime. Consider:

  1. Documenting this behavior clearly
  2. Only falling back to these if GITHUB_COPILOT_API_KEY and COPILOT_GITHUB_TOKEN are not set

The current implementation already does this correctly (chained or_else), but users should be warned that their GH_TOKEN will be tried for Copilot access.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/config/load.rs` around lines 434 - 438, The fallback chain for
github_copilot_key (the github_copilot_key field in load.rs) may unexpectedly
pick up general-purpose GH_TOKEN or GITHUB_TOKEN; keep the existing or_else
order but add a short code comment above github_copilot_key explaining that
GH_TOKEN/GITHUB_TOKEN are only used as last-resort fallbacks and may not grant
Copilot access, and also emit a runtime warning/log when the resolved
github_copilot_key value originated from GH_TOKEN or GITHUB_TOKEN (e.g., detect
the env var used during construction inside the same load/config initialization
and call the project logger to warn the user).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@interface/src/routes/Settings.tsx`:
- Around line 245-252: The Copilot provider card seeds the wrong default model
via provider.defaultModel which initializes modelInput and then gets applied to
every routing role on save; update the provider entry with id "github-copilot"
to use the correct defaultModel "github-copilot/gpt-4.1-mini" (or remove
defaultModel) and/or change the Settings save logic that uses modelInput to
avoid overwriting provider-specific routing defaults—only apply modelInput to
roles that have no existing model override (check the save handler that reads
modelInput and assigns to routing roles) so Copilot-specific routing defaults
are preserved.

In `@src/api/providers.rs`:
- Around line 420-421: get_providers currently uses
has_value("github_copilot_key", "GITHUB_COPILOT_API_KEY") which is inconsistent
with load_from_env's fallback list; update get_providers to consider the same
env keys used by load_from_env by checking for any of COPILOT_GITHUB_TOKEN,
GH_TOKEN, GITHUB_TOKEN in addition to GITHUB_COPILOT_API_KEY (and mirror this
change where get_providers checks copilot again, e.g., the second occurrence
around the provider status logic) so the UI reflects the same configured state
as load_from_env.

In `@src/config/providers.rs`:
- Around line 217-224: The provider test flow is incorrectly using
default_provider_config() for "github-copilot" and sending the raw PAT; update
the test path (where build_test_llm_config() / test_provider_model handle
providers) to special-case the "github-copilot" provider: call the GitHub
token-exchange flow (github_copilot_auth::exchange_github_token()) or reuse
LlmManager::get_github_copilot_provider() logic to exchange the PAT for a
Copilot token, set use_bearer_auth to true, set api_key to the exchanged token,
and derive base_url from the exchanged token's proxy-ep instead of using
GITHUB_COPILOT_DEFAULT_BASE_URL; do not call default_provider_config() for
"github-copilot" in the test path.

In `@src/config/types.rs`:
- Around line 509-513: The SecretField list currently only registers the
"github_copilot_key" -> "GITHUB_COPILOT_API_KEY" mapping; add additional
SecretField entries so SystemSecrets will resolve the other documented Copilot
env aliases (e.g. secret_name values "COPILOT_GITHUB_TOKEN", "GH_TOKEN", and
"GITHUB_TOKEN" with the same toml_key "github_copilot_key" or appropriate
toml_key variants) alongside the existing SecretField, so lookups for Copilot
honor all those environment variable names; update the entries near the existing
SecretField block to include these aliases.

---

Nitpick comments:
In `@src/config/load.rs`:
- Around line 434-438: The fallback chain for github_copilot_key (the
github_copilot_key field in load.rs) may unexpectedly pick up general-purpose
GH_TOKEN or GITHUB_TOKEN; keep the existing or_else order but add a short code
comment above github_copilot_key explaining that GH_TOKEN/GITHUB_TOKEN are only
used as last-resort fallbacks and may not grant Copilot access, and also emit a
runtime warning/log when the resolved github_copilot_key value originated from
GH_TOKEN or GITHUB_TOKEN (e.g., detect the env var used during construction
inside the same load/config initialization and call the project logger to warn
the user).

In `@src/github_copilot_auth.rs`:
- Around line 63-75: The exchange_github_token function currently constructs a
new reqwest::Client on each call (in exchange_github_token), causing missed
connection pooling; change the signature of exchange_github_token to accept an
existing HTTP client (e.g., &reqwest::Client or &Client) instead of creating one
inside the function, remove the Client::new() call, and use the passed client
for the request; then update callers (notably in LlmManager or
src/llm/manager.rs) to forward self.http_client() when invoking
exchange_github_token so the shared client is reused.
- Around line 176-191: The current save_cached_token writes the file then sets
permissions, leaving a brief exposure window; modify save_cached_token to
create-and-write the file with restrictive mode atomically on Unix by using
std::fs::OpenOptions with std::os::unix::fs::OpenOptionsExt::mode(0o600) (use
create(true)|truncate(true)|write(true)), write the serde_json data into that
open file (and optionally flush/fsync), and remove the separate
std::fs::set_permissions call; keep the existing serialize to data and the
credentials_path lookup, and retain the non-Unix path behavior (or fall back to
current write+set_permissions only on non-Unix platforms).

In `@src/llm/manager.rs`:
- Around line 70-73: The current set_instance_dir code silently ignores errors
when calling crate::github_copilot_auth::load_cached_token(&instance_dir);
update it to mirror with_instance_dir's behavior by matching the Result and
logging warnings on Err(e) and when Ok(None) to aid debugging, while still
setting *self.copilot_token.write().await = Some(token) on Ok(Some(token));
reference the load_cached_token call and self.copilot_token mutation to locate
and modify the logic.
- Around line 369-378: The extra_headers block currently hardcodes
"editor-version" and "editor-plugin-version" values (inside the extra_headers
vector) which will become stale; refactor by replacing those hardcoded strings
with constants or configuration-driven values (e.g., EDITOR_VERSION and
EDITOR_PLUGIN_VERSION constants or env/config fields) and update the code that
builds extra_headers to read those constants/config values so they can be
updated without editing the header literal in extra_headers.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 48d60412-3598-40c2-b4e4-0d16d1883433

📥 Commits

Reviewing files that changed from the base of the PR and between 0a97964 and d84fe73.

📒 Files selected for processing (15)
  • interface/src/api/client.ts
  • interface/src/components/ModelSelect.tsx
  • interface/src/lib/providerIcons.tsx
  • interface/src/routes/Settings.tsx
  • src/api/providers.rs
  • src/config/load.rs
  • src/config/providers.rs
  • src/config/toml_schema.rs
  • src/config/types.rs
  • src/github_copilot_auth.rs
  • src/lib.rs
  • src/llm/manager.rs
  • src/llm/model.rs
  • src/llm/routing.rs
  • src/tools/send_message_to_another_channel.rs

@ibhagwan
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 11, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/llm/manager.rs`:
- Line 386: Update the shorthand GitHub Copilot provider registration to use the
chat completions API type: change the provider's ApiType from
ApiType::OpenAiCompletions to ApiType::OpenAiChatCompletions so the runtime and
shorthand configurations match and the provider uses the /chat/completions
endpoint; modify the registration code that constructs the provider (the
shorthand provider registration in load configuration) to set api_type =
ApiType::OpenAiChatCompletions, ensuring any related enum usages or matches
expect the chat variant.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 77fa6b34-af68-4941-8aee-03d977cb19ce

📥 Commits

Reviewing files that changed from the base of the PR and between d84fe73 and b7e7be7.

📒 Files selected for processing (4)
  • src/config/load.rs
  • src/config/providers.rs
  • src/github_copilot_auth.rs
  • src/llm/manager.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/config/load.rs

@ibhagwan
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 11, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@ibhagwan ibhagwan marked this pull request as ready for review March 11, 2026 16:00
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/github_copilot_auth.rs (1)

114-129: Consider documenting the heuristic threshold.

The 10 billion threshold for distinguishing seconds from milliseconds works for dates through ~2286 (in seconds) or ~1970+116 days (in milliseconds). This is reasonable, but a brief inline comment explaining the boundary would help future maintainers.

📝 Suggested documentation
 fn parse_expires_at(value: &serde_json::Value) -> Option<i64> {
     let raw = match value {
         serde_json::Value::Number(number) => number.as_i64()?,
         serde_json::Value::String(string) => string.trim().parse::<i64>().ok()?,
         _ => return None,
     };
 
+    // Heuristic: 10^10 seconds ≈ year 2286, so values above this are likely milliseconds.
+    // This safely handles both formats for any reasonable token expiry.
     if raw > 10_000_000_000 {
         Some(raw) // already milliseconds
     } else {
         Some(raw * 1000) // convert seconds to milliseconds
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/github_copilot_auth.rs` around lines 114 - 129, Add a concise inline
comment in parse_expires_at explaining the 10_000_000_000 heuristic: state that
values >10 billion are treated as milliseconds because 10_000_000_000 seconds
corresponds to a date around year 2286, so smaller values are likely seconds and
are converted to milliseconds by multiplying by 1000; reference the function
name parse_expires_at and place the comment near the threshold check to make the
rationale clear for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/github_copilot_auth.rs`:
- Around line 114-129: Add a concise inline comment in parse_expires_at
explaining the 10_000_000_000 heuristic: state that values >10 billion are
treated as milliseconds because 10_000_000_000 seconds corresponds to a date
around year 2286, so smaller values are likely seconds and are converted to
milliseconds by multiplying by 1000; reference the function name
parse_expires_at and place the comment near the threshold check to make the
rationale clear for future maintainers.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 17c39d16-9859-4f41-b116-e5a2e89f95bc

📥 Commits

Reviewing files that changed from the base of the PR and between 771504e and d62a5a0.

📒 Files selected for processing (2)
  • src/github_copilot_auth.rs
  • src/llm/manager.rs

@ibhagwan
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 11, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/github_copilot_auth.rs (1)

234-305: Good test coverage for core functionality.

The tests cover the main paths including expiry parsing, hashing determinism, and URL derivation.

Consider adding a test for the security validation (rejecting hostnames that don't end with .githubcopilot.com):

🧪 Optional test for security validation
#[test]
fn derive_base_url_rejects_invalid_suffix() {
    let token = "tid=abc123;proxy-ep=proxy.evil.com;st=dotcom";
    assert_eq!(derive_base_url_from_token(token), None);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/github_copilot_auth.rs` around lines 234 - 305, Add a unit test to cover
the security validation that rejects proxy endpoints not ending with
".githubcopilot.com": create a test named like
derive_base_url_rejects_invalid_suffix that calls derive_base_url_from_token
with a token containing "proxy-ep=proxy.evil.com" (and other fields like tid/st)
and asserts the result is None; this ensures the hostname suffix check in
derive_base_url_from_token is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/github_copilot_auth.rs`:
- Around line 234-305: Add a unit test to cover the security validation that
rejects proxy endpoints not ending with ".githubcopilot.com": create a test
named like derive_base_url_rejects_invalid_suffix that calls
derive_base_url_from_token with a token containing "proxy-ep=proxy.evil.com"
(and other fields like tid/st) and asserts the result is None; this ensures the
hostname suffix check in derive_base_url_from_token is enforced.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a28145af-97e7-49dc-b1eb-1efa4f8d84b0

📥 Commits

Reviewing files that changed from the base of the PR and between d62a5a0 and 29a7ae3.

📒 Files selected for processing (2)
  • src/github_copilot_auth.rs
  • src/llm/manager.rs

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/llm/manager.rs (2)

22-28: Hardcoded editor/plugin versions will become stale over time.

These version strings mimic a specific VSCode/Copilot Chat release. When GitHub updates their API or deprecates older client versions, requests may start failing. Consider making these configurable or documenting the update process.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/llm/manager.rs` around lines 22 - 28, The hardcoded constants
COPILOT_EDITOR_VERSION and COPILOT_EDITOR_PLUGIN_VERSION can become stale;
change them to configurable values (e.g., read from environment variables or a
config struct) with sensible defaults equal to the current constants, update any
places that reference these constants to use the new config fields or accessor
functions, and add documentation/comments indicating where to update the
defaults when GitHub/Copilot versions change so the values can be rotated
without code edits.

412-415: Consider clarifying that the disk cache is not deleted.

This method only clears the in-memory token. The on-disk cache file persists and will be reloaded on the next manager initialization. This is fine if delete_provider always handles file deletion first (as shown in src/api/providers.rs:929-934), but the docstring could be more explicit to prevent misuse.

📝 Suggested docstring clarification
-    /// Clear cached GitHub Copilot token from memory.
+    /// Clear cached GitHub Copilot token from memory only.
+    ///
+    /// Note: Does not delete the on-disk cache file. Use
+    /// `github_copilot_auth::credentials_path()` and delete the file separately
+    /// if persistent removal is needed (e.g., in `delete_provider`).
     pub async fn clear_copilot_token(&self) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/llm/manager.rs` around lines 412 - 415, Update the docstring for the
method clear_copilot_token to explicitly state it only clears the in-memory
Copilot token and does not delete any on-disk cache file (the on-disk token will
persist and be reloaded on next Manager initialization); mention that callers
should use delete_provider (or an equivalent file-deleting routine) if they need
the disk cache removed as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/llm/manager.rs`:
- Around line 22-28: The hardcoded constants COPILOT_EDITOR_VERSION and
COPILOT_EDITOR_PLUGIN_VERSION can become stale; change them to configurable
values (e.g., read from environment variables or a config struct) with sensible
defaults equal to the current constants, update any places that reference these
constants to use the new config fields or accessor functions, and add
documentation/comments indicating where to update the defaults when
GitHub/Copilot versions change so the values can be rotated without code edits.
- Around line 412-415: Update the docstring for the method clear_copilot_token
to explicitly state it only clears the in-memory Copilot token and does not
delete any on-disk cache file (the on-disk token will persist and be reloaded on
next Manager initialization); mention that callers should use delete_provider
(or an equivalent file-deleting routine) if they need the disk cache removed as
well.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 57353777-d27f-4f5f-a683-b43041700dd4

📥 Commits

Reviewing files that changed from the base of the PR and between d62a5a0 and 29a7ae3.

📒 Files selected for processing (2)
  • src/github_copilot_auth.rs
  • src/llm/manager.rs

Two-step auth: exchanges a GitHub PAT for a short-lived Copilot API
token, caches it to disk, and auto-refreshes when expired. The Copilot
endpoint is OpenAI-compatible so it routes through ApiType::OpenAiCompletions.

Backend:
- New github_copilot_auth module: token exchange, disk caching (0600),
  base URL derivation from proxy-ep field, expiry handling
- Config: github_copilot_key in [llm] section (TOML + env:GITHUB_COPILOT_API_KEY)
- LlmManager: copilot_token RwLock, lazy exchange/refresh, provider
  config generation with derived base URL and bearer auth
- Routing: defaults (channel=claude-sonnet-4, worker=gpt-4.1-mini),
  prefix mapping, priority ordering
- API: provider status, TOML key mapping, delete handler (clears cached
  token file + in-memory state)

Frontend:
- Settings page: add/update/remove provider entry
- ModelSelect: provider label and ordering
- Provider icons: GithubCopilot from @lobehub/icons
@jamiepine jamiepine merged commit 8195de9 into spacedriveapp:main Mar 12, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants