Uh oh!
There was an error while loading. Please reload this page.
Standardize scenario setup/execution failure reporting (#248) - #327
Standardize scenario setup/execution failure reporting (#248)#327rinaldofesta wants to merge 2 commits into
Conversation
79c2705 to
86beac9Compare86beac9 to
0083e27Comparepanyam
commented
Jul 22, 2026
Would like to share a data point in favor of making setup failures loud. This was from a case we just closed in mcpkit (an experimental community Go SDK): The auth/authorization-server-migration scenario spent months in our runs reporting "2 passed, 1 failed" - This was just the three unconditional sep-2352 evaluation checks - while the other ~76 checks the scenario can emit never ran. Our client was failing the initial wire handshake (a bug on our side, since fixed), so the OAuth flow never started. The scenario-level verdict was correctly red the whole time. But two of the three emitted checks were passing trivially (the client never contacted AS2 at all, so "no credential reuse at AS2" passed for wrong reasons) And ofcourse the missing depth was invisible in every report we were generating from the results. On the open question in the description is whether this incident warrants the dedicated -setup check being necessary but not sufficient on its own. A consumer looking at "2 passed / 1 failed / setup FAILURE" still cannot tell that ~76 checks went missing. I think emitting all declared IDs as FAILURE - or, less invasively, including a declared-vs-emitted count per scenario in the results would what makes this gap visible in the numbers intead of only in the scenario source. For the variant this PR's shape cannot reach - a scenario that PASSES its thin shadow because only accept-case checkpoints were exercised - we added a downstream guard that snapshots per-scenario emitted-check counts and fails when a passing scenario's count drops (PR 1115). we think count-based upstream would make this PR unnecessary for every SDK individually. Happy to share run artifacts from the before/after if useful. |
…ure helper Scenarios that can't execute (connect failure, missing fixture, capability not advertised) currently hand-roll a try/catch around connect and pin the error onto whichever check ID happens to be first. That mislabels the failure and silently drops any *other* checks the scenario would emit (e.g. a connect failure in tools-list also makes tools-name-format vanish). Add `reportSetupFailure(scenarioName, error, specReferences?)` to sdk-client.ts, emitting a single dedicated `<scenario>-setup` check as FAILURE with the error detail. Route the setup path of four representative multi-check server scenarios through it (server-initialize, tools-list, prompts-list, resources-list): connect runs in its own try/catch that returns the setup check; genuine post-connect failures keep their real check IDs. Rebased onto the version-aware connection abstraction (modelcontextprotocol#318): scenarios now take a RunContext and connect via ctx.connect(); the setup/exec split and the reportSetupFailure helper carry over unchanged. This is the "-setup check as a first cut" from modelcontextprotocol#248. Refs modelcontextprotocol#248.
rinaldofesta
commented
Aug 9, 2026
You are right that this PR does not close the gap you are describing. #327 fixes attribution: a setup failure stops being reported as a failing check. It says nothing about how many declared checks never got the chance to emit, so "2 passed / 1 failed / setup FAILURE" still reads as three checks when the scenario can emit about seventy-nine. The variant you name is the sharper one, and it is out of reach here by construction: when setup succeeds and only accept-path checkpoints run, there is no setup failure to make loud, so the thin shadow passes quietly. A declared-vs-emitted count per scenario catches both cases, and it is cheap, since the declared IDs are known at registration time. I would keep it separate from this PR rather than fold it in: the setup/execution split changes a reporting contract, the count adds a coverage signal, and the two will get reviewed on different grounds. Happy to open the count as its own issue and cite mcpkit#1115 as prior art, unless you would rather own it. The before/after run artifacts would be useful, yes. |
0083e27 to
660b042CompareconnectToServer, the initialize SUCCESS push and close() all sat inside one try, so a close() failure after a completed handshake returned `server-initialize-setup` and discarded the SUCCESS just recorded. That is the same misattribution this PR exists to remove, in the scenario it is named after; prompts.ts, resources.ts and tools.ts already isolate the connect call. Isolate connect so only a handshake failure reports as setup. Teardown is best-effort at this layer to match terminateSessionRaw, which already swallows the session-terminating DELETE, so the only thing that can still reject is the client-side close, and that is not a server conformance result.
rinaldofesta
commented
Aug 9, 2026
Follow-up commit on the same misattribution, found while reviewing today's rebase. Connect is isolated now. Teardown stays best-effort at that layer to match 530 tests, typecheck and lint green. |
panyam
commented
Aug 9, 2026
hey @rinaldofesta thanks heaps for looking at this. Happy either way. I can submit a PR if that is easier. If you already have an issue and can assign it to me - even better. wdyt? |
Description
Closes#248. Standardizes how scenarios report connect/setup failures.
Today each scenario hand-rolls a
try/catcharound connection + fixture setup and reports the error onto an arbitrary primary check ID, so a setup failure is mislabeled as "check X failed." This adds a shared helperreportSetupFailure(scenarioName, error)insrc/scenarios/server/client-helper.tsthat emits a single dedicated<scenario>-setupcheck as FAILURE with the error detail, and routes the setup path of four representative multi-check scenarios (server-initialize,tools-list,prompts-list,resources-list) through it.One open question (the issue leaves this open): I implemented the
<scenario>-setupdedicated-check shape you called "probably reasonable as a first cut." The alternative — emitting all of a scenario's declared check IDs as FAILURE on a setup error — is also viable. Happy to switch if you'd prefer that.Notes
<scenario>-setupis a non-sep-*advisory ID, so it doesn't affect traceability (keyed by SEP) or the expected-failures baseline (keyed by scenario name).tryrewrite preserves connection-close semantics exactly (no new leak).Validation
npm run check(tsgo + eslint + prettier) ✓npm run build✓npm test— 21 files / 228 tests ✓ (incl. new unit tests forreportSetupFailureand a lifecycle setup-failure test)Implemented with the help of a coding agent (Claude Code) and reviewed locally by the author.