fix(session): migrate legacy session statuses - #3159
Conversation
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review. 📝 WalkthroughSummaryThis PR removes legacy The PR extends the existing sources of truth. Core defines the current status union. The shared This is the smallest coherent solution. The migration protects persisted data. The shared decoder avoids duplicate validation logic. The migration uses one clock read to preserve timestamp precision and consistency. No code or test can be removed without weakening behavior or regression coverage. Validation and risksFocused tests cover the status contract, protocol normalization and rejection, SQLite v24-to-v25 migration, metadata revision updates, timestamp preservation, catalog listing, and reopen behavior. Builds, formatting, linting, type checking, and focused tests reportedly pass. The full test suite has a pre-existing timeout in The migration updates affected metadata revisions and commit timestamps once. It preserves Review-relevant risks
The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe session status contract removes ChangesSession status compatibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change normalizes legacy session statuses during migration and compatibility decoding while preserving strict handling of other unknown values. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ProtocolClient
participant SessionCatalog
participant SessionContinuity
participant decodeSessionStatus
ProtocolClient->>SessionCatalog: send session status
SessionCatalog->>decodeSessionStatus: decode status
ProtocolClient->>SessionContinuity: send continuity status
SessionContinuity->>decodeSessionStatus: decode status
decodeSessionStatus-->>SessionCatalog: normalized or validated status
decodeSessionStatus-->>SessionContinuity: normalized or validated status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR narrows the session-status domain to the six current values and makes legacy review/done statuses safe to read by migrating persisted SQLite rows and normalizing those legacy wire values during protocol decoding.
Changes:
- Bumps the SQLite session metadata schema to v25 and migrates stored
review/donestatuses toactive. - Removes
review/donefrom core and UI session-status models/copy/presentation. - Adds Runtime Host decode-only compatibility that maps legacy
review/donetoactive, while still rejecting all other unknown statuses; includes focused tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/stories/session-list-panel.stories.tsx | Removes legacy-status story fixtures now that the UI model no longer includes them. |
| packages/ui/src/session-status-presentation.ts | Drops review/done presentation mapping to match the narrowed status union. |
| packages/ui/src/conversation-copy.ts | Removes legacy status labels from both zh/en copy maps. |
| packages/storage/src/sqlite-session-metadata-schema.ts | Bumps schema to v25 and adds migration rewriting persisted legacy statuses to active. |
| packages/storage/src/tests/sqlite-session-metadata-store.test.ts | Adds migration regression test ensuring legacy statuses normalize once and preserve statusUpdatedAt. |
| packages/runtime-host/src/protocol/session-status.ts | Introduces decode helper that normalizes legacy wire values to active and rejects others. |
| packages/runtime-host/src/protocol/session-continuity.ts | Switches continuity decoding to use the new legacy-normalizing decoder. |
| packages/runtime-host/src/protocol/session-catalog.ts | Switches catalog decoding to use the new legacy-normalizing decoder. |
| packages/runtime-host/src/tests/session-catalog-protocol.test.ts | Adds tests for legacy normalization and strict rejection in catalog projections. |
| packages/runtime-host/src/tests/protocol.test.ts | Adds tests for legacy normalization and strict rejection in continuity snapshots. |
| packages/core/src/session.ts | Removes review/done from SESSION_STATUSES. |
| packages/core/src/tests/session-status.test.ts | Adds a contract test asserting only the current six statuses are accepted. |
💡 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 (1)
packages/runtime-host/src/protocol/session-catalog.ts (1)
871-871: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInline
decodeSessionStatus.Line 871 only forwards its value to
decodeSessionStatus. CalldecodeSessionStatusat the projection field and deletesessionStatus. This removes redundant indirection.Proposed simplification
- status: sessionStatus(record.status), + status: decodeSessionStatus(record.status), ... -function sessionStatus(value: unknown): SessionStatus { - return decodeSessionStatus(value); -}As per path instructions, flag concrete cases where code can be deleted or simplified.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d94dd8cb-a72b-4042-a8ed-a74e535d4810
📒 Files selected for processing (12)
packages/core/src/__tests__/session-status.test.tspackages/core/src/session.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/__tests__/session-catalog-protocol.test.tspackages/runtime-host/src/protocol/session-catalog.tspackages/runtime-host/src/protocol/session-continuity.tspackages/runtime-host/src/protocol/session-status.tspackages/storage/src/__tests__/sqlite-session-metadata-store.test.tspackages/storage/src/sqlite-session-metadata-schema.tspackages/ui/src/conversation-copy.tspackages/ui/src/session-status-presentation.tspackages/ui/stories/session-list-panel.stories.tsx
💤 Files with no reviewable changes (3)
- packages/core/src/session.ts
- packages/ui/stories/session-list-panel.stories.tsx
- packages/ui/src/session-status-presentation.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Generated-by: Codex
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/storage/src/sqlite-session-metadata-schema.ts:912
committed_atis computed from two separatestrftime(..., 'now')calls. If the second rolls over between the%sand%fevaluations, the combined millisecond timestamp can jump backwards by up to ~999ms (e.g., seconds from the previous tick with fractional seconds from the next), which can also make the new migration test flaky (committedAt >= migrationStartedAt). Prefer a single-source timestamp expression.
committed_at = MAX(
committed_at,
CAST(strftime('%s', 'now') AS INTEGER) * 1000
+ CAST(substr(strftime('%f', 'now'), 4, 3) AS INTEGER)
)
Generated-by: Codex
|
Maintainer follow-up pushed in 91057e0. The bug and migration are real, and the overall v25 + mixed-version decoder approach is the right authority boundary. I changed the migration timestamp to a single |
|
LGTM — merging. The migration is well-tested (revision advance, statusUpdatedAt preserved, legacy wire normalization without epoch bump), CI is green, and the AI-use disclosure is complete. Note for the parallel PRs: this PR takes schema v25 — #3165 and #3074 must renumber to v26 (and bump epoch past) when they rebase. AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on |
Summary
reviewanddonesession statuses toactivein SQLite schema v25Fixes #3058
Verification
npm --workspace @maka/core run buildnpm --workspace @maka/storage run buildnpm --workspace @maka/runtime-host run buildnpm --workspace @maka/ui run buildnpm run format:checknpm run lintnpm run typecheckFull
npm testpasses in every workspace except@maka/runtime, where the pre-existingnode-pty-write-lifecycletest times out after 10 seconds withETIMEDOUT. The same failure reproduces on the branch base and standalone; this PR does not touch that test or PTY implementation.Migration
Opening an existing workspace upgrades session metadata from schema v24 to v25 in the existing transaction. Rows with
reviewordonein either persisted status representation are normalized toactive, their metadata revision is advanced once, andstatusUpdatedAtis preserved. Downgrade-opening the migrated database with older binaries is not supported.AI use
Tool(s) and scope: OpenAI Codex implemented the migration, compatibility decoder, status cleanup, and tests under human direction. Each affected commit includes a
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?