refactor(interaction): move post-gesture stability and scroll movement onto observeUntil - #3078
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Thanks for the migration. At 66eaec5 the code has two regressions, and the iOS and Android smoke jobs fail with what looks like the first one. I did not check that the base branch passes the same jobs, and I found no conflicts. The post-gesture schedule at https://github.com/callstack/agent-device/blob/66eaec5/packages/capture-kit/src/post-gesture-stability.ts#L227 uses captureDeadline 'cancel', but the capture ignores the signal. A capture that finishes at or past max(remaining budget, 200 ms) ends the loop as 'stalled', even though it succeeded. Two ordinary cases hit this: a 1.4 s poll on an unsettled screen, and a first capture slower than 1.5 s. Then The scroll schedule at https://github.com/callstack/agent-device/blob/66eaec5/src/daemon/scroll-movement.ts#L68 has the same shape. readOneCapture ignores the signal, and a post-scroll capture of 1.5 s or more, or a later poll that outlives its tail deadline, ends 'stalled'. Then The new scenarios in https://github.com/callstack/agent-device/blob/66eaec5/test/integration/provider-scenarios/scroll-movement-observation.test.ts#L1 use instant captures only. They would pass the same on main and on this head, so they do not guard the timing changes above. No post-gesture test covers a slow capture either. The scenario also times out at the 5 s default in your local run. Please add injected-clock slow-capture cases for both loops as adapter unit tests, and fix or raise the scenario timeout so it runs green. Both device-facing polling paths change, and the description only reports typecheck. Please attach two live runs. First, on an iOS simulator, do a gesture and then Not blocking, take or leave: baselineEndsInDirection in src/daemon/scroll-movement.ts is now eager, so a dynamic import and analysis run on every scroll and scroll_movement_edge_rest_required can fire without any 'changed' reading, so restoring the lazy Could this PR be split? The two loop migrations are the refactor: set captureDeadline 'none' on both, keep changeNeedsRest lazy, and map every non-done end back to main's outcome. The outcomeObservation guarantee column is a separate contract change and might fit better in its own PR. With 'none', the post-gesture adapter would need no stalled or failed branch beyond rethrowing a real capture error. #3072 (observeUntil) would need to merge first. Would it also help if observeUntil defaulted captureDeadline to 'none' and required an explicit opt-in for captures that honor the signal? That would make a signal-blind 'cancel' adapter impossible to declare by accident. Before merge, both schedules need 'none' with the slow-capture tests, the smoke jobs need to pass, and the two live runs need to be attached. |
c128073 to
d476097
Compare
66eaec5 to
578bebb
Compare
|
[claude-fable-5-1] responding on behalf of @thymikee Both regressions are fixed at
Live runs, private iPhone 17 Pro simulator, iOS 26.2, this branch built. The fixture app has no never-settling screen and no tree over 1.5 s on this host, so both used local Safari pages:
The iOS and Android smoke jobs rerun on this head; the |
|
This PR is ready at 938a38c. The stalled-capture route that broke the iOS and Android smoke jobs at 66eaec5 is gone, and all 13 checks now pass, including both smoke jobs. Not blocking, and you can take or leave it: in post-gesture-stability.ts, The live-run numbers come from the PR body. I did not open the raw There are no conflicts. Please merge #3072 (proto/reliability-contract) first, then take this PR out of draft. |
…ment onto observeUntil Restores the two loop migrations, their scroll-movement provider scenario, and the outcomeObservation guarantee cell on top of the engine and readiness wait. Both schedules declare captureDeadline 'cancel', the engine's behavior before the mode existed.
… stalled The post-gesture capture hook takes no signal, so declaring 'cancel' let the loop end stalled on a capture that finished past its deadline and rethrow it, where main judged the late capture. The schedule now uses the default 'none'. Every non-done end maps to main's outcome: the stabilization-timeout warning and the last value, marked unsettled unless its pair agreed. A capture error is rethrown as itself. The loop takes an optional clock for its elapsed-time reads and the engine. Tests with an injected clock pin a capture that overruns the remaining budget (judged, unsettled, one warning) and a first capture slower than the budget (still forms the quiet pair); both fail with the schedule switched to 'cancel'.
…lysis lazy The scroll capture takes no signal, so the movement schedule now uses the default 'none': a capture that finishes past the budget is judged, and only a budget that ends while the verdict still continues maps to budgetExpiredVerdict. A capture error is rethrown. The migration asked baselineEndsInDirection before the first capture. It is lazy again: the edge analysis and its dynamic import run only after a changed reading, as on main. Tests with an injected clock pin a first capture slower than the budget that shows movement (moved) and a later poll that outlives the budget (judged, moved); both fail with the schedule switched to 'cancel'. A third pins that an untouched surface never asks the edge question of the baseline.
…n the scroll scenario The scroll scenario stubs launchctl and ps so the snapshot route resolves one stable app target. That also makes every capture eligible for the Simulator AX bridge, which no scenario provider scripts: the bridge source runs xcrun on the host, builds or locates its binary, spawns it with simctl, and retries its socket until the bridge deadline before the route falls back to the scripted runner. On a host with Xcode that cost more than a second per test, and under the full provider-integration run both tests timed out at 5 s; the scroll loop itself took 8 ms and 204 ms. The scenario now mocks the bridge source to report unsupported at once, as it does on a host without Xcode, so every capture takes the scripted runner. The file runs in 1.5 to 1.8 s in the full project run.
The outcomeObservation waiver said the deferred-outcome mark applied only to navigation-sensitive Android actions and target-authored gestures. The tap paths finalize through finalizeTouchInteraction, which calls markDeferredInteractionOutcome with the tap point: press/click get the Android snapshot-freshness mark, the no-change tap retry when the request sets interactionOutcome.retryOnNoChange, and post-gesture stabilization when the request sets postGestureStabilization. Target drag (gesture drag) gets none. The waiver now names these marks and says the next capture judges them, never the tap's own response. The coordinate cell said inapplicable, but coordinate press/click reach the same finalize call with the same marks and accept --verify/--settle. It now shares the runtime paths' gap waiver, and the bounded gap list gains coordinate/outcomeObservation.
938a38c to
dfe9629
Compare
There was a problem hiding this comment.
5 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/contracts/src/interaction-guarantees.ts">
<violation number="1" location="packages/contracts/src/interaction-guarantees.ts:192">
P3: This waiver claims the direct route "observes no outcome beyond the runner's own report", but the success path finalizes through finalizeTouchInteraction with scheduleInteractionOutcomeRetry defaulting to true, so it sets the same deferred markers (pending-outcome retry, post-gesture stabilization) that TAP_OUTCOME_NOT_OBSERVED_GAP documents as "judged by the next capture, never in this response". Scope the claim to the response ("never in its own response") and name the deferred markers as the sibling does, so the two waivers that the comment says share the same reasoning stay equally precise.</violation>
<violation number="2" location="packages/contracts/src/interaction-guarantees.ts:604">
P2: This shared waiver is inaccurate for the `maestro-non-hittable-fallback` path: `fill` can execute the fallback while carrying `--verify` or `--settle`, and its response can include that observation. Scope the direct-selector waiver separately or update this path’s contract to model the runtime fill case.</violation>
</file>
<file name="packages/capture-kit/src/post-gesture-stability.ts">
<violation number="1" location="packages/capture-kit/src/post-gesture-stability.ts:152">
P2: The loop caches surface metadata by object identity even though capture values are not required to be immutable or unique. If a provider reuses and updates a capture object, later polls compare stale signature/backend data and can report a false settle or no-effect result; read the surface for each observation instead of caching it.</violation>
</file>
<file name="packages/capture-kit/src/post-gesture-stability.test.ts">
<violation number="1" location="packages/capture-kit/src/post-gesture-stability.test.ts:72">
P3: The loop's other timeout shape is untested: every case here passes `needsBaselineDistrust: false` and never triggers a rebase, so nothing exercises the branch where the budget expires after a quiet pair already agreed — the loop then returns a bare `{ value }` with no `unsettled` outcome (post-gesture-stability.ts: the `if (lastPairAgreed) return { value }` path, which comments say is reached when a rebase or distrust verdict keeps polling on an at-rest surface). This is exactly the branch the PR claims to preserve ('budget can expire on a surface that is already at rest'); a regression would silently change when the agent sees the stabilization warning. Add a test with `needsBaselineDistrust: true` where captures keep agreeing on the unchanged baseline past the 1.5s cap, and assert `postGestureOutcome` stays undefined while the stabilization_timeout warning fires.</violation>
</file>
<file name="test/integration/provider-scenarios/scroll-movement-observation.test.ts">
<violation number="1" location="test/integration/provider-scenarios/scroll-movement-observation.test.ts:106">
P3: `scrollEntry()` returns `{ x, y, x2, y2 }`, but `honoredScrollSwipeMidpoint` (packages/contracts/src/scroll-command.ts:122-133) reads `x1`/`y1`/`x2`/`y2`, so the swipe midpoint is always `undefined` here. The comment's claim that midpoint (201, 437) sits inside CONTAINER never materializes, and in the no-progress test the `containerHoldsSwipe` gate in decideEdgeVerdict is silently skipped — the refusal passes for the wrong reason (no container-position verification). Use `x1`/`y1` to match the leaf contract.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| via: 'src/daemon/interaction/internal/interaction-touch-response.ts#buildInteractionResponseData', | ||
| }, | ||
| targetReadiness: DIRECT_IOS_SINGLE_QUERY_READINESS, | ||
| outcomeObservation: DIRECT_IOS_OUTCOME_NOT_OBSERVED_GAP, |
There was a problem hiding this comment.
P2: This shared waiver is inaccurate for the maestro-non-hittable-fallback path: fill can execute the fallback while carrying --verify or --settle, and its response can include that observation. Scope the direct-selector waiver separately or update this path’s contract to model the runtime fill case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/contracts/src/interaction-guarantees.ts, line 604:
<comment>This shared waiver is inaccurate for the `maestro-non-hittable-fallback` path: `fill` can execute the fallback while carrying `--verify` or `--settle`, and its response can include that observation. Scope the direct-selector waiver separately or update this path’s contract to model the runtime fill case.</comment>
<file context>
@@ -558,6 +601,7 @@ export const INTERACTION_DISPATCH_PATHS: Record<InteractionPathId, InteractionPa
via: 'src/daemon/interaction/internal/interaction-touch-response.ts#buildInteractionResponseData',
},
targetReadiness: DIRECT_IOS_SINGLE_QUERY_READINESS,
+ outcomeObservation: DIRECT_IOS_OUTCOME_NOT_OBSERVED_GAP,
},
},
</file context>
| let surfaceCache: CapturedSurface<T, S> | undefined; | ||
|
|
||
| const surfaceOf = (value: T): CapturedSurface<T, S> => { | ||
| if (surfaceCache?.value === value) return surfaceCache; | ||
| surfaceCache = { value, ...hooks.readSurface(value) }; | ||
| return surfaceCache; |
There was a problem hiding this comment.
P2: The loop caches surface metadata by object identity even though capture values are not required to be immutable or unique. If a provider reuses and updates a capture object, later polls compare stale signature/backend data and can report a false settle or no-effect result; read the surface for each observation instead of caching it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/capture-kit/src/post-gesture-stability.ts, line 152:
<comment>The loop caches surface metadata by object identity even though capture values are not required to be immutable or unique. If a provider reuses and updates a capture object, later polls compare stale signature/backend data and can report a false settle or no-effect result; read the surface for each observation instead of caching it.</comment>
<file context>
@@ -116,38 +126,52 @@ export function decidePostGestureStabilityVerdict<S extends readonly unknown[]>(
- // the deadline can expire on a surface that is already at rest.
+ // the budget can expire on a surface that is already at rest.
let lastPairAgreed = false;
+ let surfaceCache: CapturedSurface<T, S> | undefined;
+
+ const surfaceOf = (value: T): CapturedSurface<T, S> => {
</file context>
| let surfaceCache: CapturedSurface<T, S> | undefined; | |
| const surfaceOf = (value: T): CapturedSurface<T, S> => { | |
| if (surfaceCache?.value === value) return surfaceCache; | |
| surfaceCache = { value, ...hooks.readSurface(value) }; | |
| return surfaceCache; | |
| const surfaceOf = (value: T): CapturedSurface<T, S> => ({ | |
| value, | |
| ...hooks.readSurface(value), | |
| }); |
|
|
||
| const outcome = await runPostGestureStabilityLoop({ | ||
| pending: PENDING, | ||
| needsBaselineDistrust: false, |
There was a problem hiding this comment.
P3: The loop's other timeout shape is untested: every case here passes needsBaselineDistrust: false and never triggers a rebase, so nothing exercises the branch where the budget expires after a quiet pair already agreed — the loop then returns a bare { value } with no unsettled outcome (post-gesture-stability.ts: the if (lastPairAgreed) return { value } path, which comments say is reached when a rebase or distrust verdict keeps polling on an at-rest surface). This is exactly the branch the PR claims to preserve ('budget can expire on a surface that is already at rest'); a regression would silently change when the agent sees the stabilization warning. Add a test with needsBaselineDistrust: true where captures keep agreeing on the unchanged baseline past the 1.5s cap, and assert postGestureOutcome stays undefined while the stabilization_timeout warning fires.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/capture-kit/src/post-gesture-stability.test.ts, line 72:
<comment>The loop's other timeout shape is untested: every case here passes `needsBaselineDistrust: false` and never triggers a rebase, so nothing exercises the branch where the budget expires after a quiet pair already agreed — the loop then returns a bare `{ value }` with no `unsettled` outcome (post-gesture-stability.ts: the `if (lastPairAgreed) return { value }` path, which comments say is reached when a rebase or distrust verdict keeps polling on an at-rest surface). This is exactly the branch the PR claims to preserve ('budget can expire on a surface that is already at rest'); a regression would silently change when the agent sees the stabilization warning. Add a test with `needsBaselineDistrust: true` where captures keep agreeing on the unchanged baseline past the 1.5s cap, and assert `postGestureOutcome` stays undefined while the stabilization_timeout warning fires.</comment>
<file context>
@@ -0,0 +1,120 @@
+
+ const outcome = await runPostGestureStabilityLoop({
+ pending: PENDING,
+ needsBaselineDistrust: false,
+ hooks: hooksFor(clock, [{ signature: ['a'], costMs: 0 }, late]),
+ clock,
</file context>
| command: 'ios.runner.scroll', | ||
| deviceId: DEVICE_ID, | ||
| platform: 'apple', | ||
| result: { x: 201, y: 487, x2: 201, y2: 387 }, |
There was a problem hiding this comment.
P3: scrollEntry() returns { x, y, x2, y2 }, but honoredScrollSwipeMidpoint (packages/contracts/src/scroll-command.ts:122-133) reads x1/y1/x2/y2, so the swipe midpoint is always undefined here. The comment's claim that midpoint (201, 437) sits inside CONTAINER never materializes, and in the no-progress test the containerHoldsSwipe gate in decideEdgeVerdict is silently skipped — the refusal passes for the wrong reason (no container-position verification). Use x1/y1 to match the leaf contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At test/integration/provider-scenarios/scroll-movement-observation.test.ts, line 106:
<comment>`scrollEntry()` returns `{ x, y, x2, y2 }`, but `honoredScrollSwipeMidpoint` (packages/contracts/src/scroll-command.ts:122-133) reads `x1`/`y1`/`x2`/`y2`, so the swipe midpoint is always `undefined` here. The comment's claim that midpoint (201, 437) sits inside CONTAINER never materializes, and in the no-progress test the `containerHoldsSwipe` gate in decideEdgeVerdict is silently skipped — the refusal passes for the wrong reason (no container-position verification). Use `x1`/`y1` to match the leaf contract.</comment>
<file context>
@@ -0,0 +1,233 @@
+ command: 'ios.runner.scroll',
+ deviceId: DEVICE_ID,
+ platform: 'apple',
+ result: { x: 201, y: 487, x2: 201, y2: 387 },
+ };
+}
</file context>
| result: { x: 201, y: 487, x2: 201, y2: 387 }, | |
| result: { x1: 201, y1: 487, x2: 201, y2: 387 }, |
| const DIRECT_IOS_OUTCOME_NOT_OBSERVED_GAP: GuaranteeEnforcement = { | ||
| kind: 'waived', | ||
| reason: | ||
| "gap: this replay-only route observes no outcome beyond the runner's own report; --verify/--settle do not apply here (see the inapplicable cells on this row), and the shared ambiguous-failure corroboration only reconsiders a thrown error, never a successful dispatch.", |
There was a problem hiding this comment.
P3: This waiver claims the direct route "observes no outcome beyond the runner's own report", but the success path finalizes through finalizeTouchInteraction with scheduleInteractionOutcomeRetry defaulting to true, so it sets the same deferred markers (pending-outcome retry, post-gesture stabilization) that TAP_OUTCOME_NOT_OBSERVED_GAP documents as "judged by the next capture, never in this response". Scope the claim to the response ("never in its own response") and name the deferred markers as the sibling does, so the two waivers that the comment says share the same reasoning stay equally precise.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/contracts/src/interaction-guarantees.ts, line 192:
<comment>This waiver claims the direct route "observes no outcome beyond the runner's own report", but the success path finalizes through finalizeTouchInteraction with scheduleInteractionOutcomeRetry defaulting to true, so it sets the same deferred markers (pending-outcome retry, post-gesture stabilization) that TAP_OUTCOME_NOT_OBSERVED_GAP documents as "judged by the next capture, never in this response". Scope the claim to the response ("never in its own response") and name the deferred markers as the sibling does, so the two waivers that the comment says share the same reasoning stay equally precise.</comment>
<file context>
@@ -165,6 +180,19 @@ const DIRECT_IOS_SINGLE_QUERY_READINESS: GuaranteeEnforcement = {
+const DIRECT_IOS_OUTCOME_NOT_OBSERVED_GAP: GuaranteeEnforcement = {
+ kind: 'waived',
+ reason:
+ "gap: this replay-only route observes no outcome beyond the runner's own report; --verify/--settle do not apply here (see the inapplicable cells on this row), and the shared ambiguous-failure corroboration only reconsiders a thrown error, never a successful dispatch.",
+ trackingIssue: GAPS_UMBRELLA_ISSUE,
+};
</file context>
| "gap: this replay-only route observes no outcome beyond the runner's own report; --verify/--settle do not apply here (see the inapplicable cells on this row), and the shared ambiguous-failure corroboration only reconsiders a thrown error, never a successful dispatch.", | |
| "gap: this replay-only route observes no outcome in its own response beyond the runner's report — the deferred markers set after dispatch (pending outcome retry, post-gesture stabilization) are judged by the next capture, never here; --verify/--settle do not apply (see the inapplicable cells on this row), and the shared ambiguous-failure corroboration only reconsiders a thrown error, never a successful dispatch.", |
|
This PR is ready. The earlier review at 938a38c found it clean, and dfe9629 has the same patch, so that result still holds. #3081 does not touch these files, so I carried the review over without a fresh run against b9b410a. Not blocking: the verdict in post-gesture-stability.ts calls Smoke Tests on dfe9629 is still running and has not failed. The diff overlaps its route, since iOS and Android smoke run gestures and scrolls through post-gesture-stability.ts and scroll-movement.ts. This route broke smoke at 66eaec5, and both smoke jobs passed at 938a38c. A regression is not expected, but a red result would need a look before anyone blames a flake. I did not run the tests locally. Merge waits on Smoke Tests finishing green. I know of no conflicts. |
|
[claude-fable-5-1] responding on behalf of @thymikee iOS Smoke Tests red on What happenedThe E2E daemon was healthy. A second daemon took its Evidence (from the
|
|
[claude-fable-5-1] responding on behalf of @thymikee iOS Smoke red on Mechanism
Comparison
Action: I ran |
|
Summary
Part of #3069. Stacked on #3072 (base branch
proto/reliability-contract); do not merge before it.Both loops now judge a late capture instead of ending
stalled(the engine defaults tocaptureDeadline: 'none'on the base; readiness is the one opt-in). Post-gesture: every non-doneend returnsmain's outcome (last value,unsettledunless the last pair agreed, plus the stabilization-timeout warning); a capture error is rethrown as itself. Scroll: only anexpiredend maps tobudgetExpiredVerdict; the edge-rest analysis is lazy again. Clock-driven adapter tests cover a capture that overruns the budget, a 2 s first capture, and a late poll that shows movement. The scroll provider scenario mocks the Simulator AX bridge probe that was costing 1.5 s per test. TheoutcomeObservationcell's waiver reasons are corrected from the traced marks; the coordinate cell joins the bounded gap list.Moves the two existing polling loops onto
observeUntil: post-gesture stability (packages/capture-kit/src/post-gesture-stability.ts) and scroll movement (src/daemon/scroll-movement.ts), plus theoutcomeObservationguarantee cell and the scroll provider scenario. Verdicts are meant to stay exactly as onmain(#2984's edge-rest rule, the distrust deadline reset, the no-effect bar).Still a draft only because it must merge after #3072.
Validation
Tested commit
938a38c260. Localpnpm check:affected --run: 1593 of 1594 files green;daemon-session-idle-expiry.test.ts(untouched here) hit a tmp-dir cleanup race once and passes alone. Live, private iPhone 17 Pro simulator, this head: a gesture thensnapshoton a page whose counter ticks every 100 ms returned the snapshot with thepost_gesture_snapshot_stabilization_timeoutwarning (attempts: 4, durationMs: 1843), noUNKNOWN;scroll downon a 3000-node page reportedmovement: "moved"with a 3757 ms post-scroll capture. Both used local Safari pages because the fixture app has no never-settling screen and no tree over 1.5 s on this host. Details in the review thread.