Skip to content

test(turn): keep the current-Turn decision owner guards on the owner - #4467

Merged
huangruiteng merged 2 commits into
mainfrom
codex/restore-turn-decision-dedup
Sep 16, 2026
Merged

huangruiteng merged 2 commits into
mainfrom
codex/restore-turn-decision-dedup

Conversation

@huangruiteng

@huangruiteng huangruiteng commented Sep 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

main is currently red on
tests/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.py
now owns a FreshTurnDecisionOwner carrying the live status, the scheduler
execution context, the operator-inbox projector and the decision builder, and
cli_commands/turn.py resolves 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.py left the inspect-journal guard
patching names on the command module that no longer exist there:

AttributeError: <module 'loopx.cli_commands.turn'> has no attribute 'build_lark_operator_inbox_urgency_projector'

turn.py no longer resolves its own live status, scheduler context or
operator-inbox projector, so the guard raised instead of proving anything. It is
red on main today and reproducible on a clean origin/main checkout.

The fix

  • Patch the three moved reads where they now live (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.
  • Add a regression test that fails when a subcommand re-inlines the shared live
    status read instead of resolving through the owner.
  • Keep the existing adaptive-envelope injection on the command module: that is
    still where run-once signs its envelope, so retargeting it at the decision
    owner would have silently stopped exercising the injected contract.

Validation

  • Tested revision: 263d08e
  • Run state: finished
  • Input classes: synthetic
Check kind Result Public-safe evidence / limitation
unit passed tests/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)
unit passed The two guards are mutation-checked. A duplicate live status read moves the new test to shared-owner status reads: 2 and fails it; a live read placed on the inspection path fails the existing guard with inspect-journal reached a live or write path
static passed loopx canary premerge --from-git-diff — status: passed, merge_gate_passed: true, manual_holds: 0, 2 changed files, surface python
  • Coverage and gaps: the change is test-only, so the covered behavior is exactly the
    two 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.
  • Delivery completeness: no user-facing entry point changes; the affected surface is
    the repository's own test suite, which is what CI reads.
  • Frontend / Visual Evidence: Before: N/A. After: N/A.

@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 — 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_binding instead of after. Both are pure functions of args and the decision, and the byte-identical turn plan envelope 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_dispatch is threaded through), and the managed step still cannot: build_fresh_envelope_for_managed_step has no hook parameter and turn_managed_step.py is unchanged. The inspect-journal guard 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_projector in turn.py now serves only the settlement spend and post-commit scheduler callbacks; 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_decision and project_live_explore_composition_frontier in turn.py are still used by those settlement paths, and their route source is loopx_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 0 shared-owner status reads against the pre-fix turn.py in this worktree and 1 on the head, so it fails exactly on the regression the review described.
  • ruff check clean on both modules and both test files; turn.py is 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 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: 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>
@huangruiteng
huangruiteng force-pushed the codex/restore-turn-decision-dedup branch from 0d5154f to 263d08e Compare September 16, 2026 06:45
@huangruiteng huangruiteng changed the title fix(turn): restore the shared current-Turn decision owner test(turn): keep the current-Turn decision owner guards on the owner Sep 16, 2026
@huangruiteng

Copy link
Copy Markdown
Collaborator Author

Self-merge validation (263d08e)

Owner-authorized self-merge for a test-only diff. Before merging:

  • loopx canary premerge --from-git-diff: status: passed, merge_gate_passed: true, self_merge_allowed: true, manual_holds: 0, 2 changed files, surface python
  • tests/test_loopx_turn_driver.py + tests/test_loopx_turn_journal_inspection.py: 88 passed
  • Main-red reproduced on a clean origin/main checkout before the fix, and fixed after it
  • Mutation checks: duplicate live status read fails the new guard; a live read on the inspection path fails the existing guard
  • Public/private boundary: test files only; no product code, private state, raw logs, credentials or local paths

Superseded scope: the source-side refactor this branch originally proposed landed as #4496, so it is dropped here rather than re-applied.

@huangruiteng
huangruiteng merged commit 592b760 into main Sep 16, 2026
16 of 17 checks passed
@huangruiteng
huangruiteng deleted the codex/restore-turn-decision-dedup branch September 16, 2026 06:46
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