Skip to content

feat: persist external session origins - #3103

Merged
Astro-Han merged 10 commits into
apache:mainfrom
Sun-GLiang:feat/issue-3081-external-session-origin
Aug 19, 2026
Merged

feat: persist external session origins#3103
Astro-Han merged 10 commits into
apache:mainfrom
Sun-GLiang:feat/issue-3081-external-session-origin

Conversation

@Sun-GLiang

@Sun-GLiang Sun-GLiang commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Persist immutable external adapter/session provenance for imported Sessions in SQLite schema v25.
  • Add a batched lookup that returns the complete durable import count and newest eight surviving published Session IDs.
  • Enrich external Session catalog items with durable history and orthogonal in-flight state while preserving the 72 KiB response bound.
  • Bump the closed Runtime Host compatibility epoch for the wire-contract change.
  • Make the Desktop catalog authoritative across remounts, repeat imports, in-flight polling, and commit_outcome_unknown recovery, 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 typecheck
  • npm --workspace @maka/runtime-host run typecheck
  • npm --workspace @maka/desktop run typecheck
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook (131 stories)
  • npm run format:check
  • npm run lint
  • git diff --check

Review 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:

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

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: Codex trailer.

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

@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Summary

This PR solves missing provenance for imported external Sessions. It stores immutable adapterId and sourceSessionId metadata in SQLite schema v27. It exposes durable import counts, recent imported Session IDs, and active import state through the Runtime Host. The Desktop catalog remains authoritative across remounts, repeat imports, polling, and commit_outcome_unknown recovery.

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 ImportTasksSettingsPage test harness adds substantial maintenance cost. Shared test utilities could simplify it if they preserve coverage for recovery, polling, pagination, stale responses, retries, and filter preservation. No deletion is justified from the available evidence.

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 windows_recovery failure was reported for an unrelated named-pipe ACL fixture. The repository is clean, and no uncommitted changes remain. Required check status remains unverified without direct current check output.

Complexity delta

  • Authorities: SQLite metadata becomes authoritative for durable import history. External provenance no longer relies on Session revision lineage.
  • States: The PR adds never-imported, imported-history, active-import, and unknown-outcome recovery states.
  • Branches: The PR adds pagination, polling, stale-response protection, retry, and recovery branches.
  • Configuration and limits: The PR adds a schema migration, compatibility epoch update, an eight-ID protocol limit, and bounded lookup limits.
  • Public surface: The PR adds SessionExternalOrigin, ExternalSessionCatalogItem.importState, lookup APIs, updated store signatures, and typed preload projections.
  • Test burden: The PR adds storage, protocol, Runtime Host, and Desktop regression coverage. The Desktop page harness is the main maintenance increase.
  • Removed paths: The former summary-only external-session projection is replaced by a catalog-item projection that carries import state.

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

  • The PR changes public storage, Runtime Host, protocol, preload bridge, and SessionHeader contracts. These changes require independent human review under repository policy.
  • SQLite schema v27 adds nullable provenance columns and a partial index. Migration correctness, immutability, and backward compatibility require independent human review under repository policy.
  • The PR changes user-visible import counts, progress, retry behavior, recovery behavior, and localized messages. These changes require independent human review under repository policy.
  • The Runtime Host compatibility epoch changes from 24 to 25. Compatibility and release effects require independent human review under repository policy.
  • Imported-session persistence and lookup now affect remount and cross-client behavior. These changes require independent human review under repository policy.
  • No licensing or governance effect was identified in the change set.
  • Required checks are unverified. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

The 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.

Changes

External import tracking

