fix(app-a11y): keep scanning across browser.reloadSession() - #165
Conversation
…ture aliased configs Three defects, each reproduced on an App Automate device run and verified against the session's own appAllyScan telemetry. App-A11y scanning is gated on a per-session entry that only onBeforeTest and onHookStart ever wrote, and that entry is keyed on the session id: - Commands issued from WDIO's config-level before()/beforeSession() ran before any entry existed, so a hook that launches the app and signs in produced no scans at all while still counting as expected coverage. The gate now opens at driver creation. - browser.reloadSession() leaves the entry stranded under the old session id, so nothing scanned for the rest of the reloading test. onReload now migrates it, on both the CLI and the classic handler paths. Config auto-capture followed only relative import specifiers, so a project that wires its split configs through tsconfig `paths` uploaded its entry config alone — the file carrying the hooks was never in the bundle. Aliases are now resolved from the nearest tsconfig/jsconfig (following `extends`, tolerating JSONC); the *.conf.* name filter still keeps application source out. SDK-7422
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…replace CodeQL flagged the substitution as an incomplete replacement. The sharper problem is the other half of `String.replace` with a string pattern: `$&` / `$'` in the REPLACEMENT are interpreted, so a specifier carrying those characters resolved to a corrupted path and the imported config was silently dropped from the bundle. Slicing at the first `*` is immune to both, and TypeScript allows at most one `*` per target, so the first is the substitution point by definition. Test pins it — it fails on the previous implementation, which resolved `@confs/a$&b.conf` to `configs/a*b.conf.ts`. SDK-7422
Phase-annotated pre/post verificationThe bench now annotates every phase boundary into the session log with The markers are themselves driver commands, but they carry a Pre-fix —
|
| phase | commands issued | scans |
|---|---|---|
config-level before() |
startActivity ×2, click ×3 |
0 — window dark |
mocha before all |
back ×2 |
2 |
mocha before each #1 |
back ×1 |
1 |
| test 1, pre-reload | back ×3 |
3 |
| 6 total |
Reloaded session f5d3de3bbd66cbd804c721e6c35276afb916ae70:
| phase | commands issued | scans |
|---|---|---|
| test 1, post-reload | click ×3, back ×3 |
0 — window dark |
mocha before each #2 |
back ×1 |
1 |
| test 2 | click ×1, back ×2 |
3 |
mocha before each #3 |
back ×1 |
1 |
| test 3 | back ×2 (click element absent) |
2 |
| 9 total (+2 end-of-test) |
Post-fix — this branch
Build xjfwleeeyemcwi7jrsbttqsetrha9wfvbiuvneub
Original session f095f87f968ee6f502db77a8ef8e485d9ee3b441:
| phase | scans | change |
|---|---|---|
config-level before() |
7 — execute ×2, executeScript ×2, click ×3 |
0 → 7 |
mocha before all |
2 | unchanged |
mocha before each #1 |
1 | unchanged |
| test 1, pre-reload | 3 | unchanged |
| 13 total | from 6 |
Reloaded session 4c2cd4a8d74c3e397ed177b5e6fa5cd98b8ec32e:
| phase | scans | change |
|---|---|---|
| test 1, post-reload | 6 — click ×3, back ×3 |
0 → 6 |
mocha before each #2 |
1 | unchanged |
| test 2 | 3 | unchanged |
mocha before each #3 |
1 | unchanged |
| test 3 | 2 | unchanged |
| 16 total | from 9 |
Command counts are identical across the two runs (T3_click_fail in both — the element genuinely wasn't there), so every delta above is the fix and nothing else.
Three things the annotations settle
-
Framework hooks were never the problem.
before allandbefore eachscan identically before and after the fix — 2 and 1, in both runs, in both sessions. Only the config-level hook was dark. This retires a hypothesis that survived several rounds of this investigation on the strength of hook annotations appearing near unscanned commands. -
Pre-fix reload damage is bounded to the reloading test, not the session.
before eachchore: update package and lock files devDependencies #2 recovers on its own, because the nextonBeforeTest/onHookStartregisters the gate under the new session id thatonReloadhad already published. So the cost is the remainder of the one test that calledreloadSession()— 6 commands here — after which scanning resumes unaided. Worth stating precisely: earlier framing in this PR implied a wider blast radius than the log supports. -
The 6 ms race is only reachable by a hook whose first act is a scannable command. Both
startActivitycalls scanned in this run (execute×2 +executeScript×2), where the earlier unannotated run caught only the late one. The difference is theannotatesitting ahead of them: it is itself a device round-trip, and it absorbs the window. Any prior driver call does the same, which bounds the residual further than the previous comment claimed.
Also in this push: the CodeQL comment on the alias wildcard is addressed in e943a78.
Scans fired from WDIO's config-level before() had neither thTestRunUuid (no test exists yet) nor thHookRunUuid, so App-A11y's lookup of the scan's parent in BTCER had nothing to find — the scan reached the hub and then belonged to nothing. The config-level window is now reported as a BEFORE_ALL hook run through the existing framework path, so the framework mints the uuid, the binary emits HookRunStarted/Finished for it, and this module's own onHookStart observer stamps it onto the scans. The instance the tracked hook creates is the one the first test reuses, so the hook also lands parented to a real test_run_id rather than orphaned. Opened ON DEMAND, from the scan path only. WDIO gives a service no event for a user's config-level before() — ConfigParser.addService folds the user's config hooks and every service's hooks into one config.before array that the runner fires with Promise.all — so there is nothing to detect, and opening it from the lifecycle would report a hook that never ran on every build in the fleet. Opening it when a scan needs a parent is exactly the condition that matters, and it self-limits: verified against a control config with no config-level hook, which reports only its four real Mocha hooks. SDK-7422
Pre-test scans now carry a hook run TRA can resolve —
|
| config | hook events reported |
|---|---|
with config-level before() |
5 hook runs — 4 Mocha + 1 pre-test window |
without it — lpbvftwaslksugogditfsvc1pspzd4fwgtneyxnh |
4 hook runs — Mocha only, nothing extra |
Bench control kept at examples/repro-triple/repro-nohook.conf.ts.
Notes for review
'idle' → 'attempted' → 'open' → 'closed'rather than a boolean:attemptedis what stops a failed PRE from being retried on every wrapped command and from being closed by a POST that would pair with nothing.- The state is set before the await, so concurrent commands cannot each open their own hook run.
- The window is closed at the first
onBeforeTest. Its duration therefore includes any Mocha hooks that ran in between — the end of a user's config hook is genuinely not observable, so this is an upper bound, and the scans inside those Mocha hooks still carry their own hook uuids. - 4 unit tests: opens once and only once, never opens while a framework hook is already the parent, closes at the first test, never closes what it did not open.
- Full suite at parity (75 pre-existing failures before and after); lint clean.
…hook Wraps the handlers registered for each WDIO hook that runs with a live driver, so the window a driver command falls into is observable. WDIO tells a service nothing about the other handlers in a hook array — ConfigParser folds the user's config hooks and every service's hooks into one array the runner fires with Promise.all — so patching the array in place is the only way to see a non-ours handler start and finish. Logging only; no events, no behaviour change. Sync handlers stay sync so a sync throw is not converted into a rejection. Verified: 'before' appears only when the user declares one (control config with no hooks shows none), while the beforeTest/afterTest entries come from expect-webdriverio's snapshot and soft-assert services — so 'registered at construction' means the user's hooks plus any service constructed before ours, not user code alone. SDK-7422
… instead
Reverts the tsconfig `paths` alias resolution added earlier in this PR, restoring
configCapture to what it was: relative imports only, entry config plus package.json.
Following aliases widened what gets uploaded from a customer's project, and the actual
need — seeing the hook code behind a failure — does not require uploading anything.
Instead the launcher reads the hook sources out of the config file and logs them. The
parsed config cannot supply them: ConfigParser folds every hook into an array as
`hook.bind(service)`, and a bound function stringifies to `[native code]`, so the file
is the only place the bodies survive.
Also records which hooks call reloadSession and publishes the list on the environment as
BROWSERSTACK_HOOKS_WITH_RELOAD_SESSION — a reload inside a hook swaps the session under
the driver, which is the shape behind the scan-gate defect this PR fixes.
Parsing notes: brace matching is lexical, so a `}` inside a string, comment or template
literal does not truncate a body; and the parameter list is skipped by balancing its
parentheses before looking for the body brace, because a typed parameter carries its own
braces (`beforeTest: function (test: { title?: string })`) and taking the first one
captured the signature instead of the hook.
Known limit, unchanged from before: only hooks written in the entry config are found. A
split config that imports its hooks elsewhere yields nothing — the same blind spot the
alias work was aimed at, now without uploading files.
The config-level before() scan fix moves to SDK-7422-config-level-before-hook.
SDK-7422
kamal-kaur04
changed the title
Scope narrowed —
|
Hook bodies are customer code and the debug log is uploaded, so the captured source now goes
through redactSensitiveContent — the same routine that guards an uploaded config file.
BStackLogger's own scrub is not enough on its own. Measured against a hook body carrying nine
sensitive shapes, it leaves six readable: authToken, password, clientSecret, a snake_case
AWS secret, URL userinfo and an inline `token:` value. It knows only user/key/userName/accessKey
in a key-value or query-string position.
The identifier scan moved ahead of redaction, which is not cosmetic: redaction replaces the
whole matching line, so `await browser.reloadSession({ userName, accessKey })` — reloading with
fresh credentials, a real pattern — collapses to [REDACTED] and the call would disappear from
the detection. Raw text is scanned for the tracked identifiers, then the stored copy is
redacted; extractUserHookSources returns both, and hooksUsing documents that it can only be as
complete as the text handed to it.
Test fixtures use neutral sentinels rather than realistic secrets — the redactor keys on the
field name, not the value shape, so nothing is lost by not committing token-shaped strings.
SDK-7422
Everything that writes to the uploaded logs leaves this branch, so the PR is the reloadSession scan-gate fix and nothing else. configCapture, constants and launcher are identical to main again. Parked on SDK-7422-hook-source-logging, complete with the redaction fix: reading a customer's hook bodies into an uploaded log needs its own review, on its own timeline, not as a rider on a scan-gate fix. SDK-7422
kamal-kaur04
changed the title
Scope final — reloadSession only (
|
| shape | logger scrub | redactSensitiveContent |
|---|---|---|
accessKey, apiKey, userName |
redacted | redacted |
authToken, password, clientSecret, inline token: |
readable | redacted |
AWS_SECRET_ACCESS_KEY (snake_case) |
readable | redacted |
URL userinfo https://admin:…@host |
readable | redacted |
| PEM private-key block | readable | redacted |
So captured sources now go through redactSensitiveContent — the same routine that already guards an uploaded config file — rather than relying on the logger.
One ordering detail that matters there: the identifier scan had to move ahead of redaction. Redaction replaces the whole matching line, so await browser.reloadSession({ userName, accessKey }) — reloading with fresh credentials, a real pattern — collapses to [REDACTED] and the call would vanish from the very detection it feeds. Raw text is scanned first, then the stored copy is redacted.
Also: the test fixtures use neutral sentinels rather than realistic tokens. The repo's gitleaks guard rejected the commit when they looked like real secrets, and it was right to — the redactor keys on the field name, not the value shape, so nothing is lost.
Where the rest went
| branch | what | why it's not here |
|---|---|---|
SDK-7422-config-level-before-hook |
config-level before() commands unscanned; the BEFORE_ALL hook run for that window; hook-window instrumentation |
needs a hook-lifecycle design |
SDK-7422-hook-source-logging |
hook sources logged, redacted | writes customer code into an uploaded log |
| — | tsconfig-alias following in config capture | reverted; widened what gets uploaded |
All device-verified; the evidence for each is in this PR's comment history so it survives the merge.
Review catch: the classic handler's migration ran in both flows, and under the CLI it is redundant. The handler is constructed either way, but `before(sessionId)` — which records _sessionId and populates the scan map — runs only in the else branch of the CLI check, so in the binary flow the object holds nothing to migrate. Worse than redundant, in fact: the old `_sessionId === null` fallback meant the CLI flow would adopt the new id onto an otherwise inert handler. Now an explicit either/or: the CLI flow migrates the AccessibilityModule gate, the classic flow migrates the handler. The handler also only migrates the session it was actually tracking, rather than treating null as "adopt this one". SDK-7422
Re-verified E2E after the review fix —
|
| phase | original session 48e4d25f… |
reloaded session 10d05433… |
|---|---|---|
config-level before() |
0 | — |
mocha before all |
2 | — |
mocha before each #1 |
1 | — |
| test 1, pre-reload | 3 | — |
| test 1, post-reload | — | 0 |
before each #2 |
— | 1 |
| test 2 | — | 3 |
before each #3 |
— | 1 |
| test 3 | — | 2 |
| 6 | 9 (incl. 2 end-of-test) |
Post-fix — this branch — pdneyhvmmyowe85n7jzcxmi23nyru15xukh9flju
| phase | original session fcfbe107… |
reloaded session 8980fd6f… |
|---|---|---|
config-level before() |
0 | — |
mocha before all |
2 | — |
mocha before each #1 |
1 | — |
| test 1, pre-reload | 3 | — |
| test 1, post-reload | — | 6 — click ×3, back ×3 |
before each #2 |
— | 1 |
| test 2 | — | 3 |
before each #3 |
— | 1 |
| test 3 | — | 2 |
| 6 | 16 (incl. 3 end-of-test) |
Exactly one phase moves: 0 → 6, the remainder of the test that called reloadSession(). Every other row is identical across the two runs, including the pre-reload session totalling 6 in both — which is the check that the fix is narrow rather than generally louder.
Two things this pair also confirms:
- The later tests were already recovering without the fix —
before each #2onward is 9 scans pre-fix. That is the evidence behind the bounded-blast-radius claim in the description: a reload costs the remainder of one test, not the session. - The config-level
before()window is still dark in both, 0 scans either side. Correct for this PR — that fix is onSDK-7422-config-level-before-hook, and its absence here is visible rather than assumed.
Unit side after the review fix: 82 passing across the two accessibility suites, service.test.ts back at its 40-failure baseline with the new flow-gating test passing (the +1 failure my first cut introduced was the new test itself hitting _printSessionURL's live fetch — now stubbed, since that is not what the test is about). Full suite 75 pre-existing failures, unchanged. Lint clean.
|
RUN_TESTS |
Five review findings, all the same underlying shape: the scan gate is read per command from framework state, which a reload updates, while several writers captured the session id once and kept writing to it. Migrating the gate turned that latent disagreement into a live one. Before the migration both sides were stale, so they agreed and nothing scanned; after it, commandWrapper reads the new key while a captured writer sets the old one. Concretely: browser.stopA11yScanning() called AFTER a reload set false on the dead key while the live key stayed true, so scanning carried on against an explicit instruction, and startA11yScanning() was a silent no-op — in exactly the window this PR exists to fix. - accessibilityModule: the scanning toggles and the results getters resolve the session at call time via currentSessionId(). getAccessibilityResults/Summary previously reported the PRE-reload session's results, while the classic path reported the live one, so the two flows disagreed on the same operation. - accessibility-handler: the toggles write this._sessionId, which commandWrapper reads and this PR migrates, instead of the id captured in before(). - service: one _isCliAccessibilityFlow() predicate, used both where AccessibilityHandler.before() is gated and where the reload migration chooses a path. isRunning() alone is NOT equivalent — with the binary up against a non-BrowserStack provider, before() DID run, so the handler holds the live state and must still be migrated. - accessibilityMap / LOG_DISABLED_SHOWN are Map<string, boolean>: session ids are strings, the Map<number> declaration was a mis-declaration, and the casts it forced (including `as never` in the tests) are gone. Tests: the CLI branch's positive path is now asserted through the module registry rather than left to an optional chain; the non-BrowserStack-provider case is covered; and both toggle paths are pinned against the post-reload inversion. SDK-7422
|
RUN_TESTS |
PR Review: wdio-browserstack-service PR #165SummaryIntent: Migrate the accessibility scan-gate state (and, for the classic handler, ═══════════════════════════════════════════════════════════════ FindingsTwo channels. Blocking = Critical + Warning — the must-fix set the Verdict gates on. Non-blocking = Suggestions — polish; best-effort, never gates. 🔴 Critical (Blocking)
1. [Correctness] New private method inserted between an existing decorator and its target silently re-targets the decorator —
─────────────────────────────────────────────────────────────── 🟠 Warnings (Blocking)None. 💡 Suggestions (Non-blocking)None. ═══════════════════════════════════════════════════════════════ External ServicesNo external-contract changes detected. ═══════════════════════════════════════════════════════════════ Per-File Confidence (for reviewers)
═══════════════════════════════════════════════════════════════ What's Good
═══════════════════════════════════════════════════════════════ Coverage Ledger
Totals: 21/21 regions judged, ═══════════════════════════════════════════════════════════════ Verdict🔴 Fix 1 blocking issue
═══════════════════════════════════════════════════════════════ — SDK PR Review Agent |
The predicate added in cec234c landed between the decorator and the method, so the decoration moved to it: onReload stopped emitting its SDK_HOOK measurement, and the predicate started reporting one under hookType 'onReload' on every call — including from the before hook, so every session reported an onReload measurement whether or not it ever reloaded. Behaviour was unaffected (PerformanceTester.measure returns the raw value on the synchronous path), which is why nothing surfaced it: valid TypeScript, lint has no opinion, and the unit suites mock Measure into a pass-through so the binding has no observable behaviour to assert. Hence a structural test over the decorated files instead — verified to fail with the mistake reintroduced and pass without it. Doc comment corrected too: the two accessibility paths are NOT strictly exclusive. The module is registered on startBinResponse.accessibility?.success alone, independent of provider, so under a non-BrowserStack provider it can hold real state while the classic handler's before() has also run. What the predicate decides is which one owns the CLASSIC handler's state. SDK-7422
bd75537
|
RUN_TESTS |
SDK PR Review — ✅ Good to goHead reviewed: Prior blocking finding — RESOLVED
private _isCliAccessibilityFlow (): boolean {
return Boolean(isBrowserstackSession(this._browser)) && BrowserstackCLI.getInstance().isRunning()
}
@PerformanceTester.Measure(PERFORMANCE_SDK_EVENTS.EVENTS.SDK_HOOK, { hookType: 'onReload' })
async onReload(oldSessionId: string, newSessionId: string) {Nothing but a blank line stands between the decorator and its target. New finding (non-blocking) — the new guard's file list misses 3 real decorator sites
Recommend adding A second, narrower gap in the same check: it flags only when the next non-blank line starts with Neither point blocks this PR — they don't touch the fix and don't reintroduce the bug being fixed. Scope notePer commit-graph inspection ( Coverage ledger
Regions judged: 2/2 (100%), Falsified / dropped findingsNone dropped. One candidate concern was investigated and closed as not-a-bug: whether Verdict✅ No blocking issues. The prior blocking finding is resolved and verified against real code at the new head. One non-blocking follow-up noted on the new guard's file coverage. — SDK PR Review Agent |
6 participants
What is this about?
browser.reloadSession()no longer stops accessibility scanning. That is the whole PR — 125 lines across three source files.A reload hands the worker a new session id while the driver object, the wrapped commands and the running test all stay the same. The scan gate is keyed on the session id, so the entry registered for the old id is orphaned the moment the reload lands: every command for the rest of that test looks up a key that no longer exists and is silently not scanned.
onReloadalready updatedKEY_FRAMEWORK_SESSION_ID; it now carries the gate entry across with it.Migrating the entry is only half of it: several writers had captured the session id once and kept writing to it, while
commandWrapperre-reads framework state per command. Before the migration both sides were stale, so they agreed and nothing scanned; after it they would disagree — astopA11yScanning()called after a reload would set the dead key while the live key stayedtrue, so scanning would carry on against an explicit instruction. So the scanning toggles and the results getters now resolve the session at call time. (Reproduced — 3 scans after a post-reload stop — and fixed — 0.)The classic (
accessibility-handler) path had the same staleness —_sessionIdis captured once inbefore()and never updated, so every later scan-gate lookup and results call addressed a session that had ended — so it is migrated too.Blast radius, measured
The loss is bounded to the test that called
reloadSession(), not the session. The next test'sonBeforeTest/onHookStartre-registers under the new id, so scanning resumes on its own from the followingbefore each. Worth stating precisely because an earlier revision of this description implied wider damage than the logs support.Verification
Bench:
test-samples/app_automate-wdio_mocha-android/examples/repro-triple/— three tests withreloadSession()inside the first, and phase boundaries annotated into the session log viabrowserstack_executorso each scan is attributed by position rather than inferred from its command type. Verdicts read from the App Automate session-logs API.va0eji8ln9bke4crc9jq4q0n44ru25autnebse7ljy8hf6xg3bcpw8zxd3dnsaql0zwqqis7ipvi8u5kIdentical commands either side of the reload; the post-reload session goes from nothing to fully scanned, and the pre-reload half is unchanged.
Per-phase, post-fix, in the reloaded session:
test 1 body RESUMED6 scans (click×3,back×3) where the baseline had 0, thenbefore each #21,test 23,before each #31,test 32 — the later tests were already recovering before this fix, which is what bounds the blast radius above.Re-verified on the stacked head —
r89xojas5cvljamjpwrj8x7zcf3pc3umt8qyoptk(sessions72d8f60ebfd6485d670021fb68f43c4163ee6897pre-reload,e60022566902b3f72651c4784448e2d39ff73302post-reload), run from #168 which stacks on this branch: still 6 pre-reload and 16 post-reload, so the work layered on top does not regress the migration. That run's own subject — global-hook scans no longer carrying a test run uuid, mocha and cucumber, pre-fix and post-fix — is evidenced on #168 rather than duplicated here.Which path owns the migration is decided by one predicate,
_isCliAccessibilityFlow()—isBrowserstackSession(browser) && isRunning()— used both here and whereAccessibilityHandler.before()is gated, so the two cannot drift.isRunning()alone is not equivalent: with the binary up against a non-BrowserStack provider the classic handler did initialise and still holds the live state.11 unit tests across the three files, covering the migration itself, a user-closed gate surviving it, the post-reload toggle on both paths, the module-registry lookup in the CLI flow, the non-BrowserStack-provider case, and no-op/missing ids. Full suite at parity — 75 pre-existing failures before and after, verified by stashing and re-running.
npm run lintclean.Split out of this PR
Two other things came out of the same investigation and are parked on their own branches, each needing review on its own terms rather than as a rider here:
SDK-7422-config-level-before-hook— driver commands in a WDIO config-levelbefore()are never scanned. Fix is device-verified (hook window 0 → 7 scans) but needs a hook-lifecycle design, including how such a scan gets ahook_run_uuidTRA can resolve.SDK-7422-hook-source-logging— logging the user's hook sources for triage, with credential redaction. Anything that writes customer code into an uploaded log needs its own review.Earlier revisions of this branch also followed tsconfig
pathsaliases in config capture; that is reverted andconfigCapture.tsis identical tomain. The commit history shows both add-then-revert cycles.Related Jira task/s
https://browserstack.atlassian.net/browse/SDK-7422
Release (mandatory for every PR — required for the
ready-for-reviewlabel)Version bump: (required — tick exactly one)
Release notes type: (optional)
Release notes (customer-facing): (optional but encouraged)
browser.reloadSession().Release notes (internal): (required — engineer-facing; what actually changed / why)
accessibilityModule.onSessionReload+service.onReload: migrate the session-keyed scan gate (andLOG_DISABLED_SHOWN) onto the new session id afterreloadSession(), which previously orphaned it for the remainder of the reloading test.accessibility-handler: same migration for_a11yScanSessionMapand_sessionIdon the classic (non-CLI) path, where_sessionIdwas captured once inbefore()and never updated. Its scanning toggles writethis._sessionId— whatcommandWrapperreads — instead of the id captured inbefore().accessibilityModule: scanning toggles andgetAccessibilityResults/Summaryresolve the session at call time (currentSessionId()); the getters previously reported the pre-reload session's results while the classic path reported the live one.service: one_isCliAccessibilityFlow()predicate gates bothAccessibilityHandler.before()and the reload migration.accessibilityMap/LOG_DISABLED_SHOWNareMap<string, boolean>— session ids are strings; theMap<number>declaration was a mis-declaration and the casts it forced are gone.before()scanning →SDK-7422-config-level-before-hook; hook-source logging →SDK-7422-hook-source-logging; tsconfig-alias config capture → reverted.Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.