refactor(daemon): serialize replay transactions behind a locked coordinator (#1478 P4b)

[thymikee]

#1478 P4b: all use of the P4a replay projection is serialized behind one daemon-owned, request-scoped coordinator.

What this actually is (13 files)

• src/daemon/session-replay-coordinator.ts — ReplayCoordinator, created exactly once per locked native .ad replay request in runReplayScriptFile. It owns every repair-transaction write on the replay path: per-step arming (boundary stamps once, explicit <out> wins over the healed sibling), --from demotion, completion, divergence-hold stamping (markSessionHeldIfArmed), tombstone clear, and the pendingRecordAndHeal corrective watermark (set + clear, moved bodily from session-replay-resume.ts; the R7 ownership row moved with it).
• ReplayResumeStamper — the narrow bound capability (sessionExists(), stampCorrectiveWatermark()) threaded from that single coordinator through ReplayStepContext and the failure-wrapper params into both divergence routes (target-verification and the action-failure chain). buildAndPersistReplayDivergenceResume takes the stamper and can no longer construct a coordinator or receive SessionStore + session name — the review's blocking finding, closed at cb35d978.
• ReplaySessionView — minimal immutable read projection (repairBoundary, pendingRecordAndHeal) for the read sites this slice touches.
• src/daemon/__tests__/replay-coordinator-ownership.test.ts — the structural test the review asked for: oxc-parser AST scan asserting createReplayCoordinator has exactly one production call site and that none of the five divergence-chain files imports the factory, session-replay-transaction.ts, or SessionStore (value-level). Plant-proven in both directions.

Deliberate boundary: close-time sequencing (session-close.ts receipts, session-close-script.ts commit/abort) stays a direct session-replay-transaction.ts caller — teardown is a different capability with its own ordering, not part of the locked replay request. General lifecycle read-only callers (store, recorder, writer, idle-reap) likewise stay direct readers.

Validation

• Gates at head: typecheck, lint, format, check:layering (R7/R10 baselines unchanged by the rebase; the ownership row move was in the original commit), fallow zero at stack scope, full suite green.
• The rebases onto merged refactor(daemon): session script publication behind one capability (#1478 P4a) #1532 and test(daemon): pin resolveScriptTarget's retention table directly #1539 were verified patch-equivalent by the reviewer (range-diff).
• Live replay --save-script repair evidence at exact head 11946b1e5 — see the evidence comment below: full arm → diverge (held) → corrective → --from resume → commit loop on a real emulator, healed .ad with target-v1 annotation and heal-complete sentinel.

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	1.92 MB	1.92 MB	+668 B
JS gzip	615.5 kB	615.7 kB	+278 B
npm tarball	733.5 kB	733.8 kB	+266 B
npm unpacked	2.57 MB	2.57 MB	+668 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	28.3 ms	26.8 ms	-1.5 ms
CLI --help	55.9 ms	56.5 ms	+0.6 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/session.js	+668 B	+278 B

[thymikee]

Blocking P4b ownership gap at exact head 8405c7548: runReplayScriptFile creates the request-scoped coordinator once (session-replay-runtime.ts:224-226), but both divergence paths bypass it. session-replay-target-verification.ts:159-166 and the action-failure chain through session-replay-runtime-failure.ts / session-replay-divergence.ts call buildAndPersistReplayDivergenceResume, whose implementation in session-replay-resume.ts:20-36 creates a second coordinator from SessionStore + sessionName. One locked replay request therefore has multiple coordinator instances, and a lower handler can still manufacture repair authority by naming the session—the opposite of P4b’s single locked gateway.

Please thread the existing request coordinator, or a narrower bound resume-stamp capability, through both divergence paths. Add a focused structural/counterfactual test that fails if either path can construct a coordinator or directly receive SessionStore + sessionName; the unchanged behavior tests cannot prove singleton ownership.

Separate blockers/evidence: this stack legitimately depends on open #1532, so retarget/rebase onto latest main after P4a merges. iOS smoke failed in unrelated alert dismiss with an XCTest main-thread timeout while Android/Linux/macOS and all static/integration/coverage/Fallow checks passed; rerun iOS after the code fix. No ready-for-human until the architecture finding, stack dependency, and red check are resolved.

[thymikee]

Fixed: the divergence-report chain no longer manufactures a second coordinator

Confirmed the finding: buildAndPersistReplayDivergenceResume (session-replay-resume.ts) constructed a second ReplayCoordinator from a bare SessionStore + session name, reachable from both divergence paths (session-replay-target-verification.ts, and the action-failure chain session-replay-runtime-failure.ts → session-replay-divergence.ts). That let a lower handler manufacture repair authority by naming a session instead of using the request's own locked coordinator.

The capability threaded

Added ReplayResumeStamper (session-replay-coordinator.ts) — a narrow, read-only-shaped capability bound to the SAME coordinator instance runReplayScriptFile already creates:

export type ReplayResumeStamper = Readonly<{
  sessionExists(): boolean;
  stampCorrectiveWatermark(params: {
    resume: ReplayDivergenceResume;
    repairHint: ReplayRepairHint;
    failedIndex: number;
    actions: SessionAction[];
  }): void;
}>;

ReplayCoordinator now exposes it as readonly resumeStamper: ReplayResumeStamper. It's threaded from the one construction site through the existing plumbing:

• ReplayStepContext.coordinator.resumeStamper → verifyReplayActionTarget's ReplayTargetDivergenceParams.resumeStamper → TargetBindingDivergenceContext.resumeStamper (target-verification chain)
• ReplayActionExecution.stepContext.coordinator.resumeStamper → withReplayFailureDiagnostics/withReplayFailureContext's params → buildReplayFailureDivergence's params (action-failure chain)

The deleted second-construction path

buildAndPersistReplayDivergenceResume no longer takes sessionStore/sessionName — it takes resumeStamper: ReplayResumeStamper and does nothing else with session state:

export function buildAndPersistReplayDivergenceResume(params: {
  readonly failedIndex: number;
  readonly actions: SessionAction[];
  readonly planDigest: string;
  readonly repairHint: ReplayRepairHint;
  readonly resumeStamper: ReplayResumeStamper;
}): ReplayDivergenceResume {
  const resume = buildReplayDivergenceResume({ ..., sessionExists: params.resumeStamper.sessionExists() });
  params.resumeStamper.stampCorrectiveWatermark({ resume, ... });
  return resume;
}

session-replay-resume.ts now imports neither createReplayCoordinator nor SessionStore — its only coordinator-related import is a type-only ReplayResumeStamper.

Exact allowed-importer set (decided and enforced by the new test)

• createReplayCoordinator (the factory): exactly one production call site, src/daemon/handlers/session-replay-runtime.ts.
• session-replay-transaction.ts (P4a projection): forbidden for all five divergence-chain files (already true; now regression-tested).
• session-store.ts:
  • session-replay-resume.ts — zero imports of any kind (it has no legitimate remaining reason to hold SessionStore).
  • session-replay-divergence.ts, session-replay-target-verification.ts, session-replay-runtime-failure.ts — type-only (import type { SessionStore }) is allowed, since they genuinely need it to type a sessionStore parameter forwarded from their caller for unrelated capture/publication plumbing (captureDivergenceObservation, boundReplayDivergenceForSession) untouched by this fix. A value import (which is what lets code construct or statically call the store) is forbidden.
  • session-replay-runtime-failure-response.ts — doesn't reference session-store.ts at all; unaffected, kept that way.

Ownership test: src/daemon/__tests__/replay-coordinator-ownership.test.ts

Uses the same oxc-parser AST approach as scripts/layering/session-state.ts (walks src/, parses each file's ImportDeclarations, resolves relative specifiers, checks per-specifier importKind). Five assertions: single factory call site; no divergence-chain file imports the factory; none imports session-replay-transaction.ts; session-replay-resume.ts holds no session-store.ts import; the other four hold SessionStore only as a type.

Plant-proof (both reverted before commit):

1. Added a value import of createReplayCoordinator (aliased) to session-replay-resume.ts → the "exactly one call site" and "never imports the coordinator factory" tests both failed with messages naming the file and the invariant broken (createReplayCoordinator must be imported by exactly src/daemon/handlers/session-replay-runtime.ts ... Found: src/daemon/handlers/session-replay-resume.ts, src/daemon/handlers/session-replay-runtime.ts).
2. Replaced session-replay-divergence.ts's type-only SessionStore import with a value import (aliased) → the "SessionStore only as a type" test failed (src/daemon/handlers/session-replay-divergence.ts value-imports SessionStore from session-store.ts — ...).
   Both plants removed; full 5/5 green afterward.

Gate evidence

• pnpm typecheck — exit 0.
• pnpm lint (oxlint --deny-warnings) — exit 0.
• pnpm format:check — exit 0 (ran node ./node_modules/oxfmt/bin/oxfmt src scripts test once first).
• pnpm check:layering — exit 0, R7/R10 baseline unchanged (22 writer-owned fields / 28 owner claims).
• npx fallow audit --base origin/p4a/session-script-publication — ✓ No issues in 13 changed files.
• pnpm vitest run (full suite): 626 files / 5238 tests total, 617 files / 5207 tests passed, 29 failed, 2 skipped on the first pass — 28 of 29 were plain Test timed out in Xms errors spread across nine files with zero relation to this change (android-lifecycle, android-recording, android-test-suite, doctor, apps.test.ts, help-conformance-bench, daemon-entrypoint, device-claims, process-lock), consistent with resource contention (I'd briefly had a duplicate full run going). The 29th, process-lock.test.ts's "reports live lock owner details on timeout", is an assertion (not a bare timeout) so I did not wave it off as flaky — inspected it: it races a real timeoutMs: 5, pollMs: 1 clock budget, exactly the class of test that misreads under contention. Reran all 9 failing files together in isolation: 9/9 files, 138/138 tests passed, 0 failures, including that exact process-lock test. No test files were moved, so the discovered-file-count check doesn't apply.

STOP-condition check

None triggered: no wire-response change, no parallel repair state, and engines remain unable to reach the coordinator (unaffected LOGICAL_MODULE_POLICIES).

Note on the iOS smoke flake the maintainer flagged (alert dismiss XCTest main-thread timeout): unrelated to this change; not chased, CI will retrigger on this push.

Generated by Claude Code

[thymikee]

Re-reviewed exact head cb35d978. The prior P4b ownership finding is fixed: the request’s single coordinator now supplies a narrow bound ReplayResumeStamper through both divergence routes, lower handlers can no longer construct a second coordinator from SessionStore + session name, and the new structural test fails on the previous offending topology. No new code finding.

Not ready to label yet:

• Android smoke is red on an unrelated pre-replay Automation lab wait timeout (captureTruncated:true); rerun is required, and iOS smoke was still pending at review time.
• refactor(daemon): session script publication behind one capability (#1478 P4a) #1532 remains the open base dependency and must merge before retarget/rebase plus exact-head checks.
• The PR body is stale after this fix: it still describes direct coordinator stamping, says there is no new structural test/eight changed files, and cites old validation. Refresh summary/validation and include targeted live replay --save-script repair evidence or state the exact blocker as residual risk.

Code review is clean; ready-for-human remains withheld only for the failed/pending CI and sequencing/evidence blockers.

[thymikee]

Re-reviewed at 9a381ba after #1532 merged: the rebase preserved the previously reviewed P4b patch exactly (range-diff patch-equivalent; old/new PR trees identical), so the code verdict remains clean. The stack blocker is resolved and every completed check is green; iOS smoke is still running. Readiness still needs the PR body refreshed to the actual 13-file implementation (resumeStamper plus the structural ownership test) and targeted live replay --save-script repair evidence, or an explicit blocker/residual-risk statement if that evidence cannot yet be produced.

[thymikee]

Re-reviewed exact head 11946b1 after rebasing onto #1539. The three P4b commits are patch-equivalent to the previously reviewed head; the sole tree delta is #1539’s test-only retention coverage inherited from main. The single-coordinator / bound resumeStamper route remains clean, the structural regression remains valid, and all exact-head checks are green. No code finding. Readiness still needs the stale body updated to the actual implementation/current validation, plus targeted live replay --save-script repair evidence or an explicit blocker/residual-risk statement.

[thymikee]

Targeted live replay --save-script repair evidence at exact head 11946b1e5 (fresh build, isolated --state-dir, Pixel_7_CI emulator, API 36; sessions closed after):

Arm + diverge (held through the coordinator):

replay flow.ad --save-script         → REPLAY_DIVERGENCE at step 2 (click "label=\"NoSuchControl\"")
  resume: { allowed: true, from: 2, planDigest: 1c0d1748…, repairSessionHeld: true }

Corrective press into the held session, then resume:

click 'label="Home"' --session repair-live      → ok (recorded)
replay flow.ad --from 3 --plan-digest 1c0d1748… → ok, replayed: 1   (completion)
close --save-script                              → ok               (commit; idempotent — completion had auto-committed)

The healed sibling (flow.healed.ad), verbatim:

context platform=android device="Pixel 7 CI" kind=emulator theme=unknown
open "com.callstack.agentdevicelab" --relaunch
# agent-device:target-v1 {"role":"framelayout","label":"Home","ancestry":[{"role":"scrollview"}],"sibling":1,"viewportOrder":0,"scrollRegion":{"role":"scrollview"},"rect":{"x":0,"y":2126,"width":216,"height":211},"verification":"verified"}
click "role=\"framelayout\" label=\"Home\" || label=\"Home\""
wait "Gesture lab" 5000
close
# agent-device:heal-complete

Boundary-sliced (R6), the corrective press serialized with its verified target-v1 annotation, the excluded diverged step absent, the synthetic finalize close present, and the heal-complete sentinel terminal — every transition on this path (arm, held-stamp, watermark, demote-for-rerun, complete, commit) went through the request's single coordinator / bound stamper by construction, which the ownership test enforces structurally.

One observation, not a finding: the explicit close --save-script after auto-commit-at-completion reports success without savedScript in its payload (the idempotent already-committed branch carries no path). Pre-existing response shape, unchanged by this PR; noting it since BLOCKER 2a's positive-report intent arguably extends to the idempotent case.

Environment note for reproducers: run every command of the flow with an explicit shared --state-dir — the per-worktree dev default spawned a fresh daemon per invocation in this checkout (three coexisting daemons observed; reproduces on main, unrelated to this PR), which makes the held session unreachable from the next command.

---

Generated by Claude Code

[thymikee]

Re-reviewed at 11946b1: clean and merge-ready. The exact-head live evidence covers the held repair loop end-to-end: divergence produces repairSessionHeld, a corrective action is recorded to that session, --from plus the plan digest resumes and completes, and the healed sibling excludes the diverged action while retaining the target-annotated corrective click, synthetic close, and terminal heal-complete sentinel. All checks are green and merge state is clean.

#1545 reproduces on main; the explicit shared state-dir used for this run is a valid workaround, so that issue is environment debt rather than a P4b blocker. The successful post-auto-commit close response lacking savedScript is an unrelated pre-existing response-shape detail.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-01 11:51 UTC