Skip to content

fix(scenarios): stop MRTR retries when no input is requested - #498

Open
Yudis-bit wants to merge 3 commits into
modelcontextprotocol:mainfrom
Yudis-bit:fix/issue-440-input-required-undefined-key
Open

Yudis-bit wants to merge 3 commits into
modelcontextprotocol:mainfrom
Yudis-bit:fix/issue-440-input-required-undefined-key

Conversation

@Yudis-bit

@Yudis-bit Yudis-bit commented Sep 9, 2026

Copy link
Copy Markdown

Fixes #440.

An empty inputRequests map made five retry sites send inputResponses under the fabricated key "undefined". Those sites now stop before sending the retry and report the existing check as FAILURE with details.untestable: true. Omitting the optional field follows the same path, including when round 2 leaves round 3 without input to answer.

The guard uses key === undefined, preserving server-assigned keys such as "" and the literal "undefined". The remaining non-null assertions on inputRequests are removed; sampling, roots, and multiple-input scenarios reuse their validated maps and keys.

The sampling, roots, elicitation, and multiple-input fixtures also report an empty or omitted map as an untestable prerequisite, following #248 and #442. A response carrying only requestState is valid, but cannot exercise these fixtures. Wrong request methods still produce ordinary failures.

Validation:

  • npm run check and npm run build pass.
  • npm test: 48 files, 655 tests pass.
  • Linux CI on the exact PR head, 75b6ead passes with Node 24.20.0: install, typecheck, lint, build, and the full test suite. The publish job was skipped. Upstream workflows still await maintainer approval.
  • 32 new HTTP regression tests cover empty and omitted maps, all five retry sites, exact request counts, empty-string and literal "undefined" keys, and wrong-method failures. Against main at 7169291, 20 fail; all pass with this change.
  • The eight affected scenarios pass 24/24 checks, including wire-schema validation, against the unmodified TypeScript SDK conformance server at b65426158ed9f29aea8ef3dc09ca22d7d9d6f970. Every CLI invocation exits 0 using Node 22.14.0 on Windows. Node 24.19.0 passed the wire checks but hit a libuv assertion during CLI shutdown; those runs are excluded from this result.

AI assistance: Antigravity for the initial change; Codex for the revision and validation.

…enarios

When a server returns an InputRequiredResult with an empty or missing
inputRequests object, `Object.keys(result.inputRequests!)[0]` evaluates
to undefined. This previously caused the test harness to send
`inputResponses: { "undefined": ... }` over the wire in subsequent
rounds instead of recognizing that the round-2/round-3 retry is
untestable.

Per the untestable convention (modelcontextprotocol#248, modelcontextprotocol#372), mark the completion check as
untestable when round 1 returns no inputRequests, and safely guard all 5
inputRequests extraction sites in input-required-result.ts.

Closes modelcontextprotocol#440
@itguruhaseeb

Copy link
Copy Markdown

I verified this against main at 7169291 and I think the selection of call sites is right, which is worth saying out loud because at first glance it looks incomplete.

There are nine inputRequests! assertions in src/scenarios/server/input-required-result.ts on main, at lines 270, 406, 540, 673, 706, 866, 909, 1096 and 1338. This PR guards five: 540, 866, 909, 1096 and 1338. The four it leaves alone are not reachable with an empty map.

Lines 270 (sampling) and 406 (list roots) run round 2 under r1Errors.length === 0, and their round 1 blocks push an error both for the absent case, InputRequiredResult missing inputRequests, and separately for the empty case, inputRequests map is empty. So round 2 is only reached with at least one key. Lines 673 and 706 (multiple inputs) sit behind else if (!r1Result.inputRequests) in round 1, and round 1 also errors when keys.length < 3, with round 2 again gated on r1Errors.length === 0.

So the five you guarded are exactly the ones whose round 1 check tolerates an absent or empty inputRequests, which is the case PR 442 established is conformant rather than a violation. That is the right cut and nobody had written down why.

One thing I would still change. Those four surviving assertions are load bearing on error pushes dozens of lines away, with nothing in the code connecting the two. A later edit that relaxes a round 1 check, which is exactly what PR 442 did in a neighbouring scenario, turns one of them into a crash with no test failing first. Since you are already in this file, using ?? {} at all nine sites, with the untestable guard only where it is needed, would make it uniformly safe and remove the reasoning I just had to do in order to review it.

The question I think is the real one here. PR 442 settled that a server naming no inputRequests has not violated anything, because the field is optional, and routed that through notTestable. But the sampling and list roots scenarios still record inputRequests map is empty as a plain FAILURE in round 1. That is the same server behaviour producing a violation verdict in one scenario and an untestable verdict in another, decided only by which scenario happened to observe it.

There is a defensible answer, that those two scenarios carry an explicit fixture contract so a server returning nothing has failed the prerequisite. But that is issue 248's case, which is what PR 442 cited when it chose notTestable. Worth settling before more scenarios copy one pattern or the other.

One practical note for a maintainer passing by: the workflows here are still awaiting approval, so there is no CI signal on this yet.

Copilot AI lite review requested due to automatic review settings September 19, 2026 13:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Yudis-bit Yudis-bit changed the title fix(scenarios): guard empty inputRequests in input-required-result scenarios fix(scenarios): stop MRTR retries when no input is requested Sep 19, 2026
@Yudis-bit

Copy link
Copy Markdown
Author

@itguruhaseeb Addressed both points in 75b6ead.

The four remaining inputRequests! assertions are removed. Sampling and roots reuse the key checked in round 1; multiple-inputs reuses a map defaulted with ?? {}. The retry guards use key === undefined, preserving server-assigned keys such as "" and the literal "undefined".

For the verdict question, empty or omitted inputRequests now goes through untestableCheck in sampling, roots, elicitation, and multiple-inputs. A state-only result is valid, but leaves the fixture prerequisite missing: FAILURE with details.untestable: true, following #248 and #442. Wrong request methods still produce ordinary failures. I also removed the earlier field-presence gates that prevented omitted inputRequests from reaching the retry guards in request-state, multi-round, and prompts.

The 32 new HTTP tests check the actual requests sent, including stopping before round 3 when round 2 requests nothing. Against main at 7169291, 20 fail; all pass with this change. The full suite passes 655 tests, and the eight affected scenarios pass 24/24 checks against the TypeScript SDK.

Linux CI on this commit passes in the fork. The upstream workflows still need maintainer approval.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

input-required-result: harness sends the literal string "undefined" as an inputResponses key when round 1 names no input requests

3 participants