From a51fbfc9e7d2631a62c5352957a33eae74222243 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:36:32 -0400 Subject: [PATCH 1/2] ship: prepare lane for review --- apps/ade-cli/README.md | 9 + .../src/headlessLinearServices.test.ts | 50 ++++ apps/ade-cli/src/headlessLinearServices.ts | 55 ++-- .../services/github/githubCredentialHealth.ts | 6 + .../main/services/github/githubRateLimit.ts | 14 ++ .../src/main/services/github/githubService.ts | 5 +- .../services/github/githubStatusPage.test.ts | 169 +++++++++++++ .../main/services/github/githubStatusPage.ts | 181 ++++++++++++++ .../src/main/services/prs/prService.test.ts | 50 ++++ .../src/main/services/prs/prService.ts | 9 + .../app/IntegrationBannerHost.test.tsx | 143 +++++++++++ .../components/app/IntegrationBannerHost.tsx | 51 +++- .../github/GitHubAppInstallPanel.tsx | 9 + .../components/settings/GitHubSection.tsx | 98 ++++++-- .../lib/githubIntegrationStatus.test.ts | 96 +++++++ .../renderer/lib/githubIntegrationStatus.ts | 118 ++++++++- .../src/shared/githubOperationCredential.ts | 9 +- .../src/shared/githubServiceHealth.test.ts | 149 +++++++++++ .../desktop/src/shared/githubServiceHealth.ts | 235 ++++++++++++++++++ apps/desktop/src/shared/types/git.ts | 19 +- docs/ARCHITECTURE.md | 2 +- .../onboarding-and-settings/README.md | 63 ++++- docs/features/pull-requests/README.md | 112 ++++++++- 23 files changed, 1579 insertions(+), 73 deletions(-) create mode 100644 apps/desktop/src/main/services/github/githubStatusPage.test.ts create mode 100644 apps/desktop/src/main/services/github/githubStatusPage.ts create mode 100644 apps/desktop/src/shared/githubServiceHealth.test.ts create mode 100644 apps/desktop/src/shared/githubServiceHealth.ts diff --git a/apps/ade-cli/README.md b/apps/ade-cli/README.md index 3f3d9b5c7..7e1469664 100644 --- a/apps/ade-cli/README.md +++ b/apps/ade-cli/README.md @@ -664,6 +664,15 @@ stored PAT order. Writes skip the read-only GitHub App. `github.getStatus` reports the active read/write sources, per-credential failure/cooldown state, fallback details, and any background-refresh pause without exposing tokens. +When GitHub itself is failing rather than rejecting the credential, +`authFailure.kind` is `service_unavailable` (GitHub returned 5xx) — scripts must +not treat that as a reason to re-auth or rotate a token. In that case, and for +an unclassifiable failure, `github.getStatus` also consults githubstatus.com and +attaches `serviceHealth` (`indicator`, `affected` components, `incidentUrl`) +when a GitHub surface ADE depends on is confirmed down. `serviceHealth` is +present only as positive corroboration: its absence never means the failure is +local, so do not branch on it being missing. + Pi sign-in has no typed command, the same way OpenCode's `ai.opencodeOAuth*` actions do not. `ai.piLoginStart` blocks until a human finishes Pi's own browser or device-code flow, and any prompt it raises is answered by a *second* call diff --git a/apps/ade-cli/src/headlessLinearServices.test.ts b/apps/ade-cli/src/headlessLinearServices.test.ts index e64e14e87..e571908aa 100644 --- a/apps/ade-cli/src/headlessLinearServices.test.ts +++ b/apps/ade-cli/src/headlessLinearServices.test.ts @@ -36,6 +36,7 @@ vi.mock("../../desktop/src/main/services/automations/automationSecretService", ( import { EncryptedFileCredentialStore } from "./services/credentials/credentialStore"; import { createHeadlessGitHubService, createHeadlessLinearServices } from "./headlessLinearServices"; +import { resetGitHubServiceHealthCache } from "../../desktop/src/main/services/github/githubStatusPage"; import { clearGithubCredentialHealth, githubCredentialCooldown, @@ -185,6 +186,55 @@ describe("headlessLinearServices", () => { } }); + // The renderer reaches GitHub through whichever service owns the project, so + // corroboration applied only to the desktop in-process service left the whole + // outage attribution dead in the shipping, runtime-backed (brain) build. + it("corroborates a GitHub server error against the status page", async () => { + const previousAdeHome = process.env.ADE_HOME; + const previousFetch = globalThis.fetch; + process.env.ADE_HOME = fs.mkdtempSync(path.join(os.tmpdir(), "ade-headless-github-outage-")); + const statusPagePayload = { + status: { indicator: "major", description: "Partial System Outage" }, + components: [{ id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }], + incidents: [{ name: "Incident with GitHub.com", shortlink: "https://stspg.io/live", resolved_at: null }], + }; + const statusPageCalls: string[] = []; + const fetchImpl = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input instanceof Request ? input.url : input); + if (url.includes("githubstatus.com")) statusPageCalls.push(url); + if (url.includes("githubstatus.com")) { + return new Response(JSON.stringify(statusPagePayload), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + // Every api.github.com call fails the way GitHub fails during an incident. + return new Response("No server is currently available to service your request.", { status: 503 }); + }) as unknown as typeof fetch; + globalThis.fetch = fetchImpl; + const githubService = createHeadlessGitHubService( + "/tmp/ade-project", + { debug() {}, info() {}, warn() {}, error() {} } as any, + { fetchImpl }, + ); + try { + githubService.setToken("ghp_outage_token"); + const status = await githubService.getStatus({ forceRefresh: true }); + + expect(status.authFailure?.kind).toBe("service_unavailable"); + expect(status.serviceHealth?.affected.map((entry) => entry.surface)).toEqual(["api"]); + expect(status.serviceHealth?.incidentUrl).toBe("https://stspg.io/live"); + expect(statusPageCalls).toHaveLength(1); + } finally { + // Module-level cache: leaving it warm would silently change what the + // sibling status-lookup tests observe. + resetGitHubServiceHealthCache(); + globalThis.fetch = previousFetch; + if (previousAdeHome == null) delete process.env.ADE_HOME; + else process.env.ADE_HOME = previousAdeHome; + } + }); + it("does not let an invalidated GitHub status lookup overwrite the newer cache", async () => { const previousAdeHome = process.env.ADE_HOME; const previousFetch = globalThis.fetch; diff --git a/apps/ade-cli/src/headlessLinearServices.ts b/apps/ade-cli/src/headlessLinearServices.ts index 75e81d56f..c4263b2ef 100644 --- a/apps/ade-cli/src/headlessLinearServices.ts +++ b/apps/ade-cli/src/headlessLinearServices.ts @@ -17,6 +17,7 @@ import type { createLinearIssueTracker } from "../../desktop/src/main/services/c import type { createAutomationSecretService } from "../../desktop/src/main/services/automations/automationSecretService"; import type { ComputerUseArtifactBrokerService } from "../../desktop/src/main/services/computerUse/computerUseArtifactBrokerService"; import { resolveSmartLinkPreview } from "../../desktop/src/main/services/chat/smartLinkPreviewService"; +import { attachGitHubServiceHealth } from "../../desktop/src/main/services/github/githubStatusPage"; import type { SmartLinkPreview } from "../../desktop/src/shared/smartLinks"; import { getModelById, @@ -1873,26 +1874,46 @@ export function createHeadlessGitHubService( } }; + const resolveStatus = async (opts: { forceRefresh?: boolean }): Promise => { + const forceRefresh = opts.forceRefresh === true; + if (forcedStatusLookupInFlight?.generation === statusLookupGeneration) { + return await forcedStatusLookupInFlight.promise; + } + if (!forceRefresh) { + return await performStatusLookup(false, statusLookupGeneration); + } + + invalidateStatusCache(); + const generation = statusLookupGeneration; + const lookup = performStatusLookup(true, generation); + forcedStatusLookupInFlight = { generation, promise: lookup }; + try { + return await lookup; + } finally { + if (forcedStatusLookupInFlight?.promise === lookup) forcedStatusLookupInFlight = null; + } + }; + service = { verifyStoredPat, async getStatus(opts: { forceRefresh?: boolean } = {}) { - const forceRefresh = opts.forceRefresh === true; - if (forcedStatusLookupInFlight?.generation === statusLookupGeneration) { - return await forcedStatusLookupInFlight.promise; - } - if (!forceRefresh) { - return await performStatusLookup(false, statusLookupGeneration); - } - - invalidateStatusCache(); - const generation = statusLookupGeneration; - const lookup = performStatusLookup(true, generation); - forcedStatusLookupInFlight = { generation, promise: lookup }; - try { - return await lookup; - } finally { - if (forcedStatusLookupInFlight?.promise === lookup) forcedStatusLookupInFlight = null; - } + // Corroborate a failing status against githubstatus.com, exactly as the + // desktop in-process service does. The renderer reaches GitHub through + // whichever service owns the project, so applying this to only one of + // them would leave the outage attribution dead in the shipping, + // runtime-backed build. Wrapped at this single exit rather than inside + // `resolveStatus` so a cache hit still gets fresh corroboration — and so + // a future early return cannot skip it. + // `options.fetchImpl` is threaded through deliberately: it is the same + // seam every other GitHub call in this service uses, so a test that + // provokes a 5xx/unclassified failure stubs the status page instead of + // reaching the real githubstatus.com from the unit suite. Passed raw + // rather than via `requestGitHub`, which would replace the status-page + // lookup's own abort signal and defeat its timeout. + return await attachGitHubServiceHealth(await resolveStatus(opts), { + logger, + fetchImpl: options.fetchImpl, + }); }, async getBackgroundRequestPauseUntilMs() { const inventory = await readCredentialInventoryAsync(); diff --git a/apps/desktop/src/main/services/github/githubCredentialHealth.ts b/apps/desktop/src/main/services/github/githubCredentialHealth.ts index a45840416..cbb919362 100644 --- a/apps/desktop/src/main/services/github/githubCredentialHealth.ts +++ b/apps/desktop/src/main/services/github/githubCredentialHealth.ts @@ -18,6 +18,12 @@ import { const FALLBACK_COOLDOWN_MS = 5 * 60_000; const SECONDARY_RATE_LIMIT_COOLDOWN_MS = 60_000; +// A GitHub 5xx deliberately gets NO cooldown. Cooldowns here are consulted by +// every operation path (githubRawRequest, githubRequest, readAuthToken), not +// just background probes, so parking a credential after one transient 502 would +// fail the user's next merge or PR read locally, without a request, for the +// whole window. The credential is not the problem, so it must stay usable the +// instant GitHub recovers. const REPOSITORY_ACCESS_TTL_MS = 2 * 60_000; export const GITHUB_BACKGROUND_RATE_LIMIT_RESERVE = 500; diff --git a/apps/desktop/src/main/services/github/githubRateLimit.ts b/apps/desktop/src/main/services/github/githubRateLimit.ts index 232d72fbc..05310568c 100644 --- a/apps/desktop/src/main/services/github/githubRateLimit.ts +++ b/apps/desktop/src/main/services/github/githubRateLimit.ts @@ -1,4 +1,5 @@ import type { GitHubAuthFailure, GitHubRateLimitState } from "../../../shared/types"; +import { isGithubServiceUnavailable } from "../../../shared/githubServiceHealth"; export class GitHubRateLimitError extends Error { constructor( @@ -95,6 +96,19 @@ export function classifyGitHubAuthFailure(args: { }, }; } + // Ordered before the transient-network check: GitHub's 503 body contains + // "temporarily unavailable"-adjacent wording that the transient regex also + // matches, and "GitHub is down" is the more precise, more actionable answer. + if (isGithubServiceUnavailable({ status: args.status, message })) { + return { + rateLimit, + authFailure: { + kind: "service_unavailable", + message, + retryAt: null, + }, + }; + } if (isTransientGithubProbeFailure(message)) { return { rateLimit, diff --git a/apps/desktop/src/main/services/github/githubService.ts b/apps/desktop/src/main/services/github/githubService.ts index 2517fb271..9e303eace 100644 --- a/apps/desktop/src/main/services/github/githubService.ts +++ b/apps/desktop/src/main/services/github/githubService.ts @@ -46,6 +46,7 @@ import { requestGithubRawWithCredentialFallback, type GithubRawRequestArgs, } from "./githubRawRequest"; +import { attachGitHubServiceHealth } from "./githubStatusPage"; import { classifyGitHubAuthFailure, classifyGitHubGraphqlCredentialFailure, @@ -1872,7 +1873,9 @@ export function createGithubService({ if (!opts.forceRefresh) return await statusInFlight; await statusInFlight.catch(() => {}); } - const work = computeStatus(opts); + // Corroborate a failing status against githubstatus.com. Shared with the + // headless (brain) service so both getStatus owners behave identically. + const work = computeStatus(opts).then((status) => attachGitHubServiceHealth(status, { logger })); statusInFlight = work; try { return await work; diff --git a/apps/desktop/src/main/services/github/githubStatusPage.test.ts b/apps/desktop/src/main/services/github/githubStatusPage.test.ts new file mode 100644 index 000000000..4cd52c389 --- /dev/null +++ b/apps/desktop/src/main/services/github/githubStatusPage.test.ts @@ -0,0 +1,169 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + attachGitHubServiceHealth, + getGitHubServiceHealth, + resetGitHubServiceHealthCache, +} from "./githubStatusPage"; +import type { GitHubStatus } from "../../../shared/types"; + +const OUTAGE_PAYLOAD = { + status: { indicator: "major", description: "Partial System Outage" }, + components: [{ id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }], + incidents: [{ name: "Incident with GitHub.com", shortlink: "https://stspg.io/x", resolved_at: null }], +}; + +function jsonResponse(payload: unknown, ok = true): Response { + return { + ok, + status: ok ? 200 : 503, + json: async () => payload, + } as unknown as Response; +} + +describe("githubStatusPage", () => { + beforeEach(() => { + resetGitHubServiceHealthCache(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + resetGitHubServiceHealthCache(); + }); + + it("returns a corroborated incident", async () => { + vi.stubGlobal("fetch", vi.fn(async () => jsonResponse(OUTAGE_PAYLOAD))); + const health = await getGitHubServiceHealth(); + expect(health?.affected[0]?.surface).toBe("api"); + expect(health?.incidentUrl).toBe("https://stspg.io/x"); + }); + + it("caches so repeated failures don't re-hit the status page", async () => { + const fetchMock = vi.fn(async () => jsonResponse(OUTAGE_PAYLOAD)); + vi.stubGlobal("fetch", fetchMock); + await getGitHubServiceHealth(); + await getGitHubServiceHealth(); + await getGitHubServiceHealth(); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + // Without a negative cache, an outage that also breaks egress would retry the + // status page on every failed GitHub call — exactly when it helps least. + it("caches the null result too", async () => { + const fetchMock = vi.fn(async () => { + throw new Error("getaddrinfo ENOTFOUND"); + }); + vi.stubGlobal("fetch", fetchMock); + expect(await getGitHubServiceHealth()).toBeNull(); + expect(await getGitHubServiceHealth()).toBeNull(); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("coalesces concurrent lookups into one request", async () => { + const fetchMock = vi.fn(async () => jsonResponse(OUTAGE_PAYLOAD)); + vi.stubGlobal("fetch", fetchMock); + const [a, b] = await Promise.all([getGitHubServiceHealth(), getGitHubServiceHealth()]); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(a).toEqual(b); + }); + + // An unreachable or broken status page must never itself produce UI: that + // would swap one wrong accusation for another. + it("stays silent on a non-ok response or unparseable body", async () => { + vi.stubGlobal("fetch", vi.fn(async () => jsonResponse(OUTAGE_PAYLOAD, false))); + expect(await getGitHubServiceHealth()).toBeNull(); + + resetGitHubServiceHealthCache(); + vi.stubGlobal("fetch", vi.fn(async () => ({ + ok: true, + status: 200, + json: async () => { + throw new Error("Unexpected token < in JSON"); + }, + } as unknown as Response))); + expect(await getGitHubServiceHealth()).toBeNull(); + }); + + it("never reports an outage when nothing ADE uses is degraded", async () => { + vi.stubGlobal("fetch", vi.fn(async () => jsonResponse({ + status: { indicator: "major", description: "Partial System Outage" }, + components: [{ id: "pjmpxvq2cmr2", name: "Copilot", status: "major_outage" }], + }))); + expect(await getGitHubServiceHealth()).toBeNull(); + }); +}); + +function status(overrides: Partial = {}): GitHubStatus { + return { + tokenStored: true, + patTokenStored: false, + tokenDecryptionFailed: false, + storageScope: "app", + authSource: "gh", + tokenType: "oauth", + repo: { owner: "arul28", name: "ADE" }, + hasOrigin: true, + userLogin: null, + scopes: [], + ghCliPath: null, + ghAuthError: null, + checkedAt: null, + repoAccessOk: null, + repoAccessError: null, + connected: false, + ...overrides, + }; +} + +describe("attachGitHubServiceHealth", () => { + beforeEach(() => resetGitHubServiceHealthCache()); + afterEach(() => { + vi.restoreAllMocks(); + resetGitHubServiceHealthCache(); + }); + + it("attaches a corroborated incident to a GitHub-side failure", async () => { + vi.stubGlobal("fetch", vi.fn(async () => jsonResponse(OUTAGE_PAYLOAD))); + const result = await attachGitHubServiceHealth( + status({ + authFailure: { kind: "service_unavailable", message: "503", retryAt: null }, + }), + ); + expect(result.serviceHealth?.affected[0]?.surface).toBe("api"); + }); + + // Definitive answers FROM GitHub about this credential. Letting a mild, + // unrelated degradation overwrite them would tell a rate-limited or + // under-scoped user there is nothing to fix, hiding their real remedy. + it("never overrides a definitive GitHub answer, and never asks the status page about one", async () => { + const fetchMock = vi.fn(async () => jsonResponse(OUTAGE_PAYLOAD)); + vi.stubGlobal("fetch", fetchMock); + // `network` is ADE's own connectivity failing — the status page is just as + // unreachable, so asking only adds latency to every offline blip. + for (const kind of ["invalid_token", "permission_denied", "rate_limited", "network"] as const) { + const result = await attachGitHubServiceHealth( + status({ authFailure: { kind, message: "x", retryAt: null } }), + ); + expect(result.serviceHealth).toBeFalsy(); + } + expect(fetchMock).not.toHaveBeenCalled(); + }); + + // A fine-grained PAT that simply lacks this repo is a LOCAL misconfiguration: + // connected === false, but GitHub never failed. Treating that as a trigger + // would make this module a once-a-minute beacon for the whole session. + it("does not consult the status page when GitHub reported no failure", async () => { + const fetchMock = vi.fn(async () => jsonResponse(OUTAGE_PAYLOAD)); + vi.stubGlobal("fetch", fetchMock); + const result = await attachGitHubServiceHealth(status({ connected: false, authFailure: null })); + expect(result.serviceHealth).toBeFalsy(); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("stays silent when the status page is unreachable", async () => { + vi.stubGlobal("fetch", vi.fn(async () => { throw new Error("ENOTFOUND"); })); + const result = await attachGitHubServiceHealth( + status({ authFailure: { kind: "service_unavailable", message: "503", retryAt: null } }), + ); + expect(result.serviceHealth).toBeFalsy(); + }); +}); diff --git a/apps/desktop/src/main/services/github/githubStatusPage.ts b/apps/desktop/src/main/services/github/githubStatusPage.ts new file mode 100644 index 000000000..71c2c024b --- /dev/null +++ b/apps/desktop/src/main/services/github/githubStatusPage.ts @@ -0,0 +1,181 @@ +import { + GITHUB_STATUS_SUMMARY_URL, + deriveGitHubServiceHealth, + type GitHubServiceHealth, +} from "../../../shared/githubServiceHealth"; +import type { GitHubStatus } from "../../../shared/types"; +import type { Logger } from "../logging/logger"; + +/** + * Reads githubstatus.com to corroborate a GitHub failure ADE already observed. + * + * Deliberately NOT a poller. ADE is local-first: while GitHub works, this module + * makes zero network calls and no third party learns the app is running. The + * only trigger is an actual GitHub-reported failure, which is also when the + * answer is worth anything. + * + * Every failure mode here resolves to null ("say nothing"). An unreachable or + * malformed status page must never itself become a banner — that would replace + * one wrong accusation with another. + */ + +/** + * Results — including null ones — are cached for this window. Caching the + * negative matters most: an outage that also breaks egress would otherwise + * retry the status page on every failed GitHub call, exactly when the network + * is least able to absorb it. + */ +const CACHE_TTL_MS = 60_000; +/** + * Hard ceiling on the lookup. `getStatus` awaits this before resolving, so it + * is also the worst-case stall added to a GitHub status read during an + * incident — kept short because the answer is a nicety, not a requirement. + */ +const REQUEST_TIMEOUT_MS = 2_000; + +type CacheEntry = { + health: GitHubServiceHealth | null; + expiresAtMs: number; +}; + +let cache: CacheEntry | null = null; +let inFlight: Promise | null = null; +/** + * Bumped by {@link resetGitHubServiceHealthCache}. A lookup that was already in + * flight when the cache was reset must not write its now-stale result back — + * without this, a reset is silently undone a moment later. + */ +let cacheGeneration = 0; + +/** + * Injectable so a test that produces a corroborated failure cannot reach the + * real githubstatus.com from the unit suite. Both GitHub services already carry + * a `fetchImpl` for the same reason. + */ +export type StatusPageFetch = typeof fetch; + +async function fetchServiceHealth( + logger?: Logger, + fetchImpl: StatusPageFetch = fetch, +): Promise { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS); + try { + const response = await fetchImpl(GITHUB_STATUS_SUMMARY_URL, { + method: "GET", + headers: { accept: "application/json", "user-agent": "ade-desktop" }, + signal: controller.signal, + // The status page is public; sending credentials would be a privacy leak + // and Statuspage's CDN ignores them anyway. + credentials: "omit", + }); + if (!response.ok) return null; + const payload = (await response.json()) as unknown; + const health = deriveGitHubServiceHealth(payload); + if (health) { + logger?.info("github.service_incident_detected", { + indicator: health.indicator, + affected: health.affected.map((entry) => entry.surface), + }); + } + return health; + } catch (error) { + // Includes the abort path. Never surfaced: a status page we cannot reach + // tells us nothing about GitHub, so ADE keeps its existing (credential- + // shaped) error rather than inventing an outage. + logger?.debug("github.service_status_unreachable", { + error: error instanceof Error ? error.message : String(error), + }); + return null; + } finally { + clearTimeout(timer); + } +} + +/** + * Corroborated GitHub incident affecting ADE, or null when GitHub reports + * nothing relevant (or could not be reached). + * + * Call this ONLY after a GitHub request has already failed. + */ +export async function getGitHubServiceHealth( + options: { forceRefresh?: boolean; logger?: Logger; fetchImpl?: StatusPageFetch } = {}, +): Promise { + if (!options.forceRefresh) { + const now = Date.now(); + if (cache && cache.expiresAtMs > now) return cache.health; + // Only ordinary callers join an in-flight lookup. A forced refresh that + // joined would silently receive the pre-incident answer it asked to bypass. + if (inFlight) return await inFlight; + } + + const generation = cacheGeneration; + const work = (async () => { + const health = await fetchServiceHealth(options.logger, options.fetchImpl); + if (generation === cacheGeneration) { + cache = { health, expiresAtMs: Date.now() + CACHE_TTL_MS }; + } + return health; + })(); + if (!options.forceRefresh) inFlight = work; + try { + return await work; + } finally { + if (inFlight === work) inFlight = null; + } +} + +export function resetGitHubServiceHealthCache(): void { + cache = null; + inFlight = null; + cacheGeneration += 1; +} + +/** + * The only failure kinds worth corroborating — an allowlist, so a future + * failure kind has to opt in rather than silently inherit an outbound request. + * + * - `service_unavailable`: GitHub returned 5xx. Exactly the case that used to + * render as "authentication check failed". + * - `unknown`: GitHub replied with something we could not classify, which is + * the other way an incident reaches the user as a credential accusation. + * + * Everything else is deliberately excluded. `invalid_token`, + * `permission_denied`, and `rate_limited` are definitive answers FROM GitHub + * about this credential — letting a mild, unrelated degradation ("Pages: + * degraded_performance") overwrite them would tell a rate-limited or + * under-scoped user there is nothing to fix, hiding their actual remedy. + * `network` is ADE's own connectivity failing; the status page is just as + * unreachable then, and asking would add latency to every offline blip. + */ +const CORROBORATED_FAILURE_KINDS: ReadonlySet["kind"]> = + new Set(["service_unavailable", "unknown"] as const); + +/** + * Attach githubstatus.com corroboration to a status that reports a failure. + * + * Shared by BOTH `getStatus` implementations — the desktop in-process service + * and the headless (brain) one. The renderer reaches GitHub through whichever + * of those owns the project, so a wrapper applied to only one leaves the + * feature inert in the shipping runtime-backed build. + * + * Corroboration is advisory in one direction only: a confirmed incident lets + * the UI stop blaming the user's credential, while a silent status page changes + * nothing (it lags real incidents, so it cannot prove the failure is local). + */ +export async function attachGitHubServiceHealth( + status: GitHubStatus, + options: { logger?: Logger; fetchImpl?: StatusPageFetch } = {}, +): Promise { + const failure = status.authFailure; + // Gated on an actual GitHub-reported failure. `connected === false` alone is + // not enough: a fine-grained PAT that simply lacks access to this repo is a + // purely local misconfiguration, and treating it as a trigger would turn this + // module into a once-a-minute beacon for the life of the session. + if (!failure || !CORROBORATED_FAILURE_KINDS.has(failure.kind)) return status; + const serviceHealth = await getGitHubServiceHealth({ + logger: options.logger, + fetchImpl: options.fetchImpl, + }).catch(() => null); + return serviceHealth ? { ...status, serviceHealth } : status; +} diff --git a/apps/desktop/src/main/services/prs/prService.test.ts b/apps/desktop/src/main/services/prs/prService.test.ts index 93e54a78c..f2800a034 100644 --- a/apps/desktop/src/main/services/prs/prService.test.ts +++ b/apps/desktop/src/main/services/prs/prService.test.ts @@ -1671,6 +1671,56 @@ describe("prService.getGithubSnapshot", () => { expect(githubService.apiRequest).not.toHaveBeenCalled(); }); + // A GitHub 5xx used to fall through to "GitHub auth is invalid or missing + // required access. Update it in Settings" — telling the user to replace a + // credential that was never broken, on the surface they act from most. + it("blames GitHub, not the credential, when GitHub returns a server error", async () => { + const githubService = makeGithubService({ + getStatus: vi.fn(async () => makeGithubStatus({ + connected: false, + authFailure: { + kind: "service_unavailable", + message: "No server is currently available to service your request.", + retryAt: null, + }, + })), + apiRequest: vi.fn(async () => ({ data: [] })), + }); + const { service } = buildService({ githubService, laneService: makeLaneService([]) }); + + const error = await service.getGithubSnapshot().then( + () => { throw new Error("expected getGithubSnapshot to reject"); }, + (reason: unknown) => reason as Error, + ); + expect(error.message).toContain("This isn't a problem with your GitHub connection"); + // The pre-fix string, which sent the user to replace a working credential. + expect(error.message).not.toContain("Update it in Settings"); + expect(githubService.apiRequest).not.toHaveBeenCalled(); + }); + + it("names the corroborated outage when GitHub's status page confirms one", async () => { + const githubService = makeGithubService({ + getStatus: vi.fn(async () => makeGithubStatus({ + connected: false, + authFailure: { + kind: "service_unavailable", + message: "No server is currently available to service your request.", + retryAt: null, + }, + serviceHealth: { + indicator: "major", + affected: [{ surface: "api", name: "API Requests", status: "major_outage" }], + incidentUrl: "https://stspg.io/live", + }, + })), + apiRequest: vi.fn(async () => ({ data: [] })), + }); + const { service } = buildService({ githubService, laneService: makeLaneService([]) }); + + await expect(service.getGithubSnapshot()).rejects.toThrow(/GitHub is having problems/); + expect(githubService.apiRequest).not.toHaveBeenCalled(); + }); + it("does not return an in-memory GitHub snapshot when token status is invalid", async () => { const githubService = makeGithubService({ getStatus: vi.fn() diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 1374ae34f..8347e57ce 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -9208,6 +9208,15 @@ export function createPrService({ : " Try again after GitHub resets the API limit."; return `GitHub API rate limit reached.${retry}`; } + // GitHub itself failed, so nothing about the credential is in question. + // Without this arm a 503 falls through to the "auth is invalid — update it + // in Settings" default, which is the exact accusation the outage work + // exists to remove, on the surface a user is most likely to act from. + if (githubStatus.authFailure?.kind === "service_unavailable") { + return githubStatus.serviceHealth + ? "GitHub is having problems, so pull requests can't sync right now. Nothing to change here — ADE will catch up when GitHub recovers." + : "GitHub returned an error, so pull requests can't sync right now. This isn't a problem with your GitHub connection — ADE will keep retrying."; + } if (githubStatus.repo && githubStatus.repoAccessError) { return `GitHub auth cannot access ${githubStatus.repo.owner}/${githubStatus.repo.name}: ${githubStatus.repoAccessError}. Update it in Settings to sync pull requests.`; } diff --git a/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx b/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx index 90ab2a4b6..d9e9de32e 100644 --- a/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx +++ b/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx @@ -9,6 +9,37 @@ import type { SyncRouteHealth, } from "../../../shared/types"; import { IntegrationBannerHost, type IntegrationBannerHostProps } from "./IntegrationBannerHost"; +import { deriveGitHubServiceHealth } from "../../../shared/githubServiceHealth"; + +// Built through the real parser rather than hand-written, so the fixture stays +// honest against deriveGitHubServiceHealth's own rules. +const OUTAGE_HEALTH = deriveGitHubServiceHealth({ + status: { indicator: "major", description: "Partial System Outage" }, + components: [ + { id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }, + { id: "hhtssxt0f5v2", name: "Pull Requests", status: "major_outage" }, + ], + incidents: [{ name: "Incident with GitHub.com", shortlink: "https://stspg.io/live", resolved_at: null }], +})!; + +/** + * The real broken state from the reported bug: a stored gh credential that + * GitHub refused to validate because GitHub itself was returning 503. + */ +function makeOutageStatus(overrides: Partial = {}): GitHubStatus { + return makeGithubStatus({ + tokenStored: true, + authSource: "gh", + tokenType: "oauth", + writeAuthSource: "none", + authFailure: { + kind: "service_unavailable", + message: "No server is currently available to service your request.", + retryAt: null, + }, + ...overrides, + }); +} function makeInstall(overrides: Partial = {}): GitHubAppInstallationStatus { return { @@ -325,4 +356,116 @@ describe("IntegrationBannerHost relay-offline banner", () => { expect(screen.getByText("Another ADE process owns this machine's relay connection")).toBeTruthy(); }); + + // The bug this feature exists for: GitHub's own 503 rendered as "GitHub + // authentication check failed", blaming the user's credential and offering a + // reconnect that risks replacing a token that was never broken. + it("replaces every GitHub credential complaint with one neutral outage notice", async () => { + setAdeMock({ + getAppInstallationStatus: vi.fn(async () => makeInstall()), + getAppUserAuthStatus: vi.fn(async () => makeAuth({ tokenStored: false })), + onStatusChanged: vi.fn(() => () => {}), + }); + + await act(async () => { + render( + , + ); + }); + await act(async () => {}); + + expect(screen.getByText("GitHub is down")).toBeTruthy(); + expect(screen.getByText(/GitHub reports problems with API Requests and Pull Requests/)).toBeTruthy(); + expect(screen.getByText(/isn't your setup/)).toBeTruthy(); + + // The accusations, and the App-not-authorized sibling, are all gone. + expect(screen.queryByText(/authentication check failed/i)).toBeNull(); + expect(screen.queryByText("GitHub CLI or token not connected")).toBeNull(); + expect(screen.queryByText("GitHub App not authorized")).toBeNull(); + expect(screen.getAllByRole("status")).toHaveLength(1); + + // No CTA that would push the user to replace a working credential. Scoped + // to buttons: the body copy legitimately says ADE will "reconnect on its + // own", which is the opposite of asking them to act. + const labels = [...document.querySelectorAll("button")].map((node) => node.textContent ?? ""); + expect(labels.some((label) => /reconnect|connect github|fix github|set up/i.test(label))).toBe(false); + }); + + it("sends the outage action to the live incident, not ADE settings", async () => { + const openExternal = vi.fn(async () => {}); + Object.defineProperty(window, "ade", { + configurable: true, + value: { + github: { onStatusChanged: vi.fn(() => () => {}) }, + prs: { onEvent: vi.fn(() => () => {}) }, + app: { openExternal }, + }, + }); + const navigate = vi.fn(); + + await act(async () => { + render( + , + ); + }); + await act(async () => { + screen.getByRole("button", { name: /github status/i }).click(); + }); + + expect(openExternal).toHaveBeenCalledWith("https://stspg.io/live"); + expect(navigate).not.toHaveBeenCalled(); + }); + + // The notice is `info`, which severity-sorts LAST. With enough competing + // banners the two-slot cap would push it into overflow while it is still + // suppressing the GitHub banners — the complaints would vanish with their + // explanation hidden behind a toggle. + it("keeps the outage notice visible when higher-severity banners compete", async () => { + setAdeMock({ + getAppInstallationStatus: vi.fn(async () => makeInstall()), + getAppUserAuthStatus: vi.fn(async () => makeAuth()), + onStatusChanged: vi.fn(() => () => {}), + }); + + await act(async () => { + render( + , + ); + }); + await act(async () => {}); + + // Something must overflow — but not this. + expect(screen.getByText(/more integration issue/)).toBeTruthy(); + expect(screen.getAllByRole("status")[0]?.textContent).toContain("GitHub is down"); + // Unrelated banners are NOT suppressed by a GitHub outage. + expect(screen.getByText("No AI provider configured")).toBeTruthy(); + }); + + // Without corroboration ADE keeps its existing copy: the status page lags + // real incidents, so silence is not evidence the user is at fault. + it("falls back to the credential banner when no outage is corroborated", async () => { + setAdeMock({ onStatusChanged: vi.fn(() => () => {}) }); + + await act(async () => { + render(); + }); + await act(async () => {}); + + expect(screen.getByText("GitHub isn't responding")).toBeTruthy(); + expect(screen.queryByText("GitHub is down")).toBeNull(); + }); }); diff --git a/apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx b/apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx index 6e6c39226..b1afb51fe 100644 --- a/apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx +++ b/apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx @@ -15,6 +15,7 @@ import { deriveGithubRealtimeBlock, deriveGithubRepoConnectionState, describeGithubCliBanner, + describeGithubOutage, githubStatusHasWriteCredential, githubAccountIssueCopy, githubRepoIssueCopy, @@ -264,6 +265,7 @@ export function IntegrationBannerHost({ clearDismissal(`mock-provider:${currentProjectRoot}`); } if (relayOutage == null) clearDismissal("relay-offline"); + if (!describeGithubOutage(githubStatus)) clearDismissal("github-outage"); }, [ currentProjectRoot, relayOutage, @@ -281,6 +283,39 @@ export function IntegrationBannerHost({ const models = useMemo(() => { const list: BannerModel[] = []; + // 0) GitHub outage (NEW). When GitHub's own status page confirms an + // incident on a surface ADE uses, every GitHub banner below is a symptom of + // the SAME cause and would read as three separate accusations against the + // user's setup. Collapse them into one honest notice and stop offering + // fixes that cannot work — re-authorizing during a GitHub outage is how a + // user destroys a credential that was never broken. + const outage = describeGithubOutage(githubStatus); + if (currentProjectRoot && outage) { + list.push({ + id: "github-outage", + // Informational: nothing here is the user's to fix, and it clears on + // its own. An error/warning tone would imply an action they don't have. + severity: "info", + title: outage.title, + detail: outage.detail, + actions: [ + { + label: outage.action, + variant: "primary", + onClick: () => openExternalUrl(outage.actionUrl), + }, + ], + // Outages are machine-wide, not per-project. Fingerprinted on the + // affected surfaces so a widening incident resurfaces a dismissed banner. + dismiss: { key: "github-outage", fingerprint: outage.fingerprint }, + }); + } + // Suppresses ONLY the GitHub-derived banners below. The AI-provider, mock- + // provider, and relay banners have nothing to do with GitHub and stay. + // Gated on the same condition that renders the outage banner, so the GitHub + // family can never be silenced without its replacement being shown. + const githubSuppressed = currentProjectRoot != null && outage != null; + // 1) GitHub App real-time block (NEW). Only once a real read has landed FOR // the current project (loadedRoot === currentProjectRoot), so an unloaded/ // absent API never masquerades as "not authorized" and a project switch @@ -297,7 +332,7 @@ export function IntegrationBannerHost({ && typeof rawInstall.appName === "string" && typeof rawInstall.relayConfigured === "boolean" && (!rawAuth || typeof rawAuth.configured === "boolean"); - if (appStatusLoaded && currentProjectRoot && loadedRoot === currentProjectRoot && githubAppStatusSupported) { + if (!githubSuppressed && appStatusLoaded && currentProjectRoot && loadedRoot === currentProjectRoot && githubAppStatusSupported) { const account = deriveGithubAccountAuthState(appAuth); const repo = deriveGithubRepoConnectionState(appInstall); const block = deriveGithubRealtimeBlock(account, repo); @@ -361,7 +396,8 @@ export function IntegrationBannerHost({ // 2) gh CLI / PAT not connected (MIGRATED). A DISTINCT concern from the App // block: this is the token ADE uses for git & PR operations, not webhooks. if ( - currentProjectRoot + !githubSuppressed + && currentProjectRoot && githubStatus && (!githubStatus.connected || !githubStatusHasWriteCredential(githubStatus)) ) { @@ -464,7 +500,16 @@ export function IntegrationBannerHost({ return models .map((model, index) => ({ model, index })) .filter(({ model }) => !(model.dismiss && dismissals.isDismissed(model.dismiss.key, model.dismiss.fingerprint))) - .sort((a, b) => SEVERITY_RANK[a.model.severity] - SEVERITY_RANK[b.model.severity] || a.index - b.index) + .sort((a, b) => { + // The outage notice is deliberately `info` (nothing here is the user's + // to fix), but severity ordering would then rank it last and the + // MAX_VISIBLE_BANNERS slice could push it into overflow — while it is + // still suppressing the GitHub banners. The GitHub complaints would + // vanish with their explanation hidden behind a toggle. Pin it first. + const pinned = Number(b.model.id === "github-outage") - Number(a.model.id === "github-outage"); + if (pinned !== 0) return pinned; + return SEVERITY_RANK[a.model.severity] - SEVERITY_RANK[b.model.severity] || a.index - b.index; + }) .map(({ model }) => model); }, [models, dismissals]); diff --git a/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.tsx b/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.tsx index b5cc515a7..95ade73a8 100644 --- a/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.tsx +++ b/apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.tsx @@ -17,6 +17,7 @@ import { isGithubRealtimeHealthy, isGithubRepoAccessPending, } from "../../lib/githubIntegrationStatus"; +import { isGithubServiceUnavailable } from "../../../shared/githubServiceHealth"; const ADE_GITHUB_APP_NAME = "ADE"; const ADE_GITHUB_APP_INSTALL_URL = "https://github.com/apps/ade-for-github/installations/new"; @@ -490,6 +491,14 @@ function repoView( subtext: "GitHub temporarily paused automatic App checks. Wait for the cooldown, then recheck.", }; } + // GitHub answered with a server error, so the install state is unknown + // rather than broken. Say so instead of implying ADE's setup failed. + if (isGithubServiceUnavailable({ message: error })) { + return { + pill: { tone: "neutral", color: COLORS.textMuted, label: "Waiting on GitHub" }, + subtext: "GitHub returned a server error, so ADE couldn't check this repo. Nothing to fix here — it will recheck once GitHub recovers.", + }; + } return { pill: { tone: "neutral", color: COLORS.textMuted, label: "Couldn't verify" }, subtext: error?.trim() diff --git a/apps/desktop/src/renderer/components/settings/GitHubSection.tsx b/apps/desktop/src/renderer/components/settings/GitHubSection.tsx index 15d60a52f..53c6eb5ea 100644 --- a/apps/desktop/src/renderer/components/settings/GitHubSection.tsx +++ b/apps/desktop/src/renderer/components/settings/GitHubSection.tsx @@ -22,6 +22,7 @@ import { describeGithubPatVerification, describeGithubAuthFailure, githubCredentialPresentation, + describeGithubOutage, } from "../../lib/githubIntegrationStatus"; type TokenType = "classic" | "fine-grained" | "unknown"; @@ -89,24 +90,35 @@ function shortRetryTime(value: string | null | undefined): string | null { return parsed.toLocaleTimeString([], { hour: "numeric", minute: "2-digit" }); } -function credentialStateLabel(state: GitHubCredentialState): string { - if (state.activeFor.length === 2) return "Reads & writes"; - if (state.activeFor[0] === "read") return "Reads"; - if (state.activeFor[0] === "write") return "Writes"; +/** + * One badge per credential row. Label and color are derived together because + * they switch on the same states — deriving them apart let the cooldown branch + * drift between the two. + */ +function credentialStateBadge( + state: GitHubCredentialState, + outage: boolean, +): { label: string; color: string } { + if (state.activeFor.length === 2) return { label: "Reads & writes", color: COLORS.success }; + if (state.activeFor[0] === "read") return { label: "Reads", color: COLORS.success }; + if (state.activeFor[0] === "write") return { label: "Writes", color: COLORS.success }; if (state.state === "cooldown") { + // During a GitHub outage a cooldown says nothing about the credential — + // it only records that GitHub failed to answer. "Reconnect needed" here + // would be an outright false accusation. + if (outage) return { label: "Waiting on GitHub", color: COLORS.textMuted }; const retryAt = shortRetryTime(state.failure?.retryAt); - if (state.failure?.kind === "rate_limited") return retryAt ? `Paused until ${retryAt}` : "Paused"; - if (state.failure?.kind === "invalid_token") return "Reconnect needed"; - if (state.failure?.kind === "permission_denied") return "Access unavailable"; - return "Temporarily unavailable"; + if (state.failure?.kind === "rate_limited") { + return { label: retryAt ? `Paused until ${retryAt}` : "Paused", color: COLORS.warning }; + } + if (state.failure?.kind === "invalid_token") return { label: "Reconnect needed", color: COLORS.warning }; + if (state.failure?.kind === "permission_denied") return { label: "Access unavailable", color: COLORS.warning }; + return { label: "Temporarily unavailable", color: COLORS.warning }; } - return state.available ? "Fallback" : "Not set up"; -} - -function credentialStateColor(state: GitHubCredentialState): string { - if (state.state === "active") return COLORS.success; - if (state.state === "cooldown") return COLORS.warning; - return state.available ? COLORS.textSecondary : COLORS.textDim; + return { + label: state.available ? "Fallback" : "Not set up", + color: state.available ? COLORS.textSecondary : COLORS.textDim, + }; } export function GitHubSection({ embedded = false }: { embedded?: boolean }) { @@ -215,9 +227,18 @@ export function GitHubSection({ embedded = false }: { embedded?: boolean }) { let readsWithLabel = authSourceLabel(githubStatus); if (authFailure?.kind === "rate_limited") readsWithLabel = "Paused"; if (activeReadCredential) readsWithLabel = credentialSourceLabel(activeReadCredential.source); + // A corroborated GitHub outage explains every red state on this card. While + // one is active the card stops reading as "your setup is broken": the chip + // goes neutral, the ladder's failure badges are held back, and the gh-auth + // instructions are hidden so nobody re-runs `gh auth login` and replaces a + // credential that was working fine. + const outage = describeGithubOutage(githubStatus); let statusColor: string; let statusLabel: string; - if (isConnected && credentialFallback) { + if (outage) { + statusColor = COLORS.textMuted; + statusLabel = outage.statusLabel; + } else if (isConnected && credentialFallback) { statusColor = COLORS.warning; statusLabel = "Connected · fallback"; } else if (isConnected) { @@ -245,9 +266,11 @@ export function GitHubSection({ embedded = false }: { embedded?: boolean }) { && !isConnected && githubStatus.authSource !== "pat" && ( + // A missing token is a local fact that an outage cannot explain away, so + // that one instruction still stands. The other two are inferred from + // GitHub's answers and are unreliable while GitHub is failing. !githubStatus.tokenStored - || authFailure?.kind === "invalid_token" - || hasMissingScopes + || (!outage && (authFailure?.kind === "invalid_token" || hasMissingScopes)) ); const classicTokenUrl = transcriptGistsEnabled ? GITHUB_CLASSIC_TOKEN_WITH_GIST_NEW_URL : GITHUB_CLASSIC_TOKEN_NEW_URL; const openExternal = (url: string) => { @@ -391,8 +414,19 @@ export function GitHubSection({ embedded = false }: { embedded?: boolean }) { > {summaryCell("USER", githubStatus?.userLogin ?? null)} {summaryCell("REPOSITORY", githubStatus?.repo ? `${githubStatus.repo.owner}/${githubStatus.repo.name}` : null)} - {summaryCell("READS WITH", readsWithLabel)} - {summaryCell("WRITES WITH", credentialSourceLabel(effectiveWriteAuthSource))} + {/* While GitHub is down ADE can't resolve which credential would + win, so it reports the honest "Unknown" rather than the + false-negative "Not connected". */} + {summaryCell( + "READS WITH", + outage && !activeReadCredential ? "Unknown" : readsWithLabel, + )} + {summaryCell( + "WRITES WITH", + outage && effectiveWriteAuthSource === "none" + ? "Unknown" + : credentialSourceLabel(effectiveWriteAuthSource), + )} {credentialFallback ? ( @@ -423,7 +457,7 @@ export function GitHubSection({ embedded = false }: { embedded?: boolean }) {
CONNECTION ORDER
{credentialStates.map((credential, index) => { - const stateColor = credentialStateColor(credential); + const badge = credentialStateBadge(credential, outage != null); return (
- {credentialStateLabel(credential)} + {badge.label}
); })} @@ -462,14 +496,30 @@ export function GitHubSection({ embedded = false }: { embedded?: boolean }) { {permissionMode === "auth-failure" ? (
{authFailurePresentation?.title}
{authFailurePresentation?.settingsDetail}
+ {outage ? ( + + ) : null}
) : permissionMode === "app" ? (
diff --git a/apps/desktop/src/renderer/lib/githubIntegrationStatus.test.ts b/apps/desktop/src/renderer/lib/githubIntegrationStatus.test.ts index 67cf08566..783f0369e 100644 --- a/apps/desktop/src/renderer/lib/githubIntegrationStatus.test.ts +++ b/apps/desktop/src/renderer/lib/githubIntegrationStatus.test.ts @@ -4,12 +4,24 @@ import { deriveGithubRepoConnectionState, describeGithubAuthFailure, describeGithubCliBanner, + describeGithubOutage, describeGithubPatVerification, githubCredentialPresentation, githubStatusHasUsablePat, isGithubRateLimitMessage, isGithubRepoAccessPending, } from "./githubIntegrationStatus"; +import { deriveGitHubServiceHealth } from "../../shared/githubServiceHealth"; + +function outageHealth(components = [{ id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }]) { + return deriveGitHubServiceHealth( + { + status: { indicator: "major", description: "Partial System Outage" }, + components, + incidents: [{ name: "Incident with GitHub.com", shortlink: "https://stspg.io/x", resolved_at: null }], + }, + )!; +} function makeStatus(overrides: Partial = {}): GitHubAppInstallationStatus { return { @@ -296,3 +308,87 @@ describe("isGithubRateLimitMessage", () => { expect(isGithubRateLimitMessage(null)).toBe(false); }); }); + +describe("GitHub outage attribution", () => { + // The bug this whole feature exists for: a GitHub 503 used to fall through to + // the generic "unknown" branch and render as "GitHub authentication check + // failed", blaming the user's credential for GitHub's incident. + const githubOutage503 = { + kind: "service_unavailable" as const, + message: "No server is currently available to service your request.", + retryAt: null, + }; + + it("never blames the credential for a GitHub 5xx, even with no status page corroboration", () => { + const failure = describeGithubAuthFailure(makeCliStatus({ authFailure: githubOutage503 })); + expect(failure?.title).toBe("GitHub isn't responding"); + expect(failure?.subState).toBe("service-unavailable"); + expect(failure?.settingsDetail).toContain("not a problem with your credential"); + expect(failure?.action).not.toMatch(/reconnect/i); + }); + + it("attributes a corroborated outage to GitHub and drops the reconnect CTA", () => { + const status = makeCliStatus({ + authFailure: githubOutage503, + serviceHealth: outageHealth(), + }); + const failure = describeGithubAuthFailure(status); + expect(failure?.statusLabel).toBe("GitHub outage"); + expect(failure?.title).toBe("GitHub is down"); + expect(failure?.detail).toContain("API Requests"); + expect(failure?.detail).toContain("isn't your setup"); + expect(failure?.action).toBe("GitHub status"); + }); + + // Outage attribution has to beat every credential-shaped reading of the same + // failure, including the ones that fire before the authFailure branch. + it("overrides the missing-write-credential banner during an outage", () => { + const banner = describeGithubCliBanner(makeCliStatus({ + connected: true, + writeAuthSource: "none", + serviceHealth: outageHealth(), + })); + expect(banner.title).toBe("GitHub is down"); + expect(banner.subState).toBe("outage:major"); + }); + + // A stored token is a local fact; an outage cannot explain it away, so the + // genuine "connect GitHub" instruction must survive. + it("still asks an unconnected user to connect GitHub during an outage", () => { + const banner = describeGithubCliBanner(makeCliStatus({ + tokenStored: false, + serviceHealth: outageHealth(), + })); + expect(banner.subState).toBe("no-token"); + }); + + it("says nothing about GitHub health when no incident is corroborated", () => { + expect(describeGithubOutage(makeCliStatus())).toBeNull(); + expect(describeGithubOutage(makeCliStatus({ serviceHealth: null }))).toBeNull(); + expect(describeGithubOutage(null)).toBeNull(); + }); + + it("points the action at the live incident, falling back to the status page", () => { + expect(describeGithubOutage(makeCliStatus({ serviceHealth: outageHealth() }))?.actionUrl) + .toBe("https://stspg.io/x"); + const noIncident = deriveGitHubServiceHealth( + { + status: { indicator: "major", description: "Partial System Outage" }, + components: [{ id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }], + incidents: [], + }, + )!; + expect(describeGithubOutage(makeCliStatus({ serviceHealth: noIncident }))?.actionUrl) + .toBe("https://www.githubstatus.com"); + }); + + it("softens the wording when GitHub reports degradation rather than an outage", () => { + const failure = describeGithubAuthFailure(makeCliStatus({ + authFailure: githubOutage503, + serviceHealth: outageHealth([ + { id: "8l4ygp009s5s", name: "Git Operations", status: "degraded_performance" }, + ]), + })); + expect(failure?.title).toBe("GitHub is having problems"); + }); +}); diff --git a/apps/desktop/src/renderer/lib/githubIntegrationStatus.ts b/apps/desktop/src/renderer/lib/githubIntegrationStatus.ts index ed1da4b2c..9c7719cee 100644 --- a/apps/desktop/src/renderer/lib/githubIntegrationStatus.ts +++ b/apps/desktop/src/renderer/lib/githubIntegrationStatus.ts @@ -4,6 +4,11 @@ import type { GitHubSetTokenResult, GitHubStatus, } from "../../shared/types"; +import { + GITHUB_STATUS_PAGE_URL, + githubServiceAffectedLabel, + type GitHubServiceHealth, +} from "../../shared/githubServiceHealth"; export type GithubCredentialPresentation = { tokenTypeLabel: string; @@ -13,6 +18,70 @@ export type GithubCredentialPresentation = { repoAccessLabel: string; }; +/** Copy for any GitHub failure Settings and the banner both render. */ +export type GithubFailurePresentation = { + subState: string; + statusLabel: string; + title: string; + detail: string; + settingsDetail: string; + action: string; +}; + +/** + * The outage case, which additionally has somewhere real to send the user. + * Declared as an extension so the two shapes cannot silently drift apart. + */ +export type GithubOutagePresentation = GithubFailurePresentation & { + /** Where the action button goes — the live incident when GitHub named one. */ + actionUrl: string; + /** Banner dismissal fingerprint; a changed incident resurfaces the banner. */ + fingerprint: string; +}; + +/** + * The single outage notice shown in place of every GitHub credential complaint, + * or null when no incident is corroborated. + * + * One function rather than a family of predicates because every caller needs + * the same three things together (is there an outage, what do we say, where + * does the button go) — splitting them just duplicated the `?? statusPage` + * fallback and the fingerprint at each call site. + * + * Every GitHub-blaming surface gates on this: banners collapse to one outage + * notice, Settings drops its red "check failed" framing, and reconnect CTAs + * disappear (reconnecting cannot fix an outage, and re-running `gh auth login` + * during one risks replacing a working credential with a broken one). + * + * Deliberately one-directional. When this returns null ADE says nothing about + * GitHub's health and keeps its existing error copy — the status page trails + * real incidents by 10-20 minutes, so "no incident reported" is not evidence + * that the user's setup is at fault. + */ +export function describeGithubOutage( + status: GitHubStatus | null | undefined, +): GithubOutagePresentation | null { + const health: GitHubServiceHealth | null = status?.serviceHealth ?? null; + if (!health) return null; + const affected = githubServiceAffectedLabel(health); + const severe = health.affected.some((entry) => entry.status === "major_outage"); + return { + subState: `outage:${health.indicator}`, + statusLabel: "GitHub outage", + title: severe ? "GitHub is down" : "GitHub is having problems", + // Names the affected parts (so the user knows which of their work is + // blocked) and then says the only thing they need to do, which is nothing. + detail: `GitHub reports problems with ${affected}. This isn't your setup — ADE will reconnect on its own.`, + settingsDetail: `GitHub reports problems with ${affected}. Nothing here needs changing — ADE keeps retrying and reconnects when GitHub is back.`, + action: "GitHub status", + actionUrl: health.incidentUrl ?? GITHUB_STATUS_PAGE_URL, + // Surfaces only, sorted. GitHub flips component severities several times + // per incident; including the level would resurface a dismissed banner on + // every flip, including when the incident NARROWS. + fingerprint: health.affected.map((entry) => entry.surface).sort().join(","), + }; +} + export function githubCredentialPresentation( status: GitHubStatus | null, ): GithubCredentialPresentation { @@ -283,6 +352,15 @@ export function describeGithubPatVerification(result: GitHubSetTokenResult): { message: `Token saved, but ADE cannot use it for write actions on ${repoLabel}. Check the token's repository access and write permissions.`, }; } + // GitHub itself failed, so the token is unverified rather than bad. This is + // the highest-risk place to get the blame wrong: the user is already in the + // token field, so "check the token" reads as "replace it". + if (failure?.kind === "service_unavailable") { + return { + verified: false, + message: "Token saved, but GitHub returned an error instead of verifying it. Nothing to change here — ADE will verify it once GitHub recovers.", + }; + } if (failure?.kind === "network") { return { verified: false, @@ -309,6 +387,16 @@ export function describeGithubCliBanner(status: GitHubStatus): { action: "Connect GitHub", }; } + // Below this point every state is inferred from GitHub's answers, so an + // outage invalidates all of them. (The check sits AFTER `!tokenStored`: that + // one is a purely local fact and stays true regardless of GitHub's health.) + // + // Belt-and-braces: IntegrationBannerHost suppresses this whole banner during + // a corroborated outage, so in production this branch is already unreachable. + // It stays so that any other caller — or a future refactor of that + // suppression — cannot silently reintroduce the credential accusation. + const outage = describeGithubOutage(status); + if (outage) return outage; if (status.connected && !githubStatusHasWriteCredential(status)) { return { subState: "no-write-credential", @@ -338,14 +426,13 @@ export function describeGithubCliBanner(status: GitHubStatus): { }; } -export function describeGithubAuthFailure(status: GitHubStatus): { - subState: string; - statusLabel: string; - title: string; - detail: string; - settingsDetail: string; - action: string; -} | null { +export function describeGithubAuthFailure( + status: GitHubStatus, +): GithubFailurePresentation | null { + // Corroborated outage outranks every credential-shaped reading of the same + // failure: whatever GitHub returned, the cause is GitHub. + const outage = describeGithubOutage(status); + if (outage) return outage; if (status.authFailure?.kind === "rate_limited") { const retryAt = formatGithubRetryAt(status.authFailure.retryAt); return { @@ -371,6 +458,21 @@ export function describeGithubAuthFailure(status: GitHubStatus): { action: "Reconnect GitHub", }; } + // GitHub answered with a 5xx. Even without status-page corroboration this is + // provably not a credential problem, so it must never suggest reconnecting. + if (status.authFailure?.kind === "service_unavailable") { + return { + subState: "service-unavailable", + statusLabel: "GitHub error", + title: "GitHub isn't responding", + detail: "This isn't your setup — ADE will keep retrying.", + settingsDetail: `GitHub returned an error instead of an answer, so ADE couldn't finish the check. This is not a problem with your credential, and reconnecting won't help. GitHub said: ${status.authFailure.message}`, + // Matches the rate-limited/network siblings: the banner wires this to + // ADE Settings, so it must not read as a link to githubstatus.com. Only + // the corroborated-outage presentation carries a real external URL. + action: "View GitHub status", + }; + } if (status.authFailure?.kind === "network") { return { subState: "network", diff --git a/apps/desktop/src/shared/githubOperationCredential.ts b/apps/desktop/src/shared/githubOperationCredential.ts index e73bf4453..2e9dcb3b5 100644 --- a/apps/desktop/src/shared/githubOperationCredential.ts +++ b/apps/desktop/src/shared/githubOperationCredential.ts @@ -314,7 +314,14 @@ export async function resolveGithubStatusCredentials< } failures.push({ candidate, ...result }); args.onRejectedProbe(candidate, result, { repositoryAccessFailure, phase: "read" }); - if (result.authFailure.kind === "network" || result.authFailure.kind === "unknown") break; + if ( + result.authFailure.kind === "network" + || result.authFailure.kind === "unknown" + // GitHub returning 5xx says nothing about this credential, so the next + // one in the chain would fail identically. Stop instead of multiplying + // load against a service that is already failing. + || result.authFailure.kind === "service_unavailable" + ) break; continue; } const candidateCapabilities = args.capabilities(candidate, result.value); diff --git a/apps/desktop/src/shared/githubServiceHealth.test.ts b/apps/desktop/src/shared/githubServiceHealth.test.ts new file mode 100644 index 000000000..dbc717af4 --- /dev/null +++ b/apps/desktop/src/shared/githubServiceHealth.test.ts @@ -0,0 +1,149 @@ +import { describe, expect, it } from "vitest"; +import { + deriveGitHubServiceHealth, + githubServiceAffectedLabel, + isGithubServiceUnavailable, +} from "./githubServiceHealth"; + +function summary(components: Array<{ id?: string; name: string; status: string }>, extra: Record = {}) { + return { + status: { indicator: "major", description: "Partial System Outage" }, + components, + incidents: [], + ...extra, + }; +} + +describe("deriveGitHubServiceHealth", () => { + it("reports the components ADE depends on, worst first", () => { + const health = deriveGitHubServiceHealth( + summary([ + { id: "8l4ygp009s5s", name: "Git Operations", status: "degraded_performance" }, + { id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }, + { id: "4230lsnqdsld", name: "Webhooks", status: "partial_outage" }, + ]), + ); + expect(health).not.toBeNull(); + expect(health?.affected.map((entry) => entry.surface)).toEqual(["api", "webhooks", "git"]); + }); + + it("stays silent when every ADE-relevant component is operational", () => { + expect( + deriveGitHubServiceHealth( + summary([ + { id: "brv1bkgrwx7q", name: "API Requests", status: "operational" }, + { id: "hhtssxt0f5v2", name: "Pull Requests", status: "operational" }, + ]), + ), + ).toBeNull(); + }); + + // The strictness rule: a page-wide "major" indicator driven entirely by + // products ADE never calls must not become an ADE-facing outage claim. + it("ignores outages confined to components ADE does not use", () => { + expect( + deriveGitHubServiceHealth( + summary([ + { id: "pjmpxvq2cmr2", name: "Copilot", status: "major_outage" }, + { id: "h2ftsgbw7kmk", name: "Codespaces", status: "major_outage" }, + { id: "vg70hn9s2tyj", name: "Pages", status: "degraded_performance" }, + { id: "st3j38cctv9l", name: "Packages", status: "partial_outage" }, + { id: "brv1bkgrwx7q", name: "API Requests", status: "operational" }, + ]), + ), + ).toBeNull(); + }); + + it("matches components by name when the id is unfamiliar", () => { + const health = deriveGitHubServiceHealth( + summary([{ id: "rotated-id", name: "Pull Requests", status: "major_outage" }]), + ); + expect(health?.affected[0]?.surface).toBe("pulls"); + }); + + it("attaches the active incident and skips resolved ones", () => { + const health = deriveGitHubServiceHealth( + summary( + [{ id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }], + { + incidents: [ + { name: "Old incident", shortlink: "https://stspg.io/old", resolved_at: "2026-08-16T00:00:00Z" }, + { name: "Incident with GitHub.com", shortlink: "https://stspg.io/new", resolved_at: null }, + ], + }, + ), + ); + expect(health?.incidentUrl).toBe("https://stspg.io/new"); + }); + + // The incident link is third-party data that reaches window.open on the web + // client, which has no allowlist of its own. + it("drops an incident link that is not https", () => { + const health = deriveGitHubServiceHealth( + summary( + [{ id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }], + { incidents: [{ name: "x", shortlink: "javascript:alert(1)", resolved_at: null }] }, + ), + ); + expect(health?.incidentUrl).toBeNull(); + }); + + it("returns null for malformed payloads rather than inventing an outage", () => { + expect(deriveGitHubServiceHealth(null)).toBeNull(); + expect(deriveGitHubServiceHealth("503")).toBeNull(); + expect(deriveGitHubServiceHealth({})).toBeNull(); + expect(deriveGitHubServiceHealth({ components: "nope" })).toBeNull(); + expect( + deriveGitHubServiceHealth({ components: [{ id: "brv1bkgrwx7q", status: "bogus_status" }] }), + ).toBeNull(); + }); + + it("assumes a major incident when the status block is missing", () => { + const health = deriveGitHubServiceHealth( + { components: [{ id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }] }, + ); + expect(health?.indicator).toBe("major"); + }); +}); + +describe("githubServiceAffectedLabel", () => { + it("joins component names for display", () => { + const health = deriveGitHubServiceHealth( + summary([ + { id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }, + { id: "hhtssxt0f5v2", name: "Pull Requests", status: "major_outage" }, + ]), + )!; + expect(githubServiceAffectedLabel(health)).toBe("API Requests and Pull Requests"); + }); +}); + +describe("isGithubServiceUnavailable", () => { + it("detects GitHub's own 5xx body", () => { + expect( + isGithubServiceUnavailable({ + message: "No server is currently available to service your request. Sorry about that.", + }), + ).toBe(true); + }); + + it("detects any 5xx status even with an opaque message", () => { + expect(isGithubServiceUnavailable({ status: 503, message: "" })).toBe(true); + expect(isGithubServiceUnavailable({ status: 500, message: "oops" })).toBe(true); + }); + + it("does not claim an outage for credential rejections", () => { + expect(isGithubServiceUnavailable({ status: 401, message: "Bad credentials" })).toBe(false); + expect(isGithubServiceUnavailable({ status: 403, message: "Resource not accessible" })).toBe(false); + expect(isGithubServiceUnavailable({ message: null })).toBe(false); + }); + + // Generic wording appears on responses that ARE the user's problem (a proxy + // error page, a 404 for a repo the credential cannot see). Telling those + // users "nothing to fix here" would be a lie, so message-only matching must + // stay narrow to GitHub's own phrasing. + it("does not treat generic error wording as a GitHub outage", () => { + expect(isGithubServiceUnavailable({ message: "Internal server error" })).toBe(false); + expect(isGithubServiceUnavailable({ message: "This is not the web page you are looking for" })).toBe(false); + }); +}); diff --git a/apps/desktop/src/shared/githubServiceHealth.ts b/apps/desktop/src/shared/githubServiceHealth.ts new file mode 100644 index 000000000..bf7d2f9d9 --- /dev/null +++ b/apps/desktop/src/shared/githubServiceHealth.ts @@ -0,0 +1,235 @@ +/** + * GitHub's own status page, used to tell "your credential is broken" apart from + * "GitHub is broken". + * + * ADE's GitHub failure classification can only see the response it got. A 503 + * from api.github.com is indistinguishable from a misconfigured token at that + * layer, so ADE used to render GitHub outages as "GitHub authentication check + * failed" — blaming the user for an incident they can't fix, and pushing them + * toward a reconnect that risks destroying a working credential. + * + * githubstatus.com is a Statuspage instance hosted OUTSIDE GitHub's own + * infrastructure, so it stays reachable while GitHub is down. We use it purely + * as corroboration for a failure we already observed — never as a poll. + * + * STRICTNESS: attribution requires a specific component ADE actually depends on + * to be non-operational. A global "major" indicator driven entirely by + * Codespaces or Copilot must NOT make ADE claim GitHub is broken for us, and a + * healthy status page must never be used to assert the opposite ("GitHub is + * fine, so it's your fault") — the status page lags real incidents by 10-20 + * minutes, so absence of a reported incident proves nothing. + */ + +/** Statuspage component health, in increasing order of severity. */ +export type GitHubServiceComponentStatus = + | "operational" + | "under_maintenance" + | "degraded_performance" + | "partial_outage" + | "major_outage"; + +/** Statuspage rollup indicator for the whole page. */ +export type GitHubServiceIndicator = "none" | "minor" | "major" | "critical"; + +/** + * The GitHub capabilities ADE depends on. Deliberately a subset of the status + * page's components: Pages, Packages, Codespaces, and Copilot are excluded + * because ADE's GitHub integration does not use them, and an outage confined to + * those must not be attributed to ADE's failing request. + */ +export type GitHubServiceSurface = + | "api" + | "pulls" + | "issues" + | "actions" + | "webhooks" + | "git"; + +/** + * Statuspage component IDs are stable identifiers, so they are the primary key. + * Names are matched as a fallback in case GitHub ever re-creates a component. + */ +const COMPONENT_SURFACES: ReadonlyArray<{ + id: string; + name: string; + surface: GitHubServiceSurface; +}> = [ + { id: "brv1bkgrwx7q", name: "api requests", surface: "api" }, + { id: "hhtssxt0f5v2", name: "pull requests", surface: "pulls" }, + { id: "kr09ddfgbfsf", name: "issues", surface: "issues" }, + { id: "br0l2tvcx85d", name: "actions", surface: "actions" }, + { id: "4230lsnqdsld", name: "webhooks", surface: "webhooks" }, + { id: "8l4ygp009s5s", name: "git operations", surface: "git" }, +]; + +const COMPONENT_STATUSES: ReadonlySet = new Set([ + "operational", + "under_maintenance", + "degraded_performance", + "partial_outage", + "major_outage", +]); + +const INDICATORS: ReadonlySet = new Set([ + "none", + "minor", + "major", + "critical", +]); + +export const GITHUB_STATUS_PAGE_URL = "https://www.githubstatus.com"; +export const GITHUB_STATUS_SUMMARY_URL = "https://www.githubstatus.com/api/v2/summary.json"; + +export type GitHubServiceComponent = { + surface: GitHubServiceSurface; + /** GitHub's own display name, e.g. "Pull Requests". */ + name: string; + status: Exclude; +}; + +/** + * A corroborated GitHub incident affecting at least one surface ADE uses. + * Only ever constructed when {@link deriveGitHubServiceHealth} finds a + * non-operational ADE-relevant component — its existence IS the assertion that + * GitHub is genuinely having issues. + */ +export type GitHubServiceHealth = { + indicator: GitHubServiceIndicator; + /** Non-operational ADE-relevant components, worst first. */ + affected: GitHubServiceComponent[]; + /** Link to the live incident, when the status page named one. */ + incidentUrl: string | null; +}; + +const STATUS_SEVERITY: Record = { + operational: 0, + under_maintenance: 1, + degraded_performance: 2, + partial_outage: 3, + major_outage: 4, +}; + +function asRecord(value: unknown): Record | null { + return typeof value === "object" && value !== null && !Array.isArray(value) + ? (value as Record) + : null; +} + +function asTrimmedString(value: unknown): string | null { + return typeof value === "string" && value.trim() !== "" ? value.trim() : null; +} + +function surfaceFor(id: string | null, name: string | null): GitHubServiceSurface | null { + const normalizedName = name?.toLowerCase() ?? null; + const match = COMPONENT_SURFACES.find((entry) => ( + (id != null && entry.id === id) + || (normalizedName != null && entry.name === normalizedName) + )); + return match?.surface ?? null; +} + +/** + * Parse a Statuspage `summary.json` payload into ADE's view of GitHub health. + * + * Returns null when the payload is unusable OR when nothing ADE depends on is + * degraded. Callers treat null as "say nothing", which is the whole point: a + * silent status page must never produce UI. + */ +export function deriveGitHubServiceHealth( + payload: unknown, +): GitHubServiceHealth | null { + const root = asRecord(payload); + if (!root) return null; + + const components = Array.isArray(root.components) ? root.components : []; + const affected: GitHubServiceComponent[] = []; + for (const raw of components) { + const component = asRecord(raw); + if (!component) continue; + const status = asTrimmedString(component.status)?.toLowerCase() ?? null; + if (status == null || !COMPONENT_STATUSES.has(status)) continue; + if (status === "operational") continue; + const name = asTrimmedString(component.name); + const surface = surfaceFor(asTrimmedString(component.id), name); + if (surface == null) continue; + affected.push({ + surface, + name: name ?? surface, + status: status as GitHubServiceComponent["status"], + }); + } + // Strict: no ADE-relevant component is degraded, so ADE has nothing honest to + // say about GitHub, regardless of the page-wide indicator. + if (affected.length === 0) return null; + affected.sort((left, right) => STATUS_SEVERITY[right.status] - STATUS_SEVERITY[left.status]); + + const statusBlock = asRecord(root.status); + const rawIndicator = asTrimmedString(statusBlock?.indicator)?.toLowerCase() ?? null; + const indicator: GitHubServiceIndicator = rawIndicator != null && INDICATORS.has(rawIndicator) + ? (rawIndicator as GitHubServiceIndicator) + : "major"; + + const incidents = Array.isArray(root.incidents) ? root.incidents : []; + const incident = incidents + .map(asRecord) + .find((entry) => entry != null && asTrimmedString(entry.resolved_at) == null) ?? null; + + return { + indicator, + affected, + incidentUrl: incident ? asHttpsUrl(incident.shortlink) : null, + }; +} + +/** + * The incident link is third-party data that ends up in `openExternalUrl`. + * Desktop routes that through an allowlist, but the web client falls back to a + * bare `window.open`, so the scheme is validated here at the trust boundary + * rather than relied upon at each sink. + */ +function asHttpsUrl(value: unknown): string | null { + const raw = asTrimmedString(value); + if (raw == null) return null; + try { + return new URL(raw).protocol === "https:" ? raw : null; + } catch { + return null; + } +} + +/** Human list of the affected components, e.g. "API Requests and Pull Requests". */ +export function githubServiceAffectedLabel(health: GitHubServiceHealth): string { + const names = health.affected.slice(0, 3).map((entry) => entry.name); + if (names.length === 0) return "some services"; + if (names.length === 1) return names[0]!; + return `${names.slice(0, -1).join(", ")} and ${names[names.length - 1]!}`; +} + +/** + * GitHub's own 5xx bodies. During an incident api.github.com serves these + * instead of JSON, and by the time the text reaches a UI surface the HTTP + * status is usually gone, so the wording has to be matchable on its own. + * + * Deliberately narrow — only phrases GitHub uses for its OWN failures. + * Generic wording ("server error") and GitHub's 404 page text were tried and + * removed: both also appear on responses that are genuinely the user's problem + * (a proxy error page, a 404 for a repo the credential cannot see), where + * "nothing to fix here" would be a lie. A flat literal alternation, so it is + * linear-time on any input. + */ +const GITHUB_SERVICE_UNAVAILABLE_MESSAGE = + /no server is currently available to service your request|service unavailable|bad gateway|gateway time-?out|unicorn!/i; + +/** + * True when GitHub itself failed, as opposed to rejecting the credential. + * + * Distinct from a transient local network fault: that one is the user's to + * retry, while this one must never be presented as an authentication problem. + */ +export function isGithubServiceUnavailable(args: { + status?: number; + message: string | null | undefined; +}): boolean { + if (args.status != null && args.status >= 500 && args.status <= 599) return true; + return GITHUB_SERVICE_UNAVAILABLE_MESSAGE.test(args.message ?? ""); +} diff --git a/apps/desktop/src/shared/types/git.ts b/apps/desktop/src/shared/types/git.ts index 0b5067d41..fc5fd4c6e 100644 --- a/apps/desktop/src/shared/types/git.ts +++ b/apps/desktop/src/shared/types/git.ts @@ -2,6 +2,8 @@ // Git types // --------------------------------------------------------------------------- +import type { GitHubServiceHealth } from "../githubServiceHealth"; + export type GitSyncMode = "merge" | "rebase"; export type GitPullMode = "ff-only" | "rebase" | "merge"; @@ -308,7 +310,17 @@ export type GitHubRateLimitState = { }; export type GitHubAuthFailure = { - kind: "rate_limited" | "invalid_token" | "permission_denied" | "network" | "unknown"; + // `service_unavailable` means GitHub itself returned 5xx. It is NOT a + // credential problem, and clients must not offer reconnect/re-auth for it — + // reconnecting during a GitHub outage cannot help and risks the user + // replacing a perfectly good token. + kind: + | "rate_limited" + | "invalid_token" + | "permission_denied" + | "service_unavailable" + | "network" + | "unknown"; message: string; retryAt: string | null; }; @@ -367,6 +379,11 @@ export type GitHubStatus = { // flattened into "missing scopes" by clients. authFailure?: GitHubAuthFailure | null; rateLimit?: GitHubRateLimitState | null; + // Set only when a GitHub request failed AND githubstatus.com corroborates an + // incident on a surface ADE depends on. Present means "this failure is + // GitHub's, not the user's"; absent means ADE makes no claim either way (the + // status page lags real incidents, so absence proves nothing). + serviceHealth?: GitHubServiceHealth | null; // Optional for compatibility with older runtimes. These fields describe the // operation credential chain without exposing credential material. writeAuthSource?: Exclude; diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index c56c02c1e..c8c17a1dc 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1107,7 +1107,7 @@ Themes: six shipped themes (`e-paper`, `bloomberg`, `github`, `rainbow`, `sky`, - `renderer/lib/dialogBus.ts` — tiny pub/sub that lets shared UI open/close dialogs by a stable id (`lanes.create`, `settings.ai`, etc.) without prop-drilling. Dialogs subscribe by id; a `subscribeAll` channel exists for devtools. Default singleton export `dialogBus`. - `renderer/components/app/toast/` - shared renderer-only toast primitive. `toastStore.ts` owns stack order, timers, hover pause/resume, sticky toasts, and in-place replacement; `ToastStack.tsx` renders inside AppShell's existing bottom-right notice container. Lane lifecycle and automated rebase terminal events subscribe through `useLaneEventToasts.ts`. -- `renderer/components/shared/Banner.tsx` + `renderer/components/app/IntegrationBannerHost.tsx` — the shared connection/health banner system. `Banner` is the one severity-tinted row every integration banner renders through (error/warning/info accent, normal UI font, never monospace). `IntegrationBannerHost` (mounted once by `AppShell`) computes and renders the whole family — GitHub App account authorization, per-repo App install, gh-CLI/token, missing-AI-provider, mock-provider, and ADE Relay outage — as one severity-ranked list capped at two visible (`MAX_VISIBLE_BANNERS`) with a collapse-the-rest control, in place of the hand-ordered `? :` conditionals that used to live inline in `AppShell` (feature-local one-off banners such as the provider-settings and rebase-tab notices are unaffected). Dismissal is durable and fingerprint-aware via `renderer/lib/bannerDismiss.ts` (localStorage-backed, so a dismissal survives restart and project reopen; a dismissed banner auto-resurfaces after ~2 weeks or the moment its underlying state changes/regresses to a different fingerprint). GitHub App health is derived in `renderer/lib/githubIntegrationStatus.ts` (see [Onboarding and settings](./features/onboarding-and-settings/README.md)), so the banner and Settings never disagree. The relay banner reads `routeHealth.relay` from `AppShell`'s `sync-status` subscription seeded by `sync.getLocalStatus` (relay belongs to the physical machine, not to whichever runtime a remote-bound project routes to): a deliberate suppression is reported immediately, while a plain outage waits out `RELAY_OUTAGE_GRACE_MS` (2 minutes) of uninterrupted failure on a single armed timer rather than a poll. See [Sync and multi-device](./features/sync-and-multi-device/README.md). +- `renderer/components/shared/Banner.tsx` + `renderer/components/app/IntegrationBannerHost.tsx` — the shared connection/health banner system. `Banner` is the one severity-tinted row every integration banner renders through (error/warning/info accent, normal UI font, never monospace). `IntegrationBannerHost` (mounted once by `AppShell`) computes and renders the whole family — GitHub outage, GitHub App account authorization, per-repo App install, gh-CLI/token, missing-AI-provider, mock-provider, and ADE Relay outage — as one severity-ranked list capped at two visible (`MAX_VISIBLE_BANNERS`) with a collapse-the-rest control, in place of the hand-ordered `? :` conditionals that used to live inline in `AppShell` (feature-local one-off banners such as the provider-settings and rebase-tab notices are unaffected). Dismissal is durable and fingerprint-aware via `renderer/lib/bannerDismiss.ts` (localStorage-backed, so a dismissal survives restart and project reopen; a dismissed banner auto-resurfaces after ~2 weeks or the moment its underlying state changes/regresses to a different fingerprint). GitHub App health is derived in `renderer/lib/githubIntegrationStatus.ts` (see [Onboarding and settings](./features/onboarding-and-settings/README.md)), so the banner and Settings never disagree. When `describeGithubOutage` reports a githubstatus.com-corroborated incident, the three GitHub banners are suppressed and replaced by a single neutral `info` notice linking the live incident — three separate accusations against the user's setup would all be symptoms of one cause they cannot fix, and re-authorizing during a GitHub outage is how a working credential gets destroyed. The suppression is gated on the same condition that renders the replacement, and the notice is pinned ahead of severity ordering so the slice into overflow can never hide the explanation while it is still silencing the banners; only the GitHub family is affected. See [Pull requests](./features/pull-requests/README.md#telling-a-github-outage-apart-from-a-broken-credential). The relay banner reads `routeHealth.relay` from `AppShell`'s `sync-status` subscription seeded by `sync.getLocalStatus` (relay belongs to the physical machine, not to whichever runtime a remote-bound project routes to): a deliberate suppression is reported immediately, while a plain outage waits out `RELAY_OUTAGE_GRACE_MS` (2 minutes) of uninterrupted failure on a single armed timer rather than a poll. See [Sync and multi-device](./features/sync-and-multi-device/README.md). - `renderer/onboarding/docsLinks.ts` — typed registry of internal/public doc URLs (`docs.lanes`, `docs.cto`, …) used by `DidYouKnow`, glossary/help surfaces, and the `HelpMenu`. - `renderer/components/onboarding/LaunchGate.tsx` — fresh-process account-choice gate. New installs see the welcome card first; returning signed-out launches go directly to sign-in or **Continue without an account**. Resolving it is process-local so extra windows and renderer reloads do not repeat it. - `renderer/components/onboarding/WelcomeVideoGate.tsx` — app-level one-time welcome card using the website's canonical desktop/mobile/terminal hero assets, a YouTube thumbnail button that hands off to the system browser (the packaged app's `file://` origin makes YouTube's embed player reject an actual iframe with error 153), and the ADE Mobile TestFlight QR/download/copy panel. Seen/dismissed state is stored in the global app state file, separate from per-project setup onboarding. diff --git a/docs/features/onboarding-and-settings/README.md b/docs/features/onboarding-and-settings/README.md index 0e88085e4..a552a8258 100644 --- a/docs/features/onboarding-and-settings/README.md +++ b/docs/features/onboarding-and-settings/README.md @@ -106,15 +106,38 @@ Main process: user, OAuth, and PAT credentials for the same account, so an exhausted primary bucket pauses known credentials for that account instead of cycling tokens. `GitHubStatus.authFailure` distinguishes rate limiting, invalid credentials, - permission denial, network failures, and unknown validation errors so clients - do not flatten every failed probe into missing permissions. + permission denial, GitHub's own 5xx (`service_unavailable`), network failures, + and unknown validation errors so clients do not flatten every failed probe + into missing permissions. `service_unavailable` gets no credential cooldown, + because the credential is not the problem and must stay usable the instant + GitHub recovers. +- `apps/desktop/src/shared/githubServiceHealth.ts` and + `apps/desktop/src/main/services/github/githubStatusPage.ts` — telling a GitHub + outage apart from a broken credential. The shared module parses + githubstatus.com's Statuspage `summary.json` into `GitHubServiceHealth` and + exposes `isGithubServiceUnavailable`; it counts only components ADE actually + uses (API Requests, Pull Requests, Issues, Actions, Webhooks, Git Operations), + so a Copilot or Codespaces outage never becomes an ADE claim. The main-process + module is failure-triggered and never polls: it runs only after a GitHub + request already failed, caches results (including negative ones) for 60 s, + times out at 2 s, and fails silent. `attachGitHubServiceHealth` populates + `GitHubStatus.serviceHealth` and is applied in both `getStatus` owners. + Attribution is one-directional — a corroborated incident clears the user of + blame, while a healthy status page never implies the opposite, because the + page lags real incidents by 10-20 minutes. See + [pull requests](../pull-requests/README.md#telling-a-github-outage-apart-from-a-broken-credential). - `apps/desktop/src/shared/githubOperationCredential.ts` — the capability-aware read/write credential order, App read-only rule, and duplicate-token removal - used by desktop and runtime-side GitHub services. + used by desktop and runtime-side GitHub services. A `service_unavailable` + probe stops the credential walk instead of retrying every candidate against a + service that is already failing. - `apps/ade-cli/src/headlessLinearServices.ts` — runtime-owned mirror of the GitHub request/status path. It applies the same candidate order, cooldowns, - GraphQL classification, conditional-request cache isolation, and read/write - status fields when a packaged or remote-bound window uses `ade serve`. + GraphQL classification, conditional-request cache isolation, read/write + status fields, and githubstatus.com corroboration when a packaged or + remote-bound window uses `ade serve`. The renderer reaches GitHub through + whichever service owns the project, so anything applied to only one of the two + `getStatus` implementations is dead in the shipping runtime-backed build. - `apps/desktop/src/main/services/config/projectConfigService.ts` — YAML config read/merge/save, AI mode migration, lane env init, Linear sync resolver. ~3,150 lines, the largest service. @@ -139,9 +162,9 @@ Shared types and IPC: - `apps/desktop/src/shared/types/git.ts` — `GitHubStatus`, `GitHubAuthFailure`, `GitHubRateLimitState`, and the credential source, capability, state, and fallback contracts. `writeAuthSource`, - `credentialStates`, `credentialFallback`, and - `backgroundRefreshPausedUntil` are optional so a newer client remains - compatible with an older remote runtime. + `credentialStates`, `credentialFallback`, `backgroundRefreshPausedUntil`, and + `serviceHealth` are optional so a newer client remains compatible with an + older remote runtime. - `apps/desktop/src/shared/ipc.ts` — channels: - `ade.onboarding.*` (status, detectDefaults, applySuggestedConfig, complete, setDismissed) @@ -335,8 +358,16 @@ Renderer — settings: GitHub's raw request-id / scraping-policy error. Raw network/unknown validation errors stay in Settings rather than the global banner. The shared `renderer/lib/githubIntegrationStatus.ts` presentation helper keeps banner - and Settings classification aligned. This section also hosts the - `GitHubAppInstallPanel` (below) for installing "ADE for GitHub". + and Settings classification aligned. When githubstatus.com corroborates an + outage the whole card goes neutral instead of reading as "your setup is + broken": the status chip says "GitHub outage", the failure box drops its + warning tint and grows a link to the live incident, `READS WITH` / `WRITES + WITH` say "Unknown" rather than the false-negative "Not connected", + connection-order badges read "Waiting on GitHub" instead of "Reconnect + needed", and the `gh auth login` instructions are hidden so nobody replaces a + credential that was working. A missing token still shows its instruction — + that is a local fact an outage cannot explain away. This section also hosts + the `GitHubAppInstallPanel` (below) for installing "ADE for GitHub". - `apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.tsx` — install / status card for the hosted ADE GitHub App that backs webhook-relay PR updates. Reads per-repo installation + webhook state via @@ -361,7 +392,9 @@ Renderer — settings: After device authorization succeeds, the panel force-refreshes the hosted relay status with a short retry window and treats GitHub repo-access 404s as a temporary "Checking access" (`access_pending`) state so App installation - propagation does not look like failed authorization. + propagation does not look like failed authorization. A GitHub server error + renders as "Waiting on GitHub" rather than "Couldn't verify": the install + state is unknown, not broken. `clearAppUserAuth` revokes the local token. Offers a Refresh. Rendered in Settings and, in a compact `onboarding` variant, during setup. The device-flow, @@ -381,7 +414,13 @@ Renderer — settings: sub-states. Imported by `GitHubAppInstallPanel`, `IntegrationBannerHost`, and write surfaces such as `FeedbackReporterModal`; App-only read connectivity therefore keeps PR data live while still prompting for GitHub CLI or a PAT - before a mutation. + before a mutation. `describeGithubOutage(status)` is the one presentation + entry point for a corroborated GitHub outage — it answers "is there an + outage", "what do we say", and "where does the button go" together, and every + GitHub-blaming surface gates on it. `describeGithubAuthFailure` and + `describeGithubCliBanner` consult it first, so an outage outranks every + credential-shaped reading of the same failure. When it returns null ADE says + nothing about GitHub's health and keeps its existing copy. - `apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx` and `FeedbackReporterModal.tsx` — consume the shared read/write distinction. The app shell raises a write-access banner for an otherwise connected App-only diff --git a/docs/features/pull-requests/README.md b/docs/features/pull-requests/README.md index 7ff0257eb..860746448 100644 --- a/docs/features/pull-requests/README.md +++ b/docs/features/pull-requests/README.md @@ -182,8 +182,9 @@ GitHub access and relay dependencies: | File | Responsibility | |------|---------------| | `apps/desktop/src/main/services/github/githubService.ts`, `apps/ade-cli/src/headlessLinearServices.ts` | Desktop-local and runtime-owned GitHub request paths. Both build the environment → App → GitHub CLI → PAT read chain, skip the read-only App for writes, retry compatible credentials after auth/permission/rate failures, and expose the active/fallback sources through `GitHubStatus`. | -| `apps/desktop/src/main/services/github/githubCredentialHealth.ts`, `githubRateLimit.ts` | Token-digest health keyed by REST/GraphQL resource, five-minute invalid/permission cooldowns, rate-limit reset handling, same-account primary-quota propagation, and the 500-request background reserve. | -| `apps/desktop/src/shared/githubOperationCredential.ts`, `apps/desktop/src/shared/types/git.ts` | Capability-aware credential order and the optional status DTOs for source state, fallback, write availability, and background-pause time. | +| `apps/desktop/src/main/services/github/githubCredentialHealth.ts`, `githubRateLimit.ts` | Token-digest health keyed by REST/GraphQL resource, five-minute invalid/permission cooldowns, rate-limit reset handling, same-account primary-quota propagation, and the 500-request background reserve. `classifyGitHubAuthFailure` maps GitHub 5xx (and GitHub's own outage bodies) to `service_unavailable` ahead of the transient-network check, and that kind is deliberately given **no** credential cooldown — the credential is not the problem, so parking it would fail the user's next local merge or PR read for the whole window. | +| `apps/desktop/src/main/services/github/githubStatusPage.ts`, `apps/desktop/src/shared/githubServiceHealth.ts` | GitHub-outage attribution. See [Telling a GitHub outage apart from a broken credential](#telling-a-github-outage-apart-from-a-broken-credential). The shared module is the pure half — Statuspage `summary.json` parsing into `GitHubServiceHealth`, the ADE-relevant component allowlist, and `isGithubServiceUnavailable`; the main-process module owns the failure-triggered lookup, its cache, and `attachGitHubServiceHealth`. | +| `apps/desktop/src/shared/githubOperationCredential.ts`, `apps/desktop/src/shared/types/git.ts` | Capability-aware credential order and the optional status DTOs for source state, fallback, write availability, background-pause time, and corroborated service health. `resolveGithubStatusCredentials` stops walking the chain on `service_unavailable` alongside `network` / `unknown`: a GitHub 5xx says nothing about the credential, so the next candidate would fail identically and only add load to a failing service. | | `apps/desktop/src/main/services/automations/automationIngressService.ts`, `apps/ade-cli/src/bootstrap.ts`, `apps/desktop/src/main/main.ts` | Relay cursor drain and targeted reconciliation, relay-health tracking, and injection of relay/quota state into the runtime-owned or desktop-local PR poller. | | `apps/webhook-relay/src/relay.ts` | Hosted event/subscription authorization. Signed-in ADE account requests use the installed repository binding in D1 first; legacy clients fall back to a GitHub-token repository-access check. | @@ -924,9 +925,16 @@ Fields: attempted once. - `authFailure` — optional structured validation failure for compatibility with older runtimes: `rate_limited`, `invalid_token`, `permission_denied`, - `network`, or `unknown`, with the original message and optional retry time. A - present failure means ADE found credentials but could not finish validating - a usable read path; clients must not reinterpret that as missing scopes. + `service_unavailable`, `network`, or `unknown`, with the original message and + optional retry time. A present failure means ADE found credentials but could + not finish validating a usable read path; clients must not reinterpret that + as missing scopes. `service_unavailable` means GitHub itself answered with + 5xx: it is provably not a credential problem, so no client may offer + reconnect or re-auth for it. +- `serviceHealth` — optional `GitHubServiceHealth`, set only when a request + already failed **and** githubstatus.com corroborates an incident on a surface + ADE uses. Present means "this failure is GitHub's, not yours"; absent means + ADE makes no claim in either direction. - `rateLimit` — the latest quota headers (`limit`, `remaining`, `used`, `resetAt`, and `resource`) from the active status probe. - `backgroundRefreshPausedUntil` — optional reset time exposed when the core or @@ -989,6 +997,100 @@ stays quiet when a fallback keeps reads and writes usable, distinguishes App-only read access from a write-capable connection, and never advertises a reconnect command for an account-level rate-limit pause. +## Telling a GitHub outage apart from a broken credential + +A failing GitHub request looks the same at the response layer whether the +credential is wrong or GitHub is down, so ADE used to render an incident as +"GitHub authentication check failed" — blaming the user for something they +cannot fix and pushing them toward a reconnect that can destroy a working +credential. Two layers now separate the two cases. + +**The response itself.** `isGithubServiceUnavailable` in +`apps/desktop/src/shared/githubServiceHealth.ts` treats any 5xx status as +GitHub's failure, and additionally matches GitHub's own outage bodies (`no +server is currently available to service your request`, `service unavailable`, +`bad gateway`, `gateway timeout`, `unicorn!`) for the surfaces where the HTTP +status is already gone by the time the text reaches the UI. The pattern is +deliberately narrow: generic wording like "server error" and GitHub's 404 page +text also appear on responses that genuinely are the user's problem, where +"nothing to fix here" would be a lie. This alone produces the +`service_unavailable` auth-failure kind, with no network call. + +**Corroboration.** `apps/desktop/src/main/services/github/githubStatusPage.ts` +reads githubstatus.com's Statuspage `summary.json` — hosted outside GitHub's +infrastructure, so it stays reachable while GitHub is down. It is **not a +poller**: while GitHub works, ADE makes zero requests to it and no third party +learns the app is running. The only trigger is a failure ADE already observed, +and only for the `service_unavailable` and `unknown` kinds. `invalid_token`, +`permission_denied`, and `rate_limited` are definitive answers from GitHub +about *this credential*, so letting an unrelated mild degradation overwrite +them would hide the user's actual remedy; `network` is ADE's own connectivity +failing, when the status page is just as unreachable. Results — including +negative ones — are cached for 60 s, the lookup has a 2 s timeout, and every +failure mode resolves to "say nothing", because an unreachable status page must +never itself become a banner. + +`deriveGitHubServiceHealth` is strict about what counts: attribution requires a +component ADE actually depends on (API Requests, Pull Requests, Issues, +Actions, Webhooks, Git Operations) to be non-operational. A page-wide "major" +indicator driven entirely by Copilot, Codespaces, Pages, or Packages produces +nothing. Components are keyed by Statuspage's stable IDs with name matching as +a fallback, and the incident shortlink is validated as `https:` at this trust +boundary because it ends up in `openExternalUrl`. + +**Attribution is one-directional.** A corroborated incident lets ADE stop +blaming the credential. A *healthy* status page never means "so it's your +fault" — the page lags real incidents by 10-20 minutes, so absence of a +reported incident proves nothing, and ADE keeps its existing error copy. + +`attachGitHubServiceHealth` applies this at the single exit of `getStatus` in +**both** owners — the desktop in-process `githubService` and the headless +`createHeadlessGitHubService` in `apps/ade-cli/src/headlessLinearServices.ts`. +The renderer reaches GitHub through whichever of those owns the project, so +wrapping only one leaves the feature inert in the shipping runtime-backed +build. It wraps the resolved status rather than sitting inside the lookup, so a +status-cache hit still gets fresh corroboration. + +### What the UI does during a corroborated outage + +`describeGithubOutage(status)` in `renderer/lib/githubIntegrationStatus.ts` is +the single presentation entry point — one function rather than a family of +predicates, because every caller needs the same three things together (is there +an outage, what do we say, where does the button go). It returns null when +nothing is corroborated, and every GitHub-blaming surface gates on it: + +- `IntegrationBannerHost` collapses the whole GitHub banner family into one + neutral `info` notice linking the live incident. The other banners (AI + provider, mock provider, relay) are untouched, and the suppression is gated + on the same condition that renders the replacement, so the GitHub family can + never go silent without its explanation appearing. The notice is pinned first + in the sort order despite being `info`, so severity ranking cannot push the + explanation into the collapsed overflow while it is still suppressing the + banners it replaces. Its dismissal fingerprint is the affected surfaces only + (not their severity levels), so a widening incident resurfaces a dismissed + banner while GitHub's routine severity flapping does not. +- `GitHubSection` goes neutral: the status chip reads "GitHub outage", the + auth-failure box drops its warning tint, `READS WITH` / `WRITES WITH` report + "Unknown" instead of the false-negative "Not connected", credential-ladder + cooldown badges read "Waiting on GitHub" instead of "Reconnect needed", and + the `gh auth login` instructions are hidden so nobody replaces a credential + that was never broken. A *missing* token still shows its instruction — that + is a local fact an outage cannot explain away. +- `GitHubAppInstallPanel` reports the per-repo install state as "Waiting on + GitHub" rather than "Couldn't verify". +- `describeGithubPatVerification` says a saved token is unverified rather than + bad. This is the highest-risk place to misattribute: the user is already in + the token field, so "check the token" reads as "replace it". + +`describeGithubAuthFailure` and `describeGithubCliBanner` both consult +`describeGithubOutage` first, so a corroborated outage outranks every +credential-shaped reading of the same failure even though `IntegrationBannerHost` +already suppresses those banners — the redundancy exists so a future refactor of +that suppression cannot silently reintroduce the accusation. Without +corroboration, a bare `service_unavailable` still renders its own honest copy +("GitHub isn't responding", pointing at ADE Settings rather than an external +link) and never suggests reconnecting. + ## Background polling `prPollingService` runs inside the process that backs the window's runtime — From fedcf8b5723ae3d4096f05f47cbfff3ace02da4a Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:03:25 -0400 Subject: [PATCH 2/2] Address review: corroborated outage for unknown failures, stop write fallback on 5xx, incident-aware banner fingerprint --- .../src/main/services/prs/prService.test.ts | 30 ++++++++++++++ .../src/main/services/prs/prService.ts | 10 ++++- .../app/IntegrationBannerHost.test.tsx | 34 +++++++++++++++ .../renderer/lib/githubIntegrationStatus.ts | 14 +++++-- .../shared/githubOperationCredential.test.ts | 41 +++++++++++++++++++ .../src/shared/githubOperationCredential.ts | 4 ++ 6 files changed, 128 insertions(+), 5 deletions(-) diff --git a/apps/desktop/src/main/services/prs/prService.test.ts b/apps/desktop/src/main/services/prs/prService.test.ts index f2800a034..48db674a7 100644 --- a/apps/desktop/src/main/services/prs/prService.test.ts +++ b/apps/desktop/src/main/services/prs/prService.test.ts @@ -1698,6 +1698,36 @@ describe("prService.getGithubSnapshot", () => { expect(githubService.apiRequest).not.toHaveBeenCalled(); }); + // Corroboration is attached to `unknown` failures too (GitHub answered with + // something we could not classify). A confirmed incident is positive evidence + // regardless of how the response itself was classified. + it("reports a corroborated outage for an unknown auth failure", async () => { + const githubService = makeGithubService({ + getStatus: vi.fn(async () => makeGithubStatus({ + connected: false, + authFailure: { + kind: "unknown", + message: "GitHub returned an unexpected response.", + retryAt: null, + }, + serviceHealth: { + indicator: "major", + affected: [{ surface: "api", name: "API Requests", status: "major_outage" }], + incidentUrl: "https://stspg.io/live", + }, + })), + apiRequest: vi.fn(async () => ({ data: [] })), + }); + const { service } = buildService({ githubService, laneService: makeLaneService([]) }); + + const error = await service.getGithubSnapshot().then( + () => { throw new Error("expected getGithubSnapshot to reject"); }, + (reason: unknown) => reason as Error, + ); + expect(error.message).toContain("GitHub is having problems"); + expect(error.message).not.toContain("Update it in Settings"); + }); + it("names the corroborated outage when GitHub's status page confirms one", async () => { const githubService = makeGithubService({ getStatus: vi.fn(async () => makeGithubStatus({ diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 8347e57ce..9dde2959c 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -9212,7 +9212,15 @@ export function createPrService({ // Without this arm a 503 falls through to the "auth is invalid — update it // in Settings" default, which is the exact accusation the outage work // exists to remove, on the surface a user is most likely to act from. - if (githubStatus.authFailure?.kind === "service_unavailable") { + // + // `serviceHealth` counts on its own: corroboration is also attached to an + // `unknown` failure (GitHub answered with something we could not classify), + // and a confirmed incident is positive evidence regardless of how the + // response itself was classified. + if ( + githubStatus.authFailure?.kind === "service_unavailable" + || githubStatus.serviceHealth != null + ) { return githubStatus.serviceHealth ? "GitHub is having problems, so pull requests can't sync right now. Nothing to change here — ADE will catch up when GitHub recovers." : "GitHub returned an error, so pull requests can't sync right now. This isn't a problem with your GitHub connection — ADE will keep retrying."; diff --git a/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx b/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx index d9e9de32e..5137fd3a7 100644 --- a/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx +++ b/apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx @@ -468,4 +468,38 @@ describe("IntegrationBannerHost relay-offline banner", () => { expect(screen.getByText("GitHub isn't responding")).toBeTruthy(); expect(screen.queryByText("GitHub is down")).toBeNull(); }); + + // Fingerprint carries incident identity, so a NEW incident on the same + // surfaces is not inheriting the previous incident's dismissal. + it("resurfaces a dismissed outage when GitHub changes the incident for the same surfaces", async () => { + setAdeMock({ onStatusChanged: vi.fn(() => () => {}) }); + const secondIncident = deriveGitHubServiceHealth({ + status: { indicator: "major", description: "Partial System Outage" }, + components: [ + { id: "brv1bkgrwx7q", name: "API Requests", status: "major_outage" }, + { id: "hhtssxt0f5v2", name: "Pull Requests", status: "major_outage" }, + ], + incidents: [{ name: "A different incident", shortlink: "https://stspg.io/second", resolved_at: null }], + })!; + + const { rerender } = render( + , + ); + await act(async () => {}); + await act(async () => { + screen.getByRole("button", { name: /^Dismiss/ }).click(); + }); + expect(screen.queryByText("GitHub is down")).toBeNull(); + + await act(async () => { + rerender( + , + ); + }); + expect(screen.getByText("GitHub is down")).toBeTruthy(); + }); }); diff --git a/apps/desktop/src/renderer/lib/githubIntegrationStatus.ts b/apps/desktop/src/renderer/lib/githubIntegrationStatus.ts index 9c7719cee..c1f408611 100644 --- a/apps/desktop/src/renderer/lib/githubIntegrationStatus.ts +++ b/apps/desktop/src/renderer/lib/githubIntegrationStatus.ts @@ -75,10 +75,16 @@ export function describeGithubOutage( settingsDetail: `GitHub reports problems with ${affected}. Nothing here needs changing — ADE keeps retrying and reconnects when GitHub is back.`, action: "GitHub status", actionUrl: health.incidentUrl ?? GITHUB_STATUS_PAGE_URL, - // Surfaces only, sorted. GitHub flips component severities several times - // per incident; including the level would resurface a dismissed banner on - // every flip, including when the incident NARROWS. - fingerprint: health.affected.map((entry) => entry.surface).sort().join(","), + // Incident identity + the affected surfaces, sorted. Severity is + // deliberately excluded: GitHub flips component levels several times per + // incident, and including them would resurface a dismissed banner on every + // flip, including when the incident NARROWS. The incident link IS included + // so a genuinely new incident on the same surfaces resurfaces the notice + // instead of inheriting the previous one's dismissal. + fingerprint: [ + health.incidentUrl ?? "no-incident", + ...health.affected.map((entry) => entry.surface).sort(), + ].join(","), }; } diff --git a/apps/desktop/src/shared/githubOperationCredential.test.ts b/apps/desktop/src/shared/githubOperationCredential.test.ts index 18c188ad0..06f216a98 100644 --- a/apps/desktop/src/shared/githubOperationCredential.test.ts +++ b/apps/desktop/src/shared/githubOperationCredential.test.ts @@ -135,4 +135,45 @@ describe("githubOperationCredential", () => { { repositoryAccessFailure: false, phase: "read" }, ); }); + + // A GitHub 5xx is not credential-specific, so the next write candidate would + // fail identically. Probing on only adds load to a service already failing. + it("stops write fallback after service_unavailable", async () => { + type Candidate = { source: "app" | "gh" | "pat"; token: string }; + const app: Candidate = { source: "app", token: "app" }; + const gh: Candidate = { source: "gh", token: "gh" }; + const pat: Candidate = { source: "pat", token: "pat" }; + type Probe = { repoAccessOk: boolean; write: boolean }; + const probe = vi.fn(async (candidate: Candidate): Promise> => ( + candidate.source === "app" + ? { ok: true as const, value: { repoAccessOk: true, write: false } } + : { + ok: false as const, + error: "503", + authFailure: { + kind: "service_unavailable" as const, + message: "No server is currently available to service your request.", + retryAt: null, + }, + rateLimit: null, + } + )); + + const result = await resolveGithubStatusCredentials({ + readCandidates: [app], + writeCandidates: [gh, pat], + cooldown: () => null, + probe, + capabilities: (_candidate, value) => ({ read: value.repoAccessOk, write: value.write }), + isRepositoryAccessFailure: () => false, + onAuthenticatedProbe: vi.fn(), + onUsableProbe: vi.fn(), + onRejectedProbe: vi.fn(), + }); + + expect(result.activeWrite).toBeNull(); + // `pat` must never be probed: one 5xx ends the write chain. + const writeProbes = probe.mock.calls.map(([candidate]) => candidate.source); + expect(writeProbes).not.toContain("pat"); + }); }); diff --git a/apps/desktop/src/shared/githubOperationCredential.ts b/apps/desktop/src/shared/githubOperationCredential.ts index 2e9dcb3b5..a6a3b44a0 100644 --- a/apps/desktop/src/shared/githubOperationCredential.ts +++ b/apps/desktop/src/shared/githubOperationCredential.ts @@ -376,6 +376,10 @@ export async function resolveGithubStatusCredentials< if (!result.ok) { const repositoryAccessFailure = args.isRepositoryAccessFailure(result); args.onRejectedProbe(candidate, result, { repositoryAccessFailure, phase: "write" }); + // Same reasoning as the read chain: a GitHub 5xx says nothing about + // this credential, so the next one fails identically. Stop instead of + // adding load to a service that is already failing. + if (result.authFailure.kind === "service_unavailable") break; continue; } successfulProbes.set(candidate.token, result.value);