Skip to content

mcp: four follow-ups from the #71 review panel #103

Description

@carldebilly

Four small items from the review panel on #71, none worth its own issue, all in Repl.Mcp.

CarriesSessionlessFields matches one literal revision

McpProtocolRevisions.CarriesSessionlessFields compares against 2026-07-28 exactly, so any later
sessionless revision reads as legacy. Two provable consequences on the next SDK bump: discovery falls
back to the per-connection view and the stale-catalog fallback on a revision that forbids both, and
ResolveThreshold drops to Debug, emitting message notifications for a request that declared no
log level — which the surrounding code says it must not. Latent today (the SDK is pinned), and
nothing goes red when the revision set grows. Compare ordinally against Sessionless as a lower
bound, and pin the SDK's revision set with a test.

The presence overlay is rebuilt per call

McpToolAdapter constructs a fresh McpServiceProviderOverlay for every frozen-catalog invocation,
so _routingCacheByServiceProvider.GetOrCreateValue always misses: every tool call re-runs every
module presence predicate and leaves a dead entry in the conditional weak table. The discovery
channel is documented as holding no answer and therefore interchangeable, and
McpInteractionChannel's fields are all readonly, so one instance per adapter is sound. The ripple
is the "fresh dictionary per call" note on CreateSessionScopedOverrides.

The execution prologue is written three times

McpExplicitPrompt.GetAsync, McpAppResource.ReadAsync and McpToolAdapter.ExecuteThroughPipelineAsync
each prime roots, resolve IMcpFeedback as McpFeedbackService, push the undelivered buffer, share
an identical cancellation filter, and drain-and-append on the way out. Two of the three copies were
written in #71, so the rule of three is met. One internal static helper, not a new abstraction —
the next prologue step should not have to be found in three places.

A presence predicate cannot tell it is being probed

On 2026-07-28 discovery runs predicates against frozen answers: a predicate that writes session
state or sets soft roots is silently no-op'd, and capability probes always read as supported. That is
documented, but only in prose — the predicate itself has no way to know. A discovery flag on
ModulePresenceContext would make it legible, and belongs with the per-caller graph work in #97.

Cancellation provenance survives reflection by accident

HandlerArgumentBinder.IsApplicationFailure decides whether a binder callback failed or the caller
withdrew, by asking who cancelled rather than reading the exception's type. But it reads the exception
reflection handed it, and reflection wraps: a probe calling PropertyInfo.SetValue on a setter that
throws OperationCanceledException returns TargetInvocationException. So a withdrawal raised
through an options-group constructor, a property setter or a keyed-service factory is marked as an
application failure.

The observable outcome is nonetheless correct, measured both ways: a caller withdrawal ends the run
Cancelled with no error result, and a callback cancelling on a token of its own is rendered
execution_error. The right answer is reached at the pipeline boundary, where
RunUnderCancellationPolicyAsync converts on the caller's own token — one level out from where the
rule is written. Both directions are pinned by guards in Given_ExitCodes, so drift turns red.

Tightening it means rethrowing the unwrapped cancellation from the binder rather than unwrapping
inside the predicate: a filter returning false leaves reflection's wrapper to propagate, and the
pipeline's cancellation arm does not match a TargetInvocationException either, so it would land in
the general arm and report an execution error — the outcome this would be trying to avoid.

Raised as a P2 by Codex on #71.

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