test: 6 kunluncode tests leak their subprocess mock into effect_runtime._probe_node and fail as node_unavailable

[Job28703]

Summary

Six tests in tests/test_kunluncode_goal_mode.py deterministically fail on a machine where Node.js is present and new enough, all with EffectRuntimeStartupError: LoopX Effect runtime requires Node.js 22.6.0 or newer (node_unavailable):

• test_mcp_uses_kunluncode_profile_and_rejects_agent_impersonation
• test_fastmcp_complete_task_forwards_complete_task_lease_fence
• test_mcp_spends_only_after_typed_completed_state
• test_complete_task_spends_bound_to_selected_todo_and_refreshes_state
• test_complete_task_classifies_terminal_no_selection_and_fails_closed
• test_complete_task_fails_closed_on_unparseable_snapshot

Root cause

These tests mock the subprocess layer with:

monkeypatch.setattr(goal_mode_mcp.subprocess, "run", capture)

goal_mode_mcp.subprocess and loopx.control_plane.effect_runtime.subprocess are the same stdlib module object, so this assignment patches subprocess.run globally for the whole test process. complete_task then goes through settle_host_todo_completion -> effect_runtime_request -> _start_runtime -> _node_executable -> _probe_node, whose subprocess.run([node, "--version"]) now returns the fake CompletedProcess(command, 0, "not-json", ""). The version regex does not match "not-json", the probe returns probe_failed, and every effect-runtime path raises node_unavailable.

Reproduction (in-process):

import loopx.goal_mode_mcp as gm, loopx.control_plane.effect_runtime as er
print(gm.subprocess is er.subprocess)  # True -- one shared module object
er.subprocess.run = lambda command, **kw: __import__("subprocess").CompletedProcess(command, 0, "not-json", "")
print(er._probe_node())  # ('probe_failed', '/opt/homebrew/bin/node', None)

Observed with node v26.8.1 on PATH: _probe_node() returns ('ready', ...) when called directly, but the same probe inside these tests is poisoned. A/B verified: the same six tests fail identically with and without any other local changes, i.e. this is pre-existing (checked on the #4088 head 012fd40).

Why CI may not have caught it: these tests pass whenever the effect runtime is already up (or the probe result is otherwise bypassed) before the mock is installed -- execution-order dependent.

Suggested fix

Patch a module-local runner instead of the shared subprocess module attribute, e.g. give GoalModeMCPControlPlane an injectable runner (some call sites already have control.command_prefix style seams) and monkeypatch that, so the stdlib module object is never mutated.

Interim mitigation

The six tests can carry @pytest.mark.skip referencing this issue until the runner seam lands.

[Job28703]

A design note on the interim mitigation, in case the maintainers prefer a different shape:

What the companion patches do (current shape): a centralized allowlist — one ALLOWED_SKIPS set in a dedicated contract test (tests/test_kunluncode_goal_mode_skip_allowlist.py) that AST-enumerates the actual pytest.mark.skip/skipif marks in test_kunluncode_goal_mode.py and requires set equality in both directions:

• new skip without an entry → CI red (skip creep),
• stale entry after a skip is removed upstream → CI red (so the allowlist cannot outlive the problem),
• plus every skipped test must keep referencing this issue in its decorator block.

Alternative shape (more distributed): instead of one central list, put the allowlist knowledge in the markers themselves — e.g. @pytest.mark.skip(reason="... see #4151", allow_id="issue-4151") plus a small conftest hook or contract test that only checks "every skip in this file carries an allow_id that maps to an open issue". That scales better if skips spread across many files (no central file to keep editing), but it is weaker in the other direction: nothing forces cleanup when the issue closes unless someone enumerates open issues, and there is no single place to see the whole known-skip inventory.

Why the centralized version was chosen for now: exactly six known skips in exactly one file; a two-sided equality contract is stronger than a per-marker attribute check (it fails on both drift directions, deterministically, with the diff in the assertion message). If skips of this kind spread to more files later, the per-marker allow_id shape is the natural evolution — the contract test could then consume marks instead of AST.

Both companion patches (skip marks referencing this issue + the two-sided allowlist contract) are available for review; happy to rework to the distributed shape if preferred.