refactor: replace Maestro compat with typed direct engine

[thymikee]

Summary

Replace the production Maestro YAML lowering/replay trampoline with a source-preserving typed program, immutable replay plan, and direct runtime port.

The new engine owns Maestro control flow, variables, target policy, assertions, and lifecycle semantics while reusing shared agent-device gesture and platform backends. The old SessionAction conversion, private __maestro commands, positional decoding, hidden caches, recursive dispatch, and runtime fallback were removed. Android snapshots now fail closed on missing, invalid, stale, or system-chrome-only helper output; persistent-to-command-scoped fallback remains within the same bundled helper contract and never invokes the legacy snapshot engine.

iOS snapshot presentation also narrows React Native LogBox/RedBox Dismiss targets to the innermost action geometry for all consumers, preventing wrapper bounds from misrouting taps.

Validation

• Android API 36 Pixel 9 Pro XL: react-native-pager-view 9/9 in 430.4s; react-navigation 38/38 in 493.3s. Snapshot evidence reported android-helper 0.19.3 with persistent-session transport.
• iOS 26.2 simulator: react-navigation 38/38 in 814.6s.
• iOS 26.2 simulator: react-native-pager-view 4/9. All five remaining failures reproduce unchanged with upstream Maestro 2.5.1 on the freshly rebuilt app, including the suite cleanup label mismatch; no agent-device-only divergence remains.
• Review fixes: focused Maestro, gesture geometry, PNG worker, and replay failure suites passed 182/182; build, format, typecheck, lint, and fallow passed.
• The aggregate unit and coverage runs were affected by host-load timeouts in unrelated Metro, runtime-hint, Apple cache, and recording fixtures. The review-related Maestro replay file passed 69/69 in isolation, and all changed tests passed in the coverage run.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-15 19:26 UTC

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	1.7 MB	1.7 MB	+26.6 kB
JS gzip	550.9 kB	557.5 kB	+6.6 kB
npm tarball	665.2 kB	673.0 kB	+7.8 kB
npm unpacked	2.3 MB	2.4 MB	+31.3 kB

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	26.8 ms	26.8 ms	-0.0 ms
CLI --help	58.2 ms	57.6 ms	-0.6 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/session.js	+25.7 kB	+6.7 kB
dist/src/internal/daemon.js	+3.8 kB	+1.1 kB
dist/src/registry.js	+1.5 kB	+539 B
dist/src/tv-remote.js	-816 B	-452 B
dist/src/runner-client.js	+564 B	+146 B

[thymikee]

CI blocks review on 7352d329:

• Fallow reports new production-unused exports: MAESTRO_NEW_ISSUE_URL and pointFromPercentInFrame.
• Provider integration and coverage fail broadly because the bundled Android snapshot-helper artifact is missing; Maestro, Android lifecycle, find, and related scenarios then fail through the real helper-required path.
• iOS smoke fails testSinglePointerFlingFallsBackToXCTestCoordinateDragWhenPrivateSynthesisFails: all expected fallback metadata is nil, so the changed routing is not exercising the asserted fallback.

These are branch failures requiring owner action, not a readiness review. Please address them and update the head; review should resume only after the relevant checks pass.

[thymikee]

Independent review pass at 0bd3c0186 (verified in-tree, not from commit messages)

Reconciled backlog — all items confirmed present

• Percentage coordinates: integer-only regex with explicit "must be whole numbers" rejection (matches upstream, which throws on "50.5".toInt() rather than truncating input); Math.round/Math.trunc split in runtime-port-geometry.ts unified to truncation, matching upstream's integer division.
• Target-swipe direction default: defaultDirection preset deleted (upstream YamlSwipeDeserializer requires direction; the driver-level default it previously cited is unreachable from YAML).
• pasteText: de-advertised (removed from the parser map) rather than shipped with wrong semantics — upstream takes no argument and pastes the copyTextFrom/setClipboard clipboard.
• ${...} interpolation: undefined and cyclic references now throw INVALID_ARGS. The single lenient path (resolveDeferred) is plan-build-time static decisions on runFlow.when.true, strictly re-resolved at execution — sound two-phase design, not a silent hole.
• optional: one shared interpreter boundary, suppression gated on the dedicated isMaestroTestFailure classification (not the retryability set), cancellation checkpointed first, and the propagates AMBIGUOUS_MATCH from an optional target command regression test present. Matches the agreed policy: Maestro resolution keeps first-match/index semantics; ambiguity from nested public commands is never suppressible.
• waitForAnimationToEnd: dedicated screenshot-stability operation whose captures bypass ordinary stabilization. Threshold units independently confirmed: upstream delegates to romankh3 image-comparison, whose getDifferencePercent is 100·Σdiff/(3·255·W·H) — a 0–100 scale — so the pinned 0.005 really is 0.005%, and the implementation replicates the upstream formula exactly. (An earlier audit gloss of "0.5%" was wrong.)
• ADR 0015: now honestly describes the current pinned harness, specifies the three-layer oracle (JVM-generated semantic vectors included) as the accepted follow-up, and contains the ambiguity-placement and retry-clamp wording as agreed.

