feat(interaction): guaranteed press baselines and --expect readback - #4297

Coverage failed on an eager-closure ratchet: the new evidence module joined the command-runtime entry closure as a static edge. It now loads behind a function-scoped dynamic import (type import kept separate, so types cost nothing), with a follow-up fixing an intermediate scope mistake — verified green locally (typecheck/lint, 135 tests).

🤖 Generated with Claude Code

At 6606bb9, --expect-screenshot-diff does nothing on the shipped routes, and a delivered tap can still be reported as failed. All 16 of 16 checks pass, but they cannot catch this. Before merge, wire the flag into the daemon touch route, make the evidence step never throw, and post a live run.

Right now press 120 640 --expect-screenshot-diff returns no screenshotDiff and no warning. createInteractionBackend never sets captureScreenshot, so capturePairScreenshotFrame returns early. The same happens for click, MCP press and Node client press through the daemon. Under the "absent claims nothing" contract, a pair that was never taken looks like an unsupported platform, so a caller on a PIN pad gets no warning and keeps tapping. The rule: every route that admits expectScreenshotDiff must hold a bound captureScreenshot for the dispatch device. The touch route's request binding must admit it when the flag is set, the way screenshot-runtime.ts does. Please add a daemon-route test that sends press --expect-screenshot-diff through dispatchTargetedTouchViaRuntime and asserts data.screenshotDiff. Every current test injects captureScreenshot into a fake, so they prove the helper and not the route.

compareScreenshotPair runs after dispatch() resolves and only uses try/finally. A truncated after-frame makes decodePng throw COMMAND_FAILED. That error reaches the dispatch catch in interactions.ts and is rethrown, so a tap that landed is reported as failed. The caller retries and enters the digit twice, which is the case this flag exists to prevent. The doc comment ("any decode or capture failure drops the evidence") says the opposite. The rule: nothing after a resolved dispatch may throw out of the evidence lane. Catch around the sleep, capture, import, read, decode and compare, return {}, and keep cleanup best-effort. A test with a second capture that writes non-PNG bytes should leave the tap recorded and the result without screenshotDiff. It fails on this head.

This change alters live dispatch: it bypasses the native ref fast path and the direct-iOS fused route, and brackets the tap with two captures and a 150 ms sleep. The PR body lists typecheck, lint, unit tests and progress checks at an older head (69032ca), with no device run. After the two fixes above, please run through the CLI on one iOS simulator and one Android emulator, against a screen with a static region or keypad. The run needs to show: press <x> <y> --expect-screenshot-diff --json on an effective key with data.screenshotDiff.changedFromBefore true and a changedPixelRatio; the same flag on a dead region with changedFromBefore false and the warning text; press @ref --expect-screenshot-diff --json, which proves the ref bypass still dispatches; and press <x> <y> --verify --json with a failing baseline capture or a tree-blind surface, where evidence has a digest and no changedFromBefore key. Please paste the JSON excerpts and the head SHA into the PR body. I did not run tests or a device myself, so the 150 ms settle and status-bar handling on real backends are unverified.

Not blocking, take or leave: the flag could be a screenshotDiff cell on the existing postActionObservation trait instead of hand-added flag lists, readers and a second bypass check, which would also remove the new 165-line repo guard in favor of rows in the existing descriptor gate; the two full-resolution PNG decodes run synchronously on the event loop when png-worker-client already has async decode; changedFromBefore is ratio > 0, so one animated pixel reads as a landed tap, which needs a doc note or a small minimum ratio; the two near-identical frame-plan fakes should become one, with taps recorded in every "dispatch still succeeds" test; commit b37da26 (the omit changedFromBefore fix) can merge as its own small PR, since the diff is over 1,000 lines; and the title and body describe a "guaranteed fresh pre-tree" baseline that no hunk implements, the user docs and descriptions should say an absent field means no comparison was made, and the dynamic import at interactions.ts:190 could be gated on the flag.

Could the readback be smaller? After splitting out b37da26, put screenshotPair in PostActionObservationPlan with one bypassesFastPaths bit read by maybeTapRefTarget and the direct-iOS eligibility. Let the daemon touch route's binding admit captureScreenshot when the plan asks for it. Then decode and compare through one PNG worker job that never throws into the dispatch result. That would remove most of the new surface and make the feature run. What would have to change first is that the touch-route binding must admit a captureScreenshot operation next to the touch executor. I did not confirm how ADR 0019 operation admission allows a second operation on a touch request, so please check it before starting.

The open review threads still apply, apart from one. P2: absent screenshotDiff in docs, decode throws after tap, capture-failure test stub, silent continue in guard, ratio > 0 semantics and sync decode. Six lower-priority threads still apply too. This one does not apply and can be resolved: flag in buildFlags follows the existing verify/settle pattern, and the readers consult the flag only on press and click.

