fix(SDK-4748): honour buildIdentifier precedence in _handleBuildIdentifier (v8)

[shivam5643]

What is this about?

_handleBuildIdentifier deleted an explicitly configured buildIdentifier whenever BROWSERSTACK_BUILD_NAME was present in the environment, and never consulted BROWSERSTACK_BUILD_RUN_IDENTIFIER at all. A user-set identifier silently vanished from the dashboard build name.

This is the WDIO half of SDK-4748. The python (browserstack/browserstack-python-sdk#1206, merged) and node (browserstack/browserstack-node-agent#2571) halves fix the equivalent defect in their SDKs.

The bug — in packages/browserstack-service/src/launcher.ts, the delete fired on an env-supplied build name:

if ((!this._buildName || process.env.BROWSERSTACK_BUILD_NAME) && this._buildIdentifier) {
    this._updateCaps(capabilities, 'buildIdentifier')   // deletes it
    return
}

Separately, BROWSERSTACK_BUILD_RUN_IDENTIFIER appeared only in telemetry payloads (src/util.ts, src/testorchestration/test-ordering-server.ts) — never as a source for buildIdentifier. BROWSERSTACK_BUILD_IDENTIFIER had zero references anywhere in the repo.

The fix — three changes, aligned with the precedence contract used across all BrowserStack SDKs (CLI args > env vars > config file > script):

1. Env tier added, ahead of service options / caps: BROWSERSTACK_BUILD_IDENTIFIER, then BROWSERSTACK_BUILD_RUN_IDENTIFIER. Blank/whitespace values ignored.
2. Delete gate narrowed to if (!this._buildName). The env-buildName clause is removed outright rather than narrowed: unlike python, this service has no default identifier (_handleBuildIdentifier early-returns when none is configured), so the clause had no legitimate job here. Python's equivalent suppresses only the default #${BUILD_NUMBER}, never an explicitly requested identifier.
3. Generic ${ENV_VAR} sweep after the existing ${DATE_TIME} / ${BUILD_NUMBER} substitutions, mirroring node's computeBuildIdentifier and python's substitute_env_vars_in_build_identifier. Unset vars stay literal, so no silent data loss.

this.browserStackConfig.buildIdentifier is now set alongside the caps update so funnel telemetry and launchTestSession see the resolved value.

Verification

BStackAutomation App-Automate wdio-cucumber lanes — 6 passed / 0 failed, run against this branch built and linked through BSA's own clone/build/link flow on Node 20.11.1. Clean before/after on the same two lanes:

	published service	this branch
test_build_identifier_custom_date	FAILED — build name ...customdate_hYBbAQ, no identifier	PASSED
test_build_identifier_env_variable	FAILED — build name ...envvariable_WKmZlI, no identifier	PASSED
test_sesion_run_status x2	FAILED	PASSED
test_app_automate_cbt x2	FAILED	PASSED

Identifiers exercised: custom_date: 2026-09-27_17-37-26 and BROWSERSTACK_BUILD_RUN_IDENTIFIER: test_run_20260927_174002, both of which reached the dashboard build name.

Unit tests: the _handleBuildIdentifier block goes from 7 to 17 cases, covering each precedence tier, blank-env handling, the absent-buildName path and the ${ENV_VAR} sweep. Full suite green (3 pre-existing unrelated ENOENT errors in uploadLogsArchive.test.ts, confirmed identical on a clean tree). tsc --noEmit and eslint clean.

One misnamed pre-existing test was replaced: 'should delete buildIdentifier if BROWSERSTACK_BUILD_NAME is defined as env var' asserted nothing about the env var — its caps carried no buildName, so it passed through the !this._buildName arm and duplicated the test above it.

⚠️ Required paired BStackAutomation change — please read before merging

This will turn a currently-green lane red in CI unless BSA is updated in the same window.

SDK/api/tests/automate/wdio_cucumber/wdio_cucumber_wrapper/test_wdio_cucumber_wrapper_build_identifier_null.py configures buildIdentifier: None and asserts assert not re.search(r'\d+$', fetched_build_name). BSA Jenkins injects BROWSERSTACK_BUILD_RUN_IDENTIFIER globally (observed as SDKWdioTestSharded-394). After this change the service honours it even when the config sets none, so the build name ends in digits and the assertion fails.

This is correct per the precedence contract — node, python and C# all let an env var beat an explicit yml null — so the remedy belongs in BSA: unset BROWSERSTACK_BUILD_RUN_IDENTIFIER BROWSERSTACK_BUILD_IDENTIFIER in those fixtures, the pattern already used in SDK/api/run_fixverify_6547.sh. The same shape exists in test_android_wdio_mocha_wrapper_build_identifier_null.py.

This is the v8 port of #236 (v9 / main). Same three changes; the only difference between the two _handleBuildIdentifier bodies after both land is the capabilities type (RemoteCapabilities here vs TestrunnerCapabilities on v9). v8 also needed afterEach added to the vitest import. Verification above was performed on the v9 branch; the v8 unit suite passes the same 17 _handleBuildIdentifier cases, and its full isolated run is 45/46 files — the one failure, tests/crash-reporter.test.ts, needs a prior npm run build and fails identically on a clean tree.

Related Jira task/s

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

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 a configured buildIdentifier being dropped from the build name when BROWSERSTACK_BUILD_NAME was set via environment variable.
• BROWSERSTACK_BUILD_RUN_IDENTIFIER and BROWSERSTACK_BUILD_IDENTIFIER are now honoured as build-identifier overrides.
• Placeholders such as ${CUSTOM_DATE} in buildIdentifier are now substituted from the environment.

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

• _handleBuildIdentifier (packages/browserstack-service/src/launcher.ts) reworked into an explicit precedence chain (env BROWSERSTACK_BUILD_IDENTIFIER -> BROWSERSTACK_BUILD_RUN_IDENTIFIER -> service options/caps), replacing the clause that deleted the identifier whenever BROWSERSTACK_BUILD_NAME was in env.
• Added a generic ${ENV_VAR} sweep after the existing ${DATE_TIME} / ${BUILD_NUMBER} substitutions; unresolved placeholders are left literal rather than blanked.
• Brings the service in line with node, python and C#, closing a cross-SDK precedence drift rather than widening it.

Checklist

• Ready to review
• Has it been tested locally?

PR Validations

Run Tests: Comment RUN_TESTS to trigger sanity tests.

[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: 9d565ef4-8f99-4347-9c12-62335ac49731

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

[shivam5643]

🔴 Blocking — 1 Critical, 2 Warnings, 2 Suggestions

Recommendation: REQUEST_CHANGES — posted as a COMMENT-event review (GitHub rejects a formal decision on a self-authored PR). Reviewed at head 4a49c0783f7321530abf5d0b966e79ae112bd9a4. Coverage ledger: 3/3 files judged, 0 gap.

Reviewed jointly with the v9 sibling #236. The two PRs were first reviewed independently, and each pass returned findings the other missed entirely — zero overlap, despite both reporting complete coverage. A diff-of-diffs confirms the src/launcher.ts changes are identical across the pair apart from Capabilities.RemoteCapabilities (v8) vs Capabilities.TestrunnerCapabilities (v9), and all 141 added test lines are byte-identical (this PR additionally adds afterEach to the vitest import, which v9's test file already had). Every finding below therefore applies to both PRs, and the same unioned set is posted on #236. Please fix in both.

🔴 Critical — env-sourced identifier leaks into the TestOps build-start payload after being suppressed

The new env tier sets the instance field unconditionally whenever either env var is present — including when nothing was configured anywhere:

const envBuildIdentifier = [
    process.env.BROWSERSTACK_BUILD_IDENTIFIER,
    process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER
].find((value) => value && value.trim())
if (envBuildIdentifier) {
    this._buildIdentifier = envBuildIdentifier.trim()
}

The if (!this._buildIdentifier) return guard below it now passes because the env tier just set it. Then the narrowed delete-gate strips the identifier from capabilities but never resets the field:

if (!this._buildName) {
    this._updateCaps(capabilities, 'buildIdentifier')
    BStackLogger.warn('Skipping buildIdentifier as buildName is not passed.')
    return
}

The caller then reads that same stale field and forwards it to launchTestSession, which puts it on the build-start POST as build_identifier: bsConfig.buildIdentifier.

The field-staleness itself pre-dates this PR, but it was only reachable if a user had explicitly configured a buildIdentifier. This PR makes it reachable with zero user configuration, because — as this PR's own description notes — CI injects BROWSERSTACK_BUILD_RUN_IDENTIFIER globally.

Failure scenario: a CI job that exports BROWSERSTACK_BUILD_RUN_IDENTIFIER runs a wdio suite with no buildName. The dashboard build name correctly shows no identifier — which is what the new tests check — while O11y's build-start record for that same build carries a non-empty build_identifier that was never applied anywhere visible. A dashboard/telemetry disagreement, on a path this PR turns from rare into common.

Suggested fix: clear the in-memory field in the !this._buildName branch before returning — this._buildIdentifier = undefined alongside the existing _updateCaps(...) call — so what reaches launchTestSession always matches what was actually applied to capabilities.

---

🟠 Warning — the generic ${ENV_VAR} sweep reprocesses the reserved ${BUILD_NUMBER} token

this._buildIdentifier = this._buildIdentifier.replace(
    /\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g,
    (match, varName) => process.env[varName] ?? match
)

This runs unconditionally after the ${BUILD_NUMBER} block, with no exclusion list. Previously, when getCiInfo() returned null and _getLocalBuildNumber() was falsy, ${BUILD_NUMBER} was left literal. Now the sweep matches that leftover token and substitutes raw process.env.BUILD_NUMBER if set — silently converting an unresolved placeholder into a real value, and one without the 'CI ' prefix that every other resolution path applies.

getCiInfo() recognizes a fixed vendor list that excludes GitHub Actions; TeamCity conventionally exports a bare BUILD_NUMBER with none of the markers it checks. So: identifier '#${BUILD_NUMBER}' on such a CI previously rendered literally, and now renders as #<raw value> in a format inconsistent with every getCiInfo()-resolved build.

This also puts an existing, untouched test at risk — the one that mocks both resolution paths to null and asserts the identifier stays literal. This PR's new afterEach clears only BROWSERSTACK_BUILD_NAME / BROWSERSTACK_BUILD_IDENTIFIER / BROWSERSTACK_BUILD_RUN_IDENTIFIER, not BUILD_NUMBER, so that test's guarantee depends on the ambient environment rather than on the code.

Suggested fix: exclude the already-special-cased tokens from the sweep, and add BUILD_NUMBER to the afterEach cleanup:

const RESERVED_BUILD_IDENTIFIER_TOKENS = new Set(['DATE_TIME', 'BUILD_NUMBER'])
this._buildIdentifier = this._buildIdentifier.replace(
    /\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g,
    (match, varName) => RESERVED_BUILD_IDENTIFIER_TOKENS.has(varName) ? match : (process.env[varName] ?? match)
)

---

🟠 Warning — the "buildName absent" test asserts only the caps side-effect, not the property its name promises

it('should not set buildIdentifier from env when buildName is absent', async() => {
    process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER = 'per-run-identifier'
    const caps: any = [{ 'bstack:options': {} }]
    const service = new BrowserstackLauncher(options as any, caps, config)

    service._handleBuildIdentifier(caps)
    expect(caps[0]['bstack:options']?.buildIdentifier).toBeUndefined()
})

The single assertion is satisfied by three different worlds: (a) the intended behavior — env read, then correctly stripped before the caps write; (b) the pre-PR code, which had no env-reading path at all and would pass this assertion unchanged; and (c) the Critical above, where the instance field keeps the env value and only the caps write is suppressed.

None of the new tests assert on service._buildIdentifier after the call, or on what would be forwarded downstream. So the one property this test is named for is the one property nothing here verifies — which is exactly why the Critical was able to ship.

Suggested fix: add expect((service as any)._buildIdentifier).toBeUndefined(), or spy on the launchTestSession boundary to confirm no stale identifier is forwarded.

---

💡 Suggestions

A blank-but-set env var silently blanks part of the identifier. The sweep's process.env[varName] ?? match guards only nullish, not empty string — so FOO='' erases that piece of the identifier. This contradicts the adjacent doc comment ("An unset variable is left as its literal placeholder rather than blanked, so nothing is silently lost") and is inconsistent with the tier-1 env check a few lines above, which correctly treats whitespace-only as absent via .find((value) => value && value.trim()). One-line fix: (match, varName) => { const v = process.env[varName]; return v && v.trim() ? v : match }.

No coverage for multiremote/object-form capabilities. Every new test uses array-form caps: any = [{ 'bstack:options': {...} }]. _handleBuildIdentifier and _updateCaps both have a distinct branch for the object/multiremote capabilities shape, which the new tiers never exercise. The two branches look structurally parallel so a latent divergence is unlikely, but it's untested — worth one mirrored case given this method is capability-format agnostic by design.

---

What's good

• The precedence chain is well-commented and explicitly cross-references the equivalent logic in node/python/C# — exactly the cross-SDK consistency this contract is meant to buy. Cross-checked against browserstack-python-sdk's substitute_env_vars_in_build_identifier(): identical ${VAR} regex and the same leave-unresolved-literal semantics.
• Strong, genuinely discriminating regression coverage for the actual SDK-4748 bug — separate tests for "keep identifier with BUILD_NAME env + buildName present" vs "still delete when buildName truly absent", correctly distinguishing the narrowed delete-gate from the previous over-broad one. Each of the four-tier precedence tests fails under the pre-fix code.
• Blank/whitespace-only env values correctly ignored at tier 1, and explicitly unit-tested.
• The env read uses plain process.env and doesn't depend on any CLI-vs-legacy config-normalization layer, so the SDK-7075 class of gap ("the legacy path reads no config env var") does not apply here.
• Changeset present, correctly scoped to @wdio/browserstack-service at patch, release notes accurate.
• Proactive, specific disclosure of the paired BStackAutomation break — exact failing assertion, exact injected env var, and the fixture pattern that fixes it.

---

Merge sequencing

This PR self-discloses that it turns a currently-green BSA lane red (test_wdio_cucumber_wrapper_build_identifier_null.py) because BSA Jenkins injects BROWSERSTACK_BUILD_RUN_IDENTIFIER globally and env outranks yml. The remedy chosen is correct — unset in the BSA fixture rather than weakening the precedence contract — but this PR shouldn't merge or release ahead of that BSA-side change landing.

🤖 Generated with Claude Code

[shivam5643]

Thanks — all four actionable findings are addressed. Pushed as a single commit on both PRs (079f4b8 on #236, 9850045 on #237).

🔴 Critical — stale identifier reaching the build-start payload

Confirmed and fixed. The !this._buildName branch stripped the capability but left this._buildIdentifier set, and launchTestSession forwards that field as build_identifier. The finding's characterisation is exactly right: the staleness pre-dated this PR, but the new env tier makes it reachable with no user configuration at all, which is a materially different exposure.

Both the instance field and browserStackConfig.buildIdentifier are now cleared alongside the _updateCaps call, so what reaches the build-start payload always matches what was applied to capabilities.

🟠 Warning — ${BUILD_NUMBER} reprocessed by the generic sweep

Confirmed and fixed. DATE_TIME and BUILD_NUMBER are now excluded via a module-level RESERVED_BUILD_IDENTIFIER_TOKENS set. The reasoning in the finding holds: when neither getCiInfo() nor _getLocalBuildNumber() can resolve it the token is left literal by design, and the sweep was silently converting that into a raw process.env.BUILD_NUMBER without the 'CI ' prefix every other resolution path applies.

BUILD_NUMBER is also added to the afterEach cleanup, so the pre-existing "stays literal" assertions no longer depend on the ambient environment.

🟠 Warning — the "buildName absent" test asserted only the caps side-effect

Confirmed and fixed. The test now also asserts expect((service as any)._buildIdentifier).toBeUndefined(). The observation that the assertion was satisfiable by the pre-PR code is what made this worth fixing rather than arguing — a test that passes against the unfixed code is not a regression test, and that gap is precisely why the Critical was able to ship.

💡 Suggestion — blank-but-set env var blanks part of the identifier

Fixed. ?? match guarded only nullish, so FOO='' erased that segment — contradicting the adjacent doc comment and inconsistent with the tier-1 check a few lines above. Empty and whitespace-only values now leave the literal placeholder.

💡 Suggestion — no multiremote/object-form coverage

Not addressed, deliberately. The object-form branch is untouched by this PR and the two branches are structurally parallel, so this is pre-existing coverage debt rather than a gap this change introduces. Happy to add it if you'd rather it land here, but it seemed better kept out of a fix PR.

---

Verification after the fix

• _handleBuildIdentifier block: 17 → 19 tests, all passing. The two new cases cover the reserved-token exclusion (with getCiInfo() and _getLocalBuildNumber() both mocked to null and a raw BUILD_NUMBER exported) and the empty-env case.
• v9 full suite: 1347 passed (3 errors pre-existing in uploadLogsArchive.test.ts, unchanged on a clean tree).
• v8: 45/46 files pass; the one failure, crash-reporter.test.ts, is the pre-existing Failed to load url ../build/constants.js that needs a prior npm run build, and reproduces on a clean tree. tsc --noEmit exit 0.

On the cross-PR note

Agreed, and acted on — the same commit is applied to both. Worth recording that two of these defects also exist outside this repo: browserstack-node-agent's computeBuildIdentifier runs its sweep after the ${BUILD_NUMBER} block with no exclusion and uses isUndefined(envValue) ? match : envValue, and browserstack-python-sdk's substitute_env_vars_in_build_identifier() has the same ordering and an is not None check. The Critical is wdio-specific (the others mutate a config object rather than an instance field). Those are being handled in their own repos.