diff --git a/CONTEXT.md b/CONTEXT.md index ef6492a..3a2f31c 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -97,11 +97,21 @@ _Avoid_: helper, macro, formula, built-in **Slot**: A named **Template** together with the type it must produce — `{name: "to", template: "{{ var.digest_to }}", expectedType: text}`. It names the *place* something is resolved into, which is -what distinguishes it from the **Template** it holds. The type is always the field's, declared by -the **Component Manifest** or — for a **Branch**'s `when` — by the language; it is never inferred -from the expression. +what distinguishes it from the **Template** it holds. **The type is never inferred from the +expression**, and it comes from one of three places: the **Component Manifest**'s field, a +declaration in the document (a **Block**'s `params`/`outputs`, a **Variable**'s `t`), or the +language — a **Branch**'s `when` and a **Repeat**'s `until` are boolean because a condition is. _Avoid_: binding, target, assignment, field value +**Variable**: +Named mutable state declared under a **Board**'s `vars:` and read anywhere on it as `{{var.}}`, +regardless of where it was written — which is what lets a **Repeat**'s body carry something back to +a **Step** that runs before it. **Its type is declared, in `t`, never read off its value**: `value` +is only the *initial* value, because `core.set_var` writes the same variable from a **Step**. A +Board's variables are its own — the workflow's at the root, a **Block**'s inside one, rebuilt on +every invocation — so a `core.set_var` can never reach out of the Board it is on. +_Avoid_: state, global, parameter, field + **Mapping**: A **Step** (`core.map`) whose outputs are the entries the user wrote into it rather than anything its **Component Manifest** declares. It is the third verb Hatua interprets structurally, alongside @@ -130,6 +140,16 @@ One labelled child path of a **Fork**, with an optional `when` condition and its Order is meaningful in a condition fork. _Avoid_: path, case, leg +**Repeat**: +A container **Step** (`core.repeat`) that runs its children, then evaluates its `until` condition, +and runs them again while that is false. **The body always runs at least once**, which is what +distinguishes it from `core.for_each` — a list may be empty — and what lets one discharge a +**Block**'s obligation to reach a `core.return`. `until` sits beside `steps:` rather than under +`with:`, because a **Component Manifest** field carries a rendering kind and cannot say "a +**Template** that must produce a boolean". It binds nothing: a counter is a **Variable**, written +by `core.set_var`. Nothing in the document bounds the iterations — a runner imposes its own ceiling. +_Avoid_: while, do-while, until-loop, retry + **Derived Layout**: The rule that a **Step**'s position on the flow map is computed from the tree on every render and never persisted. This is what guarantees a hand-edited **Workflow Definition** and the map can never diff --git a/docs/adr/0013-control-flow-nests.md b/docs/adr/0013-control-flow-nests.md index 4eb30e5..4027760 100644 --- a/docs/adr/0013-control-flow-nests.md +++ b/docs/adr/0013-control-flow-nests.md @@ -83,7 +83,7 @@ that gives it a reader**, and amends this ADR when it does. - **`core.repeat`** — repeats its children until a condition holds. The gap `core.for_each` leaves: it iterates a collection, and nothing repeated on a condition. This is what "send it back for another revision" is, and what "ask whether to process another batch" is — the target of both is - the head of an enclosing container, which is why neither needs a jump. + the head of an enclosing container, which is why neither needs a jump. Its shape is below. - **`core.return`** — publishes a **Block**'s declared outputs and ends that Block. It is the mirror of `core.map`: the one component whose *inputs* no manifest can declare, because they are the enclosing Block's `outputs:`. @@ -195,10 +195,64 @@ the ordinary missing-field diagnostic. condition fork is first-match-wins, so one whose every branch is conditional can match none of them and fall straight through. A return inside a `core.for_each` body exits the Block early and is perfectly legal, but it never discharges the obligation, because the list may be empty and the body may never -run. That is the same reasoning that keeps sibling branches out of scope, applied to time instead of -to paths. Steps sitting after a return on the same path can never run, and are reported the way an +run. A `core.repeat`'s body does discharge it, for the mirror-image reason, and the section below +settles why. That is the same reasoning that keeps sibling branches out of scope, applied to time +instead of to paths. Steps sitting after a return on the same path can never run, and are reported the way an unconditional Branch that swallows the ones behind it already is. +## `core.repeat` tests after the body, and that decides four things + +```yaml +- id: revise + use: core.repeat + until: "{{ var.approved }}" + steps: + - { id: draft, use: component.agent.act } + - { id: record, use: core.set_var, with: { key: approved, value: "{{ steps.draft.ok }}" } } +``` + +**The body runs, then `until` is evaluated; false runs it again.** A pre-tested loop was the +alternative and is refused, because the two are not symmetric. A pre-tested loop is expressible as a +post-tested one whose body opens with a `core.fork` — a `core.return`, a future `core.stop`, or +simply nothing on the other branch. A post-tested loop is expressible as a pre-tested one only by +**duplicating the body** above the loop, which is the deduplication cost `blocks:` exists to pay +back, reintroduced by a control-flow choice. The motivating cases decide the same way: "send it back +for another revision" and "ask whether to process another batch" both have nothing to test until the +body has run once, so a pre-tested loop would make every use of one begin with a `core.set_var` +seeding a condition the user did not want to think about. + +**So a `core.repeat` discharges a Block's return obligation, and a `core.for_each` does not.** The +question `alwaysReturns` asks is only ever *is this region guaranteed to run at all* — a list may be +empty, a repeat's first pass cannot be skipped. That is the same reasoning that keeps sibling +branches out of scope, applied to time rather than to paths, and it now has one answer covering both +loop verbs rather than a special case for each. + +**`until:` is a structural key beside `steps:`, not a field under `with:`.** This is the wall +`blocks:` already hit from the other side: a Component Manifest field carries a rendering `kind` and +no type, so `slotsFor` recovers the expected type from `FIELD_KIND_TYPES`, and that vocabulary cannot +express "a Template that must produce a boolean" at all — `bool` holds a literal rather than a +Template. Under `with:` a condition would type-check as *text*, so `{{ steps.s2.count }}` would pass +as a termination condition and the half of the contract the field exists to carry would be gone. A +Branch's `when` sits in the same position for the same reason, and `repeatSlot` is `whenSlot` with a +different name. + +**A repeat binds nothing.** `core.for_each` exposes `item`, and it can: `item` is resolved by +following the loop's `list` back to its source output, so its type is derivable from the document. A +repeat has no list. An iteration index or count would therefore be a binding nothing declares and +nothing types — and it would have to live somewhere, which under ADR-0014's closed roots means a +seventh root or a second bare token beside `TRIGGER`. Both are a permanent cost for a counter +`core.set_var` already writes, which is the trade the section below makes once and should not make +twice. + +**Nothing bounds the iterations, and that is a decision rather than an omission.** Recursion is +refused above because it is a property of the *document* — a cycle in the call graph, decidable by +reading the file. Whether an `until` ever goes false is not: it depends on values that exist only +during a run. A `max:` written into the document would be a number Hatua could neither check nor +enforce, and a runner ignoring it would still be conformant, which is a promise the file does not +keep. **Bounding is the Host runner's obligation**: a runner imposes its own iteration ceiling and +fails the execution when it is reached, the way it already owns timeouts and retries. Hatua does not +execute, so that is the one place the contract can honestly sit. + ## Loop state is a Board variable A repeated region usually has to carry something backwards — the reviewer's feedback reaching the @@ -206,6 +260,23 @@ draft step that runs before it. Nothing positional can do that: the writer runs it is not in scope. **`core.set_var` is the mechanism**, because a `var` is scoped to its **Board** and readable anywhere on it regardless of where it was written. +```yaml +- id: record + use: core.set_var + with: { key: approved, value: "{{ steps.draft.ok }}" } +``` + +**`key` names a variable on the Step's own Board**, and there is no second list to fall back to — +which is what makes "a Block's `core.set_var` can never reach the workflow's variables" true by +construction rather than by a rule. A key naming nothing the Board declares is a diagnostic, and it +blocks Publish rather than editing for the reason a stale `block.` does: renaming a variable is +ordinary building. + +**`value` is a Slot no manifest can type**, which makes `core.set_var` the third such verb beside a +call and a `core.return`. Its expected type is the named variable's, read where the variable is +declared — so a write that does not fit is the ordinary type diagnostic every other Slot already +produces, rather than a rule of its own. + A **Block** therefore declares `vars:` of its own, and `core.set_var` inside one writes those and can never reach out of the Board it is on. That is not a second concept: it is the same rule stated once, where "the Board" is the root for a Step in `steps:` and the Block for a Step inside one. @@ -214,6 +285,47 @@ The alternative was iteration state declared on `core.repeat` itself, initialise container, typed, and reset structurally on re-entry. It was rejected for costing a second concept where one already works. +## A variable's type is declared, because `core.set_var` made inference a lie + +`varType` read a variable's type off the literal beside it in the document, and the Workflow tab was +built on that: *"a variable field is the one input with no type marking"*. **That stops being true +the moment a Step can write the variable.** `value: ""` infers `text`; a `core.set_var` writing +`{{ 1 + 1 }}` into it makes the builder say `text` while the runner produces a number, and every +downstream answer — the type marking, the completion list's ranking, the Publish gate — was given +against a claim about one moment in an execution rather than about the variable. + +So **`vars` gains a required `t`, and an optional `of` for shape**, spelled exactly as a declaration +and a Run Context key are. The schema anticipated this in its own words — *"a list of key/value +objects rather than a map, so a `type` or `label` can be added later without a breaking change"* — +and ADR-0012's argument against inventing a second spelling for an idea the contract already has one +of holds here unchanged. A variable is still **not** a declaration: it carries a value, which no +declaration does, and its key is its own label, so three shared fields out of five is not one idea. + +`value` stops being the contract and becomes what it always was: the **initial** value. That is a +gain rather than a loss, because until `t` existed there was nothing to check an initial value +*against*, and a var seeded with `{{ … }}` was unchecked in both languages. + +Two alternatives were refused: + +- **Constrain `core.set_var` to the inferred type and report violations.** Keeps two mechanisms for + one idea, and makes the contract depend on how the first value happened to be written — a var + holding an object is unexpressible without an object literal, and an expression-valued var infers + `unknown`, so every write into it goes unchecked. +- **Weaken the inference to `unknown` for any var a `core.set_var` targets.** Makes the type marking + depend on a Step elsewhere in the document: adding a writer silently degrades every reader, so the + builder gets quieter exactly as the workflow gets more complicated. + +The cost is paid in full and is the one ADR-0014 already priced: **every existing document, fixture +and manifest is rewritten**, and `t` is required rather than defaulted, because a fallback spelling +is a second definition of the thing on the day it was declared. It also settles a divergence the +inference had no answer for — `yaml.v3` decodes `value: 2024-01-01T00:00:00Z` into a `time.Time` +while the builder's parser leaves it a string, so the two languages typed one scalar differently and +the Go SDK carried a comment saying so. A declared type is decoder-independent. + +The consequence for the builder is that the **type control**, not the value box, is what re-types +every Expression reading the variable. `CONTEXT.md`'s Slot entry and `docs/handoff.md`'s Workflow tab +are corrected to say so. + The cost is real and is documented rather than designed away: **a var written inside a loop survives into the next iteration of an enclosing loop**, so a workflow that must start each pass clean resets it explicitly. Nothing type-checks that reset. diff --git a/docs/handoff.md b/docs/handoff.md index 3fd3165..9e24c67 100644 --- a/docs/handoff.md +++ b/docs/handoff.md @@ -132,8 +132,9 @@ booleans. ### 3. Workflow variables -Rows of two mono `Input size="sm"` — key 118px, value flexible — plus a ghost trash button, then -`Button size="sm" variant="secondary" icon="plus"` **Add variable**. +Rows of a mono `Input size="sm"` for the key plus a ghost trash button, a full-width `Select` for +the declared type, and the value below, then `Button size="sm" variant="secondary" icon="plus"` +**Add variable**. **A variable's value is a Template**, not a literal. It may hold `{{ … }}`, so the value input is a [Template input](#the-template-input) like any other, and it gets the same completion. @@ -144,14 +145,29 @@ subset already exists inside `scopeFor`, which computes it before appending upst it as `boardScope(doc, board, manifests, runContext)` and let `scopeFor` be that plus the Steps, so the two readers share one definition. -**A variable field is the one input with no type marking**, because `varType` in `model/scope.ts` -infers a variable's type *from* its value. There is nothing to check it against. - -Editing a variable therefore changes what downstream Expressions type-check against. That is -correct, and it needs a test: change a variable from text to a number and a field reading it changes -verdict. It runs through `@hatua/expressions` with `scopeFor` output — not through the validation -store, which checks required fields, unknown components and malformed containers, and does no -expression type-checking at all. +**A variable declares its type**, read from `t` rather than from the value beside it, because +`core.set_var` writes the same variable from a Step — so the literal in the document is only what it +*starts* as, and a type inferred from it would be a claim about one moment in an execution +(ADR-0013). + +What that buys on this tab is the **completion list**, not a marking on the field: passing +`expectedType` is what lets the picker rail the candidate rows that fit, where a variable's value +input could rail none. Nothing is ever marked wrong — neutral covers "does not fit" and "cannot be +judged" alike — so the field itself looks the same either way. The declared type also shows beside +every `var.*` row in the reference tree, where an expression-valued variable previously read +`unknown`. + +**The type control is the one edit on the row that re-types downstream Expressions**, and the value +box is not. That needs a test on both halves: retyping `threshold` from number to text changes the +verdict of a number field reading it, and editing its value does not. It runs through +`@hatua/expressions` with `scopeFor` output — not through the validation store, which checks +required fields, unknown components and malformed containers, and does no expression type-checking +at all. + +A row's controls therefore map one to one onto commands: `renameVariable`, `setVariableType`, +`setVariableValue`, `removeVariable`. `addVariable` writes `t: text` rather than leaving it out, for +the reason it mints a key rather than leaving one blank — the schema requires it, so a row without +one is a document that stops projecting the moment it appears. #### Renaming a key @@ -221,13 +237,20 @@ names is an *undiscriminated container arm*, which this does not have. The substantial half of the builder, and the part the original handoff specified least. -Three places hold a Template, and all three use the same widget: +Four places hold a Template, and all four use the same widget: | Site | Where it is edited | | --- | --- | | A Step's mappable `with:` fields, including `map` entries | Step editor | | A Branch's `when` | Step editor, via its Fork | -| A workflow variable's value | Workflow tab | +| A `core.repeat`'s `until` | Step editor, via its Repeat | +| A variable's value | Workflow tab | + +The two conditions are one row twice over. Both are a structural key on a container rather than a +field under `with:`, so neither is reached through the **Component Manifest** and both are typed +`boolean` by the language — which is why they are edited through the container that owns them rather +than as a field of their own. Whatever surface holds one holds the other; a builder that could +author a repeat's condition and not a fork's would be a worse gap than having neither. ### The Template input diff --git a/source/conformance/definition/invalid/variable-key-is-not-an-identifier.yaml b/source/conformance/definition/invalid/variable-key-is-not-an-identifier.yaml index 7654410..d890c51 100644 --- a/source/conformance/definition/invalid/variable-key-is-not-an-identifier.yaml +++ b/source/conformance/definition/invalid/variable-key-is-not-an-identifier.yaml @@ -8,5 +8,6 @@ version: 1 status: draft vars: - key: digest-to + t: text value: me@dane.dev steps: [] diff --git a/source/conformance/definition/invalid/variable-type-is-not-a-declared-type.yaml b/source/conformance/definition/invalid/variable-type-is-not-a-declared-type.yaml new file mode 100644 index 0000000..372de44 --- /dev/null +++ b/source/conformance/definition/invalid/variable-type-is-not-a-declared-type.yaml @@ -0,0 +1,16 @@ +# expect: SCHEMA_INVALID +# +# A variable's type is what every `{{var.}}` read and every `core.set_var` +# write is checked against, so a type outside the declared set is not a narrower +# contract — it is no contract at all. `unknown` and `item` both match anything +# in the checker, so accepting either here would silently switch the type gate +# off for that variable while the builder still drew a marking beside it. +id: wf +name: W +version: 1 +status: draft +vars: + - key: attempt + t: unknown + value: 0 +steps: [] diff --git a/source/conformance/definition/rules/repeat-and-variables.yaml b/source/conformance/definition/rules/repeat-and-variables.yaml new file mode 100644 index 0000000..b67ca3e --- /dev/null +++ b/source/conformance/definition/rules/repeat-and-variables.yaml @@ -0,0 +1,326 @@ +about: >- + `core.repeat` and `core.set_var`, as both languages must report them. + + Two verbs Hatua interprets structurally and a manifest cannot describe. A + repeat's `until` is a key beside `steps:`, not a field under `with:`, because + `FIELD_KIND_TYPES` has no mappable boolean. A `core.set_var`'s `key` names one + of its own Board's `vars` and its `value` is typed by that var's declaration, + so neither field is a manifest's to declare either. + + The load-bearing pair is here: a repeat DISCHARGES a block's return obligation + and a `core.for_each` does not, because a repeat tests its condition after the + body and therefore always runs it once, while a list may be empty. Both + scenarios sit side by side so a language that collapses the two fails on one of + them. + +manifests: + - kind: component + use: component.email.send + name: Send email + fields: + - { k: to, label: To, kind: text, req: true } + outputs: [] + - kind: component + use: core.fork + name: Branch + fields: [] + outputs: [] + - kind: component + use: core.for_each + name: For each + fields: [] + outputs: [] + - kind: component + use: core.repeat + name: Repeat + fields: [] + outputs: [] + - kind: component + use: core.set_var + name: Set a variable + fields: [] + outputs: [] + - kind: component + use: core.return + name: Return + fields: [] + outputs: [] + - kind: trigger + use: core.schedule + name: Schedule + fields: [] + outputs: [] + +scenarios: + # ---- core.repeat ---------------------------------------------------------- + + - name: a repeat with a condition and a body reports nothing + definition: + id: wf + name: W + version: 1 + status: draft + steps: + - id: again + use: core.repeat + until: "{{ var.done }}" + steps: + - { id: s1, use: component.email.send, with: { to: "me@dane.dev" } } + vars: + - { key: done, t: boolean, value: false } + expect: [] + + - name: a repeat with no condition never ends, and says so + definition: + id: wf + name: W + version: 1 + status: draft + steps: + - id: again + use: core.repeat + steps: + - { id: s1, use: component.email.send, with: { to: "me@dane.dev" } } + expect: + - { code: REPEAT_HAS_NO_CONDITION, blocks: publish, stepId: again } + + - name: a condition of nothing but whitespace is no condition + definition: + id: wf + name: W + version: 1 + status: draft + steps: + - id: again + use: core.repeat + until: " " + steps: + - { id: s1, use: component.email.send, with: { to: "me@dane.dev" } } + expect: + - { code: REPEAT_HAS_NO_CONDITION, blocks: publish, stepId: again } + + - name: an empty repeat is both an empty body and a missing condition + definition: + id: wf + name: W + version: 1 + status: draft + steps: + - { id: again, use: core.repeat } + expect: + - { code: LOOP_HAS_NO_BODY, blocks: publish, stepId: again } + - { code: REPEAT_HAS_NO_CONDITION, blocks: publish, stepId: again } + + - name: an empty body is one code whichever loop verb it is + definition: + id: wf + name: W + version: 1 + status: draft + steps: + - { id: each, use: core.for_each } + - { id: again, use: core.repeat, until: "{{ var.done }}", steps: [] } + vars: + - { key: done, t: boolean, value: false } + expect: + - { code: LOOP_HAS_NO_BODY, blocks: publish, stepId: each } + - { code: LOOP_HAS_NO_BODY, blocks: publish, stepId: again } + + - name: a repeat's condition is not held to a manifest, so a repeat needs no fields + definition: + id: wf + name: W + version: 1 + status: draft + steps: + - id: again + use: core.repeat + until: "{{ var.done }}" + with: {} + steps: + - { id: s1, use: component.email.send, with: { to: "me@dane.dev" } } + vars: + - { key: done, t: boolean, value: false } + expect: [] + + # ---- a repeat and the return obligation ----------------------------------- + + - name: a repeat whose body always returns discharges the obligation, because the body always runs + definition: + id: wf + name: W + version: 1 + status: draft + steps: [] + blocks: + - id: ask + outputs: [{ k: answer, label: Answer, t: text }] + steps: + - id: again + use: core.repeat + until: "{{ var.done }}" + steps: + - { id: ret, use: core.return, with: { answer: "yes" } } + vars: + - { key: done, t: boolean, value: false } + expect: [] + + - name: a for_each whose body always returns does not, because the list may be empty + definition: + id: wf + name: W + version: 1 + status: draft + steps: [] + blocks: + - id: ask + outputs: [{ k: answer, label: Answer, t: text }] + steps: + - id: each + use: core.for_each + steps: + - { id: ret, use: core.return, with: { answer: "yes" } } + expect: + - { code: BLOCK_PATH_WITHOUT_RETURN, blocks: publish, blockId: ask } + + - name: a repeat whose body only sometimes returns discharges nothing + definition: + id: wf + name: W + version: 1 + status: draft + steps: [] + blocks: + - id: ask + outputs: [{ k: answer, label: Answer, t: text }] + steps: + - id: again + use: core.repeat + until: "{{ var.done }}" + steps: + - { id: s1, use: component.email.send, with: { to: "me@dane.dev" } } + vars: + - { key: done, t: boolean, value: false } + expect: + - { code: BLOCK_PATH_WITHOUT_RETURN, blocks: publish, blockId: ask } + + - name: nothing after a repeat that always returns can run + definition: + id: wf + name: W + version: 1 + status: draft + steps: [] + blocks: + - id: ask + outputs: [{ k: answer, label: Answer, t: text }] + steps: + - id: again + use: core.repeat + until: "{{ var.done }}" + steps: + - { id: ret, use: core.return, with: { answer: "yes" } } + - { id: after, use: component.email.send, with: { to: "me@dane.dev" } } + vars: + - { key: done, t: boolean, value: false } + expect: + - { code: STEP_AFTER_RETURN, blocks: publish, stepId: after, blockId: ask } + + # ---- core.set_var --------------------------------------------------------- + + - name: a set_var at the root writes the workflow's variable + definition: + id: wf + name: W + version: 1 + status: draft + vars: + - { key: attempt, t: number, value: 0 } + steps: + - { id: bump, use: core.set_var, with: { key: attempt, value: "{{ var.attempt }}" } } + expect: [] + + - name: a set_var naming a variable nothing declares blocks publish + definition: + id: wf + name: W + version: 1 + status: draft + vars: + - { key: attempt, t: number, value: 0 } + steps: + - { id: bump, use: core.set_var, with: { key: attemp, value: "1" } } + expect: + - { code: VAR_UNKNOWN, blocks: publish, stepId: bump, fieldKey: key } + + - name: a set_var inside a block writes that block's variable + definition: + id: wf + name: W + version: 1 + status: draft + steps: [] + blocks: + - id: ask + vars: + - { key: note, t: text, value: "" } + steps: + - { id: bump, use: core.set_var, with: { key: note, value: "again" } } + expect: [] + + - name: a set_var inside a block cannot reach the workflow's variables + definition: + id: wf + name: W + version: 1 + status: draft + vars: + - { key: attempt, t: number, value: 0 } + steps: [] + blocks: + - id: ask + vars: + - { key: note, t: text, value: "" } + steps: + - { id: bump, use: core.set_var, with: { key: attempt, value: "1" } } + expect: + - { code: VAR_UNKNOWN, blocks: publish, stepId: bump, blockId: ask, fieldKey: key } + + - name: a set_var with nothing to write and nowhere to write it reports both + definition: + id: wf + name: W + version: 1 + status: draft + vars: + - { key: attempt, t: number, value: 0 } + steps: + - { id: bump, use: core.set_var, with: {} } + expect: + - { code: FIELD_REQUIRED, blocks: publish, stepId: bump, fieldKey: key } + - { code: FIELD_REQUIRED, blocks: publish, stepId: bump, fieldKey: value } + + - name: a set_var whose value is false has a value, because false is an answer + definition: + id: wf + name: W + version: 1 + status: draft + vars: + - { key: done, t: boolean, value: false } + steps: + - { id: finish, use: core.set_var, with: { key: done, value: false } } + expect: [] + + - name: an unnamed variable is not resolved against the board, so only the missing key is reported + definition: + id: wf + name: W + version: 1 + status: draft + vars: + - { key: attempt, t: number, value: 0 } + steps: + - { id: bump, use: core.set_var, with: { value: "1" } } + expect: + - { code: FIELD_REQUIRED, blocks: publish, stepId: bump, fieldKey: key } diff --git a/source/conformance/definition/valid/blocks.yaml b/source/conformance/definition/valid/blocks.yaml index 37a746a..ded843f 100644 --- a/source/conformance/definition/valid/blocks.yaml +++ b/source/conformance/definition/valid/blocks.yaml @@ -19,6 +19,7 @@ triggers: vars: - key: digest_to + t: text value: me@dane.dev blocks: @@ -53,6 +54,7 @@ blocks: # here: anything a block needs from the call site arrives as a parameter. vars: - key: subject_line + t: text value: "Digest: {{ params.entry.headline }}" params: - k: entry diff --git a/source/conformance/definition/valid/full.yaml b/source/conformance/definition/valid/full.yaml index 3bbeef0..70c3259 100644 --- a/source/conformance/definition/valid/full.yaml +++ b/source/conformance/definition/valid/full.yaml @@ -35,8 +35,10 @@ triggers: vars: - key: triage_date + t: datetime value: "{{ TRIGGER == 'nightly' ? triggers.nightly.triggered_at : triggers.on_mail.received_at }}" - key: digest_to + t: text value: me@dane.dev steps: diff --git a/source/packages/document/src/round-trip.test.ts b/source/packages/document/src/round-trip.test.ts index f3e6394..ad08f13 100644 --- a/source/packages/document/src/round-trip.test.ts +++ b/source/packages/document/src/round-trip.test.ts @@ -56,6 +56,7 @@ blocks: - { k: url, label: "Archive URL", t: text } vars: - key: attempt_note + t: text value: "" steps: - id: put @@ -94,6 +95,59 @@ describe('a document with blocks', () => { }) }) +/* + * A repeat and a set_var: the two keys neither a manifest nor `with:` holds. An + * `until:` sits beside `steps:` and a var carries a declared `t:`, so both are + * structure the CST has to carry through untouched — a key silently dropped + * here is a condition or a type marking that vanishes the first time anything + * edits the file. + */ +const WITH_A_LOOP = `id: wf_review +name: "Revision loop" +version: 2 +status: draft + +vars: + # Reset by the loop body, which is the cost ADR-0013 documents. + - key: approved + t: boolean + value: false + +steps: + - id: revise + use: core.repeat + until: "{{ var.approved }}" # tested after the body, so it always runs once + steps: + - id: draft + use: component.agent.act + - id: record + use: core.set_var + with: { key: approved, value: "{{ steps.draft.approved }}" } +` + +describe('a document with a repeat and a set_var', () => { + it('reproduces it byte for byte, the condition and its comment included', () => { + expect(parseWorkflow(WITH_A_LOOP).toString()).toBe(WITH_A_LOOP) + }) + + it('projects the condition beside the body rather than into `with:`', () => { + const doc = parseWorkflow(WITH_A_LOOP).toJSON() + const repeat = doc.steps[0] + + expect(repeat?.until).toBe('{{ var.approved }}') + expect(repeat?.with).toBeUndefined() + expect(repeat?.steps?.map((step) => step.id)).toEqual(['draft', 'record']) + }) + + it('projects a variable’s declared type, which nothing infers', () => { + expect(parseWorkflow(WITH_A_LOOP).toJSON().vars?.[0]).toEqual({ + key: 'approved', + t: 'boolean', + value: false, + }) + }) +}) + describe('yaml layer fidelity', () => { // Pins the reason @hatua/document keeps the CST rather than the Document API // alone. If a future yaml release makes the AST byte-exact, this test fails diff --git a/source/packages/model/src/blocks.test.ts b/source/packages/model/src/blocks.test.ts index 6f9718a..b19ae9d 100644 --- a/source/packages/model/src/blocks.test.ts +++ b/source/packages/model/src/blocks.test.ts @@ -45,7 +45,7 @@ const doc = (overrides: Partial = {}): WorkflowDefinition => version: 1, status: 'draft', triggers: [{ id: 'nightly', use: 'core.schedule' }], - vars: [{ key: 'digest_to', value: 'me@dane.dev' }], + vars: [{ key: 'digest_to', t: 'text', value: 'me@dane.dev' }], steps: [], ...overrides, }) @@ -65,7 +65,7 @@ const ARCHIVE: WorkflowDefinition = doc({ }, ], outputs: [{ k: 'url', label: 'Archive URL', t: 'text' }], - vars: [{ key: 'attempt_note', value: '' }], + vars: [{ key: 'attempt_note', t: 'text', value: '' }], steps: [ { id: 'put', diff --git a/source/packages/model/src/fixtures.ts b/source/packages/model/src/fixtures.ts index daf8369..774829b 100644 --- a/source/packages/model/src/fixtures.ts +++ b/source/packages/model/src/fixtures.ts @@ -20,7 +20,7 @@ export const DOC: WorkflowDefinition = { with: { connection: 'mailbox' }, }, ], - vars: [{ key: 'digest_to', value: 'me@dane.dev' }], + vars: [{ key: 'digest_to', t: 'text', value: 'me@dane.dev' }], steps: [ { id: 's2', diff --git a/source/packages/model/src/generated/diagnostics.ts b/source/packages/model/src/generated/diagnostics.ts index 8facf0b..101e20f 100644 --- a/source/packages/model/src/generated/diagnostics.ts +++ b/source/packages/model/src/generated/diagnostics.ts @@ -14,6 +14,8 @@ export type DefinitionCode = | 'FORK_NEEDS_TWO_BRANCHES' | 'BRANCH_UNREACHABLE_AFTER' | 'LOOP_HAS_NO_BODY' + | 'REPEAT_HAS_NO_CONDITION' + | 'VAR_UNKNOWN' | 'BLOCK_UNKNOWN' | 'BLOCK_RECURSION' | 'RETURN_OUTSIDE_BLOCK' @@ -63,6 +65,16 @@ export const DEFINITION_DIAGNOSTICS: Record = {}): WorkflowDefinition => ({ + id: 'wf', + name: 'W', + version: 1, + status: 'draft', + steps: [], + ...over, +}) + +const setVar = (key: unknown, value: unknown): Step => ({ + id: 'bump', + use: 'core.set_var', + with: { key, value } as Record, +}) + +describe('a repeat’s condition', () => { + it('is a boolean, typed by the language rather than by a manifest', () => { + expect(repeatSlot('{{ var.done }}')).toEqual({ + name: 'until', + template: '{{ var.done }}', + expectedType: 'boolean', + }) + }) + + it('refuses a count where a condition belongs, which `with:` could not', () => { + const scope = scopeFor( + doc({ + vars: [{ key: 'seen', t: 'number', value: 0 }], + steps: [{ id: 'again', use: 'core.repeat', until: '{{ var.seen }}', steps: [] }], + }), + { board: null, id: 'again' }, + ) + const slot = repeatSlot('{{ var.seen }}') + + expect( + validate(slot.template, slot.expectedType, { scope, functions: coreFunctions() }), + ).not.toEqual([]) + expect( + validate('{{ var.seen > 3 }}', slot.expectedType, { scope, functions: coreFunctions() }), + ).toEqual([]) + }) +}) + +describe('what a set_var writes', () => { + const WORKFLOW = doc({ + vars: [{ key: 'attempt', t: 'number', value: 0 }], + steps: [setVar('attempt', '{{ 1 + 1 }}')], + }) + + it('is typed by the variable it names', () => { + expect(setVarSlot(WORKFLOW, null, WORKFLOW.steps[0] as Step)).toEqual({ + name: 'value', + template: '{{ 1 + 1 }}', + expectedType: 'number', + }) + }) + + it('is not a Slot at all when the board declares no such variable', () => { + expect(setVarSlot(WORKFLOW, null, setVar('attemp', '1'))).toBeNull() + expect(setVarSlot(WORKFLOW, null, setVar(undefined, '1'))).toBeNull() + }) + + it('is not a Slot when the value is a literal rather than a Template', () => { + expect(setVarSlot(WORKFLOW, null, setVar('attempt', 7))).toBeNull() + }) + + /** + * The end of the argument, from the document to a verdict. A var declared + * `boolean` refuses a number written into it, and the same document with `t: + * number` accepts it — so the marking the builder shows and the value the + * runner produces cannot disagree. + */ + it('is refused when it does not match the declaration, and accepted when it does', () => { + const asBoolean = doc({ + vars: [{ key: 'attempt', t: 'boolean', value: false }], + steps: [setVar('attempt', '{{ 1 + 1 }}')], + }) + const scope = scopeFor(asBoolean, { board: null, id: 'bump' }) + const wrong = setVarSlot(asBoolean, null, asBoolean.steps[0] as Step) + expect(wrong).not.toBeNull() + expect( + validate(wrong?.template ?? '', wrong?.expectedType ?? 'text', { + scope, + functions: coreFunctions(), + }), + ).toEqual([expect.objectContaining({ code: 'EXPR_TYPE_MISMATCH' })]) + + const right = setVarSlot(WORKFLOW, null, WORKFLOW.steps[0] as Step) + expect( + validate(right?.template ?? '', right?.expectedType ?? 'text', { + scope: scopeFor(WORKFLOW, { board: null, id: 'bump' }), + functions: coreFunctions(), + }), + ).toEqual([]) + }) + + /** + * A var declared on the wrong Board is out of reach rather than resolved + * differently, which is what makes `core.set_var` Board-scoped by + * construction rather than by a rule. + */ + it('cannot reach the workflow’s variables from inside a block', () => { + const inBlock = doc({ + vars: [{ key: 'attempt', t: 'number', value: 0 }], + blocks: [ + { + id: 'ask', + vars: [{ key: 'note', t: 'text', value: '' }], + steps: [setVar('attempt', '{{ 1 + 1 }}')], + }, + ], + }) + const block = inBlock.blocks?.[0] + expect(setVarSlot(inBlock, 'ask', block?.steps[0] as Step)).toBeNull() + }) +}) + +describe('a variable’s type', () => { + it('comes from its declaration, whatever the value beside it looks like', () => { + expect(variableType({ key: 'a', t: 'number', value: 'not a number' })).toBe('number') + expect(variableType({ key: 'a', t: 'boolean', value: '{{ run.tenant }}' })).toBe('boolean') + }) + + it('is unknown when nothing declares one, rather than guessed from the value', () => { + expect(variableType({ key: 'a', value: 7 } as never)).toBe('unknown') + // Empty and absent alike, matching the Go SDK. A `t: ""` reaching here is a + // hand-edit like a missing key is, and treating it as a type nothing + // declares would report a mismatch on every read of the variable. + expect(variableType({ key: 'a', t: '', value: 7 } as never)).toBe('unknown') + }) + + it('checks the initial value, which nothing could before it was declared', () => { + expect(variableSlot({ key: 'attempt', t: 'number', value: '{{ 1 + 1 }}' })).toEqual({ + name: 'attempt', + template: '{{ 1 + 1 }}', + expectedType: 'number', + }) + // A literal is not a Template, so there is no Slot and nothing to check. + expect(variableSlot({ key: 'attempt', t: 'number', value: 0 })).toBeNull() + }) + + it('reaches the checker through scope, so `of:` shapes a member read', () => { + const scope = scopeFor( + doc({ + vars: [ + { + key: 'entry', + t: 'object', + of: [{ k: 'headline', label: 'Headline', t: 'text' }], + value: '', + }, + ], + steps: [{ id: 's1', use: 'component.email.send' }], + }), + { board: null, id: 's1' }, + ) + + expect( + validate('{{ var.entry.headline }}', 'text', { scope, functions: coreFunctions() }), + ).toEqual([]) + expect( + validate('{{ var.entry.headline }}', 'number', { scope, functions: coreFunctions() }), + ).not.toEqual([]) + }) +}) diff --git a/source/packages/model/src/scope.ts b/source/packages/model/src/scope.ts index d739202..e30a893 100644 --- a/source/packages/model/src/scope.ts +++ b/source/packages/model/src/scope.ts @@ -1,4 +1,4 @@ -import type { TypeNode, ValueType } from '@hatua/expressions' +import type { TypeNode } from '@hatua/expressions' import type { Block, ContextKey, @@ -6,11 +6,12 @@ import type { Manifest, Output, Step, + Variable, WorkflowDefinition, } from '@hatua/schema' import { TRIGGER_BUILTIN } from '@hatua/schema' import { blockIdOf, blockOf } from './blocks' -import { MAPPING_VERB, mapEntries } from './slots' +import { MAPPING_VERB, mapEntries, variableType } from './slots' import { type BoardId, boardOf, type StepRef } from './tree' /** @@ -199,7 +200,7 @@ export function boardScope( path: `var.${variable.key}`, kind: 'var', label: variable.key, - type: { type: varType(variable.value) }, + type: variableToType(variable), }) } @@ -270,19 +271,19 @@ const declarationMembers = (declarations: readonly Declaration[]): Record ({ /** The verb whose outputs come from its own configuration. */ export const MAPPING_VERB = 'core.map' +/** The verb that repeats its children until a condition holds. */ +export const REPEAT_VERB = 'core.repeat' + +/** The verb that iterates a collection. */ +export const FOR_EACH_VERB = 'core.for_each' + +/** The verb that branches. */ +export const FORK_VERB = 'core.fork' + +/** The verb that writes one of its Board's variables. */ +export const SET_VAR_VERB = 'core.set_var' + +/** + * The Slot a `core.repeat`'s `until` resolves into. + * + * The mirror of `whenSlot`, and for the same reason: a condition is a boolean, + * and no manifest field can say so — `FIELD_KIND_TYPES` has no mappable boolean + * at all, because `bool` holds a literal rather than a Template. That is why + * `until` is a structural key beside `steps:` rather than a field under `with:`. + * Under `with:` it would type-check as text, so `{{ steps.s2.count }}` would + * pass as a termination condition. + * + * A repeat tests this AFTER its body, so the body always runs at least once. + */ +export const repeatSlot = (until: string): Slot => ({ + name: 'until', + template: until, + expectedType: 'boolean', +}) + +/** + * The type a variable's `{{ var. }}`, its initial `value` and every + * `core.set_var` writing it are all checked against. + * + * Declared rather than read off the value. A var is the one addressable thing + * whose content changes while the document does not, so inferring its type from + * the literal in the file would make the marking a lie the moment a + * `core.set_var` wrote something else — and every downstream check was answered + * against it (ADR-0013). + * + * `unknown` for a var carrying no `t` at all — absent or empty alike, matching + * the Go SDK, because a hand-edit is exactly what reaches here and `t: ""` is as + * plausible a one as a missing key. The schema requires a type, so refusing to + * check is the honest answer where guessing `text` would refuse a document over + * a type nothing declared. + */ +export const variableType = (variable: Variable): ValueType => + variable.t ? (variable.t as ValueType) : 'unknown' + +/** + * The Slot a `core.set_var`'s `value` resolves into, typed by the variable it + * names. + * + * The third verb a manifest cannot describe, alongside a call and a + * `core.return`, and for the same reason: what its field must produce is + * declared elsewhere in the document. Here it is the Board's `vars`, which is + * also why a `core.set_var` inside a Block can only ever name that Block's — + * `vars` is read from the Board the Step sits on, so there is no reaching out. + * + * Takes the Board rather than a list of variables, so the caller cannot supply + * the wrong one: the Go SDK's `SetVarSlot` has the same signature, and a runner + * handed a list would be the one deciding whether a Block falls back to the + * workflow's variables — which is the rule this verb exists inside. + * + * Null when the step names no variable, or names one the Board does not + * declare: both have their own diagnostic, and resolving a Template against a + * type nothing declared would report a mismatch the user cannot act on. + */ +export function setVarSlot(doc: WorkflowDefinition, board: BoardId, step: Step): Slot | null { + const values = (step.with ?? {}) as Record + + const key = own(values, 'key') + if (typeof key !== 'string') return null + + const variable = variableOn(doc, board, key) + if (!variable) return null + + const template = own(values, 'value') + if (typeof template !== 'string') return null + + return { name: 'value', template, expectedType: variableType(variable) } +} + +/** + * The Slot a variable's initial value resolves into. + * + * A var's `value` may hold `{{ … }}`, and until `t` was declared there was + * nothing to check it against. Null for a literal: only a Template is a Slot. + */ +export function variableSlot(variable: Variable): Slot | null { + if (typeof variable.value !== 'string') return null + return { name: variable.key, template: variable.value, expectedType: variableType(variable) } +} + /** The `{key, value, type}` entries of a `map` field, ignoring anything malformed. */ export function mapEntries(value: unknown): MapEntry[] { if (!Array.isArray(value)) return [] diff --git a/source/packages/model/src/tree.ts b/source/packages/model/src/tree.ts index 31e0421..52d1af6 100644 --- a/source/packages/model/src/tree.ts +++ b/source/packages/model/src/tree.ts @@ -1,4 +1,4 @@ -import type { Block, Step, WorkflowDefinition } from '@hatua/schema' +import type { Block, Step, Variable, WorkflowDefinition } from '@hatua/schema' /** * Pure domain rules over the step tree. No state, no I/O, no YAML — those live @@ -108,6 +108,28 @@ export function findStep(doc: WorkflowDefinition, ref: StepRef): Step | undefine return undefined } +/** + * The variables one Board declares: the workflow's at the root, a Block's inside + * one. + * + * This is the whole of "a `core.set_var` can never reach out of the Board it is + * on" — there is no second list to fall back to, so a Block naming a workflow + * variable is an unknown name rather than a scope a runner resolves differently. + * + * Exported because a runner has to answer the same question the builder does, + * and the Go SDK's `VarsOn` is this function: a rule restated at two call sites + * is two rules the day one of them gains a fallback. + */ +export const varsOn = (doc: WorkflowDefinition, board: BoardId): readonly Variable[] => + board === null ? (doc.vars ?? []) : (boardOf(doc, board)?.block?.vars ?? []) + +/** One Board's variable by key, or undefined when that Board declares none. */ +export const variableOn = ( + doc: WorkflowDefinition, + board: BoardId, + key: string, +): Variable | undefined => varsOn(doc, board).find((variable) => variable.key === key) + /** Every step id on one Board, for detecting references to steps that vanished. */ export function stepIds(doc: WorkflowDefinition, board: BoardId): Set { const found = boardOf(doc, board) diff --git a/source/packages/model/src/validity.ts b/source/packages/model/src/validity.ts index d1e11b9..6949204 100644 --- a/source/packages/model/src/validity.ts +++ b/source/packages/model/src/validity.ts @@ -2,7 +2,8 @@ import type { Block, Declaration, Manifest, Step, WorkflowDefinition } from '@ha import { blockIdOf, blockOf, cyclicBlocks, RETURN_VERB } from './blocks' import type { Diagnostic } from './connections' import { DEFINITION_DIAGNOSTICS, type DefinitionCode } from './generated/diagnostics' -import { type BoardId, boards, own, stepKey, walkDocument, walkSteps } from './tree' +import { FOR_EACH_VERB, FORK_VERB, REPEAT_VERB, SET_VAR_VERB } from './slots' +import { type BoardId, boards, own, stepKey, varsOn, walkDocument, walkSteps } from './tree' /** * Whether a Workflow Definition is filled in enough to run — the rules that read @@ -164,6 +165,19 @@ export function missingRequiredFields( continue } + if (step.use === SET_VAR_VERB) { + // Structural, for the reason a return's fields are: what a `core.set_var` + // takes is a var key and a value typed by the var that key names, and no + // manifest knows which Board a Step is on. Its manifest declares no + // fields at all, so without this a set_var with nothing in it reports + // nothing. + for (const [key, label] of SET_VAR_FIELDS) { + if (!unfilled(own(values, key))) continue + out.push(raise('FIELD_REQUIRED', { ...subject, fieldKey: key }, { label })) + } + continue + } + fromManifest(subject, step.use, values) } @@ -232,7 +246,7 @@ export function malformedContainers(doc: WorkflowDefinition): Diagnostic[] { for (const { step, board } of walkDocument(doc)) { const subject: Partial = { stepId: step.id, ...boardOn(board) } - if (step.use === 'core.fork') { + if (step.use === FORK_VERB) { const branches = step.branches ?? [] // CONTEXT.md defines a Fork as "holding two or more Branches". One branch // is not a fork — it is the same path with a condition on it, which is @@ -255,29 +269,72 @@ export function malformedContainers(doc: WorkflowDefinition): Diagnostic[] { } } - if (step.use === 'core.for_each' && (step.steps ?? []).length === 0) { + // One code for both loop verbs: the mistake is the same and so is the fix, + // and the message names neither. + if ( + (step.use === FOR_EACH_VERB || step.use === REPEAT_VERB) && + (step.steps ?? []).length === 0 + ) { out.push(raise('LOOP_HAS_NO_BODY', subject)) } + + // Read from the tree rather than from `with:`, because that is where it + // lives: `FIELD_KIND_TYPES` has no mappable boolean, so a condition under + // `with:` would type-check as text — see `repeatSlot`. + if (step.use === REPEAT_VERB && (step.until ?? '').trim() === '') { + out.push(raise('REPEAT_HAS_NO_CONDITION', subject)) + } + + if (step.use === SET_VAR_VERB) { + const key = own((step.with ?? {}) as Record, 'key') + // A missing key is FIELD_REQUIRED's to report. Resolving `undefined` + // against the Board would say no variable is called "undefined", which + // names a variable the user never wrote. + if (typeof key === 'string' && key !== '' && !varsOn(doc, board).some((v) => v.key === key)) { + out.push(raise('VAR_UNKNOWN', { ...subject, fieldKey: 'key' }, { name: key })) + } + } } return out } +/** + * The fields a `core.set_var` takes. Labels rather than keys in the message, + * matching every other required-field diagnostic — the sentence is read by + * someone looking at a form, not at YAML. + */ +const SET_VAR_FIELDS: readonly (readonly [string, string])[] = [ + ['key', 'Variable'], + ['value', 'Value'], +] + /** * Whether a step list, read from its own root level, always reaches a return. * * A `core.fork` discharges the obligation only when EVERY branch does, which is * the same all-paths reasoning `scopeFor` applies to what a Step can read. * - * A `core.for_each` never discharges it, and that is the load-bearing case: a - * return inside a loop body exits the Block early and is perfectly legal, but - * the list may be empty and the body may never run. That is the sibling-branch - * argument applied to time rather than to paths. + * The two loop verbs answer differently, and the difference is the whole rule. + * A `core.for_each` never discharges it: a return inside its body exits the + * Block early and is perfectly legal, but the list may be empty and the body + * may never run. A `core.repeat` does, because it tests its `until` after the + * body and therefore always runs it once. One question — is this region + * guaranteed to run at all — which is the sibling-branch argument applied to + * time rather than to paths. */ function alwaysReturns(steps: readonly Step[]): boolean { return steps.some((step) => { if (step.use === RETURN_VERB) return true - if (step.use !== 'core.fork') return false + + // A repeat tests its `until` AFTER the body, so the body always runs — and + // a region guaranteed to run discharges what it guarantees. This is the one + // line separating the two loop verbs, and it is the same question asked of + // both: a `core.for_each`'s list may be empty, a repeat's first pass is + // unconditional. + if (step.use === REPEAT_VERB) return alwaysReturns(step.steps ?? []) + + if (step.use !== FORK_VERB) return false const branches = step.branches ?? [] if (branches.length === 0) return false diff --git a/source/packages/react/package.json b/source/packages/react/package.json index c300baf..6f5467d 100644 --- a/source/packages/react/package.json +++ b/source/packages/react/package.json @@ -37,6 +37,7 @@ "@hatua/services": "workspace:*" }, "devDependencies": { + "@hatua/document": "workspace:*", "@storybook/react-vite": "^10.5.8", "@testing-library/react": "^16.3.0", "@types/react": "^19.2.2", diff --git a/source/packages/react/src/compounds/TemplateInput.stories.tsx b/source/packages/react/src/compounds/TemplateInput.stories.tsx index 76d5ce6..a0bb35e 100644 --- a/source/packages/react/src/compounds/TemplateInput.stories.tsx +++ b/source/packages/react/src/compounds/TemplateInput.stories.tsx @@ -350,8 +350,8 @@ export const TypeMarkingInsideMixedText: Story = { } /** - * A workflow variable: no type marking at all, because `varType` reads a - * variable's type *from* its value and there is nothing to check it against. + * A Template nothing declares a type for: no marking at all, because there is + * nothing to check against and a rail that is always neutral is at least honest. */ export const NoDeclaredType: Story = { args: { value: '{{ run. }}', route: 'shortcut-inside', caret: 7 }, diff --git a/source/packages/react/src/compounds/TemplateInput.tsx b/source/packages/react/src/compounds/TemplateInput.tsx index ba23cee..c63d324 100644 --- a/source/packages/react/src/compounds/TemplateInput.tsx +++ b/source/packages/react/src/compounds/TemplateInput.tsx @@ -100,9 +100,9 @@ export interface TemplateInputProps { /** * The type the field declares, and what the left rail judges against. * - * Undefined where nothing declares one — a workflow variable, whose type is - * read *from* its value — and then no row is ever marked. There is nothing to - * check against, and a rail that is always neutral is at least honest. + * Optional because a caller may hold a Template nothing declares a type for, + * and then no row is ever marked. There is nothing to check against, and a + * rail that is always neutral is at least honest. */ expectedType?: ValueType /** diff --git a/source/packages/react/src/compounds/insertion.ts b/source/packages/react/src/compounds/insertion.ts index 2baf8e9..7b00ff8 100644 --- a/source/packages/react/src/compounds/insertion.ts +++ b/source/packages/react/src/compounds/insertion.ts @@ -139,8 +139,8 @@ function tokenStart(written: string): number { * necessary is not a harmless conservatism — it is a green rail withheld from * a row that is exactly right, which is the only signal the rail carries. * - * Undefined when the field declares no type at all — a workflow variable, whose - * type is read *from* its value, so there is nothing to check it against. + * Undefined when the field declares no type at all, and then no candidate is + * ever railed: there is nothing to judge a row against. * * `[start, end)` is the range the insertion covers: the hole being edited, or * the caret. Whether what results is a whole-value Template is then one diff --git a/source/packages/react/src/layouts/Workflow.module.css b/source/packages/react/src/layouts/Workflow.module.css index 1fc6574..c0b0d92 100644 --- a/source/packages/react/src/layouts/Workflow.module.css +++ b/source/packages/react/src/layouts/Workflow.module.css @@ -195,6 +195,16 @@ min-inline-size: 0; } + /* + * Its own row between the key and the value, rather than a third column in + * the head: the head is already a name box that must stay wide enough to read + * a key in, and squeezing a type box in beside the bin button makes both + * unreadable at the panel's width. + */ + .varType { + inline-size: 100%; + } + .add { display: grid; grid-template-columns: minmax(0, 1fr) auto; diff --git a/source/packages/react/src/layouts/Workflow.stories.tsx b/source/packages/react/src/layouts/Workflow.stories.tsx index 4938606..518b9f9 100644 --- a/source/packages/react/src/layouts/Workflow.stories.tsx +++ b/source/packages/react/src/layouts/Workflow.stories.tsx @@ -57,12 +57,16 @@ triggers: vars: # Where the digest goes. - key: digest_to + t: text value: "ops@example.com" - key: subject_prefix + t: text value: "[triage]" - key: threshold + t: number value: 10 - key: greeting + t: text value: "Morning, {{ triggers.t1.owner }}" steps: diff --git a/source/packages/react/src/layouts/Workflow.test.tsx b/source/packages/react/src/layouts/Workflow.test.tsx index ff325ee..d715ca9 100644 --- a/source/packages/react/src/layouts/Workflow.test.tsx +++ b/source/packages/react/src/layouts/Workflow.test.tsx @@ -44,8 +44,10 @@ triggers: vars: # Where the digest goes. - key: digest_to + t: text value: "ops@example.com" - key: threshold + t: number value: 10 steps: @@ -834,10 +836,10 @@ describe('variables', () => { expect(source.writes[0]).toContain('{{ var.digest_to }}') }) - it('stores a value as what the text denotes, so the type follows the value', async () => { - // `varType` reads a variable's type off its value, so this box is also a - // type control — and typing `25` here has to mean the same as typing it in - // Text Mode (ADR-0001). + it('stores a value as what the text denotes, so Text Mode and this box agree', async () => { + // The type comes from `t`, but the value box still writes the scalar the + // text denotes: typing `25` here has to mean what typing it in Text Mode + // means (ADR-0001). const source = host() mount(source) @@ -846,6 +848,36 @@ describe('variables', () => { expect(source.writes[0]).toContain('value: 25') }) + /* + * The type control, which is the one edit on the row that re-types every + * Expression reading the variable. The value box does not: `core.set_var` + * writes the same variable from a Step, so the literal in the document is + * only what it starts as (ADR-0013). + */ + it('shows each variable’s declared type, and writes a change to it', async () => { + const source = host() + mount(source) + + const control = (await screen.findByLabelText('Type of threshold')) as HTMLSelectElement + expect(control.value).toBe('number') + + fireEvent.change(control, { target: { value: 'text' } }) + await waitFor(() => expect(source.writes).toHaveLength(1), AUTOSAVED) + expect(source.writes[0]).toContain('t: text') + // The value beside it is untouched, because the two say different things. + expect(source.writes[0]).toContain('value: 10') + }) + + it('gives a new variable a type, because the schema requires one', async () => { + const source = host() + mount(source) + + fireEvent.click(await screen.findByRole('button', { name: 'Add variable' })) + await waitFor(() => expect(source.writes).toHaveLength(1), AUTOSAVED) + expect(source.writes[0]).toContain('key: new_variable') + expect(source.writes[0]).toContain('t: text') + }) + it('stores a Template as a Template, holes and all', async () => { const source = host() mount(source) diff --git a/source/packages/react/src/layouts/Workflow.tsx b/source/packages/react/src/layouts/Workflow.tsx index 9b5574d..4beb951 100644 --- a/source/packages/react/src/layouts/Workflow.tsx +++ b/source/packages/react/src/layouts/Workflow.tsx @@ -20,6 +20,7 @@ import { sequence, setTriggerField, setTriggerName, + setVariableType, setVariableValue, setWorkflowName, setWorkflowSlug, @@ -278,6 +279,7 @@ export function Workflow({ className, ...rest }: WorkflowProps) { onAdd={() => store?.apply(addVariable())} onRemove={(key) => store?.apply(removeVariable(key))} onRename={(from, to) => store?.apply(renameVariable(from, to))} + onType={(key, t) => store?.apply(setVariableType(key, t))} onValue={(key, value) => store?.apply(setVariableValue(key, value))} /> @@ -570,13 +572,23 @@ function TriggerCard({ ) } +/** The types a variable may declare, in the order the schema lists them. */ +const VARIABLE_TYPES = ['text', 'number', 'boolean', 'datetime', 'object', 'list'] as const + /** - * The workflow's variables: a key and a Template, per row. + * The workflow's variables: a key, a declared type and a Template, per row. + * + * **The type is declared, not read off the value.** `core.set_var` writes the + * same variable from a Step, which is what makes the literal in the document its + * FIRST value rather than its contract — a type read off it would be a claim + * about one moment in an execution (ADR-0013). The type control is therefore the + * one edit on this row that re-types every Expression reading the variable, and + * the value box is not. * - * **A variable field is the one input with no type marking**, because `varType` - * infers a variable's type *from* its value. There is nothing to check it - * against — and editing one therefore changes what every downstream Expression - * reading it type-checks against, which is correct. + * The declared type reaches the screen through the value box's completion list + * rather than through the box itself: it is what lets the picker rail the + * candidate rows that fit, where a variable's value could rail none. Nothing is + * ever marked wrong, so the field looks the same either way. * * **Renaming a key does not rewrite References.** `{{ var.old_name }}` goes * stale and the checker reports it, exactly as it does for a Step that was @@ -590,6 +602,7 @@ function Variables({ onAdd, onRemove, onRename, + onType, onValue, }: { variables: readonly Variable[] @@ -597,6 +610,7 @@ function Variables({ onAdd: () => void onRemove: (key: string) => void onRename: (from: string, to: string) => void + onType: (key: string, t: string) => void onValue: (key: string, value: string) => void }) { return ( @@ -640,6 +654,18 @@ function Variables({ + onValue(variable.key, next)} /> diff --git a/source/packages/react/src/layouts/stories.fixtures.test.ts b/source/packages/react/src/layouts/stories.fixtures.test.ts new file mode 100644 index 0000000..1fd1a35 --- /dev/null +++ b/source/packages/react/src/layouts/stories.fixtures.test.ts @@ -0,0 +1,81 @@ +/** biome-ignore-all lint/correctness/noNodejsModules: a Node test reading story sources from disk; nothing here ships to a browser. */ +import { readdirSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import { parseWorkflow } from '@hatua/document' +import { describe, expect, it } from 'vitest' + +/** + * Every Workflow Definition a story hands a region has to project. + * + * A story fixture is the only document in the repo nothing executes: Storybook + * renders it by hand and no test mounts it, so a fixture that stops satisfying + * the schema does not fail — the region simply falls back to the state it shows + * a Host that wired nothing up, and every story quietly draws the wrong screen. + * That is invisible in review precisely because the file still compiles. + * + * A field added to the schema is what makes this reachable: it lands in the + * fixtures under test and in the fixtures under `stories`, and only the first + * set has anything watching it. + * + * Through `@hatua/document` rather than the schema directly, because that is the + * path a region's document actually takes — the editing store parses the Host's + * text and hands the region `projection.success ? projection.data : null`, and + * `null` is what draws the wrong screen. A test-only dependency: nothing under + * `src/` outside this file imports it, and the layering rule in this + * directory's README still stands. + */ + +const LAYOUTS = import.meta.dirname + +/** + * Template literals holding a workflow document, by the one marker every one of + * them carries: a top-level `id:` before any nesting. Narrow on purpose — a + * story may hold YAML that is a manifest or a fragment, and neither is this + * schema's to judge. + */ +function documentsIn(source: string): string[] { + const found: string[] = [] + for (const match of source.matchAll(/`([^`]*)`/g)) { + const body = unescaped(match[1] ?? '') + if (/^(?:#[^\n]*\n|\s*\n)*id:\s*\S/.test(body)) found.push(body) + } + return found +} + +/** + * What the compiler would have made of the literal. + * + * Read from source rather than from the module, so the escapes are still text: + * a fixture written `id: wf\nsteps: []` holds two characters where the running + * story holds a newline, and feeding that to a YAML parser fails for a reason + * that has nothing to do with the fixture. One pass, so a literal backslash + * cannot be re-read as the start of the next escape. + */ +const unescaped = (body: string): string => + body.replace(/\\([nrt\\`$])/g, (_, char: string) => + char === 'n' ? '\n' : char === 'r' ? '\r' : char === 't' ? '\t' : char, + ) + +describe('story fixtures', () => { + const files = readdirSync(LAYOUTS).filter((name) => name.endsWith('.stories.tsx')) + expect(files.length).toBeGreaterThan(0) + + const documents = files.flatMap((file) => + documentsIn(readFileSync(join(LAYOUTS, file), 'utf8')).map( + (source, index) => [`${file} · document ${index + 1}`, source] as const, + ), + ) + + // A guard that found nothing is a guard that protects nothing. + it('finds the documents the stories hold', () => { + expect(documents.length).toBeGreaterThan(0) + }) + + for (const [name, source] of documents) { + it(`${name} projects`, () => { + const projected = parseWorkflow(source).validate() + expect(projected.error?.issues).toBeUndefined() + expect(projected.success).toBe(true) + }) + } +}) diff --git a/source/packages/schema/src/contract.test.ts b/source/packages/schema/src/contract.test.ts index b156437..ee42ec9 100644 --- a/source/packages/schema/src/contract.test.ts +++ b/source/packages/schema/src/contract.test.ts @@ -21,7 +21,7 @@ const DEFINITION = { with: { cron: '0 7 * * 1-5' }, }, ], - vars: [{ key: 'digest_to', value: 'me@dane.dev' }], + vars: [{ key: 'digest_to', t: 'text', value: 'me@dane.dev' }], steps: [ { id: 's2', diff --git a/source/packages/schema/src/generated/definition.ts b/source/packages/schema/src/generated/definition.ts index 393336d..5790646 100644 --- a/source/packages/schema/src/generated/definition.ts +++ b/source/packages/schema/src/generated/definition.ts @@ -113,14 +113,26 @@ export const trigger = z.strictObject({ export type Trigger = z.infer /** - * A list of key/value objects rather than a map, so a `type` or `label` can be added later without a breaking change to every existing file. + * A list of key/value objects rather than a map, which is what let `t` be added here without inventing a second spelling for a variable. + * NOT a `declaration`. A declaration is a contract with nothing in it; a variable carries a value, and its key is its own label — the builder shows `var.digest_to`, not a friendly name. `t` and `of` are spelled identically to a declaration's so one function reads both, but three shared fields out of five is not one idea. */ export const variable = z.strictObject({ get key() { return identifier }, /** - * A literal, or an expression evaluated by the SDK's shared evaluator. + * The type every `{{var.}}` is checked against, and the type `core.set_var` must write. Declared rather than inferred from `value`, because `value` is only the FIRST value: a `core.set_var` writing a number into a var that started as `""` would make an inferred `text` a lie, and every downstream type check was answered against it. Required rather than defaulted, for the reason ADR-0014 rewrote every document at once — a fallback spelling is a second definition of the thing on the day it was declared. + * `item` is absent, for the reason it is absent from a declaration: it is the for-each escape hatch, resolved by following a loop's `list` back to its source output, and a variable is not the output of anything. + */ + t: z.enum(['text', 'number', 'boolean', 'datetime', 'object', 'list']), + /** + * Shape of each list element or object member, spelled as a declaration's `of` is. + */ + get of() { + return z.array(declaration).optional() + }, + /** + * The initial value: a literal, or an expression evaluated by the SDK's shared evaluator. Checked against `t` like any other Slot when it is a Template. */ value: z.unknown(), }) @@ -136,7 +148,8 @@ export const step = z.strictObject({ }, /** * The manifest verb. Its root says who declares it and there are only three: `core.` is Hatua's, `component.` is the Host's, `block.` names a Block in this document. Nothing sits at the root itself, so a Host may declare `component.block.render` and collide with nothing — see ADR-0014. - * Hatua treats most verbs as opaque, but interprets three structurally: `core.fork` creates branches, `core.for_each` nests and exposes `item`, and `core.map` derives its outputs from its own `entries` field rather than from its manifest. The first two drive reference scope and derived layout; the third drives reference scope alone. + * Hatua treats most verbs as opaque, but interprets five structurally: `core.fork` creates branches, `core.for_each` nests and exposes `item`, `core.repeat` nests and carries an `until:` condition, `core.map` derives its outputs from its own `entries` field rather than from its manifest, and `core.set_var` writes one of the Board's `vars` and is typed by that var's declaration rather than by a manifest field. + * The three nesting verbs drive reference scope and derived layout; `core.map` drives reference scope alone; `core.set_var` drives neither and is here because its `value` is a Slot no manifest can type. * `core.map` is the one component whose outputs a manifest cannot declare, because they are whatever the user named. Its `with.entries` is a list of `{key, value, type}`, and a downstream step addresses them as `{{steps..}}` with the declared type — which is what lets the type checker treat a mapping step exactly like any other. */ use: z.string().min(1), @@ -151,11 +164,17 @@ export const step = z.strictObject({ return z.array(branch).min(1).optional() }, /** - * Loop children, nested directly with no branch wrapper. Only on `core.for_each`. + * Loop children, nested directly with no branch wrapper. On `core.for_each` and `core.repeat`. */ get steps() { return z.array(step).optional() }, + /** + * A `core.repeat`'s termination condition, and only meaningful there. The body runs, then this is evaluated; false runs it again. So a repeat ALWAYS runs its children at least once — which is what lets one discharge a block's return obligation where a `core.for_each` cannot, and what makes a pre-tested loop expressible as a body that starts with a fork while the reverse costs a duplicated body. + * A structural key beside `steps:` rather than a field under `with:`, for the reason a Branch's `when` is one: a manifest field carries a rendering `kind` and no type, so the expected type is recovered from `FIELD_KIND_TYPES` — and that vocabulary cannot express "a Template that must produce a boolean" at all. Under `with:` the condition would type-check as text, which is the whole half of the contract it exists to carry. + * Nothing bounds the iterations, and that is deliberate rather than missing: whether an `until` ever goes false depends on run-time values, so unlike recursion it is not a property the document has. Hatua does not execute, so a bound written here would be a number no reader could enforce. Bounding is the Host runner's obligation — see ADR-0013. + */ + until: z.string().optional(), }) export type Step = z.infer @@ -215,6 +234,7 @@ export const workflowDefinition = z.strictObject({ }, /** * Mutable workflow-scoped state, readable as `{{var.}}` and written by `core.set_var`. Distinct from trigger payloads, which arrive from outside. A value may be a literal or an expression, so a var can normalise differently-shaped trigger payloads into one shape. + * Its type is declared, never read off that value — a `core.set_var` writing it later means the value in the file is an initial value rather than a contract. See ADR-0013. */ get vars() { return z.array(variable).optional() diff --git a/source/packages/services/src/blocks.test.ts b/source/packages/services/src/blocks.test.ts index 1c98abd..c195f60 100644 --- a/source/packages/services/src/blocks.test.ts +++ b/source/packages/services/src/blocks.test.ts @@ -40,6 +40,7 @@ triggers: vars: - key: digest_to + t: text value: "ops@example.com" steps: @@ -77,6 +78,7 @@ triggers: vars: - key: digest_to + t: text value: "ops@example.com" blocks: - id: archive_entry @@ -288,10 +290,12 @@ describe('a block’s own variables', () => { setVariableValue('attempt_note', 'first pass', 'archive'), ) - expect(projected(out).blocks?.[0]?.vars).toEqual([{ key: 'attempt_note', value: 'first pass' }]) + expect(projected(out).blocks?.[0]?.vars).toEqual([ + { key: 'attempt_note', t: 'text', value: 'first pass' }, + ]) // The workflow's own list is untouched, which is the lifetime difference: // a block's vars are rebuilt on every invocation. - expect(projected(out).vars).toEqual([{ key: 'digest_to', value: 'ops@example.com' }]) + expect(projected(out).vars).toEqual([{ key: 'digest_to', t: 'text', value: 'ops@example.com' }]) }) it('puts `vars:` before `steps:` inside the block', () => { diff --git a/source/packages/services/src/index.test.ts b/source/packages/services/src/index.test.ts index 31c6eff..6fcf323 100644 --- a/source/packages/services/src/index.test.ts +++ b/source/packages/services/src/index.test.ts @@ -62,6 +62,7 @@ const SURFACE = [ 'addVariable', 'removeVariable', 'renameVariable', + 'setVariableType', 'setVariableValue', ].sort() diff --git a/source/packages/services/src/variables.test.ts b/source/packages/services/src/variables.test.ts index 02cb09b..f53069a 100644 --- a/source/packages/services/src/variables.test.ts +++ b/source/packages/services/src/variables.test.ts @@ -2,7 +2,13 @@ import { parseWorkflow } from '@hatua/document' import { coreFunctions, validate } from '@hatua/expressions' import { scopeFor } from '@hatua/model' import { describe, expect, it } from 'vitest' -import { addVariable, removeVariable, renameVariable, setVariableValue } from './variables' +import { + addVariable, + removeVariable, + renameVariable, + setVariableType, + setVariableValue, +} from './variables' /** * The variable commands against a document directly. @@ -10,9 +16,9 @@ import { addVariable, removeVariable, renameVariable, setVariableValue } from '. * Two things are being protected. The first is the round trip: a variable is * added, renamed and removed out of a file that lives in the Host's repository, * and the comments, key order and quoting around it come back untouched - * (ADR-0001). The second is the consequence of editing one — `varType` reads a - * variable's type off its value, so a value box is also a type control, and the - * last test here follows that all the way to a verdict. + * (ADR-0001). The second is which edit re-types a variable: `t` is declared, so + * the type control moves every downstream verdict and the value box moves none, + * and the last tests here follow both all the way to a verdict. */ const SOURCE = `# The overnight triage. @@ -24,8 +30,10 @@ status: draft vars: # Where the digest goes. - key: digest_to + t: text value: "ops@example.com" - key: threshold + t: number value: 10 steps: @@ -186,11 +194,12 @@ describe('setVariableValue', () => { }) }) -describe('editing a variable changes what an Expression checks against', () => { +describe('retyping a variable changes what an Expression checks against', () => { /** - * The consequence of `varType`: a variable is the one addressable thing with - * no declaration to consult, so its type is read off its value. Editing one - * therefore re-types every Expression that reads it. + * The consequence of declaring `t`: the type control is what re-types every + * Expression reading the variable, and the value box is not — because a + * `core.set_var` writes the same variable from a Step, so the literal in the + * document is only what it starts as. * * Through `@hatua/expressions` over `scopeFor` output, which is where * expression type-checking happens. The validation store does none — it @@ -206,19 +215,22 @@ describe('editing a variable changes what an Expression checks against', () => { } it('goes from clean to reported when a number field starts reading text', () => { - const asNumber = apply(SOURCE, setVariableValue('threshold', '25')).toString() - expect(verdicts(asNumber, '{{ var.threshold }}', 'number')).toEqual([]) + expect(verdicts(SOURCE, '{{ var.threshold }}', 'number')).toEqual([]) - const asText = apply(SOURCE, setVariableValue('threshold', 'twenty five')).toString() + const asText = apply(SOURCE, setVariableType('threshold', 'text')).toString() expect(verdicts(asText, '{{ var.threshold }}', 'number')).not.toEqual([]) }) - it('clears the report when the value is edited back, in the same field', () => { - const asText = apply(SOURCE, setVariableValue('threshold', 'twenty five')).toString() - const reported = verdicts(asText, '{{ var.threshold }}', 'number') - expect(reported).toHaveLength(1) + it('clears the report when the type is set back, in the same control', () => { + const asText = apply(SOURCE, setVariableType('threshold', 'text')).toString() + expect(verdicts(asText, '{{ var.threshold }}', 'number')).toHaveLength(1) - const repaired = apply(asText, setVariableValue('threshold', '25')).toString() + const repaired = apply(asText, setVariableType('threshold', 'number')).toString() expect(verdicts(repaired, '{{ var.threshold }}', 'number')).toEqual([]) }) + + it('leaves the marking alone when only the value changes, because the value is only the first one', () => { + const written = apply(SOURCE, setVariableValue('threshold', 'twenty five')).toString() + expect(verdicts(written, '{{ var.threshold }}', 'number')).toEqual([]) + }) }) diff --git a/source/packages/services/src/variables.ts b/source/packages/services/src/variables.ts index 730250c..9709cd2 100644 --- a/source/packages/services/src/variables.ts +++ b/source/packages/services/src/variables.ts @@ -32,11 +32,12 @@ import type { EditCommand } from './command' * earlier variables, never a Step's outputs, because no Step is guaranteed to * have run. * - * Its *type* follows from what is stored: `varType` in @hatua/model reads a - * variable's type off its value, because a variable is the one addressable - * thing with no declaration to consult. So editing one changes what every - * downstream Expression reading it type-checks against, which is correct and is - * the reason `variables.test.ts` asserts it end to end. + * Its *type* is declared, in `t`, and is the one thing here that re-types every + * downstream Expression. The value box does not: `value` is only the FIRST + * value, because `core.set_var` writes the same variable from a Step, so a type + * read off the literal in the document would be a claim about one moment rather + * than about the variable (ADR-0013). `setVariableType` is therefore a command + * of its own, and `variables.test.ts` follows it to a verdict. */ /** @@ -91,7 +92,16 @@ export function addVariable(key?: string, board: BoardId = null): EditCommand { const listPath = ensureVars(document, board) const list = readAt(document, listPath) const index = Array.isArray(list) ? list.length : 0 - const value: Record = { key: key ?? mintKey(document, board), value: '' } + // `t` is written rather than left out, for the reason the key is minted + // rather than left blank: the schema requires it, so a row without one is + // a document that stops projecting the moment it appears. `text` because + // it is the type an empty value is, and it is the one every other type + // can be typed over from the row's control. + const value: Record = { + key: key ?? mintKey(document, board), + t: 'text', + value: '', + } insertNode(document, listPath, index, document.ast.createNode(value)) }, } @@ -147,6 +157,24 @@ export function renameVariable(from: string, to: string, board: BoardId = null): } } +/** + * Write a variable's declared type. + * + * The type control, and the only edit here that changes what a Template reading + * `{{ var. }}` is checked against. Separate from the value box because the + * two answer different questions once a `core.set_var` can write the variable: + * the value box says what it starts as, and this says what it must always be. + */ +export function setVariableType(key: string, t: string, board: BoardId = null): EditCommand { + return { + label: `Retype ${key}`, + apply(document) { + const listPath = varsPath(document, board) + setScalar(document, [...listPath, locateVariable(document, board, key), 't'], t) + }, + } +} + /** * The scalar a line of typed text denotes, by YAML's own rules. * @@ -154,8 +182,7 @@ export function renameVariable(from: string, to: string, board: BoardId = null): * Template, because `{{ … }}` is not a YAML scalar form — is text. That is not * a convenience: the same text typed into Text Mode produces exactly these * values, and a Workflow Definition edited two ways has to mean one thing - * (ADR-0001). It is also what makes the type marking move, since `varType` - * reads a variable's type off the value stored here. + * (ADR-0001). * * Only what round-trips. `007` and `1e400` are left as text rather than * normalised to `7` and `Infinity`, because rewriting what the user typed is @@ -170,13 +197,14 @@ const scalarFor = (text: string): string | number | boolean => { } /** - * Write a variable's value, as the Template it is. + * Write a variable's initial value, as the Template it is. * * `setIn` rather than an assignment onto the existing scalar, because this is * the one field whose *style* is part of its meaning: `value: "7"` is text and - * `value: 7` is a number, so keeping the quoting the previous value was written - * in would make a variable that starts out quoted impossible to turn into a - * number. Everywhere else the quoting is the user's and stays. + * `value: 7` is a number. What it is checked against comes from `t` rather than + * from the quoting, but a document round-trips what the user typed, and + * silently requoting a number as a string is the one thing a value box must not + * do. */ export function setVariableValue( key: string, diff --git a/source/pnpm-lock.yaml b/source/pnpm-lock.yaml index f95bdf0..29e3314 100644 --- a/source/pnpm-lock.yaml +++ b/source/pnpm-lock.yaml @@ -117,6 +117,9 @@ importers: specifier: workspace:* version: link:../services devDependencies: + '@hatua/document': + specifier: workspace:* + version: link:../document '@storybook/react-vite': specifier: ^10.5.8 version: 10.5.8(@types/react-dom@19.2.4(@types/react@19.2.18))(@types/react@19.2.18)(esbuild@0.28.2)(react-dom@19.2.8(react@19.2.8))(react@19.2.8)(rollup@4.62.4)(storybook@10.5.8(@types/react@19.2.18)(react@19.2.8))(typescript@7.0.2)(vite@8.2.1(@types/node@26.2.0)(esbuild@0.28.2)(yaml@2.9.0)) diff --git a/source/schemas/definition-diagnostics.yaml b/source/schemas/definition-diagnostics.yaml index 6662faa..a9deb7d 100644 --- a/source/schemas/definition-diagnostics.yaml +++ b/source/schemas/definition-diagnostics.yaml @@ -67,7 +67,37 @@ codes: - code: LOOP_HAS_NO_BODY blocks: publish message: "This loop repeats nothing. Add at least one Step inside it." - summary: A `core.for_each` with no children. + summary: >- + A `core.for_each` or a `core.repeat` with no children. One code rather than + two: the mistake is the same and so is the fix, and the message names + neither verb. + + - code: REPEAT_HAS_NO_CONDITION + blocks: publish + message: "This repeat has no condition, so nothing ever ends it. Give it an `until`." + summary: >- + A `core.repeat` whose `until` is absent or blank. Structural rather than a + required field, because the condition is a structural key beside `steps:` + and not something a manifest declares — the same position a Branch's + `when` holds, and for the same reason: `FIELD_KIND_TYPES` cannot express + "a Template that must produce a boolean". + + Publish-blocking rather than edit-blocking, because a repeat a second after + it is added is exactly this. + + - code: VAR_UNKNOWN + blocks: publish + message: 'No variable called "{name}" is declared on this board.' + summary: >- + A `core.set_var` naming a var its Board does not declare. The BLOCK_UNKNOWN + argument one level down: a var key goes stale the moment someone renames or + removes it, which is ordinary building, so this blocks Publish and never + editing. + + "on this board" is the load-bearing half of the message. A `core.set_var` + inside a block writes that block's vars and can never reach the workflow's, + so naming a workflow var from inside a block is this diagnostic and not a + scoping accident the runner discovers. # ---- blocks --------------------------------------------------------------- @@ -105,10 +135,14 @@ codes: A path returns via a `core.return` at the Board's root level, or a root-level fork that is exhaustive — an unconditional last branch — and whose every branch returns. A fork whose every branch carries a `when` is - not exhaustive: first-match-wins means it can match none of them. A return inside a loop exits - the block early and is legal, but never discharges the obligation: the list - may be empty and the body may never run — the same reasoning that keeps - sibling branches out of scope, applied to time instead of to paths. + not exhaustive: first-match-wins means it can match none of them. + + A `core.repeat` DOES discharge it, and a `core.for_each` never does. That is + one rule rather than two special cases: a repeat tests its `until` after the + body, so the body always runs, while a for_each's list may be empty. The + question is only ever "is this region guaranteed to run at all" — the same + reasoning that keeps sibling branches out of scope, applied to time instead + of to paths. - code: STEP_AFTER_RETURN blocks: publish diff --git a/source/schemas/workflow-definition.schema.yaml b/source/schemas/workflow-definition.schema.yaml index e327768..78c4a3d 100644 --- a/source/schemas/workflow-definition.schema.yaml +++ b/source/schemas/workflow-definition.schema.yaml @@ -69,6 +69,9 @@ properties: Mutable workflow-scoped state, readable as `{{var.}}` and written by `core.set_var`. Distinct from trigger payloads, which arrive from outside. A value may be a literal or an expression, so a var can normalise differently-shaped trigger payloads into one shape. + + Its type is declared, never read off that value — a `core.set_var` writing it later means + the value in the file is an initial value rather than a contract. See ADR-0013. items: { $ref: "#/$defs/variable" } blocks: @@ -228,15 +231,39 @@ $defs: variable: type: object additionalProperties: false - required: [key, value] + required: [key, t, value] description: >- - A list of key/value objects rather than a map, so a `type` or `label` can be added later - without a breaking change to every existing file. + A list of key/value objects rather than a map, which is what let `t` be added here without + inventing a second spelling for a variable. + + NOT a `declaration`. A declaration is a contract with nothing in it; a variable carries a + value, and its key is its own label — the builder shows `var.digest_to`, not a friendly + name. `t` and `of` are spelled identically to a declaration's so one function reads both, + but three shared fields out of five is not one idea. properties: key: $ref: "#/$defs/identifier" + t: + enum: [text, number, boolean, datetime, object, list] + description: >- + The type every `{{var.}}` is checked against, and the type `core.set_var` must + write. Declared rather than inferred from `value`, because `value` is only the FIRST + value: a `core.set_var` writing a number into a var that started as `""` would make an + inferred `text` a lie, and every downstream type check was answered against it. Required + rather than defaulted, for the reason ADR-0014 rewrote every document at once — a + fallback spelling is a second definition of the thing on the day it was declared. + + `item` is absent, for the reason it is absent from a declaration: it is the for-each + escape hatch, resolved by following a loop's `list` back to its source output, and a + variable is not the output of anything. + of: + type: array + description: Shape of each list element or object member, spelled as a declaration's `of` is. + items: { $ref: "#/$defs/declaration" } value: - description: A literal, or an expression evaluated by the SDK's shared evaluator. + description: >- + The initial value: a literal, or an expression evaluated by the SDK's shared evaluator. + Checked against `t` like any other Slot when it is a Template. step: type: object @@ -261,10 +288,15 @@ $defs: sits at the root itself, so a Host may declare `component.block.render` and collide with nothing — see ADR-0014. - Hatua treats most verbs as opaque, but interprets three structurally: `core.fork` creates - branches, `core.for_each` nests and exposes `item`, and `core.map` derives its outputs - from its own `entries` field rather than from its manifest. The first two drive reference - scope and derived layout; the third drives reference scope alone. + Hatua treats most verbs as opaque, but interprets five structurally: `core.fork` creates + branches, `core.for_each` nests and exposes `item`, `core.repeat` nests and carries an + `until:` condition, `core.map` derives its outputs from its own `entries` field rather + than from its manifest, and `core.set_var` writes one of the Board's `vars` and is typed + by that var's declaration rather than by a manifest field. + + The three nesting verbs drive reference scope and derived layout; `core.map` drives + reference scope alone; `core.set_var` drives neither and is here because its `value` is + a Slot no manifest can type. `core.map` is the one component whose outputs a manifest cannot declare, because they are whatever the user named. Its `with.entries` is a list of `{key, value, type}`, and a @@ -281,8 +313,29 @@ $defs: items: { $ref: "#/$defs/branch" } steps: type: array - description: Loop children, nested directly with no branch wrapper. Only on `core.for_each`. + description: >- + Loop children, nested directly with no branch wrapper. On `core.for_each` and + `core.repeat`. items: { $ref: "#/$defs/step" } + until: + type: string + description: >- + A `core.repeat`'s termination condition, and only meaningful there. The body runs, then + this is evaluated; false runs it again. So a repeat ALWAYS runs its children at least + once — which is what lets one discharge a block's return obligation where a + `core.for_each` cannot, and what makes a pre-tested loop expressible as a body that + starts with a fork while the reverse costs a duplicated body. + + A structural key beside `steps:` rather than a field under `with:`, for the reason a + Branch's `when` is one: a manifest field carries a rendering `kind` and no type, so the + expected type is recovered from `FIELD_KIND_TYPES` — and that vocabulary cannot express + "a Template that must produce a boolean" at all. Under `with:` the condition would + type-check as text, which is the whole half of the contract it exists to carry. + + Nothing bounds the iterations, and that is deliberate rather than missing: whether an + `until` ever goes false depends on run-time values, so unlike recursion it is not a + property the document has. Hatua does not execute, so a bound written here would be a + number no reader could enforce. Bounding is the Host runner's obligation — see ADR-0013. branch: type: object diff --git a/source/sdk/go/definition.go b/source/sdk/go/definition.go index 33903fc..9f735d1 100644 --- a/source/sdk/go/definition.go +++ b/source/sdk/go/definition.go @@ -84,11 +84,23 @@ type Trigger struct { With map[string]any `yaml:"with,omitempty"` } -// Variable is workflow-scoped mutable state. A list rather than a map so a type -// or label can be added later without breaking every existing file. +// Variable is Board-scoped mutable state: the workflow's at the root, a block's +// inside one. +// +// T is declared rather than read off Value, which is the decision core.set_var +// forced. Value is only the FIRST value — a core.set_var writes the same +// variable from a step — so a type inferred from the literal in the document +// would be a claim about one moment in an execution rather than about the +// variable, and every downstream check was answered against it (ADR-0013). +// +// NOT a Declaration: a declaration is a contract with nothing in it, and a +// variable's key is its own label. T and Of are spelled identically so one +// function reads both. type Variable struct { - Key string `yaml:"key"` - Value any `yaml:"value"` + Key string `yaml:"key"` + T string `yaml:"t"` + Of []Declaration `yaml:"of,omitempty"` + Value any `yaml:"value"` } // Step is one node of the tree. Steps nest through Branches (forks) and Steps @@ -101,6 +113,12 @@ type Step struct { With map[string]any `yaml:"with,omitempty"` Branches []Branch `yaml:"branches,omitempty"` Steps []Step `yaml:"steps,omitempty"` + // Until is a core.repeat's termination condition, tested AFTER the body — + // so a repeat always runs its children at least once. A structural key + // beside Steps rather than a field under With, for the reason a branch's + // When is one: FieldKindTypes has no mappable boolean, so under With the + // condition would type-check as text. + Until string `yaml:"until,omitempty"` } // Branch is one labelled path of a fork. Order matters in a condition fork: @@ -174,16 +192,50 @@ func WalkDocument(d Definition, visit func(StepRef, Step)) { // BlockPrefix is the verb root that says a step calls a block in this document. const BlockPrefix = "block." -// ReturnVerb publishes a block's declared outputs and ends it. ForkVerb and -// ForEachVerb are the other two Hatua reads structurally: their shape is a -// position in the document rather than a field under `with:`, so no manifest -// can describe them. +// The verbs Hatua reads structurally, because their shape is a position in the +// document rather than a field under `with:` — so no manifest can describe them. +// +// ReturnVerb publishes a block's declared outputs and ends it. ForkVerb +// branches. ForEachVerb and RepeatVerb both nest, and differ in exactly one +// way that matters here: a repeat's body always runs and a for-each's may not. +// SetVarVerb writes one of its Board's variables, and is typed by that +// variable's declaration. const ( ReturnVerb = "core.return" ForkVerb = "core.fork" ForEachVerb = "core.for_each" + RepeatVerb = "core.repeat" + SetVarVerb = "core.set_var" ) +// VarsOn reports the variables one Board declares: the workflow's at the root, a +// block's inside one. +// +// This is the whole of "a core.set_var can never reach out of the Board it is +// on" — there is no second list to fall back to, so a block naming a workflow +// variable is an unknown name rather than a scope the runner resolves +// differently. +func VarsOn(doc Definition, board BoardID) []Variable { + if board == RootBoard { + return doc.Vars + } + if block := BlockOf(doc, board); block != nil { + return block.Vars + } + return nil +} + +// VariableOf returns one Board's variable by key, or nil. +func VariableOf(doc Definition, board BoardID, key string) *Variable { + vars := VarsOn(doc, board) + for i := range vars { + if vars[i].Key == key { + return &vars[i] + } + } + return nil +} + // CallsOf reports which blocks a step list reaches directly, in document order. // // Reads the whole board rather than its top level: a call nested inside a fork diff --git a/source/sdk/go/diagnostics.gen.go b/source/sdk/go/diagnostics.gen.go index ae0fab7..61add40 100644 --- a/source/sdk/go/diagnostics.gen.go +++ b/source/sdk/go/diagnostics.gen.go @@ -23,6 +23,8 @@ const ( CodeForkNeedsTwoBranches DefinitionCode = "FORK_NEEDS_TWO_BRANCHES" CodeBranchUnreachableAfter DefinitionCode = "BRANCH_UNREACHABLE_AFTER" CodeLoopHasNoBody DefinitionCode = "LOOP_HAS_NO_BODY" + CodeRepeatHasNoCondition DefinitionCode = "REPEAT_HAS_NO_CONDITION" + CodeVarUnknown DefinitionCode = "VAR_UNKNOWN" CodeBlockUnknown DefinitionCode = "BLOCK_UNKNOWN" CodeBlockRecursion DefinitionCode = "BLOCK_RECURSION" CodeReturnOutsideBlock DefinitionCode = "RETURN_OUTSIDE_BLOCK" @@ -51,6 +53,8 @@ var DefinitionDiagnostics = map[DefinitionCode]DefinitionDiagnosticSpec{ CodeForkNeedsTwoBranches: {Code: CodeForkNeedsTwoBranches, Blocks: BlocksPublish, Message: "A fork needs at least two branches — add the other path."}, CodeBranchUnreachableAfter: {Code: CodeBranchUnreachableAfter, Blocks: BlocksPublish, Message: "\"{label}\" has no condition, so nothing after it can ever run. Only the last branch may be unconditional."}, CodeLoopHasNoBody: {Code: CodeLoopHasNoBody, Blocks: BlocksPublish, Message: "This loop repeats nothing. Add at least one Step inside it."}, + CodeRepeatHasNoCondition: {Code: CodeRepeatHasNoCondition, Blocks: BlocksPublish, Message: "This repeat has no condition, so nothing ever ends it. Give it an `until`."}, + CodeVarUnknown: {Code: CodeVarUnknown, Blocks: BlocksPublish, Message: "No variable called \"{name}\" is declared on this board."}, CodeBlockUnknown: {Code: CodeBlockUnknown, Blocks: BlocksPublish, Message: "No block called \"{name}\" is declared in this workflow."}, CodeBlockRecursion: {Code: CodeBlockRecursion, Blocks: BlocksPublish, Message: "\"{name}\" calls itself, directly or through another block."}, CodeReturnOutsideBlock: {Code: CodeReturnOutsideBlock, Blocks: BlocksPublish, Message: "`core.return` publishes a block's outputs, and this is not inside a block."}, diff --git a/source/sdk/go/load.go b/source/sdk/go/load.go index fa72f56..f468670 100644 --- a/source/sdk/go/load.go +++ b/source/sdk/go/load.go @@ -136,10 +136,8 @@ func (d *Definition) Validate() error { return fmt.Errorf("%s: trigger %q needs a use", prefix, t.ID) } } - for _, v := range d.Vars { - if err := identifier(v.Key, "var key", prefix); err != nil { - return err - } + if err := validateVariables(d.Vars, "", prefix); err != nil { + return err } for _, b := range d.Blocks { if err := identifier(b.ID, "block id", prefix); err != nil { @@ -148,13 +146,11 @@ func (d *Definition) Validate() error { if b.Steps == nil { return fmt.Errorf("%s: block %q needs a steps list", prefix, b.ID) } - for _, v := range b.Vars { - if err := identifier(v.Key, "var key", prefix); err != nil { - return err - } + if err := validateVariables(b.Vars, fmt.Sprintf(" of block %q", b.ID), prefix); err != nil { + return err } for _, side := range [][]Declaration{b.Params, b.Outputs} { - if err := validateDeclarations(side, b.ID, prefix); err != nil { + if err := validateDeclarations(side, fmt.Sprintf("block %q", b.ID), prefix); err != nil { return err } } @@ -207,31 +203,72 @@ func identifier(value, what, prefix string) error { return nil } +// validateType holds a `t` to the set a document may name. +// +// One function rather than a switch per caller, because a block's contract and a +// Board's variables are checked against the same set and a second copy is a +// second answer: a type this accepts and the JSON Schema refuses is a document +// the runner loads and the builder will not open. +// +// `item` is refused along with everything else outside the set. It resolves by +// following a loop's list back to its source output, and neither a parameter nor +// a variable is the output of anything. `unknown` is refused for a sharper +// reason: the checker treats it as matching everything, so accepting it would +// switch the type gate off for that name while the builder still drew a marking +// beside it. +func validateType(t, what, prefix string) error { + switch t { + case "text", "number", "boolean", "datetime", "object", "list": + return nil + } + return fmt.Errorf("%s: %s declares an unusable type %q", prefix, what, t) +} + +// validateVariables holds a Board's variables to the same contract a block's +// declarations are held to. +// +// A variable's `t` is what every `{{var.}}` read and every `core.set_var` +// write is checked against, so it is the contract rather than a hint, and `of` +// carries the same nested shape a declaration's does. +func validateVariables(vars []Variable, where, prefix string) error { + for _, v := range vars { + if err := identifier(v.Key, "var key", prefix); err != nil { + return err + } + owner := fmt.Sprintf("variable %q%s", v.Key, where) + if err := validateType(v.T, owner, prefix); err != nil { + return err + } + if err := validateDeclarations(v.Of, owner, prefix); err != nil { + return err + } + } + return nil +} + // validateDeclarations holds a block's contract to the shape a manifest output -// has. `item` is refused: it resolves by following a loop's list back to its -// source, and a parameter is not the output of anything. +// has, and a variable's `of` to the same one. // // Deliberately unbounded in depth, because the schema is: a cap here and none in // the JSON Schema would refuse a document the builder published, which is the // divergence this function exists to prevent. A bound belongs in the shared // contract or nowhere. -func validateDeclarations(declarations []Declaration, block, prefix string) error { +func validateDeclarations(declarations []Declaration, owner, prefix string) error { for _, declaration := range declarations { if err := identifier(declaration.K, "declaration key", prefix); err != nil { return err } if declaration.Label == "" { - return fmt.Errorf("%s: %q in block %q needs a label", prefix, declaration.K, block) + return fmt.Errorf("%s: %q in %s needs a label", prefix, declaration.K, owner) } - switch declaration.T { - case "text", "number", "boolean", "datetime", "object", "list": - default: - return fmt.Errorf( - "%s: %q in block %q declares an unusable type %q", - prefix, declaration.K, block, declaration.T, - ) + if err := validateType( + declaration.T, + fmt.Sprintf("%q in %s", declaration.K, owner), + prefix, + ); err != nil { + return err } - if err := validateDeclarations(declaration.Of, block, prefix); err != nil { + if err := validateDeclarations(declaration.Of, owner, prefix); err != nil { return err } } diff --git a/source/sdk/go/loops_test.go b/source/sdk/go/loops_test.go new file mode 100644 index 0000000..c6860d3 --- /dev/null +++ b/source/sdk/go/loops_test.go @@ -0,0 +1,252 @@ +package hatua + +import ( + "testing" + + "hatua.dev/go/expressions" +) + +// core.repeat and core.set_var, on the side the rules corpus cannot reach. +// +// The corpus compares diagnostics; these follow the other half — what a Slot +// expects and what the checker then says about it. That is where the type +// marking lives, and it is the whole reason a var's type is declared: a +// core.set_var writing a number into a var the builder marked `text` would make +// every downstream check an answer to the wrong question. +// +// packages/model/src/loops.test.ts mirrors these assertion for assertion. + +func setVarStep(with map[string]any) Step { + return Step{ID: "bump", Use: SetVarVerb, With: with} +} + +func TestRepeatConditionIsBoolean(t *testing.T) { + slot := RepeatSlot("{{ var.done }}") + if slot.Name != "until" || slot.ExpectedType != expressions.TypeBoolean { + t.Fatalf("expected a boolean `until` slot, got %+v", slot) + } +} + +func TestRepeatConditionRefusesACount(t *testing.T) { + doc := Definition{ + Vars: []Variable{{Key: "seen", T: "number", Value: 0}}, + Steps: []Step{{ID: "again", Use: RepeatVerb, Until: "{{ var.seen }}"}}, + } + ctx := expressions.CheckContext{ + Scope: ScopeFor(doc, StepRef{Board: RootBoard, ID: "again"}, nil, nil), + Functions: expressions.CoreFunctions(), + } + + if found := expressions.Validate("{{ var.seen }}", expressions.TypeBoolean, ctx); len(found) == 0 { + t.Fatalf("expected a count to be refused where a condition belongs") + } + if found := expressions.Validate("{{ var.seen > 3 }}", expressions.TypeBoolean, ctx); len(found) != 0 { + t.Fatalf("expected a comparison to pass, got %v", found) + } +} + +func TestSetVarSlotIsTypedByTheVariableItNames(t *testing.T) { + doc := Definition{ + Vars: []Variable{{Key: "attempt", T: "number", Value: 0}}, + Steps: []Step{setVarStep(map[string]any{"key": "attempt", "value": "{{ 1 + 1 }}"})}, + } + + slot, ok := SetVarSlot(doc, RootBoard, doc.Steps[0]) + if !ok { + t.Fatalf("expected a slot") + } + if slot.Name != "value" || slot.ExpectedType != expressions.TypeNumber { + t.Fatalf("expected a number `value` slot, got %+v", slot) + } +} + +func TestSetVarSlotIsAbsentWithoutAVariableToTypeIt(t *testing.T) { + doc := Definition{Vars: []Variable{{Key: "attempt", T: "number", Value: 0}}} + + cases := map[string]map[string]any{ + "a key no board declares": {"key": "attemp", "value": "1"}, + "no key at all": {"value": "1"}, + "a literal value": {"key": "attempt", "value": 7}, + } + for name, with := range cases { + if _, ok := SetVarSlot(doc, RootBoard, setVarStep(with)); ok { + t.Fatalf("%s: expected no slot", name) + } + } +} + +// The end of the argument, from the document to a verdict. A var declared +// `boolean` refuses a number written into it, and the same document with +// `t: number` accepts it — so the marking the builder shows and the value the +// runner produces cannot disagree. +func TestSetVarIsHeldToTheDeclaredType(t *testing.T) { + refused := Definition{ + Vars: []Variable{{Key: "attempt", T: "boolean", Value: false}}, + Steps: []Step{setVarStep(map[string]any{"key": "attempt", "value": "{{ 1 + 1 }}"})}, + } + slot, ok := SetVarSlot(refused, RootBoard, refused.Steps[0]) + if !ok { + t.Fatalf("expected a slot") + } + found := expressions.Validate(slot.Template, slot.ExpectedType, expressions.CheckContext{ + Scope: ScopeFor(refused, StepRef{Board: RootBoard, ID: "bump"}, nil, nil), + Functions: expressions.CoreFunctions(), + }) + if len(found) != 1 || found[0].Code != "EXPR_TYPE_MISMATCH" { + t.Fatalf("expected one EXPR_TYPE_MISMATCH, got %v", found) + } + + accepted := Definition{ + Vars: []Variable{{Key: "attempt", T: "number", Value: 0}}, + Steps: []Step{setVarStep(map[string]any{"key": "attempt", "value": "{{ 1 + 1 }}"})}, + } + slot, _ = SetVarSlot(accepted, RootBoard, accepted.Steps[0]) + found = expressions.Validate(slot.Template, slot.ExpectedType, expressions.CheckContext{ + Scope: ScopeFor(accepted, StepRef{Board: RootBoard, ID: "bump"}, nil, nil), + Functions: expressions.CoreFunctions(), + }) + if len(found) != 0 { + t.Fatalf("expected the declared type to accept it, got %v", found) + } +} + +// A var declared on the wrong Board is out of reach rather than resolved +// differently, which is what makes core.set_var Board-scoped by construction +// rather than by a rule. +func TestSetVarInsideABlockCannotReachTheWorkflowsVariables(t *testing.T) { + doc := Definition{ + Vars: []Variable{{Key: "attempt", T: "number", Value: 0}}, + Blocks: []Block{{ + ID: "ask", + Vars: []Variable{{Key: "note", T: "text", Value: ""}}, + Steps: []Step{setVarStep(map[string]any{"key": "attempt", "value": "{{ 1 + 1 }}"})}, + }}, + } + + if _, ok := SetVarSlot(doc, "ask", doc.Blocks[0].Steps[0]); ok { + t.Fatalf("expected the workflow's variable to be out of reach from inside a block") + } +} + +func TestVariableTypeComesFromItsDeclaration(t *testing.T) { + if got := variableType(Variable{Key: "a", T: "number", Value: "not a number"}); got != expressions.TypeNumber { + t.Fatalf("expected number, got %v", got) + } + // Nothing declares one, so nothing is guessed from the value beside it. + if got := variableType(Variable{Key: "a", Value: 7}); got != expressions.TypeUnknown { + t.Fatalf("expected unknown, got %v", got) + } +} + +func TestVariableSlotChecksTheInitialValue(t *testing.T) { + slot, ok := VariableSlot(Variable{Key: "attempt", T: "number", Value: "{{ 1 + 1 }}"}) + if !ok || slot.Name != "attempt" || slot.ExpectedType != expressions.TypeNumber { + t.Fatalf("expected a number slot named attempt, got %+v (%v)", slot, ok) + } + // A literal is not a Template, so there is no Slot and nothing to check. + if _, ok := VariableSlot(Variable{Key: "attempt", T: "number", Value: 0}); ok { + t.Fatalf("expected no slot for a literal") + } +} + +func TestVariableOfShapesAMemberRead(t *testing.T) { + doc := Definition{ + Vars: []Variable{{ + Key: "entry", + T: "object", + Of: []Declaration{{K: "headline", Label: "Headline", T: "text"}}, + Value: "", + }}, + Steps: []Step{{ID: "s1", Use: "component.email.send"}}, + } + ctx := expressions.CheckContext{ + Scope: ScopeFor(doc, StepRef{Board: RootBoard, ID: "s1"}, nil, nil), + Functions: expressions.CoreFunctions(), + } + + if found := expressions.Validate("{{ var.entry.headline }}", expressions.TypeText, ctx); len(found) != 0 { + t.Fatalf("expected a declared member to resolve, got %v", found) + } + if found := expressions.Validate("{{ var.entry.headline }}", expressions.TypeNumber, ctx); len(found) == 0 { + t.Fatalf("expected a text member to be refused where a number belongs") + } +} + +// A variable's declared type is held to the same set a block's declarations are. +// +// The loader is a layer above the rules corpus — that corpus calls +// ValidateDefinition directly and never reaches Validate() — so a type the +// loader accepts and the JSON Schema refuses is a document this SDK loads and +// the builder will not open. `unknown` and `item` are the sharp cases: the +// checker treats both as matching everything, so accepting one switches the type +// gate off for that variable while the builder still draws a marking beside it. +func TestVariableTypeIsHeldToTheDeclaredSet(t *testing.T) { + document := func(declared string) string { + return "id: wf\nname: W\nversion: 1\nstatus: draft\n" + + "vars:\n - { key: attempt, t: " + declared + ", value: 0 }\nsteps: []\n" + } + + for _, declared := range []string{"unknown", "item", "Text", "str", ""} { + if _, err := LoadDefinition([]byte(document(declared))); err == nil { + t.Fatalf("expected t: %q to be refused", declared) + } + } + + for _, declared := range []string{"text", "number", "boolean", "datetime", "object", "list"} { + if _, err := LoadDefinition([]byte(document(declared))); err != nil { + t.Fatalf("expected t: %q to load, got: %v", declared, err) + } + } +} + +// A variable's `of` carries the same nested shape a declaration's does, so its +// members are held to the same contract rather than to none. +func TestVariableMembersAreHeldToTheSameContract(t *testing.T) { + const bad = `id: wf +name: W +version: 1 +status: draft +vars: + - key: entry + t: object + of: + - { k: headline, label: Headline, t: nonsense } + value: "" +steps: [] +` + if _, err := LoadDefinition([]byte(bad)); err == nil { + t.Fatalf("expected a member declaring an unusable type to be refused") + } + + const missingLabel = `id: wf +name: W +version: 1 +status: draft +vars: + - key: entry + t: object + of: + - { k: headline, t: text } + value: "" +steps: [] +` + if _, err := LoadDefinition([]byte(missingLabel)); err == nil { + t.Fatalf("expected a member with no label to be refused") + } + + const good = `id: wf +name: W +version: 1 +status: draft +vars: + - key: entry + t: object + of: + - { k: headline, label: Headline, t: text } + value: "" +steps: [] +` + if _, err := LoadDefinition([]byte(good)); err != nil { + t.Fatalf("expected a well-formed member to load, got: %v", err) + } +} diff --git a/source/sdk/go/slots.go b/source/sdk/go/slots.go index f1829fb..86195be 100644 --- a/source/sdk/go/slots.go +++ b/source/sdk/go/slots.go @@ -1,9 +1,6 @@ package hatua import ( - "strings" - "time" - "hatua.dev/go/expressions" ) @@ -124,8 +121,8 @@ func declaredSlots(declarations []Declaration, step Step) []expressions.Slot { // SlotsForStep is the Slots any step resolves into, whichever kind it is. // -// One entry point so a runner never has to know that a call and a return are -// the two verbs a manifest cannot describe. +// One entry point so a runner never has to know that a call, a return and a +// core.set_var are the three verbs a manifest cannot describe. func SlotsForStep(doc Definition, board BoardID, step Step, manifest Manifest) []expressions.Slot { if called, ok := BlockIDOf(step.Use); ok { if block := BlockOf(doc, called); block != nil { @@ -139,9 +136,73 @@ func SlotsForStep(doc Definition, board BoardID, step Step, manifest Manifest) [ } return []expressions.Slot{} } + if step.Use == SetVarVerb { + if slot, ok := SetVarSlot(doc, board, step); ok { + return []expressions.Slot{slot} + } + return []expressions.Slot{} + } return SlotsFor(step, manifest) } +// RepeatSlot is the Slot a core.repeat's Until resolves into. +// +// The mirror of WhenSlot, and for the same reason: a condition is a boolean, and +// no manifest field can say so — FieldKindTypes has no mappable boolean at all, +// because `bool` holds a literal rather than a Template. That is why Until is a +// structural key beside Steps rather than a field under With; under With it +// would type-check as text, so `{{steps.s2.count}}` would pass as a termination +// condition. +// +// A repeat tests this AFTER its body, so the body always runs at least once. +func RepeatSlot(until string) expressions.Slot { + return expressions.Slot{Name: "until", Template: until, ExpectedType: expressions.TypeBoolean} +} + +// SetVarSlot is the Slot a core.set_var's `value` resolves into, typed by the +// variable it names. +// +// The third verb a manifest cannot describe, alongside a call and a core.return, +// and for the same reason: what its field must produce is declared elsewhere in +// the document. Here it is the Board's vars — which is also why a core.set_var +// inside a block can only ever name that block's, since the list is read from +// the Board the step sits on and there is no second one to fall back to. +// +// Not ok when the step names no variable, or names one the Board does not +// declare: both have their own diagnostic, and resolving a Template against a +// type nothing declared would report a mismatch the user cannot act on. +func SetVarSlot(doc Definition, board BoardID, step Step) (expressions.Slot, bool) { + key, ok := step.With["key"].(string) + if !ok { + return expressions.Slot{}, false + } + variable := VariableOf(doc, board, key) + if variable == nil { + return expressions.Slot{}, false + } + template, ok := step.With["value"].(string) + if !ok { + return expressions.Slot{}, false + } + return expressions.Slot{ + Name: "value", Template: template, ExpectedType: variableType(*variable), + }, true +} + +// VariableSlot is the Slot a variable's initial value resolves into. +// +// A var's Value may hold `{{ … }}`, and until T was declared there was nothing +// to check it against. Not ok for a literal: only a Template is a Slot. +func VariableSlot(variable Variable) (expressions.Slot, bool) { + template, ok := variable.Value.(string) + if !ok { + return expressions.Slot{}, false + } + return expressions.Slot{ + Name: variable.Key, Template: template, ExpectedType: variableType(variable), + }, true +} + // WhenSlot is the Slot a branch's condition resolves into. // // Separate from SlotsFor because a branch is not a step and has no manifest — @@ -318,7 +379,7 @@ func BoardScope(doc Definition, board BoardID, manifests []Manifest, context []C for _, variable := range vars { entries = append(entries, expressions.ScopeEntry{ Path: "var." + variable.Key, - Type: expressions.TypeNode{Type: varType(variable.Value)}, + Type: variableToType(variable), }) } @@ -409,35 +470,39 @@ func contextKeyType(key ContextKey) expressions.TypeNode { return node } -// varType reads a workflow variable's type from its literal value. +// variableType reads a variable's type from its declaration. +// +// Read from T rather than from the value beside it: a var's value is only its +// FIRST value, because core.set_var writes the same variable from a step, so a +// type read off the literal in the document is a claim about one moment in an +// execution rather than about the variable (ADR-0013). // -// Vars are the one addressable thing with no declaration to consult, and calling -// them all unknown would make every `{{ var.x }}` in a workflow warn — which -// trains people to ignore warnings. A var holding text is text. A var holding a -// Template is genuinely unknown until it is evaluated, and says so. -func varType(value any) expressions.ValueType { - switch v := value.(type) { - case string: - if strings.Contains(v, "{{") { - return expressions.TypeUnknown +// A declared type is also decoder-independent, which a type read off the value +// cannot be: yaml.v3 turns `value: 2024-01-01T00:00:00Z` into a time.Time while +// the builder's parser leaves it a string, so reading the value types one scalar +// two ways and the two languages disagree about the same document. +// +// Unknown for a var carrying no T at all: the schema requires one, so this is a +// hand-edit, and refusing to check is the honest answer where guessing `text` +// would refuse a document over a type nothing declared. +func variableType(variable Variable) expressions.ValueType { + if variable.T == "" { + return expressions.TypeUnknown + } + return expressions.ValueType(variable.T) +} + +// variableToType is that, plus the shape Of carries — spelled exactly as a +// declaration's Of is, so one traversal reads both. +func variableToType(variable Variable) expressions.TypeNode { + node := expressions.TypeNode{Type: variableType(variable)} + if len(variable.Of) > 0 { + node.Members = make(map[string]expressions.TypeNode, len(variable.Of)) + for _, member := range variable.Of { + node.Members[member.K] = declarationToType(member) } - return expressions.TypeText - case float64, float32, int, int8, int16, int32, int64, - uint, uint8, uint16, uint32, uint64: - return expressions.TypeNumber - case bool: - return expressions.TypeBoolean - case []any: - return expressions.TypeList - case time.Time: - // Text, not datetime, and the reason is the decoders rather than the - // language: yaml.v3 turns `value: 2024-01-01T00:00:00Z` into a - // time.Time, and the `yaml` package the builder uses leaves it a string. - // Typing it `datetime` here would block a publish in the Go SDK that the - // builder allows, over one scalar neither of them was told the type of. - return expressions.TypeText - } - return expressions.TypeUnknown + } + return node } // stepOutputType reports a step's outputs as a type. diff --git a/source/sdk/go/validity.go b/source/sdk/go/validity.go index 4ff8c03..dbd1fe8 100644 --- a/source/sdk/go/validity.go +++ b/source/sdk/go/validity.go @@ -223,6 +223,24 @@ func MissingRequiredFields(doc Definition, byUse map[string]Manifest) []Diagnost return } + if step.Use == SetVarVerb { + // Structural, for the reason a return's fields are: what a + // core.set_var takes is a var key and a value typed by the var that + // key names, and no manifest knows which Board a step is on. Its + // manifest declares no fields at all, so without this a set_var with + // nothing in it reports nothing. + for _, field := range setVarFields { + if !unfilled(values[field.key]) { + continue + } + subject.FieldKey = field.key + out = append(out, raise(CodeFieldRequired, subject, map[string]string{ + "label": field.label, + })) + } + return + } + fromManifest(subject, step.Use, values) }) @@ -233,6 +251,14 @@ func MissingRequiredFields(doc Definition, byUse map[string]Manifest) []Diagnost return out } +// setVarFields is what a core.set_var takes. Labels rather than keys in the +// message, matching every other required-field diagnostic — the sentence is read +// by someone looking at a form, not at YAML. +var setVarFields = []struct{ key, label string }{ + {"key", "Variable"}, + {"value", "Value"}, +} + // UnknownComponents reports a step or a trigger whose verb nothing declares. // // The two roots fail differently, so they are two codes. A `component.*` or @@ -319,9 +345,29 @@ func MalformedContainers(doc Definition) []Diagnostic { } } - if step.Use == ForEachVerb && len(step.Steps) == 0 { + // One code for both loop verbs: the mistake is the same and so is the + // fix, and the message names neither. + if (step.Use == ForEachVerb || step.Use == RepeatVerb) && len(step.Steps) == 0 { out = append(out, raise(CodeLoopHasNoBody, subject, nil)) } + + // Read from the tree rather than from With, because that is where it + // lives: FieldKindTypes has no mappable boolean, so a condition under + // With would type-check as text — see RepeatSlot. + if step.Use == RepeatVerb && strings.TrimSpace(step.Until) == "" { + out = append(out, raise(CodeRepeatHasNoCondition, subject, nil)) + } + + if step.Use == SetVarVerb { + key, named := step.With["key"].(string) + // A missing key is CodeFieldRequired's to report. Resolving an + // absent one against the Board would say no variable is called "" + // which names a variable the user never wrote. + if named && key != "" && VariableOf(doc, ref.Board, key) == nil { + subject.FieldKey = "key" + out = append(out, raise(CodeVarUnknown, subject, map[string]string{"name": key})) + } + } }) return out @@ -332,14 +378,25 @@ func MalformedContainers(doc Definition) []Diagnostic { // // A fork discharges the obligation only when EVERY branch does and the fork is // exhaustive — a falsy `when` on the last branch, matching how -// MalformedContainers reads one. A core.for_each never discharges it: the list -// may be empty and the body may never run, which is the sibling-branch argument -// applied to time rather than to paths. +// MalformedContainers reads one. +// +// The two loop verbs answer differently, and the difference is the whole rule. A +// core.for_each never discharges it: the list may be empty and the body may +// never run. A core.repeat does, because it tests its Until AFTER the body and +// therefore always runs it once. One question — is this region guaranteed to run +// at all — which is the sibling-branch argument applied to time rather than to +// paths. func alwaysReturns(steps []Step) bool { for _, step := range steps { if step.Use == ReturnVerb { return true } + if step.Use == RepeatVerb { + if alwaysReturns(step.Steps) { + return true + } + continue + } if step.Use != ForkVerb || len(step.Branches) == 0 { continue } diff --git a/source/sdk/js/src/expression.test.ts b/source/sdk/js/src/expression.test.ts index 4d66749..f7074e9 100644 --- a/source/sdk/js/src/expression.test.ts +++ b/source/sdk/js/src/expression.test.ts @@ -55,7 +55,7 @@ status: draft triggers: - { id: nightly, use: core.schedule, name: Nightly } vars: - - { key: digest_to, value: me@dane.dev } + - { key: digest_to, t: text, value: me@dane.dev } steps: - { id: s2, use: component.email.fetch, name: Fetch, with: { connection: mailbox } } - id: s6 diff --git a/source/sdk/js/src/index.ts b/source/sdk/js/src/index.ts index 5368d5f..5c32e53 100644 --- a/source/sdk/js/src/index.ts +++ b/source/sdk/js/src/index.ts @@ -46,12 +46,20 @@ export { callSlots, cyclicBlocks, findStep, + REPEAT_VERB, + repeatSlot, returnSlots, + SET_VAR_VERB, type StepRef, scopeFor, + setVarSlot, slotsFor, stepKey, upstreamOf, + variableOn, + variableSlot, + variableType, + varsOn, walkDocument, walkSteps, whenSlot,