Skip to content

mcp: no operator-facing diagnostics for the three failures the server absorbs #99

Description

@carldebilly

Repl.Mcp emits no operator-facing diagnostics at all: a search across src/Repl.Mcp/**/*.cs for
ILogger, ActivitySource, Meter, Console.Error and Trace returns nothing. The only
System.Diagnostics use is Stopwatch.GetTimestamp() for retry cooldown timing.

That was tolerable while the package was a thin projection. It is not once mcp serve is a
long-lived server whose failures are recoverable by design: three separate paths now absorb a
failure deliberately, and none of them records what they absorbed.

The three

The withheld failure cause. McpToolAdapter.DescribeFailure returns
Command failed with exit code {n}. and drops invocation.Output, which already holds the cause
rendered by CoreReplApp.DescribeLocally — the operator-facing formatter that unwraps
ReplBindingCallbackException and peels reflection's layers. The exception object itself is in
scope as invocation.Failure. Withholding it from the remote client is the point; discarding it
entirely is not. DescribeLocally's own doc comment anticipates this consumer.

The stale-catalog fallback. McpServerHandler.BuildOrServePreviousAsync re-serves a previous
catalog with no log, metric or notice, on every request, for as long as the failure lasts. It hides
a client-driven McpException("Client roots changed repeatedly…"), an InvalidOperationException
from compatibility-name validation, a presence predicate's NullReferenceException, a
JsonException. The operator sees InvalidateRouting() apparently doing nothing.

The swallowed roots prime. McpClientRootsService.PrimeFromServicesAsync is awaited before
every invocation and absorbs everything but the caller's own cancellation. A client that declares
roots and then stalls costs ten seconds per thirty-second window, invisibly.

Shape

The idiom exists in this repository and needs no new dependency: Microsoft.Extensions.Logging
already reaches Repl.Mcp transitively through Repl.Defaults → Repl.Logging, with no
PrivateAssets. Copy ReplResultFlowLoggerDiagnosticsILoggerFactory fetched optionally from
DI with a NullLoggerFactory.Instance fallback, a const category, [LoggerMessage]
source-generated methods. AddReplLogging() is already applied on the MCP path, so the factory is
resolvable from McpToolAdapter._services at runtime with no wiring change.

DescribeFailure and WithholdsFailureText are private static taking an in struct, so they
become instance methods or take the logger as a parameter — that is the whole of the plumbing.

Raised by the operability lens during the review panel on #71, where it was scoped out
deliberately to keep that pull request's diff bounded.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions