test(turn): keep the current-Turn decision owner guards on the owner - #4467
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
Self-review — shared Turn decision owner (head 0d5154fef)
Re-read the final diff against origin/main 26eaefc21 to check it for what it is: a de-duplication of the decision chain, not a behavior change.
What the diff actually moves. Before: turn.py read live status, resolved the scheduler context, built the capability-hook projection, called build_live_quota_should_run_decision, applied the advisory primary, and signed the envelope; turn_decision.py did the same chain for managed-step. After: build_fresh_turn_decision owns all six steps and both subcommands call it. turn.py keeps only what is genuinely its own: the executing-Turn hook dispatch (passed in), the plan build, and the post-settlement quota readback.
Checks I ran on the review, not just on the tests.
- Ordering. The one ordering change is that the envelope is now signed before
resolve_turn_resume_session_bindinginstead of after. Both are pure functions ofargsand the decision, and the byte-identicalturn planenvelope dump plus the unchanged managed-step smoke output confirm no observable difference. - Hook semantics. An executing Turn still publishes Go/No-Go hooks (
turn_start_hook_dispatchis threaded through), and the managed step still cannot:build_fresh_envelope_for_managed_stephas no hook parameter andturn_managed_step.pyis unchanged. Theinspect-journalguard now also covers the shared owner's live reads, so the read-only branch keeps the same protection it had before. - Leftovers.
operator_inbox_urgency_projectorinturn.pynow serves only the settlementspendand post-commitschedulercallbacks; the shared owner builds its own with identical arguments. Both are pure factories, so this is a second closure allocation, not a second read or write.collect_status,build_live_quota_should_run_decisionandproject_live_explore_composition_frontierinturn.pyare still used by those settlement paths, and their route source isloopx_turn_run_once, not the plan route - that decision is deliberately different and was left alone. - Re-inlining cannot come back silently. The new plan-mode test reads
0shared-owner status reads against the pre-fixturn.pyin this worktree and1on the head, so it fails exactly on the regression the review described. ruff checkclean on both modules and both test files;turn.pyis 1060 lines against its 1114-line frozen budget.
Residual risk / not covered. No live managed Turn against a real provider credential was run here; the hermetic managed-step and managed-default-flow smokes cover the same CLI surface. Nothing in this PR touches protocol fields, defaults, permissions, or private state.
Verdict: the reported duplication is removed with one owner and parity evidence; no blockers found in self-review.
huangruiteng
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
Exact head reviewed: 0d5154fefe0815921f3b2a886f73889556db8ec3
动机
#4443 把 run-once 的 Turn 决策改回内联构造后,同一条链在仓库里存在两份:turn.py 自己读 live status、自己算 scheduler context、自己造决策、自己签 envelope,而 turn_decision.py 里那份共享构造器只留给 managed-step 用。这不是格式重复:两个子命令要回答的是同一个问题——"当前这个 Turn 该做什么"——所以只要以后有人给这条链加一个决策输入(新的 quota 规则、新的 capability hook、scheduler context 字段),就有可能只改到一边,run-once 和 managed-step 对同一个 live 状态给出不同结论。影响面是所有走 loopx turn 的 goal;不修的代价随决策输入数量线性增长,而且漂移是静默的,两个子命令返回的都是 schema 合法的 envelope。
本 PR 就是按上一轮评审要求做的收敛:删掉内联副本,让 turn.py 走共享 owner,并补一条能看见"副本"这件事的回归测试。修法已经是最小可用形态——既不需要新决策服务,也不需要保留双份再加注释(后者并不会消除漂移成本)。
改动思路
入口是 loopx turn <plan|run-once|managed-step>:handle_turn_command 现在只调用 build_fresh_turn_decision(args, registry_path=..., runtime_root=..., runtime_root_arg=..., turn_start_hook_dispatch=...),拿到 FreshTurnDecision(decision, envelope)。唯一的决定者是 turn_decision.py 这条链:collect_turn_status_payload → build_turn_decision_builder(同一个 route source loopx_turn_plan、同一个 periodic-report interaction hook、同一个 bounded-research frontier projector)→ apply_controller_advisory_primary → fresh_turn_envelope。整个模块只读:不铸造 Turn、不写状态、不花配额。
正路径与改动前完全一致,实测 crowded turn plan 的 JSON 在 merge base 与 head 上是同一份字节(9386 chars 两边相同),所以这是行为保持的去重而不是行为变更。与既有实现的关系也清楚:managed-step 那条 build_fresh_envelope_for_managed_step 保留原签名,只是改成委托同一个函数,仍然不派发 Turn-start hook;而"是否派发 hook"这种真正属于调用方的差异继续由调用方传入(turn_start_hook_dispatch),没有被塞进共享 owner。
共享 owner 的返回形状是本 PR 里唯一的新契约:FreshTurnDecision 用 frozen dataclass 把"决策"和"由这份决策签出的 envelope"绑在一起,调用方不会再拿到一份决策、自己另外签一份 envelope。
具体改动
4 个文件,全部在 CLI 包与测试里。turn_decision.py 新增 turn_scheduler_execution_context(把 scheduler context 的解析规则命名一次)、FreshTurnDecision、build_fresh_turn_decision,并把 build_fresh_envelope_for_managed_step 改成委托;turn.py 删掉内联的 status/scheduler/decision/envelope 构造与随之失效的 5 个 import,改为消费共享结果;test_loopx_turn_driver.py 新增一条回归测试并把 2 个 monkeypatch 目标改到真正的签名方;test_loopx_turn_journal_inspection.py 扩展了 inspect-journal 不得触达的读取点。
关键代码讲解
build_fresh_turn_decision(loopx/cli_commands/turn_decision.py:181):先读一次 live status,再交给 build_turn_decision_builder,应用 advisory primary 之后用同一份 decision 签 envelope,整体以 FreshTurnDecision 返回。它把"决策"和"envelope"从两次构造变成一次绑定的产出,这是消除漂移的机制本身。
build_fresh_envelope_for_managed_step(loopx/cli_commands/turn_decision.py:203):签名与 hook 策略不变(managed step 仍然只读、不派发 hook),只是内部改为委托。这样两个子命令共用一条链,而"是否在执行前发 Go/No-Go hook"仍然由调用方决定。
handle_turn_command 的调用点(loopx/cli_commands/turn.py:154):原来约 40 行的内联构造变成一次调用,再加 decision = fresh_decision.decision、turn_envelope = fresh_decision.envelope。调用方只保留真正属于自己的东西:hook 派发决策,以及 run-once 后续对原始 decision 的使用。
turn_scheduler_execution_context(loopx/cli_commands/turn_decision.py:60):把 scheduler context 的解析从"两处各写一遍"改成命名规则,决策构造与 envelope 签名都从这里取,签名一致性不再靠人工对齐。
test_turn_cli_resolves_its_decision_through_the_shared_owner(tests/test_loopx_turn_driver.py:1518):断言 len(status_reads) == 1——即 live status 只能通过 turn_decision.collect_status 被读一次。它锁的不是 payload 形状,而是"是谁读的",这正是私有副本与共享 owner 的可观测差别。
对主干的风险
非阻断(P3)——本地 pre-merge 门在当前分支状态下会红,但原因不是这份 diff。 loopx canary premerge --from-git-diff 报 status=failed:diff 检查与 changed_python_py_compile 通过,8 条 catalog canary 里 examples/control_plane/cli-output-budget-regression-smoke.py 失败,报 surface/loopx_turn_plan/crowded/json 的 chars grew by 404; allowance is 160。我逐层定位过:干净 origin/main worktree 上同一条 smoke 通过;把差分基准换成 merge base(26eaefc21)后通过(base=102 candidate=102 review_required=0);crowded turn plan 的 JSON 在 merge base 与 head 上完全一致(9386 chars);而当前 origin/main 渲染同一个 fixture 只有 8984 chars。也就是说 main 在本分支分叉之后把这一面缩小了约 402 chars,本分支落后于 main,于是"相对 main 增长"是分支陈旧的假信号。修法是在合并前 rebase 到当前 origin/main 再重跑门禁,不需要改这份 diff 的代码。评审建议:合并动作必须发生在 rebase 后的新 head 上,重新跑一次门禁。
非阻断(P3)——run-once 执行后的 scheduler phase 复读仍是独立构造。 turn.py:918 在 Turn 跑完后会用 route_source="loopx_turn_run_once" 重新读一次 live decision 来投影 execution_phase。它是一次"执行后复读",与本次收敛的"执行前治理决策"不是同一个投影,因此把它折进同一个 owner 反而会混淆两个变更理由。但它是目前唯一还在重复这份决策输入参数列表的地方;若以后新增决策输入也需要被 phase 投影看到,建议把参数定义抽到共享 owner 里参数化 route source,而不是再抄一遍。
残余风险与证据边界。 我实跑过的是:exact head 上 pytest tests/test_loopx_turn_driver.py -q(70 passed)、把 merge base 的 turn.py 还原后的变异对照(新测试以 shared-owner status reads: 0 失败,随后工作区已复原并确认 clean)、merge base 与 head 两个 worktree 上同一 fixture 的 turn plan JSON 逐字节比对、干净 origin/main worktree 上的同一门禁,以及以 merge base 为基准的差分 smoke。未取用的是远程 CI(本轮策略 wait_for_ci=false),所以 GitHub 侧该 head 的门禁结果不构成本次证据;分支落后于 main,rebase 之后上述复现需要重跑。
我的整体评价
这是我上一轮点名的"接手时要额外核对的真实成本"的正面修复,而且修得干净:重复的不是格式而是权威,PR 的处理方式也是删掉第二份权威而不是给它加注释;managed-step 的只读边界(不发 hook、不铸造 Turn)在重构后仍然成立;FreshTurnDecision 用类型把"决策"和"由它签出的 envelope"绑在一起,避免调用方配错对。行为保持也不是自述:crowded turn plan 的输出在 merge base 与 head 上逐字节相同,全量 Turn driver 套件绿,变异对照证明新测试真的会在私有副本回归时失败。
唯一需要在合并前处理的是流程性的:本分支落后于 origin/main,而 main 之后把 turn plan 这一面的输出缩小了,导致本地 pre-merge 门出现"相对 main 增长"的假阳性。rebase 到当前 main 再重跑门禁即可,代码无需改动。剩下那条 P3(run-once 执行后的 phase 复读)已经记录为后续可选收敛项,不构成阻断。请以 rebase 后的新 head 重新走一次合并前门禁。
English verdict: APPROVE — exact head 0d5154fefe0815921f3b2a886f73889556db8ec3 of #4467. The change restores the single current-Turn decision owner: turn.py now calls turn_decision.build_fresh_turn_decision instead of re-inlining status -> scheduler context -> decision -> envelope, the managed step keeps its read-only contract through a delegating build_fresh_envelope_for_managed_step, and FreshTurnDecision binds the decision to the envelope signed from it. Evidence: pytest tests/test_loopx_turn_driver.py -q passes 70/70; the crowded turn plan JSON is byte-identical between the merge base 26eaefc21 and this head (9386 chars each), so the de-duplication is behavior-preserving; and a mutation control restoring the pre-change turn.py makes the new regression test fail with shared-owner status reads: 0, so the test really observes the private copy. Two non-blocking P3s: the local loopx canary premerge --from-git-diff currently fails on surface/loopx_turn_plan/crowded/json (chars grew by 404; allowance is 160) but that is branch staleness rather than a diff defect — the same smoke passes on a clean origin/main worktree, the differential with the merge base as base passes (102/102 rows), and current origin/main renders that fixture at 8984 chars while this head equals its merge base — so rebase onto current main and re-run the gate before merge; and the post-execution scheduler phase probe at turn.py:918 still carries its own copy of the decision inputs for a deliberately different (post-run) projection. Remote CI was not consulted (wait_for_ci=false), and no reproduction from this review should be carried across the rebase.
`run-once` and `managed-step` must resolve the same governing decision. #4443 (2e96a57) replaced the shared call in `loopx/cli_commands/turn.py` with a private copy of the whole chain - live status, scheduler context, capability hook projection, decision parameters, advisory-primary rebinding and the signed envelope - and a1213ed (#4451) restored only the advisory-primary half. The copy stayed, so a decision input added to the shared owner would have moved one Turn subcommand and not the other. Route `run-once` through the shared owner. `build_fresh_turn_decision` owns the whole chain and returns the decision together with the envelope signed from that same decision, so an owner that also needs the raw decision (reward recall) cannot re-derive a second copy. `build_fresh_envelope_for_managed_step` stays the read-only projection: it exposes no hook parameter, so the managed step cannot start publishing Go/No-Go hooks by accident. Evidence (`/private/tmp/loopx-turn-dedup`, origin/main 292b85e): - `turn plan` on the same fixture emits a byte-identical `turn_envelope` before and after (`diff` of the JSON dumps: no difference). - `examples/loopx-turn-managed-step-self-heal-smoke.py` prints identical output before and after. - `tests/test_loopx_turn_{managed_step,driver,executor,codex_cli}.py`, `tests/test_turn_{envelope,managed_executor_binding,default_host_binding, loop_disposition}.py`, `tests/test_loop_{turn_loop_controller}.py`, `tests/test_loopx_turn_{settlement_parity,host_failure,journal_inspection, transaction}.py`: 386 passed. - `tests/architecture` plus the control-plane portfolio/CLI-budget/capability memory/shadow-e2e suites: 140 passed. The test seam for the adaptive orchestration contract moves with the code: the envelope is signed by the shared owner, so `tests/test_loopx_turn_driver.py` injects `build_turn_envelope` where that owner resolves it, and `tests/test_loopx_turn_journal_inspection.py` guards the live reads of the shared owner instead of the removed `turn.py` re-import. A new plan-mode test fails if `turn.py` ever reads its live status outside the shared owner again. The post-settlement `loopx_turn_run_once` scheduler re-evaluation still builds its own decision: it deliberately re-reads status after the commit, has no advisory primary and carries a different route source, so folding it into the plan owner would change behavior. Boundary considered, left as is. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
The shared Turn decision owner now performs the live reads a Turn used to resolve for itself: live status, scheduler execution context and the operator-inbox projector live in `cli_commands/turn_decision.py`, and the envelope is signed from the owner's decision. The inspect-journal guard still patched those names on the command module, so it raised `AttributeError` instead of proving the inspection branch stays read-free, and `main` has been red on it since the owner landed. Patch the three reads where they live, keep the envelope builder the command still signs with, and add a regression test that fails when a subcommand re-inlines the shared status read. Both guards are mutation-checked: a duplicate status read fails the new test, and a live read on the inspection path fails the existing one. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
0d5154f to
263d08e
Compare
Self-merge validation (263d08e)Owner-authorized self-merge for a test-only diff. Before merging:
Superseded scope: the source-side refactor this branch originally proposed landed as #4496, so it is dropped here rather than re-applied. |
Summary
mainis currently red ontests/test_loopx_turn_journal_inspection.py::test_inspect_journal_cli_branches_before_live_or_write_paths.This PR fixes it and restores the guard's real coverage.
What happened to this PR's original scope
This branch originally proposed restoring a shared current-Turn decision owner.
That refactor already landed as #4496, in a fuller shape:
cli_commands/turn_decision.pynow owns a
FreshTurnDecisionOwnercarrying the live status, the schedulerexecution context, the operator-inbox projector and the decision builder, and
cli_commands/turn.pyresolves every Turn through it.So the source-side change of this branch is superseded and has been dropped here:
this PR no longer touches product code. What remains is the part #4496 did not
carry over — the guard tests that keep that ownership honest.
The bug
Moving the live reads out of
cli_commands/turn.pyleft the inspect-journal guardpatching names on the command module that no longer exist there:
turn.pyno longer resolves its own live status, scheduler context oroperator-inbox projector, so the guard raised instead of proving anything. It is
red on
maintoday and reproducible on a cleanorigin/maincheckout.The fix
cli_commands.turn_decision),keep the remaining names on the command module, and add the envelope builder the
command still signs with — so the guard again proves inspect-journal branches
before any live or write path.
status read instead of resolving through the owner.
still where
run-oncesigns its envelope, so retargeting it at the decisionowner would have silently stopped exercising the injected contract.
Validation
unitpassedtests/test_loopx_turn_driver.py+tests/test_loopx_turn_journal_inspection.py— 88 passed. Baseline on the same tree before the fix: 1 failed (the AttributeError above)unitpassedshared-owner status reads: 2and fails it; a live read placed on the inspection path fails the existing guard withinspect-journal reached a live or write pathstaticpassedloopx canary premerge --from-git-diff—status: passed,merge_gate_passed: true,manual_holds: 0, 2 changed files, surfacepythontwo guards. No product behavior, permission, benchmark, quota or scheduler
semantics change. Real-backend/PostgreSQL gates do not apply to a test-file diff,
and are not claimed here.
the repository's own test suite, which is what CI reads.