Skip to content

fix(providers): route Copilot enterprise tokens via proxy endpoint#355

Open
penso wants to merge 1 commit intomainfrom
copilot-enterprise
Open

fix(providers): route Copilot enterprise tokens via proxy endpoint#355
penso wants to merge 1 commit intomainfrom
copilot-enterprise

Conversation

@penso
Copy link
Copy Markdown
Collaborator

@penso penso commented Mar 8, 2026

Summary

  • Parse proxy-ep from Copilot token exchange response and persist it alongside the cached API token
  • Route all API calls (models, chat completions) through the enterprise proxy endpoint when proxy-ep is present
  • Force stream: true for complete() on enterprise proxy (non-streaming returns 400), transparently collecting the SSE stream into a CompletionResponse
  • Individual (non-enterprise) tokens continue using api.individual.githubcopilot.com unchanged

Closes #352

Validation

Completed

  • cargo +nightly-2025-11-30 fmt --all -- --check
  • cargo check -p moltis-providers -p moltis-gateway
  • cargo test -p moltis-providers --features provider-github-copilot -- copilot (23/23 pass)
  • No warnings

Remaining

  • ./scripts/local-validate.sh <PR_NUMBER>
  • Manual QA with an enterprise Copilot account

Manual QA

  1. Authenticate Moltis with a Copilot Enterprise account
  2. Verify model listing works (GET {proxy-ep}/models returns 200)
  3. Send a chat message — verify streaming works
  4. Verify complete() path works (tool-calling flows use this)
  5. Authenticate with an individual Copilot account — verify existing behavior unchanged

🤖 Generated with Claude Code

)

Enterprise Copilot tokens receive a `proxy-ep` hostname in the token
exchange response. Requests sent to the individual endpoint return
HTTP 421. This change parses `proxy-ep`, persists it alongside the
cached API token, and routes all API calls through the enterprise proxy
when present. The proxy also requires `stream: true` for chat
completions, so `complete()` transparently streams and collects when
using an enterprise endpoint.

Closes #352
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 8, 2026

Greptile Summary

This PR adds enterprise GitHub Copilot account support by parsing the proxy-ep field from the Copilot token exchange response, persisting it in the account_id field of the cached token, and routing all API requests (models + chat completions) through the enterprise proxy URL. Because the enterprise proxy rejects non-streaming completions, a new collect_streamed_completion bridge transparently issues a streaming request and assembles the SSE events into a CompletionResponse. Individual (non-enterprise) accounts continue using the original api.individual.githubcopilot.com endpoint unchanged.

Key changes:

  • New CopilotTokenResponse.proxy_ep field (deserialized from "proxy-ep") and CopilotAuth struct that bundles token, base URL, and is_enterprise flag
  • fetch_valid_copilot_tokenfetch_copilot_auth: now returns a CopilotAuth instead of a raw token string
  • collect_streamed_completion: enterprise-only streaming-to-sync bridge; collects SSE events via stream_events_to_completion
  • complete() short-circuits to the streaming bridge when auth.is_enterprise is set
  • account_id field of the cached OAuthTokens is repurposed to store the proxy hostname
  • 23 new tests covering deserialization, auth resolution, streaming collection, and tool-call assembly

Issues found:

  • Logic bug: In collect_streamed_completion, the trailing-buffer handling (after the main byte-stream loop ends) silently discards any SseLineResult::Events returned from the final partial line. This is inconsistent with the identical trailing-buffer logic in stream_with_tools, which correctly accumulates those events. In practice the bug only triggers when the stream closes without a [DONE] terminator, but it can silently drop content in that degraded case.
  • Style: account_id is repurposed to hold the proxy-ep hostname without any comment explaining the dual use, which could confuse future readers.

Confidence Score: 3/5

  • Safe to merge for individual accounts; enterprise path has a narrow trailing-buffer bug that can silently drop content when the SSE stream closes without [DONE].
  • The core routing logic and token caching are correct and well-tested (23/23). The one confirmed logic bug (trailing SSE events dropped in the enterprise completion path) is a narrow edge case — it only fires when the enterprise proxy closes the HTTP connection without emitting a [DONE] frame, which is outside normal operation. The account_id field reuse is a maintainability concern rather than a correctness issue. No regressions to the individual account path.
  • Pay close attention to crates/providers/src/github_copilot.rs — specifically the trailing-buffer match arm in collect_streamed_completion (line 586–588).

Important Files Changed

Filename Overview
crates/providers/src/github_copilot.rs Enterprise proxy routing added via CopilotAuth struct and collect_streamed_completion bridge. One logic bug: trailing-buffer SSE events are silently dropped in the enterprise completion path. Minor: account_id field is repurposed to store the proxy hostname without documentation.

Sequence Diagram

sequenceDiagram
    participant C as Caller
    participant P as GitHubCopilotProvider
    participant GH as GitHub API (token exchange)
    participant IND as api.individual.githubcopilot.com
    participant ENT as proxy.enterprise.githubcopilot.com

    C->>P: complete(messages, tools)
    P->>P: get_copilot_auth()
    P->>GH: GET /copilot_internal/v2/token
    GH-->>P: { token, expires_at, proxy-ep? }
    P->>P: copilot_auth_from_parts(token, proxy_ep?)
    P->>P: persist token + proxy_ep in account_id cache

    alt is_enterprise == true
        P->>ENT: POST /chat/completions (stream: true)
        ENT-->>P: SSE stream
        P->>P: collect_streamed_completion()
        P->>P: stream_events_to_completion(events)
        P-->>C: CompletionResponse
    else is_enterprise == false
        P->>IND: POST /chat/completions
        IND-->>P: JSON response
        P-->>C: CompletionResponse
    end
Loading

Last reviewed commit: 3bb64be

Comment on lines +586 to +588
match process_openai_sse_line(data, &mut state) {
SseLineResult::Done | SseLineResult::Events(_) | SseLineResult::Skip => {},
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Trailing-buffer events silently dropped

When the byte stream ends without a [DONE] line (or when [DONE] arrives in the final chunk without a trailing newline), process_openai_sse_line can return SseLineResult::Events(evts) for the remaining buffer content. The current match arm ignores those events instead of accumulating them, so any text or tool-call deltas in the tail are silently discarded.

Compare the parallel logic in stream_with_tools (lines 867–881), which correctly yields the events:

match process_openai_sse_line(data, &mut state) {
    SseLineResult::Done | SseLineResult::Events(_) | SseLineResult::Skip => {},
}

Should be:

Suggested change
match process_openai_sse_line(data, &mut state) {
SseLineResult::Done | SseLineResult::Events(_) | SseLineResult::Skip => {},
}
match process_openai_sse_line(data, &mut state) {
SseLineResult::Done => {
events.extend(finalize_stream(&mut state));
return Ok(stream_events_to_completion(events));
},
SseLineResult::Events(evts) => events.extend(evts),
SseLineResult::Skip => {},
}

Comment on lines 280 to 281
refresh_token: None,
id_token: None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

account_id field repurposed for proxy hostname

account_id is being used here to persist the enterprise proxy_ep hostname — a field whose name implies it stores an account identifier. While this works as a persistence mechanism, it creates an implicit convention that future readers may not expect. Consider at least a short inline comment noting this dual purpose, or (if the OAuthTokens struct can be modified) adding a dedicated field.

Suggested change
refresh_token: None,
id_token: None,
// NOTE: account_id is repurposed here to persist the enterprise
// proxy-ep hostname so it can be recovered from the token cache.
account_id: copilot_resp.proxy_ep.clone(),

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!

@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq bot commented Mar 8, 2026

Merging this PR will not alter performance

✅ 39 untouched benchmarks
⏩ 5 skipped benchmarks1


Comparing copilot-enterprise (3bb64be) with main (e492d90)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 8, 2026

Codecov Report

❌ Patch coverage is 83.55263% with 50 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/providers/src/github_copilot.rs 83.55% 50 Missing ⚠️

📢 Thoughts on this report? Let us know!

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.

[Model]: Copilot enterprise tokens return 421 unless using proxy endpoint + streaming

1 participant