feat(llm): add GitHub Copilot chat backend + model discovery - #185
Merged
Conversation
Add a third ChatClient backend that routes LLM calls (query expansion, explain, community naming, memory extraction) through a GitHub Copilot subscription, and make the OpenAI-compatible backend the default. - CopilotChatClient: drives the official `copilot` CLI over JSON-RPC via github-copilot-sdk. The CLI owns OAuth device-flow login and token refresh; COPILOT_HOME is scoped to <data-dir>/copilot. - config.json: persist the selected Copilot model (and optional token) at <data-dir>/config.json. - `codesearch copilot login|models|status`: login opens a ratatui model picker and saves the choice; models/status inspect the account. - serve: GET /api/llm/models lists the active backend's models (?target=openai|copilot); explain/index streams accept a `model` override for on-the-fly selection. - OpenAiChatClient::list_models() discovers models via GET /v1/models. - Default --llm-target flips from anthropic to open-ai everywhere (CLI, container, serve/stream request body). Anthropic stays selectable. The github-copilot-sdk dep uses default-features = false (no bundled CLI); set COPILOT_SKIP_CLI_DOWNLOAD=1 at build time to skip its CLI download. A generic ACP-based ChatClient is noted as a follow-up.
Contributor
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds GitHub Copilot and OpenAI-compatible LLM backends with persisted configuration, direct HTTP clients, CLI management commands, model discovery, request-level streaming selection, and integration across container, controller, and management flows. ChangesLLM backend integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CopilotCommand
participant GitHubOAuth
participant CodesearchConfig
participant CopilotChatClient
User->>CopilotCommand: run copilot login
CopilotCommand->>GitHubOAuth: request device code and poll token
GitHubOAuth-->>CopilotCommand: return access token
CopilotCommand->>CodesearchConfig: save token
CopilotCommand->>CopilotChatClient: list models
CopilotChatClient-->>CopilotCommand: return model metadata
CopilotCommand->>User: display model picker
User-->>CopilotCommand: choose model
CopilotCommand->>CodesearchConfig: save model
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Apply /simplify cleanups to the Copilot backend, no behavior change: - Add LlmTarget::as_str + FromStr in cli/mod.rs; the /api/llm/models handler now uses them instead of local parse_target/target_name. - Fold build_copilot_client's model-override into CopilotChatClient::from_data_dir_with_model, removing the duplicated config-load helper in streaming.rs. This also fixes serve-mode Copilot to use the same COPILOT_HOME as the CLI command. - Add CodesearchConfig::load_copilot to load + default the Copilot section in one step; use it in from_data_dir_with_model and status. - Model picker moves `models` into the blocking task and returns the chosen entry, cloning one Model instead of the whole Vec.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/connector/api/copilot_command/picker.rs (1)
115-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid relying on
Debugformatting for user-facing category text.
format!("{category:?}").to_lowercase()depends on the SDK enum's derivedDebugoutput, which isn't guaranteed stable and can produce run-on text for multi-word variants (e.g."generalpurpose").♻️ Proposed fix
- if let Some(category) = &m.model_picker_category { - lines.push(field("Category", &format!("{category:?}").to_lowercase())); - } + if let Some(category) = &m.model_picker_category { + lines.push(field("Category", category.as_ref())); + }(Adjust to whatever display/accessor the SDK's
ModelPickerCategorytype actually exposes — worth checking the crate docs for a purpose-builtDisplay/label rather thanDebug.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connector/api/copilot_command/picker.rs` around lines 115 - 117, Update the Category rendering in the model picker response to use the SDK’s purpose-built Display implementation or label/accessor for ModelPickerCategory instead of Debug formatting and lowercasing. Preserve the existing field output while ensuring multi-word categories retain stable, user-facing spacing and capitalization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/mod.rs`:
- Around line 269-271: Update the clap doc comments for all three `llm`
arguments in the memory import, memory add, and explain command definitions to
accurately state that `open-ai` is the default and include `copilot` among the
supported options. Keep the existing `LlmTarget` argument configuration and
defaults unchanged.
In `@src/connector/adapter/codesearch_config.rs`:
- Around line 34-45: Harden the persisted Copilot configuration containing
CopilotConfig::github_token by applying owner-only permissions to the
config.json file immediately after its std::fs::write creation in the config
persistence flow. Use Unix-specific permissions (mode 0600), while preserving
the existing write behavior and handling permission-setting errors consistently
with the surrounding code.
In `@src/connector/adapter/copilot_chat_client.rs`:
- Around line 209-249: Handle errors from events.recv().await explicitly in the
collector spawned around the event loop instead of using while let Ok. Preserve
normal event processing, but when receiving returns Err (including Lagged or
Closed), set error to a descriptive message containing the receive error before
exiting, so run_turn does not treat partial streamed output as successful.
- Around line 173-188: Update session_config to initialize SessionConfig with
deny_all_permissions() instead of approve_all_permissions(), while preserving
the existing streaming, system-message, and model configuration.
In `@src/connector/adapter/management/handlers/llm.rs`:
- Around line 77-88: Wrap the CopilotChatClient::list_models() await in the
LlmTarget::Copilot branch with the project’s established timeout mechanism,
ensuring stalled CLI/SDK calls terminate and propagate the timeout error through
the existing handler flow. Preserve the current model mapping and collection
behavior after a successful call.
In `@src/connector/adapter/management/streaming.rs`:
- Around line 102-114: Update build_copilot_client to scope CopilotChatClient
initialization to the provided data_dir, matching
CopilotChatClient::from_data_dir by binding COPILOT_HOME to <data_dir>/copilot
before constructing the client. Preserve the existing config loading and model
override behavior.
In `@src/connector/adapter/openai_chat_client.rs`:
- Around line 191-197: Update the non-success response handling in the OpenAI
models request to avoid silently discarding failures from resp.text().await:
preserve the response body when reading succeeds, and log the text-read error
with tracing::warn! or tracing::error! before falling back to an empty body.
In `@src/connector/api/copilot_command.rs`:
- Around line 126-144: Update the auth_status handling in status to log the
error with tracing::warn! or tracing::error! before converting it to None, while
preserving the existing auth_line mapping and generic unreachable message.
---
Nitpick comments:
In `@src/connector/api/copilot_command/picker.rs`:
- Around line 115-117: Update the Category rendering in the model picker
response to use the SDK’s purpose-built Display implementation or label/accessor
for ModelPickerCategory instead of Debug formatting and lowercasing. Preserve
the existing field output while ensuring multi-word categories retain stable,
user-facing spacing and capitalization.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f72d4297-e351-48eb-ad79-c285f28dd324
📒 Files selected for processing (23)
AGENTS.mdCargo.tomlsrc/cli/mod.rssrc/connector/adapter/codesearch_config.rssrc/connector/adapter/copilot_chat_client.rssrc/connector/adapter/management/handlers/llm.rssrc/connector/adapter/management/handlers/mod.rssrc/connector/adapter/management/server.rssrc/connector/adapter/management/streaming.rssrc/connector/adapter/mod.rssrc/connector/adapter/openai_chat_client.rssrc/connector/api/container.rssrc/connector/api/controller/clusters_controller.rssrc/connector/api/controller/explain_controller.rssrc/connector/api/controller/memory_controller.rssrc/connector/api/controller/mod.rssrc/connector/api/controller/symbol_clusters_controller.rssrc/connector/api/copilot_command.rssrc/connector/api/copilot_command/picker.rssrc/connector/api/mod.rssrc/connector/api/router.rssrc/lib.rssrc/main.rs
Comment on lines
+77
to
+88
| LlmTarget::Copilot => { | ||
| let client = CopilotChatClient::from_data_dir(state.container.data_dir())?; | ||
| client | ||
| .list_models() | ||
| .await? | ||
| .into_iter() | ||
| .map(|m| ModelInfo { | ||
| id: m.id, | ||
| name: Some(m.name), | ||
| }) | ||
| .collect() | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect whether the Copilot client enforces any timeout around CLI/session calls.
fd copilot_chat_client.rs --exec cat -n {}Repository: ArtemisMucaj/codesearch
Length of output: 14756
Add a timeout around CopilotChatClient::list_models() list_models() still reaches the Copilot CLI/SDK without a timeout, so a stalled call can hang the /api/llm/models request indefinitely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/connector/adapter/management/handlers/llm.rs` around lines 77 - 88, Wrap
the CopilotChatClient::list_models() await in the LlmTarget::Copilot branch with
the project’s established timeout mechanism, ensuring stalled CLI/SDK calls
terminate and propagate the timeout error through the existing handler flow.
Preserve the current model mapping and collection behavior after a successful
call.
Comment on lines
+126
to
+144
| async fn status(data_dir: &str) -> Result<String> { | ||
| let cfg = CodesearchConfig::load(data_dir)?; | ||
| let selected = cfg | ||
| .copilot | ||
| .as_ref() | ||
| .and_then(|c| c.model.clone()) | ||
| .unwrap_or_else(|| "(none — CLI default)".to_string()); | ||
|
|
||
| let auth = client(data_dir)?.auth_status().await.ok(); | ||
| let auth_line = match auth { | ||
| Some(a) if a.is_authenticated => { | ||
| format!( | ||
| "Authenticated as {}", | ||
| a.login.as_deref().unwrap_or("(unknown)") | ||
| ) | ||
| } | ||
| Some(_) => "Not authenticated (run `codesearch copilot login`)".to_string(), | ||
| None => "Copilot CLI unreachable (is `copilot` installed?)".to_string(), | ||
| }; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Log the swallowed auth_status error before dropping it.
.ok() on auth_status() silently discards the actual failure reason, leaving only the generic "Copilot CLI unreachable" message. Per coding guidelines, errors should be logged before being dropped.
🪵 Proposed fix
- let auth = client(data_dir)?.auth_status().await.ok();
+ let auth = match client(data_dir)?.auth_status().await {
+ Ok(a) => Some(a),
+ Err(e) => {
+ tracing::warn!("failed to query Copilot auth status: {e}");
+ None
+ }
+ };As per coding guidelines, "Do not silently swallow errors; log with tracing::warn! or tracing::error! before dropping an error."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async fn status(data_dir: &str) -> Result<String> { | |
| let cfg = CodesearchConfig::load(data_dir)?; | |
| let selected = cfg | |
| .copilot | |
| .as_ref() | |
| .and_then(|c| c.model.clone()) | |
| .unwrap_or_else(|| "(none — CLI default)".to_string()); | |
| let auth = client(data_dir)?.auth_status().await.ok(); | |
| let auth_line = match auth { | |
| Some(a) if a.is_authenticated => { | |
| format!( | |
| "Authenticated as {}", | |
| a.login.as_deref().unwrap_or("(unknown)") | |
| ) | |
| } | |
| Some(_) => "Not authenticated (run `codesearch copilot login`)".to_string(), | |
| None => "Copilot CLI unreachable (is `copilot` installed?)".to_string(), | |
| }; | |
| async fn status(data_dir: &str) -> Result<String> { | |
| let cfg = CodesearchConfig::load(data_dir)?; | |
| let selected = cfg | |
| .copilot | |
| .as_ref() | |
| .and_then(|c| c.model.clone()) | |
| .unwrap_or_else(|| "(none — CLI default)".to_string()); | |
| let auth = match client(data_dir)?.auth_status().await { | |
| Ok(a) => Some(a), | |
| Err(e) => { | |
| tracing::warn!("failed to query Copilot auth status: {e}"); | |
| None | |
| } | |
| }; | |
| let auth_line = match auth { | |
| Some(a) if a.is_authenticated => { | |
| format!( | |
| "Authenticated as {}", | |
| a.login.as_deref().unwrap_or("(unknown)") | |
| ) | |
| } | |
| Some(_) => "Not authenticated (run `codesearch copilot login`)".to_string(), | |
| None => "Copilot CLI unreachable (is `copilot` installed?)".to_string(), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/connector/api/copilot_command.rs` around lines 126 - 144, Update the
auth_status handling in status to log the error with tracing::warn! or
tracing::error! before converting it to None, while preserving the existing
auth_line mapping and generic unreachable message.
Source: Coding guidelines
Apply CodeRabbit review findings on the Copilot backend: - Deny tool permissions in Copilot sessions (deny_all_permissions instead of approve_all_permissions). These one-shot search prompts never invoke tools; failing closed removes a prompt-injection side-effect path. The DenyAll policy answers instantly, so the model just proceeds without the tool. - Restrict <data_dir>/config.json to 0600 on Unix after write — it can hold a GitHub OAuth token and must not inherit a world/group-readable umask. - Surface Copilot event-channel errors: a Closed/Lagged before the terminal assistant.message now returns an error instead of passing partial streamed text off as a complete response. - Bound list_models() with a 30s timeout so a stalled CLI can't hang /api/llm/models or the login picker. - Fix stale --llm help text on memory import/add + explain: now says 'open-ai (default), anthropic, or copilot'.
Owner
Author
|
Addressed the review findings in c27f059:
461 tests pass, clippy clean (no new warnings), fmt clean. |
Contributor
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/connector/adapter/management/streaming.rs (1)
175-175: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the container's configured LLM target as the default.
If the client request omits the
llmtarget,unwrap_or_default()falls back to theLlmTargetenum's default (typically OpenAI), bypassing the actual default configured for the container (e.g., if the server was started with--llm-target copilot). This will break the explain stream by incorrectly initializing the wrong backend.Pass the container's configured default target down from the
explain_streamhandler (where it's accessible viastate.container.llm_target()) and use it as the fallback here.🐛 Proposed fix for the fallback logic
Update the
run_explain_streamfunction signature to accept adefault_llm: LlmTargetargument (passed fromexplain_stream), and apply it here:- let llm: LlmTarget = req.llm.map(Into::into).unwrap_or_default(); + let llm: LlmTarget = req.llm.map(Into::into).unwrap_or(default_llm);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connector/adapter/management/streaming.rs` at line 175, Update run_explain_stream to accept a default_llm: LlmTarget argument, pass state.container.llm_target() from the explain_stream handler, and use default_llm instead of unwrap_or_default() when req.llm is absent.
🧹 Nitpick comments (2)
src/connector/adapter/management/streaming.rs (1)
205-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant cloning of owned variables.
These variables are cloned before moving into the
async moveclosure, but the original variables (symbol,chat_client) are never used again after the closure. Furthermore, we can just consume therepositoryandregexfields directly fromreqinstead of cloning them. As per coding guidelines, avoid unnecessaryclone()calls.♻️ Proposed refactor
- let symbol_c = symbol.clone(); - let repo_c = req.repository.clone(); - let is_regex = req.regex; - let client_c = chat_client.clone(); + let repository = req.repository; + let is_regex = req.regex; let work = tokio::spawn(async move { use_case .execute_streaming( - &symbol_c, - repo_c.as_deref(), - client_c.as_ref(), + &symbol, + repository.as_deref(), + chat_client.as_ref(), is_regex, token_tx, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connector/adapter/management/streaming.rs` around lines 205 - 218, Remove the redundant symbol_c, repo_c, is_regex, and client_c assignments before the tokio::spawn closure. Move the owned symbol and chat_client values directly into the async block, and consume req.repository and req.regex there without cloning; update execute_streaming to use those moved values while preserving its existing arguments and behavior.Source: Coding guidelines
src/connector/adapter/management/handlers/llm.rs (1)
77-79: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueWrap synchronous disk I/O in
spawn_blocking.Both sites call Copilot initialization functions that read the
config.jsonfile from disk synchronously on the async request thread. As per coding guidelines, wrap blocking calls intokio::task::spawn_blockingto avoid stalling the async reactor.
src/connector/adapter/management/handlers/llm.rs#L77-L79: wrapCopilotChatClient::from_data_dirintokio::task::spawn_blocking.src/connector/adapter/management/streaming.rs#L188-L190: wrapCopilotChatClient::from_data_dir_with_modelintokio::task::spawn_blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connector/adapter/management/handlers/llm.rs` around lines 77 - 79, Wrap the synchronous Copilot initialization calls in tokio::task::spawn_blocking and await their results, preserving existing error propagation: update CopilotChatClient::from_data_dir in src/connector/adapter/management/handlers/llm.rs lines 77-79 and CopilotChatClient::from_data_dir_with_model in src/connector/adapter/management/streaming.rs lines 188-190.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/connector/adapter/management/streaming.rs`:
- Line 175: Update run_explain_stream to accept a default_llm: LlmTarget
argument, pass state.container.llm_target() from the explain_stream handler, and
use default_llm instead of unwrap_or_default() when req.llm is absent.
---
Nitpick comments:
In `@src/connector/adapter/management/handlers/llm.rs`:
- Around line 77-79: Wrap the synchronous Copilot initialization calls in
tokio::task::spawn_blocking and await their results, preserving existing error
propagation: update CopilotChatClient::from_data_dir in
src/connector/adapter/management/handlers/llm.rs lines 77-79 and
CopilotChatClient::from_data_dir_with_model in
src/connector/adapter/management/streaming.rs lines 188-190.
In `@src/connector/adapter/management/streaming.rs`:
- Around line 205-218: Remove the redundant symbol_c, repo_c, is_regex, and
client_c assignments before the tokio::spawn closure. Move the owned symbol and
chat_client values directly into the async block, and consume req.repository and
req.regex there without cloning; update execute_streaming to use those moved
values while preserving its existing arguments and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8abca5ee-5a06-49fb-a896-7c1386d9742a
📒 Files selected for processing (6)
src/cli/mod.rssrc/connector/adapter/codesearch_config.rssrc/connector/adapter/copilot_chat_client.rssrc/connector/adapter/management/handlers/llm.rssrc/connector/adapter/management/streaming.rssrc/connector/api/copilot_command.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/connector/adapter/codesearch_config.rs
- src/connector/api/copilot_command.rs
- src/connector/adapter/copilot_chat_client.rs
Replace the github-copilot-sdk (which spawned the `copilot` CLI over JSON-RPC) with a direct HTTP integration, matching how tools like opencode do it. - copilot_auth: hand-rolled GitHub OAuth device flow (POST /login/device/code, poll /login/oauth/access_token per RFC 8628, handling authorization_pending/slow_down) using the public VS Code Copilot client id. Returns the ghu_ token. - CopilotChatClient: direct reqwest client to https://api.githubcopilot.com with the ghu_ token as Bearer + the Copilot headers (Copilot-Integration-Id, Editor-Version, X-GitHub-Api-Version, Openai-Intent). Chat/streaming delegate to OpenAiChatClient::with_parts so the OpenAI-compatible request/SSE logic is shared, not duplicated. list_models via GET /models with an own CopilotModel type. - copilot login now runs the device flow itself (prints code + URL, polls, stores the token 0600), then the model picker — no external CLI, fully self-contained login. - Drop the github-copilot-sdk dependency and the COPILOT_SKIP_CLI_DOWNLOAD build note; update docs. BREAKING CHANGE: the Copilot backend no longer uses the `copilot` CLI; `codesearch copilot login` performs the OAuth device flow directly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/connector/adapter/copilot_chat_client.rs (1)
26-44: 🔒 Security & Privacy | 🔵 TrivialReverse-engineered Copilot API access carries GitHub ToS/stability risk. Both sites implement the same unofficial mechanism — presenting codesearch as the VS Code Copilot Chat extension to satisfy GitHub's Copilot API gating. This is a known community pattern, but GitHub does not support or endorse it, and community reports describe account suspensions for non-editor/proxy usage detected by GitHub. Since this is intentional and central to the PR, no code change is required, but worth confirming the team has accepted this operational risk (headers/client id could change or be blocked without notice) and communicated it to users (e.g. in
codesearch copilot loginoutput or docs).
src/connector/adapter/copilot_chat_client.rs#L26-L44: theEditor-Version/Editor-Plugin-Version/User-Agent/Copilot-Integration-Idvalues impersonate the VS Code Copilot Chat extension; confirm this is an accepted risk and consider surfacing a disclaimer to users.src/connector/adapter/copilot_auth.rs#L26-L36:CLIENT_IDreuses VS Code's public OAuth client id for the device flow, part of the same unofficial-access approach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connector/adapter/copilot_chat_client.rs` around lines 26 - 44, No direct code change is required at src/connector/adapter/copilot_chat_client.rs lines 26-44 or src/connector/adapter/copilot_auth.rs lines 26-36; confirm team acceptance of the unofficial Copilot access and impersonation risk, and consider surfacing a user disclaimer through copilot login output or documentation.src/connector/adapter/copilot_auth.rs (1)
115-177: 📐 Maintainability & Code Quality | 🔵 TrivialConsider
#[tracing::instrument]on the polling loop.
poll_for_tokenis the longest-running, most failure-prone step of login; a debug span would make timing/retry issues easier to diagnose alongside the existingdebug!calls.As per coding guidelines, "Instrument meaningful async functions with
#[tracing::instrument]when a debugging span is useful."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connector/adapter/copilot_auth.rs` around lines 115 - 177, Add a tracing instrumentation attribute to the async function poll_for_token, creating a debug span for the polling operation while avoiding sensitive device or token data in captured fields. Preserve the existing polling logic and debug messages unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/connector/adapter/copilot_auth.rs`:
- Around line 80-177: Define or reuse a request-timeout constant such as
REQUEST_TIMEOUT, then apply it to the request builders in both
request_device_code and poll_for_token, including the .post(ACCESS_TOKEN_URL)
call. Keep the existing error mapping and polling behavior unchanged while
ensuring each external request terminates when the timeout is reached.
---
Nitpick comments:
In `@src/connector/adapter/copilot_auth.rs`:
- Around line 115-177: Add a tracing instrumentation attribute to the async
function poll_for_token, creating a debug span for the polling operation while
avoiding sensitive device or token data in captured fields. Preserve the
existing polling logic and debug messages unchanged.
In `@src/connector/adapter/copilot_chat_client.rs`:
- Around line 26-44: No direct code change is required at
src/connector/adapter/copilot_chat_client.rs lines 26-44 or
src/connector/adapter/copilot_auth.rs lines 26-36; confirm team acceptance of
the unofficial Copilot access and impersonation risk, and consider surfacing a
user disclaimer through copilot login output or documentation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19498288-e6bd-43e8-a7b8-d1abe5e655c3
📒 Files selected for processing (9)
AGENTS.mdCargo.tomlsrc/connector/adapter/codesearch_config.rssrc/connector/adapter/copilot_auth.rssrc/connector/adapter/copilot_chat_client.rssrc/connector/adapter/mod.rssrc/connector/adapter/openai_chat_client.rssrc/connector/api/copilot_command.rssrc/connector/api/copilot_command/picker.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- AGENTS.md
- src/connector/adapter/openai_chat_client.rs
- src/connector/adapter/codesearch_config.rs
Generalize the OpenAI backend beyond a single OPENAI_BASE_URL to
several named endpoints (LM Studio, vLLM, hosted OpenAI, …), selectable
and configurable from both the CLI and the serve management API.
- config: `openai` section with an `endpoints` map ({base_url, model,
api_key}) and an `active` name. resolve_openai_endpoint() picks the
override, else active, else falls back to OPENAI_* env. Round-trip +
precedence tests.
- OpenAiChatClient::from_config()/from_endpoint(): build against a named
endpoint; from_env delegates to from_endpoint (shared header/client
logic). All dispatch sites (build_chat_client, container expander,
serve stream, /api/llm/models) resolve via config.
- serve management API: GET /api/llm/endpoints (keys masked, has_key),
PUT /api/llm/endpoints/{name} (write-only api_key), POST /api/llm/active;
GET /api/llm/models and the explain stream take ?endpoint=/endpoint.
- CLI: `codesearch openai add|use|endpoints|models|select`; select reuses
a ratatui picker over model ids. Handled early in main (no container),
like `copilot`.
Also add per-request timeouts to the Copilot device-flow HTTP calls so a
stalled github.com connection can't hang login (CodeRabbit).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/connector/adapter/codesearch_config.rs (1)
161-169: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReturn a reference to avoid unnecessary cloning.
OpenAiEndpointcontains multiple strings. Since downstream callers (like the chat client constructor) only need to read these fields, returning a reference avoids an unnecessary allocation. As per coding guidelines, prefer borrowing where sufficient and avoid unnecessaryclone()calls.♻️ Proposed refactor
- pub fn resolve_openai_endpoint(&self, name_override: Option<&str>) -> Option<OpenAiEndpoint> { + pub fn resolve_openai_endpoint(&self, name_override: Option<&str>) -> Option<&OpenAiEndpoint> { let openai = self.openai.as_ref()?; let name = name_override.or(openai.active.as_deref())?; - openai.endpoints.get(name).cloned() + openai.endpoints.get(name) }Since the returned type changes to a reference, you will also need to update the
resolve_openai_endpoint_precedencetest further down in this file to borrow thebase_url:// Explicit override wins over active. assert_eq!( cfg.resolve_openai_endpoint(Some("b")).unwrap().base_url.as_str(), "http://b" ); // Falls back to active when no override. assert_eq!( cfg.resolve_openai_endpoint(None).unwrap().base_url.as_str(), "http://a" );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connector/adapter/codesearch_config.rs` around lines 161 - 169, Update resolve_openai_endpoint to return Option<&OpenAiEndpoint> and borrow the matching endpoint from openai.endpoints instead of cloning it. Adjust downstream read-only callers to accept the borrowed result, and update the resolve_openai_endpoint_precedence test assertions to borrow base_url with as_str() as shown.Source: Coding guidelines
src/connector/api/openai_command/picker.rs (1)
66-69: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid unnecessary string allocations in the render loop.
As per coding guidelines, prefer borrowing where sufficient to avoid unnecessary
clone()calls. Sinceratatuiwidgets can borrow data for the duration of the frame, you can pass a string slice toLine::frominstead of cloning the model ID on every render cycle.♻️ Proposed refactor
let items: Vec<ListItem> = ids .iter() - .map(|id| ListItem::new(Line::from(id.clone()))) + .map(|id| ListItem::new(Line::from(id.as_str()))) .collect();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connector/api/openai_command/picker.rs` around lines 66 - 69, Update the ListItem construction in the ids iteration to pass a borrowed string slice to Line::from instead of cloning each id. Preserve the existing item ordering and rendering behavior while removing the per-frame allocation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/connector/adapter/management/handlers/llm.rs`:
- Line 1: Prevent synchronous configuration and client initialization from
blocking Tokio workers: update CodesearchConfig::load/save and the
from_config/from_data_dir initializers in the LLM handlers to run via
spawn_blocking, apply the same treatment to OpenAiChatClient::from_config and
CodesearchConfig operations in the async openai command handlers, and dispatch
build_chat_client through spawn_blocking in the async controller methods that
invoke it.
- Around line 199-202: Update the endpoint activation logic in the handler
around openai.active so a newly registered endpoint becomes active when either
body.set_active is true or no active endpoint currently exists. Preserve the
existing configuration save flow and avoid replacing an already active endpoint
unless explicitly requested.
---
Nitpick comments:
In `@src/connector/adapter/codesearch_config.rs`:
- Around line 161-169: Update resolve_openai_endpoint to return
Option<&OpenAiEndpoint> and borrow the matching endpoint from openai.endpoints
instead of cloning it. Adjust downstream read-only callers to accept the
borrowed result, and update the resolve_openai_endpoint_precedence test
assertions to borrow base_url with as_str() as shown.
In `@src/connector/api/openai_command/picker.rs`:
- Around line 66-69: Update the ListItem construction in the ids iteration to
pass a borrowed string slice to Line::from instead of cloning each id. Preserve
the existing item ordering and rendering behavior while removing the per-frame
allocation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17512304-c516-45ff-9944-e02173c4158e
📒 Files selected for processing (16)
AGENTS.mdsrc/cli/mod.rssrc/connector/adapter/codesearch_config.rssrc/connector/adapter/copilot_auth.rssrc/connector/adapter/management/handlers/llm.rssrc/connector/adapter/management/server.rssrc/connector/adapter/management/streaming.rssrc/connector/adapter/openai_chat_client.rssrc/connector/api/container.rssrc/connector/api/controller/mod.rssrc/connector/api/mod.rssrc/connector/api/openai_command.rssrc/connector/api/openai_command/picker.rssrc/connector/api/router.rssrc/lib.rssrc/main.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- src/connector/api/router.rs
- AGENTS.md
- src/lib.rs
- src/connector/api/container.rs
- src/main.rs
- src/connector/api/mod.rs
- src/connector/adapter/copilot_auth.rs
- src/connector/adapter/openai_chat_client.rs
- src/cli/mod.rs
- src/connector/adapter/management/streaming.rs
- API PUT /api/llm/endpoints/{name} now activates the first endpoint by
default (matching the `openai add` CLI).
- Config load/save in the async management handlers moved off the
executor via CodesearchConfig::{load,save}_async (spawn_blocking).
- list_models error path logs the response-body read error instead of
silently dropping it.
Stale findings skipped: COPILOT_HOME scoping and auth_status().ok()
logging — that CLI/SDK code was removed in the direct-HTTP rewrite;
device-flow request timeouts were already added.
Owner
Author
|
Addressed the review findings in 35ad2e9: Fixed:
Skipped (stale — the flagged code was removed in the direct-HTTP rewrite
464 tests pass, clippy clean (no new warnings), fmt clean. |
This was referenced Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a GitHub Copilot subscription as a third LLM backend, generalizes the OpenAI-compatible backend to multiple named endpoints (LM Studio, vLLM, hosted OpenAI, …) configurable at runtime, and makes the OpenAI backend the default. Embeddings are untouched (still local/ONNX).
Select a backend with
--llm-target open-ai(new default) |anthropic|copilot.Copilot backend: direct HTTP, no external CLI
The Copilot API is OpenAI-compatible, so this talks to
https://api.githubcopilot.comdirectly over HTTP — the same approach opencode uses. NocopilotCLI or SDK dependency.CopilotChatClient— areqwestclient with the OAuthghu_…token asBearer+ Copilot headers. Chat + streaming delegate toOpenAiChatClient::with_parts(shared request/SSE logic). Only model discovery (GET /models) is Copilot-specific.copilot_auth.rs) — codesearch runs the GitHub OAuth device flow itself (RFC 8628; public VS Code Copilot client id), stores the token inconfig.json(0600), then opens a ratatui model picker. Fully self-contained.codesearch copilot login | models | status.OpenAI backend: named endpoints, configurable via CLI and the server
openaisection: anendpointsmap ({base_url, model, api_key}) + anactivename.--llm-target open-airesolves theactiveendpoint, elseOPENAI_*env.codesearch openai add | use | endpoints | models | select(select reuses a ratatui picker;endpointsmasks API keys).GET /api/llm/endpoints(keys masked →has_key),PUT /api/llm/endpoints/{name}(write-onlyapi_key),POST /api/llm/active, andGET /api/llm/models?target=openai&endpoint=<name>.GET /v1/models; the explain SSE stream acceptsmodel+endpointoverrides for on-the-fly switching.Default
--llm-targetflips toopen-aieverywhere. Anthropic remains selectable and is excluded from/api/llm/models(no portable discovery endpoint).Verified
0600) →GET /models(full real catalog) → a real chat completion via query expansion.0600.cargo test— 464 passed, 2 ignored.cargo clippy— no new warnings.cargo fmt --check— clean.Tests use in-memory storage + mock embeddings and make no network calls; the live paths are exercised manually.
Caveat: unofficial Copilot integration
The Copilot path is reverse-engineered, not an official GitHub API — it uses the public VS Code Copilot OAuth client id (
Iv1.b507a08c87ecfe98) and Copilot chat headers againstapi.githubcopilot.com(the same surface opencode/copilot-api rely on). GitHub could change or restrict it. Deliberate trade-off (chosen overgithub/copilot-sdk, which spawns the CLI) for a self-contained login and no external dependency.