Layer / File(s) Summary
Persist external import provenance
packages/core/src/session.ts, packages/storage/src/..., packages/runtime/src/...
Imported sessions store immutable adapter and source-session identifiers. SQLite schema version 27 adds indexed provenance columns. Storage exposes bounded import lookups.
Expose host catalog import state
packages/runtime-host/src/protocol/..., packages/runtime-host/src/server/...
Catalog items include validated imported counts, recent imported session IDs, and active-import state. Runtime compatibility epoch increases to 25.
Update Desktop import workflow
apps/desktop/src/preload/..., apps/desktop/src/renderer/settings/..., apps/desktop/src/renderer/locales/..., apps/desktop/stories/...
The page supports paginated loading, polling, stale-response protection, unknown-outcome recovery, retries, localized status, and opening imported tasks.
Validate behavior
apps/desktop/src/main/__tests__/*, packages/runtime-host/src/__tests__/*, packages/storage/src/__tests__/*
Tests cover durable history, protocol validation, persistence, migration, catalog polling, pagination, recovery, retries, filters, localization, and IPC import state.

Estimated code review effort: 4 (Complex) | ~60 minutes

Merge Risk: 🟡 Moderate · up to e1a52

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
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: persisting external session origins.
Description check ✅ Passed The description includes the required summary, verification, AI-use, checklist, and behavior sections with relevant implementation and review details.
Linked Issues check ✅ Passed The changes implement issue #3081 by persisting provenance, exposing import state, and updating Runtime Host and Desktop behavior.
Out of Scope Changes check ✅ Passed The changes remain focused on issue #3081, including supporting storage, protocol, Runtime Host, Desktop, Storybook, and test updates.
Ai Use Disclosure ✅ Passed The PR selects only substantive generative-tool use, names Codex and its scope, and all 8 introduced commits contain the valid standalone trailer Generated-by: Codex.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@Sun-GLiang
Sun-GLiang force-pushed the feat/issue-3081-external-session-origin branch from ace8ce4 to b974af1 Compare August 16, 2026 07:32
@Sun-GLiang
Sun-GLiang marked this pull request as ready for review August 16, 2026 07:32
@github-actions
github-actions Bot requested a lite review from Copilot August 16, 2026 07:32

Copilot AI 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.

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 lookupExternalSessionImports query; imported Sessions now persist immutable externalOrigin.
  • 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_unknown with 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.

Comment thread packages/storage/src/sqlite-session-metadata-schema.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 value

Delete the dead recoveredSource !== undefined check.

Line 319 throws when recoveredSource is undefined, so the narrowing at line 346 is already established. The recoveredSessionId !== undefined test 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 win

Replace the real sleeps with node:test mock timers.

Enable mock.timers before renderPage, call mock.timers.runAll(), flush pending microtasks, and reset timers in afterEach. This removes the three-second wall-clock delay and avoids coupling the tests to EXTERNAL_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

📥 Commits

Reviewing files that changed from the base of the PR and between a032beb and b974af1.

📒 Files selected for processing (27)
  • apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts
  • apps/desktop/src/main/__tests__/runtime-host-external-sessions-ipc-main.test.ts
  • apps/desktop/src/main/runtime-host-external-sessions-ipc-main.ts
  • apps/desktop/src/preload/bridge-contract.d.ts
  • apps/desktop/src/preload/external-session-catalog.ts
  • apps/desktop/src/preload/preload.ts
  • apps/desktop/src/renderer/locales/external-session-import-copy.ts
  • apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx
  • apps/desktop/src/renderer/settings/settings-surface.tsx
  • apps/desktop/stories/settings/settings-pages.stories.tsx
  • packages/core/src/session.ts
  • packages/runtime-host/src/__tests__/external-session-coordinator.test.ts
  • packages/runtime-host/src/__tests__/external-session-protocol.test.ts
  • packages/runtime-host/src/__tests__/protocol.test.ts
  • packages/runtime-host/src/protocol/external-session.ts
  • packages/runtime-host/src/protocol/index.ts
  • packages/runtime-host/src/server/external-session-coordinator.ts
  • packages/runtime/src/__tests__/runtime-ledger-repair.test.ts
  • packages/storage/src/__tests__/external-session-importer.test.ts
  • packages/storage/src/__tests__/session-store.test.ts
  • packages/storage/src/__tests__/sqlite-session-metadata-store.test.ts
  • packages/storage/src/execution-stores.ts
  • packages/storage/src/external-session-importer.ts
  • packages/storage/src/index.ts
  • packages/storage/src/session-store.ts
  • packages/storage/src/sqlite-session-metadata-schema.ts
  • packages/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.

Comment thread apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx
@github-actions
github-actions Bot requested a lite review from Copilot August 16, 2026 08:10

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

The failing CI job appears to be a transient Desktop E2E timing failure rather than a regression from this PR.

The only failure was:

  • e2e/streaming-remount.spec.ts:75
  • Timed out waiting for the “停止” button to become visible.

Verification on the same commit:

  • Focused test: passed
  • Focused test repeated 10 times: 10/10 passed
  • Full Desktop E2E suite: 26 passed, 1 skipped
  • The same E2E job also passed on the preceding PR commit
  • All other CI jobs passed

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?

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

@Astro-Han

The failing CI job appears to be a transient Desktop E2E timing failure rather than a regression from this PR.

The only failure was:

  • e2e/streaming-remount.spec.ts:75
  • Timed out waiting for the “停止” button to become visible.

Verification on the same commit:

  • Focused test: passed
  • Focused test repeated 10 times: 10/10 passed
  • Full Desktop E2E suite: 26 passed, 1 skipped
  • The same E2E job also passed on the preceding PR commit
  • All other CI jobs passed

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?

@Sun-GLiang
Sun-GLiang force-pushed the feat/issue-3081-external-session-origin branch from d714f40 to 1ccd5ae Compare August 17, 2026 02:39
@github-actions
github-actions Bot requested a lite review from Copilot August 17, 2026 02:39

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

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.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

@Astro-Han Could you please rerun the failed windows_recovery job?

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 RuntimeHostEndpointError: insecure_endpoint_directory while applying the named-pipe ACL:

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 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.

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 仍需重新跑绿。

Comment thread apps/desktop/src/preload/preload.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 57371ca and e1a52a5.

📒 Files selected for processing (4)
  • apps/desktop/src/main/__tests__/external-session-catalog-projection.test.ts
  • apps/desktop/src/main/runtime-host-external-sessions-ipc-main.ts
  • apps/desktop/src/preload/external-session-catalog.ts
  • apps/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.

Comment thread apps/desktop/src/preload/preload.ts
@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

@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?

@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.

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 状态复核该问题。

Comment thread packages/storage/src/sqlite-session-metadata-schema.ts
@Sun-GLiang
Sun-GLiang force-pushed the feat/issue-3081-external-session-origin branch from e1a52a5 to a573527 Compare August 18, 2026 13:15

@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.

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 日志复核结论。

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

@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?

@Astro-Han

Copy link
Copy Markdown
Contributor

/agentic_review

@qodo-code-review

qodo-code-review Bot commented Aug 18, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Sparse IDs bypass validation ✓ Resolved 🐞 Bug ≡ Correctness
Description
Array.prototype.map skips empty array slots, so a sparse importedSessionIds array such as
Array(1) passes the length and count checks without validating every position and is returned as
malformed protocol state. This violates the closed decoder’s fail-closed entity-ID contract and can
propagate an undefined imported-task identity into catalog projections.
Code

packages/runtime-host/src/protocol/external-session.ts[R263-265]

+  const importedSessionIds = state.importedSessionIds.map((id) =>
+    requireEntityId(id, 'imported Session id'),
+  );
Relevance

●●● Strong

Accepted PR #3079 fixed the identical sparse-array map bypass in a Runtime Host protocol decoder.

PR-#3079

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The decoder only verifies that the value is an array and that its length is within the item limit
before calling .map; because JavaScript .map preserves and skips sparse holes, Array(1) can
reach the return path without requireEntityId ever being called, while the count comparison still
relies only on array length. This is the same fail-open decoder pattern previously identified and
fixed for sparse runningTurnIds in accepted PR #3079.

packages/runtime-host/src/protocol/external-session.ts[257-269]
packages/runtime-host/src/protocol/external-session.ts[257-274]
PR-#3079

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Reject sparse `importedSessionIds` arrays instead of allowing `Array.prototype.map` to skip unoccupied positions. The current behavior violates the decoder's fail-closed entity-ID contract and can return unvalidated, malformed protocol state.

## Issue Context
The new `importedSessionIds` field is part of a closed Runtime Host protocol surface, where every array position must pass `requireEntityId`. Length validation does not establish that every numeric position exists, and the current mapping callback is not invoked for holes; this is the same decoder pattern previously fixed for sparse `runningTurnIds`.

Iterate numeric indexes or use `Array.from` so every position is passed to `requireEntityId`, and add a regression test confirming that a sparse array is rejected.

## Fix Focus Areas
- packages/runtime-host/src/protocol/external-session.ts[257-269]
- packages/runtime-host/src/__tests__/external-session-protocol.test.ts[103-174]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
Review mode: 🧠 Deep: This is a dense, cross-cutting behavioral change spanning SQLite migration and queries, a closed protocol contract, Runtime Host coordination, and Desktop pagination/recovery races, making independent review passes materially valuable.

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread packages/runtime-host/src/protocol/external-session.ts Outdated

@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.

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。

@Astro-Han

Copy link
Copy Markdown
Contributor

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.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Added the requested annotated composite below. It shows:

  1. completed import history together with a Host-authoritative in-flight state; and
  2. a commit_outcome_unknown result recovered through the Host catalog, including the direct task entry.

The screenshots were captured from the current PR head (8dea32a3d) using the Import Tasks Storybook fixtures.
pr-3103-import-states-review

@Astro-Han

Copy link
Copy Markdown
Contributor

Human merge confirmation: Astro-Han reviewed the final diff at 8dea32a3dc and explicitly authorized this squash merge. This satisfies the independent human-review gate for the public protocol, migration, and user-visible behavior. All required checks are green, the current head is approved, and there are no unresolved review threads. The squash commit will retain Generated-by: Codex.

@Astro-Han
Astro-Han merged commit e2da1ba into apache:main Aug 19, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

external sessions: an imported task does not record where it came from

3 participants