refactor(maestro): load the engine as a managed replay plugin - #4505

Size Report

Startup median (7 runs, lower is better):

At bea660f, two likely defects need checking before this is ready for human review. There are no conflicts.

Could the bundled dependency configuration give the installed plugin a private diagnostics AsyncLocalStorage, causing its direct emitDiagnostic calls to return without recording anything? That would lose maestro_tap_dispatch and maestro_screenshot_compare events in debug and request logs, while source-symlink tests keep passing. Please pass diagnostics through ReplayBackendHost, as with registerSensitiveValue, and pin the host-state boundary with a package check. This conclusion comes from code inspection; it still needs a packed-artifact run.

With an empty plugin store, could startAgentDevice({ plugins: [maestroPlugin()] }) fail to replay a YAML flow because withReplayBackend opens the caller’s store rather than using the embedded host’s registry? A different installed version would also make source collection and execution use different engines. Please give the client the host’s resolver and add an imported-plugin, empty-store embedded test. This path was inspected, not executed.

The 15 failed checks look related: 13 mutation shards lack the plugin installation supplied by the unit-test global setup, and their missing reports fail the aggregate; Repo Guards fails because the compound check:package script is no longer credited by verbatimScripts. Please fix those setup and gate-model paths.

Could an explicit resolver on the client and writer hosts replace the ambient binding, per-flow registries and global test setup? The CLI could share one store scope per command, filtered to the declared package, while the embedded client uses its runtime’s resolver. Would adding an optional ReplayBackendResolver to those host contracts be enough to make that smaller design possible first?

Not blocking: count only active backends in shutdown reports, open Maestro lazily for its fuzz target, and keep the remaining test/type cleanups and #3871 failure-classification work separate.

After the fixes, please attach packed-dist Android and iOS replay results, including selector taps, an animation or screenshot wait, and debug output with both diagnostic events above; also include the ADR 0015 Android timing comparison and the empty-store embedded run. The placeholder publication and trusted-publishing setup remain separate draft blockers.

bea660f now conflicts with the target branch, 0.22. Please rebase onto 0.22 and address the outstanding review in the same push. I will re-check the updated head.

Thanks for the rebase. The two new commits fix the mutation shard and Repo Guards failures from bea660f, and all 32 checks pass at 2e004ce. Two problems from my earlier review (#4505 (comment)) are still in the code, and the live validation is still missing. There are no conflicts.

The bundled engine still loses its diagnostics. packages/maestro/tsdown.config.ts:19 bundles every @agent-device/* import, so the plugin carries its own copy of host-kit diagnostics with its own module-level AsyncLocalStorage (diagnostics.ts:61). The engine calls emitDiagnostic directly (daemon-runtime-tap.ts:235, maestro-screenshot-comparison.ts:74 and :122). In the installed dist that copy has no store, so it returns early and the maestro_tap_dispatch and maestro_screenshot_compare events never reach --debug or the request diagnostics. Those events are the dispatch evidence the ADR 0015 timing work uses. Workspace tests resolve the shared module, so CI cannot see this. The rule should be: the bundled engine reaches per-request or per-runtime host state only through ReplayBackendHost. Please add emitDiagnostic to the host the way registerSensitiveValue is already passed, and add a package-boundary rule that forbids packages/maestro/src from importing host-kit diagnostics, host-kit/request* and runtime slots.

Embedded hosts still split the plugin set. startAgentDevice accepts plugins (host-types.ts:14) but gives them only to the embedded daemon and builds the client with { host } (host.ts:73). The client's replay and test writers call loadReplayScriptSourceBundle outside the runtime slot, so withReplayBackend (replay-backend-registry.ts:40) finds no bound resolver and opens the caller's plugin store. With startAgentDevice({ stateDir, plugins: [maestroPlugin()] }) and an empty store, replaying a flow.yaml with maestro fails with plugin-not-installed. With a different store version, source collection and execution would use different engine versions. run-maestro-flows.md:21 tells users the opposite. The rule should be: source collection and export resolve through the same plugin set as the host that executes. Please pass the embedded runtime's resolver to its client host, let withReplayBackend prefer a host-supplied resolver over the store, and add an embedded-host test with an imported plugin and an empty store.

Before merge, please run the packed plugin dist, not the workspace symlink (scripts/install-workspace-maestro.ts with source=false, or pnpm pack and plugins add), through a daemon started from this head. On Android and iOS, replay a flow.yaml with --maestro that reaches selector taps and an animation or screenshot wait. Show the replay success output, and with --debug show the request diagnostics containing maestro_tap_dispatch and maestro_screenshot_compare. Add the ADR 0015 Android timing comparison against upstream Maestro. Also show the embedded startAgentDevice run with an imported maestroPlugin() and an empty store replaying a YAML flow. I did not run a packed-dist install; both findings come from reading the code, and I did not execute the embedded path. I also could not confirm from the check data that the 13 mutation shards ran rather than being path-skipped, and I did not re-review full replay, test and export parity in this pass.

Not blocking: the Validation section in the PR body still names bea660f, "86 files; 2,393 gross lines"; it could name 2e004ce on 4d75aa4 with 88 files and 1,685 gross lines, and say which review items this push addressed. Take it or leave it.

After the two fixes and the live run, package publication (private: true, trusted publishing) is the remaining draft gate.

I reviewed 284b35f against the earlier findings at #4505 (comment). The code fixes landed: the engine now gets diagnostics and process execution through the host, and the packed embedded empty-store run is covered by packed-runtime 'embedded-worker'. CI is green at this head (25 checks, none failing), and no conflicts are known.

One item is still open. This change moves device-facing Maestro dispatch (selector taps, animation and screenshot waits, runScript children) into a bundled, separately installed plugin. CI drives the packed dist only with a fake invoke (packages/maestro/test/packed-runtime.mjs). No run has reached a real device through a daemon started from this head with the packed plugin installed in the store, and the PR body still says the run is underway. So nobody has seen real tap timing, the screenshot worker on real captures, or the ADR 0015 timing parity. Please install the packed dist (pnpm pack and plugins add of the tarball, or scripts/install-workspace-maestro.ts in source=false mode, not the workspace symlink) and start a daemon from 284b35f or a later head. On one Android and one iOS device, run replay flow.yaml --maestro --debug with a flow that reaches a selector tapOn and a waitForAnimationToEnd or screenshot wait. Paste the success output and the maestro_tap_dispatch and maestro_screenshot_compare diagnostics lines, and add the ADR 0015 Android timing comparison against upstream Maestro. After that, the private:true and trusted-publishing draft gate is the next step before merge.

Not blocking, take or leave: scripts/layering/architecture-ownership.ts:154 still has no rule that keeps packages/maestro/src off host-kit diagnostics, request, runtime-slots and command imports (exempting the child entries already in PROCESS_ENTRIES), so a future direct import would pass the workspace tests; src/host.ts:52 passes the replay resolver through an extra field on the composeDaemonRuntimeDeps return type that the daemon never reads, and a local variable captured in the composeDeps callback would avoid that; and DiagnosticEventInput and DiagnosticLevel are now spelled both through @agent-device/contracts/host-diagnostics and the host-kit re-export in packages/host-kit/src/internal/diagnostics.ts:13, so moving the 8 importers and dropping the re-export would leave one spelling.

The rebase to 0ef1318 keeps all 25 checks green, and the delta is one upstream conformance fixture line from #4538. The packed-plugin evidence asked for in the earlier review (#4505 (comment)) is still missing, so this is not ready to merge yet.

The PR moves device-facing Maestro dispatch (selector taps, animation and screenshot waits, runScript children) into a bundled plugin that users install separately. CI exercises the packed dist only with a fake invoke (packed-runtime.mjs:164). The PR body gives only a summary of a packed run at 284b35f, with no output or diagnostics lines. So nobody has shown real tap dispatch (daemon-runtime-tap.ts:235) or the screenshot worker on real captures from the installed tarball, and ADR 0015 Android timing parity is unchecked.

Please install the packed tarball, not the workspace symlink (pnpm pack and agent-device plugins add , or scripts/install-workspace-maestro.ts with source=false). Start a daemon from 0ef1318 or later. On one Android and one iOS device, run replay flow.yaml --maestro --debug on a flow with a selector tapOn and a waitForAnimationToEnd or screenshot wait. Paste the success output, the plugins list line that shows the store-installed version, and the maestro_tap_dispatch and maestro_screenshot_compare diagnostics lines. Add the ADR 0015 Android timing comparison against upstream Maestro.

After that, the private:true and trusted-publishing draft gate is the next step before merge.

Thanks for the update. The build config change at 2f016c0 looks fine, and all 24 checks pass, including the packed-runtime and package-smoke jobs. One thing is still missing before this can merge.

Device-facing Maestro dispatch (selector tap, animation and screenshot waits, runScript children) now runs from a separately installed packed plugin, but CI drives it only with a fake invoke (packed-runtime.mjs#L164). No run has gone through a daemon started from this head with the packed tarball in the plugin store and a real device. So real tap timing, the screenshot worker on real captures and ADR 0015 Android timing parity are unverified for the installed artifact. The issue and the earlier reviews ask for this before merge. Please pack the plugin and install the tarball, not the workspace symlink. Start a daemon from 2f016c0 or later. On one Android and one iOS device, run replay flow.yaml --maestro --debug on a flow that reaches a selector tapOn and a waitForAnimationToEnd or screenshot wait. Paste the success output, the plugins list line showing the store-installed version, and the maestro_tap_dispatch and maestro_screenshot_compare diagnostics lines. Please also paste the Android timing against the ADR 0015 baseline.

I did not run the build or inspect the packed d.mts, so I inferred the type-only effect from the import kind and the PR body. I could not check live device behavior. The branch has no conflicts.

Thanks for checking the public-type fix. The requested exact-head device/timing acceptance is still open; I am keeping this draft. Here is the concrete evidence already captured, with its actual source attribution rather than calling it a new 2f016 execution.

At source 0ef131824533ee97eed578a8f82c16f2e8485926, the real packed Maestro store reported:

{"backend":"maestro","source":"store","package":"@agent-device/maestro","version":"0.0.0","state":"active"}

Its managed factory was apiVersion 2 / replay-backend / maestro, without a symlink. Plugin tar SHA256 68c00ada2a2b9e4f292cff431a0bdb932690bc7abc5e7e27b89f6485385faf01; entry SHA256 163213f278f194f11e12a18b1fbda5e6e8ef4c39018eade28778045cfb2d2131. The T3-bound 0.21.31 client reached the source-built 0ef daemon (codeOrigin: checkout, codeSignature: graph:126:c5d88f294026f80f51201b848e527af805cafc13). The updated five-step optional-scroll scenario passed both engines on iOS and Android: all four exits 0; optional steps 3/4 warned, final Home passed. That scenario declares no engine invariants.

Earlier selector/animation paths were actually executed at 284b35f, not the new head. Retained raw diagnostic fields include:

{"phase":"maestro_tap_dispatch","data":{"kind":"selector","selectorKey":"text","expectedPoint":{"x":338,"y":822}}}
{"phase":"maestro_screenshot_compare","data":{"purpose":"animation","result":"stable","differencePercent":0.0006035887973621569}}

Those are inherited iOS trace excerpts, not fresh head performance or Android diagnostic attestation. The Android run also used the bundled android-helper 0.21.38-dev / API2; packed include/replay and authored screenshot execution succeeded there. Runtime-byte identity across the later declaration-only fix does not turn these into new executions.

ADR0015 timing is not admitted: unchanged full Pager correctness runs were base 6/9, extracted 6/9, upstream 7/9, with the exact failure locations retained; React Navigation ARM Release/Debug builds are blocked by the pinned Reanimated/Worklets output-path mismatch. I have not timed a passing subset or changed assertions. The Android shutdown route subsequently reported success with console KO while inventory remained booted; cleanup is explicitly partial and that route stopped.

Current 2f016 frozen install/build, affected 42/25 and full 41/25, plus hosted checks passed. These establish code/validation evidence separately from fresh current-head two-platform selector/animation execution, canonical timing and npm placeholder/trusted-publisher clearance. Local evidence roots: C3-report.txt under /private/tmp/c3-4505-0ef1318-20261114/ and /private/tmp/c3-4505-284b35f-3mq5bP/; exact receipts under /private/tmp/v022-six-exact-9n6ttv8l/.

Found 1 test failure on Blacksmith runners:

Failure

^{Need help on this PR? Tag @codesmith-bot with what you need.}

Apex Sentinel · 🟠 Request changes

Since the last review: 1 new · 0 still open · 0 no longer raised

1 finding(s): 1 major.

Required fixes

• A transient replay engine load failure is cached for the daemon's whole life

🟠 Major (1)

Reviewed with this repository's own Sentinel configuration (d38e35e61193).

Earlier reviews

The newly enabled Android smoke lane now supplies concrete head execution evidence, but it is red: the emulator booted and the fixture completed earlier scenarios, then smoke:maestro-clickable-first failed at the valid no-index tapOn replay with INVALID_ARGS, details.reason: plugin-not-installed, details.package: @agent-device/maestro. This is a missing migration prerequisite in the smoke workflow, not an emulator boot failure or evidence that the selector engine executed incorrectly. Failed job.

The workflow calls pnpm gate build and starts CLI/device tests without selecting the optional workspace plugin. I prepared the minimal fix: set the lane's isolated AGENT_DEVICE_HOME and invoke its existing built-workspace installer before daemon cleanup/start. The C4 layer must use its renamed existing installer for both plugins so the subsequent native suite also has its declared execution dependency. No scenario/assertion/timeout/selector fallback is changed. These patches are queued until the owned checkout's integration validation and the active head freezes release; they are not published fixes or passing receipts yet. I will validate the actual install/consumer path and push the corrected layers once, rather than rerun this unchanged failure. This PR remains ready for review with smoke execution still a merge blocker.

At 2f016c0 this branch now conflicts with its base 0.22. Please rebase it onto the current 0.22 and push, so CI and the stacked PRs (#4534, #4535, #4537) run against the right base. The earlier review still applies: the live packed-plugin replay runs on Android and iOS are the remaining merge condition.

Thanks for the update. I reviewed 0158fac against the earlier review (2f016c0 conflict). Most review threads are now fixed, but three problems remain, so this is not ready to merge yet.

The new job-level AGENT_DEVICE_HOME: ${{ runner.temp }}/... in android.yml:34 and apple.yml:39 is invalid. The runner context is not available in jobs.<id>.env, so GitHub rejects both files. Runs 43584109661 and 43584110724 at this head failed with zero jobs ("workflow file issue"). No Android or Apple smoke job runs on this PR, so the new Maestro install steps never execute, and the checks list hides this because the failed runs have no check runs. Please set the home at step scope instead, for example a first step that appends AGENT_DEVICE_HOME=$RUNNER_TEMP/agent-device-<lane>-smoke-home to $GITHUB_ENV. Then confirm both workflows start and their smoke jobs pass at the new head.

The private-package thread is only half answered. The changeset and run-maestro-flows.md now say the package is private, but the hint in replay-backend-declarations.ts:44 and website/docs/docs/plugins.md:61 still tell users to run agent-device plugins add @agent-device/maestro. packages/maestro/package.json is private at 0.0.0 and the changeset bumps only agent-device. A 0.22 CLI therefore removes Maestro replay and points replay --maestro, test --maestro and replay export users at an install that npm cannot resolve. Every surface that names the install command (the error hint, plugins.md, run-maestro-flows.md, the changeset) must name a package the shipping release can install. Please make @agent-device/maestro publishable in this PR: remove private, give it a real version, and add a changeset entry. If publishing is deliberately deferred, keep this PR off the 0.22 release until it happens, and do not pair a working-install hint with a "not yet available" doc.

The live-evidence ask is still open. packed-runtime.mjs:164 drives the packed plugin through a fake invoke only, and no run uses a daemon with the packed tarball in the plugin store on a real device. Selector tap dispatch, animation and screenshot waits on real captures, runScript children and the ADR 0015 Android timing are unverified for the installed artifact. Please install the tarball with plugins add (not the workspace symlink) and start a daemon from 0158fac or later. On one Android and one iOS device, run replay flow.yaml --maestro --debug on a flow that reaches a selector tapOn and a waitForAnimationToEnd or screenshot wait. Paste the success output, the plugins list --json line showing the store-installed version, and the maestro_tap_dispatch and maestro_screenshot_compare diagnostic lines. Also paste the Android timing against the ADR 0015 baseline.

Not blocking: the new it.each in harness.test.ts:253 creates a mkdtemp directory per case and never removes it, unlike its sibling tests, so a try/finally with fs.rmSync would fix it. You can take or leave this.

Simplicity question: could PluginInventoryEntry identity be a discriminated union like PluginCleanupOutcome, that is { kind: 'provider'; provider } | { kind: 'replay-backend'; backend } | { kind: 'unidentified' }? Lease admission would then match provider and unidentified variants by type, and the guard at lease.ts:441 would not be needed. The registry could also hand createPluginClose the list of cleanup owners it will dispose, instead of the daemon re-deriving it from inventory state. A future row-kind mix-up would then fail to compile. It needs a contract change in packages/contracts/src/plugin-inventory.ts, readPluginInventory and the daemon.json readers. This PR introduces the backend field, so it can land before 0.22 ships.

On the other review threads: the android.yml env thread and the apple.yml env thread still apply (first finding). The private-package thread is marked resolved but still applies (second finding). One lower-priority thread still applies: the temp directory and one non-blocking resilience note on registry.ts:290-297 (thread). These do not apply, so please resolve them: execute.ts:203 (runner.close() has only sequential callers) and the fixed-at-head threads r4572488729, r4572488730, r4572488740, r4572488744, r4572488752, r4572488753, r4572488761, r4572488765, r4572488769, r4572488773, r4572488780, r4572488788, r4572488796 (the guard works; the union above would remove it) and r4572513397.

The pull_request CI, Linux, Coverage and Integration lanes pass. The pending action-required check likely comes from the open resilience note on registry.ts, a file this PR changes. The Android and Apple push runs fail only because of the invalid workflow files above.

Before merge, the smoke workflows must start and pass, @agent-device/maestro must be installable from the release, and the live packed-plugin Android and iOS replay output must be attached.

For scope: I reviewed the delta 1a9eadd..0158fac and did not run tests, builds or the packed smoke, so the 46 focused tests and 6 packed cases are unverified by me. The workflow rejection is inferred from GitHub's documented context rules and the run records, not from an annotation text. I did not check whether 0.22 is cut from this branch before the package can be published, and I did not re-read the resolved threads on files outside the delta (replay-plugin-setup.ts, vitest-tmpdir-global-setup.ts, host.ts).