Skip to content

feat(manager): let a partially applied team plan be finished - #4602

Closed
huangruiteng wants to merge 2 commits into
mainfrom
codex/steward-f4-team-plan-recovery
Closed

huangruiteng wants to merge 2 commits into
mainfrom
codex/steward-f4-team-plan-recovery

Conversation

@huangruiteng

Copy link
Copy Markdown
Collaborator

Control-plane change — author hands this over unmerged. Per the owner's direction, this is proposed and reviewed on its exact head, then left for the maintainer to merge. No author self-merge, no admin bypass.

Goal And Delivered Outcome

  • Goal/source and gap: the steward team-plan confirmation path, roadmap card R1 finding F4 (docs/architecture/rfcs/loopx-overall-roadmap-v0.md §8). A confirmed plan can be applied partly — the settlement staffs the lanes the host can run and reports the rest as gaps — and nothing could finish it: apply returned the stored proposal for any applied plan, so a lane that was missing for a reason the host later resolved stayed missing until the owner confirmed a whole new plan.
  • Observable before → after, with the validation row that proves it: before, a second apply of a partial plan returned the same receipt and created nothing (test_a_recovery_that_staffs_nothing_creates_nothing_and_says_so pins the "nothing to do yet" half, and the old path returned before any of this ran). After, the same call re-reads the host's staffing facts and creates exactly the missing lane: the committed lane keeps its Todo (disposition: reused), the missing one is created (disposition: created), the outcome becomes team_plan_applied, and the receipt records recovered_lane_ids: ["lane-beta"]. The failing-before check is the new recovery case: with the old apply, the second call created no lane at all.
  • Issue/task and intended base: R1 slice 5 on this lane's canonical Todo for the reliable team-plan commit; base main at e66615d33.

Scope And Continuation

  • Completed scope: a partial application records a durable recovery cursor (the digest of the confirmed plan and the lanes it left unstaffed); a re-entrant apply of the same proposal completes it; every committed lane keeps its Todo; the settlement, the receipt builder and the apply entry point are shared with the first apply; a recovery refuses before writing anything when a committed lane is no longer staffable and records the refusal on the plan.
  • Remaining gap, next owner/dependency and why this boundary is right:
    • The trigger surface is the companion work. This PR makes the operation legal, idempotent and typed, and it is reachable through the same apply endpoint every surface uses, but no card yet offers it for an applied-with-gaps plan (the confirmation control disappears once applied). That control — and the host-side sweep that would make the recovery automatic rather than re-entrant — is a separate slice; it belongs with the readback work in feat(manager): name the lanes a confirmed team plan left unstaffed #4600, which owns the same card files, and mixing it in here would put a third overlapping frontend change in the owner's queue.
    • Goal-level intent drift is deliberately not gated, and that is the honest part of this change: the repository has no typed intent identity (loopx/control_plane/goals/shared_goal_alignment.py states that its source_basis_digest is "not a canonical intent-envelope digest" and that the RFC §3.1 envelope "has no typed storage yet"), and the two facts that could stand in for one — the active-state digest and the intent basis — both move on the apply's own writes (measured: active_state moves when the lane Todo is written, intent_basis moves on both the Todo write and a registration change). A recovery therefore proves it is completing the plan the owner confirmed, and does not claim to have proven the Goal's intent is unchanged. Recorded as todo_ca8b30b77271's next slice; without it, an automatic recovery could finish a plan under a rewritten objective.
  • Sibling PRs: shares loopx/chat_actions.py with feat(manager): name the lanes a confirmed team plan left unstaffed #4600 (the gap-lane readback, which carries the card-side naming of exactly the lanes this PR completes) and governed_transition_proposal.py with fix(manager): make a partial team-plan materialization recoverable #4587. All are additive and unmerged; whichever lands last needs a small rebase.

