Feat/maestro repeat while by ahmedu007 · Pull Request #3214 · callstack/agent-device

ahmedu007 · 2026-10-04T21:49:34Z

Summary

Maestro flows that used repeat.while were rejected with "repeat.while is not supported; use repeat.times". This adds support that follows the Maestro repeat reference:

- repeat:
    while:
      notVisible: "ValueX"
    commands:
      - tapOn: Button
- evalScript: ${output.counter = 0}
- repeat:
    times: 4            # optional upper bound
    while:
      true: ${output.counter < 3}
    commands:
      - evalScript: ${output.counter++}

Parser: repeat now needs times, while, or both. while accepts platform, visible, notVisible, and true.
Execution: the condition is checked before each iteration. The loop stops when the condition is false or when times is reached, whichever comes first.
- Visibility checks use the same lookup timeout as runFlow.when.
- true expressions run JavaScript through the same evaluator as evalScript. Results count as false using the same rule as assertTrue. So "false", "0", "", "null", and "undefined" from string output values are false.
- Changes the expression makes to output.* carry into later steps.
Security: while.true is refused for flows received over the remote daemon connection, the same as evalScript.
Conformance and docs: added an authored repeat-while flow and regenerated the layer-1 fixtures. Updated the support matrix and replay-e2e.md.

19 files changed. No scope expansion.

Test plan

Ran pnpm check:affected --run on e4dc41a24 and it passed.
- The first run had two unrelated timeouts (interaction-response-shape, apple-platform-output-guard). Both passed when run alone and on the rerun.
New unit tests cover:
- parsing, including the errors for a missing or empty while
- a JavaScript loop
- a visibility loop
- stopping at the times bound
- refusal for untrusted remote flows
- true/false handling of script results
pnpm maestro:conformance passes.
Not tested on a device yet. To check manually, run the YAML above with agent-device replay on an iOS simulator or Android emulator.

Parse and execute repeat.while with platform, visible, notVisible, and JavaScript true conditions, optionally bounded by times. Script results use Maestro string truthiness, and while.true is refused for untrusted remote flows like evalScript.

cubic-dev-ai

All reported issues were addressed across 19 files

_{Reply to a comment to ask cubic a question or push back. It learns from your replies.

Re-trigger cubic}

Copilot

Copilot review overview

🟡 Changes recommended

Per-iteration selector resolution, event-loop cancellation, and boolean conformance normalization have unresolved correctness issues.

Review effort: Balanced
Findings: 1 · 2

Open (3)

Synchronous loop can monopolize the event loop · New
Normalize boolean script conditions to strings · New
Reevaluate loop selectors on every condition check · New

What changed in this PR

Adds Maestro repeat.while support across parsing, execution, conformance, security, and documentation.

Changes:

Supports conditional repeats with optional iteration limits.
Adds JavaScript condition evaluation and remote-flow restrictions.
Adds conformance fixtures, tests, and documentation.

File	Description
`website/docs/docs/replay-e2e.md`	Documents conditional repeats and trust boundaries.
`scripts/maestro-conformance/fixtures/layer1-parser.json`	Updates generated parser fixtures.
`scripts/maestro-conformance/corpus/manifest.json`	Registers the new fixture.
`scripts/maestro-conformance/corpus/authored/repeat-while.yaml`	Adds a representative flow.
`scripts/maestro-conformance/build-manifest.mjs`	Adds fixture metadata.
`packages/maestro/src/internal/support-matrix.ts`	Updates supported capabilities.
`packages/maestro/src/internal/replay-plan-steps.ts`	Preserves repeat conditions in plans.
`packages/maestro/src/internal/replay-plan-step-execution.ts`	Executes conditional loops.
`packages/maestro/src/internal/program-ir.ts`	Extends repeat IR types.
`packages/maestro/src/internal/program-ir-flow-parser.ts`	Parses and validates `while`.
`packages/maestro/src/internal/engine-types.ts`	Extends control descriptors.
`packages/maestro/src/internal/engine-truthiness.ts`	Adds script-result truthiness.
`packages/maestro/src/internal/engine-flow.ts`	Handles repeat condition resolution.
`packages/maestro/src/internal/engine-eval-script.ts`	Returns expression results and output.
`packages/maestro/src/internal/conformance-normalize.ts`	Canonicalizes repeat conditions.
`packages/maestro/src/internal/__tests__/program-ir-parser.test.ts`	Tests parsing and validation.
`packages/maestro/src/internal/__tests__/engine.test.ts`	Tests execution and trust behavior.
`packages/maestro/src/internal/__tests__/engine-truthiness.test.ts`	Tests condition truthiness.
`packages/maestro/src/internal/__tests__/conformance-selector-projection.test.ts`	Tests conformance projection.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

