diff --git a/src/commands/cloud.ts b/src/commands/cloud.ts index a7173a3..deac9d9 100644 --- a/src/commands/cloud.ts +++ b/src/commands/cloud.ts @@ -621,6 +621,7 @@ export const cloudCommand = defineCommand({ flowMetadata, flowOverrides, flowsToRun: testFileNames, + includedFiles, referencedFiles, sequence, } = executionPlan; @@ -635,10 +636,27 @@ export const cloudCommand = defineCommand({ out(`[DEBUG] Test file names: ${testFileNames.join(', ')}`); } - const commonRoot = computeCommonRoot(testFileNames, referencedFiles); + const commonRoot = computeCommonRoot( + testFileNames, + referencedFiles, + includedFiles, + ); if (debug) { out(`[DEBUG] Common root directory: ${commonRoot}`); + + // `includedPaths` files sitting beside the flows tree rather than + // inside it raise the common root, so every server-side flow key gains + // a leading segment. Harmless but visible in the console, so say it. + const rootWithoutIncludes = computeCommonRoot( + testFileNames, + referencedFiles, + ); + if (includedFiles.length > 0 && rootWithoutIncludes !== commonRoot) { + out( + `[DEBUG] \`includedPaths\` raised the common root from ${rootWithoutIncludes} to ${commonRoot} — flow paths gain a leading segment`, + ); + } } const testMetadataMap = buildTestMetadataMap(flowMetadata, commonRoot); diff --git a/src/mcp/tools/run-cloud-test.ts b/src/mcp/tools/run-cloud-test.ts index 5c94dca..a94d07e 100644 --- a/src/mcp/tools/run-cloud-test.ts +++ b/src/mcp/tools/run-cloud-test.ts @@ -158,6 +158,7 @@ export function registerRunCloudTest(server: McpServer): void { const commonRoot = computeCommonRoot( executionPlan.flowsToRun, executionPlan.referencedFiles, + executionPlan.includedFiles, ); const testMetadataMap = buildTestMetadataMap( executionPlan.flowMetadata, diff --git a/src/services/execution-plan.service.ts b/src/services/execution-plan.service.ts index 9c1284c..994c378 100644 --- a/src/services/execution-plan.service.ts +++ b/src/services/execution-plan.service.ts @@ -35,6 +35,12 @@ export interface IExecutionPlan { flowMetadata: Record>; flowOverrides: Record>; flowsToRun: string[]; + /** + * Extra files pulled in by `config.yaml`'s `includedPaths`. Kept separate + * from `referencedFiles` (which is derived from flow commands) so the zip + * manifest and the common-root calculation can tell the two apart. + */ + includedFiles: string[]; referencedFiles: string[]; sequence?: IFlowSequence | null; totalFlowFiles: number; @@ -178,6 +184,7 @@ async function planSingleFile( normalizedInput: string, warn: (message: string) => void, resolvedConfigFile?: string, + debug = false, ): Promise { const inputBasename = path.basename(normalizedInput); if ( @@ -218,11 +225,24 @@ async function planSingleFile( } } + // A single-file input has no workspace directory, so `includedPaths` (which + // only reaches here via --config) anchors on the flow file's own directory — + // the same place Maestro resolves an assertScreenshot baseline from. + const includedFiles = workspaceConfig + ? resolveIncludedPaths( + workspaceConfig, + path.dirname(normalizedInput), + warn, + debug, + ) + : []; + const checkedDependancies = await checkDependencies(normalizedInput); return { flowMetadata, flowOverrides, flowsToRun: [normalizedInput], + includedFiles, referencedFiles: [...new Set(checkedDependancies)], totalFlowFiles: 1, workspaceConfig, @@ -288,6 +308,119 @@ async function applyFlowGlobs( return unfilteredFlowFiles.filter((file) => !isExcludedConfig(file)); } +/** + * The whole archive is buffered in memory by `compressFilesFromRelativePath`, + * so an unbounded `**` glob is a real footgun. These are warn thresholds, not + * hard limits — a legitimately large baseline set should still upload. + */ +const INCLUDED_PATHS_FILE_WARN_THRESHOLD = 200; +const INCLUDED_PATHS_BYTES_WARN_THRESHOLD = 50 * 1024 * 1024; + +/** + * Resolve `config.yaml`'s `includedPaths` globs into absolute file paths. + * + * This is the general-purpose escape hatch for shipping files the flow + * commands don't reference: `assertScreenshot` baselines above all, but also + * fixtures, test data and certificates. Only `addMedia` / `runFlow` / + * `runScript` arguments are discovered by walking the flows, so without this + * key such files are silently absent from the uploaded zip. + * + * Glob semantics deliberately mirror `flows:` (`applyFlowGlobs`): patterns + * resolve against the workspace root, never the config file's directory, so + * `--config ci/workspace.yaml` behaves identically to an auto-detected config. + * + * @param workspaceConfig - Validated workspace config + * @param normalizedInput - Normalized path to the workspace directory + * @param warn - Sink for non-fatal problems + * @param debug - Whether to emit debug logging + * @returns Absolute paths of every matched file, deduped and sorted + * @throws Error if a pattern escapes the workspace root + */ +function resolveIncludedPaths( + workspaceConfig: IWorkspaceConfig, + normalizedInput: string, + warn: (message: string) => void, + debug = false, +): string[] { + const patterns = workspaceConfig.includedPaths; + if (!patterns || patterns.length === 0) return []; + + const workspaceRoot = path.resolve(normalizedInput); + const resolved = new Set(); + const unmatched: string[] = []; + + for (const pattern of patterns) { + // fs.globSync lands in Node 22; the CLI's `engines.node` already requires + // it. No `nodir` option — directories are stripped by the stat check below. + const matches = fs.globSync(pattern, { cwd: normalizedInput }); + let matchedFile = false; + + for (const match of matches) { + const absolute = path.resolve(normalizedInput, match); + + // Containment guard: `flows:` has none because its matches are only ever + // parsed as YAML, but this key ships arbitrary bytes to a remote runner, + // so a `../../../` climb must not silently leave the workspace. + const relative = path.relative(workspaceRoot, absolute); + if (relative.startsWith('..') || path.isAbsolute(relative)) { + throw new Error( + `\`includedPaths\` pattern "${pattern}" resolves outside the workspace: ${absolute}\n\n` + + `Included paths must stay within ${workspaceRoot}.`, + ); + } + + try { + if (!fs.statSync(absolute).isFile()) continue; + } catch { + continue; + } + + matchedFile = true; + resolved.add(absolute); + } + + if (!matchedFile) unmatched.push(pattern); + } + + if (unmatched.length > 0) { + warn( + `Warning: \`includedPaths\` pattern(s) in config matched no files:\n` + + `${unmatched.map((pattern) => ` ${pattern}`).join('\n')}\n\n` + + `Patterns are resolved relative to ${workspaceRoot}.`, + ); + } + + const files = [...resolved].sort((a, b) => a.localeCompare(b)); + + let totalBytes = 0; + for (const file of files) { + try { + totalBytes += fs.statSync(file).size; + } catch { + // Raced away between glob and stat; the zip step reports it properly. + } + } + + if ( + files.length > INCLUDED_PATHS_FILE_WARN_THRESHOLD || + totalBytes > INCLUDED_PATHS_BYTES_WARN_THRESHOLD + ) { + warn( + `Warning: \`includedPaths\` matched ${files.length} file(s) totalling ` + + `${Math.round(totalBytes / (1024 * 1024))} MB. The flow archive is built in ` + + `memory, so consider narrowing the patterns.`, + ); + } + + if (debug) { + console.log( + `[DEBUG] includedPaths matched ${files.length} file(s):\n${files.join('\n')}`, + ); + } + + return files; +} + /** * Resolve sequential execution order from workspace config * @param workspaceConfig - Workspace configuration with executionOrder @@ -382,7 +515,7 @@ export async function plan(options: PlanOptions): Promise { } if (fs.lstatSync(normalizedInput).isFile()) { - return planSingleFile(normalizedInput, warn, resolvedConfigFile); + return planSingleFile(normalizedInput, warn, resolvedConfigFile, debug); } let unfilteredFlowFiles = await readDirectory(normalizedInput, isFlowFile); @@ -522,6 +655,12 @@ export async function plan(options: PlanOptions): Promise { flowMetadata, flowOverrides, flowsToRun: normalFlows, + includedFiles: resolveIncludedPaths( + workspaceConfig, + normalizedInput, + warn, + debug, + ), referencedFiles: [...new Set(allFiles)], sequence: { continueOnFailure: workspaceConfig.executionOrder?.continueOnFailure, diff --git a/src/services/execution-plan.utils.ts b/src/services/execution-plan.utils.ts index dbb23ee..9fcc12c 100644 --- a/src/services/execution-plan.utils.ts +++ b/src/services/execution-plan.utils.ts @@ -9,7 +9,53 @@ import { WORKSPACE_CONFIG_KEYS, } from './workspace-config.schema.js'; -const commandsThatRequireFiles = new Set(['addMedia', 'runFlow', 'runScript']); +const commandsThatRequireFiles = new Set([ + 'addMedia', + 'assertScreenshot', + 'runFlow', + 'runScript', +]); + +/** + * Commands whose file references are best-effort rather than mandatory. + * + * `assertScreenshot` baselines are legitimately absent on a first run, and + * Maestro's own "Screenshot file not found — searched in: …" error is more + * useful than ours, so a missing baseline must not abort the upload the way a + * missing `addMedia` file does. + */ +const commandsWithOptionalFiles = new Set(['assertScreenshot']); + +/** + * Extensions Maestro's `normalizeScreenshotPath` recognises; anything else + * gets `.png` appended, so `assertScreenshot: home` means `home.png`. + */ +const SCREENSHOT_EXTENSIONS = new Set([ + '.bmp', + '.gif', + '.heic', + '.heif', + '.jpeg', + '.jpg', + '.png', + '.tiff', + '.wbmp', +]); + +/** + * Mirror Maestro's `Orchestra.normalizeScreenshotPath`: a screenshot path with + * no image extension gets `.png`. Without this, `assertScreenshot: home` looks + * like a missing file here while resolving fine on the device. + * + * @param relativePath - The path as written in the flow + * @returns The path with an image extension guaranteed + */ +function normalizeScreenshotPath(relativePath: string): string { + const extension = path.extname(relativePath).toLowerCase(); + return SCREENSHOT_EXTENSIONS.has(extension) + ? relativePath + : `${relativePath}.png`; +} export function getFlowsToRunInSequence( paths: { [key: string]: string }, @@ -189,6 +235,8 @@ export const checkIfFilesExistInWorkspace = ( const errors: string[] = []; const files: string[] = []; const directory = path.dirname(absoluteFilePath); + const isScreenshot = commandName === 'assertScreenshot'; + const isOptional = commandsWithOptionalFiles.has(commandName); const buildError = (error: string) => `Flow file "${absoluteFilePath}" has a command "${commandName}" that references a ${error} ${JSON.stringify( @@ -196,11 +244,25 @@ export const checkIfFilesExistInWorkspace = ( )}`; const processFilePath = (relativePath: string) => { + // A JS/variable-interpolated path (`screenshots/${DCD_DEVICE}/home`) can't + // be resolved without running the flow. Skip it rather than guessing — the + // config.yaml `includedPaths` key is how those files get bundled. + if (relativePath.includes('${')) return; + + const resolvedRelativePath = isScreenshot + ? normalizeScreenshotPath(relativePath) + : relativePath; const absoluteFilePath = path.normalize( - path.resolve(directory, relativePath), + path.resolve(directory, resolvedRelativePath), ); const error = checkFile(absoluteFilePath); - if (error) errors.push(buildError(error)); + if (error) { + // Optional references drop out entirely when missing: pushing them onto + // `files` would put a non-existent path into the zip manifest. + if (isOptional) return; + errors.push(buildError(error)); + } + files.push(absoluteFilePath); }; @@ -216,9 +278,13 @@ export const checkIfFilesExistInWorkspace = ( } } - // object command + // object command. `file` is addMedia/runFlow/runScript; `path` is + // assertScreenshot's own key for the same thing. const x = command as Record; // prevent annoying ts error - if (typeof command === 'object' && x?.file) processFilePath(x.file); + if (typeof command === 'object' && !Array.isArray(command)) { + if (x?.file) processFilePath(x.file); + if (isScreenshot && typeof x?.path === 'string') processFilePath(x.path); + } return { errors, files }; }; diff --git a/src/services/flow-paths.ts b/src/services/flow-paths.ts index 195bdd3..9242aed 100644 --- a/src/services/flow-paths.ts +++ b/src/services/flow-paths.ts @@ -14,14 +14,25 @@ import { toPortableRelativePath } from '../utils/paths.js'; * file path. Segment comparison (not `startsWith`) so sibling dirs like * `flows`/`flows-extra` can't merge, and the file segment itself is never * consumed. Returns '' when the paths share no root at all (or none are given). + * + * `includedFiles` (config.yaml `includedPaths`) must be folded in for the same + * reason referenced files are: the zip strips this root as an anchored prefix, + * so a file outside it would get a non-relative entry name. Folding them in + * can raise the root — flows in `flows/` beside baselines in `screenshots/` + * shifts it from `/flows` to ``, so flow keys gain a `flows/` + * segment. That shift is what preserves the flow→baseline relative offset + * Maestro resolves against, and is already how `addMedia` behaves. */ export function computeCommonRoot( testFileNames: string[], referencedFiles: string[], + includedFiles: string[] = [], ): string { - const pathsShortestToLongest = [...testFileNames, ...referencedFiles].sort( - (a, b) => a.split(path.sep).length - b.split(path.sep).length, - ); + const pathsShortestToLongest = [ + ...testFileNames, + ...referencedFiles, + ...includedFiles, + ].sort((a, b) => a.split(path.sep).length - b.split(path.sep).length); if (pathsShortestToLongest.length === 0) return ''; const splitPaths = pathsShortestToLongest.map((p) => p.split(path.sep)); diff --git a/src/services/test-submission.service.ts b/src/services/test-submission.service.ts index 4a5fe78..5e79036 100644 --- a/src/services/test-submission.service.ts +++ b/src/services/test-submission.service.ts @@ -124,6 +124,7 @@ export class TestSubmissionService { flowMetadata, flowOverrides, flowsToRun: testFileNames, + includedFiles = [], referencedFiles, sequence, workspaceConfig, @@ -168,6 +169,22 @@ export class TestSubmissionService { } } + // Logged separately from referencedFiles: these come from config.yaml's + // `includedPaths` rather than from a flow command, and they can raise the + // common root (see computeCommonRoot) — which shows up here as every flow + // key gaining a leading directory segment. + if (includedFiles.length > 0) { + this.logDebug( + debug, + logger, + `[DEBUG] Uploading ${includedFiles.length} file(s) from \`includedPaths\`:`, + ); + for (const file of includedFiles) { + const normalizedPath = this.normalizeFilePath(file, commonRoot); + this.logDebug(debug, logger, `[DEBUG] - ${normalizedPath}`); + } + } + this.logDebug(debug, logger, `[DEBUG] Compressing files from path: ${flowFile}`); const plaintextZip = await compressFilesFromRelativePath( @@ -179,6 +196,7 @@ export class TestSubmissionService { ...referencedFiles, ...testFileNames, ...sequentialFlows, + ...includedFiles, ]), ], commonRoot, diff --git a/src/services/workspace-config.schema.ts b/src/services/workspace-config.schema.ts index 960cee9..71c2a29 100644 --- a/src/services/workspace-config.schema.ts +++ b/src/services/workspace-config.schema.ts @@ -47,6 +47,7 @@ export const WorkspaceConfigSchema = z.looseObject({ excludeTags: tagList.nullish(), executionOrder: ExecutionOrderSchema.nullish(), flows: z.array(z.string()).nullish(), + includedPaths: z.array(z.string()).nullish(), includeTags: tagList.nullish(), local: z .looseObject({ deterministicOrder: z.boolean().nullish() }) @@ -92,10 +93,15 @@ export const WORKSPACE_CONFIG_KEYS: ReadonlySet = new Set( * Near-misses that aren't just a casing slip on a real key. Keyed lowercase. */ const KEY_ALIASES: Record = { + assets: 'includedPaths', continueonfailure: 'executionOrder.continueOnFailure', excludetag: 'excludeTags', + files: 'includedPaths', floworder: 'executionOrder.flowsOrder', flowsorder: 'executionOrder.flowsOrder', + includedpath: 'includedPaths', + includefiles: 'includedPaths', + includepaths: 'includedPaths', includetag: 'includeTags', tags: 'includeTags / excludeTags', }; diff --git a/test/unit/included-paths.test.ts b/test/unit/included-paths.test.ts new file mode 100644 index 0000000..3f36e5c --- /dev/null +++ b/test/unit/included-paths.test.ts @@ -0,0 +1,249 @@ +import { expect } from 'chai'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +import { plan } from '../../src/services/execution-plan.service.js'; +import { + checkIfFilesExistInWorkspace, + isWorkspaceConfigFile, +} from '../../src/services/execution-plan.utils.js'; +import { computeCommonRoot } from '../../src/services/flow-paths.js'; +import { parseWorkspaceConfig } from '../../src/services/workspace-config.schema.js'; + +/** + * Fixtures are built on disk because `includedPaths` is glob-driven — a stubbed + * filesystem would test the stub, not `fs.globSync`'s actual matching. + * Paths are composed with `path.join` so the assertions hold on Windows too. + */ +function makeWorkspace( + files: Record, +): { cleanup: () => void; root: string } { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'dcd-included-')); + for (const [relativePath, contents] of Object.entries(files)) { + const absolute = path.join(root, relativePath); + fs.mkdirSync(path.dirname(absolute), { recursive: true }); + fs.writeFileSync(absolute, contents); + } + + return { cleanup: () => fs.rmSync(root, { recursive: true, force: true }), root }; +} + +const FLOW = ['appId: com.example', '---', '- launchApp'].join('\n'); + +describe('includedPaths', () => { + describe('schema', () => { + it('accepts includedPaths without warning', () => { + const warnings: string[] = []; + const config = parseWorkspaceConfig( + { includedPaths: ['screenshots/**'] }, + { filePath: 'config.yaml', warn: (m) => warnings.push(m) }, + ); + + expect(config.includedPaths).to.deep.equal(['screenshots/**']); + expect(warnings).to.deep.equal([]); + }); + + it('suggests includedPaths for near-miss keys', () => { + const warnings: string[] = []; + parseWorkspaceConfig( + { assets: ['screenshots/**'] }, + { filePath: 'config.yaml', warn: (m) => warnings.push(m) }, + ); + + expect(warnings.join('\n')).to.contain('did you mean includedPaths'); + }); + + it('counts as a workspace-config key for shape detection', () => { + const { cleanup, root } = makeWorkspace({ + 'config.yaml': 'includedPaths:\n - screenshots/**\n', + }); + + try { + expect(isWorkspaceConfigFile(path.join(root, 'config.yaml'))).to.equal( + true, + ); + } finally { + cleanup(); + } + }); + }); + + describe('resolution', () => { + it('bundles files no flow command references', async () => { + const { cleanup, root } = makeWorkspace({ + 'config.yaml': 'includedPaths:\n - screenshots/**\n', + 'screenshots/home.png': 'png-bytes', + 'visual.yaml': FLOW, + }); + + try { + const result = await plan({ input: root, warn: () => {} }); + expect(result.includedFiles).to.deep.equal([ + path.join(root, 'screenshots', 'home.png'), + ]); + // The baseline is NOT a flow-command reference — that is the whole + // point. referencedFiles holds only the flow its BFS was seeded with. + expect(result.referencedFiles).to.deep.equal([ + path.join(root, 'visual.yaml'), + ]); + } finally { + cleanup(); + } + }); + + it('skips directories that match the glob', async () => { + const { cleanup, root } = makeWorkspace({ + 'assets/nested/keep.png': 'png-bytes', + 'config.yaml': 'includedPaths:\n - assets/**\n', + 'visual.yaml': FLOW, + }); + + try { + const result = await plan({ input: root, warn: () => {} }); + expect(result.includedFiles).to.deep.equal([ + path.join(root, 'assets', 'nested', 'keep.png'), + ]); + } finally { + cleanup(); + } + }); + + it('warns when a pattern matches nothing', async () => { + const { cleanup, root } = makeWorkspace({ + 'config.yaml': 'includedPaths:\n - screenshots/**\n', + 'visual.yaml': FLOW, + }); + const warnings: string[] = []; + + try { + const result = await plan({ + input: root, + warn: (m) => warnings.push(m), + }); + expect(result.includedFiles).to.deep.equal([]); + expect(warnings.join('\n')).to.contain('matched no files'); + } finally { + cleanup(); + } + }); + + it('refuses a pattern that escapes the workspace', async () => { + const { cleanup, root } = makeWorkspace({ + 'config.yaml': 'includedPaths:\n - ../outside.png\n', + 'visual.yaml': FLOW, + }); + fs.writeFileSync(path.join(root, '..', 'outside.png'), 'png-bytes'); + + try { + await plan({ input: root, warn: () => {} }); + expect.fail('expected the containment guard to throw'); + } catch (error) { + expect((error as Error).message).to.contain( + 'resolves outside the workspace', + ); + } finally { + cleanup(); + } + }); + }); + + describe('computeCommonRoot', () => { + it('folds included files in so the zip can strip the prefix', () => { + const flows = [path.join('/a', 'b', 'flows', 'login.yaml')]; + const included = [path.join('/a', 'b', 'screenshots', 'home.png')]; + + // Without the included file the root sits at the flows dir; folding it in + // raises the root so the flow -> baseline relative offset survives the zip. + expect(computeCommonRoot(flows, [])).to.equal( + path.join('/a', 'b', 'flows'), + ); + expect(computeCommonRoot(flows, [], included)).to.equal( + path.join('/a', 'b'), + ); + }); + + it('defaults includedFiles so existing callers are unaffected', () => { + const flows = [path.join('/a', 'b', 'login.yaml')]; + expect(computeCommonRoot(flows, [])).to.equal(path.join('/a', 'b')); + }); + }); + + describe('assertScreenshot dependency walking', () => { + const flowPath = path.join('/ws', 'visual.yaml'); + + it('does not error on a missing baseline', () => { + const { errors, files } = checkIfFilesExistInWorkspace( + 'assertScreenshot', + 'screenshots/home.png', + flowPath, + ); + + // Maestro's own "searched in:" message is better than ours, and a first + // run legitimately has no baseline — so this must not abort the upload. + expect(errors).to.deep.equal([]); + expect(files).to.deep.equal([]); + }); + + it('still errors on a missing addMedia file', () => { + const { errors } = checkIfFilesExistInWorkspace( + 'addMedia', + 'screenshots/home.png', + flowPath, + ); + expect(errors).to.have.lengthOf(1); + }); + + it('bundles an existing baseline referenced by the object path key', () => { + const { cleanup, root } = makeWorkspace({ + 'screenshots/home.png': 'png-bytes', + 'visual.yaml': FLOW, + }); + + try { + const { errors, files } = checkIfFilesExistInWorkspace( + 'assertScreenshot', + { path: 'screenshots/home.png' }, + path.join(root, 'visual.yaml'), + ); + expect(errors).to.deep.equal([]); + expect(files).to.deep.equal([path.join(root, 'screenshots', 'home.png')]); + } finally { + cleanup(); + } + }); + + it('appends .png when the path has no image extension', () => { + const { cleanup, root } = makeWorkspace({ + 'screenshots/home.png': 'png-bytes', + 'visual.yaml': FLOW, + }); + + try { + // Mirrors Maestro's normalizeScreenshotPath: `assertScreenshot: home` + // resolves to home.png on the device, so it must here too. + const { files } = checkIfFilesExistInWorkspace( + 'assertScreenshot', + 'screenshots/home', + path.join(root, 'visual.yaml'), + ); + expect(files).to.deep.equal([path.join(root, 'screenshots', 'home.png')]); + } finally { + cleanup(); + } + }); + + it('skips an interpolated path rather than guessing', () => { + const { errors, files } = checkIfFilesExistInWorkspace( + 'assertScreenshot', + 'screenshots/${DCD_DEVICE}/home.png', + flowPath, + ); + + // Per-device baselines are what `includedPaths` is for; a static walk + // cannot resolve the variable. + expect(errors).to.deep.equal([]); + expect(files).to.deep.equal([]); + }); + }); +});