refactor(replay-test): make an attempt failure's warnings channel always present

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.
This commit is contained in:
Michał Pierzchała
2026-09-13 21:00:51 +02:00
parent c1c3d30022
commit 4932e0f15e
10 changed files with 50 additions and 37 deletions
@@ -106,6 +106,7 @@ test('materializeReplayTestAttemptArtifacts writes failure manifest and copies l
details: { reason: 'timeout', artifactPaths: [screenshotPath] }, details: { reason: 'timeout', artifactPaths: [screenshotPath] },
}, },
artifactPaths: [screenshotPath], artifactPaths: [screenshotPath],
warnings: [],
infrastructure: false, infrastructure: false,
}, },
filePath: replayPath, filePath: replayPath,
@@ -189,6 +190,7 @@ test('materialization preserves replay sources and diagnostics named after attem
status: 'failed', status: 'failed',
error: { code: 'COMMAND_FAILED', message: 'original failure' }, error: { code: 'COMMAND_FAILED', message: 'original failure' },
artifactPaths, artifactPaths,
warnings: [],
infrastructure: false, infrastructure: false,
}, },
filePath: replayPath, filePath: replayPath,
@@ -267,6 +269,7 @@ test('materialization copies a log listed in both the outcome and error only onc
status: 'failed', status: 'failed',
error: { code: 'COMMAND_FAILED', message: 'failed', logPath }, error: { code: 'COMMAND_FAILED', message: 'failed', logPath },
artifactPaths: [logPath, logPath], artifactPaths: [logPath, logPath],
warnings: [],
infrastructure: false, infrastructure: false,
}, },
filePath: replayPath, filePath: replayPath,
@@ -300,6 +303,7 @@ test.each(['result.txt', 'failure.txt', 'RESULT.TXT'])(
status: 'failed', status: 'failed',
error: { code: 'COMMAND_FAILED', message: 'original failure' }, error: { code: 'COMMAND_FAILED', message: 'original failure' },
artifactPaths: [diagnosticPath], artifactPaths: [diagnosticPath],
warnings: [],
infrastructure: false, infrastructure: false,
}, },
filePath: replayPath, filePath: replayPath,
@@ -22,6 +22,7 @@ const FAILED_WITHOUT_WARNINGS: ReplayTestAttemptOutcome = {
status: 'failed', status: 'failed',
error: { code: 'COMMAND_FAILED', message: 'tap failed' }, error: { code: 'COMMAND_FAILED', message: 'tap failed' },
artifactPaths: [], artifactPaths: [],
warnings: [],
infrastructure: false, infrastructure: false,
}; };
@@ -117,6 +117,7 @@ test('runReplayTestAttempt keeps cancellation active until a timed-out replay se
status: 'failed', status: 'failed',
error: { code: 'COMMAND_FAILED', message: 'request canceled' }, error: { code: 'COMMAND_FAILED', message: 'request canceled' },
artifactPaths: [], artifactPaths: [],
warnings: [],
infrastructure: false, infrastructure: false,
}); });
await replaySettled; await replaySettled;
@@ -142,6 +143,7 @@ test('runReplayTestAttempt keeps a passing replay passed when finalization fails
status: 'failed', status: 'failed',
error: { code: 'COMMAND_FAILED', message: 'failed to stop recording' }, error: { code: 'COMMAND_FAILED', message: 'failed to stop recording' },
artifactPaths: [], artifactPaths: [],
warnings: [],
infrastructure: false, infrastructure: false,
}), }),
cleanupSession, cleanupSession,
@@ -167,6 +169,7 @@ test('runReplayTestAttempt marks a failed cleanup as infrastructure so the sched
status: 'failed', status: 'failed',
error: { code: 'COMMAND_FAILED', message: 'open "System Settings" failed' }, error: { code: 'COMMAND_FAILED', message: 'open "System Settings" failed' },
artifactPaths: [], artifactPaths: [],
warnings: [],
infrastructure: false, infrastructure: false,
}), }),
cleanupSession, cleanupSession,
@@ -388,16 +388,21 @@ function buildReplayTestFailedResult(
attempts: outcome.attempts, attempts: outcome.attempts,
artifactsDir: context.testArtifactsDir, artifactsDir: context.testArtifactsDir,
error, error,
...(attemptOutcome?.status === 'failed' && (attemptOutcome.warnings?.length ?? 0) > 0 ...replayTestFailedAttemptFields(attemptOutcome),
? { warnings: [...attemptOutcome.warnings!] } ...replayTestShardResultMetadata(shard),
: {}), };
...(attemptOutcome?.status === 'failed' && attemptOutcome.infrastructure }
? { infrastructure: true as const }
: {}), function replayTestFailedAttemptFields(
attemptOutcome: ReplayTestCaseOutcome['finalOutcome'],
): Pick<ReplaySuiteTestFailed, 'warnings' | 'infrastructure' | 'snapshotDiagnostics'> {
const failed = attemptOutcome?.status === 'failed' ? attemptOutcome : undefined;
return {
...(failed && failed.warnings.length > 0 ? { warnings: [...failed.warnings] } : {}),
...(failed?.infrastructure ? { infrastructure: true as const } : {}),
...(attemptOutcome?.snapshotDiagnostics ...(attemptOutcome?.snapshotDiagnostics
? { snapshotDiagnostics: attemptOutcome.snapshotDiagnostics } ? { snapshotDiagnostics: attemptOutcome.snapshotDiagnostics }
: {}), : {}),
...replayTestShardResultMetadata(shard),
}; };
} }
@@ -145,7 +145,7 @@ export type ReplayTestAttemptFailed = {
error: ReplayTestAttemptError; error: ReplayTestAttemptError;
artifactPaths: readonly string[]; artifactPaths: readonly string[];
/** Warnings accumulated before the failing step (skipped `optional` steps, capture degradations). */ /** Warnings accumulated before the failing step (skipped `optional` steps, capture degradations). */
warnings?: readonly string[]; warnings: readonly string[];
snapshotDiagnostics?: SnapshotDiagnosticsSummary; snapshotDiagnostics?: SnapshotDiagnosticsSummary;
/** /**
* The host's verdict that this failure is environmental (device/runner/boot) rather than a * 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: { export function replayTestAttemptFailure(params: {
error: ReplayTestAttemptError; error: ReplayTestAttemptError;
artifactPaths?: readonly string[]; artifactPaths?: readonly string[];
warnings?: readonly string[];
infrastructure?: boolean; infrastructure?: boolean;
snapshotDiagnostics?: SnapshotDiagnosticsSummary; snapshotDiagnostics?: SnapshotDiagnosticsSummary;
}): ReplayTestAttemptFailed { }): ReplayTestAttemptFailed {
@@ -283,6 +284,7 @@ export function replayTestAttemptFailure(params: {
status: 'failed', status: 'failed',
error: params.error, error: params.error,
artifactPaths: params.artifactPaths ?? [], artifactPaths: params.artifactPaths ?? [],
warnings: params.warnings ?? [],
infrastructure: params.infrastructure ?? false, infrastructure: params.infrastructure ?? false,
...(params.snapshotDiagnostics ? { snapshotDiagnostics: params.snapshotDiagnostics } : {}), ...(params.snapshotDiagnostics ? { snapshotDiagnostics: params.snapshotDiagnostics } : {}),
}; };
+2 -2
View File
@@ -54,14 +54,14 @@ export function printHumanError(
} }
} }
function readResponseWarnings(details: Record<string, unknown> | undefined): string[] { export function readResponseWarnings(details: Record<string, unknown> | undefined): string[] {
const warnings = details?.warnings; const warnings = details?.warnings;
return Array.isArray(warnings) return Array.isArray(warnings)
? warnings.filter((warning): warning is string => typeof warning === 'string') ? warnings.filter((warning): warning is string => typeof warning === 'string')
: []; : [];
} }
function formatErrorCandidateViews(views: ErrorCandidateView[]): string[] { export function formatErrorCandidateViews(views: ErrorCandidateView[]): string[] {
return views.flatMap((view) => { return views.flatMap((view) => {
if (view.kind === 'element-match') { if (view.kind === 'element-match') {
const remaining = view.matches - view.candidates.length; const remaining = view.matches - view.candidates.length;
@@ -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({ const withoutWarnings = toReplayTestAttemptOutcome({
ok: false, ok: false,
error: { code: 'COMMAND_FAILED', message: 'step failed' }, error: { code: 'COMMAND_FAILED', message: 'step failed' },
}); });
expect('warnings' in withoutWarnings).toBe(false); expect(withoutWarnings.status === 'failed' && withoutWarnings.warnings).toEqual([]);
const emptyWarnings = toReplayTestAttemptOutcome({ const emptyWarnings = toReplayTestAttemptOutcome({
ok: false, ok: false,
error: { code: 'COMMAND_FAILED', message: 'step failed', details: { warnings: [7] } }, 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', () => { test('passed attempt outcome keeps reading warnings from response data', () => {
@@ -14,12 +14,11 @@ import type { ReplayTestAttemptFailed, ReplayTestAttemptOutcome } from '@agent-d
*/ */
export function toReplayTestAttemptOutcome(response: DaemonResponse): ReplayTestAttemptOutcome { export function toReplayTestAttemptOutcome(response: DaemonResponse): ReplayTestAttemptOutcome {
if (!response.ok) { if (!response.ok) {
const warnings = readStringArray(response.error.details?.warnings);
return { return {
status: 'failed', status: 'failed',
error: response.error, error: response.error,
artifactPaths: readArtifactPaths(response.error.details?.artifactPaths), artifactPaths: readArtifactPaths(response.error.details?.artifactPaths),
...(warnings.length > 0 ? { warnings } : {}), warnings: readStringArray(response.error.details?.warnings),
infrastructure: isReplayInfrastructureFailure(response), infrastructure: isReplayInfrastructureFailure(response),
...snapshotDiagnostics(response.error.details?.snapshotDiagnostics), ...snapshotDiagnostics(response.error.details?.snapshotDiagnostics),
}; };
+16
View File
@@ -36,6 +36,22 @@ test('formatToolErrorText omits the candidates block for non-ambiguous errors',
const text = formatToolErrorText(normalizeToolError(err)); const text = formatToolErrorText(normalizeToolError(err));
assert.equal(text.includes('Candidates:'), false); 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', () => { test('formatToolErrorText renders a structured cause', () => {
+5 -22
View File
@@ -1,10 +1,11 @@
import { import {
normalizeError, normalizeError,
readErrorCandidateViews, readErrorCandidateViews,
type ErrorCandidateView,
type NormalizedError, type NormalizedError,
} from '@agent-device/kernel/errors'; } from '@agent-device/kernel/errors';
import { formatReplayDivergenceReport } from '@agent-device/ad-replay/divergence'; 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 { export function normalizeToolError(error: unknown): NormalizedError {
return normalizeError(error); return normalizeError(error);
@@ -17,30 +18,12 @@ export function formatToolErrorText(normalized: NormalizedError): string {
lines.push(`Cause: ${code}${normalized.cause.message}`); lines.push(`Cause: ${code}${normalized.cause.message}`);
} }
if (normalized.hint) lines.push(`Hint: ${normalized.hint}`); 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))); lines.push(...formatErrorCandidateViews(readErrorCandidateViews(normalized.details)));
if (normalized.supportedOn) lines.push(`Supported on: ${normalized.supportedOn}`); if (normalized.supportedOn) lines.push(`Supported on: ${normalized.supportedOn}`);
const divergence = formatReplayDivergenceReport(normalized.details); const divergence = formatReplayDivergenceReport(normalized.details);
if (divergence) lines.push(divergence); if (divergence) lines.push(divergence);
return lines.join('\n'); 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}`);
}