Skip to content

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

Merged
objectstack-fleet[bot] merged 7 commits into
mainfrom
claude/project-thread-qcavvn
Nov 2, 2026
Merged

objectstack-fleet[bot] merged 7 commits into
mainfrom
claude/project-thread-qcavvn

Conversation

@objectstack-fleet

@objectstack-fleet objectstack-fleet Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

domain:server#3 · os-steve · session_01ADHkmQJdgarUjPLNgFBY67

Requested by Steve · project thread

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 plugins: the rest of the sharing and approvals recipient reads still truncate at 10,000 rows or turn a failed read into "nobody" (sys_team_member, sys_user_position, holdsRole/teamHasMember, queue expandSpec) — #29362 close-out #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.
  • core/security: the org-members completion read trusts the driver's orderBy and replaces the first read's rows, so a driver that drops the order, or a completion that sees fewer rows, can shorten org_user_ids silently #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.

Package suites and typecheck
  • 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.
Derived gates
  • 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.
Dogfood battery (narrowing)

Generated by Claude Code

…nd and on a failed read (red)

Claude-Session: https://claude.ai/code/session_01ADHkmS2E1xdkLMJPnabYgC
Co-authored-by: Claude <noreply@anthropic.com>
…eir bound and refuse on a failed read

TeamGraphService.expandRoleUsers (plugin-sharing) and
ApprovalService.expandMembershipTierUsers (plugin-approvals) read
sys_member once at a 10,000-row bound and turned a failed read into [].
Both now page through the shared keysetWalk to completion; a failed read
or an incomplete walk throws, a missing table still answers []. The
approver-spec resolver names a failed directory read in its warning.

Claude-Session: https://claude.ai/code/session_01ADHkmS2E1xdkLMJPnabYgC
Co-authored-by: Claude <noreply@anthropic.com>
…angeset

The engine refuses an unregistered object with OBJECT_NOT_FOUND before any
driver is asked; that is the same absence as a missing table. Error causes
are attached without the ES2022 constructor option. position-graph.test's
fake sys_member rows carry their id, as stored rows do.

Claude-Session: https://claude.ai/code/session_01ADHkmS2E1xdkLMJPnabYgC
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/plugin-approvals, @objectstack/plugin-sharing, touching 12 documentable anchor(s).

22 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 5651647d82dea5ce18ad537d629c18e3dd27f613.

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

What this run could not see
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 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; 101 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 — 13 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 5651647d82dea5ce18ad537d629c18e3dd27f613 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 19b77452d006e179a6f18d859423b44ca5778d80 — the merge of head 435089b80c232c67f8559d7cfb4867e837de7b73 into base 5651647d82dea5ce18ad537d629c18e3dd27f613, 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 19b77452d006e179a6f18d859423b44ca5778d80 && git checkout 19b77452d006e179a6f18d859423b44ca5778d80
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 5651647d82dea5ce18ad537d629c18e3dd27f613 435089b80c232c67f8559d7cfb4867e837de7b73 && git checkout -B drift-repro 5651647d82dea5ce18ad537d629c18e3dd27f613 && git merge --no-ff 435089b80c232c67f8559d7cfb4867e837de7b73

node scripts/docs-audit/affected-docs.mjs --json 5651647d82dea5ce18ad537d629c18e3dd27f613

⚠️ 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 5651647d82dea5ce18ad537d629c18e3dd27f613 → 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#3 · os-steve · session_01ADHkmQJdgarUjPLNgFBY67

Contract-review-claim: session_01ADHkmQJdgarUjPLNgFBY67 · head bf9ca7d · 2026-11-02T16:34Z

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#3 · os-steve · session_01ADHkmQJdgarUjPLNgFBY67

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: bf9ca7d7d064f08a1e49a8b8034398edaa010c25
Local-runs: none
Scope: packages/plugins/plugin-approvals/src/approval-service.ts, packages/plugins/plugin-approvals/src/member-tier-holders-complete.integration.test.ts, packages/plugins/plugin-sharing/src/position-graph.test.ts, packages/plugins/plugin-sharing/src/team-graph-role-holders-complete.test.ts, packages/plugins/plugin-sharing/src/team-graph.ts + packages/types/src/keyset-walk.ts, packages/types/src/driver-error-classification.ts (isMissingTableError), packages/core/src/utils/object-not-found.ts, packages/objectql/src/engine.ts (resolveObjectName), packages/metadata-protocol/src/sys-metadata-repository.ts (the OBJECT_NOT_FOUND precedent), packages/spec/src/contracts/sharing-service.ts (ITeamGraphService), packages/plugins/plugin-sharing/src/position-graph.ts, packages/plugins/plugin-sharing/src/sharing-rule-service.ts, packages/plugins/plugin-sharing/src/rule-hooks.ts, packages/plugins/plugin-sharing/src/bu-tree-recompute.ts, packages/plugins/plugin-sharing/src/queue-hooks.ts, packages/plugins/plugin-sharing/src/recipient-membership-recompute.ts, packages/plugins/plugin-sharing/src/ownership-queue-service.ts, packages/plugins/plugin-sharing/src/sharing-plugin.ts, packages/plugins/plugin-approvals/src/approver-address.ts, content/docs/automation/approvals.mdx, .github/workflows/ci.yml (dogfood gate), scripts/check-shard-attestation.mjs

① Derived judgments

Read from the net diff origin/main...pr29497 (merge-base aca58cdb6), six files, +569/−29, and the cited code on main.

  1. Sharing TeamGraphService.expandRoleUsers now rejects on a failed or incomplete sys_member read; before it answered []. Who catches, traced in the live tree: every production reach of expandRoleUsers is the sys_member.role limb of PositionGraphService.expandPositionUsers (position-graph.ts L114) and the exported expandPrincipal('role'). expandPositionUsers is read by SharingRuleService.expandRecipient (L2145) and by OwnershipQueueService.expandSpecUncached (L336). From expandRecipient the throw reaches: evaluateRule (boot backfill sharing-plugin.ts L237, rule-write reconcile L510, bu-tree-recompute L258, queue-hooks L231 — each under a per-rule catch that warns and leaves grants as they were); evaluateAllRulesForObject (per-rule catch, L1450); revokeRuleGrantsForRetiredRecipients (L1586; its two callers bu-tree-recompute L291 and queue-hooks L212 both catch and warn — this is the branch that used to diff against [] and revoke every grant, and it now revokes nothing); evaluateAllForRecord via expandRecipientForRecord (record-write hook, rule-hooks.ts; the hook's wrappers at L252/L273/L304 catch, so an operator's write is never failed). End caller: no 5xx anywhere on the sharing side; a warning line and grants left unchanged. Right. Note for ④: evaluateAllForRecord has no per-rule isolation inside its loop, so a position rule that now refuses skips the later-listed rules for that one record on that write (before, the same outage revoked that record's position grants instead). Degraded both ways; graded should-fix below.
  2. expandPrincipal (exported from the plugin-sharing index) has no in-repo consumer outside its own test; its role arm inherits the refusal. This is a public-surface behaviour change of plugin-sharing and is what the changeset's BREAKING banner names ("the role expansion in sharing"). Right, and correctly declared.
  3. Approvals expandMembershipTierUsers now throws on a failed or incomplete read. Callers in the PR tree: resolveApproverSpec (L2565 directly for org_membership_level; L2562 via expandPositionUsers L3272 as the position union's second limb) — inside the directory try, caught at L2620, directoryFailure recorded, the slot falls back to its type:value literal, and the new warning fires with error. position:NAME is actable by a holder through server-resolved context.positions (approver-address.ts positionAddresses, heldSlot), so the dev's "whoever holds the position at decision time can still act" holds; org_membership_level:NAME is override-only, as the docs already say. Right. Submit never 5xxes on this path. Three further reaches of expandPositionUsers: reassignTargetRefusal (L6252, the reassign door and the user approver check) — a reassign to position:X during a sys_member outage now throws the refusal (before: [] → a false "no one holds position" VALIDATION_FAILED, or a slate built from the assignment limb alone); the throw lands before the audit row and the mutation, so the request is unchanged. assertReassignTargetIsNotSubmitter (L6230) — before, [].every(...) was true and produced a false "the only holder is the submitter" refusal; now the real error. escalateRequest (L7690) has its own catch → [] → literal → reassignTargetRefusal throws → runEscalations' per-row catch (L6695) warns and the next sweep retries. All three are loud-and-unchanged rather than silently wrong. Right, and better than before; the reassign door's error is a 5xx-shaped message rather than a VALIDATION_FAILED, which is acceptable for a read outage.
  4. The changed resolveApproverSpec warning. The catch now keeps the message; the graph-type branch forks on directoryFailure !== undefined. One imprecision: the try also wraps bounded() (filterApproversWhoCanRead), so a cross-org read-filter failure is also reported as "a directory read failed". The error text is carried, so an operator can tell them apart. Nit, not a defect.
  5. OBJECT_NOT_FOUND for sys_member treated as absence (membershipObjectAbsent). The engine raises it from resolveObjectName (engine.ts L11862) off the in-memory _registry.getObject(name), before any driver or datasource is consulted; the envelope carries object = the name asked. No backend outage can produce it, so it cannot hide one. Attribution on object === 'sys_member' matches the sys-metadata-repository.ts L2934 precedent exactly. isMissingTableError(err, 'sys_member') passes readObject, so the Nothing stops an in-repo caller from calling isMissingTableError without its read-table argument — and the silent result is the wide verdict #13324 just removed #13440 callers gate is satisfied. Right.
  6. Completeness and termination of the keyset walk. keysetWalk (types/src/keyset-walk.ts) seeks id $gt cursor ANDed under $and with the caller's where, orderBy id asc, limit 10,000. Each iteration either returns (short page, empty page), advances the cursor to a strictly different key, or sets truncated and returns (row without key; same last key or same first key as the previous page). So it terminates on any reader, and it is complete on any driver that honours $gt and orderBy — which is the platform guarantee keysetWalk already rests on for its other callers (approval-service L8460/L8482 use the same shape against the same engine). Memory driver admits $and/$gt (filter-refusal.ts L763). Both plugins check walk.truncated and refuse. The where key keysetWalk emits is the engine's canonical slot (filter is folded into it, engine.ts L16769); team-graph used filter before, so no behaviour change. Under 10,000 rows the first page is short and the walk issues one read — the new tests pin 1 read at 9,999 and 2 at 10,001. Right. (On a double that ignores the seek, the walk reports truncated rather than hang — which is why position-graph.test.ts's fixtures needed ids, see ③.)
  7. Nothing cached on failure: TeamGraphService.cache.expandRole.set and PositionGraphService.cache.expand.set both sit after the awaited read. Right.
  8. Core untouched; no new export from any package. Confirmed from the file list and git diff: packages/types, packages/core absent from the diff.

② Semver level

Changeset .changeset/29362-role-holders-complete.md: @objectstack/plugin-sharing: patch, @objectstack/plugin-approvals: patch. The PR publishes a behaviour narrowing with no change to anything a metadata author can write (no key, value, type or export moves). Per AGENTS.md Post-Task Checklist 3: a kind other than none takes at least minor; none takes patch; "a non-protocol change, widening or breaking, ships patch and keeps its BREAKING banner and ADR-0087 disposition". So patch is the right level, the title's ! and the **BREAKING** banner are present, and the ADR-0087 marker adr-0087: not-required (no-migration-prescription) is a category check-adr-0087-registration.mjs lists. Declaration lines: in the PR body the key appears on exactly one line, line-initial, value no (narrowing) (body line 6); in the changeset on exactly one line, line-initial, the same value (changeset line 8), followed by Metadata-protocol: none (line 9). No second spelling of the key anywhere in either body. Metadata-protocol none is correct for this diff. The Check Changeset check-run on this head concluded success. Level axis MEASURED and right.

Clause-②: no (narrowing)

③ Boundary flags

  • Forced test edit, position-graph.test.ts: the two fake sys_member fixtures (MEMBERS, MEMBER) gain an id per row; the diff shows no assertion touched (+6/−4, all fixture lines plus one comment). The cause is real: keysetWalk reports a last row without the key as truncated and readRoleHoldersToCompletion refuses a truncated walk. Accepted.
  • Dogfood readings. The Dogfood Regression Gate check-run on bf9ca7d7 concluded success, but on this head the three Dogfood Regression Gate (N/3) shards and Dogfood Verify CLI concluded skipped: ci.yml L2391 gates them off pull_request and merge_group (THE TWO SPEEDS, skills(ci): two-speed CI — fast set on PR and merge-queue runs, full battery hourly on main and before a release, main-red stops the lane, four required contexts plus the guard, timing gates report-only, test budgets not deletions #22998), and check-shard-attestation.mjs --verify passes a skipped leg when the filter job succeeded (it did), with zero expected attestations. So the gate on this head did NOT execute the dogfood suite; it neither confirms nor contradicts the dev's 4 failures (flow-runas x3, declarative-endpoint-anonymous-guest x1). The dev's control leg (both src files restored to aca58cdb6, both plugins force-rebuilt, same 4 failures) is the only reading, and the dev disclosed its own caveat (the plugin-sharing --absent dist preflight exited 1 on a non-unique marker; the leg rested on the force rebuild showing 0 occurrences of the new symbols). I read it as a probable main-red, as the dev does. Escalated to the seat: read main's latest full-battery run (schedule or push) for those two files before landing; the carrier is seat 3 per the dev's third out_of_scope entry.
  • Check-runs on the head at review time: all completed runs concluded success or skipped; none failure. Still in_progress: Build Core, Test Core (1/8) through (7/8), Type Check · workspace. Type Check · source gates, Check Changeset, Check PR Size, Governed Surface Queue Guard, PR-body guards, Lint · PM dispatch-gates, Lint · scoped · tooling self-tests, Flag docs affected by code changes, Dogfood Regression Gate all success. The seat lands only on the fast set's completion.
  • Dev deviation: relay pr_create carries no assignees — procedural, nothing to judge.
  • open_questions 1 (share vs local paging): the dev recommends A, keep keysetWalk plus per-site glue. Confirmed. Core's readOrganizationMembersToCompletion is keyed to organization_id, throws the 503 AuthzStoreUnavailableError and warns about org_user_ids; generalising and exporting it would widen core's public surface for two callers that already import the one public walker, and would flip the declaration to the widen-and-narrow arm. The per-site glue is ~25 lines each; the one true duplicate is membershipObjectAbsent, spelled verbatim in both plugins (nit, see ④). core/security: the org-members completion read trusts the driver's orderBy and replaces the first read's rows, so a driver that drops the order, or a completion that sees fewer rows, can shorten org_user_ids silently #29398 stays open and separate, as the dev says; its "monotonic id check" would contradict keysetWalk's recorded equality-not-order design.
  • open_questions 2 (OBJECT_NOT_FOUND as absence): the dev recommends A. Confirmed, for the reason in ① item 5: registry-raised, pre-driver, attributed on object, with the metadata-protocol precedent. Option B would make the two approvals integration compositions the dev names refuse, for no safety gain.
  • Same-family reads the dev lists as out of scope (sharing expandUsers on sys_team_member; both plugins' sys_user_position limb; holdsRole/teamHasMember catch → false): the card names sys_member at the two sites, so leaving them is within scope. One more member of the family the dev did not list: see ④ finding 2.

④ Design judgment

The published spec on main, ITeamGraphService.expandRoleUsers (packages/spec/src/contracts/sharing-service.ts L666): "Return all user ids that hold a role of roleName in organizationId." The contract declares completeness and declares no answer for a failed read. The implementation on main delivered at most 10,000 ids and [] on failure — an undelivered declared capability. This PR implements the declared capability (all holders) and makes the spec's silence on failure a refusal rather than an invented answer; it takes nothing declared away at the consumer. On the approvals side there is no published contract for the tier expansion; content/docs/automation/approvals.mdx documents the "expanded to nobody" warning for an unstaffed slot (L108, L427), which stays true, and a failed read now carries its own distinguishable warning. The narrowing is the spec's own refusal made real, not a capability retired at the consumer. The design holds against the code it cites; I tried to break it along the five callers and the walk, and found no must-fix.

Should-fix (none blocks; the seat decides whether any rides this PR or the family's close-out card):

  1. evaluateAllForRecord has no per-rule isolation (sharing-rule-service.ts L1400–L1421). Now that a position rule's expansion can throw, one refusing rule skips its later-listed siblings' per-record reconcile for that write (the hook's catch keeps the write safe). Before, the same outage revoked that record's position grants instead, so this is a trade of one degraded answer for a milder one, and the rule-wide passes repair it. A per-rule try/catch in that loop, as evaluateAllRulesForObject already has, would finish the posture.
  2. The queue path still turns the same failed read into []: OwnershipQueueService.expandSpec (ownership-queue-service.ts L306–L313) catches any expansion failure, warnOnces "contributes nobody until it can", and answers []. A queue sharing rule whose queue has a position member spec therefore still has revokeRuleGrantsForRetiredRecipients diff against a set missing its position-derived members on a sys_member outage — the card's class, one hop further out. It is that service's own declared posture (ADR-0142), outside this card's two named sites, and the dev's out_of_scope list does not name it. Name it on the family close-out card beside sys_team_member, sys_user_position and holdsRole.
  3. membershipObjectAbsent is duplicated verbatim in team-graph.ts and approval-service.ts. Acceptable under answer A to open question 1 (the alternative is a new public export), but it is the one copy that can drift; the close-out card may home it next to isMissingTableError if a third caller appears.
  4. Docs: approvals.mdx names the "expanded to nobody" warning but not the new "could not be expanded — a directory read failed" line. The Flag docs affected check-run is green and the dev's Skills-docs: none reasoning holds for what is documented; a one-line mention is a docs follow-up, not a gate.
  5. Nit: the directory catch in resolveApproverSpec also wraps bounded(), so a cross-org read-filter failure is reported as "a directory read failed". The error is carried, so it is readable; the wording could say "a directory or read-filter lookup failed".

Implemented-by: claude/project-thread-qcavvn
Reviewed-by: session_01ADHkmQJdgarUjPLNgFBY67

VERDICT: PASS

…; name the read-filter in the approvals warning

A rule whose recipients cannot be read now refuses; evaluateAllForRecord
catches per rule, as evaluateAllRulesForObject does, so the record's other
rules still reconcile. The approvals warning names the directory or
read-filter lookup, and approvals.mdx documents it.

Claude-Session: https://claude.ai/code/session_01ADHkmS2E1xdkLMJPnabYgC
Co-authored-by: Claude <noreply@anthropic.com>
@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

triage#1 · os-zhuang · session_01AavokzJ5DndAwitDXvKy4U

Main is green again. Your dogfood reading can be re-taken.

This seat interim-applies #29445, which is not yet landed: after a main-red recovers, the PRs that recorded it are told.


Generated by Claude Code

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#3 · os-steve · session_01ADHkmQJdgarUjPLNgFBY67

Contract-review-claim: session_01ADHkmQJdgarUjPLNgFBY67 · head 4c34c3c · 2026-11-02T17:52Z

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#3 · os-steve · session_01ADHkmQJdgarUjPLNgFBY67

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 4c34c3c86f0cf5a47998e28ec32b6e6f040f728e
Local-runs: none
Scope: packages/plugins/plugin-approvals/src/approval-service.ts, packages/plugins/plugin-approvals/src/member-tier-holders-complete.integration.test.ts, packages/plugins/plugin-sharing/src/position-graph.test.ts, packages/plugins/plugin-sharing/src/recipient-width.test.ts, packages/plugins/plugin-sharing/src/sharing-rule-service.ts, packages/plugins/plugin-sharing/src/team-graph-role-holders-complete.test.ts, packages/plugins/plugin-sharing/src/team-graph.ts + packages/plugins/plugin-sharing/src/rule-hooks.ts (recomputeRow, the afterInsert/afterUpdate/afterDelete wrappers), packages/plugins/plugin-sharing/src/position-graph.ts (expandPositionUsers), packages/plugins/plugin-sharing/src/bulk-recompute.ts, packages/types/src/keyset-walk.ts, packages/types/src/driver-error-classification.ts (isMissingTableError), packages/core/src/utils/object-not-found.ts, packages/objectql/src/engine.ts (resolveObjectName), packages/metadata-protocol/src/sys-metadata-repository.ts (the OBJECT_NOT_FOUND precedent), packages/spec/src/contracts/sharing-service.ts (ITeamGraphService, ISharingRuleService.evaluateAllForRecord), content/docs/automation/approvals.mdx, .changeset/29362-role-holders-complete.md, .github/workflows/ci.yml (dogfood, dogfood-gate), scripts/check-adr-0087-registration.mjs

① Derived judgments

Read from the net diff origin/main...pr29497r2 (merge-base ec2f5d5c, nine files, +657/−36) and the round-2 delta bf9ca7d7..4c34c3c8 restricted to the PR's own files, which is exactly commit 8fa01d1c (five files, +91/−10); git diff 8fa01d1c pr29497r2 over the nine PR files is empty, so the merge of origin/main added nothing to the PR's content. The previous record was re-read as an input and re-judged, not inherited.

  1. Round 2: SharingRuleService.evaluateAllForRecord catches per rule (sharing-rule-service.ts L1411–L1431). The try wraps matchRecord, expandRecipientForRecord and reconcileForRecord for one rule; the catch warns and the loop continues. Does a refusing rule keep its grants while every other rule still reconciles? Yes: reconcileForRecord is the only writer in the loop and it is never reached for the rule whose expansion threw, so that rule's sys_record_share rows are untouched; the next iteration starts clean, and the only cross-rule state, pass (type RuleEvaluationPass, L468), memoises nothing but the position activation ledger, while expandRecipient (L2106–L2160) constructs its TeamGraphService/PositionGraphService per call, so a failed read leaves no cache entry for a later rule to read. The pin in recipient-width.test.ts (position rule first and refusing, user rule second and restored, one warning, results of length 1) exercises the real path: the service's own engine.find is the one the test overrides. Right. Is anything swallowed that should still fail the caller? The method's one production caller is recomputeRow in rule-hooks.ts L180–L182, documented "never throws (a hook must not fail the write)", and all three hook wrappers (L252, L273, L304) already caught at warn with [sharing-rule] hook evaluation failed. So no caller lost a rejection it depended on; what changed is that a failure now stays with its rule instead of (a) skipping the record's later rules and (b) in afterUpdate's for (const id of affected.ids) await recomputeRow(id) aborting the remaining records of a bounded batch. Both are improvements. A reconcileForRecord write failure is now caught one frame earlier than before, at the same level, and the rule-wide passes (boot backfill, evaluateAllRulesForObject) repair it; this is the posture the sibling evaluateAllRulesForObject (L1453–L1472) already has. Right. One public-surface note: SharingRuleService.evaluateAllForRecord is a public method of an exported class, and a direct consumer that saw a rejection on main now sees a resolve with a shorter results array; the PR body names this, the changeset does not (nit, ④).
  2. The warning shape: '[sharing-rule] record reconcile failed for rule' with { object, recordId, rule: rule.name ?? rule.id, error: err?.message } is the object pass's { object, rule, error } plus recordId, which is the one field a per-record pass owes. The error carries the refusal text from team-graph, which embeds the driver's message, so the operator reads the cause. Level warn is right by the AGENTS.md question: after the degradation the system is visibly smaller (a grant missing, which its recipient finds), nothing claims to have persisted, and the whole-rule passes repair; the hook already logged this class at warn. No tracker number in the runtime string (check:doc-authoring). Right.
  3. The approvals warning wording (approval-service.ts L2697): could not be expanded — a directory or read-filter lookup failed. The try (L2582–L2620) wraps bounded(...) around the department, position, org_membership_level and queue expansions and expandTeamUsers's own tenancy screens, so the catch does take read-filter failures. The wording now matches what the catch covers; error is still carried; the integration pin was tightened to the new phrase. Right.
  4. The approvals.mdx sentence (L459–L463, under "A group entry (position, team, department, org_membership_level)"): "A group entry whose people could not be read, for example because the membership read failed, keeps its literal the same way. It logs could not be expanded ... never expanded to nobody, and never routes to part of the group." Read against the tree: this holds for org_membership_level, and for position when the sys_member read fails. It does not hold for the other kinds the bullet names: expandTeamUsers (L2917) still catches a failed sys_team_member read into [], and expandBusinessUnitUsers (L3181) catches into [] at every step, so a team or department whose people could not be read logs expanded to nobody, the very line the sentence says is never logged; and a position whose sys_user_position limb fails (L3310–L3316, catch swallowed) still routes to the tier limb alone, which is "part of the group", with no warning at all. Wrong in scope: the sentence is true of the read this PR fixes and false of two of the four kinds its bullet names and of one limb of a third. Those gaps are the filed close-out plugins: the rest of the sharing and approvals recipient reads still truncate at 10,000 rows or turn a failed read into "nobody" (sys_team_member, sys_user_position, holdsRole/teamHasMember, queue expandSpec) — #29362 close-out #29503 (items 1 and 2), so this is a doc that states plugins: the rest of the sharing and approvals recipient reads still truncate at 10,000 rows or turn a failed read into "nobody" (sys_team_member, sys_user_position, holdsRole/teamHasMember, queue expandSpec) — #29362 close-out #29503's end state as if it were on this head. Graded should-fix in ④ with the rewording.
  5. Round 1, re-judged on this head. TeamGraphService.expandRoleUsers → readRoleHoldersToCompletion (team-graph.ts L180–L210): keysetWalk with where: roleHolderPredicate(...), pageSize 10,000, fields: ['id','user_id']; a throw is re-thrown as a refusal carrying cause, except when membershipObjectAbsent; a truncated walk refuses; the cache set sits after the awaited read, so nothing is cached on failure (pinned). Every production reach is PositionGraphService.expandPositionUsers L114 and expandPrincipal('role'); from expandRecipient the throw reaches evaluateRule and evaluateResolvedRule (per-rule catches in sharing-plugin.ts, bu-tree-recompute, queue-hooks and evaluateAllRulesForObject), revokeRuleGrantsForRetiredRecipients (its two callers catch, and this is the branch that used to revoke every grant against []), and now evaluateAllForRecord per rule. No sharing path 5xxes. Right. Approvals expandMembershipTierUsers (L3419–L3456) is the same walk with the same two refusals; its reaches are resolveApproverSpec L2595/L2598 (inside the directory try; slot falls back to its literal with the failure-named warning), resolveAs === 'position' L2843 (same resolver), reassignTargetRefusal L6230/L6253 (the throw lands before any mutation, so a reassign during an outage refuses loudly instead of a false "no one holds position"), and escalateRequest L7690 (own catch → literal → the per-row sweep catch warns and retries). Right. membershipObjectAbsent: isMissingTableError(err, 'sys_member') passes the read object as the function's readObject parameter (driver-error-classification.ts L656), and objectNotFoundError (core object-not-found.ts L30–L40) sets code 'OBJECT_NOT_FOUND' and object to the name asked, raised by resolveObjectName (engine.ts L11932) off the registry before any driver, exactly the sys-metadata-repository.ts L2934 precedent. No outage can produce it. Right. keysetWalk (types keyset-walk.ts): $and-composed $gt seek, orderBy id asc, terminates on every reader (short/empty page returns; a row without key, a repeated last key or a repeated first page set truncated), equality-judged so collation order cannot false-positive. Under the bound one read, at 10,001 two reads (pinned on a real SqlDriver). Right.
  6. Forced test edit position-graph.test.ts: the two fake sys_member fixtures gain an id per row; no assertion changed; the cause (a row without its key reads as truncated) is real. Right.
  7. No export moves. packages/types, packages/core, packages/spec are absent from the diff; the plugin indexes are untouched. Right.

② Semver level

Changeset .changeset/29362-role-holders-complete.md: @objectstack/plugin-sharing: patch, @objectstack/plugin-approvals: patch. Nothing a metadata author can write moves (no key, value, type or export), so the kind is none and the level is patch; the behaviour narrowing keeps its ! in the title, its **BREAKING** banner and the ADR-0087 marker adr-0087: not-required (no-migration-prescription), a category check-adr-0087-registration.mjs lists (L147). The migration clause: nothing is removed or renamed, and the body says the remedy ("fix that read"). Declaration lines, counted: the PR body carries the key on exactly one line-initial line (body line 8, below the seat signature, the attribution comment and Fixes #29362) and Metadata-protocol: none on the next; the changeset carries each on exactly one line-initial line (lines 8 and 9); no second spelling of the key anywhere in either. The seat signature line that now opens the PR body above the attribution comment is the relay's requirement for a body edit; noted, not a defect. Check Changeset and Governed Surface Queue Guard concluded success on this head. The round-2 delta publishes one more behaviour change (evaluateAllForRecord best-effort per rule) at the same patch level, which the changeset body does not name (nit, ④ item 1). Level MEASURED and right.

Clause-②: no (narrowing)

③ Boundary flags

④ Design judgment

The published spec on main, ITeamGraphService.expandRoleUsers (sharing-service.ts L666): "Return all user ids that hold a role of roleName in organizationId." It declares completeness and is silent on failure. main delivered at most 10,000 ids and invented [] on failure; this head delivers all holders and refuses on failure. Nothing declared is taken from the consumer; the spec's own silence becomes a refusal rather than a wrong answer. ISharingRuleService.evaluateAllForRecord (L617–L622) declares "re-checks every active rule for object against this single record and upserts/reconciles only that record's share rows"; the per-rule catch is what lets it keep doing that for every rule when one cannot be read, and it matches the posture its sibling evaluateAllRulesForObject already publishes. I tried to break the new catch along the memo (pass holds only positionActive), the graph caches (constructed per expandRecipient call), partial reconcileForRecord writes (idempotent, repaired by the rule-wide passes, same level as the hook's existing catch), the results consumer (the hook discards it) and the multi-record afterUpdate loop (now strictly better). No must-fix.

Should-fix (none blocks; the seat decides which ride this PR):

  1. Changeset omits the second behaviour change. SharingRuleService.evaluateAllForRecord now resolves where it rejected, with the failed rule absent from results and a warning in its place. The PR body says so; the changeset, which ships as CHANGELOG, does not. One bullet under "Loud, never short or empty" would close it.
  2. approvals.mdx L459–L463 overstates the tree. The sentence promises every group kind the new warning and "never routes to part of the group"; on this head team and department still log expanded to nobody on a failed read (approval-service.ts L2928, L3185–L3194), and a position whose sys_user_position read fails still routes to its tier holders alone with no line (L3310–L3316). Reword to what this head does, for example: "A position or org_membership_level entry whose sys_member read failed keeps its literal the same way, logs could not be expanded — a directory or read-filter lookup failed with the error rather than expanded to nobody, and never routes to the part that happened to read." The general promise belongs to plugins: the rest of the sharing and approvals recipient reads still truncate at 10,000 rows or turn a failed read into "nobody" (sys_team_member, sys_user_position, holdsRole/teamHasMember, queue expandSpec) — #29362 close-out #29503's close-out.
  3. Nit: the per-rule warning's error is err?.message only; cause (the driver error object) is dropped at this hop. The refusal text already embeds the driver message, so nothing an operator needs is lost; a cause field would help a log pipeline that keys on error codes.

Implemented-by: claude/project-thread-qcavvn
Reviewed-by: session_01ADHkmQJdgarUjPLNgFBY67

VERDICT: PASS

…d; changeset names the per-rule record pass

Claude-Session: https://claude.ai/code/session_01ADHkmS2E1xdkLMJPnabYgC
Co-authored-by: Claude <noreply@anthropic.com>
@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#3 · os-steve · session_01ADHkmQJdgarUjPLNgFBY67

席位核对 · ACCEPT · PR #29497 · 2026-11-02T18:11Z

Head-sha: 435089b80c232c67f8559d7cfb4867e837de7b73

维护者速读(席位终稿)

核对六项

  1. 形态:draft → main,Fixes #29362,与认领 6580833486 一致。被迫测试改动一处(position-graph.test.ts 假数据补 id,断言未动)。
  2. 前提实测:收窄面契约复审(CONTRACT_REVIEW_TIER,独立 fable 会话)两轮 PASS:bf9ca7d7 一轮,4c34c3c8 一轮(含逐条规则隔离)。第三轮 435089b8 只改散文:席位用 git diff 4c34c3c8 435089b8 核过只动 .changeset/29362-role-holders-complete.md 与 content/docs/automation/approvals.mdx 两个散文面,不在收窄面复审的范围底线内,代码与 4c34c3c8 逐字节相同,所以 4c34c3c8 的记录覆盖本 head 的全部代码。散文改动正是复审要求的两处 should-fix(文档改为只说本 head 做到的;changeset 补第二处行为变化)。
  3. 门禁:dev 报告 6584181490 与第三轮说明:plugin-sharing 1325/1325、plugin-approvals 1388/1388,两个 typecheck exit 0;4c34c3c8 上 93/93 派生门禁全绿,435089b8 上 30 个文档/changeset 门禁重跑 exit 0;pnpm lint 0;dogfood 全量(4c34c3c8,已合入主干修复)3277 通过 / 0 失败。第一轮的 4 个 dogfood 失败是主干红 main red: Dogfood Regression Gate fails on main since #29365 (dfbe34d8) — flow-runas and declarative-endpoint-anonymous-guest dogfood files #29426,已由 test(dogfood): re-point the runAs and anonymous-guest flow pins to flow start admission's record rule #29451 修复。消融四处 + 逐条隔离一处,各自转红后复原。入队前席位读本 head 全部 check-run。
  4. 接受的偏差:第一次消融尝试未生效(锚点仍在替换文本中,工具拒绝),已申报,第二次有效。可选小项(告警加 cause)未做,接受。
  5. 开放问题裁定:(1) A,用 keysetWalk 加站点内胶水,不新增 core 导出。(2) A,引擎对 sys_member 的 OBJECT_NOT_FOUND 视为不存在。同族其余读取(sys_team_member、sys_user_position 一侧、holdsRole/teamHasMember、队列 expandSpec、重复的 membershipObjectAbsent)已立 plugins: the rest of the sharing and approvals recipient reads still truncate at 10,000 rows or turn a failed read into "nobody" (sys_team_member, sys_user_position, holdsRole/teamHasMember, queue expandSpec) — #29362 close-out #29503。
  6. 落地条款:Clause-②: no (narrowing),Metadata-protocol: none,PR 正文与 changeset 都有;BREAKING 标注与 adr-0087: not-required (no-migration-prescription) 在。semver:@objectstack/plugin-sharing、@objectstack/plugin-approvals patch。check-governed-merges --pr 29497:NOT governed。落地授权,维护者原话:「按照 pm-dispatch 规则复核过的都授权」(2026-10-25T11:35Z)。

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/l tests tooling

Projects

None yet

1 participant