fix(core): let BS hosts inject a complete Maestro scope root

[rishigupta1599]

Summary

Follow-up to #2353. That PR assumed the App Automate tmp-dir relocation only moved the root while preserving the {root}/{sessionId}{_test_suite} layout beneath it. That holds on Android but not on iOS.

	Relocated path	What the CLI composes	PERCY_APP_AUTOMATE_TMP_DIR enough?
Android	<aa-tmp>/<device>/test-suites/<sid>_test_suite/logs/*/screenshots/	{TMP}/{sid}_test_suite/logs/*/screenshots/	✅
iOS	<aa-tmp>/<device>/logs/maestro_debug_*/	{TMP}/{sid}/*_maestro_debug_*/	❌

The iOS layout drops the sessionId path segment entirely — the CLI appends it, so no tmp-root value can absorb <device>/logs — and drops the <device>_ prefix that the *_maestro_debug_* glob keys on. Lookups there 404 no matter what PERCY_APP_AUTOMATE_TMP_DIR is set to.

What this adds

PERCY_MAESTRO_BS_SCOPE_ROOT, for layouts the convention can no longer express. When set, the value is the scope root:

• the relay globs {root}/**/{name}.png with no layout assumption
• the manual-walker fallback recurses the same root instead of the platform convention
• the realpath containment check anchors on it

The security boundary is relocated, never widened — an out-of-root filePath still 404s. Non-absolute values are ignored so a malformed injection falls back to the composed convention rather than producing a cwd-relative root. Android (PERCY_APP_AUTOMATE_TMP_DIR) and self-hosted (PERCY_MAESTRO_SCREENSHOT_DIR) are untouched.

No existence pre-check on the override, unlike self-hosted: this is host config, not customer config, so a stale root should surface as the same 404 the containment check emits rather than a 400 aimed at the customer.

Host side

• browserstack/realmobile — injects PERCY_MAESTRO_BS_SCOPE_ROOT (needs this PR released)
• browserstack/mobile — injects PERCY_APP_AUTOMATE_TMP_DIR, fixed by fix(core): follow BS App Automate tmp dir relocation for Maestro screenshots #2353 alone

Both host PRs pin 1.32.5 today; realmobile needs a second bump once this ships.

Test plan

8 new specs in packages/core/test/api.test.js: locating a screenshot the platform convention can't reach, android under the same override, override winning over PERCY_APP_AUTOMATE_TMP_DIR, trailing-slash tolerance, non-absolute fallback, filePath containment re-anchored on the override (in-root resolves, out-of-root 404s), missing root 404, plus direct trim/null assertions on the exported helper.

Local run caveat, stated plainly: every real-fs-fixture spec in api.test.js fails on my machine with ENOTEMPTY from the afterEach rmSync, including blocks this PR doesn't touch. Measured baseline on clean master: 15 failures, all ENOTEMPTY. With this PR: 23 — a delta of exactly the 8 new specs, same environmental cause, no new failure mode. Those blocks need CI to actually be exercised.

Because of that, I verified the logic directly against the real filesystem using the exact relocated layouts — 7/7: helper trim/absolute semantics, #2353-alone still 404ing on the realmobile layout, explicit root locating the file, out-of-root filePath rejected, in-root accepted; plus a separate check that the Android glob still composes correctly from PERCY_APP_AUTOMATE_TMP_DIR.

ESLint clean on all three changed files. (yarn lint for the whole package fails environmentally here — 54 identical "No Babel config file detected" errors across every test file, untouched ones included.)

Ref: PER-10040, AAP-18965

🤖 Generated with Claude Code

[aryanku-dev]

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

[aryanku-dev]

[Medium] No symlink-escape test for the overridden scope root

The self-hosted arm already has 404s when a globbed file resolves outside the root (symlink escape); this override block has no analogue. The containment code is shared, so present-day risk is low — but a future edit that shortcuts realpath for the "host-trusted" root would go undetected, and re-anchored containment is the property this PR explicitly claims.

Suggestion: Add a spec here that places a symlink inside REALMOBILE_DIR pointing outside SCOPE_ROOT, then asserts a 404 matching /resolved outside session dir/.

Reviewer: stack-code-reviewer

[aryanku-dev]

[Low] A rejected override falls through silently

When PERCY_MAESTRO_BS_SCOPE_ROOT is set but rejected by bsScopeRootOverride() (non-absolute, or reducing to /), this branch composes the convention root with no log line. The accepted path logs at debug; the rejected path is silent, so a host-side typo surfaces only as 404s indistinguishable from an ordinary missing file.

Suggestion: Emit percy.log.warn here when process.env.PERCY_MAESTRO_BS_SCOPE_ROOT is truthy but the helper returned null, mirroring the existing debug log.

Reviewer: stack-code-reviewer

[aryanku-dev]

[Low] Rejection of root / is incidental to the trim regex, not an explicit guard

'/' is rejected only because the trailing-separator strip turns it into '', which then fails path.isAbsolute. The behaviour is pinned by a test, but the safety property — an override can never widen the root to the whole filesystem — is incidental in the code, so a future refactor of the trim could silently reintroduce it.

Suggestion: Record the intent at the return, e.g. // '/' trims to '' and is rejected here — intentional; prevents an override that would recurse the whole filesystem.

Reviewer: stack-code-reviewer

[aryanku-dev]

[Low] No multi-match mtime tie-break test for the override glob

recursiveScope routes the override through the same files.length > 1 mtime-descending sort, but no spec exercises it. This matters more here than for the convention glob precisely because the layout constraint was removed: a same-named PNG from a second flow anywhere under the session root can now match.

Suggestion: Add a spec with two ${SS_NAME}.png fixtures at different depths and mtimes under SCOPE_ROOT, asserting the newer one is chosen.

Reviewer: stack-code-reviewer

[aryanku-dev]

[Low] Override applies to Android too — worth documenting on the host side

