Make MCP tool-level auth failures visible, and unblock netclaw mcp auth - #1720
Merged
Aaronontheweb merged 6 commits intoJul 31, 2026
Merged
Conversation
An MCP server can report a failure inside a successful response, with `isError: true`. No exception reaches the transport layer, so two things went wrong at once. The daemon recorded only the length of the result. The server's own detail went to the model and never reached disk, so an operator who saw the agent report a problem had nothing to debug from. The daemon now logs the detail at warning level. It redacts the text first, because an MCP error body can echo the arguments it rejected and daemon logs leave the box when the OTLP exporter is on. A rejected credential arrives this way, not as an HTTP 401. The transport stays healthy, so the server continued to report `Connected` while every call failed, and the one state that needs an operator action was the one state that status never showed. A tool-level failure whose text reads as an authentication rejection now moves the server to `AuthFailed` and names `netclaw mcp auth <name>`. It also raises the same operational alert that a transport authentication failure raises. The message match is shared with the exception path through `IsAuthFailureMessage`, so both paths recognize the same wording. A tool-level failure carries no exception and no HTTP status, so the wording is all there is. `invalid_token` and `token expired` join the list. Two tests cover the two halves: an authentication failure moves the server out of `Connected`, and a failure that is not about authentication reaches the log while leaving the connection state alone. Both fail when the fix is reverted. `RecordingLogger<T>` moves to the shared test doubles and now captures rendered messages as well as exceptions. Validation: 6081 tests pass and 0 fail. `dotnet slopwatch analyze`, the copyright header check, and `git diff --check` are clean.
An invocation against a server that waits on authorization threw "MCP server '<name>' is unavailable or tool '<tool>' is not registered". That message is what the agent repeats to the operator. It reads as a broken server and sends the operator to look for the wrong problem, while `netclaw mcp list` names the correct remedy for the same condition. The message now names `netclaw mcp auth <name>` when the server state is `AuthFailed` or `AwaitingAuth`, and keeps the original text for every other cause. Validation: 6082 tests pass and 0 fail. `dotnet slopwatch analyze`, the copyright header check, and `git diff --check` are clean.
`netclaw mcp auth <name>` failed about five seconds after it printed the authorization URL, before an operator could reach the browser. SDK 2.0 probes `server/discover` before it falls back to the `initialize` handshake, and it bounds that probe at five seconds by default. A server that answers the probe with 401 sends the SDK into the authorization callback handler, which cannot return until the operator finishes. The probe timeout cancels that wait, the SDK falls back to `initialize`, gets a second 401, and calls the handler again for the same flow. The single-owner guard rejects the second call, and the whole connection attempt fails. The guard is correct: each call to `InitiateAuthorizationCodeFlowAsync` generates a new `state` value, so a second call cannot reuse the URL the operator already has. The fix is to stop the probe from expiring while a person works. Both connect timeouts now match the flow lifetime when a flow exists. A background reconnect keeps the SDK defaults. Its handler returns immediately, nothing waits, and a longer timeout would only delay an unreachable server. Release 1.4.1 had no discovery probe, so one 401 produced one handler call. This defect arrived with the 2.0 upgrade. Validation: 6084 tests pass and 0 fail. `dotnet slopwatch analyze`, the copyright header check, and `git diff --check` are clean.
The CLI and the TUI each waited a hardcoded five minutes for an MCP OAuth flow to complete. The daemon enforces its own deadline. The two agreed by coincidence, and a change to the flow lifetime would have separated them silently: a client that stops first reports a timeout for a flow the daemon is still ready to complete. `McpOAuthStartResponse` now carries the flow's `ExpiresAt`, and both clients wait until that moment. Sharing the constant was the other option and it is worse. `FlowLifetime` is internal to the daemon, and the CLI is an HTTP client of the daemon rather than a reference to it. Either a project reference or a hoisted constant would make the dependency graph worse to keep one number in agreement. A per-flow deadline in the response also survives a lifetime that later varies by server. Validation: 6084 tests pass and 0 fail. `dotnet slopwatch analyze`, the copyright header check, and `git diff --check` are clean.
The CLI and the TUI read `expiresAt` from the start response with a required property access, which throws when the daemon does not send it. The CLI and the daemon swap separately during an upgrade, so a newer CLI against an older daemon is a normal window, and `netclaw mcp auth` would have failed for the length of it. Both clients now fall back to the five minute lifetime that an older daemon enforces. Validation: 6084 tests pass and 0 fail. `dotnet slopwatch analyze`, the copyright header check, and `git diff --check` are clean.
Aaronontheweb
commented
Jul 31, 2026
| // daemon is a normal window rather than a broken install, and crashing here | ||
| // would take out `netclaw mcp auth` for the length of it. Fall back to the | ||
| // lifetime that daemon enforces. | ||
| var deadline = startResult.TryGetProperty("expiresAt", out var expiresAt) |
| : null; | ||
| var result = await _clientRuntime.InvokeAsync(function, aiArgs, ct); | ||
|
|
||
| if (McpToolResultFormatter.TryGetErrorDetail(result, out var detail)) |
| _logger.LogWarning( | ||
| "MCP tool '{Tool}' reported a failure: {Detail}", | ||
| qualifiedToolName, | ||
| SecretOutputRedactor.Redact(detail)); |
| /// Recognizes an authentication rejection from message text alone. A tool-level | ||
| /// failure carries no exception and no HTTP status, so the wording is all there is. | ||
| /// </summary> | ||
| private static bool IsAuthFailureMessage(string message) |
Collaborator
Author
There was a problem hiding this comment.
kind of jank but as the comment says, the tool doesn't see the HTTP status codes
Aaronontheweb
enabled auto-merge (squash)
July 31, 2026 16:09
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1714. Four defects found by running the SDK 2.0 migration against live MCP servers, plus one compatibility fix.
An MCP server can report a failure inside a successful response, with
isError: true. No exception reaches the transport layer, and that single fact caused three separate problems.An expired credential was invisible
The daemon recorded only the length of the result. The server's own detail went to the model and never reached disk, so an operator whose agent said "Notion is auth-failing" had nothing to check.
McpToolResultFormatteralready extracted that detail; nothing logged it.The detail is now logged at warning level, redacted first — an MCP error body can echo the arguments it rejected, and daemon logs leave the box when the OTLP exporter is on.
Status reported a healthy server while every call failed
A rejected credential arrives as a tool-level failure, not an HTTP 401. The transport stays healthy, so the server kept reporting
Connectedwhile every invocation failed. The one state that needs an operator action was the one state that status never showed.A tool-level failure whose text reads as an authentication rejection now moves the server to
AuthFailed, namesnetclaw mcp auth <name>, and raises the same operational alert a transport failure raises. The message match is shared with the exception path so both recognize the same wording.The invocation error did not name the remedy
A call against a server awaiting authorization threw
MCP server '<name>' is unavailable or tool '<tool>' is not registered. That text is what the agent repeats to the operator, and it reads as a broken server. It now namesnetclaw mcp auth <name>when the server state isAuthFailedorAwaitingAuth, and keeps the original wording for every other cause.netclaw mcp authfailed five seconds after printing the URLThis one is a regression from #1714 and it blocked reauthorization completely.
SDK 2.0 probes
server/discoverbefore falling back to theinitializehandshake, and bounds that probe at five seconds. A server that answers the probe with 401 sends the SDK into the authorization callback handler, which cannot return until the operator finishes in a browser. The probe timeout cancels that wait, the SDK falls back toinitialize, gets a second 401, and calls the handler again for the same flow. The single-owner guard rejects the second call and the whole attempt dies.The guard is correct and stays. Each call to
InitiateAuthorizationCodeFlowAsyncmints a freshstate, so a second call cannot reuse the URL the operator already has on screen. The fix is to stop the probe expiring while a person works: both connect timeouts now match the flow lifetime when a flow exists. A background reconnect keeps the SDK defaults, because its handler returns immediately and nothing waits.Release 1.4.1 had no discovery probe, so one 401 produced one handler call.
Flow deadline
The CLI and the TUI each waited a hardcoded five minutes while the daemon enforced its own deadline. They agreed by coincidence.
McpOAuthStartResponsenow carries the flow'sExpiresAtand both clients wait until that moment.Sharing the
FlowLifetimeconstant was the other option and it is worse: the constant is internal to the daemon, and the CLI is an HTTP client of the daemon rather than a reference to it. A per-flow deadline also survives a lifetime that later varies by server.The clients tolerate a daemon that does not send the field. The CLI and the daemon swap separately during an upgrade, so a newer CLI against an older daemon is a normal window rather than a broken install.
Validation
dotnet slopwatch analyze, the copyright header check, andgit diff --checkare clean.netclaw mcp auth notionnow completes, and the resulting record carriesAuthorizationServerandTokenEndpointAuthMethod, so it refreshes without another authorization.