fix(proxy): preserve fetch semantics through the proxy adapter
Addresses review round 4 on #2099 — three posted comments and three suppressed ones, all valid. - A rebuilt Response reported `url: ""`, `redirected: false`, and `type: "default"` even after undici followed a redirect: a fetch-contract regression visible only to proxy users, and one `core/auth/endpointOverrides.ts` explicitly guards against by declining to rebuild at all. This adapter has no such choice — the response arrives from the wrong realm — so it now reinstates the three with `defineProperty`. - Normalizing a `Request` input dropped every option the Request itself carried beyond method/headers/body/signal. Most visibly `redirect`: a `new Request(url, { redirect: "manual" })` started following redirects instead of surfacing them. All of them are now carried through, with an explicit `init` still winning per the fetch contract. Both are covered by tests driving a real redirect through a real proxy, and both were confirmed to fail without the fix. - `@modelcontextprotocol/ext-apps` was added only to the CLI's `external` list. AGENTS.md requires every root-declared runtime dependency in all three, since which client reaches it is a function of what `core/` imports rather than of the client's own code. Added to the web runner and TUI lists too. - The `BUNDLED_CLIENTS` test compared the hard-coded list against a hard-coded copy of itself, so a new tsup-bundled client would have left it green while going unchecked. It now discovers clients from disk (a client is tsup-bundled iff it ships a tsup config) and compares. Verified it fails when a stub client is added. - AGENTS.md's project tree still described `core/mcp/node/` as only the stdio transport factory; it now carries `proxyFetch.ts` and its role. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
dbe95e58ed
commit
96f96aad41
4 changed files with 135 additions and 17 deletions
|
|
@ -39,10 +39,10 @@ export default defineConfig({
|
|||
"@modelcontextprotocol/client",
|
||||
"@modelcontextprotocol/core",
|
||||
// Root-declared and reached through `core/mcp/apps.ts`, so it must be
|
||||
// external here like every other root runtime dependency — it was being
|
||||
// inlined (dragging the transitive v1 SDK and zod-to-json-schema with it),
|
||||
// which the #2067 guard surfaced. ESM, so it was not failing the way
|
||||
// `undici` did; the rule is what it violated.
|
||||
// external here like every other root runtime dependency (AGENTS.md). This
|
||||
// client was actually inlining it — dragging the transitive v1 SDK and
|
||||
// zod-to-json-schema in with it — which the #2067 guard surfaced. ESM, so it
|
||||
// was not failing the way `undici` did; the rule is what it violated.
|
||||
"@modelcontextprotocol/ext-apps",
|
||||
"commander",
|
||||
"pino",
|
||||
|
|
|
|||
|
|
@ -127,6 +127,12 @@ export default defineConfig({
|
|||
"pino",
|
||||
"@modelcontextprotocol/client",
|
||||
"@modelcontextprotocol/core",
|
||||
// Root-declared and reached through `core/mcp/apps.ts`, so it must be
|
||||
// external here like every other root runtime dependency — which client
|
||||
// actually reaches it is a function of what `core/` imports, not of this
|
||||
// client's own code, so all three lists carry it (AGENTS.md). The CLI was
|
||||
// inlining it; the #2067 guard surfaced that.
|
||||
"@modelcontextprotocol/ext-apps",
|
||||
"@napi-rs/keyring",
|
||||
// Root-declared (see the repo's dependency-placement rule) and CJS, which
|
||||
// is the combination that bites: tsup externalizes what the *client's*
|
||||
|
|
|
|||
|
|
@ -467,6 +467,11 @@ describe("Transport", () => {
|
|||
res.end();
|
||||
return;
|
||||
}
|
||||
if (req.url === "/redirect") {
|
||||
res.writeHead(302, { location: `${originUrl}/landed` });
|
||||
res.end();
|
||||
return;
|
||||
}
|
||||
res.writeHead(200, { "content-type": "text/plain", "x-origin": "1" });
|
||||
res.end("hello from origin");
|
||||
});
|
||||
|
|
@ -552,6 +557,37 @@ describe("Transport", () => {
|
|||
expect(res.body).toBeNull();
|
||||
});
|
||||
|
||||
it("reports url/redirected/type after following a redirect", async () => {
|
||||
// A rebuilt Response cannot carry these through its constructor, so they
|
||||
// are reinstated explicitly. Without that, a proxied response reports
|
||||
// `url: ""` and `redirected: false` even when undici did follow a
|
||||
// redirect — a fetch-contract regression only proxy users would see.
|
||||
clearProxyEnv();
|
||||
process.env.HTTP_PROXY = proxyUrl;
|
||||
const proxyFetch = createProxyFetch();
|
||||
|
||||
const res = await proxyFetch!(`${originUrl}/redirect`);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.url).toBe(`${originUrl}/landed`);
|
||||
expect(res.redirected).toBe(true);
|
||||
expect(res.type).toBe("basic");
|
||||
});
|
||||
|
||||
it("honors a Request's own redirect mode", async () => {
|
||||
// The Request carries options the adapter must not drop on the way into
|
||||
// undici. `redirect: "manual"` is the visible one: lose it and a caller
|
||||
// asking to see the 302 silently gets the followed 200 instead.
|
||||
clearProxyEnv();
|
||||
process.env.HTTP_PROXY = proxyUrl;
|
||||
const proxyFetch = createProxyFetch();
|
||||
|
||||
const res = await proxyFetch!(
|
||||
new Request(`${originUrl}/redirect`, { redirect: "manual" }),
|
||||
);
|
||||
expect(res.status).toBe(302);
|
||||
expect(res.redirected).toBe(false);
|
||||
});
|
||||
|
||||
it("accepts a URL instance and forwards the init", async () => {
|
||||
clearProxyEnv();
|
||||
process.env.HTTP_PROXY = proxyUrl;
|
||||
|
|
|
|||
|
|
@ -44,6 +44,12 @@ export default defineConfig({
|
|||
"proper-lockfile",
|
||||
"@modelcontextprotocol/client",
|
||||
"@modelcontextprotocol/core",
|
||||
// Root-declared and reached through `core/mcp/apps.ts`, so it must be
|
||||
// external here like every other root runtime dependency — which client
|
||||
// actually reaches it is a function of what `core/` imports, not of this
|
||||
// client's own code, so all three lists carry it (AGENTS.md). The CLI was
|
||||
// inlining it; the #2067 guard surfaced that.
|
||||
"@modelcontextprotocol/ext-apps",
|
||||
],
|
||||
esbuildOptions(options) {
|
||||
options.alias = {
|
||||
|
|
|
|||
Loading…
Reference in a new issue