Flow decision / conditional-edge conditions never resolve variables — every bare-identifier condition is compared as a literal string

[yinlianghui]

Split out of #489 (found while verifying #560, which fixes the days_in_stage half). This is pre-existing on main and untouched by that PR.

Symptom

opportunity_stagnation selects the right deals and then does nothing. Every loop iteration stops at the check_not_nudged decision node — no notification, no follow-up task. The sweep looks healthy (sys_job.last_status = success, failure_count = 0) and produces zero output.

Root cause

Flow conditions are authored as bare strings and stay bare strings in the build artifact — objectstack build does not normalize them into an Expression envelope:

$ node -e "…dist/objectstack.json…"
decision node config: {"condition":"existingStallTask == null"}
edge b2: {…,"condition":"existingStallTask == null","label":"First nudge"}

AutomationEngine.evaluateCondition (@objectstack/service-automation) branches on whether the condition is an envelope. Only an envelope reaches the CEL engine; a plain string falls through to a legacy template path that:

1. substitutes only {var} brace placeholders from the variable map,
2. splits the remainder on the first comparison operator,
3. hands both sides to compareValues, which compares them as strings when they are not both numeric.

So existingStallTask == null never resolves the variable. It evaluates 'existingStallTask' === 'null' → false, and the gate never opens.

Note the irony: the CEL branch's own error message tells authors to write bare CEL and not to use braces —

Conditions are bare CEL (e.g. record.rating >= 4); do not wrap field references in {…} template braces.

— but bare strings never reach that branch, so following the advice is exactly what breaks.

Verified

Against a booted server (fresh DB, temporary 1-minute cron):

Condition form	Run 1	Run 2
existingStallTask == null (current)	loop reaches check_not_nudged, 0 tasks	0 tasks
{existingStallTask} == null	notify + create fire, 30 tasks	gate closes correctly, still 30

The brace form works in both directions: String(null) is 'null' so the first pass opens the gate, and a found record stringifies to '[object Object]' so the second pass closes it. That confirms both the diagnosis and that the idempotency logic itself is sound — only the variable reference is broken.

Blast radius

This is not one flow. Every decision node and conditional edge in src/flows/ uses the bare-identifier form, so all of them are currently evaluating string literals. Reading the same code path, the failure mode is not uniformly "false" — which is worse than a uniform no-op:

• existingMember == null (campaign_enrollment), existingRenewalTask == null / existingRenewalOpp == null (contract_renewal) — same shape as the verified case, so these gates should be stuck closed too.
• record.rating >= 4 (lead_assignment) — neither side is numeric, so it string-compares 'record.rating' >= '4', and 'r' > '4' is true. The Hot branch would always be taken and record.rating < 4 always false. Not yet verified on a running server; flagging it as the case that argues for fixing the class rather than one flow.

Suggested fix

Two candidate directions, and the choice matters more than the diff:

1. Author-side — switch every flow condition to the brace form ({existingStallTask} == null). Verified to work, but it keeps the stringly-typed comparison semantics (no types, no null-vs-'null' distinction) and contradicts the engine's own guidance.
2. Envelope-side — author conditions as CEL envelopes (e.g. via a tagged template) so they reach the CEL branch and get real typed evaluation. Closer to the platform's stated intent; needs a check that flow variables bind the way the CEL branch expects (it passes them as both record and extra).

Worth deciding before touching the flows, since option 2 may also belong upstream — build arguably should be normalizing these strings into envelopes, which is what the "plain string accepted for back-compat; build emits canonical envelope" note in @objectstack/spec describes for other expression sites.

Whichever way it goes, a test/ guard should pin it so the bare form cannot come back silently.

[yinlianghui]

Correcting the root cause and the suggested fix above — further digging found the actual missing link, and the "Author-side / brace form" option should not be followed.

What the original analysis missed

defineFlow() exists and runs FlowSchema.parse() (@objectstack/spec/src/automation/flow.zod.ts:324), and the edge condition field is already typed ExpressionInputSchema (:185) — a union whose string arm transforms into { dialect: 'cel', source } (src/shared/expression.zod.ts:92-95). The normalization machinery has been there all along.

The reason it never runs: every flow in src/flows/ is authored as a bare typed object literal (import type * as Automation + const X: Flow = {…}) and defineFlow() is never called. No parse, so the transform never fires and the bare string reaches the artifact untouched. Flow accepts the bare-string shorthand by design (that is what ExpressionInput is for), so TypeScript is satisfied and nothing downstream normalizes it. That closes the gap in the original write-up, which described the artifact contents correctly but not why the correctly-typed edge field was also affected.

Which condition actually gates the branch

The edges, not the decision node. traverseNext partitions outgoing edges and calls evaluateCondition(edge.condition, variables) (@objectstack/service-automation/dist/index.js:1692). So edge-condition normalization is the behavioural fix.

Separately, the decision node's config.condition (singular) is dead config: the executor reads config.conditions (plural, an array of {expression, label} — :2539), finds nothing, and returns branchLabel: 'default'. That is currently harmless only because no edge carries label: 'default', so traverseNext falls back to the full edge set (:1678-1681). If anyone ever labels an edge default, that edge will monopolise the branch and bypass every conditional edge. Worth removing the dead key and keeping a guard against the label.

Why the brace form is the wrong fix

It works, but by accident: it is handled by the legacy template path, which is exactly the path that should stop being used. Migrating to it would cement stringly-typed comparison semantics across the whole flow layer.

What an envelope migration actually requires — it is not drop-in

The CEL branch binds variables as { extra: { ...vars, vars }, record: vars } (:1797), so flow variables live under record.* / extra.*; a bare identifier like existingStallTask does not resolve in CEL scope. And that branch throws on evaluation failure rather than returning false.

So switching authoring to defineFlow() without rewriting the condition sources converts today's silent-false into a runtime error. The two have to land together, per flow:

existingStallTask == null   →   record.existingStallTask == null

Loud failure is the improvement worth having — but it means this needs per-flow verification, not a bulk find-and-replace.

Revised plan

1. In this repo — author flows through defineFlow() and rewrite every condition to resolvable CEL, together, verified per flow. This covers the full behavioural defect, including the lead_assignment record.rating >= 4 case that currently string-compares to a true Hot branch.
2. Upstream — two things cannot be fixed from here: FlowNodeSchema.config is z.record(z.string(), z.unknown()) (:102), so no node-level expression config can ever be normalized; and evaluateCondition's legacy path silently string-compares instead of failing, which is what turns an unresolved identifier into a wrong branch rather than an error.

Item 1 does not depend on item 2 and should not wait for it.

---

Generated by Claude Code

[yinlianghui]

Correction to my previous comment: the claim that condition sources must be rewritten to record.* / extra.* paths is wrong. I derived it from record: vars in the engine's CEL call without following through to how buildScope assembles the scope. The existing sources are already valid CEL, and the migration is effectively drop-in. Details, because the reason matters for the fix:

buildScope (@objectstack/formula/dist/index.js:187-207) sets scope.record = ctx.record first, then finishes with Object.assign(scope, ctx.extra). The engine passes { extra: { ...vars, vars }, record: vars }, and vars contains a record key (AutomationEngine does variables.set("record", context.record) at dist/index.js:861). So the spread overwrites scope.record with the real triggering record, and every flow variable also lands in the top-level scope under its bare name.

Verified against the packaged engine with the exact context shape the automation engine builds:

Source	Result
record.rating >= 4	true (record.rating = 5)
record.rating < 4	false
existingStallTask == null	true
oppRecord.amount > 500000	true
vars.firstUser != null	true
record.stage == "closed_won" && previous.stage != "closed_won"	true

All six forms currently authored in src/flows/ evaluate correctly as CEL. Nothing is wrong with the condition sources — the only defect is that they never reach the CEL branch, because they are bare strings rather than Expression envelopes. That also retires the "loud runtime errors" risk I raised: there is no identifier rewrite, so nothing new starts throwing.

Consequence for the fix

defineFlow() is not the best mechanism after all. Trigger conditions live in the start node's config.condition (e.g. case-csat-followup.flow.ts:37), and node config is z.record(z.string(), z.unknown()) — so FlowSchema.parse() cannot normalize them. defineFlow() would fix edges only and leave every trigger gate broken.

The P tagged template from @objectstack/spec covers both sites, because it puts the envelope in at authoring time with no parse step involved:

import { P } from '@objectstack/spec';

// edge — `condition` is ExpressionInput (string | Expression), so this type-checks
{ id: 'b2', …, condition: P`existingStallTask == null`, label: 'First nudge' }

// start-node trigger gate — config is Record<string, unknown>, so an envelope passes through
config: { objectName: 'crm_case', triggerType: 'record-after-update',
          condition: P`record.status == "closed" && (previous == null || previous.status != "closed")` }

evaluateCondition branches on typeof expression === 'object' && 'dialect' in expression, so an envelope from either site reaches the CEL engine.

So the repo-side fix is: wrap every condition in P, sources unchanged, plus a test/ guard that no flow condition is a bare string. The upstream items from my previous comment stand unchanged — the untyped node config is exactly why P has to be applied by hand at the trigger sites rather than falling out of schema parsing.

---

Generated by Claude Code

[yinlianghui]

ref:objectstack-ai/objectstack#4336