Interaction, content, ECA and the modelers, and deletion that refuses to cascade

Problem/Motivation

An ECA node that can take an outer scope's answer as its own, a delete confirmation relabelled Save, a retype that strands the old task type's data, and the visitor-facing queries that read more than they need.

Found by a complete file-by-file audit of the whole module at commit bc205f4f: 1130 tracked files, 1126 read in full, 4 binary assets recorded as skipped. Every linter was already clean at that commit, so none of this is something a tool reports: phpcs Drupal over 1008 files and DrupalPractice over 814 both at zero, PHPStan level 5 with no errors, cspell against the CI configuration over 1061 files with no issues, eslint with no errors, stylelint with no problems, composer validate, and the per-project translation check reporting every string translated in the catalog of the project that ships it.

Findings

High

1. An eca_event node reads the async marker and outcome without clearing them, so it can take an outer scope's value as its own answer (modules/orchestra_eca/src/Plugin/TaskType/EcaEventTask.php line 157, correctness). execute() writes TOKEN_ID, TOKEN_INSTANCE and TOKEN_PAYLOAD_VARIABLE before dispatch but never clears TOKEN_ASYNC or the configured outcome token, then reads both back from ECA's shared request-scoped bag as this node's answer. An ECA model that calls 'answer this task later' (or holds any value under the node's outcome-token name) and then starts a synchronous Orchestra workflow inline makes the inner run's eca_event node park forever with nobody to resume it, or route on an outcome no model gave it, with no log and no incident. Fix: Unset the two answer-carrying names before dispatching; the existing restore loop already puts the outer values back. Add a test nesting an unanswered eca_event node inside a model that has set the async marker.

Medium

