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: self-hosted iOS status-bar derivation needs PERCY_IOS_DRIVER_HOST_PORT, which maestro-inject.js does not set, so self-hosted iOS falls back to the SDK default there (graceful). Tracked as a follow-up.

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 (so self-hosted iOS gets exact status-bar insets instead of the SDK fallback) — optional enhancement.

🤖 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.