fix(service): report screenshot logs on the binary flow (SDK-4177)

[anish353]

What is this about?

Screenshots taken during a Mocha run never reach Test Reporting's consolidated logs. Mocha is the only framework that takes the binary flow (CLISupportedFrameworks = ['mocha']), and three separate links in that path were broken. Jasmine and Cucumber stay on the Direct flow and were unaffected throughout.

O11Y stores screenshots as a TEST_LOG artifact on the test run — a gzipped JSONL manifest whose lines carry kind: "TEST_SCREENSHOT". On this path no manifest was ever built.

1. The producer never ran. InsightsHandler.browserCommand is the only producer of TEST_SCREENSHOT entries, and both of its call sites in service.ts sat inside if (!BrowserstackCLI.getInstance().isRunning()), after the CLI branch's own early return. So the WebDriver result event it is built from was never subscribed to on the binary path.

Fixed by subscribing result inside the CLI branch. command (beforeCommand) is deliberately left unsubscribed: it only fills the map that browserCommand's HTTP-log half reads, and that half emits on the JS listener pipeline the binary owns here. Leaving it out makes the screenshot the single effect, with no second code path to maintain.

2. The upload was unauthorized. With the producer running, listener.onScreenshot POSTs to the screenshot endpoint with Authorization: Bearer $BROWSERSTACK_TESTHUB_JWT — and that endpoint answers 401 to the binary's JWT:

DEBUG @wdio/browserstack-service [screenshot_upload] Failed. Error: ResponseError: Error response from server 401
ERROR @wdio/browserstack-service Error in executing browserCommand with args client:afterCommand,…: Error: Error response from server 401

On the binary path the binary owns reporting, so the entry now rides the same LOG rail appendTestItemLog already uses for console logs — trackEvent(TestFrameworkState.LOG, HookState.POST, { logEntry }) — which stamps the test uuid and forwards over gRPC (LogEntry carries kind, message, timestamp).

3. The rail relabelled it. WdioMochaTestFramework.loadLogEntries destructured only { level, message, timestamp } and then hardcoded logRecord.kind = KIND_LOG, discarding the producer's kind. The screenshot therefore arrived at O11Y labelled as a console log, and still no manifest was built. It now keeps an explicit kind and falls back to KIND_LOG — which is what the KIND_SCREENSHOT constant, defined in that same constants file and referenced nowhere in the repo until now, was for.

Console logs are byte-identical: StdLog.kind is already 'TEST_LOG', the same value KIND_LOG holds.

Two things that were not wrong, checked so they don't get re-investigated: identity is already correct on this path (setTestData seeds _tests[id] = { uuid: KEY_TEST_UUID }, which browserCommand reads), and the permission gate does open — the probe showed allow="true" in the worker. So this is not the node agent's SDK-4177 root cause, where logs were stamped with the Cucumber testCaseStartedId instead of the test-run uuid.

One incidental hardening: allow_screenshots is an optional string on the wire, so a denial arrives as the string 'false', which Boolean() read as permission granted. Now guarded with isFalse — the same defect class shouldProcessEventForTesthub already guards against ("Raw Boolean('false') is truthy, which kept events flowing…").

Related Jira task/s

SDK-4177

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 screenshots taken during a Mocha test not appearing in Test Reporting's consolidated logs.

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

• service.ts: subscribe the result event on the binary path so browserCommand, the only TEST_SCREENSHOT producer, actually runs there. command left unsubscribed on purpose so the HTTP-log half stays inert and the binary keeps ownership of the batch-event pipeline.
• insights-handler.ts: on the binary path send the screenshot through trackEvent(TestFrameworkState.LOG, …) instead of listener.onScreenshot — the direct screenshot endpoint 401s on the binary's JWT.
• wdioMochaTestFramework.ts: loadLogEntries no longer hardcodes kind = KIND_LOG; an entry that carries its own kind keeps it (falls back to KIND_LOG). This is what made the screenshot arrive labelled as a console log. Console logs unchanged — StdLog.kind is already 'TEST_LOG'.
• insights-handler.ts: TESTOPS_SCREENSHOT_ENV read with an explicit isFalse guard, since allow_screenshots is a proto optional string and 'false' passed a bare Boolean().

Checklist

• Ready to review
• Has it been tested locally?

Unit — lint clean, npm run build succeeds. New tests/cli/wdioMochaTestFramework.logKind.test.ts (3/3) pins the kind behaviour in both directions. tests/insights-handler.test.ts 79/79. Across tests/insights-handler.test.ts + tests/service.test.ts + tests/cli: 497 passed, 44 failed — all 44 pre-existing, verified by baselining the same files on clean main (identical failing-test sets: 40 in service.test.ts, 4 in cliUtils*). They are environmental here: a local proxy returns HTML for api.browserstack.com, so res.json() throws in _printSessionURL.

End to end, wdio_mocha automate sample on this branch, build xkphdxm0lhatqvje9kc4dd7byqdu5pi6nxf0dxqe:

Check	Before	After
kind on the gRPC wire	(no log emitted)	"kind":"TEST_SCREENSHOT" ×2 (one per platform)
/ext/v1/builds/{uuid}/testRuns → retries[].logs	[], []	['TEST_LOG'], ['TEST_LOG']
BStackAutomation validate_o11y_screenshot	False for both test ids	True for both — SCREENSHOT_FAILURES=0/2

The matching BStackAutomation PR that re-enables the assertion is browserstack/BStackAutomation#83444.

PR Validations

Run Tests: Comment RUN_TESTS to trigger sanity tests.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited)

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 892239ec-80a5-4978-93da-75920f0d346d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[anish353]

⚠️ Needs human review

File	Status	Reason
packages/browserstack-service/src/insights-handler.ts	🔴 Author to Fix 1	Receiving-side handling of the new KIND_SCREENSHOT LOG-rail entry on the binary is unconfirmed from this repo alone (ungrounded — needs a reviewer to check the cited TRA builds or the paired Binary PR). The prior review's other finding on this file (unguarded getTestFramework()!) is fixed and independently re-verified in this run.
packages/browserstack-service/tests/insights-handler.test.ts	✅ All Clear	Both new tests genuinely discriminate the fix — the framework-fallback test uses a faithful mock, and the denial test asserts against the real isFalse() implementation.

Change map (generated deterministically from the diff)

graph LR
  subgraph nwdio_service["wdio-service"]
    npackages_browserstack_service_tests_cli_wdioMochaTestFramework_logKind_test_ts["⚠ wdioMochaTestFramework.logKind.test.ts<br/>~69 lines"]
    npackages_browserstack_service_src_insights_handler_ts["insights-handler.ts<br/>~33 lines"]
    npackages_browserstack_service_tests_insights_handler_test_ts["insights-handler.test.ts<br/>~33 lines"]
    npackages_browserstack_service_src_service_ts["service.ts<br/>~23 lines"]
    npackages_browserstack_service_tests_service_test_ts["service.test.ts<br/>~23 lines"]
    npackages_browserstack_service_src_cli_frameworks_wdioMochaTestFramework_ts["wdioMochaTestFramework.ts<br/>~6 lines"]
    n_changeset_pr_190_md["pr-190.md<br/>~5 lines"]
  end

Loading

↻ This verdict comment is the review anchor — it's updated in place on each run (the gate posts its status separately).

— SDK PR Review Agent

[anish353]

Review response — finding 1 fixed in 0f825d0; findings 2 and 3 answered

Finding 1 — fixed, but the stated failure mode does not hold

Fixed in 0f825d0: the framework is now resolved rather than asserted, falling back to the direct upload when it is absent, plus a test covering isRunning() === true with no test framework.

The narrow point was right — getTestFramework() can be undefined while isRunning() is true, and the screenshot was then thrown away. But the consequence in the report is not reachable:

browserCommand itself has no try/catch … A thrown TypeError here propagates into the WDIO command hook and fails the customer's test

InsightsHandler is exported through o11yClassErrorHandler (insights-handler.ts:1249), which rewrites every prototype method — browserCommand included — wrapping it in try/catch and attaching .catch(error => processError(...)) when the method returns a promise (util.ts:266-284). So the class-level net the report says is not reused is applied to this method automatically; appendTestItemLog's inner try/catch is additive, not the only guard.

Observed, not inferred: before this branch was rerouted onto the LOG rail, listener.onScreenshot threw a 401 from inside browserCommand on a real run. It surfaced as ERROR @wdio/browserstack-service Error in executing browserCommand with args client:afterCommand,… and that same run finished Spec Files: 2 passed, 2 total. The throw was caught and the customer's test was unaffected — which is exactly the mechanism the report expected to be missing.

So: worth fixing because a swallowed throw means a lost screenshot, not because it breaks a test.

Finding 2 — resolved by the end-to-end evidence

The report allows for this directly:

The PR body's own end-to-end evidence table … is good signal this already works in practice; if that evidence is reproducible on demand, this can be marked resolved directly.

It is reproducible, and it has since been reproduced on CI rather than only locally. SDKWdioTest build 489 is SUCCESS on both matrix children against this branch:

• automate 7 passed / 0 failed; test_wdio_mocha_wrapper_default.py::test_o11y_consolidated_and_network_log passed
• app_automate: test_android_wdio_mocha_wrapper.py and test_ios_wdio_mocha_wrapper.py both passed
• TRA builds: automate · app_automate

validate_o11y_screenshot passes only when a TEST_LOG artifact exists on the test run and one of its manifest lines has kind == "TEST_SCREENSHOT". Against wdio main the same build shape had retries[].logs == []; with this branch it is ['TEST_LOG'] and the assertion returns True. The binary therefore does store a kind: 'TEST_SCREENSHOT' LOG-rail entry as a screenshot artifact — no companion Binary PR is needed.

Finding 3 — not a defect; '' is this file's convention

pass the same argument shape that call site uses

That is already what the new call does. shouldProcessEventForTesthub('') appears six times in service.ts — lines 123, 304, 328, 366, 382, 396 — and every one passes ''. There is no call site in this file that passes a lifecycle event name.

And the function cannot return false for the reason described (testHub/utils.ts:37):

if (isLoadTestingSession()) { return true }
if (isTrue(process.env[BROWSERSTACK_OBSERVABILITY])) { return true }
if (isTrue(process.env[BROWSERSTACK_ACCESSIBILITY])) { return !(['HookRunStarted','HookRunFinished','LogCreated'].includes(eventType)) }
if (isTrue(process.env[BROWSERSTACK_PERCY]) && eventType) { return false }
return isTrue(...ACCESSIBILITY) || isTrue(...OBSERVABILITY) || isTrue(...PERCY)

With observability on it returns true on line 2, before eventType is read at all. On the accessibility-only path '' is not in the exclusion list, so it also returns true. The one branch that does consult eventType (percy) treats a falsy value as "not a specific event" and falls through — so '' is the correct value for a gate that is not tied to a named lifecycle event.

No change made for this one. Happy to add a clarifying comment at the call site if a reviewer would still prefer it.

🤖 Generated with Claude Code

[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.