feature(opencode): support opencode sqlite storage#887
Open
unsync wants to merge 9 commits intoryoppippi:mainfrom
Open
feature(opencode): support opencode sqlite storage#887unsync wants to merge 9 commits intoryoppippi:mainfrom
unsync wants to merge 9 commits intoryoppippi:mainfrom
Conversation
Add support for OpenCode >= 1.2.2 SQLite database format while maintaining backward compatibility with legacy JSON files. Adds better-sqlite3 as optional dependency with Bun SQLite fallback.
|
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:
📝 WalkthroughWalkthroughSQLite-first loading for OpenCode (>=1.2.2) was added: code now locates and reads a local opencode.db via runtime adapters (better-sqlite3 or bun:sqlite), parses/deduplicates/validates messages and sessions, and falls back to legacy JSON on DB errors; build configs were updated to allow/externalize better-sqlite3. Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant Loader as loadOpenCodeMessages/Sessions
participant DbPath as getDbPath
participant Sqlite as SqliteAdapter
participant JSON as JSON Loader
Caller->>Loader: Request OpenCode data
Loader->>DbPath: Locate opencode.db
DbPath-->>Loader: DB path or null
alt DB path found
Loader->>Sqlite: openSqliteDb & run queries
Sqlite-->>Loader: rows
alt parse & validate success
Loader->>Loader: dedupe, map to LoadedUsage/Metadata
Loader-->>Caller: return processed data
else parse/error
Loader->>Loader: logSqliteFallback
Loader->>JSON: load from JSON files
JSON-->>Caller: return JSON-based data
end
else no DB
Loader->>JSON: load from JSON files
JSON-->>Caller: return JSON-based data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/opencode/src/commands/session.ts (1)
14-14: Consider migratingdaily.tsto the unified API for consistency.Per the context snippet,
apps/opencode/src/commands/daily.tsstill importsloadOpenCodeMessages()separately. While this works (the old API is preserved), migrating it toloadOpenCodeData()would provide consistent behavior and avoid redundant DB reads if both commands are invoked in sequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/commands/session.ts` at line 14, The daily command still uses the old function loadOpenCodeMessages(), causing inconsistent behavior and potential redundant DB reads; update apps/opencode/src/commands/daily.ts to import and call the unified loader loadOpenCodeData() instead (replace any import of loadOpenCodeMessages with loadOpenCodeData and adapt usage to the returned shape), and ensure any downstream code in daily.ts that expects the old structure is adjusted to the new data shape returned by loadOpenCodeData() so both session.ts and daily.ts use the same unified API.apps/opencode/src/data-loader.ts (1)
496-512: Standalone loaders wastefully load the entire database.Both
loadOpenCodeSessions()andloadOpenCodeMessages()callloadOpenCodeDataFromSqlite()which loads both messages and sessions, then discards half the result. If both functions are called separately, the DB is read and parsed twice.Consider either:
- Deprecating these in favor of
loadOpenCodeData()(already the preferred API)- Creating targeted
loadMessagesFromSqlite/loadSessionsFromSqlitewrappers that don't load unnecessary dataThis is a minor efficiency concern since
loadOpenCodeData()is the recommended API.Also applies to: 514-530
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 496 - 512, The current loaders loadOpenCodeSessions and loadOpenCodeMessages both call loadOpenCodeDataFromSqlite which parses the entire DB twice; change them to avoid redundant work by either (A) delegating to the unified loadOpenCodeData() API and returning the appropriate map/message list from that single result, or (B) add targeted sqlite helpers (e.g., loadSessionsFromSqlite and loadMessagesFromSqlite) that query only sessions or messages respectively and call those from loadOpenCodeSessions/loadOpenCodeMessages; update error handling to still call logSqliteFallback('sessions'/'messages', error) as appropriate and keep the JSON fallback (loadOpenCodeSessionsFromJson/loadOpenCodeMessagesFromJson) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/opencode/src/commands/session.ts`:
- Line 14: The daily command still uses the old function loadOpenCodeMessages(),
causing inconsistent behavior and potential redundant DB reads; update
apps/opencode/src/commands/daily.ts to import and call the unified loader
loadOpenCodeData() instead (replace any import of loadOpenCodeMessages with
loadOpenCodeData and adapt usage to the returned shape), and ensure any
downstream code in daily.ts that expects the old structure is adjusted to the
new data shape returned by loadOpenCodeData() so both session.ts and daily.ts
use the same unified API.
In `@apps/opencode/src/data-loader.ts`:
- Around line 496-512: The current loaders loadOpenCodeSessions and
loadOpenCodeMessages both call loadOpenCodeDataFromSqlite which parses the
entire DB twice; change them to avoid redundant work by either (A) delegating to
the unified loadOpenCodeData() API and returning the appropriate map/message
list from that single result, or (B) add targeted sqlite helpers (e.g.,
loadSessionsFromSqlite and loadMessagesFromSqlite) that query only sessions or
messages respectively and call those from
loadOpenCodeSessions/loadOpenCodeMessages; update error handling to still call
logSqliteFallback('sessions'/'messages', error) as appropriate and keep the JSON
fallback (loadOpenCodeSessionsFromJson/loadOpenCodeMessagesFromJson) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e21053a-ae27-4b7e-bfa3-a30793c65fae
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/opencode/package.jsonapps/opencode/src/commands/session.tsapps/opencode/src/data-loader.tsapps/opencode/tsdown.config.tspnpm-workspace.yaml
|
It works on my environment, great job! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/opencode/src/data-loader.ts (3)
198-201: ExportLoadedOpenCodeDatatype for consumer use.This type is the return type of the exported
loadOpenCodeData()function. Exporting it allows consumers to properly type their variables without relying on type inference.♻️ Proposed fix
-type LoadedOpenCodeData = { +export type LoadedOpenCodeData = { entries: LoadedUsageEntry[]; sessionMetadataMap: Map<string, LoadedSessionMetadata>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 198 - 201, The type LoadedOpenCodeData is used as the return type of the exported function loadOpenCodeData but is currently not exported; export the type so downstream consumers can import it. Update the type declaration for LoadedOpenCodeData to be exported (e.g., add the export modifier) and ensure any related types (LoadedUsageEntry, LoadedSessionMetadata) are also exported or available to consumers as needed so consumers can properly type variables using loadOpenCodeData().
485-485: Simplify deduplication key.The template literal is unnecessary here since
message.idis already a string.♻️ Proposed fix
- const dedupeKey = `${message.id}`; + const dedupeKey = message.id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` at line 485, Replace the unnecessary template literal when building the deduplication key: change the assignment to use message.id directly (e.g., set dedupeKey = message.id) in the code that defines dedupeKey so you avoid creating a needless string interpolation; if there's any type uncertainty, use String(message.id) to ensure a string.
532-566: Inefficient: loading all data when only one part is needed.Both
loadOpenCodeSessions()andloadOpenCodeMessages()callloadOpenCodeDataFromSqlite()which loads both messages and sessions from the database, then discards one part. This wastes I/O and memory, especially for large databases.Consider either:
- Creating separate
loadMessagesOnlyFromSqliteandloadSessionsOnlyFromSqlitewrappers, or- Accepting the inefficiency if these functions are rarely called independently (the
daily.tscommand only usesloadOpenCodeMessages, whilesession.tsusesloadOpenCodeData)♻️ Proposed fix: separate SQLite loading paths
+function loadMessagesOnlyFromSqlite(dbPath: string): LoadedUsageEntry[] { + const db = openSqliteDb(dbPath); + try { + return loadMessagesFromSqlite(db); + } finally { + db.close(); + } +} + +function loadSessionsOnlyFromSqlite(dbPath: string): Map<string, LoadedSessionMetadata> { + const db = openSqliteDb(dbPath); + try { + return loadSessionsFromSqlite(db); + } finally { + db.close(); + } +} + export async function loadOpenCodeSessions(): Promise<Map<string, LoadedSessionMetadata>> { const openCodePath = getOpenCodePath(); if (openCodePath == null) { return new Map(); } const dbPath = getDbPath(openCodePath); if (dbPath != null) { try { - return loadOpenCodeDataFromSqlite(dbPath).sessionMetadataMap; + return loadSessionsOnlyFromSqlite(dbPath); } catch (error) { logSqliteFallback('sessions', error); } } return loadOpenCodeSessionsFromJson(openCodePath); } export async function loadOpenCodeMessages(): Promise<LoadedUsageEntry[]> { const openCodePath = getOpenCodePath(); if (openCodePath == null) { return []; } const dbPath = getDbPath(openCodePath); if (dbPath != null) { try { - return loadOpenCodeDataFromSqlite(dbPath).entries; + return loadMessagesOnlyFromSqlite(dbPath); } catch (error) { logSqliteFallback('messages', error); } } return loadOpenCodeMessagesFromJson(openCodePath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 532 - 566, Both loaders call loadOpenCodeDataFromSqlite() which reads both messages and sessions then discards one half, causing unnecessary I/O/memory; change the SQLite path so each loader only reads what it needs: add two targeted helpers (e.g. loadMessagesOnlyFromSqlite(dbPath) and loadSessionsOnlyFromSqlite(dbPath)) or extend loadOpenCodeDataFromSqlite to accept a mode flag ('messages' | 'sessions'), implement the SQL queries to return only the needed rows, then update loadOpenCodeMessages() to call the messages-only helper and loadOpenCodeSessions() to call the sessions-only helper (keeping the same error fallback to logSqliteFallback and JSON fallbacks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/opencode/src/data-loader.ts`:
- Around line 234-255: The openSqliteDb function currently unconditionally
requires 'bun:sqlite' which will throw in Node.js when better-sqlite3 is
unavailable; update openSqliteDb to detect Bun before loading Bun's SQLite
adapter (e.g., check typeof globalThis.Bun !== 'undefined' or
process?.versions?.bun) and only call require('bun:sqlite') when running under
Bun; if not running under Bun, avoid the require and return null or propagate a
controlled error so the caller can fall back to the JSON path; adjust the return
type/handling accordingly and keep references to requireBetterSqlite3,
createBetterSqliteAdapter and createBunSqliteAdapter intact.
---
Nitpick comments:
In `@apps/opencode/src/data-loader.ts`:
- Around line 198-201: The type LoadedOpenCodeData is used as the return type of
the exported function loadOpenCodeData but is currently not exported; export the
type so downstream consumers can import it. Update the type declaration for
LoadedOpenCodeData to be exported (e.g., add the export modifier) and ensure any
related types (LoadedUsageEntry, LoadedSessionMetadata) are also exported or
available to consumers as needed so consumers can properly type variables using
loadOpenCodeData().
- Line 485: Replace the unnecessary template literal when building the
deduplication key: change the assignment to use message.id directly (e.g., set
dedupeKey = message.id) in the code that defines dedupeKey so you avoid creating
a needless string interpolation; if there's any type uncertainty, use
String(message.id) to ensure a string.
- Around line 532-566: Both loaders call loadOpenCodeDataFromSqlite() which
reads both messages and sessions then discards one half, causing unnecessary
I/O/memory; change the SQLite path so each loader only reads what it needs: add
two targeted helpers (e.g. loadMessagesOnlyFromSqlite(dbPath) and
loadSessionsOnlyFromSqlite(dbPath)) or extend loadOpenCodeDataFromSqlite to
accept a mode flag ('messages' | 'sessions'), implement the SQL queries to
return only the needed rows, then update loadOpenCodeMessages() to call the
messages-only helper and loadOpenCodeSessions() to call the sessions-only helper
(keeping the same error fallback to logSqliteFallback and JSON fallbacks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0eb2a4d-87d3-42b9-91ea-376502905421
📒 Files selected for processing (1)
apps/opencode/src/data-loader.ts
Replaces unified loadOpenCodeData() with separate loadOpenCodeMessages() and loadOpenCodeSessions() functions, allowing parallel loading of data sources.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/opencode/src/data-loader.ts (1)
234-255:⚠️ Potential issue | 🟠 MajorGuard the Bun-only fallback before requiring
bun:sqlite.If
better-sqlite3is unavailable under Node.js, Line 251 throws before any SQLite data can load. On installs that only haveopencode.db, the later JSON fallback returns empty results. This same Node/Bun gap was raised earlier and is still present.🛡️ Proposed fix
if (BetterSqlite3 != null) { try { return createBetterSqliteAdapter(new BetterSqlite3(dbPath, { readonly: true })); } catch { // Fall back to Bun's SQLite adapter when better-sqlite3 cannot open in Bun. } } + if (typeof globalThis.Bun === "undefined" && process.versions.bun == null) { + throw new Error("SQLite adapter unavailable: better-sqlite3 is not installed and bun:sqlite is not available"); + } + const { Database } = require("bun:sqlite") as { Database: new (path: string, opts?: { readonly?: boolean }) => BunSqliteDatabase; };Run this to confirm the missing Bun runtime guard. Expected result:
require("bun:sqlite")is present inopenSqliteDb, and there is noglobalThis.Bun/process.versions.buncheck in that function.#!/bin/bash set -euo pipefail sed -n '234,255p' apps/opencode/src/data-loader.ts rg -n "bun:sqlite|globalThis\.Bun|process\.versions\.bun" apps/opencode/src/data-loader.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 234 - 255, The openSqliteDb function currently unconditionally requires 'bun:sqlite', which will throw under Node when Bun is not present; modify openSqliteDb to first detect the Bun runtime (check globalThis.Bun or process.versions?.bun) before calling require('bun:sqlite'), so only require Bun's Database when running in Bun; keep the existing requireBetterSqlite3()/createBetterSqliteAdapter flow and on non-Bun runtimes fall back to the JSON/other fallback instead of attempting require('bun:sqlite'); ensure references to require('bun:sqlite'), Database, createBunSqliteAdapter, and requireBetterSqlite3 are guarded by the Bun runtime check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/opencode/src/data-loader.ts`:
- Around line 521-555: Both loaders independently call
loadOpenCodeDataFromSqlite(dbPath) causing duplicate reads; refactor to a single
shared loader that returns the full snapshot (sessions + entries) and have
loadOpenCodeSessions and loadOpenCodeMessages delegate to it. Add a new function
(e.g. loadOpenCodeData or loadOpenCodeSnapshot) that: resolves
openCodePath/getDbPath, tries loadOpenCodeDataFromSqlite(dbPath) once (logging
via logSqliteFallback on error) and falls back to calling
loadOpenCodeSessionsFromJson/loadOpenCodeMessagesFromJson to build the combined
result; then update loadOpenCodeSessions and loadOpenCodeMessages to call that
shared loader and return only the sessionMetadataMap or entries. Optionally
memoize the snapshot by openCodePath/dbPath to avoid repeated reads within the
same process.
- Around line 294-296: The current filter in data-loader.ts skips rows when
data.tokens is null or tokens.input and tokens.output are zero, which drops rows
that only have cache usage; update the check around data.tokens (the block using
data.tokens.input and data.tokens.output) to treat a row as empty only if all
four fields are absent or zero — tokens.input, tokens.output, tokens.cache.read,
and tokens.cache.write — so rows with cache.read or cache.write > 0 are
preserved; ensure you reference and safely access data.tokens.cache.read and
data.tokens.cache.write when performing the emptiness check.
- Around line 270-286: The dedupe logic currently marks row IDs in dedupeSet
before JSON.parse and v.safeParse, causing malformed or filtered rows to prevent
later valid duplicates from being processed; update the loop so you first
attempt JSON.parse(row.data) and validate with
v.safeParse(sqliteMessageDataSchema), and only after result.success do you check
dedupeSet.has(row.id) and then add row.id to dedupeSet (ensuring you continue
appropriately on parse/validation failure).
---
Duplicate comments:
In `@apps/opencode/src/data-loader.ts`:
- Around line 234-255: The openSqliteDb function currently unconditionally
requires 'bun:sqlite', which will throw under Node when Bun is not present;
modify openSqliteDb to first detect the Bun runtime (check globalThis.Bun or
process.versions?.bun) before calling require('bun:sqlite'), so only require
Bun's Database when running in Bun; keep the existing
requireBetterSqlite3()/createBetterSqliteAdapter flow and on non-Bun runtimes
fall back to the JSON/other fallback instead of attempting
require('bun:sqlite'); ensure references to require('bun:sqlite'), Database,
createBunSqliteAdapter, and requireBetterSqlite3 are guarded by the Bun runtime
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1d40500-d01c-480e-bb1e-38f763eb569b
📒 Files selected for processing (1)
apps/opencode/src/data-loader.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/opencode/src/data-loader.ts (1)
502-505:⚠️ Potential issue | 🟠 MajorSame cache-only filtering issue in JSON loader.
Consistent with the SQLite path, this filter also drops rows that only have cache tokens but zero input/output tokens.
🧮 Proposed fix
// Skip messages with no tokens - if (message.tokens == null || (message.tokens.input === 0 && message.tokens.output === 0)) { + const hasUsage = + message.tokens != null && + ((message.tokens.input ?? 0) > 0 || + (message.tokens.output ?? 0) > 0 || + (message.tokens.cache?.read ?? 0) > 0 || + (message.tokens.cache?.write ?? 0) > 0); + if (!hasUsage) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 502 - 505, The current filter in the JSON loader skips messages when message.tokens.input and message.tokens.output are both zero even if message.tokens.cache exists; update the condition around message.tokens in the loop to only skip when tokens are null or all token counts are zero (input, output and cache). Concretely, modify the if that references message.tokens so it continues only when message.tokens == null || (message.tokens.input === 0 && message.tokens.output === 0 && (message.tokens.cache == null || message.tokens.cache === 0)), leaving the rest of the loader logic unchanged.
♻️ Duplicate comments (2)
apps/opencode/src/data-loader.ts (2)
304-306:⚠️ Potential issue | 🟠 MajorCache-only usage rows are incorrectly filtered out.
This condition drops rows that have
cache.readorcache.writetokens but zeroinput/outputtokens, causing cache-only usage to be undercounted.As per coding guidelines, "Message structure must include
tokens.input,tokens.output,tokens.cache.read, andtokens.cache.writefields".🧮 Proposed fix: include cache tokens in emptiness check
- if (data.tokens == null || (data.tokens.input === 0 && data.tokens.output === 0)) { + const hasUsage = + data.tokens != null && + ((data.tokens.input ?? 0) > 0 || + (data.tokens.output ?? 0) > 0 || + (data.tokens.cache?.read ?? 0) > 0 || + (data.tokens.cache?.write ?? 0) > 0); + if (!hasUsage) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 304 - 306, The current filter wrongly skips rows when tokens.input and tokens.output are zero even though cache usage exists; update the emptiness check around data.tokens to treat a row as empty only if all four fields are zero (tokens.input, tokens.output, tokens.cache.read, tokens.cache.write) or tokens is null/undefined. Locate the branch that uses data.tokens (the if checking data.tokens == null || (data.tokens.input === 0 && data.tokens.output === 0)) and modify it to include tokens.cache.read and tokens.cache.write in the emptiness test so cache-only rows are kept.
280-284:⚠️ Potential issue | 🟠 MajorDedupe check should occur after parsing succeeds.
The deduplication currently marks an ID as seen before JSON parsing and validation. If the first row with a given ID is malformed or fails validation, any later valid duplicate with the same ID will be incorrectly skipped.
🐛 Proposed fix: move dedupe after validation
for (const row of rows) { - if (dedupeSet.has(row.id)) { - continue; - } - dedupeSet.add(row.id); - let parsed: unknown; try { parsed = JSON.parse(row.data); } catch { continue; } const result = v.safeParse(sqliteMessageDataSchema, parsed); if (!result.success) { continue; } const data = result.output; if (data.role !== 'assistant') { continue; } - if (data.tokens == null || (data.tokens.input === 0 && data.tokens.output === 0)) { + const hasUsage = + data.tokens != null && + ((data.tokens.input ?? 0) > 0 || + (data.tokens.output ?? 0) > 0 || + (data.tokens.cache?.read ?? 0) > 0 || + (data.tokens.cache?.write ?? 0) > 0); + if (!hasUsage) { continue; } if (data.providerID == null || data.modelID == null) { continue; } + if (dedupeSet.has(row.id)) { + continue; + } + dedupeSet.add(row.id); + const createdMs = data.time?.created ?? row.time_created;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 280 - 284, The dedupeSet check is happening before JSON parsing/validation so a malformed first row can wrongly block later valid duplicates; move the dedupeSet.has(row.id) check and dedupeSet.add(row.id) to occur only after the row has been successfully parsed and validated (i.e., after your parsing/validation logic for each row succeeds), and ensure any parse/validation errors skip adding the id so later valid rows with the same row.id are processed normally.
🧹 Nitpick comments (1)
apps/opencode/src/data-loader.ts (1)
594-624: Consider using valid LiteLLM model names in tests.Lines 599 and 623 use
'claude-sonnet-4-5'which doesn't match LiteLLM's pricing database entries. While this test only validates pass-through behavior, using consistent model names like'claude-sonnet-4-20250514'would align with the coding guidelines.♻️ Suggested fix
- modelID: 'claude-sonnet-4-5' as v.InferOutput<typeof modelNameSchema>, + modelID: 'claude-sonnet-4-20250514' as v.InferOutput<typeof modelNameSchema>, ... - expect(entry.model).toBe('claude-sonnet-4-5'); + expect(entry.model).toBe('claude-sonnet-4-20250514');As per coding guidelines, "All test files must use current Claude 4 models (claude-sonnet-4-20250514, claude-opus-4-20250514), not outdated Claude 3 models".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 594 - 624, The test data uses an outdated model name 'claude-sonnet-4-5'; update the test to use a current LiteLLM Claude 4 model name (e.g. 'claude-sonnet-4-20250514') so the convertOpenCodeMessageToUsageEntry test uses a valid model string—replace the modelID in the test message object passed to convertOpenCodeMessageToUsageEntry and update the final expect that checks entry.model accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/opencode/src/data-loader.ts`:
- Around line 502-505: The current filter in the JSON loader skips messages when
message.tokens.input and message.tokens.output are both zero even if
message.tokens.cache exists; update the condition around message.tokens in the
loop to only skip when tokens are null or all token counts are zero (input,
output and cache). Concretely, modify the if that references message.tokens so
it continues only when message.tokens == null || (message.tokens.input === 0 &&
message.tokens.output === 0 && (message.tokens.cache == null ||
message.tokens.cache === 0)), leaving the rest of the loader logic unchanged.
---
Duplicate comments:
In `@apps/opencode/src/data-loader.ts`:
- Around line 304-306: The current filter wrongly skips rows when tokens.input
and tokens.output are zero even though cache usage exists; update the emptiness
check around data.tokens to treat a row as empty only if all four fields are
zero (tokens.input, tokens.output, tokens.cache.read, tokens.cache.write) or
tokens is null/undefined. Locate the branch that uses data.tokens (the if
checking data.tokens == null || (data.tokens.input === 0 && data.tokens.output
=== 0)) and modify it to include tokens.cache.read and tokens.cache.write in the
emptiness test so cache-only rows are kept.
- Around line 280-284: The dedupeSet check is happening before JSON
parsing/validation so a malformed first row can wrongly block later valid
duplicates; move the dedupeSet.has(row.id) check and dedupeSet.add(row.id) to
occur only after the row has been successfully parsed and validated (i.e., after
your parsing/validation logic for each row succeeds), and ensure any
parse/validation errors skip adding the id so later valid rows with the same
row.id are processed normally.
---
Nitpick comments:
In `@apps/opencode/src/data-loader.ts`:
- Around line 594-624: The test data uses an outdated model name
'claude-sonnet-4-5'; update the test to use a current LiteLLM Claude 4 model
name (e.g. 'claude-sonnet-4-20250514') so the convertOpenCodeMessageToUsageEntry
test uses a valid model string—replace the modelID in the test message object
passed to convertOpenCodeMessageToUsageEntry and update the final expect that
checks entry.model accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3437e751-1291-4966-958d-7ef28d73eb68
📒 Files selected for processing (1)
apps/opencode/src/data-loader.ts
Replaces object return type with function overloads that return messages or sessions based on mode parameter, enabling selective data loading.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/opencode/src/data-loader.ts (1)
509-511:⚠️ Potential issue | 🟡 MinorApply the same cache-only usage fix to the JSON loader.
For consistency with the SQLite loader fix, this filter should also consider
cache.readandcache.writefields to avoid undercounting cached-token usage.♻️ Proposed fix
// Skip messages with no tokens - if (message.tokens == null || (message.tokens.input === 0 && message.tokens.output === 0)) { + const hasUsage = + message.tokens != null && + ((message.tokens.input ?? 0) > 0 || + (message.tokens.output ?? 0) > 0 || + (message.tokens.cache?.read ?? 0) > 0 || + (message.tokens.cache?.write ?? 0) > 0); + if (!hasUsage) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 509 - 511, The JSON loader currently skips messages when message.tokens is null or both input/output tokens are zero, which undercounts cached usage; modify the filter to also check message.cache.read and message.cache.write so you only continue when tokens are null or both tokens.input and tokens.output are zero AND (message.cache.read || message.cache.write) are falsy/zero. Update the conditional around the "message" check (the block using message.tokens) to include these cache fields (message.cache.read, message.cache.write) so cached-only messages aren't skipped—mirror the same logic used in the SQLite loader fix.
♻️ Duplicate comments (1)
apps/opencode/src/data-loader.ts (1)
277-293:⚠️ Potential issue | 🟠 MajorDeduplicate only after a row survives parsing and validation.
The dedupe check at lines 278-281 marks an ID as seen before JSON parsing and validation. If the first occurrence of an ID is malformed or fails validation, a later valid duplicate will be incorrectly skipped.
♻️ Proposed fix
for (const row of rows) { - if (dedupeSet.has(row.id)) { - continue; - } - dedupeSet.add(row.id); - let parsed: unknown; try { parsed = JSON.parse(row.data); } catch { continue; } const result = v.safeParse(sqliteMessageDataSchema, parsed); if (!result.success) { continue; } const data = result.output; if (data.role !== 'assistant') { continue; } - if (data.tokens == null || (data.tokens.input === 0 && data.tokens.output === 0)) { + const hasUsage = + data.tokens != null && + ((data.tokens.input ?? 0) > 0 || + (data.tokens.output ?? 0) > 0 || + (data.tokens.cache?.read ?? 0) > 0 || + (data.tokens.cache?.write ?? 0) > 0); + if (!hasUsage) { continue; } if (data.providerID == null || data.modelID == null) { continue; } + if (dedupeSet.has(row.id)) { + continue; + } + dedupeSet.add(row.id); + const createdMs = data.time?.created ?? row.time_created;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 277 - 293, The dedupeSet check currently runs before parsing/validation so malformed or invalid first-seen rows can cause later valid duplicates to be skipped; update the loop in data-loader (the for (const row of rows) block) to perform JSON.parse and v.safeParse(sqliteMessageDataSchema, parsed) first, and only after result.success add row.id to dedupeSet and skip duplicates using dedupeSet.has(row.id); ensure you still continue on parse or validation failures and only mark IDs as seen when validation passes.
🧹 Nitpick comments (1)
apps/opencode/src/data-loader.ts (1)
655-748: Test doesn't cover the dedupe edge case with invalid-then-valid duplicates.The test verifies that duplicate IDs are deduplicated when both rows are valid (the second is dropped). However, it doesn't cover the scenario where the first occurrence fails validation but a later duplicate with the same ID is valid—which would incorrectly be skipped with the current dedupe logic.
Consider adding a test case where the first row for an ID has invalid data (e.g., missing required fields or role='user') and a second row with the same ID is valid, then verify the valid row is kept.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/opencode/src/data-loader.ts` around lines 655 - 748, Add a new unit test for loadMessagesFromSqlite that verifies the dedupe logic handles an invalid-first then valid-second duplicate: use createSqliteAdapter to seed messageRows where one ID appears twice with the first row containing invalid JSON or role='user' (so it would be filtered/invalid) and the second row valid (assistant with required fields); call loadMessagesFromSqlite and assert the output includes the valid record (and not dropped) — reference loadMessagesFromSqlite and createSqliteAdapter when locating where to add the test and name the test something like "keeps valid duplicate when earlier duplicate is invalid".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/opencode/src/data-loader.ts`:
- Around line 509-511: The JSON loader currently skips messages when
message.tokens is null or both input/output tokens are zero, which undercounts
cached usage; modify the filter to also check message.cache.read and
message.cache.write so you only continue when tokens are null or both
tokens.input and tokens.output are zero AND (message.cache.read ||
message.cache.write) are falsy/zero. Update the conditional around the "message"
check (the block using message.tokens) to include these cache fields
(message.cache.read, message.cache.write) so cached-only messages aren't
skipped—mirror the same logic used in the SQLite loader fix.
---
Duplicate comments:
In `@apps/opencode/src/data-loader.ts`:
- Around line 277-293: The dedupeSet check currently runs before
parsing/validation so malformed or invalid first-seen rows can cause later valid
duplicates to be skipped; update the loop in data-loader (the for (const row of
rows) block) to perform JSON.parse and v.safeParse(sqliteMessageDataSchema,
parsed) first, and only after result.success add row.id to dedupeSet and skip
duplicates using dedupeSet.has(row.id); ensure you still continue on parse or
validation failures and only mark IDs as seen when validation passes.
---
Nitpick comments:
In `@apps/opencode/src/data-loader.ts`:
- Around line 655-748: Add a new unit test for loadMessagesFromSqlite that
verifies the dedupe logic handles an invalid-first then valid-second duplicate:
use createSqliteAdapter to seed messageRows where one ID appears twice with the
first row containing invalid JSON or role='user' (so it would be
filtered/invalid) and the second row valid (assistant with required fields);
call loadMessagesFromSqlite and assert the output includes the valid record (and
not dropped) — reference loadMessagesFromSqlite and createSqliteAdapter when
locating where to add the test and name the test something like "keeps valid
duplicate when earlier duplicate is invalid".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb60e287-57f7-4e0b-94c1-8f8b8ef6dafd
📒 Files selected for processing (1)
apps/opencode/src/data-loader.ts
Extracts token validation into reusable function that checks cache tokens. Moves deduplication after validation to avoid processing invalid entries twice.
Author
|
@ryoppippi this is an intentional duplicate of #850 so solve the same issue, but with:
AI Disclosure: I threw gpt-5.4 at the first PR and adressed coderabbit + ran it locally. Initial credit goes to @dobbymaniac and kudos to @toliner for the added manual tests 🙏 |
|
Hi, I found bug for this PR. If a session uses multiple model, cost calculation doesn't work correctly. I ran In this session, I use claude-opus-4.6 for main model and minimax-m2.5-free for compaction. |
Converts dotted Claude model names (e.g. claude-opus-4.5) to dash format (claude-opus-4-5) before pricing lookup to ensure consistent model name resolution.
Author
|
@toliner it seem it could have have been a naming issue when pulling claude models names from LiteLLM, could you pull the branch and try again ? 👀 |
|
@unsync Fixed, thank you! |
unsync
changed the title
Author
|
@yukukotani i see you made #879 For the sake of reducing duplicates, do you have a take on both implementations ? |
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
2 participants
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
Initial credits to #850
Duplicate: #879
Why this change was made
OpenCode moved its usage data to
opencode.dbin newer releases, soccusagewas missing recent OpenCode activity and falling back to stale legacy JSON data.Description
This PR adds SQLite-backed OpenCode loading with a legacy JSON fallback so the OpenCode reports stay accurate across both old and new OpenCode storage formats.
~/.local/share/opencode/opencode.dbwhen present, while preserving the existing JSON-file path for older installs.dailyandsessionreports consistent by sharing one combined SQLite load path instead of re-reading usage and session data separately.bun:sqlitewhenbetter-sqlite3is unavailable or cannot open under Bun, which fixes local Bun-based CLI runs.better-sqlite3as an optional OpenCode dependency and updates workspace build approval/catalog entries for the new native package.Blast Radius
apps/opencodeplus the minimal workspace dependency metadata needed to ship the SQLite reader.ccusageagainst newer OpenCode installs that now store usage in SQLite instead of JSON files.Practical Consequence
better-sqlite3directly, because the loader now falls back tobun:sqliteinstead of dropping to legacy JSON.Decisions
better-sqlite3optional and externalized it so Node builds can use it while Bun runtime still has a supported fallback.Verification
pnpm --filter @ccusage/opencode run testsuccessfully.pnpm --filter ccusage run testsuccessfully.pnpm typechecksuccessfully.pnpm --filter @ccusage/opencode run start dailysuccessfully under Bun and confirmed it now prints a real report instead of falling back after a SQLite open failure.Summary by CodeRabbit
New Features
Chores
Tests