Hand a workflow node out as an object rather than an array, so its shape has an owner

Problem/Motivation

A workflow node is handed around as a plain array. getNode() is getNodes()[$node_id] ?? NULL, so what a caller receives is whatever the definition happens to hold, and the shape of it lives nowhere the reader can reach: it is written out by hand as an array{...} PHPDoc in three places, on WorkflowDefinitionInterface::getNodes(), on ::getNode() and on the Workflow entity's own $nodes property. Those three have to agree, and nothing checks that they do.

Forty-six method signatures take an array $node. Three of them are on the plugin interfaces a module outside this project implements: TimeoutActionInterface::onTimeout(), DeadlineProviderInterface::getDeadline() and shouldRearm(), and NodeFeatureInterface::toConfig(). Somebody writing a timeout action is handed an untyped array and has to read our documentation to find out what is in it, or guess. The engine's own readers are in the same position, which is why a node key that moves has to be chased through every one of them by hand rather than through one accessor.

What the change does

WorkflowNode is a final class holding the stored node in a private readonly array, built through a fromArray() factory. It answers for the node's own structure with getId(), getLabel(), getTaskType(), getConfig(), getJoin() and getSplit(), and reaches a node feature's settings by name with getFeature(), hasFeature() and getFeatureKeys(), which is the one part that cannot be its own getter because the features are not the kernel's to enumerate.

It is a view, not a parse: the stored array is kept whole and toArray() gives it back unchanged, because a version's executable hash is taken over the definition exactly as written, so anything that reordered keys or turned an absent key into an empty one would cut a new version of every workflow the first time one was read.

Views are built per call rather than cached. The engine asks for one node at a time on the paths it walks per token, so that is one object per call; caching them on the config entity would put a derived object into what gets serialized and leave a stale view behind every save.

The shared definition contract answers with views through getNodes() and getNode(), and with ids through getNodeIds(), for a caller that walks a definition by name and reads nothing off the nodes. getRawNodes() is the seam back to storage, and it is on WorkflowInterface, the editable workflow, rather than on the contract both holders share: writing and encoding only ever happen from the editable side, so an immutable version snapshot does not offer the door at all.

The three plugin interfaces take a node now, so an implementation keeps its body and changes its signature. docs/extending.md gains a Reading a node section stating the rule: getNodes() to read, getNodeIds() to walk by name, getRawNodes() only where the result is written back or serialized, toArray() for a single node already in hand.

Bugs this fixes

Reading a node through one accessor made three existing faults reachable rather than tolerated. Each is fixed here with a test that pins it.

A run can outlive the node it stands on, because an unpinned run reads the live workflow and the workflow can be edited under it. Every reader on that path now answers for the absence instead of dereferencing it. The worst was the completion seam: OutcomeSignaler::resolve() took a node and the inbox handed it whatever the token resolved to, so completing a task whose node had been deleted was a TypeError rather than a completion, reachable by a holder pressing the button on their own open task. It now answers as it already answered for a task type that no longer resolves, and the run walks the flow out of the node.

A node named with digits crashed the definition walk. A node id is validated against ^[a-z0-9_]+$, so 123 is a name an author can type; PHP keys the map with the int, and getOutgoingFlows() takes a string under strict_types. getNodeIds() answers with strings, which is what the four sites walking a definition by name were already assuming.

createForToken() minted a task for a token whose node the definition no longer has, resolving its audience against nothing, which reads as open to everyone. It is refused outright now and the throw is documented on the interface; the park step never reaches it, because createForParkedToken() asks for the task type first and mints nothing.

Remaining tasks

Giving the features Orchestra ships their own types, so a timer ladder is TimerDefinition[] rather than a list of maps, is where the strings inside a feature would disappear too. That is a second pass and a much larger one; this stops at the node, and it is not filed.

The same numeric-node-id crash exists in the Complete Modeler's editing form, in separate code and with a separate cause, and is filed as #3622608.

Alternatives considered

A helper on the array, orchestra_node_feature($node, 'timers') or a static reader, gives the call sites one place to change and nothing else. It leaves the shape undocumented in the type system, leaves the plugin interfaces handing out untyped arrays, and adds a function call to a path that reads a node per token. Rejected: it buys the smallest part of the problem.

Typed accessors with no object, a NodeFeatureReader service asked for a node's settings, moves the reading out of the node rather than into it, so a caller still holds the array and can still reach into it directly. It also puts an injected service in front of what is a plain read of already-loaded data.

Keeping the array and relying on the PHPDoc array shape is what happened before. It works for PHPStan, which is how the last node-shape change was found, but only for code this project analyses: it says nothing to a contrib implementer at the call site, and the three copies of the shape are maintained by hand.

Notes

The authoring surfaces are deliberately not in scope. OrchestraCmForm and the modeler owner build nodes by merging submitted values into definitions, and they want arrays to do it; getRawNodes() and toArray() are the seams between them. Making the object mutable to serve both would give back the property that makes it worth having.

PHPStan enumerated this change and could not check it. Updating the declared node shape first turned 208 errors into a worklist, and iterating to no errors fixed the whole typed surface. The suite then found four runtime failures it had passed clean, every one of them a node view written back where an array belonged, and it is blind to a method call on a maybe-null value, which is what the first bug above is made of.

AI-Generated: Yes (Claude Code was used to help draft this issue summary and to write the code and tests on the merge request. I reviewed and ran the work myself before posting it.)