Skip to content

Commit 16d4cfc

Browse files
thymikeethymikee
authored
refactor(replay): pass the Maestro script-trust flag as a command input (#2663 PR A) (#2675)
* refactor(replay): pass the Maestro script-trust flag as a command input Part of #2663. The next step of PR A: the replay unit stops reading `req.internal`. No file moves, no behavior change. - `ReplayCommand` gains a `publicNetworkOnly?: boolean` field. The daemon handler computes it from the request's private half and passes it in, so the Maestro engine decides script trust from an explicit command input instead of reaching into `req.internal`, mirroring the #2652 pattern. - `session-replay-maestro-runtime.ts` reads the field once and derives both engine inputs from it: `trustedScripts: !publicNetworkOnly` for `executeMaestroFlow`, and the plain `publicNetworkOnly` port option. The evalScript-sandbox rationale comment moved to the field computation so the security reasoning stays with the decision. - The flag threads to the per-test-case `runReplayCommand` in the replay-test runner, so Maestro flows under `replay test` keep the same trust decision. - One direct-entry test sets the new field the way the handler does; its remote-vs-local assertions still exercise the engine input. This retires 2 of the 3 `req.internal` reads #2668 flagged. The remaining one (`resolvedSessionScope` in the action runtime) folds an `open` dispatch's implicit caller scope and its fallback `resolveSessionScope(req)`, and the `SessionScope` union it carries is shared with the session-lifecycle and snapshot families — so it moves with the dependencies bag in the next PR A slice, not here. `internal` is still *written* into nested dispatch (that is daemon mechanics, not a unit read), so the Maestro envelope is unchanged. * test(replay): cover script-trust mapping at every entry point * test(replay): cover both script-trust routes through their handlers
1 parent 0cd679f commit 16d4cfc

7 files changed

Lines changed: 322 additions & 21 deletions

‎src/daemon/handlers/__tests__/session-replay-command-trust.test.ts‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
/**
2+
* #2663 PR A: the daemon handler is the ONLY producer of `ReplayCommand`, and the sole place
3+
* that turns the request-private `internal.publicNetworkOnly` into the Maestro script-trust
4+
* command input. If a later edit — such as the PR B move — drops the mapping on either entry
5+
* point, the field reads absent, the engine trusts the flow, and a remote HTTP replay would run
6+
* `evalScript`. These assertions pin the mapping for BOTH `replay` and `replay test` so a dropped
7+
* line fails here instead of failing open. The engine-level assertions for both routes live in
8+
* `session-replay-maestro-remote-evalscript.test.ts`, routed through these handlers.
9+
*/
10+
import path from 'node:path';
11+
import { beforeEach, expect, test, vi } from 'vitest';
12+
import { LeaseRegistry } from '../../lease-registry.ts';
13+
import { platformResourceCleanup } from '../../../platform-runtime-resource-cleanup.ts';
14+
import { createScreenRecordingAdmissionLedger } from '@agent-device/capture-kit/screen-recording-admission-ledger';
15+
import {
16+
unavailableBindDevice,
17+
unavailableBindExactDevice,
18+
} from '../../__tests__/test-device-runtime-gateway.ts';
19+
import type { ReplayCommand, ReplayTestCommand } from '../../replay/internal/command-types.ts';
20+
import type { SessionCommandParams } from '../session-command-input.ts';
21+
import { makeSessionStore } from './session-test-harness.ts';
22+
import { mkdtempForTestSync } from '../../../__tests__/test-utils/tmp-dir.ts';
23+
24+
const runReplayCommand = vi.fn(async (_command: ReplayCommand) => ({
25+
ok: true as const,
26+
data: {},
27+
}));
28+
const runReplayTestCommand = vi.fn(async (_command: ReplayTestCommand) => ({
29+
ok: true as const,
30+
data: {},
31+
}));
32+
33+
vi.mock('../../replay/index.ts', () => ({
34+
runReplayCommand,
35+
runReplayTestCommand,
36+
}));
37+
38+
const { handleReplayCommand, handleReplayTestCommand } =
39+
await import('../session-replay-command.ts');
40+
41+
beforeEach(() => {
42+
runReplayCommand.mockClear();
43+
runReplayTestCommand.mockClear();
44+
});
45+
46+
function baseParams(
47+
command: string,
48+
internal?: { publicNetworkOnly?: true },
49+
): SessionCommandParams {
50+
const root = mkdtempForTestSync('agent-device-replay-trust-');
51+
return {
52+
req: {
53+
token: 'token',
54+
session: 'default',
55+
command,
56+
positionals: [path.join(root, 'flow.ad')],
57+
flags: { platform: 'ios' },
58+
...(internal ? { internal } : {}),
59+
meta: { cwd: root, requestId: `replay-trust-${command}` },
60+
},
61+
sessionName: 'default',
62+
logPath: path.join(root, 'daemon.log'),
63+
sessionStore: makeSessionStore(),
64+
leaseRegistry: new LeaseRegistry(),
65+
invoke: async () => ({ ok: true as const, data: {} }),
66+
reconcileOrphanedDeviceClaim: async () => ({
67+
status: 'retained' as const,
68+
reason: 'test' as const,
69+
}),
70+
} as unknown as SessionCommandParams;
71+
}
72+
73+
function testParams(internal?: { publicNetworkOnly?: true }): SessionCommandParams {
74+
return {
75+
...baseParams('test', internal),
76+
bindDevice: unavailableBindDevice,
77+
bindExactDevice: unavailableBindExactDevice,
78+
inspectFacts: async () => undefined,
79+
screenRecordingAdmissionLedger: createScreenRecordingAdmissionLedger(),
80+
requestScope: {
81+
signal: new AbortController().signal,
82+
diagnostics: { emit: () => {} },
83+
progress: { report: () => {} },
84+
},
85+
retainDeviceExecutionLock: async () => {},
86+
throwIfCanceled: () => {},
87+
platformResourceCleanup,
88+
} as unknown as SessionCommandParams;
89+
}
90+
91+
test('replay handler maps internal.publicNetworkOnly to the command input', async () => {
92+
await handleReplayCommand(baseParams('replay', { publicNetworkOnly: true }));
93+
expect(runReplayCommand.mock.calls[0]?.[0].publicNetworkOnly).toBe(true);
94+
});
95+
96+
test('replay handler marks a local request as trusted', async () => {
97+
await handleReplayCommand(baseParams('replay'));
98+
expect(runReplayCommand.mock.calls[0]?.[0].publicNetworkOnly).toBe(false);
99+
});
100+
101+
test('replay test handler maps internal.publicNetworkOnly to the command input', async () => {
102+
await handleReplayTestCommand(testParams({ publicNetworkOnly: true }));
103+
expect(runReplayTestCommand.mock.calls[0]?.[0].publicNetworkOnly).toBe(true);
104+
});
105+
106+
test('replay test handler marks a local request as trusted', async () => {
107+
await handleReplayTestCommand(testParams());
108+
expect(runReplayTestCommand.mock.calls[0]?.[0].publicNetworkOnly).toBe(false);
109+
});

‎src/daemon/handlers/session-replay-command.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export const handleReplayCommand: SessionCommandHandler = async ({
2323
request: req,
2424
session: createReplaySession(sessionName, logPath, sessionStore),
2525
invoke: invokeReplayAction ?? invoke,
26+
publicNetworkOnly: req.internal?.publicNetworkOnly === true,
2627
});
2728

2829
export const handleReplayTestCommand: SessionCommandHandler = async ({
@@ -63,6 +64,7 @@ export const handleReplayTestCommand: SessionCommandHandler = async ({
6364
createSession: (testSessionName, testLogPath) =>
6465
createReplaySession(testSessionName, testLogPath, sessionStore),
6566
invoke: invokeReplayAction ?? invoke,
67+
publicNetworkOnly: req.internal?.publicNetworkOnly === true,
6668
cleanupSession: async (testSessionName) =>
6769
await closeReplayTestSession({
6870
req,

‎src/daemon/replay/internal/__tests__/session-replay-maestro-remote-evalscript.test.ts‎

Lines changed: 125 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,51 @@
11
import fs from 'node:fs';
22
import path from 'node:path';
33
import { describe, expect, test, vi } from 'vitest';
4-
import { runTypedMaestroReplay } from '../session-replay-maestro-runtime.ts';
4+
import { createScreenRecordingAdmissionLedger } from '@agent-device/capture-kit/screen-recording-admission-ledger';
5+
import { LeaseRegistry } from '../../../lease-registry.ts';
6+
import { platformResourceCleanup } from '../../../../platform-runtime-resource-cleanup.ts';
57
import { SessionStore } from '../../../session-store.ts';
68
import { makeIosSession } from '../../../../__tests__/test-utils/session-factories.ts';
79
import { maestroScriptSourceBundleFor } from '../../../../__tests__/test-utils/replay-script-source.ts';
810
import { mkdtempForTestSync } from '../../../../__tests__/test-utils/tmp-dir.ts';
911
import type { DaemonRequest } from '../../../daemon-request.ts';
10-
import { createReplaySession } from '../../../handlers/session-replay-command.ts';
12+
import {
13+
unavailableBindDevice,
14+
unavailableBindExactDevice,
15+
} from '../../../__tests__/test-device-runtime-gateway.ts';
16+
import {
17+
handleReplayCommand,
18+
handleReplayTestCommand,
19+
} from '../../../handlers/session-replay-command.ts';
20+
import type { SessionCommandParams } from '../../../handlers/session-command-input.ts';
1121
import * as maestro from '@agent-device/maestro';
1222

1323
const spy = vi.spyOn(maestro, 'executeMaestroFlow');
1424

25+
const maestroFlow = [
26+
'appId: com.example.app',
27+
'---',
28+
'- evalScript: ${output.sum = 1 + 2}',
29+
'',
30+
].join('\n');
31+
32+
function writeFlow(root: string, name: string): string {
33+
const filePath = path.join(root, name);
34+
fs.writeFileSync(filePath, maestroFlow);
35+
return filePath;
36+
}
37+
38+
function engineTrustFlags(): boolean[] {
39+
return spy.mock.calls.map(([, , options]) => {
40+
const { trustedScripts } = options as { trustedScripts?: boolean };
41+
if (typeof trustedScripts !== 'boolean') throw new Error('Expected engine trust flag');
42+
return trustedScripts;
43+
});
44+
}
45+
1546
async function runWithNetworkFlag(publicNetworkOnly: boolean | undefined) {
1647
const root = mkdtempForTestSync('agent-device-maestro-remote-wire-');
17-
const flowPath = path.join(root, 'flow.yaml');
18-
fs.writeFileSync(
19-
flowPath,
20-
['appId: com.example.app', '---', '- evalScript: ${output.sum = 1 + 2}', ''].join('\n'),
21-
);
48+
const flowPath = writeFlow(root, 'flow.yaml');
2249
const sessionStore = new SessionStore(path.join(root, 'sessions'));
2350
sessionStore.set('default', makeIosSession('default'));
2451

@@ -29,22 +56,94 @@ async function runWithNetworkFlag(publicNetworkOnly: boolean | undefined) {
2956
positionals: [flowPath],
3057
flags: {
3158
platform: 'ios',
59+
replayBackend: 'maestro',
3260
replayScriptSource: await maestroScriptSourceBundleFor(flowPath),
3361
},
3462
...(publicNetworkOnly === true ? { internal: { publicNetworkOnly: true } } : {}),
3563
meta: { requestId: `req-maestro-wire-${publicNetworkOnly}` },
3664
} as unknown as DaemonRequest;
3765

38-
spy.mockResolvedValueOnce({ ok: true, replayed: 1, planDigest: 'test', startIndex: 0 } as never);
66+
spy.mockReset();
67+
spy.mockResolvedValue({ ok: true, replayed: 1, planDigest: 'test', startIndex: 0 } as never);
3968

40-
const response = await runTypedMaestroReplay({
41-
request: req,
42-
session: createReplaySession('default', path.join(root, 'daemon.log'), sessionStore),
69+
// Drive the real `replay` handler so the request-private → command-input
70+
// mapping (`req.internal.publicNetworkOnly → command.publicNetworkOnly`) is on
71+
// the asserted path: dropping it in the handler must fail this test, since an
72+
// unset field reads as trusted and a remote flow would run evalScript.
73+
const response = await handleReplayCommand({
74+
req,
75+
sessionName: 'default',
76+
logPath: path.join(root, 'daemon.log'),
77+
sessionStore,
4378
invoke: async () => ({ ok: true, data: {} }) as never,
79+
leaseRegistry: new LeaseRegistry(),
80+
reconcileOrphanedDeviceClaim: async () => ({
81+
status: 'retained' as const,
82+
reason: 'test' as const,
83+
}),
4484
});
4585

86+
if (!response) throw new Error('Expected replay response');
4687
expect(response.ok).toBe(true);
47-
return spy.mock.calls.at(-1)?.[2] as { trustedScripts?: boolean } | undefined;
88+
return engineTrustFlags();
89+
}
90+
91+
async function runWithTestNetworkFlag(publicNetworkOnly: boolean | undefined) {
92+
const root = mkdtempForTestSync('agent-device-maestro-remote-test-');
93+
const firstPath = writeFlow(root, '01-flow.yaml');
94+
const secondPath = writeFlow(root, '02-flow.yaml');
95+
const sessionStore = new SessionStore(path.join(root, 'sessions'));
96+
97+
const req = {
98+
token: 'test-token',
99+
session: 'default',
100+
command: 'test',
101+
positionals: [firstPath, secondPath],
102+
flags: {
103+
platform: 'ios',
104+
replayBackend: 'maestro',
105+
replayScriptSources: await Promise.all([
106+
maestroScriptSourceBundleFor(firstPath),
107+
maestroScriptSourceBundleFor(secondPath),
108+
]),
109+
},
110+
...(publicNetworkOnly === true ? { internal: { publicNetworkOnly: true } } : {}),
111+
meta: { requestId: `req-maestro-test-wire-${publicNetworkOnly}` },
112+
} as unknown as DaemonRequest;
113+
114+
spy.mockReset();
115+
spy.mockResolvedValue({ ok: true, replayed: 1, planDigest: 'test', startIndex: 0 } as never);
116+
117+
// Drive the real `replay test` handler and suite scheduler so the handler mapping and the
118+
// per-case forwarding of `publicNetworkOnly` are both on the asserted engine path.
119+
const response = await handleReplayTestCommand({
120+
req,
121+
sessionName: 'default',
122+
logPath: path.join(root, 'daemon.log'),
123+
sessionStore,
124+
leaseRegistry: new LeaseRegistry(),
125+
invoke: async () => ({ ok: true, data: {} }) as never,
126+
reconcileOrphanedDeviceClaim: async () => ({
127+
status: 'retained' as const,
128+
reason: 'test' as const,
129+
}),
130+
bindDevice: unavailableBindDevice,
131+
bindExactDevice: unavailableBindExactDevice,
132+
inspectFacts: async () => undefined,
133+
screenRecordingAdmissionLedger: createScreenRecordingAdmissionLedger(),
134+
requestScope: {
135+
signal: new AbortController().signal,
136+
diagnostics: { emit: () => {} },
137+
progress: { report: () => {} },
138+
},
139+
retainDeviceExecutionLock: async () => {},
140+
throwIfCanceled: () => {},
141+
platformResourceCleanup,
142+
} as unknown as SessionCommandParams);
143+
144+
if (!response) throw new Error('Expected replay test response');
145+
expect(response).toMatchObject({ ok: true, data: { total: 2, executed: 2, passed: 2 } });
146+
return engineTrustFlags();
48147
}
49148

50149
describe('remote Maestro evalScript trust wiring', () => {
@@ -53,12 +152,22 @@ describe('remote Maestro evalScript trust wiring', () => {
53152
// packages/maestro/src/internal/__tests__/engine.test.ts) proves false refuses
54153
// before vm evaluation — together they prove remote evalScript never runs.
55154
test('remote HTTP (publicNetworkOnly) forwards trustedScripts:false so the engine refuses before vm', async () => {
56-
const options = await runWithNetworkFlag(true);
57-
expect(options?.trustedScripts).toBe(false);
155+
const trustFlags = await runWithNetworkFlag(true);
156+
expect(trustFlags).toEqual([false]);
58157
});
59158

60159
test('local flow does not forward trustedScripts:false', async () => {
61-
const options = await runWithNetworkFlag(undefined);
62-
expect(options?.trustedScripts).not.toBe(false);
160+
const trustFlags = await runWithNetworkFlag(undefined);
161+
expect(trustFlags).toEqual([true]);
162+
});
163+
164+
test('remote replay test forwards trustedScripts:false to each Maestro attempt', async () => {
165+
const trustFlags = await runWithTestNetworkFlag(true);
166+
expect(trustFlags).toEqual([false, false]);
167+
});
168+
169+
test('local replay test does not forward trustedScripts:false', async () => {
170+
const trustFlags = await runWithTestNetworkFlag(undefined);
171+
expect(trustFlags).toEqual([true, true]);
63172
});
64173
});

‎src/daemon/replay/internal/__tests__/session-test-command-trust.test.ts‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/**
2+
* #2663 PR A: the suite command must forward the request-private trust bit to every nested replay.
3+
* If only the suite handler maps the bit, each Maestro attempt would reconstruct it as trusted.
4+
*/
5+
import fs from 'node:fs';
6+
import path from 'node:path';
7+
import { beforeEach, expect, test, vi } from 'vitest';
8+
import { SessionStore } from '../../../session-store.ts';
9+
import { createReplaySession } from '../../../handlers/session-replay-command.ts';
10+
import { runReplayTestCommand } from '../../index.ts';
11+
import type { ReplayCommand, ReplayTestCommand } from '../command-types.ts';
12+
import { replayScriptSourceBundleFor } from '../../../../__tests__/test-utils/replay-script-source.ts';
13+
import { mkdtempForTestSync } from '../../../../__tests__/test-utils/tmp-dir.ts';
14+
import type { DaemonInvokeFn, DaemonRequest } from '../../../daemon-request.ts';
15+
16+
const capturedCommands: ReplayCommand[] = [];
17+
18+
vi.mock('../native-command.ts', () => ({
19+
runReplayCommand: async (command: ReplayCommand) => {
20+
capturedCommands.push(command);
21+
return { ok: true as const, data: { replayed: 1 } };
22+
},
23+
}));
24+
25+
beforeEach(() => {
26+
capturedCommands.length = 0;
27+
});
28+
29+
async function forwardedCommands(publicNetworkOnly?: boolean): Promise<readonly ReplayCommand[]> {
30+
const root = mkdtempForTestSync('agent-device-test-command-trust-');
31+
const scriptPath = path.join(root, 'flow.ad');
32+
fs.writeFileSync(scriptPath, 'context platform=ios\nopen "Demo"\n');
33+
const sessionStore = new SessionStore(path.join(root, 'sessions'));
34+
const req: DaemonRequest = {
35+
token: 'token',
36+
session: 'default',
37+
command: 'test',
38+
positionals: [scriptPath],
39+
flags: {
40+
platform: 'ios',
41+
replayScriptSources: [replayScriptSourceBundleFor(scriptPath)],
42+
},
43+
meta: { cwd: root, requestId: 'test-command-trust' },
44+
};
45+
const command: ReplayTestCommand = {
46+
request: req,
47+
session: createReplaySession('default', path.join(root, 'daemon.log'), sessionStore),
48+
createSession: (sessionName, logPath) =>
49+
createReplaySession(sessionName, logPath, sessionStore),
50+
invoke: (async () => ({ ok: true as const, data: {} })) as DaemonInvokeFn,
51+
cleanupSession: async () => {},
52+
...(publicNetworkOnly === undefined ? {} : { publicNetworkOnly }),
53+
};
54+
55+
const response = await runReplayTestCommand(command);
56+
expect(response.ok).toBe(true);
57+
return capturedCommands;
58+
}
59+
60+
test('test command forwards untrusted replay trust to each attempt', async () => {
61+
const commands = await forwardedCommands(true);
62+
expect(commands).toHaveLength(1);
63+
expect(commands[0]?.publicNetworkOnly).toBe(true);
64+
});
65+
66+
test('test command forwards trusted replay trust to each attempt', async () => {
67+
const commands = await forwardedCommands(false);
68+
expect(commands).toHaveLength(1);
69+
expect(commands[0]?.publicNetworkOnly).toBe(false);
70+
});

‎src/daemon/replay/internal/command-types.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,13 @@ export type ReplayCommand = Readonly<{
3939
invoke: DaemonInvokeFn;
4040
tracePath?: string;
4141
onStep?: ReplayTestAttemptStepSink;
42+
/**
43+
* True when the request reached the daemon only over its public HTTP surface. The handler draws
44+
* it from the request's daemon-private half and hands it here so the Maestro engine decides
45+
* script trust from an explicit input instead of reading `req.internal`. Absent means a local
46+
* caller, which trusts its own scripts.
47+
*/
48+
publicNetworkOnly?: boolean;
4249
}>;
4350

4451
type ReplayRequestContext = Readonly<{

0 commit comments

Comments
 (0)