Skip to content

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

Merged
objectstack-fleet[bot] merged 2 commits into
mainfrom
claude/project-thread-77mjum
Nov 3, 2026
Merged

objectstack-fleet[bot] merged 2 commits into
mainfrom
claude/project-thread-77mjum

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Requested by Warren · project thread

Fixes #29713

domain:server#1 · os-warren · session_01JQwxbXRtwbd8e9gz1XdRgn

Clause-②: no (narrowing)
Metadata-protocol: none
Skills-docs: not needed. No skill and no content/docs page names headOf or the standalone managerOf fallback, or says what either answers on a failed read.

维护者速读

What changes

BusinessUnitGraphService (packages/plugins/plugin-sharing/src/business-unit-graph.ts): the two lookups #29655 left now read through readMembershipToCompletion, the walk seedIsUsable and readChildUnits already use. No new helper.

  • headOf(businessUnitId). A failed sys_business_unit read throws [sharing] the head of business unit 'ID' could not be read: …. It used to answer null and cache that null for the pass. A failed read now throws before any set, so the next call reads again. A unit that does not exist, sits in another organization, or names no head still answers null and is cached as before. A missing sys_business_unit (isObjectAbsentError) answers null.
  • managerOf(userId, organizationId), standalone fallback (no teamGraph). A failed sys_user read throws [sharing] the manager of user 'ID' could not be read: …. It used to answer null. This fallback caches nothing, before or after. A missing sys_user answers null. The organization screen (managerIsProvablyOutsideOrg) keeps its ruled fail-open posture (approvals: the manager approver resolves sys_user.manager_id with no organization screen, while every sibling approver expansion is org-scoped #10153) and is unchanged.
  • The wired limb is unchanged. With a teamGraph, managerOf still returns TeamGraphService.managerOf's answer as it stands.
  • The module-level SYSTEM_CTX constant had no reader left and is removed. The walk reads under team-graph's own system context, as the other reads of this file already did.
  • No packages/spec change, no protocol.ts, no new export.

Measured first (real engine, before and after)

The same scratch probe ran on main (87956dcc) and on the fix. On each, one read failed, then the same instance was called again.

Reading main this PR
headOf on a failed read null throws the head of business unit 'bu_sales' could not be read: SQLITE_IOERR…
headOf, same instance, after the read recovers null (the failed answer was cached) usr_head
headOf, sys_business_unit not registered null null
standalone managerOf on a failed read null throws the manager of user 'usr_alice' could not be read: SQLITE_IOERR…
standalone managerOf, same instance, after recovery usr_bob (nothing cached) usr_bob
standalone managerOf, sys_user not registered null null

The card said both lookups cache the failed answer. Only headOf does. The standalone managerOf fallback has no cache, so its fix is the refusal alone.

Callers on main (traced)

Caller Calls What it sees on a refusal
Sharing rules (SharingRuleService.expandRecipient) expandUnitMembers / expandUsers only nothing changes: neither lookup is called
Ownership queues (ownership-queue-service.ts) expandUnitMembers / expandUsers only nothing changes
Approvals (ApprovalService.resolveApproverSpec) its own lookupManager and expandBusinessUnitUsers; it does not construct BusinessUnitGraphService nothing changes
expandPrincipal manager arm (team-graph.ts) TeamGraphService.managerOf, not this class nothing changes
BusinessUnitGraphService.managerOf wired limb TeamGraphService.managerOf unchanged (control pin)

headOf and BusinessUnitGraphService.managerOf have no production caller in this repository, hotcrm or docs.objectos.ai. The only reach is the exported class (@objectstack/plugin-sharing index) and the IBusinessUnitGraphService contract. No caller fails a whole pass where it used to drop one person.

Tests

New file src/business-unit-head-manager-reads.test.ts runs on a real engine: ObjectQL + SqlDriver over in-memory SQLite, with the shipped SysOrganization, SysBusinessUnit, SysUser and SysMember. 11 tests:

Pin headOf standalone managerOf
control: healthy read the head; a unit without one answers null the manager; a user without one answers null
⛔ failed read refuses throws, names the unit throws, names the user
⛔ not cached: after recovery the same instance answers usr_head the same instance answers usr_bob
absence: object never registered null null
absence: table never created null null

CONTROL: with a TeamGraphService wired, managerOf answers exactly what TeamGraphService.managerOf answers, on a healthy and on a failed read.

  • pnpm --filter @objectstack/plugin-sharing test at 39b2f260: 65 files / 1380 tests passed.
  • pnpm --filter @objectstack/plugin-sharing typecheck: exit 0.
  • Existing headOf / managerOf pins (business-unit-graph.test.ts, sharing-rule.test.ts, manager-org-screen.test.ts) pass unchanged.
Ablations: each negative pin's behaviour put back (red), restored by ablation-replace (blob == HEAD, git diff HEAD empty)

The test imports the subject from src/ by relative path, so no build sits between the mutation and the run.

A1, headOf refusal: .catch(() => null) put back on the read

ablation-replace: anchor   x1 -> x0
ablation-replace: blob     ffdef39d42df0cde33818a9700573b679ab2ff70 -> 2473fb0bc967e5519ae69a6f02baada817d44996
     × ⛔ a failed read refuses — it never answers an invented null 165ms
     × ⛔ a failed read is not cached: the next read on the same pass answers the real head 26ms
      Tests  2 failed | 9 passed (11)
ablation-replace: ok restored: blob == HEAD (ffdef39d42df) and `git diff HEAD` is empty

A2, headOf cache: a null cached for the unit before the read

ablation-replace: anchor   x1 -> x0
ablation-replace: blob     ffdef39d42df0cde33818a9700573b679ab2ff70 -> 6b89d6dbd3360cc0344a7c61ebca39e47b3d60f2
     × ⛔ a failed read is not cached: the next read on the same pass answers the real head 34ms
      Tests  1 failed | 10 passed (11)
ablation-replace: ok restored: blob == HEAD (ffdef39d42df) and `git diff HEAD` is empty

A3, standalone managerOf refusal: .catch(() => null) put back on the read

ablation-replace: anchor   x1 -> x0
ablation-replace: blob     ffdef39d42df0cde33818a9700573b679ab2ff70 -> e02db2c72bafbc2ddeb6084d6b888b5e0790f440
     × ⛔ a failed read refuses — it never answers an invented null 24ms
     × ⛔ a failed read is not cached: the next read on the same pass answers the real manager 20ms
      Tests  2 failed | 9 passed (11)
ablation-replace: ok restored: blob == HEAD (ffdef39d42df) and `git diff HEAD` is empty

The standalone managerOf has no cache, so this diff has no cache line to put back. Its not-cached pin guards against a cache being added, and A3 turns it red at its first leg.

Acceptance notes

  • TeamGraphService.managerOf (team-graph.ts) also catches a failed sys_user read to null and caches it for the pass. It belongs to the same family, but the order keeps the wired limb unchanged, and its one in-repo consumer, expandPrincipal's manager arm, is exported with no in-repo caller. Noted, not filed.
  • Both refusals reuse readMembershipToCompletion's sentence family. Its closing clause speaks of a holder set, which reads loosely for a single head or manager. Reusing the helper was the order. A wording change belongs to that helper, not here.

Verification

All local readings at f357cc8f (the fix 39b2f260 plus a merge of main at c76c0e04).

  • node scripts/pm/dispatch-gates.mjs --commands: 60 derived families, all run. --ran reconciliation exit 0: "60 derived, 60 run, 0 NOT-MEASURED, 0 UNRUN". Two 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 before seven unrelated packages had a dist/. Both were re-run and passed after git fetch --unshallow origin main and a build of those packages.
  • pnpm lint: exit 0.
  • Dogfood battery (the claim carries a (narrowing) arm): f357cc8f · os-verify-lock: VERDICT command-exit 0 · held the lock 2046s · Test Files 402 passed, 3 skipped (405) · Tests 3317 passed, 28 skipped (3345) · no failures.
  • Every family CI runs is cited from this PR's check-runs at landing, not re-run here.

Generated by Claude Code

…er read refuses instead of answering nobody

BusinessUnitGraphService.headOf read sys_business_unit, caught any error
to null and cached that null for the pass. The standalone managerOf
fallback (no TeamGraphService wired) caught a failed sys_user read to
null. Both now read through readMembershipToCompletion, the walk
seedIsUsable already uses: a failed read throws, absence of the object
answers null, and nothing is cached after a failure. The wired
TeamGraphService limb is unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JQwxbXRtwbd8e9gz1XdRgn
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-sharing, touching 7 documentable anchor(s).

11 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/automation/approvals.mdx (via manager_id (literal, a string literal in managerOf), sys_business_unit (literal, a string literal in headOf))
  • content/docs/capabilities/approvals.mdx (via manager_id (literal, a string literal in managerOf))
  • content/docs/permissions/administrator-guide.mdx (via sys_business_unit (literal, a string literal in headOf))
  • content/docs/permissions/authorization.mdx (via sys_business_unit (literal, a string literal in headOf))
  • content/docs/permissions/delegated-administration.mdx (via sys_business_unit (literal, a string literal in headOf))
  • content/docs/permissions/permissions-matrix.mdx (via sys_business_unit (literal, a string literal in headOf))
  • content/docs/permissions/positions.mdx (via sys_business_unit (literal, a string literal in headOf))
  • content/docs/permissions/sharing-rules.mdx (via sys_business_unit (literal, a string literal in headOf))
  • content/docs/permissions/tenant-audit-census.mdx (via SYSTEM_CTX (symbol, a top-level const object))
  • content/docs/protocol/objectql/query-syntax.mdx (via manager_id (literal, a string literal in managerOf))
  • content/docs/protocol/objectql/security.mdx (via sys_business_unit (literal, a string literal in headOf))

⛔ 5 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v13.mdx (via sys_business_unit (literal, a string literal in headOf))
  • content/docs/releases/v16.mdx (via sys_business_unit (literal, a string literal in headOf))
  • content/docs/releases/v17/17-0.mdx (via SYSTEM_CTX (symbol, a top-level const object))
  • content/docs/releases/v17/17-4.mdx (via sys_business_unit (literal, a string literal in headOf))
  • content/docs/releases/v17/17-5.mdx (via manager_id (literal, a string literal in managerOf))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: sys_user (literal, 48 pages)
  • the SDK route bridge reached 61 of 218 client-bound route-ledger rows — the other 157 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 157: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 11 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 946a173fcbe44a7d6961656a47156322a6ae4b1e → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5e9a0461ad39c9e3d82aacd1d13c46e2f0622af5 — the merge of head f357cc8f8fe25e9f7fc24e622331692b34c005c7 into base 946a173fcbe44a7d6961656a47156322a6ae4b1e, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 5e9a0461ad39c9e3d82aacd1d13c46e2f0622af5 && git checkout 5e9a0461ad39c9e3d82aacd1d13c46e2f0622af5
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 946a173fcbe44a7d6961656a47156322a6ae4b1e f357cc8f8fe25e9f7fc24e622331692b34c005c7 && git checkout -B drift-repro 946a173fcbe44a7d6961656a47156322a6ae4b1e && git merge --no-ff f357cc8f8fe25e9f7fc24e622331692b34c005c7

node scripts/docs-audit/affected-docs.mjs --json 946a173fcbe44a7d6961656a47156322a6ae4b1e

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 946a173fcbe44a7d6961656a47156322a6ae4b1e → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#1 · os-warren · session_01JQwxbXsK9j25sd7eVwW96G

Contract-review-claim: session_01JQwxbXsK9j25sd7eVwW96G · head f357cc8 · 2026-11-03T04:47Z

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#1 · os-warren · session_01JQwxbXsK9j25sd7eVwW96G

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: f357cc8f8fe25e9f7fc24e622331692b34c005c7
Local-runs: none
Scope: packages/plugins/plugin-sharing/src/business-unit-graph.ts, packages/plugins/plugin-sharing/src/business-unit-head-manager-reads.test.ts + packages/plugins/plugin-sharing/src/team-graph.ts (readMembershipToCompletion, holderReadRefusal, managerIsProvablyOutsideOrg, TeamGraphService.managerOf, expandPrincipal), packages/types/src/keyset-walk.ts, packages/types/src/object-absence.ts, packages/spec/src/contracts/sharing-service.ts (IBusinessUnitGraphService, ITeamGraphService), packages/plugins/plugin-sharing/src/index.ts, packages/plugins/plugin-sharing/src/sharing-rule-service.ts, packages/plugins/plugin-sharing/src/ownership-queue-service.ts, packages/plugins/plugin-approvals/src/approval-service.ts (lookupManager, expandBusinessUnitUsers), the existing pins business-unit-graph.test.ts / manager-org-screen.test.ts / sharing-rule.test.ts, and the hotcrm and docs.objectos.ai trees (caller trace). The changeset is a prose face and is read for ②, not reviewed.

Face: Clause-②: no (narrowing) on the PR body and on the changeset; reviewFace on the three paths with that declaration answers the narrowing face (no schema or governed hits). Net diff against the merge base c76c0e04: exactly the three files above; team-graph.ts and packages/spec are untouched. Predecessor PR #29703 (48981622) is an ancestor of this head, so the serial constraint holds.

① Derived judgments

  1. BusinessUnitGraphService.headOf: a failed sys_business_unit read now throws [sharing] the head of business unit 'ID' could not be read: REASON. … instead of answering null. Judged RIGHT. In the tree the read goes through readMembershipToCompletion, which rethrows every non-absence error via holderReadRefusal with the driver error as cause; the catch { row = null } is gone.
  2. headOf caches nothing after a failed read. Judged RIGHT. this.cache.head!.set(businessUnitId, head) is the first statement after the awaited read, so a throw leaves the map untouched; the cache key (the unit id), the has() short-circuit and the cached value (string | null, set only after a completed read) are byte-identical to main and the same shape as fix(plugins)!: a failed business-unit hierarchy read refuses instead of answering nobody, and a unit keeps every child unit past 1,000 #29703's descendants cache (key = unit id, set only after seedIsUsable and the child walk complete).
  3. headOf on absence: readMembershipToCompletion answers null on isObjectAbsentError (missing table, or OBJECT_NOT_FOUND naming the read object), rows?.[0] is undefined, head is null and is cached — the same answer and the same caching main gave absence through its catch. Judged RIGHT. A unit that does not exist, sits in another organization (strict orgScope) or has no manager_user_id still answers null; the three existing headOf pins in business-unit-graph.test.ts and sharing-rule.test.ts that cover those cases are unchanged.
  4. headOf projection and read count: fields: ['manager_user_id'] becomes ['id', 'manager_user_id'] inside the helper, the projection main had. keysetWalk returns on a short page (an emitted page smaller than the page it asked for), so a one-row answer is still one read. Judged RIGHT; no accept-set change.
  5. Standalone BusinessUnitGraphService.managerOf (no teamGraph): a failed sys_user read throws [sharing] the manager of user 'ID' could not be read: …; absence answers null; a user with no manager_id answers null. Judged RIGHT. This limb has no cache on main or here, so the card's "and caches it" premise holds for headOf only — the dev's correction is confirmed from the main source.
  6. The organization screen on the standalone limb is unchanged: managerIsProvablyOutsideOrg keeps its own try/catch that fails open on a failed sys_member read (ruled, approvals: the manager approver resolves sys_user.manager_id with no organization screen, while every sibling approver expansion is org-scoped #10153), so dropping the outer try around it changes nothing on that read. Judged RIGHT.
  7. The wired limb if (this.teamGraph) return this.teamGraph.managerOf(userId, organizationId) is unchanged, and team-graph.ts is not in the diff. Judged RIGHT.
  8. Module constant SYSTEM_CTX removed from business-unit-graph.ts: not exported, no reader left (the helper reads under team-graph.ts's own SYSTEM_CTX, as seedIsUsable, readChildUnits and readMembers already did). Judged RIGHT; no public-surface change. The memberScope doc comment's {@link SYSTEM_CTX} becomes prose, so no dangling link.
  9. Public surface: IBusinessUnitGraphService (packages/spec) is unchanged, index.ts exports are unchanged, no new export. The only observable narrowing is the failure posture of two methods on the exported class. Judged RIGHT.
  10. Callers (traced in this tree, in hotcrm and in docs.objectos.ai): headOf has no production caller anywhere (tests only; hotcrm's managerOf hit is a local test variable). BusinessUnitGraphService.managerOf has no production caller: sharing-rule-service.ts constructs the class and calls expandUnitMembers / expandUsers only; ownership-queue-service.ts likewise; approval-service.ts uses its own lookupManager and expandBusinessUnitUsers and never constructs the class; expandPrincipal's manager arm calls TeamGraphService.managerOf, not this class. So no pass on main can newly fail where it used to drop one person. Judged RIGHT, and this is what makes the narrowing safe to ship with no caller change.

② Semver level

.changeset/29713-business-unit-head-manager-reads-refuse.md: @objectstack/plugin-sharing: patch, fix(plugin-sharing)!: title, Clause-②: no (narrowing), Metadata-protocol: none, ADR-0087 not-required (no-migration-prescription). Correct under ruling A (check-changeset-no-major.mjs): a non-protocol narrowing with none ships patch and keeps its BREAKING banner; both predecessors (#29606 e0187d0f, #29655 48981622) shipped the same shape at patch. The changeset's claims match the diff: it names both methods, says absence still answers null, says the wired limb is unchanged, and says no caller on this release changes behaviour (confirmed in ①.10). Clause-②: no (narrowing) is the right line: no accept set widens; a failure answer narrows from null to a refusal.

Check-runs on f357cc8f at the time of this record (read four times through the commits API, the last read governs): 23 completed success (Build Core, Check Changeset, Check PR Size, Governed Surface Queue Guard, PR-body guards, Lint & Repo Gates, Lint · PM dispatch-gates, Lint · ESLint, Lint · docs & code guards, Lint · repo & tooling gates, Lint · CI, release & engine gates, Lint · scoped · tooling self-tests, Lint · scoped · slot lookup & corpus, Lint · scoped · query-options erasure, Type Check · source gates, Type Check · debt ledger, Dogfood Regression Gate, Test UI, Live E2E UI, Plan Test Core shards, filter, Auto Label, Flag docs affected by code changes), 13 skipped by path filter, and 10 still in progress: Test Core shards 1 to 8, Type Check · workspace, Type Check · consumer gates. No run is red. Those ten conclusions are the gate verdicts the landing check reads; this record judges the tree and does not pre-empt them.

③ Boundary flags

  • open_questions: none declared. Nothing to answer.
  • Author: PR user.login is objectstack-fleet[bot]; attribution comment names os-warren. No login note owed.
  • Skills-docs not needed: confirmed. No content/docs/** page and no skills/** file names headOf, managerOf, manager_user_id or the standalone fallback, or states what either answers on a failed read.
  • Premise correction (dev): "only headOf caches". Confirmed from main: DeptCache has a head map and no manager map; the standalone limb never wrote a cache. The card's wanted shape ("nothing is cached after a failed read") is still met for both, and the managerOf not-cached pin is a regression guard, as the dev says.
  • Out-of-scope finding 1 (dev, not filed): TeamGraphService.managerOf (team-graph.ts) catches a failed sys_user read to null and caches it under org::userId for the pass. Confirmed in the tree. Judged a FOLLOW-UP CARD, not this PR: the order and the claim's file surface kept the wired limb and team-graph.ts unchanged; it is a different class with its own organization-qualified cache and its own pins (manager-org-screen.test.ts); and its reach today is the exported expandPrincipal manager arm with no in-repo caller, so no grant on main is computed from it either. Same family, same grade (p3). ESCALATED here for the seat to file; see ④ S2.
  • Out-of-scope finding 2 (dev): the refusal sentence's consequence clause speaks of a "holder set" and an "expansion" for a single head or manager. Confirmed; belongs to holderReadRefusal in team-graph.ts, which has two arms (expansion, check) and neither fits a single lookup. Should-fix in the follow-up (④ S4), not a reason to hold this PR.
  • exceptions (PR body folds the template; model-free co-author; pnpm lint run by order): none touches a review face.

④ Design judgment

The narrowing read against the published spec on main (packages/spec/src/contracts/sharing-service.ts): IBusinessUnitGraphService.headOf is declared "Return the department head (manager_user_id) — best-effort, null when none", and managerOf "proxy to ITeamGraphService.managerOf", itself "best-effort; null when none". The declared capability is "the head, or null when the unit has none"; the PR keeps that at every consumer (no head, unknown unit, foreign-org unit, absent object all still answer null). What it takes away is an answer the spec never declared: null for a read that failed. "Best-effort" is prose on the lookup, not a declared accept set, and expandUsers / descendants on the same interface were given the same refusal by #29606 and #29655 without a spec change. So the narrowing does not remove a declared capability at the consumer; it makes the service consistent with its own sibling reads and with the spec's "null when none". The design holds.

Tried to break it:

  • Cache after a failed read: the set sits after the awaited helper; the helper rethrows; nothing is written. The not-cached pin fails one read then expects the real head on the same instance, and the dev's A1/A2 ablations (catch-to-null put back; null pre-cached) each turn it red. Holds.
  • Absence masking a real failure: isObjectAbsentError requires a missing-table error or OBJECT_NOT_FOUND naming the read object itself; a SQLITE_IOERR is neither, and the pins run on a real SqlDriver with the table dropped versus the object never registered, so "absent" is what the driver says, not a double's guess. Holds.
  • Pagination against where: { id }: keysetWalk ANDs the cursor with $and rather than overwriting the id equality, and returns on the short first page, so a unique-id lookup is one read with the same projection. Holds (and is the shape seedIsUsable already runs on main).
  • The standalone limb's screen: the fail-open sys_member read keeps its own catch inside managerIsProvablyOutsideOrg; the removed outer try only ever covered the sys_user read and trivial string coercion. The screen's six existing pins in manager-org-screen.test.ts are unchanged. Holds.
  • Pins cover each method's three cases: for headOf — refuses (named unit, SQLITE_IOERR), absence (object not registered; table dropped), not cached (fail once then real head on the same instance); for standalone managerOf — the same three with the user named; plus healthy controls (head / no head; manager / no manager) and a wired-limb control. Each negative pin was ablated red once (A1 to A3). Covered.
  • Whole-pass failure: no production caller exists for either method (①.10), so no reconciler, approval or queue pass can newly throw where it used to drop one person. Holds.

Findings (none is a must-fix):

  • S1 should-fix (spec prose, follow-up): the JSDoc "best-effort" on IBusinessUnitGraphService.headOf and ITeamGraphService.managerOf now describes one implementation that refuses and one that swallows. Align the two sentences to "refuses on a failed read; null when none" when the sibling is fixed — a packages/spec prose change that belongs with S2, not in this PR.
  • S2 should-fix (follow-up card, p3): TeamGraphService.managerOf swallows a failed sys_user read to null and caches it for the pass; same class as this card and plugins: a failed sys_business_unit hierarchy read still answers nobody (and sharing caches it for the pass), so a business-unit graph write can revoke a rule's grants on an outage #29655. Reach today: expandPrincipal manager arm (exported, no in-repo caller) and the wired BusinessUnitGraphService.managerOf limb. File it; it is the last read of this family in plugin-sharing.
  • S3 should-fix (test, fold into S2): the CONTROL test hard-codes expect(await failed.managerOf(ALICE, ORG)).toBeNull() for the wired limb on a failed read. That line pins the sibling's swallow rather than the pass-through; the equality assertion beside it (toBe(await new TeamGraphService(...).managerOf(...))) is the control. The S2 fix will have to delete that one toBeNull(); nothing to change here.
  • S4 should-fix (wording, fold into S2): holderReadRefusal's consequence clause for a single head or manager lookup. A third arm (lookup) in the helper, not a local sentence.

Implemented-by: claude/project-thread-77mjum
Reviewed-by: session_01JQwxbXsK9j25sd7eVwW96G

VERDICT: PASS

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#1 · os-warren · session_01JQwxbXsK9j25sd7eVwW96G

席位核对

ACCEPT · domain:server#1 · head f357cc8f8fe25e9f7fc24e622331692b34c005c7 · 2026-11-03T05:09Z · contract review PASS 6595814797 at CONTRACT_REVIEW_TIER (narrowing face, whole diff outside prose, fable subagent, independent of the dev); changeset read by the seat at TIER_DEFAULT

  1. Shape: draft, base main, Fixes #29713. 3 files, +270/-32: business-unit-graph.ts (headOf and the standalone managerOf fallback now read through readMembershipToCompletion), a new pin file business-unit-head-manager-reads.test.ts, and one changeset (@objectstack/plugin-sharing patch, Clause-②: no (narrowing), Metadata-protocol: none, using the same lines as fix(plugins)!: a failed business-unit hierarchy read refuses instead of answering nobody, and a unit keeps every child unit past 1,000 #29703). Not governed, under 3000 lines.
  2. Premise measured on a real engine at 87956dcc (report 6595695677): headOf answered null on a failed read and cached it. The standalone managerOf answered null but cached nothing, a premise correction the review confirmed. Ablations A1–A3 went red, then were restored. Dogfood (narrowing arm) gave VERDICT 0, with 402 files passing.
  3. Gates on this head: check-runs pinned to f357cc8f: 48 total, 35 success, 13 skipped, 0 failed. Dev local: dispatch-gates --ran 60/60 exit 0, pnpm lint 0, plugin-sharing 1380/1380, typecheck 0.
  4. Accepted deviations: none. Caller trace (dev and review, objectstack + hotcrm + docs): headOf and BusinessUnitGraphService.managerOf have no production caller, so no pass newly fails where it dropped one person.
  5. Out of scope: TeamGraphService.managerOf still swallows and caches a failed sys_user read. Filed plugin-sharing: TeamGraphService.managerOf still answers null on a failed sys_user read and caches it for the pass #29792 (p3 suggested) together with the review's S1 (spec JSDoc "best-effort"), S3 (the CONTROL pin's toBeNull()) and S4 (holderReadRefusal lookup arm).
  6. Landing: not governed and under 3000 lines ⇒ the seat lands it (pr_ready + automerge_enable). At MERGED: landing record on plugin-sharing: a failed business-unit head or standalone manager read still answers null and caches it for the pass (headOf, managerOf fallback) #29713 (closed by Fixes), pm:dispatched removed; plugin-sharing: TeamGraphService.managerOf still answers null on a failed sys_user read and caches it for the pass #29792 is serial after this PR (same family, test file).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

plugin-sharing: a failed business-unit head or standalone manager read still answers null and caches it for the pass (headOf, managerOf fallback)

1 participant