Refactor shared authority transaction semantics - #4280
Conversation
Signed-off-by: huangruiteng <huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
Reviewed PR #4280 at exact head 0acc8c1b8ae2b995dc67435ace0a994c18393755.
动机
这个 PR 识别出了 file 与 NoKV authority provider 中重复的 transaction 解码、clone 和 revision projection 逻辑。把这三类语义收拢到一个 TypeScript semantic core 是合理的,且能降低两个 provider 发生语义漂移的风险。
改动思路
authority_store_transactions.ts 现在负责严格的顶层 key 校验、JSON canonicalization、显式 clone,以及去掉 provider_revision 后供 revision 计算的逻辑交易形状;file/NoKV provider 保留各自的 envelope、salt 和持久化细节。这条边界基本正确,也没有看到新增 authority writer 或改变默认 authority 的行为。
但 PR 描述和 RFC 都承诺了更完整的 malformed-record/complex fixture 验证,而实际提交只有一个 10 行的直接单元测试;同时新增 RFC 没有加入 RFC README,中文 RFC 的主体段落仍是英文,且与已有 shared-goal-authority-state-provider / TypeScript migration RFC 存在明显的文档 ownership 重叠。这个阶段更适合把语义核心作为现有 RFC 的阶段性落点,而不是再生成一个未索引、未完成双语化的第三份 RFC。
具体改动
关键代码讲解
- 新增
decodeAuthorityTransaction:要求精确的顶层字段集合,并校验身份字段与列表字段。 - 新增
cloneAuthorityTransaction:统一使用结构化 clone,避免 provider 间共享可变对象。 - 新增
transactionForRevision:明确排除 storage/provider metadata 后再参与 revision 计算。 - file 与 NoKV 删除各自的同名本地 helper,改为调用共享模块;PostgreSQL storage-row decoder 保持未迁移,并在 RFC 中作为后续阶段。
对主干的风险
- 阻塞:验证承诺与提交不一致。 请补上可复用的 public complex fixture/数据驱动矩阵,并至少覆盖 file/NoKV parity、reordered keys、未知顶层 key、非字符串 identity、嵌套 malformed values、clone isolation、revision projection 和合法 legacy-compatible records;不要只保留当前两个断言。
- 阻塞:RFC 交付不完整。 请将新增语义整合进已有 shared-authority / TypeScript RFC,或至少把新 RFC 加入
docs/architecture/rfcs/README.md,并提供真正的中英文语义镜像。当前中文文件大部分正文仍是英文,无法作为双语 RFC 交付。 - 阻塞:远端必需检查未绿。 GitHub run
34683393154的windows-powershell失败:121 passed、1 skipped、1 failed,失败在tests/test_windows_install.py::test_windows_installer_promotes_release_and_runs_doctor的quota should-run子进程返回码。请用 base/head 对照或修复后重新跑绿,并在 PR 中给出归因;当前 merge-gate 也因此失败。
本地针对 exact head 的证据:npm run typecheck:control-plane 通过;完整 npm run test:control-plane 为 1214 passed、1 skipped、0 failed;focused transaction test 1/1 通过;只读 loopx --format json status --goal-id loopx-meta 返回 ok=true、run_count=25467、errors=0(保留 1 个已有 duplicate-index warning)。这些结果支持当前代码路径的基本正确性,但不能替代上述 fixture、RFC 和远端 Windows 证据。
我的整体评价
代码抽取本身是一个合理且边界清楚的 Stage A,暂未发现 valid-record path 的明显语义回归;但按 PR 自己声明的目标,文档整合、复杂 fixture 和远端验证尚未闭环,因此当前不建议合并。修复上述三项后再复审。
English verdict: REQUEST_CHANGES. The shared semantic-core direction is sound, but the promised fixture/RFC integration is incomplete and the required Windows check is failing.
|
Request changes conclusion (author-owned PR; GitHub blocks formal self-review) Reviewed PR #4280 at exact head 动机这个 PR 识别出了 file 与 NoKV authority provider 中重复的 transaction 解码、clone 和 revision projection 逻辑。把这三类语义收拢到一个 TypeScript semantic core 是合理的,且能降低两个 provider 发生语义漂移的风险。 改动思路
但 PR 描述和 RFC 都承诺了更完整的 malformed-record/complex fixture 验证,而实际提交只有一个 10 行的直接单元测试;同时新增 RFC 没有加入 RFC README,中文 RFC 的主体段落仍是英文,且与已有 shared-goal-authority-state-provider / TypeScript migration RFC 存在明显的文档 ownership 重叠。这个阶段更适合把语义核心作为现有 RFC 的阶段性落点,而不是再生成一个未索引、未完成双语化的第三份 RFC。 具体改动关键代码讲解
对主干的风险
本地针对 exact head 的证据: 我的整体评价代码抽取本身是一个合理且边界清楚的 Stage A,暂未发现 valid-record path 的明显语义回归;但按 PR 自己声明的目标,文档整合、复杂 fixture 和远端验证尚未闭环,因此当前不建议合并。修复上述三项后再复审。 English verdict: REQUEST_CHANGES. The shared semantic-core direction is sound, but the promised fixture/RFC integration is incomplete and the required Windows check is failing. |
Signed-off-by: huangruiteng <huangruiteng@users.noreply.github.com>
|
Follow-up on the requested changes at head
The runtime extraction is now review-complete for the file/NoKV Stage 1 scope. SQLite/PostgreSQL migration and the unrelated Windows baseline remain explicitly outside this PR. |
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
动机
本次评审针对 exact head a34fa8edd198632de2e3c0d73a9726c27994d058,基线为 main 的 787e8dd830887df0114793c59e88bd01d6b6d73d。PR 要解决 file 与 NoKV authority adapter 重复实现 committed transaction 解码、clone 隔离和 revision 投影的问题。重复代码不仅增加维护成本,还会让“未知 key 是否拒绝”“嵌套 JSON 是否 canonicalize”“返回对象是否可被调用方修改”等协议细节在两个 provider 上悄悄分叉。因此,把 provider-neutral 的 transaction 语义集中到一个小模块,而把 envelope、generation、identity 和 provider salt 留在各自 adapter,是合理且右尺寸的重构目标。
改动思路
新的 authority_store_transactions.ts 成为共享的纯语义边界:先验证输入是 plain JSON object,再要求 committed transaction 的顶层 key 集合精确匹配,随后 canonicalize events、projection 和 receipts,并提供显式 structuredClone 与 logical transactionForRevision。File 和 NoKV 仍各自验证 schema、goal/tenant、store identity、cursor/generation、历史 operation identity 以及 provider-specific revision hash;共享模块没有新增 writer、lease、promotion 或外部权限。正向路径是同一份合法记录由两个 provider 解码成等价逻辑 transaction;负向路径是 unknown key、错误 list、坏 nested record 或非字符串 operation identity 在 provider 边界 fail closed,不能被某个 adapter 宽松接受。
具体改动
exact head 共 25 个文件,新增 1621 行、删除 373 行,包含共享语义模块、file/NoKV adapter 的去重、296 行跨 provider parity fixture,以及中英文 RFC 的 Stage 1 说明。测试还明确声明 SQLite/PostgreSQL 仍属后续 stage,没有把 candidate provider 误报为 runtime promotion。
关键代码讲解
decodeAuthorityTransaction的 exact-key 检查和canonicalAuthorityObject*调用统一了两个 provider 的 wire contract;它不会因为字段顺序变化而拒绝合法历史记录,也不会接受额外顶层 metadata。cloneAuthorityTransaction使用structuredClone,使scanCommitted返回的 projection/events/receipts 与 authority store 内部状态隔离,调用方修改返回值不会污染下一次读取。transactionForRevision只保留 cursor、operation_id、events、projection、receipts;file/NoKV 仍把各自 identity、generation 和前一 revision 加入 provider hash,因此逻辑内容与物理 envelope 的职责没有混在一起。FileAuthorityStore.decodeDocument与NoKVAuthorityStore.decodeDocument继续在共享解码之后检查各自 lineage、head 和 generation,这个分层保留了 provider-specific failure ownership。
对主干的风险
我在 exact head 上运行共享 authority transaction 测试,12 个 case 全部通过:native 与 reordered legacy-compatible 记录被接受;unknown top-level key、malformed receipts、malformed nested event 和非字符串 operation identity 被拒绝;file/NoKV 两条读取路径的结果和 provider revision 投影一致;scan 返回值及 clone 也都能证明隔离。packet 中 control-plane suite/typecheck 及其余 required checks 也通过,说明目前没有从这次去重代码本身发现新的 authority 或默认行为扩张。
但 exact head 的 required windows-powershell 和 merge-gate 仍失败。Windows 日志显示 installer/quota status 测试走到 skip 后以 exit 1 结束;PR body 将其与 main 对比并标为 baseline,这是很有价值的诊断,却不能把 required check 变成可合入证据。负向风险是:在门禁仍红时,维护者无法区分“共享 transaction 重构安全”与“发布/安装路径已有回归”,而 merge-gate 也会继续阻止后续变更。最小修复是先在当前 main 重现并按仓库规则修复或正式处理该 Windows status contract,再在这个 exact head 重跑 Windows 与 merge-gate;如果确认是基线问题,也应让 gate 以仓库认可的方式记录并通过,而不是只写在 PR 描述里。
我的整体评价
共享语义核心本身是一个清晰、可复用且没有扩大 authority 的边界;file/NoKV parity fixture 也比各自复制测试更能防止未来漂移。当前阻塞不是我凭空推测的代码缺陷,而是 required platform/integration evidence 尚未闭合,且 merge-gate 仍明确失败。因此我给出 REQUEST_CHANGES,不是批准。请解决或正式闭合 Windows/merge-gate,并在不改变 exact transaction contract 的前提下重跑 required checks;下一轮我会复查 provider lineage、malformed-record fail-closed 以及 clone isolation 是否仍保持一致。
English verdict: REQUEST_CHANGES on exact head a34fa8e; shared transaction semantics pass focused and control-plane tests, but required windows-powershell and merge-gate remain unresolved.
Problem
File and NoKV authority providers duplicated transaction decoding, cloning, and revision-input shaping, allowing semantic drift between adapters.
Change
authority_store_transactions.ts.Semantic boundaries
Valid records retain their existing behavior. Malformed records now fail closed with the same shared rules across file and NoKV. No claim, lease, archive, promotion, or new authority writer is introduced. SQLite and PostgreSQL row/envelope migration remain later provider stages.
Validation
npm run typecheck:control-planenode --no-warnings --experimental-sqlite --experimental-strip-types --test tests/control_plane_ts/authority_store_transactions.test.ts— 12 passednpm run test:control-plane— 1225 passed, 1 skipped, 0 failedloopx --format json status --goal-id loopx-meta—ok=true, contract errors0, existing duplicate-index/artifact-collision warning1git diff --checkloopx canary premerge --from-git-diff --goal-id loopx-meta— 13 selected checks passed, 0 failuresfe0c2a383a0eb7089414190d4d3f517f89578f80b086a1e99f8aba6296880e26The required Windows lifecycle check is independently failing on the current
mainbaseline as well as this head: base workflow run34681511188and PR run34683393154both failtest_windows_installer_promotes_release_and_runs_doctoratquota should-runreturning exit code 1. This PR does not touch the installer, quota, or Windows paths; the failure is recorded as a baseline CI hold rather than masked or changed here.Commits
0acc8c1b8— original shared authority semantic-core extractiona34fa8edd— existing-RFC integration and file/NoKV parity fixture