fix(scroll): observe the movement a directional scroll can claim

[thymikee]

What

The observation itself, as one module: src/daemon/scroll-movement.ts. It reads the difference a directional scroll can actually prove — the tree the session already held before the gesture, and one tree after it — and answers what that evidence supports. #2771 wires scroll <direction> to it; nothing here changes an answer yet, which is why it is a separate layer: the rule and its 13 pinned behaviors can be reviewed apart from the dispatch seam, the runtime-use declaration, and the response shape that consume them.

evidence	answer
surface differs from the baseline	moved — one capture
unchanged, container still hides content in that direction, swipe ran inside it	throws scroll_no_progress with the container frame and the recovery hint
unchanged, nothing left to reveal	at-edge, no distance claimed
unchanged, horizontal (no end-of-content signal exists)	unchanged — the measurement, not a guess about which of the two it was
anything not comparable	unobserved

The three rules that make the claim worth having

1. Nothing is classified before the pair is established as two views of one screen. presentationKey and the comparison key are checked on every capture, before the changed/unchanged classifier runs — so a pair from different capture lineages (#1569's XCTest-channel fallback swapping the producer mid-request, or a snapshot -i baseline against this command's broad read) cannot produce moved either. The deferred loop adopts a new producer and keeps polling, because its question is whether the surface settles; this one compares against a pre-gesture tree, so a drifted pair is withheld rather than re-based. Pinned by a test whose frames genuinely differ, so the gate is proven on the path that would otherwise claim movement.
2. The baseline is frozen while it is still current. session.snapshot is not automatically the screen as it stands. ADR 0014 already encodes the question — the ref frame stays active while no device side effect has crossed since the last publication — so after a tap, scroll answers unobserved and spends zero captures rather than crediting its own swipe with what the tap changed (stored-surface-not-current). Same for a gesture nobody has read yet (prior-gesture-unsettled).
3. An untouched surface must be at rest before it is reported. A refusal needs one quiet pair, because a gesture still in flight and a gesture that did nothing answer a single read identically. A surface that never holds still (ticking clock, blinking spinner) expires its 1.5 s budget with a warn and answers unobserved — refusing there would fail a working scroll over a busy screen.

Evidence rules are imported, not restated: the signature, the subset-tolerant baseline classifier and the strict both-directions no-effect bar all come from interaction-outcome-policy.ts, so "that scroll did nothing" means one thing in this daemon (#1601 review P1). Where a capture cannot be trusted, deferred-interaction-outcome.ts and this module now read the same snapshotSurfaceComparisonKey — that owner moved here from the former private copy, and both callers were repointed in the same commit.

What this round changed

moved is now confined to the scroller the swipe ran in. Review note 2 was right and it is measurable: on the Android tester's /catalog, changing only a system status-bar icon (dumpsys battery set level) turned the pair into "changed" over a list whose own scroll-state label never moved — the module answered movement: 'moved', attempts: 1. A whole-surface difference is not the gesture's doing when it lives outside the container, so claimMoved resolves the container from the tree it already read (no extra capture) and asks the shared comparator whether the region changed. It did not → movement: 'unobserved' with the typed reason change-outside-container. With no container to confine to (no scroller resolved, or the horizontal axis the analyzer does not read) the whole-surface answer stands. The comparator, discriminatingSurfaceChangedWithinRect, lives in interaction-outcome-policy.ts beside the strict bar it reuses, and is tested there as well as through the module.

The plan/reporter surface is smaller. ScrollMovementDecline, FrozenScrollSurface and ScrollMovementBlindReason are gone: ScrollMovementPlan is the only plan union (its declined arm names its own Exclude), freezeScrollSurfaceBaseline returns the plan, and reportBlind + reportScrollMovementUnobservable are one reportScrollMovementUnobserved(direction, reason, evidence) over one exported reason union. Two reporters remain rather than one, because the response shapes they describe are different facts: reportScrollMovementNotApplicable accompanies no movement field at all (this command owed no observation), while reportScrollMovementUnobserved accompanies movement: 'unobserved' (it owed one and could not make it). Collapsing them would put an absent field and a present-but-blind field in one log phase.

verticalEdgeFor is imported, not copied — from packages/capture-kit/src/snapshot/scroll-edge-state.ts, where ScrollEdge lives, next to the axis-honest formatScrollEdgeMessage change in #2770. scroll-until.ts imports it too, so --until and the observation cannot drift apart about which directions have an end signal.

CI's Repo Guards finding is resolved at the owner, not suppressed. planScrollMovement and the two reporters were flagged as unused exports because nothing imported them until #2771; they are now exercised here — the four plan arms and both reporters are asserted, which also closes "the tests assert unobserved without checking the reason": the reason and phase are read out of a mocked emitDiagnostic in every withheld case.

Not taken this round, deliberately: the poll's bare catch and sleep still ignore request cancellation, which needs an AbortSignal plumbed from the request scope rather than a local fix — the same gap the other polling loops in this daemon have.

Validation

• Full pnpm check:affected --run → all runnable checks passed on the stack head (882256121): lint, format, typecheck, layering, fallow, 585 files / 4,356 tests. At this layer's own head (d97aba9fb): 24 module tests + 21 policy tests, tsc, and pnpm check:fallow --base fix/scroll-surface-vocabulary clean.
• 24 module tests, each verdict paired with the closest input that must not produce it: refusal vs at-edge vs no-container vs swipe-outside-container; moved vs a surface still in motion; moved vs a change that never touched the container; the strict-bar case (a toast over an unchanged list is not a no-op); budget expiry asserted as calls > 2 so it cannot pass by accidentally forming a quiet pair; a capture that throws answers unobserved instead of failing the scroll; every withheld answer asserts its reason.
• Red/green: removing the lineage gate turns the two drift tests red; removing the container confinement turns the change-outside-container test red; the capture-count assertions go red if the fast path stops being one capture.

No device run is owed by this layer — it is unreachable from the CLI until #2771, which carries the live evidence (including the Android measurement that produced the container confinement above).

🤖 Generated with Apex by Callstack

[github-actions]

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	4.79 MB	4.79 MB	0 B
Package (unpacked)	4.79 MB	4.79 MB	0 B
Package (download)	1.43 MB	1.43 MB	0 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	29.8 ms	28.3 ms	-1.5 ms
CLI --help	84.2 ms	84.0 ms	-0.2 ms

[thymikee]

Reviewed f728698. One blocking finding and one design question.

verticalEdgeFor(direction) in scroll-movement.ts#L546 is an exact copy of the function in scroll-until.ts#L286-L290. If horizontal edge support comes to readScrollEdgeState, one copy can drift, and then --until and the directional observation will disagree about which direction has an end signal. Please export it once from packages/capture-kit/src/snapshot/scroll-edge-state.ts, next to ScrollEdge, and import it in both places.

Can this module be smaller? It is 552 lines, which is under the size threshold. But it adds a plan union, three reason unions and four diagnostic reporters. A separate loop from runPostGestureStabilityLoop looks justified, because this one exits on the first change and holds back the claim on drift. But could ScrollMovementInapplicability, ScrollMovementDecline and FrozenScrollSurface become one plan union, and reportBlind and reportScrollMovementUnobservable one reporter? If there is a reason for the current split, please say what it is.

Smaller notes, not blocking. Any of them can wait for a follow-up:

• The bare catch in captureSurface treats cancellation the same as an unreadable tree, and the poll sleep ignores cancellation.
• Could a toast or a keyboard change outside the container count as "moved" after a no-op swipe? classifyBaselineSurfaceEvidence does not check the container rect.
• The tests assert unobserved without checking the reason.
• The long narrative comments could be cut down to the invariants.

CI was queued with no failures. Repo Guards may flag planScrollMovement and the two reporters as unused at this layer, because nothing imports them until #2771. Next step: move the edge helper, then answer the size question.

[thymikee]

I reviewed a26d175 as a follow-up to the earlier findings on f728698 (#2776 (comment)), and the code here is clean.

I did not run the tests, so I have not reproduced the author's claim that removing the lineage gate turns the drift tests red. This layer is not reachable from the CLI, so live device evidence of the scroll route belongs to #2771/#2773 and I did not judge it here. The earlier non-blocking notes on cancellation in captureSurface and on changes outside the container counting as "moved" are outside this delta, so I have not re-raised them; whether they matter depends on the wiring in #2771.

All 16 checks were still queued when I looked, and none had failed yet. Repo Guards (fallow unused exports) overlaps this diff: planScrollMovement, observeScrollMovement, and reportScrollMovementNotApplicable have no production importer until #2771 wires them in, so that check may flag them as unused. Handle that at the stack level once #2771 lands rather than adding an allowlist entry here.

[thymikee]

All three due items are in, on 048283105. Two of your "not blocking" notes turned out to be load-bearing, and one of them is now a behavior change — detail below.

verticalEdgeFor — moved, not copied. It lives next to ScrollEdge in packages/capture-kit/src/snapshot/scroll-edge-state.ts and both scroll-until.ts and scroll-movement.ts import it. The move landed in #2770 so the axis definition has one owner for --until, the movement module, and the message formatter (which now refuses to speak "above" about a horizontal scroll).

The type surface is smaller. Gone: ScrollMovementDecline (the declined arm names its own Exclude<ScrollMovementInapplicability, 'owner-without-capture'>), FrozenScrollSurface (freezeScrollSurfaceBaseline returns ScrollMovementPlan), ScrollMovementBlindReason (folded into the one exported reason union), and reportBlind — reportScrollMovementUnobserved(direction, reason, evidence) now covers the pre-gesture baseline absences and the during-poll blindness, with evidence typed (swipe, divergence, containerRect) instead of a Record.

Why two reporters remain. They describe two different response shapes, and collapsing them would merge two facts that the response keeps apart: reportScrollMovementNotApplicable accompanies no movement field at all (this command owed no observation of its own effect), while reportScrollMovementUnobserved accompanies movement: 'unobserved' (it owed one and the evidence was not there). One phase for both would make the log say less than the response does. If you want a single reporter anyway, the honest version is one function over the whole ScrollMovementPlan returning the claim — I would take that only with the phase split kept.

Your note 2 is a real defect, and I could reproduce it. classifyBaselineSurfaceEvidence does not consult the container rect, and on Android the captured tree carries the status bar. On the tester's /catalog, changing only a system icon (adb shell dumpsys battery set level 90 → 40) between the baseline and the post-gesture read produced movement: 'moved', attempts: 1, durationMs: 118 over a list whose own catalog-scroll-state label never moved. So moved is now confined: claimMoved resolves the container from the tree it has already read (no extra capture), asks discriminatingSurfaceChangedWithinRect(baseline, observed, containerRect), and answers unobserved with the new reason change-outside-container when every difference lies outside the scroller. Deliberately not unchanged: a difference the gesture cannot be credited with also cannot convict it, and the no-effect bar belongs to the strict comparator, not to this deduction. With no container to confine to — no scroller resolved, or the horizontal axis the analyzer does not read — the whole-surface answer stands, and there is a test for that too so the confinement cannot silently become a requirement.

The comparator is in interaction-outcome-policy.ts beside haveIdenticalDiscriminatingSurfaces, reusing the same key matching and rect tolerance, with three tests there (inside-region movement true, outside-region-only change false while the whole-surface classifier says changed, content appearing inside the region true) plus two through the module. Red/green verified by stubbing the confinement.

Reasons are now asserted, not implied. The diagnostics sink is mocked in scroll-movement.test.ts; every withheld answer pins phase + reason, and planScrollMovement's four arms plus both reporters are exercised at this layer — which is also what clears the Repo Guards finding you predicted: those three exports were dead here only until their own tests existed. Suppressions were not used.

Not taken this round, on purpose: the bare catch in captureSurface and the poll sleep still treat cancellation like an unreadable tree / a full budget. Fixing that means threading the request's AbortSignal into the loop, which is the same gap pollForScrollRest and the stabilization loop have; a local half-fix here would make the three loops disagree about what cancellation means. I would rather take it across all three. Comment trimming: the plan and verdict blocks are shorter than before, but I kept the ones that carry a defect number, since those are the sentences that stop a future reader re-litigating #1542/#1601/#2714.

Module is 591 lines (was 552): +39 for the confinement, which is the part the device evidence demanded. CI: pnpm check:fallow --base fix/scroll-surface-vocabulary → no issues in the layer's files; 24 module + 21 policy tests; tsc clean.

🤖 Generated with Apex by Callstack

[thymikee]

The confinement gap I flagged in f728698 (#2776 (comment)) is still open at 0482831.

claimMoved still calls readScrollEdgeState(observed.nodes, edge) with no target (https://github.com/callstack/agent-device/blob/0482831/src/daemon/scroll-movement.ts#L1219), so analyzeScrollEdgeState picks the container by edge hints and viewport center, not by the swipe point, and claimMoved never checks containerHoldsSwipe the way decideEdgeVerdict does. Picture a split view with two vertical scrollers: swipe lands in list B and does nothing, but list A auto-scrolls on its own; the result comes back moved. Swap it around and B really scrolls while A is selected, and the result is unobserved with reason change-outside-container, which is also wrong. The doc comment says "inside the scroller the swipe ran in", but nothing enforces that today. Once #2771 wires this in, a no-op directional scroll can claim movement from another container, the same false credit the delta exists to remove, now scoped to scrollers instead of chrome, and diagnostics will mislabel real movement as unobserved or vice versa. Every site that resolves "the swiped container" — claimMoved and decideEdgeVerdict — needs to go through one path keyed on the swipe point: pass swipe.midpoint as ScrollEdgeTarget.point through readScrollEdgeState (analyzeScrollEdgeState already accepts a target), and share one helper between both call sites that returns either the container holding the swipe or container-outside-swipe. Can we get a two-scroller test where only the unswiped container changes, asserting the result is not moved?

Not blocking: the confinement in claimMoved runs only after the loop has already exited on a whole-surface changed read (https://github.com/callstack/agent-device/blob/0482831/src/daemon/scroll-movement.ts#L1224), so a first-read chrome change (clock, battery) while the list scroll is still in flight returns unobserved immediately instead of letting the loop poll again and see moved — worth confining the per-read verdict inside readOneCapture instead of after the loop exits, take or leave; and discriminatingEntriesWithinRect hand-writes its own exclusive rect-overlap test (https://github.com/callstack/agent-device/blob/0482831/src/daemon/interaction-outcome-policy.ts#L496) when isRectVisibleInViewport in packages/kernel/src/rect.ts already owns rect overlap with inclusive edges, so reusing it or adding an exclusive variant there would be nice but isn't required now.

I did not run tests, tsc, or fallow, and did not reproduce the author's red/green stub claim. The two-scroller mechanism above comes from reading how analyzeScrollEdgeState selects a container (viewport center and edge-hint fallback with no target), not from a device run on a real multi-scroller screen. This layer has no production importer until #2771, so live device behavior of the confinement belongs there; the author's Android /catalog battery repro was not re-run either. Size stays at 634 net production lines, under the 700 threshold, so the earlier size question is closed. CI is still running Smoke Tests with no failure yet; the only change reachable from that route is the rename-only move of snapshotComparisonKey to snapshotSurfaceComparisonKey on the post-gesture stabilization path in deferred-interaction-outcome.ts, with an identical body, and scroll-movement.ts has no production importer yet, so a smoke failure looks unlikely to trace back to this delta. The code isn't ready to merge until the confinement is resolved through the swipe point in one shared path for claimMoved and decideEdgeVerdict, with a two-scroller test proving the unswiped list can't claim movement.

The head has since moved to d97aba9, a rebase onto main in which this commit is patch-identical, so the review above still applies.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-23 14:20 UTC