fix(maestro): measure settle outcomes directly

[thymikee]

Summary

Replace the differential settle detector’s tap-duration proxy with the stability loop’s own outcome. The settle-after-tap flow now exercises the retry/settle path and fails only when that loop exhausts its budget.

This removes the cold-simulator false failure where target resolution and dispatch made a healthy tap exceed the settle timeout.

Validation

• pnpm test:maestro-compat
• pnpm maestro:conformance
• pnpm check:affected --run

The device-backed differential remains GitHub-authoritative. A local iOS attempt was blocked by a pre-existing simulator-session claim, which was closed after the check.

15 files changed; scope is limited to Maestro settle outcome instrumentation, its differential flow, and trace coverage.

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	2.53 MB	2.53 MB	+334 B
JS gzip	847.4 kB	847.5 kB	+115 B
npm tarball	973.6 kB	973.7 kB	+106 B
npm unpacked	3.36 MB	3.37 MB	+334 B

npm unpacked components

Component	Base	Current	Diff
JS / dist source	2.68 MB	2.68 MB	+334 B
Apple runner source/project	581.1 kB	581.1 kB	0 B
macOS helper source	54.8 kB	54.8 kB	0 B
Android helper artifacts	0 B	0 B	0 B
Other package files	45.6 kB	45.6 kB	0 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	26.7 ms	27.0 ms	+0.3 ms
CLI --help	73.4 ms	72.6 ms	-0.8 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/session2.js	+239 B	+92 B
dist/src/perf-runtime-plan.js	+95 B	+23 B

Top changed packed files

Packed file	Base	Current	Diff
dist/src/session2.js	216.4 kB	216.6 kB	+239 B
dist/src/perf-runtime-plan.js	64.3 kB	64.4 kB	+95 B

[thymikee]

Reviewed exact head fbfcb08b65ab0504ef634b0dbae29b07c58781c7. Blocking regression-validity gap: the detector’s shipped path is not pinned. waitForTypedSnapshotStability(...).settled and the metricAtMost evaluator are unit-tested separately, but no runtime-port test drives an exhausted tap settle and asserts readMetrics().settleTimeouts === 1 (and its trace delta). Deleting the production increment leaves the new tests green. Add an observed-red runtime regression for a non-settled tap, ideally including trace/invariant propagation, then green it. Exact-head iOS Smoke is also red with TEXT_INPUT_COMMIT_NOT_OBSERVED; rerun/classify it and provide the requested device-backed differential evidence.

[thymikee]

Code quality review

The direction is right: a duration proxy that folds in target resolution and dispatch cannot distinguish a slow tap from a stability loop that never latched. But three properties make the replacement detector weaker than the proxy it replaces, and one of them I was able to demonstrate rather than argue.

1. The new invariant is vacuously satisfiable; the old one was not

stepDurationBelow could never be satisfied by "the measured thing did not happen" — every step carries a real durationMs. metricAtMost: settleTimeouts <= 0 can be. runtimeMetricsDelta (packages/maestro/src/internal/replay-plan-execution.ts:115) emits settleTimeouts unconditionally, so every tapOn step carries settleTimeouts: 0 whether the stability loop latched or never ran at all. The no-data guard added at invariants.test.ts:64 therefore cannot fire on a real trace; it only fires on a synthetic one.

So the sole bug-class-4 detector (scenarios.ts:98-107) reports held in both of these worlds:

• the tap's stability loop ran and latched promptly (intended), and
• no settle ran inside the tap step at all.

This is precisely the failure mode the sibling scenario already documents from #1300 — "outcome parity cannot see a retry: a tap that never re-taps passes just the same, which is how this scenario spent its first two runs proving nothing" (scenarios.ts:141). The fix there was a companion metricAtLeast proving the path actually ran. settle-after-tap has no equivalent.

Suggest pairing the bound with a proof-of-life invariant on the same scenario: metricAtLeast hierarchyCaptures >= 2 on tapOn (the stability loop always costs a baseline plus at least one poll capture), or record a settleLatches counter and assert >= 1. Then held means "a settle ran and latched" instead of "nothing contradicted me".

2. Three of four settle sites are instrumented; the missing one is the default path

waitForTypedSnapshotStability has four callers. Three increment the counter (daemon-runtime-port.ts:81, :226, daemon-runtime-tap.ts:139). The fourth — settlePending at daemon-runtime-port-snapshot-source.ts:113 — discards stable.settled.

That is not a marginal caller. Per ADR-0015 retryTapIfNoChange defaults to false, so an ordinary tap takes the !policy.retryIfNoChange branch (daemon-runtime-tap.ts:81-88), which only calls requireStability; the settle then happens in beforeExecute → settlePending on the next command. For the default tap the metric named settleTimeouts is structurally 0, and a timeout that does occur is attributed to the following step.

This is also why the flow had to grow retryTapIfNoChange: true — it pulls the settle inline so the tapOn step can own it. Worth stating in the flow comment, because as written it reads as a stylistic choice rather than the thing that arms the detector.

Two ways out: instrument settlePending as well (needs the metrics object threaded into the snapshot source), or make the omission unrepresentable. StableMaestroSnapshot currently lets a caller silently ignore settled — which is exactly what happened at the fourth site. Having waitForTypedSnapshotStability own the counter, or returning a shape callers must destructure, would have turned that into a compile error.

