Skip to content

feat(cli): self-hosted (non-BrowserStack) Maestro + Percy support — V1#2261

Open
Sriram567 wants to merge 14 commits into
masterfrom
feat/self-hosted-maestro-percy-v1
Open

feat(cli): self-hosted (non-BrowserStack) Maestro + Percy support — V1#2261
Sriram567 wants to merge 14 commits into
masterfrom
feat/self-hosted-maestro-percy-v1

Conversation

@Sriram567
Copy link
Copy Markdown
Contributor

@Sriram567 Sriram567 commented Jun 2, 2026

Summary

Adds full self-hosted (non-BrowserStack) Maestro + Percy support to the CLI, plus follow-on UX completion. Customers running maestro test on their own infrastructure (local dev machines, Maestro Cloud, customer device labs) can now use Percy visual testing identically to BrowserStack App Automate customers — same SDK, same flow shape, same Percy dashboard outcomes.

Consolidates and supersedes four PRs into a single review surface:

  • #2248 — relay file-find + iOS port cascade (V1 foundation)
  • #2254 — auto-inject -e PERCY_SERVER + PERCY_CLI_API alias (V1 UX)
  • #2263 — auto-resolve PERCY_MAESTRO_SCREENSHOT_DIR + --test-output-dir + WARN on no-addr injection skip (V1.1 UX completion)
  • #2264 — explicit runtime field gates /percy/maestro-screenshot self-hosted detection (V1.1 architecture cleanup)

All 8 commits, single review surface, single CI run, single merge.

What changes

@percy/core/percy/maestro-screenshot relay (5 commits)

The relay handler at core/src/api.js previously required sessionId to find the screenshot file via BS's /tmp/{sessionId}_test_suite/logs/*/screenshots/<name>.png convention. After this PR, the handler treats sessionId as optional and falls back to a PERCY_MAESTRO_SCREENSHOT_DIR-scoped recursive glob for the self-hosted path.

  • Self-hosted file-find — when sessionId is absent, the relay searches under PERCY_MAESTRO_SCREENSHOT_DIR (customer-exported or auto-set, see below) with fast-glob + dot: true (so .maestro/ default dirs are reachable). Realpath + trailing-separator prefix check defends against sibling-prefix bypass.
  • iOS element-region resolver — when sessionId is absent (self-hosted iOS), a new port cascade resolves the Maestro driver: explicit PERCY_IOS_DRIVER_HOST_PORT override → deterministic 127.0.0.1:7001 probe (Maestro ≤ 2.4.0) → range 7001-7128 probe → lsof-based maestro-driver-iosUITests.xctrunner listener discovery (Maestro ≥ 2.6.0 ephemeral-port mode) → graceful warn-skip with actionable log. Per-Percy-instance cache (iosPortCache) — same pattern as grpcClientCache.
  • Explicit runtime field gating (squashed from feat(core): explicit runtime field gates maestro-screenshot self-hosted detection (R2) #2264) — relay now reads runtime: "selfhosted" | "browserstack" from the SDK payload as the primary self-hosted-vs-BS signal. Falls back to sessionId-absent for older SDKs that predate the field. Two percy.log.debug lines surface bidirectional inconsistencies (runtime + sessionId disagree) for diagnostic surface, never failing the request. Type validation: non-string runtime is rejected; unknown string values fall through to the legacy fallback. Companion SDK change lands in percy/percy-maestro-app#7.
  • R7 invariant preserved — when sessionId is present (BS App Automate) and/or runtime: "browserstack" is declared, the relay's BS code path runs byte-identically to today. The parseIosDriverHostPort range was relaxed from BS-specific 11100-11110 to any valid TCP port 1-65535 (the BS canonical range is a strict subset; BS port 11100 continues to parse identically).

@percy/cli-exec + @percy/cli-appapp:exec UX completion (3 commits)

  • PERCY_CLI_API env alias in cli-exec/src/exec.js alongside the existing PERCY_SERVER_ADDRESS export. Aligns with percy-appium-python's SDK env-var reader (PERCY_CLI_API) so SDK clients auto-align to whatever port app:exec picked — no manual port-pairing needed for multi-device.
  • Auto-inject -e PERCY_SERVER for maestro test in cli-app/src/exec.js. 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. Customer's only channel is Maestro -e flags. app:exec already knows the resolved port via percy.address() — this PR threads it into Maestro's argv automatically. Detector is conservative: basename(argv[0]) === 'maestro' && argv[1] === 'test', customer-supplied -e PERCY_SERVER=... wins. npx maestro and other shim invocations correctly fall through to the explicit--e pattern.
  • Auto-resolve PERCY_MAESTRO_SCREENSHOT_DIR + --test-output-dir (squashed from feat(cli-app): auto-resolve screenshot dir + WARN on no-addr injection (R1+R3) #2263) — cli-app now also auto-resolves the screenshot directory. Default: ${process.cwd()}/.percy-out (auto-mkdir -p'd). On mkdir failure (read-only CWD, EACCES, EROFS, EEXIST as a file), falls back to ${os.tmpdir()}/percy-maestro-${process.pid} with a WARN log. Customer-set PERCY_MAESTRO_SCREENSHOT_DIR env var or customer-supplied --test-output-dir flag always wins; when both are set, the helper is fully passive. The env-var and the flag are always aligned to the same path — the SDK reads the env var; Maestro reads the flag.
  • WARN log on maybeInjectMaestroServer no-addr skip (squashed from feat(cli-app): auto-resolve screenshot dir + WARN on no-addr injection (R1+R3) #2263) — when percy.address() is falsy (percy disabled, start failed), the auto-inject silently skipped before. Now emits a log.warn explaining what won't happen (-e PERCY_SERVER not injected; snapshots will NOT be uploaded; set PERCY_TOKEN and re-run). Customer-supplied -e PERCY_SERVER override skip stays silent — that's legitimate flow control, not a problem.

Customer-facing outcome

The minimum-viable self-hosted Maestro+Percy command goes from:

# Before this PR
export PERCY_SERVER_ADDRESS=http://localhost:5338  # (irrelevant to GraalJS)
export PERCY_MAESTRO_SCREENSHOT_DIR=$PWD/.percy-out
percy app:exec -- maestro test \
  --test-output-dir $PERCY_MAESTRO_SCREENSHOT_DIR \
  -e PERCY_SERVER=http://localhost:5338 \
  flow.yaml

to just:

# After this PR
percy app:exec -- maestro test flow.yaml

-e PERCY_SERVER, PERCY_MAESTRO_SCREENSHOT_DIR, and --test-output-dir are all auto-resolved by app:exec. Customer overrides (explicit env vars or argv flags) continue to win.

Testing

  • @percy/core — full test suites pass on every package. Includes 11 new self-hosted cascade tests in maestro-hierarchy.test.js (iOS port cascade scenarios: 7001 probe hit, range secondary, lsof single match, lsof zero match, lsof multi-match-must-not-guess, explicit-port-out-of-legacy-range, wrong-service falls through, cache hit, schema-drift, BS regression-guard, Android regression-guard). 9 new test scenarios in api.test.js for the runtime field gating (8-way matrix of runtime × sessionId presence + type-validation 400). 2 deferred tests for memfs+fast-glob dot-prefix interaction (tracked separately as a test-infra fix; production code is correct, verified via local repro).
  • @percy/cli-exec — new test asserts PERCY_CLI_API matches PERCY_SERVER_ADDRESS in spawned child env. CLEAN_PERCY_ENV fixture in cli-doctor/test/env-audit.test.js updated for hermeticity.
  • @percy/cli-app — 30/30 specs pass. 14 scenarios for maybeInjectMaestroServer (happy path injection, customer override at multiple positions in argv, basename match on absolute paths, npx maestro/python/appium skip, maestro hierarchy/list-devices skip, percy-disabled skip, multi-device port isolation). 14 new scenarios for maybeInjectScreenshotDir (happy path; env override; flag override; both set; EACCES / EROFS / EEXIST fallbacks; hierarchy / npx / python / short-args skips; basename match on absolute paths). 2 scenarios for the WARN-log-on-no-addr-skip behavior (fires on no-addr; does NOT fire on customer-override).
  • End-to-end validation — Android Pixel 10 + iOS iPhone 16 simulator (build ⬆️ Bump @oclif/config from 1.16.0 to 1.17.0 #14 baseline + ⬆️ Bump eslint from 7.5.0 to 7.7.0 #20 comparison) confirm regions resolve correctly through the relay; BS App Automate canary builds on Android (host 183.177.55.134) and iOS (host 103.234.68.130) confirm R7 — BS path produces snapshots identically pre/post these changes. See PER-8599 status update for the full validation matrix.

Companion SDK

  • percy/percy-maestro-app#7 — SDK-side changes: server default http://percy.cli:5338http://localhost:5338 (BS-safe; BS continues to inject PERCY_SERVER explicitly), session-id upload gate relaxed (allow uploads without sessionId), PERCY_SERVER_ADDRESS env fallback added to the read order, explicit runtime field added to the /percy/maestro-screenshot POST payload (companion to the runtime gating in this PR). Published as @percy/maestro-app v1.0.0-Beta.0+.

Documentation

Post-Deploy Monitoring & Validation

  • What to monitor
    • R7 — BS path unchanged: tail percy_cli.<session>_<port>.log on BS Android (183.177.55.134) and iOS (103.234.68.130) canary hosts; confirm Snapshot taken + Finalized build lines appear with sessionId=... for every Maestro session.
    • Self-hosted relay: customer reports of 404 Screenshot not found or Snapshot command was not called self-hosted. Honeycomb relay-handler latency / error-rate dashboard.
    • iOS port cascade: CLI debug log runIosHttpDump ok ... port=7001 for self-hosted iOS sessions confirms the deterministic path is hitting. lsof fallback lines appear only on Maestro ≥ 2.6.0.
    • runtime field bidirectional-consistency log: two percy.log.debug lines now appear when SDK's runtime declaration disagrees with sessionId presence. Non-zero rate in production indicates a customer misconfiguration or a future BS-host bug.
    • Auto-screenshot-dir fallback rate: customers landing on ${TMPDIR}/percy-maestro-<pid> instead of ${CWD}/.percy-out would emit a WARN line; > 5% rate could indicate widespread read-only CWDs.
  • Validation checks
    • Re-run the canonical validation flow from the self-hosted validation runbook post-deploy. Expected: Android Pixel + iOS simulator both produce baseline #N + comparison #N+1 with all snapshots Unchanged when regions cover the dynamic content.
    • BS canary regression: trigger one Maestro v2 build on each of the canary hosts. Expected: passed, identical snapshot count to pre-deploy, sessionId visible in CLI relay log.
    • Auto-screenshot-dir UX: run example-percy-maestro-selfhosted's Android quickstart with no env exports → expect green Percy build with snapshots in ${CWD}/.percy-out/screenshots/.
  • Expected healthy signals
    • [percy] Percy CLI healthcheck passed. on every SDK init.
    • runIosHttpDump ok sid=none nodes=N via maestro-http (self-hosted, port=7001) on every self-hosted iOS element-region resolution.
    • Zero Snapshot command was not called errors finalizing self-hosted builds.
    • Bidirectional-consistency debug lines remain quiet in production (no SDK is sending inconsistent payloads).
  • Failure signals / rollback triggers
    • Any BS Maestro build that previously passed reports Snapshot command was not called post-deploy → revert (R7 break).
    • parseIosDriverHostPort parsing the BS canonical port (11100) rejects with out-of-range → revert (range relaxation regressed).
    • Multi-device self-hosted runs where two app:exec --port invocations show the same injected PERCY_SERVER → investigate isolation.
    • Customer reports where the auto-resolved screenshot dir doesn't match where Maestro actually writes → revert (alignment contract broken).
  • Validation window & owner
    • Window: 7 days post-merge with focus on BS Maestro v2 builds.
    • Owner: Arumulla Sri Ram (arumulla@browserstack.com) + the Percy team.

Closes / Supersedes

🤖 Generated with Claude Opus 4.7 via Claude Code + Compound Engineering v2.54.0

Sriram567 added 6 commits June 2, 2026 16:42
…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).
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.
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.
Sriram567 added 2 commits June 2, 2026 17:36
…put-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.
…lf-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)
Sriram567 and others added 6 commits June 3, 2026 16:07
…lay 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>
…tro-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>
…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>
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>
…291)

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>
@Sriram567
Copy link
Copy Markdown
Contributor Author

Claude Code PR Review

PR: #2261Head: 67a14bdReviewers: stack:code-reviewer

Summary

Adds self-hosted (non-BrowserStack) Maestro + Percy support: the /percy/maestro-screenshot relay makes sessionId optional and scopes file lookup to PERCY_MAESTRO_SCREENSHOT_DIR (recursive dot:true glob + realpath/prefix guard); a runtime field makes self-hosted detection explicit; an iOS element-region resolver discovers the Maestro driver port (probe 127.0.0.1:7001 → lsof → warn-skip); and @percy/cli-app/cli-exec auto-inject -e PERCY_SERVER and --test-output-dir into maestro test.

Review Table

Priority Category Check Status Notes
High Security No hardcoded secrets or credentials Pass Tokens flow via env; none committed.
High Security Authentication/authorization checks present N/A Localhost relay; auth model unchanged by this PR.
High Security Input validation and sanitization Pass SAFE_ID name regex, runtime type check, regions shape validation, filePath rejected in self-hosted mode.
High Security No IDOR — resource ownership validated Pass realpath + scopeRoot prefix check defeats symlink/traversal escape; sessionId scoping on BS path.
High Security No SQL injection (parameterized queries) N/A No SQL.
High Correctness Logic is correct, handles edge cases Pass One Medium (iOS port-cache eviction) + one Low (BS /tmp/undefined 404) noted below; neither is a High-severity break.
High Correctness Error handling is explicit, no swallowed exceptions Pass glob/realpath failures map to explicit 404s; lsof failures degrade to warn-skip.
High Correctness No race conditions or concurrency issues Pass PERCY_MAESTRO_SCREENSHOT_DIR read at request time (Low smell — see findings), but stable in practice.
Medium Testing New code has corresponding tests Pass Thorough branch coverage (100% gate enforced and met).
Medium Testing Error paths and edge cases tested Pass lsof failure modes, invalid ports, no-cache, probe-fail, multi-match all covered.
Medium Testing Existing tests still pass (no regressions) Pass Test @percy/core green on head.
Medium Performance No N+1 queries or unbounded data fetching Pass Self-hosted glob is unbounded in theory (Low — see findings); bounded in practice.
Medium Performance Long-running tasks use background jobs N/A No new long-running work.
Medium Quality Follows existing codebase patterns Pass Mirrors existing maestro/grpc cache + resolver patterns.
Medium Quality Changes are focused (single concern) Pass All scoped to the self-hosted Maestro feature.
Low Quality Meaningful names, no dead code Pass PERCY_CLI_API introduced without a shown consumer (Low — see findings).
Low Quality Comments explain why, not what Pass Strong rationale comments throughout.
Low Quality No unnecessary dependencies added Pass No new deps.

Findings

  • File: packages/core/src/maestro-hierarchy.js:1329-1334

  • Severity: Medium (reviewer rated High; downgraded — graceful degradation, not a break)

  • Reviewer: stack:code-reviewer

  • Issue: In resolveSelfHostedIosPort, the cache-hit path runs runIosHttpDump on the cached port but returns its result unconditionally. If the Maestro driver restarts on a new port mid-session, the cache keeps pointing at the dead port; every subsequent element-region resolution fails and never re-discovers. The code re-probes for liveness (per its comment) but doesn't act on a dead result.

  • Suggestion: On a non-hierarchy/no-aut-tree result, null the cache and fall through to re-discovery: if (result.kind === 'hierarchy' || result.kind === 'no-aut-tree') return { port: cached, result }; iosPortCache.port = null; — plus a test for the eviction path. (Impact is bounded: element regions degrade to skip; screenshots + coordinate regions still upload.)

  • File: packages/core/src/api.js:537-540

  • Severity: Low (reviewer rated High; downgraded — intended, tested behavior)

  • Reviewer: stack:code-reviewer

  • Issue: runtime=browserstack with no sessionId builds scopeRoot=/tmp/undefined_test_suite and 404s. The reviewer suggested a 400 instead.

  • Suggestion: Optional: fail fast with 400 sessionId required when runtime="browserstack". Note this is the author's documented design — api.test.js:1305 deliberately asserts the 404 ("trust the runtime field"), /tmp/undefined_test_suite won't exist, and there is no security exposure. Cosmetic; safe to leave.

  • File: packages/core/src/api.js:524-536

  • Severity: Low

  • Reviewer: stack:code-reviewer

  • Issue: PERCY_MAESTRO_SCREENSHOT_DIR is operator-controlled and interpolated into the fast-glob pattern; glob metacharacters ({, [, *) in it could malform the pattern. Contained after-the-fact by the realpath/prefix check.

  • Suggestion: Optional defensive check rejecting glob metacharacters in the dir; low priority since it's operator- not caller-controlled.

  • File: packages/cli-app/src/index.js / cli-exec/src/exec.js:89

  • Severity: Low

  • Reviewer: stack:code-reviewer

  • Issue: maybeInjectScreenshotDir mutates process.env and is on the public export surface; PERCY_CLI_API is set to the same value as PERCY_SERVER_ADDRESS with no shown consumer.

  • Suggestion: Keep injection helpers internal; document PERCY_CLI_API's purpose / link the consuming SDK change.

  • Reviewer self-resolved (no action): lsof port regex with -nP output, the args.length - 1 scan bound, and the post-splice --test-output-dir index were all examined and found correct.


Verdict: PASS — well-engineered, layered file-access security, and unusually thorough tests; no Critical/High defects. The Medium iOS port-cache eviction is a worthwhile follow-up (recommend addressing) but degrades gracefully and does not block merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant