read plan model output frame

[Devin T. Currie (DTCurrie)]

Fixes which frame a plan model hands its children to, so a camera or gripper mounted on an arm attaches to the model's real output frame rather than to whichever link the model happened to declare last. Base of the stack, so it sits on main.

Stack

1. This PR: Read a plan model's output frame from where RDK writes it
2. Decode STL collision meshes and rotate unoriented link geometry (geometry decode fixes #912)
3. Drive mimic joints from the joint they mimic (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's shape from the dimensions it sets (infer collisions #919)
7. Read mesh data delivered as a number array (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. Reconstruct RDK's flattened frame system from a robot's config (reconstructed flattened frames from rdk #923)
11. Add client-side forward kinematics for placing a plan's frames (forward kinematics for player #924)
12. Add interpolateTrajectory, budgeting preview frames per joint unit (budget frame movement between waypoints #925)
13. Group a preview ghost with the component it stands in for when checking collisions (handle preview collisions #926)
14. Add a client for the motion service's plan and execute do-commands (add do command wiring for planning and execution #927)
15. Draw a previewed plan as ghost geometry alongside the live machine (add preview ghosts #928)
16. Add the request lifecycle for a previewed move (preview lifecycle #929)
17. Add move preview to the MoveFrame plugin (Motion plan preview #908)
18. Fill in the frames between a previewed plan's waypoints (interpolation #930)

Frontend

• modelOutputFrame (new) resolves a model's terminal frame through three branches: primary_output_frame on the model envelope, then the model config's output_frames[0], then soleLeafOf.
• soleLeafOf (new) computes the model's one childless frame over links and joints, returning undefined when there is more than one. A links or joints that is not an array is treated as empty rather than spread, so a malformed capture degrades to whatever the other list resolves instead of throwing a bare TypeError that would take the whole plan render down.
• buildFrameContexts now calls modelOutputFrame. It previously read model?.primary_output_frame and fell back to links.at(-1)?.id.

Why?

Why did the old read never find anything?

RDK serializes primary_output_frame on the SimpleModel envelope, as a sibling of model. ModelConfigJSON has no such field; its analog is output_frames. So model?.primary_output_frame was always undefined and every model fell through to the array-position fallback. All four captured plans agree on the placement: across their 29 model frames it is on the envelope 29 times and inside model zero.

Why is the last declared link not good enough?

UnmarshalModelXML collects links into a map[string]*LinkConfig and then ranges that map into a slice. Go randomizes map iteration per range statement, so two parses in one process can disagree, not merely two robot restarts. joints is appended in XML document order and is unaffected, which is exactly why the link array was the wrong thing to index. The consequence is a gripper rendered hanging off a mid-arm link, with its collision volume sweeping space the plan never occupies.

Why is the ladder in this order, when RDK resolves the other way round?

SimpleModel.UnmarshalJSON ignores the envelope's primary_output_frame whenever model is present and recomputes from the config, so RDK's own precedence is output_frames[0], then leaves[0], then the envelope. This reads the envelope first instead. The two cannot disagree on anything RDK marshalled, because the envelope is written from the value the config produced, and reading it first means trusting what RDK resolved rather than re-deriving it. Only a hand-edited dump can tell the two orders apart.

Why does the last resort refuse rather than pick?

Because that is RDK's rule, and the refusal is the whole of it. primaryOutput = leaves[0] is reachable only when requireSingleLeaf holds, and requireSingleLeaf is len(cfg.OutputFrames) == 0. leaves is built by ranging a Go map and is therefore unordered, so index 0 means something only because the length is pinned to one. A model with several leaves is legal as long as it names its output, which is the rung above this one, so by the time the leaf rule runs, single-leaf is the only rule RDK would apply.

Why does nothing render differently on the captures in this repo?

Because the old code resolved to links.at(-1), and on all 29 captured model frames that is the same frame the declared primary_output_frame names. None of the four captures declares output_frames, and none has a model whose sole leaf is a joint, so the second and third branches are exercised only by tests. The fix is for models this repo has no capture of: URDF-derived arms, where link order is randomized, and models whose output frame is a joint, which links.at(-1) could never name. Reviewers should not expect a visual difference on any dump here.

Why does soleLeafOf read only links and joints?

A kinematic_param_type: "DH" model carries its topology in dhParams, from which RDK synthesizes <id> and <id>_j nodes and computes leaves over those. soleLeafOf sees no nodes for such a model and returns undefined, so the branches above it answer instead. No capture uses DH, so implementing that topology has no fixture to test against.

What happens with a dump captured from an older RDK?

The leaf rule answers, which is the same contract that version's sortTransforms used. primary_output_frame was added to the envelope between RDK v0.110.0 and v0.115.0, and for anything older the field is simply absent. There is no RDK or api version floor and no proto change.

Why is a model with neither model nor internal_fs left unguarded?

SimpleModel.UnmarshalJSON's last branch sets primary_output_frame to fs.World().Name(), so such a model serializes "world" and the first branch would take it, parenting every child to <model>:world, a frame that does not exist. All 29 captured frames resolve to a real frame, so this is latent. Guarding it means checking the resolved id against frames, which is a change to buildFrameContexts rather than to the ladder, and this is the base of an 18 PR stack.

Testing

npx vitest run passes 638 tests across 66 files, up 7 from main's 631. pnpm check reports 0 errors and 0 warnings. pnpm lint:eslint is clean.

Each branch is pinned by deleting it and naming the test that catches it:

deleted	now fails
branch 1, primary_output_frame	reads primary_output_frame off the model envelope, not out of \model``
branch 2, output_frames[0]	falls back to the model config\s own output_frames`
branch 3, soleLeafOf	falls back to the model\s sole childless frame rather than its last link`, plus 2 more
the refusal, so soleLeafOf returns leaves[0]	declines to pick when a model has more than one childless frame
the Array.isArray guard on output_frames	ignores an output_frames that is not an array
the Array.isArray guard on links/joints	does not throw when a model\s links is not an array`

The captured plans test is an equivalence check rather than a regression gate: on all 29 captured model frames the declared value, the sole leaf and the last declared link are the same string, so it catches a ladder that stops resolving a terminal at all, not a ladder that resolves one differently.

[changeset-bot]

🦋 Changeset detected

Latest commit: 8f1130c

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
Preview removed because the pull request was closed.
2026-08-13 17:55 UTC

[viam-overwatch]

Hey Devin T. Currie (@DTCurrie) — CI is green and no reviewer is assigned yet. Could you request one when you have a chance?

Auto-comment from overwatch. Will not re-nudge for 7 days.

[Devin T. Currie (DTCurrie)]

Claude (@claude) review

[claude]

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

---

Review

• Read CLAUDE.md and gather context
• Read modified files in full
• Run git diff origin/main...HEAD
• Analyze correctness, edge cases, tests
• Post review

---

Correctness

The three-branch ladder in modelOutputFrame is correct and the precedence matches the reasoning in the PR description. Key checks:

Branch 1 — envelope primary_output_frame

const declared = (entry.frame as Record<string, unknown>).primary_output_frame
if (typeof declared === 'string' && declared !== '') return declared

Reads from the model envelope (the sibling of model), not from model itself — this is the whole fix. The !== '' guard is correct because Go marshals primary_output_frame without omitempty.

Branch 2 — output_frames[0]

const frames = model?.output_frames
const configured = Array.isArray(frames) ? (frames[0] as unknown) : undefined
if (typeof configured === 'string' && configured !== '') return configured

The Array.isArray before indexing is important and well-commented: output_frames: 'gripper_mount' (a bare string) would otherwise index to 'g', and the typeof check below would accept it.

Branch 3 — soleLeafOf

The claimed-set approach is correct. base and other synthetic roots appear in claimed via node.parent flatMap but have no matching {id: 'base'} node, so they're never candidates for leaves. DH models have no links/joints arrays, so nodes is empty and undefined is returned, falling through to the caller's childMap heuristic — documented and acceptable.

The Array.isArray guard in soleLeafOf is correct: {} is iterable in a spread ([...{}] does not throw, but spreading a non-iterable like {} with ... in an array literal does throw in some engines). The comment accurately calls out the failure mode.

Caller truthiness check

const endEffectorId = modelOutputFrame(entry, model)
if (endEffectorId) {

soleLeafOf can return '' if a node carries id: '' (Go marshals without omitempty). The comment explains why truthiness rather than a null check is used here. Branches 1 and 2 already reject '' explicitly; branch 3 delegates to the caller. Slight asymmetry but the comment documents it.

---

Tests

Strong coverage across all branches and guards:

• All six deletion tests in the PR description table are reflected in the spec.
• The expect(d.kind).toBe('static') additions before discriminated-union if blocks are a good improvement — tests now fail clearly rather than silently skipping inner assertions when the kind is wrong.
• Removal of afterEach(() => warn.mockClear()) is correct — clearMocks: true in vite.config.ts resets mocks between tests.
• The checked count hardcode (14) is documented as an equivalence check, not a regression gate — acceptable.

One nit (see inline): the armed helper's JSDoc comment uses a "so" clause that reads backwards — it describes what was avoided, not what was done. Suggestion attached.

---

Summary

The fix is correct and the reasoning is airtight — model?.primary_output_frame was always undefined because the field lives on the envelope, not inside model. The three-branch ladder correctly mirrors RDK's precedence (envelope first, which is always what RDK wrote). Covers URDF arms whose link order is randomized and models whose output frame is a joint, neither of which links.at(-1) could handle. One comment clarity nit, otherwise ready to merge.

[claude]

The "so" clause reads as if listing gripper_mount first is what was done, but it's actually what was avoided. The sentence is logically inverted — consider rephrasing:

Suggested change

	* last joint, so listing `gripper_mount` first makes that fallback match what each branch asserts.
	*/
	* `extra_link` first on purpose: the childMap fallback returns the first child of the last joint,
	* so if `gripper_mount` were first, a deleted branch would silently resolve to it and mask the failure.