feat(webdriver-utils): env-var overrides for staging/percy-dev testing

[bhokaremoin]

Summary

Replaces the manual node_modules/@percy/webdriver-utils edits devs currently make to run Percy against staging (percy-dev project) with environment variables that ship in the package and default to current production behavior.

Env var	Effect when set	Default (unset)
PERCY_ENABLE_DEV=true	inject projectId: 'percy-dev' into percyScreenshot payloads	no projectId field (prod, byte-identical to today)
PERCY_CDP_DOMAIN	CDP host for the Playwright driver execute URL	cdp.browserstack.com
PERCY_AUTOMATE_DOMAIN	host for Automate session debug URLs	automate.browserstack.com

Key decisions

• Single chokepoint, not four edits. All four manual projectId sites (percyScreenshotBegin/End, automate + playwright getTiles) converge on the shared genericProvider.browserstackExecutor(action, args) method. projectId is injected there once, gated on action === 'percyScreenshot' (excludes getSessionDetails etc.).
• Adopts the existing PERCY_ENABLE_DEV convention already shipped in the Python percy-appium-app SDK (app_automate.py), so one export works across JS + Python — rather than inventing a divergent new var.
• Self-gating / prod-safe. When PERCY_ENABLE_DEV is unset, JS sends no projectId field (server treats absent ≡ percy-prod), so customer payloads are unchanged. A customer can't accidentally misroute a prod build.
• Mirrors the existing AA_DOMAIN env-var precedent (automateProvider.js).

Usage

The per-environment export blocks / staging hostnames live in internal docs (not this public repo) to avoid exposing staging cluster hostnames. The shipped knobs here default to production.

Testing

• 6 new specs: projectId injected with the flag, absent without it, untouched for non-percyScreenshot actions, and PERCY_CDP_DOMAIN / PERCY_AUTOMATE_DOMAIN host overrides with prod fallbacks.
• Full webdriver-utils suite: 238/238 pass.
• yarn lint clean at repo root.

Follow-ups (separate repo)

• percy-appium-app: add the same PERCY_ENABLE_DEV projectId line to execute_percy_screenshot_begin/_end (currently only execute_percy_screenshot sets it) for full begin/end/screenshot parity.

Post-Deploy Monitoring & Validation

• What to monitor/search
  • Logs: nothing new in prod — overrides are gated behind env vars that are unset in production/CI.
  • Validate in staging by exporting the override vars and confirming the Percy SHA lands in the percy-dev project.
• Expected healthy behavior
  • With no new env vars set, browserstack_executor payloads and CDP/automate URLs are byte-identical to pre-change (covered by the "byte-identical to prod" unit test).
• Failure signal / rollback trigger
  • Any prod build unexpectedly showing projectId in its executor payload, or routing to a non-prod host ⇒ revert this commit.
• Validation window & owner
  • One staging run per flow (POA Automate, App, Playwright) by the author before wider rollout.

---

🤖 Generated with Claude Opus 4.8 (1M context) via Claude Code

[bhokaremoin]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[bhokaremoin]

[Low] Optional: allowlist override domains

PERCY_CDP_DOMAIN (and PERCY_AUTOMATE_DOMAIN in the providers) is interpolated verbatim into an outbound HTTPS URL. This is a developer-only, default-off override, and anyone able to set env vars in the run already holds a broader trust boundary — so this is not blocking. If you want cheap hardening, validate the value against an allowlist of trusted suffixes (.browserstack.com, .bsstag.com, .bs-local.com) and throw otherwise.

Suggestion: optional defense-in-depth; safe to ship as-is.

Reviewer: stack:code-reviewer

[bhokaremoin]

[Low] Optional: future-proof the spread

{ ...args, projectId: 'percy-dev' } assumes args is a plain object and always overrides projectId. Verified all current percyScreenshot call sites pass plain object literals and none set projectId, so this is correct today — purely a defensive note.

Suggestion (optional):

Suggested change

	args = { ...args, projectId: 'percy-dev' };
	args = (args && typeof args === 'object' && !Array.isArray(args)) ? { ...args, projectId: 'percy-dev' } : { projectId: 'percy-dev' };

Reviewer: stack:code-reviewer

[bhokaremoin]

Claude Code PR Review

PR: #2274 • Head: 364dded • Reviewers: stack:code-reviewer

Summary

Adds three developer-only, default-off environment-variable overrides to @percy/webdriver-utils so staging/percy-dev testing no longer requires hand-editing files in node_modules: PERCY_CDP_DOMAIN (Playwright CDP host), PERCY_AUTOMATE_DOMAIN (Automate debug-URL host), and PERCY_ENABLE_DEV=true (injects projectId: 'percy-dev' into percyScreenshot executor payloads). All three default to current production behavior when unset, are covered by new unit tests, and are documented in a new DEVELOPMENT.md.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets added; only env-var reads.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass*	Override domains interpolated into outbound URLs without allowlisting (finding [1]) — env-gated dev-only; env-var write access is already a trust boundary. Hardening note, not a blocker.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	Guards on action === 'percyScreenshot' and explicit === 'true'; defaults to prod. All real call sites pass plain object literals.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No error paths changed.
High	Correctness	No race conditions or concurrency issues	Pass	Cache-hit domain concern (finding [4]) is theoretical: env var is constant within a process run.
Medium	Testing	New code has corresponding tests	Pass	Each override has a dedicated test; includes a "byte-identical to prod when disabled" test.
Medium	Testing	Error paths and edge cases tested	Pass*	Missing undefined-args and cache-hit edge cases (findings [6], [7]) — minor.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Defaults preserved; existing assertions unchanged.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	—
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	`process.env.X
Medium	Quality	Changes are focused (single concern)	Pass	Single concern: staging env overrides.
Low	Quality	Meaningful names, no dead code	Pass	Clear names; explanatory comment on the dev override.
Low	Quality	Comments explain why, not what	Pass	Comment explains the prod-parity rationale and Python-SDK convention.
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

All findings below are Low severity for this PR's context (an internal, env-gated, default-off developer override). The original reviewer rated [1] Critical and [2]/[3] High; those severities are downgraded here after verifying the real call sites and threat model — documented so the rationale is transparent.

• File: packages/webdriver-utils/src/playwrightDriver.js:30, src/providers/automateProvider.js:109, src/providers/playwrightProvider.js:38

• Severity: Low (reviewer rated Critical)

• Reviewer: stack:code-reviewer

• Issue: PERCY_CDP_DOMAIN / PERCY_AUTOMATE_DOMAIN are interpolated verbatim into outbound HTTPS URLs without allowlisting (OWASP A10 / SSRF lens). PERCY_CDP_DOMAIN in particular fronts the session-execute endpoint that carries session context.

• Context: Developer-only override, default-off, only reachable by someone who can already set process env vars in the run — a trust boundary that, if breached, implies broader compromise. Not blocking for an internal staging tool.

• Suggestion: Optional hardening — validate override values against an allowlist of trusted suffixes (.browserstack.com, .bsstag.com, .bs-local.com) and throw otherwise.

• File: packages/webdriver-utils/src/providers/genericProvider.js:104

• Severity: Low (reviewer rated High)

• Reviewer: stack:code-reviewer

• Issue: args = { ...args, projectId: 'percy-dev' } would mishandle a non-plain-object args (primitive/array → corrupted payload), and unconditionally overrides any caller-supplied projectId.

• Context: Verified all three percyScreenshot call sites (automateProvider.js:68, genericProvider.js:114/:153, playwrightProvider.js:92) — every one passes a plain object literal and none sets projectId. Defensive-only today; overriding projectId is the intended behavior.

• Suggestion: Optional: guard the spread (args && typeof args === 'object' && !Array.isArray(args) ? {...args} : {}) to future-proof the signature.

• File: packages/webdriver-utils/src/providers/automateProvider.js:109

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: automateDomain is computed outside the Cache.withCache callback, so on a cache hit the freshly-read value is unused.

• Context: Harmless in practice — the env var doesn't change within a process run, so a cached URL already reflects the correct domain. At most a dead read on cache hit.

• Suggestion: Optional readability nit: move the const inside the callback.

• File: packages/webdriver-utils/test/providers/genericProvider.test.js, test/providers/automateProvider.test.js

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: No test for browserstackExecutor('percyScreenshot', undefined) with PERCY_ENABLE_DEV=true, nor for the Cache.withCache cache-hit path (tests call Cache.reset() first).

• Suggestion: Optional: add an undefined-args case; current coverage is otherwise solid.

Note: the reviewer's documentation finding ("no docs for the new env vars") is not valid — this PR adds packages/webdriver-utils/DEVELOPMENT.md, which documents all three variables, their defaults, and copy-paste export blocks.

---

Verdict: PASS — Small, focused, well-tested change that defaults to byte-identical production behavior when the new env vars are unset. Remaining findings are optional Low-severity hardening/nits, not blockers.