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

[Sriram567]

Summary

Behavior-preserving decomposition of the code merged in #2261 ("self-hosted (non-BrowserStack) Maestro + Percy support — V1"), which layered self-hosted logic onto two already-large files:

• packages/core/src/api.js — the /percy/maestro-screenshot handler had grown to ~517 lines (BS file-find + feat(cli): self-hosted (non-BrowserStack) Maestro + Percy support — V1 #2261's runtime detection, PERCY_MAESTRO_SCREENSHOT_DIR scope-root, recursive dot:true glob, branched realpath, all inline).
• packages/cli-app/src/exec.js — 34 → 211 lines: five Maestro argv/env-injection functions buried in the command file.

This restores the module boundary (route table delegating to domain modules — the handleSyncJob pattern) with no wire/CLI behavior change.

Supersedes #2286. That PR decomposed the pre-#2261 handler and now conflicts with master. This is the fresh, master-based decomposition that captures the original relay and #2261's self-hosted additions in one pass (reusing #2286's proven module design).

What changes

@percy/core — api.js 1072 → 382 lines (a route table):

• comparison-upload.js — handleComparisonUpload (multipart /percy/comparison/upload); encodeURLSearchParams promoted to utils.js.
• maestro-screenshot.js — handleMaestroScreenshot orchestrator + parsePngDimensions; owns runtime detection (selfHosted = !sessionId), conditional sessionId validation, and PERCY_MAESTRO_SCREENSHOT_DIR scope-root resolution + self-hosted filePath rejection.
• maestro-screenshot-file.js — locateScreenshot({…, scopeRoot, selfHosted}): BS session glob and self-hosted recursive dot:true glob (Windows-normalized) + manualScreenshotWalk (BS only) + realpath/scopeRoot containment check (forward-slash normalized).
• maestro-regions.js — validateRegionInputs + resolveRegions (request-scoped dump cache, explicit-key payload merge).

@percy/cli-app — exec.js 211 → 47 lines (a thin command):

• maestro-inject.js — maybeInjectMaestroServer + maybeInjectScreenshotDir (and the private argv helpers). app:exec's callback calls them; index.js re-exports them.

Not touched: maestro-hierarchy.js's #2261 iOS dispatch (small/coherent — the elaborate lsof cascade in #2261's description was never actually implemented). .semgrepignore path-traversal suppression retargeted to maestro-screenshot-file.js (+ api.js for createStaticServer's sitemap join).

Testing

• @percy/core api.test.js (the HTTP-level contract covering both BrowserStack and self-hosted paths, incl. feat(cli): self-hosted (non-BrowserStack) Maestro + Percy support — V1 #2261's self-hosted specs) passes unchanged — the only failure is the pre-existing IPv4/IPv6 server-disabled ECONNREFUSED→AggregateError flake. percy.test.js + maestro-hierarchy.test.js green.
• @percy/cli-app exec.test.js — 43/43 unchanged (imports the injectors via the @percy/cli-app barrel).
• Decomposition is coverage-neutral (api.test.js drives the same route into the relocated modules; /* istanbul ignore */ annotations travel verbatim). The 100% NYC gate, lint, and semgrep are validated authoritatively in CI.

Post-Deploy Monitoring & Validation

• No additional operational monitoring required: behavior-preserving internal decomposition — no HTTP/CLI contract, resolver-cascade, or runtime change; BS and self-hosted paths behave identically.
• CI gates to watch on this PR: full Jasmine suites, nyc --check-coverage (100% across @percy/core + @percy/cli-app), lint, and the semgrep path-join-resolve-traversal job (confirms the .semgrepignore retarget to maestro-screenshot-file.js).

Also included: exact device system-bar insets (commits 77cfec38, ba51f3d5, efd6501a)

Ported the device-inset feature onto the new decomposed modules (originally built on the now-superseded #2286 branch). The Maestro tile's statusBarHeight/navBarHeight are now derived per-device, authoritative over the SDK's static defaults, cached per session, with graceful fallback:

• iOS status bar ← /viewHierarchy statusBars element frame × empirical PNG→points scale (iPhone 14 → 141px). iOS bottom = 0 (static home indicator; fleet-consistent with percy-xcui-swift/percy-appium-python).
• Android ← adb dumpsys window mStableInsets (status + nav).
• deriveDeviceInsets is additive in maestro-hierarchy.js (the resolver cascade is untouched); per-session percy.maestroInsetCache; any derivation failure → SDK default (never blocks a snapshot).
• ~30 specs (deriveDeviceInsets unit specs + relay override/fallback/iOS-0/derive-and-cache).
• Plan: docs/plans/2026-06-24-001-feat-port-status-bar-insets-plan.md; requirements: docs/.../2026-06-19-maestro-exact-system-bar-insets-requirements.md.

Known limitation (now closed below): self-hosted iOS status-bar derivation needs PERCY_IOS_DRIVER_HOST_PORT — addressed by the --driver-host-port auto-injection section below.

Also included: version-aware --driver-host-port auto-injection (commit 5073c2f9)

Prescribes the iOS Maestro driver port on Maestro ≥ 2.6 so the relay reaches /viewHierarchy deterministically — closing the self-hosted iOS element-region warn-skip and the inset fallback (the limitation noted above). Bytecode-verified against the 2.4.0/2.6.1 JARs.

• Maestro 2.6.0 switched the iOS driver from the deterministic 7001 to an OS-assigned ephemeral port (ServerSocket(0)), breaking the relay's 7001 probe. maybeInjectDriverHostPort picks a free port, pins Maestro via --driver-host-port <port>, and mirrors process.env.PERCY_IOS_DRIVER_HOST_PORT (same Node process as the relay).
• Version-gated: the flag is a fatal Unknown option on ≤ 2.4.0 (why 13616a87 was reverted), so it fires only on ≥ 2.6.0; older versions no-op and the existing 7001 probe serves them.
• Conservatively gated to iOS (--platform=ios/-p ios), skips sharded runs (a single pinned port collides), respects customer overrides (argv flag / valid env), and no-ops on any maestro --version detection failure. BrowserStack unaffected (host-injected port; never routes through app:exec).
• ~35 exec.test.js specs (version threshold, platform gate, override precedence, shard-skip, detection failures, concurrency). Validated E2E on an iPhone 16 Pro simulator + Maestro 2.6.1.
• Plan: docs/plans/2026-06-24-002-feat-version-aware-driver-host-port-inject-plan.md.

Follow-ups

• Close refactor(core): decompose Maestro screenshot relay + derive exact system-bar insets #2286 once this merges (its decomposition is superseded here and its device-inset work is now ported in).
• Self-hosted iOS driver-host-port prescription — done in 5073c2f9 (see section above).

🤖 Generated with Claude Code

[Sriram567]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[Sriram567]

[Medium] Zero-height status bar collapses to null

node.frame?.Height || null treats a legitimate numeric 0 as absent, falling back to SDK defaults. Safe on today's devices (no Apple device reports a 0pt status bar), but the intent is implicit and a future immersive-mode layout reporting 0 would silently fall through.

Suggestion: make the guard explicit:

Suggested change

	return node.frame?.Height || null;
	return (typeof node.frame?.Height === 'number' && node.frame.Height > 0) ? node.frame.Height : null;

Reviewer: stack:code-reviewer

[Sriram567]

[Medium] Inset cache keys on undefined in self-hosted mode

Self-hosted runs have sessionId === undefined, so the cache keys on undefined. Correct for a single self-hosted run, but a later self-hosted snapshot with a different platform/pngDims on the same Percy instance would reuse the first derived value. Unlikely to bite (one self-hosted run per instance) but fragile.

Suggestion: use a sentinel key, e.g. const cacheKey = sessionId ?? '__self_hosted__'; and get/set on cacheKey.

Reviewer: stack:code-reviewer

[Sriram567]

Claude Code PR Review

PR: #2315 • Head: efd6501 • Reviewers: stack:code-reviewer

Summary

Decomposes the merged self-hosted Maestro relay (PR #2261) — extracting the /percy/maestro-screenshot and /percy/comparison/upload handlers out of api.js into domain modules, plus the cli-app app:exec Maestro argv/env injection into maestro-inject.js — and adds device system-bar inset derivation (deriveDeviceInsets: iOS status bar from /viewHierarchy, Android from dumpsys window mStableInsets) wired into the relay with a per-session percy.maestroInsetCache, authoritative over SDK static defaults with graceful fallback.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	Local relay (localhost:5338); no auth surface changed.
High	Security	Input validation and sanitization	Pass	SAFE_ID regex on name/sessionId, platform whitelist, filePath shape checks preserved verbatim in extracted modules.
High	Security	No IDOR — resource ownership validated	Pass	realpath + scopeRoot containment for screenshot reads preserved; self-hosted rejects filePath.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL in this codebase path.
High	Correctness	Logic is correct, handles edge cases	Pass	Faithful extraction; inset derivation has full failure taxonomy + graceful null fallback.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Derivation failures intentionally fall back to SDK default (documented); region resolver warn-skips.
High	Correctness	No race conditions or concurrency issues	Pass	Per-session cache; see Medium finding on undefined key for the single self-hosted nuance.
Medium	Testing	New code has corresponding tests	Pass	12 iOS + 7 Android deriveDeviceInsets cases; relay wiring + cache-clear specs.
Medium	Testing	Error paths and edge cases tested	Pass	Non-JSON, null body, missing port, throwing getEnv, spawn-error, non-zero-exit all covered.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI green incl. NYC 100% coverage gate (@percy/core job).
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Inset derivation runs once per session (cached). See Low finding on dumpsys window output size.
Medium	Performance	Long-running tasks use background jobs	N/A	Relay request path; iOS HTTP bounded by 1500ms deadline.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors handleSyncJob-in-snapshot.js decomposition precedent; injectable transports for tests.
Medium	Quality	Changes are focused (single concern)	Pass	Refactor + feature cleanly separated across commits.
Low	Quality	Meaningful names, no dead code	Pass	Clear names; no dead code.
Low	Quality	Comments explain why, not what	Pass	Rationale-rich comments; surgical istanbul ignore with justification.
Low	Quality	No unnecessary dependencies added	Pass	No new deps; pure stdlib PNG parse + existing adb/http helpers.

Findings

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

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: findStatusBarFrameHeight returns node.frame?.Height || null, which collapses a legitimate numeric 0 to null. Safe in practice (no Apple device has a 0pt status bar), but the intent is implicit and a future immersive-mode layout reporting 0 would silently fall back to SDK defaults.

• Suggestion: Make the guard explicit: return (typeof node.frame?.Height === 'number' && node.frame.Height > 0) ? node.frame.Height : null;

• File: packages/core/src/maestro-screenshot.js:173

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: In self-hosted mode sessionId is undefined, so the cache keys on undefined. Correct for a single self-hosted run, but a subsequent self-hosted snapshot with a different platform/pngDims on the same Percy instance would reuse the first derived value. Unlikely to bite (one self-hosted run per instance) but fragile.

• Suggestion: Use a sentinel key and document the scope: const cacheKey = sessionId ?? '__self_hosted__'; then get/set on cacheKey.

Low / informational (non-blocking, not anchored)

• maestro-hierarchy.js (deriveAndroidInsets): runs dumpsys window (full WM output) and regex-scans stdout; bounded but large on some devices — dumpsys window displays is a future scoping optimization.
• maestro-screenshot.js: deriveDeviceInsets uses production defaultExecAdb/defaultHttpRequest (by design); first self-hosted iOS snapshot may pay up to ~1500ms before fallback when the driver is unreachable — mitigated by caching.
• api.js / maestro-screenshot.js: encodeURLSearchParams moved to utils.js, parsePngDimensions re-exported from maestro-screenshot.js — confirmed no external/source callers depend on the old api.js export locations; resolved on next yarn build.

---

Verdict: PASS — faithful decomposition with security invariants intact and comprehensive coverage for the new inset derivation; two Medium nits worth tightening but nothing blocking.

[Sriram567]

Claude Code PR Review

PR: #2315 • Head: b5bfa86 • Reviewers: stack:code-reviewer

Follows the earlier review of efd6501 (pre-marker format). This run scopes to the novel delta efd6501a…b5bfa86b — the master merge + conflict resolution — since the rest of the PR was already reviewed and passed at efd6501.

Summary

Merge of master into the PR branch, resolving one conflict in api.js (kept the decomposed /percy/maestro-screenshot one-liner while preserving master's #2314 sync-upload fixes to /percy/comparison + /percy/automateScreenshot) and manually porting #2314's .catch(reject) fix into the extracted maestro-screenshot.js (the module was branched before #2314 and still carried the old for await drain that throws "upload is not async iterable").

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None.
High	Security	Authentication/authorization checks present	N/A	No auth surface in the delta.
High	Security	Input validation and sanitization	N/A	Delta is sync-plumbing only; validation unchanged.
High	Security	No IDOR — resource ownership validated	N/A	Not touched by the delta.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	.catch(reject) port is structurally identical to the two proven api.js routes; handleSyncJob call + 'comparison' type arg correct; fire-and-forget path untouched.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Both error paths (sync-queue callback.reject, and a rejected upload Promise via .catch(reject)) surface as data.error.
High	Correctness	No race conditions or concurrency issues	Pass	Same Promise/queue mechanics as the shipped #2314 fix.
Medium	Testing	New code has corresponding tests	Pass	Merged #2314 rewrote all three sync tests to the real Promise shape; maestro relay-wiring tests intact. See Medium finding on one direct-reject test gap.
Medium	Testing	Error paths and edge cases tested	Pass	callback.reject path tested for maestro (data.error='boom'); direct Promise.reject path tested for /comparison.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI green incl. @percy/core NYC 100% coverage gate on this HEAD.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	Not applicable to the delta.
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Port mirrors #2314's pattern verbatim across all three sync routes.
Medium	Quality	Changes are focused (single concern)	Pass	Delta is exactly: conflict resolution + the one ported fix.
Low	Quality	Meaningful names, no dead code	Pass	No stale drain/for-await/iterCount code survives in src or tests.
Low	Quality	Comments explain why, not what	Pass	Ported #2314's explanatory comment (accurate; references percy.yield.upload()).
Low	Quality	No unnecessary dependencies added	Pass	None.

Findings

• File: packages/core/test/api.test.js (maestro sync describe, ~line 1595)
• Severity: Medium
• Reviewer: stack:code-reviewer
• Issue: /percy/comparison has a dedicated test (api.test.js:371) that mocks percy.upload returning Promise.reject(...) and asserts { data: { error: ... } }, exercising the .catch(reject) arm (rejection before the sync-queue callback fires). The maestro-screenshot block has no equivalent — it tests the callback.reject path but not a direct Promise.reject.
• Impact: Non-gating. The Test/coverage CI job already passed at 100% on this HEAD, so the .catch(reject) branch in maestro-screenshot.js is covered; this is a clarity/parity gap, not a coverage failure.
• Suggestion: Optionally add, in the maestro describe block:

  it('sync mode: surfaces a rejected upload Promise as data.error', async () => {
    spyOn(percy, 'upload').and.callFake(() => Promise.reject(new Error('upload boom')));
    await percy.start();
    await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, platform: 'android', sync: true }))
      .toBeResolvedTo(jasmine.objectContaining({ data: { error: 'upload boom' } }));
  });

Low / informational (non-blocking, no action on this PR)

• api.test.js:1599 — callback.reject(...) returns undefined; the mock relies on the Promise executor running synchronously so snapshotPromise is already rejected before .catch(reject) is reached. Fragile if the executor is ever made async, but this is a pre-existing pattern inherited from fix(core): [PER-9706] sync Automate snapshots fail with "upload is not async iterable" #2314, not introduced here.

---

Verdict: PASS — the merge is clean and the .catch(reject) port into maestro-screenshot.js is a faithful, correct application of the shipped #2314 fix; no High/Critical findings, no regressions. The single Medium is an optional test-parity nicety already covered by the green coverage gate.

[Sriram567]

Claude Code PR Review

PR: #2315 • Head: 8d9546f • Reviewers: stack:code-reviewer

Continues the previous review — changes since b5bfa86 (delta: the --driver-host-port auto-injection commit 5073c2f9 + review-fix 8d9546f5).

Summary

Adds maybeInjectDriverHostPort to @percy/cli-app — on Maestro ≥ 2.6 (iOS), picks a free port, splices --driver-host-port <port> into maestro test argv, and mirrors PERCY_IOS_DRIVER_HOST_PORT so the relay reaches /viewHierarchy deterministically. Version-gated, iOS-gated, shard-skipping, override-respecting, graceful on failure. Review findings addressed in 8d9546f5.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None.
High	Security	Authentication/authorization checks present	N/A	No auth surface in the delta.
High	Security	Input validation and sanitization	Pass	spawnSync('maestro',['--version']) — no shell, fixed args; port validated 1..65535 (mirrors parseIosDriverHostPort).
High	Security	No IDOR — resource ownership validated	N/A	Not applicable.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	Guard ladder + version compare (≥2.6, forward-compat 3.x) + override precedence verified correct by reviewer.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Every failure path degrades to the 7001 probe with a log; the one gap (pickFreePort reject) fixed in 8d9546f5.
High	Correctness	No race conditions or concurrency issues	Pass	process.env write is same-process (relay); per-app:exec-process, no cross-invocation leakage.
Medium	Testing	New code has corresponding tests	Pass	29 exec.test.js scenarios incl. the new pickFreePort-reject path.
Medium	Testing	Error paths and edge cases tested	Pass	version<2.6, spawn throw/ENOENT/non-zero/unparseable, empty/stale env, sharding, reject-path.
Medium	Testing	Existing tests still pass (no regressions)	Pass	79/79 cli-app specs; validated E2E on iPhone 16 Pro + Maestro 2.6.1.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	One maestro --version spawn per app:exec on the test path only.
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors maybeInjectScreenshotDir (dual argv+env), DI shims istanbul-ignored.
Medium	Quality	Changes are focused (single concern)	Pass	Delta is exactly the driver-host-port feature + its review-fix.
Low	Quality	Meaningful names, no dead code	Pass	Finder scan-start made consistent (i=1) in 8d9546f5.
Low	Quality	Comments explain why, not what	Pass	Stale "two helpers" comment refreshed in 8d9546f5.
Low	Quality	No unnecessary dependencies added	Pass	Stdlib net + child_process only.

Findings

All findings from the delta review were addressed in 8d9546f5:

• packages/cli-app/src/maestro-inject.js Medium — unhandled pickFreePort() rejection could crash app:exec and leave Maestro 2.6+ with no port Resolved — wrapped in try/catch; now warns and no-ops (falls back to the 7001 relay probe), with a covering test. Reviewer: stack:code-reviewer
• packages/cli-app/src/exec.js Low — stale "two helpers" comment + argv example Resolved — comment refreshed (three helpers; iOS argv shape). Reviewer: stack:code-reviewer
• packages/cli-app/src/maestro-inject.js Low/info — findDriverHostPortValue scanned from i=0 Resolved — starts at i=1 for consistency with sibling finders. Reviewer: stack:code-reviewer

Informational (no action): -s= equals-form not in the sharding-prefix set (picocli short flags rarely accept =; Maestro doesn't use it).

Reviewer also verified clean: guard-ladder order/precedence, the async-generator await, argv splicing composition, process.env semantics, NYC 100% coverage of every branch, and command-injection safety.

---

Verdict: PASS — the --driver-host-port injector is correct, well-tested (79/79 + E2E-validated on an iOS simulator), and the reviewer's one Medium + two Low findings were all fixed in 8d9546f5. No High/Critical, no open findings.