refactor: extract daemon replay behind one application facade

[thymikee]

Summary

Extract daemon replay and replay-test application orchestration behind the named src/daemon/replay/index.ts facade.

• Adds named runReplayCommand and runReplayTestCommand entry points with a ReplaySession aggregate and narrow replay-owned capabilities.
• Moves replay/test implementation and tests under src/daemon/replay/internal, while keeping close, recording, session storage, publication, script writing, transaction, and request-binding ownership in their existing modules.
• Keeps coordinator construction in one native replay owner and adds structural checks for facade exports, handler deep imports, sibling-owner imports, engine edges, aliases, namespaces, and static/dynamic/re-export imports.
• Removes the old null-returning replay router and caller-side replay/test orchestration without changing wire responses, progress, artifacts, error ordering, repair/resume state, or session lifecycle.

Closes #2131

Dependencies and base

• Rebased onto current main at cfed1ae56745a81ef659d8adcebd67ea8a015ad1 (including the latest merged default-branch changes). The only rebase conflict was in src/daemon/replay/internal/session-replay-action-runtime.ts; it was resolved by preserving the current-main tenant-aware resolveSessionScope import and replayed-open resolvedSessionScope propagation.
• Prerequisite refactor: dissolve caller-side src/replay into command and CLI owners #2127: merged PR refactor: dissolve caller-side src/replay into command and CLI owners #2151 at caa3dc23f9f3f6a829e91e8c5f00a1e4a3084f53.
• Prerequisite test: characterize replay application behavior and coordinator ownership #2130: merged PR test: characterize replay application behavior and coordinator ownership #2137 at dd988847c8f5a5a21dc9315e829d228c025cfc4d.
• This is the terminal PR for refactor: extract daemon replay behind one application facade #2131. No merge was performed.

Validation

• pnpm check:quick — pass.
• pnpm check:affected --run — all non-Vitest selected checks passed: format, lint, typecheck, layering, DI seams, fallow, MCP metadata, build, package verification, Node integration, and macOS coverage. The Vitest-related lane completed 536/537 files and 4,531/4,532 tests; the single failure was the 5-second iOS Settings provider timeout under host load. The replay, repair, resume, routing, and structural tests passed. An independent behavior review compared the timeout on the PR and its base and found no regression; no production timeout or behavior change was made.
• Focused replay/repair/resume/routing/observation suite — 46 files, 376 tests passed.
• Merged replay application behavior contract — passed.
• Coordinator structural gate — 9 tests passed. The multiline dynamic-import and re-export ownership regressions were observed red against the old scanner and green after the fixes.
• Layering — 174 tests plus the global guard passed (1538 source files, zero forbidden logical-module imports).
• Dynamic-import parser model tests — 23 tests passed.
• Fallow — no issues in 121 changed files.
• Test-size ratchet plus divergence regression — 17 tests passed; the moved divergence test was reduced to its 1,100-line pin.
• Independent adversarial behavior review of final diff — no valid findings.
• Independent adversarial architecture review — the initial dynamic-import ownership gap was fixed and re-tested; final review found no remaining actionable ownership finding.

Touched files and size

