SDK 7414 wdio cucumber platformisation

[AdityaHirapara]

What is this about?

Related Jira task/s

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)

• WebdriverIO + CucumberJS now runs on the BrowserStack CLI flow, the same path Mocha already uses. Reporting, accessibility, Percy and session naming behave as before — no config change is needed.
• Fixed: sessions were left unmarked pass/fail when setSessionName: false was set. Naming and status are independent options again.
• Fixed: the accessibility extension was not applied on non-BrowserStack infrastructure, leaving scans empty on otherwise green runs.

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

• Adds WdioCucumberTestFramework, driving TestFrameworkState from cucumber's own feature/scenario/step hooks rather than Mocha's test model, and adds cucumber to CLISupportedFrameworks.
• Session verdict correctness on the cucumber path: any failing scenario fails the session regardless of order; a failing build-level hook fails a session with zero scenarios; ignoreHooksStatus is honoured; the results map is keyed on the scenario uuid so Scenario Outline rows sharing a pickle name no longer collapse last-write-wins.
• preferScenarioName moves into automateModule and is recorded above the skipSessionStatus opt-out, so naming is not coupled to status. Added to NOT_ALLOWED_KEYS_IN_CAPS.
• Shared-code fixes reaching Mocha on the CLI flow (disclosed as pre-existing): driver registration when every product is off; status marking under skipSessionName; goog:chromeOptions arriving JSON-encoded over gRPC.
• Accessibility: hook-window scan gate narrowed to Mocha, matching legacy; cucumber scan filtering by gherkin tag via the 6-arg shouldScanTestForAccessibility.
• bdd_meta_info.feature.path relativised to match legacy.

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: 855d18a5-4dd5-4f63-84f6-7390251f3c1a

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

[AdityaHirapara]

RUN_TESTS

[AdityaHirapara]

⚠️ Needs human review

See the SDK PR Review Agent's report from your local run.

Process integrity

The review gate could NOT certify this run — the verdict is held at ⚠️ pending regardless of findings:

• G6 — 1 kb proposal(s) but no proposal PR recorded (kb-pr.txt) — run pr-kb-proposal.sh

Change map (generated deterministically from the diff)

