fix(flows): make conditions over flow variables total, in the two ways their two failure classes need (#643) - #649
Merged
Conversation
…s their two failure classes need (#643) `test/flow-condition-totality.test.ts` (#633) made every `record.x` / `previous.x` read in a record-change flow condition total. It filtered explicitly to the trigger record, leaving conditions over flow-local variables unmeasured. Measured now, on 17.0.0-rc.1, they are exposed in TWO distinct ways whose remedies are opposite. Measured mechanism (`AutomationEngine.evaluateCondition`): | expression | var unbound | var = null | sparse row | | ---------------- | ------------------------ | ---------- | ---------- | | `X.f == 1` | ABORT Unknown variable | ABORT | ABORT | | `vars.X.f == 1` | ABORT No such key | ABORT | ABORT | | `has(X.f)` | ABORT Unknown variable | false | false | | `has(vars.X)` | false | true | true | Only the `vars.`-scoped guard survives an unbound variable, so every guard added here leads with `has(vars.X)`. Three further facts decided the shape of the fix: `get_record` always writes its `outputVariable` and `findOne` answers a miss with `null`; a failed node throws and the run stops, so a node that could not bind never reaches a reader; and declaring a variable in `flow.variables` binds NOTHING at runtime — `FlowVariableSchema` is strict `{ name, type, isInput, isOutput }` with no `defaultValue`, and `execute` binds a declared input only when the caller passed it in `context.params`. Class 1 — field reads off a `get_record` output are driver rows, and get #633's guards (`vars.`-scoped): - `campaign_enrollment` `vars.campaignRecord.status` (node check_campaign_open, edge e4) — LIVE DEFECT, reproduced end-to-end. A campaign deleted or sharing-hidden between the action click and the run left the variable bound to `null`; the read aborted with `No such key: status`, the run was recorded failed and no lead was enrolled. `crm_campaign.status` is `required`, so the sparse-COLUMN variant was already closed — the null-RECORD variant was not. - `quote_generation` `oppRecord.stage` (check_stage, e4a/e4b) and `opportunity_approval` (+ `_on_create`) `oppRecord.amount` (check_high_value, e5/e6) — total TODAY only because neighbouring schemas close the gap (`stage`/`amount` are `required`; `crm_quote.crm_account` is `required` so a null `oppRecord` fails `create_quote` one node earlier). Guarded anyway. - `contract_renewal` `currentContract.end_date` / `.renewal_notice_days` / `.auto_renewal` (check_notice_window, check_auto_renewal, b1/b6) — NOT in the issue's table; found by sweeping every flow. A loop item over `data.find` rows, with the two renewal columns only DEFAULTED, and the aborts land inside `timestamp()` / `int()`, taking a 500-contract sweep down with them. Class 2 — an unbindable VARIABLE, which must NOT get a `has()` guard: - `lead_conversion` `vars.createOpportunity` (decision_opportunity, e16/e17) — LIVE DEFECT, reproduced end-to-end. It is a screen- collected input, so it is bound only if the runner sends it back in the resume signal; a runner posting just the touched fields left it unbound and edge e16 aborted with `No such key: createOpportunity`, so the lead was never marked converted. Fixed by BINDING it — an `assignment` node ahead of the screen seeding `false`, matching the screen field's own `defaultValue` — not by guarding. A guard would bury the policy "a missing answer means No" inside a predicate and leave the graph defect in place. - `matchedAccount` / `matchedContact` / `firstUser` / `existingMember` / `existingRenewalTask` / `existingRenewalOpp` / `existingStallTask` / `ownerAnyDeal` / `existingForecast` measured CLEAN — a `get_record` dominates every read — and are deliberately left unguarded. `test/flow-variable-conditions.test.ts` pins both properties as separate assertions with distinct failure messages (per #643 item 3): a `has()` sweep for field reads, and a structural intersection-dataflow proof that every variable a condition reads is bound on every path reaching it. It also re-measures the abort table on this evaluator, sweeps every condition on the real engine, and reproduces both defects end-to-end over `InMemoryDriver`. Without the flow fixes it fails 21 assertions. Also documented, and used by the sweep: a `decision` node's SINGULAR `config.condition` — the shape every flow here authors — is never read by the engine, which evaluates `config.conditions[]` and the edges. Both copies are guarded so the inert statement of intent cannot drift from the edge that decides. Fixes #643 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SS7C5SXpniKeCApxgARyf
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
os-zhuang
marked this pull request as ready for review
August 2, 2026 19:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #643
#633 让 record-change 流程条件里每一处
record.x/previous.x读取都变成全函数,但它的扫描面被显式限制在触发记录上。本件把剩下的那一面——流程局部变量——量了一遍。结论没有继承 #633:同一个AutomationEngine.evaluateCondition、同一种 CEL、同一种后果,但稀疏形状的来源有两种,补法完全相反,而且 issue 正文里给出的那张表并不完整。一、实测到的机制(17.0.0-rc.1,全部在
evaluateCondition上跑出来)evaluateCondition把运行时变量 Map 摊平成一个对象,用{ extra: { ...vars, vars }, record: vars }求值——所以变量X既能裸读(X.f)也能带作用域读(vars.X.f)。两者并不等价:X.f == 1Unknown variable: XNo such key: fNo such key: fvars.X.f == 1No such key: XNo such key: fNo such key: fvars.X != nullNo such key: Xfalsetruetruehas(X.f)Unknown variable: Xfalsefalsetruehas(vars.X)falsetruetruetrue只有
vars.作用域的守卫能扛住未绑定变量。 最顺手的裸写法has(X.f)照样抛错。所以本 PR 新加的每一处守卫都以has(vars.X)开头,并把quote_generation/opportunity_approval原有的裸作用域一并改成vars.作用域。还有三条实测事实决定了修法:
get_record无条件执行variables.set(outputVariable, record),而findOne未命中返回null(对InMemoryDriver实测)——所以跑过get_record之后变量一定是绑定的,最差绑到null。flow.variables里声明变量,在运行时什么也不绑定。FlowVariableSchema是 strict 的{ name, type, isInput, isOutput },没有defaultValue这个键;execute只在context.params[name] !== undefined时才绑定一个声明过的 input。issue 正文和认领评论里写的"在flow.variables里声明变量并给默认值"这条补法,在当前 spec 上不可表达——详见下面第三节。另外顺带量到并被本 PR 依赖的一点:
decision节点单数的config.condition(本仓库每个流程都这么写)引擎从不读取,它只读config.conditions[](复数)和边上的condition。所以下面每一处复现都是在边上炸的。两份拷贝都补了守卫——节点上那份今天是惰性的,但它是意图的书面声明,不能和真正做决定的边漂移。二、第 1 类:
get_record输出是驱动行 → 补has()守卫campaign_enrollmentvars.campaignRecord.status(节点check_campaign_open、边e4)——实打实的缺陷,已端到端复现。用户点了动作之后、流程走到这一步之前,campaign 被删掉(或被共享规则挡住),campaignRecord就绑定成null,读取以No such key: status中止,run 记为 failed,一条线索都没入组。注意crm_campaign.status是required: true,所以稀疏列那一路本来就是关着的——开着的是空记录那一路。照搬 Flow start conditions carry no has() guards — measure whether the abort-and-skip class of #630 reaches them #633 的结论只会检查 requiredness,然后得出"没问题"。quote_generationoppRecord.stage(check_stage、e4a/e4b)与opportunity_approval(含_on_create)oppRecord.amount(check_high_value、e5/e6)——今天之所以是全函数,纯粹是两个邻居 schema 恰好堵住了:stage/amount在crm_opportunity上required,而crm_quote.crm_account也required,所以oppRecord为空时create_quote会先一步失败。这是邻居的性质、不是谓词的性质,离一个required: false就翻车。照样补守卫。contract_renewalcurrentContract.end_date/.renewal_notice_days/.auto_renewal(check_notice_window、check_auto_renewal、b1/b6)——issue 的表里没有这三处,是把全部 24 个流程扫一遍才捞出来的。currentContract是对data.find结果的循环项,每个元素都是原始驱动行;两个 renewal 列只是defaultValue、不是required,默认值出现之前写入的行两样都没有。而且这里的中止发生在timestamp()/int()函数调用内部,一条合同就能把 500 行的定时扫描整个带走。两个分支必须划分的地方(
e4a/e4b、e5/e6)守卫写成相反极性,未知形状落在保守分支上:quote 保留原 stage(报价照样生成),deal 落到mark_approved(经理已经批过了,读不出金额不该把一个已批准的 deal 卡在锁死的、无法裁决的总监步骤里)。三、第 2 类:变量本身可能未绑定 → 绝不补
has(),改成在图里绑定lead_conversionvars.createOpportunity(decision_opportunity、e16/e17)——实打实的缺陷,已端到端复现。它是 screen 收集的输入,只有 runner 在 resume signal 里把它送回来才会绑定;runner 只回传用户动过的字段时它就是未绑定的,边e16以No such key: createOpportunity中止,线索永远没被标成已转换,account / contact / opportunity 一个都没留下。补法不是守卫。守卫会把"没答等于否"这条策略埋进谓词里,真正错的地方——图把变量留在未绑定状态——原封不动。但如上所述,"在
flow.variables里给默认值"这条路在 17.0.0-rc.1 上不存在(schema 是 strict 且无defaultValue,引擎也没有任何应用默认值的步骤)。于是用平台真正有的机制表达同一个语义:在 screen 之前放一个assignment节点把它绑成false,和 screen 字段自己的defaultValue: false一致;用户真答了的时候 resume signal 会覆盖它。这样"每条路径上都绑定"是图保证的,不是客户端保证的。matchedAccount/matchedContact/firstUser/existingMember/existingRenewalTask/existingRenewalOpp/existingStallTask/ownerAnyDeal/existingForecast量下来都是干净的——每一处读取都有一个get_record支配它,未命中时绑定null,条件有结论。demo_bootstrap在零用户的 org 上跑完整个流程(有复现用例)。这些故意不加守卫:加了就正是上面那种"拿守卫掩盖"的动作。四、测试策略(issue 第 3 条):单独一个文件,两条独立断言
采纳了认领评论里的倾向,但拆分的轴是作用域而不是类:
test/flow-condition-totality.test.ts继续独占触发记录作用域(#633),新建的test/flow-variable-conditions.test.ts独占流程变量作用域,并在内部把两类拆成各自的it()、各自的失败信息——因为两类的改法相反,合进一条断言的话失败信息说不清该怎么改。文件头的 house-rule 块把上面那张实测机制表、两类的分工、以及"声明 ≠ 绑定"这个坑写在了一个地方。文件里有四层:
has()扫描——每一处字段读取都必须同时带has(vars.X)和has(vars.X.f);序关系比较的操作数额外要!= null(两种极性都算)。loop体作为区域在外层变量作用域里展开),证明每个条件读到的变量都在到达它的每条路径上被绑定。失败信息明确写着"不要用has()修这个"。附一个合成流程的负例,证明分析本身有效(声明在flow.variables里 + screen 收集 = 仍然不保证)。evaluateCondition,跑遍get_record输出能取到的各种形状;探针取值全部来自谓词自己的字面量,保证类型正确(拿任意值去填测的是类型一致性、不是全函数性)。同时把上面那张中止表在这个求值器上重新量一遍——平台哪天让 CEL 变宽容了,这里会红,那是预期信号。InMemoryDriver+ 真AutomationEngine:消失的 campaign、省略了复选框的 resume signal、零用户的 org、以及各条正常路径(守卫没有把合法的入组 / stage 推进 / 用户明确选"是"关掉)。把
src/flows/恢复成 main 的版本后,这个文件红 21 条,其中两条是端到端复现。验证
已在最新
origin/main(c9d5009f)上 rebase 后重跑全绿。附带发现(未在本 PR 修,另开 issue)
decision节点单数的config.condition引擎从不读取——本仓库 24 个流程全都这么写,全是惰性元数据(行为正确只因为条件在边上有一份拷贝)。这是一处"declared ≠ enforced"。FlowVariableSchema没有defaultValue,导致"声明一个带默认值的流程变量"这件事在 spec 层面无法表达;本 PR 只能用assignment节点绕过去。这是平台侧的缺口。Generated by Claude Code