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.

[07souravkunda]

This is the third copy of "is this an executor script?" in the package, and the only one with these semantics. The two existing ones are case-insensitive substring matches:

• packages/browserstack-service/src/accessibility-handler.ts:546-555 — script.toLowerCase().indexOf('browserstack_executor') !== -1
• packages/browserstack-service/src/cli/modules/accessibilityModule.ts:401-408 — same check, duplicated

Evidence / risk: a script those two already classify as an executor call (leading whitespace, or BROWSERSTACK_EXECUTOR:) is not matched by startsWith('browserstack_executor:') here. On a BiDi session it therefore still goes out over script.callFunction and is swallowed, while the identical script keeps working on non-BiDi — a BiDi-only behaviour divergence. I could not verify whether the hub itself tolerates those variants; that determines whether this is live today or only latent.

Fix: extract a single predicate and use it in all three places, e.g. in util.ts:

export const isBrowserstackExecutorScript = (script: unknown): script is string =>
    typeof script === 'string' && script.toLowerCase().includes('browserstack_executor')

Question: is the case-sensitive startsWith deliberate (i.e. the hub rejects the variants)? If not, reusing the existing predicate keeps BiDi and non-BiDi behaviour identical.

[07souravkunda]

This line ships to the public CHANGELOG, and two of the three things it names were not affected by the BiDi issue.

Evidence:

• Session name and status never used the executor — they go through the REST API: _updateJob (packages/browserstack-service/src/service.ts:881) → _update (:912), a PUT/PATCH to api.browserstack.com. BiDi cannot affect that path.
• The service's own annotate paths already use executeScript (classic HTTP /execute/sync), so they were never swallowed either: _executeCommand (service.ts:1018-1038), AccessibilityHandler._setAnnotation (accessibility-handler.ts:603), InsightsHandler (insights-handler.ts:139).

What this PR actually fixes:

1. User-written browser.execute('browserstack_executor: …') calls — the main win.
2. util.ts:2153 performO11ySync (reached via cli/modules/observabilityModule.ts:42 on the CLI/binary path).
3. cli/modules/accessibilityModule.ts:570 _setAnnotation (also CLI path).

Fix: reword to something like "Fixed browserstack_executor commands issued via browser.execute() being ignored in WebDriver BiDi sessions." Otherwise customers will attribute unrelated session-name/status problems to BiDi. The same wording appears in the PR description and the internal release notes.

[07souravkunda]

The guard checks isBidi but not whether this is a BrowserStack session, which diverges from every other executor path in the package:

• service.ts:1022 (_executeCommand) — isBrowserstackSession(this._browser)
• accessibility-handler.ts:602, util.ts:2154, cli/modules/accessibilityModule.ts:570 — same guard

The service does run against non-BrowserStack sessions (see the self-healing branch at service.ts:219, gated on !isBrowserstackSession), so on any non-BrowserStack BiDi session execute still gets overwritten and prefix-matched scripts get re-routed to /execute/sync.

Low impact in practice — nobody sends executor payloads to a non-BrowserStack grid — but it is a one-condition fix that keeps this consistent with the rest of the file:

if (!browser.isBidi || !isBrowserstackSession(browser)) {
    return
}

[07souravkunda]

executeAsync is routed over BiDi under exactly the same condition as execute, and is left unpatched here.

Evidence — webdriverio@9.28.0, build/index.js:3534-3538:

async function executeAsync(script, ...args) {
  ...
  if (this.isBidi && !this.isMultiremote) {   // same gate as execute() at :3509
    ...
    const result = await browser.scriptCallFunction(params);

No internal caller passes an executor payload to executeAsync, so this is a user-facing gap only — a user doing browser.executeAsync('browserstack_executor: …') still gets it silently swallowed on BiDi.

Question: intentionally out of scope, or worth mirroring the same overwrite for executeAsync (here or as a follow-up)? Either is fine — flagging so it is a decision rather than an omission.