feat: add SQLite session reading support for OpenCode v1.2+#569
Open
chr1syy wants to merge 5 commits intoRunMaestro:mainfrom
Open
feat: add SQLite session reading support for OpenCode v1.2+#569chr1syy wants to merge 5 commits intoRunMaestro:mainfrom
chr1syy wants to merge 5 commits intoRunMaestro:mainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SQLite-backed session storage (v1.2+) with JSON fallback; reads SQLite first, merges/deduplicates JSON results, preserves JSON-only remote/SSH behavior, blocks deletion for DB-backed sessions, and introduces DB helpers, row types, and migration-safe listing/loading logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Consumer
participant Orchestrator as listSessions
participant SQLite as SQLite DB
participant JSON as JSON Storage
participant Merger as Merge/Dedup
Client->>Orchestrator: Request session list(project)
Orchestrator->>SQLite: openOpenCodeDb() & query sessions
SQLite-->>Orchestrator: SQLiteSessionRows
Orchestrator->>JSON: listSessionsJson(project) (fallback)
JSON-->>Orchestrator: JSON sessions
Orchestrator->>Merger: Merge results
Merger->>Merger: Deduplicate by sessionId, prefer SQLite
Merger-->>Orchestrator: Combined session list
Orchestrator-->>Client: Return merged, sorted sessions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryThis PR adds SQLite session reading support for OpenCode v1.2+, which switched from JSON file-based storage to a SQLite database ( Key changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[listSessions / readSessionMessages] --> B{sshConfig?}
B -- yes --> C[listSessionsRemote / loadSessionMessagesRemote\nJSON over SSH]
B -- no --> D[openOpenCodeDb\ncheck opencode.db exists]
D -- file absent --> E[listSessionsJson / loadSessionMessages\nJSON fallback]
D -- file present --> F[db.pragma journal_mode=WAL\n⚠️ throws on read-only connection]
F -- throws --> G[catch: log warn, return null\n⚠️ DB handle leaked]
G --> E
F -- succeeds --> H[tableExists: session + project]
H -- missing --> E
H -- present --> I[Query project table\npath matching + global project]
I --> J[Query session rows\nIN matchingIds + LIKE directory\n⚠️ unescaped % and _]
J --> K[convertSqliteSessionRows\nload messages + parts per session\n⚠️ preview overwritten for each user msg]
K --> L{jsonSessions also present?}
L -- yes --> M[Merge: SQLite authoritative\nadd JSON-only sessions\nsort by modifiedAt]
L -- no --> N[Return SQLite sessions only]
M --> O[Return merged sessions]
E --> P[Return JSON sessions]
Last reviewed commit: 1ac2855 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+227
to
+228
| const db = new Database(dbPath, { readonly: true, fileMustExist: true }); | ||
| db.pragma('journal_mode = WAL'); |
There was a problem hiding this comment.
WAL pragma throws on read-only connection, silently breaks all SQLite reading
PRAGMA journal_mode = WAL is a write operation. When better-sqlite3 opens a database with { readonly: true }, any write pragma throws SqliteError: attempt to write a readonly database. Since the error is caught by the catch block in openOpenCodeDb, the function will always return null for every user who has a SQLite database, causing a silent full regression to the JSON path (or returning no sessions).
There are also two secondary problems caused by this:
- Resource leak: If the pragma throws, the
dbhandle opened on line 227 is never closed because there is nofinallyblock inopenOpenCodeDb. Every attempted call leaks a database file handle. - Unnecessary pragma: A reader does not need to set
journal_mode. SQLite automatically detects WAL-mode databases from the file header. Simply removing this line is the correct fix.
Suggested change
| const db = new Database(dbPath, { readonly: true, fileMustExist: true }); | |
| db.pragma('journal_mode = WAL'); | |
| const db = new Database(dbPath, { readonly: true, fileMustExist: true }); | |
| return db; |
Comment on lines
+896
to
+908
| // Fall back to first user message if no assistant text found | ||
| if (!foundPreview && data.role === 'user' && hasPartTable) { | ||
| const parts = db | ||
| .prepare('SELECT data FROM part WHERE message_id = ? ORDER BY time_created ASC') | ||
| .all(msg.id) as Array<{ data: string }>; | ||
| for (const part of parts) { | ||
| const partData = safeJsonParse<SqlitePartData>(part.data); | ||
| if (partData?.type === 'text' && partData.text?.trim()) { | ||
| firstMessage = partData.text; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
User preview gets overwritten for every user message before the first assistant response
The user-message fallback block (lines 896-908) never sets foundPreview = true. As a result, the guard !foundPreview && data.role === 'user' remains true for every user message encountered while scanning, so firstMessage is silently overwritten on each pass. If a session begins with two consecutive user messages (e.g. multi-turn context followed by an actual prompt), the preview shown in the session list will be the last user message before the first assistant response, not the first.
By contrast, the analogous JSON code in listSessionsJson uses a separate firstUserMessage variable that is set once and never overwritten. The SQLite path should mirror that behaviour.
Suggested change
| // Fall back to first user message if no assistant text found | |
| if (!foundPreview && data.role === 'user' && hasPartTable) { | |
| const parts = db | |
| .prepare('SELECT data FROM part WHERE message_id = ? ORDER BY time_created ASC') | |
| .all(msg.id) as Array<{ data: string }>; | |
| for (const part of parts) { | |
| const partData = safeJsonParse<SqlitePartData>(part.data); | |
| if (partData?.type === 'text' && partData.text?.trim()) { | |
| firstMessage = partData.text; | |
| break; | |
| } | |
| } | |
| } | |
| // Fall back to first user message if no assistant text found | |
| if (!foundPreview && data.role === 'user' && hasPartTable) { | |
| const parts = db | |
| .prepare('SELECT data FROM part WHERE message_id = ? ORDER BY time_created ASC') | |
| .all(msg.id) as Array<{ data: string }>; | |
| for (const part of parts) { | |
| const partData = safeJsonParse<SqlitePartData>(part.data); | |
| if (partData?.type === 'text' && partData.text?.trim()) { | |
| firstMessage = partData.text; | |
| foundPreview = true; // lock in the first user message | |
| break; | |
| } | |
| } | |
| } |
Comment on lines
+794
to
+796
| "SELECT id, project_id, directory, title, version, time_created, time_updated, summary_additions, summary_deletions, summary_files FROM session WHERE project_id = 'global' AND (directory = ? OR directory LIKE ?) ORDER BY time_updated DESC" | ||
| ) | ||
| .all(normalizedPath, normalizedPath + '/%') as SqliteSessionRow[]; |
There was a problem hiding this comment.
Unescaped LIKE wildcards in directory path query
normalizedPath + '/%' is passed directly as a LIKE pattern. If the project directory path contains the SQLite LIKE wildcard characters % or _ (uncommon but valid in Unix paths), the query will match unintended rows. For example, a path like /home/user/my_project would match /home/user/myXproject/ as well.
The fix is to escape the literal path before appending /%:
const escapedPath = normalizedPath.replace(/[%_]/g, '\\$&');
// then use:
.all(normalizedPath, escapedPath + '/%', '\\')
// and the LIKE clause becomes: directory LIKE ? ESCAPE ?Or, if the SQLite schema stores paths in a normalised form that avoids these characters in practice, add a comment explaining why the unescaped form is safe here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/storage/opencode-session-storage.ts (2)
882-908: N+1 query pattern when extracting message previews.For each session, this loops through messages and queries parts individually to find preview text. Consider loading all parts in a single query per session or using a JOIN/subquery to reduce round-trips. This is acceptable for local SQLite but may become noticeable with many sessions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/opencode-session-storage.ts` around lines 882 - 908, The current preview extraction does N+1 queries by calling the part SELECT inside the message loop (see variables foundPreview, firstMessage, hasPartTable and safeJsonParse usage); change to fetch all parts for the session in one query before iterating messages (e.g. SELECT data, message_id FROM part WHERE message_id IN (...session message ids...) ORDER BY time_created ASC or a JOIN/subquery) and then group/lookup parts by message_id in memory so the inner loops over messages just scan the preloaded parts instead of running db.prepare(...).all(...) per message.
227-228: WAL pragma on read-only connection has no effect.Setting
journal_mode = WALon a read-only connection is a no-op since the database cannot be modified. While harmless, it's misleading. Consider removing it or adding a comment explaining intent (e.g., if you expect to switch to read-write later).🔧 Suggested fix
const db = new Database(dbPath, { readonly: true, fileMustExist: true }); - db.pragma('journal_mode = WAL'); return db;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/opencode-session-storage.ts` around lines 227 - 228, The call to db.pragma('journal_mode = WAL') is ineffective on a read-only connection and is misleading; remove that line (the db.pragma('journal_mode = WAL') call) from the code where the Database is constructed with readonly: true (the Database db = new Database(..., { readonly: true, ... }) block), or alternatively guard it so you only set journal_mode when opening the Database in read-write mode and add a brief comment explaining the intent if you expect to switch to write mode later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/storage/opencode-session-storage.ts`:
- Around line 1420-1425: The code currently returns OPENCODE_DB_PATH merely if
the DB file exists, which is incorrect for sessions stored only as JSON;
instead, update the logic in the function that currently checks
fsSync.existsSync(OPENCODE_DB_PATH) to first verify the given sessionId actually
exists in the SQLite store before returning OPENCODE_DB_PATH: open the SQLite DB
at OPENCODE_DB_PATH (or use the existing DB client), run a single existence
query against the sessions/messages table for the given sessionId (e.g., SELECT
1 FROM sessions WHERE id = ? or equivalent), and only return OPENCODE_DB_PATH if
the query indicates the session is present; otherwise fall back to
this.getMessageDir(sessionId).
- Line 962: The early-return of null when messageRows.length === 0 should be
replaced with an explicit empty-result object so callers can distinguish
"session exists but has no messages" from "session not found"; update the
function that currently checks messageRows (the method containing the
messageRows.length === 0 check) to return the same shape/type as a normal
successful response but with an empty messages array (and any other expected
fields populated/empty), and adjust any downstream handling (the caller that
currently falls back to JSON loading) to treat null as "not found" only and
treat the empty-result object as a valid, empty session.
- Around line 792-796: The LIKE pattern using normalizedPath can misbehave if
normalizedPath contains SQL wildcards (%) or (_) — update the code that builds
the parameters for the db.prepare call (the assignment to globalSessions) to
escape SQL wildcard characters in normalizedPath before using it in the LIKE
pattern (e.g., create an escapedPath by replacing % and _ (and the escape char
itself) with escaped versions, then pass escapedPath and escapedPath + '/%' as
parameters and include an ESCAPE clause in the SQL), so the db.prepare("... LIKE
? ...", ...) call uses the escaped values and the SQL includes ESCAPE '\' to
ensure only the appended '/%' acts as the wildcard.
---
Nitpick comments:
In `@src/main/storage/opencode-session-storage.ts`:
- Around line 882-908: The current preview extraction does N+1 queries by
calling the part SELECT inside the message loop (see variables foundPreview,
firstMessage, hasPartTable and safeJsonParse usage); change to fetch all parts
for the session in one query before iterating messages (e.g. SELECT data,
message_id FROM part WHERE message_id IN (...session message ids...) ORDER BY
time_created ASC or a JOIN/subquery) and then group/lookup parts by message_id
in memory so the inner loops over messages just scan the preloaded parts instead
of running db.prepare(...).all(...) per message.
- Around line 227-228: The call to db.pragma('journal_mode = WAL') is
ineffective on a read-only connection and is misleading; remove that line (the
db.pragma('journal_mode = WAL') call) from the code where the Database is
constructed with readonly: true (the Database db = new Database(..., { readonly:
true, ... }) block), or alternatively guard it so you only set journal_mode when
opening the Database in read-write mode and add a brief comment explaining the
intent if you expect to switch to write mode later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cca3101-f991-44d3-8d25-8d481c771320
📒 Files selected for processing (1)
src/main/storage/opencode-session-storage.ts
- Remove WAL pragma on readonly connection (was silently breaking all SQLite reads) - Set foundPreview=true for user message fallback to prevent overwrite - Escape LIKE wildcards in directory path queries - Return empty result instead of null for sessions with no messages - Check session existence in SQLite before returning DB path in getSessionPath - Fix prettier formatting in docs/releases.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/storage/opencode-session-storage.ts (1)
957-974: Consider: Ambiguous result for non-existent sessions.When
messageRows.length === 0, the function returns an empty result object. However, this doesn't distinguish between:
- Session exists in SQLite but has no messages yet
- Session doesn't exist in SQLite at all
Both cases return the same empty result, which could cause the caller to incorrectly assume the session exists.
For
readSessionMessagesandgetSearchableMessages, this is likely fine since they'll just return empty results. However, fordeleteMessagePairat line 1467, this matters because an empty result triggers JSON fallback.If this distinction is important, consider adding a session existence check:
💡 Optional fix to verify session existence
const messageRows = db .prepare( 'SELECT id, session_id, time_created, time_updated, data FROM message WHERE session_id = ? ORDER BY time_created ASC' ) .all(sessionId) as SqliteMessageRow[]; - // Session exists in SQLite but has no messages yet — return empty result + // Check if session actually exists in SQLite (might have no messages yet) if (messageRows.length === 0) { + const sessionExists = tableExists(db, 'session') + ? db.prepare('SELECT 1 FROM session WHERE id = ? LIMIT 1').get(sessionId) + : null; + if (!sessionExists) { + return null; // Session not in SQLite, caller should try JSON + } + // Session exists but has no messages yet return { messages: [], parts: new Map(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/opencode-session-storage.ts` around lines 957 - 974, The current early return when messageRows.length === 0 conflates "session exists but has no messages" with "session does not exist", which affects callers like deleteMessagePair and JSON fallback; update readSessionMessages (and similarly getSearchableMessages) to first check session existence (e.g., query the session table for the given sessionId using the same DB connection) and return a distinct result or error when the session is missing (for example null or a flag) instead of the empty-message payload; adjust deleteMessagePair to handle the new absent-session signal so it doesn't wrongly trigger JSON fallback.docs/releases.md (1)
201-201: Minor: Capitalize "GitHub" correctly."Github" should be "GitHub" (capital H) per official branding.
-🌳 Github Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`. +🌳 GitHub Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 201, Update the release note sentence that currently reads "Github Worktree support was added." to use the correct branding "GitHub Worktree support was added." Locate the exact sentence containing "Github Worktree support was added." in the docs/releases.md content and change "Github" to "GitHub" while preserving the rest of the text and punctuation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/storage/opencode-session-storage.ts`:
- Around line 1463-1476: The code only blocks deletion when
loadSessionMessagesSqlite(sessionId) returns messages.length > 0, letting
empty-but-existing SQLite sessions fall through to JSON deletion; change the
guard to treat any non-null/non-undefined sqliteResult as an existing OpenCode
SQLite session. Update the condition in the block that calls
loadSessionMessagesSqlite(sessionId) (referencing sqliteResult and sessionId) to
check for sqliteResult truthiness (e.g., if (sqliteResult) or explicit !== null)
and keep the same logger.warn and return error path so no SQLite-backed
session—empty or not—is allowed to proceed to JSON deletion.
---
Nitpick comments:
In `@docs/releases.md`:
- Line 201: Update the release note sentence that currently reads "Github
Worktree support was added." to use the correct branding "GitHub Worktree
support was added." Locate the exact sentence containing "Github Worktree
support was added." in the docs/releases.md content and change "Github" to
"GitHub" while preserving the rest of the text and punctuation.
In `@src/main/storage/opencode-session-storage.ts`:
- Around line 957-974: The current early return when messageRows.length === 0
conflates "session exists but has no messages" with "session does not exist",
which affects callers like deleteMessagePair and JSON fallback; update
readSessionMessages (and similarly getSearchableMessages) to first check session
existence (e.g., query the session table for the given sessionId using the same
DB connection) and return a distinct result or error when the session is missing
(for example null or a flag) instead of the empty-message payload; adjust
deleteMessagePair to handle the new absent-session signal so it doesn't wrongly
trigger JSON fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4523c3b8-eb3c-4272-99e3-9f4e61534ba4
📒 Files selected for processing (2)
docs/releases.mdsrc/main/storage/opencode-session-storage.ts
…y ones Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/storage/opencode-session-storage.ts (1)
848-911: Consider batching queries to avoid N+1 pattern.The current implementation queries messages for each session row, then queries parts for individual messages when searching for preview text. For a user with many sessions, this results in O(sessions × messages) database queries.
If performance becomes an issue, consider using JOINs or batch queries to load all needed data in fewer round trips.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/opencode-session-storage.ts` around lines 848 - 911, The code performs N+1 queries inside the messages loop by calling the part table per message to build a preview; refactor the block around hasMessageTable/messages and the part SELECTs so parts are fetched in bulk (e.g., a single query that JOINs part to message or a single IN(...) query for all message ids) and then map parts back to their message ids before iterating; keep the existing logic using safeJsonParse/SqlitePartData and the foundPreview/firstMessage selection but remove the per-message db.prepare('SELECT data FROM part...').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/storage/opencode-session-storage.ts`:
- Around line 963-974: The empty-message branch currently returns an empty
session result whenever messageRows.length === 0, but must first verify the
session exists in SQLite to allow JSON fallback; update the logic in the
function that reads messages (the code path handling messageRows and used by
readSessionMessages()) to run a lightweight existence check (e.g., SELECT 1 FROM
sessions WHERE id = ? or reuse any existing getSession/getSessionById helper)
before returning, and if the session row is not found return null (so
loadSessionMessages() can be attempted), otherwise return the empty result
object exactly as now; reference messageRows, readSessionMessages(),
loadSessionMessages() when making the change.
---
Nitpick comments:
In `@src/main/storage/opencode-session-storage.ts`:
- Around line 848-911: The code performs N+1 queries inside the messages loop by
calling the part table per message to build a preview; refactor the block around
hasMessageTable/messages and the part SELECTs so parts are fetched in bulk
(e.g., a single query that JOINs part to message or a single IN(...) query for
all message ids) and then map parts back to their message ids before iterating;
keep the existing logic using safeJsonParse/SqlitePartData and the
foundPreview/firstMessage selection but remove the per-message
db.prepare('SELECT data FROM part...').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7681756-ca2c-441d-93dc-1cfcd8c2259d
📒 Files selected for processing (1)
src/main/storage/opencode-session-storage.ts
When no messages are found for a sessionId, check the session table to distinguish "session exists but empty" from "session not in SQLite" before deciding whether to block JSON fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/storage/opencode-session-storage.ts`:
- Around line 768-796: The descendant checks and SQL LIKE prefix currently
hardcode '/' causing Windows misses; update all uses of '/' to use path.sep (or
construct a platform-aware sep variable) when trimming trailing separators
(replace(/\/+$/) -> replace(new RegExp(`${path.sep.replace('\\','\\\\')}+$`)),
when comparing prefixes (storedPath + '/' and normalizedPath + '/' -> storedPath
+ path.sep and normalizedPath + path.sep) and when building the escapedPath LIKE
pattern (escapedPath + '/%' -> escapedPath + path.sep + '%'). Apply these
changes around the storedPath/normalizedPath comparisons and the global-sessions
query where hasGlobalProject, escapedPath, and the SQL .all(...) call are used
so subdirectory matches work on Windows.
- Around line 223-231: The current try/catch around new Database(dbPath, ...)
swallows all SQLite errors and returns null; change it to only return null for
expected "file not found"/missing DB cases (e.g., when
!fsSync.existsSync(dbPath) or when fileMustExist triggers a known ENOENT), but
for any other unexpected errors (corruption, permission, query errors) capture
and rethrow: import and call captureException(error, { extra: { dbPath } }) and
logger.warn(...) then rethrow the error so it bubbles to Sentry/upper layers
instead of falling back to JSON; apply the same change pattern to the other
analogous blocks referenced (the Database creation/use sites you noted around
the other ranges) and preserve LOG_CONTEXT in logs.
- Around line 797-817: After merging globalSessions into the sessions array,
ensure the combined array is re-sorted so newest sessions come first;
specifically, sort the combined sessions (the local variable sessions that you
push global entries into) by the session timestamp field (e.g., created_at or
createdAt) in descending order before calling convertSqliteSessionRows. Locate
the merge logic that references globalSessions and sessions in
listSessions()/opencode-session-storage.ts and add a sort on sessions
(newest-first) right before the return/convertSqliteSessionRows(...) call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40c901c1-f9f1-4891-af0b-49a51446b5c9
📒 Files selected for processing (1)
src/main/storage/opencode-session-storage.ts
Comment on lines
+865
to
+909
| // Aggregate stats and find preview message | ||
| let foundPreview = false; | ||
| for (const msg of messages) { | ||
| const data = safeJsonParse<SqliteMessageData>(msg.data); | ||
| if (!data) continue; | ||
|
|
||
| if (data.tokens) { | ||
| totalInputTokens += data.tokens.input || 0; | ||
| totalOutputTokens += data.tokens.output || 0; | ||
| totalCacheReadTokens += data.tokens.cache?.read || 0; | ||
| totalCacheWriteTokens += data.tokens.cache?.write || 0; | ||
| } | ||
| if (data.cost) { | ||
| totalCost += data.cost; | ||
| } | ||
|
|
||
| // Get preview from first assistant message with text | ||
| if (!foundPreview && data.role === 'assistant' && hasPartTable) { | ||
| const parts = db | ||
| .prepare('SELECT data FROM part WHERE message_id = ? ORDER BY time_created ASC') | ||
| .all(msg.id) as Array<{ data: string }>; | ||
| for (const part of parts) { | ||
| const partData = safeJsonParse<SqlitePartData>(part.data); | ||
| if (partData?.type === 'text' && partData.text?.trim()) { | ||
| firstMessage = partData.text; | ||
| foundPreview = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fall back to first user message if no assistant text found | ||
| if (!foundPreview && data.role === 'user' && hasPartTable) { | ||
| const parts = db | ||
| .prepare('SELECT data FROM part WHERE message_id = ? ORDER BY time_created ASC') | ||
| .all(msg.id) as Array<{ data: string }>; | ||
| for (const part of parts) { | ||
| const partData = safeJsonParse<SqlitePartData>(part.data); | ||
| if (partData?.type === 'text' && partData.text?.trim()) { | ||
| firstMessage = partData.text; | ||
| foundPreview = true; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Assistant previews never override the first user preview.
The user fallback sets foundPreview = true, so later assistant text is skipped entirely. That makes SQLite-backed previews diverge from the JSON path below, which prefers the first assistant response and only falls back to user text if no assistant text exists.
- Use platform-aware path separators (path.sep) instead of hardcoded '/' for trailing separator stripping and subdirectory prefix comparisons, fixing Windows path matching in listSessions and findProjectId - Extract TRAILING_SEP_RE constant to avoid repeated regex construction - Improve openOpenCodeDb error handling: report unexpected SQLite errors (corruption, permissions) to Sentry and rethrow instead of silently falling back to JSON; file-not-found still returns null - Re-sort sessions after merging global project sessions so combined results maintain newest-first ordering by time_updated Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/storage/opencode-session-storage.ts (1)
890-918:⚠️ Potential issue | 🟡 MinorLogic bug: User preview blocks assistant preview.
The preview selection processes messages in chronological order (ASC). When a user message with text is encountered first,
foundPreviewis set totrue, preventing subsequent assistant messages from being considered. This contradicts the intended priority (assistant response > user message) documented in the JSON path at line 1168-1169.🐛 Proposed fix to check all messages before setting foundPreview
// Aggregate stats and find preview message - let foundPreview = false; + let assistantPreview = ''; + let userPreview = ''; for (const msg of messages) { const data = safeJsonParse<SqliteMessageData>(msg.data); if (!data) continue; if (data.tokens) { totalInputTokens += data.tokens.input || 0; totalOutputTokens += data.tokens.output || 0; totalCacheReadTokens += data.tokens.cache?.read || 0; totalCacheWriteTokens += data.tokens.cache?.write || 0; } if (data.cost) { totalCost += data.cost; } // Get preview from first assistant message with text - if (!foundPreview && data.role === 'assistant' && hasPartTable) { + if (!assistantPreview && data.role === 'assistant' && hasPartTable) { const parts = db .prepare('SELECT data FROM part WHERE message_id = ? ORDER BY time_created ASC') .all(msg.id) as Array<{ data: string }>; for (const part of parts) { const partData = safeJsonParse<SqlitePartData>(part.data); if (partData?.type === 'text' && partData.text?.trim()) { - firstMessage = partData.text; - foundPreview = true; + assistantPreview = partData.text; break; } } + if (assistantPreview) break; // Found assistant preview, stop scanning } // Fall back to first user message if no assistant text found - if (!foundPreview && data.role === 'user' && hasPartTable) { + if (!userPreview && data.role === 'user' && hasPartTable) { const parts = db .prepare('SELECT data FROM part WHERE message_id = ? ORDER BY time_created ASC') .all(msg.id) as Array<{ data: string }>; for (const part of parts) { const partData = safeJsonParse<SqlitePartData>(part.data); if (partData?.type === 'text' && partData.text?.trim()) { - firstMessage = partData.text; - foundPreview = true; + userPreview = partData.text; break; } } } } + // Priority: assistant response > user message > title + firstMessage = assistantPreview || userPreview || row.title || '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/opencode-session-storage.ts` around lines 890 - 918, The current logic sets foundPreview as soon as any user text is found, which blocks later assistant text from being chosen; change to prefer assistant responses by collecting a candidate user preview (e.g., candidateUserPreview) instead of setting foundPreview when data.role === 'user', keep the existing assistant branch to set firstMessage and foundPreview immediately when data.role === 'assistant', and after processing all messages (or after the surrounding loop over messages) only set firstMessage = candidateUserPreview if no assistant preview (foundPreview) was found; reference symbols: foundPreview, data.role, firstMessage, candidateUserPreview (new), safeJsonParse, hasPartTable, msg.id.
🧹 Nitpick comments (3)
src/main/storage/opencode-session-storage.ts (3)
536-549: Inconsistent path separator usage for Windows compatibility.This method uses hardcoded
/for path normalization (lines 539-540) and subdirectory matching (line 546), while the SQLite methods use platform-awarepath.sepandTRAILING_SEP_RE. This inconsistency could cause session matching to fail on Windows with JSON storage.♻️ Proposed fix for platform consistency
private sessionMatchesPath(sessionDirectory: string | undefined, projectPath: string): boolean { if (!sessionDirectory) return false; - const normalizedSessionDir = path.resolve(sessionDirectory).replace(/\/+$/, ''); - const normalizedProjectPath = path.resolve(projectPath).replace(/\/+$/, ''); + const normalizedSessionDir = path.resolve(sessionDirectory).replace(TRAILING_SEP_RE, ''); + const normalizedProjectPath = path.resolve(projectPath).replace(TRAILING_SEP_RE, ''); // Exact match if (normalizedSessionDir === normalizedProjectPath) return true; // Session is in a subdirectory of the project - if (normalizedSessionDir.startsWith(normalizedProjectPath + '/')) return true; + if (normalizedSessionDir.startsWith(normalizedProjectPath + path.sep)) return true; return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/opencode-session-storage.ts` around lines 536 - 549, The sessionMatchesPath function uses hardcoded '/' when trimming trailing separators and when checking subdirectory startsWith; update it to use platform-aware separators by replacing the regex /\/+$/ with a RegExp built from path.sep (or reuse TRAILING_SEP_RE if available) to trim trailing separators from normalizedSessionDir and normalizedProjectPath, and change the subdirectory check to use normalizedProjectPath + path.sep instead of '/' so that sessionMatchesPath, normalizedSessionDir and normalizedProjectPath behave consistently with the SQLite methods on Windows.
1059-1064: Same issue: Catch block masks unexpected errors.Similar to
listSessionsSqlite, this catch-all returnsnullfor any error, preventing Sentry from capturing unexpected failures. Consider distinguishing expected errors (schema mismatch) from unexpected ones (corruption, memory).♻️ Proposed fix
} catch (error) { - logger.warn(`Error loading messages from OpenCode SQLite: ${error}`, LOG_CONTEXT); - return null; + const errStr = String(error); + if (errStr.includes('no such table') || errStr.includes('no such column')) { + logger.info(`OpenCode SQLite schema mismatch for messages, falling back to JSON: ${error}`, LOG_CONTEXT); + return null; + } + logger.warn(`Unexpected error loading messages from OpenCode SQLite: ${error}`, LOG_CONTEXT); + captureException(error instanceof Error ? error : new Error(String(error)), { + extra: { sessionId }, + }); + throw error; } finally { db.close(); }Based on learnings: "For unexpected errors, rethrow to let Sentry capture them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/opencode-session-storage.ts` around lines 1059 - 1064, The catch-all in the message-loading function in src/main/storage/opencode-session-storage.ts currently logs the error via logger.warn and returns null (same pattern as listSessionsSqlite), which masks unexpected failures; update the catch to detect expected schema/migration errors (e.g., check error.message or Sqlite-specific error codes) and only swallow those by logging and returning null, but rethrow any other/unexpected errors so Sentry can capture them; keep the finally block to always call db.close().
827-832: Consider distinguishing expected vs unexpected errors in catch block.The current catch-all returns
nullfor every error, including unexpected ones (e.g., malformed queries, memory issues). This prevents Sentry from capturing actionable errors.Per coding guidelines, expected errors (missing tables, empty results) should allow fallback, but unexpected errors should be captured and potentially rethrown.
♻️ Proposed fix to capture unexpected errors
} catch (error) { - logger.warn(`Error reading OpenCode SQLite database: ${error}`, LOG_CONTEXT); - return null; + // Schema mismatch is expected during migrations — allow JSON fallback + const errStr = String(error); + if (errStr.includes('no such table') || errStr.includes('no such column')) { + logger.info(`OpenCode SQLite schema mismatch, falling back to JSON: ${error}`, LOG_CONTEXT); + return null; + } + // Unexpected error — report to Sentry and rethrow + logger.warn(`Unexpected error reading OpenCode SQLite database: ${error}`, LOG_CONTEXT); + captureException(error instanceof Error ? error : new Error(String(error)), { + extra: { projectPath }, + }); + throw error; } finally { db.close(); }Based on learnings: "Do not silently swallow errors with try-catch blocks that only log. Let exceptions bubble up to Sentry for error tracking in production."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/opencode-session-storage.ts` around lines 827 - 832, The catch-all in the read routine that calls logger.warn(..., LOG_CONTEXT) and returns null should distinguish expected SQLite "not found"/empty-result errors from unexpected failures: update the catch to inspect the thrown error (e.g., error.code or error.message for patterns like "SQLITE_ERROR", "no such table", or known benign cases) and only return null for those expected cases; for any other/unrecognized errors, log with logger.error (including the error object) and rethrow (or forward to Sentry) so they surface to monitoring. Keep the existing finally block that calls db.close() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/storage/opencode-session-storage.ts`:
- Around line 890-918: The current logic sets foundPreview as soon as any user
text is found, which blocks later assistant text from being chosen; change to
prefer assistant responses by collecting a candidate user preview (e.g.,
candidateUserPreview) instead of setting foundPreview when data.role === 'user',
keep the existing assistant branch to set firstMessage and foundPreview
immediately when data.role === 'assistant', and after processing all messages
(or after the surrounding loop over messages) only set firstMessage =
candidateUserPreview if no assistant preview (foundPreview) was found; reference
symbols: foundPreview, data.role, firstMessage, candidateUserPreview (new),
safeJsonParse, hasPartTable, msg.id.
---
Nitpick comments:
In `@src/main/storage/opencode-session-storage.ts`:
- Around line 536-549: The sessionMatchesPath function uses hardcoded '/' when
trimming trailing separators and when checking subdirectory startsWith; update
it to use platform-aware separators by replacing the regex /\/+$/ with a RegExp
built from path.sep (or reuse TRAILING_SEP_RE if available) to trim trailing
separators from normalizedSessionDir and normalizedProjectPath, and change the
subdirectory check to use normalizedProjectPath + path.sep instead of '/' so
that sessionMatchesPath, normalizedSessionDir and normalizedProjectPath behave
consistently with the SQLite methods on Windows.
- Around line 1059-1064: The catch-all in the message-loading function in
src/main/storage/opencode-session-storage.ts currently logs the error via
logger.warn and returns null (same pattern as listSessionsSqlite), which masks
unexpected failures; update the catch to detect expected schema/migration errors
(e.g., check error.message or Sqlite-specific error codes) and only swallow
those by logging and returning null, but rethrow any other/unexpected errors so
Sentry can capture them; keep the finally block to always call db.close().
- Around line 827-832: The catch-all in the read routine that calls
logger.warn(..., LOG_CONTEXT) and returns null should distinguish expected
SQLite "not found"/empty-result errors from unexpected failures: update the
catch to inspect the thrown error (e.g., error.code or error.message for
patterns like "SQLITE_ERROR", "no such table", or known benign cases) and only
return null for those expected cases; for any other/unrecognized errors, log
with logger.error (including the error object) and rethrow (or forward to
Sentry) so they surface to monitoring. Keep the existing finally block that
calls db.close() unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bffa123-3e7f-4a57-a815-632d7681c812
📒 Files selected for processing (1)
src/main/storage/opencode-session-storage.ts
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
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation