Skip to content

refactor(replay-test): neutralize the values crossing the scheduler seam (#1478 P3, part 1) - #1509

Merged
thymikee merged 3 commits into
mainfrom
p3/extract-replay-test
Jul 31, 2026
Merged

refactor(replay-test): neutralize the values crossing the scheduler seam (#1478 P3, part 1)#1509
thymikee merged 3 commits into
mainfrom
p3/extract-replay-test

Conversation

@thymikee

@thymikee thymikee commented Jul 30, 2026

Copy link
Copy Markdown
Member

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:

  • P3a (this PR) — make every value crossing the scheduler seam neutral, in place under src/. No files move.
  • P3b (next) — the physical move to packages/replay-test/src/internal/, the pure re-export barrel façade, the neutral manifest + discovery split, the src/utils leaf splits (duration-format, status-markers, and the colour/TTY leaf carved out of output.ts), the scheduler-minted ReplayTestAttemptId with host.attemptSessionLabel, the remaining request-global ports (cancellation, diagnostics), and the R10 roots retarget.

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. DaemonResponse no longer crosses the seam

This was the largest single piece of P3 and the reason the phase is bigger than the original brief implied.

session-test-types.ts:1 typed runReplay/finalizeAttempt as returning DaemonResponse, and the scheduler read .error.code, .error.details, and .data.replayed/.healed/.warnings/.snapshotDiagnostics off it throughout session-test-attempt.ts, session-test-runtime.ts, and session-test-artifacts.ts. R10 does not see this today only because checkDaemonTypesImporters skips files under src/daemon/. The moment those files land in packages/replay-test/ they become external daemon/types.ts importers, 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:

type ReplayTestAttemptOutcome =
  | { status: 'passed';  replayed; healed; warnings; artifactPaths; snapshotDiagnostics? }
  | { status: 'failed';  error; artifactPaths; snapshotDiagnostics?; infrastructure: boolean };

The failure error is ReplaySuiteTestFailed['error'] — the ADR 0010 error the public suite result already publishes, not a daemon bag. artifactPaths is hoisted out of data.artifactPaths/error.details.artifactPaths because the artifact writer is the only reader.

infrastructure is the one genuinely new field. Classifying an environmental failure needs isInfrastructureBootFailureReason from src/platforms/boot-diagnostics.ts plus a message-pattern table — platform vocabulary a format-neutral scheduler must not import. So the host decides and the scheduler reads the verdict. runReplayTestCase correspondingly returns a ReplayTestCaseReport ({ result, infrastructure }) so fail-fast and infrastructure-stop stay scheduler-owned.

New src/daemon/handlers/session-test-outcome.ts is the single place a DaemonResponse becomes an outcome. Finalization returns ReplayTestAttemptFailed | undefined — a successful finalization has nothing the scheduler can act on, and undefined is what the port already meant by "nothing to report".

2. Step events get a narrow per-attempt onStep port

Step payloads originate below the attempt boundary, inside engine execution, and reached the reporter through a request-global AsyncLocalStorage seeded at session-test-attempt.ts:153-163 and read at session-replay-runtime.ts:461 and session-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 way tracePath already is (runReplayScriptFileexecuteReplayActions, and runTypedMaestroReplayFilecreateMaestroReplayObserver). Two real adapters — native .ad and Maestro — so the port is earned. withReplayTestActionProgress and readReplayTestActionProgress are deleted. A direct replay simply 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 from progress.file || file fallbacks.

3. formatReplayDivergenceReport becomes a neutral leaf

P5 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.tspackages/contracts/src/replay-divergence.ts, exposed at @agent-device/contracts/divergence (focused subpath). 15 importers updated. Renderer output text is unchanged and still pinned by src/replay/test/__tests__/reporters-default.test.ts.

4. Progress wire vocabulary moves to @agent-device/contracts/progress

RequestProgressEvent is serialized by src/daemon/request-progress-protocol.ts and 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 importing src/request/progress.ts without changing runReplayTestReporterProgress's signature, so the pinned characterization test compiles and passes unmodified. src/request/progress.ts keeps 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.ts and the ratchet enforces unconditionally for replay-test:

before after
replay-test recordedMigrationImports 4 0
recorded migration imports, all modules 4 0
external daemon/types.ts importers 2 2 (unchanged)
R7 writer-owned fields / owner claims 30 / 42 30 / 42 (unchanged)
R9 largest type cycle 76 76 (unchanged)
R11 exported subpaths 21 23

roots: ['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:

  • The reporter contract in full — module export spellings, object/factory loading, hook names, hook timing/order, value fields, the synchronous live-hook rule, awaited suite completion, error handling, exit-code semantics. src/daemon/handlers/__tests__/session-test-reporter-values.test.ts passes unmodified.
  • The --shard-all asymmetry (onSuiteStart reports total shard-multiplied but runnable pre-shard) as characterized in test: characterize replay-test reporter contract and extend R10 #1505.
  • result.txt/failure.txt line set and ordering, including timeoutMode: cooperative and copiedArtifacts (artifact paths still deduped; warnings still not).
  • Finalize-before-cleanup ordering, the 2s late-cleanup grace window, timeout_cleanup_pending details on the wire, and the exactly-once/deferred-second cleanup contract.
  • No reporter v2, no v1 adapter, no deprecation, no migration docs, no new public reporter type exports.

Files

Moved: src/replay/divergence.tspackages/contracts/src/replay-divergence.ts; src/replay/__tests__/divergence.test.tspackages/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

pnpm check:affected --base origin/main --run

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 metadata expects ['android', 'ios'] and gets ['ios', 'android']. It fails identically on clean origin/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 raw opendirSync/readSync order, so a directory of .ad files executes in filesystem order. Users number files 01-, 02- precisely to control suite order, and that ordinal also feeds the session name (1-01-android) and the reporter index, 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-121 was made order-agnostic (.find() instead of positional indexing).

Neither session-test-runner.test.ts nor src/daemon/replay-source-discovery.ts is modified here — git diff origin/main on 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:layering standalone: ✅ — 971 source files, R2–R11 all green.

Bundle size (informational check, measured locally, 3 startup runs, same machine):

Metric Base This PR Diff
JS gzip 611.8 kB 611.9 kB +0.1 kB
npm tarball 729.4 kB 729.5 kB +0.1 kB
CLI --version 50.3 ms 52.7 ms +2.4 ms (noise)
CLI --help 97.2 ms 90.2 ms −7.0 ms (noise)

Chunk 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

#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
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.91 MB 1.91 MB +508 B
JS gzip 612.2 kB 612.3 kB +91 B
npm tarball 729.7 kB 729.8 kB +102 B
npm unpacked 2.56 MB 2.56 MB +508 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.2 ms 25.9 ms -0.2 ms
CLI --help 56.7 ms 59.1 ms +2.4 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/session.js +616 B +125 B
dist/src/runner-disposal.js -108 B -32 B
dist/src/selector-runtime.js 0 B -2 B
dist/src/internal/daemon.js 0 B -1 B
dist/src/interaction.js 0 B +1 B

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed exact head b339c640ffd0da4437c9bc3afbab124f6f23d196 against #1478 P3 and the relevant architecture constraints. No blocking findings.

What I verified:

  • The scheduler no longer receives DaemonResponse; the conversion is confined to the host seam and preserves replay/heal metrics, warnings, artifact paths, snapshot diagnostics, and the public error shape.
  • Infrastructure classification remains host-owned. Retry, fail-fast, and suite-stop behavior are preserved, including timeout_cleanup_pending.
  • The per-attempt onStep capability has two real adapters. Native .ad and Maestro preserve their existing step indices/values, while direct replay remains a no-op when no observer is supplied. Reporter attempt identity is now derived by the scheduler rather than leaked through daemon-local context.
  • Finalize-before-cleanup ordering, the 2-second cleanup grace period, deferred late cleanup, and pass-with-warning behavior on finalization failure remain intact.
  • Moving replay divergence into contracts is appropriate: it is neutral shared vocabulary consumed by Maestro, CLI, and MCP paths. Progress event types likewise belong to the wire-contract package; daemon-local sink plumbing stays private.
  • The four recorded R10 migration imports are gone without widening engine control or reporter access. P3b is still needed for the physical replay-test package extraction, as intended.

Exact-head validation:

  • 8 focused test files / 133 tests passed
  • layering gate passed (R10 at zero; package/R11 constraints green)
  • git diff --check passed
  • bundle-size check passed; total raw/gzip/tarball sizes decreased slightly

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.

Copy link
Copy Markdown
Member Author

Coverage lane failed at b339c640. Diagnosed — not caused by this PR, and I'm not patching around it.

What failed

FAIL  unit-core  src/daemon/handlers/__tests__/find.test.ts
      > handleFindCommands wait captures fresh snapshots while polling
AssertionError: expected "vi.fn()" to be called 2 times, but got 3 times
      find.test.ts:751  expect(mockDispatch).toHaveBeenCalledTimes(2)

Test Files  1 failed | 615 passed (616)
Tests       1 failed | 5182 passed | 4 skipped (5187)

The second error in the log (check:coverage-changed: no lcov report … Run pnpm test:coverage first) is downstream — the coverage run aborted before emitting lcov.info, so the changed-line gate had nothing to read. One root cause, not two.

Why it isn't this PR

Structural, and I think decisive: this PR touches no file on the find/snapshot/polling path. find.test.ts, find.ts, and the snapshot polling code are absent from git diff --name-only origin/main.... The 40 changed files are confined to session-test-*, the replay divergence/progress move into @agent-device/contracts, the reporter tree, and the layering scripts.

The assertion itself is a poll count inside a timeout window (find wait timed out, then exactly 2 dispatches), which is inherently timing-sensitive. The same run's slow-test gate reported three Apple tests over budget "within the 2x load-variance band," so the runner was loaded — one extra poll fitting before the deadline is the expected symptom.

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 establish

I ran the test 5× in isolation on clean main and 3× more under synthetic CPU load — 8 passes, no reproduction. So I have not empirically demonstrated flakiness; I've only ruled out this PR as the cause. If it reproduces on a re-run at the same head, that would point at something real in the test rather than load, and it should be looked at on its own.

Suggested next step is a re-run of the Coverage lane. If it recurs, enrolling find.test.ts in the #1419 retry list is the wrong first move — a poll-count assertion that drifts under load is worth understanding before it's made tolerant.

Separately, a useful data point on the other known failure

session-test-runner.test.ts > test binds each replay script to its declared platform metadata passed in this lane (615 files passed, only find.test.ts failed), while it fails deterministically on my machine at the same commit.

That is the P2 discovery-ordering issue showing both faces on one commit: discoverReplaySourcePaths doesn't sort directory inputs (src/daemon/replay-source-discovery.ts:31-35), so order follows raw readdir — creation-ordered on this runner, hash-ordered locally. The defect isn't a wrong order, it's the absence of one. Still tracked separately; untouched by this PR.


Generated by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed exact head b339c640ffd0da4437c9bc3afbab124f6f23d196 against #1478 P3a. One actionable test gap remains.

The native reporter ratchet covers the new onStep path, but it only executes an .ad suite. Please add the equivalent Maestro test --maestro reporter scenario and assert the step payload plus attempt/session identity. The changed Maestro observer → runtime → scheduler sink is otherwise unprotected: removing one forwarding link would silently drop onTestStep while current reporter tests remain green. Maestro is one of P3’s two required real adapters, so this needs a counterfactual regression test.

The remaining P3a seam is coherent. Exact-head Coverage is also red because unchanged find.test.ts expected 2 polling dispatches and observed 3 (5,182 other tests passed); rerun/reconcile that owner-action CI failure before merge. Do not apply ready-for-human yet.

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

Copy link
Copy Markdown
Member Author

Fixed in ec3a452f8. The reviewer is right, and the gap was wider than "one missing scenario": below runReplayScriptFile the two onStep chains share no links at all.

native:   scheduler sink -> runReplayScriptFile -> executeReplayActions -> onStep(...)
maestro:  scheduler sink -> runReplayScriptFile -> runTypedMaestroReplayFile
                         -> createMaestroReplayObserver({ onStep }) -> actionStarted -> onStep(...)

So an .ad-only ratchet could not have caught a Maestro break by construction.

New scenario

src/daemon/handlers/__tests__/session-test-reporter-values-maestro.test.ts — a test --maestro suite driven through the real handleSessionCommands and the real reporter registry, i.e. the same route a --reporter ./mine.mjs module gets. Two cases:

1. Step payload + attempt identity. A two-step flow (assertNotVisible: "Nope", then back) so stepIndex/stepTotal are distinguishable and one step carries a value while the other does not. It asserts the full hook sequence (onSuiteStart, onTestStart, onTestStep, onTestStep, onTestResult, onSuiteEnd), then, on the step events, both halves of the value:

  • engine-produced payload, arriving through the observer's onStep: stepIndex: 1, stepTotal: 2, stepCommand: 'assertNotVisible', stepValue: 'Nope', then stepIndex: 2, stepTotal: 2, stepCommand: 'back' with stepValue explicitly undefined — the observer must not invent a value where the engine emits none;
  • scheduler-produced identity, which before P3 came from the request-global AsyncLocalStorage: file, index, total, attempt, maxAttempts, session, artifactsDir. Also 'status' in step === false, matching the native pin.

The flow's name: populates the reporter title ('Login flow'), asserted on start, step and result — a shipped reporter value only the Maestro path can produce, since .ad scripts have no title. It was previously unpinned anywhere.

2. Attempt-scoped session across a retry. First attempt's back fails, so attempt 1 fails at step 2 and attempt 2 passes. Every step event of attempt 1 must carry …:attempt-1 and every step event of attempt 2 …:attempt-2 — the exact identity the deleted ALS used to supply, now threaded from the scheduler's attempt context through the Maestro observer. Asserted as a per-event attemptsession correspondence, not a fixed list, plus new Set(sessions).size === 2 so a chain that froze the session at attempt 1 fails.

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.ts (device resolution) — vitest allows one mock per module per file.

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 onStep from createMaestroReplayObserver(...) in session-replay-maestro-runtime.ts (Maestro-only link, invisible to the native chain):

 ✓ |unit-core| src/daemon/handlers/__tests__/session-test-reporter-values.test.ts (4 tests) 46ms
 ❯ |unit-core| src/daemon/handlers/__tests__/session-test-reporter-values-maestro.test.ts (2 tests | 2 failed) 54ms
   × a Maestro suite reaches the reporter with step payloads and attempt-scoped identity 40ms
   × Maestro step sessions track the running attempt across a retry 13ms

AssertionError: expected [ 'onSuiteStart', 'onTestStart', …(2) ] to deeply equal [ 'onSuiteStart', 'onTestStart', …(4) ]

- Expected
+ Received

  [
    "onSuiteStart",
    "onTestStart",
-   "onTestStep",
-   "onTestStep",
    "onTestResult",
    "onSuiteEnd",
  ]

AssertionError: expected Set{} to deeply equal Set{ 1, 2 }

 Test Files  1 failed | 1 passed (2)
      Tests  2 failed | 4 passed (6)

Link B — drop the emitMaestroStep(onStep, event) call from actionStarted in session-replay-maestro-observer.ts:

 ✓ |unit-core| src/daemon/handlers/__tests__/session-test-reporter-values.test.ts (4 tests) 47ms
 ❯ |unit-core| src/daemon/handlers/__tests__/session-test-reporter-values-maestro.test.ts (2 tests | 2 failed) 55ms
      Tests  2 failed | 4 passed (6)

Both restored; git diff on src/daemon/handlers/ is empty and the pair is green:

 ✓ session-test-reporter-values.test.ts (4 tests) 47ms
 ✓ session-test-reporter-values-maestro.test.ts (2 tests) 50ms
 Test Files  2 passed (2)   Tests  6 passed (6)

Gates

pnpm check:affected --base origin/main --run (written to a file, exit code read directly):

format ✅ · lint ✅ · typecheck ✅ · layering ✅ · fallow ✅ (No issues in 41 changed files) · mcp-metadata ✅ · build ✅ · vitest-related 2640 passed, 1 failed (was 2638 passed — the two new cases).

The summary line check:affected: all runnable checks passed. does not appear, and cannot on this branch. The single failure is the excluded pre-existing P2/#1506 discovery-ordering regression, session-test-runner.test.ts:148 > test binds each replay script to its declared platform metadata, which I was told not to touch pending the product decision. It fails identically on clean origin/main. git diff origin/main is empty for both session-test-runner.test.ts and src/daemon/replay-source-discovery.ts. Every other check in the gate reports clean, and session-test-reporter-values.test.ts passes unmodified (4/4). find.test.ts was not touched and did not run in this lane.

session-test-reporter-values-maestro.test.ts is picked up by vitest related off the changed session-replay-maestro-observer.ts / session-replay-maestro-runtime.ts, so the new ratchet runs whenever either link is edited.

Bundle size

Test-only change; output is byte-identical to b339c640f.

Metric main b339c640f ec3a452f8
JS gzip 611.8 kB 611.9 kB 611.9 kB
npm tarball 729.4 kB 729.5 kB 729.5 kB

No label applied.


Generated by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Code review is clean at ec3a452f887b3459e9eea0e2df3f52e9afa04091: the new Maestro characterization drives the real test --maestro session-handler → reporter-registry path, pins step payload/title/attempt-session identity including retries, and closes the previous onTestStep forwarding gap.

Merge readiness still needs fresh integration and practical evidence. This head is three commits behind live main (209ac83), so rebase/update and rerun the exact-head gates. Because this changes a device-facing Maestro reporter route, attach one supported simulator/emulator/device test --maestro --reporter retry trace proving onTestStep delivery (and record android-helper backend/version if Android is used). Keep ready-for-human off until both are present.

Residual risk: no separately authorized cross-vendor review was performed.

Copy link
Copy Markdown
Member Author

Updated onto live main and reran the exact-head gates. The second ask — a real-device test --maestro --reporter retry trace — I cannot produce, and I'm not going to substitute anything for it. Details below. ready-for-human stays off.

1. Branch updated: ec3a452f82313f304d

Merged origin/main (209ac83df, which added #1502, #1508, #1512). Clean merge, no conflicts.

The one file worth checking by hand is scripts/layering/daemon-modularity.ts, since #1508 and this PR both edit it. The merge resolved correctly — #1508's lowered R7 baseline and this PR's recordedMigrationImports deletion both survive:

Layering guard: OK — 975 source files ... R10 pins R7 at 29 writer-owned fields /
40 owner claims, R9 at 76 files with zone ceilings, 2 external daemon/types.ts
importers, and zero forbidden logical-module imports beyond 0 recorded migration
import(s); and R11 holds 3 workspace package(s) behind 23 exported subpath(s)

Note this makes one line of the PR body stale: the R10 table says "R7 writer-owned fields / owner claims — 30 / 42 unchanged". Post-#1508 that reads 29 / 40. The number is #1508's, not this PR's; this PR still changes neither.

2. Exact-head gates

pnpm check:affected --base origin/main --run at 2313f304d: format ✅ · lint ✅ · typecheck ✅ · layering ✅ · fallow ✅ · mcp-metadata ✅ · build ✅ · vitest-related 2639 passed, 2 failed.

Both failures are pre-existing, and I checked each rather than assuming:

(a) session-test-runner.test.ts:148 > test binds each replay script to its declared platform metadata — the known P2/#1506 discovery-ordering regression, unchanged. git diff origin/main is still empty for both that test and src/daemon/replay-source-discovery.ts.

(b) cloud-webdriver-runtime.test.ts > Cloud WebDriver allocation preserves create-session failure when cleanup fails — new since the last run, and not a behavioral failure. It is the harness's own teardown throwing:

Error: ENOTEMPTY: directory not empty, rmdir '/tmp/agent-device-provider-scenario-session-Rb2ZyD/default'
 ❯ removeProviderScenarioTempDir test/integration/provider-scenarios/harness.ts:133:6

Passes in isolation on clean origin/main (7/7) and in isolation at this head (7/7); this PR's 41 changed files match nothing under provider/limrun/webdriver/harness; and CI's Integration Tests lane — which runs it — is green at this head. It is a temp-dir teardown race that surfaces under parallel local load, arriving with #1502. Flagging it as a flaky-harness issue for whoever owns that lane; it is not this PR's and I have not touched it.

3. Coverage went red once at this head, then green on re-run

First attempt failed the slow-test gate, not an assertion:

FAIL src/platforms/apple/core/__tests__/screenshot-density.test.ts
     > screenshotIos caches simulator screen scale per device
Error: Test timed out in 5000ms.
Slow-test gate: 1 test(s) exceeded 2x the wall-clock budget.
  5.01s (budget 2.5s)

That file is untouched by this PR and unchanged since #1494, and it runs in 834 ms locally at this head — 6× under the budget it blew on the runner. The same run flagged two more apple/core screenshot tests over budget "within the 2x load-variance band", so the whole cluster was running slow on that machine. #1419's contention-retry declined it by policy ("a reason a rerun cannot re-check (slow-test gate)"), so I re-ran the job manually: Coverage now passes at 2313f304d. The trailing check:coverage-changed: no lcov report in that first log was downstream of the aborted run, not a second cause.

Every other required check is green at this head; one long Smoke lane is still finishing.

4. The device trace — blocked, needs an owner decision

I can't produce it from this environment, and a mocked run would not be the evidence you asked for:

  • Linux x86_64 — iOS simulator needs macOS + Xcode; xcrun/simctl absent
  • /dev/kvm absent and no vmx/svm in /proc/cpuinfo, so no Android emulator
  • maestro, adb, emulator, sdkmanager all absent; ANDROID_HOME unset

So there is no supported simulator/emulator/device here, and no android-helper backend/version to record.

The natural place to generate it is .github/workflows/ios.yml, which already boots a real simulator on macos-26 and runs bin.ts test …/01-settings.ad --udid … --retries 2 --artifacts-dir …. It exercises a native .ad script with no --reporter, so producing this evidence means adding a --maestro --reporter step to that lane — a CI change on an otherwise behavior-neutral refactor. I'd rather have that sanctioned than slip it in: say the word and I'll add it, or someone with hardware can attach the trace directly.

Worth noting what is already pinned in the meantime, since it bounds the risk: session-test-reporter-values-maestro.test.ts drives the real handleSessionCommands → reporter-registry route and asserts step payload, title, and attempt-scoped session identity across a retry, and I verified by counterfactual that breaking either Maestro forwarding link fails it while the native ratchet stays green. What that cannot cover is the real Maestro engine emitting real actionStarted events on a real device — which is exactly your point.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

CI red on Smoke Tests (iOS live-simulator E2E) at 2313f304dfailing on the base branch too, so I'm not treating it as this PR's and will re-run when main recovers.

The failure on this PR and on clean main at 209ac83df are the same one, step for step:

✖ live iOS simulator fixture E2E
  AssertionError: step: wait for id="automation-longpress" after scroll (attempt 4)
  command: agent-device wait id="automation-longpress" 1000 --platform ios --udid … --json
  "message": "wait timed out for selector: id=\"automation-longpress\"",
  "details": { "reason": "wait_capture_stalled", "captureStalled": true, "timeoutMs": 1000 }
  scenario: smoke:automation-input
      at assertElementTextAfterScrolling (…/ios-simulator-e2e/live-assertions.ts:28:21)
      at async Object.assertAutomationInput (…/live-automation-scenario.ts:128:3)

Same test (test/integration/smoke-ios-simulator.test.ts), same scenario, same selector, same wait_capture_stalled reason, same stack — on a commit that contains none of my changes.

ios.yml history on main:

run head result
05:57 209ac83df (#1512) ❌ failure
05:24 1fc916918 (#1508) ❌ failure
19:29 6067b9a48 (#1502) ❌ failure
19:07 32b9db2d7 (#1484) ⚪ cancelled
18:58 0e51007b0 (#1506) ⚪ cancelled
18:22 e70fac7b4 (#1507) ✅ success

Last known green is e70fac7b4. The first observed failure is 6067b9a48, but the two runs between it and the last green were cancelled, so the break could have entered with #1506, #1484 or #1502 — I can't narrow it further from run history alone, and I'm not guessing at a culprit.

For completeness on why it isn't this PR: the diff is 41 files confined to session-test-*/session-replay-*, the reporter tree, packages/contracts, and the layering scripts. git diff --name-only origin/main...HEAD matches nothing under src/platforms/, and nothing on the wait/capture/snapshot path this scenario exercises. Every other required check is green at this head, including Coverage after the slow-test-gate re-run described above.

Two related notes:

  • The wait_capture_stalled symptom is worth someone owning independently — it is a simulator capture stalling against a 1000 ms wait budget, and it has now blocked three consecutive main runs.
  • It also bears on the device-trace request above: the macos-26 lane I proposed extending with a --maestro --reporter step is this same currently-broken lane. Whatever we decide there, the trace can't be produced from it until this is green.

Generated by Claude Code

@thymikee
thymikee merged commit a3ab69a into main Jul 31, 2026
43 of 45 checks passed
@thymikee
thymikee deleted the p3/extract-replay-test branch July 31, 2026 06:48
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-31 06:49 UTC

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants