Skip to content

fix(plugin-approvals): a user approver naming no account expands to nobody and takes onEmptyApprovers - #27749

Merged
objectstack-fleet[bot] merged 7 commits into
mainfrom
claude/issue-27397-user-approver-unresolvable
Oct 29, 2026
Merged

objectstack-fleet[bot] merged 7 commits into
mainfrom
claude/issue-27397-user-approver-unresolvable

Conversation

@objectstack-fleet

@objectstack-fleet objectstack-fleet Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

domain:server#2 · os-sales · session_01L6bhgUCizoYWQVJWjhdR6F

Requested by Sales · project thread
Fixes #27397

Clause-②: no
Metadata-protocol: none
Skills-docs: none — no skills/** text describes a user approver's resolution; the one docs page that did (content/docs/automation/approvals.mdx, two sentences that this change makes false) is corrected here

维护者速读

  • 改了什么:user 类型审批人现在会先查目录(sys_user,按 id 或邮箱)。查不到这个账号时,它和部门、团队、岗位这些图类型一样:打同一条 expanded to nobody 警告,不占槽位。名单因此变空时,走节点的 onEmptyApprovers,默认是 admin_rescue。
  • 为什么改:以前写错的用户 id,或者已经删掉的账号,会作为一个谁都处理不了的幽灵槽位开出审批单。没有日志,也没有转交管理员,这张单子会永远挂着。
  • 风险与回滚:低。只有 sys_user 读成功、而且确实没有这一行时才丢槽位:读失败时保留原槽位;被封禁的账号、别的组织目录里的人、以及 OOO 委托到真实代理人的情况都不变。回滚就是 revert 这个 PR。
  • 先看:packages/plugins/plugin-approvals/src/approval-service.ts 的 resolveApproverSpec 里 type === 'user' 分支和共用的警告分支;admin-rescue-named-slate.integration.test.ts 新增的 describe;approval-service.test.ts 的 ROUTED_USERS 与 putAccounts(夹具改动)。
  • 两分钟验证:pnpm --filter @objectstack/plugin-approvals exec vitest run src/admin-rescue-named-slate.integration.test.ts,预期 22/22 通过,其中包含 "a user approver naming no user expands to nobody" 这一组。
  • 需要你做的:无,席位落地。

What changes

  • The user approver branch in resolveApproverSpec. A user value is now asked of the directory through the predicate the reassign door already uses (reassignTargetRefusal). That predicate looks for a sys_user row carrying the id, or the email for an email-shaped value. It is asked with no organization, so its tenancy screen does not run: user is directory-less, and ADR-0105 D9 leaves it untouched (the existing approver-cross-org pin holds that). When no account carries the value, the branch falls through to the graph types' existing warning (same text, now reached for user too) and returns no slot. A slate it leaves empty reaches the node's onEmptyApprovers by the existing path; there is no new branch and no new warning text.
  • What counts as "no such user": a successful sys_user read that returns no row. These cases still keep the slot:
    • a banned account. The team and department expansions do not read the flag either; I measured this on expandTeamUsers and expandBusinessUnitUsers.
    • a failed read. A failed read proves nothing, which is this file's fail-open posture on addressing paths. A new unit pin covers it.
    • a value spelled as a TYPE:VALUE literal. slateNamesAPerson already reads that as naming no one.
  • Checked after OOO delegation: the check asks about the slot the route ends on. A removed account whose out-of-office delegation names a live delegate still routes to the delegate (the existing plugin-approvals: the out-of-office notifications name the absent approver and the delegate by raw sys_user id ("… while u_ooo is out of office", "… was routed to u_bob …") — the person half of the approval-notification wording #24065 notification-body pin).
  • A mixed slate (one real user plus one phantom) keeps the real one, drops only the phantom, and logs the warning once for the phantom. That is the same per-entry warning a graph entry beside a person logs.
  • Docs: content/docs/automation/approvals.mdx said that a user entry never resolves to nobody. Two sentences are corrected.
  • Fixtures: the unit doubles started with an empty sys_user, so every user approver in them was a phantom by the new rule. They now start with the accounts each suite routes to (ROUTED_USERS, plus putAccounts where a test writes a named row). Three dogfood fixtures used an address deliberately carried by no account: CO_APPROVER (two of them) and DEPARTED_APPROVER. Each pin now signs that address up as an account that never acts, so the scene each pin measures is unchanged. The test-typecheck ledger shrinks by two, because a replaced test took two pre-existing errors with it.

Tests

The PIN is through the real ApprovalService.openNodeRequest on a real ObjectQL plus SqlDriver, with real administrators. It is a new describe in admin-rescue-named-slate.integration.test.ts:

  • a user approver naming usr_does_not_exist takes the default admin_rescue route. The request opens on u_admin and u_owner, and they are told. approver 'user:usr_does_not_exist' expanded to nobody is logged once, and the routing line is logged once. pending_approvers holds no usr_does_not_exist, and admin_rescue:abandoned is [].
  • a mixed list [u_alice, usr_does_not_exist] opens on ['u_alice'] alone, with the phantom's warning logged once and no administrator routing.
  • an email that no account carries is dropped. An email that an account carries keeps its slot.
  • CONTROL: a real user keeps the slot, with no expanded-to-nobody line. A person of another organization's directory keeps the slot too (ADR-0105 D9).

The unit pins are in approval-service.test.ts. The old "stays quiet for user" test, which pinned the phantom slot, is replaced by three tests:

  • the warning and its meta, with no slot;
  • a control in which a real account stays quiet;
  • a failed sys_user read keeps the slot.
Red first, then green

Before the fix, the new describe at base a75ddba1 gave 4 failed and 17 passed (21):
expected [ 'usr_does_not_exist' ] to deeply equal [ 'u_admin', 'u_owner' ] · expected [ 'u_alice', 'usr_does_not_exist' ] to deeply equal [ 'u_alice' ] · the email leg · (and the other-org leg, which was later turned into the D9 control).

At final head 10a61cc4, with the closure rebuilt after the origin/main merge, the run under the verify lock gave:
pnpm --filter @objectstack/plugin-approvals exec vitest run: Test Files 105 passed (105), Tests 1338 passed (1338).
pnpm --filter @objectstack/plugin-approvals typecheck: exit 0, check:test-typecheck: OK (8 files / 321 errors / 27 pinned signatures).
pnpm --filter @objectstack/http-conformance exec vitest run src/hono-approvals-act.conformance.test.ts: 4/4. It covers an unregistered sys_user: the read throws and the slot is kept.

Ablations (scripts/ablation-replace.mjs; the suite reads src, so no dist leg)
  1. if (refusal === undefined) return routed; was replaced with return routed;, which removes the existence check. Result: 4 failed and 405 passed (409). The failures were the three integration legs and the unit warning pin, for example expected [ 'usr_does_not_exist' ] to deeply equal [ 'u_admin', 'u_owner' ]. The controls stayed green. The restore was checked: blob ae44df7ffede == HEAD, and git diff HEAD was empty.
  2. if (GRAPH_APPROVER_TYPES.has(type) || type === 'user') { was replaced with if (GRAPH_APPROVER_TYPES.has(type)) {, which removes the shared warning. Result: 3 failed and 406 passed (409): expected [] to have a length of 1 but got +0 (twice) and no warning for user: expected undefined to be truthy. The restore was checked: blob == HEAD, and git diff HEAD was empty.

Acceptance notes

  • Out of scope, and not fixed here: type: 'field' has the same unchecked shape. A record field holding a user id that no account carries opens a phantom slot in the same way (resolveApproverSpec, the field branch, csvSplit(record[value]) straight to applyOooDelegation). Not filed by this PR; it is named in the report for the seat.
  • Out of scope, and not fixed here: a delegation's delegate_id is not existence-checked either. An OOO hop to a removed account would open a phantom slot the same way. Noted, not filed.
  • examples/app-showcase approver-bindings.flow.ts carries { type: 'user', value: '' } on an obsolete flow. If that flow ever runs, it now logs approver 'user:' expanded to nobody, where before it was silent. The empty slot was already dropped before this change.
  • The dogfood battery does not run on pull requests (its if: excludes them), so the dogfood fixture edits are verified by the local run below, not by CI on this PR.

Verification

Gates on `10a61cc4`

node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack derived 94 command(s) on 10a61cc4. I ran every one at that head and captured each exit code before any pipe: 94/94 exit 0.

dispatch-gates --ran: Run reconciliation — 94 derived, 94 run, 0 NOT-MEASURED, 0 UNRUN · a DERIVED zero — all 94 recorded an exit code and none of them is 3.

I also ran these locally, outside the derived set:

  • eslint --no-inline-config over packages/plugins/plugin-approvals and the six dogfood files: exit 0.
  • the package test suite and typecheck (above).
  • the http-conformance approvals pin.
  • the nine approval dogfood files that open user approvers (override-composite, my-pending-position, position-address-readers, admin-rescue-slate, notification-body, pending-approver-record-read, record-lock-read, sla-reassign-target-resolves, seed-ownership-claim-dispatch). Run under the verify lock after building the dogfood closure: Test Files 9 passed (9), Tests 9 passed (9).

merge-tree against the three open PRs that touch packages/qa/dogfood/test/ (#27713, #27667, #27649) shows no conflict on any file this PR changes.


Generated by Claude Code

🤖 Generated with Claude Code

https://claude.ai/code/session_01L6bhgUCizoYWQVJWjhdR6F

…obody

A user approver's id is now checked against the directory with the one
predicate the reassign door already asks. One that names no account (or
names a user provably outside the organization) takes the graph types'
expanded-to-nobody warning and adds no slot, so a slate it leaves empty
reaches the node's onEmptyApprovers (default admin_rescue).

Claude-Session: https://claude.ai/code/session_01L6bhgUCizoYWQVJWjhdR6F
Co-authored-by: Claude <noreply@anthropic.com>
…nts their user approvers name

Claude-Session: https://claude.ai/code/session_01L6bhgUCizoYWQVJWjhdR6F
Co-authored-by: Claude <noreply@anthropic.com>
…counts that never act

Claude-Session: https://claude.ai/code/session_01L6bhgUCizoYWQVJWjhdR6F
Co-authored-by: Claude <noreply@anthropic.com>
… user it routes to

Claude-Session: https://claude.ai/code/session_01L6bhgUCizoYWQVJWjhdR6F
Co-authored-by: Claude <noreply@anthropic.com>
…e test-layer ledger shrinks by two

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

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-approvals, touching 2 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/plugins/plugin-approvals/test-typecheck-debt.json), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/automation/flows.mdx (via ApprovalService (symbol, a top-level class))

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

  • content/docs/releases/v17/17-6.mdx (via ApprovalService (symbol, a top-level class))
  • content/docs/releases/v18/18-1.mdx (via ApprovalService (symbol, a top-level class))

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

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/plugins/plugin-approvals/test-typecheck-debt.json) — pages documenting those are invisible to this run
  • the SDK route bridge reached 58 of 210 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 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; 97 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 — 6 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 81d00a4ed938eb896859630498fc59d91183cf71 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 3a4fb1f926754bf697d9493002c529ce2aabb8da — the merge of head 10a61cc4b370976aad5fce482d7873eeb8035a24 into base 81d00a4ed938eb896859630498fc59d91183cf71, 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 3a4fb1f926754bf697d9493002c529ce2aabb8da && git checkout 3a4fb1f926754bf697d9493002c529ce2aabb8da
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 81d00a4ed938eb896859630498fc59d91183cf71 10a61cc4b370976aad5fce482d7873eeb8035a24 && git checkout -B drift-repro 81d00a4ed938eb896859630498fc59d91183cf71 && git merge --no-ff 10a61cc4b370976aad5fce482d7873eeb8035a24

node scripts/docs-audit/affected-docs.mjs --json 81d00a4ed938eb896859630498fc59d91183cf71

⚠️ 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 81d00a4ed938eb896859630498fc59d91183cf71 → 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#2 · os-sales · session_01L6bhgUCizoYWQVJWjhdR6F

维护者速读(席位终稿)

  • 改了什么:审批节点里 { type: 'user', value } 的审批人,若其 id 或邮箱在 sys_user 里查无此人,不再开一个谁也处理不了的待办;它记一条与团队、部门等审批人相同的「expanded to nobody」警告,不占位;整个名单因此为空时走节点的 onEmptyApprovers(默认 admin_rescue,交给组织管理员)。
  • 为什么改:写错 id 或人员已删除时,审批请求会永远挂在一个幽灵位上,不进任何人的待办,也没有任何日志。
  • 风险与回滚:存在性判断复用改派入口已有的谓词,且不带组织(不做租户筛选,符合 ADR-0105 D9);被封禁账户、跨组织的人、外出委托到的代理人、sys_user 读取失败都照旧保留原位。三条 dogfood 用例改为先注册对应账户;文档两句改正。回滚 = revert 本 PR。
  • 先看:packages/plugins/plugin-approvals/src/approval-service.ts 中 resolveApproverSpec 的 user 分支。
  • 两分钟验证:pnpm --filter @objectstack/plugin-approvals exec vitest run src/admin-rescue-named-slate.integration.test.ts。
  • 需要你做的:无。非受管面,席位落地。

席位意见: ACCEPT。一个谓词、一条警告、复用已有的空名单路径;field 审批人与已删除代理人的同形缺口另开卡。

席位核对

ACCEPT · head 10a61cc4b370976aad5fce482d7873eeb8035a24 · 2026-10-29T22:43Z

  • 形态: 24 files, +307/−53: approval-service.ts +42/−8, 14 test files (fixtures now seed the accounts they name), 3 dogfood pins and fixtures, two approvals.mdx sentences, the test-typecheck ledger (shrank by 2), one changeset. No packages/spec, no governed path; Tier S.
  • Clause-②: no, Metadata-protocol: none, patch. No input is refused; an approver that names no account now routes through the node's existing onEmptyApprovers instead of a dead slot. No contract review owed.
  • 前提实测: admin-rescue-named-slate 4 of 21 red before the fix; ablation 1 (no existence check) 4 red, ablation 2 (no shared-warning widening) 3 red, both restored to the HEAD blob.
  • 抽查读数: the seat read the hunk: the check runs on the slot the out-of-office route ends on, skips a type:value literal, fails open on a thrown read, and calls reassignTargetRefusal(value, null) so no tenancy screen applies.
  • 门禁 check-runs: 54 on this head: 40 success, 14 skipped, 0 other (read 2026-10-29T22:43Z); mergeable_state: clean. Derived gates 94/94 exit 0, 0 NOT-MEASURED; plugin-approvals 1338/1338; nine approval dogfood files 9/9 (Dogfood Regression Gate does not run on PRs).
  • 接受的偏差: the wider file surface (stale fixtures, dogfood pins and two docs sentences this change made false), reassignTargetRefusal with no organization (ADR-0105 D9; the org screen reds two existing pins), the declared lock narrowing for intermediate runs (final-head runs under the lock), the report signature line, model-free trailers.
  • 范围外去向: type: 'field' ids and an out-of-office delegate that name no account → plugin-approvals: a field approver id, or an out-of-office delegate, that names no account still opens a slot no one can take #27752.
  • 散文面: changeset and approvals.mdx checked against the diff by the seat.
  • 落地条款: relay pr_ready + automerge_enable through the merge queue.

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

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

1 participant