refactor: extract snapshot and replay identity semantics by thymikee · Pull Request #1582 · callstack/agent-device

thymikee · 2026-08-04T10:25:24Z

Summary

extract shared snapshot text, tree, scroll, geometry, visibility, and tap-point semantics behind @agent-device/contracts/snapshot
move replay identity and structural helpers to @agent-device/ad-script, with guard vocabulary in @agent-device/contracts/replay
preserve root presentation/orchestration and add focused package/parity coverage

Validation

pnpm check:affected --run

github-actions · 2026-08-04T10:26:05Z

Size Report

Metric	Base	Current	Diff
JS raw	1.96 MB	1.96 MB	+64 B
JS gzip	626.9 kB	627.5 kB	+615 B
npm tarball	747.1 kB	747.2 kB	+102 B
npm unpacked	2.62 MB	2.62 MB	+316 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	27.1 ms	27.5 ms	+0.4 ms
CLI --help	63.7 ms	66.1 ms	+2.5 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
`dist/src/screenshot-geometry.js`	+27.3 kB	+9.1 kB
`dist/src/viewport-dimension.js`	+484 B	+111 B
`dist/src/selector-runtime.js`	+59 B	+18 B
`dist/src/cli.js`	+26 B	+12 B
`dist/src/registry.js`	-3 B	+11 B

containsPoint, pickLargestRect, and isRectVisibleInViewport are raw rectangle arithmetic with no snapshot awareness, so they belong beside rectContains/rectArea in @agent-device/kernel/rect rather than in the snapshot-semantics vocabulary. The node-aware resolveViewportRect folds into contracts/snapshot-visibility.ts, retiring snapshot-geometry.ts; after this split, everything behavioral in @agent-device/contracts/snapshot is policy that interprets the snapshot model.

thymikee · 2026-08-04T11:03:02Z

Review of 53f9df7: prerequisite direction is sound, but not ready and the selectors-package GO is not yet earned.

[P1] One selector→root value import remains. src/selectors/predicates.ts still imports findSnapshotAncestor from src/snapshot/snapshot-processing.ts, so an atomic @agent-device/selectors cutover still requires a root import or shim. Move the generic ancestor walker into @agent-device/contracts/snapshot (naturally snapshot-tree), re-export and façade-pin it, repoint both root processing and selectors, and move its non-contiguous-index/cycle coverage with it.
The required refactor: extract shared snapshot and replay identity semantics before selector packaging #1581 remeasurement has not been posted. After the extraction, confirm zero selector→root value imports, the acyclic package DAG/current consumers, and that src/selectors, both replay adapters, and the parity suite can all be deleted atomically.
CI is currently red because packages/contracts/src/snapshot-visibility.ts fails pnpm format:check; iOS Smoke remains pending.

The contracts/ad-script ownership split, package direction, layering, and packaged build otherwise look correct. Keep the PR draft until these items are closed.

thymikee · 2026-08-04T11:29:55Z

Addressed the linked review comment in local commit bd14d7522 (rebased onto 53f9df7ae).

The generic findSnapshotAncestor walker now lives in @agent-device/contracts/snapshot snapshot-tree.ts, is façade-pinned and re-exported, and both selector-side processing and root snapshot processing use it. The non-contiguous-index and cycle-safety cases now live in the contracts package tests.

Remeasurement

Production src/selectors to root src value imports: 0.
Dependency graph: acyclic. check:layering passed 71/71 with zero value-import cycles and zero root back-imports; depgraph:test passed 22/22.
Current production consumers of src/selectors (21):
src/cli/commands/replay.ts, src/commands/capture/wait.ts, src/commands/cli-grammar/common.ts, src/commands/interaction/metadata.ts, src/commands/interaction/runtime/{resolution.ts,selector-capture-policy.ts,selector-read-utils.ts,selector-read.ts,selector-wait.ts}, src/commands/interaction/selectors.ts, src/core/command-descriptor/registry.ts, src/core/{interaction-positionals.ts,press-retarget.ts,wait-positionals.ts}, src/daemon/{direct-ios-selector.ts,handlers/find.ts,parameterized-recorded-fill.ts,replay-selector-port.ts,selector-runtime.ts,session-script-active-publication.ts}, and src/sdk/selectors.ts.
Replay cutover closure:
- createDaemonReplaySelectorPort is consumed in session-replay-runtime.ts, session-replay-divergence.ts, session-replay-maestro-failure.ts, and session-replay-runtime-engine-adapter.ts.
- createInMemoryReplaySelectorPort is consumed only by the dual-adapter contract suite.
- The dual-adapter parity suite imports only those two adapters.

This makes the four targets an eligible single atomic subsequent cutover: src/selectors, the production replay adapter/module, the in-memory adapter, and the dual-adapter parity suite. No package back-import, compatibility shim, or duplicate selector semantic is required by this boundary. Per issue #1581, this prerequisite intentionally does not perform that later selectors-package cutover.

Validation

pnpm lint: pass
pnpm format:check: pass
pnpm typecheck: pass
pnpm test:maestro-compat: 31 files / 197 tests passed
Focused contracts, snapshot-processing, and selector tests: 16/16 passed
Isolated contention reruns: doctor 9/9, runtime-hints 15/15, save-script transports 8/8, Android lifecycle 12/12

The full pnpm check:affected --run chain passed format, lint, typecheck, layering, Fallow, metadata, build, and 391/392 test files, but aggregate coverage repeatedly hit the known Android Settings provider timeout under host load. Since the repository requires pnpm check:affected --run && git push, the push did not execute; the commit is ready locally but is not yet on the PR head.

thymikee · 2026-08-04T11:31:37Z

Both blockers in this review are addressed in local commit 092be0eef (with the subsequent ancestor-walker commit bd14d7522):

The ADR-0012 invariant rationale is restored at the canonical definitions:
- idMatchCountInTree: deliberately no ancestry or parent-walk exclusion; uniqueness is global across the capture and is the shared predicate behind identity and selector id demotion.
- structuralDenotationsEqual: fail-closed comparison of document order and sibling ordinal to prevent same-identity duplicate mis-binding.
- ReplayTargetGuardDenotation: local identity alone is insufficient because a different duplicate could pass verification and receive the action.
- REPLAY_TARGET_GUARD_MISMATCH_REASON and WAIT_LANDMARK_MISMATCH_REASON: explicit who-throws/who-detects protocol-marker contracts.
contracts/replay.ts now references the named LocalIdentity, NodeStructuralDenotation, and TargetAncestryEntry types owned by contracts/target-annotation.ts; there are no inline structural twins.

The PR head is still 53f9df7ae because the repository-required affected gate repeatedly hit aggregate-load Android provider timeouts, so its chained git push did not execute. The local branch is clean and contains the fixes.

…shapes The #1478/#1581 extraction moved the identity/structural helpers but compressed their invariant documentation to one-liners; the deliberate no-ancestry-exclusion rule on idMatchCountInTree, the fail-closed guard comparison, and the who-throws/who-detects contracts on the two divergence reason markers now travel with their definitions again. LocalIdentity and NodeStructuralDenotation move to contracts/target-annotation.ts (beside TargetAncestryEntry, which WaitLandmarkMismatchEvidence now references directly), so the guard shapes in contracts/replay.ts are nominal instead of hand-rolled structural twins; ad-script re-exports the vocabulary beside the readers that produce it. Also inlines the demoteNonUniqueId pass-through wrapper in session-target-evidence.ts.

thymikee · 2026-08-04T12:05:32Z

Re-review of f85a5b3: still not ready; selectors GO is not earned.

[P1] src/selectors/predicates.ts still value-imports findSnapshotAncestor from root src/snapshot/snapshot-processing.ts; @agent-device/contracts/snapshot still does not own/export it. Move the walker into the snapshot package boundary, re-export/pin it, repoint both consumers, and move its non-contiguous-index/cycle coverage.
The mandatory refactor: extract shared snapshot and replay identity semantics before selector packaging #1581 remeasurement is still absent, so zero root imports, the package DAG/consumer map, and the four atomic deletions have not been demonstrated.

CI, formatting, iOS Smoke, layering, and merge state are now green/clean, but keep ready-for-human off until the architectural prerequisite and remeasurement are complete.

The last root value import from src/selectors: predicates.ts reached src/snapshot/snapshot-processing.ts for the ancestor walker. The walker is index-based tree traversal with no presentation policy, so it joins buildSnapshotNodeMap in contracts/snapshot-tree.ts; both consumers repoint to the façade and the non-contiguous-index/cycle coverage moves to the package test. src/selectors now has zero value imports from root src in production files.

thymikee · 2026-08-04T12:14:30Z

Re-review of f85a5b3 addressed at 3e39977c2:

[P1] fixed: findSnapshotAncestor moved into contracts/snapshot-tree.ts beside buildSnapshotNodeMap, exported and pinned through the @agent-device/contracts/snapshot façade; both consumers (src/selectors/predicates.ts, findNearestAncestor in snapshot-processing.ts) repointed; root definition deleted with no compatibility re-export; non-contiguous-index and parent-linkage-cycle coverage moved to packages/contracts/src/snapshot.test.ts. src/selectors production files now have zero value imports from root src.
Remeasurement posted: refactor: extract shared snapshot and replay identity semantics before selector packaging #1581 (comment) — covers the four required points; all four atomic deletions measure as feasible without root imports, shims, or semantic duplication, with one named design decision (AST privacy via façade pins instead of port indirection) for the fresh GO/NO-GO.

Gates at 3e39977c2: typecheck, check:layering, check:production-exports, format:check, and 155 affected tests green locally.

🤖 Addressed by Claude Code

thymikee · 2026-08-04T13:04:02Z

Re-review of 3e39977: clean and merge-ready.

The prior package-boundary blocker is resolved: findSnapshotAncestor now lives in @agent-device/contracts/snapshot, is façade-exported and exact-symbol pinned, both consumers repoint to it, and the focused non-contiguous-index/cycle coverage moved with the implementation. The updated #1581 remeasurement confirms zero production selector→root value imports, an acyclic future dependency path, the remaining consumers, and credible atomic deletion of src/selectors, both replay adapters, and the dual-adapter parity suite. Exact-head CI is green, including layering, package smoke, coverage, and platform smoke.

This clears the prerequisite and gives a separate atomic @agent-device/selectors cutover a GO. Scope guard for that next PR: preserve AST privacy at the public façade and rehome the two daemon-side AST presentation needs as higher-level package operations while deleting the replay adapter—do not keep a compatibility shim.

github-actions · 2026-08-04T13:09:11Z

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-04 13:09 UTC

…n table Thirteen of the fourteen `@agent-device/contracts` façades were bare `export *` barrels. `facades/snapshot.ts`, added by #1582, was the one exception — explicit named re-exports — and that is now the rule. Everything #1574 built to cope with `export *` goes with them: scripts/layering/facade-symbols.ts -980 (816 pinned names) scripts/layering/facade-exports.ts -192 (readFacadeExports) scripts/layering/facade-exports.test.ts -234 (star semantics) scripts/layering/package-boundaries.test.ts -55 `readFacadeExports` re-implemented ESM `GetExportedNames`/`ResolveExport` — star-chain resolution, ambiguity rejection, diamond binding identity, cycle guards, spec-accurate `default` filtering at the star rather than the source. All of it existed to enumerate what `export *` hides. 523 of the 816 pinned names belonged to contracts, i.e. to those thirteen files. Once a façade names its exports, the façade file IS the pin, and it is visible in the diff of the file that widened rather than in a separate table a reviewer has to cross-check. `readNamedExports` (20 lines) stays and is enough: it already throws on bare `export *` and on `export default`. The pin is replaced by one structural gate — no façade may contain a bare star — which reuses that rejection rather than adding a regex. Surface equivalence verified independently, not asserted: main's own `readFacadeExports` run over the new façades, compared against main's own `FACADE_SYMBOLS` table — 31 subpaths, 0 added, 0 removed. Red evidence for the new gate: planting `export * from '../request-progress.ts'` back into facades/progress.ts fails it with the file named and the reason quoted; 12 pass / 0 fail once reverted. Not included: the `lowerAndroidTouchPlan` tuple-assertion drive-by. It needs `sampleGestureOffsets` to carry a min-arity tuple through `.map()`, which TypeScript will not infer without a typed helper — a real change to the gesture-plan contract rather than a drive-by, so it stays out.

