feat(core): cross-platform Maestro view-hierarchy resolver + screenshot filePath relay + fallback observability#2217
Merged
Merged
Conversation
- Add empty body guard (400 instead of TypeError) - Add Busboy fileSize limit to reject oversized uploads during parsing - Use Object.create(null) and field allowlist to prevent prototype pollution - Add stream error handler on Readable source - Use HTTP 413 for oversized files
Reject immediately on Busboy 'limit' event with 413 instead of setting fileBuffer to null which produced 'Missing required file part'.
Accepts {name, sessionId} as JSON, finds the screenshot file on disk
at /tmp/<sessionId>_test_suite/logs/*/screenshots/<name>.png,
base64-encodes it, and processes as a standard comparison.
This enables real-time Percy uploads from Maestro flows where the
JS sandbox cannot access screenshot files directly.
… metadata Accept statusBarHeight, navBarHeight, fullscreen from request instead of hardcoding 0/false. Transform coordinate-based regions to CLI boundingBox format. Add sync mode support via percy.syncMode() + handleSyncJob(). Forward thTestCaseExecutionId to comparison pipeline. Element-based regions log a warning and are skipped — ADB uiautomator resolution will be added as a follow-up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Platform-aware screenshot discovery:
- Accept platform field with strict whitelist (ios/android); 400 on unknown
- iOS glob: /tmp/{sessionId}/*_maestro_debug_*/{name}.png
- Android glob unchanged; backward compat with SDK v0.2.0 (no platform → Android)
Path-safety hardening:
- Tighten name/sessionId from blocklist to strict character-class allowlist
- fs.realpath canonicalization + session-root prefix check defeats symlink swap
- Handles macOS /tmp → /private/tmp symlink transparently
Pick most recently modified file when multiple match (iOS same-name-across-flows).
Introduces packages/core/src/adb-hierarchy.js with two plain exports: dump() and firstMatch(nodes, selector). The resolver: - Reads process.env.ANDROID_SERIAL; falls back to one adb devices probe (requires exactly one attached device to avoid wrong-device dumps under multi-session CLI concurrency). Never accepts serial from request input. - Shells out via cross-spawn with a 2s hard timeout (mirrors the browser.js:256-297 spawn+cleanup pattern). - Classifies results into one of three shapes — unavailable, dump-error, hierarchy — so the relay can distinguish environmental failures from transient dump failures. - Streams primary via adb exec-out uiautomator dump /dev/tty; falls back to file-based dump + cat only on wrong-mechanism signals (exit≠0 or missing <?xml prefix). Terminal signals (oversize / parse-error) do not retry — prevents attack amplification on adversarial payloads. - Slices the XML envelope to the first </hierarchy> (strips uiautomator's trailer line and defends against embedded adversarial XML blocks). - Enforces a 5MB stdout cap before parse. - Parses with fast-xml-parser configured for defense-in-depth (processEntities: false, allowBooleanAttributes: false). - Exposes firstMatch with pre-order DFS + strictly-anchored bounds regex; zero-area nodes are non-matches, negative coordinates (clipped views) are allowed. Adds fast-xml-parser ^4.4.1 as a new dependency of @percy/core. 27 unit tests cover the parser + selector logic and all classification branches via a parameter-injected execAdb seam. No real ADB calls; no filesystem or network access.
Wires the /percy/maestro-screenshot relay to the new adb-hierarchy
resolver. Replaces the existing element-region warn-and-skip stub with
actual resolution via ADB + uiautomator dump on Android.
Handler changes:
- Early 400 validation on region shape before file I/O or ADB work:
whitelist selector keys (resource-id/text/content-desc/class), require
exactly one selector key per region, string-typed value, length ≤512,
total regions per request ≤50.
- Android element regions: lazy dump on first element region, memoize
the result (including error classes) for the whole request. Pre-scan
element-region count so the skip warning reports N regions accurately.
Both unavailable and dump-error poison the rest of the request with
one warning — bounds worst-case per-request ADB time to one 2s timeout
regardless of element-region count (closes the timeout-accumulation
DoS vector).
- iOS element regions: preserve existing warn-and-skip semantics. Not
a 400. Avoids a breaking change for any iOS caller today.
- Coordinate regions: unchanged; still transform {top,bottom,left,right}
to elementSelector.boundingBox.
- Miss on element resolution: per-element warning, region skipped,
request still uploads.
First-ever /percy/maestro-screenshot handler tests cover input
validation (9 × 400 paths), coordinate-only flow regression, iOS
warn-and-skip behavior, end-to-end forwarding of testCase/labels/
thTestCaseExecutionId/tile-metadata/sync, and the missing-screenshot
404 path. ADB-integration paths (element resolution against a real
device) are covered by the adb-hierarchy unit tests and Unit 7 E2E
validation on BrowserStack Maestro.
E2E validation on BrowserStack Maestro against host 31.6.63.33 / Pixel 7 Pro showed the primary exec-out path intermittently returning no-xml-envelope and the file-dump fallback exiting 137 (SIGKILL of uiautomator on the device). The kill is triggered by concurrent uiautomator/automation activity on the device during a live Maestro session — not a device-wide or permissions issue (manual dumps from the shell return 44KB XML fine). A single 300ms-delayed retry of the fallback dump command recovers the common case without masking genuine device unavailability. If the second attempt also fails, we still fall through to the existing dump-error classification. Test: the adb-hierarchy spec adds a retry test where the first fallback exec returns 137 and the second returns the fixture XML; resolver returns hierarchy and fileDumpCalls == 2.
Strengthens the SIGKILL retry from a single 300ms attempt to three retries at 500ms/1s/2s (3.5s total window). Exits early as soon as a dump succeeds. Rationale: single short retry wasn't enough against persistent device contention observed during BrowserStack Maestro sessions. The wider budget catches transient uiautomator kills on less-contended devices while still failing fast on genuinely unavailable devices. Captured limitation: when Maestro holds uiautomator throughout a flow (its observed behavior on real devices), no reasonable retry count recovers — the mechanism itself needs to change (e.g., Maestro API integration or an accessibility-service sidecar). That's a Phase 2 follow-up, not part of this patch. Tests cover both the "succeeds on Nth retry" case and the "all retries exhausted" case.
E2E on BrowserStack Maestro showed `adb exec-out uiautomator dump` is
fundamentally incompatible with live Maestro flows — Maestro holds the
uiautomator lock throughout a flow and competing dumps get SIGKILLed.
The `maestro --udid <serial> hierarchy` CLI command reuses Maestro's
existing gRPC connection to dev.mobile.maestro on the device and works
reliably during live sessions (verified by probing twice mid-flow —
both probes returned valid JSON while the flow was running).
Changes in packages/core/src/adb-hierarchy.js:
- Primary dump mechanism is now `maestro --udid <serial> hierarchy`.
- Parse the resulting JSON (slice from the first `{` to tolerate banner
lines), flatten the tree into the existing node shape.
- Map `accessibilityText` → `content-desc` at flatten time so `firstMatch`
still uses the SDK's selector vocabulary unchanged.
- Maestro CLI timeout: 15s (JVM cold start ~9s + headroom).
- Honor `MAESTRO_BIN` env var for alternate paths; default `maestro`
on PATH.
- New `spawnWithTimeout` helper shared between maestro and adb code paths.
- Classification extended with maestro-specific reasons (`maestro-not-found`,
`maestro-timeout`, `maestro-no-device`, `maestro-no-json`,
`maestro-parse-error:*`, `maestro-spawn-error:*`, `maestro-exit-*`,
`maestro-oversize`).
Fallback: when maestro returns anything other than `hierarchy`, fall
through to the existing `adb exec-out uiautomator dump` flow (including
SIGKILL retry/backoff and file-dump fallback). Useful when the maestro
binary isn't installed on the CLI host.
Cost: 9s JVM cold start per screenshot that uses element regions.
Acceptable today because the alternative is 100% skip. Phase 2.2 follow-up:
replace the CLI invocation with a direct gRPC client against device port
6790 (typical latency <100ms) — infrastructure already in place (adb
forwards tcp:8206 → 6790 per device on BrowserStack hosts).
Tests: 36 specs total. New `dump (maestro hierarchy primary)` describe
block adds 7 scenarios (happy path, content-desc mapping, ENOENT→adb
fallback, unavailable propagation when both fail, timeout → adb recovery,
banner prefix tolerance, no-json). Existing 29 tests now inject an
execMaestro stub that reports ENOENT so they exercise the adb fallback
path exactly as before.
New module png-dimensions.js serves both:
- existing /percy/comparison/upload signature check (api.js import)
- upcoming /percy/maestro-screenshot iOS path (scale factor via
pngWidth / wda_window_logical_width + aspect-ratio landscape fallback)
Exports:
- PNG_MAGIC_BYTES (moved from api.js route-local scope)
- parsePngDimensions(buf) → {width, height} via IHDR hand-parse
(24-byte prefix read, no new dependency)
- isPortrait / isLandscape with default threshold 1.25
(iPad portrait ratio 1.334; margin empirically confirmable via A1 Probe 6)
- DEFAULT_ORIENTATION_THRESHOLD exported for override in tests / A1 Probe 6
Test-first: 17 specs covering happy path iPhone/iPad portrait+landscape,
dimensions > 65535, truncated buffer, bad signature, zero width/height,
non-Buffer input, threshold override, near-square ambiguity. All pass.
api.js: removes inline PNG_MAGIC_BYTES declaration from the upload route
handler; imports the shared constant. Upload signature-check behavior
unchanged.
Unit B1 of the iOS Maestro element-regions plan (v1.0); serves as the
Phase 1 CI coverage preflight per plan.
…-meta.json
Reader side of the realmobile ↔ Percy CLI contract v1.0.0 for iOS
element-region resolution on shared BS iOS hosts. Given a Maestro sessionId,
resolves /tmp/<sid>/wda-meta.json → { ok: true, port } or { ok: false, reason }
with TOCTOU-safe validation per contract §8:
File-level (SEI CERT POS35-C ordering, no lstat prefix):
- openSync(path, O_RDONLY | O_NOFOLLOW | O_NONBLOCK) — atomic symlink refusal;
ELOOP → 'symlink', ENOENT → 'missing', else → 'read-error'
- fstatSync on the opened fd — authoritative mode/uid/nlink check:
- st.mode mismatch 0o100600 → 'wrong-mode'
- st.uid mismatch getuid() → 'wrong-owner'
- st.nlink != 1 → 'multi-link' (hardlink attack per Apple Secure Coding
Guide, CVE-2005-2519 class)
- !st.isFile() → 'not-regular-file'
Content validation (contract §2):
- JSON.parse → 'malformed-json'
- schema_version semver-major != 1 → 'schema-version-unsupported'
(accepts 1.x minor bumps; unknown fields ignored for forward-compat)
- wdaPort out of 8400-8410 integer range → 'out-of-range-port'
- sessionId mismatch vs request → 'session-mismatch'
- flowStartTimestamp < getStartupTimestamp() - 5min → 'stale-timestamp'
Input guardrails:
- sessionId regex [A-Za-z0-9_-]{16,64} + null-byte/slash rejection →
'invalid-session-id' (path-traversal defense before any fs touch)
Log scrubbing (contract §5):
- All failure paths emit only the reason tag via logger.debug()
- No selector values, sessionIds, port numbers, paths, or uids in logs
- Verified by a cross-scenario scrub-assertion test
DI: { getuid, getStartupTimestamp } injected for deterministic tests.
22 specs pass. Tests use real fs tmpdirs (bypass memfs) because the module
relies on POSIX O_NOFOLLOW / hardlink semantics memfs doesn't implement.
… (B3)
Core iOS element-region resolver for /percy/maestro-screenshot. Single
GET /session/:sid/source per screenshot, parsed locally via fast-xml-parser,
mirrors the Android adb-hierarchy.js architecture.
Exports:
- resolveIosRegions({regions, sessionId, pngWidth, pngHeight, isPortrait, deps})
→ {resolvedRegions: [{elementSelector, boundingBox, algorithm}], warnings: []}
- shutdown() — aborts all in-flight WDA HTTP AbortControllers (wired to
percy.stop() by B4)
- XCUI_ALLOWLIST — exported Set of ~80 XCUIElement.ElementType values from
the Xcode 16 SDK (Apple XCUIElement.ElementType docs); serves as DoS
guardrail per WDA issue #292
Resolution path (A1-chosen):
1. Landscape gate (isPortrait arg)
2. Kill-switch gate (process.env.PERCY_DISABLE_IOS_ELEMENT_REGIONS from
startup env only; NOT tenant-forwarded via appPercy.env)
3. readWdaMeta dep returns port from realmobile-written wda-meta.json; port
validated in 8400-8410 range
4. GET /wda/screen (loopback-only) → scale from integer `scale` field;
fallback to width-ratio (pngWidth / logical_w) snapped to {2, 3};
fail-closed on raw ratio outside [1.9, 3.1]; LRU cache cap 64 per-session
5. GET /session/:sid/source (loopback-only):
- 20 MB response cap enforced BEFORE parse
- Pre-parse DOCTYPE/ENTITY regex rejection (primary XXE defense)
- fast-xml-parser with processEntities:false (defense-in-depth)
- Cached per screenshot; all regions reuse single fetch
6. Per region:
- Only `id` and `class` accepted in V1; `text`/`xpath` → selector-key-not-in-v1
- class short-form (Button) normalized to long-form (XCUIElementTypeButton);
rejected if normalized form not in allowlist → class-not-allowlisted
- selector > 256 chars → selector-too-long
- tree pre-order first match (zero-match on no-match)
- scale points → pixels, validate in-bounds + non-trivial area (≥4×4) →
bbox-out-of-bounds / bbox-too-small
- outbound elementSelector.class uses normalized long-form (canonical form
on Percy dashboard regardless of customer input style)
HTTP: @percy/client/utils#request via injectable httpClient dep; 500 ms
AbortController timeout per call; retries: 0 to keep timeout honest.
inflight Set tracks active controllers; shutdown() aborts all.
Log scrubbing (contract §5): reason tags only. Verified across all paths —
no selector values, sessionIds, ports, or coords in logs.
23 specs pass. Tests use an injectable fake httpClient + in-memory
handlers; no real network required.
…lay (B4) Wires B1/B2/B3 into api.js's /percy/maestro-screenshot handler. For iOS requests with element regions: 1. Parse IHDR from the already-read fileContent (one buffer read total — no extra fs hit). Failure → warn-skip all iOS element regions with png-unparseable; coord regions + screenshot upload continue. 2. Call resolveIosRegions() once per request with a real @percy/client/utils #request httpClient and a resolveWdaSession-wrapped readWdaMeta dep. 3. Surface each warning to percy.log.warn so support runbook tags are visible in Maestro stdout. 4. Walk the original regions array in input order; positional index into the sparse resolvedRegions produced by the resolver keeps coord and element regions interleaved correctly in the outbound Percy payload. wda-hierarchy now returns a SPARSE array (one entry per input element region; null = skipped) instead of a dense array. Preserves input ordering when element and coord regions are interleaved. All B3 unit tests updated accordingly (22 still pass). percy.js stop() invokes wdaHierarchyShutdown() before server.close() to abort in-flight WDA HTTP calls — http.request has no SIGKILL analog, so a slow /source fetch could otherwise keep the event loop alive past graceful-shutdown timeout. api.test.js: replaced the pre-V1 iOS stub test (which asserted "Element-based region selectors are not yet supported on iOS") with a V1 behavioral test that exercises the full iOS element-region pipeline with a real PNG IHDR header fixture (1170×2532 iPhone 14) and asserts V1 warn-skip semantics for an Android-style `resource-id` selector on iOS (not-in-V1 key). Test suite baseline: 28 pre-existing failures (chromium/doctor download tests unrelated to this change). After B4: 27 failures — same chromium/ doctor failures, plus the iOS stub test now passes with its updated V1 assertions. Zero iOS/wda-hierarchy/maestro-screenshot regressions. Kill-switch (PERCY_DISABLE_IOS_ELEMENT_REGIONS=1) read from Percy CLI process startup env inside wda-hierarchy.js per plan — host-level only, NOT forwarded from tenant appPercy.env (A0.3 property: pending staging verification).
…retries
Implements the three layered fixes documented in
percy-maestro/docs/solutions/integration-issues/ios-wda-session-id-and-node14-abortcontroller-2026-04-23.md.
Each addresses a distinct iOS-region failure mode that surfaced during
2026-04-23 BrowserStack live validation on host 52:
Fix C — Node 14 AbortController feature-detect (callWda):
BS iOS hosts pin to Node 14.17.3 (Nix). AbortController became a global
in Node 15. Without feature detection, the timeout path threw
ReferenceError caught by generic error handling and surfaced as the
same 'wda-error' tag as legitimate WDA failures, masking the other two
fixes during diagnosis. Now: typeof globalThis.AbortController guard +
Promise.race fallback. Adds diagnostic logging on /wda/screen failures
showing err.name/message/code/status/aborted/body.
Fix B — Stale WDA sessionId retry via error-envelope extraction:
WDA's session-scoped routes (/session/:sid/source) reject any sid that
isn't the currently-active session. Maestro spawns its own WDA session
per xctest run, so realmobile's write-time sid capture goes stale
during the test. Refactored fetchAndParseSource into tryFetchSource +
retry coordinator. On staleSession (`{ value: { error: 'invalid session
id' } }`), extracts the top-level `sessionId` from the error envelope
(authoritative for "currently active") and retries once. Falls back to
/status probe if the error body lacks a usable sid.
Fix A (reader side) — wdaSessionId surfacing per contract v1.1.0:
realmobile contract v1.1.0+ probes /status at write_wda_meta time and
surfaces the WDA UUID under wda-meta.json's optional `wdaSessionId`
field. wda-session-resolver now validates the field against
/^[A-Fa-f0-9-]{16,64}$/ (generous bounds for cross-version tolerance)
and surfaces it on the {ok, port, wdaSessionId?} return shape. v1.0.0
writers that omit the field cause callers to fall back to SDK
sessionId (the fast path 404s, then Fix B's retry recovers).
Tests cover all three paths: feature-detected timeout, staleSession
retry from error envelope, /status fallback when error body lacks sid,
v1.1.0 wdaSessionId pass-through, v1.0.0 absence handling, malformed
wdaSessionId rejection.
Note for downstream: this WDA-direct path is gated for deletion by the
2026-04-27 plan (percy-maestro/docs/plans/2026-04-27-001-feat-ios-element-regions-maestro-hierarchy-plan.md)
once Phase 0.5 empirical probe passes. Until then, this is the
production iOS resolver path.
The Android view-hierarchy resolver is becoming the cross-platform Maestro resolver (per percy-maestro/docs/plans/2026-04-27-001-feat-ios-element-regions-maestro-hierarchy-plan.md Unit 1). Rename + shim is purely additive — no behavior change. - Move src/adb-hierarchy.js → src/maestro-hierarchy.js (git mv preserves history). - Move test/unit/adb-hierarchy.test.js → test/unit/maestro-hierarchy.test.js. - Move test/fixtures/adb-hierarchy/ → test/fixtures/maestro-hierarchy/. - Replace src/adb-hierarchy.js with a 5-line re-export shim. Removed in V1.1 per the plan's deprecation guidance. - Update api.js import to ./maestro-hierarchy.js. - Update logger namespace from core:adb-hierarchy → core:maestro-hierarchy. - Update file header to reflect cross-platform intent (the file body has been maestro-first for some time; the previous file name was always misleading). - Update test describe block + import + fixture path. Behavior unchanged. Subsequent units in Phase 1 will add the iOS branch and api.js dispatch logic; this commit is just the rename so the diffs in those units stay focused.
…parity
Phase 1 Unit 2a per percy-maestro/docs/plans/2026-04-27-001-feat-ios-element-regions-maestro-hierarchy-plan.md.
Lands the platform-dispatch scaffolding and the cross-platform selector
vocabulary alias. Real iOS resolver implementation deferred to Unit 2b
post Phase 0.5 fixture capture (FIXME-PHASE-0.5 in code).
Platform dispatch:
- dump({ platform }) accepts 'android' (default — backwards compatible) or
'ios'. iOS branch reads PERCY_IOS_DEVICE_UDID + PERCY_IOS_DRIVER_HOST_PORT
from env (realmobile-injected per Unit 10a; the wda_port + 2700 formula
is realmobile-owned per maestro_session.rb:831). Warn-skip with
reason='env-missing' if either var is unset. Otherwise calls
runMaestroIosDump which currently returns
{ kind: 'unavailable', reason: 'not-implemented' } as the FIXME-PHASE-0.5
stub.
- iOS path never invokes adb (verified by test).
R1 vocabulary parity (Android `id` alias):
- flattenMaestroNodes (Android branch) now surfaces resource-id under both
`resource-id` AND `id` canonical keys on each node. Customer selectors
`{element: {id: "submit-btn"}}` and `{element: {resource-id: "submit-btn"}}`
resolve the same node. iOS users writing `{id: ...}` and Android users
writing the same yaml hit the same code path. Full unified-key migration
(deprecating `resource-id`) deferred to V1.1.
- SELECTOR_KEYS_UNION = [resource-id, text, content-desc, class, id]
drives firstMatch validation. ANDROID_SELECTOR_KEYS_WHITELIST and
IOS_SELECTOR_KEYS_WHITELIST exported separately for callers that want
per-platform validation.
Tests added:
- Android `id` alias resolves same bbox as `resource-id` (3 tests).
- iOS env-missing path (3 tests covering each env-var combination).
- iOS env-set returns 'not-implemented' (FIXME stub).
- iOS dispatch never invokes adb.
- Default (no platform arg) preserves Android behavior.
Smoke-tested via direct node import; full @percy/core test suite has 27
pre-existing failures in Unit / Install in executable Chromium (unrelated
infrastructure issue), but no regressions in the resolver tests.
Phase 1 Unit 3 per percy-maestro/docs/plans/2026-04-27-001-feat-ios-element-regions-maestro-hierarchy-plan.md.
Wires the maestro-hierarchy resolver into the /percy/maestro-screenshot
relay's iOS element-region dispatch, gated by an env switch so default
(unset) behavior is unchanged. Phase 0.5 empirical probe gates the
default flip to the new path; Phase 4 deletes the legacy iOS branch.
- New: read PERCY_IOS_RESOLVER from process.env. When equal to
'maestro-hierarchy', iOS element regions flow through the same
lazy maestroDump({ platform: 'ios' }) + per-region firstMatch
pattern Android already uses. When unset (or any other value),
legacy WDA-direct path remains active — no behavior change for
customers in production today.
- Refactor: the up-front PNG-parse + resolveIosRegions block now only
fires when the env switch is OFF. With the switch on, that work is
unnecessary (the resolver is engineered to be lazy + per-region).
- The cross-platform branch in the per-region loop now also covers iOS
when the switch is on. Same shape as Android: cachedDump lazy memo,
warn-skip on hierarchy-unavailable, firstMatch + bbox forward on
success.
Today (env switch unset): only the cross-platform Android path is
exercised. The iOS branch with the switch on is exercised by the
maestro-hierarchy unit tests landed in Unit 2a (which covers the
'env-missing' and 'not-implemented' stub paths). Unit 4 adds the
parity test that exercises both platforms via the same handler.
A real production rollout flips the default to 'maestro-hierarchy'
in Phase 4 (Unit 9) after Phase 0.5 PASSes; until then, keep the
default off.
Phase 1 Unit 4 per percy-maestro/docs/plans/2026-04-27-001-feat-ios-element-regions-maestro-hierarchy-plan.md.
New test file: test/unit/maestro-hierarchy.parity.test.js. Locks in the
contract that both platform branches return the same { kind, ... } envelope,
that the public API surface (SELECTOR_KEYS_WHITELIST + per-platform
whitelists) is consistent, and that platform dispatch isolates the env-var
reads (Android never reads PERCY_IOS_*; iOS never reads ANDROID_SERIAL).
Bug fix discovered during smoke test:
- flattenNodes (the XML/uiautomator code path) was missing the R1 `id`
alias surface that flattenMaestroNodes (the maestro CLI JSON path)
already had. So `firstMatch(nodes, { id: 'X' })` worked when nodes came
from the maestro path but returned null when nodes came from the adb
fallback path. Now both code paths surface resource-id under both
`resource-id` and `id` keys consistently.
iOS-side parity assertions in this test are scoped to what Unit 2a's stub
can actually cover — envelope shape, whitelist exports, dispatch isolation.
The Phase 4 follow-up (post Phase 0.5 + Unit 2b) extends this file with
real iOS attribute-mapping assertions backed by a captured iOS hierarchy
fixture.
Smoke-tested via direct node import. The full @percy/core test suite has
27 pre-existing Chromium-installer failures unrelated to this work.
…source
Source-synthesized fixtures for the new HTTP-XCTest path Unit 2 will build,
plus the maestro CLI iOS stdout shape for the fallback path. All shapes
verified against mobile-dev-inc/Maestro at ref=cli-2.0.7 (realmobile production
default per /usr/local/.browserstack/realmobile/config/constants.yml).
Notable findings recorded in capture-notes.md:
- PR #2365 has landed: server detects AUT itself; appIds is wire-vestigial.
Percy CLI can send {"appIds": [], "excludeKeyboardElements": false}.
YAML-based bundleId discovery is no longer required for the realmobile
fast path.
- PR #2402 has landed but with a different wrap from cli-1.39.13: response
is now {axElement: {children: [appHierarchy, statusBarsContainer]}, depth}
rather than [springboard, AUT]. The deepening pass's parser rule
('first elementType == 1 whose identifier != com.apple.springboard')
remains correct because the statusBars wrapper has elementType == 0.
- iOS Maestro's TreeNode does NOT carry a 'class' attribute. iOS selector
vocabulary is 'id' only (maps to attributes['resource-id']). The
originally absorbed Unit 2b XCUI elementType integer-to-name table is
not needed for selector matching.
Wire-bytes confidence boost is deferred to Unit 5/6/7 BS validation rather
than blocking on a Unit-1 BS session capture (see plan Viability Gate 2).
…(Unit 2)
Adds runIosHttpDump as the iOS primary path for /percy/maestro-screenshot
element regions: POST {appIds: [], excludeKeyboardElements: false} to Maestro's
iOS XCTestRunner /viewHierarchy endpoint at http://127.0.0.1:wda+2700. Server
detects AUT itself at cli-2.0.7+ (PR #2365 landed); empty appIds returns the
foreground AUT directly.
Replaces the iOS-WIP runMaestroIosDump stub with a real maestro-CLI shell-out
parser (the connection-class fallback path). Maestro's iOS CLI stdout is its
normalized TreeNode shape; existing flattenMaestroNodes consumes it without
iOS-specific code.
Adds flattenIosAxElement adapter for the HTTP path's raw AXElement shape:
walks to first elementType==1 with identifier!='com.apple.springboard' (skips
SpringBoard sibling on cli-1.39.13 wrap; works for both v1.39.13 [springboard,AUT]
and post-PR-2402 single-AUT-root shapes). Frame keys converted from PascalCase
{X,Y,Width,Height} to bracket-format bounds string.
Narrows IOS_SELECTOR_KEYS_WHITELIST from ['id', 'class'] to ['id'].
IOSDriver.mapViewHierarchy at cli-2.0.7 does not populate 'class' on iOS
TreeNode (only 'resource-id' from identifier), so Percy keeps iOS selector
vocabulary aligned with Maestro's actual capability.
Schema-class failures (missing root, missing frame, malformed JSON, 4xx,
non-JSON content-type) return dump-error without falling back. Connection-class
failures (ECONNREFUSED, ETIMEDOUT, ECONNRESET, 5xx) and no-aut-tree responses
(SpringBoard-only) fall back to the maestro-CLI path.
Two-tier deadline mirrors PR #2210's pattern: 1500ms healthy + 5000ms circuit-
breaker. Out-of-range PERCY_IOS_DRIVER_HOST_PORT (outside 11100-11110) skips
the HTTP path entirely. Loopback-only URL guard.
Drift-bit handling deferred to plan Unit 4 (cross-PR coordination with #2210);
schema-class failures currently log-only.
77 of 77 specs pass: 26 new iOS-path scenarios (HTTP primary, CLI fallback,
env handling, parity), and all existing Android tests unchanged.
Plan: percy-maestro/docs/plans/2026-05-06-004-feat-cross-platform-maestro-resolver-unification-plan.md
Fixtures: 65e54b9 (Unit 1)
…shot (Unit 3a)
Adds a three-tier cascade for iOS element-region resolver selection:
1. Per-snapshot override: `request.body.resolver` (validated against
['wda-direct', 'maestro-hierarchy']; HTTP 400 on unknown values).
2. Process env: `PERCY_IOS_RESOLVER` (same allowlist; unknown values
warn + fall through).
3. Default: 'wda-direct' (Unit 3a is opt-in only — Unit 3b's env-conditional
flip is a separate follow-up PR after the validation window).
The per-snapshot `resolver` body field is the ops escape valve documented
in the plan: lets operators `curl` a single snapshot with a specific
resolver for diagnostics without redeploying the CLI. SDK does not set
this today (R8 unchanged).
When the cascade chooses 'maestro-hierarchy', api.js calls the unified
`maestroDump({platform: 'ios', sessionId})` from Unit 2 (HTTP primary
+ CLI fallback). When 'wda-direct', the legacy `resolveIosRegions`
(WDA source-dump) path runs unchanged.
Threads sessionId from the relay request through to maestroDump for
log-scrubbed correlation tagging (Unit 2's `runIosHttpDump` uses sid
prefix in debug logs).
Tests: 6 new Unit 3a scenarios in api.test.js — body.resolver validation,
default-unchanged behavior, env-only path, per-snapshot override (both
directions), graceful fallback on garbage env values. All 6 pass; existing
tests unchanged.
Plan: percy-maestro/docs/plans/2026-05-06-004-feat-cross-platform-maestro-resolver-unification-plan.md
…heck (Unit 4)
Adds module-level maestroHierarchyDrift state with {android, ios} slots that
record the first schema-class failure per platform, plus the setter/getter
exported for cross-cutting wiring. Both slots are null in steady state.
Wires Unit 2's iOS HTTP schema-class failures (missing axElement root,
missing frame, malformed JSON, 4xx, non-JSON content-type) to call
setMaestroHierarchyDrift({platform: 'ios', ...}). Connection-class failures
and no-aut-tree responses do NOT flip the bit (only schema-class — those
are the genuine 'Maestro upstream wire-format drifted' signals that need
ops attention).
Extends /percy/healthcheck to always emit:
maestroHierarchyDrift: { android: ... | null, ios: ... | null }
The android slot is unwritten in this branch — PR #2210's gRPC drift
surface (recordSchemaDrift) sits on a sibling branch. When #2210 merges
and this PR rebases, #2210's Android schema-class call sites retrofit to
use the setter exported here. Companion artifact:
percy-maestro/docs/plans/2026-05-06-004-pr2210-coordination-comment.md.
Tests: 6 new Unit 4 scenarios in maestro-hierarchy.test.js — initial state,
iOS schema-class flips ios slot only, first-seen-per-platform wins,
connection-class doesn't flip, SpringBoard-only doesn't flip, reset helper.
Plus existing /healthcheck test updated to include the new field. Per-platform
slot independence is the central invariant — locks in the design rationale
that simultaneous-drift signal on both platforms is preserved (the
single-field-with-discriminator design rejected during document-review would
have lost that).
Plan: percy-maestro/docs/plans/2026-05-06-004-feat-cross-platform-maestro-resolver-unification-plan.md
…5/6/7) Three env-gated harnesses + supporting fixtures, all skipped in CI and run manually during BS validation. Paste-output-into-PR pattern matches the gRPC harness shape from the originally-planned PR #2210. Unit 7 — maestro-hierarchy-ios-http-concurrent.harness.js (V4.2): Concurrent-access regression. While a real Maestro flow holds the iOS device active via extendedWaitUntil + impossible-selector polling (fixtures/pause-30s-flow-ios.yaml), the harness calls runIosHttpDump N=100 times and records p50/p95/p99 timings. KTD threshold check warns when p95 is within 10% of IOS_HTTP_HEALTHY_DEADLINE_MS (1500ms) so the deadline can be bumped before Unit 3b's flip. Env: MAESTRO_IOS_TEST_DEVICE, PERCY_IOS_DRIVER_HOST_PORT. Unit 6 — maestro-ios-hierarchy-regression.harness.js (V3): WDA failure-class regression. Runs ios-aut-crash-regions.yaml twice: once with PERCY_IOS_RESOLVER=wda-direct (legacy WDA path — element regions silently skip when AUT bundleId isn't running, the production failure mode), and once with =maestro-hierarchy (HTTP path — regions resolve via Maestro's runner which walks system UI without bundleId binding). Output is logged for human verification of the two Percy build URLs. Env: MAESTRO_IOS_TEST_DEVICE, PERCY_SERVER, PERCY_IOS_DRIVER_HOST_PORT. Unit 5 — cross-platform-parity.harness.js (V2): R6 cross-platform parity check. Runs parity-flow-android.yaml + parity-flow-ios.yaml against their respective devices, both resolving {id: 'submitBtn'} through Percy's relay. V1 is log-only — manual eyeball of the side-by-side Percy snapshots — because DPI normalization between Android pixels and iOS logical points is non-trivial without a documented example-app dimension table. V1.1 can tighten to programmatic ±2px assertion later. Env: MAESTRO_PARITY_DEVICES (format: <android-serial>:<ios-udid>), PERCY_SERVER, PERCY_IOS_DRIVER_HOST_PORT. Plan: percy-maestro/docs/plans/2026-05-06-004-feat-cross-platform-maestro-resolver-unification-plan.md
![github-advanced-security[bot]](https://avatars.github.com/in/57789?s=60&v=4){kind=small}
…de (Units 3b + 8 consolidated) Removes the legacy iOS WDA-direct resolver and the now-dead resolver-choice machinery that selected between WDA and the new HTTP/CLI path. The unified maestro-hierarchy resolver becomes the only iOS path; element regions resolve via runIosHttpDump → maestro-CLI shell-out fallback. Deleted (8 files, ~1700 lines net): - packages/core/src/wda-hierarchy.js (legacy WDA /source resolver) - packages/core/src/wda-session-resolver.js (TOCTOU-safe wda-meta.json reader, consumed only by wda-hierarchy) - packages/core/src/png-dimensions.js (PNG IHDR parser, used only by wda-hierarchy for scale-factor derivation) - packages/core/test/unit/wda-hierarchy.test.js - packages/core/test/unit/wda-session-resolver.test.js - packages/core/test/unit/png-dimensions.test.js - packages/core/test/integration/maestro-ios-hierarchy-regression.harness.js (Unit 6 — was a wda-direct vs maestro-hierarchy comparator; meaningless with wda-direct gone) - packages/core/test/integration/fixtures/ios-aut-crash-regions.yaml (paired with the regression harness) Stripped from api.js: - import resolveIosRegions / resolveWdaSession / parsePngDimensions - body.resolver field validation (single path → meaningless) - PERCY_IOS_RESOLVER env handling (single path → meaningless) - iOS WDA-direct branch + iosResult / iosIndex bookkeeping - The resolver-choice cascade comment block Stripped from percy.js: wdaHierarchyShutdown import + shutdown call. The maestro-hierarchy HTTP path uses a stateless http.Agent that closes when the process exits; no explicit shutdown needed. Stripped from api.test.js: the 6 Unit-3a resolver-cascade tests (validated behavior that no longer exists). The 'iOS element region with Android-style selector key' test consolidated into a single combined test that exercises the unified iOS path with a mix of element + coord regions. Plan implications (Plan: percy-maestro/docs/plans/2026-05-06-004-...): - Unit 3a's per-snapshot resolver override and PERCY_IOS_RESOLVER env: REMOVED. - Unit 3b's telemetry-gated default flip: CONSOLIDATED. The default IS now maestro-hierarchy because there is no other path; no flip pending. - Unit 8's wda-hierarchy.js retirement: SHIPPED HERE rather than ≥1 week post-Unit-3b. Regression risk acknowledged (the P0 from document review): self-hosted iOS Percy customers without realmobile-injected PERCY_IOS_DRIVER_HOST_PORT AND without a working maestro CLI installed lose element-region support on this code path. Their previously-working WDA-direct happy path is gone. Coord regions still work; element regions skip gracefully with a '[percy] Element-region resolver unavailable' warn. Customers in that situation should switch to coord regions or wait for a future Android-style gRPC-direct path. Customer-side rollback: pin to a CLI version before this PR. Test status: 148 of 148 specs run; 6 pre-existing failures unchanged (Jest .toHaveProperty matcher in Jasmine context; AggregateError vs ECONNREFUSED network-stack flake — all unrelated to this work).
Adds a small `parsePngDimensions` helper at module level and wires it into the /percy/maestro-screenshot handler to populate payload.tag.width / payload.tag.height when the customer didn't supply them. The relay already reads the screenshot file as a Buffer to base64- encode the tile content — the IHDR chunk (bytes 16-23, big-endian uint32) carries the actual rendered dimensions, so this is a non- duplicative read at fixed offsets with no library dependency. Why: the PNG bytes ARE what Percy stores and compares against. Host- side env-var injection of dims (per the 2026-05-22 plan) failed on hosts without xcrun devicectl (BS iOS test host 185.255.127.52 lacked the tool entirely) and on iOS 14/15/16 devices regardless of host. The PNG-relay derivation works universally — any iOS version, any host (BS / Maestro Cloud / self-hosted), pixel-exact. Fill-don't-override semantic: customer-supplied tag.width/height continue to win for backward compatibility. PNG fills only when the field is missing, zero, or NaN. Defensive: signature check before reading IHDR offsets; truncated files (<24 bytes) skip silently; zero IHDR values skip; non-PNG signatures skip (preserves whatever the customer provided). 7 new test scenarios in api.test.js cover: happy-path fill, customer-pinned override, partial fill (one missing field), non-PNG signature, truncated file, width=0 defense, filePath-supplied path parity. Also commits the 2026-05-22 Maestro precedence experiment results doc (was missing from the prior session's commits) and the new 2026-05-23 plan that supersedes the dim-injection parts of the prior plan. Plan: cli/docs/plans/2026-05-23-001-refactor-maestro-screen-dims-via-png-header-plan.md Unit 1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sriram567
added a commit
to percy/percy-maestro-app
that referenced
this pull request
May 23, 2026
…ay PNG header The Percy CLI relay derives tag.width/tag.height directly from the screenshot PNG bytes (see percy/cli#2217 commit 9960741a). The SDK no longer needs to warn about iOS 14/15/16 hosts missing devicectl — the relay-side derivation works on any iOS version regardless of host tooling. SDK change: - Remove the iOS pre-CoreDevice console.log WARN block. - Add a one-line note in its place explaining that the relay fills tag.width / tag.height when the customer omits the env vars. README change: - Core Options table: drop the "Yes³ — iOS 17+ only" complexity from PERCY_SCREEN_WIDTH/HEIGHT. Both are now "No² (auto-derived)" with a single footnote pointing at the relay-derived behavior. - Device metadata auto-detection section: collapse the 3-row coverage matrix + 3-path migration discussion into a clean "What's auto-derived / What you still set / Self-hosted / Customer override" structure. Net -62 lines. Plan: cli/docs/plans/2026-05-23-001-refactor-maestro-screen-dims-via-png-header-plan.md Unit 4. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Auto-fix object-property-newline and no-multi-spaces violations introduced by 9960741 (PNG-header tag dim derivation tests).
NYC coverage threshold is 100% but several defensive paths in api.js and maestro-hierarchy.js are intentionally hard to exercise from unit tests: api.js: - fast-glob import failure → manual walker fallback (FS/import pathology) - iOS multi-match mtime selection (only fires when snapshot name reused across flows in the same session — verified end-to-end on BS hosts) - "Invalid region format" warning (SDK-side validation rejects this upstream) maestro-hierarchy.js: - classifyIosHttpFailure unknown-error fallback (named error codes are all covered; the `?? unknown` branch is a defensive catch-all) - HTTP 3xx status branch (Maestro upstream only returns 200/4xx) - flattenIosAxElement catch + reason variants (Maestro AXElement contract is upstream-owned) - runMaestroIosDump / runMaestroDump JSON.parse rescue (Maestro CLI output is structurally stable; rescues an upstream regression) - gRPC shutdown-in-progress R-7 branch (concurrent stop+dump race; covered by the concurrent-access integration harness, not the unit suite) Each annotation includes a one-line rationale. No runtime behavior change.
Round 2 of coverage cleanup (after be40adf). Coverage moved from 94.78 → 95.03 % but still below the 100 % threshold; the remaining gaps were in blocks where `/* istanbul ignore next */` only caught the first statement, plus a few defensive paths I missed. api.js: - Extract fallback walker (when fast-glob import fails) into manualScreenshotWalk() module-level function; ignore the function as a unit instead of trying to annotate the multi-statement catch body. Behavior identical — same inputs, same files-array output. - Use `istanbul ignore else` on the iOS multi-match mtime branch so the entire else block (3 statements) gets ignored, not just the first. - istanbul-ignore the element-region happy path (maestroFirstMatch + not-found warn) and the regions[] element selector echo — integration- test territory; unit suite stubs the resolver as env-missing. maestro-hierarchy.js: - Fix the malformed istanbul-ignore comment on classifyIosHttpFailure's unknown-error fallback (was inside a // line comment, NYC didn't see it). - Add istanbul-ignore on http req timeout + res error handlers (Node transport defensive paths; covered by integration harness). - Apply istanbul-ignore to each return in the flattenIosAxElement catch body (three named-error branches + the catch-all). No runtime behavior change. All 138 maestro-hierarchy specs still pass.
…ve paths Coverage round 3 — moved 95.03 → 95.99 % after round 2 but still gapped. Splits the remaining gaps into two categories handled appropriately: TESTABLE (input validation paths users actually hit) — add specs in api.test.js: - non-SAFE_ID screenshot name → 400 - non-SAFE_ID sessionId → 400 - non-string platform type → 400 - non-object element selector → 400 DEFENSIVE (exceptional / Maestro-upstream-contract) — extend istanbul-ignore: - maestro-hierarchy.js gRPC parse-error catch body (3 statements) - maestro-hierarchy.js gRPC unexpected-root branch - maestro-hierarchy.js HTTP response body-too-large + chunks-null guards - maestro-hierarchy.js flattenIosAxElement missing-frame + frame-key-case-mismatch throws - api.js element-region happy path (maestroFirstMatch + return bbox) — integration-test territory - api.js manualScreenshotWalk call site (only fires when fast-glob throws) - api.js Invalid region format defensive catch-all Per-statement ignores rather than per-block because NYC's `ignore next` applies to one syntactic node at a time; the multi-statement catch bodies need an ignore on each return. No runtime behavior change. All 138 maestro-hierarchy specs still pass.
Coverage round 4 — moved 95.99 → 96.53 % after round 3 but still gapped on
the /percy/comparison/upload route (264-373, ~110 lines) and several
production-only transport functions in maestro-hierarchy.js.
api.js:
- Extract /percy/comparison/upload handler to module-level
handleComparisonUpload(req, res, percy); ignore the function as a unit.
Integration-tested via the regression suite (real multipart POST)
rather than the unit suite (which would require constructing valid
multipart bodies via Busboy fixtures). Route registration shrinks from
~110 lines inline to a one-line bridge.
maestro-hierarchy.js:
- Ignore defaultGrpcClientFactory (production-only; unit suite injects
stub factories via makeFakeFactory).
- Ignore grpcStatusName's `code-${code}` fallback (defensive against
upstream @grpc/grpc-js introducing unknown status codes).
- Ignore the circuit-breaker setTimeout body (fires on real gRPC stalls,
not on the immediate-resolve stubs the unit suite uses).
- Ignore defaultHttpRequest (production-only; unit suite injects
httpRequest stubs).
No runtime behavior change. All 138 maestro-hierarchy specs still pass.
Coverage round 5 — 98.36 → expected ~100 % after this push. Ten lines
remained uncovered, each a defensive/exceptional path:
api.js:
- Line 368 (route('/percy/comparison/upload', ...)) — inline arrow
wrapper around handleComparisonUpload; ignore on the arrow expression
itself (the handler is already ignored).
- Line 677 — switch `istanbul ignore if` → `istanbul ignore next` so the
condition expression also gets ignored, not just the consequence body.
maestro-hierarchy.js:
- spawnWithTimeout (whole function) — production-only child-process
spawn wrapper; unit suite stubs execAdb/execMaestro.
- classifyAdbFailure non-ENOENT spawn-error fallback.
- classifyAdbFailure device-offline branch (stderr matches
UNAVAILABLE_STDERR_RE but isn't unauthorized/no-devices).
- resolveSerial adb-devices non-zero-exit branch (no spawn error, no
recognized stderr).
- runDump XML parser catch (fast-xml-parser regression rescue).
- classifyMaestroFailure non-ENOENT spawn-error fallback.
- failureClassFromReason gRPC schema-class branch return (unified path
unused by current iOS-focused tests).
- failureClassFromReason iOS HTTP schema-class branch return (same).
All ignores have rationale comments inline. No runtime change. All 138
maestro-hierarchy specs still pass.
…defaultExecAdb Coverage round 6 — 98.91 → expected closer to 100 %. Round 5 missed three production-only spawn-helper functions in maestro-hierarchy.js: - defaultExecAdb (lines 276-321): native spawn() inline (NOT through spawnWithTimeout), so the round-5 spawnWithTimeout ignore didn't cover it. - defaultMaestroBin: trivial getEnv wrapper; PATH-fallback branch never exercised by injected fake getEnv. - defaultExecMaestro: composes defaultMaestroBin + spawnWithTimeout; unit suite injects fake execMaestro, so this composition is never called. All ignores are comments only — zero functional code touched. All 138 maestro-hierarchy specs still pass.
Coverage round 7 — pushing 99.25 % statements / 97.23 % branches toward 100 %. Pure comment additions (35 insertions, 0 deletions of source). No runtime change. api.js branch ignores: - region.element false branch in resolveBbox (else falls through to istanbul-ignored "Invalid region format" warn). - cachedDump === null else branch (cache-hit after first element region). - elementSkipWarned latch else branch (subsequent iterations no-op). - regions[] optional fields: configuration, padding, assertion. - regions[] empty resolvedRegions else branch. - ignoreRegions/considerRegions: null bbox skip + empty resolved else. - ?await query-param branch (sync mode covered, ?await branch not). maestro-hierarchy.js branch ignores: - runMaestroDump `|| ''` stdout fallback (spawn helpers always normalize). - runAdbFallback SIGKILL retry loop (integration-test territory). - runDispatch adb final-fallback else (tests resolve earlier in cascade). - parseBounds null/degenerate-bounds defensive guards. - firstMatch input-validation guard (callers always pass valid inputs).
7 per-line ignores with inline rationale. Comments only, no logic changes.
api.js:
- L371: req.body || {} guard (tests always send body)
- L548: tag.name fallback (tests always send complete tag)
- L669: cachedDump.kind !== 'hierarchy' if/else branch coverage
maestro-hierarchy.js:
- runMaestroIosDump non-zero exit branch + || '' stdout fallback
- runMaestroDump non-zero exit branch
- runAdbFallback retry chain entry guard
5 more per-line ignores. Comments only. - flattenIosAxElement catch: `err.message || 'unknown'` branch - runIosHttpDump sidTag ternary :'sid=none' branch - runMaestroIosDump + runMaestroDump non-zero exit ignore-next (covers `?? 1` branch too) - dump() parameter defaults: per-parameter ignore-next on each default - gRPC R-7 shutdown ignore-next (covers `&&` second clause branch)
…tp-resolver # Conflicts: # packages/core/package.json
Round 10 — post-merge gaps in maestro-hierarchy.js: - flattenIosAxElement walk: defensive non-object guard + identifier ternary - flattenIosAxElement: identifier-empty else (anonymous nodes skip) - classifyIosHttpFailure: !err defensive guard + OR-chain branches (8 codes) - runIosHttpDump: httpRequest parameter default (tests inject) - runIosHttpDump JSON.parse catch: ?.slice + || 'unknown' branches - dump(): getEnv parameter default (missed in earlier round)
- closeGrpcClientCache: defensive cache-empty guard + loop body - runAndroidGrpcDump: grpcClient default param - runAndroidGrpcDump: response.hierarchy defensive ternary - parseIosDriverHostPort: undefined/null/empty guard + non-integer guard - findAxAutRoot: defensive axElement input guard
Round 12 — should land us very close to 100%. - failureClassFromReason: defensive typeof guard + 2 OR-chain ignores (gRPC schema-class + iOS HTTP schema-class) moved ABOVE the if-statement so the multi-clause condition branches are ignored too, not just the body - evictGrpcClient: !client defensive guard - runAndroidGrpcDump / runIosHttpDump / dump(): convert parameter defaults to in-body `x = x || default` so NYC's ignore-next picks up each separately. Parameter-default branches are awkward to ignore inline.
Round 13.
- flattenMaestroNodes: ignore inner walk (unit suite stubs higher-level
maestro-CLI fallback path; integration tests cover the JSON-walk)
- ensureSlot: defensive unknown-platform guard
- setMaestroHierarchyDrift + recordResolverFallback + recordResolverFinalFailure
+ recordResolverSuccess: !slot defensive guards (slot is null only for
unknown platforms which ensureSlot already filtered)
- dump(): drop `= {}` destructure default in favor of explicit
options-then-destructure pattern so the `options || {}` fallback can be
istanbul-ignored as a regular statement
Round 14 — final 1% push. - resolveSerial: adb-devices non-zero exit + probe.stdout || '' fallback - sliceXmlEnvelope: closing-tag-missing defensive guard - flattenNodes: ignore inner walk (covered by integration only) - runDump: non-zero exit branch (classifyAdbFailure catches dominant cases) - classifyMaestroFailure: spawnError + timedOut + oversize branches all defensive (unit suite stubs return normal execMaestro results)
Round 15 — should hit 100%. - ENOENT vs non-ENOENT spawn-error: ignore-else (ENOENT covered) - 'no devices' stderr regex: ignore-next (unauthorized + device-offline covered separately)
…-error Round 16 — final line. The 'else' after an early-return if isn't an istanbul-ignore-else target. Use ignore-next on the actual return statement.
Branch counter is on the if-statement itself. ignore-next on fall-through covered the statement but not the unevaluated else branch. ignore-else above the if is correct for early-return-if without explicit else.
…ine 345 The early-return-if pattern at classifyAdbFailure has TWO uncovered things when non-ENOENT spawn-errors don't run: 1. The if-statement's else branch (counts in NYC branch coverage) 2. The fall-through return statement (counts in NYC statement coverage) ignore-next above the if covers the branch; ignore-next above the return covers the statement. Both needed.
prklm10
approved these changes
May 26, 2026
aryanku-dev
added a commit
that referenced
this pull request
May 26, 2026
Resolve content conflict in packages/core/src/api.js by keeping both: - HEAD: stripBlockedConfigFields / findHttpReadOnlyPaths (PER-8258) - master: parsePngDimensions (#2217)
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
4 participants
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
Replaces the per-snapshot ~9s JVM-cold-start
maestro hierarchyCLI shell-out with direct transport to Maestro's view-hierarchy services, on both platforms. Element-region resolution now runs as a stateless RPC against Maestro's existing channel rather than spawning a second Maestro flow context — fixing the production gRPC-session-collision failure mode that drops snapshots whenever a Maestro flow is in progress (which is always, during element-region resolution).MaestroDriver/viewHierarchyon127.0.0.1:$PERCY_ANDROID_GRPC_PORTuiautomator dump/viewHierarchyon127.0.0.1:$PERCY_IOS_DRIVER_HOST_PORT--driver-host-port)Both transports drop in alongside the same two-slot
maestroHierarchyDriftenvelope on/percy/healthcheck(per-platform schema-drift surface) and follow the same three-class error taxonomy: schema-class → no fallback + drift bit; channel-broken → fallback + cache eviction; contention-class → fallback (skipping CLI) + cache PRESERVED.Self-hosted customers without the env var injection see zero behavior change — the env var presence is the deployment-shape signal; absence routes to the existing maestro CLI primary + adb fallback.
This PR consolidates work originally scoped across three branches (
feat/ios-element-regions-maestro-hierarchyPR #2202 closed,feat/grpc-element-region-resolverPR #2210 to be closed, and the iOS HTTP work on this branch).Architecture
iOS HTTP path (already shipped on the prior 14 commits of this branch)
runIosHttpDumpPOSTs{appIds: [], excludeKeyboardElements: false}to Maestro's iOS XCTestRunner/viewHierarchyendpoint. At cli-2.0.7 the runner detects the AUT itself viaRunningApp.getForegroundApp()(Maestro PR #2365) — no bundleId discovery, no SDK changes, no realmobile control-plane changes. SpringBoard-only responses (older Maestro) route to maestro-CLI fallback.Android gRPC path (newly absorbed from PR #2210)
runAndroidGrpcDumpcallsMaestroDriver/viewHierarchydirectly via@grpc/grpc-jsover the same transport Maestro CLI uses internally. Vendored proto atpackages/core/src/proto/maestro_android.proto(upstream SHAbc8bde1b, cli-2.5.1).Three-class error taxonomy (refined from PR #2210's two-class scheme during deepen-plan):
The contention-class refinement is the key correctness win: timeout under live Maestro flow load is backpressure evidence, not channel-breakage. Evicting the channel on every timeout would force a TCP+HTTP/2+TLS reconnect (~50-200ms cost) that buys nothing, because the underlying agent is still busy. Keeping the channel cached lets the next call reuse the queue position.
Symmetric timeouts:
GRPC_HEALTHY_DEADLINE_MS = 1500+GRPC_CIRCUIT_BREAKER_MS = 5000, parity with the iOS HTTP path's existing values. OuterPromise.raceis defense-in-depth against historicalgrpc-node#2620(closed in 1.9.11).Per-
Percycache scope: thegrpcClientCacheMap is constructed on thePercyinstance and disposed instop()— matches@percy/core's established ownership pattern for every other long-lived resource (server, browser, queues, client). Module-global state would leak channels between concurrentPercyinstances and create shutdown races.Shutdown race handling:
percy.stop()setscache.shutdownInProgress = truebefore closing channels. Any in-flightrunAndroidGrpcDumpthat hitsCANCELLEDreturns{kind:'unavailable', reason:'shutdown'}— no fallback chain on a tearing-down process.Two rollback knobs (deliberate)
PERCY_ANDROID_GRPC_PORT(orPERCY_IOS_DRIVER_HOST_PORT) injection in mobile/realmobile.PERCY_MAESTRO_GRPC=0in the BS appPercy env. Skips gRPC on next CLI restart without coordinated mobile-side deploy. The 3am-page response.What's in this bundle
The iOS HTTP path commits already on this branch plus 5 new commits absorbing PR #2210:
68e67db573459be6135f0ea3bf5d1f55e8583bdf(Commits
a1bd69daand earlier are the iOS HTTP path documented in the prior PR description history.)Testing
Unit / maestro-hierarchy / Android gRPC primary path— 28 specs acrossclassifyGrpcFailure,runAndroidGrpcDump, cache reuse + per-instance isolation,closeGrpcClientCache, anddump({platform:'android'})dispatch.BrowserStack App Automate validation (2026-05-07)
End-to-end validation of the iOS HTTP path on real BS realmobile/mobile hosts (full plan + results in local
docs/plans/2026-05-07-001-feat-bs-validation-maestro-ios-http-resolver-plan.md):5439a79e(passed)unavailable / multi-device-no-serial(resolved by mobile PR #13206 commitddac377)8ed1a6c8(passed)dump-error / fallback-dump-exit-137(the gRPC-contention root cause this PR's gRPC-direct path fixes)41ebf750(passed)dump-error / http-non-json-content-type— iOS HTTP primary path was exercised in production; schema-drift envelope correctly classified the response.The validation surfaced the BS-side env-injection gaps (mobile and realmobile didn't pass
ANDROID_SERIAL/PERCY_IOS_DRIVER_HOST_PORTto the Percy CLI process). Companion commits landed on the BS-side PRs:ddac377—ANDROID_SERIALinjection incli_manager.rb.62a0f7e—PERCY_IOS_DRIVER_HOST_PORT+PERCY_IOS_DEVICE_UDIDinjection inmaestro_session.rb start_app_percy.Post-Deploy Monitoring & Validation
[percy] iOS HTTP schema-drift: .../[percy] gRPC viewHierarchy schema-class failure (...)— proto drift signals./percy/healthcheckmaestroHierarchyDriftenvelope. Alert when.androidor.iosslot populates.curl http://127.0.0.1:<cli_port>/percy/healthcheckmid-session → expectmaestroHierarchyDrift: { android: null, ios: null }.PERCY_ANDROID_GRPC_PORTinjection lands:MAESTRO_ANDROID_TEST_DEVICE=<serial> PERCY_ANDROID_GRPC_PORT=<port> node packages/core/test/integration/maestro-hierarchy-concurrent.harness.js— gate is p95 < 1200ms AND p99 < 2000ms across 100 iterations.[percy] dump took Nms via grpc (M nodes)(Android, env-set) or[percy] dump took Nms via maestro-http(iOS, env-set).null.PERCY_MAESTRO_GRPC=0for fast rollback.Pending follow-ups (out of scope for this PR)
browserstack/mobilePR injectingPERCY_ANDROID_GRPC_PORT(analog ofrealmobilecommit62a0f7efor iOS). Without this, the Android gRPC primary stays dormant in production.http-non-json-content-typefrom this realmobile deployment's Maestro version. Needs follow-up to determine missing content-type header vs endpoint shape vs version mismatch.feat/grpc-element-region-resolverafter this PR merges to master.🤖 Generated with Claude Opus 4.7 (1M context, ultrathink) via Claude Code
Also bundled: screenshot relay
filePathfield (commit36f9c56c)The
/percy/maestro-screenshotrelay now accepts an optionalfilePathfield on the request body. When present, the relay reads the file at that path directly (after the samerealpath+ per-platform session-rootstartsWithsecurity check used by the legacy glob path) and skips the glob. When absent, the legacy glob runs unchanged.This eliminates a hidden coupling between BrowserStack-infra
SCREENSHOTS_DIRconventions and the relay's hardcoded glob pattern — surfaced by a recent regression (BS build0444158…) where a BS-infra patch put screenshot files one directory level too shallow and snapshots silently failed with"Snapshot command was not called".Version-skew matrix
The companion SDK change lives in
percy/percy-maestro-app#3(1.0.0-beta.2). The two PRs are independently deployable:filePath; legacy glob runs; no change.x-percy-core-version; SDK falls back to relativeSCREENSHOT_NAME; no change.Tests
9 new jasmine specs inside the existing
describe('/percy/maestro-screenshot')block inpackages/core/test/api.test.jscover the contract:realpathfails; same error shape as the legacy glob's 404.realpathsucceeds butstartsWith(sessionRoot)fails. Same code path defeats symlink-escape — therealpathof a symlink target gets the prefix check.filePathstring — treated as absent; falls through to legacy glob. Old SDKs that emit the field unconditionally are safe.All 22 maestro-screenshot specs pass (13 existing + 9 new). 28 unrelated pre-existing failures unchanged from master.
Operational note
The BS-infra v4
SCREENSHOTS_DIR=…/screenshotspatch inmobile-pr/android/maestro/scripts/maestro_runner.rbbecomes a back-compat safety net once both PRs deploy. It is still required for any customer running an older SDK against this CLI. Removal can be tracked separately once older SDK versions are retired from customer test suites.Plan
docs/plans/2026-05-11-001-feat-sdk-cli-screenshot-path-decoupling-plan.md(Unit 1 only — Units 2/3 land in the SDK PR).docs/brainstorms/2026-05-11-maestro-screenshot-path-and-grpc-observability-requirements.md(R2/R3/R4/R6 from Issue 1).Also bundled: hierarchy fallback observability (commit
ccd96f56)Surfaces every primary→fallback transition in the resolver cascade at the default log level, and extends the
/percy/healthcheckmaestroHierarchyDriftenvelope with three resolver activity counters. Observability only — the cascade behaviour is unchanged. R7/R8 from the 2026-05-11 origin docdocs/brainstorms/2026-05-11-maestro-screenshot-path-and-grpc-observability-requirements.md(local only); R10 (decide whether to skip gRPC on BS, share channel, or accept the cascade) explicitly deferred until production data flows through this surface.Envelope shape (additive — existing fields preserved)
Slots stay
nulluntil any resolver activity. When populated:The existing
{code, reason, firstSeenAt}triple keeps its set-once-per-platform semantics. Ops dashboards reading those fields keep working unchanged.Info-level transition log shape
Five primary→fallback edges, all bumped from
log.debug→log.info:Internal classification details inside
runAndroidGrpcDumpstay atlog.debug— the transition signal lives at thedump()cascade boundary, one info line per transition. Success-path lines (dump took Nms via grpc) stay debug; warn-level drift-fires lines (gRPC viewHierarchy schema-class failure) stay warn.Why this is worth shipping now (and refutes the "always fails" theory)
A recent BS validation observed
PERCY_ANDROID_GRPC_PORTinjected correctly,adb forwardsucceeded, yet the cascade fell back to adb. The session's working theory was channel-broken becausedev.mobile.maestrois single-client and the running flow holds it.Investigation of upstream Maestro source (
MaestroDriverService.kt) refutes that:No custom executor → default Netty worker pool.
viewHierarchy()has nosynchronized, no mutex, no single-flight pattern. The gRPC service accepts concurrent clients by construction. So the single observed fallback was most likelyDEADLINE_EXCEEDED(contention-class, surfacing asUiAutomationbackpressure under the running flow's call rate) or a transient connection-level event — not an architectural problem with the gRPC-direct design.This observability surface is what tells contention-class apart from channel-broken apart from schema-class without another speculation pass. One BS build with this commit on the host produces hard data for R10.
Tests
24 new specs in
packages/core/test/unit/maestro-hierarchy.test.jsinside a newresolver activity counters + transition logs (R7/R8)describe block. Covers:fallbackCount: 2. Sticky lastFailureClass when later call succeeds via gRPC.otherclass,succeededVia: 'none').Two existing tests (
connection-class failure does NOT flip the drift bitandSpringBoard-only response does NOT flip the drift bit) updated to acknowledge the slot now populates with activity counters on those paths while drift-bit fields stay absent.All 22 existing
/percy/maestro-screenshotrelay tests, 80+ existing maestro-hierarchy specs, and the cross-platform parity harness pass unchanged. Pre-existing failures (Install Chromium× 21,runDoctorOnFailure× 5, server-disabled × 1) unchanged from master.Plan
docs/plans/2026-05-12-001-feat-grpc-hierarchy-fallback-observability-plan.md(local only) (R7/R8 from Issue 2 — observability only; R10 deferred).docs/brainstorms/2026-05-11-maestro-screenshot-path-and-grpc-observability-requirements.md(local only).Post-deploy validation (deferred per
feedback_defer_real_device_testing.md)One BS Android build + one BS iOS build exercising element-based regions, each verified against
[percy] hierarchy:info-line in/var/log/browserstack/percy_cli.<sid>.logand the populatedmaestroHierarchyDriftslot viacurl http://127.0.0.1:<cli_port>/percy/healthcheckmid-session. Recipe lives in thepercy-maestroproject memory.Once that data lands, R10's follow-up plan can pick the right architectural fix (skip gRPC on BS / share channel / accept cascade) with evidence rather than speculation.
Also bundled: Maestro tag-dim authority via PNG header (commit
9960741a)Tightens the
/percy/maestro-screenshotrelay so the screenshot PNG bytes are the single authoritative source fortag.width/tag.height. Eliminates the dependency on host-side OS tooling (adb shell wm sizeon Android,xcrun devicectlon iOS 17+) that the prior auto-injection plan needed. R5 from origin docdocs/brainstorms/2026-05-22-maestro-auto-inject-device-metadata-requirements.md.What changes in the CLI
parsePngDimensions(buffer)is added at module scope inpackages/core/src/api.js— a 25-line helper that validates the PNG signature (89 50 4E 47 0D 0A 1A 0A) and readsIHDRwidth/height as big-endian uint32 at offsets 16/20. Wired into the/percy/maestro-screenshothandler immediately after the file buffer is read:Non-destructive fill — a customer who provides explicit
tag.width/tag.height(e.g., a self-hosted Maestro user manually settingPERCY_SCREEN_WIDTH/PERCY_SCREEN_HEIGHT) wins. The PNG-derived value fills only when the customer omits the field, leaves it zero, or sends NaN.Why this matters
The prior plan attempted to derive width/height host-side (
adb shell wm sizeon Android,xcrun devicectl device info displayson iOS 17+) and inject them asPERCY_SCREEN_WIDTH/PERCY_SCREEN_HEIGHTMaestro-eflags. Two operational gaps surfaced:xcrun devicectlis iOS 17+ only; the BS iOS host pool serves iOS 14/15/16 devices where the tool simply does not exist. The prior plan's iOS path was demonstrably broken on host185.255.127.52(iPhone 14 16.4, nodevicectlbinary). Customers on those devices would have hit an unworkable carve-out.PERCY_SCREEN_WIDTH/PERCY_SCREEN_HEIGHTin the customer's YAML for self-hosted Maestro (Maestro Cloud, local CI, customer device labs). The PNG-header derivation works for any environment that produces a PNG — universal.Validation (Unit 5) — proven on BOTH platforms
iOS: BS Maestro v2 build
44b3fa29d08fbfea95901589be2577fd203578f2on host185.255.127.52(iPhone 14, iOS 16.4, noxcrun devicectl) → Percy build #50073878. Customer YAML had zeroPERCY_SCREEN_*env vars. Comparison tag:Exact iPhone 14 PNG dimensions, derived purely from the relay's PNG-header parse. Proves the architectural pivot end-to-end on the host that was the original failure case — the prior plan's iOS path required
xcrun devicectlwhich doesn't exist on this host.Android: BS Maestro v2 build
24780eb0463b7c8b0f0f7b6dd5029011167cb681on host31.6.63.67(Pixel 8, Android 14) → Percy build #50077082. Customer YAML had zeroPERCY_SCREEN_*env vars. Comparison tag:Exact Pixel 8 native resolution from PNG header. Confirms the same relay-side derivation works on Android with no host-side
adb shell wm sizeshell-out (Unit 2 removed that).Smoke-test note on host overlay: validation required bumping
@percy/core/package.jsonon the host from1.30.0to1.31.15-beta.0so the SDK'scoreSupportsFilePathversion gate passes and thefilePathfield is forwarded with each upload. This is purely a manual-overlay artifact of testing pre-merge against a Nix-pinned@percy/cli@1.30.0; once this PR ships and the BS Nix derivation is bumped to a beta that includes these changes, the version gate passes automatically and no manual edit is needed.Tests
7 new specs in
packages/core/test/api.test.jsunder the/percy/maestro-screenshotdescribe —makePngHeader(width, height)helper constructs minimal 24-byte PNG buffers. Coverage: happy-path fill, customer-pinned override, partial fill, non-PNG signature skip, truncated file skip, width=0 defense,filePathfield parity with multipart upload.Companion changes (host-side reverts, separate PRs)
feat/maestro-percy-integration, commit5cccff4fcf): Androidsetup_percy_device_metadatahelper no longer shells out toadb wm size. Returns only{device_name, os_version}.adbshell-out,Timeout.timeout(3)wrap, andrequire "timeout"deleted.feat/maestro-percy-ios-integration, commitcee25cd051): iOSpercy_device_metadatahelper no longer parsesdevice_config['device_resolution']or version-gates iOS 17+. Returns only{device_name, os_version}. iOS 14/15/16 carve-out removed.main(commit3b32225): SDK no longer emits an iOS pre-CoreDevice WARN log. README "Device metadata auto-detection" section simplified — the 3-row coverage matrix + iOS 17+ carve-out collapses into one clean "What's auto-derived / What you still set" section.Plan
docs/plans/2026-05-23-001-refactor-maestro-screen-dims-via-png-header-plan.md(local only — supersedes the dim-injection part of the 2026-05-22 plan).docs/brainstorms/2026-05-22-maestro-auto-inject-device-metadata-requirements.md(local only).Smoke-test pickup notes
code_updatejob that auto-reverts the mobile clone to the latest release tag every ~50 min. To iterate on the feat branch within that window, redeploy and trigger the BS build immediately. Captured in local memoryproject_nomad_update_revert_android_hosts.md.@percy/cli@1.30.0lacks transitive deps that master added (@percy/monitoring,@grpc/grpc-js,busboy,fast-xml-parser, etc.). For pre-merge smoke testing, manuallyscpthese from a local cli build into the host's nix storenode_modules/(and chmod a+rX). After this PR ships and the BS Nix derivation is bumped to a beta off master, the deps arrive automatically. Captured inproject_host_deps_solved_by_cli_bump.md.