Skip to content

fix(flows): author conditions as CEL envelopes so they actually evaluate - #565

Merged
yinlianghui merged 1 commit into
mainfrom
claude/hooks-flows-automation-conflict-isglbj
Jul 31, 2026
Merged

fix(flows): author conditions as CEL envelopes so they actually evaluate#565
yinlianghui merged 1 commit into
mainfrom
claude/hooks-flows-automation-conflict-isglbj

Conversation

@yinlianghui

Copy link
Copy Markdown
Collaborator

Description

Every decision node and conditional edge in src/flows/ authored its condition as a bare string. AutomationEngine.evaluateCondition only routes an Expression envelope ({ dialect, source }) to the CEL engine — a bare string falls through to a legacy template path that substitutes {var} braces and then compares both sides as strings. No condition in this app was ever evaluated as an expression.

The failure mode was not a uniform no-op, which is what made it dangerous:

Authored Actually evaluated Result
existingStallTask == null 'existingStallTask' === 'null' always false
record.rating >= 4 'record.rating' >= '4' always true ('r' > '4')

So opportunity_stagnation selected the right stalled deals (after #560) and then silently dropped every one of them at its idempotency gate — no notification, no follow-up task, and a success run record either way. In the other direction, lead_assignment pinned the Hot branch open and never took the Standard path.

This is #562, split out of #489 while verifying #560.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Related Issues

Refs #562

Changes Made

  • All 41 condition sites across 13 flow files now use the P tagged template from @objectstack/spec, which emits the envelope at authoring time.
  • The condition sources are unchanged. They were already valid CEL: the engine merges its variable map onto the CEL scope via ctx.extra (buildScope assigns ctx.extra after scope.record, and the variable map carries a record key), so flow variables resolve by bare name and record is the triggering record. Only the envelope was missing.
  • defineFlow() would not have been sufficient. It normalizes the typed edge condition (FlowEdgeSchema.condition is ExpressionInputSchema), but a node's config is z.record(z.string(), z.unknown()) — so every start-node trigger gate would have stayed a bare string. P covers both sites and needs no parse step.
  • New guard in test/metadata-references.test.ts: no flow condition may be a bare string, at either site, with a floor on the number of sites inspected so a walker that stops matching cannot make the test vacuous.
  • The two case_escalation assertions in test/actions-flows-integrity.test.ts read condition.source instead of the raw value.
  • Two conditions that were built by string concatenation across lines (case_escalation, opportunity_approval) are now single multi-line templates — CEL treats the newline as whitespace.

Testing

  • Unit tests pass (pnpm test — 169 passing)
  • Linting passes (pnpm lint — 1 warning / 9 suggestions, byte-identical to the same run on an unmodified tree)
  • Build succeeds (pnpm build); pnpm verify clean end to end
  • Manual testing completed
  • New tests added

Artifact check — all 46 condition sites in dist/objectstack.json (more than the 41 source sites because demo_bootstrap generates edges with flatMap) are cel envelopes, zero bare strings, and every source parses against the real CEL engine.

Verified on a booted server (pnpm start, fresh DB, temporary 1-minute cron on opportunity_stagnation)

  • The sweep now creates 30 follow-up tasks, one per stalled deal — it created zero before this change, stopping at the check_not_nudged decision node every iteration.
  • The count holds at 30 across later ticks, and each subject appears exactly once, so the idempotency gate now opens on the first pass and closes on every pass after it.
  • The guard was checked against a deliberate regression: reverting one condition to a bare string fails the new test with the flow name, node id, and offending source.

Additional Notes

Not verified end to end: the lead_assignment Hot/Standard branch. Its trigger is record-after-create, the REST data route requires auth, and the seeded leads' next_followup_date values are seed literals rather than flow output — so there was no zero-auth path to exercise it on a live server. What is verified is the mechanism (identical evaluateCondition → edge-condition path, proven end to end by the stagnation sweep) and the expressions themselves (record.rating >= 4true and record.rating < 4false against the packaged CEL engine using the exact context shape the automation engine builds).

Dead config left in place: a decision node's config.condition (singular) is never read — the executor reads config.conditions (plural, an array of {expression, label}) and so always returns branchLabel: 'default'. That is currently harmless because no edge carries label: 'default', which makes traverseNext fall back to the full edge set. Restructuring it would change routing, so it is out of scope here, but if anyone ever labels an edge default, that edge will monopolise the branch and bypass every conditional edge. Noted on #562.

Upstream, not fixable here: node config is an untyped z.record(z.string(), z.unknown()), so no node-level expression can ever be normalized by schema parsing; 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. Both are recorded on #562.


Generated by Claude Code

Every decision node and conditional edge in src/flows/ wrote its condition
as a bare string. AutomationEngine.evaluateCondition only routes an
Expression envelope ({ dialect, source }) to the CEL engine; a bare string
falls through to a legacy template path that substitutes {var} braces and
then compares both sides as strings. No condition in the app was ever
evaluated as an expression.

The failure mode was not a uniform no-op, which is what made it dangerous:

  existingStallTask == null  ->  'existingStallTask' === 'null'  ->  always false
  record.rating >= 4         ->  'record.rating' >= '4'          ->  always TRUE

so opportunity_stagnation dropped every stalled deal at its idempotency
gate while reporting a successful run, and lead_assignment pinned the Hot
branch open and never took the Standard path.

All 41 condition sites now use the P tagged template from @objectstack/spec,
which emits the envelope at authoring time. The sources are unchanged: they
were already valid CEL, because the engine merges its variable map onto the
CEL scope via ctx.extra, so flow variables resolve by bare name and `record`
is the triggering record. Only the envelope was missing.

defineFlow() would not have sufficed: it normalizes the typed edge
condition, but a node's config is z.record(z.unknown()), so every start-node
trigger gate would have stayed a bare string.

Verified on a booted server with a temporary 1-minute cron: the stagnation
sweep now creates 30 follow-up tasks, one per stalled deal, and holds at 30
across later ticks as the idempotency gate closes. It created zero before.

A guard in test/metadata-references.test.ts fails on any condition that is
not a CEL envelope, at either site, so the bare form cannot come back
silently. The two case-escalation assertions that read the condition as a
string now read the envelope's source.

Refs #562

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FJygXUafjc7ckGMmE2AaGz
@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hotcrm Ignored Ignored Jul 31, 2026 3:48am

Request Review

@yinlianghui
yinlianghui marked this pull request as ready for review July 31, 2026 04:06
@yinlianghui
yinlianghui merged commit 672e653 into main Jul 31, 2026
9 checks passed
yinlianghui pushed a commit that referenced this pull request Jul 31, 2026
#565 landed the same CEL-envelope fix this branch carried, using the `P`
tagged-template helper from @objectstack/spec — a cleaner notation, so the flow
files take main's version wholesale. What remains here is the part #565 did not
cover, plus the two red tests its merge left on main.

main is currently RED. Both failures are tests this branch's parent (#563)
introduced, and both fired exactly as designed:

  - flow-scheduled: the KNOWN DEFECT case asserted the stagnation gate stays
    shut. #565 opened it, so the test failed with the message it was written to
    emit — "gate now opens — rewrite this test". Rewritten here to assert the
    real behaviour (nudge created, idempotent across sweeps, re-arms once the
    task completes).
  - flow-record-change: `opportunity_approval` asserted `typeof
    startCondition === 'string'`. #565 converted it to an envelope. The
    assertion was pinning the NOTATION rather than the thing that matters, so
    it now accepts either form and checks a non-empty expression exists.

Second defect #565 left reachable: `timestamp(currentContract.end_date)` throws
"timestamp() requires a string in ISO 8601 format" because `end_date` is a DATE
field arriving as YYYY-MM-DD. While the condition was a bare string it was
never evaluated, so this was latent; wrapping it as a real envelope makes it
throw mid-sweep. Appending T00:00:00Z fixes it — verified load-bearing by
reverting it, which fails 4 contract_renewal tests.

So on main as it stands, contract_renewal books nothing: the gate evaluates and
then blows up. That is a live regression, not a theoretical one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
yinlianghui pushed a commit that referenced this pull request Jul 31, 2026
#565 landed the CEL-envelope conversion, so this branch no longer carries it.
What remains is the timestamp() ISO defect it left reachable, the two tests its
merge turned red on main, and the guards that keep the class fixed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
yinlianghui added a commit that referenced this pull request Jul 31, 2026
…ns-and-recurrence

fix(flows): unbreak main — contract_renewal throws; rewrite the two tests #565 turned red
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants