Skip to content

fix(server): include SQLite conditions in persistence errors - #9536

Merged
t3dotgg merged 3 commits into
mainfrom
t3code/takeover-merge-pr-4837
Sep 4, 2026
Merged

fix(server): include SQLite conditions in persistence errors#9536
t3dotgg merged 3 commits into
mainfrom
t3code/takeover-merge-pr-4837

Conversation

@t3dotgg

@t3dotgg t3dotgg commented Sep 4, 2026

Copy link
Copy Markdown
Member

Persistence SQL errors repeat the operation name and hide the cause.

Include Node's fixed SQLite description, Bun's numeric error code, or schema issue tags without copying driver messages or rejected values. This improves diagnosis. It does not fix the database failure reported in issue #4818.

Continues @Sy-D's #4837. The original commits and co-author credit are preserved. This adds the missing Bun handling and tests through the SQL client.

Verified with 14 focused tests, server typecheck, changed-file lint and formatting, and real in-memory Node and Bun failures.

Created with GPT-6 Astra (preview) in Codex.


Note

Low Risk
Diagnostics-only change in the persistence error mapper; behavior for uncategorized errors matches omitting detail, with no query or auth logic changes.

Overview
toPersistenceSqlError no longer sets a tautological detail (Failed to execute …). It now derives optional detail from the cause: normalized SQLite conditions (Node errcode/errstr or Bun SQLiteError errno, walking a short cause chain), or schema issue tags when the failure is a SchemaError. Driver text, query values, and rejected payloads stay out of message/detail.

Tests cover in-memory constraint failures, wrapped Node driver errors, Bun errors behind SqlError, uncategorizable causes (no detail), and schema mapping without leaking sentinels.

Reviewed by Cursor Bugbot for commit 577b098. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Include SQLite conditions in toPersistenceSqlError persistence diagnostics

  • Adds sqliteCondition helper in Errors.ts that traverses the cause chain (up to four wrappers) and recognizes Node SQLite errors by numeric errcode+errstr and Bun SQLiteError by integer errno; driver messages are not read for classification
  • Adds describeSqlCause helper that routes schema causes to the existing issue-tag summarizer and all other causes to the SQLite condition extractor, returning undefined when neither applies
  • toPersistenceSqlError now derives detail from these helpers and no longer assigns a fixed failed-operation detail to every cause; operation and original cause remain attached
  • Risk: toPersistenceSqlError no longer emits detail for uncategorized causes — callers that relied on the previous fixed detail string will see PersistenceSqlError with no detail for non-SQLite, non-schema errors

Macroscope summarized 577b098.

Sy-D and others added 3 commits September 3, 2026 18:20
toPersistenceSqlError set the detail to "Failed to execute <operation>", 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Handle Bun errno separately from the Node SQLite fields. Test a real Node SQL failure and wrapped Bun errors without copying query data.
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.6 KiB 13.8 KiB +170 B (+1.2%) 15.1 KiB
Codex Thread snapshot wire 7.0 KiB 7.0 KiB +4 B (+0.1%) 7.3 KiB
Codex Live turn WebSocket wire 6.6 KiB 6.7 KiB +166 B (+2.5%) 7.8 KiB
Codex Live turn WebSocket decoded 57.1 KiB 58.5 KiB +1.4 KiB (+2.5%) 66.4 KiB
Codex Live turn messages 10 10 0 (0.0%) 21
Claude Total thread wire 13.7 KiB 13.8 KiB +18 B (+0.1%) 15.1 KiB
Claude Thread snapshot wire 7.0 KiB 7.0 KiB +6 B (+0.1%) 7.3 KiB
Claude Live turn WebSocket wire 6.7 KiB 6.7 KiB +12 B (+0.2%) 7.8 KiB
Claude Live turn WebSocket decoded 59.3 KiB 59.3 KiB 0 B (0.0%) 66.4 KiB
Claude Live turn messages 10 10 0 (0.0%) 21

Baseline: 0ba06a1 · PR result: 577b098 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 113.8 KiB
  • Claude decoded thread snapshot: 114.5 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@macroscopeapp

macroscopeapp Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 577b098

Macroscope's review found this PR approvable — This is a focused persistence diagnostics fix that preserves the original cause and operation while adding normalized SQLite condition details without exposing query values. Its only production effect is bounded error-message formatting, with focused coverage and no changes to defaults, schemas, workflows, or execution behavior.

You can add or adjust custom eligibility rules. Learn more.

Comment thread apps/server/src/persistence/Errors.ts
@t3dotgg
t3dotgg merged commit dddc0bd into main Sep 4, 2026
23 checks passed
@t3dotgg
t3dotgg deleted the t3code/takeover-merge-pr-4837 branch September 4, 2026 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants