fix: read OpenCode v1.2+ sessions from SQLite database

[pedramamini]

Summary

Fixes #387 — OpenCode v1.2+ moved session storage from JSON files (~/.local/share/opencode/storage/) to SQLite (~/.local/share/opencode/opencode.db). Maestro only read JSON files, so sessions created after upgrading to v1.2 were invisible while older migrated sessions (with JSON files still on disk) continued to appear.

• SQLite reading via better-sqlite3 (already a project dependency) — queries project, session, message, and part tables matching OpenCode's v1.2+ Drizzle ORM schema
• Hybrid fallback — tries SQLite first, falls back to JSON for pre-v1.2 installs, merges and deduplicates by session ID during the migration period
• Read-only access — opens the DB in read-only mode; blocks deleteMessagePair for SQLite-backed sessions
• SSH note — remote sessions still use JSON-only path (no remote SQLite access)

Needs validation

• Test against an actual OpenCode v1.2+ installation with real SQLite data
• Verify old JSON-only sessions still appear correctly
• Verify dedup works when both JSON and SQLite have the same session
• Check SSH remote session listing is unaffected

Test plan

• 12 unit tests added covering SQLite listing, token aggregation, message reading, JSON fallback, deduplication, delete rejection, and path resolution
• All existing storage tests pass (43 total across both storage test files)
• TypeScript and ESLint clean

[claude]

PR Review: fix: read OpenCode v1.2+ sessions from SQLite database

Overall this is a well-structured fix for a real user-facing regression. The hybrid fallback strategy is the right approach for a migration period. Below are my findings organized by severity.

---

Bugs / Correctness Issues

1. DB connection leak in loadSessionMessagesSqlite when messageRows.length === 0

At line ~1009, when messageRows is empty the method returns null early — but this early return is inside the try block, so it bypasses the finally { db.close() }. The openOpenCodeDb() call at the top already opened the connection.

// Current code (leaks connection):
if (messageRows.length === 0) return null;  // ← jumps past finally

// Fix: assign and fall through, or close explicitly before returning
if (messageRows.length === 0) {
    db.close();   // or let the finally handle it by not returning early
    return null;
}

Wait — actually better-sqlite3's try/finally pattern should still execute finally on early returns within the same try block. Double-check: yes, finally runs even on early return. So this is safe — but the early return pattern at line ~1001 (if (!tableExists(db, 'session') || !tableExists(db, 'project')) return null;) in listSessionsSqlite is outside the try/finally block and after db has been opened. That one does leak. The check happens after openOpenCodeDb() but before the try:

private listSessionsSqlite(projectPath: string): AgentSessionInfo[] | null {
    const db = openOpenCodeDb();
    if (!db) return null;

    try {
        if (!tableExists(db, 'session') || !tableExists(db, 'project')) {
            return null;  // ← db is never closed here
        }
        ...
    } finally {
        db.close();
    }
}

This early return on schema check leaks the DB file handle. Move the tableExists check inside the try block, or call db.close() before returning.

---

2. Null-dereference risk in readSessionMessages and searchSessions

loadSessionMessagesSqlite can return null, and loadSessionMessages (the JSON path) can also theoretically throw. The fallback chain:

loaded = this.loadSessionMessagesSqlite(sessionId);
if (!loaded) {
    loaded = await this.loadSessionMessages(sessionId);
}
const { messages, parts } = loaded;  // ← loaded could still be null if JSON also fails?

loadSessionMessages is declared async and returns Promise<{...}>, not Promise<{...} | null>, so it shouldn't return null — but the destructure on loaded without a null guard is risky if the contract ever changes. A defensive check would prevent future crashes.

---

3. WAL pragma on a read-only database

In openOpenCodeDb:

db.pragma('journal_mode = WAL');

Setting journal_mode to WAL on a read-only connection will throw in some better-sqlite3 versions, or at minimum it's a no-op that may generate a warning. WAL mode is already controlled by the writer; a reader doesn't need to set it. Remove this pragma or use PRAGMA wal_checkpoint if needed.

---

4. SQL injection via template literal in listSessionsSqlite

const placeholders = matchingProjectIds.map(() => '?').join(',');
const sessions = db
    .prepare(`SELECT ... FROM session WHERE project_id IN (${placeholders}) ...`)
    .all(...matchingProjectIds);

