refactor: narrow Maestro flow runtime bridge#626
Conversation
thymikee
left a comment
There was a problem hiding this comment.
Review — narrow Maestro flow runtime bridge
Strong structural improvement. Replacing the stringly-typed __maestroRunFlowWhen / __maestroRetry commands + batchSteps-smuggled-through-flags with a typed SessionAction.replayControl discriminated union is much cleaner, and it lets control-flow-runtime drop its core/dispatch (CommandFlags) dependency entirely. Executing control metadata before daemon dispatch via invokeReplayControl(...) ?? invokeMaestroRuntimeCommand(...) reads well, and retry is now genuinely backend-agnostic rather than Maestro-private — good direction for the broader compat layer.
A few points:
-
Add an exhaustiveness check to
invokeReplayControl. Bothswitcharms return, so the trailingreturn undefinedis dead code today, but it also means adding a thirdSessionReplayControl.kindlater will silently fall through toundefined(→ dispatched as a literal command) with no compile error. Adefault: { const _exhaustive: never = control; return undefined; }would turn that into a type error. -
Double variable-resolution of nested actions.
resolveReplayActionnow runsresolveStringProps(action.replayControl, ...), which deep-resolves${VAR}inside every nestedactions[].positionals/flags. Those same nested actions are then resolved again when executed viainvokeReplayActionBlock→invokeReplayAction. It's idempotent for normal values, but a resolved value that itself contains${...}(or any non-idempotent token) would be re-substituted. Probably benign, but a test with a variable used inside arunFlow.whencommand would pin down the intended single vs. double pass. -
Serialization round-trip.
SessionActioncan now carry a nestedSessionAction[]viareplayControl. If actions are ever persisted/serialized to artifacts or the session log, confirm the nested structure round-trips (the oldbatchStepslived inflags, which presumably already serialized). Just a sanity check, not a known break.
The runFlow.when + retry integration guard is the right thing to add alongside this — though note it duplicates the test added in #620 (see my comment there about picking a single home).
Nice cleanup overall.
Generated by Claude Code
d7dcb1d to
4b472a7
Compare
|
Rebased onto latest
Local validation is reflected in the PR body. CI is running on the rebased head. |
|
Summary
__maestroRunFlowWhenand__maestroRetrycommand bridge with replay-owned in-memory control metadata that executes before daemon dispatch.replay -userialization.Touched files: 12.
The remaining Maestro runtime commands still cover behavior that does not map cleanly to native batch/wait/replay semantics yet. I do not see a remaining persistence gap for this PR: Maestro compat flow controls are rejected for
replay -ubefore serialization, andSessionAction.replayControlremains an in-memory execution contract for parsed Maestro YAML.Validation
pnpm exec vitest run --project provider-integration test/integration/provider-scenarios/android-test-suite.test.tspassed.pnpm exec vitest run src/daemon/handlers/__tests__/session-replay-vars.test.ts -t "Maestro compat flow controls|Maestro runFlow.when|Maestro retry|nested Maestro runFlow.when"passed.pnpm exec vitest run src/compat/maestro/__tests__/replay-flow.test.ts src/compat/maestro/__tests__/runtime-flow.test.tspassed.pnpm exec vitest run src/compat/maestro/__tests__/replay-flow.test.ts src/compat/maestro/__tests__/runtime-flow.test.ts src/daemon/handlers/__tests__/session-replay-vars.test.ts -t "Maestro compat flow controls|Maestro runFlow.when|Maestro retry|nested Maestro runFlow.when|visible-gated runFlow|retry commands"passed.pnpm typecheckpassed.pnpm exec oxfmt --check src/compat/maestro/flow-control.ts src/compat/maestro/runtime-flow.tspassed.pnpm check:fallow --base origin/mainpassed.Note: a broader combined Vitest run that included
session-replay-vars.test.tsand Maestro compat tests hit the existing/private/tmpworktreerunScriptchild-process issue:Child process exited before writing stdout.