Skip to content

fix(authority): keep legacy JSON keys readable across the sqlite migration - #4497

Merged
huangruiteng merged 1 commit into
mainfrom
codex/authority-state-legacy-json-keys
Sep 16, 2026
Merged

huangruiteng merged 1 commit into
mainfrom
codex/authority-state-legacy-json-keys

Conversation

@huangruiteng

Copy link
Copy Markdown
Collaborator

Summary

#4408 replaced the retained full projection per commit with one checkpoint plus
one exact delta per commit, and added an explicit V1 to V2 migration. A model
review of that change found a migration gap on a real SQLite database:

retained V1 projection V1 read/write migration V2 read
ordinary JSON fields ok ok ok
contains an empty-string key "" ok reports migrated live head and history read fail
contains a __proto__ key ok reports migrated history read fails

Both keys were legal V1 data, because V1 stored the whole projection as JSON.
The migration is supposed to keep such a database readable, so it must not
adopt a state log the new provider cannot read.

Root causes

  1. The delta codec did not keep V1's JSON key semantics. The path segment
    decoder rejected a zero-length segment, while the encoder legitimately emits
    one for a "" key, so a migrated row decoded into
    authority state delta operation 0 path segment is invalid. Reconstruction
    wrote values with container[key] = value, which for __proto__ runs the
    inherited accessor and replaces the object's prototype instead of storing a
    key, so the replayed state silently lost it. Whole-object keys are now
    created as own data properties, the same way the canonicalizer creates them,
    and any string is accepted as a path segment.
  2. The migration never proved that its own output was readable. It proved
    the encoder in memory, then verified the post-commit store only by copied
    identity columns and digests, which cannot show that a state log decodes.
    The migration now replays the deltas it wrote -- read back from
    commits_v2/checkpoints_v2/head_v2 inside the same transaction, decoded
    with the store's own decoder -- and fails closed before the swap if any
    projection, checkpoint or head does not reconstruct to its published digest.
    This is what docs/reference/sqlite-authority-store.md already claimed.

Evidence

New tests were written first and reproduced the reported failures on the
unmodified head (migration status: migrated, then
provider_protocol_violation: authority state delta operation 0 path segment is invalid on the first read), then pass with the fix.

  • node --test tests/control_plane_ts/authority_state_log.test.ts tests/control_plane_ts/sqlite_authority_migration.test.ts tests/control_plane_ts/sqlite_authority_store.test.ts on the qualified runtime (Node 22.22.3 / SQLite 3.51.3) -> 123 passed, 0 failed (clean origin/main baseline: 121 passed, 0 failed)
  • npx tsc --project tsconfig.control-plane.json --noEmit -> clean
  • npm run test:control-plane -> 1617 passed / 14 failed, with the identical 14 pre-existing failures on a clean origin/main baseline (entry-identity, transport-alias, successor-fingerprint and --from-context CLI tests); the 2 extra passes are the new tests
  • pytest -q tests/control_plane/test_sqlite_authority_cli.py with the qualified runtime on PATH -> 5 passed

The new migration test seeds V1 rows whose retained projections carry "" and
__proto__ (including one nested __proto__ and one commit that removes them),
asserts the frozen V1 rows really contain those keys, then requires the live
head and every retained projection to read back byte-identically with
__proto__ present as data and the prototype unchanged.

Risk

Low. The codec change only widens what decodes (any string key) and makes
reconstruction write own properties, so no previously readable state changes
meaning. The added migration work is one extra pass over rows the migration
already reads, inside the existing transaction, and it fails closed. The
version-2 write path already had the equivalent in-memory proof, so live commits
are unaffected. No schema, on-disk format, cursor, digest or CLI change.

中文摘要

修复 #4408 迁移缺口:delta 编解码现在保留 V1 接受的 JSON key 语义(空字符串
key、__proto__ 作为数据而非原型访问器),迁移在事务内、换表之前把刚写入的
commits_v2/checkpoints_v2/head_v2 读回来,用 store 自己的解码器重放并
比对每行发布的摘要,无法还原即失败回滚。新测试先复现了报告中的失败再转绿;
合格 runtime 上 123 项通过,类型检查干净,整套 TS 测试与干净 origin/main
基线失败集合一致。

…ation

The version-2 delta codec rejected an empty path segment and rebuilt
`__proto__` through the inherited accessor, so a retained version-1 projection
using either key migrated "successfully" into a state log the new provider could
not read. The migration also proved only copied identity and digest columns.

Any string is now a legal path segment, decoded keys are written as own data
properties, and the migration replays the rows it just wrote through the store's
own decoder before swapping tables, so an unreadable log fails closed inside the
transaction instead of after it.

Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Self-review of the final head (2e136845b), re-read against origin/main.

What I re-checked

  • Whether the same two defects exist in sibling code: other container[key] = value reconstructions in loopx/control_plane/coordination/ are todo_successor_derivation.ts:209, todo_continuation.ts:113 and ownership_observation.ts:30, which all copy closed key sets, so __proto__ cannot enter them.
  • Whether the Python file-authority provider has an equivalent delta codec: it stores whole canonical projections, so the defect is specific to the retained state log added by #4408.
  • Whether the codec change widens anything a previous provider accepted: authority_store_codec.ts already created "" and __proto__ as own data properties through Object.fromEntries/Object.keys, so the previous decoder was rejecting paths its own encoder emits. The change restores that symmetry; no state that decoded before changes meaning.

Findings

[P3, follow-up kept out of this PR] loopx/control_plane/coordination/todo_monitor_poll.ts:98 copies monitor metadata with for (const [key, value] of Object.entries(metadata)) updated[key] = value. metadata is an arbitrary authored object, so the same [[Set]] hazard applies: a __proto__ key holding an object would replace the rebuilt Todo's prototype and make the downstream canonicalizer refuse the poll ("JSON objects must be plain objects"), while a primitive value would be dropped silently. It is a different owner with a different failure mode (no digest reconstruction), so I did not widen this PR to it. It needs the same Object.defineProperty treatment plus its own fixture if the owner wants the generic-metadata passthrough to be key-complete.

[P3, cost note] The migration now makes one extra bounded pass over retained deltas (512 rows per page) while it still holds the BEGIN IMMEDIATE transaction. It is the same order as the pass that writes the log, it is bounded per page, and it is what makes a failed cutover impossible instead of merely unlikely.

[P3, alternative rejected] Reading the migrated store back through SqliteAuthorityStore instead of replaying commits_v2 in the transaction. A store-level read can only run after COMMIT, where a failure can no longer roll back the swap -- exactly the gap this PR closes. The in-transaction replay uses the store's own decodeAuthorityStateDelta/applyAuthorityStateDelta, then verifyMigratedStore still proves identity and lineage after the commit.

Future-facing refactor pass

Applied inside the touched boundary: the encoder proof was already a named contract on the live write path, and the migration now uses the same rule plus a stored-row read-back instead of inventing a second verification style. No new module, flag, or schema field was added; authority_state_log.ts keeps the codec authority and the migration keeps the storage authority.

Validation

  • Qualified runtime (Node 22.22.3 / SQLite 3.51.3): authority_state_log.test.ts + sqlite_authority_migration.test.ts + sqlite_authority_store.test.ts -> 123 passed / 0 failed; clean origin/main baseline on the same files -> 121 passed / 0 failed (the delta is the two new tests)
  • Both new tests fail on the unmodified head first: the codec round trip throws authority state delta operation 0 path segment is invalid, and the migration reports migrated before the first V2 read fails with the same protocol violation
  • npx tsc --project tsconfig.control-plane.json --noEmit -> clean
  • npm run test:control-plane -> 1617 passed / 14 failed, with the identical 14 failures on a clean origin/main baseline (entry-identity, transport-alias, successor-fingerprint and --from-context CLI tests, all environment-local)
  • pytest -q tests/control_plane/test_sqlite_authority_cli.py with the qualified runtime on PATH -> 5 passed

No blockers found. The migration is idempotent, fails closed inside its transaction, and the new tests cover an empty-key, a nested __proto__, and a commit that removes both from the retained state.

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Approval conclusion (author-owned PR; GitHub blocks formal self-approval)

Exact head reviewed: 2e136845b1e024b7a127f14b2268e1bad9b603db

动机

SQLite 权威 store 的迁移承诺是"旧 provider 能读的库,迁移后仍然能读"。这条承诺此前没有真正被检查:v2 增量编解码器拒绝长度为 0 的路径段(而编码器为 "" 这个合法 JSON key 恰好会产出空段),并且用 container[key] = value 还原 __proto__,会走原型上的访问器、把重建对象的原型替换掉。结果是包含这两种 key 的 v1 保留投影会"迁移成功",却在下一次读取时报 provider_protocol_violation: authority state delta operation 0 path segment is invalid,或者静默丢掉 __proto__ 键。迁移本身只核对了搬过去的标识与摘要,无法发现新格式其实读不出来。受影响的是所有在依赖 v2 provider 之前做迁移的操作者;失败形态是"报告成功、状态随后不可读",对迁移来说是最糟的一种。只修编解码器不够——下一次编解码不对称仍会以同样方式溜过去,所以本 PR 同时补上"提交前用 store 自己的解码器重放刚写入的行"。

改动思路

入口是 executeSqliteAuthorityMigration(一次一个 Goal 库、原地迁移)。权威输入是冻结的 v1 行(完整投影加标识与摘要);决策所有者仍是 authority_state_log.ts 的 v2 编解码器(authorityStateDelta 编码、decodeAuthorityStateDelta 解码、applyAuthorityStateDelta 应用)。副作用被限制在 BEGIN IMMEDIATE 一个事务内:写 commits_v2/checkpoints_v2/head_v2,证明可读,再 DROP v1 表并更新 schema version;任何失败回滚并保持 v1 原样,第二次运行报 already_current。复用判断:迁移没有新增第二个解析器,而是调用 store 自己的解码器做重放证明——这正是"要删除重复权威、不要制造第二个权威"的用法。状态模型上没有新增持久字段或关系,只是让已有保留数据在新 schema 下可读,所以 state_model_assessment 记为 not_applicable。规则归属上,"保留投影允许哪些 JSON key"和"迁移如何证明可读"都落在既有 owner:key 语义在 codec,读回证明在 migration;被删除的是"空段非法"和 [[Set]] 写法,以及只核对标识的旧证明。

具体改动

loopx/control_plane/coordination/authority_state_log.ts:新增 setOwnJsonKey(:202),用 Object.defineProperty 写成自有可枚举数据属性,applyAuthorityStateDelta 不再直接赋值;decodeAuthorityStatePath(:252)接受任意字符串作为路径段,只拒绝非字符串,并把这条不对称的教训写进 AuthorityStatePath 的注释里。loopx/control_plane/coordination/sqlite_authority_migration.ts:在写入之前先做编码自证(:200,authorityStateDigest(applyAuthorityStateDelta(previous, delta)) === stateDigest),再把刚写入的 v2 表读回来重放(:268 verifyWrittenStateLogReadable),要求父子 lineage、游标连续性、摘要一致、checkpoint 覆盖其游标、head 与最后重放游标一致、保留事务数不少,然后才 swap。文档 docs/reference/sqlite-authority-store.md 中英文同步改写了迁移承诺与"只核对标识无法证明可读"的原因。测试先复现后修复:迁移测试在未修复代码上复现了报告的失败,再断言修复后的可读与 fail-closed 两条路径。

关键代码讲解

  • setOwnJsonKey(authority_state_log.ts:202):解码出来的 key 是数据,不是行为。只有写成自有数据属性,才和 canonicalizer 建键的方式一致,__proto__ 才不会变成"改原型"。
  • decodeAuthorityStatePath(authority_state_log.ts:252):关键不变量是"解码器不能拒绝编码器能产出的值"。空字符串是合法 JSON key,旧规则与编码器直接矛盾。
  • executeSqliteAuthorityMigration 的编码自证(sqlite_authority_migration.ts:200):对应一次真实 commit 的等价检查,保证不可读的投影在写任何东西之前就失败。
  • verifyWrittenStateLogReadable(sqlite_authority_migration.ts:268):读回真实落盘行、用 store 自己的解码器从空状态重放,再比对每个 payload 的摘要;这正是"只核对复制过去的标识与摘要"补不上的那一块。

