fix(automate): detect App Automate from appium:app capability for session name/status (SDK-7093) - #100
fix(automate): detect App Automate from appium:app capability for session name/status (SDK-7093)#100pri-gadhiya wants to merge 7 commits into
Conversation
…sion name/status (SDK-7093) When the app is supplied only via the appium:app capability (not the top-level browserstack service `app` option), this.config.app is undefined, so isAppAutomate resolved false and the session name/status PUT was routed to the web Automate endpoint for an App Automate session — returning a swallowed 404, leaving the static sessionName capability on the dashboard. Route both markSessionName and markSessionStatus through a new isAppAutomate() helper that also inspects the input + resolved capabilities for appium:app / appium:options.app, mirroring accessibilityModule.isAppAutomateSession. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
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. |
|
🔴 Blocking findings — fix required See the SDK PR Review Agent's report from your local run. ↻ 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 |
|
🔴 SDK PR Review gate is red. Pending:
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. |
…K-7093) Address SDK PR review findings on #100: - Promote hasAppCap + APP_AUTOMATE_CAP_KEYS from config.ts (file-local) to util.ts as a shared export; config.ts reuses it (no behavior change). - automateModule.isAppAutomate() reuses hasAppCap — now covers all app-cap keys (appium:app / appium:bundleId / appium:appPackage / appium:appActivity / appium:options.app) instead of only appium:app. - percyModule: fix the same service-config-only detection (`'app' in this.config`) that misrouted Percy screenshots to the web Automate path for appium:app sessions — now resolves App Automate via hasAppCap over browser + input caps. - changeset: note now covers session name, status, and Percy routing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
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. |
|
🔴 SDK PR Review gate is red. Pending:
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. |
…sors (SDK-7093) The reuse refactor made automateModule + percyModule depend on the shared util.hasAppCap and (for the capability fallback) AutomationFramework getTrackedInstance/getState. Update the unit-test mocks accordingly: - automateModule.test: add hasAppCap to the wholesale util.js mock (was undefined → threw → swallowed → fetch never called). - percyModules.test: add getTrackedInstance/getState to the AutomationFramework mock so onAfterCreate's isAppAutomateSession() no longer throws. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
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. |
| const autoInstance = AutomationFramework.getTrackedInstance() | ||
| return [AutomationFrameworkConstants.KEY_INPUT_CAPABILITIES, AutomationFrameworkConstants.KEY_CAPABILITIES] | ||
| .some(key => hasAppCap(AutomationFramework.getState(autoInstance, key) as WebdriverIO.Capabilities | undefined)) |
There was a problem hiding this comment.
| const autoInstance = AutomationFramework.getTrackedInstance() | |
| return [AutomationFrameworkConstants.KEY_INPUT_CAPABILITIES, AutomationFrameworkConstants.KEY_CAPABILITIES] | |
| .some(key => hasAppCap(AutomationFramework.getState(autoInstance, key) as WebdriverIO.Capabilities | undefined)) | |
| return hasAppCapInFrameworkState() |
| const appiumOptions = record['appium:options'] as Record<string, unknown> | undefined | ||
| return !!(appiumOptions && !isUndefined(appiumOptions.app)) | ||
| } | ||
|
|
There was a problem hiding this comment.
We can export another function here:
| import AutomationFramework from './cli/frameworks/automationFramework.js' | |
| import { AutomationFrameworkConstants } from './cli/frameworks/constants/automationFrameworkConstants.js' | |
| export function hasAppCapInFrameworkState(): boolean { | |
| const autoInstance = AutomationFramework.getTrackedInstance() | |
| const keys = [ | |
| AutomationFrameworkConstants.KEY_INPUT_CAPABILITIES, | |
| AutomationFrameworkConstants.KEY_CAPABILITIES | |
| ] | |
| return keys.some(key => | |
| hasAppCap(AutomationFramework.getState(autoInstance, key) as WebdriverIO.Capabilities | undefined) | |
| ) | |
| } |
| const autoInstance = AutomationFramework.getTrackedInstance() | ||
| return [AutomationFrameworkConstants.KEY_INPUT_CAPABILITIES, AutomationFrameworkConstants.KEY_CAPABILITIES] | ||
| .some(key => hasAppCap(AutomationFramework.getState(autoInstance, key) as WebdriverIO.Capabilities | undefined)) |
There was a problem hiding this comment.
| const autoInstance = AutomationFramework.getTrackedInstance() | |
| return [AutomationFrameworkConstants.KEY_INPUT_CAPABILITIES, AutomationFrameworkConstants.KEY_CAPABILITIES] | |
| .some(key => hasAppCap(AutomationFramework.getState(autoInstance, key) as WebdriverIO.Capabilities | undefined)) | |
| return hasAppCapInFrameworkState() |
… BaseModule (SDK-7093) Addresses the PR #100 DRY review suggestions: automateModule.isAppAutomate() and percyModule.isAppAutomateSession() duplicated the same getTrackedInstance() + [INPUT_CAPABILITIES, CAPABILITIES] hasAppCap loop. Extract it once into BaseModule.hasAppCapInFrameworkState() (the common parent of both modules); both now delegate to it. Placed on BaseModule rather than as an exported util.ts function (as literally suggested) to avoid a circular import: util.ts -> automationFramework.ts -> cliUtils.ts -> util.ts. BaseModule already sits above both modules and can import AutomationFramework + the shared util.hasAppCap without a cycle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
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. |
🤖 SDK PR Review Agent — report
Status on current head
|
| Review finding | Status on 64c67a7 |
|---|---|
🔴 Critical #1/#2 — percyModule still detects App Automate from this.config.app alone |
✅ RESOLVED — percyModule.isAppAutomateSession() is now capability-aware and delegates to the shared BaseModule.hasAppCapInFrameworkState() (commits dc27d93 + refactor 64c67a7). |
🟠 Warning — automateModule multiremote: single worker-wide tracked instance can't discriminate per-session caps in a mixed app/non-app multiremote combo |
service.ts._isAppAutomate() single-instance behavior (which reads one browser's caps the same way). Not newly introduced by this PR; a genuine per-session-scoping fix is a broader follow-up. Flagging for a maintainer decision on whether it blocks. |
| 💡 Suggestion — changeset note omits session-status | ➖ Broadened wording was applied but the repo's changeset auto-regeneration reverts it to name-only (it's driven by the PR body's Release notes section). Can update the PR body if desired. |
The DRY suggestion from the last inline review (extract a shared capability detector) is also applied on 64c67a7 — hasAppCapInFrameworkState() on BaseModule, reused by both modules.
PR Review: wdio-browserstack-service PR #100
Summary
Intent: Fixes App Automate session names (and status) showing/reporting incorrectly when the app is supplied via the appium:app capability instead of the top-level browserstack.app service option. automateModule's old isAppAutomate check read only this.config.app/skipAppOverride (service-level config), so an appium:app-only App-Automate session misclassified as a plain Automate session and its session-name/status PUT was routed to the web automate/sessions/{id}.json endpoint instead of app-automate/sessions/{id}.json, returning a 404 that was silently logged and never surfaced — leaving the static capability name/no status on the dashboard. The fix adds a private isAppAutomate() helper that additionally inspects KEY_INPUT_CAPABILITIES/KEY_CAPABILITIES for appium:app/appium:options.app, mirroring accessibilityModule.isAppAutomateSession, and routes both markSessionName and markSessionStatus through it. Strictly additive to the existing config-level check.
Risk: Medium
2 critical · 1 warning · 1 suggestion | Files reviewed: 3
═══════════════════════════════════════════════════════════════
Findings
Populated mechanically from the union of three independent review passes over this diff — never hand-merged. Confidence 🟢/🟡 and severity are derived mechanically from grounding + category, not chosen by the reviewers.
Two channels. Blocking = Critical + Warning — the must-fix set the Verdict gates on. Non-blocking = Suggestions — polish; best-effort, never gates.
🔴 Critical (Blocking)
| # | Finding | File · Symbol | Confidence | Sources |
|---|---|---|---|---|
| 1 | [Correctness] percyModule.ts still detects App Automate from this.config.app alone — same root cause this PR fixes elsewhere, left unfixed |
packages/browserstack-service/src/cli/modules/percyModule.ts · onAfterCreate |
🟢 | 1 reviewer |
| 2 | [Cross-Module Inconsistency] PercyModule.isAppAutomate has the identical unfixed bug the PR claims to fix elsewhere | packages/browserstack-service/src/cli/modules/percyModule.ts · onAfterCreate |
🟢 | 1 reviewer |
Findings 1 and 2 above were surfaced independently by two different review passes and describe the same underlying gap — presented together below as one issue (the independent convergence is itself a stronger signal, not two separate bugs).
1–2. [Correctness / Cross-Module Inconsistency] percyModule.ts still detects App Automate from this.config.app alone — packages/browserstack-service/src/cli/modules/percyModule.ts · onAfterCreate
Problem:
PercyModule.onAfterCreate()(line 49) setsthis.isAppAutomate = this.isAppAutomate || 'app' in this.config— this is exactly the same service-level-config-only pattern this PR's own description names as SDK-7093's root cause.this.configonPercyModuleis the same genericRecord<string, unknown>object (set viaBaseModule.configure()) thatautomateModule.tsused before this fix.percyModule.tswas not touched by this PR, so for the exact reported config shape — app supplied only viaappium:app/appium:options.app, no top-levelbrowserstack.app, noskipAppOverride—PercyModule.isAppAutomatestill resolvesfalse.That flag feeds directly into
new PercyHandler(...)(line 54), which uses it (Percy-Handler.ts's_isAppAutomate) to choosePercySDK.screenshotApp(...)vsPercySDK.screenshot(...)and theSCREENSHOT_APPvsSCREENSHOTperformance event. So on an App-Automate session identified only viaappium:app, Percy will still call the web-Automate screenshot path instead of the app path — the same class of bug this PR fixes for session name/status, left standing in a sibling module.Cross-checked the other candidate siblings:
service.ts's classic-flow_isAppAutomate()andinsights-handler.ts's_isAppAutomate()already checkappium:app(viabrowser.capabilities) plusskipAppOverride— they're fine.config.tsalso already handles this.observabilityModule.tshas no independent App-Automate detection logic at all — not a sibling.percyModule.tsis the one outlier.Suggested Fix:
GivePercyModulethe same capability-aware check as the newautomateModule.isAppAutomate()/ the existingaccessibilityModule.isAppAutomateSession()— ideally by extracting one shared helper all three modules call (checkingappium:app/appium:options.appagainstKEY_INPUT_CAPABILITIES/KEY_CAPABILITIES, plusisTrue(this.config.skipAppOverride)), rather than leaving three near-identical hand-rolled implementations (one of which — this one — is still wrong).Confidence: 🟢 — directly verified by reading
percyModule.ts:49,baseModule.ts(config shape), and cross-referencingservice.ts,insights-handler.ts,config.ts(all correct) andPercy-Handler.ts(confirms the flag's effect on which Percy API gets called).
───────────────────────────────────────────────────────────────
🟠 Warnings (Blocking)
| # | Finding | File · Symbol | Confidence | Sources |
|---|---|---|---|---|
| 1 | [Graceful Degradation] isAppAutomate()'s capability check collapses to one answer across a multiremote session set |
packages/browserstack-service/src/cli/modules/automateModule.ts · isAppAutomate |
🟢 | 1 reviewer |
1. [Graceful Degradation] isAppAutomate() reads a single worker-wide tracked instance, not the per-session instance — packages/browserstack-service/src/cli/modules/automateModule.ts · isAppAutomate
Problem:
The new capability check resolves the instance viaAutomationFramework.getTrackedInstance(), which keys off${process.pid}:${threadId}only (cliUtils.ts'sgetCurrentInstanceName()) — there is exactly ONE trackedAutomationFrameworkInstanceper worker.wdioAutomationFramework.ts'strackWebdriverIOInstance()explicitly early-returns ("instance already exists") once that single instance is created, so in a multiremote combo (multiple browser drivers created within the same worker),KEY_INPUT_CAPABILITIES/KEY_CAPABILITIESon that one shared instance get overwritten by whichever browser'sonBeforeDriverCreate/onDriverCreatedran most recently (webdriverIOModule.ts).
onAfterExecute()loops overthis.sessionMap.entries()— one entry persessionId, which for a mixed multiremote run (e.g. one instance carryingappium:app, another without it) can be more than one — and callsmarkSessionName/markSessionStatuspersessionId. Each call re-invokesisAppAutomate(), which re-reads the SAME single tracked instance regardless of whichsessionIdis currently being processed. So in a multiremote group with mixed app/non-app instances, every session in the loop gets routed to the SAME endpoint, based on whichever browser's capabilities happen to still be in the shared state — not the specific session's own capabilities. A session whose real capabilities disagree with that shared snapshot silently gets the wrong endpoint (and, per the same swallowed-404 pattern this PR fixes, no visible error).Note: the pre-existing
this.config.app || isTrue(this.config.skipAppOverride)branch (unchanged, still checked first) does not have this problem — it's genuinely global/config-level and was already consistent across all sessions in a run. The new capability-based branch is the one that looks per-session but isn't.Suggested Fix:
Either (a) scope the capability check to the specificsessionId/browser being processed — e.g. storeKEY_INPUT_CAPABILITIES/KEY_CAPABILITIESkeyed bysessionIdor multiremote instance name rather than on the single worker-wide tracked instance, and haveisAppAutomate()accept asessionId; or (b) at minimum, confirm this capability-based branch is scoped to single-remote sessions today and track a follow-up for multiremote, rather than shipping a partial fix silently for that shape.Confidence: 🟢 — verified by tracing
getCurrentInstanceName()→trackWebdriverIOInstance()'s early-return →webdriverIOModule.ts's state-setting →onAfterExecute()'s per-sessionIdloop. Related to the documented anti-pattern class "SDK actions target the wrong test instance, causing silent data loss or misattribution" (.claude/knowledge/workspace/shared/anti-patterns/sdk-anti-patterns.md#17).
───────────────────────────────────────────────────────────────
💡 Suggestions (Non-blocking — never gates the verdict)
| # | Finding | File · Symbol | Sources |
|---|---|---|---|
| 1 | [doc-polish] Changeset release note only mentions the session-name fix, not session-status | .changeset/pr-100.md · module |
1 reviewer |
1. [doc-polish] Changeset release note only mentions the session-name fix, not session-status — .changeset/pr-100.md · module
Suggestion:
The code change fixes endpoint routing for BOTH session name and session status PUT calls (both previously misrouted to the web Automate endpoint and silently 404'd for App-Automate sessions identified only viaappium:app). The changeset's customer-facing release note describes only the session-name symptom, omitting that session pass/fail status reporting was equally broken and is now also fixed. Consider broadening it to: "Fixed App Automate session name and status not updating correctly when the app is provided only via theappium:appcapability (previously misrouted to the web Automate endpoint)." Thepatchbump itself is correct — no change needed there.
═══════════════════════════════════════════════════════════════
External Services
No external-contract changes detected.
═══════════════════════════════════════════════════════════════
Per-File Confidence (for reviewers)
| File | Status | Reason |
|---|---|---|
.changeset/pr-100.md |
✅ All Clear | Covered; 1 non-blocking suggestion only |
packages/browserstack-service/src/cli/modules/automateModule.ts |
🔴 Author to Fix 1 | 1 Warning, grounded (multiremote instance-scoping gap) |
packages/browserstack-service/src/cli/modules/percyModule.ts |
🔴 Author to Fix 2 | Same sibling gap independently confirmed by two review passes, both grounded |
═══════════════════════════════════════════════════════════════
What's Good
- The fix is strictly additive: the pre-existing
this.config.app || isTrue(this.config.skipAppOverride)branch is preserved as the first check, so every existing (non-appium:app) configuration is unaffected. isAppAutomate()is a faithful, byte-for-byte mirror of the establishedaccessibilityModule.isAppAutomateSession()pattern over bothKEY_INPUT_CAPABILITIESandKEY_CAPABILITIES, andtsc --noEmitpasses cleanly with no type errors.- Both new call sites invoke
isAppAutomate()from inside their existing try/catch, so even an undefined tracked instance degrades gracefully (logged, no thrown exception into the user's test) rather than crashing — actually safer than the pre-existing unguarded tracked-instance calls elsewhere in the same file. - The
.changeset/pr-100.mdaddition uses a correctpatchbump via the repo's Changesets convention, matching the PR's ticked release-bump box — no stray manual version/CHANGELOG edit.
═══════════════════════════════════════════════════════════════
Verdict
🔴 Fix 3 blocking issues (2 Critical — both on the same percyModule.ts gap, independently confirmed twice; 1 Warning on the multiremote instance-scoping gap in the new helper).
- percyModule.ts (Critical, 🟢): still detects App Automate from
this.config.appalone — the exact bug this PR fixes elsewhere, left unaddressed in a sibling module that feeds Percy's screenshot-API selection. - automateModule.ts
isAppAutomate()(Warning, 🟢): the new capability check reads a single worker-wide tracked instance, so it cannot correctly discriminate per-session capabilities in a mixed multiremote (app + non-app) combo — narrower in scope than the percyModule gap, but a real correctness edge case in the exact function this PR adds.
Both are grounded (🟢) — nothing here is "needs human review" noise; they're concrete, verified gaps with suggested fixes above.
═══════════════════════════════════════════════════════════════
— SDK PR Review Agent
What is this about?
Fixes App Automate session names showing the static
sessionNamecapability instead of the per-test title.automateModule'smarkSessionName/markSessionStatusderived App-Automate-ness fromthis.config.app(the top-levelbrowserstackserviceappoption) alone. When the app is supplied only via theappium:appcapability,this.config.appisundefined, soisAppAutomateresolvedfalseand the session name/status PUT was routed to the web Automate endpoint (automate/sessions/{id}.json) for an App-Automate session — returning a404that is silently swallowed, leaving the static capability name on the dashboard.Both call sites now go through a new
isAppAutomate()helper that also inspects the input + resolved capabilities forappium:app/appium:options.app(checks bothKEY_INPUT_CAPABILITIESandKEY_CAPABILITIES, since the resolved caps BrowserStack echoes back dropappium:app), mirroringaccessibilityModule.isAppAutomateSession. Strictly additive — the existing top-levelapp/skipAppOverridepath is unchanged.Verified on App-Automate iOS (own account): broken build kept the static name + logged the 404; after the fix, sessions get their per-test titles and the PUT hits the App-Automate endpoint with a 200.
Related Jira task/s
https://browserstack.atlassian.net/browse/SDK-7093
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)
appium:appcapability.Release notes (internal): (required — engineer-facing; what actually changed / why)
automateModule.isAppAutomate()now detects App Automate from theappium:app/appium:options.appcapability (input + resolved) in addition to the top-level serviceapp/skipAppOverride, so session name/status PUTs are routed to the App-Automate endpoint instead of silently 404-ing against the web Automate endpoint.Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.