fix: #2305 emit Condition rule groups under conditionConfig

[bongbongcrypto]

Issue

Closes #2305

What this changes

Following the shape you accepted, in three parts.

1. lib/workflow/node-builders.ts emits conditionConfig: { group }. It was the only
producer of the top-level group, and it has emitted it since the file was introduced.
lib/scan/factory/node-builders.ts already produces the nested shape, so this removes the
drift rather than adding a second supported shape. I did not teach the resolver to read
group, for the reason you gave: the run never reaches the resolver, and an alias would
make the second shape permanent.

2. drizzle/0151_keep_2305_condition_group_to_condition_config.sql repairs rows already
written. For each Condition node carrying a top-level group, the key moves under
conditionConfig and the stale one is dropped. Where both keys exist the existing
conditionConfig wins and only the stale key is removed, which is the state a user reaches
by opening the node in the editor. Re-running changes nothing.

updated_at is deliberately not touched. This is a repair, not a user edit, and moving it
would reorder every affected workflow in the user's list.

3. The leftover-literal error names the field that carried the token.
scanForLeftoverLiterals now threads a path, UnresolvedRef carries it, and the message
reads {{@step-1:Node.field}} at group.rules[0].leftOperand (Reference left in rendered config...).

On your question about how far to take that third part: I kept it to naming the path and
did not attempt to validate keys. data.config is an open record by design, so any
allowlist would either be wrong for plugins or need a registry that does not exist. The
path is already known at the point the token is found, costs one parameter to carry, and
generalises to any array-valued or unlifted config field rather than to Condition alone.
Root-level tokens omit the clause, so single-field configs read as they do today.

Scope

One change. Part 1 without part 2 leaves every organization provisioned so far broken,
including the public hub rows that other users clone. Part 2 without part 1 repairs the
data and then the next signup writes it wrong again. Part 3 is the diagnostic that made the
first two take four runs to find, and it touches the same failure path.

Touched outside the obvious: drizzle/meta/_journal.json gains the entry for the new
migration.

How it was verified

Added to tests/unit/template-fail-closed.test.ts, over a config with the exact shape from
the issue:

• the message names group.rules[0].leftOperand and still contains the token
• UnresolvedRef.path carries the same path, with reason literal-leftover
• a root-level token produces no path clause
• the same config without the stale key does not throw

For the migration I could not run it against a database, so I checked the transformation
rather than the SQL: the same rules applied in JavaScript to the node shapes this touches.

case	result
seeded starter	stale group removed, promoted under conditionConfig
seeded starter	condition, id, label, status, position unchanged
opened in the editor, both keys present	existing conditionConfig kept, stale key dropped
already correct	byte identical
non-Condition node with a group key	untouched
run twice	identical to running once
node order in the row	preserved

Two things I would like checked on your side, since I cannot:

• the SQL itself against a real database, particularly the jsonb_agg ... ORDER BY ord
  regrouping, which is where an error would silently reorder a workflow's nodes
• whether 0151 is the right way to ship a data fix here, or whether these run outside the
  numbered sequence. I added the journal entry on the assumption that they do not.

Same honest note as on #2319: I have not run pnpm check or pnpm type-check locally,
because I do not install and run an unfamiliar repository on the machine I work from. The
changed files were type-checked in isolation for syntax. CI on this PR is the real check
and I will fix what it reports.

Screenshots

Nothing renders.

---

• Targets staging
• Title carries the issue number, or an exemption applies
• pnpm check and pnpm type-check pass (not run locally, see the note above)
• No secrets, .env files, or credentials committed

🤖 Generated with Claude Code

[github-actions]

About the build check on this pull request

This pull request comes from a fork, so GitHub does not pass it the credentials build normally uses for our image registry cache and staging build configuration. The build still runs and still compiles the image, so a red build here is real; it just takes longer than on team branches.

Every workflow run on a pull request from a fork also waits for a maintainer to approve it, so checks can sit at "awaiting approval" for a while after each push. Nothing is needed from you for either of these.

[suisuss]

What this changes

