diff --git a/.changeset/15429-decision-edge-branching-first-match.md b/.changeset/15429-decision-edge-branching-first-match.md new file mode 100644 index 00000000000..e0f19ecd4e7 --- /dev/null +++ b/.changeset/15429-decision-edge-branching-first-match.md @@ -0,0 +1,100 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-automation": minor +"@objectstack/lint": minor +"@objectstack/metadata-protocol": minor +"@objectstack/metadata-core": patch +--- + +feat(automation)!: an edge-branched `decision` is exclusive — the first out-edge whose condition holds, in declaration order, wins; `mode: 'inclusive'` takes every one (#15429) + + + +Clause-②: yes + +**BREAKING** — the run-time semantics of a shipped node type change. A `decision` node that +declares no `config.conditions` and branches on its out-edges used to take EVERY out-edge whose +condition held, one after another, while its schema, the docs and the engine's own comment all +called it an exclusive gateway; hotcrm#1555 rendered a refusal screen AND ran the conversion in +one execution. Maintainer ruling on #15429 (2026-09-23, 「跟主流对齐」): the gateway follows +BPMN's exclusive gateway, Salesforce Flow's Decision and n8n's Switch default, and taking every +true branch is a declaration the author writes down. + +| | before | after | +|:--|:--|:--| +| two conditioned out-edges, both hold | both successors run, sequentially, nothing reported | the FIRST declared one runs; the second records a `skipped` step | +| `config: { mode: 'inclusive' }` | accepted, never read | every out-edge whose condition holds runs, sequentially | +| none holds | the `isDefault` edge runs | unchanged | +| `mode` beside a non-empty `conditions` list, or outside `'exclusive' \| 'inclusive'` | refused by a direct parse only | refused at `registerFlow` and by `os validate`, with the schema's own sentence | + +## Migration: FROM → TO + +`os migrate meta --from 17` lists the mechanical edits for existing sources and applies them +to the migrated stack: the ADR-0087 D2 conversion `flow-decision-mode-inclusive-explicit` +writes `mode: 'inclusive'` onto every decision that has no `conditions` list and two or more +conditioned out-edges, inside ADR-0031 regions included, so a migrated flow runs exactly as it +did. + +```ts +// FROM — every true out-edge ran +{ id: 'verdict', type: 'decision', label: 'Verdict?' } +// TO — what the conversion writes; delete the key where the conditions partition +{ id: 'verdict', type: 'decision', label: 'Verdict?', config: { mode: 'inclusive' } } +``` + +Then review each written key (the paired D3 entry `flow-decision-edge-branching-first-match` +carries the acceptance criteria): delete it where the conditions partition (`== 'a'` beside +`!= 'a'`, `>` beside `<=`, a guard beside `isDefault: true`), keep it where the flow relies on +more than one branch running for one record, and where the overlap was accidental narrow the +conditions into a partition and delete the key. `os validate` reports +`flow-decision-inclusive-overlap` on every decision that keeps the key with two or more +conditioned out-edges, so the review list is the lint output. + +## BREAKING for flows stored in `sys_metadata` — maintainer ruling letter C on #15429 + +A `decision` node stored in `sys_metadata` (a flow built or edited in the Studio designer) with +**no `config.conditions`, no `mode`, and two or more out-edges carrying a `condition`** evaluates +**first-match** after this upgrade: where it took every out-edge whose condition held, it now takes +only the first one that holds, in the order the flow declares its edges. Nothing rewrites that row +— no stored-row migration, no cutoff, no read-path completion — because nothing about a stored row +says it was saved before the flip. The one-line fix, for a node that meant every branch: + +```ts +{ id: 'route', type: 'decision', label: 'Route', config: { mode: 'inclusive' } } +``` + +`os migrate meta --stored` (and `POST /api/v1/meta/_migrate-stored`) lists every such node under +`decisionModeReview` — flow row, node id, label and path — on a preview and an `--apply` run +alike, and writes nothing for it: the list moves no row outcome, no count and no exit code, so an +operator can review the candidates before and after the upgrade. A node leaves the list once it +declares `mode`, either member. Every such node in the measured corpus below is a partition, where +the new meaning runs exactly what the old one did. + +Authored sources and built artifacts keep the old behaviour instead, where the source's age is a +fact: `os migrate meta --from 17` writes `mode: 'inclusive'` (above), while the authoring funnel, +the automation engine's flow rehydration seam and the artifact-ingestion door all refuse the +conversion by id — a default flip replayed there would turn a decision written today against this +contract, where an omitted `mode` means exclusive, into an inclusive gateway. + +## Reach, measured at landing + +- Release state: the npm registry's `latest` `@objectstack/spec` is `17.4.0` (`npm view`, + 2026-09-27), whose `json-schema/automation/DecisionConfig.json` declares `conditions` only — + `mode` has not shipped; `.changeset/19867-decision-config-mode.md` and + `.changeset/20168-decision-mode-beside-conditions-refused.md` are still unconsumed in this + tree. So `mode` reaches its first release together with the traversal that reads it and the + conversion that writes it; no published accept set narrows, and the registration and + `os validate` refusals narrow nothing that shipped. +- Corpus census (this repository at the branch base and `objectstack-ai/hotcrm` at `2f7b2326`, + read-only): 30 decision nodes across 48 flows; 17 have two or more conditioned out-edges and + no `mode` (the conversion's positives — every one a hand-written partition, including + hotcrm's `lead_conversion.decision_duplicate`, the #1555 node), 13 have one conditioned + out-edge (left alone), and no node of any other type carries a conditioned out-edge, so the + exclusive traversal is scoped to `decision` with nothing else to migrate. +- What the published surface gains: the D2 conversion and its D3 entry in the protocol-18 + chain (`spec-changes.json`, the upgrade guide), `DecisionConfigSchema.mode`'s describe and + docblock now state the run-time semantics, and `@objectstack/lint` gains + `flow-decision-mode-invalid` (gating) and `flow-decision-inclusive-overlap` (advisory). + +The traversal change is scoped to `decision` nodes: conditioned out-edges of any other node +type keep the every-true-edge traversal they had (none was measured to exist). diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 215daf88f36..5fa10b1e134 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -1370,7 +1370,7 @@ Edges connect nodes and define the execution path: A node has exactly two ways to split its path, and mixing them is what makes a guard stop guarding (#4414). -**Branch on the edges** (BPMN exclusive gateway — the default choice): +**Branch on the edges** (BPMN gateway — the default choice): ```typescript { id: 'check', type: 'decision', label: 'Already converted?' }, // no config @@ -1388,6 +1388,61 @@ in parallel with `abort` — so an already-converted lead sees the abort screen condition by hand is the only other correct spelling; `isDefault` is the one that stays correct when a third branch is added. +**The gateway is exclusive.** When more than one out-edge carries a +`condition`, the engine evaluates them **in the order the `edges` array +declares them** and takes the **first** one whose condition holds — the BPMN +exclusive gateway, Salesforce Flow's Decision element, n8n's Switch default. +The siblings after it are not evaluated, and each records a `skipped` step in +the run log, so a run says which branch won and which were passed over. When +none holds, the `isDefault` edge runs. Two conditions that both hold for one +record therefore run **one** branch, the earlier one; put the branch you want +to win first. + +To take **every** out-edge whose condition holds — the BPMN inclusive gateway, +n8n's "send to all matching outputs" — declare it on the node: + +```typescript +{ id: 'route', type: 'decision', label: 'Route', config: { mode: 'inclusive' } }, +edges: [ + { id: 'e_vip', source: 'route', target: 'notify_account_manager', condition: 'lead.tier == "vip"' }, + { id: 'e_big', source: 'route', target: 'notify_finance', condition: 'lead.amount > 100000' }, +] +``` + +A VIP lead over the threshold takes both, one after another (never in +parallel). `mode` has two members, `'exclusive'` (what an omitted key means) +and `'inclusive'`; anything else, and a `mode` written beside a `config.conditions` +list (which is first-match on its own), is refused when the flow registers and +by `os validate` (`flow-decision-mode-invalid`), with the same sentence at both +doors. `os validate` also reports `flow-decision-inclusive-overlap` on an +inclusive decision with two or more conditioned out-edges, because that is the +shape in which more than one branch can run for one record. + + +**Upgrading a flow written before protocol 18.** An edge-branched decision used +to take every out-edge whose condition held. The ADR-0087 conversion +`flow-decision-mode-inclusive-explicit` writes `mode: 'inclusive'` onto every +decision with two or more conditioned out-edges and no `mode` when the chain is +replayed, so the migrated flow runs exactly as before; delete the key where the +two conditions partition (`== 'a'` beside `!= 'a'`, `>` beside `<=`), which is +the common case and the honest declaration. Nothing rewrites a flow at load: an +author who writes two branches today gets the exclusive gateway the contract +describes. Run `os migrate meta --from 17` to list the mechanical edits for +existing sources; apply them by hand. + + + +**A flow stored from the Studio takes the new meaning (breaking).** A decision +saved in `sys_metadata` before protocol 18 — no `config.conditions`, no `mode`, +two or more out-edges with a `condition` — runs **first-match** after the +upgrade, and nothing rewrites the stored row: nothing about a row says it was +saved before the change. `os migrate meta --stored` lists every such node (flow, +node id, label and path) on a preview and an `--apply` run alike and writes +nothing for it, so you can review them before and after upgrading. Where a node +meant every branch, declare `config: { mode: 'inclusive' }` on it; declaring +`mode` either way takes it off the list. + + **Branch on the node** (Salesforce-style decision outcomes): the node declares `config.conditions[]` and traversal restricts itself to the out-edge whose `label` matches the first matching entry. @@ -1410,11 +1465,14 @@ fallback used to be silent, and a decision declaring `'Yes — already converted against an out-edge labelled `'Yes'` is how #4414 shipped. `os validate` reports the shape as `flow-branch-label-unmatched` at build time, along with `flow-decision-unconditional-branch` (a guarded decision with an unconditional -sibling), `flow-default-edge-with-condition` and `flow-multiple-default-edges`. +sibling), `flow-default-edge-with-condition`, `flow-multiple-default-edges`, +`flow-decision-mode-invalid` and `flow-decision-inclusive-overlap`. A decision node that declares **no** `conditions` reports no branch at all — it -is a plain gateway and its out-edges do the routing. +is a plain gateway and its out-edges do the routing: the first out-edge whose +condition holds, in declaration order, unless the node declares +`config: { mode: 'inclusive' }`. Declaring **both** — `config.conditions` *and* per-edge `condition`s — is redundant but not wrong: the node picks a branch, and then that branch's edge diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index 69d875e7b7d..cc9dd12178b 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -1203,6 +1203,18 @@ can produce, because both supply a live one. | A flow whose rename the conflict guard refused | The old node-type token is a live name something else owns here. Rewriting would clobber that owner, so the row fails loudly naming the token — never a silent skip | | A site the conversion chain leaves as stored because no lossless rewrite exists — above all a page filter carrying `$and` / `$or` / `$not` | Flattening a combinator changes which rows the page selects, so it is never done. Each site is printed as a `TODO` line under its row — path, block, and what blocks the rewrite — whatever the row's outcome; a row with nothing but TODOs is reported `skipped`. It does not fail the run, since no run of this pass can clear it: rewrite each site by hand | +**One thing it lists and never writes: decision nodes that changed meaning at +protocol 18.** A stored `decision` with no `config.conditions`, no `mode` and two +or more out-edges carrying a `condition` took every out-edge whose condition held +before protocol 18, and takes only the first one now. A stored row keeps that new +meaning — the conversion that writes `mode: 'inclusive'` replays over authored +sources only, where you assert the source's age, and nothing asserts a row's — so +the report lists each such node under `decisionModeReview` (flow row, node id, +label and path) for you to review before and after the upgrade, on a preview and +an `--apply` run alike. The list changes no row, no count and no exit code. Where +a node meant every branch, declare `mode: 'inclusive'` on it; declaring `mode` +either way takes it off the list. + **Flows are covered, and cost one extra plugin.** Flow-node conversions carry an open-namespace conflict guard that has to consult the *live* executor registry to tell a rename from a clobber, so this run boots the automation engine — in an diff --git a/content/docs/references/automation/schemaless-node-config.mdx b/content/docs/references/automation/schemaless-node-config.mdx index debe18cda04..8f0bf19f832 100644 --- a/content/docs/references/automation/schemaless-node-config.mdx +++ b/content/docs/references/automation/schemaless-node-config.mdx @@ -66,13 +66,18 @@ The two halves reach different audiences, which is why they shipped together: nothing read — and then refuses, naming the `function` it does not have, instead of logging a line and reporting success as it used to. -`decision` stays export-only: nothing parses it at run time. It may carry no -`conditions` at all when it branches purely on edge predicates, its executor -reads `conditions` and nothing else, and its one other key — `mode` — is -declared AHEAD of the engine change that reads it (#15429; see -`DecisionConfigSchema`). Its enforcement remains the objectui -reconciliation test, which is what #4278 was actually about (a form -authoring keys nothing reads). +`decision` is parsed at **registration**, for one key (#15429): the +automation engine's `registerFlow` runs every decision node's config through +`DecisionConfigSchema` and refuses the flow on any issue rooted at +`mode` — a value outside the closed pair, or a `mode` beside a non-empty +`conditions` list — with this schema's own sentence, and `os validate` +reports the same issues as `flow-decision-mode-invalid`, so the two doors +cannot disagree. A decision may carry no `conditions` at all when it +branches purely on edge predicates; its executor reads `conditions` and +nothing else, and the engine's traversal reads `mode`. Its strictness +(unknown keys) still binds at authoring, in the published JSON Schema and in +the objectui reconciliation test, which is what #4278 was actually about (a +form authoring keys nothing reads). Undeclared aliases are NOT part of these contracts: `subflow`'s historical `flow` spelling graduated into the ADR-0087 D2 conversion @@ -96,9 +101,10 @@ door in front of its author, and the class it structurally could not cover is precisely the class with no second door. Closing these shapes is therefore not a duplicate check for `script` and `subflow`; it is their first one. -`decision` is still export-only, so its strictness binds at authoring -(`tsc`), in the published JSON Schema, and in objectui's reconciliation — -not at run time. It is closed anyway, because the campaign's whole finding +`decision`'s strictness binds at authoring (`tsc`), in the published JSON +Schema, and in objectui's reconciliation — not at run time, where the +registration reader judges `mode` alone (#15429). It is closed anyway, +because the campaign's whole finding is that a shape left open accretes a test, a form and a fixture that assert the openness, and then closing it is a migration instead of an edit. @@ -137,7 +143,7 @@ const result = DecisionConditionSchema.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | | **conditions** | `{ label: string; expression: string }[]` | optional | Ordered decision branches (first true expression wins; omit to branch purely on edge conditions) | -| **mode** | `Enum<'exclusive' \| 'inclusive'>` | optional | Declares how many out-edges an edge-branched decision takes when more than one out-edge condition holds: 'exclusive' = only the first, in the order the edges are declared (what an omitted mode means); 'inclusive' = every one that holds. Declared ahead of the engine change that reads it: until that lands, an edge-branched decision takes every out-edge whose condition holds, whatever this says. Refused beside a non-empty conditions list, which is first-match on its own: delete mode there, or move the branches onto the out-edges, delete conditions, and keep mode. | +| **mode** | `Enum<'exclusive' \| 'inclusive'>` | optional | Declares how many out-edges an edge-branched decision takes when more than one out-edge condition holds: 'exclusive' = only the first, in the order the edges are declared (what an omitted mode means; the siblings after it are not evaluated and record a skipped step); 'inclusive' = every one that holds, one after another. When none holds the isDefault edge runs either way. Refused beside a non-empty conditions list, which is first-match on its own: delete mode there, or move the branches onto the out-edges, delete conditions, and keep mode. Authored sources written while every true branch ran keep that behaviour through the os migrate meta --from 17 conversion, which writes mode: inclusive onto every edge-branched decision with two or more conditioned out-edges; a flow stored in sys_metadata is not rewritten and takes the first-match reading on upgrade (os migrate meta --stored lists those decisions). | ### Nested Shape: `DecisionConfig.conditions[number]` diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 137d4c4a109..c32eda832bf 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -841,6 +841,8 @@ export { FLOW_MULTI_WRITE_UNFILTERED, FLOW_LOOP_BODY_UNCONTAINED, FLOW_TRY_CATCH_WITHOUT_CATCH, + FLOW_DECISION_MODE_INVALID, + FLOW_DECISION_INCLUSIVE_OVERLAP, } from './lint-flow-patterns.js'; export { lintLivenessProperties } from './lint-liveness-properties.js'; diff --git a/packages/lint/src/lint-flow-patterns.test.ts b/packages/lint/src/lint-flow-patterns.test.ts index 2b947c69065..5a9cf751841 100644 --- a/packages/lint/src/lint-flow-patterns.test.ts +++ b/packages/lint/src/lint-flow-patterns.test.ts @@ -31,6 +31,8 @@ import { FLOW_MULTI_WRITE_UNFILTERED, FLOW_LOOP_BODY_UNCONTAINED, FLOW_TRY_CATCH_WITHOUT_CATCH, + FLOW_DECISION_MODE_INVALID, + FLOW_DECISION_INCLUSIVE_OVERLAP, } from './lint-flow-patterns.js'; // [#17495] Cross-site pin only — the re-judged `flow-inert-node-condition` // case below. This family's own coverage is unaffected by it; @@ -2605,3 +2607,147 @@ describe('a non-record member of a flow `edges` list (#16910)', () => { } }); }); + +/** + * #15429 — the decision's `mode`, at the `os validate` door. + * + * Two rules, two severities. `flow-decision-mode-invalid` GATES: the finding is + * the spec's own `DecisionConfigSchema` issue message, and `registerFlow` + * refuses the same declaration with the same sentence, so the flow can never + * arm. `flow-decision-inclusive-overlap` (ruling item 4) is advisory: an + * inclusive gateway with two or more conditioned out-edges is legal, and the + * rule says what it does rather than proving the conditions overlap. + */ +describe('decision `mode` (#15429)', () => { + /** One edge-branched decision with two conditioned out-edges, plus whatever config the case declares. */ + const modeFlow = (config: Record | undefined, edges?: Record[]) => ({ + flows: [{ + name: 'verdict', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'check', type: 'decision', ...(config ? { config } : {}) }, + { id: 'refuse', type: 'screen', config: {} }, + { id: 'convert', type: 'screen', config: {} }, + ], + edges: edges ?? [ + { id: 'e1', source: 'start', target: 'check' }, + { id: 'e2', source: 'check', target: 'refuse', condition: "lead.status != 'suspected'" }, + { id: 'e3', source: 'check', target: 'convert', condition: "lead.status == 'confirmed'" }, + ], + }], + }); + const ofRule = (stack: unknown, rule: string) => lintFlowPatterns(stack as AnyRec).filter((f) => f.rule === rule); + type AnyRec = Record; + + /** The same decision inside a loop body — the region walk must reach it. */ + const nestedModeFlow = (config: Record) => ({ + flows: [{ + name: 'sweep', + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 9 * * *' } }, + { + id: 'each', type: 'loop', label: 'Each', + config: { + collection: '{vars.rows}', + iteratorVariable: 'row', + body: { + nodes: [ + { id: 'check', type: 'decision', config }, + { id: 'refuse', type: 'screen', config: {} }, + { id: 'convert', type: 'screen', config: {} }, + ], + edges: [ + { id: 'e2', source: 'check', target: 'refuse', condition: "row.status != 'suspected'" }, + { id: 'e3', source: 'check', target: 'convert', condition: "row.status == 'confirmed'" }, + ], + }, + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'each' }], + }], + }); + + describe('flow-decision-mode-invalid — gating, with the schema sentence', () => { + it('gates `mode` beside a non-empty `conditions` list, either member, at config.mode', () => { + for (const mode of ['inclusive', 'exclusive']) { + const fnds = ofRule(modeFlow({ mode, conditions: [{ label: 'Refuse', expression: "lead.status != 'suspected'" }] }), FLOW_DECISION_MODE_INVALID); + expect(fnds).toHaveLength(1); + expect(fnds[0].severity).toBe('error'); + expect(fnds[0].where).toBe("flow 'verdict' · decision 'check' · config.mode"); + expect(fnds[0].message).toContain(`\`mode: '${mode}'\` is not valid on a decision that declares a \`conditions\` list`); + expect(fnds[0].message).toContain('Either delete `mode` and keep the list'); + expect(fnds[0].hint).toContain('`registerFlow` refuses this flow with the same sentence'); + } + }); + + it('gates a `mode` outside the closed pair with the value prescription', () => { + const fnds = ofRule(modeFlow({ mode: 'all' }), FLOW_DECISION_MODE_INVALID); + expect(fnds).toHaveLength(1); + expect(fnds[0].severity).toBe('error'); + expect(fnds[0].message).toContain("`mode: 'all'` is not a decision mode"); + expect(fnds[0].message).toContain("'exclusive' declares that only the FIRST out-edge"); + }); + + it('CONTROLS — an omitted `mode`, `mode` alone, `mode` on an empty list, and a list without `mode` are clean', () => { + for (const config of [undefined, { mode: 'inclusive' }, { mode: 'exclusive' }, { mode: 'inclusive', conditions: [] }]) { + expect(ofRule(modeFlow(config), FLOW_DECISION_MODE_INVALID)).toHaveLength(0); + } + expect(ofRule(modeFlow( + { conditions: [{ label: 'Refuse', expression: "lead.status != 'suspected'" }] }, + [ + { id: 'e1', source: 'start', target: 'check' }, + { id: 'e2', source: 'check', target: 'refuse', label: 'Refuse' }, + { id: 'e3', source: 'check', target: 'convert', isDefault: true }, + ], + ), FLOW_DECISION_MODE_INVALID)).toHaveLength(0); + }); + + it('reaches a decision inside a loop body and names the region', () => { + const fnds = ofRule(nestedModeFlow({ mode: 'parallel' }), FLOW_DECISION_MODE_INVALID); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toContain("loop 'each'"); + expect(fnds[0].where).toContain("decision 'check' · config.mode"); + }); + }); + + describe('flow-decision-inclusive-overlap — advisory (ruling item 4)', () => { + it("advises an inclusive decision with two conditioned out-edges, naming them and what the mode does", () => { + const fnds = ofRule(modeFlow({ mode: 'inclusive' }), FLOW_DECISION_INCLUSIVE_OVERLAP); + expect(fnds).toHaveLength(1); + // Advisory: the declaration is legal and the rule cannot prove overlap. + expect(fnds[0].severity).toBeUndefined(); + expect(fnds[0].where).toBe("flow 'verdict' · decision 'check'"); + expect(fnds[0].message).toContain('2 conditioned out-edge(s)'); + expect(fnds[0].message).toContain("'refuse', 'convert'"); + expect(fnds[0].message).toContain('EVERY one whose condition holds runs'); + expect(fnds[0].hint).toContain('delete `mode`'); + expect(fnds[0].hint).toContain('os migrate meta --from 17'); + }); + + it('does NOT advise an exclusive decision — omitted or written — with the same two edges', () => { + expect(ofRule(modeFlow(undefined), FLOW_DECISION_INCLUSIVE_OVERLAP)).toHaveLength(0); + expect(ofRule(modeFlow({ mode: 'exclusive' }), FLOW_DECISION_INCLUSIVE_OVERLAP)).toHaveLength(0); + }); + + it('does NOT advise one conditioned out-edge plus a default: inclusive and exclusive cannot differ there', () => { + const stack = modeFlow({ mode: 'inclusive' }, [ + { id: 'e1', source: 'start', target: 'check' }, + { id: 'e2', source: 'check', target: 'refuse', condition: "lead.status == 'suspected'" }, + { id: 'e3', source: 'check', target: 'convert', isDefault: true }, + ]); + expect(ofRule(stack, FLOW_DECISION_INCLUSIVE_OVERLAP)).toHaveLength(0); + }); + + it('does not double-report through flow-decision-unconditional-branch — both edges are gated', () => { + expect(ofRule(modeFlow({ mode: 'inclusive' }), FLOW_DECISION_UNCONDITIONAL_BRANCH)).toHaveLength(0); + }); + + it('reaches a decision inside a loop body', () => { + const fnds = ofRule(nestedModeFlow({ mode: 'inclusive' }), FLOW_DECISION_INCLUSIVE_OVERLAP); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toContain("loop 'each'"); + }); + }); +}); diff --git a/packages/lint/src/lint-flow-patterns.ts b/packages/lint/src/lint-flow-patterns.ts index 228a408b478..17fdb186d9f 100644 --- a/packages/lint/src/lint-flow-patterns.ts +++ b/packages/lint/src/lint-flow-patterns.ts @@ -159,6 +159,9 @@ import { collectFlowGraphs, } from '@objectstack/spec/automation'; import type { FlowNodeParsed, FlowEdgeParsed } from '@objectstack/spec/automation'; +// [#15429] The decision's `mode` contract, parsed here so `os validate` and +// `registerFlow` refuse the same declaration with the same sentence. +import { DecisionConfigSchema } from '@objectstack/spec/automation'; // [#5659] The Filter Protocol's boolean identity reduction — the same predicate // driver-sql, driver-mongodb and driver-memory execute. This linter asks it // rather than hand-writing a fourth copy; see {@link filterCarriesNoCondition}. @@ -230,6 +233,21 @@ export const FLOW_DEFAULT_EDGE_WITH_CONDITION = 'flow-default-edge-with-conditio export const FLOW_MULTIPLE_DEFAULT_EDGES = 'flow-multiple-default-edges'; /** #4414 — `config.condition` on a node whose executor never reads it. */ export const FLOW_INERT_NODE_CONDITION = 'flow-inert-node-condition'; +/** + * #15429 — a `decision` `config.mode` the spec refuses: a value outside + * `'exclusive' | 'inclusive'`, or a legal `mode` beside a non-empty + * `conditions` list (ruling A on #20168). `error`: `registerFlow` refuses the + * same declaration with the same sentence, so the flow can never arm. + */ +export const FLOW_DECISION_MODE_INVALID = 'flow-decision-mode-invalid'; +/** + * #15429 (ruling item 4) — a `decision` declaring `mode: 'inclusive'` with two + * or more conditioned out-edges: every edge whose condition holds runs, so + * where the conditions overlap more than one branch runs for one record. + * Advisory — that is exactly what the declaration asks for, and the rule + * cannot prove the conditions disjoint over CEL; it says what the shape does. + */ +export const FLOW_DECISION_INCLUSIVE_OVERLAP = 'flow-decision-inclusive-overlap'; /** * #5482 — a `delete_record` / `update_record` node that declares `multi: true` * and bounds it with NOTHING: the whole-object write, by declaration. @@ -752,6 +770,23 @@ function scanErrorLabelledEdges( * (5) `flow-inert-node-condition` — `config.condition` on a node that never * reads it. The key is the trigger gate on `start` and dead on every other * builtin, so the predicate reads like a guard and gates nothing. + * (6) `flow-decision-mode-invalid` (#15429) — a `config.mode` the spec's + * `DecisionConfigSchema` refuses: a value outside the closed pair, or a + * legal `mode` beside a non-empty `conditions` list, which is first-match + * on its own so the key would be accepted and never read (ruling A on + * #20168). The finding IS the schema's issue message, and `registerFlow` + * refuses the same shape with the same sentence. + * (7) `flow-decision-inclusive-overlap` (#15429, ruling item 4) — a decision + * declaring `mode: 'inclusive'` with two or more conditioned out-edges. + * An edge-branched decision is exclusive by default (the first true edge + * in declaration order wins); `inclusive` takes every one that holds, and + * where the conditions overlap that is more than one branch for one + * record. Beside (2), not a repeat of it: (2) is about an out-edge nothing + * gates, this is about gates that can all open. + * + * (6) GATES — the engine refuses the flow at registration with the same + * sentence, so a warning would just be a slower way of finding out. (7) stays + * advisory: it names what the declaration does, and the declaration is legal. * * (1) and (3) GATE — neither has a reading under which the author's metadata * routes what it says, on any run, so a warning would just be a slower way of @@ -869,6 +904,59 @@ function scanBranchRouting( ); const edgeLabels = new Set(outs.map(edgeLabelOf).filter(Boolean)); + // (6) #15429 — the decision's `mode`, judged by the spec's own contract. + // Only issues rooted at `mode` are reported here: the same parse also + // refuses an undeclared key, but that strictness binds at authoring by + // the standing decision in `schemaless-node-config.zod.ts`, and (5) + // above already owns the one such key an author reaches for. Read + // before (1)/(2): a decision whose `mode` is refused never registers, + // so what its branches would route is moot until the key is fixed — + // but the other findings still print, so the author fixes it once. + const modeVerdict = DecisionConfigSchema.safeParse(cfg); + if (!modeVerdict.success) { + for (const issue of modeVerdict.error.issues) { + if (issue.path[0] !== 'mode') continue; + findings.push({ + where: `${at} · decision '${nid}' · config.mode`, + message: issue.message, + hint: + `\`registerFlow\` refuses this flow with the same sentence, so it can never arm. Fix the node ` + + `in the flow definition: an omitted \`mode\` is exclusive (the first true out-edge wins), ` + + `\`mode: 'inclusive'\` takes every true out-edge, and a \`conditions\` list is first-match ` + + `on its own and takes no \`mode\`.`, + rule: FLOW_DECISION_MODE_INVALID, + // Gating: the engine refuses the same declaration at registration. + severity: 'error', + }); + } + } + + // (7) #15429 ruling item 4 — an inclusive gateway whose gates can all + // open. Counted over conditioned out-edges only (a `fault` edge is error + // routing and was dropped above; an `isDefault` edge opens only when + // nothing else did), so one conditioned edge plus a default is not + // this shape: there, inclusive and exclusive cannot differ. + if (cfg.mode === 'inclusive') { + const conditioned = outs.filter((e) => e.condition && conditionSource(e.condition).trim() !== ''); + if (conditioned.length >= 2) { + findings.push({ + where: `${at} · decision '${nid}'`, + message: + `declares \`mode: 'inclusive'\` with ${conditioned.length} conditioned out-edge(s) ` + + `(${conditioned.map((e) => `'${String(e.target)}'`).join(', ')}) — EVERY one whose condition ` + + `holds runs, one after another, so where the conditions overlap more than one branch runs ` + + `for one record. Nothing checks that they partition.`, + hint: + `If exactly one branch was meant, delete \`mode\`: an omitted \`mode\` is exclusive, and the ` + + `first out-edge whose condition holds, in declaration order, wins (mark the fallback ` + + `\`isDefault: true\`). Keep \`mode: 'inclusive'\` only where running every matching branch is ` + + `the intent. A flow migrated by \`os migrate meta --from 17\` carries this key wherever two ` + + `or more conditioned out-edges left a decision — delete it where the branches partition.`, + rule: FLOW_DECISION_INCLUSIVE_OVERLAP, + }); + } + } + // (1) a declared branch label nothing claims. `default` is the engine's own // sentinel for "no declared condition matched" and is additionally // claimed by the BPMN default edge, so it is never counted as unclaimed. diff --git a/packages/metadata-core/src/artifact-forward-conversion.test.ts b/packages/metadata-core/src/artifact-forward-conversion.test.ts index 207575e6717..0a748a28e54 100644 --- a/packages/metadata-core/src/artifact-forward-conversion.test.ts +++ b/packages/metadata-core/src/artifact-forward-conversion.test.ts @@ -17,7 +17,7 @@ */ import { describe, it, expect } from 'vitest'; -import { ObjectStackDefinitionSchema, applyConversionsToStoredItem, type ConversionNotice } from '@objectstack/spec'; +import { ObjectStackDefinitionSchema, applyConversions, applyConversionsToStoredItem, type ConversionNotice } from '@objectstack/spec'; import { applyArtifactForwardConversions, parseRangeFloor, @@ -401,6 +401,108 @@ describe('the artifact door never turns an authored `hidden: true` into an unpub }); }); +/** + * #15429 — the second member of the DEFAULT-FLIP class this door refuses. + * + * `flow-decision-mode-inclusive-explicit` writes `mode: 'inclusive'` onto an + * edge-branched `decision` with two or more conditioned out-edges, so a flow + * written while every true branch ran keeps that behaviour now that the + * traversal is exclusive. An omitted `mode` IS the exclusive gateway by the + * contract on `DecisionConfigSchema`, so the rewrite reinterprets a legal + * shape — sound only where the source's age is a fact (`os migrate meta + * --from 17`, the operator's assertion). At this door the trigger is the + * artifact's declared `engines.protocol` floor, and `^17.0.0` is what + * `create-objectstack` stamps: an app scaffolded today, authored against the + * exclusive contract, lands inside the window. Replaying the entry here would + * hand it an inclusive gateway it never asked for — the #17885 shape, on a + * key whose omission is the ruled default. + * + * Same four legs as the block above: SUBJECT (window open, no `mode` + * written) · the strict parse the door feeds · NEGATIVE (window shut) · + * FIRING CONTROL (the entry WOULD rewrite this very fixture with the window + * open and no refusal, so the subject leg cannot pass vacuously). + */ +describe('the artifact door never writes `mode: inclusive` onto an authored exclusive decision (#15429)', () => { + /** One edge-branched decision with two conditioned out-edges and no `mode` — the shape the entry rewrites. */ + const twoBranchDecisionDefinition = (protocolRange: string) => ({ + manifest: { + id: 'app.example.leads', name: 'leads', version: '1.0.0', type: 'app', + engines: { protocol: protocolRange }, + }, + flows: [{ + name: 'lead_verdict', + label: 'Lead verdict', + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'verdict', type: 'decision', label: 'Verdict?' }, + { id: 'refuse', type: 'end', label: 'Refuse' }, + { id: 'convert', type: 'end', label: 'Convert' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'verdict' }, + { id: 'e2', source: 'verdict', target: 'refuse', condition: "lead.status != 'suspected'", label: 'Refuse' }, + { id: 'e3', source: 'verdict', target: 'convert', condition: "lead.status == 'confirmed'", label: 'Convert' }, + ], + }], + }); + const ID = 'flow-decision-mode-inclusive-explicit'; + const verdictNodeOf = (definition: unknown) => + (definition as { flows: { nodes: { id: string; config?: Record }[] }[] }) + .flows[0]!.nodes.find((n) => n.id === 'verdict')!; + + it('leaves the decision without `mode` on an artifact the retired window IS open for', () => { + const def = twoBranchDecisionDefinition('^17.0.0'); + const result = applyArtifactForwardConversions(def, { runtimeSpecVersion: '17.4.0' }); + + // ⭐ ANTI-VACUITY: the window really is open on this input. + expect(result.verdict).toBe('converted-forward'); + expect(result.authoredFloor).toBe('17.0.0'); + + const verdict = verdictNodeOf(result.definition); + expect(verdict.config, 'the authored exclusive gateway is untouched').toBeUndefined(); + expect(result.notices.map((n) => n.conversionId)).not.toContain(ID); + // Copy-on-write: nothing was recognized, so the same reference comes back. + expect(result.definition).toBe(def); + }); + + it('registers the decision without `mode` — asserted AFTER the strict parse the door feeds', () => { + const def = twoBranchDecisionDefinition('^17.0.0'); + const result = applyArtifactForwardConversions(def, { runtimeSpecVersion: '17.4.0' }); + expect(result.verdict).toBe('converted-forward'); + + const parsed = ObjectStackDefinitionSchema.parse(result.definition); + const registered = verdictNodeOf(parsed); + expect(Object.keys(registered.config ?? {}), 'what registration receives').not.toContain('mode'); + }); + + it('floor ^99.0.0 — the window is shut and nothing is replayed at all', () => { + const def = twoBranchDecisionDefinition('^99.0.0'); + const result = applyArtifactForwardConversions(def, { runtimeSpecVersion: '17.4.0' }); + expect(result.verdict).toBe('authored-current'); + expect(result.notices).toEqual([]); + expect(verdictNodeOf(result.definition).config).toBeUndefined(); + }); + + /** + * ⭐ FIRING CONTROL — the entry WOULD rewrite this exact fixture: the same + * bytes through the primitive with the retired window open and no refusal + * come back inclusive, with the entry's own notice. A door that had merely + * stopped recognizing the shape, or a fixture the entry never matched, would + * go green above and prove nothing; this leg is what makes the subject a + * reading of the refusal. + */ + it('FIRING CONTROL — without the refusal, the same fixture IS rewritten to `mode: inclusive`', () => { + const notices: ConversionNotice[] = []; + const rewritten = applyConversions(twoBranchDecisionDefinition('^17.0.0'), { + includeRetired: true, + onNotice: (n) => notices.push(n), + }); + expect(verdictNodeOf(rewritten).config).toEqual({ mode: 'inclusive' }); + expect(notices.map((n) => n.conversionId)).toContain(ID); + }); +}); + describe('parseRangeFloor — the range spellings artifacts actually carry', () => { it.each([ ['^17.1.0', [17, 1, 0]], diff --git a/packages/metadata-core/src/artifact-forward-conversion.ts b/packages/metadata-core/src/artifact-forward-conversion.ts index 309b84443aa..16742f49e72 100644 --- a/packages/metadata-core/src/artifact-forward-conversion.ts +++ b/packages/metadata-core/src/artifact-forward-conversion.ts @@ -287,8 +287,30 @@ export function resolveInstalledSpecVersion(): string | null { * materialization path, and `os migrate meta --from <=16`, where the * operator asserts the source's age) — this door is neither, so it opts out * rather than the entry ceasing to fire. + * - `flow-decision-mode-inclusive-explicit` (#15429, maintainer ruling + * 「跟主流对齐」): writes `mode: 'inclusive'` onto an edge-branched `decision` + * (no `conditions` list) that has two or more conditioned out-edges, so a + * flow written while every true branch ran keeps that behaviour now that the + * traversal is exclusive (first true edge in declaration order). Both shapes + * are legal and mean different things — an omitted `mode` IS the exclusive + * gateway, by the contract on `DecisionConfigSchema` — so the rewrite is a + * reinterpretation, sound only where "this source predates the flip" is a + * fact. Here it is a guess: the trigger is the artifact's declared + * `engines.protocol` floor, and `^17.0.0` is the range `create-objectstack` + * stamps, so an app scaffolded today, whose author wrote two branches + * against the contract that says an omitted `mode` is exclusive, lands + * inside the window and would be handed an inclusive gateway it never + * asked for — the ruled default made unobservable for every artifact-deployed + * app. The entry is sound at exactly one seam: `os migrate meta --from 17`, + * where the operator asserts the source's age and reviews the diff. The + * automation engine's flow rehydration seam refuses it by id for the same + * reason (`CONVERSIONS_NOT_REPLAYED_AT_REHYDRATION` in + * `packages/services/service-automation/src/engine.ts`); this door does too. */ -const DEFAULT_FLIPS_NOT_REPLAYED_HERE: readonly string[] = ['app-hidden-to-unpublished']; +const DEFAULT_FLIPS_NOT_REPLAYED_HERE: readonly string[] = [ + 'app-hidden-to-unpublished', + 'flow-decision-mode-inclusive-explicit', +]; /** * Apply the versioned forward conversion to one compiled-artifact definition. diff --git a/packages/metadata-protocol/src/index.ts b/packages/metadata-protocol/src/index.ts index 294b1be9ad5..ca7e60d48cd 100644 --- a/packages/metadata-protocol/src/index.ts +++ b/packages/metadata-protocol/src/index.ts @@ -159,6 +159,7 @@ export type { export { formatStoredMigrationReport, storedMigrationClean } from './stored-migration.js'; export type { + StoredDecisionModeReview, StoredFlowCanonicalization, StoredMigrationNotice, StoredMigrationOutcome, diff --git a/packages/metadata-protocol/src/protocol.stored-migration.test.ts b/packages/metadata-protocol/src/protocol.stored-migration.test.ts index 8329120bb1a..bc12317fcd8 100644 --- a/packages/metadata-protocol/src/protocol.stored-migration.test.ts +++ b/packages/metadata-protocol/src/protocol.stored-migration.test.ts @@ -26,8 +26,14 @@ import { describe, expect, it } from 'vitest'; // of this package's (file, verb) pairs sat in the gate's DEBT ledger until // #5619 sank the two predicates into a package both sides already depend on. import { assertEngineDeleteDispatch, assertEngineUpdateDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core'; +import { applyMetaMigrations } from '@objectstack/spec'; import { ObjectStackProtocolImplementation } from './protocol.js'; -import { formatStoredMigrationReport, storedMigrationClean } from './stored-migration.js'; +import { + DECISION_MODE_REVIEW_CONVERSION_ID, + collectDecisionModeReview, + formatStoredMigrationReport, + storedMigrationClean, +} from './stored-migration.js'; interface Row { id: string; @@ -803,3 +809,221 @@ describe('migrateStoredMetadata — a site the chain leaves as stored is a TODO, expect(again.canonical).toBe(0); }); }); + +describe('migrateStoredMetadata — the decision review list: stored rows take first-match, and are LISTED, never rewritten (#15429, ruling C)', () => { + // Maintainer ruling letter C on #15429: a stored decision with no + // `conditions` list, no `mode` and two or more conditioned out-edges takes + // the protocol-18 meaning (first match) on upgrade — no stored-row rewrite, + // no cutoff, no read-path completion — and `os migrate meta --stored` lists + // every such node, report only, so an operator can review candidates + // before and after the upgrade. + // + // The hotcrm#1555 shape: both predicates hold for a confirmed lead. + const OVERLAP = { a: "lead.status != 'suspected'", b: "lead.status == 'confirmed'" }; + const gatewayBody = (name: string, opts: { + config?: Record; + b?: string; + bType?: string; + withDefault?: boolean; + withLegacyPurge?: boolean; + } = {}) => ({ + name, + label: 'Lead Verdict', + type: 'autolaunched', + status: 'active', + nodes: [ + // A second, unrelated pre-protocol shape the canonicalizer rewrites, + // so an apply run DOES write this row — and must still write no `mode`. + ...(opts.withLegacyPurge + ? [{ id: 'purge', type: 'delete_record', label: 'Purge', config: { objectName: 'lead', filters: { status: 'stale' } } }] + : []), + { id: 'check', type: 'decision', label: 'Verdict?', ...(opts.config ? { config: opts.config } : {}) }, + { id: 'refuse', type: 'end', label: 'Refuse' }, + { id: 'convert', type: 'end', label: 'Convert' }, + ...(opts.withDefault ? [{ id: 'fallback', type: 'end', label: 'Fallback' }] : []), + ], + edges: [ + { id: 'e_refuse', source: 'check', target: 'refuse', condition: OVERLAP.a }, + { + id: 'e_convert', source: 'check', target: 'convert', + ...(opts.bType ? { type: opts.bType } : {}), + ...(opts.b === '' ? {} : { condition: opts.b ?? OVERLAP.b }), + }, + ...(opts.withDefault ? [{ id: 'e_fallback', source: 'check', target: 'fallback', isDefault: true }] : []), + ], + }); + const flowRow = (name: string, body: unknown, extra: Record = {}) => + ({ type: 'flow', name, metadata: body, ...extra }); + /** + * Stands in for `AutomationEngine.canonicalizeStoredFlow`, and answers what + * the real seam answers for these bodies: it refuses the D2 entry by id, so + * a two-branch decision comes back WITHOUT `mode`. It rewrites only the + * legacy `filters` alias, so a row carrying one is a real rewrite. + */ + const canonicalizeFlow = (_name: string, body: any) => { + const purge = body?.nodes?.find((n: any) => n.id === 'purge'); + if (!purge || !('filters' in (purge.config ?? {}))) return { storable: body, notices: [], conflicts: [] }; + return { + storable: { + ...body, + nodes: body.nodes.map((n: any) => (n === purge + ? { ...n, config: { objectName: 'lead', filter: purge.config.filters } } + : n)), + }, + notices: [{ + conversionId: 'flow-node-crud-filter-alias', + surface: 'flow.node.config.filter', + from: 'filters', + to: 'filter', + path: 'flows[0].nodes[0].config', + message: 'filters → filter', + }], + conflicts: [], + }; + }; + + it('a stored two-branch decision with no `mode` is LISTED — row, flow, node, label and path — and the row is canonical', async () => { + const { engine, tables } = makeStubEngine([flowRow('lead_verdict', gatewayBody('lead_verdict'), { organization_id: 'org_1' })]); + const before = JSON.stringify(metaRows(tables)); + const protocol = new ObjectStackProtocolImplementation(engine); + + const report = await protocol.migrateStoredMetadata({ canonicalizeFlow }); + + expect(report.decisionModeReview).toEqual([{ + id: metaRows(tables)[0]!.id, + name: 'lead_verdict', + organizationId: 'org_1', + packageId: null, + state: 'active', + nodeId: 'check', + nodeLabel: 'Verdict?', + path: 'nodes[0]', + }]); + // ON protocol: listed, not pending — the list moves no count and no verdict. + expect(report).toMatchObject({ scanned: 1, canonical: 1, pending: 0, skipped: 0, failed: 0, rows: [] }); + expect(storedMigrationClean(report)).toBe(true); + expect(JSON.stringify(metaRows(tables))).toBe(before); + }); + + it('`--apply` changes nothing for it: no rewrite, no history row, the stored bytes identical — and the list is the same', async () => { + const { engine, tables } = makeStubEngine([flowRow('lead_verdict', gatewayBody('lead_verdict'))]); + const before = JSON.stringify(metaRows(tables)); + const protocol = new ObjectStackProtocolImplementation(engine); + + const report = await protocol.migrateStoredMetadata({ apply: true, canonicalizeFlow }); + + expect(report.decisionModeReview.map((r) => `${r.name}:${r.nodeId}`)).toEqual(['lead_verdict:check']); + expect(report).toMatchObject({ canonical: 1, rewritten: 0, failed: 0 }); + expect(historyRows(tables)).toHaveLength(0); + expect(JSON.stringify(metaRows(tables))).toBe(before); + expect(JSON.parse(metaRows(tables)[0]!.metadata).nodes[0]).not.toHaveProperty('config'); + }); + + it('a row `--apply` DOES rewrite for another conversion persists no `mode` — the list never rides into the write', async () => { + const { engine, tables } = makeStubEngine([flowRow('lead_verdict', gatewayBody('lead_verdict', { withLegacyPurge: true }))]); + const protocol = new ObjectStackProtocolImplementation(engine); + + const report = await protocol.migrateStoredMetadata({ apply: true, canonicalizeFlow }); + + expect(report.rewritten).toBe(1); + expect(report.rows[0]!.notices.map((n) => n.conversionId)).toEqual(['flow-node-crud-filter-alias']); + const stored = JSON.parse(metaRows(tables)[0]!.metadata); + expect(stored.nodes.find((n: any) => n.id === 'purge').config).toEqual({ objectName: 'lead', filter: { status: 'stale' } }); + expect(stored.nodes.find((n: any) => n.id === 'check')).not.toHaveProperty('config'); + expect(report.decisionModeReview.map((r) => `${r.nodeId}@${r.path}`)).toEqual(['check@nodes[1]']); + }); + + it('needs no engine: with no canonicalizer the flow row is `skipped` and its decision is STILL listed', async () => { + const { engine } = makeStubEngine([flowRow('lead_verdict', gatewayBody('lead_verdict'))]); + const protocol = new ObjectStackProtocolImplementation(engine); + + const report = await protocol.migrateStoredMetadata(); + + expect(report.skipped).toBe(1); + expect(report.rows[0]!.reason).toMatch(/no automation service/); + expect(report.decisionModeReview.map((r) => r.nodeId)).toEqual(['check']); + }); + + it('lists a decision inside a loop body, judged against the region\'s own edges, at its region path', async () => { + const body = { + name: 'lead_sweep', + label: 'Sweep', + type: 'autolaunched', + status: 'active', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'sweep', type: 'loop', label: 'Sweep', + config: { + collection: '{leads}', + iteratorVariable: 'lead', + body: { + nodes: [ + { id: 'gate', type: 'decision' }, + { id: 'x', type: 'end', label: 'X' }, + { id: 'y', type: 'end', label: 'Y' }, + ], + edges: [ + { id: 'g1', source: 'gate', target: 'x', condition: OVERLAP.a }, + { id: 'g2', source: 'gate', target: 'y', condition: { dialect: 'cel', source: OVERLAP.b } }, + ], + }, + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'sweep' }], + }; + + expect(collectDecisionModeReview(body)).toEqual([{ nodeId: 'gate', path: 'nodes[1].config.body.nodes[0]' }]); + }); + + it('CONTROLS — a declared `mode` (either member), one conditioned edge beside a default, a `conditions` list and a `fault` second edge are NOT listed', () => { + expect(collectDecisionModeReview(gatewayBody('f', { config: { mode: 'exclusive' } }))).toEqual([]); + expect(collectDecisionModeReview(gatewayBody('f', { config: { mode: 'inclusive' } }))).toEqual([]); + expect(collectDecisionModeReview(gatewayBody('f', { b: '', withDefault: true }))).toEqual([]); + expect(collectDecisionModeReview(gatewayBody('f', { + config: { conditions: [{ label: 'Refuse', expression: OVERLAP.a }] }, + }))).toEqual([]); + expect(collectDecisionModeReview(gatewayBody('f', { bType: 'fault' }))).toEqual([]); + // …and the POSITIVE for the same builder, so the controls cannot pass vacuously. + expect(collectDecisionModeReview(gatewayBody('f')).map((r) => r.nodeId)).toEqual(['check']); + }); + + it('ONE PREDICATE — the list is exactly what the `--from 17` chain writes `mode` at, through the same registry entry', () => { + const body = gatewayBody('lead_verdict'); + const bodyBytes = JSON.stringify(body); + + const listed = collectDecisionModeReview(body).map((r) => `flows[0].${r.path}.config.mode`); + const chain = applyMetaMigrations({ flows: [body] }, 17, 18); + const written = chain.applied + .filter((a) => a.conversionId === DECISION_MODE_REVIEW_CONVERSION_ID) + .map((a) => a.path); + + expect(written).toEqual(['flows[0].nodes[0].config.mode']); + expect(listed).toEqual(written); + // The listing ran the entry and discarded its output: the body is untouched. + expect(JSON.stringify(body)).toBe(bodyBytes); + }); + + it('the renderer prints the list with the one-line fix, beside — not instead of — the on-protocol verdict', async () => { + const { engine } = makeStubEngine([flowRow('lead_verdict', gatewayBody('lead_verdict'), { state: 'draft' })]); + const protocol = new ObjectStackProtocolImplementation(engine); + + const text = formatStoredMigrationReport(await protocol.migrateStoredMetadata({ canonicalizeFlow })).join('\n'); + + expect(text).toMatch(/1 decision node\(s\) in 1 flow row\(s\) take the FIRST matching branch since protocol 18/); + expect(text).toContain(`flow/lead_verdict [env-wide, draft] — decision 'check' "Verdict?" at nodes[0]`); + expect(text).toContain("declare `mode: 'inclusive'`"); + expect(text).toMatch(/already on protocol/); + }); + + it('CONTROL — a run with nothing to review prints no review section', async () => { + const { engine } = makeStubEngine([flowRow('lead_verdict', gatewayBody('lead_verdict', { config: { mode: 'exclusive' } }))]); + const protocol = new ObjectStackProtocolImplementation(engine); + + const report = await protocol.migrateStoredMetadata({ canonicalizeFlow }); + + expect(report.decisionModeReview).toEqual([]); + expect(formatStoredMigrationReport(report).join('\n')).not.toMatch(/FIRST matching branch/); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 9c5b9aec575..711fded5dde 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -176,6 +176,7 @@ import type { StoredMigrationRow, StoredMigrationTodo, } from './stored-migration.js'; +import { collectDecisionModeReview } from './stored-migration.js'; /** * Canonical Zod schema per metadata type lives in @@ -16682,6 +16683,20 @@ export class ObjectStackProtocolImplementation implements * - `source: 'migrate-stored'` — so a history diff distinguishes a * canonicalization pass from an edit someone made. * + * ## What it lists and never writes + * + * `decisionModeReview` (#15429, maintainer ruling letter C): every stored + * `decision` node with no `conditions` list, no `mode` and two or more + * conditioned out-edges. Such a node took every true branch before + * protocol 18 and takes the first one now, and a stored row keeps that + * new meaning — the D2 entry that writes `mode: 'inclusive'` replays only + * over authored sources (`os migrate meta --from 17`), where the operator + * asserts the source's age; nothing asserts a row's. The list is the + * operator's review of that change, before and after the upgrade: it is + * built from the stored body with the D2 entry's own predicate + * ({@link collectDecisionModeReview}), it is the same on preview and apply, + * and it moves no row outcome, no count and no verdict. + * * ## What it declines to touch, and says so * * This section documents the function's FULL internal surface, which is @@ -16808,6 +16823,7 @@ export class ObjectStackProtocolImplementation implements skipped: 0, failed: 0, rows: [], + decisionModeReview: [], }; // Two scoped queries rather than one unfiltered scan: `state` is an @@ -16914,6 +16930,29 @@ export class ObjectStackProtocolImplementation implements continue; } + // [#15429, ruling C] The decision review list — REPORT ONLY. A stored + // decision with no `conditions` list, no `mode` and two or more + // conditioned out-edges took every true branch before protocol 18 + // and takes the first one now; by ruling the row keeps that new + // meaning (no pass rewrites it — nothing can say the row predates + // the flip), and this names each such node so an operator can find + // the one that MEANT every branch. Read off the stored body BEFORE + // the canonicalizer and with no engine, so the list is the same + // whether the row below converts, is skipped for want of an + // engine, or fails; it touches no count, no outcome and no write. + if (singular === 'flow') { + for (const node of collectDecisionModeReview(body)) { + report.decisionModeReview.push({ + id: base.id, + name: base.name, + organizationId, + packageId, + state, + ...node, + }); + } + } + // Flow rows need the automation engine's live executor registry for // ADR-0078's open-namespace conflict guard — supplied by the caller // (#4454) or resolved from the services registry (#4498), and diff --git a/packages/metadata-protocol/src/stored-migration.ts b/packages/metadata-protocol/src/stored-migration.ts index 91b5d28ab74..38e8687df66 100644 --- a/packages/metadata-protocol/src/stored-migration.ts +++ b/packages/metadata-protocol/src/stored-migration.ts @@ -27,6 +27,8 @@ * that reports every row canonical is the evidence, and it costs one command. */ +import { ALL_CONVERSIONS } from '@objectstack/spec'; + /** * What a caller with a live automation engine hands back for a stored `flow` * body (#4454) — structurally `AutomationEngine.canonicalizeStoredFlow`'s @@ -141,6 +143,46 @@ export interface StoredMigrationRow { reason?: string; } +/** + * One stored `decision` node whose evaluation changed meaning at protocol 18 + * (#15429) — LISTED for an operator to review, never rewritten. + * + * The shape: a decision with no `config.conditions` list, no `mode`, and two or + * more out-edges carrying a `condition`. Before protocol 18 such a node took + * EVERY out-edge whose condition held; it now takes the first one, in the order + * the flow declares its edges. By maintainer ruling (letter C on #15429) a + * stored row takes that new meaning on upgrade: `os migrate meta --from 17` + * writes `mode: 'inclusive'` onto an authored SOURCE of this shape, because the + * operator asserts the source's age there, and no pass writes it onto a stored + * row, whose age nothing can assert. So the row is on protocol as it stands + * — this entry is not residue, it does not make the row `pending`, and it never + * flips {@link storedMigrationClean}; it is the list an operator reads before + * and after the upgrade to find the one node in a hundred that MEANT every + * branch, and declares `mode: 'inclusive'` on it by hand. + * + * The predicate is not restated here: {@link collectDecisionModeReview} runs + * the ADR-0087 D2 entry `flow-decision-mode-inclusive-explicit` itself over the + * stored body and keeps only where it would write — so this list is, by + * construction, the set of nodes the `--from 17` chain rewrites in a source. + */ +export interface StoredDecisionModeReview { + /** `sys_metadata.id` of the flow row. */ + id: string; + /** The flow's name. */ + name: string; + /** `null` = the env-wide overlay bucket. */ + organizationId: string | null; + /** `null` = a package-less (global) overlay row. */ + packageId: string | null; + state: 'active' | 'draft'; + /** The decision node's `id`. */ + nodeId: string; + /** The node's `label`, when it has one — what the flow designer shows. */ + nodeLabel?: string; + /** Where the node sits in the stored body, e.g. `nodes[3]` or `nodes[1].config.body.nodes[0]`. */ + path: string; +} + /** The whole run. `apply: false` is a preview — it writes nothing, by construction. */ export interface StoredMigrationReport { /** False = preview. A preview never writes, not even a row it would leave identical. */ @@ -159,6 +201,14 @@ export interface StoredMigrationReport { failed: number; /** Every row that is not `canonical`, in scan order. */ rows: StoredMigrationRow[]; + /** + * Stored `decision` nodes that take first-match since protocol 18 and were + * written before anyone had to say otherwise (#15429, ruling C) — in scan + * order, whatever each row's outcome, on a preview and an apply run alike. + * REPORT ONLY: nothing in this list is ever written, and it moves no count + * and no verdict. See {@link StoredDecisionModeReview}. + */ + decisionModeReview: StoredDecisionModeReview[]; } /** @@ -191,11 +241,92 @@ export interface StoredMigrationReport { * does not flip this verdict. TODOs never move it in either direction: a row * that also converts something stays `pending` / `rewritten` / `failed` exactly * as it would without them. + * + * The decision review list ({@link StoredMigrationReport.decisionModeReview}) + * is not a skip class and never moves this verdict either: a row it names is ON + * protocol — by ruling it takes the protocol-18 meaning as stored — so there is + * nothing for any run of this pass to do about it. */ export function storedMigrationClean(report: StoredMigrationReport): boolean { return report.pending === 0 && report.failed === 0; } +/** + * The ADR-0087 D2 entry whose predicate {@link collectDecisionModeReview} runs + * (#15429): the conversion `os migrate meta --from 17` replays over authored + * sources to write `mode: 'inclusive'`. + */ +export const DECISION_MODE_REVIEW_CONVERSION_ID = 'flow-decision-mode-inclusive-explicit'; + +/** + * The decision nodes in ONE stored flow body that the review list names — see + * {@link StoredDecisionModeReview} for what they are and why a stored row + * keeps them as they are (#15429, ruling C). + * + * **One predicate, not two.** The D2 entry's own `apply` runs over + * `{ flows: [body] }` and only the paths it WOULD write `mode` at are kept; the + * stack it returns is discarded unread, so nothing here can reach a write, and + * the entry is copy-on-write, so the body is not touched either. The list is + * therefore exactly the set a `--from 17` replay rewrites in a source — regions + * included — with no second statement of "two or more conditioned out-edges" + * to drift from the first. + * + * **Read off the stored body, with no engine.** The entry renames no node type, + * so it needs no executor registry, and it runs before — and independently of — + * the flow canonicalizer: a host with no automation service (where the flow row + * itself is reported `skipped`) and a row that fails to canonicalize still + * list their decisions. No earlier conversion writes a decision's `mode` or + * `conditions` or an edge's `condition`, so the stored body and the canonical + * one give the same answer. + * + * ⛔ Throws when the entry is gone from the registry, or names a path of a + * shape this reader does not know: a list that silently came back empty would + * tell every deployment "no decision to review". The entry leaves the registry + * only when the chain floor passes protocol 17 — the upgrade this list serves + * is then outside the supported window, and the list goes with it. + */ +export function collectDecisionModeReview( + body: unknown, +): Array<{ nodeId: string; nodeLabel?: string; path: string }> { + if (body === null || typeof body !== 'object' || Array.isArray(body)) return []; + const entry = ALL_CONVERSIONS.find((c) => c.id === DECISION_MODE_REVIEW_CONVERSION_ID); + if (!entry) { + throw new Error( + `the decision review list runs the conversion '${DECISION_MODE_REVIEW_CONVERSION_ID}', and the ` + + 'registry no longer carries it. Remove the review list together with the conversion.', + ); + } + const paths: string[] = []; + entry.apply({ flows: [body as Record] }, (detail) => { + paths.push(detail.path); + }); + return paths.map((path) => { + const match = /^flows\[0\]\.(.+)\.config\.mode$/.exec(path); + if (!match) { + throw new Error( + `the conversion '${DECISION_MODE_REVIEW_CONVERSION_ID}' reported a path of an unknown shape ` + + `('${path}'), so the decision review list cannot name the node it is about.`, + ); + } + const nodePath = match[1]!; + const node = readAtPath(body, nodePath) as { id?: unknown; label?: unknown } | undefined; + const label = typeof node?.label === 'string' && node.label.trim() !== '' ? node.label : undefined; + return { nodeId: String(node?.id ?? ''), ...(label ? { nodeLabel: label } : {}), path: nodePath }; + }); +} + +/** Read `nodes[1].config.body.nodes[0]`-style paths — the spelling the conversions emit. */ +function readAtPath(root: unknown, path: string): unknown { + let current: unknown = root; + for (const step of path.matchAll(/([^.[\]]+)|\[(\d+)\]/g)) { + if (current === null || typeof current !== 'object') return undefined; + current = step[2] !== undefined + ? (current as unknown[])[Number(step[2])] + : (current as Record)[step[1]!]; + } + return current; +} + /** * Render a run for a terminal. One line per non-canonical row, its notices and * its TODOs nested under it — wherever the row is listed, since a TODO rides on @@ -259,6 +390,29 @@ export function formatStoredMigrationReport(report: StoredMigrationReport): stri ); } + // Printed on a preview and an apply run alike, and beside the "already on + // protocol" verdict below without contradicting it: these rows ARE on + // protocol — a stored decision takes the protocol-18 meaning as it stands — + // and the list is what an operator reviews, not what this pass owes. + const review = report.decisionModeReview; + if (review.length > 0) { + const flowRows = new Set(review.map((r) => r.id)).size; + lines.push( + `◆ ${review.length} decision node(s) in ${flowRows} flow row(s) take the FIRST matching branch ` + + 'since protocol 18 — listed for review, never rewritten:', + ); + for (const r of review) { + const label = r.nodeLabel ? ` "${r.nodeLabel}"` : ''; + lines.push(` • flow/${r.name} ${describeScope(r)} — decision '${r.nodeId}'${label} at ${r.path}`); + } + lines.push( + ' Each has two or more out-edges with a condition and no `mode`. Before protocol 18 it took ' + + 'EVERY out-edge whose condition held; it now takes only the first one that holds, in the order ' + + "the flow declares its edges. Where a node meant every branch, declare `mode: 'inclusive'` on " + + 'it; declaring `mode` either way takes it off this list. No run of this pass writes it.', + ); + } + if (report.scanned === 0) { // "Nothing to convert" and "nothing was looked at" are different claims, // and only the first is a pass. A run pointed at the wrong project — the @@ -287,7 +441,7 @@ function pushTodos(lines: string[], row: StoredMigrationRow): void { } /** `[org=… package=… draft]` — only the parts that are not the default. */ -function describeScope(row: StoredMigrationRow): string { +function describeScope(row: Pick): string { const parts: string[] = []; parts.push(row.organizationId ? `org=${row.organizationId}` : 'env-wide'); if (row.packageId) parts.push(`package=${row.packageId}`); diff --git a/packages/services/service-automation/src/builtin/decision-overlapping-edge-conditions.pin.test.ts b/packages/services/service-automation/src/builtin/decision-overlapping-edge-conditions.pin.test.ts index 21310b3508f..254b845cf2a 100644 --- a/packages/services/service-automation/src/builtin/decision-overlapping-edge-conditions.pin.test.ts +++ b/packages/services/service-automation/src/builtin/decision-overlapping-edge-conditions.pin.test.ts @@ -1,65 +1,56 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, beforeEach } from 'vitest'; +import { ALL_CONVERSIONS, applyMetaMigrations } from '@objectstack/spec'; import { AutomationEngine } from '../engine.js'; import { registerLogicNodes } from './logic-nodes.js'; /** - * ⭐ A STATUS-QUO PIN, NOT A CONTRACT (#15429). + * ⭐ THE CONTRACT PIN for an edge-branched `decision` (#15429). * - * Every assertion below records what the engine **does today** when two - * out-edges of one `decision` node carry conditions that both hold. None of it - * says the behaviour is right, and nothing here blesses it. The card that asked - * for this file is explicit that changing evaluation semantics on a shipped node - * type is a behaviour change with its own ruling, ⛔ not a bug fix — so the - * first step is a baseline that cannot drift while that ruling is pending. + * This file began life as a STATUS-QUO pin: it recorded that a decision with + * no declared `config.conditions` took EVERY out-edge whose condition held, + * one after another, and reported nothing — the hotcrm#1555 hazard — under a + * header that said "when the ruling lands, this file is rewritten with it". + * The ruling landed (maintainer, 2026-09-23, 「跟主流对齐」), and this is that + * rewrite. Every assertion below is the contract now, not a measurement of + * drift: * - * ⇒ When the ruling lands, **this file is rewritten with it**. A failure here - * after a deliberate semantic change is the pin doing its job, not a regression: - * update the assertions and the prose together. A failure here after a change - * that did NOT intend to touch branch selection is the drift it exists to catch. + * 1. **Exclusive by default.** The conditioned out-edges are evaluated in the + * order the flow's `edges` array declares them and the FIRST one whose + * condition holds is the branch — the BPMN exclusive gateway, Salesforce + * Flow's Decision, n8n's Switch default. Its later siblings are passed over + * unevaluated and record the same `skipped` step a closed gate does. + * 2. **Inclusive by declaration.** `config.mode: 'inclusive'` takes every + * out-edge whose condition holds, sequentially — never `Promise.all`; that + * fan-out belongs to the unconditional bucket, kept below as the positive + * control for the interleaving instrument. + * 3. **`isDefault` is unchanged**: it runs when no conditioned sibling did, + * in either mode. + * 4. **`mode` is judged at registration** through the spec's own + * `DecisionConfigSchema`: a value outside the closed pair, or a `mode` + * beside a non-empty `conditions` list, refuses the flow with the schema's + * sentence — the same one `os validate` prints. + * 5. **The rehydration seam does not rewrite the flip, and a stored row takes + * the new meaning.** The ADR-0087 conversion + * `flow-decision-mode-inclusive-explicit` writes `mode: 'inclusive'` onto a + * two-branch decision so an OLD authored source keeps its behaviour, but + * only where the operator asserts the source's age (`os migrate meta + * --from 17`); `registerFlow` cannot date a body and refuses the entry by + * id, or every NEW exclusive decision would register as an inclusive one. + * So a decision stored in `sys_metadata` before protocol 18, with no + * `mode` and overlapping conditions, runs FIRST-MATCH after the upgrade — + * by maintainer ruling letter C on #15429 (no stored-row rewrite, no + * cutoff, no read-path completion); `os migrate meta --stored` lists such + * nodes for review and writes nothing. + * 6. **Scoped to `decision`.** Conditioned out-edges of any other node type + * keep the every-true-edge traversal they had. * - * What the card reported, and what this file measured against it: - * - * • REPORTED (from one hotcrm deployment, reasoned backwards from a - * reproduction): a decision with no declared `config.conditions` takes every - * out-edge whose condition holds, **in parallel**. - * • MEASURED here: the take-every-match half is real. The **in parallel** half - * is not — matching conditional edges are traversed one at a time, each - * successor fully executed before the next is evaluated (`traverseNext` - * awaits inside the loop). Only the *unconditional* bucket fans out through - * `Promise.all`, and this file keeps that fan-out as a positive control so - * the interleaving instrument is shown to detect parallelism where it exists. - * - * The distinction matters for whoever writes the ruling: the hazard is - * multi-branch execution, not concurrency. Two branches that both write the same - * record run in a defined order, which is a different (and easier) starting - * point than a genuine race. - * - * The two modes are separate on purpose (`logic-nodes.ts`, #4414): a decision - * that DECLARES `config.conditions` reports the first matching entry's label and - * traversal narrows to the edge carrying it; a decision that declares none - * reports no branch at all and is a plain gateway whose out-edges route. The - * last two tests pin both halves of that split, because "is undeclared a - * distinct mode?" is the other question the ruling has to start from. - * - * On provenance, stated because the answer is "none": the take-every-match loop - * predates the buckets it lives in. Before `cc8484224` (2026-02-21) traversal - * was one loop that `continue`d past a closed gate and executed everything else; - * that commit split conditional from unconditional edges, recorded a decision - * about the unconditional half ("parallel branch execution (Promise.all for - * unconditional edges)") and carried the conditional half over unchanged, under - * a new comment reading `// Conditional edges: evaluate sequentially (mutually - * exclusive)`. That comment is the only written trace of the assumption, and it - * is an assumption: nothing in the engine, the schema or the linter makes - * sibling conditions exclusive. ⛔ No commit message, ADR or doc records the - * multi-take as a decision — searched with `git log -S` over `engine.ts` for the - * loop's symbols, and `content/docs/automation/flows.mdx` describes the mode - * ("BPMN exclusive gateway") without ever saying what happens when two - * conditions hold at once. + * A failure here after a change that did NOT intend to touch branch selection + * is the drift this file exists to catch. */ -/** One captured `warn` call, so "nothing reports this" is measured, not assumed. */ +/** One captured `warn` call, so "nothing reports this" stays measured, not assumed. */ const warnings: Array<{ msg: string; meta?: Record }> = []; function createTestLogger(): any { @@ -77,7 +68,11 @@ function createCtx(): any { return { logger: createTestLogger(), getService: () => undefined }; } -describe('decision with overlapping out-edge conditions — status-quo pin (#15429)', () => { +/** The refusal sentences, as the spec spells them — pinned by prefix, never re-derived. */ +const MODE_BESIDE_LIST = /is not valid on a decision that declares a `conditions` list/; +const MODE_NOT_A_MODE = /is not a decision mode/; + +describe('decision edge branching — exclusive by default, inclusive by declaration (#15429)', () => { let engine: AutomationEngine; /** `enter:` / `exit:` per visited successor — order AND nesting. */ let trace: string[]; @@ -101,18 +96,28 @@ describe('decision with overlapping out-edge conditions — status-quo pin (#154 }, }); // This harness is an embedded host, so it owes the ADR-0018 host half: - // the vocabulary it contributes is complete (#4771). Without the seal the - // first `execute()` warns about it and the zero-warning assertion below — - // which is about branch reporting, not node types — would count that line. + // the vocabulary it contributes is complete (#4771). engine.sealNodeTypeVocabulary(); }); /** - * The hotcrm shape, reduced: one `decision`, two out-edges, no - * `config.conditions` on the node. `a` and `b` are the two edge predicates; - * an empty string means the edge carries no condition at all. + * The hotcrm shape, reduced: one `decision`, two conditioned out-edges + * (`a` on `refuse`, `b` on `convert`; an empty string means no condition), + * optionally a third `isDefault` edge to `fallback`, optionally a node + * config. `reversed` declares `convert` before `refuse`, so declaration + * order — not edge id, not target name — is what the pin reads. */ - function gatewayFlow(opts: { a: string; b: string; conditions?: Array<{ label: string; expression: string }> }) { + function gatewayFlow(opts: { + a: string; + b: string; + config?: Record; + withDefault?: boolean; + reversed?: boolean; + gatewayType?: string; + }) { + const gatewayType = opts.gatewayType ?? 'decision'; + const refuseEdge = { id: 'e_refuse', source: 'check', target: 'refuse', label: 'Refuse', ...(opts.a ? { condition: opts.a } : {}) }; + const convertEdge = { id: 'e_convert', source: 'check', target: 'convert', label: 'Convert', ...(opts.b ? { condition: opts.b } : {}) }; return { name: 'gateway', label: 'Gateway', @@ -120,21 +125,22 @@ describe('decision with overlapping out-edge conditions — status-quo pin (#154 variables: [{ name: 'lead', type: 'object', isInput: true }], nodes: [ { id: 'start', type: 'start' as const, label: 'Start' }, - { - id: 'check', type: 'decision' as const, label: 'Check', - ...(opts.conditions ? { config: { conditions: opts.conditions } } : {}), - }, + { id: 'check', type: gatewayType, label: 'Check', ...(opts.config ? { config: opts.config } : {}) }, { id: 'refuse', type: 'mark' as const, label: 'Refuse' }, { id: 'convert', type: 'mark' as const, label: 'Convert' }, + ...(opts.withDefault ? [{ id: 'fallback', type: 'mark' as const, label: 'Fallback' }] : []), ], edges: [ { id: 'e1', source: 'start', target: 'check' }, - { id: 'e_refuse', source: 'check', target: 'refuse', label: 'Refuse', ...(opts.a ? { condition: opts.a } : {}) }, - { id: 'e_convert', source: 'check', target: 'convert', label: 'Convert', ...(opts.b ? { condition: opts.b } : {}) }, + ...(opts.reversed ? [convertEdge, refuseEdge] : [refuseEdge, convertEdge]), + ...(opts.withDefault ? [{ id: 'e_fallback', source: 'check', target: 'fallback', label: 'Otherwise', isDefault: true }] : []), ], }; } + /** The hotcrm#1555 pair: both predicates hold for a confirmed record. */ + const OVERLAP = { a: "lead.status != 'suspected'", b: "lead.status == 'confirmed'" }; + const run = (lead: Record) => engine.execute('gateway', { params: { lead } } as any); const stepsOfLastRun = async () => { @@ -144,9 +150,7 @@ describe('decision with overlapping out-edge conditions — status-quo pin (#154 // ── The instrument, before any reading taken with it ────────────────── - it('CONTROL — disjoint edge conditions take exactly one successor', async () => { - // Same node, same two edges, same record: only the predicates differ. - // If this read "both" the counting method would prove nothing below. + it('CONTROL — disjoint edge conditions take exactly one successor, and the closed gate leaves a skipped step', async () => { engine.registerFlow('gateway', gatewayFlow({ a: "lead.status == 'suspected'", b: "lead.status == 'confirmed'", @@ -155,95 +159,310 @@ describe('decision with overlapping out-edge conditions — status-quo pin (#154 await run({ status: 'confirmed' }); expect(trace).toEqual(['enter:convert', 'exit:convert']); - // …and the branch not taken leaves a trace: a closed gate records a - // `skipped` step naming the edge that closed it (#4354). expect(await stepsOfLastRun()).toContainEqual({ nodeId: 'refuse', status: 'skipped', edgeId: 'e_refuse' }); }); - // ── The reading ─────────────────────────────────────────────────────── + it('POSITIVE CONTROL — the same instrument reads interleaving on the unconditional fan-out', async () => { + // Drop both conditions and the identical pair of edges lands in the + // unconditional bucket, which really is `Promise.all`. This is what + // proves the sequential readings below measured sequencing rather than + // an instrument that cannot see concurrency at all. + engine.registerFlow('gateway', gatewayFlow({ a: '', b: '' })); - it('takes EVERY out-edge whose condition holds — the reported hazard, confirmed', async () => { - // The hotcrm pair verbatim in shape: a `Clean` guard spelled as a - // negation, and a later `== confirmed` branch added beside it. For a - // confirmed record both predicates are true, and the author's reading of - // the node — "one of these" — is nowhere written down. - engine.registerFlow('gateway', gatewayFlow({ - a: "lead.status != 'suspected'", - b: "lead.status == 'confirmed'", - })); + await run({ status: 'confirmed' }); + + expect(trace).toEqual(['enter:refuse', 'enter:convert', 'exit:refuse', 'exit:convert']); + }); + + // ── 1. Exclusive by default ─────────────────────────────────────────── + + it('two overlapping true edges → exactly ONE successor runs: the first declared', async () => { + engine.registerFlow('gateway', gatewayFlow(OVERLAP)); + + const result = await run({ status: 'confirmed' }); + + expect(trace).toEqual(['enter:refuse', 'exit:refuse']); + expect(result.success).toBe(true); + expect(warnings).toEqual([]); + }); + + it('the passed-over sibling records a `skipped` step naming the gate and its edge', async () => { + engine.registerFlow('gateway', gatewayFlow(OVERLAP)); + + await run({ status: 'confirmed' }); + + const steps = await stepsOfLastRun(); + expect(steps).toContainEqual({ nodeId: 'convert', status: 'skipped', edgeId: 'e_convert' }); + expect(steps.filter(s => s.status === 'success').map(s => s.nodeId)).toEqual(['start', 'check', 'refuse']); + }); + + it('DECLARATION ORDER decides — the same two predicates, declared the other way round, take the other branch', async () => { + engine.registerFlow('gateway', gatewayFlow({ ...OVERLAP, reversed: true })); + + await run({ status: 'confirmed' }); + + expect(trace).toEqual(['enter:convert', 'exit:convert']); + expect(await stepsOfLastRun()).toContainEqual({ nodeId: 'refuse', status: 'skipped', edgeId: 'e_refuse' }); + }); + + it("`mode: 'exclusive'` written out says the same thing as an omitted `mode`", async () => { + engine.registerFlow('gateway', gatewayFlow({ ...OVERLAP, config: { mode: 'exclusive' } })); + + await run({ status: 'confirmed' }); + + expect(trace).toEqual(['enter:refuse', 'exit:refuse']); + }); + + // ── 2. Inclusive by declaration ─────────────────────────────────────── + + it("`mode: 'inclusive'` → BOTH run, one after another (nested, never interleaved), with no skipped step", async () => { + engine.registerFlow('gateway', gatewayFlow({ ...OVERLAP, config: { mode: 'inclusive' } })); const result = await run({ status: 'confirmed' }); - // Both successors run. The refusal screen renders AND the conversion - // runs, in one execution — pinned as today's behaviour, ⛔ not endorsed. - expect(trace.filter(t => t.startsWith('enter:'))).toEqual(['enter:refuse', 'enter:convert']); + expect(trace).toEqual(['enter:refuse', 'exit:refuse', 'enter:convert', 'exit:convert']); expect(result.success).toBe(true); + const steps = await stepsOfLastRun(); + expect(steps.filter(s => s.status === 'skipped')).toEqual([]); + expect(steps.filter(s => s.status === 'success').map(s => s.nodeId)).toEqual(['start', 'check', 'refuse', 'convert']); }); - it('takes them one at a time, NOT in parallel — the card says parallel; the engine does not', async () => { + it("`mode: 'inclusive'` with one true edge takes that one and records the closed gate", async () => { engine.registerFlow('gateway', gatewayFlow({ - a: "lead.status != 'suspected'", + a: "lead.status == 'suspected'", b: "lead.status == 'confirmed'", + config: { mode: 'inclusive' }, })); await run({ status: 'confirmed' }); - // Nested, not interleaved: `refuse` finishes before `convert` starts. - expect(trace).toEqual(['enter:refuse', 'exit:refuse', 'enter:convert', 'exit:convert']); + expect(trace).toEqual(['enter:convert', 'exit:convert']); + expect(await stepsOfLastRun()).toContainEqual({ nodeId: 'refuse', status: 'skipped', edgeId: 'e_refuse' }); }); - it('POSITIVE CONTROL — the same instrument reads interleaving on the unconditional fan-out', async () => { - // Drop both conditions and the identical pair of edges lands in the - // unconditional bucket, which really is `Promise.all`. This is what - // proves the previous test measured sequencing rather than an instrument - // that cannot see concurrency at all. - engine.registerFlow('gateway', gatewayFlow({ a: '', b: '' })); + // ── 3. `isDefault` is unchanged in both modes ───────────────────────── - await run({ status: 'confirmed' }); + it('none true → the `isDefault` edge runs (exclusive), and every closed gate is recorded', async () => { + engine.registerFlow('gateway', gatewayFlow({ + a: "lead.status == 'suspected'", + b: "lead.status == 'confirmed'", + withDefault: true, + })); - expect(trace).toEqual(['enter:refuse', 'enter:convert', 'exit:refuse', 'exit:convert']); + await run({ status: 'new' }); + + expect(trace).toEqual(['enter:fallback', 'exit:fallback']); + const steps = await stepsOfLastRun(); + expect(steps).toContainEqual({ nodeId: 'refuse', status: 'skipped', edgeId: 'e_refuse' }); + expect(steps).toContainEqual({ nodeId: 'convert', status: 'skipped', edgeId: 'e_convert' }); }); - it('reports the multi-take NOWHERE — no warning, and no `skipped` step to notice it by', async () => { + it('none true → the `isDefault` edge runs (inclusive too)', async () => { engine.registerFlow('gateway', gatewayFlow({ - a: "lead.status != 'suspected'", + a: "lead.status == 'suspected'", b: "lead.status == 'confirmed'", + withDefault: true, + config: { mode: 'inclusive' }, })); + await run({ status: 'new' }); + + expect(trace).toEqual(['enter:fallback', 'exit:fallback']); + }); + + it('a true edge beside a default → the branch runs and the default is passed over, in both modes', async () => { + engine.registerFlow('gateway', gatewayFlow({ ...OVERLAP, withDefault: true })); await run({ status: 'confirmed' }); + expect(trace).toEqual(['enter:refuse', 'exit:refuse']); + expect(await stepsOfLastRun()).toContainEqual({ nodeId: 'fallback', status: 'skipped', edgeId: 'e_fallback' }); - // Both edges opened, so neither records the `skipped` step that makes a - // closed gate visible in the CONTROL above: the run log of a - // two-branch execution is indistinguishable from a flow that was - // *authored* to run both. - const steps = await stepsOfLastRun(); - expect(steps.filter(s => s.status === 'skipped')).toEqual([]); - expect(steps.filter(s => s.status === 'success').map(s => s.nodeId)).toEqual(['start', 'check', 'refuse', 'convert']); - expect(warnings).toEqual([]); + trace = []; + engine.registerFlow('gateway', gatewayFlow({ ...OVERLAP, withDefault: true, config: { mode: 'inclusive' } })); + await run({ status: 'confirmed' }); + expect(trace).toEqual(['enter:refuse', 'exit:refuse', 'enter:convert', 'exit:convert']); + expect(await stepsOfLastRun()).toContainEqual({ nodeId: 'fallback', status: 'skipped', edgeId: 'e_fallback' }); }); - // ── The other half of the question: is undeclared a distinct mode? ──── + // ── A `conditions` list is a different mechanism, and it is unchanged ── - it('declared `config.conditions` is a DIFFERENT mode — first match wins, even when two match', async () => { - // The same two overlapping predicates, moved onto the node. The executor - // returns on the first match (`logic-nodes.ts`), traversal narrows to the - // edge carrying that label, and the second branch is never reached. + it('declared `config.conditions` stays first-match by label narrowing — the losing edge leaves no step at all', async () => { engine.registerFlow('gateway', gatewayFlow({ a: '', b: '', - conditions: [ - { label: 'Refuse', expression: "lead.status != 'suspected'" }, - { label: 'Convert', expression: "lead.status == 'confirmed'" }, - ], + config: { + conditions: [ + { label: 'Refuse', expression: "lead.status != 'suspected'" }, + { label: 'Convert', expression: "lead.status == 'confirmed'" }, + ], + }, })); await run({ status: 'confirmed' }); expect(trace).toEqual(['enter:refuse', 'exit:refuse']); - // Narrowed away, not gated: the losing edge leaves no step at all — not - // even the `skipped` one a closed gate writes. The two modes differ in - // what they record as well as in what they run. const steps = await stepsOfLastRun(); expect(steps.map(s => s.nodeId)).toEqual(['start', 'check', 'refuse']); expect(warnings).toEqual([]); }); + + // ── 4. `mode` is judged at registration, with the spec's own sentence ── + + describe('registration parses `DecisionConfigSchema` and refuses an invalid `mode`', () => { + it('refuses `mode` beside a non-empty `conditions` list — either member — at config.mode, with the refinement sentence', async () => { + for (const mode of ['inclusive', 'exclusive']) { + const definition = gatewayFlow({ + a: '', b: '', + config: { mode, conditions: [{ label: 'Refuse', expression: "lead.status != 'suspected'" }] }, + }); + expect(() => engine.registerFlow('gateway', definition)).toThrow(MODE_BESIDE_LIST); + try { + engine.registerFlow('gateway', definition); + } catch (e) { + const message = (e as Error).message; + expect(message).toContain("Flow 'gateway' rejected"); + expect(message).toContain("node 'check' (decision) at config.mode"); + expect(message).toContain(`\`mode: '${mode}'\``); + // Both ruled ways out ride along, verbatim from the schema. + expect(message).toContain('Either delete `mode` and keep the list'); + expect(message).toContain('move the branches onto the out-edges'); + } + // Refused means never armed (`getFlow` answers `null` for a name it does not hold). + expect(await engine.getFlow('gateway')).toBeNull(); + } + }); + + it('refuses a `mode` outside the closed pair with the value prescription', async () => { + const definition = gatewayFlow({ ...OVERLAP, config: { mode: 'all' } }); + expect(() => engine.registerFlow('gateway', definition)).toThrow(MODE_NOT_A_MODE); + try { + engine.registerFlow('gateway', definition); + } catch (e) { + const message = (e as Error).message; + expect(message).toContain("`mode: 'all'` is not a decision mode"); + expect(message).toContain("at config.mode"); + } + expect(await engine.getFlow('gateway')).toBeNull(); + }); + + it('refuses the same shape inside a loop body, naming the region', () => { + const definition = { + name: 'gateway', + label: 'Gateway', + type: 'autolaunched' as const, + variables: [{ name: 'leads', type: 'object', isInput: true }], + nodes: [ + { id: 'start', type: 'start' as const, label: 'Start' }, + { + id: 'sweep', type: 'loop' as const, label: 'Sweep', + config: { + collection: '{leads}', + iteratorVariable: 'lead', + body: { + nodes: [ + { id: 'gate', type: 'decision', label: 'Gate', config: { mode: 'parallel' } }, + { id: 'x', type: 'mark', label: 'X' }, + ], + edges: [{ id: 'g1', source: 'gate', target: 'x', condition: 'true' }], + }, + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'sweep' }], + }; + expect(() => engine.registerFlow('gateway', definition)).toThrow(MODE_NOT_A_MODE); + try { + engine.registerFlow('gateway', definition); + } catch (e) { + expect((e as Error).message).toMatch(/loop 'sweep'.*node 'gate' \(decision\) at config\.mode/); + } + }); + + it('CONTROLS — `mode` on an empty list, `mode` alone, and a list without `mode` all register', () => { + const shapes: Record[] = [ + { mode: 'inclusive', conditions: [] }, + { mode: 'exclusive', conditions: [] }, + { mode: 'inclusive' }, + { conditions: [{ label: 'Refuse', expression: "lead.status != 'suspected'" }] }, + ]; + for (const config of shapes) { + const registered = engine.registerFlow('gateway', gatewayFlow({ ...OVERLAP, config })); + expect(registered.name).toBe('gateway'); + engine.unregisterFlow('gateway'); + } + }); + }); + + // ── 5. The rehydration seam does not rewrite the flip (ruling C) ────── + + describe('the flow rehydration seam refuses `flow-decision-mode-inclusive-explicit` by id', () => { + const twoBranches = () => gatewayFlow(OVERLAP); + + it('the refused id is a real registry entry, retired from the load path (anti-vacuity for the seam pin)', () => { + const entry = ALL_CONVERSIONS.find((c) => c.id === 'flow-decision-mode-inclusive-explicit'); + expect(entry).toBeDefined(); + expect(entry!.retiredFromLoadPath).toBe(true); + expect(entry!.toMajor).toBe(18); + }); + + it('`canonicalizeStoredFlow` leaves a two-branch decision without `mode` — parsed AND storable — and emits no notice for it', () => { + const { parsed, storable, notices } = engine.canonicalizeStoredFlow('gateway', twoBranches()); + const parsedCheck = parsed.nodes.find((n) => n.id === 'check')!; + expect(parsedCheck.config).toBeUndefined(); + const storableCheck = ((storable as { nodes: Array<{ id: string; config?: unknown }> }).nodes).find((n) => n.id === 'check')!; + expect(storableCheck.config).toBeUndefined(); + expect(notices.map((n) => n.conversionId)).not.toContain('flow-decision-mode-inclusive-explicit'); + }); + + it('…and the registered flow therefore runs EXCLUSIVE, which is the ruled default for a body this seam cannot date', async () => { + engine.registerFlow('gateway', twoBranches()); + await run({ status: 'confirmed' }); + expect(trace).toEqual(['enter:refuse', 'exit:refuse']); + }); + + it('RULING C — a STORED decision lacking `mode`, with overlapping conditions, evaluates FIRST-MATCH after the upgrade: the row registers as stored', async () => { + // The body exactly as a `sys_metadata` row written before protocol 18 + // carries it — JSON text, no `mode` — and every door a stored row + // re-enters by (the boot pull, a Studio save, `os migrate meta + // --stored`) goes through this one seam. + const stored = JSON.parse(JSON.stringify(twoBranches())); + + // What the stored pass would persist: no `mode` written, no notice for it. + const { storable, notices } = engine.canonicalizeStoredFlow('gateway', stored); + const storableCheck = ((storable as { nodes: Array<{ id: string; config?: unknown }> }).nodes).find((n) => n.id === 'check')!; + expect(storableCheck.config).toBeUndefined(); + expect(notices.map((n) => n.conversionId)).not.toContain('flow-decision-mode-inclusive-explicit'); + + engine.registerFlow('gateway', stored); + await run({ status: 'confirmed' }); + + // Both predicates hold for a confirmed lead; only the first declared runs. + expect(trace).toEqual(['enter:refuse', 'exit:refuse']); + expect(await stepsOfLastRun()).toContainEqual({ nodeId: 'convert', status: 'skipped', edgeId: 'e_convert' }); + }); + + it('RULING C, and the FIRING CONTROL for both pins above — a SOURCE migrated with `os migrate meta --from 17` carries explicit `mode: inclusive` and still takes EVERY branch', async () => { + const result = applyMetaMigrations({ flows: [twoBranches()] }, 17, 18); + const migrated = (result.stack.flows as Array<{ nodes: Array<{ id: string; config?: Record }> }>)[0]!; + expect(migrated.nodes.find((n) => n.id === 'check')!.config).toEqual({ mode: 'inclusive' }); + expect(result.applied.map((a) => a.conversionId)).toContain('flow-decision-mode-inclusive-explicit'); + + engine.registerFlow('gateway', migrated); + await run({ status: 'confirmed' }); + + // Every true branch, one after another — exactly what the flow did before protocol 18. + expect(trace).toEqual(['enter:refuse', 'exit:refuse', 'enter:convert', 'exit:convert']); + expect((await stepsOfLastRun()).filter((s) => s.status === 'skipped')).toEqual([]); + }); + }); + + // ── 6. Scoped to `decision` ─────────────────────────────────────────── + + it('a NON-decision node with two true conditioned out-edges keeps the every-true-edge traversal (the boundary)', async () => { + // `mark` returns success with no branch label, so its out-edges route + // through the same conditional loop — and `mode` is not its key. + engine.registerFlow('gateway', gatewayFlow({ ...OVERLAP, gatewayType: 'mark' })); + + await run({ status: 'confirmed' }); + + expect(trace).toEqual(['enter:check', 'exit:check', 'enter:refuse', 'exit:refuse', 'enter:convert', 'exit:convert']); + }); }); diff --git a/packages/services/service-automation/src/builtin/logic-nodes.ts b/packages/services/service-automation/src/builtin/logic-nodes.ts index 1fb75432337..6042787d92e 100644 --- a/packages/services/service-automation/src/builtin/logic-nodes.ts +++ b/packages/services/service-automation/src/builtin/logic-nodes.ts @@ -42,6 +42,17 @@ export function registerLogicNodes(engine: AutomationEngine, ctx: PluginContext) * node silently fell back to "consider every out-edge", which is * the state #4414 measured (0 label matches across all example * apps) and, on an unconditional sibling, ran both branches. + * + * On that second path the gateway is EXCLUSIVE (#15429): the + * engine's traversal takes the first conditioned out-edge that + * holds, in declaration order, and passes its siblings over; a + * decision declaring `config.mode: 'inclusive'` takes every one + * that holds. Both live in `AutomationEngine.traverseNext`, not + * here — this executor reads `conditions` and nothing else. The + * `mode` key itself is judged at registration: `registerFlow` parses + * every decision's config through the spec's `DecisionConfigSchema` + * and refuses a value outside the closed pair, or a `mode` beside a + * non-empty `conditions` list, with the schema's own sentence. */ async execute(node, variables, _context) { const config = node.config as Record | undefined; diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 95c1722169d..daac6de0730 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -19,7 +19,7 @@ import { type ScreenFieldVisibility, } from './screen-input-contract.js'; import type { Logger } from '@objectstack/spec/contracts'; -import { FlowSchema, FLOW_STRUCTURAL_NODE_TYPES, validateControlFlow, collectFlowGraphs, findRegionEntry, defineActionDescriptor } from '@objectstack/spec/automation'; +import { FlowSchema, FLOW_STRUCTURAL_NODE_TYPES, validateControlFlow, collectFlowGraphs, findRegionEntry, defineActionDescriptor, DecisionConfigSchema } from '@objectstack/spec/automation'; // [#14328] The ONE answer to "which trigger kind does this flow ask for?" — // shared with `defineStack`'s trigger-capability refusal and `@objectstack/lint`'s // `validate-flow-trigger-readiness`, so the runtime cannot drift from what @@ -229,6 +229,18 @@ import { describeThrownForLog } from './thrown-cause-diagnostics.js'; // reaches back here. import { interpolateText } from './builtin/template.js'; +/** + * Does this `decision` take EVERY out-edge whose condition holds (#15429)? + * Only an explicit `config.mode: 'inclusive'` says so; an omitted `mode` and + * `'exclusive'` both mean the first true edge in declaration order wins. Any + * other value never reaches here — {@link AutomationEngine.registerFlow} + * refuses it with the spec's prescription. + */ +function decisionTakesEveryBranch(node: FlowNodeParsed): boolean { + const config = node.config as { mode?: unknown } | undefined; + return config?.mode === 'inclusive'; +} + // ─── Node Executor Interface (Plugin Extension Point) ─────────────── /** @@ -2112,6 +2124,36 @@ export interface FlowActivationStore { */ export const IN_PROCESS_DISPATCH_CLAIM_TTL_MS = 48 * 60 * 60 * 1000; +/** + * ADR-0087 conversions the flow rehydration seam refuses to replay, by id — + * the DEFAULT-FLIP class, the same per-door refusal the artifact-ingestion + * door makes (`DEFAULT_FLIPS_NOT_REPLAYED_HERE` in + * `packages/metadata-core/src/artifact-forward-conversion.ts`, which lists this + * seam's one id beside its own precedent). The registry stays the single + * authority on what converts; this is only this seam saying which entries its + * own evidence cannot carry, and each id owes its reason beside it. + * + * - `flow-decision-mode-inclusive-explicit` (#15429) writes `mode: 'inclusive'` + * onto an edge-branched `decision` with two or more conditioned out-edges, + * so a flow written while every true branch ran keeps that behaviour under + * the exclusive traversal. The rewrite is sound only where "this body + * predates the flip" is a FACT, and here it never is: `canonicalizeStoredFlow` + * sees every body alike — a code-shipped flow at the boot pull, a REST + * `POST /automation` definition, a Studio save and a package duplication + * (both resolve this same method) — and a decision written yesterday against + * the contract that says an omitted `mode` is exclusive is byte-identical to + * a row written before the contract said so. Replaying it would rewrite every + * new exclusive decision into an inclusive one at registration and persist + * that at save, and the ruled default would be unobservable. The entry + * replays where the age IS asserted: `os migrate meta --from 17`, by the + * operator, over authored sources — never at a load seam. The artifact + * door refuses it for the same reason (its declared `^17.0.0` floor is a + * dependency range, not an age), in the module named above. + */ +const CONVERSIONS_NOT_REPLAYED_AT_REHYDRATION: readonly string[] = [ + 'flow-decision-mode-inclusive-explicit', +]; + /** * Lift the `{ dialect, source }` envelopes the flow schema derives for edge * `condition`s back onto the conversion output — and take nothing else with @@ -4062,6 +4104,13 @@ export class AutomationEngine implements IAutomationService { // exact hazard this seam exists to prevent. Authored sources keep // window semantics at their own seam (`normalizeStackInput` applies // live-window entries only; the schema tombstones the retired shape). + // + // `excludeConversionIds`: the retired window is opened for a CLASS of + // caller, and one kind of entry inside it is not a rescue but a + // reinterpretation — a DEFAULT FLIP, whose old shape still parses and + // now means something else. This seam cannot say a body predates such + // a flip, so it refuses those entries by id, each with its reason on + // `CONVERSIONS_NOT_REPLAYED_AT_REHYDRATION`. const reservedNodeTypes = new Set([ ...FLOW_STRUCTURAL_NODE_TYPES, ...this.nodeExecutors.keys(), @@ -4072,6 +4121,7 @@ export class AutomationEngine implements IAutomationService { const converted = applyConversionsToFlow(definition, { reservedNodeTypes, includeRetired: true, + excludeConversionIds: CONVERSIONS_NOT_REPLAYED_AT_REHYDRATION, onNotice: (n) => { notices.push(n); this.logger.warn(`[flow '${name}'] ${n.code}: ${n.message}`); @@ -4129,6 +4179,11 @@ export class AutomationEngine implements IAutomationService { // types, keyValue maps). this.validateNodeConfigKeys(name, parsed); + // #15429 — parse every `decision` node's config against the spec's + // `DecisionConfigSchema` and refuse the flow on an invalid `mode`, with + // the schema's own sentence — the same answer `os validate` gives. + this.validateDecisionModes(name, parsed); + // ADR-0032 §Decision 1a — parse-validate every predicate at registration, // so a malformed condition (e.g. the #1491 `{record.x}` template-brace-in- // CEL mistake) is a LOUD registration error with the offending source, @@ -9240,6 +9295,60 @@ export class AutomationEngine implements IAutomationService { } } + /** + * [#15429] The registration-time reader of a `decision` node's `mode`. + * + * `decision` publishes no descriptor `configSchema` (its Target column is + * derived from the out-edges), so {@link validateNodeConfigKeys}' + * schemaless exemption skips it and, until this pass, nothing at run time + * parsed its config at all — `mode` was export-only, enforced by `tsc`, the + * published JSON Schema and the objectui reconciliation, and a stored + * `mode: 'bogus'` registered clean and ran as whatever the traversal made + * of it. Now every decision's config goes through the spec's + * `DecisionConfigSchema` here and the flow is refused on any issue rooted + * at `mode`: a value outside `'exclusive' | 'inclusive'`, or a legal `mode` + * beside a non-empty `conditions` list (a list is first-match on its own, + * so a `mode` beside it would be accepted and never read — ruling A on + * #20168). The refusal is the schema's own sentence, shared with + * `os validate`'s `flow-decision-mode-invalid`, so build time and + * registration cannot disagree about the key. + * + * Judged on `mode` alone, deliberately. The same parse also refuses an + * undeclared key on a decision (the shape is `strictObject`), but that + * strictness binds at authoring by a standing decision (the module header + * of `schemaless-node-config.zod.ts`): a flow carrying an inert extra key + * registered and ran before this pass, and refusing it at boot would be a + * second behaviour change riding a ruling that ordered one. `os validate` + * keeps reporting that shape through its own advisory rule. + * + * Hard-fail, like {@link validateNodeConfigKeys}: a flow whose gateway + * declares a mode the engine does not have is wrong metadata, and the + * caller's per-flow try/catch skips it loudly at boot rather than arming a + * gateway that would run as something the author did not write. + */ + private validateDecisionModes(flowName: string, flow: FlowParsed): void { + const failures: string[] = []; + for (const graph of collectFlowGraphs(flow)) { + const at = graph.scope ? `${graph.scope}: ` : ''; + for (const node of graph.nodes) { + if (node.type !== 'decision') continue; + const verdict = DecisionConfigSchema.safeParse(node.config ?? {}); + if (verdict.success) continue; + for (const issue of verdict.error.issues) { + if (issue.path[0] !== 'mode') continue; + failures.push(` • ${at}node '${node.id}' (decision) at config.mode: ${issue.message}`); + } + } + } + if (failures.length > 0) { + throw new Error( + `Flow '${flowName}' rejected: ${failures.length} invalid decision \`mode\` ` + + `declaration${failures.length > 1 ? 's' : ''}. The config is metadata, so re-registering ` + + `changes nothing; fix the node in the flow definition:\n${failures.join('\n')}`, + ); + } + } + /** * Walk `value` against `schema` in lockstep, collecting keys the schema does * not declare into `violations`. @@ -10119,7 +10228,12 @@ export class AutomationEngine implements IAutomationService { * computed a branch and nothing routed it, which is how app-crm's * convert-lead guard ran its abort screen AND its wizard. * 2. **`edge.condition`** — evaluated per edge; a closed gate records a - * `skipped` step (#4354). + * `skipped` step (#4354). On a `decision` node the conditioned + * out-edges are EXCLUSIVE (#15429): evaluated in declaration order, + * the first one that holds is the branch and its later siblings are + * passed over unevaluated, recording the same `skipped` step. A + * decision declaring `config.mode: 'inclusive'` takes every one that + * holds instead. * 3. **`edge.isDefault`** — BPMN default flow. Traversed **only** when no * conditional sibling in the selected set matched. Before #4414 this key * had zero readers: it parsed, it was documented as "the default path @@ -10203,42 +10317,84 @@ export class AutomationEngine implements IAutomationService { } } - // Conditional edges: evaluate sequentially (mutually exclusive) + // #4354 — a gate that did not open leaves a trace. Record it: this is + // THE event that had no trace anywhere, and the reason #4347 shipped + // three inert production flows. A closed gate inside a loop body is + // logged once per iteration (region tagging in `runRegion` attaches the + // container + iteration), so the run summary can say "selected 30, + // acted 0, skipped 30 by " instead of reporting a green run that + // did nothing. + // + // The step is `skipped`, never a run: the re-entrancy guard, per-node + // `runs` counts and node status all exclude it, so recording a + // non-event stays a non-event to execution. + const recordSkipped = (nextNode: FlowNodeParsed, edge: FlowEdgeParsed): void => { + const at = new Date().toISOString(); + steps.push({ + nodeId: nextNode.id, + nodeType: nextNode.type, + ...(nextNode.label ? { nodeLabel: nextNode.label } : {}), + status: 'skipped', + startedAt: at, + completedAt: at, + durationMs: 0, + skippedBy: { + nodeId: node.id, + ...(edge.id ? { edgeId: edge.id } : {}), + ...(edge.label ? { label: edge.label } : {}), + }, + }); + }; + + // Conditional edges: evaluated sequentially, in the order the flow's + // `edges` array declares them — `FlowSchema.parse` and every conversion + // are copy-on-write maps that never reorder, so declaration order here + // IS the author's. + // + // On a `decision` node they are the gateway's branches, and the gateway + // is EXCLUSIVE unless it declares `config.mode: 'inclusive'` (#15429, + // maintainer ruling 「跟主流对齐」): the FIRST edge whose condition holds + // is the branch — the BPMN exclusive gateway, Salesforce Flow's + // Decision, n8n's Switch default — and its later siblings are not + // evaluated at all; they record the same `skipped` step a closed gate + // does, so the run log says which branch won and which were passed + // over. Until this change every true edge ran, one after another, under + // a comment calling that "mutually exclusive": hotcrm#1555 rendered a + // refusal screen AND ran the conversion in one execution. Authored + // sources written against that behaviour are carried across by the + // ADR-0087 conversion `flow-decision-mode-inclusive-explicit` (`os + // migrate meta --from 17`), which writes the inclusive declaration onto + // them — see `CONVERSIONS_NOT_REPLAYED_AT_REHYDRATION` for why this seam + // does not. A flow stored in `sys_metadata` arrives here as stored and + // takes THIS reading (maintainer ruling letter C on #15429: no + // stored-row rewrite, no read-path completion); `os migrate meta + // --stored` lists such decisions for an operator to review. + // + // `mode: 'inclusive'` is the BPMN inclusive gateway: every edge whose + // condition holds runs, one successor at a time (never `Promise.all` — + // that fan-out belongs to the UNCONDITIONAL bucket below alone). + // + // Scoped to `decision` on purpose: `mode` is a decision-config key, and + // conditioned out-edges of any other node type keep the traversal they + // had — every one whose condition holds, sequentially. The ruling and + // its migration cover the gateway, and nothing else was measured to + // carry two conditioned out-edges (the corpus census on #15429). + const exclusive = node.type === 'decision' && !decisionTakesEveryBranch(node); let anyConditionMet = false; for (const edge of conditionalEdges) { const nextNode = flow.nodes.find(n => n.id === edge.target); + if (exclusive && anyConditionMet) { + // A sibling already won: passed over, not evaluated. + if (nextNode) recordSkipped(nextNode, edge); + continue; + } if (this.evaluateCondition(edge.condition!, variables)) { anyConditionMet = true; if (nextNode) { await this.executeNode(nextNode, flow, variables, context, steps); } } else if (nextNode) { - // #4354 — the gate closed. Record it: this is THE event that had - // no trace anywhere, and the reason #4347 shipped three inert - // production flows. A closed gate inside a loop body is logged - // once per iteration (region tagging in `runRegion` attaches the - // container + iteration), so the run summary can say - // "selected 30, acted 0, skipped 30 by " instead of - // reporting a green run that did nothing. - // - // The step is `skipped`, never a run: the re-entrancy guard, - // per-node `runs` counts and node status all exclude it, so - // recording a non-event stays a non-event to execution. - const at = new Date().toISOString(); - steps.push({ - nodeId: nextNode.id, - nodeType: nextNode.type, - ...(nextNode.label ? { nodeLabel: nextNode.label } : {}), - status: 'skipped', - startedAt: at, - completedAt: at, - durationMs: 0, - skippedBy: { - nodeId: node.id, - ...(edge.id ? { edgeId: edge.id } : {}), - ...(edge.label ? { label: edge.label } : {}), - }, - }); + recordSkipped(nextNode, edge); } } diff --git a/packages/spec/src/automation/schemaless-node-config.test.ts b/packages/spec/src/automation/schemaless-node-config.test.ts index 256796925b3..90f1893e3ae 100644 --- a/packages/spec/src/automation/schemaless-node-config.test.ts +++ b/packages/spec/src/automation/schemaless-node-config.test.ts @@ -6,12 +6,15 @@ * `script` and `subflow` run through `service-automation`'s `parseNodeConfig()` * before their executors do anything, so what this file pins is not decoration: * a shape accepted here runs, and a shape rejected here refuses the node as a - * guard. `decision` is the exception — it stays export-only (nothing parses it - * at run time), so its pins below bind the authoring doors only: `tsc`, the - * published JSON Schema and a direct parse. Its `mode` key is declared ahead of - * the engine change that reads it (#15429), and is refused beside a non-empty - * `conditions` list (ruling 5856786357 on #20168) — a refinement, so of those - * doors it binds the direct parse and is declared dropped in the JSON Schema. + * guard. `decision` is different — no execute-time parse; its strictness binds + * at the authoring doors only (`tsc`, the published JSON Schema and a direct + * parse), while its `mode` key is ALSO judged at registration (#15429): the + * automation engine's `registerFlow` parses every decision's config through + * this schema and refuses the flow on any issue rooted at `mode`, and + * `os validate` reports the same as `flow-decision-mode-invalid`. `mode` is + * refused beside a non-empty `conditions` list (ruling 5856786357 on #20168) — + * a refinement, so of the authoring doors it binds the direct parse and is + * declared dropped in the JSON Schema; the two run-time doors carry it too. * * The structural assertions at the bottom guard the downstream walkers that a * union-shaped contract would have broken, which is why #4343 converged the diff --git a/packages/spec/src/automation/schemaless-node-config.zod.ts b/packages/spec/src/automation/schemaless-node-config.zod.ts index 70f2ae5d5b0..bbae9357f35 100644 --- a/packages/spec/src/automation/schemaless-node-config.zod.ts +++ b/packages/spec/src/automation/schemaless-node-config.zod.ts @@ -64,13 +64,18 @@ * nothing read — and then refuses, naming the `function` it does not have, * instead of logging a line and reporting success as it used to. * - * `decision` stays export-only: nothing parses it at run time. It may carry no - * `conditions` at all when it branches purely on edge predicates, its executor - * reads `conditions` and nothing else, and its one other key — `mode` — is - * declared AHEAD of the engine change that reads it (#15429; see - * {@link DecisionConfigSchema}). Its enforcement remains the objectui - * reconciliation test, which is what #4278 was actually about (a form - * authoring keys nothing reads). + * `decision` is parsed at **registration**, for one key (#15429): the + * automation engine's `registerFlow` runs every decision node's config through + * {@link DecisionConfigSchema} and refuses the flow on any issue rooted at + * `mode` — a value outside the closed pair, or a `mode` beside a non-empty + * `conditions` list — with this schema's own sentence, and `os validate` + * reports the same issues as `flow-decision-mode-invalid`, so the two doors + * cannot disagree. A decision may carry no `conditions` at all when it + * branches purely on edge predicates; its executor reads `conditions` and + * nothing else, and the engine's traversal reads `mode`. Its strictness + * (unknown keys) still binds at authoring, in the published JSON Schema and in + * the objectui reconciliation test, which is what #4278 was actually about (a + * form authoring keys nothing reads). * * Undeclared aliases are NOT part of these contracts: `subflow`'s historical * `flow` spelling graduated into the ADR-0087 D2 conversion @@ -94,9 +99,10 @@ * precisely the class with no second door. Closing these shapes is therefore * not a duplicate check for `script` and `subflow`; it is their first one. * - * `decision` is still export-only, so its strictness binds at authoring - * (`tsc`), in the published JSON Schema, and in objectui's reconciliation — - * not at run time. It is closed anyway, because the campaign's whole finding + * `decision`'s strictness binds at authoring (`tsc`), in the published JSON + * Schema, and in objectui's reconciliation — not at run time, where the + * registration reader judges `mode` alone (#15429). It is closed anyway, + * because the campaign's whole finding * is that a shape left open accretes a test, a form and a fixture that assert * the openness, and then closing it is a migration instead of an edit. */ @@ -497,20 +503,34 @@ export type DecisionCondition = z.input; * forbids that one when the list is non-empty" — and the site is declared in * `dropped-refinements.baseline.json` and on the artifact as * `x-dropped-refinements`. Like the rest of this contract it binds wherever - * `DecisionConfigSchema` is parsed: a direct parse, and the registration-time - * reader #15429 adds. - * - * ⚠️ **Declared ahead of its enforcement, and the status quo does NOT match - * the default above.** The ruling's split order lands this key first, then - * the engine semantics together with the `os migrate meta` conversion in one - * change, then the docs. Until that second step lands, nothing reads `mode`, - * and an edge-branched decision takes EVERY out-edge whose condition holds, - * one after another, whatever `mode` says — the behaviour - * `decision-overlapping-edge-conditions.pin.test.ts` (service-automation) - * pins as the status quo. The engine change and the conversion that writes - * `mode: 'inclusive'` onto every decision relying on that behaviour land - * together, so no shipped flow changes behaviour silently; this paragraph is - * rewritten with them (#15429). + * `DecisionConfigSchema` is parsed: a direct parse, the automation engine's + * registration reader (`registerFlow` refuses the flow with this sentence), + * and `os validate` (`flow-decision-mode-invalid`, a gating finding). + * + * ## Where `mode` is honoured — the traversal, and the migration that keeps old flows whole + * + * The engine's traversal reads it (#15429): on a `decision` with no + * `conditions` list, the conditioned out-edges are evaluated in the order the + * flow's `edges` array declares them and the FIRST one whose condition holds + * is the branch; the siblings after it are not evaluated and record the same + * `skipped` step a closed gate does. With `mode: 'inclusive'` every out-edge + * whose condition holds runs, one after another. When none holds, the + * `isDefault` edge runs either way. `os validate` reports + * `flow-decision-inclusive-overlap` on an inclusive decision with two or more + * conditioned out-edges, because that is the shape in which more than one + * branch can run for one record. + * + * Authored sources written while every true branch ran keep their behaviour + * through the ADR-0087 D2 conversion `flow-decision-mode-inclusive-explicit`: + * `os migrate meta --from 17` writes `mode: 'inclusive'` onto every + * edge-branched decision with two or more conditioned out-edges, and the author + * deletes it where the branches partition. It is a default flip, so no load + * seam replays it — the authoring funnel is where a source written against THIS + * contract arrives, and the flow rehydration seam cannot date a body — which is + * why the key has to be written into the source, once, by the operator's own + * command. A flow STORED in `sys_metadata` is not rewritten at all (maintainer + * ruling letter C on #15429): it takes the first-match meaning on upgrade, and + * `os migrate meta --stored` lists each such decision for review. * * The legacy singular `config.condition` is a structural surface the engine * parse-validates on every node at registration but the decision executor never @@ -526,23 +546,28 @@ export const DecisionConfigSchema = lazySchema(() => strictObject({ .describe('Ordered decision branches (first true expression wins; omit to branch purely on edge conditions)'), /** * How many out-edges an edge-branched decision takes when more than one - * condition holds — `'exclusive'` (the first; what an omitted key means) or - * `'inclusive'` (every one). Not read by the engine yet: see the - * "Declared ahead of its enforcement" note above. Any other value is refused - * with {@link decisionModePrescription}; either member beside a non-empty - * `conditions` list is refused by the `.superRefine` below, with - * {@link decisionModeWithConditionsRefusal}. + * condition holds: `'exclusive'` is the first, in declaration order, and is + * the reading an absent key gets — the narrower one, a single branch; + * `'inclusive'` is all of them. Read by the engine's + * traversal: see "Where `mode` is honoured" above. Any other value is + * refused with {@link decisionModePrescription}; either member beside a + * non-empty `conditions` list is refused by the `.superRefine` below, with + * {@link decisionModeWithConditionsRefusal} — at a direct parse, at + * registration and by `os validate` alike. */ mode: z.enum(['exclusive', 'inclusive'], { error: (issue) => (issue.code === 'invalid_value' ? decisionModePrescription(issue.input) : undefined), }).optional() .describe( 'Declares how many out-edges an edge-branched decision takes when more than one out-edge condition holds: ' - + "'exclusive' = only the first, in the order the edges are declared (what an omitted mode means); " - + "'inclusive' = every one that holds. Declared ahead of the engine change that reads it: until that lands, " - + 'an edge-branched decision takes every out-edge whose condition holds, whatever this says. Refused beside a ' - + 'non-empty conditions list, which is first-match on its own: delete mode there, or move the branches onto ' - + 'the out-edges, delete conditions, and keep mode.', + + "'exclusive' = only the first, in the order the edges are declared (what an omitted mode means; the " + + "siblings after it are not evaluated and record a skipped step); 'inclusive' = every one that holds, one " + + 'after another. When none holds the isDefault edge runs either way. Refused beside a non-empty conditions ' + + 'list, which is first-match on its own: delete mode there, or move the branches onto the out-edges, delete ' + + 'conditions, and keep mode. Authored sources written while every true branch ran keep that behaviour ' + + 'through the os migrate meta --from 17 conversion, which writes mode: inclusive onto every edge-branched ' + + 'decision with two or more conditioned out-edges; a flow stored in sys_metadata is not rewritten and takes ' + + 'the first-match reading on upgrade (os migrate meta --stored lists those decisions).', ), }).superRefine((config, ctx) => { // Ruling 5856786357 on #20168 (letter A): `mode` belongs to the diff --git a/packages/spec/src/conversions/conversions.test.ts b/packages/spec/src/conversions/conversions.test.ts index 2a284e4731d..1281ab11ebd 100644 --- a/packages/spec/src/conversions/conversions.test.ts +++ b/packages/spec/src/conversions/conversions.test.ts @@ -14,6 +14,7 @@ import { import { normalizeStackInput } from '../shared/metadata-collection.zod.js'; import { ElementButtonPropsSchema, PageHeaderProps, PageTabsProps } from '../ui/component.zod.js'; import { PageSchema } from '../ui/page.zod.js'; +import { applyMetaMigrations } from '../migrations/chain.js'; import { applyConversions, collectConversionNotices } from './apply.js'; import { ALL_CONVERSIONS, CONVERSIONS_BY_MAJOR } from './registry.js'; import { applyConversionsToStoredItem } from './stored.js'; @@ -345,6 +346,131 @@ describe('conversion layer (ADR-0087 D2)', () => { }); }); + /** + * `flow-decision-mode-inclusive-explicit` (#15429) — the DEFAULT FLIP that + * carries an edge-branched decision across the exclusive-gateway ruling. + * + * The fixture pair above pins the rewrite; what needs its own cover is the + * PREDICATE's edges (the count, the two shapes it must leave alone, regions) + * and its JURISDICTION: the chain replays it, the authoring funnel does not, + * and a second replay is a no-op. The flow rehydration seam's refusal by id + * is pinned where that seam lives (`service-automation`). + */ + describe('flow-decision-mode-inclusive-explicit (#15429)', () => { + const ID = 'flow-decision-mode-inclusive-explicit'; + const entry = () => ALL_CONVERSIONS.find((c) => c.id === ID)!; + const decisionFlow = (edges: Record[], config?: Record) => ({ + flows: [{ + name: 'gateway', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'check', type: 'decision', label: 'Check', ...(config ? { config } : {}) }, + { id: 'a', type: 'end', label: 'A' }, + { id: 'b', type: 'end', label: 'B' }, + { id: 'c', type: 'end', label: 'C' }, + ], + edges: [{ id: 'e0', source: 'start', target: 'check' }, ...edges], + }], + }); + const chain = (stack: Record) => applyMetaMigrations(structuredClone(stack), 17, 18); + const checkConfigAfter = (result: ReturnType) => + ((result.stack.flows as any[])[0].nodes as any[]).find((n) => n.id === 'check').config; + const two = [ + { id: 'e1', source: 'check', target: 'a', condition: "x != 'a'" }, + { id: 'e2', source: 'check', target: 'b', condition: "x == 'b'" }, + ]; + + it('is registered at protocol 18, retired from the load path, and wired into the step-18 chain', () => { + expect(entry().toMajor).toBe(18); + expect(entry().retiredFromLoadPath).toBe(true); + const result = chain(decisionFlow(two)); + expect(result.applied.map((a) => a.conversionId)).toContain(ID); + }); + + it('writes `mode: inclusive` on two conditioned out-edges, and says what the site relied on', () => { + const result = chain(decisionFlow(two)); + expect(checkConfigAfter(result)).toEqual({ mode: 'inclusive' }); + const mine = result.applied.filter((a) => a.conversionId === ID); + expect(mine).toHaveLength(1); + expect(mine[0]!.path).toBe('flows[0].nodes[1].config.mode'); + expect(mine[0]!.from).toContain('2 conditioned out-edges'); + expect(mine[0]!.to).toBe('inclusive'); + }); + + it('counts an envelope condition, and a third conditioned edge, but never a fault edge or a blank one', () => { + const three = [ + ...two, + { id: 'e3', source: 'check', target: 'c', condition: { dialect: 'cel', source: "x == 'c'" } }, + ]; + expect(checkConfigAfter(chain(decisionFlow(three)))).toEqual({ mode: 'inclusive' }); + // A `fault` edge is error routing; a blank condition is no condition. + const notBranches = [ + { id: 'e1', source: 'check', target: 'a', condition: "x != 'a'" }, + { id: 'e2', source: 'check', target: 'b', condition: "x == 'b'", type: 'fault' }, + { id: 'e3', source: 'check', target: 'c', condition: ' ' }, + ]; + expect(checkConfigAfter(chain(decisionFlow(notBranches)))).toBeUndefined(); + }); + + it('leaves ONE conditioned out-edge plus a default alone — first-match and every-true-edge cannot differ', () => { + const guarded = [ + { id: 'e1', source: 'check', target: 'a', condition: "x == 'a'" }, + { id: 'e2', source: 'check', target: 'b', isDefault: true }, + ]; + const result = chain(decisionFlow(guarded)); + expect(checkConfigAfter(result)).toBeUndefined(); + expect(result.applied.filter((a) => a.conversionId === ID)).toEqual([]); + }); + + it('leaves a decision that already declares `mode` alone — either member — and a `conditions` list alone', () => { + expect(checkConfigAfter(chain(decisionFlow(two, { mode: 'exclusive' })))).toEqual({ mode: 'exclusive' }); + expect(checkConfigAfter(chain(decisionFlow(two, { mode: 'inclusive' })))).toEqual({ mode: 'inclusive' }); + const listed = { conditions: [{ label: 'A', expression: "x == 'a'" }] }; + expect(checkConfigAfter(chain(decisionFlow(two, listed)))).toEqual(listed); + // An EMPTY list declares no branch: the node routes on its edges, so it is rewritten. + expect(checkConfigAfter(chain(decisionFlow(two, { conditions: [] })))).toEqual({ conditions: [], mode: 'inclusive' }); + }); + + it('never touches a non-decision node with the same two conditioned out-edges', () => { + const stack = decisionFlow(two); + (stack.flows[0]!.nodes[1] as Record).type = 'screen'; + const result = chain(stack); + expect(checkConfigAfter(result)).toBeUndefined(); + expect(result.applied.filter((a) => a.conversionId === ID)).toEqual([]); + }); + + it('is idempotent: the migrated stack replays to itself with nothing applied', () => { + const first = chain(decisionFlow(two)); + const again = applyMetaMigrations(structuredClone(first.stack), 17, 18); + expect(again.stack).toEqual(first.stack); + expect(again.applied.filter((a) => a.conversionId === ID)).toEqual([]); + }); + + it('⛔ never replays on the authoring funnel — a decision written against the new contract stays exclusive', () => { + const authored = decisionFlow(two); + const notices: ConversionNotice[] = []; + const out = normalizeStackInput(structuredClone(authored), { onConversionNotice: (n) => notices.push(n) }); + expect(out).toEqual(authored); + expect(notices.map((n) => n.conversionId)).not.toContain(ID); + }); + + it('a seam that opens the retired window can still refuse it by id — the flow rehydration seam does', () => { + // The primitive behind `CONVERSIONS_NOT_REPLAYED_AT_REHYDRATION` in the + // automation engine: same bytes, window open, entry refused by name. + const notices: ConversionNotice[] = []; + const out = applyConversions(decisionFlow(two), { + includeRetired: true, + excludeConversionIds: [ID], + onNotice: (n) => notices.push(n), + }); + expect(((out.flows as any[])[0].nodes as any[])[1].config).toBeUndefined(); + expect(notices.map((n) => n.conversionId)).not.toContain(ID); + // FIRING CONTROL: with the window open and no refusal, it does fire. + const fired = applyConversions(decisionFlow(two), { includeRetired: true }); + expect(((fired.flows as any[])[0].nodes as any[])[1].config).toEqual({ mode: 'inclusive' }); + }); + }); + describe('registry invariants', () => { it('every conversion carries a fixture pair and a positive retirement window', () => { for (const c of ALL_CONVERSIONS) { diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index e8d00f813df..3bf05d89674 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -44,6 +44,7 @@ import { type ViewFilterOperator, } from '../ui/view.zod.js'; import { ASSEMBLED_VIEW_ITEMS_KEY } from '../ui/assembled-views.zod.js'; +import { FLOW_REGION_SLOTS_BY_TYPE } from '../automation/region-slots.js'; /** * Flow callout node type rename (protocol 11.0). @@ -11873,6 +11874,298 @@ const permissionRlsTagsRemoved: MetadataConversion = { }, }; +/** + * `decision` edge branching became EXCLUSIVE — first match in declaration + * order — and taking every true branch is now the declared `mode: 'inclusive'` + * (protocol 18, #15429; maintainer ruling 「跟主流对齐」, 2026-09-23). + * + * Until this change an edge-branched decision (no `config.conditions`) took + * EVERY out-edge whose condition held, one after another, while its schema, + * its docs and the engine's own comment all called it an exclusive gateway. The + * traversal now takes the FIRST conditioned out-edge that holds, in the order + * the flow's `edges` array declares them — the BPMN exclusive gateway, + * Salesforce Flow's Decision, n8n's Switch default — and `mode: 'inclusive'` + * is what an author writes to take every one (the BPMN inclusive gateway). + * + * ## What this rewrites, and the one thing it does not infer + * + * A decision with no `conditions` list (absent or empty), no `mode` of its + * own, and TWO OR MORE out-edges carrying a `condition` (a `fault` edge is + * error routing, not a branch) gets `mode: 'inclusive'` written explicitly, so + * a flow written while every true branch ran keeps that behaviour under the + * exclusive traversal. One conditioned edge plus a default is left alone: + * first-match and every-true-edge cannot differ there, so there is nothing to + * preserve. A decision whose author already wrote `mode` — either member — has + * spoken and is left alone, which is also what makes a second replay a no-op. + * + * ⛔ No smarter inference: a pair of conditions that PROVABLY partition + * (`x == 'a'` beside `x != 'a'`) is rewritten too. First-match equals + * every-true-edge only when the conditions are exclusive, and the ruling's + * predicate is the count, not a decision procedure over CEL — the author then + * deletes the key where the branches partition, and the `os migrate meta` diff + * is where that judgment is made (the paired D3 entry + * `flow-decision-edge-branching-first-match` says how). + * + * Reaches the nodes inside ADR-0031 regions (`loop.config.body`, + * `parallel.config.branches[]`, `try_catch.config.try` / `.catch`) through the + * shared slot table, judging each region's decisions against ITS OWN edges — + * a nested gate's out-edges live in the region, not in `flow.edges`. + * + * ## Where it replays — `os migrate meta --from 17`, and no load seam + * + * This is a DEFAULT FLIP, not a rename or a delete: the old shape (no `mode`) + * still parses and now MEANS exclusive, and the rewrite changes what it means. + * Such an entry is sound only where "this source predates the flip" is a fact, + * and that is the D3 chain alone — the operator asserts the source's age with + * `--from`. `retiredFromLoadPath: true` keeps it off the authoring funnel + * (`normalizeStackInput`), where an author who wrote two branches today, against + * the contract that says an omitted `mode` is exclusive, must not be rewritten + * into an inclusive gateway. The flag's jurisdiction ends there — its own + * docblock on `MetadataConversion` says so, and ADR-0087's 2026-07-31 addendum + * is why: data-at-rest seams replay retired entries on purpose — so every + * data-at-rest seam that opens the retired window has to refuse this id by + * name, on the artifact door's precedent for `app-hidden-to-unpublished` + * (#17885, `DEFAULT_FLIPS_NOT_REPLAYED_HERE` in `@objectstack/metadata-core`): + * the automation engine's flow rehydration seam does + * (`CONVERSIONS_NOT_REPLAYED_AT_REHYDRATION` in `service-automation/src/engine.ts`: + * `registerFlow` serves code-shipped flows, REST bodies and Studio saves alike, + * none of them dated), and the artifact-ingestion door does + * (`DEFAULT_FLIPS_NOT_REPLAYED_HERE` in `metadata-core/src/artifact-forward-conversion.ts`: + * a scaffolded `^17.0.0` floor is a dependency range, not an age). ⛔ Not replayed over stored `sys_metadata` + * flows either, by maintainer ruling (letter C on #15429): a stored row takes + * the first-match meaning on upgrade — BREAKING, stated in the D3 entry and the + * changeset with the one-line fix `mode: 'inclusive'` — with no stored-row + * rewrite, no cutoff and no read-path completion. `os migrate meta --stored` + * runs this entry's `apply` over each stored flow body only to LIST the nodes + * it would write (`collectDecisionModeReview` in + * `@objectstack/metadata-protocol`), discarding the result, so the review list + * and this predicate are one and the same. + */ +const flowDecisionModeInclusiveExplicit: MetadataConversion = { + id: 'flow-decision-mode-inclusive-explicit', + toMajor: 18, + retiredFromLoadPath: true, + surface: 'flow.nodes[].config.mode (decision)', + summary: + "edge-branched decision with two or more conditioned out-edges and no `mode`: `mode: 'inclusive'` written " + + 'explicitly (#15429 — the traversal became exclusive, first match in declaration order; the key keeps the ' + + 'every-true-edge behaviour those nodes had, and the author deletes it where the branches partition)', + apply(stack, emit) { + return mapCollection(stack, 'flows', (flow, path) => rewriteDecisionModesInGraph(flow, path, emit, 0)); + }, + fixture: { + // DISJOINT from every other flow fixture: only `start` / `decision` / `end` + // / `loop` nodes, no key another entry rewrites, so the whole table hits + // only this one. + before: { + flows: [ + { + name: 'lead_verdict', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + // The hotcrm#1555 shape: two conditioned out-edges that can BOTH + // hold for a confirmed record. Rewritten — every true branch ran. + { id: 'verdict', type: 'decision', label: 'Verdict?' }, + { id: 'refuse', type: 'end', label: 'Refuse' }, + { id: 'convert', type: 'end', label: 'Convert' }, + // One guarded branch plus the default: first-match and + // every-true-edge agree, so there is nothing to preserve. Left alone. + { id: 'converted', type: 'decision', label: 'Already converted?' }, + { id: 'abort', type: 'end', label: 'Abort' }, + { id: 'proceed', type: 'end', label: 'Proceed' }, + // The author already spoke. Left alone (and a second replay is a no-op). + { id: 'spoken', type: 'decision', label: 'Spoken', config: { mode: 'exclusive' } }, + { id: 'a', type: 'end', label: 'A' }, + { id: 'b', type: 'end', label: 'B' }, + // A `conditions` list is first-match on its own and refuses `mode`. Left alone. + { + id: 'listed', type: 'decision', label: 'Listed', + config: { conditions: [{ label: 'Hot', expression: 'lead.score > 80' }] }, + }, + { id: 'hot', type: 'end', label: 'Hot' }, + // The same two-branch shape inside a loop body, judged against the + // region's own edges. Rewritten. + { + id: 'sweep', type: 'loop', label: 'Sweep', + config: { + collection: '{leads}', + iteratorVariable: 'lead', + body: { + nodes: [ + { id: 'gate', type: 'decision', label: 'Gate' }, + { id: 'x', type: 'end', label: 'X' }, + { id: 'y', type: 'end', label: 'Y' }, + ], + edges: [ + { id: 'g1', source: 'gate', target: 'x', condition: "lead.status != 'suspected'" }, + { id: 'g2', source: 'gate', target: 'y', condition: { dialect: 'cel', source: "lead.status == 'confirmed'" } }, + ], + }, + }, + }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'verdict' }, + { id: 'e2', source: 'verdict', target: 'refuse', condition: "lead.status != 'suspected'", label: 'Refuse' }, + { id: 'e3', source: 'verdict', target: 'convert', condition: "lead.status == 'confirmed'", label: 'Convert' }, + { id: 'e4', source: 'converted', target: 'abort', condition: "lead.status == 'converted'" }, + { id: 'e5', source: 'converted', target: 'proceed', isDefault: true }, + { id: 'e6', source: 'spoken', target: 'a', condition: 'x > 1' }, + { id: 'e7', source: 'spoken', target: 'b', condition: 'x > 2' }, + { id: 'e8', source: 'listed', target: 'hot', label: 'Hot' }, + ], + }, + ], + }, + after: { + flows: [ + { + name: 'lead_verdict', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'verdict', type: 'decision', label: 'Verdict?', config: { mode: 'inclusive' } }, + { id: 'refuse', type: 'end', label: 'Refuse' }, + { id: 'convert', type: 'end', label: 'Convert' }, + { id: 'converted', type: 'decision', label: 'Already converted?' }, + { id: 'abort', type: 'end', label: 'Abort' }, + { id: 'proceed', type: 'end', label: 'Proceed' }, + { id: 'spoken', type: 'decision', label: 'Spoken', config: { mode: 'exclusive' } }, + { id: 'a', type: 'end', label: 'A' }, + { id: 'b', type: 'end', label: 'B' }, + { + id: 'listed', type: 'decision', label: 'Listed', + config: { conditions: [{ label: 'Hot', expression: 'lead.score > 80' }] }, + }, + { id: 'hot', type: 'end', label: 'Hot' }, + { + id: 'sweep', type: 'loop', label: 'Sweep', + config: { + collection: '{leads}', + iteratorVariable: 'lead', + body: { + nodes: [ + { id: 'gate', type: 'decision', label: 'Gate', config: { mode: 'inclusive' } }, + { id: 'x', type: 'end', label: 'X' }, + { id: 'y', type: 'end', label: 'Y' }, + ], + edges: [ + { id: 'g1', source: 'gate', target: 'x', condition: "lead.status != 'suspected'" }, + { id: 'g2', source: 'gate', target: 'y', condition: { dialect: 'cel', source: "lead.status == 'confirmed'" } }, + ], + }, + }, + }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'verdict' }, + { id: 'e2', source: 'verdict', target: 'refuse', condition: "lead.status != 'suspected'", label: 'Refuse' }, + { id: 'e3', source: 'verdict', target: 'convert', condition: "lead.status == 'confirmed'", label: 'Convert' }, + { id: 'e4', source: 'converted', target: 'abort', condition: "lead.status == 'converted'" }, + { id: 'e5', source: 'converted', target: 'proceed', isDefault: true }, + { id: 'e6', source: 'spoken', target: 'a', condition: 'x > 1' }, + { id: 'e7', source: 'spoken', target: 'b', condition: 'x > 2' }, + { id: 'e8', source: 'listed', target: 'hot', label: 'Hot' }, + ], + }, + ], + }, + expectedNotices: 2, + }, +}; + +/** + * The ruling's predicate, as a number: an edge-branched decision is rewritten + * when at least this many of its out-edges carry a `condition`. Below it, + * first-match and every-true-edge cannot differ. + */ +const DECISION_MODE_INCLUSIVE_MIN_CONDITIONED_EDGES = 2; + +/** + * Depth ceiling for the region recursion — mirrors the walkers' ceiling + * (`walk.ts`, `control-flow.zod.ts`): a self-referencing region in a + * hand-built stack must not recurse without bound. + */ +const DECISION_MODE_REGION_DEPTH_CEILING = 32; + +/** An edge whose `condition` carries a predicate — bare text, or the parsed `{ dialect, source }` envelope. */ +function edgeCarriesCondition(edge: Record): boolean { + const c = edge.condition; + if (typeof c === 'string') return c.trim() !== ''; + if (isDict(c)) return typeof c.source === 'string' && c.source.trim() !== ''; + return false; +} + +/** + * One graph — a flow, or one ADR-0031 region — for + * {@link flowDecisionModeInclusiveExplicit}: rewrite its own decisions against + * its own `edges`, then descend into each node's region slots. Copy-on-write: + * the same reference comes back when nothing under it changed. + */ +function rewriteDecisionModesInGraph( + graph: Record, + path: string, + emit: (detail: ConversionApplication) => void, + depth: number, +): Record { + const nodes = graph.nodes; + if (!Array.isArray(nodes)) return graph; + const edges = Array.isArray(graph.edges) ? graph.edges.filter(isDict) : []; + let changed = false; + const nextNodes = nodes.map((node, i) => { + if (!isDict(node)) return node; + const nodePath = `${path}.nodes[${i}]`; + let next = node; + + if (node.type === 'decision') { + const cfg = isDict(node.config) ? node.config : {}; + const declaresBranches = Array.isArray(cfg.conditions) && cfg.conditions.length > 0; + if (!declaresBranches && !('mode' in cfg)) { + const conditioned = edges.filter( + (e) => e.source === node.id && e.type !== 'fault' && edgeCarriesCondition(e), + ).length; + if (conditioned >= DECISION_MODE_INCLUSIVE_MIN_CONDITIONED_EDGES) { + next = { ...node, config: { ...cfg, mode: 'inclusive' } }; + emit({ + from: `mode unset (${conditioned} conditioned out-edges; every one whose condition held was taken)`, + to: 'inclusive', + path: `${nodePath}.config.mode`, + }); + } + } + } + + if (depth < DECISION_MODE_REGION_DEPTH_CEILING) { + const slots = typeof next.type === 'string' ? FLOW_REGION_SLOTS_BY_TYPE.get(next.type) : undefined; + if (slots && isDict(next.config)) { + let nextConfig = next.config; + for (const slot of slots) { + const raw = nextConfig[slot.key]; + if (slot.arity === 'many') { + if (!Array.isArray(raw)) continue; + let branchesChanged = false; + const nextBranches = raw.map((branch, bi) => { + if (!isDict(branch)) return branch; + const mapped = rewriteDecisionModesInGraph(branch, `${nodePath}.config.${slot.key}[${bi}]`, emit, depth + 1); + if (mapped !== branch) branchesChanged = true; + return mapped; + }); + if (branchesChanged) nextConfig = { ...nextConfig, [slot.key]: nextBranches }; + } else { + if (!isDict(raw)) continue; + const mapped = rewriteDecisionModesInGraph(raw, `${nodePath}.config.${slot.key}`, emit, depth + 1); + if (mapped !== raw) nextConfig = { ...nextConfig, [slot.key]: mapped }; + } + } + if (nextConfig !== next.config) next = { ...next, config: nextConfig }; + } + } + + if (next !== node) changed = true; + return next; + }); + return changed ? { ...graph, nodes: nextNodes } : graph; +} + export const CONVERSIONS_BY_MAJOR: Readonly> = { 11: [flowNodeHttpRename, pageKindJsxToHtml, flowNodeFilterAlias, objectCompactLayoutRename], 13: [stackRolesToPositions, owdLegacyReadAliases, sharingRecipientRoleToPosition], @@ -11990,6 +12283,7 @@ export const CONVERSIONS_BY_MAJOR: Readonly` beside `<=`, or a ' + + 'guard beside `isDefault: true` — delete the written `mode`; the run is unchanged either ' + + 'way and the exclusive default is the honest declaration; (2) where the flow relies on ' + + 'more than one branch running for one record, keep `mode: \'inclusive\'`; (3) where the ' + + 'conditions overlap by accident, narrow them into a partition and delete the key, then ' + + 're-run the flow on a record that satisfied both and confirm exactly one successor ' + + 'ran — the passed-over branch now leaves a `skipped` step in the run log. `os validate` ' + + 'reports `flow-decision-inclusive-overlap` on every decision that keeps the key with ' + + 'two or more conditioned out-edges, so the review list is the lint output. Then each ' + + 'deployment: `os migrate meta --stored` lists, under `decisionModeReview`, every stored ' + + 'decision with two or more conditioned out-edges and no `mode` — each one already ' + + 'evaluates first-match, and the pass writes none of them — so where one of those nodes ' + + 'meant every branch, declare `mode: \'inclusive\'` on it in the designer; a node ' + + 'that declares `mode` either way leaves the list. A decision registering with ' + + '`mode` beside a non-empty `conditions` list, or with a `mode` outside ' + + '`\'exclusive\' | \'inclusive\'`, is refused at registration and by `os validate` with the ' + + 'schema\'s own sentence; nothing else about `conditions`-list decisions changes. ' + + 'Run `os migrate meta --from 17` to list the mechanical edits for existing sources; apply them by hand.', +}; diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index b73066717ef..5ed7b57ef98 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -5463,7 +5463,19 @@ const step18: MigrationStep = { + 'rename above: `health.circuitBreaker.monitoringWindow` → `monitoringWindowMs` is no longer ' + 'converted, because the whole block it lived in is now removed, and ' + '`connector-health-and-trigger-durations-unit-in-key` keeps only `triggers[].interval` → ' - + '`intervalSeconds`.', + + '`intervalSeconds`. ' + + 'Finally it makes edge-branched `decision` nodes EXCLUSIVE (#15429, maintainer ruling ' + + '「跟主流对齐」): the first conditioned out-edge that holds, in declaration order, is the ' + + 'branch, and taking every true branch is the declared `mode: \'inclusive\'`. The D2 ' + + 'conversion `flow-decision-mode-inclusive-explicit` writes that key onto every decision ' + + 'with two or more conditioned out-edges and no `conditions` list, so a flow written while ' + + 'every true branch ran keeps its behaviour; it is a default flip, so it is retired from ' + + 'the load path AND refused by the flow rehydration seam and the artifact-ingestion door, ' + + 'and replays only here — the paired semantic entry carries the judgment the diff then ' + + 'asks for. BREAKING for flows stored in `sys_metadata`, by maintainer ruling: such a ' + + 'decision with no `mode` takes the first-match meaning on upgrade and nothing rewrites ' + + 'it; `os migrate meta --stored` lists each one for review, and `mode: \'inclusive\'` is ' + + 'the one-line fix where a node meant every branch.', conversionIds: [ 'field-malformed-scale-precision-removed', 'record-chatter-position-vocabulary', @@ -5508,6 +5520,7 @@ const step18: MigrationStep = { 'currency-config-precision-removed', 'permission-rls-tags-removed', 'action-aria-removed', + 'flow-decision-mode-inclusive-explicit', ], semantic: [ // One file per entry under `entries/semantic/`, concatenated here sorted by @@ -10563,6 +10576,71 @@ const step18: MigrationStep = { + 'predicate parses and registers byte-identically to before, a decision with no `conditions` ' + 'still routes by its out-edges, and an absent screen field `visibleWhen` is still legal.', }, + { + id: 'flow-decision-edge-branching-first-match', + // No backticks in `surface` — build-upgrade-guide.ts renders it inside a code + // span AND a table cell. + surface: + 'flow.nodes[].config.mode (decision) — an OMITTED mode on a decision that branches ' + + 'on its out-edges and carries two or more conditioned ones', + replacement: + 'nothing, where the out-edge conditions partition (exactly one can hold for any ' + + 'record): an omitted `mode` now means exclusive, the first true edge in declaration ' + + 'order wins, and the run is what it always was. `mode: \'inclusive\'` where the flow ' + + 'RELIES on more than one branch running for one record — the value the D2 conversion ' + + '`flow-decision-mode-inclusive-explicit` writes onto every such decision so nothing ' + + 'changes silently. Where the conditions overlap by accident (a `!=` guard beside a ' + + 'later `==` branch), neither: narrow them into a partition, or mark the fallback ' + + '`isDefault: true`, and delete the written key.', + reason: + 'A DEFAULT FLIP of a shipped node type, ruled rather than patched: the schema, the docs ' + + 'and the engine\'s own comment all called an edge-branched decision an exclusive gateway ' + + 'while the traversal took EVERY out-edge whose condition held, one after another, and ' + + 'reported nothing — hotcrm#1555 rendered a refusal screen AND ran the conversion in one ' + + 'execution. The traversal now matches the declaration (BPMN exclusive gateway, ' + + 'Salesforce Flow Decision, n8n Switch default), and the every-true-edge behaviour is the ' + + 'BPMN inclusive gateway an author must write down. The KEY converts mechanically and ' + + 'does: `flow-decision-mode-inclusive-explicit` writes `mode: \'inclusive\'` wherever two ' + + 'or more conditioned out-edges leave a decision that declares no `conditions` list, so ' + + 'the migrated source runs exactly as before. What does NOT convert is the INTENT: the ' + + 'count cannot tell a partition (where the key is redundant) from a reliance on ' + + 'multi-branch runs (where it is load-bearing) from an accidental overlap (where the ' + + 'old behaviour was the bug), so the mechanical edit list the chain replay prints is ' + + 'where that judgment is made, node by node. And the conversion replays ONLY there: it is a ' + + 'default flip, so the authoring funnel never rewrites a source written against the ' + + 'new contract, and the automation engine\'s flow rehydration seam and the ' + + 'artifact-ingestion door both refuse it by id (a code-shipped flow, a REST body, a Studio ' + + 'save and a scaffolded artifact all arrive undated). BREAKING for stored rows, by ' + + 'maintainer ruling: the promise that a flow keeps its behaviour is kept by authored ' + + 'sources and built artifacts only. A decision stored in `sys_metadata` ' + + 'with no `conditions` list, no `mode` and two or more conditioned out-edges takes the new ' + + 'meaning on upgrade — it evaluates first-match — and nothing rewrites the row: no ' + + 'stored-row migration, no cutoff, no read-path completion, because nothing about a stored ' + + 'row says it was saved before the flip. The one-line fix, for a stored node that meant ' + + 'every branch, is `mode: \'inclusive\'`; `os migrate meta --stored` lists every such node, ' + + 'report only, so an operator can review the candidates before and after the upgrade.', + acceptanceCriteria: + 'Review every `flow-decision-mode-inclusive-explicit` line the chain replay lists for ' + + 'each authored stack: (1) where the two (or more) ' + + 'out-edge conditions partition — a predicate and its negation, `>` beside `<=`, or a ' + + 'guard beside `isDefault: true` — delete the written `mode`; the run is unchanged either ' + + 'way and the exclusive default is the honest declaration; (2) where the flow relies on ' + + 'more than one branch running for one record, keep `mode: \'inclusive\'`; (3) where the ' + + 'conditions overlap by accident, narrow them into a partition and delete the key, then ' + + 're-run the flow on a record that satisfied both and confirm exactly one successor ' + + 'ran — the passed-over branch now leaves a `skipped` step in the run log. `os validate` ' + + 'reports `flow-decision-inclusive-overlap` on every decision that keeps the key with ' + + 'two or more conditioned out-edges, so the review list is the lint output. Then each ' + + 'deployment: `os migrate meta --stored` lists, under `decisionModeReview`, every stored ' + + 'decision with two or more conditioned out-edges and no `mode` — each one already ' + + 'evaluates first-match, and the pass writes none of them — so where one of those nodes ' + + 'meant every branch, declare `mode: \'inclusive\'` on it in the designer; a node ' + + 'that declares `mode` either way leaves the list. A decision registering with ' + + '`mode` beside a non-empty `conditions` list, or with a `mode` outside ' + + '`\'exclusive\' | \'inclusive\'`, is refused at registration and by `os validate` with the ' + + 'schema\'s own sentence; nothing else about `conditions`-list decisions changes. ' + + 'Run `os migrate meta --from 17` to list the mechanical edits for existing sources; apply them by hand.', + }, // No backticks in `surface` — build-upgrade-guide.ts renders it inside a code // span already, and a nested backtick would close it. {