diff --git a/src/email-templates/assignment.email-template.ts b/src/email-templates/assignment.email-template.ts new file mode 100644 index 0000000..24842fb --- /dev/null +++ b/src/email-templates/assignment.email-template.ts @@ -0,0 +1,109 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { defineEmailTemplateDefinition } from '@objectstack/spec'; + +/** + * The assigner-facing text of the fan-out failure handler + * (`src/flows/assignment.flow.ts`, the `try_catch` catch region), as a + * `sys_email_template` bundle. + * + * ── Why this bundle exists at all ──────────────────────────────────────── + * A fan-out that quietly drops one of five people is the failure #123 is + * about, and "the run log records it" is not an answer: the run log is an + * operator surface and the assigner never opens it. This is the sentence the + * assigner actually reads, in their inbox, once per assignee who got no task. + * + * ── Why the words are here and not on the notify node ──────────────────── + * The same reason as `reminders.email-template.ts`: `NotifyConfigSchema` + * makes inline `title`/`message` and `template` mutually exclusive, and the + * inline path is the one its own `.describe()` calls "not localizable". + * AGENTS.md §8 ("English is the source language … do not hard-code display + * text in a hook or flow") is only satisfiable on the template path. Two rows + * — `en` and `zh-CN` — which is exactly the pair `objectstack.config.ts` + * declares in `i18n.supportedLocales`. + * + * ── `{{{…}}}` in `subject` / `bodyText`, `{{…}}` in `bodyHtml` ─────────── + * Measured and pinned by `test/email-templates.test.ts`: `renderTemplate` + * HTML-escapes a `{{hole}}` and leaves a `{{{hole}}}` raw. The inbox channel + * writes the rendered subject into `sys_inbox_message.title` and the rendered + * TEXT into `body_md` — neither is an HTML document — so an assignment whose + * subject carries an apostrophe would otherwise put `'` on the assigner's + * screen. `bodyHtml` IS markup and keeps the escaping form. + * + * ── Why only `subject` is a REQUIRED variable ──────────────────────────── + * `required: true` is enforced at render (`requireVars` → `MISSING_VARIABLES`, + * which the inbox channel classifies as PERMANENT — a dead delivery, not a + * retry). So it is declared only where the value is guaranteed: + * `duly_assignment.subject` is `required: true` on the object. + * + * `assignee` is deliberately NOT required, and that is the whole point of this + * notification rather than an oversight. The commonest bad row is an assignee + * entry that is itself blank — the "missing owner" shape — and a hole declared + * required would then dead-letter the one message whose job is to report it. + * The reason line carries the diagnosis in that case, and the click-through + * lands on the assignment where the assignee list can be read directly. + */ + +/** Declared render inputs. One place, so the two rows cannot drift. */ +const FANOUT_FAILURE_VARIABLES = [ + { + name: 'subject', + type: 'string' as const, + required: true, + description: "The assignment's subject — duly_assignment.subject, required on the object.", + }, + { + name: 'assignee', + type: 'string' as const, + required: false, + description: + 'The assignee handle the fan-out was iterating when it failed. Blank when the ' + + 'assignee entry itself is the defect, which is why this is not required.', + }, + { + name: 'reason', + type: 'string' as const, + required: false, + description: "The engine's own failure sentence, naming the flow node that failed.", + }, +]; + +export const AssignmentFanoutFailedEn = defineEmailTemplateDefinition({ + name: 'duly.assignment_fanout_failed', + label: 'Assignment fan-out could not reach one assignee', + category: 'notification', + locale: 'en', + subject: 'No task was created for one assignee: {{{subject}}}', + bodyHtml: + '

Everyone else on this assignment has their task. This one did not get created,' + + ' so nobody is holding it.

' + + '

Assignee: {{assignee}}
Reason: {{reason}}

', + bodyText: + 'Everyone else on this assignment has their task. This one did not get created, so' + + ' nobody is holding it.\nAssignee: {{{assignee}}}\nReason: {{{reason}}}', + variables: FANOUT_FAILURE_VARIABLES, + description: + 'Sent to the assigner, once per assignee the fan-out could not create a task for. ' + + 'The rest of the fan-out completed.', +}); + +export const AssignmentFanoutFailedZhCN = defineEmailTemplateDefinition({ + name: 'duly.assignment_fanout_failed', + label: '指派分发未能覆盖某位成员', + category: 'notification', + locale: 'zh-CN', + subject: '有一位成员没有生成任务:{{{subject}}}', + bodyHtml: + '

这项指派中其他人的任务都已生成,只有这一条没有创建成功,因此目前无人承担。

' + + '

成员:{{assignee}}
原因:{{reason}}

', + bodyText: + '这项指派中其他人的任务都已生成,只有这一条没有创建成功,因此目前无人承担。\n' + + '成员:{{{assignee}}}\n原因:{{{reason}}}', + variables: FANOUT_FAILURE_VARIABLES, + description: '每有一位成员未能生成任务,就向指派人发送一次;分发的其余部分已正常完成。', +}); + +export const dulyAssignmentEmailTemplates = [ + AssignmentFanoutFailedEn, + AssignmentFanoutFailedZhCN, +]; diff --git a/src/email-templates/index.ts b/src/email-templates/index.ts index 77c758a..832e339 100644 --- a/src/email-templates/index.ts +++ b/src/email-templates/index.ts @@ -20,6 +20,11 @@ // here twice — once as `en`, once as `zh-CN`. Adding a locale is adding an // entry to this array, never editing an existing one. +import { + AssignmentFanoutFailedEn, + AssignmentFanoutFailedZhCN, + dulyAssignmentEmailTemplates, +} from './assignment.email-template.js'; import { TaskDueSoonReminderEn, TaskDueSoonReminderZhCN, @@ -38,6 +43,12 @@ export { TaskOverdueEscalationEn, TaskOverdueEscalationZhCN, dulyReminderEmailTemplates, + AssignmentFanoutFailedEn, + AssignmentFanoutFailedZhCN, + dulyAssignmentEmailTemplates, }; -export const dulyEmailTemplates = [...dulyReminderEmailTemplates]; +export const dulyEmailTemplates = [ + ...dulyReminderEmailTemplates, + ...dulyAssignmentEmailTemplates, +]; diff --git a/src/flows/assignment.flow.ts b/src/flows/assignment.flow.ts index a68c96f..e8d4ae7 100644 --- a/src/flows/assignment.flow.ts +++ b/src/flows/assignment.flow.ts @@ -67,6 +67,52 @@ import { defineFlow } from '@objectstack/spec'; * so it is a decision and not an accident: an assigner who is also one of the * assignees already owns a task on this assignment, so `needs_collection` adds * no second one for them. + * + * ── One bad assignee must not cost the other four (#123) ───────────────── + * The loop body is wrapped in a `try_catch` container, and that wrapper is + * load-bearing rather than defensive dressing. `loop-node.ts` iterates with a + * bare `await engine.runRegion(...)` and has no try/catch of its own, so a + * body node that fails throws straight out of the CONTAINER: the first bad + * item ends the whole run, every later assignee is never processed, and the + * loop never returns its `childSteps` — so the rows it DID write are not even + * counted. Measured on the real engine before this wrapper, with a three-name + * assignment whose middle row was bad: + * + * status: failed · acted: 0 · duly_task rows actually written: 1 + * + * Two people were silently dropped and the one task that was created was + * reported as nothing at all. That is worse than either failure on its own, + * which is why "abort the fan-out" is not an acceptable reading of a bad row. + * + * **What the assignment shows afterwards**, decided here so it is a contract + * and not an accident: + * + * 1. **The count is the truth.** `duly_assignment.task_count` is a + * `Field.summary` count over the children, so it reports the tasks that + * actually exist — two, on a three-name assignment with one bad row. The + * run summary now agrees with it (`acted: 2`), because a loop that + * completes returns the `childSteps` an aborted one threw away. + * 2. **The failure is named to the assigner**, in their inbox, one + * notification per failed assignee, carrying the assignee handle, the + * engine's own reason, and a click-through to the assignment. A partial + * fan-out that nobody is told about is the "silent partial success" this + * card exists to remove; the run log alone does not count, because it is + * an operator surface and the assigner never sees it. + * + * The flow still writes NOTHING back onto `duly_assignment` — not a note, not + * a status. It cannot: the start trigger is `record-after-write`, so a write + * back onto the trigger record re-enters this same flow on a row whose status + * is still `dispatched`, and the notification would then be re-sent on every + * re-entry. The inbox is the assigner-visible surface that costs no such loop. + * + * **The handler reads `{fanout_assignee}` and nothing else about the person.** + * The iterator variable is re-bound by the loop before every iteration, so it + * is the one value guaranteed to describe THIS item. `fanout_assignee_user` is + * not: a region that throws at `fanout_find_unit` leaves it holding the + * PREVIOUS iteration's row, and a handler that read a name from it would + * calmly name the wrong colleague. Where the assignee handle itself is the + * defect (a blank entry in `assignees` — the "missing owner" shape), the + * handle renders empty and the engine's reason carries the diagnosis. */ export const AssignmentFanout = defineFlow({ name: 'duly_assignment_fanout', @@ -131,71 +177,129 @@ export const AssignmentFanout = defineFlow({ // of `sys_user` ids. collection: '{record.assignees}', iteratorVariable: 'fanout_assignee', + // The body is ONE node: the try_catch container. Everything that can + // fail for one person lives inside its `try`, so the blast radius of a + // bad row is that row (see the header). A region is single-entry / + // single-exit by construction, which a one-node body trivially is. body: { nodes: [ { - id: 'fanout_find_existing', - type: 'get_record', - label: 'Does this assignee already have a task?', + id: 'fanout_attempt', + type: 'try_catch', + label: 'One assignee, isolated from the rest', config: { - objectName: 'duly_task', - // Per OWNER, not per assignment. `limit` omitted → findOne → - // the variable is set to the row or to null, never to []. - filter: { assignment: '{record.id}', owner: '{fanout_assignee}' }, - fields: ['id'], - outputVariable: 'existing_task', - }, - }, - { - id: 'fanout_find_unit', - type: 'get_record', - label: "Read the assignee's business unit", - config: { - objectName: 'sys_user', - filter: { id: '{fanout_assignee}' }, - fields: ['id', 'primary_business_unit_id'], - outputVariable: 'fanout_assignee_user', - }, - }, - { - id: 'fanout_create_task', - type: 'create_record', - label: 'Create the assignee task', - config: { - objectName: 'duly_task', - fields: { - subject: '{record.subject}', - owner: '{fanout_assignee}', - // Denormalised at dispatch so a later transfer does not - // rewrite history (see duly_task.business_unit). - business_unit: '{fanout_assignee_user.primary_business_unit_id}', - assignment: '{record.id}', - source: 'assigned', - due_date: '{record.due_date}', - // An assignment has no lead time to spread, so the task is - // visible from the day it is due. - visible_from: '{record.due_date}', - status: 'open', - // `period_key` is NOT written. An assignment has no period, - // and the dispatch identity index does not apply to it. + // `errorVariable` is left at its declared default `$error`, + // which is also the name `executeNode` binds a node failure + // to — one spelling, and no second key to keep in step. + try: { + nodes: [ + { + id: 'fanout_find_existing', + type: 'get_record', + label: 'Does this assignee already have a task?', + config: { + objectName: 'duly_task', + // Per OWNER, not per assignment. `limit` omitted → + // findOne → the variable is set to the row or to null, + // never to []. + filter: { assignment: '{record.id}', owner: '{fanout_assignee}' }, + fields: ['id'], + outputVariable: 'existing_task', + }, + }, + { + id: 'fanout_find_unit', + type: 'get_record', + label: "Read the assignee's business unit", + config: { + objectName: 'sys_user', + filter: { id: '{fanout_assignee}' }, + fields: ['id', 'primary_business_unit_id'], + outputVariable: 'fanout_assignee_user', + }, + }, + { + id: 'fanout_create_task', + type: 'create_record', + label: 'Create the assignee task', + config: { + objectName: 'duly_task', + fields: { + subject: '{record.subject}', + owner: '{fanout_assignee}', + // Denormalised at dispatch so a later transfer does + // not rewrite history (see duly_task.business_unit). + business_unit: '{fanout_assignee_user.primary_business_unit_id}', + assignment: '{record.id}', + source: 'assigned', + due_date: '{record.due_date}', + // An assignment has no lead time to spread, so the + // task is visible from the day it is due. + visible_from: '{record.due_date}', + status: 'open', + // `period_key` is NOT written. An assignment has no + // period, and the dispatch identity index does not + // apply to it. + }, + }, + }, + ], + edges: [ + { + id: 'fanout_e_missing', + source: 'fanout_find_existing', + target: 'fanout_find_unit', + type: 'conditional', + label: 'No task yet', + // `isBlank` takes the value itself (`dyn`), so it is + // total over null/undefined/'' /[] — unlike a field + // access through a null root, which aborts the predicate. + condition: P`isBlank(vars.existing_task)`, + }, + { id: 'fanout_e_create', source: 'fanout_find_unit', target: 'fanout_create_task' }, + ], + }, + // The handler is what turns "this person got nothing" from a + // dropped row into something the assigner is told. It must not + // be able to fail the container itself: `notify` degrades to a + // no-op success when no messaging service is mounted, and a + // catch region that threw would put the abort straight back. + catch: { + nodes: [ + { + id: 'fanout_report_failure', + type: 'notify', + label: 'Tell the assigner this person got no task', + config: { + recipients: '{record.assigner}', + // The localizable content path (AGENTS.md §8) — the + // words live in src/email-templates/, never inline. + template: 'duly.assignment_fanout_failed', + templateData: { + subject: '{record.subject}', + // The iterator, not `fanout_assignee_user`: see the + // header on why a stale row would name the wrong + // colleague. + assignee: '{fanout_assignee}', + // `$error.message` is the engine's own sentence and + // it names the node that failed, e.g. "Node + // 'fanout_create_task' failed: create_record + // (duly_task) failed: Owner is required". + reason: '{$error.message}', + }, + severity: 'warning', + topic: 'duly.assignment_fanout_failed', + sourceObject: 'duly_assignment', + sourceId: '{record.id}', + }, + }, + ], + edges: [], }, }, }, ], - edges: [ - { - id: 'fanout_e_missing', - source: 'fanout_find_existing', - target: 'fanout_find_unit', - type: 'conditional', - label: 'No task yet', - // `isBlank` takes the value itself (`dyn`), so it is total over - // null/undefined/'' /[] — unlike a field access through a null - // root, which aborts the predicate. - condition: P`isBlank(vars.existing_task)`, - }, - { id: 'fanout_e_create', source: 'fanout_find_unit', target: 'fanout_create_task' }, - ], + edges: [], }, }, }, diff --git a/src/translations/authored-text.ts b/src/translations/authored-text.ts index f91a760..9119663 100644 --- a/src/translations/authored-text.ts +++ b/src/translations/authored-text.ts @@ -653,6 +653,15 @@ const RECORD_MAPS: ReadonlySet = new Set(['object.fields']); */ const REENTER: Readonly> = { 'flow.nodes[].config.body': 'flow', + // ADR-0031 gives a `try_catch` container two regions of the same shape as a + // `loop` body, so they re-enter for the same reason (#123). Listed only for + // the slots this app actually authors: a `parallel` block's + // `config.branches[]` is NOT here, because a branch is `{name, nodes, edges}` + // and its `name` would land on `flow.name` — a verdict that reads "flow + // name" and would be wrong about it. When a parallel block is first + // authored here, it needs its own verdict for that key, not this shortcut. + 'flow.nodes[].config.try': 'flow', + 'flow.nodes[].config.catch': 'flow', }; /** Two words of three-plus letters — the shape machine values do not have. */ diff --git a/test/assignment-fanout.test.ts b/test/assignment-fanout.test.ts index 89f28e9..e79686b 100644 --- a/test/assignment-fanout.test.ts +++ b/test/assignment-fanout.test.ts @@ -1,7 +1,14 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. -import { describe, expect, it } from 'vitest'; - +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { AppPlugin, ObjectKernel, createStandaloneStack } from '@objectstack/runtime'; +import { PlatformObjectsPlugin } from '@objectstack/platform-objects'; +import { AutomationServicePlugin } from '@objectstack/service-automation'; +import { JobServicePlugin } from '@objectstack/service-job'; +import { MessagingServicePlugin } from '@objectstack/service-messaging'; +import { EmailServicePlugin } from '@objectstack/plugin-email'; + +import stack from '../objectstack.config.js'; import { AssignmentFanout } from '../src/flows/assignment.flow.js'; import { dulyFlows } from '../src/flows/index.js'; import { Assignment, Task } from '../src/objects/index.js'; @@ -60,12 +67,55 @@ const startNode = (): NodeLike => { if (!found) throw new Error('flow has no start node'); return found; }; +interface RegionLike { nodes?: NodeLike[]; edges?: EdgeLike[] } + +/** The ADR-0031 container slots this flow uses, in the order they nest. */ +const region = (n: NodeLike, slot: 'body' | 'try' | 'catch'): RegionLike => + ((n.config?.[slot] ?? {}) as RegionLike); + +/** The loop's outermost body region — one node, the `try_catch` container. */ +const loopRegion = (): RegionLike => region(node('fan_out'), 'body'); + +/** The `try_catch` container the loop body is made of. */ +const attempt = (): NodeLike => { + const found = (loopRegion().nodes ?? []).find((n) => n.type === 'try_catch'); + if (!found) throw new Error('the loop body holds no try_catch container'); + return found; +}; + +/** + * The PROTECTED region — what used to be the loop body directly. + * + * Every assertion below that reads "the loop body" means this: the three nodes + * that do one assignee's work. They moved one level down when #123 wrapped them + * in a `try_catch`, and reading them through this helper is what keeps those + * assertions about the same three nodes instead of quietly finding nothing. + */ const loopBody = (): { nodes: NodeLike[]; edges: EdgeLike[] } => { - const body = (node('fan_out').config?.body ?? {}) as { nodes?: NodeLike[]; edges?: EdgeLike[] }; + const body = region(attempt(), 'try'); return { nodes: body.nodes ?? [], edges: body.edges ?? [] }; }; -/** Every node in the flow, region bodies included. */ -const allNodes = (): NodeLike[] => [...nodes, ...loopBody().nodes]; + +/** The handler region — what runs for an assignee whose work threw. */ +const catchBody = (): { nodes: NodeLike[]; edges: EdgeLike[] } => { + const body = region(attempt(), 'catch'); + return { nodes: body.nodes ?? [], edges: body.edges ?? [] }; +}; + +/** Every node in the flow, every container region included. */ +const allNodes = (): NodeLike[] => [ + ...nodes, + ...(loopRegion().nodes ?? []), + ...loopBody().nodes, + ...catchBody().nodes, +]; +/** Every edge in the flow, every container region included. */ +const allEdges = (): EdgeLike[] => [ + ...edges, + ...(loopRegion().edges ?? []), + ...loopBody().edges, + ...catchBody().edges, +]; /** Every predicate authored anywhere, as its bare CEL source. */ const allPredicates = (): { where: string; source: string }[] => { const out: { where: string; source: string }[] = []; @@ -75,7 +125,7 @@ const allPredicates = (): { where: string; source: string }[] => { if (source !== '') out.push({ where, source }); }; for (const n of allNodes()) read(`node '${n.id}' condition`, n.config?.condition as EdgeLike['condition']); - for (const e of [...edges, ...loopBody().edges]) read(`edge '${e.id}'`, e.condition); + for (const e of allEdges()) read(`edge '${e.id}'`, e.condition); return out; }; @@ -341,3 +391,272 @@ describe('assignment fan-out — invariants a refactor would undo', () => { } }); }); + +// ───────────────────────────────────────────────────────────────────────── +// One bad assignee, isolated — the structure (#123) +// ───────────────────────────────────────────────────────────────────────── + +describe('assignment fan-out — a bad row costs one task, not the fan-out', () => { + it('the loop body is a try_catch container and nothing else', () => { + // `loop-node.ts` iterates with a bare `await runRegion(...)` and holds no + // try/catch of its own, so anything that can throw must be INSIDE one. A + // second node beside the container would be exactly that unprotected gap. + const body = loopRegion(); + expect((body.nodes ?? []).map((n) => n.type)).toEqual(['try_catch']); + expect(body.edges ?? []).toEqual([]); + }); + + it('every node that can fail sits inside the protected region', () => { + // The three data nodes are the ones that return `success: false` or throw; + // a `get_record`/`create_record` left outside the `try` would abort the + // whole loop exactly as before the wrapper existed. + expect(loopBody().nodes.map((n) => n.id)).toEqual([ + 'fanout_find_existing', 'fanout_find_unit', 'fanout_create_task', + ]); + for (const n of loopBody().nodes) { + expect(['get_record', 'create_record'], `${n.id}`).toContain(n.type); + } + }); + + it('the handler tells the ASSIGNER, and names the assignee from the iterator', () => { + const handler = catchBody().nodes; + expect(handler.map((n) => n.type)).toEqual(['notify']); + const config = (handler[0].config ?? {}) as AnyRec; + expect(config.recipients).toBe('{record.assigner}'); + const templateData = (config.templateData ?? {}) as AnyRec; + // `fanout_assignee` is re-bound by the loop before every iteration. + // `fanout_assignee_user` is NOT: a region that throws at `fanout_find_unit` + // leaves it holding the PREVIOUS assignee's row, so a handler reading a + // name from it would calmly name the wrong colleague. + expect(templateData.assignee).toBe('{fanout_assignee}'); + expect(JSON.stringify(templateData)).not.toContain('fanout_assignee_user'); + }); + + it('the handler says something, in a bundle, never inline', () => { + // AGENTS.md §8. `NotifyConfigSchema` refuses `template` beside inline + // `title`/`message`, so the check that matters is that a template is named + // at all — a handler with neither would not parse, and one with inline copy + // would ship English into a zh-CN deployment. + const config = (catchBody().nodes[0].config ?? {}) as AnyRec; + expect(config.template).toBe('duly.assignment_fanout_failed'); + expect(Object.keys(config)).not.toContain('title'); + expect(Object.keys(config)).not.toContain('message'); + }); + + it('the handler still writes nothing back onto the assignment', () => { + // A write to the trigger record re-enters this same flow (the start node is + // `record-after-write` on a row still `dispatched`), so the notification + // would be re-sent on every re-entry. The inbox costs no such loop. + for (const n of catchBody().nodes) { + expect(['create_record', 'update_record', 'delete_record'], `${n.id}`).not.toContain(n.type); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────── +// The same claim, against a REAL booted engine (#123) +// ───────────────────────────────────────────────────────────────────────── + +/** + * Everything above is structure. None of it can answer the question the card + * is actually about — *what does the engine do with a bad row* — because the + * abort lived in `loop-node.ts`, not in this flow's shape. + * + * ── Why the run is started by `automation.execute` and not by an insert ── + * This repo installs no `record_change` trigger. The triggers ship as separate + * packages (`@objectstack/trigger-*`) and only `trigger-schedule` is a + * devDependency here, so a `duly_assignment` insert fires nothing and the boot + * says so out loud: + * + * WARN flow 'duly_assignment_fanout' declares a 'record_change' trigger but + * is NOT bound — it will never auto-launch. + * + * `execute(flowName, context)` is not a test-only side door around that: it is + * the *same* door the trigger uses. `AutomationEngine.registerFlow` arms every + * trigger with `trigger.start(binding, (ctx) => this.execute(flowName, ctx))`, + * so a record-change fire and the call below differ only in who assembles the + * context. WHICH context the trigger would assemble is pinned separately, by + * the start-node assertions at the top of this file. + */ +describe('assignment fan-out — one bad row, driven through the real engine', () => { + let kernel: any; + let data: any; + /** The two assignees whose rows are fine, and the manager who assigned. */ + let alice: string; + let carol: string; + let boss: string; + let assignmentId: string; + let result: AnyRec; + let tasks: AnyRec[]; + let assignmentAfter: AnyRec; + let inbox: AnyRec[]; + let notifications: AnyRec[]; + + const SUBJECT = 'Quarterly control walkthrough'; + + /** Per-node rollup from the run summary, by node id. */ + const nodeSummary = (id: string): AnyRec => { + const nodesOut = ((result.summary as AnyRec)?.nodes ?? []) as AnyRec[]; + const found = nodesOut.find((n) => n.nodeId === id); + if (!found) throw new Error(`run summary has no node '${id}'`); + return found; + }; + + beforeAll(async () => { + const { plugins } = await createStandaloneStack({ + // Memory, not sqlite: nothing here rests on a unique index (a fan-out + // task writes neither `duty` nor `period_key`, so the dispatch identity + // index cannot constrain these rows — see the idempotency block above). + // What this suite DOES need is `sys_user`, which the bare standalone + // stack does not declare — hence PlatformObjectsPlugin below. Measured: + // on `databaseDriver: 'sqlite'` the `sys_user` read inside the loop is + // refused by the driver, and every iteration fails for a reason that has + // nothing to do with this card. + databaseDriver: 'memory', + skipSeedData: true, + // Same reason as every other suite here: left to its default this + // resolves `/dist/objectstack.json`, and a local `pnpm build` would + // make the run report on the last BUILD rather than on `src/`. + artifactPath: 'dist/objectstack.this-suite-must-not-load-an-artifact.json', + }); + kernel = new ObjectKernel(); + for (const plugin of plugins) await kernel.use(plugin); + await kernel.use(new PlatformObjectsPlugin()); + await kernel.use(new AppPlugin(stack as any, undefined, { skipSeedData: true })); + await kernel.use(new JobServicePlugin()); + await kernel.use(new AutomationServicePlugin()); + // The handler's notification only reaches an inbox row if the delivery + // path is real, and "a notification was emitted" is not the claim being + // tested — "the assigner can read what went wrong" is. + await kernel.use(new MessagingServicePlugin()); + await kernel.use(new EmailServicePlugin()); + await kernel.bootstrap(); + data = kernel.getService('data'); + const automation = kernel.getService('automation') as { + execute(flow: string, ctx: AnyRec): Promise; + }; + + const mkUser = async (name: string): Promise => { + const created = await data.insert( + 'sys_user', + { name, username: name, email: `${name}@example.test` }, + { context: { isSystem: true } }, + ); + const row = (Array.isArray(created) ? created[0] : created) as AnyRec; + return String(row.id); + }; + alice = await mkUser('fanout_alice'); + carol = await mkUser('fanout_carol'); + boss = await mkUser('fanout_boss'); + + const created = await data.insert('duly_assignment', { + subject: SUBJECT, + assigner: boss, + // Three people, and the middle row is bad — a blank entry, the "missing + // owner" shape from the card. The position is the point: `carol` comes + // AFTER the failure, so a task of hers is proof the loop kept going + // rather than proof it never had to. + assignees: [alice, '', carol], + due_date: '2026-10-01', + status: 'dispatched', + needs_collection: false, + }); + const assignment = (Array.isArray(created) ? created[0] : created) as AnyRec; + assignmentId = String(assignment.id); + + result = await automation.execute('duly_assignment_fanout', { + record: assignment, + object: 'duly_assignment', + event: 'afterInsert', + }); + + tasks = (await data.find('duly_task', { where: { assignment: assignmentId } })) as AnyRec[]; + assignmentAfter = ((await data.find('duly_assignment', { where: { id: assignmentId } })) as AnyRec[])[0]; + // The messaging service hands the inbox channel the delivery and returns; + // the row lands a moment later, so poll rather than sleep a fixed guess. + const deadline = Date.now() + 10_000; + for (;;) { + inbox = (await data.find('sys_inbox_message', { where: { user_id: boss } })) as AnyRec[]; + if (inbox.length > 0 || Date.now() > deadline) break; + await new Promise((r) => setTimeout(r, 100)); + } + notifications = (await data.find('sys_notification', {})) as AnyRec[]; + }, 180_000); + + afterAll(async () => { + await kernel?.shutdown?.(); + }); + + it('creates the other two tasks — including the assignee AFTER the bad row', () => { + // The regression, stated as the product outcome: two of the three people + // are holding their work. Without the try_catch this is ONE task (the loop + // threw on the middle item and iteration three never ran). + expect(tasks.map((t) => String(t.owner)).sort()).toEqual([alice, carol].sort()); + expect(tasks.every((t) => t.subject === SUBJECT)).toBe(true); + expect(tasks.every((t) => t.status === 'open' && t.source === 'assigned')).toBe(true); + }); + + it('the count the assigner reads is the tasks that exist — two', () => { + // `task_count` is a `Field.summary` over the children, so it cannot drift + // from the rows; this asserts the rows are what a partial fan-out should + // leave behind, not three and not zero. + expect(assignmentAfter.task_count).toBe(2); + }); + + it('the run finishes and REPORTS the two it wrote, instead of acted: 0', () => { + // The measured symptom in the card: an aborted loop never returns its + // `childSteps`, so the two rows it had already written were invisible to + // the run summary and it reported `acted: 0` next to `status: failed`. + expect(result.success, String(result.error ?? '')).toBe(true); + expect((result.summary as AnyRec).acted).toBe(2); + }); + + it('records the failure rather than swallowing it', () => { + // "Recovered" must not read as "clean". The container reports success — + // that is the point of catching — and underneath it the failing node still + // carries its own failure, once, at the iteration it happened in. + expect(nodeSummary('fanout_attempt')).toMatchObject({ + nodeType: 'try_catch', runs: 3, failures: 0, + }); + expect(nodeSummary('fanout_create_task')).toMatchObject({ + nodeType: 'create_record', runs: 3, failures: 1, acted: 2, + }); + }); + + it('tells the assigner, in their inbox, which row failed and why', () => { + // Not "a notification was emitted": a template that resolves to nothing + // emits one too. The assertion is on the words the assigner reads. + expect(inbox.length, 'the assigner got no notification at all').toBe(1); + const message = inbox[0]; + expect(message.topic).toBe('duly.assignment_fanout_failed'); + expect(message.severity).toBe('warning'); + // The subject rides in unescaped (`{{{subject}}}`) — the inbox title is not + // an HTML document. + expect(message.title).toBe(`No task was created for one assignee: ${SUBJECT}`); + // The engine's own sentence, naming the node and the real cause. + expect(String(message.body_md)).toContain('Owner is required'); + expect(String(message.body_md)).toContain('fanout_create_task'); + // And a way back to the assignment, so the assigner can fix the row. + expect(String(message.action_url)).toContain(assignmentId); + }); + + it('one notification per failed assignee — not one per fan-out', () => { + // Three assignees, one bad row, one message. A handler that fired per RUN + // could not name the person; one that fired per iteration would tell the + // assigner twice about the people who are fine. + expect(notifications.length).toBe(1); + }); + + it('the handler read the ITERATOR, not the previous assignee it looked up', () => { + // `fanout_assignee_user` still holds ALICE's row when the middle iteration + // fails — the loop shares one variable scope across iterations. A handler + // that named the person from it would have reported Alice, who is fine. + // The blank handle below is the bad row's own value, and its blankness is + // the evidence: a stale read could not have produced it. + const payload = (notifications[0].payload ?? {}) as AnyRec; + const templateData = (payload.templateData ?? {}) as AnyRec; + expect(templateData.assignee).toBe(''); + expect(templateData.subject).toBe(SUBJECT); + expect(String(templateData.reason)).toContain('Owner is required'); + }); +}); diff --git a/test/email-templates.test.ts b/test/email-templates.test.ts index cdd71c7..32eec87 100644 --- a/test/email-templates.test.ts +++ b/test/email-templates.test.ts @@ -46,9 +46,37 @@ type AnyRec = Record; interface NodeLike { id: string; type: string; config?: AnyRec } interface FlowLike { name: string; nodes: NodeLike[] } +/** + * Every node in a flow, INCLUDING the ones inside ADR-0031 container regions + * (`loop.config.body`, `try_catch.config.try` / `.catch`). + * + * The recursion is not decoration. `duly_assignment_fanout`'s failure handler + * is a `notify` node inside a `try_catch` catch region (#123), and a walk that + * only read `flow.nodes` skipped it — so the "names a template that EXISTS" + * assertion below silently stopped covering the one notify node whose template + * was newest. A miss there dead-letters the delivery permanently while the run + * still reports success, which is precisely the failure this file exists for. + * `FLOW_REGION_CONFIG_KEYS` is the platform's own list of these slots; it is + * spelled out here rather than imported to keep this file's walk readable + * beside `test/flow-predicates.test.ts`, which uses the spec helper. + */ +const REGION_SLOTS = ['body', 'try', 'catch'] as const; + +const allNodesOf = (nodes: NodeLike[] | undefined): NodeLike[] => { + const out: NodeLike[] = []; + for (const node of nodes ?? []) { + out.push(node); + for (const slot of REGION_SLOTS) { + const region = node.config?.[slot] as { nodes?: NodeLike[] } | undefined; + if (region?.nodes) out.push(...allNodesOf(region.nodes)); + } + } + return out; +}; + const notifyNodes = (): { flow: string; node: NodeLike }[] => (dulyFlows as unknown as FlowLike[]).flatMap((f) => - f.nodes.filter((n) => n.type === 'notify').map((node) => ({ flow: f.name, node })), + allNodesOf(f.nodes).filter((n) => n.type === 'notify').map((node) => ({ flow: f.name, node })), ); /** Every `(name, locale)` row in the barrel, keyed by name. */ @@ -123,7 +151,9 @@ describe('the email-template barrel', () => { const key = (r: AnyRec) => `${String(r.name)}@${String(r.locale)}`; const onStack = ((stack as AnyRec).emailTemplates ?? []) as AnyRec[]; expect(onStack.map(key)).toEqual((dulyEmailTemplates as unknown as AnyRec[]).map(key)); - expect(onStack.length).toBe(6); + // 6 before #123: three reminder bundles x two locales. The fan-out failure + // handler adds a fourth bundle, also x two locales. + expect(onStack.length).toBe(8); }); it('is a named ARRAY of rows that each carry a name', () => { @@ -152,7 +182,9 @@ describe('the email-template barrel', () => { ).toBe(true); seen.push(name); } - expect(seen.length).toBe(3); + // Three reminder sweeps plus the fan-out failure handler (#123), which + // only counts here because `notifyNodes()` recurses into container regions. + expect(seen.length).toBe(4); }); it('every referenced bundle has an `en` row — the source language (AGENTS.md §8)', () => {