Skip to content

feat(storage): scan OpenCode sessions in foreign-session store - #5055

Closed
wutongyuonce wants to merge 4 commits into
apache:mainfrom
wutongyuonce:feat/storage-opencode-foreign-scan
Closed

wutongyuonce wants to merge 4 commits into
apache:mainfrom
wutongyuonce:feat/storage-opencode-foreign-scan

Conversation

@wutongyuonce

@wutongyuonce wutongyuonce commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #5053.

Add OpenCode to the foreign-session scanner (list + digest), not the full import path. Reads stay bounded and read-only; child sessions and tool output are excluded.

Follow-up on review:

  • SQL-filter archived sessions (time_archived IS NULL) so they cannot fill the scan window
  • Make the digest walk actually bounded (oversize break, non-string byte accounting, row cap, one shared budget for messages + parts)
  • Reuse codexCwdSqlVariants for directory matching
  • Drop synthetic text parts from the digest

Verification

  • npm --workspace @maka/core run build && npm --workspace @maka/storage run build
  • node --test packages/core/dist/__tests__/foreign-session.test.js packages/storage/dist/__tests__/foreign-session-store.test.js — 75 pass
  • npx biome check on the four touched files — clean
  • Not run: live scan against a real OpenCode install (checklist on the original PR)

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: pi(gpt5.6 sol) authored the scanner implementation, review follow-up, and tests. Commits carry Generated-by: pi.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

List and digest OpenCode sessions from ~/.local/share/opencode/opencode.db
in the foreign-session store, with the same enable-flag, sanitize, and
read-only digest contract as Claude Code and Codex.

Generated-by: pi
Iterate OpenCode message/part rows newest-first and stop at the digest
byte cap instead of loading the whole session. Filter, order, and LIMIT
session listing in SQL the same way Codex does.

Generated-by: pi

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review (Command Code) — not an approval

Two bounded-read defects in the new OpenCode path: one makes archived sessions visible, the other means the digest read is not actually bounded. Both are small fixes. Line numbers are the head revision.

P2 (Should-Fix) — archived OpenCode sessions are listed as resumable, and they can crowd live ones out of the 100-row window.

The OpenCode session reader never looks at time_archived:

// packages/storage/src/foreign-session-store.ts:988-995
const selected = [
  'id',
  'title',
  'directory',
  'time_created',
  'time_updated',
  'parent_id',
].filter((column) => columns.has(column));

and the where clauses cover only the age window, parent_id, and cwd (:997-1010). There is no archived predicate anywhere in the OpenCode path — I checked every archived occurrence in the file: :705/:710 are the Codex sibling (if (columns.has('archived')) where.push('(archived IS NULL OR archived = 0)')), i.e. Codex filters archived and OpenCode does not. The adapter this PR's sibling (#5125) reuses also treats a non-null time_archived as archived.

Failure mode: a user-archived OpenCode conversation reappears in the CLI resume picker as resumable. Worse, the ordering is ORDER BY time_updated DESC LIMIT 100 (:1013) — the cap is applied before any JS filtering, so archived rows consume slots and can push live sessions out of the window entirely.

Smallest sound fix: add time_archived to the allowlist and where.push('time_archived IS NULL') when the column exists (mirroring the Codex path), plus a test that seeds an archived row and asserts it is absent.

P2 (Should-Fix) — the digest byte cap does not bound the work it exists to bound.

// :931-945
for (const value of iterable) {
  const rec = asObject(value);
  if (rec === undefined) continue;
  const dataRaw = rec.data;
  const nbytes = typeof dataRaw === 'string' ? Buffer.byteLength(dataRaw) : 0;
  if (nbytes > FOREIGN_SESSION_DIGEST_MAX_READ_BYTES) {
    truncated = true;
    continue;                                   // <- keeps iterating the whole table
  }
  if (used + nbytes > FOREIGN_SESSION_DIGEST_MAX_READ_BYTES) {
    truncated = true;
    break;
  }
  used += nbytes;
  rows.push(rec);
}

