Repository navigation
fix: recognize bsstag hosts and localize session REST host on named staging envs (SDK-6948) - #275
AakashHotchandani wants to merge 5 commits into
Conversation
…taging envs (SDK-6948) With BROWSERSTACK_STAGING_ENV set to a bare token (e.g. k8s) the hub is hub-<env>.bsstag.com. getCloudProvider / isBrowserstackInfra only matched browserstack.com, so the session was classified unknown_grid: session status was never marked and TRA test runs carried no Automate session link. Recognize *.bsstag.com only for a bare-token staging env; dotted value or unset keeps prod behaviour. The service also hardcoded the prod api host for the session REST base URL (session URL print + onReload status marking). Build it from APIUtils, which the CLI bootstrap has already localized to the binary-supplied hosts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Verdict: APIUtils defaults. Fix (B) now makes isBrowserstackSession() true on staging, so _printSessionURL / _updateJob / onReload send staging credentials to api.browserstack.com. Also verify independently (non-blocking 🟡): App Automate on a named env resolves to api-cloud-<env>.bsstag.com/app-automate/sessions.
Summary: 0 critical · 2 warnings · 4 suggestions across 5 files reviewed.
Production behaviour is unchanged: with BROWSERSTACK_STAGING_ENV unset, isStagingEnvBsstagHost is false, and the APIUtils defaults equal the old hardcoded hosts for Automate, App Automate and TurboScale. The verdict hinges on the classic (non-CLI / multiremote) path. Fix (B) now activates session REST calls there, but (C) only localizes hosts on the CLI path. Fix (C) also depends on browserstack-binary#1889 (OPEN): binary main still hands back api-<env>, which 404s. Rovo enrichment unavailable — review based on local SDK docs only.
Cross-SDK parity (SDK-6948)
| Item | node #2655 | python #1282 | wdio #275 |
|---|---|---|---|
| Host check shape | Replaces prod recognition under a named env | Additive (browserstack.com OR named env + bsstag) |
Additive (includes('browserstack') / .browserstack.com OR isStagingEnvBsstagHost) |
| Host matching | — | Legacy substring; CLI netloc.endswith missed port/uppercase |
Exact bsstag.com or .bsstag.com suffix on port-free options.hostname; rejects evilbsstag.com and bsstag.com.attacker.io; case-sensitive (S3) |
| Named-env definition | Non-empty, no .; read at module load |
Non-empty, no .; read per call |
Non-empty, no .; read per call |
| Session REST host | — | — | APIUtils ← binary config.apis on the CLI flow (prod api.browserstack.com / api-cloud.browserstack.com; k8s apik8s with #1889, api-k8s on binary main; preprod api-preprod); classic flow stays prod (W1); App Automate on a named env → api-cloud-<env> (W2) |
| Vacuous tests | — | 3 | 1 assertion (anything-saucelabs.com → unknown_grid) (S2) |
| #56 / #65 overlap | n/a | n/a | Both still OPEN, same 3 lines, ungated includes('bsstag') — close as superseded (S4) |
See inline comments below for full Problem and Suggested Fix detail on each finding.
Generated by Automated SDK PR review.
|
|
||
| // Resolved here, after beforeSession's CLI bootstrap has applied the binary-supplied | ||
| // GRR/staging hosts (APIUtils.updateURLSForGRR); the defaults are the production hosts. | ||
| this._sessionBaseUrl = `${APIUtils.BROWSERSTACK_AUTOMATE_API_URL}/automate/sessions` |
There was a problem hiding this comment.
⚠️ Warning — [CORRECTNESS] Classic (non-CLI) flow now sends staging creds to the prod session REST API
Problem
APIUtils is localized in only one place: BrowserstackCLI.loadModules() → APIUtils.updateURLSForGRR(config.apis) (src/cli/index.ts:156). On the classic flow, that call never runs. The classic flow covers multiremote (BROWSERSTACK_IS_MULTIREMOTE=true), frameworks outside CLISupportedFrameworks, and a CLI bootstrap that errors and is logged at cli/index.ts:104. On that flow APIUtils.BROWSERSTACK_AUTOMATE_API_URL stays https://api.browserstack.com, so line 252 still produces the production host.
Before this PR that did not matter on staging. getCloudProvider returned unknown_grid for hub-k8s.bsstag.com, so isBrowserstackSession() was false and _printSessionURL, _update and _updateJob all returned early. Fix (B) now makes isBrowserstackSession() true on staging, including multiremote. Because the multiremote branch of getCloudProvider was also changed, the classic flow now actively calls these:
GET https://api.browserstack.com/automate/sessions/<id>.jsoninbefore()(_printSessionURL)PUT …/<id>.jsoninafter()(_updateJob, gated!isRunning()) and inonReload(_update)
All three carry the staging user:key. That is exactly what the PR body says (C) fixes ("On staging it sent staging creds to the production API"). Two things follow. Session status is never marked on staging for these runs. _printSessionURL also dereferences (await res.json()).automation_session.browser_url without a guard, so a 401 JSON reply throws a TypeError out of before(). WDIO logs it rather than failing the test, but it is new noise. This is anti-pattern #23 (one path fixed, the parallel path not). Production is unaffected, because with the variable unset isStagingEnvBsstagHost returns false.
Suggested Fix
When the CLI is not running and BROWSERSTACK_STAGING_ENV is a bare token, derive the staging API host locally, mirroring binary #1889's stagingApiHost. Do it in one helper next to isStagingEnvBsstagHost so both checks share the "named env" definition:
export function stagingEnvApiHost(): string | undefined {
const stg = process.env.BROWSERSTACK_STAGING_ENV
if (!stg || stg.includes('.')) { return undefined }
return stg === 'preprod' ? 'https://api-preprod.bsstag.com' : `https://api${stg}.bsstag.com`
}
// before():
const automateApi = BrowserstackCLI.getInstance().isRunning()
? APIUtils.BROWSERSTACK_AUTOMATE_API_URL
: (stagingEnvApiHost() ?? APIUtils.BROWSERSTACK_AUTOMATE_API_URL)If that is out of scope, document the classic flow as unsupported on staging in the PR body and the changeset. Add a unit test that pins the classic + staging host either way.
Confidence: 🟢 Objectively traceable. updateURLSForGRR has a single caller, in CLI loadModules, and the gates on isBrowserstackSession / !isRunning() are in the file. Matches shared anti-pattern #23 (parallel-path drift).
There was a problem hiding this comment.
Valid, thanks. Following the cross-SDK decision for this ticket, I did not add staging host-name derivation to the SDK. Instead, b01c4dc: (1) The PR body now documents that the classic (non-CLI) flow is not supported on named staging envs. It covers multiremote, frameworks outside CLISupportedFrameworks, and a failed CLI bootstrap; in all of them APIUtils keeps the prod hosts, so marking there gets a 401. (2) _printSessionURL no longer dereferences .automation_session.browser_url unguarded. A reply without automation_session (e.g. a 401) logs Could not fetch Browserstack session URL (<status>) at debug and returns, instead of throwing a TypeError out of before(). Test: logs at debug instead of throwing when the session REST reply has no automation_session, which fails without the guard. Production is unaffected, because isStagingEnvBsstagHost is false with the variable unset.
|
|
||
| if (this._isAppAutomate()) { | ||
| this._sessionBaseUrl = 'https://api-cloud.browserstack.com/app-automate/sessions' | ||
| this._sessionBaseUrl = `${APIUtils.BROWSERSTACK_AA_API_CLOUD_URL}/app-automate/sessions` |
There was a problem hiding this comment.
⚠️ Warning — [CORRECTNESS] App Automate session REST on a named env resolves to api-cloud-<env>.bsstag.com, not the API_SUBDOMAIN host
Problem
To keep production byte-identical, the App Automate base URL is mapped to BROWSERSTACK_AA_API_CLOUD_URL. updateURLSForGRR fills that from apis.appAutomate.upload, not apis.appAutomate.api. In production the binary's appAutomate.upload is https://api-cloud.browserstack.com, so the value matches the old hardcoded host. ✅
On a named env, binary core/config/apis.js (main, and unchanged by #1889) sets grp.upload = https://api-cloud-${stg}.bsstag.com. The resolved URL is then https://api-cloud-k8s.bsstag.com/app-automate/sessions/<id>.json. Binary #1889's own rationale says railsApp serves /automate/* and /app-automate/* only when the subdomain equals API_SUBDOMAIN (api<env>, or api-preprod). By that rule, /app-automate/sessions on api-cloud-<env> reaches Rails and 404s. So (C) does not yet deliver for App Automate on staging. It is not a regression (before, the request went to production with staging creds), but the PR body and changeset say the session REST host "follows the binary-supplied hosts" without that caveat. The E2E only covered Automate (Chrome / Windows 11).
Suggested Fix
Either verify api-cloud-k8s.bsstag.com/app-automate/sessions/<id>.json on k8s, or limit the claim. The PR body and changeset would then say App Automate session REST on named envs is pending. The binary appAutomate.upload staging host (or an apis.appAutomate.api-based path for staging only) needs a follow-up there. Also add a service.test.ts case that sets APIUtils.BROWSERSTACK_AA_API_CLOUD_URL and asserts the /app-automate/sessions base, so the App Automate mapping is pinned.
Confidence: 🟡 It depends on how staging infra routes api-cloud-<env>.bsstag.com, which was not verified E2E. Inferred from binary #1889's documented API_SUBDOMAIN rule.
There was a problem hiding this comment.
Partly valid. I checked it on k8s with the same credentials. api-cloud-k8s.bsstag.com/automate/sessions/<automate id>.json returns 200 with the same JSON as apik8s, and an unknown /app-automate/sessions/<id>.json returns the same JSON {"status":404,"message":"Not Found"} on both api-cloud-k8s and apik8s. So api-cloud-<env> does reach the Rails API, which goes against the "404s at the edge" inference. I did not have an App Automate session to prove the real AA path, so b01c4dc scopes the claim: the PR body says the App Automate path is not E2E-validated (Automate only). It also adds a service.test.ts case that pins BROWSERSTACK_AA_API_CLOUD_URL to /app-automate/sessions. No code change: production stays identical (api-cloud.browserstack.com).
| // Ensure capabilities are not null in case of multiremote | ||
|
|
||
| // Resolved here, after beforeSession's CLI bootstrap has applied the binary-supplied | ||
| // GRR/staging hosts (APIUtils.updateURLSForGRR); the defaults are the production hosts. |
There was a problem hiding this comment.
💡 Suggestion — [DOCS] "Production behaviour is identical" holds only when the config server's grr_urls returns the default hosts
Binary API.initialize(grrDetails) replaces automate and appAutomate wholesale with the config server's grr_urls. For a GRR-enabled production customer, _sessionBaseUrl therefore now follows the regional automate.api / appAutomate.upload instead of api.browserstack.com / api-cloud.browserstack.com. On the CLI flow _sessionBaseUrl only drives _printSessionURL and the onReload update. The binary already marks sessions against the same automate.api, so this is very likely correct, but it is a production behaviour change for those customers. The PR body, the changeset, and the code comment ("the defaults are the production hosts") should say so. A GRR-host unit case would pin it.
There was a problem hiding this comment.
Agreed, fixed in b01c4dc. The code comment in before(), the PR body, and the internal release note (the source of the auto-changeset) now say that for a GRR-enabled account _sessionBaseUrl follows the regional automate.api / appAutomate.upload, the same automate.api the binary already marks sessions against, rather than the global prod hosts. The existing uses the GRR/staging api host test plus the new TurboScale case (https://api-eu.browserstack.com/automate-turboscale/v1/sessions) pin the GRR mapping.
| it('does not treat non-bsstag hosts as browserstack for a named staging env', () => { | ||
| process.env.BROWSERSTACK_STAGING_ENV = 'k8s' | ||
| expect(utils.isStagingEnvBsstagHost('hub.bsstag.com.evil.example')).toBe(false) | ||
| expect(getCloudProvider({ options: { hostname: 'anything-saucelabs.com' } } as any)).toEqual('unknown_grid') |
There was a problem hiding this comment.
💡 Suggestion — [TESTING] Edge-host and dotted-env coverage gaps; one assertion is vacuous
getCloudProvider({ hostname: 'anything-saucelabs.com' })→unknown_gridpasses with or without this PR, so it does not exercise the new branch. Replace it with the attack hosts it is meant to cover:evilbsstag.com(suffix without a dot),bsstag.com.attacker.io, and the barebsstag.com(true). Thehub.bsstag.com.evil.exampleassertion on the line above is meaningful. Keep it.- The dotted-env case does not assert
isBrowserstackInfra(stagingConfig)is false. That is the second OR site, and it is only covered on the named and unset paths. service.test.tscovers only the Automate host. Add App Automate (BROWSERSTACK_AA_API_CLOUD_URL) and turboscale cases (see Warning 2).
There was a problem hiding this comment.
Fixed in b01c4dc. The vacuous anything-saucelabs.com assertion is replaced with evilbsstag.com (false, also via getCloudProvider), bsstag.com.attacker.io (false) and bare bsstag.com (true); hub.bsstag.com.evil.example is kept. The dotted-env case now also asserts isBrowserstackInfra(stagingConfig) is false. service.test.ts gains App Automate (BROWSERSTACK_AA_API_CLOUD_URL -> /app-automate/sessions) and TurboScale (/automate-turboscale/v1/sessions) base URL cases.
| if (!stagingEnv || stagingEnv.includes('.')) { | ||
| return false | ||
| } | ||
| return hostname === 'bsstag.com' || hostname.endsWith('.bsstag.com') |
There was a problem hiding this comment.
💡 Suggestion — [CORRECTNESS] isStagingEnvBsstagHost does not normalize case or whitespace
The matching itself is a sound suffix check, not a substring. evilbsstag.com and bsstag.com.attacker.io are rejected, and the env is read per call, as in python and unlike node's module-load read. Two hardening gaps remain. HUB-K8S.BSSTAG.COM is rejected, because DNS is case-insensitive but the check is not. A whitespace-only BROWSERSTACK_STAGING_ENV (' ') counts as a named env. Both are cheap to close: const h = hostname.toLowerCase() and const stagingEnv = process.env.BROWSERSTACK_STAGING_ENV?.trim(). This also avoids python's CLI-path miss (uppercase netloc.endswith). The existing .browserstack.com check is equally case-sensitive, so this is consistency hardening, not a regression.
There was a problem hiding this comment.
Fixed in b01c4dc, as a shared rule across all SDKs for this ticket. BROWSERSTACK_STAGING_ENV is now .trim()-ed, so a blank value counts as unset, and the hostname is lowercased before the bsstag.com / .bsstag.com suffix check. The no-dot rule is unchanged. Tests: trims BROWSERSTACK_STAGING_ENV and matches the host case-insensitively (' k8s ' + HUB-K8S.BSSTAG.COM -> browserstack) and treats a blank BROWSERSTACK_STAGING_ENV as unset.
There was a problem hiding this comment.
Follow-up for cross-SDK consistency in 1b89b02: the bsstag host check is now also true when BROWSERSTACK_ENV, trimmed and lowercased, is staging, stag, preprod or pre-prod. This matches node-agent constants and the binary's getEnvironmentUrl. The same trim and case-insensitive host rule applies. Tests: those four values (plus ' PreProd ') are recognised through isStagingEnvBsstagHost, getCloudProvider and isBrowserstackInfra; production / prod / empty keep the prod behaviour; look-alike hosts (evilbsstag.com, hub.bsstag.com.attacker.io) are rejected. Full suite 1402/1402.
| } | ||
| } | ||
| } else if (browser.options && browser.options.hostname && browser.options.hostname.includes('browserstack')) { // Single browser instance | ||
| } else if (browser.options && browser.options.hostname && (browser.options.hostname.includes('browserstack') || isStagingEnvBsstagHost(browser.options.hostname))) { // Single browser instance |
There was a problem hiding this comment.
💡 Suggestion — [PROCESS] #56 / #65 are still OPEN (not on main), edit the same three lines, and would reintroduce unconditional bsstag matching
The PR body says #56/#65 are "on main". They are both OPEN and unmerged. Each rewrites the same three lines (both getCloudProvider branches with .includes('bsstag'), and isBrowserstackInfra with endsWith('.bsstag.com')), ungated. Whichever merges second conflicts. A careless resolution that keeps both would bring back production-wide bsstag matching (substring includes('bsstag') also matches evilbsstag.com). Close #56/#65 as superseded by this PR, and correct the body.
There was a problem hiding this comment.
Agreed, thanks. The PR body wording was wrong: #56 and #65 are open, unmerged PRs against main, not on main. I have corrected the Overlaps paragraph. It now says they rewrite the same three lines with ungated includes('bsstag') / endsWith('.bsstag.com'), that the second to merge will conflict, and that keeping both would bring back production-wide bsstag matching. This PR supersedes their util.ts change. Closing #56/#65 is left to their author (no change in this PR).
…SDK-6948) - isStagingEnvBsstagHost trims BROWSERSTACK_STAGING_ENV (blank = unset) and compares the hostname case-insensitively (shared rule across SDKs). - _printSessionURL logs at debug instead of throwing when the session REST reply has no automation_session (e.g. a 401 on the classic flow against a named staging env). - Tests: attack hosts (evilbsstag.com, bsstag.com.attacker.io, bare bsstag.com), dotted-env isBrowserstackInfra, trim/case, App Automate and TurboScale session base URLs, and the _printSessionURL guard. - Comment: document that GRR-enabled accounts follow the regional api host. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…reprod (SDK-6948) Cross-SDK consistency: a *.bsstag.com host is also treated as BrowserStack when BROWSERSTACK_ENV (trimmed, lowercased) is staging, stag, preprod or pre-prod, matching node-agent constants and the binary's getEnvironmentUrl. Production/unset values keep the prod-only checks; look-alike hosts are still rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Re-review (round 2)
Verdict: ✅ Good to go. There are no 🔴 findings. The two 🟡 items below are non-blocking.
Scope: 832f42cd...1b89b023 (b01c4dc review fixes, 5aa7589 changeset, 1b89b02 BROWSERSTACK_ENV staging/preprod recognition). I also re-checked the full PR diff: 5 files, +208 −6. Rovo was unavailable, and the summary gate was auto-confirmed.
Summary: 0 critical · 1 warning · 1 suggestion (new). All 6 prior findings are closed: 4 RESOLVED and 2 DECLINED-ACCEPTABLE.
═══════════════════════════════════════
Prior findings (review 5453042923 @ 832f42c)
| # | Prior finding | Ruling | Evidence |
|---|---|---|---|
| W1 | Classic (non-CLI / multiremote) flow sends staging creds to the prod session REST API | DECLINED-ACCEPTABLE | APIUtils.updateURLSForGRR still has a single caller, src/cli/index.ts:156 (CLI loadModules). Every non-CLI path that reaches session REST still builds on the prod APIUtils defaults: before() → _printSessionURL (service.ts:436 → :1326 GET); after() → _updateJob (:797, :842, gated !isRunning()) → _update (:1297 PUT/PATCH); _setSessionName → _updateJob (:1394-1396); onReload → _update (:1207) and _printSessionURL (:1222). APIUtils is not localized before any of these. The author declined the SDK-side host derivation, following the cross-SDK decision. Instead, they documented the classic flow as unsupported on named staging envs (PR body "Not supported" section, .changeset/pr-275.md:6) and guarded the TypeError (service.ts:1350-1356, test service.test.ts:461). This is acceptable: the path is internal staging only, and production is unchanged because isStagingEnvBsstagHost is false with both env vars unset. Two residuals remain. The guard only covers JSON replies (S-new below), and 1b89b02 widens the affected surface (W-new below). |
| W2 | App Automate on a named env resolves to api-cloud-<env> (possible 404) |
RESOLVED (claim scoped) | The author probed k8s: api-cloud-k8s.bsstag.com reaches the Rails API, which refutes the inferred edge 404. The PR body now says the App Automate path is not E2E-validated (Automate only). The App Automate base URL is pinned by service.test.ts:756. |
| S1 | "Production identical" holds only for default grr_urls |
RESOLVED | The code comment at service.ts:250-253, the PR body (C) and .changeset/pr-275.md:6 now name the GRR regional host. The TurboScale GRR case is in service.test.ts:774. |
| S2 | Vacuous anything-saucelabs.com assertion; missing dotted-env infra and App Automate / TurboScale cases |
RESOLVED | The vacuous assertion is replaced by evilbsstag.com / bsstag.com.attacker.io (false) and bare bsstag.com (true) (util.test.ts:621-625). The dotted env now asserts isBrowserstackInfra false (:615). Service App Automate and TurboScale cases were added. |
| S3 | No case or whitespace normalization | RESOLVED | util.ts:1157 (?.trim(), blank means unset) and :1163 (hostname.toLowerCase()). Tests cover ' k8s ' + HUB-K8S.BSSTAG.COM, and a blank env. |
| S4 | #56 / #65 described as "on main" | RESOLVED (body) / closure DECLINED-ACCEPTABLE | The Overlaps paragraph is corrected. Closing #56/#65 is left to their author, which is outside this PR's scope. |
═══════════════════════════════════════
New findings
🟡 Warning: [CORRECTNESS/DOCS] BROWSERSTACK_ENV=staging|stag|preprod|pre-prod widens bsstag recognition with no E2E, and is missing from the changeset and the "Not supported" section (util.ts:1159)
1b89b02 also makes isBrowserstackSession() true for *.bsstag.com hubs when BROWSERSTACK_ENV names staging or preprod. On that path, APIUtils is not localized even on the CLI flow:
- The binary's
API.initialize(grrDetails)re-asserts the bsstag hosts only forBROWSERSTACK_STAGING_ENV. - Under
BROWSERSTACK_ENV,automate.api/appAutomate.uploadare whatever the config server'sgrr_urlsreturn. The binary's own comment says these are the prod values.
So _printSessionURL, onReload → _update, and (classic) _updateJob can now send preprod or staging credentials to api.browserstack.com. Before this commit they returned early on unknown_grid. That is the W1 / anti-pattern #23 surface again, now on a second env selector. It still affects internal envs only.
Two smaller gaps:
- The binary's
getEnvironmentUrllowercases the value but does not trim it. So the' PreProd 'test case asserts behaviour that the binary does not share. The SDK would call the hub BrowserStack while the binary keeps prod hosts. - The PR E2E covers only
BROWSERSTACK_STAGING_ENV=k8s..changeset/pr-275.md(auto-generated in 5aa7589, before 1b89b02) does not mentionBROWSERSTACK_ENVat all.
Suggested fix: do one of the following, then mention BROWSERSTACK_ENV in the changeset:
- Validate a
BROWSERSTACK_ENV=preprodrun end to end and confirm_sessionBaseUrlisapi-preprod.bsstag.comthere. - Extend the "Not supported" note: on
BROWSERSTACK_ENVstaging/preprod, the SDK-side session URL print and reload update may target prod hosts, and only the binary's marking is localized.
Confidence: 🟡. The grr_urls contents on preprod or staging config servers were not verified. This is inferred from binary core/config/apis.js initialize(), read from a local checkout at 1.53.1.
💡 Suggestion: [ROBUSTNESS] The _printSessionURL guard only covers JSON replies (service.ts:1350)
(await res.json())?.automation_session still throws a SyntaxError when the reply is not JSON. The PR body's own "Known gap" observed exactly that: "Session-status marking then fails with an HTML reply". A plain-text Basic-auth 401 would also do it. In those cases the new guard is never reached, and before() still throws (anti-pattern #5). The TurboScale branch ((await res.json()).url) has the same shape, but it predates this PR.
Suggested fix: check response.ok before parsing, or wrap the parse:
if (!response.ok) {
BStackLogger.debug(`Could not fetch Browserstack session URL (${response.status}) at ${sessionUrl}`)
return
}A second test using new Response('<html>…</html>', { status: 401 }) would pin it.
═══════════════════════════════════════
Test quality (new commits)
- Guard test (
service.test.ts:461): not vacuous. It rejects with aTypeErrorwithout the guard. It models a JSON 401 only (see the 💡 above). - App Automate / TurboScale base-URL tests (
:756,:774): not vacuous, because they set non-default hosts. Like the pre-existing case, they callasync before()withoutawait. They depend on noawaitrunning before line 254, which holds only while the mockedbrowsercounts as a BrowserStack session. That is fragile but passes. BROWSERSTACK_ENVtests (util.test.ts): the positive and negative pairs are meaningful, and the look-alike host rejection is covered. The prod-valuesit.eachdoes not assertisBrowserstackInfra, which is minor.
Anti-patterns
- #5: the guard improves graceful degradation but is incomplete (💡).
- #8: N/A. There is no build or packaging change.
- #23: W1 is accepted as a documented limitation, and 1b89b02 widens it (🟡).
- #26: N/A. The new guard is reachable: a real 401 with a JSON body hits it. It is not dead code.
Per-file confidence
.changeset/pr-275.md 🟡 missing BROWSERSTACK_ENV note
src/service.ts 🟢 guard OK; non-JSON reply gap is 💡
src/util.ts 🟡 BROWSERSTACK_ENV path not E2E-validated
tests/service.test.ts 🟢
tests/util.test.ts 🟢
Verdict
✅ Good to go. There are no 🔴 findings, and the 🟡 BROWSERSTACK_ENV item is non-blocking. Every prior finding is resolved, or declined with documentation that is acceptable for an internal-only path.
Generated by Automated SDK PR review.
| export function isStagingEnvBsstagHost(hostname: string): boolean { | ||
| const stagingEnv = process.env.BROWSERSTACK_STAGING_ENV?.trim() | ||
| const namedStagingEnv = Boolean(stagingEnv) && !stagingEnv!.includes('.') | ||
| const browserstackEnv = process.env.BROWSERSTACK_ENV?.trim().toLowerCase() || '' |
There was a problem hiding this comment.
🟡 Warning: [CORRECTNESS/DOCS] BROWSERSTACK_ENV staging/preprod widens bsstag recognition with no E2E
With BROWSERSTACK_ENV set to staging, stag, preprod or pre-prod, isBrowserstackSession() is now true for *.bsstag.com hubs. On this path, though, APIUtils is not localized even on the CLI flow. The binary's API.initialize(grrDetails) re-asserts bsstag hosts only for BROWSERSTACK_STAGING_ENV. Otherwise automate.api / appAutomate.upload come from the config server's grr_urls, which the binary's own comment says are prod values. As a result, _printSessionURL and onReload → _update (plus _updateJob on the classic flow) can now send preprod or staging credentials to api.browserstack.com. Before this commit they returned early on unknown_grid. This is the W1 / anti-pattern #23 surface again, on a second env selector. It affects internal envs only.
Two smaller gaps:
- The binary's
getEnvironmentUrllowercases the value but does not trim it, so the' PreProd 'case is SDK-only parity. - The E2E covers only
BROWSERSTACK_STAGING_ENV=k8s, and.changeset/pr-275.mddoes not mentionBROWSERSTACK_ENV.
Suggested fix: do one of these, and add BROWSERSTACK_ENV to the changeset:
- Validate a
BROWSERSTACK_ENV=preprodrun and confirm_sessionBaseUrlisapi-preprod.bsstag.com. - Extend the PR body's "Not supported" note to say that SDK-side session REST calls may target prod hosts under
BROWSERSTACK_ENV.
Confidence: 🟡. The grr_urls contents on preprod or staging were not verified. This is inferred from binary core/config/apis.js initialize().
| }) | ||
| const res = response.clone() | ||
| browserUrl = (await res.json()).automation_session.browser_url | ||
| const session = (await res.json())?.automation_session |
There was a problem hiding this comment.
💡 Suggestion: [ROBUSTNESS] The guard only covers JSON replies
res.json() throws a SyntaxError on a non-JSON body before ?.automation_session is ever checked. The PR body's "Known gap" saw exactly this ("fails with an HTML reply"), and a plain-text Basic-auth 401 would do the same. In those cases before() still throws (anti-pattern #5). Checking response.ok first covers both cases:
if (!response.ok) {
BStackLogger.debug(`Could not fetch Browserstack session URL (${response.status}) at ${sessionUrl}`)
return
}A test with new Response('<html>…</html>', { status: 401 }) would pin it.
1 participant
What is this about?
Running wdio v9 against a named internal staging env (
BROWSERSTACK_STAGING_ENV=k8s) so Automate session status marking and Automate <-> TRA session linking work there. Mirrors browserstack-node-agent#2655 (fix B) and fixes the hardcoded production session REST host inservice.ts(fix C).(B) Recognize
*.bsstag.comhubs as BrowserStack on a named staging env (util.ts)Why: with
BROWSERSTACK_STAGING_ENV=k8sthe binary hands backhub-k8s.bsstag.comas the hub.getCloudProvideronly matchedbrowserstackandisBrowserstackInfraonly matched*.browserstack.com, so the session was classified asunknown_grid. Because of that,automateModuleskipped session name/status marking (isBrowserstackSessionfalse),webdriverIOModulesetKEY_IS_BROWSERSTACK_HUB=false, and the TRA test run had nointegrations.browserstack.session_id.Change: new
isStagingEnvBsstagHost(hostname). It is true only whenBROWSERSTACK_STAGING_ENVis a bare token (no.) and the host isbsstag.com/*.bsstag.com. It is OR-ed into the two existing host checks. With the variable unset, or set to a dotted value, behaviour is unchanged from production. The env value is trimmed (blank means unset) and the hostname is compared case-insensitively. This is the shared rule across SDKs. The same bsstag recognition also applies whenBROWSERSTACK_ENV(trimmed, lowercased) isstaging,stag,preprodorpre-prod(1b89b02), matching node-agent constants and the binary'sgetEnvironmentUrl. This is the same gating as node-agent#2655. It is narrower than #56/#65, which matchbsstagunconditionally.(C) Session REST base URL follows the binary-supplied hosts (
service.ts)Why:
_sessionBaseUrlwas hardcoded tohttps://api.browserstack.com/...(plusapi-cloudfor App Automate). It is used by_printSessionURLinbefore()and byonReloadstatus marking. On staging it sent staging creds to the production API.Change:
before()now builds it fromAPIUtils(BROWSERSTACK_AUTOMATE_API_URL/BROWSERSTACK_AA_API_CLOUD_URL).beforeSession's CLI bootstrap has already localized those viaupdateURLSForGRR(config.apis). The defaults are the same production URLs, so production behaviour is identical whenever the config server returns the defaultgrr_urls. For a GRR-enabled production account,_sessionBaseUrlnow follows the regionalautomate.api/appAutomate.upload(the sameautomate.apithe binary already uses to mark the session) instead ofapi.browserstack.com/api-cloud.browserstack.com. On the CLI flow,_sessionBaseUrlonly drives_printSessionURLand theonReloadupdate. App Automate on a named env resolves toapi-cloud-<env>.bsstag.com/app-automate/sessions. On k8s,api-cloud-k8s.bsstag.comserves/automate/sessions/<id>.json(200, same JSON asapik8s) and returns the same JSON 404 asapik8sfor an unknown app-automate id, so it does reach the Rails API. The App Automate path itself is not E2E-validated here (Automate only). The existingservice.test.tsassertions on the prod URLs still pass unchanged.Not supported: classic (non-CLI) flow on a named staging env
The session REST host for a named env is only known to the CLI (
updateURLSForGRR(config.apis)inloadModules). On the classic flow (multiremote, frameworks outsideCLISupportedFrameworks, or a CLI bootstrap that failed),APIUtilskeeps the production hosts. With (B),isBrowserstackSession()is true there, so_printSessionURL,_updateJobandonReloadcallapi.browserstack.comwith the staging credentials and get a 401. Session marking is therefore not supported on the classic flow for named staging envs. The SDK does not derive staging host names itself._printSessionURLnow logs that 401 at debug instead of throwing aTypeErrorout ofbefore()(tested). Production is unaffected.Dependency: (A) BINARY_URL fallback is in #224
(A) is not duplicated here. #224 now also handles wdio's 401 reply:
requestToUpdateCLIresolved{"message":"Unauthorized"}instead of throwing, which was fixed in follow-up commit f3694d8. It is validated E2E on k8s there: empty CLI dir -> 401 -> fallback download -> spawn -> build-start onsdk-k8s. The E2E below pins the binary withSDK_CLI_BIN_PATH, so (B) and (C) are validated on their own.Known gap (outside both PRs): without
BROWSERSTACK_BINARY_URL, a staging run reuses whatever binary is already in~/.browserstack/cli, becauseupdate_clicannot update it with staging creds. On this machine that was a stock 1.58.5 build whoseapis.automate.apiisapi-k8s.bsstag.com(wrong host; the #1889 build givesapik8s). Session-status marking then fails with an HTML reply. Sessiona261f64ccc2733ba38b8784f33b835a9e48feb88shows this. Workaround: setBROWSERSTACK_BINARY_URLorSDK_CLI_BIN_PATH.Validation
Unit tests (vitest)
util.test.tscases cover: a named staging env treatshub-k8s.bsstag.comas browserstack (single + multiremote +isBrowserstackInfra); unsetBROWSERSTACK_STAGING_ENVkeeps itunknown_grid/ non-infra; a dotted value is ignored; non-bsstag hosts are still rejected.service.test.tscase:_sessionBaseUrluses the GRR-appliedAPIUtilshost.isBrowserstackInfra, trim/case, App Automate and TurboScale session base URL, and_printSessionURLguard tests (the guard test fails without the guard).BROWSERSTACK_ENVcases: recognised values, prod/unset unchanged, look-alike hosts rejected).eslintandtsc --noEmitare clean on the changed files. (An earlier local run showed 62 failures, the same set onmain. The cause was a localnpxwrapper injectingHTTPS_PROXY, which routes_fetchpast theglobal.fetchmocks. With it unset everything passes.)E2E on k8s staging (minimal wdio v9 + mocha sample, Chrome / Windows 11, local
npm packof this branch with #224 cherry-picked,SDK_CLI_BIN_PATH= binary from browserstack-binary#1889,BROWSERSTACK_STAGING_ENV=k8s)a015ac4b4b0bf58565f8cd6705e4af90ebd8d6bbc2572b70f2194af8ccfd3510c1ae7112bc0b601dunknown_gridbrowserstackapik8s.bsstag.com/automate/sessions/<id>.jsonstatus: done,name: ''(never marked)status: passed, name set by the SDK/ext/v1/builds/<uuid>/testRunssession_idnullc2572b70f2194af8ccfd3510c1ae7112bc0b601dintegrations: { unknown_grid: ... }integrations: { browserstack: { product: 'automate', session_id: 'c2572b70...' } }AFTER: build-start
POST https://sdk-k8s.bsstag.com, hubhub-k8s.bsstag.com,apis.automate.api = https://apik8s.bsstag.com,Marking session status for Automatewith no error. Automate builde3f7ad99bdc0b65b8689790823e96d4be0741aa1, TRA buildbwm8tumdau97lp716zzhukhcppuk470zskdpy16u.Related Jira task/s
SDK-6948
Companion PRs: browserstack/browserstack-node-agent#2655 (node reference), browserstack/browserstack-binary#1889 (binary
api<env>REST host). Dependency: #224 (A, BINARY_URL fallback; needs the 401 follow-up above). Overlaps: #56 / #65 are still open, unmerged PRs againstmain. They rewrite the same three lines with an ungatedincludes('bsstag')/endsWith('.bsstag.com')match. Whichever merges second will conflict, and resolving it by keeping both would bring back production-widebsstagmatching. This PR's gated version supersedes their util.ts change; closing them is the author's call.Release (mandatory for every PR — required for the
ready-for-reviewlabel)Version bump: (required — tick exactly one)
Release notes type: (optional)
Release notes (customer-facing): (optional but encouraged)
Release notes (internal): (required — engineer-facing; what actually changed / why)
BROWSERSTACK_STAGING_ENVset to a bare env name (e.g.k8s),*.bsstag.comhubs are now treated as BrowserStack, so session status marking and Automate <-> TRA session linking work on internal staging envs. Production behaviour is unchanged.service.tsnow uses the binary-supplied API host (APIUtils) instead of a hardcoded production host. For GRR-enabled accounts this is the regional api host the binary already uses to mark sessions. The classic (non-CLI) flow on a named staging env is not supported;_printSessionURLlogs a non-session reply at debug instead of throwing.BROWSERSTACK_STAGING_ENVis trimmed (blank means unset), andbsstaghosts are matched case-insensitively.Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.
🤖 Generated with Claude Code