Repository navigation
Commit 2789ba7
fix(plugins)!: sharing role and approvals membership-tier holders are read past 10,000 rows, and a failed membership read refuses instead of answering nobody (#29497)
domain:server#3 · os-steve · session_01ADHkmQJdgarUjPLNgFBY67
<!-- claude-projects-attribution: {"github_login":"os-steve"} -->
_Requested by **Steve** · [project
thread](https://claude.ai/code/project/chan_01FdL1XeUyvVNxLreh3F4Qi9?thread=cmsg_01FdL1XeUyvVNxLreh3F4Qi95XUAuk2vpvE7ZaE2sQ18jS)_
Fixes #29362
Clause-②: no (narrowing)
Metadata-protocol: none
Skills-docs: none — no skill or doc teaches a bound on role or
membership-tier holders, or what a failed membership read answers;
`content/docs/automation/approvals.mdx` documents the "expanded to
nobody" warning for an unstaffed slot, which stays true (a failed read
now gets its own warning).
## 维护者速读
- 改了什么:共享规则的「角色持有人」展开(plugin-sharing)和审批的「成员级别」展开(plugin-approvals,也是
position 审批人的第二来源)现在分页读完全部成员;超过 1 万人的角色/级别不再静默漏人,读失败时不再假装「没人」,而是明确报错。
- 为什么改:两处都只读一次、上限 1
万行,读失败还会变成空列表:共享规则会漏授权,读失败甚至会把规则已有的授权全部撤销;审批会把单子派给缩水的名单,看起来却像完整名单。
- 风险与回滚:唯一会「从有答案变成报错」的情形是 `sys_member` 读本身失败(原来答空列表),属收窄,标题带 `!`;1
万人以下仍是一次读、结果不变。回滚即 revert 本 PR。
-
先看:`packages/plugins/plugin-sharing/src/team-graph.ts`(`readRoleHoldersToCompletion`)、`packages/plugins/plugin-approvals/src/approval-service.ts`(`expandMembershipTierUsers`
和 `resolveApproverSpec` 的 catch/告警)、两个新测试文件。
- 两分钟验证:`pnpm --filter @objectstack/plugin-sharing exec vitest run
src/team-graph-role-holders-complete.test.ts`,预期 8/8 通过(含 10,001
人全部在集合内、读失败抛错)。
- 需要你做的:无,席位落地。
## What changes
Both expansions read `sys_member` once with a 10,000-row bound and took
that read as the whole holder list, and both turned any read error into
`[]`. Same class as #29281 (PR #29356).
- **plugin-sharing `TeamGraphService.expandRoleUsers`** (the card's
`expandRole`): the `sys_member.role` limb of every `position` sharing
recipient (`PositionGraphService.expandPositionUsers`), of
`expandPrincipal('role')`, and of queue members that resolve through a
position. It now reads through `readRoleHoldersToCompletion`.
- **plugin-approvals `expandMembershipTierUsers`** (the card's role-tier
recipients): the whole of an `org_membership_level` approver, and the
second limb of every `position` approver.
- **Completion.** Each read pages through the platform's existing keyset
walk, `keysetWalk` from `@objectstack/types`. That helper is already
public and already imported by both plugins. Pages are 10,000 rows,
ordered by `id`, with `fields: ['id', 'user_id']`. Under the bound this
is still one read with the same set. Past the bound the walk reads
further pages until a short one. The core helper
(`readOrganizationMembersToCompletion`) is neither exported nor copied,
so core's surface does not change; see Acceptance notes.
- **Loud failure.** A read that throws, or a walk that reports
`truncated` (a row with no `id`, or a page that did not honour the
cursor), now throws. The thrown error carries the driver's error as
`cause`, and nothing is cached for a failed read. What each caller now
sees:
- **Sharing:** the expansion rejects, so that rule's reconcile or revoke
pass fails. The existing per-rule catches (`evaluateAllRulesForObject`,
`bu-tree-recompute`, `queue-hooks`) log their warning, and the grants
the rule already holds stay. Before,
`revokeRuleGrantsForRetiredRecipients` diffed the rule's grants against
`[]` and revoked every one. **Narrowing:** a call that answered `[]` now
refuses.
- **Approvals:** the throw reaches `resolveApproverSpec`'s directory
catch, and the slot falls back to its literal. `position:NAME` can still
be acted on by whoever holds the position at decision time.
`org_membership_level:NAME` is decided by the override or
`onEmptyApprovers`. The catch now keeps the error, and the graph-type
warning becomes `approver 'TYPE:VALUE' could not be expanded — a
directory read failed …`, with `error` on the record. Before, a
`position` approver routed to its `sys_user_position` holders alone, a
shrunk slate that looked complete, and an `org_membership_level`
approver logged "expanded to nobody". **Narrowing:** a position slate
that routed to part of its people now routes to the literal.
- **Absence is still an answer.** A stack with no membership object
keeps answering `[]`. `membershipObjectAbsent` accepts exactly two
errors, each tied to `sys_member`: the shared `isMissingTableError(err,
'sys_member')`, and the engine's `OBJECT_NOT_FOUND` with `object:
'sys_member'` (a composition that never registers it, refused before any
driver is asked). The second follows the precedent
`sys-metadata-repository.ts` set for its history object. The full
approvals suite surfaced that composition in
`actor-lookups-hold-ids.integration.test.ts` and
`approval-requested-notification.integration.test.ts`.
- **Per-rule isolation on the record pass (seat round 2).**
`SharingRuleService.evaluateAllForRecord` now catches per rule, as
`evaluateAllRulesForObject` already did. A rule whose recipients cannot
be read keeps its grants and logs `[sharing-rule] record reconcile
failed for rule` with `object`, `recordId`, `rule` and `error`. The
record's remaining rules still reconcile.
- The approvals warning now reads `could not be expanded — a directory
or read-filter lookup failed`, because the try also wraps the cross-org
read filter (`bounded`). `content/docs/automation/approvals.mdx` names
it for exactly what this head does: a `position` or
`org_membership_level` entry whose `sys_member` read failed keeps its
literal, logs that warning with the `error`, and never routes to the
holders that happened to read. The general promise (team, department,
the `sys_user_position` limb) belongs to #29503.
- The changeset now names the second behaviour change:
`SharingRuleService.evaluateAllForRecord` resolves per rule. A rule
whose recipients cannot be read is left out of the results with a
warning, where before the whole call rejected. Its missing-object bullet
also names the engine's `OBJECT_NOT_FOUND` arm.
**Forced test edit (one file):**
`packages/plugins/plugin-sharing/src/position-graph.test.ts`. Its two
fake `sys_member` fixtures (`MEMBERS`, and `MEMBER` in the
`holdsPosition` matrix) now carry an `id` on each row, as every stored
row does. `keysetWalk` reports a row without its key as `truncated`, and
the expansion refuses such a walk. No assertion changed.
## Tests
-
`packages/plugins/plugin-sharing/src/team-graph-role-holders-complete.test.ts`
(new, real engine: ObjectQL + SqlDriver over in-memory SQLite, shipped
`SysMember`, scrambled membership ids):
- Past the bound: 10,001 holders all in the set, the other-org and
other-role rows out, 2 role reads.
- The position recipient carries the completed limb.
- Control: 9,999 holders, 1 read, the same set.
- A failed read rejects.
- A failed read is not cached.
- A position whose role limb fails rejects.
- An unregistered `sys_member` answers `[]`, and so does a dropped
table.
-
`packages/plugins/plugin-approvals/src/member-tier-holders-complete.integration.test.ts`
(new, same rig):
- Past the bound: every holder, 2 reads.
- An `org_membership_level` approver routes to all 10,001.
- Control: 1 read.
- A failed tier read rejects.
- A position whose tier limb fails rejects.
- What the caller sees: both slots fall back to their literals, and the
warning names the failed read and carries `error`.
- An unregistered `sys_member` answers `[]` with no failure warning.
- Reds on `origin/main` `7b1ae338` (tests committed first, code
untouched):
- sharing 5 failed / 2 passed: `expected [ 'usr_000000', …(9998) ] to
deeply equal [ …(9999) ]`, and `promise resolved "[]" instead of
rejecting`;
- approvals 5 failed / 1 passed: the same two shapes, plus `expected []
to deeply equal [ 'position', 'org_membership_level' ]`.
- Ablations: each leg mutated through `scripts/ablation-replace.mjs`.
The anchor hit 1→0 and the blob moved; after each leg the tool restored
the file and proved blob == HEAD with an empty `git diff HEAD`. No dist
leg is needed, because both suites import the plugin source directly.
- S1, sharing, `break` after the first page: the 2 past-bound pins are
red.
- S2, sharing, catch back to `return []`: the 3 failure pins are red.
- A1, approvals, `break` after the first page: the 2 past-bound pins are
red.
- A2, approvals, catch back to `return []`: the 3 failure pins are red,
the caller-sees pin among them.
- `recipient-width.test.ts` › `#29362 — the per-record pass is
best-effort per rule`. The position rule is reconciled first, and its
`sys_member` read fails. Its grant stays. The user rule after it
restores its missing grant. Exactly one `record reconcile failed for
rule` warning names the position rule. Ablation: rethrowing in the
catch, via `ablation-replace.mjs` (anchor 1→0, restored to blob ==
HEAD), turns it red with `[sharing] the holders of role 'sales_rep'
could not be read: SQLITE_IOERR …`.
## Acceptance notes
- **Share vs local paging (delta question).** The paging mechanism is
shared. Both sites use `keysetWalk` (`@objectstack/types`, already
exported, already imported by both plugins), so there is no third copy
of the cursor loop and no core export. What each plugin keeps locally is
the glue: page size, the absence predicate, and the refusal text. That
is about 25 lines per site. Exporting core's
`readOrganizationMembersToCompletion` would not have fitted as is: it is
keyed to `organization_id`, throws core's `AuthzStoreUnavailableError`
(503), and warns about `org_user_ids`. Generalising it would widen
core's public surface for two callers that already have a shared walker.
Put to the seat in `open_questions`.
- **#29398 is not folded in, and stays open.** It covers core's helper,
which this PR does not touch. Its should-fix 1 (a monotonic id check per
page) also conflicts with `keysetWalk`'s recorded design: "Judged by
EQUALITY, never by order". PG and MySQL collations do not order string
ids the way JS compares them, so an order check would refuse healthy
walks.
- **Same-family reads left as they are** (out of this card's
`sys_member` scope; listed in the report for the seat):
- sharing `expandUsers` (`sys_team_member`, `limit: 10000`, catch →
`[]`);
- both plugins' `sys_user_position` limb (`limit: 10000`, catch
swallowed);
- `holdsRole` / `teamHasMember`, the user-scoped twins (`limit: 1`,
catch → `false`), which now differ from `expandRoleUsers` only on a
failed read.
## Verification
Head: `435089b8`. Round 3 is prose only: `approvals.mdx` and the
changeset, no code.
- The 30 docs and changeset families derived for this head all ran `exit
0`. They include `check-adr-0087-registration`,
`check-changeset-no-major`, `check-empty-changeset`,
`check:changeset-gate-self-tests`, `check:docs` and `check:doc-anchors`.
- The other 63 derived families read code that has not changed since
`4c34c3c8`, where all 93 ran green. They were not re-run on this head (a
declared narrowing).
- The readings below were taken on `4c34c3c8`; no code moved since. It
merges `origin/main` after #29451 (`6af3d295`), the fix for main-red
#29426.
<details><summary>Package suites and typecheck</summary>
- Typecheck: both plugins exit 0 on `8fa01d1c`; the merge touched
neither plugin.
- plugin-sharing tests: Test Files 61/61, Tests 1325/1325.
- plugin-approvals tests: Test Files 108/108, Tests 1388/1388.
</details>
<details><summary>Derived gates</summary>
- Derived gates on `4c34c3c8`: 93 derived, 93 run, 0 NOT-MEASURED (`✓
dispatch-gates --ran: 93 derived famil(ies) accounted for`). The docs
file added 30 docs families. `check:type-check-debt` first ran `exit 3`
because it was queued behind the dogfood lock, then `exit 0`.
- `pnpm lint`: exit 0.
</details>
<details><summary>Dogfood battery (narrowing)</summary>
- Dogfood battery on `4c34c3c8` (`turbo run build
--filter='@objectstack/dogfood^...' && pnpm --filter
@objectstack/dogfood test`, under the lock, 37m15s): Test Files 397
passed / 3 skipped (400); Tests 3277 passed / 28 skipped (3305). Clean.
- The earlier four failures on `bf9ca7d7` were main-red #29426, fixed on
`main` by #29451.
</details>
---
_Generated by [Claude
Code](https://claude.ai/code/session_01ADHkmS2E1xdkLMJPnabYgC)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 3ab9136 commit 2789ba7
9 files changed
Lines changed: 658 additions & 36 deletions
File tree
- .changeset
- content/docs/automation
- packages/plugins
- plugin-approvals/src
- plugin-sharing/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
455 | 455 | | |
456 | 456 | | |
457 | 457 | | |
458 | | - | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
459 | 463 | | |
460 | 464 | | |
461 | 465 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
24 | | - | |
| 24 | + | |
25 | 25 | | |
26 | 26 | | |
27 | 27 | | |
| |||
1118 | 1118 | | |
1119 | 1119 | | |
1120 | 1120 | | |
| 1121 | + | |
| 1122 | + | |
| 1123 | + | |
| 1124 | + | |
| 1125 | + | |
| 1126 | + | |
| 1127 | + | |
| 1128 | + | |
| 1129 | + | |
| 1130 | + | |
| 1131 | + | |
| 1132 | + | |
| 1133 | + | |
| 1134 | + | |
| 1135 | + | |
| 1136 | + | |
| 1137 | + | |
| 1138 | + | |
| 1139 | + | |
| 1140 | + | |
| 1141 | + | |
| 1142 | + | |
| 1143 | + | |
| 1144 | + | |
| 1145 | + | |
1121 | 1146 | | |
1122 | 1147 | | |
1123 | 1148 | | |
| |||
2551 | 2576 | | |
2552 | 2577 | | |
2553 | 2578 | | |
| 2579 | + | |
| 2580 | + | |
| 2581 | + | |
2554 | 2582 | | |
2555 | 2583 | | |
2556 | 2584 | | |
| |||
2589 | 2617 | | |
2590 | 2618 | | |
2591 | 2619 | | |
2592 | | - | |
| 2620 | + | |
| 2621 | + | |
| 2622 | + | |
| 2623 | + | |
| 2624 | + | |
2593 | 2625 | | |
2594 | 2626 | | |
2595 | 2627 | | |
| |||
2656 | 2688 | | |
2657 | 2689 | | |
2658 | 2690 | | |
2659 | | - | |
2660 | | - | |
2661 | | - | |
2662 | | - | |
2663 | | - | |
2664 | | - | |
| 2691 | + | |
| 2692 | + | |
| 2693 | + | |
| 2694 | + | |
| 2695 | + | |
| 2696 | + | |
| 2697 | + | |
| 2698 | + | |
| 2699 | + | |
| 2700 | + | |
| 2701 | + | |
| 2702 | + | |
| 2703 | + | |
| 2704 | + | |
| 2705 | + | |
| 2706 | + | |
| 2707 | + | |
| 2708 | + | |
| 2709 | + | |
2665 | 2710 | | |
2666 | 2711 | | |
2667 | 2712 | | |
| |||
3355 | 3400 | | |
3356 | 3401 | | |
3357 | 3402 | | |
| 3403 | + | |
| 3404 | + | |
| 3405 | + | |
| 3406 | + | |
| 3407 | + | |
| 3408 | + | |
| 3409 | + | |
| 3410 | + | |
| 3411 | + | |
| 3412 | + | |
| 3413 | + | |
| 3414 | + | |
| 3415 | + | |
| 3416 | + | |
| 3417 | + | |
3358 | 3418 | | |
3359 | 3419 | | |
3360 | 3420 | | |
3361 | | - | |
| 3421 | + | |
3362 | 3422 | | |
3363 | | - | |
| 3423 | + | |
| 3424 | + | |
| 3425 | + | |
| 3426 | + | |
| 3427 | + | |
| 3428 | + | |
3364 | 3429 | | |
3365 | | - | |
3366 | | - | |
3367 | | - | |
| 3430 | + | |
| 3431 | + | |
| 3432 | + | |
| 3433 | + | |
| 3434 | + | |
| 3435 | + | |
| 3436 | + | |
| 3437 | + | |
| 3438 | + | |
| 3439 | + | |
| 3440 | + | |
| 3441 | + | |
| 3442 | + | |
| 3443 | + | |
| 3444 | + | |
| 3445 | + | |
| 3446 | + | |
| 3447 | + | |
| 3448 | + | |
| 3449 | + | |
| 3450 | + | |
3368 | 3451 | | |
3369 | 3452 | | |
3370 | 3453 | | |
| |||
0 commit comments