Design note, not a defect. The override is applied regardless of platform, so an Android session under it also loses the _test_suite/logs/*/screenshots structural guard. That is intentional and tested (applies to android too), but the PR description frames the feature as iOS-only, and nothing in code prevents a host from setting the var too broadly and quietly loosening Android's guard.

Suggestion: State in the BS-host runbook that this is set per-invocation for iOS sessions, not host-wide, unless Android's layout has also changed.

Reviewer: stack-code-reviewer

[aryanku-dev]

Claude Code PR Review

PR: #2357 • Head: 0b084ed • Reviewers: stack-code-reviewer

Summary

Adds PERCY_MAESTRO_BS_SCOPE_ROOT so a BrowserStack host can inject a complete Maestro session scope root for layouts the {appAutomateTmpDir()}/{sessionId}{_test_suite} convention can no longer express (the iOS realmobile relocation); when set, the relay globs {root}/**/{name}.png, the manual-walker fallback recurses that root, and the existing realpath containment check re-anchors on it.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Config read from process.env only; nothing embedded.
High	Security	Authentication/authorization checks present	Pass	No auth surface added. The relay's authz-analog is the realpath containment check, which is preserved (see next rows).
High	Security	Input validation and sanitization	Pass	bsScopeRootOverride() rejects non-absolute values and (via the trailing-separator trim) /; name remains SAFE_ID-validated upstream.
High	Security	No IDOR — resource ownership validated	Pass	Containment logic at maestro-screenshot-file.js:193-205 is untouched and generic over scopeRoot: both file and root are realpath'd, then compared with startsWith(prefix + '/'). The trailing slash defeats sibling-prefix collisions (/root vs /root-evil), and realpath defeats symlink swaps. Boundary is relocated, not widened — the PR's central claim holds on inspection.
High	Security	No SQL injection (parameterized queries)	N/A	No database access in this change.
High	Correctness	Logic is correct, handles edge cases	Pass	recursiveScope is derived in locateScreenshot rather than trusted from the caller, so selfHosted keeps its existing meaning; trailing-slash, non-absolute, and missing-root cases all behave as documented.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Missing/out-of-root still throws ServerError(404). The deliberate omission of an existence pre-check (vs self-hosted's 400) is reasoned in-code: host config should not produce a customer-facing 400.
High	Correctness	No race conditions or concurrency issues	Pass	Multi-match tie-break (mtime-descending, :177-181) is pre-existing and unchanged; the override only widens what reaches it.
Medium	Testing	New code has corresponding tests	Pass	8 new specs covering locate, Android parity, precedence over PERCY_APP_AUTOMATE_TMP_DIR, trailing slash, non-absolute fallback, filePath re-anchoring, missing root, and helper trim/null semantics.
Medium	Testing	Error paths and edge cases tested	Fail	Two gaps in exactly the risk-prone areas: no symlink-escape test for the override root (the self-hosted arm has one), and no multi-match tie-break test for the now-unbounded override glob. See Findings 1 and 4.
Medium	Testing	Existing tests still pass (no regressions)	Unverified	No clean local run was achievable. Two unrelated environmental failure classes hit the file: a Chromium snapshot download 404, and ENOTEMPTY from rmSync cleanup that fires identically in the pre-existing, untouched PERCY_APP_AUTOMATE_TMP_DIR override block. This corroborates the PR's own account, but it means CI is the only place the new assertions get exercised — confirm this file is green in CI before merge.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	The override glob is broader than the convention glob (no *_maestro_debug_* / _test_suite/logs segment requirement), but bounded by a host-narrowed per-session root, and the walker keeps its depth-15 cap.
Medium	Performance	Long-running tasks use background jobs	N/A	Single-request file lookup.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors appAutomateTmpDir()'s validate-and-trim shape and reuses the self-hosted recursive-glob path rather than inventing new containment logic.
Medium	Quality	Changes are focused (single concern)	Pass	Three files, one concern; the walker refactor into walkFrom(root, accept) is in service of it.
Low	Quality	Meaningful names, no dead code	Pass	bsScopeRootOverride, recursiveScope, scopedGlob all read clearly.
Low	Quality	Comments explain why, not what	Pass	Unusually thorough — the motivating layout change, the security rationale, and the 404-vs-400 choice are all recorded at the point of decision.
Low	Quality	No unnecessary dependencies added	Pass	None added.

Findings

1.

• File: packages/core/test/api.test.js:2009
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: No symlink-escape test exists for the new override root. The self-hosted arm already has 404s when a globbed file resolves outside the root (symlink escape); the override block has no analogue. The containment code is shared so present-day risk is low, but a future edit that shortcuts realpath for the "host-trusted" root would go undetected — and this is the one property the PR explicitly claims.
• Suggestion: Add a spec in the PERCY_MAESTRO_BS_SCOPE_ROOT override block: place a symlink inside REALMOBILE_DIR pointing outside SCOPE_ROOT, then assert a 404 matching /resolved outside session dir/.

2.

• File: packages/core/src/maestro-screenshot.js:143
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: When PERCY_MAESTRO_BS_SCOPE_ROOT is set but rejected (non-absolute, or reducing to /), the request falls through to the composed convention with no log line. The accepted path logs at debug; the rejected path is silent, so a host-side typo surfaces only as 404s indistinguishable from an ordinary missing file.
• Suggestion: In the fall-through else, emit percy.log.warn when process.env.PERCY_MAESTRO_BS_SCOPE_ROOT is truthy but bsScopeRootOverride() returned null, mirroring the existing debug log.

3.

• File: packages/core/src/maestro-screenshot-file.js:40
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: Rejecting the literal value / is a side effect of the trailing-separator regex ('/' → '', which fails path.isAbsolute) rather than an explicit guard. The behaviour is pinned by a test, but the safety property — an override can never widen the root to the entire filesystem — is incidental in the code, so a future refactor of the trim could silently reintroduce it.
• Suggestion: Add a one-line comment at the return: // '/' trims to '' and is rejected here — intentional; prevents an override that would recurse the whole filesystem.

4.

• File: packages/core/test/api.test.js:2034
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: No multi-match tie-break test for the override glob, though recursiveScope routes it through the same files.length > 1 mtime sort. This matters more here than for the convention glob precisely because the layout constraint was removed: a same-named PNG from a second flow anywhere under the session root can now match.
• Suggestion: Add a spec with two ${SS_NAME}.png fixtures at different depths and mtimes under SCOPE_ROOT, asserting the newer one is chosen.

5.

• File: packages/core/src/maestro-screenshot.js:139
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: Design/documentation note, not a defect. The override is applied regardless of platform, so an Android session under it also loses the _test_suite/logs/*/screenshots structural guard — intentional and tested (applies to android too), but the PR description frames the feature as iOS-only ("Android kept its shape... stays on PERCY_APP_AUTOMATE_TMP_DIR"). Nothing in code prevents a host from setting it too broadly and quietly loosening Android's guard.
• Suggestion: State in the BS-host runbook that this must be set per-invocation for iOS sessions, not host-wide, unless Android's layout has also changed.

---

Verdict: PASS — the security boundary is genuinely relocated rather than widened, and the reasoning is well documented; the open items are test-coverage and observability gaps, none gating. Please confirm packages/core/test/api.test.js is green in CI before merge, since the new specs could not be exercised locally.