…n table (#1614) * refactor(contracts): name façade exports explicitly and retire the pin table Thirteen of the fourteen `@agent-device/contracts` façades were bare `export *` barrels. `facades/snapshot.ts`, added by #1582, was the one exception — explicit named re-exports — and that is now the rule. Everything #1574 built to cope with `export *` goes with them: scripts/layering/facade-symbols.ts -980 (816 pinned names) scripts/layering/facade-exports.ts -192 (readFacadeExports) scripts/layering/facade-exports.test.ts -234 (star semantics) scripts/layering/package-boundaries.test.ts -55 `readFacadeExports` re-implemented ESM `GetExportedNames`/`ResolveExport` — star-chain resolution, ambiguity rejection, diamond binding identity, cycle guards, spec-accurate `default` filtering at the star rather than the source. All of it existed to enumerate what `export *` hides. 523 of the 816 pinned names belonged to contracts, i.e. to those thirteen files. Once a façade names its exports, the façade file IS the pin, and it is visible in the diff of the file that widened rather than in a separate table a reviewer has to cross-check. `readNamedExports` (20 lines) stays and is enough: it already throws on bare `export *` and on `export default`. The pin is replaced by one structural gate — no façade may contain a bare star — which reuses that rejection rather than adding a regex. Surface equivalence verified independently, not asserted: main's own `readFacadeExports` run over the new façades, compared against main's own `FACADE_SYMBOLS` table — 31 subpaths, 0 added, 0 removed. Red evidence for the new gate: planting `export * from '../request-progress.ts'` back into facades/progress.ts fails it with the file named and the reason quoted; 12 pass / 0 fail once reverted. Not included: the `lowerAndroidTouchPlan` tuple-assertion drive-by. It needs `sampleGestureOffsets` to carry a min-arity tuple through `.map()`, which TypeScript will not infer without a typed helper — a real change to the gesture-plan contract rather than a drive-by, so it stays out. * test(layering): assert façades stay exhaustive over their sources Review on #1614 caught this conversion silently narrowing the public surface. The explicit lists were generated against the surface at fork time; #1567 landed 13 exports meanwhile — `DragOptions`, the drag-gesture vocabulary (`COORDINATE_GESTURE_KINDS`, `CoordinateGesturePayload`, the three `DEFAULT_DRAG_*` constants, `DragGestureInput`, `DragGesturePayload`, `GestureCommandInput`, `buildDragGesturePlan`, `dragGesturePayloadFromPositionals`, `normalizeGestureCommandInput`) and `MultiTargetAnnotationV1`. The `export *` barrels had been forwarding all 13 automatically; the rebase dropped every one, and only a human diff caught it. The star-rejection gate could not: it only proves a façade does not WIDEN invisibly. Narrowing is the failure an explicit list newly makes possible, because `export *` could not narrow by construction. So the property the stars gave for free is now asserted directly — every name a re-exported source declares must appear in the façade. Scoped to `packages/*/src/facades/`, the barrels this PR converted. A hand-curated package `index.ts` is a different thing: `ad-replay` deliberately publishes two values out of a much larger `internal/`, and forcing exhaustiveness there would widen a surface its owner narrowed on purpose (#1555). A source that itself carries a bare `export *` is skipped — unknowable from that file alone, and reachable because the façade re-exports the starred module directly too, which IS checked. Red evidence: dropping `MultiTargetAnnotationV1` from facades/replay.ts — one of the 13 the old gate was blind to — fails with the file, the source and the symbol named. 13 pass / 0 fail once restored. * fix(layering): close the exhaustiveness gate's starred-source hole Two review findings, plus a third the gate caught on itself. P1 — the three `DEFAULT_DRAG_*` constants join the existing public-façade suppression, alongside `COORDINATE_GESTURE_KINDS` and `normalizePublicGesture` which the same conversion surfaced. All five are #1567's drag vocabulary, made individually visible to `--production` analysis for the first time because a bare star used to hide them from that exact check. Kept rather than narrowed, for the reason the existing entry already states: the façade's surface stays byte-identical to what the retired pin table asserted, and narrowing is a follow-up with its own review. P2 — the exhaustiveness gate skipped any source carrying a bare `export *`, which dropped that module's DIRECT exports from the check too. `gesture-plan.ts` stars `gesture-plan-types.ts`, so removing `buildDragGesturePlan` from the façade narrowed the public surface and still passed. `readDirectNamedExports` now reads exactly the names a module declares or re-exports BY NAME and ignores the star, so direct exports are checked while the starred set stays covered by the façade's own direct re-export of that module. Red evidence: removing `buildDragGesturePlan` from facades/interaction.ts now fails naming file, source and symbol; 13 pass / 0 fail restored. Third, and the reason the gate is worth having: rebasing onto main after #1612 merged silently dropped `TEXT_ENTRY_ROUTES`, `TextEntryRoute` and `TypeTextBackendResult` from the interaction façade — the same narrowing class as the #1567 one review caught by hand, one merge later. The gate failed on it before CI did. Restored.

refactor: extract snapshot and replay identity semantics

63e3c81

thymikee marked this pull request as ready for review August 4, 2026 11:48

style: fix oxfmt formatting in snapshot-visibility

f85a5b3

thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 4, 2026

thymikee merged commit bcaa106 into main Aug 4, 2026
31 checks passed

thymikee deleted the agent/issue-1581-snapshot-replay-boundaries branch August 4, 2026 13:08

This was referenced Aug 4, 2026

refactor: move selector engine into workspace package #1589

Merged

refactor(contracts): name façade exports explicitly and retire the pin table #1614

Merged

thymikee mentioned this pull request Aug 6, 2026

refactor: extract shared snapshot and replay identity semantics before selector packaging #1581

Closed

Navigation Menu

Uh oh!

Uh oh!

refactor: extract snapshot and replay identity semantics - #1582

refactor: extract snapshot and replay identity semantics#1582
thymikee merged 5 commits into
mainfrom
agent/issue-1581-snapshot-replay-boundaries

thymikee commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

thymikee commented Aug 4, 2026

Summary

Validation

Uh oh!

github-actions Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

thymikee commented Aug 4, 2026

Remeasurement

Validation

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

thymikee commented Aug 4, 2026

Uh oh!

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

github-actions Bot commented Aug 4, 2026 •

edited

Loading