diff --git a/backend/__tests__/unit/routes/github.statusLiveness.test.js b/backend/__tests__/unit/routes/github.statusLiveness.test.js new file mode 100644 index 00000000..89909c42 --- /dev/null +++ b/backend/__tests__/unit/routes/github.statusLiveness.test.js @@ -0,0 +1,174 @@ +/** + * `GET /api/github/status` is the endpoint whose only job is to diagnose the + * GitHub credential. Until this change it answered `configured: true` off + * `!!process.env.GITHUB_PAT` — presence, not liveness — which is why it read + * `configured: true` for the whole 2026-08-04 outage while every proxied call + * returned 401. + * + * Two invariants are pinned here, and the second is the one most likely to be + * "cleaned up" later: + * + * 1. A dead credential is reported as `credentialLive: false`. + * 2. A dead credential is still an HTTP **200**. This route must NOT adopt + * mapGitHubUpstreamError. If the diagnostic returned 502 on a rejected + * credential, a caller could not tell "the credential is dead" from "the + * diagnostic is broken" — the exact collapse #808 removed from the seven + * proxying routes, reintroduced at the one endpoint that exists to + * prevent it. + * + * Unreachable is a third answer, not a synonym for dead: reporting `false` + * when GitHub could not be reached would send an operator to rotate a working + * credential. + */ + +jest.mock('../../../middleware/agentRuntimeAuth', () => (req, res, next) => { + req.agentUser = { _id: 'bot-1' }; + next(); +}); + +let mockCurrentUser = { _id: 'user-1', role: 'admin' }; +jest.mock('../../../middleware/auth', () => (req, res, next) => { + req.user = mockCurrentUser; + req.userId = mockCurrentUser._id; + next(); +}); + +jest.mock('axios'); + +const express = require('express'); +const request = require('supertest'); +const axios = require('axios'); +// eslint-disable-next-line import/no-unresolved, import/extensions +const router = require('../../../routes/github'); + +const app = express(); +app.use(express.json()); +app.use('/api/github', router); + +const upstream = (status, headers) => Object.assign( + new Error(`Request failed with status code ${status}`), + { response: { status, headers } }, +); + +describe('GET /status reports credential liveness, not just presence', () => { + const priorPat = process.env.GITHUB_PAT; + + beforeEach(() => { + jest.clearAllMocks(); + process.env.GITHUB_PAT = 'ghp_test_token'; + mockCurrentUser = { _id: 'user-1', role: 'admin' }; + }); + + afterAll(() => { + if (priorPat === undefined) delete process.env.GITHUB_PAT; + else process.env.GITHUB_PAT = priorPat; + }); + + it('reports a working credential as live', async () => { + axios.get.mockResolvedValue({ data: { rate: { remaining: 4999 } } }); + + const res = await request(app).get('/api/github/status'); + + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ + mode: 'pat', + configured: true, + credentialLive: true, + credentialStatus: 'accepted', + }); + }); + + it('probes /rate_limit, which GitHub does not charge against quota', async () => { + axios.get.mockResolvedValue({ data: {} }); + + await request(app).get('/api/github/status'); + + expect(axios.get).toHaveBeenCalledTimes(1); + expect(axios.get.mock.calls[0][0]).toBe('https://api.github.com/rate_limit'); + }); + + it('reports a rejected credential as NOT live while still answering 200', async () => { + axios.get.mockRejectedValue(upstream(401)); + + const res = await request(app).get('/api/github/status'); + + // The whole finding: `configured: true` alone said everything was fine. + expect(res.body.configured).toBe(true); + expect(res.body.credentialLive).toBe(false); + expect(res.body.credentialStatus).toBe('rejected'); + expect(res.body.upstreamStatus).toBe(401); + // Load-bearing: a diagnostic must not express its finding as its own + // failure status. 502 here would be indistinguishable from a broken + // diagnostic. + expect(res.status).toBe(200); + }); + + it('treats a bare 403 as credential rejection, same as 401', async () => { + // No rate-limit headers — this is GitHub refusing the credential, not + // throttling. It is the discriminating negative for the two tests below. + axios.get.mockRejectedValue(upstream(403)); + + const res = await request(app).get('/api/github/status'); + + expect(res.status).toBe(200); + expect(res.body.credentialLive).toBe(false); + expect(res.body.credentialStatus).toBe('rejected'); + expect(res.body.upstreamStatus).toBe(403); + }); + + // GitHub overloads 403: refused credential AND exhausted quota. Reporting a + // throttled-but-valid PAT as dead sends an operator to rotate a working + // credential — the exact outcome `live: null` exists to prevent, arriving + // through the branch the first cut of this probe did not guard (msg 52320). + it('does not call a rate-limited PAT dead — primary quota exhausted', async () => { + axios.get.mockRejectedValue(upstream(403, { 'x-ratelimit-remaining': '0' })); + + const res = await request(app).get('/api/github/status'); + + expect(res.status).toBe(200); + expect(res.body.credentialLive).toBeNull(); + expect(res.body.credentialStatus).toBe('rate_limited'); + expect(res.body.upstreamStatus).toBe(403); + }); + + it('does not call a rate-limited PAT dead — secondary limit (retry-after)', async () => { + // The likelier 403 on /rate_limit specifically: primary exhaustion does not + // throttle that endpoint, so a 403 there is more often abuse-detection. + axios.get.mockRejectedValue(upstream(403, { 'retry-after': '60' })); + + const res = await request(app).get('/api/github/status'); + + expect(res.body.credentialLive).toBeNull(); + expect(res.body.credentialStatus).toBe('rate_limited'); + }); + + it('reports a 429 as rate-limited, never as dead', async () => { + axios.get.mockRejectedValue(upstream(429)); + + const res = await request(app).get('/api/github/status'); + + expect(res.body.credentialLive).toBeNull(); + expect(res.body.credentialStatus).toBe('rate_limited'); + }); + + it('reports unreachable as unknown, never as dead', async () => { + axios.get.mockRejectedValue(Object.assign(new Error('connect ETIMEDOUT'), {})); + + const res = await request(app).get('/api/github/status'); + + expect(res.status).toBe(200); + // null, not false — `false` would send an operator to rotate a credential + // that may be perfectly good. + expect(res.body.credentialLive).toBeNull(); + expect(res.body.credentialStatus).toBe('unreachable'); + }); + + it('still refuses non-admins before probing anything', async () => { + mockCurrentUser = { _id: 'user-2', role: 'member' }; + + const res = await request(app).get('/api/github/status'); + + expect(res.status).toBe(403); + expect(axios.get).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js b/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js new file mode 100644 index 00000000..46636d55 --- /dev/null +++ b/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js @@ -0,0 +1,116 @@ +/** + * AX #9 maps GitHub's credential rejection to a non-retryable 502. The + * mapper unit tests prove the taxonomy; these mount the real router so every + * route that proxies GitHub is pinned to that taxonomy. + * + * Keep this explicit table when adding a GitHub proxy route. The request + * shapes are intentionally visible here: deriving cases from `router.stack` + * would hide the route-specific validation each request must pass before it + * reaches the upstream service. `/status` is excluded because it only reads + * local configuration and signs locally; it must keep its honest local 500. + */ + +jest.mock('../../../middleware/agentRuntimeAuth', () => (req, res, next) => { + req.agentUser = { _id: 'bot-1' }; + next(); +}); + +jest.mock('../../../middleware/auth', () => (req, res, next) => { + req.user = { _id: 'user-1', role: 'member' }; + req.userId = 'user-1'; + next(); +}); + +jest.mock('../../../services/githubAppService', () => ({ + isPatConfigured: jest.fn(), + isConfigured: jest.fn(), + getInstallationToken: jest.fn(), + listOpenIssues: jest.fn(), + createIssue: jest.fn(), + addIssueComment: jest.fn(), + closeIssue: jest.fn(), + getPullDiff: jest.fn(), + createPullReview: jest.fn(), +})); + +const express = require('express'); +const request = require('supertest'); +// The backend source is TypeScript, while the legacy ESLint resolver only +// discovers JavaScript module extensions. +// eslint-disable-next-line import/no-unresolved, import/extensions +const GitHubAppService = require('../../../services/githubAppService'); +// eslint-disable-next-line import/no-unresolved, import/extensions +const router = require('../../../routes/github'); + +const app = express(); +app.use(express.json()); +app.use('/api/github', router); + +const credentialRejected = { + message: 'Request failed with status code 401', + response: { status: 401 }, +}; +const CREDENTIAL_REJECTION_TITLE = '$name maps an upstream 401 to non-retryable credential guidance'; + +// Each row is a distinct route-level call site of mapGitHubUpstreamError. +// Adding another GitHub-proxy route means adding a row here with the smallest +// valid request that reaches its service boundary. +const PROXYING_ROUTE_CASES = [ + { + name: 'POST /token', + service: 'getInstallationToken', + send: (client) => client.post('/api/github/token'), + }, + { + name: 'GET /issues', + service: 'listOpenIssues', + send: (client) => client.get('/api/github/issues'), + }, + { + name: 'POST /issues', + service: 'createIssue', + send: (client) => client.post('/api/github/issues').send({ title: 'Test issue' }), + }, + { + name: 'POST /issues/:number/comment', + service: 'addIssueComment', + send: (client) => client.post('/api/github/issues/1/comment').send({ body: 'Test comment' }), + }, + { + name: 'POST /issues/:number/close', + service: 'closeIssue', + send: (client) => client.post('/api/github/issues/1/close'), + }, + { + name: 'GET /pulls/:number/diff', + service: 'getPullDiff', + send: (client) => client.get('/api/github/pulls/1/diff'), + }, + { + name: 'POST /pulls/:number/review', + service: 'createPullReview', + send: (client) => client.post('/api/github/pulls/1/review').send({ event: 'APPROVE' }), + }, +]; + +describe('GitHub proxy routes preserve upstream credential guidance (AX #9)', () => { + beforeEach(() => { + jest.clearAllMocks(); + GitHubAppService.isPatConfigured.mockReturnValue(false); + GitHubAppService.isConfigured.mockReturnValue(true); + }); + + test.each(PROXYING_ROUTE_CASES)(CREDENTIAL_REJECTION_TITLE, async ({ service, send }) => { + GitHubAppService[service].mockRejectedValue(credentialRejected); + + const res = await send(request(app)); + + expect(GitHubAppService[service]).toHaveBeenCalledTimes(1); + expect(res.status).toBe(502); + expect(res.body).toEqual(expect.objectContaining({ + code: 'github_credential_rejected', + upstreamStatus: 401, + retryable: false, + })); + }); +}); diff --git a/backend/__tests__/unit/routes/github.upstreamErrors.test.js b/backend/__tests__/unit/routes/github.upstreamErrors.test.js new file mode 100644 index 00000000..a827c82c --- /dev/null +++ b/backend/__tests__/unit/routes/github.upstreamErrors.test.js @@ -0,0 +1,108 @@ +// AX audit #9: `commonly_pr_diff` reported an upstream 401 as a 500. The two +// codes carry opposite instructions — 500 says retry, 401 says stop and fix +// the credential — so a caller doing the right thing by the status retried +// forever against a fault no retry resolves. The only true signal lived in a +// `detail` string nothing machine-readable reads. +// +// These pin the mapping. The assertion that matters in every case is +// `retryable`: it is the field a caller can branch on, and it is the thing the +// old shape got backwards. + +const { mapGitHubUpstreamError } = require('../../../routes/github'); + +const LABELS = { fallback: 'Failed to fetch pull diff', notFound: 'Pull request not found' }; + +// Shaped like a real axios error, since that is what the routes catch. +const upstream = (status, headers = {}) => ({ + message: `Request failed with status code ${status}`, + response: { status, headers }, +}); + +describe('mapGitHubUpstreamError', () => { + it('maps an upstream 401 to a non-retryable 502, not a 500', () => { + const { status, body } = mapGitHubUpstreamError(upstream(401), LABELS); + // The whole finding: this used to be 500, which instructs a retry. + expect(status).toBe(502); + expect(body.code).toBe('github_credential_rejected'); + expect(body.upstreamStatus).toBe(401); + expect(body.retryable).toBe(false); + // The upstream status survives in a machine-readable field rather than + // only inside the human-readable detail string. + expect(body.detail).toBe('Request failed with status code 401'); + }); + + it('reports a credential rejection as 502 and does not pass the 401 through', () => { + // A bare 401 would relocate the false model onto the caller's own token: + // the caller's auth is fine, it is our server credential GitHub refused. + // + // The positive assertion is load-bearing (@ux-lead, msg 52276): with only + // `not.toBe(401)` this test stayed green under the exact 502→500 mutation + // it reads like it guards, because a 500 isn't a 401 either. A test that + // pins what a value ISN'T has to pin what it IS, or it passes under the + // bug. + const { status, body } = mapGitHubUpstreamError(upstream(401), LABELS); + expect(status).toBe(502); + expect(status).not.toBe(401); + expect(String(body.error)).toMatch(/server credential/i); + }); + + it('maps a plain upstream 403 the same way (also a credential fault)', () => { + const { status, body } = mapGitHubUpstreamError(upstream(403), LABELS); + expect(status).toBe(502); + expect(body.code).toBe('github_credential_rejected'); + expect(body.retryable).toBe(false); + }); + + it('distinguishes a rate-limited 403 from a rejected credential', () => { + // GitHub overloads 403 for rate limiting; the remaining-budget header is + // the only thing that separates them, and they need opposite advice. + const { status, body } = mapGitHubUpstreamError( + upstream(403, { 'x-ratelimit-remaining': '0' }), + LABELS, + ); + expect(status).toBe(429); + expect(body.code).toBe('github_rate_limited'); + expect(body.retryable).toBe(true); + }); + + it('maps an upstream 429 to 429, retryable', () => { + const { status, body } = mapGitHubUpstreamError(upstream(429), LABELS); + expect(status).toBe(429); + expect(body.retryable).toBe(true); + }); + + it('maps a genuine upstream 5xx to a retryable 502', () => { + const { status, body } = mapGitHubUpstreamError(upstream(503), LABELS); + expect(status).toBe(502); + expect(body.code).toBe('github_upstream_error'); + expect(body.upstreamStatus).toBe(503); + // This one IS worth retrying — the flag has to move, or it is decorative. + expect(body.retryable).toBe(true); + }); + + it('keeps 404 as 404 and uses the caller-supplied noun', () => { + const { status, body } = mapGitHubUpstreamError(upstream(404), LABELS); + expect(status).toBe(404); + expect(body.error).toBe('Pull request not found'); + expect(body.retryable).toBe(false); + + const issue = mapGitHubUpstreamError(upstream(404), { fallback: 'x', notFound: 'Issue not found' }); + expect(issue.body.error).toBe('Issue not found'); + }); + + it('still returns 500 when there is no upstream response at all', () => { + // The one honest 500: our own bug, no GitHub verdict to report. + const { status, body } = mapGitHubUpstreamError(new Error('socket hang up'), LABELS); + expect(status).toBe(500); + expect(body.code).toBe('github_proxy_error'); + expect(body.error).toBe('Failed to fetch pull diff'); + expect(body.upstreamStatus).toBeUndefined(); + }); + + it('never reports a credential rejection as retryable, across every auth status', () => { + // The single invariant this file exists to defend. + [401, 403].forEach((s) => { + expect(mapGitHubUpstreamError(upstream(s), LABELS).body.retryable).toBe(false); + }); + }); +}); diff --git a/backend/routes/github.ts b/backend/routes/github.ts index c638b893..24c9395f 100644 --- a/backend/routes/github.ts +++ b/backend/routes/github.ts @@ -8,6 +8,10 @@ const agentRuntimeAuth = require('../middleware/agentRuntimeAuth'); const auth = require('../middleware/auth'); // eslint-disable-next-line global-require const GitHubAppService = require('../services/githubAppService'); +// Its own module, not a static on the service: route tests legitimately mock +// GitHubAppService wholesale, which would take this pure predicate with it. +// eslint-disable-next-line global-require +const { isRateLimitError } = require('../services/githubRateLimit'); interface AuthReq { user?: { role?: string }; @@ -40,6 +44,80 @@ const githubPrRateLimit = rateLimit({ }), }); +// AX audit #9. Every route below proxies GitHub, and every failure — including +// GitHub rejecting OUR credential — was collapsed into a 500 whose only true +// signal lived in a human-readable `detail` string. The two codes carry +// opposite instructions: 500 means *the server failed, retry*, while an +// upstream 401 means *stop, the credential is wrong, retrying changes +// nothing*. A caller that reads the status and does the right thing by it +// retries forever against a fault no retry resolves. +// +// So: map the upstream status into the same class, and put the instruction in +// a machine-readable field (`retryable`) rather than in prose. The status +// stays 502 for a credential rejection rather than passing 401 through, +// because the CALLER's auth is fine — it is our server credential GitHub +// refused, and a bare 401 would just relocate the false model onto the +// caller's own token. `code` + `upstreamStatus` say which of the two it is. +// NOTE: deliberately not `export function`. This file ends in +// `module.exports = router`, which replaces the exports object wholesale — a +// TS named export would compile to `exports.x = …` and then be silently +// discarded. It is re-attached to the router below instead, which is the +// shape that actually survives. +function mapGitHubUpstreamError( + err: unknown, + labels: { fallback: string; notFound: string }, +): { status: number; body: Record } { + const e = err as { + response?: { status?: number; headers?: Record }; + message?: string; + }; + const upstreamStatus = e.response?.status; + const detail = e.message; + + if (upstreamStatus === 404) { + return { status: 404, body: { error: labels.notFound, code: 'github_not_found', retryable: false } }; + } + + // GitHub signals rate limiting as 429, or as 403 with the remaining budget + // at zero. Both are retryable — but only after a wait, so say so. + // The predicate lives in the service so the liveness probe shares it exactly; + // when this test existed only here, the probe called every throttled PAT dead. + if (isRateLimitError(upstreamStatus, e.response?.headers)) { + return { + status: 429, + body: { + error: 'GitHub rate limit exceeded', code: 'github_rate_limited', upstreamStatus, retryable: true, detail, + }, + }; + } + + if (upstreamStatus === 401 || upstreamStatus === 403) { + return { + status: 502, + body: { + error: 'GitHub rejected the server credential — this is not your token, and retrying will not fix it', + code: 'github_credential_rejected', + upstreamStatus, + retryable: false, + detail, + }, + }; + } + + if (typeof upstreamStatus === 'number' && upstreamStatus >= 500) { + return { + status: 502, + body: { + error: 'GitHub is failing upstream', code: 'github_upstream_error', upstreamStatus, retryable: true, detail, + }, + }; + } + + // No upstream response at all: our own bug, our own 500. This is the only + // branch where 500 is the honest answer. + return { status: 500, body: { error: labels.fallback, code: 'github_proxy_error', retryable: false, detail } }; +} + function anyAuth(req: AuthReq, res: Res, next: () => void) { const token = ((req.header?.('Authorization') || '').replace('Bearer ', '')); if (token.startsWith('cm_agent_')) return agentRuntimeAuth(req, res, next); @@ -69,17 +147,54 @@ router.post('/token', agentRuntimeAuth, async (req: AuthReq, res: Res) => { const result = await GitHubAppService.getInstallationToken(installationId); return res.json(result); } catch (err) { - const e = err as { response?: { status?: number }; message?: string }; - const status = e.response?.status; - if (status === 404) return res.status(404).json({ message: 'GitHub App not installed on this repository' }); - return res.status(500).json({ message: 'Failed to generate GitHub token', error: e.message }); + // The seventh proxying route, and the one where the flattening bit + // hardest (@ux-lead, msg 52276): this endpoint's entire job is + // credentials, so the caller most likely to hit an upstream 401 here is + // someone ALREADY debugging a credential failure — and a 500 tells them to + // retry. `getInstallationIdForRepo` and `getInstallationToken` both call + // GitHub, so the same mapping applies. + const mapped = mapGitHubUpstreamError(err, { + fallback: 'Failed to generate GitHub token', + notFound: 'GitHub App not installed on this repository', + }); + // `message` is kept alongside the mapped body: this route has always + // answered with `message`, and CLI/driver callers read it. Additive, so + // nothing that parses the old shape breaks. + console.error('POST /github/token error:', mapped.body.code, mapped.body.detail); + return res.status(mapped.status).json({ ...mapped.body, message: mapped.body.error }); } }); +// Deliberately NOT mapped, and this is the ONE route where that is a hard rule +// rather than an observation. The mapper turns an upstream 401 into a 502; a +// diagnostic that does the same makes "the credential is dead" and "the +// diagnostic is broken" the same response — reintroducing, at the single +// endpoint whose job is to prevent it, exactly the collapse the mapper exists +// to eliminate. A diagnostic must never express its finding as its own failure +// status: a dead credential is a successful diagnosis. So this route answers +// 200 with `credentialLive: false` and keeps its local-fault 500 for genuine +// local faults only. (@ux-lead, msg 52286.) +// +// `configured` answers "is a credential present", which is what the four +// proxying gates need. It answered `true` all through the 2026-08-04 outage +// while every call 401'd — presence read as health, and it fails in the +// direction that ends the search: three seats were blocked, one concluded the +// fault was its own seat (msg 52256), and this endpoint would have agreed. +// `credentialLive` is the separate question, from the separate predicate. router.get('/status', auth, async (req: AuthReq, res: Res) => { try { if (req.user?.role !== 'admin') return res.status(403).json({ message: 'Admin only' }); - if (GitHubAppService.isPatConfigured()) return res.json({ mode: 'pat', configured: true }); + if (GitHubAppService.isPatConfigured()) { + const liveness = await GitHubAppService.checkPatLiveness(); + return res.json({ + mode: 'pat', + configured: true, + credentialLive: liveness.live, + credentialStatus: liveness.status, + ...(liveness.upstreamStatus ? { upstreamStatus: liveness.upstreamStatus } : {}), + ...(liveness.detail ? { detail: liveness.detail } : {}), + }); + } if (!GitHubAppService.isConfigured()) return res.json({ mode: 'none', configured: false, message: 'Set GITHUB_PAT or GitHub App env vars' }); const appJWT = GitHubAppService.generateAppJWT(); return res.json({ mode: 'app', configured: true, appId: process.env.GITHUB_APP_ID, installationId: process.env.GITHUB_APP_INSTALLATION_ID_COMMONLY, jwtGenerated: !!appJWT }); @@ -99,9 +214,9 @@ router.get('/issues', anyAuth, async (req: AuthReq, res: Res) => { const issues = await GitHubAppService.listOpenIssues({ owner, repo, perPage: Number(per_page) || 20 }); return res.json({ issues: issues.map((i: { number: number; title: string; body: string; html_url: string; labels?: Array<{ name: string }>; milestone?: { title?: string } }) => ({ number: i.number, title: i.title, body: i.body, url: i.html_url, labels: i.labels?.map((l) => l.name), milestone: i.milestone?.title || null })) }); } catch (err) { - const e = err as { message?: string }; - console.error('GET /github/issues error:', e.message); - return res.status(500).json({ error: 'Failed to list issues', detail: e.message }); + const mapped = mapGitHubUpstreamError(err, { fallback: 'Failed to list issues', notFound: 'Repository not found' }); + console.error('GET /github/issues error:', mapped.body.code, mapped.body.detail); + return res.status(mapped.status).json(mapped.body); } }); @@ -116,9 +231,9 @@ router.post('/issues', anyAuth, async (req: AuthReq, res: Res) => { const issue = await GitHubAppService.createIssue({ owner, repo, title, body, labels }); return res.status(201).json({ number: issue.number, title: issue.title, url: issue.html_url }); } catch (err) { - const e = err as { message?: string }; - console.error('POST /github/issues error:', e.message); - return res.status(500).json({ error: 'Failed to create issue', detail: e.message }); + const mapped = mapGitHubUpstreamError(err, { fallback: 'Failed to create issue', notFound: 'Repository not found' }); + console.error('POST /github/issues error:', mapped.body.code, mapped.body.detail); + return res.status(mapped.status).json(mapped.body); } }); @@ -130,9 +245,9 @@ router.post('/issues/:number/comment', anyAuth, async (req: AuthReq, res: Res) = await GitHubAppService.addIssueComment({ owner, repo, issueNumber, body }); return res.json({ ok: true }); } catch (err) { - const e = err as { message?: string }; - console.error('POST /github/issues/comment error:', e.message); - return res.status(500).json({ error: 'Failed to comment', detail: e.message }); + const mapped = mapGitHubUpstreamError(err, { fallback: 'Failed to comment', notFound: 'Issue not found' }); + console.error('POST /github/issues/comment error:', mapped.body.code, mapped.body.detail); + return res.status(mapped.status).json(mapped.body); } }); @@ -143,9 +258,9 @@ router.post('/issues/:number/close', anyAuth, async (req: AuthReq, res: Res) => await GitHubAppService.closeIssue({ owner, repo, issueNumber, comment }); return res.json({ ok: true, closed: issueNumber }); } catch (err) { - const e = err as { message?: string }; - console.error('POST /github/issues/close error:', e.message); - return res.status(500).json({ error: 'Failed to close issue', detail: e.message }); + const mapped = mapGitHubUpstreamError(err, { fallback: 'Failed to close issue', notFound: 'Issue not found' }); + console.error('POST /github/issues/close error:', mapped.body.code, mapped.body.detail); + return res.status(mapped.status).json(mapped.body); } }); @@ -163,10 +278,9 @@ router.get('/pulls/:number/diff', githubPrRateLimit, anyAuth, async (req: AuthRe const diff = await GitHubAppService.getPullDiff({ owner, repo, pullNumber }); return res.json({ number: pullNumber, diff }); } catch (err) { - const e = err as { response?: { status?: number }; message?: string }; - if (e.response?.status === 404) return res.status(404).json({ error: 'Pull request not found' }); - console.error('GET /github/pulls/diff error:', e.message); - return res.status(500).json({ error: 'Failed to fetch pull diff', detail: e.message }); + const mapped = mapGitHubUpstreamError(err, { fallback: 'Failed to fetch pull diff', notFound: 'Pull request not found' }); + console.error('GET /github/pulls/diff error:', mapped.body.code, mapped.body.detail); + return res.status(mapped.status).json(mapped.body); } }); @@ -185,13 +299,19 @@ router.post('/pulls/:number/review', githubPrRateLimit, anyAuth, async (req: Aut const r = review as { id?: number; state?: string; html_url?: string }; return res.status(201).json({ ok: true, id: r?.id, state: r?.state, url: r?.html_url }); } catch (err) { - const e = err as { response?: { status?: number }; message?: string }; - if (e.response?.status === 404) return res.status(404).json({ error: 'Pull request not found' }); - console.error('POST /github/pulls/review error:', e.message); - return res.status(500).json({ error: 'Failed to submit review', detail: e.message }); + // AX #9 left this one explicitly unverified — "whether commonly_pr_review + // shares the same broken credential; assume it does until someone checks." + // It does: both routes go through GitHubAppService._apiHeaders and the same + // GITHUB_PAT, so a rejected credential fails the write path identically. + const mapped = mapGitHubUpstreamError(err, { fallback: 'Failed to submit review', notFound: 'Pull request not found' }); + console.error('POST /github/pulls/review error:', mapped.body.code, mapped.body.detail); + return res.status(mapped.status).json(mapped.body); } }); module.exports = router; +// Exposed for unit tests: the mapping is the load-bearing part, and testing it +// through six routes' worth of axios mocks would test the mocks. +module.exports.mapGitHubUpstreamError = mapGitHubUpstreamError; export {}; diff --git a/backend/services/githubAppService.ts b/backend/services/githubAppService.ts index 48ee61ca..a9436b24 100644 --- a/backend/services/githubAppService.ts +++ b/backend/services/githubAppService.ts @@ -1,6 +1,31 @@ import jwt from 'jsonwebtoken'; import axios from 'axios'; +/** + * Result of a PAT liveness probe. `live: null` means "could not determine", + * which is a distinct answer from `false` and must stay distinct — see + * GitHubAppService.checkPatLiveness. + * + * `status` is canonical; `live` is a lossy projection of it for humans reading + * the JSON at a glance. Code should branch on `status`. `!live` merges + * `rejected` with `absent`, and `live === null` merges `unreachable` with + * `rate_limited` — three-into-two and two-into-one, so every `live`-based + * branch is a question answered less precisely than it was asked + * (@ux-lead, msg 52320, finding 3). + */ +export interface PatLiveness { + live: boolean | null; + status: 'absent' | 'accepted' | 'rejected' | 'rate_limited' | 'unreachable'; + upstreamStatus?: number; + detail?: string; +} + +// The 403-disambiguation predicate is shared with routes/github.ts and lives in +// its own module — see githubRateLimit.ts for why it is not a static on this +// class. +// eslint-disable-next-line @typescript-eslint/no-require-imports +const { isRateLimitError } = require('./githubRateLimit'); + export interface GitHubToken { token: string; expiresAt: string | null; @@ -129,11 +154,70 @@ class GitHubAppService { /** * Check whether a PAT is configured (simpler alternative to GitHub App). + * + * PRESENCE, not liveness — and that is correct for its four callers, which + * ask "should I even attempt this?" before proxying. Keep it free of I/O. + * If you need to know whether the credential still WORKS, use + * `checkPatLiveness` instead; see the note there for why these are two + * predicates rather than one corrected one. */ static isPatConfigured(): boolean { return !!process.env.GITHUB_PAT; } + /** + * Liveness, not presence: does GitHub still accept the configured PAT? + * + * A second predicate rather than a fix to `isPatConfigured` (@ux-lead, msg + * 52286). Those six call sites ask two different questions: four ask + * "should I attempt this?", where presence is the right test and a network + * round-trip on every proxying request would be the wrong price; `/status` + * asks "is it working?", which only GitHub can answer. One constant serving + * two jobs is how the answer ends up wrong for one of them. + * + * `/rate_limit` is the probe because GitHub documents it as not counting + * against the rate limit, so a diagnostic may call it freely. A 401 there is + * precisely the signal wanted: present, well-formed, and refused. + * + * `live: null` is deliberate and load-bearing. If GitHub is unreachable we + * do not know, and reporting `false` would send an operator to rotate a + * working credential — a diagnostic that guesses in the confident direction + * is the defect this method exists to remove, not a smaller version of it. + * Throttling gets the same treatment for the same reason: a 403 that means + * "slow down" is not a dead credential, and `isRateLimitError` is what keeps + * this branch from asserting one. + * + * SCOPE LIMIT, and it is not academic: `accepted` proves GitHub *recognises* + * the token, not that the token is *authorized* for what the seven proxying + * routes do. `/rate_limit` is not scope-gated, so a PAT regenerated without + * `repo` scope, or without SSO re-authorization, returns 200 here and 403 on + * every real call. That is this method's own version of the bug it fixes — + * the verdict is narrower than the name (@ux-lead, msg 52320). Unmeasured: + * settle it at the next rotation by probing this endpoint and one real repo + * read with the new token and comparing. + */ + static async checkPatLiveness(timeoutMs = 5000): Promise { + if (!this.isPatConfigured()) return { live: false, status: 'absent' }; + try { + const headers = await this._apiHeaders(); + await axios.get('https://api.github.com/rate_limit', { headers, timeout: timeoutMs }); + return { live: true, status: 'accepted' }; + } catch (err) { + const e = err as { + response?: { status?: number; headers?: Record }; + message?: string; + }; + const upstreamStatus = e.response?.status; + if (isRateLimitError(upstreamStatus, e.response?.headers)) { + return { live: null, status: 'rate_limited', upstreamStatus, detail: e.message }; + } + if (upstreamStatus === 401 || upstreamStatus === 403) { + return { live: false, status: 'rejected', upstreamStatus }; + } + return { live: null, status: 'unreachable', detail: e.message }; + } + } + /** * Return the PAT directly as a token response. * PATs don't have a server-issued expiry, so expiresAt is null. diff --git a/backend/services/githubRateLimit.ts b/backend/services/githubRateLimit.ts new file mode 100644 index 00000000..2f4c71a8 --- /dev/null +++ b/backend/services/githubRateLimit.ts @@ -0,0 +1,39 @@ +/** + * Is this upstream error GitHub throttling us, rather than refusing our + * credential? GitHub overloads 403 for both, so the status code alone cannot + * tell them apart — the headers can. + * + * Two callers ask this identical question and must never answer it + * differently: `mapGitHubUpstreamError` (routes/github.ts), which decides + * 429-retryable vs 502-not-your-fault, and `GitHubAppService.checkPatLiveness`, + * which decides `live: null` vs `live: false`. When only the route had the + * header test, the liveness probe reported every throttled-but-valid PAT as + * dead — sending an operator to rotate a working credential, which is the exact + * outcome that probe's tri-state exists to prevent (@ux-lead, msg 52320). + * + * It lives in its own module rather than on GitHubAppService deliberately. + * Parked on the service, it disappeared whenever a test mocked the service — + * which is ordinary and correct for route tests — taking a piece of pure + * routing logic with it and turning the mapper's 502 into a 500. A predicate + * over an HTTP status and a header bag has no business being reachable only + * through a stateful service object. + * + * `x-ratelimit-remaining: '0'` is primary-quota exhaustion. `retry-after` is + * the secondary (abuse-detection) limit — the likelier 403 on `/rate_limit` + * specifically, since primary exhaustion does not throttle that endpoint at + * all. Documented, not measured: the shared PAT was dead while this was + * written, so neither header has been observed from real GitHub here. + */ +export function isRateLimitError( + upstreamStatus: number | undefined, + headers: Record | undefined, +): boolean { + if (upstreamStatus === 429) return true; + if (upstreamStatus !== 403) return false; + return headers?.['x-ratelimit-remaining'] === '0' || headers?.['retry-after'] !== undefined; +} + +export default isRateLimitError; +// CJS compat: let require() return the named export bag, matching sibling services. +// eslint-disable-next-line @typescript-eslint/no-require-imports +module.exports = exports["default"]; Object.assign(module.exports, exports);