fix(plugin-security): tell a read that did not answer from one that answered nothing, per site (#15840)

.changeset/plugin-security-read-fault-vs-empty.md

@@ -0,0 +1,13 @@
+---
+'@objectstack/plugin-security': patch
+---
+
+Tell a read that DID NOT ANSWER apart from a read that answered NOTHING at two boot-reconciler seams, so a transient storage fault can no longer withdraw a standing org-admin grant or report an unreadable catalog as an already-canonical one (#15840).
+
+`reconcileOrgAdminGrant`'s `sys_member` read swallowed a fault into `[]`, and `[]` is what that function reads as "this user is not an admin of this organization" — the input to a DELETE. One transient read fault therefore revoked a sitting admin's standing grant, and the store kept it withdrawn after the fault cleared; only a `debug` line separated that run from a healthy one. That read now reports at `error` and returns `{ action: 'skipped', reason: 'membership_unreadable' }`, performing no write at all for the pair: nothing is granted, so nothing widens, and nothing standing is destroyed. The next `sys_member` write and the `kernel:ready` backfill ask again.
+
+`normalizeManagedByVocab` swallowed a catalog read fault into `[]` too, so an unreadable catalog and an already-canonical one were byte-identical on both channels — the same `{ positions: 0, permissionSets: 0 }` and zero log lines at any level — while the row that needed healing stayed legacy. A read that does not answer now reports at `error` and refuses the pass instead of attesting counts it could not read. The refusal aborts at the first un-answered read, so it is one line per refused boot rather than the four the report-and-continue shape measured. Its only production consumer already declared the handling: the `kernel:ready` bootstrap catches it, reports it at `warn` as non-fatal, and boot proceeds.
+
+⭐ Per-site, not a sweep. A genuine EMPTY read keeps today's behaviour EXACTLY at both seams — a demotion with no membership row still revokes, a membership still grants, an already-canonical catalog still answers `{ positions: 0, permissionSets: 0 }` in silence. `claim-seed-ownership.ts` is untouched: its fault already propagates to a per-predicate handler that reports at `warn` and names the consequence, which is the right disposition already. The plugin's other reads keep their existing best-effort contract, where an unanswered read costs a grant that is not created rather than one that is destroyed.
+
+No exported symbol, published payload key or spec path changes: `action: 'skipped'` is already in the returned union, `reason` is already free text, and the two logger option types gain an optional `error` method a caller may omit. Healthy-path behaviour is byte-identical; only the fault path moves.

content/docs/permissions/system-context.mdx

@@ -350,7 +350,7 @@ still holds equal to the census on every pull request:
 | — in tests | 1013 | — |
 | — in non-test sources | 798 | — |
 | Appearances of the bare identifier `isSystem` in non-test sources | 813 | — |
-| — parsed as a declaration | 22 | ✅ |
+| — parsed as a declaration | 23 | ✅ |
 | — parsed as an object-literal / type key (producers and option objects) | 310 | — |
 | — parsed as a property **read** | 114 | ✅ |
 | — parsed in some other syntactic position (a local, a cast, a conditional) | 9 | ✅ |

packages/plugins/plugin-security/src/auto-org-admin-grant.ts

@@ -99,8 +99,32 @@ function supersededOrgAdminSetName(posture: TenancyPosture, suppressUnbounded =
 
 interface MaybeLogger {
   info?: (message: string, meta?: Record<string, any>) => void;
-  warn?: (message: string, meta?: Record<string, any>) => void;
+  /**
+   * [#9754] NON-optional, and it is the `error?` member below that makes it so.
+   * An optional `error` with no declared alternative is a contract that permits
+   * silence: a value of this type could carry no channel at all, and the
+   * durability report this module owes — a standing capability withdrawn, or a
+   * revoke that did not land — would have nowhere to go. `warn` is the level a
+   * durability degradation degrades TO and no further (AGENTS.md, "Degradation
+   * log levels"), so it is the one that must exist in every value of the type.
+   * ⛔ Not solved by requiring `error` instead (hosts legitimately inject
+   * reduced sinks), and ⛔ not by requiring `info`: a lost write reported at
+   * `info` reads as normal operation.
+   */
+  warn: (message: string, meta?: Record<string, any>) => void;
   debug?: (message: string, meta?: Record<string, any>) => void;
+  /**
+   * [#15840] The level the ruling names for the one read whose un-answered
+   * value would otherwise DESTROY standing state. Optional like its siblings:
+   * this is an input the caller supplies, not a channel this module publishes.
+   *
+   * ⚠️ Three parameters, not two: the platform `Logger` contract
+   * (`packages/spec/src/contracts/logger.ts`) takes the `Error` in its OWN
+   * second argument at this level and only this level. Declaring the sibling
+   * `(message, meta)` shape here would make the real `ctx.logger` unassignable
+   * — measured, as three TS2322s in `security-plugin.ts`.
+   */
+  error?: (message: string, error?: Error, meta?: Record<string, any>) => void;
 }
 
 function genId(prefix: string): string {
@@ -147,25 +171,66 @@ async function tryFind(
    */
   context: { isSystem: true; tenantId?: string } = SYSTEM_CTX,
 ): Promise<any[]> {
-  try {
-    const rows = await ql.find(object, { where, limit }, { context });
-    // Bare array, driven — see `engine-find-bare-array.pin.test.ts`, which boots
-    // a real engine over a real `SqlDriver` and pins this seam. The `{ records }`
-    // limb removed from here was dead code that read as a contract.
-    //
-    // The `[]` arm is left exactly as it was: this function's whole contract is
-    // `Promise<any[]>` best-effort, and turning it into a gap is a different
-    // change with a different blast radius than removing an unreachable limb.
-    return Array.isArray(rows) ? rows : [];
-  } catch (e) {
+  const answer = await readRows(ql, object, where, limit, context);
+  if (answer.answered) return answer.rows;
+  if (answer.why === 'threw') {
     // Reads legitimately fail before the tables exist (boot ordering), so this
     // is debug rather than warn — but it is no longer nothing (#4640).
     logger?.debug?.('[security] org-admin reconcile read failed — treated as no rows', {
       object,
-      error: (e as Error)?.message,
+      error: (answer.error as Error)?.message,
     });
-    return [];
   }
+  return [];
+}
+
+/**
+ * [#15840] What `tryFind` above throws away: WHETHER THE READ ANSWERED.
+ *
+ * `tryFind` maps both "the store answered nothing" and "the store did not
+ * answer" onto the same `[]`. For most of this module's reads that is a
+ * defensible best-effort contract — a missing answer means a grant is not
+ * created, and the next boot sweep asks again. For exactly one read it is not:
+ * the `sys_member` read that decides {@link reconcileOrgAdminGrant}'s
+ * `shouldGrant`. There, `[]` means "this user is not an admin of this org",
+ * which is the input to a DELETE — so an un-answered read withdraws a standing
+ * capability, and the store keeps it withdrawn after the fault clears.
+ *
+ * This is the read-seam invention rule in AGENTS.md, and #15840's ruling
+ * (decision batch #105 item 5, option A) is per-site, ⛔ NOT a uniform sweep:
+ * only the caller that turns the value into a revoke asks this question. Every
+ * other caller keeps going through `tryFind` and behaves exactly as it did.
+ *
+ * A non-array answer is reported as "did not answer" rather than as an empty
+ * page for the same reason: an engine that handed back `{ records: [...] }`
+ * would be carrying the memberships, and reading that envelope as "not a
+ * member" is the same wrongful revoke by another route. It is unreached on the
+ * shipped engine (#15598 drove every seam), and it stays unreachable-by-value
+ * rather than being silently re-invented here.
+ */
+type ReadAnswer =
+  | { answered: true; rows: any[] }
+  | { answered: false; why: 'threw'; error: unknown }
+  | { answered: false; why: 'not_an_array'; error?: undefined };
+
+async function readRows(
+  ql: any,
+  object: string,
+  where: any,
+  limit: number,
+  context: { isSystem: true; tenantId?: string },
+): Promise<ReadAnswer> {
+  let rows: any;
+  try {
+    rows = await ql.find(object, { where, limit }, { context });
+  } catch (e) {
+    return { answered: false, why: 'threw', error: e };
+  }
+  // Bare array, driven — see `engine-find-bare-array.pin.test.ts`, which boots
+  // a real engine over a real `SqlDriver` and pins this seam. The `{ records }`
+  // limb removed from here was dead code that read as a contract.
+  if (Array.isArray(rows)) return { answered: true, rows };
+  return { answered: false, why: 'not_an_array' };
 }
 
 async function tryInsert(ql: any, object: string, data: any, logger?: MaybeLogger): Promise<any | null> {
@@ -521,6 +586,11 @@ async function resolvePermissionSetIdsForName(
  *   removal symmetrically).
  *
  * Returns a structured report for observability. Never throws.
+ *
+ * [#15840] One `skipped` reason is load-bearing rather than diagnostic:
+ * `membership_unreadable` means the `sys_member` read did not answer, so this
+ * call declined to decide at all. It is NOT `noop` and NOT `revoked` — the pair
+ * is left exactly as it was found, and the caller's next round asks again.
  */
 export async function reconcileOrgAdminGrant(
   ql: any,
@@ -599,13 +669,38 @@ export async function reconcileOrgAdminGrant(
   //    in this org. Better-auth allows multiple membership rows per
   //    pair under some edge cases (legacy data) — any qualifying row
   //    is enough.
-  const memberships = await tryFind(
+  //
+  // [#15840] This is the read the ruling names, and it is the only read in this
+  // module asked through {@link readRows} instead of `tryFind`. `[]` here does
+  // not mean "nothing to do": it means `shouldGrant === false`, which is the
+  // input to the revoke branch below. So an un-answered read must NOT be spelled
+  // `[]` — that is the difference between "this user is not an admin" and "the
+  // store would not tell me", and the first of those DELETES a standing grant.
+  //
+  // ⛔ The skip lands HERE, before the superseded-revoke leg and before either
+  // branch: the ruling's disposition is that a read fault "skips that user for
+  // the round" and "never enters the revoke branch". A round that could not read
+  // performs no write at all — nothing is granted, so nothing widens, and
+  // nothing is revoked, so nothing standing is destroyed by a transient fault.
+  // The next `sys_member` write and the `kernel:ready` backfill ask again.
+  const membershipRead = await readRows(
     ql,
     'sys_member',
     { user_id: userId, organization_id: orgId },
     10,
-    logger,
+    SYSTEM_CTX,
   );
+  if (!membershipRead.answered) {
+    logger?.error?.(
+      '[security] org-admin reconcile SKIPPED — the sys_member read did not answer, so this ' +
+        'round cannot tell "not a member" from "could not ask"; NOTHING was granted or revoked ' +
+        'for this pair, and any standing grant is left exactly as it was',
+      membershipRead.error instanceof Error ? membershipRead.error : undefined,
+      { object: 'sys_member', userId, orgId, why: membershipRead.why },
+    );
+    return { action: 'skipped', reason: 'membership_unreadable' };
+  }
+  const memberships = membershipRead.rows;
   // The row that QUALIFIES is also the row the grant is provenance-linked to
   // (#4586) — "this capability exists because of that membership".
   const qualifyingMembership = memberships.find((m: any) => isAdminRole(m?.role));

packages/plugins/plugin-security/src/normalize-managed-by.ts

@@ -23,7 +23,10 @@
  * renaming a stored value without updating that map silently disarms the gate
  * (#2926 ①). Keep the two in lockstep whenever this vocabulary changes.
  * Idempotent: canonical rows are skipped, so a re-run is a no-op.
- * Best-effort and non-fatal, like the sibling boot reconcilers.
+ * Non-fatal to boot, like the sibling boot reconcilers — but [#15840] no longer
+ * best-effort about its own reads: a catalog read that does not answer refuses
+ * the pass instead of reporting the same counts an already-canonical catalog
+ * reports. The `kernel:ready` caller catches the refusal and carries on.
  *
  * Runs on `kernel:ready` after the seeders, as `isSystem` (the field is
  * `readonly`, so only a system write may set it).
@@ -45,20 +48,76 @@ interface NormalizeOptions {
   logger?: {
     info: (message: string, meta?: Record<string, any>) => void;
     warn: (message: string, meta?: Record<string, any>) => void;
+    /**
+     * [#15840] The level the ruling names for a refused pass. Optional, unlike
+     * its two siblings, so every caller that compiles today still compiles: it
+     * is an input this module asks for, not a channel it publishes.
+     *
+     * ⚠️ Three parameters, not two: the platform `Logger` contract
+     * (`packages/spec/src/contracts/logger.ts`) takes the `Error` in its OWN
+     * second argument at this level and only this level. Declaring the sibling
+     * `(message, meta)` shape here makes the real `ctx.logger` unassignable.
+     */
+    error?: (message: string, error?: Error, meta?: Record<string, any>) => void;
   };
 }
 
-async function tryFind(ql: any, object: string, where: any): Promise<any[]> {
+/**
+ * [#15840] Read the legacy rows, or REFUSE — never invent an empty catalog.
+ *
+ * The `catch { return []; }` that stood here is the read-seam invention rule's
+ * worst case, and it was measured (report 5553806224): an unreadable catalog and
+ * an already-canonical one were BYTE-IDENTICAL on both channels — the same
+ * `{ positions: 0, permissionSets: 0 }` and zero log lines at any level — while
+ * the row that needed healing stayed legacy. "I could not read the catalog" was
+ * reported as "the catalog is already canonical".
+ *
+ * #15840's ruling (decision batch #105 item 5, option A) is that a read fault
+ * REFUSES the normalisation pass for that batch and reports at `error`; ⛔ it
+ * never answers "already canonical". So the fault leaves this function as a
+ * throw, and {@link normalizeManagedByVocab} lets it out.
+ *
+ * The consumer contract for that throw already exists and is the reason a
+ * refusal is decidable here at all: `security-plugin.ts`'s `kernel:ready`
+ * bootstrap wraps this call in `try { … } catch { logger.warn('[security]
+ * managed_by vocab normalization failed (non-fatal)') }`, so boot proceeds and
+ * the remaining bootstrap steps still run. Nothing reached that handler before,
+ * because the fault was swallowed one frame below.
+ *
+ * ⚠️ Refusing is also the CHEAPER report. The report-and-continue option was
+ * measured at four lines per boot — this pass calls the read once per legacy
+ * value, three for `sys_position` and one for `sys_permission_set` — so a
+ * whole-catalog outage said the same thing four times. A refusal aborts at the
+ * first un-answered read, which is exactly one `error` line per refused boot.
+ *
+ * ⛔ Not a relaxation: the system-row write gate's provenance map recognizes
+ * BOTH the canonical and the legacy vocabulary (see the header note and
+ * #2926 ①), so rows left un-normalised by a refusal are still gated. Nothing is
+ * granted, widened or disarmed by declining to rewrite them; the next boot
+ * asks again.
+ */
+function readRefused(object: string, cause?: unknown): Error {
+  const why = cause === undefined ? 'the engine did not answer with a row array' : (cause as Error)?.message;
+  return new Error(
+    `[security] managed_by normalize REFUSED for ${object} — the catalog read did not answer, ` +
+      `so this pass cannot tell "already canonical" from "could not ask": ${why}`,
+  );
+}
+
+async function findOrRefuse(ql: any, object: string, where: any): Promise<any[]> {
+  let rows: any;
   try {
-    const rows = await ql.find(object, { where, limit: 10_000, fields: ['id', 'managed_by'] }, { context: SYSTEM_CTX });
-    // Bare array, driven — `engine-find-bare-array.pin.test.ts` boots a real
-    // engine over a real `SqlDriver` and pins this seam. The `{ records }` limb
-    // that stood here was dead code that read as a contract.
-    if (Array.isArray(rows)) return rows;
-    return [];
-  } catch {
-    return [];
+    rows = await ql.find(object, { where, limit: 10_000, fields: ['id', 'managed_by'] }, { context: SYSTEM_CTX });
+  } catch (e) {
+    throw readRefused(object, e);
   }
+  // Bare array, driven — `engine-find-bare-array.pin.test.ts` boots a real
+  // engine over a real `SqlDriver` and pins this seam. The `{ records }` limb
+  // that stood here was dead code that read as a contract; a non-array answer
+  // is now a refusal for the same reason a throw is — the pass did not get the
+  // rows, so it must not report on them.
+  if (Array.isArray(rows)) return rows;
+  throw readRefused(object);
 }
 
 async function normalizeObject(
@@ -71,7 +130,24 @@ async function normalizeObject(
   for (const [legacy, canonical] of Object.entries(map)) {
     // Narrow equality scan per legacy value keeps the where-clause
     // driver-portable (no IN / OR predicate).
-    const rows = await tryFind(ql, object, { managed_by: legacy });
+    let rows: any[];
+    try {
+      rows = await findOrRefuse(ql, object, { managed_by: legacy });
+    } catch (e) {
+      // [#15840] The one report the ruling names, emitted where the count that
+      // will NOT be returned is still known: rows healed before the refusal
+      // stay healed, and saying so is the difference between a refusal and a
+      // rollback. Reported once — the throw aborts the whole pass.
+      logger?.error?.((e as Error).message, e instanceof Error ? e : undefined, {
+        object,
+        legacyValue: legacy,
+        healedBeforeRefusal: updated,
+        consequence:
+          'the remaining legacy rows keep their legacy managed_by; the write gate recognizes ' +
+          'both vocabularies, so nothing is disarmed, and the next boot asks again',
+      });
+      throw e;
+    }
     for (const row of rows) {
       if (!row?.id) continue;
       try {
@@ -90,6 +166,15 @@ async function normalizeObject(
 /**
  * Rewrite legacy `managed_by` values on `sys_permission_set` and `sys_position`
  * to the unified tri-state vocab. Returns a per-object count of rows healed.
+ *
+ * [#15840] THROWS if a catalog read does not answer. The returned counts are an
+ * attestation — "these rows were legacy and are now canonical" — and a pass that
+ * could not read the catalog has nothing to attest, so it refuses rather than
+ * reporting `{ positions: 0, permissionSets: 0 }`, which is what an
+ * already-canonical catalog reports. Callers already handle this: the
+ * `kernel:ready` bootstrap catches it, reports at `warn` as non-fatal, and
+ * continues. An engine with no `find`/`update` at all is NOT a read fault and
+ * still returns zeros.
  */
 export async function normalizeManagedByVocab(
   ql: any,