From 613d8c95d6e409de79df5249084d87416ddca0e8 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Thu, 28 May 2026 12:57:17 +0530 Subject: [PATCH 01/22] =?UTF-8?q?feat(core):=20self-hosted=20Maestro=20rel?= =?UTF-8?q?ay=20=E2=80=94=20sessionId=20optional=20+=20PERCY=5FMAESTRO=5FS?= =?UTF-8?q?CREENSHOT=5FDIR=20scope?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/percy/maestro-screenshot` now supports non-BrowserStack ("self-hosted") Maestro runs by making `sessionId` optional. When the request omits it (the BrowserStack host's exclusive marker), the relay enters self-hosted mode and resolves the file-find scope root from a new required env var: - PERCY_MAESTRO_SCREENSHOT_DIR — process.env only, never request body. Must be an absolute, existing directory (typically the customer's `maestro test --test-output-dir ` path). Missing/invalid → 400 with actionable guidance, emitted before realpath so it isn't masked as a 404. Self-hosted file-find uses a recursive `/**/.png` glob and goes through the same realpath + trailing-separator prefix check as the BS path, just rooted at the env-supplied dir. The security boundary is relocated, not removed: - filePath is rejected outright self-hosted (SDK never emits it; an absolute filePath against a caller-influenceable root would re-open arbitrary in-root reads). - SAFE_ID on `name` stays load-bearing for the recursive glob. - Trailing-separator guard preserved (prevents /x/.maestro vs /x/.maestro-secrets bypass at the relocated root). BS path (sessionId present) is byte-identical — the existing `/tmp/{sessionId}{_test_suite}` glob, walker fallback, and realpath check all run exactly as before. R7 from the plan. Tests: BS-path tests pass unchanged. New self-hosted describe locks the new branches: happy path (recursive find, payload omits sessionId), env var validation (unset/relative/non-existent/file-not-dir → 400), filePath rejected, file missing → 404, name traversal rejected, coordinate region pass-through. Plan: docs/plans/2026-05-27-001-feat-self-hosted-maestro-percy-plan.md (Unit 1 of 4; iOS port resolver + SDK + docs follow). --- packages/core/src/api.js | 80 +++++++++++++++++---- packages/core/test/api.test.js | 125 ++++++++++++++++++++++++++++++++- 2 files changed, 187 insertions(+), 18 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 6a4ec27e6..f8a5df8f6 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -425,15 +425,19 @@ export function createPercyServer(percy, port) { let { name, sessionId } = req.body || {}; if (!name) throw new ServerError(400, 'Missing required field: name'); - if (!sessionId) throw new ServerError(400, 'Missing required field: sessionId'); + // `sessionId` is host-injected on BrowserStack; its absence signals + // self-hosted mode (gated separately on PERCY_MAESTRO_SCREENSHOT_DIR + // below). When present, the BS path runs byte-identical. + let selfHosted = !sessionId; // Strict character-class validation — rejects path separators, shell metacharacters, // NUL, newlines, and anything else that could confuse the glob or the filesystem. + // `name` is load-bearing for the recursive glob — must not be loosened. const SAFE_ID = /^[a-zA-Z0-9_-]+$/; if (!SAFE_ID.test(name)) { throw new ServerError(400, 'Invalid screenshot name'); } - if (!SAFE_ID.test(sessionId)) { + if (sessionId && !SAFE_ID.test(sessionId)) { throw new ServerError(400, 'Invalid sessionId'); } @@ -473,6 +477,42 @@ export function createPercyServer(percy, port) { suppliedFilePath = req.body.filePath; } + // Resolve the file-find scope root. On BrowserStack (sessionId present), + // the root is the BS host's /tmp/{sessionId}{_test_suite} convention. + // Self-hosted (sessionId absent) requires PERCY_MAESTRO_SCREENSHOT_DIR + // (read from process.env, never the request body) to be an absolute, + // existing directory — typically the customer's + // `maestro test --test-output-dir ` path. The realpath + prefix + // check below enforces the security invariant at whichever root applies; + // the boundary is relocated, not removed. + let scopeRoot; + if (selfHosted) { + // Reject filePath outright in self-hosted mode. The SDK never emits + // it (it sends a relative SCREENSHOT_NAME); honoring an absolute + // filePath against a caller-influenceable root would re-open + // arbitrary in-root reads. + if (suppliedFilePath) { + throw new ServerError(400, 'filePath is not accepted in self-hosted mode (omit it; PERCY_MAESTRO_SCREENSHOT_DIR + relative SCREENSHOT_NAME is the supported path)'); + } + let dir = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + if (!dir) { + throw new ServerError(400, 'Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR (set it to your `maestro test --test-output-dir` path)'); + } + if (!path.isAbsolute(dir)) { + throw new ServerError(400, 'PERCY_MAESTRO_SCREENSHOT_DIR must be an absolute path'); + } + let stat; + try { stat = await fs.promises.stat(dir); } catch { stat = null; } + if (!stat || !stat.isDirectory()) { + throw new ServerError(400, `PERCY_MAESTRO_SCREENSHOT_DIR is not an existing directory: ${dir}`); + } + scopeRoot = dir; + } else { + scopeRoot = platform === 'ios' + ? `/tmp/${sessionId}` + : `/tmp/${sessionId}_test_suite`; + } + // Validate regions input shape early (before file I/O and ADB work) so // malformed requests don't consume resolver/relay work. Three parallel // input arrays share the same per-item shape; algorithm semantics differ @@ -529,9 +569,18 @@ export function createPercyServer(percy, port) { // {device}_maestro_debug_ root. The `**` recursive match handles any depth. // Exact {name}.png match at the leaf filters out Maestro's emoji-prefixed // debug frames (e.g., `screenshot-❌--(flow).png`). - let searchPattern = platform === 'ios' - ? `/tmp/${sessionId}/*_maestro_debug_*/**/${name}.png` - : `/tmp/${sessionId}_test_suite/logs/*/screenshots/${name}.png`; + let searchPattern; + if (selfHosted) { + // Self-hosted: recursive glob under the customer's --test-output-dir + // (PERCY_MAESTRO_SCREENSHOT_DIR). Recursive depth handles arbitrary + // Maestro layouts; `name` is SAFE_ID-validated above so it cannot + // contain separators or traversal characters. + searchPattern = `${scopeRoot}/**/${name}.png`; + } else { + searchPattern = platform === 'ios' + ? `/tmp/${sessionId}/*_maestro_debug_*/**/${name}.png` + : `/tmp/${sessionId}_test_suite/logs/*/screenshots/${name}.png`; + } let files; try { @@ -541,9 +590,12 @@ export function createPercyServer(percy, port) { // Fast-glob import / glob call failed — fall back to manual walker. // See manualScreenshotWalk() at file top for the rationale + the // file-level .semgrepignore covering path-traversal sinks inside. + // Self-hosted has no walker fallback (no fixed-layout convention) — + // empty files → 404 with the actionable PERCY_MAESTRO_SCREENSHOT_DIR + // guidance above. /* istanbul ignore next — only fires when fast-glob import throws (broken install / FS corruption); integration-test territory. */ - files = await manualScreenshotWalk(platform, sessionId, name); + files = selfHosted ? [] : await manualScreenshotWalk(platform, sessionId, name); } if (!files || files.length === 0) { @@ -568,22 +620,20 @@ export function createPercyServer(percy, port) { } } - // Canonicalize and confirm the resolved path still lives under the sessionId-owned dir. - // Defeats symlink swaps where a sessionId-named dir points elsewhere. - // We resolve both the file and the expected prefix because /tmp is a symlink on macOS - // (iOS hosts run macOS, where /tmp → /private/tmp). - let expectedSessionRoot = platform === 'ios' - ? `/tmp/${sessionId}` - : `/tmp/${sessionId}_test_suite`; + // Canonicalize and confirm the resolved path still lives under scopeRoot. + // Defeats symlink swaps where the root points elsewhere. Both ends are + // realpath'd because /tmp is a symlink on macOS (where iOS hosts run). + // The trailing `/` on the prefix is load-bearing — it prevents + // sibling-prefix bypass (e.g. /x/.maestro vs /x/.maestro-secrets). let realPath, realPrefix; try { realPath = await fs.promises.realpath(chosenFile); - realPrefix = await fs.promises.realpath(expectedSessionRoot); + realPrefix = await fs.promises.realpath(scopeRoot); } catch { throw new ServerError(404, `Screenshot not found: ${name}.png (path resolution failed)`); } if (!realPath.startsWith(`${realPrefix}/`)) { - throw new ServerError(404, `Screenshot not found: ${name}.png (resolved outside session dir)`); + throw new ServerError(404, `Screenshot not found: ${name}.png (resolved outside ${selfHosted ? 'PERCY_MAESTRO_SCREENSHOT_DIR' : 'session dir'})`); } // Read and base64-encode the screenshot diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index e22a4c2c7..e432f9d2c 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -1214,9 +1214,21 @@ describe('API Server', () => { await expectAsync(postMaestro({ sessionId: SID })).toBeRejectedWithError(/Missing required field: name/); }); - it('rejects missing sessionId with 400', async () => { - await percy.start(); - await expectAsync(postMaestro({ name: SS_NAME })).toBeRejectedWithError(/Missing required field: sessionId/); + it('400s missing sessionId + missing PERCY_MAESTRO_SCREENSHOT_DIR (self-hosted mode requires the env var)', async () => { + // `sessionId` absent is the self-hosted detection signal. Without + // PERCY_MAESTRO_SCREENSHOT_DIR set, the relay 400s with actionable + // guidance rather than 404'ing on a glob it cannot scope. The + // self-hosted happy path is covered in the dedicated describe below. + let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + try { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME })) + .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); + } finally { + if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; + } }); it('rejects invalid platform with 400', async () => { @@ -1851,6 +1863,113 @@ describe('API Server', () => { expect(payload.tag.width).toBe(1179); expect(payload.tag.height).toBe(2556); }); + + // ───────────────────────────────────────────────────────────────────── + // Self-hosted mode: `sessionId` absent triggers PERCY_MAESTRO_SCREENSHOT_DIR + // resolution. The BS path (sessionId present) is byte-identical and + // covered above; these tests lock the new branches. + // ───────────────────────────────────────────────────────────────────── + describe('self-hosted (sessionId absent)', () => { + const SELF_HOSTED_ROOT = '/tmp/percy-self-hosted-root'; + const NESTED_SUBDIR = `${SELF_HOSTED_ROOT}/.maestro/run-x/screenshots`; + const SELF_HOSTED_NAME = 'SelfHostedScreen'; + let priorEnv; + + beforeEach(() => { + priorEnv = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = SELF_HOSTED_ROOT; + fs.mkdirSync(NESTED_SUBDIR, { recursive: true }); + // Valid 24-byte PNG header (1080 x 2400) exercises PNG-fill on the + // self-hosted path too. + const pngHeader = Buffer.alloc(24); + Buffer.from([0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]).copy(pngHeader, 0); + pngHeader.writeUInt32BE(13, 8); + Buffer.from('IHDR', 'ascii').copy(pngHeader, 12); + pngHeader.writeUInt32BE(1080, 16); + pngHeader.writeUInt32BE(2400, 20); + fs.writeFileSync(`${NESTED_SUBDIR}/${SELF_HOSTED_NAME}.png`, pngHeader); + }); + + afterEach(() => { + if (priorEnv === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = priorEnv; + }); + + it('finds screenshot via recursive glob under PERCY_MAESTRO_SCREENSHOT_DIR and uploads without sessionId', async () => { + await percy.start(); + spyOn(percy, 'upload').and.resolveTo(); + let res = await postMaestro({ name: SELF_HOSTED_NAME, platform: 'android' }); + expect(res.success).toBe(true); + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.name).toBe(SELF_HOSTED_NAME); + expect(payload.tag.width).toBe(1080); + expect(payload.tag.height).toBe(2400); + expect(payload.tiles[0].content).toBeDefined(); + // sessionId is never forwarded into the upload payload (relay only + // used it for scoping; self-hosted has no equivalent). + expect(payload.sessionId).toBeUndefined(); + }); + + it('400s when PERCY_MAESTRO_SCREENSHOT_DIR is not absolute', async () => { + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = 'relative/path'; + await percy.start(); + await expectAsync(postMaestro({ name: SELF_HOSTED_NAME })) + .toBeRejectedWithError(/PERCY_MAESTRO_SCREENSHOT_DIR must be an absolute path/); + }); + + it('400s when PERCY_MAESTRO_SCREENSHOT_DIR does not exist', async () => { + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = '/tmp/this-path-does-not-exist-percy-self-hosted'; + await percy.start(); + await expectAsync(postMaestro({ name: SELF_HOSTED_NAME })) + .toBeRejectedWithError(/PERCY_MAESTRO_SCREENSHOT_DIR is not an existing directory/); + }); + + it('400s when PERCY_MAESTRO_SCREENSHOT_DIR points to a file, not a directory', async () => { + fs.writeFileSync('/tmp/percy-self-hosted-not-a-dir', 'plain-file'); + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = '/tmp/percy-self-hosted-not-a-dir'; + await percy.start(); + await expectAsync(postMaestro({ name: SELF_HOSTED_NAME })) + .toBeRejectedWithError(/PERCY_MAESTRO_SCREENSHOT_DIR is not an existing directory/); + }); + + it('rejects a supplied filePath in self-hosted mode (security invariant)', async () => { + // The SDK never emits filePath self-hosted; honoring it against a + // caller-influenceable root would re-open arbitrary in-root reads. + await percy.start(); + await expectAsync(postMaestro({ + name: SELF_HOSTED_NAME, + filePath: `${NESTED_SUBDIR}/${SELF_HOSTED_NAME}.png` + })).toBeRejectedWithError(/filePath is not accepted in self-hosted mode/); + }); + + it('404s when the screenshot is missing under the configured root', async () => { + await percy.start(); + await expectAsync(postMaestro({ name: 'NoSuchScreenshot' })) + .toBeRejectedWithError(/Screenshot not found/); + }); + + it('rejects name with traversal characters (SAFE_ID is load-bearing for the recursive glob)', async () => { + await percy.start(); + await expectAsync(postMaestro({ name: '../etc/passwd' })) + .toBeRejectedWithError(/Invalid screenshot name/); + }); + + it('coordinate regions still pass through on the self-hosted path', async () => { + await percy.start(); + spyOn(percy, 'upload').and.resolveTo(); + await postMaestro({ + name: SELF_HOSTED_NAME, + platform: 'android', + regions: [{ top: 10, bottom: 50, left: 0, right: 100, algorithm: 'ignore' }] + }); + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.regions).toBeDefined(); + expect(payload.regions.length).toBe(1); + expect(payload.regions[0].elementSelector.boundingBox) + .toEqual({ x: 0, y: 10, width: 100, height: 40 }); + expect(payload.regions[0].algorithm).toBe('ignore'); + }); + }); }); }); From 55574ba2f4c954aa1de2b5c609ef77770589866a Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Thu, 28 May 2026 13:25:42 +0530 Subject: [PATCH 02/22] =?UTF-8?q?fix(core):=20self-hosted=20glob=20?= =?UTF-8?q?=E2=80=94=20enable=20`dot:=20true`=20for=20Maestro=20`.maestro/?= =?UTF-8?q?`=20default?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maestro's default output directory is `.maestro/` (dot-prefixed). When a self-hosted customer runs `maestro test` without `--test-output-dir`, screenshots land under `/.maestro/...`. fast-glob's default is `dot: false`, so the recursive `/**/.png` glob silently skipped dot-prefixed segments and 404'd the file. CI surfaced this on two self-hosted tests (happy path + coordinate regions pass-through), whose fixtures correctly mirror Maestro's `.maestro/run-x/screenshots/` real-world layout. `dot: true` is applied only on the self-hosted glob; the BS glob is unchanged (BS layouts have no dot-prefixed segments — byte-identical R7). --- packages/core/src/api.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index f8a5df8f6..88075afe7 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -585,7 +585,11 @@ export function createPercyServer(percy, port) { let files; try { let { default: glob } = await import('fast-glob'); - files = await glob(searchPattern); + // Self-hosted needs `dot: true` because Maestro's default output + // directory is `.maestro/` — a dot-prefixed entry that fast-glob + // hides by default. BS layouts have no dot-prefixed segments, so + // omitting the option there keeps the byte-identical behavior. + files = await glob(searchPattern, selfHosted ? { dot: true } : undefined); } catch { // Fast-glob import / glob call failed — fall back to manual walker. // See manualScreenshotWalk() at file top for the rationale + the From eeec0a8689c4b5e066fe4cf2c966f4a85f441a41 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Thu, 28 May 2026 14:15:53 +0530 Subject: [PATCH 03/22] =?UTF-8?q?feat(core):=20iOS=20self-hosted=20element?= =?UTF-8?q?-region=20resolver=20=E2=80=94=20probe=207001=20+=20lsof=20+=20?= =?UTF-8?q?warn-skip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds non-BrowserStack support to the iOS element-region resolver. Before this change, `dump({ platform: 'ios' })` required BOTH `PERCY_IOS_DEVICE_UDID` AND `PERCY_IOS_DRIVER_HOST_PORT` to be set (host-injected by realmobile) — if either was absent (the normal self-hosted state) the resolver bailed with `unavailable/env-missing` and element regions silently dropped. The dispatch now splits into two branches by `PERCY_IOS_DRIVER_HOST_PORT` presence: EXPLICIT (port set) — BS and customer-supplied-port self-hosted both take this path. Existing HTTP-primary → CLI-fallback cascade runs unchanged when UDID is also set. New: when UDID is absent (a self-hosted-with-explicit-port scenario), HTTP is the only path; on HTTP failure the resolver warn-skips with the new `self-hosted-no-udid` reason rather than running maestro CLI without a target. `parseIos DriverHostPort` is relaxed from the BS-specific 11100-11110 range to any valid TCP port (1-65535) — BS values remain a strict subset. IMPLICIT (port unset) — self-hosted discovery cascade, all HTTP-based: 1. Cache hit (iosPortCache, per-Percy-instance, mirroring grpcClientCache). 2. Probe 127.0.0.1:7001 — source-verified deterministic single- simulator port on Maestro ≤2.4.0 (cli-2.4.0 TestCommand.kt#selectPort). 3. `lsof -nP -iTCP -sTCP:LISTEN | grep maestro-driver-ios…xctrunner` — exactly-one-match guard (zero or multiple → warn-skip, no guess). 4. Warn-skip with actionable log (`self-hosted-no-driver` reason). "Probe" reuses runIosHttpDump as both liveness check AND dump — a `hierarchy` or `no-aut-tree` result confirms the port is a valid Maestro driver (the existing axElement-root schema check rejects wrong services); `dump-error`/`connection-fail` advances to the next candidate (no drift-bit flip in cascade — drift is reserved for the explicit-port BS path). No cold-start `maestro hierarchy` tier — cut per plan after the spike verified 7001 is deterministic on current GA Maestro and lsof covers the future ephemeral-port case (Maestro 2.6+ `ServerSocket(0)`). BS path R7: the EXPLICIT branch with both UDID + valid PORT runs byte-identical to today. Existing iOS-HTTP, schema-drift, kill-switch, and healthcheck-drift tests pass unchanged. Three previously-expected `env-missing` tests are updated to the new branching: - PORT set + UDID unset → enters EXPLICIT, HTTP fails → `self-hosted-no-udid`. - PORT unset (regardless of UDID) → enters IMPLICIT, cascade → `self-hosted-no-driver` on no resolution. New describe `iOS self-hosted port cascade` (7 tests) covers: probe-7001 hit + caching, lsof single-match discovery, lsof zero/multi- match warn-skip, explicit out-of-legacy-range port (e.g., 6001 for real device) bypassing the cascade, wrong-service response (no axElement) falling through without caching, and cache-hit reuse on subsequent snapshots. Files: - packages/core/src/maestro-hierarchy.js: new constants (IOS_SELF_HOSTED_PROBE_PORT), three new helpers (execLsofDefault, lsofXctrunnerPort, resolveSelfHostedIosPort), iOS dispatch refactor, dump() signature extended with execLsof + iosPortCache injectables. - packages/core/src/api.js: thread `iosPortCache: percy.iosPortCache` through the maestroDump call (one line; mirrors grpcClientCache). - packages/core/src/percy.js: initialize `this.iosPortCache = { port: null }` next to grpcClientCache (per-Percy-instance scope, D9 of the maestro 4-PR plan). - packages/core/test/unit/maestro-hierarchy.test.js: 3 existing tests updated for the new branching, 7 new cascade tests added. Plan: docs/plans/2026-05-27-001-feat-self-hosted-maestro-percy-plan.md (Unit 2 of 4; Unit 1 already in ef66ccc0/8ac60b87; companion SDK + docs in percy/percy-maestro-app#7). --- packages/core/src/api.js | 6 +- packages/core/src/maestro-hierarchy.js | 187 +++++++++++++-- packages/core/src/percy.js | 6 + .../core/test/unit/maestro-hierarchy.test.js | 218 +++++++++++++++++- 4 files changed, 392 insertions(+), 25 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 88075afe7..21652835b 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -770,10 +770,14 @@ export function createPercyServer(percy, port) { // Thread the per-Percy gRPC client cache so the Android gRPC // primary path can reuse channels across snapshots in the same // session (D9 of 2026-05-07-002 plan). iOS path ignores it. + // Also thread iosPortCache so the self-hosted iOS port cascade + // (probe 7001 + lsof) resolves once per session and reuses the + // port for subsequent snapshots — same per-Percy scope. cachedDump = await maestroDump({ platform, sessionId, - grpcClientCache: percy.grpcClientCache + grpcClientCache: percy.grpcClientCache, + iosPortCache: percy.iosPortCache }); } /* istanbul ignore else — branch where dump resolves to hierarchy is diff --git a/packages/core/src/maestro-hierarchy.js b/packages/core/src/maestro-hierarchy.js index b7a8ae35a..3cda11fc9 100644 --- a/packages/core/src/maestro-hierarchy.js +++ b/packages/core/src/maestro-hierarchy.js @@ -101,10 +101,16 @@ const SELECTOR_KEYS_UNION = ['resource-id', 'text', 'content-desc', 'class', 'id // past the socket timeout. const IOS_HTTP_HEALTHY_DEADLINE_MS = 1500; const IOS_HTTP_CIRCUIT_BREAKER_MS = 5000; -// Maestro iOS driver-host-port is realmobile-derived as wda_port + 2700. -// WDA ports are 8400-8410 → driver host ports are 11100-11110. -const IOS_DRIVER_HOST_PORT_MIN = 11100; -const IOS_DRIVER_HOST_PORT_MAX = 11110; +// BS realmobile derives the iOS driver-host-port as wda_port + 2700 (WDA +// ports 8400-8410 → driver host ports 11100-11110). The validator +// `parseIosDriverHostPort` accepts any valid TCP port (1-65535); the BS +// range is a strict subset. Self-hosted iOS port resolution probes +// `127.0.0.1:7001` first — source-verified on Maestro 1.40-2.4.0 (cli-2.4.0 +// `TestCommand.kt#selectPort`) as the deterministic single-simulator port; +// sharded runs use the fixed range 7001-7128. Ephemeral-port Maestro +// (`main`/2.6.0+ uses `ServerSocket(0)`) is handled by `lsof` +// self-discovery rather than range-probing. +const IOS_SELF_HOSTED_PROBE_PORT = 7001; // HTTP response cap before parse — sized for WebView-heavy iOS apps. const IOS_HTTP_RESPONSE_MAX_BYTES = 20 * 1024 * 1024; @@ -902,17 +908,23 @@ function defaultHttpRequest({ host, port, method, path: requestPath, headers, bo }); } -// Validate PERCY_IOS_DRIVER_HOST_PORT env value as integer in the realmobile -// range (wda_port + 2700 = 11100-11110). Out-of-range values return null. +// Validate PERCY_IOS_DRIVER_HOST_PORT env value as an integer in the valid +// TCP range (1-65535). Was previously clamped to the BS-specific realmobile +// range (wda_port+2700 = 11100-11110); the relaxed range is a superset so BS +// continues to accept its canonical port unchanged. Self-hosted callers can +// pass any port they pinned via `maestro test --driver-host-port

` (e.g. +// the deterministic `7001` on a simulator, or a forwarded port like ~6001 on +// a real device). function parseIosDriverHostPort(raw) { /* istanbul ignore if — undefined/null/empty raw value branch; iOS dispatch pre-checks PERCY_IOS_DRIVER_HOST_PORT before calling, so these never fire. */ if (raw === undefined || raw === null || raw === '') return null; const n = Number(raw); /* istanbul ignore if — non-integer port (e.g. NaN from non-numeric env); - env var is set by realmobile as the canonical wda_port+2700 integer. */ + env var is set by realmobile as the canonical wda_port+2700 integer or by + a self-hosted customer as their explicit `--driver-host-port` value. */ if (!Number.isInteger(n)) return null; - if (n < IOS_DRIVER_HOST_PORT_MIN || n > IOS_DRIVER_HOST_PORT_MAX) return null; + if (n < 1 || n > 65535) return null; return n; } @@ -1238,11 +1250,111 @@ async function runAdbFallback(serial, execAdb) { return result; } +// ===== Self-hosted iOS port resolution helpers ===== +// Used only on the implicit branch of the iOS dispatch — when +// PERCY_IOS_DRIVER_HOST_PORT is absent, the resolver auto-discovers the +// running Maestro iOS driver's host port via deterministic probe + lsof. +// The BS path (explicit env vars present) does not invoke these. + +// Production default for `execLsof`. Tests inject a fake. The 5s timeout +// is generous — lsof is local and listing TCP LISTEN sockets is fast; the +// budget is mostly defense against a hung lsof. Returns the spawn result +// in the shape spawnWithTimeout produces. +/* istanbul ignore next — child-process spawn wrapper; tests inject a fake. */ +async function execLsofDefault({ timeoutMs } = {}) { + return spawnWithTimeout('lsof', ['-nP', '-iTCP', '-sTCP:LISTEN'], { timeoutMs: timeoutMs ?? 5000 }); +} + +// Parse `lsof -nP -iTCP -sTCP:LISTEN` output and return the LISTEN port of +// the Maestro iOS XCTest runner — ONLY when exactly one matching listener +// is found. Zero matches, multiple matches, or any spawn/parse failure +// returns null so the caller falls through to warn-skip rather than +// guessing. Match criterion: process name (column 1, case-insensitive +// substring) contains `maestro-driver-ios` (covers names like +// `dev.mobile.maestro-driver-iosUITests.xctrunner` plus future variants). +async function lsofXctrunnerPort(execLsof) { + let result; + try { result = await execLsof({ timeoutMs: 5000 }); } catch { return null; } + if (!result || result.spawnError || result.timedOut || (result.exitCode ?? 1) !== 0) return null; + const lines = (result.stdout || '').split('\n'); + const matches = new Set(); + for (const line of lines) { + if (!line) continue; + const cols = line.split(/\s+/); + // lsof default columns: COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME... + if (cols.length < 9) continue; + if (!/maestro-driver-ios/i.test(cols[0])) continue; + // NAME column for LISTEN sockets looks like `*:7001` or `127.0.0.1:7001`, + // sometimes suffixed with `(LISTEN)`. Pull the last `:` group. + const name = cols.slice(8).join(' '); + const m = name.match(/:(\d+)(?=\D|$)/g); + if (!m || m.length === 0) continue; + const last = m[m.length - 1]; + const port = Number(last.slice(1)); + if (!Number.isInteger(port) || port < 1 || port > 65535) continue; + matches.add(port); + if (matches.size > 1) return null; // multi-match → don't guess + } + return matches.size === 1 ? [...matches][0] : null; +} + +// Self-hosted iOS port resolution cascade. Returns either +// `{ port, result }` where `result` is a `runIosHttpDump` outcome +// (caller decides what to do with it), or `null` (caller emits +// warn-skip). Order: +// 1. Cache hit → re-probe the cached port (subsequent snapshots). +// 2. Probe `127.0.0.1:7001` (deterministic single-simulator port on +// Maestro ≤2.4.0 per the source-verified spike). +// 3. `lsof` self-discovery for ephemeral-port Maestro (2.6+ uses +// `ServerSocket(0)`); exactly-one-match guard. +// 4. null → warn-skip. +// +// "Probe" reuses runIosHttpDump as both the liveness check AND the dump. +// A `hierarchy` or `no-aut-tree` result confirms the port is a valid +// Maestro driver (`no-aut-tree` means the driver is alive but the AUT +// isn't foregrounded — still a Maestro listener worth caching). +// `dump-error` and `connection-fail` are NOT cached and move to the +// next candidate; in the cascade we don't propagate schema-drift since +// the wrong service answering on `7001` would also produce `dump-error`. +async function resolveSelfHostedIosPort({ httpRequest, execLsof, sessionId, iosPortCache }) { + const probe = async port => { + const result = await runIosHttpDump({ port, sessionId, httpRequest }); + if (result.kind === 'hierarchy' || result.kind === 'no-aut-tree') { + if (iosPortCache) iosPortCache.port = port; + return { port, result }; + } + return null; + }; + + // 1. Cache hit. + if (iosPortCache && Number.isInteger(iosPortCache.port)) { + const cached = iosPortCache.port; + const result = await runIosHttpDump({ port: cached, sessionId, httpRequest }); + return { port: cached, result }; + } + + // 2. Deterministic primary. + const primary = await probe(IOS_SELF_HOSTED_PROBE_PORT); + if (primary) return primary; + + // 3. lsof self-discovery — only try its port if it's distinct from the + // primary we already probed (otherwise the earlier probe failure + // already settled it). + const lsofPort = await lsofXctrunnerPort(execLsof); + if (lsofPort !== null && lsofPort !== IOS_SELF_HOSTED_PROBE_PORT) { + const hit = await probe(lsofPort); + if (hit) return hit; + } + + // 4. Nothing resolved. + return null; +} + export async function dump(options) { /* istanbul ignore next — options-omitted default; callers always pass an object (tests inject every dependency; production code binds them). */ options = options || {}; - let { platform, sessionId, execAdb, execMaestro, httpRequest, grpcClient, grpcClientCache, getEnv } = options; + let { platform, sessionId, execAdb, execMaestro, httpRequest, grpcClient, grpcClientCache, getEnv, execLsof, iosPortCache } = options; /* istanbul ignore next — defaults applied only when caller omits the corresponding key; tests inject every dependency, production callers bind these from defaults at runtime. */ @@ -1257,18 +1369,52 @@ export async function dump(options) { grpcClient = grpcClient || defaultGrpcClientFactory; /* istanbul ignore next */ getEnv = getEnv || defaultGetEnv; + /* istanbul ignore next — execLsof default for the self-hosted iOS + cascade; tests inject a fake. iosPortCache has no default — undefined + is the "no cache" sentinel and the cascade handles it. */ + execLsof = execLsof || execLsofDefault; const started = Date.now(); if (platform === 'ios') { - // iOS dispatch: read realmobile-injected env vars; warn-skip if absent. const udid = getEnv('PERCY_IOS_DEVICE_UDID'); const driverHostPortRaw = getEnv('PERCY_IOS_DRIVER_HOST_PORT'); - if (!udid || !driverHostPortRaw) { - log.warn(`iOS resolver env-missing: udid=${udid ? 'set' : 'unset'} driver_port=${driverHostPortRaw ? 'set' : 'unset'}`); + + // ─── Self-hosted (implicit) — PERCY_IOS_DRIVER_HOST_PORT absent ──────── + // Cascade-discover the running Maestro iOS driver's host port: cache → + // probe 127.0.0.1:7001 → lsof self-discovery → warn-skip. No CLI cold- + // start tier (that was Tier B from the original plan, cut after the + // spike confirmed `7001` is deterministic on Maestro ≤2.4.0 + lsof + // covers the ephemeral-port case). UDID is not required here — the + // HTTP `/viewHierarchy` POST doesn't take a udid; the driver host is + // already bound to one device. + if (!driverHostPortRaw) { + const cascade = await resolveSelfHostedIosPort({ httpRequest, execLsof, sessionId, iosPortCache }); + if (cascade) { + const { port, result } = cascade; + if (result.kind === 'hierarchy') { + log.debug(`dump took ${Date.now() - started}ms via maestro-http (self-hosted, port=${port}, ${result.nodes.length} nodes)`); + recordResolverSuccess({ platform: 'ios', via: 'maestro-http' }); + } else { + // `no-aut-tree` from a resolved port — Maestro driver alive (port + // cached) but the AUT isn't foregrounded for THIS snapshot. + // Subsequent snapshots in the session re-probe the cached port + // and may succeed. + log.debug(`dump took ${Date.now() - started}ms via maestro-http (self-hosted, port=${port}, ${result.kind}/${result.reason})`); + recordResolverFinalFailure({ platform: 'ios', failureClass: failureClassFromReason(result.reason) }); + } + return result; + } + log.warn('[percy] iOS element regions: no Maestro driver found on :7001 or via lsof. Set PERCY_IOS_DRIVER_HOST_PORT (real devices use a forwarded port; sharded/newer Maestro may use a different port).'); recordResolverFinalFailure({ platform: 'ios', failureClass: 'other' }); - return { kind: 'unavailable', reason: 'env-missing' }; + return { kind: 'unavailable', reason: 'self-hosted-no-driver' }; } + // ─── Explicit — PERCY_IOS_DRIVER_HOST_PORT present ───────────────────── + // BrowserStack (host-injected) and self-hosted-with-explicit-port both + // take this path. The HTTP primary runs against the supplied port; the + // CLI fallback runs only when UDID is also present (without UDID + // there's no way to target a specific device via `maestro --udid`). + // D3 kill switch (PERCY_MAESTRO_GRPC=0): same env name gates BOTH Maestro // primaries. On iOS this skips runIosHttpDump and routes straight to the // maestro-CLI fallback below. Read every call so toggling at runtime is @@ -1278,8 +1424,10 @@ export async function dump(options) { log.warn('PERCY_MAESTRO_GRPC=0 kill switch active; skipping iOS HTTP primary'); } - // Validate driver-host-port range before attempting HTTP. Out-of-range - // values skip the HTTP path entirely and fall through to maestro-CLI. + // Validate driver-host-port — integer 1-65535 (relaxed from the BS + // 11100-11110 range; BS values stay valid as a subset). Out-of-range + // values skip the HTTP path entirely and fall through to maestro-CLI + // (when UDID is present) or warn-skip (when not). const driverHostPort = parseIosDriverHostPort(driverHostPortRaw); let httpResult = null; if (!iosKillSwitch && driverHostPort !== null) { @@ -1308,6 +1456,15 @@ export async function dump(options) { log.info(`[percy] hierarchy: maestro-http failed (other: ${oorReason}) → falling back to maestro-cli-fallback`); } + // CLI fallback requires UDID. Without it (port-only self-hosted), the + // HTTP attempt above was the only available path — emit warn-skip with + // an actionable message rather than running maestro without a target. + if (!udid) { + log.warn('[percy] iOS resolver: PERCY_IOS_DEVICE_UDID absent — no CLI fallback. HTTP primary either succeeded above and returned, or failed and there is no further path. Set UDID to enable the CLI fallback.'); + recordResolverFinalFailure({ platform: 'ios', failureClass: 'other' }); + return { kind: 'unavailable', reason: 'self-hosted-no-udid' }; + } + const cliResult = await runMaestroIosDump(udid, driverHostPort ?? driverHostPortRaw, execMaestro, getEnv); const httpReason = httpResult ? `${httpResult.kind}/${httpResult.reason}` : 'out-of-range-port'; log.debug(`dump took ${Date.now() - started}ms via maestro-cli-fallback (${httpReason}) kind=${cliResult.kind}`); diff --git a/packages/core/src/percy.js b/packages/core/src/percy.js index 2878299c5..29c90d9b5 100644 --- a/packages/core/src/percy.js +++ b/packages/core/src/percy.js @@ -145,6 +145,12 @@ export class Percy { this.grpcClientCache = new Map(); this.grpcClientCache.shutdownInProgress = false; + // Self-hosted iOS port cache — mirrors the per-`Percy`-instance scope of + // grpcClientCache (D9 of the maestro 4-PR plan). Set on the first + // successful self-hosted iOS port resolution and reused for the rest of + // the session so subsequent snapshots skip the probe/lsof cascade. + this.iosPortCache = { port: null }; + // Domain validation state for auto domain allow-listing this.domainValidation = { autoConfiguredHosts: new Set(), // Domains from project config diff --git a/packages/core/test/unit/maestro-hierarchy.test.js b/packages/core/test/unit/maestro-hierarchy.test.js index 6597bfce9..28099947a 100644 --- a/packages/core/test/unit/maestro-hierarchy.test.js +++ b/packages/core/test/unit/maestro-hierarchy.test.js @@ -476,27 +476,42 @@ describe('Unit / maestro-hierarchy', () => { }); describe('iOS dispatch — env handling', () => { - it('returns env-missing when PERCY_IOS_DEVICE_UDID is unset', async () => { + // Self-hosted-with-explicit-port-but-no-UDID: enters the EXPLICIT branch + // (port present), runs the HTTP primary. With UDID absent, the CLI + // fallback is unavailable — on HTTP success the dump returns; on HTTP + // failure (connection-fail here), warn-skip with the new + // `self-hosted-no-udid` reason. + it('warn-skips with self-hosted-no-udid when port is set, udid is unset, and HTTP primary fails', async () => { const getEnv = key => { if (key === 'PERCY_IOS_DRIVER_HOST_PORT') return '11100'; return undefined; }; - const res = await dump({ platform: 'ios', getEnv }); - expect(res).toEqual({ kind: 'unavailable', reason: 'env-missing' }); + const httpRequest = async () => { throw Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); }; + const res = await dump({ platform: 'ios', getEnv, httpRequest }); + expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-udid' }); }); - it('returns env-missing when PERCY_IOS_DRIVER_HOST_PORT is unset', async () => { + // Self-hosted (UDID-set, PORT-unset): enters the IMPLICIT branch and + // runs the discovery cascade (probe 7001 → lsof). With both injected + // fakes failing, cascade returns null → warn-skip with the new + // `self-hosted-no-driver` reason. UDID being set is irrelevant on the + // implicit path — HTTP `/viewHierarchy` doesn't take a udid. + it('warn-skips with self-hosted-no-driver when port is unset and the discovery cascade finds nothing', async () => { const getEnv = key => { if (key === 'PERCY_IOS_DEVICE_UDID') return '00008110-000065081404401E'; return undefined; }; - const res = await dump({ platform: 'ios', getEnv }); - expect(res).toEqual({ kind: 'unavailable', reason: 'env-missing' }); + const httpRequest = async () => { throw Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); }; + const execLsof = async () => ({ stdout: '', stderr: '', exitCode: 0 }); + const res = await dump({ platform: 'ios', getEnv, httpRequest, execLsof }); + expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); }); - it('returns env-missing when both env vars are unset', async () => { - const res = await dump({ platform: 'ios', getEnv: () => undefined }); - expect(res).toEqual({ kind: 'unavailable', reason: 'env-missing' }); + it('warn-skips with self-hosted-no-driver when both env vars are unset and the cascade finds nothing', async () => { + const httpRequest = async () => { throw Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); }; + const execLsof = async () => ({ stdout: '', stderr: '', exitCode: 0 }); + const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof }); + expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); }); it('does not invoke adb on iOS dispatch', async () => { @@ -546,6 +561,191 @@ describe('Unit / maestro-hierarchy', () => { }); }); + // Self-hosted iOS path: triggered when PERCY_IOS_DRIVER_HOST_PORT is + // absent. The resolver auto-discovers the running Maestro driver port + // via probe 127.0.0.1:7001 → lsof → warn-skip. The BS path (explicit + // env vars) does not exercise this code at all. + describe('iOS self-hosted port cascade', () => { + // Minimal axElement response matching the existing iOS HTTP fixture + // shape — single AUT root, one button child with a frame. Enough for + // runIosHttpDump to return { kind: 'hierarchy' }. + const minimalAxElementJson = JSON.stringify({ + axElement: { + elementType: 1, + identifier: 'com.example.app', + frame: { X: 0, Y: 0, Width: 100, Height: 100 }, + children: [ + { elementType: 9, identifier: 'btn', label: 'OK', frame: { X: 10, Y: 10, Width: 50, Height: 30 } } + ] + } + }); + + const successfulHttpResponse = { + statusCode: 200, + headers: { 'content-type': 'application/json' }, + body: minimalAxElementJson + }; + + const connectionRefused = () => Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); + + it('probe-7001 hit: cascade returns hierarchy and caches port 7001', async () => { + const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(successfulHttpResponse); + const execLsof = jasmine.createSpy('execLsof'); + const iosPortCache = { port: null }; + + const res = await dump({ + platform: 'ios', + getEnv: () => undefined, + httpRequest, + execLsof, + iosPortCache + }); + + expect(res.kind).toBe('hierarchy'); + expect(res.nodes.length).toBeGreaterThan(0); + // Probed exactly :7001; never invoked lsof (probe succeeded first). + expect(httpRequest.calls.count()).toBe(1); + expect(httpRequest.calls.first().args[0].port).toBe(7001); + expect(execLsof).not.toHaveBeenCalled(); + // Cache populated. + expect(iosPortCache.port).toBe(7001); + }); + + it('lsof discovery: 7001 fails, lsof returns exactly one xctrunner listener, cascade probes it', async () => { + const ephemeralPort = 51234; + const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async ({ port }) => { + if (port === 7001) throw connectionRefused(); + if (port === ephemeralPort) return successfulHttpResponse; + throw connectionRefused(); + }); + const lsofStdout = `COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME\ndev.mobile.maestro-driver-iosUITests.xctrunner 12345 user 8u IPv4 0x1 0t0 TCP *:${ephemeralPort} (LISTEN)\n`; + const execLsof = jasmine.createSpy('execLsof').and.resolveTo({ stdout: lsofStdout, stderr: '', exitCode: 0 }); + const iosPortCache = { port: null }; + + const res = await dump({ + platform: 'ios', + getEnv: () => undefined, + httpRequest, + execLsof, + iosPortCache + }); + + expect(res.kind).toBe('hierarchy'); + expect(execLsof).toHaveBeenCalled(); + // Probed 7001 first, then lsof-discovered port. + const probedPorts = httpRequest.calls.allArgs().map(args => args[0].port); + expect(probedPorts).toEqual([7001, ephemeralPort]); + expect(iosPortCache.port).toBe(ephemeralPort); + }); + + it('lsof zero matches: cascade warn-skips without guessing', async () => { + const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); + // No xctrunner row in lsof output. + const lsofStdout = 'COMMAND PID USER FD TYPE DEVICE NAME\nnode 999 user 8u IPv4 0t0 TCP *:3000 (LISTEN)\n'; + const execLsof = async () => ({ stdout: lsofStdout, stderr: '', exitCode: 0 }); + const iosPortCache = { port: null }; + + const res = await dump({ + platform: 'ios', + getEnv: () => undefined, + httpRequest, + execLsof, + iosPortCache + }); + + expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); + expect(iosPortCache.port).toBeNull(); + }); + + it('lsof multi-match (two xctrunner listeners): cascade refuses to guess and warn-skips', async () => { + const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); + const lsofStdout = [ + 'COMMAND PID USER FD TYPE DEVICE NAME', + 'dev.mobile.maestro-driver-iosUITests.xctrunner 100 user 8u IPv4 0t0 TCP *:51234 (LISTEN)', + 'dev.mobile.maestro-driver-iosUITests.xctrunner 101 user 8u IPv4 0t0 TCP *:51235 (LISTEN)', + '' + ].join('\n'); + const execLsof = async () => ({ stdout: lsofStdout, stderr: '', exitCode: 0 }); + const iosPortCache = { port: null }; + + const res = await dump({ + platform: 'ios', + getEnv: () => undefined, + httpRequest, + execLsof, + iosPortCache + }); + + expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); + expect(iosPortCache.port).toBeNull(); + }); + + it('explicit PERCY_IOS_DRIVER_HOST_PORT (out-of-legacy-range) bypasses cascade and runs HTTP primary', async () => { + // Customer pinned --driver-host-port 6001 (e.g., real-device-forwarded + // port). UDID absent — common single-device self-hosted case. The + // EXPLICIT branch runs runIosHttpDump on 6001 (relaxed range admits + // it); cascade/lsof are NOT invoked. + const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(successfulHttpResponse); + const execLsof = jasmine.createSpy('execLsof'); + const getEnv = key => (key === 'PERCY_IOS_DRIVER_HOST_PORT' ? '6001' : undefined); + + const res = await dump({ platform: 'ios', getEnv, httpRequest, execLsof }); + + expect(res.kind).toBe('hierarchy'); + expect(httpRequest.calls.count()).toBe(1); + expect(httpRequest.calls.first().args[0].port).toBe(6001); + expect(execLsof).not.toHaveBeenCalled(); + }); + + it('probe returns dump-error (wrong service): cascade does not cache and falls through', async () => { + // The probed port answers 200 but with a body missing axElement — + // runIosHttpDump returns { kind: 'dump-error', reason: 'http-missing-root' }. + // This is NOT a Maestro driver; cascade must not cache, must move on + // to lsof (which here returns no matches), and end in warn-skip with + // an empty cache. + const wrongServiceResponse = { + statusCode: 200, + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ unrelated: 'shape' }) + }; + const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(wrongServiceResponse); + const execLsof = async () => ({ stdout: '', stderr: '', exitCode: 0 }); + const iosPortCache = { port: null }; + + const res = await dump({ + platform: 'ios', + getEnv: () => undefined, + httpRequest, + execLsof, + iosPortCache + }); + + expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); + expect(iosPortCache.port).toBeNull(); + }); + + it('cache hit: a second invocation with the same iosPortCache reuses the resolved port without re-probing/re-lsof', async () => { + const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(successfulHttpResponse); + const execLsof = jasmine.createSpy('execLsof'); + const iosPortCache = { port: null }; + + // First call: cascade resolves to 7001 and caches it. + await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache }); + expect(iosPortCache.port).toBe(7001); + const firstCallCount = httpRequest.calls.count(); + + // Second call: cache hit → single HTTP to the cached port. lsof never + // invoked on either call. + httpRequest.calls.reset(); + await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache }); + expect(httpRequest.calls.count()).toBe(1); + expect(httpRequest.calls.first().args[0].port).toBe(7001); + expect(execLsof).not.toHaveBeenCalled(); + // Sanity: first call probed exactly once (7001 hit) — no range/lsof. + expect(firstCallCount).toBe(1); + }); + }); + describe('iOS HTTP dump (runIosHttpDump primary path)', () => { const iosFixtureDir = path.resolve(url.fileURLToPath(import.meta.url), '../../fixtures/maestro-ios-hierarchy'); const loadIosFixture = name => fs.readFileSync(path.join(iosFixtureDir, name), 'utf8'); From d0c61bbf046f8e1f2bccd7d8cad0b73728ea609d Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Thu, 28 May 2026 15:35:00 +0530 Subject: [PATCH 04/22] =?UTF-8?q?test(core):=20cross-platform=20parity=20?= =?UTF-8?q?=E2=80=94=20iOS=20env-missing=20now=20returns=20self-hosted-no-?= =?UTF-8?q?driver?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught by CI on PR #2248: the cross-platform parity test asserted the old `env-missing` reason tag, which Unit 2 replaced with `self-hosted-no-driver` (PERCY_IOS_DRIVER_HOST_PORT absent enters the new self-hosted IMPLICIT cascade and warn-skips with the new reason when no driver is found). Test now injects fakes for httpRequest + execLsof so the cascade deterministically exhausts and returns `self-hosted-no-driver`. The envelope-shape parity invariant (kind = 'unavailable') is preserved. Fix-up to commit ccb9bc0b (Unit 2). --- .../test/unit/maestro-hierarchy.parity.test.js | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/core/test/unit/maestro-hierarchy.parity.test.js b/packages/core/test/unit/maestro-hierarchy.parity.test.js index 6ab5ceb86..b7eee3e0b 100644 --- a/packages/core/test/unit/maestro-hierarchy.parity.test.js +++ b/packages/core/test/unit/maestro-hierarchy.parity.test.js @@ -89,11 +89,19 @@ describe('Unit / maestro-hierarchy / cross-platform parity', () => { expect(res.nodes.length).toBeGreaterThan(0); }); - it('iOS env-missing returns { kind: "unavailable", reason: "env-missing" }', async () => { - // Same envelope shape as Android-failure paths, just a different reason tag. - const res = await dump({ platform: 'ios', getEnv: () => undefined }); + it('iOS env-missing returns { kind: "unavailable", reason: "self-hosted-no-driver" }', async () => { + // Same envelope shape as Android-failure paths, just a different reason + // tag. Post-Unit-2: with both PERCY_IOS_* env vars absent, the + // dispatch enters the self-hosted IMPLICIT branch and runs the port + // cascade (probe 7001 → lsof). With injected fakes that fail at every + // tier, the cascade returns null and the dispatch emits the new + // `self-hosted-no-driver` warn-skip reason. The envelope shape (kind + // = 'unavailable') matches Android-failure paths unchanged. + const httpRequest = async () => { throw Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); }; + const execLsof = async () => ({ stdout: '', stderr: '', exitCode: 0 }); + const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof }); expect(res.kind).toBe('unavailable'); - expect(res.reason).toBe('env-missing'); + expect(res.reason).toBe('self-hosted-no-driver'); }); it('iOS env-set with no http/maestro reachable returns same envelope kinds as Android failure paths', async () => { From 8aa75c6d5d5d49888b6cc0354a90cd7d18e0aeac Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Fri, 29 May 2026 13:28:36 +0530 Subject: [PATCH 05/22] feat(cli-exec): export PERCY_CLI_API alongside PERCY_SERVER_ADDRESS Aligns the env contract `percy exec` / `percy app:exec` propagate to child processes with what `percy-appium-python` reads. The Python SDK reads `PERCY_CLI_API`; today it only works by coincidence because both `app:exec` and the SDK default to port 5338. With `--port` overrides (or any future port shift), the SDK silently points at the wrong CLI unless the customer exports `PERCY_CLI_API` themselves. Setting it next to the existing `PERCY_SERVER_ADDRESS` removes that footgun without changing any other behavior. Matches the unconditional- override semantics already in place for `PERCY_SERVER_ADDRESS`. Also adds `PERCY_CLI_API` to cli-doctor's CLEAN_PERCY_ENV test fixture so env-audit tests stay hermetic against shells that have the var set. The runtime env-audit code filters on `k.startsWith('PERCY_')` and needs no change. --- packages/cli-doctor/test/env-audit.test.js | 1 + packages/cli-exec/src/exec.js | 1 + packages/cli-exec/test/exec.test.js | 17 +++++++++++++++++ 3 files changed, 19 insertions(+) diff --git a/packages/cli-doctor/test/env-audit.test.js b/packages/cli-doctor/test/env-audit.test.js index 73d84776e..cf661fd3d 100644 --- a/packages/cli-doctor/test/env-audit.test.js +++ b/packages/cli-doctor/test/env-audit.test.js @@ -62,6 +62,7 @@ const CLEAN_PERCY_ENV = { PERCY_CHROMIUM_BASE_URL: undefined, PERCY_PAC_FILE_URL: undefined, PERCY_CLIENT_API_URL: undefined, + PERCY_CLI_API: undefined, PERCY_SERVER_ADDRESS: undefined, PERCY_SERVER_HOST: undefined, PERCY_COMMIT: undefined, diff --git a/packages/cli-exec/src/exec.js b/packages/cli-exec/src/exec.js index be4850e86..81bfa89de 100644 --- a/packages/cli-exec/src/exec.js +++ b/packages/cli-exec/src/exec.js @@ -86,6 +86,7 @@ export const exec = command('exec', { // provide SDKs with useful env vars env.PERCY_SERVER_ADDRESS = percy?.address(); + env.PERCY_CLI_API = percy?.address(); env.PERCY_BUILD_ID = percy?.build?.id; env.PERCY_BUILD_URL = percy?.build?.url; env.PERCY_LOGLEVEL = log.loglevel(); diff --git a/packages/cli-exec/test/exec.test.js b/packages/cli-exec/test/exec.test.js index 792fa72c7..592b561d1 100644 --- a/packages/cli-exec/test/exec.test.js +++ b/packages/cli-exec/test/exec.test.js @@ -377,4 +377,21 @@ describe('percy exec', () => { '[percy] Finalized build #1: https://percy.io/test/test/123' ])); }); + + it('provides the child process with a PERCY_CLI_API env var matching the server address', async () => { + await exec(['--port=4567', '--', 'node', '--eval', ( + 'process.env.PERCY_CLI_API === process.env.PERCY_SERVER_ADDRESS ' + + '&& process.env.PERCY_CLI_API === "http://localhost:4567" || process.exit(2)' + )]); + + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy] Detected error for percy build', + '[percy] Failure: Snapshot command was not called' + ])); + expect(logger.stdout).toEqual(jasmine.arrayContaining([ + '[percy] Percy has started!', + jasmine.stringMatching('\\[percy] Running "node '), + '[percy] Finalized build #1: https://percy.io/test/test/123' + ])); + }); }); From 690c5009cb618e8b3681ac6c7a7ebfd556de275a Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Fri, 29 May 2026 14:02:44 +0530 Subject: [PATCH 06/22] feat(cli-app): auto-inject -e PERCY_SERVER for `maestro test` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 self-hosted. Every customer hits this: their `maestro test` flow falls through to the BS-safe `http://percy.cli:5338` default, the healthcheck fails, and the build finalizes with "Snapshot command was not called". The workaround today is for the customer to thread `-e PERCY_SERVER=http://localhost:` through every Maestro invocation themselves, pairing ports manually when running multi- device. `app:exec` already knows the resolved port via `percy.address()`. Detecting `maestro test` in argv and prepending one `-e` pair removes the footgun without changing any other behavior. The detector is conservative: basename match on argv[0], `test` subcommand, and a scan for any pre-existing `-e PERCY_SERVER=...` (customer override wins). `npx maestro` and other shim invocations correctly fall through to the explicit-`-e` pattern. BrowserStack path is unaffected — BS doesn't wrap maestro with `app:exec`, so this code path is unreachable on BS. --- packages/cli-app/src/exec.js | 31 ++++++- packages/cli-app/src/index.js | 2 +- packages/cli-app/test/exec.test.js | 142 ++++++++++++++++++++++++++++- 3 files changed, 170 insertions(+), 5 deletions(-) diff --git a/packages/cli-app/src/exec.js b/packages/cli-app/src/exec.js index ccec76328..9caa8672b 100644 --- a/packages/cli-app/src/exec.js +++ b/packages/cli-app/src/exec.js @@ -1,3 +1,4 @@ +import path from 'path'; import command from '@percy/cli-command'; import * as ExecPlugin from '@percy/cli-exec'; @@ -15,6 +16,31 @@ export const start = command('start', { } }, ExecPlugin.start.callback); +function hasExistingPercyServerFlag(args) { + for (let i = 2; i < args.length - 1; i++) { + if (args[i] === '-e' && /^PERCY_SERVER=/.test(args[i + 1])) return true; + } + return false; +} + +// 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=...`. +export function maybeInjectMaestroServer(ctx) { + const args = ctx?.argv; + if (!Array.isArray(args) || args.length < 2) return; + if (path.basename(args[0]) !== 'maestro') return; + if (args[1] !== 'test') return; + if (hasExistingPercyServerFlag(args)) return; + const addr = ctx.percy?.address(); + if (!addr) return; + args.splice(2, 0, '-e', `PERCY_SERVER=${addr}`); +} + export const exec = command('exec', { description: 'Start and stop Percy around a supplied command for native apps', usage: '[options] -- ', @@ -29,6 +55,9 @@ export const exec = command('exec', { projectType: 'app', skipDiscovery: true } -}, ExecPlugin.default.callback); +}, async function*(ctx) { + maybeInjectMaestroServer(ctx); + yield* ExecPlugin.default.callback(ctx); +}); export default exec; diff --git a/packages/cli-app/src/index.js b/packages/cli-app/src/index.js index 49012033f..4f95c7b7d 100644 --- a/packages/cli-app/src/index.js +++ b/packages/cli-app/src/index.js @@ -1,2 +1,2 @@ export { default, app } from './app.js'; -export { exec, start, stop, ping } from './exec.js'; +export { exec, start, stop, ping, maybeInjectMaestroServer } from './exec.js'; diff --git a/packages/cli-app/test/exec.test.js b/packages/cli-app/test/exec.test.js index 100c1fd5e..7b859c8c4 100644 --- a/packages/cli-app/test/exec.test.js +++ b/packages/cli-app/test/exec.test.js @@ -1,14 +1,18 @@ import { setupTest } from '@percy/cli-command/test/helpers'; import * as ExecPlugin from '@percy/cli-exec'; -import { exec, start, stop, ping } from '@percy/cli-app'; +import { exec, start, stop, ping, maybeInjectMaestroServer } from '@percy/cli-app'; describe('percy app:exec', () => { beforeEach(async () => { await setupTest(); }); - it('has shared exec commands with differing definitions', async () => { - expect(exec.callback).toEqual(ExecPlugin.default.callback); + it('wraps cli-exec callbacks while preserving differing definitions', async () => { + // exec.callback wraps cli-exec's callback (auto-injects -e PERCY_SERVER + // for `maestro test`), so it is no longer reference-equal — but start, + // stop, and ping remain straight delegations. + expect(typeof exec.callback).toBe('function'); + expect(exec.callback).not.toEqual(ExecPlugin.default.callback); expect(exec.definition).not.toEqual(ExecPlugin.default.definition); expect(start.callback).toEqual(ExecPlugin.start.callback); expect(start.definition).not.toEqual(ExecPlugin.start.definition); @@ -23,4 +27,136 @@ describe('percy app:exec', () => { await expectAsync(start(['--network-idle-timeout', '500'])) .toBeRejectedWithError("Unknown option '--network-idle-timeout'"); }); + + describe('maybeInjectMaestroServer', () => { + function ctxFor(argv, addr = 'http://localhost:5338') { + return { + argv: [...argv], + percy: { address: () => addr } + }; + } + + it('injects -e PERCY_SERVER at index 2 for `maestro test`', () => { + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '-e', 'PERCY_SERVER=http://localhost:5338', 'flow.yaml' + ]); + }); + + it('preserves other -e flags and customer args after injection', () => { + const ctx = ctxFor([ + 'maestro', 'test', '--test-output-dir', 'out', + '-e', 'PERCY_DEVICE_NAME=Pixel 10', 'flow.yaml' + ]); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + 'maestro', 'test', + '-e', 'PERCY_SERVER=http://localhost:5338', + '--test-output-dir', 'out', + '-e', 'PERCY_DEVICE_NAME=Pixel 10', + 'flow.yaml' + ]); + }); + + it('skips when customer already supplied -e PERCY_SERVER (adjacent to test)', () => { + const argv = ['maestro', 'test', '-e', 'PERCY_SERVER=http://custom:9999', 'flow.yaml']; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when customer supplied -e PERCY_SERVER deeper in args (R3 scan)', () => { + const argv = [ + 'maestro', 'test', '--test-output-dir', 'out', + '-e', 'PERCY_SERVER=http://custom:9999', 'flow.yaml' + ]; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for `maestro hierarchy` (not a test command)', () => { + const argv = ['maestro', 'hierarchy', '--udid', 'X']; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for `maestro list-devices` (not a test command)', () => { + const argv = ['maestro', 'list-devices']; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when args has fewer than two elements', () => { + const argv = ['maestro']; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('matches by basename when the command is an absolute path', () => { + const ctx = ctxFor(['/Users/foo/.maestro/bin/maestro', 'test', 'flow.yaml']); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + '/Users/foo/.maestro/bin/maestro', 'test', + '-e', 'PERCY_SERVER=http://localhost:5338', + 'flow.yaml' + ]); + }); + + it('skips for `npx maestro test` (argv[0] is npx, not maestro)', () => { + const argv = ['npx', 'maestro', 'test', 'flow.yaml']; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for non-maestro commands (python, appium, etc.)', () => { + const pyArgv = ['python', 'test.py']; + const pyCtx = ctxFor(pyArgv); + maybeInjectMaestroServer(pyCtx); + expect(pyCtx.argv).toEqual(pyArgv); + + const apiumArgv = ['appium', '--port', '4723']; + const apiumCtx = ctxFor(apiumArgv); + maybeInjectMaestroServer(apiumCtx); + expect(apiumCtx.argv).toEqual(apiumArgv); + }); + + it('flows --port through into the injected address (multi-device)', () => { + const ctx = ctxFor(['maestro', 'test', 'flow.yaml'], 'http://localhost:5339'); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '-e', 'PERCY_SERVER=http://localhost:5339', 'flow.yaml' + ]); + }); + + it('skips when percy is disabled (no address)', () => { + const argv = ['maestro', 'test', 'flow.yaml']; + const ctx = { argv: [...argv], percy: { address: () => undefined } }; + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when ctx has no percy at all', () => { + const argv = ['maestro', 'test', 'flow.yaml']; + const ctx = { argv: [...argv] }; + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('two concurrent contexts pick their own addresses (multi-device isolation)', () => { + const a = ctxFor(['maestro', 'test', 'flow.yaml'], 'http://localhost:5338'); + const b = ctxFor(['maestro', 'test', 'flow.yaml'], 'http://localhost:5339'); + maybeInjectMaestroServer(a); + maybeInjectMaestroServer(b); + expect(a.argv).toContain('PERCY_SERVER=http://localhost:5338'); + expect(b.argv).toContain('PERCY_SERVER=http://localhost:5339'); + expect(a.argv).not.toContain('PERCY_SERVER=http://localhost:5339'); + expect(b.argv).not.toContain('PERCY_SERVER=http://localhost:5338'); + }); + }); }); From d9d508d06ea6d88e77310fbb2162f061507dd34c Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram <56213889+Sriram567@users.noreply.github.com> Date: Tue, 2 Jun 2026 17:36:11 +0530 Subject: [PATCH 07/22] feat(cli-app): auto-resolve PERCY_MAESTRO_SCREENSHOT_DIR + --test-output-dir; WARN on no-addr injection skip (#2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the last piece of customer-side bookkeeping for self-hosted Maestro+Percy. Today customers must export PERCY_MAESTRO_SCREENSHOT_DIR AND pass --test-output-dir to maestro test. After this PR, `percy app:exec` does both automatically. New helper `maybeInjectScreenshotDir(ctx, log)` next to the existing `maybeInjectMaestroServer`. Resolution order: 1. Customer set BOTH env + --test-output-dir flag → trust them. 2. Customer set env only → use env value, inject matching flag. 3. Customer set flag only → use flag value, mirror to env. 4. Neither set → try ${CWD}/.percy-out. On mkdir failure (EACCES, EROFS, EEXIST), fall back to ${TMPDIR}/percy-maestro- with a WARN log explaining why. The env var and the flag are always kept aligned to the same path. The SDK reads the env var; Maestro reads the flag — both pointing at the same dir is the contract. Also adds a WARN log in `maybeInjectMaestroServer` when `percy.address()` is falsy (percy disabled, start failed). Previously this skip was silent — customer's maestro test would run, no snapshots upload, Percy build would finalize empty with no log hint. The WARN now tells the customer what won't happen and how to fix it (set PERCY_TOKEN). Customer-supplied `-e PERCY_SERVER` override skip does NOT warn — that's legitimate flow control, not a problem. Tests: - 14 new scenarios for maybeInjectScreenshotDir (happy path, env override, flag override, both, EACCES/EROFS/EEXIST fallbacks, hierarchy/npx/python/short-args skip, basename match on full path). - 2 new scenarios for maybeInjectMaestroServer's WARN log. - 30 of 30 cli-app specs pass. --- packages/cli-app/src/exec.js | 100 ++++++++++++++- packages/cli-app/src/index.js | 2 +- packages/cli-app/test/exec.test.js | 197 ++++++++++++++++++++++++++++- 3 files changed, 292 insertions(+), 7 deletions(-) diff --git a/packages/cli-app/src/exec.js b/packages/cli-app/src/exec.js index 9caa8672b..540d7b9d9 100644 --- a/packages/cli-app/src/exec.js +++ b/packages/cli-app/src/exec.js @@ -1,3 +1,5 @@ +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'; @@ -23,6 +25,16 @@ function hasExistingPercyServerFlag(args) { return false; } +// Returns the index of the value following `--test-output-dir`, or -1 if absent. +// We return the value-index (not just a boolean) so the screenshot-dir helper +// can align PERCY_MAESTRO_SCREENSHOT_DIR with a customer-supplied flag value. +function findTestOutputDirValueIdx(args) { + for (let i = 2; i < args.length - 1; i++) { + if (args[i] === '--test-output-dir') return i + 1; + } + return -1; +} + // 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 @@ -30,17 +42,91 @@ function hasExistingPercyServerFlag(args) { // 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=...`. -export function maybeInjectMaestroServer(ctx) { +// +// 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; if (args[1] !== 'test') return; if (hasExistingPercyServerFlag(args)) return; const addr = ctx.percy?.address(); - if (!addr) return; + 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; + } args.splice(2, 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 ` 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 ` 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-` 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; + if (args[1] !== 'test') return; + + const envSet = !!process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + const flagValueIdx = findTestOutputDirValueIdx(args); + const flagSet = flagValueIdx > 0; + + // 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 = args[flagValueIdx]; + } 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; + if (!flagSet) args.splice(2, 0, '--test-output-dir', resolved); +} + export const exec = command('exec', { description: 'Start and stop Percy around a supplied command for native apps', usage: '[options] -- ', @@ -56,7 +142,15 @@ export const exec = command('exec', { skipDiscovery: true } }, async function*(ctx) { - maybeInjectMaestroServer(ctx); + // Order matters: maestro-server splice goes in at index 2 first, then + // screenshot-dir splice goes in at index 2 too (pushing the server flags + // to index 4). Resulting argv for `maestro test flow.yaml` becomes: + // maestro test --test-output-dir

-e PERCY_SERVER= flow.yaml + // Both flag groups land between `test` and the flow file, which Maestro + // accepts. Reversing the order would still work; this order is chosen + // for log readability. + maybeInjectMaestroServer(ctx, ctx.log); + maybeInjectScreenshotDir(ctx, ctx.log); yield* ExecPlugin.default.callback(ctx); }); diff --git a/packages/cli-app/src/index.js b/packages/cli-app/src/index.js index 4f95c7b7d..d1c8d4ffd 100644 --- a/packages/cli-app/src/index.js +++ b/packages/cli-app/src/index.js @@ -1,2 +1,2 @@ export { default, app } from './app.js'; -export { exec, start, stop, ping, maybeInjectMaestroServer } from './exec.js'; +export { exec, start, stop, ping, maybeInjectMaestroServer, maybeInjectScreenshotDir } from './exec.js'; diff --git a/packages/cli-app/test/exec.test.js b/packages/cli-app/test/exec.test.js index 7b859c8c4..be32f1ee9 100644 --- a/packages/cli-app/test/exec.test.js +++ b/packages/cli-app/test/exec.test.js @@ -1,6 +1,12 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; import { setupTest } from '@percy/cli-command/test/helpers'; import * as ExecPlugin from '@percy/cli-exec'; -import { exec, start, stop, ping, maybeInjectMaestroServer } from '@percy/cli-app'; +import { + exec, start, stop, ping, + maybeInjectMaestroServer, maybeInjectScreenshotDir +} from '@percy/cli-app'; describe('percy app:exec', () => { beforeEach(async () => { @@ -9,8 +15,8 @@ describe('percy app:exec', () => { it('wraps cli-exec callbacks while preserving differing definitions', async () => { // exec.callback wraps cli-exec's callback (auto-injects -e PERCY_SERVER - // for `maestro test`), so it is no longer reference-equal — but start, - // stop, and ping remain straight delegations. + // and --test-output-dir for `maestro test`), so it is no longer reference- + // equal — but start, stop, and ping remain straight delegations. expect(typeof exec.callback).toBe('function'); expect(exec.callback).not.toEqual(ExecPlugin.default.callback); expect(exec.definition).not.toEqual(ExecPlugin.default.definition); @@ -158,5 +164,190 @@ describe('percy app:exec', () => { expect(a.argv).not.toContain('PERCY_SERVER=http://localhost:5339'); expect(b.argv).not.toContain('PERCY_SERVER=http://localhost:5338'); }); + + it('emits WARN log when percy address is falsy and there is no customer override', () => { + const log = { warn: jasmine.createSpy('warn') }; + const ctx = { argv: ['maestro', 'test', 'flow.yaml'], percy: { address: () => undefined } }; + maybeInjectMaestroServer(ctx, log); + expect(log.warn).toHaveBeenCalledTimes(1); + expect(log.warn.calls.argsFor(0)[0]).toContain('-e PERCY_SERVER not injected'); + }); + + it('does NOT emit WARN when customer-supplied -e PERCY_SERVER override is present', () => { + const log = { warn: jasmine.createSpy('warn') }; + const ctx = { + argv: ['maestro', 'test', '-e', 'PERCY_SERVER=http://custom:9999', 'flow.yaml'], + percy: { address: () => undefined } + }; + maybeInjectMaestroServer(ctx, log); + expect(log.warn).not.toHaveBeenCalled(); + }); + }); + + describe('maybeInjectScreenshotDir', () => { + let originalEnvValue; + + beforeEach(() => { + originalEnvValue = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + }); + + afterEach(() => { + if (originalEnvValue === undefined) { + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + } else { + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = originalEnvValue; + } + }); + + function ctxFor(argv) { + return { argv: [...argv] }; + } + + it('injects --test-output-dir and sets env var to /.percy-out on happy path', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const expectedDir = path.join(process.cwd(), '.percy-out'); + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx); + expect(mkdir).toHaveBeenCalledWith(expectedDir, { recursive: true }); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(expectedDir); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '--test-output-dir', expectedDir, 'flow.yaml' + ]); + }); + + it('honors customer-set PERCY_MAESTRO_SCREENSHOT_DIR and injects flag aligned to it', () => { + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = '/custom/screenshot/dir'; + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe('/custom/screenshot/dir'); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '--test-output-dir', '/custom/screenshot/dir', 'flow.yaml' + ]); + }); + + it('honors customer-supplied --test-output-dir and mirrors value into env var', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const ctx = ctxFor(['maestro', 'test', '--test-output-dir', '/custom/path', 'flow.yaml']); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe('/custom/path'); + // argv unchanged — customer's flag stays where they put it + expect(ctx.argv).toEqual(['maestro', 'test', '--test-output-dir', '/custom/path', 'flow.yaml']); + }); + + it('skips entirely when both env var and --test-output-dir are customer-set', () => { + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = '/from/env'; + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const argv = ['maestro', 'test', '--test-output-dir', '/from/flag', 'flow.yaml']; + const ctx = ctxFor(argv); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe('/from/env'); + expect(ctx.argv).toEqual(argv); + }); + + it('falls back to /percy-maestro- on EACCES and emits WARN', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake((dirPath) => { + if (dirPath === path.join(process.cwd(), '.percy-out')) { + const err = new Error('EACCES'); err.code = 'EACCES'; + throw err; + } + }); + const log = { warn: jasmine.createSpy('warn') }; + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx, log); + const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`); + expect(mkdir).toHaveBeenCalledWith(fallback, { recursive: true }); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(fallback); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '--test-output-dir', fallback, 'flow.yaml' + ]); + expect(log.warn).toHaveBeenCalledTimes(1); + expect(log.warn.calls.argsFor(0)[0]).toContain('EACCES'); + expect(log.warn.calls.argsFor(0)[0]).toContain(fallback); + }); + + it('falls back on EROFS (read-only filesystem)', () => { + spyOn(fs, 'mkdirSync').and.callFake((dirPath) => { + if (dirPath === path.join(process.cwd(), '.percy-out')) { + const err = new Error('EROFS'); err.code = 'EROFS'; + throw err; + } + }); + const log = { warn: jasmine.createSpy('warn') }; + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx, log); + const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(fallback); + expect(log.warn).toHaveBeenCalled(); + }); + + it('falls back on EEXIST (.percy-out collides with a non-directory)', () => { + spyOn(fs, 'mkdirSync').and.callFake((dirPath) => { + if (dirPath === path.join(process.cwd(), '.percy-out')) { + const err = new Error('EEXIST'); err.code = 'EEXIST'; + throw err; + } + }); + const log = { warn: jasmine.createSpy('warn') }; + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx, log); + const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(fallback); + expect(log.warn).toHaveBeenCalled(); + }); + + it('skips for `maestro hierarchy` (not a test command)', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const argv = ['maestro', 'hierarchy', '--udid', 'X']; + const ctx = ctxFor(argv); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBeUndefined(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for `npx maestro test` (argv[0] is npx, not maestro)', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const argv = ['npx', 'maestro', 'test', 'flow.yaml']; + const ctx = ctxFor(argv); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for non-maestro commands', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const argv = ['python', 'test.py']; + const ctx = ctxFor(argv); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when args has fewer than two elements', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const argv = ['maestro']; + const ctx = ctxFor(argv); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('matches by basename when the command is an absolute path', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const expectedDir = path.join(process.cwd(), '.percy-out'); + const ctx = ctxFor(['/Users/foo/.maestro/bin/maestro', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx); + expect(mkdir).toHaveBeenCalledWith(expectedDir, { recursive: true }); + expect(ctx.argv).toEqual([ + '/Users/foo/.maestro/bin/maestro', 'test', + '--test-output-dir', expectedDir, + 'flow.yaml' + ]); + }); }); }); From ac864475bb257fc387b61c3be60bb7a1d66071ac Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram <56213889+Sriram567@users.noreply.github.com> Date: Tue, 2 Jun 2026 17:36:17 +0530 Subject: [PATCH 08/22] feat(core): explicit runtime field gates /percy/maestro-screenshot self-hosted detection (R2) (#2264) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Promotes the self-hosted-vs-BrowserStack discriminator in the relay's /percy/maestro-screenshot handler from an implicit signal (sessionId absence) to an explicit one (runtime: "selfhosted" | "browserstack" in the SDK payload). The sessionId-absent fallback stays for backward compatibility with SDKs that predate the runtime field. Why: cli#2261's `let selfHosted = !sessionId;` derives the runtime from a field's absence. If BS ever omits sessionId in a future code path (retry, new session type), the self-hosted branch activates by accident → wrong file-find scope → 404. Moving the declaration to the SDK (where the knowledge originates — the SDK knows whether PERCY_SESSION_ID was injected) eliminates that future-proofing risk. Wire contract (additive, fully back-compat): selfHosted = (runtime === "selfhosted") || (!runtime && !sessionId) Unknown runtime values are NOT rejected — the relay falls back to the sessionId check, so SDKs experimenting with future values ("maestro-cloud", "saucelabs", etc.) don't break. Two `percy.log.debug` lines surface bidirectional inconsistencies (runtime + sessionId disagree) for diagnostic surface, never failing the request. R7 (BS regression) preserved — when SDK emits runtime: "browserstack" + sessionId, the BS branch runs byte-identically to today. Tests: - 9 new scenarios for the runtime field gating (8-way matrix of runtime × sessionId presence, plus the type-validation 400). - Existing /percy/maestro-screenshot tests unchanged. Stacks on cli#2261; companion SDK PR (percy-maestro-app) ships the runtime field in a future @percy/maestro-app release. Relay works today with older SDKs via the back-compat fallback. Origin: percy-maestro/docs/brainstorms/2026-06-02-selfhosted-followup-bundle-requirements.md (R2) Plan: percy-maestro/docs/plans/2026-06-02-001-feat-explicit-runtime-field-plan.md (Unit 2) --- packages/core/src/api.js | 31 +++++++++-- packages/core/test/api.test.js | 97 ++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 5 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 21652835b..8fae6d516 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -422,13 +422,34 @@ export function createPercyServer(percy, port) { // post a comparison by reading a Maestro screenshot from disk .route('post', '/percy/maestro-screenshot', async (req, res) => { /* istanbul ignore next — req.body falsy guard; tests always pass a body. */ - let { name, sessionId } = req.body || {}; + let { name, sessionId, runtime } = req.body || {}; if (!name) throw new ServerError(400, 'Missing required field: name'); - // `sessionId` is host-injected on BrowserStack; its absence signals - // self-hosted mode (gated separately on PERCY_MAESTRO_SCREENSHOT_DIR - // below). When present, the BS path runs byte-identical. - let selfHosted = !sessionId; + + // Validate `runtime` if present: must be a string, but unknown values + // are NOT rejected — we treat anything other than "selfhosted" / + // "browserstack" as "fall back to sessionId-absent detection". This + // is defensive against future runtime values (maestro-cloud, + // saucelabs, etc.) the SDK might introduce ahead of relay support. + if (runtime !== undefined && typeof runtime !== 'string') { + throw new ServerError(400, 'Invalid runtime: must be a string'); + } + + // Self-hosted detection — prefer the SDK's explicit declaration when + // present, fall back to sessionId absence (the implicit signal used + // by SDKs that predate the runtime field). The fallback is what + // ships in cli#2261 today; the explicit runtime field arrives with + // @percy/maestro-app >= v1.0.0-Beta.1 (cf. percy-maestro-app#7+R2). + let selfHosted = (runtime === 'selfhosted') || (!runtime && !sessionId); + + // Diagnostic surface for any inconsistency between the two signals + // (debug-only — never breaks the request). + if (runtime === 'browserstack' && !sessionId) { + percy.log.debug('maestro-screenshot: runtime=browserstack but sessionId missing — trusting runtime field'); + } + if (runtime === 'selfhosted' && sessionId) { + percy.log.debug('maestro-screenshot: runtime=selfhosted but sessionId present — trusting runtime field'); + } // Strict character-class validation — rejects path separators, shell metacharacters, // NUL, newlines, and anything else that could confuse the glob or the filesystem. diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index e432f9d2c..9dadae5ad 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -1255,6 +1255,103 @@ describe('API Server', () => { .toBeRejectedWithError(/Invalid platform: must be a string/); }); + describe('runtime field gating', () => { + // The relay's self-hosted detection prefers an explicit `runtime` field + // from the SDK when present, falling back to sessionId-absence for older + // SDKs. These tests cover the eight-way matrix of (runtime ∈ + // {undefined, "selfhosted", "browserstack", "unknown"}) × (sessionId ∈ + // {present, absent}) — plus the type-validation 400. + + it('rejects non-string runtime type with 400', async () => { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, runtime: 123 })) + .toBeRejectedWithError(/Invalid runtime: must be a string/); + }); + + it('runtime=selfhosted + no sessionId → self-hosted branch (400s on missing env)', async () => { + let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + try { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME, runtime: 'selfhosted' })) + .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); + } finally { + if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; + } + }); + + it('runtime=selfhosted + sessionId PRESENT → still self-hosted branch (trust runtime field)', async () => { + // Bidirectional-consistency case: SDK explicitly says self-hosted but + // a sessionId leaked into the payload. Trust the SDK's declaration. + let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + try { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'selfhosted' })) + .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); + } finally { + if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; + } + }); + + it('runtime=browserstack + sessionId present → BS branch finds file under /tmp//...', async () => { + await percy.start(); + const res = await postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'browserstack' }); + expect(res.status).toBe(200); + }); + + it('runtime=browserstack + sessionId ABSENT → BS branch (trust runtime), but file-find fails → 404', async () => { + // Bidirectional-consistency case: SDK explicitly says browserstack + // but sessionId is missing. BS branch runs, can't locate the file + // because `/tmp//...` doesn't resolve. + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME, runtime: 'browserstack' })) + .toBeRejectedWithError(/Screenshot not found|sessionId/i); + }); + + it('runtime=unknown-value + no sessionId → falls back to sessionId-absent → self-hosted branch', async () => { + // Defensive: unknown runtime values are NOT rejected. The relay falls + // back to sessionId-absence detection, so this lands in self-hosted. + let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + try { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME, runtime: 'maestro-cloud' })) + .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); + } finally { + if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; + } + }); + + it('runtime=unknown-value + sessionId present → falls back to sessionId-present → BS branch', async () => { + await percy.start(); + const res = await postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'maestro-cloud' }); + expect(res.status).toBe(200); + }); + + it('no runtime + sessionId present → BS branch (backward-compat with pre-runtime SDKs)', async () => { + await percy.start(); + const res = await postMaestro({ name: SS_NAME, sessionId: SID }); + expect(res.status).toBe(200); + }); + + it('no runtime + no sessionId → self-hosted branch (backward-compat with pre-runtime SDKs)', async () => { + let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + try { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME })) + .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); + } finally { + if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; + } + }); + }); + it('rejects non-object element selector with 400', async () => { await percy.start(); await expectAsync(postMaestro({ From a3d967303d2e81952f20fa3e9d021b4297114947 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Wed, 3 Jun 2026 16:07:31 +0530 Subject: [PATCH 09/22] fix(core): correct self-hosted maestro runtime routing + stabilize relay tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - api.js: route unknown/non-browserstack runtimes by sessionId-absence. Was `(!runtime && !sessionId)`, which left selfHosted=false for an unknown non-empty runtime (e.g. "maestro-cloud") and wrongly sent it to the BS branch. Now `(runtime !== 'browserstack' && !sessionId)` — matches the documented fallback and the 8-case runtime×sessionId test matrix. - api.test.js: assert `res.success` (the request helper resolves to the response body on success; there is no `res.status` → was "undefined to be 200"). - api.test.js: run the self-hosted glob fixtures on the real filesystem via mockfs `$bypass`. fast-glob `**`+`dot:true` over a `.maestro/` dir is unreliable against memfs across volume resets in the full suite (works in isolation and on real fs); the bypass also exercises the true production glob path. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/src/api.js | 8 +++++++- packages/core/test/api.test.js | 22 +++++++++++++++++----- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 8fae6d516..5513bb5b9 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -440,7 +440,13 @@ export function createPercyServer(percy, port) { // by SDKs that predate the runtime field). The fallback is what // ships in cli#2261 today; the explicit runtime field arrives with // @percy/maestro-app >= v1.0.0-Beta.1 (cf. percy-maestro-app#7+R2). - let selfHosted = (runtime === 'selfhosted') || (!runtime && !sessionId); + // + // `runtime === 'browserstack'` always means BS. Anything else that is + // NOT 'selfhosted' (absent OR an unknown future value like + // 'maestro-cloud') falls back to sessionId-absence detection — hence + // `runtime !== 'browserstack' && !sessionId`, not `!runtime && !sessionId` + // (the latter wrongly routed unknown non-empty runtimes to the BS branch). + let selfHosted = (runtime === 'selfhosted') || (runtime !== 'browserstack' && !sessionId); // Diagnostic surface for any inconsistency between the two signals // (debug-only — never breaks the request). diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index 9dadae5ad..a77c0b2e3 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -17,7 +17,13 @@ describe('API Server', () => { beforeEach(async () => { process.env.PERCY_FORCE_PKG_VALUE = JSON.stringify({ name: '@percy/client', version: '1.0.0' }); jasmine.DEFAULT_TIMEOUT_INTERVAL = 150000; - await setupTest(); + // The self-hosted maestro tests exercise a recursive `**` + `dot:true` + // fast-glob against a `.maestro/` directory. fast-glob's recursive-dot + // traversal is unreliable against memfs across volume resets in the full + // suite (works in isolation; returns [] mid-suite), so route the + // self-hosted root to the REAL filesystem — this also tests the true + // production glob path. Only paths under this unique root are affected. + await setupTest({ filesystem: { $bypass: [p => typeof p === 'string' && p.includes('percy-self-hosted-real')] } }); percy = new Percy({ token: 'PERCY_TOKEN', @@ -1299,7 +1305,7 @@ describe('API Server', () => { it('runtime=browserstack + sessionId present → BS branch finds file under /tmp//...', async () => { await percy.start(); const res = await postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'browserstack' }); - expect(res.status).toBe(200); + expect(res.success).toBe(true); }); it('runtime=browserstack + sessionId ABSENT → BS branch (trust runtime), but file-find fails → 404', async () => { @@ -1329,13 +1335,13 @@ describe('API Server', () => { it('runtime=unknown-value + sessionId present → falls back to sessionId-present → BS branch', async () => { await percy.start(); const res = await postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'maestro-cloud' }); - expect(res.status).toBe(200); + expect(res.success).toBe(true); }); it('no runtime + sessionId present → BS branch (backward-compat with pre-runtime SDKs)', async () => { await percy.start(); const res = await postMaestro({ name: SS_NAME, sessionId: SID }); - expect(res.status).toBe(200); + expect(res.success).toBe(true); }); it('no runtime + no sessionId → self-hosted branch (backward-compat with pre-runtime SDKs)', async () => { @@ -1967,7 +1973,10 @@ describe('API Server', () => { // covered above; these tests lock the new branches. // ───────────────────────────────────────────────────────────────────── describe('self-hosted (sessionId absent)', () => { - const SELF_HOSTED_ROOT = '/tmp/percy-self-hosted-root'; + // Real-fs root (matched by the $bypass registered in the top-level + // beforeEach) so the recursive `**` + `dot:true` glob runs against the + // real filesystem — the production path — rather than memfs. + const SELF_HOSTED_ROOT = '/tmp/percy-self-hosted-real-root'; const NESTED_SUBDIR = `${SELF_HOSTED_ROOT}/.maestro/run-x/screenshots`; const SELF_HOSTED_NAME = 'SelfHostedScreen'; let priorEnv; @@ -1975,6 +1984,7 @@ describe('API Server', () => { beforeEach(() => { priorEnv = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; process.env.PERCY_MAESTRO_SCREENSHOT_DIR = SELF_HOSTED_ROOT; + fs.rmSync(SELF_HOSTED_ROOT, { recursive: true, force: true }); fs.mkdirSync(NESTED_SUBDIR, { recursive: true }); // Valid 24-byte PNG header (1080 x 2400) exercises PNG-fill on the // self-hosted path too. @@ -1990,6 +2000,8 @@ describe('API Server', () => { afterEach(() => { if (priorEnv === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = priorEnv; + // Clean up the real-fs fixture root (see beforeEach / top-level $bypass). + fs.rmSync(SELF_HOSTED_ROOT, { recursive: true, force: true }); }); it('finds screenshot via recursive glob under PERCY_MAESTRO_SCREENSHOT_DIR and uploads without sessionId', async () => { From c01a59d4ebe15d6a6ab38d22c83157d15fae71cd Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Thu, 4 Jun 2026 12:25:30 +0530 Subject: [PATCH 10/22] test(core): cover self-hosted maestro coverage gaps (api.js:667, maestro-hierarchy else) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 100% coverage gate flagged two spots in the self-hosted maestro code: - api.js:667 — the self-hosted arm of the "resolved outside" 404 ternary (`PERCY_MAESTRO_SCREENSHOT_DIR`); only the BrowserStack arm was covered. - maestro-hierarchy.js:1402-1403 — the cascade `else` where a resolved iOS port returns a non-hierarchy result (driver alive, AUT not foregrounded). Add two targeted tests: - api.test.js: a symlink inside PERCY_MAESTRO_SCREENSHOT_DIR pointing outside it → realpath+prefix check rejects (self-hosted arm). Uses real fs via the existing $bypass. - maestro-hierarchy.test.js: probe-7001 returns a springboard-only response → no-aut-tree; the port is cached and the non-hierarchy result is returned. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/test/api.test.js | 13 ++++++++ .../core/test/unit/maestro-hierarchy.test.js | 32 +++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index a77c0b2e3..2669e9486 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -2057,6 +2057,19 @@ describe('API Server', () => { .toBeRejectedWithError(/Screenshot not found/); }); + it('404s when a globbed file resolves outside the root (symlink escape)', async () => { + // A symlink inside the root that points outside it must not exfiltrate + // the target — the realpath + prefix check rejects it (self-hosted arm + // of the "resolved outside" guard). Uses real fs via the $bypass. + const outside = '/tmp/percy-self-hosted-real-OUTSIDE.png'; + fs.writeFileSync(outside, 'OUTSIDE'); + fs.symlinkSync(outside, `${NESTED_SUBDIR}/EscapeScreen.png`); + await percy.start(); + await expectAsync(postMaestro({ name: 'EscapeScreen', platform: 'android' })) + .toBeRejectedWithError(/resolved outside PERCY_MAESTRO_SCREENSHOT_DIR/); + fs.rmSync(outside, { force: true }); + }); + it('rejects name with traversal characters (SAFE_ID is load-bearing for the recursive glob)', async () => { await percy.start(); await expectAsync(postMaestro({ name: '../etc/passwd' })) diff --git a/packages/core/test/unit/maestro-hierarchy.test.js b/packages/core/test/unit/maestro-hierarchy.test.js index 28099947a..4d08b5254 100644 --- a/packages/core/test/unit/maestro-hierarchy.test.js +++ b/packages/core/test/unit/maestro-hierarchy.test.js @@ -611,6 +611,38 @@ describe('Unit / maestro-hierarchy', () => { expect(iosPortCache.port).toBe(7001); }); + it('probe-7001 no-aut-tree: driver alive but AUT not foregrounded — caches port, returns no-aut-tree', async () => { + // The resolver caches the port for hierarchy OR no-aut-tree (the driver + // is alive either way). dump()'s self-hosted branch then surfaces the + // non-hierarchy result instead of treating it as "no driver found". + const springboardResponse = { + statusCode: 200, + headers: { 'content-type': 'application/json' }, + body: fs.readFileSync(path.resolve( + url.fileURLToPath(import.meta.url), + '../../fixtures/maestro-ios-hierarchy/viewHierarchy-response-springboard-only.json' + ), 'utf8') + }; + const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(springboardResponse); + const execLsof = jasmine.createSpy('execLsof'); + const iosPortCache = { port: null }; + + const res = await dump({ + platform: 'ios', + getEnv: () => undefined, + httpRequest, + execLsof, + iosPortCache + }); + + expect(res.kind).toBe('no-aut-tree'); + expect(httpRequest.calls.first().args[0].port).toBe(7001); + // Port cached even on no-aut-tree (the driver is alive). + expect(iosPortCache.port).toBe(7001); + // Probe succeeded on :7001 → lsof never consulted. + expect(execLsof).not.toHaveBeenCalled(); + }); + it('lsof discovery: 7001 fails, lsof returns exactly one xctrunner listener, cascade probes it', async () => { const ephemeralPort = 51234; const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async ({ port }) => { From 816d37a9297edb7a3c65658bcc0e8b9e7c9506fa Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Thu, 4 Jun 2026 13:19:35 +0530 Subject: [PATCH 11/22] test(core): cover remaining maestro-hierarchy iOS self-hosted branch gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 100% coverage gate flagged uncovered branches in the new self-hosted iOS port helpers (maestro-hierarchy.js 1278, 1294-1296, 1323, 1346): - lsofXctrunnerPort result guard: execLsof throw (catch), null result, spawnError, timedOut, non-zero exit, and missing exit (?? 1). - port-validation arms: port < 1, port > 65535, and !Number.isInteger (a 400-digit port overflows to Infinity). - resolveSelfHostedIosPort probe with no iosPortCache (if-cache false arm). - lsof finds a candidate port but probing it also fails (if(hit) false arm). Row format matches the lsof column layout the parser expects (NAME at cols[8]) — the DEVICE column is required for the port to be parsed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/test/unit/maestro-hierarchy.test.js | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/packages/core/test/unit/maestro-hierarchy.test.js b/packages/core/test/unit/maestro-hierarchy.test.js index 4d08b5254..4793af47f 100644 --- a/packages/core/test/unit/maestro-hierarchy.test.js +++ b/packages/core/test/unit/maestro-hierarchy.test.js @@ -776,6 +776,72 @@ describe('Unit / maestro-hierarchy', () => { // Sanity: first call probed exactly once (7001 hit) — no range/lsof. expect(firstCallCount).toBe(1); }); + + it('probe-7001 hit without an iosPortCache: resolves hierarchy, nothing to cache', async () => { + // Covers the `if (iosPortCache)` false arm in the probe helper — a + // caller that doesn't thread a cache still resolves normally. + const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(successfulHttpResponse); + const execLsof = jasmine.createSpy('execLsof'); + + const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof }); + + expect(res.kind).toBe('hierarchy'); + expect(httpRequest.calls.first().args[0].port).toBe(7001); + expect(execLsof).not.toHaveBeenCalled(); + }); + + it('lsof failure modes (throw / null / spawnError / timedOut / non-zero & missing exit) all warn-skip', async () => { + // Covers every arm of lsofXctrunnerPort's result guard (and the catch). + const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); + const failureModes = [ + () => { throw new Error('lsof spawn failed'); }, // catch + async () => null, // !result + async () => ({ spawnError: true }), // spawnError + async () => ({ timedOut: true }), // timedOut + async () => ({ stdout: '', exitCode: 1 }), // exitCode !== 0 + async () => ({ stdout: '' }) // exitCode undefined → (?? 1) !== 0 + ]; + for (const execLsof of failureModes) { + const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache: { port: null } }); + expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); + } + }); + + it('lsof rows with invalid ports (0, out-of-range, overflow→Infinity) are all skipped', async () => { + // Covers each arm of the port-validation guard: port < 1, port > 65535, + // and !Number.isInteger (a 400-digit port overflows to Infinity). + const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); + const bigPort = '9'.repeat(400); + const lsofStdout = [ + 'COMMAND PID USER FD TYPE DEVICE NAME', + 'dev.mobile.maestro-driver-iosUITests.xctrunner 1 user 8u IPv4 0x1 0t0 TCP *:0 (LISTEN)', + 'dev.mobile.maestro-driver-iosUITests.xctrunner 2 user 8u IPv4 0x1 0t0 TCP *:99999 (LISTEN)', + `dev.mobile.maestro-driver-iosUITests.xctrunner 3 user 8u IPv4 0x1 0t0 TCP *:${bigPort} (LISTEN)`, + '' + ].join('\n'); + const execLsof = async () => ({ stdout: lsofStdout, stderr: '', exitCode: 0 }); + + const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache: { port: null } }); + + expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); + }); + + it('lsof finds a port but probing it also fails → warn-skip (hit falsy)', async () => { + // Covers the `if (hit) return hit` false arm: lsof yields a candidate, + // but the probe of that port fails too, so the cascade ends unresolved. + const lsofPort = 51234; + const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); + const lsofStdout = `COMMAND PID USER FD TYPE DEVICE NAME\ndev.mobile.maestro-driver-iosUITests.xctrunner 1 user 8u IPv4 0x1 0t0 TCP *:${lsofPort} (LISTEN)\n`; + const execLsof = jasmine.createSpy('execLsof').and.resolveTo({ stdout: lsofStdout, stderr: '', exitCode: 0 }); + const iosPortCache = { port: null }; + + const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache }); + + expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); + // Probed 7001 then the lsof candidate; both failed. + expect(httpRequest.calls.allArgs().map(a => a[0].port)).toEqual([7001, lsofPort]); + expect(iosPortCache.port).toBeNull(); + }); }); describe('iOS HTTP dump (runIosHttpDump primary path)', () => { From 05b2295b3776f82739067747739089f76cbac424 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Thu, 4 Jun 2026 14:10:44 +0530 Subject: [PATCH 12/22] test(core): fix lsof multi-match test row format to cover the >1 branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing "lsof multi-match" test's rows omitted the DEVICE column, so the NAME (`*:port`) landed at cols[7] instead of cols[8] and the ports were never parsed — `matches` never exceeded 1, leaving the `matches.size > 1` guard (maestro-hierarchy.js:1296) uncovered. Add the column so both ports parse and the multi-match refuse-to-guess branch is actually exercised. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/test/unit/maestro-hierarchy.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/test/unit/maestro-hierarchy.test.js b/packages/core/test/unit/maestro-hierarchy.test.js index 4793af47f..926d4d2cb 100644 --- a/packages/core/test/unit/maestro-hierarchy.test.js +++ b/packages/core/test/unit/maestro-hierarchy.test.js @@ -693,8 +693,8 @@ describe('Unit / maestro-hierarchy', () => { const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); const lsofStdout = [ 'COMMAND PID USER FD TYPE DEVICE NAME', - 'dev.mobile.maestro-driver-iosUITests.xctrunner 100 user 8u IPv4 0t0 TCP *:51234 (LISTEN)', - 'dev.mobile.maestro-driver-iosUITests.xctrunner 101 user 8u IPv4 0t0 TCP *:51235 (LISTEN)', + 'dev.mobile.maestro-driver-iosUITests.xctrunner 100 user 8u IPv4 0x1 0t0 TCP *:51234 (LISTEN)', + 'dev.mobile.maestro-driver-iosUITests.xctrunner 101 user 8u IPv4 0x1 0t0 TCP *:51235 (LISTEN)', '' ].join('\n'); const execLsof = async () => ({ stdout: lsofStdout, stderr: '', exitCode: 0 }); From d6a88d6eb0720b6d42e0538d24c2b52d89da9a98 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Thu, 4 Jun 2026 14:32:50 +0530 Subject: [PATCH 13/22] test(core): cover the no-port lsof row branch (maestro-hierarchy.js:1291) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixing the multi-match test's row format (so its ports parse) moved coverage off the `!m` arm of the name-match guard — the broken rows had been the only thing exercising "row has no :port → skip". Add an explicit non-socket FD row (cwd/DIR, no :port) so both the multi-match (>1) and the no-port (!m) branches are covered. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/test/unit/maestro-hierarchy.test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/core/test/unit/maestro-hierarchy.test.js b/packages/core/test/unit/maestro-hierarchy.test.js index 926d4d2cb..3fb03269f 100644 --- a/packages/core/test/unit/maestro-hierarchy.test.js +++ b/packages/core/test/unit/maestro-hierarchy.test.js @@ -842,6 +842,23 @@ describe('Unit / maestro-hierarchy', () => { expect(httpRequest.calls.allArgs().map(a => a[0].port)).toEqual([7001, lsofPort]); expect(iosPortCache.port).toBeNull(); }); + + it('lsof xctrunner row whose NAME has no port (non-socket FD) is skipped', async () => { + // The maestro-driver process also holds non-socket FDs (cwd/DIR, files); + // those NAME columns have no `:port`, so the row is skipped (the `!m` + // arm of the name-match guard). + const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); + const lsofStdout = [ + 'COMMAND PID USER FD TYPE DEVICE NAME', + 'dev.mobile.maestro-driver-iosUITests.xctrunner 1 user cwd DIR 1,2 0 12 /private/var/containers', + '' + ].join('\n'); + const execLsof = async () => ({ stdout: lsofStdout, stderr: '', exitCode: 0 }); + + const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache: { port: null } }); + + expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); + }); }); describe('iOS HTTP dump (runIosHttpDump primary path)', () => { From 1487d9a155b09f9d25a7261e76241578f738ec37 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Tue, 9 Jun 2026 22:49:45 +0530 Subject: [PATCH 14/22] =?UTF-8?q?refactor(core,cli-exec):=20address=20Rish?= =?UTF-8?q?i=20review=20=E2=80=94=20drop=20runtime=20field=20+=20PERCY=5FC?= =?UTF-8?q?LI=5FAPI=20alias?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - core: simplify self-hosted detection to selfHosted = !sessionId (drop the SDK-supplied `runtime` field, its parsing/validation, and the two consistency-debug logs). sessionId-presence is the canonical BS-vs-self-hosted signal; the runtime field added no value over it. - core/test: remove the 9-test `runtime field gating` describe block. - cli-exec: remove the unused `env.PERCY_CLI_API` export (no consumer in the codebase; YAGNI). - cli-exec/test: drop the `PERCY_CLI_API` assertion test. - cli-doctor/test: drop `PERCY_CLI_API` from the clean-env fixture. Addresses PR #2261 inline comments by @rishigupta1599 on packages/core/src/api.js:458 and packages/cli-exec/src/exec.js:89. Co-Authored-By: Claude Opus 4.7 --- packages/cli-doctor/test/env-audit.test.js | 1 - packages/cli-exec/src/exec.js | 1 - packages/cli-exec/test/exec.test.js | 17 ---- packages/core/src/api.js | 35 +------- packages/core/test/api.test.js | 97 ---------------------- 5 files changed, 4 insertions(+), 147 deletions(-) diff --git a/packages/cli-doctor/test/env-audit.test.js b/packages/cli-doctor/test/env-audit.test.js index cf661fd3d..73d84776e 100644 --- a/packages/cli-doctor/test/env-audit.test.js +++ b/packages/cli-doctor/test/env-audit.test.js @@ -62,7 +62,6 @@ const CLEAN_PERCY_ENV = { PERCY_CHROMIUM_BASE_URL: undefined, PERCY_PAC_FILE_URL: undefined, PERCY_CLIENT_API_URL: undefined, - PERCY_CLI_API: undefined, PERCY_SERVER_ADDRESS: undefined, PERCY_SERVER_HOST: undefined, PERCY_COMMIT: undefined, diff --git a/packages/cli-exec/src/exec.js b/packages/cli-exec/src/exec.js index 81bfa89de..be4850e86 100644 --- a/packages/cli-exec/src/exec.js +++ b/packages/cli-exec/src/exec.js @@ -86,7 +86,6 @@ export const exec = command('exec', { // provide SDKs with useful env vars env.PERCY_SERVER_ADDRESS = percy?.address(); - env.PERCY_CLI_API = percy?.address(); env.PERCY_BUILD_ID = percy?.build?.id; env.PERCY_BUILD_URL = percy?.build?.url; env.PERCY_LOGLEVEL = log.loglevel(); diff --git a/packages/cli-exec/test/exec.test.js b/packages/cli-exec/test/exec.test.js index 592b561d1..792fa72c7 100644 --- a/packages/cli-exec/test/exec.test.js +++ b/packages/cli-exec/test/exec.test.js @@ -377,21 +377,4 @@ describe('percy exec', () => { '[percy] Finalized build #1: https://percy.io/test/test/123' ])); }); - - it('provides the child process with a PERCY_CLI_API env var matching the server address', async () => { - await exec(['--port=4567', '--', 'node', '--eval', ( - 'process.env.PERCY_CLI_API === process.env.PERCY_SERVER_ADDRESS ' + - '&& process.env.PERCY_CLI_API === "http://localhost:4567" || process.exit(2)' - )]); - - expect(logger.stderr).toEqual(jasmine.arrayContaining([ - '[percy] Detected error for percy build', - '[percy] Failure: Snapshot command was not called' - ])); - expect(logger.stdout).toEqual(jasmine.arrayContaining([ - '[percy] Percy has started!', - jasmine.stringMatching('\\[percy] Running "node '), - '[percy] Finalized build #1: https://percy.io/test/test/123' - ])); - }); }); diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 5513bb5b9..bc0af0fef 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -422,40 +422,13 @@ export function createPercyServer(percy, port) { // post a comparison by reading a Maestro screenshot from disk .route('post', '/percy/maestro-screenshot', async (req, res) => { /* istanbul ignore next — req.body falsy guard; tests always pass a body. */ - let { name, sessionId, runtime } = req.body || {}; + let { name, sessionId } = req.body || {}; if (!name) throw new ServerError(400, 'Missing required field: name'); - // Validate `runtime` if present: must be a string, but unknown values - // are NOT rejected — we treat anything other than "selfhosted" / - // "browserstack" as "fall back to sessionId-absent detection". This - // is defensive against future runtime values (maestro-cloud, - // saucelabs, etc.) the SDK might introduce ahead of relay support. - if (runtime !== undefined && typeof runtime !== 'string') { - throw new ServerError(400, 'Invalid runtime: must be a string'); - } - - // Self-hosted detection — prefer the SDK's explicit declaration when - // present, fall back to sessionId absence (the implicit signal used - // by SDKs that predate the runtime field). The fallback is what - // ships in cli#2261 today; the explicit runtime field arrives with - // @percy/maestro-app >= v1.0.0-Beta.1 (cf. percy-maestro-app#7+R2). - // - // `runtime === 'browserstack'` always means BS. Anything else that is - // NOT 'selfhosted' (absent OR an unknown future value like - // 'maestro-cloud') falls back to sessionId-absence detection — hence - // `runtime !== 'browserstack' && !sessionId`, not `!runtime && !sessionId` - // (the latter wrongly routed unknown non-empty runtimes to the BS branch). - let selfHosted = (runtime === 'selfhosted') || (runtime !== 'browserstack' && !sessionId); - - // Diagnostic surface for any inconsistency between the two signals - // (debug-only — never breaks the request). - if (runtime === 'browserstack' && !sessionId) { - percy.log.debug('maestro-screenshot: runtime=browserstack but sessionId missing — trusting runtime field'); - } - if (runtime === 'selfhosted' && sessionId) { - percy.log.debug('maestro-screenshot: runtime=selfhosted but sessionId present — trusting runtime field'); - } + // Self-hosted vs BrowserStack is signaled by sessionId presence: BS + // host-injection always supplies it; self-hosted runs never do. + let selfHosted = !sessionId; // Strict character-class validation — rejects path separators, shell metacharacters, // NUL, newlines, and anything else that could confuse the glob or the filesystem. diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index 2669e9486..071904b67 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -1261,103 +1261,6 @@ describe('API Server', () => { .toBeRejectedWithError(/Invalid platform: must be a string/); }); - describe('runtime field gating', () => { - // The relay's self-hosted detection prefers an explicit `runtime` field - // from the SDK when present, falling back to sessionId-absence for older - // SDKs. These tests cover the eight-way matrix of (runtime ∈ - // {undefined, "selfhosted", "browserstack", "unknown"}) × (sessionId ∈ - // {present, absent}) — plus the type-validation 400. - - it('rejects non-string runtime type with 400', async () => { - await percy.start(); - await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, runtime: 123 })) - .toBeRejectedWithError(/Invalid runtime: must be a string/); - }); - - it('runtime=selfhosted + no sessionId → self-hosted branch (400s on missing env)', async () => { - let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - try { - await percy.start(); - await expectAsync(postMaestro({ name: SS_NAME, runtime: 'selfhosted' })) - .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); - } finally { - if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; - } - }); - - it('runtime=selfhosted + sessionId PRESENT → still self-hosted branch (trust runtime field)', async () => { - // Bidirectional-consistency case: SDK explicitly says self-hosted but - // a sessionId leaked into the payload. Trust the SDK's declaration. - let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - try { - await percy.start(); - await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'selfhosted' })) - .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); - } finally { - if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; - } - }); - - it('runtime=browserstack + sessionId present → BS branch finds file under /tmp//...', async () => { - await percy.start(); - const res = await postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'browserstack' }); - expect(res.success).toBe(true); - }); - - it('runtime=browserstack + sessionId ABSENT → BS branch (trust runtime), but file-find fails → 404', async () => { - // Bidirectional-consistency case: SDK explicitly says browserstack - // but sessionId is missing. BS branch runs, can't locate the file - // because `/tmp//...` doesn't resolve. - await percy.start(); - await expectAsync(postMaestro({ name: SS_NAME, runtime: 'browserstack' })) - .toBeRejectedWithError(/Screenshot not found|sessionId/i); - }); - - it('runtime=unknown-value + no sessionId → falls back to sessionId-absent → self-hosted branch', async () => { - // Defensive: unknown runtime values are NOT rejected. The relay falls - // back to sessionId-absence detection, so this lands in self-hosted. - let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - try { - await percy.start(); - await expectAsync(postMaestro({ name: SS_NAME, runtime: 'maestro-cloud' })) - .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); - } finally { - if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; - } - }); - - it('runtime=unknown-value + sessionId present → falls back to sessionId-present → BS branch', async () => { - await percy.start(); - const res = await postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'maestro-cloud' }); - expect(res.success).toBe(true); - }); - - it('no runtime + sessionId present → BS branch (backward-compat with pre-runtime SDKs)', async () => { - await percy.start(); - const res = await postMaestro({ name: SS_NAME, sessionId: SID }); - expect(res.success).toBe(true); - }); - - it('no runtime + no sessionId → self-hosted branch (backward-compat with pre-runtime SDKs)', async () => { - let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - try { - await percy.start(); - await expectAsync(postMaestro({ name: SS_NAME })) - .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); - } finally { - if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; - } - }); - }); - it('rejects non-object element selector with 400', async () => { await percy.start(); await expectAsync(postMaestro({ From 13616a877e85b8d6fe4509d68c4a2bb9b3f58703 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Thu, 11 Jun 2026 17:45:18 +0530 Subject: [PATCH 15/22] feat(cli-app): prescribe iOS Maestro driver port via --driver-host-port injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @percy/cli-app's `app:exec` callback now auto-injects --driver-host-port into `maestro test` argv alongside the existing -e PERCY_SERVER and --test-output-dir injections. The chosen port is mirrored into process.env.PERCY_IOS_DRIVER_HOST_PORT so the @percy/core relay reads the same value when resolving iOS element regions. Resolution order in maybeInjectDriverHostPort: 1. Argv already has --driver-host-port → no-op (customer override) 2. Sharded test run detected (--shards, -s, --shard-split, --shard-all) → no-op. Per Maestro's TestCommand.kt#selectPort, each shard calls selectPort() independently; injecting a single port would error shard 2+ with "Requested driver host port N is not available". 3. PERCY_IOS_DRIVER_HOST_PORT env set to a valid int 1-65535 → inject that value. Preserves BS-host injection and self-hosted pinning. 4. Otherwise → pickFreePort() via net.createServer().listen(0). Same channel Maestro itself uses (ServerSocket(0)). Mirror the chosen value to process.env. Companion change in @percy/core removes the now-unnecessary probe/lsof self-discovery cascade. Refs: docs/plans/2026-06-11-001-refactor-prescribe-driver-host-port-plan.md Co-Authored-By: Claude Opus 4.7 --- packages/cli-app/src/exec.js | 141 ++++++++++++++++++- packages/cli-app/src/index.js | 2 +- packages/cli-app/test/exec.test.js | 211 ++++++++++++++++++++++++++++- 3 files changed, 345 insertions(+), 9 deletions(-) diff --git a/packages/cli-app/src/exec.js b/packages/cli-app/src/exec.js index 540d7b9d9..f2c99e696 100644 --- a/packages/cli-app/src/exec.js +++ b/packages/cli-app/src/exec.js @@ -1,4 +1,5 @@ import fs from 'fs'; +import net from 'net'; import os from 'os'; import path from 'path'; import command from '@percy/cli-command'; @@ -127,6 +128,131 @@ export function maybeInjectScreenshotDir(ctx, log) { if (!flagSet) args.splice(2, 0, '--test-output-dir', resolved); } +// True when argv contains a `--driver-host-port` flag the customer already +// supplied. Scans the full argv slice past argv[2] (where flow files live) +// because customers can pass the flag at any position. +function hasExistingDriverHostPortFlag(args) { + for (let i = 2; i < args.length; i++) { + if (args[i] === '--driver-host-port') return true; + } + return false; +} + +// True when argv contains a Maestro sharding flag. Per Maestro's +// TestCommand.kt#selectPort, each shard calls selectPort() independently — +// if --driver-host-port N is set, shard 1 binds N and shards 2+ fail with +// `CliError("Requested driver host port N is not available")`. So when the +// customer is running sharded, we MUST NOT inject a single port. Sharded +// runs without our inject fall through to Maestro's ServerSocket(0) per- +// shard port assignment, which works fine (status quo). +// +// Matches `--shards N`, `-s N` (deprecated short form), `--shard-split N`, +// `--shard-all N`. Conservative: any flag form gates us out. +function hasShardingFlag(args) { + for (let i = 2; i < args.length; i++) { + if (args[i] === '--shards' || args[i] === '-s' || + args[i] === '--shard-split' || args[i] === '--shard-all') { + return true; + } + } + return false; +} + +// Ask the OS for a free TCP port via Node's `net` module. Mirrors Maestro's +// own `ServerSocket(0)` strategy in TestCommand.kt#selectPort. Binds, reads +// the assigned port, closes immediately. Linux and macOS kernels do NOT +// immediately re-assign released ephemeral ports — the reservation window +// is tens of seconds, comfortably longer than the ~1s gap before Maestro's +// own bind. The TOCTOU race is theoretical, not realistic; if it ever +// fires, Maestro errors loudly with "Requested driver host port N is not +// available" which surfaces through `percy app:exec`'s stderr forwarding. +function pickFreePort() { + return new Promise((resolve, reject) => { + const server = net.createServer(); + server.unref(); + server.once('error', reject); + server.listen(0, '127.0.0.1', () => { + const port = server.address()?.port; + server.close(() => { + if (Number.isInteger(port) && port > 0) resolve(port); + /* istanbul ignore next — defensive guard; Node's listen(0) on an + available interface returns an integer port on every supported + OS. This branch is only reachable if address() returns null + between listen and close, which we have never observed. */ + else reject(new Error('pickFreePort: invalid port from net.createServer')); + }); + }); + }); +} + +// Parse the candidate value for `PERCY_IOS_DRIVER_HOST_PORT` env override. +// Returns an integer in 1-65535 or null. Mirrors the validator semantics +// in @percy/core's `parseIosDriverHostPort` — both readers must agree on +// what "valid" means so customer-supplied values that the relay accepts +// are also the values we'll inject as the Maestro `--driver-host-port`. +function parseDriverHostPortEnv(raw) { + if (raw === undefined || raw === null || raw === '') return null; + const n = Number(raw); + if (!Number.isInteger(n) || n < 1 || n > 65535) return null; + return n; +} + +// Prescribe-don't-discover for the iOS driver host port. +// +// `maestro test` accepts a hidden but fully-supported `--driver-host-port` +// flag (see Maestro source `App.kt:99` and `TestCommand.kt:538-549`). When +// passed, Maestro binds the driver to exactly that port. When absent, +// Maestro uses `ServerSocket(0)` and assigns a random ephemeral port we +// could not predict from outside the process. +// +// We auto-inject `--driver-host-port ` so the relay in @percy/core +// can hit the iOS driver deterministically via `PERCY_IOS_DRIVER_HOST_PORT` +// (which we also write). This replaces the older probe-and-lsof cascade. +// +// Resolution: +// 1. Customer set `--driver-host-port` in argv → no-op (their override). +// 2. Argv contains a sharding flag → no-op. Sharded runs need per-shard +// ports; a single injected port would break shards 2+ at startup. +// 3. Customer set valid `PERCY_IOS_DRIVER_HOST_PORT` env → inject that +// value (preserves BS-host injection where the env arrives via +// `cli_manager.rb#start_percy_cli`, though that path doesn't reach +// `app:exec`; preserves self-hosted customer pinning). +// 4. Otherwise → ask the OS for a free port via `pickFreePort()` and +// write it back to `process.env.PERCY_IOS_DRIVER_HOST_PORT` so the +// @percy/core relay reads the same value. +// +// On any internal failure (extremely unlikely — only if `pickFreePort` +// throws), emit a WARN and skip the inject so the customer still gets a +// maestro test run, just without iOS element-region support. +export async function maybeInjectDriverHostPort(ctx, log) { + const args = ctx?.argv; + if (!Array.isArray(args) || args.length < 2) return; + if (path.basename(args[0]) !== 'maestro') return; + if (args[1] !== 'test') return; + if (hasExistingDriverHostPortFlag(args)) return; + if (hasShardingFlag(args)) return; + + let port = parseDriverHostPortEnv(process.env.PERCY_IOS_DRIVER_HOST_PORT); + if (port === null) { + try { + port = await pickFreePort(); + } catch (err) { + /* istanbul ignore next — pickFreePort rejection is reachable only on + systems with no free ephemeral ports, an effectively impossible + state. The WARN-and-skip path is defensive insurance. */ + log?.warn( + `Could not auto-pick a Maestro driver host port (${err?.code || err?.message || err}); ` + + '--driver-host-port not injected. iOS element regions may be unavailable. ' + + 'Set PERCY_IOS_DRIVER_HOST_PORT to a known-free port to override.' + ); + return; + } + process.env.PERCY_IOS_DRIVER_HOST_PORT = String(port); + } + + args.splice(2, 0, '--driver-host-port', String(port)); +} + export const exec = command('exec', { description: 'Start and stop Percy around a supplied command for native apps', usage: '[options] -- ', @@ -142,15 +268,16 @@ export const exec = command('exec', { skipDiscovery: true } }, async function*(ctx) { - // Order matters: maestro-server splice goes in at index 2 first, then - // screenshot-dir splice goes in at index 2 too (pushing the server flags - // to index 4). Resulting argv for `maestro test flow.yaml` becomes: - // maestro test --test-output-dir -e PERCY_SERVER= flow.yaml - // Both flag groups land between `test` and the flow file, which Maestro - // accepts. Reversing the order would still work; this order is chosen - // for log readability. + // Each helper splices at index 2, so later calls push earlier flag + // groups to higher indices. Final argv for `maestro test flow.yaml`: + // maestro test --driver-host-port --test-output-dir + // -e PERCY_SERVER= flow.yaml + // All flag groups land between `test` and the flow file, which Maestro + // accepts. The driver-host-port helper is async (it may need to ask + // the OS for a free port), so we await it; the other two are sync. maybeInjectMaestroServer(ctx, ctx.log); maybeInjectScreenshotDir(ctx, ctx.log); + await maybeInjectDriverHostPort(ctx, ctx.log); yield* ExecPlugin.default.callback(ctx); }); diff --git a/packages/cli-app/src/index.js b/packages/cli-app/src/index.js index d1c8d4ffd..b6d8d7b59 100644 --- a/packages/cli-app/src/index.js +++ b/packages/cli-app/src/index.js @@ -1,2 +1,2 @@ export { default, app } from './app.js'; -export { exec, start, stop, ping, maybeInjectMaestroServer, maybeInjectScreenshotDir } from './exec.js'; +export { exec, start, stop, ping, maybeInjectMaestroServer, maybeInjectScreenshotDir, maybeInjectDriverHostPort } from './exec.js'; diff --git a/packages/cli-app/test/exec.test.js b/packages/cli-app/test/exec.test.js index be32f1ee9..f6367e570 100644 --- a/packages/cli-app/test/exec.test.js +++ b/packages/cli-app/test/exec.test.js @@ -1,11 +1,12 @@ import fs from 'fs'; +import net from 'net'; import os from 'os'; import path from 'path'; import { setupTest } from '@percy/cli-command/test/helpers'; import * as ExecPlugin from '@percy/cli-exec'; import { exec, start, stop, ping, - maybeInjectMaestroServer, maybeInjectScreenshotDir + maybeInjectMaestroServer, maybeInjectScreenshotDir, maybeInjectDriverHostPort } from '@percy/cli-app'; describe('percy app:exec', () => { @@ -350,4 +351,212 @@ describe('percy app:exec', () => { ]); }); }); + + describe('maybeInjectDriverHostPort', () => { + // Prescribe-don't-discover: cli-app injects --driver-host-port so the + // @percy/core relay can hit the iOS Maestro driver deterministically + // via the matching PERCY_IOS_DRIVER_HOST_PORT env var. Customer overrides + // at two layers (argv flag, env var) and the sharded-run gate must all + // be honored. + + let originalEnvValue; + + beforeEach(() => { + originalEnvValue = process.env.PERCY_IOS_DRIVER_HOST_PORT; + delete process.env.PERCY_IOS_DRIVER_HOST_PORT; + }); + + afterEach(() => { + if (originalEnvValue === undefined) { + delete process.env.PERCY_IOS_DRIVER_HOST_PORT; + } else { + process.env.PERCY_IOS_DRIVER_HOST_PORT = originalEnvValue; + } + }); + + function ctxFor(argv) { + return { argv: [...argv] }; + } + + // Helper: a net.createServer stub that emits the listen callback with + // a fake "address().port" value, then succeeds on close. Mirrors the + // real shape closely enough for the helper's promise to resolve to + // `fakePort` without touching real sockets in unit tests. + function stubNetWithPort(fakePort) { + return spyOn(net, 'createServer').and.callFake(() => { + const fakeServer = { + unref() {}, + once() {}, + listen(_port, _host, cb) { setImmediate(cb); }, + address() { return { port: fakePort }; }, + close(cb) { setImmediate(cb); } + }; + return fakeServer; + }); + } + + it('injects --driver-host-port with PERCY_IOS_DRIVER_HOST_PORT env value when set', async () => { + process.env.PERCY_IOS_DRIVER_HOST_PORT = '7001'; + const created = spyOn(net, 'createServer').and.callThrough(); + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + await maybeInjectDriverHostPort(ctx); + // Did NOT call net.createServer — env value short-circuits pickFreePort + expect(created).not.toHaveBeenCalled(); + // env preserved (we use the customer value, don't rewrite it) + expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('7001'); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '--driver-host-port', '7001', 'flow.yaml' + ]); + }); + + it('picks a free port and writes env when neither env nor argv flag is set', async () => { + stubNetWithPort(54321); + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + await maybeInjectDriverHostPort(ctx); + expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('54321'); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '--driver-host-port', '54321', 'flow.yaml' + ]); + }); + + it('skips when customer already passed --driver-host-port in argv', async () => { + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['maestro', 'test', '--driver-host-port', '8000', 'flow.yaml']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBeUndefined(); + expect(ctx.argv).toEqual(argv); + }); + + it('treats invalid env values as absent and falls through to pickFreePort', async () => { + process.env.PERCY_IOS_DRIVER_HOST_PORT = 'not-a-port'; + stubNetWithPort(40000); + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + await maybeInjectDriverHostPort(ctx); + expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('40000'); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '--driver-host-port', '40000', 'flow.yaml' + ]); + }); + + it('rejects out-of-range env values (70000) and picks a free port', async () => { + process.env.PERCY_IOS_DRIVER_HOST_PORT = '70000'; + stubNetWithPort(40001); + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + await maybeInjectDriverHostPort(ctx); + expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('40001'); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '--driver-host-port', '40001', 'flow.yaml' + ]); + }); + + it('skips when argv has --shards (would break shard 2+ on a single injected port)', async () => { + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['maestro', 'test', '--shards', '3', 'flow.yaml']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBeUndefined(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when argv has -s (deprecated short form of --shards)', async () => { + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['maestro', 'test', '-s', '3', 'flow.yaml']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when argv has --shard-split', async () => { + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['maestro', 'test', '--shard-split', '3', 'flow.yaml']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when argv has --shard-all', async () => { + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['maestro', 'test', '--shard-all', '2', 'flow.yaml']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('preserves customer env value when sharded — gates on sharding, does not touch env', async () => { + process.env.PERCY_IOS_DRIVER_HOST_PORT = '7001'; + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['maestro', 'test', '--shard-split', '2', 'flow.yaml']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + // env left untouched — sharded customers must pin per-shard themselves + expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('7001'); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for `maestro hierarchy` (not a test command)', async () => { + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['maestro', 'hierarchy', '--udid', 'X']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for non-maestro commands (argv[0] is npx or other)', async () => { + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['npx', 'maestro', 'test', 'flow.yaml']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when args has fewer than two elements', async () => { + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['maestro']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + }); + + it('matches by basename when the command is an absolute path', async () => { + stubNetWithPort(50500); + const ctx = ctxFor(['/Users/foo/.maestro/bin/maestro', 'test', 'flow.yaml']); + await maybeInjectDriverHostPort(ctx); + expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('50500'); + expect(ctx.argv).toEqual([ + '/Users/foo/.maestro/bin/maestro', 'test', + '--driver-host-port', '50500', + 'flow.yaml' + ]); + }); + + it('splices at index 2 alongside sibling injections from --test-output-dir / -e PERCY_SERVER', async () => { + // Simulate post-state of sibling helpers having already injected + // their flags. New flag should land between `test` and them. + stubNetWithPort(55555); + const ctx = ctxFor([ + 'maestro', 'test', + '--test-output-dir', '/some/dir', + '-e', 'PERCY_SERVER=http://localhost:5338', + 'flow.yaml' + ]); + await maybeInjectDriverHostPort(ctx); + expect(ctx.argv).toEqual([ + 'maestro', 'test', + '--driver-host-port', '55555', + '--test-output-dir', '/some/dir', + '-e', 'PERCY_SERVER=http://localhost:5338', + 'flow.yaml' + ]); + }); + }); }); From 9f40e2313244ec6c5994c8653a43b1aa524a945d Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Thu, 11 Jun 2026 17:46:04 +0530 Subject: [PATCH 16/22] refactor(core): remove iOS port-discovery cascade after prescribe shift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With @percy/cli-app's maybeInjectDriverHostPort ensuring PERCY_IOS_DRIVER_HOST_PORT is always set for self-hosted runs (and BS hosts have always set it via cli_manager.rb#start_percy_cli), the probe-7001 + lsof self-discovery cascade in maestro-hierarchy.js is no longer needed. The criticized code is removed at the source rather than reasoned-around. Removed: - IOS_SELF_HOSTED_PROBE_PORT constant - lsofXctrunnerPort() helper - resolveSelfHostedIosPort() cascade - execLsofDefault() production default - execLsof / iosPortCache dump() parameters - iosPortCache field on Percy class - iosPortCache plumbing through api.js → maestroDump call - The entire `iOS self-hosted port cascade` describe block in maestro-hierarchy.test.js (~295 lines) - Parity test reason updated from `self-hosted-no-driver` to `env-missing` iOS dispatch now collapses to a single env-read branch: * env absent → return { kind: 'unavailable', reason: 'env-missing' } with an actionable warn. Graceful degradation — coordinate regions and the snapshot itself continue to upload via other paths. * env present → existing runIosHttpDump → runMaestroIosDump cascade runs unchanged. R7 (BrowserStack production path stays byte-identical) is preserved structurally — BS hosts always set the env var, so they only ever take the env-present branch (which is the same code as before). Addresses Rishi's review feedback on cli#2261 ("lsof might not be a good solution; probing 128 ports can be an issue") and the deepened-plan finding that Maestro 2.6+ stable's ephemeral-port behavior makes lsof a real customer concern (resolved by prescribe). Refs: docs/plans/2026-06-11-001-refactor-prescribe-driver-host-port-plan.md Co-Authored-By: Claude Opus 4.7 --- packages/core/src/api.js | 11 +- packages/core/src/maestro-hierarchy.js | 166 ++------- packages/core/src/percy.js | 6 - .../unit/maestro-hierarchy.parity.test.js | 22 +- .../core/test/unit/maestro-hierarchy.test.js | 331 +----------------- 5 files changed, 58 insertions(+), 478 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index bc0af0fef..33dd7a7b7 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -769,15 +769,14 @@ export function createPercyServer(percy, port) { if (cachedDump === null) { // Thread the per-Percy gRPC client cache so the Android gRPC // primary path can reuse channels across snapshots in the same - // session (D9 of 2026-05-07-002 plan). iOS path ignores it. - // Also thread iosPortCache so the self-hosted iOS port cascade - // (probe 7001 + lsof) resolves once per session and reuses the - // port for subsequent snapshots — same per-Percy scope. + // session (D9 of 2026-05-07-002 plan). iOS path ignores it + // (the iOS resolver reads PERCY_IOS_DRIVER_HOST_PORT directly; + // no per-session port cache needed since the port is prescribed + // upstream by `@percy/cli-app`'s `maybeInjectDriverHostPort`). cachedDump = await maestroDump({ platform, sessionId, - grpcClientCache: percy.grpcClientCache, - iosPortCache: percy.iosPortCache + grpcClientCache: percy.grpcClientCache }); } /* istanbul ignore else — branch where dump resolves to hierarchy is diff --git a/packages/core/src/maestro-hierarchy.js b/packages/core/src/maestro-hierarchy.js index 3cda11fc9..97a197a5b 100644 --- a/packages/core/src/maestro-hierarchy.js +++ b/packages/core/src/maestro-hierarchy.js @@ -101,16 +101,16 @@ const SELECTOR_KEYS_UNION = ['resource-id', 'text', 'content-desc', 'class', 'id // past the socket timeout. const IOS_HTTP_HEALTHY_DEADLINE_MS = 1500; const IOS_HTTP_CIRCUIT_BREAKER_MS = 5000; -// BS realmobile derives the iOS driver-host-port as wda_port + 2700 (WDA -// ports 8400-8410 → driver host ports 11100-11110). The validator -// `parseIosDriverHostPort` accepts any valid TCP port (1-65535); the BS -// range is a strict subset. Self-hosted iOS port resolution probes -// `127.0.0.1:7001` first — source-verified on Maestro 1.40-2.4.0 (cli-2.4.0 -// `TestCommand.kt#selectPort`) as the deterministic single-simulator port; -// sharded runs use the fixed range 7001-7128. Ephemeral-port Maestro -// (`main`/2.6.0+ uses `ServerSocket(0)`) is handled by `lsof` -// self-discovery rather than range-probing. -const IOS_SELF_HOSTED_PROBE_PORT = 7001; +// `parseIosDriverHostPort` accepts any valid TCP port (1-65535). The +// BrowserStack-realmobile canonical range (11100-11110, derived as +// wda_port + 2700 from WDA ports 8400-8410) is a strict subset. Self-hosted +// callers prescribe the port via `@percy/cli-app`'s `maybeInjectDriverHostPort` +// helper, which auto-injects `--driver-host-port ` into the +// `maestro test` argv and mirrors the chosen value to +// `PERCY_IOS_DRIVER_HOST_PORT`. This relay reads the env var unconditionally +// and no longer auto-discovers the port via probe / lsof — see Phase 3 of +// the self-hosted Maestro architecture doc and brainstorm +// `2026-06-11-ios-port-discovery-simplification-requirements.md`. // HTTP response cap before parse — sized for WebView-heavy iOS apps. const IOS_HTTP_RESPONSE_MAX_BYTES = 20 * 1024 * 1024; @@ -1251,110 +1251,11 @@ async function runAdbFallback(serial, execAdb) { } // ===== Self-hosted iOS port resolution helpers ===== -// Used only on the implicit branch of the iOS dispatch — when -// PERCY_IOS_DRIVER_HOST_PORT is absent, the resolver auto-discovers the -// running Maestro iOS driver's host port via deterministic probe + lsof. -// The BS path (explicit env vars present) does not invoke these. - -// Production default for `execLsof`. Tests inject a fake. The 5s timeout -// is generous — lsof is local and listing TCP LISTEN sockets is fast; the -// budget is mostly defense against a hung lsof. Returns the spawn result -// in the shape spawnWithTimeout produces. -/* istanbul ignore next — child-process spawn wrapper; tests inject a fake. */ -async function execLsofDefault({ timeoutMs } = {}) { - return spawnWithTimeout('lsof', ['-nP', '-iTCP', '-sTCP:LISTEN'], { timeoutMs: timeoutMs ?? 5000 }); -} - -// Parse `lsof -nP -iTCP -sTCP:LISTEN` output and return the LISTEN port of -// the Maestro iOS XCTest runner — ONLY when exactly one matching listener -// is found. Zero matches, multiple matches, or any spawn/parse failure -// returns null so the caller falls through to warn-skip rather than -// guessing. Match criterion: process name (column 1, case-insensitive -// substring) contains `maestro-driver-ios` (covers names like -// `dev.mobile.maestro-driver-iosUITests.xctrunner` plus future variants). -async function lsofXctrunnerPort(execLsof) { - let result; - try { result = await execLsof({ timeoutMs: 5000 }); } catch { return null; } - if (!result || result.spawnError || result.timedOut || (result.exitCode ?? 1) !== 0) return null; - const lines = (result.stdout || '').split('\n'); - const matches = new Set(); - for (const line of lines) { - if (!line) continue; - const cols = line.split(/\s+/); - // lsof default columns: COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME... - if (cols.length < 9) continue; - if (!/maestro-driver-ios/i.test(cols[0])) continue; - // NAME column for LISTEN sockets looks like `*:7001` or `127.0.0.1:7001`, - // sometimes suffixed with `(LISTEN)`. Pull the last `:` group. - const name = cols.slice(8).join(' '); - const m = name.match(/:(\d+)(?=\D|$)/g); - if (!m || m.length === 0) continue; - const last = m[m.length - 1]; - const port = Number(last.slice(1)); - if (!Number.isInteger(port) || port < 1 || port > 65535) continue; - matches.add(port); - if (matches.size > 1) return null; // multi-match → don't guess - } - return matches.size === 1 ? [...matches][0] : null; -} - -// Self-hosted iOS port resolution cascade. Returns either -// `{ port, result }` where `result` is a `runIosHttpDump` outcome -// (caller decides what to do with it), or `null` (caller emits -// warn-skip). Order: -// 1. Cache hit → re-probe the cached port (subsequent snapshots). -// 2. Probe `127.0.0.1:7001` (deterministic single-simulator port on -// Maestro ≤2.4.0 per the source-verified spike). -// 3. `lsof` self-discovery for ephemeral-port Maestro (2.6+ uses -// `ServerSocket(0)`); exactly-one-match guard. -// 4. null → warn-skip. -// -// "Probe" reuses runIosHttpDump as both the liveness check AND the dump. -// A `hierarchy` or `no-aut-tree` result confirms the port is a valid -// Maestro driver (`no-aut-tree` means the driver is alive but the AUT -// isn't foregrounded — still a Maestro listener worth caching). -// `dump-error` and `connection-fail` are NOT cached and move to the -// next candidate; in the cascade we don't propagate schema-drift since -// the wrong service answering on `7001` would also produce `dump-error`. -async function resolveSelfHostedIosPort({ httpRequest, execLsof, sessionId, iosPortCache }) { - const probe = async port => { - const result = await runIosHttpDump({ port, sessionId, httpRequest }); - if (result.kind === 'hierarchy' || result.kind === 'no-aut-tree') { - if (iosPortCache) iosPortCache.port = port; - return { port, result }; - } - return null; - }; - - // 1. Cache hit. - if (iosPortCache && Number.isInteger(iosPortCache.port)) { - const cached = iosPortCache.port; - const result = await runIosHttpDump({ port: cached, sessionId, httpRequest }); - return { port: cached, result }; - } - - // 2. Deterministic primary. - const primary = await probe(IOS_SELF_HOSTED_PROBE_PORT); - if (primary) return primary; - - // 3. lsof self-discovery — only try its port if it's distinct from the - // primary we already probed (otherwise the earlier probe failure - // already settled it). - const lsofPort = await lsofXctrunnerPort(execLsof); - if (lsofPort !== null && lsofPort !== IOS_SELF_HOSTED_PROBE_PORT) { - const hit = await probe(lsofPort); - if (hit) return hit; - } - - // 4. Nothing resolved. - return null; -} - export async function dump(options) { /* istanbul ignore next — options-omitted default; callers always pass an object (tests inject every dependency; production code binds them). */ options = options || {}; - let { platform, sessionId, execAdb, execMaestro, httpRequest, grpcClient, grpcClientCache, getEnv, execLsof, iosPortCache } = options; + let { platform, sessionId, execAdb, execMaestro, httpRequest, grpcClient, grpcClientCache, getEnv } = options; /* istanbul ignore next — defaults applied only when caller omits the corresponding key; tests inject every dependency, production callers bind these from defaults at runtime. */ @@ -1369,44 +1270,29 @@ export async function dump(options) { grpcClient = grpcClient || defaultGrpcClientFactory; /* istanbul ignore next */ getEnv = getEnv || defaultGetEnv; - /* istanbul ignore next — execLsof default for the self-hosted iOS - cascade; tests inject a fake. iosPortCache has no default — undefined - is the "no cache" sentinel and the cascade handles it. */ - execLsof = execLsof || execLsofDefault; const started = Date.now(); if (platform === 'ios') { const udid = getEnv('PERCY_IOS_DEVICE_UDID'); const driverHostPortRaw = getEnv('PERCY_IOS_DRIVER_HOST_PORT'); - // ─── Self-hosted (implicit) — PERCY_IOS_DRIVER_HOST_PORT absent ──────── - // Cascade-discover the running Maestro iOS driver's host port: cache → - // probe 127.0.0.1:7001 → lsof self-discovery → warn-skip. No CLI cold- - // start tier (that was Tier B from the original plan, cut after the - // spike confirmed `7001` is deterministic on Maestro ≤2.4.0 + lsof - // covers the ephemeral-port case). UDID is not required here — the - // HTTP `/viewHierarchy` POST doesn't take a udid; the driver host is - // already bound to one device. + // `PERCY_IOS_DRIVER_HOST_PORT` is the single source of truth for the + // iOS Maestro driver port. It is set by: + // * BrowserStack hosts (canonical 11100-11110 range, via + // `cli_manager.rb#start_percy_cli` env injection) + // * `@percy/cli-app`'s `maybeInjectDriverHostPort` helper for + // self-hosted customers running `percy app:exec` (any port, + // either customer-supplied or picked by the OS at startup) + // * Manually by power users who pin the driver port themselves + // + // When the env var is absent, the customer skipped `percy app:exec` + // (or BS host injection failed). Element regions degrade gracefully + // with `env-missing` — coordinate regions and the snapshot itself + // continue to upload via the relay's other paths. if (!driverHostPortRaw) { - const cascade = await resolveSelfHostedIosPort({ httpRequest, execLsof, sessionId, iosPortCache }); - if (cascade) { - const { port, result } = cascade; - if (result.kind === 'hierarchy') { - log.debug(`dump took ${Date.now() - started}ms via maestro-http (self-hosted, port=${port}, ${result.nodes.length} nodes)`); - recordResolverSuccess({ platform: 'ios', via: 'maestro-http' }); - } else { - // `no-aut-tree` from a resolved port — Maestro driver alive (port - // cached) but the AUT isn't foregrounded for THIS snapshot. - // Subsequent snapshots in the session re-probe the cached port - // and may succeed. - log.debug(`dump took ${Date.now() - started}ms via maestro-http (self-hosted, port=${port}, ${result.kind}/${result.reason})`); - recordResolverFinalFailure({ platform: 'ios', failureClass: failureClassFromReason(result.reason) }); - } - return result; - } - log.warn('[percy] iOS element regions: no Maestro driver found on :7001 or via lsof. Set PERCY_IOS_DRIVER_HOST_PORT (real devices use a forwarded port; sharded/newer Maestro may use a different port).'); + log.warn('[percy] iOS element regions unavailable — PERCY_IOS_DRIVER_HOST_PORT is not set. Run `percy app:exec -- maestro test ...` so it is injected automatically, or set it manually to your Maestro driver-host-port.'); recordResolverFinalFailure({ platform: 'ios', failureClass: 'other' }); - return { kind: 'unavailable', reason: 'self-hosted-no-driver' }; + return { kind: 'unavailable', reason: 'env-missing' }; } // ─── Explicit — PERCY_IOS_DRIVER_HOST_PORT present ───────────────────── diff --git a/packages/core/src/percy.js b/packages/core/src/percy.js index 29c90d9b5..2878299c5 100644 --- a/packages/core/src/percy.js +++ b/packages/core/src/percy.js @@ -145,12 +145,6 @@ export class Percy { this.grpcClientCache = new Map(); this.grpcClientCache.shutdownInProgress = false; - // Self-hosted iOS port cache — mirrors the per-`Percy`-instance scope of - // grpcClientCache (D9 of the maestro 4-PR plan). Set on the first - // successful self-hosted iOS port resolution and reused for the rest of - // the session so subsequent snapshots skip the probe/lsof cascade. - this.iosPortCache = { port: null }; - // Domain validation state for auto domain allow-listing this.domainValidation = { autoConfiguredHosts: new Set(), // Domains from project config diff --git a/packages/core/test/unit/maestro-hierarchy.parity.test.js b/packages/core/test/unit/maestro-hierarchy.parity.test.js index b7eee3e0b..923d7b1ed 100644 --- a/packages/core/test/unit/maestro-hierarchy.parity.test.js +++ b/packages/core/test/unit/maestro-hierarchy.parity.test.js @@ -89,19 +89,19 @@ describe('Unit / maestro-hierarchy / cross-platform parity', () => { expect(res.nodes.length).toBeGreaterThan(0); }); - it('iOS env-missing returns { kind: "unavailable", reason: "self-hosted-no-driver" }', async () => { + it('iOS env-missing returns { kind: "unavailable", reason: "env-missing" }', async () => { // Same envelope shape as Android-failure paths, just a different reason - // tag. Post-Unit-2: with both PERCY_IOS_* env vars absent, the - // dispatch enters the self-hosted IMPLICIT branch and runs the port - // cascade (probe 7001 → lsof). With injected fakes that fail at every - // tier, the cascade returns null and the dispatch emits the new - // `self-hosted-no-driver` warn-skip reason. The envelope shape (kind - // = 'unavailable') matches Android-failure paths unchanged. - const httpRequest = async () => { throw Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); }; - const execLsof = async () => ({ stdout: '', stderr: '', exitCode: 0 }); - const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof }); + // tag. Post-Phase-3 (prescribe-don't-discover): the port-discovery + // cascade was removed. PERCY_IOS_DRIVER_HOST_PORT is now the single + // source of truth — set by BS hosts, by @percy/cli-app's + // `maybeInjectDriverHostPort`, or manually by power users. When + // absent the dispatch returns env-missing and the snapshot continues + // to upload via other paths; element regions degrade gracefully. + const httpRequest = jasmine.createSpy('httpRequest'); + const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest }); expect(res.kind).toBe('unavailable'); - expect(res.reason).toBe('self-hosted-no-driver'); + expect(res.reason).toBe('env-missing'); + expect(httpRequest).not.toHaveBeenCalled(); }); it('iOS env-set with no http/maestro reachable returns same envelope kinds as Android failure paths', async () => { diff --git a/packages/core/test/unit/maestro-hierarchy.test.js b/packages/core/test/unit/maestro-hierarchy.test.js index 3fb03269f..aa8f92543 100644 --- a/packages/core/test/unit/maestro-hierarchy.test.js +++ b/packages/core/test/unit/maestro-hierarchy.test.js @@ -491,27 +491,28 @@ describe('Unit / maestro-hierarchy', () => { expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-udid' }); }); - // Self-hosted (UDID-set, PORT-unset): enters the IMPLICIT branch and - // runs the discovery cascade (probe 7001 → lsof). With both injected - // fakes failing, cascade returns null → warn-skip with the new - // `self-hosted-no-driver` reason. UDID being set is irrelevant on the - // implicit path — HTTP `/viewHierarchy` doesn't take a udid. - it('warn-skips with self-hosted-no-driver when port is unset and the discovery cascade finds nothing', async () => { + // Self-hosted (UDID-set, PORT-unset): the prescribe-don't-discover refactor + // (Phase 3) removed the port-discovery cascade. PERCY_IOS_DRIVER_HOST_PORT + // is now the single source of truth. When it is absent the relay returns + // env-missing and the snapshot continues to upload via other paths; + // element regions degrade gracefully. UDID presence/absence is irrelevant + // when the port env is unset — without a port there is nothing to call. + it('warn-skips with env-missing when port is unset (udid set)', async () => { const getEnv = key => { if (key === 'PERCY_IOS_DEVICE_UDID') return '00008110-000065081404401E'; return undefined; }; - const httpRequest = async () => { throw Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); }; - const execLsof = async () => ({ stdout: '', stderr: '', exitCode: 0 }); - const res = await dump({ platform: 'ios', getEnv, httpRequest, execLsof }); - expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); + const httpRequest = jasmine.createSpy('httpRequest'); + const res = await dump({ platform: 'ios', getEnv, httpRequest }); + expect(res).toEqual({ kind: 'unavailable', reason: 'env-missing' }); + expect(httpRequest).not.toHaveBeenCalled(); }); - it('warn-skips with self-hosted-no-driver when both env vars are unset and the cascade finds nothing', async () => { - const httpRequest = async () => { throw Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); }; - const execLsof = async () => ({ stdout: '', stderr: '', exitCode: 0 }); - const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof }); - expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); + it('warn-skips with env-missing when both env vars are unset', async () => { + const httpRequest = jasmine.createSpy('httpRequest'); + const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest }); + expect(res).toEqual({ kind: 'unavailable', reason: 'env-missing' }); + expect(httpRequest).not.toHaveBeenCalled(); }); it('does not invoke adb on iOS dispatch', async () => { @@ -561,306 +562,6 @@ describe('Unit / maestro-hierarchy', () => { }); }); - // Self-hosted iOS path: triggered when PERCY_IOS_DRIVER_HOST_PORT is - // absent. The resolver auto-discovers the running Maestro driver port - // via probe 127.0.0.1:7001 → lsof → warn-skip. The BS path (explicit - // env vars) does not exercise this code at all. - describe('iOS self-hosted port cascade', () => { - // Minimal axElement response matching the existing iOS HTTP fixture - // shape — single AUT root, one button child with a frame. Enough for - // runIosHttpDump to return { kind: 'hierarchy' }. - const minimalAxElementJson = JSON.stringify({ - axElement: { - elementType: 1, - identifier: 'com.example.app', - frame: { X: 0, Y: 0, Width: 100, Height: 100 }, - children: [ - { elementType: 9, identifier: 'btn', label: 'OK', frame: { X: 10, Y: 10, Width: 50, Height: 30 } } - ] - } - }); - - const successfulHttpResponse = { - statusCode: 200, - headers: { 'content-type': 'application/json' }, - body: minimalAxElementJson - }; - - const connectionRefused = () => Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); - - it('probe-7001 hit: cascade returns hierarchy and caches port 7001', async () => { - const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(successfulHttpResponse); - const execLsof = jasmine.createSpy('execLsof'); - const iosPortCache = { port: null }; - - const res = await dump({ - platform: 'ios', - getEnv: () => undefined, - httpRequest, - execLsof, - iosPortCache - }); - - expect(res.kind).toBe('hierarchy'); - expect(res.nodes.length).toBeGreaterThan(0); - // Probed exactly :7001; never invoked lsof (probe succeeded first). - expect(httpRequest.calls.count()).toBe(1); - expect(httpRequest.calls.first().args[0].port).toBe(7001); - expect(execLsof).not.toHaveBeenCalled(); - // Cache populated. - expect(iosPortCache.port).toBe(7001); - }); - - it('probe-7001 no-aut-tree: driver alive but AUT not foregrounded — caches port, returns no-aut-tree', async () => { - // The resolver caches the port for hierarchy OR no-aut-tree (the driver - // is alive either way). dump()'s self-hosted branch then surfaces the - // non-hierarchy result instead of treating it as "no driver found". - const springboardResponse = { - statusCode: 200, - headers: { 'content-type': 'application/json' }, - body: fs.readFileSync(path.resolve( - url.fileURLToPath(import.meta.url), - '../../fixtures/maestro-ios-hierarchy/viewHierarchy-response-springboard-only.json' - ), 'utf8') - }; - const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(springboardResponse); - const execLsof = jasmine.createSpy('execLsof'); - const iosPortCache = { port: null }; - - const res = await dump({ - platform: 'ios', - getEnv: () => undefined, - httpRequest, - execLsof, - iosPortCache - }); - - expect(res.kind).toBe('no-aut-tree'); - expect(httpRequest.calls.first().args[0].port).toBe(7001); - // Port cached even on no-aut-tree (the driver is alive). - expect(iosPortCache.port).toBe(7001); - // Probe succeeded on :7001 → lsof never consulted. - expect(execLsof).not.toHaveBeenCalled(); - }); - - it('lsof discovery: 7001 fails, lsof returns exactly one xctrunner listener, cascade probes it', async () => { - const ephemeralPort = 51234; - const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async ({ port }) => { - if (port === 7001) throw connectionRefused(); - if (port === ephemeralPort) return successfulHttpResponse; - throw connectionRefused(); - }); - const lsofStdout = `COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME\ndev.mobile.maestro-driver-iosUITests.xctrunner 12345 user 8u IPv4 0x1 0t0 TCP *:${ephemeralPort} (LISTEN)\n`; - const execLsof = jasmine.createSpy('execLsof').and.resolveTo({ stdout: lsofStdout, stderr: '', exitCode: 0 }); - const iosPortCache = { port: null }; - - const res = await dump({ - platform: 'ios', - getEnv: () => undefined, - httpRequest, - execLsof, - iosPortCache - }); - - expect(res.kind).toBe('hierarchy'); - expect(execLsof).toHaveBeenCalled(); - // Probed 7001 first, then lsof-discovered port. - const probedPorts = httpRequest.calls.allArgs().map(args => args[0].port); - expect(probedPorts).toEqual([7001, ephemeralPort]); - expect(iosPortCache.port).toBe(ephemeralPort); - }); - - it('lsof zero matches: cascade warn-skips without guessing', async () => { - const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); - // No xctrunner row in lsof output. - const lsofStdout = 'COMMAND PID USER FD TYPE DEVICE NAME\nnode 999 user 8u IPv4 0t0 TCP *:3000 (LISTEN)\n'; - const execLsof = async () => ({ stdout: lsofStdout, stderr: '', exitCode: 0 }); - const iosPortCache = { port: null }; - - const res = await dump({ - platform: 'ios', - getEnv: () => undefined, - httpRequest, - execLsof, - iosPortCache - }); - - expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); - expect(iosPortCache.port).toBeNull(); - }); - - it('lsof multi-match (two xctrunner listeners): cascade refuses to guess and warn-skips', async () => { - const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); - const lsofStdout = [ - 'COMMAND PID USER FD TYPE DEVICE NAME', - 'dev.mobile.maestro-driver-iosUITests.xctrunner 100 user 8u IPv4 0x1 0t0 TCP *:51234 (LISTEN)', - 'dev.mobile.maestro-driver-iosUITests.xctrunner 101 user 8u IPv4 0x1 0t0 TCP *:51235 (LISTEN)', - '' - ].join('\n'); - const execLsof = async () => ({ stdout: lsofStdout, stderr: '', exitCode: 0 }); - const iosPortCache = { port: null }; - - const res = await dump({ - platform: 'ios', - getEnv: () => undefined, - httpRequest, - execLsof, - iosPortCache - }); - - expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); - expect(iosPortCache.port).toBeNull(); - }); - - it('explicit PERCY_IOS_DRIVER_HOST_PORT (out-of-legacy-range) bypasses cascade and runs HTTP primary', async () => { - // Customer pinned --driver-host-port 6001 (e.g., real-device-forwarded - // port). UDID absent — common single-device self-hosted case. The - // EXPLICIT branch runs runIosHttpDump on 6001 (relaxed range admits - // it); cascade/lsof are NOT invoked. - const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(successfulHttpResponse); - const execLsof = jasmine.createSpy('execLsof'); - const getEnv = key => (key === 'PERCY_IOS_DRIVER_HOST_PORT' ? '6001' : undefined); - - const res = await dump({ platform: 'ios', getEnv, httpRequest, execLsof }); - - expect(res.kind).toBe('hierarchy'); - expect(httpRequest.calls.count()).toBe(1); - expect(httpRequest.calls.first().args[0].port).toBe(6001); - expect(execLsof).not.toHaveBeenCalled(); - }); - - it('probe returns dump-error (wrong service): cascade does not cache and falls through', async () => { - // The probed port answers 200 but with a body missing axElement — - // runIosHttpDump returns { kind: 'dump-error', reason: 'http-missing-root' }. - // This is NOT a Maestro driver; cascade must not cache, must move on - // to lsof (which here returns no matches), and end in warn-skip with - // an empty cache. - const wrongServiceResponse = { - statusCode: 200, - headers: { 'content-type': 'application/json' }, - body: JSON.stringify({ unrelated: 'shape' }) - }; - const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(wrongServiceResponse); - const execLsof = async () => ({ stdout: '', stderr: '', exitCode: 0 }); - const iosPortCache = { port: null }; - - const res = await dump({ - platform: 'ios', - getEnv: () => undefined, - httpRequest, - execLsof, - iosPortCache - }); - - expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); - expect(iosPortCache.port).toBeNull(); - }); - - it('cache hit: a second invocation with the same iosPortCache reuses the resolved port without re-probing/re-lsof', async () => { - const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(successfulHttpResponse); - const execLsof = jasmine.createSpy('execLsof'); - const iosPortCache = { port: null }; - - // First call: cascade resolves to 7001 and caches it. - await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache }); - expect(iosPortCache.port).toBe(7001); - const firstCallCount = httpRequest.calls.count(); - - // Second call: cache hit → single HTTP to the cached port. lsof never - // invoked on either call. - httpRequest.calls.reset(); - await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache }); - expect(httpRequest.calls.count()).toBe(1); - expect(httpRequest.calls.first().args[0].port).toBe(7001); - expect(execLsof).not.toHaveBeenCalled(); - // Sanity: first call probed exactly once (7001 hit) — no range/lsof. - expect(firstCallCount).toBe(1); - }); - - it('probe-7001 hit without an iosPortCache: resolves hierarchy, nothing to cache', async () => { - // Covers the `if (iosPortCache)` false arm in the probe helper — a - // caller that doesn't thread a cache still resolves normally. - const httpRequest = jasmine.createSpy('httpRequest').and.resolveTo(successfulHttpResponse); - const execLsof = jasmine.createSpy('execLsof'); - - const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof }); - - expect(res.kind).toBe('hierarchy'); - expect(httpRequest.calls.first().args[0].port).toBe(7001); - expect(execLsof).not.toHaveBeenCalled(); - }); - - it('lsof failure modes (throw / null / spawnError / timedOut / non-zero & missing exit) all warn-skip', async () => { - // Covers every arm of lsofXctrunnerPort's result guard (and the catch). - const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); - const failureModes = [ - () => { throw new Error('lsof spawn failed'); }, // catch - async () => null, // !result - async () => ({ spawnError: true }), // spawnError - async () => ({ timedOut: true }), // timedOut - async () => ({ stdout: '', exitCode: 1 }), // exitCode !== 0 - async () => ({ stdout: '' }) // exitCode undefined → (?? 1) !== 0 - ]; - for (const execLsof of failureModes) { - const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache: { port: null } }); - expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); - } - }); - - it('lsof rows with invalid ports (0, out-of-range, overflow→Infinity) are all skipped', async () => { - // Covers each arm of the port-validation guard: port < 1, port > 65535, - // and !Number.isInteger (a 400-digit port overflows to Infinity). - const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); - const bigPort = '9'.repeat(400); - const lsofStdout = [ - 'COMMAND PID USER FD TYPE DEVICE NAME', - 'dev.mobile.maestro-driver-iosUITests.xctrunner 1 user 8u IPv4 0x1 0t0 TCP *:0 (LISTEN)', - 'dev.mobile.maestro-driver-iosUITests.xctrunner 2 user 8u IPv4 0x1 0t0 TCP *:99999 (LISTEN)', - `dev.mobile.maestro-driver-iosUITests.xctrunner 3 user 8u IPv4 0x1 0t0 TCP *:${bigPort} (LISTEN)`, - '' - ].join('\n'); - const execLsof = async () => ({ stdout: lsofStdout, stderr: '', exitCode: 0 }); - - const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache: { port: null } }); - - expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); - }); - - it('lsof finds a port but probing it also fails → warn-skip (hit falsy)', async () => { - // Covers the `if (hit) return hit` false arm: lsof yields a candidate, - // but the probe of that port fails too, so the cascade ends unresolved. - const lsofPort = 51234; - const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); - const lsofStdout = `COMMAND PID USER FD TYPE DEVICE NAME\ndev.mobile.maestro-driver-iosUITests.xctrunner 1 user 8u IPv4 0x1 0t0 TCP *:${lsofPort} (LISTEN)\n`; - const execLsof = jasmine.createSpy('execLsof').and.resolveTo({ stdout: lsofStdout, stderr: '', exitCode: 0 }); - const iosPortCache = { port: null }; - - const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache }); - - expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); - // Probed 7001 then the lsof candidate; both failed. - expect(httpRequest.calls.allArgs().map(a => a[0].port)).toEqual([7001, lsofPort]); - expect(iosPortCache.port).toBeNull(); - }); - - it('lsof xctrunner row whose NAME has no port (non-socket FD) is skipped', async () => { - // The maestro-driver process also holds non-socket FDs (cwd/DIR, files); - // those NAME columns have no `:port`, so the row is skipped (the `!m` - // arm of the name-match guard). - const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { throw connectionRefused(); }); - const lsofStdout = [ - 'COMMAND PID USER FD TYPE DEVICE NAME', - 'dev.mobile.maestro-driver-iosUITests.xctrunner 1 user cwd DIR 1,2 0 12 /private/var/containers', - '' - ].join('\n'); - const execLsof = async () => ({ stdout: lsofStdout, stderr: '', exitCode: 0 }); - - const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest, execLsof, iosPortCache: { port: null } }); - - expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-driver' }); - }); - }); - describe('iOS HTTP dump (runIosHttpDump primary path)', () => { const iosFixtureDir = path.resolve(url.fileURLToPath(import.meta.url), '../../fixtures/maestro-ios-hierarchy'); const loadIosFixture = name => fs.readFileSync(path.join(iosFixtureDir, name), 'utf8'); From a2adad6d0a343f3af79e56a05329c50edf359d56 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Sun, 14 Jun 2026 22:44:44 +0530 Subject: [PATCH 17/22] fix(cli-app): detect --flag=value equals-form for driver-host-port + sharding gates The detection helpers in cli-app's app:exec wrapper compared argv tokens exactly against `--driver-host-port`, `--shards`, `-s`, `--shard-split`, and `--shard-all`. picocli (Maestro's CLI parser) accepts the equals-form identically (`--driver-host-port=8000`, `--shards=3`, etc.), so a customer passing the equals form would bypass our gates and either: - Cause Maestro to error on duplicate `--driver-host-port` when we inject our own next to the customer's, OR - Break shard 2+ silently (each shard calls selectPort() independently; if our injected port collides, Maestro emits `CliError("Requested driver host port N is not available")`) Fix in `hasExistingDriverHostPortFlag` and `hasShardingFlag`: also match `args[i].startsWith('--driver-host-port=')`, `--shards=`, `-s=`, `--shard-split=`, `--shard-all=`. Two new test scenarios cover `--driver-host-port=8000` (customer override) and all four sharding equals-forms. Also cleans up a stale `// ===== Self-hosted iOS port resolution helpers =====` section header in `packages/core/src/maestro-hierarchy.js:1253` that was left orphaned above `dump()` after the previous commit deleted the helpers it introduced. Addresses self-review findings on cli#2261 head 9f40e23. Co-Authored-By: Claude Opus 4.7 --- packages/cli-app/src/exec.js | 19 +++++++++++++---- packages/cli-app/test/exec.test.js | 28 ++++++++++++++++++++++++++ packages/core/src/maestro-hierarchy.js | 1 - 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/packages/cli-app/src/exec.js b/packages/cli-app/src/exec.js index f2c99e696..5c6c53fd6 100644 --- a/packages/cli-app/src/exec.js +++ b/packages/cli-app/src/exec.js @@ -130,10 +130,14 @@ export function maybeInjectScreenshotDir(ctx, log) { // True when argv contains a `--driver-host-port` flag the customer already // supplied. Scans the full argv slice past argv[2] (where flow files live) -// because customers can pass the flag at any position. +// because customers can pass the flag at any position. Detects BOTH +// space-separated (`--driver-host-port 8000`) and equals (`--driver-host-port=8000`) +// forms — picocli accepts both identically, so we must too or we'll +// double-inject and error out on Maestro startup. function hasExistingDriverHostPortFlag(args) { for (let i = 2; i < args.length; i++) { if (args[i] === '--driver-host-port') return true; + if (typeof args[i] === 'string' && args[i].startsWith('--driver-host-port=')) return true; } return false; } @@ -147,11 +151,18 @@ function hasExistingDriverHostPortFlag(args) { // shard port assignment, which works fine (status quo). // // Matches `--shards N`, `-s N` (deprecated short form), `--shard-split N`, -// `--shard-all N`. Conservative: any flag form gates us out. +// `--shard-all N`, and the equals-forms (`--shards=N`, `-s=N`, etc.). picocli +// accepts both; conservative — any flag form gates us out. function hasShardingFlag(args) { for (let i = 2; i < args.length; i++) { - if (args[i] === '--shards' || args[i] === '-s' || - args[i] === '--shard-split' || args[i] === '--shard-all') { + const a = args[i]; + if (typeof a !== 'string') continue; + if (a === '--shards' || a === '-s' || + a === '--shard-split' || a === '--shard-all') { + return true; + } + if (a.startsWith('--shards=') || a.startsWith('-s=') || + a.startsWith('--shard-split=') || a.startsWith('--shard-all=')) { return true; } } diff --git a/packages/cli-app/test/exec.test.js b/packages/cli-app/test/exec.test.js index f6367e570..fa3b95b1a 100644 --- a/packages/cli-app/test/exec.test.js +++ b/packages/cli-app/test/exec.test.js @@ -429,6 +429,19 @@ describe('percy app:exec', () => { expect(ctx.argv).toEqual(argv); }); + it('skips when customer passed equals-form --driver-host-port=8000 in argv', async () => { + // picocli accepts `--driver-host-port=N` and `--driver-host-port N` + // identically. The detection helper must catch BOTH or Maestro errors + // on duplicate `--driver-host-port` when we inject our own. + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['maestro', 'test', '--driver-host-port=8000', 'flow.yaml']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBeUndefined(); + expect(ctx.argv).toEqual(argv); + }); + it('treats invalid env values as absent and falls through to pickFreePort', async () => { process.env.PERCY_IOS_DRIVER_HOST_PORT = 'not-a-port'; stubNetWithPort(40000); @@ -488,6 +501,21 @@ describe('percy app:exec', () => { expect(ctx.argv).toEqual(argv); }); + it('skips when argv has equals-form sharding flags (--shards=3, --shard-split=3, --shard-all=2, -s=3)', async () => { + // picocli accepts `--shards=N` and `--shards N` identically. Without + // detecting the equals-form, we would inject `--driver-host-port` and + // Maestro would error on shard 2+ when it tries to bind the same port. + for (const shardingForm of ['--shards=3', '--shard-split=3', '--shard-all=2', '-s=3']) { + const created = spyOn(net, 'createServer').and.callThrough(); + const argv = ['maestro', 'test', shardingForm, 'flow.yaml']; + const ctx = ctxFor(argv); + await maybeInjectDriverHostPort(ctx); + expect(created).not.toHaveBeenCalled(); + expect(ctx.argv).toEqual(argv); + created.calls.reset(); + } + }); + it('preserves customer env value when sharded — gates on sharding, does not touch env', async () => { process.env.PERCY_IOS_DRIVER_HOST_PORT = '7001'; const created = spyOn(net, 'createServer').and.callThrough(); diff --git a/packages/core/src/maestro-hierarchy.js b/packages/core/src/maestro-hierarchy.js index 97a197a5b..b7b55ebe4 100644 --- a/packages/core/src/maestro-hierarchy.js +++ b/packages/core/src/maestro-hierarchy.js @@ -1250,7 +1250,6 @@ async function runAdbFallback(serial, execAdb) { return result; } -// ===== Self-hosted iOS port resolution helpers ===== export async function dump(options) { /* istanbul ignore next — options-omitted default; callers always pass an object (tests inject every dependency; production code binds them). */ From 841b8ad39aea32990d6785de3a89c947b8904274 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Tue, 16 Jun 2026 11:07:36 +0530 Subject: [PATCH 18/22] test(core): use os.tmpdir() for self-hosted maestro fixtures (fix Windows CI) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 4 self-hosted /percy/maestro-screenshot tests hardcoded POSIX paths under `/tmp/percy-self-hosted-*`. Windows has no `/tmp/`, so on the Windows CI runner: - fs.mkdirSync('/tmp/...') doesn't create the expected fixture root - the relay's `path.isAbsolute('/tmp/...')` is false on Windows - PERCY_MAESTRO_SCREENSHOT_DIR rejects, glob returns no match - all 4 tests 404 — caused both the original task #81 failures and the persistent Windows-only Test @percy/core red status on cli#2261 Fix: use `path.join(os.tmpdir(), 'percy-self-hosted-...')` so the fixtures land in the OS's real temp directory on every platform. Replace string-template path concatenation with `path.join(...)` to keep separators platform-correct. Production code is unaffected (it accepts absolute paths from the customer regardless of platform). This is purely a test-fixture portability fix. Resolves task #81 (self-hosted unit-test failures on Windows / memfs+ fast-glob interaction). Co-Authored-By: Claude Opus 4.7 --- packages/core/test/api.test.js | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index 071904b67..f71aaab33 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -1,3 +1,4 @@ +import os from 'os'; import path from 'path'; import PercyConfig from '@percy/config'; import { logger, setupTest, fs } from './helpers/index.js'; @@ -1879,8 +1880,11 @@ describe('API Server', () => { // Real-fs root (matched by the $bypass registered in the top-level // beforeEach) so the recursive `**` + `dot:true` glob runs against the // real filesystem — the production path — rather than memfs. - const SELF_HOSTED_ROOT = '/tmp/percy-self-hosted-real-root'; - const NESTED_SUBDIR = `${SELF_HOSTED_ROOT}/.maestro/run-x/screenshots`; + // Use os.tmpdir() (not hardcoded `/tmp/`) so the fixtures work on + // Windows runners too — Windows has no `/tmp/`; the CI fails 404 on + // every glob when the root never gets created. + const SELF_HOSTED_ROOT = path.join(os.tmpdir(), 'percy-self-hosted-real-root'); + const NESTED_SUBDIR = path.join(SELF_HOSTED_ROOT, '.maestro', 'run-x', 'screenshots'); const SELF_HOSTED_NAME = 'SelfHostedScreen'; let priorEnv; @@ -1897,7 +1901,7 @@ describe('API Server', () => { Buffer.from('IHDR', 'ascii').copy(pngHeader, 12); pngHeader.writeUInt32BE(1080, 16); pngHeader.writeUInt32BE(2400, 20); - fs.writeFileSync(`${NESTED_SUBDIR}/${SELF_HOSTED_NAME}.png`, pngHeader); + fs.writeFileSync(path.join(NESTED_SUBDIR, `${SELF_HOSTED_NAME}.png`), pngHeader); }); afterEach(() => { @@ -1930,15 +1934,16 @@ describe('API Server', () => { }); it('400s when PERCY_MAESTRO_SCREENSHOT_DIR does not exist', async () => { - process.env.PERCY_MAESTRO_SCREENSHOT_DIR = '/tmp/this-path-does-not-exist-percy-self-hosted'; + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = path.join(os.tmpdir(), 'this-path-does-not-exist-percy-self-hosted'); await percy.start(); await expectAsync(postMaestro({ name: SELF_HOSTED_NAME })) .toBeRejectedWithError(/PERCY_MAESTRO_SCREENSHOT_DIR is not an existing directory/); }); it('400s when PERCY_MAESTRO_SCREENSHOT_DIR points to a file, not a directory', async () => { - fs.writeFileSync('/tmp/percy-self-hosted-not-a-dir', 'plain-file'); - process.env.PERCY_MAESTRO_SCREENSHOT_DIR = '/tmp/percy-self-hosted-not-a-dir'; + const notADir = path.join(os.tmpdir(), 'percy-self-hosted-not-a-dir'); + fs.writeFileSync(notADir, 'plain-file'); + process.env.PERCY_MAESTRO_SCREENSHOT_DIR = notADir; await percy.start(); await expectAsync(postMaestro({ name: SELF_HOSTED_NAME })) .toBeRejectedWithError(/PERCY_MAESTRO_SCREENSHOT_DIR is not an existing directory/); @@ -1964,9 +1969,9 @@ describe('API Server', () => { // A symlink inside the root that points outside it must not exfiltrate // the target — the realpath + prefix check rejects it (self-hosted arm // of the "resolved outside" guard). Uses real fs via the $bypass. - const outside = '/tmp/percy-self-hosted-real-OUTSIDE.png'; + const outside = path.join(os.tmpdir(), 'percy-self-hosted-real-OUTSIDE.png'); fs.writeFileSync(outside, 'OUTSIDE'); - fs.symlinkSync(outside, `${NESTED_SUBDIR}/EscapeScreen.png`); + fs.symlinkSync(outside, path.join(NESTED_SUBDIR, 'EscapeScreen.png')); await percy.start(); await expectAsync(postMaestro({ name: 'EscapeScreen', platform: 'android' })) .toBeRejectedWithError(/resolved outside PERCY_MAESTRO_SCREENSHOT_DIR/); From 9c8754af375001fbea6ce9587fa4ab7ce62747de Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Tue, 16 Jun 2026 12:16:45 +0530 Subject: [PATCH 19/22] fix(core): make self-hosted maestro relay path-handling Windows-portable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two production-code bugs in the self-hosted /percy/maestro-screenshot relay only manifest on Windows: 1) **fast-glob pattern with backslashes** (api.js:578) `searchPattern = \`${scopeRoot}/**/${name}.png\`` mixes the OS-native separators in `scopeRoot` (backslashes on Windows from `path.resolve`) with the literal forward-slashes elsewhere in the pattern. fast-glob requires forward-slashes in patterns regardless of OS, per its docs. Result on Windows: every glob returns zero matches → 404. Fix: normalize backslashes to forward-slashes in the embedded scopeRoot just for pattern construction. POSIX behavior is identical (no backslashes to replace). 2) **realpath prefix check with hardcoded `/`** (api.js:646) `if (!realPath.startsWith(\`${realPrefix}/\`))` uses a literal `/` that does not match `path.sep` on Windows (which is `\`). Result: even when the glob would have succeeded, the security guard rejects the resolved file as "resolved outside ..." because the prefix check never matches. Fix: use `${realPrefix}${path.sep}` instead. Platform-aware; same load-bearing trailing-separator semantic (rejects sibling-prefix bypass like `/x/.maestro` vs `/x/.maestro-secrets`). These bugs went undetected on the Linux/macOS CI but break the 3 self- hosted relay specs on the Windows runner — the actual root cause of the persistent `Test @percy/core` Windows failure on cli#2261. Co-Authored-By: Claude Opus 4.7 --- packages/core/src/api.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 33dd7a7b7..d28204649 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -575,7 +575,14 @@ export function createPercyServer(percy, port) { // (PERCY_MAESTRO_SCREENSHOT_DIR). Recursive depth handles arbitrary // Maestro layouts; `name` is SAFE_ID-validated above so it cannot // contain separators or traversal characters. - searchPattern = `${scopeRoot}/**/${name}.png`; + // + // fast-glob requires forward-slashes in patterns on every platform + // (per its docs: "Always use forward-slashes in glob expressions"). + // On Windows the scopeRoot from path.resolve contains backslashes, + // so we normalize before embedding into the pattern. Production- + // code Windows portability — verified by the CI Windows runner. + const globRoot = scopeRoot.replace(/\\/g, '/'); + searchPattern = `${globRoot}/**/${name}.png`; } else { searchPattern = platform === 'ios' ? `/tmp/${sessionId}/*_maestro_debug_*/**/${name}.png` @@ -627,8 +634,10 @@ export function createPercyServer(percy, port) { // Canonicalize and confirm the resolved path still lives under scopeRoot. // Defeats symlink swaps where the root points elsewhere. Both ends are // realpath'd because /tmp is a symlink on macOS (where iOS hosts run). - // The trailing `/` on the prefix is load-bearing — it prevents + // The trailing separator on the prefix is load-bearing — it prevents // sibling-prefix bypass (e.g. /x/.maestro vs /x/.maestro-secrets). + // Use `path.sep` (not hardcoded `/`) so the prefix check is correct on + // Windows, where realpath returns backslash-separated paths. let realPath, realPrefix; try { realPath = await fs.promises.realpath(chosenFile); @@ -636,7 +645,7 @@ export function createPercyServer(percy, port) { } catch { throw new ServerError(404, `Screenshot not found: ${name}.png (path resolution failed)`); } - if (!realPath.startsWith(`${realPrefix}/`)) { + if (!realPath.startsWith(`${realPrefix}${path.sep}`)) { throw new ServerError(404, `Screenshot not found: ${name}.png (resolved outside ${selfHosted ? 'PERCY_MAESTRO_SCREENSHOT_DIR' : 'session dir'})`); } From e22793d3783cacfbb703d2c3d3db72fad078dc69 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Tue, 16 Jun 2026 13:59:46 +0530 Subject: [PATCH 20/22] fix(core): forward-slash normalize both sides of self-hosted realpath check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous attempt at Windows portability (commit 9c8754af) used `path.sep` in the prefix check. That breaks the BS-path tests on Windows because memfs (used by api.test.js mocks) returns POSIX-style virtual paths regardless of host OS, so the realpath check expected `\` but got `/` and 404'd 23 specs. The correct fix: forward-slash normalize BOTH sides before the startsWith comparison. realPath: POSIX → `/tmp/abc/foo.png` (unchanged) Windows real-fs → `C:\...\Temp\...\foo.png` → `C:/.../foo.png` memfs (tests) → `/tmp/abc/foo.png` (unchanged) realPrefix: same — normalize, then compare with trailing `/`. This works on every platform: no-op on POSIX (no backslashes to replace), correct on Windows real-fs (matches normalized prefix), correct on memfs tests (POSIX-style paths pass through unchanged). The trailing `/` on the prefix is still load-bearing — prevents sibling-prefix bypass (e.g. `/x/.maestro` vs `/x/.maestro-secrets`). Co-Authored-By: Claude Opus 4.7 --- packages/core/src/api.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index d28204649..954eff78d 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -634,10 +634,13 @@ export function createPercyServer(percy, port) { // Canonicalize and confirm the resolved path still lives under scopeRoot. // Defeats symlink swaps where the root points elsewhere. Both ends are // realpath'd because /tmp is a symlink on macOS (where iOS hosts run). - // The trailing separator on the prefix is load-bearing — it prevents + // The trailing `/` on the prefix is load-bearing — it prevents // sibling-prefix bypass (e.g. /x/.maestro vs /x/.maestro-secrets). - // Use `path.sep` (not hardcoded `/`) so the prefix check is correct on - // Windows, where realpath returns backslash-separated paths. + // + // Normalize both sides to forward-slashes before the prefix check so + // the same code works on Windows (real-fs returns backslashes) AND on + // POSIX (no-op) AND on memfs in tests (POSIX-style virtual paths + // regardless of host OS). let realPath, realPrefix; try { realPath = await fs.promises.realpath(chosenFile); @@ -645,7 +648,9 @@ export function createPercyServer(percy, port) { } catch { throw new ServerError(404, `Screenshot not found: ${name}.png (path resolution failed)`); } - if (!realPath.startsWith(`${realPrefix}${path.sep}`)) { + const realPathFwd = realPath.replace(/\\/g, '/'); + const realPrefixFwd = realPrefix.replace(/\\/g, '/'); + if (!realPathFwd.startsWith(`${realPrefixFwd}/`)) { throw new ServerError(404, `Screenshot not found: ${name}.png (resolved outside ${selfHosted ? 'PERCY_MAESTRO_SCREENSHOT_DIR' : 'session dir'})`); } From 8b60341c02f382221538fc75347a8f7a4b0f8a04 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Tue, 16 Jun 2026 16:31:37 +0530 Subject: [PATCH 21/22] fix(self-hosted maestro): align iOS port discovery with Maestro 2.4.0 CLI surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local end-to-end validation against Maestro 2.4.0 (the version most customers run today) surfaced two issues with the prior auto-injection of `--driver-host-port`: * Maestro 2.4.0 does not expose `--driver-host-port` at all — picocli rejects it on both `maestro test --driver-host-port N` and `maestro --driver-host-port N test`. The flag was added in a later Maestro release (>= 2.6.x). * Without the flag, the CLI cli-app's auto-injection caused `maestro test` to exit with status 2 ("Unknown option") on every self-hosted run, producing zero snapshots. This change stabilizes the self-hosted iOS path so it works on the dominant Maestro version (2.4.0) while preserving the existing explicit-port behavior for BS and power-user pinning. Stability changes: - `@percy/cli-app`: remove `maybeInjectDriverHostPort`. The exec callback now only injects `-e PERCY_SERVER` and `--test-output-dir` — both `test`-subcommand options Maestro 2.4.0 accepts. The associated 17-scenario test block is removed. - `@percy/core` relay: when `PERCY_IOS_DRIVER_HOST_PORT` is unset, probe the documented Maestro 2.4.0 single-simulator default (`127.0.0.1:7001`) once via the existing `runIosHttpDump` transport and use the result if the driver is alive. No `lsof`, no range probe — a single HTTP request to the deterministic port. If the probe finds no driver, return `env-missing` with the prior warn-skip behavior. Customers on Maestro 2.6+ (ephemeral port) or sharded runs set `PERCY_IOS_DRIVER_HOST_PORT` to pin the port. - Tests: existing env-missing cases updated to allow the new probe call (asserting port=7001 and connection-refused triggers the env-missing warn-skip). New positive case covers the probe-hit path returning a hierarchy dump. R7 (BS production path unchanged) holds: BS hosts always set `PERCY_IOS_DRIVER_HOST_PORT` via `cli_manager.rb#start_percy_cli`, so the explicit-branch HTTP transport runs identically. Validated locally on iOS Simulator (iPhone 15 Pro, iOS 17.2, Maestro 2.4.0): 3 snapshots uploaded successfully, build finalized at https://percy.io/9560f98d/app/RegionsIos-3d2ab5a4/builds/50894457 Co-Authored-By: Claude Opus 4.7 --- packages/cli-app/src/exec.js | 153 +---------- packages/cli-app/src/index.js | 2 +- packages/cli-app/test/exec.test.js | 239 +----------------- packages/core/src/maestro-hierarchy.js | 43 +++- .../core/test/unit/maestro-hierarchy.test.js | 53 +++- 5 files changed, 81 insertions(+), 409 deletions(-) diff --git a/packages/cli-app/src/exec.js b/packages/cli-app/src/exec.js index 5c6c53fd6..f72d2d28f 100644 --- a/packages/cli-app/src/exec.js +++ b/packages/cli-app/src/exec.js @@ -1,5 +1,4 @@ import fs from 'fs'; -import net from 'net'; import os from 'os'; import path from 'path'; import command from '@percy/cli-command'; @@ -128,142 +127,6 @@ export function maybeInjectScreenshotDir(ctx, log) { if (!flagSet) args.splice(2, 0, '--test-output-dir', resolved); } -// True when argv contains a `--driver-host-port` flag the customer already -// supplied. Scans the full argv slice past argv[2] (where flow files live) -// because customers can pass the flag at any position. Detects BOTH -// space-separated (`--driver-host-port 8000`) and equals (`--driver-host-port=8000`) -// forms — picocli accepts both identically, so we must too or we'll -// double-inject and error out on Maestro startup. -function hasExistingDriverHostPortFlag(args) { - for (let i = 2; i < args.length; i++) { - if (args[i] === '--driver-host-port') return true; - if (typeof args[i] === 'string' && args[i].startsWith('--driver-host-port=')) return true; - } - return false; -} - -// True when argv contains a Maestro sharding flag. Per Maestro's -// TestCommand.kt#selectPort, each shard calls selectPort() independently — -// if --driver-host-port N is set, shard 1 binds N and shards 2+ fail with -// `CliError("Requested driver host port N is not available")`. So when the -// customer is running sharded, we MUST NOT inject a single port. Sharded -// runs without our inject fall through to Maestro's ServerSocket(0) per- -// shard port assignment, which works fine (status quo). -// -// Matches `--shards N`, `-s N` (deprecated short form), `--shard-split N`, -// `--shard-all N`, and the equals-forms (`--shards=N`, `-s=N`, etc.). picocli -// accepts both; conservative — any flag form gates us out. -function hasShardingFlag(args) { - for (let i = 2; i < args.length; i++) { - const a = args[i]; - if (typeof a !== 'string') continue; - if (a === '--shards' || a === '-s' || - a === '--shard-split' || a === '--shard-all') { - return true; - } - if (a.startsWith('--shards=') || a.startsWith('-s=') || - a.startsWith('--shard-split=') || a.startsWith('--shard-all=')) { - return true; - } - } - return false; -} - -// Ask the OS for a free TCP port via Node's `net` module. Mirrors Maestro's -// own `ServerSocket(0)` strategy in TestCommand.kt#selectPort. Binds, reads -// the assigned port, closes immediately. Linux and macOS kernels do NOT -// immediately re-assign released ephemeral ports — the reservation window -// is tens of seconds, comfortably longer than the ~1s gap before Maestro's -// own bind. The TOCTOU race is theoretical, not realistic; if it ever -// fires, Maestro errors loudly with "Requested driver host port N is not -// available" which surfaces through `percy app:exec`'s stderr forwarding. -function pickFreePort() { - return new Promise((resolve, reject) => { - const server = net.createServer(); - server.unref(); - server.once('error', reject); - server.listen(0, '127.0.0.1', () => { - const port = server.address()?.port; - server.close(() => { - if (Number.isInteger(port) && port > 0) resolve(port); - /* istanbul ignore next — defensive guard; Node's listen(0) on an - available interface returns an integer port on every supported - OS. This branch is only reachable if address() returns null - between listen and close, which we have never observed. */ - else reject(new Error('pickFreePort: invalid port from net.createServer')); - }); - }); - }); -} - -// Parse the candidate value for `PERCY_IOS_DRIVER_HOST_PORT` env override. -// Returns an integer in 1-65535 or null. Mirrors the validator semantics -// in @percy/core's `parseIosDriverHostPort` — both readers must agree on -// what "valid" means so customer-supplied values that the relay accepts -// are also the values we'll inject as the Maestro `--driver-host-port`. -function parseDriverHostPortEnv(raw) { - if (raw === undefined || raw === null || raw === '') return null; - const n = Number(raw); - if (!Number.isInteger(n) || n < 1 || n > 65535) return null; - return n; -} - -// Prescribe-don't-discover for the iOS driver host port. -// -// `maestro test` accepts a hidden but fully-supported `--driver-host-port` -// flag (see Maestro source `App.kt:99` and `TestCommand.kt:538-549`). When -// passed, Maestro binds the driver to exactly that port. When absent, -// Maestro uses `ServerSocket(0)` and assigns a random ephemeral port we -// could not predict from outside the process. -// -// We auto-inject `--driver-host-port ` so the relay in @percy/core -// can hit the iOS driver deterministically via `PERCY_IOS_DRIVER_HOST_PORT` -// (which we also write). This replaces the older probe-and-lsof cascade. -// -// Resolution: -// 1. Customer set `--driver-host-port` in argv → no-op (their override). -// 2. Argv contains a sharding flag → no-op. Sharded runs need per-shard -// ports; a single injected port would break shards 2+ at startup. -// 3. Customer set valid `PERCY_IOS_DRIVER_HOST_PORT` env → inject that -// value (preserves BS-host injection where the env arrives via -// `cli_manager.rb#start_percy_cli`, though that path doesn't reach -// `app:exec`; preserves self-hosted customer pinning). -// 4. Otherwise → ask the OS for a free port via `pickFreePort()` and -// write it back to `process.env.PERCY_IOS_DRIVER_HOST_PORT` so the -// @percy/core relay reads the same value. -// -// On any internal failure (extremely unlikely — only if `pickFreePort` -// throws), emit a WARN and skip the inject so the customer still gets a -// maestro test run, just without iOS element-region support. -export async function maybeInjectDriverHostPort(ctx, log) { - const args = ctx?.argv; - if (!Array.isArray(args) || args.length < 2) return; - if (path.basename(args[0]) !== 'maestro') return; - if (args[1] !== 'test') return; - if (hasExistingDriverHostPortFlag(args)) return; - if (hasShardingFlag(args)) return; - - let port = parseDriverHostPortEnv(process.env.PERCY_IOS_DRIVER_HOST_PORT); - if (port === null) { - try { - port = await pickFreePort(); - } catch (err) { - /* istanbul ignore next — pickFreePort rejection is reachable only on - systems with no free ephemeral ports, an effectively impossible - state. The WARN-and-skip path is defensive insurance. */ - log?.warn( - `Could not auto-pick a Maestro driver host port (${err?.code || err?.message || err}); ` + - '--driver-host-port not injected. iOS element regions may be unavailable. ' + - 'Set PERCY_IOS_DRIVER_HOST_PORT to a known-free port to override.' - ); - return; - } - process.env.PERCY_IOS_DRIVER_HOST_PORT = String(port); - } - - args.splice(2, 0, '--driver-host-port', String(port)); -} - export const exec = command('exec', { description: 'Start and stop Percy around a supplied command for native apps', usage: '[options] -- ', @@ -279,16 +142,16 @@ export const exec = command('exec', { skipDiscovery: true } }, async function*(ctx) { - // Each helper splices at index 2, so later calls push earlier flag - // groups to higher indices. Final argv for `maestro test flow.yaml`: - // maestro test --driver-host-port --test-output-dir - // -e PERCY_SERVER= flow.yaml - // All flag groups land between `test` and the flow file, which Maestro - // accepts. The driver-host-port helper is async (it may need to ask - // the OS for a free port), so we await it; the other two are sync. + // 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 -e PERCY_SERVER= 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`. maybeInjectMaestroServer(ctx, ctx.log); maybeInjectScreenshotDir(ctx, ctx.log); - await maybeInjectDriverHostPort(ctx, ctx.log); yield* ExecPlugin.default.callback(ctx); }); diff --git a/packages/cli-app/src/index.js b/packages/cli-app/src/index.js index b6d8d7b59..d1c8d4ffd 100644 --- a/packages/cli-app/src/index.js +++ b/packages/cli-app/src/index.js @@ -1,2 +1,2 @@ export { default, app } from './app.js'; -export { exec, start, stop, ping, maybeInjectMaestroServer, maybeInjectScreenshotDir, maybeInjectDriverHostPort } from './exec.js'; +export { exec, start, stop, ping, maybeInjectMaestroServer, maybeInjectScreenshotDir } from './exec.js'; diff --git a/packages/cli-app/test/exec.test.js b/packages/cli-app/test/exec.test.js index fa3b95b1a..be32f1ee9 100644 --- a/packages/cli-app/test/exec.test.js +++ b/packages/cli-app/test/exec.test.js @@ -1,12 +1,11 @@ import fs from 'fs'; -import net from 'net'; import os from 'os'; import path from 'path'; import { setupTest } from '@percy/cli-command/test/helpers'; import * as ExecPlugin from '@percy/cli-exec'; import { exec, start, stop, ping, - maybeInjectMaestroServer, maybeInjectScreenshotDir, maybeInjectDriverHostPort + maybeInjectMaestroServer, maybeInjectScreenshotDir } from '@percy/cli-app'; describe('percy app:exec', () => { @@ -351,240 +350,4 @@ describe('percy app:exec', () => { ]); }); }); - - describe('maybeInjectDriverHostPort', () => { - // Prescribe-don't-discover: cli-app injects --driver-host-port so the - // @percy/core relay can hit the iOS Maestro driver deterministically - // via the matching PERCY_IOS_DRIVER_HOST_PORT env var. Customer overrides - // at two layers (argv flag, env var) and the sharded-run gate must all - // be honored. - - let originalEnvValue; - - beforeEach(() => { - originalEnvValue = process.env.PERCY_IOS_DRIVER_HOST_PORT; - delete process.env.PERCY_IOS_DRIVER_HOST_PORT; - }); - - afterEach(() => { - if (originalEnvValue === undefined) { - delete process.env.PERCY_IOS_DRIVER_HOST_PORT; - } else { - process.env.PERCY_IOS_DRIVER_HOST_PORT = originalEnvValue; - } - }); - - function ctxFor(argv) { - return { argv: [...argv] }; - } - - // Helper: a net.createServer stub that emits the listen callback with - // a fake "address().port" value, then succeeds on close. Mirrors the - // real shape closely enough for the helper's promise to resolve to - // `fakePort` without touching real sockets in unit tests. - function stubNetWithPort(fakePort) { - return spyOn(net, 'createServer').and.callFake(() => { - const fakeServer = { - unref() {}, - once() {}, - listen(_port, _host, cb) { setImmediate(cb); }, - address() { return { port: fakePort }; }, - close(cb) { setImmediate(cb); } - }; - return fakeServer; - }); - } - - it('injects --driver-host-port with PERCY_IOS_DRIVER_HOST_PORT env value when set', async () => { - process.env.PERCY_IOS_DRIVER_HOST_PORT = '7001'; - const created = spyOn(net, 'createServer').and.callThrough(); - const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); - await maybeInjectDriverHostPort(ctx); - // Did NOT call net.createServer — env value short-circuits pickFreePort - expect(created).not.toHaveBeenCalled(); - // env preserved (we use the customer value, don't rewrite it) - expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('7001'); - expect(ctx.argv).toEqual([ - 'maestro', 'test', '--driver-host-port', '7001', 'flow.yaml' - ]); - }); - - it('picks a free port and writes env when neither env nor argv flag is set', async () => { - stubNetWithPort(54321); - const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); - await maybeInjectDriverHostPort(ctx); - expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('54321'); - expect(ctx.argv).toEqual([ - 'maestro', 'test', '--driver-host-port', '54321', 'flow.yaml' - ]); - }); - - it('skips when customer already passed --driver-host-port in argv', async () => { - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['maestro', 'test', '--driver-host-port', '8000', 'flow.yaml']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBeUndefined(); - expect(ctx.argv).toEqual(argv); - }); - - it('skips when customer passed equals-form --driver-host-port=8000 in argv', async () => { - // picocli accepts `--driver-host-port=N` and `--driver-host-port N` - // identically. The detection helper must catch BOTH or Maestro errors - // on duplicate `--driver-host-port` when we inject our own. - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['maestro', 'test', '--driver-host-port=8000', 'flow.yaml']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBeUndefined(); - expect(ctx.argv).toEqual(argv); - }); - - it('treats invalid env values as absent and falls through to pickFreePort', async () => { - process.env.PERCY_IOS_DRIVER_HOST_PORT = 'not-a-port'; - stubNetWithPort(40000); - const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); - await maybeInjectDriverHostPort(ctx); - expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('40000'); - expect(ctx.argv).toEqual([ - 'maestro', 'test', '--driver-host-port', '40000', 'flow.yaml' - ]); - }); - - it('rejects out-of-range env values (70000) and picks a free port', async () => { - process.env.PERCY_IOS_DRIVER_HOST_PORT = '70000'; - stubNetWithPort(40001); - const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); - await maybeInjectDriverHostPort(ctx); - expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('40001'); - expect(ctx.argv).toEqual([ - 'maestro', 'test', '--driver-host-port', '40001', 'flow.yaml' - ]); - }); - - it('skips when argv has --shards (would break shard 2+ on a single injected port)', async () => { - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['maestro', 'test', '--shards', '3', 'flow.yaml']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBeUndefined(); - expect(ctx.argv).toEqual(argv); - }); - - it('skips when argv has -s (deprecated short form of --shards)', async () => { - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['maestro', 'test', '-s', '3', 'flow.yaml']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - expect(ctx.argv).toEqual(argv); - }); - - it('skips when argv has --shard-split', async () => { - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['maestro', 'test', '--shard-split', '3', 'flow.yaml']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - expect(ctx.argv).toEqual(argv); - }); - - it('skips when argv has --shard-all', async () => { - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['maestro', 'test', '--shard-all', '2', 'flow.yaml']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - expect(ctx.argv).toEqual(argv); - }); - - it('skips when argv has equals-form sharding flags (--shards=3, --shard-split=3, --shard-all=2, -s=3)', async () => { - // picocli accepts `--shards=N` and `--shards N` identically. Without - // detecting the equals-form, we would inject `--driver-host-port` and - // Maestro would error on shard 2+ when it tries to bind the same port. - for (const shardingForm of ['--shards=3', '--shard-split=3', '--shard-all=2', '-s=3']) { - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['maestro', 'test', shardingForm, 'flow.yaml']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - expect(ctx.argv).toEqual(argv); - created.calls.reset(); - } - }); - - it('preserves customer env value when sharded — gates on sharding, does not touch env', async () => { - process.env.PERCY_IOS_DRIVER_HOST_PORT = '7001'; - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['maestro', 'test', '--shard-split', '2', 'flow.yaml']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - // env left untouched — sharded customers must pin per-shard themselves - expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('7001'); - expect(ctx.argv).toEqual(argv); - }); - - it('skips for `maestro hierarchy` (not a test command)', async () => { - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['maestro', 'hierarchy', '--udid', 'X']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - expect(ctx.argv).toEqual(argv); - }); - - it('skips for non-maestro commands (argv[0] is npx or other)', async () => { - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['npx', 'maestro', 'test', 'flow.yaml']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - expect(ctx.argv).toEqual(argv); - }); - - it('skips when args has fewer than two elements', async () => { - const created = spyOn(net, 'createServer').and.callThrough(); - const argv = ['maestro']; - const ctx = ctxFor(argv); - await maybeInjectDriverHostPort(ctx); - expect(created).not.toHaveBeenCalled(); - expect(ctx.argv).toEqual(argv); - }); - - it('matches by basename when the command is an absolute path', async () => { - stubNetWithPort(50500); - const ctx = ctxFor(['/Users/foo/.maestro/bin/maestro', 'test', 'flow.yaml']); - await maybeInjectDriverHostPort(ctx); - expect(process.env.PERCY_IOS_DRIVER_HOST_PORT).toBe('50500'); - expect(ctx.argv).toEqual([ - '/Users/foo/.maestro/bin/maestro', 'test', - '--driver-host-port', '50500', - 'flow.yaml' - ]); - }); - - it('splices at index 2 alongside sibling injections from --test-output-dir / -e PERCY_SERVER', async () => { - // Simulate post-state of sibling helpers having already injected - // their flags. New flag should land between `test` and them. - stubNetWithPort(55555); - const ctx = ctxFor([ - 'maestro', 'test', - '--test-output-dir', '/some/dir', - '-e', 'PERCY_SERVER=http://localhost:5338', - 'flow.yaml' - ]); - await maybeInjectDriverHostPort(ctx); - expect(ctx.argv).toEqual([ - 'maestro', 'test', - '--driver-host-port', '55555', - '--test-output-dir', '/some/dir', - '-e', 'PERCY_SERVER=http://localhost:5338', - 'flow.yaml' - ]); - }); - }); }); diff --git a/packages/core/src/maestro-hierarchy.js b/packages/core/src/maestro-hierarchy.js index b7b55ebe4..aec35f696 100644 --- a/packages/core/src/maestro-hierarchy.js +++ b/packages/core/src/maestro-hierarchy.js @@ -1273,23 +1273,40 @@ export async function dump(options) { if (platform === 'ios') { const udid = getEnv('PERCY_IOS_DEVICE_UDID'); - const driverHostPortRaw = getEnv('PERCY_IOS_DRIVER_HOST_PORT'); - - // `PERCY_IOS_DRIVER_HOST_PORT` is the single source of truth for the - // iOS Maestro driver port. It is set by: + let driverHostPortRaw = getEnv('PERCY_IOS_DRIVER_HOST_PORT'); + + // Self-hosted env-absent path: probe the Maestro 2.4.0 documented + // single-simulator default port (127.0.0.1:7001) before giving up. + // Maestro 2.4.0's TestCommand binds the driver to 7001 deterministically + // for the single-simulator case, so a single HTTP probe resolves the + // dominant self-hosted scenario without any port discovery from outside + // the process. Older releases used the same default; newer releases + // (2.6+, ephemeral port via ServerSocket(0)) require the customer to + // pin via `PERCY_IOS_DRIVER_HOST_PORT` (or `maestro --driver-host-port`, + // which 2.6+ accepts at the parent level). + // + // `PERCY_IOS_DRIVER_HOST_PORT` is the only signal the explicit branch + // below reads. Set by: // * BrowserStack hosts (canonical 11100-11110 range, via // `cli_manager.rb#start_percy_cli` env injection) - // * `@percy/cli-app`'s `maybeInjectDriverHostPort` helper for - // self-hosted customers running `percy app:exec` (any port, - // either customer-supplied or picked by the OS at startup) - // * Manually by power users who pin the driver port themselves + // * Customers who pin the driver host port themselves // - // When the env var is absent, the customer skipped `percy app:exec` - // (or BS host injection failed). Element regions degrade gracefully - // with `env-missing` — coordinate regions and the snapshot itself - // continue to upload via the relay's other paths. + // If neither the env var nor a live driver on 7001 is found, element + // regions degrade gracefully with `env-missing` — coordinate regions + // and the snapshot itself continue to upload via the relay's other + // paths. if (!driverHostPortRaw) { - log.warn('[percy] iOS element regions unavailable — PERCY_IOS_DRIVER_HOST_PORT is not set. Run `percy app:exec -- maestro test ...` so it is injected automatically, or set it manually to your Maestro driver-host-port.'); + const probe = await runIosHttpDump({ port: 7001, sessionId, httpRequest }); + if (probe.kind === 'hierarchy' || probe.kind === 'no-aut-tree') { + log.debug(`dump took ${Date.now() - started}ms via maestro-http (self-hosted probe :7001, ${probe.kind === 'hierarchy' ? `${probe.nodes.length} nodes` : probe.reason})`); + if (probe.kind === 'hierarchy') { + recordResolverSuccess({ platform: 'ios', via: 'maestro-http' }); + } else { + recordResolverFinalFailure({ platform: 'ios', failureClass: failureClassFromReason(probe.reason) }); + } + return probe; + } + log.warn('[percy] iOS element regions unavailable — no Maestro driver responded on 127.0.0.1:7001. Set PERCY_IOS_DRIVER_HOST_PORT if your driver binds a non-default port (e.g. Maestro 2.6+ ephemeral or sharded runs).'); recordResolverFinalFailure({ platform: 'ios', failureClass: 'other' }); return { kind: 'unavailable', reason: 'env-missing' }; } diff --git a/packages/core/test/unit/maestro-hierarchy.test.js b/packages/core/test/unit/maestro-hierarchy.test.js index aa8f92543..57fd6203b 100644 --- a/packages/core/test/unit/maestro-hierarchy.test.js +++ b/packages/core/test/unit/maestro-hierarchy.test.js @@ -491,28 +491,57 @@ describe('Unit / maestro-hierarchy', () => { expect(res).toEqual({ kind: 'unavailable', reason: 'self-hosted-no-udid' }); }); - // Self-hosted (UDID-set, PORT-unset): the prescribe-don't-discover refactor - // (Phase 3) removed the port-discovery cascade. PERCY_IOS_DRIVER_HOST_PORT - // is now the single source of truth. When it is absent the relay returns - // env-missing and the snapshot continues to upload via other paths; - // element regions degrade gracefully. UDID presence/absence is irrelevant - // when the port env is unset — without a port there is nothing to call. - it('warn-skips with env-missing when port is unset (udid set)', async () => { + // Self-hosted env-absent path: with PERCY_IOS_DRIVER_HOST_PORT unset, the + // relay probes Maestro 2.4.0's documented single-simulator default port + // (127.0.0.1:7001) before degrading. When the probe finds a live Maestro + // driver, element regions resolve through the same `runIosHttpDump` + // transport the BS path uses. When the probe fails, we warn-skip with + // env-missing — coordinate regions and the snapshot itself still upload. + // Maestro 2.6+ (ephemeral port) customers set PERCY_IOS_DRIVER_HOST_PORT + // explicitly to bypass the probe. + + it('probes 127.0.0.1:7001 when env is unset, returns the dump on hit', async () => { + let observedPort = null; + const minimalAxElement = JSON.stringify({ + axElement: { + elementType: 1, + identifier: 'com.example.app', + frame: { X: 0, Y: 0, Width: 100, Height: 100 }, + children: [] + } + }); + const httpRequest = async opts => { + observedPort = opts.port; + return { statusCode: 200, headers: { 'content-type': 'application/json' }, body: minimalAxElement }; + }; + const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest }); + expect(observedPort).toBe(7001); + expect(res.kind).toBe('hierarchy'); + }); + + it('warn-skips with env-missing when port is unset and the 7001 probe finds no driver', async () => { const getEnv = key => { if (key === 'PERCY_IOS_DEVICE_UDID') return '00008110-000065081404401E'; return undefined; }; - const httpRequest = jasmine.createSpy('httpRequest'); + const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { + throw Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); + }); const res = await dump({ platform: 'ios', getEnv, httpRequest }); expect(res).toEqual({ kind: 'unavailable', reason: 'env-missing' }); - expect(httpRequest).not.toHaveBeenCalled(); + // The probe must have been attempted exactly once (port 7001). + expect(httpRequest).toHaveBeenCalledTimes(1); + expect(httpRequest.calls.mostRecent().args[0].port).toBe(7001); }); - it('warn-skips with env-missing when both env vars are unset', async () => { - const httpRequest = jasmine.createSpy('httpRequest'); + it('warn-skips with env-missing when both env vars are unset and the 7001 probe finds no driver', async () => { + const httpRequest = jasmine.createSpy('httpRequest').and.callFake(async () => { + throw Object.assign(new Error('econnrefused'), { code: 'ECONNREFUSED' }); + }); const res = await dump({ platform: 'ios', getEnv: () => undefined, httpRequest }); expect(res).toEqual({ kind: 'unavailable', reason: 'env-missing' }); - expect(httpRequest).not.toHaveBeenCalled(); + expect(httpRequest).toHaveBeenCalledTimes(1); + expect(httpRequest.calls.mostRecent().args[0].port).toBe(7001); }); it('does not invoke adb on iOS dispatch', async () => { From 93fb5c28fcba1a0a5b5d175c5869898c87a8b78a Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Tue, 16 Jun 2026 18:01:21 +0530 Subject: [PATCH 22/22] fix(cli-app): inject maestro env+output-dir past global parent flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The auto-injection helpers in `@percy/cli-app` previously assumed `test` was always at `args[1]`. Maestro accepts global parent flags BEFORE the subcommand (picocli convention) — e.g. `maestro --udid test flow.yaml`, `maestro --platform=android test flow.yaml`. When a customer pinned a device with `--udid`, both helpers silently no-op'd, so `-e PERCY_SERVER=...` never reached the Maestro JS sandbox and snapshots were never uploaded ("Snapshot command was not called" with exit 0). Replace the index-1 check with `findTestSubcommandIdx`, which scans for the `test` literal while skipping the value of known value-taking parent flags (`--udid`, `--device`, `--platform`, `-p`, `--host`, `--port`, `--driver-host-port`). Equals-form (`--flag=value`) doesn't need a skip. `-e PERCY_SERVER` and `--test-output-dir` are then spliced immediately after `test`, so both flags bind to the `test` subcommand (the only Maestro subcommand that accepts them). The `hasExistingPercyServerFlag` and `findTestOutputDirValueIdx` scans now start from `testIdx + 1` so customer-supplied overrides that sit after the subcommand still suppress injection. Adds 8 new test scenarios: `--udid test`, `--device test`, `--platform=android test` (equals form), multiple parent flags chained, and deeper customer overrides for both helpers. --- packages/cli-app/src/exec.js | 54 ++++++++++++--- packages/cli-app/test/exec.test.js | 102 +++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 10 deletions(-) diff --git a/packages/cli-app/src/exec.js b/packages/cli-app/src/exec.js index f72d2d28f..4b2f6d767 100644 --- a/packages/cli-app/src/exec.js +++ b/packages/cli-app/src/exec.js @@ -18,8 +18,37 @@ export const start = command('start', { } }, ExecPlugin.start.callback); -function hasExistingPercyServerFlag(args) { - for (let i = 2; i < args.length - 1; i++) { +// Locate the `test` subcommand in argv. Maestro accepts global parent +// flags before the subcommand, e.g.: +// maestro test flow.yaml +// maestro --udid 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; @@ -28,8 +57,8 @@ function hasExistingPercyServerFlag(args) { // Returns the index of the value following `--test-output-dir`, or -1 if absent. // We return the value-index (not just a boolean) so the screenshot-dir helper // can align PERCY_MAESTRO_SCREENSHOT_DIR with a customer-supplied flag value. -function findTestOutputDirValueIdx(args) { - for (let i = 2; i < args.length - 1; i++) { +function findTestOutputDirValueIdx(args, testIdx) { + for (let i = testIdx + 1; i < args.length - 1; i++) { if (args[i] === '--test-output-dir') return i + 1; } return -1; @@ -51,8 +80,9 @@ export function maybeInjectMaestroServer(ctx, log) { const args = ctx?.argv; if (!Array.isArray(args) || args.length < 2) return; if (path.basename(args[0]) !== 'maestro') return; - if (args[1] !== 'test') return; - if (hasExistingPercyServerFlag(args)) return; + const testIdx = findTestSubcommandIdx(args); + if (testIdx < 0) return; + if (hasExistingPercyServerFlag(args, testIdx)) return; const addr = ctx.percy?.address(); if (!addr) { log?.warn( @@ -62,7 +92,9 @@ export function maybeInjectMaestroServer(ctx, log) { ); return; } - args.splice(2, 0, '-e', `PERCY_SERVER=${addr}`); + // 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 @@ -87,10 +119,11 @@ export function maybeInjectScreenshotDir(ctx, log) { const args = ctx?.argv; if (!Array.isArray(args) || args.length < 2) return; if (path.basename(args[0]) !== 'maestro') return; - if (args[1] !== 'test') return; + const testIdx = findTestSubcommandIdx(args); + if (testIdx < 0) return; const envSet = !!process.env.PERCY_MAESTRO_SCREENSHOT_DIR; - const flagValueIdx = findTestOutputDirValueIdx(args); + const flagValueIdx = findTestOutputDirValueIdx(args, testIdx); const flagSet = flagValueIdx > 0; // Fully customer-controlled — nothing to do. @@ -124,7 +157,8 @@ export function maybeInjectScreenshotDir(ctx, log) { } if (!envSet) process.env.PERCY_MAESTRO_SCREENSHOT_DIR = resolved; - if (!flagSet) args.splice(2, 0, '--test-output-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', { diff --git a/packages/cli-app/test/exec.test.js b/packages/cli-app/test/exec.test.js index be32f1ee9..e5dcee969 100644 --- a/packages/cli-app/test/exec.test.js +++ b/packages/cli-app/test/exec.test.js @@ -182,6 +182,64 @@ describe('percy app:exec', () => { maybeInjectMaestroServer(ctx, log); expect(log.warn).not.toHaveBeenCalled(); }); + + // Maestro accepts global flags BEFORE the `test` subcommand + // (picocli convention): `maestro --udid X test flow.yaml`, + // `maestro --platform=android test flow.yaml`, etc. The injection + // must locate `test` and splice the -e pair right after it, not + // assume args[1] === 'test'. + it('injects after `test` when --udid precedes the subcommand', () => { + const ctx = ctxFor(['maestro', '--udid', '61031VDCR0004B', 'test', 'flow.yaml']); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + 'maestro', '--udid', '61031VDCR0004B', 'test', + '-e', 'PERCY_SERVER=http://localhost:5338', + 'flow.yaml' + ]); + }); + + it('injects after `test` when --device precedes the subcommand', () => { + const ctx = ctxFor(['maestro', '--device', 'Pixel-10', 'test', 'flow.yaml']); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + 'maestro', '--device', 'Pixel-10', 'test', + '-e', 'PERCY_SERVER=http://localhost:5338', + 'flow.yaml' + ]); + }); + + it('injects after `test` when --platform=android (= form) precedes the subcommand', () => { + const ctx = ctxFor(['maestro', '--platform=android', 'test', 'flow.yaml']); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + 'maestro', '--platform=android', 'test', + '-e', 'PERCY_SERVER=http://localhost:5338', + 'flow.yaml' + ]); + }); + + it('injects after `test` when multiple parent flags precede the subcommand', () => { + const ctx = ctxFor([ + 'maestro', '--udid', '61031VDCR0004B', '--platform', 'android', + 'test', 'flow.yaml' + ]); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + 'maestro', '--udid', '61031VDCR0004B', '--platform', 'android', 'test', + '-e', 'PERCY_SERVER=http://localhost:5338', + 'flow.yaml' + ]); + }); + + it('detects deeper -e PERCY_SERVER override when global flags precede `test`', () => { + const argv = [ + 'maestro', '--udid', '61031VDCR0004B', 'test', + '-e', 'PERCY_SERVER=http://custom:9999', 'flow.yaml' + ]; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); }); describe('maybeInjectScreenshotDir', () => { @@ -349,5 +407,49 @@ describe('percy app:exec', () => { 'flow.yaml' ]); }); + + // Mirror the maestro-server tests: `test` may follow global parent + // flags (--udid, --device, --platform, ...), so the helper must + // locate `test` and splice --test-output-dir right after it. + it('injects --test-output-dir after `test` when --udid precedes the subcommand', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const expectedDir = path.join(process.cwd(), '.percy-out'); + const ctx = ctxFor(['maestro', '--udid', '61031VDCR0004B', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx); + expect(mkdir).toHaveBeenCalledWith(expectedDir, { recursive: true }); + expect(ctx.argv).toEqual([ + 'maestro', '--udid', '61031VDCR0004B', 'test', + '--test-output-dir', expectedDir, + 'flow.yaml' + ]); + }); + + it('injects --test-output-dir after `test` when --platform=android precedes the subcommand', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const expectedDir = path.join(process.cwd(), '.percy-out'); + const ctx = ctxFor(['maestro', '--platform=android', 'test', 'flow.yaml']); + maybeInjectScreenshotDir(ctx); + expect(mkdir).toHaveBeenCalledWith(expectedDir, { recursive: true }); + expect(ctx.argv).toEqual([ + 'maestro', '--platform=android', 'test', + '--test-output-dir', expectedDir, + 'flow.yaml' + ]); + }); + + it('detects deeper customer --test-output-dir when global flags precede `test`', () => { + const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {}); + const ctx = ctxFor([ + 'maestro', '--udid', '61031VDCR0004B', 'test', + '--test-output-dir', '/custom/path', 'flow.yaml' + ]); + maybeInjectScreenshotDir(ctx); + expect(mkdir).not.toHaveBeenCalled(); + expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe('/custom/path'); + expect(ctx.argv).toEqual([ + 'maestro', '--udid', '61031VDCR0004B', 'test', + '--test-output-dir', '/custom/path', 'flow.yaml' + ]); + }); }); });