Repository navigation
fix(cli): fall back to BROWSERSTACK_BINARY_URL when update_cli fails (SDK-6948) - #224
AakashHotchandani wants to merge 4 commits into
Conversation
…(SDK-6948)
update_cli runs during bootstrap — before the binary spawns and before GRR
localizes hosts — so it always targets the prod api host. On an internal staging
run (BROWSERSTACK_STAGING_ENV) the staging creds are rejected there with 401,
leaving the CLI binary path empty and crashing the launcher with
spawn('') -> ERR_INVALID_ARG_VALUE ('file' cannot be empty).
Wrap the update_cli call: on failure, if BROWSERSTACK_BINARY_URL is set, download
the binary from that URL and use it; otherwise re-throw (behaviour unchanged when
the var is unset). Adds unit tests for both paths. v9 port of the v8 fix (PR #183),
validated end-to-end against devapplca (update_cli 401 -> fallback download from a
served zip -> binary spawned -> build-start reached).
Co-Authored-By: Claude Opus 4.8 <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 |
…Y_URL fallback (SDK-6948)
requestToUpdateCLI resolved a 401 as a plain JSON body ({"message":"Unauthorized"}),
so the BROWSERSTACK_BINARY_URL fallback added earlier in this PR never ran: with an
empty CLI dir the bootstrap failed with "Failed to get CLI binary path", and with an
existing binary the stale one was reused silently.
- requestToUpdateCLI now rejects on !response.ok, carrying the status code and body.
- checkAndUpdateCli also falls back when a reply has neither url nor
updated_cli_version (prod always returns both on success).
- Without BROWSERSTACK_BINARY_URL a non-2xx reply still keeps the existing binary,
exactly as before; network errors still propagate.
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 Review
Verdict: spawn('') crash and claims the error "propagates exactly as before". On this line, start() already guards the empty path, and a non-2xx reply keeps the existing binary. Rewrite the CHANGELOG note to match v9. (2) The new 401/2xx tests mock global.fetch, but _fetch bypasses it under HTTP(S)_PROXY. Clear the proxy variables in beforeEach and assert that the mock was called, so the 2xx-error-body test cannot pass through the 401 branch.
Summary: 0 critical · 2 warnings · 4 suggestions across 3 files reviewed.
Intent: Make BROWSERSTACK_BINARY_URL a real bootstrap fallback when update_cli fails. update_cli always targets prod api.browserstack.com, so it rejects staging credentials with a 401. checkAndUpdateCli now wraps the call in a try/catch. With the URL set, it downloads the binary from that URL on any update_cli failure, or on a 2xx reply with neither url nor updated_cli_version. Without the URL, a non-2xx reply keeps the existing binary and other errors re-throw. requestToUpdateCLI now rejects on a non-2xx reply, because v9's fetch does not throw on 401.
The code change is sound. The no-URL path preserves behaviour, the fallback reuses the hardened download path, and graceful degradation holds. On this line, the ERR_INVALID_ARG_VALUE crash in the description was already prevented by the empty-path guard in start(). Both warnings are about accuracy: the CHANGELOG text, and tests that can pass for the wrong reason under a proxy environment.
Cross-SDK parity (#23)
| Aspect | wdio #224 | node #2655 | python #1282 |
|---|---|---|---|
| (a) Fallback gating | Ungated when URL set (network, any non-2xx, error-shaped 2xx) | Ungated | Ungated, and replaces a working cached binary |
| (a) No URL, HTTP-status error | Keeps existing binary ('' if none) |
— | — |
| (a) No URL, network/parse error | Re-throws -> setupCliPath null -> non-CLI flow |
— | — |
(b) Empty BROWSERSTACK_BINARY_URL |
Treated as unset (|| null) |
Treated as unset | Treated as set |
| (c) Busy-binary reuse | Kept: isBinaryBusy runs before update_cli, so the fallback never bypasses it |
Fallback skips it | Kept |
| (d) Vacuous tests | Hermetic only without a proxy env; under HTTP(S)_PROXY the 2xx test passes via the 401 branch |
— | 3 tests could never fail |
| (e) Span close | At most once; zero times on download rejection (no try/finally) |
try/finally, exactly once |
— |
Rovo enrichment unavailable — review based on local SDK docs only.
See inline comments below for full Problem and Suggested Fix detail on each finding.
Generated by Automated SDK PR review.
|
|
||
| fix(cli): fall back to `BROWSERSTACK_BINARY_URL` when `update_cli` fails (SDK-6948) | ||
|
|
||
| `update_cli` runs during bootstrap — before the binary is spawned and before GRR localizes the API hosts — so it always targets production `api.browserstack.com`. On an internal staging run (`BROWSERSTACK_STAGING_ENV`) the staging credentials are rejected there (`401`), which left the CLI binary path empty and crashed the launcher with `spawn('') … ERR_INVALID_ARG_VALUE: 'file' cannot be empty`. When `update_cli` fails and `BROWSERSTACK_BINARY_URL` is set, download the binary from that URL instead so the run can proceed. Opt-in only — with the variable unset the original error propagates exactly as before. |
There was a problem hiding this comment.
⚠️ Warning — [DOCS] Changeset describes the v8 symptom, not this package's behaviour
Problem
This changeset becomes the public CHANGELOG entry, and two of its statements are wrong for this package line:
- "…left the CLI binary path empty and crashed the launcher with
spawn('') … ERR_INVALID_ARG_VALUE". On this line,BrowserstackCLI.start()already guards the empty path before spawning (src/cli/index.ts:258-260):const SDK_CLI_BIN_PATH = await this.getCliBinPath() if (!SDK_CLI_BIN_PATH) { throw new Error('Failed to get CLI binary path. CLI setup failed.') }
bootstrap()catches that error (index.ts:100-105), and the run continues in the direct (non-CLI) flow. The PR body's own v9 "before" log confirms this:Resolved binary path:->Failed to get CLI binary path-> direct flow tohub-cloud. TheERR_INVALID_ARG_VALUEcrash and the "gotthrows" description belong to the v8 line (PR fix(util): recognize *.bsstag.com hosts as BrowserStack (v8, SDK-6948) #183). On this linerequestToUpdateCLIuses_fetch, which never threw on a 401. - "Opt-in only — with the variable unset the original error propagates exactly as before." For a non-2xx
update_clireply, nothing propagated before and nothing propagates now. The existing binary path is returned (cliUtils.ts:283-288), or''when there is none. Only network or parse errors propagate.
The note also omits a real contract change: CLIUtils.requestToUpdateCLI (a public static) now rejects on a non-2xx reply where it used to resolve the JSON body.
Suggested Fix
Rewrite the body to match v9:
fix(cli): fall back to `BROWSERSTACK_BINARY_URL` when `update_cli` fails (SDK-6948)
`update_cli` runs during bootstrap against production `api.browserstack.com`, so on an internal staging run (`BROWSERSTACK_STAGING_ENV`) it answers `401`. The SDK then found no CLI binary and silently fell back to the non-CLI flow, or reused a stale cached binary. When `update_cli` fails (non-2xx, network error, or a reply with neither `url` nor `updated_cli_version`) and `BROWSERSTACK_BINARY_URL` is set, the binary is now downloaded from that URL. `requestToUpdateCLI` now rejects on a non-2xx reply. With `BROWSERSTACK_BINARY_URL` unset, behaviour is unchanged: a non-2xx reply keeps the existing binary, and network errors propagate.Confidence: 🟢 Objectively verifiable: src/cli/index.ts:258-260 at head already throws on an empty path before spawn, and requestToUpdateCLI uses _fetch, not got.
There was a problem hiding this comment.
Fixed in 4eb68d2: rewrote the changeset in v9 terms. It now says a 401 led to the non-CLI flow (empty dir) or a silent stale binary, records the requestToUpdateCLI contract change (rejects on non-2xx), and says that with BROWSERSTACK_BINARY_URL unset a non-2xx keeps the existing binary and network errors propagate. I corrected the PR description Problem section the same way. The spawn('') crash is now attributed to v8 / #183.
| expect(download).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('falls back when a 2xx body has neither url nor updated_cli_version', async () => { |
There was a problem hiding this comment.
⚠️ Warning — [TESTING] New 401/2xx tests bypass their global.fetch mock whenever a proxy env var is set
Problem
The new checkAndUpdateCli update_cli 401 (SDK-6948) block mocks global.fetch, but requestToUpdateCLI calls _fetch from src/fetchWrapper.ts:
const proxyUrl = process.env.HTTP_PROXY || process.env.HTTPS_PROXY
if (proxyUrl) { ... return undiciFetch(request.url, { ..., dispatcher: new ProxyAgent(proxyUrl) }) }
return fetch(input, init)When either proxy variable is set (the PR body says a local npx wrapper injects HTTPS_PROXY), these tests send a real GET api.browserstack.com/.../update_cli with testuser:testkey. Prod answers that with a real 401, so:
falls back when a 2xx body has neither url nor updated_cli_versionstill passes, but through the 401catchbranch. The new 2xx error-body branch (cliUtils.ts:294-297) is never executed, so the test is vacuous in that environment (the same failure class as the python tests that could not fail).- The two 401 tests pass only because prod happens to return 401. If the proxy cannot reach the host, the "keeps existing binary" test fails, because the error has no
statusCodeand is re-thrown.
None of the tests asserts that the mock was hit, so nothing detects the bypass.
Suggested Fix
Make the block hermetic and prove the mock was reached:
const savedProxy = { http: process.env.HTTP_PROXY, https: process.env.HTTPS_PROXY }
beforeEach(() => {
delete process.env.HTTP_PROXY
delete process.env.HTTPS_PROXY
// ...existing mocks
})
afterEach(() => {
if (savedProxy.http !== undefined) { process.env.HTTP_PROXY = savedProxy.http }
if (savedProxy.https !== undefined) { process.env.HTTPS_PROXY = savedProxy.https }
// ...existing restore
})
// in each test
expect(global.fetch).toHaveBeenCalledTimes(1)Alternatively, vi.mock('../../src/fetchWrapper.js') and stub _fetch directly. Apply the same treatment to the requestToUpdateCLI describe block (the rejects with the status code on a non-2xx reply test at line 680).
Confidence: 🟢 Objectively verifiable: _fetch (src/fetchWrapper.ts:19-33) routes through undici ProxyAgent when HTTP_PROXY/HTTPS_PROXY is set, which skips global.fetch. The PR body confirms this.
There was a problem hiding this comment.
Fixed in 4eb68d2: both the update_cli 401 block and the requestToUpdateCLI block now delete HTTP_PROXY/HTTPS_PROXY in beforeEach (restored in afterEach), and each new test asserts expect(global.fetch).toHaveBeenCalledTimes(1). Verified hermetic: tests/cli/cliUtils.test.ts passes 47/47 both with the proxy vars unset and with HTTPS_PROXY=http://127.0.0.1:9 (dead proxy).
| // only: with no BROWSERSTACK_BINARY_URL behaviour is exactly as before. | ||
| const statusCode = (err as { response?: { statusCode?: number } })?.response?.statusCode | ||
| if (!isNullOrEmpty(browserStackBinaryUrl)) { | ||
| return fallbackToBinaryUrl(statusCode ?? (err as Error)?.message) |
There was a problem hiding this comment.
💡 Suggestion — [CORRECTNESS] Fallback is ungated: any update_cli failure swaps the binary when BROWSERSTACK_BINARY_URL is set
Problem
With BROWSERSTACK_BINARY_URL set, the fallback fires on every failure class: network error or timeout, prod 5xx, 429, 400, any 401/403, and any 2xx body missing both url and updated_cli_version. The fallback calls downloadLatestBinary(url, cliDir) without an expectedVersion. A custom URL carries no -x.y.z.zip version, so targetVersion is null, isBinaryAtVersion() returns false, and a working cached binary is re-downloaded and replaced on every transient prod blip. Node #2655 and Python #1282 behave the same way, so this matches them, but it is broader than the staging-401 problem the PR targets. Because the variable is opt-in and internal, the blast radius is limited.
Consider gating the fallback to the case it exists for, for example statusCode === 401 || statusCode === 403 || process.env.BROWSERSTACK_STAGING_ENV. Alternatively, on 5xx, 429 or network errors, prefer existingCliPath when one exists and only download when it is empty. If the broad behaviour is intended, record the decision in the PR body so the three SDKs stay aligned on purpose.
There was a problem hiding this comment.
Thanks. After a cross-SDK review we are keeping the broad behaviour on purpose, with no gating to 401/403. BROWSERSTACK_BINARY_URL is an opt-in internal "use this binary" override, and on the success path it already replaces the update_cli URL unconditionally (response.url = browserStackBinaryUrl). Falling back to it on any update_cli failure is therefore consistent, and it matches node-agent#2655 and python#1282. I recorded the decision in the PR body. Your re-download concern is addressed in 5c67d07, aligned with python: a 2xx reply with neither url nor updated_cli_version now keeps the cached binary and only downloads when none is cached. If the URL download itself fails, a cached binary is used instead of failing the CLI setup. Tests: keeps the cached binary when a 2xx body has neither url nor updated_cli_version and uses the cached binary when the fallback download fails.
| ) | ||
| const jsonResponse = await response.json() | ||
| logger.debug(`response ${JSON.stringify(jsonResponse)}`) | ||
| if (!response.ok) { |
There was a problem hiding this comment.
💡 Suggestion — [CORRECTNESS] response.json() runs before the !response.ok check; non-JSON error bodies lose the status code
Problem
requestToUpdateCLI parses the body (cliUtils.ts:472) before checking response.ok (:474). An HTML or empty error body (an LB 502/503/504) makes json() throw a SyntaxError with no response.statusCode. With BROWSERSTACK_BINARY_URL unset, that error is re-thrown, setupCliPath returns null, and the run drops to the non-CLI flow. A JSON-bodied 5xx instead keeps the existing binary. The same HTTP status therefore takes two different paths depending on body format. This is pre-existing for the no-URL case, but the new statusCode branch is meant to cover it.
Check ok first and parse defensively:
const body = await response.json().catch(() => null)
if (!response.ok) {
throw Object.assign(new Error(`update_cli request failed with status ${response.status}`),
{ response: { statusCode: response.status, body } })
}Also consider reusing the existing ResponseError from src/fetchWrapper.ts instead of a new ad-hoc { response: { statusCode } } shape. Note that ResponseError.response is a Response, so it exposes .status, not .statusCode.
There was a problem hiding this comment.
Partly fixed in 4eb68d2: requestToUpdateCLI now checks response.ok first and parses the error body defensively (response.json().catch(() => null)), so an HTML or empty 502/503/504 rejects with response.statusCode instead of a bare SyntaxError. The new test is keeps the status code when a non-2xx reply has a non-JSON body. I kept the { response: { statusCode, body } } shape rather than reusing ResponseError. ResponseError.response is a Response (it exposes .status), and its message consumes the body via res.text(). The fallback catch in checkAndUpdateCli and the first-commit tests read response.statusCode, so switching would change both for no behavioural gain.
| browserStackBinaryUrl as string, | ||
| cliDir, | ||
| ) | ||
| PerformanceTester.end(PerformanceEvents.SDK_CLI_CHECK_UPDATE) |
There was a problem hiding this comment.
💡 Suggestion — [OBSERVABILITY] SDK_CLI_CHECK_UPDATE span is never closed when the fallback download rejects
Problem
fallbackToBinaryUrl calls PerformanceTester.end(SDK_CLI_CHECK_UPDATE) only after await this.downloadLatestBinary(...) resolves. A bad or unreachable BROWSERSTACK_BINARY_URL, a zip without a binary-* entry, or a lock timeout makes the download reject, so the span is never ended. The span closes at most once on every path, but zero times on this one. The pre-existing updated_cli_version path at :311-316 has the same shape. Node #2655 wraps this in try/finally and is cleaner here.
Wrap the body of checkAndUpdateCli after PerformanceTester.start in try { ... } finally { PerformanceTester.end(...) } (or pass success=false and the error in a catch), and delete the scattered end() calls.
There was a problem hiding this comment.
Fixed in 4eb68d2: everything in checkAndUpdateCli after PerformanceTester.start(SDK_CLI_CHECK_UPDATE) now runs in a try { ... } finally { PerformanceTester.end(...) }, and the scattered end() calls are removed. The span closes exactly once on every path, including a rejected fallback download and the pre-existing updated_cli_version download. Test: propagates a rejected fallback download and still closes the span asserts end is called once.
| expect(download).toHaveBeenCalledWith('https://example.com/staging-binary.zip', mockCliDir) | ||
| }) | ||
|
|
||
| it('keeps the existing binary on a 401 when BROWSERSTACK_BINARY_URL is unset (unchanged behaviour)', async () => { |
There was a problem hiding this comment.
💡 Suggestion — [TESTING] Branch coverage gaps: no cached binary, download rejection, empty BROWSERSTACK_BINARY_URL
Problem
The tests cover fallback-on-reject, re-throw without the URL, 401 with the URL, 401 without the URL (with a cached binary), and the 2xx error body. Three branches are still untested:
- Prod transient failure with no cached binary and no URL. A non-2xx reply with
existingCliPath = ''returns''. This is the prod-customer case behind the empty-path concern. Assert thatcheckAndUpdateCli('', …)resolves'', and thatsetupCliPaththen returns a falsy value sostart()takes theFailed to get CLI binary pathexit into the non-CLI flow. - Fallback download rejects. Assert that the error propagates to
setupCliPath, which returnsnull, and that the span is closed (see the span suggestion). BROWSERSTACK_BINARY_URL=''.|| nullmakes it count as unset, which matches node and differs from python. Pin that with a test so the three SDKs don't drift further.
There was a problem hiding this comment.
Fixed in 4eb68d2, with three new tests in the update_cli 401 block. (1) returns an empty path on a 401 with no cached binary and no BROWSERSTACK_BINARY_URL: checkAndUpdateCli('', ...) resolves ''. setupCliPath passes that through, and start() then takes the existing Failed to get CLI binary path exit; that part is unchanged code. (2) propagates a rejected fallback download and still closes the span: the rejection propagates, and the existing setupCliPath test returns null when an error occurs during setup covers the null mapping. (3) treats an empty BROWSERSTACK_BINARY_URL as unset: this pins today's || null behaviour (matches node). Whether python should align is a cross-SDK call that I have raised separately.
- requestToUpdateCLI checks response.ok before parsing, so an HTML/empty error page keeps its status code instead of surfacing as a SyntaxError. - checkAndUpdateCli closes the SDK_CLI_CHECK_UPDATE span once in a finally, including when the fallback download rejects. - Tests: clear HTTP(S)_PROXY so _fetch cannot bypass the global.fetch mock, assert the mock was hit, and cover no-cached-binary 401, rejected fallback download, empty BROWSERSTACK_BINARY_URL and a non-JSON 5xx. - Changeset rewritten for the v9 behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lback download (SDK-6948) - A 2xx update_cli reply with neither url nor updated_cli_version keeps the cached binary; BROWSERSTACK_BINARY_URL is only fetched when none is cached (python #1282 parity). - If the BROWSERSTACK_BINARY_URL download fails and a cached binary exists, use it instead of failing the CLI setup. - Tests for both, plus the no-cached-binary 2xx fallback. 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): f3694d8 → 5c67d07
Verdict: ✅ Good to go. Both prior warnings are resolved at head. Three 💡 suggestions below are non-blocking.
Rovo enrichment unavailable. This review is based on local SDK docs only.
═══════════════════════════════════════════════════════════════
Prior findings (review 5453029954)
| # | Finding | Ruling | Evidence at 5c67d07 |
|---|---|---|---|
| a | spawn('') crash |
RESOLVED | .changeset/sdk-6948-binary-url-fallback.md:7 now describes the v9 behaviour: non-CLI flow or a silent stale binary. It records the requestToUpdateCLI reject-on-non-2xx contract change and the no-URL behaviour. |
| b | global.fetch mock under HTTP(S)_PROXY |
RESOLVED | tests/cli/cliUtils.test.ts:505-510,524-525 and :690-696,706-707 delete and restore both proxy vars. Every fetch-driven test asserts expect(global.fetch).toHaveBeenCalledTimes(1) (:542, :553, :569, :585, :606, :762, :775). The 2xx error-body test now runs on the 2xx branch. |
| c | 💡 Fallback is ungated | DECLINED-ACCEPTABLE | The decision is recorded in the PR body, and it matches node#2655 and python#1282. The re-download concern is reduced: a 2xx reply with neither url nor updated_cli_version keeps the cache (cliUtils.ts:298), and a failed URL download falls back to the cached binary (cliUtils.ts:264-272). One residual remains: with the URL set, a 401 still re-downloads on every run. That is acceptable for an opt-in internal variable. |
| d | 💡 json() was parsed before the ok check |
RESOLVED (the ResponseError reuse part is DECLINED-ACCEPTABLE) |
cliUtils.ts:476-485 checks ok first and uses .catch(() => null). The new test is keeps the status code when a non-2xx reply has a non-JSON body. The author is right that ResponseError.response exposes .status and consumes the body, so keeping the existing shape is sound. |
| e | 💡 Span left open when the download rejects | RESOLVED | cliUtils.ts:231 / :322-324 close the span in try/finally, and the scattered end() calls are removed. Test at :609 asserts one end(SDK_CLI_CHECK_UPDATE). |
| f | 💡 Coverage gaps | RESOLVED | Tests at :598 (no cache, no URL → ''), :609 (download rejection propagates) and :619 (empty URL treated as unset). |
═══════════════════════════════════════════════════════════════
New commits (4eb68d2, 5c67d07)
0 critical · 0 warnings · 3 suggestions
| # | Finding | File |
|---|---|---|
| 1 | 💡 [TESTING] uses the cached binary when the fallback download fails never asserts that the download ran |
tests/cli/cliUtils.test.ts:588 |
| 2 | 💡 [DOCS] New comment says a 2xx reply without url/updated_cli_version means "nothing to update". The PR body says prod always returns both. |
src/cli/cliUtils.ts:296 |
| 3 | 💡 [DOCS] Changeset says behaviour is "unchanged" without the URL, but a non-JSON non-2xx reply now keeps the binary instead of throwing | .changeset/sdk-6948-binary-url-fallback.md:7 |
Details are in the inline comments.
═══════════════════════════════════════════════════════════════
Anti-pattern checks
- #5 error boundaries: pass. Every failure in
checkAndUpdateClieither returns the cached binary or'', or it throws into thesetupCliPathcatch, which returnsnull.start()then takes the existingFailed to get CLI binary pathexit into the non-CLI flow, so the user's tests never break. - #8 build verification: not applicable. This package ships
tscoutput without obfuscation. CIBuild & testpassed on node 18.20, 20 and 22, andLintpassed at 5c67d07. - #23 parallel paths: pass. Only the CLI bootstrap calls
update_cli. The worker branch (cliUtils.ts:216-226, JWT set and no binary) falls through to the same fallback. The cross-SDK behaviour matches node#2655 and python#1282: an ungated fallback, cache kept on a 2xx reply with neither field, cache used when the URL download fails, and an empty URL treated as unset (python differs on the empty URL; the author has raised that separately). - #26 unreachable fallbacks: pass. Each new branch is reachable and tested:
json().catchon an HTML 5xx, the download-failure cache fallback, and the 2xx-neither-field branch. Finding 2 is about how that last branch is described, not whether it can be reached.
═══════════════════════════════════════════════════════════════
Full-diff regression check
The PR diff touches only checkAndUpdateCli (:227-325) and requestToUpdateCLI (:461-489), plus the changeset and tests.
- The busy-binary short-circuits still run before
update_cli, inside the newtry/finally. - Success-path URL substitution (
:308-313) is unchanged. - A
return awaitinside thetrymakes the span end only after the download settles. utilis already imported (:5).
No regressions found.
═══════════════════════════════════════════════════════════════
Per-file confidence
| File | Status | Reason |
|---|---|---|
.changeset/sdk-6948-binary-url-fallback.md |
🟢 All Clear | Prior warning resolved. One wording suggestion. |
packages/browserstack-service/src/cli/cliUtils.ts |
🟢 All Clear | Covered by sdk-anti-patterns #5, #23 and #26. One comment-accuracy suggestion. |
packages/browserstack-service/tests/cli/cliUtils.test.ts |
🟢 All Clear | Prior warning resolved; tests are hermetic and assert the mock was hit. One assertion suggestion. |
✅ Good to go. Suggestions 1-3 are non-blocking.
Generated by Automated SDK PR review.
| expect(global.fetch).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('uses the cached binary when the fallback download fails', async () => { |
There was a problem hiding this comment.
💡 Suggestion: [TESTING] This test never shows the fallback download ran
This test proves the result is mockExistingPath. That is also what checkAndUpdateCli would return if a regression skipped the fallback for a 401 when a binary is cached (for example, gating the fallback on isNullOrEmpty(existingCliPath)). The test fails on 4eb68d2, because the rejection propagated there, so it is not vacuous today. It does not, however, pin the path its name describes. Unlike the other tests in this block, it also does not assert that global.fetch was hit.
Suggested Fix: capture the spy and assert that the attempt happened:
const download = vi.spyOn(CLIUtils, 'downloadLatestBinary').mockRejectedValue(new Error('download failed'))
// ...
expect(download).toHaveBeenCalledWith('https://example.com/staging-binary.zip', mockCliDir)
expect(global.fetch).toHaveBeenCalledTimes(1)| throw err | ||
| } | ||
|
|
||
| // A 2xx reply with neither `updated_cli_version` nor `url` means there is nothing to update: |
There was a problem hiding this comment.
💡 Suggestion: [DOCS] The comment's reading of a 2xx reply with neither field contradicts the PR's own evidence
The comment says a 2xx reply with neither updated_cli_version nor url "means there is nothing to update". The PR body says the opposite. It reports that prod update_cli returns both updated_cli_version and url for cli_version 0, 1.58.5 and 99.0.0, so an up-to-date client still gets both fields. This branch is therefore a guard for an unexpected reply shape (for example {success:true} or an error-shaped 2xx), not the up-to-date reply. The behaviour itself is fine and matches python#1282: keep the cache, and download only when nothing is cached.
Suggested Fix: reword the comment, for example: // A 2xx reply with neither field is not a normal update_cli reply: keep a cached binary, and only fetch from BROWSERSTACK_BINARY_URL when there is none.
|
|
||
| fix(cli): fall back to `BROWSERSTACK_BINARY_URL` when `update_cli` fails (SDK-6948) | ||
|
|
||
| `update_cli` runs during bootstrap against production `api.browserstack.com`, so on an internal staging run (`BROWSERSTACK_STAGING_ENV`) it answers `401`. The SDK then found no CLI binary and fell back to the non-CLI flow, or silently reused a stale cached binary. When `BROWSERSTACK_BINARY_URL` is set and `update_cli` fails (non-2xx or network error), the binary is now downloaded from that URL. If that download fails, a cached binary is used when there is one. A 2xx reply with neither `url` nor `updated_cli_version` keeps the cached binary, and only downloads from the URL when none is cached. `CLIUtils.requestToUpdateCLI` now rejects on a non-2xx reply (with `response.statusCode`) instead of resolving the error body. With `BROWSERSTACK_BINARY_URL` unset, behaviour is unchanged: a non-2xx reply keeps the existing binary, and network errors propagate. |
There was a problem hiding this comment.
💡 Suggestion: [DOCS] "Behaviour is unchanged" is slightly too broad without the URL
With BROWSERSTACK_BINARY_URL unset, a non-2xx reply with a non-JSON body (for example an HTML 502/503 from the load balancer) used to surface as a SyntaxError. setupCliPath then returned null, and the run used the non-CLI flow. Now requestToUpdateCLI attaches statusCode (cliUtils.ts:476-485), so checkAndUpdateCli returns the cached binary (:288-291) and the CLI flow continues. This affects prod customers, and it is an improvement, but the changeset says the behaviour did not change.
Suggested Fix: add a short clause, for example: "…a non-2xx reply (including a non-JSON error page) keeps the existing binary, and network errors propagate."
- util: treat *.bsstag.com as BrowserStack only on a named staging env
(BROWSERSTACK_STAGING_ENV bare token, trimmed; or BROWSERSTACK_ENV
staging/stag/preprod/pre-prod), exact suffix match, case-insensitive.
Replaces the unconditional `includes('bsstag')` match.
- cli: BROWSERSTACK_BINARY_URL fallback also covers a 2xx update_cli reply
with no url and no cached binary; a failed fallback download uses the
cached binary when present; the update_cli error is logged; the perf span
closes on every path.
- service: _printSessionURL logs a failed or empty session lookup at debug
instead of throwing (v8 uses got, which throws on 404/401).
- tests: bsstag gating, fallback download failures, 2xx-without-url,
_printSessionURL 404 / missing automation_session.
Addresses review comments 4256353854 and 4256353859.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Problem
update_cliruns during bootstrap, before the CLI binary is spawned and before GRR localizes the API hosts, so it always targets productionapi.browserstack.com. On an internal staging run (BROWSERSTACK_STAGING_ENV) the staging credentials are rejected there with a401. On this (v9) line,requestToUpdateCLIused_fetchand resolved the 401 body ({"message":"Unauthorized"}) without throwing. That gave two outcomes:Resolved binary path:->start()throwsFailed to get CLI binary path(src/cli/index.ts) ->bootstrap()catches it -> the run continues in the direct (non-CLI) flow againsthub-cloud.browserstack.com.(The
spawn('')/ERR_INVALID_ARG_VALUEcrash and "gotthrows" belong to the v8 line, PR #183.)Fix
Wrap the
update_clicall in try/catch. On failure, ifBROWSERSTACK_BINARY_URLis set, download the binary from that URL and use it; otherwise re-throw (behaviour unchanged when the var is unset). This turnsBROWSERSTACK_BINARY_URLfrom a download-source override (only read after a successfulupdate_cli) into a real bootstrap fallback, so a worker can pull a binary from a URL even when the stagingapi-<env>/update_clitier is unreachable.Follow-up commit f3694d8 (wdio v9 specifics). On v9,
requestToUpdateCLIusesfetchand returnsresponse.json(). It does not throw on a 401: it resolves{"message":"Unauthorized"}, so thecatchabove never ran. With an empty CLI dir the bootstrap still failed withFailed to get CLI binary path. With an existing binary, the stale one was reused silently. The follow-up does three things:requestToUpdateCLInow rejects on!response.ok, withresponse.statusCodeand the body attached.checkAndUpdateClialso falls back when a reply has neitherurlnorupdated_cli_version. A successful prod reply always carries both; checked against produpdate_cliforcli_version0, 1.58.5 and 99.0.0, and all three returnupdated_cli_version+url.BROWSERSTACK_BINARY_URL, a non-2xx reply still keeps the existing binary, exactly as before. Network errors still propagate.Fallback scope (cross-SDK decision): with
BROWSERSTACK_BINARY_URLset, anyupdate_clifailure (non-2xx, network error) falls back to that URL, deliberately not gated to 401/403. The variable is an opt-in internal "use this binary" override; on the success path it already replaces theupdate_cliURL unconditionally. This matches node-agent#2655 and python#1282. Follow-up 5c67d07 (python #1282 parity): a 2xx reply with neitherurlnorupdated_cli_versionkeeps the cached binary and downloads from the URL only when none is cached. If the URL download itself fails and a cached binary exists, the cached one is used.Opt-in / zero default impact: with no
BROWSERSTACK_BINARY_URL, a non-2xx reply still keeps the existing binary (or''when there is none), and network errors still propagate, the same as before. Contract change:CLIUtils.requestToUpdateCLInow rejects on a non-2xx reply (withresponse.statusCode) instead of resolving the error body.Tests
requestToUpdateCLIrejects with the URL set, and the error is re-thrown without it.requestToUpdateCLIrejects withstatusCode: 401on a 401 JSON reply.urlnorupdated_cli_versiontriggers the fallback.okis checked before parsing (non-JSON 5xx keeps its status); the span closes in afinally; the tests clearHTTP(S)_PROXYand assert thefetchmock was hit; new cases cover a 401 with no cached binary, a rejected fallback download, an emptyBROWSERSTACK_BINARY_URL, and a non-JSON 502.keeps the cached binary when a 2xx body has neither url nor updated_cli_version,falls back ... and no binary is cached, anduses the cached binary when the fallback download fails(the first and last fail on 4eb68d2 source).tests/cli/cliUtils.test.ts(47) also passes with a deadHTTPS_PROXYset, so those tests are hermetic. Lint andtsc --noEmitare clean.requestToUpdateCLItests that "fail on main" only fail locally. A localnpxwrapper injectsHTTPS_PROXY, which makes_fetchroute through undici'sProxyAgentand bypass theglobal.fetchmock. With the proxy variables unset they pass, onmainand here.Validation
wdio v9, k8s staging, after the follow-up commit:
npm packof this branch,BROWSERSTACK_STAGING_ENV=k8s, an emptyBROWSERSTACK_FILES_DIR,BROWSERSTACK_BINARY_URL=https://127.0.0.1:8766/binary.zip(the browserstack-binary#1889 build, served locally).54833a27e4a834686d8af1e64143b426c8a0b2ccran onhub-k8s.bsstag.comand the spec passed.response {"message":"Unauthorized"}->Resolved binary path:->Failed to get CLI binary path-> direct flow tohub-cloud.browserstack.com->Invalid username or password.Earlier validation on the v8 line (identical code path, PR #183): a prod-mode run with
BROWSERSTACK_STAGING_ENV=devapplca+BROWSERSTACK_BINARY_URL->update_cli401 -> fallback -> binary downloaded -> spawned -> build-start reached.Follow-up (separate): properly localize
update_cliviaBROWSERSTACK_STAGING_ENV(port of ruby-sdk SDK-6583) + bring up the stagingapi-<env>tier. This fallback is the production-safe bridge until then.🤖 Generated with Claude Code