graph LR
  subgraph nwdio_service["wdio-service"]
    npackages_browserstack_service_src_cli_frameworks_wdioCucumberTestFramework_ts["wdioCucumberTestFramework.ts<br/>~645 lines"]
    npackages_browserstack_service_tests_cli_modules_automateModule_test_ts["automateModule.test.ts<br/>~390 lines"]
    npackages_browserstack_service_tests_service_test_ts["service.test.ts<br/>~325 lines"]
    npackages_browserstack_service_src_cli_modules_automateModule_ts["automateModule.ts<br/>~222 lines"]
    npackages_browserstack_service_src_service_ts["service.ts<br/>~213 lines"]
    npackages_browserstack_service_src_cli_modules_customTagsModule_ts["customTagsModule.ts<br/>~61 lines"]
    npackages_browserstack_service_tests_cli_modules_customTagsModule_test_ts["customTagsModule.test.ts<br/>~61 lines"]
    npackages_browserstack_service_tests_cli_wdioCucumberTestFramework_test_ts["wdioCucumberTestFramework.test.ts<br/>~60 lines"]
    npackages_browserstack_service_src_cli_modules_accessibilityModule_ts["accessibilityModule.ts<br/>~42 lines"]
    npackages_browserstack_service_tests_cli_modules_accessibilityModule_test_ts["accessibilityModule.test.ts<br/>~42 lines"]
    npackages_browserstack_service_src_scripts_accessibility_scripts_ts["accessibility-scripts.ts<br/>~40 lines"]
    npackages_browserstack_service_tests_accessibility_scripts_test_ts["accessibility-scripts.test.ts<br/>~40 lines"]
    npackages_browserstack_service_src_cli_index_ts["index.ts<br/>~11 lines"]
    npackages_browserstack_service_tests_skipAppOverride_test_ts["⚠ skipAppOverride.test.ts<br/>~4 lines"]
    npackages_browserstack_service_src_cli_cliUtils_ts["cliUtils.ts<br/>~2 lines"]
    npackages_browserstack_service_src_constants_ts["constants.ts<br/>~2 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

[AdityaHirapara]

Automated SDK PR Review

Verdict: 🔴 Fix 16 blocking issues

Summary: 5 critical · 11 warnings · 0 suggestions across 15 files reviewed.

The verdict hinges on three defects in automateModule.ts that make the cucumber platformisation incomplete rather than incorrect: preferScenarioName has no producer on the CLI flow so the feature is dead, the scenario bookkeeping sits below the skipSessionStatus early return so naming got coupled to status, and fullName is the raw pickle name so Scenario Outline rows still collapse last-write-wins. Two findings are 🟡 ungrounded and want a maintainer's confirmation rather than a blind fix.

See inline comments below for full Problem and Suggested Fix detail on each finding.

Generated by Automated SDK PR review.

— SDK PR Review Agent

[AdityaHirapara]

🔴 Critical — [CORRECTNESS] Scenario bookkeeping sits below the skipSessionStatus return, so setSessionStatus: false also disables scenario naming

Problem

The preferScenarioName decision reads sessionData.scenariosRan / lastScenarioName, but the only place those are written is the block added in onAfterTest (hunk 6) — and that block sits after this early return, which was already in onAfterTest:

if (testContextOptions.skipSessionStatus) {
    this.logger.info('Skipping session status update as per configuration')
    return
}

So with setSessionStatus: false, scenariosRan stays 0 and lastScenarioName stays undefined for every scenario, and this condition can never be true — the user silently loses the scenario-based session name because they opted out of session status.

That inverts the invariant this PR itself states two hunks earlier (hunk 5): "Registration is independent of both opt-outs: setSessionStatus: false must still get the name repair". Naming and status are independent options everywhere else in this file (flushSessionName gates on skipSessionName alone; markSessionStatus gates on skipSessionStatus alone); this is the one place the two get coupled, and it is a new coupling introduced by where the recording block was placed.

This is independent of the finding on hunk 6: even once args.preferScenarioName is populated, setSessionStatus: false users would still not get the behaviour.

Suggested Fix

Move the scenario bookkeeping in onAfterTest above the skipSessionStatus early return, so it runs regardless of the status opt-out — it feeds naming, not status:

const sessionDataForName = this.sessionMap.get(sessionId)
if (sessionDataForName && !skipped && this.isCucumberInstance(instace)) {
    sessionDataForName.scenariosRan++
    sessionDataForName.lastScenarioName = testTitle
    sessionDataForName.preferScenarioName = <populated source>   // see hunk 6 finding
}

if (testContextOptions.skipSessionStatus) { ... return }

leaving only testResults.set(resultKey, testResult) below the return. A test covering setSessionStatus: false + preferScenarioName: true on a single-scenario feature would pin the independence.

Confidence: 🟢 packs/default.md DEF-13 (unreachable branch on a real path); the inputs are written after onAfterTest's skipSessionStatus early return, so they stay 0/undefined whenever that option is set

— SDK PR Review Agent

[AdityaHirapara]

Confirmed and fixed in 343cc78.

The scenario bookkeeping now sits above the skipSessionStatus early return, so it runs regardless of the status opt-out:

// Scenario bookkeeping feeds the session NAME, not its status, so it sits above the status
// opt-out: `setSessionStatus: false` must still get the preferScenarioName rename.
const isCucumber = this.isCucumberInstance(instace)
if (!skipped && isCucumber) {
    const nameData = this.sessionMap.get(sessionId)
    if (nameData) {
        nameData.scenariosRan++
        nameData.lastScenarioName = testTitle
        nameData.preferScenarioName = isTrue(args.preferScenarioName)
    }
}

You were right that this inverted the file's own stated invariant — naming and status are independent everywhere else here, and placing the recording block below the return was what coupled them.

Added renames even when setSessionStatus is false, and falsified it: moving the block back below the return makes it fail.

[AdityaHirapara]

🔴 Critical — [FEATURE CORRECTNESS] Scenario Outline rows still collapse in the session-result map — fullName is the raw pickle name, which repeats across Examples

Problem

This hunk fixes the feature-title collapse by re-keying sessionData.testResults on test.fullName, but the only producer of that field on the cucumber path is _cucumberTestView() in src/service.ts, which sets it to world.pickle?.name with no Examples qualifier. A Scenario Outline whose title carries no <placeholder> yields the identical pickle name for every Examples row, so N rows of one outline still write to a single map entry under last-write-wins — a failing row followed by a passing row leaves testResults holding only 'passed', and onAfterExecute marks the Automate session passed. That is the same defect this hunk's own comment describes, merely narrowed from feature scope to outline scope. Neither unit's reviewer can see it: the u005 reviewer sees an opaque test.fullName and no way to know it repeats, and the u009 reviewer sees fullName set for automateModule's SHAPE key (else if (test && !test.fullName)), with nothing in service.ts indicating the value is also used as a map key. Only u002 — a third unit — carries the evidence that the raw name is non-unique by design. The same value also flows to lastScenarioName for preferScenarioName, where an outline with one Examples row is named without its qualifier while Observability reports the qualified name.

Suggested Fix

Make the cucumber-side result key as unique as the row it represents. Either set fullName in _cucumberTestView() to the Examples-qualified name the framework already computes (pickle.name + ' (' + getScenarioExamples(world).join(', ') + ')' when examples exist) — safe for naming, since automateModule only tests fullName for truthiness and never uses its value to build the session name — or, better, carry the scenario's KEY_TEST_UUID on the event and key testResults on that, so the map is keyed on scenario identity rather than on a display string.

Confidence: 🟢 The key's producer and its consumer are in different units, and the proof that the key is not unique is in a third. u009 (src/service.ts, _cucumberTestView) sets fullName: world.pickle?.name ?? '' — the RAW pickle name. u005 (src/cli/modules/automateModule.ts, onAfterTest) adopts that exact value as the uniqueness key of sessionData.testResults. u002 (src/cli/frameworks/wdioCucumberTestFramework.ts, loadScenarioData) establishes in this same PR that the raw pickle name is deliberately shared across Examples rows — it builds qualifiedName = pickle.name + ' (' + examples.join(', ') + ')' for KEY_TEST_NAME/KEY_TEST_SCOPE and keeps the raw name only on KEY_TEST_ID precisely so the dashboard can GROUP Scenario Outline rows under it.

— SDK PR Review Agent

[AdityaHirapara]

Confirmed and fixed in 343cc78 — and it was a real parity break, not just a residual.

What legacy did: afterScenario accumulates into a flat array, appended once per failing scenario —

this._failReasons.push(exception)   // service.ts:1043

— and the verdict is a length check (service.ts:782):

const hasReasons = this._failReasons.length > 0
sessionStatus = hasReasons ? 'failed' : 'passed'

No key, so legacy could never collapse. A failing Examples row followed by a passing one still reports failed there, while the CLI flow reported passed. Both keys tried were non-unique for cucumber: name is the Feature title, and fullName is the raw pickle name shared across Examples rows of an outline whose title carries no placeholder.

Fix: cucumber now keys on the scenario's own KEY_TEST_UUID.

One thing worth recording — the fix is gated on isCucumberInstance rather than applied unconditionally, because wdioMochaTestFramework.ts:200 also sets KEY_TEST_UUID. An ungated change would have silently altered mocha's keying too. Mocha's path is byte-identical.

Added keeps both rows of a Scenario Outline that share a name, falsified against the old key.

[AdityaHirapara]

🔴 Critical — [CORRECTNESS] args.preferScenarioName is never populated, so the preferScenarioName feature is dead on the CLI flow

Problem

onAfterTest records the user's preferScenarioName choice off the hook event args:

sessionData.preferScenarioName = isTrue(args.preferScenarioName)

but nothing ever puts preferScenarioName into those args. The TestFrameworkState.TEST / HookState.POST producers in service.ts send a fixed payload — { test, result: results, suiteTitle } (service.ts:653) and the cucumber-path equivalent (service.ts:1056) — and a repo-wide search for the key finds it in only three places: constants.ts:45 (NOT_ALLOWED_KEYS_IN_CAPS), types.ts:201 (the option's type) and service.ts:723-728 (the legacy, non-CLI path that destructures it from this._options). Contrast the sibling flag in this same PR: ignoreHooksStatus is explicitly ridden onto the event at service.ts:507-511 (ignoreHooksStatus: this._options.testObservabilityOptions?.ignoreHooksStatus === true) precisely so the module can read it off args. preferScenarioName got no such rider.

Consequence on every path: args.preferScenarioName is undefined, isTrue(undefined) is false, so sessionData.preferScenarioName is always false, and the block added in hunk 8 —

if (sessionData.preferScenarioName && sessionData.scenariosRan === 1 && sessionData.lastScenarioName) {

— can never be entered. The feature is dead on the CLI/binary flow while it still works on the legacy flow (service.ts:723-728), i.e. a silent behaviour regression for any cucumber user with preferScenarioName: true once the binary is up: the session keeps the Feature title instead of the single scenario's name, with no error and a green exit. scenariosRan / lastScenarioName are still maintained, so the defect is exactly the one missing input.

Secondary, same hunk (worth fixing in the same pass, OB-02): the new resultKey = test.fullName removes the Feature-title collapse, but cucumber-js gives every example row of a Scenario Outline the same pickle name when the scenario title carries no <placeholder>, so those rows still share one fullName and still collapse last-write-wins — a failing row followed by a passing row will report the session passed. Adding a distinguishing component (the test uuid already in TestFrameworkConstants.KEY_TEST_UUID, or an incrementing index) closes the residual.

Suggested Fix

Give preferScenarioName a producer, mirroring the ignoreHooksStatus rider added in this same PR. In service.ts, at both TestFrameworkState.TEST / HookState.POST trackEvent sites (service.ts:653 and the cucumber site at service.ts:1056):

await BrowserstackCLI.getInstance().getTestFramework()!.trackEvent(TestFrameworkState.TEST, HookState.POST, {
    test,
    result: results,
    suiteTitle: this._suiteTitle,
    preferScenarioName: this._options.preferScenarioName === true
})

Alternatively read it where the module already reads user options — this.config / testContextOptions — instead of from args, if preferScenarioName is propagated into the CLI module config; that removes the cross-file coupling entirely. Either way, add a test asserting a single-scenario cucumber run on the CLI flow ends up named after the scenario, since a green run proves nothing here (DEF-12).

For the secondary point, key the results map on something unique per execution, e.g. String(test.fullName) + '::' + TestFramework.getState(instace, TestFrameworkConstants.KEY_TEST_UUID) with the same mocha fallback.

Confidence: 🟢 packs/default.md DEF-13 (a branch that cannot be reached); verified by repo-wide search: no trackEvent producer sets preferScenarioName on the TEST/POST args (service.ts:653, :1056), unlike ignoreHooksStatus at service.ts:507-511

— SDK PR Review Agent

[AdityaHirapara]

Not valid — the rider exists. service.ts:1062, inside the very trackEvent call this comment cites as service.ts:1056:

await cliFramework.trackEvent(TestFrameworkState.TEST, HookState.POST, {
    world,
    test: this._cucumberTestView(world),
    suiteTitle: this._suiteTitle,
    result: this._cucumberTestResult(world),
    ignoreHooksStatus: this._options.testObservabilityOptions?.ignoreHooksStatus === true,
    preferScenarioName: this._options.preferScenarioName === true,   // <- line 1062
})

So args.preferScenarioName is populated on the cucumber TEST/POST path and the block is reachable. The repo-wide search that found it in "only three places" missed this one, six lines into the call it names.

Two things in the comment were right, though, and both are now fixed in 343cc78:

• The secondary point — the outline rows still collapsing under fullName — was correct, and is addressed on the sibling thread.
• The feature genuinely was dead for anyone running setSessionStatus: false, for the unrelated reason on the :362 thread: the bookkeeping sat below the status opt-out, so scenariosRan never incremented. Right symptom, different cause.

[AdityaHirapara]

🔴 Critical — [TEST COVERAGE] A test that cannot fail, plus a block that only passes because earlier describes seeded the mocks

Problem

TS-01 — does not count a skipped scenario is unfalsifiable.

register(mod, 'Login Feature', { preferScenarioName: true })
await mod.onAfterExecute()
expect(namesPUT()).not.toContain('Can log in')

register seeds scenariosRan: 0 and no lastScenarioName, and the test never drives a scenario — skipped or otherwise — through onAfterTest. The string 'Can log in' is therefore never in play anywhere on this path: it is neither seeded, nor produced, nor reachable. The assertion holds for every possible implementation of the production code, including one that renames unconditionally (it would send undefined, still not 'Can log in'). It is the sibling of the two tests above it, where 'Can log in' IS seeded and the assertion is meaningful.

It also does not test what its name says: nothing in it represents a skipped scenario. The real production guard is sessionData.scenariosRan++ being gated on the scenario not being skipped (automateModule.ts, the onAfterTest cucumber arm) — that gate is never exercised here.

TS-03 (second defect in this region) — AutomateModule — session marking depends on an earlier describe's beforeEach.

Anchor line: + const runScenario = (mod: AutomateModule, passed: boolean) => mod.onAfterTest({

onAfterTest returns early unless isBrowserstackSession(browser) is truthy (automateModule.ts:178). This describe's beforeEach mocks getTrackedInstance, getState, TestFramework.getState and fetch — but not isBrowserstackSession. The only reason keeps the session PASSED under ignoreHooksStatus once a scenario has run passes is that vi.mocked(isBrowserstackSession).mockReturnValue(true) was set by the previous describe's beforeEach, and vi.clearAllMocks() clears calls but not implementations — so the value leaks forward across describes.

Run that describe in isolation (it.only, vitest -t 'ignoreHooksStatus', a future file split, or turning on mockReset/restoreMocks in the vitest config) and isBrowserstackSession returns undefined: runScenario no-ops, scenariosRan stays 0, the assertion flips to the !_specsRan arm and the test reports failed instead of passed. The pair of ignoreHooksStatus tests — the one place the suite discriminates 'a scenario ran' from 'none ran' — is exactly the pair this leak silently breaks. The same shape recurs in keys on the scenario for cucumber and leaves the key unchanged for mocha, which calls vi.clearAllMocks() mid-test and re-establishes four mocks but relies on AutomationFramework.getTrackedInstance's implementation surviving.

Suggested Fix

TS-01: make the test drive the case it names and assert something that can fail, e.g.

it('does not count a skipped scenario', async () => {
    const mod = newModule()
    register(mod, 'Login Feature', { preferScenarioName: true })
    await mod.onAfterTest({ /* the skipped-scenario shape onAfterTest receives */ })

    await mod.onAfterExecute()

    expect(namesPUT()).not.toContain('<the skipped scenario title>')
    expect(namesPUT()).toContain('Login Feature')
})

Seeding lastScenarioName (so the negative assertion has a real target) and adding the positive toContain('Login Feature') is the minimum; the two tests above it already do exactly that.

TS-03: add vi.mocked(isBrowserstackSession).mockReturnValue(true) to the AutomateModule — session marking beforeEach so the block is self-contained. Each new describe should re-establish every mock its code path reads rather than inheriting them; the added blocks are already close to this — isBrowserstackSession is the one omission.

Confidence: 🟢 packs/tests.md TS-01 (assertion that cannot fail; test name claims a behaviour its assertions do not check) and TS-03 (test relies on ordering relative to another test)

— SDK PR Review Agent

[AdityaHirapara]

TS-01 — confirmed, fixed in 343cc78. You're right that it could not fail: register seeded no lastScenarioName, nothing drove a scenario, so 'Can log in' was never in play and the assertion held for any implementation.

It now drives an actually-skipped scenario through onAfterTest, and is paired with the non-skipped case — which is what gives it the ability to fail:

it('does not count a skipped scenario', ...)   // runScenario(mod, { skipped: true })
it('counts a scenario that actually ran', ...) // runScenario(mod)

The describe also now sets its own isBrowserstackSession and TestFramework.getState mocks rather than inheriting them.

TS-03 — the fragility is real, the mechanism isn't. Running that describe in isolation would not yield undefined: the vi.mock factory declares isBrowserstackSession: vi.fn(() => true), and clearAllMocks() clears calls but not implementations, so the factory default applies and the test still passes. What is true is the implicit dependency — the describe doesn't establish the mock itself and relies on an earlier mockReturnValue(true) surviving. That would bite if a later edit changed the last setter to false or reordered the describes. Worth tightening, but it is not currently load-bearing in the way described.

[AdityaHirapara]

🔴 Critical — [MAINTAINABILITY] bdd-meta path assertion compares against a forward-slash literal, so it is platform-dependent

Problem

RELATIVE_URI is the literal 'features/checkout.feature', and both ABSOLUTE_URI and the value under test are derived through node:path:

• ABSOLUTE_URI = path.resolve(process.cwd(), RELATIVE_URI)
• the implementation's featureUriForMeta() is path.relative(process.cwd(), this.featurePath()), and featurePath() is path.resolve(process.cwd(), uri)

path.relative returns platform-native separators. On POSIX the round-trip gives back features/checkout.feature and the assertion holds; on Windows it gives features\checkout.feature, so expect(meta.feature.path).toBe(RELATIVE_URI) fails even though the production behaviour is exactly right. The same latent mismatch sits in the third test's expect(meta.feature.path).not.toBe(framework['featurePath']()) only benignly (it would still pass), so the hard failure is isolated to this one comparison.

The other two assertions in the file are separator-agnostic (path.isAbsolute, and toBe(ABSOLUTE_URI) where both sides come from path.resolve), which is why this is a single-line issue rather than a file-wide one.

I could not establish within the tool budget whether this repo's CI matrix includes a Windows runner — the package's vitest.config.ts and package.json say nothing about platforms. If the suite only ever runs on Linux/macOS the assertion never bites, and this is then a portability wart rather than a break; the fix is cheap enough either way that it is worth taking rather than depending on the runner OS.

Suggested Fix

Compare against a path built the same way the implementation builds it, so the expectation is separator-neutral:

const RELATIVE_URI = path.join('features', 'checkout.feature')
const ABSOLUTE_URI = path.resolve(process.cwd(), RELATIVE_URI)

path.join yields features/checkout.feature on POSIX and features\checkout.feature on Windows, which is precisely what path.relative(process.cwd(), ABSOLUTE_URI) returns on each platform — so the assertion keeps its full regression-catching strength on every runner. (If the intent is specifically to prove the value carries no drive/home prefix, expect(path.isAbsolute(meta.feature.path as string)).toBe(false) — already present on the next line — is the separator-free half and can stay as-is.)

Confidence: 🟢 Verifiable from the code under test: featureUriForMeta() returns path.relative(process.cwd(), path.resolve(process.cwd(), uri)), and path.relative emits platform-native separators, while the expected value is a hardcoded forward-slash literal.

— SDK PR Review Agent

[AdityaHirapara]

Valid observation, not being fixed in this PR.

path.relative does return platform-native separators, so toBe('features/checkout.feature') would fail on Windows while the production behaviour is correct. The analysis is right, including that the other two assertions in the file are separator-agnostic.

Leaving it because the suite runs on POSIX CI, so it cannot fail today, and this PR's scope is the cucumber flow migration rather than test portability. If Windows CI is ever added this is a one-line change (split(path.sep).join('/') on the value under test, or comparing against path.join('features', 'checkout.feature')).

[AdityaHirapara]

⚠️ Warning — [GRACEFUL DEGRADATION] Malformed chrome options are dropped with no diagnostic at all

Problem

Both drop paths in toChromeOptions are completely silent:

• catch { return null } discards the JSON.parse error without recording it anywhere.
• The final return null discards any value that parsed but is not a plain object (a JSON array, "null", a bare number/string).

When either fires, update() leaves this.ChromeExtension at its previous value ({} on a fresh process), and launcher.ts then applies an empty overrideOptions — so on non-BrowserStack infra the accessibility extension is never injected, the scan produces nothing, and the run still exits 0. That is the silent-accessibility-failure shape OB-05 points at: nothing in the SDK logs says the capability payload was rejected, so the only symptom a customer or a support engineer sees is an empty a11y report with no cause anywhere in the debug log.

DEF-02 and SH-11 both require the pair: don't break the test (this change satisfies that half), and don't be silent about it (this half is missing). The DEF-02 NOT exemption covers a boundary that "logs at debug and is explicitly documented as the degradation point" — the JSDoc documents the drop, but nothing logs it, so the exemption does not apply.

Note this is a new silent path, not pre-existing behaviour: before this change a string value was assigned through (wrongly, which is the bug being fixed); the decision to discard it instead is introduced here, so the diagnostic belongs here too.

Suggested Fix

Log both drop paths at debug before returning null, including the type and a truncated value so the payload shape is recoverable from a debug log:

function toChromeOptions(value: unknown): { [key: string]: unknown } | null {
    let parsed = value
    if (typeof parsed === 'string') {
        try {
            parsed = JSON.parse(parsed)
        } catch (err) {
            BStackLogger.debug(`Ignoring non-JSON nonBStackInfraA11yChromeOptions: ${util.format(err)}`)
            return null
        }
    }
    if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) {
        return parsed as { [key: string]: unknown }
    }
    BStackLogger.debug(`Ignoring nonBStackInfraA11yChromeOptions of type ${typeof parsed}`)
    return null
}

(Use whichever logger this module already has in scope — BStackLogger.debug is the repo convention; I did not spend a read confirming it is already imported in this file, so wire it to the existing import if one is present rather than adding a new logging dependency.) Keeping the return values unchanged means no behavioural risk: the test added in this PR for the drop case still passes, and the degradation stays non-fatal.

Confidence: 🟢 packs/default.md DEF-02 (an error boundary must not swallow the signal) + cards/_shared.md SH-11/SH-12 (degradation must be loud at debug, never silent); packs/observability.md OB-05 cites failure-patterns/accessibility-zero-scan.md as this exact class

— SDK PR Review Agent

[AdityaHirapara]

Fixed in bc28ca6 — both drop paths now log before returning null, and behaviour is unchanged:

} catch (err) {
    // Dropping this leaves the extension uninjected and the a11y report empty, with the
    // run still green — so the drop has to be findable in the log.
    BStackLogger.debug(`toChromeOptions: goog:chromeOptions is not valid JSON, dropping it: ${err}`)
    return null
}
...
BStackLogger.debug(`toChromeOptions: goog:chromeOptions resolved to ${Array.isArray(parsed) ? 'an array' : typeof parsed}, not an object; dropping it`)
return null

Your scoping point is the one that settled it: this is a new silent path, not pre-existing behaviour — before the change a string was assigned straight through, and the decision to discard it instead is introduced here, so the diagnostic belongs here too.

[AdityaHirapara]

⚠️ Warning — [CORRECTNESS] ignoreHooksStatus now asks a scenario-less question — hasStepFailures() vs legacy hasTestStepFailures(world)

Problem

_cucumberTestResult() re-implements the legacy ignoreHooksStatus exemption that the
untouched code a few lines above in afterScenario() still performs — but it asks a different
question, and I could not settle from this unit whether the two agree.

Legacy (unchanged, same method, runs on both flows):

const hasTestStepFailures = this._insightsHandler.hasTestStepFailures(world)

— scenario-scoped: it is handed world and answers "did a test step of this scenario fail?".

Added (CLI flow, feeds automateModule's session marking):

const hasStepFailures = this._cliCucumberFramework()?.hasStepFailures() ?? true

— no argument at all. Whether it answers the same scenario-scoped question depends entirely on
WdioCucumberTestFramework's internal tally (fed by the onStepStart/onStepEnd bookkeeping added
in this same PR) being reset at every TEST/PRE. Two failure modes if it is not:

1. Sticky across scenarios — scenario 1 has a real step failure, scenario 2 fails only in an
   After hook. hasStepFailures() still returns true, hookOnlyFailure is false, and scenario 2
   fails the session even though ignoreHooksStatus: true asked for exactly the opposite. Legacy,
   reading world, exempts it.
2. Cleared too eagerly (e.g. reset on step start, or only tracking the last step) — a genuine
   step failure is read as hook-only and the added passed = status === 'passed' || hookOnlyFailure
   marks a failed Automate session passed. That is the silent-false-pass shape OB-01 names;
   nothing errors and the dashboard is simply wrong.

Secondary, same expression: the exemption is narrowed to status === 'failed', while the legacy
block above applies it to every member of _failureStatuses (ambiguous, undefined, and
pending under cucumberOpts.strict). For those statuses the CLI flow now marks the session failed
where the legacy flow, with ignoreHooksStatus on and no failing test step, does not. That corner is
probably unreachable for UNDEFINED/AMBIGUOUS (an undefined step is a test step), but PENDING under
strict is reachable, and the divergence is not stated anywhere in the added comments even though
the rest of this method documents its legacy-fidelity reasoning in detail.

What I could not establish: WdioCucumberTestFramework.hasStepFailures() lives in another
review unit's file, so its reset semantics are outside what I can verify here. If it is reset in the
TEST/PRE handler and scoped to the current scenario, this finding is void — please confirm, ideally
with a test covering two scenarios in one feature where the first fails a step and the second fails
only a hook, with ignoreHooksStatus: true.

Suggested Fix

Make the scenario scoping explicit at the call site rather than implicit in the framework's
state, so the two paths in this method ask the same question:

const hasStepFailures = this._cliCucumberFramework()?.hasStepFailures(world) ?? true

with hasStepFailures(world) keyed on the scenario (pickle id / world.testCaseStartedId), or —
cheaper — reuse the value the legacy block above already computed for the same scenario
(this._insightsHandler?.hasTestStepFailures(world)), which is already scenario-scoped and is what
this path is porting.

If hasStepFailures() genuinely is per-scenario state that TEST/PRE clears, say so in the comment
next to the call (the rest of this method documents its legacy-fidelity reasoning; this is the one
load-bearing assumption left implicit), and add the two-scenario regression case described above.

For the secondary point, either widen the exemption to this._failureStatuses.includes(status) to
match the legacy block twenty lines up, or state in the comment why failed alone is the intended
scope.

Confidence: 🟡 packs/observability.md OB-01 ('a status set on one branch only, so a failure reports as passed'). Unverified: the scenario-scoping of WdioCucumberTestFramework.hasStepFailures() is in another unit's file, outside this unit's read scope.

— SDK PR Review Agent

[AdityaHirapara]

Checked both sides — they ask the same question.

	legacy hasTestStepFailures(world)	CLI hasStepFailures()
populated by	beforeStep(step, scenario) (insights-handler.ts:702)	onStepStart(step) (wdioCucumberTestFramework.ts:140)
contents	pickle steps only	pickle steps only
predicate	steps.some(s => s.result === 'FAILED')	scenarioSteps.some(s => s.result === 'FAILED')

Both are driven by WDIO's beforeStep, so neither counts Before/After hooks — which is what makes the ignoreHooksStatus exemption meaningful in the first place. Same data, same predicate.

Two differences, neither reachable as a divergence:

• Scoping. Legacy keys per world; the CLI resets scenarioSteps at scenario start and holds it on the instance. Equivalent while scenarios run sequentially within a worker, which is what cucumber does — WDIO parallelises across specs, each in its own process.
• The ?? true fallback is the opposite of legacy's implicit false (no step data → legacy treats it as hook-only and marks passed). It is unreachable at the only call site, which sits inside if (cliFramework), and it fails in the safer direction — keeping a failure rather than converting it to a pass.

[AdityaHirapara]

⚠️ Warning — [TEST COVERAGE] The SDK-7270 inverse-leak guard is inverted, and nothing replaces it

Problem

This test used to be a named regression guard — adds no status traffic for skipSessionName users (SDK-7270 inverse-leak guard) — asserting expect(fetch).not.toHaveBeenCalled(). The PR keeps the same test body and flips it to assert the opposite: exactly one call, status: 'passed', no name field.

The new behaviour may well be right (the comment argues legacy gates its after() status block on setSessionStatus alone), but the consequence for the suite is that SDK-7270 now has no guard anywhere in this file. The ticket id survives only in the deleted lines. If SDK-7270 was filed because status traffic appeared for a class of session that should have had none, the shape that regressed is no longer pinned by any test — the file now only pins the inverse.

I could not settle this from my unit: the ticket's actual failing scenario is not described in the diff, and the production change that motivates the flip lives in another unit. Flagging so a human confirms rather than asserting a defect.

Suggested Fix

Either (a) state in the test file — one line, with the ticket id — what SDK-7270's defect actually was and why legacy parity supersedes it, so the next reader does not have to reconstruct it from git history; or (b) keep a separate, narrowly-scoped case that still pins whatever SDK-7270 fixed (e.g. the session shape that genuinely must produce no status traffic at all), alongside the new parity assertion. Re-using a ticket-tagged guard's body for the opposite claim leaves no trace that the guard ever existed.

Confidence: 🟡 The only guard this file carried for SDK-7270 is repurposed into asserting the opposite outcome; I could not establish from this unit whether SDK-7270's actual defect keeps any coverage.

— SDK PR Review Agent

[AdityaHirapara]

Not valid — SDK-7270 keeps three named guards in this same file:

• names the session created by a mid-test reload (SDK-7270) (line 226)
• issues no extra name call when the session is unchanged across the test (SDK-7270 de-dupe) (line 258)
• still names a mid-test-reload session when skipSessionStatus is true (SDK-7270 guard independence) (line 279)

SDK-7270 is the mid-test browser.reloadSession() case — the replacement session never being registered and keeping its creation-time name — and those three pin it directly. The production comment at automateModule.ts:199 describes the same shape.

The flipped test carried the ticket id in its name but asserted something else: that a skipSessionName session gets no traffic at all. That was the CLI's own behaviour rather than parity with legacy, which gates its after() status block on setSessionStatus alone. Flipping it was the point of the change; no SDK-7270 coverage was lost with it.

[AdityaHirapara]

⚠️ Warning — [TEST COVERAGE] Null-framework test asserts only the no-op half of the behaviour it names

Problem

The test is named warns and no-ops when no CLI test framework is registered, but its only assertion is expect(instance.updateMultipleEntries).not.toHaveBeenCalled() — the warns half is never checked (the sibling cucumber test does assert BStackLogger.warn).

That matters here beyond tidiness, because setCustomTags's whole body sits inside a try { … } catch (error) { this.logger.warn(...) }. On the null path, BrowserstackCLI.getInstance().getTestFramework() returns null and null instanceof WdioMochaTestFramework is false, so the gate is what should fire — but if that expression ever threw instead (e.g. the singleton or the accessor changing shape), the exception would be swallowed by the catch, updateMultipleEntries would still not have been called, and this test would keep passing. The assertion therefore cannot distinguish "the gate rejected it" from "the gate crashed and the error boundary hid it", which is precisely the regression this test exists to catch.

Suggested Fix

Assert the warn half too, so the test pins the gate rather than any path that happens to skip the merge:

it('warns and no-ops when no CLI test framework is registered', async () => {
    useFramework(null)
    const setCustomTags = await register()

    await setCustomTags('TC', 'TC-1')

    expect(instance.updateMultipleEntries).not.toHaveBeenCalled()
    expect(BStackLogger.warn).toHaveBeenCalledWith(
        'setCustomTags is only supported for the mocha framework; ignoring call'
    )
})

The gate's message is the discriminating signal — the catch-block's warn is a different string (setCustomTags: error while recording custom tags: …), so toHaveBeenCalledWith separates the two outcomes cleanly.

Confidence: 🟢 packs/tests.md TS-01 — 'a test whose name claims a behaviour its assertions do not check'

— SDK PR Review Agent

[AdityaHirapara]

Fixed in bc28ca6 — the test now asserts both halves of what its name claims:

expect(instance.updateMultipleEntries).not.toHaveBeenCalled()
expect(BStackLogger.warn).toHaveBeenCalledWith(
    'setCustomTags is only supported for the mocha framework; ignoring call'
)

Worth noting for the record that this module had no tests at all before this PR — the file is new here, added to cover the mocha gate this PR introduces. The null-framework case routes through that same new gate, so the missing assertion was an inconsistency inside our own new file rather than inherited, which is why it is being closed rather than deferred.

Falsified: downgrading the production warn to debug now fails this test alongside its sibling.

[AdityaHirapara]

⚠️ Warning — [TEST COVERAGE] Skipped-scenario tally test asserts the counter's initial value, so it cannot fail for the regression it names

Problem

_scenariosRanCount is initialised to 0 on BrowserstackService (see service.ts field declaration private _scenariosRanCount: number = 0). The test drives exactly one afterScenario with result.status === 'skipped' and then asserts the counter is 0 — which is also what it reads if the increment path is removed entirely, if afterScenario bails out early before reaching the tally, or if the whole _scenariosRanCount bookkeeping is deleted from the service. The named behaviour ("skipped scenarios are excluded from the tally") is therefore only half-covered: nothing in this describe block ever observes the counter moving, so the test is green both when the exclusion works and when the counting does not work at all.

This matters here specifically because the tally is what preferScenarioName keys off on the legacy/service side (the describe's own comment says _scenariosRanCount is what legacy reads), so a silent zero would flip the rename decision for every run without any test in this file failing.

Suggested Fix

Add the positive half so the assertion has to move. Either extend this test or add a sibling:

it('counts a scenario that ran, and not a skipped one, toward the service-side tally', async () => {
    const service = makeService({ preferScenarioName: true })
    await service.afterScenario({ pickle: { name: 'Ran one' }, result: { status: 'passed' } } as never)
    expect(service['_scenariosRanCount']).toBe(1)

    await service.afterScenario({ pickle: { name: 'Skipped one' }, result: { status: 'skipped' } } as never)
    expect(service['_scenariosRanCount']).toBe(1)
})

That keeps the existing intent (a skipped scenario must not increment) while making the test fail if the counter stops incrementing at all.

Confidence: 🟢 packs/tests.md TS-01 — a test must assert the behaviour it names; an assertion that compares a counter to its own initial default cannot distinguish the named behaviour from the counter never working at all.

— SDK PR Review Agent

[AdityaHirapara]

Not valid — the test does fail when the behaviour regresses. I checked by falsification rather than by reading it.

Replacing the gate in afterScenario with an unconditional branch:

const status = world.result?.status.toLowerCase()
if (true) {          // was: if (status !== 'skipped')

makes it fail:

× preferScenarioName reaches the module > does not count a skipped scenario toward the service-side tally
  Tests  1 failed

_scenariosRanCount does start at 0, but the test drives a skipped scenario through afterScenario between construction and the assertion — so the 0 it asserts is a result, not the initial value. Remove the skip gate and the counter reaches 1.