refactor: make isArchived the sole session archive authority - #3074
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughProblem solvedThe PR makes It removes It migrates legacy SQLite archive data, removes obsolete status projections, and updates protocol compatibility and continuity schemas. Source of truthThe PR extends the existing Runtime Host retirement remains the only lifecycle orchestration path. Complexity deltaThe PR removes:
The PR adds:
These changes remove duplicate authorities, lifecycle states, public mutation paths, and status branches. The migration and protocol changes support compatibility. Total maintenance complexity decreases, subject to verification of migration ordering and compatibility behavior. Simplification opportunitiesThe PR already removes unused archive methods, the generic session configuration update path, obsolete status projections, and redundant test fixtures. No further deletion is evident without weakening migration compatibility or behavioral regression coverage. Risks and validationConcrete risks include:
The PR reports focused tests, builds, typechecks, formatting checks, and lint checks as passed. Full-repository tests were not run locally. Required checks remain unverified without direct CI evidence. Review-relevant risksThe current diff affects public TypeScript contracts, SQLite persistence and migration behavior, protocol compatibility, and user-visible session archive and status behavior. Material changes in these areas require independent human review under repository policy. No security, licensing, release, or governance effect was identified in the current diff summary. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe change makes ChangesSession archive state migration
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to This refactor changes session archive authority and compatibility behavior, but archived sessions may still be able to accept configuration changes through a public update path, and peer compatibility is not directly behavior-tested. Those issues could allow invalid state changes or integration failures, so they should be fixed or explicitly accepted before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
3f68c0a to
ce8f729
Compare
Migrate legacy archived Session payloads, remove archivedAt and the duplicate SQLite status projections, and preserve execution status across archive transitions. Generated-by: Codex
ce8f729 to
58ac73f
Compare
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)
packages/runtime/src/session-manager.ts (1)
1569-1588: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlock configuration updates for archived Sessions.
updateSessionwrites configuration fields without checkingisArchived. An archived Session can updatemodel,backend, orpermissionModethrough this path.transitionSessionConfigurationrejects the same operation at Line 1145.Apply the same archive guard before configuration writes, or route these updates through
transitionSessionConfiguration.As per path instructions, “Review the diff adversarially against the problem it claims to solve.”
Source: Path instructions
🧹 Nitpick comments (2)
packages/runtime-host/src/__tests__/protocol.test.ts (1)
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the protocol boundary, not only the constants.
These assertions only compare exported constants with literals. They do not prove that epoch 20 or schema 3 frames are rejected, or that current frames are accepted. Add decoder or handshake behavior checks. Keep the numeric assertions only as a separate release-number check if the exact values are an explicit contract.
As per path instructions, flag tests that assert implementation details or do not protect observable behavior.
Also applies to: 55-55
Source: Path instructions
packages/ui/stories/session-list-panel.stories.tsx (1)
202-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve archived-row coverage.
isArchivedremains an observable UI state.packages/ui/src/session-history-list.tsx, Line 588-694, selects archive or unarchive actions fromsession.isArchived. Keep a fixture with a valid execution status, such asdone, andisArchived: true; otherwise this story no longer covers the archived-row rendering and unarchive action.As per path instructions, flag tests or fixtures that do not protect observable behavior.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23f076b0-b88e-4f97-959d-d657a12e3ba8
📒 Files selected for processing (57)
apps/desktop/src/main/__tests__/app-shell-session-purge.test.tsapps/desktop/src/main/__tests__/desktop-transcript-range-store.test.tsapps/desktop/src/main/__tests__/runtime-host-bot-session-adapter.test.tsapps/desktop/src/main/__tests__/runtime-host-client-uds.test.tsapps/desktop/src/main/__tests__/runtime-host-desktop-candidate.test.tsapps/desktop/src/main/__tests__/runtime-host-session-execution-ipc-main.test.tsapps/desktop/src/main/__tests__/runtime-host-session-observer.test.tsapps/desktop/src/main/runtime-host-bot-session-adapter.tsapps/desktop/stories/settings/settings-pages.stories.tsxpackages/cli/src/__tests__/runtime-host-session-driver.test.tspackages/core/src/session.tspackages/runtime-host/scripts/transcript-data-plane-benchmark.mjspackages/runtime-host/src/__tests__/fixtures/execution-host-suite.tspackages/runtime-host/src/__tests__/goal-coordinator.test.tspackages/runtime-host/src/__tests__/host-kernel.test.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/__tests__/runtime-resource-coordinator.test.tspackages/runtime-host/src/__tests__/session-effect-coordinator.test.tspackages/runtime-host/src/__tests__/session-projector.test.tspackages/runtime-host/src/__tests__/session-retirement-coordinator.test.tspackages/runtime-host/src/__tests__/session-retirement-protocol.test.tspackages/runtime-host/src/protocol/index.tspackages/runtime-host/src/protocol/session-continuity.tspackages/runtime-host/src/protocol/session-retirement.tspackages/runtime-host/src/server/canonical-session-projection.tspackages/runtime-host/src/server/context-coordinator.tspackages/runtime-host/src/server/deep-research-coordinator.tspackages/runtime-host/src/server/execution-composition.tspackages/runtime-host/src/server/goal-coordinator.tspackages/runtime-host/src/server/hosted-execution-recovery.tspackages/runtime-host/src/server/plan-coordinator.tspackages/runtime-host/src/server/root-turn-coordinator.tspackages/runtime-host/src/server/runtime-resource-coordinator.tspackages/runtime-host/src/server/scheduled-task-coordinator.tspackages/runtime-host/src/server/session-catalog-coordinator.tspackages/runtime-host/src/server/session-effect-coordinator.tspackages/runtime-host/src/server/session-retirement-coordinator.tspackages/runtime-host/src/server/session-revision-coordinator.tspackages/runtime/src/__tests__/runtime-event-read-model.test.tspackages/runtime/src/__tests__/runtime-kernel-interaction.test.tspackages/runtime/src/__tests__/session-manager-terminal-ledger.test.tspackages/runtime/src/__tests__/session-manager.test.tspackages/runtime/src/__tests__/stream-graph-coordinator.test.tspackages/runtime/src/agent-run.tspackages/runtime/src/runtime-kernel.tspackages/runtime/src/session-manager.tspackages/runtime/src/stream-graph-coordinator.tspackages/storage/src/__tests__/goal-authority.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/session-store.tspackages/storage/src/sqlite-session-metadata-schema.tspackages/storage/src/sqlite-session-metadata-store.tspackages/ui/src/conversation-copy.tspackages/ui/src/session-status-presentation.tspackages/ui/stories/session-list-panel.stories.tsx
💤 Files with no reviewable changes (6)
- packages/runtime/src/tests/session-manager-terminal-ledger.test.ts
- packages/runtime/src/tests/runtime-kernel-interaction.test.ts
- packages/runtime-host/src/tests/runtime-resource-coordinator.test.ts
- packages/runtime-host/src/server/canonical-session-projection.ts
- packages/runtime/src/tests/runtime-event-read-model.test.ts
- apps/desktop/src/main/tests/runtime-host-client-uds.test.ts
Reject execution and workspace configuration changes through the legacy SessionManager update path when the Session is archived. Restore archived-row Storybook coverage with an independent done execution status. Generated-by: Codex
Delete the uncalled generic SessionManager update path instead of preserving a second configuration authority. Remove its implementation-detail tests; the versioned configuration transition, permission, and relocation paths retain their behavioral coverage. Generated-by: Codex
Combine main's removal of the legacy timestamp read marker with this branch's removal of direct archive and generic configuration mutation paths. Keep both deletions and retain the versioned message read marker and Runtime Host retirement authorities. Generated-by: Codex
|
Thanks for the work here. I did another independent review pass focused on first principles, migration safety, Occam’s razor, and test quality. The core model is correct:
However, I don’t think the PR is merge-ready yet. P1 — Must fix before merge[P1] Schema version 25 conflicts with current
|
|
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. |
|
Thanks for the thorough pass, @YayoiNanoka — all points landed. Verified on head (7fb0f50) with evidence:
Ran the relevant suites against head: One remaining P2 (I'd take the deferral): epoch 24 doesn't isolate this PR's own retirement wire change. Removing the P3 (non-blocking): migration 27's ELSE branch removes only AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要感谢 @YayoiNanoka 的深入审查,全部意见已落地(head 7fb0f50 已核实):① schema 冲突→迁移改为 v27(v25=#3159 review/done、v26=#3165),迁移链 1..27 连续无跳号,含 main-v26→v27 与降级护栏测试;② 意外恢复归档→迁移 27 保守归一化(任一历史信号:json.isArchived ∨ json.status='archived' ∨ sql.is_archived ∨ sql.status='archived' ∨ archivedAt 存在 → is_archived=1),JSON 与 SQL 同事务写回,测试覆盖 json-only/sql-only/status-only/archivedAt-only/矛盾/active-unchanged;旧 writer 路径核实:archive/unarchive 始终同步 SQL status 列、restore 清 archivedAt,active 行不会残留归档痕迹,无误归档路径;③ epoch→24 与 #3165 一致,且是行为拒绝测试(伪造 epoch-23 host 在握手期被拒)非常量断言;④ writer 拆分真实(updateHeaderSync 对携带 isArchived 的 patch 直接 throw、setArchivedSync 唯一落盘路径、类型层 isArchived?: never 双封,update()/setExecutionBoundaryKindSync 均经守卫,Bot adapter 只读 isArchived);⑤ 毫秒精度统一(unixepoch subsec*1000 / Date.now())。本地实跑相关 suites 全绿(metadata-store 45、handshake、retirement-protocol 2、retirement-coordinator 21、protocol 28、session-status、session-store 16)。剩余 1 个 P2(建议接受延后):epoch 24 未隔离本 PR 自身的 retirement wire 变更——移除 session-retirement.ts:55 的 (status==='archived')===archived 校验与客户端 connection.ts:514 的 assertOutputForInput 是对 wire 契约的放宽,只骑 #3165 的 23→24 bump 而未再 bump:epoch-24 旧客户端连本宿主时归档/恢复会在握手之后以 invalidProtocolFrame 失败而非干净拒绝。鉴于 host+client 从单一 @maka/runtime-host 原子发布、混合版本仅手动部署且失败是可恢复的操作错误(无数据损坏),提议显式延后并在评审注明;如评审偏好严格"每个 wire 变更都 bump"纪律也可 bump 25。P3(不阻塞):迁移 27 ELSE 分支只删 |
Summary
SessionHeader.isArchivedthe sole durable and public authority for Session archive statearchivedfromSessionStatus, remove SessionarchivedAt, and collapse duplicate archive guardsisArchivedfrom generic header patchesSessionManager.updateSessionconfiguration path instead of preserving a second configuration authoritystatus/status_updated_atshadow columns and indexRefs #2984
Verification
node --test packages/storage/dist/__tests__/sqlite-session-metadata-store.test.js— 43 passednode --test packages/runtime/dist/__tests__/session-manager.test.js packages/runtime/dist/__tests__/model-factory-thinking.test.js— 246 passednode --test packages/cli/dist/__tests__/runtime-host-session-driver.test.js— 27 passednpm run format:checknpm run lintFull-repository tests were not run locally; CI owns that coverage.
Migration
Schema v25 maps legacy
status: "archived"payloads toactive, removes staleblockedReason,statusUpdatedAt, andarchivedAt, increments their metadata revision, and then drops the two duplicate status projection columns. The indexedis_archivedprojection remains unchanged.Mapping legacy archived rows to
activematches the previous restore behavior: the old archive write overwrote the prior execution status, so no more precise state can be recovered. New archive and restore operations preserve the current execution status.AI use
Select exactly one:
Tool(s) and scope: Codex performed the simplification audit, implemented the migration and authority consolidation, adjudicated review feedback, added and ran focused tests, and drafted this PR description. Claude and independent Codex subagents performed read-only adversarial reviews. The human contributor owns review and submission.
Checklist
Does this PR entail a change in behavior?