From 8d75248d371760c9bcbf371ca8e2e590843cb449 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 10:44:17 +0000 Subject: [PATCH] =?UTF-8?q?fix(example-todo):=20arm=20`task=5Fcompletion`?= =?UTF-8?q?=20=E2=80=94=20it=20bound=20to=20nothing=20and=20gated=20on=20a?= =?UTF-8?q?=20key=20nothing=20reads=20(#6882)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TaskCompletionFlow` declared `type: 'record_change'` and then declared neither key that arms one, so it was 1 of the 34 authored flows across the three bundled apps and the only dead one. - no `config.triggerType`: `resolveTriggerBinding` claims a record-change flow only for a `record-`-prefixed token, so it returned `undefined` and `activateFlowTrigger` never bound. `getTriggerBindingAudit` skips exactly that case (`if (!resolved) continue`), which is why no channel ever reported it. - the predicate sat on `config.triggerCondition`, a key nothing reads. Arming the trigger alone would have fired the flow on EVERY update. Repaired with `triggerType: 'record-after-update'` plus `condition: 'status == "completed" && previous.status != "completed"'` — the shape `showcase_task_completed` uses for this semantic. `-after-update` rather than `-after-write` keeps the insert leg out, where `previous` is `null` and `previous.status` aborts the predicate with `No such key: status`. A third fault surfaced the moment the flow could run: `get_task` filtered on `{taskId}`, an `isInput` variable nothing binds (a record-change run seeds `params` from the record, which carries `id`). It reads `{record.id}` now and the dead declaration is gone. `status: 'active'` declares the firing. `@objectstack/example-todo` runs `vitest run` now instead of `objectstack test` (a Quality-Protocol runner needing a live server, matching no `qa/*.test.json`), so its test files execute in CI for the first time. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01USNUyHEr7uaU6MoEWXitei --- .../todo-task-completion-trigger-armed.md | 51 +++++ examples/app-todo/package.json | 5 +- examples/app-todo/src/flows/task.flow.ts | 56 ++++- .../test/task-completion-trigger.test.ts | 207 ++++++++++++++++++ pnpm-lock.yaml | 9 + 5 files changed, 324 insertions(+), 4 deletions(-) create mode 100644 .changeset/todo-task-completion-trigger-armed.md create mode 100644 examples/app-todo/test/task-completion-trigger.test.ts diff --git a/.changeset/todo-task-completion-trigger-armed.md b/.changeset/todo-task-completion-trigger-armed.md new file mode 100644 index 0000000000..5134cca807 --- /dev/null +++ b/.changeset/todo-task-completion-trigger-armed.md @@ -0,0 +1,51 @@ +--- +"@objectstack/example-todo": patch +--- + +fix(example-todo): `task_completion` is a real record-change flow again — it bound to nothing and gated on a key nothing reads (#6882) + +`examples/app-todo`'s `TaskCompletionFlow` declared `type: 'record_change'` and then +declared neither key that arms one. It was 1 of the 34 authored flows across the three +bundled apps, and the only dead one. + +**Two faults on one start node, both silent.** + +1. **No `triggerType` at all.** `AutomationEngine.resolveTriggerBinding` claims a + record-change flow only when the authored token starts with `record-`. With the key + absent every later branch missed too (`timeRelative`, `config.schedule`, + `flow.type === 'schedule'`, `flow.type === 'api'`), the method returned `undefined`, + and `activateFlowTrigger` returned without binding. The flow declared itself + record-triggered and was, at runtime, a manual flow that never fired. +2. **The predicate was written to `triggerCondition`.** The trigger gate is + `config.condition` — the key the binding copies and `execute()` evaluates. A node + `config` is an open slot by design (ADR-0018), so the misspelling parsed silently. + Fixing (1) alone would have been *worse* than dead: the flow would have fired on every + update of every task. + +**Why no channel reported it.** `getTriggerBindingAudit` — the platform's own silent-miss +surface, and the source for both the automation plugin's `kernel:bootstrapped` warn loop +and the CLI startup summary's `unbound` list — opens with `if (!resolved) continue`, +reading "no binding" as "manual/screen flow, nothing to bind". So the missing key did not +*add* a diagnostic; it removed the flow from every diagnostic channel there is. The only +trace anywhere was the startup banner counting one more flow registered than bound, with +no name and no reason. + +**The repair.** `triggerType: 'record-after-update'` plus the predicate moved to +`config.condition` as `status == "completed" && previous.status != "completed"` — the +shape `showcase_task_completed` already uses for this exact semantic. `-after-update` +rather than `-after-write` on purpose: "marked as complete" is a transition, and the +insert leg has no `previous` to transition from — `previous` binds to `null` there, and +`previous.status` against `null` aborts the whole CEL predicate with `No such key: +status` rather than answering false. + +A third fault surfaced the moment the flow could run: `get_task` filtered on `{taskId}`, +an `isInput` variable nothing ever bound (a record-change run seeds `params` from the +triggering record, which carries `id`, not `taskId`), so the first armed run failed with +"1 filter condition(s) resolved to nothing and were dropped from the query". It now reads +`{record.id}`, the handle every other record-change flow in the corpus uses, and the dead +declaration is gone. + +`@objectstack/example-todo` also runs its own vitest suite now (`vitest run`, as +`app-crm` and `app-showcase` already do) instead of `objectstack test`, which is a +Quality-Protocol runner that needs a live server and matched no `qa/*.test.json` here — +so the package's test files had never executed in CI. diff --git a/examples/app-todo/package.json b/examples/app-todo/package.json index aa24f5cae5..5dc82f7f88 100644 --- a/examples/app-todo/package.json +++ b/examples/app-todo/package.json @@ -16,7 +16,7 @@ "build": "objectstack build", "validate": "objectstack validate", "typecheck": "tsc --noEmit", - "test": "objectstack test", + "test": "vitest run", "test:mcp": "tsx test/mcp-actions.e2e.ts" }, "dependencies": { @@ -32,6 +32,9 @@ }, "devDependencies": { "@objectstack/cli": "workspace:*", + "@objectstack/core": "workspace:*", + "@objectstack/service-automation": "workspace:*", + "@objectstack/trigger-record-change": "workspace:*", "tsx": "^4.23.1", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/examples/app-todo/src/flows/task.flow.ts b/examples/app-todo/src/flows/task.flow.ts index cd75dccc63..70433f56d3 100644 --- a/examples/app-todo/src/flows/task.flow.ts +++ b/examples/app-todo/src/flows/task.flow.ts @@ -120,17 +120,67 @@ export const TaskCompletionFlow: Flow = { label: 'Task Completion Process', description: 'Flow triggered when a task is marked as complete', type: 'record_change', + // The other half of "arm it deliberately" (#6882). `draft` is the status the + // schema applies when none is authored, and draft flows DO fire — so on a + // flow that now genuinely routes, the omission is the ambiguity + // `flow-draft-status-ambiguous` exists to report, and it started reporting + // here the moment the trigger resolved (while the flow was dead it routed + // nowhere, so even that rule skipped it). Declared, not silenced: `active` + // says the firing is intended, which is exactly this card's decision. The two + // schedule flows above keep their pre-existing `draft` — a separate call. + status: 'active', variables: [ - { name: 'taskId', type: 'text', isInput: true, isOutput: false }, + // #6882 — `taskId` used to be declared here as an `isInput` variable and + // read by `get_task` as `{taskId}`. Nothing ever bound it: a record-change + // run seeds `params` from the triggering RECORD, which carries `id`, not + // `taskId`, and `seedDeclaredVariables` binds an input only from + // `context.params[name]` (or a `defaultValue`, #4697 — this had neither). + // Invisible while the flow was dead; the first armed run failed at + // `get_task` with "1 filter condition(s) resolved to nothing and were + // dropped from the query: `{taskId}` (at id)". The triggering record is + // already in scope as `record`, which is how every other record-change flow + // in the corpus addresses it, so the declaration is gone rather than + // re-plumbed — declared means bound. { name: 'completedTask', type: 'record', isInput: false, isOutput: false }, ], nodes: [ - { id: 'start', type: 'start', label: 'Start', config: { objectName: 'todo_task', triggerCondition: 'record.status != previous.status && record.status == "completed"' } }, + // #6882 — this start node declared NEITHER of the two keys that arm a + // record-change flow, so the flow was registered and never bound: + // + // • no `triggerType` at all. `AutomationEngine.resolveTriggerBinding` + // claims a record-change flow only for a token starting with `record-`; + // with the key absent every later branch missed too (`timeRelative`, + // `config.schedule`, `flow.type === 'schedule'|'api'`) and the method + // returned `undefined`, so `activateFlowTrigger` returned without + // binding. `getTriggerBindingAudit` then SKIPS it (`if (!resolved) + // continue` — it reads as a manual/screen flow), which is why nothing + // anywhere reported the dead flow. + // • the predicate was written to `triggerCondition`, which no code reads. + // The trigger gate is `config.condition` — the key the binding copies + // and `execute()` evaluates. A node `config` is an open slot by design + // (ADR-0018), so the misspelling parsed silently; arming the trigger + // without moving it would have fired this flow on EVERY update. + // + // `record-after-update` (not `-write`): "marked as complete" is a + // transition, so the insert leg has no `previous` to transition FROM — the + // same shape the showcase's `showcase_task_completed` uses for this exact + // semantic. Keeping insert out of the binding is also what makes the + // predicate total: `previous` is bound to `null` on the insert leg, and + // `previous.status` against `null` aborts the whole CEL predicate with + // `No such key: status` (measured) rather than answering false. + { + id: 'start', type: 'start', label: 'Start', + config: { + objectName: 'todo_task', + triggerType: 'record-after-update', + condition: 'status == "completed" && previous.status != "completed"', + }, + }, { id: 'get_task', type: 'get_record', label: 'Get Completed Task', - config: { objectName: 'todo_task', filter: { id: '{taskId}' }, outputVariable: 'completedTask' }, + config: { objectName: 'todo_task', filter: { id: '{record.id}' }, outputVariable: 'completedTask' }, }, // A plain exclusive gateway — the branching is on the OUT-EDGES (e3/e4 // carry the predicate and its negation). It used to also set diff --git a/examples/app-todo/test/task-completion-trigger.test.ts b/examples/app-todo/test/task-completion-trigger.test.ts new file mode 100644 index 0000000000..9837c8e674 --- /dev/null +++ b/examples/app-todo/test/task-completion-trigger.test.ts @@ -0,0 +1,207 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6882] `task_completion` is a LIVE record-change flow — binding, gate, and + * the silence that hid it. + * + * The defect this pins was not a wrong answer; it was **no answer anywhere**. + * `TaskCompletionFlow` declared `type: 'record_change'` and then declared + * neither key that arms one: + * + * 1. no `config.triggerType`. `AutomationEngine.resolveTriggerBinding` claims + * a record-change flow only for a token starting with `record-`; absent, + * every later branch missed too and it returned `undefined`, so + * `activateFlowTrigger` returned without binding. + * 2. the predicate was written to `config.triggerCondition`, a key nothing + * reads. The gate is `config.condition`. A node `config` is an open slot + * by design (ADR-0018), so the misspelling parsed silently — and arming + * (1) without fixing (2) would have been WORSE than dead: the flow would + * have fired on every single update. + * + * Why nothing caught it, and why the reverse check below asserts an ABSENCE: + * `getTriggerBindingAudit` — the platform's own silent-miss channel, and the + * source for both the automation plugin's `kernel:bootstrapped` warn loop and + * the CLI startup summary's `unbound` list — opens with + * `if (!resolved) continue`, reading "no binding" as "manual/screen flow, + * nothing to bind". So removing `triggerType` does not ADD a diagnostic; it + * REMOVES the flow from every diagnostic channel there is. That inverted + * direction is the whole reason this needs a test rather than a lint rule + * reading a warning nobody emits. + * + * The suite drives the app's REAL metadata (`allFlows`, the real `Task` + * object) through a real kernel and real writes, so it fails if any link is + * re-broken: the trigger token, the condition key, the predicate's dialect, or + * the engine's resolver. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectKernel } from '@objectstack/core'; +import { ObjectQLPlugin } from '@objectstack/objectql'; +import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm'; +import { AutomationServicePlugin, type AutomationEngine } from '@objectstack/service-automation'; +import { RecordChangeTriggerPlugin } from '@objectstack/trigger-record-change'; + +import { allFlows, TaskCompletionFlow } from '../src/flows/index.js'; +import { Task } from '../src/objects/task.object.js'; + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +/** Every driver a test opened, closed when that test ends. */ +const openDrivers: Array<{ disconnect?: () => Promise }> = []; +afterEach(async () => { + while (openDrivers.length) { + try { await openDrivers.pop()?.disconnect?.(); } catch { /* noop */ } + } +}); + +/** + * A real kernel with the app's real `todo_task` object: ObjectQL over an + * in-process sqlite-wasm database (the same driver `test/mcp-actions.e2e.ts` + * boots), the automation engine, and the record-change trigger. No test double + * anywhere in the chain — #6882 was precisely a chain that looked correct in + * the metadata and bound to nothing at runtime. + */ +async function bootTodoKernel(): Promise<{ + automation: AutomationEngine & Record; + data: any; +}> { + const kernel = new ObjectKernel({ logger: { level: 'silent' } } as any); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin()); + await kernel.use(new RecordChangeTriggerPlugin()); + await kernel.bootstrap(); + + const objectql: any = kernel.getService('objectql'); + const data: any = kernel.getService('data'); + const automation = kernel.getService('automation') as AutomationEngine & Record; + + const driver: any = new SqliteWasmDriver({ filename: ':memory:' }); + await driver.connect(); + objectql.registerDriver(driver, true); + openDrivers.push(driver); + objectql.registry.registerObject(Task, 'todo', 'todo'); + await objectql.syncSchemas(); + + for (const flow of allFlows) automation.registerFlow(flow.name, flow); + return { automation, data }; +} + +/** The `start` node config of a flow, as authored. */ +function startConfig(flow: unknown): Record { + const nodes = (flow as { nodes?: Array<{ type?: string; config?: Record }> }).nodes ?? []; + return nodes.find((n) => n?.type === 'start')?.config ?? {}; +} + +describe('#6882 — app-todo `task_completion` is armed, not dead', () => { + it('declares the two keys the engine actually reads', () => { + const config = startConfig(TaskCompletionFlow); + + // The token has to be one the record-change trigger maps to a hook, not + // merely `record-`-prefixed: the engine routes ANY `record-` string to the + // trigger, which then maps an unknown token to no hook at all. + expect(config.triggerType).toBe('record-after-update'); + expect(config.objectName).toBe('todo_task'); + + // `condition` is the gate; `triggerCondition` is the misspelling that made + // the predicate inert. Assert BOTH, so re-introducing the typo alongside a + // correct `condition` is still caught. + expect(typeof config.condition).toBe('string'); + expect(config).not.toHaveProperty('triggerCondition'); + }); + + it('no start node in the app writes its predicate to an unread key', () => { + // The class, not just the instance: a node `config` is an open slot + // (ADR-0018), so any misspelling here parses silently and gates nothing. + for (const flow of allFlows) { + const config = startConfig(flow); + for (const key of ['triggerCondition', 'trigger_condition', 'startCondition']) { + expect(config, `flow '${flow.name}' start node`).not.toHaveProperty(key); + } + } + }); + + it('BINDS: the flow is wired to afterUpdate on todo_task at runtime', async () => { + const { automation } = await bootTodoKernel(); + + const state = automation.getFlowRuntimeStates().find((s: any) => s.name === 'task_completion'); + expect(state).toBeDefined(); + expect(state!.bound, 'the flow resolved a binding and the trigger accepted it').toBe(true); + expect(state!.triggerType).toBe('record_change'); + expect(state!.object).toBe('todo_task'); + + // And the silent-miss audit has nothing to say about it, because there is + // nothing left unbound. + const audit = automation.getTriggerBindingAudit(); + expect(audit.find((a: any) => a.flowName === 'task_completion')).toBeUndefined(); + }); + + it('REVERSE: drop `triggerType` and the flow vanishes from every audit channel', async () => { + const { automation } = await bootTodoKernel(); + + // The pre-#6882 shape, rebuilt from the real flow so it cannot drift: the + // authored trigger token removed, everything else identical. + const dead = JSON.parse(JSON.stringify(TaskCompletionFlow)) as any; + dead.name = 'task_completion_unarmed_fixture'; + delete dead.nodes.find((n: any) => n.type === 'start').config.triggerType; + automation.registerFlow(dead.name, dead); + + const state = automation.getFlowRuntimeStates().find((s: any) => s.name === dead.name); + expect(state!.bound).toBe(false); + // `triggerType` undefined is the resolver having given up entirely — the + // flow reads as manual/screen, which is why nothing downstream reports it. + expect(state!.triggerType).toBeUndefined(); + + // The direction that makes this a test and not a lint rule: the audit gains + // NOTHING. An unbound-but-resolvable flow appears here; this one does not, + // so no warn loop and no CLI `unbound` line ever names it. + const audit = automation.getTriggerBindingAudit(); + expect(audit.find((a: any) => a.flowName === dead.name)).toBeUndefined(); + + // Non-vacuous: the same fixture WITH the key is bound and audited-clean — + // so the assertions above are about the missing key, not about the fixture + // failing to register. + expect( + automation.getFlowRuntimeStates().find((s: any) => s.name === 'task_completion')!.bound, + ).toBe(true); + }); + + it('GATES: only a transition INTO completed fires the flow', async () => { + const { automation, data } = await bootTodoKernel(); + const ctx = { context: { userId: 'u_todo' } }; + const runCount = async () => (await automation.listRuns('task_completion')).length; + + // `completed_date` is seeded on CREATE (the engine allows a read-only field + // to be seeded by an insert, and the object's `completed_date_required` + // rule refuses the completion write otherwise — see the note on that rule). + const created = await data.insert('todo_task', { + subject: 'Water the plants', + status: 'not_started', + priority: 'normal', + is_recurring: false, + completed_date: '2026-08-09T10:00:00.000Z', + }, ctx); + const id = Array.isArray(created) ? created[0].id : created.id; + await sleep(150); + expect(await runCount(), 'an INSERT must not reach an after-update flow').toBe(0); + + // A write that should NOT fire it: the record changes, the status does not. + await data.update('todo_task', { priority: 'high' }, { where: { id }, ...ctx }); + await sleep(150); + expect(await runCount(), 'the start condition must gate on the status transition').toBe(0); + + // A write that SHOULD fire it. + await data.update('todo_task', { status: 'completed' }, { where: { id }, ...ctx }); + await sleep(400); + const afterTransition = await automation.listRuns('task_completion'); + expect(afterTransition.length, 'the completion transition must launch the flow').toBe(1); + // It RAN, rather than merely being dispatched: the whole flow reached `end`. + expect(afterTransition[0].status).toBe('completed'); + expect((afterTransition[0].steps ?? []).map((s: any) => s.nodeId)).toContain('get_task'); + + // And it does not re-fire on every later save of an already-completed task — + // the half of the predicate that `previous` exists for. + await data.update('todo_task', { progress_percent: 100 }, { where: { id }, ...ctx }); + await sleep(300); + expect(await runCount(), 're-saving a completed task must not re-fire it').toBe(1); + }, 30000); +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 981971c9df..30fa172e88 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -251,6 +251,15 @@ importers: '@objectstack/cli': specifier: workspace:* version: link:../../packages/cli + '@objectstack/core': + specifier: workspace:* + version: link:../../packages/core + '@objectstack/service-automation': + specifier: workspace:* + version: link:../../packages/services/service-automation + '@objectstack/trigger-record-change': + specifier: workspace:* + version: link:../../packages/triggers/trigger-record-change tsx: specifier: ^4.23.1 version: 4.23.1