test: add subprocess-level transport layer tests

[nnennandukwe]

Summary

• Adds 13 subprocess-level tests that spawn real server processes over stdio pipes, closing the transport layer testing gap where in-process BytesIO tests can't exercise real pipe behavior (buffering, encoding, EOF handling)
• Introduces MCPSubprocessClient helper class with thread-based pipe I/O that avoids the select()+BufferedReader mismatch
• Registers @pytest.mark.subprocess marker for selective execution (poetry run pytest -m subprocess)

What's Tested

Test Class	Tests	Coverage
TestMultiMessageSession	3	serve() loop, notifications, sequential tool calls
TestErrorRecovery	2	Server continues after malformed JSON / unknown methods
TestEntryPointLifecycle	4	sync_main(), SIGINT, EOF shutdown, stderr logging
TestEdgeCases	4	>80KB payloads, multi-byte UTF-8, Content-Length: 0, stdout purity

Test plan

• poetry run pytest tests/test_subprocess_transport.py -v — 13 passed in 1.16s
• poetry run pytest — 135 total tests pass (122 existing + 13 new) in 1.9s
• poetry run ruff check tests/test_subprocess_transport.py — all checks passed
• Pre-commit hooks pass (ruff lint, ruff format, detect-secrets, trailing whitespace)

🤖 Generated with Claude Code

[qodo-code-review]

Review Summary by Qodo

Add subprocess-level transport layer tests with real pipe I/O

🧪 Tests

Walkthroughs

Description

• Adds 13 subprocess-level transport layer tests with real server processes over stdio pipes
• Introduces MCPSubprocessClient helper class for incremental pipe I/O with thread-based reading
• Tests cover multi-message sessions, error recovery, lifecycle, and edge cases (large payloads,
  UTF-8, framing)
• Registers @pytest.mark.subprocess marker for selective test execution

Diagram

flowchart LR
  A["Test Infrastructure"] -->|MCPSubprocessClient| B["Multi-Message Sessions"]
  A -->|Thread-based Reader| C["Error Recovery"]
  A -->|Content-Length Framing| D["Lifecycle Tests"]
  B -->|serve() loop| E["13 New Tests"]
  C -->|Error Handling| E
  D -->|sync_main Entry Point| E
  F["Edge Cases"] -->|Large Payloads UTF-8| E

Loading

File Changes

1. tests/test_subprocess_transport.py 🧪 Tests +511/-0

New subprocess transport layer tests with real pipe I/O

• New 511-line test file with MCPSubprocessClient helper class for subprocess-level transport
 testing
• Implements background reader thread using os.read() on raw file descriptors to avoid
 BufferedReader buffering issues
• Adds 13 tests across 4 test classes: TestMultiMessageSession (3 tests), TestErrorRecovery (2
 tests), TestEntryPointLifecycle (4 tests), TestEdgeCases (4 tests)
• Tests cover multi-message sessions, error recovery mid-session, sync_main() lifecycle, SIGINT
 handling, large payloads (>80KB), multi-byte UTF-8, and stdout purity verification

tests/test_subprocess_transport.py

---

2. docs/plans/2026-02-10-feat-subprocess-transport-layer-tests-plan.md 📝 Documentation +311/-0

Planning document for subprocess transport tests

• New 311-line planning document detailing the subprocess transport layer testing strategy
• Documents problem statement: BytesIO-based tests hide real pipe behavior (buffering, EOF,
 encoding)
• Outlines 6-phase technical approach with MCPSubprocessClient design, test scenarios, and
 acceptance criteria
• Includes risk analysis and references to relevant server code paths

docs/plans/2026-02-10-feat-subprocess-transport-layer-tests-plan.md

---

3. pyproject.toml ⚙️ Configuration changes +3/-0

Register pytest subprocess marker

• Adds subprocess pytest marker definition to [tool.pytest.ini_options] section
• Enables selective test execution via poetry run pytest -m subprocess

pyproject.toml

---

---

🛠️ Relevant configurations:

These are the relevant configurations for this tool:

[config]

