From 40c6f9fae2ccc0a467e485b62ac542f35ebe229c Mon Sep 17 00:00:00 2001 From: Tom Riglar Date: Mon, 26 Oct 2026 16:35:40 +0000 Subject: [PATCH] fix(mcp): dry runs honour tags, single flow files bring their subflows, failed starts say what to check - 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 --- src/mcp/bridge.ts | 89 ++++++++++++++++++++++++++++- src/mcp/tools/run-cloud-test.ts | 33 ++++++++--- test/unit/mcp-bridge.test.ts | 99 ++++++++++++++++++++++++++++++++- test/unit/mcp-dry-run.test.ts | 69 +++++++++++++++++++++++ 4 files changed, 278 insertions(+), 12 deletions(-) create mode 100644 test/unit/mcp-dry-run.test.ts diff --git a/src/mcp/bridge.ts b/src/mcp/bridge.ts index d271feb..3dab41e 100644 --- a/src/mcp/bridge.ts +++ b/src/mcp/bridge.ts @@ -16,6 +16,7 @@ * stdout is the JSON-RPC channel: everything else goes to stderr. */ import { readFile, stat } from 'node:fs/promises'; +import * as path from 'node:path'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; @@ -28,7 +29,11 @@ import { type Tool, } from '@modelcontextprotocol/sdk/types.js'; +import { DeviceGateway } from '../gateways/device-gateway.js'; +import { processDependencies, readTestYamlFileAsJson } from '../services/execution-plan.utils.js'; +import { computeCommonRoot } from '../services/flow-paths.js'; import type { AuthContext } from '../types/domain/auth.types.js'; +import type { DeviceSession } from '../types/domain/device-session.types.js'; import { uploadLocalApp } from '../utils/app-upload.js'; import { getCliVersion } from '../utils/cli.js'; import { uploadFlowBundle } from '../utils/device-bundle.js'; @@ -63,7 +68,7 @@ const LOCAL_INPUTS: Record; + try { + const { config, testSteps } = readTestYamlFileAsJson(absolute); + deps = processDependencies({ config, input: absolute, testSteps }); + } catch { + return null; + } + + // A missing file goes this way too, so the upload names it. + if (deps.allFiles.length === 0 && deps.allErrors.length === 0) return null; + const dir = computeCommonRoot([absolute], deps.allFiles) || path.dirname(absolute); + return { dir, entry: path.relative(dir, absolute) }; +} + /** * Do the local part of a call (uploads, reading a file) and return the * arguments the hosted tool takes. @@ -123,12 +152,56 @@ export async function resolveLocalInputs( return { ...rest, flow_upload_id: bundle.id }; } + const bundled = flowFileBundle(flowPath as string); + if (bundled) { + logStderr(`${flowPath} uses other files; uploading it with ${bundled.dir}…`); + const bundle = await uploadFlowBundle({ ...ctx, ...bundled, log: logStderr }); + return { ...rest, flow: bundled.entry, flow_upload_id: bundle.id }; + } + return { ...rest, yaml: await readFile(flowPath as string, 'utf8') }; } return args; } +/** How recent an open session must be to count as one a failed start may have made. */ +export const START_RECOVERY_WINDOW_MS = 2 * 60 * 1000; + +/** + * What to tell the agent when a start_device call failed without an answer + * (the connection dropped, or the server never replied). The request may + * still have started a session, which is billed while it runs, and an agent + * that just retries would end up with two. So list the open sessions started + * in the last couple of minutes, and say to check before starting again. + */ +export async function explainFailedStart( + message: string, + ctx: { apiUrl: string; auth: AuthContext }, + now = Date.now(), +): Promise { + const head = `start_device failed: ${message}. The request may still have started a session, and a session is billed while it runs.`; + let recent: DeviceSession[]; + try { + const { sessions } = await DeviceGateway.list(ctx.apiUrl, ctx.auth); + recent = sessions.filter((s) => now - Date.parse(s.created_at) <= START_RECOVERY_WINDOW_MS); + } catch { + return `${head} Call list_devices to check for one before you call start_device again.`; + } + + if (recent.length === 0) { + 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.`; + } + + const listed = recent.map((s) => ({ + client_label: s.client_label, + created_at: s.created_at, + id: s.id, + state: s.state, + })); + 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.`; +} + /** * The hosted device tools, over one connection made on first use and shared * by every call. @@ -247,8 +320,18 @@ export async function createBridgeServer( return errorResult(`${name} is not available in read-only mode.`); } const client = await remote.connect(); - const remoteArgs = await resolveLocalInputs(name, args, await getContext()); - return (await client.callTool({ name, arguments: remoteArgs }, undefined, options)) as CallToolResult; + const ctx = await getContext(); + const remoteArgs = await resolveLocalInputs(name, args, ctx); + try { + return (await client.callTool({ name, arguments: remoteArgs }, undefined, options)) as CallToolResult; + } catch (error) { + // A tool's own refusal comes back as a result. A throw means no answer + // arrived, so for a start the session's fate is unknown. + if (name !== 'start_device' || extra.signal.aborted) throw error; + return errorResult( + await explainFailedStart(error instanceof Error ? error.message : String(error), ctx), + ); + } }); }); diff --git a/src/mcp/tools/run-cloud-test.ts b/src/mcp/tools/run-cloud-test.ts index d099fe3..38ae9c6 100644 --- a/src/mcp/tools/run-cloud-test.ts +++ b/src/mcp/tools/run-cloud-test.ts @@ -4,12 +4,16 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { z } from 'zod'; import { ApiGateway } from '../../gateways/api-gateway.js'; -import { plan } from '../../services/execution-plan.service.js'; +import { type IExecutionPlan, plan } from '../../services/execution-plan.service.js'; import { isSubmitEndpointMissing, submitWithRetry, } from '../../services/flow-submit.service.js'; -import { computeCommonRoot, buildTestMetadataMap } from '../../services/flow-paths.js'; +import { + computeCommonRoot, + buildTestMetadataMap, + type FlowMetadataEntry, +} from '../../services/flow-paths.js'; import { DeviceValidationService } from '../../services/device-validation.service.js'; import { platformFromAppFile } from '../../services/notices.service.js'; import type { EncryptionTelemetry } from '../../services/telemetry.service.js'; @@ -20,6 +24,7 @@ import { refreshAuth } from '../../utils/auth.js'; import { getCliVersion } from '../../utils/cli.js'; import { fetchCompatibilityData } from '../../utils/compatibility.js'; import { checkKekForEncryption, resolveEncryption } from '../../utils/envelope.js'; +import { toPortableRelativePath } from '../../utils/paths.js'; import { getConsoleUrl } from '../../utils/styling.js'; import { getContext, logStderr } from '../context.js'; import { jsonResult, runTool } from '../helpers.js'; @@ -31,6 +36,24 @@ const TERMINAL_STATUSES = new Set(['PASSED', 'FAILED', 'CANCELLED', 'ERROR']); const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); +/** + * The flows a dry run lists: the ones a real run would submit, after the + * include/exclude tags and excludeFlows, as `dcd cloud --dry-run` lists + * them. `flowMetadata` holds every flow in the folder, filtered out or not, + * so it can't be the list. + */ +export function dryRunFlows( + executionPlan: Pick, + testMetadataMap: Record, + commonRoot: string, +): { file: string; flowName: string; tags: string[] }[] { + return [...executionPlan.flowsToRun, ...(executionPlan.sequence?.flows ?? [])].map((absolute) => { + const file = toPortableRelativePath(absolute, commonRoot); + const meta = testMetadataMap[file]; + return { file, flowName: meta?.flowName ?? path.parse(absolute).name, tags: meta?.tags ?? [] }; + }); +} + /** * Submit a Maestro flow (or directory of flows) to devicecloud.dev. * @@ -198,11 +221,7 @@ export function registerRunCloudTest(server: McpServer): void { if (args.dryRun) { return jsonResult({ dryRun: true, - flows: Object.entries(testMetadataMap).map(([file, meta]) => ({ - file, - flowName: meta.flowName, - tags: meta.tags, - })), + flows: dryRunFlows(executionPlan, testMetadataMap, commonRoot), sequentialFlowCount: executionPlan.sequence?.flows.length ?? 0, }); } diff --git a/test/unit/mcp-bridge.test.ts b/test/unit/mcp-bridge.test.ts index 622cdbd..ead46a9 100644 --- a/test/unit/mcp-bridge.test.ts +++ b/test/unit/mcp-bridge.test.ts @@ -11,7 +11,13 @@ import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; import { z } from 'zod'; -import { createBridgeServer, resolveLocalInputs, withLocalInputs } from '../../src/mcp/bridge.js'; +import { + createBridgeServer, + explainFailedStart, + flowFileBundle, + resolveLocalInputs, + withLocalInputs, +} from '../../src/mcp/bridge.js'; import { clearContextCache } from '../../src/mcp/context.js'; /** @@ -20,13 +26,21 @@ import { clearContextCache } from '../../src/mcp/context.js'; */ function startHosted(): Promise<{ calls: { args: unknown; auth?: string; name: string }[]; + failStart: boolean; http: HttpServer; inits: number; + sessions: Record[]; url: string; }> { const calls: { args: unknown; auth?: string; name: string }[] = []; - const hosted = { calls, inits: 0 }; + const hosted = { calls, failStart: false, inits: 0, sessions: [] as Record[] }; const http = createHttpServer(async (req, res) => { + // The open sessions, as GET /device/sessions answers. + if (req.method === 'GET' && req.url === '/device/sessions') { + res.writeHead(200, { 'content-type': 'application/json' }).end(JSON.stringify({ sessions: hosted.sessions })); + return; + } + // Stateless, as the API: no GET stream, no session to delete. if (req.method !== 'POST') { res.writeHead(405).end(); @@ -37,6 +51,11 @@ function startHosted(): Promise<{ for await (const c of req) chunks.push(c as Buffer); const body = JSON.parse(Buffer.concat(chunks).toString() || '{}'); if (body.method === 'initialize') hosted.inits++; + // A start whose answer never arrives, as when a proxy gives up on it. + if (hosted.failStart && body.method === 'tools/call' && body.params?.name === 'start_device') { + res.writeHead(502, { 'content-type': 'application/json' }).end('{"message":"Application failed to respond"}'); + return; + } const auth = req.headers['x-app-api-key'] as string | undefined; const server = new McpServer({ name: 'hosted', version: '1' }); const record = (name: string) => async (args: unknown) => { @@ -64,6 +83,11 @@ function startHosted(): Promise<{ return { content: [{ type: 'text' as const, text: 'late' }] }; }, ); + server.registerTool( + 'start_device', + { description: 'Start a device.', inputSchema: { platform: z.string() } }, + record('start_device'), + ); server.registerTool( 'inspect_screen', { description: 'Inspect.', inputSchema: {}, annotations: { readOnlyHint: true } }, @@ -181,6 +205,34 @@ describe('dcd mcp bridge', () => { await client.close(); }); + it('says a failed start may have made a session, and lists the recent ones', async () => { + hosted.failStart = true; + const now = Date.now(); + hosted.sessions = [ + { client_label: 'claude-code', created_at: new Date(now - 30_000).toISOString(), id: 'new-one', state: 'provisioning' }, + { client_label: 'other', created_at: new Date(now - 30 * 60_000).toISOString(), id: 'old-one', state: 'active' }, + ]; + const client = await connect(); + await client.listTools(); + const result = await client.callTool({ name: 'start_device', arguments: { platform: 'ios' } }); + expect(result.isError).to.equal(true); + const text = JSON.stringify(result.content); + expect(text).to.include('may still have started a session'); + expect(text).to.include('new-one'); + expect(text).to.not.include('old-one'); + expect(text).to.include('list_devices'); + await client.close(); + }); + + it('leaves a start that answered alone', async () => { + const client = await connect(); + await client.listTools(); + const result = await client.callTool({ name: 'start_device', arguments: { platform: 'ios' } }); + expect(result.isError).to.not.equal(true); + expect(hosted.calls.map((c) => c.name)).to.deep.equal(['start_device']); + await client.close(); + }); + it('read-only mode keeps only device tools that change nothing', async () => { process.env.DCD_MCP_READONLY = '1'; const client = await connect(); @@ -211,6 +263,49 @@ describe('dcd mcp bridge', () => { }); describe('helpers', () => { + it('explainFailedStart says it is safe to retry when no session started', async () => { + hosted.sessions = []; + const ctx = { apiUrl: hosted.url, auth: { headers: {}, mode: 'apiKey' as const } }; + const text = await explainFailedStart('fetch failed', ctx); + expect(text).to.include('safe to call start_device again'); + }); + + it('explainFailedStart still warns when the sessions cannot be read', async () => { + const ctx = { apiUrl: 'http://127.0.0.1:9', auth: { headers: {}, mode: 'apiKey' as const } }; + const text = await explainFailedStart('fetch failed', ctx); + expect(text).to.include('Call list_devices to check'); + }); + + it('flowFileBundle sends a flow that uses no other files as yaml', () => { + const flow = path.join(tempDir, 'plain.yaml'); + fs.writeFileSync(flow, 'appId: x\n---\n- launchApp\n- tapOn: OK\n'); + expect(flowFileBundle(flow)).to.equal(null); + }); + + it('flowFileBundle uploads the folder of a flow that runs a subflow or a script', () => { + fs.mkdirSync(path.join(tempDir, 'flows', 'subflows'), { recursive: true }); + fs.writeFileSync(path.join(tempDir, 'flows', 'subflows', 'login.yaml'), 'appId: x\n---\n- tapOn: Log in\n'); + fs.writeFileSync(path.join(tempDir, 'flows', 'setup.js'), 'output.ok = true\n'); + const flow = path.join(tempDir, 'flows', 'main.yaml'); + fs.writeFileSync(flow, 'appId: x\n---\n- runFlow: subflows/login.yaml\n- runScript: setup.js\n'); + expect(flowFileBundle(flow)).to.deep.equal({ dir: path.join(tempDir, 'flows'), entry: 'main.yaml' }); + }); + + it('flowFileBundle reaches up to a shared folder the flow uses', () => { + fs.mkdirSync(path.join(tempDir, 'shared'), { recursive: true }); + fs.mkdirSync(path.join(tempDir, 'ios'), { recursive: true }); + fs.writeFileSync(path.join(tempDir, 'shared', 'login.yaml'), 'appId: x\n---\n- tapOn: Log in\n'); + const flow = path.join(tempDir, 'ios', 'main.yaml'); + fs.writeFileSync(flow, 'appId: x\n---\n- runFlow: ../shared/login.yaml\n'); + expect(flowFileBundle(flow)).to.deep.equal({ dir: tempDir, entry: path.join('ios', 'main.yaml') }); + }); + + it('flowFileBundle still bundles a flow whose subflow is missing, so the upload names it', () => { + const flow = path.join(tempDir, 'broken.yaml'); + fs.writeFileSync(flow, 'appId: x\n---\n- runFlow: subflows/missing.yaml\n'); + expect(flowFileBundle(flow)).to.deep.equal({ dir: tempDir, entry: 'broken.yaml' }); + }); + it('withLocalInputs adds the local inputs and leaves other tools alone', () => { const tool = { name: 'run', description: 'Run.', inputSchema: { type: 'object' as const, properties: { yaml: {} } } }; const patched = withLocalInputs(tool); diff --git a/test/unit/mcp-dry-run.test.ts b/test/unit/mcp-dry-run.test.ts new file mode 100644 index 0000000..d927b2a --- /dev/null +++ b/test/unit/mcp-dry-run.test.ts @@ -0,0 +1,69 @@ +import { expect } from 'chai'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +import { dryRunFlows } from '../../src/mcp/tools/run-cloud-test.js'; +import { plan } from '../../src/services/execution-plan.service.js'; +import { buildTestMetadataMap, computeCommonRoot } from '../../src/services/flow-paths.js'; + +/** What dcd_run_cloud_test's dry run lists for a folder, given these filters. */ +async function preview(dir: string, filters: { excludeTags?: string[]; includeTags?: string[] }) { + const executionPlan = await plan({ + excludeTags: filters.excludeTags ?? [], + includeTags: filters.includeTags ?? [], + input: `${dir}/`, + warn() {}, + }); + const commonRoot = computeCommonRoot( + executionPlan.flowsToRun, + executionPlan.referencedFiles, + executionPlan.includedFiles, + ); + const map = buildTestMetadataMap(executionPlan.flowMetadata, commonRoot); + return { + // What `dcd cloud --dry-run` lists. + cli: executionPlan.flowsToRun.map((f) => path.basename(f)).sort(), + mcp: dryRunFlows(executionPlan, map, commonRoot) + .map((f) => path.basename(f.file)) + .sort(), + }; +} + +describe('dcd_run_cloud_test dry run', () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'dcd-dry-run-')); + const flow = (name: string, tag: string) => + fs.writeFileSync(path.join(dir, name), `appId: x\ntags:\n - ${tag}\n---\n- launchApp\n`); + flow('android-a.yaml', 'android'); + flow('android-b.yaml', 'android'); + flow('ios-a.yaml', 'ios'); + }); + + afterEach(() => fs.rmSync(dir, { force: true, recursive: true })); + + it('lists only the flows the include tags pick, as the CLI does', async () => { + const { cli, mcp } = await preview(dir, { includeTags: ['ios'] }); + expect(mcp).to.deep.equal(['ios-a.yaml']); + expect(mcp).to.deep.equal(cli); + }); + + it('leaves out the flows the exclude tags drop, as the CLI does', async () => { + const { cli, mcp } = await preview(dir, { excludeTags: ['android'] }); + expect(mcp).to.deep.equal(['ios-a.yaml']); + expect(mcp).to.deep.equal(cli); + }); + + it('lists every flow with no filters, with its tags', async () => { + const executionPlan = await plan({ excludeTags: [], includeTags: [], input: `${dir}/`, warn() {} }); + const root = computeCommonRoot(executionPlan.flowsToRun, [], []); + const flows = dryRunFlows(executionPlan, buildTestMetadataMap(executionPlan.flowMetadata, root), root); + expect(flows).to.deep.equal([ + { file: './android-a.yaml', flowName: 'android-a', tags: ['android'] }, + { file: './android-b.yaml', flowName: 'android-b', tags: ['android'] }, + { file: './ios-a.yaml', flowName: 'ios-a', tags: ['ios'] }, + ]); + }); +});