fix(server): surface the cause behind a persistence SQL error

[Sy-D]

What Changed

toPersistenceSqlError no longer restates the operation as the detail. It carries a payload-safe description of the actual cause instead.

Why

Reported in #4818. The screenshot there reads:

SQL error in OrchestrationCommandReceiptRepository.upsert:query: Failed to execute OrchestrationCommandReceiptRepository.upsert:query

The operation name twice, and nothing else. PersistenceSqlError.message composes SQL error in ${operation}: ${detail}, and the mapper set detail to Failed to execute ${operation}. The real failure was captured in cause and never rendered.

Errors.test.ts already asserts this intent, in a test named "keeps SQL operation context without a tautological detail" — it builds the error without a detail and expects SQL error in AuthSessionRepository.list:query. toPersistenceSqlError was the path that did not honour it.

Notes For Review

This does not fix the SQL failure in #4818. It makes it identifiable. The upsert that failed there runs inside withTransaction in OrchestrationEngine.ts:190, so SQLITE_BUSY is plausible — and so is a Request schema encode failure, which this mapper would have labelled a "SQL error" despite not being one. Those want opposite fixes and today's message cannot tell them apart, which is presumably why the issue is filed as not reproducible.

On the payload rule. The neighbouring test "maps schema errors without copying rejected payloads into diagnostics" establishes that rejected values must not reach diagnostics, so the cause is not copied blindly:

• a schema failure contributes summarizeSchemaIssue, which emits issue tags only,
• a driver failure contributes its own message, which names the SQLite condition and at most the constraint it violated, never a bound value,
• anything else leaves the detail unset, which message already renders cleanly.

A new test asserts a rejected value cannot appear in the message.

Blast radius. This mapper shapes every persistence error message that goes through it. The diff is small, the effect is broad — that is the point, but it is worth a deliberate look.

Follow-up, deliberately not in this PR. Newer call sites (AuthPairingLinks.ts:112) route a schema cause to PersistenceDecodeError rather than labelling it a SQL error, which is the better shape. The comment above toPersistenceDecodeError notes these call sites are "being revamped separately", so I left the routing alone and only fixed the message.

Validation

• vp run --filter t3 test — 1690 passed, 7 skipped. No existing test depended on the old tautological detail.
• vp run --filter t3 typecheck — no errors
• vp lint / vp fmt --check on the changed files — clean

New cases in Errors.test.ts:

cause	resulting message
Error("SQLITE_BUSY: database is locked")	SQL error in …upsert:query: SQLITE_BUSY: database is locked
schema error	issue tags only; asserts the rejected value is absent
a value with no message	SQL error in …list:query (unchanged)

Checklist

• This PR is small and focused
• I explained what changed and why
• No UI change, so no screenshots

---

Note

Medium Risk
The mapper is used broadly across persistence call sites, so every SQL error message shape changes; detail derivation is designed to avoid leaking query/payload data but alters operator-facing diagnostics.

Overview
toPersistenceSqlError no longer sets detail to Failed to execute <operation>, which duplicated the operation in PersistenceSqlError.message and hid the real failure in cause.

It now derives detail via describeSqlCause: SQLite driver errors contribute a bounded SQLITE(code) errstr string (walking up to four levels of cause for wrapped driver errors), schema failures contribute issue tags only through summarizeSchemaIssue, and unrecognized causes leave detail unset so the message stays SQL error in <operation>.

Tests in Errors.test.ts cover normalized SQLite codes, nested driver causes, exclusion of driver prose like column names from messages, uncategorizable causes, and schema paths that must not leak rejected values.

^{Reviewed by Cursor Bugbot for commit fbb2ec7. Bugbot is set up for automated code reviews on this repo. Configure here.}

Note

Surface SQLite condition and schema issue details in toPersistenceSqlError

• toPersistenceSqlError in Errors.ts now derives detail from the error cause instead of always setting 'Failed to execute <operation>'.
• A new sqliteCondition helper walks up to four levels of the cause chain to find errcode/errstr properties and returns a normalized string like 'SQLITE(1555) constraint failed'.
• A new describeSqlCause helper categorizes the cause: SQLite errors get the normalized condition string; Schema errors get a summary via issue tag; uncategorizable causes result in detail being omitted entirely.
• Behavioral Change: detail on persistence SQL errors changes from the fixed string 'Failed to execute <operation>' to a normalized SQLite condition, a schema issue tag, or undefined.

