Skip to content

fix(core-internal): match query variables that expand omitted - #3147

Open
lhy-dcf wants to merge 1 commit into
modelcontextprotocol:mainfrom
lhy-dcf:fix/uritemplate-optional-query
Open

lhy-dcf wants to merge 1 commit into
modelcontextprotocol:mainfrom
lhy-dcf:fix/uritemplate-optional-query

Conversation

@lhy-dcf

@lhy-dcf lhy-dcf commented Nov 4, 2026

Copy link
Copy Markdown

What breaks

UriTemplate disagrees with itself for query expressions. expand treats a query variable as optional — it drops the pair when the variable is undefined:

new UriTemplate('/search{?q,page}').expand({ q: 'test' }); // '/search?q=test'

match does not. partToRegExp builds one mandatory group per name:

pattern: prefix + this.escapeRegExp(name) + '=([^&]+)'

so the matcher demands every pair in the template:

const t = new UriTemplate('/search{?q,page}');
t.match(t.expand({ q: 'test' }));        // null   <- a URI it just produced
t.match('/search?q=test');               // null
t.match('/search?q=test&page=1');        // { q: 'test', page: '1' }

A client that registers a resource template with optional query variables ({?cursor,limit} is the common shape: the server expands it with whatever it needs, and the client then asks whether a resource URI belongs to the template) can never match a partially expanded URI. The second failure mode is the same bug seen from the other side: expand({ q: '' }) emits /search?q=, and ([^&]+) refuses that too, because it requires at least one character.

Fix

Each query pair becomes optional and the value may be empty:

pattern: `(?:${prefix}${this.escapeRegExp(name)}=([^&]*))?`

A group that did not participate is skipped rather than turned into undefined:

const value = match[i + 1];
if (value === undefined) continue;

match(t.expand(vars)) can then no longer be null for reasons the class itself created, and an omitted variable is simply absent from the result ({ q: 'test' }, not { q: 'test', page: undefined }).

Order, literal text and the mandatory path segments are untouched: the groups stay in template order and every other operator keeps its existing pattern.

Tests

Three cases added to packages/core-internal/test/shared/uriTemplate.test.ts (matching complex patterns):

  1. match(expand({ q: 'test' })) → { q: 'test' }
  2. match('/search?q=test') → { q: 'test' }
  3. control: match('/search?q=test&page=1') → { q: 'test', page: '1' } (unchanged)

Results (vitest, Node v22.22.2, Windows):

  • before: 2 failed | 39 passed — cases 1 and 2 fail with expected null to deeply equal { q: 'test' }, the control and all 36 pre-existing cases pass;
  • after: 41 passed (36 pre-existing + 2 new + control).

Counter-proof: the new tests were re-run with uriTemplate.ts restored byte-for-byte from main and only the test file kept — the same two cases fail, the control still passes.

Evidence boundary

  • The suite was executed against a copy of uriTemplate.ts in a standalone vitest workspace (this machine has no pnpm install of the repo), with the repo's own test file; the assertions use only the module's public API.
  • prettier --check with the repo config (.prettierrc.json: 4 spaces, single quotes, printWidth 140) passes on both files.
  • tsc --noEmit --strict passes on uriTemplate.ts; match[i + 1] narrowing to string | undefined is what the new guard consumes.
  • Not verified: behaviour of consumers that pass templates with query expressions only ({?a} against a bare path now returns {}); that is the documented expansion of an empty variable set, but it is a visible semantic change for such callers and CI will run the package's own suites.

`UriTemplate.match` built one mandatory group per query name:

    prefix + name + '=([^&]+)'

so a template like `/search{?q,page}` only matched a URI carrying *every*
pair. `expand` is the opposite: it drops a pair whose variable is undefined,
and it emits `name=` for an empty string. `match(expand({ q: 'test' }))`
therefore returned `null` for a URI the class had just produced.

Each query pair is now optional — `(?:\?name=([^&]*))?` — and a group that did
not participate is skipped instead of being read as `undefined`. The full-query
case is unchanged, and `/search` alone now matches the documented expansion of
an empty variable set.
@lhy-dcf
lhy-dcf requested a review from a team as a code owner November 4, 2026 13:19
@changeset-bot

changeset-bot Bot commented Nov 4, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 8a4dda1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Nov 4, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@3147

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3147

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@3147

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@3147

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3147

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@3147

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3147

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3147

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@3147

commit: 8a4dda1

@claude claude Bot added the v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes label Nov 5, 2026

This branch has not been deployed

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

Labels

v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant