_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

Pull request overview

This pull request fixes a security issue where transient LLM API errors (500, api_error, server_error, internal_error) from providers like Anthropic were exposing raw error details including request IDs to end users via WhatsApp and other messaging channels. The fix introduces a new helper function isTransientApiError() to detect these errors and guards both error formatting functions to return a generic user-friendly message instead.

Changes:

• Added isTransientApiError() helper to detect transient server errors in JSON API payloads
• Updated formatAssistantErrorText() and formatRawAssistantErrorForUi() to suppress transient errors with a generic message
• Modified existing test and added new test case for the exact Anthropic 500 error pattern

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Reviewed the diff carefully. The fix is well-structured and covers the primary case reported in #20250. A few observations from someone who hit a related variant in production:

What this PR gets right

• isTransientApiError() has three clean detection layers: error-type set (api_error, server_error, internal_error), message-content matching, and HTTP code fallback. That's thorough for the 500-class case.
• Both formatAssistantErrorText() and formatRawAssistantErrorForUi() are guarded, so the fix covers both code paths into the messaging layer.
• Tests are solid — the existing test that asserted raw output was updated correctly, and the new test for the exact Anthropic 500 payload from the incident is a good regression anchor.

Gaps worth flagging

1. Auth errors (401) are not covered. We experienced FailoverError: HTTP 401 authentication_error and All models failed (2): ... being delivered as WhatsApp messages to end users. These come from the failover layer, not from formatRawAssistantErrorForUi directly. 401 is not in TRANSIENT_HTTP_ERROR_CODES, and authentication_error is not in TRANSIENT_API_ERROR_TYPES. A misconfigured or expired token will still leak raw error text after this PR merges. I'd suggest either: (a) expanding TRANSIENT_API_ERROR_TYPES to include authentication_error / permission_error (they're equally opaque to end users), or (b) ensuring the failover aggregation path sanitizes before emitting.

2. No admin notification path. The PR suppresses the raw message uniformly — the end user sees a friendly string, but so does the gateway admin. In a self-hosted scenario it would be valuable to still surface the raw error somewhere (log line, admin-channel message, or a DM to a configured admin number). Without this, a persistent auth failure or misconfiguration becomes invisible until someone notices Devinho stopped responding entirely.

3. classifyFailoverReason is not updated (also flagged by Greptile). If a transient JSON-format error from a provider doesn't trigger failover classification, the gateway may not try the next model. That's a correctness issue separate from the leak, but it compounds the user experience for the same error event.

Overall

The core fix is correct and safe to merge as-is for the api_error/server_error/internal_error surface. But given that auth errors produce the same bad UX (raw technical text to end users) and are arguably more common in practice (expired tokens, misconfigured providers), I'd recommend at minimum opening a follow-up issue for the auth-error path before closing #20250 as fully resolved. Marking this as a comment rather than approval for that reason.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

  if (isTimeoutErrorMessage(raw)) {
    return "LLM request timed out.";
  }

  if (isBillingErrorMessage(raw)) {
    return formatBillingErrorMessage(opts?.provider);
  }

  if (isLikelyHttpErrorText(raw) || isRawApiErrorPayload(raw)) {