Skip to content

fix: specify closure handler signature - #475

Draft
simonyang08 wants to merge 1 commit into
modelcontextprotocol:mainfrom
simonyang08:codex/fix-handler-closure-type
Draft

fix: specify closure handler signature#475
simonyang08 wants to merge 1 commit into
modelcontextprotocol:mainfrom
simonyang08:codex/fix-handler-closure-type

Conversation

@simonyang08

@simonyang08 simonyang08 commented Aug 22, 2026

Copy link
Copy Markdown

Summary

  • give the registry handler Closure alias an explicit variadic signature
  • allow strict PHPStan users to decorate RegistryInterface without a missing callable signature error

Root cause

The Handler PHPStan alias used a bare \Closure, so consumers inheriting the alias received missingType.callable under strict analysis.

Current validation status

  • consumer-only minimal PHPStan reproducer: fails before and passes with the proposed alias
  • ReferenceHandlerTest: 4 tests, 9 assertions pass
  • git diff --check: passes

Upstream CI exposed a variance conflict that the isolated reproducer missed: the proposed Closure(mixed...): mixed type rejects the repository's valid handlers with narrower required parameter lists. The full PHPStan job reports 29 argument.type errors introduced by this alias. The PR is therefore intentionally Draft while the correct way to represent an arbitrary Closure signature is discussed; the current commit should not be merged as-is.

Related to #468 — design spike; current commit is not merge-ready.

@simonyang08
simonyang08 marked this pull request as ready for review August 22, 2026 06:07
@simonyang08
simonyang08 marked this pull request as draft August 22, 2026 06:14
@chr-hertel

Copy link
Copy Markdown
Member

Thanks for opening the PR @simonyang08 - the pipeline failures are def related, please have a look

@chr-hertel chr-hertel added the Server Issues & PRs related to the Server component label Aug 23, 2026
@simonyang08

Copy link
Copy Markdown
Author

Thanks for flagging this. I've confirmed that the Closure(mixed...): mixed alias causes the QA variance failures, so I've kept the PR in Draft. I won't suppress those errors or widen the existing handlers just to make the pipeline green. I'm validating whether the API can preserve concrete handler signatures, or whether this needs to remain a PHPStan-specific/documented limitation, and I'll report the trade-off here before pushing any further change.

@simonyang08

Copy link
Copy Markdown
Author

I checked the bounded type options against the repository's existing handler shapes and the failed QA log. There is no precise one-line callable signature that satisfies both sides:

  • bare Closure preserves the heterogeneous handlers but leaves the consumer missing-signature warning;
  • Closure(mixed...): mixed (and the callable equivalent) promises arbitrary mixed arguments, so it correctly rejects the existing narrower handlers;
  • object|array|string would likely avoid the analyzer conflict, but it weakens the PHPDoc contract by accepting arbitrary objects statically while the native constructor remains stricter.

The registry intentionally reflects heterogeneous handlers (RequestContext-only, RequestContext plus arguments, scalar arguments, arrays), so a universal callable signature is not sound. I don't plan to push another speculative alias. The viable choices appear to be: keep bare Closure and document a PHPStan stub/limitation; intentionally accept the broader object union; or defer a precise fix to a larger design that separates typed handler categories. Please let me know which direction you prefer.

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

Labels

Server Issues & PRs related to the Server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants