Skip to content

Commit 91e08b9

Browse files
committed
refactor(mcp): drop result projection and duplicate tool-call paths
1 parent e0e31cd commit 91e08b9

14 files changed

Lines changed: 190 additions & 933 deletions

‎e2e/mcp-late-dispatch.test.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,17 @@ function linearClient(opts: {
5555
},
5656
],
5757
call: opts.call ?? (async () => "ISSUE-1"),
58+
async callResult(toolName, args, signal) {
59+
const text = await (opts.call ?? (async () => "ISSUE-1"))(
60+
toolName,
61+
args,
62+
signal,
63+
);
64+
return {
65+
blocks: [{ type: "text", text }],
66+
isError: false,
67+
};
68+
},
5869
async close() {
5970
return undefined;
6071
},

‎e2e/subagent-permission.test.ts‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -296,9 +296,13 @@ test.serial(
296296
inputSchema: { type: "object", properties: {} },
297297
},
298298
],
299-
call: async () => {
299+
call: async () => "changed",
300+
callResult: async () => {
300301
calls++;
301-
return "changed";
302+
return {
303+
blocks: [{ type: "text", text: "changed" }],
304+
isError: false,
305+
};
302306
},
303307
close: async () => undefined,
304308
};
@@ -350,9 +354,13 @@ test.serial(
350354
inputSchema: { type: "object", properties: {} },
351355
},
352356
],
353-
call: async () => {
357+
call: async () => "changed",
358+
callResult: async () => {
354359
calls++;
355-
return "changed";
360+
return {
361+
blocks: [{ type: "text", text: "changed" }],
362+
isError: false,
363+
};
356364
},
357365
close: async () => undefined,
358366
};
@@ -480,9 +488,13 @@ test.serial(
480488
inputSchema: { type: "object", properties: {} },
481489
},
482490
],
483-
call: async () => {
491+
call: async () => "changed",
492+
callResult: async () => {
484493
calls++;
485-
return "changed";
494+
return {
495+
blocks: [{ type: "text", text: "changed" }],
496+
isError: false,
497+
};
486498
},
487499
close: async () => undefined,
488500
};
@@ -561,9 +573,13 @@ test("storing parent-gated MCP tools then wrapping again denies without requestA
561573
inputSchema: { type: "object", properties: {} },
562574
},
563575
],
564-
call: async () => {
576+
call: async () => "changed",
577+
callResult: async () => {
565578
calls++;
566-
return "changed";
579+
return {
580+
blocks: [{ type: "text", text: "changed" }],
581+
isError: false,
582+
};
567583
},
568584
close: async () => undefined,
569585
};

‎src/mcp/add-server.ts‎

Lines changed: 36 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -14,44 +14,34 @@ import {
1414
import { deleteAuthState } from "./auth-store.js";
1515
import { EXA_MCP_SERVER_NAME } from "./exa.js";
1616

17-
export interface GlobalSettingsWriter {
18-
readonly enqueue: <T>(job: () => Promise<T>) => Promise<T>;
19-
readonly update: (
20-
apply: (base: Settings) => Settings | null,
21-
) => Promise<Settings | null>;
17+
interface SettingsWriter<T> {
18+
readonly enqueue: <R>(job: () => Promise<R>) => Promise<R>;
19+
readonly update: (apply: (base: T) => T | null) => Promise<T | null>;
2220
readonly updateAt: (
2321
path: string,
24-
apply: (base: Settings) => Settings | null,
25-
) => Promise<Settings | null>;
26-
readonly mutate: (
27-
apply: (base: Settings) => Settings | null,
28-
) => Promise<"ok" | "skipped">;
22+
apply: (base: T) => T | null,
23+
) => Promise<T | null>;
24+
readonly mutate: (apply: (base: T) => T | null) => Promise<"ok" | "skipped">;
2925
readonly mutateAt: (
3026
path: string,
31-
apply: (base: Settings) => Settings | null,
27+
apply: (base: T) => T | null,
3228
) => Promise<"ok" | "skipped">;
3329
}
3430

35-
interface GlobalSettingsWriterDeps {
36-
readonly load: (path: string) => Promise<Settings | null>;
37-
readonly save: (path: string, settings: Settings) => Promise<void>;
38-
}
31+
export type GlobalSettingsWriter = SettingsWriter<Settings>;
32+
export type LocalSettingsWriter = SettingsWriter<LocalSettings>;
3933

40-
const defaultWriterDeps: GlobalSettingsWriterDeps = {
41-
load: loadGlobalSettingsWriteBase,
42-
save: saveGlobalSettings,
43-
};
34+
interface SettingsWriterDeps<T> {
35+
readonly load: (path: string) => Promise<T | null>;
36+
readonly save: (path: string, settings: T) => Promise<void>;
37+
}
4438

45-
export function createGlobalSettingsWriter(
39+
function createSettingsWriter<T>(
4640
path: string,
47-
deps: Partial<GlobalSettingsWriterDeps> = {},
48-
): GlobalSettingsWriter {
49-
const writerDeps: GlobalSettingsWriterDeps = {
50-
...defaultWriterDeps,
51-
...deps,
52-
};
41+
deps: SettingsWriterDeps<T>,
42+
): SettingsWriter<T> {
5343
let tail = Promise.resolve();
54-
const enqueue = <T>(job: () => Promise<T>): Promise<T> => {
44+
const enqueue = <R>(job: () => Promise<R>): Promise<R> => {
5545
const run = tail.then(job);
5646
tail = run.then(
5747
() => undefined,
@@ -62,20 +52,20 @@ export function createGlobalSettingsWriter(
6252

6353
const updateAt = (
6454
settingsPath: string,
65-
apply: (base: Settings) => Settings | null,
66-
): Promise<Settings | null> =>
55+
apply: (base: T) => T | null,
56+
): Promise<T | null> =>
6757
enqueue(async () => {
68-
const base = await writerDeps.load(settingsPath);
58+
const base = await deps.load(settingsPath);
6959
if (base === null) return null;
7060
const next = apply(base);
7161
if (next === null) return base;
72-
await writerDeps.save(settingsPath, next);
62+
await deps.save(settingsPath, next);
7363
return next;
7464
});
7565

7666
const mutateAt = async (
7767
settingsPath: string,
78-
apply: (base: Settings) => Settings | null,
68+
apply: (base: T) => T | null,
7969
): Promise<"ok" | "skipped"> =>
8070
(await updateAt(settingsPath, apply)) === null ? "skipped" : "ok";
8171

@@ -88,78 +78,26 @@ export function createGlobalSettingsWriter(
8878
};
8979
}
9080

91-
export interface LocalSettingsWriter {
92-
readonly enqueue: <T>(job: () => Promise<T>) => Promise<T>;
93-
readonly update: (
94-
apply: (base: LocalSettings) => LocalSettings | null,
95-
) => Promise<LocalSettings | null>;
96-
readonly updateAt: (
97-
path: string,
98-
apply: (base: LocalSettings) => LocalSettings | null,
99-
) => Promise<LocalSettings | null>;
100-
readonly mutate: (
101-
apply: (base: LocalSettings) => LocalSettings | null,
102-
) => Promise<"ok" | "skipped">;
103-
readonly mutateAt: (
104-
path: string,
105-
apply: (base: LocalSettings) => LocalSettings | null,
106-
) => Promise<"ok" | "skipped">;
107-
}
108-
109-
interface LocalSettingsWriterDeps {
110-
readonly load: (path: string) => Promise<LocalSettings | null>;
111-
readonly save: (path: string, settings: LocalSettings) => Promise<void>;
81+
export function createGlobalSettingsWriter(
82+
path: string,
83+
deps: Partial<SettingsWriterDeps<Settings>> = {},
84+
): GlobalSettingsWriter {
85+
return createSettingsWriter(path, {
86+
load: loadGlobalSettingsWriteBase,
87+
save: saveGlobalSettings,
88+
...deps,
89+
});
11290
}
11391

114-
const defaultLocalWriterDeps: LocalSettingsWriterDeps = {
115-
load: loadLocalSettingsWriteBase,
116-
save: saveLocalSettings,
117-
};
118-
11992
export function createLocalSettingsWriter(
12093
path: string,
121-
deps: Partial<LocalSettingsWriterDeps> = {},
94+
deps: Partial<SettingsWriterDeps<LocalSettings>> = {},
12295
): LocalSettingsWriter {
123-
const writerDeps: LocalSettingsWriterDeps = {
124-
...defaultLocalWriterDeps,
96+
return createSettingsWriter(path, {
97+
load: loadLocalSettingsWriteBase,
98+
save: saveLocalSettings,
12599
...deps,
126-
};
127-
let tail = Promise.resolve();
128-
const enqueue = <T>(job: () => Promise<T>): Promise<T> => {
129-
const run = tail.then(job);
130-
tail = run.then(
131-
() => undefined,
132-
() => undefined,
133-
);
134-
return run;
135-
};
136-
137-
const updateAt = (
138-
settingsPath: string,
139-
apply: (base: LocalSettings) => LocalSettings | null,
140-
): Promise<LocalSettings | null> =>
141-
enqueue(async () => {
142-
const base = await writerDeps.load(settingsPath);
143-
if (base === null) return null;
144-
const next = apply(base);
145-
if (next === null) return base;
146-
await writerDeps.save(settingsPath, next);
147-
return next;
148-
});
149-
150-
const mutateAt = async (
151-
settingsPath: string,
152-
apply: (base: LocalSettings) => LocalSettings | null,
153-
): Promise<"ok" | "skipped"> =>
154-
(await updateAt(settingsPath, apply)) === null ? "skipped" : "ok";
155-
156-
return {
157-
enqueue,
158-
update: (apply) => updateAt(path, apply),
159-
updateAt,
160-
mutate: (apply) => mutateAt(path, apply),
161-
mutateAt,
162-
};
100+
});
163101
}
164102

165103
export type PersistMCPServerResult =

‎src/mcp/client.ts‎

Lines changed: 16 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import {
2626
} from "../telemetry/product-events.js";
2727
import { liveTelemetry } from "../telemetry/singleton.js";
2828
import { MCP_CLIENT_NAME } from "../branding.js";
29+
import { closeQuietly, isAbortError } from "./quiet.js";
2930

3031
export interface MCPTool {
3132
name: string;
@@ -60,16 +61,9 @@ export interface MCPClient {
6061
args: Record<string, unknown>,
6162
signal: AbortSignal,
6263
): Promise<string>;
63-
/** Validated content blocks before flattening — for post-policy
64-
* archive capture. */
65-
callBlocks?(
66-
toolName: string,
67-
args: Record<string, unknown>,
68-
signal: AbortSignal,
69-
): Promise<MCPContentBlock[]>;
7064
/** Full tool-result envelope: blocks plus tool-level
7165
* isError/structuredContent. */
72-
callResult?(
66+
callResult(
7367
toolName: string,
7468
args: Record<string, unknown>,
7569
signal: AbortSignal,
@@ -176,15 +170,6 @@ function isRecoverableAuthError(
176170
return err instanceof UnauthorizedError || err instanceof OAuthError;
177171
}
178172

179-
function isAbortError(err: unknown): boolean {
180-
return (
181-
typeof err === "object" &&
182-
err !== null &&
183-
"name" in err &&
184-
err.name === "AbortError"
185-
);
186-
}
187-
188173
export const MAX_BROWSER_AUTH_ATTEMPTS = 1;
189174
export const BROWSER_AUTH_COOLDOWN_MS = 5 * 60_000;
190175
export const BROWSER_AUTH_WAIT_MS = 2 * 60_000;
@@ -612,12 +597,13 @@ async function finishClient(
612597
serverName,
613598
tools,
614599
async callResult(toolName, args, signal) {
615-
const context =
616-
authContext === undefined ? undefined : { ...authContext, signal };
617-
const result = await withHTTPAuthorizationRecovery(context, () =>
618-
client.callTool({ name: toolName, arguments: args }, undefined, {
619-
signal,
620-
}),
600+
const result = await withHTTPAuthorizationRecovery(
601+
authContext,
602+
() =>
603+
client.callTool({ name: toolName, arguments: args }, undefined, {
604+
signal,
605+
}),
606+
signal,
621607
);
622608
const envelope: MCPToolResultEnvelope = {
623609
blocks: validateMcpContentBlocks(result.content),
@@ -634,34 +620,17 @@ async function finishClient(
634620
}
635621
return envelope;
636622
},
637-
async callBlocks(toolName, args, signal) {
638-
const result = await withHTTPAuthorizationRecovery(
639-
authContext,
640-
() =>
641-
client.callTool({ name: toolName, arguments: args }, undefined, {
642-
signal,
643-
}),
644-
signal,
645-
);
646-
return validateMcpContentBlocks(result.content);
647-
},
648-
async call(toolName, args, signal) {
649-
const result = await withHTTPAuthorizationRecovery(
650-
authContext,
651-
() =>
652-
client.callTool({ name: toolName, arguments: args }, undefined, {
653-
signal,
654-
}),
655-
signal,
623+
async call(this: MCPClient, toolName, args, signal) {
624+
return unwrapToolContent(
625+
(await this.callResult(toolName, args, signal)).blocks,
656626
);
657-
return unwrapToolContent(validateMcpContentBlocks(result.content));
658627
},
659628
async close() {
660629
// Disarm first: the owner's hook reports unintentional death only.
661630
if (liveTransport !== undefined) liveTransport.onclose = () => undefined;
662631
closeLifecycle?.();
663632
authContext?.callback.close();
664-
await client.close().catch(() => undefined);
633+
await closeQuietly(() => client.close());
665634
},
666635
};
667636
}
@@ -730,7 +699,7 @@ async function connectStdio(
730699
),
731700
};
732701
} catch (err) {
733-
await client.close().catch(() => undefined);
702+
await closeQuietly(() => client.close());
734703
return {
735704
ok: false,
736705
serverName: config.name,
@@ -798,7 +767,7 @@ async function connectHttp(
798767
signal: lifecycle.signal,
799768
});
800769
} finally {
801-
await probeClient.close().catch(() => undefined);
770+
await closeQuietly(() => probeClient.close());
802771
}
803772
},
804773
};
@@ -855,7 +824,7 @@ async function connectHttp(
855824
};
856825
} catch (err) {
857826
closeLifecycle();
858-
await client?.close().catch(() => undefined);
827+
await closeQuietly(() => client?.close() ?? Promise.resolve());
859828
try {
860829
callback?.close();
861830
} catch {

0 commit comments

Comments
 (0)