Skip to content

Commit 6fb0658

Browse files
committed
Merge origin/main into claude/issue-16111-flow-template-leaves-outside-filters
origin/main landed a change to `packages/lint/src/lint-flow-patterns.ts` — the other caller of `stripRegions`, whose behaviour this branch's signature change has to preserve. Merged before opening the PR so the suite is measured on the joint tree (AGENTS.md §10). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8
2 parents 50f343a + 3890244 commit 6fb0658

11 files changed

Lines changed: 1136 additions & 55 deletions
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
"@objectstack/lint": patch
3+
---
4+
5+
`flow-decision-unconditional-branch` now reports the decision that gates on nothing — the shape the rule used to skip.
6+
7+
A `decision` whose out-edges carry no `condition` and no `isDefault`, and whose node declares no `config.conditions[]`, selects no branch at all: the automation engine's own decision executor reports no branch when `conditions[]` is empty, so traversal considers every out-edge and each successor runs on every pass. The gateway is decoration. The rule could not see that shape, because it was framed as "an unconditional edge undercuts a guarded one" and read zero guarded edges as nothing to undercut — so the strictly worse gateway was the one case that stayed silent, and it is the harder one to notice in review, because the node still says `type: 'decision'`.
8+
9+
Same rule id, same `warning` tier, with its own message: it names the out-edges that run unconditionally and offers the three fixes (a `condition` per branch plus `isDefault: true` on the fallback, a `config.conditions[]` whose `label` matches an out-edge, or dropping `type: 'decision'` for the node the gateway already behaves as). The mixed shape — one guarded out-edge beside an unconditional one — keeps its existing wording and its single finding.
10+
11+
Decisions that do declare their routing stay silent, including the two that are easiest to catch by mistake: an ordinary gateway with guarded edges, and a decision that routes by `config.conditions[]` labels alone with bare out-edges. A decision declaring a label no out-edge claims remains the gating `flow-branch-label-unmatched` on its own, with no second finding piled on the same node.
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
"@objectstack/service-automation": patch
3+
---
4+
5+
A wizard screen is no longer skipped after a durable pause because a record column happens to be an array.
6+
7+
`judgeHeadlessScreen` decides a screen was already answered by proving the negative: a field is **not** caller-supplied when the subject record carries that key and `params` holds the same value — necessary because the params bag a flow action arrives with is `{ ...record, recordId, <object>Id, ...params }`, so every column of the launched row is in there whether the caller named it or not.
8+
9+
That comparison was reference identity (`Object.is`), which is real in memory and does not survive persistence. A suspended run stores its context as JSON and resumes from the parsed copy — and the store is preferred over the in-process cache whenever one is wired, so no restart is needed. After that round trip an **array or object** column is equal but no longer identical: the record leg could not disprove it, the field read as caller-supplied, and a later screen with no required fields of its own was **skipped on a run that had supplied nothing**. An interactive user pressed a button and never saw a form they should have been shown; the run completed carrying the row's own value as if they had typed it. Reproduced end to end against a wired store, not inferred.
10+
11+
The record leg now compares by value (`isDeepStrictEqual`), which survives serialisation. That predicate compares primitives with `Object.is` itself, so this is a strict widening of the "not caller-supplied" set — every pair the old check called equal it still calls equal, plus the structurally identical non-primitives. More screens render, never fewer, which is the direction this module resolves every ambiguity in.
12+
13+
**Accepted cost, precisely.** A caller that genuinely re-sends a value structurally identical to the row's column is no longer distinguishable from the dispatcher's seed, so it now gets the screen rendered instead of skipped — a lost skip on a headless call, never a lost run, and the same trade the module's other legs already make. Scalar columns behave exactly as before, on both sides of a pause. The row-id leg keeps identity comparison deliberately: a row id is a scalar by construction, so serialisation cannot defeat it and there is nothing there to widen. Measured overhead is a deep compare per declared screen field at screen entry: ~1.5 µs added for a deliberately maximal screen that declares a field for every one of a ten-column row, which is about 38% of one `JSON.stringify` of the run context — a cost the durable store already pays on every suspend.
14+
15+
This closes the gap the same release's screen-flow headless-satisfaction note records as known.
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/service-automation": patch
3+
---
4+
5+
A restored suspension now carries the state the run was paused with, including nested values.
6+
7+
`restoreConsumedSuspension` is the operator exit from a run whose resume consumed the pause and then failed downstream: it puts the suspension back so the run is resumable again. What it put back was documented as the pause "verbatim", and was — for the top-level variables only.
8+
9+
The flow scope a resume hands the downstream nodes was rebuilt as `new Map(Object.entries(run.variables))`: that copies the keys and shares every value object with the parked snapshot. An executor that keeps state in the scope and updates it **in place**`map` tracks its progress in `<nodeId>.$mapState` — therefore wrote straight through into the snapshot, and the journal recorded the result as the pause. An operator repairing a stranded `map` run got a snapshot claiming progress made by the attempt that failed, not the progress the run actually had when it paused.
10+
11+
Measured, not inferred: the durable row held `started: 1` at the pause and the restore put back `started: 99`.
12+
13+
The pause's variables are now copied before the failed attempt runs, on the line that already captures the pause's step count for the same reason. No later placement works — the node mutates and then throws, so a copy taken when the journal is written copies the mutation. Nothing else changes: the running flow still sees exactly the scope it saw before, the resume ordering is untouched, and a value that cannot be copied falls back to the previous behaviour with a warning rather than costing the operator the repair.

packages/lint/src/lint-flow-patterns.test.ts

Lines changed: 164 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -893,7 +893,18 @@ describe('flow-error-label-not-fault (#3863)', () => {
893893
it.each(['decision', 'approval'])(
894894
"does NOT flag label:'error' out of a %s node — the label IS the branch selector",
895895
(sourceType) => {
896-
expect(lintFlowPatterns(edgeFlow({ label: 'error' }, sourceType))).toHaveLength(0);
896+
// Scoped to THIS rule since #16093. The assertion was a global zero,
897+
// which held only because the fully-inert decision was invisible: the
898+
// `decision` half of this fixture declares no `conditions[]` and no edge
899+
// is guarded, so it now (correctly) draws a `flow-decision-unconditional-
900+
// branch` warning of its own. A bare `label` is not a guard in either
901+
// half of that rule — the mixed branch has always counted a labelled
902+
// edge the decision cannot select as ungated — so exempting it here to
903+
// keep the zero would make one rule id answer two ways. What this case
904+
// tests is unchanged: an error-ish label out of a branching node is not
905+
// the #3863 footgun.
906+
const fnds = lintFlowPatterns(edgeFlow({ label: 'error' }, sourceType));
907+
expect(fnds.filter((f) => f.rule === FLOW_ERROR_LABEL_NOT_FAULT)).toHaveLength(0);
897908
},
898909
);
899910
});
@@ -1013,8 +1024,15 @@ describe('flow-decision-unconditional-branch (#4414)', () => {
10131024
})).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH)).toHaveLength(0);
10141025
});
10151026

1016-
it('does NOT flag a decision with no guarded edge at all — nothing to undercut', () => {
1017-
expect(lintFlowPatterns({
1027+
// #16093 — the fully-inert gateway, the shape the rule could not see. Until
1028+
// this case landed, `if (gated.length === 0) continue` dropped exactly the
1029+
// node whose declared branching is worst: a `decision` with NO edge
1030+
// `condition`, NO `isDefault` and NO `config.conditions[]`. The mixed case
1031+
// (one guarded edge, one not) was at least partially routed; this one was not
1032+
// routed at all, and it is the harder one to spot, because the node still
1033+
// says `type: 'decision'`.
1034+
it('flags a decision that gates on NOTHING — no edge guard, no conditions[]', () => {
1035+
const fnds = lintFlowPatterns({
10181036
flows: [{
10191037
name: 'plain',
10201038
nodes: [{ id: 'start', type: 'start', config: {} }, { id: 'check', type: 'decision' }, { id: 'a', type: 'screen', config: {} }],
@@ -1023,9 +1041,152 @@ describe('flow-decision-unconditional-branch (#4414)', () => {
10231041
{ id: 'e2', source: 'check', target: 'a' },
10241042
],
10251043
}],
1044+
}).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH);
1045+
1046+
expect(fnds).toHaveLength(1);
1047+
// Advisory, like the mixed shape: an unconditional fan-out is legal — the
1048+
// node is merely typed `decision` while behaving as a plain step. See the
1049+
// severity policy at the top of the rule module.
1050+
expect(fnds[0].severity).toBeUndefined();
1051+
expect(fnds[0].where).toContain("decision 'check'");
1052+
// Names the node's out-edge targets and what the decision gates on.
1053+
expect(fnds[0].message).toContain('gates on NOTHING');
1054+
expect(fnds[0].message).toContain("'a'");
1055+
expect(fnds[0].message).toContain('EVERY pass');
1056+
expect(fnds[0].hint).toContain('conditions');
1057+
});
1058+
1059+
// The wording is the whole point of keeping this under the SAME rule id: the
1060+
// fully-inert shape must NOT read as the mixed "guarded alongside unguarded"
1061+
// sentence, which would send the author looking for a guard that is not there.
1062+
it('does not describe the inert decision with the mixed-branch wording', () => {
1063+
const inert = lintFlowPatterns({
1064+
flows: [{
1065+
name: 'plain',
1066+
nodes: [{ id: 'start', type: 'start', config: {} }, { id: 'check', type: 'decision' }, { id: 'a', type: 'screen', config: {} }],
1067+
edges: [
1068+
{ id: 'e1', source: 'start', target: 'check' },
1069+
{ id: 'e2', source: 'check', target: 'a' },
1070+
],
1071+
}],
1072+
}).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH);
1073+
expect(inert[0].message).not.toContain('alongside');
1074+
1075+
const mixed = lintFlowPatterns(guardFlow()).filter(
1076+
(f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH,
1077+
);
1078+
expect(mixed[0].message).toContain('alongside');
1079+
// One finding each — the inert branch must not double-report the mixed one.
1080+
expect(mixed).toHaveLength(1);
1081+
});
1082+
1083+
it('flags EVERY out-edge of an inert decision in one finding, not one each', () => {
1084+
const fnds = lintFlowPatterns({
1085+
flows: [{
1086+
name: 'plain',
1087+
nodes: [
1088+
{ id: 'start', type: 'start', config: {} },
1089+
{ id: 'check', type: 'decision' },
1090+
{ id: 'a', type: 'screen', config: {} },
1091+
{ id: 'b', type: 'screen', config: {} },
1092+
],
1093+
edges: [
1094+
{ id: 'e1', source: 'start', target: 'check' },
1095+
{ id: 'e2', source: 'check', target: 'a' },
1096+
{ id: 'e3', source: 'check', target: 'b' },
1097+
],
1098+
}],
1099+
}).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH);
1100+
expect(fnds).toHaveLength(1);
1101+
expect(fnds[0].message).toContain("'a'");
1102+
expect(fnds[0].message).toContain("'b'");
1103+
});
1104+
1105+
// As measured on the reporting app: the inert decision sat inside a `loop`
1106+
// body, so the finding has to survive region descent (#5383) too.
1107+
it('flags a nested inert decision inside a loop body', () => {
1108+
const fnds = lintFlowPatterns(loopBodyFlow({
1109+
nodes: [
1110+
{ id: 'gate', type: 'decision' },
1111+
{ id: 'nudge', type: 'create_record', config: { objectName: 'touch_log' } },
1112+
],
1113+
edges: [{ id: 'b1', source: 'gate', target: 'nudge' }],
1114+
})).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH);
1115+
expect(fnds).toHaveLength(1);
1116+
expect(fnds[0].where).toBe("flow 'campaign_enrollment' · loop 'loop_leads' body · decision 'gate'");
1117+
expect(fnds[0].message).toContain("'nudge'");
1118+
});
1119+
1120+
// ⛔ NEGATIVE CONTROL — the ordinary gateway. Guarded edges AND a matching
1121+
// `config.conditions[]`: the shape every correct flow has, and the one a new
1122+
// positive case is most likely to break. It must stay silent.
1123+
it('does NOT flag a normal decision — guarded edges plus matching conditions[]', () => {
1124+
expect(lintFlowPatterns(guardFlow({
1125+
proceed: { isDefault: true },
1126+
conditions: [{ label: 'Yes', expression: "lead.status == 'converted'" }],
1127+
}))).toHaveLength(0);
1128+
});
1129+
1130+
// ⛔ NEGATIVE CONTROL — routing by `config.conditions[]` alone. The edges
1131+
// carry no `condition` and no `isDefault`, so `gated` is empty, but the
1132+
// decision DOES declare its branching: the labels select the path. Not inert.
1133+
it('does NOT flag a decision that routes by conditions[] alone', () => {
1134+
expect(lintFlowPatterns({
1135+
flows: [{
1136+
name: 'by_label',
1137+
nodes: [
1138+
{ id: 'start', type: 'start', config: {} },
1139+
{
1140+
id: 'check', type: 'decision',
1141+
config: { conditions: [{ label: 'Yes', expression: 'lead.score > 50' }, { label: 'No', expression: 'true' }] },
1142+
},
1143+
{ id: 'a', type: 'screen', config: {} },
1144+
{ id: 'b', type: 'screen', config: {} },
1145+
],
1146+
edges: [
1147+
{ id: 'e1', source: 'start', target: 'check' },
1148+
{ id: 'e2', source: 'check', target: 'a', label: 'Yes' },
1149+
{ id: 'e3', source: 'check', target: 'b', label: 'No' },
1150+
],
1151+
}],
10261152
})).toHaveLength(0);
10271153
});
10281154

1155+
// ⛔ NO DOUBLE REPORT — a decision that declares `conditions[]` no out-edge
1156+
// claims is already the GATING `flow-branch-label-unmatched`; the inert branch
1157+
// must not pile a second, contradictory finding on the same node.
1158+
it('leaves an unmatched-label decision to flow-branch-label-unmatched alone', () => {
1159+
const fnds = lintFlowPatterns({
1160+
flows: [{
1161+
name: 'unmatched',
1162+
nodes: [
1163+
{ id: 'start', type: 'start', config: {} },
1164+
{ id: 'check', type: 'decision', config: { conditions: [{ label: 'Yes', expression: 'true' }] } },
1165+
{ id: 'a', type: 'screen', config: {} },
1166+
],
1167+
edges: [
1168+
{ id: 'e1', source: 'start', target: 'check' },
1169+
{ id: 'e2', source: 'check', target: 'a' },
1170+
],
1171+
}],
1172+
});
1173+
expect(fnds.filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH)).toHaveLength(0);
1174+
expect(fnds.filter((f) => f.rule === FLOW_BRANCH_LABEL_UNMATCHED)).toHaveLength(1);
1175+
});
1176+
1177+
// A decision with no out-edge at all is a different defect and is skipped
1178+
// before any of this (`outs.length === 0`); the inert branch must not claim it.
1179+
it('does NOT flag a decision with no out-edge at all', () => {
1180+
expect(lintFlowPatterns({
1181+
flows: [{
1182+
name: 'dead_end',
1183+
nodes: [{ id: 'start', type: 'start', config: {} }, { id: 'check', type: 'decision' }],
1184+
edges: [{ id: 'e1', source: 'start', target: 'check' }],
1185+
}],
1186+
}).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH)).toHaveLength(0);
1187+
});
1188+
1189+
10291190
it('does NOT flag a fault edge — error routing is not branch selection', () => {
10301191
expect(lintFlowPatterns(guardFlow({
10311192
proceed: { isDefault: true },

0 commit comments

Comments
 (0)