Audit of duplicated logic: the exactly-once timeout claim written twice, a migration preview that cannot follow the migration, three definitions of the definition an instance runs under, and a mail attachment mapped twice

An audit of 1.x at dce054f, the alpha13 release plus the three commits since it, read for duplicated logic. Every tool the project runs is clean at this commit: PHPStan level 3 over 730 files, PHPCS over the module root, and every CI job that ran on pipeline 932794. The findings below were all invisible to them, because every one of them is the same computation written more than once, which no linter reports.

Duplicated logic

• The exactly-once timeout claim is implemented twice. TimeoutSweeper::fireInner() and fireTimerInner() each re-read the token, refuse a cleared or re-armed deadline, and clear the deadline with a conditional UPDATE bounded by due_only, in about 25 lines apiece. This is the mechanism that stops an operator "fire now" from double-firing against a cron sweep, added for exactly that reason in #3611505: Engine exactly-once, notification side-effect isolation, and audit completeness. A change to one leaves timers and timeouts on different rules, and nothing fails.
• The migration preview cannot follow the migration. OrchestraVersionCommands::survey() and WorkflowMigrateForm::survey() are 22 identical lines, and both re-derive the instance selection that WorkflowVersionManager::migrate() performs for real, a third copy. The count an operator is shown and the set the migration moves come from three separate expressions. The command's copy is reached only indirectly, by the Drush command test; the operator form's copy has no test at all.
• "The node a token stands on" is written out sixteen times. Seven of them are the same four-line ternary; one guards with !== NULL where its siblings use instanceof; fourteen of the sixteen let definitionFor()'s InvalidArgumentException escape, including an access check and three forms, while the non-throwing sibling written for that case, effectiveDefinition(), is used by none of them.
• Three implementations of "the definition an instance runs under". DefinitionResolver::definitionFor(), DefinitionResolver::effectiveDefinition() (the same body without the throw), and a private byte-identical copy in InteractionResolver, which injects the entity type manager and the version manager only to keep it.
• The parked non-timer tokens of an instance are re-queried four times inside InteractionResolver, in two variants ("the first one" and "each of them").
• The capped role-recipient query is written twice, in the roles and roles_variable audience plugins, along with a second MAX_RECIPIENTS constant, candidates() and viewerTokens(). Only the source of the role list actually differs. roles_variable has no test of its own.
• The "read the configured variable" head is written three times, in the email, users and roles variable audiences, and with it the whole single-setting configuration form: defaultConfiguration(), submitConfigurationForm() and the textfield differ only in one description.
• A mail attachment is mapped twice, docblock included. toMailAttachment() is byte-identical in orchestra_mail and orchestra_easy_email, both of which depend only on the base module, where NotificationAttachment itself lives. Each copy's docblock claims "the channel owns this mapping", which two identical channels no longer make true.
• The subprocess step hand-builds the completion-scope select that CompletionConfigTrait::completionScopeElement() exists to provide, and drops its description in the process, so the setting is explained on six node types and unexplained on the seventh.
• Two of the current-user Views filters are the same class with a different message. CurrentUserCompleter and CurrentUserOnBehalfOf have identical query() bodies; five filters repeat the same constructor, factory and cache-context scaffolding; and they disagree on the non-SQL backend, where three throw and two silently drop their restriction. Both filters are exercised through the shipped personal lists, but only in their enabled half: the negated half, which is where the disagreement above lives, is tested nowhere.
• The assignment-aware field cacheability triple is written four times, in InstanceActLink, WorkItemHolder, TaskActLink and TaskOperations, with identical getCacheTags() and getCacheMaxAge() bodies under freshly written comments, while the module that holds two of them already has a field base class used by two of its fourteen handlers.
• "The payments pinned to an instance's tokens" is computed in three places: PinnedPayment::forInstance(), StoredCardCleanup and InstancePaymentCleanup, the last two bypassing the service that owns the concept. PinnedPayment also repeats its own "load the first id or nothing" tail.
• "The rows of type X referencing this instance" is queried at nine sites across the cleanup hooks, each pairing the same four-line query with ChunkedDeleteTrait, which could take the whole step.
• The comment task and comment operation repeat their configuration form, two elements whose only difference is the word for the person acting, though the module already has the shared-element-with-a-per-type-description pattern in CompletionConfigTrait::payloadVariableElement().

Other findings

• Two French strings are filed under the wrong module. "Interaction settings" and the interaction-plugin description are declared in orchestra_interaction but appear only in orchestra_interaction_task's fr.po, so a site running the interaction module without the task module shows both in English. The same class of misfiling as the payment_lapsed strings in #3615875: Pre-release audit since alpha12: a checkout left open by a throw, an outcome named wrong, a query on every node left, an inert control with no role, dead code, stale docs and misfiled translations. About thirty base-module strings are also copied redundantly into submodule catalogues.
• A cleanup hook's docblock describes a design that no longer holds. InstanceWorkItemCleanup says user tasks are "owned by this submodule" and that the engine "knows nothing about user tasks", from a namespace inside the engine, next to the work item entity. Two hook classes now sit on the same hook for a reason that has gone.
• The Drupal 12 lane has not run since Drupal 12 was declared. composer (next major) is a manual job, so the whole next-major lane is skipped on the last three 1.x pipelines, including the one for the commit that declared ^11.3 || ^12 in #3618602: Declare Drupal 12 compatibility, so a dependent's next-major lane resolves instead of failing on orchestra's core pin. A skipped lane is not a green one.
• A docblock narrates a retirement. ReviewInteractionBase opens with what happened "once handler_url and the built-in review form were retired", which is pre-release history rather than the current design.
• Test coverage gaps, all on code the fixes above touch: the roles_variable audience, which has no test of its own at all; the negated half of the personal Views lenses; and the operator form's migration preview.

Every finding is fixed on the merge request, each as its own commit, with tests for the four gaps listed above and for the two claims and the preview that are being unified.

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.)