From 718f226761520f8cb60c2a1eea5a3730252b03fc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 09:53:06 +0000 Subject: [PATCH] fix(hooks): query by `where`, not `filter`, on ctx.api reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eighteen `ctx.api` calls across eight `*.hook.ts` files passed their predicate as `filter:`. The ObjectQL kernel does not accept that key on every read path, and drops it without raising: - `find` normalizes `filter` -> `where`, so those calls worked by luck. - `findOne` spreads the query into the AST (`{ ...query, limit: 1 }`) and never aliases, so the predicate vanished and the call returned the object's FIRST ROW — for any argument, including one matching nothing. It never returned null and never threw. - `count` reads `query.where` explicitly, so the predicate vanished and the call counted the ENTIRE object. Verified against a real ObjectQL 16.1.0 engine, not the test harness. Impact of the mis-reads: - opportunity/quote line items defaulted `list_price` / `unit_price` from the first product in the catalog instead of the chosen one — wrong money on the record. - quote acceptance, won-opportunity and contract-activation evaluated their "already in the target state?" gate against an unrelated record while writing to the correct one. - case escalation assigned the follow-up task to the first account's owner. - account and product delete guards counted every row in the object, so they blocked deletes with no real references and reported a made-up count in the error message. - campaign ROI snapshots stored whole-table opportunity and lead counts as campaign attribution. `HookQuery` no longer declares `filter?`, so this fails at compile time now rather than in the data. `src/actions/lead.actions.ts` is normalized too — behaviour-identical there (it is a `find`), but it was the seed of the next copy-paste. The suite could not have caught this: `test/helpers/hook-harness.ts` resolved its predicate as `q.filter ?? q.where ?? {}`, making the stand-in more permissive than the kernel it stands in for, so every affected hook tested green. The harness now throws on `filter`, and the new `test/hook-query-predicate.test.ts` pins the contract against a real in-memory kernel — including the kernel's silent-drop behaviour, so the trap stays documented — plus a source scan that fails any `*.hook.ts` reintroducing the key. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01R7h5XeGgNPBqNuV6LCRWpg --- .changeset/hook-query-where-not-filter.md | 38 +++++ src/actions/lead.actions.ts | 2 +- src/objects/_hook-api.ts | 22 ++- src/objects/account.hook.ts | 2 +- src/objects/campaign.hook.ts | 10 +- src/objects/case.hook.ts | 2 +- src/objects/contract.hook.ts | 2 +- src/objects/opportunity.hook.ts | 2 +- src/objects/opportunity_line_item.hook.ts | 6 +- src/objects/product.hook.ts | 4 +- src/objects/quote.hook.ts | 2 +- src/objects/quote_line_item.hook.ts | 6 +- test/helpers/hook-harness.ts | 21 ++- test/hook-query-predicate.test.ts | 165 ++++++++++++++++++++++ 14 files changed, 262 insertions(+), 22 deletions(-) create mode 100644 .changeset/hook-query-where-not-filter.md create mode 100644 test/hook-query-predicate.test.ts diff --git a/.changeset/hook-query-where-not-filter.md b/.changeset/hook-query-where-not-filter.md new file mode 100644 index 00000000..8f153d1a --- /dev/null +++ b/.changeset/hook-query-where-not-filter.md @@ -0,0 +1,38 @@ +--- +'hotcrm': patch +--- + +Stop hook reads from silently querying the wrong record. Eighteen `ctx.api` +calls across eight `*.hook.ts` files passed their predicate as `filter:`, a key +the ObjectQL kernel does not accept on every read path — and drops without +raising: + +- `find` normalizes `filter` → `where`, so those calls were correct by luck. +- `findOne` spreads the query into the AST (`{ ...query, limit: 1 }`) and never + aliases, so the predicate vanished and the call returned **the object's first + row** — for any argument, including one matching nothing. It never returned + `null` and never threw. +- `count` reads `query.where` explicitly, so the predicate vanished and the call + counted **the entire object**. + +Confirmed against a real ObjectQL 16.1.0 engine, not the test harness. The +mis-reads were load-bearing: line-item pricing defaulted `list_price` / +`unit_price` from the first product in the catalog rather than the chosen one; +quote acceptance and won-opportunity / contract-activation account promotion +evaluated their "already in the target state?" gate against an unrelated record +while writing to the correct one; case escalation assigned the follow-up task to +the first account's owner; the account and product delete guards counted every +row in the object, so they blocked deletes that had no real references and +reported an invented number; and campaign ROI snapshots recorded whole-table +opportunity and lead counts as campaign attribution. + +`HookQuery` no longer declares `filter?`, so the compiler now rejects the +spelling at authoring time instead of leaving it to be discovered in the data. + +The suite could not have caught this: `test/helpers/hook-harness.ts` resolved +its predicate as `q.filter ?? q.where ?? {}`, making the stand-in more +permissive than the kernel it stands in for, so every affected hook tested +green. The harness now throws on `filter`, and `test/hook-query-predicate.test.ts` +pins the contract against a real in-memory kernel — including the kernel's +silent-drop behaviour, so the trap stays documented — plus a source scan that +fails any `*.hook.ts` reintroducing the key. diff --git a/src/actions/lead.actions.ts b/src/actions/lead.actions.ts index 0f6754ca..60af563a 100644 --- a/src/actions/lead.actions.ts +++ b/src/actions/lead.actions.ts @@ -115,7 +115,7 @@ export const CreateCampaignAction: Action = { // duplicates are skipped, and re-running the action on the same // selection must not double-count marketing touches. const raw = await ctx.api.object('crm_campaign_member').find({ - filter: { crm_campaign: campaignId }, + where: { crm_campaign: campaignId }, fields: ['crm_lead'], top: 5000, }); diff --git a/src/objects/_hook-api.ts b/src/objects/_hook-api.ts index 5ab84cb0..9832401c 100644 --- a/src/objects/_hook-api.ts +++ b/src/objects/_hook-api.ts @@ -16,9 +16,27 @@ type Doc = Record; -/** Query options accepted by read operations. Drivers accept `filter` or `where`. */ +/** + * Query options accepted by read operations. + * + * The predicate key is `where` — and ONLY `where`. An earlier version of this + * type also allowed `filter`, on the belief that "drivers accept either". They + * do not, and the mismatch fails SILENTLY: + * + * - `find` normalizes `filter` → `where`, so it happens to work. + * - `findOne` spreads the query straight into the AST (`{...query, limit: 1}`) + * and never aliases. An unknown `filter` key is dropped by the driver, so + * the query degrades to "first row of the object" — no error, no null, just + * the wrong record. + * - `count` reads `query.where` explicitly, so `filter` is dropped and the + * call counts the WHOLE object. + * + * `filter` is therefore deliberately absent: a hook that reaches for it must + * fail at compile time rather than silently compute against a stranger's row. + * (See `test/hook-query-predicate.test.ts`, which pins this against the real + * kernel rather than the test harness.) + */ export interface HookQuery { - filter?: Doc; where?: Doc; fields?: string[]; top?: number; diff --git a/src/objects/account.hook.ts b/src/objects/account.hook.ts index 55ff28ad..bb7b8623 100644 --- a/src/objects/account.hook.ts +++ b/src/objects/account.hook.ts @@ -51,7 +51,7 @@ const accountHook: Hook = { const api = ctx.api as HookApi | undefined; if (!api) return; const openOpps = await api.object('crm_opportunity').count({ - filter: { + where: { crm_account: previous.id, stage: { $nin: ['closed_won', 'closed_lost'] }, }, diff --git a/src/objects/campaign.hook.ts b/src/objects/campaign.hook.ts index b6b1e1fd..2a6c48aa 100644 --- a/src/objects/campaign.hook.ts +++ b/src/objects/campaign.hook.ts @@ -66,14 +66,14 @@ const campaignCompleted: Hook = { // metric snapshot was silently zero. const [memberRows, opportunities, wonOpps, wonOppRecords] = await Promise.all([ api.object('crm_campaign_member').find({ - filter: { crm_campaign: id }, + where: { crm_campaign: id }, fields: ['crm_lead', 'status'], top: 5000, }), - api.object('crm_opportunity').count({ filter: { crm_campaign: id } }), - api.object('crm_opportunity').count({ filter: { crm_campaign: id, stage: 'closed_won' } }), + api.object('crm_opportunity').count({ where: { crm_campaign: id } }), + api.object('crm_opportunity').count({ where: { crm_campaign: id, stage: 'closed_won' } }), api.object('crm_opportunity').find({ - filter: { crm_campaign: id, stage: 'closed_won' }, + where: { crm_campaign: id, stage: 'closed_won' }, fields: ['amount'], top: 5000, }), @@ -90,7 +90,7 @@ const campaignCompleted: Hook = { ); const convertedLeads = leadIds.length > 0 - ? await api.object('crm_lead').count({ filter: { id: { $in: leadIds }, is_converted: true } }) + ? await api.object('crm_lead').count({ where: { id: { $in: leadIds }, is_converted: true } }) : 0; const actualRevenue = wonOppRecords.reduce((sum, row) => { diff --git a/src/objects/case.hook.ts b/src/objects/case.hook.ts index 527d9602..6fb0f70f 100644 --- a/src/objects/case.hook.ts +++ b/src/objects/case.hook.ts @@ -111,7 +111,7 @@ const caseSideEffects: Hook = { // Escalation: open task for account owner if (input.status === 'escalated' && previous.status !== 'escalated' && accountId) { - const account = await api.object('crm_account').findOne({ filter: { id: accountId } }); + const account = await api.object('crm_account').findOne({ where: { id: accountId } }); const ownerId = (account as { owner?: string } | null)?.owner ?? ctx.user?.id; const due = new Date(); due.setDate(due.getDate() + 1); diff --git a/src/objects/contract.hook.ts b/src/objects/contract.hook.ts index 122ea31e..78f7c83d 100644 --- a/src/objects/contract.hook.ts +++ b/src/objects/contract.hook.ts @@ -106,7 +106,7 @@ const contractActivation: Hook = { } if (accountId) { - const account = await api.object('crm_account').findOne({ filter: { id: 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' }); } diff --git a/src/objects/opportunity.hook.ts b/src/objects/opportunity.hook.ts index 71b7a53d..863577d0 100644 --- a/src/objects/opportunity.hook.ts +++ b/src/objects/opportunity.hook.ts @@ -166,7 +166,7 @@ const opportunityWonHook: Hook = { undefined; if (!accountId) return; - const account = await api.object('crm_account').findOne({ filter: { id: 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' }); } diff --git a/src/objects/opportunity_line_item.hook.ts b/src/objects/opportunity_line_item.hook.ts index 390efa3b..c729d053 100644 --- a/src/objects/opportunity_line_item.hook.ts +++ b/src/objects/opportunity_line_item.hook.ts @@ -53,7 +53,7 @@ const opportunityLineItemPriceFill: Hook = { const productId = typeof input.crm_product === 'string' ? input.crm_product : undefined; if (!productId) return; const product = await api.object('crm_product').findOne({ - filter: { id: productId }, fields: ['id', 'list_price'], + where: { id: productId }, fields: ['id', 'list_price'], }); const listPrice = product && typeof product.list_price === 'number' ? product.list_price : undefined; if (listPrice === undefined) return; @@ -95,13 +95,13 @@ const opportunityAmountRollup: Hook = { // editing a line item would silently rewrite a closed-won deal's recorded // amount. A closed deal's value is settled; leave it. const opp = await api.object('crm_opportunity').findOne({ - filter: { id: oppId }, fields: ['id', 'stage'], + where: { id: oppId }, fields: ['id', 'stage'], }); const stage = opp && typeof opp.stage === 'string' ? opp.stage : undefined; if (stage === 'closed_won' || stage === 'closed_lost') continue; const lines = await api.object('crm_opportunity_line_item').find({ - filter: { crm_opportunity: oppId }, + where: { crm_opportunity: oppId }, fields: ['quantity', 'unit_price', 'discount'], top: 5000, }); diff --git a/src/objects/product.hook.ts b/src/objects/product.hook.ts index 6486672b..670f8e94 100644 --- a/src/objects/product.hook.ts +++ b/src/objects/product.hook.ts @@ -51,8 +51,8 @@ const productHook: Hook = { const id = previous?.id; if (!api || !id) return; const [oppRefs, quoteRefs] = await Promise.all([ - api.object('crm_opportunity_line_item').count({ filter: { crm_product: id } }).catch(() => 0), - api.object('crm_quote_line_item').count({ filter: { crm_product: id } }).catch(() => 0), + api.object('crm_opportunity_line_item').count({ where: { crm_product: id } }).catch(() => 0), + api.object('crm_quote_line_item').count({ where: { crm_product: id } }).catch(() => 0), ]); const total = oppRefs + quoteRefs; if (total > 0) { diff --git a/src/objects/quote.hook.ts b/src/objects/quote.hook.ts index 54794719..8a14c105 100644 --- a/src/objects/quote.hook.ts +++ b/src/objects/quote.hook.ts @@ -128,7 +128,7 @@ const quoteAccepted: Hook = { }); if (opportunityId) { - const opp = await api.object('crm_opportunity').findOne({ filter: { id: 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', diff --git a/src/objects/quote_line_item.hook.ts b/src/objects/quote_line_item.hook.ts index cdebf254..ee63aded 100644 --- a/src/objects/quote_line_item.hook.ts +++ b/src/objects/quote_line_item.hook.ts @@ -50,7 +50,7 @@ const quoteLineItemPriceFill: Hook = { const productId = typeof input.crm_product === 'string' ? input.crm_product : undefined; if (!productId) return; const product = await api.object('crm_product').findOne({ - filter: { id: productId }, fields: ['id', 'list_price'], + where: { id: productId }, fields: ['id', 'list_price'], }); const listPrice = product && typeof product.list_price === 'number' ? product.list_price : undefined; if (listPrice === undefined) return; @@ -83,7 +83,7 @@ const quoteTotalRollup: Hook = { for (const quoteId of quoteIds) { const quote = await api.object('crm_quote').findOne({ - filter: { id: quoteId }, + where: { id: quoteId }, fields: ['id', 'status', 'discount', 'tax', 'shipping_handling'], }); if (!quote) continue; @@ -91,7 +91,7 @@ const quoteTotalRollup: Hook = { if (status === 'accepted' || status === 'expired') continue; const lines = await api.object('crm_quote_line_item').find({ - filter: { crm_quote: quoteId }, + where: { crm_quote: quoteId }, fields: ['quantity', 'unit_price', 'discount'], top: 5000, }); diff --git a/test/helpers/hook-harness.ts b/test/helpers/hook-harness.ts index a80ffb55..dbd80032 100644 --- a/test/helpers/hook-harness.ts +++ b/test/helpers/hook-harness.ts @@ -84,7 +84,26 @@ export function makeHarness(store: Record = {}): Harness { const calls: RecordedCall[] = []; let seq = 0; const rows = (object: string): Rec[] => (store[object] ??= []); - const whereOf = (q: HookQuery = {}): Rec => (q.filter ?? q.where ?? {}) as Rec; + /** + * Extract the predicate — from `where`, and ONLY from `where`. + * + * This used to read `q.filter ?? q.where ?? {}`, which made the harness + * MORE permissive than the kernel and turned it into a liar: the real + * `findOne` ignores an unknown `filter` key and returns the object's first + * row, and the real `count` ignores it and counts everything, but every test + * here went green because the harness quietly honoured both spellings. A + * fake replacement that accepts inputs the real thing rejects cannot prove + * anything — so `filter` is now a loud failure, not a silent alias. + */ + const whereOf = (q: HookQuery = {}): Rec => { + if ('filter' in (q as Rec)) { + throw new Error( + "hook-harness: query key \"filter\" is not a predicate — the kernel silently " + + 'ignores it on findOne/count and reads the wrong record. Use "where".', + ); + } + return (q.where ?? {}) as Rec; + }; const api: HookApi = { object(name: string) { diff --git a/test/hook-query-predicate.test.ts b/test/hook-query-predicate.test.ts new file mode 100644 index 00000000..1404ba48 --- /dev/null +++ b/test/hook-query-predicate.test.ts @@ -0,0 +1,165 @@ +// 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 { makeHarness } from './helpers/hook-harness'; + +/** + * The predicate key on `ctx.api` reads is `where`. This file is the proof. + * + * Every other hook test in this repo runs against `test/helpers/hook-harness.ts` + * — a hand-written stand-in. A stand-in can only disprove what it models, and + * this one used to accept `filter` as a synonym for `where`, so a whole class + * of "reads the wrong record" bugs was invisible: the suite was green while + * eight hooks were querying by a key the kernel drops on the floor. + * + * So these tests do NOT use the harness for the kernel claims. They stand up a + * real ObjectQL engine on the in-memory driver and query it through a real + * `ScopedContext` — which is literally the object the kernel injects as + * `ctx.api` (see `ObjectQL.buildHookApi`). What passes here is what happens in + * production. + */ + +// Persistence off: the memory driver defaults to writing +// `.objectstack/data/memory-driver.json` and would otherwise carry rows over +// between runs, so "the first row" would mean a different row each time. +// +// The schema is deliberately minimal — this exercises the query layer, not the +// field system. `ServiceObject`'s field type demands every resolved property +// (`required`, `searchable`, …) that the registry fills in at runtime, so the +// literal is cast rather than padded with a dozen irrelevant defaults. +const makeEngine = () => + ObjectQL.create({ + datasources: { default: new InMemoryDriver({ persistence: false }) }, + objects: { + crm_account: { + name: 'crm_account', + fields: { + id: { type: 'text' }, + name: { type: 'text' }, + annual_revenue: { type: 'number' }, + }, + }, + } as never, + }); + +describe('ctx.api query predicate — against the real kernel', () => { + let ql: Awaited>; + let api: ReturnType; + let first: Record; + let second: Record; + + beforeAll(async () => { + ql = await makeEngine(); + api = ql.createContext({ isSystem: true }); + // Two rows, deliberately in insertion order: "the first row" is a + // meaningful, distinguishable answer only if there is a second one. + first = await api.object('crm_account').insert({ name: 'First Row', annual_revenue: 100 }); + second = await api.object('crm_account').insert({ name: 'Second Row', annual_revenue: 200 }); + }); + + afterAll(async () => { + await ql?.close(); + }); + + describe('where: — the supported spelling', () => { + it('findOne returns the record actually asked for', async () => { + const hit = await api.object('crm_account').findOne({ where: { id: second.id } }); + expect(hit?.id).toBe(second.id); + expect(hit?.name).toBe('Second Row'); + }); + + it('findOne returns null when nothing matches', async () => { + const hit = await api.object('crm_account').findOne({ where: { id: 'no_such_id' } }); + expect(hit).toBeNull(); + }); + + it('count counts only the matching rows', async () => { + expect(await api.object('crm_account').count({ where: { annual_revenue: 100 } })).toBe(1); + }); + + it('find returns only the matching rows', async () => { + const rows = await api.object('crm_account').find({ where: { annual_revenue: 200 } }); + expect(rows.map((r: any) => r.name)).toEqual(['Second Row']); + }); + }); + + /** + * Characterisation of the kernel as it ships in @objectstack/objectql 16.1.0. + * These are NOT the behaviour we want — they are the trap this change exists + * to close, pinned so nobody "helpfully" reintroduces `filter` believing the + * two spellings are interchangeable. + * + * If one of these starts failing, the platform has fixed the underlying + * silent-drop (reported separately to the @objectstack maintainers — it is + * framework behaviour, not ours to patch here). That is good news: confirm + * against the release notes, then relax the assertion. Do not just delete it, + * and do not read a kernel fix as licence to go back to `filter` — `where` is + * the spelling every read path has always honoured. + */ + describe('filter: — the unsupported spelling the kernel drops silently', () => { + it('findOne ignores the predicate and yields the first row', async () => { + const hit = await api.object('crm_account').findOne({ filter: { id: second.id } } as any); + expect(hit?.id).not.toBe(second.id); + expect(hit?.id).toBe(first.id); + }); + + it('findOne yields the first row even when the predicate matches nothing', async () => { + // The most dangerous shape: a caller that checks for null gets a record. + const hit = await api.object('crm_account').findOne({ where: undefined, filter: { id: 'no_such_id' } } as any); + expect(hit).not.toBeNull(); + expect(hit?.id).toBe(first.id); + }); + + it('count ignores the predicate and counts the whole object', async () => { + const n = await api.object('crm_account').count({ filter: { annual_revenue: 100 } } as any); + expect(n).toBe(2); // 1 row actually matches + }); + + it('never throws — which is exactly why this was invisible', async () => { + await expect( + api.object('crm_account').findOne({ filter: { id: 'no_such_id' } } as any), + ).resolves.toBeDefined(); + }); + }); +}); + +describe('no hook may query by `filter`', () => { + const hookDir = join(process.cwd(), 'src', 'objects'); + const hookFiles = readdirSync(hookDir).filter((f) => f.endsWith('.hook.ts')); + + it('finds hook files to check (guards against a silently empty scan)', () => { + expect(hookFiles.length).toBeGreaterThan(10); + }); + + it.each(hookFiles)('%s uses `where:`, never `filter:`', (file) => { + const src = readFileSync(join(hookDir, file), 'utf8'); + // `.filter(` (the array method) is fine; `filter:` as an object key is not. + const offenders = src + .split('\n') + .map((line, i) => ({ line: line.trim(), no: i + 1 })) + .filter(({ line }) => /(^|[{,\s])filter\s*:/.test(line)); + expect( + offenders, + `${file} passes a \`filter:\` query key — the kernel drops it and reads the wrong record; use \`where:\``, + ).toEqual([]); + }); +}); + +describe('the hook harness must not be more permissive than the kernel', () => { + it('rejects `filter` instead of quietly treating it as `where`', async () => { + const h = makeHarness({ crm_account: [{ id: 'a1' }, { id: 'a2' }] }); + await expect( + h.api.object('crm_account').findOne({ filter: { id: 'a2' } } as any), + ).rejects.toThrow(/filter/); + }); + + it('still honours `where`', async () => { + const h = makeHarness({ crm_account: [{ id: 'a1' }, { id: 'a2' }] }); + const hit = await h.api.object('crm_account').findOne({ where: { id: 'a2' } }); + expect(hit?.id).toBe('a2'); + }); +});