• src/daemon/replay/index.ts, src/daemon/replay/internal/**, and src/daemon/replay/**/__tests__/**: named facade, private implementation, and relocated replay/test coverage.
• src/daemon/handlers/session-command-input.ts, src/daemon/handlers/session-replay-command.ts, src/daemon/handlers/session-replay-video-owner.ts, and src/daemon/handlers/session.ts: handler composition, command types, and existing owner adapters.
• src/daemon/session-replay-coordinator.ts, src/daemon/internal-observation.ts, src/daemon/handlers/session-runtime.ts, and src/daemon/session-action-recorder.ts: named capability and moved-path seams.
• scripts/layering/**, src/daemon/__tests__/replay-coordinator-ownership.test.ts, fallow-baselines/health.json, oxlint.config.ts, and scripts/__tests__/test-file-size-ratchet.test.ts: structural enforcement, import scanning, and baselines.
• packages/ad-replay/**, packages/ad-script/**, packages/contracts/**, and packages/replay-test/**: current replay module paths and contract/test imports.
• Remaining touched paths are replay/test imports and fixtures updated for their new ownership roots; the complete rename-aware path list is represented by the 121-file diff.

Rename-aware diff against origin/main...HEAD: 121 files, gross +2217/-1593. For production src/** excluding __tests__ and *.test.ts, changed-line accounting is +1515/-1337, a move-adjusted net +178 lines.

The net production growth is deliberate and independently adversarially reviewed: it is the cost of the named facade, the session command-input seam, bound read/mutation/observation capabilities, and the existing-owner adapters needed to keep replay internals from acquiring close/record authority. The extraction removes the null router, duplicated session-handler decomposition, and scheduler-side owner plumbing; further shrinking by passing the broad SessionCommandInput/SessionStore or by moving close/record implementations into replay would violate the issue's least-authority and ownership constraints. No parallel mutable state or compatibility shim was added.

Residual risks

• Native, physical-device, Apple/Android, provider, full-coverage, and live-network evidence remains GitHub-authoritative and was not run locally.
• The pre-rebase Android Smoke failure was an alert-wait timeout on the old head; current-main Android smoke evidence passed, and no replay-related code was changed for it.
• Exact-head GitHub CI remains authoritative; the rebased head is 66e02f04bb4c0e98ed052e61c2f09735e834f81d.

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	2.54 MB	2.54 MB	+700 B
JS gzip	855.1 kB	855.5 kB	+423 B
npm tarball	979.6 kB	980.0 kB	+404 B
npm unpacked	3.38 MB	3.38 MB	+700 B

npm unpacked components

Component	Base	Current	Diff
JS / dist source	2.70 MB	2.70 MB	+700 B
Apple runner source/project	581.2 kB	581.2 kB	0 B
macOS helper source	54.8 kB	54.8 kB	0 B
Android helper artifacts	0 B	0 B	0 B
Other package files	45.7 kB	45.7 kB	0 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	33.2 ms	32.2 ms	-1.0 ms
CLI --help	86.7 ms	84.8 ms	-1.9 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/session2.js	+700 B	+423 B

Top changed packed files

Packed file	Base	Current	Diff
dist/src/session2.js	217.4 kB	218.1 kB	+700 B

[thymikee]

Code-quality review

I read the whole diff against a79f0d2e81 with rename detection, then re-derived every claim in the PR body from the tree rather than from the summary. I found no behavior regression. The replay arm threads the same invokeReplayAction ?? invoke; the test arm keeps the platformResourceCleanup → recordVideo-without-runtime error ordering, the same cleanupSession capability set, and the same ['start', videoPath] / ['stop'] record requests. The null-returning router was dead under a descriptor-keyed dispatch table and is correctly gone.

What follows is about the enforcement the PR adds, not about what it broke. Six findings; none blocking, but 1–4 are worth fixing before merge because they are what pins the extraction in place.

---

1. The store-authority gate is now vacuous, and its rationale comment is false

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

test('the remaining divergence-report chain holds SessionStore only as a type', () => {
  for (const file of DIVERGENCE_CHAIN_FILES) {
    if (!SESSION_STORE_TYPE_ONLY_ALLOWED.has(file)) continue;
    for (const site of collectImportSites(file)) {
      if (site.target !== SESSION_STORE_MODULE) continue;
      ...

After this PR, none of the five DIVERGENCE_CHAIN_FILES imports session-store.ts at all — the switch to ReplaySessionStore removed the last one:

$ for f in session-replay-{divergence,target-verification,runtime-failure,resume,runtime-failure-response}.ts; do
    grep -c session-store src/daemon/replay/internal/$f; done
0 0 0 0 0

So the inner continue fires on every site and the test body executes zero assertions. SESSION_STORE_TYPE_ONLY_ALLOWED (line 45) is dead data.

The header comment is now wrong too:

A type-only reference to SessionStore … is allowed for files with an unrelated, legitimate reason to type one — session-replay-resume.ts has none post-fix, so it is held to a stricter "no session-store.ts import at all" bar than the other four.

There is no longer any distinction: all five have none. This PR earned the stronger ratchet and then left the weaker one in place, so the state it achieved is unpinned and can silently regrow.

Fix: delete SESSION_STORE_TYPE_ONLY_ALLOWED, fold session-replay-resume.ts holds no SessionStore binding at all into one test over all five files, and rewrite the header to say the bar is now uniform. Net: less code, a real gate instead of a no-op one.

---

2. The least-authority claim — the stated justification for the +68 production lines — is not enforced

The PR body argues the net growth buys "keeping replay internals from acquiring close/record authority", and the new policy encodes exactly two forbidden targets (architecture-ownership.ts:33):

forbiddenTargetRoots: [
  'src/daemon/handlers/session-close.ts',
  'src/daemon/handlers/record-runtime.ts',
],

But the authority that matters is SessionStore itself — recordAction, delete, setRuntimeHints, scriptWriter — and it is not on the list. It is also already imported inside the tree:

src/daemon/replay/internal/test-command.ts:8

import { SessionStore } from '../../session-store.ts';   // value import

…used once, at line 317, for SessionStore.expandHome(artifactsDir, cwd) — a pure static that just delegates to expandSessionPath from session-paths.ts, which a sibling in the same tree (session-replay-runtime-session.ts) already imports directly.

So today the ReplaySessionStore narrowing is a convention, not a boundary: one import { SessionStore } restores full authority and no gate objects.

Fix (two lines):

1. test-command.ts → import { expandSessionPath } from '../../session-paths.ts' and call it directly; drop the SessionStore import.
2. Add 'src/daemon/session-store.ts' to daemon-replay's forbiddenTargetRoots.

That converts the PR's central argument from prose into a gate, which is the whole point of the ownership scripts.

---

3. New type-import cycle session.ts ↔ session-replay-command.ts — the only handler back-edge in the tree

src/daemon/handlers/session.ts:15 imports ./session-replay-command.ts, and src/daemon/handlers/session-replay-command.ts:10 imports back:

import type { SessionCommandHandler } from './session.ts';

session-replay-command.ts is the only file in src/daemon/handlers/ that imports from session.ts. Every other handler module (session-close.ts, record-runtime.ts, …) declares its own parameter shape and lets session.ts adapt; the previous inline handleSessionReplayCommandGroup needed no such edge because it lived inside session.ts.

R9's own failure message names this exact anti-pattern:

A type-only import that closes a loop makes every file in the loop unreadable in isolation. Declare the shared type below both modules.

It passes CI only because R9 measures the largest cycle (baseline 6, provider-webdriver) and a 2-node cycle does not beat it. Passing the ratchet is not the same as satisfying the rule the ratchet approximates — and this is a refactor whose stated purpose is untangling ownership.

Fix: move SessionCommandInput / SessionCommandParams / SessionCommandHandler (all three currently at session.ts:37, :67, :71) into src/daemon/handlers/session-command-input.ts. Mechanical, and it drops the cycle to zero.

---

4. The deleted router test guarded exactly the seam this PR replaced with hand-narrowing

src/daemon/handlers/__tests__/session-replay.test.ts is removed. One of its three cases said, verbatim:

// The suite owns video recording, sharding and device binding, so it receives `params` whole —
// narrowing here is what would silently drop those capabilities.
expect(mockRunReplayTestSuiteCommand).toHaveBeenCalledWith(params);

This PR introduces that narrowing: handleReplayTestCommand destructures 14 fields and hand-threads six of them into createReplayTestVideoOwner, which returns undefined if any one is missing. Drop one in a future edit and --record-video degrades silently to COMMAND_FAILED: Screen-recording runtime is not configured for replay video capture — a green suite, a wrong product.

Nothing replaces the guard. session-test-suite-command-video.test.ts used to reach this path through handleSessionReplayCommands; it now constructs createReplayTestVideoOwner itself, so the handler's threading is untested. Test-name accounting across src/: −3, +0 behavioral tests (application.test.ts is an 85%-similarity rename of the pre-existing session-replay-application.test.ts, not new coverage). The two additions are structural tests in replay-coordinator-ownership.test.ts.

Fix: one test on handleReplayTestCommand asserting that a fully-populated SessionCommandParams yields a defined video. Cheapest honest version: spy on createReplayTestVideoOwner, or assert that --record-video with all six capabilities present does not return the "not configured" error.

---

5. ReplayRecordVideoRequest makes an invalid state representable

command-types.ts:26-33 types phase: 'start' | 'stop' alongside an optional outputPath?: string, so "start with no path" type-checks. The adapter then papers over it:

session-replay-video-owner.ts:66

positionals: request.phase === 'start' ? ['start', request.outputPath ?? ''] : ['stop'],

'' is not the same as absent. resolveRecordingOutputPaths (src/recording/output-path.ts:32) branches on requestedPath === undefined, so '' skips the defaultRecordingPath fallback and goes to resolveUserPath('', { cwd }) — i.e. the recording is aimed at the cwd directory, failing at write time instead of at the call. Before the refactor, the start path was a required string and this shape did not exist.

Unreachable today (startReplayTestVideoRecordingIfReady always computes videoPath), so this is prevention, not a bug report — but it is the "make the defect unrepresentable" case, and the union is free:

export type ReplayRecordVideoRequest = Readonly<
  { request: ReplayRequestContext; sessionName: string } & (
    | { phase: 'start'; outputPath: string }
    | { phase: 'stop' }
  )
>;

Then ?? '' deletes itself.

---

6. Cross-package pointers to the moved files now dangle

The moved files' own headers were updated carefully, but the inbound references from other packages were not. Five production files still name src/daemon/handlers/session-replay-*.ts:

pointer	in
session-replay-repair-hint.ts	packages/contracts/src/replay-divergence.ts:221
session-replay-target-classification.ts	packages/ad-script/src/internal/target-annotation-identity.ts:6,202, …/target-annotation-classification.ts:13
session-replay-target-verification.ts	packages/ad-replay/src/internal/target-verification.ts:3, …/target-annotation-identity.ts:53

Plus three dangling symbol references to runReplayScriptSource in packages/ad-replay/src/internal/{inspect,step-loop}.ts and its test. In a repo that leans this hard on comments as navigation, a pointer into a path that no longer exists costs a reader (or an agent) a full-tree search. docs/adr/** is a historical record and can stay as-is; the packages/** ones should follow the move.

---

Nits

• Test names now describe the fixture, not the behavior. A bulk rename turned 40 tests from runReplayScriptSource … into runReplayForTest … — naming the local test helper rather than the production entry point. They should read runReplayCommand …; sed -i '' 's/runReplayForTest /runReplayCommand /' over replay/internal/__tests__/.
• index.ts carries no header. It is the artifact the whole PR exists to create, and it is 8 lines of re-exports with no statement of what the module owns or what stays outside it. Every module it re-exports has one.
• Two of the six pinned façade exports have no production consumer. ReplayCommand and ReplayTestCommand are used only by replay/__tests__/, which sits inside the module and can import ./internal/command-types.ts directly. Pinning them widens the façade for test convenience.
• daemon-replay is declared twice — LOGICAL_MODULE_POLICIES (root src/daemon/replay/) and DAEMON_REPLAY_FACADE (root src/daemon/replay/index.ts) — with nothing tying them together. A facade field on the policy would make the module and its façade undriftable.
• command: params at session-replay-maestro-runtime.ts:136,144 hands the error/success builders an object that also carries bundle, startedAt and the mutable state in a slot typed ReplayCommand. Structurally fine, but it un-narrows the type the parameter exists to narrow.

---

Summary: the move itself is clean and I could not fault its behavior. The gap is that the enforcement shipped alongside it is weaker than the PR body claims — one gate asserts nothing (1), the central least-authority argument has no gate at all and is already breached once inside the tree (2), a new cycle slips under a ratchet that measures only the largest one (3), and the one test that guarded the capability threading was deleted in the same change that introduced the threading (4). Items 1, 2 and 5 are each a handful of lines.

[thymikee]

One actionable finding blocks readiness at 93b80128771f36d6e56b6bd9a80ac39364b2b712:

scanDynamicImports() scans one source line at a time. A valid multiline dynamic import such as import( '../replay/internal/native-command.ts' ) therefore produces no edge and bypasses the new handler/engine/internal-tree policy. Existing regression coverage exercises static imports, and the dynamic-import parser test is one-line, so the enforcement proof does not cover this syntax. Parse dynamic imports through the existing OXC AST path (or otherwise support multiline syntax) and plant this exact bypass as a red test.

The replay facade/least-authority extraction and move-adjusted +68 production-line rationale otherwise look coherent; no smaller design finding. Android Smoke currently failed in an unrelated alert-dismiss scenario and should be rerun on this exact head, but it is separate from the confirmed code finding.

No ready-for-human label until the guard is fixed and exact-head evidence changes.

[thymikee]

Addressed in 25c08c4194 and c5ed604c8b, and pushed after rebasing onto 70fb783729 (latest main). The rebase had one conflict in src/daemon/handlers/session.ts; it was resolved while retaining current-main ProviderAppCatalog forwarding.

• Removed the vacuous SESSION_STORE_TYPE_ONLY_ALLOWED exception and folded resume coverage into one uniform scan of every divergence-chain file. Replay production code now has no session-store.ts import.
• Replaced the test command's SessionStore.expandHome dependency with the session-path owner, and added session-store.ts to the replay forbidden roots. The structural test has a planted-red/observed-green check for static, dynamic, and re-export imports.
• Extracted SessionCommandInput, SessionCommandParams, and SessionCommandHandler into session-command-input.ts, removing the handler/replay command type-import cycle without widening replay authority.
• Added a handler-level regression with the complete video input shape; missing video source now stays a typed INVALID_ARGS response rather than reaching the recording runtime error.
• Made ReplayRecordVideoRequest a discriminated start/stop union so only start carries outputPath, and updated the existing video owner and recording builder.
• Updated stale cross-package paths, current replay command names, test paths, and titles; the public replay facade remains limited to the named command/session/video-owner exports.

Validation after the rebase: focused replay/repair/resume tests passed (54 files, 474 tests), the merged application contract passed, the coordinator structural gate passed (7 tests), layering passed (172 tests plus guard), fallow passed for 117 changed files, and the affected non-Vitest checks passed. The affected Vitest lane had only the existing 5-second iOS lifecycle/gesture-admission timeout pair under host load; gesture passed when isolated, and the independent behavior review found no PR regression when comparing PR/base. No wire, progress, artifact, error-ordering, repair/resume, lifecycle, or single-coordinator behavior changed.

[thymikee]

Reviewed exact head c5ed604c8b689ed178b3a079ca4626dd2af3c86d. The refactor is code-clean against its declared base and preserves the daemon-registry/replay ownership boundaries, but it is not ready because GitHub reports CONFLICTING / DIRTY against current main. The conflict in src/daemon/replay/internal/session-replay-action-runtime.ts is semantic: current main replaced resolveImplicitSessionScope with tenant-aware resolveSessionScope, while the moved PR file retains the old implicit-only resolver. Resolve the rebase by preserving resolveSessionScope at the moved import and replayed open scope propagation; otherwise tenant scope is lost. Then rerun exact-new-head CI, especially replay repair/resume and session-routing coverage. No ready-for-human label until the conflict is resolved and the new head is green.

[thymikee]

Rebased and pushed the PR onto origin/main at cfed1ae56745a81ef659d8adcebd67ea8a015ad1.

The reported conflict in src/daemon/replay/internal/session-replay-action-runtime.ts is resolved by retaining current-main's tenant-aware resolveSessionScope import and the replayed-open resolvedSessionScope propagation. The new exact head is 66e02f04bb4c0e98ed052e61c2f09735e834f81d.

I also refreshed the PR description with the final review fixes and measurements: bound read/mutation/observation capabilities instead of exposing keyed session operations, no replay imports of the concrete SessionStore, OXC-backed multiline/template/unresolved dynamic-import and re-export enforcement, the narrow public facade, and the divergence test-size reduction.

Validation on this exact head:

• Focused replay/repair/resume/routing/observation: 46 files, 376 tests passed.
• Coordinator ownership: 9 tests; dynamic-import and re-export planted-red checks were observed red before the fixes and green afterward.
• Layering: 174 tests plus the global guard passed; fallow reported no issues in 121 changed files.
• pnpm check:affected --run: all non-Vitest selected checks passed; Vitest completed 536/537 files and 4,531/4,532 tests, with only the known 5-second iOS Settings provider timeout under host load. The timeout reproduced as non-regressing in the independent PR/base behavior review, so no timeout or production behavior change was made.

The PR remains unmerged; GitHub exact-head checks are now running for the new commit.

[thymikee]

Reviewed exact head 66e02f0. The semantic rebase conflict is correctly resolved: replayed open actions retain current main's tenant-aware resolveSessionScope. The replay facade remains narrow and session-bound, with no concrete SessionStore authority or second coordinator construction exposed. Code review is clean and all completed exact-head checks are green; iOS Smoke Tests is the only remaining in-progress job. Ready for human review now, and merge-ready once that exact-head iOS job passes.

[github-actions]

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