feat: persist external session origins - #3103
Conversation
|
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:
📝 WalkthroughSummaryThis PR solves missing provenance for imported external Sessions. It stores immutable The PR extends the existing storage, Runtime Host, preload, and Desktop catalog flow. It does not create a parallel import-state path. SQLite metadata is the durable source of truth, and Desktop consumes a typed projection of the Host catalog item. The solution is the smallest coherent path shown by the change set. The migration, indexed batched lookup, bounded protocol results, catalog pagination, polling, and recovery branches directly support the required behavior and the 72 KiB response limit. The eight-ID limit bounds storage queries and protocol payloads. The Validation includes storage, Runtime Host, and Desktop tests and typechecks, Storybook build and smoke tests, formatting, linting, and diff checks. The reported Desktop E2E suite passed with 26 tests and 1 skipped. The focused test passed 10 consecutive times after a transient timeout while waiting for “停止”. A Windows Complexity delta
Total maintenance complexity increases. The increase is justified by durable cross-client state, bounded responses, and unknown-outcome recovery. The current evidence does not support simplification without reducing regression coverage. Review-relevant risks
WalkthroughThe change persists external-session provenance, exposes validated import state through storage and the runtime host, and updates the Desktop import page with durable history, polling, pagination, unknown-outcome recovery, localized status, and imported-task opening. ChangesExternal import tracking
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to Desktop pagination cannot display the remaining item count because the page-total field is missing from the exposed bridge contract. Merge should wait until the IPC and bridge types expose that value consistently. Sequence Diagram(s)sequenceDiagram
participant ImportTasksSettingsPage
participant DesktopBridge
participant HostCoordinator
participant SessionMetadataStore
ImportTasksSettingsPage->>DesktopBridge: Request catalog page
DesktopBridge->>HostCoordinator: List external sessions
HostCoordinator->>SessionMetadataStore: Lookup external imports
SessionMetadataStore-->>HostCoordinator: Counts and recent session IDs
HostCoordinator-->>DesktopBridge: Catalog with importState
DesktopBridge-->>ImportTasksSettingsPage: Render import status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ace8ce4 to
b974af1
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds durable provenance for imported external Sessions by persisting the external adapter/source identity in Storage (schema v25), exposes batched import-state lookups through the Host external-session catalog protocol, and updates Desktop’s “Import tasks” UI to rely on authoritative catalog state across remounts and unknown commit outcomes.
Changes:
- Storage: schema v25 adds external-origin columns and a batched
lookupExternalSessionImportsquery; imported Sessions now persist immutableexternalOrigin. - Runtime Host protocol: external-session catalog items now include
importState(count, newest imported IDs, in-flight flag) and bumps compatibility epoch. - Desktop: import-tasks page renders durable import history, survives remounts, polls in-flight state, and recovers from
commit_outcome_unknownwith UI + tests/stories.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/storage/src/sqlite-session-metadata-store.ts | Writes external-origin columns on insert; enforces immutability; adds batched external import lookup query. |
| packages/storage/src/sqlite-session-metadata-schema.ts | Bumps schema to v25; adds external-origin columns + index migration. |
| packages/storage/src/session-store.ts | Adds external-origin + import-lookup API surface and validation; normalizes/validates persisted headers. |
| packages/storage/src/index.ts | Re-exports external import lookup types/limits. |
| packages/storage/src/external-session-importer.ts | Passes external origin through to imported session creation. |
| packages/storage/src/execution-stores.ts | Threads new createImportedSession signature and exposes lookup method via execution store wrapper. |
| packages/storage/src/tests/sqlite-session-metadata-store.test.ts | Adds migration coverage for v24→v25 without backfill. |
| packages/storage/src/tests/session-store.test.ts | Adds coverage for external-origin immutability and import lookup correctness/bounds/malformed persisted data. |
| packages/storage/src/tests/external-session-importer.test.ts | Verifies origin forwarding and persistence across reopen. |
| packages/runtime/src/tests/runtime-ledger-repair.test.ts | Updates imported-session creation call sites for new signature. |
| packages/runtime-host/src/server/external-session-coordinator.ts | Enriches catalog with durable importState + in-flight state; factors import key; updates pagination sizing logic. |
| packages/runtime-host/src/protocol/index.ts | Bumps compatibility epoch for the protocol contract change. |
| packages/runtime-host/src/protocol/external-session.ts | Adds importState to external-session catalog wire shape + decoder validation; introduces max imported-id constant. |
| packages/runtime-host/src/tests/protocol.test.ts | Updates epoch assertion to new value and wording. |
| packages/runtime-host/src/tests/external-session-protocol.test.ts | Adds round-trip and rejection tests for required importState. |
| packages/runtime-host/src/tests/external-session-coordinator.test.ts | Adds coordinator tests for importState projection, batching, polling interaction, and size bounds. |
| packages/core/src/session.ts | Introduces SessionExternalOrigin and persists it on SessionHeader as immutable provenance. |
| apps/desktop/stories/settings/settings-pages.stories.tsx | Updates story fixtures to new catalog item shape; adds unknown-outcome recovery story. |
| apps/desktop/src/renderer/settings/settings-surface.tsx | Wires onOpenImported through settings surface to import page. |
| apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx | Implements durable import-state UI (import history, open-latest link, polling, unknown-outcome recovery). |
| apps/desktop/src/renderer/locales/external-session-import-copy.ts | Adds localized strings for durable import state, retry/recovery, and “open latest imported task”. |
| apps/desktop/src/preload/preload.ts | Updates bridge typings to use Desktop-projected external-session catalog items. |
| apps/desktop/src/preload/external-session-catalog.ts | Adds Desktop projection type for Host external-session catalog items (cwd mapping). |
| apps/desktop/src/preload/bridge-contract.d.ts | Updates external session list return type to Desktop-projected catalog items. |
| apps/desktop/src/main/runtime-host-external-sessions-ipc-main.ts | Maps Host hostCwd → Desktop cwd while preserving new importState payload. |
| apps/desktop/src/main/tests/runtime-host-external-sessions-ipc-main.test.ts | Updates IPC tests for required importState in list results. |
| apps/desktop/src/main/tests/import-tasks-settings-page.test.ts | Adds comprehensive DOM-level tests for durable import state, polling, and unknown-outcome recovery behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx (1)
344-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the dead
recoveredSource !== undefinedcheck.Line 319 throws when
recoveredSourceisundefined, so the narrowing at line 346 is already established. TherecoveredSessionId !== undefinedtest carries the remaining work.♻️ Proposed simplification
- const recoveredSessionId = recoveredSource?.importState.importedSessionIds[0]; + const recoveredSessionId = recoveredSource.importState.importedSessionIds[0]; const landed = - recoveredSource !== undefined && recoveredSessionId !== undefined && (recoveredSource.importState.importedCount > attempt.importedCountBefore || recoveredSessionId !== attempt.latestImportedSessionIdBefore);apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts (1)
119-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the real sleeps with
node:testmock timers.Enable
mock.timersbeforerenderPage, callmock.timers.runAll(), flush pending microtasks, and reset timers inafterEach. This removes the three-second wall-clock delay and avoids coupling the tests toEXTERNAL_SESSION_IMPORT_POLL_MS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6eb934af-7751-41b8-a8ca-e529f993f279
📒 Files selected for processing (27)
apps/desktop/src/main/__tests__/import-tasks-settings-page.test.tsapps/desktop/src/main/__tests__/runtime-host-external-sessions-ipc-main.test.tsapps/desktop/src/main/runtime-host-external-sessions-ipc-main.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/external-session-catalog.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/locales/external-session-import-copy.tsapps/desktop/src/renderer/settings/import-tasks-settings-page.tsxapps/desktop/src/renderer/settings/settings-surface.tsxapps/desktop/stories/settings/settings-pages.stories.tsxpackages/core/src/session.tspackages/runtime-host/src/__tests__/external-session-coordinator.test.tspackages/runtime-host/src/__tests__/external-session-protocol.test.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/protocol/external-session.tspackages/runtime-host/src/protocol/index.tspackages/runtime-host/src/server/external-session-coordinator.tspackages/runtime/src/__tests__/runtime-ledger-repair.test.tspackages/storage/src/__tests__/external-session-importer.test.tspackages/storage/src/__tests__/session-store.test.tspackages/storage/src/__tests__/sqlite-session-metadata-store.test.tspackages/storage/src/execution-stores.tspackages/storage/src/external-session-importer.tspackages/storage/src/index.tspackages/storage/src/session-store.tspackages/storage/src/sqlite-session-metadata-schema.tspackages/storage/src/sqlite-session-metadata-store.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
|
The failing CI job appears to be a transient Desktop E2E timing failure rather than a regression from this PR. The only failure was:
Verification on the same commit:
The failed run did not publish its Playwright trace artifact, so there is no additional renderer/runtime evidence to diagnose. I did not add a speculative code change for a failure that cannot currently be reproduced. Could a maintainer please re-run the failed E2E job? |
|
d714f40 to
1ccd5ae
Compare
1ccd5ae to
57371ca
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@Astro-Han Could you please rerun the failed The only failing check is the Windows Local IPC trust-boundary fixture. The Runtime Host endpoint unit tests passed, but the fixture subsequently failed with https://github.com/maka-agent/maka-agent/actions/runs/32109625079/job/95626046535?pr=3103 This appears unrelated to #3103:
A rerun should confirm whether this was a transient Windows runner/ACL failure. If it reproduces, I suggest tracking it separately as a flake or regression related to #3182 rather than changing #3103. |
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed exact head 57371ca9bb9bdda7dd4536a486a0dff832d219cb, including the complete diff, linked issue, current CI, prior reviews and replies, and all review threads.
The ownership model is sound. Storage owns one immutable external origin, Runtime Host owns the combined durable/in-flight import projection, and Desktop consumes that catalog instead of promoting page-local state into a second authority. The SQLite v27 migration, partial index, bounded lookup, exact protocol decoder, compatibility-epoch bump, and post-enrichment 72 KiB sizing are all placed at the right boundaries. The earlier index and stale recovery-banner findings are fixed on this head.
I found one P2 at the Desktop identity boundary. importState.importedSessionIds remains in Host-local identity space, while every real Desktop Session uses a scoped desktopSessionKey. The catalog list path does not perform the projection that the successful import path already performs, so the new “Open latest imported task” actions deterministically fail to select the real task. I left the concrete owner and minimal correction inline. I found no other P0–P3 issue.
A non-blocking follow-up remains around numeric offset pagination over a live updated-at-sorted catalog: rows can reorder between page reads, causing recovery search to miss its target and remain uncertain. It fails closed and is retryable, so stable cursor semantics do not need to broaden this fix.
The issue’s total-count / “N remaining” outcome was explicitly proposed as a separate pagination change before assignment, so I accept that code scope. Please nevertheless keep that outcome tracked: Fixes #3081 would otherwise close the issue without a concrete follow-up.
I would not split this PR or delete the mounted lifecycle tests. After the scoped-ID projection is fixed, the design is appropriately cohesive and minimal.
CI is currently unstable only because windows_recovery failed in the unchanged Local IPC ACL fixture with insecure_endpoint_directory; the affected tests, typechecks, E2E, Storybook, and package checks are green. Please rerun that required check before merge.
Recommendation: revise, then re-review.
Disclosure: Codex performed the read-only source, lifecycle, identity-contract, protocol, test, CI, and prior-feedback analysis. The human contributor remains responsible for independently verifying the finding and owns the final review and merge decision.
中文说明
整体 owner 和生命周期方向正确。当前唯一 P0–P3 问题是 Host catalog 的 imported Session ID 没有在 preload 投影成 Desktop 的多 Host session key,因此“打开最近导入的任务”对真实数据无法导航。应在 preload 单一投影边界修复并补 bridge regression。
动态 catalog 上的数字 offset 稳定性可以作为非阻塞 follow-up;“N remaining”也应单独跟踪。当前 Windows recovery CI 仍需重新跑绿。
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 66170437-03cc-4542-8ba4-e3373d55c024
📒 Files selected for processing (4)
apps/desktop/src/main/__tests__/external-session-catalog-projection.test.tsapps/desktop/src/main/runtime-host-external-sessions-ipc-main.tsapps/desktop/src/preload/external-session-catalog.tsapps/desktop/src/preload/preload.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/runtime-host-external-sessions-ipc-main.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
|
@Astro-Han The P2 finding in #3103 (comment) has been fixed in e1a52a5, and all CI checks are green. Could you please re-review the latest head and resolve the conversation if the fix looks correct? |
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
The external-origin design is well aligned with the existing ownership boundaries: durable identity stays in storage, Runtime Host owns reconciliation, and Desktop consumes a projection instead of inventing another authority. The focused CI coverage is green.
The remaining issue is integration with current main: both branches independently claim schema migration 27. The simplest correct solution is to preserve main's archive-authority migration as 27, move this PR's external-origin migration to 28, bump the schema version, and add the corresponding 27-to-28 migration assertion. That keeps the schema monotonic and avoids any compatibility path or parallel schema authority.
Review performed with Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I verified the finding against the latest PR head, current main, the merge conflict, and live CI state.
中文评论
外部来源身份的整体设计符合现有职责边界:持久身份由 storage 管理,Runtime Host 负责协调,Desktop 只消费投影,没有新建并行权威。相关 CI 已通过。
当前唯一问题是与最新 main 的集成:两边分别占用了 schema migration 27。最简单且正确的处理是保留 main 的归档权威迁移为 27,把本 PR 的 external-origin 迁移顺延为 28,提升 schema version,并补充 27 到 28 的迁移断言。这样能保持 schema 单调升级,无需兼容分支或并行实现。
本次审查使用了 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具;我已依据最新 PR head、当前 main、实际合并冲突和实时 CI 状态复核该问题。
Generated-by: Codex
e1a52a5 to
a573527
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The latest head resolves my previous migration finding correctly: current main retains archive-authority migration 27, while external-origin storage is introduced monotonically as migration 28 with an explicit 27-to-28 fixture. The rest of the ownership model remains coherent: origin is written atomically when the imported Session is created, cannot be rewritten through header updates, Runtime Host owns reconciliation, and Desktop projects host-scoped identities.
I found no new reproducible P0-P2 issue on a573527. This approval supersedes my review on the prior head. The only red check is the unrelated slash-command-menu E2E lookup; focused storage/Host checks pass.
Re-review performed with two Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I verified the result against latest main, a clean merge tree, and live CI logs.
中文评论
最新 head 已正确解决我此前提出的 migration 问题:当前 main 保留 archive-authority migration 27,external-origin storage 单调顺延为 migration 28,并增加明确的 27 到 28 fixture。其余职责模型也保持闭环:origin 在 imported Session 创建时原子写入,不能通过 header update 改写,Runtime Host 管理 reconciliation,Desktop 投影 host-scoped identity。
在 a573527 上未发现新的可复现 P0-P2。本 APPROVE 覆盖我对旧 head 的评论。唯一红色检查是无关的 slash-command-menu E2E 元素查找;storage/Host 相关检查通过。
本次复审使用了两位 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具;我已依据最新 main、干净的 merge tree 和实时 CI 日志复核结论。
|
@Astro-Han Thanks for approving the latest head. The only remaining failed required check is the unrelated Desktop E2E job:
Could you please rerun the failed E2E job when convenient? |
|
/agentic_review |
Code Review by Qodo
1.
|
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
The current head closes the two previously material integration gaps: migration ownership is correctly rebased as v27/v28, and Host-local imported session IDs are projected into Desktop identity space at the preload boundary with focused coverage. Storage remains the atomic provenance authority, Runtime Host owns durable/importing state, and the query/protocol limits are coherent.
No remaining P0-P3 findings. I verified the fixes and resolved the two superseded review threads. Offset pagination under a concurrently changing external catalog can remain a non-blocking retryability follow-up; it does not justify expanding this PR.
AI-assisted review disclosure: two Codex reviewers independently reviewed exact head 8dea32a, current CI, migrations, identity projection, and thread state.
中文说明
当前 head 已解决两个实质集成问题:迁移编号按 main 的权威正确调整为 v27/v28,Host-local imported session ID 也在 preload 边界投影到 Desktop identity space,并有针对性测试。没有剩余 P0-P3;两个已被修复的旧线程已 resolve。动态目录下的 offset pagination 可作为非阻塞后续,不需要扩大本 PR。
|
This PR changes the Desktop import-tasks experience and its visible history and in-flight states. Could you please add screenshots showing the updated import catalog, including a completed history state and an in-flight or recovery state? One annotated composite is fine. Thanks! Posted by Codex on behalf of Astro-Han. |
|
Human merge confirmation: Astro-Han reviewed the final diff at |

Summary
commit_outcome_unknownrecovery, with localized UI and a newest-task entry.Fixes #3081
Verification
npm --workspace @maka/storage test(821 passed, 14 existing skips)npm --workspace @maka/runtime-host test(944 passed)npm --workspace @maka/desktop test(876 passed)npm --workspace @maka/storage run typechecknpm --workspace @maka/runtime-host run typechecknpm --workspace @maka/desktop run typechecknpm --workspace @maka/desktop run build-storybooknpm --workspace @maka/desktop run smoke:storybook(131 stories)npm run format:checknpm run lintgit diff --checkReview focus
This changes a public closed protocol contract and migrates Session metadata to schema v25. It requires independent human review and is not eligible for the self-merge fast path. Please pay particular attention to the migration transaction, batched provenance lookup filters/order, 72 KiB catalog pagination, and Desktop unknown-outcome recovery races.
AI use
Select exactly one:
Tool(s) and scope: Codex implemented and tested the Core, Storage, protocol, Runtime Host, and Desktop changes under human direction. Each materially authored commit includes a
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?