Pre-release fixes: reuse the MCP client across calls; error bodies on streaming calls

[h3xxit]

Summary

Follow-ups from a pre-release review of dev (#103), done before merging to main. Two batches.

Batch 1

• MCP client was rebuilt on every call (pre-existing leak). _ensure_mcp_client compared the client's whole config dict with just the mcpServers mapping, which is always unequal, so every tool call created a new MCPClient and spawned a fresh server process that was never closed. Fixed; when the configuration really changes the previous client's sessions are closed first. Test asserts one client and one session across two calls.
• Error bodies on streaming calls: the SSE and Streamable HTTP streaming paths now raise through raise_for_status_with_body, like their discovery paths.

Batch 2

• SSE spec conformance: an event block without an event: field has the type message, so event_type="message" now matches it; an empty id: resets the last event ID and no Last-Event-ID header is sent for it; ids containing NUL are ignored; a 200 that is not text/event-stream raises SseProtocolError instead of parsing into zero events.
• No re-executed POSTs: calls that send a request body are never reconnected, since a re-issued POST could re-execute a non-idempotent tool.
• _errors robustness: a body nested deeper than the JSON parser's recursion limit escaped as RecursionError; it is now treated as text. Control characters are collapsed so server text cannot forge log records or terminal escapes.
• MCP cleanup (pre-existing): close() referenced a _session_locks attribute that was never assigned and always raised AttributeError after cleaning up. A child that starts but fails the MCP handshake is now disconnected and removed from active_sessions instead of lingering.

Batch 3 (cubic's findings on #103 that were not already covered)

• retry: is honoured only when made of ASCII digits; a stream that ends mid-event no longer dispatches the incomplete event (both per spec).
• Streamable HTTP matches Content-Type case-insensitively; stale UDP comment removed; the slow-handshake test handler no longer outlives its test.

Batch 4 (cubic's findings on #104)

• One MCP client per configuration. The protocol object is registered once per process and shared by every manual, so a single shared client made manuals with different configurations evict each other's sessions, including ones in use by a concurrent call. Clients are now keyed by the canonical mcpServers JSON, created under a lock, and never evicted by another manual's activity. close() keeps a client whose shutdown failed so it can be retried.
• The error-body read on a refused SSE stream is bounded by the handshake timeout; a final blank line ending in a lone CR still completes the last event; huge retry digit strings are ignored; the sanitizer covers C1 controls.
• Tests: streaming calls against a 5xx surface the body for SSE and Streamable HTTP; separate clients per configuration.

Batches 5 to 7 (cubic's findings on this PR)

• A manual whose configuration changes releases its previous client when nothing else uses it; deregistering a manual releases only its own claim, so two manuals sharing a configuration keep their client until both are gone; ownership is keyed by the calling UtcpClient plus the manual name, since the protocol object is a process-wide singleton.
• The SSE event cap applies to the residual buffer at end of stream; retry digit strings are bounded before conversion.
• Tests: connection-drop handlers wait 100 ms before closing the socket (on Windows CI the event and the close otherwise arrived together); streaming error-body tests use a body distinct from the reason phrase.

Behavior changes worth noting for the release notes: ClientResponseError.message on failed HTTP calls is now <reason>: <detail> rather than the bare reason phrase; structuredContent handling in MCP changed (a tool without structured output no longer returns None, and a single-key {"result": {...}} object is no longer unwrapped).

Test plan

• pytest over the http plugin tests and the MCP stdio tests: 244 passed, then 251 across http and MCP stdio after batch 6

🤖 Generated with Claude Code

[cubic-dev-ai]

5 issues found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="plugins/communication_protocols/http/src/utcp_http/streamable_http_communication_protocol.py">

<violation number="1" location="plugins/communication_protocols/http/src/utcp_http/streamable_http_communication_protocol.py:296">
P3: This change's whole purpose is surfacing the server's error body on refused streaming calls, but there is no test exercising call_tool_streaming against a 4xx/5xx response. The discovery path has test_register_manual_surfaces_server_error_body, so add an equivalent that hits a 4xx stream endpoint and asserts the raised ClientResponseError message (or call_tool's raised error) contains the server's reason/detail.</violation>
</file>

<file name="plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py">

<violation number="1" location="plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py:114">
P1: When different MCP configurations are used concurrently, this shared client closes the first call's active session before that call finishes, causing in-flight operations to fail. Serialize client replacement with active calls or keep separate clients per configuration instead of closing the shared client during another call.</violation>

<violation number="2" location="plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py:114">
P2: The config-change path awaits close_all_sessions and then reassigns self._mcp_client without any lock. The README advertises concurrent calls, so two calls that both observe a config change can both close the old client and both build a new _QuietStdioMCPClient.from_dict(...); the one assigned last survives and the other's freshly spawned server process is never closed - the exact leak this PR is meant to fix. Guard the ensure/create sequence with an asyncio.Lock so only one call rebuilds the client.</violation>

<violation number="3" location="plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py:115">
P2: When `close_all_sessions()` fails, this handler still replaces the only reference to the previous client, so any session it did not close can leak indefinitely. Preserve the old client for cleanup or fail the reconfiguration instead of discarding it after a shutdown error.</violation>
</file>

<file name="plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py">

<violation number="1" location="plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py:295">
P2: In `call_tool_streaming`, `asyncio.wait_for(..., HANDSHAKE_TIMEOUT_SECONDS)` bounds only `session.request`; the new `raise_for_status_with_body` body read runs outside it. A server that answers the handshake with a 4xx/5xx but never sends the body and keeps the connection open now blocks in `iter_chunked` (bounded only by the session's default 5-min total timeout), where `raise_for_status()` previously raised immediately. Since the handshake is intentionally bounded to prevent a silent server hanging the call, bound the error-body read too — e.g. wrap `raise_for_status_with_body` in `asyncio.wait_for(...)` with a short timeout, or pass a read timeout — so a refused stream still fails fast.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P3: This change's whole purpose is surfacing the server's error body on refused streaming calls, but there is no test exercising call_tool_streaming against a 4xx/5xx response. The discovery path has test_register_manual_surfaces_server_error_body, so add an equivalent that hits a 4xx stream endpoint and asserts the raised ClientResponseError message (or call_tool's raised error) contains the server's reason/detail.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At plugins/communication_protocols/http/src/utcp_http/streamable_http_communication_protocol.py, line 296:

<comment>This change's whole purpose is surfacing the server's error body on refused streaming calls, but there is no test exercising call_tool_streaming against a 4xx/5xx response. The discovery path has test_register_manual_surfaces_server_error_body, so add an equivalent that hits a 4xx stream endpoint and asserts the raised ClientResponseError message (or call_tool's raised error) contains the server's reason/detail.</comment>

<file context>
@@ -293,7 +293,7 @@ async def call_tool_streaming(self, caller, tool_name: str, tool_args: Dict[str,
                     f"call template to point at the final URL directly."
                 )
-            response.raise_for_status()
+            await raise_for_status_with_body(response)
 
             async for chunk in self._process_http_stream(response, tool_call_template.chunk_size, tool_call_template.name):
</file context>

[h3xxit]

Valid, fixed: tests added for both SSE and Streamable HTTP streaming calls against a 5xx, asserting the body is in the message.

[cubic-dev-ai]

1 issue found across 6 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="plugins/communication_protocols/http/src/utcp_http/_errors.py">

<violation number="1" location="plugins/communication_protocols/http/src/utcp_http/_errors.py:30">
P2: Escaped C1 controls such as `\u009b` and `\u009c` survive `_clean`, so C1-aware log or terminal consumers can still interpret server-controlled control sequences. Include the C1 range in the sanitizer.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

3 issues found across 4 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py">

<violation number="1" location="plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py:397">
P2: When an SSE `retry` field contains more than 4,300 ASCII digits, `int(value)` raises `ValueError` and aborts the stream instead of ignoring the unusable value. Keep the conversion inside a `ValueError` guard.</violation>

<violation number="2" location="plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py:435">
P2: A complete, spec-valid event whose closing blank line ends in a lone CR is now dropped at end-of-stream. `normalise` holds a trailing `\r` in `pending_cr` (so a CRLF split across chunks isn't parsed early), and a lone CR is a valid SSE line terminator that can form the event's blank line (e.g. the stream `data: x\n\r`). The removed EOF flush completed it via `if pending_cr: buffer += "\n"` then flushed; the new code unconditionally discards the buffer, so only genuinely incomplete events should be dropped, not CR-terminated ones.</violation>
</file>

<file name="plugins/communication_protocols/http/tests/test_sse_communication_protocol.py">

<violation number="1" location="plugins/communication_protocols/http/tests/test_sse_communication_protocol.py:688">
P3: The test title claims malformed retry is ignored, but the call_template has no reconnect, so the retry parsing result is never observable: the same [{"seq": 1}] passes whether `retry: -1`/`retry: 20ms` is honored or ignored. Only the unterminated-trailing-event behavior is actually verified. Either enable reconnect with an assertion that a server-sent malformed retry does not delay the reconnect (mirroring test_reconnect_delay_is_capped), or rename the test to reflect what it verifies.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

1 issue found across 6 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py">

<violation number="1" location="plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py:405">
P2: On Python 3.10, `int(value)` accepts arbitrarily long digit strings, so the new `ValueError` guard does not bound this conversion. A server can send a delimiter-terminated `retry:` field containing up to the 16 MiB event limit and block the event loop before the later reconnect-delay cap runs. Bound the digit length before calling `int()` (values above the effective delay cap can be ignored or treated as capped).</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: On Python 3.10, int(value) accepts arbitrarily long digit strings, so the new ValueError guard does not bound this conversion. A server can send a delimiter-terminated retry: field containing up to the 16 MiB event limit and block the event loop before the later reconnect-delay cap runs. Bound the digit length before calling int() (values above the effective delay cap can be ignored or treated as capped).

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py, line 405:

<comment>On Python 3.10, `int(value)` accepts arbitrarily long digit strings, so the new `ValueError` guard does not bound this conversion. A server can send a delimiter-terminated `retry:` field containing up to the 16 MiB event limit and block the event loop before the later reconnect-delay cap runs. Bound the digit length before calling `int()` (values above the effective delay cap can be ignored or treated as capped).</comment>

<file context>
@@ -397,8 +399,12 @@ def flush(event_string: str):
                     if value.isascii() and value.isdigit():
-                        current_event['retry'] = int(value)
+                        try:
+                            current_event['retry'] = int(value)
+                        except ValueError:
+                            pass
</file context>

Suggested change

	current_event['retry'] = int(value)
	if len(value) <= len(str(self.MAX_RECONNECT_DELAY_MS)):
	current_event['retry'] = int(value)

[h3xxit]

Valid, fixed: the digit string is bounded to 18 characters before conversion, which keeps it cheap on any interpreter and still covers every value the delay cap could honour.

[cubic-dev-ai]

1 issue found across 6 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="plugins/communication_protocols/http/tests/test_streamable_http_communication_protocol.py">

<violation number="1" location="plugins/communication_protocols/http/tests/test_streamable_http_communication_protocol.py:370">
P2: This test cannot detect the fix it claims to verify. The /error endpoint body ("Internal Server Error") equals the standard HTTP reason phrase for status 500, so `excinfo.value.message` contains that string whether or not the server body is surfaced: plain raise_for_status() yields message "Internal Server Error" and raise_for_status_with_body yields "Internal Server Error: Internal Server Error". Make the body distinct from the reason (e.g. return `web.Response(status=500, text="streaming refused: backend down")`) and assert on that distinct string, so the test actually proves the streaming path surfaces the body.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: This test cannot detect the fix it claims to verify. The /error endpoint body ("Internal Server Error") equals the standard HTTP reason phrase for status 500, so excinfo.value.message contains that string whether or not the server body is surfaced: plain raise_for_status() yields message "Internal Server Error" and raise_for_status_with_body yields "Internal Server Error: Internal Server Error". Make the body distinct from the reason (e.g. return web.Response(status=500, text="streaming refused: backend down")) and assert on that distinct string, so the test actually proves the streaming path surfaces the body.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At plugins/communication_protocols/http/tests/test_streamable_http_communication_protocol.py, line 370:

<comment>This test cannot detect the fix it claims to verify. The /error endpoint body ("Internal Server Error") equals the standard HTTP reason phrase for status 500, so `excinfo.value.message` contains that string whether or not the server body is surfaced: plain raise_for_status() yields message "Internal Server Error" and raise_for_status_with_body yields "Internal Server Error: Internal Server Error". Make the body distinct from the reason (e.g. return `web.Response(status=500, text="streaming refused: backend down")`) and assert on that distinct string, so the test actually proves the streaming path surfaces the body.</comment>

<file context>
@@ -356,3 +356,15 @@ async def test_register_manual_surfaces_server_error_body(streamable_http_transp
+        async for _ in streamable_http_transport.call_tool_streaming(None, "test-provider.t", {}, call_template):
+            pass
+    assert excinfo.value.status == 500
+    assert "Internal Server Error" in excinfo.value.message
</file context>

[h3xxit]

Valid, fixed: both streaming error-body tests now hit a 503 whose body is distinct from the reason phrase and assert on that string.

[cubic-dev-ai]

All reported issues were addressed across 5 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>