test(manager-context): guard receiver isolation and successive requests - #4640
huangruiteng merged 2 commits into
Conversation
…gations for M2 loopx-project#4312 implemented a same-Goal dispatch broker for loopx-project#4311 and was closed as superseded by the merged capable-manager semantic-handoff RFC (loopx-project#4330). The defect is still real, but the accepted direction is one typed Core collaboration request/assessment/result contract, so the acceptance obligations loopx-project#4312 recorded as migration inputs are characterized here against the shipped manager-context inbox and the shipped claim-scope projection instead of reintroducing a parallel broker. Pinned against shipped code: - O1 one executor-excluded independent_handoff successor reads as selectable for a same-Goal peer and as non-selectable for the origin agent, both from the same claim-scope projection - O2 an empty eligible-peer set fails closed on dispatch instead of projecting a user gate - O3 and O5 a repeated dispatch replays one durable entry and never resets a decision the receiver already recorded - O4 delivery, read and decision read back intact from the store after a restart - O6 a conflicting acknowledgement is rejected rather than overwritten - O7 a dispatch is filed under the recipient scope hash, so another agent in the same Goal cannot reach it - O8 dispatch and successor handoff identities are pure functions of their tuples, so a replay is never a redispatch O9 (canonical Todo claim readback) is already pinned by test_links_use_core_state_and_do_not_copy_progress and is only recorded in the ledger to keep the retained set complete. No production code changes. Refs loopx-project#4311, loopx-project#4312, loopx-project#4330 Signed-off-by: YZJF <195568136+YZJF@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Reviewed exact head: 88d2e0f1745c57c31790129bbf5f8695df2893b4 (codex/manager-handoff-m2-characterization).
动机
#4311 描述的真实缺陷是:被 executor 排除的 independent_handoff 后继一直无人认领,而源 monitor 继续轮询。#4312 曾实现同 Goal 的 dispatch broker,被维护者以「已被合并的 capable-manager semantic-handoff RFC(#4330)取代」关闭,并在关闭说明里保留了一份接受义务清单,声明它们会进入 M2 的 characterization / compatibility fixture——但 main 上并没有任何东西把它们钉住。本 PR 给这些义务一个归属:8 条义务绑定到已发布代码的断言,第 9 条记录为「已被既有测试覆盖」,并且由一份 obligation ledger 驱动,如果某条义务被丢弃或失去绑定就会失败。
这是一个边界清晰的增量,而不是缺陷修复:真实缺口(typed collaboration request/assessment/result 契约、也就是真正把后继派给合格 peer)由 RFC #4330 的 M2 工作拥有,本 PR 只是它的兼容基线,并且把这条依赖写进了 PR 描述、模块 docstring 和 fixture 的 why_a_fixture。改动是纯测试、两个新文件、可独立运行、可整文件删除回退。
改动思路
入口是 pytest tests/control_plane/test_manager_handoff_m2_characterization.py,由 ledger fixture(义务 id、retained_from 出处、invariant、asserted_by 或 covered_by_existing)加一个合成 registry(一个双 agent 的 Goal、一个 solo Goal)驱动。被测代码是既有的 loopx/capabilities/manager_context(authority / deliver / pending / acknowledge / tracking.query / record_read)与 loopx/control_plane/todos/quota_selection.project_quota_planning;测试文件本身只拥有断言,没有 mock——写操作全部落在 pytest 的 tmp_path runtime root 上,不触碰任何真实 goal、registry 或生产状态。
我没有把「characterization 自然有价值」当作前提,而是先去找不发布的最强理由,并独立扫了既有覆盖:tests/test_manager_context_handoff.py:56 已经覆盖并发投递幂等(O3),tests/test_manager_context_tracking.py:31 已覆盖投递/读取/决策的重启安全与决策后再投递(O4、O5),:190 已覆盖冲突决策记录(O6),:85 覆盖了 exact audience(O7 的跨 channel 类比),tests/control_plane/test_quota_selection.py:45 覆盖了 executor-excluded 计数(O1 的计数类比)。真正没有既有等价物的是:#4311 特有的组合(被排除的 independent_handoff 后继 + handoff_note + 同一投影对 peer/origin 的两种读法)、以及「没有合格 peer 时是 typed 失败而不是发明 user gate」。这也构成本次唯一的一条非阻塞 P2(见风险一节)。
另外我按仓库要求做了同作者批量扫描:gh pr list --author YZJF --state all 显示只有 4640、4644 处于 open,其余为往日合并,不存在几分钟内同形状批量提交,不属于 PR farming。
具体改动
2 个新文件、510 行新增、0 删除,全部在测试侧:tests/control_plane/test_manager_handoff_m2_characterization.py(404 行)与 tests/fixtures/control_plane/manager_handoff_m2_characterization_v0.json(106 行)。生产代码零改动,我通过 git diff origin/main...HEAD 只列出这两个文件、并重跑既有 parity 套件确认行为未变。
Ledger 自身有真实约束价值:schema 版本、public_safe/synthetic_only 为真、三个「不含凭据/提供商 payload/私有定位符」标志为假、义务 id 唯一、每条义务必须有 retained_from、invariant,以及 asserted_by(必须存在于模块 globals 中)或 covered_by_existing 之一。这让「义务被悄悄丢掉」变成红灯,而不是日后靠人回忆。
关键代码讲解
tests/control_plane/test_manager_handoff_m2_characterization.py:106的 ledger 完整性测试:把 provenance 与绑定变成可执行契约,是整份 fixture 的守门人。- O1(
:136):同一个后继在 origin 侧lanes.open_items为空且executor_excluded_self_count == 1,在 peer 侧selectable_open_count == 1且出现在open_items——两种读法来自同一个project_quota_planning投影,而不是两份事实源。 - O2(
:158):solo Goal 下authority()的 target 列表只有被排除的 origin,deliver以授权错误 fail-closed,并显式断言结果里不存在user_gate/gate键。 - O4/O6(
:210、:283):投递→读取→决策链在「重启」后由query()读回,且第二次不同决策的acknowledge抛错而不覆盖已记录决策。 - O8(
:342):断言 dispatch 身份等于_hash([source_id, request]),并把handoff_note.handoff_id的确定性(同 item 稳定、换todo_id即变)钉住——这正是 RFC #4330 §8「保留原始 tuple-derived dispatch identity 作为 legacy alias」要求的可执行形式。
对主干的风险
我在 exact head 上跑了:新文件 9 passed(2.97s);parity 套件 tests/test_manager_context_handoff.py + test_manager_context_tracking.py + test_manager_context_roundtrip.py + tests/control_plane/test_agent_scope_frontier_contract.py 46 passed(11.84s);文件与 fixture 中不含本地路径、私有定位符或内部标识。头部 18 项检查中 14 项成功、0 失败、4 项仍在排队(review 时 CI 未收敛,已在正文与证据里注明),merge_state=BEHIND 属于分支保护/待更新,不是冲突。
两条非阻塞 P2:
- O3–O6 与既有覆盖重复,约 150 行。仓库明确要求 compress 而不是 append,而这份 fixture 自己已经给出正确处理方式——O9 就是
covered_by_existing。建议把 O3–O6 也指向既有测试(或在 ledger 行里说明「为了 cutover 独立性有意重复」并写明对应测试),只对 O1、O2、O7 的同 Goal 身份隔离、O8 保留新断言。这样 ledger 对新旧覆盖的判定才自洽。 - 断言里混入了实现细节:
_hash/_root私有符号、entries/<hash>/<request_id>.json的存在性与目录列举、以及身份等式。O8 的身份钉住是 RFC §8 明确要求的,合理;但 O3 的目录列举、O7 的存储形状断言会把「保持行为不变的重构」判为失败,可能反过来挡住它本想保护的 M2 cutover。建议最多保留一条显式的结构断言(O7 的「隔离是结构性的、不是过滤读」最有理由),其余改用公开读面(pending/query)表达。
我的整体评价
结论 APPROVE。它把一个已关闭 PR 里保留的义务清单变成了有出处、有绑定、可执行的兼容基线,并且守住了两个真实边界:被 executor 排除的同 Goal 后继在投影里对 peer 可派、对 origin 不可选,以及 dispatch 身份的确定性(RFC §8 的 legacy alias 前置条件)。它没有假装修好了 #4311,也没有生产代码改动,回退成本是删除两个文件。
两条 P2 不影响这个判断:重复覆盖是有界的(约 150 行,且用 ledger 已有的 covered_by_existing 机制即可薄化),结构断言的风险也是可通过收窄到一条来消除的维护问题,而不是当前行为缺陷。CI 尚未收敛这一点我记在证据里:本评审为 exact head 的代码评审结论,合并就绪度(CI 完成、BEHIND 更新)属于另一道关卡,不由本评审授予。
English verdict: APPROVE - exact head 88d2e0f; the ledger turns the closed #4312 obligation list into an enforced compatibility baseline (9 new tests pass, parity suite unchanged at 46 passed, ledger fails if an obligation loses its binding) and O1/O2/O8 guard the shipped projections and the RFC section 8 tuple-derived identity. Two non-blocking P2s: O3-O6 duplicate existing manager-context coverage (bind them with the ledger's own covered_by_existing mechanism instead), and the storage-layout/private-helper assertions in O3/O7 could fail a behaviour-preserving cutover refactor (keep at most one documented structural pin). CI was not final at review time (4 of 18 checks queued, none failing); merge readiness is a separate gate.
…gressions Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Reviewed exact head: 45340f38ba2c4444971f51c6a8908d4d88cd01bd.
Base: a96c9aa91c9d92454efcd7937483bac76e35fd4a (main).
动机
当前目标需要可靠的接收者隔离、方向修正和结果返回。按总体路线图以及 manager handoff RFC §8/§11,characterization 可以独立交付,但必须保护真实语义。本次最终改动是现有消息交换的回归覆盖,不是 #4311 的派发修复或 M2 完成证明。
早先 88d2e0f17 的 APPROVE 对测试价值判断过宽:O4/O6 把消息决定当作 claim/stale-claim;O2 只验证未注册收件人错误;O8 使用的消息身份不是 #4312 的四元组派发身份;ledger 删除全部义务仍能通过。既有 todo-claim-visibility-lanes-smoke.py::assert_executor_exclusion_filters_only_the_named_peer 也已经覆盖 O1 的两种读法。因此旧评审关于“不可丢失的 M2 兼容基线”的解释撤回,由本次最终 diff 的证据取代。
改动思路
复用 tests/test_manager_context_handoff.py 的合成 registry 和真实临时文件存储,走 deliver → pending → acknowledge → query,不新增协议、测试框架或存储 owner。消息、读取、决定和结论仍属于现有 manager_context;Todo/lease 仍有自己的 owner。
与不合并相比,新增了同 Goal 双接收者和连续请求轮次的语义压力;与原案相比,删除独立 ledger 及重复用例。预期结果来自接收者/请求隔离和结论义务,不再由私有 _hash 公式或目录形状充当 oracle。已有并发幂等、外部来源授权、读取/决定跟踪和 roundtrip 用例继续保留。
具体改动
相对最新主线,完整 PR 只改 1 个已有测试文件,+75/-2;原案的两个新增文件均不进入最终 diff。
test_same_goal_recipients_keep_inboxes_and_decisions_separate:同一 Goal 中,未被指定的 peer 看不到请求且不能 ACK;同一输入分别投递给两个接收者,产生不同请求身份,最后分别读回adopt/reject,互不覆盖。test_new_request_round_preserves_the_previous_receiver_decision:修正信息有新的输入身份;旧adopt保留,新请求独立决定为defer,重放第一轮不覆盖任一决定。test_hook_keeps_decided_requests_open_until_receiver_returns_conclusion:已有no_change用例扩展到adopt;两者都保留待回复项和 hook,返回结论后才退出。
这是当前消息生命周期的完整测试增量。#4311 的真实 peer 派发、canonical claim、stale-claim、#4312 legacy alias、崩溃对账及新 M2 消费者仍归既有实现任务;本 PR 不把消息 adopt 升格为 Todo 所有权或工作完成。
对主干的风险
未发现剩余阻塞项。运行时、配置、权限、持久化契约、CLI/frontend/Lark 实现均未修改,因此没有新增默认行为、开关、授权或 UI 配套交付;实际调用的是原有 API 和文件 backend。对 typed-state、领域中立、默认行为披露和 guidance/obligation 的检查没有发现新增生产规则;测试名称也不再夸大 actor 生命周期。
验证:
uv run --extra test python -m pytest -q tests/test_manager_context_handoff.py tests/test_manager_context_tracking.py tests/test_manager_context_roundtrip.py tests/test_chat_manager_context.py:51 passed。- 三个隔离的负向 mutation 全部被对应测试捕获:合并同 Goal 收件人身份、合并输入轮次身份、把
adopt当作已返回结论。故障注入只在临时测试进程生效,未修改生产文件。 - Ruff、diff/compile、变更文件 public-boundary scan 通过;contract-check 的两个警告来自无关的既有全局状态投影,零错误,未伪称整机状态无警告。
loopx canary premerge --from-git-diff --goal-id …:7/7 canaries + 4/4 direct checks passed,无失败、跳过或 manual hold。覆盖 namespace、control-plane risk characterization、hot-path budget、resume gate、quota plan/work-lane 和 monitor scheduler。- 严格质量回执
cqr_3c00e9e52169d69c59f8:valid/pass,精确 scope3c00e9e52169d69c59f85c1614212dea2f85502177796a1a467a3577beba6ec0;0 blockers,未使用额外 quality safe-fix pass。
未执行真实 worker、跨进程迁移、跨主机或 Lark 实网验收;这些不属于本次 test-only delta,也没有被标记为通过。未来维护性检查已应用:将有用断言归回已有测试 owner,取消重复 ledger;没有理由新增抽象。
我的整体评价
APPROVE。当前版本具备明确的回归价值,且不再误报 M2/认领保障。依据本次 owner 对 fork 分支修改并自合并的明确授权,最终范围属于允许自合并的单一测试改动;合并仍以不变 head 的正式评审回读和 check-merge-readiness 返回 ready 为条件。
已读取项目既有评审配置:wait_for_ci=false。本次不以远端 CI 为评审/合并条件,也不把先前看到的排队状态当作通过;本地必需验证和 exact-head review/readiness 仍全部执行。
English verdict: APPROVE - exact head 45340f3; one existing test module now guards same-Goal recipient isolation, independent request rounds and the conclusion obligation after adoption. The duplicate/misleading M2 ledger is removed. 51 focused tests, three mutation probes, 7 canaries, 4 direct checks and the exact-scope quality receipt pass. No production change or M2 dispatch/claim qualification is claimed.
huangruiteng
left a comment
There was a problem hiding this comment.
Reviewed exact head: 45340f38ba2c4444971f51c6a8908d4d88cd01bd (codex/m2-peer-obligation-characterization).
动机
相关 #4311 / #4312 / #4330。已交付的 manager-context 交换已经有重复投递、跨 Goal 隔离、决策与已读追踪的覆盖,但缺少两块:同一个 Goal 内两个接收者之间的收件箱/决策隔离,以及同一输入的后续轮次。回归一旦落在这些语义上(把一条请求同时写进两个 worker 的收件箱、把「请求轮次」的标识折叠、或在收到结论前把已 adopt 的请求移除),现有测试会全绿。
改动把这三种漏掉的情形补上,closes-之外的边界也写清楚了:不关闭 #4311,不宣称 M2 peer dispatch、Todo claim、stale-claim 对账、重启恢复或崩溃后副作用。
改动思路
改动只在既有 owner tests/test_manager_context_handoff.py 内扩展,复用原 fixture 与真实的临时文件系统收件箱 API,并用 loopx.capabilities.manager_context.tracking.query 断言读模型;期望值是语义关系(各自独立的 request id、各自的决策、轮次之间保留先前决策、replay 返回原 id),没有去钉私有 hash 公式或目录布局——这点很重要,否则测试会变成实现的镜像。既有的 hook 用例改为 no_change / adopt 参数化,原有覆盖没有丢。
具体改动
1 个文件、+75/-2(仅测试):两个新用例 + 一个参数化 + 一个 import。
我跑了 pytest tests/test_manager_context_handoff.py tests/test_manager_context_tracking.py tests/test_manager_context_roundtrip.py tests/test_chat_manager_context.py -q → 51 passed;gh pr diff --name-only 确认只有那一个测试文件变化,符合正文「Only tests/test_manager_context_handoff.py changes relative to main」。
关键内容讲解
test_same_goal_recipients_keep_inboxes_and_decisions_separate:先把同一请求投给 worker,断言 peer 收件箱为空、用 worker 的 request id 去 peer 处 acknowledge 会失败;再把同一输入投给 peer,断言两侧 id 不同、各自决策互不串行(query 读出的两行决策分别是adopt/reject)。test_new_request_round_preserves_the_previous_receiver_decision:第一轮 adopt 后,用改过 message 的第二轮再投,断言两轮并存、第一轮receiver_decision_recorded仍为真、第二轮 message 是新的且未决;最后 replay 原始输入必须返回第一轮的 id 且replayed。- 三处 mutation(收件人身份、请求轮次身份、adopted 请求提前移除)由作者报告可被检出,且未改动的实现通过——与用例的断言方向一致。
对主干的风险
测试-only,最强风险是「断言太松、让隔离缺陷溜过」。从实现看主要断言都是关系式的:peer 收件箱投递前为空、两侧 id 不同、query 的两行决策不同、replay 身份保持,因此「一条请求进两个收件箱」这类实现会直接失败。
一条 P3(F1,非阻塞):错误收件人那次 acknowledge 用的是 pytest.raises((OSError, ValueError))。loopx/capabilities/manager_context/__init__.py::acknowledge 的路径是按目标 scope 派生的(entries/_hash(target)/request_id.json),entry 不存在时 _read 抛 OSError,而 ValueError 覆盖 scope 不匹配与非法 id 两类。接受两者意味着这个断言分不清「对方收件箱里没有该 entry」与「id/scope 被拒」——将来若某改动让 entry 存在但 scope 校验失败,测试仍是绿的。隔离缺陷本身仍被卡住(若真写进两个收件箱,调用会成功、pytest.raises 就会失败),所以不阻塞。建议改成断言具体错误(缺失 entry 的 FileNotFoundError,或带消息的 scope mismatch ValueError)。
我的整体评价
结论 APPROVE。这是把 M2 语义连续性所依赖的两条关系(同 Goal 接收者隔离、请求轮次身份保持)钉在既有 owner 里的紧凑回归增量:复用 fixture、用语义断言而非实现细节、参数化不丢覆盖,并明确划出了不做的事。我复跑了 51 个用例并确认 diff 只动一个测试文件。
唯一 P3 是那处过宽的错误元组,非阻塞;修与不修都不影响本次结论。
English verdict: APPROVE - exact head 45340f3; a test-only increment that guards same-Goal receiver isolation and successive request rounds in the existing manager-context owner (51 passed across the four modules, diff limited to tests/test_manager_context_handoff.py), with semantic assertions (distinct request ids, per-receiver decisions, preserved prior decision, replay identity) rather than private hash or layout details. One non-blocking P3: the wrong-recipient acknowledgement expects (OSError, ValueError), which does not pin whether the other inbox lacked the entry or the id/scope was rejected.
|
Post-review note on the failing This PR's own commits are DCO-clean: The failure comes from the range the workflow scans, not from this branch. Suggested fix, verified locally against this head: - name: Require a DCO trailer on every commit
env:
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
BASE_REF: ${{ github.event.pull_request.base.ref }}
run: |
git fetch --no-tags origin "${BASE_REF}"
while IFS= read -r commit; do ... done < <(git rev-list --reverse "FETCH_HEAD..${HEAD_SHA}")
This affects every PR that merges |
|
Correction to one prediction in my note above, after checking the other merged heads. I wrote that "#4630, #4631, #4629, #4643 will show the same false failure once their checks run on the merged head". That did not happen: their
So the defect is not universal per PR: it depends on whether The rest of the note stands: this PR's own two commits both carry valid |
Merge-readiness qualification — #4640 @
|
| 阻塞项 | 触发规则 | 需要的动作 |
|---|---|---|
merge_state_requires_update(BEHIND) |
strict_required_status_checks_policy: true —— head 必须包含 main 的最新提交 |
更新分支(作者 rebase/合并 main,或维护者用 “Update branch”) |
github_review_decision_not_approved(GitHub 显示 REVIEW_REQUIRED,尽管本 head 上有两条 APPROVED) |
require_last_push_approval: true —— 最后推入 commit 的人不能提供计入的审批。该分支最后一个 commit 是 45340f38ba,author/committer 均为 huangruiteng(维护者本人) |
由另一位维护者审批;或由原作者重新推入 head 后再由维护者审批 |
status_checks_failed / status_checks_incomplete |
必需检查 Sign-off 失败(merge-gate 通过) |
见下方 DCO 判定问题 |
关于 Sign-off:在同一 base 上,4630、4631、4629、4643 等已并入的 PR 通过该检查,而本 PR 的失败信息指向的提交范围包含 29 个已存在于 main 的合并提交。原因是 .github/workflows/dco.yml 使用 github.event.pull_request.base.sha 作为范围起点,该值在 base 分支前进后会变旧,于是范围里混入了不属于本 PR 的历史提交。这是间歇性的判定缺陷,不是本 PR 缺少 sign-off;修复它属于 CI 工作流改动,需要维护者决定,我不会在未获授权的情况下改 .github/workflows/**。
本轮做了什么、没做什么
- 读取了该 exact head 的审批状态、检查结论、review thread 摘要,并运行了上面的合并门命令。
- 没有合并,也没有使用 admin bypass —— 门本身返回
admin_bypass_overrides_this_gate=false,且维护流程不允许以旁路替代该门。 - 该 head 的代码评审结论不变;门必须在合并前对未变化的 head 重新运行(
required_timing: immediately_before_merge,head 变动则restart_review_and_rerun_gate)。
一条流程教训(我自己的)
这个 PR 的 head commit 由维护者本人推入,因此 require_last_push_approval 让维护者的审批无法计入。也就是说:在他人的 PR 分支上直接推 commit,会让该 PR 无法由推入者自己的审批合并。后续遇到社区 PR 需要内容调整时,应改为在独立分支上提出并请求作者采纳,而不是替作者推送;本轮已把这条记为本 lane 的操作约束。
English verdict: APPROVE - PR #4640 at head 45340f3; the review conclusion is valid and unchanged at this exact head, but merge readiness is NOT READY (blocking reasons: github_review_decision_not_approved from require_last_push_approval because the last commit was pushed by the maintainer, status_checks_failed on the required Sign-off check that mis-ranges commits via a stale github.event.pull_request.base.sha, and merge_state_requires_update from strict required status checks), with no merge and no bypass performed.
|
该 head 的 |
Goal and delivered outcome
Protect the shipped manager-context exchange while preparing for the semantic continuation direction in the overall roadmap and the manager handoff RFC.
The existing tests cover duplicate ingress, cross-Goal isolation and decision/read tracking. This change adds the missing same-Goal receiver and successive-request cases to that existing owner:
no_changeandadoptleave the request pending until the receiver returns a conclusion.Only
tests/test_manager_context_handoff.pychanges relative tomain. It reuses the existing fixture and actual temporary filesystem inbox APIs. Expected results are semantic relationships, without pinning private hash formulas or directory layouts. There is no separate obligation ledger.Boundary and remaining work
Related to #4311, #4312 and #4330. This is a complete regression-coverage increment for current message delivery. It does not close #4311 or qualify M2 peer dispatch, canonical Todo claims, stale-claim reconciliation, legacy peer-dispatch aliases, process-restart recovery or effect-after-crash behavior. Those remain with the existing Core collaboration/M2 implementation and its actual migration/receiver tests. A context
adoptdecision is not a Todo claim or completed work.No production code, CLI, frontend or Lark behavior changes. Their current shared inbox/return owner is exercised; new product-entry validation is not claimed.
Validation
45340f38ba2c4444971f51c6a8908d4d88cd01bd; basea96c9aa91c9d92454efcd7937483bac76e35fd4a.uv run --extra test python -m pytest -q tests/test_manager_context_handoff.py tests/test_manager_context_tracking.py tests/test_manager_context_roundtrip.py tests/test_chat_manager_context.py: 51 passed.cqr_3c00e9e52169d69c59f8: valid/pass. Risk-selected premerge: 7/7 canaries and 4/4 direct checks passed, with no failures, skips or manual holds.wait_for_ci=false: remote CI is not awaited or used as passing evidence; local validation and exact-head review/readiness remain required.The bounded maintainability pass consolidates coverage in the existing test module; no new helper layer, fixture schema or production owner is introduced.