Skip to content

Commit bbb98c4

Browse files
fix(plugin-sharing)!: a failed team-graph manager read refuses instead of answering no manager (#29836)
<!-- claude-projects-attribution: {"github_login":"os-warren"} --> _Requested by **Warren** · [project thread](https://claude.ai/code/project/chan_01U5RdnAAmkSQmZbqh6FtqZ8?thread=cmsg_01U5RdnAAmkSQmZbqh6FtqZ87Fk1F3zNZJFiNofw8PYfxN)_ Fixes #29792 domain:server#1 · os-warren · session_01JQwxbXvyuTnWc7sv5i5DBw Clause-②: no (narrowing) Metadata-protocol: none Skills-docs: not needed. No skill and no `content/docs` page names `TeamGraphService.managerOf` or `expandPrincipal`, or says what either answers on a failed read. ## 维护者速读 - 改了什么:`TeamGraphService.managerOf`(查用户的经理)读 `sys_user` 失败时不再答「没有经理」,而是明确报错,且失败结果不再缓存一整轮;对象或表不存在仍答 `null`。报错文案新增「lookup」一类,单个负责人/经理的查询不再说成「持有人集合」。`packages/spec` 里两个接口方法的注释从「best-effort」改为「读失败即报错;没有则为 null」。 - 为什么改:#29647、#29703、#29787 已让 `plugin-sharing` 里决定授权的读在故障时报错;这是同族最后一处仍把故障吞成 `null` 并缓存到本轮结束的读,读恢复后经理仍是空的。今天没有调用方,但第一个按「记录所有者的经理」授权或派审批的规则会在故障期间悄悄漏掉经理。 - 风险与回滚:三个仓里都没有生产调用方(`expandPrincipal` 无调用方,`BusinessUnitGraphService.managerOf` 接线分支也无调用方,已逐一追踪),故本版运行行为不变;唯一「从答 `null` 变成报错」的是导出类或函数的外部调用方在读失败时,属收窄,标题带 `!`。`sys_member` 组织筛选的读失败放行(#10153 裁决)不变。回滚即 revert 本 PR。 - 先看:`packages/plugins/plugin-sharing/src/team-graph.ts`(`managerOf` 与 `holderReadRefusal`)、`packages/plugins/plugin-sharing/src/business-unit-head-manager-reads.test.ts`、`packages/spec/src/contracts/sharing-service.ts`(仅注释)。 - 两分钟验证:`pnpm --filter @objectstack/plugin-sharing exec vitest run src/business-unit-head-manager-reads.test.ts`,预期 18/18 通过(读失败报错、恢复后答真值、`expandPrincipal` 一并报错、对象缺失答 null、组织筛选仍放行)。 - 需要你做的:无,席位落地。 ## What changes `TeamGraphService.managerOf` (`packages/plugins/plugin-sharing/src/team-graph.ts`) reads `sys_user` through `readMembershipToCompletion`, the walk #29787 gave `BusinessUnitGraphService.headOf` and the standalone `managerOf`. No new helper. - **A failed read refuses.** It throws `[sharing] the manager of user 'ID' could not be read: …`, with the driver error as `cause`. It used to answer `null` (no manager) and cache that `null` under `org::userId` for the pass. - **Nothing is cached after a failed read.** The throw comes before the cache `set`, so the next call on the same pass reads again. A healthy answer, a manager or `null`, is still cached as before. - **Absence is still nobody.** A stack that never registers or never creates `sys_user` answers `null` (`isObjectAbsentError`, through the walk). - **The organization screen is unchanged.** `managerIsProvablyOutsideOrg` keeps its ruled fail-open posture on `sys_member` (#10153). A control pin holds it. - **S4, a `lookup` arm.** `holderReadRefusal` takes a third kind, `lookup`, for a single head or manager. Its closing sentence says answering "none" would drop the person it names, instead of speaking of a holder set or an expansion. `TeamGraphService.managerOf`, `BusinessUnitGraphService.headOf` and the standalone `BusinessUnitGraphService.managerOf` all read as `lookup`. The `expansion` and `check` sentences are byte-identical. - **S1, the contract prose.** In `packages/spec/src/contracts/sharing-service.ts`, the JSDoc of `ITeamGraphService.managerOf` and `IBusinessUnitGraphService.headOf` reads "refuses (throws) on a failed read; `null` when none" instead of "best-effort". JSDoc only: no key, type or signature changes (`check:api-surface` green). It ships in `dist/contracts/index.d.ts`, so `@objectstack/spec` takes a `patch`. - No `protocol.ts`, no new export, no metadata change. ### Measured first (real engine, before and after) A scratch probe on ObjectQL + SqlDriver over in-memory SQLite, with the shipped identity objects, ran on `main` (`eb05ea2f`) and on the fix (`872978b4`). One `sys_user` read failed, then the same instance was called again. The probe's users are `alice`, managed by `bob`. | Reading | `main` | this PR | |:--|:--|:--| | healthy read | `bob` | `bob` | | failed read | `null` | throws `the manager of user 'alice' could not be read: SQLITE_IOERR…` | | same instance, after the read recovers | `null` (the failed answer was cached) | `bob` | | a new instance, after recovery | `bob` | `bob` | | `sys_user` not registered | `null` | `null` | | `sys_user` table dropped | `null` | `null` | The card's premise holds: the failed answer was cached under `org::userId` for the pass. ### Callers (traced in objectstack, hotcrm and docs.objectos.ai) | Caller | Where | What it sees on a refusal | |:--|:--|:--| | `expandPrincipal` `manager` arm | `team-graph.ts`, exported from the package index | the refusal propagates. It has no caller in any of the three repositories | | `BusinessUnitGraphService.managerOf`, wired limb | `business-unit-graph.ts` | passes the refusal through. The method has no production caller (#29787 trace) | | Sharing rules (`SharingRuleService`) | builds a `TeamGraphService` | calls `expandUsers` / `expandRoleUsers` only: nothing changes | | Ownership queues (`ownership-queue-service.ts`) | builds a `TeamGraphService` | `expandUsers` / `expandRoleUsers` only: nothing changes | | Positions (`PositionGraphService`) | holds a `TeamGraphService` | `holdsRole` / role reads only: nothing changes | | Approvals (`plugin-approvals`) | its own `lookupManager` | does not call `TeamGraphService.managerOf`: nothing changes | hotcrm and docs.objectos.ai have no reference to `managerOf`, `expandPrincipal` or `TeamGraphService` at their `origin/main` (`d4ffe99`, `4a00661`). No caller fails a whole pass where it used to drop one person, so the STOP condition did not trip. ## Tests `src/business-unit-head-manager-reads.test.ts` is the shared real-engine harness for these lookups, so the new pins go into it as a top-level `describe`. It now has 18 tests. New, `[#29792] TeamGraphService.managerOf`: | Pin | Asserts | |:--|:--| | control: healthy read | the manager; a user without one answers `null` | | ⛔ failed read refuses | throws, names the user; `cause.code` is the driver's `SQLITE_IOERR` | | ⛔ not cached | the same instance answers `usr_bob` after one failed read | | ⛔ `expandPrincipal` manager arm | `[usr_bob]` when healthy; refuses when the read fails | | absence: object never registered | `null` | | absence: table never created | `null` | | control: organization screen | an unreadable `sys_member` keeps the manager | **S3.** The `[#29713] CONTROL` pin no longer hard-codes `toBeNull()` for the wired limb on a failed read. Its equality control stays: the wired limb's settled outcome (answer or refusal message) equals `TeamGraphService.managerOf`'s, on a healthy and on a failed read. The refusal sentence's wording (S4) is not pinned: no consumer parses it. The existing pins match the first clause, which did not change. - `pnpm --filter @objectstack/plugin-sharing test` at `872978b4`: 65 files / 1387 tests passed. - `pnpm --filter @objectstack/plugin-sharing typecheck`: exit 0 (its `check:test-typecheck` compiles the test layer). - `pnpm --filter @objectstack/spec typecheck`: exit 0. `pnpm --filter @objectstack/spec check:generated`: exit 0, all 14 artifacts current. <details><summary>Ablations: each negative pin's behaviour put back (red), restored by ablation-replace (blob == HEAD, git diff HEAD empty)</summary> The test imports the subject from `src/` by relative path, so no build sits between the mutation and the run. A1, refusal: `.catch(() => null)` put back on the `sys_user` read in `TeamGraphService.managerOf` ``` ablation-replace: anchor x1 -> x0 ablation-replace: blob 611685b -> 8f45c95f20455b37cc4fdf8f539f8a1527eee57b × ⛔ a failed read refuses — it never answers an invented null 16ms × ⛔ a failed read is not cached: the next read on the same pass answers the real manager 13ms × ⛔ expandPrincipal's manager arm refuses with it, rather than answering nobody 16ms Tests 3 failed | 15 passed (18) ablation-replace: ok restored: blob == HEAD (611685b) and `git diff HEAD` is empty ``` A2, cache: the read still throws, but caches `null` for the key first ``` ablation-replace: anchor x1 -> x0 ablation-replace: blob 611685b -> e2a786a9ddc37a2ac4410452259c5c4da16ea26b × ⛔ a failed read is not cached: the next read on the same pass answers the real manager 16ms Tests 1 failed | 17 passed (18) ablation-replace: ok restored: blob == HEAD (611685b) and `git diff HEAD` is empty ``` A3, `expandPrincipal`: `.catch(() => null)` on the manager arm's `managerOf` call ``` ablation-replace: anchor x1 -> x0 ablation-replace: blob 611685b -> 33b2407d944b8883cc8878b0ca462b650cb904de × ⛔ expandPrincipal's manager arm refuses with it, rather than answering nobody 24ms Tests 1 failed | 17 passed (18) ablation-replace: ok restored: blob == HEAD (611685b) and `git diff HEAD` is empty ``` </details> ## Acceptance notes - `expandPrincipal`'s `manager` arm now refuses with the lookup instead of answering `[]`. The function is exported and has no caller in the three repositories, so nothing on this release changes. A future caller decides per pass what a refused principal means. - `managerIsProvablyOutsideOrg` still answers "not outside" when `sys_member` cannot be read, by the #10153 ruling. This PR keeps it and pins it. ## Verification Readings at `0609359` (the fix `872978b4` plus a merge of `main` at `b62463f1`) unless named. The merge brought no change to `plugin-sharing`; the derived gate list is identical before and after it. - `node scripts/pm/dispatch-gates.mjs --commands`: 82 derived families, run at `872978b4` and derived again, identical, at `0609359`. `--ran` reconciliation exit 0: "82 derived, 82 run, 0 NOT-MEASURED". Three first exited 3 on prerequisites, not on the diff: the `check-plugin-teardown-shape` self-test on a shallow clone, and `check:dual-build-cjs-loads` and `check:i18n` before the packages had a `dist/`. All three passed at `0609359` after `git fetch --unshallow origin main` and a full `pnpm build` (117/117 tasks). - `pnpm lint`: exit 0 (at `872978b4`). - `pnpm --filter @objectstack/spec check:generated`: exit 0. The JSDoc is present in `packages/spec/dist/contracts/index.d.ts` and `.d.mts` after the build, which is why `@objectstack/spec` takes a `patch`. - Dogfood battery (the claim carries a `(narrowing)` arm): `0609359` · `os-verify-lock: VERDICT command-exit 0 · held the lock 1439s` · Test Files 405 passed, 3 skipped (408) · Tests 3352 passed, 28 skipped (3380) · no failures. - Every family CI runs is cited from this PR's check-runs at landing, not re-run here. --- _Generated by [Claude Code](https://claude.ai/code/session_01JQwxbXvyuTnWc7sv5i5DBw)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent bb2c04e commit bbb98c4

5 files changed

Lines changed: 151 additions & 31 deletions

File tree

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
---
2+
'@objectstack/plugin-sharing': patch
3+
'@objectstack/spec': patch
4+
---
5+
6+
fix(plugin-sharing)!: a failed team-graph manager read refuses instead of answering no manager
7+
8+
Clause-②: no (narrowing)
9+
Metadata-protocol: none
10+
11+
<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped and no metadata changes: the lookup reads the row it already read, and refuses a read that already failed. The spec change is interface documentation only. No author has anything to rewrite. -->
12+
13+
**BREAKING** only where the read fails: `TeamGraphService.managerOf` used to answer `null` then, and now refuses.
14+
15+
The holder reads of `TeamGraphService`, and the head and standalone manager lookups of `BusinessUnitGraphService`, already refuse on a failed read. This was the last lookup of that family that did not.
16+
17+
- **`TeamGraphService.managerOf(userId, organizationId)`.** A failed `sys_user` read now throws. Before, it answered `null` (no manager) and kept that answer for the rest of the pass, so the manager stayed missing after the read recovered. Nothing is cached after a failed read now: the next call reads again.
18+
- **Who sees it.** The `manager` arm of the exported `expandPrincipal`, and `BusinessUnitGraphService.managerOf` when it is built with a `teamGraph`, pass the refusal through instead of answering no manager.
19+
- **A missing object is still nobody.** A stack that never registers, or never creates, `sys_user` keeps answering `null`. That is judged by `isObjectAbsentError`; any other error stays loud.
20+
- **The organization screen is unchanged.** When `sys_member` cannot be read, the manager is still kept, as before.
21+
- **The refusal names a lookup.** A failed head or manager lookup now says it is a lookup of one person, not a holder set or an expansion.
22+
- **The contract says so.** The documentation of `ITeamGraphService.managerOf` and `IBusinessUnitGraphService.headOf` in `@objectstack/spec` now reads "refuses (throws) on a failed read; `null` when none" instead of "best-effort". No key or type changes.
23+
- **No caller on this release changes behaviour.** No sharing rule, approval or ownership queue on this release calls `managerOf` or `expandPrincipal`. A caller of the exported class or function sees the refusal instead of `null`.
24+
25+
**The remedy.** None is needed. A refusal means the user table could not be read: fix that read, and the next lookup succeeds.

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,7 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService {
278278
where: this.orgScope({ id: businessUnitId }),
279279
fields: ['manager_user_id'],
280280
subject: `the head of business unit '${businessUnitId}'`,
281+
kind: 'lookup',
281282
});
282283
const row = rows?.[0];
283284
const head = row?.manager_user_id ? String(row.manager_user_id) : null;
@@ -302,8 +303,9 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService {
302303
* `sys_user` has no manager on file and answers `null`. The fallback caches
303304
* nothing, before or after. The organization screen keeps its ruled
304305
* fail-open posture ({@link managerIsProvablyOutsideOrg}), and the
305-
* delegating limb is {@link TeamGraphService.managerOf}'s own answer,
306-
* unchanged here.
306+
* delegating limb is {@link TeamGraphService.managerOf}'s own answer.
307+
* [#29792] That answer now refuses on a failed read too, so both limbs
308+
* agree on the failure as well as on the screen.
307309
*/
308310
async managerOf(userId: string, organizationId?: string): Promise<string | null> {
309311
if (this.teamGraph) return this.teamGraph.managerOf(userId, organizationId);
@@ -315,6 +317,7 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService {
315317
where: { id: userId },
316318
fields: ['manager_id'],
317319
subject: `the manager of user '${userId}'`,
320+
kind: 'lookup',
318321
});
319322
const row = rows?.[0];
320323
const managerId = row?.manager_id ? String(row.manager_id) : null;

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

Lines changed: 75 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@
1111
* the read recovered.
1212
* 2. The standalone `managerOf` (no `TeamGraphService` wired) caught any
1313
* error from its `sys_user` read to `null`. It caches nothing.
14+
* 3. [#29792] `TeamGraphService.managerOf` — the wired limb, and the
15+
* `manager` arm of `expandPrincipal` — caught any error from its own
16+
* `sys_user` read to `null` and cached that `null` under `org::userId`
17+
* for the pass, so the manager stayed missing after the read recovered.
1418
*
1519
* Absence is the one failure that IS an answer — the object is not
1620
* registered, or its table was never created — judged by `isObjectAbsentError`
@@ -26,7 +30,7 @@ import { ObjectQL } from '@objectstack/objectql';
2630
import { SqlDriver } from '@objectstack/driver-sql';
2731
import { SysBusinessUnit, SysMember, SysOrganization, SysUser } from '@objectstack/platform-objects/identity';
2832
import { BusinessUnitGraphService } from './business-unit-graph.js';
29-
import { TeamGraphService } from './team-graph.js';
33+
import { TeamGraphService, expandPrincipal } from './team-graph.js';
3034

3135
const ORG = 'org_head';
3236
const UNIT = 'bu_sales';
@@ -39,7 +43,7 @@ interface Rig {
3943
engine: ObjectQL;
4044
driver: SqlDriver;
4145
/** Fail the next `times` reads of `object`; every other read runs as before. */
42-
failReads: (object: 'sys_business_unit' | 'sys_user', times?: number) => void;
46+
failReads: (object: 'sys_business_unit' | 'sys_user' | 'sys_member', times?: number) => void;
4347
}
4448

4549
const engines: ObjectQL[] = [];
@@ -107,6 +111,12 @@ async function boot(opts: { omit?: string[] } = {}): Promise<Rig> {
107111
const graph = (rig: Rig, extra: { teamGraph?: TeamGraphService } = {}) =>
108112
new BusinessUnitGraphService({ engine: rig.engine as never, organizationId: ORG, ...extra });
109113

114+
const team = (rig: Rig) => new TeamGraphService({ engine: rig.engine as never, organizationId: ORG });
115+
116+
/** An answer or a refusal, as one comparable value. */
117+
const settled = (p: Promise<unknown>) =>
118+
p.then((value) => ({ value }), (err: unknown) => ({ refused: err instanceof Error ? err.message : String(err) }));
119+
110120
describe('[#29713] headOf: a failed unit read refuses, and nothing is cached for it', () => {
111121
it('control — a healthy read answers the head, and a unit with no head answers null', async () => {
112122
const g = graph(await boot());
@@ -187,22 +197,76 @@ describe('[#29713] standalone managerOf: a failed user read refuses', () => {
187197
});
188198
});
189199

190-
describe('[#29713] CONTROL — the wired TeamGraphService limb is unchanged', () => {
200+
describe('[#29713] CONTROL — the wired limb answers what TeamGraphService.managerOf answers', () => {
191201
it('a wired managerOf answers exactly what TeamGraphService.managerOf answers, on a healthy and on a failed read', async () => {
192202
const rig = await boot();
193-
const team = new TeamGraphService({ engine: rig.engine as never, organizationId: ORG });
194-
const g = graph(rig, { teamGraph: team });
203+
const g = graph(rig, { teamGraph: team(rig) });
195204

196205
expect(await g.managerOf(ALICE, ORG)).toBe(BOB);
197206
expect(await g.managerOf(ALICE, ORG)).toBe(await new TeamGraphService({ engine: rig.engine as never }).managerOf(ALICE, ORG));
198207

199-
// TeamGraphService's own failure posture, not this card's: it is passed
200-
// through as it stands, and this card changes nothing on that limb.
208+
// The wired limb passes TeamGraphService's answer through, its refusal
209+
// included: the two outcomes are equal, whatever that outcome is.
210+
rig.failReads('sys_user');
211+
const failed = graph(rig, { teamGraph: team(rig) });
212+
expect(await settled(failed.managerOf(ALICE, ORG))).toEqual(await settled(team(rig).managerOf(ALICE, ORG)));
213+
});
214+
});
215+
216+
describe('[#29792] TeamGraphService.managerOf: a failed user read refuses, and nothing is cached for it', () => {
217+
it('control — a healthy read answers the manager, and a user with none answers null', async () => {
218+
const t = team(await boot());
219+
220+
expect(await t.managerOf(ALICE, ORG)).toBe(BOB);
221+
expect(await t.managerOf(BOB, ORG)).toBeNull();
222+
});
223+
224+
it('⛔ a failed read refuses — it never answers an invented null', async () => {
225+
const rig = await boot();
226+
rig.failReads('sys_user');
227+
228+
const refusal = team(rig).managerOf(ALICE, ORG);
229+
await expect(refusal).rejects.toThrow(/\[sharing\] the manager of user 'usr_alice' could not be read: SQLITE_IOERR/);
230+
await expect(refusal).rejects.toMatchObject({ cause: { code: 'SQLITE_IOERR' } });
231+
});
232+
233+
it('⛔ a failed read is not cached: the next read on the same pass answers the real manager', async () => {
234+
const rig = await boot();
235+
rig.failReads('sys_user', 1);
236+
const t = team(rig);
237+
238+
await expect(t.managerOf(ALICE, ORG)).rejects.toThrow(/could not be read/);
239+
expect(await t.managerOf(ALICE, ORG)).toBe(BOB);
240+
});
241+
242+
it('⛔ expandPrincipal\'s manager arm refuses with it, rather than answering nobody', async () => {
243+
const rig = await boot();
244+
const principal = { type: 'manager', value: 'owner_id', record: { owner_id: ALICE } };
245+
246+
expect(await expandPrincipal(principal, { team: team(rig), organizationId: ORG })).toEqual([BOB]);
201247
rig.failReads('sys_user');
202-
const failed = graph(rig, { teamGraph: new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }) });
203-
expect(await failed.managerOf(ALICE, ORG)).toBeNull();
204-
expect(await failed.managerOf(ALICE, ORG)).toBe(
205-
await new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }).managerOf(ALICE, ORG),
248+
await expect(expandPrincipal(principal, { team: team(rig), organizationId: ORG })).rejects.toThrow(
249+
/the manager of user 'usr_alice' could not be read/,
206250
);
207251
});
252+
253+
it('absence — a composition that never registers sys_user has no manager on file: null', async () => {
254+
const t = team(await boot({ omit: ['sys_user'] }));
255+
256+
expect(await t.managerOf(ALICE, ORG)).toBeNull();
257+
});
258+
259+
it('absence — a stack whose sys_user table was never created has no manager on file: null', async () => {
260+
const rig = await boot();
261+
await (rig.driver as any).knex.schema.dropTable('sys_user');
262+
263+
expect(await team(rig).managerOf(ALICE, ORG)).toBeNull();
264+
});
265+
266+
it('control — the organization screen stays fail-open: an unreadable sys_member keeps the manager', async () => {
267+
const rig = await boot();
268+
rig.failReads('sys_member');
269+
270+
expect(await team(rig).managerOf(ALICE, ORG)).toBe(BOB);
271+
});
208272
});

‎packages/plugins/plugin-sharing/src/team-graph.ts‎

Lines changed: 38 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -136,15 +136,28 @@ function roleHolderPredicate(roleName: string, org: string | null | undefined):
136136
*/
137137
const MEMBERSHIP_PAGE_ROWS = 10000;
138138

139+
/**
140+
* What a refused read was for, which decides how its refusal words the
141+
* consequence: a holder-set `expansion`, a user-scoped holder `check`, or a
142+
* `lookup` of the one person a head or manager column names.
143+
*/
144+
type HolderReadKind = 'expansion' | 'check' | 'lookup';
145+
139146
/**
140147
* [#29503] Why a holder read refused, as one sentence family: what could not
141148
* be read, the reason, and why the answer is a refusal rather than "nobody".
142149
* The driver's error rides along as `cause`.
150+
*
151+
* [#29792] A `lookup` reads one person (a unit's head, a user's manager), not
152+
* a set of holders, so its refusal says so: answering "none" would drop that
153+
* person from whatever the caller routes through the answer.
143154
*/
144-
function holderReadRefusal(subject: string, reason: string, cause: unknown, kind: 'expansion' | 'check'): Error {
155+
function holderReadRefusal(subject: string, reason: string, cause: unknown, kind: HolderReadKind): Error {
145156
const consequence = kind === 'expansion'
146157
? 'A shortened or empty holder set would withhold or revoke the grants it confers, so the expansion refuses instead.'
147-
: 'Answering "not a holder" would revoke the grants the holding confers, so the check refuses instead.';
158+
: kind === 'check'
159+
? 'Answering "not a holder" would revoke the grants the holding confers, so the check refuses instead.'
160+
: 'Answering "none" would drop the person it names from everything routed through the answer, so the lookup refuses instead.';
148161
return Object.assign(
149162
new Error(`[sharing] ${subject} could not be read: ${reason}. ${consequence}`),
150163
cause === undefined ? {} : { cause },
@@ -188,8 +201,11 @@ export async function readMembershipToCompletion(
188201
fields: readonly string[];
189202
subject: string;
190203
pageRows?: number;
191-
/** A user-scoped holder check words its refusal as a check, not an expansion. */
192-
kind?: 'expansion' | 'check';
204+
/**
205+
* A user-scoped holder check words its refusal as a check, and a single
206+
* head or manager lookup as a lookup; the default is an expansion.
207+
*/
208+
kind?: HolderReadKind;
193209
},
194210
): Promise<Record<string, unknown>[] | null> {
195211
const rows: Record<string, unknown>[] = [];
@@ -374,24 +390,30 @@ export class TeamGraphService implements ITeamGraphService {
374390
*
375391
* Resolution order matches `expandRoleUsers` exactly — the argument, else the
376392
* instance's `organizationId`, else no screen at all.
393+
*
394+
* [#29792] The `sys_user` read goes through {@link readMembershipToCompletion}
395+
* as a `lookup`, the read `BusinessUnitGraphService`'s standalone fallback
396+
* uses: a read that FAILS throws and nothing is cached for it, so the next
397+
* caller on the same pass reads again. It caught any error to `null` ("no
398+
* manager") and cached that `null` under `org::userId` for the pass, so a
399+
* consumer naming "the owner's manager" dropped the manager on an outage and
400+
* kept dropping it after the read recovered. A stack that never materialises
401+
* `sys_user` has no manager on file and answers `null`. The organization
402+
* screen keeps its ruled fail-open posture ({@link managerIsProvablyOutsideOrg}).
377403
*/
378404
async managerOf(userId: string, organizationId?: string): Promise<string | null> {
379405
if (!userId) return null;
380406
const org = organizationId ?? this.organizationId;
381407
const key = `${org ?? '*'}::${userId}`;
382408
if (this.cache.manager!.has(key)) return this.cache.manager!.get(key) ?? null;
383-
let row: any = null;
384-
try {
385-
const rows = await this.engine.find('sys_user', {
386-
where: { id: userId },
387-
fields: ['id', 'manager_id'],
388-
limit: 1,
389-
context: SYSTEM_CTX,
390-
});
391-
row = Array.isArray(rows) ? rows[0] : null;
392-
} catch {
393-
row = null;
394-
}
409+
const rows = await readMembershipToCompletion(this.engine, {
410+
object: 'sys_user',
411+
where: { id: userId },
412+
fields: ['manager_id'],
413+
subject: `the manager of user '${userId}'`,
414+
kind: 'lookup',
415+
});
416+
const row = rows?.[0];
395417
let mgr = row?.manager_id ? String(row.manager_id) : null;
396418
if (mgr && (await managerIsProvablyOutsideOrg(this.engine, mgr, org))) mgr = null;
397419
this.cache.manager!.set(key, mgr);

‎packages/spec/src/contracts/sharing-service.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -664,7 +664,10 @@ export interface ITeamGraphService {
664664
expandUsers(teamId: string): Promise<string[]>;
665665
/** Return all user ids that hold a role of `roleName` in `organizationId`. */
666666
expandRoleUsers(roleName: string, organizationId?: string): Promise<string[]>;
667-
/** Return the manager id for a user (best-effort; null when none). */
667+
/**
668+
* Return the manager id for a user: refuses (throws) on a failed read;
669+
* `null` when none.
670+
*/
668671
managerOf(userId: string, organizationId?: string): Promise<string | null>;
669672
}
670673

@@ -683,7 +686,10 @@ export interface IBusinessUnitGraphService {
683686
descendants(businessUnitId: string): Promise<string[]>;
684687
/** Return all user ids in `businessUnitId` or any descendant business unit. */
685688
expandUsers(businessUnitId: string): Promise<string[]>;
686-
/** Return the department head (manager_user_id) — best-effort, null when none. */
689+
/**
690+
* Return the department head (manager_user_id): refuses (throws) on a failed
691+
* read; `null` when none.
692+
*/
687693
headOf(businessUnitId: string): Promise<string | null>;
688694
/** Return the manager id for a user — proxy to {@link ITeamGraphService.managerOf}. */
689695
managerOf(userId: string, organizationId?: string): Promise<string | null>;

0 commit comments

Comments
 (0)