lib/workflow/node-builders.ts:101 stops emitting data.config.group and emits data.config.conditionConfig = { group }; condition is unchanged. drizzle/0151_keep_2305_condition_group_to_condition_config.sql repairs existing rows with one UPDATE workflows, and _journal.json gains the matching entry. lib/workflow/executor/template-resolution.ts threads a path accumulator through scanForLeftoverLiterals, adds it to UnresolvedRef, and interpolates it into the message. Four tests.

I confirmed the diagnosis independently rather than taking it: processTemplates (executor.workflow.ts:971-983) recurses into plain objects but excludes arrays, so config.group.rules is copied verbatim with its tokens intact, while scanForLeftoverLiterals does walk arrays and therefore finds them. processActionConfig (:2341-2342) blanks only condition and conditionConfig before rendering, which is why moving the group under conditionConfig fixes it. node-builders.ts really was the only producer of the top-level key.

Does it match the description

Matches, and the three parts are as described.

Blocking

• drizzle/0151_keep_2305_condition_group_to_condition_config.sql:41-42 - || on a non-object conditionConfig concatenates rather than merges. I ran the migration file against Postgres 16.14: with "conditionConfig": null alongside a stale group, the result is "conditionConfig": [null, {"group": {...}}], and an existing array concatenates. -> resolveConditionExpression (lib/workflow/nodes/condition/resolver.ts:20-26) then reads .group off an array, gets undefined, and silently falls back to the condition string, while sanitize-nodes.ts:148-165 declines to repair it because typeof group !== "object". -> Guard the ELSE branch on jsonb_typeof(... conditionConfig) = 'object' and replace outright otherwise.

