From 9b085bcf747a3aecf60f1b928a4e0e7ab13a29ab Mon Sep 17 00:00:00 2001 From: St0rmz1 Date: Mon, 27 Jul 2026 11:20:12 -0700 Subject: [PATCH 1/2] feat(code-review): tell customers when their own provider key is rate limited --- .../[reviewId]/CodeReviewDetailClient.tsx | 25 +++++-- .../[reviewId]/route.test.ts | 65 +++++++++++++++++++ .../code-review-status/[reviewId]/route.ts | 27 ++++++-- .../code-reviews/CodeReviewJobsCard.tsx | 25 +++++-- .../code-reviews/terminal-reason-copy.test.ts | 45 +++++++++++++ .../lib/code-reviews/terminal-reason-copy.ts | 62 ++++++++++++++++++ .../src/routers/admin-code-reviews-router.ts | 6 +- 7 files changed, 237 insertions(+), 18 deletions(-) create mode 100644 apps/web/src/lib/code-reviews/terminal-reason-copy.test.ts create mode 100644 apps/web/src/lib/code-reviews/terminal-reason-copy.ts diff --git a/apps/web/src/app/(app)/code-reviews/[reviewId]/CodeReviewDetailClient.tsx b/apps/web/src/app/(app)/code-reviews/[reviewId]/CodeReviewDetailClient.tsx index ab72458138..8d517e502c 100644 --- a/apps/web/src/app/(app)/code-reviews/[reviewId]/CodeReviewDetailClient.tsx +++ b/apps/web/src/app/(app)/code-reviews/[reviewId]/CodeReviewDetailClient.tsx @@ -8,6 +8,7 @@ import { CodeReviewStreamView } from '@/components/code-reviews/CodeReviewStream import { CouncilGovernancePanel } from '@/components/code-reviews/CouncilGovernancePanel'; import { formatTokenCount } from '@/lib/code-reviews/summary/usage-footer'; import { getCodeReviewJobsHref } from '@/lib/code-reviews/code-review-links'; +import { getCodeReviewTerminalReasonCopy } from '@/lib/code-reviews/terminal-reason-copy'; import { ExternalLink, GitPullRequest, Loader2, ArrowLeft, RotateCcw, Ban } from 'lucide-react'; import { getCodeReviewStatusIcon } from '@/components/code-reviews/code-review-status-icons'; import { useTRPC } from '@/lib/trpc/utils'; @@ -118,15 +119,25 @@ export function CodeReviewDetailClient({ reviewId }: CodeReviewDetailClientProps status === 'cancelled' && (review.terminal_reason === 'superseded' || review.error_message?.toLowerCase().includes('superseded')); - const reviewMessage = review.error_message + // Reasons with customer-facing copy render muted rather than destructive: the + // cause is stated plainly and there is nothing broken on our side to alarm + // about. Checked before the raw error_message so the friendlier text wins. + const terminalReasonCopy = getCodeReviewTerminalReasonCopy(review.terminal_reason); + const reviewMessage = terminalReasonCopy ? { - label: isSupersededCancellation ? 'Cancelled' : 'Error', - message: isSupersededCancellation ? 'Superseded by a newer push.' : review.error_message, - className: isSupersededCancellation - ? 'border-border bg-muted/30 text-muted-foreground' - : 'border-destructive/30 bg-destructive/10 text-destructive', + label: terminalReasonCopy.label, + message: terminalReasonCopy.message, + className: 'border-border bg-muted/30 text-muted-foreground', } - : null; + : review.error_message + ? { + label: isSupersededCancellation ? 'Cancelled' : 'Error', + message: isSupersededCancellation ? 'Superseded by a newer push.' : review.error_message, + className: isSupersededCancellation + ? 'border-border bg-muted/30 text-muted-foreground' + : 'border-destructive/30 bg-destructive/10 text-destructive', + } + : null; return ( {/* Back link */} diff --git a/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.test.ts b/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.test.ts index 35ceda92c2..7fcd444da0 100644 --- a/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.test.ts +++ b/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.test.ts @@ -2953,6 +2953,71 @@ describe('POST /api/internal/code-review-status/[reviewId]', () => { ); }); + // Notification only: the customer is told the cause, but the check stays a + // plain failure and Code Reviewer is not disabled. A provider rate limit is + // transient and there is nothing to reconfigure. + it('names the customer key for a byok rate limit without requiring action', async () => { + mockGetCodeReviewById.mockResolvedValue(makeReview()); + + await POST( + makeRequest({ + status: 'failed', + errorMessage: 'Assistant request was rate limited', + terminalReason: 'assistant_rate_limited_byok', + }), + makeParams(REVIEW_ID) + ); + + expect(mockUpdateCheckRun).toHaveBeenCalledWith( + 'inst-1', + 'owner', + 'repo', + 12345, + expect.objectContaining({ + status: 'completed', + conclusion: 'failure', + output: expect.objectContaining({ + title: 'Kilo Code Review rate limited', + summary: 'Your provider API key hit its rate limit.', + }), + }), + 'standard' + ); + }); + + // A customer's exhausted quota will still be exhausted a moment later, so + // retrying burns a second review against the same closed door. + it('does not auto-retry a byok rate limit', async () => { + mockGetCodeReviewById.mockResolvedValue(makeReview()); + + await POST( + makeRequest({ + status: 'failed', + errorMessage: 'Assistant request was rate limited', + terminalReason: 'assistant_rate_limited_byok', + }), + makeParams(REVIEW_ID) + ); + + expect(mockCreateInfraRetryAttemptIfMissing).not.toHaveBeenCalled(); + }); + + // Our own capacity can free up, so this one stays retryable. + it('still auto-retries a managed key rate limit', async () => { + mockGetCodeReviewById.mockResolvedValue(makeReview()); + + await POST( + makeRequest({ + status: 'failed', + errorMessage: 'Assistant request was rate limited', + terminalReason: 'assistant_rate_limited_managed', + }), + makeParams(REVIEW_ID) + ); + + expect(mockCreateInfraRetryAttemptIfMissing).toHaveBeenCalled(); + }); + it('uses failure conclusion for non-billing failures', async () => { mockGetCodeReviewById.mockResolvedValue(makeReview()); diff --git a/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.ts b/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.ts index 171acdbbf3..299feb216b 100644 --- a/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.ts +++ b/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.ts @@ -62,6 +62,7 @@ import { verifyCallbackToken } from '@kilocode/worker-utils/callback-token'; import { PLATFORM } from '@/lib/integrations/core/constants'; import { appendPreviousReviewSummaryHistory } from '@/lib/code-reviews/summary/history'; import { terminalReasonFromCloudAgentFailure } from '@/lib/code-reviews/terminal-reason-from-failure'; +import { getCodeReviewTerminalReasonCopy } from '@/lib/code-reviews/terminal-reason-copy'; import { appendReviewSummaryFooter, buildReviewSummaryFooter, @@ -482,6 +483,14 @@ function hasKnownUnretryableTerminalReason(terminalReason?: CodeReviewTerminalRe terminalReason === 'user_cancelled' || terminalReason === 'superseded' || terminalReason === 'interrupted' || + // The customer's own provider quota is exhausted, so an immediate retry + // burns a second review against the same closed door. hasKnownUnretryableFailureMessage + // below already tries to catch this, but only by matching the raw '[BYOK] Your + // API key has hit its rate limit...' text, which safe-failure projection + // replaces with 'Assistant request was rate limited' before the callback ever + // sees it. That check has therefore never fired; the structured reason makes + // the existing intent actually work. + terminalReason === 'assistant_rate_limited_byok' || isCodeReviewActionRequiredReason(terminalReason) ); } @@ -777,6 +786,13 @@ function mapStatusToCheckRun( const actionRequiredCopy = actionRequiredReason ? getCodeReviewActionRequiredCopy(actionRequiredReason) : null; + // Notification only. Unlike actionRequiredCopy this does not change the + // conclusion to 'action_required' and does not disable Code Reviewer: a + // provider rate limit is transient and there is nothing to reconfigure. + const terminalReasonCopy = + reviewStatus === 'failed' && !actionRequiredCopy && !billingFailure + ? getCodeReviewTerminalReasonCopy(terminalReason) + : null; const conclusionMap: Record = { completed: reviewFailed ? 'failure' : 'success', @@ -791,7 +807,7 @@ function mapStatusToCheckRun( ? actionRequiredCopy.checkTitle : billingFailure ? 'Insufficient credits to run review' - : 'Kilo Code Review failed', + : (terminalReasonCopy?.checkTitle ?? 'Kilo Code Review failed'), cancelled: modelNotFoundCancellation ? MODEL_NOT_FOUND_CHECK_TITLE : 'Kilo Code Review cancelled', @@ -806,9 +822,8 @@ function mapStatusToCheckRun( ? actionRequiredCopy.checkSummary : billingFailure ? 'Review could not start because the account has insufficient credits.' - : errorMessage - ? `Review failed: ${errorMessage}` - : 'Review failed.', + : (terminalReasonCopy?.checkSummary ?? + (errorMessage ? `Review failed: ${errorMessage}` : 'Review failed.')), cancelled: modelNotFoundCancellation ? MODEL_NOT_FOUND_STATUS_SUMMARY : 'Review was cancelled.', }; @@ -868,6 +883,10 @@ function getGitLabStatusDescription( if (actionRequiredReason) { return getCodeReviewActionRequiredCopy(actionRequiredReason).gitlabDescription; } + if (reviewStatus === 'failed') { + const terminalReasonCopy = getCodeReviewTerminalReasonCopy(terminalReason); + if (terminalReasonCopy) return terminalReasonCopy.checkSummary; + } if (reviewStatus === 'failed' && errorMessage) { const desc = `Review failed: ${errorMessage}`; return desc.length > 255 ? desc.slice(0, 252) + '...' : desc; diff --git a/apps/web/src/components/code-reviews/CodeReviewJobsCard.tsx b/apps/web/src/components/code-reviews/CodeReviewJobsCard.tsx index 8e3ae2952f..b6f388c068 100644 --- a/apps/web/src/components/code-reviews/CodeReviewJobsCard.tsx +++ b/apps/web/src/components/code-reviews/CodeReviewJobsCard.tsx @@ -80,6 +80,7 @@ import { getCodeReviewDetailHref, type CodeReviewUiPlatform, } from '@/lib/code-reviews/code-review-links'; +import { getCodeReviewTerminalReasonCopy } from '@/lib/code-reviews/terminal-reason-copy'; import { CODE_REVIEW_STATUS_LABELS, hasInFlightReview, @@ -894,12 +895,24 @@ export function CodeReviewJobsCard({ )} - {/* Error Message */} - {review.error_message && ( -
- Error: {review.error_message} -
- )} + {/* Error Message. Reasons with customer-facing copy render + muted instead of destructive: the cause is stated + plainly and nothing is broken on our side. */} + {(() => { + const reasonCopy = getCodeReviewTerminalReasonCopy(review.terminal_reason); + if (reasonCopy) { + return ( +
+ {reasonCopy.label}: {reasonCopy.message} +
+ ); + } + return review.error_message ? ( +
+ Error: {review.error_message} +
+ ) : null; + })()} {/* View Progress Button */} {canShowStream && ( diff --git a/apps/web/src/lib/code-reviews/terminal-reason-copy.test.ts b/apps/web/src/lib/code-reviews/terminal-reason-copy.test.ts new file mode 100644 index 0000000000..8ef793bfb7 --- /dev/null +++ b/apps/web/src/lib/code-reviews/terminal-reason-copy.test.ts @@ -0,0 +1,45 @@ +import { CODE_REVIEW_ACTION_REQUIRED_REASONS } from './action-required-shared'; +import { getCodeReviewTerminalReasonCopy } from './terminal-reason-copy'; + +describe('getCodeReviewTerminalReasonCopy', () => { + it('returns null for reasons without customer-facing copy', () => { + expect(getCodeReviewTerminalReasonCopy(null)).toBeNull(); + expect(getCodeReviewTerminalReasonCopy(undefined)).toBeNull(); + expect(getCodeReviewTerminalReasonCopy('sandbox_error')).toBeNull(); + expect(getCodeReviewTerminalReasonCopy('assistant_failed')).toBeNull(); + }); + + it('names the customer key for a byok rate limit', () => { + expect(getCodeReviewTerminalReasonCopy('assistant_rate_limited_byok')).toEqual({ + label: 'Rate limited', + message: 'Your provider API key hit its rate limit.', + checkTitle: 'Kilo Code Review rate limited', + checkSummary: 'Your provider API key hit its rate limit.', + }); + }); + + // 'managed' means the request used Kilo's credentials, which currently + // conflates our upstream quota with our own abuse rules. Telling a customer to + // check their key would be wrong in both cases. + it('does not claim a customer key for managed or unqualified rate limits', () => { + expect(getCodeReviewTerminalReasonCopy('assistant_rate_limited_managed')).toBeNull(); + expect(getCodeReviewTerminalReasonCopy('assistant_rate_limited')).toBeNull(); + }); + + // This map is notification only. Action-required reasons additionally disable + // Code Reviewer and own their own copy, so overlapping the two would give a + // reason two competing messages. + it('does not overlap with the action-required reasons', () => { + const overlapping = CODE_REVIEW_ACTION_REQUIRED_REASONS.filter(reason => + getCodeReviewTerminalReasonCopy(reason) + ); + + expect(overlapping).toEqual([]); + }); + + it('ignores prototype keys', () => { + expect(getCodeReviewTerminalReasonCopy('constructor')).toBeNull(); + expect(getCodeReviewTerminalReasonCopy('__proto__')).toBeNull(); + expect(getCodeReviewTerminalReasonCopy('toString')).toBeNull(); + }); +}); diff --git a/apps/web/src/lib/code-reviews/terminal-reason-copy.ts b/apps/web/src/lib/code-reviews/terminal-reason-copy.ts new file mode 100644 index 0000000000..ace73a81ec --- /dev/null +++ b/apps/web/src/lib/code-reviews/terminal-reason-copy.ts @@ -0,0 +1,62 @@ +/** + * Customer-facing copy for terminal reasons that need something friendlier than + * the raw `error_message`. + * + * Deliberately narrow. Most failures either have nothing useful to say to the + * customer or are already handled by the action-required path, which owns its + * own copy AND disables Code Reviewer. This map is the opposite: it is + * notification only and changes no behaviour. + * + * A reason belongs here only when we can state the cause accurately. Notably + * `assistant_rate_limited_managed` is absent: it means the request used Kilo's + * provider credentials, which currently conflates our upstream quota running out + * with our own abuse rules throttling the request. Those mean different things + * to a customer, so until they can be told apart it keeps the generic message + * rather than getting a sentence that would be wrong half the time. + */ + +import type { CodeReviewTerminalReason } from '@kilocode/db/schema-types'; + +export type CodeReviewTerminalReasonCopy = { + /** Replaces the default "Error" label in the app. */ + label: string; + /** Shown in place of the raw error_message. */ + message: string; + /** GitHub check run title. Kept short; GitHub truncates aggressively. */ + checkTitle: string; + /** GitHub check run summary and the GitLab commit status description. */ + checkSummary: string; +}; + +/** + * A Map rather than an object literal. Callers pass the raw `terminal_reason` + * column, so a value of 'constructor' or '__proto__' would resolve an inherited + * Object.prototype member on an object literal. That value is truthy, defeating + * the null fallback and returning a function where copy is expected. Map keys + * have no prototype chain. + */ +const COPY_BY_TERMINAL_REASON = new Map([ + [ + 'assistant_rate_limited_byok', + { + label: 'Rate limited', + message: 'Your provider API key hit its rate limit.', + checkTitle: 'Kilo Code Review rate limited', + checkSummary: 'Your provider API key hit its rate limit.', + }, + ], +]); + +/** + * Resolve customer-facing copy for a terminal reason, or null when the raw + * error message should be shown as-is. + * + * Accepts the loose `string | null` the DB column and UI props carry, so callers + * do not have to narrow before asking. + */ +export function getCodeReviewTerminalReasonCopy( + terminalReason: string | null | undefined +): CodeReviewTerminalReasonCopy | null { + if (!terminalReason) return null; + return COPY_BY_TERMINAL_REASON.get(terminalReason as CodeReviewTerminalReason) ?? null; +} diff --git a/apps/web/src/routers/admin-code-reviews-router.ts b/apps/web/src/routers/admin-code-reviews-router.ts index daa396d76d..9c079b3e81 100644 --- a/apps/web/src/routers/admin-code-reviews-router.ts +++ b/apps/web/src/routers/admin-code-reviews-router.ts @@ -100,8 +100,12 @@ const TERMINAL_REASON_LABELS: Record = { sandbox_connection: 'Sandbox Connection', container_shutdown: 'Container Shutdown', assistant_rate_limited: 'Rate Limited', + // Named by owner rather than by the internal `providerOwnership` value. + // "managed key" required knowing that 'managed' means Kilo's own credential, + // which is the wrong thing to have to recall while triaging: one of these + // should page us and the other should not. assistant_rate_limited_byok: 'Rate Limited (customer key)', - assistant_rate_limited_managed: 'Rate Limited (managed key)', + assistant_rate_limited_managed: 'Rate Limited (Kilo key)', assistant_unavailable: 'Assistant Unavailable', assistant_timeout: 'Assistant Timeout', assistant_unauthorized: 'Assistant Unauthorized', From d1d6470d1a547608a2c20ac63f99bcd215100b5f Mon Sep 17 00:00:00 2001 From: St0rmz1 Date: Mon, 27 Jul 2026 12:28:04 -0700 Subject: [PATCH 2/2] test(code-review): cover the GitLab commit status for byok rate limits --- .../[reviewId]/route.test.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.test.ts b/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.test.ts index 7fcd444da0..630e651c7c 100644 --- a/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.test.ts +++ b/apps/web/src/app/api/internal/code-review-status/[reviewId]/route.test.ts @@ -3002,6 +3002,34 @@ describe('POST /api/internal/code-review-status/[reviewId]', () => { expect(mockCreateInfraRetryAttemptIfMissing).not.toHaveBeenCalled(); }); + // The GitLab commit status is built by a separate function from the GitHub + // check run, so it needs its own coverage. + it('names the customer key in the GitLab commit status', async () => { + mockGetCodeReviewById.mockResolvedValue( + makeReview({ platform: 'gitlab', platform_project_id: 42, check_run_id: null }) + ); + + await POST( + makeRequest({ + status: 'failed', + errorMessage: 'Assistant request was rate limited', + terminalReason: 'assistant_rate_limited_byok', + }), + makeParams(REVIEW_ID) + ); + + expect(mockSetCommitStatus).toHaveBeenCalledWith( + 'mock-token', + 42, + 'abc123', + 'failed', + expect.objectContaining({ + description: 'Your provider API key hit its rate limit.', + }), + 'https://gitlab.com' + ); + }); + // Our own capacity can free up, so this one stays retryable. it('still auto-retries a managed key rate limit', async () => { mockGetCodeReviewById.mockResolvedValue(makeReview());