Skip to content

Commit 498f300

Browse files
riglarclaude
andauthored
fix(mcp): dry runs honour tags, single flow files bring their subflows, failed starts say what to check (#278)
- dcd_run_cloud_test with dryRun listed every flow in the folder, because it read the metadata of all flows rather than the ones that would run. It now lists the flows after includeTags, excludeTags and excludeFlows, as `dcd cloud --dry-run` does. Real runs already submitted only those flows. - `run` with path set to one flow file sent it as yaml on its own, so a flow that uses other files (runFlow, runScript, addMedia and the like) failed on the device with an invalid file path. Such a file is now uploaded with its folder as a bundle, with flow set to the file. A file that uses no other files is still sent as yaml. - A start_device call that fails without an answer may still have started a session, which is billed while it runs. The error now says so and lists the open sessions started in the last two minutes, so an agent checks list_devices instead of starting a second device. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 7d751d2 commit 498f300

4 files changed

Lines changed: 278 additions & 12 deletions

File tree

‎src/mcp/bridge.ts‎

Lines changed: 86 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
* stdout is the JSON-RPC channel: everything else goes to stderr.
1717
*/
1818
import { readFile, stat } from 'node:fs/promises';
19+
import * as path from 'node:path';
1920

2021
import { Client } from '@modelcontextprotocol/sdk/client/index.js';
2122
import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js';
@@ -28,7 +29,11 @@ import {
2829
type Tool,
2930
} from '@modelcontextprotocol/sdk/types.js';
3031

32+
import { DeviceGateway } from '../gateways/device-gateway.js';
33+
import { processDependencies, readTestYamlFileAsJson } from '../services/execution-plan.utils.js';
34+
import { computeCommonRoot } from '../services/flow-paths.js';
3135
import type { AuthContext } from '../types/domain/auth.types.js';
36+
import type { DeviceSession } from '../types/domain/device-session.types.js';
3237
import { uploadLocalApp } from '../utils/app-upload.js';
3338
import { getCliVersion } from '../utils/cli.js';
3439
import { uploadFlowBundle } from '../utils/device-bundle.js';
@@ -63,7 +68,7 @@ const LOCAL_INPUTS: Record<string, { description: string; properties: Record<str
6368
},
6469
run: {
6570
description:
66-
' Or pass path: a flow file on this machine (sent as yaml), or a directory of flows (flow picks the entry file inside it). Only the YAML files and the files the flows use (scripts, media) are uploaded, not builds or anything else in the directory.',
71+
' Or pass path: a flow file on this machine, or a directory of flows (flow picks the entry file inside it). A file that uses other files (runFlow, runScript, addMedia and the like) is uploaded with its folder; one that uses none is sent as yaml. Only the YAML files and the files the flows use (scripts, media) are uploaded, not builds or anything else in the directory.',
6772
properties: {
6873
path: {
6974
type: 'string',
@@ -87,6 +92,30 @@ export function withLocalInputs(tool: Tool): Tool {
8792
};
8893
}
8994

95+
/**
96+
* For a single flow file that uses other files (runFlow, runScript, addMedia
97+
* and the like): the folder to upload as a bundle, and the file's path
98+
* inside it. Sent as yaml on its own, the flow would fail on the device with
99+
* an invalid file path, because the files it uses would not be there. Null
100+
* when the flow uses no other files, or can't be read (the device reports
101+
* that itself).
102+
*/
103+
export function flowFileBundle(file: string): { dir: string; entry: string } | null {
104+
const absolute = path.resolve(file);
105+
let deps: ReturnType<typeof processDependencies>;
106+
try {
107+
const { config, testSteps } = readTestYamlFileAsJson(absolute);
108+
deps = processDependencies({ config, input: absolute, testSteps });
109+
} catch {
110+
return null;
111+
}
112+
113+
// A missing file goes this way too, so the upload names it.
114+
if (deps.allFiles.length === 0 && deps.allErrors.length === 0) return null;
115+
const dir = computeCommonRoot([absolute], deps.allFiles) || path.dirname(absolute);
116+
return { dir, entry: path.relative(dir, absolute) };
117+
}
118+
90119
/**
91120
* Do the local part of a call (uploads, reading a file) and return the
92121
* arguments the hosted tool takes.
@@ -123,12 +152,56 @@ export async function resolveLocalInputs(
123152
return { ...rest, flow_upload_id: bundle.id };
124153
}
125154

155+
const bundled = flowFileBundle(flowPath as string);
156+
if (bundled) {
157+
logStderr(`${flowPath} uses other files; uploading it with ${bundled.dir}…`);
158+
const bundle = await uploadFlowBundle({ ...ctx, ...bundled, log: logStderr });
159+
return { ...rest, flow: bundled.entry, flow_upload_id: bundle.id };
160+
}
161+
126162
return { ...rest, yaml: await readFile(flowPath as string, 'utf8') };
127163
}
128164

129165
return args;
130166
}
131167

168+
/** How recent an open session must be to count as one a failed start may have made. */
169+
export const START_RECOVERY_WINDOW_MS = 2 * 60 * 1000;
170+
171+
/**
172+
* What to tell the agent when a start_device call failed without an answer
173+
* (the connection dropped, or the server never replied). The request may
174+
* still have started a session, which is billed while it runs, and an agent
175+
* that just retries would end up with two. So list the open sessions started
176+
* in the last couple of minutes, and say to check before starting again.
177+
*/
178+
export async function explainFailedStart(
179+
message: string,
180+
ctx: { apiUrl: string; auth: AuthContext },
181+
now = Date.now(),
182+
): Promise<string> {
183+
const head = `start_device failed: ${message}. The request may still have started a session, and a session is billed while it runs.`;
184+
let recent: DeviceSession[];
185+
try {
186+
const { sessions } = await DeviceGateway.list(ctx.apiUrl, ctx.auth);
187+
recent = sessions.filter((s) => now - Date.parse(s.created_at) <= START_RECOVERY_WINDOW_MS);
188+
} catch {
189+
return `${head} Call list_devices to check for one before you call start_device again.`;
190+
}
191+
192+
if (recent.length === 0) {
193+
return `${head} No session was started in the last ${START_RECOVERY_WINDOW_MS / 60_000} minutes, so it is safe to call start_device again.`;
194+
}
195+
196+
const listed = recent.map((s) => ({
197+
client_label: s.client_label,
198+
created_at: s.created_at,
199+
id: s.id,
200+
state: s.state,
201+
}));
202+
return `${head} Open sessions started in the last ${START_RECOVERY_WINDOW_MS / 60_000} minutes: ${JSON.stringify(listed)}. If one is yours, use it (get_device_status with wait: true until it is ready) instead of starting another. Otherwise stop it with stop_device. Check list_devices before you call start_device again.`;
203+
}
204+
132205
/**
133206
* The hosted device tools, over one connection made on first use and shared
134207
* by every call.
@@ -247,8 +320,18 @@ export async function createBridgeServer(
247320
return errorResult(`${name} is not available in read-only mode.`);
248321
}
249322
const client = await remote.connect();
250-
const remoteArgs = await resolveLocalInputs(name, args, await getContext());
251-
return (await client.callTool({ name, arguments: remoteArgs }, undefined, options)) as CallToolResult;
323+
const ctx = await getContext();
324+
const remoteArgs = await resolveLocalInputs(name, args, ctx);
325+
try {
326+
return (await client.callTool({ name, arguments: remoteArgs }, undefined, options)) as CallToolResult;
327+
} catch (error) {
328+
// A tool's own refusal comes back as a result. A throw means no answer
329+
// arrived, so for a start the session's fate is unknown.
330+
if (name !== 'start_device' || extra.signal.aborted) throw error;
331+
return errorResult(
332+
await explainFailedStart(error instanceof Error ? error.message : String(error), ctx),
333+
);
334+
}
252335
});
253336
});
254337

‎src/mcp/tools/run-cloud-test.ts‎

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,16 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
44
import { z } from 'zod';
55

66
import { ApiGateway } from '../../gateways/api-gateway.js';
7-
import { plan } from '../../services/execution-plan.service.js';
7+
import { type IExecutionPlan, plan } from '../../services/execution-plan.service.js';
88
import {
99
isSubmitEndpointMissing,
1010
submitWithRetry,
1111
} from '../../services/flow-submit.service.js';
12-
import { computeCommonRoot, buildTestMetadataMap } from '../../services/flow-paths.js';
12+
import {
13+
computeCommonRoot,
14+
buildTestMetadataMap,
15+
type FlowMetadataEntry,
16+
} from '../../services/flow-paths.js';
1317
import { DeviceValidationService } from '../../services/device-validation.service.js';
1418
import { platformFromAppFile } from '../../services/notices.service.js';
1519
import type { EncryptionTelemetry } from '../../services/telemetry.service.js';
@@ -20,6 +24,7 @@ import { refreshAuth } from '../../utils/auth.js';
2024
import { getCliVersion } from '../../utils/cli.js';
2125
import { fetchCompatibilityData } from '../../utils/compatibility.js';
2226
import { checkKekForEncryption, resolveEncryption } from '../../utils/envelope.js';
27+
import { toPortableRelativePath } from '../../utils/paths.js';
2328
import { getConsoleUrl } from '../../utils/styling.js';
2429
import { getContext, logStderr } from '../context.js';
2530
import { jsonResult, runTool } from '../helpers.js';
@@ -31,6 +36,24 @@ const TERMINAL_STATUSES = new Set(['PASSED', 'FAILED', 'CANCELLED', 'ERROR']);
3136
const sleep = (ms: number) =>
3237
new Promise<void>((resolve) => setTimeout(resolve, ms));
3338

39+
/**
40+
* The flows a dry run lists: the ones a real run would submit, after the
41+
* include/exclude tags and excludeFlows, as `dcd cloud --dry-run` lists
42+
* them. `flowMetadata` holds every flow in the folder, filtered out or not,
43+
* so it can't be the list.
44+
*/
45+
export function dryRunFlows(
46+
executionPlan: Pick<IExecutionPlan, 'flowsToRun' | 'sequence'>,
47+
testMetadataMap: Record<string, FlowMetadataEntry>,
48+
commonRoot: string,
49+
): { file: string; flowName: string; tags: string[] }[] {
50+
return [...executionPlan.flowsToRun, ...(executionPlan.sequence?.flows ?? [])].map((absolute) => {
51+
const file = toPortableRelativePath(absolute, commonRoot);
52+
const meta = testMetadataMap[file];
53+
return { file, flowName: meta?.flowName ?? path.parse(absolute).name, tags: meta?.tags ?? [] };
54+
});
55+
}
56+
3457
/**
3558
* Submit a Maestro flow (or directory of flows) to devicecloud.dev.
3659
*
@@ -198,11 +221,7 @@ export function registerRunCloudTest(server: McpServer): void {
198221
if (args.dryRun) {
199222
return jsonResult({
200223
dryRun: true,
201-
flows: Object.entries(testMetadataMap).map(([file, meta]) => ({
202-
file,
203-
flowName: meta.flowName,
204-
tags: meta.tags,
205-
})),
224+
flows: dryRunFlows(executionPlan, testMetadataMap, commonRoot),
206225
sequentialFlowCount: executionPlan.sequence?.flows.length ?? 0,
207226
});
208227
}

‎test/unit/mcp-bridge.test.ts‎

Lines changed: 97 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,13 @@ import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
1111
import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js';
1212
import { z } from 'zod';
1313

14-
import { createBridgeServer, resolveLocalInputs, withLocalInputs } from '../../src/mcp/bridge.js';
14+
import {
15+
createBridgeServer,
16+
explainFailedStart,
17+
flowFileBundle,
18+
resolveLocalInputs,
19+
withLocalInputs,
20+
} from '../../src/mcp/bridge.js';
1521
import { clearContextCache } from '../../src/mcp/context.js';
1622

1723
/**
@@ -20,13 +26,21 @@ import { clearContextCache } from '../../src/mcp/context.js';
2026
*/
2127
function startHosted(): Promise<{
2228
calls: { args: unknown; auth?: string; name: string }[];
29+
failStart: boolean;
2330
http: HttpServer;
2431
inits: number;
32+
sessions: Record<string, unknown>[];
2533
url: string;
2634
}> {
2735
const calls: { args: unknown; auth?: string; name: string }[] = [];
28-
const hosted = { calls, inits: 0 };
36+
const hosted = { calls, failStart: false, inits: 0, sessions: [] as Record<string, unknown>[] };
2937
const http = createHttpServer(async (req, res) => {
38+
// The open sessions, as GET /device/sessions answers.
39+
if (req.method === 'GET' && req.url === '/device/sessions') {
40+
res.writeHead(200, { 'content-type': 'application/json' }).end(JSON.stringify({ sessions: hosted.sessions }));
41+
return;
42+
}
43+
3044
// Stateless, as the API: no GET stream, no session to delete.
3145
if (req.method !== 'POST') {
3246
res.writeHead(405).end();
@@ -37,6 +51,11 @@ function startHosted(): Promise<{
3751
for await (const c of req) chunks.push(c as Buffer);
3852
const body = JSON.parse(Buffer.concat(chunks).toString() || '{}');
3953
if (body.method === 'initialize') hosted.inits++;
54+
// A start whose answer never arrives, as when a proxy gives up on it.
55+
if (hosted.failStart && body.method === 'tools/call' && body.params?.name === 'start_device') {
56+
res.writeHead(502, { 'content-type': 'application/json' }).end('{"message":"Application failed to respond"}');
57+
return;
58+
}
4059
const auth = req.headers['x-app-api-key'] as string | undefined;
4160
const server = new McpServer({ name: 'hosted', version: '1' });
4261
const record = (name: string) => async (args: unknown) => {
@@ -64,6 +83,11 @@ function startHosted(): Promise<{
6483
return { content: [{ type: 'text' as const, text: 'late' }] };
6584
},
6685
);
86+
server.registerTool(
87+
'start_device',
88+
{ description: 'Start a device.', inputSchema: { platform: z.string() } },
89+
record('start_device'),
90+
);
6791
server.registerTool(
6892
'inspect_screen',
6993
{ description: 'Inspect.', inputSchema: {}, annotations: { readOnlyHint: true } },
@@ -181,6 +205,34 @@ describe('dcd mcp bridge', () => {
181205
await client.close();
182206
});
183207

208+
it('says a failed start may have made a session, and lists the recent ones', async () => {
209+
hosted.failStart = true;
210+
const now = Date.now();
211+
hosted.sessions = [
212+
{ client_label: 'claude-code', created_at: new Date(now - 30_000).toISOString(), id: 'new-one', state: 'provisioning' },
213+
{ client_label: 'other', created_at: new Date(now - 30 * 60_000).toISOString(), id: 'old-one', state: 'active' },
214+
];
215+
const client = await connect();
216+
await client.listTools();
217+
const result = await client.callTool({ name: 'start_device', arguments: { platform: 'ios' } });
218+
expect(result.isError).to.equal(true);
219+
const text = JSON.stringify(result.content);
220+
expect(text).to.include('may still have started a session');
221+
expect(text).to.include('new-one');
222+
expect(text).to.not.include('old-one');
223+
expect(text).to.include('list_devices');
224+
await client.close();
225+
});
226+
227+
it('leaves a start that answered alone', async () => {
228+
const client = await connect();
229+
await client.listTools();
230+
const result = await client.callTool({ name: 'start_device', arguments: { platform: 'ios' } });
231+
expect(result.isError).to.not.equal(true);
232+
expect(hosted.calls.map((c) => c.name)).to.deep.equal(['start_device']);
233+
await client.close();
234+
});
235+
184236
it('read-only mode keeps only device tools that change nothing', async () => {
185237
process.env.DCD_MCP_READONLY = '1';
186238
const client = await connect();
@@ -211,6 +263,49 @@ describe('dcd mcp bridge', () => {
211263
});
212264

213265
describe('helpers', () => {
266+
it('explainFailedStart says it is safe to retry when no session started', async () => {
267+
hosted.sessions = [];
268+
const ctx = { apiUrl: hosted.url, auth: { headers: {}, mode: 'apiKey' as const } };
269+
const text = await explainFailedStart('fetch failed', ctx);
270+
expect(text).to.include('safe to call start_device again');
271+
});
272+
273+
it('explainFailedStart still warns when the sessions cannot be read', async () => {
274+
const ctx = { apiUrl: 'http://127.0.0.1:9', auth: { headers: {}, mode: 'apiKey' as const } };
275+
const text = await explainFailedStart('fetch failed', ctx);
276+
expect(text).to.include('Call list_devices to check');
277+
});
278+
279+
it('flowFileBundle sends a flow that uses no other files as yaml', () => {
280+
const flow = path.join(tempDir, 'plain.yaml');
281+
fs.writeFileSync(flow, 'appId: x\n---\n- launchApp\n- tapOn: OK\n');
282+
expect(flowFileBundle(flow)).to.equal(null);
283+
});
284+
285+
it('flowFileBundle uploads the folder of a flow that runs a subflow or a script', () => {
286+
fs.mkdirSync(path.join(tempDir, 'flows', 'subflows'), { recursive: true });
287+
fs.writeFileSync(path.join(tempDir, 'flows', 'subflows', 'login.yaml'), 'appId: x\n---\n- tapOn: Log in\n');
288+
fs.writeFileSync(path.join(tempDir, 'flows', 'setup.js'), 'output.ok = true\n');
289+
const flow = path.join(tempDir, 'flows', 'main.yaml');
290+
fs.writeFileSync(flow, 'appId: x\n---\n- runFlow: subflows/login.yaml\n- runScript: setup.js\n');
291+
expect(flowFileBundle(flow)).to.deep.equal({ dir: path.join(tempDir, 'flows'), entry: 'main.yaml' });
292+
});
293+
294+
it('flowFileBundle reaches up to a shared folder the flow uses', () => {
295+
fs.mkdirSync(path.join(tempDir, 'shared'), { recursive: true });
296+
fs.mkdirSync(path.join(tempDir, 'ios'), { recursive: true });
297+
fs.writeFileSync(path.join(tempDir, 'shared', 'login.yaml'), 'appId: x\n---\n- tapOn: Log in\n');
298+
const flow = path.join(tempDir, 'ios', 'main.yaml');
299+
fs.writeFileSync(flow, 'appId: x\n---\n- runFlow: ../shared/login.yaml\n');
300+
expect(flowFileBundle(flow)).to.deep.equal({ dir: tempDir, entry: path.join('ios', 'main.yaml') });
301+
});
302+
303+
it('flowFileBundle still bundles a flow whose subflow is missing, so the upload names it', () => {
304+
const flow = path.join(tempDir, 'broken.yaml');
305+
fs.writeFileSync(flow, 'appId: x\n---\n- runFlow: subflows/missing.yaml\n');
306+
expect(flowFileBundle(flow)).to.deep.equal({ dir: tempDir, entry: 'broken.yaml' });
307+
});
308+
214309
it('withLocalInputs adds the local inputs and leaves other tools alone', () => {
215310
const tool = { name: 'run', description: 'Run.', inputSchema: { type: 'object' as const, properties: { yaml: {} } } };
216311
const patched = withLocalInputs(tool);

0 commit comments

Comments
 (0)