Skip to content

feat(cli): auto-inject PERCY_SERVER for maestro test + PERCY_CLI_API alias#2254

Closed
Sriram567 wants to merge 2 commits into
feat/self-hosted-maestro-percyfrom
feat/cli-app-auto-inject-percy-server
Closed

feat(cli): auto-inject PERCY_SERVER for maestro test + PERCY_CLI_API alias#2254
Sriram567 wants to merge 2 commits into
feat/self-hosted-maestro-percyfrom
feat/cli-app-auto-inject-percy-server

Conversation

@Sriram567
Copy link
Copy Markdown
Contributor

Summary

Two small UX-completion changes on top of #2248. Both remove the port-pairing footgun every native-SDK customer hits self-hosted, with no behavior change on the BrowserStack path.

Unit 1 — PERCY_CLI_API alias in @percy/cli-exec

percy app:exec already exports PERCY_SERVER_ADDRESS into the child process. The Appium Python SDK reads PERCY_CLI_API instead. Today this works only by coincidence — both default to port 5338. With --port overrides (multi-device parallelism), the SDK silently points at the wrong CLI. This adds env.PERCY_CLI_API = percy?.address() alongside the existing exports so any SDK reading PERCY_CLI_API auto-aligns to whatever port app:exec picked.

Unit 2 — auto-inject -e PERCY_SERVER for maestro test in @percy/cli-app

Maestro's GraalJS sandbox does not inherit the parent process's env, so PERCY_SERVER_ADDRESS is invisible to the SDK. Customer's only channel is Maestro -e flags. Today every self-hosted customer threads -e PERCY_SERVER=http://localhost:<port> through every Maestro invocation themselves, pairing ports manually for multi-device.

app:exec already knows the resolved port via percy.address(). The detector is conservative — basename match on argv[0], test subcommand, and a scan for any pre-existing -e PERCY_SERVER=… (customer override wins per R3). npx maestro and other shim invocations fall through to the explicit--e pattern.

Stacks on #2248

Base branch: feat/self-hosted-maestro-percy. Rebase target after #2248 merges: master. Both units are functionally independent of #2248's content but ship in the same release window as the customer-facing completion of self-hosted Maestro support.

Why this is safe on BrowserStack

  • BS doesn't wrap maestro test with app:exec. maestro_runner.rb runs maestro directly while the CLI runs in app exec:start mode separately, so cli-app's wrapped callback is unreachable on BS.
  • Even if it ever did fire on BS, Maestro's documented precedence is runFlow.env > -e > environment, so BS's runFlow.env setting would still win.
  • The PERCY_CLI_API export is unconditional but harmless: BS doesn't run the Appium Python SDK against an app:exec-spawned child either.

Env-var conflict audit

Both names verified against the cli monorepo + every other Percy repo on disk:

  • PERCY_CLI_API — exactly one existing reader (percy-appium-python/percy/lib/cli_wrapper.py:10), zero existing writers. No CLI internal use. Our write semantically matches the reader's existing default. cli-doctor's env-audit.js filters with k.startsWith('PERCY_') so the new var auto-surfaces; only the test fixture (CLEAN_PERCY_ENV) needs a one-line addition for hermeticity.
  • PERCY_SERVER — read only inside Maestro's GraalJS, never by the CLI process itself. Four writer layers after this PR (runFlow.env > customer -e > our auto-inject > SDK default http://percy.cli:5338) layer cleanly with no overlap. R3 detector skip prevents double-injection.

Testing

  • cli-app: 16/16 specs pass, including 14 new scenarios for maybeInjectMaestroServer (happy path, customer override at multiple positions, basename match on absolute paths, npx/python/appium skip, maestro hierarchy/list-devices skip, percy-disabled skip, multi-device port isolation).
  • cli-exec: new PERCY_CLI_API env-export test mirrors the existing PERCY_SERVER_ADDRESS and PERCY_BUILD_URL test patterns. Asserts the child sees PERCY_CLI_API === PERCY_SERVER_ADDRESS === \"http://localhost:4567\" when --port=4567.
  • cli-doctor: CLEAN_PERCY_ENV fixture updated; 507/508 specs pass (the single failure is a pre-existing proxy-credentials network test in connectivity.test.js, unrelated).
  • Lint: clean across all touched files.

Post-Deploy Monitoring & Validation

  • What to monitor
    • CLI relay logs (/percy/maestro-screenshot POSTs in self-hosted runs) — should land without sessionId in the request body for self-hosted, and continue to land WITH sessionId for BS App Automate.
    • Maestro flow logs — [percy] Percy CLI healthcheck passed. line should now appear without the customer setting PERCY_SERVER.
  • Validation checks
    • Re-run the validation runbook at percy-maestro/docs/solutions/best-practices/2026-05-27-self-hosted-maestro-validation.md with the explicit -e PERCY_SERVER=… line removed from the maestro test command. Build ⬆️ Bump @oclif/config from 1.16.0 to 1.17.0 #14-equivalent (9×9=81) should still produce 3 Unchanged snapshots against an approved baseline.
    • Smoke test a BS App Automate Maestro build on the same bs-nixpkgs cli pin; confirm snapshots produce identically to pre-deploy and that sessionId=… is present in every CLI relay log line.
  • Expected healthy behavior
    • Self-hosted: customer can run percy app:exec -- maestro test flow.yaml with no -e PERCY_SERVER, no manual env aliasing; SDK healthcheck passes; snapshots upload.
    • BrowserStack: byte-identical to pre-deploy — auto-inject never fires there.
  • Failure signal / rollback trigger
    • Any BS App Automate Maestro build that previously passed reports Snapshot command was not called post-deploy → revert. (Should not be possible given the unreachable-on-BS analysis, but watch the first BS run.)
    • Self-hosted multi-device runs where two app:exec --port invocations show the same injected PERCY_SERVER= in their maestro logs → investigate multi-device isolation.
  • Validation window & owner

🤖 Generated with Claude Opus 4.7 via Claude Code + Compound Engineering v2.54.0

Sriram567 added 2 commits May 29, 2026 13:28
Aligns the env contract `percy exec` / `percy app:exec` propagate to
child processes with what `percy-appium-python` reads. The Python SDK
reads `PERCY_CLI_API`; today it only works by coincidence because both
`app:exec` and the SDK default to port 5338. With `--port` overrides
(or any future port shift), the SDK silently points at the wrong CLI
unless the customer exports `PERCY_CLI_API` themselves.

Setting it next to the existing `PERCY_SERVER_ADDRESS` removes that
footgun without changing any other behavior. Matches the unconditional-
override semantics already in place for `PERCY_SERVER_ADDRESS`.

Also adds `PERCY_CLI_API` to cli-doctor's CLEAN_PERCY_ENV test fixture
so env-audit tests stay hermetic against shells that have the var set.
The runtime env-audit code filters on `k.startsWith('PERCY_')` and
needs no change.
Maestro's GraalJS sandbox does not inherit the parent process's env,
so `PERCY_SERVER_ADDRESS` exported by app:exec is invisible to the
SDK self-hosted. Every customer hits this: their `maestro test` flow
falls through to the BS-safe `http://percy.cli:5338` default, the
healthcheck fails, and the build finalizes with "Snapshot command
was not called". The workaround today is for the customer to thread
`-e PERCY_SERVER=http://localhost:<port>` through every Maestro
invocation themselves, pairing ports manually when running multi-
device.

`app:exec` already knows the resolved port via `percy.address()`.
Detecting `maestro test` in argv and prepending one `-e` pair removes
the footgun without changing any other behavior. The detector is
conservative: basename match on argv[0], `test` subcommand, and a
scan for any pre-existing `-e PERCY_SERVER=...` (customer override
wins). `npx maestro` and other shim invocations correctly fall
through to the explicit-`-e` pattern.

BrowserStack path is unaffected — BS doesn't wrap maestro with
`app:exec`, so this code path is unreachable on BS.
@Sriram567 Sriram567 requested a review from a team as a code owner May 29, 2026 08:38
@Sriram567 Sriram567 requested review from aryanku-dev and prklm10 and removed request for a team May 29, 2026 08:38
@Sriram567
Copy link
Copy Markdown
Contributor Author

Consolidated into #2261 (single self-hosted Maestro+Percy V1 PR — all 6 commits, same content, same UX outcomes). Closing this stacked PR in favor of the unified review surface.

@Sriram567 Sriram567 closed this Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant