Skip to content

Commit 4049cac

Browse files
fix(plugin-security): the boot heal converges on duplicated permission-set names, and a refused existence read never inserts (#22214)
Fixes #22169 Clause-②: no ## What was wrong (measured on `main` at `6ed0c0f3e5`) All three mechanisms the card names were confirmed on current `main` before any edit, with an in-repo reproduction: a real `ObjectQL` over a real `SqlDriver` (better-sqlite3, in memory), a real `ObjectStackProtocolImplementation`, and the per-organization unique index dropped after schema sync. Dropping it gives the shape of a remote table built before the index retrofit, the only shape that can hold duplicates. 1. **The heal did not converge.** `reconcilePermissionSetProjection` handed each drifted, overlay-less record to `upsertEnvPermissionSet`, which re-read the record **by name** (first row, lowest id). With 8 declared names × 3 rows, boot 1 wrote 8 rows, logged 24 drift warnings and left 16 rows drifted. Boot 2, boot 3 and every later boot logged 16 warnings and wrote nothing. 2. **A layered read was discarded on every row.** `getMetaItemLayered` was awaited for every candidate row before the trust rule decided whether it needed the answer. While a `SchemaRegistry` is readable, the rule never uses it. One layered read is **4 statements** on the real protocol, so the shape above paid 24 × 4 per boot. 3. **A refused existence read inserted a duplicate.** `bootstrapPlatformAdmin` reads each default set by name through a `tryFind` that answers `[]` for a refused read, and it inserted on both "absent" and "refused". The heal's by-name upsert had the same insert branch. When by-name reads were refused, the unpatched heal inserted 24 rows (24 → 48) and the bootstrap inserted 2 more. ## The change (`@objectstack/plugin-security` only: 2 source files) - `permission-set-projection.ts`, `reconcilePermissionSetProjection` heal branch: - **The heal writes the row it read, by its id, facets only.** It calls `tryUpdate` with `{ id: row.id, ...permissionSetRowFields(effective) }`, the same columns the upsert's update leg wrote. Identity, `active`, `customized` and provenance are not touched. The heal no longer asks an existence question, so it has no insert branch. - **The drift line is logged after the write.** It names the row's `id`, and a refused write is logged as refused instead of "re-projected". `driftHealed` counts landed writes, now one per drifted row. - **The layered read is lazy, once per name.** It is asked only when no `SchemaRegistry` answers, and the answer is cached per name for the pass. The cache entry is dropped after this pass backfills that name, because the backfill changes the answer. A later row of that name then sees the backfilled definition and heals to it, instead of backfilling a second time. - `bootstrap-platform-admin.ts`, the default-set seed loop: - **A refused existence read declines the insert.** `tryFind`'s existing `onRefusal` observer tells "refused" apart from "empty". - **The pass says so once.** It writes one `warn` line per pass, not one per set, naming the sets. The line goes through `reportThroughSink`, the derivation the sibling seeders' unreadable lines use. When `admin_full_access` is one of the declined sets, the promotion half returns `admin_permission_set_unreadable` instead of `admin_permission_set_missing`. Nothing in the repository branches on that string; the only reads are doc comments. - One `patch` changeset for `@objectstack/plugin-security`. **Not changed (card scope):** - No existing duplicate is deleted; that is an operator repair. - `upsertEnvPermissionSet`'s per-item read on the live single-mutation path is unchanged. A refused read there still inserts. - No change to `packages/spec`, no new gate. ## Measurement: statements per boot, in-repo reproduction This was measured with the harness above on the fix at `5ccce70966` (source unchanged since; later commits touch tests and the changeset only). Each count is every SQL statement knex emitted during `reconcilePermissionSetProjection`. The harness was not committed. | shape | boot | `main` `6ed0c0f3e5` | this PR | |---|---|---|---| | 8 names × 3 rows, all drifted | 1 | 148 statements (140 select / 8 update), 24 layered reads, 24 drift warns, 8 healed, **16 still drifted** | 76 statements (52 select / 24 update), 0 layered reads, 24 drift warns, 24 healed, 0 drifted | | | 2, 3 | 116 statements, 24 layered reads, **16 drift warns**, 16 still drifted | **4 statements**, 0 layered reads, 0 warns | | 60 names × 5 rows, all drifted | 1 | 1684 (1624 / 60), 300 layered reads, 300 warns, 60 healed, 240 still drifted | 904 (604 / 300), 0 layered reads, 300 warns, 300 healed, 0 drifted | | | 2, 3 | 1444 statements, 300 layered reads, 240 drift warns | **4 statements**, 0 layered reads, 0 warns | | by-name reads refused, 8 × 3 | heal + bootstrap | heal inserted 24, bootstrap inserted 2 (24 → 50 rows) | 0 inserts (24 → 24), 1 bootstrap line | On the healing boot, each by-id `UPDATE` costs the engine one extra `SELECT`. That is the 52 − 4 = 48 selects for 24 updates. ⛔ **The hosted timing is not measured here.** The card's cloud reproduction (cloud's `ArtifactKernelFactory` on the hosted Turso face, ~36 ms per statement, 99.85 s / 170.70 s) cannot be reached from this session, and no cloud time is claimed. The before/after boot times on that reproduction belong to the cloud seat. ## Pins (new) and ablations **Pins:** - `permission-set-projection-heal-by-id.test.ts` (real engine, spied and never replaced; index dropped). Five cases: - A duplicated set heals once, by id, and the next boot logs no drift warn. - Control: a single drifted row heals as before. - No layered read while a registry answers. - Without a registry, the layered read happens once per name. - A refused by-name read inserts nothing. - `bootstrap-platform-admin-refused-existence-read.test.ts`: - A refused read inserts nothing, logs one line and reports `admin_permission_set_unreadable`. - Two controls: an empty answer still seeds, and an existing row is left alone. - `permission-set-projection.test.ts` gains one case: a just-backfilled name is re-read and the second row heals to the backfilled body, with one `saveMetaItem`, not two. **Ablations.** Each leg ran on `e41760d89c` through `scripts/ablation-replace.mjs` in WRAP mode, with the anchor-hit count and blob change verified on disk. Each restore was proven with blob == HEAD and an empty `git diff HEAD`. The subjects are imported by relative path from `src/`, so no `dist/` leg applies. | leg | mutation | result | |---|---|---| | A1 | heal through the by-name upsert again | 4 red: duplicate heal wrote `['ps_a']` instead of `['ps_a','ps_b']`; refused read inserted 2; no-registry case `['ps_a']`; backfill case `driftHealed` 0. The control and the no-layered-read case stayed green. | | A2 | eager per-row layered read restored | 3 red: no-layered-read case saw 6 reads instead of `[]`; once-per-name and backfill read counts were also red | | A3 | per-name cache removed | 1 red: `['member_default','member_default']` instead of one read | | A4 | post-backfill cache drop removed | 1 red: 2 `saveMetaItem` calls instead of 1 | | A5 | bootstrap decline removed | 1 red: 3 inserts instead of `[]`; both controls stayed green | A first A2 attempt was a refused no-op: its replacement contained the anchor, so the anchor count stayed x1 → x1, nothing ran, and the restore was proven. It was re-run with a falling anchor. ## Verification (HEAD `e41760d89c`) - `pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2`: 176 files, 3695 passed, 45 skipped. - `pnpm --filter @objectstack/plugin-security typecheck` exits 0, including `check:test-typecheck` OK. `tsc -p tsconfig.test.json --listFiles` lists all 3 touched test files. - Gates: - `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 66 commands, plus `pnpm check:durability-log-level`; all exit 0. - `--ran`: 66 derived, 66 run, 0 NOT-MEASURED, 0 UNRUN. - The first run, at `af7b0a339f`, had `check:engine-double-contract` and `check:objectql-double-limit` red on this PR's new test doubles. Both were fixed in `e41760d89c`: the heal tests spy on the real engine instead of an object-literal double, and the bootstrap double applies `limit` by presence after the filter. - ESLint, narrowed and proven: 5 touched files, 0 errors, 0 warnings, run with `--no-inline-config --format json`. - All 5 files are in the population: `--print-config` resolves each, and no ignored-file warning was emitted. - The repository config never enables type-aware linting (`eslint.config.mjs`, no `parserOptions.project`), so this diff cannot move any untouched file's verdict. - No plugin `init()`/`start()` or declaration was edited, so `check:init-service-contract` and `check:startup-registry-verdict` were not owed. ## Acceptance notes - **Measured, not fixed (scope fence): the overlay pass leaves a duplicated overlay-backed name unconverged, silently.** - Probe: real engine, 2 drifted rows of `member_default` plus an active env `sys_metadata` overlay for it. - Boot 1 projected the overlay onto `ps_a` only, and `ps_b` kept its drifted facets on boots 1, 2 and 3 with 0 warnings. Step 1 resolves one row per name through `buildExistingByName`, and step 2 skips every overlay name. - It costs no boot time and logs nothing. It is display drift only, because enforcement reads the metadata definition. It is the same by-name family, outside this card's fence. - **Reserved by the card for its own card:** `upsertEnvPermissionSet`'s per-item read on the live single-mutation path still inserts on a refused read. - `retirePermissionSetRecord` has the same by-name `LIMIT 1` shape: on a metadata delete it removes only the first row of a duplicated name. Read, not exercised. - #15196's census marks this machinery for retirement in C3 (#15204, `pm:blocked`, not scheduled). C3 would delete these paths. That does not change this card. --- _Generated by [Claude Code](https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0e9371f commit 4049cac

6 files changed

Lines changed: 527 additions & 13 deletions
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
---
4+
5+
fix(plugin-security): the boot heal of drifted `sys_permission_set` rows converges on duplicated names, stops paying a discarded metadata read per row, and a refused existence read no longer inserts a duplicate (#22169)
6+
7+
Clause-②: no
8+
9+
**The heal writes the row it read, by its id.** `reconcilePermissionSetProjection` heals every overlay-less, env-authored record that drifted from its metadata definition. It used to hand each drifted record to a by-name upsert, which re-read the first record of that name (lowest id). On a table holding more than one record per name, only that one was ever written: every other record was logged "re-projected" and left drifted, on every boot. Now each drifted record is updated by its own id, facets only (label, description and the permission columns; identity, `active` and provenance are never touched). The drift line is logged after the write and names the record's id; a refused write is logged as refused instead of as re-projected. On a real engine with 8 names × 3 records, the boot after the healing one went from 116 statements and 16 drift warnings to 4 statements and none.
10+
11+
**The layered metadata read is lazy, once per name.** The heal awaited `getMetaItemLayered` (four statements on the protocol) for every candidate record before deciding whether it needed it, and it does not need it while a `SchemaRegistry` is readable. It is now read only when no registry answers, and once per name per pass.
12+
13+
**A refused existence read declines the insert.** `bootstrapPlatformAdmin` reads each default permission set by name and inserts when it finds none. A refused read answered as "none", so on a table without the per-organization unique index the boot inserted a second record of a set that already existed. A refused read now inserts nothing, and the pass says so once, naming the sets. When `admin_full_access` is one of them, the promotion half reports `admin_permission_set_unreadable` instead of `admin_permission_set_missing`.
14+
15+
**Unchanged.** Existing duplicate records are healed, never deleted: removing them is an operator repair. The live single-mutation projection keeps its per-item read.
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#22169] `bootstrapPlatformAdmin`'s existence read: a read that did not
5+
* answer is not an absent row.
6+
*
7+
* The seed loop reads each default permission set BY NAME and inserts when it
8+
* finds none. Its `tryFind` answers `[]` for a refused read as well as for an
9+
* empty table, and the loop inserted on both — so on a table without the
10+
* per-organization unique index a refused read minted a second row of a set
11+
* that already existed, and a table holding such duplicates can never build
12+
* that index again. Measured on a real engine with that index dropped: two
13+
* declared sets, two INSERTs, under a refused read.
14+
*
15+
* The double below has no `update` or `delete` member — the seed pass without
16+
* `resync` calls neither — so it is not an engine double those dispatch
17+
* contracts govern. Its `find` answers from its rows, bounded by the caller's
18+
* `limit`; the refused read is a spy laid over it, so the two inputs the pass
19+
* must tell apart — an answer, and a throw — come from one table.
20+
*/
21+
22+
import { describe, it, expect, afterEach, vi } from 'vitest';
23+
import { bootstrapPlatformAdmin } from './bootstrap-platform-admin.js';
24+
25+
afterEach(() => { vi.restoreAllMocks(); });
26+
27+
function makeQl(seedRows: any[] = []) {
28+
const rows: any[] = seedRows.map((r) => ({ ...r }));
29+
const inserted: any[] = [];
30+
return {
31+
rows,
32+
inserted,
33+
async find(object: string, q: any) {
34+
if (object !== 'sys_permission_set') return [];
35+
const where = q?.where ?? {};
36+
const matched = rows.filter((r) => Object.entries(where).every(([k, v]) => {
37+
if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`);
38+
return r[k] === v;
39+
}));
40+
return typeof q?.limit === 'number' ? matched.slice(0, q.limit) : matched;
41+
},
42+
async insert(object: string, data: any) {
43+
if (object !== 'sys_permission_set') return null;
44+
inserted.push({ ...data });
45+
rows.push({ ...data });
46+
return { id: data.id };
47+
},
48+
};
49+
}
50+
51+
/** Every `sys_permission_set` read now throws; every other read still answers. */
52+
function refuseSetReads(ql: ReturnType<typeof makeQl>): void {
53+
const answer = ql.find.bind(ql);
54+
vi.spyOn(ql, 'find').mockImplementation(async (object: string, q: any) => {
55+
if (object === 'sys_permission_set') throw new Error('fake outage: sys_permission_set read refused');
56+
return answer(object, q);
57+
});
58+
}
59+
60+
const set = (name: string) => ({ name, label: name, objects: {}, systemPermissions: [] }) as any;
61+
62+
function capture() {
63+
const warns: Array<{ msg: string; meta?: any }> = [];
64+
return { warns, logger: { info: () => {}, warn: (msg: string, meta?: any) => warns.push({ msg, meta }) } };
65+
}
66+
67+
describe('#22169 — bootstrapPlatformAdmin declines the insert when its existence read was refused', () => {
68+
it('a refused read inserts NOTHING, and says so once for the whole pass', async () => {
69+
const ql = makeQl([{ id: 'ps_existing', name: 'admin_full_access' }]);
70+
refuseSetReads(ql);
71+
const { warns, logger } = capture();
72+
73+
const r = await bootstrapPlatformAdmin(ql, [set('admin_full_access'), set('member_default'), set('viewer_readonly')], { logger });
74+
75+
expect(ql.inserted, '⛔ no row minted on a read that did not answer').toEqual([]);
76+
expect(r.seeded).toBe(0);
77+
const lines = warns.filter((w) => w.msg.includes('could not be read'));
78+
expect(lines, 'one line per pass, never one per set').toHaveLength(1);
79+
expect(lines[0]!.meta).toMatchObject({
80+
unreadable: 3, total: 3, names: ['admin_full_access', 'member_default', 'viewer_readonly'],
81+
});
82+
// The promotion half reports the read that failed, not a set that is "missing".
83+
expect(r.reason).toBe('admin_permission_set_unreadable');
84+
});
85+
86+
it('control: an EMPTY answer is still "absent" — a fresh install seeds every default set', async () => {
87+
const ql = makeQl();
88+
const { warns, logger } = capture();
89+
90+
const r = await bootstrapPlatformAdmin(ql, [set('admin_full_access'), set('member_default')], { logger });
91+
92+
expect(ql.inserted.map((row) => row.name)).toEqual(['admin_full_access', 'member_default']);
93+
expect(r.seeded).toBe(2);
94+
expect(warns.filter((w) => w.msg.includes('could not be read'))).toEqual([]);
95+
});
96+
97+
it('control: an existing row is found and left alone — no insert, no unreadable line', async () => {
98+
const ql = makeQl([{ id: 'ps_existing', name: 'member_default' }]);
99+
const { warns, logger } = capture();
100+
101+
await bootstrapPlatformAdmin(ql, [set('member_default')], { logger });
102+
103+
expect(ql.inserted).toEqual([]);
104+
expect(warns.filter((w) => w.msg.includes('could not be read'))).toEqual([]);
105+
});
106+
});

‎packages/plugins/plugin-security/src/bootstrap-platform-admin.ts‎

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ import {
9090
reportSeedWriteRefusals,
9191
type SeedWriteRefusals,
9292
} from './per-organization-catalog.js';
93+
import { reportThroughSink } from './seed-refusal-sink.js';
9394
import {
9495
resolvePlatformAdminStanding,
9596
type PlatformAdminStandingEntry,
@@ -778,9 +779,26 @@ export async function bootstrapPlatformAdmin(
778779
// One log per pass, not per refused row: a legacy platform-wide unique index
779780
// refuses EVERY default permission set, and a line each would bury the remedy.
780781
const refusals = createSeedWriteRefusals();
782+
// [#22169] Names whose existence read was REFUSED, so nothing was inserted
783+
// for them — reported once below, after the loop.
784+
const unreadable: string[] = [];
781785
for (const ps of bootstrapPermissionSets) {
782786
if (!ps.name) continue;
783-
const existing = await tryFind(ql, 'sys_permission_set', { name: ps.name }, 1);
787+
// ⛔ "Refused" is not "absent". `tryFind` answers `[]` for both, and this
788+
// loop used to INSERT on both: on a table without the per-organization
789+
// unique index (remote tables built before its retrofit) a refused read
790+
// minted a second row of a set that already existed, and a table holding
791+
// such duplicates can never build that index again. The observer tells
792+
// the two apart; a read that could not answer declines the insert.
793+
let readRefused = false;
794+
const existing = await tryFind(
795+
ql, 'sys_permission_set', { name: ps.name }, 1, undefined, undefined,
796+
() => { readRefused = true; },
797+
);
798+
if (readRefused) {
799+
unreadable.push(ps.name);
800+
continue;
801+
}
784802
if (existing.length > 0 && existing[0].id) {
785803
const row = existing[0];
786804
seeded[ps.name] = row.id;
@@ -835,6 +853,21 @@ export async function bootstrapPlatformAdmin(
835853
// finished either way. Placing it at the end would make the diagnosis
836854
// conditional on how promotion happened to resolve.
837855
reportSeedWriteRefusals(logger, refusals);
856+
if (unreadable.length > 0) {
857+
// [#22169] ONCE per pass with the names, never a line per set: a refused
858+
// read usually means the whole table is unreadable, and a per-name line
859+
// would bury its own remedy. `warn`, through the sink derivation the
860+
// sibling seeders' unreadable lines use, so it is heard with no logger
861+
// injected: nothing was written, so nothing claims to have landed.
862+
reportThroughSink(
863+
logger,
864+
'[security] platform default permission sets left untouched — their sys_permission_set rows could not ' +
865+
'be read, so none was inserted: a read that did not answer is not an absent row, and inserting on it ' +
866+
'duplicates a set that may already exist. A set that is genuinely absent is seeded by the next boot ' +
867+
'whose read answers.',
868+
{ unreadable: unreadable.length, total: bootstrapPermissionSets.length, names: unreadable },
869+
);
870+
}
838871

839872
const seededCount = Object.keys(seeded).length;
840873
// [#11532] Under a walled posture these rows are organization-less BY RULING
@@ -880,7 +913,12 @@ export async function bootstrapPlatformAdmin(
880913

881914
const adminPsId = seeded[PLATFORM_ADMIN_PERMISSION_SET_NAME];
882915
if (!adminPsId) {
883-
return { seeded: seededCount, adminPromoted: false, reason: 'admin_permission_set_missing', ...resyncCounts };
916+
// [#22169] `unreadable`, not `missing`, when the read was what failed: the
917+
// set may well exist, and the line above says so.
918+
const reason = unreadable.includes(PLATFORM_ADMIN_PERMISSION_SET_NAME)
919+
? 'admin_permission_set_unreadable'
920+
: 'admin_permission_set_missing';
921+
return { seeded: seededCount, adminPromoted: false, reason, ...resyncCounts };
884922
}
885923

886924
// ── Does this deployment ALREADY have a platform admin? (commit 1c83ca226) ─

0 commit comments

Comments
 (0)