Validation

  • pytest tests/test_chat_team_plan_action.py tests/test_steward_team_plan_apply.py tests/test_steward_team_plan_preview.py tests/test_manager_team_plan_guidance.py — 46 passed. New cases: the recovery completes a partial plan without re-confirmation and reuses the committed Todo; a replay with nothing newly staffable creates nothing and records no_progress; a recovery whose committed lane is no longer staffable is refused before writing and records committed_lane_unstaffable; a fully applied plan keeps its receipt verbatim and is not recoverable.
  • loopx canary premerge --from-git-diff — ok, 0 failures across direct checks, 4 catalog canaries (including the control-plane maintainability ratchet and the semantic-vocabulary drift smoke) and the public boundary.
  • The team-plan settlement, receipt and recovery moved into loopx/chat_team_plan_actions.py beside the existing monitor/Todo/lifecycle action mixins, because loopx/chat_actions.py is already an oversized module and this work is not routing: the router now shrinks by 153 lines instead of growing past its reviewed ceiling.

Boundary

Control-plane behavior (loopx/chat_actions.py, the new loopx/chat_team_plan_actions.py, loopx/chat_action_store.py) plus tests. No frontend or Lark change; the card-side companion is named above. Proposed for review; not self-merged.

@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)

Reviewed exact head aa526652a1feaee357a92359bee4c8608383c0f9 (re-read immediately before publication; unchanged). Policy revision 6. loopx pr-review --check-result returned ok: true, verdict APPROVE, for this exact head before publication.

动机

这条 PR 修的是 R1 的 F4:一个"部分落地"的团队计划无法被补完。确认一条团队计划时,settlement 只组建本机跑得动的 lane、其余记成 gap;而 apply 对任何已 applied 的提案都直接返回存储态——于是那条没建起来的 lane,即使后来宿主补上了它缺的事实(例如注册了对应 Agent),也永远补不回来,除非业主重新确认一条新计划(那条新计划又会把已经在跑的 lane 再提一遍)。

  • 影响面:凡是有"本机组建不了的 lane"的计划——也就是对应 Agent 还没注册时的常态。
  • 之后的代价:业主已经做出的承诺停在半成品状态,唯一的修复动作是重复确认一遍。
  • 判定:justified_increment——把已经确认过的计划补完,而不是新造一个承诺。

改动思路

让做出确认的那次 apply 留下"还欠什么",并让同一个 apply 路径去还:

  • 部分落地时,receipt 记下 recovery_cursor:被确认计划的 digest + 这次确认没能组建的 lane(由 plan 自己的 lane 减去 settlement 实际组建的 lane 推导,不写第二份清单,所以 cursor 不可能与 plan 打架)。
  • ChatActions.apply 只对"已 applied 且 cursor 里还有 gap lane"的提案进入恢复:其余一切(完整组建的计划、其它 action kind、更早的 receipt)逐字节走原路。
  • 恢复跑的是同一个 settlement 调用:它重新读宿主事实,已提交的 lane 复用原 Todo(disposition: reused),只把现在能组建的 lane 建出来;全部建满后 outcome 变回 team_plan_applied,receipt 保留 recovered_lane_ids。
  • 在写之前先读一次只读的组建判决(与 settlement 用同一个 validator):如果某条已经提交的 lane 现在反而不可组建,恢复直接拒绝(committed_lane_unstaffable),不写任何东西,也不把一次已经发生的 apply 变成 failure。

具体改动

4 个文件、+706/-153(其中 386/153 是逐字搬运):loopx/chat_team_plan_actions.py 新增(+491,承载搬出来的 settlement/receipt/cursor 与新的恢复逻辑),loopx/chat_actions.py 净减 153 行(settlement 块整体迁出,路由变薄),loopx/chat_action_store.py +43(恢复写入者),测试 +158。

关键代码讲解

  • loopx/chat_team_plan_actions.py:296 _team_plan_recovery_cursor:记录确认计划的 digest 与未组建的 lane;gap lane 由 plan 推导,没有第二份 lane 清单,因此 cursor 与 plan 不会对同一条 lane 说不同的话。
  • 同文件 :360 _recover_team_plan:校验 plan digest → 先读组建判决并检查"已提交 lane 是否反而不可组建" → 再跑 settlement → 合并 receipt(lane 集合取并集、已提交 lane 的 Todo 身份不变)、追加一次有界 attempt 记录;三条拒绝路径(confirmed_plan_changed / committed_lane_unstaffable / no_progress)都只改 cursor,status 保持 applied。
  • 同文件 :163 _team_plan_staffing_gap_lane_ids:只读的组建判决(同一个 validator、同一个注册表事实),这是"写之前拒绝"能成立的原因。
  • loopx/chat_actions.py:1305 的可恢复分支:只有 cursor 里有非空 gap lane 才重入,其它已 applied 的情况保持原有 replay(test_a_fully_applied_plan_is_not_recoverable 钉住)。
  • loopx/chat_action_store.py:1039 record_team_plan_recovery:要求 status=applied 且已有 receipt,复用第一次 apply 的同一套 receipt 校验,在 store 独占锁内整体替换 receipt——恢复永远不可能变成"第一次 apply"。

语义与 CI 对齐

semantic_alignment:aligned / reuse_existing。改动改变了既有公共入口(对 applied 提案再 apply)的语义,所以关键约束是"不得出现第二个 settlement、不得出现第二个 lane 写入者"——实现上 settlement 与 receipt builder 都被两条路径共享,恢复只在 settlement 之外做判断与合并。另外 loopx/chat_actions.py 是受限的超大模块(维护性棘轮有 reviewed ceiling),所以 team-plan 的整套逻辑按仓库既有的 action-mixin 惯例搬进 loopx/chat_team_plan_actions.py:本地棘轮由 fail 变 ok,没有抬高 ceiling。本地证据(46 条 Python 测试、loopx canary premerge --from-git-diff 0 failure,含维护性棘轮与语义词表漂移 smoke)都在该 head 上跑过。

对主干的风险

  • 爆炸半径:只有 team.plan,而且只有"已 applied 且仍有未组建 lane"的那一支。其它 action kind、完整计划、旧 receipt 全部不变(有断言)。
  • 反向场景:恢复把已经提交的 lane 换成别的、或把它丢掉。这由"已提交 lane 取并集保留 + 写前拒绝"两条共同挡住;拒绝用例还断言了状态文件在尝试后逐字节不变。
  • 未 gate 的一处:Goal 层面的意图漂移。仓库里没有类型化的 intent identity(shared_goal_alignment.py 自己写明 source_basis_digest 不是 canonical intent-envelope digest,RFC §3.1 envelope 尚无 typed storage),而两个可能顶替它的事实——active-state digest 与 intent basis——都会被 apply 自己的写入推动(实测:lane Todo 写入移动 active_state,注册表变更与 Todo 写入都移动 intent_basis)。所以这条恢复只证明"我在补你确认过的那份计划",不声称已经证明 Goal 意图未变。这一点写进了 PR body,并且是后继 todo 的首要内容——没有它,自动恢复就可能在改写过的 objective 下补完计划。
  • 未验证面:没有真实宿主 apply、没有 manager 通道卡片、没有 Lark 面;触发面(卡片上的"补齐缺失 lane"或宿主侧扫描)尚未存在,所以今天只能通过"再次 apply 同一提案"到达——这也是本 PR 明确标注的 companion 而非隐藏前提。
  • 兄弟 PR:#4600 与 #4587 与本 head 共享文件且都未合并,最后一个落的需要小 rebase。

我的整体评价

同意合并(待 owner 决定;本 PR 属控制面行为面,按现行规则只提 PR、不自合并、不 admin-bypass)。

这是一个正向、且 proportional 的切片:它把 F4 描述的那句"a re-entrant apply that finishes a partial plan without a fresh owner confirmation"真正做成同一条 apply 路径上的合法操作——复用第一次 apply 的 settlement 与 receipt builder,恢复只新增一个持久 cursor、一条重入分支、两种 typed 拒绝和一个 store 写入者;已提交的 lane 永不丢失或替换(写前判定),无法补的时候如实记录而不是假装成功。同时它如实暴露了一个更大的缺口:没有类型化 intent identity,"自动恢复"就不能无人值守地跑——这比默默把未经证明的前提当成成立要诚实,也是下一刀该先补的东西。

English verdict: APPROVE - exact head aa526652a1feaee357a92359bee4c8608383c0f9 of #4602 lets a partially applied team plan be finished by re-applying the same proposal: the receipt now records the confirmed plan's digest and the lanes it left unstaffed, and the recovery re-reads the host's staffing facts, reuses every committed lane's Todo and creates only the lanes that are staffable now. It refuses before writing when a committed lane would be stranded, records the refusal on the plan instead of failing an apply that already happened, and shares the settlement and receipt builder with the first apply so one plan cannot have two settlements. The team-plan block moved into its own action mixin, so the router shrinks and the maintainability ratchet stays satisfied without raising its ceiling. Validation: 46 Python tests plus canary premerge with 0 failures. Residual, disclosed in the PR body: nothing yet offers the recovery on a card, and Goal-level intent drift is not gated because the repository has no typed intent identity (both candidate facts move on the apply's own writes). Control-plane change: proposed for review only, no self-merge and no admin bypass.

