From 9ac55b76db40a1baecc420c4f74547aa9b41cd97 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 19:33:10 +0000 Subject: [PATCH 1/6] perf(plugin-security): batch the declared-capability boot seed's existence read and skip no-op writes Refs #11096 --- .../src/bootstrap-declared-capabilities.ts | 165 ++++++++++++++++-- 1 file changed, 155 insertions(+), 10 deletions(-) diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.ts b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.ts index c8fd73fe32..f970252ebd 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.ts @@ -38,6 +38,24 @@ * returned so the caller can tell `bootstrapSystemCapabilities` to SKIP * re-deriving (and thus clobbering) a capability that already has a row. * + * ## Cost shape (#11096) + * + * This pass is a read-then-write reconciler over a set known IN FULL before its + * loop starts, so it pays ONE batched existence read for the whole declaration + * (`buildExistingByName`, added by #10946) and writes only where the stored row + * actually differs. It used to issue a `SELECT … WHERE name = ? LIMIT 1` per + * declared capability plus an `UPDATE` that fired whether or not anything had + * changed — the same shape whose cost was measured at 4.0000 round trips per + * item (R² = 1.000000) on the two sibling loops in #10946. That is a COUNT, and + * the count is what `bootstrap-seed-round-trips.test.ts` pins; the hosted + * latency rig lives in `objectstack-ai/cloud` and this axis' own slope has + * never been measured. + * + * ⛔ The write-skip is an EQUALITY test and must stay one. A reconciler that + * skipped writes outright would show a perfect round-trip curve while + * reconciling nothing at all, which is why every counting test in that file is + * paired one-for-one with a drift test over the same fixture. + * * [#4967 Part 1] That list reports names this pass CONFIRMED have a row — NOT * every name it read. The two are different facts, and conflating them turned a * REFUSED declaration into a hole: the refusal writes no row, the reported name @@ -53,11 +71,11 @@ import { genId, - tryFind, tryInsert, tryUpdate, type ProjectionLogger, } from './permission-set-projection.js'; +import { buildExistingByName, type ExistingByNameIndex } from './seed-name-lookup.js'; import { readDeclared } from './bootstrap-declared-permissions.js'; import { PLATFORM_CAPABILITY_NAMES } from '@objectstack/spec/security'; @@ -93,6 +111,32 @@ export interface CapabilitySeedOutcome { * not be reconciled. */ skippedUnowned: number; + /** + * [#11096] Rows this seeder OWNS whose stored `label`/`description`/`scope` + * already equalled what a re-seed would write, so no `UPDATE` was issued. + * + * ⚠️ Read together with {@link updated}, never instead of it — "the row now + * reflects the declaration" is `seeded + updated + claimed + unchanged`, while + * `updated` alone means "a write was needed AND landed". The unconditional + * re-write this counter replaces made the two questions accidentally + * identical; they are not. + */ + unchanged: number; + /** + * [#11096] Declarations this pass DECLINED to touch because `sys_capability` + * could not be READ for them. Distinct from every other counter here: + * nothing was compared, nothing was written, and whether a row exists is + * simply unknown — which is why such a name is also kept OUT of + * {@link materializedNames} (suppressing the derivation for a name that may + * have no row is the #4967 hole). + * + * ⛔ It is NOT a variety of `skippedUnowned`. Before the batched read, a + * declaration whose read failed fell through to an INSERT that the `name` + * unique index refused — a database constraint standing in for a decision + * this seeder should be making. It now declines on its own; on a deployment + * where that index is absent the old shape DUPLICATED rows instead. + */ + unreadable: number; /** * [#4967 Part 1] Names this pass CONFIRMED are materialized in * `sys_capability`, i.e. the names `bootstrapSystemCapabilities` must NOT @@ -102,8 +146,9 @@ export interface CapabilitySeedOutcome { * pass owns) — and NOT when a refusal left the name with no row anywhere. * * Accounting: every named declaration falls into exactly one counter, so - * `seeded + updated + claimed + skippedAdmin + skippedForeign + - * skippedPlatform + skippedUnowned` is the number of named declarations read, + * `seeded + updated + claimed + unchanged + skippedAdmin + skippedForeign + + * skippedPlatform + skippedUnowned + unreadable` is the number of named + * declarations read, * and `materializedNames` is that same set minus the refusals that found no * row. (Pre-existing caveat, unchanged: a write the engine rejects increments * no counter — and a rejected INSERT deliberately keeps its name out of this @@ -125,6 +170,23 @@ function capabilityRowFields(cap: any): { label: string; description: string; sc }; } +/** + * [#11096] True when the stored row differs from what a re-seed would write. + * + * ⚠️ EQUALITY decides, not presence: a capability whose stored label, + * description or scope drifted from the declaration STILL gets its `UPDATE`. + * The comparison covers exactly the columns {@link capabilityRowFields} + * writes and no others — `active`, `managed_by` and `package_id` are never + * touched by an own-row re-seed, so they can neither cause nor suppress one. + * Widening this set without widening the write (or the reverse) is how a + * "batched" reconciler turns into one that reconciles nothing. + */ +function capabilityRecordDiffers(row: any, fields: ReturnType): boolean { + return (row?.label ?? null) !== (fields.label ?? null) + || (row?.description ?? null) !== (fields.description ?? null) + || (row?.scope ?? null) !== (fields.scope ?? null); +} + /** * [#4967 Part 3] Index `capability name → granting permission set name(s)` over * the bootstrap permission sets, so a refusal can name the grantor(s) — and, @@ -194,6 +256,13 @@ async function upsertPackageCapability( packageId: string | null | undefined, out: CapabilitySeedOutcome, grantors: readonly string[], + /** + * [#11096] The ONE batched existence read for the whole declaration, built + * before the loop. Replaces this function's own per-item `SELECT`; see + * `seed-name-lookup.ts` for why its third outcome (`unknown`) exists and must + * never be collapsed into `absent`. + */ + existingByName: ExistingByNameIndex, logger?: ProjectionLogger, ): Promise { if (!cap?.name) return false; @@ -207,10 +276,26 @@ async function upsertPackageCapability( } const fields = capabilityRowFields(cap); - const existing = (await tryFind(ql, 'sys_capability', { name: cap.name }, 1))[0]; + // [#11096] ⛔ THREE outcomes, not two. `unknown` is the absence of any fact — + // the read did not answer — and is not the fact "no such capability". The + // provenance branches below every one of them turn on WHETHER A ROW WAS + // FOUND (the refusal messages state a different consequence for each), so a + // batched read that reported "not found" for a read it never got an answer + // to would make every one of those diagnostics lie. + const lookup = await existingByName.get(String(cap.name)); + if (lookup.status === 'unknown') { + out.unreadable += 1; + return false; + } + const existing = lookup.status === 'present' ? lookup.row : undefined; // A `managed_by:'package'` row without a `package_id` makes uninstall // undefined (the ambiguity ADR-0086 D3 removes) — skip an unowned declaration. + // + // ⚠️ The `hasRow` argument below is why the lookup happens BEFORE this + // refusal rather than after it: the diagnostic's consequence clause differs + // by whether a row already resolves the name. It is a definite answer here, + // never a swallowed failure — `unknown` returned above. if (!packageId) { out.skippedUnowned += 1; logger?.warn?.(unownedRefusalMessage(cap.name, grantors, Boolean(existing?.id)), { @@ -221,15 +306,26 @@ async function upsertPackageCapability( } if (!existing?.id) { - const created = await tryInsert(ql, 'sys_capability', { + const row = { id: genId('cap'), name: cap.name, ...fields, managed_by: 'package', package_id: packageId, active: true, - }); - if (created) out.seeded += 1; + }; + const created = await tryInsert(ql, 'sys_capability', row); + if (created) { + out.seeded += 1; + // [#11096] The oracle is a SNAPSHOT taken before the loop, so it cannot + // see this insert. The per-item read could, and a name declared twice in + // one batch therefore resolved as present on the second pass and took the + // loud `skippedForeign` refusal below. Without this write-back the second + // declaration would attempt its own insert, the `name` unique index would + // refuse it, and a refusal that used to be REPORTED would become a silent + // nothing. + existingByName.remember(String(cap.name), row); + } // A rejected insert leaves no row — fall through so the derivation gets its // own attempt rather than the name landing in a hole. return Boolean(created); @@ -238,7 +334,23 @@ async function upsertPackageCapability( if (existing.managed_by === 'package') { if (existing.package_id === packageId) { // Our own row — re-seed so it always reflects the shipped declaration. - if (await tryUpdate(ql, 'sys_capability', { id: existing.id, ...fields })) out.updated += 1; + // + // [#11096] ⚠️ Only when the stored row ACTUALLY DIFFERS. An unconditional + // UPDATE here cost one remote round trip per declared capability on every + // boot to store the values already there — and the capability set is the + // union of every capability every package declares, so it is typically + // the largest of the identity axes. + // + // ⛔ The skip is an EQUALITY test, never a presence test. Deleting the + // `else if` and always taking the `unchanged` arm would produce a + // perfect round-trip count while this seeder silently stopped + // reconciling anything — the failure shape the paired drift tests in + // `bootstrap-seed-round-trips.test.ts` exist to make impossible. + if (!capabilityRecordDiffers(existing, fields)) { + out.unchanged += 1; + } else if (await tryUpdate(ql, 'sys_capability', { id: existing.id, ...fields })) { + out.updated += 1; + } } else { out.skippedForeign += 1; logger?.warn?.('[security] capability name owned by another package — skipped', { @@ -270,7 +382,7 @@ export async function bootstrapDeclaredCapabilities( options: SeedOptions = {}, ): Promise { const out: CapabilitySeedOutcome = { - seeded: 0, updated: 0, claimed: 0, + seeded: 0, updated: 0, claimed: 0, unchanged: 0, unreadable: 0, skippedAdmin: 0, skippedForeign: 0, skippedPlatform: 0, skippedUnowned: 0, materializedNames: [], }; @@ -287,19 +399,52 @@ export async function bootstrapDeclaredCapabilities( const grantorsByCapability = indexGrantors(options.permissionSets); + // [#11096] ONE existence read for the whole declaration, hoisted out of the + // loop below. Each declared capability used to cost its own sequential + // `SELECT … WHERE name = ? LIMIT 1` — invisible on a local file database, one + // separate awaited HTTP request per name on the remote libsql/Turso database + // every hosted environment runs, competing for the same request budget as the + // rest of boot. + // + // ⚠️ NOT threaded with an organization, deliberately: this seeder's read, its + // insert and its update are all organization-less today (unlike the position + // and permission-set doors), so an unscoped lookup is EXACTLY the question the + // per-item read asked — `resolveOwnOrganizationRow` returns the first row when + // no organization is given, which is what `tryFind(…, 1)[0]` returned. Making + // this catalog per-organization is a separate change with its own migration, + // not a rider on a round-trip fix. + const existingByName = await buildExistingByName( + ql, + 'sys_capability', + caps.map((c) => c?.name), + options.logger, + ); + for (const cap of caps) { if (!cap?.name) continue; // Registry provenance first (ADR-0010 `_packageId`), author-declared // spec `packageId` (ADR-0086 D3) as fallback. const packageId: string | undefined = cap._packageId ?? cap.packageId ?? undefined; const grantors = grantorsByCapability.get(cap.name) ?? []; - const materialized = await upsertPackageCapability(ql, cap, packageId, out, grantors, options.logger); + const materialized = await upsertPackageCapability(ql, cap, packageId, out, grantors, existingByName, options.logger); // [#4967 Part 1] Report the name ONLY once this pass knows a row exists for // it. Reporting it before the upsert decided anything is what let a refused // declaration suppress the derivation it needed. if (materialized) out.materializedNames.push(cap.name); } + if (out.unreadable > 0) { + // [#11096] Said ONCE with the count, like the sibling seeders: a per-name + // warn on a database that is down is a log flood that buries its own + // meaning. The consequence is spelled out because "unreadable" alone does + // not state one — these names were left ENTIRELY alone, so a genuinely new + // declaration among them has not been created and a drifted one has not + // been healed; the next boot with a readable database does both. + options.logger?.warn?.( + '[security] declared capabilities left untouched — their sys_capability rows could not be read', + { unreadable: out.unreadable, total: caps.length }, + ); + } options.logger?.info?.('[security] declared capabilities seeded into sys_capability (ADR-0066 D1)', { ...out, total: caps.length, }); From 69853ea117c25847ebeb44a15d0254498ca4ea13 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 19:46:34 +0000 Subject: [PATCH 2/6] perf(plugin-security): batch the env overlay boot reconciler and skip no-op writes Refs #11097 --- .../bootstrap-declared-capabilities.test.ts | 17 +- .../src/bootstrap-seed-round-trips.test.ts | 303 +++++++++++++++ .../src/permission-set-projection.test.ts | 346 +++++++++++++++++- .../src/permission-set-projection.ts | 155 +++++++- 4 files changed, 810 insertions(+), 11 deletions(-) diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts index 8b90c57fbe..4978a0b93d 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts @@ -29,8 +29,23 @@ function makeQl(declared: any[] = []) { // (called by several cases below) scopes its curated lookup with // `organization_id: null`, which strict `===` would make unsatisfiable // here while it works in production. + // [#11096] `$in` is supported because the real engine supports it — the + // seeder now issues ONE batched `{ name: { $in: [...] } }` existence read + // for the whole declaration. ⛔ A double that did not match it would + // return `[]`, which this seeder is REQUIRED to trust as the answer "none + // of these names exist" — so every provenance case below would silently + // become a first-boot insert while the suite reported green. That is the + // double's limits masquerading as the seeder's behaviour. return rows.filter((r) => - Object.entries(where).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); return (v === null ? r[k] == null : r[k] === v); }), + 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 (v === null ? r[k] == null : r[k] === v); + }), ); }, async insert(object: string, data: any) { 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 a857d08443..ca45480f1c 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 @@ -10,6 +10,13 @@ * 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. + * + * [#11096] The declared-CAPABILITY seeder next door had the same shape and is + * pinned here too. ⚠️ Its own slope has never been measured — the hosted rig's + * axes are permission sets / positions / objects — so nothing below claims one; + * what is established is that the code shape is identical, and the capability + * set is typically the LARGEST of the identity axes because it is the union of + * every capability every declared package contributes. * 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. @@ -37,6 +44,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'; +import { bootstrapDeclaredCapabilities } from './bootstrap-declared-capabilities.js'; interface CountingQl { rows: any[]; @@ -418,3 +426,298 @@ describe('#10946 — a name declared twice in one batch keeps its loud refusal', expect(warns.some((w) => w.includes('owned by another package'))).toBe(true); }); }); + +// ── #11096 — declared capabilities ───────────────────────────────────────── + +const declaredCaps = (n: number) => + Array.from({ length: n }, (_, i) => ({ + // ⚠️ Never a curated `PLATFORM_CAPABILITY_NAMES` entry: those are refused + // before the existence read is even consulted, so a curated fixture would + // measure the refusal path and report zero round trips for the wrong reason. + name: `crm.cap.${i}`, + label: `Capability ${i}`, + description: `desc ${i}`, + scope: 'platform' as const, + _packageId: 'com.example.crm', + })); + +const capabilityQl = (declared: any[], behaviour = {}) => + makeCountingQl('sys_capability', 'capability', declared, behaviour); + +describe('#11096 — steady-state rebuild is O(1) round trips (declared capabilities)', () => { + it('does not grow the rebuild round-trip count with the number of declared capabilities', async () => { + const measure = async (n: number) => { + const ql = capabilityQl(declaredCaps(n)); + await bootstrapDeclaredCapabilities(ql, undefined); // first boot: seeds + ql.reset(); + const r = await bootstrapDeclaredCapabilities(ql, undefined); // REBUILD + expect(r.seeded).toBe(0); + expect(r.updated).toBe(0); + expect(r.unchanged).toBe(n); + // The suppression list is the whole reason this seeder reports names, and + // an unchanged row is still a materialized one (#4967 Part 1). + expect(r.materializedNames).toHaveLength(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 = capabilityQl(declaredCaps(12)); + await bootstrapDeclaredCapabilities(ql, undefined); + ql.reset(); + await bootstrapDeclaredCapabilities(ql, undefined); + expect(ql.calls.find).toBe(1); + expect(ql.wheres[0]).toEqual({ name: { $in: declaredCaps(12).map((c) => c.name) } }); + }); + + it('first boot costs one batched read plus one INSERT per genuinely new capability', async () => { + const ql = capabilityQl(declaredCaps(10)); + const r = await bootstrapDeclaredCapabilities(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); + }); +}); + +/** + * ⚠️ LOAD-BEARING. Without these, an implementation that skipped every write + * would pass every capability count above while reconciling nothing at all — + * the exact failure shape a round-trip suite alone cannot see. + */ +describe('#11096 — drift STILL reconciles (declared capabilities)', () => { + it('a capability row whose stored label/description differ still gets its UPDATE', async () => { + const ql = capabilityQl(declaredCaps(20)); + await bootstrapDeclaredCapabilities(ql, undefined); + + // The package ships new copy for exactly ONE of the 20. + const upgraded = declaredCaps(20); + upgraded[7] = { ...upgraded[7], label: 'Renamed', description: 'new text' }; + (ql as any).registry = { listItems: (t: string) => (t === 'capability' ? upgraded : []) }; + + ql.reset(); + const r = await bootstrapDeclaredCapabilities(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 === 'crm.cap.7'); + expect(row.label).toBe('Renamed'); + expect(row.description).toBe('new text'); + }); + + it('a capability whose declared SCOPE changed still gets its UPDATE', async () => { + const ql = capabilityQl(declaredCaps(3)); + await bootstrapDeclaredCapabilities(ql, undefined); + + const upgraded = declaredCaps(3); + upgraded[1] = { ...upgraded[1], scope: 'org' as const }; + (ql as any).registry = { listItems: (t: string) => (t === 'capability' ? upgraded : []) }; + + ql.reset(); + const r = await bootstrapDeclaredCapabilities(ql, undefined); + expect(r.updated).toBe(1); + expect(ql.rows.find((x) => x.name === 'crm.cap.1').scope).toBe('org'); + }); + + it('a capability row a hand-edit drifted is healed back to the declaration', async () => { + const ql = capabilityQl(declaredCaps(3)); + await bootstrapDeclaredCapabilities(ql, undefined); + // Someone wrote straight at the row. + ql.rows[1].description = 'hand-edited'; + + ql.reset(); + const r = await bootstrapDeclaredCapabilities(ql, undefined); + expect(r.updated).toBe(1); + expect(r.unchanged).toBe(2); + expect(ql.rows[1].description).toBe('desc 1'); + }); + + it('a re-seed still never touches the provenance columns', async () => { + const ql = capabilityQl([{ + name: 'crm.cap.0', label: 'Capability v2', description: 'new', scope: 'platform', + _packageId: 'com.example.crm', + }]); + ql.rows.push({ + id: 'cap_1', name: 'crm.cap.0', label: 'Capability', description: 'old', scope: 'platform', + active: false, managed_by: 'package', package_id: 'com.example.crm', + }); + await bootstrapDeclaredCapabilities(ql, undefined); + const row = ql.rows[0]; + expect(row.label).toBe('Capability v2'); + expect(row.active).toBe(false); + expect(row.managed_by).toBe('package'); + expect(row.package_id).toBe('com.example.crm'); + }); +}); + +describe('#11096 — a genuinely NEW declaration is still created', () => { + it('the batched read does not turn "absent" into "present" (capabilities)', async () => { + const ql = capabilityQl(declaredCaps(5)); + await bootstrapDeclaredCapabilities(ql, undefined); + + const grown = [...declaredCaps(5), { + name: 'crm.cap.new', label: 'New', description: 'brand new', scope: 'platform' as const, + _packageId: 'com.example.crm', + }]; + (ql as any).registry = { listItems: (t: string) => (t === 'capability' ? grown : []) }; + + ql.reset(); + const r = await bootstrapDeclaredCapabilities(ql, undefined); + expect(r.seeded).toBe(1); + expect(r.unchanged).toBe(5); + expect(ql.rows.map((x) => x.name)).toContain('crm.cap.new'); + // one batched read + one insert — the other five cost nothing at all + expect(ql.roundTrips()).toBe(2); + }); +}); + +/** + * ⛔ The provenance half of triage's clause ②: the refusal diagnostics at the + * top of `upsertPackageCapability` state a DIFFERENT consequence depending on + * whether a row was found, so the batched read has to preserve the found / + * not-found distinction, not merely return rows. + */ +describe('#11096 — the batched read preserves the provenance branches', () => { + it('still refuses a capability owned by ANOTHER package, loudly', async () => { + const ql = capabilityQl([{ + name: 'crm.cap.0', label: 'Mine', description: 'd', scope: 'platform', + _packageId: 'com.example.b', + }]); + ql.rows.push({ + id: 'cap_1', name: 'crm.cap.0', label: 'Theirs', description: 'd', scope: 'platform', + managed_by: 'package', package_id: 'com.example.a', active: true, + }); + const warns: string[] = []; + const r = await bootstrapDeclaredCapabilities(ql, undefined, { + logger: { info: () => {}, warn: (m) => warns.push(m) }, + }); + expect(r.skippedForeign).toBe(1); + expect(r.materializedNames).toEqual(['crm.cap.0']); + expect(ql.rows[0].package_id).toBe('com.example.a'); // untouched + expect(warns.some((w) => w.includes('owned by another package'))).toBe(true); + }); + + it('still CLAIMS a derived platform placeholder for an explicit declaration', async () => { + const ql = capabilityQl(declaredCaps(1)); + ql.rows.push({ + id: 'cap_1', name: 'crm.cap.0', label: 'Crm Cap 0', description: 'Capability crm.cap.0.', + scope: 'platform', managed_by: 'platform', active: true, + }); + const r = await bootstrapDeclaredCapabilities(ql, undefined); + expect(r.claimed).toBe(1); + expect(ql.rows[0].managed_by).toBe('package'); + expect(ql.rows[0].package_id).toBe('com.example.crm'); + }); + + it('never clobbers an admin-authored row', async () => { + const ql = capabilityQl(declaredCaps(1)); + ql.rows.push({ + id: 'cap_1', name: 'crm.cap.0', label: 'Admin Copy', description: 'admin wrote this', + scope: 'platform', managed_by: 'admin', active: true, + }); + const r = await bootstrapDeclaredCapabilities(ql, undefined); + expect(r.skippedAdmin).toBe(1); + expect(r.materializedNames).toEqual(['crm.cap.0']); + expect(ql.rows[0].label).toBe('Admin Copy'); + expect(ql.calls.update).toBe(0); + }); + + it('an UNOWNED declaration still reports the consequence for a name WITH a row', async () => { + const ql = capabilityQl([{ name: 'crm.cap.0', label: 'L', description: 'd', scope: 'platform' }]); + ql.rows.push({ + id: 'cap_1', name: 'crm.cap.0', label: 'L', description: 'd', scope: 'platform', + managed_by: 'platform', active: true, + }); + const warns: string[] = []; + const r = await bootstrapDeclaredCapabilities(ql, undefined, { + logger: { info: () => {}, warn: (m) => warns.push(m) }, + }); + expect(r.skippedUnowned).toBe(1); + // A row resolves it — so the derivation must be suppressed, and the + // diagnostic must say so rather than promising a placeholder. + expect(r.materializedNames).toEqual(['crm.cap.0']); + expect(warns.some((w) => w.includes('already resolves it and is left as-is'))).toBe(true); + }); + + it('an UNOWNED declaration with NO row reports the OTHER consequence, and stays unsuppressed', async () => { + const ql = capabilityQl([{ name: 'crm.cap.0', label: 'L', description: 'd', scope: 'platform' }]); + const warns: string[] = []; + const r = await bootstrapDeclaredCapabilities(ql, undefined, { + logger: { info: () => {}, warn: (m) => warns.push(m) }, + permissionSets: [{ name: 'crm_rep', systemPermissions: ['crm.cap.0'] }], + }); + expect(r.skippedUnowned).toBe(1); + expect(r.materializedNames).toEqual([]); // ⛔ #4967: no row ⇒ no suppression + expect(warns.some((w) => w.includes('falls back to the back-compat derived placeholder'))).toBe(true); + expect(warns.some((w) => w.includes('crm_rep'))).toBe(true); + }); + + it('a name declared twice in one batch keeps its loud refusal', async () => { + const ql = capabilityQl([ + { name: 'crm.shared', label: 'A', description: 'a', scope: 'platform', _packageId: 'com.example.a' }, + { name: 'crm.shared', label: 'B', description: 'b', scope: 'platform', _packageId: 'com.example.b' }, + ]); + const warns: string[] = []; + const r = await bootstrapDeclaredCapabilities(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); + }); +}); + +/** + * ⛔ #3807's conflation class on the capability axis. Note the second half: a + * name whose row could not be read must ALSO stay out of `materializedNames`, + * because suppressing the back-compat derivation for a name that may have no + * row is precisely the #4967 hole. + */ +describe('#11096 — a read that CANNOT ANSWER is not the answer "none exist"', () => { + it('a throwing read does NOT re-create capabilities that are already seeded', async () => { + const ql = capabilityQl(declaredCaps(4)); + await bootstrapDeclaredCapabilities(ql, undefined); + expect(ql.rows).toHaveLength(4); + + const broken = capabilityQl(declaredCaps(4), { findThrows: true }); + broken.rows.push(...ql.rows.map((r) => ({ ...r }))); + const warns: string[] = []; + const r = await bootstrapDeclaredCapabilities(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(r.materializedNames).toEqual([]); // ⛔ unknown ⇒ never suppress + 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 = capabilityQl(declaredCaps(4), { findReturnsNonArray: true }); + ql.rows.push(...declaredCaps(4).map((c, i) => ({ + id: `cap_${i}`, name: c.name, label: c.label, description: c.description, + scope: 'platform', managed_by: 'package', package_id: 'com.example.crm', active: true, + }))); + const r = await bootstrapDeclaredCapabilities(ql, undefined); + expect(r.seeded).toBe(0); + expect(r.unreadable).toBe(4); + expect(ql.calls.insert).toBe(0); + expect(ql.rows).toHaveLength(4); + }); + + it('an EMPTY result set is still trusted as "none exist" — the first boot depends on it', async () => { + const ql = capabilityQl(declaredCaps(3)); + const r = await bootstrapDeclaredCapabilities(ql, undefined); + expect(r.seeded).toBe(3); + }); +}); diff --git a/packages/plugins/plugin-security/src/permission-set-projection.test.ts b/packages/plugins/plugin-security/src/permission-set-projection.test.ts index ed2caa05b5..00bd08ba8e 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.test.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.test.ts @@ -36,7 +36,20 @@ import { type ProjectionLogger, } from './permission-set-projection.js'; -/** In-memory ql over sys_permission_set + sys_metadata. */ +/** + * In-memory ql over sys_permission_set + sys_metadata. + * + * [#11097] Supports the `$in` membership operator, because the real engine does + * — `bootstrapDeclaredPermissions` has read `sys_permission_set` with + * `{ name: { $in: names } }` since #10946, on this very table. A double that + * refused it (or, worse, quietly matched nothing and returned `[]`) would pin + * the double's limits rather than the projector's behaviour: `[]` is the answer + * "none of these names exist", so a non-matching double would make the + * reconciler re-create every overlay record on every boot while this suite + * stayed green. + * + * `calls` counts every round trip — the defect #11096/#11097 fix is a COUNT. + */ function makeQl() { const permRows: any[] = []; const metaRows: any[] = []; @@ -45,13 +58,22 @@ function makeQl() { const matches = (r: any, where: any) => 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 v === null ? (r[k] ?? null) === null : r[k] === v; }); return { permRows, metaRows, + /** Round trips by table, since issue order matters to the count tests. */ + calls: { find: 0, insert: 0, update: 0 }, + resetCalls() { (this as any).calls = { find: 0, insert: 0, update: 0 }; }, async find(object: string, q: any) { const rows = tableFor(object); + if (object === 'sys_permission_set') (this as any).calls.find += 1; return rows ? rows.filter((r) => matches(r, q?.where)) : []; }, async findOne(object: string, q: any) { @@ -61,11 +83,13 @@ function makeQl() { async insert(object: string, data: any) { const rows = tableFor(object); if (!rows) return null; + if (object === 'sys_permission_set') (this as any).calls.insert += 1; rows.push({ ...data }); return { id: data.id }; }, async update(object: string, data: any) { const rows = tableFor(object); + if (object === 'sys_permission_set') (this as any).calls.update += 1; const r = rows?.find((x) => x.id === data.id); if (r) Object.assign(r, data); }, @@ -1205,3 +1229,323 @@ describe('mergeRowPatchIntoBody', () => { expect('_provenance' in merged).toBe(false); }); }); + +// ── #11097 — boot reconciliation is O(1) round trips, and still reconciles ── + +/** + * [#11097] The env door's boot reconciler projected every env-scope `permission` + * overlay in a per-name loop, and each iteration issued its OWN existence + * `SELECT` inside `upsertEnvPermissionSet` plus an `UPDATE` that fired whether + * or not the record already matched. Invisible on a local file database; one + * sequential HTTP request per leg on the remote libsql/Turso database every + * hosted environment runs. + * + * ## What is measured here, and what is NOT + * + * A COUNT — every `find`/`insert`/`update` the reconciler issues against + * `sys_permission_set` is one round trip, counted by `makeQl().calls`. ⚠️ The + * slope of THIS axis has never been measured: the hosted `bootstrap-curve.mjs` + * rig lives in `objectstack-ai/cloud`, and its axes are permission sets / + * positions / objects, not env overlays. What is established is that the code + * shape is the one #10946 measured at 4.0000 round trips per item on the two + * sibling loops. Nothing here measures wall time. + * + * ## Why every counting test below is paired with a reconciliation test + * + * ⚠️ LOAD-BEARING. A reconciler that simply stopped writing would produce a + * perfect count while silently reconciling nothing — the loop keeping its shape + * and losing its purpose. So each count is paired with a drift fixture over the + * same data, and `customized` gets its own pair because it is written by this + * projector and deliberately NOT compared by `recordDiffersFromBody`. + */ +describe('#11097 — env overlay reconciliation: round trips', () => { + const overlay = (n: number) => `env_set_${n}`; + + /** Seed `n` env-authored overlays and settle them into records. */ + const seedOverlays = async (n: number) => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + for (let i = 0; i < n; i += 1) { + ql.metaRows.push({ + id: `m${i}`, type: 'permission', name: overlay(i), state: 'active', + organization_id: null, metadata: JSON.stringify(envBody({ name: overlay(i) })), + }); + } + await reconcilePermissionSetProjection(protocol, { ql }); // first boot: creates + return { ql, protocol }; + }; + + it('does not grow the steady-state round-trip count with the number of overlays', async () => { + const measure = async (n: number) => { + const { ql, protocol } = await seedOverlays(n); + ql.resetCalls(); + const out = await reconcilePermissionSetProjection(protocol, { ql }); // REBUILD + // Nothing changed, so nothing is written — and that is the whole point. + expect(out.projectedFromMetadata).toBe(0); + expect(ql.calls.update).toBe(0); + expect(ql.calls.insert).toBe(0); + return ql.calls.find; + }; + + const [n1, n5, n20, n40] = [await measure(1), await measure(5), await measure(20), await measure(40)]; + // ONE batched `$in` existence read + the pre-existing full-record page read. + // The COUNT is asserted, never the wall time. + expect([n1, n5, n20, n40]).toEqual([2, 2, 2, 2]); + }); + + it('issues ONE batched `$in` existence read for the whole overlay set', async () => { + const { ql, protocol } = await seedOverlays(12); + const seen: any[] = []; + const origFind = ql.find.bind(ql); + (ql as any).find = async (object: string, q: any) => { + if (object === 'sys_permission_set') seen.push(q?.where); + return origFind(object, q); + }; + ql.resetCalls(); + await reconcilePermissionSetProjection(protocol, { ql }); + expect(seen).toHaveLength(2); + expect(seen[0]).toEqual({ name: { $in: Array.from({ length: 12 }, (_, i) => overlay(i)) } }); + }); + + it('first boot costs one batched read plus one INSERT per genuinely new overlay', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + for (let i = 0; i < 10; i += 1) { + ql.metaRows.push({ + id: `m${i}`, type: 'permission', name: overlay(i), state: 'active', + organization_id: null, metadata: JSON.stringify(envBody({ name: overlay(i) })), + }); + } + const out = await reconcilePermissionSetProjection(protocol, { ql }); + expect(out.projectedFromMetadata).toBe(10); + expect(ql.calls.insert).toBe(10); + expect(ql.calls.update).toBe(0); + expect(ql.permRows).toHaveLength(10); + }); +}); + +/** + * ⚠️ LOAD-BEARING. Without these, an implementation that skipped every write + * would pass every count above while reconciling nothing at all. + */ +describe('#11097 — drift STILL reconciles', () => { + it('an overlay whose stored facets differ still gets its UPDATE', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + ql.metaRows.push({ + id: 'm1', type: 'permission', name: 'organization_admin', state: 'active', + organization_id: null, metadata: JSON.stringify(envBody()), + }); + await reconcilePermissionSetProjection(protocol, { ql }); + expect(ql.permRows).toHaveLength(1); + + // Someone wrote straight at the record. + ql.permRows[0].object_permissions = JSON.stringify({ crm_lead: { allowDelete: true } }); + + ql.resetCalls(); + const out = await reconcilePermissionSetProjection(protocol, { ql }); + expect(out.projectedFromMetadata).toBe(1); + expect(ql.calls.update).toBe(1); + expect(JSON.parse(ql.permRows[0].object_permissions)).toEqual({ crm_lead: { allowRead: true, allowEdit: true } }); + }); + + it('an overlay whose label drifted still gets its UPDATE', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + ql.metaRows.push({ + id: 'm1', type: 'permission', name: 'organization_admin', state: 'active', + organization_id: null, metadata: JSON.stringify(envBody()), + }); + await reconcilePermissionSetProjection(protocol, { ql }); + ql.permRows[0].label = 'Stale Label'; + + ql.resetCalls(); + await reconcilePermissionSetProjection(protocol, { ql }); + expect(ql.calls.update).toBe(1); + expect(ql.permRows[0].label).toBe('Organization Administrator'); + }); + + it('only the DRIFTED overlay is written — the other 19 cost nothing', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + for (let i = 0; i < 20; i += 1) { + ql.metaRows.push({ + id: `m${i}`, type: 'permission', name: `env_set_${i}`, state: 'active', + organization_id: null, metadata: JSON.stringify(envBody({ name: `env_set_${i}` })), + }); + } + await reconcilePermissionSetProjection(protocol, { ql }); + const target = ql.permRows.find((r: any) => r.name === 'env_set_7'); + target.label = 'drifted'; + + ql.resetCalls(); + const out = await reconcilePermissionSetProjection(protocol, { ql }); + expect(out.projectedFromMetadata).toBe(1); + expect(ql.calls.update).toBe(1); + expect(target.label).toBe('Organization Administrator'); + }); +}); + +/** + * ⛔ [#11097] The `customized` stamp is written by this projector and is + * deliberately NOT part of `recordDiffersFromBody` — it is provenance, not + * definition, so no metadata body can declare it. That makes it the one column + * a naive "skip when the body matches" would stop maintaining, while the Setup + * list badges on it and the reset action reads it. These pin the flag's own + * comparison term. + */ +describe('#11097 — the `customized` stamp is still maintained', () => { + const pkgRow = (over: Record = {}) => ({ + id: 'ps_pkg', name: 'organization_admin', managed_by: 'package', + package_id: 'com.example.crm', active: true, + ...permissionSetRowFields(envBody()), + ...over, + }); + + it('an overlay appearing over a package row still STAMPS `customized` on a matching row', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + // The record's facets already equal the overlay body — only the flag differs. + ql.permRows.push(pkgRow({ customized: false })); + ql.metaRows.push({ + id: 'm1', type: 'permission', name: 'organization_admin', state: 'active', + organization_id: null, metadata: JSON.stringify(envBody()), + }); + + ql.resetCalls(); + await reconcilePermissionSetProjection(protocol, { ql }); + expect(ql.permRows[0].customized).toBe(true); + expect(ql.calls.update).toBe(1); + }); + + it('a row that predates the flag (NULL) is still stamped, not read as already-customized', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + ql.permRows.push(pkgRow({ customized: null })); + ql.metaRows.push({ + id: 'm1', type: 'permission', name: 'organization_admin', state: 'active', + organization_id: null, metadata: JSON.stringify(envBody()), + }); + + await reconcilePermissionSetProjection(protocol, { ql }); + expect(ql.permRows[0].customized).toBe(true); + }); + + it('a package row whose flag is ALREADY true costs no write', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + ql.permRows.push(pkgRow({ customized: true })); + ql.metaRows.push({ + id: 'm1', type: 'permission', name: 'organization_admin', state: 'active', + organization_id: null, metadata: JSON.stringify(envBody()), + }); + + ql.resetCalls(); + await reconcilePermissionSetProjection(protocol, { ql }); + expect(ql.permRows[0].customized).toBe(true); + expect(ql.calls.update).toBe(0); + }); + + it('an env-authored row carrying a stale flag is still CLEARED', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + // `managed_by:'admin'` — the flag does not apply; a stale `true` must go. + ql.permRows.push({ + id: 'ps_env', name: 'organization_admin', managed_by: 'admin', active: true, + customized: true, ...permissionSetRowFields(envBody()), + }); + ql.metaRows.push({ + id: 'm1', type: 'permission', name: 'organization_admin', state: 'active', + organization_id: null, metadata: JSON.stringify(envBody()), + }); + + ql.resetCalls(); + await reconcilePermissionSetProjection(protocol, { ql }); + expect(ql.permRows[0].customized).toBe(false); + expect(ql.calls.update).toBe(1); + }); +}); + +/** + * ⛔ [#11097] The in-memory evaluator registry sync is NOT a database round trip, + * and gating it on "a write happened" is what compare-before-write would + * otherwise have done to it. The evaluator resolves permission sets + * registry-first, so a steady-state boot that skipped the sync would leave it + * enforcing the stale DECLARED body while the record and Setup showed the + * overlay. + */ +describe('#11097 — the evaluator registry is synced even when nothing was written', () => { + it('an overlay whose record already matches STILL syncs the evaluator registry', async () => { + const ql = makeQl(); + const declared = { organization_admin: envBody({ label: 'Shipped Baseline' }) }; + const protocol = makeProtocol(ql, declared); + const registered: any[] = []; + const metadata = { + registerInMemory: (type: string, name: string, body: any) => { registered.push({ type, name, body }); }, + }; + ql.metaRows.push({ + id: 'm1', type: 'permission', name: 'organization_admin', state: 'active', + organization_id: null, metadata: JSON.stringify(envBody({ label: 'Overlay Wins' })), + }); + + // First boot creates the record AND syncs. + await reconcilePermissionSetProjection(protocol, { ql, metadata }); + expect(registered).toHaveLength(1); + + // Steady state: the record already matches, so NO update is issued — the + // sync must happen anyway. + registered.length = 0; + ql.resetCalls(); + await reconcilePermissionSetProjection(protocol, { ql, metadata }); + expect(ql.calls.update).toBe(0); + expect(registered).toHaveLength(1); + expect(registered[0].body.label).toBe('Overlay Wins'); + }); +}); + +/** + * ⛔ #3807's conflation class at the seam the batched read newly exposes: a read + * that CANNOT ANSWER is not the answer "no such record". The per-item shape was + * accidentally immune (a failed read fell through to an insert that failed too, + * for that ONE name); a batched read speaks for the whole set at once. + */ +describe('#11097 — a read that CANNOT ANSWER is not the answer "none exist"', () => { + it('a throwing existence read does NOT re-create records that are already projected', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + for (let i = 0; i < 4; i += 1) { + ql.metaRows.push({ + id: `m${i}`, type: 'permission', name: `env_set_${i}`, state: 'active', + organization_id: null, metadata: JSON.stringify(envBody({ name: `env_set_${i}` })), + }); + } + await reconcilePermissionSetProjection(protocol, { ql }); + expect(ql.permRows).toHaveLength(4); + + // Every sys_permission_set read now fails — batched and per-item alike. + const origFind = ql.find.bind(ql); + (ql as any).find = async (object: string, q: any) => { + if (object === 'sys_permission_set') throw new Error('fake driver: read unavailable'); + return origFind(object, q); + }; + const warns: string[] = []; + ql.resetCalls(); + await reconcilePermissionSetProjection(protocol, { ql, logger: { warn: (m: string) => warns.push(m) } }); + + expect(ql.calls.insert).toBe(0); // ⛔ no blind insert + expect(ql.permRows).toHaveLength(4); // ⛔ nothing re-created + expect(warns.some((w) => w.includes('batched seed existence read failed'))).toBe(true); + }); + + it('an EMPTY result set is still trusted as "none exist" — the first boot depends on it', async () => { + const ql = makeQl(); + const protocol = makeProtocol(ql); + ql.metaRows.push({ + id: 'm1', type: 'permission', name: 'organization_admin', state: 'active', + organization_id: null, metadata: JSON.stringify(envBody()), + }); + const out = await reconcilePermissionSetProjection(protocol, { ql }); + expect(out.projectedFromMetadata).toBe(1); + expect(ql.permRows).toHaveLength(1); + }); +}); diff --git a/packages/plugins/plugin-security/src/permission-set-projection.ts b/packages/plugins/plugin-security/src/permission-set-projection.ts index 260c6c7b85..56d0d76852 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.ts @@ -69,6 +69,7 @@ import { PermissionSetSchema } from '@objectstack/spec/security'; import { seedCtx } from './per-organization-catalog.js'; +import { buildExistingByName, type ExistingByNameIndex } from './seed-name-lookup.js'; export const SYSTEM_CTX = { isSystem: true }; @@ -201,6 +202,23 @@ const parseMaybeJson = (v: any, fallback: any): any => { const asBool = (v: any): boolean => !(v === false || v === 0 || v === '0' || v === 'false'); +/** + * [#11097] Read the `customized` COLUMN as a definite boolean. + * + * ⛔ Deliberately NOT {@link asBool}, which answers `true` for `undefined` and + * `null` because its subject (`active`) defaults to on. `customized` defaults + * to **off** (`Field.boolean({ defaultValue: false })` on + * `sys_permission_set`), and a row that predates the flag stores NULL — so + * `asBool` would read every such row as "already customized" and the + * compare-before-write below would then decline to stamp the flag on exactly + * the legacy rows that need it. + * + * Absent, null, 0, '0', false and 'false' are all "not customized"; everything + * else is customized. + */ +const customizedFlag = (v: any): boolean => + !(v === undefined || v === null || v === false || v === 0 || v === '0' || v === 'false' || v === ''); + /** * `sys_permission_set` columns that are ROW STATE, not part of the metadata * DEFINITION — the spec declares no such key, so they must never travel into a @@ -435,7 +453,22 @@ export async function upsertEnvPermissionSet( ql: any, ps: any, _logger?: ProjectionLogger, - opts?: { customized?: boolean }, + opts?: { + customized?: boolean; + /** + * [#11097] A batched existence oracle built by the CALLER over every name + * it is about to project, so a boot reconciliation pays one read for the + * whole overlay set instead of one per name. + * + * Omitted — the live single-mutation path, which has exactly one name and + * nothing to batch — keeps the per-item read verbatim, including its + * pre-existing treatment of a failed read (fall through to an insert the + * `name` unique index refuses). Tightening THAT path to the oracle's + * decline-on-unknown rule is a real behaviour change on a hot write seam + * and belongs to its own card, not to a round-trip fix. + */ + existing?: ExistingByNameIndex; + }, ): Promise { 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; @@ -446,9 +479,24 @@ export async function upsertEnvPermissionSet( // anything (it IS the definition), so the flag only rides on package rows. const customized = opts?.customized; - const existing = (await tryFind(ql, 'sys_permission_set', { name: ps.name }, 1))[0]; + let existing: any; + if (opts?.existing) { + // ⛔ THREE outcomes, not two (`seed-name-lookup.ts`): a read that could not + // ANSWER is not the answer "no such record". Collapsing them here would + // make a boot during a brief database outage conclude that every overlay + // needs creating — and a batched read fails for the WHOLE set at once, + // where the per-item read below could only ever mislead about one name. + const lookup = await opts.existing.get(String(ps.name)); + if (lookup.status === 'unknown') { + out.unreadable += 1; + return out; + } + existing = lookup.status === 'present' ? lookup.row : undefined; + } else { + existing = (await tryFind(ql, 'sys_permission_set', { name: ps.name }, 1))[0]; + } if (!existing?.id) { - const created = await tryInsert(ql, 'sys_permission_set', { + const row = { id: genId('ps'), name: ps.name, ...permissionSetRowFields(ps), @@ -463,9 +511,29 @@ export async function upsertEnvPermissionSet( // ADMIN-owned (formerly stamped 'user'). No runtime path branches on the // value except the 'package' guard, so this is a pure vocab rename. managed_by: 'admin', - ...(customized !== undefined ? { customized: !!customized } : {}), - }); - if (created) out.seeded += 1; + // [#11097] `false`, not `!!customized`. This row is being CREATED, so it + // is `managed_by:'admin'` one line up — an env-authored definition, not a + // customization of a packaged one — and the update branch below already + // encodes exactly that rule (`existing.managed_by === 'package' ? + // !!customized : false`). The insert used to stamp the caller's raw + // opinion instead, so a new overlay-backed record was born badged + // "customized" in the Setup list while owning no package to be a + // customization OF, and the very next boot's projection cleared it. That + // second write is invisible while every boot re-writes every record; it + // stops being invisible the moment writes are skipped when nothing + // differs, which is why it is repaired here rather than left to make the + // steady state take two boots to settle. + ...(customized !== undefined ? { customized: false } : {}), + }; + const created = await tryInsert(ql, 'sys_permission_set', row); + if (created) { + out.seeded += 1; + // [#11097] The oracle is a SNAPSHOT taken before the caller's loop, so it + // cannot see this insert. Two projections of the same name in one pass + // would otherwise each attempt an insert, and the second would be refused + // by the `name` unique index instead of taking the update branch below. + opts?.existing?.remember(String(ps.name), row); + } return out; } @@ -480,6 +548,34 @@ export async function upsertEnvPermissionSet( if (customized !== undefined) { patch.customized = existing.managed_by === 'package' ? !!customized : false; } + + // [#11097] Write only when the stored row actually differs from the patch. On + // every boot the reconciler re-projected each env overlay unconditionally, + // paying a remote `UPDATE` per overlay to store what was already there. + // + // ⛔ THE COMPARISON MUST COVER EVERY COLUMN THE PATCH WRITES, and + // `recordDiffersFromBody` alone does NOT: it compares the facet/display + // columns, and `customized` is deliberately not among them (it is provenance, + // not definition — no metadata body can declare it). Skipping on the facets + // alone would therefore stop maintaining a flag the Setup list badges on and + // the reset action reads — the record would keep saying "customized" after + // its overlay was lifted, or never say it after one appeared. So the flag + // gets its own term, evaluated against the SAME `existing.managed_by` + // condition the patch uses, and only against the patch's own value: when the + // caller passes no `customized` opinion the patch carries no such key and + // there is nothing to compare. + // + // ⚠️ EQUALITY decides, never presence. Replacing this with an unconditional + // `unchanged` would give the reconciler a perfect round-trip count while it + // silently stopped healing anything. + const facetsDiffer = recordDiffersFromBody(existing, ps); + const customizedDiffers = customized !== undefined + && customizedFlag(existing.customized) !== customizedFlag(patch.customized); + if (!facetsDiffer && !customizedDiffers) { + out.unchanged += 1; + return out; + } + if (await tryUpdate(ql, 'sys_permission_set', patch)) { out.updated += 1; } @@ -596,6 +692,12 @@ export async function projectPermissionMutation( protocol: any, deps: ProjectionDeps, evt: { type?: string; name?: string; state?: string; organizationId?: string | null } | null | undefined, + /** + * [#11097] Forwarded to {@link upsertEnvPermissionSet} so a boot pass over + * many overlay names pays ONE existence read for the whole set. The live + * mutation projector passes nothing and keeps its per-item read. + */ + opts?: { existing?: ExistingByNameIndex }, ): Promise { if (evt?.type !== 'permission' || evt.state === 'draft' || !evt.name) return null; const { ql, metadata, logger } = deps; @@ -642,8 +744,21 @@ export async function projectPermissionMutation( await syncEvaluatorRegistry(metadata, evt.name, null, false); return retirePermissionSetRecord(ql, metadata, evt.name, logger); } - const out = await upsertEnvPermissionSet(ql, body, logger, { customized: overlayBacked }); - if (out.seeded + out.updated > 0) { + const out = await upsertEnvPermissionSet(ql, body, logger, { + customized: overlayBacked, + ...(opts?.existing ? { existing: opts.existing } : {}), + }); + // [#11097] ⚠️ `unchanged` COUNTS HERE, and leaving it out was a real defect + // waiting on the write-skip above. This sync is an IN-MEMORY registry write, + // not a database round trip, and the evaluator resolves permission sets from + // that registry REGISTRY-FIRST — so on every steady-state boot (records + // already matching, hence no `UPDATE`) an overlay-backed name would have gone + // unsynced, and the evaluator would have kept enforcing the stale DECLARED + // body while the record and the Setup list showed the overlay. The gate is + // "the record is now in the desired state", which is `seeded + updated + + // unchanged` — never "a write happened". A FAILED write still leaves all + // three at zero and still skips the sync, exactly as before. + if (out.seeded + out.updated + out.unchanged > 0) { await syncEvaluatorRegistry(metadata, evt.name, body, overlayBacked); } return out; @@ -1039,10 +1154,32 @@ export async function reconcilePermissionSetProjection( overlayNames.add(String(r.name)); } } + // [#11097] ONE existence read for every overlay name, hoisted out of the loop + // below. Each iteration used to issue its own `SELECT … WHERE name = ? LIMIT + // 1` inside `upsertEnvPermissionSet` — one sequential HTTP request per overlay + // on the remote libsql/Turso database every hosted environment runs. + // + // ⚠️ Deliberately NOT reusing the `tryFind(ql, 'sys_permission_set', {}, 1000)` + // page read a few lines below, tempting though that is (it reads the same + // table and would cost nothing extra). That page is CAPPED at 1000 rows, and a + // truncated page reads as "absent" — which INSERTS. An environment past the + // cap would re-attempt an insert for every overlay beyond it on every boot. + // The chunked `$in` read has no such cap by construction. + const existingByName = await buildExistingByName( + ql, + 'sys_permission_set', + [...overlayNames], + logger, + ); for (const name of overlayNames) { const res = await projectPermissionMutation(protocol, deps, { type: 'permission', name, state: 'active', organizationId: null, - }); + }, { existing: existingByName }); + // ⚠️ `unchanged` is deliberately NOT summed in. This counter reports records + // this pass CREATED OR CHANGED, and now that an already-matching record is + // no longer re-written, a steady-state boot honestly reports 0 here where it + // used to report one per overlay. Reading the old number as "N overlays are + // healthy" was always reading a write count as a health count. out.projectedFromMetadata += (res?.seeded ?? 0) + (res?.updated ?? 0); } From dc302392bda7de0e4233632b182a04fcc59b4065 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 20:04:21 +0000 Subject: [PATCH 3/6] docs(changeset): batch capability + overlay boot seeds Refs #11096 #11097 --- ...batch-capability-and-overlay-boot-seeds.md | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .changeset/batch-capability-and-overlay-boot-seeds.md diff --git a/.changeset/batch-capability-and-overlay-boot-seeds.md b/.changeset/batch-capability-and-overlay-boot-seeds.md new file mode 100644 index 0000000000..a0fdbfc54c --- /dev/null +++ b/.changeset/batch-capability-and-overlay-boot-seeds.md @@ -0,0 +1,25 @@ +--- +"@objectstack/plugin-security": patch +--- + +**Perf:** the declared-capability boot seed and the environment permission-set overlay reconciler each pay ONE batched existence read instead of one per item, and stop re-writing rows that already match (#11096, #11097). + +Both were read-then-write reconcilers over a set known in full before their loop started, and both had the shape #10946 removed from the permission-set and position seeders next door: + +- `bootstrapDeclaredCapabilities` issued a `SELECT … WHERE name = ? LIMIT 1` per declared capability, then an `UPDATE` on its own row whether or not anything had changed; +- `reconcilePermissionSetProjection` projected every environment-scope `permission` overlay in a per-name loop, each iteration issuing its own existence `SELECT` inside `upsertEnvPermissionSet` plus an unconditional `UPDATE`. + +On a local file database these loops are invisible. On the remote libsql/Turso database every hosted environment runs, each leg is its own sequential HTTP request, and the capability set is typically the largest of the identity axes — it is the union of every capability every declared package contributes, not a count bounded by the number of permission sets. + +Both now hoist one chunked `{ name: { $in: [...] } }` read out of the loop through `buildExistingByName`, which keeps the tri-state judgement that makes hoisting safe: **a read that could not ANSWER is not the answer "none of them exist"**. A batched read fails for the whole set at once, so collapsing those two would make a boot during a brief outage try to re-create everything; the seeders now decline the names they could not read, and say so. + +**The write-skip is an equality test, and the reconciliation leg is pinned.** A row whose stored value genuinely differs still gets its `UPDATE` — a reconciler that skipped writes outright would show a perfect round-trip count while silently reconciling nothing, so every counting test added here is paired one-for-one with a drift test over the same fixture, and both pairs were ablated to confirm the drift half fails when the write is removed. + +Two behaviour repairs the write-skip forced into the open, both on the environment door: + +- **`customized` is now compared, not just written.** The flag is provenance rather than definition, so `recordDiffersFromBody` deliberately does not compare it; skipping on the facets alone would have stopped maintaining a flag the Setup list badges on and the reset action reads. It gets its own comparison term, against the same `managed_by:'package'` condition the write uses. +- **A newly created environment-authored record is no longer born badged "customized".** The insert stamped the caller's raw overlay opinion while the update path stamped `false` for a non-package row; the record was corrected on the next boot, which was invisible only because every boot re-wrote every record. + +`projectPermissionMutation` also syncs the in-memory evaluator registry on an unchanged record, not only on a write. That sync is not a database round trip, and the evaluator resolves permission sets registry-first — gating it on "a write happened" would have left a steady-state boot enforcing the stale declared body while the record and Setup showed the overlay. + +⚠️ **No curve number is claimed for either axis.** The hosted `bootstrap-curve.mjs` rig lives in `objectstack-ai/cloud` and neither of these axes has ever been measured on it. What is established is that the code shape is the one measured at slope 4.0000 / R² = 1.000000 on the two sibling loops in #10946, and that the round-trip COUNT is now flat in the number of declared items — which is what the new tests assert, in counts, never in wall time. From 4318373b426fc3128885bb137cb5dbf87f9754ce Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 20:34:37 +0000 Subject: [PATCH 4/6] fix(plugin-security): revert insert-path customized stamp to main's behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit upsertEnvPermissionSet's INSERT was changed to always stamp customized:false, replacing main's !!customized. That's a real, if narrow, provenance-flag correctness fix (a brand-new admin-owned row can otherwise be born badged "customized" for one boot cycle) — but it's a different defect class from #11096/#11097's round-trip and no-op-write scope, it changes the resulting projected state (not just the write count), and no test in this diff pins it. Revert to keep this PR a strictly behaviour-preserving perf fix, and update the changeset to match. The adjacent bug is being filed as a separate, unassigned issue. --- ...batch-capability-and-overlay-boot-seeds.md | 5 ++-- .../src/permission-set-projection.ts | 27 ++++++++++--------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/.changeset/batch-capability-and-overlay-boot-seeds.md b/.changeset/batch-capability-and-overlay-boot-seeds.md index a0fdbfc54c..1d8618793d 100644 --- a/.changeset/batch-capability-and-overlay-boot-seeds.md +++ b/.changeset/batch-capability-and-overlay-boot-seeds.md @@ -15,11 +15,12 @@ Both now hoist one chunked `{ name: { $in: [...] } }` read out of the loop throu **The write-skip is an equality test, and the reconciliation leg is pinned.** A row whose stored value genuinely differs still gets its `UPDATE` — a reconciler that skipped writes outright would show a perfect round-trip count while silently reconciling nothing, so every counting test added here is paired one-for-one with a drift test over the same fixture, and both pairs were ablated to confirm the drift half fails when the write is removed. -Two behaviour repairs the write-skip forced into the open, both on the environment door: +One behaviour repair the write-skip required, on the environment door: - **`customized` is now compared, not just written.** The flag is provenance rather than definition, so `recordDiffersFromBody` deliberately does not compare it; skipping on the facets alone would have stopped maintaining a flag the Setup list badges on and the reset action reads. It gets its own comparison term, against the same `managed_by:'package'` condition the write uses. -- **A newly created environment-authored record is no longer born badged "customized".** The insert stamped the caller's raw overlay opinion while the update path stamped `false` for a non-package row; the record was corrected on the next boot, which was invisible only because every boot re-wrote every record. `projectPermissionMutation` also syncs the in-memory evaluator registry on an unchanged record, not only on a write. That sync is not a database round trip, and the evaluator resolves permission sets registry-first — gating it on "a write happened" would have left a steady-state boot enforcing the stale declared body while the record and Setup showed the overlay. +⚠️ **Scope note.** An adjacent bug was found and left untouched on purpose: `upsertEnvPermissionSet`'s INSERT still stamps `customized` from the caller's raw opinion (`!!customized`), unchanged from `main`. A brand-new environment-authored record with no package baseline at all can therefore still be born badged "customized" for one boot cycle, self-correcting on the next reconciliation pass (the update branch's rule already forces the flag to `false` for a non-package row). That is a pre-existing provenance-flag defect — a different defect class from the round-trip/no-op-write fix this changeset describes, and one that would change the *resulting projected state* rather than the number of writes taken to reach it — so it is filed and reviewed on its own rather than riding this perf fix. + ⚠️ **No curve number is claimed for either axis.** The hosted `bootstrap-curve.mjs` rig lives in `objectstack-ai/cloud` and neither of these axes has ever been measured on it. What is established is that the code shape is the one measured at slope 4.0000 / R² = 1.000000 on the two sibling loops in #10946, and that the round-trip COUNT is now flat in the number of declared items — which is what the new tests assert, in counts, never in wall time. diff --git a/packages/plugins/plugin-security/src/permission-set-projection.ts b/packages/plugins/plugin-security/src/permission-set-projection.ts index 56d0d76852..92e972f396 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.ts @@ -511,19 +511,20 @@ export async function upsertEnvPermissionSet( // ADMIN-owned (formerly stamped 'user'). No runtime path branches on the // value except the 'package' guard, so this is a pure vocab rename. managed_by: 'admin', - // [#11097] `false`, not `!!customized`. This row is being CREATED, so it - // is `managed_by:'admin'` one line up — an env-authored definition, not a - // customization of a packaged one — and the update branch below already - // encodes exactly that rule (`existing.managed_by === 'package' ? - // !!customized : false`). The insert used to stamp the caller's raw - // opinion instead, so a new overlay-backed record was born badged - // "customized" in the Setup list while owning no package to be a - // customization OF, and the very next boot's projection cleared it. That - // second write is invisible while every boot re-writes every record; it - // stops being invisible the moment writes are skipped when nothing - // differs, which is why it is repaired here rather than left to make the - // steady state take two boots to settle. - ...(customized !== undefined ? { customized: false } : {}), + // [#11097 SCOPE] Deliberately UNCHANGED from pre-#11097 behaviour: + // `!!customized`, the caller's raw opinion, exactly as before this card. + // A fresh `managed_by:'admin'` row can be born `customized: true` when + // this insert runs with `customized: overlayBacked` from a name that has + // NO package baseline at all — that reads wrong against the update + // branch's rule two lines below (`managed_by === 'package' ? !!customized + // : false`), and self-heals on the very next `projectPermissionMutation` + // call because `customizedDiffers` (below) compares against that SAME + // rule and issues a corrective UPDATE. Tightening the insert to match is + // a real, if narrow, PROJECTED-STATE change — not a round-trip-count + // change — and belongs to its own card with its own review, not a rider + // on a perf fix (AGENTS.md Prime Directive #10 / Rule 3's same-defect- + // class test). Filed separately; left exactly as `main` has it. + ...(customized !== undefined ? { customized: !!customized } : {}), }; const created = await tryInsert(ql, 'sys_permission_set', row); if (created) { From 3cb2101a3f8b2a88816686b689b1f1e68bf4e386 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 21:01:49 +0000 Subject: [PATCH 5/6] =?UTF-8?q?fix(plugin-security):=20restore=20the=20cus?= =?UTF-8?q?tomized:false=20insert=20stamp=20=E2=80=94=20required,=20not=20?= =?UTF-8?q?a=20rider?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correction to the previous commit on this branch: reverting upsertEnvPermissionSet's INSERT-path customized fix back to main's !!customized turned out to be wrong, confirmed by an actual test run. The UPDATE branch's rule (`managed_by === 'package' ? !!customized : false`) already forces `customized: false` on every subsequent call for a non-package row. Stamping the caller's raw opinion on INSERT instead means a fresh managed_by:'admin' row disagrees with its own update rule the moment it's created. Before #11096/#11097 every boot rewrote every record unconditionally, so this was invisible. Once writes are equality-gated, that disagreement is measured as real drift: `does not grow the steady-state round-trip count` and `only the DRIFTED overlay is written` both failed with `!!customized` on insert — a real corrective UPDATE recurs one boot after every overlay-backed admin row is created, forever, not once historically. So this insert-side fix is required for the round-trip claim to hold on this path; it isn't separable from it. It IS a resulting-state change beyond a pure write-count reduction (a freshly created record is never transiently observed customized:true), called out explicitly in the changeset per that distinction. Verified: all 122 tests in bootstrap-declared-capabilities.test.ts, bootstrap-seed-round-trips. test.ts and permission-set-projection.test.ts pass with the fix restored; 3 fail without it. --- ...batch-capability-and-overlay-boot-seeds.md | 5 +-- .../src/permission-set-projection.ts | 35 +++++++++++-------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/.changeset/batch-capability-and-overlay-boot-seeds.md b/.changeset/batch-capability-and-overlay-boot-seeds.md index 1d8618793d..0d6e7cb5fd 100644 --- a/.changeset/batch-capability-and-overlay-boot-seeds.md +++ b/.changeset/batch-capability-and-overlay-boot-seeds.md @@ -15,12 +15,13 @@ Both now hoist one chunked `{ name: { $in: [...] } }` read out of the loop throu **The write-skip is an equality test, and the reconciliation leg is pinned.** A row whose stored value genuinely differs still gets its `UPDATE` — a reconciler that skipped writes outright would show a perfect round-trip count while silently reconciling nothing, so every counting test added here is paired one-for-one with a drift test over the same fixture, and both pairs were ablated to confirm the drift half fails when the write is removed. -One behaviour repair the write-skip required, on the environment door: +Two behaviour repairs the write-skip REQUIRED, both on the environment door — not optional polish, but corrections the equality test itself demands, verified by ablation (each one made a specific test fail when reverted): - **`customized` is now compared, not just written.** The flag is provenance rather than definition, so `recordDiffersFromBody` deliberately does not compare it; skipping on the facets alone would have stopped maintaining a flag the Setup list badges on and the reset action reads. It gets its own comparison term, against the same `managed_by:'package'` condition the write uses. +- **A newly created environment-authored record is no longer born badged "customized".** The INSERT used to stamp the caller's raw overlay opinion (`!!customized`) while the UPDATE branch's rule stamps `false` for a non-package row — those two disagree for any fresh `managed_by:'admin'` row created while its overlay is still active. Before this changeset, that disagreement was invisible: every boot re-wrote every record unconditionally, so the very next reconciliation pass silently overwrote the wrong value back to `false`. Once writes are equality-gated, that disagreement stops being invisible and becomes a REAL, PERMANENT one-boot-late corrective `UPDATE` after every such creation — the "steady state" round-trip count is not actually flat without this fix. Confirmed on this branch: reverting it to `!!customized` fails `#11097 — env overlay reconciliation: round trips > does not grow the steady-state round-trip count` and `#11097 — drift STILL reconciles > only the DRIFTED overlay is written` (both start seeing a real `UPDATE` on the boot immediately after any overlay-backed admin row is created). `projectPermissionMutation` also syncs the in-memory evaluator registry on an unchanged record, not only on a write. That sync is not a database round trip, and the evaluator resolves permission sets registry-first — gating it on "a write happened" would have left a steady-state boot enforcing the stale declared body while the record and Setup showed the overlay. -⚠️ **Scope note.** An adjacent bug was found and left untouched on purpose: `upsertEnvPermissionSet`'s INSERT still stamps `customized` from the caller's raw opinion (`!!customized`), unchanged from `main`. A brand-new environment-authored record with no package baseline at all can therefore still be born badged "customized" for one boot cycle, self-correcting on the next reconciliation pass (the update branch's rule already forces the flag to `false` for a non-package row). That is a pre-existing provenance-flag defect — a different defect class from the round-trip/no-op-write fix this changeset describes, and one that would change the *resulting projected state* rather than the number of writes taken to reach it — so it is filed and reviewed on its own rather than riding this perf fix. +⚠️ **This is a behaviour change beyond the write COUNT**, flagged explicitly: today, a brand-new environment-authored permission set with no package baseline can be observed `customized: true` for the one boot between its creation and the next reconciliation pass (or, on the live write-through door, self-heals within the same request). After this changeset it is never observed `true`. The change is required for the round-trip fix's own steady-state claim to hold on this path — the two are not separable — but it is a resulting-STATE change, not merely a write-count change, and is called out here for that reason. ⚠️ **No curve number is claimed for either axis.** The hosted `bootstrap-curve.mjs` rig lives in `objectstack-ai/cloud` and neither of these axes has ever been measured on it. What is established is that the code shape is the one measured at slope 4.0000 / R² = 1.000000 on the two sibling loops in #10946, and that the round-trip COUNT is now flat in the number of declared items — which is what the new tests assert, in counts, never in wall time. diff --git a/packages/plugins/plugin-security/src/permission-set-projection.ts b/packages/plugins/plugin-security/src/permission-set-projection.ts index 92e972f396..d4fd09dd0b 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.ts @@ -511,20 +511,27 @@ export async function upsertEnvPermissionSet( // ADMIN-owned (formerly stamped 'user'). No runtime path branches on the // value except the 'package' guard, so this is a pure vocab rename. managed_by: 'admin', - // [#11097 SCOPE] Deliberately UNCHANGED from pre-#11097 behaviour: - // `!!customized`, the caller's raw opinion, exactly as before this card. - // A fresh `managed_by:'admin'` row can be born `customized: true` when - // this insert runs with `customized: overlayBacked` from a name that has - // NO package baseline at all — that reads wrong against the update - // branch's rule two lines below (`managed_by === 'package' ? !!customized - // : false`), and self-heals on the very next `projectPermissionMutation` - // call because `customizedDiffers` (below) compares against that SAME - // rule and issues a corrective UPDATE. Tightening the insert to match is - // a real, if narrow, PROJECTED-STATE change — not a round-trip-count - // change — and belongs to its own card with its own review, not a rider - // on a perf fix (AGENTS.md Prime Directive #10 / Rule 3's same-defect- - // class test). Filed separately; left exactly as `main` has it. - ...(customized !== undefined ? { customized: !!customized } : {}), + // [#11097] `false`, not `!!customized`, and this is NOT optional once the + // write-skip below exists. This row is being CREATED, so it is + // `managed_by:'admin'` one line up — an env-authored definition, not a + // customization of a packaged one — and the update branch's rule a few + // lines down already encodes exactly that (`existing.managed_by === + // 'package' ? !!customized : false`). Stamping the caller's raw opinion + // here instead (main's behaviour) means a fresh overlay-backed admin row + // is born `customized: true`; the NEXT `projectPermissionMutation` call + // for the same name recomputes `patch.customized` via that SAME rule, + // gets `false`, and — now that writes are equality-gated — that mismatch + // is a real, measured drift, not a no-op. Verified by ablation: with + // `!!customized` here, `#11097 — env overlay reconciliation: round + // trips > does not grow the steady-state round-trip count` and + // `#11097 — drift STILL reconciles > only the DRIFTED overlay is + // written` both fail — the "steady state" boot immediately following a + // creation issues a real corrective UPDATE for every overlay-backed + // admin row, forever, not once historically. So this insert-time fix is + // required for the round-trip claim itself to hold, not an unrelated + // rider — the equality check needed on the write side implies the + // matching write on the create side. + ...(customized !== undefined ? { customized: false } : {}), }; const created = await tryInsert(ql, 'sys_permission_set', row); if (created) { From 8eef22337077cc1a5ace2b0e07b52600f8e5dc20 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 21:47:44 +0000 Subject: [PATCH 6/6] fix(plugin-security): type declaredCaps explicitly to fix TEST_DEBT +1 check:type-check-debt (the TEST_DEBT ratchet, tsc run over the package's test-inclusive program) reported plugin-security going 11 -> 12 (+1). Root cause: declaredCaps's return type was inferred from its body, narrowing scope to the literal 'platform' (from `scope: 'platform' as const`). The "declared SCOPE changed" drift fixture then does `upgraded[1] = { ...upgraded[1], scope: 'org' as const }`, assigning a 'org'-scoped object into an array slot typed to only ever hold 'platform' -- TS2322. Same shape DeclaredSet already solves one property over for permission sets (see its docstring): give the generator function an explicit return type wide enough for the upgrade fixtures to widen into, rather than letting scope infer down to a literal. Added `interface DeclaredCapability` with `scope: 'platform' | 'org'` and annotated declaredCaps's return type; dropped the now-redundant `as const` on the steady-state value. Reproduced with the exact generated project scripts/check-type-check-coverage.mjs's --re-measure builds for a TEST_DEBT package (extends the package's own tsconfig, drops the **/*.test.ts exclude, adds the same typeRoots) -- 12 errors before this commit, 11 after, matching the frozen TEST_DEBT entry exactly. `pnpm --filter @objectstack/plugin-security typecheck` cannot see this class of error at all: the package is in TEST_DEBT precisely because its own typecheck script excludes its tests from tsc. --- .../src/bootstrap-seed-round-trips.test.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 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 ca45480f1c..74930f2f71 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 @@ -429,7 +429,21 @@ describe('#10946 — a name declared twice in one batch keeps its loud refusal', // ── #11096 — declared capabilities ───────────────────────────────────────── -const declaredCaps = (n: number) => +/** + * `scope` is typed rather than inferred: the "declared SCOPE changed" drift + * fixture below widens `'platform'` to `'org'` to simulate a package version + * bump, which an inferred `'platform'` literal type rejects — the same reason + * `DeclaredSet` above types `objects` explicitly. + */ +interface DeclaredCapability { + name: string; + label: string; + description: string; + scope: 'platform' | 'org'; + _packageId: string; +} + +const declaredCaps = (n: number): DeclaredCapability[] => Array.from({ length: n }, (_, i) => ({ // ⚠️ Never a curated `PLATFORM_CAPABILITY_NAMES` entry: those are refused // before the existence read is even consulted, so a curated fixture would @@ -437,7 +451,7 @@ const declaredCaps = (n: number) => name: `crm.cap.${i}`, label: `Capability ${i}`, description: `desc ${i}`, - scope: 'platform' as const, + scope: 'platform', _packageId: 'com.example.crm', }));