From cedea4072b47310a64194c81fa7d6ddee1fd47ee Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 17:59:55 +0300 Subject: [PATCH] refactor(ai-chat)!: unify provider error formatting via describeProviderError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behaviour change (split out of the test commit per review, and now covered). Both the stream onError log line and the error text streamed to the client were formatted by separate inline blocks that only emitted ": ". Route both through the shared describeProviderError() so formatting stays in one place. BEHAVIOUR CHANGE: describeProviderError additionally appends a single-line, 300-char-truncated snippet of the provider responseBody/text. So the log line AND the user-facing stream error now include that snippet (e.g. the HTML error page from a misconfigured endpoint), which previously neither did. This is intentional — it makes a misconfigured external endpoint diagnosable — and is safe: the API key travels in the Authorization header and is never echoed in the response body (see the util's docstring). A `fallback` param is added so each call site keeps its own default ('AI stream error' for the stream). Adds ai-error.util.spec.ts covering the formatter, including the appended / truncated body snippet, so this behaviour is no longer untested. Co-Authored-By: Claude Opus 4.8 --- .../src/core/ai-chat/ai-chat.service.ts | 25 +++----- .../src/integrations/ai/ai-error.util.spec.ts | 61 +++++++++++++++++++ .../src/integrations/ai/ai-error.util.ts | 12 +++- 3 files changed, 80 insertions(+), 18 deletions(-) create mode 100644 apps/server/src/integrations/ai/ai-error.util.spec.ts diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index 91b8ebd4..8230dc6d 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -10,6 +10,7 @@ import { } from 'ai'; import { AiService } from '../../integrations/ai/ai.service'; import { AiSettingsService } from '../../integrations/ai/ai-settings.service'; +import { describeProviderError } from '../../integrations/ai/ai-error.util'; import { AiChatRepo } from '@docmost/db/repos/ai-chat/ai-chat.repo'; import { AiChatMessageRepo } from '@docmost/db/repos/ai-chat/ai-chat-message.repo'; import { User, Workspace, AiChatMessage } from '@docmost/db/types/entity.types'; @@ -271,15 +272,10 @@ export class AiChatService { onError: async ({ error }) => { // NestJS Logger.error(message, stack?, context?): pass the real message // (with statusCode when present) + the stack string, not the Error - // object, so the actual provider cause is clearly logged. - const e = error as { - statusCode?: number; - message?: string; - stack?: string; - }; - const errorText = e?.statusCode - ? `${e.statusCode}: ${e.message ?? String(error)}` - : (e?.message ?? String(error)); + // object, so the actual provider cause is clearly logged. Reuse the + // shared formatter so provider error formatting stays unified. + const e = error as { stack?: string }; + const errorText = describeProviderError(error, String(error)); this.logger.error(`AI chat stream error: ${errorText}`, e?.stack); // Persist whatever text we have (likely empty) so the turn is recorded, // and record the error text in metadata so it is visible in history. @@ -340,10 +336,9 @@ export class AiChatService { result.pipeUIMessageStreamToResponse(res.raw, { headers: { 'X-Accel-Buffering': 'no' }, onError: (error: unknown) => { - const e = error as { statusCode?: number; message?: string }; - return e?.statusCode - ? `${e.statusCode}: ${e.message}` - : (e?.message ?? 'AI stream error'); + // Reuse the shared formatter so provider error formatting stays + // unified between the log line and the streamed error message. + return describeProviderError(error, 'AI stream error'); }, }); @@ -538,8 +533,8 @@ function compactValue(value: unknown, depth: number): unknown { * recovers the name. Falls back to a single `text` part built from * `fallbackText` when the steps carry no text. */ -// Exported only so the unit tests can import this pure helper; exporting it -// does not change runtime behavior. +// Exported only so the unit tests can import these pure helpers; exporting +// them does not change runtime behavior. export function assistantParts( steps: ReadonlyArray | undefined, fallbackText: string, diff --git a/apps/server/src/integrations/ai/ai-error.util.spec.ts b/apps/server/src/integrations/ai/ai-error.util.spec.ts new file mode 100644 index 00000000..c9b7fb3e --- /dev/null +++ b/apps/server/src/integrations/ai/ai-error.util.spec.ts @@ -0,0 +1,61 @@ +import { describeProviderError } from './ai-error.util'; + +/** + * Unit tests for describeProviderError: the shared formatter used both for the + * server log line and for the error text streamed back to the client. This + * pins the behaviour, including the one behaviour change introduced when the + * two inline formatters were unified: a truncated, single-line snippet of the + * provider `responseBody`/`text` is appended (so a misconfigured endpoint's + * HTML error page is diagnosable). The util guarantees the API key is never in + * the response body, so this is safe to surface. + */ +describe('describeProviderError', () => { + it('uses the fallback for a null/empty/undefined error', () => { + expect(describeProviderError(null, 'AI stream error')).toBe( + 'AI stream error', + ); + expect(describeProviderError('', 'AI stream error')).toBe('AI stream error'); + expect(describeProviderError(undefined)).toBe('Unknown error'); + }); + + it('returns a non-empty plain string error as-is', () => { + expect(describeProviderError('boom')).toBe('boom'); + }); + + it('formats statusCode + message', () => { + expect( + describeProviderError({ statusCode: 401, message: 'Unauthorized' }), + ).toBe('401: Unauthorized'); + }); + + it('falls back to message when there is no statusCode', () => { + expect(describeProviderError({ message: 'nope' })).toBe('nope'); + }); + + it('appends a whitespace-collapsed response body snippet', () => { + const out = describeProviderError({ + statusCode: 502, + message: 'Bad Gateway', + responseBody: '\n upstream error\n', + }); + expect(out.startsWith('502: Bad Gateway | response body: ')).toBe(true); + // Newlines and runs of spaces are collapsed to single spaces. + expect(out).toContain(' upstream error '); + }); + + it('reads `text` when responseBody is absent', () => { + expect(describeProviderError({ message: 'e', text: 'body-text' })).toBe( + 'e | response body: body-text', + ); + }); + + it('truncates a long body to 300 chars + ellipsis', () => { + const out = describeProviderError({ + message: 'e', + responseBody: 'x'.repeat(500), + }); + expect(out).toContain('…'); + // 'e | response body: ' + 300 chars + '…' + expect(out.length).toBeLessThan('e | response body: '.length + 305); + }); +}); diff --git a/apps/server/src/integrations/ai/ai-error.util.ts b/apps/server/src/integrations/ai/ai-error.util.ts index 68fa328b..0a0f949b 100644 --- a/apps/server/src/integrations/ai/ai-error.util.ts +++ b/apps/server/src/integrations/ai/ai-error.util.ts @@ -9,10 +9,16 @@ * * None of these fields contain the API key (it is sent as an Authorization * header and never echoed in the response body), so this is safe to log/return. + * + * `fallback` is used when the error carries no usable message (e.g. a bare + * object); defaults to 'Unknown error'. */ -export function describeProviderError(err: unknown): string { +export function describeProviderError( + err: unknown, + fallback = 'Unknown error', +): string { if (typeof err !== 'object' || err === null) { - return typeof err === 'string' ? err : 'Unknown error'; + return typeof err === 'string' && err ? err : fallback; } const e = err as { statusCode?: number; @@ -23,7 +29,7 @@ export function describeProviderError(err: unknown): string { const base = typeof e.statusCode === 'number' ? `${e.statusCode}: ${e.message ?? ''}`.trim() - : (e.message ?? 'Unknown error'); + : (e.message ?? fallback); const body = (e.responseBody ?? e.text ?? '').trim(); if (!body) return base; // Collapse whitespace so a multi-line HTML body stays on one log line.