Uh oh!
There was an error while loading. Please reload this page.
fix(core): follow BS App Automate tmp dir relocation for Maestro screenshots - #2353
Conversation
prklm10
left a comment
There was a problem hiding this comment.
Claude Code Review (automated) — 5 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| // BrowserStack-mode only — self-hosted scoping stays on | ||
| // PERCY_MAESTRO_SCREENSHOT_DIR. | ||
| export function appAutomateTmpDir() { | ||
| let dir = process.env.PERCY_APP_AUTOMATE_TMP_DIR || '/usr/local/.browserstack/app-automate-tmp'; |
There was a problem hiding this comment.
[Medium] Rollout-skew risk: no legacy /tmp fallback
If any BS host fleet segment (Linux/Android vs macOS/iOS are separate fleets) still writes to /tmp/{sid}, this CLI version globs only the new root and hard-404s — and non-relocated hosts by definition run older injection that won't set PERCY_APP_AUTOMATE_TMP_DIR=/tmp.
Suggestion: Confirm the relocation is fleet-wide for both Android and iOS hosts before release (stragglers can inject PERCY_APP_AUTOMATE_TMP_DIR=/tmp), or add a legacy fallback that retries the /tmp pattern (with matching scopeRoot) when the new-root glob is empty and the env var is unset.
Reviewer: stack:code-reviewer
| // PERCY_MAESTRO_SCREENSHOT_DIR. | ||
| export function appAutomateTmpDir() { | ||
| let dir = process.env.PERCY_APP_AUTOMATE_TMP_DIR || '/usr/local/.browserstack/app-automate-tmp'; | ||
| return dir.replace(/[/\\]+$/, ''); |
There was a problem hiding this comment.
[Low] No shape validation on PERCY_APP_AUTOMATE_TMP_DIR
Asymmetric with PERCY_MAESTRO_SCREENSHOT_DIR, which gets absolute-path + existing-dir checks with actionable 400s. A relative value here yields a cwd-relative glob surfacing only as an opaque 404; / trims to '', silently making the filesystem root the tmp root. Not a security issue (env is host-injected; containment fails closed), but host misconfiguration will be painful to debug.
Suggestion: After trimming, fall back to the default (optionally with a warn log) when the value is empty or not absolute.
Reviewer: stack:code-reviewer
| scopeRoot = platform === 'ios' | ||
| ? `/tmp/${sessionId}` | ||
| : `/tmp/${sessionId}_test_suite`; | ||
| ? `${appAutomateTmpDir()}/${sessionId}` |
There was a problem hiding this comment.
[Low] scopeRoot and glob root resolved by separate env reads
The containment boundary (scopeRoot) and the glob search root come from independent appAutomateTmpDir() calls; a mid-request env change would make them diverge. Divergence fails closed (404), so this is coherence-only.
Suggestion: Optionally resolve the root once in the handler and derive the glob root from the already-passed scopeRoot, as the self-hosted branch does.
Reviewer: stack:code-reviewer
| // Real-fs root (matched by the top-level $bypass) — fast-glob caches its | ||
| // fs bindings on first import, so a memfs-only root created in a later | ||
| // test is invisible to it; real-fs fixtures sidestep the staleness. | ||
| describe('PERCY_APP_AUTOMATE_TMP_DIR override', () => { |
There was a problem hiding this comment.
[Low] Override tests cover the Android branch only
The iOS glob and scopeRoot expressions are never exercised under an override (low risk — same tmpRoot expression as Android).
Suggestion: Optionally add one iOS spec mirroring the Android override test, with a fixture under ${CUSTOM_ROOT}/${SID}/emu_maestro_debug_abc/....
Reviewer: stack:code-reviewer
| expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-CUSTOM-ROOT').toString('base64')); | ||
| }); | ||
| it('tolerates a trailing slash on the override', async () => { |
There was a problem hiding this comment.
[Low] Trailing-slash spec doesn't strictly pin the trim behavior
fast-glob normalizes // in patterns and realpath collapses duplicate separators, so this spec would likely pass even without the replace(/[/\\]+$/, '') trim — it documents intent more than it guards the code.
Suggestion: Optionally add a direct unit assertion on the exported helper, e.g. expect(appAutomateTmpDir()).toBe(CUSTOM_ROOT) with the env set to ${CUSTOM_ROOT}///.
Reviewer: stack:code-reviewer
Claude Code PR ReviewPR:#2353 • Head:f28af57 • Reviewers: stack:code-reviewer SummaryFixes the Maestro screenshot relay after BrowserStack App Automate hosts relocated session artifact directories from Review Table
Findings
Informational (not counted against the verdict): the fixed real-fs fixture name Verdict: PASS — approved by stack:code-reviewer; 1 Medium deployment-verification question (fleet-wide relocation confirmation) and 4 non-blocking polish items. |
prklm10
left a comment
There was a problem hiding this comment.
Claude Code Review (automated) — 3 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| // producing a cwd-relative glob. BrowserStack-mode only — self-hosted scoping | ||
| // stays on PERCY_MAESTRO_SCREENSHOT_DIR. | ||
| export function appAutomateTmpDir() { | ||
| let dir = (process.env.PERCY_APP_AUTOMATE_TMP_DIR || '/tmp').replace(/[/\\]+$/, ''); |
There was a problem hiding this comment.
[Low] Silent fallback hides host misconfiguration (+ no whitespace trim)
A set-but-invalid value — non-absolute, trimming to empty, or carrying incidental whitespace from shell injection (whitespace fails isAbsolute too) — is silently discarded; the operator only sees Screenshot not found 404s against /tmp. Partially mitigated because the 404 message echoes the searched pattern.
Suggestion: two-line hardening: add .trim() before the separator trim, and emit a debug log when a set value is rejected, e.g. Ignoring non-absolute PERCY_APP_AUTOMATE_TMP_DIR: <raw>.
Reviewer: stack:code-reviewer
| scopeRoot = platform === 'ios' | ||
| ? `/tmp/${sessionId}` | ||
| : `/tmp/${sessionId}_test_suite`; | ||
| ? `${appAutomateTmpDir()}/${sessionId}` |
There was a problem hiding this comment.
[Low] Tmp root still read independently at three sites per request
scopeRoot here, the glob root, and the manual walker each call appAutomateTmpDir() separately. Divergence between reads fails closed (404, never an escape), so this is coherence-only — carried over from the prior round.
Suggestion: in the BS branch of locateScreenshot, derive the glob from the already-passed scopeRoot ({scopeRoot}/logs/*/screenshots/… Android, {scopeRoot}/*_maestro_debug_*/** iOS), which also unifies the two branches' slash normalization. Fine as a fast-follow.
Reviewer: stack:code-reviewer
| // Real-fs root (matched by the top-level $bypass) — fast-glob caches its | ||
| // fs bindings on first import, so a memfs-only root created in a later | ||
| // test is invisible to it; real-fs fixtures sidestep the staleness. | ||
| describe('PERCY_APP_AUTOMATE_TMP_DIR override', () => { |
There was a problem hiding this comment.
[Low] Override coverage is Android-glob-only end-to-end
The iOS override glob shares the helper (low risk, and helper semantics are now pinned platform-independently by the direct assertions below) but is never exercised under a custom root.
Suggestion: optionally add one iOS spec under CUSTOM_ROOT reusing the existing makePngHeader fixture helper.
Reviewer: stack:code-reviewer
prklm10
commented
Jul 27, 2026
Claude Code PR ReviewPR:#2353 • Head:d07e89a • Reviewers: stack:code-reviewer SummaryFixes the Maestro screenshot relay after BrowserStack App Automate hosts relocated session artifact directories away from Review Table
Prior-round concerns: rollout-skew (legacy fallback) and env shape validation — resolved with test pins; trailing-slash trim — resolved via direct helper assertions; separate env reads and iOS override coverage — carried forward as Low (Findings 2–3). Findings
Informational (not counted against the verdict): on win32 a degenerate root like Verdict: PASS — approved by stack:code-reviewer; both prior blocking concerns resolved with test pins, containment security preserved under relocated roots; remaining findings are Low polish (a two-line helper hardening is recommended pre-merge, the rest can follow up). |
…_TMP_DIR
BrowserStack hosts can relocate the App Automate session directories away
from /tmp/{sessionId}, which broke the Maestro screenshot relay's hardcoded
/tmp globs and scope roots. Route every BrowserStack-mode path (glob
patterns, manual-walker fallback, containment scope roots) through a new
appAutomateTmpDir() helper that reads PERCY_APP_AUTOMATE_TMP_DIR — injected
by the host with the relocated root, so no infra path is baked into the
public CLI — and falls back to the legacy /tmp convention when the var is
unset or not absolute, keeping hosts with older injection working during
rollout. Self-hosted mode (PERCY_MAESTRO_SCREENSHOT_DIR) is untouched.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>d07e89a to
fb9afa0Compareprklm10
commented
Jul 27, 2026
Claude Code PR ReviewPR:#2353 • Head:fb9afa0 • Reviewers: stack:code-reviewer SummaryFixes the Maestro screenshot relay after BrowserStack App Automate hosts relocated session artifact directories away from
Review Table
Findings
Informational: on win32 a degenerate root like Verdict: PASS — approved by stack:code-reviewer on this exact diff; remaining findings are Low polish already tracked as inline comments on the PR. |
Summary
/tmp/{sessionId}, breaking the Maestro screenshot relay's hardcoded/tmpglobs and scope roots — screenshot lookups started 404'ing.scopeRootused by the realpath containment check) now route through a newappAutomateTmpDir()helper.PERCY_APP_AUTOMATE_TMP_DIRenv var — injected by the host with the relocated root — and falls back to the legacy/tmpconvention when unset, so no internal infra path is hardcoded in the CLI and hosts with older injection keep working during rollout. Trailing separators are trimmed, backslashes normalized for the glob, and non-absolute values fall back to/tmp.PERCY_MAESTRO_SCREENSHOT_DIR.Test plan
/percy/maestro-screenshotspecs (Android + iOS globs,filePathacceptance/containment, PNG-fill, regions) — all passing against the default root.PERCY_APP_AUTOMATE_TMP_DIR overridespecs: globbing under an overridden root (real-fs fixture), trailing-slash tolerance, non-absolute fallback to/tmp, direct trim/fallback assertions on the exported helper, andfilePathcontainment re-scoping to the overridden root.packages/core/test/api.test.jsrun locally: all maestro specs pass; the single unrelated failure (when the server is disabled…, ECONNREFUSED vs AggregateError) also fails on a clean master checkout — pre-existing local Node environment issue.🤖 Generated with Claude Code