-
-
Notifications
You must be signed in to change notification settings - Fork 325
refactor(interaction): move post-gesture stability and scroll movement onto observeUntil #3078
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8d8b384
4f4793e
3286765
ce509a9
a764d75
249fb95
dfe9629
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { beforeEach, test, vi } from 'vitest'; | ||
| import { emitDiagnostic } from '@agent-device/host-kit/diagnostics'; | ||
| import type { ObservationClock } from './observe-until.ts'; | ||
| import { | ||
| runPostGestureStabilityLoop, | ||
| type PostGestureStabilityHooks, | ||
| } from './post-gesture-stability.ts'; | ||
|
|
||
| vi.mock('@agent-device/host-kit/diagnostics', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('@agent-device/host-kit/diagnostics')>(); | ||
| return { ...actual, emitDiagnostic: vi.fn() }; | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| vi.mocked(emitDiagnostic).mockClear(); | ||
| }); | ||
|
|
||
| type Surface = Readonly<{ signature: readonly string[]; costMs: number }>; | ||
|
|
||
| function fakeClock(): ObservationClock & { advance(ms: number): void } { | ||
| let nowMs = 0; | ||
| return { | ||
| now: () => nowMs, | ||
| advance: (ms) => { | ||
| nowMs += ms; | ||
| }, | ||
| sleep: async (ms) => { | ||
| nowMs += ms; | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| /** Captures that each cost their own `costMs` of clock time and ignore any signal. */ | ||
| function hooksFor( | ||
| clock: ReturnType<typeof fakeClock>, | ||
| surfaces: readonly Surface[], | ||
| ): PostGestureStabilityHooks<Surface, readonly string[]> { | ||
| let calls = 0; | ||
| const same = (a: readonly string[], b: readonly string[]) => a.join() === b.join(); | ||
| return { | ||
| capture: async () => { | ||
| const surface = surfaces[calls]; | ||
| calls += 1; | ||
| if (!surface) throw new Error('the loop captured more surfaces than the case supplied'); | ||
| clock.advance(surface.costMs); | ||
| return surface; | ||
| }, | ||
| readSurface: (value) => ({ signature: value.signature, backend: 'xctest' }), | ||
| signaturesStable: same, | ||
| classifyBaselineEvidence: (baseline, quiet) => | ||
| same(baseline, quiet) ? 'unchanged' : 'changed', | ||
| surfacesIdentical: same, | ||
| summarizeDivergence: () => ({}), | ||
| }; | ||
| } | ||
|
|
||
| const PENDING = { action: 'scroll', positionals: ['down'] }; | ||
|
|
||
| function timeoutWarnings(): unknown[] { | ||
| return vi | ||
| .mocked(emitDiagnostic) | ||
| .mock.calls.filter(([event]) => event.phase === 'post_gesture_snapshot_stabilization_timeout'); | ||
| } | ||
|
|
||
| test('a capture that runs past the remaining budget is judged and ends unsettled, not thrown', async () => { | ||
| const clock = fakeClock(); | ||
| const late: Surface = { signature: ['b'], costMs: 2_000 }; | ||
|
|
||
| const outcome = await runPostGestureStabilityLoop({ | ||
| pending: PENDING, | ||
| needsBaselineDistrust: false, | ||
| hooks: hooksFor(clock, [{ signature: ['a'], costMs: 0 }, late]), | ||
| clock, | ||
| }); | ||
|
|
||
| assert.equal(outcome.value, late); | ||
| assert.deepEqual(outcome.postGestureOutcome, { | ||
| kind: 'unsettled', | ||
| gesture: { action: 'scroll', positionals: ['down'] }, | ||
| }); | ||
| assert.equal(timeoutWarnings().length, 1); | ||
| }); | ||
|
|
||
| test('a first capture slower than the whole budget still forms a quiet pair', async () => { | ||
| const clock = fakeClock(); | ||
| const quiet: Surface = { signature: ['a'], costMs: 0 }; | ||
|
|
||
| const outcome = await runPostGestureStabilityLoop({ | ||
| pending: PENDING, | ||
| needsBaselineDistrust: false, | ||
| hooks: hooksFor(clock, [{ signature: ['a'], costMs: 2_000 }, quiet]), | ||
| clock, | ||
| }); | ||
|
|
||
| assert.equal(outcome.value, quiet); | ||
| assert.equal(outcome.postGestureOutcome, undefined); | ||
| assert.equal(timeoutWarnings().length, 0); | ||
| }); | ||
|
|
||
| test('a capture error ends the loop by rethrowing that error', async () => { | ||
| const clock = fakeClock(); | ||
| const failure = new Error('capture failed'); | ||
| const hooks = hooksFor(clock, []); | ||
|
|
||
| await assert.rejects( | ||
| runPostGestureStabilityLoop({ | ||
| pending: PENDING, | ||
| needsBaselineDistrust: false, | ||
| hooks: { | ||
| ...hooks, | ||
| capture: async () => { | ||
| throw failure; | ||
| }, | ||
| }, | ||
| clock, | ||
| }), | ||
| (error) => error === failure, | ||
| ); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,21 +1,31 @@ | ||||||||||||||||||||||
| import { emitDiagnostic } from '@agent-device/host-kit/diagnostics'; | ||||||||||||||||||||||
| import { sleep } from '@agent-device/host-kit/retry'; | ||||||||||||||||||||||
| import type { PostGestureAction, PostGestureOutcome } from '@agent-device/kernel/snapshot'; | ||||||||||||||||||||||
| import { observeUntil, type ObservationClock, type ObservationSchedule } from './observe-until.ts'; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Pure post-gesture stability mechanics: the quiet-window polling loop and the | ||||||||||||||||||||||
| * baseline-distrust verdict, parameterized over the capture value and the | ||||||||||||||||||||||
| * signature comparators. Deliberately a leaf — it imports no cycle owners and | ||||||||||||||||||||||
| * no `SessionState`, so it stays outside the R9 type cycle while the | ||||||||||||||||||||||
| * deferred-interaction-outcome owner (which holds the pending record and the | ||||||||||||||||||||||
| * session mutation) stays the one seam callers see. The owner supplies the | ||||||||||||||||||||||
| * comparators from interaction-outcome-policy; their semantics (subset | ||||||||||||||||||||||
| * tolerance, identity keying, discriminating entries) are documented there. | ||||||||||||||||||||||
| * Pure post-gesture stability mechanics: the quiet-window verdict over the shared | ||||||||||||||||||||||
| * `observeUntil` engine, plus the baseline-distrust decision, parameterized over | ||||||||||||||||||||||
| * the capture value and the signature comparators. Deliberately a leaf — it | ||||||||||||||||||||||
| * imports no cycle owners and no `SessionState`, so it stays outside the R9 | ||||||||||||||||||||||
| * type cycle while the deferred-interaction-outcome owner (which holds the | ||||||||||||||||||||||
| * pending record and the session mutation) stays the one seam callers see. The | ||||||||||||||||||||||
| * owner supplies the comparators from interaction-outcome-policy; their | ||||||||||||||||||||||
| * semantics (subset tolerance, identity keying, discriminating entries) are | ||||||||||||||||||||||
| * documented there. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const STABILIZATION_DEADLINE_MS = 1_500; | ||||||||||||||||||||||
| const STABILIZATION_INTERVAL_MS = 200; | ||||||||||||||||||||||
| const STABILIZATION_MIN_ATTEMPTS = 2; | ||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Cadence and budget for the quiet-window loop: poll every 200ms, allow 1.5s | ||||||||||||||||||||||
| * before an unsettled surface times out, and always complete two observations | ||||||||||||||||||||||
| * (an `initial` counts) so a quiet pair can form even under a tight budget. | ||||||||||||||||||||||
| * No per-capture deadline: `hooks.capture` takes no signal, so a late capture | ||||||||||||||||||||||
| * is judged when it returns. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| const POST_GESTURE_STABILITY_SCHEDULE: ObservationSchedule = { | ||||||||||||||||||||||
| intervalMs: 200, | ||||||||||||||||||||||
| budgetMs: 1_500, | ||||||||||||||||||||||
| minPolls: 2, | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Defect 2 (#1542): a bounded extra budget used ONLY when a quiet signature | ||||||||||||||||||||||
|
|
@@ -30,7 +40,7 @@ const STABILIZATION_MIN_ATTEMPTS = 2; | |||||||||||||||||||||
| * settle-zero-margin-flake, a week-long contention-flake root cause), so this | ||||||||||||||||||||||
| * cap is sized to never come close to that trap. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| const STABILIZATION_DISTRUST_DEADLINE_MS = STABILIZATION_DEADLINE_MS + 2_000; | ||||||||||||||||||||||
| const STABILIZATION_DISTRUST_DEADLINE_MS = POST_GESTURE_STABILITY_SCHEDULE.budgetMs + 2_000; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export type BaselineSurfaceEvidence = 'changed' | 'unchanged' | 'ambiguous'; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -116,38 +126,52 @@ export function decidePostGestureStabilityVerdict<S extends readonly unknown[]>( | |||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * The quiet-window stability loop: poll until two consecutive captures agree | ||||||||||||||||||||||
| * and the verdict accepts the agreement, or the (possibly distrust-extended) | ||||||||||||||||||||||
| * deadline expires. Session state never enters here — the caller owns the | ||||||||||||||||||||||
| * pending record's lifecycle and clears it when this returns. | ||||||||||||||||||||||
| * The quiet-window stability verdict over the shared observation engine: keep | ||||||||||||||||||||||
| * polling until two consecutive captures agree and the decision accepts the | ||||||||||||||||||||||
| * agreement, or the (possibly distrust-extended) budget expires. Session | ||||||||||||||||||||||
| * state never enters here — the caller owns the pending record's lifecycle | ||||||||||||||||||||||
| * and clears it when this returns. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| export async function runPostGestureStabilityLoop<T, S extends readonly unknown[]>(params: { | ||||||||||||||||||||||
| pending: PostGestureStabilityPending<S>; | ||||||||||||||||||||||
| needsBaselineDistrust: boolean; | ||||||||||||||||||||||
| initial?: T; | ||||||||||||||||||||||
| hooks: PostGestureStabilityHooks<T, S>; | ||||||||||||||||||||||
| clock?: ObservationClock; | ||||||||||||||||||||||
| }): Promise<PostGestureStabilityOutcome<T>> { | ||||||||||||||||||||||
| const { pending, needsBaselineDistrust, hooks } = params; | ||||||||||||||||||||||
| const startedAt = Date.now(); | ||||||||||||||||||||||
| let attempts = 1; | ||||||||||||||||||||||
| let previous = await captureSurface(hooks, params.initial); | ||||||||||||||||||||||
| const { pending, needsBaselineDistrust, hooks, clock } = params; | ||||||||||||||||||||||
| const now = () => clock?.now() ?? Date.now(); | ||||||||||||||||||||||
| const startedAt = now(); | ||||||||||||||||||||||
| let attempts = 0; | ||||||||||||||||||||||
| let baselineSignature = pending.baselineSignature; | ||||||||||||||||||||||
| let baselineBackend = pending.baselineBackend; | ||||||||||||||||||||||
| let baselineRebased = false; | ||||||||||||||||||||||
| // Extended past STABILIZATION_DEADLINE_MS only when the distrust verdict | ||||||||||||||||||||||
| // fires below; the ordinary (non-distrust) timeout path is unaffected. | ||||||||||||||||||||||
| let effectiveDeadlineMs = STABILIZATION_DEADLINE_MS; | ||||||||||||||||||||||
| // A rebase or a distrust verdict keeps polling on a pair that DID agree, so | ||||||||||||||||||||||
| // 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> => { | ||||||||||||||||||||||
| if (surfaceCache?.value === value) return surfaceCache; | ||||||||||||||||||||||
| surfaceCache = { value, ...hooks.readSurface(value) }; | ||||||||||||||||||||||
| return surfaceCache; | ||||||||||||||||||||||
|
Comment on lines
+152
to
+157
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| while (attempts < STABILIZATION_MIN_ATTEMPTS || Date.now() - startedAt < effectiveDeadlineMs) { | ||||||||||||||||||||||
| await sleep(STABILIZATION_INTERVAL_MS); | ||||||||||||||||||||||
| attempts += 1; | ||||||||||||||||||||||
| const current = await captureSurface(hooks); | ||||||||||||||||||||||
| lastPairAgreed = hooks.signaturesStable(previous.signature, current.signature); | ||||||||||||||||||||||
| if (lastPairAgreed) { | ||||||||||||||||||||||
| const elapsedMs = Date.now() - startedAt; | ||||||||||||||||||||||
| const observed = await observeUntil<T, PostGestureStabilityOutcome<T>>({ | ||||||||||||||||||||||
| ...(params.initial !== undefined ? { initial: params.initial } : {}), | ||||||||||||||||||||||
| capture: () => hooks.capture(), | ||||||||||||||||||||||
| schedule: POST_GESTURE_STABILITY_SCHEDULE, | ||||||||||||||||||||||
| ...(clock ? { clock } : {}), | ||||||||||||||||||||||
| verdict: (latest, previousValue) => { | ||||||||||||||||||||||
| attempts += 1; | ||||||||||||||||||||||
| const current = surfaceOf(latest); | ||||||||||||||||||||||
| if (previousValue === undefined) return { kind: 'continue' }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const previous = surfaceOf(previousValue); | ||||||||||||||||||||||
| lastPairAgreed = hooks.signaturesStable(previous.signature, current.signature); | ||||||||||||||||||||||
| if (!lastPairAgreed) return { kind: 'continue' }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const elapsedMs = now() - startedAt; | ||||||||||||||||||||||
| // A capture plan may fall back or be pre-empted by the XCTest-channel | ||||||||||||||||||||||
| // penalty at any time, so the backend can change mid-poll. Backends do | ||||||||||||||||||||||
| // not agree on which nodes exist, so this pair says nothing about the | ||||||||||||||||||||||
|
|
@@ -162,8 +186,7 @@ export async function runPostGestureStabilityLoop<T, S extends readonly unknown[ | |||||||||||||||||||||
| baselineSignature = current.signature; | ||||||||||||||||||||||
| baselineBackend = current.backend; | ||||||||||||||||||||||
| baselineRebased = true; | ||||||||||||||||||||||
| previous = current; | ||||||||||||||||||||||
| continue; | ||||||||||||||||||||||
| return { kind: 'continue' }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| const verdict = decidePostGestureStabilityVerdict({ | ||||||||||||||||||||||
| needsBaselineDistrust, | ||||||||||||||||||||||
|
|
@@ -174,28 +197,33 @@ export async function runPostGestureStabilityLoop<T, S extends readonly unknown[ | |||||||||||||||||||||
| classifyBaselineEvidence: hooks.classifyBaselineEvidence, | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
| if (verdict === 'distrust') { | ||||||||||||||||||||||
| effectiveDeadlineMs = STABILIZATION_DISTRUST_DEADLINE_MS; | ||||||||||||||||||||||
| previous = current; | ||||||||||||||||||||||
| continue; | ||||||||||||||||||||||
| return { kind: 'continue', budgetMs: STABILIZATION_DISTRUST_DEADLINE_MS }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| emitSettleDiagnostic(verdict, pending.action, attempts, elapsedMs); | ||||||||||||||||||||||
| return buildAcceptedOutcome(verdict, pending, current, hooks, baselineRebased); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| previous = current; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return { | ||||||||||||||||||||||
| kind: 'done', | ||||||||||||||||||||||
| result: buildAcceptedOutcome(verdict, pending, current, hooks, baselineRebased), | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (observed.kind === 'done') return observed.result; | ||||||||||||||||||||||
| if (observed.kind === 'failed') throw observed.error; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| emitDiagnostic({ | ||||||||||||||||||||||
| level: 'warn', | ||||||||||||||||||||||
| phase: 'post_gesture_snapshot_stabilization_timeout', | ||||||||||||||||||||||
| data: { | ||||||||||||||||||||||
| action: pending.action, | ||||||||||||||||||||||
| attempts, | ||||||||||||||||||||||
| durationMs: Date.now() - startedAt, | ||||||||||||||||||||||
| durationMs: observed.waitedMs, | ||||||||||||||||||||||
| lastPairAgreed, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
| if (lastPairAgreed) return { value: previous.value }; | ||||||||||||||||||||||
| return { value: previous.value, postGestureOutcome: postGestureOutcome('unsettled', pending) }; | ||||||||||||||||||||||
| // minPolls guarantees at least one judged value before an `expired` end can fire. | ||||||||||||||||||||||
| const value = observed.last as T; | ||||||||||||||||||||||
| if (lastPairAgreed) return { value }; | ||||||||||||||||||||||
| return { value, postGestureOutcome: postGestureOutcome('unsettled', pending) }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| type CapturedSurface<T, S> = { | ||||||||||||||||||||||
|
|
@@ -204,14 +232,6 @@ type CapturedSurface<T, S> = { | |||||||||||||||||||||
| backend: string | undefined; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| async function captureSurface<T, S extends readonly unknown[]>( | ||||||||||||||||||||||
| hooks: PostGestureStabilityHooks<T, S>, | ||||||||||||||||||||||
| initial?: T, | ||||||||||||||||||||||
| ): Promise<CapturedSurface<T, S>> { | ||||||||||||||||||||||
| const value = initial ?? (await hooks.capture()); | ||||||||||||||||||||||
| return { value, ...hooks.readSurface(value) }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| function emitSettleDiagnostic( | ||||||||||||||||||||||
| verdict: 'trust' | 'accept-stale', | ||||||||||||||||||||||
| action: string, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: The loop's other timeout shape is untested: every case here passes
needsBaselineDistrust: falseand 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 nounsettledoutcome (post-gesture-stability.ts: theif (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 withneedsBaselineDistrust: truewhere captures keep agreeing on the unchanged baseline past the 1.5s cap, and assertpostGestureOutcomestays undefined while the stabilization_timeout warning fires.Prompt for AI agents