Skip to content

Commit f693e9a

Browse files
committed
fix(ci): split the differential CLI env into entry path and node flags
Addresses the P1 review on #2069. Tokenizing AGENT_DEVICE_CLI on whitespace fixed the flags-plus-script shape but broke the other one: an override like `/tmp/agent device.mjs`, which main passes through intact, became two arguments. One variable cannot encode both — any delimiter that separates flags from the entry can also occur inside a path. So the two concerns become two variables that cannot be confused: AGENT_DEVICE_CLI the entry script — ONE path, never split AGENT_DEVICE_CLI_NODE_FLAGS node flags — split on whitespace, which is exact because a node flag cannot contain a space Defaults reproduce today's behavior, and the empty string runs an entry that needs no flags. The regression now runs through the production route the review asked for — environment, parseRunnerArgs, runScenario, spawn — rather than calling runAgentDeviceEngine with a hand-built argv, which cannot see the environment contract at all. Each direction is pinned by its own case, verified against both broken implementations: main's unsplit string fails "node flags stay separate arguments", and the whitespace split fails "a CLI path containing spaces reaches the spawn unsplit". The maestro stub stays out of the spaced directory on purpose: runMaestroEngine still splits its command on spaces, and a spaced stub path would fail these tests for the other engine's reason.
1 parent bcfde56 commit f693e9a

5 files changed

Lines changed: 146 additions & 45 deletions

File tree

.github/workflows/conformance-differential.yml

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,11 @@ concurrency:
3232
cancel-in-progress: true
3333

3434
env:
35-
AGENT_DEVICE_CLI: '--experimental-strip-types src/bin.ts'
35+
# Entry script and node flags are separate on purpose: one variable cannot
36+
# encode both without either corrupting a path that contains spaces or
37+
# spawning the whole line as a single node option.
38+
AGENT_DEVICE_CLI: 'src/bin.ts'
39+
AGENT_DEVICE_CLI_NODE_FLAGS: '--experimental-strip-types'
3640
DIFFERENTIAL_ONLY: ${{ github.event.inputs.only || '' }}
3741
# CI should not phone home, and it keeps `maestro --version` to just the
3842
# version instead of prefixing an analytics notice.

packages/maestro/test/conformance/differential/engine-process.test.ts

Lines changed: 17 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -43,33 +43,24 @@ test('agent-device execution accepts a CLI path containing spaces', () => {
4343
}
4444
});
4545

46-
// The regression the nightly ran into for two days: AGENT_DEVICE_CLI carries node
47-
// flags, and passing that line to node as one argument aborts before the CLI loads
48-
// ("bad option: --experimental-strip-types src/bin.ts"), which every scenario then
49-
// reports as an engine infrastructure failure rather than a divergence. Running the
50-
// workflow's own shape end to end is what pins it — a fixture path with no flag
51-
// cannot tell an argv from a command line.
52-
test('agent-device execution spawns node flags as their own arguments', () => {
53-
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-engine-argv-'));
54-
const cliPath = path.join(root, 'cli.ts');
55-
try {
56-
fs.writeFileSync(cliPath, 'const code: number = 0;\nprocess.exit(code);\n');
57-
assert.deepEqual(
58-
runAgentDeviceEngine(resolveAgentDeviceCliArgv(`--experimental-strip-types ${cliPath}`), []),
59-
{ engine: 'agent-device', outcome: 'pass', exitCode: 0 },
60-
);
61-
} finally {
62-
fs.rmSync(root, { recursive: true, force: true });
63-
}
64-
});
65-
66-
test('the agent-device CLI argv is the workflow command line, tokenized', () => {
67-
// The fallback is the source-CLI line the device workflows pass explicitly, so a
68-
// hand run without the variable reproduces CI rather than looking for a dist build.
69-
assert.deepEqual(resolveAgentDeviceCliArgv(undefined), [
46+
test('the CLI argv keeps node flags and the entry script apart', () => {
47+
// The default is the source CLI the device workflows name explicitly.
48+
assert.deepEqual(resolveAgentDeviceCliArgv(undefined, undefined), [
7049
'--experimental-strip-types',
7150
'src/bin.ts',
7251
]);
73-
assert.deepEqual(resolveAgentDeviceCliArgv(' '), ['--experimental-strip-types', 'src/bin.ts']);
74-
assert.deepEqual(resolveAgentDeviceCliArgv(' bin/agent-device.mjs '), ['bin/agent-device.mjs']);
52+
// An entry path is never split, so spaces in it survive.
53+
assert.deepEqual(resolveAgentDeviceCliArgv('/tmp/agent device.mjs', ''), [
54+
'/tmp/agent device.mjs',
55+
]);
56+
assert.deepEqual(resolveAgentDeviceCliArgv('/tmp/agent device.mjs', undefined), [
57+
'--experimental-strip-types',
58+
'/tmp/agent device.mjs',
59+
]);
60+
// Flags are split, which is exact because a node flag cannot contain a space.
61+
assert.deepEqual(resolveAgentDeviceCliArgv('bin/x.mjs', '--no-warnings --enable-source-maps'), [
62+
'--no-warnings',
63+
'--enable-source-maps',
64+
'bin/x.mjs',
65+
]);
7566
});

packages/maestro/test/conformance/differential/engine-process.ts

Lines changed: 27 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -32,28 +32,38 @@ export function classifyAgentDeviceFailure(stdout: string): 'behavioral' | 'infr
3232
}
3333
}
3434

35-
/** What the device workflows set AGENT_DEVICE_CLI to, and the default here. */
36-
const DEFAULT_AGENT_DEVICE_CLI = '--experimental-strip-types src/bin.ts';
37-
3835
/**
39-
* AGENT_DEVICE_CLI is a command *line* — node flags plus the entry script —
40-
* mirroring AGENT_DEVICE_PERF_CLI in scripts/perf/config.ts. Tokenizing it here,
41-
* beside the spawn it feeds, is what keeps `runAgentDeviceEngine`'s precondition
42-
* satisfiable: no caller downstream holds a string that is neither a path nor an
43-
* argv. A path with spaces is unreachable through the variable, as in the perf
44-
* harness; pass it as a single array element instead.
36+
* The agent-device CLI the differential drives, as two variables that cannot be
37+
* confused for one another:
38+
*
39+
* AGENT_DEVICE_CLI the entry script — ONE path, never split, so a
40+
* path containing spaces survives verbatim
41+
* AGENT_DEVICE_CLI_NODE_FLAGS node flags — split on whitespace, which is exact
42+
* because a node flag cannot contain a space
43+
*
44+
* One variable holding `--experimental-strip-types src/bin.ts` cannot express
45+
* both: splitting it corrupts `/tmp/agent device.mjs`, and not splitting it
46+
* spawns the whole line as a single node option — the bug that infrastructure-
47+
* failed every scenario from 2026-08-25. Set the flags variable to the empty
48+
* string to run an entry that needs none (a built `bin/agent-device.mjs`).
4549
*/
46-
export function resolveAgentDeviceCliArgv(value: string | undefined): string[] {
47-
return (value?.trim() || DEFAULT_AGENT_DEVICE_CLI).split(/\s+/);
50+
const DEFAULT_CLI_ENTRY = 'src/bin.ts';
51+
const DEFAULT_CLI_NODE_FLAGS = '--experimental-strip-types';
52+
53+
export function resolveAgentDeviceCliArgv(
54+
entry: string | undefined,
55+
nodeFlags: string | undefined,
56+
): string[] {
57+
const flags = (nodeFlags ?? DEFAULT_CLI_NODE_FLAGS).split(/\s+/).filter(Boolean);
58+
return [...flags, entry?.trim() || DEFAULT_CLI_ENTRY];
4859
}
4960

5061
/**
51-
* `cliArgv` is a node argv — node flags plus the entry script — never a single
52-
* command string. AGENT_DEVICE_CLI carries `--experimental-strip-types src/bin.ts`
53-
* on every device workflow, so a lone `string` here spawns node with that whole
54-
* line as one option ("bad option: --experimental-strip-types src/bin.ts") and
55-
* every scenario infrastructure-fails. By the time a path reaches this array it
56-
* is already its own element, spaces and all.
62+
* `cliArgv` is a node argv — flags and entry script as separate elements, built
63+
* by `resolveAgentDeviceCliArgv`. Never a command line: spawning one as a single
64+
* argument aborts node before the CLI loads ("bad option: --experimental-strip-
65+
* types src/bin.ts"), which every scenario then reports as an infrastructure
66+
* failure. An entry path reaching this array is already its own element.
5767
*/
5868
export function runAgentDeviceEngine(cliArgv: readonly string[], args: string[]): EngineResult {
5969
const result = spawnSync(process.execPath, [...cliArgv, ...args, '--json'], {

packages/maestro/test/conformance/differential/run.test.ts

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,99 @@ test('an ordinary Maestro process failure cannot satisfy a behavioral waiver', (
186186
}
187187
});
188188

189+
// The production route the P1 review asked for: environment -> parseRunnerArgs ->
190+
// runScenario -> spawn. Calling runAgentDeviceEngine with a hand-built argv proves
191+
// the spawn, but not that the two variables reach it intact — and it is the
192+
// environment contract that broke the nightly and that a whitespace-split
193+
// AGENT_DEVICE_CLI would break again in the other direction.
194+
describe('the agent-device CLI environment route', () => {
195+
const ROUTE_SCENARIO = {
196+
id: 'cli-env-route',
197+
flow: 'differential/flows/settle-after-tap.yaml',
198+
comparesAcrossEngines: 'test fixture',
199+
expect: 'pass',
200+
divergenceMeans: 'test fixture',
201+
} as const;
202+
203+
/** Run one scenario with AGENT_DEVICE_CLI* set, restoring the environment after. */
204+
function reportForEnvironment(entry: string, nodeFlags: string, maestroCli: string) {
205+
const previous = {
206+
entry: process.env.AGENT_DEVICE_CLI,
207+
flags: process.env.AGENT_DEVICE_CLI_NODE_FLAGS,
208+
};
209+
process.env.AGENT_DEVICE_CLI = entry;
210+
process.env.AGENT_DEVICE_CLI_NODE_FLAGS = nodeFlags;
211+
try {
212+
const options = parseRunnerArgs([]);
213+
return {
214+
argv: options.agentDeviceCliArgv,
215+
report: runScenario(ROUTE_SCENARIO, {
216+
...options,
217+
maestroBin: `${process.execPath} ${maestroCli}`,
218+
}),
219+
};
220+
} finally {
221+
restoreEnv('AGENT_DEVICE_CLI', previous.entry);
222+
restoreEnv('AGENT_DEVICE_CLI_NODE_FLAGS', previous.flags);
223+
}
224+
}
225+
226+
function restoreEnv(name: string, value: string | undefined): void {
227+
if (value === undefined) delete process.env[name];
228+
else process.env[name] = value;
229+
}
230+
231+
/**
232+
* `spaced` is a subdirectory whose NAME carries the space, so the entry path
233+
* holds one whatever the file is called. The maestro stub deliberately stays
234+
* out of it: `runMaestroEngine` still splits its command on spaces, and a
235+
* spaced stub path would fail this test for the other engine's reason.
236+
*/
237+
function withFixtureRoot(run: (spaced: string, maestroCli: string) => void): void {
238+
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-env-route-'));
239+
const spaced = path.join(root, 'agent device');
240+
fs.mkdirSync(spaced);
241+
const maestroCli = path.join(root, 'maestro.mjs');
242+
fs.writeFileSync(maestroCli, 'process.exit(0);\n');
243+
try {
244+
run(spaced, maestroCli);
245+
} finally {
246+
fs.rmSync(root, { recursive: true, force: true });
247+
}
248+
}
249+
250+
test('a CLI path containing spaces reaches the spawn unsplit', () => {
251+
withFixtureRoot((spaced, maestroCli) => {
252+
const entry = path.join(spaced, 'cli.mjs');
253+
fs.writeFileSync(entry, 'process.exit(0);\n');
254+
255+
const { argv, report } = reportForEnvironment(entry, '', maestroCli);
256+
257+
assert.deepEqual(argv, [entry], 'the entry path must stay one argument');
258+
assert.equal(report.agentDevice.outcome, 'pass');
259+
assert.equal(report.failed, false);
260+
});
261+
});
262+
263+
test('node flags stay separate arguments alongside a spaced path', () => {
264+
withFixtureRoot((spaced, maestroCli) => {
265+
const entry = path.join(spaced, 'cli.ts');
266+
fs.writeFileSync(entry, 'const code: number = 0;\nprocess.exit(code);\n');
267+
268+
const { argv, report } = reportForEnvironment(
269+
entry,
270+
'--experimental-strip-types',
271+
maestroCli,
272+
);
273+
274+
// Neither property is expressible in a single whitespace-joined variable.
275+
assert.deepEqual(argv, ['--experimental-strip-types', entry]);
276+
assert.equal(report.agentDevice.outcome, 'pass');
277+
assert.equal(report.failed, false);
278+
});
279+
});
280+
});
281+
189282
test('every device flow targets the fixture app the workflow installs', () => {
190283
for (const scenario of DIFFERENTIAL_SCENARIOS) {
191284
const flowPath = path.join(CONFORMANCE_DIR, scenario.flow);

packages/maestro/test/conformance/differential/run.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,10 @@ export function parseRunnerArgs(argv: readonly string[]): RunnerOptions {
5454
const options: RunnerOptions = {
5555
dryRun: false,
5656
maestroBin: process.env.MAESTRO_BIN ?? 'maestro',
57-
agentDeviceCliArgv: resolveAgentDeviceCliArgv(process.env.AGENT_DEVICE_CLI),
57+
agentDeviceCliArgv: resolveAgentDeviceCliArgv(
58+
process.env.AGENT_DEVICE_CLI,
59+
process.env.AGENT_DEVICE_CLI_NODE_FLAGS,
60+
),
5861
traceRoot: process.env.AGENT_DEVICE_ARTIFACTS_DIR,
5962
};
6063
for (let i = 0; i < argv.length; i += 1) {

0 commit comments

Comments
 (0)