fix(gestures): fail pre-removal gesture forms at .ad parse time (#1216)#1393
fix(gestures): fail pre-removal gesture forms at .ad parse time (#1216)#1393thymikee wants to merge 3 commits into
Conversation
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact head P1 — the Node P2 — variable-backed stale P2 — correct the release version. The migration page says 0.21 removed these forms, but removal commit Literal stale forms do fail before dispatch; ADR alignment, explicit Maestro 100ms export, green CI, and reported iOS/Android fixture runs otherwise look sound. |
|
All three addressed in P1 — Node The SDK regression in P2 — variable-backed durations missed the migration. Fixed. The numeric-only guard existed to stop a stray A stray flag or word still gets plain usage text — both are covered by tests. P2 — release version. You're right, and my source was wrong: Also updated: the guide's Node section now states the rejection is client-side (so a JS caller gets the error, not a retimed gesture), and the Re-verified: full unit suite, 🤖 Addressed by Claude Code |
|
Re-reviewed exact head P2 — the saved-script sweep does not find every retired form. The documented grep matches only five-argument P2 — MCP does not return the CLI’s concrete replacement command. The guide promises the same contextual rewrite, but structured readers return generic messages such as Current checks are green and the reported iOS/Android fixture evidence is present. |
#1315 removed the timed forms of `swipe`, `gesture fling`, and `gesture swipe` and `gesture rotate`'s `velocity`, but shipped without the migration guide, the repository sweep, or the parse-time error that issue #1216's own checklist gates a removal on. The sweep finds what that left behind: both `06-swipe-gestures.ad` integration fixtures still carried the 5-argument swipe and would fail at replay, two tests still asserted the removed shapes, and two branches still read the retired positional. Argument arity for every public gesture syntax now lives in one table keyed off the canonical `GESTURE_KINDS`, so a new kind cannot skip it and a form removed from the CLI is removed from `.ad` in the same edit. Both callers read it: the CLI argv parse, and a new `.ad` preflight. A stale script now fails when it is parsed — before the replay executes any device action — naming the line and computing its rewrite, instead of running up to that step and failing as a repairable divergence. The preflight checks arity only: `${VAR}` tokens resolve after planning, and interpolation never splits a token, so the count is decidable while the values are not. Deleting the dead duration read in `readSwipeGeometry` would have left `replay export` emitting no duration, handing Maestro's 400ms default to a gesture the script runs at 100ms, so the export now states `duration: 100`. `.ad` positional gesture parsing is NOT removed. Its only remaining callers are the CLI argv parse and the `.ad` line parse, both the current public syntax rather than a bridge to an older one, so there is nothing to migrate off. ADR 0013 records that and drops the "compatibility" framing that made it read as debt. Both migrated fixtures verified on real devices with the repo's own CLI: iOS simulator 34.9s, Android emulator 45.9s.
Review findings on d88c6ed. P1: `interactionDaemonWriters.swipe` hand-projects five fields, so a JavaScript caller's `durationMs` was dropped before the daemon's `readSwipeInput` could reject it and a default-duration fling ran instead — the exact silent reinterpretation the guide promises does not happen. `gesture` was already safe because its writer runs `readGestureInput` -> `readGesturePayload`, which rejects the removed keys; `swipe` was the one surface with no reader of its own. The rejection now lives in contracts and is shared by the client writer and the daemon handler, so there is one rule and one message. The SDK regression covers all four removed keys and asserts the transport is never reached; reverting the writer call fails it on `swipe durationMs`. P2: the preflight's retired-slot test required a numeric token, so `swipe 197 650 197 300 ${DURATION}` fell back to bare usage text. An unresolved `${VAR}` now counts as the retired slot and is carried into the pan rewrite, while a stray flag or word stays a plain usage error. P2: the removal shipped in 0.20.0, not 0.21 — removal commit 6d99914 is contained in tag v0.20.0. The guide said 0.21 because the CHANGELOG still files it under `Unreleased`; the tag is the truth (headings lag several releases repo-wide, so that is pre-existing and left alone). The `.ad` grep recipe now matches variable-backed durations too.
Re-review findings on 328bad8 (both migration-guide accuracy). The saved-script sweep matched only the five-argument `swipe` form; it missed timed `gesture fling`, timed `gesture swipe`, and `gesture rotate ... velocity`, so the repository-cleanliness step was incomplete. Provide one grep per retired form (number-or-`${VAR}` token), each verified to flag the removed shape and skip the valid one. The MCP section claimed the structured rejection carries the CLI's concrete replacement command. It does not: `readGesturePayload` and `assertNoRemovedSwipeInput` return a message that names the removed key and the replacement command (e.g. `gesture fling does not accept durationMs; use gesture pan for timed movement`) but not the fully-substituted coordinate rewrite, because the structured request carries no positional string to rewrite. Describe what the structured path actually returns.
328bad8 to
4fd5d39
Compare
|
Both re-review findings addressed in P2 — the saved-script sweep missed three of the four retired forms. Correct — it only matched five-argument # swipe x1 y1 x2 y2 durationMs
grep -rnE '\bswipe( +([-0-9.]+|\$\{[^}]*\})){5}' --include='*.ad' .
# gesture fling <direction> x y distance durationMs
grep -rnE '\bgesture +fling +[a-z]+( +([-0-9.]+|\$\{[^}]*\})){4}' --include='*.ad' .
# gesture swipe <preset> durationMs
grep -rnE '\bgesture +swipe +[a-z-]+ +([-0-9.]+|\$\{[^}]*\})' --include='*.ad' .
# gesture rotate degrees x y velocity
grep -rnE '\bgesture +rotate( +([-0-9.]+|\$\{[^}]*\})){4}' --include='*.ad' .P2 — MCP does not return the CLI's concrete rewrite. Correct, and the honest fix is to describe what the structured path actually returns rather than implement a rewrite it can't meaningfully produce. Rebase. Onto Re-verified post-rebase: 🤖 Addressed by Claude Code |
|
Review at |
1 participant
Closes #1216.
Why
#1315 removed the timed forms of
swipe,gesture fling, andgesture swipe, plusgesture rotate'svelocity— but shipped without the migration guide, the repository sweep, or the parse-time error that #1216's own "before removal" checklist gates a removal on.The sweep this PR performs finds what that left behind:
06-swipe-gestures.adintegration fixtures still carried the 5-argumentswipeand would fail at replay today;export-flow.test.tsandscript.test.tsstill asserted the removed shapes;readSwipeGeometry(Maestro export) and a swipe-duration fallback inrecording-gestures.tsstill read the retired positional from a slot that can no longer be populated.What changed
One arity table. Positional arity for every public gesture syntax now lives in
PUBLIC_GESTURE_SYNTAXinsrc/contracts/gesture-normalization.ts, keyed off the canonicalGESTURE_KINDSvia a template-literal type — a new gesture kind cannot skip it, and a form removed from the CLI is removed from.adin the same edit. Both callers read it: the CLI argv parse and a new.adpreflight.Stale scripts fail at parse, not at step N. A
.adscript carrying a removed positional now fails when the script is parsed — before the replay executes any device action — naming the line and computing the rewrite:Previously the script ran up to that step and then failed as a repairable
REPLAY_DIVERGENCE, which is wrong: it is a script syntax error, not a UI divergence. #1216 allows "an upgrader or an actionable parse error"; this is the latter, and the message already contains the exact replacement line.The preflight checks arity only —
${VAR}tokens resolve after planning and interpolation never splits a token, so the count is decidable at parse time while the values are not. A single extra numeric argument is reported as the migration; anything else stays a plain usage error, so an unknown flag is not misreported as a retired duration.Migration guide published.
website/docs/docs/migrating-gestures.mdcovers CLI, Node.js, MCP, and saved.adrecordings (plus why Maestro flows need no migration), and documents the five-step deprecation policy the next such removal has to clear — announce, warn for one minor, publish the migration, prove the repository is clean, then remove the branch and its tests together.Reviewer notes
One judgment call. Deleting the dead duration read in
readSwipeGeometrywould have leftreplay exportemitting no duration at all, handing Maestro's own 400ms default to a gesture the.adscript runs at 100ms. The export now statesduration: 100(the canonical fling duration) explicitly. Maestro flows replayed by agent-device are untouched — a timed Maestroswipestill normalizes togesture panwith theendpoint-holdprofile..adpositional parsing is deliberately NOT removed. #1216 listed it as the second shim, conditional on "recordings and migration tooling having a structured replacement". Its only remaining callers are the CLI argv parse and the.adline parse — both the current public syntax, not a bridge to an older one — so there is nothing to migrate off, and a structured payload would make recordings unreadable and ungreppable for no behavioral gain. ADR 0013 already decided to keep it; this PR makes that explicit and drops the "compatibility" framing that made it read as debt. If you would rather keep that half of the issue open, the guide'sPositional .ad syntaxsection is the piece to revert.Fixture migration. Both
06-swipe-gestures.adfiles drop the trailing duration and keepswipe(rather than converting togesture pan), preserving each suite's coverage of theswipecommand. That does change the motion — a 300ms drag becomes a 100ms fling, which travels further on a scrollable list — so it was verified rather than assumed.Verification
ad test <fixture> --platform ... --state-dir ...).lint,typecheck,format:check,check:layering,check:production-exports,check:fallow,maestro:conformance,test:smoke— all clean.🤖 Generated with Claude Code