thymikee · 2026-10-04T22:34:57Z

The reviewed commit e4dc41a has one code defect and one missing validation, so it is not ready to merge yet. The one check that ran is green, and there are no conflicts.

The same Maestro true: condition has two evaluators. runFlow.when.true goes through evaluateMaestroBooleanExpression (engine-flow.ts:164), which knows maestro.platform, quoted strings and plain literals. repeat.while.true goes through scriptConditionMatches (replay-plan-step-execution.ts:291), which runs node:vm with no maestro object. So while: {true: "${maestro.platform == 'ios'}"} fails with "maestro is not defined", and a bare literal like true: yes throws a ReferenceError. The same condition passes under runFlow.when, and upstream Maestro accepts both. The rule should be that every Maestro true: condition is evaluated by one owner with the same bindings. Please add the maestro binding and the literal handling to that one owner, or route both commands through one evaluator. Then add tests for repeat.while.true with maestro.platform and with a non-${} literal.

The PR says it is not tested on a device. repeat.while.visible and notVisible call observe() on a live snapshot each iteration (replay-plan-step-execution.ts:245). Nothing yet shows that the loop ends when a real element appears, or that the cached observation is not reused across iterations. Please run agent-device test <flow> --maestro on an iOS simulator or Android emulator with a repeat: while: notVisible: X loop whose body makes X appear after N taps. Paste the output showing N body iterations and the loop exit. Also paste a times-bounded while.true run that shows the final output counter value.

Could MaestroRepeatCondition and parseMaestroRepeatCondition go away? The type is a Pick of all four MaestroRunFlowCondition fields, so it is the same type. parseMaestroRunFlowCondition already threads name, so it could take the field name and serve both commands. One matching function could then serve runFlow.when and repeat.while. That would also remove the second UNAUTHORIZED message and the special case in resolveCommand. First, we would need to decide whether true: is a restricted expression or full JS for every command, and record it in the support matrix.

On the open review threads, r4179477864 and its duplicate r4179481837 still stand (selectors resolved once). The 0n truthiness thread r4179477876 and the Copilot starvation thread r4179481812 still stand, as do the parser copy r4179477880, the missing empty-string test case r4179477883, the duplicated UNAUTHORIZED text r4179477886 and the boolean normalization thread r4179481825. Two threads do not apply, so please resolve them. r4179477867: running the script before the platform check matches upstream. r4179477871: an unbounded loop without times is upstream behavior, and cancellation is checked each iteration.

I did not run the tests or pnpm maestro:conformance. I also did not read upstream Maestro's source, so the upstream points above come from memory.

Before merge, repeat.while.true must use the same bindings and rules as runFlow.when.true, and the live simulator or emulator run must be attached.

…ditions Upstream stores a literal true: condition as a string scriptCondition, so the agent projection stringifies booleans too.

…ition owner Both conditions share one parser, one IR type, and one true: evaluator. Booleans, maestro.platform comparisons, and literal text after ${VAR} lookups need no JavaScript; other expressions run in node:vm with a maestro.platform binding and are refused for untrusted remote flows through a shared trust check. Condition selectors are resolved at every check, and repeat yields to the event loop each iteration so cancellation timers can fire.

cubic-dev-ai