Size report (+22.6 kB unpacked): not dead weight

Rebuilt base and head and diffed all 132 output files:

• No chunk duplication — the engine's distinctive strings each appear in exactly one chunk (session.js); the old engine is fully deleted from dist.
• Conformance fixtures/scripts do not ship in the tarball.
• session.js +22.7 kB tracks the engine's legitimate growth (measured: 5,595 → 6,782 production lines in src/compat/maestro, +21%): source provenance per IR node, observation-generation protocol, screenshot-stability op, retryIfNoChange, optional classification, structured failure evidence.
• The new compatibility-policy.js chunk (9.7 kB) is mostly relocation: shared replay-variable/preset code now imported by both cli.js (export flow) and session.js; other chunks shrank correspondingly.
• Wire cost: +4.5 kB gzip / +5.7 kB tarball; startup medians unchanged.

Why the engine is bigger than the old one (and why that isn't over-engineering)

The +21% splits into two roughly equal halves:

1. Semantics the old engine didn't have, or had wrong. At main, waitForAnimationToEnd lowers to a positional string action into a generic runtime wait — no screenshot diff at all, so its core semantic was wrong. retryTapIfNoChange was explicitly rejected (there's a test asserting the rejection) even though upstream defaults it to true. optional was partial; atomic iOS selector dispatch, deferred gesture stabilization, wrapper-chain normalization, integer-percent validation, and ranked failure suggestions didn't exist.
2. Explicitness replacing hidden state. The old engine was compact because it was implicit: string actions with positional payloads (['15000']), execution through the replay trampoline, compatibility state in three WeakMap caches, a second selector resolver. That compactness is where the bugs lived — ADR 0015's own complaint was that successful responses were hard to prove. The per-command allow-list parse layer and the evidence/generation protocol are bigger than what they replaced because they check what the old code assumed.

Calibration: the PR is net-negative in repo lines overall (−15.1k vs +14.5k — the deleted trampoline/caches and old tests offset the growth), and the wire cost is +4.5 kB gzip (+0.8%) since type declarations and validation messages compress to almost nothing.

The one structurally debatable piece is maintaining both the direct interpreter (engine*) and the compiled replay-plan representation (replay-plan-*, ~900 lines) — two execution surfaces for one language. That's the cost of replay resume/digest/progress integration, and the ADR's rejection of a shared replay VM until a second caller needs it is the right call; it's the first thing to revisit if native .ad replay ever grows structured control flow.

Pre-merge watch list (agrees with the "reviewable, not merge-ready" status)

1. iOS wrapper-chain normalization before index is the riskiest semantic change in this push (alters selector matching) — the full 38-flow iOS run is the right evidence for it.
2. tapOn retryIfNoChange final adjustment is unit-tested but not yet device-rerun.
3. The two iOS runner lifecycle fixes (prewarm reset after URL open; target refresh after relaunch) reach outside compat/ — they need the non-Maestro gesture canaries the ADR performance contract already mandates, since they affect every iOS session.
4. The horizontal-swipe preset deviation (avoiding iOS interactive-back) is a deliberate, documented parity break — the future app-observable oracle layer should encode it so it stays intentional.

Scope rule remains: new correctness defects in advertised surfaces block; everything else goes to the oracle backlog.

[thymikee]

🔬 Thermo-nuclear code-quality review

Structure/maintainability pass (not a behavior/parity re-audit) over the whole diff, done with six focused reviewers over disjoint slices (parser · replay-plan · runtime-port · engine/target · daemon · export/oracle), with every lead finding independently re-verified against the tree at ead2022ed.

Verdict: the architecture is sound — this is not a redesign. The typed program → immutable plan → runtime-port split is a real improvement over the old trampoline, and several things I went in expecting to be messy are clean (see below). What remains is a set of high-conviction extract/delete moves — mostly duplication that has already drifted, plus a couple of drift-traps with behavior consequences.

What's genuinely good (worth protecting)

• Decomposition respects the size budget. ~45 files in src/compat/maestro, none over 1k lines (largest export-flow.ts at 547). No file crossed the 1000-line line. Dispatch is properly table-driven (MAESTRO_RUNTIME_COMMAND_HANDLERS … satisfies), and the executeMaestroProgram/executeMaestroPlan facade is a clean 45 lines.
• Type hygiene is excellent: 0 any, 0 @ts-ignore, only 21 thin as X casts and 1 as unknown across the whole module.
• No feature-logic leak into shared daemon paths. Maestro sits behind exactly one seam — isTypedMaestroReplay() at session-replay-runtime.ts:158, which delegates and returns; the Maestro identifier appears in zero shared handlers. Where the generic and Maestro paths share code they do it through purpose-built seams, not if (isMaestro) branches. This is the thing I most expected to be wrong, and it isn't.
• The replay-plan layer earns its keep — it does real compile-time work with no IR equivalent (runFlow include I/O in replay-plan-steps.ts:169, static when evaluation → omit/flatten/opaque, content digest for resume). It's not a pass-through remap.

---

Tier 1 — highest-value structural moves

T1 · runReplayScriptFile is a 260-line body with a suppressed complexity lint, not a decomposed orchestrator — session-replay-runtime.ts:138-398.
Line 138 is // fallow-ignore-next-line complexity. The team's own linter flagged this and it was silenced rather than split. Nearly every concern inside it is already a well-named top-level helper (preflightReplayAgainstActiveRepair, resolveReplayEntryIndex, createReplaySaveScriptArmer, …) — they're just hand-sequenced inline instead of composed by a smaller driver. Split into prepareReplayRun → executeReplaySteps → finalizeReplayRun; each becomes independently testable and none needs the suppression. A silenced complexity lint is an admission the decomposition is owed.

T2 · Opaque plan steps carry a dead, uncompiled IR subtree — wasted work + a latent landmine — replay-plan-steps.ts:133/143/153, type replay-plan-types.ts:19-23.
For every repeat/retry/opaque runFlow, opaqueStep(plannedCommand, …) stores the raw command with its full commands[] subtree intact as step.command, and separately stores the compiled body. Execution only ever reads body + the scalar control fields (executeOpaqueStep, replay-plan-step-execution.ts:151-184; confirmed command.commands is read nowhere in the module). But executeOpaqueStep calls resolveCommand(step.command) (:155), and resolveValue (engine-flow.ts:152-160) deep-walks arrays/objects calling context.resolve() on every string — with failOnUnresolved=true (engine-context.ts:124). So the entire dead subtree is eagerly re-interpolated under the outer scope on every visit: not just wasted work, but a ${VAR} scoped to an inner body step would throw at the opaque boundary. It's also cloned, frozen, and hashed into the digest. Strip commands/include.commands before storing — let body be the sole representation of the nested sequence. This is the one place the plan layer should delete IR content, not carry it.

T3 · Delete parseScalarCommand — it's a second, hand-synced command dispatch table — program-ir-command-parser.ts:83-107.
The scalar switch hard-codes { kind, source } for the 7 no-arg commands, but every one of those commands' value-parsers already handles isNullNode(value) returning the identical result (verified: parseLaunchApp:162, parseEraseText:223, parseScroll, parseHideKeyboard, parseBack, parseWaitForAnimationToEnd:431, parseStopApp:450). Route bare scalars through parseCommandValue(node.value, null, node, context) and delete the whole function + its parallel table. Behavior-preserving for valid input; malformed bare commands even get a better domain error than the generic "not supported".

T4 · 75× hand-rolled ...(x === undefined ? {} : { x }) — reinvents an existing helper — 75 occurrences across the module (14 in program-ir-command-parser.ts, 13 in program-ir-gesture-parser.ts, 11 in runtime-port-commands.ts, …).
The repo already ships stripUndefined() (src/utils/parsing.ts:146) and compactRecord() (src/commands/command-input.ts:506) for exactly this, and they're used this way elsewhere (client-normalizers.ts, command-flags.ts). exactOptionalPropertyTypes is off, so there's no type-level excuse, and stripUndefined produces the same absent-key result the plan digest needs. Replace each builder with stripUndefined({ kind, source, a, b }) — the actual field list stops being buried in 3-token spreads. (This also dissolves the worst offender, the nested-ternary delay logic in tapOnInput, runtime-port-commands.ts:224-240.)

