Skip to content

fix: publish replay refs after response projection - #1489

Open
thymikee wants to merge 4 commits into
mainfrom
agent/1478-p1-ref-publication-pr
Open

fix: publish replay refs after response projection#1489
thymikee wants to merge 4 commits into
mainfrom
agent/1478-p1-ref-publication-pr

Conversation

@thymikee

Copy link
Copy Markdown
Member

Summary

  • keep internal replay and Maestro captures from changing client ref authority
  • publish only refs exposed by the exact inline or successful overflow projection, through one-shot session-bound evidence
  • preserve replay lock continuity across fresh-session device binding and strip daemon-internal fields at both public transports

Part of #1478 (P1).

Validation

  • pnpm format:check && pnpm typecheck && pnpm lint
  • full low-contention Vitest run: 547 files, 4,862 tests passed
  • pnpm test:smoke: 24 passed, 2 opt-in live tests skipped
  • pnpm check:affected --base origin/main --run: all non-test gates passed; six aggregate host-load timeouts passed in isolated file reruns (40/40)
  • pnpm maestro:conformance: 46/46
  • iOS live, iPhone 17 Pro simulator: divergence generation 938959 accepted its exposed ref once, then rejected reuse as ref_frame_expired
  • Android live, android-helper 0.20.2 on emulator-5554: divergence generation 553004 produced the same one-shot authority result

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.88 MB 1.88 MB +4.8 kB
JS gzip 604.0 kB 605.4 kB +1.5 kB
npm tarball 722.5 kB 724.0 kB +1.5 kB
npm unpacked 2.53 MB 2.53 MB +4.8 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 25.4 ms 25.6 ms +0.1 ms
CLI --help 52.5 ms 52.3 ms -0.3 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/session.js +3.4 kB +977 B
dist/src/internal/daemon.js +1.3 kB +457 B
dist/src/selector-runtime.js -19 B -4 B

@thymikee

Copy link
Copy Markdown
Member Author

Blocking finding on 98668b73: fresh replay can still lose same-device serialization when the first open is dynamic or a deep link. In that case preflight holds only session:<name>; the nested open reuses the inherited scope and never acquires/promotes device:<id>. A command in another session targeting the newly bound device can interleave before divergence publication, while lineage checks only the replay session and can publish stale refs. Add a cross-session/same-device dynamic-binding regression and hold the device lock from first binding through replay finalization.

@thymikee
thymikee force-pushed the agent/1478-p1-ref-publication-pr branch from 98668b7 to ea02113 Compare July 29, 2026 17:08

Copy link
Copy Markdown
Member Author

Reviewed against the #1478 P1 brief at ea02113. This holds up — and it was reviewed at the behavior-fix bar the brief's risk deserves, including the live-device evidence, which the PR supplies for both platforms with the one-shot property demonstrated end-to-end (accepted once, then ref_frame_expired on reuse).

What's right, specifically:

  • internal-observation.ts makes the engine/daemon authority split structural, not conventional: evidence is an opaque unique symbol type resolved through a module-private WeakMap, so an engine can carry it but cannot forge, inspect, or redeem it. The brief's "engine has no publication capability" is enforced by the type system and module boundary, not by review vigilance.
  • One-shot consumption is airtight: the lineage entry is deleted before every outcome — empty, cancelled, stale, invalid — so request-end finalization can never be retried into publication. The handler even burns the evidence on the no-refs path rather than leaving a live capability dangling.
  • Lineage validation covers the brief's full invalidation list (session identity by name + object + createdAt, snapshot identity, generation, runtime revision, ref-frame quadruple), and the counterfactual suites map to the Done list case-for-case — plus cases the brief didn't demand (same-name session replacement, revision bump on an already-expired frame). Failed publication suppresses the outward refs and removes the orphaned overflow artifact, so "refs exposed count as published" holds in both directions.
  • The internal field strip at both public transports closes the capability-forgery hole symmetrically, and the socket parser keeps its legacy wire shape otherwise.
  • STOP-condition check: no engine-callable publication, finalizer is synchronous with no async gap before response return, no rollback of pre-side-effect expiry (stale-capture preserves newer authority — pinned by test). None tripped.

