From 8053638e30e1a424e7168b2cf08ff861612497a5 Mon Sep 17 00:00:00 2001 From: Arumulla Sri Ram Date: Tue, 2 Jun 2026 17:07:06 +0530 Subject: [PATCH] feat(core): explicit runtime field gates /percy/maestro-screenshot self-hosted detection (R2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Promotes the self-hosted-vs-BrowserStack discriminator in the relay's /percy/maestro-screenshot handler from an implicit signal (sessionId absence) to an explicit one (runtime: "selfhosted" | "browserstack" in the SDK payload). The sessionId-absent fallback stays for backward compatibility with SDKs that predate the runtime field. Why: cli#2261's `let selfHosted = !sessionId;` derives the runtime from a field's absence. If BS ever omits sessionId in a future code path (retry, new session type), the self-hosted branch activates by accident → wrong file-find scope → 404. Moving the declaration to the SDK (where the knowledge originates — the SDK knows whether PERCY_SESSION_ID was injected) eliminates that future-proofing risk. Wire contract (additive, fully back-compat): selfHosted = (runtime === "selfhosted") || (!runtime && !sessionId) Unknown runtime values are NOT rejected — the relay falls back to the sessionId check, so SDKs experimenting with future values ("maestro-cloud", "saucelabs", etc.) don't break. Two `percy.log.debug` lines surface bidirectional inconsistencies (runtime + sessionId disagree) for diagnostic surface, never failing the request. R7 (BS regression) preserved — when SDK emits runtime: "browserstack" + sessionId, the BS branch runs byte-identically to today. Tests: - 9 new scenarios for the runtime field gating (8-way matrix of runtime × sessionId presence, plus the type-validation 400). - Existing /percy/maestro-screenshot tests unchanged. Stacks on cli#2261; companion SDK PR (percy-maestro-app) ships the runtime field in a future @percy/maestro-app release. Relay works today with older SDKs via the back-compat fallback. Origin: percy-maestro/docs/brainstorms/2026-06-02-selfhosted-followup-bundle-requirements.md (R2) Plan: percy-maestro/docs/plans/2026-06-02-001-feat-explicit-runtime-field-plan.md (Unit 2) --- packages/core/src/api.js | 31 +++++++++-- packages/core/test/api.test.js | 97 ++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 5 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 21652835b..8fae6d516 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -422,13 +422,34 @@ export function createPercyServer(percy, port) { // post a comparison by reading a Maestro screenshot from disk .route('post', '/percy/maestro-screenshot', async (req, res) => { /* istanbul ignore next — req.body falsy guard; tests always pass a body. */ - let { name, sessionId } = req.body || {}; + let { name, sessionId, runtime } = req.body || {}; if (!name) throw new ServerError(400, 'Missing required field: name'); - // `sessionId` is host-injected on BrowserStack; its absence signals - // self-hosted mode (gated separately on PERCY_MAESTRO_SCREENSHOT_DIR - // below). When present, the BS path runs byte-identical. - let selfHosted = !sessionId; + + // Validate `runtime` if present: must be a string, but unknown values + // are NOT rejected — we treat anything other than "selfhosted" / + // "browserstack" as "fall back to sessionId-absent detection". This + // is defensive against future runtime values (maestro-cloud, + // saucelabs, etc.) the SDK might introduce ahead of relay support. + if (runtime !== undefined && typeof runtime !== 'string') { + throw new ServerError(400, 'Invalid runtime: must be a string'); + } + + // Self-hosted detection — prefer the SDK's explicit declaration when + // present, fall back to sessionId absence (the implicit signal used + // by SDKs that predate the runtime field). The fallback is what + // ships in cli#2261 today; the explicit runtime field arrives with + // @percy/maestro-app >= v1.0.0-Beta.1 (cf. percy-maestro-app#7+R2). + let selfHosted = (runtime === 'selfhosted') || (!runtime && !sessionId); + + // Diagnostic surface for any inconsistency between the two signals + // (debug-only — never breaks the request). + if (runtime === 'browserstack' && !sessionId) { + percy.log.debug('maestro-screenshot: runtime=browserstack but sessionId missing — trusting runtime field'); + } + if (runtime === 'selfhosted' && sessionId) { + percy.log.debug('maestro-screenshot: runtime=selfhosted but sessionId present — trusting runtime field'); + } // Strict character-class validation — rejects path separators, shell metacharacters, // NUL, newlines, and anything else that could confuse the glob or the filesystem. diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index e432f9d2c..9dadae5ad 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -1255,6 +1255,103 @@ describe('API Server', () => { .toBeRejectedWithError(/Invalid platform: must be a string/); }); + describe('runtime field gating', () => { + // The relay's self-hosted detection prefers an explicit `runtime` field + // from the SDK when present, falling back to sessionId-absence for older + // SDKs. These tests cover the eight-way matrix of (runtime ∈ + // {undefined, "selfhosted", "browserstack", "unknown"}) × (sessionId ∈ + // {present, absent}) — plus the type-validation 400. + + it('rejects non-string runtime type with 400', async () => { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, runtime: 123 })) + .toBeRejectedWithError(/Invalid runtime: must be a string/); + }); + + it('runtime=selfhosted + no sessionId → self-hosted branch (400s on missing env)', async () => { + let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + try { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME, runtime: 'selfhosted' })) + .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); + } finally { + if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; + } + }); + + it('runtime=selfhosted + sessionId PRESENT → still self-hosted branch (trust runtime field)', async () => { + // Bidirectional-consistency case: SDK explicitly says self-hosted but + // a sessionId leaked into the payload. Trust the SDK's declaration. + let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + try { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'selfhosted' })) + .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); + } finally { + if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; + } + }); + + it('runtime=browserstack + sessionId present → BS branch finds file under /tmp//...', async () => { + await percy.start(); + const res = await postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'browserstack' }); + expect(res.status).toBe(200); + }); + + it('runtime=browserstack + sessionId ABSENT → BS branch (trust runtime), but file-find fails → 404', async () => { + // Bidirectional-consistency case: SDK explicitly says browserstack + // but sessionId is missing. BS branch runs, can't locate the file + // because `/tmp//...` doesn't resolve. + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME, runtime: 'browserstack' })) + .toBeRejectedWithError(/Screenshot not found|sessionId/i); + }); + + it('runtime=unknown-value + no sessionId → falls back to sessionId-absent → self-hosted branch', async () => { + // Defensive: unknown runtime values are NOT rejected. The relay falls + // back to sessionId-absence detection, so this lands in self-hosted. + let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + try { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME, runtime: 'maestro-cloud' })) + .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); + } finally { + if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; + } + }); + + it('runtime=unknown-value + sessionId present → falls back to sessionId-present → BS branch', async () => { + await percy.start(); + const res = await postMaestro({ name: SS_NAME, sessionId: SID, runtime: 'maestro-cloud' }); + expect(res.status).toBe(200); + }); + + it('no runtime + sessionId present → BS branch (backward-compat with pre-runtime SDKs)', async () => { + await percy.start(); + const res = await postMaestro({ name: SS_NAME, sessionId: SID }); + expect(res.status).toBe(200); + }); + + it('no runtime + no sessionId → self-hosted branch (backward-compat with pre-runtime SDKs)', async () => { + let prior = process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + try { + await percy.start(); + await expectAsync(postMaestro({ name: SS_NAME })) + .toBeRejectedWithError(/Missing required env: PERCY_MAESTRO_SCREENSHOT_DIR/); + } finally { + if (prior === undefined) delete process.env.PERCY_MAESTRO_SCREENSHOT_DIR; + else process.env.PERCY_MAESTRO_SCREENSHOT_DIR = prior; + } + }); + }); + it('rejects non-object element selector with 400', async () => { await percy.start(); await expectAsync(postMaestro({