History

fix(mcp): report completed connected tool (#1964)

* fix(mcp): report completed connected tool * Test queue timeout with a controlled clock * Verify connected tool provenance over MCP * Exclude upstream tool failures from completion metadata ---------

Dhairyashil Shinde committed 2 months ago

Commit 89b0f8d1 parent d64639b8 files · +191 −14Browse files

Unified · Split

8 files
  1. .changeset/completed-mcp-tool-name.md +5 −0
  2. e2e/scenarios/tool-call-contract.test.ts +96 −6
  3. packages/core/execution/src/engine.test.ts +29 −0
  4. packages/core/execution/src/engine.ts +21 −5
  5. packages/core/execution/src/tool-invoker.test.ts +19 −0
  6. packages/core/execution/src/tool-invoker.ts +10 −1
  7. packages/hosts/mcp/src/tool-server.test.ts +9 −2
  8. packages/kernel/core/src/types.ts +2 −0
.changeset/completed-mcp-tool-name.md+5−0
@@ -0,0 +1,5 @@
1---
2"@executor-js/execution": patch
3---
4
5Completed MCP execute results now include `toolName` when a script successfully uses exactly one connected tool. Executions that use distinct tools remain unlabeled, and internal call provenance is not exposed in the MCP response.
e2e/scenarios/tool-call-contract.test.ts+96−6
@@ -21,7 +21,7 @@ import { randomBytes } from "node:crypto"; · 20 unmodified lines
21import { createServer } from "node:http";21import { createServer } from "node:http";
22 22
23import { expect } from "@effect/vitest";23import { expect } from "@effect/vitest";
24import { Effect } from "effect";24import { Effect, Schema } from "effect";
25import { composePluginApi } from "@executor-js/api/server";25import { composePluginApi } from "@executor-js/api/server";
26import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api";26import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api";
27import {27import {
@@ -38,6 +38,13 @@ import type { McpSession } from "../src/surfaces/mcp"; · 10 unmodified lines
38 38
39const api = composePluginApi([openApiHttpPlugin()] as const);39const api = composePluginApi([openApiHttpPlugin()] as const);
40 40
41const completion = Schema.Struct({
42 structuredContent: Schema.Struct({
43 status: Schema.String,
44 toolName: Schema.optional(Schema.String),
45 }),
46});
47
41const unique = (prefix: string) => `${prefix}_${randomBytes(4).toString("hex")}`;48const unique = (prefix: string) => `${prefix}_${randomBytes(4).toString("hex")}`;
42 49
43type UpstreamHandle = {50type UpstreamHandle = {
@@ -114,14 +121,14 @@ const executeApproved = (session: McpSession, code: string) => · 70 unmodified lines
114 guard += 1;121 guard += 1;
115 }122 }
116 expect(result.ok, `execute completed (got: ${result.text.slice(0, 400)})`).toBe(true);123 expect(result.ok, `execute completed (got: ${result.text.slice(0, 400)})`).toBe(true);
117 return result.text;124 return result;
118 });125 });
119 126
120/** Invoke a dynamic tool by full address and parse the envelope it returns. */127/** Invoke a dynamic tool by full address and parse the envelope it returns. */
121const invokeEnvelope = (session: McpSession, address: string, args: unknown = {}) =>128const invokeEnvelope = (session: McpSession, address: string, args: unknown = {}) =>
122 Effect.map(129 Effect.map(
123 executeApproved(session, invokeByAddressCode(address, args)),130 executeApproved(session, invokeByAddressCode(address, args)),
124 (text) => JSON.parse(text) as ToolEnvelope,131 (result) => JSON.parse(result.text) as ToolEnvelope,
125 );132 );
126 133
127// ---------------------------------------------------------------------------134// ---------------------------------------------------------------------------
@@ -136,6 +143,20 @@ const widgetsSpec = (baseUrl: string): string => · 8 unmodified lines
136 info: { title: "Widgets API", version: "1.0.0" },143 info: { title: "Widgets API", version: "1.0.0" },
137 servers: [{ url: baseUrl }],144 servers: [{ url: baseUrl }],
138 paths: {145 paths: {
146 "/unavailable": {
147 get: {
148 operationId: "unavailableWidgets",
149 summary: "Unavailable widgets",
150 responses: { "200": { description: "widgets" } },
151 },
152 },
153 "/widgets/count": {
154 get: {
155 operationId: "countWidgets",
156 summary: "Count widgets",
157 responses: { "200": { description: "widget count" } },
158 },
159 },
139 "/widgets": {160 "/widgets": {
140 get: {161 get: {
141 operationId: "listWidgets",162 operationId: "listWidgets",
@@ -184,7 +205,7 @@ scenario( · 42 unmodified lines
184 },205 },
185 });206 });
186 const created = JSON.parse(207 const created = JSON.parse(
187 yield* executeApproved(session, createConnectionCode(slug)),208 (yield* executeApproved(session, createConnectionCode(slug))).text,
188 ) as ToolEnvelope;209 ) as ToolEnvelope;
189 expect(created.ok, `the no-auth connection was created: ${JSON.stringify(created)}`).toBe(210 expect(created.ok, `the no-auth connection was created: ${JSON.stringify(created)}`).toBe(
190 true,211 true,
@@ -199,7 +220,8 @@ scenario( · 8 unmodified lines
199 const path = address!.replace(/^tools\./, "");220 const path = address!.replace(/^tools\./, "");
200 221
201 // 1. A well-addressed call executes and carries the upstream's payload.222 // 1. A well-addressed call executes and carries the upstream's payload.
202 const success = yield* invokeEnvelope(session, address!);223 const successfulCall = yield* executeApproved(session, invokeByAddressCode(address!, {}));
224 const success = JSON.parse(successfulCall.text) as ToolEnvelope;
203 expect(225 expect(
204 success.ok,226 success.ok,
205 `the call succeeded (got: ${JSON.stringify(success.error ?? {}).slice(0, 400)})`,227 `the call succeeded (got: ${JSON.stringify(success.error ?? {}).slice(0, 400)})`,
@@ -207,7 +229,73 @@ scenario( · 1 unmodified line
207 expect(JSON.stringify(success.data), "the upstream's payload comes back").toContain(229 expect(JSON.stringify(success.data), "the upstream's payload comes back").toContain(
208 "anvil",230 "anvil",
209 );231 );
232 const structured = (successfulCall.raw as { readonly structuredContent?: unknown })
233 .structuredContent;
234 expect(structured, "the completed MCP result includes structured content").toMatchObject({
235 status: "completed",
236 toolName: path,
237 });
238 expect(structured, "the internal tool-call trace is not exposed").not.toHaveProperty(
239 "toolPaths",
240 );
210 expect(upstream.requests(), "the upstream served exactly one call").toBe(1);241 expect(upstream.requests(), "the upstream served exactly one call").toBe(1);
242 const discovery = yield* executeApproved(
243 session,
244 'return await tools.search({ query: "widgets" });',
245 );
246 const discoveryResult = yield* Schema.decodeUnknownEffect(completion)(discovery.raw);
247 expect(discoveryResult.structuredContent.toolName).toBeUndefined();
248 expect(discovery.raw).not.toHaveProperty("structuredContent.toolPaths");
249
250 const failed = yield* executeApproved(
251 session,
252 invokeByAddressCode(address!.replace(/listWidgets$/, "missingWidget"), {}),
253 );
254 const failedResult = yield* Schema.decodeUnknownEffect(completion)(failed.raw);
255 expect(failedResult.structuredContent.toolName).toBeUndefined();
256 expect(failed.raw).not.toHaveProperty("structuredContent.toolPaths");
257 const unavailableAddress = yield* Schema.decodeUnknownEffect(Schema.String)(
258 tools
259 .filter((tool) => String(tool.integration) === slug)
260 .map((tool) => String(tool.address))
261 .find((candidate) => candidate.endsWith("unavailableWidgets")),
262 );
263 const upstreamFailure = yield* executeApproved(
264 session,
265 invokeByAddressCode(unavailableAddress, {}),
266 );
267 const upstreamFailureResult = yield* Schema.decodeUnknownEffect(completion)(
268 upstreamFailure.raw,
269 );
270 expect(upstreamFailure.text).toContain('"ok":false');
271 expect(upstreamFailureResult.structuredContent.toolName).toBeUndefined();
272 expect(upstreamFailure.raw).not.toHaveProperty("structuredContent.toolPaths");
273
274 const repeated = yield* executeApproved(
275 session,
276 `
277 await tools[${JSON.stringify(path)}]({});
278 return await tools[${JSON.stringify(path)}]({});
279 `,
280 );
281 const repeatedResult = yield* Schema.decodeUnknownEffect(completion)(repeated.raw);
282 expect(repeatedResult.structuredContent.toolName).toBe(path);
283 expect(repeated.raw).not.toHaveProperty("structuredContent.toolPaths");
284 const anotherAddress = tools
285 .filter((tool) => String(tool.integration) === slug)
286 .map((tool) => String(tool.address))
287 .find((candidate) => candidate.endsWith("countWidgets"));
288 const anotherPath = yield* Schema.decodeUnknownEffect(Schema.String)(anotherAddress);
289 const multiple = yield* executeApproved(
290 session,
291 `
292 await tools[${JSON.stringify(path)}]({});
293 return await tools[${JSON.stringify(anotherPath.replace(/^tools\./, ""))}]({});
294 `,
295 );
296 const multipleResult = yield* Schema.decodeUnknownEffect(completion)(multiple.raw);
297 expect(multipleResult.structuredContent.toolName).toBeUndefined();
298 expect(multiple.raw).not.toHaveProperty("structuredContent.toolPaths");
211 299
212 // 2a. A wrong TOOL name on a live connection: tool_not_found, and the300 // 2a. A wrong TOOL name on a live connection: tool_not_found, and the
213 // suggestions name the connection's real tools so the agent can301 // suggestions name the connection's real tools so the agent can
@@ -257,7 +345,9 @@ scenario( · 43 unmodified lines
257 "the defect mask never surfaces for a missing connection",345 "the defect mask never surfaces for a missing connection",
258 ).not.toContain("Internal tool error");346 ).not.toContain("Internal tool error");
259 347
260 expect(upstream.requests(), "no misaddressed call ever reached the upstream").toBe(1);348 expect(upstream.requests(), "only the five successful calls reached the upstream").toBe(
349 5,
350 );
261 }),351 }),
262 // Selfhost shares one workspace identity — leaked resources fail other352 // Selfhost shares one workspace identity — leaked resources fail other
263 // scenarios' zero-state assertions.353 // scenarios' zero-state assertions.
packages/core/execution/src/engine.test.ts+29−0
@@ -309,6 +309,35 @@ describe("formatExecuteResult output identity", () => { · 308 unmodified lines
309 expect(formatted.isError).toBe(false);309 expect(formatted.isError).toBe(false);
310 });310 });
311 311
312 it("returns the sole distinct connected tool name without exposing the call trace", () => {
313 const result = {
314 result: { issues: [] },
315 logs: [],
316 toolPaths: ["linear.org.work.issues.list", "linear.org.work.issues.list"],
317 } as ExecuteResult & { readonly toolPaths: readonly string[] };
318
319 const formatted = formatExecuteResult(result);
320
321 expect(formatted.structured).toEqual({
322 status: "completed",
323 result: { issues: [] },
324 toolName: "linear.org.work.issues.list",
325 logs: [],
326 });
327 });
328
329 it("omits a tool name when distinct connected tools were used", () => {
330 const result = {
331 result: { issues: [], projects: [] },
332 logs: [],
333 toolPaths: ["linear.org.work.issues.list", "linear.org.work.projects.list"],
334 } as ExecuteResult & { readonly toolPaths: readonly string[] };
335
336 const formatted = formatExecuteResult(result);
337
338 expect(formatted.structured).not.toHaveProperty("toolName");
339 });
340
312 it("truncates a long preview with the exact suffix and untouched structured value", () => {341 it("truncates a long preview with the exact suffix and untouched structured value", () => {
313 const value = { data: "é🎉".repeat(12_000) };342 const value = { data: "é🎉".repeat(12_000) };
314 const pretty = JSON.stringify(value, null, 2);343 const pretty = JSON.stringify(value, null, 2);
packages/core/execution/src/engine.ts+21−5
@@ -138,6 +138,11 @@ const truncate = (value: string, max: number): string => · 137 unmodified lines
138 ? `${value.slice(0, max)}\n... [truncated ${value.length - max} chars]`138 ? `${value.slice(0, max)}\n... [truncated ${value.length - max} chars]`
139 : value;139 : value;
140 140
141const soleConnectedToolName = (toolPaths: readonly string[] | undefined): string | undefined => {
142 const names = [...new Set(toolPaths ?? [])];
143 return names.length === 1 ? names[0] : undefined;
144};
145
141export const formatExecuteResult = (146export const formatExecuteResult = (
142 result: ExecuteResult,147 result: ExecuteResult,
143): {148): {
@@ -183,11 +188,13 @@ export const formatExecuteResult = ( · 39 unmodified lines
183 ? `(no return value; ${emittedNote})`188 ? `(no return value; ${emittedNote})`
184 : "(no result)";189 : "(no result)";
185 const parts = [resultPart, ...(logText ? [`\nLogs:\n${logText}`] : [])];190 const parts = [resultPart, ...(logText ? [`\nLogs:\n${logText}`] : [])];
191 const toolName = soleConnectedToolName(result.toolPaths);
186 return {192 return {
187 text: parts.join("\n"),193 text: parts.join("\n"),
188 structured: {194 structured: {
189 status: "completed",195 status: "completed",
190 result: result.result ?? null,196 result: result.result ?? null,
197 ...(toolName ? { toolName } : {}),
191 ...emittedField,198 ...emittedField,
192 logs: result.logs ?? [],199 logs: result.logs ?? [],
193 },200 },
@@ -318,8 +325,9 @@ const makeFullInvoker = ( · 124 unmodified lines
318 executor: Executor,325 executor: Executor,
319 invokeOptions: InvokeOptions,326 invokeOptions: InvokeOptions,
320 toolDiscoveryProvider: ToolDiscoveryProvider,327 toolDiscoveryProvider: ToolDiscoveryProvider,
328 onConnectedToolCall?: (path: string) => void,
321): SandboxToolInvoker => {329): SandboxToolInvoker => {
322 const base = makeExecutorToolInvoker(executor, { invokeOptions });330 const base = makeExecutorToolInvoker(executor, { invokeOptions, onConnectedToolCall });
323 return {331 return {
324 invoke: ({ path, args }) => {332 invoke: ({ path, args }) => {
325 if (path === "search") {333 if (path === "search") {
@@ -694,13 +702,18 @@ export const createExecutionEngine = <E extends Cause.YieldableError = CodeExecu · 368 unmodified lines
694 return yield* Deferred.await(responseDeferred);702 return yield* Deferred.await(responseDeferred);
695 });703 });
696 704
705 const toolPaths: string[] = [];
697 const invoker = makeFullInvoker(706 const invoker = makeFullInvoker(
698 executor,707 executor,
699 { onElicitation: elicitationHandler },708 { onElicitation: elicitationHandler },
700 toolDiscoveryProvider,709 toolDiscoveryProvider,
710 (path) => toolPaths.push(path),
701 );711 );
702 fiber = yield* Effect.forkDetach(712 fiber = yield* Effect.forkDetach(
703 codeExecutor.execute(code, invoker).pipe(Effect.withSpan("executor.code.exec")),713 codeExecutor.execute(code, invoker).pipe(
714 Effect.map((result) => (toolPaths.length === 0 ? result : { ...result, toolPaths })),
715 Effect.withSpan("executor.code.exec"),
716 ),
704 );717 );
705 liveSandboxFibers.add(fiber);718 liveSandboxFibers.add(fiber);
706 719
@@ -825,16 +838,19 @@ export const createExecutionEngine = <E extends Cause.YieldableError = CodeExecu · 118 unmodified lines
825 "mcp.execute.mode": "inline",838 "mcp.execute.mode": "inline",
826 "mcp.execute.code_length": code.length,839 "mcp.execute.code_length": code.length,
827 });840 });
841 const toolPaths: string[] = [];
828 const invoker = makeFullInvoker(842 const invoker = makeFullInvoker(
829 executor,843 executor,
830 {844 {
831 onElicitation: options.onElicitation,845 onElicitation: options.onElicitation,
832 },846 },
833 toolDiscoveryProvider,847 toolDiscoveryProvider,
848 (path) => toolPaths.push(path),
849 );
850 const result = yield* codeExecutor.execute(code, invoker).pipe(
851 Effect.map((result) => (toolPaths.length === 0 ? result : { ...result, toolPaths })),
852 Effect.withSpan("executor.code.exec"),
834 );853 );
835 const result = yield* codeExecutor
836 .execute(code, invoker)
837 .pipe(Effect.withSpan("executor.code.exec"));
838 yield* annotateExecuteOutcome(result);854 yield* annotateExecuteOutcome(result);
839 return result;855 return result;
840 });856 });
packages/core/execution/src/tool-invoker.test.ts+19−0
@@ -674,6 +674,25 @@ describe("tool discovery", () => { · 673 unmodified lines
674 }),674 }),
675 );675 );
676 676
677 it.effect("records only the connected tool resolved after discovery", () =>
678 Effect.gen(function* () {
679 const executor = yield* makeSearchExecutor();
680 const engine = createExecutionEngine({ executor, codeExecutor });
681
682 const execution = yield* engine.execute(
683 [
684 'const search = await tools.search({ query: "repository details", namespace: "github", limit: 1 });',
685 'const result = await tools[search.items[0].path]({ owner: "executor", repo: "executor" });',
686 "return result;",
687 ].join("\n"),
688 { onElicitation: acceptAll },
689 );
690
691 expect(execution.error).toBeUndefined();
692 expect(execution.toolPaths).toEqual(["github.org.main.getRepositoryDetails"]);
693 }),
694 );
695
677 it.effect("lets execution hosts provide custom tool discovery", () =>696 it.effect("lets execution hosts provide custom tool discovery", () =>
678 Effect.gen(function* () {697 Effect.gen(function* () {
679 const executor = yield* makeSearchExecutor();698 const executor = yield* makeSearchExecutor();
packages/core/execution/src/tool-invoker.ts+10−1
@@ -306,7 +306,10 @@ const extractNamespace = (path: string): string => { · 305 unmodified lines
306 */306 */
307export const makeExecutorToolInvoker = (307export const makeExecutorToolInvoker = (
308 executor: Executor,308 executor: Executor,
309 options: { readonly invokeOptions: InvokeOptions },309 options: {
310 readonly invokeOptions: InvokeOptions;
311 readonly onConnectedToolCall?: (path: string) => void;
312 },
310): SandboxToolInvoker => ({313): SandboxToolInvoker => ({
311 invoke: Effect.fn("mcp.tool.dispatch")(function* ({ path, args }) {314 invoke: Effect.fn("mcp.tool.dispatch")(function* ({ path, args }) {
312 yield* Effect.annotateCurrentSpan({315 yield* Effect.annotateCurrentSpan({
@@ -372,6 +375,12 @@ export const makeExecutorToolInvoker = ( · 59 unmodified lines
372 // outcome annotation the dispatch span reads as healthy even when the375 // outcome annotation the dispatch span reads as healthy even when the
373 // caller hit an upstream error or auth wall.376 // caller hit an upstream error or auth wall.
374 yield* annotateToolResultOutcome(result);377 yield* annotateToolResultOutcome(result);
378 const connectedToolPath = parseToolAddress(String(address))
379 ? addressToPath(String(address))
380 : undefined;
381 if (connectedToolPath && (!isToolResult(result) || result.ok)) {
382 options.onConnectedToolCall?.(connectedToolPath);
383 }
375 if (isToolResult(result)) {384 if (isToolResult(result)) {
376 return result;385 return result;
377 }386 }
packages/hosts/mcp/src/tool-server.test.ts+9−2
@@ -1039,12 +1039,15 @@ describe("MCP host server — native form-only elicitation", () => { · 1038 unmodified lines
1039// ---------------------------------------------------------------------------1039// ---------------------------------------------------------------------------
1040 1040
1041describe("MCP host server — client without elicitation (pause/resume)", () => {1041describe("MCP host server — client without elicitation (pause/resume)", () => {
1042 it("completed execution returns result directly", async () => {1042 it("completed execution returns result and connected-tool metadata directly", async () => {
1043 const engine = makeStubEngine({1043 const engine = makeStubEngine({
1044 executeWithPause: () =>1044 executeWithPause: () =>
1045 Effect.succeed({1045 Effect.succeed({
1046 status: "completed",1046 status: "completed",
1047 result: { result: "done" },1047 result: {
1048 result: "done",
1049 toolPaths: ["linear.org.work.issues.list"],
1050 },
1048 }),1051 }),
1049 });1052 });
1050 1053
@@ -1054,6 +1057,10 @@ describe("MCP host server — client without elicitation (pause/resume)", () => · 3 unmodified lines
1054 arguments: { code: "ok" },1057 arguments: { code: "ok" },
1055 });1058 });
1056 expect(result.content).toEqual([{ type: "text", text: "done" }]);1059 expect(result.content).toEqual([{ type: "text", text: "done" }]);
1060 expect(result.structuredContent).toMatchObject({
1061 status: "completed",
1062 toolName: "linear.org.work.issues.list",
1063 });
1057 expect(result.isError).toBeFalsy();1064 expect(result.isError).toBeFalsy();
1058 });1065 });
1059 });1066 });
packages/kernel/core/src/types.ts+2−0
@@ -46,6 +46,8 @@ export type ExecuteResult = { · 45 unmodified lines
46 /** Enumerable failure class for telemetry; never carries message content. */46 /** Enumerable failure class for telemetry; never carries message content. */
47 errorKind?: ExecuteErrorKind;47 errorKind?: ExecuteErrorKind;
48 logs?: string[];48 logs?: string[];
49 /** Successful connected-tool paths observed during this execution. */
50 toolPaths?: readonly string[];
49};51};
50 52
51/**53/**