3. The increment has no coverage anywhere

Every settleTimeouts assertion in the tree is 0. I checked by deleting all three increment statements and running the suite:

# all three `if (!stable.settled) metrics.settleTimeouts += 1;` removed
$ npx vitest run --project unit-core packages/maestro src/daemon/adapters/maestro
Test Files  45 passed (45)
     Tests  327 passed (327)

The pure evaluator is well tested against synthetic traces, but nothing tests that the runtime ever writes a non-zero value. Combined with the PR body noting the iOS device attempt was blocked, the write path is right now verified by nothing — no unit test, no device run. A port-level test driving tapOn against an always-changing snapshot and asserting settleTimeouts: 1 from readMetrics() would close it; the ingredients already exist in the new observation test.

4. settleTimeouts conflates two different outcomes

At daemon-runtime-port.ts:220 the loop is given Math.min(settleTimeoutMs, remainingMs). scrollUntilTypedMaestroTarget invokes scroll whenever remaining > 0, so late in a scrollUntilVisible the settle can be handed a 30ms budget and then record a "settle timeout" for a loop that was never given the settle budget. The counter mixes "the UI would not go quiet" with "the enclosing command ran out of time". Harmless for the tapOn invariant today; a trap for the next metricAtMost written against scrollUntilVisible.

5. The flow guard is a source regex, and is not itself guarded

invariants.test.ts:150 uses assert.match(source, /retryTapIfNoChange: true/) because the conformance parse model drops the field — the deepEqual at :146-149 has no such key and still passes. So the guard also accepts the flag sitting in a comment or attached to a different command. And the mutation test at :166 covers changing the tap target and inserting a scroll, but not deleting the flag — the single edit that, per finding 1, silently disarms the detector. Worth adding:

assert.throws(() => assertSettleFlowSemantics(flow.replace(/\n\s*retryTapIfNoChange: true/, '')));

On the stated motivation

"Removes the cold-simulator false failure" holds for the resolution and dispatch component, which is real progress. It does not fully hold for the settle loop itself: the 2000ms budget is wall clock and includes hierarchy capture time, so a cold simulator whose captures take several hundred ms each can still exhaust it on a healthy multi-frame transition and record settled: false. Narrower blast radius, not zero — and unconfirmed in either direction until the device-backed differential runs.

Minor

• facade-execution.ts:52-58 re-declares MaestroRuntimeMetrics structurally inside the same package. readonly runtimeMetrics?: MaestroRuntimeMetrics drops a hand-synced copy.
• invariants.ts:26-30 MetricKey is a third copy of the same key set — keyof MaestroRuntimeMetrics if the test tree can import it.
• daemon-runtime-tap.ts:74 and :122 spell the counter shape inline twice; Omit<MaestroRuntimeMetrics, 'hierarchyCaptures'> would let the compiler enumerate it. A fifth metric currently costs five manual edits, only some of them compiler-guided.
• invariants.ts:22 jsdoc still lists three metrics.
• The comment deleted from the metricAtLeast branch ("the strongest single step is what proves the path ran") explained why Math.max is correct. That reduction now carries two opposite meanings — worst step for atMost, best step for atLeast — and the shared peak name reads as neither.

---

Verified locally on fbfcb08: pnpm maestro:conformance (61/61) and the maestro/adapter vitest projects (327/327) both pass as shipped; the mutation run above is the only deliberate modification and was reverted.

[thymikee]

Addressed in 7f5df7f.

• Added settleLatches and require tapOn to record at least one latch as well as zero exhausted full-budget settles.
• The runtime now records inline, text, and deferred settle outcomes; the shortened scrollUntilVisible budget is deliberately not counted as a settle timeout.
• Added a runtime-port regression with an always-changing hierarchy. I removed the production increment and observed it fail (settleTimeouts: 0, expected 1), then restored it. The replay trace test now carries a real nonzero latch delta.
• The flow guard now parses retryTapIfNoChange and rejects removing it, rather than using a source regex.
• Revalidated with pnpm test:maestro-compat (328 tests), pnpm maestro:conformance (62 tests), and pnpm check:affected --run.

The device-backed differential and iOS Smoke remain GitHub-authoritative. The new exact-head CI run is pending; I am not treating this as merge-ready until those lanes report.

[thymikee]

Reviewed exact head 7f5df7fbf1f472deb1542ec9dcfc72d711c8c64b. No actionable code findings.

• The detector now records the stability loop's actual settled outcome without changing its cancellation/error behavior.
• The scenario requires both proof that the inline loop ran (settleLatches >= 1) and zero exhausted settles, and the trace carries per-step metric deltas.
• The regression drives an always-changing tap settle and observes settleTimeouts: 1, so reverting the instrumentation is not vacuously green.
• Exact-head CI is green, including iOS simulator smoke.

Residual merge-readiness evidence: the device-backed Conformance Differential workflow runs on main, not this PR head, and its latest pre-PR run used the retired duration proxy. Code/CI are clean for human review, but do not treat that scheduled evidence as exact-head validation.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-31 11:38 UTC