fix(hooks): give ctx.api.update the engine's real (document, options) shape (#616)

.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.

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<string, unknown>;
@@ -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<number>;
   find: (q: HookQuery) => Promise<Array<Doc>>;
   findOne: (q: HookQuery) => Promise<Doc | null>;
   insert: (doc: Doc) => Promise<unknown>;
-  update: (id: string, doc: Doc) => Promise<unknown>;
-  updateMany: (q: { where: Doc; doc: Doc }) => Promise<unknown>;
-  delete: (id: string) => Promise<unknown>;
+  update: (doc: HookUpdateDoc, options: HookUpdateOptions) => Promise<unknown>;
+  delete: (options: HookDeleteOptions) => Promise<unknown>;
 }
 
 export interface HookApi {

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 } },
+    );
   },
 };
 

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 } },
+        );
       }
     }
   },

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 } },
+        );
       }
     }
 

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;

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 } },
+        );
       }
     }
   },

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 } },
+        );
       }
     }
   },

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 } },
+      );
     }
   },
 };

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).

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<string, any>;
 
-/** 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<string, Rec[]> = {}): 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;
     },
   };