feat(interaction): readiness wait for replay and the Node client on a shared observation engine - #3072
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 48 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/capture-kit/src/post-gesture-stability.ts">
<violation number="1" location="packages/capture-kit/src/post-gesture-stability.ts:158">
P2: This adapter discards `observeUntil`’s deadline signal, so a snapshot capture that runs past the 1.5s/3.5s budget cannot be cancelled and joined by the shared engine. Pass the observer signal through `PostGestureStabilityHooks.capture` and the snapshot capture path; otherwise the loop can remain in flight with the pending stabilization record uncleared until the backend eventually resolves.</violation>
<violation number="2" location="packages/capture-kit/src/post-gesture-stability.ts:206">
P2: The migration changes the quiet-window verdict on the stakes path the old loop never hit: `observeUntil` now aborts an in-flight capture at the per-poll deadline (each subsequent capture is capped at `Math.max(remainingMs, intervalMs)`, e.g. 200 ms once the 1.5 s budget is nearly spent) and reports `stalled`, which this line converts into a hard throw. The previous loop passed no deadline into the capture, waited for it, and still returned the `unsettled`/value outcome (with the `post_gesture_snapshot_stabilization_timeout` warning) after the deadline passed. A capture slower than its remaining budget slice therefore escalates a previously graceful `unsettled` settlement into a thrown error, contradicting the PR's "without changing their verdicts" claim and skipping the timeout diagnostic.</violation>
</file>
<file name="packages/contracts/src/interaction-guarantees.ts">
<violation number="1" location="packages/contracts/src/interaction-guarantees.ts:167">
P2: The shared waiver misclassifies the runtime tree paths as having no always-on outcome observation. Runtime dispatch already performs post-action Android escape checks and iOS failure corroboration, so mark this cell as runtime or narrow `outcomeObservation` to successful-return verification.</violation>
<violation number="2" location="packages/contracts/src/interaction-guarantees.ts:545">
P2: The coordinate path is not inapplicable to outcome observation; it uses the shared post-dispatch observation lifecycle even without an element. Classify this cell as runtime, or redefine the guarantee explicitly around element identity rather than action outcome.</violation>
</file>
<file name="src/daemon/scroll-movement.ts">
<violation number="1" location="src/daemon/scroll-movement.ts:274">
P1: This callback drops `observeUntil`’s deadline signal, so a hung snapshot can keep `observeScrollMovement` awaiting indefinitely instead of returning `surface-unsettled` after 1.5 seconds. Thread the signal through `readOneCapture` and the scroll capture input to `captureSnapshot`.</violation>
</file>
<file name="src/commands/interaction/runtime/interaction-snapshot-capture.ts">
<violation number="1" location="src/commands/interaction/runtime/interaction-snapshot-capture.ts:38">
P1: The readiness poll's `forceFresh` flag is dropped by the daemon interaction backend, so later polls can keep reading the cached tree instead of observing a target that appears during the wait. Thread this flag through the interaction capture options and bound capture before relying on it for readiness retries.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| await sleep(params.pollMs ?? MOVEMENT_POLL_MS); | ||
| } | ||
| const observed = await observeUntil<CaptureReading, SurfaceJudgement>({ | ||
| capture: () => readOneCapture(baseline, params.capture), |
There was a problem hiding this comment.
P1: This callback drops observeUntil’s deadline signal, so a hung snapshot can keep observeScrollMovement awaiting indefinitely instead of returning surface-unsettled after 1.5 seconds. Thread the signal through readOneCapture and the scroll capture input to captureSnapshot.
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 src/daemon/scroll-movement.ts, line 274:
<comment>This callback drops `observeUntil`’s deadline signal, so a hung snapshot can keep `observeScrollMovement` awaiting indefinitely instead of returning `surface-unsettled` after 1.5 seconds. Thread the signal through `readOneCapture` and the scroll capture input to `captureSnapshot`.</comment>
<file context>
@@ -257,30 +265,32 @@ async function pollForSurfaceVerdict(
- await sleep(params.pollMs ?? MOVEMENT_POLL_MS);
- }
+ const observed = await observeUntil<CaptureReading, SurfaceJudgement>({
+ capture: () => readOneCapture(baseline, params.capture),
+ schedule: {
+ intervalMs: params.pollMs ?? SCROLL_MOVEMENT_SCHEDULE.intervalMs,
</file context>
| const result = await runtime.backend.captureSnapshot(toBackendContext(runtime, options), { | ||
| interactiveOnly, | ||
| includeRects: true, | ||
| ...(forceFresh ? { forceFresh: true } : {}), |
There was a problem hiding this comment.
P1: The readiness poll's forceFresh flag is dropped by the daemon interaction backend, so later polls can keep reading the cached tree instead of observing a target that appears during the wait. Thread this flag through the interaction capture options and bound capture before relying on it for readiness retries.
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 src/commands/interaction/runtime/interaction-snapshot-capture.ts, line 38:
<comment>The readiness poll's `forceFresh` flag is dropped by the daemon interaction backend, so later polls can keep reading the cached tree instead of observing a target that appears during the wait. Thread this flag through the interaction capture options and bound capture before relying on it for readiness retries.</comment>
<file context>
@@ -0,0 +1,50 @@
+ const result = await runtime.backend.captureSnapshot(toBackendContext(runtime, options), {
+ interactiveOnly,
+ includeRects: true,
+ ...(forceFresh ? { forceFresh: true } : {}),
+ });
+ const snapshot =
</file context>
|
|
||
| const observed = await observeUntil<T, PostGestureStabilityOutcome<T>>({ | ||
| ...(params.initial !== undefined ? { initial: params.initial } : {}), | ||
| capture: () => hooks.capture(), |
There was a problem hiding this comment.
P2: This adapter discards observeUntil’s deadline signal, so a snapshot capture that runs past the 1.5s/3.5s budget cannot be cancelled and joined by the shared engine. Pass the observer signal through PostGestureStabilityHooks.capture and the snapshot capture path; otherwise the loop can remain in flight with the pending stabilization record uncleared until the backend eventually resolves.
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 158:
<comment>This adapter discards `observeUntil`’s deadline signal, so a snapshot capture that runs past the 1.5s/3.5s budget cannot be cancelled and joined by the shared engine. Pass the observer signal through `PostGestureStabilityHooks.capture` and the snapshot capture path; otherwise the loop can remain in flight with the pending stabilization record uncleared until the backend eventually resolves.</comment>
<file context>
@@ -129,24 +138,34 @@ export async function runPostGestureStabilityLoop<T, S extends readonly unknown[
+
+ const observed = await observeUntil<T, PostGestureStabilityOutcome<T>>({
+ ...(params.initial !== undefined ? { initial: params.initial } : {}),
+ capture: () => hooks.capture(),
+ schedule: POST_GESTURE_STABILITY_SCHEDULE,
+ verdict: (latest, previousValue) => {
</file context>
| // after a tap is an ambiguous outcome, per docs/agents/selector-capture.md) is scoped to | ||
| // navigation-sensitive Android actions and target-authored gestures, not the tap-shaped runtime | ||
| // paths; --settle/--verify are the opt-in ways to observe one. | ||
| const TAP_OUTCOME_NOT_OBSERVED_GAP: GuaranteeEnforcement = { |
There was a problem hiding this comment.
P2: The shared waiver misclassifies the runtime tree paths as having no always-on outcome observation. Runtime dispatch already performs post-action Android escape checks and iOS failure corroboration, so mark this cell as runtime or narrow outcomeObservation to successful-return verification.
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 167:
<comment>The shared waiver misclassifies the runtime tree paths as having no always-on outcome observation. Runtime dispatch already performs post-action Android escape checks and iOS failure corroboration, so mark this cell as runtime or narrow `outcomeObservation` to successful-return verification.</comment>
<file context>
@@ -149,6 +159,42 @@ const SHARED_RESPONSE_CONSTRUCTION: GuaranteeEnforcement = {
+// after a tap is an ambiguous outcome, per docs/agents/selector-capture.md) is scoped to
+// navigation-sensitive Android actions and target-authored gestures, not the tap-shaped runtime
+// paths; --settle/--verify are the opt-in ways to observe one.
+const TAP_OUTCOME_NOT_OBSERVED_GAP: GuaranteeEnforcement = {
+ kind: 'waived',
+ reason:
</file context>
|
Thanks for the PR. At c19112a I found four defects that need fixing before merge, and I have a question on scope at the end. The post-gesture adapter passes Scroll has the same problem. In observe-until.ts the ride-out branch returns without setting I think the replay default is applied too late (session-replay-action-runtime.ts:148). Recorded press, click, and longpress steps carry Not blocking: the docs (replay-e2e.md:66) point replay users at Would it help to split this into two PRs? The first would hold I ran no tests and no live device runs, so the first two findings come from reading the code. The reported live A/B runs covered only the hit path. Before merge I would like three live runs. First, an iOS simulator replay with On CI, the checks do not clear the change yet. Both CodeQL Analyze logs end at "Uploading results" with no error, and the java-kotlin job shows only a Maven fetch warning. This PR changes no Kotlin or Java, so those look like upload problems. Coverage and Compatibility & Provenance have no failed-step log that I could read, and Coverage runs the suites this change touches, so it cannot be cleared without one. Smoke Tests is still running and exercises the interaction and scroll routes this PR rewrites. Next, please fix the first two defects by keeping the judge-late-capture behavior in the post-gesture and scroll adapters. Then record |
There was a problem hiding this comment.
5 issues found across 30 files (changes from recent commits).
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="src/commands/interaction/runtime/resolution-disclosure.test.ts">
<violation number="1" location="src/commands/interaction/runtime/resolution-disclosure.test.ts:9">
P3: This test re-asserts the two disclosures that `tryResolveRefNode discloses exact for a resolved ref and label-fallback for label recovery` in ref-target-resolution.test.ts (lines 80-101) already verifies end-to-end, and that test was added in the same commit. `buildRefResolution` is only reachable through `tryResolveRefNode`/`adoptPreresolvedRefTarget`, so this file adds no coverage; the fields it uniquely contributes — the `ref` and `node` passthrough — are never asserted. Drop the file, or give it distinct value by asserting `buildRefResolution('e1', node, kind).ref` and that the returned node is the passed node.</violation>
</file>
<file name="src/commands/interaction/runtime/native-ref-interaction.ts">
<violation number="1" location="src/commands/interaction/runtime/native-ref-interaction.ts:64">
P1: `preflightNativeRefInteraction` validates `@ref` against the latest observation instead of the authorized ref frame. After a read-only capture advances `session.snapshot`, native fast paths can guard and report a different node while the backend acts on the authorized ref; use the same frame selection as normal ref resolution.</violation>
<violation number="2" location="src/commands/interaction/runtime/native-ref-interaction.ts:131">
P2: Native dispatch reports `resolution.kind: 'exact'` even when `fallbackLabel` recovered the target. Preserve the preflight resolution so clients receive the required `label-fallback` disclosure, matching the runtime path.</violation>
</file>
<file name="src/commands/interaction/runtime/target-visibility-stages.ts">
<violation number="1" location="src/commands/interaction/runtime/target-visibility-stages.ts:163">
P3: The generated recovery command does not escape apostrophes in selectors. Copying a hint for a selector such as `label=O'Reilly` produces invalid shell quoting, so the documented `scroll --until` recovery cannot run; escape single quotes before wrapping the selector.</violation>
</file>
<file name="packages/command-registry/src/types.ts">
<violation number="1" location="packages/command-registry/src/types.ts:83">
P3: Adding `targetReadiness` to `CommandTimeoutPolicy` makes it legal to declare the trait inside a descriptor's own `timeoutPolicy`, which the doc comment right above explicitly forbids ("Descriptors declare it as `targetReadiness`, never on their `timeoutPolicy`"). If a future descriptor did attach it there, `registry.ts`'s merge (`descriptor.targetReadiness ? {...} : descriptor.timeoutPolicy`) stores the policy object as-is, so `readinessBudgetMs` would honor the trait while `commandAcceptsReadinessBudget` (which reads `descriptor.targetReadiness`) would not — envelope widening and replay default-budget gating diverge silently. The sealing invariant is currently prose-only.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| preAction?: SurfaceScopedNodes; | ||
| }> { | ||
| const session = await runtime.sessions.get(options.session ?? 'default'); | ||
| const storedSnapshot = session?.snapshot; |
There was a problem hiding this comment.
P1: preflightNativeRefInteraction validates @ref against the latest observation instead of the authorized ref frame. After a read-only capture advances session.snapshot, native fast paths can guard and report a different node while the backend acts on the authorized ref; use the same frame selection as normal ref resolution.
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 src/commands/interaction/runtime/native-ref-interaction.ts, line 64:
<comment>`preflightNativeRefInteraction` validates `@ref` against the latest observation instead of the authorized ref frame. After a read-only capture advances `session.snapshot`, native fast paths can guard and report a different node while the backend acts on the authorized ref; use the same frame selection as normal ref resolution.</comment>
<file context>
@@ -0,0 +1,135 @@
+ preAction?: SurfaceScopedNodes;
+}> {
+ const session = await runtime.sessions.get(options.session ?? 'default');
+ const storedSnapshot = session?.snapshot;
+ const nodes = storedSnapshot?.nodes;
+ if (!storedSnapshot || !nodes || normalizeRef(target.ref) === null) return {};
</file context>
| return { | ||
| kind: 'ref', | ||
| target: { kind: 'ref', ref: target.ref }, | ||
| resolution: EXACT_REF_RESOLUTION, |
There was a problem hiding this comment.
P2: Native dispatch reports resolution.kind: 'exact' even when fallbackLabel recovered the target. Preserve the preflight resolution so clients receive the required label-fallback disclosure, matching the runtime path.
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 src/commands/interaction/runtime/native-ref-interaction.ts, line 131:
<comment>Native dispatch reports `resolution.kind: 'exact'` even when `fallbackLabel` recovered the target. Preserve the preflight resolution so clients receive the required `label-fallback` disclosure, matching the runtime path.</comment>
<file context>
@@ -0,0 +1,135 @@
+ return {
+ kind: 'ref',
+ target: { kind: 'ref', ref: target.ref },
+ resolution: EXACT_REF_RESOLUTION,
+ ...preflight,
+ ...(formattedBackendResult ? { backendResult: formattedBackendResult } : {}),
</file context>
| test('buildRefResolution is the shared exact and label-fallback disclosure constructor', () => { | ||
| const node = selectorSnapshot().nodes[0]!; | ||
|
|
||
| assert.deepEqual(buildRefResolution('e1', node, 'exact').resolution, { |
There was a problem hiding this comment.
P3: This test re-asserts the two disclosures that tryResolveRefNode discloses exact for a resolved ref and label-fallback for label recovery in ref-target-resolution.test.ts (lines 80-101) already verifies end-to-end, and that test was added in the same commit. buildRefResolution is only reachable through tryResolveRefNode/adoptPreresolvedRefTarget, so this file adds no coverage; the fields it uniquely contributes — the ref and node passthrough — are never asserted. Drop the file, or give it distinct value by asserting buildRefResolution('e1', node, kind).ref and that the returned node is the passed node.
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 src/commands/interaction/runtime/resolution-disclosure.test.ts, line 9:
<comment>This test re-asserts the two disclosures that `tryResolveRefNode discloses exact for a resolved ref and label-fallback for label recovery` in ref-target-resolution.test.ts (lines 80-101) already verifies end-to-end, and that test was added in the same commit. `buildRefResolution` is only reachable through `tryResolveRefNode`/`adoptPreresolvedRefTarget`, so this file adds no coverage; the fields it uniquely contributes — the `ref` and `node` passthrough — are never asserted. Drop the file, or give it distinct value by asserting `buildRefResolution('e1', node, kind).ref` and that the returned node is the passed node.</comment>
<file context>
@@ -0,0 +1,19 @@
+test('buildRefResolution is the shared exact and label-fallback disclosure constructor', () => {
+ const node = selectorSnapshot().nodes[0]!;
+
+ assert.deepEqual(buildRefResolution('e1', node, 'exact').resolution, {
+ source: 'ref',
+ phase: 'pre-action',
</file context>
| ): string { | ||
| if (!direction) return 'Scroll toward it,'; | ||
| if (!selector) return `Scroll ${direction} toward it,`; | ||
| return `Run scroll ${direction} --until '${selector}' to bring it on screen,`; |
There was a problem hiding this comment.
P3: The generated recovery command does not escape apostrophes in selectors. Copying a hint for a selector such as label=O'Reilly produces invalid shell quoting, so the documented scroll --until recovery cannot run; escape single quotes before wrapping the selector.
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 src/commands/interaction/runtime/target-visibility-stages.ts, line 163:
<comment>The generated recovery command does not escape apostrophes in selectors. Copying a hint for a selector such as `label=O'Reilly` produces invalid shell quoting, so the documented `scroll --until` recovery cannot run; escape single quotes before wrapping the selector.</comment>
<file context>
@@ -0,0 +1,219 @@
+): string {
+ if (!direction) return 'Scroll toward it,';
+ if (!selector) return `Scroll ${direction} toward it,`;
+ return `Run scroll ${direction} --until '${selector}' to bring it on screen,`;
+}
+
</file context>
| return `Run scroll ${direction} --until '${selector}' to bring it on screen,`; | |
| return `Run scroll ${direction} --until '${selector.replaceAll("'", "'\\''")}' to bring it on screen,`; |
| * so the envelope can widen by the readiness budget. Descriptors declare it as `targetReadiness`, | ||
| * never on their `timeoutPolicy`. | ||
| */ | ||
| targetReadiness?: CommandTargetReadiness; |
There was a problem hiding this comment.
P3: Adding targetReadiness to CommandTimeoutPolicy makes it legal to declare the trait inside a descriptor's own timeoutPolicy, which the doc comment right above explicitly forbids ("Descriptors declare it as targetReadiness, never on their timeoutPolicy"). If a future descriptor did attach it there, registry.ts's merge (descriptor.targetReadiness ? {...} : descriptor.timeoutPolicy) stores the policy object as-is, so readinessBudgetMs would honor the trait while commandAcceptsReadinessBudget (which reads descriptor.targetReadiness) would not — envelope widening and replay default-budget gating diverge silently. The sealing invariant is currently prose-only.
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/command-registry/src/types.ts, line 83:
<comment>Adding `targetReadiness` to `CommandTimeoutPolicy` makes it legal to declare the trait inside a descriptor's own `timeoutPolicy`, which the doc comment right above explicitly forbids ("Descriptors declare it as `targetReadiness`, never on their `timeoutPolicy`"). If a future descriptor did attach it there, `registry.ts`'s merge (`descriptor.targetReadiness ? {...} : descriptor.timeoutPolicy`) stores the policy object as-is, so `readinessBudgetMs` would honor the trait while `commandAcceptsReadinessBudget` (which reads `descriptor.targetReadiness`) would not — envelope widening and replay default-budget gating diverge silently. The sealing invariant is currently prose-only.</comment>
<file context>
@@ -75,8 +75,21 @@ export type CommandTimeoutPolicy = {
+ * so the envelope can widen by the readiness budget. Descriptors declare it as `targetReadiness`,
+ * never on their `timeoutPolicy`.
+ */
+ targetReadiness?: CommandTargetReadiness;
};
</file context>
|
Thanks for the update. I reviewed bd0aff2. The delta mostly moves target-resolution code out of resolution.ts, but the four defects from the first review are still open, and there is no live replay run yet. First, a capture that ignores the engine's signal can end as stalled. In post-gesture-stability.ts, the capture adapter at line 158 ignores the deadline signal. If a 300-800 ms iOS capture outlives the rest of the 1.5 s budget, Second, the same engine rule breaks scroll in scroll-movement.ts. Line 274 wraps Third, the ride-out branch loses the unreadable reason. In observe-until.ts, the branch returns Fourth, the replay readiness wait skips the verify gate. In session-replay-action-runtime.ts, the new change swaps the hardcoded set for No live replay run is attached yet. Please replay a recorded I read the code and the new registry consistency test but did not run any suite, and I could not observe device behavior. Smoke Tests and Coverage are still running. This change moves the press, click and longpress target-resolution code that smoke tests exercise, so a failure there could relate to it. There is no failure to point at yet. No conflicts. Before merge, please fix the four defects at their owners, add a regression test for each, and attach the live pass and miss runs. |
|
[claude-fable-5-1] responding on behalf of @thymikee Split as you suggested, and the four defects are fixed at Split. This PR is now the engine plus its one consumer, the readiness wait. The post-gesture and scroll migrations, the Defects 1 and 2 (late captures) ( Defect 3 ( Defect 4 (replay gate) ( Non-blocking, all taken ( One cubic finding was false and led to a deletion: press captures never reach the 750 ms selector cache (that cache lives only in the selector backend serving Live runs: the two from the migration half move with it. The annotated replay with an engaged wait is still open: 46 local runs never engaged the wait on this host, which is the reason #3077 exists. |
There was a problem hiding this comment.
3 issues found across 44 files (changes from recent commits).
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/replay-port/src/daemon-port/session-replay-runtime-engine-adapter.ts">
<violation number="1" location="packages/replay-port/src/daemon-port/session-replay-runtime-engine-adapter.ts:210">
P2: This branch collapses sparse and exhausted unreadable captures into generic target-unavailable failures. Preserve the typed capture outcome through `AdReplayTargetObservation` and map sparse to `capture_sparse` and exhausted unreadable captures to their original typed error before building the divergence.</violation>
</file>
<file name="src/commands/interaction/runtime/selector-readiness.test.ts">
<violation number="1" location="src/commands/interaction/runtime/selector-readiness.test.ts:118">
P3: `notEqual(error.details?.reason, 'selector_not_found')` asserts absence loosely: it passes for any other `reason` value (e.g. a wrapping failure that somehow preserves the `androidSnapshotHelperFailureReason` detail), and duplicates the magic string instead of the contract. The declared behavior is that the ridden-out unreadable error is surfaced unchanged with no selector-miss conversion, so pin that exactly: `assert.equal(error.details?.reason, INTERACTION_ERROR_REASONS.selectorNotFound)` would also be wrong — assert the selector-miss reason is *absent*: `assert.equal(error.details?.reason, undefined)` (import `INTERACTION_ERROR_REASONS` from `@agent-device/selectors/interaction-error` if you want to assert against the constant instead of the literal).</violation>
</file>
<file name="src/daemon/__tests__/replay-target/session-replay-target-verification-runtime.test.ts">
<violation number="1" location="src/daemon/__tests__/replay-target/session-replay-target-verification-runtime.test.ts:249">
P3: The test name promises `fill` refuses a selector miss on one capture, but nothing asserts `fill` captured only once. A regression that made `fill` wait for its target — then time out with the target never rendering — would still end in a selector-miss and pass this test unchanged (only the wait-then-hit variant is caught, indirectly, by the mocked second capture). Mirror the click gate test's `toBeGreaterThan(1)` assertion and pin the single capture.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| const observation = await captureTargetObservation(action); | ||
| lastObservation = observation; | ||
| if (observation.state !== 'available') { | ||
| return { state: 'unavailable', reason: observation.reason, hint: observation.hint }; |
There was a problem hiding this comment.
P2: This branch collapses sparse and exhausted unreadable captures into generic target-unavailable failures. Preserve the typed capture outcome through AdReplayTargetObservation and map sparse to capture_sparse and exhausted unreadable captures to their original typed error before building the divergence.
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/replay-port/src/daemon-port/session-replay-runtime-engine-adapter.ts, line 210:
<comment>This branch collapses sparse and exhausted unreadable captures into generic target-unavailable failures. Preserve the typed capture outcome through `AdReplayTargetObservation` and map sparse to `capture_sparse` and exhausted unreadable captures to their original typed error before building the divergence.</comment>
<file context>
@@ -167,46 +202,62 @@ export function createAdReplayStepRuntime(params: {
+ const observation = await captureTargetObservation(action);
+ lastObservation = observation;
+ if (observation.state !== 'available') {
+ return { state: 'unavailable', reason: observation.reason, hint: observation.hint };
+ }
+ const session = ctx.sessionStore.get();
</file context>
| (error: unknown) => { | ||
| assert.ok(error instanceof AppError); | ||
| assert.equal(error.details?.androidSnapshotHelperFailureReason, 'system-window-only'); | ||
| assert.notEqual(error.details?.reason, 'selector_not_found'); |
There was a problem hiding this comment.
P3: notEqual(error.details?.reason, 'selector_not_found') asserts absence loosely: it passes for any other reason value (e.g. a wrapping failure that somehow preserves the androidSnapshotHelperFailureReason detail), and duplicates the magic string instead of the contract. The declared behavior is that the ridden-out unreadable error is surfaced unchanged with no selector-miss conversion, so pin that exactly: assert.equal(error.details?.reason, INTERACTION_ERROR_REASONS.selectorNotFound) would also be wrong — assert the selector-miss reason is absent: assert.equal(error.details?.reason, undefined) (import INTERACTION_ERROR_REASONS from @agent-device/selectors/interaction-error if you want to assert against the constant instead of the literal).
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 src/commands/interaction/runtime/selector-readiness.test.ts, line 118:
<comment>`notEqual(error.details?.reason, 'selector_not_found')` asserts absence loosely: it passes for any other `reason` value (e.g. a wrapping failure that somehow preserves the `androidSnapshotHelperFailureReason` detail), and duplicates the magic string instead of the contract. The declared behavior is that the ridden-out unreadable error is surfaced unchanged with no selector-miss conversion, so pin that exactly: `assert.equal(error.details?.reason, INTERACTION_ERROR_REASONS.selectorNotFound)` would also be wrong — assert the selector-miss reason is *absent*: `assert.equal(error.details?.reason, undefined)` (import `INTERACTION_ERROR_REASONS` from `@agent-device/selectors/interaction-error` if you want to assert against the constant instead of the literal).</comment>
<file context>
@@ -93,3 +93,31 @@ test('runtime press caps a readinessTimeoutMs larger than the row maxTimeoutMs a
+ (error: unknown) => {
+ assert.ok(error instanceof AppError);
+ assert.equal(error.details?.androidSnapshotHelperFailureReason, 'system-window-only');
+ assert.notEqual(error.details?.reason, 'selector_not_found');
+ return true;
+ },
</file context>
| assert.notEqual(error.details?.reason, 'selector_not_found'); | |
| assert.equal(error.details?.reason, undefined); |
|
|
||
| const response = await scene.replay(); | ||
|
|
||
| expect(scene.invoked.length).toBe(0); |
There was a problem hiding this comment.
P3: The test name promises fill refuses a selector miss on one capture, but nothing asserts fill captured only once. A regression that made fill wait for its target — then time out with the target never rendering — would still end in a selector-miss and pass this test unchanged (only the wait-then-hit variant is caught, indirectly, by the mocked second capture). Mirror the click gate test's toBeGreaterThan(1) assertion and pin the single capture.
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 src/daemon/__tests__/replay-target/session-replay-target-verification-runtime.test.ts, line 249:
<comment>The test name promises `fill` refuses a selector miss on one capture, but nothing asserts `fill` captured only once. A regression that made `fill` wait for its target — then time out with the target never rendering — would still end in a selector-miss and pass this test unchanged (only the wait-then-hit variant is caught, indirectly, by the mocked second capture). Mirror the click gate test's `toBeGreaterThan(1)` assertion and pin the single capture.</comment>
<file context>
@@ -177,6 +194,63 @@ test('a selector-miss divergence blocks dispatch and never sends the action', as
+
+ const response = await scene.replay();
+
+ expect(scene.invoked.length).toBe(0);
+ expect(response.ok).toBe(false);
+ if (response.ok) return;
</file context>
| expect(scene.invoked.length).toBe(0); | |
| expect(scene.invoked.length).toBe(0); | |
| expect(mockDispatchCommand).toHaveBeenCalledTimes(1); |
|
The earlier findings at bd0aff2 are mostly addressed in c128073, but two things remain: a duplicated readiness schedule and missing live evidence. The replay gate and the dispatch still build the same readiness schedule in two packages. The gate wait changes a device-facing route, and no live run shows it engaging. Replay of an annotated press, click, or longpress now re-captures the device before dispatch, at this call. The 46 local runs you report never entered the wait. Unit tests use a fake capture and an instant clock, so the capture cadence, the launch-race retry inside each poll, and the hand-off of the remaining budget to the dispatch are not proven on a device. Please make the route deterministic, for example with a test-app screen whose target button renders about 1 s after navigation. Then attach two simulator runs with Not blocking, and you can take or leave it: Is there a smaller shape than this? The split answers the earlier challenge, and the one new seam ( I read the code at c128073 and ran no tests. I did not trace whether press captures ever reach the 750 ms selector snapshot reuse window, so the reason for deleting The Smoke Tests job failed in "Install Linux desktop dependencies", where the apt step timed out after 6 minutes, before any test ran. The diff touches no CI config, apt, or Linux desktop setup, so this looks unrelated to the change. Please rerun it, since a real Smoke result is needed for the press and click resolution route this PR changes. Before merge, the schedule needs one owner, the annotated-replay pass and miss runs need to be attached, and Smoke Tests needs a green rerun. |
…get, two loops migrated Prototype only. Shared observeUntil loop in capture-kit with one ride-out classifier in contracts; promotedTarget declares a 2 s / 200 ms readiness budget consumed by selector resolution with the first capture unbounded; post-gesture stability and scroll movement run as verdict predicates over the engine; targetReadiness and outcomeObservation guarantee cells.
…gpress
promotedTarget.poll becomes {intervalMs, maxTimeoutMs} (no row default). A
new operator-only readinessTimeoutMs common input field (no CLI flag, no MCP
exposure) gates resolveSelectorInteractionTarget's poll: a positive integer
caps at the row's maxTimeoutMs and runs the readiness loop; absent (MCP,
CLI, and every caller today) takes the one-attempt path, so a wrong-selector
miss fails on the first capture instead of paying a 2s wait. Replay defaults
readinessTimeoutMs: 2_000 onto dispatched press/click/longpress steps unless
already set.
Split resolution.ts (1,411 lines) three ways: selector-readiness.ts owns the
readiness poll and its capture-attempt/failure machinery,
interaction-snapshot-capture.ts and covered-interaction-error.ts are new
leaves shared with resolution.ts without a value-import cycle (verified via
the layering scan). interaction-guarantees.ts's targetReadiness via now
points at selector-readiness.ts#pollForSelectorReadiness.
…t.ts client.test.ts is already over the test-file-size-ratchet tripwire and may not grow; the new press/click/longpress readinessTimeoutMs test moves to its own small file instead.
…e request envelope by the readiness budget
resolution.ts (1,131 lines) keeps the entry and the point/selector target kinds (249 lines). The rest moves, function bodies unchanged: - ref-target-resolution.ts (262): how an @ref becomes a node, and when it is refused (adopt/read, frame reconciliation, tryResolveRefNode, refMissRefusal). - resolution-disclosure.ts (179): what a resolution records and reports (EXACT_REF_RESOLUTION, buildRefResolution and its ResolvedRefNode type, selector disclosure, non-hittable hint, press recording retarget). - target-visibility-stages.ts (219): is the resolved target reachable on screen: the node-stage runner, the covered refusal (covered-interaction- error.ts folded in and deleted) and the off-screen stage. - native-ref-interaction.ts (135): the native-ref preflight and dispatch. - resolution-touch-point.ts (48): node to touch point, shared by the ref and selector kinds, so neither kind module owns the other. - replay-target-guard.ts (65): assertExpectedResolvedTarget and assertReplayTargetResolution, used by the ref and selector kinds and by selector-is/selector-read. - interaction-resolution-request.ts (73): InteractionAction, ResolveInteractionTargetParams and ExpectedResolvedTarget. The layering scan (R9/R10) refused the split while sibling modules type-imported resolution.ts: the largest type cycle grew from 6 to 8 files. The request types now sit below every module that reads them. Not a pure move: `export` added to symbols now crossing a module boundary; two comments that pointed at "above" now name the file; the stale fallow-ignore on EXACT_REF_RESOLUTION (now statically imported) and on pollForSelectorReadiness (already statically imported by resolution.ts) are removed. interaction-guarantees.ts `via` paths and three cross-package comments follow the symbols. resolution.test.ts (706) splits along the same seams: resolution.test.ts 350, ref-target-resolution.test.ts 126, resolution-disclosure.test.ts 19, target-visibility-stages.test.ts 227. Test bodies unchanged. git diff -M90% --stat: 24 files changed, 1399 insertions(+), 1312 deletions(-). No file passes the 90% rename threshold (it is a split); resolution.ts +18/-900, resolution.test.ts +1/-357. A line-multiset comparison of old against new bodies (imports and blank lines excluded) differs only in the export keywords and comments listed above. No fallow baseline entry was keyed on the moved paths.
…laration Replay hardcoded press/click/longpress as the commands that get a default readinessTimeoutMs. Nothing declared that set per command: readinessTimeoutMs is a common input field every command reads. press, click and longpress now declare `targetReadiness: 'budgeted'` in the command registry. Replay reads it through commandAcceptsReadinessBudget. resolveCommandTimeoutPolicy attaches the trait to the resolved timeout policy, and resolveCommandRequestTimeoutMs widens the envelope by the readiness budget only when that trait is present. Before, it widened for any command whose flags carried readinessTimeoutMs; only these three commands forward the field to request flags, so no request changes. A test asserts that the commands with the trait equal the commands the `runtime` targetReadiness guarantee cells in interaction-guarantees.ts apply to. The replay default test iterates the declared set.
…igrations move to a stacked PR The post-gesture stability and scroll-movement loops return to their main versions, together with the scroll-movement observation provider scenario and the outcomeObservation guarantee cell that described them. They move to a stacked PR: both loops run captures that may not honor a per-capture deadline signal, so the engine needs a declared per-capture deadline mode and slow-capture scenarios before they can migrate without turning a late capture into a stalled end. The shared ride-out classifier had no production reader (readiness rides out isUnreadableCaptureContentError), so it is deleted with the contracts observation subpath. ObservationEnd has one reader, the engine, and now lives in observe-until.ts.
…den-out errors observeUntil schedules now declare captureDeadline. 'cancel' keeps the existing contract: each capture is armed with the remaining budget and a capture that ends at or past its deadline ends the loop stalled. 'none' hands the capture no deadline, so a capture that cannot be cancelled is still judged when it finishes late and the loop ends done or expired, never stalled. A ridden-out poll now records its error, so a loop whose every capture was ridden out ends expired with lastError set; the readiness wait raises that typed unreadable-content error instead of a selector miss against an empty tree. A terminal capture failure is labelled 'failed' in the poll timeline instead of 'observed'.
…ap its envelope readinessTimeoutMs was a common input every structured command accepted and ignored. Only commands whose descriptor declares targetReadiness: 'budgeted' now carry it, through targetReadinessFields; the common input reader refuses the key with INVALID_ARGS for every command whose fields do not declare it. The request envelope widened by the raw readinessTimeoutMs, so a caller value of 999000 stretched it by 999 s while the poll itself stops at the promotedTarget row's ceiling. The widening now uses the same capped budget. The press readiness provider scenario claimed to exercise forceFresh through the selector capture cache; the press route captures through the interaction backend, which keeps no such cache, so the comments now say so.
…rt readiness A recorded press/click/longpress step carries target evidence, and replay classified that target from one capture before dispatch. A target that had not rendered yet diverged as a selector miss before the dispatch's own readiness wait could run, so the replay default budget never applied to annotated steps. The engine now asks the port to observeTarget, which captures and classifies in one capability. For a step whose dispatch would wait (a readiness-budgeted command resolving a selector), the port re-captures under the same schedule while the recorded target is a selector miss; any other classification or an unavailable capture ends the wait. A target that never appears diverges as before. The divergence capture takes no per-capture signal, so the wait declares captureDeadline 'none'. ReplayDaemonDependencies gains an optional clock the wait paces itself by. REPLAY_DIVERGENCE now carries the cause's readiness detail at error.details.readiness.
The client API page says the command performs the requested interaction after the wait, and names the failures that carry readiness evidence and those that do not. The replay page says how a missing target surfaces for annotated and unannotated steps.
…ess capture route has no cache The readiness poll asked for a fresh capture from its second poll on, but no press, click, or longpress capture can be served from the selector capture cache, so the flag and the backend option that carried it are removed and selector-runtime-backend.ts, backend.ts, and backend-snapshot-options.ts return to main. Trace: - selector-readiness.ts pollSelectorReadinessOnce captured through attemptSelectorResolution, which calls captureInteractionSnapshot (interaction-snapshot-capture.ts:35) and so runtime.backend.captureSnapshot. - The daemon press route builds that runtime with createInteractionRuntimeForRoute (src/daemon/interaction/internal/interaction-touch-press.ts:10, used for longPress/click/press at :177-187). - Its backend is createInteractionBackend (src/daemon/interaction/internal/interaction-runtime.ts:126), whose captureSnapshot (:132-139) forwards interactiveOnly, preferredBackend, includeRects, and the signal, and nothing cache-related. - That capture calls captureSnapshotForSession (src/daemon/interaction/index.ts:21), which calls the daemon captureSnapshot (src/daemon/snapshot-capture.ts:77). - captureSnapshot runs captureSnapshotAttempt, which runs captureSnapshotData (snapshot-capture.ts:99) and captures live through the bound capture or captureSnapshotWithInteractor on every call. - The 750 ms cache (SELECTOR_CAPTURE_CACHE_TTL_MS, src/daemon/selector-capture-runtime.ts:24, read at :253 and :280) exists only inside createSelectorBackend (src/daemon/selector-runtime-backend.ts:142). - createSelectorRuntimeForDevice is reached from the selector read and wait routes (selector-runtime.ts, wait-runtime.ts:158), never from interaction-touch-press.ts. The runtime-selector targetReadiness contract scenario keyed its late target on the removed option; it now keys on the capture count.
…he dispatch A replayed step has one readiness budget. The pre-dispatch target gate and the dispatch each spent the full budget, so a target that rendered late could cost a step twice the budget. When the gate polled more than once, the dispatch now receives what is left, max(0, budget - gate waitedMs), as its readinessTimeoutMs; 0 makes the dispatch resolve its target once. A gate that matched on its first capture leaves the dispatch the whole budget. A gate that polled more than once emits the interaction_target_readiness diagnostic the dispatched wait emits, with polls, waitedMs, end, and the step's command. A selector-miss divergence after such a wait carries the same evidence at error.details.readiness, where an unannotated step's target-not-found failure already carries it.
…lector policy The envelope widening imported the promotedTarget selector row to read its 2 s poll ceiling, which made every command-registry entry and src/cli.ts evaluate the selector policy module. The cap is now READINESS_BUDGET_MAX_MS in timeout-policy.ts, pinned by test to the row's maxTimeoutMs; selectors cannot import command-registry, so the test is the tie. The same eager-closure gate showed two more new edges from this branch: - replay's native and test command entries evaluated observe-until and the selector policy through the target gate. Both now load inside the gate's readiness path, the only code that uses them. - src/cli.ts evaluated the new target-readiness-grammar module. Its one function moves into interaction/metadata.ts, its one caller, and its tests into metadata.test.ts.
The replay target gate and the dispatch each built the promotedTarget readiness schedule from the row's poll budget. readinessScheduleFor now lives next to SELECTOR_PIPELINE_POLICIES and both callers use it: the dispatch directly, the replay gate through its existing dynamic import. The two waits also counted the budget from different origins: the gate from the start of its wait, the dispatch from the end of its first capture. The schedule now carries budgetFrom: 'first-capture' for both, and the budget the gate leaves the dispatch is the step budget minus the gate's time after its first capture ended. The first capture is the one-attempt lookup a step pays without a wait, in the gate as in the dispatch.
…ne; cancel is an opt-in captureDeadline is now optional and defaults to 'none'. A capture only ends the loop stalled when its caller declares 'cancel', so an adapter whose capture ignores the signal cannot claim cancellation by omission. The selector readiness poll keeps its explicit 'cancel': its poll signal reaches the platform as CaptureSnapshotInput.signal. The replay target gate drops its explicit 'none'.
An error thrown out of the replay run reached the wire as code and message only; the catch rebuilt it with errorResponse and dropped details, hint, and diagnostic references. A request canceled during the pre-dispatch target gate therefore lost its request_canceled reason. The catch now normalizes the error with normalizeError and appends the run's artifactPaths to its details.
The target gate's divergence capture takes no signal, so the gate honors a canceled request only at a poll boundary. A request canceled during the gate's second capture ends the wait when that capture returns: two captures, the request_canceled reason on the response, and no dispatch.
…gentDeviceRuntime.clock
observeUntil checked the caller's signal only at the top of each iteration, before the sleep. A cancel that arrived during the sleep still paid one more capture before the loop noticed. The loop now checks the signal after the sleep and ends with the typed request-canceled error before starting a poll. The host-kit sleep takes no signal, so the check after the sleep is the boundary.
c128073 to
d476097
Compare
|
[claude-fable-5-1] responding on behalf of @thymikee Both blocking items are addressed at One owner for the schedule ( Engine default ( Cancel ( Clock ( Live runs: on this head ( Pass, The other three presses show Miss, same script with the last press changed to Smoke Tests: the earlier red was the |
|
This PR is ready. Both blocking findings from the earlier review at c128073 are fixed in d476097, and the delta no longer has any blocking problem. Not blocking, and you can take or leave these: the adapter rebuilds the engine's budget start from the poll timeline in budgetSpentMs, so could CI shows 21 checks with none failing. The earlier Smoke failure was an apt-install timeout that ran before any repository code, and the rerun is reported green. There are no conflicts. For evidence, I took the live pass and miss runs from your quoted output and did not inspect the raw ndjson or request-log artifacts. I ran no tests, and I confirmed the regression is real by reading the pre-change code in 1481f36. The miss run's 2576 ms wait against the 2000 ms cap matches the engine's rule that the budget counts from the end of the first capture, so one capture can overrun. I did not re-check the request envelope's headroom, since that code is outside this delta. I also did not re-review the Nothing else stands in the way, so this is ready for a maintainer to merge. |
thymikee
added
the
ready-for-human
1 participant
Summary
Part of #3069. Merge after #3070 and #3071. The post-gesture and scroll migrations moved to a stacked PR, so this PR is the engine plus its one consumer, the readiness wait, and can be dropped whole if the #3077 soak says so.
press,click, andlongpresstake an operator-onlyreadinessTimeoutMs. With it, a target that is not on screen yet is looked for every 200 ms, up to the value (capped at 2 s), then the requested interaction runs once. A covered, off-screen, or ambiguous target still fails at once; only an unreadable capture is waited out (and if every capture is unreadable, that typed error surfaces, notselector_not_found); a sparse capture ends the wait withreason: capture_sparse. Which commands take the budget is one registry trait,targetReadiness: 'budgeted'; a command without it refuses the key; the envelope widens by the capped budget only for those commands; replay reads the trait.Replay: recorded steps carry
targetEvidenceand are verified before dispatch, so the pre-dispatch gate now polls under the same schedule, then hands the remaining budget to the dispatch (one 2 s budget per step, not two). A gate that waited emits the sameinteraction_target_readinessdiagnostic as the daemon, and aselector-missdivergence carrieserror.details.readiness.The wait runs on
observeUntilin capture-kit: one cadence, one deadline rule (the budget bounds when a poll starts), a declared per-capture deadline mode (captureDeadline: 'cancel' | 'none'; a capture that cannot honor the signal is judged late, neverstalled), ride-out withlastError, and a per-poll timeline with afailedoutcome. The verdict stays with the caller. ADR 0011 gains thetargetReadinesscell.resolution.ts(1411 lines onmain) is split by domain question into ten modules, the largest 323 (refactor(move), bodies unchanged).captureDeadlinedefaults to'none'; readiness opts into'cancel'; the schedule has one owner in the selectors policy; the replay gate polls under it and hands the remainder to the dispatch.Over the 1,000-line budget by maintainer decision; the split is most of the churn. Docs:
replay-e2e.md,client-api.md.Validation
Tested commit
d476097d60(rebased on main after #3070): all 19 CI checks pass; localpnpm check:affected --run: 1591 of 1592 files green,scripts/fuzz/harness.test.ts(untouched here) timed out once and passes alone.lastError,minPollspast the budget,failedoutcome). Readiness: all-unreadable surfaces the typed reason; scenarios for target on the third capture, never appears, first capture hits, option absent, sparse.selector-misswithreadiness;fillgets no wait..adround-trip of annotations attarget-annotation-serde.ts:169/249.targetReadinesscells;fillrefusesreadinessTimeoutMs;999_000widens by 2000.