fix(observability): disable observability throughout the run when build-start is blocked

[kamal-kaur04]

What is this about?

When the Test Observability build-start returns success:false (e.g. an unsupported framework blocked at ingestion), observability was not fully disabled for the rest of the run. Two paths still advertised it:

1. Event gate (src/testHub/utils.ts) — shouldProcessEventForTesthub's fallback used raw Boolean(process.env[...]), so BROWSERSTACK_OBSERVABILITY='false' read as truthy and TestHub events kept flowing for an observability-only session. Now uses the isTrue(...) chain (consistent with the guards above it), so 'false' reads as off.

2. Automate session-start caps (src/launcher.ts, classic flow) — the session still carried buildProductMap.observability: true and a testhubBuildUuid, because getProductMap reads the static config and testhubBuildUuid was stamped from build_hashed_id (which ingest returns even on a block). This keeps the session counted as an SDK observability session even though no TestHub build was created. Now: the blocked outcome is folded into config so buildProductMap reports observability:false, and testhubBuildUuid is stamped only when a TestHub product (observability or accessibility) actually succeeded. Mirrors the existing accessibility fold-back; scoped to the classic flow (the CLI/gRPC flow owns its own build-start).

Verified end-to-end (serenity-js/webdriverio → prod, blocked as WebdriverIO-@serenity-js/webdriverio): before, the session caps had observability:true + a testhubBuildUuid; after, observability:false + no testhubBuildUuid, while the Automate build/session still ran.

Related Jira task/s

• https://browserstack.atlassian.net/browse/SDK-7030

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 Test Observability not being fully disabled for a run when the build could not be started (e.g. an unsupported framework) — such sessions no longer emit observability events or get linked to a non-existent build.

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

• shouldProcessEventForTesthub fallback now evaluates the product env flags with isTrue(...) instead of raw Boolean(...), so BROWSERSTACK_OBSERVABILITY='false' suppresses events (previously Boolean('false') was truthy).
• Classic-flow launcher now folds a blocked observability build-start into browserStackConfig.testObservability.enabled=false (so getProductMap/buildProductMap reports observability:false) and only stamps testhubBuildUuid when observability or accessibility succeeded — preventing orphan-linking the Automate session to a TestHub build that was never created. gRPC/CLI flow untouched.
• Added unit tests for both paths (tests/observability-build-start-success-false.test.ts, and an onPrepare case in tests/launcher.test.ts).

Checklist

• Ready to review
• Has it been tested locally?

PR Validations

Run Tests: Comment RUN_TESTS to trigger sanity tests.

[kamal-kaur04]

Test note: the new unit tests pass — tests/observability-build-start-success-false.test.ts and the new onPrepare case + all existing onPrepare caps tests in tests/launcher.test.ts. Two failures in tests/launcher.test.ts under _uploadApp ("throw SevereServiceError if upload fails" and one sibling) are pre-existing on main — a fetch-mock/env issue unrelated to this change (verified by reverting these edits and re-running: they still fail). Not introduced here.

[kamal-kaur04]

Claude Code Review

Verdict: ⚠️ Needs human review — 1 blocking finding needs human judgment: the guard's scope decision (CLI/gRPC exclusion + classic-flow request-failure handling) is a real code-traceable gap whose priority/scope is a team call, not something I can confirm clean or broken from the SDK diff alone.

Summary: 0 critical · 1 warning · 1 suggestion across 4 files reviewed.

The core fix is precise and well-targeted: isTrue(...) correctly replaces the truthy-string bug in shouldProcessEventForTesthub, and the testhubBuildUuid/buildProductMap fold-back in launcher.ts correctly distinguishes "observability blocked but accessibility succeeded" from "observability blocked, nothing else succeeded" — I traced both through the real (non-mocked) code paths, not just the tests. The one open question is whether the Boolean(buildStartResponse) scoping gate is airtight for every reason buildStartResponse can be falsy, not just the CLI/gRPC case it's commented to cover — see the inline note.

CI is green (Lint, CodeQL, Semgrep, Build & test on Node 18/20/22), corroborating the author's own local test-note comment.

