feat(backend): decision request. A node asks a human to decide before the run continues - #127
Open
piotrblaszczyk wants to merge 24 commits into
Open
feat(backend): decision request. A node asks a human to decide before the run continues#127piotrblaszczyk wants to merge 24 commits into
piotrblaszczyk wants to merge 24 commits into
Conversation
piotrblaszczyk
requested review from
librowski,
lukasz-jazwa and
szymon-t-sc
as code owners
September 8, 2026 08:44
Hand-written contract types for a gate node: declarable effects tuple, DecisionAction discriminated on effect, DecisionDeadline, DecisionContract. BaseNode gains an optional decision field; presence marks a gate. WB-500
Loose objects at every level so unknown keys survive. Actions are a discriminated union on effect with parse-time defaults for port, reasonRequired and maxIterations. Cardinality, unique names, distinct resume/reject ports, required-subset-of-properties and the deadline duration format are checked in the schema; graph rules come next. WB-500
…apshot properties becomes a loose object with the reserved decision key, so a gate contract is parsed in place and everything else passes through. A superRefine on the snapshot checks the graph rules: an explicit proposalSourceNodeId must be a direct predecessor, and a gate declaring rerun-source needs a resolvable source that is not itself a gate. resolveProposalSource is a pure function over the execution-model shape, shared with the pending-decision resource and the rerun loop later. WB-500
Every domain message the contract and graph validation can produce lives in DECISION_ISSUE_MESSAGES; the schemas and the snapshot refine read it instead of carrying inline strings. Tests assert the message at each path through the same dictionary, which surfaced one row that had been passing on path alone with the wrong rule in mind. WB-500
The mapper destructures decision beside errorPolicy and label, so a gate's contract reaches the engine as node.decision and never as ordinary config. The value is already validated and defaulted by the snapshot parse. WB-500
Publish parses the draft through workflowSnapshotSchema before copying it and answers with the same invalid_snapshot 400 as execute; both go through one parseSnapshot helper in routes/snapshot-validation.ts so they cannot drift. A null draft keeps its old behaviour and is not validated. Draft save stays unvalidated, pinned by a test. WB-500
A node that carries a decision contract is still a node; the product has no gate concept, and coining one invented a node kind the SDK never had. Identifiers, issue keys, messages, JSDoc, comments, test names and fixtures now say node, contract, or deciding node. The public JSDoc in packages/types changes with it, so the temporal dist was rebuilt and its tests re-run. WB-500
…mission A pure function the decision endpoint will call: it matches the submitted action against the contract, requires a reason on reject when the contract says so and a comment on rerun-source, checks every edited field for being declared, editable and, if required, not emptied, and derives resume-with-edits when a resume carries edits. The accepted result is a Decision carrying the matched action, the effect and what was submitted; refusals carry a code from the shared issue dictionary and a path into the request. Value types are a later validator. WB-500
The field on a node holds what the node asks a human to decide, not a decision, so it is now data.properties.decisionRequest and BaseNode.decisionRequest, typed DecisionRequest. That frees the word decision for the lifecycle it now names end to end: DecisionRequest is what the node asks, SubmittedDecision is what the decider sends, Decision is what validation accepts. Issue keys follow (node_without_decision_request, source_has_decision_request). The example rerun action is ask-again, so request means one thing in an authored snapshot. WB-500
The log keeps only what the code cannot say: why the request is data on a node rather than a node kind, why name and effect are split, why edit is not an action, when validation runs, the lifecycle names and the alternatives that were rejected. The README points at where the request lives, when it is validated and how a broken one is reported. WB-500
…re parsing JSON.parse turns "__proto__" into an ordinary own key and zod's loose objects copy unknown keys with a plain assignment, which for that key swaps the output's prototype. A draft could smuggle an unvalidated decision request through properties.__proto__: the snapshot schema saw nothing, the mapper copied the inherited value into a real field on the way to the engine, and a malformed variant made safeParse throw a TypeError (500 on publish and execute). workflowSnapshotSchema is now wrapped in a preprocess that walks the raw value and refuses the key at its path with the usual invalid_snapshot 400. Draft save is unchanged. WB-500
…s rules validateSubmittedDecision took a typed SubmittedDecision but nothing enforced the shape at runtime: edits as an array or a number were accepted into Decision.edits, and a non-string reason threw from trim(). The shape now has one source, submittedDecisionSchema, from which the type is derived; the decision endpoint parses a body with it before calling, and the function checks only the rules. WB-500
Pins four behaviours that were live but untested: an own __proto__ key in a submission's edits is dropped by the record parser without touching the prototype; edits on a reject or rerun-source submission keep the declared effect and ride along (the open point in the decision log); a request with no schema at all is refused at schema; addressing an action by its label instead of its name is unknown_action. WB-500
Blank names, labels and ports are refused, not just empty ones. A self-loop no longer counts as a predecessor when resolving the proposal source. The deadline message names the upper bound. Comments trimmed to the repo rule; the decision log records that decisionRequest is present or absent and never null, points at the worked example fixture, and lists node-id uniqueness as a pre-existing gap. README notes that structural issues surface before graph rules. WB-500
The exported request schema states that it parses a request already inside a guarded snapshot; raw JSON goes through workflowSnapshotSchema. The errorRoute comment sits on portSchema again, the decision log names the guarded parser precisely, a test name says only what it exercises, and three matrix edges are pinned: the inclusive deadline ceiling, a string in edits, a non-string action. WB-500
piotrblaszczyk
force-pushed
the
feat/WB-500-decision-contract
branch
from
September 10, 2026 05:47
dbc95c4 to
857a357
Compare
dawidaksamski
approved these changes
Sep 10, 2026
Nesting depth is whatever the client sent, and the scan that runs before parsing was recursive. A draft a few thousand levels deep, well inside the 1 MB body limit, made it throw RangeError. zod does not catch that, so publish and execute answered 500 where the contract promises invalid_snapshot 400, and the stored row stayed unpublishable for good. Measured on Node 22, the version CI uses: the old scan died above depth 3125, around 6 KB of body. The walk now uses an explicit stack and copies the path once, on a hit, which also drops the quadratic path copying. The README gains a note that the check does not weigh position, so it also refuses a key buried in an opaque node property, where zod never copies keys one by one and the key is inert.
The form schema is documented as validated for shape only and opaque to the engine, but every property was required to carry a string `type`. JSON Schema makes `type` optional and allows a list of names, and JsonForms 3.5.1 generates a control for a property carrying only `enum`, `$ref`, `anyOf` or `oneOf`. All of those were refused with invalid_snapshot on publish and execute. Nothing reads a form property's `type`; `readOnly` and `x-pii` are the two keys the backend does read, and their checks are unchanged. Decision 4 now spells out what "shape only" covers, since the loose wording is what let this creep in.
The backend README and the field's public JSDoc both said its presence, never `type`, is what makes the run park. No engine reads the field: run a graph whose node carries a valid request and the executor runs, downstream runs, the run completes, and nobody is asked anything. The JSDoc ships in the Temporal plugin's declarations, so a consumer would have built against it. Not a gap to close but a sentence that conflated two markers. The backend and the decision endpoint find a request by the field; a run stops where a node's executor returns a waiting result, deliberately, so the runner learns no product's vocabulary. Both texts now say that, the node type whose executor only parks is listed as work outside this change, and a runner test holds the docs to it by failing the day the runner starts reading the field.
The submission validator only walked the outermost object, so a readOnly child was rewritten by replacing the object that held it, a child named by a nested required list was cleared, and a readOnly field inside an array item was swapped. All three came back as an accepted resume-with-edits. The walk is now one recursion over the edited value paired with its schema. An array is described as an object whose keys are indices and whose elements are all declared by `items`, so the three rules stay a single flat loop and the error carries the full path. A level the form does not describe inline declares nothing editable, so an edit into it is an unknown field rather than being waved through.
The guard wrapped only workflowSnapshotSchema, so the exported decisionRequestSchema swapped the parsed output's prototype on its own. Every level of a request is a loose object, and an own __proto__ at the request, an action, the form schema or a form property came back as an inherited field no schema had seen. Today's routes reach a request through the guarded snapshot, so nothing was exposed; the trap was waiting for the decision endpoint and the pending-decision resource. The export now guards itself. own-proto-key moves to domain/schema/, so the decision schema can use it without domain/decision importing from domain/mapper, which would have turned a one-way dependency into a two-way one. mapNode reads own keys only. It is the one read that turns an inherited field into a real one on the node sent to Temporal, and it is exported, so it can be reached down a path the guard never saw.
Resolving the proposal source was skipped unless the request declared an explicit source or a rerun-source action. A node with no predecessor published, and so did one behind a join with no explicit source, while the same resolver answered source_missing and source_ambiguous for those graphs. The first is an orphan the runner fails anyway; the second hands the decider a form and nothing to judge, discovered while a person is already waiting. The resolver now runs for every request. Only the rule that the source must not carry its own request stays tied to rerun-source, the one effect that re-runs it. Two accepting cases became refusals, and two fixtures gained a predecessor so they go on testing what their names say. The field's public JSDoc and decision 6 said the permissive thing; both now state the rule. Decision 6 also records why the pending-decision resource keeps its no-proposal path: a resolved source can still be skipped at run time.
Publish validated any draft that was not null; execute short-circuited on any falsy value. For an empty string, zero or false the two routes disagreed, one answering invalid_snapshot and the other published_version_missing, while the shared helper's comment promised they could never drift. Execute now tests for null, and the route test that compares their answers became a table over exactly those values. The section comments this change had added to the route tests restated the test names; each is now the header plus the one fact the code does not show.
The collector had been failing since June: it looks for a Date line and the TenantContextPort log carried Proposed/Landed instead, and the durable-pause log had no metadata block at all. Both headers now follow the convention of the other logs, with every date and the landing commit kept. The regenerated index gains eight entries that had been missing, five of them since spring.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A node asks a human for a decision by carrying
data.properties.decisionRequest: the actions offered, the JSON Schema of the form, the node whose output is judged, and an optional deadline. This PR adds the data shape, its validation, and the pure function that checks a submitted decision against it. No endpoint, no timer, no rerun loop yet; those are the follow-up tasks.Changes
packages/types:DecisionRequestand friends,BaseNode.decisionRequest. Public JSDoc on every field (it ships in the Temporal plugin's.d.ts).apps/backend/src/domain/decision/decision-request-schema.ts: zod schema, loose objects at every level, defaults forport/reasonRequired/maxIterationsmaterialised at parse time.decision-issues.ts: one dictionary of every domain message the request and submission validation can produce.proposal-source.ts:resolveProposalSource, the single definition of "whose output does this node judge".validate-submitted-decision.ts:DecisionRequest+SubmittedDecision->Decisionor a typed refusal.apps/backend/src/domain/mapper: the snapshot schema validates the request in place and checks the graph rules in asuperRefine; the mapper liftsdecisionRequestout ofconfig.apps/backend/src/routes: publish now validates the draft through the sameparseSnapshothelper as execute; draft save never validates (pinned by a test).apps/backend/decision-request.decision-log.mdand a README section.Decisions worth a look
Recorded in the decision log; the ones that changed during implementation:
decisionRequest: it holds the question, not the answer. Lifecycle names:DecisionRequest->SubmittedDecision->Decision.{ value; error?: undefined } | { value?: undefined; error }.approve,reject,ask-again.Known gaps, unchanged on purpose
draftJson === nullstill publishesnull, unvalidated, as before this PR.Notes for the reviewer
@workflow-builder/typesreaches consumers only through@workflowbuilder/temporal, which is unpublished.