Repository navigation
Conversation
|
Hi @Dante-dan, the DCO If the log confirms a missing |
songoow
left a comment
There was a problem hiding this comment.
Reviewer: model_agent — OpenAI GPT-6 Astra Pro.
详细中文评审
审查版本:77aabe3297d1168d45651e92dc23a545f78c9a13;base:2b72d30fca7f452a71dffb723ebf6d1e2e878484。发布前已重新读取远端,head 未变化。本次覆盖全部 8 个变更文件,并结合 ledger、receipt、stats normalizer 和 DSH observer producer 做了三路独立核查。
结论:未发现本 PR 引入的、需要阻塞合并的正确性缺陷。 本次以 COMMENT 记录代码审查、非阻塞建议和验证边界;完整审查工作流及 merge readiness 尚未验证通过。
1. 动机
本 PR 对应 #5208 第二条 Acceptance及作者的认领说明:原有 Goal ledger 的 aggregate stage 无法向调用方证明它属于所期待的 session、observer 和运行配置。新增显式绑定、评价时间及年龄上限,解决的是手动读取时的证据接纳问题。
我按该边界评审:本次可交付 manual exact-read prerequisite;C0/C1、面板、自动轮询、runtime promotion、overhead 和 retention/deletion 仍属于父任务。没有将这些明确排除的后续工作列为当前 PR 的缺陷,也不把本次测试通过视为父任务完成。
2. 改动思路
实现继续沿用 ledger → LedgerReading → integrity receipt / diagnostic projection → CLI 输出,复用既有 normalize_observer_run_identity() 和 receipt 的完整性判断。新增 BindingIneligibility 表达本地读取拒绝原因,没有成为第二套 runtime-state owner。
发现冲突时拒绝整个绑定,而非过滤其他运行后声称当前健康;拒绝后 stage=unknown,counts/signals/integrity 保留历史证据。这与 RFC 新增读取合同一致。没有新增 provider 调用、调度输入、canonical state 写入或轮询路径。
3. 具体改动
- projection 输入校验:要求精确的四个顶层字段;复用八字段 treatment normalizer;绑定必须提供显式
as_of和非负整数年龄上限;拒绝 bool、缺失参数以及只有年龄上限没有绑定的组合。 - 绑定判定:检查所有 envelope 的 provider/observer/session,要求唯一 stats identity,逐字段比较完整 treatment,并要求 receipt 为
valid。空 ledger、额外 observer、不同 session/treatment 或非 valid 完整性均不可采用。 - 时间比较沿用精确的时间换算,覆盖 UTC offset 和小数秒边界。任一未来 envelope 都拒绝;最新观测年龄超过上限才拒绝,等于上限允许。
- CLI 将 JSON 解析及 normalizer 的 ValueError 转为退出码 2。不提供绑定时,新增判定不执行,旧 projection 形状及条件式文本输出保持兼容。
- 两个测试模块覆盖真实 CLI 正常/异常 readback、时间边界及文件字节保持;四份文档明确手动读取、历史证据和无控制权的边界。
额外核查了两个可能的漏洞假设:
- 同一 observer 的不同 treatment 不会被后写 stats 静默覆盖。既有
_store_stats_record()会将冲突记录计为无效,新绑定又要求 valid receipt。两种冲突记录顺序均验证为不可采用;正常同运行累计 stats 仍可采用。 - 八个 treatment 字段本身描述运行配置,但本次还固定 observer/session。DSH producer 默认为实例生成 UUID observer ID。没有发现当前调用链存在必须新增另一套 run ID 才能修复的混淆问题。
4. 对主干的风险
当前变更为 opt-in、只读且局限于既有 owner;审查和聚焦测试未发现默认行为回归。
非阻塞建议 A:统一既有 identity token 校验。
新增顶层身份使用 fullmatch(),而 intake._identity_token() 仍使用 IDENTITY_TOKEN_PATTERN.match(value)。正则结束锚点可匹配最终换行符之前的位置。我复现了 stats 与 expected binding 都包含 model_id="model-fixture\n" 时,binding.eligible=true 且 integrity.status=valid。
这是共享 normalizer 的既有问题,不是本 PR 引入的身份比较绕过;没有证据表明它能把不同身份当成相同身份。建议在共享 owner 中统一为 fullmatch() 并补尾部换行回归,以符合新增文档中非法 token 应被拒绝的约定。
非阻塞建议 B:补两个当前已正确工作的回归场景。
- 第二个 observer 只有 stats、accepted events 为 0:仍应拒绝歧义。
- 同一 observer/treatment 的多条累计 stats:应继续允许,避免以后把正常快照误判为多运行。
两者均已通过额外探针验证,建议固化为测试;它们不是当前缺陷。
5. 我的整体评价
实现方向、owner 边界和拒绝逻辑合理。当前证据支持“该手动读取功能未发现阻塞级新增缺陷”,建议保留现有方向,处理上述非阻塞改进并补齐最终验证记录。
本次独立执行:
| 验证 | 结果 |
|---|---|
python -m pytest -q tests/capabilities/test_reliability_diagnostics.py tests/capabilities/test_reliability_diagnostics_readback.py |
148 passed,43.38 秒 |
| 四个变更 Python 文件的 Ruff | 全部通过 |
| 三个顶层身份、八个 treatment 字段逐项 mismatch | 均拒绝 |
| 冲突 stats 的两种顺序、额外 stats-only observer | 均拒绝 |
| 同运行累计 stats | 保持可采用 |
| 真实 CLI readback、异常输入和 ledger 文件字节保持 | 相关测试通过 |
未完成/未验证的边界:
- 本次没有独立执行全仓 pytest、完整 premerge、live DSH/model、Windows 或发布资格验证。
- 发布前 PR 描述仍记录全量 pytest 未结束且已有 failures/errors,CLI output budget smoke / diff-selected premerge 待最终运行;这些记录没有被视为通过,也没有由本次审查独立完成失败归因。
- 最新机器人评论 报告 Sign-off check 未通过。具体原因仍需检查该 check 的日志,不能仅凭状态认定一定缺少 Signed-off-by trailer。
- 仓库
loopx-pr-review编排在审查环境因本地 TypeScript Effect runtime 权限限制未完成,未生成完整 packet / check-result 证明。该限制与上述源码审查和聚焦测试分别记录;本 COMMENT 不授予正式 approval 或 merge readiness。
English verdict
COMMENT — No blocking regression found in the bounded manual exact-read change at 77aabe3297d1168d45651e92dc23a545f78c9a13. The 148 focused tests and changed-source Ruff passed independently; additional identity and ambiguity probes behaved as intended. The trailing-newline token issue is pre-existing and non-blocking; two additional regression cases are recommended. Formal review-workflow validation, full-suite/premerge qualification and merge readiness remain unverified.
|
@songoow, could you address these two baseline failures? I checked the logs for this PR at
The separate DCO log confirms missing trailers on |
Goal And Delivered Outcome
status --expected-binding <JSON> --as-of <timestamp> --max-observation-age-ms <integer>returns explicit binding eligibility; wrong/ambiguous identities, non-valid integrity and stale/future observations producestage: unknown. Unbound historical reads keep their response shape.main. This completes only the manual exact-read slice, leaving the parent task open.Author Declaration
Implemented against
docs/architecture/rfcs/harness-selection-dsh-pi-v0.mdImplemented Readback Increment at2b72d30fca7f452a71dffb723ebf6d1e2e878484.build_diagnostic_projectionand reliability diagnostics CLItest_exact_binding_*,test_cli_exact_binding_*BindingIneligibilityis local to diagnostic read refusal, not a second runtime-state vocabulary.Scope And Continuation
Validation
77aabe3297d1168d45651e92dc23a545f78c9a13(same source tree as the working-tree runs).python -m pytest -q tests/capabilities/test_reliability_diagnostics.py tests/capabilities/test_reliability_diagnostics_readback.pyexplore_history_rowsin unchangedtests/presentation/test_linked_goal_sources.py:23on candidate and exact main baselinepytestaggregate failed without running them.git diff origin/main...HEAD --checkFrontend / Visual Evidence
Type of Change
LoopX Area
Technical Direction
Shared-authority RFC fixture impact
N/A: this change does not migrate authority or alter runtime routing.
Boundary Checklist
Current CI triage
I checked the logs for this PR at
77aabe3against its exact base2b72d30:kernel-static-checksfails with Ruff F401 forexplore_history_rowsattests/presentation/test_linked_goal_sources.py:23on both this PR and the base. Newer mainb952235has the same failure. In this PR, Stage2c and test shards were skipped after that failure; thestage2c-correctness-e2e,checks,pytestandmerge-gatefailures are downstream aggregation failures, not completed test results.smoke:chat-model-menuand failsassert.match(goal, /goal-only-model/): PR build. The exact base's chat-bundle-browser (3) runs that same smoke and fails at the same assertion with the same rendered Goal header. These are different jobs sharing the failing smoke, rather than a claim that a base release build passed or failed.The separate DCO log confirms missing trailers on
5a1c3bacand77aabe32; it is not the cause of the Python or release failures. Full-suite/premerge qualification remains incomplete, and #5208 remains open beyond this manual read slice.