Bidi browserstack executor http

[xxshubhamxx]

What is this about?

In WebDriver BiDi sessions, browser.execute() is dispatched over the BiDi socket (script.callFunction) instead of the classic W3C /execute/sync HTTP endpoint. BrowserStack's browserstack_executor: {...} commands are interpreted by the hub on that HTTP endpoint, so every executor call the service makes (session name/status, annotations, etc.) is silently swallowed when BiDi is enabled.

This PR makes the service route only those executor calls back over HTTP while leaving normal scripts on BiDi:

• _routeBidiExecutorToHttp(browser) — no-op unless browser.isBidi. On a BiDi browser it overwriteCommand('execute', ...), and when the script is a string starting with browserstack_executor: it delegates to browser.executeScript(script, args) (classic HTTP). Everything else falls through to the original execute.
• Wired up in before(): applied to each instance via getInstance(browserName) for multiremote, and to the single browser otherwise.
• The whole patch step is wrapped in try/catch — a failure logs a BStackLogger.warn and the session continues rather than breaking the user's test run.

Unit tests cover: the overwrite routing executor scripts to executeScript while passing normal scripts (and their args) through to the original execute; no overwrite on non-BiDi sessions; and per-instance overwrite in multiremote with no cross-instance leakage.

Files touched: packages/browserstack-service/src/service.ts, packages/browserstack-service/tests/service.test.ts.

Related Jira task/s

N/A — no Jira ticket linked. Originates from #118.

Release (mandatory for every PR — required for the ready-for-review label)

Version bump: (required — tick exactly one)

• minor (backwards-compatible feature)
• patch (bug fix or other small change)

Release notes type: (optional)

• New Feature
• Bug Fix
• Other Improvement

Release notes (customer-facing): (optional but encouraged)

• Fixed BrowserStack executor commands (session name, status, annotations) being ignored in WebDriver BiDi sessions.

Release notes (internal): (required — engineer-facing; what actually changed / why)

• browser.execute is overwritten on BiDi browsers so browserstack_executor: scripts are sent via executeScript (classic HTTP /execute/sync) instead of BiDi script.callFunction, which the hub does not intercept. Non-executor scripts still go through the original execute.
• Applied in before() per multiremote instance (getInstance) or to the single browser; skipped entirely when browser.isBidi is false.
• Patching is guarded by try/catch with a BStackLogger.warn so a failure degrades gracefully instead of failing the session.

Checklist

• Ready to review
• Has it been tested locally?

PR Validations

Run Tests: Comment RUN_TESTS to trigger sanity tests.

[github-actions]

🔴 SDK PR Review gate is red. Pending:

• The SDK PR Review Agent has not reviewed the current head commit yet — run the SDK PR Review Agent.

It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge.

[github-actions]

🔴 SDK PR Review gate is red. Pending:

• The SDK PR Review Agent has not reviewed the current head commit yet — run the SDK PR Review Agent.

It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge.

[xxshubhamxx]

SDK PR Review — 🔴 Fix 2 blocking issues

Reviewed b9bb4eb3 · 2 units · 5/5 changed regions judged · 0 coverage gap.

Blocking

1. packages/browserstack-service/src/service.ts:245-255 — Critical — Graceful degradation (SH-12)

The try/catch wraps the entire Object.keys(this._caps).forEach(...) loop, not each iteration. Array.prototype.forEach propagates a callback throw straight up, so if multiRemoteBrowser.getInstance(browserName) throws — or returns undefined, making browser.isBidi throw — for any one key, the loop aborts and every instance after the failing one never gets patched. The single warn at line 255 names neither the failing instance nor the skipped remainder, so a half-patched multiremote session is indistinguishable in the logs from a fully-failed one.

Per SH-12, a feature that cannot run must disable itself loudly and leave the test unaffected — never half-enable.

-            try {
-                if (this._browser.isMultiremote) {
-                    const multiRemoteBrowser = this._browser as unknown as WebdriverIO.MultiRemoteBrowser
-                    Object.keys(this._caps).forEach((browserName) => {
-                        this._routeBidiExecutorToHttp(multiRemoteBrowser.getInstance(browserName))
-                    })
-                } else {
-                    this._routeBidiExecutorToHttp(this._browser as WebdriverIO.Browser)
-                }
-            } catch (err) {
-                BStackLogger.warn(`Failed to patch execute for BiDi browserstack_executor routing; executor commands may not work in BiDi sessions: ${err}`)
-            }
+            const patch = (browser: WebdriverIO.Browser, label?: string) => {
+                try {
+                    this._routeBidiExecutorToHttp(browser)
+                } catch (err) {
+                    BStackLogger.warn(`Failed to patch execute for BiDi browserstack_executor routing${label ? ` on ${label}` : ''}; executor commands may not work in BiDi sessions: ${err}`)
+                }
+            }
+
+            if (this._browser.isMultiremote) {
+                const multiRemoteBrowser = this._browser as unknown as WebdriverIO.MultiRemoteBrowser
+                Object.keys(this._caps).forEach((browserName) => {
+                    patch(multiRemoteBrowser.getInstance(browserName), browserName)
+                })
+            } else {
+                patch(this._browser as WebdriverIO.Browser)
+            }

Note getInstance(...) is itself inside the callback, so the per-instance try has to wrap the getInstance call too — not just _routeBidiExecutorToHttp.

2. packages/browserstack-service/tests/service.test.ts (~L687, multiremote case) — Test coverage (DEF-12)

All three new tests are happy-path: single-browser BiDi patch, non-BiDi no-op, and a fully-successful 2-instance multiremote patch. None makes getInstance throw, so the catch branch — exactly where finding #1 lives — is never exercised. A test asserting that instance B is still patched when instance A throws would cover both the current bug and its fix.

Non-blocking

None. Both findings above survived a falsification pass.

Checked and cleared

• executeScript(script, args) vs execute(script, ...args) — correct, not a bug. executeScript takes an args array, not variadic, and the rest param already yields one. Matches the existing _executeCommand precedent (browser.executeScript(script, [])).
• Object.keys(this._caps) as the multiremote key source — follows the existing _executeCommand precedent, so not a novel choice. Worth noting _multiRemoteAction (service.ts:917) uses the more defensive multiRemoteBrowser.instances filtered by isBrowserstackCapability(cap); this change followed the less defensive of the two.
• Other executor call sites — _executeCommand (backing _setAnnotation and the session name/status paths) already calls executeScript directly and never routed through execute, so it was never affected by the BiDi issue. The fix is correctly scoped to the customer's own browser.execute('browserstack_executor:...') calls. Consistent, not a gap.
• Lifecycle placement / double-apply — wiring at the top of before() ahead of the sessionId block is the right point, and no double-apply path is reachable from this diff. Flagging only as a gut-check: overwriteCommand re-wraps rather than errors, so if some retry/reload path can re-fire before() against the same live browser instance, execute would get double-wrapped. No evidence such a path exists.

Per-file confidence

File	Verdict
.changeset/pr-119.md	✅ All clear
packages/browserstack-service/src/service.ts	🔴 Author to fix
packages/browserstack-service/tests/service.test.ts	🔴 Author to fix

[github-actions]

🔴 SDK PR Review gate is red. Pending:

• The SDK PR Review Agent reported blocking findings (🔴) on the current head commit — fix them and re-run the agent.

It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge.

[github-actions]

🔴 SDK PR Review gate is red. Pending:

• The SDK PR Review Agent has not reviewed the current head commit yet — run the SDK PR Review Agent.

It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge.

[xxshubhamxx]

SDK PR Review — ✅ GTG

Re-reviewed at 1deec858. Both blockers from the previous review are resolved; no new blocking issues.

Prior findings

1. Half-patched multiremote loop — RESOLVED. getInstance() now runs inside resolveBrowser(), invoked as the argument to this._routeBidiExecutorToHttp(resolveBrowser()) within patchBidiExecutorRouting's own try. Each forEach iteration catches independently, so one instance's failure no longer aborts the loop.

Confirmed non-cosmetic: reverting service.ts to the whole-loop-try shape while keeping the new test makes the callback throw uncaught on the browserA iteration, which per spec aborts the remaining iterations — browserB never gets patched and the test's final assertion fails.

2. Missing throw-path test — RESOLVED. should keep patching remaining multiremote instances when one instance fails to resolve throws via mockImplementationOnce on the first getInstance call only. With _caps = { browserA: {}, browserB: {} }, Object.keys preserves string-key insertion order, so the throw lands on browserA deterministically rather than by luck. Resolution and patching are synchronous per iteration, so there's no interleaving between keys.

Thunk approach

• getInstance is genuinely inside the try. The alternative of passing the already-resolved browser would have left it outside — correctly avoided.
• Single-browser path is semantically unchanged; it was covered by a broader catch before and an equivalent narrower one now.
• No this-binding issue — the helper and both thunks are arrow functions inside before(), so this stays lexically the service instance.

Non-blocking

service.ts:255 — Object.keys(this._caps) now sits outside any try, where the previous blanket try happened to cover it. If _caps were ever null/undefined this would throw uncaught in before(). Low risk: _caps is a required constructor param and the same unguarded call already appears at ~268, 296, 319, 372, 1021 and 1061 in this file. Optional to wrap; consistent with the file's existing convention as-is.

CI at this head

Lint ✅ · Build & test node 18.20 / 20 / 22 ✅ · CodeQL ✅ · Semgrep ✅

Per-file confidence

File	Verdict
.changeset/pr-119.md	✅ All clear
packages/browserstack-service/src/service.ts	✅ All clear
packages/browserstack-service/tests/service.test.ts	✅ All clear

[github-actions]

🟢 SDK PR Review gate is green — the SDK PR Review Agent has given a GTG for this PR (the ready-for-review label is present and the latest SDK PR Review Agent run reports success on the current head commit).

A native GitHub reviewer approval is still separately required by branch protection before this PR can merge — this check does not substitute for that.

[github-actions]

🟢 SDK PR Review gate is green — the SDK PR Review Agent has given a GTG for this PR (the ready-for-review label is present and the latest SDK PR Review Agent run reports success on the current head commit).

A native GitHub reviewer approval is still separately required by branch protection before this PR can merge — this check does not substitute for that.