One nit, non-blocking: in boundReplayDivergenceForSession, the publication.reason === 'empty' branch returns bounded with its refs still visible. That's unreachable for a genuinely empty screen (guarded earlier), so it only fires when screen.refs is non-empty but every ref normalizes to an empty body (@, @~s3) — degenerate refs that are then exposed but unpublished. Harmless in practice since they can't be redeemed, but routing that case through suppressUnpublishedDivergenceRefs too would make "unpublished ⇒ not exposed" exception-free. Fine as a follow-up or a // degenerate refs only comment.

CI is green across the board (one smoke lane still finishing at review time). Good to land from my side.


Generated by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Addressed both review findings in 8b0d63f23:

  • Fresh replay now retains a device lock selected by a nested/dynamic open (including deep links) through outer replay finalization. Added a cross-session regression proving another request for the same device cannot interleave before publication completes.
  • Degenerate unpublished refs such as @ and @~s3 are no longer exposed; only successfully published refs survive response projection, including divergence suggestions.

Validation: pnpm check:affected --run passed with 134 test files / 1,045 tests, plus format, lint, typecheck, layering, fallow, and build. The prior iOS smoke failure was a timeout despite the diagnostic surface containing the requested exact text; the new head will rerun that lane.

@thymikee

Copy link
Copy Markdown
Member Author

Follow-up CI fix pushed in 15c79dbe7.

The iOS fixture lane reproduced the same contradiction twice: XCTest findText returned found: false, but the timeout diagnostic’s canonical snapshot contained the exact Agent Device Tester label. Text wait was treating the Apple query as authoritative; selector wait already falls back after the equivalent miss.

Text wait now treats the Apple query as a fast path: success returns immediately, while a miss is verified against the canonical snapshot. Added a regression for the runner-miss/snapshot-hit case.

Validation: focused snapshot-handler suite 51/51; full affected gate 134 files / 1,046 tests plus format, lint, typecheck, layering, fallow, and build.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed exact head 15c79db; one concurrency gap remains. completeOpenCommand retains the dynamically selected device only immediately before prepareOpenDispatchSession, but selected-device work already happens earlier: iOS runner prewarm can start, relaunch-close can dispatch, and applyRuntimeHintsToApp performs Android/iOS device writes. A fresh deep-link replay therefore still holds only its session lock during those effects, allowing another session on the same device to interleave.

Move retainDeviceExecutionLock(device.id) to the start of completeOpenCommand, before any prewarm/close/runtime-hint work, and extend the router regression to block the first pre-dispatch device-side effect rather than only home after open. The retained-lock lifetime after acquisition and the Apple runner-false → canonical snapshot verification otherwise look sound.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed the final concurrency gap in df0f9c03f.

completeOpenCommand now retains the dynamically selected device immediately on entry, before runner prewarm, relaunch-close, runtime-hint writes, provisional-session setup, or open dispatch.

The router regression now blocks inside pre-dispatch runtime-hint work. While blocked, a request from another session for the same device must remain unsettled; this fails with the old acquisition point and passes with the new one. The retained lock still spans replay finalization.

Validation: focused open/replay suites 31/31; full affected gate 134 files / 1,046 tests plus format, lint, typecheck, layering, fallow, and build.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed exact head df0f9c03; device serialization still begins after real selected-device effects. handleOpenCommand calls prepareOpenCommandDetails before entering completeOpenCommand. That preparation calls ensureDeviceReady (which can boot an iOS simulator, wait for Android boot, or ensure a physical device) and, for an existing session, can call maybeClearRemovedRuntimeTransportHintsclearRuntimeHintsFromApp. The iOS cold-boot hook can also start runner-cache prewarm. Only afterward does completeOpenCommand retain the dynamically selected device.

The revised router test mocks ensureDeviceReady to a no-op and blocks applyRuntimeHintsToApp, so it proves the moved line protects that later effect but not the first production effect. A fresh replay whose device was unavailable during admission can still interleave with another same-device session during readiness/transport cleanup.

Retain immediately after the device is resolved/refreshed and before prepareOpenCommandDetails on both new- and existing-session paths (or otherwise move all device effects behind retention), then make the regression stall the earliest readiness/preparation effect. The later retained-lock lifetime through response/ref finalization remains sound.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant