Opus finding
isTransientError always returns true — non-transient errors are retried, burning attempts and LLM budget
highbughigh
- apps/web/lib/review-worker.ts:268-282
- apps/web/lib/review-worker.ts:247-282
The function's documentation says non-transient errors (own bugs, 4xx other than 429) should not be retried because retrying won't help and would burn LLM budget. However, the function unconditionally returns true at the end after the (incomplete) heuristics. This means any TypeError, ReferenceError, 400, 401, 403, 404, 422 from the GitHub API, validation error in code, etc. is classified as transient and triggers retries up to MAX_PROCESSING_ATTEMPTS (6) attempts, costing ~62 minutes of backoff per failure plus 6× the cost of the LLM call (since reviewPR can be invoked again each retry). The tests in review-worker.test.ts only exercise positive cases of isTransientError; they never assert that a deterministic non-retryable error returns false, so this lying default is unguarded. The comment block above the bare `return true;` actively misleads readers: it says reviewPR-pipeline throws are 'almost certainly infrastructure' — but ANY error (even from getChangedFiles, updateReview, postPRComment, recordFindingStatuses, getInstallationToken, runFirstReviewSummary, formatPRComment) is rolled into this default-true.
Recommendation
Default to `return false;` after the explicit transient signal checks. Optionally add an allow-list for known 4xx-permanent codes (400/401/403/404/422) to be explicit. Update tests to cover the false case.