feat(cli-app): auto-resolve screenshot dir + WARN on no-addr injection (R1+R3)

packages/cli-app/src/exec.js

@@ -1,3 +1,5 @@
+import fs from 'fs';
+import os from 'os';
 import path from 'path';
 import command from '@percy/cli-command';
 import * as ExecPlugin from '@percy/cli-exec';
@@ -23,24 +25,108 @@ function hasExistingPercyServerFlag(args) {
   return false;
 }
 
+// Returns the index of the value following `--test-output-dir`, or -1 if absent.
+// We return the value-index (not just a boolean) so the screenshot-dir helper
+// can align PERCY_MAESTRO_SCREENSHOT_DIR with a customer-supplied flag value.
+function findTestOutputDirValueIdx(args) {
+  for (let i = 2; i < args.length - 1; i++) {
+    if (args[i] === '--test-output-dir') return i + 1;
+  }
+  return -1;
+}
+
 // 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) {
+//
+// When percy?.address() is falsy (percy disabled, start failed), emit a
+// WARN so the customer is not surprised by a silent zero-snapshot build.
+// The customer-override skip case (their own `-e PERCY_SERVER=...` is in
+// argv) does NOT warn — that's intentional flow control, not a problem.
+export function maybeInjectMaestroServer(ctx, log) {
   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;
+  if (!addr) {
+    log?.warn(
+      'app:exec did not start the Percy CLI server (percy disabled or start ' +
+      'failed); -e PERCY_SERVER not injected into maestro test. Snapshots will ' +
+      'NOT be uploaded. Set PERCY_TOKEN and re-run, or check the percy log above.'
+    );
+    return;
+  }
   args.splice(2, 0, '-e', `PERCY_SERVER=${addr}`);
 }
 
+// Auto-resolve the Maestro screenshot output directory so customers don't
+// have to pair `export PERCY_MAESTRO_SCREENSHOT_DIR=...` with a matching
+// `--test-output-dir <same>` in their maestro test command.
+//
+// Resolution order:
+//   1. Customer set BOTH process.env.PERCY_MAESTRO_SCREENSHOT_DIR and
+//      --test-output-dir in argv → trust them, do nothing.
+//   2. Customer set PERCY_MAESTRO_SCREENSHOT_DIR only → use it, inject
+//      `--test-output-dir <env value>` into argv.
+//   3. Customer set --test-output-dir only → use that value, mirror it
+//      into process.env.PERCY_MAESTRO_SCREENSHOT_DIR (so the SDK +
+//      CLI relay see the same path).
+//   4. Neither set → pick `${process.cwd()}/.percy-out`. On any mkdir
+//      failure (read-only CWD, EACCES, EEXIST as a file), fall back to
+//      `${os.tmpdir()}/percy-maestro-<pid>` with a WARN log.
+//
+// The env-var update and argv splice always keep both sources of truth
+// (SDK reads env var; Maestro reads the flag) aligned to the same path.
+export function maybeInjectScreenshotDir(ctx, log) {
+  const args = ctx?.argv;
+  if (!Array.isArray(args) || args.length < 2) return;
+  if (path.basename(args[0]) !== 'maestro') return;
+  if (args[1] !== 'test') return;
+
+  const envSet = !!process.env.PERCY_MAESTRO_SCREENSHOT_DIR;
+  const flagValueIdx = findTestOutputDirValueIdx(args);
+  const flagSet = flagValueIdx > 0;
+
+  // Fully customer-controlled — nothing to do.
+  if (envSet && flagSet) return;
+
+  let resolved;
+  if (envSet) {
+    resolved = process.env.PERCY_MAESTRO_SCREENSHOT_DIR;
+  } else if (flagSet) {
+    resolved = args[flagValueIdx];
+  } else {
+    const preferred = path.join(process.cwd(), '.percy-out');
+    try {
+      fs.mkdirSync(preferred, { recursive: true });
+      resolved = preferred;
+    } catch (err) {
+      const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`);
+      try {
+        fs.mkdirSync(fallback, { recursive: true });
+      } catch (_) {
+        // tmpdir mkdir failure is exceedingly rare; fall through and let
+        // downstream code surface a clearer error than this helper can.
+      }
+      resolved = fallback;
+      log?.warn(
+        `Could not create ${preferred} (${err.code || err.message}); ` +
+        `falling back to ${fallback}. Set PERCY_MAESTRO_SCREENSHOT_DIR to ` +
+        'pick a specific location.'
+      );
+    }
+  }
+
+  if (!envSet) process.env.PERCY_MAESTRO_SCREENSHOT_DIR = resolved;
+  if (!flagSet) args.splice(2, 0, '--test-output-dir', resolved);
+}
+
 export const exec = command('exec', {
   description: 'Start and stop Percy around a supplied command for native apps',
   usage: '[options] -- <command>',
@@ -56,7 +142,15 @@ export const exec = command('exec', {
     skipDiscovery: true
   }
 }, async function*(ctx) {
-  maybeInjectMaestroServer(ctx);
+  // Order matters: maestro-server splice goes in at index 2 first, then
+  // screenshot-dir splice goes in at index 2 too (pushing the server flags
+  // to index 4). Resulting argv for `maestro test flow.yaml` becomes:
+  //   maestro test --test-output-dir <dir> -e PERCY_SERVER=<url> flow.yaml
+  // Both flag groups land between `test` and the flow file, which Maestro
+  // accepts. Reversing the order would still work; this order is chosen
+  // for log readability.
+  maybeInjectMaestroServer(ctx, ctx.log);
+  maybeInjectScreenshotDir(ctx, ctx.log);
   yield* ExecPlugin.default.callback(ctx);
 });
 

packages/cli-app/src/index.js

@@ -1,2 +1,2 @@
 export { default, app } from './app.js';
-export { exec, start, stop, ping, maybeInjectMaestroServer } from './exec.js';
+export { exec, start, stop, ping, maybeInjectMaestroServer, maybeInjectScreenshotDir } from './exec.js';

packages/cli-app/test/exec.test.js

@@ -1,6 +1,12 @@
+import fs from 'fs';
+import os from 'os';
+import path from 'path';
 import { setupTest } from '@percy/cli-command/test/helpers';
 import * as ExecPlugin from '@percy/cli-exec';
-import { exec, start, stop, ping, maybeInjectMaestroServer } from '@percy/cli-app';
+import {
+  exec, start, stop, ping,
+  maybeInjectMaestroServer, maybeInjectScreenshotDir
+} from '@percy/cli-app';
 
 describe('percy app:exec', () => {
   beforeEach(async () => {
@@ -9,8 +15,8 @@ describe('percy app:exec', () => {
 
   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.
+    // and --test-output-dir 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);
@@ -158,5 +164,190 @@ describe('percy app:exec', () => {
       expect(a.argv).not.toContain('PERCY_SERVER=http://localhost:5339');
       expect(b.argv).not.toContain('PERCY_SERVER=http://localhost:5338');
     });
+
+    it('emits WARN log when percy address is falsy and there is no customer override', () => {
+      const log = { warn: jasmine.createSpy('warn') };
+      const ctx = { argv: ['maestro', 'test', 'flow.yaml'], percy: { address: () => undefined } };
+      maybeInjectMaestroServer(ctx, log);
+      expect(log.warn).toHaveBeenCalledTimes(1);
+      expect(log.warn.calls.argsFor(0)[0]).toContain('-e PERCY_SERVER not injected');
+    });
+
+    it('does NOT emit WARN when customer-supplied -e PERCY_SERVER override is present', () => {
+      const log = { warn: jasmine.createSpy('warn') };
+      const ctx = {
+        argv: ['maestro', 'test', '-e', 'PERCY_SERVER=http://custom:9999', 'flow.yaml'],
+        percy: { address: () => undefined }
+      };
+      maybeInjectMaestroServer(ctx, log);
+      expect(log.warn).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('maybeInjectScreenshotDir', () => {
+    let originalEnvValue;
+
+    beforeEach(() => {
+      originalEnvValue = process.env.PERCY_MAESTRO_SCREENSHOT_DIR;
+      delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR;
+    });
+
+    afterEach(() => {
+      if (originalEnvValue === undefined) {
+        delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR;
+      } else {
+        process.env.PERCY_MAESTRO_SCREENSHOT_DIR = originalEnvValue;
+      }
+    });
+
+    function ctxFor(argv) {
+      return { argv: [...argv] };
+    }
+
+    it('injects --test-output-dir and sets env var to <CWD>/.percy-out on happy path', () => {
+      const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {});
+      const expectedDir = path.join(process.cwd(), '.percy-out');
+      const ctx = ctxFor(['maestro', 'test', 'flow.yaml']);
+      maybeInjectScreenshotDir(ctx);
+      expect(mkdir).toHaveBeenCalledWith(expectedDir, { recursive: true });
+      expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(expectedDir);
+      expect(ctx.argv).toEqual([
+        'maestro', 'test', '--test-output-dir', expectedDir, 'flow.yaml'
+      ]);
+    });
+
+    it('honors customer-set PERCY_MAESTRO_SCREENSHOT_DIR and injects flag aligned to it', () => {
+      process.env.PERCY_MAESTRO_SCREENSHOT_DIR = '/custom/screenshot/dir';
+      const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {});
+      const ctx = ctxFor(['maestro', 'test', 'flow.yaml']);
+      maybeInjectScreenshotDir(ctx);
+      expect(mkdir).not.toHaveBeenCalled();
+      expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe('/custom/screenshot/dir');
+      expect(ctx.argv).toEqual([
+        'maestro', 'test', '--test-output-dir', '/custom/screenshot/dir', 'flow.yaml'
+      ]);
+    });
+
+    it('honors customer-supplied --test-output-dir and mirrors value into env var', () => {
+      const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {});
+      const ctx = ctxFor(['maestro', 'test', '--test-output-dir', '/custom/path', 'flow.yaml']);
+      maybeInjectScreenshotDir(ctx);
+      expect(mkdir).not.toHaveBeenCalled();
+      expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe('/custom/path');
+      // argv unchanged — customer's flag stays where they put it
+      expect(ctx.argv).toEqual(['maestro', 'test', '--test-output-dir', '/custom/path', 'flow.yaml']);
+    });
+
+    it('skips entirely when both env var and --test-output-dir are customer-set', () => {
+      process.env.PERCY_MAESTRO_SCREENSHOT_DIR = '/from/env';
+      const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {});
+      const argv = ['maestro', 'test', '--test-output-dir', '/from/flag', 'flow.yaml'];
+      const ctx = ctxFor(argv);
+      maybeInjectScreenshotDir(ctx);
+      expect(mkdir).not.toHaveBeenCalled();
+      expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe('/from/env');
+      expect(ctx.argv).toEqual(argv);
+    });
+
+    it('falls back to <TMPDIR>/percy-maestro-<pid> on EACCES and emits WARN', () => {
+      const mkdir = spyOn(fs, 'mkdirSync').and.callFake((dirPath) => {
+        if (dirPath === path.join(process.cwd(), '.percy-out')) {
+          const err = new Error('EACCES'); err.code = 'EACCES';
+          throw err;
+        }
+      });
+      const log = { warn: jasmine.createSpy('warn') };
+      const ctx = ctxFor(['maestro', 'test', 'flow.yaml']);
+      maybeInjectScreenshotDir(ctx, log);
+      const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`);
+      expect(mkdir).toHaveBeenCalledWith(fallback, { recursive: true });
+      expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(fallback);
+      expect(ctx.argv).toEqual([
+        'maestro', 'test', '--test-output-dir', fallback, 'flow.yaml'
+      ]);
+      expect(log.warn).toHaveBeenCalledTimes(1);
+      expect(log.warn.calls.argsFor(0)[0]).toContain('EACCES');
+      expect(log.warn.calls.argsFor(0)[0]).toContain(fallback);
+    });
+
+    it('falls back on EROFS (read-only filesystem)', () => {
+      spyOn(fs, 'mkdirSync').and.callFake((dirPath) => {
+        if (dirPath === path.join(process.cwd(), '.percy-out')) {
+          const err = new Error('EROFS'); err.code = 'EROFS';
+          throw err;
+        }
+      });
+      const log = { warn: jasmine.createSpy('warn') };
+      const ctx = ctxFor(['maestro', 'test', 'flow.yaml']);
+      maybeInjectScreenshotDir(ctx, log);
+      const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`);
+      expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(fallback);
+      expect(log.warn).toHaveBeenCalled();
+    });
+
+    it('falls back on EEXIST (.percy-out collides with a non-directory)', () => {
+      spyOn(fs, 'mkdirSync').and.callFake((dirPath) => {
+        if (dirPath === path.join(process.cwd(), '.percy-out')) {
+          const err = new Error('EEXIST'); err.code = 'EEXIST';
+          throw err;
+        }
+      });
+      const log = { warn: jasmine.createSpy('warn') };
+      const ctx = ctxFor(['maestro', 'test', 'flow.yaml']);
+      maybeInjectScreenshotDir(ctx, log);
+      const fallback = path.join(os.tmpdir(), `percy-maestro-${process.pid}`);
+      expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBe(fallback);
+      expect(log.warn).toHaveBeenCalled();
+    });
+
+    it('skips for `maestro hierarchy` (not a test command)', () => {
+      const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {});
+      const argv = ['maestro', 'hierarchy', '--udid', 'X'];
+      const ctx = ctxFor(argv);
+      maybeInjectScreenshotDir(ctx);
+      expect(mkdir).not.toHaveBeenCalled();
+      expect(process.env.PERCY_MAESTRO_SCREENSHOT_DIR).toBeUndefined();
+      expect(ctx.argv).toEqual(argv);
+    });
+
+    it('skips for `npx maestro test` (argv[0] is npx, not maestro)', () => {
+      const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {});
+      const argv = ['npx', 'maestro', 'test', 'flow.yaml'];
+      const ctx = ctxFor(argv);
+      maybeInjectScreenshotDir(ctx);
+      expect(mkdir).not.toHaveBeenCalled();
+      expect(ctx.argv).toEqual(argv);
+    });
+
+    it('skips for non-maestro commands', () => {
+      const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {});
+      const argv = ['python', 'test.py'];
+      const ctx = ctxFor(argv);
+      maybeInjectScreenshotDir(ctx);
+      expect(mkdir).not.toHaveBeenCalled();
+      expect(ctx.argv).toEqual(argv);
+    });
+
+    it('skips when args has fewer than two elements', () => {
+      const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {});
+      const argv = ['maestro'];
+      const ctx = ctxFor(argv);
+      maybeInjectScreenshotDir(ctx);
+      expect(mkdir).not.toHaveBeenCalled();
+      expect(ctx.argv).toEqual(argv);
+    });
+
+    it('matches by basename when the command is an absolute path', () => {
+      const mkdir = spyOn(fs, 'mkdirSync').and.callFake(() => {});
+      const expectedDir = path.join(process.cwd(), '.percy-out');
+      const ctx = ctxFor(['/Users/foo/.maestro/bin/maestro', 'test', 'flow.yaml']);
+      maybeInjectScreenshotDir(ctx);
+      expect(mkdir).toHaveBeenCalledWith(expectedDir, { recursive: true });
+      expect(ctx.argv).toEqual([
+        '/Users/foo/.maestro/bin/maestro', 'test',
+        '--test-output-dir', expectedDir,
+        'flow.yaml'
+      ]);
+    });
   });
 });