diff --git a/.changeset/engine-double-delete-contract.md b/.changeset/engine-double-delete-contract.md new file mode 100644 index 0000000000..f06788f305 --- /dev/null +++ b/.changeset/engine-double-delete-contract.md @@ -0,0 +1,53 @@ +--- +"@objectstack/objectql": minor +--- + +feat(objectql): export the delete-dispatch contract so test doubles can be pinned to it (#4550) + +A test double that is **looser** than the implementation it replaces converts a +green suite into no suite at all — silently, and on exactly the paths a double +was introduced for, which are the paths that were hard to test, which are +usually where the contract is densest. #4434 is the worked example: +`DELETE /api/v1/sharing/rules/:idOrName` answered 500 for every rule and both +address forms it advertises, from the day it was written, while +`deleteRule drops rule + all its grants` asserted success against it the whole +time — against a fake engine whose `delete` accepted the one call shape +`ObjectQL.delete` refuses. + +`ObjectQL.delete`'s dispatch decision now lives in one exported place instead of +being re-derived by every fake: + +```ts +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; + +async delete(object: string, options?: any) { + assertEngineDeleteDispatch(options); // refuses what a real server refuses + … +} +``` + +New exports, all pure and side-effect free: + +- `resolveEngineDeleteDispatch(options)` → `{ kind: 'by-id', id }` | + `{ kind: 'multi' }` | `{ kind: 'reject', message }` — what the engine will do + with this call, without doing it. +- `assertEngineDeleteDispatch(options)` — throws exactly what the engine throws + on `reject`, returns the dispatch otherwise. This is the line a fake engine's + `delete` opens with. +- `scalarDeleteId(options)` — the SCALAR `where.id` or `undefined`. The half a + hand-written mirror drops: `where: { id: { $in: [...] } }` looks like an id + and is a multi-row predicate, so the engine rejects it without `multi`. +- `ENGINE_DELETE_REJECT_MESSAGE`, `ENGINE_DELETE_DISPATCH_CASES` — the message + and the shared conformance case-set, the same role + `packages/spec/src/data/*-conformance.ts` plays for drivers. + +`ObjectQL.delete` itself reads `resolveEngineDeleteDispatch`, so a double that +imports it cannot be looser than the engine, ever — that is the property, and +it is the one a hand-mirrored `if` can only have until somebody edits one side. +No runtime behaviour changes: the same three verdicts, over the same inputs, +proved case-by-case against the real engine in +`engine-delete-dispatch.test.ts`. + +Repo-side, `pnpm check:engine-double-contract` (wired into `lint.yml`) finds all +39 fake ObjectQL engines in the repo, holds new ones to this predicate, and +keeps the 30 not yet converted in a measured, shrink-only baseline. diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index dcd8419ad0..8fecffd980 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -275,6 +275,23 @@ jobs: - name: Published-files whitelist guard run: pnpm check:published-files + # Engine test-double contract gate (#4550, from #4434). A test double + # LOOSER than the implementation it replaces turns a green suite into no + # suite at all, silently, on exactly the paths a double was introduced + # for. #4434 is the worked example: DELETE /sharing/rules/:idOrName + # answered 500 for every rule and both address forms from the day it was + # written, while `deleteRule drops rule + all its grants` asserted success + # against a fake engine that accepted the one call shape ObjectQL.delete + # refuses. This holds every fake ObjectQL engine's `delete` to the real + # dispatch predicate — imported from @objectstack/objectql, not + # hand-mirrored, so it cannot drift — with the pre-existing fakes in a + # shrink-only, measured baseline. Static AST only, so it needs no build + # and belongs in this job. Runs its own --self-test first: the detector + # can be broken while every fake is fine, and a scan that quietly stops + # matching would report OK while reading nothing (#4868's family). + - name: Engine test-double contract gate + run: pnpm check:engine-double-contract + typecheck: name: TypeScript Type Check runs-on: ubuntu-latest diff --git a/package.json b/package.json index 3b09b3343f..27a225cfd7 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,8 @@ "check:node-version": "node scripts/check-node-version.mjs", "check:published-files": "node scripts/check-published-files.mjs --self-test && node scripts/check-published-files.mjs", "check:type-check-coverage": "node scripts/check-type-check-coverage.mjs --self-test && node scripts/check-type-check-coverage.mjs", - "check:driver-conformance": "node scripts/check-driver-conformance.mjs --self-test && node scripts/check-driver-conformance.mjs" + "check:driver-conformance": "node scripts/check-driver-conformance.mjs --self-test && node scripts/check-driver-conformance.mjs", + "check:engine-double-contract": "node scripts/check-engine-double-contract.mjs --self-test && node scripts/check-engine-double-contract.mjs" }, "keywords": [ "objectstack", diff --git a/packages/objectql/src/engine-delete-dispatch.test.ts b/packages/objectql/src/engine-delete-dispatch.test.ts new file mode 100644 index 0000000000..3f3ecd3584 --- /dev/null +++ b/packages/objectql/src/engine-delete-dispatch.test.ts @@ -0,0 +1,106 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// objectstack#4550 — the shared delete-dispatch predicate must be the REAL +// engine's answer, not a second opinion that happens to agree today. +// +// A shared predicate that drifted from `ObjectQL.delete` would be worse than +// no predicate at all: every fake engine pinned to it would be confidently, +// uniformly wrong, and the gate over them would report success (route-ownership +// rule 3 — prefer failing to falling back). So this file does not test the +// predicate against a table of expectations written next to it. It drives the +// **real engine** with a recording driver over `ENGINE_DELETE_DISPATCH_CASES` +// and asserts the engine's observed behaviour equals the predicate's verdict, +// case by case. +// +// If someone changes the dispatch rule in `engine.ts` without changing +// `engine-delete-dispatch.ts`, this goes red here — the one place where both +// halves are in the room together. + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { + ENGINE_DELETE_DISPATCH_CASES, + ENGINE_DELETE_REJECT_MESSAGE, + resolveEngineDeleteDispatch, + assertEngineDeleteDispatch, + scalarDeleteId, +} from './engine-delete-dispatch.js'; + +/** Records which driver entry point the engine chose, if any. */ +function makeRecordingDriver() { + const calls: Array<{ fn: 'delete' | 'deleteMany'; arg: unknown }> = []; + const driver: any = { + name: 'recording', + version: '0.0.0', + supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find() { return []; }, + async findOne() { return null; }, + async create(_o: string, data: Record) { return { id: 'r1', ...data }; }, + async update(_o: string, id: string, data: Record) { return { id, ...data }; }, + async delete(_o: string, id: string) { calls.push({ fn: 'delete', arg: id }); return true; }, + async deleteMany(_o: string, ast: unknown) { calls.push({ fn: 'deleteMany', arg: ast }); return 0; }, + async count() { return 0; }, + async bulkCreate() { return []; }, async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, calls }; +} + +async function makeEngine() { + const engine = new ObjectQL(); + const { driver, calls } = makeRecordingDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ name: 'task', fields: { title: { type: 'text' } } } as any); + return { engine, calls }; +} + +/** What the real engine actually did with this options bag. */ +async function observeEngine(options: unknown): Promise<'by-id' | 'multi' | 'reject'> { + const { engine, calls } = await makeEngine(); + try { + await engine.delete('task', options as any); + } catch (e) { + if ((e as Error).message === ENGINE_DELETE_REJECT_MESSAGE) return 'reject'; + throw e; + } + if (calls.length !== 1) { + throw new Error(`expected exactly one driver call, saw ${JSON.stringify(calls)}`); + } + return calls[0].fn === 'delete' ? 'by-id' : 'multi'; +} + +describe('engine delete dispatch — the shared predicate IS the engine (#4550)', () => { + it('has cases on both sides of the guard (an empty or one-sided set proves nothing)', () => { + const kinds = new Set(ENGINE_DELETE_DISPATCH_CASES.map((c) => c.expect)); + expect(kinds).toEqual(new Set(['by-id', 'multi', 'reject'])); + expect(ENGINE_DELETE_DISPATCH_CASES.filter((c) => c.expect === 'reject').length).toBeGreaterThan(3); + }); + + for (const c of ENGINE_DELETE_DISPATCH_CASES) { + it(`real engine agrees with the predicate: ${c.what} → ${c.expect}`, async () => { + expect(resolveEngineDeleteDispatch(c.options).kind, 'predicate').toBe(c.expect); + expect(await observeEngine(c.options), 'real ObjectQL.delete').toBe(c.expect); + }); + } + + it('rejects with the exact message a fake must reproduce', () => { + expect(() => assertEngineDeleteDispatch({ where: { rule_id: 'r1' } })) + .toThrow(ENGINE_DELETE_REJECT_MESSAGE); + // …and returns the dispatch (never `reject`) when the call is legal. + expect(assertEngineDeleteDispatch({ where: { id: 'a' } })).toEqual({ kind: 'by-id', id: 'a' }); + expect(assertEngineDeleteDispatch({ multi: true })).toEqual({ kind: 'multi' }); + }); + + it('scalarDeleteId treats operator objects and arrays as predicates, not ids', () => { + expect(scalarDeleteId({ where: { id: 'a' } })).toBe('a'); + expect(scalarDeleteId({ where: { id: 7 } })).toBe(7); + expect(scalarDeleteId({ where: { id: { $in: ['a'] } } })).toBeUndefined(); + expect(scalarDeleteId({ where: { id: ['a'] } })).toBeUndefined(); + expect(scalarDeleteId({ where: { id: null } })).toBeUndefined(); + expect(scalarDeleteId({ where: {} })).toBeUndefined(); + expect(scalarDeleteId(undefined)).toBeUndefined(); + }); +}); diff --git a/packages/objectql/src/engine-delete-dispatch.ts b/packages/objectql/src/engine-delete-dispatch.ts new file mode 100644 index 0000000000..27366cabbe --- /dev/null +++ b/packages/objectql/src/engine-delete-dispatch.ts @@ -0,0 +1,168 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The **one** answer to "what does `ObjectQLEngine.delete` do with this call?" + * — extracted so that the engine and every test double that stands in for it + * read the same predicate rather than two hand-written approximations of it + * (objectstack#4550, from objectstack#4434). + * + * ## Why this is a shared module and not four lines inside `engine.ts` + * + * `#4434` shipped green. `DELETE /api/v1/sharing/rules/:idOrName` answered 500 + * for **both** address forms the route advertises, for every rule, from the day + * it was written — and `plugin-sharing`'s `deleteRule drops rule + all its + * grants` test asserted success against it the whole time. The route was not + * untested; it was tested against a **fake engine whose `delete` accepted a + * call the real engine refuses**. A predicate-shaped purge of + * `sys_record_share` (no scalar `where.id`, no `options.multi`) is precisely + * the one shape `delete()` throws on, and the fake happily deleted by + * predicate. + * + * The fix for #4434 mirrored the guard into that fake by hand. That closes one + * fake and starts a second copy of the contract — the failure mode this module + * exists to remove. A double that *imports the producer's own decision* cannot + * be looser than the producer, ever, which is the property the gate wants and + * the property a copy can only have until someone edits one side. + * + * Same reasoning as `packages/spec/src/data/*-conformance.ts` for drivers, and + * the same shape as objectstack#4455: **the scan and the validator must answer + * with one predicate.** + * + * ## The contract, normatively + * + * `delete(object, options)` dispatches on exactly one question — *does this + * call identify a single row by primary key?* + * + * - `options.where.id` is a **scalar** (`string` / `number` / `bigint`, not + * `null`) → `by-id`: routes to `driver.delete`, runs cascade-delete and the + * by-id RLS pre-image check. + * - otherwise, `options.multi` is truthy → `multi`: routes to + * `driver.deleteMany` with the middleware-composed AST. + * - otherwise → **`reject`**. The call names neither one row nor a bulk + * intent, and the engine throws rather than guessing. + * + * The scalar test is load-bearing and is the half a hand-written double most + * often drops: `where: { id: { $in: [...] } }` is a *multi-row predicate*, not + * an id. Treating it as an id would bind the operator object literally into + * `driver.delete(object, {$in: […]})` **and** skip both the row-scoping AST + * seeding (#2982) and the by-id pre-image check. So it is `reject` unless the + * caller also said `multi`. + * + * @see ObjectQL.delete in `engine.ts` — the only production caller. + * @see scripts/check-engine-double-contract.mjs — the gate that keeps doubles on it. + */ + +/** The message `delete()` throws when a call identifies neither one row nor a bulk intent. */ +export const ENGINE_DELETE_REJECT_MESSAGE = 'Delete requires an ID or options.multi=true'; + +/** What `ObjectQLEngine.delete` will do with a given options bag. */ +export type EngineDeleteDispatch = + /** A scalar `where.id` — `driver.delete`, cascade + by-id RLS pre-image. */ + | { readonly kind: 'by-id'; readonly id: string | number | bigint } + /** No single id but `options.multi` — `driver.deleteMany` with the composed AST. */ + | { readonly kind: 'multi' } + /** Neither — the engine throws `ENGINE_DELETE_REJECT_MESSAGE`. */ + | { readonly kind: 'reject'; readonly message: string }; + +/** The subset of `EngineDeleteOptions` the dispatch decision actually reads. */ +export interface EngineDeleteDispatchInput { + readonly where?: unknown; + readonly multi?: unknown; + readonly [k: string]: unknown; +} + +/** + * Extract the SCALAR `where.id`, or `undefined` when the call does not name one + * row by primary key. + * + * `null`, `undefined`, arrays, and operator objects (`{ $in: [...] }`, + * `{ $ne: … }`) all yield `undefined` — they are predicates over many rows, not + * a primary key. + */ +export function scalarDeleteId( + options?: EngineDeleteDispatchInput | null, +): string | number | bigint | undefined { + const where = options?.where; + if (!where || typeof where !== 'object') return undefined; + if (!('id' in (where as Record))) return undefined; + const whereId = (where as Record).id; + const t = typeof whereId; + if (whereId !== null && (t === 'string' || t === 'number' || t === 'bigint')) { + return whereId as string | number | bigint; + } + return undefined; +} + +/** + * Decide what `ObjectQLEngine.delete` does with `options`, without doing it. + * + * Pure and side-effect free, so a test double can call it to *classify* a call + * and then implement `by-id` / `multi` however its fixture stores rows — while + * being bound to the real engine's `reject` surface for free. + */ +export function resolveEngineDeleteDispatch( + options?: EngineDeleteDispatchInput | null, +): EngineDeleteDispatch { + const id = scalarDeleteId(options); + if (id !== undefined) return { kind: 'by-id', id }; + if (options?.multi) return { kind: 'multi' }; + return { kind: 'reject', message: ENGINE_DELETE_REJECT_MESSAGE }; +} + +/** + * Throw exactly what `ObjectQLEngine.delete` throws when a call is neither + * `by-id` nor `multi`; return the resolved dispatch otherwise. + * + * This is the line a fake engine's `delete` opens with. One call pins the fake + * to the producer's rejection surface, and — unlike a mirrored `if` — it cannot + * drift when the producer's rule changes. + * + * ```ts + * async delete(object: string, options?: any) { + * assertEngineDeleteDispatch(options); // refuses what a real server refuses + * … + * } + * ``` + */ +export function assertEngineDeleteDispatch( + options?: EngineDeleteDispatchInput | null, +): Exclude { + const dispatch = resolveEngineDeleteDispatch(options); + if (dispatch.kind === 'reject') throw new Error(dispatch.message); + return dispatch; +} + +/** + * The shared conformance case-set for the delete dispatch — the same role + * `packages/spec/src/data/*-conformance.ts` plays for drivers. + * + * Every case names a call shape and the verdict the **real engine** gives it. + * A double proved against these is proved against the producer, including the + * three shapes that look like an id and are not. + */ +export interface EngineDeleteDispatchCase { + /** What the shape is, in the words a failure message should use. */ + readonly what: string; + /** The options bag handed to `delete(object, options)`. */ + readonly options: EngineDeleteDispatchInput | undefined; + /** The verdict the engine gives it. */ + readonly expect: EngineDeleteDispatch['kind']; +} + +export const ENGINE_DELETE_DISPATCH_CASES: readonly EngineDeleteDispatchCase[] = [ + { what: 'scalar string id', options: { where: { id: 'rec_1' } }, expect: 'by-id' }, + { what: 'scalar number id', options: { where: { id: 42 } }, expect: 'by-id' }, + { what: 'scalar id alongside other predicates', options: { where: { id: 'rec_1', tenant: 't1' } }, expect: 'by-id' }, + { what: 'multi with a predicate', options: { where: { rule_id: 'r1' }, multi: true }, expect: 'multi' }, + { what: 'multi with no predicate at all', options: { multi: true }, expect: 'multi' }, + { what: 'multi alongside an $in id set', options: { where: { id: { $in: ['a', 'b'] } }, multi: true }, expect: 'multi' }, + // ── The rejects. Everything below is what #4434 shipped against a fake that + // accepted it, and what a running server answers 500 to. + { what: 'predicate on a non-id column, no multi', options: { where: { rule_id: 'r1' } }, expect: 'reject' }, + { what: '$in over ids, no multi (an operator object is NOT an id)', options: { where: { id: { $in: ['a', 'b'] } } }, expect: 'reject' }, + { what: 'array id, no multi', options: { where: { id: ['a', 'b'] } }, expect: 'reject' }, + { what: 'null id, no multi', options: { where: { id: null } }, expect: 'reject' }, + { what: 'empty where, no multi', options: { where: {} }, expect: 'reject' }, + { what: 'no options at all', options: undefined, expect: 'reject' }, + { what: 'multi explicitly false with a predicate', options: { where: { rule_id: 'r1' }, multi: false }, expect: 'reject' }, +]; diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 8cc23d4073..f531cb3705 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -86,6 +86,11 @@ import { validateRecord, normalizeMultiValueFields, coerceBooleanFields, Validat import type { AdmittedValueShapeViolation, AdmittedValueShapeViolationSink } from './validation/record-validator.js'; import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, stripReadonlyFields } from './validation/rule-validator.js'; import { applyInMemoryAggregation } from './in-memory-aggregation.js'; +import { + resolveEngineDeleteDispatch, + ENGINE_DELETE_REJECT_MESSAGE, + type EngineDeleteDispatchInput, +} from './engine-delete-dispatch.js'; import { applyHaving } from './having-filter.js'; import { auditDanglingReferences, @@ -5059,14 +5064,14 @@ export class ObjectQL implements IObjectQLEngine { // literally (driver.delete(object, {$in:[…]})) and both skip the #2982 AST // seeding below AND bypass the by-id RLS pre-image check. Leave `id` // undefined so the call routes to deleteMany with the scoped AST. - let id: any = undefined; - if (options?.where && typeof options.where === 'object' && 'id' in options.where) { - const whereId = (options.where as Record).id; - const t = typeof whereId; - if (whereId !== null && (t === 'string' || t === 'number' || t === 'bigint')) { - id = whereId; - } - } + // + // [#4550] The decision lives in `engine-delete-dispatch.ts` so the fake + // engines that stand in for this method import it instead of re-deriving + // it. #4434 shipped a dead REST route green because plugin-sharing's fake + // accepted the one shape the `reject` branch below refuses; a double that + // reads THIS predicate cannot be looser than this method. + const dispatch = resolveEngineDeleteDispatch(options as EngineDeleteDispatchInput | undefined); + const id: any = dispatch.kind === 'by-id' ? dispatch.id : undefined; const opCtx: OperationContext = { object, @@ -5134,7 +5139,10 @@ export class ObjectQL implements IObjectQLEngine { result = await driver.deleteMany(object, ast, hookContext.input.options as any); isPredicateWrite = true; } else { - throw new Error('Delete requires an ID or options.multi=true'); + // The `reject` verdict of resolveEngineDeleteDispatch, re-asked + // here because a beforeDelete hook may have cleared the id since + // (#4550 keeps the wording in one place either way). + throw new Error(ENGINE_DELETE_REJECT_MESSAGE); } hookContext.event = 'afterDelete'; diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 59626d8e79..3cc6148caa 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -63,6 +63,25 @@ export type { } from './driver-connect-errors.js'; export type { InsertManyRowOutcome } from './engine.js'; +// [#4550] The delete-dispatch contract, exported so a TEST DOUBLE that stands +// in for the engine can import the producer's own decision rather than +// re-deriving it. A fake looser than the contract it replaces is how #4434 +// shipped a REST route that 500'd for every caller with its suite green. +// `scripts/check-engine-double-contract.mjs` is the gate that keeps new +// engine doubles on this. +export { + resolveEngineDeleteDispatch, + assertEngineDeleteDispatch, + scalarDeleteId, + ENGINE_DELETE_REJECT_MESSAGE, + ENGINE_DELETE_DISPATCH_CASES, +} from './engine-delete-dispatch.js'; +export type { + EngineDeleteDispatch, + EngineDeleteDispatchInput, + EngineDeleteDispatchCase, +} from './engine-delete-dispatch.js'; + // Export in-memory aggregation fallback (used by engine.aggregate when the // driver lacks native groupBy/aggregations support; also useful for tests). export { applyInMemoryAggregation, bucketDateValue } from './in-memory-aggregation.js'; diff --git a/packages/objectql/src/layered-overlay-integration.test.ts b/packages/objectql/src/layered-overlay-integration.test.ts index b6669c929c..e82fb92eec 100644 --- a/packages/objectql/src/layered-overlay-integration.test.ts +++ b/packages/objectql/src/layered-overlay-integration.test.ts @@ -19,6 +19,7 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { LayeredRepository, InMemoryRepository, hashSpec } from '@objectstack/metadata-core'; import type { MetaRef } from '@objectstack/metadata-core'; import { SysMetadataRepository } from '@objectstack/metadata-protocol'; +import { assertEngineDeleteDispatch } from './engine-delete-dispatch.js'; interface Row { id: string; @@ -90,6 +91,10 @@ function makeFakeEngine() { return { id: found.row.id }; }, async delete(_t: string, opts: { where: Record }) { + // [#4550] Pinned to ObjectQL.delete's OWN dispatch predicate. A double + // looser than the engine it stands in for is how #4434 shipped a REST + // route that answered 500 to every caller with its suite green. + assertEngineDeleteDispatch(opts); const found = findRow(opts.where); if (!found) return { deleted: 0 }; rows.delete(found.key); diff --git a/packages/objectql/src/protocol-publish-rollback.test.ts b/packages/objectql/src/protocol-publish-rollback.test.ts index 770bc654d0..91e0d77898 100644 --- a/packages/objectql/src/protocol-publish-rollback.test.ts +++ b/packages/objectql/src/protocol-publish-rollback.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest'; import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { assertEngineDeleteDispatch } from './engine-delete-dispatch.js'; /** * Protocol-level coverage for the per-item draft / publish / rollback / @@ -133,6 +134,10 @@ function makeStubEngine() { return { id: found.row.id }; }, async delete(_t: string, opts: { where: Record }) { + // [#4550] Pinned to ObjectQL.delete's OWN dispatch predicate. A double + // looser than the engine it stands in for is how #4434 shipped a REST + // route that answered 500 to every caller with its suite green. + assertEngineDeleteDispatch(opts); const found = findRow(opts.where); if (!found) return { deleted: 0 }; rows.delete(found.key); diff --git a/packages/objectql/src/protocol-save-meta-repo-path.test.ts b/packages/objectql/src/protocol-save-meta-repo-path.test.ts index 418d6e4fb6..fd67fe0a23 100644 --- a/packages/objectql/src/protocol-save-meta-repo-path.test.ts +++ b/packages/objectql/src/protocol-save-meta-repo-path.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from 'vitest'; import { hashSpec } from '@objectstack/metadata-core'; import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { assertEngineDeleteDispatch } from './engine-delete-dispatch.js'; /** * Repository write-path coverage (post PR-10d.6). @@ -70,6 +71,10 @@ function makeStubEngine() { return { id: found.row.id }; }, async delete(_t: string, opts: { where: Record }) { + // [#4550] Pinned to ObjectQL.delete's OWN dispatch predicate. A double + // looser than the engine it stands in for is how #4434 shipped a REST + // route that answered 500 to every caller with its suite green. + assertEngineDeleteDispatch(opts); const found = findRow(opts.where); if (!found) return { deleted: 0 }; rows.delete(found.key); diff --git a/packages/objectql/src/protocol-view-identity-overlay.test.ts b/packages/objectql/src/protocol-view-identity-overlay.test.ts index 6d523d32c8..475acb958f 100644 --- a/packages/objectql/src/protocol-view-identity-overlay.test.ts +++ b/packages/objectql/src/protocol-view-identity-overlay.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'; import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { assertEngineDeleteDispatch } from './engine-delete-dispatch.js'; /** * #2555 — a console personalization PUT (grid column sort, inline edit, …) @@ -87,6 +88,10 @@ function makeStubEngine(registryViews: Record = {}) { return { id: found.row.id }; }, async delete(_t: string, opts: { where: Record }) { + // [#4550] Pinned to ObjectQL.delete's OWN dispatch predicate. A double + // looser than the engine it stands in for is how #4434 shipped a REST + // route that answered 500 to every caller with its suite green. + assertEngineDeleteDispatch(opts); const found = findRow(opts.where); if (!found) return { deleted: 0 }; rows.delete(found.key); diff --git a/packages/objectql/src/sys-metadata-repository.test.ts b/packages/objectql/src/sys-metadata-repository.test.ts index 7b322308f9..87f11f39d8 100644 --- a/packages/objectql/src/sys-metadata-repository.test.ts +++ b/packages/objectql/src/sys-metadata-repository.test.ts @@ -11,6 +11,7 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { ConflictError, hashSpec } from '@objectstack/metadata-core'; import { SysMetadataRepository } from '@objectstack/metadata-protocol'; +import { assertEngineDeleteDispatch } from './engine-delete-dispatch.js'; interface Row { id: string; @@ -119,6 +120,10 @@ function makeFakeEngine() { return { id: found.row.id }; }, async delete(_t: string, opts: { where: Record }) { + // [#4550] Pinned to ObjectQL.delete's OWN dispatch predicate. A double + // looser than the engine it stands in for is how #4434 shipped a REST + // route that answered 500 to every caller with its suite green. + assertEngineDeleteDispatch(opts); const found = findRow(opts.where); if (!found) return { deleted: 0 }; rows.delete(found.key); diff --git a/packages/plugins/plugin-sharing/src/boot-backfill.test.ts b/packages/plugins/plugin-sharing/src/boot-backfill.test.ts index 7394cb6e35..625e29bd5d 100644 --- a/packages/plugins/plugin-sharing/src/boot-backfill.test.ts +++ b/packages/plugins/plugin-sharing/src/boot-backfill.test.ts @@ -12,6 +12,7 @@ */ import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; import { SharingService } from './sharing-service.js'; import { SharingRuleService } from './sharing-rule-service.js'; import { backfillRuleGrants, backfillRetiredAccessLevels } from './sharing-plugin.js'; @@ -58,13 +59,11 @@ function makeEngine() { return t[i]; }, async delete(o: string, opts?: any) { - // Mirror `ObjectQLEngine.delete`'s dispatch guard (#4434) — see the same - // note in sharing-rule.test.ts. A fake looser than the contract it - // stands in for is how a green suite ships a dead route. - const whereId = opts?.where && typeof opts.where === 'object' ? (opts.where as any).id : undefined; - const t0 = typeof whereId; - const scalarId = whereId != null && (t0 === 'string' || t0 === 'number' || t0 === 'bigint'); - if (!scalarId && !opts?.multi) throw new Error('Delete requires an ID or options.multi=true'); + // Pinned to `ObjectQLEngine.delete`'s own dispatch predicate (#4434, + // #4550) — see the longer note in sharing-rule.test.ts. This used to + // MIRROR the guard by hand; a mirror is a second copy of the contract, + // so it now calls the producer's exported decision instead. + assertEngineDeleteDispatch(opts); const t = ensure(o); const where = opts?.where ?? {}; for (let i = t.length - 1; i >= 0; i--) if (matches(t[i], where)) t.splice(i, 1); return { ok: true }; diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-provenance.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule-provenance.test.ts index a9cb185583..c3fd86e26a 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-provenance.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-provenance.test.ts @@ -17,6 +17,7 @@ */ import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; import { SharingService } from './sharing-service.js'; import { SharingRuleService } from './sharing-rule-service.js'; import { bindRuleProvenanceStamp, SHARING_RULE_PROVENANCE_PACKAGE } from './sharing-rule-provenance.js'; @@ -53,6 +54,9 @@ function makeEngine() { return t[i]; }, async delete(o: string, opts?: any) { + // [#4550] Pinned to ObjectQL.delete's own dispatch predicate — see the + // note in sharing-rule.test.ts for what a looser fake cost (#4434). + assertEngineDeleteDispatch(opts); const t = ensure(o); const where = opts?.where ?? {}; for (let i = t.length - 1; i >= 0; i--) if (matches(t[i], where)) t.splice(i, 1); return { ok: true }; diff --git a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts index 292aef923b..f438ff8590 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; import { SharingService } from './sharing-service.js'; import { SharingRuleService } from './sharing-rule-service.js'; import { TeamGraphService, expandPrincipal } from './team-graph.js'; @@ -59,7 +60,23 @@ function makeEngine() { return t[i]; }, async delete(o: string, opts?: any) { - assertDeletable(opts); + // Pinned to `ObjectQLEngine.delete`'s OWN dispatch predicate + // (objectstack#4434, objectstack#4550). + // + // The real engine routes a delete by SCALAR `where.id` to `driver.delete` + // and anything else to `driver.deleteMany` — but only when + // `options.multi` is set; otherwise it throws. This fake used to accept + // any `where`, so `deleteRule`'s predicate-shaped purge of + // `sys_record_share` passed here while the running server answered 500 to + // every `DELETE /sharing/rules/:idOrName`. A fake looser than the + // contract it stands in for is how a green suite ships a dead route. + // + // #4434 closed that by MIRRORING the guard here. A mirror is a second + // copy of the contract and drifts the moment either side is edited, so it + // now calls the producer's exported decision instead — the same function + // `ObjectQL.delete` dispatches on. `pnpm check:engine-double-contract` + // holds every fake engine to this. + assertEngineDeleteDispatch(opts); const t = ensure(o); const where = opts?.where ?? {}; for (let i = t.length - 1; i >= 0; i--) if (matches(t[i], where)) t.splice(i, 1); return { ok: true }; @@ -67,24 +84,6 @@ function makeEngine() { }; } -/** - * Mirror `ObjectQLEngine.delete`'s dispatch guard (objectstack#4434). - * - * The real engine routes a delete by SCALAR `where.id` to `driver.delete` and - * anything else to `driver.deleteMany` — but only when `options.multi` is set; - * otherwise it throws `'Delete requires an ID or options.multi=true'`. The fake - * used to accept any `where`, so `deleteRule`'s predicate-shaped purge of - * `sys_record_share` passed here while the running server answered 500 to every - * `DELETE /sharing/rules/:idOrName`. A fake looser than the contract it stands - * in for is how a green suite ships a dead route. - */ -function assertDeletable(opts?: any): void { - const whereId = opts?.where && typeof opts.where === 'object' ? (opts.where as any).id : undefined; - const t = typeof whereId; - const scalarId = whereId != null && (t === 'string' || t === 'number' || t === 'bigint'); - if (!scalarId && !opts?.multi) throw new Error('Delete requires an ID or options.multi=true'); -} - describe('TeamGraphService (flat — better-auth sys_team)', () => { let engine: ReturnType; beforeEach(() => { diff --git a/packages/plugins/plugin-sharing/src/sharing-service.test.ts b/packages/plugins/plugin-sharing/src/sharing-service.test.ts index c83a48bee6..80a649597b 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.test.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, beforeEach } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; import { SharingService } from './sharing-service.js'; import { buildSharingMiddleware } from './sharing-plugin.js'; @@ -74,6 +75,9 @@ function makeFakeEngine(schemas: Record) { return table[i]; }, async delete(object: string, options?: any) { + // [#4550] Pinned to ObjectQL.delete's own dispatch predicate — see the + // note in sharing-rule.test.ts for what a looser fake cost (#4434). + assertEngineDeleteDispatch(options); const table = ensure(object); const id = options?.where?.id ?? options?.id; const i = table.findIndex(r => r.id === id); diff --git a/scripts/check-engine-double-contract.mjs b/scripts/check-engine-double-contract.mjs new file mode 100644 index 0000000000..ac8ab6dc5b --- /dev/null +++ b/scripts/check-engine-double-contract.mjs @@ -0,0 +1,557 @@ +#!/usr/bin/env node +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// check-engine-double-contract -- a fake ObjectQL engine's `delete` must be +// pinned to the real engine's dispatch contract, not a looser hand-written +// approximation of it (objectstack#4550, from objectstack#4434). +// +// node scripts/check-engine-double-contract.mjs +// node scripts/check-engine-double-contract.mjs --self-test +// +// ## The failure mode this exists for +// +// `DELETE /api/v1/sharing/rules/:idOrName` answered **500 for every rule and +// both address forms** from the day it was written. It was not untested: +// plugin-sharing's `deleteRule drops rule + all its grants` asserted success on +// it the whole time -- against a FAKE ENGINE whose `delete` accepted a call the +// real engine refuses. `deleteRule` purged `sys_record_share` with a +// predicate-shaped delete carrying neither a scalar `where.id` nor +// `options.multi`, which is precisely the one shape `ObjectQL.delete` throws +// on. The fake deleted by predicate happily, so the suite was green, the gate +// was green, and the route was dead. That is #4434. +// +// The general shape, and the reason this is a gate rather than a fixed test: +// **a test double looser than the implementation it replaces converts a green +// suite into no suite at all**, silently, on exactly the paths a double was +// introduced for -- which are the paths that were hard to test, which are +// usually the paths where the contract is densest. +// +// ## Slice: `delete` dispatch only, and only on ENGINE doubles +// +// #4550 lists four instances of the family. This gate takes exactly one of +// them, the one whose criterion is mechanically decidable with no judgment +// call: the engine's delete dispatch is a total function from an options bag to +// one of three verdicts, so "is this double looser?" has a yes/no answer that +// does not depend on reading the test's intent. +// +// Deliberately NOT covered, and why (each wants its own gate, not a vaguer +// version of this one -- a gate whose scope is fuzzy is indistinguishable from +// no gate to everyone downstream of it): +// +// - fixtures that disable a platform constraint in prose (`// FK enforcement +// is off in this harness`, #4441). The criterion is a comment, so it is +// both evadable by deleting the comment and unable to find the silent +// cases. That one wants a declared debt ledger, not a scanner. +// - stubbing the very thing under assertion (objectui#3129) and missing +// counterparts (objectui#3134). Both live in the `objectui` repo, which +// this script cannot see, and #3134 names no double at all. +// - every other method a fake engine offers (`find` filter semantics, +// `update`'s twin dispatch, unknown-option rejection). Same family, but +// each needs its own producer-side predicate extracted first. `delete` had +// one available because #4434 already paid for it. +// +// ## Invariants +// +// DISCOVERED the scan found engine doubles at all. Zero is not "a clean +// repo", it is a broken scan: PINNED iterates the discovered set, +// so a discovery that silently stops matching makes this script +// print OK while checking nothing -- the #4868 family, where a +// check runs, is green, and structurally cannot reach its subject. +// PINNED every discovered engine double's `delete` routes through +// `assertEngineDeleteDispatch` / `resolveEngineDeleteDispatch` +// from `@objectstack/objectql` -- the predicate the real +// `ObjectQL.delete` itself uses -- or its file carries a measured +// baseline entry. +// RECONCILED in both directions. A baseline entry for a file with no +// unguarded doubles left, for a file that no longer exists, or +// whose count is now lower, is an error. A ratchet that can only +// accrete rots into a list nobody trusts. +// +// ## Why "routes through the shared predicate" and not "mirrors the guard" +// +// The #4434 fix mirrored the engine's guard into that one fake by hand. That +// closes one fake and opens a second copy of the contract -- and the scalar +// test is the half a hand-written copy drops: `where: { id: { $in: [...] } }` +// LOOKS like an id and is a multi-row predicate, so the real engine rejects it +// without `multi` and a mirrored `if (!opts?.where?.id && !opts?.multi)` accepts +// it. Requiring the producer's own function removes the class: a double that +// imports the decision cannot be looser than the decision. Same reasoning as +// objectstack#4455 -- the scan and the validator must answer with ONE predicate. +// +// ## What this deliberately does NOT claim +// +// It checks that the shared predicate is CALLED, not that the double's by-id +// and multi branches then behave like the driver would. A gate cannot judge +// that, and one that pretended to would be the verifier that reports success +// while degrading. What it can do is make the rejection surface impossible to +// drift, which is the half that shipped #4434. + +import { readFileSync, readdirSync, existsSync } from 'node:fs'; +import { join, dirname, relative, sep } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import ts from 'typescript'; + +const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..'); +const BASELINE_PATH = join(ROOT, 'scripts', 'engine-double-contract.baseline.json'); +const SCAN_ROOTS = ['packages', 'examples']; + +/** The producer-side predicate a double must route through. */ +const PINNED_SYMBOLS = new Set(['assertEngineDeleteDispatch', 'resolveEngineDeleteDispatch']); +/** Where it may legitimately come from: the public export, or objectql's own relative path. */ +const PINNED_MODULES = [/^@objectstack\/objectql$/, /engine-delete-dispatch(\.js)?$/]; + +/** + * Members that mark an object literal as standing in for the ENGINE. + * + * The engine and the driver both have `delete`, so the sibling set is what + * separates them alongside the parameter test below: drivers speak + * `create`/`bulkCreate`/`checkHealth`, the engine speaks `insert`/`findOne`. + */ +const ENGINE_SIBLINGS = new Set([ + 'find', 'findOne', 'insert', 'update', 'count', 'aggregate', 'getSchema', 'registry', 'insertMany', +]); + +/** Parameter names that mean "this is the DRIVER's delete(object, id, options)". */ +const ID_PARAM = /^_*(id|recordId|ids|pk)$/i; + +// ── Discovery ─────────────────────────────────────────────────────────────── + +function walk(dir, out = []) { + let entries; + try { + entries = readdirSync(dir, { withFileTypes: true }); + } catch { + return out; + } + for (const e of entries) { + if (e.name === 'node_modules' || e.name === 'dist' || e.name === '.git' || e.name === '.cache') continue; + const p = join(dir, e.name); + if (e.isDirectory()) walk(p, out); + else if (/\.(test|spec)\.(ts|tsx|mts)$/.test(e.name)) out.push(p); + } + return out; +} + +function testFiles() { + const out = []; + for (const r of SCAN_ROOTS) walk(join(ROOT, r), out); + return out.sort(); +} + +/** A member's function-ish implementation, or null. */ +function implOf(member) { + if (ts.isMethodDeclaration(member) || ts.isMethodSignature(member)) return member; + if (ts.isPropertyAssignment(member)) { + const init = member.initializer; + if (init && (ts.isFunctionExpression(init) || ts.isArrowFunction(init))) return init; + return null; + } + if (ts.isPropertyDeclaration(member) && member.initializer) { + const init = member.initializer; + if (ts.isFunctionExpression(init) || ts.isArrowFunction(init)) return init; + return null; + } + if (ts.isShorthandPropertyAssignment(member)) return null; + return null; +} + +function memberName(member) { + const n = member.name; + if (!n) return null; + if (ts.isIdentifier(n) || ts.isStringLiteral(n)) return n.text; + return null; +} + +/** + * Is this `delete(a, b, …)` the ENGINE's shape (`object, options`) rather than + * the DRIVER's (`object, id, options`)? + * + * The second parameter is the whole question: the engine takes an options bag + * there, the driver takes a primary key. Judged on the name first (the repo + * writes `id` when it means one) and on a scalar type annotation second. + */ +function isEngineDeleteShape(fn) { + const params = fn.parameters ?? []; + if (params.length < 2) return false; + const second = params[1]; + const name = ts.isIdentifier(second.name) ? second.name.text : ''; + if (ID_PARAM.test(name)) return false; + const t = second.type ? second.type.getText() : ''; + if (/^(string|number|bigint|string \| number)$/.test(t.trim())) return false; + return true; +} + +/** Collect every identifier that is CALLED anywhere inside `node`. */ +function calleesIn(node) { + const names = new Set(); + const visit = (n) => { + if (ts.isCallExpression(n)) { + const e = n.expression; + if (ts.isIdentifier(e)) names.add(e.text); + else if (ts.isPropertyAccessExpression(e) && ts.isIdentifier(e.name)) names.add(e.name.text); + } + ts.forEachChild(n, visit); + }; + visit(node); + return names; +} + +/** + * LOCAL names in this file that are bound to one of the pinned predicates by an + * import from the producer. + * + * Keyed on the local binding (so `import { assertEngineDeleteDispatch as guard }` + * still counts) but only when the IMPORTED name is one of the pinned symbols — + * a same-named local look-alike must not qualify, since the whole property is + * that one predicate answers. + */ +function pinnedImportsOf(sourceFile) { + const found = new Set(); + for (const st of sourceFile.statements) { + if (!ts.isImportDeclaration(st) || !ts.isStringLiteral(st.moduleSpecifier)) continue; + const spec = st.moduleSpecifier.text; + if (!PINNED_MODULES.some((re) => re.test(spec))) continue; + const named = st.importClause?.namedBindings; + if (named && ts.isNamedImports(named)) { + for (const el of named.elements) { + const imported = (el.propertyName ?? el.name).text; + if (PINNED_SYMBOLS.has(imported)) found.add(el.name.text); + } + } + } + return found; +} + +/** Top-level function declarations / const-arrow functions, by name. */ +function localFunctions(sourceFile) { + const map = new Map(); + const visit = (n) => { + if (ts.isFunctionDeclaration(n) && n.name) map.set(n.name.text, n); + if (ts.isVariableStatement(n)) { + for (const d of n.declarationList.declarations) { + if (ts.isIdentifier(d.name) && d.initializer + && (ts.isArrowFunction(d.initializer) || ts.isFunctionExpression(d.initializer))) { + map.set(d.name.text, d.initializer); + } + } + } + ts.forEachChild(n, visit); + }; + visit(sourceFile); + return map; +} + +/** + * Every engine double in one file, with a verdict on whether its `delete` is + * pinned to the shared predicate. + * + * Pinning is accepted one level of indirection deep: a fake that opens with a + * local `assertDeletable(opts)` helper which itself calls the shared predicate + * is pinned. Two hops is not — at that point the gate would be guessing. + */ +function scanSource(fileName, text) { + const sf = ts.createSourceFile(fileName, text, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + const pinnedNames = pinnedImportsOf(sf); + const locals = localFunctions(sf); + const doubles = []; + + const bodyIsPinned = (fn) => { + if (pinnedNames.size === 0) return false; + const direct = calleesIn(fn); + for (const n of direct) if (pinnedNames.has(n)) return true; + for (const n of direct) { + const local = locals.get(n); + if (!local) continue; + for (const m of calleesIn(local)) if (pinnedNames.has(m)) return true; + } + return false; + }; + + const consider = (members, node) => { + const names = new Set(); + let del = null; + for (const m of members) { + const n = memberName(m); + if (!n) continue; + names.add(n); + if (n === 'delete') del = implOf(m); + } + if (!del) return; + const siblings = [...names].filter((n) => ENGINE_SIBLINGS.has(n)); + if (siblings.length < 2) return; + if (!isEngineDeleteShape(del)) return; + const line = sf.getLineAndCharacterOfPosition(node.getStart(sf)).line + 1; + doubles.push({ line, siblings: siblings.sort(), pinned: bodyIsPinned(del) }); + }; + + const visit = (n) => { + if (ts.isObjectLiteralExpression(n)) consider(n.properties, n); + else if (ts.isClassDeclaration(n) || ts.isClassExpression(n)) consider(n.members, n); + ts.forEachChild(n, visit); + }; + visit(sf); + return doubles; +} + +// ── Baseline ──────────────────────────────────────────────────────────────── + +function readBaseline() { + if (!existsSync(BASELINE_PATH)) return { entries: [] }; + return JSON.parse(readFileSync(BASELINE_PATH, 'utf8')); +} + +// ── Audit ─────────────────────────────────────────────────────────────────── + +function audit() { + const baseline = readBaseline(); + const byFile = new Map(baseline.entries.map((e) => [e.file, e])); + const errors = []; + const found = []; + + for (const abs of testFiles()) { + const rel = relative(ROOT, abs).split(sep).join('/'); + const text = readFileSync(abs, 'utf8'); + // Cheap pre-filter: no `delete` member, nothing to parse. + if (!/\bdelete\s*[(:]/.test(text)) continue; + const doubles = scanSource(abs, text); + if (doubles.length === 0) continue; + found.push({ file: rel, doubles }); + } + + // DISCOVERED + if (found.length === 0) { + errors.push( + 'DISCOVERED: the scan found no engine doubles anywhere. That is not a clean repo, it is a ' + + 'broken scan — PINNED iterates this set, so every other invariant passes vacuously and ' + + 'this script reports OK while reading nothing. Fix the discovery before trusting a green run.', + ); + } + + const seen = new Set(); + for (const { file, doubles } of found) { + const unguarded = doubles.filter((d) => !d.pinned); + const entry = byFile.get(file); + if (entry) seen.add(file); + + if (unguarded.length === 0) { + if (entry) { + errors.push( + `RECONCILED: ${file} has no unguarded engine double left, but the baseline still ` + + `records ${entry.unguarded}. Delete the entry in the same PR that fixed it.`, + ); + } + continue; + } + if (!entry) { + errors.push( + `PINNED: ${file} declares ${unguarded.length} engine double(s) whose delete() does not route ` + + `through assertEngineDeleteDispatch (line${unguarded.length > 1 ? 's' : ''} ` + + `${unguarded.map((d) => d.line).join(', ')}). A fake looser than ObjectQL.delete is how ` + + '#4434 shipped a dead REST route with its suite green. Open the fake\'s delete with ' + + "`assertEngineDeleteDispatch(options)` from '@objectstack/objectql' (add it as a " + + 'devDependency if the package lacks it), or add a MEASURED entry to ' + + 'scripts/engine-double-contract.baseline.json saying why not.', + ); + continue; + } + if (unguarded.length > entry.unguarded) { + errors.push( + `PINNED: ${file} now has ${unguarded.length} unguarded engine double(s), baseline records ` + + `${entry.unguarded}. The baseline is shrink-only — pin the new one rather than raising it.`, + ); + } else if (unguarded.length < entry.unguarded) { + errors.push( + `RECONCILED: ${file} is down to ${unguarded.length} unguarded engine double(s) from the ` + + `baseline's ${entry.unguarded}. Lower the number in the same PR, so the ratchet holds.`, + ); + } + } + + for (const entry of baseline.entries) { + if (seen.has(entry.file)) continue; + errors.push( + `RECONCILED: baseline entry for ${entry.file}, which declares no engine double any more ` + + '(file deleted, fake removed, or the shape changed). Delete the entry.', + ); + } + + return { found, baseline, errors }; +} + +function report() { + const { found, baseline, errors } = audit(); + const doubles = found.reduce((n, f) => n + f.doubles.length, 0); + const pinned = found.reduce((n, f) => n + f.doubles.filter((d) => d.pinned).length, 0); + + console.log( + `\nengine doubles: ${doubles} in ${found.length} test file(s) — ${pinned} pinned to ` + + `ObjectQL.delete's dispatch predicate, ${doubles - pinned} in the shrink-only baseline.\n`, + ); + + if (errors.length) { + for (const e of errors) console.error(` x ${e}`); + console.error(`\ncheck-engine-double-contract: ${errors.length} problem(s).\n`); + process.exit(1); + } + + for (const f of found.filter((f) => f.doubles.some((d) => d.pinned))) { + console.log(` pinned ${f.file}`); + } + + // Print the EXEMPT reasons, not only the count. An entry whose justification + // is never surfaced is how a ledger decays into a list nobody reads — and + // these rows are part of why this run is green. + const exempt = baseline.entries.filter((e) => e.kind === 'EXEMPT'); + if (exempt.length) console.log(''); + for (const e of exempt) { + console.log(` EXEMPT ${e.file}`); + console.log(` ${e.why}`); + } + console.log(''); + const debt = baseline.entries.filter((e) => e.kind !== 'EXEMPT').length; + console.log( + `check-engine-double-contract: OK — ${pinned} pinned, ${debt} in the DEBT ledger, ` + + `${exempt.length} exempt.\n`, + ); +} + +// ── Self-test ─────────────────────────────────────────────────────────────── +// +// A guard that cannot fail is not a guard (#4118). This drives the detector +// against synthetic sources on BOTH sides of every decision it makes, so a +// refactor that neuters it fails here instead of turning every future PR green. + +function selfTest() { + const failures = []; + const expect = (label, cond) => { if (!cond) failures.push(label); }; + + const IMPORT = "import { assertEngineDeleteDispatch } from '@objectstack/objectql';\n"; + const engineFake = (deleteBody, header = '') => `${header} +function makeEngine() { + return { + async find(o: string, opts?: any) { return []; }, + async insert(o: string, data: any) { return data; }, + async update(o: string, d: any) { return d; }, + async delete(o: string, opts?: any) { ${deleteBody} }, + }; +} +`; + + // ── Detection: an unpinned engine fake is found, a pinned one is not flagged. + let d = scanSource('a.test.ts', engineFake('return { ok: true };')); + expect('finds an unpinned engine double', d.length === 1 && d[0].pinned === false); + + d = scanSource('a.test.ts', engineFake('assertEngineDeleteDispatch(opts); return { ok: true };', IMPORT)); + expect('a directly pinned double is not flagged', d.length === 1 && d[0].pinned === true); + + // One level of indirection through a local helper counts; the helper must + // itself reach the shared predicate. + d = scanSource('a.test.ts', engineFake('assertDeletable(opts); return 1;', + IMPORT + 'function assertDeletable(o: any) { assertEngineDeleteDispatch(o); }\n')); + expect('a local helper that calls the predicate counts as pinned', d.length === 1 && d[0].pinned === true); + + d = scanSource('a.test.ts', engineFake('assertDeletable(opts); return 1;', + IMPORT + 'function assertDeletable(o: any) { if (!o?.where?.id && !o?.multi) throw new Error("x"); }\n')); + expect('a HAND-MIRRORED local helper does not count as pinned', + d.length === 1 && d[0].pinned === false); + + // Importing the symbol without calling it is not pinning — the #4434 fake + // would have passed a check that only looked at imports. + d = scanSource('a.test.ts', engineFake('return { ok: true };', IMPORT)); + expect('an unused import is not pinning', d.length === 1 && d[0].pinned === false); + + // ── Scope: the DRIVER's delete(object, id, options) is a different contract + // and must not be swept in, or the gate drowns in false positives. + const driverFake = ` +const driver = { + async find(o: string) { return []; }, + async create(o: string, d: any) { return d; }, + async update(o: string, id: string, d: any) { return d; }, + async delete(object: string, id: string) { return true; }, +}; +`; + expect('a driver double (delete by scalar id) is out of scope', scanSource('d.test.ts', driverFake).length === 0); + + const typedIdDriver = ` +const driver = { + async find(o: string) { return []; }, + async insert(o: string, d: any) { return d; }, + async delete(object: string, key: string) { return true; }, +}; +`; + expect('a scalar-typed second parameter is out of scope', + scanSource('d.test.ts', typedIdDriver).length === 0); + + // A lone `delete` with no engine siblings is a Map-ish or route helper. + const bare = 'const cache = { delete(k: string, o: any) { return true; } };\n'; + expect('an object with no engine siblings is out of scope', scanSource('c.test.ts', bare).length === 0); + + // ── Shape coverage: the fake shapes this repo actually writes. + const classFake = `${IMPORT} +class FakeEngine { + async find(o: string, q?: any) { return []; } + async insert(o: string, d: any) { return d; } + async delete(o: string, opts?: any) { assertEngineDeleteDispatch(opts); return 1; } +} +`; + d = scanSource('k.test.ts', classFake); + expect('a class-shaped fake engine is in scope and pinnable', d.length === 1 && d[0].pinned === true); + + const arrowFake = ` +const engine = { + find: async (o: string) => [], + insert: async (o: string, d: any) => d, + update: async (o: string, d: any) => d, + delete: async (o: string, opts: any) => ({ ok: true }), +}; +`; + d = scanSource('p.test.ts', arrowFake); + expect('an arrow-property fake engine is in scope', d.length === 1 && d[0].pinned === false); + + // The import must come from the producer. A same-named local function is not + // the contract — the whole point is that ONE predicate answers. + d = scanSource('q.test.ts', engineFake('assertEngineDeleteDispatch(opts); return 1;', + 'function assertEngineDeleteDispatch(o: any) { /* look-alike */ }\n')); + expect('a locally re-declared look-alike is not pinning', d.length === 1 && d[0].pinned === false); + + d = scanSource('r.test.ts', engineFake('assertEngineDeleteDispatch(opts); return 1;', + "import { assertEngineDeleteDispatch } from './my-helpers.js';\n")); + expect('the predicate imported from an unrelated module is not pinning', + d.length === 1 && d[0].pinned === false); + + // objectql's own tests import it by relative path; that IS the producer. + d = scanSource('s.test.ts', engineFake('assertEngineDeleteDispatch(opts); return 1;', + "import { assertEngineDeleteDispatch } from './engine-delete-dispatch.js';\n")); + expect("objectql's relative import of the producer counts", d.length === 1 && d[0].pinned === true); + + expect('a file with no fakes yields no doubles', + scanSource('empty.test.ts', 'export const x = 1;\n').length === 0); + + // Discovery must reach the real tree, and specifically must reach the fake + // #4434 was shipped past. Everything above is synthetic; this is the wiring. + // + // Deliberately NOT asserted here: that the real tree is clean, or that any + // particular fake is pinned. That is the job of the run this self-test gates, + // and duplicating it would make a genuine violation surface as a self-test + // failure — the least legible message available. + const { found } = audit(); + expect('discovers engine doubles in the real tree', found.length > 0); + expect( + 'discovery reaches the #4434 fake', + found.some((f) => f.file === 'packages/plugins/plugin-sharing/src/sharing-rule.test.ts'), + ); + + if (failures.length) { + for (const f of failures) console.error(` x self-test: ${f}`); + console.error(`\ncheck-engine-double-contract --self-test: ${failures.length} failure(s).\n`); + process.exit(1); + } + console.log( + 'OK self-test: separates engine doubles from driver doubles, accepts only the producer\'s ' + + 'predicate (direct or one helper deep), rejects unused imports, hand-mirrored guards and ' + + 'look-alikes, and proves discovery reaches the real tree.', + ); +} + +if (process.argv.includes('--self-test')) selfTest(); +else report(); diff --git a/scripts/engine-double-contract.baseline.json b/scripts/engine-double-contract.baseline.json new file mode 100644 index 0000000000..d4c446aeaa --- /dev/null +++ b/scripts/engine-double-contract.baseline.json @@ -0,0 +1,238 @@ +{ + "$comment": [ + "Measured baseline for scripts/check-engine-double-contract.mjs (#4550).", + "", + "Each entry is a test file that declares a fake ObjectQL engine whose `delete` does NOT", + "route through `assertEngineDeleteDispatch` from @objectstack/objectql — so it may accept", + "a call the real engine refuses, which is exactly how #4434 shipped a dead REST route with", + "its suite green.", + "", + "SHRINK-ONLY and hand-edited under review. The checker fails in BOTH directions: a file", + "whose count went down must have its entry lowered or deleted in the same PR, and a file", + "with no unguarded double left must lose its entry entirely. There is deliberately no", + "`--fix`/`--update` flag — a generator would let a new lax fake be admitted by 'just run", + "the update command', which is precisely how a gate stops meaning anything.", + "", + "`unguarded` is measured by the scan, not asserted by hand. `why` says what stands between", + "the file and being pinned; `closes` says what removes the entry.", + "", + "WHAT THESE ENTRIES DO AND DO NOT CLAIM. They record that the double is structurally", + "looser than the contract. They do NOT claim the looseness is currently harmless — proving", + "that per file means running each suite with the guard installed, and the one package where", + "that was done wholesale in this PR (@objectstack/spec, 295 files) passed. The rest is", + "unproven either way, which is the honest state and the reason each entry is DEBT rather", + "than EXEMPT." + ], + "entries": [ + { + "file": "packages/metadata-protocol/src/protocol-publish-drafts-org-scope.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/metadata-protocol does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/metadata-protocol/src/protocol.runtime-authoring-gate.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/metadata-protocol does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/metadata-protocol/src/protocol.save-flow-canonicalization.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/metadata-protocol does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/metadata-protocol/src/sys-metadata-repository.recorded-by.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/metadata-protocol does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/plugins/plugin-approvals/src/approval-actor-impersonation.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "The package DOES depend on @objectstack/objectql, so this is a one-line pin — deferred only because this PR's slice is the gate plus the package #4434 came from, and an unmeasured suite flipping red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite" + }, + { + "file": "packages/plugins/plugin-approvals/src/approval-node.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "The package DOES depend on @objectstack/objectql, so this is a one-line pin — deferred only because this PR's slice is the gate plus the package #4434 came from, and an unmeasured suite flipping red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite" + }, + { + "file": "packages/plugins/plugin-approvals/src/approval-override-audit.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "The package DOES depend on @objectstack/objectql, so this is a one-line pin — deferred only because this PR's slice is the gate plus the package #4434 came from, and an unmeasured suite flipping red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite" + }, + { + "file": "packages/plugins/plugin-approvals/src/approval-restart-resume.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "The package DOES depend on @objectstack/objectql, so this is a one-line pin — deferred only because this PR's slice is the gate plus the package #4434 came from, and an unmeasured suite flipping red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite" + }, + { + "file": "packages/plugins/plugin-approvals/src/approval-revise.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "The package DOES depend on @objectstack/objectql, so this is a one-line pin — deferred only because this PR's slice is the gate plus the package #4434 came from, and an unmeasured suite flipping red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite" + }, + { + "file": "packages/plugins/plugin-approvals/src/approval-service.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "The package DOES depend on @objectstack/objectql, so this is a one-line pin — deferred only because this PR's slice is the gate plus the package #4434 came from, and an unmeasured suite flipping red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite" + }, + { + "file": "packages/plugins/plugin-auth/src/auth-manager.optional-plugin-isolation.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/plugin-auth does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/plugins/plugin-reports/src/report-export-axis.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/plugin-reports does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/plugins/plugin-reports/src/report-service.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/plugin-reports does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "HAND-MIRRORS the guard already (a copy of the #4434 fix's `if`), which is the second copy of the contract this gate exists to remove — but @objectstack/plugin-security does not depend on @objectstack/objectql, so replacing the copy with the producer's predicate needs a devDependency change.", + "closes": "add @objectstack/objectql to devDependencies, then replace the mirrored `if` with assertEngineDeleteDispatch(options)" + }, + { + "file": "packages/plugins/plugin-security/src/permission-set-projection.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/plugin-security does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/plugins/plugin-security/src/suggested-audience-bindings.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/plugin-security does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/plugins/plugin-webhooks/src/auto-enqueuer.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/plugin-webhooks does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/runtime/src/action-body-identity.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "The package DOES depend on @objectstack/objectql, so this is a one-line pin — deferred only because this PR's slice is the gate plus the package #4434 came from, and an unmeasured suite flipping red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite" + }, + { + "file": "packages/services/service-automation/src/builtin/crud-config-aliases.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/services/service-automation/src/builtin/crud-filter-guard.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/services/service-automation/src/builtin/crud-runas.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/services/service-automation/src/fault-edge-guard-containment.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/services/service-automation/src/plugin-suspended-run-wiring.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/services/service-automation/src/record-lookup-expand.integration.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/services/service-automation/src/runas-grant-resolution.integration.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/services/service-automation/src/suspended-run-store.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "HAND-MIRRORS the guard already, which is the second copy of the contract this gate exists to remove — but @objectstack/service-automation does not depend on @objectstack/objectql, so replacing the copy with the producer's predicate needs a devDependency change.", + "closes": "add @objectstack/objectql to devDependencies, then replace the mirrored `if` with assertEngineDeleteDispatch(options)" + }, + { + "file": "packages/services/service-datasource/src/__tests__/datasource-admin-plugin.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/service-datasource does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/services/service-datasource/src/__tests__/datasource-secret-binder.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "@objectstack/service-datasource does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", + "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + }, + { + "file": "packages/services/service-queue/src/db-queue-adapter.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "HAND-MIRRORS the guard already, which is the second copy of the contract this gate exists to remove — but @objectstack/service-queue does not depend on @objectstack/objectql, so replacing the copy with the producer's predicate needs a devDependency change.", + "closes": "add @objectstack/objectql to devDependencies, then replace the mirrored `if` with assertEngineDeleteDispatch(options)" + }, + { + "file": "packages/spec/src/contracts/data-engine.test.ts", + "unguarded": 1, + "kind": "EXEMPT", + "why": "Not a stand-in that code under test drives — it is a TYPE-CONFORMANCE witness that IDataEngine is implementable, asserting only `typeof engine.delete === 'function'`. And it cannot be pinned even in principle: @objectstack/objectql depends on @objectstack/spec, so the import would invert the dependency. Ran clean under the guard anyway (spec: 295/295 files passed with the dispatch guard installed in every engine double).", + "closes": "nothing — permanent" + } + ] +}