fix(api): map upstream GitHub 401s to 400/502 UPSTREAM_AUTH_FAILED (#734)

[frankbria]

Summary

Implements #734: [P1.7] Don't return upstream GitHub 401s as this API's 401 (forces web-UI logout)

A bad or revoked GitHub PAT previously surfaced as this API's own 401. The web UI's axios interceptor treats any 401 as CodeFRAME session expiry (clearToken() + redirect to /login), so entering an invalid PAT — or having a stored one revoked — logged the user out of CodeFRAME itself.

• New ErrorCodes.UPSTREAM_AUTH_FAILED (codeframe/ui/response_models.py), documented as never carried on a 401.
• github_integrations_v2.py: connect maps InvalidTokenError → 400 UPSTREAM_AUTH_FAILED (the PAT is bad client input in this request); get_issues and _map_github_error (import path) map it → 502 UPSTREAM_AUTH_FAILED (the stored credential died upstream).
• pr_v2.py: new shared _github_error_http() helper used at all 8 former status_code=e.status_code sites — a GitHub 401 is rewritten to 502 UPSTREAM_AUTH_FAILED; every other upstream status still propagates verbatim (404/405 special cases untouched).

Acceptance Criteria

• Upstream-credential failures map to a distinct code (400/502 UPSTREAM_AUTH_FAILED), never raw 401 on authenticated routes.
• (Optional) Web UI scopes its logout redirect to a specific auth-expiry code — deliberately skipped, see Known Limitations.

Test Plan

• Unit tests written first (TDD): connect → 400, issues/import → 502, plus a parametrized suite hitting all 6 previously-uncovered pr_v2 endpoints with a GitHub 401 (tests/ui/test_pr_v2_upstream_auth.py)
• Full tests/ui/ suite passing (517 tests) + CI subset (tests/ --ignore=tests/e2e -m "not lifecycle")
• Diff coverage 92% on changed lines (≥85% gate, diff-cover)
• ruff check clean
• Internal code review (advisory): no Critical/Major findings; confirmed no remaining 401-leak paths in codeframe/ui/
• Cross-family review pass: codex review — "consistently remap upstream GitHub 401/InvalidTokenError cases to non-401 responses… no correctness or security regression"
• Mutation sanity check: disabling the helper's 401 branch and swapping the error code each made the new tests fail

Known Limitations / Intentionally Deferred

• The optional web-UI hardening (scoping the logout redirect to a specific auth-expiry error code) is skipped: CodeFRAME's own auth 401s (JWT expiry, stream tickets) don't carry a structured error code, so scoping the interceptor risks breaking the real re-auth path. The backend now guarantees no upstream 401s reach the client, which removes the bug.
• cf CLI (pr_commands.py) still prints raw GitHub status codes to the console — the CLI has no logout-on-401 behavior, so this is cosmetic and out of scope.
• The 400-vs-502 split for the same InvalidTokenError is intentional: 400 when the bad PAT arrives in the request body (connect), 502 when a previously-stored PAT is rejected upstream.

Implementation Notes

Plan was self-authored (no plan comment on the issue); no deviations.

Closes #734

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 27 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 03ae197d-672a-4b1a-b667-9bec04340f9e

📥 Commits

Reviewing files that changed from the base of the PR and between 69358e8 and 1de373d.

📒 Files selected for processing (7)

• CLAUDE.md
• codeframe/ui/response_models.py
• codeframe/ui/routers/github_integrations_v2.py
• codeframe/ui/routers/pr_v2.py
• tests/ui/test_github_integrations_v2.py
• tests/ui/test_pr_status.py
• tests/ui/test_pr_v2_upstream_auth.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/734-upstream-github-401

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Code Review: fix(api) upstream GitHub 401s to 400/502 UPSTREAM_AUTH_FAILED

Overview

Well-targeted, semantically clean fix. The upstream 401 leak was a real UX hazard and the fix is confined to exactly the right layers. The 400-vs-502 split for InvalidTokenError (bad PAT in request body vs. stored PAT rejected upstream) is the correct semantic choice and is clearly explained in comments.

Strengths

• _github_error_http() helper in pr_v2.py cleanly eliminates 8 repetitive raise HTTPException blocks in one go. Clean DRY improvement.
• UPSTREAM_AUTH_FAILED in response_models.py has a comment stating it is never carried on a 401, documenting the invariant right where future readers will look.
• 400 for connect / 502 for issues+import is semantically correct and well-commented.
• Test naming documents the contract precisely. Good TDD discipline shown by the mutation sanity check in the PR description.
• pytestmark = pytest.mark.v2 on the new test file follows project conventions.

One Test Coverage Gap

The parametrized suite in test_pr_v2_upstream_auth.py covers 6 endpoints, and get_pr_status is covered separately in test_pr_status.py. But get_pr_history (GET /api/v2/pr/history, line 399 in pr_v2.py) also uses _github_error_http(e) and has no 401-leak test.

The PR says "all 6 previously-uncovered pr_v2 endpoints" -- if get_pr_history was intended as the 7th and was missed, worth adding one parametrized case:

("GET", "/api/v2/pr/history", None),

This is the only functional gap I found.

Minor Notes (non-blocking)

• _github_error_http docstring: "Other statuses propagate verbatim" is slightly ambiguous -- status codes propagate verbatim but the error code shifts to EXECUTION_FAILED. Not a regression, just worth a clarifying word if this function is ever read in isolation.
• _mock_get_issue in the import test: confirm the mock targets the same call site the import handler uses internally (github_issues_service.get_issue), not a different layer. If it does, the test is airtight.

Summary

The fix is correct, the helper extraction is clean, and the test suite is solid except for the missing /api/v2/pr/history parametrized case. Adding that entry would complete the coverage consistent with the PR's stated goal.

Verdict: Approve pending the get_pr_history test gap or an explanation of why it is excluded.

[claude]

Follow-up to my prior review: the diff is unchanged, so my prior feedback still stands.

Only outstanding item: get_pr_history (the endpoint at /api/v2/pr/history) uses _github_error_http(e) but has no 401-leak test in either test_pr_v2_upstream_auth.py or test_pr_status.py. Adding one parametrized entry closes the gap and makes the coverage consistent with the PR's stated goal of covering all pr_v2 endpoints.

Everything else — 400/502 semantic split, DRY helper, test naming, pytestmark = pytest.mark.v2 — remains solid. Ready to approve once this gap is addressed or explained.

[claude]

Correction to my prior two reviews.

Both previous comments flagged get_pr_history (GET /api/v2/pr/history) as a missing test case. That was wrong — it is line 2 of the parametrized suite in test_pr_v2_upstream_auth.py:

("GET", "/api/v2/pr/history", None),

All 8 _github_error_http call sites are covered: 7 in the parametrized suite and get_pr_status in test_pr_status.py. There is no coverage gap.

The PR is clean. The 400/502 semantic split, the DRY helper, the UPSTREAM_AUTH_FAILED constant with its anti-regression comment, and the test suite are all correct and complete. Approving.

[frankbria]

Final Triage Summary (PR #808)

Cutoff: 2026-07-04T17:03:58Z (last push before triage) → updated 17:11:23Z after fix push

New findings since PR open: 3 (all from claude-review bot)

Fixed

• [claude] Missing 401-remap regression test for GET /api/v2/pr/history → fixed in 1de373d (added parametrized case; mutation-checked)
• [claude] _github_error_http docstring ambiguity re: error code on non-401 statuses → clarified in 1de373d

Skipped (with justification)

• [claude] "confirm _mock_get_issue targets the handler's call site" — verified, no change needed: the test patches github_integrations_v2.get_issue, which is exactly the name import_issues calls.

Notes

• claude-review posted a follow-up at 17:12:24Z confirming all 8 _github_error_http call sites are covered and approving.
• CodeRabbit App review was rate-limited this window (its PR check reports pass with no findings). Cross-family independent review was performed pre-PR via codex review — clean, no findings.