Skip to content

Commit 5fa95fd

Browse files
committed
fix: preserve selector get and screenshot cleanup
1 parent 6811ac3 commit 5fa95fd

6 files changed

Lines changed: 108 additions & 13 deletions

File tree

src/__tests__/runtime-public.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,35 @@ test('runtime screenshot command reserves output and calls backend primitive', a
113113
});
114114
});
115115

116+
test('runtime screenshot command cleans reserved output when publish fails', async () => {
117+
let cleanupCalled = false;
118+
const device = createAgentDevice({
119+
backend,
120+
artifacts: {
121+
...artifacts,
122+
reserveOutput: async (ref: FileOutputRef | undefined, options) => ({
123+
path: ref?.kind === 'path' ? ref.path : `/tmp/${options.field}${options.ext}`,
124+
visibility: options.visibility ?? 'client-visible',
125+
publish: async () => {
126+
throw new Error('publish failed');
127+
},
128+
cleanup: async () => {
129+
cleanupCalled = true;
130+
},
131+
}),
132+
},
133+
sessions,
134+
policy: localCommandPolicy(),
135+
});
136+
137+
await assert.rejects(
138+
() => device.capture.screenshot({ out: { kind: 'path', path: '/tmp/screen.png' } }),
139+
/publish failed/,
140+
);
141+
142+
assert.equal(cleanupCalled, true);
143+
});
144+
116145
test('public runtime policy helpers expose local and restricted defaults', async () => {
117146
assert.equal(typeof createLocalArtifactAdapter, 'function');
118147
assert.equal(rootCommands.capture.screenshot, commands.capture.screenshot);

src/__tests__/runtime-selector-read.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,34 @@ test('runtime get reads text from a selector target', async () => {
3535
]);
3636
});
3737

38+
test('runtime get selector target captures fresh snapshot without a stored session snapshot', async () => {
39+
const snapshot = selectorSnapshot();
40+
const sessions = createMemorySessionStore([{ name: 'default' }]);
41+
let captures = 0;
42+
const device = createAgentDevice({
43+
backend: {
44+
platform: 'ios',
45+
captureSnapshot: async () => {
46+
captures += 1;
47+
return { snapshot };
48+
},
49+
readText: async () => ({ text: 'Fresh text' }),
50+
} satisfies AgentDeviceBackend,
51+
artifacts: createLocalArtifactAdapter(),
52+
sessions,
53+
policy: localCommandPolicy(),
54+
});
55+
56+
const result = await device.selectors.getText(selector('label=Continue'), {
57+
session: 'default',
58+
});
59+
60+
assert.equal(result.kind, 'text');
61+
assert.equal(result.text, 'Fresh text');
62+
assert.equal(captures, 1);
63+
assert.equal((await sessions.get('default'))?.snapshot?.nodes[0]?.label, 'Continue');
64+
});
65+
3866
test('runtime get returns attrs for a ref target without recapturing', async () => {
3967
const snapshot = selectorSnapshot();
4068
let captures = 0;

src/commands/capture-screenshot.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export const screenshotCommand: RuntimeCommand<
2323
ext: '.png',
2424
});
2525

26+
let artifact: ArtifactDescriptor | undefined;
2627
try {
2728
await runtime.backend.captureScreenshot(
2829
{
@@ -40,12 +41,12 @@ export const screenshotCommand: RuntimeCommand<
4041
surface: options.surface,
4142
},
4243
);
44+
artifact = await reserved.publish();
4345
} catch (error) {
4446
await reserved.cleanup?.();
4547
throw error;
4648
}
4749

48-
const artifact = await reserved.publish();
4950
return {
5051
path: reserved.path,
5152
...(artifact ? { artifacts: [artifact] } : {}),

src/commands/selector-read.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { buildSelectorChainForNode } from '../utils/selector-build.ts';
1414
import { evaluateIsPredicate, isSupportedPredicate } from '../utils/selector-is-predicates.ts';
1515
import type { RuntimeCommand } from './index.ts';
1616
import {
17+
type CapturedSnapshot,
1718
captureSelectorSnapshot,
1819
readText,
1920
requireSnapshotSession,
@@ -177,8 +178,8 @@ export const getCommand: RuntimeCommand<GetCommandOptions, GetCommandResult> = a
177178
runtime,
178179
options,
179180
): Promise<GetCommandResult> => {
180-
const capture = await requireSnapshotSession(runtime, options.session);
181181
if (options.target.kind === 'ref') {
182+
const capture = await requireSnapshotSession(runtime, options.session);
182183
const resolved = resolveRefNode(capture.snapshot.nodes, options.target.ref, {
183184
fallbackLabel: options.target.fallbackLabel ?? '',
184185
invalidRefMessage: 'get text requires a ref like @e2',
@@ -195,7 +196,7 @@ export const getCommand: RuntimeCommand<GetCommandOptions, GetCommandResult> = a
195196
return { kind: 'text', target, text, node: resolved.node, selectorChain };
196197
}
197198

198-
const resolved = await resolveSelectorNode(runtime, options, capture.sessionName, {
199+
const resolved = await resolveSelectorNode(runtime, options, options.session ?? 'default', {
199200
selector: options.target.selector,
200201
disambiguateAmbiguous: options.property === 'text',
201202
});
@@ -213,7 +214,7 @@ export const getCommand: RuntimeCommand<GetCommandOptions, GetCommandResult> = a
213214
};
214215
}
215216

216-
const text = await readText(runtime, capture, resolved.node);
217+
const text = await readText(runtime, resolved.capture, resolved.node);
217218
return {
218219
kind: 'text',
219220
target: { kind: 'selector', selector: resolved.selector },
@@ -455,7 +456,7 @@ async function resolveSelectorNode(
455456
options: GetCommandOptions,
456457
sessionName: string,
457458
params: { selector: string; disambiguateAmbiguous: boolean },
458-
): Promise<{ node: SnapshotNode; selector: string; ref: string }> {
459+
): Promise<{ capture: CapturedSnapshot; node: SnapshotNode; selector: string; ref: string }> {
459460
const capture = await captureSelectorSnapshot(
460461
runtime,
461462
{ ...options, session: sessionName },
@@ -473,5 +474,10 @@ async function resolveSelectorNode(
473474
if (!resolved) {
474475
throw new AppError('COMMAND_FAILED', formatSelectorFailure(chain, [], { unique: true }));
475476
}
476-
return { node: resolved.node, selector: resolved.selector.raw, ref: `@${resolved.node.ref}` };
477+
return {
478+
capture,
479+
node: resolved.node,
480+
selector: resolved.selector.raw,
481+
ref: `@${resolved.node.ref}`,
482+
};
477483
}

src/daemon/__tests__/request-router-screenshot.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ vi.mock('../../core/dispatch.ts', async (importOriginal) => {
1010

1111
import { dispatchCommand } from '../../core/dispatch.ts';
1212
import { createRequestHandler } from '../request-router.ts';
13+
import { dispatchScreenshotViaRuntime } from '../screenshot-runtime.ts';
1314
import type { SessionState } from '../types.ts';
1415
import { LeaseRegistry } from '../lease-registry.ts';
1516
import { attachRefs } from '../../utils/snapshot.ts';
@@ -94,6 +95,28 @@ test('screenshot resolves relative positional path against request cwd', async (
9495
expect(recordedAction?.positionals).toEqual([path.join(callerCwd, 'evidence/test.png')]);
9596
});
9697

98+
test('default screenshot temp directory is cleaned when capture fails', async () => {
99+
const session = makeSession('default');
100+
let capturedPath: string | undefined;
101+
mockDispatch.mockImplementation(async (_device, command, positionals) => {
102+
if (command === 'screenshot') capturedPath = positionals[0];
103+
throw new Error('capture failed');
104+
});
105+
106+
await expect(
107+
dispatchScreenshotViaRuntime({
108+
session,
109+
sessionName: session.name,
110+
outputPlacement: 'default',
111+
dispatchContext: {},
112+
}),
113+
).rejects.toThrow(/capture failed/);
114+
115+
expect(capturedPath).toBeTruthy();
116+
expect(path.basename(capturedPath!)).toBe('screenshot.png');
117+
expect(fs.existsSync(path.dirname(capturedPath!))).toBe(false);
118+
});
119+
97120
test('router serializes concurrent commands for the same device across sessions', async () => {
98121
const sessionStore = makeSessionStore('agent-device-router-screenshot-');
99122
sessionStore.set('session-a', makeSession('session-a'));

src/daemon/screenshot-runtime.ts

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -90,18 +90,26 @@ function createDaemonScreenshotArtifactAdapter(): ArtifactAdapter {
9090
throw new AppError('UNSUPPORTED_OPERATION', 'screenshot does not resolve input artifacts');
9191
},
9292
reserveOutput: async (ref) => {
93-
const outputPath =
94-
ref?.kind === 'path'
95-
? ref.path
96-
: path.join(
97-
await fs.mkdtemp(path.join(os.tmpdir(), 'agent-device-screenshot-')),
98-
'screenshot.png',
99-
);
93+
let tempRoot: string | undefined;
94+
let outputPath: string;
95+
if (ref?.kind === 'path') {
96+
outputPath = ref.path;
97+
} else {
98+
tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'agent-device-screenshot-'));
99+
outputPath = path.join(tempRoot, 'screenshot.png');
100+
}
100101
await fs.mkdir(path.dirname(outputPath), { recursive: true });
101102
return {
102103
path: outputPath,
103104
visibility: 'client-visible',
104105
publish: async () => undefined,
106+
...(tempRoot
107+
? {
108+
cleanup: async () => {
109+
await fs.rm(tempRoot, { recursive: true, force: true });
110+
},
111+
}
112+
: {}),
105113
};
106114
},
107115
createTempFile: async (options) => {

0 commit comments

Comments
 (0)