fix(cli): keep dotted tool names in a call selector - #333
Conversation
resolveCallTarget split the selector with `split('.', 2)`, so everything after
the second dot was discarded and `mcporter call <server>.<a>.<b>` dispatched to
the tool named `<a>`. Tool names containing dots are common, and the module
already resolves them correctly three other ways: the ad-hoc HTTP branch uses
splitServerToolSelector, the call-expression parser rejoins the remainder, and
list matches the configured server name first.
Route the configured-server branch through the existing helper so the four
surfaces agree, keeping the current errors for a selector that starts or ends
with a dot.
…ector splitHttpToolSelector rebuilt the server URL from `url.origin` plus a hand-built path, so `https://host/mcp.tool?tenant=b` resolved to a different endpoint than the user typed. The connection then targets the wrong tenant, and the stripped URL no longer matches a configured server whose URL carries a query, so the CLI falls through to an ad-hoc server. normalizeHttpUrl in the same module already round-trips through URL; do the same here so both functions answer with the URL they were given.
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2a11a2558
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Serialize through URL rather than concatenating the origin, so the query and fragment the | ||
| // user typed reach the server instead of being dropped from the connection target. | ||
| url.pathname = basePath.startsWith('/') ? basePath : `/${basePath}`; | ||
| return { baseUrl: url.href, tool }; |
There was a problem hiding this comment.
When a quoted selector contains a fragment (for example, https://example.com/mcp.tool#frag) but the configured server uses the normal fragment-free URL, returning url.href makes findServerByHttpUrl compare https://example.com/mcp#frag against https://example.com/mcp and miss the configured definition. prepareEphemeralServerTarget then registers an ad-hoc server, dropping configured headers, OAuth settings, and other options. Since URL fragments are never sent in HTTP requests and cannot identify a different MCP endpoint, preserve the query but clear url.hash before returning the base URL.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ea240b5.
I measured the case you describe before changing anything:
splitHttpToolSelector('https://example.com/mcp.tool#frag').baseUrl
-> 'https://example.com/mcp#frag'
normalizeHttpUrl(that) -> 'https://example.com/mcp#frag'
normalizeHttpUrl('https://example.com/mcp') -> 'https://example.com/mcp'
match -> false
So the fragment did exactly what you said: it only affected comparison, and the configured definition was missed. The commit keeps url.search and clears url.hash, and the test now asserts both halves - the query survives, and a fragment selector still matches a fragment-free configured server.
|
The Windows job failed in
The same case is flaky in my local runs too, which is why the PR body called it out: running the three Node-24-sensitive suites on For a control: #332 was opened from the same base commit a few minutes earlier and passed the same I do not have rerun rights here. Say the word and I will rebase to trigger a fresh run, or open a separate PR if you would rather have the |
A fragment is never sent on the wire, so keeping it on the resolved base URL only affected comparison: findServerByHttpUrl normalizes both sides, so a selector written with a fragment stopped matching a configured server whose URL has none, and the CLI fell through to an ad-hoc server without its headers or OAuth settings. Keep the query, clear the hash.
clawsweeper
Bot
added
P2
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close this PR as superseded by #325. So I’m closing this here and keeping the remaining discussion on #325. Review detailsDo we have a high-confidence way to reproduce the issue? Yes—current-main source directly shows both faulty transformations, and the PR supplies focused before/after terminal evidence plus regression cases for each path. Is this the best way to solve the issue? Yes—the patch reuses the existing first-dot helper and URL serializer rather than adding a parallel parsing or normalization path. Security review: Security review cleared: The four-file diff adds no dependency, workflow, permission, secret-handling, or new code-execution surface; it preserves the user-selected HTTP query and drops the non-transmitted fragment. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against ae3d9000c320. |
1 participant
Summary
mcporter call <server>.<tool>truncates the tool name at the second dot, so a tool whose name contains a dot is never reachable through the configured-server path. Tool names likebrowser.navigateare common.Root cause
resolveCallTargetsplits the selector with a limit of two:'proof.browser.navigate'.split('.', 2)is['proof', 'browser'], so the trailing segment is dropped.The module already resolves the same string correctly three other ways, and the correct helper sits nineteen lines below the defect in the same file:
src/cli/call-expression-parser.ts:95rejoins the remainder, andsrc/cli/list-command.ts:552matches the longest configured server name first, somcporter listresolves the selector thatmcporter callcannot.Fix
Route the configured-server branch through the existing helper. A selector that starts or ends with a dot still produces the same
Missing server name./Missing tool name.errors as before, since the helper returnsundefinedfor both and the fallback keeps the current split point.Adjacent finding: the HTTP tool selector drops the query
Second and third commits, drop them if you would rather not take them - four lines in
http-utils.tsplus four test cases.splitHttpToolSelectorrebuilds the server URL by concatenatingurl.originwith a hand-built path, so everything after the path is discarded:normalizeHttpUrl, eight lines below in the same module, round-trips throughURLand keeps the query - that was the change in #325. This function was not part of it, so the two now answer differently for one input:The consequence is not cosmetic: the connection is opened against a different endpoint than the user typed, and because
findServerByHttpUrlcompares throughnormalizeHttpUrl, a configured server whose URL carries a query no longer matches the stripped target, so the CLI silently falls through to an ad-hoc server.Behavior proof
Dotted tool name. A stdio fixture advertising one tool literally named
browser.navigate:HTTP selector query. A local HTTP server that logs the request line and answers 404, so the only thing being measured is where the request went. Same port, same command, both revisions:
Tests
Three new cases, all red against unpatched
main:Two further cases pass on both revisions and are there to pin behaviour rather than change it:
keeps the port and the encoded path segments, andlets a configured server without a fragment still match a fragment selector.The fragment half of the third commit came out of review: keeping
url.hashwould have been a new way to miss a configured server, sincefindServerByHttpUrlnormalizes both sides and a fragment is never sent on the wire. Measured before changing it:Gates
Local runtime is Node 22.20.0 while the repo asks for
>=24, so three suites fail before this branch as well:tests/chrome-devtools-relay-handoff.test.ts,tests/runtime-chrome-relay-handoff.test.tsandtests/oauth-refresh-process.integration.test.ts.Running those three on their own and diffing the failing test names between
mainand this branch leaves a single difference,redeems a rotating refresh token exactly once across concurrent processes (provider path), which is flaky here: it failed onmainin an earlier run of the same three files and passed onmainin the run used for the diff. Every other failing name is identical on both revisions.Also ran the nine call-related suites plus every consumer of
splitHttpToolSelector/extractHttpServerTarget(command-inference,generate/definition,generate/flags,emit-ts-command,list-command): 104 and 45 passed respectively.Production delta is +10/-6 across two files.