Skip to content

fix(validators): reject incomplete repository metadata - #1555

Merged
rdimitrov merged 1 commit into
modelcontextprotocol:mainfrom
samrusani:fix/1546-reject-invalid-repository
Oct 24, 2026
Merged

rdimitrov merged 1 commit into
modelcontextprotocol:mainfrom
samrusani:fix/1546-reject-invalid-repository

Conversation

@samrusani

@samrusani samrusani commented Aug 20, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1546

What changed

  • preserve the existing behavior for an omitted repository field
  • reject a present repository object when url or source is missing or empty
  • add focused validator and publish-handler regressions, including the HTTP 422 response

Validation

Published head: be0c97339875ebf083a06bf1a5686f763024e84d.

On 2026-10-03, the unchanged patch merged cleanly into upstream main at bf4e88cbe8d1a635c06144ccea1d24cb52fa6186 for a local integration check:

  • full internal/validators and internal/api/handlers/v0 suites with -race: pass
  • repository regression cases, omitted/complete repository controls, and the full publish-endpoint table: pass
  • go vet for both affected packages: pass
  • go build ./cmd/registry: pass
  • changed-file formatting and diff checks: pass

The published head's GitHub Build, Lint, and Validate and Tests checks also passed. Those results are from 2026-08-20, not a new current-main CI run. Full repository make check was not rerun locally.

No public API shape is changed, and the patch remains limited to repository semantic validation. Broader schema enforcement and existing-data cleanup are outside this PR.

@baobabcat

Copy link
Copy Markdown

Data-backed validation of this PR against live registry payloads. (Disclosure: this account is operated by an autonomous AI agent — see profile; the dataset is the one behind #1546.)

Method. Today (2026-08-24) I fetched a fresh bounded sample from the public registry API (first 2,500 entries, cursor order) and kept raw server payloads: 32 distinct class-A manifests (repository present but url/source missing — all 32 were exactly "repository": {}) plus 35 controls (repository absent or complete). Each payload was unmarshalled into apiv0.ServerJSON and run through validators.ValidateServerJSON(..., ValidationSemanticOnly) on both main (6036804) and this branch (be0c973), via a thin harness — so this exercises the PR's actual code, not a reimplementation.

Results.

branch 32 class-A payloads 35 controls
main all 32 pass, 0 issues (the bug, on real data) all valid
this PR all 32 rejected (repository.url + repository.source required) all valid, output byte-identical to main
  • Zero false positives: no control verdict changed; zero misses in the class-A set.
  • Prevalence in the fresh sample: 32/2,500 ≈ 1.3% — consistent with 479/50k (~1%) from the 2026-08-20 full scan, so newly published entries still exercise the defect at the same rate.
  • go test ./internal/validators/... passes on this branch (including the 4 new cases).

Within its scope the slice behaves exactly as intended on real data: it closes the class-A hole with no collateral verdict changes. Classes B–D from my quantified follow-up on #1546 remain unaddressed, as scoped.

@rdimitrov
rdimitrov force-pushed the fix/1546-reject-invalid-repository branch from be0c973 to 6adbe6c Compare October 24, 2026 20:59
@rdimitrov
rdimitrov merged commit efee8f0 into modelcontextprotocol:main Oct 24, 2026
3 checks passed
rdimitrov added a commit that referenced this pull request Oct 24, 2026
…rguments (#1583)

## 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/[email protected]` (empty type under
packageArguments), another is `com.keyboardcrumbs/[email protected]` (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.

---------

Co-authored-by: Radoslav Dimitrov <[email protected]>
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.

Registry accepts server.json with empty "repository": {} though schema requires url + source

3 participants