Repository navigation
fix(github_copilot): disable shared device login on the proxy - #45697
yassin-berriai wants to merge 2 commits into
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
|
|
| "Each LiteLLM user connects their own GitHub account from LLM Credentials, and their requests use their own GitHub Copilot access. Users who have not connected get a 401.", | ||
| fieldKeys: [GITHUB_COPILOT_AUTH_TYPE_KEY], | ||
| requiredFieldKeys: [GITHUB_COPILOT_AUTH_TYPE_KEY], | ||
| hiddenFieldKeys: ["api_base", "api_key"], |
There was a problem hiding this comment.
Team admins save unusable models
Removing the shared option leaves team admins with no available auth type. The form then shows raw credentials and skips its credential-required check. Models saved without a per-user credential return 401 because shared authentication is disabled. Keep credential selection required even when the user cannot create credentials
Knowledge Base Used: Dashboard and enterprise UI
|
|
||
| from litellm.llms.github_copilot.authenticator import disable_github_copilot_shared_login | ||
|
|
||
| disable_github_copilot_shared_login() |
There was a problem hiding this comment.
Breaking change lacks title marker
Existing shared-token deployments now return 401, but the title lacks the breaking-change marker required by the repository. Change it to fix(github_copilot)!: before merging so release notes flag the upgrade impact
Rule Used: What: When a PR changes behavior that existing users rely on, the PR title must mark it breaking with ! after the type or scope (e.g. fix(mcp)!: ..., feat!: ...), and the PR description must have a short "Breaking change" note saying who is aff... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| from litellm.llms.github_copilot.authenticator import disable_github_copilot_shared_login | ||
|
|
||
| disable_github_copilot_shared_login() |
There was a problem hiding this comment.
Required live proof is incomplete
The supplied proof shows rejected requests but no connected-user success, Responses or embeddings calls, or dashboard setup. The repository requires live proof across affected endpoints and both success and failure paths. Add those cases before merging
Rule Used: Flag if the PR has no proof of e2e testing (if e2e testing is applicable here. If it's like an AGENTS.md change or something then probably no need to have e2e test proof) that either doesn't exist, claims that the box that it was trying to run in doe... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def _get_github_copilot_shared_api_key( | ||
| get_api_key: Callable[[], str], | ||
| model: str, | ||
| custom_llm_provider: str, | ||
| ) -> str: | ||
| from litellm.llms.github_copilot.common_utils import GetAPIKeyError | ||
|
|
||
| try: | ||
| return get_api_key() | ||
| except GetAPIKeyError as exc: |
There was a problem hiding this comment.
Provider logic enters shared code
_get_github_copilot_shared_api_key adds GitHub Copilot’s error mapping to main.py. The repository requires provider-specific logic inside litellm/llms/<provider>/. Move the helper there before merging
Rule Used: What: Keep provider-specific logic inside litellm/llms/<provider>/. Shared code (main.py, utils.py, router.py, proxy/) should not branch on a provider name. Why: provider branches in shared code spread across the codebase and break silently when a p... (source)
| mock_makedirs.assert_called_once() | ||
|
|
||
| def test_disabled_shared_login_ignores_existing_tokens(self, tmp_path): | ||
| token_dir = tmp_path / "tokens" |
There was a problem hiding this comment.
Tests miss required variable annotations
New test locals such as token_dir lack Final annotations. AGENTS.md requires every variable to use Final. Apply that requirement throughout the added Python tests before merging
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will not alter performance
|
TLDR
Problem this solves:
How it solves it:
Intentional product change: gateways that relied on a shared
access-tokenfile for GitHub Copilot now get a 401 on those models until the admin attaches a Per-user GitHub OAuth credential and each user clicks Connect under LLM Credentials. Shared login is too risky on the gateway because a missing token makes workers wait on a human device codeUser Flow
Before: an admin adds a GitHub Copilot model without a per-user credential, and the gateway tries to run GitHub's device login inside its workers
github_copilot/gpt-4.1with no credential at https://litellm-domain/ui/?page=modelsAfter: the same model fails fast with a clear 401, and per-user OAuth is the only gateway setup
github_copilot/gpt-4.1with no credential at https://litellm-domain/ui/?page=modelsLinear ticket
Resolves LIT-9306
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/unit/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Proxy config used on both sides, with
GITHUB_COPILOT_TOKEN_DIRpointed at a directory holding placeholderaccess-tokenandapi-key.jsonfiles:Before (merge base 4cb47ed)
Placeholder token files on disk
curl -i -X POST https://litellm-domain/v1/chat/completions -d '{"model":"ghc-shared-chat","messages":[{"role":"user","content":"ping"}]}'HTTP/1.1 400 Bad RequestwithGithub_copilotException - bad request: Authorization header is badly formatted, because the gateway read the shared token file and sent it to GitHub./v1/messagesonghc-shared-messagesreturns the same 400statshows theapi-key.jsonatime moved from1791589739to1791590357, so the gateway read the shared token directoryEmpty token directory
HTTP/1.1 400 Bad RequestwithThere are no healthy deployments for this modelIn both runs the unconnected per-user model returns the same Connect 401 as After
After (13ae055)
Shared /v1/chat/completions
curl -i -X POST https://litellm-domain/v1/chat/completions -d '{"model":"ghc-shared-chat","messages":[{"role":"user","content":"ping"}]}'HTTP/1.1 401 Unauthorizedwith{"error":{"message":"litellm.AuthenticationError: GitHub Copilot shared device login is disabled on the LiteLLM proxy. Create an LLM credential with Auth Type 'Per-user GitHub OAuth' and reference it from the deployment with litellm_credential_name ...","type":"authentication_error","code":"401"}}Shared /v1/messages
curl -i -X POST https://litellm-domain/v1/messages -d '{"model":"ghc-shared-messages","max_tokens":50,"messages":[{"role":"user","content":"ping"}]}'HTTP/1.1 401 Unauthorizedwith{"type":"error","error":{"type":"authentication_error","message":"litellm.AuthenticationError: GitHub Copilot shared device login is disabled on the LiteLLM proxy. ..."}}Per-user model, user not connected
curl -i -X POST https://litellm-domain/v1/chat/completions -d '{"model":"ghc-per-user","messages":[{"role":"user","content":"ping"}]}'HTTP/1.1 401 UnauthorizedwithConnect GitHub Copilot for credential 'ghc-per-user-credential' in the LiteLLM UI (LLM Credentials), unchanged from mainToken directory
staton the token directory and both files before starting the proxy and after the three requestsType
🐛 Bug Fix
Link to Devin session: https://app.devin.ai/sessions/453c40b054f240f1bf0e4512da9a9aae
Open in Devin Desktop: https://app.devin.ai/desktop/session/453c40b054f240f1bf0e4512da9a9aae?variant=devin
Requested by: @yassin-berriai