feat(core): self-hosted (non-BrowserStack) Maestro relay support#2248
Closed
Sriram567 wants to merge 4 commits into
Closed
feat(core): self-hosted (non-BrowserStack) Maestro relay support#2248Sriram567 wants to merge 4 commits into
Sriram567 wants to merge 4 commits into
Conversation
…ESTRO_SCREENSHOT_DIR scope
`/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 <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 `<root>/**/<name>.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).
…o/` default Maestro's default output directory is `.maestro/` (dot-prefixed). When a self-hosted customer runs `maestro test` without `--test-output-dir`, screenshots land under `<cwd>/.maestro/...`. fast-glob's default is `dot: false`, so the recursive `<root>/**/<name>.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).
…of + warn-skip
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 ef66ccc/8ac60b87; companion SDK + docs
in percy/percy-maestro-app#7).
…hosted-no-driver 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 ccb9bc0 (Unit 2).
Sriram567
added a commit
to percy/percy-maestro-app
that referenced
this pull request
May 28, 2026
…vice run
Marks status: validated-2026-05-28. The runbook stub anticipated this
backfill ("Backfill on first real run") — these are the verified
commands, build IDs, and the two gotchas discovered during the first
real-device acceptance run on a Pixel 10 / Android 16 / Maestro 2.4.0
host.
Validated:
- /percy/maestro-screenshot relay resolves PERCY_MAESTRO_SCREENSHOT_DIR-
scoped screenshots with no sessionId (Unit 1).
- SDK uploads successfully self-hosted with PERCY_SESSION_ID omitted from
the payload (Unit 3).
- Coordinate regions are forwarded SDK → relay → Percy comparison engine.
Baseline build #50215463 (7×8=56) approved; comparison build #50215516
(9×9=81) reported `02_CalcResult` as Unchanged — masked area's diff
ignored, canonical regions-working demonstration.
Two gotchas captured for the next person validating self-hosted on a
fresh machine:
1. Maestro hangs silently on the driver-APK install step without
JAVA_TOOL_OPTIONS="-Djava.net.preferIPv4Stack=true". The hang is in
dadb trying to talk to adb over IPv6 loopback. Reproduces with both
Maestro 2.6.0 and 2.4.0, with Play Protect disabled, on Pixel 10 +
Android 16 + macOS 15.5 (Apple Silicon). Independent of Percy —
`maestro test` alone hangs identically without the env var.
2. Maestro's GraalJS runScript context does NOT inherit the parent
process's environment. PERCY_SERVER_ADDRESS exported by percy
app:exec is invisible to the SDK. Customer must pass
`-e PERCY_SERVER=http://localhost:5338` as a Maestro -e flag on every
`maestro test` invocation. This is the one extra config item required
for self-hosted vs. BrowserStack.
iOS validation pending (no iOS device at the time of this run); the
runbook captures the runtime-verify items for that next pass.
Plan: docs/plans/2026-05-27-001-feat-self-hosted-maestro-percy-plan.md
Companion CLI PR: percy/cli#2248
This was referenced May 29, 2026
Contributor
Author
|
Consolidated into #2261 (single self-hosted Maestro+Percy V1 PR — all 6 commits, same content). Closing this stack-base in favor of the unified review surface. |
Sriram567
added a commit
that referenced
this pull request
Jun 23, 2026
#2261) * feat(core): self-hosted Maestro relay — sessionId optional + PERCY_MAESTRO_SCREENSHOT_DIR scope `/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 <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 `<root>/**/<name>.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). * fix(core): self-hosted glob — enable `dot: true` for Maestro `.maestro/` default Maestro's default output directory is `.maestro/` (dot-prefixed). When a self-hosted customer runs `maestro test` without `--test-output-dir`, screenshots land under `<cwd>/.maestro/...`. fast-glob's default is `dot: false`, so the recursive `<root>/**/<name>.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). * feat(core): iOS self-hosted element-region resolver — probe 7001 + lsof + warn-skip 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 ef66ccc/8ac60b87; companion SDK + docs in percy/percy-maestro-app#7). * test(core): cross-platform parity — iOS env-missing now returns self-hosted-no-driver 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 ccb9bc0 (Unit 2). * 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. * feat(cli-app): auto-inject -e PERCY_SERVER for `maestro test` 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:<port>` 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. * feat(cli-app): auto-resolve PERCY_MAESTRO_SCREENSHOT_DIR + --test-output-dir; WARN on no-addr injection skip (#2263) 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 <same path> 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-<pid> 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. * feat(core): explicit runtime field gates /percy/maestro-screenshot self-hosted detection (R2) (#2264) 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) * fix(core): correct self-hosted maestro runtime routing + stabilize relay tests - 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) <noreply@anthropic.com> * test(core): cover self-hosted maestro coverage gaps (api.js:667, maestro-hierarchy else) 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) <noreply@anthropic.com> * test(core): cover remaining maestro-hierarchy iOS self-hosted branch gaps 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) <noreply@anthropic.com> * test(core): fix lsof multi-match test row format to cover the >1 branch 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) <noreply@anthropic.com> * test(core): cover the no-port lsof row branch (maestro-hierarchy.js:1291) 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) <noreply@anthropic.com> * refactor(core,cli-exec): address Rishi review — drop runtime field + PERCY_CLI_API alias - 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 <noreply@anthropic.com> * feat(cli-app): prescribe iOS Maestro driver port via --driver-host-port injection @percy/cli-app's `app:exec` callback now auto-injects --driver-host-port <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 <noreply@anthropic.com> * refactor(core): remove iOS port-discovery cascade after prescribe shift 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * test(core): use os.tmpdir() for self-hosted maestro fixtures (fix Windows CI) 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 <noreply@anthropic.com> * fix(core): make self-hosted maestro relay path-handling Windows-portable 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 <noreply@anthropic.com> * fix(core): forward-slash normalize both sides of self-hosted realpath check The previous attempt at Windows portability (commit 9c8754a) 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 <noreply@anthropic.com> * fix(self-hosted maestro): align iOS port discovery with Maestro 2.4.0 CLI surface 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 <noreply@anthropic.com> * fix(cli-app): inject maestro env+output-dir past global parent flags 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 <serial> 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 <value> test`, `--device <value> test`, `--platform=android test` (equals form), multiple parent flags chained, and deeper customer overrides for both helpers. * fix(cli-app,core): address PR review findings — equals-form, TOCTOU comment, mtime coverage Three issues raised on PR #2261; all are low-or-medium robustness fixes with no semantic change to the load-bearing security or R7 invariants. cli-app — equals-form `--test-output-dir=<path>` (Medium) ========================================================= `findTestOutputDirValueIdx` only matched the bare `--test-output-dir` token, so the picocli equals-form `--test-output-dir=/my/dir` slipped through undetected. The helper then thought no flag was set, spliced in a second `--test-output-dir` pointing at the auto-resolved `.percy-out`, AND overwrote `PERCY_MAESTRO_SCREENSHOT_DIR`. Maestro would write screenshots to the customer's `=`-form path while the relay scanned the auto-resolved path → silent all-404 snapshots. Renamed to `findTestOutputDirValue` and made it return the value string (or null) so both space- and equals-forms are handled uniformly. Caller updated to consume the value directly instead of indexing back into argv. Adds three new spec scenarios: - `honors customer-supplied --test-output-dir=<value> equals-form` - `honors --test-output-dir=<value> when global flags precede test` - `treats env var as override when both env and equals-form are set` core — TOCTOU stat→realpath (Low) ================================== There is a small TOCTOU window between the pre-flight `stat` existence check on `PERCY_MAESTRO_SCREENSHOT_DIR` and the subsequent `realpath`+sep-prefix containment check. The window is harmless because `realpath` (not `stat`) is the security invariant — even if the dir is replaced with a symlink in between, `realpath` resolves the target and the prefix check rejects anything outside scopeRoot. This commit only strengthens the comment around the `stat` to make explicit that it is a UX guard (actionable 400 vs opaque 404) and that `realpath` is the load-bearing security boundary. No behavior change. core — multi-match mtime selection coverage (Low) ================================================== The relay's most-recently-modified selection across multiple PNG matches under `PERCY_MAESTRO_SCREENSHOT_DIR` was exercised only by BS-side integration tests. Adds a focused unit test under the existing `self-hosted (sessionId absent)` block: two PNG fixtures at different depths with distinct dimensions and explicitly-stamped mtimes; asserts the newer one's dimensions reach the upload payload. * fix(core): align iOS-env-missing parity test with current 7001-probe behavior CI `Test @percy/core` was failing on a stale parity test in `maestro-hierarchy.parity.test.js:92`. The test was authored under the deprecated "prescribe-don't-discover" iteration (cascade removed) and stubbed `httpRequest` as a bare `jasmine.createSpy('httpRequest')` — returning undefined. When commit 8b60341 restored the single 127.0.0.1:7001 probe on iOS dispatch, the bare spy started getting called and crashed: TypeError: Cannot destructure property 'statusCode' of '((cov_x5ynemye3(...).s[213]++) , response)' as it is undefined. at maestro-hierarchy.js:1308 Update the spy to `.and.callFake(throw ECONNREFUSED)` — same pattern as the equivalent test in `maestro-hierarchy.test.js:537` — and assert the probe IS attempted exactly once on port 7001 before the warn-skip. The parity invariant (cross-platform envelope shape) is preserved; the internal expectation just matches current production behavior. * test(core): cover no-aut-tree branch in iOS self-hosted 7001 probe CI `Test @percy/core` on Linux failed coverage at 99.91% branches. maestro-hierarchy.js:1305 — the `recordResolverFinalFailure` call inside the `probe.kind === 'no-aut-tree'` arm of the self-hosted 7001 probe — had no test exercising it. Add a focused test: env unset, httpRequest returns a SpringBoard-only axElement tree (identifier=com.apple.springboard, no AUT child). The runIosHttpDump returns kind: 'no-aut-tree' with reason: 'springboard-only'; the dispatch records the classified failure and returns the probe result directly so callers surface the actionable "AUT not foregrounded" diagnostic instead of the generic env-missing warn-skip. * fix(cli-app): treat empty --test-output-dir value as absent Caught in PR review on `db6fa66d`. The post-review-fix `findTestOutputDirValue` returns the value-string OR null. An empty value (`--test-output-dir=` with no path, or space-form `--test-output-dir ""`) slipped through as `""`, and the caller's `flagSet = existingFlagValue !== null` treated `""` as a customer-set value — bypassing both the argv splice and the auto-resolve fallback. The SDK then read an empty `PERCY_MAESTRO_SCREENSHOT_DIR` while Maestro defaulted its own output dir, producing the same all-404 silent failure the equals-form work was meant to prevent in the opposite direction. Treat empty values as absent so the helper falls through to the auto- resolve path (`${CWD}/.percy-out`). Both forms covered: - `--test-output-dir=` (equals-form, empty) - `--test-output-dir ""` (space-form, empty value token) Also: fix the comment in the multi-match mtime test in api.test.js. The risk isn't "sub-millisecond filesystems" (CI filesystems are coarser, not finer); the 60-second gap exists to survive 1-second mtime resolution floors (ext4 without `relatime`). --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds non-BrowserStack ("self-hosted") support to the Maestro screenshot relay. Customers running
percy app:exec -- maestro test ...on their own devices, CI, or device farms can now produce Percy builds without BS host injection.Architecture, in one sentence:
sessionIdabsent on/percy/maestro-screenshotis the self-hosted detection signal (it's a BrowserStack host-injected marker that lives nowhere else in the CLI); when absent, the relay resolves a file-find scope root fromPERCY_MAESTRO_SCREENSHOT_DIR(process env, required, absolute existing directory — typically the customer'smaestro test --test-output-dir <DIR>path), globs<root>/**/<name>.png, and applies the same realpath + trailing-separator prefix check that protects the BS path today. The BrowserStack path is byte-identical — the existing/tmp/{sessionId}{_test_suite}glob, manual-walker fallback, and security check all run exactly as before for any request withsessionId.What changed
packages/core/src/api.js—sessionIdis optional;selfHosted = !sessionIdderives the mode. New scope-root resolution block validatesPERCY_MAESTRO_SCREENSHOT_DIR(absolute + existing dir, 400 with actionable message otherwise, emitted before realpath so it isn't masked as a 404). File-find branches onselfHostedfor the glob pattern; the realpath/prefix check is now rooted at the resolvedscopeRoot.filePathis rejected in self-hosted mode (the SDK never emits it; honoring it against a caller-influenceable root would re-open arbitrary in-root reads).packages/core/test/api.test.js— newself-hosted (sessionId absent)describe locks the new branches: happy path (recursive glob find + payload omits sessionId + PNG-fill works), env-var validation (unset/relative/non-existent/file-not-dir → 400),filePathrejected, file-missing → 404,name-traversal rejected, coordinate region pass-through. The existing "rejects missing sessionId" test is updated to reflect the new behavior (missing sessionId + missing env var → 400 with the new actionable message).Security invariants (relocated, not removed)
SAFE_IDonname(^[a-zA-Z0-9_-]+$) is load-bearing for the recursive glob — separators and traversal characters rejected./x/.maestrovs/x/.maestro-secrets).PERCY_MAESTRO_SCREENSHOT_DIRread fromprocess.envonly, never the request body — root cannot be moved per-call.filePathrejected self-hosted.What this PR does not include yet
This is the first of two cli commits for the feature; the iOS element-region resolver work (auto-discover port
7001→ BS-proven HTTP/viewHierarchyattach +lsofself-discovery fallback + graceful warn-skip, no cold-start tier) lands as a follow-up commit on this same branch. Marking the PR draft until it's in. SDK changes (@percy/maestro-appserver default + session-id gate relaxation) and docs land in a companion PR againstpercy/percy-maestro— also pending.Testing
/percy/maestro-screenshotdescribe) stays green — locks R7 (no BrowserStack regression).1536376returns 404 fromchromium-browser-snapshots, breaking unrelatedServer+ browser tests). Buildkite CI runs in a clean environment.Post-Deploy Monitoring & Validation
bs-nixpkgs.service.name=percy-apiwithbuild.client=maestro(host-side BS sessions) —snapshot_countshould stay at baseline;"Snapshot command was not called"should not appear.400from/percy/maestro-screenshoton BS sessions.bs-nixpkgsderivation bump. The change is gated so the BS path is byte-identical — any regression points at this PR.bs-nixpkgscli bump that includes this commit.percy app:exec -- maestro testoff-BS). Acceptance is the real-device gate in the plan (Android + iOS device builds with coordinate + element regions).Plan reference
Plan:
percy-maestro/docs/plans/2026-05-27-001-feat-self-hosted-maestro-percy-plan.md(cross-repo plan, not in this repo — Unit 1 of 4). Jira: PER-8599.🤖 Generated with Claude Opus 4.7 (200K context, extended thinking) via Claude Code