对主干的风险

最强回归场景是:某个 v1 保留投影带上新 codec 处理不当的 key,迁移照样报成功,事后才发现读不出来。触发状态是 v1 库保留过含 "" 或 __proto__ 的投影;旧代码允许(解码器拒绝编码器产出的空段 + [[Set]] 原型写入),现在被自有属性写入与事务内重放证明共同挡住。爆炸半径是该 Goal 库的状态日志及其后续投影读取;可观测点是迁移报告与迁移后的第一次读取;回退/恢复是事务回滚,v1 原样保留,修好后重跑。我复核了最关键的反驳:仓库里原有 SQLite 相关测试全绿(报告里的 12 项)而这两个 key 仍然失败——这说明测试数量与完整叙述不能替代对契约本身的检查,正是本 PR 补上的部分。未覆盖维度:没有在真实生产权威 store 上做过迁移,证据是真实 SQLite(qualified Node 22.22.3 / SQLite 3.51.3)+ 仓库自带测试。重复规则方面,v1 与 v2 两条路径都映射到了同一 codec owner;guidance 与机器强制义务没有混淆——迁移是 fail-closed 的强制行为,不是建议。另有一个同形但本 PR 未触碰的兄弟风险见下方 P3。

我的整体评价

结论:APPROVE(author-owned PR,GitHub 不允许作者正式自审,故以 COMMENTED 形式记录同一结论)。 问题真实、可复现、修复面小且同域;observable_semantics = intentional_change_validated(放宽接受域以匹配 v1 已接受的 key 语义,并用重放证明兜底),repository_reuse = separation_justified(复用 store 自己的解码器,不新增第二解析器),change_proportionality = proportionate(一个 helper、一次校验放宽、一个有界重放函数),authority_semantics = aligned(不默认 v2 provider、不退役 v1、不放宽权限),default_off_isolation = not_applicable(无开关)。head 上重跑:node --test 三个 TS 文件 123 passed / 0 failed;pytest tests/control_plane/test_sqlite_authority_cli.py 5 passed;tsc --noEmit 干净;完整 control-plane 套件与未改动的基线保持同样 14 项环境相关失败。

残留风险:未在真实生产库上迁移过。唯一非阻断发现(P3):loopx/control_plane/coordination/todo_monitor_poll.ts:98 仍是 updated[key] = value 写法,任意监控元数据都会遇到同一个 [[Set]] 陷阱(__proto__ 对象值会让下游 canonicalizer 抛 "JSON objects must be plain objects",原始值则被静默丢弃)。它不在本 PR 的改动面内,所以按 P3 记录、不扩进这次迁移修复:下次触碰该文件时复用同一个自有属性写入 helper,并补一条 __proto__ 元数据的回归测试即可。复审只需要在该文件被改动时确认一次。

English verdict: APPROVE (recorded as a COMMENTED review because GitHub blocks formal self-approval on author-owned PRs). Exact head 2e136845b1e024b7a127f14b2268e1bad9b603db. The v2 delta codec rejected a zero-length path segment the encoder emits for a "" key and rebuilt __proto__ through the inherited accessor; the migration also proved only copied identity and digest columns. Any string is now a legal segment, decoded keys are written as own data properties, and the migration replays the rows it just wrote through the store's own decoder before swapping, so an unreadable log fails closed inside the transaction. Verified at the head: node --test on the three TypeScript files 123 pass / 0 fail (baseline 121), pytest tests/control_plane/test_sqlite_authority_cli.py 5 passed, tsc --noEmit clean, full control-plane suite unchanged at its 14 pre-existing environment-local failures. No blocking finding. One P3 sibling: coordination/todo_monitor_poll.ts:98 keeps the same [[Set]] hazard and should reuse the own-property helper when next touched. Residual risk: no live production store was migrated.

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.

1 participant