fix(plugin-sharing)!: a failed business-unit head or standalone manager read refuses instead of answering nobody

.changeset/29713-business-unit-head-manager-reads-refuse.md

@@ -0,0 +1,21 @@
+---
+'@objectstack/plugin-sharing': patch
+---
+
+fix(plugin-sharing)!: a failed business-unit head or standalone manager read refuses instead of answering no head or no manager
+
+Clause-②: no (narrowing)
+Metadata-protocol: none
+
+<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped and no metadata changes: the two lookups read the rows they already read, and refuse a read that already failed. No author has anything to rewrite. -->
+
+**BREAKING** only where the read fails: `BusinessUnitGraphService.headOf` and the standalone `BusinessUnitGraphService.managerOf` used to answer `null` then, and now refuse.
+
+The member, seed and descent reads of `BusinessUnitGraphService` already refuse on a failed read. Two sibling lookups on the same service did not.
+
+- **`headOf(businessUnitId)`.** A failed `sys_business_unit` read now throws. Before, it answered `null` (no head) and kept that answer for the rest of the pass, so the head stayed missing after the read recovered. Nothing is cached after a failed read now: the next call reads again.
+- **`managerOf(userId, organizationId)` without a `TeamGraphService`.** A failed `sys_user` read now throws. Before, it answered `null` (no manager). This fallback caches nothing, before or after. The organization screen keeps its fail-open posture, and a service built with a `teamGraph` still answers exactly what `TeamGraphService.managerOf` answers.
+- **A missing object is still nobody.** A stack that never registers, or never creates, `sys_business_unit` or `sys_user` keeps answering `null`. That is judged by `isObjectAbsentError`; any other error stays loud.
+- **No caller on this release changes behaviour.** No sharing rule, approval or ownership queue on this release calls either lookup. A caller of the exported class sees the refusal instead of `null`.
+
+**The remedy.** None is needed. A refusal means the business-unit or user table could not be read: fix that read, and the next lookup succeeds.

packages/plugins/plugin-sharing/src/business-unit-graph.ts

@@ -4,8 +4,6 @@ import type { IBusinessUnitGraphService } from '@objectstack/spec/contracts';
 import type { SharingEngine } from './sharing-service.js';
 import { TeamGraphService, managerIsProvablyOutsideOrg, readMembershipToCompletion } from './team-graph.js';
 
-const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const;
-
 /**
  * [#29655] Rows per page of a child-unit read — the bound the read always
  * carried, so a parent with fewer children issues the one read it always did,
@@ -261,21 +259,27 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService {
     return Array.from(new Set(rows.map((r) => String(r?.user_id ?? '')).filter(Boolean)));
   }
 
+  /**
+   * The unit's head (`manager_user_id`), or `null` when the unit has none.
+   *
+   * [#29713] Read through the walk {@link seedIsUsable} uses
+   * ({@link readMembershipToCompletion}), so a read that FAILS throws and
+   * nothing is cached for it. It caught any error to `null` and cached that
+   * for the pass: a consumer naming a unit's head would have dropped the head
+   * on an outage, and kept dropping it after the read recovered. A unit that
+   * does not exist, sits in another organization or names no head answers
+   * `null`, and so does a stack that never materialises `sys_business_unit`.
+   */
   async headOf(businessUnitId: string): Promise<string | null> {
     if (!businessUnitId) return null;
     if (this.cache.head!.has(businessUnitId)) return this.cache.head!.get(businessUnitId) ?? null;
-    let row: any = null;
-    try {
-      const rows = await this.engine.find('sys_business_unit', {
-        where: this.orgScope({ id: businessUnitId }),
-        fields: ['id', 'manager_user_id'],
-        limit: 1,
-        context: SYSTEM_CTX,
-      });
-      row = Array.isArray(rows) ? rows[0] : null;
-    } catch {
-      row = null;
-    }
+    const rows = await readMembershipToCompletion(this.engine, {
+      object: 'sys_business_unit',
+      where: this.orgScope({ id: businessUnitId }),
+      fields: ['manager_user_id'],
+      subject: `the head of business unit '${businessUnitId}'`,
+    });
+    const row = rows?.[0];
     const head = row?.manager_user_id ? String(row.manager_user_id) : null;
     this.cache.head!.set(businessUnitId, head);
     return head;
@@ -291,29 +295,34 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService {
    * unscreened answer from the same method name purely by how the service
    * happened to be constructed. A screen that depends on a constructor option
    * is not a screen.
+   *
+   * [#29713] The standalone fallback's `sys_user` read goes through
+   * {@link readMembershipToCompletion}: a read that FAILS throws, where it
+   * answered `null` ("no manager"); a stack that never materialises
+   * `sys_user` has no manager on file and answers `null`. The fallback caches
+   * nothing, before or after. The organization screen keeps its ruled
+   * fail-open posture ({@link managerIsProvablyOutsideOrg}), and the
+   * delegating limb is {@link TeamGraphService.managerOf}'s own answer,
+   * unchanged here.
    */
   async managerOf(userId: string, organizationId?: string): Promise<string | null> {
     if (this.teamGraph) return this.teamGraph.managerOf(userId, organizationId);
     // Standalone fallback: read sys_user.manager_id directly.
     if (!userId) return null;
     const org = organizationId ?? this.organizationId;
-    try {
-      const rows = await this.engine.find('sys_user', {
-        where: { id: userId },
-        fields: ['id', 'manager_id'],
-        limit: 1,
-        context: SYSTEM_CTX,
-      });
-      const row: any = Array.isArray(rows) ? rows[0] : null;
-      const managerId = row?.manager_id ? String(row.manager_id) : null;
-      if (!managerId) return null;
-      // The SAME screen the delegating limb applies — one implementation,
-      // imported rather than restated, so the two limbs cannot drift.
-      if (await managerIsProvablyOutsideOrg(this.engine, managerId, org)) return null;
-      return managerId;
-    } catch {
-      return null;
-    }
+    const rows = await readMembershipToCompletion(this.engine, {
+      object: 'sys_user',
+      where: { id: userId },
+      fields: ['manager_id'],
+      subject: `the manager of user '${userId}'`,
+    });
+    const row = rows?.[0];
+    const managerId = row?.manager_id ? String(row.manager_id) : null;
+    if (!managerId) return null;
+    // The SAME screen the delegating limb applies — one implementation,
+    // imported rather than restated, so the two limbs cannot drift.
+    if (await managerIsProvablyOutsideOrg(this.engine, managerId, org)) return null;
+    return managerId;
   }
 
   /**
@@ -358,7 +367,7 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService {
    * ## Why the member rows are screened at all
    *
    * Both member reads used to carry no organization predicate whatever, under
-   * a {@link SYSTEM_CTX} that carries no tenant either — so the query was
+   * a system context that carries no tenant either — so the query was
    * completely unscoped by organization: any unit an org-stamped rule could
    * see handed back EVERY tenant's membership rows hanging off it, and tenant
    * A's rule materialised real `sys_record_share` rows for tenant B's users.

packages/plugins/plugin-sharing/src/business-unit-head-manager-reads.test.ts

@@ -0,0 +1,208 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#29713] `BusinessUnitGraphService.headOf` and the standalone `managerOf`
+ * fallback answer the real head or manager, or refuse — never an invented
+ * "nobody" on a read that failed. #29606 and #29655 closed the member, seed
+ * and descent reads of the same service; these were the two left.
+ *
+ *   1. `headOf` caught any error from its `sys_business_unit` read to `null`
+ *      and cached that `null` for the pass, so the head stayed missing after
+ *      the read recovered.
+ *   2. The standalone `managerOf` (no `TeamGraphService` wired) caught any
+ *      error from its `sys_user` read to `null`. It caches nothing.
+ *
+ * Absence is the one failure that IS an answer — the object is not
+ * registered, or its table was never created — judged by `isObjectAbsentError`
+ * through `readMembershipToCompletion`, the walk `seedIsUsable` reads with.
+ *
+ * Measured on a REAL engine (ObjectQL + SqlDriver over in-memory SQLite)
+ * carrying the shipped identity objects, so "absent" is what a real driver
+ * answers for a missing object or table, not what a double guesses.
+ */
+
+import { describe, it, expect, afterEach, vi } from 'vitest';
+import { ObjectQL } from '@objectstack/objectql';
+import { SqlDriver } from '@objectstack/driver-sql';
+import { SysBusinessUnit, SysMember, SysOrganization, SysUser } from '@objectstack/platform-objects/identity';
+import { BusinessUnitGraphService } from './business-unit-graph.js';
+import { TeamGraphService } from './team-graph.js';
+
+const ORG = 'org_head';
+const UNIT = 'bu_sales';
+const HEADLESS = 'bu_headless';
+const HEAD = 'usr_head';
+const ALICE = 'usr_alice';
+const BOB = 'usr_bob';
+
+interface Rig {
+  engine: ObjectQL;
+  driver: SqlDriver;
+  /** Fail the next `times` reads of `object`; every other read runs as before. */
+  failReads: (object: 'sys_business_unit' | 'sys_user', times?: number) => void;
+}
+
+const engines: ObjectQL[] = [];
+afterEach(async () => {
+  vi.restoreAllMocks();
+  while (engines.length) {
+    try { await engines.pop()?.destroy(); } catch { /* noop */ }
+  }
+});
+
+/** UNIT headed by HEAD, HEADLESS with no head; ALICE managed by BOB, both members of ORG. */
+async function boot(opts: { omit?: string[] } = {}): Promise<Rig> {
+  const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as never);
+  const engine = new ObjectQL();
+  engine.registerDriver(driver as never, true);
+  await engine.init();
+  const omit = new Set(opts.omit ?? []);
+  engine.registerApp({
+    id: 'com.objectstack.qa.business-unit-head-manager-reads',
+    name: 'business unit head and manager reads',
+    version: '1.0.0',
+    type: 'plugin',
+    scope: 'system',
+    objects: [SysOrganization, SysBusinessUnit, SysUser, SysMember].filter((o) => !omit.has(o.name)),
+  } as never);
+  await engine.syncSchemas();
+  engines.push(engine);
+
+  const e = engine as any;
+  await e.insert('sys_organization', { id: ORG, name: ORG, slug: 'head' }, { context: { isSystem: true } });
+  if (!omit.has('sys_business_unit')) {
+    await e.getDriverForObject('sys_business_unit').bulkCreate('sys_business_unit', [
+      { id: UNIT, name: 'Sales', kind: 'department', active: true, manager_user_id: HEAD, organization_id: ORG },
+      { id: HEADLESS, name: 'Headless', kind: 'department', active: true, organization_id: ORG },
+    ]);
+  }
+  if (!omit.has('sys_user')) {
+    await e.getDriverForObject('sys_user').bulkCreate('sys_user', [
+      { id: ALICE, name: 'Alice', email: 'alice@example.test', manager_id: BOB },
+      { id: BOB, name: 'Bob', email: 'bob@example.test' },
+    ]);
+  }
+  await e.getDriverForObject('sys_member').bulkCreate('sys_member', [
+    { id: 'mem_alice', user_id: ALICE, organization_id: ORG, role: 'member' },
+    { id: 'mem_bob', user_id: BOB, organization_id: ORG, role: 'member' },
+  ]);
+
+  // The driver's own `find`, taken once: a second fault wraps it again rather
+  // than wrapping the first fault's spy.
+  const d = e.getDriverForObject('sys_organization');
+  const real = d.find.bind(d);
+  const failReads: Rig['failReads'] = (object, times = Number.POSITIVE_INFINITY) => {
+    let left = times;
+    vi.spyOn(d, 'find').mockImplementation(((obj: string, ...rest: unknown[]) => {
+      if (obj === object && left > 0) {
+        left -= 1;
+        return Promise.reject(Object.assign(new Error('SQLITE_IOERR: disk I/O error'), { code: 'SQLITE_IOERR' }));
+      }
+      return real(obj, ...rest);
+    }) as never);
+  };
+  return { engine, driver, failReads };
+}
+
+const graph = (rig: Rig, extra: { teamGraph?: TeamGraphService } = {}) =>
+  new BusinessUnitGraphService({ engine: rig.engine as never, organizationId: ORG, ...extra });
+
+describe('[#29713] headOf: a failed unit read refuses, and nothing is cached for it', () => {
+  it('control — a healthy read answers the head, and a unit with no head answers null', async () => {
+    const g = graph(await boot());
+
+    expect(await g.headOf(UNIT)).toBe(HEAD);
+    expect(await g.headOf(HEADLESS)).toBeNull();
+  });
+
+  it('⛔ a failed read refuses — it never answers an invented null', async () => {
+    const rig = await boot();
+    rig.failReads('sys_business_unit');
+
+    await expect(graph(rig).headOf(UNIT)).rejects.toThrow(
+      /\[sharing\] the head of business unit 'bu_sales' could not be read: SQLITE_IOERR/,
+    );
+  });
+
+  it('⛔ a failed read is not cached: the next read on the same pass answers the real head', async () => {
+    const rig = await boot();
+    rig.failReads('sys_business_unit', 1);
+    const g = graph(rig);
+
+    await expect(g.headOf(UNIT)).rejects.toThrow(/could not be read/);
+    expect(await g.headOf(UNIT)).toBe(HEAD);
+  });
+
+  it('absence — a composition that never registers sys_business_unit has no head: null', async () => {
+    const g = graph(await boot({ omit: ['sys_business_unit'] }));
+
+    expect(await g.headOf(UNIT)).toBeNull();
+  });
+
+  it('absence — a stack whose sys_business_unit table was never created has no head: null', async () => {
+    const rig = await boot();
+    await (rig.driver as any).knex.schema.dropTable('sys_business_unit');
+
+    expect(await graph(rig).headOf(UNIT)).toBeNull();
+  });
+});
+
+describe('[#29713] standalone managerOf: a failed user read refuses', () => {
+  it('control — a healthy read answers the manager, and a user with none answers null', async () => {
+    const g = graph(await boot());
+
+    expect(await g.managerOf(ALICE, ORG)).toBe(BOB);
+    expect(await g.managerOf(BOB, ORG)).toBeNull();
+  });
+
+  it('⛔ a failed read refuses — it never answers an invented null', async () => {
+    const rig = await boot();
+    rig.failReads('sys_user');
+
+    await expect(graph(rig).managerOf(ALICE, ORG)).rejects.toThrow(
+      /\[sharing\] the manager of user 'usr_alice' could not be read: SQLITE_IOERR/,
+    );
+  });
+
+  it('⛔ a failed read is not cached: the next read on the same pass answers the real manager', async () => {
+    const rig = await boot();
+    rig.failReads('sys_user', 1);
+    const g = graph(rig);
+
+    await expect(g.managerOf(ALICE, ORG)).rejects.toThrow(/could not be read/);
+    expect(await g.managerOf(ALICE, ORG)).toBe(BOB);
+  });
+
+  it('absence — a composition that never registers sys_user has no manager on file: null', async () => {
+    const g = graph(await boot({ omit: ['sys_user'] }));
+
+    expect(await g.managerOf(ALICE, ORG)).toBeNull();
+  });
+
+  it('absence — a stack whose sys_user table was never created has no manager on file: null', async () => {
+    const rig = await boot();
+    await (rig.driver as any).knex.schema.dropTable('sys_user');
+
+    expect(await graph(rig).managerOf(ALICE, ORG)).toBeNull();
+  });
+});
+
+describe('[#29713] CONTROL — the wired TeamGraphService limb is unchanged', () => {
+  it('a wired managerOf answers exactly what TeamGraphService.managerOf answers, on a healthy and on a failed read', async () => {
+    const rig = await boot();
+    const team = new TeamGraphService({ engine: rig.engine as never, organizationId: ORG });
+    const g = graph(rig, { teamGraph: team });
+
+    expect(await g.managerOf(ALICE, ORG)).toBe(BOB);
+    expect(await g.managerOf(ALICE, ORG)).toBe(await new TeamGraphService({ engine: rig.engine as never }).managerOf(ALICE, ORG));
+
+    // TeamGraphService's own failure posture, not this card's: it is passed
+    // through as it stands, and this card changes nothing on that limb.
+    rig.failReads('sys_user');
+    const failed = graph(rig, { teamGraph: new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }) });
+    expect(await failed.managerOf(ALICE, ORG)).toBeNull();
+    expect(await failed.managerOf(ALICE, ORG)).toBe(
+      await new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }).managerOf(ALICE, ORG),
+    );
+  });
+});