From 4e1be953d0939bdcf4c28dd0703a32cb77036eef Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 16:11:32 +0000 Subject: [PATCH 1/3] perf(plugin-security): batch the identity boot seeds' existence read and skip no-op writes Every declared permission set and every declared position cost 4 sequential DB round trips on every kernel boot, 2 of them an UPDATE that fired when nothing had changed. Hoist ONE $in existence read out of each loop and write only when the stored row actually differs. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .../batch-identity-boot-seed-round-trips.md | 55 +++ .../bootstrap-declared-permissions.test.ts | 14 +- .../src/bootstrap-declared-permissions.ts | 107 ++++- .../src/bootstrap-declared-positions.test.ts | 14 +- .../src/bootstrap-declared-positions.ts | 94 ++++- .../src/bootstrap-seed-round-trips.test.ts | 392 ++++++++++++++++++ .../src/permission-set-projection.ts | 25 +- .../plugin-security/src/security-plugin.ts | 12 +- .../plugin-security/src/seed-name-lookup.ts | 220 ++++++++++ 9 files changed, 901 insertions(+), 32 deletions(-) create mode 100644 .changeset/batch-identity-boot-seed-round-trips.md create mode 100644 packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts create mode 100644 packages/plugins/plugin-security/src/seed-name-lookup.ts diff --git a/.changeset/batch-identity-boot-seed-round-trips.md b/.changeset/batch-identity-boot-seed-round-trips.md new file mode 100644 index 0000000000..674db4afc1 --- /dev/null +++ b/.changeset/batch-identity-boot-seed-round-trips.md @@ -0,0 +1,55 @@ +--- +"@objectstack/plugin-security": minor +--- + +Batch the identity boot seeds' existence read and stop re-writing rows that +already match the declaration (#10946). + +Every permission set and every position an environment declared cost **exactly +4 sequential database round trips on every kernel boot** — measured on a real +per-environment kernel build with every `@libsql/client` call counted: slope +4.0000, R² = 1.000000 on both axes, with a per-statement histogram naming the +four legs (2 × existence `SELECT`, 1 × `UPDATE`, 1 × `SELECT`). Two of the four +were an `UPDATE` that fired even when nothing had changed. On a local file +database the loop is invisible; on a remote libsql/Turso database — every hosted +environment — each leg is its own sequential HTTP request. Schema sync had +already been batched (`TursoDriver.supports.batchSchemaSync`), which is why +objects, views and artifact seeds add 0.00 round trips each on the same rig; +identity content was the one content axis still paying per item. + +Both loops now hoist **one** `{ name: { $in: [...] } }` existence read out of the +loop — the declaration is known in full before the loop starts — and write only +when the stored row actually differs from what would be written. A steady-state +rebuild of both loops is now O(1) round trips: measured in-repo against a +call-counting ObjectQL double, a rebuild of 1, 5, 20 and 40 declared items costs +1 round trip in every case, for permission sets and positions alike. + +Three things the change is careful **not** to become: + +- **Drift still reconciles.** The skip is on equality, never on "we have seen + this name": a row whose stored value differs — a package version bump, a + hand-edit, a partially applied write — still gets its `UPDATE`. An + implementation that skipped all writes would show the same round-trip curve + and silently stop reconciling, so the round-trip pins are paired one-for-one + with drift pins over the same fixtures. +- **A read that could not answer is not the answer "none exist."** A batched + read fails for the whole set at once, so swallowing its failure into `[]` + would make every boot conclude nothing is seeded and re-create everything. The + seam is judged on whether the driver returned a result set, never on whether + the array came back empty; a failed batched read degrades to the per-item read + (loudly warned), and a name whose record cannot be read at all is declined + rather than inserted. That last step is deliberately stricter than the code it + replaces, which turned a failed read into an insert attempt and leaned on the + `name` unique index to refuse it. +- **A converged publish is still a successful publish.** `PermissionSeedOutcome` + gains `unchanged` (rows that already matched) and `unreadable` (names declined + because their record could not be read). The ADR-0086 P2 publish materializer + asks "did the record end up matching the published body", which was + accidentally identical to "was a write issued" only because the seeder always + wrote; it now reads `seeded + updated + unchanged`, so every case that reported + a materialization before still reports one. A re-publish of a byte-identical + body reports `inserted: 0, updated: 0` instead of `updated: 1` — the one + reporting difference, and the truthful reading. + +`bootstrapDeclaredPositions` likewise returns `unchanged` and `unreadable` +alongside `seeded`/`updated`. diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts b/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts index 2ae1b613cd..2cfa72f197 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts @@ -15,7 +15,19 @@ function makeQl(declared: any[] = []) { async find(object: string, q: any) { if (object !== 'sys_permission_set') return []; const where = q?.where ?? {}; - return rows.filter((r) => Object.entries(where).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); return r[k] === v; })); + // Membership is modelled because the real engine supports it and the + // #10946 boot seeders now hoist ONE `$in` existence read out of their + // loop. A double that silently answered `[]` to `$in` would report + // "nothing is seeded" and make every re-seed look like a first boot. + return rows.filter((r) => Object.entries(where).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + if (v && typeof v === 'object' && !Array.isArray(v)) { + const inList = (v as any).$in; + if (Array.isArray(inList)) return inList.includes(r[k]); + throw new Error(`fake driver: unsupported operator ${Object.keys(v).join(',')}`); + } + return r[k] === v; + })); }, async insert(object: string, data: any) { if (object !== 'sys_permission_set') return null; diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts b/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts index aa861131ca..3ad017323f 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts @@ -36,17 +36,41 @@ */ import { + SYSTEM_CTX, genId, permissionSetRowFields, - tryFind, + recordDiffersFromBody, tryInsert, tryUpdate, type PermissionSeedOutcome, type ProjectionLogger, } from './permission-set-projection.js'; +import { + buildExistingByName, + type ExistingByNameIndex, + type ExistingLookupResult, +} from './seed-name-lookup.js'; export type { PermissionSeedOutcome } from './permission-set-projection.js'; +/** + * The per-name existence read used when no batched oracle was supplied (the + * ADR-0086 P2 publish materializer, which upserts exactly one set). Reports the + * same three outcomes the batched oracle does — a failed read must not read as + * "absent" on this path either. + */ +async function defaultLookup(ql: any, name: string): Promise { + let rows: any; + try { + rows = await ql.find('sys_permission_set', { where: { name }, limit: 1 }, { context: SYSTEM_CTX }); + } catch { + return { status: 'unknown' }; + } + const list = Array.isArray(rows) ? rows : Array.isArray(rows?.records) ? rows.records : null; + if (list === null) return { status: 'unknown' }; + return list[0] ? { status: 'present', row: list[0] } : { status: 'absent' }; +} + interface SeedOptions { logger?: ProjectionLogger; } @@ -96,8 +120,17 @@ export async function upsertPackagePermissionSet( ps: any, packageId: string | null | undefined, logger?: SeedOptions['logger'], + opts?: { + /** + * Existence oracle to consult instead of this function's own per-name + * `SELECT` (#10946). The boot loop passes ONE batched read covering every + * declared name; the publish materializer, which upserts a single set, + * passes nothing and keeps the per-name read. + */ + existingByName?: ExistingByNameIndex; + }, ): Promise { - const out: PermissionSeedOutcome = { seeded: 0, updated: 0, skippedEnvAuthored: 0, skippedForeign: 0 }; + const out: PermissionSeedOutcome = { seeded: 0, updated: 0, unchanged: 0, unreadable: 0, skippedEnvAuthored: 0, skippedForeign: 0 }; if (!ps?.name) return out; // A `managed_by:'package'` row without a `package_id` would make uninstall // undefined again — the exact ambiguity ADR-0086 D3 exists to remove — so a @@ -107,17 +140,35 @@ export async function upsertPackagePermissionSet( return out; } - const existing = (await tryFind(ql, 'sys_permission_set', { name: ps.name }, 1))[0]; + // ⛔ Three outcomes, not two (#10946 / #3807). `unknown` — the read FAILED — + // is not "no such row": inserting on it would re-create a set that already + // exists, and on a batched read one failure speaks for every declared name at + // once. Declining is the answer; the caller's warn reports it. + const lookup = opts?.existingByName + ? await opts.existingByName.get(String(ps.name)) + : await defaultLookup(ql, String(ps.name)); + if (lookup.status === 'unknown') { + out.unreadable += 1; + return out; + } + const existing = lookup.status === 'present' ? lookup.row : undefined; if (!existing?.id) { - const created = await tryInsert(ql, 'sys_permission_set', { + const row = { id: genId('ps'), name: ps.name, ...permissionSetRowFields(ps), active: true, package_id: packageId, managed_by: 'package', - }); - if (created) out.seeded += 1; + }; + const created = await tryInsert(ql, 'sys_permission_set', row); + if (created) { + out.seeded += 1; + // A batched oracle is a snapshot taken before the loop — tell it about + // the row we just made, so a name declared twice in one batch still + // reaches the collision branch below instead of a second insert. + opts?.existingByName?.remember(String(ps.name), row); + } return out; } @@ -125,7 +176,23 @@ export async function upsertPackagePermissionSet( if (existing.package_id === packageId) { // Our own row — re-seed so the record always reflects the shipped/published // declaration (idempotent; covers version bumps without bookkeeping). - if (await tryUpdate(ql, 'sys_permission_set', { id: existing.id, ...permissionSetRowFields(ps) })) { + // + // [#10946] "Idempotent" was implemented as "write the same columns every + // time", which on a remote libsql/Turso database is two HTTP round trips + // per set on every boot to change nothing. `recordDiffersFromBody` is the + // SAME comparison the ADR-0094 boot reconciler already trusts to decide + // whether a record drifted, over exactly the columns + // `permissionSetRowFields` writes — so a row it calls equal is a row this + // UPDATE could not have changed. + // + // ⚠️ The skip is on EQUALITY, never on "we have seen this name". A row + // whose stored value differs — a version bump, a hand-edit, a partially + // applied write — still gets its UPDATE, because dropping that leg would + // turn the loop into a no-op that reconciles nothing while showing a + // beautiful round-trip curve. + if (!recordDiffersFromBody(existing, ps)) { + out.unchanged += 1; + } else if (await tryUpdate(ql, 'sys_permission_set', { id: existing.id, ...permissionSetRowFields(ps) })) { out.updated += 1; } } else { @@ -151,7 +218,7 @@ export async function bootstrapDeclaredPermissions( metadataService: any, options: SeedOptions = {}, ): Promise { - const out: PermissionSeedOutcome = { seeded: 0, updated: 0, skippedEnvAuthored: 0, skippedForeign: 0 }; + const out: PermissionSeedOutcome = { seeded: 0, updated: 0, unchanged: 0, unreadable: 0, skippedEnvAuthored: 0, skippedForeign: 0 }; if (!ql || typeof ql.find !== 'function' || typeof ql.insert !== 'function') return out; let sets: any[] = readDeclared(ql, 'permission'); @@ -163,18 +230,40 @@ export async function bootstrapDeclaredPermissions( } if (!Array.isArray(sets) || sets.length === 0) return out; + // [#10946] ONE existence read for the whole declaration, before the loop — + // the set of names is known in full here. See `seed-name-lookup.ts` for why + // a read that cannot ANSWER must not be read as "none of them exist". + const existingByName = await buildExistingByName( + ql, + 'sys_permission_set', + sets.map((ps) => ps?.name), + options.logger, + ); + for (const ps of sets) { if (!ps?.name) continue; // Registry provenance first (ADR-0010 `_packageId`), author-declared // spec `packageId` (ADR-0086 D3) as fallback. const packageId: string | undefined = ps._packageId ?? ps.packageId ?? undefined; - const r = await upsertPackagePermissionSet(ql, ps, packageId, options.logger); + const r = await upsertPackagePermissionSet(ql, ps, packageId, options.logger, { existingByName }); out.seeded += r.seeded; out.updated += r.updated; + out.unchanged += r.unchanged; + out.unreadable += r.unreadable; out.skippedEnvAuthored += r.skippedEnvAuthored; out.skippedForeign += r.skippedForeign; } + if (out.unreadable > 0) { + // Said once, with the count: these sets were neither seeded nor reconciled + // because the record could not be READ. Silence here would read exactly + // like "everything was already in order". + options.logger?.warn?.( + '[security] declared permission sets left untouched — their records could not be read', + { unreadable: out.unreadable, total: sets.length }, + ); + } + options.logger?.info?.('[security] declared permission sets seeded into sys_permission_set (ADR-0086 D5)', { ...out, total: sets.length, }); diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-positions.test.ts b/packages/plugins/plugin-security/src/bootstrap-declared-positions.test.ts index 2bd7bb626b..33c6e6f946 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-positions.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-positions.test.ts @@ -26,7 +26,19 @@ function makeQl(declared: any[] = []) { async find(object: string, q: any) { if (object !== 'sys_position') return []; const where = q?.where ?? {}; - return rows.filter((r) => Object.entries(where).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); return r[k] === v; })); + // Membership is modelled because the real engine supports it and the + // #10946 boot seeders now hoist ONE `$in` existence read out of their + // loop. A double that silently answered `[]` to `$in` would report + // "nothing is seeded" and make every re-seed look like a first boot. + return rows.filter((r) => Object.entries(where).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + if (v && typeof v === 'object' && !Array.isArray(v)) { + const inList = (v as any).$in; + if (Array.isArray(inList)) return inList.includes(r[k]); + throw new Error(`fake driver: unsupported operator ${Object.keys(v).join(',')}`); + } + return r[k] === v; + })); }, async insert(object: string, data: any) { if (object !== 'sys_position') return null; diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-positions.ts b/packages/plugins/plugin-security/src/bootstrap-declared-positions.ts index f056b50c34..51ec4c4b9e 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-positions.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-positions.ts @@ -17,6 +17,8 @@ * tree, not `sys_position.parent`. */ +import { buildExistingByName } from './seed-name-lookup.js'; + const SYSTEM_CTX = { isSystem: true }; function genId(prefix: string): string { @@ -25,12 +27,6 @@ function genId(prefix: string): string { return `${prefix}_${ts}${rand}`; } -async function tryFind(ql: any, object: string, where: any, limit = 100): Promise { - try { - const rows = await ql.find(object, { where, limit }, { context: SYSTEM_CTX }); - return Array.isArray(rows) ? rows : []; - } catch { return []; } -} async function tryInsert(ql: any, object: string, data: any): Promise { try { return await ql.insert(object, data, { context: SYSTEM_CTX }); } catch { return null; } } @@ -63,13 +59,28 @@ function readDeclared(engine: any, type: string): any[] { return []; } +/** + * The columns a re-seed writes. Position IDENTITY + display only: the record + * side (bindings, `active`, `is_default`, `delegatable`, `managed_by`) belongs + * to the runtime/admin and is never projected from the declaration (#2909 T2). + */ +function positionRowFields(r: any): { label: any; description: any } { + return { label: r.label ?? r.name, description: r.description ?? null }; +} + +/** True when the stored row differs from what a re-seed would write (#10946). */ +function positionRecordDiffers(row: any, fields: { label: any; description: any }): boolean { + return (row?.label ?? null) !== (fields.label ?? null) + || (row?.description ?? null) !== (fields.description ?? null); +} + export async function bootstrapDeclaredPositions( ql: any, metadataService: any, options: SeedOptions = {}, -): Promise<{ seeded: number; updated: number }> { +): Promise<{ seeded: number; updated: number; unchanged: number; unreadable: number }> { if (!ql || typeof ql.find !== 'function' || typeof ql.insert !== 'function') { - return { seeded: 0, updated: 0 }; + return { seeded: 0, updated: 0, unchanged: 0, unreadable: 0 }; } let positions: any[] = readDeclared(ql, 'position'); if (positions.length === 0) { @@ -78,23 +89,70 @@ export async function bootstrapDeclaredPositions( positions = typeof (listed as any)?.then === 'function' ? await listed : (listed ?? []); } catch { positions = []; } } - if (!Array.isArray(positions) || positions.length === 0) return { seeded: 0, updated: 0 }; + if (!Array.isArray(positions) || positions.length === 0) return { seeded: 0, updated: 0, unchanged: 0, unreadable: 0 }; + + // [#10946] ONE existence read for the whole declaration, before the loop. + // See `seed-name-lookup.ts` for why a read that cannot ANSWER must never be + // read as "none of them exist" — that conflation would re-create every + // position on every boot. + const existingByName = await buildExistingByName( + ql, + 'sys_position', + positions.map((r) => r?.name), + options.logger, + ); let seeded = 0; let updated = 0; + let unchanged = 0; + let unreadable = 0; for (const r of positions) { if (!r?.name) continue; - const fields = { label: r.label ?? r.name, description: r.description ?? null }; - const existing = await tryFind(ql, 'sys_position', { name: r.name }, 1); - if (existing[0]?.id) { - if (await tryUpdate(ql, 'sys_position', { id: existing[0].id, ...fields })) updated += 1; + const fields = positionRowFields(r); + // ⛔ Three outcomes, not two (#10946 / #3807): a read that FAILED is not + // "no such position". Inserting on it would re-create every position on + // every boot the database is briefly unreachable. + const lookup = await existingByName.get(String(r.name)); + if (lookup.status === 'unknown') { unreadable += 1; continue; } + const existing = lookup.status === 'present' ? lookup.row : undefined; + if (existing?.id) { + // [#10946] Only write when the stored row actually differs. An + // unconditional UPDATE here cost two remote round trips per position on + // every boot to store the values already there. + // + // ⚠️ EQUALITY decides, not presence: a position whose stored label or + // description drifted from the declaration still gets its UPDATE. Only + // the display fields are compared because only the display fields are + // written — the record-authoritative columns (`active`, `is_default`, + // `delegatable`, `managed_by`) are deliberately never touched by a + // re-seed (#2909 T2), so they can neither cause nor suppress one. + if (!positionRecordDiffers(existing, fields)) { + unchanged += 1; + } else if (await tryUpdate(ql, 'sys_position', { id: existing.id, ...fields })) { + updated += 1; + } } else { - const created = await tryInsert(ql, 'sys_position', { + const row = { id: genId('position'), name: r.name, ...fields, active: true, is_default: false, - }); - if (created) seeded += 1; + }; + const created = await tryInsert(ql, 'sys_position', row); + if (created) { + seeded += 1; + // The batched oracle is a snapshot taken before the loop; a name + // declared twice in one batch must resolve to the row we just made + // rather than attempting a second insert the unique index refuses. + existingByName.remember(String(r.name), row); + } } } - options.logger?.info?.('[security] declared positions seeded into sys_position', { seeded, updated, total: positions.length }); - return { seeded, updated }; + if (unreadable > 0) { + // Said once, with the count — see the sibling warn in + // `bootstrap-declared-permissions.ts`. + options.logger?.warn?.( + '[security] declared positions left untouched — their records could not be read', + { unreadable, total: positions.length }, + ); + } + options.logger?.info?.('[security] declared positions seeded into sys_position', { seeded, updated, unchanged, unreadable, total: positions.length }); + return { seeded, updated, unchanged, unreadable }; } diff --git a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts new file mode 100644 index 0000000000..04e584494e --- /dev/null +++ b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts @@ -0,0 +1,392 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10946] The identity boot seeders cost O(1) database round trips on a + * steady-state rebuild — and still reconcile. + * + * ## What is measured here, and what is NOT + * + * The defect this file pins is a **COUNT**, not a latency: every declared + * permission set and every declared position cost exactly 4 sequential database + * round trips on every kernel boot (2 × existence `SELECT`, 1 × `UPDATE`, + * 1 × `SELECT`), of which the `UPDATE` fired even when nothing had changed. + * On a local file database that loop is invisible; on a remote libsql/Turso + * database — every hosted environment — each leg is its own sequential HTTP + * request. + * + * A count is measurable without the hosted rig, so that is what these tests + * measure: every `find` / `insert` / `update` the seeder issues against the + * ObjectQL facade is one round trip, counted by {@link makeCountingQl}. + * + * ⚠️ The card's LATENCY figure (the whole `bootstrap` step growing 171.7 ms per + * ms of injected RTT, R² = 0.998) is **inherited from the hosted rig in + * `objectstack-ai/cloud`, not reproduced here** — nothing in this file measures + * wall time, and a test that did would measure the machine it ran on. + * + * ## Why the assertions are shaped the way they are + * + * A round-trip suite alone is a trap: an implementation that simply stopped + * writing would produce a perfect curve and silently stop reconciling — the + * loops would keep their shape and lose their purpose. So the counting tests + * are paired, one for one, with reconciliation tests over the same fixtures: + * a drifted row still gets its `UPDATE`, an absent name is still created, and a + * read that FAILED is never mistaken for a read that answered "none". + */ + +import { describe, it, expect } from 'vitest'; +import { bootstrapDeclaredPermissions } from './bootstrap-declared-permissions.js'; +import { bootstrapDeclaredPositions } from './bootstrap-declared-positions.js'; + +interface CountingQl { + rows: any[]; + calls: { find: number; insert: number; update: number }; + /** Every round trip in issue order — `find`/`insert`/`update`. */ + log: string[]; + /** Payloads of the `where` clauses the seeder issued, for shape assertions. */ + wheres: any[]; + roundTrips(): number; + reset(): void; + registry: { listItems: (type: string) => any[] }; + find(object: string, q: any, opts?: any): Promise; + insert(object: string, data: any, opts?: any): Promise; + update(object: string, data: any, opts?: any): Promise; +} + +/** + * An in-memory ObjectQL facade that COUNTS calls. Supports the `$in` membership + * operator, because the real engine does (`security-plugin.ts` already reads + * `sys_permission_set` with `{ name: { $in: names } }`) — a double that refused + * it would be pinning the double's limits, not the seeder's behaviour. + */ +function makeCountingQl( + object: string, + metadataType: string, + declared: any[], + behaviour: { findThrows?: boolean; findReturnsNonArray?: boolean } = {}, +): CountingQl { + const rows: any[] = []; + const matches = (row: any, where: any): boolean => + Object.entries(where ?? {}).every(([key, cond]) => { + if (cond && typeof cond === 'object' && !Array.isArray(cond)) { + const inList = (cond as any).$in; + if (Array.isArray(inList)) return inList.includes(row[key]); + throw new Error(`counting driver: unsupported operator ${Object.keys(cond).join(',')}`); + } + return row[key] === cond; + }); + + const ql: CountingQl = { + rows, + calls: { find: 0, insert: 0, update: 0 }, + log: [], + wheres: [], + roundTrips() { return this.calls.find + this.calls.insert + this.calls.update; }, + reset() { this.calls = { find: 0, insert: 0, update: 0 }; this.log = []; this.wheres = []; }, + registry: { listItems: (type: string) => (type === metadataType ? [...declared] : []) }, + async find(obj: string, q: any) { + if (obj !== object) return []; + ql.calls.find += 1; + ql.log.push('find'); + ql.wheres.push(q?.where); + if (behaviour.findThrows) throw new Error('counting driver: read unavailable'); + if (behaviour.findReturnsNonArray) return undefined as any; + return rows.filter((r) => matches(r, q?.where)); + }, + async insert(obj: string, data: any) { + if (obj !== object) return null; + ql.calls.insert += 1; + ql.log.push('insert'); + rows.push({ ...data }); + return { id: data.id }; + }, + async update(obj: string, data: any) { + if (obj !== object) return; + ql.calls.update += 1; + ql.log.push('update'); + const r = rows.find((x) => x.id === data.id); + if (r) Object.assign(r, data); + }, + }; + return ql; +} + +const declaredSets = (n: number) => + Array.from({ length: n }, (_, i) => ({ + name: `pkg_set_${i}`, + label: `Set ${i}`, + objects: { crm_lead: { allowRead: true } }, + systemPermissions: [`crm.use.${i}`], + _packageId: 'com.example.crm', + })); + +const declaredPositions = (n: number) => + Array.from({ length: n }, (_, i) => ({ + name: `pkg_pos_${i}`, + label: `Position ${i}`, + description: `desc ${i}`, + })); + +const permissionQl = (declared: any[], behaviour = {}) => + makeCountingQl('sys_permission_set', 'permission', declared, behaviour); +const positionQl = (declared: any[], behaviour = {}) => + makeCountingQl('sys_position', 'position', declared, behaviour); + +describe('#10946 — steady-state rebuild is O(1) round trips (permission sets)', () => { + it('does not grow the rebuild round-trip count with the number of declared sets', async () => { + const measure = async (n: number) => { + const ql = permissionQl(declaredSets(n)); + await bootstrapDeclaredPermissions(ql, undefined); // first boot: seeds + ql.reset(); + const r = await bootstrapDeclaredPermissions(ql, undefined); // REBUILD + expect(r.seeded).toBe(0); + expect(r.updated).toBe(0); + expect(r.unchanged).toBe(n); + return ql.roundTrips(); + }; + + const [n1, n5, n20, n40] = [await measure(1), await measure(5), await measure(20), await measure(40)]; + // The count is asserted, never the wall time. + expect([n1, n5, n20, n40]).toEqual([1, 1, 1, 1]); + }); + + it('issues ONE batched `$in` existence read for the whole declaration', async () => { + const ql = permissionQl(declaredSets(12)); + await bootstrapDeclaredPermissions(ql, undefined); + ql.reset(); + await bootstrapDeclaredPermissions(ql, undefined); + expect(ql.calls.find).toBe(1); + expect(ql.wheres[0]).toEqual({ name: { $in: declaredSets(12).map((s) => s.name) } }); + }); + + it('first boot costs one batched read plus one INSERT per genuinely new set', async () => { + const ql = permissionQl(declaredSets(10)); + const r = await bootstrapDeclaredPermissions(ql, undefined); + expect(r.seeded).toBe(10); + expect(ql.calls.find).toBe(1); + expect(ql.calls.insert).toBe(10); + expect(ql.calls.update).toBe(0); + expect(ql.rows).toHaveLength(10); + }); +}); + +describe('#10946 — steady-state rebuild is O(1) round trips (positions)', () => { + it('does not grow the rebuild round-trip count with the number of declared positions', async () => { + const measure = async (n: number) => { + const ql = positionQl(declaredPositions(n)); + await bootstrapDeclaredPositions(ql, null); + ql.reset(); + const r = await bootstrapDeclaredPositions(ql, null); + expect(r.seeded).toBe(0); + expect(r.updated).toBe(0); + expect(r.unchanged).toBe(n); + return ql.roundTrips(); + }; + + const [n1, n5, n20, n40] = [await measure(1), await measure(5), await measure(20), await measure(40)]; + expect([n1, n5, n20, n40]).toEqual([1, 1, 1, 1]); + }); + + it('issues ONE batched `$in` existence read for the whole declaration', async () => { + const ql = positionQl(declaredPositions(12)); + await bootstrapDeclaredPositions(ql, null); + ql.reset(); + await bootstrapDeclaredPositions(ql, null); + expect(ql.calls.find).toBe(1); + expect(ql.wheres[0]).toEqual({ name: { $in: declaredPositions(12).map((p) => p.name) } }); + }); +}); + +/** + * ⚠️ LOAD-BEARING. Without these, an implementation that skipped every write + * would pass every count above while reconciling nothing at all. + */ +describe('#10946 — drift STILL reconciles', () => { + it('a permission-set row whose stored grants differ still gets its UPDATE', async () => { + const ql = permissionQl(declaredSets(20)); + await bootstrapDeclaredPermissions(ql, undefined); + + // The package ships a changed declaration for exactly ONE of the 20. + const upgraded = declaredSets(20); + upgraded[7] = { ...upgraded[7], objects: { crm_lead: { allowRead: true, allowEdit: true } } }; + (ql as any).registry = { listItems: (t: string) => (t === 'permission' ? upgraded : []) }; + + ql.reset(); + const r = await bootstrapDeclaredPermissions(ql, undefined); + expect(r.updated).toBe(1); + expect(r.unchanged).toBe(19); + expect(ql.calls.update).toBe(1); + const row = ql.rows.find((x) => x.name === 'pkg_set_7'); + expect(JSON.parse(row.object_permissions)).toEqual({ crm_lead: { allowRead: true, allowEdit: true } }); + }); + + it('a permission-set row a hand-edit drifted is healed back to the declaration', async () => { + const ql = permissionQl(declaredSets(3)); + await bootstrapDeclaredPermissions(ql, undefined); + // Someone wrote straight at the row. + ql.rows[1].object_permissions = JSON.stringify({ crm_lead: { allowDelete: true } }); + + ql.reset(); + const r = await bootstrapDeclaredPermissions(ql, undefined); + expect(r.updated).toBe(1); + expect(JSON.parse(ql.rows[1].object_permissions)).toEqual({ crm_lead: { allowRead: true } }); + }); + + it('a position row whose stored label/description differ still gets its UPDATE', async () => { + const ql = positionQl(declaredPositions(20)); + await bootstrapDeclaredPositions(ql, null); + + const upgraded = declaredPositions(20); + upgraded[3] = { ...upgraded[3], label: 'Renamed', description: 'new text' }; + (ql as any).registry = { listItems: (t: string) => (t === 'position' ? upgraded : []) }; + + ql.reset(); + const r = await bootstrapDeclaredPositions(ql, null); + expect(r.updated).toBe(1); + expect(r.unchanged).toBe(19); + expect(ql.calls.update).toBe(1); + const row = ql.rows.find((x) => x.name === 'pkg_pos_3'); + expect(row.label).toBe('Renamed'); + expect(row.description).toBe('new text'); + }); + + it('a re-seed still never touches the record-authoritative columns (#2909 T2 kept)', async () => { + const ql = positionQl([{ name: 'contributor', label: 'Contributor v2', description: 'new' }]); + ql.rows.push({ + id: 'pos_1', name: 'contributor', label: 'Contributor', description: 'old', + active: false, is_default: true, delegatable: true, managed_by: 'package', + }); + await bootstrapDeclaredPositions(ql, null); + const row = ql.rows[0]; + expect(row.label).toBe('Contributor v2'); + expect(row.active).toBe(false); + expect(row.is_default).toBe(true); + expect(row.delegatable).toBe(true); + expect(row.managed_by).toBe('package'); + }); +}); + +describe('#10946 — a genuinely NEW declaration is still created', () => { + it('the batched read does not turn "absent" into "present" (permission sets)', async () => { + const ql = permissionQl(declaredSets(5)); + await bootstrapDeclaredPermissions(ql, undefined); + + const grown = [...declaredSets(5), { + name: 'pkg_set_new', label: 'New', objects: {}, _packageId: 'com.example.crm', + }]; + (ql as any).registry = { listItems: (t: string) => (t === 'permission' ? grown : []) }; + + ql.reset(); + const r = await bootstrapDeclaredPermissions(ql, undefined); + expect(r.seeded).toBe(1); + expect(r.unchanged).toBe(5); + expect(ql.rows.map((x) => x.name)).toContain('pkg_set_new'); + // one batched read + one insert — the other five cost nothing at all + expect(ql.roundTrips()).toBe(2); + }); + + it('the batched read does not turn "absent" into "present" (positions)', async () => { + const ql = positionQl(declaredPositions(5)); + await bootstrapDeclaredPositions(ql, null); + + const grown = [...declaredPositions(5), { name: 'pkg_pos_new', label: 'New', description: null }]; + (ql as any).registry = { listItems: (t: string) => (t === 'position' ? grown : []) }; + + ql.reset(); + const r = await bootstrapDeclaredPositions(ql, null); + expect(r.seeded).toBe(1); + expect(r.unchanged).toBe(5); + expect(ql.roundTrips()).toBe(2); + }); +}); + +/** + * ⛔ #3807's conflation class, at the seam a batched read newly exposes. The + * per-item shape was accidentally immune: a failed read fell through to an + * insert that failed too, for that ONE item. A batched read that swallowed its + * failure into `[]` would speak for the WHOLE set — every boot would conclude + * nothing is seeded and try to re-create everything. + * + * The judgement is "did the driver return a result set", never "is the array + * empty": an empty array is the answer "none of these names exist", and the + * first-boot tests above depend on that answer being trusted. + */ +describe('#10946 — a read that CANNOT ANSWER is not the answer "none exist"', () => { + it('a throwing read does NOT re-create rows that are already seeded (permission sets)', async () => { + const ql = permissionQl(declaredSets(4)); + await bootstrapDeclaredPermissions(ql, undefined); + expect(ql.rows).toHaveLength(4); + + // Every read now fails — the batched one and the per-item fallback alike. + const broken = permissionQl(declaredSets(4), { findThrows: true }); + broken.rows.push(...ql.rows.map((r) => ({ ...r }))); + const warns: string[] = []; + const r = await bootstrapDeclaredPermissions(broken, undefined, { + logger: { info: () => {}, warn: (m) => warns.push(m) }, + }); + + expect(r.seeded).toBe(0); + expect(r.unreadable).toBe(4); + expect(broken.calls.insert).toBe(0); // ⛔ no blind insert + expect(broken.rows).toHaveLength(4); // ⛔ nothing re-created + expect(warns.some((w) => w.includes('batched seed existence read failed'))).toBe(true); + expect(warns.some((w) => w.includes('could not be read'))).toBe(true); + }); + + it('a read returning a non-result (undefined) is not read as "none exist"', async () => { + const ql = permissionQl(declaredSets(4), { findReturnsNonArray: true }); + ql.rows.push(...declaredSets(4).map((s, i) => ({ + id: `ps_${i}`, name: s.name, managed_by: 'package', package_id: 'com.example.crm', + label: s.label, description: null, + object_permissions: '{}', field_permissions: '{}', system_permissions: '[]', + row_level_security: '[]', tab_permissions: '{}', admin_scope: null, + }))); + const r = await bootstrapDeclaredPermissions(ql, undefined); + expect(r.seeded).toBe(0); + expect(r.unreadable).toBe(4); + expect(ql.calls.insert).toBe(0); + expect(ql.rows).toHaveLength(4); + }); + + it('a throwing read does NOT re-create rows that are already seeded (positions)', async () => { + const seeded = positionQl(declaredPositions(4)); + await bootstrapDeclaredPositions(seeded, null); + + const broken = positionQl(declaredPositions(4), { findThrows: true }); + broken.rows.push(...seeded.rows.map((r) => ({ ...r }))); + const r = await bootstrapDeclaredPositions(broken, null); + expect(r.seeded).toBe(0); + expect(r.unreadable).toBe(4); + expect(broken.calls.insert).toBe(0); + expect(broken.rows).toHaveLength(4); + }); + + it('an EMPTY result set is still trusted as "none exist" — the first boot depends on it', async () => { + const ql = permissionQl(declaredSets(3)); + const r = await bootstrapDeclaredPermissions(ql, undefined); + expect(r.seeded).toBe(3); + }); +}); + +/** + * The batched oracle is a snapshot taken before the loop. Without the + * `remember` write-back, a name declared twice in one batch would take the + * INSERT branch the second time — and the loud ADR-0086 D4 refusal it used to + * produce would become a unique-index rejection nobody reports. + */ +describe('#10946 — a name declared twice in one batch keeps its loud refusal', () => { + it('still reports skippedForeign for a second package declaring the same name', async () => { + const ql = permissionQl([ + { name: 'shared_name', label: 'A', objects: {}, _packageId: 'com.example.a' }, + { name: 'shared_name', label: 'B', objects: {}, _packageId: 'com.example.b' }, + ]); + const warns: string[] = []; + const r = await bootstrapDeclaredPermissions(ql, undefined, { + logger: { info: () => {}, warn: (m) => warns.push(m) }, + }); + expect(r.seeded).toBe(1); + expect(r.skippedForeign).toBe(1); + expect(ql.rows).toHaveLength(1); + expect(ql.rows[0].package_id).toBe('com.example.a'); + expect(warns.some((w) => w.includes('owned by another package'))).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-security/src/permission-set-projection.ts b/packages/plugins/plugin-security/src/permission-set-projection.ts index 55bdd0bf00..5ea4e4e9b4 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.ts @@ -132,6 +132,27 @@ export interface ProjectionLogger { export interface PermissionSeedOutcome { seeded: number; updated: number; + /** + * Rows this seeder OWNS whose stored columns already equalled what it would + * have written, so no `UPDATE` was issued (#10946). + * + * ⚠️ Read this together with {@link updated}, never instead of it. "The row + * now matches the declaration" is `seeded + updated + unchanged`; `updated` + * alone means "a write was needed AND landed". A consumer that asks the + * first question while reading only the second reports a converged row as a + * failure — which is exactly what the ADR-0086 P2 publish materializer did + * before this counter existed, because an unconditional re-write made the + * two questions accidentally identical. They are not, and the boot loops + * that stopped re-writing unchanged rows are why. + */ + unchanged: number; + /** + * Names this pass DECLINED to touch because their record could not be read + * (#10946). Distinct from `unchanged` in every way that matters: nothing was + * compared, nothing was reconciled, and the declaration may or may not be + * materialized. Never fold it into a success count. + */ + unreadable: number; skippedEnvAuthored: number; skippedForeign: number; /** Records retired because their definition was deleted from metadata. */ @@ -405,7 +426,7 @@ export async function upsertEnvPermissionSet( _logger?: ProjectionLogger, opts?: { customized?: boolean }, ): Promise { - const out: PermissionSeedOutcome = { seeded: 0, updated: 0, skippedEnvAuthored: 0, skippedForeign: 0 }; + const out: PermissionSeedOutcome = { seeded: 0, updated: 0, unchanged: 0, unreadable: 0, skippedEnvAuthored: 0, skippedForeign: 0 }; if (!ql || typeof ql.find !== 'function' || !ps?.name) return out; // [ADR-0094] `customized` marks a PACKAGE-owned row that an env overlay is @@ -517,7 +538,7 @@ async function retirePermissionSetRecord( name: string, logger?: ProjectionLogger, ): Promise { - const out: PermissionSeedOutcome = { seeded: 0, updated: 0, skippedEnvAuthored: 0, skippedForeign: 0, deleted: 0 }; + const out: PermissionSeedOutcome = { seeded: 0, updated: 0, unchanged: 0, unreadable: 0, skippedEnvAuthored: 0, skippedForeign: 0, deleted: 0 }; const existing = (await tryFind(ql, 'sys_permission_set', { name }, 1))[0]; if (!existing?.id) return out; if (existing.managed_by === 'package') { diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 3a2ed20f24..60f4895915 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -2837,7 +2837,17 @@ export class SecurityPlugin implements Plugin { 'permission', async (args: { body: unknown; packageId: string | null; organizationId: string | null }) => { const r = await upsertPackagePermissionSet(ql, args.body, args.packageId, ctx.logger); - const applied = r.seeded + r.updated; + // [#10946] `unchanged` counts here. The question this line asks + // is "did the record end up matching the published body", NOT + // "was a write issued" — and until the seeder stopped re-writing + // rows it had nothing to change, those two were accidentally the + // same number. A re-publish of an identical body now lands in + // `unchanged`, and reading only `seeded + updated` would report + // that converged publish as a FAILURE and skip the audience + // reconcile below. The three refusal branches (foreign package, + // env-authored name, no owning package) all leave `unchanged` at + // 0, so every case that reported 0 before still reports 0. + const applied = r.seeded + r.updated + r.unchanged; // [ADR-0090 D5] A published set carrying the install-time // suggestion flag surfaces (or retires) its pending // suggestion row right away — same convergent sync as boot. diff --git a/packages/plugins/plugin-security/src/seed-name-lookup.ts b/packages/plugins/plugin-security/src/seed-name-lookup.ts new file mode 100644 index 0000000000..d94955caf0 --- /dev/null +++ b/packages/plugins/plugin-security/src/seed-name-lookup.ts @@ -0,0 +1,220 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * ONE batched existence read for a boot seeder whose input set is known in + * full before its loop starts (#10946). + * + * ## Why this exists + * + * `bootstrapDeclaredPermissions` and `bootstrapDeclaredPositions` are + * read-then-write reconcilers over a list the caller already holds. Written + * as a per-item `SELECT … WHERE name = ? LIMIT 1` inside a `for await` loop, + * each declared item costs its own database ROUND TRIP — invisible on a local + * file database, one sequential HTTP request per leg on a remote libsql/Turso + * database, i.e. on every hosted environment. Measured on a real per-environment + * kernel build with every `@libsql/client` call counted, the two loops together + * grew a REBUILD (tables present, rows already seeded, nothing to change) by + * **exactly 4.0000 round trips per declared item, R² = 1.000000** on both axes. + * + * Schema sync had already been batched (`TursoDriver.supports.batchSchemaSync`), + * which is why objects/views/artifact seeds add 0.00 round trips each on the + * same rig; identity content was the one content axis still paying per item. + * + * ## The seam that has to be judged carefully + * + * ⛔ **A read that CANNOT ANSWER is not the answer "none of them exist."** That + * conflation is the whole risk of hoisting the read: one swallowed failure + * would make every boot conclude that nothing is seeded and re-create + * everything — a far worse defect than the round trips being removed. The + * per-item shape was accidentally immune to it (a failed read fell through to + * an insert that failed too, for that one item only); a batched read is not, + * because one failure now speaks for the entire set. + * + * So the seam is judged on **whether the driver returned a result set**, never + * on whether the array came back empty: + * + * - a thrown read → could not answer + * - a response that is neither an array + * nor `{ records: [...] }` → could not answer + * - `[]` → ANSWERED: none of these names exist + * + * "Could not answer" degrades — loudly warned — to the per-item read the loops + * used before, which is the case that matters for a driver that simply does not + * do `$in`: everything then proceeds exactly as it did pre-#10946, only slower. + * When the per-item read cannot answer either (the database is genuinely + * unreachable), the oracle reports {@link ExistingLookupResult} `unknown` and + * the seeder declines to touch that name at all. + * + * ⚠️ That last step is STRICTER than the code it replaces, deliberately. The old + * loop turned a failed read into an insert attempt and relied on the `name` + * unique index to refuse it — a database constraint standing in for a decision + * the seeder should have been making. On any deployment where that index is + * absent or not yet created, the old shape DUPLICATED rows instead of declining. + * + * None of this is a lenient fallback for off-contract input (AGENTS.md Prime + * Directive #12): the batched and per-item reads ask the driver the same + * question, and the answer has one meaning. + * + * ## Chunking + * + * `$in` binds one parameter per name, and SQLite builds cap bound parameters + * (`SQLITE_MAX_VARIABLE_NUMBER`, historically 999). {@link NAME_CHUNK_SIZE} + * keeps a single read well under every such cap, so the cost is + * `ceil(N / 500)` reads — constant for every realistic declaration count and, + * unlike an unchunked read, incapable of turning a large environment's boot + * into a hard driver error. + */ + +const SYSTEM_CTX = { isSystem: true }; + +/** Names bound into one `$in` read. See the chunking note in the module header. */ +export const NAME_CHUNK_SIZE = 500; + +export interface SeedLookupLogger { + info?: (m: string, meta?: Record) => void; + warn?: (m: string, meta?: Record) => void; +} + +/** + * What the oracle knows about one name. THREE outcomes, not two: `absent` is a + * fact the driver reported, `unknown` is the absence of any fact at all. + * + * ⛔ Collapsing `unknown` into `absent` is the whole hazard of hoisting the + * read, and the reason this is a union rather than `row | undefined`. A caller + * that treats "I could not find out" as "it is not there" INSERTS — and does so + * for every name at once, because a batched read fails for the whole set. The + * per-item shape hid this behind the unique index (the blind insert was refused + * by the database, which is a guard, not a design); the tri-state makes the + * seeder decline on its own. + */ +export type ExistingLookupResult = + | { status: 'present'; row: any } + | { status: 'absent' } + | { status: 'unknown' }; + +const ABSENT: ExistingLookupResult = { status: 'absent' }; +const UNKNOWN: ExistingLookupResult = { status: 'unknown' }; + +/** + * The existence oracle a seed loop consults in place of its own per-item read. + * + * ⚠️ {@link ExistingByNameIndex.remember} is not an optimization — it is what + * keeps hoisting the read out of the loop behaviour-preserving. The per-item + * read saw rows the SAME loop had just inserted, so a name declared twice in + * one batch resolved as present on the second pass and took the caller's + * collision branch (which, for permission sets, is the loud ADR-0086 D4 + * "owned by another package" refusal). A snapshot taken before the loop cannot + * see those inserts: without `remember`, the second declaration would attempt + * an insert instead, the unique index would refuse it, and a refusal that used + * to be reported would become a silent nothing. So every caller that inserts + * records the row it created. + */ +export interface ExistingByNameIndex { + /** What is known about `name` — see {@link ExistingLookupResult}. */ + get(name: string): Promise; + /** Record a row the calling loop just created under `name`. */ + remember(name: string, row: any): void; +} + +/** + * Read one page of names. Returns `null` — distinct from `[]` — when the driver + * did not return a result set at all. + */ +async function readNamePage(ql: any, object: string, names: string[]): Promise { + let rows: any; + try { + rows = await ql.find( + object, + { where: { name: { $in: names } }, limit: names.length }, + { context: SYSTEM_CTX }, + ); + } catch { + return null; + } + if (Array.isArray(rows)) return rows; + // Some drivers wrap the page (`{ records }`) — a wrapped array is still an + // answer. Anything else (undefined/null/a scalar) is not. + if (Array.isArray(rows?.records)) return rows.records as any[]; + return null; +} + +/** + * The per-item read the loops used before #10946 — the degradation path. + * + * `remember` is a deliberate no-op: this oracle re-reads the database on every + * call, so it already sees rows the loop inserted a moment ago. + */ +function perItemIndex(ql: any, object: string): ExistingByNameIndex { + return { + async get(name: string): Promise { + let rows: any; + try { + rows = await ql.find(object, { where: { name }, limit: 1 }, { context: SYSTEM_CTX }); + } catch { + return UNKNOWN; + } + const list = Array.isArray(rows) ? rows : Array.isArray(rows?.records) ? rows.records : null; + if (list === null) return UNKNOWN; + return list[0] ? { status: 'present', row: list[0] } : ABSENT; + }, + remember() { /* re-read every call — nothing to cache */ }, + }; +} + +/** + * Build the existence lookup a seed loop should use: ONE batched read for the + * whole name set, degrading to the per-item read when that read cannot answer. + * + * `names` may contain duplicates and blanks; both are dropped before the read. + */ +export async function buildExistingByName( + ql: any, + object: string, + names: readonly (string | null | undefined)[], + logger?: SeedLookupLogger, +): Promise { + const index = new Map(); + const fromIndex: ExistingByNameIndex = { + async get(name: string): Promise { + const row = index.get(name); + // The batched read ANSWERED for every requested name, so a miss here is + // the driver's own "no such row", not a gap in what we know. + return row ? { status: 'present', row } : ABSENT; + }, + remember(name: string, row: any) { + if (name && row && !index.has(name)) index.set(name, row); + }, + }; + + const wanted: string[] = []; + const seen = new Set(); + for (const raw of names) { + if (raw == null) continue; + const name = String(raw); + if (!name || seen.has(name)) continue; + seen.add(name); + wanted.push(name); + } + if (wanted.length === 0) return fromIndex; + + for (let i = 0; i < wanted.length; i += NAME_CHUNK_SIZE) { + const page = await readNamePage(ql, object, wanted.slice(i, i + NAME_CHUNK_SIZE)); + if (page === null) { + // ⛔ NOT "none of them exist" — see the module header. Fall back to the + // per-item read so behaviour is exactly what it was before the hoist. + logger?.warn?.( + '[security] batched seed existence read failed — falling back to one read per item', + { object, names: wanted.length }, + ); + return perItemIndex(ql, object); + } + for (const row of page) { + const name = row?.name; + if (name == null) continue; + // First row wins: the caller's own uniqueness rules decide what a + // duplicate name means, and this read must not reorder that judgement. + if (!index.has(String(name))) index.set(String(name), row); + } + } + return fromIndex; +} From 7f91d96e4fbfb13b82734d3f5d5ea63ecabe438a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 17:06:14 +0000 Subject: [PATCH 2/3] test(plugin-security): pin the counting double to ObjectQL.update's dispatch predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check:engine-double-contract and check:where-matcher both reddened on the new counting double: its update() did not route through assertEngineUpdateDispatch, and its WHERE matcher read a combinator as a field name. Fixed in the double — the shrink-only baseline is untouched; only the pinned (tightening) ledger grew. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .../src/bootstrap-seed-round-trips.test.ts | 23 +++++++++++++++---- scripts/engine-double-contract.pinned.json | 5 ++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts index 04e584494e..c057e44b54 100644 --- a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts @@ -34,6 +34,7 @@ */ import { describe, it, expect } from 'vitest'; +import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; import { bootstrapDeclaredPermissions } from './bootstrap-declared-permissions.js'; import { bootstrapDeclaredPositions } from './bootstrap-declared-positions.js'; @@ -49,7 +50,7 @@ interface CountingQl { registry: { listItems: (type: string) => any[] }; find(object: string, q: any, opts?: any): Promise; insert(object: string, data: any, opts?: any): Promise; - update(object: string, data: any, opts?: any): Promise; + update(object: string, data: any, options?: any): Promise; } /** @@ -67,6 +68,13 @@ function makeCountingQl( const rows: any[] = []; const matches = (row: any, where: any): boolean => Object.entries(where ?? {}).every(([key, cond]) => { + // REFUSE the combinators this double does not implement rather than + // reading `$and`/`$or` as a column name — a matcher that silently treats + // a combinator as a field is how a fake quietly answers a question the + // real engine would have answered differently. + if (key.startsWith('$')) { + throw new Error(`counting driver: unsupported combinator ${key}`); + } if (cond && typeof cond === 'object' && !Array.isArray(cond)) { const inList = (cond as any).$in; if (Array.isArray(inList)) return inList.includes(row[key]); @@ -99,12 +107,19 @@ function makeCountingQl( rows.push({ ...data }); return { id: data.id }; }, - async update(obj: string, data: any) { + // Routed through the real dispatch predicate: a fake looser than + // ObjectQL.update would let the seeder drift to a call shape the engine + // refuses while this suite stayed green. + async update(obj: string, data: any, options?: any) { if (obj !== object) return; ql.calls.update += 1; ql.log.push('update'); - const r = rows.find((x) => x.id === data.id); - if (r) Object.assign(r, data); + const dispatch = assertEngineUpdateDispatch(data, options); + const targets = dispatch.kind === 'by-id' + ? rows.filter((r) => r.id === dispatch.id) + : rows.filter((r) => matches(r, options?.where)); + for (const r of targets) Object.assign(r, data); + return dispatch.kind === 'by-id' ? (targets[0] ?? null) : targets.length; }, }; return ql; diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 2cce7bbeb9..8c2881286a 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -1376,6 +1376,11 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-security/src/check-only-write-scope.test.ts", "verb": "delete", From 4568d6fff2c3cf50d627dc94fa14fc4afe8b1822 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 17:19:36 +0000 Subject: [PATCH 3/3] test(plugin-security): type the seed fixture so the TEST_DEBT ratchet stays at 11 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check:type-check-debt --re-measure went 11 -> 12: the upgrade fixture widens a grant literal, which the inferred type rejects (TS2353). Fixed by declaring the fixture's type — the ledger entry is untouched. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .../src/bootstrap-seed-round-trips.test.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts index c057e44b54..a857d08443 100644 --- a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts @@ -125,7 +125,20 @@ function makeCountingQl( return ql; } -const declaredSets = (n: number) => +/** + * `objects` is typed rather than inferred: the upgrade fixtures below widen a + * grant (`{ allowRead }` -> `{ allowRead, allowEdit }`) to simulate a package + * version bump, which an inferred literal type rejects. + */ +interface DeclaredSet { + name: string; + label: string; + objects: Record>; + systemPermissions: string[]; + _packageId: string; +} + +const declaredSets = (n: number): DeclaredSet[] => Array.from({ length: n }, (_, i) => ({ name: `pkg_set_${i}`, label: `Set ${i}`,