Two holes in the same loop:

  1. The oversize branch uses continue, not break, so a single row larger than the cap does not stop the walk — it keeps iterating (and paying Buffer.byteLength on every remaining large value) to the end of the message/part table. Work is bounded by the table, not by the cap.
  2. nbytes is 0 for a non-string data (NULL/BLOB/numeric), so such rows are pushed without accounting. Memory is then bounded only by row count, not by the byte cap.

The PR's own commit is described as bounding these reads, and this path is reached from the UI resume flow (readDigest), so the input is an untrusted local database. A database with many tiny/NULL parts, or many multi-megabyte parts, turns this into a full-table walk holding O(rows) objects.

Smallest sound fix: add a hard row cap and exit the loop once it is exceeded, and change the oversize branch to break.

P3 (Nice-to-have) — the cwd SQL prefilter is weaker than its Codex sibling.

variants = [...new Set([cwdFilter, normalizePath(cwdFilter)])] (:1009) omits the separator and trailing-separator forms that the Codex path generates via codexCwdSqlVariants. A stored directory with a trailing separator (or the alternate separator) is filtered out in SQL before the authoritative JS normalizePath comparison can match it. Reusing codexCwdSqlVariants(cwdFilter) fixes it.

Note on duplication. This PR reimplements a read-only OpenCode SQLite reader in the store (~250 lines: schema introspection, JSON decoding, child/parent rules) alongside OpenCodeSessionAdapter, and that copy has already drifted — the archived rule is the drift. The linked issue keeps the adapter as the single DB authority, so the durable shape is the adapter as the only reader with the #1057 bounds layered in the store. That is not required to fix the defects above, but it is what prevents the next drift.

Verified clean (so the fixes stay narrow): withOpenCodeDb opens the database readOnly: true and never writes; the DB path is realpath-confined to the OpenCode home with a basename check, and re-confined in the digest read; all SQL identifiers come from fixed allowlists with bound parameters, and a session id cannot reach a path; child sessions are rejected in both scan and digest; the digest privacy contract holds (user/assistant text plus tool-input file paths only — state.output is never read, thinking excluded, shared sanitize/redact pipeline); and the new source is wired through every enumeration (FOREIGN_SESSION_SOURCES, foreignSourceLabel, availableSources, listSessions, readDigest).

Review-relevant risks. No public contract, security boundary, dependency or licensing effect identified. The new behavior is user-visible (what the resume picker offers) and reads an untrusted local database, so it warrants independent human review under CONTRIBUTING.md.

Required conclusion.

  1. Optimal for the actual problem? No — the bounds are the right goal and partially present, but archived filtering is missing and the digest cap does not bound the walk.
  2. Production code that can be deleted? The duplicated OpenCode reader, in favor of OpenCodeSessionAdapter plus a bounds layer (see the note above).
  3. Low-quality tests to delete or replace? none identified — the added tests are behavioral (flag gating, parent/child, cwd, caps, digest privacy, DB-symlink refusal). The gap is missing coverage: there is no archived-exclusion test, no test for the oversize/non-string-data path, and none for cwd separator forms.
  4. Deeper refactor required? Yes, as described in the note — adapter-backed reads with the scan/digest caps in the store.
  5. Ready to merge? Not as-is; the two P2s should be fixed first.
  6. Residual risks / verification gaps: no live run against a real OpenCode install (the PR checklist says so); OpenCode synthetic text parts (compaction/system-injected) are not excluded the way Claude's equivalents are — whether a real database places such text in data.text is unverified.

Approval boundary. This is automated review; it is not an approval. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.

Filter time_archived in SQL so archived rows cannot fill the scan
window. Count non-string SQLite payloads, stop at the first oversize
row, and share one byte/row budget across message and part walks.
Match cwd separator forms the same way Codex does, and drop synthetic
text parts from digests.

Generated-by: pi
@wutongyuonce

Copy link
Copy Markdown
Contributor Author

Thanks for the review — addressed in 7fa5b41.

P2 archived sessions listed as resumable. time_archived is now on the column allowlist, and listing adds time_archived IS NULL when the column exists. A test seeds 55 newer archived rows plus one live session and asserts only the live id survives, so archived rows cannot fill the 100-row SQL window or the 50-session result cap.

P2 digest read was not actually bounded. collectSqliteRowsUntilReadCap now:

  • breaks on the first oversize row instead of continueing through the rest of the table
  • counts non-string data (objects / Uint8Array) toward the byte cap
  • stops at a hard 2048-row walk cap
  • shares one byte/row budget across the message walk and the part walk, so the two iterators cannot add up to 4MB / 4096 rows

Tests cover the oversize-break path, non-string payloads, the row cap, and leftover-budget sharing.

P3 cwd SQL prefilter. OpenCode directory matching now reuses codexCwdSqlVariants, same trailing-slash / separator forms as Codex. Added a test that a session stored as /repo/one/ is found when listing with { cwd: '/repo/one' }.

Also fixed (not in the original notes). Digest extraction now drops OpenCode synthetic: true text parts the same way Claude drops meta/compact records, so compaction / injected text does not enter the handoff digest.

Left as follow-up, not this PR: routing scan/digest through OpenCodeSessionAdapter. The adapter is the full import path (unbounded, includes archived as archived: true); the scanner has a different bounded read-only contract, and folding them together is the deeper refactor you flagged rather than a merge blocker for #5053.

@wutongyuonce

Copy link
Copy Markdown
Contributor Author

Follow-up hardening is included in commit fe014855d:

  • Fixed legacy OpenCode sessions with NULL time_updated by falling back to time_created in SQL filtering and ordering.
  • Redacted secrets in digest file paths at the shared digest boundary.
  • Added bounded SQLite projections for IDs, metadata, payload reads, and shared byte/row limits to avoid unbounded materialization.
  • Failed closed when legacy schemas lack parent_id, since child-session history cannot be ruled out safely.
  • Added regression coverage for all of the above.

Validation: the focused core/storage foreign-session suites pass (78 tests), both packages build and typecheck, Biome checks pass, and git diff --check passes. No unrelated refactor was included.

@wutongyuonce

wutongyuonce commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

这个 PR 解决什么问题?

OpenCode 会把会话保存在本机的 SQLite 数据库里,但 Maka 之前只能读取 Claude Code 和 Codex 的会话。这个 PR 让 Maka 也能发现并导入 OpenCode 会话:列出最近的会话、按工作目录筛选,并在用户选择后提取一份安全的摘要,帮助用户继续之前的工作,而不用重新解释上下文。摘要只保留用户消息、助手文字和涉及过的文件路径,不把工具输出、思考过程或系统提示词带进新的会话。

Review 过程中发现并修复了什么?

  1. 有些旧会话会被错误地隐藏。 OpenCode 的旧数据库里,time_updated 可能为空,但 time_created 仍然是有效时间。原来的 SQL 只看前者,导致明明是最近的会话却查不出来。现在会在 time_updated 为空时使用 time_created

  2. 文件路径里可能带着密钥。 某些路径可能包含 API key 等敏感信息。之前消息会脱敏,但文件路径经过摘要入口时没有统一脱敏。现在所有进入摘要的路径都会经过清理和密钥脱敏。

  3. 数据库里的超大字段可能被一次性读进内存。 “最多读这么多字节”的限制如果放在 JavaScript 层才做,就已经晚了:SQLite 可能先把超大的消息、标题或 ID 完整取出来。现在查询本身就只取有界数据,并同时限制字节数和行数,避免恶意或异常数据库拖垮 Maka。

  4. 无法确认父子关系时不能冒险导入。 如果旧版数据库没有 parent_id,Maka 无法判断某个会话是不是子代理会话,贸然导入可能把不该暴露的子代理历史带出来。现在这种情况会安全地拒绝读取(fail closed)。

