From 66e6e48e94b8f474f7fa683ed4a43ccf103811bf Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 15:57:02 +0000 Subject: [PATCH 1/2] test: characterize replay-test reporter contract and extend R10 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 Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 --- scripts/layering/daemon-modularity.test.ts | 84 +++- scripts/layering/daemon-modularity.ts | 68 +++- .../session-test-reporter-values.test.ts | 365 ++++++++++++++++++ .../__tests__/session-test-runtime.test.ts | 129 +++++++ .../test/reporters/__tests__/custom.test.ts | 70 ++++ .../test/reporters/__tests__/progress.test.ts | 223 +++++++++++ .../test/reporters/__tests__/registry.test.ts | 49 +++ 7 files changed, 976 insertions(+), 12 deletions(-) create mode 100644 src/daemon/handlers/__tests__/session-test-reporter-values.test.ts create mode 100644 src/replay/test/reporters/__tests__/progress.test.ts diff --git a/scripts/layering/daemon-modularity.test.ts b/scripts/layering/daemon-modularity.test.ts index 981eed9f57..1a16ef7bf4 100644 --- a/scripts/layering/daemon-modularity.test.ts +++ b/scripts/layering/daemon-modularity.test.ts @@ -3,6 +3,7 @@ import { test } from 'node:test'; import { checkDaemonModularityRatchets, DAEMON_MODULARITY_BASELINE, + LOGICAL_MODULE_POLICIES, TYPE_CYCLE_BASELINE, } from './daemon-modularity.ts'; import { SESSION_STATE_FIELD_OWNERS } from './session-state.ts'; @@ -27,6 +28,20 @@ function baselineDaemonTypesEdges(): ResolvedImportEdge[] { ); } +function recordedMigrationEdges(): ResolvedImportEdge[] { + return LOGICAL_MODULE_POLICIES.flatMap((module) => + (module.recordedMigrationImports ?? []).map((recorded) => { + const [file, target] = recorded.split(' -> '); + return importEdge(file!, target!); + }), + ); +} + +/** Every recorded import present and nothing else forbidden: the quiet state of the ratchets. */ +function baselineEdges(): ResolvedImportEdge[] { + return [...baselineDaemonTypesEdges(), ...recordedMigrationEdges()]; +} + test('daemon modularity baseline records the measured R7 ownership pressure', () => { assert.equal( Object.keys(SESSION_STATE_FIELD_OWNERS).length, @@ -49,11 +64,14 @@ test('external daemon/types.ts importer membership changes require the baseline ]), ); - const violations = checkDaemonModularityRatchets([...baselineDaemonTypesEdges(), ...edges], []); + const violations = checkDaemonModularityRatchets([...baselineEdges(), ...edges], []); assert.equal(violations.length, 1); assert.match(violations[0]!.message, /may only shrink from the recorded 4/); - const removed = checkDaemonModularityRatchets(baselineDaemonTypesEdges().slice(1), []); + const removed = checkDaemonModularityRatchets( + [...baselineDaemonTypesEdges().slice(1), ...recordedMigrationEdges()], + [], + ); assert.equal(removed.length, 1); assert.match(removed[0]!.message, /delete it from externalDaemonTypesImporters/); }); @@ -66,11 +84,67 @@ test('planned logical modules start with zero forbidden imports', () => { ]), ); - const violations = checkDaemonModularityRatchets([...baselineDaemonTypesEdges(), ...edges], []); + const violations = checkDaemonModularityRatchets([...baselineEdges(), ...edges], []); assert.equal(violations.length, 1); assert.match(violations[0]!.message, /replay-test must not import/); }); +test('replay-test rejects request-global and engine-internal imports', () => { + const edges = resolveImportEdges( + new Map([ + [ + 'src/replay/test/scheduler.ts', + [ + "import { emitRequestProgress } from '../../request/progress.ts';", + "import { readReplayScriptMetadata } from '../script.ts';", + "import { parseMaestroProgram } from '../../compat/maestro/program-ir-parser.ts';", + ].join('\n'), + ], + ['src/request/progress.ts', 'export function emitRequestProgress() {}'], + ['src/replay/script.ts', 'export function readReplayScriptMetadata() {}'], + ['src/compat/maestro/program-ir-parser.ts', 'export function parseMaestroProgram() {}'], + ]), + ); + + const violations = checkDaemonModularityRatchets([...baselineEdges(), ...edges], []); + assert.deepEqual( + violations.map(({ message }) => message.replace(/;.*/, '')), + [ + 'replay-test must not import src/request/progress.ts', + 'replay-test must not import src/replay/script.ts', + 'replay-test must not import src/compat/maestro/program-ir-parser.ts', + ], + ); +}); + +test('replay-test may still import its own files inside the wider replay engine root', () => { + const edges = resolveImportEdges( + new Map([ + ['src/replay/test/reporting.ts', "import { spec } from './reporters/spec.ts';"], + ['src/replay/test/reporters/spec.ts', 'export const spec = 1;'], + ]), + ); + + assert.deepEqual(checkDaemonModularityRatchets([...baselineEdges(), ...edges], []), []); +}); + +test('recorded replay-test migration imports are exempt until the import is deleted', () => { + const recorded = LOGICAL_MODULE_POLICIES.find( + ({ name }) => name === 'replay-test', + )?.recordedMigrationImports; + assert.deepEqual(recorded, [ + 'src/replay/test/reporters/default.ts -> src/replay/divergence.ts', + 'src/replay/test/reporters/progress.ts -> src/request/progress.ts', + 'src/replay/test/reporters/registry.ts -> src/request/progress.ts', + 'src/replay/test/reporting.ts -> src/request/progress.ts', + ]); + assert.deepEqual(checkDaemonModularityRatchets(baselineEdges(), []), []); + + const withoutOne = checkDaemonModularityRatchets(baselineEdges().slice(0, -1), []); + assert.equal(withoutOne.length, 1); + assert.match(withoutOne[0]!.message, /delete it from replay-test's recordedMigrationImports/); +}); + test('internal trees reject deep imports globally, including from daemon', () => { const edges = resolveImportEdges( new Map([ @@ -79,7 +153,7 @@ test('internal trees reject deep imports globally, including from daemon', () => ]), ); - const violations = checkDaemonModularityRatchets([...baselineDaemonTypesEdges(), ...edges], []); + const violations = checkDaemonModularityRatchets([...baselineEdges(), ...edges], []); assert.equal(violations.length, 1); assert.match(violations[0]!.message, /must not import maestro's internal tree/); }); @@ -89,7 +163,7 @@ test('R9 records zone ceilings and keeps engine files outside the largest compon { length: DAEMON_MODULARITY_BASELINE.largestTypeCycle.zoneMembers.commands + 1 }, (_, index) => `src/commands/probe-${index}.ts`, ); - const violations = checkDaemonModularityRatchets(baselineDaemonTypesEdges(), [ + const violations = checkDaemonModularityRatchets(baselineEdges(), [ ...commandMembers, 'src/ad-replay/internal/engine.ts', ]); diff --git a/scripts/layering/daemon-modularity.ts b/scripts/layering/daemon-modularity.ts index 27e21a73fc..4d8060b162 100644 --- a/scripts/layering/daemon-modularity.ts +++ b/scripts/layering/daemon-modularity.ts @@ -36,6 +36,14 @@ type LogicalModulePolicy = { name: string; roots: readonly string[]; forbiddenTargetRoots: readonly string[]; + /** + * Imports that already violate `forbiddenTargetRoots` on the day the rule was written, recorded + * as `source -> target`. The rule enforces immediately for everything else, so a new violation + * cannot be added while the module waits for its extraction PR; each recorded edge must be + * deleted from this list by the change that removes the import, and re-adding one is a diff a + * reviewer sees. + */ + recordedMigrationImports?: readonly string[]; }; /** @@ -61,9 +69,28 @@ export const LOGICAL_MODULE_POLICIES: readonly LogicalModulePolicy[] = [ forbiddenTargetRoots: ['src/daemon/', 'src/platforms/', 'src/providers/', 'src/ad-replay/'], }, { + // Replay-test schedules and reports; it must stay format-neutral. `src/request/` is + // request-global daemon plumbing (progress sinks, cancellation, AsyncLocalStorage), and the + // remaining roots are engine internals — reaching into either is how a scheduler quietly + // acquires daemon authority or an engine-specific value shape. name: 'replay-test', roots: ['src/replay/test/'], - forbiddenTargetRoots: ['src/daemon/', 'src/platforms/', 'src/providers/'], + forbiddenTargetRoots: [ + 'src/daemon/', + 'src/platforms/', + 'src/providers/', + 'src/request/', + 'src/replay/', + 'src/compat/', + 'src/maestro/', + 'src/ad-replay/', + ], + recordedMigrationImports: [ + 'src/replay/test/reporters/default.ts -> src/replay/divergence.ts', + 'src/replay/test/reporters/progress.ts -> src/request/progress.ts', + 'src/replay/test/reporters/registry.ts -> src/request/progress.ts', + 'src/replay/test/reporting.ts -> src/request/progress.ts', + ], }, ]; @@ -185,6 +212,7 @@ function checkDaemonTypesImporters(edges: readonly ResolvedImportEdge[]): Layeri function checkLogicalModuleImports(edges: readonly ResolvedImportEdge[]): LayeringViolation[] { const violations: LayeringViolation[] = []; + const observedMigrationImports = new Set(); for (const edge of edges) { const sourceModule = moduleForFile(edge.file); const targetModule = moduleForFile(edge.target); @@ -203,14 +231,36 @@ function checkLogicalModuleImports(edges: readonly ResolvedImportEdge[]): Layeri } if (!sourceModule) continue; - if (sourceModule.forbiddenTargetRoots.some((root) => edge.target.startsWith(root))) { + // A module's own files are never a forbidden target: `replay-test` sits inside the wider + // `src/replay/` engine root it may not import from. + if (sourceModule.roots.some((root) => edge.target.startsWith(root))) continue; + if (!sourceModule.forbiddenTargetRoots.some((root) => edge.target.startsWith(root))) continue; + const migrationImport = `${edge.file} -> ${edge.target}`; + if (sourceModule.recordedMigrationImports?.includes(migrationImport)) { + observedMigrationImports.add(migrationImport); + continue; + } + violations.push({ + rule: 'R10 daemon-modularity', + file: edge.file, + line: edge.line, + message: `${sourceModule.name} must not import ${edge.target}; communicate through its façade and a narrow port with two real adapters.`, + }); + } + return [...violations, ...checkRecordedMigrationImports(observedMigrationImports)]; +} + +function checkRecordedMigrationImports(observed: ReadonlySet): LayeringViolation[] { + const violations: LayeringViolation[] = []; + for (const module of LOGICAL_MODULE_POLICIES) { + for (const migrationImport of module.recordedMigrationImports ?? []) { + if (observed.has(migrationImport)) continue; violations.push({ rule: 'R10 daemon-modularity', - file: edge.file, - line: edge.line, - message: `${sourceModule.name} must not import ${edge.target}; communicate through its façade and a narrow port with two real adapters.`, + file: 'scripts/layering/daemon-modularity.ts', + line: 1, + message: `${migrationImport} no longer exists — delete it from ${module.name}'s recordedMigrationImports in the same change so the import cannot return.`, }); - continue; } } return violations; @@ -237,10 +287,14 @@ function countBy(values: readonly string[], keyOf: (value: string) => string): M export function daemonModularitySummary(): string { const session = DAEMON_MODULARITY_BASELINE.sessionState; + const recordedMigrationImports = LOGICAL_MODULE_POLICIES.reduce( + (sum, module) => sum + (module.recordedMigrationImports?.length ?? 0), + 0, + ); return ( `R10 pins R7 at ${session.writerOwnedFields} writer-owned fields / ` + `${session.ownerFileClaims} owner claims, R9 at ${TYPE_CYCLE_BASELINE} files with zone ceilings, ` + `${DAEMON_MODULARITY_BASELINE.externalDaemonTypesImporters.length} external daemon/types.ts importers, ` + - 'and zero forbidden logical-module imports' + `and zero forbidden logical-module imports beyond ${recordedMigrationImports} recorded migration import(s)` ); } diff --git a/src/daemon/handlers/__tests__/session-test-reporter-values.test.ts b/src/daemon/handlers/__tests__/session-test-reporter-values.test.ts new file mode 100644 index 0000000000..8ed08e7ceb --- /dev/null +++ b/src/daemon/handlers/__tests__/session-test-reporter-values.test.ts @@ -0,0 +1,365 @@ +// Characterization of the shipped reporter contract across the scheduler seam (#1478 P3). +// +// P3 moves the scheduler into `packages/replay-test` and makes attempt identity +// scheduler-owned, mapped back to daemon session names by the daemon adapter. The values a +// custom reporter observes today are therefore the thing most at risk of silent drift, and +// `session` most of all: it is not one value with one provenance, it is four, and they +// disagree on purpose. These tests drive the real suite handler through the real reporter +// registry and pin what a reporter sees. They document today's behavior; they do not +// propose behavior. + +// The same reason session-test-suite.test.ts mocks it: ADR 0012 attempts a post-failure +// screen digest via dispatchCommand('snapshot', ...), and these fixtures model no runner. +import { expect, test, vi } from 'vitest'; + +vi.mock('../../../core/dispatch.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + dispatchCommand: vi.fn(async () => { + throw new Error('no device runner available in this test'); + }), + }; +}); + +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import type { ReplaySuiteResult } from '@agent-device/contracts/replay'; +import { handleSessionCommands } from '../session.ts'; +import { SessionStore } from '../../session-store.ts'; +import type { DaemonRequest, DaemonResponse } from '../../types.ts'; +import { withRequestProgressSink } from '../../../request/progress.ts'; +import { withDeviceInventoryProvider } from '../../../core/dispatch-resolve.ts'; +import type { DeviceInfo } from '@agent-device/kernel/device'; +import { + getReplayTestReporterExitCode, + runReplayTestReporterProgress, + runReplayTestReporters, +} from '../../../replay/test/reporters/registry.ts'; +import type { + ReplayTestReporter, + ReplayTestReporterContext, +} from '../../../replay/test/reporters/types.ts'; + +type RecordedHook = { + hook: 'onSuiteStart' | 'onTestStart' | 'onTestStep' | 'onTestResult' | 'onSuiteEnd'; + value: Record; +}; + +const ANDROID_ONE: DeviceInfo = { + platform: 'android', + id: 'emulator-5554', + name: 'Pixel 8', + kind: 'emulator', + booted: true, +}; + +const ANDROID_TWO: DeviceInfo = { + platform: 'android', + id: 'emulator-5556', + name: 'Pixel 8 Pro', + kind: 'emulator', + booted: true, +}; + +const reporterContext: ReplayTestReporterContext = { + stdout: { isTTY: false, write() {} }, + stderr: { isTTY: false, write() {} }, +}; + +function createRecordingReporter(): { reporter: ReplayTestReporter; hooks: RecordedHook[] } { + const hooks: RecordedHook[] = []; + return { + hooks, + reporter: { + name: 'recording', + onSuiteStart: (suite) => void hooks.push({ hook: 'onSuiteStart', value: { ...suite } }), + onTestStart: (testCase) => void hooks.push({ hook: 'onTestStart', value: { ...testCase } }), + onTestStep: (testCase) => void hooks.push({ hook: 'onTestStep', value: { ...testCase } }), + onTestResult: (testCase) => void hooks.push({ hook: 'onTestResult', value: { ...testCase } }), + onSuiteEnd: (suite) => void hooks.push({ hook: 'onSuiteEnd', value: { ...suite } }), + getExitCode: (suite) => (suite.failed > 0 ? 1 : 0), + }, + }; +} + +function makeSessionStore(): SessionStore { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-reporter-values-')); + return new SessionStore(path.join(root, 'sessions')); +} + +/** + * Runs `test` exactly as the daemon does, feeding every progress event through the shipped + * reporter registry — the same translation and dispatch a `--reporter ./mine.mjs` module gets. + */ +async function runSuiteThroughReporter(params: { + root: string; + requestId: string; + flags?: DaemonRequest['flags']; + invoke: (req: DaemonRequest) => Promise; + devices?: DeviceInfo[]; +}): Promise<{ hooks: RecordedHook[]; suite: ReplaySuiteResult; exitCode: number }> { + const { reporter, hooks } = createRecordingReporter(); + const reporters = [reporter]; + const call = async () => + await withRequestProgressSink( + (event) => runReplayTestReporterProgress(reporters, event, reporterContext), + async () => + await handleSessionCommands({ + req: { + token: 't', + session: 'default', + command: 'test', + positionals: [params.root], + meta: { cwd: params.root, requestId: params.requestId }, + flags: params.flags, + }, + sessionName: 'default', + logPath: path.join(os.tmpdir(), 'daemon.log'), + sessionStore: makeSessionStore(), + invoke: params.invoke, + }), + ); + + const devices = params.devices; + const response = devices + ? await withDeviceInventoryProvider(async () => devices, call) + : await call(); + + expect(response?.ok, JSON.stringify(response)).toBeTruthy(); + if (!response?.ok) throw new Error('Expected a successful suite response.'); + const suite = response.data as unknown as ReplaySuiteResult; + await runReplayTestReporters(reporters, suite, reporterContext); + return { hooks, suite, exitCode: getReplayTestReporterExitCode(reporters, suite) }; +} + +/** + * One skipped entry (no `context platform=`, filtered out by `--platform android`) followed by + * one runnable entry that fails its first attempt and passes its retry. Every reporter value a + * suite can produce shows up once. + */ +async function runRetrySuite(): Promise<{ + root: string; + hooks: RecordedHook[]; + suite: ReplaySuiteResult; + exitCode: number; +}> { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-reporter-session-')); + fs.writeFileSync(path.join(root, '01-untyped.ad'), 'open "Demo"\n'); + fs.writeFileSync( + path.join(root, '02-retry.ad'), + 'context platform=android retries=1\nopen "Demo"\n', + ); + + let attempts = 0; + const run = await runSuiteThroughReporter({ + root, + requestId: 'suite-reporter', + flags: { platform: 'android' }, + invoke: async () => { + attempts += 1; + return attempts === 1 + ? { ok: false, error: { code: 'COMMAND_FAILED', message: 'first attempt failed' } } + : { ok: true, data: { replayed: 1, healed: 0 } }; + }, + }); + return { root, ...run }; +} + +function hookValue( + hooks: RecordedHook[], + index: number, + hook: RecordedHook['hook'], +): Record { + const entry = hooks[index]; + expect(entry, `no hook recorded at ${index}`).toBeDefined(); + expect(entry?.hook).toBe(hook); + return entry?.value ?? {}; +} + +const RETRY_SUITE_HOOKS = [ + 'onSuiteStart', + 'onTestResult', + 'onTestStart', + 'onTestStep', + 'onTestResult', + 'onTestStep', + 'onTestResult', + 'onSuiteEnd', +] as const; + +test('a reporter sees the shipped suite-start, skip, and test-start values', async () => { + const { root, hooks } = await runRetrySuite(); + + expect(hooks.map((entry) => entry.hook)).toEqual([...RETRY_SUITE_HOOKS]); + + const suiteArtifactsDir = path.join(root, '.agent-device', 'test-artifacts', 'suite-reporter'); + expect(hookValue(hooks, 0, 'onSuiteStart')).toEqual({ + total: 2, + runnable: 1, + skipped: 1, + artifactsDir: suiteArtifactsDir, + shardMode: undefined, + shardCount: undefined, + }); + + // A skipped entry never opens a session, so the reporter gets no session and no duration. + expect(hookValue(hooks, 1, 'onTestResult')).toMatchObject({ + file: path.join(root, '01-untyped.ad'), + status: 'skip', + index: 1, + total: 2, + session: undefined, + artifactsDir: undefined, + durationMs: undefined, + message: 'missing platform metadata for --platform android', + }); + + // The start value is emitted once per test case, BEFORE any attempt runs, so its `session` + // is `buildReplayTestSessionName(..., attemptIndex = 0)`: always `attempt-1`, even when the + // case ends up passing on attempt 2. `attempt` is absent while `maxAttempts` is present. + // Its test number (`1`) is the runnable ordinal; `index` (`2`) is the discovery ordinal. + expect(hookValue(hooks, 2, 'onTestStart')).toEqual({ + file: path.join(root, '02-retry.ad'), + title: undefined, + index: 2, + total: 2, + attempt: undefined, + maxAttempts: 2, + session: 'default:test:suite-reporter:1-02-retry:attempt-1', + artifactsDir: path.join(suiteArtifactsDir, '02-retry.ad'), + shardIndex: undefined, + shardCount: undefined, + deviceId: undefined, + deviceName: undefined, + }); +}); + +test('reporter step and result sessions track the running attempt, not the start value', async () => { + const { root, hooks, suite, exitCode } = await runRetrySuite(); + const testArtifactsDir = path.join( + root, + '.agent-device', + 'test-artifacts', + 'suite-reporter', + '02-retry.ad', + ); + + // Step values come from the per-attempt AsyncLocalStorage context, so they track the + // running attempt: attempt-1 then attempt-2. They carry no `status` key at all. + const firstStep = hookValue(hooks, 3, 'onTestStep'); + expect('status' in firstStep).toBe(false); + expect(firstStep).toMatchObject({ + attempt: 1, + maxAttempts: 2, + session: 'default:test:suite-reporter:1-02-retry:attempt-1', + artifactsDir: testArtifactsDir, + stepIndex: 1, + stepTotal: 1, + stepCommand: 'open', + stepValue: 'Demo', + }); + expect(hookValue(hooks, 5, 'onTestStep')).toMatchObject({ + attempt: 2, + session: 'default:test:suite-reporter:1-02-retry:attempt-2', + stepIndex: 1, + stepCommand: 'open', + }); + + // The retry result carries the failed attempt's session; the final result carries the last + // attempt's session. Neither equals the start value once a retry happened. + expect(hookValue(hooks, 4, 'onTestResult')).toMatchObject({ + status: 'fail', + attempt: 1, + maxAttempts: 2, + retrying: true, + session: 'default:test:suite-reporter:1-02-retry:attempt-1', + message: 'Replay failed at step 1 (open "Demo"): first attempt failed', + }); + const finalResult = hookValue(hooks, 6, 'onTestResult'); + expect(finalResult).toMatchObject({ + status: 'pass', + attempt: 2, + maxAttempts: 2, + retrying: undefined, + session: 'default:test:suite-reporter:1-02-retry:attempt-2', + artifactsDir: testArtifactsDir, + }); + expect(finalResult.durationMs).toEqual(expect.any(Number)); + + // onSuiteEnd receives the awaited suite result; its per-test `session` is the final + // attempt's session, matching the last result value rather than the start value. + expect(hookValue(hooks, 7, 'onSuiteEnd')).toEqual(suite); + expect(suite.tests.map((entry) => ('session' in entry ? entry.session : undefined))).toEqual([ + undefined, + 'default:test:suite-reporter:1-02-retry:attempt-2', + ]); + expect(exitCode).toBe(0); +}); + +test('a failing suite reaches the reporter with the failure message, hint fields, and exit code', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-reporter-fail-')); + fs.writeFileSync(path.join(root, '01-fail.ad'), 'context platform=android\nopen "Demo"\n'); + + const { hooks, suite, exitCode } = await runSuiteThroughReporter({ + root, + requestId: 'suite-reporter-fail', + invoke: async () => ({ + ok: false, + error: { code: 'ASSERTION_FAILED', message: 'selector not found', hint: 'try replay --from' }, + }), + }); + + const results = hooks.filter((entry) => entry.hook === 'onTestResult'); + expect(results).toHaveLength(1); + expect(hookValue(results, 0, 'onTestResult')).toMatchObject({ + status: 'fail', + attempt: 1, + maxAttempts: 1, + retrying: undefined, + session: 'default:test:suite-reporter-fail:1-01-fail:attempt-1', + message: 'Replay failed at step 1 (open "Demo"): selector not found', + }); + expect(suite.failed).toBe(1); + expect(exitCode).toBe(1); +}); + +test('sharded runs give the reporter shard-scoped sessions and device identity', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-reporter-shard-')); + fs.writeFileSync(path.join(root, '01-login.ad'), 'context platform=android\nopen "Demo"\n'); + + const { hooks } = await runSuiteThroughReporter({ + root, + requestId: 'suite-reporter-shard', + flags: { + platform: 'android', + device: 'emulator-5554,emulator-5556', + shardAll: 2, + }, + devices: [ANDROID_ONE, ANDROID_TWO], + invoke: async () => ({ ok: true, data: { replayed: 1, healed: 0 } }), + }); + + // Shipped asymmetry: `total` is shard-multiplied (1 file x 2 shards) while `runnable` and + // `skipped` are counted before sharding, so a sharded suite-start value reports + // total 2 / runnable 1. Pinned as-is; the extraction must not "fix" it silently. + expect(hookValue(hooks, 0, 'onSuiteStart')).toEqual({ + total: 2, + runnable: 1, + skipped: 0, + artifactsDir: path.join(root, '.agent-device', 'test-artifacts', 'suite-reporter-shard'), + shardMode: 'all', + shardCount: 2, + }); + + const starts = hooks.filter((entry) => entry.hook === 'onTestStart'); + expect(starts.map((entry) => entry.value.session)).toEqual([ + 'default:shard-1:test:suite-reporter-shard:1-01-login:attempt-1', + 'default:shard-2:test:suite-reporter-shard:1-01-login:attempt-1', + ]); + expect(starts.map((entry) => entry.value.shardIndex)).toEqual([0, 1]); + expect(starts.map((entry) => entry.value.shardCount)).toEqual([2, 2]); + expect(starts.map((entry) => entry.value.deviceId)).toEqual(['emulator-5554', 'emulator-5556']); + expect(starts.map((entry) => entry.value.deviceName)).toEqual(['Pixel 8', 'Pixel 8 Pro']); +}); diff --git a/src/daemon/handlers/__tests__/session-test-runtime.test.ts b/src/daemon/handlers/__tests__/session-test-runtime.test.ts index f689847a0c..a1f09e7ac3 100644 --- a/src/daemon/handlers/__tests__/session-test-runtime.test.ts +++ b/src/daemon/handlers/__tests__/session-test-runtime.test.ts @@ -1,3 +1,6 @@ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; import { afterEach, expect, test, vi } from 'vitest'; import type { DaemonResponse } from '../../types.ts'; import { isRequestCanceled } from '../../../request/cancel.ts'; @@ -7,6 +10,18 @@ afterEach(() => { vi.useRealTimers(); }); +function makeArtifactsDir(label: string): string { + return fs.mkdtempSync(path.join(os.tmpdir(), `agent-device-test-runtime-${label}-`)); +} + +function readTimingEventTypes(artifactsDir: string): string[] { + const trace = fs.readFileSync(path.join(artifactsDir, 'replay-timing.ndjson'), 'utf8'); + return trace + .split('\n') + .filter(Boolean) + .map((line) => String((JSON.parse(line) as { type: unknown }).type)); +} + test('runReplayTestAttempt keeps cancellation active until a timed-out replay settles', async () => { vi.useFakeTimers(); @@ -18,6 +33,8 @@ test('runReplayTestAttempt keeps cancellation active until a timed-out replay se const lifecycleEvents: string[] = []; const cleanupSession = vi.fn(async () => { lifecycleEvents.push('cleanup'); + // The deferred late cleanup is best-effort: its failure is diagnosed, never surfaced. + if (cleanupSession.mock.calls.length > 1) throw new Error('late cleanup failed'); }); const finalizeAttempt = vi.fn(async () => { lifecycleEvents.push('finalize'); @@ -53,6 +70,10 @@ test('runReplayTestAttempt keeps cancellation active until a timed-out replay se ); expect(lifecycleEvents).toEqual(['finalize', 'cleanup']); expect(isRequestCanceled('req-timeout-open')).toBe(true); + // #1478 P3: the second cleanup is strictly deferred until the abandoned replay settles, so + // the attempt returns having cleaned up exactly once. P3 keeps this orchestration inside + // replay-test while the cleanup effect itself stays in the daemon adapter. + expect(cleanupSession).toHaveBeenCalledTimes(1); resolveReplay?.({ ok: false, @@ -65,6 +86,8 @@ test('runReplayTestAttempt keeps cancellation active until a timed-out replay se await vi.waitFor(() => { expect(cleanupSession).toHaveBeenCalledTimes(2); }); + expect(lifecycleEvents).toEqual(['finalize', 'cleanup', 'cleanup']); + await expect(attemptPromise).resolves.toBe(result); }); test('runReplayTestAttempt keeps a passing replay passed when finalization fails', async () => { @@ -89,3 +112,109 @@ test('runReplayTestAttempt keeps a passing replay passed when finalization fails ]); expect(cleanupSession).toHaveBeenCalledWith('default:test:pass'); }); + +// #1478 P3 characterization: attempt finalization happens before cleanup, always, and the +// timing trace is the durable evidence of that order. P3 moves this `finally` orchestration +// into replay-test, so the order and the recorded event names are pinned here as shipped. +test('runReplayTestAttempt finalizes before cleanup and records that order in the timing trace', async () => { + const artifactsDir = makeArtifactsDir('order'); + const lifecycleEvents: string[] = []; + + const result = await runReplayTestAttempt({ + filePath: '01-order.ad', + sessionName: 'default:test:order', + requestId: 'req-order', + artifactsDir, + runReplay: async () => { + lifecycleEvents.push('replay'); + return { ok: true, data: { replayed: 1, healed: 0 } }; + }, + finalizeAttempt: async () => { + lifecycleEvents.push('finalize'); + return undefined; + }, + cleanupSession: async () => { + lifecycleEvents.push('cleanup'); + }, + }); + + expect(result.ok).toBe(true); + expect(lifecycleEvents).toEqual(['replay', 'finalize', 'cleanup']); + expect(readTimingEventTypes(artifactsDir)).toEqual([ + 'replay_test_attempt_start', + 'replay_test_attempt_stop', + 'replay_test_finalize_start', + 'replay_test_finalize_stop', + 'replay_test_cleanup_start', + 'replay_test_cleanup_stop', + ]); +}); + +test('runReplayTestAttempt cleans up once when a timed-out replay settles inside the grace window', async () => { + vi.useFakeTimers(); + const artifactsDir = makeArtifactsDir('grace'); + + let resolveReplay: ((response: DaemonResponse) => void) | undefined; + const replayPromise = new Promise((resolve) => { + resolveReplay = resolve; + }); + const lifecycleEvents: string[] = []; + const cleanupSession = vi.fn(async () => { + lifecycleEvents.push('cleanup'); + }); + + const attemptPromise = runReplayTestAttempt({ + filePath: '01-late.ad', + sessionName: 'default:test:late', + requestId: 'req-timeout-grace', + timeoutMs: 10, + artifactsDir, + runReplay: async () => await replayPromise, + finalizeAttempt: async () => { + lifecycleEvents.push('finalize'); + return undefined; + }, + cleanupSession, + }); + + await vi.advanceTimersByTimeAsync(10); + // The replay comes back inside the 2s grace window, so no cleanup race is declared. + resolveReplay?.({ ok: true, data: { replayed: 1, healed: 0 } }); + + const result = await attemptPromise; + // The raced timeout response still wins: a late success does not un-fail the attempt. + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error.message).toBe('TIMEOUT after 10ms'); + expect(result.error.details?.reason).toBe('timeout'); + expect(result.error.details?.timeoutCleanupPending).toBe(undefined); + expect(result.error.details?.timeoutMode).toBe('cooperative'); + } + expect(lifecycleEvents).toEqual(['finalize', 'cleanup']); + expect(cleanupSession).toHaveBeenCalledTimes(1); + expect(readTimingEventTypes(artifactsDir)).toEqual([ + 'replay_test_attempt_start', + 'replay_test_attempt_stop', + 'replay_test_finalize_start', + 'replay_test_finalize_stop', + 'replay_test_cleanup_start', + 'replay_test_cleanup_stop', + ]); +}); + +test('runReplayTestAttempt cleans up without a finalizer and adds no finalization warning', async () => { + const cleanupSession = vi.fn(async () => {}); + + const result = await runReplayTestAttempt({ + filePath: '01-no-finalizer.ad', + sessionName: 'default:test:no-finalizer', + requestId: 'req-no-finalizer', + runReplay: async () => ({ ok: true, data: { replayed: 1, healed: 0 } }), + cleanupSession, + }); + + expect(result.ok).toBe(true); + if (!result.ok) throw new Error(result.error.message); + expect(result.data?.warnings).toBe(undefined); + expect(cleanupSession).toHaveBeenCalledWith('default:test:no-finalizer'); +}); diff --git a/src/replay/test/reporters/__tests__/custom.test.ts b/src/replay/test/reporters/__tests__/custom.test.ts index 5cd52f2b4b..bdbc339ba1 100644 --- a/src/replay/test/reporters/__tests__/custom.test.ts +++ b/src/replay/test/reporters/__tests__/custom.test.ts @@ -42,6 +42,76 @@ test.each([ } }); +// #1478 P3 characterization: the export spellings and hook names below are the released +// public surface a shipped custom reporter is written against. The extraction preserves them +// exactly, so they are pinned by name rather than by structural inference. +test('prefers createReporter over default and reporter when a module exports several', async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'agent-device-reporter-precedence-')); + const modulePath = path.join(root, 'reporter.mjs'); + try { + await fs.writeFile( + modulePath, + [ + "export const reporter = { name: 'named' };", + "export default { name: 'default' };", + "export function createReporter() { return { name: 'created' }; }", + ].join('\n'), + 'utf8', + ); + const created = await createCustomReplayTestReporter({ + kind: 'custom', + modulePath, + raw: modulePath, + }); + assert.equal(created.name, 'created'); + + const fallbackPath = path.join(root, 'fallback.mjs'); + await fs.writeFile( + fallbackPath, + ["export const reporter = { name: 'named' };", "export default { name: 'default' };"].join( + '\n', + ), + 'utf8', + ); + const fallback = await createCustomReplayTestReporter({ + kind: 'custom', + modulePath: fallbackPath, + raw: fallbackPath, + }); + assert.equal(fallback.name, 'default'); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } +}); + +test.each([ + 'onSuiteStart', + 'onTestStart', + 'onTestStep', + 'onTestResult', + 'onSuiteEnd', + 'getExitCode', +])('validates the shipped optional hook name %s', async (hook) => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'agent-device-reporter-hook-')); + const modulePath = path.join(root, 'reporter.mjs'); + try { + await fs.writeFile( + modulePath, + `export default { name: 'hooks', ${hook}: 'not-a-function' };`, + 'utf8', + ); + await assert.rejects( + createCustomReplayTestReporter({ kind: 'custom', modulePath, raw: modulePath }), + (error: unknown) => + error instanceof AppError && + error.code === 'INVALID_ARGS' && + error.message.includes(`${hook} must be a function`), + ); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } +}); + test('rejects malformed reporter hooks when the module is loaded', async () => { const root = await fs.mkdtemp(path.join(os.tmpdir(), 'agent-device-reporter-invalid-')); const modulePath = path.join(root, 'reporter.mjs'); diff --git a/src/replay/test/reporters/__tests__/progress.test.ts b/src/replay/test/reporters/__tests__/progress.test.ts new file mode 100644 index 0000000000..0916c72759 --- /dev/null +++ b/src/replay/test/reporters/__tests__/progress.test.ts @@ -0,0 +1,223 @@ +// Characterization of the shipped reporter value contract (#1478 P3). +// +// A custom reporter never sees a `RequestProgressEvent`; it sees the value this module +// projects. P3 moves that projection behind a package façade, so the projection's field +// set, key presence, and verbatim pass-through are pinned here as shipped behavior — not +// as a proposal. Reporters in the wild read `test.session` and probe optional keys with +// `in`, so a key that exists today with an `undefined` value is part of the contract. +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import type { RequestProgressEvent } from '../../../../request/progress.ts'; +import { toReplayTestReporterProgressEvent } from '../progress.ts'; + +const SESSION = 'default:test:req-7:1-checkout:attempt-2'; + +const TEST_CASE_KEYS = [ + 'file', + 'title', + 'index', + 'total', + 'attempt', + 'maxAttempts', + 'session', + 'artifactsDir', + 'shardIndex', + 'shardCount', + 'deviceId', + 'deviceName', +]; + +test('command progress never reaches a reporter hook', () => { + assert.equal( + toReplayTestReporterProgressEvent({ + type: 'command', + status: 'progress', + message: 'installing app', + }), + undefined, + ); +}); + +test('suite progress becomes the onSuiteStart value with exactly the shipped fields', () => { + const event = toReplayTestReporterProgressEvent({ + type: 'replay-test-suite', + status: 'start', + total: 5, + runnable: 4, + skipped: 1, + artifactsDir: '/artifacts/req-7', + shardMode: 'split', + shardCount: 2, + }); + + assert.deepEqual(event, { + type: 'suite-start', + suite: { + total: 5, + runnable: 4, + skipped: 1, + artifactsDir: '/artifacts/req-7', + shardMode: 'split', + shardCount: 2, + }, + }); +}); + +test('an unsharded suite start still carries the shard keys as undefined', () => { + const event = toReplayTestReporterProgressEvent({ + type: 'replay-test-suite', + status: 'start', + total: 1, + runnable: 1, + skipped: 0, + artifactsDir: '/artifacts/req-7', + }); + + assert.equal(event?.type, 'suite-start'); + assert.deepEqual(Object.keys(event.suite), [ + 'total', + 'runnable', + 'skipped', + 'artifactsDir', + 'shardMode', + 'shardCount', + ]); + assert.equal(event.suite.shardMode, undefined); + assert.equal(event.suite.shardCount, undefined); +}); + +test('start progress becomes onTestStart and passes session through verbatim', () => { + const event = toReplayTestReporterProgressEvent({ + type: 'replay-test', + file: '/suite/01-checkout.ad', + title: 'Checkout', + status: 'start', + index: 1, + total: 2, + maxAttempts: 3, + session: SESSION, + artifactsDir: '/artifacts/req-7/01-checkout.ad', + shardIndex: 0, + shardCount: 2, + deviceId: 'emulator-5554', + deviceName: 'Pixel 8', + }); + + assert.deepEqual(event, { + type: 'test-start', + test: { + file: '/suite/01-checkout.ad', + title: 'Checkout', + index: 1, + total: 2, + attempt: undefined, + maxAttempts: 3, + session: SESSION, + artifactsDir: '/artifacts/req-7/01-checkout.ad', + shardIndex: 0, + shardCount: 2, + deviceId: 'emulator-5554', + deviceName: 'Pixel 8', + }, + }); +}); + +test('a minimal test event still carries every case key, so `in` checks keep working', () => { + const event = toReplayTestReporterProgressEvent({ + type: 'replay-test', + file: '/suite/01-checkout.ad', + status: 'start', + index: 1, + total: 1, + }); + + assert.equal(event?.type, 'test-start'); + assert.deepEqual(Object.keys(event.test), TEST_CASE_KEYS); + assert.equal('session' in event.test, true); + assert.equal(event.test.session, undefined); +}); + +test('step progress becomes onTestStep with the step fields appended after the case fields', () => { + const event = toReplayTestReporterProgressEvent({ + type: 'replay-test', + file: '/suite/01-checkout.ad', + status: 'progress', + index: 1, + total: 1, + attempt: 2, + maxAttempts: 3, + session: SESSION, + stepIndex: 3, + stepTotal: 9, + stepCommand: 'tap', + stepValue: 'Submit', + }); + + assert.equal(event?.type, 'test-step'); + assert.deepEqual(Object.keys(event.test), [ + ...TEST_CASE_KEYS, + 'stepIndex', + 'stepTotal', + 'stepCommand', + 'stepValue', + ]); + assert.equal(event.test.session, SESSION); + assert.equal(event.test.attempt, 2); + assert.equal(event.test.stepIndex, 3); + assert.equal(event.test.stepCommand, 'tap'); + assert.equal(event.test.stepValue, 'Submit'); +}); + +test.each(['pass', 'fail', 'skip'] as const)( + '%s progress becomes onTestResult with the status carried through', + (status) => { + const event = toReplayTestReporterProgressEvent({ + type: 'replay-test', + file: '/suite/01-checkout.ad', + status, + index: 1, + total: 1, + attempt: 2, + maxAttempts: 3, + durationMs: 1234, + retrying: status === 'fail', + message: 'selector not found', + hint: 'run replay --from', + session: SESSION, + }); + + assert.equal(event?.type, 'test-result'); + assert.deepEqual(Object.keys(event.test), [ + ...TEST_CASE_KEYS, + 'status', + 'durationMs', + 'retrying', + 'message', + 'hint', + ]); + assert.equal(event.test.status, status); + assert.equal(event.test.durationMs, 1234); + assert.equal(event.test.retrying, status === 'fail'); + assert.equal(event.test.message, 'selector not found'); + assert.equal(event.test.hint, 'run replay --from'); + assert.equal(event.test.session, SESSION); + }, +); + +test('a skipped entry reaches onTestResult without a session', () => { + const skipEvent: RequestProgressEvent = { + type: 'replay-test', + file: '/suite/02-ios-only.ad', + status: 'skip', + index: 2, + total: 2, + message: 'missing platform metadata for --platform android', + }; + + const event = toReplayTestReporterProgressEvent(skipEvent); + + assert.equal(event?.type, 'test-result'); + assert.equal(event.test.status, 'skip'); + assert.equal(event.test.session, undefined); + assert.equal(event.test.durationMs, undefined); +}); diff --git a/src/replay/test/reporters/__tests__/registry.test.ts b/src/replay/test/reporters/__tests__/registry.test.ts index ea41dc2b63..25bec8c311 100644 --- a/src/replay/test/reporters/__tests__/registry.test.ts +++ b/src/replay/test/reporters/__tests__/registry.test.ts @@ -190,6 +190,55 @@ test('reports rejected live hooks and never awaits pending live promises', async resolvePending(); }); +// #1478 P3 characterization: live hooks are isolated (above), final hooks are not. A throwing +// `onSuiteEnd` propagates out of suite completion and skips the reporters after it. Pinned as +// shipped so the extraction cannot quietly start swallowing it. +test('a throwing onSuiteEnd propagates and stops later reporters from finishing', async () => { + const calls: string[] = []; + const reporters: ReplayTestReporter[] = [ + { + name: 'throwing-end', + onSuiteEnd() { + calls.push('throwing-end'); + throw new Error('final boom'); + }, + }, + { + name: 'later', + onSuiteEnd() { + calls.push('later:end'); + }, + }, + ]; + + await assert.rejects(runReplayTestReporters(reporters, suite(), context), /final boom/); + assert.deepEqual(calls, ['throwing-end']); +}); + +test('getExitCode sees the same suite value onSuiteEnd received', async () => { + const seen: ReplaySuiteResult[] = []; + const value = suite(2); + const reporters: ReplayTestReporter[] = [ + { + name: 'observer', + onSuiteEnd: (endSuite) => void seen.push(endSuite), + getExitCode: (exitSuite) => { + seen.push(exitSuite); + return undefined; + }, + }, + ]; + + await runReplayTestReporters(reporters, value, context); + const exitCode = getReplayTestReporterExitCode(reporters, value); + + assert.equal(seen.length, 2); + assert.equal(seen[0], value); + assert.equal(seen[1], value); + // Two failures still recommend the shipped generic failure exit code, not a failure count. + assert.equal(exitCode, 1); +}); + test('reporter exit codes can raise but never lower the suite exit code', () => { const reporters: ReplayTestReporter[] = [ { name: 'lower', getExitCode: () => 0 }, From c88d306824f3914904f89e53e0d7b8061cd6c2aa Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 16:47:56 +0000 Subject: [PATCH 2/2] test(daemon): assert reporter hint on the real onTestResult value 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 Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 --- .../handlers/__tests__/session-test-reporter-values.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/daemon/handlers/__tests__/session-test-reporter-values.test.ts b/src/daemon/handlers/__tests__/session-test-reporter-values.test.ts index 8ed08e7ceb..eec9e2628e 100644 --- a/src/daemon/handlers/__tests__/session-test-reporter-values.test.ts +++ b/src/daemon/handlers/__tests__/session-test-reporter-values.test.ts @@ -320,6 +320,10 @@ test('a failing suite reaches the reporter with the failure message, hint fields retrying: undefined, session: 'default:test:suite-reporter-fail:1-01-fail:attempt-1', message: 'Replay failed at step 1 (open "Demo"): selector not found', + // `hint` is a shipped reporter field: assert it on the real hook value, not + // just as test input, so dropping `hint: error.hint` from the scheduler + // path cannot pass this ratchet. + hint: 'try replay --from', }); expect(suite.failed).toBe(1); expect(exitCode).toBe(1);