TRA-1: Config + Persistence Foundation - #1
Conversation
📝 WalkthroughWalkthroughPR introduces a complete persistence foundation: TypeScript configuration system for validating Drizzle ORM paths and backup settings, SQLite schema defining six tables for memories with extraction/backup status tracking, database connection initialization with Bun SQLite runtime selection, repository pattern for type-safe queries, Drizzle migrations, and comprehensive tests validating configuration constraints and database operations. ChangesFoundation Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
34f14b0 to
bd689a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/server/config/config.test.ts (1)
56-69: ⚡ Quick winAdd a regression test for
databasePathcontainment ruleThis suite should also assert rejection when
databasePathis insideprojectPath, since that’s a core invariant enforced byvalidateTraumaConfig.🤖 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 `@tests/server/config/config.test.ts` around lines 56 - 69, Add a regression test mirroring the existing "rejects storePath outside projectPath" case but for databasePath: use createTempRoot() and writeConfig(...) to set projectPath: "./data" and databasePath: "./data/.trauma/trauma.sqlite" (or any path that resolves inside projectPath), then call loadTraumaConfig({ configPath }) and assert it throws (e.g. expect(() => loadTraumaConfig({ configPath })).toThrow(...)). Place the test alongside the existing one and reference loadTraumaConfig, validateTraumaConfig, writeConfig, and createTempRoot so the containment rule for databasePath is covered.
🤖 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 `@drizzle/0000_rapid_maria_hill.sql`:
- Around line 31-34: The migration currently defines extraction_status and
backup_status as unconstrained text columns; update the create table statement
in this migration to add DB-level CHECK constraints (or an ENUM type) that
restrict extraction_status and backup_status to the allowed set of values used
by the code (e.g., 'pending','running','failed','complete' or whatever the repo
expects). Modify the column definitions for `extraction_status` and
`backup_status` to include the CHECK (...) clauses (or create/use a named ENUM
and use that type) so invalid values cannot be persisted, and ensure any
INSERTs/defaults in this migration conform to those allowed values.
In `@src/server/config/load.ts`:
- Around line 24-27: The catch block currently throws TraumaConfigError with a
fixed "Missing trauma config" title for any I/O failure; change it to produce a
more accurate top-level message (e.g., "Failed to load trauma config") or branch
on error properties (errno/code) to distinguish ENOENT vs permission/other
errors, and include configPath plus formatUnknownError(error) in the details;
update the throw of TraumaConfigError at the catch in load (referencing
TraumaConfigError, configPath, and formatUnknownError) so the main message
reflects the actual failure type while keeping the detailed array entry with
formatted error information.
In `@src/server/db/connection.ts`:
- Around line 29-45: initializeDatabase currently opens the SQLite file and
returns repositories without applying the bundled Drizzle schema/migrations,
causing "no such table" errors on a fresh DB; modify initializeDatabase (before
calling createRepositories) to apply the shipped schema or Drizzle
migrations—e.g., load and execute the packaged SQL schema or invoke the Drizzle
migration runner (or a migrate/applyMigrations helper) against the sqlite
instance/createDrizzleDatabase(db) so all tables (like memories) are created
before repositories are constructed and returned.
- Around line 49-60: The loadDatabaseConstructor function currently
unconditionally falls back to require("node:sqlite"), which throws
ERR_UNKNOWN_BUILTIN_MODULE on Node < v22.5.0; update loadDatabaseConstructor to
detect Node version (via process.versions.node or a semver check) before trying
the "node:sqlite" builtin and, for older Node versions, import a userland SQLite
module (e.g., require("better-sqlite3") or require("sqlite3") with a compatible
API) or throw a clear error; ensure you reference the same return type
(SQLiteDatabase) and the require("bun:sqlite") path remains first, only
attempting the builtin "node:sqlite" when Node >= 22.5.0.
- Around line 63-87: The fallback branch in createDrizzleDatabase incorrectly
casts the sqlite-proxy driver to BunSQLiteDatabase; remove the "as unknown as
BunSQLiteDatabase" cast and instead let the require("drizzle-orm/sqlite-proxy")
return its actual type (SqliteRemoteDatabase) so the function's return type
reflects the async proxy driver; update the function signature or adjust callers
that rely on Bun-specific behavior (references: createDrizzleDatabase, drizzle
from "drizzle-orm/sqlite-proxy", BunSQLiteDatabase, SqliteRemoteDatabase,
schema, SQLiteDatabase) to accept the proxy's proper type rather than forcing a
Bun driver cast.
In `@src/server/db/schema.ts`:
- Around line 30-35: The TypeScript-only unions on extractionStatus and
backupStatus (fields extraction_status and backup_status) do not enforce allowed
values at the SQLite level; add persistent CHECK constraints on those columns so
the DB rejects invalid strings (e.g., a CHECK that column IN (...allowed literal
values from ExtractionStatus/BackupStatus)); implement this via your schema
builder's check/constraint API for the extractionStatus and backupStatus columns
and add a migration that either converts/cleans or rejects existing invalid rows
before applying the constraint.
---
Nitpick comments:
In `@tests/server/config/config.test.ts`:
- Around line 56-69: Add a regression test mirroring the existing "rejects
storePath outside projectPath" case but for databasePath: use createTempRoot()
and writeConfig(...) to set projectPath: "./data" and databasePath:
"./data/.trauma/trauma.sqlite" (or any path that resolves inside projectPath),
then call loadTraumaConfig({ configPath }) and assert it throws (e.g. expect(()
=> loadTraumaConfig({ configPath })).toThrow(...)). Place the test alongside the
existing one and reference loadTraumaConfig, validateTraumaConfig, writeConfig,
and createTempRoot so the containment rule for databasePath is covered.
🪄 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
Run ID: 3a34e51f-04a7-48bf-993b-6586a94b5a83
📒 Files selected for processing (13)
drizzle/0000_rapid_maria_hill.sqldrizzle/meta/0000_snapshot.jsondrizzle/meta/_journal.jsonsrc/server/config/errors.tssrc/server/config/index.tssrc/server/config/load.tssrc/server/config/types.tssrc/server/db/connection.tssrc/server/db/index.tssrc/server/db/repositories.tssrc/server/db/schema.tstests/server/config/config.test.tstests/server/db/schema.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34f14b0cb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bd689a2 to
c2bfac7
Compare
c2bfac7 to
2e3336d
Compare
There was a problem hiding this comment.
Pull request overview
Implements the TRA-1 foundation for config loading/validation and SQLite persistence, establishing the initial Drizzle schema + migration and wiring up database initialization and CI E2E startup behavior.
Changes:
- Added typed
trauma.config.jsonloading + validation with path resolution and safety constraints (storePathwithinprojectPath,databasePathoutsidestorePath). - Introduced Drizzle SQLite foundation schema, initial migration artifacts, and a minimal repository layer with DB initialization/migrations.
- Updated Playwright
webServercommand to build + start (instead of dev) for CI smoke tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/server/db/schema.test.ts | Adds unit tests asserting schema exports and a smoke check that DB initialization + migrations create the expected tables. |
| tests/server/config/config.test.ts | Adds unit tests for config loading, JSON parse errors, and path constraint validation. |
| src/server/db/schema.ts | Defines the initial Drizzle SQLite schema (tables, indexes, relations) and status CHECK constraints. |
| src/server/db/repositories.ts | Introduces a typed createRepositories entrypoint with an initial memories.findById method. |
| src/server/db/index.ts | Provides DB module exports and a schema table registry for consumers/tests. |
| src/server/db/connection.ts | Implements Bun SQLite-backed DB initialization, pragmas, migrations, and repository wiring. |
| src/server/config/types.ts | Adds types for the trauma config shape and resolved runtime config. |
| src/server/config/load.ts | Implements config file discovery/loading, JSON parsing, validation, and path resolution. |
| src/server/config/index.ts | Public barrel exports for config load/validate/types/errors. |
| src/server/config/errors.ts | Adds a dedicated TraumaConfigError carrying structured issues. |
| playwright.config.ts | Switches Playwright webServer to build && start for CI stability. |
| drizzle/meta/0000_snapshot.json | Records Drizzle schema snapshot metadata for the initial migration. |
| drizzle/meta/_journal.json | Records Drizzle migration journal entries. |
| drizzle/0000_conscious_mikhail_rasputin.sql | Adds the generated initial SQL migration for the foundation schema. |
| drizzle.config.ts | Resolves DB path for Drizzle Kit via env var or trauma.config.json (fallback default). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/server/db/connection.ts`:
- Around line 46-61: The DB handle `sqlite` is opened before several operations
that can throw (`sqlite.exec`, `createDrizzleDatabase`, `applyMigrations`,
`createRepositories`), so if any fail the file descriptor is leaked; wrap the
initialization sequence in a try/catch (declare `sqlite` in an outer scope), and
in the catch ensure you call `sqlite.close()` if `sqlite` is truthy before
rethrowing the error; apply this change around `loadDatabaseConstructor`, `new
Database(...)`, `sqlite.exec(...)`, `createDrizzleDatabase`, `applyMigrations`,
and `createRepositories`, returning or throwing only after the handle is closed.
🪄 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
Run ID: a1d59d46-1535-47ce-b6d8-d4ce88db4c74
📒 Files selected for processing (15)
drizzle.config.tsdrizzle/0000_conscious_mikhail_rasputin.sqldrizzle/meta/0000_snapshot.jsondrizzle/meta/_journal.jsonplaywright.config.tssrc/server/config/errors.tssrc/server/config/index.tssrc/server/config/load.tssrc/server/config/types.tssrc/server/db/connection.tssrc/server/db/index.tssrc/server/db/repositories.tssrc/server/db/schema.tstests/server/config/config.test.tstests/server/db/schema.test.ts
✅ Files skipped from review due to trivial changes (5)
- playwright.config.ts
- src/server/config/index.ts
- tests/server/config/config.test.ts
- src/server/config/types.ts
- drizzle/meta/0000_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/server/config/errors.ts
- drizzle/meta/_journal.json
- src/server/db/index.ts
- src/server/db/repositories.ts
- src/server/db/schema.ts
Summary
Implements the TRA-1 config and persistence foundation:
trauma.config.jsonloading and validation throughsrc/server/config.projectPath,storePath, anddatabasePathto absolute paths relative to the config file.storePathinsideprojectPathanddatabasePathoutside the markdown backup scope (storePath) before startup behavior proceeds.Config Keys Implemented
storePathprojectPathdatabasePathbackup.git.enabledbackup.git.remotebackup.git.branchbackup.git.pushbackup.git.commitMessageTemplateSchema Tables Added
memoriestagscategoriesmemory_tagsmemory_categorieshighlightsMigration Files Created
drizzle/0000_conscious_mikhail_rasputin.sqldrizzle/meta/0000_snapshot.jsondrizzle/meta/_journal.jsonRepository API Names
Future workers should use:
loadTraumaConfig()/validateTraumaConfig()fromsrc/server/configinitializeDatabase()fromsrc/server/dbcreateRepositories()fromsrc/server/dbschemafromsrc/server/dbrepositories.memories.findById()as the initial repository skeletonVerification
Initial handoff:
MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -- bun run test tests/server/config/config.test.ts tests/server/db/schema.test.ts-> passed, 2 files / 5 tests before implementation.MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -- bun run db:generate-> passed; regenerated the initial migration after adding CHECK constraints.MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -- bun run typecheck-> passed.MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -- bun run test-> passed, 3 files / 7 tests.MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -- bun run build-> passed.GIT_DIR=.tmp/gitdir GIT_WORK_TREE=. git diff --checkandgit diff --check --cached-> passed with empty output.typecheck,test, andbuildsuccessfully.CI / Verify-> passed, including baseline verification and E2E smoke tests.Rework commits
4fea9a4ande8107aa:MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -- bun run test tests/server/config/config.test.ts tests/server/db/schema.test.ts-> failed before implementation for relativeconfigPathwithcwdand cwd-coupled migration lookup.MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -- bun run test tests/server/db/schema.test.ts-> failed before the lifecycle fix withcloseCalls: 0, then passed, 1 file / 5 tests after closing SQLite on initialization failure.MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -- bun run typecheck-> passed.MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -- bun run test-> passed, 3 files / 11 tests.MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -- bun run build-> passed.GIT_DIR=.tmp/gitdir GIT_WORK_TREE=. git diff --check-> passed with empty output.typecheck,test, andbuildsuccessfully after each pushed rework commit.Review Follow-up
databasePathagainststorePathrather than the whole git worktree.drizzle.config.tstoTRAUMA_DATABASE_PATHortrauma.config.jsondatabasePath.configPathpluscwd, migrations independent of launch cwd, DB rejection of invalid memory status values, and SQLite handle closure on initialization failure.Notes
.gitwere blocked, so git operations used the in-workspace.tmp/gitdirworkaround..tmp/was not staged.package-lock.jsonexisted before implementation and was intentionally left out of scope.