^{Macroscope summarized fbb2ec7.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 209783da-9274-4582-a73b-01c07ae5ab05

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[macroscopeapp]

Reviewed against the Effect service conventions. One violation found in apps/server/src/persistence/Errors.ts (with a test that locks in the behavior): the mapper now copies cause.message into the structural detail attribute, and PersistenceSqlError.message is built from that detail.

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

Approvability

Verdict: Approved 9ff106f

This PR improves error message content in persistence SQL errors by surfacing SQLite error codes and schema issue summaries. The changes are limited to diagnostic output with no control flow impact, and include thorough test coverage.

^{You can customize Macroscope's approvability policy. Learn more.}

[Sy-D]

Both points taken, fixed in 9ff106f — and the second suggestion turned out to be the better design, so I took that rather than dropping the SQL side entirely.

Limiting detail to the schema summary alone would have left a real SQLite failure with an operation-only message, which is the state #4818 is stuck in. Your alternative — a bounded normalized code — keeps the diagnosis without the unbounded text.

SQLite names its condition through a fixed result-code table, and node:sqlite exposes it as errcode / errstr. Both are bounded and carry no query data, unlike the message. Checked against a real failure:

message : UNIQUE constraint failed: t.a          <- driver prose, names the column
errcode : 1555
errstr  : constraint failed                       <- fixed table, value-free

So describeSqlCause now contributes:

• schema failure → summarizeSchemaIssue, issue tags only, unchanged
• driver failure → SQLITE(<errcode>) <errstr>, read through the wrapping error since the mapper receives the SqlError
• anything else → nothing, leaving the operation-only message

Verified end to end against a genuine node:sqlite constraint violation on a table holding a sentinel value:

message : SQL error in OrchestrationCommandReceiptRepository.upsert:query: SQLITE(1555) constraint failed
detail  : SQLITE(1555) constraint failed
sentinel present in message? false
cause preserved? true

On the test comment: the assertion on spliced driver text is gone. The suite now asserts structural attributes — the exact detail, the preserved cause, that the driver's column name cannot reach the message, and that an uncategorizable cause leaves detail unset.

vp run --filter t3 test — 1692 passed, 7 skipped. Typecheck and lint clean.

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit fbb2ec7. Configure here.}

[cursor]

SqlError reason not traversed

High Severity

sqliteCondition only walks the .cause chain, but the Node SQLite client wraps driver failures as SqlError with reason: classifySqliteError(...) and does not pass the raw driver as cause. When PersistenceSqlError is built from that SqlError, describeSqlCause may find no errcode/errstr, leave detail unset, and #4818-style upsert failures can still show an operation-only message.

 

^{Reviewed by Cursor Bugbot for commit fbb2ec7. Configure here.}

[Sy-D]

Checked this against the real construction, and the conclusion does not hold — though the premise is a fair read of the code.

NodeSqliteClient does build the error as described, with only a reason:

catch: (cause) => new SqlError({ reason: classifySqliteError(cause, { message: "...", operation: "prepare" }) })

But Effect's SqlError exposes a cause derived from that reason, and the classified reason keeps the driver error as its cause. So the chain SqlError → reason → driver error is walkable, which is what sqliteCondition does.

Built that exact shape and ran it through the mapper:

classifySqliteError exportiert? function
hat .cause?               true
hat .reason?              true
reason.cause vorhanden?   true
detail : SQLITE(1555) constraint failed

Across the failure kinds an upsert can realistically hit:

failure	detail	driver prose leaked?
UNIQUE violation	SQLITE(1555) constraint failed	no
NOT NULL violation	SQLITE(1299) constraint failed	no
syntax error	SQLITE(1) SQL logic error	no
missing table	SQLITE(1) SQL logic error	no
missing column	SQLITE(1) SQL logic error	no

So a #4818-style upsert failure does get a categorized detail rather than an operation-only message, which is the outcome the review was worried about.

The walk is bounded at four levels of cause. If a future client wraps more deeply than that, the detail would fall back to unset — that is the intended failure mode, not silence about a value.