feat(cli-app): auto-resolve screenshot dir + WARN on no-addr injection (R1+R3)#2263
Merged
Sriram567 merged 1 commit intoJun 2, 2026
Conversation
…put-dir; WARN on no-addr injection skip
Removes the last piece of customer-side bookkeeping for self-hosted
Maestro+Percy. Today customers must export PERCY_MAESTRO_SCREENSHOT_DIR
AND pass --test-output-dir <same path> to maestro test. After this PR,
`percy app:exec` does both automatically.
New helper `maybeInjectScreenshotDir(ctx, log)` next to the existing
`maybeInjectMaestroServer`. Resolution order:
1. Customer set BOTH env + --test-output-dir flag → trust them.
2. Customer set env only → use env value, inject matching flag.
3. Customer set flag only → use flag value, mirror to env.
4. Neither set → try ${CWD}/.percy-out. On mkdir failure (EACCES,
EROFS, EEXIST), fall back to ${TMPDIR}/percy-maestro-<pid> with
a WARN log explaining why.
The env var and the flag are always kept aligned to the same path.
The SDK reads the env var; Maestro reads the flag — both pointing at
the same dir is the contract.
Also adds a WARN log in `maybeInjectMaestroServer` when `percy.address()`
is falsy (percy disabled, start failed). Previously this skip was
silent — customer's maestro test would run, no snapshots upload, Percy
build would finalize empty with no log hint. The WARN now tells the
customer what won't happen and how to fix it (set PERCY_TOKEN).
Customer-supplied `-e PERCY_SERVER` override skip does NOT warn —
that's legitimate flow control, not a problem.
Tests:
- 14 new scenarios for maybeInjectScreenshotDir (happy path, env
override, flag override, both, EACCES/EROFS/EEXIST fallbacks,
hierarchy/npx/python/short-args skip, basename match on full path).
- 2 new scenarios for maybeInjectMaestroServer's WARN log.
- 30 of 30 cli-app specs pass.
d9d508d
into
feat/self-hosted-maestro-percy-v1
38 of 40 checks passed
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
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
Stacks on top of #2261 — the consolidated self-hosted Maestro+Percy V1. Completes the customer-side UX by removing the last manual-env-var step.
After cli#2261 merges + this PR merges, the canonical self-hosted Maestro+Percy command goes from:
to just:
percy app:exec -- maestro test flow.yamlCustomer overrides (either via
PERCY_MAESTRO_SCREENSHOT_DIRenv var or--test-output-dirin maestro args) always win — same precedence pattern as cli#2261's-e PERCY_SERVERauto-inject.What changes
packages/cli-app/src/exec.js— addsmaybeInjectScreenshotDir(ctx, log)next to the existingmaybeInjectMaestroServer. The wrapped exec callback calls both helpers before yielding to cli-exec's spawn.Resolution order for the screenshot dir:
PERCY_MAESTRO_SCREENSHOT_DIRenv AND--test-output-dirflag--test-output-dir <env>flagPERCY_MAESTRO_SCREENSHOT_DIRenv so the SDK + CLI relay see the same path${CWD}/.percy-outviamkdirSync({ recursive: true }). On mkdir failure (EACCES, EROFS, EEXIST as a file), fall back to${TMPDIR}/percy-maestro-<pid>with a WARN logThe env var and the flag are always aligned to the same path. The SDK reads the env var; Maestro reads the flag — both pointing at the same dir is the contract.
Plus a diagnostic WARN log (R3) in
maybeInjectMaestroServerwhenpercy.address()is falsy (percy disabled, start failed). Previously this skip was silent — customer'smaestro testwould run, no snapshots upload, Percy build would finalize empty with no log hint. The WARN now tells the customer what won't happen and how to fix it. Customer-supplied-e PERCY_SERVERoverride skip does NOT warn (intentional flow control).Origin
Brainstorm → plan:
percy-maestro/docs/brainstorms/2026-06-02-selfhosted-followup-bundle-requirements.md(R1 + R3)cli/docs/plans/2026-06-02-001-feat-auto-screenshot-dir-plan.mdTesting
maybeInjectScreenshotDir(happy path; env override; flag override; both set; EACCES / EROFS / EEXIST fallbacks; hierarchy / npx / python / short-args skips; basename match on absolute paths). 2 new scenarios for themaybeInjectMaestroServerWARN log (fires on no-addr; does NOT fire on customer-override).example-percy-maestro-selfhostedwithout theexport PERCY_MAESTRO_SCREENSHOT_DIR=...step and reach the same green Percy build. Validation runbook + example repo README will be updated in follow-up doc PRs after this lands.Stacking
feat/self-hosted-maestro-percy-v1(cli#2261). Rebase tomasteronce feat(cli): self-hosted (non-BrowserStack) Maestro + Percy support — V1 #2261 merges.Post-Deploy Monitoring & Validation
Snapshot command was not calledafter a flow that ran without explicitPERCY_MAESTRO_SCREENSHOT_DIR— would indicate the auto-resolved dir doesn't match what the SDK / Maestro use.${TMPDIR}/percy-maestro-<pid>instead of${CWD}/.percy-out. High rate (>5%) would indicate widespread read-only CWDs.PERCY_TOKEN.percy app:exec -- maestro test flows/regions.yamlwith no env exports → expect green Percy build with snapshots in${CWD}/.percy-out/screenshots/.chmod -w .on the CWD, repeat → expect green Percy build using TMPDIR fallback + a WARN line in stdout.percy app:exec --disable -- maestro test flows/regions.yaml→ expect the no-addr WARN in stdout.Snapshot command was not callederrors on builds where the customer didn't change anything.app:exec, which BS doesn't wrap maestro through), but watch for one cycle.🤖 Generated with Claude Opus 4.7 via Claude Code + Compound Engineering v2.54.0