is_auto_command: True
is_new_pr: True
model_reasoning: vertex_ai/gemini-2.5-pro
model: gpt-5.2-2025-12-11
model_turbo: anthropic/claude-haiku-4-5-20251001
fallback_models: ['anthropic/claude-sonnet-4-5-20250929', 'bedrock/us.anthropic.claude-sonnet-4-5-20250929-v1:0']
second_model_for_exhaustive_mode: o4-mini
git_provider: github
publish_output: True
publish_output_no_suggestions: True
publish_output_progress: True
verbosity_level: 0
publish_logs: False
debug_mode: False
use_wiki_settings_file: True
use_repo_settings_file: True
use_global_settings_file: True
use_global_wiki_settings_file: False
disable_auto_feedback: False
ai_timeout: 150
response_language: en-US
clone_repo_instead_of_fetch: True
always_clone: False
add_repo_metadata: True
clone_repo_time_limit: 300
publish_inline_comments_fallback_batch_size: 5
publish_inline_comments_fallback_sleep_time: 2
max_model_tokens: 32000
custom_model_max_tokens: -1
patch_extension_skip_types: ['.md', '.txt']
extra_allowed_extensions: []
allow_dynamic_context: True
allow_forward_dynamic_context: True
max_extra_lines_before_dynamic_context: 12
patch_extra_lines_before: 5
patch_extra_lines_after: 1
ai_handler: litellm
cli_mode: False
TRIAL_GIT_ORG_MAX_INVOKES_PER_MONTH: 30
TRIAL_RATIO_CLOSE_TO_LIMIT: 0.8
invite_only_mode: False
enable_request_access_msg_on_new_pr: False
check_also_invites_field: False
allowed_users: []
calculate_context: True
disable_checkboxes: False
output_relevant_configurations: True
large_patch_policy: clip
seed: -1
temperature: 0.2
allow_dynamic_context_ab_testing: False
choose_dynamic_context_ab_testing_ratio: 0.5
ignore_pr_title: ['^\\[Auto\\]', '^Auto']
ignore_pr_target_branches: []
ignore_pr_source_branches: []
ignore_pr_labels: []
ignore_ticket_labels: []
allow_only_specific_folders: []
ignore_pr_authors: []
ignore_repositories: []
ignore_language_framework: []
enable_ai_metadata: True
present_reasoning: True
max_tickets: 10
max_tickets_chars: 8000
prevent_any_approval: False
enable_comment_approval: False
enable_auto_approval: False
auto_approve_for_low_review_effort: -1
auto_approve_for_no_suggestions: False
ensure_ticket_compliance: False
new_diff_format: True
new_diff_format_add_external_references: True
tasks_queue_ttl_from_dequeue_in_seconds: 900
enable_custom_labels: False

[pr_description]

publish_labels: False
add_original_user_description: True
generate_ai_title: False
extra_instructions: 
enable_pr_type: True
final_update_message: True
enable_help_text: False
enable_help_comment: False
bring_latest_tag: False
enable_pr_diagram: True
publish_description_as_comment: False
publish_description_as_comment_persistent: True
enable_semantic_files_types: True
collapsible_file_list: adaptive
collapsible_file_list_threshold: 8
inline_file_summary: False
use_description_markers: False
include_generated_by_header: True
enable_large_pr_handling: True
max_ai_calls: 4
auto_create_ticket: False

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Short-read breaks large frames 🐞 Bug ⛯ Reliability

Description

The server reads the request body with a single stdin.read(content_length) and treats any short
read as EOF, which can terminate the session mid-message. The new >80KB subprocess test increases
the likelihood of triggering this, making CI failures/flakiness likely and indicating a real
production transport bug.

Code

tests/test_subprocess_transport.py[R433-449]

+    def test_large_payload_exceeding_pipe_buffer(self, client: MCPSubprocessClient) -> None:
+        """Source code >80KB exercises real pipe buffering (pipe buffer ~64KB)."""
+        client.send(_make_initialize_request(1))
+        client.receive()
+
+        # Generate >80KB of valid Python source code
+        lines = []
+        for i in range(2000):
+            lines.append(f"variable_{i} = 'value_{i}' * 10  # padding line {i}")
+        large_source = "\n".join(lines) + "\n"
+        assert len(large_source.encode("utf-8")) > 80_000
+
+        client.send(_make_performance_check_request(large_source, request_id=2))
+        resp = client.receive()
+        assert resp["id"] == 2
+        assert "result" in resp
+        assert resp["result"]["content"][0]["json"]["success"] is True

Evidence

The new test intentionally sends a body >80KB over a real pipe. Server-side framing currently
assumes read(n) returns exactly n bytes; if it returns fewer bytes for any reason, the server
returns None (EOF) and stops the serve loop, corrupting multi-message sessions and causing the
client to hit EOF/timeouts.

tests/test_subprocess_transport.py[433-449]
src/workshop_mcp/server.py[117-150]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`WorkshopMCPServer._read_message()` performs a single `stdin.read(content_length)` and treats any short read as EOF. This can break large requests over real pipes and will likely make the new large-payload subprocess test flaky/fail.
### Issue Context
The new subprocess tests send >80KB requests over stdio pipes and expect the server to correctly frame and parse them.
### Fix Focus Areas
- src/workshop_mcp/server.py[117-150]
- tests/test_subprocess_transport.py[433-449]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Captured stderr can deadlock 🐞 Bug ⛯ Reliability

Description

The subprocess client captures stderr=PIPE but never drains stderr until after the process exits;
if the server logs enough, the child can block on stderr writes and tests can hang indefinitely.
This is a classic subprocess pipe deadlock scenario.

Code

tests/test_subprocess_transport.py[R51-63]

+    def start(self) -> None:
+        """Spawn server subprocess with stdin/stdout/stderr pipes."""
+        self.proc = subprocess.Popen(
+            self.invocation,
+            stdin=subprocess.PIPE,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            cwd=str(PROJECT_ROOT),
+        )
+        # Start background thread reading raw stdout fd
+        self._reader_thread = threading.Thread(target=self._stdout_reader, daemon=True)
+        self._reader_thread.start()
+

Evidence

The test harness captures stderr into a pipe, but only reads it at the end via stderr_output.
Meanwhile the server is explicitly configured to log to stderr, meaning stderr writes happen during
the session and can fill the pipe buffer if not drained.

tests/test_subprocess_transport.py[51-63]
tests/test_subprocess_transport.py[126-131]
src/workshop_mcp/server.py[21-26]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`MCPSubprocessClient` starts the server with `stderr=subprocess.PIPE` but does not read from stderr until after `wait()`. If the server logs enough, stderr can fill and block the subprocess, hanging tests.
### Issue Context
Server logging is configured to stream to `sys.stderr`. Subprocess tests spawn many servers and assert on stderr content in at least one test.
### Fix Focus Areas
- tests/test_subprocess_transport.py[51-63]
- tests/test_subprocess_transport.py[126-131]
- src/workshop_mcp/server.py[21-26]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

✅ 3. Reader-thread sync is implicit 🐞 Bug ⛯ Reliability

Description

remaining_stdout() drains stdout using get_nowait() without explicitly waiting for the stdout
reader thread to finish, which makes the “extraneous stdout” assertion more race-prone than
necessary. Joining the reader thread (or waiting for EOF sentinel) after process exit would make the
harness more deterministic.

Code

tests/test_subprocess_transport.py[R118-145]

+    def close(self) -> int:
+        """Close stdin, wait for exit, return exit code."""
+        assert self.proc is not None
+        if self.proc.stdin and not self.proc.stdin.closed:
+            self.proc.stdin.close()
+        self.proc.wait(timeout=self.timeout)
+        return self.proc.returncode
+
+    @property
+    def stderr_output(self) -> str:
+        """Read all captured stderr after process exits."""
+        assert self.proc is not None and self.proc.stderr is not None
+        return self.proc.stderr.read().decode("utf-8", errors="replace")
+
+    def remaining_stdout(self) -> bytes:
+        """Drain any remaining stdout data after process exits."""
+        # Drain the queue
+        remaining = self._buffer
+        while True:
+            try:
+                chunk = self._stdout_chunks.get_nowait()
+                if chunk is None:
+                    break
+                remaining += chunk
+            except queue.Empty:
+                break
+        self._buffer = b""
+        return remaining

Evidence

The harness uses a daemon stdout reader thread but close() doesn’t join it, and
remaining_stdout() drains the queue non-blocking. Even though proc.wait() helps, explicit
synchronization would prevent edge races and make failures easier to reason about.

tests/test_subprocess_transport.py[118-125]
tests/test_subprocess_transport.py[132-145]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`remaining_stdout()` drains the stdout queue without explicitly waiting for the stdout reader thread to finish, which can make extraneous-stdout detection more race-prone than necessary.
### Issue Context
A background daemon thread reads from the stdout fd and pushes chunks into a queue.
### Fix Focus Areas
- tests/test_subprocess_transport.py[60-77]
- tests/test_subprocess_transport.py[118-125]
- tests/test_subprocess_transport.py[132-145]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

✅ 4. UTF-8 on-wire coverage weak 🐞 Bug ✓ Correctness

Description

The “multibyte UTF-8” test validates Unicode round-tripping at the JSON level, but send() uses
json.dumps() defaults, which typically escape non-ASCII characters; this reduces coverage of true
multibyte UTF-8 bytes on the wire (and thus decode-boundary issues).

Code

tests/test_subprocess_transport.py[R79-85]

+    def send(self, request: dict[str, Any]) -> None:
+        """Frame and write a JSON-RPC message to server stdin."""
+        assert self.proc is not None and self.proc.stdin is not None
+        body = json.dumps(request).encode("utf-8")
+        header = f"Content-Length: {len(body)}\r\n\r\n".encode()
+        self.proc.stdin.write(header + body)
+        self.proc.stdin.flush()

Evidence

The test constructs non-ASCII source code, but the client serializes JSON with default settings. If
non-ASCII is escaped, the request bytes become ASCII-only, limiting the test’s ability to catch
UTF-8 boundary/decoding issues in real transport payloads.

tests/test_subprocess_transport.py[79-85]
tests/test_subprocess_transport.py[453-465]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The multibyte UTF-8 test can be strengthened: using default `json.dumps()` may escape non-ASCII, reducing coverage of real multibyte UTF-8 bytes over the pipe.
### Issue Context
Server decodes the raw body as UTF-8. If requests are ASCII-only due to escaping, some decode-boundary failure modes won’t be exercised.
### Fix Focus Areas
- tests/test_subprocess_transport.py[79-85]
- tests/test_subprocess_transport.py[453-469]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-code-review]

1. Short-read breaks large frames 🐞 Bug ⛯ Reliability

The server reads the request body with a single stdin.read(content_length) and treats any short
read as EOF, which can terminate the session mid-message. The new >80KB subprocess test increases
the likelihood of triggering this, making CI failures/flakiness likely and indicating a real
production transport bug.

Agent Prompt

### Issue description
`WorkshopMCPServer._read_message()` performs a single `stdin.read(content_length)` and treats any short read as EOF. This can break large requests over real pipes and will likely make the new large-payload subprocess test flaky/fail.

### Issue Context
The new subprocess tests send >80KB requests over stdio pipes and expect the server to correctly frame and parse them.

### Fix Focus Areas
- src/workshop_mcp/server.py[117-150]
- tests/test_subprocess_transport.py[433-449]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[nnennandukwe]

✅ Fixed — Server now loops stdin.read() until all content_length bytes are received, preventing short-read EOF on large pipe payloads.

[qodo-code-review]

2. Captured stderr can deadlock 🐞 Bug ⛯ Reliability

The subprocess client captures stderr=PIPE but never drains stderr until after the process exits;
if the server logs enough, the child can block on stderr writes and tests can hang indefinitely.
This is a classic subprocess pipe deadlock scenario.

Agent Prompt

### Issue description
`MCPSubprocessClient` starts the server with `stderr=subprocess.PIPE` but does not read from stderr until after `wait()`. If the server logs enough, stderr can fill and block the subprocess, hanging tests.

### Issue Context
Server logging is configured to stream to `sys.stderr`. Subprocess tests spawn many servers and assert on stderr content in at least one test.

### Fix Focus Areas
- tests/test_subprocess_transport.py[51-63]
- tests/test_subprocess_transport.py[126-131]
- src/workshop_mcp/server.py[21-26]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[nnennandukwe]

✅ Fixed — Added background stderr reader thread to drain stderr concurrently, preventing pipe deadlock when server logs fill the buffer.

[nnennandukwe]

Qodo Fix Summary

Reviewed and addressed all 4 Qodo review issues:

✅ Fixed Issues

• Short-read breaks large frames (🔴 CRITICAL) - Server now loops stdin.read() until all content_length bytes are received, preventing EOF on partial pipe reads for large payloads (>64KB)
• Captured stderr can deadlock (🟠 HIGH) - Added background stderr reader thread to drain stderr concurrently, preventing classic subprocess pipe deadlock
• Reader-thread sync is implicit (🟡 MEDIUM) - Join stdout/stderr reader threads in close() after process exits for deterministic drain behavior
• UTF-8 on-wire coverage weak (⚪ LOW) - Use ensure_ascii=False in json.dumps() so non-ASCII characters produce real multibyte UTF-8 bytes on the pipe

⏭️ Deferred Issues

None — all issues were fixed.

✅ All Tests Pass

• 13/13 subprocess transport tests pass
• 22/22 server protocol + integration tests pass

---

Generated by Qodo PR Resolver skill