fix(monitor): unify typed observation state and generation - #4149
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
动机
本次复审绑定 exact head cbfd22541b765e5af78878368669971874b59fc3,比较基线 faa4aad6f797a164b9bc23dacd68a6c36bed2f24。PR 的目标是把 Todo monitor metadata 的归一化、轮询状态转换和 generation 推导收敛到 TypeScript 所有权边界,避免 Python 与 TypeScript 各自维护一套规则;同时修复 grouped issue-fix monitor 不推进 generation、过期观测排序依赖 effect id,以及权限检查晚于字段诊断等问题。这个方向正确,真实调用链也已经接入,而不只是新增未使用模块。
但 refactor 宣称保持 Python 输入兼容时,timestamp codec 扩大了合法输入集合:基线 Python 会先全局把 Z/z 替换为 +00:00,因此 1970-01-01Z00:00 和 1970-01-01z00:00 会失败;新 TypeScript 以任意单字符作为日期与时间分隔符,反而接受并归一为 epoch。真实 update_goal_todo writer 已证明 head 会持久化该值,而基线明确报 --expires-at must be an ISO timestamp。这是未披露的输入语义漂移,必须修复后再审。
改动思路
正向架构为:update_goal_todo 在持锁快照上先执行 authority,再把 raw monitor intent 与 effective role/task class 交给 apply_todo_update_to_lines;field planner 调用 planMonitorMetadata,由 poll 统一判断 replay、stale observation、generation 和 no-change counter,最后 Python 只负责 durable write/receipt。grouped issue-fix materializer 也改为提交 MonitorPollObservation,不再在调用者侧预计算持久化 counter。该所有权收敛复用了现有 TypeScript field planner 和 Python writer,没有引入第二套 capability 或 provider。
负向路径中,旧观测在 typed poll 边界拒绝;replay 绑定不同字段时拒绝;权限错误先于 malformed monitor metadata 返回。问题只在兼容 codec:parseTodoTimestampMicros 的首个正则没有区分正常分隔符与本应只允许出现在末尾 timezone 的 Z/z。最小修复不是回退整个迁移,而是捕获 separator,拒绝 separator 位置的 Z/z,同时保留时间尾部 Z/z;并通过真实 public writer 增加基线/head 回归。
具体改动
完整 diff 为 21 个文件、+687/-404,主体是 monitor 规则迁移、调用者适配、TypeScript/Python 测试与迁移文档。生产热点集中在 monitor_metadata.ts、field_update.ts、todos.py 与 grouped materializer;Python adapter 明显缩小,规则所有权总体更清晰。
关键代码讲解
loopx/control_plane/runtime_timestamp.ts:44的parseTodoTimestampMicros承担旧 Pythondatetime.fromisoformat输入兼容。第 45 行的(?:[\s\S](.+))?会把Z/z当日期时间分隔符,这是本次阻塞点;末尾 timezone 的Z/z应继续合法。loopx/control_plane/todos/monitor_metadata.ts:80的poll在锁定快照语义下统一处理 target、cadence、effect replay、观测排序、generation 与 no-change counter,避免调用者各自推导。loopx/control_plane/todos/monitor_metadata.ts:150的planMonitorMetadata同时覆盖 raw metadata 与 observation,但禁止两者混用,并在 role/task class/boundedness 边界做验证。loopx/control_plane/todos/field_update.ts:193将 monitor plan 组合进既有 Todo field planner;第 197 行保留内部intent.monitor_metadatafallback,当前 production public writer 总是传monitor_context,因此这是有限兼容 seam,而非第二个活跃决策所有者。loopx/todos.py:1222与:1373先在 Python writer 完成 authority/锁/持久化责任,再把 monitor intent 交给 TypeScript;issue_fix/pr_monitor_materialization.py:176真实调用改传 observation,generation 由统一 owner 推导。
对主干的风险
[P1] 未披露的 timestamp accepted-input 漂移。 触发输入为 monitor metadata 中 expires_at=1970-01-01Z00:00(小写同理);新 regex 接受任意分隔符,真实 writer 会成功持久化,基线拒绝。差分矩阵覆盖 8,580 组日期/时间/时区组合,发现 1,080 个差异,全部收敛到 Z/z 作为日期时间分隔符;另 198,990 个 week-date 组合无差异。影响所有走 Todo monitor timestamp codec 的调用者,并会让此前非法状态进入 registry。最小修复是约束 separator,而非增加另一套 parser;回归必须从 public writer 证明拒绝、无写入,同时保留 ...T00:00Z 合法。
此外 SonarCloud 当前质量门禁仍为 ERROR,唯一未解决项是 runtime_timestamp.ts:67 的 TypeScript S5842:(:?)? 可匹配空字符串且外层可选冗余。可直接简化为 (:?) 并重新运行门禁。这项本身风险低,但远端门禁仍需变绿。
验证方面,control-plane TypeScript 共 915 项:914 passed、1 skipped、0 failed;相关 Python 四个测试文件共 116 passed;typecheck、Ruff/diff 检查以及当前主干 merge-tree 均通过。第一次用系统 Python 3.9 运行的一项环境兼容测试失败,改用项目支持的 Python 3.13 后全套通过,因此不计为产品失败。没有 optional/default-off 或新的 actor authority 语义;error text 保持 Todo/monitor 域中性,typed transition 明确 machine-enforced 规则。
我的整体评价
迁移的架构方向、活跃 caller、状态所有权和代码规模都合理,generation 与 authority 顺序修复也有真实路径覆盖;未来向整理应继续删除无活跃 caller 后的 legacy field fallback,而不是扩展新框架。但 refactor 的核心验收条件是旧入口输入、诊断、持久化结果保持兼容,绿色新实现测试不能覆盖从实现本身遗漏的 oracle。当前 exact head 存在可复现的 accepted-input 漂移且 Sonar gate 未绿,因此结论为 REQUEST_CHANGES,不合并。修正 separator、补 public writer 基线/head 回归并让质量门禁通过后可快速复审。
English verdict: REQUEST_CHANGES at exact head cbfd22541b765e5af78878368669971874b59fc3. The TypeScript migration is correctly wired into the real Todo writer and consolidates monitor-state authority, but its compatibility timestamp codec newly accepts 1970-01-01Z00:00/lowercase z, which the baseline Python writer rejects. Constrain the date-time separator while preserving terminal timezone Z/z, add a real writer regression proving no persistence, simplify the Sonar-reported empty-match regex, and rerun the gate. No merge performed.
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
cbfd225 to
af151c9
Compare
|
Addressed the review at
Final integrated-head qualification:
Architecture assessment: the review correctly identified a compatibility-oracle gap, not a reason to abandon the typed owner. Keep Monitor transitions in the existing field-plan boundary and Python as the lock/persistence adapter; do not add a second parser or authority layer. This companion fix narrows only the unintended input expansion. The PR's deliberate stale-observation, generation, boundedness-policy preservation and counter validation changes remain explicitly documented. Lease authority, provider defaults, promotion holds and successor atomicity are unchanged. No live Goal state was used or mutated in validation. The actionable review findings are fixed. Proceeding with maintainer-authorized admin self-merge based on this risk-based real-path qualification, with the pending hosted/Sonar status disclosed rather than presented as green. Temporary local Git-gate bypass remains command-scoped; no permanent policy weakening. |
Summary
Unify Monitor metadata and observation state rules in the existing typed Todo field-plan boundary. Fix two real gaps: unkeyed stale observations could overwrite newer state, and issue-fix grouped-monitor changes did not advance the generation consumed by
monitor_changedwaiters.The product diff is +334 / -397 (net -63 lines). Most additions outside product code are regression tests and bilingual migration documentation.
Issue Or Task
Maintainer-requested next cohesive stage of the TypeScript control-plane and shared Goal Authority RFCs. This is a Monitor state-owner prerequisite for T1/T2, not full native update or Monitor/successor transaction closure.
Behavior changes and preserved boundaries
MonitorPollObservation, deriving generation under the existing writer lock. A changed material observation now wakes matchingmonitor_changedconditions.material_change_generation/consecutive_no_changevalues.Ownership and removed duplication
todos/monitor_metadata.tsowns metadata normalization, boundedness, schedule composition, observation ordering, replay, generation and no-change counters.field_update.tscomposes it in-process.Retained seam: the public Python writer still owns locking, admission and persistence. Retiring that writer requires native field/effect transaction closure and retirement of its remaining callers. Permanent Markdown rendering stays; provider routing, promotion qualification and Monitor-plus-successor atomicity are not changed here.
Validation
cbfd22541b765e5af78878368669971874b59fc3finishedsynthetic,public_fixturenpm run test:control-plane: 944 passed, zero failed/skipped.faa4aad6f797a164b9bc23dacd68a6c36bed2f24versus candidate: five legal observation scenarios plus exact retries produced identical complete persisted-state and transition fingerprints.Coverage and gaps: covers the changed public writer, state rules, capability caller, retry/error fences, timestamp compatibility, packaged sources and real provider backends. Initial environment-only attempts using Python 3.9 / unavailable build tooling were replaced with Python 3.13 and successful isolated builds; no unresolved local test failure remains. Hosted CI is pending. No live Goal, registry, writer fence or lease was modified. No full native Monitor/successor atomicity or system-wide latency qualification is claimed.
Type of Change
Includes a bounded refactor with the deliberate behavior changes disclosed above; it is not described as zero-behavior-change migration.
LoopX Area
Technical Direction
mainShared-authority RFC fixture impact
production_scale_coordination_fixture.ts; 464 Todos / 64 leases.Boundary Checklist
Future-facing pass applied: remove duplicate Monitor rules, reuse the typed field boundary and existing scheduler reducer, and retain only adapters with actual callers. No speculative transaction framework or new provider is added. This PR requests review; self-merge and local product installation are not part of this delivery.