Skip to content

Commit 7edcd2a

Browse files
committed
refactor(screenshot): split crop target/policy module and trim redundant coverage
Address review comments at 570da2c: - Split the 328-line screenshot-crop.ts leaf: the target acceptance matrix, classifier, and pre-device argument policy move to screenshot-crop-target.ts, so both implementation modules meet the 300-line target. - Reuse kernel isPositiveFiniteRect/rectArea in the rect-projection module instead of redefining them locally. - Drop the crop-on CLI forwarding case (redundant with screenshot-options flag-mapping coverage + the generic dispatcher) and the transport-based warnings case, replacing the latter with a focused screenshot-result unit test. This also returns the two legacy aggregate test files to their merge-base length for the test-file size ratchet.
1 parent 570da2c commit 7edcd2a

9 files changed

Lines changed: 392 additions & 452 deletions

File tree

packages/capture-kit/src/snapshot-rect-projection.ts

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { AppError } from '@agent-device/kernel/errors';
22
import { SCREENSHOT_CROP_REASONS } from '@agent-device/contracts/capture';
33
import { isViewportRootNode, normalizeType } from '@agent-device/contracts/snapshot';
4+
import { isPositiveFiniteRect, rectArea } from '@agent-device/kernel/rect';
45
import type { Rect, SnapshotNode } from '@agent-device/kernel/snapshot';
56

67
/**
@@ -84,15 +85,15 @@ export function resolveSnapshotBounds(
8485
): Rect | null {
8586
let viewport: Rect | null = null;
8687
for (const node of nodes) {
87-
if (!isViewportRootNode(node) || !hasPositiveRect(node.rect)) continue;
88+
if (!isViewportRootNode(node) || !isPositiveFiniteRect(node.rect)) continue;
8889
if (!viewport || rectArea(node.rect) > rectArea(viewport)) {
8990
viewport = node.rect;
9091
}
9192
}
9293
if (viewport) return viewport;
9394

9495
return measureSnapshotBounds(
95-
nodes.filter((node) => hasPositiveRect(node.rect) && !isSnapshotBoundsOutlier(node)),
96+
nodes.filter((node) => isPositiveFiniteRect(node.rect) && !isSnapshotBoundsOutlier(node)),
9697
);
9798
}
9899

@@ -102,7 +103,7 @@ function measureSnapshotBounds(nodes: ReadonlyArray<Pick<SnapshotNode, 'rect'>>)
102103
let maxRight = Number.NEGATIVE_INFINITY;
103104
let maxBottom = Number.NEGATIVE_INFINITY;
104105
for (const node of nodes) {
105-
if (!node.rect || !hasPositiveRect(node.rect)) continue;
106+
if (!isPositiveFiniteRect(node.rect)) continue;
106107
minX = Math.min(minX, node.rect.x);
107108
minY = Math.min(minY, node.rect.y);
108109
maxRight = Math.max(maxRight, node.rect.x + node.rect.width);
@@ -130,14 +131,6 @@ function isMeaningfulBoundsSignal(value: string | undefined): boolean {
130131
return !/^(true|false)$/i.test(trimmed);
131132
}
132133

133-
function hasPositiveRect(rect: Rect | undefined): rect is Rect {
134-
return Boolean(rect && rect.width > 0 && rect.height > 0);
135-
}
136-
137-
function rectArea(rect: Rect): number {
138-
return rect.width * rect.height;
139-
}
140-
141134
function roundRect(rect: Rect): Rect {
142135
return {
143136
x: Math.round(rect.x),

src/__tests__/cli-client-commands.test.ts

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -482,40 +482,6 @@ test('screenshot forwards --overlay-refs to the client capture API', async () =>
482482
});
483483
});
484484

485-
test('screenshot forwards --crop-on to the client capture API', async () => {
486-
let observed: { path?: string; cropOn?: string } | undefined;
487-
const client = createStubClient({
488-
installFromSource: async () => {
489-
throw new Error('unexpected install call');
490-
},
491-
screenshot: async (options) => {
492-
observed = options;
493-
return {
494-
path: '/tmp/screenshot.png',
495-
identifiers: { session: 'default' },
496-
};
497-
},
498-
});
499-
500-
const handled = await tryRunClientBackedCommand({
501-
command: 'screenshot',
502-
positionals: ['/tmp/screenshot.png'],
503-
flags: {
504-
json: false,
505-
help: false,
506-
version: false,
507-
screenshotCropOn: 'label="Save"',
508-
},
509-
client,
510-
});
511-
512-
assert.equal(handled, true);
513-
assert.deepEqual(observed, {
514-
path: '/tmp/screenshot.png',
515-
cropOn: 'label="Save"',
516-
});
517-
});
518-
519485
test('diff screenshot forwards --surface to live client screenshot capture', async () => {
520486
const dir = mkdtempForTestSync('agent-device-cli-diff-surface-');
521487
const baseline = path.join(dir, 'baseline.png');

src/__tests__/client.test.ts

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1192,29 +1192,6 @@ test('client capture.screenshot normalizes overlay refs from daemon response dat
11921192
});
11931193
});
11941194

1195-
test('client capture.screenshot surfaces response-level warnings', async () => {
1196-
const setup = createTransport(async () => ({
1197-
ok: true,
1198-
data: {
1199-
path: '/tmp/screenshot.png',
1200-
width: 40,
1201-
height: 20,
1202-
warnings: [
1203-
'CROP_PARTIAL_INTERSECTION: the selector frame extends past the captured image; the crop was clipped to the image frame',
1204-
],
1205-
},
1206-
}));
1207-
const client = createAgentDeviceClient(setup.config, { transport: setup.transport });
1208-
1209-
const result = await client.capture.screenshot({});
1210-
1211-
assert.equal(result.path, '/tmp/screenshot.png');
1212-
assert.deepEqual(result.warnings, [
1213-
'CROP_PARTIAL_INTERSECTION: the selector frame extends past the captured image; the crop was clipped to the image frame',
1214-
]);
1215-
assert.deepEqual(result.identifiers, { session: 'qa' });
1216-
});
1217-
12181195
test('sessions.stateDir resolves locally without contacting the daemon', async () => {
12191196
const setup = createTransport(async () => {
12201197
throw new Error('unexpected daemon call');
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import assert from 'node:assert/strict';
2+
import { test } from 'vitest';
3+
import {
4+
normalizeScreenshotCaptureResult,
5+
pickScreenshotResultData,
6+
} from '../screenshot-result.ts';
7+
8+
const CROP_WARNING =
9+
'CROP_PARTIAL_INTERSECTION: the selector frame extends past the captured image; the crop was clipped to the image frame';
10+
11+
test('normalizeScreenshotCaptureResult surfaces response-level warnings', () => {
12+
const result = normalizeScreenshotCaptureResult(
13+
{
14+
path: '/tmp/screenshot.png',
15+
width: 40,
16+
height: 20,
17+
warnings: [CROP_WARNING],
18+
},
19+
'qa',
20+
);
21+
assert.equal(result.path, '/tmp/screenshot.png');
22+
assert.equal(result.width, 40);
23+
assert.equal(result.height, 20);
24+
assert.deepEqual(result.warnings, [CROP_WARNING]);
25+
assert.deepEqual(result.identifiers, { session: 'qa' });
26+
});
27+
28+
test('normalizeScreenshotCaptureResult omits warnings when the response carries none', () => {
29+
const result = normalizeScreenshotCaptureResult({ path: '/tmp/screenshot.png' }, 'qa');
30+
assert.equal(result.path, '/tmp/screenshot.png');
31+
assert.ok(!('warnings' in result));
32+
assert.deepEqual(result.identifiers, { session: 'qa' });
33+
});
34+
35+
test('pickScreenshotResultData keeps warnings only when present and non-empty', () => {
36+
assert.deepEqual(
37+
pickScreenshotResultData({ path: '/tmp/a.png', width: 40, warnings: [CROP_WARNING] }),
38+
{ path: '/tmp/a.png', width: 40, warnings: [CROP_WARNING] },
39+
);
40+
assert.deepEqual(pickScreenshotResultData({ path: '/tmp/a.png', warnings: [] }), {
41+
path: '/tmp/a.png',
42+
});
43+
});
Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
import {
2+
ANDROID_DEVICE,
3+
ANDROID_EMULATOR,
4+
IOS_DEVICE,
5+
IOS_SIMULATOR,
6+
LINUX_DEVICE,
7+
MACOS_DEVICE,
8+
TVOS_SIMULATOR,
9+
WEB_DESKTOP_DEVICE,
10+
} from '../../__tests__/test-utils/device-fixtures.ts';
11+
import { SCREENSHOT_CROP_REASONS } from '@agent-device/contracts/capture';
12+
import type { SessionSurface } from '@agent-device/contracts/session';
13+
import type { DeviceInfo } from '@agent-device/kernel/device';
14+
import { expect, test } from 'vitest';
15+
import {
16+
SCREENSHOT_CROP_TARGET_CELLS,
17+
assertScreenshotCropPolicy,
18+
classifyScreenshotCropTarget,
19+
} from '../screenshot-crop-target.ts';
20+
21+
type CropTargetDevice = Readonly<{
22+
target: (typeof SCREENSHOT_CROP_TARGET_CELLS)[number]['target'];
23+
device: DeviceInfo;
24+
surface: SessionSurface | undefined;
25+
}>;
26+
27+
/** One device per matrix cell: the classifier's whole reachable output, named by its cell. */
28+
const CROP_TARGET_DEVICES: readonly CropTargetDevice[] = [
29+
{ target: 'ios-simulator', device: IOS_SIMULATOR, surface: undefined },
30+
{ target: 'android-emulator', device: ANDROID_EMULATOR, surface: undefined },
31+
{ target: 'android-device', device: ANDROID_DEVICE, surface: undefined },
32+
{ target: 'macos-app-window', device: MACOS_DEVICE, surface: 'app' },
33+
{ target: 'ios-physical', device: IOS_DEVICE, surface: undefined },
34+
{ target: 'macos-helper', device: MACOS_DEVICE, surface: undefined },
35+
{ target: 'web', device: WEB_DESKTOP_DEVICE, surface: undefined },
36+
{ target: 'linux', device: LINUX_DEVICE, surface: undefined },
37+
{ target: 'tvos', device: TVOS_SIMULATOR, surface: undefined },
38+
{
39+
target: 'harmonyos',
40+
device: { platform: 'harmonyos', id: 'hmy-1', name: 'HarmonyOS', kind: 'device' },
41+
surface: undefined,
42+
},
43+
{
44+
target: 'vega',
45+
device: { platform: 'vega', id: 'vega-1', name: 'Vega TV', kind: 'device', target: 'tv' },
46+
surface: undefined,
47+
},
48+
];
49+
50+
test('the classifier and the acceptance matrix agree one-to-one, and the accepted cells are exactly the evidenced ones', () => {
51+
const matrixTargets = SCREENSHOT_CROP_TARGET_CELLS.map((cell) => cell.target);
52+
expect(new Set(matrixTargets).size).toBe(matrixTargets.length);
53+
for (const row of CROP_TARGET_DEVICES) {
54+
expect(classifyScreenshotCropTarget(row.device, row.surface)).toBe(row.target);
55+
}
56+
expect(CROP_TARGET_DEVICES.map((row) => row.target)).toEqual(matrixTargets);
57+
for (const cell of SCREENSHOT_CROP_TARGET_CELLS) {
58+
if (cell.target === 'ios-simulator' || cell.target === 'android-emulator') {
59+
expect(cell.status).toBe('accepted');
60+
} else {
61+
expect(cell).toEqual({
62+
target: cell.target,
63+
status: 'rejected',
64+
rejectionReason: SCREENSHOT_CROP_REASONS.pendingPixelIdentityEvidence,
65+
});
66+
}
67+
}
68+
});
69+
70+
test('an apple device with an unpopulated reserved OS is a typed refusal, not a guess', () => {
71+
const device: DeviceInfo = {
72+
platform: 'apple',
73+
id: 'watch-1',
74+
name: 'Watch',
75+
kind: 'simulator',
76+
appleOs: 'watchos',
77+
};
78+
let refusal: unknown;
79+
try {
80+
classifyScreenshotCropTarget(device, undefined);
81+
} catch (error) {
82+
refusal = error;
83+
}
84+
expect(refusal).toMatchObject({
85+
code: 'UNSUPPORTED_OPERATION',
86+
details: { reason: SCREENSHOT_CROP_REASONS.targetNotAccepted },
87+
});
88+
});
89+
90+
function expectPolicyRefusal(
91+
params: Readonly<{
92+
device: DeviceInfo;
93+
surface: SessionSurface | undefined;
94+
cropOn: string;
95+
overlayRefs: boolean;
96+
fullscreen: boolean;
97+
}>,
98+
expected: Record<string, unknown>,
99+
): void {
100+
try {
101+
assertScreenshotCropPolicy(params);
102+
} catch (error) {
103+
expect(error).toMatchObject(expected);
104+
return;
105+
}
106+
throw new Error('the crop policy must refuse before device work');
107+
}
108+
109+
const POLICY_BASE = { cropOn: 'label="Save"', overlayRefs: false, fullscreen: false } as const;
110+
111+
test('combination refusals are answered before selector validation and the matrix', () => {
112+
expectPolicyRefusal(
113+
{ device: MACOS_DEVICE, surface: 'app', ...POLICY_BASE, overlayRefs: true },
114+
{ code: 'INVALID_ARGS', details: { reason: SCREENSHOT_CROP_REASONS.frameMismatch } },
115+
);
116+
expectPolicyRefusal(
117+
{ device: ANDROID_EMULATOR, surface: undefined, ...POLICY_BASE, fullscreen: true },
118+
{ code: 'INVALID_ARGS', details: { reason: SCREENSHOT_CROP_REASONS.frameMismatch } },
119+
);
120+
});
121+
122+
test('an invalid selector expression is refused with the selector reason', () => {
123+
expectPolicyRefusal(
124+
{
125+
device: ANDROID_EMULATOR,
126+
surface: undefined,
127+
cropOn: 'label="unterminated',
128+
overlayRefs: false,
129+
fullscreen: false,
130+
},
131+
{ code: 'INVALID_ARGS', details: { reason: SCREENSHOT_CROP_REASONS.selectorInvalid } },
132+
);
133+
});
134+
135+
test('a matrix-rejected target is refused with the pending-evidence reason', () => {
136+
expectPolicyRefusal(
137+
{ device: ANDROID_DEVICE, surface: undefined, ...POLICY_BASE },
138+
{
139+
code: 'UNSUPPORTED_OPERATION',
140+
details: {
141+
reason: SCREENSHOT_CROP_REASONS.targetNotAccepted,
142+
rejectionReason: SCREENSHOT_CROP_REASONS.pendingPixelIdentityEvidence,
143+
},
144+
},
145+
);
146+
});
147+
148+
test('an accepted target with a valid selector passes the policy', () => {
149+
expect(() =>
150+
assertScreenshotCropPolicy({ device: ANDROID_EMULATOR, surface: undefined, ...POLICY_BASE }),
151+
).not.toThrow();
152+
});

0 commit comments

Comments
 (0)