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.