Skip to content

Commit 4b15606

Browse files
committed
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.
1 parent ba88cc5 commit 4b15606

14 files changed

Lines changed: 99 additions & 43 deletions
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
import assert from 'node:assert/strict';
2+
import { test } from 'vitest';
3+
import { readResponseWarnings } from '@agent-device/kernel/success-text';
4+
import { readSerializedSnapshotCaptureAnnotations } from './snapshot-capture-annotations.ts';
5+
6+
test('the annotations filter and the shared warnings parser agree on adversarial arrays', () => {
7+
for (const warnings of [
8+
['a note'],
9+
['a note', 42, { nested: true }, null],
10+
['', 'kept'],
11+
['a note', ''],
12+
]) {
13+
assert.deepEqual(
14+
readSerializedSnapshotCaptureAnnotations({ warnings }).warnings,
15+
readResponseWarnings({ warnings }),
16+
`drift for ${JSON.stringify(warnings)}`,
17+
);
18+
}
19+
});
20+
21+
test('an empty warnings array serializes back to absent', () => {
22+
assert.equal(readSerializedSnapshotCaptureAnnotations({ warnings: [] }).warnings, undefined);
23+
});
24+
25+
test('absent or non-array warnings stay absent on the serialized annotations', () => {
26+
assert.equal(readSerializedSnapshotCaptureAnnotations({}).warnings, undefined);
27+
assert.equal(
28+
readSerializedSnapshotCaptureAnnotations({ warnings: 'a note' }).warnings,
29+
undefined,
30+
);
31+
});

packages/contracts/src/snapshot-capture-annotations.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,9 @@ export function readSerializedSnapshotCaptureAnnotations(
5757
data: Record<string, unknown>,
5858
): PublicSnapshotCaptureAnnotations {
5959
const androidSnapshot = readObject(data.androidSnapshot);
60+
// Declared exception to kernel's shared `readResponseWarnings` (see its doc): this facade
61+
// pins its eager module closure, and absent-or-non-array keeps the serialized tri-state.
62+
// `snapshot-capture-annotations.test.ts` cross-checks this filter against the shared parser.
6063
const warnings = Array.isArray(data.warnings)
6164
? data.warnings.filter((entry): entry is string => typeof entry === 'string')
6265
: undefined;
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import assert from 'node:assert/strict';
2+
import { describe, test } from 'vitest';
3+
import { readCommandMessage } from './success-text.ts';
4+
5+
describe('readCommandMessage', () => {
6+
test('an empty message is absent, not an empty success line', () => {
7+
assert.equal(readCommandMessage({ message: '' }), null);
8+
assert.equal(readCommandMessage({ message: 42 }), null);
9+
assert.equal(readCommandMessage(undefined), null);
10+
assert.equal(readCommandMessage({ message: 'Replayed 7 steps' }), 'Replayed 7 steps');
11+
});
12+
});

packages/kernel/src/success-text.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,24 @@ export function withSuccessText<T extends Record<string, unknown>>(
1212
export function readCommandMessage(data: Record<string, unknown> | undefined): string | null {
1313
return typeof data?.message === 'string' && data.message.length > 0 ? data.message : null;
1414
}
15+
16+
/**
17+
* The composable response-warnings channel (skipped `optional` steps, capture
18+
* degradations): readers that project a response or error record onto note
19+
* strings go through here — daemon append and attempt outcome, CLI success
20+
* line, CLI/MCP error text, SDK client (open and screenshot result), and the
21+
* snapshot text renderer — so one field contract has one parser. Consumers may
22+
* add rendering rules on top (snapshot text and screenshot result drop empty
23+
* notes; screenshot result keeps absent-means-undefined). The one declared
24+
* exception is contracts' `readSerializedSnapshotCaptureAnnotations`, which
25+
* keeps a local copy of the filter: contracts facades pin their eager module
26+
* closure (`scripts/__tests__/eager-closure-budgets.test.ts`) and this module
27+
* is outside it; its test cross-checks both parses so the contract cannot
28+
* drift. Non-string entries are other producers' bugs.
29+
*/
30+
export function readResponseWarnings(data: Record<string, unknown> | undefined): string[] {
31+
const warnings = data?.warnings;
32+
return Array.isArray(warnings)
33+
? warnings.filter((warning): warning is string => typeof warning === 'string')
34+
: [];
35+
}

src/agent-device-client.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ import {
7272
type MetroSessionHints,
7373
} from './metro/metro-session-hints.ts';
7474
import { isRecord } from '@agent-device/kernel/record';
75+
import { readResponseWarnings } from '@agent-device/kernel/success-text';
7576
import { createLeaseClient } from './client/lease-client.ts';
7677
import { normalizeScreenshotCaptureResult } from './client/screenshot-result.ts';
7778

@@ -266,9 +267,7 @@ export function createAgentDeviceClient(
266267
const device = normalizeOpenDevice(data);
267268
const appBundleId = readOptionalString(data, 'appBundleId');
268269
const appId = appBundleId;
269-
const warnings = Array.isArray(data.warnings)
270-
? data.warnings.filter((warning): warning is string => typeof warning === 'string')
271-
: [];
270+
const warnings = readResponseWarnings(data);
272271
return {
273272
session,
274273
...(warnings.length > 0 ? { warnings } : {}),

src/client/screenshot-result.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { ScreenshotResultData } from '@agent-device/contracts/capture';
22
import type { CaptureScreenshotResult } from '@agent-device/contracts/client';
33
import { isRecord, parsePoint, parseRect, readRequiredString } from '@agent-device/kernel/record';
4+
import { readResponseWarnings } from '@agent-device/kernel/success-text';
45
import type { ScreenshotOverlayRef } from '@agent-device/kernel/snapshot';
56

67
export function pickScreenshotResultData(value: ScreenshotResultData): ScreenshotResultData {
@@ -45,7 +46,7 @@ type ScreenshotOverlayRefData = {
4546

4647
function readScreenshotResultData(value: unknown): ScreenshotResultData | undefined {
4748
if (!isRecord(value)) return undefined;
48-
const warnings = readScreenshotWarnings(value.warnings);
49+
const warnings = readScreenshotWarnings(value);
4950
return pickScreenshotResultData({
5051
path: readStringField(value, 'path'),
5152
width: readNumberField(value, 'width'),
@@ -76,9 +77,11 @@ function readScreenshotOverlayRefs(value: unknown): ScreenshotOverlayRef[] | und
7677
});
7778
}
7879

79-
function readScreenshotWarnings(value: unknown): string[] | undefined {
80-
if (!Array.isArray(value)) return undefined;
81-
return value.filter((entry): entry is string => typeof entry === 'string' && entry.length > 0);
80+
function readScreenshotWarnings(data: Record<string, unknown>): string[] | undefined {
81+
// An absent or non-array field is "no warnings channel on this result";
82+
// the field contract itself is the shared parser's.
83+
if (!Array.isArray(data.warnings)) return undefined;
84+
return readResponseWarnings(data).filter((warning) => warning.length > 0);
8285
}
8386

8487
function readScreenshotOverlayRef(

src/commands/output-common.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { readCommandMessage } from '@agent-device/kernel/success-text';
1+
import { readCommandMessage, readResponseWarnings } from '@agent-device/kernel/success-text';
22
import type { CommandProgressState } from './command-progress.ts';
33
import type { CliOutput } from './command-contract.ts';
44

@@ -37,9 +37,7 @@ export function messageCliOutput(result: Record<string, unknown>): CliOutput {
3737
*/
3838
export function messageWithWarningsText(result: Record<string, unknown>): string | null {
3939
const message = readCommandMessage(result);
40-
const warnings = Array.isArray(result.warnings)
41-
? result.warnings.filter((warning): warning is string => typeof warning === 'string')
42-
: [];
40+
const warnings = readResponseWarnings(result);
4341
if (warnings.length === 0) return message;
4442
return [message, ...warnings.map((warning) => `Warning: ${collapseWarningText(warning)}`)]
4543
.filter(Boolean)

src/commands/output/error.ts

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
type ErrorCandidateView,
66
type NormalizedError,
77
} from '@agent-device/kernel/errors';
8+
import { readResponseWarnings } from '@agent-device/kernel/success-text';
89
import { formatReplayDivergenceReport } from '@agent-device/ad-replay/divergence';
910
import { collapseWarningText } from '../output-common.ts';
1011

@@ -54,13 +55,6 @@ export function printHumanError(
5455
}
5556
}
5657

57-
export function readResponseWarnings(details: Record<string, unknown> | undefined): string[] {
58-
const warnings = details?.warnings;
59-
return Array.isArray(warnings)
60-
? warnings.filter((warning): warning is string => typeof warning === 'string')
61-
: [];
62-
}
63-
6458
export function formatErrorCandidateViews(views: ErrorCandidateView[]): string[] {
6559
return views.flatMap((view) => {
6660
if (view.kind === 'element-match') {

src/commands/output/snapshot.ts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
type SnapshotUnchanged,
1515
type SnapshotVisibility,
1616
} from '@agent-device/kernel/snapshot';
17+
import { readResponseWarnings } from '@agent-device/kernel/success-text';
1718
import { buildMobileSnapshotPresentation } from '@agent-device/capture-kit/mobile-snapshot-semantics';
1819

1920
type SnapshotTextOptions = {
@@ -281,13 +282,8 @@ function formatSparseSnapshotHint(
281282
}
282283

283284
export function readSnapshotWarnings(data: Record<string, unknown>): string[] {
284-
const rawWarnings = data.warnings;
285-
if (!Array.isArray(rawWarnings)) {
286-
return [];
287-
}
288-
return rawWarnings.filter(
289-
(entry): entry is string => typeof entry === 'string' && entry.length > 0,
290-
);
285+
// Snapshot text additionally drops empty notes; the field contract is the shared parser's.
286+
return readResponseWarnings(data).filter((warning) => warning.length > 0);
291287
}
292288

293289
type SnapshotDisplayLine = ReturnType<typeof buildSnapshotDisplayLines>[number];

src/daemon/replay/internal/session-test-outcome.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { readSnapshotDiagnosticsSummary } from '@agent-device/contracts/capture';
2+
import { readResponseWarnings } from '@agent-device/kernel/success-text';
23
import type { DaemonResponse } from '../../daemon-request.ts';
34
import { isReplayInfrastructureFailure } from './session-test-infrastructure.ts';
45
import type { ReplayTestAttemptFailed, ReplayTestAttemptOutcome } from '@agent-device/replay-test';
@@ -18,7 +19,7 @@ export function toReplayTestAttemptOutcome(response: DaemonResponse): ReplayTest
1819
status: 'failed',
1920
error: response.error,
2021
artifactPaths: readArtifactPaths(response.error.details?.artifactPaths),
21-
warnings: readStringArray(response.error.details?.warnings),
22+
warnings: readResponseWarnings(response.error.details),
2223
infrastructure: isReplayInfrastructureFailure(response),
2324
...snapshotDiagnostics(response.error.details?.snapshotDiagnostics),
2425
};
@@ -28,7 +29,7 @@ export function toReplayTestAttemptOutcome(response: DaemonResponse): ReplayTest
2829
status: 'passed',
2930
replayed: typeof data?.replayed === 'number' ? data.replayed : 0,
3031
healed: typeof data?.healed === 'number' ? data.healed : 0,
31-
warnings: readStringArray(data?.warnings),
32+
warnings: readResponseWarnings(data),
3233
artifactPaths: readArtifactPaths(data?.artifactPaths),
3334
...snapshotDiagnostics(data?.snapshotDiagnostics),
3435
};

0 commit comments

Comments
 (0)