Skip to content

Added Copilot login through GHE#827

Merged
steipete merged 1 commit intosteipete:mainfrom
ramzesenok:main
May 5, 2026
Merged

Added Copilot login through GHE#827
steipete merged 1 commit intosteipete:mainfrom
ramzesenok:main

Conversation

@ramzesenok
Copy link
Copy Markdown
Contributor

Adding a textfield in the Settings > Providers > GitHub Copilot to enter your Enterprise host and get logged in through it instead of the default github.com

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5804ca2487

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

}

public var deviceCodeURL: URL {
URL(string: "https://\(self.host)/login/device/code")!
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Guard against invalid enterprise host before force unwrap

The new host field is user-editable, but deviceCodeURL force-unwraps URL(string:); inputs that are not valid hostnames (for example foo bar) survive normalizedHost and make URL(string:) return nil, which crashes the login flow as soon as requestDeviceCode() reads this property. This should fail gracefully (validation or throwing) instead of terminating the app.

Useful? React with 👍 / 👎.

Comment on lines +63 to +64
if let parsedHost = URLComponents(string: host)?.host, !parsedHost.isEmpty {
host = parsedHost
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve custom port when normalizing enterprise hosts

When the host is provided as a full URL with a non-default port (for example https://ghe.example.com:8443), URLComponents(...).host strips the port and only keeps ghe.example.com, so subsequent device-flow and API requests are sent to port 443 instead of the configured endpoint. This breaks enterprise instances behind custom HTTPS ports.

Useful? React with 👍 / 👎.

@steipete
Copy link
Copy Markdown
Owner

steipete commented May 5, 2026

Deep review pass from current main:

Thanks for the GHE Copilot work. I don't think this is mergeable as-is yet.

What I checked:

  • PR head targeted tests pass: swift test --filter CopilotDeviceFlowTests --no-parallel.
  • The patch does not apply to current main; merge-tree shows conflicts in CopilotLoginFlow, CopilotProviderImplementation, CopilotSettingsStore, CopilotProviderDescriptor, CopilotUsageFetcher, ProviderSettingsSnapshot, and settings tests.
  • Current main now has Copilot multi-account/token-account routing. This PR is still shaped around the older single copilotAPIToken settings path, so a straight rebase would need a real integration with token accounts and account identity matching.
  • GitHub's current Copilot CLI docs still show the OAuth verification step at https://github.com/login/device even when logging into GitHub Enterprise Cloud with data residency, so we should be careful not to make the OAuth device flow host-derived without proving the exact enterprise mode. The API host derivation also needs to distinguish GHEC data-residency hosts from GHES /api/v3 style hosts.

Best next shape: rebase onto current main, keep enterprise host/account metadata per Copilot token account rather than one global provider setting, and add tests for both endpoint families before we merge.

@steipete
Copy link
Copy Markdown
Owner

steipete commented May 5, 2026

Extra live check from current main:

Using my token from .profile as COPILOT_API_TOKEN, current main successfully fetches Copilot usage via the public GitHub API path:

  • provider: copilot
  • source: api
  • plan/login method: Individual_Pro
  • primary/secondary usage windows returned

I do not have a GHE host/token in .profile, so I cannot live-verify the enterprise endpoint family here. That is why I think the PR needs endpoint-family tests and per-account host metadata before merge, rather than just a global host string.

@steipete steipete merged commit 36246e3 into steipete:main May 5, 2026
1 check passed
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7ab3f24806

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +95 to +97
let fetcher = CopilotUsageFetcher(
token: token,
enterpriseHost: enterpriseHost.isEmpty ? nil : enterpriseHost)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Resolve identity against enterprise host during Copilot login

This enterprise-host login path only applies enterpriseHost to the usage fetch, but account identity lookup in the same flow still calls CopilotUsageFetcher.fetchGitHubIdentity(token:), which is hardcoded to https://api.github.com/user. For enterprise tokens tied to api.<enterpriseHost>, that lookup returns 401/403, so re-auth fails whenever any Copilot account already exists (the code shows “Could Not Identify GitHub Account” and exits), effectively blocking account refresh/login on GHE hosts.

Useful? React with 👍 / 👎.

@ramzesenok
Copy link
Copy Markdown
Contributor Author

oh wow, that was fast, thanks Peter!

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