fix(github): retry an evidence-fetch failure on any surface, not just comments (BLO-28968)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• When a reviewer run exhausts its retries without posting a review, the commit-status delivery outbox posts a review/ally-complete failure status — but first checks GitHub for reviewer evidence, so a run that did review is not falsely failed
• That evidence check reads two surfaces in order: formal reviews, then comment-shaped reviews. classifyReviewerEvidenceError decides whether a failed read retries or is dropped permanently
• BLO-28920 made comments_* errors retryable, on the correct grounds that a permission/404 blip on an evidence surface is not evidence about the PR. reviews_* kept being classified by HTTP status, so a bare 403/404/401 there hit failPermanentDelivery
• That is the wrong way round: a reviews_* failure means the predicate read nothing — strictly less information than a comments_* failure, which by construction only happens after reviews has already been read conclusively. The surface we knew least about was the one that failed permanently
• Dropping the delivery is a stall, not a lost log line: the required context is never posted, and a required context with no status reads "Expected — waiting for status", blocking the PR until a human intervenes
• This pull request states the rule once as the default — permanent is the enumerated exception — so no surface, present or future, can inherit permanent failure by omission
• The benefit is that a transient GitHub permission blip costs a few retries instead of a wedged PR and a human

Linked Issues or Issue Description

Fixes: BLO-28968 (https://paperclip.blockcast.net/BLO/issues/BLO-28968)
Refs BLO-28920 — #1422, which established the invariant for comments_*; this PR generalises it. The defect is pre-existing and was found during pre-landing review of that PR, deliberately not bundled into it.

No duplicate or related open PR: searched Blockcast/paperclip open PRs for classifyReviewer, evidence, outbox, reviews_http, retryable, and 28968 — the only outbox-adjacent PRs are #1321 (plugin outbox FIFO) and #1243 (comment durable effects), neither touching this classifier.

What Changed

• server/src/services/github-status-delivery-outbox.ts — classifyReviewerEvidenceError now returns retryable: true for every error except the two enumerated configuration faults (no_bot_login; no_token with credentials absent). Drops the per-surface startsWith("comments_") carve-out and the HTTP-status branch.
• Same file — removes the now-unreachable file-local isRetryableGithubHttpStatus. (The identically-named helper in github-app-auth.ts is untouched and still classifies HTTP failures at the fetch layer, where a status is the right signal.)
• Same file — adds a test-only alias export _classifyReviewerEvidenceError, following the existing _resetInstallationTokenCache convention, so the rule can be asserted directly rather than only through a delivery round-trip.
• server/src/__tests__/github-status-delivery-outbox.test.ts — six unit tests pinning the rule (3 retryable reviews_http_*, 3 unchanged config outcomes) in a new database-free describe, plus one delivery-level test that a reviews 404 leaves the row queued with nextAttemptAt set.

Behaviour delta is exactly the intended one. Walking every string githubHasReviewerEvidenceForPr can return: no_bot_login, no_token, bot_login_not_app_form, reviews_rate_limited, reviews_pagination_exhausted, reviews_fetch_failed, comments_*, and retryable-status reviews_http_{408,409,429,5xx} are all unchanged. Only non-retryable-status reviews_http_* (403 without a rate-limit signal, 404, 401) changes — from permanent drop to bounded retry.

Verification

pnpm exec vitest run server/src/__tests__/github-status-delivery-outbox.test.ts — 23 passed (23), embedded Postgres available so the delivery-level test really ran:

✓ reviewer-evidence error classification > retries `reviews_http_403` rather than permanently dropping the gate-status delivery
✓ reviewer-evidence error classification > retries `reviews_http_404` rather than permanently dropping the gate-status delivery
✓ reviewer-evidence error classification > retries `reviews_http_401` rather than permanently dropping the gate-status delivery
✓ reviewer-evidence error classification > keeps a missing reviewer bot login permanent
✓ reviewer-evidence error classification > keeps an absent GitHub App credential set permanent
✓ reviewer-evidence error classification > retries a token blip when the credentials are configured
✓ GitHub commit-status delivery outbox > keeps the delivery retryable when the reviews surface 404s
✓ GitHub commit-status delivery outbox > marks permission failures as permanent instead of retrying forever
✓ GitHub commit-status delivery outbox > surfaces missing GitHub App credentials as permanent configuration failure

Negative control — the same tests against the pre-fix classifier (source reverted, test-only export kept) fail 4/23, proving they bite rather than passing vacuously:

Tests  4 failed | 19 passed (23)

FAIL > retries `reviews_http_403` ...   FAIL > retries `reviews_http_404` ...   FAIL > retries `reviews_http_401` ...
FAIL > keeps the delivery retryable when the reviews surface 404s
  - Expected                             + Received
  -   "attempts": 1,                     +   "attempts": 0,
  -   "lastErrorKind": "transient",       +   "lastErrorKind": "permanent",
  -   "status": "queued",                 +   "status": "failed_permanent",
      "lastError": "reviewer_evidence_reviews_http_404",

The three config-behaviour tests passed before the fix too — that is the point of including them: they guard AC3 against regression rather than asserting the change.

Also green: npx tsc --noEmit -p server/tsconfig.json (exit 0) and node scripts/check-test-undefined-symbols.mjs ("ok no undefined identifiers in server tests").

Risks

Low, and asymmetric in the safe direction.

• Retry cost, bounded. The only new retries are 4xx reads that previously dropped immediately. MAX_ATTEMPTS is 5 with 30s/2m/10m/30m/2h backoff, so the worst case for a genuinely persistent 403 is 5 evidence reads over ~2.7h, then a terminal failed — no unbounded loop, and no new external writes (the evidence read is a GET).
• Terminal outcome for a persistent fault changes label, failed_permanent → failed. Both are terminal and neither posts the status, so the PR-facing behaviour is identical; only lastErrorKind differs in the delivery row and its run event.
• Deliberately out of scope: handleFreshCommitStatusIfPresent still routes a non-retryable commit-status-read failure to failPermanentDelivery via githubGetLatestCommitStatusForContext. That is a different surface answering a different question, and BLO-28968 scopes itself to the evidence classifier. Worth a look separately; not bundled here, for the same reason this was not bundled into fix(review): accept COMMENTED exact-head App review as run-output attestation (BLO-28920) #1422.
• bot_login_not_app_form remains retryable. It is arguably a permanent misconfiguration, but it was retryable before this change too, so leaving it is the status quo and the fail-safe direction. Widening the permanent set is exactly the hazard this PR removes.

Model Used

Claude Opus 4.5 (claude-opus-5[1m], 1M context) with extended thinking, via Claude Code with tool use — driven by the Paperclip CTO agent.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, server-only
• I have updated relevant documentation to reflect my changes — the invariant is documented as the function's doc comment, which is where it has to hold
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-28968
🔗 Paperclip issue: BLO-28920

[allyblockcast]

🔗 Paperclip issue: BLO-28968
🔗 Paperclip issue: BLO-28920

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d745681

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The classifier now expresses the fail-safe invariant once: evidence-fetch failures retry by default, while the two explicit configuration faults remain permanent.
• The direct unit tests cover the changed HTTP cases, credential configuration behavior, and the delivery-level 404 retry path.
• The existing bounded retry policy prevents a persistent GitHub failure from creating an unbounded loop.

Recommended Action

1. Merge when the remaining CI and repository checks are green.