1. The task-screen form alter relabels a delete confirmation as 'Save', stranding the task on a 404 (modules/orchestra_content/src/Hook/OrchestraContentFormHooks.php line 82, correctness). The alter assumes the form built on the task route is an editing form and rewrites the confirm button's label to Save plus a Save-and-outcome button per outcome; on the delete operation the controller explicitly supports and access-checks, that button deletes the content while announcing 'Saved.'. A site builder who sets the step's form operation to delete gives the reviewer a screen whose primary button reads Save; pressing it deletes the content, shows Saved., and lands on the 404 'This task has no attached content to edit' with the work item still open and no screen left that can complete it. Fix: Gate the relabelling on the form object not being a ConfirmFormInterface, or reject a delete operation in the interaction's settings form and drop the delete branch from the controller's access check.
2. The plugin-swap AJAX contract is spelled out at four sites (src/InnerPluginSettingsTrait.php line 54, duplication). The six-key element that makes a plugin select re-render its settings subform (#submit changeNodeFeaturePlugin, #executes_submit_callback, #ajax rebuildStructureAjax, #limit_validation_errors, #feature, #node_id) is written out verbatim at four sites, while InnerPluginSettingsTrait already owns the inner-plugin settings contract. The keys must stay in sync with the swap handler's reader; a change that updates three sites leaves the fourth silently not swapping, so an author picking a different deadline provider sees the settings subform not follow. Fix: Add a pluginSwapElement() helper to InnerPluginSettingsTrait and call it at all four sites.
3. Changing a node's task type strands the previous type's node-feature data (modules/orchestra_modeler/src/Plugin/ModelerApiModelOwner/Orchestra.php line 477, correctness). getNodeExtras() preserves and restores every non-core node key unconditionally, while the editor only re-collects the keys of the features that apply to the node's new task type, so retyping a node leaves the old type's assignments, timers or timeout on it, invisible in every editor. The same pass does shed the old type's config, so the asymmetry is unintentional. An author who changes a User task node to Passthrough and saves gets a workflow that still declares a module dependency on orchestra_inbox, so the site refuses to uninstall that module and names a workflow with no user task left in it, with nothing in either editor showing why. Fix: Filter the preserved extras down to the config keys of getFeaturesForTaskType() for the new type before restoring, in the owner so a retype in BPMN.io is covered too.
4. A validation error inside a collapsed settings, routing or condition section leaves that section collapsed (modules/orchestra_cm/src/Form/OrchestraCmForm.php line 806, correctness). openRows() maps an error's third path segment to a feature detail key only, so it re-opens feature editors but never the settings, routing or condition sections that hold the other three kinds of embedded subform. An author who presses Save sees an error pointing at a field inside a details element the rebuild left closed, so the field the message names is not on screen - the exact failure the mechanism and its deliberate novalidate exist to prevent. Fix: Translate the third segment to the section it actually names, and add the condition case for the flows branch.
5. A workflow's tenant can never be set from either modeler (modules/orchestra_modeler/src/Plugin/ModelerApiModelOwner/Orchestra.php line 41, documentation). docs/multi-tenancy.md documents that a workflow is either shared or scoped to a single tenant, but the model owner leaves ModelOwnerInterface::modelConfigFormAlter() at the base no-op and the Complete Modeler's Model section offers only machine name and label, so nothing in the project ever writes an orchestra_workflow's tenant key. An admin on a multi-tenant site who wants a tenant-scoped workflow finds no field for it in either modeler and can only produce one by hand-editing the exported YAML and importing it. Fix: Implement modelConfigFormAlter() with a tenant select and add the same field to the Complete Modeler's model section, or correct docs/multi-tenancy.md to say scoping is a config-import-only operation.
6. The link variant's request_changes notification has no Orchestra Mail wording, so the override makes the email worse (modules/orchestra_interaction_webform/modules/orchestra_interaction_webform_examples/config/install/orchestra.orchestra_workflow.request_validation_link.yml line 104, correctness). The modify step overrides the park notification type to request_changes, but the example module's own hook_mail_alter words only request_processed and request_rejected, so under orchestra_mail the override falls through to the neutral default branch instead of the tailored message the config comment promises. A poster on a site running the shipped example with orchestra_mail gets a changes-requested email with the bare node label as subject and a generic body - no reviewer comment, no reviewer name - strictly less than the untyped default would have produced. Fix: Add a request_changes case to the example's mailAlter() mirroring the easy_email body, and correct the config comment to say the tailored HTML template needs orchestra_easy_email.
7. The interaction operation never remembers the caller's return across an interaction hand-off, unlike its task twin (modules/orchestra_interaction_operation/src/Controller/InteractionOperationController.php line 77, correctness). operation() returns the interaction response without storing the incoming orchestra_return target, so a hand-off redirect loses it; the identical task controller stores it server-side for exactly this case, and the webform handler's completion code reads a value only that controller ever writes. An operator who opens an interaction operation from the process-instances list or the pending-actions page, where the interaction is a webform, is not returned to the list they acted from after submitting; the same step modelled as an interaction task takes them back. Fix: Add the same three lines to operation(), and make signal() resolve through OrchestraReturn and forward its argument on the guarded-outcome redirect as orchestra_ui's OperationController already does.
8. Two visitor-page token queries load an unbounded token set the definition could narrow (modules/orchestra_interaction/src/InteractionResolver.php line 210, performance). findBranchContinuationTokens() loads every token of the continuation chain and findTerminalInteraction() loads every consumed token of the instance, with no range and no node-id narrowing, while the sibling findChainMessages() deliberately narrows by the nodes the pinned definition says can carry an interaction. An external party returning to the dispatcher after acting, or landing on the finished page of a loop-heavy run, has every token of that chain or of the whole run loaded to answer a question about a handful of nodes - and the dispatcher does it twice per page. Fix: Resolve the qualifying node ids from the effective definition up front and add the same node_id IN narrowing, returning early when the list is empty.
9. The interaction 'Open' operation link lacks the translation context its sibling uses (modules/orchestra_interaction/src/Hook/OrchestraInteractionOperationsHooks.php line 86, translation). The hook builds the operations link title as a context-less TranslatableMarkup('Open') where orchestra_ui builds the same link in the same dropbutton with the Operation context; the context-less French for Open in this catalog is the adjective, not the verb. A French operator on the instance trace sees the interaction preview action labelled 'Ouverte' sitting next to the operator action labelled 'Ouvrir' in the very same operations list. Fix: Add the Operation context and replace the fr.po entry with the msgctxt form.

Low

1. The dispatcher resolves the whole capability outcome twice per page view (modules/orchestra_interaction/src/Controller/InteractionController.php line 108, performance). The title callback and the controller each call the capability resolver with identical arguments, and CapabilityResolver memoizes nothing, so every entity query the resolution makes runs twice per dispatcher page load. The external party's page issues double the parked-token, chain-message and all-consumed-token queries, and the title callback is a separate controller instance so nothing on the object can help. Fix: Memoize the outcome in CapabilityResolver, keyed by instance id plus the grant's token scope; the service is a singleton.
2. park_notify is accepted on a node with no external interaction and then silently sends nothing (modules/orchestra_interaction_notification/src/Plugin/NodeFeature/ParkNotification.php line 115, correctness). validateEditor() checks only the type string's format, so an author can add a Notify on arrival audience to any parking node, but the subscriber returns early for every node whose interaction plugin is empty, with no error at authoring time and no log at runtime. An author who ticks Notify on arrival on a user task or an interaction task with no external interaction saves the audience, reads the feature's promise to send them its link, and nobody is ever emailed, with nothing saying why. Fix: Refuse a non-empty audience in validateEditor() when the node's interaction feature names no plugin, pointing the author at the External interaction setting.
3. MessageInteraction's comments say the output boundary is admin HTML; respond() renders through the restricted format (modules/orchestra_interaction/src/Plugin/Interaction/MessageInteraction.php line 110, documentation). Two comments assert unconditionally that the stored message is filtered as admin HTML on output so the format is not the render-time security boundary, which respond() contradicts whenever the format exists - and respond()'s own comment says the opposite. A maintainer reading the settings form is told the restricted_html format is only an authoring aid and may widen or drop its filters, when on the public dispatcher route that format is the render-time allowlist. Fix: Reword both comments to say the body renders through restricted_html when it exists and falls back to admin-HTML filtering when it does not.
4. Two assertNotSame() calls in the interaction token test pass whatever the code does (modules/orchestra_interaction/tests/src/Kernel/InteractionTokenTest.php line 67, tests). Both assertions compare the grant's instanceId against 43 immediately after the preceding line asserted it is 42, so they are tautologies dressed up as replay and cross-instance guards. A maintainer believes replay against another run is pinned there; it is not, and if the cross-instance test were dropped these two would still pass. Fix: Replace both with a real check, or drop them and point the class docblock at the test that proves it.
5. getEntityProcessInstances() loads one instance per attachment through the reference field (modules/orchestra_content/src/AttachmentManager.php line 182, performance). The entity-to-runs lookup loads the attachments with loadMultiple() and then resolves each instance individually via the reference field, one storage load per attachment. Site code using the documented entity-to-runs API to list every process about a content entity issues N queries for N runs instead of one, and hangs each loaded instance on the attachment's field item. Fix: Collect the instance ids and issue a single loadMultiple(); group by entity type id in getProcessInstanceEntities() for the same reason.
6. EntityTaskController's class docblock still says the interaction hands it the key and form operation (modules/orchestra_content/src/Controller/EntityTaskController.php line 19, documentation). The class docblock describes removed behaviour: it says the interaction passes which entity to edit and which form to open, which the code, the interaction plugin and the test all deliberately do not do any more. A maintainer believes the key and the form operation arrive in the request and would not think twice about reading them from the query string again, re-opening the hole the code below was written to close. Fix: Reword the first sentence to say it hands the assignee here with the parked token, and nothing else.
7. Composite sub-condition rows render an unnamed fieldset and identical Remove buttons (modules/orchestra_cm/src/ConditionTreeBuilder.php line 263, accessibility). Each sub-condition row is a fieldset with no #title, which core's template renders as an empty legend, and the row's only other landmark is a button labelled just Remove. A screen-reader user building an All/Any condition hears an unnamed group per row and N buttons all called Remove, with nothing saying which sub-condition each belongs to, in the module whose own description is 'Author Orchestra workflows in accessible Drupal forms'. Fix: Give the fieldset a #title naming its position and carry that into the Remove button's accessible name.
8. Flow- and node-label width and height are captured and stored but never restored (modules/orchestra_bpmn_io/src/Hook/OrchestraBpmnIoHooks.php line 161, documentation). extractLayout() writes width and height into flow_labels and node_labels and the comment says it captures them so the label keeps its place and size, but layout_restore.js reads only x and y for both maps. A maintainer believes label size round-trips across a modeler switch when it never has, and every workflow's stored layout carries two float values per label that nothing will read back. Fix: Drop width and height from the two label maps and their schema mappings, and reword the comment to match the honest node_labels comment above it.
9. A class docblock points at a hook method that does not exist (modules/orchestra_bpmn_io/src/Plugin/ModelerApiModeler/LayoutAwareBpmnIo.php line 16, documentation). The class docblock says the modeler is swapped in by a modelerInfoAlter hook, but that class's method is modelerApiModelerInfoAlter(). A maintainer following the pointer greps for a method name that appears nowhere in the codebase. Fix: Name modelerApiModelerInfoAlter(), ideally as a real @see.

What shipped

The nineteen findings above, one commit each, every defect with a test that fails against the unfixed code. Two of the audit's prescribed fixes were replaced by a different one, and those commits say why: the capability outcome is not memoized at all but computed once, and the delete-confirmation screen is refused rather than relabelled.

The branch also carries a second, larger piece of work, folded in here rather than opened separately. Three configuration entities stopped cascading their deletion into data. Deleting a tenant cancelled everything running in the realm and then deleted every run and workflow scoped to it; deleting a status edited the workflows referencing it, since core strips a removed dependency out of its dependents; deleting a workflow deleted every run of it whatever its state. A run is the record of work that happened and a workflow is a process someone authored, so all three now refuse while anything refers to them, in each entity's own preDelete() ahead of the dependency handling that used to do the cascading. Each delete confirmation stays reachable and lists what is in the way, with its confirm button disabled. Because a thing that cannot be deleted needs a way to be retired, workflows and statuses gained core's enabled flag: a disabled workflow starts no new run and leaves the runs it has to finish, and a disabled status stops being offered when authoring while the nodes and runs that already name it keep resolving it.

User interface changes

The delete confirmations for a tenant, a status and a workflow now list what still refers to the subject, per kind and with counts, and disable the confirm button until that list is empty. The Complete Modeler's Model section gains a Tenant scope select and an Enabled checkbox; the Statuses list and form gain the same flag, shown as a column so a retired term reads as retired. The Workflows list gains Enable and Disable operations and an Enabled column from the Modeler API, which keys them on the flag this branch declares, and no longer offers Start on a workflow the engine would refuse to start. Composite sub-condition rows are named by position, and their Remove buttons carry that name for a screen reader. No new screens.

API changes

Pre-1.0, so a signature is changed where the better shape needs it rather than preserved. StatusRepositoryInterface loses getEnabledInTenant(), which the one caller that wanted it no longer needs; AttachmentInterface gains getInstanceId() so a batch lookup can read the raw reference; three usage readers (TenantUsage, StatusUsage, WorkflowUsage) are added as services, each the single answer to "what still refers to this"; the interaction dispatcher's title callback is gone, the page carrying its own title instead. Two purge hooks are removed with the cascading they performed. Everything else is named in its finding.

AI-Generated: Yes (Claude Code was used to run this audit, to write the code and tests on the merge request, and to draft this issue summary. I reviewed the findings against the source myself before posting.)