fix(sharing): honour the declared organizationId in managerOf (#10231)

[os-sam]

Fixes#10231

ITeamGraphService.managerOf declares an organization parameter — in prose, it
takes a user id plus an optional organization id and returns a promise of
string-or-null. TeamGraphService.managerOf spelled that parameter
_organizationId and discarded it, while expandRoleUsers on the same class
applied organization_id to its own read. The BusinessUnitGraphService
standalone fallback read sys_user the same unscreened way.

Both limbs now honour it.

The card's premise moved, and the correction matters

Triage scoped this as IHierarchyScopeResolver.managerOf being a
declared-but-unenforced parameter on that seam. Measured on main, that
attribution is wrong in a way worth recording:

• IHierarchyScopeResolver does not declare managerOf at all. Its only
  member is resolveOwnerIds (packages/spec/src/contracts/sharing-service.ts).
  managerOf is declared on ITeamGraphService and IBusinessUnitGraphService.
• The open edition ships no implementation of that seam — zero implementors
  in-repo, and sharing-service.ts says "Returns null in the open edition". So
  there was no OSS implementation of it discarding anything.
• managerOf is never reached from the hierarchy-scope path; its only in-repo
  callers are expandPrincipal and the BusinessUnitGraphService proxy.

That matters because the fork clause was conditioned on the open-tier seam's
"fail-closed to own" posture intending the parameter as advisory. That posture
belongs to a different interface with a different method, so it never governed
this parameter — the fork's antecedent is false rather than unresolved, which
is why this landed as a patch instead of a report. #7497 remains open and is
untouched by this change.

Why the screen reads sys_member

sys_user carries noorganization_id. It is the global better-auth
identity table (managedBy: 'better-auth', protection.lock: 'full',
ADR-0010 section 3.7), so a membership row is the only tenancy fact that exists
for a user. Verified by grep, counter-checked against sys_member,
sys_team and sys_business_unit, which all do carry the column.

So the sibling asymmetry the card leans on is structural, not a posture:
every place the org filter is applied backs onto a table that has the column;
the one place it is not is the table that has none.

Adding organization_id to the sys_user predicate is therefore not a
stricter version of this screen — it is a broken one. The column does not
exist, so the predicate matches nothing and every manager lookup given an
organization returns null, silently, because the surrounding catch swallows
the driver's complaint. That is the over-screening defect, and on this seam it
is as severe as under-screening: it would empty every approver slate and every
manager sharing recipient while looking like "no manager on file".

The shape is the one #10153 already landed

13f533a43a screened the identical column (sys_user.manager_id) on the
approvals side. This PR mirrors managerIsProvablyOutsideOrg exactly:

• membership rows exist for the manager, none in the caller's org — screen out;
• no membership rows at all, or the membership read failed — leave the answer
  exactly as it was (fail open on an absent tenancy fact);
• no organization in play — no screen and no query at all.

One implementation, exported from team-graph.ts and imported by
business-unit-graph.ts rather than restated, so the two limbs cannot drift.
It is deliberately not re-exported from the package index.

Two deliberate deviations from the approvals version, both narrowing:

• No log line.TeamGraphService has no logger seam, and adding one would
  enrol this package's modules into check:optional-error-sink, whose ledger is
  being paid down shrink-only. That is a contract decision for that family, not
  a side effect here.
• The manager cache key is now organization-qualified (org::userId),
  mirroring the composite key expandRole already used. A user-keyed cache
  would have served one screened null to every unscoped reader behind it —
  turning a screen into a permanent outage. Pinned by two tests.

File surface

The claim comment listed team-graph.ts only, but the BusinessUnitGraphService
standalone fallback — explicitly in triage's scope — lives in
business-unit-graph.ts. Both files are edited. That is the sole divergence
from the dispatched file list, and no other file is touched: no packages/spec,
no docs/adr, no content/docs/releases, no .claude.

Verification

20 new tests, and the positive half is the longer one on purpose.

Reverse verification: reverting only the two source files (trap-restored) gives
7 failed / 13 passed — the split predicted before running. The screen, cache
and BU-fallback tests go red; the positive and absent-organization tests stay
green, which is the point — those are what would catch over-screening. The
ablation is source-resolved (the suite imports ./team-graph.js relatively, so
no dist is involved) and the mutation was confirmed on disk by anchored grep
counts in both directions before each leg.

Gate union derived with node scripts/pm/dispatch-gates.mjs — no hand-supplied
paths — on the final commit a620ecdf80 with a clean tree. All 19 to a real
verdict:

• 12 path-derived: check:changeset-gate-self-tests, check:objectui-changeset,
  check:published-files, check:slot-lookup, check:test-source-alias,
  check:type-source-resolution, check-adr-0087-registration.mjs,
  check-changeset-no-major.mjs, check-ci-filter-parity.mjs,
  check-empty-changeset.mjs, check-plugin-teardown-shape.mjs,
  docs-audit/check-affected-docs.mjs
• 7 convention-triggered: check:query-options-erasure,
  check:type-check-coverage, check:type-check-debt,
  check:engine-double-contract, check:cross-package-test-inputs,
  check:where-matcher, check:i18n

check:i18n first returned PREREQUISITE NOT MET — the workspace CLI is not built / "Nothing was checked". That is NOT MEASURED, not a pass, so the CLI was
built and it was re-run to a real verdict: check-i18n-bundles: OK (9 package(s) — all bundles in sync). check:type-check-debt likewise needs the built
closure; after building it: --re-measure: OK — 33 ledger entr(ies) re-measured, 1897 raw tsc error(s) total, none above its recorded number.

The derivation also caught two conventions the first draft of the test double
tripped — it declared unused insert/update/delete members and its WHERE
matcher walked Object.entries unconditionally. Both fixed at the source rather
than by moving a baseline: the double is now read-only (these paths call find
and nothing else) and the matcher refuses combinator keys and operator
objects instead of reading them as field names.

• pnpm --filter @objectstack/plugin-sharing test — 26 files, 644 passed
• pnpm --filter @objectstack/plugin-sharing typecheck — tsc --noEmit, clean

Downstream sweep, narrowed and declared. Rather than run all 27 dependents,
the blast radius was measured: TeamGraphService and BusinessUnitGraphService
are constructed at four sites, all inside plugin-sharing itself
(position-graph.ts, sharing-rule-service.ts), and the package index exports
no new symbol — so no consumer's behaviour can change except through this
package. Confirmed on the two highest-value consumers anyway:
plugin-approvals (30 files, 565 passed — the sibling consumer of this exact
column) and plugin-security (72 files, 1377 passed).

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

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

28 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json e353c9b50f830e2a6a533b4e45ad1faa5b6a4aab.

⛔ 4 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see

• 1 name(s) were too generic to anchor anything (single lowercase words)
• the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 8 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 e353c9b50f830e2a6a533b4e45ad1faa5b6a4aab → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 3b7f51dc91047bb22eaf47d78c95578a8b4b88ba — the merge of head a620ecdf806788d0c390d404c40470f3bb559e6e into base e353c9b50f830e2a6a533b4e45ad1faa5b6a4aab, 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 3b7f51dc91047bb22eaf47d78c95578a8b4b88ba && git checkout 3b7f51dc91047bb22eaf47d78c95578a8b4b88ba
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e353c9b50f830e2a6a533b4e45ad1faa5b6a4aab a620ecdf806788d0c390d404c40470f3bb559e6e && git checkout -B drift-repro e353c9b50f830e2a6a533b4e45ad1faa5b6a4aab && git merge --no-ff a620ecdf806788d0c390d404c40470f3bb559e6e
node scripts/docs-audit/affected-docs.mjs --json e353c9b50f830e2a6a533b4e45ad1faa5b6a4aab

⚠️ 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 e353c9b50f830e2a6a533b4e45ad1faa5b6a4aab → pass the list as
args.docs, on the commit named under Which tree this was computed on.

[os-sam]

PM: CI red on Test Core (2/6) root-caused — it is not this diff, and the test body never ran. Recorded by the dispatching seat, session_01APWX2AwT3a4xDcjPCe8bk4 (os-sam), so the work is not repeated.

Job 97169332513, head 912ac6c965, run 32629237476.

The signature

! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-10.31.0.tgz
node:internal/assert/utils:77
AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
assert(!this.paused)
at Parser.finish (node:internal/deps/undici/undici:6165:9)
at TLSSocket.<anonymous> (node:internal/deps/undici/undici:6499:36)

It died in the pnpm --version step, while corepack was downloading pnpm from the npm registry — a Node/undici assertion on a TLS socket during that fetch. Node v22.23.2.

Why this is not attributable to the diff

Two independent readings, neither of them "it looks flaky":

1. The test body never executed. The job's own next step says so verbatim: "No test log — the test step did not get far enough to produce one." No artifacts, no .turbo/runs/. The failure is upstream of anything this PR could influence.
2. It names a service the diff does not touch — the npm registry, reached by the toolchain bootstrap. This PR changes four files in plugin-sharing (team-graph.ts, business-unit-graph.ts, a new test, a changeset) and cannot reach corepack's download path.

⛔ For the record, this is not me calling it a flake and moving on. "Flake" is not a root cause. The sanctioned category here is narrower and this failure sits squarely inside it: a job that died before any test body ran (checkout / install / runner loss) may be re-run once, in total. That is the whole of the allowance and I am not stretching it.

What I did, and what happens next

rerun_failed_jobs on run 32629237476 was refused: 403 This workflow is already running — sibling jobs in the same run are still in flight, so the run cannot be re-triggered yet. The re-run is therefore pending run completion, not skipped.

Branch head is unchanged at 912ac6c965, so this red stands against the current head rather than being superseded by a push.

⛔ If the re-run reproduces the same signature, that is a different conclusion — a second identical failure stops being a bootstrap blip and becomes a real signal, and it will not be re-run a third time. Equally, ⛔ if a different signature appears, it is a new problem and gets diagnosed from scratch, not re-thrown at the queue.

⚠️ Note for whoever reads this later: this PR is still draft and its terminal report has not landed. The red above is orthogonal to the review, which has not started.

---

Generated by Claude Code

[os-sam]

Correction to my previous comment — the head has moved, and the re-run allowance was never spent.

I wrote: "Branch head is unchanged at 912ac6c965, so this red stands against the current head rather than being superseded by a push."That is now false. It was true when written (08:49Z) and stopped being true minutes later.

The branch head is now a620ecdf80 — test(sharing): keep the #10231 engine double read-only and its matcher honest, pushed by the dev on top of the commit that went red. A fresh CI run is going against it.

Consequences, so nobody acts on the stale reading:

• ⛔ Do not evaluate this PR against 912ac6c965. The corepack/undici bootstrap failure I diagnosed belongs to a superseded head.
• ⭐ The one-in-total re-run allowance was NOT consumed. The 403 This workflow is already running refusal turned out to be a favour: a push supersedes the run, so the budget is intact for a genuine bootstrap failure later. The diagnosis in my previous comment stands on its own as a recorded signature (assert(!this.paused) in undici during corepack's pnpm download, dying before any test body runs) — ⛔ but it is not a spent re-run, and it is not evidence about a620ecdf80.
• The judgement that carries forward unchanged: if that same signature appears on the new head, it is a second occurrence and stops being a bootstrap blip.

⚠️ How I caught this is worth stating, because the signal was indirect: the docs-drift bot edited its existing comment in place and the new body named a different head SHA. No push event reached me. ⛔ An edited bot comment is a head-change signal on this repo — do not read "no new comment" as "no new commit".

---

Generated by Claude Code