From 30619616c499112c2885b5ad2cae33a9e2775932 Mon Sep 17 00:00:00 2001 From: stuffbucket <231133237+stuffbucket@users.noreply.github.com> Date: Thu, 11 Jun 2026 13:04:49 -0700 Subject: [PATCH] =?UTF-8?q?fix(auth):=20gh-reuse=20=E2=80=94=20validate=20?= =?UTF-8?q?before=20reboot=20+=20refresh=20discovery=20on=20focus?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the gh-reuse feature to a clean state. Two gaps: 1. Errors were handled safely but imprecisely: a stale/no-subscription gh account got written + rebooted into, then surfaced a generic 'came back unauthenticated' ~20s later. Now POST /gh/use PRE-FLIGHTS the token against Copilot (/copilot_internal/user) BEFORE writing/rebooting and returns a specific 422 — 'token expired or revoked' (401) vs 'no Copilot subscription' (403/404) vs a network message — which the UI shows immediately, no reboot. The shell now extracts {error:{message}} so the specific reason renders. Also closes the getModels timeout gap (it was missing from the #110 set; cacheModels is on the boot critical path). 2. gh discovery was only refreshed on entering the Account section — adding an account in gh while settings was already open didn't show up. Now refreshes on window focus / visibilitychange (the natural 'returned from the terminal' moment) + a manual Refresh button. No polling (gh auth status hits the keyring). +6 pre-flight unit tests (status→message mapping, DI'd usage fn — no mock.module). Full suite green (824); valid-account import still 200 (no happy-path regression); shell tsc clean. --- shell/index.html | 13 ++++++++- shell/src/main.ts | 37 ++++++++++++++++++++++++- shell/src/styles.css | 8 ++++++ src/routes/settings/gh.ts | 40 ++++++++++++++++++++++++++- src/services/copilot/get-models.ts | 4 +++ tests/gh-preflight.test.ts | 44 ++++++++++++++++++++++++++++++ 6 files changed, 143 insertions(+), 3 deletions(-) create mode 100644 tests/gh-preflight.test.ts diff --git a/shell/index.html b/shell/index.html index 24b6108a..5e42f087 100644 --- a/shell/index.html +++ b/shell/index.html @@ -119,7 +119,18 @@

Not signed in

or
-

Reuse a GitHub CLI account

+
+

+ Reuse a GitHub CLI account +

+ +

Sign in with an account you’re already logged into with gh on this Mac — no code diff --git a/shell/src/main.ts b/shell/src/main.ts index 69a42cf6..263f4ef9 100644 --- a/shell/src/main.ts +++ b/shell/src/main.ts @@ -638,6 +638,21 @@ function hideGhError(): void { el.textContent = ""; } +/** apiCall surfaces the raw response body in `error`. Our settings endpoints + * reply `{ error: { message } }`; pull that out so the UI shows the specific + * reason (e.g. the /gh/use pre-flight message) rather than raw JSON. */ +function apiErrorMessage(raw: string): string { + try { + const parsed = JSON.parse(raw) as { error?: { message?: string } }; + if (typeof parsed.error?.message === "string" && parsed.error.message) { + return parsed.error.message; + } + } catch { + // not JSON — use the raw text + } + return raw; +} + /** * Populate the "reuse a GitHub CLI account" list from GET /gh/status. The * wrapper stays hidden unless gh is installed AND has ≥1 account — a user @@ -732,8 +747,11 @@ async function useGhAccount(button: HTMLElement): Promise { for (const b of signInButtons) b.disabled = false; buttonEl.textContent = originalLabel; row?.removeAttribute("aria-busy"); + // The pre-flight (POST /gh/use) returns a specific reason — stale token, + // no Copilot subscription, etc. — caught BEFORE any reboot. Show it. showGhError( - "Couldn't sign in with that account. Try the code-based sign-in above.", + apiErrorMessage(result.error) || + "Couldn't sign in with that account. Try the code-based sign-in above.", ); buttonEl.focus(); return; @@ -995,6 +1013,9 @@ function wireAccount(): void { case "gh-use": void useGhAccount(button); break; + case "gh-refresh": + void loadGhAccounts(); + break; default: break; } @@ -1030,3 +1051,17 @@ window.addEventListener("hashchange", () => { stopAuthPolling(); } }); + +// Refresh on returning to the window. The common flow is: open the Account +// section here, switch to a terminal, `gh auth login` (or out) of an account, +// then switch back — re-loading auth status on focus re-runs gh discovery so +// the new account appears without leaving + re-entering the section. We do NOT +// poll (gh auth status touches the OS keyring); focus/visibility is the right +// "moment of attention" to refresh. +function refreshAccountOnAttention(): void { + if (readHashSection() === "account") void loadAuthStatus(); +} +window.addEventListener("focus", refreshAccountOnAttention); +document.addEventListener("visibilitychange", () => { + if (document.visibilityState === "visible") refreshAccountOnAttention(); +}); diff --git a/shell/src/styles.css b/shell/src/styles.css index 633fef4b..c8cd5511 100644 --- a/shell/src/styles.css +++ b/shell/src/styles.css @@ -252,6 +252,14 @@ a:hover { display: none; } +/* Title + a quiet "Refresh" affordance on one row. */ +.gh-reuse__header { + display: flex; + align-items: baseline; + justify-content: space-between; + gap: var(--space-3); +} + /* Centered "or" rule between the device-code button and the gh list. */ .gh-reuse__divider { display: flex; diff --git a/src/routes/settings/gh.ts b/src/routes/settings/gh.ts index 5401ed0a..40eb8fdf 100644 --- a/src/routes/settings/gh.ts +++ b/src/routes/settings/gh.ts @@ -8,9 +8,39 @@ import { Hono } from "hono" -import { forwardError } from "~/lib/error" +import { forwardError, HTTPError } from "~/lib/error" import { makeRecord, writeDefaultRecord } from "~/lib/github-token-store" import { detectGhCli, getGhAccountToken } from "~/services/gh-cli" +import { getCopilotUsage } from "~/services/github/get-copilot-usage" + +/** + * Verify a gh account's token actually works for Copilot BEFORE we adopt it. + * Mirrors what boot does (the live identity check that was failing silently): + * a stale/revoked token → GitHub rejects it; an account with no Copilot → + * /copilot_internal/user 403/404. Returns a specific, user-facing message so + * the UI can say WHY synchronously, instead of writing the token, rebooting, + * and surfacing a generic "came back unauthenticated" 20s later. Returns null + * when the token is usable. + */ +export async function preflightCopilotError( + token: string, + login: string, + usage: (token: string) => Promise = getCopilotUsage, +): Promise { + try { + await usage(token) + return null + } catch (error) { + const status = error instanceof HTTPError ? error.response.status : 0 + if (status === 401) { + return `GitHub rejected ${login}'s token — it may be expired or revoked. Run \`gh auth login\` and try again, or sign in with a code.` + } + if (status === 403 || status === 404) { + return `${login} doesn't have access to GitHub Copilot. Pick another account, or sign in with a code.` + } + return `Couldn't verify ${login} with GitHub${status ? ` (HTTP ${status})` : ""}. Check your connection and try again.` + } +} export const ghRoutes = new Hono() @@ -70,6 +100,14 @@ ghRoutes.post("/use", async (c) => { ) } + // Pre-flight: confirm the token works for Copilot BEFORE writing it + + // rebooting, so a stale/no-subscription account fails fast with a specific + // reason rather than a generic post-reboot "came back unauthenticated". + const preflightError = await preflightCopilotError(token, login) + if (preflightError) { + return c.json({ error: { message: preflightError } }, 422) + } + await writeDefaultRecord(makeRecord(token)) return c.json({ ok: true, login, host }) } catch (error) { diff --git a/src/services/copilot/get-models.ts b/src/services/copilot/get-models.ts index 29c769ed..1092ff97 100644 --- a/src/services/copilot/get-models.ts +++ b/src/services/copilot/get-models.ts @@ -2,12 +2,16 @@ import consola from "consola" import { copilotBaseUrl, copilotModelsHeaders } from "~/lib/api-config" import { HTTPError } from "~/lib/error" +import { GITHUB_API_TIMEOUT_MS } from "~/lib/http-timeouts" import { state } from "~/lib/state" export const getModels = async () => { consola.info(`Fetching models from ${copilotBaseUrl(state)}/models`) const response = await fetch(`${copilotBaseUrl(state)}/models`, { headers: copilotModelsHeaders(state), + // Bounded like the other auth/discovery fetches — cacheModels runs on the + // cold-boot critical path, so an unbounded hang here would stall boot. + signal: AbortSignal.timeout(GITHUB_API_TIMEOUT_MS), }) if (!response.ok) { diff --git a/tests/gh-preflight.test.ts b/tests/gh-preflight.test.ts new file mode 100644 index 00000000..0d294e7f --- /dev/null +++ b/tests/gh-preflight.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, test } from "bun:test" + +import { HTTPError } from "~/lib/error" +import { preflightCopilotError } from "~/routes/settings/gh" + +const ok = () => Promise.resolve({ copilot_plan: "enterprise" }) +const throwsHttp = (status: number) => () => + Promise.reject(new HTTPError("x", new Response(null, { status }))) + +describe("preflightCopilotError", () => { + test("returns null when the token works for Copilot", async () => { + expect(await preflightCopilotError("gho_x", "alice", ok)).toBeNull() + }) + + test("401 → expired/revoked, names the account", async () => { + const msg = await preflightCopilotError("gho_x", "alice", throwsHttp(401)) + expect(msg).toContain("expired or revoked") + expect(msg).toContain("alice") + }) + + test("403 / 404 → no Copilot subscription", async () => { + for (const status of [403, 404]) { + const msg = await preflightCopilotError( + "gho_x", + "bob", + throwsHttp(status), + ) + expect(msg).toContain("doesn't have access to GitHub Copilot") + } + }) + + test("other status → generic verify message including the status", async () => { + const msg = await preflightCopilotError("gho_x", "carol", throwsHttp(503)) + expect(msg).toContain("Couldn't verify") + expect(msg).toContain("503") + }) + + test("non-HTTP error (network) → generic verify message", async () => { + const msg = await preflightCopilotError("gho_x", "carol", () => + Promise.reject(new Error("network down")), + ) + expect(msg).toContain("Couldn't verify") + }) +})