feat(orchestra): derive the flow-command schema from the parser's own types - #3560
feat(orchestra): derive the flow-command schema from the parser's own types#3560amanjeetsingh150 wants to merge 15 commits into
Conversation
… types Adds maestro.orchestra.yaml.schema.FlowCommandSchema — the command surface (names, arguments, kinds, enum vocabularies, alternative shapes, which commands may be written bare) derived by reflection from YamlFluentCommand's constructor, each command's Yaml* data class and the parser's stringCommands map. Nothing is hand-maintained, so it cannot drift from what the parser accepts. It ships as an API, not an artifact: consumers already depending on dev.mobile:maestro-orchestra call FlowCommandSchema.commands() or asJson(). .github/workflows/publish-schemas.yaml, which uploaded a hand-written JSON schema to GCS, is deleted. Supporting changes: - DarkModeValue / AirplaneValue carry their YAML wire names as @JsonProperty instead of hiding them in string literals inside the deserializers. This is the only behaviour change: setDarkMode's map form now honours `optional`, which it previously dropped, and both commands reject near-miss values that the old substring matching let through. - YamlSwipe becomes a sealed interface so its four shapes are discoverable. - stringCommands becomes internal so the schema can read its keys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012naxaR7AjmbDCmckC2Y4ru
… fill it in `required` was derived from `isOptional`, which answers a Kotlin question -- does the parameter have a default -- not the YAML one. A nullable parameter without a default still deserializes when the key is absent, because Jackson supplies null. That made the schema claim 9 arguments across 4 commands were mandatory when they are not, `- launchApp` on its own being the obvious counter-example. RequiredClaimTest pins the boundary in both directions: what the schema calls required must be rejected when omitted, what it calls optional must be accepted. Commands with alternative shapes are excluded and asserted, since dropping a variant's distinguishing argument leaves a shape a sibling variant legitimately accepts. `inputText.text` and `evalScript.script` are pinned as known parser bugs -- their delegating creators read the field with getOrDefault(name, ""), so omitting it yields an empty string instead of an error. Tightening that is a parsing behaviour change and is left out of this change; the exception fails the day it is fixed rather than lingering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012naxaR7AjmbDCmckC2Y4ru
The workflow publishes the artifact-manifest schema, which is a separate concern from the flow-command schema this PR introduces. Deleting it here left the worst of three states: the bucket object and SCHEMA_URL stay, every manifest keeps stamping $schema, and BundleUploader deliberately preserves that field into customer-downloadable bundles -- but nothing would keep the published copy current. The first edit to v1.schema.json would hand customers a URL describing a format that no longer exists, with no test to catch it. Retiring or replacing artifact-manifest publishing is its own decision and belongs in its own change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012naxaR7AjmbDCmckC2Y4ru
… validated against
pressKey.key and setOrientation.orientation are typed String -- they have to be,
because both accept ${VAR} -- but the parser only accepts a fixed set of words,
via KeyCode.getByName and DeviceOrientation.getByName. The schema could not see
that, so it reported both as free strings and every consumer hand-copied the
lists. Studio's copy had already lost `escape`.
@YamlValues(of = ...) names the enum the parser looks the value up in, and
FlowCommandSchema reports the field -- in the map form and in the shorthand --
as ENUM with those words. pressKey now advertises 30 keys and setOrientation 4,
read off the enums rather than written down anywhere.
KeyCode's YAML spelling is its `description` ("Volume Up"), not its constant
name, and it cannot say so with @JsonProperty the way DarkModeValue and
AirplaneValue do: Jackson also serializes KeyCode as PressKeyCommand.code on the
MaestroCommand wire, where the constant name is what is written and read back.
So @YamlValues carries an optional `spelledBy` naming the property that holds the
spelling, and KeyCode is left alone.
The existing `every advertised enum value parses` test picks both commands up
automatically now that their arguments are ENUM -- 60 pressKey and 8
setOrientation values go through the real parser. A second test pins that a
String field carrying the annotation surfaces its vocabulary in both forms.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012naxaR7AjmbDCmckC2Y4ru
Every existing schema test reads its expectations off the Yaml* types as they
are today, so none could say what happens the next time one changes.
FlowCommandSchemaEvolutionTest drives the derivation with synthetic Yaml*
shapes instead. It found three bugs, fixed here:
- `@JsonProperty` on a constructor parameter was ignored, so renaming a
parameter behind its YAML spelling made the schema advertise a key the
parser rejects. `wireNameOf` now reads it.
- `@JsonAlias` spellings were never advertised: `launchApp: {url: ...}` parses
today and was absent from the schema. `ArgumentSchema.aliases` carries them.
- A stale `spelledBy` killed every `commands()` call with a bare
NoSuchElementException naming neither the enum nor the property.
`@YamlRequiresOneOf` closes the last gap the tests found: `required` is derived
from the Kotlin constructor, so runFlow, retry, extendedWaitUntil and addMedia
were published as having no required arguments while the parser rejects their
empty form. The annotation declares the rule the way `@YamlValues` declares a
vocabulary, and `CommandSchema.requiredOneOf` reports it.
The two existing test classes were reworked: two assertions that could never
fail are gone, both now parse through `parseCommand` rather than `checkSyntax`
(which never runs `toCommands`, making the enum guarantee vacuous for exactly
the `@YamlValues` fields), and the optional-argument test is one question per
command since it rendered identical YAML per argument. That surfaced two gaps
left pinned, not fixed: `openBrowser` has no `_toCommands` branch at all, and
`action` publishes its five-word vocabulary as free-form String.
Verify:
- ./gradlew :maestro-orchestra:test - 427 tests, 0 failures
- ./gradlew :maestro-orchestra-models:test - 51 tests, 0 failures
- ./gradlew :maestro-cli:compileKotlin - passes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
Two holes in the guard added by the previous commit, both found in review. `binds()` asked whether a key was bound by handing it to `readValue` and looking at which exception came back. On a type with a required non-null constructor parameter Jackson fails at the creator step and never reports the unknown key, so the probe answered "bound" for anything: 26 of 54 shapes, including inputText, openLink, pressKey, setDarkMode and swipe. The direction that exists to catch a renamed argument could not fail for half the surface. It now asks the resolved BeanDeserializerBase, which is what does the binding. That also resolves `@JsonAlias` through a delegating creator, so the introspection floor and its caveat are gone, and the four commands with a hand-written deserializer are asserted rather than silently skipped. `wireNameOf` and the alias read used `parameter.findAnnotation`, which only sees Kotlin's `param` target. `@field:` and `@get:` are legal and Jackson honours them, so a rename written that way left the schema advertising the old name. `annotationOf` now looks at the parameter, the property, the getter and the backing field. Kotlin already warns that the default target is changing, which would make this the common case rather than the unusual one. Both are pinned by mutation: renaming inputText's key behind @JsonProperty with wireNameOf reverted now fails in both directions, and reverting annotationOf to parameter-only fails the new use-site-target test. Each passed before. Verify: - ./gradlew :maestro-orchestra:test - 428 tests, 0 failures - ./gradlew :maestro-orchestra-models:test - 51 tests, 0 failures - ./gradlew :maestro-cli:compileKotlin - passes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
…en it is exclusive
@YamlRequiresOneOf is the one claim in the schema that is written by hand rather
than derived, so unlike everything else it can simply be wrong — and nothing
checked it. A typo published a rule naming an argument that does not exist, and
removing the annotation from runFlow or addMedia left the suite green.
- The rule now carries `exclusive`. runFlow and retry reject `file` and
`commands` together; extendedWaitUntil accepts `visible` and `notVisible`
together. One annotation was describing both, so a consumer generating from
the schema could emit a pair the parser refuses.
- `requiredOneOf` becomes an `OneOfSchema` rather than a bare list, so the
command carries one rule with its mode instead of two parallel fields.
- A new test asserts every part of the claim against the parser: the names
exist, none of them is also `required`, the empty form really is rejected,
each member alone really is enough, and `exclusive` really matches.
- `render`'s shorthand fallback was firing for any command with no *kept*
arguments, so `launchApp: {}` and `addMedia: {}` were never written — the
single-value form was tested in their place. It is now gated on the command
having no named arguments at all, which is what the comment always claimed.
Pinned by mutation: dropping the annotation from addMedia, dropping runFlow's
exclusivity, marking extendedWaitUntil exclusive, and misspelling an argument
name each fail with a message naming the command. All four passed before.
Verify:
- ./gradlew :maestro-orchestra:test - 429 tests, 0 failures
- ./gradlew :maestro-orchestra-models:test - 51 tests, 0 failures
- ./gradlew :maestro-cli:compileKotlin - passes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
`@JsonProperty("enabled")` on DarkModeValue / AirplaneValue moved the YAML word
onto the enum so the schema could derive it. But that enum is on two wires, and
only one of them wanted changing:
YAML input enabled -> enabled (already right)
MaestroCommand JSON Enable -> enabled (not intended)
The second is what `run_step.maestro_command_meta` holds and what the worker and
backend exchange in `CommandResult.command`. The old spelling stopped being read
at all, so historic rows and in-flight payloads would fail to deserialize, and a
rolling deploy would have the two sides disagree.
KeyCode already solves this in this same branch: hold the YAML word in a plain
property and point `@YamlValues` at it. A constructor property does not change
how Jackson writes an enum, so the wire keeps the constant names. The two enums
now do the same, and the deserializers look the word up on the enum rather than
letting Jackson convert it -- still derived, so the parser and schema still
cannot disagree.
Both real fixes from the original change survive: `optional` is still honoured in
the map form, and near-miss values are still rejected. Reading the value node as
text also restores the guidance message for `setDarkMode:\n value:`, which had
degraded to "Failed to parse content".
YamlSetModeTest now pins the wire format directly, because nothing else looks at
it and every YAML test passes either way -- reintroducing `@JsonProperty` fails
it.
Verify:
- ./gradlew :maestro-orchestra:test - 430 tests, 0 failures
- ./gradlew :maestro-orchestra-models:test - 51 tests, 0 failures
- ./gradlew :maestro-test:test - 213 tests, 0 failures
- ./gradlew :maestro-cli:compileKotlin - passes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
…s, drop a dead command
`openBrowser` was declared on YamlFluentCommand but `_toCommands` has no branch
for it, so nothing it accepted ever parsed — while the schema published it as a
real command and the KDoc used it as the worked example of a scalar one. Nothing
else in the repo refers to it; AndroidDriver's `openBrowser` is a private helper
behind `openLink`.
Selector commands published `{"selector": true, "arguments": []}`. tapOn,
assertVisible, copyTextFrom and their siblings take a YamlElementSelector rather
than named arguments of their own, so Maestro's most-used commands were the ones
the schema said least about. `selectorArguments` derives the 27 of them once,
beside `commonArguments`, rather than repeating them under every command.
Writing that test turned up `selector.traits`: a String holding space-separated
ElementTrait names, published as free-form text, so anything generated from the
schema wrote a value the parser rejects. It is the third `@YamlValues` field.
Verify:
- ./gradlew :maestro-orchestra:test - 0 failures
- ./gradlew :maestro-cli:compileKotlin - passes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
`asJson()` had no version, so a consumer caching or diffing it could not tell a change in the document's shape from Maestro gaining a command — and this branch has already added three fields to it. Two orderings were unspecified. `variants` comes from `sealedSubclasses`, which the language does not order, and the enum fallback used `Class.getFields()`, whose order the JVM does not fix — while the `spelledBy` path next to it used `enumConstants`, which is declaration order. A compiler or JDK upgrade would have silently reshuffled every published document. Variants are now sorted by name and both enum paths use the same primitive. Verify: - ./gradlew :maestro-orchestra:test - 0 failures Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
…two of them
`commonArguments` is the one hand-maintained claim in an object whose KDoc says
nothing is: `argumentsOf` strips any parameter named `label` or `optional` and
the schema then asserts every command takes both. The parser has
FAIL_ON_UNKNOWN_PROPERTIES on, so a command declaring only one of them would
have the schema advertising a key the parser rejects. Now asserted.
Restores the guard that `commands()` cannot silently drop a command. It was
deleted last week as a tautology, which was half right: it duplicated the name
list, but it was also the only thing covering the `as? KClass` filter, and every
other test rebuilds that same filter.
Two claims were wrong rather than unchecked. The `@YamlValues` KDoc says
`pressKey` needs to stay a String because it accepts `${VAR}`; it does not —
`KeyCode.getByName` runs before substitution, so only `setOrientation` does.
And sealing YamlSwipe made the `else` branch of its `when` unreachable, which
the compiler does not warn about for a statement `when`.
Also: the alias test now asserts `url` lands in `appId` rather than merely
parsing, and the broad `assertThrows<Exception>` calls are narrowed —
to MismatchedInputException, not UnrecognizedPropertyException, because the
throw is MissingKotlinParameterException. That masking is the same one the
Jackson-binding check was rewritten to avoid, and the test says so.
Verify:
- ./gradlew :maestro-orchestra:test - 0 failures
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
`action` accepts exactly five words — back, hideKeyboard, scroll, clearKeychain, pasteText — held as literals in a `when`, and the schema published it as free-form String. Anything generating a flow from the schema wrote a value the parser rejects with "Unknown navigation target". The words move onto YamlNavigationAction and the parser looks them up there, so they cannot drift. Reaching them needed one change: `action` is a plain String on YamlFluentCommand itself, so `schemaOf` now consults the declaring parameter and not only its type, which is where `@YamlValues` had to go. With this and openBrowser gone, every command the schema declares can be written from the schema — RequiredClaimTest's exception list is empty and deleted. Verify: - ./gradlew :maestro-orchestra:test - 434 tests, 0 failures - ./gradlew :maestro-cli:compileKotlin - passes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
…omments e44a632 moved the YAML words off `@JsonProperty` and onto `yamlValue`, to keep the constant names as the MaestroCommand wire format, but left three comments saying the vocabulary lives on `@JsonProperty`. That points a reader at an annotation which is not there, and invites putting one back -- which is the change that commit exists to undo. The corrected text says why it is not a `@JsonProperty`, not only where the words are. Comments only. Verify: - ./gradlew :maestro-orchestra:test - 0 failures - ./gradlew :maestro-orchestra-models:test - 0 failures Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
`VariantSchema.name` came from `subclass.simpleName`, so the schema published `YamlCoordinateSwipe` and `YamlSwipeElement` -- Kotlin class names, in a document a consumer reads and generates from. `@YamlVariant` declares the YAML-facing name instead, the way `@YamlValues` and `@YamlRequiresOneOf` declare the other things reflection cannot see: byDirection direction byCoordinates start, end byRelativeCoordinates start, end byElement direction, from Nothing consumes the schema yet, so these strings are free to choose now and are not once something does. A test fails if any variant ships under a class name; removing one annotation gives `[swipe.YamlSwipeElement]`. `variantNameOf` is internal so the binding test correlates a subclass with its variant through the same mapping rather than a second copy of the rule. `byCoordinates` and `byRelativeCoordinates` still carry identical arguments -- they differ only by the `%` in the value, which the schema has no way to say. Describing a string's *shape* is a concept it lacks in four places (these two, `extendedWaitUntil.timeout`, `retry.maxRetries`, and the space separator in `selector.traits`), so it is worth designing once rather than for swipe alone. Merging the two variants falls out for free once it exists. Verify: - ./gradlew :maestro-orchestra:test - 435 tests, 0 failures - ./gradlew :maestro-orchestra-models:test - 51 tests, 0 failures - ./gradlew :maestro-cli:compileKotlin - passes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
Every other test here checks the schema against the parser. This checks it against flows people wrote by hand, for real apps, without thinking about the schema -- 119 files under e2e/, which nothing parses outside the E2E job that needs devices and takes half an hour. It reads the YAML keys rather than parsing to MaestroCommand, because parsing discards the argument names and those are exactly what the schema claims to describe. 761 command instances, 43 distinct commands, 80 distinct command/argument pairs. Nothing fails today: the schema already covers everything e2e writes. It is a regression guard, not a finding. Making `selectorArguments` return an empty list fails it with real usages -- assertNotVisible.above, .below, .containsChild and the rest -- so the selector path is genuinely exercised rather than the test passing on an empty set. Worth noting for whoever owns the E2E suite: thirteen commands the schema declares are never written by any e2e flow -- action, assertDarkMode, assertLightMode, assertNoDefectsWithAI, assertWithAI, clearKeychain, extractTextWithAI, hideKeyboard, setClipboard, setDarkMode, startRecording, stopRecording, toggleDarkMode. That is a gap in e2e, not in the schema, and is left as an observation rather than an assertion so adding a flow cannot fail it. Verify: - ./gradlew :maestro-orchestra:test - 437 tests, 0 failures Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof
pedro18x
left a comment
There was a problem hiding this comment.
Went through this against the parser at the head commit and it lines up for me: the command count, the selector fields, the dropped optional on the set-mode map form, and openBrowser having no branch in the parser. The tests that check the schema against the real parser in both directions (RequiredClaimTest, FlowCommandSchemaTest) are what make this easy to trust. Nice work.
Small things, none blocking:
-
I think the "required" rule in
FlowCommandSchemamight be a bit off for primitives with no default. As far as I can tell Jackson fills those in with false or zero instead of failing. Doesn't seem to affect anything published today, so maybe just a note on the rule. -
setOrientationvalues come out asLANDSCAPE_LEFTwhere the docs uselandscapeLeft.DeviceOrientationalready has a camel-case name property, so pointingspelledByat that might get the documented spelling for free. Same idea as your traits note. -
Minor: the
TempFileHandlerin the two tests never gets closed, and the e2e conformance test passes quietly if it can't find the e2e folder.
FlowCommandSchemadescribes the Maestro flow-command surface — command names, arguments and their kinds, enum vocabularies,swipe's alternative shapes, and which commands may be written bare (- back). It is derived by reflection from the very types the parser uses (YamlFluentCommand's constructor, eachYaml*data class, thestringCommandsmap), so it cannot drift from what the parser accepts.It is an API, not a file. Any JVM consumer already depending on
dev.mobile:maestro-orchestracallsFlowCommandSchema.commands()(orasJson()). No URL to fetch, no path to hardcode — the surface ships in the jar and moves with the dependency version. Its first consumer is mobile-dev-inc/copilot#2776, which generates Studio's autocomplete index from it.What the schema publishes
commonArguments(label,optional) andselectorArguments— the 27 fields atapOn-style selector accepts, rendered once rather than under every selector command.@YamlValuesdeclares a vocabulary aString-typed field is validated against, where reflection cannot see it. Six sites:pressKey.key,setOrientation.orientation,selector.traits,action, andsetDarkMode/setAirplaneMode's value. The words are read off the enum, never written down twice.@YamlRequiresOneOfdeclares a rule enforced insidetoCommands:runFlowandretryneedfileorcommandsand refuse both (exclusive),extendedWaitUntilneedsvisibleornotVisibleand accepts both,addMedianeedsfiles. Without it the schema publishedrunFlow: {}as valid.@YamlVariantnamesswipe's alternative shapes in YAML terms —byDirection,byCoordinates,byRelativeCoordinates,byElement— instead of leakingYamlSwipeElementinto a published document.asJson()carries aversionfor the document's shape, and orders variants and enum values deterministically (sealedSubclassesandClass.getFields()have no guaranteed order).Behaviour changes — please focus here
setDarkMode/setAirplaneModehonouroptional. It was silently dropped: the deserializers built the value with a two-argument constructor call. A flow that saidoptional: trueon either command was failing the run instead of being skipped.contains("enabled"), sosetDarkMode: "is enabled now"parsed as enabled. Case sensitivity and${VAR}behaviour are unchanged — neither worked before or after.openBrowseris deleted. It was declared onYamlFluentCommandwith no branch in_toCommands, so every form of it was rejected by the parser. Nothing else in the repo referred to it.The YAML words for
DarkModeValue/AirplaneValuedeliberately live in ayamlValueproperty rather than a@JsonPropertyon each constant: Jackson also serializes those enums asSetDarkModeCommand.valueon theMaestroCommandwire, where the constant name is what is persisted and sent between worker and backend.YamlSetModeTestpins that wire directly, because every YAML test passes either way.KeyCodehas the same constraint and the same shape.Tests
FlowCommandSchemaEvolutionTestdrives the derivation with syntheticYaml*shapes — a command being added, an argument renamed or retyped — which is the only way to ask what happens the next time someone changes one. It found three bugs, fixed here:@JsonPropertyrenames were ignored,@JsonAliasspellings were never advertised (launchApp: {url: …}parses and was absent), and a stalespelledBykilled everycommands()call with a nameless exception.RequiredClaimTestpins the boundary against the real parser in both directions, and every one-of rule is checked against it: the names exist, none is alsorequired, the empty form really is rejected, each member alone really is enough, andexclusivereally matches. Dropping any of the four annotations fails it.Known gap, pinned not hidden:
inputText.textandevalScript.scriptare reported required, but their delegating creators read the field withgetOrDefault(name, ""), so omitting it yields an empty string instead of an error. The schema is right, the parser is wrong; tightening it is a parsing behaviour change left for its own PR. The test fails the day it is fixed.Verify:
./gradlew :maestro-orchestra:test— 435 tests, 0 failures./gradlew :maestro-orchestra-models:test— 51 tests, 0 failures./gradlew :maestro-test:test— 213 tests, 0 failures./gradlew :maestro-cli:compileKotlin— passes, so theinternal/sealedchanges broke no callers🤖 Generated with Claude Code
https://claude.ai/code/session_012naxaR7AjmbDCmckC2Y4ru
https://claude.ai/code/session_015vHfmsiT7skZdV13zcbeof