From 0c510fd5538b2aa76b7558c574b3e562420b22a8 Mon Sep 17 00:00:00 2001 From: os-warren Date: Tue, 1 Sep 2026 10:10:58 +0000 Subject: [PATCH] Pass the engine facade a bare filter, not a query envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ctx.engine.find(object, query)` takes a FILTER: the runtime's `buildActionEngineFacade` adds the `where` envelope itself. Every filtered read in `catalog.handlers.ts` passed an envelope, so through the real dispatcher the query arrived as `{ where: { where: … } }`, matched nothing, and came back empty with no error — `duly_catalog_apply` reported a successful run of zero, `duly_catalog_sync` scanned nothing, and `resolveBusinessUnit` silently created unanchored duties. - flatten all five `engine.find` calls - `FakeEngine.find` in catalog-instantiate.test.ts reads the flat filter, so the fake stops honouring a shape production never produces - delete the #79 tripwire in catalog-apply-cadence.test.ts and fold its two facades into one - new test/catalog-engine-facade.test.ts dispatches through the real action route with the facade the RUNTIME builds — no double Fixes #79. Contract half filed upstream as objectstack-ai/objectstack#14175. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p --- src/actions/catalog.handlers.ts | 55 ++++- test/catalog-apply-cadence.test.ts | 90 ++------ test/catalog-engine-facade.test.ts | 350 +++++++++++++++++++++++++++++ test/catalog-instantiate.test.ts | 29 ++- 4 files changed, 443 insertions(+), 81 deletions(-) create mode 100644 test/catalog-engine-facade.test.ts diff --git a/src/actions/catalog.handlers.ts b/src/actions/catalog.handlers.ts index 4f27697..2444ec2 100644 --- a/src/actions/catalog.handlers.ts +++ b/src/actions/catalog.handlers.ts @@ -110,6 +110,41 @@ export function resolveDutyTimezone(): string { return DEFAULT_DUTY_TIMEZONE; } +// ── The engine facade's query shape ─────────────────────────────────── +// +// `ctx.engine.find(object, query)` takes a BARE FILTER, not an ObjectQL query +// envelope. The runtime builds the envelope itself — `buildActionEngineFacade` +// in @objectstack/runtime 17.2.0, read verbatim from its `dist/index.js`: +// +// async find(object, query) { +// const where = query && Object.keys(query).length ? { where: query } : {}; +// const rows = await ql.find(object, { ...where, context }); +// +// So every read in this file passes `{ field: value }`, never +// `{ where: { field: value } }`. Handing it an envelope produces +// `{ where: { where: { … } } }`; no row has a field called `where`, so the read +// comes back EMPTY WITH NO ERROR. That is the failure this file shipped with: +// `duly_catalog_apply` reported a successful run of zero, `duly_catalog_sync` +// scanned nothing and called every duty unchanged, and `resolveBusinessUnit` +// anchored no duty at all — silently, because "no position row" is a legitimate +// day-one state. The ONE unfiltered read survived, because +// `Object.keys({}).length === 0` skips the wrapping entirely, which is exactly +// what made the handler look partially alive. +// +// `ActionEngineFacade.find` in @objectstack/spec types `query` as a plain +// record of string to unknown and says nothing about which of the two shapes it +// is — the runtime's implementation is the only thing that decides, and this +// app read it the other way. Filed upstream as +// **objectstack-ai/objectstack#14175** so the shape is DECLARED rather than +// discovered; until that lands this comment is the contract. +// +// ⛔ Do NOT add a tolerant `query.where ?? query` rung — not here, not in a +// test double. A consumer that accepts both shapes is precisely what let the +// wrong one ship green: `test/catalog-instantiate.test.ts`'s fake honoured the +// envelope, so 78 assertions passed against a shape production never produces. +// The test that can see this is one that dispatches through the REAL route and +// lets the runtime build its own facade — `test/catalog-engine-facade.test.ts`. + // ── Shapes ────────────────────────────────────────────────────────────────── export interface CatalogApplyParams extends Record { @@ -256,12 +291,21 @@ export function pairKey(catalogItem: unknown, owner: unknown): string { * exists on the platform. It is NOT read here — the issue names the * assignment-level anchor, and adding a fallback rung is a product decision, * reported rather than taken.) + * + * ⚠️ That tolerance is why this read's query shape matters more than the other + * three. The other reads fail into a visibly empty report — zero items, zero + * scanned — but this one fails into a state the handler is WRITTEN to accept: + * an envelope-shaped filter returned nothing, "nothing" reads as "not yet + * modelled", and every duty was created unanchored with no error anywhere. The + * rollups that the business unit exists to feed were simply empty. See the + * facade-shape note above; the end-to-end coverage is in + * `test/catalog-engine-facade.test.ts`. */ async function resolveBusinessUnit( engine: ActionEngineFacade, userId: string, ): Promise { - const rows = await engine.find('sys_user_position', { where: { user_id: userId } }); + const rows = await engine.find('sys_user_position', { user_id: userId }); for (const row of rows) { // A person can hold several positions; take the first anchored one. // Unanchored rows (`null`) are legacy/tenant-wide and carry no depth. @@ -282,13 +326,14 @@ export const applyCatalogHandler: ActionHandler = async (ctx // stopped asking for; handing it to a new hire on their first day is the // opposite of what deactivating it meant. const items = await engine.find('duly_catalog_item', { - where: { position_code: positionCode, active: true }, + position_code: positionCode, + active: true, }); const activeItems = items.filter((item) => item?.active !== false); // One probe for the whole run, not one per (item, user). The pair set is // what makes a second apply create nothing. - const existing = await engine.find('duly_duty', { where: { owner: { $in: users } } }); + const existing = await engine.find('duly_duty', { owner: { $in: users } }); const taken = new Set(); for (const duty of existing) { // Any duty already pointing at this catalog item for this person counts — @@ -384,7 +429,7 @@ export const syncCatalogHandler: ActionHandler = async (ctx) // the retired report is made of, so it has to come back from this read. const items = await engine.find( 'duly_catalog_item', - positionCode ? { where: { position_code: positionCode } } : {}, + positionCode ? { position_code: positionCode } : {}, ); const byId = new Map>(); for (const item of items) { @@ -392,7 +437,7 @@ export const syncCatalogHandler: ActionHandler = async (ctx) byId.set(recordId(item), item); } - const duties = await engine.find('duly_duty', { where: { source: 'catalog' } }); + const duties = await engine.find('duly_duty', { source: 'catalog' }); const changes: CatalogSyncChange[] = []; const retired: CatalogSyncRetired[] = []; diff --git a/test/catalog-apply-cadence.test.ts b/test/catalog-apply-cadence.test.ts index 943bcd5..11699c7 100644 --- a/test/catalog-apply-cadence.test.ts +++ b/test/catalog-apply-cadence.test.ts @@ -23,7 +23,7 @@ import type { CatalogApplyResult } from '../src/actions/catalog.handlers.js'; * apply path unprotected is half a fix. * * ── Why a REAL booted engine and not `catalog-instantiate.test.ts`'s fake ── - * That suite's `FakeEngine` is a Map with a `where` matcher: it runs no + * That suite's `FakeEngine` is a Map with a filter matcher: it runs no * validation rules and stamps no defaults, so every claim below would pass on * it for the wrong reason. Validation and `applyFieldDefaults` are precisely * what is under test here, so the handler is dispatched through the app's own @@ -60,7 +60,7 @@ afterAll(async () => { await kernel?.shutdown?.(); }); -// ── The two engine facades ────────────────────────────────────────────────── +// ── The engine facade ─────────────────────────────────────────────────────── interface Facade { insert(object: string, values: AnyRow): Promise<{ id: string }>; @@ -70,15 +70,29 @@ interface Facade { } /** - * The facade `applyCatalogHandler` is WRITTEN against: `find(object, query)` - * takes ObjectQL's own query envelope, `where` and all. It is the convention - * `catalog-instantiate.test.ts`'s `FakeEngine` honours too, so this is the - * shape every existing assertion about the handler is made under. + * The facade the RUNTIME builds, reproduced line for line — + * `buildActionEngineFacade` in @objectstack/runtime 17.2.0: + * + * async find(object, query) { + * const where = query && Object.keys(query).length ? { where: query } : {}; + * const rows = await ql.find(object, { ...where, context }); + * + * `find(object, query)` therefore takes a BARE FILTER and wraps it here. This + * file used to carry TWO facades — this one honouring the handler's own + * `where` envelope, and a second reproducing the runtime — with a tripwire + * pinning the gap between them (#79). The handler now passes flat filters, so + * there is one convention and one facade. + * + * It is still a double, which is all it can be: the second test below has to + * put a row in front of the handler that `duly_catalog_item`'s own rules + * refuse, and only a hand-supplied facade can do that. What a double cannot do + * is prove the wire shape is right — it encodes the author's belief about it. + * That proof lives in `test/catalog-engine-facade.test.ts`, which dispatches + * through the real action route and lets the runtime build its own facade. * * `catalogItems`, when given, replaces the catalog read with rows handed * straight to the handler (unfiltered — the handler re-applies its own - * `active` filter). That is how a row the object's own rules now REFUSE can - * still be put in front of the handler, which one test below needs. + * `active` filter). */ function handlerFacade(catalogItems?: AnyRow[]): Facade { return { @@ -94,28 +108,6 @@ function handlerFacade(catalogItems?: AnyRow[]): Facade { }, find: async (object, query) => { if (catalogItems && object === 'duly_catalog_item') return catalogItems.map((r) => ({ ...r })); - return data.find(object, query); - }, - }; -} - -/** - * The facade the RUNTIME actually builds — `buildActionEngineFacade` in - * @objectstack/runtime 17.2.0, reproduced line for line: - * - * async find(object, query) { - * const where = query && Object.keys(query).length ? { where: query } : {}; - * const rows = await ql.find(object, { ...where, context }); - * ... - * - * It wraps whatever it is handed in a `where` of its own. Used by exactly one - * test, the tripwire at the bottom. - */ -function runtimeFacade(): Facade { - const base = handlerFacade(); - return { - ...base, - find: async (object, query) => { const where = query && Object.keys(query).length ? { where: query } : {}; return data.find(object, { ...where }); }, @@ -227,41 +219,3 @@ describe('duly_catalog_apply — the cadence it replicates (#65)', () => { for (const field of CADENCE_FIELDS) expect(duty?.[field] ?? null, field).toBeNull(); }); }); - -// ─────────────────────────────────────────────────────────────────────────── -// A tripwire on a filed defect — NOT an assertion that this is correct -// ─────────────────────────────────────────────────────────────────────────── -describe('the handler\'s query shape does not survive the runtime\'s own facade', () => { - /** - * Measured while covering the apply path for #65 and filed as #79 — a - * different defect from the missing validation rule, and not fixed here. - * - * `applyCatalogHandler` calls `engine.find('duly_catalog_item', { where: … - * })`. The runtime's `buildActionEngineFacade` wraps whatever it is given: - * `ql.find(object, { where: query })`. So through the real dispatcher the - * handler's own `where` becomes `{ where: { where: … } }`, no row has a - * field called `where`, and the read comes back EMPTY — with no error. The - * action then reports `{ created: 0 }` and a successful run. - * - * Pinned so the seam is visible rather than folklore. When #79 is fixed - * this goes red: delete this describe block — do not adjust it — and - * `handlerFacade` above becomes the only convention in the file. - */ - it('finds nothing, creates nothing, and reports success', async () => { - const position = 'apply_runtime_facade'; - await insertItem({ position_code: position, form: 'recurring', frequency: 'weekly' }); - - // Same item, same params, the only difference being which facade. - const viaHandlerConvention = await apply(handlerFacade(), { - position_code: position, - users: ['u_f1'], - }); - expect(viaHandlerConvention.catalog_items).toBe(1); - expect(viaHandlerConvention.created).toBe(1); - - const viaRuntime = await apply(runtimeFacade(), { position_code: position, users: ['u_f2'] }); - expect(viaRuntime.catalog_items).toBe(0); - expect(viaRuntime.created).toBe(0); - expect(await dutiesOf('u_f2')).toEqual([]); - }); -}); diff --git a/test/catalog-engine-facade.test.ts b/test/catalog-engine-facade.test.ts new file mode 100644 index 0000000..355fb9a --- /dev/null +++ b/test/catalog-engine-facade.test.ts @@ -0,0 +1,350 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { AppPlugin, HttpDispatcher, ObjectKernel, createStandaloneStack } from '@objectstack/runtime'; +import type { HttpDispatcherResult, HttpProtocolContext } from '@objectstack/runtime'; + +import stack from '../objectstack.config.js'; +import { registerDulyActionHandlers } from '../src/actions/register-handlers.js'; +import { + CATALOG_APPLY_ACTION, + CATALOG_APPLY_TO_PEOPLE_ACTION, + CATALOG_ITEM_OBJECT, + CATALOG_SYNC_ACTION, +} from '../src/actions/catalog.handlers.js'; +import type { CatalogApplyResult, CatalogSyncResult } from '../src/actions/catalog.handlers.js'; + +/** + * #79 — the catalog actions driven through the REAL action route, with the + * engine facade the RUNTIME builds. + * + * ── Why this suite exists at all ────────────────────────────────────────── + * `duly_catalog_apply` shipped reporting a successful run of zero. Every + * `engine.find` in `catalog.handlers.ts` passed an ObjectQL query ENVELOPE + * (`{ where: { … } }`), while `buildActionEngineFacade` (@objectstack/runtime + * 17.2.0) treats its second argument as a bare FILTER and adds the envelope + * itself — so in production the query arrived as `{ where: { where: { … } } }`, + * no row has a field called `where`, and every filtered read came back empty + * with no error. Apply created nothing, sync scanned nothing, and + * `resolveBusinessUnit` anchored nothing. + * + * Four green gates said otherwise for the life of the defect, and the reason is + * the shape of the coverage, not its quantity: `catalog-instantiate.test.ts`'s + * `FakeEngine` and `catalog-apply-cadence.test.ts`'s facade are both DOUBLES, + * and a double can only encode its author's belief about the contract. Both + * honoured the handler's convention, so 78 duty assertions passed against a + * query shape production never produced. + * + * So the rule this suite exists to enforce is: **no facade is written here.** + * The request goes to `HttpDispatcher.handleActions`, the runtime constructs + * `ctx.engine` itself, and the only shape assertion in the file is what comes + * back out of the database. If the handler ever goes back to the envelope, the + * counts below fall to zero and this suite is what says so. + * + * ── What layer this is, precisely ───────────────────────────────────────── + * `handleActions(path, …)` is the `/actions` domain handler itself — the exact + * function `createActionsDomain`'s route delegates to, with the API prefix + * already stripped (`req.path.substring(8)`), which is why the paths below + * start at `/duly_catalog_apply`. It resolves the action declaration, enforces + * `requiredPermissions`, validates params against the declared contract, and + * builds the engine facade — the whole server-side dispatch. What it does NOT + * include is the transport above it (`dispatch()`'s auth gate and scope + * resolution), which needs an `auth` service this open-edition boot has none + * of. That layer decides WHO is calling; it has nothing to do with the query + * shape, and the capability control at the bottom of this file pins that the + * gate below it is real rather than bypassed. + */ + +type AnyRow = Record; + +let kernel: any; +let data: any; +let dispatcher: HttpDispatcher; + +beforeAll(async () => { + const { plugins } = await createStandaloneStack({ + databaseDriver: 'memory', + skipSeedData: true, + // Must not resolve to a real path: a local `pnpm build` would make this + // suite report on the last BUILD instead of on `src/`. See the sibling + // suites' identical note. + artifactPath: 'dist/objectstack.this-suite-must-not-load-an-artifact.json', + }); + kernel = new ObjectKernel(); + for (const plugin of plugins) await kernel.use(plugin); + await kernel.use(new AppPlugin(stack, undefined, { skipSeedData: true })); + await kernel.bootstrap(); + data = kernel.getService('data'); + + // Through the REAL registration function, so a handler dropped from + // `registerDulyActionHandlers` surfaces here as the 404 the console gets. + registerDulyActionHandlers(data); + + dispatcher = new HttpDispatcher(kernel); +}, 180_000); + +afterAll(async () => { + await kernel?.shutdown?.(); +}); + +// ── Calling the route ─────────────────────────────────────────────────────── + +/** An authenticated admin holding both catalog capabilities. */ +const ADMIN: HttpProtocolContext = { + request: {}, + executionContext: { + userId: 'admin_1', + systemPermissions: ['duly.catalog.apply', 'duly.catalog.sync'], + }, +}; + +async function post( + path: string, + params: AnyRow, + as: HttpProtocolContext = ADMIN, +): Promise { + const result = await dispatcher.handleActions(path, 'POST', { params }, as); + return result.response; +} + +/** The success body of an action call, asserted 200 before it is read. */ +async function run(path: string, params: AnyRow): Promise { + const response = await post(path, params); + expect(response?.status, JSON.stringify(response?.body)).toBe(200); + expect(response?.body?.success).toBe(true); + return response?.body?.data as T; +} + +const dutiesOf = async (owner: string): Promise => + data.find('duly_duty', { where: { owner } }); + +let seq = 0; +const insertItem = async (over: AnyRow): Promise => + data.insert('duly_catalog_item', { + name: `Item ${++seq}`, + form: 'recurring', + frequency: 'monthly', + due_anchor: 'period_start', + due_offset_days: 5, + lead_days: 7, + grace_days: 0, + ...over, + }); + +// ──────────────────────────────────────────────────────────────────────────── + +describe('duly_catalog_apply through the runtime\'s own facade (#79)', () => { + it('creates the duties for a matching position_code', async () => { + // Acceptance 1. Before the fix this returned `catalog_items: 0, + // created: 0` with HTTP 200 — a successful run of nothing. + const position = 'facade_apply'; + const wanted = await insertItem({ position_code: position, frequency: 'quarterly' }); + // Two rows the read must NOT return, so a passing count cannot come from + // a filter-blind engine handing back the whole table. + await insertItem({ position_code: position, active: false }); + await insertItem({ position_code: 'facade_other_position' }); + + const result = await run(`/${CATALOG_APPLY_ACTION}`, { + position_code: position, + users: ['u_fa1', 'u_fa2'], + }); + + expect(result.catalog_items).toBe(1); + expect(result.users).toBe(2); + expect(result.created).toBe(2); + expect(result.skipped).toBe(0); + + for (const owner of ['u_fa1', 'u_fa2']) { + const duties = await dutiesOf(owner); + expect(duties, owner).toHaveLength(1); + expect(duties[0]?.name).toBe(wanted.name); + expect(duties[0]?.catalog_item).toBe(wanted.id); + expect(duties[0]?.frequency).toBe('quarterly'); + expect(duties[0]?.source).toBe('catalog'); + expect(duties[0]?.status).toBe('active'); + } + }); + + it('anchors the duty to the business unit of the owner\'s position', async () => { + // The read that fails SILENTLY. `resolveBusinessUnit` tolerates "no + // `sys_user_position` row" as a legitimate day-one state, so its broken + // read produced no error and no report — just duties with nothing to roll + // up to. Both people are applied in ONE run: the anchored one proves the + // read returns, the unanchored one proves the tolerated state is still + // tolerated and did not turn into a refusal. + const position = 'facade_anchor'; + await insertItem({ position_code: position }); + const unit = await data.insert('sys_business_unit', { name: 'Plant A' }); + await data.insert('sys_user_position', { user_id: 'u_anchored', business_unit_id: unit.id }); + + const result = await run(`/${CATALOG_APPLY_ACTION}`, { + position_code: position, + users: ['u_anchored', 'u_unanchored'], + }); + expect(result.created).toBe(2); + + const [anchored] = await dutiesOf('u_anchored'); + expect(anchored?.business_unit).toBe(unit.id); + + const [unanchored] = await dutiesOf('u_unanchored'); + expect(unanchored?.business_unit ?? null).toBeNull(); + }); + + it('is idempotent on (catalog_item, owner) across two dispatched runs', async () => { + // The pre-probe read of `duly_duty` is the second of the four filtered + // reads, and this is what can see it: when it came back empty, `taken` was + // empty, and a second apply would have duplicated every duty rather than + // skipping it. + const position = 'facade_idempotent'; + await insertItem({ position_code: position }); + + const first = await run(`/${CATALOG_APPLY_ACTION}`, { + position_code: position, + users: ['u_fi1'], + }); + expect(first.created).toBe(1); + + const second = await run(`/${CATALOG_APPLY_ACTION}`, { + position_code: position, + users: ['u_fi1'], + }); + expect(second.created).toBe(0); + expect(second.skipped).toBe(1); + expect(second.entries.every((e) => e.outcome === 'skipped')).toBe(true); + // Counting the report is not enough — assert nothing was written. + expect(await dutiesOf('u_fi1')).toHaveLength(1); + }); + + it('the object-bound twin reaches the same handler over its own route', async () => { + // `duly_catalog_apply_to_people` is the button an admin actually presses + // (`list_toolbar` on the catalog). Same handler function, different engine + // key — and it dispatches on `:`, so it is a genuinely + // different route resolution, not a re-run of the test above. + const position = 'facade_twin'; + await insertItem({ position_code: position }); + + const result = await run( + `/${CATALOG_ITEM_OBJECT}/${CATALOG_APPLY_TO_PEOPLE_ACTION}`, + { position_code: position, users: ['u_ft1'] }, + ); + + expect(result.action).toBe(CATALOG_APPLY_ACTION); + expect(result.catalog_items).toBe(1); + expect(result.created).toBe(1); + expect(await dutiesOf('u_ft1')).toHaveLength(1); + }); +}); + +describe('duly_catalog_sync through the runtime\'s own facade (#79)', () => { + it('scans the catalog-sourced duties and replays a cadence edit', async () => { + // Acceptance 2. Before the fix the `{ source: 'catalog' }` read came back + // empty, so a sync scanned 0 and reported every duty unchanged — which + // reads exactly like "nothing to do". + const position = 'facade_sync'; + const item = await insertItem({ position_code: position, grace_days: 0 }); + await run(`/${CATALOG_APPLY_ACTION}`, { + position_code: position, + users: ['u_fs1', 'u_fs2'], + }); + + const clean = await run(`/${CATALOG_SYNC_ACTION}`, { + position_code: position, + }); + expect(clean.scanned).toBe(2); + expect(clean.unchanged).toBe(2); + expect(clean.updated).toBe(0); + + await data.update('duly_catalog_item', { grace_days: 4 }, { where: { id: item.id } }); + + const replayed = await run(`/${CATALOG_SYNC_ACTION}`, { + position_code: position, + }); + expect(replayed.scanned).toBe(2); + expect(replayed.updated).toBe(2); + expect(replayed.changes).toHaveLength(2); + for (const change of replayed.changes) { + expect(change.fields.grace_days).toEqual({ from: 0, to: 4 }); + } + for (const owner of ['u_fs1', 'u_fs2']) { + const [duty] = await dutiesOf(owner); + expect(duty?.grace_days, owner).toBe(4); + } + }); + + it('narrowed to one position, it leaves another position\'s duties alone', async () => { + // The catalog read has TWO shapes — `{ position_code }` when narrowed and + // `{}` when not — and only the narrowed one was ever wrapped, which is why + // the unfiltered one kept working and made the handler look half-alive. + // This drives the narrowed one and pins that it narrows. + const mine = 'facade_narrow_mine'; + const theirs = 'facade_narrow_theirs'; + const mineItem = await insertItem({ position_code: mine, lead_days: 7 }); + const theirsItem = await insertItem({ position_code: theirs, lead_days: 7 }); + await run(`/${CATALOG_APPLY_ACTION}`, { + position_code: mine, + users: ['u_fn1'], + }); + await run(`/${CATALOG_APPLY_ACTION}`, { + position_code: theirs, + users: ['u_fn2'], + }); + + await data.update('duly_catalog_item', { lead_days: 1 }, { where: { id: mineItem.id } }); + await data.update('duly_catalog_item', { lead_days: 2 }, { where: { id: theirsItem.id } }); + + const result = await run(`/${CATALOG_SYNC_ACTION}`, { position_code: mine }); + expect(result.scanned).toBe(1); + expect(result.updated).toBe(1); + expect(result.changes[0]?.owner).toBe('u_fn1'); + + const [ours] = await dutiesOf('u_fn1'); + expect(ours?.lead_days).toBe(1); + // Untouched: the other position's edit is still pending its own sweep. + const [others] = await dutiesOf('u_fn2'); + expect(others?.lead_days).toBe(7); + }); + + it('org-wide, it reports a deactivated item\'s duties as retired without deleting them', async () => { + // The unfiltered (`{}`) catalog read — the one call the defect spared, + // kept here as the control that both branches of that read work. + const position = 'facade_retired'; + const item = await insertItem({ position_code: position }); + await run(`/${CATALOG_APPLY_ACTION}`, { + position_code: position, + users: ['u_fr1'], + }); + + await data.update('duly_catalog_item', { active: false }, { where: { id: item.id } }); + + const result = await run(`/${CATALOG_SYNC_ACTION}`, {}); + const retired = result.retired.filter((r) => r.owner === 'u_fr1'); + expect(retired).toHaveLength(1); + expect(retired[0]?.catalog_item).toBe(item.id); + // Reported, never deleted. + expect(await dutiesOf('u_fr1')).toHaveLength(1); + }); +}); + +// ── The control ───────────────────────────────────────────────────────────── + +describe('the route these tests use is the gated platform route', () => { + it('refuses a caller without the declared capability, and writes nothing', async () => { + // Everything above claims to run "through the real dispatcher". This is + // what makes that claim falsifiable: a hand-rolled harness that merely + // called the handler would happily run for this caller too. + const position = 'facade_ungated'; + await insertItem({ position_code: position }); + + const response = await post( + `/${CATALOG_APPLY_ACTION}`, + { position_code: position, users: ['u_denied'] }, + { request: {}, executionContext: { userId: 'nobody_1' } }, + ); + + expect(response?.status).toBe(403); + expect(String(response?.body?.error?.message ?? response?.body?.error)).toContain( + 'duly.catalog.apply', + ); + expect(await dutiesOf('u_denied')).toEqual([]); + }); +}); diff --git a/test/catalog-instantiate.test.ts b/test/catalog-instantiate.test.ts index 70087fe..0cae35b 100644 --- a/test/catalog-instantiate.test.ts +++ b/test/catalog-instantiate.test.ts @@ -24,19 +24,32 @@ import type { CatalogApplyResult, CatalogSyncResult } from '../src/actions/catal // ─── A fake engine ────────────────────────────────────────────────────────── // // `ActionEngineFacade` is four methods, so the handlers can be driven directly -// without a kernel. The fake HONOURS `where` (equality plus `$in`) rather than -// returning everything: a fake that ignored filters would make every test pass -// for the wrong reason, and would hide a handler that forgot to narrow its +// without a kernel. The fake HONOURS the filter (equality plus `$in`) rather +// than returning everything: a fake that ignored filters would make every test +// pass for the wrong reason, and would hide a handler that forgot to narrow its // read. The one test that needs an unfiltered read builds its own lenient // engine, deliberately — see "the source guard is in the code". +// +// ⚠️ `find(object, query)` reads `query` AS THE FILTER — flat, no `where` +// wrapper — because that is what the runtime's own facade does +// (`buildActionEngineFacade`, @objectstack/runtime 17.2.0: it wraps whatever it +// is handed in a `where` of its own before calling ObjectQL). This fake used to +// read `query.where`, honouring the handler's convention instead of the +// runtime's, and that is exactly how the defect below survived a green suite: +// all 78 duty assertions passed against a shape production never produced. A +// fake encodes its author's belief about the contract, so it can only ever +// confirm it — the coverage that can FALSIFY it dispatches through the real +// route and lets the runtime build the facade, in +// `test/catalog-engine-facade.test.ts`. Keep both: this suite is where the +// handler's logic is exercised cheaply, that one is where its wire shape is. interface Row extends Record { id: string; } -function matches(row: Row, where: Record | undefined): boolean { - if (!where) return true; - for (const [field, expected] of Object.entries(where)) { +function matches(row: Row, filter: Record | undefined): boolean { + if (!filter) return true; + for (const [field, expected] of Object.entries(filter)) { const actual = row[field]; if (expected !== null && typeof expected === 'object' && '$in' in (expected as object)) { const set = (expected as { $in: unknown[] }).$in; @@ -82,8 +95,8 @@ class FakeEngine implements ActionEngineFacade { } async find(object: string, query: Record): Promise>> { - const where = query?.where as Record | undefined; - return (this.tables.get(object) ?? []).filter((row) => matches(row, where)).map((row) => ({ ...row })); + // `query` IS the filter. No `query.where` rung — see the note above. + return (this.tables.get(object) ?? []).filter((row) => matches(row, query)).map((row) => ({ ...row })); } rows(object: string): Row[] {