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..5f23cc54 --- /dev/null +++ b/backend/__tests__/unit/routes/github.upstreamErrorRoutes.test.js @@ -0,0 +1,149 @@ +/** + * 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. The source-level check below counts every + * handler that calls GitHubAppService, not merely every mapper call, so an + * unmapped proxy route cannot make both sides of the assertion move together. + * `/status` carries the single explicit exemption 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 fs = require('fs'); +const path = require('path'); +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'; +const githubRouteSource = fs.readFileSync(path.join(__dirname, '../../../routes/github.ts'), 'utf8'); + +const githubRouteHandlers = (source) => { + const starts = [...source.matchAll(/router\.(?:get|post)\(/g)].map((match) => match.index); + + return starts.map((start, index) => { + const handler = source.slice(start, starts[index + 1]); + + return { + touchesGitHubService: handler.includes('GitHubAppService.'), + mapsUpstreamError: handler.includes('mapGitHubUpstreamError('), + upstreamExempt: handler.includes('@github-upstream-exempt'), + }; + }); +}; + +// 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'), + assertResponse: (res) => expect(res.body.message).toBe(res.body.error), + }, + { + 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); + }); + + it('maps every GitHub service handler unless it declares a local-only exemption', () => { + const handlers = githubRouteHandlers(githubRouteSource); + const githubServiceHandlers = handlers.filter((handler) => handler.touchesGitHubService); + const exemptHandlers = githubServiceHandlers.filter((handler) => handler.upstreamExempt); + const mappedHandlers = githubServiceHandlers.filter((handler) => handler.mapsUpstreamError); + + expect(exemptHandlers.every((handler) => !handler.mapsUpstreamError)).toBe(true); + expect(githubServiceHandlers.length - exemptHandlers.length).toBe(mappedHandlers.length); + expect(PROXYING_ROUTE_CASES).toHaveLength(mappedHandlers.length); + }); + + test.each(PROXYING_ROUTE_CASES)(CREDENTIAL_REJECTION_TITLE, async ({ service, send, assertResponse }) => { + 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, + })); + if (assertResponse) assertResponse(res); + }); +}); 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..49a9ece4 --- /dev/null +++ b/backend/__tests__/unit/routes/github.upstreamErrors.test.js @@ -0,0 +1,123 @@ +// 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); + }); + }); + + it('carries detail on every branch, including 404', () => { + // Every call site logs `mapped.body.detail`. The 404 branch used to omit + // it, so the commonest failure logged `github_not_found undefined` + // (@sprint-review) — the diagnostic went blank exactly where it is read + // most. Asserted across the whole taxonomy rather than on 404 alone, so a + // future branch cannot reintroduce the hole somewhere else. + [404, 429, 401, 403, 500, 503].forEach((s) => { + const { body } = mapGitHubUpstreamError(upstream(s), LABELS); + expect(body.detail).toBe(`Request failed with status code ${s}`); + }); + // The no-upstream-response case has only our own message to report. + expect(mapGitHubUpstreamError(new Error('socket hang up'), LABELS).body.detail) + .toBe('socket hang up'); + }); +}); diff --git a/backend/routes/github.ts b/backend/routes/github.ts index c638b893..1677985d 100644 --- a/backend/routes/github.ts +++ b/backend/routes/github.ts @@ -40,6 +40,88 @@ 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; + + // `detail` rides on every branch including this one. Omitting it here made + // the callers' log line read `github_not_found undefined` on the commonest + // failure there is (@sprint-review) — a diagnostic that goes blank exactly + // where it is most often read. + if (upstreamStatus === 404) { + return { + status: 404, + body: { + error: labels.notFound, code: 'github_not_found', retryable: false, detail, + }, + }; + } + + // 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. + const remaining = e.response?.headers?.['x-ratelimit-remaining']; + if (upstreamStatus === 429 || (upstreamStatus === 403 && remaining === '0')) { + 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,14 +151,36 @@ 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 — so that key + // keeps its meaning. `error` does NOT: on main this route alone put the + // raw upstream text in `error`, and it now carries the human label while + // the raw text moves to `detail`. Not additive, and worth stating plainly + // (@sprint-review) — the previous comment said "additive", which is the + // sentence someone would cite the next time they touch this file. + // The change is right for a different reason than backwards compatibility: + // the other six routes already answered `{error: