Reconnect MCP clients only for transport and session failures (#2056)

[Aaronontheweb]

Summary

McpClientManager.IsTransportOrSessionFailure treated every HttpRequestException as a transport failure. An HTTP 500, 429, or 403 on tools/call therefore built a new session. That costs about five extra requests, and it raises the published generation with no config change. A new session cannot repair a server-side application error. A rate-limited server gets hit harder, so the loop feeds itself.

The predicate now reads the typed status. It returns true only when the status is absent or 404.

Closes #2056
Stacked on #2060

Design refs

openspec/changes/mcp-tool-outcome-receipts/design.md, decision D3. Tasks 3.1 and 3.2.

What changed

• IsTransportOrSessionFailure returns true for an HttpRequestException only when StatusCode is null or 404. An absent status means the request got no answer. The Streamable HTTP spec reports an expired session as 404. Every other status is an application error from a server that answered. The IOException, EndOfStreamException, TimeoutException, ObjectDisposedException, and McpException branches do not change.
• LoadAsync owns the MCP prompt-skill path. Its first catch clause accepted only McpException, so an HttpRequestException with an application status would match neither clause and escape to the tool dispatcher. The clause now also accepts HttpRequestException and returns the same failed load result. No new clause and no new type.

The tool path is unchanged apart from the predicate. InvokeSharedAsync still logs one Warning and rethrows. The adapter still turns the exception into the tool result (#2060).

Tests added

src/Netclaw.Daemon.Tests/Mcp/McpClientManagerLifecycleTests.cs:

• ApplicationHttpStatus_ReachesTheCallerWithoutAReconnect (Theory: HTTP 500 and HTTP 429). The exception reaches the caller. A spare client plan is queued and stays unused, so a reconnect would raise CreateCount to 2 and fail the test. The generation stays 1. The test also pins the Warning line that Record MCP tool-call exceptions as failed outcomes (#2055) #2060 added: exactly one entry names test/run, and it carries the (HTTP 500) suffix ((HTTP 429) for the second case). Nothing in the stack asserted that suffix before.
• SessionExpiryStatus_ReconnectsForLaterCalls (HTTP 404). The exception reaches the caller. The manager builds one replacement client and does not replay the call. The wait on the disposal of the first client runs under the test token, so an over-narrowed predicate fails the run instead of hanging it.
• TransportFailure_ReconnectsForLaterCallsAndDoesNotReplay pins the status-less case. It still passes without a change.

src/Netclaw.Daemon.Tests/Mcp/McpPromptSkillTests.cs:

• LoadReturnsFailedResultForAnApplicationHttpStatus. An HTTP 500 on GetPromptAsync returns a failed load result that names the prompt. A spare client plan is queued and stays unused. The result text must match the first catch clause: it contains failed: and it does not contain connection closed, which is the transport clause's text.

Mutation check: with the predicate reverted to "every HttpRequestException is a transport failure", all three tests fail.

The ClientPlan test double gets a GetPromptFailure hook. It mirrors the PromptListFailure hook that is already there.

Gate outputs

Gate	Result
dotnet build src/Netclaw.Daemon/Netclaw.Daemon.csproj	Build succeeded. 0 Warning(s). 0 Error(s).
dotnet test ... --filter "FullyQualifiedName~Mcp"	Passed. Failed 0, Passed 189, Skipped 0.
dotnet test src/Netclaw.Daemon.Tests	Passed. Failed 0, Passed 1045, Skipped 0.
dotnet slopwatch analyze	Scan complete: 0 issue(s) found.
./scripts/Add-FileHeaders.ps1 -Verify	All files have headers.

Adversarial review

The review returned PARTIAL. Four NOW findings are fixed in this pull request:

1. The 500 and 429 tests did not discriminate. With one client plan queued, a reconnect throws inside ControlledMcpClientRuntime.CreateAsync before it counts the client, so CreateCount read 1 either way. Each test now queues a spare plan.
2. The prompt-load test had the same hole. It now queues a spare plan and asserts the text of the first catch clause.
3. Task 3.1 and this body claimed proof that the tests did not give. The spare plans make the claim true, and this body now states how each test discriminates.
4. The 404 test waited on a disposal task with no token. An over-narrowed predicate hung the run instead of failing it. The wait now uses the test token.

DISMISSED: the 404 test near-duplicates the status-less transport test. Both are kept because each maps to one scenario in the spec delta.

[Aaronontheweb]

LGTM

[Aaronontheweb]

LGTM