fix(errors): keep the SQLSTATE when wrapping database errors (#2027)

isTransientFailure() checks the driver's error code first, and 57014
(statement timeout) is already in its transient set. But the wrapping idiom
across the codebase was `throw new Error(\`Database error: ${err.message}\`)`,
which keeps the prose and drops the code. A retryable timeout therefore
arrived anonymous and resolved to UNKNOWN_ERROR: "Något gick fel. Försök
igen." An agent cannot dispatch on that, so it retried.

On production over 60 days, with the two bot integrations excluded: 1024 real
agent failures, 645 of them UNKNOWN_ERROR across 60 actors and 57 companies.
82 retry streaks of three or more identical failures, 462 wasted repeat calls,
53.1% of all agent error calls sitting inside a streak.

The worst offender traces to one line in core. gnubok_query_journal failed 164
times at a p50 of 8110ms while every other failing tool sat between 1 and
315ms, and its path is fetchEntryLines -> fetchAllRows, where
lib/supabase/fetch-all.ts threw `new Error(error.message)`. That is the
highest-traffic strip point in the repo: 31 callers, every paginated read.
query_journal already had a correct TRANSIENT_ERROR branch offering "retry, or
narrow with date_from/date_to" which could never fire, because by the time it
looked, the code was gone.

fetch-all keeps the driver message verbatim: callers match on the existing
text, and this adds the code rather than rewording anything.

Attaching the code is safe. extractCode() only accepts /^[A-Z_]+$/ and every
SQLSTATE contains digits, so it cannot be mistaken for one of our own stable
codes. There is a test for that, and one asserting the old bare-Error shape
still resolves to UNKNOWN_ERROR so the fix cannot silently regress.

Also stops rendering the literal "undefined" when a driver-level failure
carries no message, which is the string that made these unsearchable.

Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

lib/errors/__tests__/db-error.test.ts

@@ -0,0 +1,110 @@
+/**
+ * The database-error wrapper, and the classification it exists to unlock.
+ *
+ * The idiom `throw new Error(\`Database error: ${error.message}\`)` kept the
+ * prose and dropped `code`. `isTransientFailure()` checks that SQLSTATE FIRST,
+ * and 57014 (statement timeout) is already in its transient set, so stripping
+ * it turned a retryable timeout into UNKNOWN_ERROR: "Något gick fel. Försök
+ * igen." Agents cannot dispatch on that, so they retried: on production over
+ * 60 days, 462 wasted repeat calls, with 53.1% of all real-agent error calls
+ * sitting inside a repeat streak.
+ *
+ * The headline test is the last one: it asserts the OLD shape still resolves
+ * to UNKNOWN_ERROR and the new one resolves to TRANSIENT_ERROR, so it fails if
+ * the wrapper ever stops preserving the code.
+ */
+import { describe, it, expect } from 'vitest'
+import { dbError, errorCauseTag } from '../db-error'
+import { getStructuredError } from '../get-structured-error'
+
+/** What supabase-js hands back when Postgres cancels on statement_timeout. */
+const TIMEOUT_ERROR = {
+  message: 'canceling statement due to statement timeout',
+  code: '57014',
+  details: null,
+  hint: null,
+}
+
+describe('dbError', () => {
+  it('preserves the SQLSTATE, which is the whole point', () => {
+    const wrapped = dbError(TIMEOUT_ERROR)
+    expect(wrapped.code).toBe('57014')
+  })
+
+  it('preserves details and hint for the server log', () => {
+    const wrapped = dbError({
+      message: 'duplicate key value violates unique constraint',
+      code: '23505',
+      details: 'Key (company_id, account_number) already exists.',
+      hint: 'Use upsert.',
+    })
+    expect(wrapped.details).toContain('already exists')
+    expect(wrapped.hint).toBe('Use upsert.')
+  })
+
+  it('never renders the literal "undefined"', () => {
+    // A driver-level failure (aborted fetch, gateway timeout) can arrive with
+    // no message at all. "Database error: undefined" is the string that made
+    // these unsearchable in production.
+    for (const shape of [{}, { message: undefined }, { message: '' }, { message: '   ' }, null]) {
+      expect(dbError(shape).message).not.toContain('undefined')
+    }
+  })
+
+  it('prefixes with the historical context by default', () => {
+    expect(dbError({ message: 'boom' }).message).toBe('Database error: boom')
+  })
+
+  it('keeps the driver message verbatim when context is null', () => {
+    // fetchAllRows passes null: callers such as query_journal's
+    // sanitizeDbError already match on the exact text, and this change is
+    // meant to add the code, not reword anything.
+    expect(dbError({ message: 'boom' }, null).message).toBe('boom')
+  })
+
+  it('accepts a custom context', () => {
+    expect(dbError({ message: 'boom' }, 'Database error resolving voucher "A-7"').message).toBe(
+      'Database error resolving voucher "A-7": boom',
+    )
+  })
+})
+
+describe('errorCauseTag', () => {
+  it('returns the SQLSTATE, which carries no tenant data', () => {
+    expect(errorCauseTag(dbError(TIMEOUT_ERROR))).toBe('57014')
+  })
+
+  it('falls back to the error name when there is no code', () => {
+    expect(errorCauseTag(new TypeError('nope'))).toBe('TypeError')
+  })
+
+  it('returns null rather than inventing a tag', () => {
+    expect(errorCauseTag(new Error('plain'))).toBeNull()
+    expect(errorCauseTag(null)).toBeNull()
+  })
+})
+
+describe('classification: the regression this prevents', () => {
+  it('classifies a wrapped statement timeout as retryable', () => {
+    expect(getStructuredError(dbError(TIMEOUT_ERROR)).code).toBe('TRANSIENT_ERROR')
+  })
+
+  it('would classify the OLD bare-Error shape as UNKNOWN_ERROR', () => {
+    // The bug, pinned. `new Error(error.message)` is what fetchAllRows threw,
+    // and PostgREST does not always put the word "timeout" in the message it
+    // returns, so message-pattern matching cannot be relied on. Only the code
+    // is durable, which is why the wrapper must carry it.
+    const stripped = new Error('some driver text that names no timeout')
+    expect(getStructuredError(stripped).code).toBe('UNKNOWN_ERROR')
+
+    // Same failure, code intact: now dispatchable.
+    const preserved = dbError({ message: 'some driver text that names no timeout', code: '57014' })
+    expect(getStructuredError(preserved).code).toBe('TRANSIENT_ERROR')
+  })
+
+  it('does not mistake a SQLSTATE for one of our own stable codes', () => {
+    // extractCode() only accepts /^[A-Z_]+$/. Every SQLSTATE contains digits,
+    // so attaching one cannot hijack the application error registry.
+    expect(getStructuredError(dbError({ message: 'x', code: '23505' })).code).not.toBe('23505')
+  })
+})

lib/errors/db-error.ts

@@ -0,0 +1,97 @@
+/**
+ * Wrap a Supabase/PostgREST error without throwing away its identity.
+ *
+ * ## The bug this exists to prevent
+ *
+ * The idiom across the MCP tools was:
+ *
+ *     if (error) throw new Error(`Database error: ${error.message}`)
+ *
+ * which keeps the prose and discards `code`. That matters because `code` is
+ * the SQLSTATE, and `isTransientFailure()` in lib/errors/get-structured-error.ts
+ * checks it FIRST: `57014` (statement timeout), `40001`, `40P01`, `53300` and
+ * friends are already in its TRANSIENT_SQLSTATES set. Strip the code and a
+ * retryable timeout arrives as an anonymous Error, misses every transient
+ * check, and resolves to UNKNOWN_ERROR: "Något gick fel. Försök igen."
+ *
+ * Measured on production over 60 days (bot actors excluded): 1 024 real-agent
+ * tool failures, 645 of them UNKNOWN_ERROR across 60 actors and 57 companies,
+ * 537 carrying that exact generic string. `gnubok_query_journal` alone failed
+ * 164 times at a p50 of 8 110 ms while every other failing tool sat between 1
+ * and 315 ms: a timeout signature that should have been TRANSIENT_ERROR all
+ * along. Agents cannot dispatch on "something went wrong", so they retried:
+ * 82 streaks of three or more identical failures, 462 wasted repeat calls,
+ * 53.1% of all real-agent error calls sitting inside a streak.
+ *
+ * ## Why attaching `code` is safe
+ *
+ * `extractCode()` only treats a code as an application error code when it
+ * matches /^[A-Z_]+$/. Every SQLSTATE contains digits (`57014`, `42P01`,
+ * `23505`), and so does every PostgREST code (`PGRST200`), so none of them can
+ * be mistaken for one of our own stable codes. The only behaviour this unlocks
+ * is the transient check that was always meant to run.
+ */
+
+/** The shape of a PostgrestError, narrowed to what we read. */
+interface DatabaseErrorLike {
+  message?: string | null
+  code?: string | null
+  details?: string | null
+  hint?: string | null
+}
+
+/**
+ * An Error carrying the driver's SQLSTATE and diagnostics.
+ *
+ * `details` and `hint` are preserved for the server log, not for the agent:
+ * `getStructuredError` never reads them, so they cannot leak into a tool
+ * result. They are what makes a production failure debuggable after the fact.
+ */
+export interface DatabaseError extends Error {
+  code?: string
+  details?: string
+  hint?: string
+}
+
+/**
+ * @param error   the `error` half of a supabase-js `{ data, error }` result
+ * @param context prefix for the message; defaults to the historical
+ *                "Database error" so existing message-pattern matching in
+ *                `inferCode()` keeps working unchanged. Pass `null` to keep
+ *                the driver's message verbatim, for call sites whose exact
+ *                text callers already depend on.
+ */
+export function dbError(error: unknown, context: string | null = 'Database error'): DatabaseError {
+  const source = (error ?? {}) as DatabaseErrorLike
+  const raw = typeof source.message === 'string' && source.message.trim() ? source.message.trim() : null
+
+  // Never render the literal "undefined". A driver-level failure (an aborted
+  // fetch, a gateway timeout) can arrive with no `message` at all, and
+  // "Database error: undefined" is the string that made these unsearchable in
+  // the first place.
+  const prefix = context ?? ''
+  const message = raw
+    ? (prefix ? `${prefix}: ${raw}` : raw)
+    : `${prefix || 'Database error'}: no message from the database driver`
+
+  const wrapped = new Error(message) as DatabaseError
+  if (typeof source.code === 'string' && source.code) wrapped.code = source.code
+  if (typeof source.details === 'string' && source.details) wrapped.details = source.details
+  if (typeof source.hint === 'string' && source.hint) wrapped.hint = source.hint
+  return wrapped
+}
+
+/**
+ * A PII-safe identifier for what failed, for telemetry.
+ *
+ * A SQLSTATE is five characters of protocol vocabulary and carries no tenant
+ * data. A raw driver message can quote row values in a constraint violation,
+ * so it belongs in the server log, never in `event_log`.
+ */
+export function errorCauseTag(error: unknown): string | null {
+  if (error === null || error === undefined) return null
+  const source = error as DatabaseErrorLike & { name?: unknown }
+  if (typeof source.code === 'string' && source.code) return source.code
+  if (error instanceof Error && error.name && error.name !== 'Error') return error.name
+  return null
+}