Title: Consult request_state only for the question a resolver is asking by maxisbey · Pull Request #3019 · modelcontextprotocol/python-sdk · GitHub
Open Graph Title: Consult request_state only for the question a resolver is asking by maxisbey · Pull Request #3019 · modelcontextprotocol/python-sdk
X Title: Consult request_state only for the question a resolver is asking by maxisbey · Pull Request #3019 · modelcontextprotocol/python-sdk
Description: Summary Post-merge review of #2986 surfaced three issues in the resolver input_required flow; this PR fixes all three, each by removing the structure that made the issue possible rather than guarding around it. 1. request_state entries could stand in for a resolver's own computation. The restore gate in _resolve consulted the client-echoed request_state before running the resolver body. Wire keys are predictable (module:qualname), so a client could send an accept entry for a resolver branch that never asks, replacing a server-side computed value, and a decline/cancel entry could abort a resolver with no Elicit arm at all. That contradicted #2986's stated trust model ("a fabricated entry grants the same capability as answering the live question dishonestly"): an entry honored before the body runs grants strictly more. Restore now happens only inside _elicit, after the body has returned an Elicit this round. The body's concrete return always wins; a recorded answer satisfies exactly the question being asked, re-validated against the live Elicit.schema; an entry for a question the server is not asking is never consulted. The trust-model statement is now true by construction. (Integrity-sealing request_state remains the separate follow-up, staged the same way as in the typescript-sdk.) 2. -> Elicit[BaseModel] crashed on restore. issubclass(BaseModel, BaseModel) is true, so a dynamic-schema resolver (the natural typing for create_model(...) questions) registered BaseModel itself as its static restore schema; on round 3+, BaseModel.model_validate(...) raises PydanticUserError (a TypeError, not a ValidationError), which hard-failed the call after the user had already answered. With restore moved into _elicit, the statically-extracted schema has no remaining consumer, so that plumbing is deleted and the crash path with it: dynamic schemas restore against the live schema like everything else. (The registration-time rejection of multi-Elicit-arm annotations stays.) 3. Resolver elicitation and a hand-rolled InputRequiredResult body never converge together. A call has one input_responses/request_state channel. A tool that uses Resolve(...) parameters and returns its own InputRequiredResult overwrites the resolver codec state with its opaque body state; the next round decodes that as "no progress" and re-asks the already-answered resolver question. Round 3 is wire-identical to round 1, looping until the client's round budget runs out. A declared InputRequiredResult return arm on a resolver-using tool is now rejected at registration (InvalidSignature, seeing through Annotated wrappers, type aliases, and unions), and an undeclared one fails the call with a clear error instead of silently fighting for the channel. The low-level Server path and tools without resolver parameters are unaffected. Semantics The cost of body-wins is that an eliciting resolver's body re-runs on each round the call passes through (previously: once to ask, once to consume, then restored without running). This unifies the model: every resolver body may run on each round, each question is still asked exactly once, and only elicited outcomes persist. Wire shapes, round counts, and question batching are unchanged. The docs (dependencies.md, multi-round-trip.md, the refund_desk story README) now scope the run-once guarantee to the question. Tests Five new regression tests, each verified to fail on the pre-fix source: a crafted accept entry cannot replace a non-eliciting branch's value (and the body provably runs); a decline entry cannot abort a pure resolver; an -> Elicit[BaseModel] chain completes across rounds; the Resolve + InputRequiredResult combination is rejected at registration across five annotation spellings; a dynamically returned InputRequiredResult from a resolver-using tool errors cleanly. The existing suite is updated for the run-count change. 100% branch coverage. AI Disclaimer
Open Graph Description: Summary Post-merge review of #2986 surfaced three issues in the resolver input_required flow; this PR fixes all three, each by removing the structure that made the issue possible rather than guardi...
X Description: Summary Post-merge review of #2986 surfaced three issues in the resolver input_required flow; this PR fixes all three, each by removing the structure that made the issue possible rather than guardi...
Opengraph URL: https://github.com/modelcontextprotocol/python-sdk/pull/3019
X: @github
Domain: github.com
| route-pattern | /:user_id/:repository/pull/:id/files(.:format) |
| route-controller | pull_requests |
| route-action | files |
| fetch-nonce | v2:843c807f-8910-0d8a-79e3-bfcb885bf516 |
| current-catalog-service-hash | ae870bc5e265a340912cde392f23dad3671a0a881730ffdadd82f2f57d81641b |
| request-id | AEDA:1D6C27:8271F0:ADCFFB:6A5B66D6 |
| html-safe-nonce | fabd31344f7aa83e1a5b384ec7d1f1251730a05af80e2595bdb6e1a6136c70cf |
| visitor-payload | eyJyZWZlcnJlciI6IiIsInJlcXVlc3RfaWQiOiJBRURBOjFENkMyNzo4MjcxRjA6QURDRkZCOjZBNUI2NkQ2IiwidmlzaXRvcl9pZCI6IjI4NzE1MzkzNDY4OTcwNjk3ODIiLCJyZWdpb25fZWRnZSI6ImlhZCIsInJlZ2lvbl9yZW5kZXIiOiJpYWQifQ== |
| visitor-hmac | 1462660f6541d230773b640052588ae188a4a510ce5cbc828b55c42f2c70a3c1 |
| hovercard-subject-tag | pull_request:3956455844 |
| github-keyboard-shortcuts | repository,pull-request-list,pull-request-conversation,pull-request-files-changed,copilot |
| google-site-verification | Apib7-x98H0j5cPqHWwSMm6dNU4GmODRoqxLiDzdx9I |
| octolytics-url | https://collector.github.com/github/collect |
| analytics-location | / |
| fb:app_id | 1401488693436528 |
| apple-itunes-app | app-id=1477376905, app-argument=https://github.com/modelcontextprotocol/python-sdk/pull/3019/files |
| twitter:image | https://avatars.githubusercontent.com/u/224885523?s=400&v=4 |
| twitter:card | summary_large_image |
| og:image | https://avatars.githubusercontent.com/u/224885523?s=400&v=4 |
| og:image:alt | Summary Post-merge review of #2986 surfaced three issues in the resolver input_required flow; this PR fixes all three, each by removing the structure that made the issue possible rather than guardi... |
| og:site_name | GitHub |
| og:type | object |
| hostname | github.com |
| expected-hostname | github.com |
| None | 5290d7e14309ad1e76106a9c4237bd1041517e83ea182c8ab756752cb0c6940b |
| turbo-cache-control | no-preview |
| diff-view | unified |
| go-import | github.com/modelcontextprotocol/python-sdk git https://github.com/modelcontextprotocol/python-sdk.git |
| octolytics-dimension-user_id | 182288589 |
| octolytics-dimension-user_login | modelcontextprotocol |
| octolytics-dimension-repository_id | 862584018 |
| octolytics-dimension-repository_nwo | modelcontextprotocol/python-sdk |
| octolytics-dimension-repository_public | true |
| octolytics-dimension-repository_is_fork | false |
| octolytics-dimension-repository_network_root_id | 862584018 |
| octolytics-dimension-repository_network_root_nwo | modelcontextprotocol/python-sdk |
| turbo-body-classes | logged-out env-production page-responsive full-width |
| disable-turbo | true |
| browser-stats-url | https://api.github.com/_private/browser/stats |
| browser-errors-url | https://api.github.com/_private/browser/errors |
| release | 9c975978430e9ad293956f2bbdaf153b1bd84a99 |
| ui-target | full |
| theme-color | #1e2327 |
| color-scheme | light dark |
Links:
Viewport: width=device-width