refactor(replay-test): neutralize the values crossing the scheduler seam (#1478 P3, part 1) - #1509
Conversation
#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
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact head What I verified:
Exact-head validation:
GitHub CI is still in progress at the time of review, with no failures currently reported. From the code and local validation side, this is ready once the remaining required checks pass. |
|
Coverage lane failed at What failedThe second error in the log ( Why it isn't this PRStructural, and I think decisive: this PR touches no file on the find/snapshot/polling path. The assertion itself is a poll count inside a timeout window ( The log also notes the repo's contention-retry mechanism declined to retry: "No retry: a failure landed outside the enumerated retry list." So this test isn't enrolled in #1419's retry set. What I could not establishI ran the test 5× in isolation on clean Suggested next step is a re-run of the Coverage lane. If it recurs, enrolling Separately, a useful data point on the other known failure
That is the P2 discovery-ordering issue showing both faces on one commit: Generated by Claude Code |
|
Reviewed exact head The native reporter ratchet covers the new The remaining P3a seam is coherent. Exact-head Coverage is also red because unchanged Residual risk: no separately authorized cross-vendor review was performed. |
…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
|
Fixed in So an New scenario
1. Step payload + attempt identity. A two-step flow (
The flow's 2. Attempt-scoped session across a retry. First attempt's New file rather than an addition to Counterfactual (run, not assumed)I broke each link independently and restored it. Both times the native reporter test kept passing while the new one failed — precisely the hole described. Link A — drop Link B — drop the Both restored; Gates
format ✅ · lint ✅ · typecheck ✅ · layering ✅ · fallow ✅ ( The summary line
Bundle sizeTest-only change; output is byte-identical to
No label applied. Generated by Claude Code |
|
Code review is clean at Merge readiness still needs fresh integration and practical evidence. This head is three commits behind live Residual risk: no separately authorized cross-vendor review was performed. |
|
Updated onto live 1. Branch updated:
|
|
CI red on Smoke Tests (iOS live-simulator E2E) at The failure on this PR and on clean Same test (
Last known green is For completeness on why it isn't this PR: the diff is 41 files confined to Two related notes:
Generated by Claude Code |
|
Implements the first half of P3 of #1478, following the design corrections in #1478 (comment).
Scope decision: P3 lands as a sequence
The brief anticipated this. P3 as written is one PR containing four independent redesigns plus a physical move of ~3,450 production lines and their mirrored tests. I split it along the line the brief suggested:
src/. No files move.packages/replay-test/src/internal/, the pure re-export barrel façade, the neutral manifest + discovery split, thesrc/utilsleaf splits (duration-format,status-markers, and the colour/TTY leaf carved out ofoutput.ts), the scheduler-mintedReplayTestAttemptIdwithhost.attemptSessionLabel, the remaining request-global ports (cancellation, diagnostics), and the R10rootsretarget.Splitting this way means P3b is a move plus mechanical import rewrites against a seam that is already correct, rather than a 5,000-line diff where a behavior regression and a file relocation are indistinguishable in review.
What this PR does
1.
DaemonResponseno longer crosses the seamThis was the largest single piece of P3 and the reason the phase is bigger than the original brief implied.
session-test-types.ts:1typedrunReplay/finalizeAttemptas returningDaemonResponse, and the scheduler read.error.code,.error.details, and.data.replayed/.healed/.warnings/.snapshotDiagnosticsoff it throughoutsession-test-attempt.ts,session-test-runtime.ts, andsession-test-artifacts.ts. R10 does not see this today only becausecheckDaemonTypesImportersskips files undersrc/daemon/. The moment those files land inpackages/replay-test/they become externaldaemon/types.tsimporters, and that ratchet only permits shrinking — so neutral outcomes were required for the gate to pass, not optional polish.Attempts now resolve as a tagged value:
The failure
errorisReplaySuiteTestFailed['error']— the ADR 0010 error the public suite result already publishes, not a daemon bag.artifactPathsis hoisted out ofdata.artifactPaths/error.details.artifactPathsbecause the artifact writer is the only reader.infrastructureis the one genuinely new field. Classifying an environmental failure needsisInfrastructureBootFailureReasonfromsrc/platforms/boot-diagnostics.tsplus a message-pattern table — platform vocabulary a format-neutral scheduler must not import. So the host decides and the scheduler reads the verdict.runReplayTestCasecorrespondingly returns aReplayTestCaseReport({ result, infrastructure }) so fail-fast and infrastructure-stop stay scheduler-owned.New
src/daemon/handlers/session-test-outcome.tsis the single place aDaemonResponsebecomes an outcome. Finalization returnsReplayTestAttemptFailed | undefined— a successful finalization has nothing the scheduler can act on, andundefinedis what the port already meant by "nothing to report".2. Step events get a narrow per-attempt
onStepportStep payloads originate below the attempt boundary, inside engine execution, and reached the reporter through a request-global
AsyncLocalStorageseeded atsession-test-attempt.ts:153-163and read atsession-replay-runtime.ts:461andsession-replay-maestro-observer.ts:68. Keeping it was not an option: #1505 recorded those as shrink-only R10 entries and P3's Done criterion forbids request-global imports.The scheduler now hands each attempt an
onStep(step: { index, total, command?, value? })sink, threaded the waytracePathalready is (runReplayScriptFile→executeReplayActions, andrunTypedMaestroReplayFile→createMaestroReplayObserver). Two real adapters — native.adand Maestro — so the port is earned.withReplayTestActionProgressandreadReplayTestActionProgressare deleted. A directreplaysimply has no sink and emits nothing, exactly as an empty ALS store did.The scheduler owns the reporter-facing translation, so
file/session/attempt/shard identity on a step event now come from the attempt context by construction rather than fromprogress.file || filefallbacks.3.
formatReplayDivergenceReportbecomes a neutral leafP5 lists divergence among what moves into
packages/ad-replay. It cannot: Maestro constructs divergences too (session-replay-maestro-failure.ts), and two non-replay surfaces render them — CLI (utils/output.ts) and MCP (mcp/tool-error.ts). The module depended only on@agent-device/kernel/contracts(type-only) and@agent-device/kernel/redaction; it was already neutral.src/replay/divergence.ts→packages/contracts/src/replay-divergence.ts, exposed at@agent-device/contracts/divergence(focused subpath). 15 importers updated. Renderer output text is unchanged and still pinned bysrc/replay/test/__tests__/reporters-default.test.ts.4. Progress wire vocabulary moves to
@agent-device/contracts/progressRequestProgressEventis serialized bysrc/daemon/request-progress-protocol.tsand reconstructed by the CLI before reaching the reporter registry — it is a wire contract belonging to neither side. Moving the three event shapes into contracts lets the reporter tree stop importingsrc/request/progress.tswithout changingrunReplayTestReporterProgress's signature, so the pinned characterization test compiles and passes unmodified.src/request/progress.tskeeps only the sink type and its AsyncLocalStorage binding.5. R10
All four recorded migration imports are now gone, so they are deleted from
daemon-modularity.tsand the ratchet enforces unconditionally for replay-test:replay-testrecordedMigrationImportsdaemon/types.tsimportersroots: ['src/replay/test/']still matches, so the rule is live throughout; retargeting it is P3b's job, in the same commit as the move.Compatibility
No shipped behavior changes. Preserved deliberately:
src/daemon/handlers/__tests__/session-test-reporter-values.test.tspasses unmodified.--shard-allasymmetry (onSuiteStartreportstotalshard-multiplied butrunnablepre-shard) as characterized in test: characterize replay-test reporter contract and extend R10 #1505.result.txt/failure.txtline set and ordering, includingtimeoutMode: cooperativeandcopiedArtifacts(artifact paths still deduped; warnings still not).timeout_cleanup_pendingdetails on the wire, and the exactly-once/deferred-second cleanup contract.Files
Moved:
src/replay/divergence.ts→packages/contracts/src/replay-divergence.ts;src/replay/__tests__/divergence.test.ts→packages/contracts/src/replay-divergence.test.ts.Created:
src/daemon/handlers/session-test-outcome.ts,packages/contracts/src/request-progress.ts,packages/contracts/src/facades/divergence.ts,packages/contracts/src/facades/progress.ts.Deleted:
withReplayTestActionProgress,readReplayTestActionProgress,ReplayTestActionProgressContext,markReplayTimeoutCleanupPending's in-place mutation,appendReplayTestWarning's in-place mutation,getReplayTestArtifactPaths,replayTestResponseMetrics,replayTestWarningsResultMetadata,readReplayResponseSnapshotDiagnostics.Gates
format ✅ · lint ✅ · typecheck ✅ · layering ✅ · fallow ✅ · mcp-metadata ✅ · build ✅ · vitest-related — 2638 passed, 1 failed.
The one failure is a pre-existing P2 regression, not extraction fallout
session-test-runner.test.ts:148 > test binds each replay script to its declared platform metadataexpects['android', 'ios']and gets['ios', 'android']. It fails identically on cleanorigin/main(32b9db2d7) with this entire diff stashed. It is deliberately left failing.It is a real regression merged by P2 (#1506), not a flake.
discoverReplaySourcePaths(src/daemon/replay-source-discovery.ts:31-35) sorts glob groups but returns directory inputs in rawopendirSync/readSyncorder, so a directory of.adfiles executes in filesystem order. Users number files01-,02-precisely to control suite order, and that ordinal also feeds the session name (1-01-android) and the reporterindex, so ordinals and artifact paths drift with it — and it varies across filesystems, so it produces flaky suites rather than a consistently wrong order. It survived P2 with only one failing test because the sibling case at:112-121was made order-agnostic (.find()instead of positional indexing).Neither
session-test-runner.test.tsnorsrc/daemon/replay-source-discovery.tsis modified here —git diff origin/mainon both is empty. Making the assertion order-independent would cement the regression, and fixing the ordering is a behavior change that belongs in its own PR rather than buried in an extraction diff. Tracked separately by the coordinator.pnpm check:layeringstandalone: ✅ — 971 source files, R2–R11 all green.Bundle size (informational check, measured locally, 3 startup runs, same machine):
--version--helpChunk graph unchanged; the only movement is +0.6 kB raw in
dist/src/session.js. No P2-style shared-chunk regression.STOP conditions
None hit. No scheduler decision needed mutable daemon state, no engine-specific manifest field was required, no reporter gained control over attempts or sessions, and the reporter contract is intact. This PR does not depend on discovery ordering being deterministic.
Refs #1478
🤖 Generated with Claude Code
https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8