fix(maestro): address review feedback on numeric resolution

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

Author: thymikee

scripts/maestro-conformance/normalize.ts

@@ -322,20 +322,20 @@ function canonicalizeAgentCommand(
         stopApp: command.stopApp,
       });
     case 'tapOn': {
-      const repeat = command.repeat ?? 1;
+      const repeat = num(command.repeat) ?? 1;
       return canonicalTap({
         longPress: false,
         repeat,
-        delay: repeat > 1 ? (command.delay ?? AGENT_REPEAT_DELAY_MS) : undefined,
-        target: agentTarget(command.target, command.index, command.childOf),
+        delay: repeat > 1 ? (num(command.delay) ?? AGENT_REPEAT_DELAY_MS) : undefined,
+        target: agentTarget(command.target, num(command.index), command.childOf),
       });
     }
     case 'doubleTapOn':
       // Upstream compiles doubleTapOn to a repeat-2 tap with the same default delay.
       return canonicalTap({
         longPress: false,
         repeat: 2,
-        delay: command.delay ?? AGENT_REPEAT_DELAY_MS,
+        delay: num(command.delay) ?? AGENT_REPEAT_DELAY_MS,
         target: agentTarget(command.target),
       });
     case 'longPressOn':
@@ -366,7 +366,7 @@ function canonicalizeAgentCommand(
     case 'inputText':
       return dropUndefined({ kind: 'inputText', text: command.text });
     case 'eraseText':
-      return dropUndefined({ kind: 'eraseText', count: command.charactersToErase });
+      return dropUndefined({ kind: 'eraseText', count: num(command.charactersToErase) });
     case 'openLink':
       return dropUndefined({ kind: 'openLink', link: command.link });
     case 'scroll':
@@ -386,13 +386,13 @@ function canonicalizeAgentCommand(
     case 'takeScreenshot':
       return { kind: 'takeScreenshot' };
     case 'waitForAnimationToEnd':
-      return dropUndefined({ kind: 'waitForAnimationToEnd', timeout: command.timeout });
+      return dropUndefined({ kind: 'waitForAnimationToEnd', timeout: num(command.timeout) });
     case 'stopApp':
       return { kind: 'stopApp' };
     case 'repeat':
-      return { kind: 'repeat', times: command.times };
+      return { kind: 'repeat', times: num(command.times) ?? str(command.times) };
     case 'retry':
-      return dropUndefined({ kind: 'retry', maxRetries: command.maxRetries });
+      return dropUndefined({ kind: 'retry', maxRetries: num(command.maxRetries) ?? str(command.maxRetries) });
     case 'runFlow':
       return { kind: 'runFlow', source: command.include.kind === 'file' ? 'file' : 'commands' };
     case 'runScript':
@@ -436,7 +436,7 @@ function agentSelector(
 }
 
 function agentGesture(gesture: MaestroSwipeGesture): CanonicalGesture {
-  const duration = gesture.duration ?? AGENT_SWIPE_DEFAULT_DURATION;
+  const duration = num(gesture.duration) ?? AGENT_SWIPE_DEFAULT_DURATION;
   switch (gesture.kind) {
     case 'screen':
       return { mode: 'direction', direction: gesture.direction, duration };

src/compat/maestro/__tests__/engine-flow.test.ts

@@ -16,7 +16,18 @@ test('resolveNumeric coerces valid resolved strings and numbers', () => {
   expect(resolveNumeric(undefined, 'x', {})).toBeUndefined();
 });
 
-test('resolveNumeric rejects blank, whitespace, and malformed resolved strings', () => {
+test('resolveNumeric trims whitespace before coercion', () => {
+  expect(resolveNumeric('  42  ', 'x', {})).toBe(42);
+  expect(resolveNumeric('  0  ', 'x', { integer: true, nonNegative: true })).toBe(0);
+});
+
+test('resolveNumeric accepts JS numeric forms for backward compatibility', () => {
+  expect(resolveNumeric('1e3', 'x', {})).toBe(1000);
+  expect(resolveNumeric('+5', 'x', {})).toBe(5);
+  expect(resolveNumeric('.5', 'x', {})).toBe(0.5);
+});
+
+test('resolveNumeric rejects blank, whitespace, malformed, and unsafe resolved strings', () => {
   expect(() => resolveNumeric('', 'x', {})).toThrow(/must be a finite number/);
   expect(() => resolveNumeric('   ', 'x', {})).toThrow(/must be a finite number/);
   expect(() => resolveNumeric('', 'x', { integer: true, nonNegative: true })).toThrow(
@@ -29,3 +40,23 @@ test('resolveNumeric rejects blank, whitespace, and malformed resolved strings',
   expect(() => resolveNumeric('1.2.3', 'x', {})).toThrow(/must be a finite number/);
   expect(() => resolveNumeric('1.5', 'x', { integer: true })).toThrow(/must be an integer/);
 });
+
+test('resolveNumeric rejects out-of-range and negative values per constraints', () => {
+  const huge = String(Number.MAX_SAFE_INTEGER + 1);
+  expect(() => resolveNumeric(huge, 'x', {})).toThrow(/must be a finite number/);
+  expect(() => resolveNumeric('-1', 'x', { nonNegative: true })).toThrow(
+    /must be a non-negative finite number/,
+  );
+  expect(() => resolveNumeric('0', 'x', { integer: true, positive: true })).toThrow(
+    /must be a positive integer/,
+  );
+});
+
+test('resolveNumeric resolves constraints from the shared field map when omitted', () => {
+  expect(resolveNumeric('1000', 'extendedWaitUntil.timeout')).toBe(1000);
+  expect(() => resolveNumeric('-1', 'extendedWaitUntil.timeout')).toThrow(
+    /must be a non-negative finite number/,
+  );
+  expect(resolveNumeric('5', 'tapOn.repeat')).toBe(5);
+  expect(() => resolveNumeric('0', 'tapOn.repeat')).toThrow(/must be a positive integer/);
+});

src/compat/maestro/__tests__/runtime-port.test.ts

@@ -432,12 +432,83 @@ describe('MaestroRuntimePort', () => {
       executeMaestroProgram(program, createMaestroRuntimePort(operations), {
         env: { TIMEOUT: '' },
       }),
-    ).rejects.toThrow(/extendedWaitUntil\.timeout must be a finite number/);
+    ).rejects.toThrow(/extendedWaitUntil\.timeout must be a non-negative finite number/);
 
     await expect(
       executeMaestroProgram(program, createMaestroRuntimePort(operations), {
         env: { TIMEOUT: '   ' },
       }),
-    ).rejects.toThrow(/extendedWaitUntil\.timeout must be a finite number/);
+    ).rejects.toThrow(/extendedWaitUntil\.timeout must be a non-negative finite number/);
+  });
+
+  test('applies the default delay for doubleTapOn when none is specified', async () => {
+    const calls: RecordedCall[] = [];
+    const operations = makeOperations({
+      doubleTapOn: vi.fn(async (input, context) => record(calls, 'doubleTapOn', input, context)),
+    });
+    const program = parseMaestroProgram('---\n- doubleTapOn:\n    id: button\n');
+
+    await executeMaestroProgram(program, createMaestroRuntimePort(operations), {});
+
+    expect(calls).toHaveLength(1);
+    expect(calls[0]).toMatchObject({ kind: 'doubleTapOn', input: { delay: 100 } });
+  });
+
+  test('resolves tapOn.index into the target query', async () => {
+    const resolveTarget = vi.fn(
+      async (_input: unknown, context: { generation: number }): Promise<MaestroTargetMatch> => ({
+        generation: context.generation,
+        matched: true,
+        visible: true,
+        candidateCount: 1,
+        rect: { x: 0, y: 0, width: 100, height: 50 },
+        viewport: { x: 0, y: 0, width: 400, height: 800 },
+        ref: 'button',
+      }),
+    );
+    const tapOn = vi.fn();
+    const operations = makeOperations({ resolveTarget, tapOn });
+    const program = parseMaestroProgram('---\n- tapOn:\n    id: button\n    index: ${INDEX}\n');
+
+    await executeMaestroProgram(program, createMaestroRuntimePort(operations), {
+      env: { INDEX: '2' },
+    });
+
+    expect(resolveTarget).toHaveBeenCalledWith(
+      expect.objectContaining({ index: 2 }),
+      expect.anything(),
+    );
+    expect(tapOn).toHaveBeenCalled();
+  });
+
+  test('rejects negative, huge, and accepts whitespace-padded resolved numeric values', async () => {
+    const operations = makeOperations();
+    const negative = parseMaestroProgram(
+      '---\n- extendedWaitUntil:\n    visible: Ready\n    timeout: ${TIMEOUT}\n',
+    );
+
+    await expect(
+      executeMaestroProgram(negative, createMaestroRuntimePort(operations), {
+        env: { TIMEOUT: '-1' },
+      }),
+    ).rejects.toThrow(/extendedWaitUntil\.timeout must be a non-negative finite number/);
+
+    const huge = parseMaestroProgram(
+      '---\n- extendedWaitUntil:\n    visible: Ready\n    timeout: ${TIMEOUT}\n',
+    );
+
+    await expect(
+      executeMaestroProgram(huge, createMaestroRuntimePort(operations), {
+        env: { TIMEOUT: '99999999999999999' },
+      }),
+    ).rejects.toThrow(/extendedWaitUntil\.timeout must be a non-negative finite number/);
+
+    const padded = parseMaestroProgram('---\n- waitForAnimationToEnd: ${TIMEOUT}\n');
+
+    await expect(
+      executeMaestroProgram(padded, createMaestroRuntimePort(operations), {
+        env: { TIMEOUT: '  1000  ' },
+      }),
+    ).resolves.toBeDefined();
   });
 });

src/compat/maestro/engine-flow.ts

@@ -1,7 +1,11 @@
 import path from 'node:path';
 import { AppError } from '../../kernel/errors.ts';
 import { createRequestCanceledError } from '../../request/cancel.ts';
-import { INTEGER_STRING_PATTERN, NUMERIC_STRING_PATTERN } from './program-ir-values.ts';
+import {
+  MAESTRO_NUMERIC_FIELD_CONSTRAINTS,
+  numericDescription,
+  type NumericScalarConstraints,
+} from './program-ir-values.ts';
 import type {
   MaestroCommand,
   MaestroProgram,
@@ -22,56 +26,37 @@ export function resolveCommand<T extends { readonly source: MaestroCommand['sour
   };
 }
 
-export type ResolveNumericConstraints = {
-  integer?: boolean;
-  nonNegative?: boolean;
-  positive?: boolean;
-};
-
-function resolveNumericDescription(constraints: ResolveNumericConstraints): string {
-  if (constraints.positive) return 'a positive integer';
-  if (constraints.nonNegative) return 'a non-negative integer';
-  if (constraints.integer) return 'an integer';
-  return 'a finite number';
-}
-
 export function resolveNumeric(
   value: number | string | undefined,
   name: string,
-  constraints: ResolveNumericConstraints = {},
+  constraints?: NumericScalarConstraints,
 ): number | undefined {
   if (value === undefined) return undefined;
-  const num =
-    typeof value === 'number'
-      ? value
-      : Number(parseResolvedNumericString(value, name, constraints));
-  const description = resolveNumericDescription(constraints);
-  if (!Number.isFinite(num)) {
-    throw new AppError('INVALID_ARGS', `Maestro ${name} must be ${description}.`);
+  const effectiveConstraints = constraints ?? MAESTRO_NUMERIC_FIELD_CONSTRAINTS[name] ?? {};
+  const description = numericDescription(effectiveConstraints);
+  let num: number;
+  if (typeof value === 'number') {
+    num = value;
+  } else {
+    const trimmed = value.trim();
+    if (trimmed === '') {
+      throw new AppError('INVALID_ARGS', `Maestro ${name} must be ${description}.`);
+    }
+    num = Number(trimmed);
   }
-  if (constraints.integer && !Number.isInteger(num)) {
+  if (!Number.isFinite(num) || Math.abs(num) > Number.MAX_SAFE_INTEGER) {
     throw new AppError('INVALID_ARGS', `Maestro ${name} must be ${description}.`);
   }
-  if (constraints.nonNegative && num < 0) {
+  if (effectiveConstraints.integer && !Number.isSafeInteger(num)) {
     throw new AppError('INVALID_ARGS', `Maestro ${name} must be ${description}.`);
   }
-  if (constraints.positive && num <= 0) {
+  if (effectiveConstraints.nonNegative && num < 0) {
     throw new AppError('INVALID_ARGS', `Maestro ${name} must be ${description}.`);
   }
-  return num;
-}
-
-function parseResolvedNumericString(
-  value: string,
-  name: string,
-  constraints: ResolveNumericConstraints,
-): string {
-  const description = resolveNumericDescription(constraints);
-  const pattern = constraints.integer ? INTEGER_STRING_PATTERN : NUMERIC_STRING_PATTERN;
-  if (!pattern.test(value)) {
+  if (effectiveConstraints.positive && num <= 0) {
     throw new AppError('INVALID_ARGS', `Maestro ${name} must be ${description}.`);
   }
-  return value;
+  return num;
 }
 
 export function readIterationCount(
@@ -81,7 +66,7 @@ export function readIterationCount(
   name: string,
 ): number {
   const resolved = value === undefined ? fallback : context.resolve(String(value));
-  return resolveNumeric(resolved, name, { integer: true, nonNegative: true }) ?? fallback;
+  return resolveNumeric(resolved, name)!;
 }
 
 export function checkpointMaestroCancellation(signal: AbortSignal | undefined): void {

src/compat/maestro/program-ir-command-parser.ts

@@ -46,10 +46,9 @@ import {
   readOptionalBoolean,
   readOptionalCommandOption,
   readOptionalEntry,
-  readOptionalNumber,
-  readOptionalPositiveInteger,
+  readOptionalNumeric,
   readOptionalString,
-  readRequiredPositiveInteger,
+  readRequiredNumeric,
   readRequiredString,
   readScalarMap,
   readScalarValue,
@@ -221,12 +220,12 @@ function parseEraseText(
     return {
       kind: 'eraseText',
       source,
-      charactersToErase: readRequiredPositiveInteger(value, 'eraseText', context),
+      charactersToErase: readRequiredNumeric(value, 'eraseText', context),
     };
   const entries = readMapEntries(value, 'eraseText', context);
   assertOnlyKeys(entries, 'eraseText', ['charactersToErase'], context);
   const charactersToErase = hasEntry(entries, 'charactersToErase')
-    ? readOptionalPositiveInteger(
+    ? readOptionalNumeric(
         entryValue(entries, 'charactersToErase'),
         'eraseText.charactersToErase',
         context,
@@ -354,7 +353,7 @@ function parseExtendedWaitUntil(
   const options = readOptionalCommandOption(entries, 'extendedWaitUntil', context);
   const condition = parseExtendedWaitUntilCondition(entries, commandNode, context);
   const timeout = hasEntry(entries, 'timeout')
-    ? readOptionalNumber(entryValue(entries, 'timeout'), 'extendedWaitUntil.timeout', context)
+    ? readOptionalNumeric(entryValue(entries, 'timeout'), 'extendedWaitUntil.timeout', context)
     : undefined;
   const optional = options.optional === true || condition.optional === true ? true : undefined;
   const command: MaestroExtendedWaitUntilCommand = {
@@ -425,7 +424,7 @@ function parseScrollUntilVisible(
       )
     : undefined;
   const timeout = hasEntry(entries, 'timeout')
-    ? readOptionalNumber(entryValue(entries, 'timeout'), 'scrollUntilVisible.timeout', context)
+    ? readOptionalNumeric(entryValue(entries, 'timeout'), 'scrollUntilVisible.timeout', context)
     : undefined;
   const optional = options.optional === true || parsedElement!.optional === true ? true : undefined;
   return stripUndefined({
@@ -476,13 +475,13 @@ function parseWaitForAnimationToEnd(
   const source = sourceAt(commandNode, context);
   if (isNullNode(value)) return { kind: 'waitForAnimationToEnd', source };
   if (isScalar(value)) {
-    const timeout = readOptionalNumber(value, 'waitForAnimationToEnd', context);
+    const timeout = readOptionalNumeric(value, 'waitForAnimationToEnd', context);
     return stripUndefined({ kind: 'waitForAnimationToEnd' as const, source, timeout });
   }
   const entries = readMapEntries(value, 'waitForAnimationToEnd', context);
   assertOnlyKeys(entries, 'waitForAnimationToEnd', ['timeout'], context);
   const timeout = hasEntry(entries, 'timeout')
-    ? readOptionalNumber(entryValue(entries, 'timeout'), 'waitForAnimationToEnd.timeout', context)
+    ? readOptionalNumeric(entryValue(entries, 'timeout'), 'waitForAnimationToEnd.timeout', context)
     : undefined;
   return stripUndefined({ kind: 'waitForAnimationToEnd' as const, source, timeout });
 }

src/compat/maestro/program-ir-flow-parser.ts

@@ -13,8 +13,8 @@ import {
   entryValue,
   hasEntry,
   invalidAt,
-  readIntegerValue,
   readMapEntries,
+  readRequiredNumeric,
   readOptionalString,
   readOptionalEntry,
   readRequiredString,
@@ -137,7 +137,7 @@ export function parseMaestroRepeatCommand(
   return {
     kind: 'repeat',
     source,
-    times: readIntegerValue(entryValue(entries, 'times'), 'repeat.times', context),
+    times: readRequiredNumeric(entryValue(entries, 'times'), 'repeat.times', context),
     commands: parseCommands(entryValue(entries, 'commands'), 'repeat.commands', context),
   };
 }
@@ -154,7 +154,7 @@ export function parseMaestroRetryCommand(
   if (!hasEntry(entries, 'commands'))
     invalidAt('Maestro retry requires commands.', commandNode, context);
   const maxRetries = hasEntry(entries, 'maxRetries')
-    ? readIntegerValue(entryValue(entries, 'maxRetries'), 'retry.maxRetries', context)
+    ? readRequiredNumeric(entryValue(entries, 'maxRetries'), 'retry.maxRetries', context)
     : undefined;
   return stripUndefined({
     kind: 'retry' as const,