test: characterize replay-test reporter contract and extend R10 - #1505
Merged
Conversation
P3 of #1478 moves the replay-test scheduler into `packages/replay-test` and makes attempt identity scheduler-owned. Before production code moves, pin what a shipped custom reporter actually observes today, and close the import boundary the extraction has to end up satisfying. Reporter values (all pinned as shipped, none proposed): - the `RequestProgressEvent` -> reporter-value projection field by field, including key presence for absent optionals and the dropped `command` events; - `session` provenance across the seam: the start value is always `attempt-1` (it is built before any attempt runs), step values track the running attempt through the per-attempt AsyncLocalStorage context, and result values carry the attempt that produced them, so a retried case reports three different sessions to one reporter; - the shard-scoped session prefix and device identity a sharded run reports; - module export spelling precedence, the six optional hook names, the live-hook vs final-hook error asymmetry, and exit-code recommendation semantics. Late-timeout finalization/cleanup: - finalization always runs before cleanup, and the timing trace records that order (`finalize_start/stop` then `cleanup_start/stop`); - a replay settling inside the 2s grace window cleans up once and is not marked `timeout_cleanup_pending`, and the raced TIMEOUT response still wins; - a replay that misses the window defers its second cleanup until the abandoned replay settles, and that late cleanup's failure is swallowed. R10 now rejects replay-test imports from `src/request/**` and engine internals (`src/replay/`, `src/compat/`, `src/maestro/`, `src/ad-replay/`), with the four imports that exist today recorded as shrink-only migration entries so the rule enforces immediately and the extraction must delete them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
The failing-suite characterization supplied `hint` as input but never asserted it on the hook value, so dropping `hint: error.hint` from the scheduler path left all reporter tests green. Assert it on the real reporter value so the ratchet catches a shipped field going missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8
Member
Author
|
Fixed in Added with On the 365-line test file: agreed it's over the 300-LOC target, and I've left it as-is for now. My reasoning is that the natural split is by concern (values / session provenance / timeout-cleanup), and P3 will move these into Generated by Claude Code |
thymikee
added
the
ready-for-human
Member
Author
|
Reviewed Residual risk: no separately authorized cross-vendor review was performed. |
|
This was referenced Jul 30, 2026
thymikee
pushed a commit
that referenced
this pull request
Jul 30, 2026
…tep port Review finding on #1509: the native `.ad` reporter ratchet exercises only one of the two `onStep` forwarding chains, so deleting a link in the Maestro chain would silently stop `onTestStep` for every `test --maestro` run while every existing reporter test stayed green. Same defect class as the dropped diagnosticId/logPath (#1501) and the dropped reporter `hint` (#1505). Maestro is one of P3's two required real adapters and its chain shares no links with the native one below `runReplayScriptFile`: scheduler sink -> runReplayScriptFile -> runTypedMaestroReplayFile -> createMaestroReplayObserver({ onStep }) -> actionStarted -> onStep Adds a Maestro scenario driving `test --maestro` through the real session handler and the real reporter registry. It asserts the step payload the engine produces (`stepIndex`/`stepTotal`/`stepCommand`/`stepValue`, including that a value-less command stays value-less) together with the attempt/session identity the scheduler supplies, since that half of the event came from request-global AsyncLocalStorage before P3. A second case drives a retry so step events must carry attempt-1's session and then attempt-2's. The flow `name` also pins the reporter `title`, a value only the Maestro path can produce. New file rather than an addition to session-test-reporter-values.test.ts: that file is the pinned characterization and must keep passing unmodified, and Maestro needs its own vi.mock of core/dispatch for device resolution. Counterfactual run, both links, each restored after: - dropping `onStep` from createMaestroReplayObserver in session-replay-maestro-runtime.ts - dropping the emitMaestroStep call from actionStarted in session-replay-maestro-observer.ts Each dropped both onTestStep events ("expected [ 'onSuiteStart', 'onTestStart', …(2) ] to deeply equal [ 'onSuiteStart', 'onTestStart', …(4) ]") and failed both new cases, while session-test-reporter-values.test.ts passed all 4 — exactly the hole the reviewer identified. Test-only; no production change. Bundle output is byte-identical to b339c64. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8
thymikee
added a commit
that referenced
this pull request
Jul 31, 2026
…eam (#1478 P3, part 1) (#1509) * refactor(replay-test): neutralize the values crossing the scheduler seam #1478 P3, part 1 of 2. Prepares the replay-test extraction by removing every non-neutral value that crosses the scheduler seam, in place under `src/`, so the physical move to `packages/replay-test` is a file move rather than a redesign. `DaemonResponse` no longer crosses the seam. `session-test-types.ts` typed `runReplay`/`finalizeAttempt` as returning a daemon response and the scheduler read `.error.code`, `.error.details`, and `.data.replayed/.healed/.warnings/ .snapshotDiagnostics` off it throughout. That is invisible to R10 today only because `checkDaemonTypesImporters` skips `src/daemon/`; once the files live in a package they become external `daemon/types.ts` importers, which the ratchet only lets shrink. Attempts now resolve as tagged `ReplayTestAttemptOutcome` values carrying exactly what the scheduler consumes, including an `infrastructure` tag — classifying an environmental failure needs platform boot-diagnostic vocabulary the scheduler must not import, so the host decides and the scheduler reads the verdict. `session-test-outcome.ts` is the one place a daemon response becomes an outcome. Step events get a narrow per-attempt port. They were emitted from `session-replay-runtime.ts` and `session-replay-maestro-observer.ts`, both reading a request-global `AsyncLocalStorage` seeded per attempt. The scheduler now hands each attempt an `onStep` sink, threaded the way `tracePath` already is; both engines call it and `withReplayTestActionProgress`/`readReplayTestActionProgress` are gone. A direct `replay` simply has no sink. ADR 0012 divergence becomes a neutral leaf. `src/replay/divergence.ts` depended only on kernel contracts and redaction, yet Maestro constructs divergences too and CLI/MCP both render them, so P5 could not have moved it into `packages/ad-replay`. It is now `@agent-device/contracts/divergence`; the renderer's output text is unchanged. The progress wire vocabulary moves to `@agent-device/contracts/progress`. It is serialized by `request-progress-protocol.ts` and reconstructed by the CLI reporter path, so it belongs below both; `src/request/progress.ts` keeps only the sink and its AsyncLocalStorage binding. Together these clear all four of replay-test's recorded R10 migration imports, so the rule now enforces unconditionally for that module. Behavior is unchanged. The shipped reporter contract — export spellings, object/factory loading, hook names, timing/order, value fields, the synchronous live-hook rule, awaited suite completion, error handling, exit codes — is untouched, and `session-test-reporter-values.test.ts` passes unmodified. The `--shard-all` `total`/`runnable` asymmetry is preserved as characterized. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * test(replay-test): pin the Maestro reporter step path against the onStep port Review finding on #1509: the native `.ad` reporter ratchet exercises only one of the two `onStep` forwarding chains, so deleting a link in the Maestro chain would silently stop `onTestStep` for every `test --maestro` run while every existing reporter test stayed green. Same defect class as the dropped diagnosticId/logPath (#1501) and the dropped reporter `hint` (#1505). Maestro is one of P3's two required real adapters and its chain shares no links with the native one below `runReplayScriptFile`: scheduler sink -> runReplayScriptFile -> runTypedMaestroReplayFile -> createMaestroReplayObserver({ onStep }) -> actionStarted -> onStep Adds a Maestro scenario driving `test --maestro` through the real session handler and the real reporter registry. It asserts the step payload the engine produces (`stepIndex`/`stepTotal`/`stepCommand`/`stepValue`, including that a value-less command stays value-less) together with the attempt/session identity the scheduler supplies, since that half of the event came from request-global AsyncLocalStorage before P3. A second case drives a retry so step events must carry attempt-1's session and then attempt-2's. The flow `name` also pins the reporter `title`, a value only the Maestro path can produce. New file rather than an addition to session-test-reporter-values.test.ts: that file is the pinned characterization and must keep passing unmodified, and Maestro needs its own vi.mock of core/dispatch for device resolution. Counterfactual run, both links, each restored after: - dropping `onStep` from createMaestroReplayObserver in session-replay-maestro-runtime.ts - dropping the emitMaestroStep call from actionStarted in session-replay-maestro-observer.ts Each dropped both onTestStep events ("expected [ 'onSuiteStart', 'onTestStart', …(2) ] to deeply equal [ 'onSuiteStart', 'onTestStart', …(4) ]") and failed both new cases, while session-test-reporter-values.test.ts passed all 4 — exactly the hole the reviewer identified. Test-only; no production change. Bundle output is byte-identical to b339c64. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 --------- Co-authored-by: Claude <noreply@anthropic.com>
thymikee
added a commit
that referenced
this pull request
Jul 31, 2026
`emitReplayTestRetryProgress` publishes `hint` on every retried failure (session-test-attempt.ts), and nothing asserted it. The existing hint coverage coming out of #1505 pins only the final-attempt emit site, so deleting the retry line left the entire suite green. The retry fixture's first-attempt error carried no hint, so no assertion could have caught it even in principle. It now carries one, and the retry result value asserts it. Counterfactual, run rather than assumed — with `hint: attempt.outcome.error.hint` deleted from the retry emit site: ✓ a reporter sees the shipped suite-start, skip, and test-start values × reporter step and result sessions track the running attempt, not the start value ✓ a failing suite reaches the reporter with the failure message, hint fields, and exit code ✓ sharded runs give the reporter shard-scoped sessions and device identity - "hint": "retry hint from the failed attempt", + "hint": undefined, Only the new pin fails; the final-path hint test keeps passing, which is the gap. Production code is unchanged — `git diff` on session-test-attempt.ts is empty. This lands before P3b relocates session-test-attempt.ts into packages/replay-test, so the move cannot drop the field silently. Refs #1478 Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 Co-authored-by: Claude <noreply@anthropic.com>
thymikee
pushed a commit
that referenced
this pull request
Jul 31, 2026
…request global The scheduler called `emitRequestProgress` in eight places, which reads a sink out of a request-global `AsyncLocalStorage`. That is ambient authority a format-neutral scheduler cannot hold once it lives in `packages/replay-test`, and #1505 recorded it as a shrink-only R10 entry. The host now injects the capability through the existing `ReplayTestRuntimeDependencies` seam established in P3a, so no new seam is invented. `session-replay.ts` supplies `emitProgress: emitRequestProgress`; `src/request/progress.ts` keeps the sink and its AsyncLocalStorage binding for every other caller. The port is deliberately narrower than `RequestProgressSink`: it accepts only `ReplayTestSuiteProgressEvent | ReplayTestProgressEvent`, so the scheduler is not handed the ability to emit `CommandProgressEvent`. Authority narrows again one hop down: `runReplayTestAttempt` spread the whole dependency bag but uses three of its members and never publishes progress, so it now takes `Pick<..., 'runReplay' | 'cleanupSession' | 'finalizeAttempt'>`. That is why no runtime test fixture needed changing — the attempt runtime never gained the capability in the first place. Also drops the last two `replay/script.ts` type references from `session-test-runtime.ts`, so the engine import is gone from that file too. Reporter contract preserved: `session-test-reporter-values.test.ts` and `session-test-reporter-values-maestro.test.ts` both pass unmodified (27 tests green across the five scheduler suites). Typecheck clean. Remaining scheduler boundary for P3b: `request/cancel.ts`, `replay/format.ts`, `replay/script.ts` in discovery, `session-store.ts`, `daemon/types.ts`, `replay-source-discovery.ts`, `core/dispatch*`, `utils/diagnostics.ts`. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8
thymikee
pushed a commit
that referenced
this pull request
Jul 31, 2026
…request global The scheduler called `emitRequestProgress` in eight places, which reads a sink out of a request-global `AsyncLocalStorage`. That is ambient authority a format-neutral scheduler cannot hold once it lives in `packages/replay-test`, and #1505 recorded it as a shrink-only R10 entry. The host now injects the capability through the existing `ReplayTestRuntimeDependencies` seam established in P3a, so no new seam is invented. `session-replay.ts` supplies `emitProgress: emitRequestProgress`; `src/request/progress.ts` keeps the sink and its AsyncLocalStorage binding for every other caller. The port is deliberately narrower than `RequestProgressSink`: it accepts only `ReplayTestSuiteProgressEvent | ReplayTestProgressEvent`, so the scheduler is not handed the ability to emit `CommandProgressEvent`. Authority narrows again one hop down: `runReplayTestAttempt` spread the whole dependency bag but uses three of its members and never publishes progress, so it now takes `Pick<..., 'runReplay' | 'cleanupSession' | 'finalizeAttempt'>`. That is why no runtime test fixture needed changing — the attempt runtime never gained the capability in the first place. Also drops the last two `replay/script.ts` type references from `session-test-runtime.ts`, so the engine import is gone from that file too. Reporter contract preserved: `session-test-reporter-values.test.ts` and `session-test-reporter-values-maestro.test.ts` both pass unmodified (27 tests green across the five scheduler suites). Typecheck clean. Remaining scheduler boundary for P3b: `request/cancel.ts`, `replay/format.ts`, `replay/script.ts` in discovery, `session-store.ts`, `daemon/types.ts`, `replay-source-discovery.ts`, `core/dispatch*`, `utils/diagnostics.ts`. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8
thymikee
pushed a commit
that referenced
this pull request
Jul 31, 2026
…request global The scheduler called `emitRequestProgress` in eight places, which reads a sink out of a request-global `AsyncLocalStorage`. That is ambient authority a format-neutral scheduler cannot hold once it lives in `packages/replay-test`, and #1505 recorded it as a shrink-only R10 entry. The host now injects the capability through the existing `ReplayTestRuntimeDependencies` seam established in P3a, so no new seam is invented. `session-replay.ts` supplies `emitProgress: emitRequestProgress`; `src/request/progress.ts` keeps the sink and its AsyncLocalStorage binding for every other caller. The port is deliberately narrower than `RequestProgressSink`: it accepts only `ReplayTestSuiteProgressEvent | ReplayTestProgressEvent`, so the scheduler is not handed the ability to emit `CommandProgressEvent`. Authority narrows again one hop down: `runReplayTestAttempt` spread the whole dependency bag but uses three of its members and never publishes progress, so it now takes `Pick<..., 'runReplay' | 'cleanupSession' | 'finalizeAttempt'>`. That is why no runtime test fixture needed changing — the attempt runtime never gained the capability in the first place. Also drops the last two `replay/script.ts` type references from `session-test-runtime.ts`, so the engine import is gone from that file too. Reporter contract preserved: `session-test-reporter-values.test.ts` and `session-test-reporter-values-maestro.test.ts` both pass unmodified (27 tests green across the five scheduler suites). Typecheck clean. Remaining scheduler boundary for P3b: `request/cancel.ts`, `replay/format.ts`, `replay/script.ts` in discovery, `session-store.ts`, `daemon/types.ts`, `replay-source-discovery.ts`, `core/dispatch*`, `utils/diagnostics.ts`. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8
thymikee
added a commit
that referenced
this pull request
Jul 31, 2026
* refactor(replay-test): source the manifest device vocabulary from the kernel `session-test-types.ts` reached `ReplayScriptMetadata['platform']` and `['target']` through `replay/script.ts` — the native `.ad` engine. A format-neutral scheduler must not name an engine module, and P5 relocates that engine into `packages/ad-replay` regardless, so the import had to go before the scheduler can move. Both members already resolve to neutral kernel types (`Exclude<PlatformSelector, 'web'>` and `DeviceTarget` from `@agent-device/kernel/device`), so this re-sources them directly and the manifest shape is unchanged. Only the import direction differs. First increment of P3b; the scheduler still has request-global, engine and daemon imports to port before the physical move. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): inject the progress sink instead of reading a request global The scheduler called `emitRequestProgress` in eight places, which reads a sink out of a request-global `AsyncLocalStorage`. That is ambient authority a format-neutral scheduler cannot hold once it lives in `packages/replay-test`, and #1505 recorded it as a shrink-only R10 entry. The host now injects the capability through the existing `ReplayTestRuntimeDependencies` seam established in P3a, so no new seam is invented. `session-replay.ts` supplies `emitProgress: emitRequestProgress`; `src/request/progress.ts` keeps the sink and its AsyncLocalStorage binding for every other caller. The port is deliberately narrower than `RequestProgressSink`: it accepts only `ReplayTestSuiteProgressEvent | ReplayTestProgressEvent`, so the scheduler is not handed the ability to emit `CommandProgressEvent`. Authority narrows again one hop down: `runReplayTestAttempt` spread the whole dependency bag but uses three of its members and never publishes progress, so it now takes `Pick<..., 'runReplay' | 'cleanupSession' | 'finalizeAttempt'>`. That is why no runtime test fixture needed changing — the attempt runtime never gained the capability in the first place. Also drops the last two `replay/script.ts` type references from `session-test-runtime.ts`, so the engine import is gone from that file too. Reporter contract preserved: `session-test-reporter-values.test.ts` and `session-test-reporter-values-maestro.test.ts` both pass unmodified (27 tests green across the five scheduler suites). Typecheck clean. Remaining scheduler boundary for P3b: `request/cancel.ts`, `replay/format.ts`, `replay/script.ts` in discovery, `session-store.ts`, `daemon/types.ts`, `replay-source-discovery.ts`, `core/dispatch*`, `utils/diagnostics.ts`. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): ask the host whether the suite is canceled The scheduler called `isRequestCanceled(requestId)` in five places. That both reaches a request-global registry and forces the scheduler to name a daemon request id as the cancellation key — neither survives the move into `packages/replay-test`. The host now binds the predicate to its own request and passes `isCanceled: () => boolean`. The scheduler asks a question it is entitled to ask and learns nothing about how cancellation is tracked. `shouldStopReplayTestExecution` takes the capability rather than a request id, so no scheduler function threads a daemon identifier for this purpose any more. `session-test-attempt.ts` and `session-test.ts` no longer import `request/cancel.ts` at all. It remains in `session-test-runtime.ts`, which does something different — `registerRequestAbort`, `markRequestCanceled` and the parent-abort relay are cancellation *binding*, which the brief assigns to the daemon adapter, so that split is its own step. Behavior preserved: both pinned reporter characterizations pass unmodified, 32/33 across the five scheduler suites. The one failure is the pre-existing P2/#1506 discovery-ordering regression, unrelated and untouched here. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): drop the dead request-tracking call from attempt ids `buildReplayTestAttemptRequestId` wrapped its template in `resolveRequestTrackingId`, pulling `request/cancel.ts` into the scheduler. That wrapper substitutes a generated id only when its first argument is an empty string. The template here always contains `:test:`, so it is never empty and the wrapper always returned it unchanged — the call is unreachable in this path. Probed all three shapes (explicit request id, suite-id fallback with a shard, and degenerate empty inputs); every one returns the template verbatim. Removing it takes `request/cancel.ts` out of discovery without altering a single produced id. The scheduler mints attempt identity itself, which is what the brief asks for. Evidence the ids are byte-identical: the pinned reporter characterizations assert exact session strings such as `default:test:suite-reporter:1-02-retry:attempt-1` and pass unmodified — 30 tests green across the reporter, suite and discovery suites. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): move cancellation binding and diagnostics to the host `session-test-runtime.ts` held the last two request-globals in the scheduler: `request/cancel.ts` (registerRequestAbort, markRequestCanceled, clearRequestCanceled, plus the parent-abort relay) and `utils/diagnostics.ts`. These are different in kind from the earlier ports. The brief gives the daemon adapter the job of mapping an attempt id to daemon request identifiers and *binding cancellation*, while timeout policy stays scheduler-owned. So the scheduler now receives a per-attempt capability with exactly two verbs — `cancel()` on timeout and `release()` when the attempt settles — and every registry interaction, including `relayReplayTestAbortFromParent`, moved to `session-replay.ts` next to the rest of the adapter. Diagnostics became a narrow publish capability for the same reason: `emitDiagnostic` reads a request-global scope. The level vocabulary is spelled out at the seam rather than imported, so nothing engine- or daemon-shaped crosses it. The runtime fixtures drive the real exported host binding rather than a stub. They assert cancellation through `isRequestCanceled`, and a stubbed binding would have kept those assertions passing while proving nothing. 24 tests green across the runtime, suite and both reporter characterizations, which pass unmodified. Typecheck, lint and oxfmt clean. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): split discovery into host inspection and scheduler policy discoverReplayTestEntries expanded paths, read every file, and called both engines — readReplayScriptMetadata for .ad, inspectMaestroFlow for Maestro — plus resolveReplayFormat to choose between them. Four imports a format-neutral scheduler cannot hold. Inspection is now the host's discoverSources capability. What stays in the scheduler is the genuinely neutral half: which sources a --platform filter runs, which it skips and with what message, and the empty-suite error. The manifest carries exactly the four fields the scheduler consumes (platform, target, retries, timeoutMs) plus the reporter's title, per the brief's instruction not to add more without a demonstrated call site. The platform tag is what removes the last format leak. The filter used to ask resolveReplayFormat(...) === 'maestro' to decide whether a missing platform was disqualifying. It now reads a tag: caller-bound means the invocation supplies the platform, unspecified means the source declared none. Maestro is what caller-bound looks like from the scheduler's side, and the format cannot be recovered from it. Discovery tests drive the real inspection capability, writing actual .ad and Maestro sources — a stubbed host half would have kept them green while proving nothing about the composition they exist to pin. 35 tests green across discovery, suite, runtime and both reporter characterizations, which pass unmodified. The Maestro one is the direct check that titles still flow, since they now arrive via the manifest. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): build attempt ids from named segments; trim comments Review feedback on the attempt-id builder: the comment explained a deletion that git already records, and it sat above an opaque template literal. The id is now a segment list joined on ':', so its shape is readable without prose. Output is byte-identical — the reporter characterizations assert exact session and attempt strings and pass unmodified. Applied the same standard to four other docblocks in this PR that narrated what the code used to do rather than what it does. The durable 'why' stays: which side of the seam owns what, and why the vocabulary is neutral. The migration history goes, since git carries it and these docblocks will outlive the migration. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): move shard device binding to the host buildReplayTestShardPlan called listDeviceInventory to discover what to shard across, and buildReplayTestShardFlags constructed daemon CommandFlags for the nested request. Inventory enumeration, allowlists, simulator set paths, explicit --device selectors and the too-few-devices error are host concerns; what is scheduler-owned is deciding how many shards exist and which entries each one runs. The scheduler now receives resolved shard targets through a capability. The target is neutral: id and name for session labels and progress metadata, plus platform and target, which are already kernel vocabulary. DeviceInfo no longer crosses into scheduling. One behavior note: an explicit --device selector could in principle name a web target, which is not a shardable device. That is now rejected with INVALID_ARGS rather than widening the neutral platform vocabulary to carry something the scheduler can never run. Implicit selection already filtered to mobile. 919 of 920 handler tests pass. The one failure, session-test-runner.test.ts 'binds each replay script to its declared platform metadata', fails identically on clean origin/main in this container and is unrelated: directory discovery walks with opendirSync/readSync and directory results are deduped but not sorted, while glob results are sorted, so suite order is filesystem-dependent. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): extract packages/replay-test behind a façade Completes the P3b extraction. The scheduler, attempt runtime, discovery policy, sharding distribution, artifacts and neutral types now live in packages/replay-test/src/internal/, with one package-root export. The façade takes a neutral ReplayTestSuiteRequest and returns a tagged ReplayTestSuiteOutcome. DaemonRequest, DaemonResponse and CommandFlags no longer reach the scheduler; the adapter translates flags and meta in, and the outcome back to a daemon response. Eight flags were read by the scheduler and each became a field it owns. Host work moved to daemon adapters: source inspection (both engines and format routing), shard device binding and shard-flag parsing, and artifacts-dir home expansion, which is why the package can resolve paths without SessionStore. The one remaining shared concern was the timing trace: the host writes video lifecycle events into the same trace the scheduler owns. Rather than export a writer from the façade, each attempt hands the host an appendTimingEvent closure, so the trace format stays private and the authority is scoped to that attempt. Tests mirror the topology. Discovery tests split along the seam they now cross: ordering, traversal and routing are pinned host-side against real files, filtering policy is pinned in the package against fake sources. The runtime tests assert the scheduler's cancellation obligation (cancel once on timeout, always release) against a recording binding, and a new daemon test pins the adapter's half — registry entries, the parent-abort relay, and detach on release — so that coverage moved rather than disappeared. R10 retargeted to packages/replay-test/src/ and the zone ranked alongside maestro. R11 confirms zero root-src imports from the package. 914 of 915 handler and package tests pass. The one failure, session-test-runner 'binds each replay script to its declared platform metadata', fails identically on clean main here: directory discovery walks with opendirSync and dedupes without sorting, while globs sort, so suite order is filesystem-dependent. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(daemon): simplify replay-test request translation Fallow flagged toReplayTestSuiteRequest at 14 cyclomatic in 18 lines. The branches were self-inflicted: every req.flags?.x is one, and each optional field was written as a conditional spread to avoid setting an undefined key. exactOptionalPropertyTypes is not enabled, so assigning undefined to an optional field is equivalent and the spreads bought nothing. Destructuring flags once and extracting two flag readers removes most of the rest. One correctness note on the simplification itself: the first version used `artifactsDir && expandHome(...)`, which returns '' for an empty-string flag where the previous code called expandHome(''). Replaced with an explicit undefined check so the empty-string path is unchanged. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * test(live): share the replay test-suite harness across iOS and Android Both live journeys invoked the public test command and then re-derived the same value-contract assertions by hand — suite totals, per-script status, replay counts, non-empty JUnit. Those are claims about the published suite result and are identical on every platform, and they had already drifted: iOS iterated with readReplayCommands inline, Android cast data.tests at the call site. The shared helper owns exactly that boundary. It takes the caller's runStep rather than binding a context type, so it is not a platform-configured runner and cannot template a platform's journey. Everything a platform genuinely differs on stays with the caller: which scripts run, the retry policy (iOS 2, Android none — itself a claim worth keeping), which commands each script exercises, and the behavioral evidence. Both callers keep every verify* call they had. 67 lines removed, 15 added. Residual risk: this container has no iOS or Android devices, so the live suites could not be executed here. Typecheck and lint pass; the harness needs a run on real targets before the claim that behavior is unchanged is evidence rather than inference. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * test: pin directory enumeration in the platform-binding suite test The test wrote two scripts into a temp directory and assumed discovery would return them in creation order. Directory expansion deliberately preserves filesystem order to match Maestro — only glob expansion sorts, and 'preserves Maestro directory filesystem order' pins that with a mocked opendirSync. So the ordering contract is correct; this test's assumption about enumeration was not. It passes on CI, where small directories usually enumerate in creation order, and fails on filesystems that do not — identically on clean main, where the platform-to-script binding appears reversed. Pinning enumeration the way the discovery tests already do keeps the subject intact (each script binds to ITS declared platform, and session numbering follows discovery order) without depending on the host filesystem. The fs import became a default import because vi.spyOn cannot redefine an ESM namespace export. 915 of 915 handler and package tests now pass here. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * test: scope the enumeration spy and restore it in a finally The spy I added restored only on the happy path and asserted on its argument inside the mock implementation. Either would misfire for anything else sharing the worker: an assertion thrown from inside fs, or a leaked global opendirSync, surfaces as a worker crash with no failed test rather than a readable failure. It now delegates to the real implementation for any directory but this suite's own, and restores in a finally. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * fix(replay-test): put package tests where they are actually run Review found the moved package tests were neither executed nor typechecked. They sat under packages/replay-test/test/, but vitest's unit-core lane includes packages/*/src/**/*.test.ts, and neither the root nor the package tsconfig covers a top-level test directory. A plain unit-core run discovered zero files under the package. That is why they looked green: my earlier runs passed those paths explicitly on the command line, which masked that the default run skipped them. The count is the proof — 550 files/4741 tests before, 553/4753 now, and the delta is exactly the three files and twelve tests that were being skipped. The runtime test also imported runReplayTestAttempt from the package specifier, which the facade does not export. It would have failed the moment it was discovered. It now imports internally, like the rest of the internal tests. Also removed replayTestAttemptFailure from the facade: zero consumers outside the package, so exporting it widened the boundary for nothing. P3 asks for a one-function facade. 553 test files and 4753 tests pass; lint and the layering guard are clean. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * style: format the facade after removing the unused export A scripted edit removed the export line but left a stray blank line; oxfmt was not re-run on that file afterward, so Lint & Format caught what pnpm lint alone does not. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * fix(replay-test): typecheck the package and fix a type-only import Review found the moved package tests were transpiled by vitest but never typechecked: the root typecheck script builds six packages via tsc -b and packages/replay-test was not among them, so its tsconfig was never used. That hid a real TS2459. session-test-runtime.test.ts imported ReplayTestAttemptOutcome from ../session-test-runtime.ts, which imports that type but does not re-export it. It now imports from ../session-test-types.ts, where the type is defined. Adding the package to the tsc -b list closes the gap. Verified empirically rather than assumed: planting a string-to-number error in a package test makes typecheck fail, and removing it makes it pass. This is the second finding of the same shape on this PR — first the tests were not discovered by vitest, now they were not covered by typecheck — so the gate was confirmed to reach the files rather than trusted to. 12 package tests pass, lint, format and the layering guard are clean, and typecheck is clean with the package included. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preparatory slice of P3 from #1478. This is not the extraction — no production code moves,
packages/replay-testis not created, and no façade, manifest type, or neutral outcome type is added. The only non-test file is the layering gate.P3's brief calls for exactly this first: "Before moving production code, characterize the exact reporter start/step/result values (especially
session) and the late-timeout finalization/double-cleanup behavior" and "Extend R10 so replay-test also rejects imports fromsrc/request/**and engine internals." Doing it now keeps P3 progressing while P2 owns the Maestro façade and shared discovery/format routing, which the real extraction depends on.All characterization tests pass against current
mainunchanged — they document today's behavior, they do not propose new behavior.Reporter value projection (
toReplayTestReporterProgressEvent)commandprogress never reaches a hook;replay-test-suite→onSuiteStart;start→onTestStart;progress→onTestStep;pass|fail|skip→onTestResultwithstatusverbatim.Key presence is pinned: the projection rebuilds an object literal, so all 12 case keys (
file,title,index,total,attempt,maxAttempts,session,artifactsDir,shardIndex,shardCount,deviceId,deviceName) exist withundefinedvalues even when the wire event omitted them, soinchecks keep working. Step addsstepIndex,stepTotal,stepCommand,stepValue; result addsstatus,durationMs,retrying,message,hint. Step values carry nostatuskey at all. Suite-start is exactly{total,runnable,skipped,artifactsDir,shardMode,shardCount}.session— exact shape and provenanceFormat:
`${base}:test:${suiteInvocationId}:${runnableOrdinal}${-slug}:attempt-${n}`(session-test-discovery.ts:93,attemptIndex = 0default at:85).Four distinct provenances that deliberately disagree:
onTestStart:attempt-1— built before any attempt runs, with the defaultedattemptIndexsession-test-attempt.ts:194onTestStepattempt-1, thenattempt-2, …)session-test-attempt.ts:160, read atsession-replay-runtime.ts:449/session-replay-maestro-observer.ts:81onTestResult(retry)session-test-attempt.ts:248onTestResult(final) +suite.tests[].sessionsession-test-attempt.ts:285/292/324/333So on a retried case one reporter legitimately sees three different
sessionstrings for one test.Also pinned:
attemptis absent on start whilemaxAttemptsis present; the session's test number is the runnable ordinal whileindexis the discovery ordinal (session-test.ts:311vs:322); skipped entries reachonTestResultwith nosessionand noartifactsDir; sharded runs prefix the base name (default:shard-1:test:…) and carryshardIndex/shardCount/deviceId/deviceName.Late-timeout finalization / double cleanup
session-test-runtime.ts:124-194. Order pinned: race resolves the TIMEOUT response →clearTimeout→ if timed out, wait up to 2s (waitForReplayAfterTimeout) → finalize (:147) → cleanup (:167), with the timing trace recordingattempt_start, attempt_stop, finalize_start, finalize_stop, cleanup_start, cleanup_stop.timeoutCleanupPending,details.reason: 'timeout',timeoutMode: 'cooperative', and the raced TIMEOUT response still wins over the late success.details.reasonmutated totimeout_cleanup_pending+timeoutCleanupPending: true,test_timeout_cleanup_racediagnostic, and a second, unawaited cleanup fired only after the abandoned replay settles, whose failure is swallowed. The attempt returns having cleaned up exactly once.Loader / hook / exit-code semantics
Export precedence
createReporter→default→reporter; all six optional hook names validated by exact name; live-hook errors isolated to stderr (already pinned by P0), but a throwingonSuiteEndpropagates and skips later reporters;getExitCodesees the same suite value asonSuiteEndand can only raise the exit code.R10 extension
scripts/layering/daemon-modularity.tsonly, three hunks, none touchingDAEMON_MODULARITY_BASELINE(what P2 shrinks), so the rebase against P2 stays trivial.replay-test.forbiddenTargetRootsgainssrc/request/,src/replay/,src/compat/,src/maestro/,src/ad-replay/.checkLogicalModuleImportsskips targets inside the module's own roots (needed becausereplay-testsits insidesrc/replay/), and a new optionalrecordedMigrationImportslist exempts exactly today's four violations. It is shrink-only: deleting an import without deleting its entry fails the gate.reporters/default.ts→src/replay/divergence.tsreporters/progress.ts,reporters/registry.ts,reporting.ts→src/request/progress.tsVerified on the real graph: adding a fresh
src/request/cancel.tsimport tosrc/replay/test/trace.tsfails R10 immediately.What this surfaces for the real P3 PR
Characterization found six places where today's behavior will resist the extraction. Flagging them here so the extraction PR is designed around them rather than discovering them mid-move — the first three touch stated invariants in the brief.
sessionstring is also the liveSessionStorekey.session-test-runner.test.ts:190assertssessionStore.get('default:test:suite-cleanup:1-01-android:attempt-1'). A scheduler-owned opaqueReplayTestAttemptIdmust round-trip to that exact string through the adapter, and the adapter must render the "attempt 0" form for the start event before any attempt exists.session-replay-runtime.ts:449,session-replay-maestro-observer.ts:81) viaAsyncLocalStorageseeded atsession-test-attempt.ts:153-163.packages/replay-testcannot ownonTestStepemission; the adapter must keep it and keep injecting the per-attempt session/artifactsDir. Both emitters sit in P2's neighborhood..addivergence data —reporters/default.ts:9,224callsformatReplayDivergenceReport(result.error?.details)(ADR 0012 text surface). This directly contradicts "replay-test imports neither engine": either that formatter becomes a neutral leaf or the rendering moves to the adapter.markReplayTimeoutCleanupPendingwritesresponse.error.details(session-test-runtime.ts:316) andappendReplayTestWarningwritesresponse.data.warnings(:331). Neutral immutable outcomes need an explicit re-projection step.void cleanupSessionAfterLateReplay(...),:139). If replay-test owns thefinallyorchestration, an unawaited daemon effect escapes the module with no handle, and the suite result returns while it is still pending.onSuiteStartreportstotalshard-multiplied butrunnable/skippedpre-shard (session-test.ts:147-157— 1 file × 2 shards yieldstotal: 2, runnable: 1). Worth an explicit decision before P3 freezes it into a package contract.Gates
pnpm check:affected --base origin/main --run→ pass at this exact head (rebased onto3c09bb256)pnpm check:layering→ pass; R10 reports "zero forbidden logical-module imports beyond 4 recorded migration import(s)"One note for reviewers: an earlier run of the coverage lane failed on the pre-rebase head and did not reproduce after rebasing. The same run flagged three Apple screenshot tests over budget with "within the 2x load-variance band," which points at container load rather than this change, but it was not conclusively diagnosed. Worth a glance at the coverage lane here.
Sequencing
Does not touch
src/compat/maestro/**,session-replay-runtime.ts, orsession-test-discovery.ts— P2 owns those. Rebased cleanly onto #1501 with zero conflicts.Refs #1478.
Generated by Claude Code