feat(webdriver-utils): env-var overrides for staging/percy-dev testing by bhokaremoin · Pull Request #2274 · percy/cli

bhokaremoin · 2026-06-09T18:10:14Z

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

Replace manual node_modules edits with env vars that default to prod: - PERCY_ENABLE_DEV=true injects projectId:'percy-dev' once at the shared browserstackExecutor chokepoint (covers begin/end + automate/playwright getTiles); omitted entirely when unset, so prod payloads are unchanged. - PERCY_CDP_DOMAIN overrides the Playwright driver CDP host. - PERCY_AUTOMATE_DOMAIN overrides the Automate debug-URL host. Adopts the existing Python percy-appium-app PERCY_ENABLE_DEV convention so one export works across JS + Python. Adds tests for on/off/non-screenshot paths and host overrides. Usage runbook lives in internal docs (not this public repo) to avoid exposing staging cluster hostnames. 🤖 Generated with Claude Opus 4.8 (1M context) via Claude Code + Compound Engineering v2.50.0 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

bhokaremoin

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

bhokaremoin · 2026-06-09T18:58:02Z

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.

…cutor Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

bhokaremoin

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

bhokaremoin · 2026-06-11T18:16:36Z


  async setDebugUrl() {
    if (!this.driver) throw new Error('Driver is null, please initialize driver with createDriver().');
+    const automateDomain = process.env.PERCY_AUTOMATE_DOMAIN || 'automate.browserstack.com';


[Medium] Cache key omits the domain — stale debug URL on repeated setDebugUrl calls

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 returns the stale cached URL. Unreachable in real usage (env vars are fixed at process launch), but it's the kind of hazard this staging workflow could trip in test harnesses — the new test already needs Cache.reset() to dodge it.

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

Reviewer: stack:code-reviewer

bhokaremoin · 2026-06-11T18:16:36Z

  async browserstackExecutor(action, args) {
    if (!this.driver) throw new Error('Driver is null, please initialize driver with createDriver().');
+    if (action === 'percyScreenshot' && process.env.PERCY_ENABLE_DEV === 'true') {
+      args = { ...args, projectId: 'percy-dev' };


[Low] Spread on possibly-absent args is a latent hazard

If browserstackExecutor('percyScreenshot') were ever called without args, this 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.

Reviewer: stack:code-reviewer

bhokaremoin · 2026-06-11T18:16:36Z

    }
    const options = this.requestPostOptions(command);
-    const baseUrl = `https://cdp.browserstack.com/wd/hub/session/${this.sessionId}/execute`;
+    const cdpDomain = process.env.PERCY_CDP_DOMAIN || 'cdp.browserstack.com';


[Low] Domain env vars are interpolated without validation

PERCY_CDP_DOMAIN (and PERCY_AUTOMATE_DOMAIN in the providers) is interpolated into the URL unvalidated. Practical risk is very low: the scheme is hardcoded to https:// and these are internal-developer-only vars.

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

Reviewer: stack:code-reviewer

bhokaremoin · 2026-06-11T18:16:36Z

    });

+    it('uses PERCY_AUTOMATE_DOMAIN when set', async () => {
+      Cache.reset();


[Low] Unexplained Cache.reset() hides a test-order dependency

This reset is required because the preceding 'sets automate url' test populates bstackSessionDetails for the same session ID — without it this test would read the stale prod URL from cache. That order-dependence isn't stated anywhere.

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

Reviewer: stack:code-reviewer

bhokaremoin · 2026-06-11T18:16:39Z

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.

bhokaremoin requested a review from a team as a code owner June 9, 2026 18:10

bhokaremoin requested review from amandeepsingh333 and ninadbstack June 9, 2026 18:10

bhokaremoin commented Jun 9, 2026

View reviewed changes

Comment thread packages/webdriver-utils/src/playwrightDriver.js

Comment thread packages/webdriver-utils/src/providers/genericProvider.js

bhokaremoin force-pushed the feat/staging-env-overrides branch from 364dded to ae01e19 Compare June 10, 2026 03:39

rishigupta1599 approved these changes Jun 10, 2026

View reviewed changes

bhokaremoin mentioned this pull request Jun 10, 2026

feat(app-automate): send projectId on percyScreenshot begin/end percy/percy-appium-js#574

Open

chore(webdriver-utils): drop explanatory comment from browserstackExe…

ce10d36

…cutor Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

bhokaremoin commented Jun 11, 2026

View reviewed changes

Shivanshu-07 approved these changes Jun 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

feat(webdriver-utils): env-var overrides for staging/percy-dev testing#2274
bhokaremoin wants to merge 2 commits into
masterfrom
feat/staging-env-overrides

bhokaremoin commented Jun 9, 2026 •

edited

Loading

Uh oh!

bhokaremoin left a comment

Uh oh!

Uh oh!

Uh oh!

bhokaremoin commented Jun 9, 2026

Uh oh!

bhokaremoin left a comment

Uh oh!

bhokaremoin Jun 11, 2026

Uh oh!

bhokaremoin Jun 11, 2026

Uh oh!

bhokaremoin Jun 11, 2026

Uh oh!

bhokaremoin Jun 11, 2026

Uh oh!

bhokaremoin commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bhokaremoin commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Post-Deploy Monitoring & Validation

Uh oh!

bhokaremoin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bhokaremoin commented Jun 9, 2026

Claude Code PR Review

Summary

Review Table

Findings

Uh oh!

bhokaremoin left a comment

Choose a reason for hiding this comment

Uh oh!

bhokaremoin Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

bhokaremoin Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

bhokaremoin Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

bhokaremoin Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

bhokaremoin commented Jun 11, 2026

Claude Code PR Review

Summary

Review Table

Findings

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bhokaremoin commented Jun 9, 2026 •

edited

Loading