Transition.emit cannot express that requiresPayload already validated its payload #48

New issue

Open

opened 2026-09-03 09:50:28 +00:00 by mph · 0 comments

mph commented

2026-09-03 09:50:28 +00:00

Owner

Found in medeca-modul-poptavky while implementing MEDECA-453. Two independent code reviewers (architecture and security) flagged the same fold without seeing each other's finding.

What happens

A Transition's emit recomputes a value that requiresPayload already validated, and folds the failure case to Seq.empty:

// PodaniEngine.scala, zalozNovyDokument
emit = ... novyDokumentIdentifikator(entity, cmd).fold(_ => Seq.empty, id => Seq(...))

That Left branch is unreachable only because WorkflowInterpreter.decide runs requiresPayload — which rejects the same Left — before it ever calls emit, over the one val entity = ctx.entity it captures (workflow/shared/src/main/scala/works/iterative/workflow/WorkflowInterpreter.scala:49-75). Both reviewers traced that code and confirmed the invariant holds today.

Why it is worth fixing

The guarantee lives in a different module from the code that depends on it, and it is not expressed in emit's type. If an interpreter refactor reorders those calls, or the validating function is reused outside decide, a command that should be rejected turns into zero events and no error — work silently dropped, with nothing to observe. The transition author has no way to state the dependency and no way to be warned when it breaks.

Logging the branch was considered and rejected at the call site: PodaniEngine is cross-compiled shared code with no logger in scope.

Suggested fix

Either:

Have decide pass requiresPayload's accepted payload into emit, so emit cannot re-derive the value and cannot fail; or
Document the ordering as part of the Transition contract, so an author knows emit may assume requiresPayload succeeded and a future interpreter change knows it must preserve that.

(1) is the stronger fix — it makes the invariant unbreakable rather than merely written down.

Found in medeca-modul-poptavky while implementing MEDECA-453. Two independent code reviewers (architecture and security) flagged the same fold without seeing each other's finding. ## What happens A `Transition`'s `emit` recomputes a value that `requiresPayload` already validated, and folds the failure case to `Seq.empty`: ```scala // PodaniEngine.scala, zalozNovyDokument emit = ... novyDokumentIdentifikator(entity, cmd).fold(_ => Seq.empty, id => Seq(...)) ``` That `Left` branch is unreachable **only** because `WorkflowInterpreter.decide` runs `requiresPayload` — which rejects the same `Left` — before it ever calls `emit`, over the one `val entity = ctx.entity` it captures (`workflow/shared/src/main/scala/works/iterative/workflow/WorkflowInterpreter.scala:49-75`). Both reviewers traced that code and confirmed the invariant holds today. ## Why it is worth fixing The guarantee lives in a different module from the code that depends on it, and it is not expressed in `emit`'s type. If an interpreter refactor reorders those calls, or the validating function is reused outside `decide`, a command that should be rejected turns into **zero events and no error** — work silently dropped, with nothing to observe. The transition author has no way to state the dependency and no way to be warned when it breaks. Logging the branch was considered and rejected at the call site: `PodaniEngine` is cross-compiled shared code with no logger in scope. ## Suggested fix Either: 1. Have `decide` pass `requiresPayload`'s accepted payload into `emit`, so `emit` cannot re-derive the value and cannot fail; or 2. Document the ordering as part of the `Transition` contract, so an author knows `emit` may assume `requiresPayload` succeeded and a future interpreter change knows it must preserve that. (1) is the stronger fix — it makes the invariant unbreakable rather than merely written down.