fix(ci): close docker_smoke.sh's stdin-before-drain race

[cdeust]

Summary

scripts/docker_smoke.sh drove the bare-container gate with
printf '%s' "$REQUESTS" | docker run --rm -i ..., which closes the
container's stdin the instant the batch is written — before any
response has been read. That is the identical anti-pattern PR #331
fixed for a local subprocess in scripts/mcp_host_client.py: closing
stdin is the MCP shutdown signal (2025-06-18 §Lifecycle › Shutdown ›
stdio), not an end-of-input marker, and mcp 2.0.0's _handle_request
drops an in-flight response write rather than deliver it once EOF
fires the cancel scope. Verified against jsonrpc_dispatcher.py that
this is method-agnostic — tools/list (id=3) is exactly as vulnerable
as any other request, matching this gate's signature failure ("no
valid tools/list response (id=3)", empty stderr, no JSON-RPC error
frame) and its history of intermittent failures (main at 12:29 today,
PRs #423/#424/#425).

• New scripts/docker_smoke_client.py drives the container via
  mcp_host_client.drain_exchange (a new generic primitive extracted
  from _exchange, behavior-preserving) — stdin stays open until both
  expected response ids arrive, closes only then. docker_smoke.sh
  now just builds the image and delegates the run+exchange+assert
  sequence to it.
• The watchdog still docker kills the container via --cidfile on
  the same 60s deadline; fires at most once, never retries — no
  sleep, no wall-clock verdict, no retry loop anywhere in this diff.

Test plan

• Deterministic reproduction, no clock/docker/retries:
  tests_py/infrastructure/test_stdio_eof_drain.py::TestDockerSmokeToolsListLostBeforeDrain
  drives the real mcp 2.0.0 SDK with docker_smoke's own id=3
  tools/list request — the write-then-close-before-drain shape drops
  the response (reproducing the gate's literal historical failure);
  the drain-then-close shape does not.
• tests_py/scripts/test_docker_smoke_client.py pins that the new
  client never closes stdin before both expected ids are read.
• pytest tests_py/scripts/ tests_py/infrastructure/ — 1448
  passed, 35 skipped, 0 failed.
• ruff check / ruff format --check — clean, repo-wide.
• python scripts/check_craftsmanship.py — OK.
• shellcheck scripts/docker_smoke.sh — clean.
• Corroborating measurement (not the gate — the race is
  probabilistic, so replay count alone proves nothing): rebuilt the
  production image locally and ran scripts/docker_smoke.sh --skip-build 20/20 times successfully against the fix.

[cdeust]

Rebased onto origin/main (was 4 commits behind — same cause as #423-425) and re-ran CI's exact invocation, python3 scripts/check_craftsmanship.py --base origin/main. It caught three real, diff-local violations that plain check_craftsmanship.py (working-tree baseline) did not surface: a >40-line main() in the new docker_smoke_client.py, an unsourced TOOLS_LIST_ID constant, and test_stdio_eof_drain.py pushed to 318 lines by the prior commit. Fixed all three at the source (method split, # source: comment, test class moved to a sibling file) — no production logic changed, gate now reports OK against origin/main.

Reaffirming for the record: the deterministic reproduction — driving the real mcp 2.0.0 SDK with docker_smoke's own id=3 tools/list request, write-then-close loses the response, drain-then-close does not — is the gate. The 20/20 real docker build + smoke replays are corroboration only, not proof; the race is probabilistic and a clean run streak proves nothing on its own.

[cdeust]

ZETETIC-REVIEW: APPROVE

Summary

Fixes the docker-smoke gate's stdin-close-before-drain race by draining initialize+tools/list via mcp_host_client.drain_exchange before closing the container's stdin, instead of printf | docker run -i. Verified independently against the real diff, not just read: mergeable as-is.

Move 0 — Ledger reconciliation and seen-defect check

• Enumerated every new branch across the diff (docker_smoke.sh arg loop [unchanged], docker_smoke_client.py's _kill_container two branches, run()'s pipe-None guard [marked pragma: no cover], _evaluate's three branches). No unmapped path found; the PR body's test-plan bullets map cleanly onto the reproduction, client-drain pin, and independently-rerun lint/format/craftsmanship checks.
• Seen-defect rationalizations: none found. The PR body explicitly and correctly distinguishes the deterministic reproduction from the 20/20 corroboration runs ("the race is probabilistic, so replay count alone proves nothing") — that is the opposite of a seen-defect bypass; it is the honest evidentiary framing this repo's §14 exists to require.
• No short-circuit. Proceeded to Moves 1–6.

Stakes calibration (Move 7)

• Classification: High — objective criterion: PR is 817 lines changed (641+176), over the 400-line threshold, and it fixes a race in stdio synchronization (async/thread coordination between the drain loop and the watchdog threading.Timer). Full Move 1–6 depth applied.

1 — Deterministic reproduction, verified for real

tests_py/infrastructure/test_docker_smoke_stdio_eof_drain.py (new, split out of test_stdio_eof_drain.py to respect the 300-line file cap) drives the real mcp 2.0.0 MCPServer over real anyio memory streams with docker_smoke_client.py's own three-frame batch and id=3. _GatedWriteStream parks the tools/list write, sets an anyio.Event the instant the write is "started but unlanded," and only then triggers EOF — this is the literal interleaving the SDK's answer_write_started-before-send-checkpoint bug requires, forced by event, not by sleep. test_shutdown_before_drain_drops_tools_list asserts id=3 is not answered under write-then-close; test_drain_then_shutdown_answers_tools_list asserts it is answered under drain-then-close. This is a real refutation test — read both assertions, not just the docstring claim.

The author's own framing in the PR body ("the race is probabilistic, so replay count alone proves nothing") is exactly right and matches the ask: the 20/20 successful local reruns are corroboration, not proof — against a race that fires ~1/5 of the time on main, twenty consecutive clean runs without the fix would still occur under 1% of the time, so it is weak evidence at best, and the PR does not lean on it as the gate.

2 — drain_exchange extraction vs old _exchange, line by line

Read git diff main...pr428 -- scripts/mcp_host_client.py directly. The extraction is a pure parameterization: outstanding = set(expected_request_ids()) → set(expected_ids), stdin.write(frames(client_name)) → stdin.write(frame_text). Every other line of the loop body — the readline/absorb/outstanding -= responses.keys() drain, the stdin.close() placement, the trailing stdout.read() drain after close — is byte-identical. _exchange is retained as a one-line wrapper (return drain_exchange(stdin, stdout, frames(client_name), expected_request_ids())) preserving its original (stdin, stdout, client_name) signature, so tests_py/scripts/test_mcp_host_client.py (PR #331's suite) needed zero changes and still exercises the identical code path through the shared primitive. Behavior-preserving claim holds; this repair does not put PR #331's guarantee at risk.

3 — No wall-clock verdict reintroduced

docker_smoke_client.py::run() starts a threading.Timer(timeout, _kill_container, ...) as a pure watchdog — its only effect is docker kill <cid> (or process.kill() if no container ever started) on the container's lifecycle, never on the pass/fail decision. _evaluate() reads only the responses dict drain_exchange returns (protocol-error frames, then tools/list's presence, then the count floor) — no reference to elapsed time, no TimeoutExpired-as-outcome path anywhere in the diff. Confirmed the old bash script also never checked docker run's exit code (wait "$DOCKER_RUN_PID" 2>/dev/null || true) — this is unchanged semantics from before the PR, not a new gap. Cross-checked: no sleep/time. reference in the actual (non-docstring) logic of any of the three touched scripts.

4 — Craftsmanship-gate split, verified

Ran python scripts/check_craftsmanship.py --base main myself: OK. main was correctly split into _build_parser() / _evaluate() / main(), all comfortably under the 50-line cap (run(), the longest function, spans ~42 lines). TOOLS_LIST_ID = 3 now carries a # source: comment. The new sibling test file test_docker_smoke_stdio_eof_drain.py imports _server/_GatedWriteStream/_collect from test_stdio_eof_drain.py rather than duplicating them (confirmed by reading the import block) — both files now sit under the 300-line file cap (207 and 133 lines respectively) with no test content lost; its two tests still exercise exactly the failure mode its name promises (docker_smoke's literal id=3/tools/list shape, not a generic stand-in).

Also independently ran ruff check and ruff format --check on all four changed Python files, and shellcheck on docker_smoke.sh: all clean, matching the PR's own claims.

5 — TOOLS_LIST_ID = 3 source

Checked against git show main:scripts/docker_smoke.sh's original REQUESTS heredoc: {"jsonrpc":"2.0","id":1,"method":"initialize",...} then a notification (no id) then {"jsonrpc":"2.0","id":3,"method":"tools/list",...}. The comment states the truth.

Layer check (Move 1)

All changes confined to scripts/ (tooling) and tests_py/ — no layer-boundary crossing applicable.

SOLID audit (Move 2)

docker_smoke_client.py is well factored post-split: run/_kill_container/protocol_errors/tool_count/_evaluate/main each have one job. drain_exchange vs _exchange is a clean OCP-friendly extraction (generic primitive + a specialization), not a new conditional branch grafted onto existing logic. No SOLID violations found.

Wiring & contract drift (Move 3)

docker_smoke.sh now execs docker_smoke_client.py — the driver's exit code becomes the script's exit code directly (no shell-level piping left to race). .github/workflows/ci.yml's docker-smoke job still calls scripts/docker_smoke.sh --skip-build unchanged; --skip-build handling is untouched. _exchange's signature is preserved so the pre-existing test_mcp_host_client.py suite is unaffected — no stale caller.

Test adequacy (Move 4)

New execution paths and their coverage:

• write-then-close loses tools/list / drain-then-close does not → covered by the deterministic SDK-level reproduction (item 1).
• drain_exchange's "stdin never closes before the last expected id is read" postcondition, for docker_smoke's own three-frame batch → test_docker_smoke_client.py::test_stdin_stays_open_until_the_tools_list_response_is_read asserts stdin_closed_at_read == [False, False], not just that the function ran.
• Response-arrives-after-EOF-that-preceded-it is lost → test_a_response_arriving_after_eof_that_precedes_it_is_lost.
• tool_count/protocol_errors error cases (missing id=3, non-dict result, non-list tools, error frames on any id) → each has its own asserting test.

Gap (non-blocking, see Issues): _kill_container's two branches (cidfile present-with-content vs absent/client-side-hang fallback) have no test. This is new-shaped code, but it is a faithful port of the identical, also-untested docker kill --cidfile / process-kill-fallback logic that already existed in the old bash script (verified via git show main:scripts/docker_smoke.sh) — same semantics, not new risk, and it only fires in the pathological hang case this PR's actual fix makes far less likely to occur.

Complexity & structure (Move 5)

File sizes: docker_smoke_client.py 271 lines, mcp_host_client.py 262, docker_smoke.sh 76, both split test files 207/133 — all under caps. No function over ~42 lines. No over-engineering smells; drain_exchange extraction is exactly the "3 concrete uses before extracting" pattern in reverse (2 real callers now: _exchange and docker_smoke_client.run), justified by an actual second consumer, not speculative.

Security & hygiene (Move 6)

No shell=True, no eval/exec of untrusted input, subprocess.Popen uses list-form args throughout. Commits are conventional (fix(ci): ...), single logical change (the second commit is a follow-up gate-compliance fix on the same change, not scope creep), co-authored trailer present.

Issues

Blocking

None.

Non-blocking

• scripts/docker_smoke_client.py:103-120 (_kill_container) — no unit test for either branch (cidfile present vs absent). Suggest a mock-based test in the style of the existing _scripted_drain helpers (fake Path/subprocess.run) as a low-cost follow-up; not required for this PR since the behavior is an unchanged port of pre-existing untested watchdog logic.

Hand-offs

None — no root-cause rework, no structural decomposition question, no formal-correctness gap beyond what the deterministic SDK-level test already proves, no security surface, no unaudited performance claim (the 20/20 number is explicitly framed as corroboration, not a benchmark claim).

Verdict

APPROVE.