Payment: paths that can charge, mis-charge or lose the run

Problem

Findings in the payment integration, the highest-stakes surface in the module. The two most serious let a run end while money is in flight, and bounce a payer who cancelled straight back to the gateway with no way out.

14 findings: 2 high, 6 med, 6 low.

High severity

A visitor outcome taken while a payment is in flight ends the run and leaves the payment live, and the provider then takes money for a run that no longer exists

Where: modules/orchestra_payment/src/Plugin/Interaction/PaymentInteraction.php:354-359 and :833-846; modules/orchestra_payment/src/EventSubscriber/PaymentWorkflowSubscriber.php:118-139

Nothing prevents a parked payment token from being resumed by a visitor-driven outcome (the example workflow's cancel, a "back to the form" step) while a pinned payment is away at the provider, and nothing cancels that payment when the token advances by any route other than the payment's own event. CapabilityResolver::resolveSignal() validates the outcome against getSignalOutcomes() only; it never consults the payment. The landing page does guard the in-flight case, but only on the render path: the outcome's signal URL is a capability bearer link already in the payer's hands, so browser-back after the handoff, or the link from an earlier mail, signals it anyway. When the provider then settles, the subscriber claims the event with setHandled(), so a consumer's own reaction stands down too, and records the money-taken-for-a-dead-run case at debug, conflating it with the benign already-advanced signal.

shouldSkipLanding() tests only the failed state, so a payer who cancels at the provider is bounced straight back to the gateway, for ever

Where: modules/orchestra_payment/src/Plugin/Interaction/PaymentInteraction.php:651-654, contradicted by its own docblock at :641-643, the class docblock at :46, the config-form description at :215 and the retry notice at :805

All four of those promise that "a failed or canceled attempt still falls through to the landing page", so the payer keeps a way to retry or take a configured outcome. The condition checks the failed state only. A payer cancellation is not a failure: kessai's own contract splits them, and the real gateway does too, calling cancelPending() when the payer cancels at the checkout page and fail() only on a decline or error. So the one state the docblock names alongside failed is the one the condition misses, and with skip-landing configured the canceller is redirected to the gateway on every return, with no way to reach the cancel outcome. The expired state is a third case nothing considers.

The rest

One line each: what it is, and where. The failing scenario and the fix for each are carried by its own commit.

• med The pay path is neither locked nor conditional: two concurrent clicks create two payments pinned to one token, and only one is ever settled or released. PaymentInteraction.php:386-497, :872-899, src/PinnedPayment.php:112-122
• med SettleTask captures a re-resolved amount it never bounds by the authorization; kessai clamps it down silently and the step still records "settled". src/Plugin/TaskType/SettleTask.php:132-145
• med Nothing claims the kessai cancel event on a workflow-pinned payment, although this module itself produces that event on two paths. src/EventSubscriber/PaymentWorkflowSubscriber.php:56-64, against its class docblock; producers at PaymentInteraction.php:550 and SettleTask.php:140
• med An authorization from an earlier pass through the payment step is never released. src/PinnedPayment.php:95-122, consumed by SettleTask.php:123-127
• med A non-numeric amount variable is an uncaught ValueError on the payer's page, not "nothing to pay". src/Plugin/PayableResolver/VariableResolver.php:128-131, same exposure in src/Payable.php:73,76 and PaymentInteraction::formatAmount():683
• med The mark-paid/failed route is the only token-drive route without the tenant access check, and the operation is offered to operators who then get a 403. orchestra_payment.routing.yml:1-12, src/Hook/OrchestraPaymentOperationsHooks.php:50-76
• low Two docblocks and a schema comment promise an intrinsic cancel outcome the code never renders. PaymentInteraction.php:43-46, config/schema/orchestra_payment.schema.yml:41-43
• low The landing page prices the run twice per render, three times on a refused or changed click, unmemoized. PaymentInteraction::show():319,344, pay():447-477, PayableResolverBase.php:35-55
• low Every amount comparison is hard-coded to two decimals and truncates rather than rounds. formatAmount():683, Payable.php:73,76, SettleTask.php:135, VariableResolver.php:130
• low Dead code and stale names in the payment plugins.
• low Two tests would pass against broken code.
• low An untested transient-failure branch, and a schema key the shipped config omits.

Remaining tasks

• Fix the findings above, one commit per finding where they are independent.
• Decide what a visitor outcome should do to an in-flight payment, since refusing it and cancelling the payment are both defensible and only one can be the default.
• Add or correct a test for every behaviour change.
• Run every linter and the affected test classes before pushing.

This issue summary was drafted with the assistance of an AI agent (Claude). The analysis and the wording were reviewed by me before posting, and accountability for the content is mine.