fix(storage): retire completed legacy migration metadata - #3227
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)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review. 📝 WalkthroughSummaryThis PR retires obsolete legacy metadata tables after the atomic operational-state migration. It prevents startup failures caused by strict validation of those tables. The PR extends the existing migration. It does not create a parallel path. It validates released schemas, exact evidence keys, cutover rows, references, and migration state before cleanup. Invalid or unfamiliar state fails closed and remains unchanged. The added validation and regression tests are necessary to make destructive cleanup safe. The removed timestamp-ordering check preserves released writer behavior. No code or tests can be removed without reducing safety or regression coverage. Validation included focused tests, build and formatting checks, migration replay, SQLite integrity validation, and schema-version convergence. The full storage suite remains incomplete because unrelated tests require an unavailable Darwin Complexity delta
Total maintenance complexity decreases. The added complexity is necessary to protect destructive cleanup. Review-relevant risksThe migration can delete legacy metadata tables and change startup and recovery behavior. Material changes in these areas require independent human review under repository policy. The PR adds the exported No security, licensing, release, or governance effect was identified in the current diff. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe migration now validates exact released legacy schemas, metadata rows, and store-specific cutover evidence before cleanup. Invalid schemas, journal keys, values, or import sources block migration and preserve legacy state. Completed-row timestamp ordering is no longer validated. ChangesOperational metadata migration
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to This change retires legacy migration metadata, but chronologically inconsistent completed migration evidence may still be accepted and deleted, risking loss of migration state. Merge should wait for this bounded correctness issue to be fixed or explicitly accepted by the owner. Sequence Diagram(s)sequenceDiagram
participant OperationalStateMigration
participant SchemaValidator
participant LegacyMetadata
participant CutoverJournal
OperationalStateMigration->>SchemaValidator: validate released legacy schemas
OperationalStateMigration->>LegacyMetadata: validate legacy metadata rows
OperationalStateMigration->>CutoverJournal: validate store-specific journal evidence
CutoverJournal-->>OperationalStateMigration: valid evidence or migration-blocking error
OperationalStateMigration->>LegacyMetadata: drop validated metadata tables
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoRetire completed legacy migration metadata during storage convergence
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b74f9b4-a059-4bab-be8e-52a58db44229
📒 Files selected for processing (2)
packages/storage/src/__tests__/operational-state-store.test.tspackages/storage/src/operational-state-store.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
AI-assisted review by Codex. I inspected the current head 6f27a, the historical released writers and table definitions, the atomic migration path, the focused tests, current CI, and the existing Qodo/CodeRabbit feedback.
The problem definition and the chosen seam are sound: obsolete migration metadata should be retired inside the existing atomic schema-convergence transaction, and any state this build cannot recognize should leave the database unchanged. The transaction and rollback ordering preserve that boundary correctly.
The remaining gap is the authority for “released and completed.” Today that decision is split between column-only DDL recognition and permissive row checks. The two inline P2 findings are facets of that same invariant. The smallest coherent correction is to consolidate recognition at the existing schema-signature seam and accept only journal contracts emitted by released writers, while keeping the DROP statements in this transaction.
I would not add a completed_at >= started_at requirement: the released writer used separate Date.now() calls and guaranteed only non-negative timestamps, so wall-clock rollback can produce a legitimate reverse ordering.
中文审查意见
问题定义和处理位置是正确的：应在现有原子 schema convergence 事务内清理已完成的旧迁移元数据，无法识别的状态必须保持数据库原样。
当前剩余问题是“已发布且已完成”的权威定义被拆成了两套不完整判断：只看列形状的 DDL 判断，以及过于宽松的行内容判断。两条行内 P2 属于同一个不变量缺口。最小而完整的方案是复用现有 schema signature 能力，并且只接受历史发布 writer 实际产生过的 journal contract，DROP 仍保留在当前事务中。
不建议增加 completed_at >= started_at：历史 writer 分两次调用 Date.now()，只保证非负数，并不保证墙上时钟单调。
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/storage/src/operational-state-store.ts (1)
470-501: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix now — reject
completed_atvalues that precedestarted_at.Lines 480-481 accept any two non-negative integers. A row with
started_at = 20andcompleted_at = 10passes validation, and the migration then drops internally inconsistent cutover evidence. Add the ordering check after both integer checks.Disposition: fix-now.
Proposed fix
!isNonnegativeInteger(row.started_at) || !isNonnegativeInteger(row.completed_at) || + row.completed_at < row.started_at || typeof row.validation_json !== 'string'Source: Path instructions
🧹 Nitpick comments (1)
packages/storage/src/operational-target-schema.ts (1)
93-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse plain
CREATE TABLEfor consistency. SQLite stores the canonical SQL withoutIF NOT EXISTS, so this does not affect signature matching.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e1c5bfa-04d2-4fa4-a3a9-0c168d3bf4f6
📒 Files selected for processing (3)
packages/storage/src/__tests__/operational-state-store.test.tspackages/storage/src/operational-state-store.tspackages/storage/src/operational-target-schema.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The latest head correctly closes the full-schema half of the prior review: it reuses the existing sqlite_schema signature seam, matches the released DDL variants, rejects altered constraints and attached objects, and keeps retirement inside the existing atomic migration. The released store-name set is also complete, and the current required checks are green.
One part of the previous invariant remains open: store identity is recognized, but validation evidence is only type-checked, not matched to the released writer for that store. Three independent reviewer passes reproduced the same known-store/unknown-key path; the external DeepSeek pass independently verified the historical writer contracts and the focused 35-test result. The inline P2 is the smallest remaining correction. A completed_at >= started_at constraint should not be added because the released writer used independent wall-clock reads.
I am leaving a review comment rather than approving this head. AI-assisted review by Codex with three independent reviewer agents and OpenCode Go DeepSeek V4 Flash (high); I verified the current head, historical DDL and writers, transaction boundary, focused test evidence, live CI, and deduplicated bot feedback.
中文
最新 head 已正确关闭完整 schema 识别问题：复用现有 sqlite_schema signature，覆盖发布过的 DDL，拒绝修改约束和额外对象，并保持在原子迁移事务中。store_name 白名单也完整，CI 全绿。
但上一轮不变量仍有一半未闭合：当前只识别 store 名，validation_json 仍只做类型检查，没有匹配该 store 的历史 writer evidence contract。已知 store 加未知 key 会被接受并删除 journal。三轮独立 reviewer 均复现了该路径，DeepSeek 也核对了历史 writer contract。请按行内 P2 补齐后再 Approve。不应增加 completed_at >= started_at，因为历史 writer 使用独立 wall-clock 读取。
本次使用 Codex、三个独立 reviewer 和 OpenCode Go DeepSeek V4 Flash high；核对了当前 head、历史 DDL/writer、事务边界、focused tests、实时 CI，并去重了 bot feedback。
Astro-Han [P2] on apache#3227: the store_name allowlist closed the unknown-store path, but the evidence contract was still open — a completed `session_metadata` row with `validation_json = {"anything":0}` passed and the journal was dropped, though no released writer ever emitted that key. Missing or extra keys on any known store were accepted the same way, so the retirement still deleted migration evidence this build never wrote. Replace the store-name set with a map from each released `store_name` to the exact validation-key set its `importAndValidate` returned at the final writer generation (commit 1caea26^): the fifteen session-metadata table counts, and each other store's fixed evidence keys. A completed row must now carry exactly that key set — a missing, extra, or renamed key fails closed and preserves the journal. The contract is pinned to 1caea26^; a cutover written by an earlier writer whose key set differed fails closed (preserved, startup still blocked), which is non-regressive versus today and safer than dropping unrecognized evidence. Also reject a completed row whose `completed_at` precedes its `started_at` (CodeRabbit): both pass the integer checks yet the row is internally inconsistent evidence, not a row a released writer produced. Tests: the happy-path fixture now carries the full session-metadata contract; new fail-closed cases cover an extra key, a missing key, and a reversed timestamp, each asserting the journal survives rollback. Neutralizing either new guard turns the matching case red. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review follow-upPushed Addressed findingsAstro-Han [P2]: validation evidence was not bound to the released store contract. The previous The CodeRabbit: completion timestamp ordering. Completed rows now also require Verification
The full storage suite remains subject to CI because unrelated tests require the unavailable Darwin Review verdictThe previously identified findings are addressed. The change remains fail-closed for unfamiliar, incomplete, malformed, or internally inconsistent migration evidence. No new P0/P1 issue was found in the follow-up pass. 中文摘要已推送
复审结论：之前发现的问题已处理，未发现新的 P0/P1 问题。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The exact per-store validation-key map now matches the released writers, and the drop remains inside the existing atomic migration transaction. One compatibility blocker remains: the new timestamp ordering rule rejects state that the released writer could legitimately produce.
Several CI jobs are still in progress, so this head is not merge-ready independently of the finding.
AI-assisted review disclosure: Codex reviewed exact head 62ca27f, the released writer provenance, current CI, and thread state, with two independent reviewer passes.
中文说明
当前按 store 精确匹配 validation keys 的方向正确，且删除旧表仍处于原有原子迁移事务内。但新增的时间先后约束超出了已发布 writer 的契约：系统时钟回拨可产生合法的 completed_at < started_at，当前实现会让 workspace 每次启动都被迁移阻塞。另有数个 CI job 仍在运行。
Scope follow-upI reviewed the latest feedback and narrowed the patch back to the PR's actual contract. The exact per-store I removed the added The corresponding regression test was removed as well. The focused suite is now 36 pass / 0 fail / 1 skipped, with Biome, storage build, and The remaining CodeRabbit suggestion to prefer plain This should be the final scope-limited follow-up: the original evidence-contract blocker is addressed, while the unrelated clock-ordering behavior is not imposed. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 92f94d98858c791a257ba2b2f6bf6414b108f0f5.
The storage change is technically coherent: retirement requires the full released legacy signature, preserves fail-closed and rollback paths, and keeps the current schema/store authority intact. Focused Storage tests and current CI pass, there are no unresolved threads, and this does not affect UI/UX.
One provenance gate remains before approval. The PR body does not provide a complete AI-use declaration naming the tools and their scope. Of five substantive commits, only 4af7a288 has a Generated-by: trailer; 6f27a197, 381d4318, and 62ca27fa identify Claude as a co-author but have no matching generation trailer, and 92f94d988 also has none.
Please add the PR-template AI-use declaration and tool/scope, then add the appropriate Generated-by: <tool> trailer to every materially AI-authored commit (or explicitly clarify any human-only commit) and preserve the trailers in the final squash. No code P0-P2 findings remain after that provenance correction.
AI-assisted review disclosure: Codex reviewed the exact-head storage lifecycle, schema signatures, rollback paths, focused tests, live CI, threads, and provenance metadata. No external model was used. Astro-Han authorized this review campaign.
中文说明
代码层面已经闭环：legacy signature、fail-closed、rollback 和当前 schema/store 权威都保持正确；Storage 测试及 CI 全绿，无未解决线程，也不涉及 UI/UX。
批准前还缺 AI provenance：PR body 需要按模板明确工具和范围；5 个实质 commit 只有 1 个有 Generated-by:，其中 3 个还标注了 Claude co-author，但没有对应 generation trailer。请为实质 AI-authored commits 补 trailer，或明确哪些 commit 完全由人工完成。补齐后没有剩余代码 P0-P2。
Allow released workspaces to converge after SQLite became the sole operational authority by validating and retiring the obsolete cutover and import-source tables in the existing atomic migration. Interrupted or unfamiliar cutover state remains fail-closed and unchanged. Generated-by: Codex
…l-closed paths
Address self-review findings on the cutover-journal retirement:
- Reword the retirement doc comment so it no longer claims "exact released
table shapes": validation is column-level (name/type/notNull/pk) and does
not inspect CHECK/FK constraints or indexes. Row-level validation is the
authoritative content gate.
- Reject an empty validation_json object as evidence (previously `{}` passed
because `[].every()` is vacuously true).
- Add fail-closed regression tests that were previously missing: unfamiliar
column shape, malformed import-source row, and an orphaned session import
source. Each asserts the migration is blocked and the legacy table survives.
- Strengthen the happy-path retirement test to assert real data and schema
convergence survive (session row present, runtime/session_metadata versions
registered), not merely that the legacy tables disappeared.
operational-state suite: 31 pass / 0 fail / 1 skip (POSIX-only) on Node 22
with --experimental-sqlite; Biome and tsc clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Generated-by: Codex
…tadata The legacy-metadata retirement gate only compared column name/type/notNull/pk, so a `cutover_journal` / import-source table that shared the released columns but carried an altered CHECK/FK, or an extra index/trigger, was accepted as an "exact released shape" and dropped. Likewise a completed journal row naming any non-empty store passed the row gate, so evidence for a store no released writer ever emitted was treated as valid and retired. Both widen a destructive, fail-closed operation to inputs it cannot actually recognize. Replace the column-only check with a full `sqlite_schema` signature gate reusing the normalized-DDL seam in operational-target-schema.ts: a legacy table is retired only when every object attached to it matches the released layout down to constraints, with no extra index/trigger. Whitelist the exact `store_name` set the released writers emitted (session_metadata plus every completeOperationalStoreCutover caller) and fail closed on any other. Not tightening `completed_at`/`started_at`: the released writer used independent Date.now() calls and only guaranteed non-negativity. Add regression tests for altered-constraint, extra-trigger, and unknown-store journals — each asserts the migration fails closed and the legacy state is preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Generated-by: Codex
Astro-Han [P2] on apache#3227: the store_name allowlist closed the unknown-store path, but the evidence contract was still open — a completed `session_metadata` row with `validation_json = {"anything":0}` passed and the journal was dropped, though no released writer ever emitted that key. Missing or extra keys on any known store were accepted the same way, so the retirement still deleted migration evidence this build never wrote. Replace the store-name set with a map from each released `store_name` to the exact validation-key set its `importAndValidate` returned at the final writer generation (commit 1caea26^): the fifteen session-metadata table counts, and each other store's fixed evidence keys. A completed row must now carry exactly that key set — a missing, extra, or renamed key fails closed and preserves the journal. The contract is pinned to 1caea26^; a cutover written by an earlier writer whose key set differed fails closed (preserved, startup still blocked), which is non-regressive versus today and safer than dropping unrecognized evidence. Also reject a completed row whose `completed_at` precedes its `started_at` (CodeRabbit): both pass the integer checks yet the row is internally inconsistent evidence, not a row a released writer produced. Tests: the happy-path fixture now carries the full session-metadata contract; new fail-closed cases cover an extra key, a missing key, and a reversed timestamp, each asserting the journal survives rollback. Neutralizing either new guard turns the matching case red. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Generated-by: Codex
Generated-by: Codex
92f94d9 to
6edf57b
Compare
Provenance gate addressedThe latest review identified a repository-compliance blocker, not a new code defect: the PR body lacked the required AI-use declaration and the materially AI-authored commits did not all carry the required trailer. This is now addressed without expanding the code scope:
The rewritten commit chain was force-pushed with |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved exact head 6edf57b83d4693719737da3b256a63f17f2b7094.
The prior provenance blocker is fully resolved: the PR body now discloses the Codex and Claude Code scopes, all five material commits carry Generated-by: Codex, and the retained Claude co-author trailers match the disclosure. The storage-only diff remains sound, all review threads are resolved, and I found no P0–P3 issue. There is no UI/UX change, so no screenshot is required.
This approval is conditional on the remaining queued required checks finishing green before merge.
AI-assisted review disclosure: OpenAI Codex performed the exact-head code, provenance, review-thread, and CI analysis; I verified the commit trailers, disclosure scope, and live GitHub state before approving.
中文说明
此前的 AI provenance 缺口已完整修复：body 说明清楚，5 个实质性提交都有 Generated-by: Codex，Claude co-author 信息也一致。代码没有 P0–P3，线程已全部解决，无 UI 变化。剩余 required checks 全绿后才算 merge-ready。
2 participants
Summary
cutover_journal,runtime_import_sources, andsession_metadata_import_sourcestables during the existing atomic operational-state migrationRoot cause
Released workspaces can still contain migration bookkeeping created before SQLite became the sole operational authority. The old readers and writers were removed, but their tables were not retired. Current schema convergence therefore upgrades the real application tables, then rejects the otherwise valid database because strict target-schema validation sees
cutover_journal(followed by the two import-source tables) as unexpected objects.This presents as
OPERATIONAL_STATE_MIGRATION_BLOCKEDon startup even though SQLite integrity is healthy.Verification
npm run clean --workspace @maka/storagenpm run build --workspace @maka/storagegit diff --checkPRAGMA quick_checkreturnedokThe full storage suite was also attempted locally, but unrelated tests that load
fs-native-extensionscannot start because this checkout lacks its Darwin prebuilt addon. The focused suite and real-database-copy replay do not load that optional native path; CI remains authoritative for the complete package suite.The real workspace database was never modified; all migration replay used temporary copies.
AI use
Select exactly one:
Tool(s) and scope: Codex and Claude Code contributed to the implementation, regression tests, historical writer-contract analysis, and review-response changes. The human contributor reviewed the final diff, owns the submission, and owns the merge decision.
Checklist
git diff --checkpassesGenerated-by: Codex