这些修复都补了回归测试。当前验证结果:相关 core/storage 测试 78 个全部通过,构建、类型检查、Biome 检查和 diff 检查均通过。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on OpenCode support and tightening the read bounds. I reviewed fe014855d73135e8a3bced2c8c759eca7ef36f50, including the TUI caller and the existing OpenCode adapter/Host importer.

P1 — Resolve the import direction before adding a second OpenCode reader. The normal TUI selection path would run readOpenCodeDigestnewSession → a synthesized handoff prompt, while Desktop imports the same source through OpenCodeSessionAdapterStoredMessage[] → the Host importer. This PR additionally duplicates SQLite opening, schema discovery, session filtering, and message/part interpretation in foreign-session-store.ts. A future source-format correction would need to reach both readers, and the two surfaces would continue producing different kinds of resumed history.

The missing OpenCode entry is real. However, this overlaps #5125, where I have already withdrawn my earlier digest-specific guidance and proposed one external-session import flow. The current text of #5053 still asks for the old digest path, so I owe you a clearer scope before asking for more implementation work. Sorry for that inconsistency.

Could you pause the incremental fixes here while that scope is aligned? The direction is to reuse the existing Host catalog/import operations from TUI, keep OpenCode format conversion in its existing adapter, and open the resulting native Session. Once that replacement covers the supported behavior, remove the superseded scanner/digest handoff. Before implementation, settle how imported history enters model context and preserve the existing provenance and bounded-read constraints. No additional projection framework or importer is needed.

The inline P2 records a reproduced long-session failure as a constraint for that design discussion, rather than a request for another round of digest-specific patches.

Validation: exact-head core/storage builds and 78 focused tests passed. An additional SQLite probe through createForeignSessionStore reproduced the long-session failure. AI-assisted review with Codex and Reviewer Sol; I cross-checked the production callers, reproduction, and earlier scope discussion. No live OpenCode/TUI acceptance was performed.

);
truncated = truncated || collected.truncated;
remainingBytes = Math.max(0, remainingBytes - collected.usedBytes);
remainingRows = Math.max(0, remainingRows - collected.usedRows);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — The row budget can discard every visible message in a long session. Reachability: normal long-session selection in TUI. The reader spends the shared 2048-row budget on message records before reading any part records. At 2048 messages, remainingRows is zero, so no text parts survive even when the total payload is well below 2 MiB. An exact-head SQLite probe through createForeignSessionStore().listSessions() and readDigest() returned empty userMessages and assistantTexts for 2048 messages with small text parts, plus the inaccurate warning that the transcript exceeded 2097152 bytes. The TUI then starts a new Session with that empty handoff.

For the scope discussion in the review body, the bound needs to preserve a useful, coherent message/part window, or explicitly reject an import that cannot be completed; it must not report a usable trailing window while dropping all visible text. Please carry a regression for this case into the agreed single-reader/import design rather than adding another digest-specific workaround.

@wutongyuonce

wutongyuonce commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

This feature has been redesigned and reimplemented under Issue #5053. Therefore, this PR will be closed to avoid duplicating the implementation.

@wutongyuonce
wutongyuonce deleted the feat/storage-opencode-foreign-scan branch September 14, 2026 11:45
@Astro-Han

Copy link
Copy Markdown
Contributor

谢谢前面的实现和修复,也看到你已经关闭这个 PR,避免重复工作。我已在 #5053 确认统一方案,这里补充同步一下:这个 PR 的旧 scanner/digest 方向不用再继续修补或恢复推进了。

后续由一个整合 PR 端到端完成 TUI 复用现有 Host catalog/import、生成并打开原生 Maka Session,同时删除被替代的 reader、digest handoff 和失去用途的接口。完整导入或失败,保证原子性;具体上限用真实数据验证。

之前发现的真实问题、来源格式证据、读取加固和有价值的回归测试可以迁入现有 Adapter,不必丢弃。请与 #5125 的作者协调复用,避免重复实施。感谢这几轮投入,后续按统一契约推进即可。

由 Codex 根据我的确认整理并发布。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(cli): 统一 TUI 与 Desktop 的外部会话导入流程

3 participants