From 190221ef5060595f2b2872e9d7ffd20043c93068 Mon Sep 17 00:00:00 2001 From: finalerock44 <77282157+finalerock44@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:19:28 +0100 Subject: [PATCH] fix(cloud): reject malformed executionOrder instead of silently running in parallel A workspace config.yaml was yaml.load'ed and straight-cast to IWorkspaceConfig, so a wrong-shaped executionOrder was never checked. The intuitive bare-list form made executionOrder an Array, .flowsOrder came back undefined, resolveSequentialFlows returned [], and every flow ran in parallel - same cost, wrong semantics, green run. The only symptom was depends_on being null on every result row. Add a zod schema as the single source of truth for the config shape (src/services/workspace-config.schema.ts) and route all three former cast sites through one validated loader, loadWorkspaceConfig: - A malformed executionOrder is now fatal (exit 1), with a message showing what was found next to the expected shape. A bare list is not valid Maestro either, so there is nothing to accept - and a warning in CI logs is exactly what got missed. - Unrecognised top-level keys warn (and are preserved, since the config is forwarded to the API as fields.workspaceConfig), catching flowOrder, a top-level continueOnFailure, tags in place of includeTags, and flowTimeout. - executionOrder on a single-file input warns instead of being dropped: planSingleFile never sequences, so it was silently ignored even when well-formed. - continueOnFailure's real default (true) now lives in the schema instead of being re-specified at three read sites. - WORKSPACE_CONFIG_KEYS is derived from the schema so isWorkspaceConfigFile's detection set can no longer drift from it. - includeTags/excludeTags scalar coercion moves from readYamlFileAsJson into the schema, so the loader is a plain YAML read and the validator is pure. Warnings go through an injected callback: cloud.ts passes logger.warn (stderr, so it survives --json), the MCP tool passes logStderr since its stdout is the JSON-RPC channel. Also fixes two test fixtures that used a tags: key the CLI never read. Verified on dev: the bare-list form now exits 1 before anything is submitted, and a well-formed executionOrder chains depends_on null -> 36962 -> 36963 across results 36962-36964. Fixes #110 --- src/commands/cloud.ts | 3 + src/mcp/tools/run-cloud-test.ts | 2 + src/services/execution-plan.service.ts | 80 +++---- src/services/execution-plan.utils.ts | 57 +++-- src/services/workspace-config.schema.ts | 255 +++++++++++++++++++++ test/fixtures/basic-config.yaml | 9 +- test/fixtures/tag-filtering-config.yaml | 11 +- test/integration/cloud.integration.test.ts | 84 +++++++ 8 files changed, 412 insertions(+), 89 deletions(-) create mode 100644 src/services/workspace-config.schema.ts diff --git a/src/commands/cloud.ts b/src/commands/cloud.ts index 632a2b2..04ef937 100644 --- a/src/commands/cloud.ts +++ b/src/commands/cloud.ts @@ -569,6 +569,9 @@ export const cloudCommand = defineCommand({ excludeFlows, configFile, debug, + // Not warnOut: config problems are worth surfacing even under --json, + // and logger.warn writes to stderr so stdout stays parseable. + warn: (m: string) => logger.warn(m), }); if (debug) { diff --git a/src/mcp/tools/run-cloud-test.ts b/src/mcp/tools/run-cloud-test.ts index 2cb7072..5c94dca 100644 --- a/src/mcp/tools/run-cloud-test.ts +++ b/src/mcp/tools/run-cloud-test.ts @@ -151,6 +151,8 @@ export function registerRunCloudTest(server: McpServer): void { excludeTags: args.excludeTags ?? [], excludeFlows: args.excludeFlows, configFile: args.configFile, + // stdout is the JSON-RPC channel — config warnings must go to stderr. + warn: logStderr, }); const commonRoot = computeCommonRoot( diff --git a/src/services/execution-plan.service.ts b/src/services/execution-plan.service.ts index 9fa6c72..9c1284c 100644 --- a/src/services/execution-plan.service.ts +++ b/src/services/execution-plan.service.ts @@ -5,45 +5,12 @@ import { getFlowsToRunInSequence, isFlowFile, isWorkspaceConfigFile, + loadWorkspaceConfig, processDependencies, readDirectory, readTestYamlFileAsJson, - readYamlFileAsJson, } from './execution-plan.utils.js'; - -/** Email notification configuration */ -interface INotificationsConfig { - email?: { - enabled?: boolean; - onSuccess?: boolean; - recipients?: string[]; - }; -} - -/** Workspace configuration from config.yaml */ -interface IWorkspaceConfig { - excludeTags?: null | string[]; - executionOrder?: IExecutionOrder | null; - flows?: null | string[]; - includeTags?: null | string[]; - local?: ILocal | null; - notifications?: INotificationsConfig; - platform?: { - android?: { disableAnimations?: boolean }; - ios?: { disableAnimations?: boolean }; - }; -} - -/** Local execution configuration */ -interface ILocal { - deterministicOrder: boolean | null; -} - -/** Sequential execution configuration */ -interface IExecutionOrder { - continueOnFailure: boolean; - flowsOrder: string[]; -} +import { IWorkspaceConfig } from './workspace-config.schema.js'; /** Options for execution plan generation */ export interface PlanOptions { @@ -53,6 +20,12 @@ export interface PlanOptions { excludeTags?: string[]; includeTags?: string[]; input: string; + /** + * Sink for non-fatal config problems. Injected rather than imported so the + * MCP server can route warnings to stderr — its stdout is the JSON-RPC + * channel. + */ + warn?: (message: string) => void; } /** Execution plan containing all flows to run with metadata and dependencies */ @@ -146,11 +119,13 @@ function filterFlowFiles( * Load workspace configuration from config.yaml/yml if present * @param input - Input directory path * @param unfilteredFlowFiles - List of discovered flow files + * @param warn - Sink for non-fatal config problems * @returns Workspace configuration object (empty if no config file found) */ function getWorkspaceConfig( input: string, unfilteredFlowFiles: string[], + warn: (message: string) => void, ): IWorkspaceConfig { const possibleConfigPaths = new Set( [path.join(input, 'config.yaml'), path.join(input, 'config.yml')].map((p) => @@ -162,11 +137,7 @@ function getWorkspaceConfig( possibleConfigPaths.has(path.normalize(file)), ); - const config = configFilePath - ? (readYamlFileAsJson(configFilePath) as IWorkspaceConfig) - : {}; - - return config; + return configFilePath ? loadWorkspaceConfig(configFilePath, warn) : {}; } /** @@ -199,11 +170,13 @@ function extractDeviceCloudOverrides( /** * Generate execution plan for a single flow file * @param normalizedInput - Normalized path to the flow file + * @param warn - Sink for non-fatal config problems * @param resolvedConfigFile - Optional absolute path to a custom config file * @returns Execution plan for the single file with dependencies */ async function planSingleFile( normalizedInput: string, + warn: (message: string) => void, resolvedConfigFile?: string, ): Promise { const inputBasename = path.basename(normalizedInput); @@ -232,9 +205,17 @@ async function planSingleFile( throw new Error(`Config file does not exist: ${resolvedConfigFile}`); } - workspaceConfig = readYamlFileAsJson( - resolvedConfigFile, - ) as IWorkspaceConfig; + workspaceConfig = loadWorkspaceConfig(resolvedConfigFile, warn); + + // Sequencing is resolved against a workspace's discovered flows, which a + // single-file input doesn't have — so executionOrder is ignored here. Say so + // rather than accepting a config that reads as if it applied (dcd-cli#110). + if (workspaceConfig.executionOrder?.flowsOrder.length) { + warn( + `Warning: \`executionOrder\` in ${resolvedConfigFile} is ignored when a single flow file is passed.\n` + + `Pass the workspace folder instead so the named flows can be discovered and sequenced.`, + ); + } } const checkedDependancies = await checkDependencies(normalizedInput); @@ -386,6 +367,7 @@ export async function plan(options: PlanOptions): Promise { excludeFlows, configFile, debug = false, + warn = (message: string) => console.warn(message), } = options; const normalizedInput = path.normalize(input); const flowMetadata: Record> = {}; @@ -400,7 +382,7 @@ export async function plan(options: PlanOptions): Promise { } if (fs.lstatSync(normalizedInput).isFile()) { - return planSingleFile(normalizedInput, resolvedConfigFile); + return planSingleFile(normalizedInput, warn, resolvedConfigFile); } let unfilteredFlowFiles = await readDirectory(normalizedInput, isFlowFile); @@ -420,11 +402,13 @@ export async function plan(options: PlanOptions): Promise { throw new Error(`Config file does not exist: ${resolvedConfigFile}`); } - workspaceConfig = readYamlFileAsJson( - resolvedConfigFile, - ) as IWorkspaceConfig; + workspaceConfig = loadWorkspaceConfig(resolvedConfigFile, warn); } else { - workspaceConfig = getWorkspaceConfig(normalizedInput, unfilteredFlowFiles); + workspaceConfig = getWorkspaceConfig( + normalizedInput, + unfilteredFlowFiles, + warn, + ); } unfilteredFlowFiles = await applyFlowGlobs( diff --git a/src/services/execution-plan.utils.ts b/src/services/execution-plan.utils.ts index 3b7f24b..dbb23ee 100644 --- a/src/services/execution-plan.utils.ts +++ b/src/services/execution-plan.utils.ts @@ -3,6 +3,12 @@ import * as yaml from 'js-yaml'; import * as fs from 'node:fs'; import * as path from 'node:path'; +import { + IWorkspaceConfig, + parseWorkspaceConfig, + WORKSPACE_CONFIG_KEYS, +} from './workspace-config.schema.js'; + const commandsThatRequireFiles = new Set(['addMedia', 'runFlow', 'runScript']); export function getFlowsToRunInSequence( @@ -60,22 +66,6 @@ export function isFlowFile(filePath: string): boolean { return filePath.endsWith('.yaml') || filePath.endsWith('.yml'); } -/** - * Top-level keys that only ever appear in a workspace config (see - * IWorkspaceConfig in execution-plan.service.ts). Deliberately excludes keys - * Maestro also allows in flow front matter — appId, name, tags, env, - * onFlowStart, onFlowComplete, jsEngine. - */ -const WORKSPACE_CONFIG_KEYS = new Set([ - 'excludeTags', - 'executionOrder', - 'flows', - 'includeTags', - 'local', - 'notifications', - 'platform', -]); - /** * True when a YAML file is a workspace config rather than a runnable flow. * @@ -112,20 +102,7 @@ export const readYamlFileAsJson = (filePath: string) => { const normalizedPath = path.normalize(filePath); const yamlText = fs.readFileSync(normalizedPath, 'utf8'); - const result = yaml.load(yamlText); - - // Ensure includeTags and excludeTags are always arrays if present - if (result && typeof result === 'object') { - if ('includeTags' in result && !Array.isArray(result.includeTags)) { - result.includeTags = result.includeTags ? [result.includeTags] : []; - } - - if ('excludeTags' in result && !Array.isArray(result.excludeTags)) { - result.excludeTags = result.excludeTags ? [result.excludeTags] : []; - } - } - - return result; + return yaml.load(yamlText); } catch (error) { throw new Error(`Error parsing YAML file ${filePath}: ${error}`, { cause: error, @@ -133,6 +110,26 @@ export const readYamlFileAsJson = (filePath: string) => { } }; +/** + * Load and validate a workspace config file. + * + * The single chokepoint for reading a config: every caller gets a + * runtime-validated object instead of an unchecked `as IWorkspaceConfig` cast. + * Scalar-to-array coercion for `includeTags`/`excludeTags` lives in the schema, + * so `readYamlFileAsJson` stays a plain YAML read. + * + * @param filePath - Path to the config file + * @param warn - Sink for non-fatal problems (unrecognised keys) + * @returns The validated workspace config + * @throws Error if the file is unparseable or the config is invalid + */ +export function loadWorkspaceConfig( + filePath: string, + warn: (message: string) => void, +): IWorkspaceConfig { + return parseWorkspaceConfig(readYamlFileAsJson(filePath), { filePath, warn }); +} + export const readTestYamlFileAsJson = (filePath: string) => { try { const normalizedPath = path.normalize(filePath); diff --git a/src/services/workspace-config.schema.ts b/src/services/workspace-config.schema.ts new file mode 100644 index 0000000..960cee9 --- /dev/null +++ b/src/services/workspace-config.schema.ts @@ -0,0 +1,255 @@ +import * as yaml from 'js-yaml'; +import { z } from 'zod'; + +/** + * Runtime schema for a workspace `config.yaml`. + * + * This is the single source of truth for the config's shape — the TypeScript + * type is inferred from it (`IWorkspaceConfig`) rather than declared alongside + * it, so the compile-time and runtime views cannot drift. Before this existed + * the config was `yaml.load`ed and straight-cast, which meant an + * `executionOrder` written in the wrong shape was silently ignored and every + * flow ran in parallel (dcd-cli#110). + */ + +/** + * Tags may be written as a bare scalar (`includeTags: smoke`) or a list. + * Scalars are wrapped, and primitive members are coerced to strings so a + * YAML-numeric tag (`includeTags: [2]`) still compares against flow tags + * instead of silently matching nothing. + */ +const tagList = z.preprocess((value) => { + if (value === null || value === undefined) return value; + const members = Array.isArray(value) ? value : [value]; + return members.map((member) => + typeof member === 'number' || typeof member === 'boolean' + ? String(member) + : member, + ); +}, z.array(z.string())); + +/** Sequential execution configuration */ +const ExecutionOrderSchema = z.object({ + // Defaults to true: a failing flow does not stop the rest of the sequence. + // Declared here so the default lives in one place instead of being + // re-specified at each read site. + continueOnFailure: z.boolean().default(true), + flowsOrder: z.array(z.string()), +}); + +/** + * `looseObject`, not `object`: unknown keys are reported as warnings but must + * survive parsing, because the whole config is forwarded to the API as + * `fields.workspaceConfig` and stripping keys would silently alter that + * payload. + */ +export const WorkspaceConfigSchema = z.looseObject({ + excludeTags: tagList.nullish(), + executionOrder: ExecutionOrderSchema.nullish(), + flows: z.array(z.string()).nullish(), + includeTags: tagList.nullish(), + local: z + .looseObject({ deterministicOrder: z.boolean().nullish() }) + .nullish(), + notifications: z + .looseObject({ + email: z + .looseObject({ + enabled: z.boolean().optional(), + onSuccess: z.boolean().optional(), + recipients: z.array(z.string()).optional(), + }) + .optional(), + }) + .nullish(), + platform: z + .looseObject({ + android: z + .looseObject({ disableAnimations: z.boolean().optional() }) + .optional(), + ios: z + .looseObject({ disableAnimations: z.boolean().optional() }) + .optional(), + }) + .nullish(), +}); + +/** Workspace configuration from config.yaml */ +export type IWorkspaceConfig = z.infer; + +/** + * Top-level keys that only ever appear in a workspace config, derived from the + * schema so the two can't drift. Deliberately excludes keys Maestro also allows + * in flow front matter — appId, name, tags, env, onFlowStart, onFlowComplete, + * jsEngine — because this set also drives config-vs-flow detection + * (`isWorkspaceConfigFile`). + */ +export const WORKSPACE_CONFIG_KEYS: ReadonlySet = new Set( + Object.keys(WorkspaceConfigSchema.shape), +); + +/** + * Near-misses that aren't just a casing slip on a real key. Keyed lowercase. + */ +const KEY_ALIASES: Record = { + continueonfailure: 'executionOrder.continueOnFailure', + excludetag: 'excludeTags', + floworder: 'executionOrder.flowsOrder', + flowsorder: 'executionOrder.flowsOrder', + includetag: 'includeTags', + tags: 'includeTags / excludeTags', +}; + +/** + * Suggest the key the author probably meant. + * @param unknownKey - Unrecognised top-level key from the config + * @returns The suggested key name, or undefined if there's no close match + */ +function suggestKey(unknownKey: string): string | undefined { + const lowered = unknownKey.toLowerCase(); + const casingSlip = [...WORKSPACE_CONFIG_KEYS].find( + (key) => key.toLowerCase() === lowered, + ); + + return casingSlip ?? KEY_ALIASES[lowered]; +} + +/** + * Describe how an `executionOrder` value is malformed, in prose. + * + * Split out so the most likely mistake — a bare list of flow names — gets a + * message showing the expected shape rather than a generic schema dump. + * + * @param value - The raw `executionOrder` value from the config + * @returns A description of the problem, or undefined if the shape is valid + */ +function describeExecutionOrderShape(value: unknown): string | undefined { + if (value === null || value === undefined) return undefined; + if (Array.isArray(value)) return 'a bare list of flow names'; + if (typeof value !== 'object') return `a ${typeof value} value`; + + const { flowsOrder } = value as Record; + if (flowsOrder === undefined) return 'a map with no `flowsOrder` key'; + if (!Array.isArray(flowsOrder)) { + return 'a map whose `flowsOrder` is not a list'; + } + + return undefined; +} + +/** Indent a YAML fragment so it reads as a block inside an error message. */ +function indentYaml(value: unknown): string { + return yaml + .dump(value, { lineWidth: -1 }) + .trimEnd() + .split('\n') + .map((line) => ` ${line}`) + .join('\n'); +} + +/** + * Build the error for a malformed `executionOrder`, showing what was found + * next to what was expected. + * @param filePath - Path to the config file, for the message + * @param problem - Prose description from describeExecutionOrderShape + * @param found - The raw `executionOrder` value that failed + * @returns The error to throw + */ +function executionOrderError( + filePath: string, + problem: string, + found: unknown, +): Error { + return new Error( + `Invalid \`executionOrder\` in ${filePath}\n\n` + + `\`executionOrder\` must be a map containing a \`flowsOrder\` list, but it is ${problem}.\n\n` + + `Found:\n${indentYaml({ executionOrder: found })}\n\n` + + `Expected:\n${indentYaml({ + executionOrder: { + continueOnFailure: true, + flowsOrder: ['first-flow', 'second-flow'], + }, + })}\n\n` + + `Without \`flowsOrder\` the flows are not sequenced — they all run in parallel.`, + ); +} + +/** + * Validate a parsed workspace config. + * + * Hard errors (thrown): the config isn't a map, or `executionOrder` is present + * but malformed. Both are unambiguous mistakes with no valid interpretation, so + * failing fast beats a warning that scrolls past in CI. + * + * Soft problems (warned): unknown top-level keys. These are preserved, not + * stripped, and warned about once. + * + * @param raw - The result of loading the YAML file + * @param options - Config file path (for messages) and a warning sink + * @returns The validated config, with defaults applied + * @throws Error if the config is not a map or `executionOrder` is malformed + */ +export function parseWorkspaceConfig( + raw: unknown, + options: { filePath: string; warn: (message: string) => void }, +): IWorkspaceConfig { + const { filePath, warn } = options; + + if (raw === null || raw === undefined) return {}; + + if (typeof raw !== 'object' || Array.isArray(raw)) { + throw new Error( + `Invalid workspace config in ${filePath}\n\n` + + `Expected a map of configuration keys (${[...WORKSPACE_CONFIG_KEYS].join( + ', ', + )}), but found ${Array.isArray(raw) ? 'a list' : `a ${typeof raw} value`}.`, + ); + } + + const rawConfig = raw as Record; + + // Checked ahead of the schema so this specific mistake gets a targeted + // message; the schema would otherwise report "expected object, received array". + const executionOrderProblem = describeExecutionOrderShape( + rawConfig.executionOrder, + ); + if (executionOrderProblem) { + throw executionOrderError( + filePath, + executionOrderProblem, + rawConfig.executionOrder, + ); + } + + const unknownKeys = Object.keys(rawConfig).filter( + (key) => !WORKSPACE_CONFIG_KEYS.has(key), + ); + if (unknownKeys.length > 0) { + const lines = unknownKeys.map((key) => { + const suggestion = suggestKey(key); + return suggestion + ? ` ${key} — did you mean ${suggestion}?` + : ` ${key}`; + }); + + warn( + `Warning: unrecognised key(s) in ${filePath} — these are ignored:\n` + + `${lines.join('\n')}\n\n` + + `Supported keys: ${[...WORKSPACE_CONFIG_KEYS].join(', ')}`, + ); + } + + const result = WorkspaceConfigSchema.safeParse(rawConfig); + if (!result.success) { + const issues = result.error.issues + .map((issue) => { + const location = issue.path.length > 0 ? issue.path.join('.') : '(root)'; + return ` ${location}: ${issue.message}`; + }) + .join('\n'); + + throw new Error(`Invalid workspace config in ${filePath}\n\n${issues}`); + } + + return result.data; +} diff --git a/test/fixtures/basic-config.yaml b/test/fixtures/basic-config.yaml index 0671d08..3c49ac4 100644 --- a/test/fixtures/basic-config.yaml +++ b/test/fixtures/basic-config.yaml @@ -1,8 +1,7 @@ flows: - ./**/*.yaml - ./*.yaml -tags: - include: - - smoke - exclude: - - slow \ No newline at end of file +includeTags: + - smoke +excludeTags: + - slow diff --git a/test/fixtures/tag-filtering-config.yaml b/test/fixtures/tag-filtering-config.yaml index c08d93c..bb28996 100644 --- a/test/fixtures/tag-filtering-config.yaml +++ b/test/fixtures/tag-filtering-config.yaml @@ -1,8 +1,7 @@ flows: - ./**/*.yaml -tags: - include: - - smoke - exclude: - - slow - - integration \ No newline at end of file +includeTags: + - smoke +excludeTags: + - slow + - integration diff --git a/test/integration/cloud.integration.test.ts b/test/integration/cloud.integration.test.ts index 8ac4c93..39d77d5 100644 --- a/test/integration/cloud.integration.test.ts +++ b/test/integration/cloud.integration.test.ts @@ -408,6 +408,90 @@ includeTags: }); }); + // dcd-cli#110: a `config.yaml` whose executionOrder was the wrong *shape* was + // silently ignored and every flow ran in parallel — same cost, wrong + // semantics, green run. These assert the shape is now validated. + describe('executionOrder validation', () => { + let workspaceDir: string; + + /** Point the run at a workspace whose config.yaml holds `configBody`. */ + const commandWithConfig = (configBody: string): string => { + fs.writeFileSync(path.join(workspaceDir, 'config.yaml'), configBody); + return `${CLI} cloud ${androidAppFile} "${workspaceDir}" --api-key ${mockApiKey} --api-url ${mockApiUrl} --dry-run`; + }; + + before(() => { + workspaceDir = fs.mkdtempSync(path.join(os.tmpdir(), 'dcd-test-order-')); + + for (const name of ['a', 'b', 'c']) { + fs.writeFileSync( + path.join(workspaceDir, `${name}.yaml`), + `appId: com.example.app +--- +- launchApp +`, + ); + } + }); + + after(() => { + if (fs.existsSync(workspaceDir)) { + fs.rmSync(workspaceDir, { force: true, recursive: true }); + } + }); + + it('should reject a bare-list executionOrder instead of running in parallel', async () => { + const command = commandWithConfig(`executionOrder: + - a.yaml + - b.yaml + - c.yaml +`); + + const { output } = await runExpectingFailure(command); + expect(output).to.include('Invalid `executionOrder`'); + expect(output).to.include('flowsOrder'); + // The whole point: it must not quietly proceed to a parallel run. + expect(output).to.not.include('The following tests would have been run'); + }); + + it('should reject an executionOrder map with no flowsOrder', async () => { + const command = commandWithConfig(`executionOrder: + continueOnFailure: true +`); + + const { output } = await runExpectingFailure(command); + expect(output).to.include('Invalid `executionOrder`'); + expect(output).to.include('no `flowsOrder` key'); + }); + + it('should sequence flows for a well-formed executionOrder', async () => { + const command = commandWithConfig(`executionOrder: + continueOnFailure: true + flowsOrder: + - a.yaml + - b.yaml +`); + + const { stdout } = await exec(command, { timeout: 15_000 }); + expect(stdout).to.include('Sequential flows'); + expect(stdout).to.include('a.yaml'); + expect(stdout).to.include('b.yaml'); + }); + + it('should warn about unrecognised config keys without failing the run', async () => { + const command = commandWithConfig(`flowOrder: + - a.yaml +flowTimeout: 120000 +`); + + const { stdout, stderr } = await exec(command, { timeout: 15_000 }); + expect(stdout).to.include('The following tests would have been run'); + expect(stderr).to.include('flowOrder'); + expect(stderr).to.include('executionOrder.flowsOrder'); + expect(stderr).to.include('flowTimeout'); + }); + }); + describe('file and binary management', () => { it('should support app binary ID instead of file', async () => { const command = `${CLI} cloud --app-binary-id test-binary-123 ${testFlowFile} --api-key ${mockApiKey} --api-url ${mockApiUrl} --dry-run`;