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

* fix(plugin-security): tell a read that did not answer from one that answered nothing, per site (#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. A 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.

`normalizeManagedByVocab` swallowed a catalog read fault into `[]` too, so an
unreadable catalog and an already-canonical one were byte-identical on both
channels while the row that needed healing stayed legacy. It now reports at
`error` and refuses the pass rather than attesting counts it could not read. The
refusal aborts at the first un-answered read — one line per refused boot, not the
four the report-and-continue shape measured — and reaches the `kernel:ready`
catch that was already declared for it.

Per-site, not a sweep (#15840 ruling, decision batch #105 item 5, option A). A
genuine EMPTY read keeps today's behaviour exactly at both seams, pinned by
positive controls. `claim-seed-ownership.ts` is untouched — zero diff: its fault
already propagates to a per-predicate handler that reports at `warn` and names
the consequence, so the card's third row is falsified. The plugin's other reads
keep their best-effort contract, where an unanswered read costs a grant that is
not created rather than one that is destroyed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37

* fix(plugin-security): match the platform Logger contract's error() arity at both new report sites (#15840)

`Logger.error` (packages/spec/src/contracts/logger.ts) takes the `Error` in its
OWN second argument — `(message, error?, meta?)` — unlike `info`/`warn`/`debug`,
which take `(message, meta?)`. Both new optional `error?` members were declared
with the sibling two-parameter shape, which made the real `ctx.logger`
unassignable: measured as three TS2322s in `security-plugin.ts`, at :3818, :3911
and :3937.

Fixed at the declaration rather than by widening the consumer: the producer's
spelling is the contract. Both call sites now hand the caught `Error` to the
argument built for it, so a platform logger receives a real Error object rather
than a message flattened into meta.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37

* docs(permissions): re-tally the isSystem declaration count the new read seam adds (#15840)

`readRows`'s `context: { isSystem: true; tenantId?: string }` parameter is a
23rd `isSystem` declaration site, so `check:system-context-census` went red on
its `[declared-count]` clause (table says 22, census says 23). Mechanical repair,
applied by the gate's own `pnpm gen:system-context-census --fix`; one number in
the decomposition table, no prose and no anchors touched.

Attribution was MEASURED, not assumed — a first reading of the census JSON
suggested the site was not mine. Reverting only this branch's three source files
on top of the same merged tree gives declaration 22 and gate exit 0; restoring
them gives 23 and exit 1. It is mine.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37

* fix(plugin-security): guarantee MaybeLogger's `warn` channel now that `error` is optional (#9754)

`check:optional-error-sink` went red the moment this branch added an `error?`
member: that put `MaybeLogger` into the gate's population for the first time, and
it declared every channel optional. 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.

The gate's own remedy, taken exactly: `warn` loses its `?`. ⛔ Not by making
`error` REQUIRED (#9754 option C, falsified — hosts inject reduced sinks), and
⛔ not by requiring `info`, which is the level AGENTS.md → "Degradation log
levels" calls the reassuring half-truth.

Narrowing the type forced NO call-site change: every existing caller already
supplied `warn`, measured by tsc over both layers (typecheck exit 0, and
check:test-typecheck 0 files / 0 errors), plus 105 files / 1977 tests green.

The sibling `normalize-managed-by.ts` sink was checked rather than assumed: its
`info`/`warn` were already non-optional, so it sits in the gate's compliant set
and is not named. Census moves 33 -> 34 sinks with a guaranteed `warn`, and
"permit silence" 2 -> 1, the remaining one being the pre-existing baseline entry.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-trump

.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,