See inline comments below for full detail.

Generated by Automated SDK PR review.

[kamal-kaur04]

Thanks for the review — both findings addressed in 6eebf94.

#1 (launcher.ts:447 — Boolean(buildStartResponse) conflated two falsy cases): the guard now keys on whether the classic build-start was attempted (classicBuildStartAttempted = !isRunning() && (testObservability || accessibility || percy)) rather than on buildStartResponse being truthy. So a transport/parse failure — where launchTestSession's error handler returns null and BROWSERSTACK_OBSERVABILITY is left unset (neither 'true' nor 'false') — now folds observability off too, closing the same gap for the failure path. The CLI/gRPC (mocha) flow stays excluded because isRunning() is true there. Added a dedicated transport-error unit test (onPrepare → launchTestSession returns null without setting the flag → buildProductMap.observability=false, no testhubBuildUuid).

#2 (test cleanup): moved restoration out of the test body into an afterEach in the onPrepare describe that resets BROWSERSTACK_OBSERVABILITY + the launchTestSession/getProductMap mocks, so per-test overrides can't leak into later tests even if an assertion throws first. The shared onPrepare mock now simulates a successful build-start (env 'true') to match real behavior.

Full launcher.test.ts + observability-build-start-success-false.test.ts are green except the two pre-existing _uploadApp failures (unrelated, fail on clean main).

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

[kamal-kaur04]

✅ Good to go

File	Status	Reason
packages/browserstack-service/src/launcher.ts	✅ All Clear	Both prior review findings verified as genuinely fixed; guard logic re-derived and checked for over-fire, no issues found
packages/browserstack-service/src/testHub/utils.ts	✅ All Clear	isTrue(...) fallback change reviewed, no issues found
packages/browserstack-service/tests/launcher.test.ts	✅ All Clear	afterEach restoration + shared mock change reviewed against pre-existing assertions in the same describe block, no masked regression found
packages/browserstack-service/tests/observability-build-start-success-false.test.ts	✅ All Clear	Documentation-only tests for the classic and CLI/gRPC flows, no issues found

↻ 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

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

[kamal-kaur04]

Claude Code Review — re-review (incremental)

Verdict: ✅ Good to go

Both findings from the prior round are verified as genuinely fixed on the current head commit (6eebf94):

• Finding #1 (launcher.ts) — the observability fold-back now keys on classicBuildStartAttempted (whether the classic build-start was attempted) instead of Boolean(buildStartResponse), so a transport/parse failure (where launchTestSession's error handler returns null) now also folds observability off. The Boolean(this._options.testObservability) term is unchanged, so accessibility-/percy-only sessions still don't over-fire. CLI/gRPC flow exclusion is unchanged.
• Finding #2 (launcher.test.ts) — mock/env restoration now runs in an afterEach, which fires even if an earlier assertion in the test throws; a new transport-error unit test exercises the exact gap finding #1 covered. The shared onPrepare mock's default change (env 'true') was checked against the pre-existing samsungGalaxy/testhubBuildUuid assertions later in the same describe block and the two "should launch testhub build" tests — no masked regression found.

CI is green (Build & test passing on Node 18.20, 20, 22). The src/cli/index.ts gRPC/binary-delegation event-emission gap remains out of scope for this PR by design (scoped in the PR body and code comments) and is now documented by a dedicated test — tracked as separate follow-up work, not a blocker here.

No new or remaining findings.

File	Status	Reason
packages/browserstack-service/src/launcher.ts	All Clear	Both prior findings verified fixed; guard re-derived, no over-fire
packages/browserstack-service/src/testHub/utils.ts	All Clear	isTrue(...) fallback change reviewed, no issues
packages/browserstack-service/tests/launcher.test.ts	All Clear	afterEach + shared mock change checked against existing assertions, no masked regression
packages/browserstack-service/tests/observability-build-start-success-false.test.ts	All Clear	Documentation-only tests, no issues

This is a recommendation only — no merge decision submitted; kamal is the human reviewer of record.

— SDK PR Review Agent

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

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