matchingProjectIds values come from the database itself (not user input), so this isn't an exploitable injection in practice. However, the prepare+all(...spread) pattern with a dynamic number of ? placeholders is fragile — if matchingProjectIds is somehow empty here it would generate WHERE project_id IN () which is invalid SQL. The empty-array case is guarded by the if (matchingProjectIds.length === 0) check above, so this specific path is safe, but it's worth a comment to that effect.

---

5. deleteMessagePair opens a SQLite connection just to check if the session exists

const sqliteResult = this.loadSessionMessagesSqlite(sessionId);
if (sqliteResult && sqliteResult.messages.length > 0) {
    return { success: false, error: '...' };
}

This opens and fully queries the DB (loading all messages and parts for the session) purely to determine if deletion is blocked. A lighter check — just verifying whether the DB file exists and the session row exists — would be more efficient:

// Cheaper check: does the session exist in SQLite?
if (fsSync.existsSync(OPENCODE_DB_PATH)) {
    const db = openOpenCodeDb();
    if (db) {
        try {
            const row = db.prepare('SELECT id FROM session WHERE id = ?').get(sessionId);
            if (row) return { success: false, error: '...' };
        } finally {
            db.close();
        }
    }
}

---

Design / Architecture Notes

6. DB opened and closed on every call — consider a cached connection or connection pool

openOpenCodeDb() is called on every listSessionsSqlite and loadSessionMessagesSqlite invocation, opening and closing the file each time. For a read-only database this is acceptable, but if listSessions is called frequently (e.g., polling), this will create observable overhead. Consider holding an open read-only handle for the lifetime of the storage instance, or at minimum caching it with a short TTL.

---

7. Both listSessions paths are always run even when SQLite has data

const sqliteSessions = this.listSessionsSqlite(projectPath);
const jsonSessions = await this.listSessionsJson(projectPath);

Both are always evaluated (SQLite synchronously, JSON asynchronously). On a post-migration v1.2+ install with no JSON files, listSessionsJson will do a filesystem walk that returns nothing. This is minor but adds latency. Consider short-circuiting JSON loading if the SQLite query returned confidently non-empty results.

---

8. getSessionPath returns the same DB path for all sessions

if (fsSync.existsSync(OPENCODE_DB_PATH)) {
    return OPENCODE_DB_PATH;  // same path regardless of sessionId
}

If any consumer of getSessionPath uses the returned path to identify a specific session (e.g., file watching, display), returning the shared DB path for every session ID may be surprising. The old behavior returned a session-specific directory. The new behavior is arguably correct (the session is in that DB file), but worth documenting the behavioral change.

---

Test Coverage

9. Missing test: readSessionMessages SQLite→JSON fallback path

There's a test for SQLite message reading and one for delete rejection, but no test verifying that readSessionMessages falls back to JSON when SQLite returns null/no-messages. This is a meaningful code path since it also runs inside the searchSessions loop.

10. Missing test: empty matchingProjectIds path in listSessionsSqlite

The "fall back to sessions with matching directory field" branch (when no project row matches) is not covered by the unit tests. The setupSqliteDb helper always populates at least one matching project.

11. Mock createMockStatement used but not defined in test file

In the deduplication test (line ~354 in the test file), createMockStatement() is called but isn't defined anywhere in the test file or visible in the diff. This would cause a ReferenceError at runtime. Possibly a copy-paste artifact — please verify tests actually pass.

---

Minor / Style

• The SqlitePartData.tool field is typed as string but appears to hold structured data (tool call name?). Consider widening the type or adding a comment about the expected format.
• firstMessage is initialized to row.title || '' but then overwritten by the first text part. If the session has no parts, the title is used as the preview — this is reasonable but should be documented.
• The summary_additions, summary_deletions, summary_files columns are fetched from SQLite but not mapped into AgentSessionInfo. If they're not needed, drop them from the SELECT to reduce data transfer.

---

Summary

Priority	Issue
High	DB handle leak in listSessionsSqlite (early return before try)
High	WAL pragma on read-only connection
Medium	Both paths always evaluated — short-circuit JSON if SQLite succeeds
Medium	deleteMessagePair loads full session to check existence
Low	getSessionPath returns shared DB path regardless of sessionId
Low	createMockStatement may be undefined in dedup test
Low	Missing fallback and directory-match test cases

The core approach is solid and the SQLite schema mapping looks correct against the OpenCode Drizzle schema. Address the DB leak and WAL pragma before merging; the rest are improvements rather than blockers.

[coderabbitai]

Warning

Rate limit exceeded

@pedramamini has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 22 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/opencode-sqlite-sessions

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[pedramamini]

@CodeRabbit review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.