From 82696c278c279432be923edd302435c6ee6a94c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 13 Sep 2026 20:44:27 +0200 Subject: [PATCH 1/4] fix(replay): carry optional-skip warnings to every human surface and name system surfaces in refusals An optional: true Maestro step that skipped stayed a warning that the terminal never printed once a later step failed: the accumulated channel rode only on the success response, so the repro run in #2560 reported a bare failure with no trace of the skip. The engine now emits warnings on the failure event, the daemon copies them onto the failure response's error details, and the failed test result carries the same array the passed result already had; replay prints them through its facet formatter and the test reporter adds a Warnings section that renders whether tests pass or fail. A regular iOS presentation refusal from the runner now also carries the registry identity of the system surface the tree was acquired from in error details, so a capture-boundary miss under optional: true stops looking like a selector problem; the sparse-declared verdict path is owned by #2572. --- CHANGELOG.md | 13 ++++ packages/contracts/src/replay.ts | 2 + .../src/internal/__tests__/engine.test.ts | 56 ++++++++++++++ packages/maestro/src/internal/engine-types.ts | 2 + .../maestro/src/internal/facade-execution.ts | 3 + .../src/internal/replay-plan-execution.ts | 1 + .../internal/replay-plan-step-execution.ts | 9 ++- .../__tests__/snapshot-presentation.test.ts | 66 +++++++++++++++- .../src/runner/snapshot-presentation.ts | 22 ++++-- .../src/internal/session-test-attempt.ts | 3 + .../src/internal/session-test-types.ts | 2 + src/cli/commands/__tests__/generic.test.ts | 42 ++++++++++ .../__tests__/reporters-default.test.ts | 77 +++++++++++++++++++ src/cli/replay-test/reporters/default.ts | 22 ++++++ src/cli/replay-test/reporters/format.ts | 5 +- src/commands/output-common.test.ts | 44 +++++++++++ src/commands/output-common.ts | 32 +++++--- src/commands/replay/index.ts | 5 ++ .../session-replay-maestro-failure.test.ts | 39 +++++++++- .../__tests__/session-test-outcome.test.ts | 53 +++++++++++++ .../session-replay-maestro-failure.ts | 1 + ...session-replay-runtime-failure-response.ts | 9 +++ .../replay/internal/session-test-outcome.ts | 2 + website/docs/docs/replay-e2e.md | 1 + 24 files changed, 489 insertions(+), 22 deletions(-) create mode 100644 src/commands/output-common.test.ts create mode 100644 src/daemon/replay/internal/__tests__/session-test-outcome.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index b66724c5eb..ccd1888b73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -139,6 +139,19 @@ exactly as before, and a payload the runner did not declare sparse keeps the plain invariant byte for byte. +- Fixed: skipped `optional: true` Maestro steps are no longer invisible on the human surface + (#2560). The warning a skip leaves now travels with the run whether it later passes or fails: + `replay` prints a `Warning:` line after its summary and repeats the run's warnings after a + failed run's error, `test` prints a `Warnings:` section after the suite summary naming each + test, and a failed test result gained the `warnings` array the passing result already had, so + `--json` and JUnit carry the skipped steps of a failing test too. +- Fixed: an Apple runner presentation refusal now carries the registry identity of the system + surface the tree was acquired from as `error.details.systemSurface` (#2560). Previously a + selector-backed command failing at the capture boundary under `optional: true` gave nothing + naming that boundary — the web sign-in sheet out of `SafariViewService` was invisible in the + error, and the miss looked like a selector problem inside the app. The sparse-declared case + already names the surface host in its hint via #2572; this covers the provenance everywhere + the runner reports one. - Changed: a capture that a backend cut at one of its limits now says so in the snapshot's warnings, on every platform, instead of only setting `truncated: true` in JSON. The text path had no disclosure at all, so an agent read a screen missing its footer, tab bar, or the items diff --git a/packages/contracts/src/replay.ts b/packages/contracts/src/replay.ts index c523c3a2e6..491e1a1824 100644 --- a/packages/contracts/src/replay.ts +++ b/packages/contracts/src/replay.ts @@ -108,6 +108,8 @@ export type ReplaySuiteTestFailed = { attempts: number; artifactsDir?: string; error: DaemonError; + /** Warnings accumulated before the failing step (skipped `optional` steps, capture degradations). */ + warnings?: string[]; /** Present when the owning runtime classified the failure as device/runner infrastructure. */ infrastructure?: true; shardIndex?: number; diff --git a/packages/maestro/src/internal/__tests__/engine.test.ts b/packages/maestro/src/internal/__tests__/engine.test.ts index e8e0b1d23b..b9151b548d 100644 --- a/packages/maestro/src/internal/__tests__/engine.test.ts +++ b/packages/maestro/src/internal/__tests__/engine.test.ts @@ -334,6 +334,62 @@ describe('executeMaestroProgram', () => { expect(observer.commandCompleted).toHaveBeenCalledOnce(); }); + test('failure events carry warnings accumulated before the failing step (#2560)', async () => { + const execute = vi.fn(async (request: MaestroRuntimeRequest) => { + request.invalidateObservation(); + throw new AppError('COMMAND_FAILED', 'leaf command failed'); + }); + const port = makePort({ + observe: vi.fn(async ({ generation }) => ({ generation, matched: false })), + execute, + }); + const observer = { commandFailed: vi.fn() }; + const program = parseMaestroProgram( + [ + '---', + '- assertVisible:', + ' text: Missing assertion', + ' optional: true', + '- tapOn: Missing target', + ].join('\n'), + ); + + await expect(executeMaestroProgram(program, port, { observer })).rejects.toThrow( + 'leaf command failed', + ); + + expect(observer.commandFailed).toHaveBeenCalledWith( + expect.objectContaining({ + command: expect.objectContaining({ kind: 'tapOn' }), + warnings: [expect.stringMatching(/Optional Maestro assertVisible skipped at line 2/)], + }), + ); + }); + + test('optional-skip warnings append the failure hint', async () => { + const execute = vi.fn(async (request: MaestroRuntimeRequest) => { + request.invalidateObservation(); + throw maestroTestFailure('Maestro target did not resolve to a visible element.', { + hint: 'Regular presentation requires valid viewport evidence.', + }); + }); + const port = makePort({ + observe: vi.fn(async ({ generation }) => ({ generation, matched: false })), + execute, + }); + const program = parseMaestroProgram( + ['---', '- tapOn:', ' text: Missing target', ' optional: true'].join('\n'), + ); + + const result = await executeMaestroProgram(program, port); + + expect(result.warnings).toEqual([ + expect.stringMatching( + /Optional Maestro tapOn skipped at line 2: .*\(Regular presentation requires valid viewport evidence\.\)/, + ), + ]); + }); + test('observer failure cannot mask nested leaf failure provenance', async () => { const execute = vi.fn(async (request: MaestroRuntimeRequest) => { request.invalidateObservation(); diff --git a/packages/maestro/src/internal/engine-types.ts b/packages/maestro/src/internal/engine-types.ts index fa78788846..1456ff6152 100644 --- a/packages/maestro/src/internal/engine-types.ts +++ b/packages/maestro/src/internal/engine-types.ts @@ -138,6 +138,8 @@ export type MaestroEngineObserver = { runtimeMetrics?: MaestroRuntimeMetrics; error: unknown; artifactPaths: readonly string[]; + /** Warnings accumulated before this failure, including skipped `optional` steps. */ + warnings: readonly string[]; }, ): void; }; diff --git a/packages/maestro/src/internal/facade-execution.ts b/packages/maestro/src/internal/facade-execution.ts index 49255cd7ca..fa0d0d5a66 100644 --- a/packages/maestro/src/internal/facade-execution.ts +++ b/packages/maestro/src/internal/facade-execution.ts @@ -59,6 +59,8 @@ export type MaestroFailedAction = MaestroActionEvent & { readonly runtimeMetrics?: MaestroCompletedActionEvent['runtimeMetrics']; readonly error: unknown; readonly artifactPaths: readonly string[]; + /** Warnings accumulated before this failure, including skipped `optional` steps. */ + readonly warnings: readonly string[]; readonly isControl: boolean; readonly redactions: readonly { name: string; value: string }[]; readonly resume: @@ -215,6 +217,7 @@ function createObserver( durationMs: event.durationMs, error: event.error, artifactPaths: event.artifactPaths, + warnings: [...event.warnings], isControl: isMaestroControlCommandDescriptor(event.command), redactions: event.command.kind === 'inputText' && event.command.text.length > 0 diff --git a/packages/maestro/src/internal/replay-plan-execution.ts b/packages/maestro/src/internal/replay-plan-execution.ts index 644262f23f..ba0fa008fd 100644 --- a/packages/maestro/src/internal/replay-plan-execution.ts +++ b/packages/maestro/src/internal/replay-plan-execution.ts @@ -96,6 +96,7 @@ async function executeObservedStep( ...runtimeMetricsDelta(metricsBefore, state.port.readMetrics?.()), error: failure.error, artifactPaths: [...state.artifacts], + warnings: [...state.warnings], }), ); throw failure; diff --git a/packages/maestro/src/internal/replay-plan-step-execution.ts b/packages/maestro/src/internal/replay-plan-step-execution.ts index 2a3c02bbec..c04dde624c 100644 --- a/packages/maestro/src/internal/replay-plan-step-execution.ts +++ b/packages/maestro/src/internal/replay-plan-step-execution.ts @@ -99,8 +99,13 @@ async function executeOptionalCommand( function formatOptionalWarning(command: MaestroRuntimeCommand, error: unknown): string { const source = `${command.source.path ? `${command.source.path}:` : ''}line ${command.source.line}`; - const message = error instanceof AppError ? error.message : String(error); - return `Optional Maestro ${command.kind} skipped at ${source}: ${message}`; + const detail = + error instanceof AppError + ? typeof error.details?.hint === 'string' + ? `${error.message} (${error.details.hint})` + : error.message + : String(error); + return `Optional Maestro ${command.kind} skipped at ${source}: ${detail}`; } function isOptionalCommand(command: MaestroRuntimeCommand): boolean { diff --git a/packages/platform-apple/src/runner/__tests__/snapshot-presentation.test.ts b/packages/platform-apple/src/runner/__tests__/snapshot-presentation.test.ts index 786998e14d..be504dcd19 100644 --- a/packages/platform-apple/src/runner/__tests__/snapshot-presentation.test.ts +++ b/packages/platform-apple/src/runner/__tests__/snapshot-presentation.test.ts @@ -1,8 +1,10 @@ import { AppError } from '@agent-device/kernel/errors'; import assert from 'node:assert/strict'; import { test } from 'vitest'; +import type { RawSnapshotNode } from '@agent-device/kernel/snapshot'; +import type { IosSystemSurfaceProvenance } from '@agent-device/contracts/ios-system-surface'; import type { AppleRunnerSnapshotResult } from '../snapshot-presentation.ts'; -import { presentAppleRunnerSnapshot } from '../snapshot-presentation.ts'; +import { presentAppleRunnerSnapshot, readAppleSnapshotResult } from '../snapshot-presentation.ts'; const NO_VIEWPORT_ROOT = { index: 0, type: 'Application', label: 'App' }; @@ -73,6 +75,10 @@ test('a sparse capture of a presented system surface names the surface host', () String(error.details?.hint), /com\.apple\.SafariViewService hosts the surface presented over the app/, ); + assert.deepEqual(error.details?.systemSurface, { + bundleId: 'com.apple.SafariViewService', + kind: 'web-auth', + }); }); test('a sparse payload failing another invariant still carries the verdict', () => { @@ -125,3 +131,61 @@ test('a sparse verdict still presents the nodes it did read', () => { ['App', 'Not Now'], ); }); + +const SYSTEM_SHEET: IosSystemSurfaceProvenance = { + bundleId: 'com.apple.SafariViewService', + kind: 'web-auth', +}; + +test('a presented system surface travels with an undeclared-payload refusal as typed provenance', () => { + try { + presentAppleRunnerSnapshot('device-1', undefined, { + nodes: [NO_VIEWPORT_ROOT], + quality: { state: 'healthy', backend: 'tree' }, + systemSurface: SYSTEM_SHEET, + }); + } catch (error) { + assert.ok(error instanceof AppError); + assert.deepEqual((error.details as Record).systemSurface, SYSTEM_SHEET); + assert.equal((error.details as Record).snapshotQuality, undefined); + return; + } + assert.fail('expected the presentation to refuse the payload'); +}); + +test('readAppleSnapshotResult keeps registry-known system surface provenance only', () => { + const known = readAppleSnapshotResult({ + nodes: [], + systemSurface: { bundleId: 'com.apple.SafariViewService', kind: 'web-auth' }, + }); + assert.deepEqual(known.systemSurface, SYSTEM_SHEET); + + const unknown = readAppleSnapshotResult({ + nodes: [], + systemSurface: { bundleId: 'com.example.PhishingService', kind: 'web-auth' }, + }); + assert.equal(unknown.systemSurface, undefined); +}); + +test('a healthy payload with valid viewport roots still presents', () => { + const screen: RawSnapshotNode = { + index: 0, + type: 'Application', + rect: { x: 0, y: 0, width: 390, height: 844 }, + }; + const button: RawSnapshotNode = { + index: 1, + parentIndex: 0, + type: 'Button', + label: 'Not Now', + rect: { x: 16, y: 400, width: 80, height: 32 }, + hittable: true, + }; + const nodes = presentAppleRunnerSnapshot('device-1', undefined, { + nodes: [screen, button], + }); + assert.deepEqual( + nodes.map((node) => node.index), + [0, 1], + ); +}); diff --git a/packages/platform-apple/src/runner/snapshot-presentation.ts b/packages/platform-apple/src/runner/snapshot-presentation.ts index 342041d54b..8913c9c793 100644 --- a/packages/platform-apple/src/runner/snapshot-presentation.ts +++ b/packages/platform-apple/src/runner/snapshot-presentation.ts @@ -45,7 +45,7 @@ export function readAppleSnapshotResult( nodes: Array.isArray(result.nodes) ? (result.nodes as RawSnapshotNode[]) : undefined, truncated: typeof result.truncated === 'boolean' ? result.truncated : undefined, quality: readSnapshotQualityVerdict(result.snapshotQuality), - qualityPayload: readQualityPayload(result.qualityPayload), + qualityPayload: readQualityPayload(result.qualityPayload, systemSurface), runnerFatal: result.runnerFatal === true, ...(systemSurface ? { systemSurface } : {}), message: @@ -131,6 +131,7 @@ function throwSnapshotPresentationError(error: unknown, result: AppleRunnerSnaps error.message, { ...toIosSnapshotEngineErrorDetails(error), + ...(result.systemSurface ? { systemSurface: result.systemSurface } : {}), snapshotQuality: { state: verdict.state, backend: verdict.backend, @@ -142,7 +143,7 @@ function throwSnapshotPresentationError(error: unknown, result: AppleRunnerSnaps error, ); } - throwSnapshotEngineError(error); + throwSnapshotEngineError(error, result.systemSurface); } function sparseCaptureHint( @@ -159,7 +160,10 @@ function sparseCaptureHint( .join(' '); } -function readQualityPayload(value: unknown): IosRunnerQualityPayloadFacts | undefined { +function readQualityPayload( + value: unknown, + systemSurface?: IosSystemSurfaceProvenance, +): IosRunnerQualityPayloadFacts | undefined { if (value === undefined) return undefined; if (!isRecord(value) || !Array.isArray(value.nodes) || typeof value.truncated !== 'boolean') { throwSnapshotEngineError( @@ -167,6 +171,7 @@ function readQualityPayload(value: unknown): IosRunnerQualityPayloadFacts | unde 'invalid-quality-payload', 'iOS runner returned an invalid quality payload', ), + systemSurface, ); } if (value.scope !== undefined && value.scope !== null) { @@ -176,6 +181,7 @@ function readQualityPayload(value: unknown): IosRunnerQualityPayloadFacts | unde 'iOS runner quality payload must be unscoped', { field: 'scope' }, ), + systemSurface, ); } return { nodes: value.nodes as RawSnapshotNode[], truncated: value.truncated, scope: null }; @@ -204,12 +210,18 @@ function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } -function throwSnapshotEngineError(error: unknown): never { +function throwSnapshotEngineError( + error: unknown, + systemSurface?: IosSystemSurfaceProvenance, +): never { if (!(error instanceof IosSnapshotEngineError)) throw error; throw new AppError( 'COMMAND_FAILED', error.message, - toIosSnapshotEngineErrorDetails(error), + { + ...toIosSnapshotEngineErrorDetails(error), + ...(systemSurface ? { systemSurface } : {}), + }, error, ); } diff --git a/packages/replay-test/src/internal/session-test-attempt.ts b/packages/replay-test/src/internal/session-test-attempt.ts index e68cb9bfc0..2af7e38fef 100644 --- a/packages/replay-test/src/internal/session-test-attempt.ts +++ b/packages/replay-test/src/internal/session-test-attempt.ts @@ -388,6 +388,9 @@ function buildReplayTestFailedResult( attempts: outcome.attempts, artifactsDir: context.testArtifactsDir, error, + ...(attemptOutcome?.status === 'failed' && attemptOutcome.warnings + ? { warnings: [...attemptOutcome.warnings] } + : {}), ...(attemptOutcome?.status === 'failed' && attemptOutcome.infrastructure ? { infrastructure: true as const } : {}), diff --git a/packages/replay-test/src/internal/session-test-types.ts b/packages/replay-test/src/internal/session-test-types.ts index 0b0c296723..723016235c 100644 --- a/packages/replay-test/src/internal/session-test-types.ts +++ b/packages/replay-test/src/internal/session-test-types.ts @@ -144,6 +144,8 @@ export type ReplayTestAttemptFailed = { status: 'failed'; error: ReplayTestAttemptError; artifactPaths: readonly string[]; + /** Warnings accumulated before the failing step (skipped `optional` steps, capture degradations). */ + warnings?: readonly string[]; snapshotDiagnostics?: SnapshotDiagnosticsSummary; /** * The host's verdict that this failure is environmental (device/runner/boot) rather than a diff --git a/src/cli/commands/__tests__/generic.test.ts b/src/cli/commands/__tests__/generic.test.ts index 38d26bbf57..94e8a842f4 100644 --- a/src/cli/commands/__tests__/generic.test.ts +++ b/src/cli/commands/__tests__/generic.test.ts @@ -1,5 +1,8 @@ +import fs from 'node:fs'; +import path from 'node:path'; import { test } from 'vitest'; import assert from 'node:assert/strict'; +import { mkdtempForTestSync } from '../../../__tests__/test-utils/tmp-dir.ts'; import { createAgentDeviceClient } from '../../../agent-device-client.ts'; import type { DaemonResponse } from '@agent-device/kernel/contracts'; import type { CliFlags } from '@agent-device/contracts/command'; @@ -49,3 +52,42 @@ test('snapshot --level digest --json preserves the digest through the generic CL // snapshot formatter that expects `nodes`. assert.deepEqual(parsed.data, digest); }); + +test('replay human output keeps the composable warnings channel (#2560)', async () => { + const dir = mkdtempForTestSync('agent-device-generic-replay-'); + const scriptPath = path.join(dir, 'flow.yaml'); + fs.writeFileSync(scriptPath, 'appId: com.example\n---\n- tapOn: "Sign in"\n'); + const client = createAgentDeviceClient( + { session: 'qa' }, + { + transport: async (req): Promise => { + assert.equal(req.command, 'replay'); + return { + ok: true, + data: { + replayed: 3, + healed: 0, + session: 'qa', + sessionActive: false, + artifactPaths: [], + warnings: ['Optional Maestro tapOn skipped at flow.yaml:line 12'], + message: 'Replayed 3 steps in 9.1s', + }, + }; + }, + }, + ); + + const out = await captureStdout(() => + runGenericClientBackedCommand({ + command: 'replay' as ClientBackedCliCommandName, + positionals: [scriptPath], + flags: {} as CliFlags, + client, + }), + ); + + assert.match(out, /Replayed 3 steps in 9\.1s/); + // The skipped optional step must be visible to the human reader, not only --json. + assert.match(out, /Warning: Optional Maestro tapOn skipped at flow\.yaml:line 12/); +}); diff --git a/src/cli/replay-test/__tests__/reporters-default.test.ts b/src/cli/replay-test/__tests__/reporters-default.test.ts index 247dbc3afc..32c9ab72ec 100644 --- a/src/cli/replay-test/__tests__/reporters-default.test.ts +++ b/src/cli/replay-test/__tests__/reporters-default.test.ts @@ -175,6 +175,83 @@ function failingSuiteWithDivergence(): ReplaySuiteResult { }; } +function failingSuiteWithWarnings(): ReplaySuiteResult { + const failed = { + file: '/tmp/flow.yaml', + title: 'sign-in', + session: 'test-session', + status: 'failed' as const, + durationMs: 10, + attempts: 1, + warnings: ['Optional Maestro assertVisible skipped at /tmp/flow.yaml:line 3: sheet owns focus'], + error: { code: 'COMMAND_FAILED', message: 'tapOn Save did not resolve' }, + }; + return { + total: 1, + executed: 1, + passed: 0, + failed: 1, + skipped: 0, + notRun: 0, + durationMs: 10, + failures: [failed], + tests: [failed], + }; +} + +function passingSuiteWithWarnings(): ReplaySuiteResult { + const passed = { + file: '/tmp/flow.yaml', + title: 'sign-in', + session: 'test-session', + status: 'passed' as const, + durationMs: 20, + attempts: 1, + replayed: 3, + healed: 0, + warnings: ['Optional Maestro tapOn skipped at /tmp/flow.yaml:line 12: target did not resolve'], + }; + return { + total: 1, + executed: 1, + passed: 1, + failed: 0, + skipped: 0, + notRun: 0, + durationMs: 20, + failures: [], + tests: [passed], + }; +} + +test('default replay test reporter surfaces per-test warnings on a passing suite (#2560)', () => { + const reporter = createDefaultReplayTestReporter(); + const { context, stdout } = createReporterContext({ stderrIsTty: false }); + reporter.onSuiteEnd?.(passingSuiteWithWarnings(), context); + const out = stdout.join(''); + assert.match(out, /Warnings:\n/); + assert.match(out, /sign-in.*warning: Optional Maestro tapOn skipped at \/tmp\/flow\.yaml/s); +}); + +test('default replay test reporter omits the warnings section without warnings', () => { + const reporter = createDefaultReplayTestReporter(); + const { context, stdout } = createReporterContext({ stderrIsTty: false }); + reporter.onSuiteEnd?.(emptySuite(), context); + assert.equal(stdout.join('').includes('Warnings:'), false); +}); + +test('default replay test reporter surfaces warnings accumulated before a failure (#2560)', () => { + const reporter = createDefaultReplayTestReporter(); + const { context, stdout } = createReporterContext({ stderrIsTty: false }); + reporter.onSuiteEnd?.(failingSuiteWithWarnings(), context); + const out = stdout.join(''); + assert.match(out, /Warnings:\n/); + assert.match( + out, + /sign-in.*warning: Optional Maestro assertVisible skipped at .*sheet owns focus/s, + ); +}); + test('default replay test reporter surfaces the divergence repair report on a failure', () => { const reporter = createDefaultReplayTestReporter(); const { context, stdout } = createReporterContext({ stderrIsTty: false }); diff --git a/src/cli/replay-test/reporters/default.ts b/src/cli/replay-test/reporters/default.ts index 393f33a3eb..a8e4395ded 100644 --- a/src/cli/replay-test/reporters/default.ts +++ b/src/cli/replay-test/reporters/default.ts @@ -23,6 +23,7 @@ import { replayErrorLogLine, replayTestDisplayNameWithFile, replayTestFailureFileLine, + replayTestWarningLines, type FailedReplayTestResult, type PassedReplayTestResult, } from './format.ts'; @@ -115,10 +116,31 @@ function renderReplayTestSummary( ): void { const flaky = data.tests.filter(isFlakyReplayTestResult); context.stdout.write(`${formatReplayTestSummaryLine(data, flaky.length)}\n`); + renderWarningsSection(data.tests, context); renderFailureDetails(data.tests.filter(isFailedReplayTestResult), context); renderFlakyTestSummary(flaky, context); } +// Steps inside a test can be skipped (`optional: true`) or their capture can +// degrade, leaving a warning as the only trace — whether the test then passes or +// fails, the human surface must carry it, not only --json and JUnit (#2560). +function renderWarningsSection( + results: ReplaySuiteResult['tests'], + context: ReplayTestReporterContext, +): void { + const warned = results + .map((result) => ({ result, lines: replayTestWarningLines(result) })) + .filter((entry) => entry.lines.length > 0); + if (warned.length === 0) return; + context.stdout.write('\n'); + context.stdout.write('Warnings:\n'); + for (const { result, lines } of warned) { + for (const line of lines) { + context.stdout.write(` ${replayTestDisplayNameWithFile(result)}: ${line}\n`); + } + } +} + function formatReplayTestSummaryLine(data: ReplaySuiteResult, flakyCount: number): string { const durationMs = typeof data.durationMs === 'number' ? data.durationMs : undefined; const useColor = supportsColor(); diff --git a/src/cli/replay-test/reporters/format.ts b/src/cli/replay-test/reporters/format.ts index 496f5dc6e7..1756cb489b 100644 --- a/src/cli/replay-test/reporters/format.ts +++ b/src/cli/replay-test/reporters/format.ts @@ -1,4 +1,5 @@ import path from 'node:path'; +import { collapseWarningText } from '../../../commands/output-common.ts'; import type { ReplaySuiteTestResult } from '@agent-device/contracts/replay'; export type PassedReplayTestResult = Extract; @@ -107,8 +108,8 @@ export function appendReplayTestShardMetadata( } export function replayTestWarningLines(result: ReplaySuiteTestResult): string[] { - if (result.status !== 'passed') return []; - return (result.warnings ?? []).map((warning) => `warning: ${warning}`); + const warnings = 'warnings' in result ? result.warnings : undefined; + return (warnings ?? []).map((warning) => `warning: ${collapseWarningText(warning)}`); } export function appendOptionalLine(lines: string[], line: string | undefined): void { diff --git a/src/commands/output-common.test.ts b/src/commands/output-common.test.ts new file mode 100644 index 0000000000..9aa5c3ebab --- /dev/null +++ b/src/commands/output-common.test.ts @@ -0,0 +1,44 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { messageWithWarningsOutput, messageWithWarningsText } from './output-common.ts'; + +test('messageWithWarningsText renders the bare message when there are no warnings', () => { + assert.equal(messageWithWarningsText({ message: 'Replayed 3 steps' }), 'Replayed 3 steps'); +}); + +test('messageWithWarningsText appends one Warning line per string warning (#2560)', () => { + const text = messageWithWarningsText({ + message: 'Replayed 3 steps', + warnings: ['Optional Maestro tapOn skipped at flow.yaml:line 12', 42, null], + }); + assert.equal( + text, + 'Replayed 3 steps\nWarning: Optional Maestro tapOn skipped at flow.yaml:line 12', + ); +}); + +test('messageWithWarningsText renders warnings even without a message', () => { + assert.equal( + messageWithWarningsText({ warnings: ['capture degraded'] }), + 'Warning: capture degraded', + ); +}); + +test('messageWithWarningsText collapses newlines inside a warning', () => { + assert.equal( + messageWithWarningsText({ message: 'Replayed 1 step', warnings: ['line one\n line two'] }), + 'Replayed 1 step\nWarning: line one line two', + ); +}); + +test('messageWithWarningsText is silent for an empty response', () => { + assert.equal(messageWithWarningsText({}), null); +}); + +test('messageWithWarningsOutput carries the same text and the full data', () => { + const result = { message: 'Replayed 3 steps', warnings: ['one'] }; + assert.deepEqual(messageWithWarningsOutput({ input: {}, result }), { + data: result, + text: 'Replayed 3 steps\nWarning: one', + }); +}); diff --git a/src/commands/output-common.ts b/src/commands/output-common.ts index f2f1e72a40..32b8147527 100644 --- a/src/commands/output-common.ts +++ b/src/commands/output-common.ts @@ -31,20 +31,32 @@ export function messageCliOutput(result: Record): CliOutput { } /** - * `messageCliOutput` plus one `Warning:` line per entry of the response's `warnings` + * The response message plus one `Warning:` line per entry of the response's `warnings` * array — the composable warnings channel (`open`, `debug`, snapshot capture use it too), * so a warning the daemon appended reaches the human CLI reader, not only `--json`. */ +export function messageWithWarningsText(result: Record): string | null { + const message = readCommandMessage(result); + const warnings = Array.isArray(result.warnings) + ? result.warnings.filter((warning): warning is string => typeof warning === 'string') + : []; + if (warnings.length === 0) return message; + return [message, ...warnings.map((warning) => `Warning: ${collapseWarningText(warning)}`)] + .filter(Boolean) + .join('\n'); +} + +/** Warning text can embed runner newlines; rendered warning lines stay one-per-warning. */ +export function collapseWarningText(warning: string): string { + return warning.replaceAll(/\s*\n\s*/g, ' '); +} + +/** `messageCliOutput` carrying {@link messageWithWarningsText} as its text. */ export const messageWithWarningsOutput = resultOutput( - (result: Record): CliOutput => { - const output = messageCliOutput(result); - const warnings = Array.isArray(result.warnings) - ? result.warnings.filter((warning): warning is string => typeof warning === 'string') - : []; - if (warnings.length === 0) return output; - const lines = [output.text, ...warnings.map((warning) => `Warning: ${warning}`)]; - return { data: output.data, text: lines.filter(Boolean).join('\n') }; - }, + (result: Record): CliOutput => ({ + data: result, + text: messageWithWarningsText(result), + }), ); /** diff --git a/src/commands/replay/index.ts b/src/commands/replay/index.ts index 1881384ca7..a9f6510df5 100644 --- a/src/commands/replay/index.ts +++ b/src/commands/replay/index.ts @@ -1,4 +1,5 @@ import type { CommandSchemaOverride } from '@agent-device/command-registry/command-schema'; +import { messageWithWarningsOutput } from '../output-common.ts'; import { defineCommandFacet, defineCommandFamilyFromFacets } from '../family/types.ts'; import { booleanField, @@ -239,6 +240,10 @@ export const replayCommandFacet = defineCommandFacet({ cliSchema: replayCliSchema, cliReader: replayCliReader, daemonWriter: replayDaemonWriter, + // Replay owns a composable warnings channel (`optional` step skips, capture degradations); + // a run that reports success while warnings say otherwise must not render as a bare + // success line (#2560). + cliOutputFormatter: messageWithWarningsOutput, }); export const testCommandFacet = defineCommandFacet({ diff --git a/src/daemon/replay/internal/__tests__/session-replay-maestro-failure.test.ts b/src/daemon/replay/internal/__tests__/session-replay-maestro-failure.test.ts index 73483b2dbe..00e06c4beb 100644 --- a/src/daemon/replay/internal/__tests__/session-replay-maestro-failure.test.ts +++ b/src/daemon/replay/internal/__tests__/session-replay-maestro-failure.test.ts @@ -50,6 +50,7 @@ beforeEach(() => { async function buildFailureScenario( command: MaestroRuntimeCommand, nodes: SnapshotNode[], + options: { warnings?: readonly string[] } = {}, ): Promise<{ response: Extract>, { ok: false }>; sessionStore: SessionStore; @@ -64,7 +65,7 @@ async function buildFailureScenario( const failure = await captureMaestroFailure(command, path.join(root, 'flow.yaml')); const response = await buildTypedMaestroFailureResponse({ error: { code: 'COMMAND_FAILED', message: 'typed Maestro action failed' }, - failure, + failure: options.warnings ? { ...failure, warnings: options.warnings } : failure, replayPath: path.join(root, 'flow.yaml'), req: baseReq({ flags: { replayBackend: 'maestro', platform: 'ios' } }), sessionName, @@ -79,10 +80,44 @@ async function buildFailureScenario( async function buildFailureResponse( command: MaestroRuntimeCommand, nodes: SnapshotNode[], + options: { warnings?: readonly string[] } = {}, ): Promise>, { ok: false }>> { - return (await buildFailureScenario(command, nodes)).response; + return (await buildFailureScenario(command, nodes, options)).response; } +test('typed Maestro failure response carries warnings accumulated before the failing step (#2560)', async () => { + const response = await buildFailureResponse( + { + kind: 'tapOn' as const, + source: { path: '/flows/login.yaml', line: 4 }, + target: { space: 'target' as const, selector: { id: 'save' } }, + }, + [], + { warnings: ['Optional Maestro assertVisible skipped at line 2: no match'] }, + ); + + const details = response.error.details; + expect(details).toBeDefined(); + expect(details.warnings).toEqual([ + 'Optional Maestro assertVisible skipped at line 2: no match', + ]); + expect((details.divergence as { kind: string }).kind).toBe('action-failure'); +}); + +test('typed Maestro failure response omits an empty warnings channel', async () => { + const response = await buildFailureResponse( + { + kind: 'tapOn' as const, + source: { path: '/flows/login.yaml', line: 4 }, + target: { space: 'target' as const, selector: { id: 'save' } }, + }, + [], + { warnings: [] }, + ); + + expect('warnings' in (response.error.details ?? {})).toBe(false); +}); + test('typed Maestro failure projection keeps action and source provenance', async () => { const command = { kind: 'tapOn' as const, diff --git a/src/daemon/replay/internal/__tests__/session-test-outcome.test.ts b/src/daemon/replay/internal/__tests__/session-test-outcome.test.ts new file mode 100644 index 0000000000..ff0c5faf87 --- /dev/null +++ b/src/daemon/replay/internal/__tests__/session-test-outcome.test.ts @@ -0,0 +1,53 @@ +import { expect, test } from 'vitest'; +import { toReplayTestAttemptOutcome } from '../session-test-outcome.ts'; +import type { DaemonResponse } from '../../../daemon-request.ts'; + +test('failed attempt outcome carries warnings from the error details (#2560)', () => { + const response: DaemonResponse = { + ok: false, + error: { + code: 'REPLAY_DIVERGENCE', + message: 'Replay failed at step 2 (tapOn Save): target did not resolve', + details: { + replayPath: '/flows/login.yaml', + step: 2, + warnings: ['Optional Maestro assertVisible skipped at line 1: not found'], + }, + }, + }; + + const outcome = toReplayTestAttemptOutcome(response); + + expect(outcome).toMatchObject({ + status: 'failed', + warnings: ['Optional Maestro assertVisible skipped at line 1: not found'], + infrastructure: false, + }); +}); + +test('failed attempt outcome omits absent or empty warnings', () => { + const withoutWarnings = toReplayTestAttemptOutcome({ + ok: false, + error: { code: 'COMMAND_FAILED', message: 'step failed' }, + }); + expect('warnings' in withoutWarnings).toBe(false); + + const emptyWarnings = toReplayTestAttemptOutcome({ + ok: false, + error: { code: 'COMMAND_FAILED', message: 'step failed', details: { warnings: [7] } }, + }); + expect('warnings' in emptyWarnings).toBe(false); +}); + +test('passed attempt outcome keeps reading warnings from response data', () => { + const outcome = toReplayTestAttemptOutcome({ + ok: true, + data: { replayed: 3, warnings: ['capture degraded'], artifactPaths: [] }, + }); + + expect(outcome).toMatchObject({ + status: 'passed', + replayed: 3, + warnings: ['capture degraded'], + }); +}); diff --git a/src/daemon/replay/internal/session-replay-maestro-failure.ts b/src/daemon/replay/internal/session-replay-maestro-failure.ts index 182b541a86..667020f793 100644 --- a/src/daemon/replay/internal/session-replay-maestro-failure.ts +++ b/src/daemon/replay/internal/session-replay-maestro-failure.ts @@ -157,6 +157,7 @@ export async function buildTypedMaestroFailureResponse(params: { snapshotDiagnostics: params.snapshotDiagnostics, divergence: bounded, scrubVars, + warnings: failure.warnings, }); } diff --git a/src/daemon/replay/internal/session-replay-runtime-failure-response.ts b/src/daemon/replay/internal/session-replay-runtime-failure-response.ts index 7ac6ad3ecf..3f7dd74a05 100644 --- a/src/daemon/replay/internal/session-replay-runtime-failure-response.ts +++ b/src/daemon/replay/internal/session-replay-runtime-failure-response.ts @@ -63,6 +63,12 @@ export function buildReplayDivergenceFailureResponseFromDescriptor(params: { snapshotDiagnostics?: SnapshotDiagnosticsSummary; divergence: unknown; scrubVars: readonly ReplayVarScrubEntry[]; + /** + * Composable warnings accumulated before the failing step (skipped `optional` + * steps, capture degradations). They describe the run, not this step's cause, + * so they ride at response-error level rather than through the cause allowlist. + */ + warnings?: readonly string[]; }): DaemonResponse { const { error, @@ -75,6 +81,7 @@ export function buildReplayDivergenceFailureResponseFromDescriptor(params: { snapshotDiagnostics, divergence, scrubVars, + warnings, } = params; return { ok: false, @@ -97,6 +104,7 @@ export function buildReplayDivergenceFailureResponseFromDescriptor(params: { positionals, artifactPaths, ...(snapshotDiagnostics ? { snapshotDiagnostics } : {}), + ...(warnings && warnings.length > 0 ? { warnings: [...warnings] } : {}), divergence, }, }, @@ -117,6 +125,7 @@ const SAFE_CAUSE_DETAIL_KEYS = [ 'retriable', 'snapshotQuality', 'supportedOn', + 'systemSurface', ] as const; function pickSafeCauseDetails( diff --git a/src/daemon/replay/internal/session-test-outcome.ts b/src/daemon/replay/internal/session-test-outcome.ts index 1ce7843c0c..3d69e6a7c0 100644 --- a/src/daemon/replay/internal/session-test-outcome.ts +++ b/src/daemon/replay/internal/session-test-outcome.ts @@ -14,10 +14,12 @@ import type { ReplayTestAttemptFailed, ReplayTestAttemptOutcome } from '@agent-d */ export function toReplayTestAttemptOutcome(response: DaemonResponse): ReplayTestAttemptOutcome { if (!response.ok) { + const warnings = readStringArray(response.error.details?.warnings); return { status: 'failed', error: response.error, artifactPaths: readArtifactPaths(response.error.details?.artifactPaths), + ...(warnings.length > 0 ? { warnings } : {}), infrastructure: isReplayInfrastructureFailure(response), ...snapshotDiagnostics(response.error.details?.snapshotDiagnostics), }; diff --git a/website/docs/docs/replay-e2e.md b/website/docs/docs/replay-e2e.md index 03a52702de..6933f520e4 100644 --- a/website/docs/docs/replay-e2e.md +++ b/website/docs/docs/replay-e2e.md @@ -123,6 +123,7 @@ agent-device test ./workflows --reporter default --reporter junit:./tmp/junit.xm - `replay-timing.ndjson` records attempt, cleanup, and per-step start/stop events with durations. Upload it from CI even for passing runs when comparing local and CI performance. - Timeouts are cooperative: the runner marks the attempt failed at the timeout boundary, then gives the underlying replay a short grace period to stop before session cleanup. - The default text reporter streams live progress on stderr while a suite runs, then prints the final summary, failed tests, and passed-on-retry flaky tests. Use `--verbose` to include step traces in completed-test progress output. +- The default reporter prints a `Warnings:` section after the summary when any test accumulated composable warnings — for example a Maestro step with `optional: true` that was skipped — whether that test passed or failed. `--json` carries the same strings in each test result's `warnings` array. - `--reporter` is repeatable. Built-ins are `default` for the console summary and `junit:` for JUnit XML. Passing any explicit reporter list replaces the implicit default reporter, so include `--reporter default` when you also want terminal output. `--report-junit ` remains a compatibility alias for `--reporter junit:`. - JUnit reports preserve legal Unicode and whitespace, and replace characters forbidden by XML 1.0 (such as terminal ESC or NUL) with `U+FFFD` (`�`) so CI parsers can read the report. JSON and other reporters retain the original suite values. - When `--fail-fast` and retries are both set, the current test still consumes its retries before the suite stops. From d32d292f7fcabb8c4de710b18ee5a29f6976315b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 13 Sep 2026 20:55:33 +0200 Subject: [PATCH 2/4] fix(replay): surface warnings on failed runs and keep them honest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failing `replay` still printed no trace of the run's accumulated warnings: the human error renderer read message, hint, and divergence but never the error-level warnings channel, so the pass/fail asymmetry survived on that surface. It now prints each warning after the error. Round-two corrections: warnings on the divergence response are scrubbed against replay vars like message and hint already are; a failed attempt copies warnings only when non-empty, matching the passing branch; the optional-skip hint append is removed because no producer of a maestro test failure emits a hint — capture-error lineage stays with #2450 — along with the CHANGELOG claim it rested on; the failed-result copy gains its own module test, and JUnit covers a failed test's warning lines. --- .../src/internal/__tests__/engine.test.ts | 24 ------- .../internal/replay-plan-step-execution.ts | 9 +-- .../__tests__/session-test-attempt.test.ts | 71 +++++++++++++++++++ .../src/internal/session-test-attempt.ts | 4 +- .../reporters/__tests__/junit.test.ts | 32 +++++++++ src/commands/output/error.test.ts | 25 +++++++ src/commands/output/error.ts | 14 ++++ .../session-replay-maestro-failure.test.ts | 6 +- ...session-replay-runtime-failure-response.ts | 4 +- website/docs/docs/replay-e2e.md | 2 +- 10 files changed, 152 insertions(+), 39 deletions(-) create mode 100644 packages/replay-test/src/internal/__tests__/session-test-attempt.test.ts diff --git a/packages/maestro/src/internal/__tests__/engine.test.ts b/packages/maestro/src/internal/__tests__/engine.test.ts index b9151b548d..13212df367 100644 --- a/packages/maestro/src/internal/__tests__/engine.test.ts +++ b/packages/maestro/src/internal/__tests__/engine.test.ts @@ -366,30 +366,6 @@ describe('executeMaestroProgram', () => { ); }); - test('optional-skip warnings append the failure hint', async () => { - const execute = vi.fn(async (request: MaestroRuntimeRequest) => { - request.invalidateObservation(); - throw maestroTestFailure('Maestro target did not resolve to a visible element.', { - hint: 'Regular presentation requires valid viewport evidence.', - }); - }); - const port = makePort({ - observe: vi.fn(async ({ generation }) => ({ generation, matched: false })), - execute, - }); - const program = parseMaestroProgram( - ['---', '- tapOn:', ' text: Missing target', ' optional: true'].join('\n'), - ); - - const result = await executeMaestroProgram(program, port); - - expect(result.warnings).toEqual([ - expect.stringMatching( - /Optional Maestro tapOn skipped at line 2: .*\(Regular presentation requires valid viewport evidence\.\)/, - ), - ]); - }); - test('observer failure cannot mask nested leaf failure provenance', async () => { const execute = vi.fn(async (request: MaestroRuntimeRequest) => { request.invalidateObservation(); diff --git a/packages/maestro/src/internal/replay-plan-step-execution.ts b/packages/maestro/src/internal/replay-plan-step-execution.ts index c04dde624c..2a3c02bbec 100644 --- a/packages/maestro/src/internal/replay-plan-step-execution.ts +++ b/packages/maestro/src/internal/replay-plan-step-execution.ts @@ -99,13 +99,8 @@ async function executeOptionalCommand( function formatOptionalWarning(command: MaestroRuntimeCommand, error: unknown): string { const source = `${command.source.path ? `${command.source.path}:` : ''}line ${command.source.line}`; - const detail = - error instanceof AppError - ? typeof error.details?.hint === 'string' - ? `${error.message} (${error.details.hint})` - : error.message - : String(error); - return `Optional Maestro ${command.kind} skipped at ${source}: ${detail}`; + const message = error instanceof AppError ? error.message : String(error); + return `Optional Maestro ${command.kind} skipped at ${source}: ${message}`; } function isOptionalCommand(command: MaestroRuntimeCommand): boolean { diff --git a/packages/replay-test/src/internal/__tests__/session-test-attempt.test.ts b/packages/replay-test/src/internal/__tests__/session-test-attempt.test.ts new file mode 100644 index 0000000000..d98771b26b --- /dev/null +++ b/packages/replay-test/src/internal/__tests__/session-test-attempt.test.ts @@ -0,0 +1,71 @@ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { expect, test } from 'vitest'; +import { runReplayTestCase } from '../session-test-attempt.ts'; +import type { ReplayTestRunEntry } from '../session-test-discovery.ts'; +import type { ReplayTestAttemptOutcome } from '../session-test-types.ts'; +import type { ReplaySuiteTestFailed } from '@agent-device/contracts/replay'; + +const FAILED_WITH_WARNINGS: ReplayTestAttemptOutcome = { + status: 'failed', + error: { + code: 'REPLAY_DIVERGENCE', + message: 'Replay failed at step 2 (tapOn "Save"): target did not resolve', + }, + artifactPaths: [], + warnings: ['Optional Maestro assertVisible skipped at line 3: sheet owns focus'], + infrastructure: false, +}; + +const FAILED_WITHOUT_WARNINGS: ReplayTestAttemptOutcome = { + status: 'failed', + error: { code: 'COMMAND_FAILED', message: 'tap failed' }, + artifactPaths: [], + infrastructure: false, +}; + +function makeEntry(): ReplayTestRunEntry { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-test-attempt-')); + const filePath = path.join(root, '01-flow.ad'); + fs.writeFileSync(filePath, 'context platform=ios\nopen "Demo"\n'); + return { + kind: 'run', + path: filePath, + title: 'flow', + manifest: { device: { platform: { kind: 'declared', value: 'ios' } } }, + }; +} + +async function runFailedCase(outcome: ReplayTestAttemptOutcome): Promise { + const report = await runReplayTestCase({ + entry: makeEntry(), + sessionName: 'default', + suiteInvocationId: 'suite-attempt', + caseIndex: 0, + retries: 0, + suiteArtifactsDir: fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-test-attempt-suite-')), + suiteIndex: 1, + suiteTotal: 1, + runReplay: async () => outcome, + cleanupSession: async () => {}, + emitProgress: () => {}, + isCanceled: () => false, + emitDiagnostic: () => {}, + bindAttemptCancellation: () => ({ cancel: () => {}, release: () => {} }), + }); + if (report.result.status !== 'failed') throw new Error('expected a failed test result'); + return report.result; +} + +test('a failed test result carries the warnings accumulated before the failure (#2560)', async () => { + const result = await runFailedCase(FAILED_WITH_WARNINGS); + expect(result.warnings).toEqual([ + 'Optional Maestro assertVisible skipped at line 3: sheet owns focus', + ]); +}); + +test('a failed test result omits warnings when the attempt had none', async () => { + const result = await runFailedCase(FAILED_WITHOUT_WARNINGS); + expect('warnings' in result).toBe(false); +}); diff --git a/packages/replay-test/src/internal/session-test-attempt.ts b/packages/replay-test/src/internal/session-test-attempt.ts index 2af7e38fef..5f40bf14ff 100644 --- a/packages/replay-test/src/internal/session-test-attempt.ts +++ b/packages/replay-test/src/internal/session-test-attempt.ts @@ -388,8 +388,8 @@ function buildReplayTestFailedResult( attempts: outcome.attempts, artifactsDir: context.testArtifactsDir, error, - ...(attemptOutcome?.status === 'failed' && attemptOutcome.warnings - ? { warnings: [...attemptOutcome.warnings] } + ...(attemptOutcome?.status === 'failed' && (attemptOutcome.warnings?.length ?? 0) > 0 + ? { warnings: [...attemptOutcome.warnings!] } : {}), ...(attemptOutcome?.status === 'failed' && attemptOutcome.infrastructure ? { infrastructure: true as const } diff --git a/src/cli/replay-test/reporters/__tests__/junit.test.ts b/src/cli/replay-test/reporters/__tests__/junit.test.ts index d76e4c1d3c..7f2776d821 100644 --- a/src/cli/replay-test/reporters/__tests__/junit.test.ts +++ b/src/cli/replay-test/reporters/__tests__/junit.test.ts @@ -80,6 +80,38 @@ test('buildReplayJunitXml escapes tricky failure title/message and round-trips t assert.ok(failure.text?.startsWith(TRICKY_MESSAGE)); }); +test('buildReplayJunitXml carries accumulated warnings of a failed test into system-out (#2560)', () => { + const suite: ReplaySuiteResult = { + total: 1, + executed: 1, + passed: 0, + failed: 1, + skipped: 0, + notRun: 0, + durationMs: 1200, + failures: [], + tests: [ + { + file: '/tmp/flows/login.yaml', + title: 'sign-in', + session: 'default', + status: 'failed', + durationMs: 1200, + attempts: 1, + warnings: ['Optional Maestro assertVisible skipped at line 3: sheet owns focus'], + error: { code: 'COMMAND_FAILED', message: 'tapOn Save did not resolve' }, + }, + ], + }; + suite.failures = suite.tests.filter((result) => result.status === 'failed'); + + const nodes = writeSuiteAndParse(suite); + const testcase = findChild(findChild(nodes[0]!, 'testsuite')!, 'testcase'); + assert.ok(testcase); + const systemOut = findChild(testcase, 'system-out'); + assert.ok(systemOut?.text?.includes('warning: Optional Maestro assertVisible skipped at line 3')); +}); + test('buildReplayJunitXml escapes tricky skip message', () => { const suite: ReplaySuiteResult = { total: 1, diff --git a/src/commands/output/error.test.ts b/src/commands/output/error.test.ts index fa490bd18b..42e84d8a22 100644 --- a/src/commands/output/error.test.ts +++ b/src/commands/output/error.test.ts @@ -76,6 +76,31 @@ test('printHumanError renders a compact divergence report unconditionally (not g // Not gated behind --debug: showDetails defaults to false/undefined here. }); +// --- #2560: run-level warnings ride the failed response to the human surface --- + +test('printHumanError renders run-level warnings carried in error details', async () => { + const err = new AppError('REPLAY_DIVERGENCE', 'Replay failed at step 2 (tapOn "Save")', { + step: 2, + warnings: ['Optional Maestro assertVisible skipped at line 1: no match', 7], + }); + + const output = await captureStderr(() => printHumanError(err)); + + assert.match( + output, + /Error \(REPLAY_DIVERGENCE\)[\s\S]*^Warning: Optional Maestro assertVisible skipped at line 1: no match$/m, + ); + assert.equal(output.includes('Warning: 7'), false); +}); + +test('printHumanError prints no Warning lines without a warnings channel', async () => { + const err = new AppError('COMMAND_FAILED', 'tap failed'); + + const output = await captureStderr(() => printHumanError(err)); + + assert.equal(output.includes('Warning:'), false); +}); + // --- #1597: AMBIGUOUS_MATCH candidates print unconditionally, capped at 5 --- test('printHumanError lists AMBIGUOUS_MATCH candidates unconditionally, not gated behind --debug', async () => { diff --git a/src/commands/output/error.ts b/src/commands/output/error.ts index 084a4e1546..de39ced46a 100644 --- a/src/commands/output/error.ts +++ b/src/commands/output/error.ts @@ -6,6 +6,7 @@ import { type NormalizedError, } from '@agent-device/kernel/errors'; import { formatReplayDivergenceReport } from '@agent-device/ad-replay/divergence'; +import { collapseWarningText } from '../output-common.ts'; export function printHumanError( err: AppError | NormalizedError, @@ -20,6 +21,12 @@ export function printHumanError( if (normalized.hint) { process.stderr.write(`Hint: ${normalized.hint}\n`); } + // Composable warnings (skipped `optional` steps, capture degradations) describe the run, + // not this step's cause, so they ride at error level and must reach the reader of a + // failed run too — a warning-only trace is otherwise invisible outside --json (#2560). + for (const warning of readResponseWarnings(normalized.details)) { + process.stderr.write(`Warning: ${collapseWarningText(warning)}\n`); + } const candidateLines = formatErrorCandidateViews(readErrorCandidateViews(normalized.details)); if (candidateLines.length > 0) { process.stderr.write(`${candidateLines.join('\n')}\n`); @@ -47,6 +54,13 @@ export function printHumanError( } } +function readResponseWarnings(details: Record | undefined): string[] { + const warnings = details?.warnings; + return Array.isArray(warnings) + ? warnings.filter((warning): warning is string => typeof warning === 'string') + : []; +} + function formatErrorCandidateViews(views: ErrorCandidateView[]): string[] { return views.flatMap((view) => { if (view.kind === 'element-match') { diff --git a/src/daemon/replay/internal/__tests__/session-replay-maestro-failure.test.ts b/src/daemon/replay/internal/__tests__/session-replay-maestro-failure.test.ts index 00e06c4beb..f2f8072c93 100644 --- a/src/daemon/replay/internal/__tests__/session-replay-maestro-failure.test.ts +++ b/src/daemon/replay/internal/__tests__/session-replay-maestro-failure.test.ts @@ -96,11 +96,9 @@ test('typed Maestro failure response carries warnings accumulated before the fai { warnings: ['Optional Maestro assertVisible skipped at line 2: no match'] }, ); + if (!response.error.details) throw new Error('expected error details on the failure response'); const details = response.error.details; - expect(details).toBeDefined(); - expect(details.warnings).toEqual([ - 'Optional Maestro assertVisible skipped at line 2: no match', - ]); + expect(details.warnings).toEqual(['Optional Maestro assertVisible skipped at line 2: no match']); expect((details.divergence as { kind: string }).kind).toBe('action-failure'); }); diff --git a/src/daemon/replay/internal/session-replay-runtime-failure-response.ts b/src/daemon/replay/internal/session-replay-runtime-failure-response.ts index 3f7dd74a05..934ade1ac6 100644 --- a/src/daemon/replay/internal/session-replay-runtime-failure-response.ts +++ b/src/daemon/replay/internal/session-replay-runtime-failure-response.ts @@ -104,7 +104,9 @@ export function buildReplayDivergenceFailureResponseFromDescriptor(params: { positionals, artifactPaths, ...(snapshotDiagnostics ? { snapshotDiagnostics } : {}), - ...(warnings && warnings.length > 0 ? { warnings: [...warnings] } : {}), + ...(warnings && warnings.length > 0 + ? { warnings: warnings.map((warning) => scrubReplayVarValues(warning, scrubVars)) } + : {}), divergence, }, }, diff --git a/website/docs/docs/replay-e2e.md b/website/docs/docs/replay-e2e.md index 6933f520e4..f41d3c908e 100644 --- a/website/docs/docs/replay-e2e.md +++ b/website/docs/docs/replay-e2e.md @@ -123,7 +123,7 @@ agent-device test ./workflows --reporter default --reporter junit:./tmp/junit.xm - `replay-timing.ndjson` records attempt, cleanup, and per-step start/stop events with durations. Upload it from CI even for passing runs when comparing local and CI performance. - Timeouts are cooperative: the runner marks the attempt failed at the timeout boundary, then gives the underlying replay a short grace period to stop before session cleanup. - The default text reporter streams live progress on stderr while a suite runs, then prints the final summary, failed tests, and passed-on-retry flaky tests. Use `--verbose` to include step traces in completed-test progress output. -- The default reporter prints a `Warnings:` section after the summary when any test accumulated composable warnings — for example a Maestro step with `optional: true` that was skipped — whether that test passed or failed. `--json` carries the same strings in each test result's `warnings` array. +- The default reporter prints a `Warnings:` section after the summary when any test accumulated composable warnings — for example a Maestro step with `optional: true` that was skipped — whether that test passed or failed. A failing `replay` run repeats the warnings it accumulated as `Warning:` lines after the error. `--json` carries the same strings in each test result's `warnings` array. - `--reporter` is repeatable. Built-ins are `default` for the console summary and `junit:` for JUnit XML. Passing any explicit reporter list replaces the implicit default reporter, so include `--reporter default` when you also want terminal output. `--report-junit ` remains a compatibility alias for `--reporter junit:`. - JUnit reports preserve legal Unicode and whitespace, and replace characters forbidden by XML 1.0 (such as terminal ESC or NUL) with `U+FFFD` (`�`) so CI parsers can read the report. JSON and other reporters retain the original suite values. - When `--fail-fast` and retries are both set, the current test still consumes its retries before the suite stops. From 019a752cc53a85730d93d094ff90c5c2d197aa8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 13 Sep 2026 21:00:51 +0200 Subject: [PATCH 3/4] refactor(replay-test): make an attempt failure's warnings channel always present MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An empty array and an absent key said the same thing two ways, so every consumer needed both guards. The failed outcome now always carries the warnings array, like the passed outcome already did, and the result builder narrows the attempt once instead of per field. This also removes the clone the shared candidate-view formatter had grown between the CLI and MCP error renderers — the MCP error text gained the same warnings lines the CLI stderr prints. --- .../__tests__/session-test-artifacts.test.ts | 4 +++ .../__tests__/session-test-attempt.test.ts | 1 + .../__tests__/session-test-runtime.test.ts | 3 +++ .../src/internal/session-test-attempt.ts | 19 ++++++++----- .../src/internal/session-test-types.ts | 4 ++- src/commands/output/error.ts | 4 +-- .../__tests__/session-test-outcome.test.ts | 6 ++--- .../replay/internal/session-test-outcome.ts | 3 +-- src/mcp/__tests__/tool-error.test.ts | 16 +++++++++++ src/mcp/tool-error.ts | 27 ++++--------------- 10 files changed, 50 insertions(+), 37 deletions(-) diff --git a/packages/replay-test/src/internal/__tests__/session-test-artifacts.test.ts b/packages/replay-test/src/internal/__tests__/session-test-artifacts.test.ts index e90714f70a..7182ecc664 100644 --- a/packages/replay-test/src/internal/__tests__/session-test-artifacts.test.ts +++ b/packages/replay-test/src/internal/__tests__/session-test-artifacts.test.ts @@ -106,6 +106,7 @@ test('materializeReplayTestAttemptArtifacts writes failure manifest and copies l details: { reason: 'timeout', artifactPaths: [screenshotPath] }, }, artifactPaths: [screenshotPath], + warnings: [], infrastructure: false, }, filePath: replayPath, @@ -189,6 +190,7 @@ test('materialization preserves replay sources and diagnostics named after attem status: 'failed', error: { code: 'COMMAND_FAILED', message: 'original failure' }, artifactPaths, + warnings: [], infrastructure: false, }, filePath: replayPath, @@ -267,6 +269,7 @@ test('materialization copies a log listed in both the outcome and error only onc status: 'failed', error: { code: 'COMMAND_FAILED', message: 'failed', logPath }, artifactPaths: [logPath, logPath], + warnings: [], infrastructure: false, }, filePath: replayPath, @@ -300,6 +303,7 @@ test.each(['result.txt', 'failure.txt', 'RESULT.TXT'])( status: 'failed', error: { code: 'COMMAND_FAILED', message: 'original failure' }, artifactPaths: [diagnosticPath], + warnings: [], infrastructure: false, }, filePath: replayPath, diff --git a/packages/replay-test/src/internal/__tests__/session-test-attempt.test.ts b/packages/replay-test/src/internal/__tests__/session-test-attempt.test.ts index d98771b26b..a9c4fefbc5 100644 --- a/packages/replay-test/src/internal/__tests__/session-test-attempt.test.ts +++ b/packages/replay-test/src/internal/__tests__/session-test-attempt.test.ts @@ -22,6 +22,7 @@ const FAILED_WITHOUT_WARNINGS: ReplayTestAttemptOutcome = { status: 'failed', error: { code: 'COMMAND_FAILED', message: 'tap failed' }, artifactPaths: [], + warnings: [], infrastructure: false, }; diff --git a/packages/replay-test/src/internal/__tests__/session-test-runtime.test.ts b/packages/replay-test/src/internal/__tests__/session-test-runtime.test.ts index d6fa795565..fd88a91b33 100644 --- a/packages/replay-test/src/internal/__tests__/session-test-runtime.test.ts +++ b/packages/replay-test/src/internal/__tests__/session-test-runtime.test.ts @@ -117,6 +117,7 @@ test('runReplayTestAttempt keeps cancellation active until a timed-out replay se status: 'failed', error: { code: 'COMMAND_FAILED', message: 'request canceled' }, artifactPaths: [], + warnings: [], infrastructure: false, }); await replaySettled; @@ -142,6 +143,7 @@ test('runReplayTestAttempt keeps a passing replay passed when finalization fails status: 'failed', error: { code: 'COMMAND_FAILED', message: 'failed to stop recording' }, artifactPaths: [], + warnings: [], infrastructure: false, }), cleanupSession, @@ -167,6 +169,7 @@ test('runReplayTestAttempt marks a failed cleanup as infrastructure so the sched status: 'failed', error: { code: 'COMMAND_FAILED', message: 'open "System Settings" failed' }, artifactPaths: [], + warnings: [], infrastructure: false, }), cleanupSession, diff --git a/packages/replay-test/src/internal/session-test-attempt.ts b/packages/replay-test/src/internal/session-test-attempt.ts index 5f40bf14ff..1fae791e6a 100644 --- a/packages/replay-test/src/internal/session-test-attempt.ts +++ b/packages/replay-test/src/internal/session-test-attempt.ts @@ -388,16 +388,21 @@ function buildReplayTestFailedResult( attempts: outcome.attempts, artifactsDir: context.testArtifactsDir, error, - ...(attemptOutcome?.status === 'failed' && (attemptOutcome.warnings?.length ?? 0) > 0 - ? { warnings: [...attemptOutcome.warnings!] } - : {}), - ...(attemptOutcome?.status === 'failed' && attemptOutcome.infrastructure - ? { infrastructure: true as const } - : {}), + ...replayTestFailedAttemptFields(attemptOutcome), + ...replayTestShardResultMetadata(shard), + }; +} + +function replayTestFailedAttemptFields( + attemptOutcome: ReplayTestCaseOutcome['finalOutcome'], +): Pick { + const failed = attemptOutcome?.status === 'failed' ? attemptOutcome : undefined; + return { + ...(failed && failed.warnings.length > 0 ? { warnings: [...failed.warnings] } : {}), + ...(failed?.infrastructure ? { infrastructure: true as const } : {}), ...(attemptOutcome?.snapshotDiagnostics ? { snapshotDiagnostics: attemptOutcome.snapshotDiagnostics } : {}), - ...replayTestShardResultMetadata(shard), }; } diff --git a/packages/replay-test/src/internal/session-test-types.ts b/packages/replay-test/src/internal/session-test-types.ts index 723016235c..a56a59dac4 100644 --- a/packages/replay-test/src/internal/session-test-types.ts +++ b/packages/replay-test/src/internal/session-test-types.ts @@ -145,7 +145,7 @@ export type ReplayTestAttemptFailed = { error: ReplayTestAttemptError; artifactPaths: readonly string[]; /** Warnings accumulated before the failing step (skipped `optional` steps, capture degradations). */ - warnings?: readonly string[]; + warnings: readonly string[]; snapshotDiagnostics?: SnapshotDiagnosticsSummary; /** * The host's verdict that this failure is environmental (device/runner/boot) rather than a @@ -276,6 +276,7 @@ export type ReplayTestExecutionDependencies = Omit< export function replayTestAttemptFailure(params: { error: ReplayTestAttemptError; artifactPaths?: readonly string[]; + warnings?: readonly string[]; infrastructure?: boolean; snapshotDiagnostics?: SnapshotDiagnosticsSummary; }): ReplayTestAttemptFailed { @@ -283,6 +284,7 @@ export function replayTestAttemptFailure(params: { status: 'failed', error: params.error, artifactPaths: params.artifactPaths ?? [], + warnings: params.warnings ?? [], infrastructure: params.infrastructure ?? false, ...(params.snapshotDiagnostics ? { snapshotDiagnostics: params.snapshotDiagnostics } : {}), }; diff --git a/src/commands/output/error.ts b/src/commands/output/error.ts index de39ced46a..f718161060 100644 --- a/src/commands/output/error.ts +++ b/src/commands/output/error.ts @@ -54,14 +54,14 @@ export function printHumanError( } } -function readResponseWarnings(details: Record | undefined): string[] { +export function readResponseWarnings(details: Record | undefined): string[] { const warnings = details?.warnings; return Array.isArray(warnings) ? warnings.filter((warning): warning is string => typeof warning === 'string') : []; } -function formatErrorCandidateViews(views: ErrorCandidateView[]): string[] { +export function formatErrorCandidateViews(views: ErrorCandidateView[]): string[] { return views.flatMap((view) => { if (view.kind === 'element-match') { const remaining = view.matches - view.candidates.length; diff --git a/src/daemon/replay/internal/__tests__/session-test-outcome.test.ts b/src/daemon/replay/internal/__tests__/session-test-outcome.test.ts index ff0c5faf87..93e2c71749 100644 --- a/src/daemon/replay/internal/__tests__/session-test-outcome.test.ts +++ b/src/daemon/replay/internal/__tests__/session-test-outcome.test.ts @@ -25,18 +25,18 @@ test('failed attempt outcome carries warnings from the error details (#2560)', ( }); }); -test('failed attempt outcome omits absent or empty warnings', () => { +test('failed attempt outcome reads an empty warnings array when absent or non-string', () => { const withoutWarnings = toReplayTestAttemptOutcome({ ok: false, error: { code: 'COMMAND_FAILED', message: 'step failed' }, }); - expect('warnings' in withoutWarnings).toBe(false); + expect(withoutWarnings.status === 'failed' && withoutWarnings.warnings).toEqual([]); const emptyWarnings = toReplayTestAttemptOutcome({ ok: false, error: { code: 'COMMAND_FAILED', message: 'step failed', details: { warnings: [7] } }, }); - expect('warnings' in emptyWarnings).toBe(false); + expect(emptyWarnings.status === 'failed' && emptyWarnings.warnings).toEqual([]); }); test('passed attempt outcome keeps reading warnings from response data', () => { diff --git a/src/daemon/replay/internal/session-test-outcome.ts b/src/daemon/replay/internal/session-test-outcome.ts index 3d69e6a7c0..026ff1e3dd 100644 --- a/src/daemon/replay/internal/session-test-outcome.ts +++ b/src/daemon/replay/internal/session-test-outcome.ts @@ -14,12 +14,11 @@ import type { ReplayTestAttemptFailed, ReplayTestAttemptOutcome } from '@agent-d */ export function toReplayTestAttemptOutcome(response: DaemonResponse): ReplayTestAttemptOutcome { if (!response.ok) { - const warnings = readStringArray(response.error.details?.warnings); return { status: 'failed', error: response.error, artifactPaths: readArtifactPaths(response.error.details?.artifactPaths), - ...(warnings.length > 0 ? { warnings } : {}), + warnings: readStringArray(response.error.details?.warnings), infrastructure: isReplayInfrastructureFailure(response), ...snapshotDiagnostics(response.error.details?.snapshotDiagnostics), }; diff --git a/src/mcp/__tests__/tool-error.test.ts b/src/mcp/__tests__/tool-error.test.ts index 7bad324aea..9caea36394 100644 --- a/src/mcp/__tests__/tool-error.test.ts +++ b/src/mcp/__tests__/tool-error.test.ts @@ -36,6 +36,22 @@ test('formatToolErrorText omits the candidates block for non-ambiguous errors', const text = formatToolErrorText(normalizeToolError(err)); assert.equal(text.includes('Candidates:'), false); + assert.equal(text.includes('Warning:'), false); +}); + +// #2560: a failed `replay` carries the run's accumulated warnings at error level; +// the MCP reader must see them too, not only --json consumers. +test('formatToolErrorText renders run-level warnings carried in error details', () => { + const err = new AppError('REPLAY_DIVERGENCE', 'Replay failed at step 2 (tapOn "Save")', { + warnings: ['Optional Maestro assertVisible skipped at line 1: no match'], + }); + + const text = formatToolErrorText(normalizeToolError(err)); + + assert.match( + text, + /^Error \(REPLAY_DIVERGENCE\)[\s\S]*\nWarning: Optional Maestro assertVisible skipped at line 1: no match/, + ); }); test('formatToolErrorText renders a structured cause', () => { diff --git a/src/mcp/tool-error.ts b/src/mcp/tool-error.ts index 27f7281300..2b53343591 100644 --- a/src/mcp/tool-error.ts +++ b/src/mcp/tool-error.ts @@ -1,10 +1,11 @@ import { normalizeError, readErrorCandidateViews, - type ErrorCandidateView, type NormalizedError, } from '@agent-device/kernel/errors'; import { formatReplayDivergenceReport } from '@agent-device/ad-replay/divergence'; +import { formatErrorCandidateViews, readResponseWarnings } from '../commands/output/error.ts'; +import { collapseWarningText } from '../commands/output-common.ts'; export function normalizeToolError(error: unknown): NormalizedError { return normalizeError(error); @@ -17,30 +18,12 @@ export function formatToolErrorText(normalized: NormalizedError): string { lines.push(`Cause: ${code}${normalized.cause.message}`); } if (normalized.hint) lines.push(`Hint: ${normalized.hint}`); + for (const warning of readResponseWarnings(normalized.details)) { + lines.push(`Warning: ${collapseWarningText(warning)}`); + } lines.push(...formatErrorCandidateViews(readErrorCandidateViews(normalized.details))); if (normalized.supportedOn) lines.push(`Supported on: ${normalized.supportedOn}`); const divergence = formatReplayDivergenceReport(normalized.details); if (divergence) lines.push(divergence); return lines.join('\n'); } - -function formatErrorCandidateViews(views: ErrorCandidateView[]): string[] { - return views.flatMap((view) => { - if (view.kind === 'element-match') { - const remaining = view.matches - view.candidates.length; - return [ - 'Candidates:', - ...view.candidates.map( - (candidate) => ` ${pinCandidateLine(candidate, view.refsGeneration)}`, - ), - ...(remaining > 0 ? [` +${remaining} more`] : []), - ]; - } - return ['Devices:', ...view.devices.map((device) => ` ${device.id} ${device.name}`)]; - }); -} - -function pinCandidateLine(candidate: string, generation: number | undefined): string { - if (generation === undefined) return candidate; - return candidate.replace(/^@(e\d+)(?=\s|$)/, `@$1~s${generation}`); -} From 262b68a1b88f8fe0e2eb38933b41d5362c8f9238 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 14:47:01 +0200 Subject: [PATCH 4/4] refactor(kernel): own the response-warnings field contract beside the message reader Four call sites read the composable warnings field with their own Array.isArray filters (the daemon append, the daemon attempt outcome, the CLI success line, the CLI/MCP error text), and the PR had grown a fifth. The field has one contract now: kernel/success-text readResponseWarnings, next to readCommandMessage, with the daemon append, the attempt outcome, messageWithWarningsText, and both error renderers as its named consumers. The CLI-side reader the PR introduced is deleted, not wrapped. --- .../src/snapshot-capture-annotations.test.ts | 31 +++++++++++++++++++ .../src/snapshot-capture-annotations.ts | 3 ++ packages/kernel/src/success-text.test.ts | 12 +++++++ packages/kernel/src/success-text.ts | 22 +++++++++++++ src/agent-device-client.ts | 5 ++- src/client/screenshot-result.ts | 11 ++++--- src/commands/output-common.ts | 6 ++-- src/commands/output/error.ts | 8 +---- src/commands/output/snapshot.ts | 10 ++---- .../__tests__/snapshot-quality-latch.test.ts | 17 ++++++++++ .../replay/internal/session-test-outcome.ts | 5 +-- .../__tests__/session-open-warnings.test.ts | 16 +++++----- .../internal/session-open-foreground.ts | 2 +- .../internal/session-open-warnings.ts | 9 ++---- src/daemon/snapshot-quality-latch.ts | 4 +-- src/mcp/tool-error.ts | 3 +- 16 files changed, 119 insertions(+), 45 deletions(-) create mode 100644 packages/contracts/src/snapshot-capture-annotations.test.ts create mode 100644 packages/kernel/src/success-text.test.ts diff --git a/packages/contracts/src/snapshot-capture-annotations.test.ts b/packages/contracts/src/snapshot-capture-annotations.test.ts new file mode 100644 index 0000000000..2deeb0f761 --- /dev/null +++ b/packages/contracts/src/snapshot-capture-annotations.test.ts @@ -0,0 +1,31 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { readResponseWarnings } from '@agent-device/kernel/success-text'; +import { readSerializedSnapshotCaptureAnnotations } from './snapshot-capture-annotations.ts'; + +test('the annotations filter and the shared warnings parser agree on adversarial arrays', () => { + for (const warnings of [ + ['a note'], + ['a note', 42, { nested: true }, null], + ['', 'kept'], + ['a note', ''], + ]) { + assert.deepEqual( + readSerializedSnapshotCaptureAnnotations({ warnings }).warnings, + readResponseWarnings({ warnings }), + `drift for ${JSON.stringify(warnings)}`, + ); + } +}); + +test('an empty warnings array serializes back to absent', () => { + assert.equal(readSerializedSnapshotCaptureAnnotations({ warnings: [] }).warnings, undefined); +}); + +test('absent or non-array warnings stay absent on the serialized annotations', () => { + assert.equal(readSerializedSnapshotCaptureAnnotations({}).warnings, undefined); + assert.equal( + readSerializedSnapshotCaptureAnnotations({ warnings: 'a note' }).warnings, + undefined, + ); +}); diff --git a/packages/contracts/src/snapshot-capture-annotations.ts b/packages/contracts/src/snapshot-capture-annotations.ts index 4aebaf6787..3ccbc6bcf8 100644 --- a/packages/contracts/src/snapshot-capture-annotations.ts +++ b/packages/contracts/src/snapshot-capture-annotations.ts @@ -57,6 +57,9 @@ export function readSerializedSnapshotCaptureAnnotations( data: Record, ): PublicSnapshotCaptureAnnotations { const androidSnapshot = readObject(data.androidSnapshot); + // Declared exception to kernel's shared `readResponseWarnings` (see its doc): this facade + // pins its eager module closure, and absent-or-non-array keeps the serialized tri-state. + // `snapshot-capture-annotations.test.ts` cross-checks this filter against the shared parser. const warnings = Array.isArray(data.warnings) ? data.warnings.filter((entry): entry is string => typeof entry === 'string') : undefined; diff --git a/packages/kernel/src/success-text.test.ts b/packages/kernel/src/success-text.test.ts new file mode 100644 index 0000000000..197e07eb4b --- /dev/null +++ b/packages/kernel/src/success-text.test.ts @@ -0,0 +1,12 @@ +import assert from 'node:assert/strict'; +import { describe, test } from 'vitest'; +import { readCommandMessage } from './success-text.ts'; + +describe('readCommandMessage', () => { + test('an empty message is absent, not an empty success line', () => { + assert.equal(readCommandMessage({ message: '' }), null); + assert.equal(readCommandMessage({ message: 42 }), null); + assert.equal(readCommandMessage(undefined), null); + assert.equal(readCommandMessage({ message: 'Replayed 7 steps' }), 'Replayed 7 steps'); + }); +}); diff --git a/packages/kernel/src/success-text.ts b/packages/kernel/src/success-text.ts index 27687d3577..02ba7c607c 100644 --- a/packages/kernel/src/success-text.ts +++ b/packages/kernel/src/success-text.ts @@ -12,3 +12,25 @@ export function withSuccessText>( export function readCommandMessage(data: Record | undefined): string | null { return typeof data?.message === 'string' && data.message.length > 0 ? data.message : null; } + +/** + * The composable response-warnings channel (skipped `optional` steps, capture + * degradations): readers that project a response or error record onto note + * strings go through here — daemon append, attempt outcome, and + * recovered-quality latch, CLI success line, CLI/MCP error text, SDK client + * (open and screenshot result), and the + * snapshot text renderer — so one field contract has one parser. Consumers may + * add rendering rules on top (snapshot text and screenshot result drop empty + * notes; screenshot result keeps absent-means-undefined). The one declared + * exception is contracts' `readSerializedSnapshotCaptureAnnotations`, which + * keeps a local copy of the filter: contracts facades pin their eager module + * closure (`scripts/__tests__/eager-closure-budgets.test.ts`) and this module + * is outside it; its test cross-checks both parses so the contract cannot + * drift. Non-string entries are other producers' bugs. + */ +export function readResponseWarnings(data: Record | undefined): string[] { + const warnings = data?.warnings; + return Array.isArray(warnings) + ? warnings.filter((warning): warning is string => typeof warning === 'string') + : []; +} diff --git a/src/agent-device-client.ts b/src/agent-device-client.ts index f7bd6085a4..e3a0cd60af 100644 --- a/src/agent-device-client.ts +++ b/src/agent-device-client.ts @@ -72,6 +72,7 @@ import { type MetroSessionHints, } from './metro/metro-session-hints.ts'; import { isRecord } from '@agent-device/kernel/record'; +import { readResponseWarnings } from '@agent-device/kernel/success-text'; import { createLeaseClient } from './client/lease-client.ts'; import { normalizeScreenshotCaptureResult } from './client/screenshot-result.ts'; @@ -266,9 +267,7 @@ export function createAgentDeviceClient( const device = normalizeOpenDevice(data); const appBundleId = readOptionalString(data, 'appBundleId'); const appId = appBundleId; - const warnings = Array.isArray(data.warnings) - ? data.warnings.filter((warning): warning is string => typeof warning === 'string') - : []; + const warnings = readResponseWarnings(data); return { session, ...(warnings.length > 0 ? { warnings } : {}), diff --git a/src/client/screenshot-result.ts b/src/client/screenshot-result.ts index 7718cbffe1..47e7e32a93 100644 --- a/src/client/screenshot-result.ts +++ b/src/client/screenshot-result.ts @@ -1,6 +1,7 @@ import type { ScreenshotResultData } from '@agent-device/contracts/capture'; import type { CaptureScreenshotResult } from '@agent-device/contracts/client'; import { isRecord, parsePoint, parseRect, readRequiredString } from '@agent-device/kernel/record'; +import { readResponseWarnings } from '@agent-device/kernel/success-text'; import type { ScreenshotOverlayRef } from '@agent-device/kernel/snapshot'; export function pickScreenshotResultData(value: ScreenshotResultData): ScreenshotResultData { @@ -45,7 +46,7 @@ type ScreenshotOverlayRefData = { function readScreenshotResultData(value: unknown): ScreenshotResultData | undefined { if (!isRecord(value)) return undefined; - const warnings = readScreenshotWarnings(value.warnings); + const warnings = readScreenshotWarnings(value); return pickScreenshotResultData({ path: readStringField(value, 'path'), width: readNumberField(value, 'width'), @@ -76,9 +77,11 @@ function readScreenshotOverlayRefs(value: unknown): ScreenshotOverlayRef[] | und }); } -function readScreenshotWarnings(value: unknown): string[] | undefined { - if (!Array.isArray(value)) return undefined; - return value.filter((entry): entry is string => typeof entry === 'string' && entry.length > 0); +function readScreenshotWarnings(data: Record): string[] | undefined { + // An absent or non-array field is "no warnings channel on this result"; + // the field contract itself is the shared parser's. + if (!Array.isArray(data.warnings)) return undefined; + return readResponseWarnings(data).filter((warning) => warning.length > 0); } function readScreenshotOverlayRef( diff --git a/src/commands/output-common.ts b/src/commands/output-common.ts index 32b8147527..5cd9736671 100644 --- a/src/commands/output-common.ts +++ b/src/commands/output-common.ts @@ -1,4 +1,4 @@ -import { readCommandMessage } from '@agent-device/kernel/success-text'; +import { readCommandMessage, readResponseWarnings } from '@agent-device/kernel/success-text'; import type { CommandProgressState } from './command-progress.ts'; import type { CliOutput } from './command-contract.ts'; @@ -37,9 +37,7 @@ export function messageCliOutput(result: Record): CliOutput { */ export function messageWithWarningsText(result: Record): string | null { const message = readCommandMessage(result); - const warnings = Array.isArray(result.warnings) - ? result.warnings.filter((warning): warning is string => typeof warning === 'string') - : []; + const warnings = readResponseWarnings(result); if (warnings.length === 0) return message; return [message, ...warnings.map((warning) => `Warning: ${collapseWarningText(warning)}`)] .filter(Boolean) diff --git a/src/commands/output/error.ts b/src/commands/output/error.ts index f718161060..cca51ccd18 100644 --- a/src/commands/output/error.ts +++ b/src/commands/output/error.ts @@ -5,6 +5,7 @@ import { type ErrorCandidateView, type NormalizedError, } from '@agent-device/kernel/errors'; +import { readResponseWarnings } from '@agent-device/kernel/success-text'; import { formatReplayDivergenceReport } from '@agent-device/ad-replay/divergence'; import { collapseWarningText } from '../output-common.ts'; @@ -54,13 +55,6 @@ export function printHumanError( } } -export function readResponseWarnings(details: Record | undefined): string[] { - const warnings = details?.warnings; - return Array.isArray(warnings) - ? warnings.filter((warning): warning is string => typeof warning === 'string') - : []; -} - export function formatErrorCandidateViews(views: ErrorCandidateView[]): string[] { return views.flatMap((view) => { if (view.kind === 'element-match') { diff --git a/src/commands/output/snapshot.ts b/src/commands/output/snapshot.ts index ad9fe08b83..096ce4ae6e 100644 --- a/src/commands/output/snapshot.ts +++ b/src/commands/output/snapshot.ts @@ -14,6 +14,7 @@ import { type SnapshotUnchanged, type SnapshotVisibility, } from '@agent-device/kernel/snapshot'; +import { readResponseWarnings } from '@agent-device/kernel/success-text'; import { buildMobileSnapshotPresentation } from '@agent-device/capture-kit/mobile-snapshot-semantics'; type SnapshotTextOptions = { @@ -281,13 +282,8 @@ function formatSparseSnapshotHint( } export function readSnapshotWarnings(data: Record): string[] { - const rawWarnings = data.warnings; - if (!Array.isArray(rawWarnings)) { - return []; - } - return rawWarnings.filter( - (entry): entry is string => typeof entry === 'string' && entry.length > 0, - ); + // Snapshot text additionally drops empty notes; the field contract is the shared parser's. + return readResponseWarnings(data).filter((warning) => warning.length > 0); } type SnapshotDisplayLine = ReturnType[number]; diff --git a/src/daemon/__tests__/snapshot-quality-latch.test.ts b/src/daemon/__tests__/snapshot-quality-latch.test.ts index 9885fb8365..9f0335edce 100644 --- a/src/daemon/__tests__/snapshot-quality-latch.test.ts +++ b/src/daemon/__tests__/snapshot-quality-latch.test.ts @@ -165,6 +165,23 @@ test('sessionless responses pass through unchanged', () => { ).toBe(data); }); +test('the recovered warning rides the shared warnings channel and foreign entries are dropped', () => { + const session = makeIosSession('default', { appBundleId: 'com.example.app' }); + + const out = applyRecoveredWarningLatch({ + session, + data: { warnings: ['a note', 42, { nested: true }] }, + verdict: deferredVerdict(), + internalObservation: false, + }); + + const warnings = out.warnings as string[]; + expect(warnings).toHaveLength(2); + expect(typeof warnings[0]).toBe('string'); + expect(warnings[0]).not.toBe('a note'); + expect(warnings[1]).toBe('a note'); +}); + function scenario() { const root = path.join(os.tmpdir(), `agent-device-quality-latch-${crypto.randomUUID()}`); const sessionStore = new SessionStore(path.join(root, 'sessions')); diff --git a/src/daemon/replay/internal/session-test-outcome.ts b/src/daemon/replay/internal/session-test-outcome.ts index 026ff1e3dd..453e384a30 100644 --- a/src/daemon/replay/internal/session-test-outcome.ts +++ b/src/daemon/replay/internal/session-test-outcome.ts @@ -1,4 +1,5 @@ import { readSnapshotDiagnosticsSummary } from '@agent-device/contracts/capture'; +import { readResponseWarnings } from '@agent-device/kernel/success-text'; import type { DaemonResponse } from '../../daemon-request.ts'; import { isReplayInfrastructureFailure } from './session-test-infrastructure.ts'; import type { ReplayTestAttemptFailed, ReplayTestAttemptOutcome } from '@agent-device/replay-test'; @@ -18,7 +19,7 @@ export function toReplayTestAttemptOutcome(response: DaemonResponse): ReplayTest status: 'failed', error: response.error, artifactPaths: readArtifactPaths(response.error.details?.artifactPaths), - warnings: readStringArray(response.error.details?.warnings), + warnings: readResponseWarnings(response.error.details), infrastructure: isReplayInfrastructureFailure(response), ...snapshotDiagnostics(response.error.details?.snapshotDiagnostics), }; @@ -28,7 +29,7 @@ export function toReplayTestAttemptOutcome(response: DaemonResponse): ReplayTest status: 'passed', replayed: typeof data?.replayed === 'number' ? data.replayed : 0, healed: typeof data?.healed === 'number' ? data.healed : 0, - warnings: readStringArray(data?.warnings), + warnings: readResponseWarnings(data), artifactPaths: readArtifactPaths(data?.artifactPaths), ...snapshotDiagnostics(data?.snapshotDiagnostics), }; diff --git a/src/daemon/session-lifecycle/internal/__tests__/session-open-warnings.test.ts b/src/daemon/session-lifecycle/internal/__tests__/session-open-warnings.test.ts index 5b59ddfee3..3fdf0b3e15 100644 --- a/src/daemon/session-lifecycle/internal/__tests__/session-open-warnings.test.ts +++ b/src/daemon/session-lifecycle/internal/__tests__/session-open-warnings.test.ts @@ -1,6 +1,6 @@ import assert from 'node:assert/strict'; import { test } from 'vitest'; -import { appendResponseWarning, readResponseWarnings } from '../session-open-warnings.ts'; +import { appendResponseWarning } from '../session-open-warnings.ts'; test('a producer adds its note without dropping the notes already on the response', () => { const responseData: Record = { warnings: ['the session is already open'] }; @@ -21,10 +21,12 @@ test('a response that carries no warnings yet starts from an empty list', () => assert.deepEqual(responseData.warnings, ['the device was taken over']); }); -test('reading warnings ignores anything that is not a note', () => { - assert.deepEqual(readResponseWarnings({ warnings: ['a note', 42, { nested: true }, null] }), [ - 'a note', - ]); - assert.deepEqual(readResponseWarnings({}), []); - assert.deepEqual(readResponseWarnings(undefined), []); +test('accumulating onto non-note entries keeps only the notes and the new one', () => { + const responseData: Record = { + warnings: ['a note', 42, { nested: true }, null], + }; + + appendResponseWarning(responseData, 'the device was taken over'); + + assert.deepEqual(responseData.warnings, ['a note', 'the device was taken over']); }); diff --git a/src/daemon/session-lifecycle/internal/session-open-foreground.ts b/src/daemon/session-lifecycle/internal/session-open-foreground.ts index e9284455b4..5d5b97afd9 100644 --- a/src/daemon/session-lifecycle/internal/session-open-foreground.ts +++ b/src/daemon/session-lifecycle/internal/session-open-foreground.ts @@ -7,7 +7,7 @@ import type { InspectDeviceRuntimeFacts, } from '../../request-runtime-binding.ts'; import { errorResponse } from '../../response.ts'; -import { readResponseWarnings } from './session-open-warnings.ts'; +import { readResponseWarnings } from '@agent-device/kernel/success-text'; export type ForegroundOpenResolution = | { type: 'not-requested' } diff --git a/src/daemon/session-lifecycle/internal/session-open-warnings.ts b/src/daemon/session-lifecycle/internal/session-open-warnings.ts index ed30e662ca..05bfbfa9db 100644 --- a/src/daemon/session-lifecycle/internal/session-open-warnings.ts +++ b/src/daemon/session-lifecycle/internal/session-open-warnings.ts @@ -1,3 +1,5 @@ +import { readResponseWarnings } from '@agent-device/kernel/success-text'; + /** * Response-level warnings accumulate: every `open` producer adds its own note and keeps the ones * already there, so a producer never has to know which other note ran first. @@ -8,10 +10,3 @@ export function appendResponseWarning( ): void { responseData.warnings = [...readResponseWarnings(responseData), warning]; } - -export function readResponseWarnings(responseData: Record | undefined): string[] { - const warnings = responseData?.warnings; - return Array.isArray(warnings) - ? warnings.filter((warning): warning is string => typeof warning === 'string') - : []; -} diff --git a/src/daemon/snapshot-quality-latch.ts b/src/daemon/snapshot-quality-latch.ts index 3b46fc031b..95f2b2e2c8 100644 --- a/src/daemon/snapshot-quality-latch.ts +++ b/src/daemon/snapshot-quality-latch.ts @@ -1,4 +1,5 @@ import type { SnapshotQualityVerdict } from '@agent-device/kernel/snapshot'; +import { readResponseWarnings } from '@agent-device/kernel/success-text'; import { recoveredSnapshotQualityWarning } from '@agent-device/capture-kit/quality-warnings'; import type { DaemonResponseData } from './daemon-request.ts'; import type { SessionState } from './session-state.ts'; @@ -92,6 +93,5 @@ export function applyRecoveredWarningLatch(params: { }); session.recoveredSnapshotWarningLatch = decision.latch; if (!decision.warning) return data; - const warnings = Array.isArray(data.warnings) ? data.warnings : []; - return { ...data, warnings: [decision.warning, ...warnings] }; + return { ...data, warnings: [decision.warning, ...readResponseWarnings(data)] }; } diff --git a/src/mcp/tool-error.ts b/src/mcp/tool-error.ts index 2b53343591..a923ac785e 100644 --- a/src/mcp/tool-error.ts +++ b/src/mcp/tool-error.ts @@ -4,7 +4,8 @@ import { type NormalizedError, } from '@agent-device/kernel/errors'; import { formatReplayDivergenceReport } from '@agent-device/ad-replay/divergence'; -import { formatErrorCandidateViews, readResponseWarnings } from '../commands/output/error.ts'; +import { readResponseWarnings } from '@agent-device/kernel/success-text'; +import { formatErrorCandidateViews } from '../commands/output/error.ts'; import { collapseWarningText } from '../commands/output-common.ts'; export function normalizeToolError(error: unknown): NormalizedError {