From 6dd32616e710ebd3fff325c01ce52102586f2550 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Fri, 29 May 2026 13:28:36 +0530 Subject: [PATCH 1/2] feat(cli-exec): export PERCY_CLI_API alongside PERCY_SERVER_ADDRESS 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. --- packages/cli-doctor/test/env-audit.test.js | 1 + packages/cli-exec/src/exec.js | 1 + packages/cli-exec/test/exec.test.js | 17 +++++++++++++++++ 3 files changed, 19 insertions(+) diff --git a/packages/cli-doctor/test/env-audit.test.js b/packages/cli-doctor/test/env-audit.test.js index 73d84776e..cf661fd3d 100644 --- a/packages/cli-doctor/test/env-audit.test.js +++ b/packages/cli-doctor/test/env-audit.test.js @@ -62,6 +62,7 @@ const CLEAN_PERCY_ENV = { PERCY_CHROMIUM_BASE_URL: undefined, PERCY_PAC_FILE_URL: undefined, PERCY_CLIENT_API_URL: undefined, + PERCY_CLI_API: undefined, PERCY_SERVER_ADDRESS: undefined, PERCY_SERVER_HOST: undefined, PERCY_COMMIT: undefined, diff --git a/packages/cli-exec/src/exec.js b/packages/cli-exec/src/exec.js index be4850e86..81bfa89de 100644 --- a/packages/cli-exec/src/exec.js +++ b/packages/cli-exec/src/exec.js @@ -86,6 +86,7 @@ export const exec = command('exec', { // provide SDKs with useful env vars env.PERCY_SERVER_ADDRESS = percy?.address(); + env.PERCY_CLI_API = percy?.address(); env.PERCY_BUILD_ID = percy?.build?.id; env.PERCY_BUILD_URL = percy?.build?.url; env.PERCY_LOGLEVEL = log.loglevel(); diff --git a/packages/cli-exec/test/exec.test.js b/packages/cli-exec/test/exec.test.js index 792fa72c7..592b561d1 100644 --- a/packages/cli-exec/test/exec.test.js +++ b/packages/cli-exec/test/exec.test.js @@ -377,4 +377,21 @@ describe('percy exec', () => { '[percy] Finalized build #1: https://percy.io/test/test/123' ])); }); + + it('provides the child process with a PERCY_CLI_API env var matching the server address', async () => { + await exec(['--port=4567', '--', 'node', '--eval', ( + 'process.env.PERCY_CLI_API === process.env.PERCY_SERVER_ADDRESS ' + + '&& process.env.PERCY_CLI_API === "http://localhost:4567" || process.exit(2)' + )]); + + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy] Detected error for percy build', + '[percy] Failure: Snapshot command was not called' + ])); + expect(logger.stdout).toEqual(jasmine.arrayContaining([ + '[percy] Percy has started!', + jasmine.stringMatching('\\[percy] Running "node '), + '[percy] Finalized build #1: https://percy.io/test/test/123' + ])); + }); }); From a789910e8b7906a943ba1991e099db3c07c8f21d Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Fri, 29 May 2026 14:02:44 +0530 Subject: [PATCH 2/2] feat(cli-app): auto-inject -e PERCY_SERVER for `maestro test` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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:` 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. --- packages/cli-app/src/exec.js | 31 ++++++- packages/cli-app/src/index.js | 2 +- packages/cli-app/test/exec.test.js | 142 ++++++++++++++++++++++++++++- 3 files changed, 170 insertions(+), 5 deletions(-) diff --git a/packages/cli-app/src/exec.js b/packages/cli-app/src/exec.js index ccec76328..9caa8672b 100644 --- a/packages/cli-app/src/exec.js +++ b/packages/cli-app/src/exec.js @@ -1,3 +1,4 @@ +import path from 'path'; import command from '@percy/cli-command'; import * as ExecPlugin from '@percy/cli-exec'; @@ -15,6 +16,31 @@ export const start = command('start', { } }, ExecPlugin.start.callback); +function hasExistingPercyServerFlag(args) { + for (let i = 2; i < args.length - 1; i++) { + if (args[i] === '-e' && /^PERCY_SERVER=/.test(args[i + 1])) return true; + } + return false; +} + +// 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. When wrapping `maestro test`, surface the CLI address through +// Maestro's only env channel — `-e KEY=VALUE` flags — so the SDK +// healthcheck can find the local CLI without the customer having to +// pair ports manually. No-op when the customer already supplied their +// own `-e PERCY_SERVER=...`. +export function maybeInjectMaestroServer(ctx) { + const args = ctx?.argv; + if (!Array.isArray(args) || args.length < 2) return; + if (path.basename(args[0]) !== 'maestro') return; + if (args[1] !== 'test') return; + if (hasExistingPercyServerFlag(args)) return; + const addr = ctx.percy?.address(); + if (!addr) return; + args.splice(2, 0, '-e', `PERCY_SERVER=${addr}`); +} + export const exec = command('exec', { description: 'Start and stop Percy around a supplied command for native apps', usage: '[options] -- ', @@ -29,6 +55,9 @@ export const exec = command('exec', { projectType: 'app', skipDiscovery: true } -}, ExecPlugin.default.callback); +}, async function*(ctx) { + maybeInjectMaestroServer(ctx); + yield* ExecPlugin.default.callback(ctx); +}); export default exec; diff --git a/packages/cli-app/src/index.js b/packages/cli-app/src/index.js index 49012033f..4f95c7b7d 100644 --- a/packages/cli-app/src/index.js +++ b/packages/cli-app/src/index.js @@ -1,2 +1,2 @@ export { default, app } from './app.js'; -export { exec, start, stop, ping } from './exec.js'; +export { exec, start, stop, ping, maybeInjectMaestroServer } from './exec.js'; diff --git a/packages/cli-app/test/exec.test.js b/packages/cli-app/test/exec.test.js index 100c1fd5e..7b859c8c4 100644 --- a/packages/cli-app/test/exec.test.js +++ b/packages/cli-app/test/exec.test.js @@ -1,14 +1,18 @@ import { setupTest } from '@percy/cli-command/test/helpers'; import * as ExecPlugin from '@percy/cli-exec'; -import { exec, start, stop, ping } from '@percy/cli-app'; +import { exec, start, stop, ping, maybeInjectMaestroServer } from '@percy/cli-app'; describe('percy app:exec', () => { beforeEach(async () => { await setupTest(); }); - it('has shared exec commands with differing definitions', async () => { - expect(exec.callback).toEqual(ExecPlugin.default.callback); + it('wraps cli-exec callbacks while preserving differing definitions', async () => { + // exec.callback wraps cli-exec's callback (auto-injects -e PERCY_SERVER + // for `maestro test`), so it is no longer reference-equal — but start, + // stop, and ping remain straight delegations. + expect(typeof exec.callback).toBe('function'); + expect(exec.callback).not.toEqual(ExecPlugin.default.callback); expect(exec.definition).not.toEqual(ExecPlugin.default.definition); expect(start.callback).toEqual(ExecPlugin.start.callback); expect(start.definition).not.toEqual(ExecPlugin.start.definition); @@ -23,4 +27,136 @@ describe('percy app:exec', () => { await expectAsync(start(['--network-idle-timeout', '500'])) .toBeRejectedWithError("Unknown option '--network-idle-timeout'"); }); + + describe('maybeInjectMaestroServer', () => { + function ctxFor(argv, addr = 'http://localhost:5338') { + return { + argv: [...argv], + percy: { address: () => addr } + }; + } + + it('injects -e PERCY_SERVER at index 2 for `maestro test`', () => { + const ctx = ctxFor(['maestro', 'test', 'flow.yaml']); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '-e', 'PERCY_SERVER=http://localhost:5338', 'flow.yaml' + ]); + }); + + it('preserves other -e flags and customer args after injection', () => { + const ctx = ctxFor([ + 'maestro', 'test', '--test-output-dir', 'out', + '-e', 'PERCY_DEVICE_NAME=Pixel 10', 'flow.yaml' + ]); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + 'maestro', 'test', + '-e', 'PERCY_SERVER=http://localhost:5338', + '--test-output-dir', 'out', + '-e', 'PERCY_DEVICE_NAME=Pixel 10', + 'flow.yaml' + ]); + }); + + it('skips when customer already supplied -e PERCY_SERVER (adjacent to test)', () => { + const argv = ['maestro', 'test', '-e', 'PERCY_SERVER=http://custom:9999', 'flow.yaml']; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when customer supplied -e PERCY_SERVER deeper in args (R3 scan)', () => { + const argv = [ + 'maestro', 'test', '--test-output-dir', 'out', + '-e', 'PERCY_SERVER=http://custom:9999', 'flow.yaml' + ]; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for `maestro hierarchy` (not a test command)', () => { + const argv = ['maestro', 'hierarchy', '--udid', 'X']; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for `maestro list-devices` (not a test command)', () => { + const argv = ['maestro', 'list-devices']; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when args has fewer than two elements', () => { + const argv = ['maestro']; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('matches by basename when the command is an absolute path', () => { + const ctx = ctxFor(['/Users/foo/.maestro/bin/maestro', 'test', 'flow.yaml']); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + '/Users/foo/.maestro/bin/maestro', 'test', + '-e', 'PERCY_SERVER=http://localhost:5338', + 'flow.yaml' + ]); + }); + + it('skips for `npx maestro test` (argv[0] is npx, not maestro)', () => { + const argv = ['npx', 'maestro', 'test', 'flow.yaml']; + const ctx = ctxFor(argv); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips for non-maestro commands (python, appium, etc.)', () => { + const pyArgv = ['python', 'test.py']; + const pyCtx = ctxFor(pyArgv); + maybeInjectMaestroServer(pyCtx); + expect(pyCtx.argv).toEqual(pyArgv); + + const apiumArgv = ['appium', '--port', '4723']; + const apiumCtx = ctxFor(apiumArgv); + maybeInjectMaestroServer(apiumCtx); + expect(apiumCtx.argv).toEqual(apiumArgv); + }); + + it('flows --port through into the injected address (multi-device)', () => { + const ctx = ctxFor(['maestro', 'test', 'flow.yaml'], 'http://localhost:5339'); + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual([ + 'maestro', 'test', '-e', 'PERCY_SERVER=http://localhost:5339', 'flow.yaml' + ]); + }); + + it('skips when percy is disabled (no address)', () => { + const argv = ['maestro', 'test', 'flow.yaml']; + const ctx = { argv: [...argv], percy: { address: () => undefined } }; + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('skips when ctx has no percy at all', () => { + const argv = ['maestro', 'test', 'flow.yaml']; + const ctx = { argv: [...argv] }; + maybeInjectMaestroServer(ctx); + expect(ctx.argv).toEqual(argv); + }); + + it('two concurrent contexts pick their own addresses (multi-device isolation)', () => { + const a = ctxFor(['maestro', 'test', 'flow.yaml'], 'http://localhost:5338'); + const b = ctxFor(['maestro', 'test', 'flow.yaml'], 'http://localhost:5339'); + maybeInjectMaestroServer(a); + maybeInjectMaestroServer(b); + expect(a.argv).toContain('PERCY_SERVER=http://localhost:5338'); + expect(b.argv).toContain('PERCY_SERVER=http://localhost:5339'); + expect(a.argv).not.toContain('PERCY_SERVER=http://localhost:5339'); + expect(b.argv).not.toContain('PERCY_SERVER=http://localhost:5338'); + }); + }); });