feat(store): conversation/thread persistence (HT-14) - #11
Merged
Conversation
The threading decision (src/mail/thread.ts) produces {conversationId,
threadId} but nothing persisted them. This adds the store layer they land
on, plus the portable DB seam the whole engine will build on.
- src/db/client.ts — thin raw-SQL `Db`/`Queryable` seam + a PGlite
(in-process real Postgres, Apache-2.0) adapter. Same parameterized SQL
runs unmodified against Supabase later; no ORM, no query builder.
- src/db/migrate.ts — forward-only migration runner; migration 001
(conversations + threads) embedded as SQL constants so serverless
bundles stay self-contained (no runtime fs reads).
- src/store/conversations.ts — ConversationStore: createConversation
(conversation + first thread, atomic), appendThread, getConversation.
Resolves specs/mail/threading.md §5's open questions at the storage
layer: a valid token to a CLOSED conversation reopens it; to a DELETED
one returns {ok:false,'deleted'} so the caller starts fresh; to a
MISSING id returns 'not-found'. appendThread locks the conversation row
(FOR UPDATE) so concurrent replies resolve deterministically.
- specs/store/conversations.md — the store contract.
- Tests against real in-memory PGlite (not mocks): atomicity via a real
CHECK-violation rollback, injection-safety via a real DROP TABLE
attempt, closed→reopen, deleted→rejected, missing→not-found, ordering.
133 tests pass; typecheck + Biome clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
📝 WalkthroughWalkthroughAdds a PGlite database abstraction, embedded migration runner, and transactional conversation store. The store persists ordered threads, handles open/closed/deleted/missing statuses, exposes public barrels, and adds specifications and Vitest coverage. ChangesConversation persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IngestionPipeline
participant ConversationStore
participant DbTransaction
participant ConversationsTable
participant ThreadsTable
IngestionPipeline->>ConversationStore: appendThread(conversationId, thread)
ConversationStore->>DbTransaction: begin transaction
DbTransaction->>ConversationsTable: lock conversation row
ConversationsTable-->>ConversationStore: return conversation status
ConversationStore->>ThreadsTable: insert accepted thread
ConversationStore->>ConversationsTable: reopen closed status or update timestamp
ConversationStore-->>IngestionPipeline: return append result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/store/conversations.ts (1)
4-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"agent/assistant mail" conflates two distinct actor types the guidelines require keeping separate.
The module doc (and
specs/store/conversations.mdline 9) describes outbound threads generically as "agent/assistant mail," butNewThread/StoredThreadonly carrydirection: 'inbound' | 'outbound'— there's no field recording whether an outbound message came from a human Agent or an AI Assistant. Precise vocabulary matters here per the guideline; either distinguish the two in prose (e.g. "outbound mail from a support Agent or AI Assistant") or, if this store layer intentionally doesn't track authorship yet, say so explicitly rather than using the slash-compound as if the distinction doesn't matter.As per coding guidelines, "human support staff are
Agents, and AI actors areAssistants; never conflate them in schema, code, documentation, or prose."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/conversations.ts` around lines 4 - 9, The conversation store documentation conflates human Agents and AI Assistants. Update the module documentation near createConversation/appendThread and the corresponding conversations specification to explicitly describe outbound mail as coming from a support Agent or AI Assistant, and state that this store currently tracks only direction and does not record authorship.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/client.test.ts`:
- Around line 21-38: The test’s finally block removes dataDir while the second
database instance remains open. Update the cleanup around the createPgliteDb
persistence test to clear the first closed db handle and close the active db
instance before calling rm, ensuring teardown is safe across platforms.
In `@src/db/client.ts`:
- Line 20: Update the PGlite license note in the documentation comment to state
that it is dual-licensed under Apache-2.0 and the PostgreSQL License, or remove
the license parenthetical entirely.
- Around line 143-145: Update createPgliteDb to distinguish an explicitly
provided empty dataDir from an omitted option: branch on dataDir !== undefined
and either pass it to PGlite.create or reject blank values with a clear error,
rather than silently creating an in-memory database.
In `@src/db/migrate.ts`:
- Around line 122-138: Move migration discovery and pending selection from the
initial read in migrate() into a transaction protected by the same lock used for
applying migrations, ensuring concurrent calls serialize before reading
_migrations and executing DDL. Keep each migration’s recording atomic, and add a
test that invokes migrate() concurrently to verify migrations run once without
races or duplicate entries; retain the existing sequential repeat-call coverage.
In `@src/store/conversations.ts`:
- Around line 244-247: The thread query in the conversation-loading logic orders
same-timestamp rows by random UUID, violating deterministic oldest-first
ordering. Update the query in the `threadRows` retrieval to use a time-ordered
creation field as the tiebreaker, such as `created_at` with sufficient precision
or the thread’s monotonic sequence column, and preserve deterministic ascending
ordering.
---
Nitpick comments:
In `@src/store/conversations.ts`:
- Around line 4-9: The conversation store documentation conflates human Agents
and AI Assistants. Update the module documentation near
createConversation/appendThread and the corresponding conversations
specification to explicitly describe outbound mail as coming from a support
Agent or AI Assistant, and state that this store currently tracks only direction
and does not record authorship.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ebdb531-77bc-48b9-90ae-7c454ddb41eb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.jsonspecs/mail/threading.mdspecs/store/conversations.mdsrc/db/client.test.tssrc/db/client.tssrc/db/index.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/store/conversations.test.tssrc/store/conversations.tssrc/store/index.tsvitest.config.ts
Comment on lines
+244
to
+247
| const threadRows = await db.query<ThreadRow>( | ||
| `SELECT ${THREAD_COLUMNS} FROM threads WHERE conversation_id = $1 ORDER BY created_at, id`, | ||
| [conversationId], | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the migration for how thread/conversation ids are generated
fd migrate -e ts
rg -n -B2 -A2 'id\s+uuid|gen_random_uuid|uuid_generate|DEFAULT' src/db/migrate.ts 2>/dev/nullRepository: Helpthread/helpthread
Length of output: 1704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/store/conversations.ts (relevant slice) =="
sed -n '220,270p' src/store/conversations.ts
echo
echo "== search for oldest-first / stable ordering docs =="
rg -n "oldest-first|stable|ORDER BY created_at, id|created_at, id|threads" src/store/conversations.ts src/db/migrate.ts README.md docs 2>/dev/null || true
echo
echo "== migration test references =="
sed -n '1,220p' src/db/migrate.test.tsRepository: Helpthread/helpthread
Length of output: 5679
Use a time-ordered tiebreak here. gen_random_uuid() is random, so ORDER BY created_at, id can still return same-tick threads in arbitrary order and break the oldest-first contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/conversations.ts` around lines 244 - 247, The thread query in the
conversation-loading logic orders same-timestamp rows by random UUID, violating
deterministic oldest-first ordering. Update the query in the `threadRows`
retrieval to use a time-ordered creation field as the tiebreaker, such as
`created_at` with sufficient precision or the thread’s monotonic sequence
column, and preserve deterministic ascending ordering.
- migrate.ts (Major): take a transaction-scoped Postgres advisory lock and run the whole migrate — lock, ensure _migrations, read applied, apply pending — in ONE transaction, so two concurrent migrate() callers on real multi-connection Postgres serialize instead of racing on the same DDL. (No-op under single-connection PGlite; documented as such.) - client.ts: branch createPgliteDb on `dataDir !== undefined`, not truthiness, and reject an explicitly-empty dataDir loudly rather than silently degrading to in-memory. - client.ts: correct the PGlite license note — it's dual-licensed Apache-2.0 or the PostgreSQL License. - client.test.ts: close the open Db handle before rm-ing its dataDir, so teardown is safe on platforms that refuse to unlink open files. 133 tests pass; typecheck + Biome clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/db/migrate.ts (1)
164-166: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAvoid splitting migration SQL on every semicolon.
splitStatements()breaks valid SQL containing semicolons inside string literals, comments, or dollar-quoted function/trigger bodies. Store migrations as statement arrays or use a PostgreSQL-aware splitter, and add a fixture covering this case.Suggested direction
- for (const statement of splitStatements(migration.sql)) { + for (const statement of migration.statements) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrate.ts` around lines 164 - 166, Replace the naive splitStatements-based iteration in the migration runner with statement arrays stored in migrations or a PostgreSQL-aware SQL splitter that preserves semicolons inside literals, comments, and dollar-quoted bodies. Update the migration loading/parsing flow and add a fixture plus test covering these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/db/migrate.ts`:
- Around line 164-166: Replace the naive splitStatements-based iteration in the
migration runner with statement arrays stored in migrations or a
PostgreSQL-aware SQL splitter that preserves semicolons inside literals,
comments, and dollar-quoted bodies. Update the migration loading/parsing flow
and add a fixture plus test covering these cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c3ce9ef-da1c-4a72-8483-f98584b84c5a
📒 Files selected for processing (3)
src/db/client.test.tssrc/db/client.tssrc/db/migrate.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/db/client.test.ts
- src/db/client.ts
This was referenced Jul 10, 2026
Merged
Merged
This was referenced Jul 19, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes HT-14. The fourth mail-engine increment: the store the threading decision lands on, plus the portable DB seam the rest of the engine builds on.
What's here
src/db/client.ts— a thin raw-SQLDb/Queryableseam with a PGlite (in-process real Postgres, Apache-2.0) adapter. The same parameterized$1SQL runs unmodified against Supabase later — no ORM, no query builder, deliberately.src/db/migrate.ts— a forward-only migration runner. Migration 001 (conversations+threads) is embedded as SQL string constants so a serverless bundle stays self-contained (no runtime filesystem reads).src/store/conversations.ts—ConversationStore:createConversation(conversation + first thread, atomic),appendThread,getConversation.specs/store/conversations.md— the store contract.Resolves the threading-spec §5 open questions
A verified reply token can point at a conversation that's since changed. This layer decides what happens (and
specs/mail/threading.md§5 is updated to record it):{ ok: false, reason: 'deleted' }, nothing inserted — caller starts a fresh conversation rather than resurrecting a deleted one.{ ok: false, reason: 'not-found' }.appendThreadtakes aSELECT ... FOR UPDATErow lock so concurrent replies to the same conversation resolve deterministically.Testing
Against real in-memory PGlite, not mocks — atomicity via a genuine CHECK-constraint rollback (asserts zero orphan rows), injection-safety via a real
DROP TABLEattempt (asserts the table survives), plus closed→reopen, deleted→rejected, missing→not-found, and thread ordering.133tests pass; typecheck and Biome clean.Note for review
migrate.tssplits migration SQL on;because the thinqueryseam runs one statement per call (Postgres extended-query protocol rejects multi-statement strings). Safe here — migration bodies are first-party constants with no semicolons in literals — and documented inline, but flagged in case we'd rather grow theDbseam anexecmethod than keep a text splitter. Standard persistence (not crypto/threading-authority), so CodeRabbit + review; Codex not required.🤖 Generated with Claude Code
Summary by CodeRabbit