feat: storage migration sql.js → node:sqlite (1.0.0 prep)#39
Merged
Conversation
Required for node:sqlite DatabaseSync({ timeout }) constructor option,
added in Node 22.16. Phase 2 of the data-integrity plan.
Both ohno-cli and ohno-mcp depend on node:sqlite (Node 22.16+). Without this guard, users on older Node would see "Cannot find module 'node:sqlite'" instead of an actionable error. The guard self-executes on import and is the first import in each entry point, so it runs before any sqlite-bearing code loads. Pure helper checkNodeVersion(version) is unit-tested with 6 synthetic version strings. Phase 2 of the data-integrity plan, task-791a15c4. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rewrites TaskDatabase on top of Node 22.16+'s built-in node:sqlite
(DatabaseSync). All ~40 query methods translated from sql.js patterns
(db.run + getRowsModified, db.exec returning {columns,values},
prepare/bind/step/getAsObject/free) to node:sqlite's
prepare(sql).all/get/run().
- open() asserts journal_mode = DELETE and quick_check = ok with
verbatim error messages; closes db before throwing on either path
- 5-second busy_timeout via DatabaseSync({ timeout: 5000 })
- save()/getSqlJs()/resultToObjects removed; reload() kept as no-op
(cli.test.ts call sites removed in a later task)
- dbPath exposed as a public readonly property for diagnostics
- 4 white-box test sites in db.test.ts updated to the new API
- vitest.config.ts in core and cli get a Vite plugin to work around
vite-node 2.1.x stripping the node: prefix before resolving builtins
Tests: 250/250 ohno-core + 156/156 ohno-cli pass. db.ts net 1874 -> 1587.
Closes task-a5f53eb6 and task-e77e86ca. Phase 2 of the data-integrity plan.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After bumping engines.node to >=22.16.0, update CI to match. - ci.yml: convert single Node 20 to a matrix of 22.x + 24.x with fail-fast: false so we see all version failures - release.yml: pin publish to 22.x (the engine floor — publishing on the lowest supported version catches accidentally-newer syntax) Phase 2 of the data-integrity plan, task-54d3aaf3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kanban.ts had its own independent sql.js usage (initSqlJs, getSqlJs
cache, queryToObjects helper, fs.readFileSync into Uint8Array) that
bypassed ohno-core's TaskDatabase. Migrate to node:sqlite's DatabaseSync
in readonly mode with PRAGMA query_only = ON.
- Open DB once per request via new DatabaseSync(dbPath, { readOnly: true })
- All 7 SELECT queries use db.prepare(sql).all()
- queryToObjects helper deleted
- try/finally ensures db.close() on the error path
Smoke test: exportDatabase against .ohno/tasks.db returns expected counts
(60 tasks, 47 done, 78%, 9 stories, 2 epics).
task-fddb12c3. Unblocks task-860301ca (drop sql.js dep). Phase 2 of the
data-integrity plan.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All call sites migrated to node:sqlite (DatabaseSync) in prior commits. - ohno-core: removed sql.js dep + @types/sql.js devDep (the dependencies block is now empty — node:sqlite is built-in) - ohno-cli: removed sql.js (was a leftover direct dep from when kanban.ts imported it independently) - Lockfile shrinks ~30 lines as sql.js + transitive deps are pruned Verified: all 3 packages build clean, 250 ohno-core + 156 ohno-cli tests pass, grep returns zero sql.js references in src or lockfile. task-860301ca. Phase 2 of the data-integrity plan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds private withTransaction<T>(fn) helper using BEGIN IMMEDIATE so the write lock is acquired at transaction start, eliminating mid-transaction upgrade races between concurrent writers. The catch block attempts ROLLBACK in a nested try/catch so a throwing rollback doesn't shadow the original error. Wrapped 20 public multi-write methods: createTask, updateTask, updateTaskStatus, setHandoffNotes, updateTaskProgress, setBlocker, resolveBlocker, setNeedsRework, updateTaskWip, archiveTask, reopenTask, deleteTask, deleteEpic, deleteStory, addDependency, removeDependency, compactStoryHandoffs, deleteEpicHandoffs, rebuildWorkQueue, and summarizeTaskActivity. Extracted two private impl methods to avoid nested BEGIN IMMEDIATE when internal callers already hold a transaction: - _deleteTaskImpl: called by deleteEpic/deleteStory when cascading - _summarizeTaskActivityImpl: called by updateTaskStatus on auto-summarize Methods with a single write (createStory, updateStory, createEpic, updateEpic, addTaskActivity, recomputeQueueEntry) are intentionally unwrapped. Tests: 250/250 ohno-core + 156/156 ohno-cli pass. task-aba4f48b. Phase 2 of the data-integrity plan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a typed OhnoDatabaseLockedError class (exported from ohno-core) and
wires SQLite error mapping across the three layers that can hit a lock:
db.ts internals, the CLI bin entry, and the MCP tool dispatcher.
ohno-core (db.ts):
- Add private mapSqliteError(e): never helper that inspects e.errcode
(numeric, not e.code which is always 'ERR_SQLITE_ERROR') and throws
OhnoDatabaseLockedError for BUSY (5) and BUSY_SNAPSHOT (261), a plain
bug-flagged Error for LOCKED (6), and re-throws other SQLite errors
with [errcode=N] preserved in the message
- Wrap open()'s DatabaseSync construction + early PRAGMA block so a BUSY
at open time maps cleanly (close db before mapping; pass-through for
our own journal_mode/quick_check assertion errors)
- Wrap withTransaction's BEGIN IMMEDIATE and the inner body so both can
surface as OhnoDatabaseLockedError; rollback errors still swallowed
- Wrap the four single-write public methods (createStory, updateStory,
createEpic, updateEpic) — other writes inherit mapping via withTransaction
ohno-cli (index.ts):
- main().catch() handles OhnoDatabaseLockedError: stderr by default,
{ success: false, error, errorCode } JSON to stdout when --json is in
argv (commander state isn't available at the global handler)
ohno-mcp (server.ts):
- handleTool wraps the switch in try/catch; OhnoDatabaseLockedError
becomes { success: false, error, errorCode } in the tool response;
other errors re-throw to the framework
Drive-by fix: ohno-mcp's vitest.config.ts was missing the node-sqlite-
external plugin, so its tests weren't actually running. Plugin added;
5 pre-existing whitebox sites in server.test.ts that used the old sql.js
db.run(sql, [array]) shape now use prepare(sql).run(...args) with a
DatabaseSync type cast, matching the same fix already done in db.test.ts.
Spec said e.code is the SQLite code string; verified experimentally that
node:sqlite (Node 22.16+/24) actually uses e.errcode (numeric) — the
implementation matches reality and the test at db.test.ts:2083 provokes
a real SQLITE_BUSY to lock that in.
Tests: 258 ohno-core + 159 ohno-cli + 232 ohno-mcp = 649 pass.
task-1064ddb2. Phase 2 of the data-integrity plan.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Migrates the project’s storage layer from sql.js to Node 22.16+ built-in node:sqlite (DatabaseSync) to eliminate data loss under concurrent writers, and adds explicit handling for SQLite lock contention.
Changes:
- Rewrote
TaskDatabaseto usenode:sqlite, addedBEGIN IMMEDIATEtransactions, and introducedOhnoDatabaseLockedErrormapping for SQLITE_BUSY(-SNAPSHOT). - Updated CLI/MCP entrypoints to enforce Node >= 22.16.0 and to surface lock errors in structured/user-friendly forms.
- Updated CI to test Node 22.x + 24.x and removed
sql.jsdependencies.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/package-lock.json | Removes sql.js-related deps; updates engines to Node >= 22.16.0. |
| packages/ohno-mcp/vitest.config.ts | Adds a Vite/Vitest plugin to correctly resolve node:sqlite in tests. |
| packages/ohno-mcp/src/server.ts | Wraps tool handling with lock-error-to-structured-response mapping. |
| packages/ohno-mcp/src/server.test.ts | Updates white-box tests for DatabaseSync and adds handleTool error-shape tests. |
| packages/ohno-mcp/src/node-guard.ts | Adds Node version guard for MCP entry. |
| packages/ohno-mcp/src/index.ts | Imports guard before server startup. |
| packages/ohno-mcp/package.json | Bumps engines to Node >= 22.16.0. |
| packages/ohno-core/vitest.config.ts | Adds the node:sqlite Vitest resolver plugin. |
| packages/ohno-core/src/index.ts | Exports OhnoDatabaseLockedError from core. |
| packages/ohno-core/src/db.ts | Replaces sql.js DB layer with DatabaseSync, adds transactions + lock error mapping. |
| packages/ohno-core/src/db.test.ts | Updates tests for DatabaseSync access and adds error-code/shape tests. |
| packages/ohno-core/package.json | Drops sql.js deps; bumps engines to Node >= 22.16.0. |
| packages/ohno-cli/vitest.config.ts | Adds the node:sqlite Vitest resolver plugin. |
| packages/ohno-cli/src/node-guard.ts | Adds Node version guard for CLI entry. |
| packages/ohno-cli/src/node-guard.test.ts | Adds version guard unit tests. |
| packages/ohno-cli/src/kanban.ts | Switches kanban export to readonly DatabaseSync + PRAGMA query_only. |
| packages/ohno-cli/src/index.ts | Adds guard import and CLI lock-error handler (stderr/JSON). |
| packages/ohno-cli/src/cli.test.ts | Adds tests for CLI entrypoint lock-error output behavior. |
| packages/ohno-cli/package.json | Drops sql.js dep; bumps engines to Node >= 22.16.0. |
| .github/workflows/release.yml | Updates release workflow to Node 22.x. |
| .github/workflows/ci.yml | Updates CI matrix to Node 22.x + 24.x. |
Files not reviewed (1)
- packages/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
Addressed the Copilot review feedback in eaaf358:\n\n- Moved |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- packages/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
packages/ohno-mcp/src/server.ts:724
- The switch cases inside handleTool() are now inconsistently indented (some
caselabels are aligned differently). This makes the handler harder to scan and increases the chance of mistakes during future edits; re-indent the switch body consistently within the new try/catch wrapper.
switch (name) {
case "get_project_status":
return database.getProjectStatus();
case "get_session_context":
return database.getSessionContext();
case "get_tasks": {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
Addressed the second Copilot review pass in 6ad8a30:\n\n- Preserved original node:sqlite errors with |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- packages/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
Addressed the latest Copilot review pass in c53d808:\n\n- Corrected |
srstomp
added a commit
that referenced
this pull request
Apr 26, 2026
feat: storage migration sql.js → node:sqlite (1.0.0 prep)
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
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
Phase 2 of the data-integrity plan (#36): replace sql.js with Node 22.16+'s
built-in
node:sqlite(DatabaseSync). Fixes data loss when concurrentohno-mcp/cli/sqlite3 writers race against one
.ohno/tasks.db.BREAKING: requires Node.js >= 22.16.0 (Node 22 LTS or newer).
Users on Node 18/20 should pin to
0.20.x(which contains the worktree fix).Commits
65f1937Node version guard at cli/mcp entry points948ba18Replace sql.js with node:sqlite (db.ts core rewrite + ~40 querymethod translations + 4 white-box test sites updated)
a0196e6CI matrix → Node 22.x + 24.x (drops 20)7e01c04Migrate kanban.ts to node:sqlite (readOnly + PRAGMA query_only)0108e0fDrop sql.js + @types/sql.js dependenciese215355Wrap multi-statement writes in BEGIN IMMEDIATE transactions(20 methods + 2 extracted impl methods to avoid nested BEGIN)
981b1d6Map SQLITE_BUSY/LOCKED to OhnoDatabaseLockedError; CLI printsto stderr (or JSON in --json mode), MCP returns structured error response
Tests
node:sqlite plugin; fixed + 5 white-box tests updated)
Test plan
error wording — currently no dedicated coverage
🤖 Generated with Claude Code