fix(db/sqlite): defer user_id index creation until after column ALTER (v0.4.1 hotfix)#1792
Merged
Merged
Conversation
createSchema() ran two CREATE INDEX statements referencing user_id on remote_agent_conversations and remote_agent_workflow_runs. On databases created before v0.4.0 those columns don't exist yet — they're added by migrateColumns(), which runs AFTER createSchema(). The index creation aborted the entire createSchema() exec block, the constructor threw, and every subsequent operation failed with "no such column: user_id". New installs were unaffected because the columns exist in the same schema. Moves both CREATE INDEX statements into migrateColumns() so they run after the matching ALTER TABLE. idx_user_identities_user_id stays in createSchema() because user_identities is a new table whose user_id column always exists. Adds a regression test that seeds a pre-0.4.0 schema (no user_id columns on conversations/workflow_runs/messages, no created_by_user_id on isolation_environments) and asserts SqliteAdapter construction completes, migrates the columns, and creates both indexes. Caught by /test-release brew 0.4.0.
📝 WalkthroughWalkthroughSQLite schema migration is reorganized to create user_id indexes in the migration path rather than in initial schema setup, ensuring pre-0.4.0 databases upgrade successfully. A regression test validates the upgrade by seeding a pre-0.4.0 database, confirming the migrated schema contains the new columns and indexes, and verifying queryability. ChangesPre-0.4.0 SQLite Schema Upgrade
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
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 `@packages/core/src/db/adapters/sqlite.test.ts`:
- Around line 206-267: The seed Database instance named raw (created via new
Database(dbPath)) is closed only after raw.exec(...), so if exec throws the
handle is leaked; wrap the exec call in a try/finally (or try/catch/finally) so
that raw.close() is always called in the finally block (i.e., try {
raw.exec(...); } finally { raw.close(); }) to guarantee cleanup of the Database
regardless of errors.
🪄 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: 4f2d7caa-78be-4282-9309-bf727a8f0e65
📒 Files selected for processing (2)
packages/core/src/db/adapters/sqlite.test.tspackages/core/src/db/adapters/sqlite.ts
Comment on lines
+206
to
+267
| const raw = new Database(dbPath); | ||
| raw.exec(` | ||
| CREATE TABLE remote_agent_codebases ( | ||
| id TEXT PRIMARY KEY DEFAULT (lower(hex(randomblob(16)))), | ||
| name TEXT NOT NULL, | ||
| default_cwd TEXT NOT NULL, | ||
| repository_url TEXT, | ||
| created_at TEXT DEFAULT (datetime('now')) | ||
| ); | ||
|
|
||
| CREATE TABLE remote_agent_conversations ( | ||
| id TEXT PRIMARY KEY DEFAULT (lower(hex(randomblob(16)))), | ||
| platform_type TEXT NOT NULL, | ||
| platform_conversation_id TEXT NOT NULL, | ||
| ai_assistant_type TEXT, | ||
| codebase_id TEXT, | ||
| cwd TEXT, | ||
| isolation_env_id TEXT, | ||
| hidden INTEGER DEFAULT 0, | ||
| deleted_at TEXT, | ||
| last_activity_at TEXT, | ||
| created_at TEXT DEFAULT (datetime('now')) | ||
| ); | ||
|
|
||
| CREATE TABLE remote_agent_workflow_runs ( | ||
| id TEXT PRIMARY KEY DEFAULT (lower(hex(randomblob(16)))), | ||
| workflow_name TEXT NOT NULL, | ||
| conversation_id TEXT, | ||
| codebase_id TEXT, | ||
| status TEXT DEFAULT 'pending', | ||
| user_message TEXT, | ||
| metadata TEXT DEFAULT '{}', | ||
| parent_conversation_id TEXT, | ||
| last_activity_at TEXT, | ||
| created_at TEXT DEFAULT (datetime('now')) | ||
| ); | ||
|
|
||
| CREATE TABLE remote_agent_messages ( | ||
| id TEXT PRIMARY KEY DEFAULT (lower(hex(randomblob(16)))), | ||
| conversation_id TEXT, | ||
| role TEXT NOT NULL, | ||
| content TEXT NOT NULL, | ||
| metadata TEXT DEFAULT '{}', | ||
| created_at TEXT DEFAULT (datetime('now')) | ||
| ); | ||
|
|
||
| CREATE TABLE remote_agent_isolation_environments ( | ||
| id TEXT PRIMARY KEY DEFAULT (lower(hex(randomblob(16)))), | ||
| codebase_id TEXT NOT NULL, | ||
| workflow_type TEXT NOT NULL, | ||
| workflow_id TEXT NOT NULL, | ||
| provider TEXT NOT NULL DEFAULT 'worktree', | ||
| working_path TEXT NOT NULL, | ||
| branch_name TEXT NOT NULL, | ||
| created_by_platform TEXT, | ||
| metadata TEXT DEFAULT '{}', | ||
| status TEXT NOT NULL DEFAULT 'active', | ||
| created_at TEXT DEFAULT (datetime('now')), | ||
| updated_at TEXT DEFAULT (datetime('now')) | ||
| ); | ||
| `); | ||
| raw.close(); |
There was a problem hiding this comment.
Close the seed Database in a finally block.
If raw.exec(...) throws, raw.close() is skipped, which can leak the handle and make cleanup brittle.
Proposed fix
- const raw = new Database(dbPath);
- raw.exec(`
+ const raw = new Database(dbPath);
+ try {
+ raw.exec(`
CREATE TABLE remote_agent_codebases (
id TEXT PRIMARY KEY DEFAULT (lower(hex(randomblob(16)))),
name TEXT NOT NULL,
default_cwd TEXT NOT NULL,
repository_url TEXT,
created_at TEXT DEFAULT (datetime('now'))
);
@@
CREATE TABLE remote_agent_isolation_environments (
id TEXT PRIMARY KEY DEFAULT (lower(hex(randomblob(16)))),
codebase_id TEXT NOT NULL,
workflow_type TEXT NOT NULL,
workflow_id TEXT NOT NULL,
provider TEXT NOT NULL DEFAULT 'worktree',
working_path TEXT NOT NULL,
branch_name TEXT NOT NULL,
created_by_platform TEXT,
metadata TEXT DEFAULT '{}',
status TEXT NOT NULL DEFAULT 'active',
created_at TEXT DEFAULT (datetime('now')),
updated_at TEXT DEFAULT (datetime('now'))
);
- `);
- raw.close();
+ `);
+ } finally {
+ raw.close();
+ }As per coding guidelines, “For unit tests, test pure functions with mocked external dependencies; for integration tests, test database operations with test database and end-to-end flows with real orchestrator; clean up test data after each test”.
🤖 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 `@packages/core/src/db/adapters/sqlite.test.ts` around lines 206 - 267, The
seed Database instance named raw (created via new Database(dbPath)) is closed
only after raw.exec(...), so if exec throws the handle is leaked; wrap the exec
call in a try/finally (or try/catch/finally) so that raw.close() is always
called in the finally block (i.e., try { raw.exec(...); } finally { raw.close();
}) to guarantee cleanup of the Database regardless of errors.
Collaborator
Author
PR Review Summary (multi-agent)Reviewed by: Verdict: APPROVE WITH MINOR FIXES — the production fix is correct and the regression test exercises the exact failure path. Two findings are worth addressing in this commit since the lines are already being touched; the rest are low-impact. Critical IssuesNone. Important Issues (2)
Suggested fix for I1 — split the index call into its own try {
this.db.run(
'CREATE INDEX IF NOT EXISTS idx_conversations_user_id ON remote_agent_conversations(user_id) WHERE user_id IS NOT NULL'
);
} catch (e: unknown) {
getLog().warn({ err: e as Error }, 'db.sqlite_migration_conversations_user_id_index_failed');
}Same split for Suggestions (5)
Strengths
Documentation ImpactNo documentation changes required. The fix is internal to the SQLite adapter; the migration is described as "automatic" in the PR body and matches the existing CLAUDE.md guidance ("SQLite is the default — zero setup"). Recommended Actions
Given this is a v0.4.1 release-blocker hotfix, I1 + I2 + S1 + S2 are reasonable to land in the same PR. S3-S5 are optional polish. |
Merged
jojorgen
pushed a commit
to cognoco/zdx-archon
that referenced
this pull request
Jun 4, 2026
Upstream base advanced 0adec22 → c839bcc (40 commits). Notable: GitHub App multi-installation routing, user_id plumbing, Slack UX upgrade, OpenCode + Copilot community providers, experimental /console UI, Codex AbortController crash fixes, large-node-output temp-file spill (coleam00#1717), sqlite user_id migration + index-order hotfix (coleam00#1792). Conflicts resolved (4): - packages/providers/package.json: kept upstream's new deps (copilot, opencode, pi 0.73.1) + retained our Patch-4 codex-sdk 0.132.0 pin. - packages/web/src/App.tsx: kept our ThemeProvider wrapper around upstream's new /console route. - packages/web/src/components/layout/TopNav.tsx: kept both upstream's console CTA and our ThemeToggle. - bun.lock: regenerated via bun install against merged package.json. Fork patches preserved: dag-executor.ts auto-merged cleanly — our 100MB maxBuffer, 30min subprocess timeout + idle_timeout fallback, SIGTERM error classifier, and per-workflow ARCHON_DEPLOYMENT_ENVIRONMENT injection all coexist with upstream's new NODE_OUTPUT_FILE_THRESHOLD temp-file logic. Verified present in the rebuilt darwin-arm64 binary. KNOWN CAVEAT (pre-existing on upstream/dev, not introduced here): the Pi community provider fails type-check — pi-coding-agent 0.73.1 dropped the `codingTools` export and changed ExtensionUIContext, but upstream's options-translator.ts / ui-context-stub.ts still use the old API. Pi is a lazy-loaded community provider (builtIn:false) we don't use; bun build strips types so the compiled binary is unaffected. `bun run validate` will be red on this until upstream fixes pi or we pin pi back. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Summary
Error: no such column: user_id.brew upgrade archonorcurl install.shright now ends up with a broken install. New users (fresh~/.archon/archon.db) are unaffected.CREATE INDEXstatements foruser_idonconversationsandworkflow_runsare moved out ofcreateSchema()and intomigrateColumns(), right after theALTER TABLEthat adds the column.idx_user_identities_user_idstays increateSchema()—user_identitiesis a new table created by the same exec block, so the column always exists when the index is created. No behavior change for new installs.UX Journey
Before (v0.4.0)
After (v0.4.1)
Architecture Diagram
Before
After
Connection inventory:
createSchema()idx_conversations_user_idcreateSchema()idx_workflow_runs_user_idmigrateColumns()idx_conversations_user_idmigrateColumns()idx_workflow_runs_user_idLabel Snapshot
risk: low(3-line code change + comprehensive test)size: Scorecore:db/sqlite-adapterChange Metadata
bugcoreLinked Issue
/test-release brew 0.4.0immediately after v0.4.0 was publishedValidation Evidence (required)
Commands and result:
bun run validate # exit 0 — all checks (type-check, lint, format:check, tests, bundled checks) passSqliteAdapter > upgrade from pre-0.4.0 schema > migrates user_id columns and indexes onto an existing pre-0.4.0 databasegit stash push <prod file>+ re-running the test)~/.archon/archon.db(which was in the v0.4.0-broken state): all user_id columns added, all indexes created,archon isolation listreturned 6 environments cleanly.Security Impact (required)
NoNoNoNoCompatibility / Migration
YesNoYes — automatic. SQLite migration runs on first SqliteAdapter construction. No user action required beyond upgrading the binary.Human Verification (required)
user_id/created_by_user_idcolumns added, both new indexes created,isolation listsucceeds.WHERE user_id IS NOT NULLpartial index syntax is preserved on the migration path.migrations/, not this in-process schema sync).Side Effects / Blast Radius (required)
DATABASE_URLis unset.IF NOT EXISTS) and runs inside the same try/catch as the ALTER TABLE, so any unexpected failure is swallowed and logged like the existing migration errors.Rollback Plan (required)
brew install coleam00/archon/archon@0.3.12).Error: no such column: user_idin any DB-touching command.Risks and Mitigations
db.sqlite_migration_*_columns_failedwarnings already exist). Fixing the silent-failure pattern is a separate refactor. Releasing v0.4.1 is the priority.Summary by CodeRabbit