add do command wiring for planning and execution

[Devin T. Currie (DTCurrie)]

Adds the two DoCommand verbs the builtin motion service exposes but the motion proto does not: plan, which plans a move without executing it, and execute, which runs a trajectory verbatim. Stacks on #926.

Neither has a generated client, because neither is in the proto, and only services/motion/builtin implements them, so any other motion service errors. Nothing calls this yet; the preview lifecycle two PRs up is the consumer. It lands on its own because it is pure command construction and reply parsing, with no ECS, no Svelte and no I/O, so its spec can cover the whole surface directly.

Stack

1. Read a plan model's output frame from where RDK writes it (read plan model output frame #910)
2. Geometry decode fixes (geometry decode fixes #912)
3. Mimic joint fixes (mimic joint fixes #913)
4. Move the shared plan kinematics into $lib/motion (make motion utils reusable #917)
5. Drive plan joints by RDK's schema order (match rdk joint numbering #918)
6. Infer an untyped geometry from its dimensions (infer collisions #919)
7. Read mesh data in both the shapes RDK sends it (match rdk mesh data decoding #920)
8. Keep a plan's snapshots with the plan when another is removed (replayer plan snapshot cleanup #921)
9. Share trajectory playback between the replayer and the move panel (make trajectory playback reusable #922)
10. Draw a part's configured geometry even when it has a kinematic model (reconstructed flattened frames from rdk #923)
11. Place a plan's frames by running its kinematics on the client (forward kinematics for player #924)
12. Budget preview frames per joint unit (budget frame movement between waypoints #925)
13. Report a previewed collision as a warning about the move (handle preview collisions #926)
14. This PR: Ask RDK to check the start state before executing a previewed plan
15. Draw a previewed plan as ghost geometry (add preview ghosts #928)
16. Run a previewed plan's lifecycle (preview lifecycle #929)
17. Add move preview to the MoveFrame plugin (Motion plan preview #908)
18. Fill in the frames between planned waypoints (interpolation #930)

Frontend

• planCommand builds the plan payload: a protojson motion.v1.MoveRequest as a string, since that is what RDK unmarshals. It refuses a goal that is not a finite pose, and passes worldState and constraints through so a preview plans the same problem the subsequent move would.
• executeCommand builds the execute payload and arms executeCheckStart.
• parsePlanResult reads the reply. Each way it can fail gets its own message rather than one shared diagnosis: an unexpected response, an absent or nil trajectory, an empty one, a joint value that is not a finite number, a step naming no components, and a trajectory in the older joint-value format.
• isAlreadyAtGoal recognizes the trajectory RDK returns when the goal is already satisfied.
• PlanCommandError names every failure this module raises, so a caller can tell a bad reply apart from a transport error.

Why?

Why send executeCheckStart at all, and why zero?

Its presence is the switch (builtin.go:376). Omit it and epsilon is math.MaxFloat64, so the comparison at builtin.go:621 can never trip and the trajectory runs from wherever the components happen to be. Since execute never replans, that is exactly the case worth refusing: the plan was validated from one starting configuration, and running it from a different one flies a path nothing checked. A failed check comes back as an RPC error rather than a field in the reply, since resp[executeCheckStart] is a constant string echoed back whenever the key was sent and says only that the check was asked for.

Zero because RDK reads any value at or below it as "use the default" (defaultExecuteEpsilon, 0.01). How far an arm may have drifted before its plan is stale is a property of the arm, not something a viewer should be deciding for it.

Why is there no conversion between the reply and the replayer's kinematics?

Because there is nothing to convert. motionplan.Trajectory is []referenceframe.FrameSystemInputs, and FrameSystemInputs is map[string][]Input where Input is a float64 alias, so on the wire it is exactly the trajectory array a plan dump already carries. The kinematics added in #924 read it unchanged, which is why TrajectoryStep is re-exported from $lib/motion/jointPose here rather than redeclared.

Why rebuild worldState and constraints before sending them?

The SDK exports each of these under one name as both a class and a PlainMessage type alias, and it is the alias that lands in a type position. So what arrives at planCommand is a plain object with no toJson on it, and rebuilding is what reaches protojson. parseMoveOptions happens to return real message instances today, for which the rebuild is a no-op, so this is the one thing in the module that the obvious spec input cannot exercise. There is a test that passes the plain shape the signature actually describes.

Why refuse a non-finite goal instead of sending it?

JSON.stringify writes null for NaN, and Go's protojson skips a JSON null for a scalar field rather than rejecting it, leaving the field at its zero. A NaN in the goal would therefore reach RDK as 0 mm, plan successfully, and preview a move to a destination the user never asked for, with nothing wrong on either side. Only this path can do that: client.move sends a proto double, which carries the NaN and gets refused by the planner. Pose.isFinite() already exists and guards the same way in FrameEditor.ts.

Why does a step have to be a non-empty object of finite number arrays?

Because every says yes to the two structural degenerate cases by default. An array is typeof 'object', so [[0, 1], [2, 3]] walked as a step; and Object.values({}) is empty, so {} passed vacuously. Neither errors downstream either, because jointValueAt resolves a missing column to 0, so a reply this guard exists to reject instead drew a plausible-looking arm at the zero configuration. [[], []] was worse again: it parsed and satisfied isAlreadyAtGoal, so the panel reported the machine was already there.

Finite rather than merely numeric for a different reason. typeof NaN === 'number', and one NaN in a reply is not a local defect: the frame budget in #925 sums every segment's cost to decide how finely to sample, so a single non-finite value makes the total non-finite, which survives Math.ceil until the interior loop stops running and every segment of the plan collapses to one frame. That is the raw waypoint teleport interpolation exists to prevent, reached with nothing reporting an error and a NaN on the scrubber's coarsening readout. planCommand already refuses a non-finite goal on exactly this reasoning, so applying the same rule to the reply is consistency rather than a new position.

Whether RDK can send one is unproven, and it is cheap either way. protojson refuses to marshal a non-finite structpb.Value, but a machine connection is WebRTC binary proto, which has no such objection. This client cannot draw a NaN configuration under any reading of it.

Why does one malformed shape get a message about RDK's age, and the rest not?

There is no capability or version RPC to probe a machine with, so the shape of the reply is the only evidence available. An RDK older than ~v0.101 serializes Input as a struct, so a plan that succeeded comes back as [{"arm": [{"Value": 0.1}]}]. That one shape is worth naming, rather than reporting as "no plan", and the claim behind it is sound as an upper bound: Input is a struct in v0.90 and a float64 alias in v0.101, and the same older versions take component_name as a ResourceName, so the request would not have unmarshalled either.

It is evidence for that shape only. Every other way a reply can fail isTrajectory is not something an upgrade explains, so describeMalformedTrajectory walks the value and says what is actually wrong instead of borrowing a diagnosis that fits one cause. The user-facing message names no version at all, only "an older version of RDK", since the exact cutover is somewhere between the two and a version number in a string a user reads goes stale the moment it is wrong. The bound stays in the code comment, where it can be checked against RDK.

A Go nil trajectory marshals to JSON null, which is nothing to draw for the same reason an absent key is, so it reports "no trajectory" rather than sending someone on current RDK off to upgrade it.

Why is isAlreadyAtGoal an exact comparison, and why exactly two steps?

RDK seeds the trajectory with the start configuration (plan_manager.go:51) and appends the IK solution, so a satisfied goal comes back as two steps, never empty and never as an error. The two are bit-identical, but not because the second is a copy: it is an nlopt output, identical because nlopt runs with SetStopVal(defaultGoalThreshold) from exactly the start configuration and short-circuits at x0 when the goal is already met.

That makes this a guard which under-fires rather than one which mis-fires. A goal near enough to look identical on screen but far enough to clear the threshold gets a real two-step plan and a preview that does not visibly move. A tolerance would trade that for hiding real short moves, which is the worse mistake.

Length two is load-bearing for a different reason: a longer plan that ends where it began is a real move that goes somewhere and comes back, and hiding it would be worse than showing it.

Testing

pnpm exec vitest --run passes 1045 tests across 81 files, up 39 tests and one file from the base branch. svelte-check reports 0 errors and 0 warnings.

planDoCommand.spec.ts covers all four exported functions and uses the real parseMoveOptions and Pose rather than mocking either. Every behavior was verified by reverting the line that implements it and confirming the matching test fails, including the mutants a weaker spec survives: sameInputs' per-component loop degraded to some, the length check loosened to < 2, the trajectory reordered on the way out, and a stray key added to the payload.

Three shapes in the spec are deliberate. The payload is asserted as a whole key set, not field by field, because RDK unmarshals it with default protojson options: DiscardUnknown is false, so any key that is not a MoveRequest field is a hard RPC error, and a field-by-field spec would stay green while every real plan call failed. executeCommand's output is compared against a literal rather than against the array it was handed, since the command holds the same reference and comparing it to its own source is a tautology that any in-place reordering would survive. And each row of the malformed-reply table asserts its own message rather than just that something threw, so the messages cannot drift into sharing one that fits only a single cause.

[changeset-bot]

🦋 Changeset detected

Latest commit: 33bb831

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@viamrobotics/motion-tools	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

PR Preview Action v1.8.1
🚀 View preview at https://viamrobotics.github.io/visualization/pr-preview/pr-927/
Built to branch gh-pages at 2026-08-13 18:48 UTC. Preview will be ready when the GitHub Pages deployment is complete.

[Devin T. Currie (DTCurrie)]

Claude (@claude) review

[claude]

Claude finished Devin T. Currie (@DTCurrie)'s task in 3s —— View job

---

Review

• Read CLAUDE.md and gather context
• Read changed files
• Run lint/typecheck/tests
• Post review

---

Clean module and very thorough spec. svelte-check passes with 0 errors and 0 warnings. Two minor findings, both inline.

What's well done

• isTrajectoryStep is defensively correct. The !Array.isArray guard, the Object.keys(step).length > 0 check, and Number.isFinite rather than typeof === 'number' together close all four structural degenerate cases the PR description enumerates — and the spec confirms each one was actually tested by reversion.

• Old-shape detection precedes the general trajectory check. Giving the known-explainable failure its own message before falling into the generic describeMalformedTrajectory walk is the right ordering; a user on an old RDK needs that signal rather than "a trajectory this client cannot read."

• Object.hasOwn in sameInputs. The comment and the matching spec case (toString component name) make clear this is not defensive boilerplate but a real prototype-chain trap. The test is also correctly written to use two different component names (toString vs arm) so the step lengths differ — a weaker version using { toString: [], arm: [] } on both sides would pass even with a plain property lookup.

• executeCheckStart: 0 rationale is sound. The reasoning that its presence is the switch (not its value), that zero selects the arm-level default, and that a viewer shouldn't be picking epsilon is all explained inline and in the PR body. The test correctly asserts <= 0 rather than === 0 so RDK's "any value at or below" semantics are captured too.

• Test shapes are asserted as wholes. Testing the full MoveRequest key set (not field-by-field) is the right call given DiscardUnknown: false on the Go side; the PR description explains exactly why.

Findings

Two findings, both minor — see the inline comments for specifics:

1. PlanCommandError placement (line 170) — class is declared after its first use in planCommand. Works at runtime, hurts top-down readability.

2. Misleading "null" message for non-null non-object steps (line 139) — the null || typeof !== 'object' branch returns "null trajectory step" even for a number or string step. These shapes don't come from RDK today but the message would be wrong if one did. Suggestion block included.

Neither is a correctness issue. Both are easy one-liner fixes if you want to clean them up before merge.

[claude]

Minor – class declared after first use.

PlanCommandError is exported and referenced at line 44 (inside planCommand) and again inside parsePlanResult, but the class body isn't declared until here. This works at runtime because both usages are inside function bodies that are called only after the module finishes evaluating, so the class is initialised in time. But reading top-down, a reader hits throw new PlanCommandError(...) before ever seeing the class, which is surprising.

Consider moving the class declaration above planCommand (or at least above its first use), which is the conventional spot for exported error types.

[claude]

Nit – message says "null" for any non-object step, not just null.

if (step === null || typeof step !== 'object') {
    return 'Motion service returned a null trajectory step.'
}

If step is a number (42) or a string ("oops"), the branch fires and the user reads "null trajectory step", which is wrong. These shapes don't come from RDK in practice, but the message will be confusing if one ever does surface. A small tweak covers both cases:

Suggested change

	return 'Motion service returned a null trajectory step.'
	if (step === null) {
	return 'Motion service returned a null trajectory step.'
	}
	if (typeof step !== 'object') {
	return `Motion service returned a non-object trajectory step (got ${typeof step}).`
	}