Skip to content

Commit 30de0c4

Browse files
author
Griffin Evans
committed
Make tool refresh best effort instead of failing save so we can't end up in a weird state
1 parent 1481b29 commit 30de0c4

7 files changed

Lines changed: 213 additions & 8 deletions

File tree

packages/plugins/mcp/src/api/group.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,11 @@ const ConfigureServerPayload = Schema.Struct({
101101

102102
const ConfigureServerResponse = Schema.Struct({
103103
config: McpIntegrationConfig,
104+
toolsRefreshFailed: Schema.Boolean,
105+
});
106+
107+
const RefreshServerToolsResponse = Schema.Struct({
108+
toolsRefreshFailed: Schema.Boolean,
104109
});
105110

106111
// The configureAuth payload/response — custom auth methods to merge-append
@@ -176,6 +181,13 @@ export const McpGroup = HttpApiGroup.make("mcp")
176181
error: [InternalError, McpConnectionError, McpToolDiscoveryError, IntegrationNotFound],
177182
}),
178183
)
184+
.add(
185+
HttpApiEndpoint.post("refreshServerTools", "/mcp/servers/:slug/tools/refresh", {
186+
params: SlugParams,
187+
success: RefreshServerToolsResponse,
188+
error: [InternalError, McpConnectionError, McpToolDiscoveryError, IntegrationNotFound],
189+
}),
190+
)
179191
.add(
180192
HttpApiEndpoint.post("configureAuth", "/mcp/servers/:slug/auth", {
181193
params: SlugParams,

packages/plugins/mcp/src/api/handlers.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ const failingExtension: McpPluginExtension = {
3030
reconcileStdioConnections: () => unused,
3131
getServer: () => Effect.succeed(null),
3232
configureServer: () => unused,
33+
refreshServerTools: () => unused,
3334
configureAuth: () => unused,
3435
};
3536

packages/plugins/mcp/src/api/handlers.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,15 @@ export const McpHandlers = HttpApiBuilder.group(ExecutorApiWithMcp, "mcp", (hand
139139
capture(
140140
Effect.gen(function* () {
141141
const ext = yield* McpExtensionService;
142-
yield* ext.configureServer(path.slug, payload.config);
143-
return { config: payload.config };
142+
return yield* ext.configureServer(path.slug, payload.config);
143+
}),
144+
),
145+
)
146+
.handle("refreshServerTools", ({ params: path }) =>
147+
capture(
148+
Effect.gen(function* () {
149+
const ext = yield* McpExtensionService;
150+
return yield* ext.refreshServerTools(path.slug);
144151
}),
145152
),
146153
)

packages/plugins/mcp/src/react/EditMcpSource.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,13 @@ function StdioEdit(props: {
286286
setError(errorMessageFromExit(exit, "Failed to update command settings"));
287287
return { ok: false };
288288
}
289+
if (exit.value.toolsRefreshFailed) {
290+
return {
291+
ok: true,
292+
summary:
293+
"Command settings updated, but tools could not be refreshed. Check secrets or retry.",
294+
};
295+
}
289296
return { ok: true, summary: "Command settings updated." };
290297
}, [argsDraft, commandDraft, cwdDraft, doConfigure, envDraft, server.config, server.slug]);
291298

packages/plugins/mcp/src/react/atoms.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,5 +26,6 @@ export const probeMcpEndpoint = McpClient.mutation("mcp", "probeEndpoint");
2626
export const addMcpServer = McpClient.mutation("mcp", "addServer");
2727
export const removeMcpServer = McpClient.mutation("mcp", "removeServer");
2828
export const configureMcpServer = McpClient.mutation("mcp", "configureServer");
29+
export const refreshMcpServerTools = McpClient.mutation("mcp", "refreshServerTools");
2930
// Merge-append auth methods onto an integration's `authenticationTemplate`.
3031
export const configureMcpAuth = McpClient.mutation("mcp", "configureAuth");

packages/plugins/mcp/src/sdk/plugin.test.ts

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,27 @@ const withStdioFixtureScript = Effect.acquireRelease(
155155
({ dir }) => Effect.promise(() => Fs.rm(dir, { recursive: true, force: true })),
156156
);
157157

158+
const withFlakyStdioRefreshScript = Effect.acquireRelease(
159+
Effect.gen(function* () {
160+
const root = Path.join(process.cwd(), ".tmp");
161+
yield* Effect.promise(() => Fs.mkdir(root, { recursive: true }));
162+
const dir = yield* Effect.promise(() => Fs.mkdtemp(Path.join(root, "mcp-stdio-flaky-")));
163+
const script = Path.join(dir, "server.mjs");
164+
const counter = Path.join(dir, "attempts.txt");
165+
yield* Effect.promise(() =>
166+
Fs.writeFile(
167+
script,
168+
`import fs from "node:fs";\nimport { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";\nimport { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js";\nconst toolName = process.argv[2] ?? "two";\nconst counter = process.argv[3];\nconst previous = fs.existsSync(counter) ? Number(fs.readFileSync(counter, "utf8")) : 0;\nconst attempt = previous + 1;\nfs.writeFileSync(counter, String(attempt));\nif (attempt === 2) process.exit(1);\nconst server = new McpServer({ name: "stdio-flaky-fixture", version: "1.0.0" }, { capabilities: {} });\nserver.registerTool(toolName, { description: "Stdio flaky fixture tool", inputSchema: {} }, async () => ({ content: [{ type: "text", text: "ok" }] }));\nawait server.connect(new StdioServerTransport());\n`,
169+
),
170+
);
171+
return { dir, script, counter } as const;
172+
}),
173+
({ dir }) => Effect.promise(() => Fs.rm(dir, { recursive: true, force: true })),
174+
);
175+
176+
const readStdioAttemptCount = (counter: string) =>
177+
Effect.promise(() => Fs.readFile(counter, "utf8")).pipe(Effect.map((value) => Number(value)));
178+
158179
const seedCallToolExecutor = (input: { slug: string; callTool: CallToolResponder }) =>
159180
Effect.acquireRelease(
160181
Effect.gen(function* () {
@@ -649,6 +670,86 @@ describe("mcpPlugin", () => {
649670
),
650671
);
651672

673+
it.effect("configureServer reports post-commit stdio refresh failure as a warning result", () =>
674+
Effect.scoped(
675+
Effect.gen(function* () {
676+
const fixture = yield* withStdioFixtureScript;
677+
const flaky = yield* withFlakyStdioRefreshScript;
678+
const executor = yield* createExecutor(
679+
makeTestConfig({
680+
plugins: [
681+
memoryCredentialsPlugin(),
682+
mcpPlugin({ dangerouslyAllowStdioMCP: true }),
683+
] as const,
684+
}),
685+
);
686+
687+
yield* executor.mcp.addServer({
688+
transport: "stdio",
689+
name: "Stdio refresh warning",
690+
slug: "stdio_refresh_warning",
691+
command: process.execPath,
692+
args: [fixture.script, "one"],
693+
});
694+
695+
const result: unknown = yield* executor.mcp.configureServer("stdio_refresh_warning", {
696+
transport: "stdio",
697+
command: process.execPath,
698+
args: [flaky.script, "two", flaky.counter],
699+
authenticationTemplate: [{ slug: "none", kind: "none" }],
700+
});
701+
702+
expect(result).toMatchObject({ toolsRefreshFailed: true });
703+
expect(yield* readStdioAttemptCount(flaky.counter)).toBe(2);
704+
705+
const integration = yield* executor.mcp.getServer("stdio_refresh_warning");
706+
expect(integration?.config).toMatchObject({
707+
command: process.execPath,
708+
args: [flaky.script, "two", flaky.counter],
709+
});
710+
}),
711+
),
712+
);
713+
714+
it.effect("refreshServerTools retries stdio tool refresh after a warning result", () =>
715+
Effect.scoped(
716+
Effect.gen(function* () {
717+
const fixture = yield* withStdioFixtureScript;
718+
const flaky = yield* withFlakyStdioRefreshScript;
719+
const executor = yield* createExecutor(
720+
makeTestConfig({
721+
plugins: [
722+
memoryCredentialsPlugin(),
723+
mcpPlugin({ dangerouslyAllowStdioMCP: true }),
724+
] as const,
725+
}),
726+
);
727+
const config = {
728+
transport: "stdio" as const,
729+
command: process.execPath,
730+
args: [flaky.script, "two", flaky.counter],
731+
authenticationTemplate: [{ slug: "none" as const, kind: "none" as const }],
732+
};
733+
734+
yield* executor.mcp.addServer({
735+
transport: "stdio",
736+
name: "Stdio refresh retry",
737+
slug: "stdio_refresh_retry",
738+
command: process.execPath,
739+
args: [fixture.script, "one"],
740+
});
741+
742+
const configureResult = yield* executor.mcp.configureServer("stdio_refresh_retry", config);
743+
expect(configureResult).toMatchObject({ toolsRefreshFailed: true });
744+
expect(yield* readStdioAttemptCount(flaky.counter)).toBe(2);
745+
746+
const refreshResult = yield* executor.mcp.refreshServerTools("stdio_refresh_retry");
747+
expect(refreshResult).toMatchObject({ toolsRefreshFailed: false });
748+
expect(yield* readStdioAttemptCount(flaky.counter)).toBe(4);
749+
}),
750+
),
751+
);
752+
652753
it.effect("configureServer reports missing stdio secret values before preflight", () =>
653754
Effect.scoped(
654755
Effect.gen(function* () {

packages/plugins/mcp/src/sdk/plugin.ts

Lines changed: 82 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Effect, Layer, Option, Predicate, Result, Schema } from "effect";
1+
import { Cause, Effect, Layer, Option, Predicate, Result, Schema } from "effect";
22
import type { HttpClient } from "effect/unstable/http";
33

44
import type { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js";
@@ -1106,8 +1106,25 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => {
11061106

11071107
const refreshStdioConnections = (integration: IntegrationSlug) =>
11081108
Effect.gen(function* () {
1109+
const record = yield* ctx.core.integrations.get(integration);
1110+
const config = record ? parseMcpIntegrationConfig(record.config) : null;
1111+
if (!config || config.transport !== "stdio") {
1112+
return yield* new McpConnectionError({
1113+
transport: "stdio",
1114+
message: `Cannot refresh tools for MCP integration ${integration}: stdio config not found.`,
1115+
});
1116+
}
1117+
11091118
const connections = yield* ctx.connections.list({ integration });
11101119
if (connections.length === 0) {
1120+
const requiredVars = requiredStdioEnvVars(config);
1121+
if (requiredVars.length > 0) {
1122+
return yield* new McpConnectionError({
1123+
transport: "stdio",
1124+
message: `Cannot refresh tools because no visible connection has values for required environment variables: ${requiredVars.join(", ")}.`,
1125+
});
1126+
}
1127+
11111128
yield* ctx.connections
11121129
.create({
11131130
owner: "org",
@@ -1136,7 +1153,18 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => {
11361153
}
11371154

11381155
const failedNames = yield* Effect.forEach(connections, (connection) =>
1139-
ctx.connections.refresh(connection).pipe(
1156+
Effect.gen(function* () {
1157+
const values = yield* ctx.connections.resolveValues(connection);
1158+
const connectorInput = yield* buildConnectorInput(
1159+
config,
1160+
values,
1161+
String(connection.template),
1162+
allowStdio,
1163+
httpClientLayer,
1164+
);
1165+
yield* discoverTools(createMcpConnector(connectorInput));
1166+
yield* ctx.connections.refresh(connection);
1167+
}).pipe(
11401168
Effect.as(null),
11411169
Effect.catch(() => Effect.succeed(String(connection.name))),
11421170
),
@@ -1150,6 +1178,17 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => {
11501178
}
11511179
});
11521180

1181+
const refreshStdioToolsBestEffort = (integration: IntegrationSlug) =>
1182+
refreshStdioConnections(integration).pipe(
1183+
Effect.as(false),
1184+
Effect.catchCause((cause) =>
1185+
Effect.logWarning("stdio tools refresh failed after config commit").pipe(
1186+
Effect.annotateLogs("cause", Cause.pretty(cause)),
1187+
Effect.as(true),
1188+
),
1189+
),
1190+
);
1191+
11531192
const configureServer = (slug: string, config: McpIntegrationConfigType) =>
11541193
Effect.gen(function* () {
11551194
const integration = slugFrom(slug);
@@ -1167,7 +1206,7 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => {
11671206

11681207
if (current.transport !== "stdio") {
11691208
yield* ctx.core.integrations.update(integration, { config });
1170-
return;
1209+
return { config, toolsRefreshFailed: false } satisfies McpConfigureServerResult;
11711210
}
11721211

11731212
if (config.transport !== "stdio") {
@@ -1183,7 +1222,7 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => {
11831222
);
11841223
if (!processConfigChanged) {
11851224
yield* ctx.core.integrations.update(integration, { config });
1186-
return;
1225+
return { config, toolsRefreshFailed: false } satisfies McpConfigureServerResult;
11871226
}
11881227

11891228
yield* preflightStdioConfig(integration, config);
@@ -1193,13 +1232,37 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => {
11931232
yield* ctx.core.integrations.update(integration, { config });
11941233
}),
11951234
);
1196-
yield* refreshStdioConnections(integration);
1235+
const toolsRefreshFailed = yield* refreshStdioToolsBestEffort(integration);
1236+
return { config, toolsRefreshFailed } satisfies McpConfigureServerResult;
11971237
}).pipe(
11981238
Effect.withSpan("mcp.plugin.configure_server", {
11991239
attributes: { "mcp.integration.slug": slug },
12001240
}),
12011241
);
12021242

1243+
const refreshServerTools = (slug: string) =>
1244+
Effect.gen(function* () {
1245+
const integration = slugFrom(slug);
1246+
const record = yield* ctx.core.integrations.get(integration);
1247+
const current = record ? parseMcpIntegrationConfig(record.config) : null;
1248+
if (record === null || current === null) {
1249+
return yield* new IntegrationNotFoundError({ slug: integration });
1250+
}
1251+
if (current.transport !== "stdio") {
1252+
return yield* new McpConnectionError({
1253+
transport: "auto",
1254+
message:
1255+
"MCP tool refresh through the MCP server API is only supported for stdio servers.",
1256+
});
1257+
}
1258+
const toolsRefreshFailed = yield* refreshStdioToolsBestEffort(integration);
1259+
return { toolsRefreshFailed } satisfies McpRefreshServerToolsResult;
1260+
}).pipe(
1261+
Effect.withSpan("mcp.plugin.refresh_server_tools", {
1262+
attributes: { "mcp.integration.slug": slug },
1263+
}),
1264+
);
1265+
12031266
/** Merge-append auth methods onto the integration's existing
12041267
* `authenticationTemplate` (custom-method-create flow), mirroring the
12051268
* OpenAPI/GraphQL `configureAuth`. Returns the merged array. A no-op
@@ -1245,6 +1308,7 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => {
12451308
reconcileStdioConnections,
12461309
getServer,
12471310
configureServer,
1311+
refreshServerTools,
12481312
configureAuth,
12491313
};
12501314
},
@@ -1575,6 +1639,15 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => {
15751639

15761640
export type McpExtensionFailure = McpConnectionError | McpToolDiscoveryError | StorageFailure;
15771641

1642+
export interface McpConfigureServerResult {
1643+
readonly config: McpIntegrationConfigType;
1644+
readonly toolsRefreshFailed: boolean;
1645+
}
1646+
1647+
export interface McpRefreshServerToolsResult {
1648+
readonly toolsRefreshFailed: boolean;
1649+
}
1650+
15781651
export interface McpPluginExtension {
15791652
readonly probeEndpoint: (
15801653
input: string | McpProbeEndpointInput,
@@ -1598,7 +1671,10 @@ export interface McpPluginExtension {
15981671
readonly configureServer: (
15991672
slug: string,
16001673
config: McpIntegrationConfigType,
1601-
) => Effect.Effect<void, McpExtensionFailure | IntegrationNotFoundError>;
1674+
) => Effect.Effect<McpConfigureServerResult, McpExtensionFailure | IntegrationNotFoundError>;
1675+
readonly refreshServerTools: (
1676+
slug: string,
1677+
) => Effect.Effect<McpRefreshServerToolsResult, McpExtensionFailure | IntegrationNotFoundError>;
16021678
readonly configureAuth: (
16031679
slug: string,
16041680
input: McpConfigureAuthInput,

0 commit comments

Comments
 (0)