Repository navigation
Commit 13c0c5b
fix(plugin-approvals)!: an approval node refuses to open when a team or position directory read fails or a queue expansion rejects, and the ownership-queue reads page to completion (#30288)
<!-- claude-projects-attribution: {"github_login":"os-bill"} -->
_Requested by **Bill** · [project
thread](https://claude.ai/code/project/chan_016iMXWSZDzqWRWvWzdNo8wg?thread=cmsg_016iMXWSZDzqWRWvWzdNo8wgTHYrCvGEffG5116eJa3f7Z)_
domain:server#4 · os-bill · session_01Utp6CZMkHvuDeVYgTD73qF
## 维护者速读
- **改了什么:** 审批节点里 team、position 审批人的名录读取(含跨组织读过滤)失败,或 queue
审批人展开时报错,节点不再开单:四种 onEmptyApprovers 策略(含 fallback 名单)一律拒开,报可重试的
`APPROVER_DIRECTORY_UNREADABLE`,点名是哪次读取失败。队列成员与队列列表改为按 id 分页读完,不再在第
10,000 行截断。
- **为什么改:** 以前名录表一出故障,节点就当这些人不存在:只开给读得到的人、转给 fallback 或管理员,甚至自动通过(裁定
6599731698)。队列超过 1 万成员时,第 10,001 人静默不在队列里。
- **风险与回滚:** 只在名录读取失败时多拒开一次,流程可在名录恢复后重跑;department、BU、成员等级、manager
不变,决定时的旧路径保留字面值(有 pin)。队列服务自身读成员表失败时仍答「无人」,queue 审批人照旧不占位,留待第 2 部分(等
#30251)。回滚即撤 PR。
- **先看:** `approval-service.ts` 的 `ApproverDirectoryUnreadableError` 与
`resolveApproverSpec` catch 里的一处 throw;`ownership-queue-service.ts` 三处
`readMembershipToCompletion`。
- **两分钟验证:** `pnpm --filter @objectstack/plugin-approvals exec vitest
run src/approval-service.test.ts -t "refuses the node"`,预期 18 条全过;`pnpm
--filter @objectstack/plugin-sharing exec vitest run
src/ownership-queue.test.ts -t "page to completion"`,预期 5 条全过。
- **需要你做的:** 无,席位落地;第 2 部分等 spec 车道的 #30251。
Refs #29604 (part 1)
Clause-②: yes (narrowing) — an approval node no longer opens when a
`team` or `position` approver (static, or an `expression` approver with
`resolveAs` `team` / `position`) cannot be expanded because a directory
read or the cross-organization read filter failed, or when a `queue`
approver's expansion rejects. Under every `onEmptyApprovers` policy, the
`fallbackApprovers` slate included, it now fails the node with a
retryable `APPROVER_DIRECTORY_UNREADABLE:` error naming the read, where
it used to open on the rest of the slate, on the fallback slate or on
the administrators, or auto-approve. The registered ownership-queue
service does not reject yet: a failed member read inside it still
answers nobody until #29604 part 2 (#30251), so a `queue` approver
refuses here only when `expandMembers` itself rejects. Every other input
answers as before. Nothing is widened: the new error class is not on the
package entry.
Metadata-protocol: none
Skills-docs: none. No page in `skills/**` or `content/docs/**` states
what an approval node does when a directory read fails, so no text
moves.
## What changes
This is part 1 of #29604, as the seat ruled (route A, comment
6648946388): the parts that do not wait on the spec contract. The
queue's own refusal on D6 and D7 is part 2, behind the spec-lane card
#30251, and will carry `Fixes #29604`.
**1. The approval node refuses to open when a team or position directory
read fails, or a queue approver's expansion rejects (ruling 6599731698,
letter B; ADR-0142 D6 as amended).**
- `ApproverDirectoryUnreadableError` (plugin-approvals, not on the
package entry) carries `approverType`, `approverValue`, `failedRead` and
`retryable: true`, and the original error as `cause`. Its message starts
`APPROVER_DIRECTORY_UNREADABLE:` (the message-prefix family this service
already uses for `NO_APPROVERS:`), names the approver, the organization
and the read, and says that the node is not opened on the rest of the
slate, the fallback slate or the administrators, and is not
auto-approved. It never says `NO_APPROVERS`.
- In `resolveApproverSpec`, the catch that records `directoryFailure`
now throws that error first when the approver type is `team`, `position`
or `queue`. Because the throw sits in the catch, it runs before the
`queue` branch's "expanded to nobody" return, which used to answer an
unreadable queue as an empty one. That is the reorder the ruling asks
for. On this head it fires for a queue only when `expandMembers`
rejects: the registered `OwnershipQueueService` still answers its own
failed member reads with nobody until part 2, so a queue backed by it
adds no slot on such a failure, as before.
- `resolveExpressionApprovers` throws the same error for `resolveAs:
'team'` and `'position'`, since those values are read by the same
expanders. Its ADR-0105 D9 filter call now sits in its own try and
refuses the same way, naming the filter's read; any other failure there
fails the node as it always did.
- The fallback slate is expanded by the same resolver, so a
`fallbackApprovers` slate whose own team or position read fails refuses
too, rather than degrading to `admin_rescue`.
- The position expander's `sys_member` limb (the transition read) is a
failed read like any other, as the carried item says (pinned on a real
`sys_member`).
- The approval node executor already turns a thrown error into `{
success: false }` with the `runtime` failure class, which is retryable.
`approval-node.ts` is not touched.
- `expandApprovers` takes `onDirectoryFailure: 'refuse' | 'literal'`
(default `'refuse'`). Only `decideNode`'s pre-snapshot re-resolution
passes `'literal'`: it runs for requests opened before the approver
snapshot existed, after the decision's action row is written, so a
refusal there would fail a decision that is already recorded. It keeps
the literal slot it always kept.
- The static resolver's cross-organization read filter (`bounded`) sits
inside its try, so its failure is now rethrown as `the
cross-organization read filter (…) could not be read: …`, and the
warning and the refusal name that read rather than the directory's
(carried item). One helper words that read for the static and the
`expression` resolver. `filterApproversWhoCanRead` itself still answers
its own failed `sys_member` read by keeping the slate (ADR-0105 D9), so
in practice this naming is reached only when the filter throws past its
own catch.
**2. The ownership-queue reads page to completion (the 10,000-row
reading, 6591670782).**
`queuesNaming`, `authoredQueueNames` and `authoredMemberSpecs` read
`sys_queue` / `sys_queue_member` through `readMembershipToCompletion`
(`team-graph.ts`, the keyset walk the team and position reads already
use since #29503), where each read one page of `QUEUE_READ_LIMIT =
10000` rows and answered on it. `QUEUE_READ_LIMIT` is gone. What a
failed read answers is unchanged until part 2: the catch keeps its
warn-once and contributes nobody. The warning carries the read's reason
only: the driver's error, or for a walk that could not vouch for
completion its sentence without the walk's own "so the expansion refuses
instead", which is not this service's answer yet. A walk that fails on a
later page now answers like a failed read, never with the pages already
read. `authoredQueueRow` is a one-row lookup by name and stays as it is.
## Tests
- `approval-service.test.ts`, new top-level `describe` "a team, position
or queue approver whose directory read fails refuses the node (ADR-0142
D6)", 14 cases on the shared fake engine with one table's reads made to
throw:
- team (`sys_team_member`) and position (`sys_user_position`) ×
`admin_rescue`, `auto_approve`, `fail`, `fallback`: rejects with
`ApproverDirectoryUnreadableError` (`approverType`, `approverValue`,
`retryable: true`, message prefix, `failedRead` names the driver error,
never `NO_APPROVERS`), and no `sys_approval_request`,
`sys_approval_approver` or `sys_approval_action` row is written. 8
cases.
- Beside a staffed `user` approver, the node still refuses.
- A fallback slate naming a team whose read fails refuses, with the
primary slate readable and empty.
- A queue whose `expandMembers` rejects refuses, and the "expanded to
nobody" warning is not logged.
- Control: the readable slate opens as before.
- Control: a failed `department` read still opens the node (not named by
the ruling).
- `decideNode` on a pre-snapshot request still decides while the team
read fails.
- Existing integration pins on driver-sql moved from "keeps the literal"
to the refusal, each followed by the `'literal'` reader keeping the old
slot and warning: `team-position-holders-complete` (team, position),
`member-tier-holders-complete` (position whose `sys_member` limb fails;
`org_membership_level` keeps its literal),
`business-unit-holders-complete` (`expression` with `resolveAs:
'team'`).
- `approval-service.test.ts`, a second new top-level `describe` "a
failed cross-organization read filter refuses the node, named as that
read (ADR-0142 D6)", 4 cases on the shared fake engine and the cross-org
scenario of `approver-cross-org.integration.test.ts`.
`filterApproversWhoCanRead` is wrapped by a pass-through spy declared at
the top of the harness (`vi.mock` of `./approver-org-scope.js`, every
other case runs the real filter through it): a static `position`
approver and an `expression` approver resolving as `position`, each
targeted at the group with `organization: '$root'`. Each reaches the
group CFO when the filter answers (control), and refuses with
`ApproverDirectoryUnreadableError` whose `failedRead` is exactly the
filter's sentence when the filter rejects, writing no request.
- `ownership-queue.test.ts`, new top-level `describe` "the queue reads
page to completion (#29604)", 5 cases. Four at 10,001 rows: a queue's
last member is expanded, is a member, and lists the queue in `queuesOf`;
an organization's 10,001st queue is listed; `queuesNaming` finds the
queue naming the recipient on row 10,001; a read failing on its second
page answers `[]` / `false` with one warning carrying the page's error.
The fifth: a full page whose last row has no id answers `[]`, and its
one warning carries the reason without the walk's "refuses instead"
clause. The test double learned `$gt` and `orderBy`, which the keyset
walk sends. The existing fixture rows gained ids, which a keyset walk
needs.
### Ablations, one per new refusal (`scripts/ablation-replace.mjs`, wrap
mode, under `os-verify-lock.sh`)
Every ablated test imports its subject by relative source path, so no
build sits between the mutation and the run. Each restore was proven
(`blob == HEAD`, `git diff HEAD` empty).
| # | mutation | red | which |
|---|---|---|---|
| A1 | static refusal limited to `queue` | 13 of 485 | the 8
team/position × policy cases, beside-a-person, fallback slate, and the
three driver-sql refusal pins |
| A2 | static refusal skips `queue` | 1 of 485 | the queue case |
| A3 | expression refusal off | 1 of 485 | `resolveAs: 'team'` on
driver-sql |
| A4 | decide's `'literal'` removed | 1 of 485 | the pre-snapshot decide
case |
| A5 | the three queue reads cut at 10,000 rows | 3 of 46 | member,
queue and naming past one page |
| A6 | a failed page answers with the pages read | 3 of 46 | the
second-page case, and two existing failed-read pins |
| B1 | the expression filter's refusal off | 1 of 482 | the expression
filter case |
| B2 | the static filter's failure rethrown raw | 1 of 482 | the static
filter case |
| B3 | the queue warning keeps the walk's closing clause | 1 of 47 | the
incomplete-walk case |
A1 to A6 ran on `98d62fd6c`; B1 to B3 on `c18905922`, the commit that
added them. B1 and B2 ran again on `1a3854d28`, after the pins moved
into the shared harness: each still reds exactly its one case (1 of
459).
### Gates, local, on head `c4c64fdee` (`origin/main` merged once, at
`98d62fd6c`)
`node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack` derived 62 families on the changed paths.
The whole workspace was built first (`turbo run build
--filter='!@objectstack/docs'`, 119 of 119 tasks), and each exit code
was written to a file before any pipe, then reconciled:
```
Run reconciliation — 62 derived, 62 run, 0 NOT-MEASURED, 0 UNRUN.
✓ dispatch-gates --ran: 62 derived famil(ies) accounted for — 62 run, 0 NOT-MEASURED (a DERIVED zero — all 62 recorded an exit code and none of them is 3).
```
All 62 exited 0. Also exit 0: the four artifact-roster gates the
derivation marks as keeping their roster under a changed directory
(`check:authz-resolver`, `check:changeset-fixed`,
`check:error-code-casing`, `check:filter-alias-parity`), `pnpm lint`,
`pnpm --filter @objectstack/plugin-approvals --filter
@objectstack/plugin-sharing typecheck` (the test-layer ledger included),
and both packages' full suites (plugin-sharing 1392 passed). The same 62
also exited 0 on `98d62fd6c`.
On `1a3854d28` (the cross-org pins moved into the shared harness, a
test-only change): plugin-approvals 1465 passed, its typecheck
(test-layer ledger included) exit 0, `node
scripts/check-engine-double-contract.mjs` exit 0, `pnpm
check:objectql-double-limit` exit 0, `pnpm lint` exit 0.
### Dogfood battery, once, on head `98d62fd6c` (the title carries `!`
and the declaration a narrowing arm)
Not re-run after the review rounds: since `98d62fd6c` the source changes
are the `expression` resolver's filter catch and the queue warning's
text; the rest is tests and the changeset.
The whole workspace built, then `pnpm --filter @objectstack/dogfood
test` under `os-verify-lock.sh`:
```
Test Files 1 failed | 418 passed | 3 skipped (422)
Tests 1 failed | 3491 passed | 36 skipped (3528)
os-verify-lock: VERDICT command-exit 1 · held the lock 1445s (24m05s)
```
The one failure is not this change's.
`test/permission-set-clone-door.dogfood.test.ts` › "a platform set (the
lock-and-clone rule's commonest source) copies into its shipping
package's pair" expects `200` from cloning `member_default` and receives
`422 INVALID_METADATA`: the clone grants on `sys_comment` and
`sys_activity`, which the permission validator says no package
registers. It fails the same way alone (1 failed | 10 passed), and fails
the same way with both of this PR's source files (`approval-service.ts`,
`ownership-queue-service.ts`) restored to `origin/main` `771b2db2` and
both packages rebuilt (the built `dist` then carries 0 occurrences of
`ApproverDirectoryUnreadableError` and 4 of `QUEUE_READ_LIMIT`). Neither
package touches permission sets or object registration. Reported to the
seat.
## Acceptance notes
- **Part 2 owes:** the queue service's own failed reads (it still
answers `[]` / `false` and logs once), the D7 grant recompute keeping
grants, and the D6 slate refusing on a queue whose member table fails
inside the service. All three need the call-time signal #30251 adds to
the contract. Part 1's queue refusal fires only when `expandMembers`
itself rejects.
- **Types the ruling does not name keep their answer:** `department` /
`business_unit` / `bu`, `org_membership_level` and `manager` (and
`resolveAs: 'department'`) still fall back as before on a failed read,
with the #29362 warning naming the failure. Pinned for `department` and
`org_membership_level`.
- **Activation-ledger read, decided:** `switchedOff` keeps answering
"switched on" when `sys_metadata_activation` cannot be read. On the D6
slate and the D7 grant pass that errs toward keeping people, never
toward dropping them, so it cannot shrink a slate or revoke a grant. On
the admission paths it can let a switched-off position confer access
during that outage; that posture predates this card and is not changed
here.
- **No error `code` field.** The refusal travels as a message prefix and
a class, as `NO_APPROVERS:` does. A `code` would enter
`check:dispatcher-error-vocabulary`, whose table lives in
`packages/runtime`, outside the claim.
- **Contract review 6650659075 (PASS, three suggested changes), all
taken:** the queue wording now names a queue approver whose expansion
rejects and says the service's own failed reads answer nobody until part
2; the `expression` resolver's cross-organization filter refuses as the
static one does, pinned on both; the queue warning no longer quotes the
walk's "refuses instead". Commits `c18905922`, `34e14c1ef`, `50deed3e8`,
and `c4c64fdee` for the engine double ESLint flagged (it declared
`update` / `delete` the node never sends and ignored `limit`).
- **Incremental review 6651366716 (PASS, two suggested changes), both
taken:** this body's `Clause-②:` line and summary use the changeset's
wording, and the cross-org pins live in the shared
`approval-service.test.ts` harness (`1a3854d28`).
- **Assignee and reviewers:** none set. The dispatch forbids GitHub MCP
writes, so the session's usual reviewer step is left to the seat.
## Verification
```
pnpm --filter @objectstack/plugin-approvals exec vitest run src/approval-service.test.ts -t "refuses the node"
pnpm --filter @objectstack/plugin-sharing exec vitest run src/ownership-queue.test.ts -t "page to completion"
```
Expect 18 and 5 passed.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Utp6CXqXpuVf14Mqo1pznh
<!-- signed: domain:server#4 · os-bill ·
session_01Utp6CZMkHvuDeVYgTD73qF -->
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 9afcd6a commit 13c0c5b
8 files changed
Lines changed: 595 additions & 67 deletions
File tree
- .changeset
- 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 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
14 | | - | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
15 | 17 | | |
| 18 | + | |
16 | 19 | | |
17 | 20 | | |
18 | 21 | | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
19 | 29 | | |
20 | 30 | | |
21 | 31 | | |
| |||
4207 | 4217 | | |
4208 | 4218 | | |
4209 | 4219 | | |
| 4220 | + | |
| 4221 | + | |
| 4222 | + | |
| 4223 | + | |
| 4224 | + | |
| 4225 | + | |
| 4226 | + | |
| 4227 | + | |
| 4228 | + | |
| 4229 | + | |
| 4230 | + | |
| 4231 | + | |
| 4232 | + | |
| 4233 | + | |
| 4234 | + | |
| 4235 | + | |
| 4236 | + | |
| 4237 | + | |
| 4238 | + | |
| 4239 | + | |
| 4240 | + | |
| 4241 | + | |
| 4242 | + | |
| 4243 | + | |
| 4244 | + | |
| 4245 | + | |
| 4246 | + | |
| 4247 | + | |
| 4248 | + | |
| 4249 | + | |
| 4250 | + | |
| 4251 | + | |
| 4252 | + | |
| 4253 | + | |
| 4254 | + | |
| 4255 | + | |
| 4256 | + | |
| 4257 | + | |
| 4258 | + | |
| 4259 | + | |
| 4260 | + | |
| 4261 | + | |
| 4262 | + | |
| 4263 | + | |
| 4264 | + | |
| 4265 | + | |
| 4266 | + | |
| 4267 | + | |
| 4268 | + | |
| 4269 | + | |
| 4270 | + | |
| 4271 | + | |
| 4272 | + | |
| 4273 | + | |
| 4274 | + | |
| 4275 | + | |
| 4276 | + | |
| 4277 | + | |
| 4278 | + | |
| 4279 | + | |
| 4280 | + | |
| 4281 | + | |
| 4282 | + | |
| 4283 | + | |
| 4284 | + | |
| 4285 | + | |
| 4286 | + | |
| 4287 | + | |
| 4288 | + | |
| 4289 | + | |
| 4290 | + | |
| 4291 | + | |
| 4292 | + | |
| 4293 | + | |
| 4294 | + | |
| 4295 | + | |
| 4296 | + | |
| 4297 | + | |
| 4298 | + | |
| 4299 | + | |
| 4300 | + | |
| 4301 | + | |
| 4302 | + | |
| 4303 | + | |
| 4304 | + | |
| 4305 | + | |
| 4306 | + | |
| 4307 | + | |
| 4308 | + | |
| 4309 | + | |
| 4310 | + | |
| 4311 | + | |
| 4312 | + | |
| 4313 | + | |
| 4314 | + | |
| 4315 | + | |
| 4316 | + | |
| 4317 | + | |
| 4318 | + | |
| 4319 | + | |
| 4320 | + | |
| 4321 | + | |
| 4322 | + | |
| 4323 | + | |
| 4324 | + | |
| 4325 | + | |
| 4326 | + | |
| 4327 | + | |
| 4328 | + | |
| 4329 | + | |
| 4330 | + | |
| 4331 | + | |
| 4332 | + | |
| 4333 | + | |
| 4334 | + | |
| 4335 | + | |
| 4336 | + | |
| 4337 | + | |
| 4338 | + | |
| 4339 | + | |
| 4340 | + | |
| 4341 | + | |
| 4342 | + | |
| 4343 | + | |
| 4344 | + | |
| 4345 | + | |
| 4346 | + | |
| 4347 | + | |
| 4348 | + | |
| 4349 | + | |
| 4350 | + | |
| 4351 | + | |
| 4352 | + | |
| 4353 | + | |
| 4354 | + | |
| 4355 | + | |
| 4356 | + | |
| 4357 | + | |
| 4358 | + | |
| 4359 | + | |
| 4360 | + | |
| 4361 | + | |
| 4362 | + | |
| 4363 | + | |
| 4364 | + | |
| 4365 | + | |
| 4366 | + | |
| 4367 | + | |
| 4368 | + | |
| 4369 | + | |
| 4370 | + | |
| 4371 | + | |
| 4372 | + | |
| 4373 | + | |
| 4374 | + | |
| 4375 | + | |
| 4376 | + | |
| 4377 | + | |
| 4378 | + | |
| 4379 | + | |
| 4380 | + | |
| 4381 | + | |
| 4382 | + | |
| 4383 | + | |
| 4384 | + | |
| 4385 | + | |
| 4386 | + | |
| 4387 | + | |
| 4388 | + | |
| 4389 | + | |
| 4390 | + | |
| 4391 | + | |
| 4392 | + | |
| 4393 | + | |
| 4394 | + | |
| 4395 | + | |
| 4396 | + | |
| 4397 | + | |
| 4398 | + | |
| 4399 | + | |
| 4400 | + | |
| 4401 | + | |
| 4402 | + | |
| 4403 | + | |
| 4404 | + | |
| 4405 | + | |
| 4406 | + | |
| 4407 | + | |
| 4408 | + | |
| 4409 | + | |
| 4410 | + | |
| 4411 | + | |
| 4412 | + | |
| 4413 | + | |
| 4414 | + | |
| 4415 | + | |
| 4416 | + | |
| 4417 | + | |
| 4418 | + | |
4210 | 4419 | | |
4211 | 4420 | | |
4212 | 4421 | | |
| |||
0 commit comments