This now conflicts with main at 6606bb9. Please rebase onto main. The two defects and the live run from the earlier review still apply.

Tracked in #4318, group C (new public surface). We're reviewing the six feature PRs together for coherence and for performance impact on paths that don't use them, and we'll post a direction there. Please hold changes here until then.

Thanks @strzhao. We reviewed this with the rest of group C in #4318. It mixes two changes that should land separately.

1. Missing-baseline honesty: this part is a small, valid fix, and we'd like it now. When the before/after comparison was impossible, changedFromBefore is omitted instead of reported. Please split it into its own PR, rebased onto main. Drop the "guaranteed fresh baseline" claim, because the code doesn't implement it (post-action-surface.ts:55).
2. Screenshot-diff evidence: this needs a design issue before code.
   • Ordinary taps already wait for the helper import (interactions.ts:189).
   • When enabled, it costs two screenshots, a 150 ms wait, two synchronous PNG decodes and a pixel scan on the daemon thread, which can stall unrelated requests.
   • It also isn't reachable today, because the daemon's interaction backend has no captureScreenshot.

What the visual feature needs:

• One owner for the comparison: the shared post-action observation plan.
• Screenshot capability declared through runtime admission, with unsupported targets refused before dispatch.
• Evidence failures after dispatch reported as "unavailable". They must never turn a delivered tap into a failed action, and "pixels changed" must never mean "the action succeeded".
• Budget: zero extra captures or imports when off. When on, at most two screenshots and one bounded wait, with the comparison off the daemon thread and inside the request deadline.
• Coverage through real CLI, daemon and provider routes, including corrupt after-images, cancellation and unrelated animation.

Could you open the split PR for part 1 and an issue for part 2?

Rebased onto main (through 0.21.37). The only conflict was the interaction metadata field list — the plan now spreads main's interactionTargetFields() and appends expectScreenshotDiff. Typecheck, lint, and the interaction/command-surface/daemon execution suites pass at the pushed head. The reshape direction from the 11-08 group C review (short issue first, then the missing-baseline piece) is still what gates this PR; say the word and I'll open that issue.

Two defects from the earlier review (6606bb9, #4297 (comment)) are still open at 7a61db9, and the PR has no device run yet. CI is green: 19 of 19 checks pass. It cannot catch the two defects below, because every readback test injects captureScreenshot into a fake backend, no test drives the daemon touch route with the flag, and no test corrupts the after-frame. There are no conflicts.

createInteractionBackend builds the backend for every daemon touch route without captureScreenshot (interaction-runtime.ts#L194). So press <x> <y> --expect-screenshot-diff, click, MCP press and Node press through the daemon reach if (!runtime.backend.captureScreenshot) return undefined (screenshot-diff-evidence.ts#L113). The response then carries neither screenshotDiff nor the warning. Only the screenshot route binds captureScreenshot (screenshot-runtime-binding.ts#L89-L108). The flag is a silent no-op on every shipped surface, and a PIN-pad caller gets no warning. The rule: every route that admits expectScreenshotDiff must hold a bound captureScreenshot for the dispatch device, admitted through runtime operation admission. Otherwise the request must be refused before dispatch, as the 11-08 direction asks. Please add a daemon-route test through dispatchTargetedTouchViaRuntime that asserts data.screenshotDiff.

compareScreenshotPair runs after dispatch() has resolved, with only try/finally (screenshot-diff-evidence.ts#L148). A truncated after-frame makes decodePng throw COMMAND_FAILED. A readFile error, an import failure or a throwing cleanup does the same. The error leaves dispatchWithScreenshotPair, and the dispatch catch in tapCommand rethrows it. A delivered tap is then reported as failed, so the caller retries and enters the PIN digit twice. The settle sleep at line 149 also takes no signal, so a request canceled after dispatch still waits and captures. The rule: nothing after a resolved dispatch may throw out of the evidence lane, and the lane must honor the request signal and deadline. That means catching around the sleep, capture, import, read, decode and compare and returning no evidence, and passing options.signal to the sleep. The after-frame corruption test (r4497036545) must fail on this head.

Live dispatch also changed. Under the flag, the native ref fast path and the direct-iOS fused route are bypassed, and two captures plus a 150 ms wait bracket the tap (interactions.ts#L184). Nothing proves the readback works on a real backend, and the first defect shows that it currently does not. After both fixes, please run through the CLI on one iOS simulator and one Android emulator. Show press <x> <y> --expect-screenshot-diff --json on an effective key, with changedFromBefore true and changedPixelRatio. Show the same flag on a dead region, with false plus the warning text. Show click @ref --expect-screenshot-diff --json, which dispatches through runtime-ref. Paste the JSON excerpts and the head SHA in the PR body. I ran no tests or devices; the CI status comes from the check results.

Not blocking, take or leave: the before-frame capture now sits between the at-rest confirmation in admitResolvedTarget and the tap (after the #4461 rebase), so a slow screenshot round trip can run after the point was declared at rest, outside the ADR 0039 looks budget. Taking the before-frame ahead of admission, or counting it inside the admission request, would fix that. This is a reasoning concern, not a reproduced misfire. Also, #4338 merged the baseline half, so the title and body still describe a pre-tree guarantee that this PR no longer carries. Retitling to the readback and refreshing the summary, head and file count would help.

The review threads on the missing inconclusive-when-absent note in the docs (r4497036453), the missing catch after dispatch (r4497036487), the tap stub that records nothing (r4497036494), the repo guard's continue (r4497036501), the any-pixel changed rule (r4497036508) and the synchronous PNG decode (r4497036514) still apply, as do five lower-priority threads. The thread on the command-flags.ts pattern does not apply, because it follows the existing verify/settle buildFlags pattern and only the press/click readers consume the flag, so you can resolve it (r4497036570).

Before merge, please open the design issue for the screenshot readback, since the baseline half shipped in #4338, then fix the two defects and post the live iOS and Android run.

Progress at ffff797, with the decision carried to #4509.

Defect 2 fixed. Nothing after a resolved dispatch throws out of the evidence lane: the comparison runs inside a catch that yields no evidence, cleanups are caught, the after-frame settle honors the request signal, and both frames decode in the codec worker (decodePngAsync), not on the event loop. The corruption test the review asked for (r4497036545) pins the delivered-tap case: one tap, no screenshotDiff, no warning. The delivered-tap tests now record the tap, the repo guard refuses a missing descriptor instead of skipping it, the docs say the readback is best-effort, and the any-one-pixel rule is a question in #4509 (r4497036508).

Defect 1 is the design decision. #4509 carries it: admit a capture operation on touch routes, or refuse pre-dispatch where no binding exists. Per the 11-08 direction the design issue comes first; the binding follows its outcome, and the live iOS/Android runs follow the binding (before it lands, the flag cannot report on a shipped surface — which is defect 1 itself).

Taken as well: the docs' inconclusive-when-absent sentence, and r4497036570 resolved as not applying. The remaining threads (r4497036453, r4497036487, r4497036494, r4497036501, r4497036514) are fixed at this head; r4497036508 routes to #4509. Title and body retitle to the readback once defect 1 lands and the flag reports for real.

I reviewed ffff797 against my earlier review at #4297 (comment). The capture-failure, corrupted-frame and docs concerns are fixed. The silent no-op on daemon routes is not, and there is still no live device run.

The daemon touch backend is built without captureScreenshot in interaction-runtime.ts:194, while interaction-touch-press.ts:217 admits expectScreenshotDiff. So press <x> <y> --expect-screenshot-diff, click @ref --expect-screenshot-diff, MCP press and Node press hit if (!runtime.backend.captureScreenshot) return undefined in screenshot-diff-evidence.ts:115. They return neither screenshotDiff nor the warning. A caller who opted in gets no evidence, and the new best-effort docs sentence now describes this no-op as intended. The rule to satisfy: every route that admits expectScreenshotDiff holds a captureScreenshot bound for the dispatch device through runtime operation admission, or refuses the flag before dispatch with a typed reason. Please enforce it at the one place the daemon admits the flag, not with per-route guards. Please also add a daemon-route test through dispatchTargetedTouchViaRuntime that asserts data.screenshotDiff. If #4509 decides otherwise, please keep the flag off the shipped surfaces until it does.

Under the flag, the native ref fast path and the direct-iOS fused route are bypassed (interactions.ts:154). Two captures and a 150 ms wait now bracket the tap. This changes live dispatch, and no device run exists at this head. After the fix above, please run through the CLI on one iOS simulator and one Android emulator: press <x> <y> --expect-screenshot-diff --json on an effective key (expect changedFromBefore true with changedPixelRatio), the same on a dead region (expect false plus the warning text), and click @ref --expect-screenshot-diff --json on the runtime-ref route. Please paste the JSON excerpts and the head SHA in the PR body.

Of the other open threads, the abort-handling thread still applies, and so do three lower-priority ones. The dynamic-import thread does not apply, because the import is evaluated once and then served from the module cache: #4297 (comment). The threads on the docs sentence, the capture-failure catch, the capture-failure test, the metadata continue, the codec worker decode and the corrupted-frame test are fixed at this head and can be resolved. The changedFromBefore threshold thread is reasonably deferred to #4509. The buildFlags thread is benign.

CI is green with 19 of 19 checks passing, but no CI route exercises the daemon touch route with the flag, so green does not cover the first issue. Before merge, the daemon binding or refusal needs to land with its route test, and then the live iOS and Android runs need to be posted.

Defect 1 is enforced at the one place the daemon admits the flag: dispatchTargetedTouchViaRuntime refuses --expect-screenshot-diff with INVALID_ARGS / reason screenshot-readback-unavailable / dispatched: 'no' before any device work, because the touch routes bind no screenshot capture — and per your rule, the flag stays off the shipped surfaces until #4509 decides otherwise. The route test covers the plan and single-ref forms and asserts no dispatch. The abort-handling thread and the three lower-priority ones are answered on their threads; the dynamic-import thread is resolved as not applying.

The CLI live runs you specified are meaningful once the flag reports on a shipped surface — under the refusal that is the typed refusal JSON on the daemon route, which I can attach next, and the runtime-lane runs (effective key, dead region, runtime-ref click) follow the #4509 binding decision. Title/body retitle to the readback with the next push.

Thanks for the update. At 434e4ad the earlier findings on the delivered-tap tests, the after-frame error handling, the missing-descriptor guard and the PNG decode are fixed. One problem remains, and it is a new one. All 19 checks pass, but no job drives the daemon press/click route with the flag and asserts screenshotDiff, so green covers neither the problem below nor the fix. I ran no tests and no devices.

Every daemon press and click with expectScreenshotDiff now fails with INVALID_ARGS / screenshot-readback-unavailable (interaction-touch-press.ts#L45-L56, called at #L65). The flag is still advertised as a working readback in CLI help (flag-definitions-action.ts#L209-L218), the click/press allowedFlags, MCP metadata, the Node ClickOptions/PressOptions docs, the screenshotDiff response schema and website/docs/docs/commands.md. A caller who reads the help opts in and gets a hard error on every press, which is the PIN-pad case this PR targets. The rule to satisfy: a flag that any surface advertises must have a daemon route that honors it. Either bind captureScreenshot into the touch backend in createInteractionBackend through runtime operation admission, then delete the refusal, add a daemon-route test that asserts data.screenshotDiff, and post the live runs. Or remove expectScreenshotDiff from every surface that ACTION_FLAG_DEFINITIONS, the click/press allowedFlags, the metadata fields, the option types, the response schema and commands.md feed, until #4509 decides. If you bind the capture, the live runs are on one iOS simulator and one Android emulator: press <x> <y> --expect-screenshot-diff --json on an effective key must show changedFromBefore: true with changedPixelRatio; the same on a dead region must show false plus the warning; and click @ref --expect-screenshot-diff --json must work on a runtime ref.

Not blocking, and you can take or leave these: the guarantee matrix still marks screenshotDiffEvidence as enforced on the runtime-selector, runtime-ref and coordinate rows (interaction-guarantees.ts#L290), but the coverage scenarios inject captureScreenshot directly and bypass the daemon route that refuses the flag, so mark it a waived gap tracked by #4509 (or drop it with the flag); and the refusal is a new hand-written guard beside the admission policy, so if it stays, move it into targetedTouchPolicyResponse so refusedBeforeDispatch stamps dispatched: 'no', and delete the now-unreachable direct-iOS eligibility early return and the expectScreenshotDiff pass-through at interaction-touch-press.ts#L239.

Could the PR avoid shipping an advertised flag whose only production behavior is a refusal? Landing the capture binding first would remove the refusal, the dead eligibility guard and the pass-through, and the existing bracket would work on the real route. Otherwise, removing the flag from every surface and the matrix and parking the runtime lane until #4509 decides would avoid carrying about 209 production lines that no production route reaches. Could the baseline also fold into the post-action observation plan as a third evidence kind, so it has one owner?

Of the open inline threads, the abort/deadline thread still applies (it must be fixed when a capture binds), as do the unreachable direct-iOS guard, the cli-flags.ts comment, the tap-stub assertion and the metadata promise (covered by the first finding). These need no further action, so please resolve them: the interactions.ts dynamic import is a memoized lookup; the commands.md sentence, the catch after dispatch, the delivered-tap tests, the descriptor guard, the PNG decode and the after-frame failure test are fixed at this head; the any-pixel rule is deferred to #4509 and moot while every route refuses; the command-flags.ts pattern matches the existing verify/settle flags.

Before merge, the advertised flag and the daemon route must agree, either by binding the capture or by removing the flag from every surface and the matrix.