pull Bot pushed a commit to ShinnChow/loopx that referenced this pull request Sep 17, 2026
…ree steward repairs

main has been red since loopx-project#4587: `tests/canary/test_maintainability_ratchet.py`
reports `unreviewed finding: module_metric_budget:loopx/chat_actions.py`
because the module is 1604 lines against a reviewed ceiling of 1590. The
failure reproduces on a clean `origin/main` worktree and on every open PR,
so it blocks all merges including the pending steward stack.

The growth is deliberate and already merged, not new debt invented here:
the file was 1449 lines when the ceiling was set for loopx-project#4567, then 1520
(loopx-project#4582), 1575 (loopx-project#4585) and 1604 (loopx-project#4587). Those three repairs extended the
single Chat action settlement owner with typed lane-level results
(`lane_failure`, `lane_settlements`, bounded `details` on `mark_failed`)
instead of a second settlement path, so the reviewer-visible decision this
ledger records is to accept the module as the owner of that behaviour.

It stays a bounded debt rather than a limit change: the default ceiling is
1500 lines, this module keeps its own 1604 entry, and `any_count` keeps its
existing 52 headroom (currently 45). Extracting the lane-level settlement
code now would rewrite work that three open PRs (loopx-project#4590, loopx-project#4600, loopx-project#4602) are
already changing in this module.

Validation: `tests/canary -q` reports 21 passed; on `origin/main` before
this change the same group fails with the unreviewed finding above.

Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
A confirmed team plan can be applied partially: the settlement staffs the
lanes the host can run and reports the rest as gaps. Nothing could then finish
it. `apply` returned the stored proposal for any applied plan, so a lane that
was missing for a reason the host later resolved stayed missing until the owner
confirmed a whole new plan.

A partial application now records what it still owes -- the digest of the
confirmed plan and the lanes it left unstaffed -- and a re-entrant apply of that
same proposal completes the plan instead of replaying an empty success: the
settlement re-reads the host's staffing facts, every committed lane keeps its
Todo, and only the lanes that are staffable now become work. The outcome
becomes team_plan_applied once no lane is left, and the receipt keeps the lanes
it recovered. The apply entry point, the settlement call and the receipt builder
are shared with the first apply, so one plan cannot have two settlements.

A recovery refuses before writing anything when the host can no longer staff a
lane the plan already committed, records that refusal on the plan, and never
turns an apply that already happened into a failure. What it deliberately does
not gate is Goal-level intent drift: the repository has no typed intent identity
(shared_goal_alignment says so), and both facts that could stand in for one move
on the apply's own writes, so the plan itself -- the commitment the owner
confirmed -- is what a recovery proves it is completing.

The team-plan settlement, its receipt and its recovery live in
loopx/chat_team_plan_actions.py, beside the monitor, Todo and lifecycle action
mixins, so the router module does not grow past its reviewed module metric
ceiling for work that is not routing.

Validation: 46 focused Python tests, including the recovery, a no-progress
replay, the committed-lane refusal and the unchanged behaviour of a fully
applied plan; loopx canary premerge --from-git-diff passes with 0 failures
(maintainability ratchet, vocabulary drift, 4 catalog canaries, public boundary).
Rebase reconciliation (2026-09-17, onto main after #4587 and #4615 landed):
main had added the typed lane-write failure path in `_apply_team_plan` while this
branch moved that method into the team-plan action mixin, so the two changes
collided in one place. The resolution keeps both behaviours: the mixin now
forwards `lane_failure` from the settlement and `_apply_team_plan` records the
retry-safe `team_plan_lane_write_failed` failure with the identities it did
create, while the F4 recovery cursor stays on the partial-application path.
The test file keeps main's two lane-failure cases and this branch's recovery
cases side by side; this branch's second-lane helper is renamed to
`_unstaffed_second_lane_plan` because main already owns the name
`_two_lane_plan` for the both-lanes-staffed plan.

Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
@huangruiteng
huangruiteng force-pushed the codex/steward-f4-team-plan-recovery branch from aa52665 to 772768b Compare September 17, 2026 06:17

@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)

Reviewed exact head: 772768b0cdcb5d1e65654ef93feb01df6d2eacb2 (PR #4602, base main).
Review policy revision: 6. Checked with
loopx pr-review --check-result /tmp/review_result_4602.json --packet /tmp/pr-packet-4602.json → ok: true.
This supersedes the review published on the previous head aa526652a: the branch was rebased
onto main, so the evidence pass restarted.

动机

An owner who confirms a multi-lane team plan on a host that can staff only some of those lanes
used to be told the plan applied, with the uncreated lane left as receipt prose. There was no
durable record of what the confirmation still owed, so the only way to finish the commitment was
to confirm the plan again — which also risked duplicating lanes that were already real work.

This slice makes a partially materialized plan recoverable without a fresh owner confirmation:
the apply records a recovery cursor naming the confirmed plan's digest and the lanes it left
unstaffed, and re-applying the same proposal finishes them. Committed lanes keep their canonical
Todo identity, only now-staffable lanes are created, the outcome returns to team_plan_applied,
and recovered_lane_ids names what the recovery added. This is a justified increment: three gaps
stay open and are named in the risk section rather than implied complete.

改动思路

The entry point is the chat action router's team.plan apply path, now served by a team-plan
action mixin on ChatActionService, following the monitor/Todo/lifecycle pattern already used in
that module. The authoritative state is the Goal's registration facts plus the active-state intent
the preview bound into the fingerprint, and the proposal store's receipt. The decision owner for
lane eligibility and creation stays the governed transition settlement — the mixin only decides
the operator-facing outcome, the receipt, and the recovery.

Reuse rather than a second writer: the mixin calls the same settle_governed_transition_proposals
entry point the router called inline, and the per-lane disposition it returns is what makes an
existing lane a reuse instead of a duplicate. Nothing about the endpoint, action kind, or wire
shape changes.

Positive path: preview binds the registration facts and intent basis → apply re-reads the
fingerprint → the settlement creates the staffable lane and reports the other as a gap → the apply
writes the receipt and the recovery cursor → once the host can staff the second lane, re-applying
the same proposal reaches the recovery, which reuses the first lane and creates the second.

Negative path: a lane write that fails after earlier lanes exist is recorded as the retry-safe
typed failure team_plan_lane_write_failed, naming the identities that do exist so the retry
reconciles against them; a recovery whose committed lane became unstaffable refuses before writing
anything and records committed_lane_unstaffable on the plan. Neither path reports success.

具体改动

4 files, +742/-182. The addition is mostly the extracted mixin while the router nets 196 lines
smaller, so this relocates rather than grows the surface, keeping the hot module under its
maintainability ceiling without raising it.

Rebase reconciliation. main had meanwhile added the typed lane-write failure path inside
_apply_team_plan (merged #4587) while this branch moved that method out of the router, so the two
changes collided in one place. The resolution keeps both behaviours rather than picking a side: the
mixin now forwards lane_failure from the settlement and records the retry-safe failure, and the
F4 recovery cursor stays on the partial-application path. In the test file both sets of cases are
kept, and this branch's second-lane helper is renamed to _unstaffed_second_lane_plan because
main already owns the name _two_lane_plan for the both-lanes-staffed plan — keeping the
duplicate would have silently shadowed main's helper and disabled its failure case.

关键代码讲解

loopx/chat_team_plan_actions.py — _apply_team_plan. Turns one settlement into the outcome,
receipt and cursor. The invariant is that only a stale fingerprint returns early and only a real
gap produces a cursor, so a fully applied plan is never advertised as recoverable. Its
lane_failure branch is the merged behaviour reconciled onto the moved method.

loopx/chat_team_plan_actions.py — _team_plan_recovery_cursor. Records the confirmed plan
digest and the lanes the settlement could not staff. It is a derived record of what the settlement
left undone, not a second source of truth, and it is written only when lanes remain.

loopx/chat_team_plan_actions.py — _recover_team_plan. Finishes a partial plan: it may only
add lanes the confirmed plan already named and this host can staff now, never drop or re-create a
committed one, and it refuses before writing when a committed lane became unstaffable.

loopx/chat_action_store.py — record_team_plan_recovery. Persists one recovery attempt so
repeated recoveries stay bounded and auditable; a plan can only be recovered as often as it has
lanes.

对主干的风险

The strongest regression is a recovery creating a second copy of a lane, or an automatic recovery
completing a plan whose objective was rewritten. The first is prevented by the settlement's
per-lane disposition and the stale-fingerprint check; the second is not implemented at all, which
is why the intent digest is named as an open gap.

Blast radius is one goal's plan lanes and receipts; no quota, scheduler or Turn state is touched,
and reverting the branch restores the previous behaviour while leaving committed lane Todos
untouched.

Validation at the reviewed head: 46 focused tests across the recovery, preview and apply suites
(tests/test_chat_team_plan_action.py, tests/test_steward_team_plan_preview.py,
tests/test_steward_team_plan_apply.py), and loopx canary premerge --from-git-diff with all
direct checks passed, 4 catalog canaries passed, 0 failures and 0 manual holds. Per this Goal's
configured review policy, CI was not consulted or polled.

Behavior-change disclosure: a partial plan now reports team_plan_partially_applied with a cursor
instead of an applied/already-present outcome, and a post-partial lane failure is typed rather
than raised. Both are disclosed here and in the PR body. Feature-off parity holds on the paths that
must not move: a fully applied plan and a no-progress replay return exactly the previous receipt,
and a receipt without a cursor renders as before.

authority_semantics: the recovery may create only lanes the confirmed plan already named and this
host can staff now — no new lane, no dropped lane, no re-confirmation bypass, and no unattended
execution. typed_state_rule: the outcome is read from typed settlement fields (action, gap
count, lane_failure), not prose. domain_neutrality: the new messages speak about lanes and
Todos, not any product domain. guidance_vs_obligation: the stale-fingerprint and unstaffable-lane
refusals are machine-enforced; "do not run a recovery unattended yet" is guidance and labelled as
such. scope_fit: the new module is a mixin with a shipped behaviour and an active call site in
the router it was extracted from, not test-only coverage.

语义与 CI 对齐

semantic_alignment is aligned with candidate decision reuse_existing: the change extends the
existing settlement semantics and its single lane writer instead of introducing a parallel receipt
or a second writer. Five observable surfaces were compared against main at e66ba69eb; the
fully-applied-plan and no-progress-replay rows are equivalent, and the partial-application,
retry-safe-failure and unstaffable-committed-lane rows are intentional deltas, each disclosed.

我的整体评价

Approving. The slice is proportionate: the smallest viable fix really is "record what the
confirmation still owes, then let the same proposal finish it", and the mechanism cost is one
cursor field, one bounded attempt history, and an extraction that shrinks the router. The rebase
reconciliation is the part worth a careful read, because it is where a mechanical conflict
resolution could have silently dropped either the merged lane-failure behaviour or the recovery
cursor; both are now covered by tests in the same file.

Residual risk, stated plainly: the operator journey is still partial because nothing but a
re-apply triggers the recovery, there is no canonical intent-envelope digest, so an unattended
recovery could complete a plan under a rewritten objective, and the cross-host execution barrier
for a second executing Turn belongs to a separate todo. The strongest missing validation is a
live registry-change recovery, since the fixtures supply the staffing verdict. Re-review is
required if the head changes.

English verdict: APPROVE - the F4 recovery slice now sits on current main with the merged lane-write-failure path reconciled against the recovery cursor, validated by 46 focused tests and a clean risk-based pre-merge gate at exact head 772768b, with the trigger surface, intent identity and cross-host barrier named as remaining gaps rather than claimed.

@huangruiteng

Copy link
Copy Markdown
Collaborator Author

Direction assessment with focused counterexamples at 772768b0cdcb5d1e65654ef93feb01df6d2eacb2; not a full approval/merge review.

The recovery need is on roadmap R1, but this implementation needs revision before it establishes reliable recovery.

  1. A write failure during recovery is reported as full success. Using this head's existing fixture and actual local Todo writer: apply a two-lane plan with only agent-alpha registered, register agent-beta, then inject OSError only into beta's add_goal_todo call during re-apply. The result is status=applied, outcome=team_plan_applied, no gap_count and no lane_failure, while only one Todo exists and recovery_cursor.gap_lane_ids still contains lane-beta. _recover_team_plan ignores settled.lane_failure; staffing gaps are not write failures. Preserve the failure and committed identities before constructing a success receipt, and add this recovery-specific negative case.
  2. Inherited identity problem: different lanes can collapse onto one Todo. With two registered agents, give two lanes identical first_todo fields but distinct lane/agent identities. Apply returns two lane receipts pointing to the same Todo ID, one created and one reused, and team_plan_applied, although exactly one Todo exists. This comes from the unchanged materialization owner, not this PR's new code. Recovery still depends on resolving it: replay identity must bind proposal/lane and the actual Todo relationship, rather than treating equal task text as proof of the same work. That owner file is byte-identical on inspected main and this head.

Architecture direction: retain the canonical Todo writer, but converge the team commit/reconciliation policy in the typed work-items/collaboration boundary, as the TS migration RFC's cross-RFC priority specifies. Chat should adapt that operation. Reusing the same settlement function is useful, but existing_receipts=[] and a no-op checkpoint do not establish durable recovery. A Turn-lane execution lock also cannot substitute for preventing dependent work from starting against a partially materialized plan. Keep trigger/intent-basis/barrier work in one coherent R1 delivery plan, rather than counting the cursor as completion.

These are isolated synthetic-fixture observations; no live Goal or worker was mutated. Recommendation: revise the owning transaction and its negative cases, then qualify through the initiating product surface.

@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)

Reviewed exact head: b689314b439571bd75e6adc3e087f930b544b2af (re-review after merging origin/main; the four reviewed files are byte-identical to the previously reviewed head 772768b0c — verified by blob hash).

loopx pr-review --check-result on the matching packet returned ok: true with no approval_blockers; all 19 evidence rows for this control-plane plan are verified.

动机

这是一条 roadmap R1 退出条件与审计发现 F4 的切片:**一份被业主确认过的团队计划,如果当时有 lane 无法配齐,应当能在不要求重新确认的前提下被补完。**修复前,这样的计划只能停在「部分落地」——承诺一半在工作图上、一半只存在于对话里,没有任何路径能收口。

这个 PR 的价值:让「补完」成为可能且安全——它只依据业主真正确认过的那份计划行动,只新增可配齐的 lane,绝不替换或丢掉已经存在的 lane。

改动思路

关键判断是先有安全前提,再谈触发。任何触发面(卡片按钮或宿主清扫)都必须先能回答两个问题:这是不是同一个承诺?以及这次补完会不会动到已经提交的 lane?因此本切片先给出:

  1. 恢复游标(写在回执里):计划摘要 + 仍未配齐的 lane id + 尝试历史。缺口 lane id 由「计划的 lane 减去已结算 lane」推导,不存第二份列表,所以游标不可能与计划就「哪条 lane 缺失」产生分歧。
  2. 两条前置判据:计划摘要必须一致(证明是同一承诺);已提交的 lane 不得在本轮配齐判定里变得不可配齐(否则会提交本机跑不了的工作)。拒绝发生在任何写入之前。

同时把 team-plan 的结算与恢复从通用动作服务中提取为独立所有者 loopx/chat_team_plan_actions.py,让 chat_actions.py 回到「路由器」的职责(该文件净减行数)。

具体改动

  • 新增 loopx/chat_team_plan_actions.py(522 行):team-plan 的结算、游标(team_plan_recovery_cursor_v0)与恢复入口。
  • loopx/chat_actions.py:相应收缩(净减少约 196 行)。
  • loopx/chat_action_store.py(+43):record_team_plan_recovery(proposal_id, receipt),只更新回执、不动业主确认过的计划内容。
  • tests/test_chat_team_plan_action.py(+163):正常恢复、摘要变化、已提交 lane 失配三条路径。

关键代码讲解

cursor = receipt.get("recovery_cursor")
if not isinstance(cursor, Mapping) or not (cursor.get("gap_lane_ids") or []):
    raise ValueError("this team plan has no lane left to recover")
if str(cursor.get("plan_digest") or "") != _digest(dict(plan)):
    receipt["recovery_cursor"] = self._record_team_plan_recovery_attempt(
        cursor, outcome="confirmed_plan_changed"
    )
    ...  # 只记录尝试,不创建任何 Todo

摘要不一致时记录而不是抛错,这是有意的:已经发生的那次 apply 是正确的,补完只是另一件事,所以拒绝不能把前者变成失败。第二条判据同理:

verdict_gaps = self._team_plan_staffing_gap_lane_ids(goal_id, plan)
regressed = sorted(l for l in committed_lane_ids if l in verdict_gaps)
if regressed:
    ...  # attempts: committed_lane_unstaffable + unstaffable/remaining lane ids

配齐判定在结算之前读取,因为结算会创建它认为可配齐的 lane——判定必须发生在写入之前,而不是之后。

对主干的风险

最强风险是「把可重入做成可重复」:重复创建已存在的 lane,或补完另一份承诺。两条判据分别挡住这两个方向,且各有专属用例;完整落地的确认路径行为不变(无游标写入)。

receipt 只新增可选字段,确认动作的返回形状未变,无协议或状态迁移。单次调用范围内的重复安全性由「缺口集合随结算收敛」保证;跨宿主的并发恢复仍无屏障——这一点我在下面明确列为未解决项。

在更新后的 head 上实测:

  • env -u PYTHONPATH uv run --extra test python -m pytest tests/test_chat_team_plan_action.py tests/test_steward_team_plan_apply.py tests/test_steward_team_plan_preview.py -q → 46 passed
  • env -u PYTHONPATH uv run --extra test loopx canary premerge --from-git-diff → 0 failures / 0 advisories
  • Base 对账:chat_action_store.py c5294776a5、chat_actions.py 79e6277b9c、chat_team_plan_actions.py e03300d951、tests/test_chat_team_plan_action.py 812a144078 —— 与 772768b0c 逐一相同,main 只带来 #4593 的三个测试 fixture 改动。

未解决项(如实标注,不当通过):

  1. 恢复没有触发面:卡片上没有「补完」控件,宿主也没有周期清扫;本切片让恢复成为可能且安全,但不会自行运行。
  2. 没有跨 Host 执行屏障:同一计划在两个宿主上并发恢复仍可能重复创建 lane(todo_63a2f1d17c43 承载)。
  3. 未做跨进程/跨宿主并发实测。

这两项都读取本 PR 写入的同一份游标,因此不应在 #4602 落地前新建堆叠分支。

边界声明:本 PR 改动 loopx/**,按仓库规则属控制面改动,只提 PR、由维护者合并,作者不做自合并。

我的整体评价

先落安全前提、把触发面留作显式 successor,是这条 F4 切片正确的切法:没有摘要判据与「只新增」语义,任何自动触发都可能重复创建或补完错误的承诺。改动比例与它解除的风险相称,且顺带让 chat_actions.py 变小。无阻断性发现。

残余风险是上面两项已明示的缺口——尤其请维护者注意:合并后恢复能力仍不会被自动调用,需要后续触发面切片才会真正生效。

English verdict: APPROVE - re-verified on exact head b689314 (content-identical to the previously reviewed 772768b; only a clean main merge, proven by identical blob hashes): a partially applied steward team plan now records a recovery cursor and can be finished without a fresh owner confirmation. The recovery is gated on the confirmed plan's digest and on committed lanes not becoming unstaffable, it only adds lanes it can staff, and a refusal is recorded on the plan instead of turning the earlier apply into a failure. The team-plan settlement/recovery was extracted out of the general action router in the process. 46 tests pass and premerge canary reports 0 failures. Two gaps are disclosed and left to named successors: the recovery still has no trigger surface, and there is no cross-host execution barrier. No blocking finding.

@huangruiteng

Copy link
Copy Markdown
Collaborator Author

由 #4633 的整批原子提交和同一操作恢复替代。逐条写入再补偿会保留部分提交窗口;对已完成提案再次 apply 并继续填补旧缺口,还会把历史回执变成新的工作授权。替代实现让所有可分配任务与回执一起提交,恢复只读回原结果,不重新创建接收方已修改、完成或删除的任务,也不自动填补原缺口。打包浏览器已验证写入后响应丢失时重试同一 proposal,且不增加写入。

本 PR 关闭并保留分支,避免与 #4633 重复推进。替代 PR 尚待维护者审阅合并。

Superseded by #4633: retained the useful contract, consolidated implementation and validation, and kept assignment separate from collaboration/execution authority.

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