---

Tier 2 — duplication that has already drifted (extract one seam)

#	Where	What's duplicated	Remedy
D1	runtime-targets.ts:52-72 & :107-120 & session-replay-maestro-failure.ts:231-247	The match→filter→normalize→select candidate pipeline is hand-assembled 3×	Export one rankMaestroCandidates(...); all three call it
D2	runtime-target-matching.ts:10-15 vs runtime-target-policy.ts:28-53	File named for matching is a 1-line filter wrapper; the real matchesMaestroTypedSelector predicate lives in policy.ts	Merge matching.ts into policy.ts (4 target files → 3)
D3	runtime-targets.ts:96-126	iOS atomic-dispatch feasibility (countCanonicalDispatchCandidates) bolted into the general resolver every command pays for	Move to the iOS tapOn path in runtime-port-commands.ts; the resolver shouldn't know this feature exists
D4	runtime-port-geometry.ts:93-94,153-154 vs contracts/scroll-gesture.ts:245-246	percent→pixel conversion duplicated and internally inconsistent: within screenSwipeEndpoints, horizontal swipes round (shared pointFromPercent, Math.round) while vertical swipes truncate (local pointInViewport, Math.trunc)	One rounding rule for the layer — and it must be trunc (upstream Maestro integer-divides), so the horizontal→round path is the outlier to fix, not the direction the raw "route everything through pointFromPercent" instinct suggests. Behavior-affecting.
D5	daemon-runtime-port.ts:143-155	Settle-after-mutation contract is replicated per-closure and has drifted: doubleTapOn/longPressOn only invalidate(), skipping the requireStability()/settle every other mutating op does (tapOn, gesture, scroll, inputText)	Drive settle from the single invokeMutation seam (it already knows it mutated) so all mutating ops settle by construction. If the omission is intentional it's undocumented. Behavior-affecting.
D6	daemon-runtime-port-observation.ts:326-333 vs runtime-port-observation.ts:38-41	visible/notVisible match predicate written verbatim twice	Export/share conditionMatches
D7	daemon-runtime-port.ts:291-299 vs …-observation.ts sleepWithinBudget	poll-until-deadline loop 3×; the hottest path (target resolution) is the one copy missing the cancellation normalization the others share	Export sleepWithinBudget; the resolver polling loop uses it
D8	session-replay-divergence.ts:122-140 vs session-replay-target-verification.ts:138-157	resume + stampPendingRecordAndHealWatermark block copy-pasted verbatim (ADR-0012 invariant asserted twice)	resolveReplayDivergenceResume({...}) called from both
D9	session-replay-divergence.ts:508-538 vs session-replay-maestro-failure.ts:192-227	suggestion rank/dedupe-by-basis algorithm duplicated generic vs maestro	Generic rankAndDedupeByBasis<TBasis>(entries, rank)
D10	program-ir-gesture-parser.ts doubleTapOn (131-183) vs longPressOn (185-229)	~50-line "point-or-selector, conflict if both" parser near-verbatim (tapOn is a 3rd instance)	Extract parsePointOrSelectorTarget(...)
D11	export-flow.ts:44-45 vs parser BASE_SELECTOR_KEYS/TAP_SELECTOR_KEYS	Export re-encodes the Maestro selector vocabulary (TEXT_SELECTOR_KEYS/STATE_SELECTOR_KEYS) and launchApp option list independently; no round-trip test	Share the key constants + add a test that re-parses export output through parseMaestroProgram

---

Tier 3 — smaller type / dead-surface cleanups

• Vacuous Extract<…, { kind: string }> + dead runtime guard — replay-plan-steps.ts:236-251. commandStep's param type matches everything, so an AppError throw (the file's only use of AppError) simulates the narrowing at runtime. engine-types.ts already has MaestroRuntimeCommand; type the param as that, delete the throw + the now-unused import.
• runtime-port.ts is a shipped file that only tests use — createMaestroRuntimePort has zero production importers (only runtime-port.test.ts); prod goes through createDaemonMaestroRuntimePort. Delete it; build the tiny {execute, observe} inline in the test fixture.
• Dead tapOn operation fields — label/index/childOf are threaded through tapOnInput (runtime-port-commands.ts:224-240) but consumed during target resolution and never read by the impl. Drop from the operation input type.
• Dead, name-colliding type — ReplayVerifiedTargetGuard in session-replay-target-classification.ts:388-401 is never imported and has a different shape than the same-named type actually used in session-replay-target-verification.ts:62-65. Delete.
• Unsafe cast across a boundary that has a real type — session-replay-runtime.ts:579-605 reads ReplayDivergenceResume back as Record<string, unknown> via two as casts to set repairSessionHeld. One typed accessor instead.
• Identity/pass-through indirection: parseMaestroSelectorEntries wrapper (program-ir-gesture-parser.ts:69); MaestroEngineExecutionOptions = MaestroEngineOptions alias (engine-types.ts:118); resolveMaestroTimingPolicy lives in the engine-flow.ts grab-bag but validates data in compatibility-policy.ts; sortKeysDeep duplicated between replay-plan-digest.ts and replay/plan-digest.ts; the two near-identical keyboard/alertRefFrameEffect resolvers in registry.ts could share a factory.

---

Two codebase-health / scope notes (not line edits)

• The conformance "oracle" doesn't run. scripts/maestro-conformance.test.ts uses node:test, isn't matched by any vitest project (vitest.config.ts unit-core whitelists src/**/*.test.ts + one named file), and maestro:conformance is referenced by no CI workflow or check aggregate. The "upstream" fixtures are hand-typed JSON with sha256 provenance fields that nothing fetches or verifies, covering ~4 of ~20 command kinds — a passing run proves self-consistency of hand-written JSON, not Maestro conformance. I know the ADR already frames the real 3-layer oracle as a follow-up; the ask here is narrower: an unrun suite implies a guarantee it doesn't provide — either wire maestro:conformance into CI, or reframe/mark the checked-in harness as an agent-device parser regression fixture (drop the "upstream"/sha256 framing) so nobody reads it as active conformance coverage.
• Scope hygiene: the registry.ts +117 (and the client-types.ts refsGeneration) is ADR-0014 ref-frame-effect wiring with zero Maestro content — an unrelated cross-cutting feature riding in this branch. Splitting it out would let it be reviewed against its actual purpose.

---

None of the above is a redesign. T1–T4 plus the D-series extractions delete real complexity and drift-traps; treat D4 (geometry rounding) and D5 (settle drift) as correctness rather than cosmetics, since both change device behavior.

[thymikee]

Current head 13bddfab55aefa5e1c8b3f87f3e55e35b21fb71a is still CI-blocked on two actionable failures:

• Fallow Code Quality: src/compat/maestro/engine.ts:16 exports executeMaestroProgram, but production-unused-exports reports no consumer. Remove the export or wire the intended production consumer.
• Coverage: three tests in src/platforms/android/__tests__/input-actions.test.ts fail because text verification reaches captureAndroidUiHierarchy without the bundled Android helper artifact (Android snapshot helper is unavailable: the bundled helper artifact was not found). The tests/fixture need to provide the helper path or mock the helper-backed snapshot boundary consistently.

The remaining completed checks were green when inspected; Linux and iOS smoke were still running.

[thymikee]

Rechecked new head 8d0e28decfa5b29fda5476e865933e536bb6eb6a: the two previously reported CI blockers are unchanged. Fallow still reports executeMaestroProgram as a production-unused export, and Coverage still has the same three input-actions.test.ts failures because the bundled Android snapshot helper artifact is unavailable. Please address both before another readiness review.

[thymikee]

New blocker at head 05dfe06e33a0eb4f3368a7fd2b5914518e4f21f1 despite green CI:

rankMaestroCandidates scopes visible/ranked through childOf, but returns the raw pre-scope matches (src/compat/maestro/runtime-target-ranking.ts:29-40). resolveMaestroTargetFromSnapshot then uses that raw set for evidence and failure selection (runtime-targets.ts:58-62). When the parent exists but matching children exist only outside it, the result incorrectly reports matched: true, a nonzero candidate count, and “matched N element(s), but none were visible” instead of a scoped miss. The previous implementation replaced matches with scoped.matches.

Please return/use the scoped matches for evidence/failure semantics and add a regression for: parent exists, child selector matches only outside that parent. No branch changes were made.