Skip to content

Record MCP tool-call exceptions as failed outcomes (#2055) - #2060

Merged
Aaronontheweb merged 2 commits into
spec/mcp-tool-outcome-receiptsfrom
fix/mcp-tool-exception-receipts
Aug 26, 2026
Merged

Record MCP tool-call exceptions as failed outcomes (#2055)#2060
Aaronontheweb merged 2 commits into
spec/mcp-tool-outcome-receiptsfrom
fix/mcp-tool-exception-receipts

Conversation

@Aaronontheweb

@Aaronontheweb Aaronontheweb commented Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

An MCP tool call that ends in an exception now produces a failure receipt and one Warning log line.
Before this change the dispatcher recorded such a call as a success, and the daemon log showed no failure.
The model-facing result text does not change.

Closes #2055

Stacked on #2059. Review and merge that pull request first.

Design refs

  • D1: McpToolAdapter completes the receipt from the caught exception.
  • D2: McpClientManager.InvokeSharedAsync rethrows a non-transport exception and logs once.

D3 and D4 stay for the next pull requests in the stack. An HttpRequestException with status 500 is still a
transport failure here, so it still triggers one reconnect. #2056 changes that.

What changed

  • src/Netclaw.Actors/Tools/McpToolAdapter.cs: the catch-all returns the same error string through
    ToolOutcomeResults. An HTTP 401 or 403 gives AccessDenied. An HTTP 404 gives NotFound. Every other
    exception gives TransientFailure. The bound-tool path does not change.
  • src/Netclaw.Daemon/Mcp/McpClientManager.cs: the tool lookup moved out of the try block. One catch logs a
    redacted Warning and rethrows a non-transport exception. The McpException-to-string branch is gone, so the
    adapter is the only converter. A caller cancellation writes no log and starts no reconnect.
  • feeds/skills/.system/files/netclaw-operations/references/diagnostics.md: one new row for the Warning line.
  • feeds/skills/.system/files/netclaw-operations/SKILL.md: version 2.65.1 to 2.65.2.
  • openspec/changes/mcp-tool-outcome-receipts/tasks.md: tasks 2.1, 2.2, 2.3, and 5.2 are complete.

Log template: MCP tool '{Tool}' invocation failed{HttpStatus}, for example
MCP tool 'shortener/search-links' invocation failed (HTTP 500).

Tests added

src/Netclaw.Actors.Tests/Tools/McpToolAdapterTests.cs:

  • HTTP 500 gives a TransientFailure receipt.
  • HTTP 403 gives an AccessDenied receipt.
  • HTTP 404 gives a NotFound receipt.
  • A plain McpException gives a TransientFailure receipt.
  • Each case asserts that the result text names the tool.

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

  • The application MCP failure case now expects the McpException to reach the caller.
  • A thrown tool call writes exactly one Warning that names the server and the tool.
  • A caller-cancelled call writes no Warning.

Gate outputs

  • dotnet build src/Netclaw.Daemon/Netclaw.Daemon.csproj -nologo -v q: 0 warnings, 0 errors.
  • dotnet test src/Netclaw.Actors.Tests: 3615 passed, 0 failed, 3 skipped.
  • dotnet test src/Netclaw.Daemon.Tests: 1041 passed, 0 failed.
  • dotnet slopwatch analyze: 0 issues.
  • pwsh ./scripts/Add-FileHeaders.ps1 -Verify: all files have headers.

Adversarial review

  • NOW, fixed in this pull request: 404 to not_found had no test. The pull request adds one adapter test.
  • NOW, moved to Every HttpRequestException on an MCP tool call forces a full reconnect #2056: the (HTTP 500) suffix in the Warning line is asserted in the pull request for Every HttpRequestException on an MCP tool call forces a full reconnect #2056,
    where an HTTP 500 no longer reconnects.
  • PARK, decided: the Warning reads the HTTP status from the outer exception only. That matches
    IsTransportOrSessionFailure, which does not unwrap. A wrapped status maps to transient_failure.
  • PARK, decided: the HttpStatus log field carries display text (" (HTTP 500)"). A numeric field needs a
    second template. The display text keeps the fewest moving parts.
  • PARK, decided: no new test for redaction of the Warning. SecretOutputRedactor has its own tests.
  • PARK, for the operator: the eval suite did not run for the one-row change to
    netclaw-operations/references/diagnostics.md. evals/README.md requires no new case for a reference-table
    row.

@Aaronontheweb Aaronontheweb added bug Something isn't working observability mcp Model context protocol server / client issues. labels Aug 26, 2026
@Aaronontheweb Aaronontheweb mentioned this pull request Aug 26, 2026
Merged
@Aaronontheweb
Aaronontheweb marked this pull request as ready for review August 26, 2026 04:58
@Aaronontheweb
Record MCP tool-call exceptions as failed outcomes (#2055)
2067ee7 
An MCP tool call that ended in an exception became a tool result with no tool
receipt. The dispatcher then recorded the call as a success. The daemon log
showed no failure.

McpToolAdapter now completes the receipt from the caught exception. An HTTP 401
or 403 gives AccessDenied. An HTTP 404 gives NotFound. Every other exception
gives TransientFailure. The result text stays the same.

McpClientManager.InvokeSharedAsync no longer converts an McpException to a
result string. The adapter is the only converter. The manager logs one redacted
Warning for each failed call. The line names the server, the tool, and the HTTP
status when the server sent one. A caller cancellation writes no log. The tool
lookup moved out of the try block, so an unavailable tool does not log as a
failed call.

This commit implements design D1 and D2 of the mcp-tool-outcome-receipts
change. It also adds the diagnostics row for the new Warning line and bumps the
netclaw-operations skill version.
@Aaronontheweb
Add adapter test for HTTP 404 outcome (#2055)
359f95e 
The 404 branch in McpToolAdapter had no test. A deletion of that branch kept
every adapter test green.

The new test drives the shared failure helper with an HTTP 404 and asserts the
NotFound receipt category. The helper also asserts that the result text names
the tool.
@Aaronontheweb
Aaronontheweb force-pushed the fix/mcp-tool-exception-receipts branch from d0e48da to 359f95e Compare August 26, 2026 08:39
@Aaronontheweb Aaronontheweb mentioned this pull request Aug 26, 2026
Closed
Aaronontheweb added a commit that referenced this pull request Aug 26, 2026
@Aaronontheweb
Remove fixed Ask budget from approval checks and run the PowerShell 5…
31d338b 
….1 test alone

Two Windows-only tests failed on PR #2060. The job log is here:
https://github.com/netclaw-dev/netclaw/actions/runs/32948913877/job/98115679851

Failure 1: ToolApprovalActorTests.Version_two_omission_emits_one_bounded_actor_diagnostic
threw AskTimeoutException after 5 seconds. AkkaToolApprovalService hard-coded a 5 second
Ask budget on four calls. The actor answers GetUnapprovedPatterns on its own mailbox. It
takes a file lock, reads the store, and migrates v2 records to v3. A busy runner starves
the thread pool, so the first cold store access loses a 5 second budget.

Fix 1: each Ask now uses SessionConfig.ToolExecutionTimeout as its timeout. The default is
90 seconds. Both constructors take a required SessionConfig parameter. The daemon already
registers SessionConfig as a singleton, so the DI wiring needs no change. The bound stays
finite because some approval record callers pass CancellationToken.None. An unbounded Ask
on that path suspends the session actor mailbox forever after a lost reply.

Failure 2: ShellToolTests.Windows_power_shell_51_executes_through_the_selected_host
returned "Error: Command timed out after 90 seconds." The test starts a real powershell.exe
inside the parallel unit test host.

Fix 2: the test now carries Trait("Category", "NativeShell"). The parallel CI test step
excludes that category on every operating system. A new Windows-only step runs the category
alone after the parallel step, so the live process does not share CPU with other test
collections. Both steps are solution-level, so the exclusion and the serial run cover the
same set of projects. Missing_selected_executable_fails_without_fallback still proves host
selection deterministically.

Related: #1409, #1378
Aaronontheweb
Aaronontheweb commented Aug 26, 2026
View reviewed changes

@Aaronontheweb Aaronontheweb left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread src/Netclaw.Actors/Tools/McpToolAdapter.cs
{
StatusCode: HttpStatusCode.Unauthorized or HttpStatusCode.Forbidden
} => context.AccessDenied(text),
HttpRequestException { StatusCode: HttpStatusCode.NotFound } => context.NotFound(text),

@Aaronontheweb Aaronontheweb Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we're treating it as a transient failure because at some point we downloaded the MCP tool definition and we're subscribed to updates but the call we're making didn't reach that JSON RPC endpoint so it must be temporarily offline.

Aaronontheweb added a commit that referenced this pull request Aug 26, 2026
@Aaronontheweb
Widen the approval store Ask budget and run the PowerShell 5.1 test a…
919ece0 
…lone

Two Windows-only tests failed on PR #2060. A re-run of the same commit passed. Both tests
lost a fixed wall-clock budget on a slow runner. The job log is here:
https://github.com/netclaw-dev/netclaw/actions/runs/32948913877/job/98115679851

Failure 1: ToolApprovalActorTests.Version_two_omission_emits_one_bounded_actor_diagnostic
threw AskTimeoutException after 5 seconds. AkkaToolApprovalService hard-coded a 5 second
Ask budget on four calls. The approval actor opens the approvals file on its own mailbox.
It takes a file lock, reads the file, and migrates v2 records to v3. A busy runner starves
the thread pool, so the first cold read lost 5 seconds.

Fix 1: the four Ask calls now use a service constant of 30 seconds. The budget covers that
file round-trip only. It does not cover the wait for a person to approve the tool call. The
budget stays finite because some callers pass CancellationToken.None. A lost reply then
suspends the caller mailbox forever.

Failure 2: ShellToolTests.Windows_power_shell_51_executes_through_the_selected_host
returned "Error: Command timed out after 90 seconds." The test starts a real powershell.exe
inside the parallel unit test host.

Fix 2: the test now carries Trait("Category", "NativeShell"). The parallel CI test step
excludes that category on every operating system. A new Windows-only step runs the category
alone after the parallel step, so the live process does not share CPU with other test
collections. Both steps are solution-level, so the exclusion and the serial run cover the
same set of projects.

Related: #1409, #1378
@Aaronontheweb
Aaronontheweb merged commit 6521236 into dev Aug 26, 2026
32 of 33 checks passed
@Aaronontheweb
Aaronontheweb deleted the fix/mcp-tool-exception-receipts branch August 26, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Something isn't working mcp Model context protocol server / client issues. observability

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MCP tool-call exceptions are recorded as successful tool executions

1 participant

@Aaronontheweb