Skip to content

test(claude): pin the MCP complete_task validation gate coverage - #3343

Open
NIU-123370 wants to merge 2 commits into
huangruiteng:mainfrom
NIU-123370:test/mcp-complete-task-validation-gate
Open

test(claude): pin the MCP complete_task validation gate coverage#3343
NIU-123370 wants to merge 2 commits into
huangruiteng:mainfrom
NIU-123370:test/mcp-complete-task-validation-gate

Conversation

@NIU-123370

Copy link
Copy Markdown
Contributor

GH-C85 / #3082 follow-up. Stacked on #3342 (merge that first; the only dependency is the refresh-state settlement section it adds — the rest is independent).

What this pins

The MCP complete_task route reaches the caller-approved completion gate transitively: it invokes the todo complete CLI, which runs run_completion_validation_gate before the mutation lock (#3142). Nothing pinned that inheritance end to end — a future rewiring of the MCP settlement path could silently drop it.

Coverage (real control plane, real CLI subprocess, no monkeypatched subprocess.run)

case expected
declared command fails (negative case) completion blocked with the typed validation_blocked_completion receipt, spend-slot/refresh-state settlement skipped, todo left open
declared command passes completion commits, no block payload
no declared command fast path unchanged, settlement sections still appended

Verification (per the GH-C85 row)

  • new file tests/test_goal_mode_mcp_completion_validation.py: 3 passed
  • python3 examples/loopx-turn-fake-host-walkthrough-smoke.py: exit 0
  • loopx check --scan-path loopx/capabilities/issue_fix --scan-path CONTRIBUTOR_TASKS.md: public boundary scan clean
  • tests/canary: 20 passed, no baseline change (test-only)

Remaining #3082 entry points (not this PR)

…sh state

Fixes the two huangruiteng#3341 defects in the Claude adapter completion path:

- complete_task built quota spend-slot without --todo-id, so the
  control plane rejected every post-completion settlement and
  spent_slots stayed 0. The binding is order-sensitive: heartbeat
  spends bind to the todo that quota should-run currently reports as
  selected — after a completion that is the next todo, not the one
  just completed — so the id now comes from a fresh should-run
  snapshot, mirroring the interaction contract next_cli_actions
  command. When no todo is selected (or the snapshot is unparseable),
  the spend mirrors the contract fallback and omits the binding
  instead of aborting.
- refresh-state was never executed on this path; complete_task now
  runs refresh-state --goal-id --agent-id after the spend so the
  active-state next action reflects the completion.

Three new regressions fail on the pre-fix head (bound spend, unbound
fallback with no selected todo, unbound fallback on unparseable
snapshot); the updated impersonation test pins the new command order.

Fixes huangruiteng#3341

Signed-off-by: 牛瑞博 <912906590@qq.com>
GH-C85 / huangruiteng#3082 follow-up: the MCP complete_task route reaches the
declared-command completion gate transitively (it invokes the todo
complete CLI, which runs run_completion_validation_gate before the
mutation lock), but nothing pinned that inheritance end to end — a
future rewiring of the MCP settlement path could silently drop it.

Adds focused integration coverage through the real control plane
(real CLI subprocess, no monkeypatched subprocess.run):

- a failing declared command blocks the completion with the typed
  validation_blocked_completion receipt, skips spend-slot and
  refresh-state settlement, and leaves the todo open (negative case)
- a passing declared command commits the completion
- a todo without a declared command keeps the no-command fast path
  and the post-completion settlement sections unchanged

Verification per the task row: the new pytest file (3 passed),
python3 examples/loopx-turn-fake-host-walkthrough-smoke.py, and
loopx check --scan-path loopx/capabilities/issue_fix
--scan-path CONTRIBUTOR_TASKS.md (public boundary scan clean).
canary: 20 passed, no baseline change (test-only).

Signed-off-by: 牛瑞博 <912906590@qq.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

动机

这个 PR 想补的缺口是成立的:MCP complete_task 不是自己实现 validation gate,而是经由真实 todo complete CLI 继承 run_completion_validation_gate;若以后重接 settlement 路径,确实可能把这条机器强制的完成门禁悄悄绕掉。负例使用真实临时 registry/state、真实 CLI 子进程,验证 declared command 失败时返回 typed validation_blocked_completion、Todo 保持 open 且不进入 settlement,这部分是有价值、可长期保留的回归覆盖。

改动思路

当前 exact head f93cd1575a9084fdf4262e4d397ef778973e5782 是叠加 PR:第二个提交新增 3 个端到端测试,第一个提交则完整带入 #3342_settle_completed_turn 运行时改动。正路径实际流程是 todo complete -> quota should-run -> quota spend-slot -> refresh-state;负路径在 todo complete 返回 typed block receipt 后停止。这里 authoritative state 应由同一 turn/Todo 的 durable writeback 与 spend receipt 共同证明,而不能只看到第一段 completion JSON 或只看到输出分隔符就判断链路成功。

具体改动

关键代码讲解

  • test_mcp_complete_task_fails_closed_on_failing_declared_validation:覆盖最重要的反例;检查 typed block receipt、Todo 未完成,以及 spend/refresh 均未触发。
  • test_mcp_complete_task_with_passing_declared_validation_commits:目前只经 _first_json_blob 读取第一段 completion JSON,并检查 Todo 状态;它没有读取或校验后续 settlement receipt。
  • test_mcp_complete_task_without_declared_command_keeps_fast_path:目前只断言 spend-slot/refresh-state 分隔符存在,没有断言两段 JSON 的 okappended、identity 或执行顺序语义。
  • _settle_completed_turn(来自堆叠的 #3342):先按 completion 后的新 selected_todo 执行 spend,再做无 typed delta 的 refresh;snapshot 解析失败时还降级为 unbound spend。这个运行时语义已经在 #3342 的 exact-head review 中被 REQUEST_CHANGES,本 PR 当前 diff 也包含它。

改动规模为 3 个文件、+357/-16;其中第二个提交是 158 行测试,第一提交是 51 行运行时代码加 148 行 mocked tests。活跃生产调用点是 MCP complete_task,所以这不是可按“test-only”忽略的 exact-head 风险。

对主干的风险

[P1 blocker] 绿色测试会接受 settlement 已失败的正路径,因而把当前错误 contract 钉成回归基线。 我在 exact head 上运行 uv run --extra test pytest -q tests/test_goal_mode_mcp_completion_validation.py,结果是 3 passed in 27.40s;随后用同一 fixture 打印 passing-validation 的完整输出:第一段 completion 为 ok: true,但 --- spend-slot --- 明确返回 ok: false(缺少 latest unspent accountable delivery writeback),紧接的 --- refresh-state --- 也返回 ok: false(open autonomous replan obligation 需要 typed semantic delta)。测试之所以仍绿,是 128–141 行只解析第一段 JSON,而 144–158 行只检查分隔符存在。

这意味着 positive/no-command walkthrough 没有验证它们声称保留的 post-completion settlement,远程 7 个成功 checks 也未覆盖这个语义。最小修复是:先让 #3342 修正 settlement authority/order/identity 并 rebase 本 PR;然后把每一段 receipt 解析出来,至少断言 refresh durable writeback 成功、spend 对同一 completed Todo/turn 的 accountable identity 成功,且 malformed/missing authoritative state typed fail closed,而不是 unbound fallback。若本 PR 只想覆盖 validation gate,可保留强负例,但不要用“分隔符存在”替代 settlement 成功。

Review lenses:validation block 使用 typed receipt,合格;文案保持 domain-neutral,合格;门禁被准确描述为 machine-enforced obligation,合格;但当前 exact head 的 runtime behavior 虽在正文标注为 stacked,测试结论仍掩盖了实际失败,因此 behavior-change validation 不合格。

我的整体评价

负向 validation-gate 用例的方向正确,也比 monkeypatched subprocess 覆盖更可信;问题不在测试框架,而在正路径 oracle 丢弃了大部分真实输出。当前 head 不能合入,因为它既包含已被 #3342 review 阻塞的运行时实现,又用会产生假绿的断言保护该实现。请在 #3342 修复后 rebase,并提交能证明完整 settlement receipts 成功的 exact-head 证据;届时我可以按新的 head 重新审。

English verdict: REQUEST_CHANGES — the negative validation-gate test is valuable, but the positive tests pass while both spend-slot and refresh-state receipts are ok: false, so this exact head pins a broken settlement path instead of validating it.

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.

2 participants