fix(github): stop reporting GitHub's verdict as our own fault (AX #9)

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);
+  });
+});

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');
+  });
+});