Skip to content

fix(validators): reject invalid argument types and empty positional arguments - #1583

Open
JosephDoUrden wants to merge 1 commit into
modelcontextprotocol:mainfrom
JosephDoUrden:fix/argument-validator-gaps
Open

fix(validators): reject invalid argument types and empty positional arguments#1583
JosephDoUrden wants to merge 1 commit into
modelcontextprotocol:mainfrom
JosephDoUrden:fix/argument-validator-gaps

Conversation

@JosephDoUrden

Copy link
Copy Markdown
Contributor

Summary

Publish accepts arguments that violate the schema. validateArgument only checks the named branch, so a type that is empty or outside the enum passes through, and a positional argument with neither value nor valueHint also passes. The scan in #1546 counted 52 entries with bad argument types and 8 bare positional ones, my own paging of /v0/servers found even more of the first kind. One I checked by hand: io.github.Jordan-Horner/symbols@1.0.1 (empty type under packageArguments), another is com.keyboardcrumbs/mcp@1.0.1 (type "literal", still active). Every published schema version requires type to be "positional" or "named" and positional args to carry value or valueHint. Relates to #1546, the empty repository case is #1555's, and version "latest" is already handled since #413.

Change

validateArgument becomes a switch. Named branch unchanged. Positional now requires value or valueHint. Unknown or empty type is rejected. Two new errors in constants.go, references invalid-argument-type and positional-argument-value-or-hint-required.

Why not full schema validation at publish: the enhanced validation design doc gates that behind the anyOf error consolidation (Phase 3), raw anyOf failures on arguments come out as a noisy multi-error cluster. These two checks give one clean message each and are one case arm each, easy to delete at Phase 5 when schema-first lands. Until then publish and edit just agree with what /v0/validate already reports.

Heads up on two consequences: editing a stored server that still carries one of these legacy arguments will 422 until the argument is fixed, and the importer skips invalid servers with a warning, so a re-seed from a live dump drops those entries instead of failing. Also #1339 adds a check inside the named branch of the same function, whichever lands second has a small conflict to resolve there, happy to do it from my side.

Test

Tests first, they fail on main, the handler regression actually publishes an empty-type argument with 200 on main and gets 422 with the fix. On the branch: go test ./internal/... all green including the handler tests against postgres, golangci-lint v2.13.1 (what CI pins) 0 issues, make validate passes. Two existing fixtures had positional args with neither value nor valueHint, they got a valueHint since no published schema version ever allowed that shape. New error paths and references are pinned in validation_detailed_test.go.

AI Disclosure

AI assistance (Claude) was used for issue research and during development. All changes were reviewed and tested by the author.

@baobabcat

Copy link
Copy Markdown

Cross-validated this PR against live registry data (same protocol as the #1555 cross-validation).

Disclosure: baobabcat is an AI-operated account; this comment is data for reviewers, not a review approval.

Method

  • Worktrees of main (6036804) and this PR (053877e); a thin harness unmarshals each payload into apiv0.ServerJSON and calls validators.ValidateServerJSON(&srv, validators.ValidationSemanticOnly).
  • Violators: the 60 real class-B/C payloads from the Registry accepts server.json with empty "repository": {} though schema requires url + source #1546 fixtures (52 B, 8 C), each re-fetched from the public API and re-verified live today.
  • Controls: 50 fresh entries from a 1,300-entry /v0/servers sample (today), chosen for bearing arguments that are all enum-valid and whose positionals carry value/valueHint — the entries whose verdict must not change.

Results

branch 60 class-B/C payloads 50 argument-bearing controls
main all 60 PASS, 0 issues (the gap, reproduced on real data) all valid
this PR all 60 REJECTED — 52 × argument type must be 'positional' or 'named' (quotes the offending value, e.g. "literal", "flag"), 8 × positional argument must provide a value or a valueHint all valid, output byte-identical to main
  • Zero misses: every fixture is rejected with its class-matching message; the 52/8 split matches the 2026-08-25 full-registry scan exactly.
  • Zero false positives: no control verdict or issue changed between branches.
  • No collateral edits: per violator, the PR's issue list is a strict superset of main's.
  • go test ./internal/validators/... passes on the PR branch; PR CI green.

Together with #1555 (class A), this covers classes A–C of #1546; class D (version: "latest") is already rejected on main, so merging both PRs would close every class found by the 50k-manifest scan.

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.

2 participants