All reported issues were addressed across 15 files (changes from recent commits).

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

Copilot

Copilot review overview

🟡 Changes recommended

The expanded engine test file violates the repository’s 1,000-line limit, and live device validation remains outstanding.

Review effort: Balanced
Findings: 1

Open (1)

Move repeat-while tests to keep engine.test.ts under 1,000 lines · New

Resolved since last review (3)

Synchronous loop can monopolize the event loop
Reevaluate loop selectors on every condition check
Normalize boolean script conditions to strings

thymikee · 2026-10-05T07:34:47Z

Thanks for the update. At e3f1ecf, one code issue remains and no device run is attached yet, so it is not ready. CI is green. There are no conflicts.

Since the earlier review (#3214 (comment)), the loop now yields with setImmediate and checks cancellation on every iteration, and a cancellation test covers it. That part is fixed.

One problem remains. repeat.while.visible and notVisible call port.observe() on a live snapshot before every iteration (https://github.com/callstack/agent-device/blob/e3f1ecf/packages/maestro/src/internal/replay-plan-step-execution.ts#L245). The only proof is a unit test whose mocked observe always returns matched: true. The PR body still says "Not tested on a device yet", and I see no run attached since the last review. So nothing shows that a real loop exits once the element appears, or that the observation is not served stale across iterations. Please run agent-device test <flow> --maestro on an iOS simulator or Android emulator with repeat: while: notVisible: X, where the body makes X appear after N taps. Paste the output showing exactly N body iterations, then the loop exit. Please also paste a times-bounded while.true: ${output.counter < 3} run that prints the final counter value.

The Copilot thread on engine.test.ts still applies (#3214 (comment)). The file grew from 877 to 1076 lines, which crosses the 1,000 line limit in docs/agents/testing.md. Please move the repeat-while cases into a sibling test module. Three cubic-dev-ai threads do not apply, so please resolve them. The unbounded while without times matches upstream Maestro, and the loop now yields and checks cancellation (#3214 (comment)). The ${output.counter} < 3 case becomes literal text after interpolation, which is the documented interpolate-then-truthy behavior (#3214 (comment)). Nothing read the interpolated runFlow label before or after this change (#3214 (comment)). I recalled the upstream behavior from memory and did not check upstream source.

I did not run tests or pnpm maestro:conformance. I judged regressions by reading the pre-change code paths. I also did not check whether the static runFlow planner accepts plain literals such as yes. The runtime fallback decides those cases either way.

Before merge, please attach the live simulator or emulator run and split engine.test.ts back under 1,000 lines.

… module Keeps engine.test.ts under the 1,000-line test file limit; makePort moves to the shared runtime-port fixtures.

Copilot

Copilot review overview

🔵 Needs a closer look

The accepted architecture and security documentation still incorrectly identifies evalScript as the sole JavaScript execution surface.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Move repeat-while tests to keep engine.test.ts under 1,000 lines

Previously missed (1)

In code that hasn't changed since last review

Document condition expression evaluation and its trust boundary

packages/maestro/src/internal/engine-flow.ts:170

This introduces JavaScript evaluation outside evalScript, but accepted ADR 0015 still states that ${...} is lookup-only outside evalScript, that evalScript is the sole JavaScript-evaluating command, and that trustedScripts gates only evalScript (docs/adr/0015-direct-maestro-engine.md:193-207). Update that decision (and the trustedScripts API comment) to record condition true: evaluation and its trust boundary; otherwise the architecture and security documentation contradict the implementation.

ahmedu007 · 2026-10-05T09:05:49Z

Device run on the iOS simulator (iPhone 18 Pro Max, iOS 27.0) at ecdc3986a (engine source unchanged since e3f1ecf95):

agent-device test repeat-while-not-visible.yaml repeat-while-true-counter.yaml --maestro --platform ios
✓ repeat while notVisible exits once the element appears 26.8s
✓ times-bounded repeat while true stops at counter 3 2.06s
Test summary: 2 passed (2) in 28.8s

1. repeat: while: notVisible: X, where X appears after N=3 taps. The app under test is Safari showing a local page. Its Tap me button updates Taps: <n>, and on the 3rd tap it renders Target reached. The loop has a times: 10 ceiling, so a stale observation would show up as over-tapping or a timeout.

appId: com.apple.mobilesafari
---
- openLink: http://127.0.0.1:8765/counter.html
- assertVisible: Tap me
- evalScript: ${output.taps = 0}
- repeat:
    times: 10
    while:
      notVisible: Target reached
    commands:
      - tapOn: Tap me
      - evalScript: ${output.taps++}
- assertVisible: Target reached
- assertVisible: 'Taps: 3'
- evalScript: ${output.exactlyThree = output.taps == 3}
- assertTrue: ${output.exactlyThree}

replay-timing.ndjson:

step 1 line 4  openLink      ok=true 14058ms
step 2 line 5  assertVisible ok=true   387ms
step 3 line 6  evalScript    ok=true     6ms
step 4 line 7  repeat        ok=true 11806ms
step 5 line 14 assertVisible ok=true   216ms   # Target reached
step 6 line 15 assertVisible ok=true   222ms   # Taps: 3, counted by the page
step 7 line 16 evalScript    ok=true     4ms
step 8 line 17 assertTrue    ok=true     3ms   # loop body ran exactly 3 times

The body ran exactly 3 times. The page itself shows Taps: 3, and the flow's own count output.taps == 3 agrees. The loop then exited once Target reached appeared. A 4th iteration would have produced Taps: 4 and failed step 6.

2. times-bounded while.true: ${output.counter < 3}. To show the final counter value on the device, the flow opens the same page with ?counter=${output.counter}. The page renders Final counter: <value>.

appId: com.apple.mobilesafari
---
- evalScript: ${output.counter = 0}
- repeat:
    times: 10
    while:
      true: ${output.counter < 3}
    commands:
      - evalScript: ${output.counter++}
- openLink: http://127.0.0.1:8765/counter.html?counter=${output.counter}
- assertVisible: 'Final counter: 3'

step 1 line 4  evalScript    ok=true   34ms
step 2 line 5  repeat        ok=true    5ms
step 3 line 11 openLink      ok=true 1719ms
step 4 line 12 assertVisible ok=true  257ms   # Final counter: 3

Android: I couldn't run this on the available emulator (API 37). The bundled snapshot helper crashes there before any flow step: hiddenapi: Accessing hidden field Instrumentation->mUiAutomation ... denied, then IllegalStateException: Cannot call disconnect() while connecting UiAutomation. That is an existing helper problem on API 37 and is unrelated to this PR. It happens on a plain agent-device snapshot too.

thymikee · 2026-10-05T09:37:15Z

I found no problems in ecdc398, and the fixes from the earlier review at e3f1ecf are in. The new push only moves the repeat-while tests out of engine.test.ts into replay-plan-step-execution.test.ts, so engine.test.ts is back to 858 lines. I did not run the moved tests locally. I judged from the delta diff that they are identical. I also did not check the Android device route because of the API 37 helper crash you report. The changed engine code is platform-neutral. The device-run output is your own paste, and I did not reproduce it.

CI is green: the one check reported so far passes, and this push changes only tests. Cubic has not reviewed this head yet. No conflicts are known. Before merge, wait for the remaining checks and for Cubic's re-review of ecdc398.

On the other threads: the file-size thread (#3214 (comment)) is fixed at this head, so please resolve it. The unbounded while without times (#3214 (comment)) matches upstream Maestro. The loop yields and checks cancellation, and the cancellation test covers it, so please resolve it. The ${output.counter} < 3 interpolation thread (#3214 (comment)) describes the documented interpolate-then-truthy behavior, so please resolve it. The runFlow label thread (#3214 (comment)) is benign because nothing reads that label before or after this change, so please resolve it.

* origin/main: (77 commits) fix(apple-runner): fence prep spawns behind a start-owned admission (callstack#3239) 0.21.22 test(apple): own the simctl settings plan tests in simctl-settings.test.ts (callstack#3244) fix(limrun): report the session device id in iOS settings refusals (callstack#3243) 0.21.21 feat(remote): add a host-allocated macos-app lease backend (callstack#3236) test(android): bound the screenshot write wait by wall time, not event-loop turns (callstack#3250) feat(recording): cap the touch overlay frame rate at the caller's --fps (callstack#3241) fix(ad-script): let .ad scripts carry scroll --until and wait capture flags (callstack#3197) (callstack#3234) feat(provider-webdriver): keyboard enter, dismiss, and status over WebDriver (callstack#3233) feat(selectors): match role= against snapshot kind with a node-scoped alias window (callstack#3232) fix(provider-webdriver): read field values, placeholders, secure fields, and checked state from page source (callstack#3231) feat(replay): accept --test-ime on test and replay so flow-owned Android opens opt into the test IME (callstack#3235) refactor(daemon): route daemon-level diagnostics through one scope helper (callstack#3242) docs(adr): correct ADR 0031 pointer event delivery evidence (callstack#3245) fix(ios): stop a tap's post-gesture lookup from recording an XCTest failure (callstack#3060) (callstack#3237) fix(recording): render the touch overlay at most 30 fps and inside the record request (callstack#3219) fix(daemon): keep an idle daemon alive only for retained leases (callstack#3227) fix(provider-webdriver): send an empty JSON object on bodyless POSTs (callstack#3230) Feat/maestro repeat while (callstack#3214) ...

ahmedu007 added 3 commits October 4, 2026 22:36

test(maestro): add repeat.while conformance coverage

2000300

docs(maestro): document repeat.while support

e4dc41a

Copilot AI balanced review requested due to automatic review settings October 4, 2026 21:49

Copilot started reviewing on behalf of ahmedu007 October 4, 2026 21:50 View session

cubic-dev-ai Bot reviewed Oct 4, 2026

View reviewed changes

Copilot AI reviewed Oct 4, 2026

View reviewed changes

Comment thread packages/maestro/src/internal/replay-plan-step-execution.ts

Comment thread packages/maestro/src/internal/conformance-normalize.ts Outdated

Comment thread packages/maestro/src/internal/engine-flow.ts Outdated

ahmedu007 added 2 commits October 5, 2026 07:14

fix(maestro): treat bigint zero as falsy and canonicalize boolean con…

42af17e

…ditions Upstream stores a literal true: condition as a string scriptCondition, so the agent projection stringifies booleans too.

Copilot AI balanced review requested due to automatic review settings October 5, 2026 06:25

Copilot started reviewing on behalf of ahmedu007 October 5, 2026 06:25 View session

cubic-dev-ai Bot reviewed Oct 5, 2026

View reviewed changes

Comment thread packages/maestro/src/internal/engine-flow.ts

Comment thread packages/maestro/src/internal/replay-plan-step-execution.ts

Copilot AI reviewed Oct 5, 2026

View reviewed changes

Comment thread packages/maestro/src/internal/__tests__/engine.test.ts Outdated

test(maestro): move repeat and condition execution cases to a sibling…

ecdc398

… module Keeps engine.test.ts under the 1,000-line test file limit; makePort moves to the shared runtime-port fixtures.

Copilot AI balanced review requested due to automatic review settings October 5, 2026 08:58

Copilot started reviewing on behalf of ahmedu007 October 5, 2026 08:58 View session

Copilot AI reviewed Oct 5, 2026

View reviewed changes

thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 5, 2026

thymikee merged commit 39c4f53 into callstack:main Oct 5, 2026
16 checks passed

Uh oh!

Conversation

ahmedu007 commented Oct 4, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thymikee commented Oct 4, 2026

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

thymikee commented Oct 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

ahmedu007 commented Oct 5, 2026

Uh oh!

thymikee commented Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ahmedu007 commented Oct 4, 2026 •

edited by cubic-dev-ai Bot

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading