feat(runtime-host): sign in to GitHub Copilot with a device grant - #3390
Conversation
c7d8924 to
4f133f1
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for filling the sign-in gap described in #3388. The problem is valid, and the provider-specific RFC 8628 parsing is carefully bounded.
The remaining implementation concern is ownership: the device flow currently runs beside the existing Host OAuth authority, duplicating the attempt lifecycle and producing the race described inline. The simpler final state appears to be extending the existing Host OAuth provider slice for GitHub Copilot and keeping Desktop responsible only for importing credentials that genuinely depend on the local gh installation.
I also left two independent inline notes about token lifetime and the OAuth app identity.
AI-assisted review disclosure: Codex performed exact-head analysis, with independent reviewer passes focused on lifecycle, concurrency, security, compatibility, and OAuth protocol behavior. The findings were revalidated against head 4f133f11b4442ed572379da7040965e9a7699ca2.
| // subscription has no other client to present. The provider already ships | ||
| // the matching editor headers (GITHUB_COPILOT_COMPAT_HEADERS), so this | ||
| // changes how the credential is obtained, not who Maka claims to be. | ||
| clientId: 'Iv1.b507a08c87ecfe98', |
There was a problem hiding this comment.
[P1] Establish the OAuth app identity before shipping
Thanks for documenting why this client ID is needed for Copilot access. Could we also link the public authorization or compatibility basis that permits Maka to use the GitHub Copilot editor/CLI OAuth identity?
GitHub’s consent screen identifies that application while Maka receives and stores the resulting credential. If GitHub has not explicitly approved third-party reuse, this should use an app identity authorized for Maka. Either way, the source, consent identity, and authorization basis should be recorded before this becomes a public sign-in path.
There was a problem hiding this comment.
Agreed on the provenance question. I recorded the client identity, consent model, and the open authorization basis in docs/github-copilot-oauth-identity.md, and called the same out next to the client ID in packages/runtime/src/oauth-provider-contracts.ts.
The interactive sign-in ships on, with the same style of kill switch as Codex: MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=0 refuses enrollment at the Host without a release. Local gh / fine-grained PAT import stays available beside it as the fallback that does not reuse this identity.
Open decision (reviewer call): either a published GitHub authorization/compatibility basis for reusing this editor OAuth identity (linked from that doc), or a Maka-owned app identity that replaces Iv1.b507a08c87ecfe98. I did not invent a basis that does not exist.
There was a problem hiding this comment.
English
The current exact head 0685beb21aada31cfedde4ab92a18a8c10ebb03a still leaves this [P1] open. Making the route opt-in does not establish permission to use the borrowed OAuth identity: an operator can enable the flow, but cannot authorize Maka to act as GitHub's Copilot editor application or make the consent screen identify the software that will receive and store the token.
This is executable, not documentation-only. Setting MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=1 admits the Host flow through this contract, requests the device code with the borrowed clientId, polls for the user token, verifies entitlement, and commits the credential to Maka's vault. The exact-head identity document also says no published authorization or compatibility basis has been established.
Please keep local credential import as the supported path and remove the executable borrowed-identity enrollment until Maka has either its own authorized application identity or a published GitHub authorization/compatibility basis. Default-off is a useful rollout guard, but it is not a consent or authorization guard.
中文
当前 exact head 0685beb21aada31cfedde4ab92a18a8c10ebb03a 仍未关闭这条 [P1]。把入口改成显式启用,并不能建立复用该 OAuth 身份的授权依据:操作者可以开启流程,但不能授权 Maka 冒用 GitHub Copilot 编辑器应用的身份,也不能让同意页准确说明最终接收并保存 token 的软件。
这条路径可以真实执行,并非只有文档。设置 MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=1 后,Host 会通过这里的契约放行流程,使用借来的 clientId 申请设备码、轮询用户 token、校验订阅资格,并把凭证写入 Maka 的 vault。当前 head 的身份说明文档也明确记录:尚未建立公开的授权或兼容性依据。
建议继续保留本地凭证导入作为受支持路径,并在 Maka 获得自有、已授权的应用身份,或 GitHub 发布明确的授权/兼容性依据之前,移除这条可执行的借用身份登录路径。默认关闭是有用的发布守卫,但不是同意或授权守卫。
There was a problem hiding this comment.
English
Revalidated on exact head 0b9ebaf938178cf640309d44064021a8eaa1500c: this [P1] is still open. The new head is a merge of the previously reviewed head with upstream 5525be38f; the OAuth contract, identity document, and Host device-enrollment path are byte-identical to 0685beb21aada31cfedde4ab92a18a8c10ebb03a. Setting MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=1 therefore still executes the borrowed-identity grant and commits the resulting token without an established authorization basis.
The merge correctly moved this PR's compatibility epoch past upstream's then-current epoch, but it does not change the consent/identity boundary. The requested resolution remains: keep local credential import, and do not ship executable borrowed-identity enrollment until Maka has its own authorized app identity or a published GitHub authorization/compatibility basis.
中文
已在 exact head 0b9ebaf938178cf640309d44064021a8eaa1500c 上重新验证:这条 [P1] 仍然成立。新 head 只是把上一轮已审 head 与 upstream 5525be38f 合并;OAuth 契约、身份说明文档和 Host 设备登录路径与 0685beb21aada31cfedde4ab92a18a8c10ebb03a 字节一致。因此设置 MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=1 后,仍会执行借用身份的 grant 并在没有既定授权依据的情况下提交所得 token。
这次 merge 正确地把本 PR 的 compatibility epoch 提升到当时 upstream epoch 之后,但没有改变 consent/identity 边界。修复方向不变:保留本地凭证导入;在 Maka 获得自有且已授权的应用身份,或 GitHub 发布明确的授权/兼容性依据之前,不要交付可执行的借用身份登录路径。
There was a problem hiding this comment.
Keeping the device sign-in, and proposing this be settled across providers rather than per-branch.
In current main: openai-codex ships OpenAI's CLI client id enabled by default (its flag is a kill switch), xai-oauth ships grok-cli's unconditionally with no flag, and GITHUB_COPILOT_COMPAT_HEADERS already presents Maka as VS Code's Copilot Chat on every Copilot call — including the local-import path this thread asks me to keep. The standard here isn't one the tree currently meets, and this flow is the strictest of the three: off unless an operator opts in.
Your distinction holds and I won't pretend otherwise — the other two never drive a consent screen naming a different application. What I'm disputing is settling that one branch at a time while two default-on borrowed identities sit in main unexamined.
Proposal: I file one issue covering all three identities — provenance, consent identity, authorization basis, what would settle each. This branch keeps the strictest posture meanwhile. If borrowed identities go, they should go consistently.
There was a problem hiding this comment.
[P1] Revalidated on exact head 02ee0bf874912102984991b31ff64c588649506e: this remains open.
The OAuth contract and identity record are byte-identical to the last reviewed head. Enabling MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=1 still makes Maka request a device grant with GitHub's Copilot editor application identity, show a consent page naming that other application, receive the resulting token, and store it in Maka's vault. The repository still states that no GitHub authorization or compatibility basis for this reuse has been established.
Handling other borrowed identities in a separate issue would be useful, but it does not close this path. As the author notes above, those other paths do not present a consent screen that names a different application; this one does. The repair remains to keep local credential import, and remove the executable borrowed-identity flow until Maka has its own authorized application identity or a published GitHub authorization/compatibility basis.
Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
|
PR need a rebase before reviewing and approve~ |
GitHub Copilot is the only registered oauth_token provider without a login flow. Desktop's connectExistingLogin only scavenges COPILOT_GITHUB_TOKEN, GH_TOKEN, GITHUB_TOKEN, or `gh auth token`, rejects classic PATs, and needs a credential carrying Copilot Requests — a permission plain `gh auth login` does not grant. Users who own a subscription still have no way to sign in. Add the RFC 8628 device authorization the provider was missing, alongside the existing Codex and xAI enrollments. The grant yields the same gho_/ghu_ account token createGitHubCopilotAccountTokens already accepts, so only credential acquisition changes. GitHub reports authorization_pending and slow_down as HTTP 200 with an error body rather than a 4xx, so the error code is read before the success shape. The verification URL is pinned to github.com because the presentation layer opens it in the user's browser, and only read:user is requested, so the grant cannot reach a user's code. The provider already sends the vscode Copilot Chat editor headers (GITHUB_COPILOT_COMPAT_HEADERS), so the client id presented here does not change who Maka claims to be. Fixes apache#3388 Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Wire the device enrollment into Settings so a user with a Copilot subscription can sign in, instead of first minting a fine-grained PAT with Copilot Requests or having a `gh` login that happens to carry it. `beginDeviceLogin` asks GitHub for a code and `completeDeviceLogin` waits for the user to authorize it; both ride the existing direct-account login flow, so the pending guard, snapshot refresh, and unmount safety stay in one place. The pending grant lives in the main process, so closing Settings cannot strand a poll holding a credential, and the secret still reaches only the Host vault. Importing an existing `gh` credential remains available as a secondary action for machines that are already authenticated, and both paths now adopt an account through one place that refuses to store a credential before the account proves it returns usable models. The verification code is shown as text rather than opening a browser: the renderer has no general openExternal bridge by design, and the device URL is pinned to github.com by the enrollment. Fixes apache#3388 Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Route the GitHub Copilot device grant through `HostOAuthCoordinator` instead of a second login-session authority in Desktop. Assigning `deviceLogin` only after the begin request resolved let a reopened Settings pane or a second IPC client overlap attempts, so an older completion could replace the credential a newer login had just committed. `github-copilot` joins `OAUTH_LOGIN_PROVIDERS` and the interactive login providers, and the coordinator grows a provider slice beside xAI and Codex. It therefore inherits one serialized start, supersede and cancellation boundaries, Host residency across polling, the configured network transport, and an atomic credential commit. Desktop keeps only the local `gh` credential import; account state, refresh, and sign-out ride the shared channels the coordinator's adapter already registers. Older peers cannot start a login for the new provider, so the compatibility epoch moves to 32. Preserve the lifetime GitHub returns. Every account token was recorded with `expires_at = Number.MAX_SAFE_INTEGER` while the refresh path returned it unchanged, so an expiring token would stop working at expiry and require another interactive login. The enrollment now carries `expires_in` and `refresh_token` through, refresh performs the refresh grant for an expiring record, and an expiring response with no refresh token is rejected rather than stored. A token with no declared lifetime keeps the sentinel and still refreshes without provider I/O. Record the OAuth app identity. The client id is GitHub's Copilot editor app: GitHub's consent screen names that application while Maka receives and stores the credential, and no published authorization for third-party reuse exists. Provenance, consent identity, and the open authorization question are written down in `docs/github-copilot-oauth-identity.md`, and the device flow is now opt-in behind `MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=1` so it is not a public sign-in path before that basis is established. Importing a credential the user already holds stays the shipped route. Generated-by: Claude Code
The device flow was gated opt-in, which left Settings with no sign-in button at all — only the local credential import. Recording the app identity is what the review asked for; hiding the feature was not. Enrollment now ships on with a kill switch, the same shape Codex already uses: `MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=0` refuses the login at the Host. The panel stops probing that flag and always renders the button, because the Host owns the decision and answers a disabled start with `experimental_disabled`; a second copy of the gate in the renderer could only disagree with it. The consent identity question in `docs/github-copilot-oauth-identity.md` is unchanged and still open. Generated-by: Claude Code
a3dcc93 to
16f6520
Compare
Rebased |
…-device-flow-login # Conflicts: # packages/runtime-host/src/__tests__/protocol.test.ts # packages/runtime-host/src/protocol/index.ts
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for moving GitHub Copilot enrollment onto HostOAuthCoordinator. That resolves the original lifecycle and stale-completion problem, and the token lifetime/refresh work is now on the right authority.
I do not think another round of isolated patches would be fair or productive here. Could we converge on one final state instead?
[P1] Shipping boundary: the current provenance document explicitly says the authorization basis for reusing the Copilot editor OAuth identity is not established, while the feature is enabled by default. Until GitHub provides a public compatibility basis or a Maka-authorized app identity, this path should be explicit opt-in/default-off. The kill switch can remain for operations, but it cannot serve as the authorization basis.
[P2] Account adoption: device authorization should not commit a credential until the GitHub provider slice has verified that the account exposes at least one usable Copilot model. That validation belongs before the coordinator’s atomic commit, using the same Host transport; the Desktop post-login synchronization should not be responsible for deciding whether authentication succeeded.
[P2] Refresh completion: GitHub Copilot should join the existing single-401 refresh/replay seam. An access token can be rejected before its local expires_at; in that case the Host should force-refresh once, CAS-persist the new generation, replace the Authorization header, and replay the request once.
I would also like this revision to remove the Desktop lifecycle that the Host path has replaced, rather than adding the fixes on top:
- fold
GitHubCopilotSubscriptionServiceinto a stateless “import existing local credential” adapter; - remove the unused
directmode fromuseOAuthLoginFlow; - route connection-detail re-login through the shared Host OAuth service;
- keep local
ghimport only as a secondary provider-catalog action; - remove the unused
isExperimentalEnabledpreload/IPC query surface.
The provider-specific device parsing and its security/error tests should stay. The goal is not to make OAuth artificially tiny; it is to leave one lifecycle authority and remove the roughly 280–330 lines whose responsibilities have moved to the Host.
A clean final shape would be: Runtime owns the GitHub device codec, Host owns attempt/entitlement/credential/refresh/401 recovery, and Desktop owns only generic presentation plus local credential discovery. Once that state is in one exact head with epoch 35 and focused entitlement/401 regressions, we can review the final result as a whole instead of asking for another sequence of patches.
AI-assisted review disclosure: OpenAI Codex coordinated independent exact-head reviews of the OAuth lifecycle, consent/provenance boundary, refresh behavior, and simplification opportunities. I reviewed and approved this final-state recommendation before posting.
Leave one lifecycle authority for Copilot accounts. The Host owns the attempt, the entitlement check, the credential, refresh, and 401 recovery; Desktop keeps only generic presentation and discovery of a credential this machine already holds. The device sign-in ships off. Maka has no published authorization for the editor OAuth identity the grant presents, so an install opts in explicitly with `MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=1`. This reverses the default chosen in "restore the GitHub Copilot sign-in in Settings": the flag records an operator accepting the consent mismatch for their own install, and cannot stand in for a basis that does not exist. A GitHub account is not a Copilot subscription. The provider slice now asks the Copilot API what the authorized account reaches, on the Host transport, before the coordinator's atomic commit; an account with no usable model fails the attempt as `provider_rejected` instead of committing a credential that breaks on first use. Copilot joins the single-401 refresh seam: force-refresh once, CAS-persist the new generation, replace the Authorization header, replay once. A record GitHub declared no lifetime for resolves to the token just rejected, so that replay is skipped rather than spent. Removed with their responsibilities: the Desktop subscription service, the `direct` mode in the login controller, the Copilot-specific re-login notice, and the unused `isExperimentalEnabled` query surface. Generated-by: Claude Code
An account Connection is created before anyone can ask the account what it has — the OAuth login path holds no credential at that point — so its enabled ids start as the provider's curated fallback list. Discovery then ran without ever reconciling them, leaving every guessed id selectable, testable, and sendable on an account that may never have exposed it. GitHub Copilot surfaced this: a connection test picks the first enabled id, the Copilot models endpoint does not list it, and the test fails on a model the user never chose while chat works on one they did. The local credential import was unaffected because it passes discovered ids at creation, so only the shared OAuth path carried the gap. Once a fetched inventory exists it is the authority: ids it does not list are dropped. Ids are never added, so a narrower selection the user made survives, and the discovered list is adopted whole only when nothing they kept remains. Generated-by: Claude Code
The notice opened the provider's device page and showed nothing else, but that page accepts nothing until the user types the one-time code the Host received. Re-login was therefore a button that led to a dead end. The code now appears beside the notice's own copy, for the flows whose device page asks for it. xAI is excluded because its verification URL carries the code itself, which is why the catalog panel has always hidden it there too. Generated-by: Claude Code
…b-copilot-device-flow-login # Conflicts: # packages/runtime-host/src/__tests__/protocol.test.ts # packages/runtime-host/src/protocol/index.ts
An account Connection is created before anyone can ask the account what it has — the OAuth login path holds no credential at that point — so its enabled ids start as the provider's curated fallback list, in the order this build ships them. Taking the first of those as the account's initial default names a model the account may never serve, which is how a GitHub Copilot connection test came to fail on a model the user never chose while chat worked on one they did. The live response now picks the order that first default is chosen from. It does not pick the set: an id the response omitted stays enabled, because a `/models` answer that cannot see a model is not evidence the account cannot run it — the rule `authorizeConnectionModel` states for every other reader of an inventory. This replaces the pruning added in "adopt the account's own model inventory after sign-in", which reached the same connection test by treating the live list as an allowlist over ids only the user writes. Generated-by: Claude Code
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for taking this through to a much cleaner final state. I re-reviewed exact head b5da959.
The original blocking concerns are resolved: interactive enrollment now runs through HostOAuthCoordinator, Copilot entitlement is verified before the atomic credential commit, rejected access tokens join the shared single-401 refresh/replay path, and the borrowed OAuth identity is default-off behind an explicit per-install opt-in. The replaced Desktop lifecycle, direct login mode, special re-login path, and unused experimental query have also been removed.
I left two non-blocking P2 comments. They are two remaining expressions of the same ownership invariant: the selected Runtime Host should be the sole authority for enrollment availability, entitlement, and credential adoption, while Desktop should only present the flow and discover credentials that exist locally. Neither warrants another broad redesign or a Request Changes review; the clean final direction is described inline.
The provenance document correctly states that authorization to reuse the editor OAuth identity has not been established. This approval accepts the dormant, explicit-opt-in shipping boundary for this change; it does not assert or create authorization from GitHub.
AI-assisted review disclosure: OpenAI Codex coordinated independent exact-head reviews of OAuth lifecycle and concurrency, identity and shipping boundaries, refresh behavior, remote-Host compatibility, and simplification. I verified the final diff, relevant source paths, prior findings, current head, CI, review threads, and the approval recommendation before posting.
| * Desktop owns exactly one thing for GitHub Copilot: importing a credential | ||
| * that already exists on this machine (`gh` / a compatible PAT). Interactive | ||
| * enrollment is not here — the device grant runs through the Host's OAuth | ||
| * coordinator like every other account login, so there is one authority that |
There was a problem hiding this comment.
[P2] Keep local credential import discovery-only
Reachability: ② reasonable concurrent or multi-client path. The local adapter currently performs entitlement discovery in Desktop, then this handler reads the Host's current credential revision and commits the imported secret. If import A is waiting on model discovery while an interactive login B commits through HostOAuthCoordinator, A can resume, adopt B's latest revision as its expected value, and overwrite the newer login. A remote Host also validates the import through the Desktop network environment rather than the Host transport.
The clean final state is for Desktop to return only the locally discovered raw credential and for a Host adoption operation to own entitlement, generation/supersede, and the atomic credential commit alongside interactive login. That closes the race and preserves one network and credential authority without adding another Desktop guard.
There was a problem hiding this comment.
[P2] Reproduced on exact head 02ee0bf874912102984991b31ff64c588649506e: the local import can still replace a newer interactive login.
I paused local import A during its Desktop-side model discovery, committed interactive login B through the Host, and then resumed A. The handler queried B's current credential revision, used that revision as the expected value, and successfully committed A's local secret over B. The observed result was ok: true; the commit expected B's revision 2, so the compare-and-swap guard accepted the overwrite rather than rejecting stale work.
The current test only covers a local import with no overlapping login. Please keep Desktop limited to discovering the local raw credential, and let a Host adoption operation own entitlement, supersession/generation, network transport, and the atomic commit alongside interactive login. That gives both paths one ordering authority.
Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
resolved. |
…b-copilot-device-flow-login # Conflicts: # apps/desktop/src/renderer/settings/provider-connection-detail.tsx # apps/desktop/src/renderer/settings/use-oauth-login-flow.ts
The IPC adapter read this Desktop process's environment before calling the Host. Against a remote Runtime Host that is the wrong authority: a Host whose operator enabled GitHub Copilot enrollment was still refused whenever the Desktop process happened not to set the same variable, and no local answer could have been right, because the environment that governs enrollment is the Host's. The start now always reaches the Host, and its `operation_unavailable` refusal is what the renderer reports — kept as `experimental_disabled` so the user is told the enrollment is off rather than that authorization failed. Generated-by: Claude Code
…t-device-flow-login
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for moving the interactive device flow onto HostOAuthCoordinator; the previous stale-completion root cause is now closed, and token lifetime, refresh, entitlement-before-commit, supersede, and atomic credential commit all follow the existing Host seam. The local-import authority thread remains open, and I found one additional recoverability issue in entitlement classification inline.
AI-assisted review disclosure: OpenAI Codex coordinated three independent exact-head review passes. I verified the retained lifecycle and error-classification paths, existing threads, current-head checks, mergeability, and review state, and I made the final review decision.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — one final cancellation edge surfaced in the Copilot 401 replay path. It is bounded and does not corrupt the credential, so I am classifying it as P3 rather than another merge-blocking P2; the two existing P2 items remain the actual readiness gate.
AI-assisted review disclosure: OpenAI Codex delegated an independent protocol and cancellation review. I verified the retained path and severity against this exact head.
…able The entitlement gate turned every `/models` failure into "this account has no Copilot". A timeout, a dropped connection, a 429, a 5xx, or an unreadable body told a paying subscriber they were ineligible and sent them back through a device login that was never the problem — while no credential had been committed and the next attempt would usually have worked. Only a proven refusal is an entitlement answer now: a catalog the account could read that lists nothing, or a deterministic 401/403. Everything else keeps the provider status and surfaces as an authorization failure the user simply retries. The shared 401 replay seam also gains the request's own signal. A caller who cancels while the forced refresh is in flight is released with its cancellation instead of waiting out the refresh timeout and then receiving the stale 401 as though the credential had been rejected; the refresh itself is left to settle, because a spent grant is worth persisting even once nobody wants the replay. Generated-by: Claude Code
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — reviewed fresh against this head rather than relying on the earlier approval, since that one predates real work on the branch.
What convinced me, in the order I checked it:
The problem is framed correctly. A device authorization grant proves a GitHub account, and a GitHub account is not a Copilot subscription. Calling /models before completeInteractiveOAuthLogin and only committing credentials when reachable models come back is the distinction that keeps a successful sign-in from being mistaken for an entitlement. Conflating those two is the usual way this feature goes wrong, and it is handled here.
The RFC 8628 polling loop covers the cases that matter. authorization_pending, slow_down, access_denied and expired_token all arrive as HTTP 200 and are handled separately, with unknown errors terminating rather than spinning. The loop sleeps by interval first and stops against local expiry rather than trusting the server to say stop. The detail I appreciated most: each token request uses its own signal, because once the grant may already have been consumed, a cancellation that discards the response throws away a credential the user really did authorize — while the sleep and retry boundaries stay cancellable. That distinction is easy to miss.
verification_uri is validated before it is opened: HTTPS only, github.com or a subdomain, no embedded credentials. Handing a server-supplied URL to the system browser unchecked is the classic hole here, and it is closed.
Credentials do not get a private path. Device code, user code, expiry and interval stay in Host memory and never reach the Desktop capability, which only receives the URL and the display user code. Storage goes through the existing canonical vault with the shared refresh/CAS seam, and a GitHub token without a lifetime uses an explicit non-expiring sentinel instead of fabricating a refresh grant. The 401 path reuses the shared refresh-and-replay rather than adding a second one.
On scope, the change removes the duplicated device flow, polling and persistence that lived on the Desktop side instead of adding a parallel implementation, so the authority for this ends up in one place.
On the default being off: gating interactive login behind MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=1 looks right to me rather than over-cautious. The device flow reuses the GitHub editor OAuth identity, so the party that gave consent and the party holding the credential are not the same — leaving the existing gh/PAT import as the default path while that is true is the honest position, and it is the kind of thing worth stating in the code rather than deciding silently.
No P0–P3 findings. test and audit are both terminal green on this exact head.
Review assisted by AI (Claude Opus 5 and GPT-5.6, reviewed independently). Findings were verified against the files at this head; the reviewers are accountable for them.
…-device-flow-login # Conflicts: # apps/desktop/src/main/__tests__/github-copilot-subscription-service.test.ts # apps/desktop/src/main/oauth/github-copilot-subscription-service.ts # apps/desktop/src/main/runtime-host-github-copilot-ipc-main.ts # packages/runtime-host/src/__tests__/protocol.test.ts # packages/runtime-host/src/protocol/index.ts # packages/runtime/src/oauth-provider-contracts.ts
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
…-device-flow-login # Conflicts: # packages/runtime-host/src/__tests__/protocol.test.ts # packages/runtime-host/src/protocol/index.ts
|
Rebased onto current main — resolved as a merge rather than a rebase to preserve your existing approval, and since the repo squash-merges the branch history won't land on main either way. Happy to redo it as a true rebase if you'd prefer. main had moved again by the time I got to it, so this is against 6b48ed3. Two conflicts, both in packages/runtime-host: the compatibility epoch (this branch held 40, which upstream has since taken for the message-queue mutation ops — Copilot is now 43, and scripts/protocol-epoch-check.mjs passes), and protocol.test.ts, where I kept your new credential-rotation and provider-capacity tests and re-anchored the Copilot assertion to > 42. I also ran npm run write:asf-headers for the five files this branch added before the header gate existed. CI shows one failure: host-kernel.test.js "slow domain work preserves multiplexed requests" timed out on goal.query. That file is untouched by this branch and passes locally (506ms); given a2f2a1a it looks like the same flaky area. Could you re-run the job? I don't have permission to. |
|
Thanks — rebased and addressed both P3s. Epoch. Resolved as you asked: main's 50 and 49 kept verbatim, the github-copilot note renumbered to 51, constant at 51. Not folded under main's entry. The merge-result guard reads 50 → 51. I also reworded the note's second line — it previously justified the number by naming the two open PRs that had claimed 49, which reads as a reservation; it now records that the value is re-derived from main on each rebase. [P3] Import during an in-flight device grant. Confirmed, including the mechanism: activeAttempts lives in the main process, so it outlives a renderer remount, and useOAuthLoginFlow never consults the snapshot it surfaces. I took a narrower fix than seeding pendingAction, and want to flag the deviation rather than let it pass silently. Seeding disables the sign-in button too — and nothing prunes activeAttempts, so an attempt whose surface died before its cancel IPC landed leaves the snapshot reporting authorizing indefinitely. Seeding would make the panel permanently inert with no way out short of an app restart, since a fresh grant is what supersedes a stale attempt. So the flow now exposes hostAttemptPending — an authorization the Host is running that this surface does not own — and only the Copilot import button consults it. The clobbering route stands down; the primary route stays offered; labels and the shared guard are unchanged, so no other provider's panel shifts behavior. Three tests cover adoption on mount, settled states, and not double-reporting an attempt this surface started. Happy to switch to the seeded version if you'd still prefer it — it's a two-line change. [P3] Title fixed. Checks on the merged tree: build, typecheck, lint, format, knip, ASF headers, surface inventory, and the epoch guard all clean; Storybook smoke 195/195; desktop 1520, runtime-host 1251, runtime 3114 tests passing. |
…b-copilot-device-flow-login # Conflicts: # packages/runtime-host/src/protocol/index.ts
…b-copilot-device-flow-login # Conflicts: # packages/runtime-host/src/protocol/index.ts
Astro-Han
left a comment
There was a problem hiding this comment.
Adds RFC 8628 device authorization for Copilot in packages/runtime, driven by the Host's HostOAuthCoordinator, and deletes Desktop's 209-line stateful service plus the renderer's direct special case.
Worth more than the title says: the real result is removing a duplicate authentication authority — Copilot account state was living in the Desktop main process, against the Runtime-Host-is-sole-authority boundary. Re-reviewing at a54b4f7e6.
P2 — in a default build the most prominent button always fails. The gate is flag === '1', default off (oauth-provider-contracts.ts:117), but "Sign in with GitHub" renders unconditionally as variant="primary" (provider-oauth-section.tsx:345) while the working import-credentials path is secondary.
P2 — the typed reason has no consumer. experimental_disabled is produced at runtime-host-oauth-ipc-main.ts:125; nothing on the OAuth side reads it, and the renderer actually matches an English error string. Add a language or reword the error and the check silently stops working.
P2 — signed-out copy still describes PATs, unrelated to the primary action.
The daily-review-coordinator concurrency fix is unrelated to Copilot sign-in and belongs in its own PR; the subscription-model-fetch signal work is a separable second intent. Not defects — reviewability.
The split is half-done in one place: the local enable gate was correctly removed, but "ask the Host for the answer" wasn't put in its place.
On the borrowed client identity: main already ships app_EMoamEEZ... (Codex, default on) and the xAI id, so this PR — default off — is more conservative than the status quo. That makes it a project-level governance question for dev@maka.apache.org, not a blocker here. I'll start that thread.
AI-assisted review: Claude Code subagents ran security and architecture passes; I verified the default-off gate, the primary-button rendering, and the two client IDs already on main myself. No tests or typecheck run. AI review is not independent human review.
简体中文
实际成果比标题大:把 Copilot 账户状态机从 Desktop 主进程搬回 Runtime Host,消除了一个重复认证权威。三条 P2 都不影响正确性:默认构建下主按钮必然失败;experimental_disabled 这个结构化原因码无人消费,实际靠英文错误串正则;未登录文案仍只讲 PAT。daily-review-coordinator 的并发修复属夹带,建议单开。借用 client id 的问题 main 上已存在且更激进,是 dev@ 的议题,不卡这个 PR。
a54b4f7 to
3fa96bf
Compare
|
Thanks for the thorough review — all three P2s addressed in the latest push, and I've split the branch to a single intent. P2 — primary button always fails in a default build. Fixed by putting "ask the Host for the answer" where the removed local gate used to be. Added an oauth.enrollment.query Host operation; HostOAuthCoordinator answers it from the same isProviderEnabled gate that governs the start, so there's still exactly one authority and it works for remote Hosts. The panel now probes on mount: when the Host reports enrollment disabled, "Sign in with GitHub" is disabled with an explanatory tooltip and Import is promoted to primary, so the prominent action is one that actually succeeds. Unknown-while-probing is treated as enabled, so a slow Host never hides sign-in. P2 — typed reason had no consumer. subscriptionResultMessage now branches on the typed reason === 'experimental_disabled', threaded through from the start/open/complete failure paths. The English-string match is kept only as a fallback, so rewording the Host message or adding a locale no longer silently breaks the check. P2 — signed-out copy still described PATs. Reworded copilotSetup (zh + en) to describe device sign-in and gh credential import, and added the tooltip copy. Tests: added protocol decode, coordinator per-provider gate, and IPC-main get-enrollment-state coverage; runtime-host and desktop typecheck + affected suites pass locally. Reviewability — daily-review split. Agreed it's unrelated. The daily-review-coordinator flight-claim fix is pulled out of this branch onto its own; this PR is now Copilot sign-in only. I'll open the daily-review PR separately. On subscription-model-fetch: that signal is entangled with adopting the account's own model inventory after sign-in, so I've left it here rather than risk a messy split — happy to separate it if you'd prefer. Re: the borrowed client identity — agreed it's a dev@ governance question and not a blocker here; will follow that thread. |
|
Two things on the current head, one mechanical and one worth a look before merge. CI is red on this branch's own stories. P2 — the entitlement question is answered twice, and the two answers disagree in a way that misdirects the user.
So when GitHub is briefly unreachable, a user importing an existing Both files are new in this PR, so this is one fact with two implementations introduced together rather than an inherited seam. Having the import path call Non-blocking — the import path still works, and the failure is recoverable by retrying. It's the message that's wrong, not the outcome. AI-assisted review: I traced both entitlement paths, the story mock gap, and the coordinator's error mapping against head 简体中文两点,一条机械性,一条建议合并前看一下。 CI 红在本分支自己的 story 上。 P2 — entitlement 这个问题被回答了两次,两个答案不一致,且会误导用户。
于是 GitHub 短暂不可达时,导入既有 两个文件都是本 PR 新增的,所以这是同时引入的“一个事实两处实现”,不是继承的接缝。让导入路径调用 不阻塞——导入路径仍然可用,重试即可恢复。错的是文案,不是结果。 |
…sign-in Address three review findings on the Copilot device-flow sign-in: - The renderer kept no local enrollment gate but never asked the Host for the answer either, so the primary "Sign in with GitHub" button always failed in a default (opt-in) build. Add an oauth.enrollment.query Host operation; the panel disables sign-in with an explanatory tooltip and promotes Import to primary when the Host reports enrollment disabled. - The typed experimental_disabled reason had no consumer: the renderer matched an English error string instead. Branch on the typed reason, keeping the string match only as a fallback. - The signed-out copy still described minting a fine-grained PAT. Reword it to describe device sign-in and gh credential import. Adds protocol, coordinator, IPC-main, and decode tests.
3fa96bf to
4096d9d
Compare
Adopt main's connection-scoped OAuth architecture for GitHub Copilot: - Copilot joins InteractiveOAuthProviderType and gets a slug base, so the Host admits its interactive login instead of refusing it as 'hidden'. - The Copilot panel drives main's shared browser-assisted controller in 'create' mode; importing a local gh credential stays the secondary route. - Desktop OAuth IPC, preload, and bridges adopt main's create/existing targets and connection-scoped account/refresh/logout calls. - The catalog row derives its state from the Connection catalog like every other provider, replacing the retired provider-wide account snapshot. - Compatibility epoch re-derived from main: 79 -> 80. Keeps this branch's Host-authoritative enrollment query (oauth.enrollment .query / get-enrollment-state) behind the sign-in disable + tooltip, and the typed experimental_disabled reason.
…llment gates The device grant reached the Host but its credential commit failed with persistence_failed: the durable OAuth login receipt decoder and the enrollment intent both still closed over codex/xai only, so a Copilot attempt was refused after the user had already authorized it. Covered by the Copilot coordinator tests, which now reach 'authenticated' instead of 'failed'.
|
Hi @Astro-Han , since it's a big change and touched many files, it's easy to have conflicts. Instead of always fixing conflicts, could you help to review if any issue? After all issues being addressed, the only thing is fix conflicts and pass CI, which makes it easier. thanks! |
Resolve the current connection-catalog, provider-registry, OAuth bridge, dependency, and Runtime Host protocol conflicts. Keep Copilot on main's connection-scoped OAuth architecture, preserve the Host enrollment query, and re-derive the incompatible protocol epoch from live main: 98 -> 100 (epoch 99 is already claimed by open PRs).
The upstream OAuth refactor reintroduced a Desktop environment precheck and an unused renderer direct-login mode. Remove both so every interactive start reaches the selected Host, whose operation_unavailable result remains the typed experimental_disabled reason consumed by the UI. Also return the verified Copilot model list from the entitlement verifier so local import makes one /models request. A transient second request can no longer fall through to misleading unsupported-credential copy. Add coverage that Desktop calls a refusing Host and that local import performs one provider request.
Keep Desktop limited to discovering and shape-checking credential material. Delegate provider discovery and persistence to connection.onboarding.save, which now accepts canonical OAuth material and can enable the non-empty model set discovered by the selected Host. The Host ticket pins the connection, credential, request headers, and proxy before provider I/O. If an interactive login commits while local adoption is blocked, adoption returns superseded instead of rereading and overwriting the newer credential. Durable onboarding commit and recovery derive the correct api_key or oauth_token locator from the provider. Add regressions for first-time OAuth adoption and the exact paused-import / newer-interactive-login race.
Retain main's ScheduledTask Connection-identity epoch at 99 and re-derive the Copilot OAuth protocol generation at the first unclaimed value, 101; epoch 100 is currently claimed by other open PRs.
…seam Fix the renderer architecture ratchet without expanding its debt ledger: - expose Host-bound OAuth provider adapters through ConnectionsBridge - create those adapters only in the allowed Desktop platform module - remove direct window.maka access from the legacy OAuth catalog/detail files - delete the runtime-host-settings-bridge middle-man - fold enrollment availability into the existing flow feedback state and reuse the controller's error reporter, restoring the base hook counts This removes bridge and hook debt relative to the PR base while preserving Copilot device sign-in, local import, relogin, and Host availability probing.
Fix the Desktop Knip gate after Host-owned Copilot adoption: - delete the unused fixed OAuth connection-slug module - remove superseded provider-wide create/credential/disable helpers - restore account synchronization tests to the behavior retained from main - stop re-exporting the internal OAuth target type - declare @babel/parser in the Desktop workspace that owns the renderer architecture script The exact command is clean.
The fresh Storybook bundle no longer has the legacy window.maka OAuth path. Provide the same bound ConnectionOAuthBridge used by production to direct provider stories, and update Host-generation revalidation to assert the current Host-owned enrollment availability instead of retired provider-wide Copilot account email state. Also label GitHub Copilot as an add-account intent now that it is a connection-scoped interactive OAuth provider rather than an import-only singleton.
Retain main's optional side-conversation source-turn epoch at 100 and re-derive the Copilot OAuth generation at 102; epoch 101 is already claimed by other open PRs.
Astro-Han
left a comment
There was a problem hiding this comment.
Re-approving at 3f07d3f3d. The earlier approval was anchored at a54b4f7e6, so this is a fresh read of the five commits since then plus the two merges, and a size audit of the whole change rather than only the delta.
Is this the smallest complete solution?
I went looking for what could come out. Measured against merge base e1e108a79: production is +1336/-972 across 34 files, tests are +1368/-389 across 18 files, docs +79, lockfile and ledger -148.
Production, by layer:
| area | files | added | removed | net |
|---|---|---|---|---|
packages/runtime (device grant, credential lifetime) |
4 | 493 | 14 | +479 |
packages/runtime-host (Host authority, protocol) |
7 | 190 | 25 | +165 |
packages/storage (onboarding, credential locator) |
4 | 39 | 20 | +19 |
packages/core (registry) |
1 | 8 | 1 | +7 |
apps/desktop/src/main |
6 | 190 | 423 | -233 |
apps/desktop renderer and preload |
12 | 416 | 489 | -73 |
Tests, by layer: runtime +404, runtime-host +634, storage +3, stories +35, Desktop -97.
That distribution is the answer to the size question. The weight lands in the two packages that own execution and protocol, and Desktop loses 306 net lines because the duplicate Copilot lifecycle it used to hold is gone. A feature that grows the authority and shrinks the client is the shape you want.
Things I checked for and did not find:
- A private route for Copilot.
packages/runtime/src/github-copilot-oauth-enrollment.tssits besidecodex-oauth-enrollment.ts(263 lines) andxai-oauth-enrollment.ts(204 lines) with the same start/poll export shape, and reuses the shared primitives inoauth-login.ts. Enrollment, refresh, logout and account state all ride the existingHostOAuthCoordinatorchannels rather than new ones. - A forwarding layer that buys nothing.
ConnectionOAuthProviderBridgeinfeatures/connection-settings/ports.tsplus its adapter inplatform/desktop/create-connection-settings-services.tsreplace the deletedsettings/runtime-host-settings-bridge.ts. That is a move, not an addition: one renderer module fewer, andrenderer-architecture.jsonshrinks by 35 net lines while the ratchet passes. - The same fact stored twice. The pre-click enrollment probe and the typed
experimental_disabledrefusal look like a pair until you notice both derive from the same Host answer at different moments, and the Desktop-side copy of the gate is exactly whate5d0aea2cdeletes. - A permutation test matrix. The added test names map to distinct classifications: error body under HTTP 200, verification host, slow_down with and without an advertised interval, denial versus elapsed window, expiring grant without a refresh token, entitlement refusal versus entitlement unavailable. The adoption race regression in
connection-effect-coordinator.test.tsdrives the realHostConnectionEffectCoordinatoragainst real stores in a temp root, not a stub.
One thing I would take out, but not here:
[P3] This is the third near-copy of the device authorization loop. Normalizing whitespace and comments, 125 of the 170 statement lines in xai-oauth-enrollment.ts also appear in the Copilot module (ratio 0.74); codex to Copilot is 0.44. The real differences are narrow: GitHub reports pending, slow_down and malformed requests as HTTP 200 with an error body, honors an advertised retry interval, has its own verification host and its own token shape. Those are the kind of differences OAUTH_PROVIDER_CONTRACTS already parameterizes. A shared driver would absorb roughly a hundred lines per provider, but it also rewrites the login path of two shipped providers, so it is its own change and not a condition on this one.
What changed since the previous approval
Credential authority is now singular, and the earlier split is closed. isOAuthEnrollmentProviderEnabled has no Desktop copy left; the only production caller is packages/runtime-host/src/server/execution-composition.ts, the is-experimental-enabled IPC channel and its preload entries are gone, and a refused start now comes back as the Host's operation_unavailable mapped to the typed experimental_disabled reason the renderer already reads. The renderer no longer has a second way to reach or write Copilot credentials: provider-oauth-section.tsx and use-connection-detail.ts both hold zero bridge paths in the ledger.
The paused-import over newer-login race is genuinely closed, not papered over. Adoption goes through connection.onboarding.save, which pins the ticket before provider I/O, so a blocked import returns superseded instead of re-reading and overwriting. The regression exercises exactly that interleaving through the production coordinator.
Local import lost its second provider request. The entitlement verifier now returns the models it already fetched, so a transient second call can no longer turn a good credential into unsupported-credential copy.
Compatibility epoch is resolved. The branch declares 102 and main has since moved to 101, so 102 is now exactly the next value rather than a reservation. Both merges keep main's earlier entries with their original wording. Across the two merges only three files carry a conflict resolution: protocol/index.ts, protocol.test.ts and renderer-architecture.json. #4508, #4440 and #4321 arrive intact.
The primary-button problem from the last review is fixed. When the Host refuses enrollment the sign-in button turns secondary and disabled with an explanatory tooltip, and import becomes primary.
One mechanical thing before it lands
main moved while I was reading: #4601 and #4608 landed and took the epoch to 101, so the branch is now DIRTY. The only conflicting file is packages/runtime-host/src/protocol/index.ts, and the only conflicting line is the epoch itself, where main went 100 to 101 and this branch goes 100 to 102. Resolve it as 102 and the value is still correct. Everything else automerges.
Remaining, none blocking
[P2] The entry-point copy still describes the old route only. Normal user path. copilotDescription ("Import compatible GitHub credentials to connect a Copilot subscription.") is the catalog card, and copilotSubtitle ("Import a compatible GitHub sign-in.") is the setup header, while the panel's primary action is now Sign in with GitHub. copilotSetup was updated; these two were not.
[P3] Dead locale key. reverifyFailedRetry in both locales lost its only caller when the direct re-verify action was removed. I deleted both lines locally: Desktop typecheck and Biome lint stay green.
[P3] The epoch 102 note is incomplete. It records the OAuth provider set and oauth.enrollment.query, but not that connection.onboarding.save now accepts empty enabledModelIds as enable-everything-discovered and that apiKey may carry serialized OAuth material. The handshake keeps mixed peers apart either way, so this is ledger accuracy, not a safety gap.
[P3] ConnectionsBridge.oauth is optional. oauthLoginServiceFor returns null when it is absent, so a services fixture that omits it drops the re-login action silently instead of failing where the mistake is.
[P3] Editorial. In docs/github-copilot-oauth-identity.md, "the interactive device flow does not ship on:" followed by that bullet list does not parse.
On the trust boundary: the local gh token now reaches the selected Host before any provider check, where before it was probed on the Desktop network first. That is the correct direction. The credential is used from the Host, so it should be validated over the Host's transport, and it goes to the Host the user selected and nowhere else. No new party sees the token.
Verification
Run on 3f07d3f3d in a clean worktree with its own npm ci:
@maka/runtime-hosttest:dist: 1616 passed, 0 failed@maka/storagetest:dist: 1086 passed, 0 failed@maka/runtimetest:dist: 3177 passed, 0 failed@maka/desktoptest:dist: 1980 passed. One failure,computer-use-host.test.js, was my own missing@maka/computer-usebuild; it passes once that workspace is built.- typecheck: runtime, runtime-host, storage, desktop all clean
npm run format:checkandnpm run lint: clean- renderer architecture ratchet against
git merge-base origin/main HEAD: passed npx knip --workspace apps/desktopand--workspace packages/ui: clean
Evidence boundary: this is source reading plus the runs above. I did not launch the app, did not run a real device grant against GitHub, and did not exercise a remote Host, so the end-to-end sign-in and the remote-Host adoption path are reasoned from code and tests rather than observed.
AI-assisted review disclosure: Claude Code ran the diff reading, the ablation experiments and the verification runs. I checked the size numbers, the single-authority claim, the merge resolutions and the epoch state myself against this head. AI review is not independent human review.
简体中文
在新 head 3f07d3f3d 上重新批准。上次绿勾锚在 a54b4f7e6,这次重读了之后的五个提交和两次 merge,并对整个改动做了一遍体量核查。
先说体量:这个大小是必要的。相对 merge base,生产代码 +1336/-972(34 个文件),测试 +1368/-389(18 个文件)。分层看,增量集中在 packages/runtime 和 packages/runtime-host 这两个真正拥有权威的地方,而 Desktop 净减 306 行,因为它原来那套重复的 Copilot 生命周期被删掉了。功能往权威层长、往客户端缩,这个形状是对的。
我逐个试着删过:按 provider 一个 enrollment 模块是仓库已有的接缝,不是新开的路;渲染层那个 OAuth port 是把已删掉的 runtime-host-settings-bridge.ts 搬过来,模块数减一,架构账本净减 35 行;点击前的探测和点击后的 typed reason 不是同一个事实存两份,它们都来自 Host,只是时机不同;测试也不是同一件事的排列,导入竞态那条回归跑的是真的 coordinator 和真的存储。
唯一确实可以少一份、但不该在这个 PR 里做的:这是仓库里第三份几乎一样的设备授权轮询循环。去掉空行和注释后,xAI 那份 170 行里有 125 行在 Copilot 模块里也出现(0.74)。差异只有 GitHub 用 HTTP 200 加 error body 报 pending 和 slow_down、会给出建议间隔、验证域名和 token 形状不同这几处,都是 OAUTH_PROVIDER_CONTRACTS 本来就在参数化的东西。但真做这件事要顺带改写两个已发布 provider 的登录路径,属于另一个改动。
增量部分:凭据权威现在只剩 Host 一个,Desktop 那份本地开关副本已删,渲染层没有第二条路能拿到或写入凭据;导入被阻塞时会返回 superseded 而不是覆盖更新的登录,而且有跑真实 coordinator 的回归;本地导入少了一次多余的 provider 请求;epoch 分支声明 102,而 main 这期间已经走到 101,所以 102 正好是下一个,两次 merge 只有三个文件带冲突解决,#4508、#4440、#4321 都完整落下;上一轮提的主按钮必然失败的问题已经修好。
合并前有一件机械的事:我读的过程中 main 动了,#4601 和 #4608 落地把 epoch 推到了 101,分支现在是 DIRTY。冲突只有 packages/runtime-host/src/protocol/index.ts 一个文件,冲突的也只有 epoch 那一行,main 是 100 改 101,这个分支是 100 改 102。按 102 解掉就对了,其余全部自动合并。
剩下几条都不卡合并:目录卡片和设置页副标题的文案还只讲导入凭据,和现在的主按钮对不上;reverifyFailedRetry 两个语言都已无人使用,我本地删掉后 typecheck 和 lint 仍然通过;epoch 102 的说明没有记录 onboarding 协议这次放宽的部分;ConnectionsBridge.oauth 是可选的,fixture 少给它会静默丢掉重新登录入口;provenance 文档里有一句话读不通。
信任边界方面,本地 gh token 现在先送到所选 Host 再做校验,方向是对的:凭据本来就在 Host 上使用,就该用 Host 的传输去验证,而且只发给用户自己选的那个 Host,没有多出第三方。
验证:runtime-host 1616、storage 1086、runtime 3177 全绿,Desktop 1980 通过(唯一一条失败是我自己漏建 computer-use,补上后通过);四个 workspace 的 typecheck、format、lint、渲染层棘轮(base 用 merge-base)、以及 CI 实际卡的两个 knip workspace 都干净。
证据边界:只做了源码阅读和上述执行,没有启动应用、没有真的走一次 GitHub 设备授权、也没有验过远程 Host,所以端到端登录和远程 Host 的导入路径是从代码和测试推出来的。
本次评审有 AI 参与:Claude Code 负责读 diff、做消融实验和跑验证,体量数据、单一权威的结论、merge 解决和 epoch 状态由我自己在这个 head 上核对。AI 评审不等于独立的人类评审。
Retain main's Session Guest Turn-request epoch at 101 and re-derive the Copilot OAuth protocol generation at 103; epoch 102 is already claimed by other open PRs. Complete the epoch note with the OAuth onboarding wire semantics introduced by this branch.
Address the remaining review findings: - describe both GitHub sign-in and compatible credential import in the catalog card and setup subtitle - remove the dead reverifyFailedRetry locale key - require ConnectionsBridge.oauth so incomplete adapters and fixtures fail at typecheck rather than silently dropping re-login - update the two story fixtures to provide that required port - repair the OAuth identity document's safeguards sentence The separate cross-provider device-loop deduplication remains intentionally out of scope because it would rewrite two shipped provider paths.
Astro-Han
left a comment
There was a problem hiding this comment.
Re-approving on 9b91ae62f, since my previous approval was anchored to 3f07d3f3d. I read the one commit you added on top rather than the merge, and all five items match what you wrote.
The catalog card and setup subtitle now describe both routes, which is what the flow actually offers. The dead reverifyFailedRetry key is gone. ConnectionsBridge.oauth is required rather than optional, so the two runtime guards (if (!bridge.oauth) throw and if (!oauth) return null) are replaced by a type obligation and both story fixtures provide the port. That is the right direction: one fewer representable state instead of two more checks. The safeguards sentence in docs/github-copilot-oauth-identity.md now matches the list under it, and the safeguards themselves are unchanged, still off by default behind MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=1 with the local-credential import as the supported default route.
Epoch 103 is correct: main is at 101 and #4439 claims 102.
17 checks green, 1 skipped, mergeable and clean. Merging this.
简体中文
在 9b91ae62f 上重新批准,上一个绿勾锚在 3f07d3f3d。我看的是你新加的那一个 commit,不是 merge,五条都和你写的对得上。
目录卡片和设置副标题现在把两条路都写上了,这才是这个流程实际提供的能力;没用的 reverifyFailedRetry 删了;ConnectionsBridge.oauth 从可选改成必需,两处运行时守卫换成了类型义务,两个 story fixture 也补上了这个 port,这个方向是对的,少一个可表示的状态好过多两道检查;文档里那句 safeguards 现在和它下面的列表对得上了,防护本身没变,仍然默认关闭、要 MAKA_GITHUB_COPILOT_DEVICE_LOGIN_EXPERIMENTAL=1 才开,导入本地凭据仍是默认支持的路线。
epoch 103 是对的:main 在 101,#4439 占了 102。
17 项绿、1 项跳过,可合并且干净,我合了。
Summary
GitHub Copilot was the only registered
oauth_tokenprovider without a real sign-in path. Desktop only scavenged an existingCOPILOT_GITHUB_TOKEN/GH_TOKEN/GITHUB_TOKEN/gh auth token, rejected classic PATs, and left users minting a fine-grained PAT with Copilot Requests.This PR adds the missing RFC 8628 device-flow enrollment in
@maka/runtimeand wires it into Desktop Settings:beginDeviceLoginrequests a user code;completeDeviceLoginpolls until authorizedghimport adopt an account only after it proves usable modelsopenExternalbridge); enrollment pins the URL to github.com and requests onlyread:userImporting an existing
ghcredential remains available as a secondary action.Fixes #3388
Verification
lint/typecheck/ workspace suites not re-run in this PR-opening step; rely on CItestcheckScreenshot
Sign in
Successful GitHub Copilot subscription account import after sign-in:
AI use
Select exactly one:
Tool(s) and scope: GitHub Copilot CLI assisted implementation of the device-flow enrollment, Desktop wiring, tests, commit message cleanup, and PR creation. The human contributor remains responsible for the change.
Checklist
Does this PR entail a change in behavior?