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

[bhokaremoin]

Summary

Makes the screenshot executor's project routing and the CDP / Automate host resolution optionally configurable, all defaulting to current behavior. No change unless explicitly opted in.

Changes

• genericProvider.browserstackExecutor: optionally include a projectId in the percyScreenshot payload, injected once at this shared chokepoint (covers begin/end and getTiles for both the Automate and Playwright providers).
• playwrightDriver: CDP execute host is now resolved with a default of cdp.browserstack.com.
• automateProvider / playwrightProvider: Automate debug-URL host resolved with a default of automate.browserstack.com.

All three are opt-in and fall back to the existing values, so the default path is unchanged.

Testing

• Default path: executor payloads and hosts are unchanged; builds finalize (web + automate + playwright).
• Opt-in path: override applied as expected.
• Unit tests pass (238/238).

Post-Deploy Monitoring & Validation

No additional operational monitoring required: the overrides are opt-in and default to current production behavior; default-path payloads and hosts are byte-identical (covered by unit tests).

---

🤖 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]

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.

[bhokaremoin]

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

[bhokaremoin]

Claude Code PR Review

PR: #2274 • Head: ce10d36 • Reviewers: stack:code-reviewer

Summary

Adds three opt-in env-var overrides to @percy/webdriver-utils for staging/percy-dev testing — PERCY_ENABLE_DEV (injects projectId: percy-dev into percyScreenshot executor payloads), PERCY_CDP_DOMAIN (CDP execute host), and PERCY_AUTOMATE_DOMAIN (Automate debug-URL host) — all defaulting to current production values, with unit tests covering both the override and the byte-identical default path.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	percy-dev is a project slug, not a credential; domains are public hosts
High	Security	Authentication/authorization checks present	N/A	No auth surface touched
High	Security	Input validation and sanitization	Pass	Env-var domains are interpolated unvalidated, but scheme is hardcoded https:// and these are internal dev-only vars (see Low finding)
High	Security	No IDOR — resource ownership validated	N/A	No resource access changes
High	Security	No SQL injection (parameterized queries)	N/A	No database code
High	Correctness	Logic is correct, handles edge cases	Pass	One latent edge case: Cache.withCache key omits the domain (see Medium finding); unreachable in real usage since env vars don't change mid-process
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Existing driver-null guards retained; no new error paths swallowed
High	Correctness	No race conditions or concurrency issues	Pass	Env reads are synchronous; cache behavior unchanged for constant env
Medium	Testing	New code has corresponding tests	Pass	All three env vars tested with try/finally / afterEach cleanup
Medium	Testing	Error paths and edge cases tested	Pass	Default-path (override unset) tested byte-identical for all three
Medium	Testing	Existing tests still pass (no regressions)	Pass	All CI checks green on ce10d36 (lint, build, regression, full test matrix)
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data fetching changes
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable
Medium	Quality	Follows existing codebase patterns	Pass	Matches existing process.env.PERCY_* flag conventions
Medium	Quality	Changes are focused (single concern)	Pass	Single concern: opt-in staging/dev routing overrides
Low	Quality	Meaningful names, no dead code	Pass	Clear names (cdpDomain, automateDomain)
Low	Quality	Comments explain why, not what	Pass	Minor: Cache.reset() in automateProvider test would benefit from a why-comment (see Low finding)
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies

Findings

No findings rise to Critical/High. The notable non-blocking items:

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

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: automateDomain is captured by the Cache.withCache closure, but the cache key is only this.driver.sessionId. A second setDebugUrl call for the same session with a different PERCY_AUTOMATE_DOMAIN value returns the stale cached URL. Unreachable in real usage (env vars are fixed at process launch), but it's exactly the kind of hazard the staging workflow this PR enables could trip in test harnesses — the new test already needs Cache.reset() to dodge it.

• Suggestion: Drop the Cache.withCache wrapper — the URL is a pure template-string construction with nothing async to cache — or include the domain in the cache key.

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

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: If browserstackExecutor('percyScreenshot') were ever called without args, the spread turns a previously argument-less payload into arguments: { projectId: 'percy-dev' }. All current call sites pass an object, so this is latent only.

• Suggestion: args = { ...(args || {}), projectId: 'percy-dev' }; to make the intent explicit.

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

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: PERCY_CDP_DOMAIN / PERCY_AUTOMATE_DOMAIN are interpolated into URLs without validation. Practical risk is very low: scheme is hardcoded to https:// and the vars are internal-developer-only.

• Suggestion: Optional — validate against a hostname pattern if these ever become user-facing.

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

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: The Cache.reset() is required because the preceding test populates bstackSessionDetails for the same session ID; the test is order-dependent without it being stated.

• Suggestion: Add a one-line comment explaining why the reset is needed.

---

Verdict: PASS — no Critical/High findings; the Medium cache-key note is non-blocking (unreachable outside test harnesses) and the rest are nits.