From 024d1bb5120943d6ec5e6ac35b8fe37137d2c598 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 12:11:00 +0000 Subject: [PATCH] fix(hooks): give ctx.api.update the engine's real (document, options) shape (#616) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `HookObjectApi.update` was declared `(id: string, doc)` while both surfaces the runtime can inject as `ctx.api` — ObjectRepository and the sandbox repo facade — forward to `engine.update(object, data, options)`. The second positional argument is the OPTIONS bag, so every hook-side derived write threw "update('crm_opportunity') does not recognise option 'amount'" (#4371) on every invocation: 96 throws on one boot of a freshly seeded install. All nine call sites are `onError: 'log'`, so the only symptom was a parent record that never moved. The declaration was the contract as far as the compiler was concerned (`HookContext.api` is `unknown`), and both hook stand-ins implemented the declaration rather than the engine, so the suite was green throughout. - `_hook-api.ts` now describes the real surface: `update(HookUpdateDoc, HookUpdateOptions)` with the id inside the document, `delete({ where })`, and no `updateMany` — a method neither injected shape has. - Migrated all nine call sites (rollups, campaign snapshot, account promotion, signed_date, case rollup, quote close-out, activity bubble). - `hook-harness.ts` rejects the broken shape, a missing/disagreeing row scope and a document without an id, instead of quietly honouring them; the second, more permissive ad-hoc stand-in in `hooks-runtime.test.ts` is deleted in favour of it. - New `test/hook-write-shape.test.ts` asserts the ARGUMENT LIST reaching the engine for all nine writes, running each hook's shipped body through the real QuickJS sandbox, plus a static scan so a future `.update(id, …)` fails. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019SS7C5SXpniKeCApxgARyf --- .changeset/hook-api-update-document-shape.md | 29 ++ src/objects/_hook-api.ts | 66 ++- src/objects/campaign.hook.ts | 26 +- src/objects/case.hook.ts | 7 +- src/objects/contract.hook.ts | 10 +- src/objects/opportunity.hook.ts | 5 +- src/objects/opportunity_line_item.hook.ts | 5 +- src/objects/quote.hook.ts | 12 +- src/objects/quote_line_item.hook.ts | 14 +- src/objects/task.hook.ts | 5 +- test/helpers/hook-harness.ts | 90 +++- test/hook-write-shape.test.ts | 409 +++++++++++++++++++ test/hooks-runtime.test.ts | 50 +-- 13 files changed, 646 insertions(+), 82 deletions(-) create mode 100644 .changeset/hook-api-update-document-shape.md create mode 100644 test/hook-write-shape.test.ts diff --git a/.changeset/hook-api-update-document-shape.md b/.changeset/hook-api-update-document-shape.md new file mode 100644 index 000000000..e3adc13f2 --- /dev/null +++ b/.changeset/hook-api-update-document-shape.md @@ -0,0 +1,29 @@ +--- +'hotcrm': patch +--- + +Fix every hook-side derived write: opportunity/quote rollups, the campaign +completion snapshot, account promotion, the contract `signed_date` stamp, the +case service rollup and the task activity bubble now actually update their +parent record. + +All nine of them called `ctx.api.object(x).update(id, doc)`, but the repository +facade the runtime injects as `ctx.api` takes `update(document, options)` — the +second positional argument is the OPTIONS bag. Every invocation therefore threw +`update('crm_opportunity') does not recognise option 'amount'` and, because +these hooks are all `onError: 'log'`, the only symptom was a parent record that +silently never moved: 96 such throws on one boot of a freshly seeded install, +one per line-item write. Editing a line item did not move the opportunity's +amount or the quote's totals. + +The cause was a type that described an API that does not exist: +`src/objects/_hook-api.ts` declared `update(id: string, doc)`, so the compiler +blessed all nine call sites, and both hook stand-ins implemented the +declaration rather than the engine, so the suite stayed green. `HookObjectApi` +now describes the real surface — `update({ id, …fields }, { where: { id } })`, +`delete({ where })`, and no `updateMany` (a method neither injected shape has) +— which makes the old spelling a compile error rather than a runtime one. The +hook harness rejects the broken shape instead of quietly honouring it, and the +new `test/hook-write-shape.test.ts` asserts the argument list that reaches the +engine for all nine writes, running each hook's shipped body through the real +QuickJS sandbox. Refs #616. diff --git a/src/objects/_hook-api.ts b/src/objects/_hook-api.ts index 9832401ce..67c0606bc 100644 --- a/src/objects/_hook-api.ts +++ b/src/objects/_hook-api.ts @@ -12,6 +12,18 @@ * * Methods are the superset actually used across the CRM hooks. Cast with * `ctx.api as HookApi | undefined` and guard for `undefined` before use. + * + * ⚠️ This file is a HAND-WRITTEN description of a surface the compiler cannot + * check for us (`HookContext.api` is `unknown`), which makes it the one place + * in the app where a type can be confidently WRONG. It was: `update` was + * declared `(id, doc)` for months while the runtime's second positional + * parameter is options, so the compiler blessed a call the engine rejects on + * every invocation — eight hook-side derived writes (rollups, the campaign + * snapshot, account promotion) were dead in production while the suite stayed + * green (#616). Every signature here must therefore be justified against the + * REAL implementation, and pinned by a test that runs the shipped body against + * a real engine rather than against a friendlier stand-in + * (`test/hook-write-shape.test.ts`). */ type Doc = Record; @@ -42,14 +54,62 @@ export interface HookQuery { top?: number; } +/** + * The document handed to `update` — the target `id` travels INSIDE it. + * + * `ctx.api.object(x)` is a repository facade, not an id-addressed CRUD client: + * both surfaces the runtime can inject (`ObjectRepository` from + * `@objectstack/objectql`, and `buildEngineRepoFacade` from + * `@objectstack/runtime` for a sandboxed body) forward straight to + * `engine.update(object, data, options)`, and the engine resolves the row from + * `data.id` (falling back to `options.where.id`). There is no `(id, doc)` + * overload anywhere on that path. + * + * Requiring `id` here is what makes the old `update(id, doc)` spelling a + * COMPILE error rather than a runtime one: an id string is not a document. + */ +export type HookUpdateDoc = Doc & { id: string }; + +/** + * Options accepted by `update`. + * + * Deliberately narrow. The engine's legal set is larger (`multi`, `upsert`, + * `returning`, `transaction`, `timezone`, …) but every extra key is a way for a + * hook to do something a hook should not — `multi` in particular turns a + * single-row derived write into a bulk one. Anything a hook legitimately needs + * is added here on purpose; excess-property checking rejects the rest at the + * call site, which is how the `amount`-as-an-option throw this type used to + * produce (#616) stays impossible. + * + * `where` is REQUIRED and duplicates `doc.id` on purpose: the row scope a write + * runs under is the thing worth being explicit about, and it is the shape the + * rest of the app already uses (`src/actions/contact.actions.ts`, pinned in + * `test/action-sandbox.test.ts`). One idiom, not two. + */ +export interface HookUpdateOptions { + where: Doc; +} + +/** Options accepted by `delete` — again a predicate, never a bare id. */ +export interface HookDeleteOptions { + where: Doc; +} + +/** + * The write surface. Only the methods that exist on BOTH injected shapes are + * declared: `ObjectRepository` has `updateById`/`deleteById`/`aggregate` that + * the sandbox facade does not, and NEITHER has `updateMany` (this type used to + * declare it — a method call that would have thrown `is not a function`). + * Declaring only the intersection means a hook cannot compile against a method + * that may not be there at runtime. + */ export interface HookObjectApi { count: (q: HookQuery) => Promise; find: (q: HookQuery) => Promise>; findOne: (q: HookQuery) => Promise; insert: (doc: Doc) => Promise; - update: (id: string, doc: Doc) => Promise; - updateMany: (q: { where: Doc; doc: Doc }) => Promise; - delete: (id: string) => Promise; + update: (doc: HookUpdateDoc, options: HookUpdateOptions) => Promise; + delete: (options: HookDeleteOptions) => Promise; } export interface HookApi { diff --git a/src/objects/campaign.hook.ts b/src/objects/campaign.hook.ts index 2a6c48aa7..926ae735e 100644 --- a/src/objects/campaign.hook.ts +++ b/src/objects/campaign.hook.ts @@ -98,17 +98,21 @@ const campaignCompleted: Hook = { return sum + amt; }, 0); - await api.object('crm_campaign').update(id, { - num_leads: leadIds.length, - num_converted_leads: convertedLeads, - num_opportunities: opportunities, - num_won_opportunities: wonOpps, - // "Total members enrolled" — the single definition of num_sent. The old - // `sent || members` under-counted as members progressed past `sent`. - num_sent: members, - num_responses: responded, - actual_revenue: actualRevenue, - }); + await api.object('crm_campaign').update( + { + id, + num_leads: leadIds.length, + num_converted_leads: convertedLeads, + num_opportunities: opportunities, + num_won_opportunities: wonOpps, + // "Total members enrolled" — the single definition of num_sent. The old + // `sent || members` under-counted as members progressed past `sent`. + num_sent: members, + num_responses: responded, + actual_revenue: actualRevenue, + }, + { where: { id } }, + ); }, }; diff --git a/src/objects/case.hook.ts b/src/objects/case.hook.ts index 6e6f0e267..a1d3bd9be 100644 --- a/src/objects/case.hook.ts +++ b/src/objects/case.hook.ts @@ -143,9 +143,10 @@ const caseSideEffects: Hook = { // its close time) and re-entered the record-change trigger surface. if (input.status === 'resolved' && previous.status !== 'resolved') { if (accountId) { - await api.object('crm_account').update(accountId, { - last_activity_date: new Date().toISOString().slice(0, 10), - }); + await api.object('crm_account').update( + { id: accountId, last_activity_date: new Date().toISOString().slice(0, 10) }, + { where: { id: accountId } }, + ); } } }, diff --git a/src/objects/contract.hook.ts b/src/objects/contract.hook.ts index 78f7c83d4..ac0fb22bf 100644 --- a/src/objects/contract.hook.ts +++ b/src/objects/contract.hook.ts @@ -102,13 +102,19 @@ const contractActivation: Hook = { undefined; if (id && !input.signed_date && !previous?.signed_date) { - await api.object('crm_contract').update(id, { signed_date: new Date().toISOString().slice(0, 10) }); + await api.object('crm_contract').update( + { id, signed_date: new Date().toISOString().slice(0, 10) }, + { where: { id } }, + ); } if (accountId) { const account = await api.object('crm_account').findOne({ where: { id: accountId } }); if (account && account.type !== 'customer') { - await api.object('crm_account').update(accountId, { type: 'customer' }); + await api.object('crm_account').update( + { id: accountId, type: 'customer' }, + { where: { id: accountId } }, + ); } } diff --git a/src/objects/opportunity.hook.ts b/src/objects/opportunity.hook.ts index 863577d06..b9c6ffe47 100644 --- a/src/objects/opportunity.hook.ts +++ b/src/objects/opportunity.hook.ts @@ -168,7 +168,10 @@ const opportunityWonHook: Hook = { const account = await api.object('crm_account').findOne({ where: { id: accountId } }); if (account && account.type !== 'customer') { - await api.object('crm_account').update(accountId, { type: 'customer' }); + await api.object('crm_account').update( + { id: accountId, type: 'customer' }, + { where: { id: accountId } }, + ); } const oppId = (typeof input.id === 'string' && input.id) || previous?.id; diff --git a/src/objects/opportunity_line_item.hook.ts b/src/objects/opportunity_line_item.hook.ts index 9622c0e70..f1905b613 100644 --- a/src/objects/opportunity_line_item.hook.ts +++ b/src/objects/opportunity_line_item.hook.ts @@ -89,7 +89,10 @@ const opportunityAmountRollup: Hook = { } sum = Math.round(sum * 100) / 100; if (sum > 0) { - await api.object('crm_opportunity').update(oppId, { amount: sum }); + await api.object('crm_opportunity').update( + { id: oppId, amount: sum }, + { where: { id: oppId } }, + ); } } }, diff --git a/src/objects/quote.hook.ts b/src/objects/quote.hook.ts index 8a14c105e..befda9fdf 100644 --- a/src/objects/quote.hook.ts +++ b/src/objects/quote.hook.ts @@ -130,10 +130,14 @@ const quoteAccepted: Hook = { if (opportunityId) { const opp = await api.object('crm_opportunity').findOne({ where: { id: opportunityId } }); if (opp && opp.stage !== 'closed_won' && opp.stage !== 'closed_lost') { - await api.object('crm_opportunity').update(opportunityId, { - stage: 'closed_won', - close_date: today, - }); + await api.object('crm_opportunity').update( + { + id: opportunityId, + stage: 'closed_won', + close_date: today, + }, + { where: { id: opportunityId } }, + ); } } }, diff --git a/src/objects/quote_line_item.hook.ts b/src/objects/quote_line_item.hook.ts index 08d47d35a..673e05e1e 100644 --- a/src/objects/quote_line_item.hook.ts +++ b/src/objects/quote_line_item.hook.ts @@ -94,11 +94,15 @@ const quoteTotalRollup: Hook = { const discountAmount = Math.round(subtotal * (quoteDiscountPct / 100) * 100) / 100; const total = Math.round((subtotal - discountAmount + tax + shipping) * 100) / 100; - await api.object('crm_quote').update(quoteId, { - subtotal, - discount_amount: discountAmount, - total_price: total, - }); + await api.object('crm_quote').update( + { + id: quoteId, + subtotal, + discount_amount: discountAmount, + total_price: total, + }, + { where: { id: quoteId } }, + ); } }, }; diff --git a/src/objects/task.hook.ts b/src/objects/task.hook.ts index ee0a4a4fc..4a051c0d7 100644 --- a/src/objects/task.hook.ts +++ b/src/objects/task.hook.ts @@ -254,7 +254,10 @@ const taskBubble: Hook = { const activityWrite = activityWriteByType[targetType]; if (!activityWrite) return; try { - await api.object(targetType).update(targetId, activityWrite); + await api.object(targetType).update( + { ...activityWrite, id: targetId }, + { where: { id: targetId } }, + ); } catch { // Best-effort activity bubble; never break the parent write. No `console` // in the L2 hook sandbox (would throw ReferenceError — cf. #471). diff --git a/test/helpers/hook-harness.ts b/test/helpers/hook-harness.ts index dbd800328..cf48d8852 100644 --- a/test/helpers/hook-harness.ts +++ b/test/helpers/hook-harness.ts @@ -1,6 +1,12 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import type { HookApi, HookQuery } from '../../src/objects/_hook-api'; +import type { + HookApi, + HookDeleteOptions, + HookQuery, + HookUpdateDoc, + HookUpdateOptions, +} from '../../src/objects/_hook-api'; /** * In-memory harness for running REAL hook handlers. @@ -20,11 +26,19 @@ import type { HookApi, HookQuery } from '../../src/objects/_hook-api'; export type Rec = Record; -/** A recorded write, for asserting that a hook did (or did not) touch the data. */ +/** + * A recorded write, for asserting that a hook did (or did not) touch the data. + * + * `args` is the ARGUMENT LIST as the engine would have received it, not a + * friendly re-packaging of it: `update` records `[doc, options]`. A test that + * only checks the resulting row cannot tell a correctly-shaped call from one + * the real engine would have thrown on, which is exactly how the `(id, doc)` + * spelling survived eight call sites (#616). + */ export interface RecordedCall { - op: 'insert' | 'update' | 'updateMany' | 'delete'; + op: 'insert' | 'update' | 'delete'; object: string; - args: Rec; + args: unknown[]; } /** Does `value` satisfy a single Mongo-style condition? */ @@ -122,26 +136,66 @@ export function makeHarness(store: Record = {}): Harness { }, async insert(doc: Rec) { const rec = { id: doc.id ?? `${name}_${++seq}`, ...doc }; - calls.push({ op: 'insert', object: name, args: rec }); + calls.push({ op: 'insert', object: name, args: [doc] }); rows(name).push(rec); return rec; }, - async update(id: string, doc: Rec) { - calls.push({ op: 'update', object: name, args: { id, doc } }); - const row = rows(name).find((r) => r.id === id); + /** + * `update(doc, options)` — the engine's shape, enforced at runtime and + * not merely declared. + * + * TypeScript alone cannot hold this line: hooks reach `ctx.api` through + * a `ctx.api as HookApi` cast on an `unknown`, and every test builds its + * ctx with `as any`. So the checks below are the ones that actually + * fire. They mirror what the kernel does with a mis-shaped call — throw + * on an id where a document belongs (`update('opp_1', {...})` reaches + * `rejectUnknownEngineOptions` as `update('opp_1' , {amount})` and dies + * with "does not recognise option 'amount'") — so a hook that regresses + * fails here rather than silently no-op'ing in production. + */ + async update(doc: HookUpdateDoc, options: HookUpdateOptions) { + if (typeof doc !== 'object' || doc === null || Array.isArray(doc)) { + throw new Error( + `hook-harness: update(${JSON.stringify(doc)}, …) was called with an id where the ` + + 'repository facade takes a DOCUMENT. The kernel reads the target from `data.id` ' + + "and rejects the second positional document as an unknown option (#616). " + + 'Use `update({ id, …fields }, { where: { id } })`.', + ); + } + if (typeof doc.id !== 'string' || !doc.id) { + throw new Error( + 'hook-harness: update() document carries no `id` — the kernel would have nothing ' + + 'to resolve the row from. Use `update({ id, …fields }, { where: { id } })`.', + ); + } + if (!options || typeof options.where !== 'object' || options.where === null) { + throw new Error( + 'hook-harness: update() was called without `{ where: { id } }`. The row scope a ' + + 'derived write runs under is not optional in this app (see `_hook-api.ts`).', + ); + } + const scoped = (options.where as Rec).id; + if (scoped !== undefined && scoped !== doc.id) { + throw new Error( + `hook-harness: update() targets '${doc.id}' but scopes to '${String(scoped)}'. The ` + + 'kernel prefers `data.id` and would silently write the wrong row.', + ); + } + calls.push({ op: 'update', object: name, args: [doc, options] }); + const row = rows(name).find((r) => r.id === doc.id); if (row) Object.assign(row, doc); return row; }, - async updateMany(q: { where: Rec; doc: Rec }) { - calls.push({ op: 'updateMany', object: name, args: q }); - const hits = rows(name).filter((r) => matches(r, q.where)); - for (const r of hits) Object.assign(r, q.doc); - return { modified: hits.length }; - }, - async delete(id: string) { - calls.push({ op: 'delete', object: name, args: { id } }); + async delete(options: HookDeleteOptions) { + if (!options || typeof options.where !== 'object' || options.where === null) { + throw new Error( + 'hook-harness: delete() takes `{ where: … }` — the facade has no id-addressed ' + + 'overload (#616).', + ); + } + calls.push({ op: 'delete', object: name, args: [options] }); const list = rows(name); - const i = list.findIndex((r) => r.id === id); + const i = list.findIndex((r) => matches(r, options.where as Rec)); if (i >= 0) list.splice(i, 1); return { deleted: i >= 0 ? 1 : 0 }; }, @@ -170,7 +224,7 @@ export function makeDeniedApi(message = "Access denied: not 'find'"): HookApi { object() { return { count: boom, find: boom, findOne: boom, - insert: boom, update: boom, updateMany: boom, delete: boom, + insert: boom, update: boom, delete: boom, } as never; }, }; diff --git a/test/hook-write-shape.test.ts b/test/hook-write-shape.test.ts new file mode 100644 index 000000000..2d6f620e2 --- /dev/null +++ b/test/hook-write-shape.test.ts @@ -0,0 +1,409 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { readFileSync, readdirSync } from 'node:fs'; +import { join } from 'node:path'; +import { ObjectQL } from '@objectstack/objectql'; +import { InMemoryDriver } from '@objectstack/driver-memory'; +import { allHooks } from '../src/hooks'; +import { makeHarness } from './helpers/hook-harness'; +import { makeSandboxEngine, runHookBody, type Rec } from './helpers/action-sandbox'; + +/** + * `ctx.api.object(x).update(...)` takes a DOCUMENT, not an id. This file is the + * proof — and the reason the proof has to exist. + * + * `src/objects/_hook-api.ts` declared `update(id, doc)` for months. Nothing in + * the repo could contradict it: `HookContext.api` is `unknown`, so the + * declaration WAS the contract as far as the compiler was concerned, and both + * hook stand-ins implemented the declaration rather than the engine. Every + * hook-side derived write in the app therefore compiled, tested green, and + * threw on every single invocation in production (#616): + * + * update('crm_opportunity') does not recognise option 'amount'. The engine + * executes none of it, so the call would succeed with the option silently + * ignored (#4371). + * + * Rollups, the campaign completion snapshot and account promotion were dead for + * as long as that type was wrong, and every one of them is `onError: 'log'`, so + * the only symptom was a parent record that never moved. + * + * The lesson is about what the tests assert, not about the signature: a test + * that watches a stand-in's row change proves the stand-in works. So these + * assert the ARGUMENT LIST that reaches the engine, through the same + * `hookBodyRunnerFactory` + repo facade the runtime wires at boot, with the + * facade's contract itself pinned against a real ObjectQL below. A test written + * this way fails against the broken signature; the ones that existed did not. + */ + +type AnyRec = Record; + +const hook = (name: string): AnyRec => { + const found = (allHooks as AnyRec[]).find((h) => h.name === name); + if (!found) throw new Error(`no ${name} hook registered`); + return found; +}; + +/** Today, the way every hook here stamps it. */ +const today = new Date().toISOString().slice(0, 10); + +const HOOK_DIR = join(process.cwd(), 'src', 'objects'); +const hookFiles = (): string[] => readdirSync(HOOK_DIR).filter((f) => f.endsWith('.ts')); + +/** + * A file's source with comment lines blanked out. + * + * Both scans below look for `.update(`, and `_hook-api.ts` documents the very + * signature it declares (`engine.update(object, data, options)`). Prose about + * the API is not a call into it. + */ +const codeOf = (file: string): string => + readFileSync(join(HOOK_DIR, file), 'utf8') + .split('\n') + .map((line) => (/^\s*(\/\/|\*|\/\*)/.test(line) ? '' : line)) + .join('\n'); + +// ────────────────────────────────── the kernel says what the shape is ── + +describe('the kernel’s update contract, first-hand', () => { + const makeEngine = () => + ObjectQL.create({ + datasources: { default: new InMemoryDriver({ persistence: false }) }, + objects: { + crm_opportunity: { + name: 'crm_opportunity', + fields: { id: { type: 'text' }, name: { type: 'text' }, amount: { type: 'number' } }, + }, + } as never, + }); + + let ql: Awaited>; + beforeAll(async () => { + ql = await makeEngine(); + }); + afterAll(async () => { + await ql?.close(); + }); + + it('rejects the (id, doc) spelling every hook used to ship', async () => { + const api = ql.createContext({ isSystem: true }); + const row = await api.object('crm_opportunity').insert({ name: 'Rollup target', amount: 1 }); + + await expect( + (api.object('crm_opportunity') as AnyRec).update(row.id, { amount: 2450 }), + ).rejects.toThrow(/does not recognise option 'amount'|Update requires an ID/); + + // The failure mode that made this invisible: the throw is the ONLY signal. + // Nothing about the record says a write was attempted. + expect((await api.object('crm_opportunity').findOne({ where: { id: row.id } }))?.amount).toBe(1); + }); + + it('applies `update({ id, …fields }, { where: { id } })` — the shape the hooks now ship', async () => { + const api = ql.createContext({ isSystem: true }); + const row = await api.object('crm_opportunity').insert({ name: 'Rollup target', amount: 1 }); + + await api + .object('crm_opportunity') + .update({ id: row.id, amount: 2450 }, { where: { id: row.id } }); + + expect((await api.object('crm_opportunity').findOne({ where: { id: row.id } }))?.amount).toBe(2450); + }); + + it('has no `updateMany` to call at all (the method `_hook-api.ts` used to declare)', () => { + const api = ql.createContext({ isSystem: true }); + expect((api.object('crm_opportunity') as AnyRec).updateMany).toBeUndefined(); + }); +}); + +// ───────────────────────── every hook-side derived write, shape-asserted ── + +/** + * One row per `update(...)` call site under `src/objects/` — the table from + * #616, executed. + * + * `runHookBody` runs the hook's LOWERED body (the `body.source` the build + * ships) inside QuickJS against the runtime's own repo facade, so what these + * assert is not "the handler function worked" but "these are the arguments the + * engine was handed". + */ +interface WriteCase { + /** Registered hook name. */ + hook: string; + /** The lifecycle event to fire. */ + event: string; + input: Rec; + previous?: Rec; + /** Rows the hook reads / writes. */ + seed: Record; + /** The derived writes the hook must perform, in no particular order. */ + writes: Array<{ object: string; id: string; doc: Rec }>; +} + +const CASES: Record = { + 'opportunity_amount_rollup — the opportunity amount re-rolls from its lines': { + hook: 'opportunity_amount_rollup', + event: 'afterInsert', + input: { crm_opportunity: 'opp_1' }, + seed: { + crm_opportunity: [{ id: 'opp_1', stage: 'qualification', amount: 1 }], + crm_opportunity_line_item: [ + { id: 'oli_1', crm_opportunity: 'opp_1', quantity: 2, unit_price: 1000, discount: 0 }, + { id: 'oli_2', crm_opportunity: 'opp_1', quantity: 1, unit_price: 500, discount: 10 }, + ], + }, + writes: [{ object: 'crm_opportunity', id: 'opp_1', doc: { amount: 2450 } }], + }, + + 'quote_total_rollup — subtotal / discount_amount / total_price re-roll': { + hook: 'quote_total_rollup', + event: 'afterInsert', + input: { crm_quote: 'q_1' }, + seed: { + crm_quote: [{ id: 'q_1', status: 'draft', discount: 10, tax: 50, shipping_handling: 25 }], + crm_quote_line_item: [ + { id: 'qli_1', crm_quote: 'q_1', quantity: 4, unit_price: 100, discount: 0 }, + ], + }, + writes: [ + { + object: 'crm_quote', + id: 'q_1', + doc: { subtotal: 400, discount_amount: 40, total_price: 435 }, + }, + ], + }, + + 'campaign_snapshot_metrics — the completion snapshot': { + hook: 'campaign_snapshot_metrics', + event: 'afterUpdate', + input: { id: 'cmp_1', status: 'completed' }, + previous: { id: 'cmp_1', status: 'in_progress' }, + seed: { + crm_campaign: [{ id: 'cmp_1', status: 'completed' }], + crm_campaign_member: [ + { id: 'cm_1', crm_campaign: 'cmp_1', crm_lead: 'lead_1', status: 'responded' }, + ], + crm_opportunity: [ + { id: 'opp_1', crm_campaign: 'cmp_1', stage: 'closed_won', amount: 1000 }, + ], + crm_lead: [{ id: 'lead_1', is_converted: true }], + }, + writes: [ + { + object: 'crm_campaign', + id: 'cmp_1', + // Only the fields the stub engine can compute exactly are asserted here + // — its `where` matcher is equality-only, so the `$in` lead count comes + // back 0. The metric arithmetic is covered in `hooks-runtime*.test.ts`; + // what is on trial in this file is the call shape. + doc: { num_opportunities: 1, num_won_opportunities: 1, num_sent: 1, actual_revenue: 1000 }, + }, + ], + }, + + 'opportunity_promote_account — the won deal promotes its account': { + hook: 'opportunity_promote_account', + event: 'afterUpdate', + input: { id: 'opp_1', stage: 'closed_won', crm_account: 'acc_1', owner: 'usr_1' }, + previous: { id: 'opp_1', stage: 'negotiation' }, + seed: { crm_account: [{ id: 'acc_1', type: 'prospect' }] }, + writes: [{ object: 'crm_account', id: 'acc_1', doc: { type: 'customer' } }], + }, + + 'contract_on_activation — signed_date stamp AND account promotion': { + hook: 'contract_on_activation', + event: 'afterUpdate', + input: { id: 'ctr_1', status: 'activated', crm_account: 'acc_1', end_date: '2027-01-01' }, + previous: { id: 'ctr_1', status: 'draft' }, + seed: { + crm_contract: [{ id: 'ctr_1', status: 'activated' }], + crm_account: [{ id: 'acc_1', type: 'prospect' }], + }, + writes: [ + { object: 'crm_contract', id: 'ctr_1', doc: { signed_date: today } }, + { object: 'crm_account', id: 'acc_1', doc: { type: 'customer' } }, + ], + }, + + 'case_status_side_effects — the account service rollup': { + hook: 'case_status_side_effects', + event: 'afterUpdate', + input: { id: 'case_1', status: 'resolved', crm_account: 'acc_1' }, + previous: { id: 'case_1', status: 'in_progress' }, + seed: { crm_account: [{ id: 'acc_1', type: 'customer' }] }, + writes: [{ object: 'crm_account', id: 'acc_1', doc: { last_activity_date: today } }], + }, + + 'quote_on_accepted — the opportunity close-out': { + hook: 'quote_on_accepted', + event: 'afterUpdate', + input: { + id: 'q_1', + status: 'accepted', + crm_account: 'acc_1', + crm_opportunity: 'opp_1', + total_price: 1000, + }, + previous: { id: 'q_1', status: 'draft' }, + seed: { crm_opportunity: [{ id: 'opp_1', stage: 'proposal' }] }, + writes: [ + { object: 'crm_opportunity', id: 'opp_1', doc: { stage: 'closed_won', close_date: today } }, + ], + }, + + 'task_activity_bubble — the activity bubble on the parent record': { + hook: 'task_activity_bubble', + event: 'afterUpdate', + input: { id: 'task_1', related_to_type: 'crm_account', related_to_account: 'acc_1' }, + previous: { id: 'task_1', status: 'in_progress' }, + seed: { crm_account: [{ id: 'acc_1' }] }, + writes: [{ object: 'crm_account', id: 'acc_1', doc: { last_activity_date: today } }], + }, +}; + +describe('every hook-side derived write reaches the engine in the engine’s own shape', () => { + it.each(Object.keys(CASES))('%s', async (label) => { + const spec = CASES[label]!; + const engine = makeSandboxEngine(spec.seed); + + await runHookBody(hook(spec.hook), { + event: spec.event, + input: { ...spec.input }, + previous: spec.previous, + user: { id: 'usr_1' }, + engine, + }); + + for (const expected of spec.writes) { + const calls = engine.callsFor(expected.object, 'update'); + expect(calls, `${spec.hook} performed no update on ${expected.object}`).toHaveLength(1); + const { args } = calls[0]!; + + // (1) Two positional arguments — a document and options. The broken + // spelling put an id string first and the document second, where the + // engine reads its OPTIONS bag; that is what produced the "does not + // recognise option" throw on every invocation. + expect(args).toHaveLength(2); + const [doc, options] = args as [Rec, Rec]; + expect(typeof doc, 'the first argument must be the document, never an id').toBe('object'); + expect(doc).not.toBeNull(); + + // (2) The target id travels INSIDE the document — that is the only place + // `ObjectQL.update` looks for it first. + expect(doc.id).toBe(expected.id); + expect(doc).toMatchObject(expected.doc); + + // (3) The row scope is explicit and agrees with the document. A `where` + // naming a different row would be silently ignored by the kernel + // (`data.id` wins), so a disagreement can only ever be a bug. + expect(options).toEqual({ where: { id: expected.id } }); + + // (4) And, because a correct shape that writes nothing is still a dead + // rollup: the stored row actually moved. + const row = engine.rows(expected.object).find((r) => r.id === expected.id); + expect(row, `${expected.object}/${expected.id} was not stored`).toBeDefined(); + expect(row).toMatchObject(expected.doc); + } + }); + + it('covers every `update(` call site under src/objects/', () => { + const sites = hookFiles().flatMap((f) => + [...codeOf(f).matchAll(/\.update\(/g)].map(() => f), + ); + const covered = Object.values(CASES).flatMap((c) => c.writes).length; + expect( + covered, + `src/objects/ has ${sites.length} update() call sites (${[...new Set(sites)].join(', ')}) ` + + `but this file exercises ${covered}. A derived write nothing runs is a derived write ` + + 'nobody notices is dead — add the case.', + ).toBe(sites.length); + }); +}); + +// ─────────────────────────────────────── the shape, enforced statically ── + +/** + * The runtime assertions above can only speak for the call sites they exercise. + * This one speaks for all of them, including ones added tomorrow: a first + * argument that is not an object literal is the #616 defect, whatever else the + * hook does. + */ +describe('no hook-side code may address a write by bare id', () => { + it('finds files to check (guards against a silently empty scan)', () => { + expect(hookFiles().length).toBeGreaterThan(10); + }); + + it.each(hookFiles())('%s passes a document to update()/delete(), never an id', (file) => { + const code = codeOf(file); + const offenders: string[] = []; + for (const m of code.matchAll(/\.(update|delete)\(/g)) { + const rest = code.slice(m.index! + m[0].length); + const firstChar = rest.replace(/^\s*/, '')[0]; + if (firstChar !== '{') offenders.push(`${m[1]}(${rest.slice(0, 24).trim()}…`); + } + expect( + offenders, + `${file} calls the repository facade with an id where a document/options object belongs — ` + + 'the engine reads the second positional argument as its options bag and throws ' + + '"does not recognise option" on every invocation (#616).', + ).toEqual([]); + }); +}); + +// ──────────────────────────────────────── the stand-in is not a liar ── + +/** + * The harness half of the same rule as `hook-query-predicate.test.ts`: a fake + * that accepts what the kernel rejects turns a green suite into evidence of + * nothing. `hook-harness.ts` previously implemented `update(id, doc)` — the + * declaration, not the engine — which is precisely why every rollup test passed + * while the rollups were dead. + */ +describe('the hook harness must not be more permissive than the kernel', () => { + const seeded = () => makeHarness({ crm_opportunity: [{ id: 'opp_1', amount: 1 }] }); + + it('rejects update(id, doc) instead of quietly doing what the caller meant', async () => { + const h = seeded(); + await expect( + (h.api.object('crm_opportunity') as AnyRec).update('opp_1', { amount: 2450 }), + ).rejects.toThrow(/DOCUMENT/); + expect(h.rows('crm_opportunity')[0]!.amount, 'a rejected write must not land').toBe(1); + }); + + it('rejects a document with no id — the kernel would have no row to resolve', async () => { + const h = seeded(); + await expect( + (h.api.object('crm_opportunity') as AnyRec).update({ amount: 2450 }, { where: { id: 'opp_1' } }), + ).rejects.toThrow(/carries no `id`/); + }); + + it('rejects a missing row scope', async () => { + const h = seeded(); + await expect( + (h.api.object('crm_opportunity') as AnyRec).update({ id: 'opp_1', amount: 2450 }), + ).rejects.toThrow(/where/); + }); + + it('rejects a scope that disagrees with the document', async () => { + const h = seeded(); + await expect( + h.api.object('crm_opportunity').update({ id: 'opp_1', amount: 2450 }, { where: { id: 'opp_2' } }), + ).rejects.toThrow(/scopes to 'opp_2'/); + }); + + it('applies the shape the hooks ship, and records both arguments', async () => { + const h = seeded(); + await h.api + .object('crm_opportunity') + .update({ id: 'opp_1', amount: 2450 }, { where: { id: 'opp_1' } }); + expect(h.rows('crm_opportunity')[0]!.amount).toBe(2450); + expect(h.callsFor('crm_opportunity', 'update')[0]!.args).toEqual([ + { id: 'opp_1', amount: 2450 }, + { where: { id: 'opp_1' } }, + ]); + }); + + it('has no `updateMany`, exactly like the surface it stands in for', () => { + expect((makeHarness().api.object('crm_opportunity') as AnyRec).updateMany).toBeUndefined(); + }); +}); diff --git a/test/hooks-runtime.test.ts b/test/hooks-runtime.test.ts index a7becc78f..0bc4fceeb 100644 --- a/test/hooks-runtime.test.ts +++ b/test/hooks-runtime.test.ts @@ -5,6 +5,7 @@ import oppLineItemHooks from '../src/objects/opportunity_line_item.hook'; import quoteLineItemHooks from '../src/objects/quote_line_item.hook'; import leadHooks from '../src/objects/lead.hook'; import opportunityHooks from '../src/objects/opportunity.hook'; +import { makeHarness } from './helpers/hook-harness'; /** * Runtime hook tests — the REAL handler code, executed against a controllable @@ -20,40 +21,23 @@ import opportunityHooks from '../src/objects/opportunity.hook'; type Rec = Record; -/** A tiny ctx.api backed by in-memory arrays, matching the HookApi surface. */ +/** + * A `ctx.api` over in-memory arrays. + * + * This used to be a SECOND, hand-rolled stand-in living in this file — one that + * accepted `filter` as a synonym for `where` and `update(id, doc)` as a synonym + * for the engine's `update(doc, options)`. Both spellings are rejected by the + * kernel, so every assertion below was being made against an API that does not + * exist: `opportunity_amount_rollup` was green here while throwing on every + * invocation in production (#616). + * + * There is now exactly one stand-in — `test/helpers/hook-harness.ts` — and it + * refuses what the kernel refuses. A fake that is more permissive than the real + * thing cannot prove anything about the real thing. + */ function makeApi(store: Record) { - const calls: Array<{ op: string; object: string; args: any }> = []; - const match = (row: Rec, where: Rec = {}) => - Object.entries(where).every(([k, v]) => row[k] === v); - const api = { - calls, - object(name: string) { - const rows = store[name] ?? (store[name] = []); - return { - async find(q: { filter?: Rec; where?: Rec } = {}) { - return rows.filter((r) => match(r, q.filter ?? q.where ?? {})); - }, - async findOne(q: { filter?: Rec; where?: Rec } = {}) { - return rows.find((r) => match(r, q.filter ?? q.where ?? {})) ?? null; - }, - async count(q: { filter?: Rec; where?: Rec } = {}) { - return rows.filter((r) => match(r, q.filter ?? q.where ?? {})).length; - }, - async update(id: string, doc: Rec) { - calls.push({ op: 'update', object: name, args: { id, doc } }); - const row = rows.find((r) => r.id === id); - if (row) Object.assign(row, doc); - return row; - }, - async insert(doc: Rec) { - calls.push({ op: 'insert', object: name, args: doc }); - rows.push(doc); - return doc; - }, - }; - }, - }; - return api; + const h = makeHarness(store); + return { ...h.api, calls: h.calls }; } const hookByName = (hooks: any, name: string) => {