Skip to content

Commit c5a7448

Browse files
os-salesclaude
andauthored
fix(automation): create_record surfaces the engine's DUPLICATE_RECORD code (#14948)
* fix(automation): create_record surfaces the engine's DUPLICATE_RECORD code engine.insert (#14095) raises DuplicateRecordError with a classified `code: 'DUPLICATE_RECORD'` (ADR-0112), driver-independent. The create_record node executor threw it away, collapsing every failure into one opaque string, so a flow's only two error-handling primitives -- try_catch and a fault edge -- could not tell "already there" from "the store is down". - NodeExecutionResult gains an optional `code?: string`, beside the existing `errorClass`, set (via a duck-typed StandardErrorCode check, not an objectql-class import -- objectql is this package's devDependency only) when create_record's catch sees the classified code. - AutomationEngine copies it onto the `$error` run variable beside `message` when a node fails by returning. - try_catch's executor preserves it across its own errorVariable binding, which previously reconstructed `{ nodeId, message }` from the caught exception's message alone and silently dropped it. Deliberately scoped to create_record: update_record / delete_record collapse the same way, but engine.update still leaks the raw driver error (#14390, not yet fixed), so those node results have nothing structured to surface yet. Fixes #14419 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * chore: regenerate system-context-census after merging origin/main pre-push refused a stale merged artifact: the merge brought in an auth-plugin.ts line shift the census's anchor didn't follow. Regenerated via `pnpm gen:system-context-census` per AGENTS.md's merge-driver deferral protocol (this is the discharging commit right after the merge, not the merge itself). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * fix(automation): patch round 1 — identity guard for stale $error.code, test corrections, spec-gap deferral Tier contract review on PR #14948 returned PASS WITH REQUIRED PATCHES. Three patches, all addressed: P1 (the round's reason — a correctness defect the review reproduced): try_catch reads `code` off the run-wide $error, but the engine only rewrites $error when a failing node RETURNS a failure, or THROWS through a node with its own `fault` edge. A node inside a try_catch's try region never has a fault edge of its own (the region's synthetic sub-flow carries only the region's own edges), so a node that fails by THROWING (a timeoutMs firing, a dying nested container) used to leave $error exactly as an earlier, unrelated failure left it -- including its code. Fixed with a minimal identity guard in try-catch-node.ts: capture $error at the start of each attempt, trust the post-catch $error only if it actually changed. Two repro flows pinned (a loop sweeping a duplicate row then a timing-out row; a plain flow where an earlier fault-routed duplicate must not leak into a later unrelated try_catch) -- verified red on the pre-guard code, green after. P2: packages/spec's TryCatchErrorValueSchema doesn't declare `code` yet and strips it on a strict parse. packages/spec is single-owner (domain:spec); deferred and filed as #14954, named in the changeset. content/docs/automation/flows.mdx (hand-written) now mentions {$error.code}; content/docs/references/automation/control-flow.mdx (generated from spec) is untouched. P3: the "sets code: DUPLICATE_RECORD" regression pin never actually asserted `code`, and its comment about the step log carrying it was wrong. Both regression-pin tests now register a fault-edge handler that reads $error directly and assert `code` on it. Fold-in: NodeExecutionResult.code's JSDoc now states create_record narrows deliberately to DUPLICATE_RECORD; changeset notes a custom IDataEngine throwing { code: 'DUPLICATE_RECORD' } directly is correctly duck-typed as a duplicate too (ADR-0112). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9c7237c commit c5a7448

7 files changed

Lines changed: 547 additions & 10 deletions

File tree

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
---
2+
"@objectstack/service-automation": patch
3+
---
4+
5+
fix(automation): `create_record` now surfaces the engine's `DUPLICATE_RECORD` code, so a `try_catch` / `fault` edge can finally tell "already there" from "the store is down" (#14419)
6+
7+
`engine.insert` (#14095) already raises `DuplicateRecordError``code: 'DUPLICATE_RECORD'` (ADR-0112) — for a unique-constraint violation, driver-independent. The `create_record` node executor threw that away: every failure, from a duplicate key to a downed connection, collapsed into one opaque string (`create_record(<object>) failed: <message>`). A flow's only two error-handling primitives, `try_catch` and a `fault` edge, saw the same shape either way — the only expressible reading of "swallow the duplicate" was "swallow everything".
8+
9+
`NodeExecutionResult` gains an optional `code?: string` field, beside the existing `errorClass`, set when the caught error carries the platform's classified `DUPLICATE_RECORD` code. `AutomationEngine` now copies it onto the `$error` run variable alongside `message` (both the direct `fault`-edge path and the `try_catch` catch-region binding, which previously reconstructed `errorVariable` from the caught exception's message alone and silently dropped it), so a flow can actually branch on `{$error.code}`:
10+
11+
```
12+
try: create_record(lead, { email })
13+
catch: { $error.code === 'DUPLICATE_RECORD' } → swallow, continue
14+
else → re-raise / route the fault edge
15+
```
16+
17+
Additive only — no existing field, message text or routing behaviour changes; an executor that never sets `code` (every one except `create_record` today) is unaffected. Deliberately scoped to `create_record` alone: `update_record` / `delete_record` collapse the same way, but `engine.update` still leaks the raw driver error (#14390, not yet fixed), so those node results have nothing structured to surface yet. `create_record` itself forwards `code` only when it equals `DUPLICATE_RECORD` — narrowly, on purpose, matching the ADR-0112 vocabulary member this repair was actually scoped to surface, not any code an as-yet-unaudited driver error might someday carry.
18+
19+
**Patch round 1 (tier contract review):** `try_catch`'s catch region reads `code` off the run-wide `$error`, but the engine only rewrites `$error` when a failing node *returns* a failure, or *throws* through a node with its own `fault` edge — and a node inside a `try_catch`'s `try` region never has one (the region's synthetic sub-flow carries only the region's own edges). A node that fails by throwing (a `timeoutMs` firing, a dying nested container) therefore used to leave `$error` exactly as an *earlier, unrelated* failure left it — its `code` included. An identity guard (`$error` must have *changed*, not merely still be present, since this attempt started) closes that; two flows now pin it: a `loop` sweeping two rows where row 1 is a genuine duplicate and row 2 times out, and a plain flow where an earlier fault-routed duplicate must not leak into a later, unrelated `try_catch`.
20+
21+
A custom `IDataEngine` implementation whose thrown error already carries `code: 'DUPLICATE_RECORD'` (without being an instance of `@objectstack/objectql`'s `DuplicateRecordError`) is treated as a duplicate too — correct under ADR-0112, since `code` is the classified envelope's public contract, not the concrete class.
22+
23+
**Known gap, filed rather than fixed here (out of this lane's scope):** `packages/spec`'s `TryCatchErrorValueSchema` — the ONE declared shape for the `errorVariable` binding shared by author, engine and run log — does not declare `code` yet, and strips it on a strict parse. `packages/spec` is single-owner (`domain:spec`); tracked as #14954.
24+
25+
<!-- adr-0087: not-required (no-migration-prescription) additive optional field; no authorable key, export removal or rename for an upgrader to migrate -->

content/docs/automation/flows.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1080,7 +1080,7 @@ The handler reads what went wrong from two variables:
10801080
| Variable | Scope | Use |
10811081
| :--- | :--- | :--- |
10821082
| `{<nodeId>.error}` | the failing node | `{charge_card.error}` — addressable by name, so one handler shared by several fault edges can tell which node it is handling |
1083-
| `{$error}` | run-wide | `{$error.nodeId}` / `{$error.message}` — the most recent failure only |
1083+
| `{$error}` | run-wide | `{$error.nodeId}` / `{$error.message}` — the most recent failure only. `{$error.code}` is also present when the failing node's own result carried a platform-classified error code (e.g. `create_record`'s `DUPLICATE_RECORD`, ADR-0112) — absent otherwise, so a handler branching on it should treat "unset" as "no classified code", not as "nothing failed" |
10841084

10851085
A run that takes a fault branch and completes reports **success**, but the
10861086
failed step stays in the run trace with `status: 'failure'` and its message —

content/docs/permissions/system-context.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ The largest single consumer — **17 of the 106 sites**.
168168
| 57 | Automation run-state read, flow-authoring write and unrelated-screen read all pass | runtime | Get: run state, flow writes and screen reads with no grant | `domains/automation.ts:254`, `:545`, `:635` |
169169
| 58 | Audience-binding suggestion recording skipped | plugin-security | Lose: install-time suggestions are not recorded for system callers | `suggested-audience-bindings.ts:703` |
170170
| 59 | Email-template / webhook provenance stamps skipped | plugin-email, plugin-webhooks | Lose: the row is not marked as an admin customization | `email-template-provenance.ts:59`, `webhook-provenance.ts:50` |
171-
| 60 | **Automation flow data nodes re-add the `owner_id` stamp** (the one place row 2's gap is compensated inline) | service-automation | Get: a flow-authored INSERT under system elevation still lands owned, when the run resolved a user. Fill-only — flow-authored values win | `runtime-identity.ts:279`, called from `builtin/crud-nodes.ts:318` |
171+
| 60 | **Automation flow data nodes re-add the `owner_id` stamp** (the one place row 2's gap is compensated inline) | service-automation | Get: a flow-authored INSERT under system elevation still lands owned, when the run resolved a user. Fill-only — flow-authored values win | `runtime-identity.ts:279`, called from `builtin/crud-nodes.ts:319` |
172172
| 61 | Inbox caller refusal names `isSystem` as what was carried | service-messaging | Get: nothing — the refusal still fires. The flag only shapes the diagnostic, because privilege is not an authorization subject | `inbox-caller.ts:148` |
173173

174174
### 6. Reads that only carry the flag onward

0 commit comments

Comments
 (0)