• drizzle/0151...sql:31,57 - both guards test key presence, not type. With "group": null the migration writes "conditionConfig": {"group": null}, which I reproduced. -> That is a new editor crash rather than a repair: components/workflow/config/action-config.tsx:299-303 calls visualConditionToExpression whenever existingConditionConfig is truthy, and groupToExpression (lib/workflow/nodes/condition/expression.ts:167-168) dereferences group.rules.length, so opening the node throws. -> Use jsonb_typeof(node #> '{data,config,group}') = 'object' in both guards. I found no path on staging that writes JSON null there, so this is latent rather than demonstrated - but the migration is the one place that should not create the shape it exists to remove.

Mechanical - actionable as-is

• drizzle/0151...sql:5-11 - the justification in the comment is wrong. It says the public hub rows "insert with a fixed id and onConflictDoNothing() and are therefore never refreshed from the fixture". Neither seeder does that: scripts/seed/seed-onboarding-workflows.ts:56-119 and seed-tempo-templates.ts:68-96 both select-then-insert-or-update, and they do refresh a fixed-id row when seededAt is set and the row is within USER_EDIT_EPSILON_MS. grep onConflictDoNothing over both returns nothing. The migration is still necessary, for a different reason worth recording instead: the per-org rows inserted at signup (lib/auth.ts:871-886) get generated ids and no seededAt, so userEdited is true and no seeder can ever match them.

• No test covers either of the two seams that matter. Nothing asserts buildConditionNode emits conditionConfig.group and no top-level group - that is the one line stopping the bug recurring, and a three-line assertion in tests/unit/onboarding-workflows.test.ts closes it. And the fourth new test, "does not throw once the group is nested under conditionConfig", destructures group away and asserts that a config with no tokens does not throw; it exercises neither processActionConfig nor the lifting.

• dedupeByToken (template-resolution.ts:133-141) keys on token::reason and keeps the first, so a token appearing at two paths reports only one location. Worth a sentence in the doc comment now that the path is user-visible.

• Check 0151 is still free before merge - it is unused on staging today, but another migration in flight would collide.

Verdict

Changes requested - the migration can write two shapes it should not, one of which crashes the editor it is meant to unbreak.

The rest of it holds up well and I want to be specific about what I checked, because a data migration deserves it. The node order is preserved by the ORDER BY ord in jsonb_agg. It is idempotent - a row carrying only conditionConfig is not matched on a re-run. There is no DDL, so it takes ROW EXCLUSIVE and row locks for the statement only, with no read blocking; measured on Postgres 16.14 over 200k rows at 88 MB with one in three matching, it ran in 2.65s. Give me the real prod row count and I will sanity-check that before it goes out. No down migration, consistent with the ten prior data-only migrations, and safe to roll the app back against because nothing has ever read the top-level key. _journal.json is contiguous with a monotonic when and no snapshot, which matches repo practice.

Not splitting this. The three seams are independent in the correctness sense, but the migration alone leaves every new signup recreating broken rows and the builder change alone leaves every existing org broken - that is a sequencing coupling, and they should land together.

One thing worth its own issue rather than this PR: the fix addresses the array reaching the scan, not the asymmetry that processTemplates skips arrays while scanForLeftoverLiterals walks them. No action schema declares an array field today, so nothing else is exposed, but web3 args and functionArgs are free-form JSON and an MCP-authored array containing a token would land in the same place.

[bongbongcrypto]

Thank you for running the migration rather than reading it. Both blockers reproduce exactly as described, and pushing back on a data migration this way caught a third one that was mine.

Pushed 34421e2. The branch is rebased onto staging because 0151 is no longer free - the org circuit breaker took it - so the migration is now 0152_keep_2305_condition_group_to_condition_config and the journal entry follows staging's.

The two blockers

• || is gone from the non-object path. jsonb_typeof(...conditionConfig) IS DISTINCT FROM 'object' replaces outright; only an object is merged into. A JSON null, an array and a string all end up as {"group": {...}}.
• Both guards now test jsonb_typeof(node #> '{data,config,group}') = 'object'. A "group": null or a non-object group is not matched at all, so nothing is written to conditionConfig and the editor has nothing new to throw on. Leaving the stale non-object key is deliberate: it carries no rules and therefore no tokens, so it does not abort a run, and removing it is a different change.

A third one, found while testing the fix

My first pass used <> rather than IS DISTINCT FROM. An absent conditionConfig makes #> return SQL NULL rather than JSON null, so <> was NULL, the CASE fell through to the merge, NULL || anything is NULL, and jsonb_set returned NULL for the whole node - replacing the node with JSON null in the array. That is the common case, not an edge one. The fixture suite now asserts no node becomes null, which is the check that caught it.

Verification

Postgres 18.3, eleven fixtures: an absent, JSON null, array, string and object conditionConfig; an object conditionConfig that already has a group; a null and a non-object group; a non-Condition node; an already-migrated row; and a three-node workflow. Asserted per row, then run a second time and diffed. The previous revision fails five, this one passes all, node order is a,n1,z either way and updated_at does not move. I do not have Postgres 16.14 here, so if the jsonb_typeof and IS DISTINCT FROM behaviour is worth confirming on the version you measured on, that is worth doing before merge. Still happy to sanity-check the runtime against a real prod row count.

The mechanical items

• Corrected the comment. I checked both seeders and you are right that neither uses onConflictDoNothing(); the reason I have recorded instead is the one you gave, and I confirmed it: lib/auth.ts:871-886 inserts the three fixtures with no id and no seededAt, so seed-onboarding-workflows.ts matches them neither by eq(workflows.id, fixture.id) nor by the updatedAt-to-seededAt window, since seededMs is 0.
• Added the assertion to tests/unit/onboarding-workflows.test.ts: every Condition node in ONBOARDING_WORKFLOW_FIXTURES carries conditionConfig.group and no top-level group. With the previous builder it fails on all four - onb-aave-health, onb-aave-health-sepolia, onb-aave-health-base-sepolia and onb-whale-withdrawal.
• Dropped the fourth test rather than repairing it. You are right that it exercised neither processActionConfig nor the lifting, and processActionConfig is declared inside the executor's closure, so a unit test cannot reach it. The builder assertion is the real guard.
• Added the sentence about dedupeByToken to the path doc comment.
• biome check is clean on all five files. It was not before: the existing expect(message).toContain(...) at the end of template-fail-closed.test.ts needed wrapping.

On the array asymmetry: agreed it is its own issue, and I will open one for it rather than widen this.

[bongbongcrypto]

Opened the array asymmetry as #2359, with a reproduction: the same token in the same node resolves when functionArgs is a JSON string and aborts when it is a real JSON array, which the API accepts and stores unchanged. Kept it out of this PR.

[suisuss]

What this changes

One commit of your own, 34421e25, plus a merge of staging - the compare spans 67 commits but the six files that move are yours.

The migration is renumbered to 0152 (staging took 0151 for the circuit breaker) and reguarded: both group checks are now jsonb_typeof(...) = 'object' rather than jsonb_exists, and the conditionConfig CASE lost its COALESCE and gained a leading IS DISTINCT FROM 'object' arm that replaces outright. node-builders.ts is unchanged from the reviewed version, template-resolution.ts gains one doc sentence, and tests/unit/onboarding-workflows.test.ts is new.

Every blocking item lands. I ran the migration on PG16 against thirteen fixtures rather than reading it: conditionConfig as JSON null or as an array is replaced rather than concatenated; group: null and group: "nope" are left untouched; an existing object conditionConfig carrying a group wins and only the stale key is dropped; non-Condition nodes, null array members, nodes without data, empty arrays and non-array nodes are all untouched; node order survives ORDER BY ord; the second run reports UPDATE 0; updated_at does not move. 916 ms over 50,000 rows with one in three matching. The journal entry is contiguous and monotonic and passes drizzle-journal-ordering.test.ts.

I also confirmed the two things the new key depends on: resolveConditionExpression (lib/workflow/nodes/condition/resolver.ts:20-31) has never read config.group, so an unmigrated row behaves exactly as it does on staging; and executor.workflow.ts:2353 / :2428 remove and re-attach conditionConfig around the assert, so the emitted key is the key that handling already names.

Does it match the description

Matches.

Blocking

• drizzle/0152_keep_2305_condition_group_to_condition_config.sql:47-81 - the migration promotes a stale seeded group over an expression the user is actually running. resolveConditionExpression prefers conditionConfig.group and only falls through to config.condition when it is absent (resolver.ts:24-31), and handleModeSwitch("expression") clears conditionConfig precisely so the raw string wins (components/workflow/config/action-config.tsx:350-353). But nothing clears the top-level group: lib/workflow/editor/sanitize-nodes.ts:168 spreads ...config, so on a seeded workflow it survives every save. -> A seeded Condition workflow that a user switched to expression mode reaches the migration as {group: <seeded>, condition: <user's>} with no conditionConfig. After it, conditionConfig.group exists and outranks the expression, so the workflow silently starts evaluating the seeded condition instead of the user's, with no error and nothing in the row to show what happened. Before the migration the stale key was inert, so this is created here. -> Skip rows where config.condition is a non-empty string and conditionConfig is absent; the stale group on those rows is not a repair candidate.

Mechanical - actionable as-is

• drizzle/0152_keep_2305_condition_group_to_condition_config.sql:33 - "That is the common case, so it is covered by the first two fixtures in the test." There is no test for this migration. The repo has the precedent (tests/unit/migration-0086, 0090, 0099), so the comment is fixable either by writing one or by deleting the sentence, but a comment asserting coverage that does not exist is worse than no comment.

• tests/unit/template-fail-closed.test.ts - the fourth case was deleted rather than strengthened, so nothing now asserts the repaired shape survives processActionConfig end to end. The old-shape case that remains asserts it still aborts, which is the bug, not the fix.

• tests/unit/template-fail-closed.test.ts:762 - not.toContain(" at ") runs against the whole message, so it is coupled to the unrelated trailing prose rather than to the path clause it means to test. Passing today, red on any reword.

• drizzle/0152_keep_2305_condition_group_to_condition_config.sql:47-81 - UPDATE ... FROM (subquery) under READ COMMITTED computes fixed.nodes from the pre-statement snapshot, so a workflow saved inside the statement's window is re-checked for the join qualifier and then overwritten with the already-computed value. About a second at 50k rows, so the window is short, but a save landing in it is discarded. Worth running against the real production row count before it ships rather than the 50k I measured.

With the team

• Whether save-time sanitisation should strip a top-level group on a Condition node. sanitizeNodes preserves unknown config keys and nothing on the import path drops it, so this migration is a one-shot repair and the same drift can recur from any producer that writes the old shape. I'm weighing a strip in sanitize-nodes.ts against leaving the shape permissive - the first makes the repair permanent and the second keeps save-time sanitisation from silently deleting caller data. I'm taking it to the core team and will come back. Nothing here is blocked on you.

Verdict

Changes requested - the migration can promote a seeded condition over a user's expression, and the guards for that case are in the resolver rather than in the statement.

[bongbongcrypto]

The blocking item is addressed, but not by the predicate as written, and I want to show why before anything else.

Blocking: the skip matches every row the migration targets

The instruction was to skip rows where config.condition is a non-empty string and conditionConfig is absent. Every row this migration exists to repair has exactly that shape. The builder before this PR:

config: {
  actionType: "Condition",
  condition: conditionConfig.condition,
  group: conditionConfig.group,
},

It always wrote condition and group together and never conditionConfig, and both fixtures that call it pass a non-empty condition: scripts/seed/fixtures/onboarding-workflows.ts:88 and :164, scripts/seed/fixtures/tempo-templates.ts:121 and :291. So with the skip as specified the statement is UPDATE 0.

The row you are protecting and the row the migration is for arrive as the same shape: {group: <seeded>, condition: <string>} with no conditionConfig. They differ only in whether condition equals visualConditionToExpression(group), and SQL cannot compute that without a second copy of the generator.

What the migration does now instead: on a Condition node carrying an object group, it drops the top-level key in every case, and promotes the group to conditionConfig only when there is no non-empty condition to outrank. Dropping is the repair. resolveConditionExpression has never read the top-level key, so removing it cannot change what a workflow evaluates, and it is what stops the rules array reaching the leftover-literal scan. Skipping the row would have left that abort in place. The editor rebuilds conditionConfig from condition the next time the node is opened, which the migration's own header already relied on for the other direction. A new test pins that every seeded expression parses back through expressionToConditionGroup, so a repaired seeded workflow opens in visual mode rather than expression mode.

The conditionConfig arms are unchanged where they still apply: an object with a group wins, an object without one is merged into, anything that is not an object is replaced rather than concatenated.

Mechanical

• The comment claiming coverage is gone, and the coverage exists: tests/unit/migration-0152-condition-group-to-condition-config.test.ts, in the shape of the 0086/0090/0099 drift tests. It pins the type guards, that every arm removes the stale key, that the expression arm drops rather than promotes, the replace-not-merge arm, ORDER BY ord, the EXISTS qualifier that makes a second run UPDATE 0, and that updated_at is not touched.
• tests/unit/template-fail-closed.test.ts: the end-to-end case is back. processActionConfig is a closure inside the executor, so the test walks its three steps with the same exported pieces it uses: blank condition and conditionConfig on a copy, processTemplates on the rest, assertResolved on the result. The repaired shape passes, the old shape throws with group.rules[0].leftOperand as the path. If you would rather this call the real function, I can lift those three steps out of the closure as an exported helper and have both the executor and the test use it; I did not want to widen the executor diff without asking.
• :762 now pins the spot the path clause would occupy, directly after the token ({{@step-1:Node.field}} at must be absent while the token is present), plus the ref's own path being undefined, instead of the absence of at anywhere in the message. My first version pinned token. and failed on the real message, which carries the (Reference left in rendered config ...) detail between them; that is the coupling you were pointing at, from the other side.
• While there: "records the path on the ref itself" passed vacuously when nothing threw. It now asserts the throw.
• The READ COMMITTED window is in the migration's header, with the suggestion to batch by id range if the production row count makes it long enough to matter.
• 0152 is still free on staging as of this push (a8f11d46); staging's newest is 0150, and 0151 came in through the earlier merge.

What I could not do

I do not have Postgres to hand this week, so unlike the last round I have not executed the statement against fixtures. The drift test reads the SQL; it does not run it. Your thirteen-fixture run is the check that matters and I would rather say so than imply otherwise.

What it cost

Nothing in the runtime or the builder changes from the revision you reviewed; node-builders.ts and template-resolution.ts are untouched this round. The migration promotes less than it did, and a seeded workflow nobody has opened since signup will have no conditionConfig until someone opens it, which is the state it is in today.