From b4c699d0ef7c948783c9ef1d67ec6b2e46304de4 Mon Sep 17 00:00:00 2001 From: Sy-D <8460326+Sy-D@users.noreply.github.com> Date: Wed, 29 Jul 2026 16:00:28 +0200 Subject: [PATCH 1/2] fix(server): surface the cause behind a persistence SQL error toPersistenceSqlError set the detail to "Failed to execute ", so the rendered message repeated the operation and dropped the only part worth reading. Errors.test.ts already asserts the opposite intent in a test named "keeps SQL operation context without a tautological detail". Carry the cause instead: a schema failure contributes its issue tags, which hold no rejected values, and a driver failure contributes its own message, which names the SQLite condition rather than any bound value. A cause with nothing to say leaves the detail unset, which the message already handles. Refs #4818. Co-Authored-By: Claude Opus 5 --- apps/server/src/persistence/Errors.test.ts | 33 +++++++++++++++++++++- apps/server/src/persistence/Errors.ts | 23 +++++++++++++-- 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/apps/server/src/persistence/Errors.test.ts b/apps/server/src/persistence/Errors.test.ts index 680a362e20a..522b57272a0 100644 --- a/apps/server/src/persistence/Errors.test.ts +++ b/apps/server/src/persistence/Errors.test.ts @@ -2,7 +2,7 @@ import { assert, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as Schema from "effect/Schema"; -import { PersistenceDecodeError, PersistenceSqlError } from "./Errors.ts"; +import { PersistenceDecodeError, PersistenceSqlError, toPersistenceSqlError } from "./Errors.ts"; const decodeRuntimePayload = Schema.decodeUnknownEffect( Schema.Struct({ @@ -25,6 +25,37 @@ it("keeps SQL operation context without a tautological detail", () => { assert.equal(error.message, "SQL error in AuthSessionRepository.list:query"); }); +it("carries the driver failure into the mapped SQL error message", () => { + const cause = new Error("SQLITE_BUSY: database is locked"); + const error = toPersistenceSqlError("OrchestrationCommandReceiptRepository.upsert:query")(cause); + + assert.equal( + error.message, + "SQL error in OrchestrationCommandReceiptRepository.upsert:query: SQLITE_BUSY: database is locked", + ); + assert.equal(error.cause, cause); +}); + +it("omits a detail when the cause carries no message of its own", () => { + const error = toPersistenceSqlError("AuthSessionRepository.list:query")("unhelpful"); + + assert.equal(error.detail, undefined); + assert.equal(error.message, "SQL error in AuthSessionRepository.list:query"); +}); + +it.effect("summarizes a schema cause by issue tag instead of by rejected value", () => + Effect.gen(function* () { + const rejectedPayload = "sql-mapper-secret-sentinel"; + const cause = yield* Effect.flip( + decodeRuntimePayload({ runtimePayload: { attempt: rejectedPayload } }), + ); + const error = toPersistenceSqlError("ProviderSessionRuntimeRepository.list:query")(cause); + + assert.ok(error.detail !== undefined); + assert.ok(!error.message.includes(rejectedPayload)); + }), +); + it.effect("maps schema errors without copying rejected payloads into diagnostics", () => Effect.gen(function* () { const rejectedPayload = "runtime-payload-secret-sentinel"; diff --git a/apps/server/src/persistence/Errors.ts b/apps/server/src/persistence/Errors.ts index 03edaec77d6..916ee0d537d 100644 --- a/apps/server/src/persistence/Errors.ts +++ b/apps/server/src/persistence/Errors.ts @@ -73,13 +73,30 @@ const isPersistenceSqlError = Schema.is(PersistenceSqlError); const isPersistenceDecodeError = Schema.is(PersistenceDecodeError); // Kept for orchestration/projection call sites, which are being revamped separately. +/** + * A rejected payload must never reach diagnostics, so a schema failure + * contributes only its issue tags. A driver error names the SQLite condition + * and at most the constraint it violated, never a bound value, so its own + * message is safe to carry and is the part that makes an occurrence + * diagnosable at all. + */ +function describeSqlCause(cause: unknown): string | undefined { + if (Schema.isSchemaError(cause)) return summarizeSchemaIssue(cause.issue); + if (cause instanceof Error && cause.message.length > 0) return cause.message; + return undefined; +} + export function toPersistenceSqlError(operation: string) { - return (cause: unknown): PersistenceSqlError => - new PersistenceSqlError({ + return (cause: unknown): PersistenceSqlError => { + // Restating the operation as the detail only doubled it in the message and + // buried the cause; `message` already reads well without a detail. + const detail = describeSqlCause(cause); + return new PersistenceSqlError({ operation, - detail: `Failed to execute ${operation}`, + ...(detail === undefined ? {} : { detail }), cause, }); + }; } // Kept for orchestration/projection call sites, which are being revamped separately. From fbb2ec7ddb1afb87928a859a92bddc6a70b45396 Mon Sep 17 00:00:00 2001 From: Sy-D <8460326+Sy-D@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:26:21 +0200 Subject: [PATCH 2/2] fix(server): carry a normalized SQLite condition, not the driver's prose Review feedback: copying cause.message into detail put unbounded driver text into a direct error attribute, and a constraint failure spells out the table and column it hit. SQLite names its condition through a fixed result-code table, so errcode and errstr are bounded and carry no query data. Carry that pair instead, read through the wrapping driver error, and leave the detail unset for a cause the mapper cannot categorize. The full text stays on cause. Co-Authored-By: Claude Opus 5 --- apps/server/src/persistence/Errors.test.ts | 35 +++++++++++++++++----- apps/server/src/persistence/Errors.ts | 27 +++++++++++++---- 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/apps/server/src/persistence/Errors.test.ts b/apps/server/src/persistence/Errors.test.ts index 522b57272a0..4223273aca7 100644 --- a/apps/server/src/persistence/Errors.test.ts +++ b/apps/server/src/persistence/Errors.test.ts @@ -25,19 +25,38 @@ it("keeps SQL operation context without a tautological detail", () => { assert.equal(error.message, "SQL error in AuthSessionRepository.list:query"); }); -it("carries the driver failure into the mapped SQL error message", () => { - const cause = new Error("SQLITE_BUSY: database is locked"); +it("names the SQLite condition by its normalized result code", () => { + const cause = Object.assign(new Error("UNIQUE constraint failed: orders.customer_email"), { + errcode: 1555, + errstr: "constraint failed", + }); const error = toPersistenceSqlError("OrchestrationCommandReceiptRepository.upsert:query")(cause); - assert.equal( - error.message, - "SQL error in OrchestrationCommandReceiptRepository.upsert:query: SQLITE_BUSY: database is locked", - ); + assert.equal(error.detail, "SQLITE(1555) constraint failed"); assert.equal(error.cause, cause); }); -it("omits a detail when the cause carries no message of its own", () => { - const error = toPersistenceSqlError("AuthSessionRepository.list:query")("unhelpful"); +it("reads the condition through a wrapping driver error", () => { + const driver = Object.assign(new Error("locked"), { errcode: 5, errstr: "database is locked" }); + const error = toPersistenceSqlError("AuthSessionRepository.list:query")( + new Error("Failed to prepare statement", { cause: driver }), + ); + + assert.equal(error.detail, "SQLITE(5) database is locked"); +}); + +it("keeps the driver's own prose out of the message", () => { + const cause = Object.assign(new Error("UNIQUE constraint failed: orders.customer_email"), { + errcode: 1555, + errstr: "constraint failed", + }); + const error = toPersistenceSqlError("AuthSessionRepository.create:query")(cause); + + assert.ok(!error.message.includes("customer_email")); +}); + +it("omits a detail for a cause it cannot categorize", () => { + const error = toPersistenceSqlError("AuthSessionRepository.list:query")(new Error("unhelpful")); assert.equal(error.detail, undefined); assert.equal(error.message, "SQL error in AuthSessionRepository.list:query"); diff --git a/apps/server/src/persistence/Errors.ts b/apps/server/src/persistence/Errors.ts index 916ee0d537d..bce1dd1f36b 100644 --- a/apps/server/src/persistence/Errors.ts +++ b/apps/server/src/persistence/Errors.ts @@ -73,17 +73,32 @@ const isPersistenceSqlError = Schema.is(PersistenceSqlError); const isPersistenceDecodeError = Schema.is(PersistenceDecodeError); // Kept for orchestration/projection call sites, which are being revamped separately. +/** + * SQLite names its condition through a fixed result-code table, so the code and + * its canonical string are bounded and carry no query data. The driver message + * is not bounded: a constraint failure spells out the table and column it hit. + * Only the normalized pair is carried here; `cause` keeps everything else. + */ +function sqliteCondition(cause: unknown): string | undefined { + let value: unknown = cause; + for (let depth = 0; depth < 4 && typeof value === "object" && value !== null; depth += 1) { + const { errcode, errstr } = value as { errcode?: unknown; errstr?: unknown }; + if (typeof errcode === "number" && typeof errstr === "string") { + return `SQLITE(${errcode}) ${errstr}`; + } + value = (value as { cause?: unknown }).cause; + } + return undefined; +} + /** * A rejected payload must never reach diagnostics, so a schema failure - * contributes only its issue tags. A driver error names the SQLite condition - * and at most the constraint it violated, never a bound value, so its own - * message is safe to carry and is the part that makes an occurrence - * diagnosable at all. + * contributes only its issue tags, and a driver failure only its normalized + * condition. Anything the mapper cannot categorize leaves the detail unset. */ function describeSqlCause(cause: unknown): string | undefined { if (Schema.isSchemaError(cause)) return summarizeSchemaIssue(cause.issue); - if (cause instanceof Error && cause.message.length > 0) return cause.message; - return undefined; + return sqliteCondition(cause); } export function toPersistenceSqlError(operation: string) {