refactor(core,cli-app): decompose merged self-hosted Maestro relay + app:exec injection

.semgrepignore

@@ -20,16 +20,24 @@ packages/core/src/lock.js
 # every join. Suppress at the file level with this rationale.
 packages/core/src/archive.js
 
-# api.js path-traversal guards live at the top of the /percy/maestro-screenshot
-# route handler: `name` and `sessionId` are both validated against
-# /^[a-zA-Z0-9_-]+$/ (SAFE_ID, line ~322) BEFORE any path.join() runs.
-# Traversal sequences (`..`, `/`, `\0`) are rejected with 400 there. The
-# fallback walker is also depth-capped at 15 levels. semgrep's
-# path-join-resolve-traversal rule cannot follow the regex validation
-# chain across the function body, so it flags the joins on lines 445
-# and 462. Inline `// nosemgrep` directives (preceding and same-line)
-# were not honored by the semgrep CI version in use — suppress at the
-# file level with this rationale.
+# maestro-screenshot-file.js owns the screenshot-location path joins. The
+# handleMaestroScreenshot handler validates `name`/`sessionId` against
+# /^[a-zA-Z0-9_-]+$/ (SAFE_ID) and rejects traversal sequences with 400 BEFORE
+# calling locateScreenshot(); the fallback walker (manualScreenshotWalk) is
+# additionally depth-capped at 15 levels, and the realpath + scopeRoot prefix
+# check enforces containment. semgrep's path-join-resolve-traversal rule cannot
+# follow that validation chain across the module boundary, so it flags the joins
+# inside manualScreenshotWalk. Inline `// nosemgrep` is not honored by the CI
+# semgrep version — suppress at the file level with this rationale.
+packages/core/src/maestro-screenshot-file.js
+
+# api.js: createStaticServer builds an automatic sitemap by joining the
+# operator-provided `baseUrl` config with locally-globbed `*.html` filenames via
+# `path.posix.join('/', baseUrl, …)` (sitemap-URL construction, not a filesystem
+# read of external request input). semgrep's path-join-resolve-traversal rule
+# flags the join regardless of the trusted sources. Inline `// nosemgrep` is not
+# honored by the CI semgrep version — suppress at the file level with this
+# rationale.
 packages/core/src/api.js
 
 # Test files load fixtures from hardcoded subpaths under

packages/cli-app/src/exec.js

@@ -1,8 +1,6 @@
-import fs from 'fs';
-import os from 'os';
-import path from 'path';
 import command from '@percy/cli-command';
 import * as ExecPlugin from '@percy/cli-exec';
+import { maybeInjectMaestroServer, maybeInjectScreenshotDir, maybeInjectDriverHostPort } from './maestro-inject.js';
 
 export const ping = ExecPlugin.ping;
 export const stop = ExecPlugin.stop;
@@ -18,168 +16,6 @@ export const start = command('start', {
   }
 }, ExecPlugin.start.callback);
 
-// Locate the `test` subcommand in argv. Maestro accepts global parent
-// flags before the subcommand, e.g.:
-//   maestro test flow.yaml
-//   maestro --udid <serial> test flow.yaml
-//   maestro --platform=android test flow.yaml
-//   maestro --verbose --no-ansi test flow.yaml
-// We must find `test` by scanning rather than checking args[1] === 'test',
-// or our injects silently no-op when the customer pins a device.
-//
-// Returns the index of the `test` literal, or -1 if not present. Skips
-// over the value of known value-taking parent flags so a literal `test`
-// supplied as a flag value (e.g. `--udid test`) isn't mistaken for the
-// subcommand. Equals-form (`--flag=value`) doesn't need a skip — the
-// value is part of the same argv token.
-const MAESTRO_PARENT_VALUE_FLAGS = new Set([
-  '--udid', '--device', '-p', '--platform',
-  '--host', '--port', '--driver-host-port'
-]);
-function findTestSubcommandIdx(args) {
-  for (let i = 1; i < args.length; i++) {
-    const tok = args[i];
-    if (tok === 'test') return i;
-    if (typeof tok === 'string' && MAESTRO_PARENT_VALUE_FLAGS.has(tok)) {
-      i++; // skip the value of this flag
-    }
-  }
-  return -1;
-}
-
-function hasExistingPercyServerFlag(args, testIdx) {
-  for (let i = testIdx + 1; i < args.length - 1; i++) {
-    if (args[i] === '-e' && /^PERCY_SERVER=/.test(args[i + 1])) return true;
-  }
-  return false;
-}
-
-// Returns the customer-supplied value of `--test-output-dir` (whether the
-// space-form `--test-output-dir <path>` or the equals-form
-// `--test-output-dir=<path>` — both are valid picocli syntax), or null if
-// absent. Returning the value (not just an index) lets the screenshot-dir
-// helper align PERCY_MAESTRO_SCREENSHOT_DIR with the customer's value in
-// either form without re-deriving it.
-//
-// An empty value (space-form value === '' or equals-form `--test-output-dir=`
-// with nothing after the `=`) is treated as ABSENT — returning the empty
-// string would tell the caller "customer set this" and bypass the
-// auto-resolve fallback, leaving Maestro to default its output location
-// while PERCY_MAESTRO_SCREENSHOT_DIR stays unset. That mismatch silently
-// produces all-404 snapshots, so fall through to the auto-resolve path
-// instead.
-const TEST_OUTPUT_DIR_EQ_PREFIX = '--test-output-dir=';
-function findTestOutputDirValue(args, testIdx) {
-  for (let i = testIdx + 1; i < args.length; i++) {
-    const tok = args[i];
-    if (tok === '--test-output-dir' && i + 1 < args.length) {
-      const val = args[i + 1];
-      if (typeof val === 'string' && val.length > 0) return val;
-    } else if (typeof tok === 'string' && tok.startsWith(TEST_OUTPUT_DIR_EQ_PREFIX)) {
-      const val = tok.slice(TEST_OUTPUT_DIR_EQ_PREFIX.length);
-      if (val.length > 0) return val;
-    }
-  }
-  return null;
-}
-
-// Maestro's GraalJS sandbox does NOT inherit the parent process's env,
-// so `PERCY_SERVER_ADDRESS` exported by app:exec is invisible to the
-// SDK. When wrapping `maestro test`, surface the CLI address through
-// Maestro's only env channel — `-e KEY=VALUE` flags — so the SDK
-// healthcheck can find the local CLI without the customer having to
-// pair ports manually. No-op when the customer already supplied their
-// own `-e PERCY_SERVER=...`.
-//
-// When percy?.address() is falsy (percy disabled, start failed), emit a
-// WARN so the customer is not surprised by a silent zero-snapshot build.
-// The customer-override skip case (their own `-e PERCY_SERVER=...` is in
-// argv) does NOT warn — that's intentional flow control, not a problem.
-export function maybeInjectMaestroServer(ctx, log) {
-  const args = ctx?.argv;
-  if (!Array.isArray(args) || args.length < 2) return;
-  if (path.basename(args[0]) !== 'maestro') return;
-  const testIdx = findTestSubcommandIdx(args);
-  if (testIdx < 0) return;
-  if (hasExistingPercyServerFlag(args, testIdx)) return;
-  const addr = ctx.percy?.address();
-  if (!addr) {
-    log?.warn(
-      'app:exec did not start the Percy CLI server (percy disabled or start ' +
-      'failed); -e PERCY_SERVER not injected into maestro test. Snapshots will ' +
-      'NOT be uploaded. Set PERCY_TOKEN and re-run, or check the percy log above.'
-    );
-    return;
-  }
-  // Inject after `test` so `-e KEY=VAL` is bound to the `test` subcommand
-  // (the only Maestro subcommand that accepts `-e`).
-  args.splice(testIdx + 1, 0, '-e', `PERCY_SERVER=${addr}`);
-}
-
-// Auto-resolve the Maestro screenshot output directory so customers don't
-// have to pair `export PERCY_MAESTRO_SCREENSHOT_DIR=...` with a matching
-// `--test-output-dir <same>` in their maestro test command.
-//
-// Resolution order:
-//   1. Customer set BOTH process.env.PERCY_MAESTRO_SCREENSHOT_DIR and
-//      --test-output-dir in argv → trust them, do nothing.
-//   2. Customer set PERCY_MAESTRO_SCREENSHOT_DIR only → use it, inject
-//      `--test-output-dir <env value>` into argv.
-//   3. Customer set --test-output-dir only → use that value, mirror it
-//      into process.env.PERCY_MAESTRO_SCREENSHOT_DIR (so the SDK +
-//      CLI relay see the same path).
-//   4. Neither set → pick `${process.cwd()}/.percy-out`. On any mkdir
-//      failure (read-only CWD, EACCES, EEXIST as a file), fall back to
-//      `${os.tmpdir()}/percy-maestro-<pid>` with a WARN log.
-//
-// The env-var update and argv splice always keep both sources of truth
-// (SDK reads env var; Maestro reads the flag) aligned to the same path.
-export function maybeInjectScreenshotDir(ctx, log) {
-  const args = ctx?.argv;
-  if (!Array.isArray(args) || args.length < 2) return;
-  if (path.basename(args[0]) !== 'maestro') return;
-  const testIdx = findTestSubcommandIdx(args);
-  if (testIdx < 0) return;
-
-  const envSet = !!process.env.PERCY_MAESTRO_SCREENSHOT_DIR;
-  const existingFlagValue = findTestOutputDirValue(args, testIdx);
-  const flagSet = existingFlagValue !== null;
-
-  // Fully customer-controlled — nothing to do.
-  if (envSet && flagSet) return;
-
-  let resolved;
-  if (envSet) {
-    resolved = process.env.PERCY_MAESTRO_SCREENSHOT_DIR;
-  } else if (flagSet) {
-    resolved = existingFlagValue;
-  } else {
-    const preferred = path.join(process.cwd(), '.percy-out');
-    try {
-      fs.mkdirSync(preferred, { recursive: true });
-      resolved = preferred;
-    } catch (err) {
-      const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`);
-      try {
-        fs.mkdirSync(fallback, { recursive: true });
-      } catch (_) {
-        // tmpdir mkdir failure is exceedingly rare; fall through and let
-        // downstream code surface a clearer error than this helper can.
-      }
-      resolved = fallback;
-      log?.warn(
-        `Could not create ${preferred} (${err.code || err.message}); ` +
-        `falling back to ${fallback}. Set PERCY_MAESTRO_SCREENSHOT_DIR to ` +
-        'pick a specific location.'
-      );
-    }
-  }
-
-  if (!envSet) process.env.PERCY_MAESTRO_SCREENSHOT_DIR = resolved;
-  // Inject right after `test` (the subcommand that owns `--test-output-dir`).
-  if (!flagSet) args.splice(testIdx + 1, 0, '--test-output-dir', resolved);
-}
-
 export const exec = command('exec', {
   description: 'Start and stop Percy around a supplied command for native apps',
   usage: '[options] -- <command>',
@@ -195,16 +31,21 @@ export const exec = command('exec', {
     skipDiscovery: true
   }
 }, async function*(ctx) {
-  // The two helpers splice their flag groups at argv index 2 (between `test`
-  // and the flow file) because `-e` and `--test-output-dir` are
-  // `test`-subcommand options. Resulting argv for `maestro test flow.yaml`:
-  //   maestro test --test-output-dir <dir> -e PERCY_SERVER=<url> flow.yaml
-  // iOS driver port: not prescribed from this side — the @percy/core relay
-  // reads `PERCY_IOS_DRIVER_HOST_PORT` (BS-host-injected on production
-  // hosts) and probes the documented Maestro 2.4.0 single-simulator default
-  // (`127.0.0.1:7001`) when it isn't set. See `packages/core/src/maestro-hierarchy.js`.
+  // The helpers splice their flag groups right after `test` because `-e` and
+  // `--test-output-dir` are `test`-subcommand options; on iOS + Maestro >= 2.6 a
+  // third adds `--driver-host-port <port>`. Resulting argv for
+  // `maestro --platform=ios test flow.yaml`:
+  //   maestro --platform=ios test --driver-host-port <port> --test-output-dir <dir> -e PERCY_SERVER=<url> flow.yaml
+  // iOS driver port: on Maestro >= 2.6 (ephemeral driver port) we prescribe a
+  // free port via `--driver-host-port` and mirror it to PERCY_IOS_DRIVER_HOST_PORT
+  // so the @percy/core relay reaches `/viewHierarchy` deterministically for
+  // element regions and device insets. On Maestro <= 2.4 the helper no-ops and
+  // the relay's `127.0.0.1:7001` probe (the deterministic 2.4.0 default) serves
+  // those customers. On BrowserStack the port is host-injected and this path
+  // never runs. See `packages/core/src/maestro-hierarchy.js`.
   maybeInjectMaestroServer(ctx, ctx.log);
   maybeInjectScreenshotDir(ctx, ctx.log);
+  await maybeInjectDriverHostPort(ctx, ctx.log);
   yield* ExecPlugin.default.callback(ctx);
 });
 

packages/cli-app/src/index.js

@@ -1,2 +1,3 @@
 export { default, app } from './app.js';
-export { exec, start, stop, ping, maybeInjectMaestroServer, maybeInjectScreenshotDir } from './exec.js';
+export { exec, start, stop, ping } from './exec.js';
+export { maybeInjectMaestroServer, maybeInjectScreenshotDir, maybeInjectDriverHostPort } from './maestro-inject.js';