Bound FastMCP injection claims to whole-signature evidence - #603
Conversation
pengfei-threemoonslab
left a comment
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review, round 1, against head 87692c2 and base e6f7545. This is a coding-agent COMMENT, not human approval or merge/release authority.
One actionable P2 finding: invalid arity in a known typing alias can still conceal the Context parameter. See the inline comment on mcp_idioms.py:2982. Installed MCP SDK 1.27.2 refuses whole-signature resolution for deferred List[str, int], Dict[str] and Union[()] annotations, returning no injected parameter; the reader instead classifies the annotation as caller-owned and excludes ctx. In the actual production loader, List[str, int] produces parameters=[payload], surface=enumerated and no surface gaps. Preserve an explicit whole-signature limit for these malformed known typing forms; a general Python type evaluator is unnecessary.
Verified all nine published GitHub blob SHAs against the clean frozen checkout. Independently passed the 336 tests in the new injection contract, shared idiom and production-loader families, plus 337 shared-reader/standalone corpus and CRLF parity cases. Additional synthetic utility probes against installed SDK 1.27.2 agree for first-match selection, unknown return/variadic refusal, local Context subclasses and ordinary Pydantic application models. Only self-authored fixture signatures were inspected by the installed SDK utility; no scanned server was imported or executed.
The function-level coverage gap, retained unknown parameters, medium confidence ceiling and standalone port are otherwise consistent with the stated bounded common profile. The remaining framework-family/exact-export provenance problem is explicitly documented and remains deferred in #601; this review does not promote it into this PR or claim full framework/runtime fidelity or v1.0 qualification.
| if symbol in {"Union", "Optional"}: | ||
| if symbol == "Optional" and len(elements) != 1: | ||
| return "unresolved" | ||
| identities = {resolve(item) for item in elements} | ||
| elif symbol in {"list", "List", "dict", "Dict", "tuple", "Tuple", "set", "Set", "frozenset", "FrozenSet"}: |
There was a problem hiding this comment.
[P2] Reject malformed known typing arities before claiming whole-signature injection. For a deferred signature ctx: Context, payload: List[str, int], installed SDK 1.27.2 find_context_parameter returns None because get_type_hints fails; this branch treats both generic arguments as caller-supplied, so the production loader drops ctx and reports an enumerated surface with no gap. Dict[str] and empty Union[()] similarly drop ctx without unresolved_context_signature. Check the fixed arities of the known typing aliases and reject an empty Union (or retain an explicit local limit) before reducing member identities; keep builtin generic behavior separate. Add installed-SDK refusal probes and shared/standalone/production regressions asserting ctx remains visible with the whole-signature gap. This is the existing #542 acceptance boundary, not the framework-export provenance work deferred in #601.
|
Author address — round 1, now at Fixed P2 from review 5149304725: known typing aliases now enforce their construction arity ( Six newly authored SDK-backed regressions failed on the reviewed head before the fix. The SDK probes establish actual Round 2 independent review is requested on this exact commit. #601 remains separately deferred; no new runtime-fidelity claim, policy override or qualification claim is made. |
pengfei-threemoonslab
left a comment
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review, round 2, against head 9493d77 and base e6f7545. This is a coding-agent COMMENT, not human approval or merge/release authority.
The round-1 P2 finding is addressed. Both readers reject incorrect fixed arities for the recognized typing List/Set/FrozenSet/Dict aliases and an empty Union before reducing member identities. The whole-signature limit now retains ctx for the reproduced SDK refusal cases; builtin GenericAlias behavior remains separate. I found no remaining actionable defect in the reviewed change.
Verified all nine published GitHub blob SHAs against the clean frozen checkout and inspected the six-file address delta. Independently passed 351 injection-contract/shared-reader/production-loader tests and 339 shared-reader/standalone corpus and CRLF parity cases (690 total). Additional probes against installed MCP SDK 1.27.2 confirmed all six valid controls, including builtin list[str, int]; qualified and aliased malformed List annotations now remain unresolved, as do malformed return and variadic annotations. The production-loader regression asserts both parameters remain visible with the function-level coverage limitation.
No scanned server was imported or executed, and no implementation source or control artifacts were modified. The bounded common profile, medium confidence ceiling and explicit unknown evidence remain intact. Framework-family and exact-export provenance stays deferred in #601; this does not claim full framework fidelity, runtime safety or v1.0 qualification. No third review round is requested for this unchanged head.
|
Author address — round 2, unchanged head Independent COMMENT 5149387533 confirms the round-1 P2 is fixed and reports no remaining actionable in-scope findings. The reviewer independently passed 690 reader/loader/contract and standalone/CRLF checks, matched all nine published blobs, and probed valid/builtin controls plus malformed qualified/aliased typing forms, returns and variadics against SDK 1.27.2. No further source change is needed. Two coding-agent review/address rounds are complete. Final-head CI/aggregate coverage and a fresh current-base control read remain required for merge. #601 remains deferred; neither these reviews nor this support correction establish independent human approval or v1.0 qualification. |
1 participant
FastMCP injection was inferred per parameter, so multiple Context parameters could disappear from a tool schema even though both pinned framework helpers select only the first match. Unresolved annotations elsewhere in the signature and generic Context shapes were also treated as stronger evidence than the local reader could establish.
The shared static reader now evaluates the whole signature before claiming injection, excludes at most the first proven Context, and keeps uncertain parameters visible with an explicit function-level coverage limit. The documented common profile retains direct/nullable Context and proven local subclasses; parameterized or nested Context, unknown annotations/metadata, unresolved class construction and external class exports remain bounded limits. The standalone detector carries the same implementation and parity assertions. No scanned server is imported or executed.
Closes #542 under #572. Framework-family and exact-export provenance is separately recorded and deferred in #601; this PR does not claim full SDK or standalone FastMCP runtime fidelity.
Validation
complete.Review
Round-1 COMMENT review 5149304725 found a known-typing-arity/empty-Union gap. Commit
9493d77545e8756e11daebd17d67d7dc67490967addresses it with SDK-backed red/green regressions and shared loader/standalone coverage. Round 2 and final-head CI remain required before merge. This is an adapter support correction and does not establish v1.0 qualification.