fix(copilot): fixes from testing and integration#131
fix(copilot): fixes from testing and integration#131theblazehen wants to merge 11 commits intoMirrowel:devfrom
Conversation
Implements GitHub Copilot integration with: - Device Flow OAuth authentication (no browser redirect needed) - Custom Copilot Chat API integration (bypasses LiteLLM) - Configurable X-Initiator header control (user vs agent mode) - Support for both github.com and GitHub Enterprise - Vision request support - Streaming and non-streaming responses Key features from the referenced implementations: - Based on https://github.com/sst/opencode-copilot-auth for OAuth flow - Agent header forcing feature from Tarquinen/dotfiles plugin - COPILOT_AGENT_PERCENTAGE env var for random user/agent ratio New files: - copilot_auth_base.py: GitHub Device Flow OAuth handler - copilot_provider.py: Custom API integration Closes Mirrowel#29
|
I'm starting my review of the Copilot provider fixes. I'll be looking closely at the new credential setup flow and the streaming response handling. Back soon with the full report! |
theblazehen
changed the title
![mirrobot-agent[bot]](https://avatars.githubusercontent.com/in/2140342?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
This PR provides a solid set of improvements for the Copilot provider, particularly around production stability and credential management. The fixes for streaming response handling (awaiting the handler and handling ResponseNotRead errors) are critical and well-implemented. The expansion of the model catalog and the improved model prefixing logic significantly enhance the provider's usability.
Architectural Feedback
- Credential Storage: The introduction of
setup_credentialand associated management methods is a great step toward standardized credential handling. Using the email for deduplication is a robust approach. - Streaming Compatibility: The switch to plain dictionaries for streaming chunks is an interesting choice to avoid serialization issues. While practical, it's important to ensure this doesn't break expectations for downstream consumers that might rely on LiteLLM's object structure.
Key Suggestions
- Locking Mechanism: The in-memory lock key using
id(creds)might be slightly fragile if objects are cloned. - Persistence Path: Consider using a more global configuration directory for OAuth credentials to avoid "orphaned" credential files when running from different directories.
Verified Fixes
- Streaming Await: Correctly added
awaitto_handle_streaming_responsecall. - Error Handling: Implemented
aread()for error responses in streaming mode, preventingResponseNotReadexceptions. - Email Fallback: Added reliable fallback from
emailtologinusername for GitHub identity.
Questions for the Author
- Was the change from
litellm.Choicesto plain dictionaries tested with all intended downstream consumers? - Is the
oauth_credsdirectory intended to be local to the project root, or would a global config directory be more appropriate for the CLI use case?
This review was generated by an AI assistant.
|
|
||
| def _get_oauth_base_dir(self) -> Path: | ||
| """Return the OAuth credentials base directory.""" | ||
| return Path.cwd() / "oauth_creds" |
There was a problem hiding this comment.
Using Path.cwd() / "oauth_creds" might cause credentials to be scattered if the application is executed from different directories. Consider using a standardized user configuration directory (e.g., Path.home() / ".config/llm-proxy/oauth_creds") to ensure persistence across sessions.
There was a problem hiding this comment.
Good catch — fixed in 229257e. Now uses the centralized get_oauth_dir() from utils/paths.py which handles PyInstaller mode and overrides.
| The Copilot API token (access_token) expires after ~30 minutes. | ||
| """ | ||
| async with await self._get_lock(path): | ||
| lock_key = path or f"in-memory://copilot/{id(creds)}" |
There was a problem hiding this comment.
Using id(creds) for an in-memory lock key is clever, but be aware that it might lead to redundant locks if the creds dictionary is copied or recreated (e.g., during serialization/deserialization cycles). If the credentials have a unique identifier like an email, that might be a more stable key.
There was a problem hiding this comment.
Fair point — id() can be recycled if the dict gets garbage collected and recreated. In practice the in-memory path only triggers for env-based credentials which are long-lived singletons, but using the email from _proxy_metadata would be more robust. Open to changing if preferred.
| } | ||
| ) | ||
|
|
||
| return litellm.ModelResponse( |
There was a problem hiding this comment.
While using plain dictionaries for choices avoids serialization issues, it might break compatibility with LiteLLM utilities or downstream consumers that expect litellm.Choices objects. Have you verified that this change doesn’t affect any of the proxy’s core functionality or existing integrations?
There was a problem hiding this comment.
Yes — the litellm.Choices/litellm.Delta objects were causing serialization issues during streaming. ModelResponse() accepts plain dicts for choices and handles the wrapping internally, so downstream consumers still get proper objects. Tested with both streaming and non-streaming through the full proxy pipeline.
| 4. Parses response into LiteLLM format | ||
| """ | ||
| credential_path = kwargs.get("api_key", "") | ||
| credential_path = kwargs.pop("credential_identifier", kwargs.get("api_key", "")) |
There was a problem hiding this comment.
Good addition of credential_identifier. However, kwargs.get("api_key", "") is used as a fallback. In this provider, api_key is repurposed to store the credential path. It might be worth adding a comment to clarify this for future maintainers.
There was a problem hiding this comment.
Makes sense. For context: all OAuth-based providers in this project pass credential file paths (or env:// virtual paths) through the api_key/credential_identifier field since the rotator assigns credentials per-request. The credential_identifier kwarg was added in a later refactor as the canonical way to pass this, with api_key kept as fallback for backward compat.
…th.cwd() Avoids credentials scattering if run from different directories and respects PyInstaller/override modes via utils.paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Mirrowel
added
enhancement
|
@greptile |
|
| Filename | Overview |
|---|---|
| src/rotator_library/providers/copilot_auth_base.py | Adds credential lifecycle management (setup, list, delete, export). The exported .env file lacks restrictive permissions (unlike _save_credentials which uses 0o600), and _get_next_credential_number has a TOCTOU race between number allocation and file write. Email null-to-login fallback fix is correct. |
| src/rotator_library/providers/copilot_provider.py | Fixes await on streaming handler, credential_identifier kwarg extraction, stream delta serialization (plain dicts instead of litellm objects), ResponseNotRead guard, and provider-prefixed model names. Changes are correct and well-targeted. |
Sequence Diagram
sequenceDiagram
participant Client
participant CopilotProvider
participant CopilotAuthBase
participant GitHubAPI
participant CopilotAPI
Note over Client,CopilotAPI: setup_credential() flow
Client->>CopilotAuthBase: setup_credential()
CopilotAuthBase->>CopilotAuthBase: initialize_token(temp_creds)
CopilotAuthBase->>GitHubAPI: POST /login/device/code
GitHubAPI-->>CopilotAuthBase: device_code, user_code
Note over CopilotAuthBase: Display code to user
loop Poll until authorized
CopilotAuthBase->>GitHubAPI: POST /login/oauth/access_token
GitHubAPI-->>CopilotAuthBase: github_token (on success)
end
CopilotAuthBase->>GitHubAPI: GET /user (fetch email/login)
GitHubAPI-->>CopilotAuthBase: email or login
CopilotAuthBase->>CopilotAPI: GET /copilot_internal/v2/token
CopilotAPI-->>CopilotAuthBase: copilot_access_token
CopilotAuthBase->>CopilotAuthBase: _find_existing_credential_by_email()
CopilotAuthBase->>CopilotAuthBase: _save_credentials(file_path, creds)
CopilotAuthBase-->>Client: CopilotCredentialSetupResult
Note over Client,CopilotAPI: acompletion() streaming flow
Client->>CopilotProvider: acompletion(credential_identifier=..., stream=true)
CopilotProvider->>CopilotAuthBase: _load_credentials(credential_path)
CopilotAuthBase-->>CopilotProvider: creds
alt token expired
CopilotProvider->>CopilotAuthBase: _refresh_copilot_token(path, creds)
CopilotAuthBase->>CopilotAPI: GET /copilot_internal/v2/token
CopilotAPI-->>CopilotAuthBase: new copilot_access_token
CopilotAuthBase-->>CopilotProvider: refreshed creds
end
CopilotProvider->>CopilotProvider: await _handle_streaming_response()
CopilotProvider->>CopilotAPI: POST /chat/completions (stream=true)
loop SSE chunks
CopilotAPI-->>CopilotProvider: data: {...}
CopilotProvider-->>Client: litellm.ModelResponse (plain dict delta)
end
Last reviewed commit: 6566ab0
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import logging | ||
| import re | ||
| from pathlib import Path | ||
| from ..utils.paths import get_oauth_dir |
There was a problem hiding this comment.
Import will fail - paths.py doesn't exist in base branch
The file src/rotator_library/utils/paths.py does not exist in the base branch feature/copilot-provider. This import will fail with ModuleNotFoundError at runtime.
The paths.py module exists in main branch (added in commit 467f294), but the target branch doesn't have it yet. Either:
- Merge/rebase with
mainto getpaths.pyfirst - Revert to using
Path.cwd() / "oauth_creds"directly (like other files in this branch do)
| from ..utils.paths import get_oauth_dir | |
| # from ..utils.paths import get_oauth_dir # TODO: add after merging with main |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/rotator_library/providers/copilot_auth_base.py
Line: 21
Comment:
Import will fail - `paths.py` doesn't exist in base branch
The file `src/rotator_library/utils/paths.py` does not exist in the base branch `feature/copilot-provider`. This import will fail with `ModuleNotFoundError` at runtime.
The `paths.py` module exists in `main` branch (added in commit 467f294), but the target branch doesn't have it yet. Either:
1. Merge/rebase with `main` to get `paths.py` first
2. Revert to using `Path.cwd() / "oauth_creds"` directly (like other files in this branch do)
```suggestion
# from ..utils.paths import get_oauth_dir # TODO: add after merging with main
```
How can I resolve this? If you propose a fix, please make it concise.get_user_info() always returns at least {"email": "unknown"}, so
the previous 'if not email' check was unreachable dead code. Now also
checks for the 'unknown' sentinel to prevent creating credentials with
placeholder emails that could cause deduplication collisions.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
| with open(env_path, "w") as f: | ||
| f.write("\n".join(self.build_env_lines(creds, cred_number))) |
There was a problem hiding this comment.
Exported .env file written without restrictive permissions
export_credential_to_env writes the GitHub OAuth token (a long-lived secret) to disk without restricting file permissions. _save_credentials (line 243) correctly sets 0o600 after an atomic write, but the .env exporter skips this step, leaving the file world-readable by default (subject to the process umask).
| with open(env_path, "w") as f: | |
| f.write("\n".join(self.build_env_lines(creds, cred_number))) | |
| with open(env_path, "w") as f: | |
| f.write("\n".join(self.build_env_lines(creds, cred_number))) | |
| try: | |
| os.chmod(env_path, 0o600) | |
| except (OSError, AttributeError): | |
| pass |
| def _get_next_credential_number(self, base_dir: Optional[Path] = None) -> int: | ||
| """Get next available credential number.""" | ||
| if base_dir is None: | ||
| base_dir = self._get_oauth_base_dir() | ||
|
|
||
| prefix = self._get_provider_file_prefix() | ||
| pattern = str(base_dir / f"{prefix}_oauth_*.json") | ||
|
|
||
| existing_numbers = [] | ||
| for cred_file in glob(pattern): | ||
| match = re.search(r"_oauth_(\d+)\.json$", cred_file) | ||
| if match: | ||
| existing_numbers.append(int(match.group(1))) | ||
|
|
||
| if not existing_numbers: | ||
| return 1 | ||
| return max(existing_numbers) + 1 |
There was a problem hiding this comment.
TOCTOU race condition in credential number allocation
_get_next_credential_number reads the directory and returns max + 1, but there is no lock guarding the read→allocate→write sequence. Two concurrent setup_credential() calls (e.g., from a parallel CLI invocation or tests) can both observe the same max, both compute the same next number, and then both try to write to the same file path. The second write will silently overwrite the first credential.
Consider holding a file-system level lock (e.g., fcntl.flock on a .lock file inside base_dir) around the allocation + save sequence in setup_credential(), or use an atomic O_EXCL open to detect collision and retry with the next number.
| def build_env_lines(self, creds: Dict[str, Any], cred_number: int) -> list[str]: | ||
| """Generate .env file lines for a Copilot credential.""" | ||
| email = creds.get("_proxy_metadata", {}).get("email", "unknown") | ||
| prefix = f"COPILOT_{cred_number}" | ||
|
|
||
| lines = [ | ||
| f"# COPILOT Credential #{cred_number} for: {email}", | ||
| f"# Exported from: copilot_oauth_{cred_number}.json", | ||
| f"# Generated at: {time.strftime('%Y-%m-%d %H:%M:%S')}", | ||
| "", | ||
| f"{prefix}_GITHUB_TOKEN={creds.get('refresh_token', '')}", | ||
| f"{prefix}_ENTERPRISE_URL={creds.get('enterprise_url', '')}", | ||
| f"{prefix}_EMAIL={email}", | ||
| ] | ||
|
|
||
| return lines |
There was a problem hiding this comment.
.env output is missing a trailing newline
build_env_lines returns a list whose elements are joined with "\n" in export_credential_to_env, but there is no final "\n" after the last line. POSIX defines a text file as a sequence of lines each terminated by a newline, and many tools (diff, wc -l, source, etc.) behave unexpectedly on files that lack a trailing newline.
Add an empty string at the end of lines so that "\n".join(lines) produces the required trailing newline:
lines = [
...,
f"{prefix}_EMAIL={email}",
"", # ← produces trailing newline when joined
]
|
Addressed in 6566ab0 + retargeted PR to Re: Re: unreachable |
2 participants
Summary
Fixes found while testing and integrating the Copilot provider:
setup_credential()method with email-based deduplication and env export supportnullemail — fall back tologinusernameget_models()now returnscopilot/gpt-5instead of baregpt-5so they're directly usable in/v1/modelsDEFAULT_COPILOT_MODELSfrom 8 → 20 models (GPT-5.x, Claude 4.5/4.6, Gemini 3, Grok)acompletion()now readscredential_identifierkwarg (used by the rotator) instead of onlyapi_key_handle_streaming_responsewas not awaited in the completion pathlitellm.Choices/litellm.Deltaobjects for streaming chunks (avoids serialization issues)aread()response body before logging HTTP errors in streaming modeTest plan
🤖 Generated with Claude Code