Skip to content

refactor: extract WebDriver provider package - #1504

Merged
thymikee merged 2 commits into
mainfrom
agent/1490-w1b-provider-webdriver
Jul 31, 2026
Merged

refactor: extract WebDriver provider package#1504
thymikee merged 2 commits into
mainfrom
agent/1490-w1b-provider-webdriver

Conversation

@thymikee

@thymikee thymikee commented Jul 30, 2026

Copy link
Copy Markdown
Member

Summary

Extract BrowserStack and AWS Device Farm behind the private @agent-device/provider-webdriver single-root facade, with client version and one narrow host-command capability supplied by root composition.

Consolidate generic XML parsing, entity decoding, and escaping in the private @agent-device/xml single-root facade. Root Apple, provider-webdriver, Android attribute decoding, runtime hints, usbmux, and JUnit now consume that facade; Android streaming node scanning and Apple plist traversal stay local.

Restore explicit provider regression coverage for all behaviors named in review while preserving static provider loading and public/cloud behavior. Links #1490.

Validation

  • VITEST_MAX_WORKERS=4 pnpm check:affected --run
  • pnpm check:coverage-changed --base origin/main — 4/4 changed executable lines, 100%
  • focused provider package/facade, XML, root composition, provider integration, and coverage tests
  • R11/layering: 4 workspace packages, 22 exports, zero root back-imports; XML deep imports rejected
  • build and pack inspection: one 661.3 kB tarball, zero private runtime dependencies, no provider/XML source or stale webdriver-xml declarations

Live BrowserStack and AWS credential checks were not run locally; deterministic facade/provider transcripts cover their HTTP and host-command contracts.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.91 MB 1.91 MB -505 B
JS gzip 611.8 kB 611.9 kB +118 B
npm tarball 729.4 kB 729.6 kB +197 B
npm unpacked 2.56 MB 2.55 MB -400 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.0 ms 28.6 ms +0.6 ms
CLI --help 60.7 ms 59.2 ms -1.5 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/script.js +43.3 kB +13.4 kB
dist/src/internal/daemon.js -30.5 kB -8.6 kB
dist/src/device2.js -958 B -347 B
dist/src/runner-disposal.js -78 B -41 B
dist/src/context.js -179 B -30 B

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-31 07:10 UTC

Copy link
Copy Markdown
Member Author

Reviewed at 21f6b3c4. The packaging mechanics are right; my concerns are the XML fork and the size of the test consolidation.

What's right

  • package.json exports only ., is private, and depends solely on @agent-device/contracts + @agent-device/kernel — no root back-dependency. Matches the #1478 requirement that deep packages expose only their façade.
  • Adding it to root devDependencies rather than dependencies is the correct call for keeping the packed manifest free of private @agent-device/* runtime deps.
  • Moving provider-webdriver into UNRANKED_ZONES follows the kernel/W0 precedent exactly, and the reasoning holds: once package→root imports are zero, spine back-edge ranking is vacuous and R11 is the real constraint. The check.ts comment updates are honest about that being a reclassification rather than an exemption.
  • fallow-baselines/health.json is a pure path rename, and the packages/contracts/src/device-provider.ts change is a comment-only reference update. Neither smuggles in a contract change.

1. The XML fork — sequencing, given @agent-device/xml is planned

You mentioned @agent-device/xml is being extracted. That resolves the destination, but not what this PR does in the meantime: as it stands it adds a second production XML parser rather than sharing one.

packages/provider-webdriver/src/webdriver-xml.ts (+215) is a reimplementation, not a copy — root src/utils/xml.ts is class-based with text-node accumulation and a decodeEntities flag, while the package version is function-based and attribute-only. But they share clear lineage: the decodeXmlEntities regex is byte-identical and both carry a __proto__ guard. Root utils/xml.ts keeps two live consumers (platforms/apple/core/runner/runner-usbmux-protocol.ts, platforms/apple/core/xml.ts), so merging this leaves two independently-maintained parsers over UI-hierarchy XML.

That matters more than usual because the duplicated surface is security-relevant — prototype-pollution guard, MAX_NESTING_DEPTH, MAX_DOCUMENT_CHARS — and a hardening fix applied to one would silently not reach the other. #1478's STOP list names "a move introduces a second production path" directly, and P2 is concurrently doing the inverse for .ad discovery ordering.

Worth noting the deleted test makes this concrete: test('WebDriver source parser reuses hardened XML parsing') is removed, because after this change it isn't reusing it. The replacement, 'WebDriver source parsing preserves hardened attributes and geometry', does still cover __proto__ rejection and > decoding — so the guard is tested, but the sharing invariant that test existed to protect is gone rather than relocated.

Suggestion: land @agent-device/xml first (or in this PR) and have both consumers depend on it, rather than merging the fork and reconciling later. If it has to land separately, the two divergences the shared package must reconcile are text-node support and the decodeEntities flag, which the webdriver version drops and Apple's consumers may rely on. Skipping CDATA outright, as the package version does, is also a behavioral difference worth deciding deliberately rather than inheriting.

2. Around ten named test behaviors disappear without visible replacement

The two integration files go −693/+363 while the package adds ~94 lines of tests. Consolidating adapter-internal tests into façade tests is legitimate for an extraction, and I confirmed two were genuinely relocated rather than dropped:

  • 'AWS Device Farm adapter lists artifacts for a released lease…''facade artifact lookup uses released provider ids without allocating a runtime'
  • 'Cloud WebDriver allocation preserves create-session failure when cleanup fails'runtime.test.ts 'session allocation preserves its primary failure when provider cleanup also fails' (arguably better — no daemon needed)
  • 'WebDriver scroll frame prefers visible scrollable containers' → moved into webdriver-source.test.ts

But these have no replacement I can find, and several read like targeted regression guards rather than incidental coverage:

  • 'AWS Device Farm endpoint selection skips live-control WebSocket URLs'
  • 'WebDriver session creation retries transient provider failures'
  • 'AWS Device Farm adapter rejects local artifact install until upload support exists'
  • 'AWS Device Farm adapter sends the requested platform in WebDriver capabilities'
  • 'cloud provider adapters declare command capabilities explicitly'
  • 'BrowserStack upload helper returns uploaded app reference'
  • 'BrowserStack session details map provider-hosted cloud artifacts'
  • 'AWS Device Farm artifacts map to shared cloud artifact kinds'
  • 'AWS CLI Device Farm client maps remote access commands'
  • 'default BrowserStack provider runtime builds sessions from daemon request profile flags'

The endpoint-selection and retry ones stand out — "skips live-control WebSocket URLs" reads like it encodes a specific past bug, and that knowledge is lost with the assertion. Could you note which of these are subsumed by the new façade tests or by deterministic provider transcripts, and which are intentionally dropped? If it's the latter, dropping them is a reasonable call to make explicitly, but it shouldn't be implicit in a refactor diff.

Minor

  • provider-definitions.ts shows as +329 added / −311 removed rather than a rename. Worth confirming that's purely import rewrites and not a behavioral edit hiding in a move — the line delta is small enough to be plausible either way, but the diff can't show it.
  • mergeable_state is unstable at this SHA; presumably just pending checks, but worth confirming before it leaves draft.

Generated by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed 21f6b3c4: the package boundary and facade shape align with #1490 W1b—single-root export, kernel/contracts-only package dependencies, narrow root-supplied client-version/host-command capabilities, static WebDriver composition, and no old-path shim. The provider transcript and daemon scenarios cover the package facade well.

Not ready: the exact-head Coverage gate is genuinely red at 2/5 changed lines (40% vs 70%). The uncovered lines are src/provider-webdriver.ts:8-9 and src/daemon/server/daemon-runtime.ts:190; current tests inject test-local dependencies, so they do not prove the root runCmd adapter or daemon artifact-delegate wiring. Add focused root-composition coverage for those seams and rerun Coverage.

Keep this draft/unlabeled as well until applicable live BrowserStack and AWS session evidence is attached; deterministic transcripts do not replace the repository’s device/provider readiness evidence. Residual risk: no separately authorized cross-vendor review was performed.

@thymikee
thymikee force-pushed the agent/1490-w1b-provider-webdriver branch from 21f6b3c to efb54db Compare July 30, 2026 17:17
@thymikee

Copy link
Copy Markdown
Member Author

Implemented the full review on head efb54db93a435f853ea0e5e87bdcb86f6b8d5b65.

Named regression mapping

  1. Endpoint selection excluding live-control WebSockets — cloud-webdriver-provider-regressions.test.ts, AWS endpoint-selection scenario.
  2. Transient session retry — same file, one failed create-session response followed by success and an exact two-attempt assertion.
  3. Unsupported local AWS artifact install — same file, facade allocation plus installApp rejection assertion.
  4. Requested platform capabilities — same file, iOS request asserted in the exact WebDriver capability body.
  5. Explicit command capabilities — cloud-webdriver-provider-adapters.test.ts, exact support levels and provider notes for BrowserStack/AWS allocations.
  6. BrowserStack upload result — adapter and daemon-flow transcripts assert the uploaded bs://uploaded-app is passed to install.
  7. BrowserStack session artifact mapping — adapter facade lookup asserts exact video, Appium log, device log, and provider-session kinds; daemon flow also asserts release artifacts.
  8. AWS artifact-kind mapping — adapter lifecycle asserts exact video, device-log, appium-log kinds.
  9. AWS CLI remote-access mapping — adapter lifecycle asserts the complete ordered create/get/stop/list FILE/list LOG argv transcript through the injected host-command capability.
  10. Default BrowserStack request-profile flags — cloud-webdriver-runtime.test.ts drives the packaged facade through the daemon and asserts the exact platform, device, OS, app, project, build, and session capability body.

Provider definitions audit

The former static definition table is now a facade-owned factory solely so clientVersion and runHostCommand can be supplied as dependencies. Provider identity/order, environment fallback order, request-flag reads, capability construction, app resolution, artifact lookup, and error behavior are unchanged. The exact BrowserStack HTTP and AWS argv transcripts above pin that behavior through the public facade.

XML consolidation

  • @agent-device/xml exports only . via src/index.ts: XmlNode, XmlParseOptions, parseXmlDocumentSync, decodeXmlCharacterReferences, and escapeXmlTextAndAttribute.
  • Parser/token/entity implementation is private under src/internal; tests cover ordered/text nodes, decoded attributes/text, CDATA without entity expansion, declarations/doctype handling, BOM, unsafe names, depth/size limits, numeric/predefined entities, and all five escaping characters.
  • Removed src/utils/xml.ts, packages/provider-webdriver/src/webdriver-xml.ts, src/platforms/apple/core/xml.ts, and stale declaration output. There are no deep or legacy XML imports.
  • Production inventory has one generic parser (packages/xml/src/internal/parser.ts), one generic decoder and escaper (packages/xml/src/internal/entities.ts). All other matches are facade imports/calls.
  • Intentionally retained specializations: Android parseXmlNodeAttributes, a streaming <node> scanner that now delegates entity decoding to @agent-device/xml; Apple visitXmlPlistEntries, which remains Apple-local metadata traversal.
  • Generic escape copies in daemon runtime hints, Apple usbmux, and JUnit were deleted and migrated to the facade with focused package/consumer tests.

Validation

VITEST_MAX_WORKERS=4 pnpm check:affected --run passed in full and then pushed. This includes 616 coverage files / 5,173 tests, 42 provider-integration files / 141 tests, build, typecheck, R11/layering, pack, node integration, progress, and replay compatibility. Changed-line coverage separately passes 4/4 executable lines (100%). The new CI run is active on the pushed head.

@thymikee

Copy link
Copy Markdown
Member Author

Independent follow-up review at efb54db93 is clean. I verified the generic XML implementation is singular behind @agent-device/xml (parser, entity decoder, and escaper), deep/legacy imports are gone, and the retained Android scanner and Apple plist traversal are domain-specific rather than competing parsers. The ten named provider regressions are either restored explicitly or pinned through the facade/daemon transcripts, including exact BrowserStack capabilities and AWS argv/artifact mappings. Root composition coverage now exercises the runCmd adapter and daemon artifact delegate, and changed-line coverage is 4/4 locally. No new code findings. The PR correctly remains draft while exact-head CI finishes and live BrowserStack/AWS evidence remains unavailable.

@thymikee

Copy link
Copy Markdown
Member Author

Exact-head CI follow-up for efb54db93a435f853ea0e5e87bdcb86f6b8d5b65: the failed Android smoke was rerun and passed on attempt 2 in 7m21s. The original failure was a structured wait_capture_stalled timeout after emulator rotation while waiting for the landscape fixture; it did not touch the provider/XML path and did not reproduce. All checks are now green, with no code change needed. The PR remains draft pending the already-disclosed live BrowserStack/AWS provider evidence.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed efb54db93: code-clean. The single-root provider/XML facades preserve the root-owned readVersion/runCmd composition seam; the root AWS adapter and daemon RPC tests exercise the shipped path. The restored facade/provider scenarios explicitly cover the prior BrowserStack and AWS regression contracts, and the XML parser/entity/escaping implementation is now singular with no deep or legacy imports. Exact-head CI is fully green.

This remains draft and not ready to merge: GitHub currently reports a merge conflict with main, so please rebase and rerun checks. Live BrowserStack and AWS session evidence is still required for this provider-facing cutover; deterministic transcripts do not replace it. Residual risk: no separately authorized cross-vendor pass.

@thymikee

Copy link
Copy Markdown
Member Author

Reconciliation follow-up: the merge-conflict requirement from this review is resolved. The branch is rebased onto merged #1506 at exact head e1a540aea22a5be05b85ea9c98c0c9079c2d412a, is currently mergeable, and the full local affected gate passed before the force-with-lease update. Fresh exact-head GitHub CI was queued by that update.

The PR remains draft because the independent live BrowserStack + AWS Device Farm evidence requested here is still outstanding; the available Limrun credential does not substitute for either vendor.

@thymikee

Copy link
Copy Markdown
Member Author

Live AWS Device Farm validation completed on exact head e1a540aea22a5be05b85ea9c98c0c9079c2d412a.

Validated through the source-built CLI with an isolated state directory:

  • connect aws-device-farm created the local provider profile and correctly deferred allocation until open.
  • open com.android.settings allocated a real LG Stylo 6 / Android 10 remote-access session and launched the preinstalled Settings app. No uploaded test app was needed for this provider lifecycle check.
  • snapshot -i --json returned the live 96-node accessibility tree through the WebDriver backend.
  • click 'label="Display"' --settle executed through backend: "webdriver" and produced a settled semantic diff showing the Display screen.
  • close released the provider session and returned its AWS Device Farm session identity.
  • AWS reported the session terminal state as COMPLETED / STOPPED.
  • A post-close artifacts --json transitioned from pending to ready and returned eight provider artifacts, including video, Appium server output, device logs, TCP dump logs, and the automation/message log.
  • disconnect then cleared the temporary connection profile; no live session remains.

This closes the AWS Device Farm live-evidence gap for #1504: real allocation, launch, accessibility retrieval, interaction, release, and artifact retrieval all pass on the current head. BrowserStack live validation remains outstanding.

@thymikee

Copy link
Copy Markdown
Member Author

Follow-up at e1a540ae: live AWS Device Farm evidence is now complete—real allocation, launch, accessibility snapshot, WebDriver interaction, release, terminal-state confirmation, and artifact retrieval all passed. BrowserStack live validation remains outstanding.

The exact-head CI run is currently not green: Android and iOS smoke both failed with structured wait_capture_stalled errors, while Android Release hit a D8 Java-heap OOM. Treat the release failure as runner/resource noise, but rerun or diagnose the two smoke failures before dismissing them because this refactor touches XML/snapshot-adjacent behavior. Keep the PR draft until BrowserStack validation and clean/reconciled exact-head smoke evidence are attached.

Residual risk: no separately authorized cross-vendor review was performed.

@thymikee

Copy link
Copy Markdown
Member Author

Live BrowserStack validation completed on exact head e1a540aea22a5be05b85ea9c98c0c9079c2d412a.

To conserve device minutes, I reused today's CI-built public test-app APK and ran one minimal session:

  • Uploaded the fixture once under custom id agent-device-pr1504-live.
  • connect browserstack created the local provider profile and correctly deferred allocation until open.
  • open com.callstack.agentdevicelab allocated a real Google Pixel 8 / Android 14 App Automate session and launched the app.
  • snapshot -i returned and parsed the live 65-node accessibility tree.
  • click @e46 --settle executed through backend: "webdriver" and produced a settled semantic diff proving Home → Catalog navigation.
  • close released the provider session immediately (roughly 39 seconds from allocation completion to close).
  • Close-time artifact retrieval returned ready video, Appium logs, device logs, dashboard, and public-session records.
  • disconnect cleared the isolated temporary connection profile; no live BrowserStack session remains.

This closes the BrowserStack live-evidence gap. Together with the AWS Device Farm run above, both provider paths now have exact-head evidence for real allocation, launch, source parsing, interaction, release, and artifact retrieval. The remaining red native CI checks are separate and still need rerun/green status before merge.

@thymikee
thymikee marked this pull request as ready for review July 31, 2026 05:46
@thymikee

Copy link
Copy Markdown
Member Author

[P1] Reconcile the governing package decision before merge. #1490 explicitly caps the internal package set through W2 at seven named packages and defines W1b as importing only kernel/contracts/npm/node. This head introduces @agent-device/xml, adds it as a provider-webdriver dependency, and migrates cross-cutting Android/Apple/JUnit consumers.

The single XML facade is technically preferable to the prior parser fork, and I found no additional runtime defect. AWS Device Farm and BrowserStack now both have credible exact-head allocation → launch → snapshot → interaction → release → artifact evidence. But the package graph and accepted W1b scope changed without amending #1490. Update that decision with the XML package rationale, allowed consumers, and revised package ceiling (or split it into an approved follow-up), then let the currently running iOS smoke/Android release jobs finish cleanly. Do not apply ready-for-human yet.

Residual risk: no separately authorized cross-vendor review was performed.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed the governing-decision mismatch in #1490.

The issue now records:

  • why avoiding a second security-relevant production XML parser earns @agent-device/xml as a domain package;
  • its package-root-only facade and the exact allowed consumer categories;
  • the rule that new consumers must demonstrate XML parsing/decoding/escaping needs and that deep imports or parallel implementations remain forbidden;
  • provider-webdriver's amended dependency allowance; and
  • the revised ceiling of eight named internal packages through W2, still a ceiling rather than a target.

The W1b Done criterion and success metric were updated consistently. No production change was required. Exact head remains e1a540ae; CI has now settled at 32 passing / 1 skipped, with no failures or pending checks. No ready-for-human label has been applied.

@thymikee

Copy link
Copy Markdown
Member Author

Clean re-review at e1a540aea22a5be05b85ea9c98c0c9079c2d412a. #1490 now explicitly approves the earned @agent-device/xml package, its root-only facade and allowed consumers, W1b dependency allowance, and the revised eight-package ceiling, so the previous architecture/scope blocker is resolved.

The XML parser/entity/escaping implementation is singular with no legacy/deep production imports. The CLI → root composition → provider facade → daemon/artifact route has substantive regression coverage, including the real runCmd adapter and artifact RPC delegate. Exact-head AWS Device Farm and BrowserStack sessions both cover allocation through interaction, release, and artifacts; all 32 required checks are green. No code findings; ready for human review.

Residual risk: no separately authorized cross-vendor review was performed.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 31, 2026
@thymikee
thymikee merged commit b125435 into main Jul 31, 2026
35 of 38 checks passed
@thymikee
thymikee deleted the agent/1490-w1b-provider-webdriver branch July 31, 2026 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant