fix(approvals): screen the manager approver to the request's organization (#10153)

[os-warren]

Fixes#10153

⚠️ Read this first: Clause-②: yes, developed one tier below the gate

This card moves one input from accepted to refused, and it was developed at claude-opus-5 under the maintainer's 2026-08-20 authorization ("while fable is exhausted, Clause-② cards may be developed at claude-opus-5 and wait for triage review before landing"). The compensating control is that triage reviews before merge — do not flip this ready, do not arm auto-merge.

The flip, measured on this branch, both directions on one tree in one run:

probe	node shape	before	after
C-a	sole approver = cross-org manager, onEmptyApprovers: 'fail'	opens, status pending, approvers ["u_mgr_b"]	throws NO_APPROVERS
C-a2	the same node, default policy (admin_rescue)	opens, ["u_mgr_b"]	still opens, ["manager:owner_id"]
C-b	a screened sibling (position) in the identical shape, 'fail'	throws NO_APPROVERS	throws NO_APPROVERS (unchanged)

Which callers are affected: only flows authored with the non-defaultonEmptyApprovers: 'fail' whose slate ends up holding no concrete person. The default admin_rescue still opens the request (decidable by a privileged admin, with the existing loud warning), and auto_approve still auto-approves. Nothing new is thrown: a screened-out manager leaves only a type:value literal, the pre-existing empty-slate test in approval-service.ts already classifies that as empty, and 'fail' already threw on empty. Every org-screened sibling has reached that same bucket since it was written — this change only routes one more input into it.

Both limbs are pinned (C-a, C-a2), so the flip is a fact in the suite rather than a claim in this description.

The defect

expandApprovers hands the directory organization to every graph-shaped approver expansion — department, position, org_membership_level. The manager branch did not: lookupManager read sys_user.manager_id under a system context and took no organization argument at all. sys_user is a global identity table with no organization_id, so nothing else on that path supplied the tenancy fact either. A manager_id crossing an organization boundary routed the submission to an approver in another organization.

What the screen is

lookupManager(userId, organizationId) resolves the manager and then asks whether he is provably outside the request's organization, via a sys_member membership test:

membership rows for the manager	result
some exist, none in the request's org	screened out — falls through to the manager:owner_id-style literal, exactly where an unstaffed position lands
one is in the request's org	resolves, unchanged
none exist at all	resolves, unchanged — the tenancy fact is absent, not negative
the sys_member read threw	resolves, unchanged
the request carries no organization	resolves, unchanged, and no read is performed

"Provably" is deliberate, not a weaker version of "must prove membership". The fail-open half is this file's already-ruled posture on addressing paths, stated twice before this PR: filterApproversWhoCanRead refuses to empty a live slate on an infrastructure hiccup, and expandPositionUsers carries "a step routing to nobody is worse than one routing to a lapsed holder". It is also load-bearing in practice — a stack that stamps an organization on its requests but never materializes sys_member rows would otherwise lose every manager approver at once, a far bigger behaviour change than the hole being closed. Measured: this repo's own type:manager out-of-office fixture in approval-service.test.ts is such a stack (its CTX carries tenantId: 't1' and seeds no sys_member), and examples/app-showcase's by_manager demo node would be too.

A drop is logged with the manager's id, the organizations he does hold, and the request's, so the remedy ("repair the link" / "grant the membership" / "retarget the step") is legible without a debugger.

Why manager can be screened alone even though spec puts it in one class with team

This has to be argued rather than assumed, because #10153's stated warrant does not survive contact with the source — round 1 measured that and filed it as #10230 / #10231. "Every sibling approver expansion is org-scoped" is false: team is not, and packages/spec/src/automation/approval.zod.ts declares team: falsenext tomanager: false in APPROVER_ORG_SCOPED. So manager and team sit in one declared class today, and screening manager alone splits it.

The split is defensible on a fact the two do not share:

• sys_team carries an organization_id. A team id therefore transitively names exactly one organization — an author who picks a team has already picked an org, and the id itself is the tenancy fact.
• sys_user.manager_id names a row in a global identity table with no tenancy fact at all. There is nothing in the id, nothing on the row, and nothing on the path.

That asymmetry is why manager is the sharper hole and why it can be closed on its own. team is #10230's card — it touches this same file and is serialized behind this one. The W pin in the test file asserts team is still unscreened, so this PR's non-treatment of it is recorded rather than left ambiguous.

APPROVER_ORG_SCOPED is untouched. It answers ADR-0105 D9 retargetability (may an author write organization: on this type?), not screening, and manager: false remains correct — approver-cross-org.integration.test.ts still requires a organization declaration on manager to be refused, and it still is.

This does NOT decide #7497

The screen reads sys_member, which looks identical to the D2 read-visibility filter beside it (filterApproversWhoCanRead). The code says at length why it is the sibling treatment instead, because the next reader will otherwise conclude #7497 was settled here:

1. Two of the three org-scoped expansions already screen on exactly this column — expandMembershipTierUsers filters sys_member.organization_id outright, and it is also the second limb of expandPositionUsers. sys_member.organization_id is already this file's answer to "which organization is this person in", independently of what they may read.
2. sys_user offers no other tenancy fact, so there is no other read this screen could have been written with.

No reads are granted and no read screen is applied to any type that lacked one. I did not hit the #7497 fork.

Verification

Ten pins in manager-approver-org-screen.test.ts, which is round 1's measurement harness inverted exactly as its own header instructed (A and C-a flipped; B/B2/C-b/W unchanged, which is what makes the inversion readable — one screened sibling next to the newly screened one, same tree, same run).

[PROBE A] request org = org_a pending_approvers = ["manager:owner_id"] was ["u_mgr_b"]
[PROBE A2] pending_approvers = ["u_mgr_b"] same-org manager STILL resolves
[PROBE A3] pending_approvers = ["u_mgr_b"] no membership anywhere: unchanged
[PROBE A4] request org = null pending_approvers = ["u_mgr_b"] no org: unchanged
[PROBE B] pending_approvers = ["position:cfo"] sibling screen, live
[PROBE B2] pending_approvers = ["u_pos_a"] sibling screen is not reject-everything
[PROBE C-a] threw = NO_APPROVERS: ... THE FLIP
[PROBE C-a2] status = pending approvers = ["manager:owner_id"] default policy still opens
[PROBE C-b] threw = NO_APPROVERS: ... unchanged
[PROBE W] org_a request, org_b team resolves ["u_team_b"] #10230, deliberately untouched

A2 is the second direction: without it a screen that rejected everything would pass A.

No ADR-0112 envelope is minted here. The NO_APPROVERS throw is pre-existing code and a bare Error, so there is no code / status to assert, and inventing one would be a fiction. C-a asserts the message prefix.

Ablation

Predicted signature, stated before running: neuter managerIsProvablyOutsideOrg and A, C-a, C-a2 must fail while A2/A3/A4/B/B2/C-b/W stay green — 3 failed / 7 passed. Observed exactly that:

x A expected [ 'u_mgr_b' ] to deeply equal [ 'manager:owner_id' ]
x C-a expected null to be truthy
x C-a2 expected [ 'u_mgr_b' ] to deeply equal [ 'manager:owner_id' ]
Tests 3 failed | 7 passed (10)

Rebuild statement, argued from the files: none was required, and that is provable rather than assumed. The subject is imported relatively (import { ApprovalService } from './approval-service.js'), which vitest resolves to this package's src/, not its dist/. check-test-source-alias.mjs's KNOWN_UNALIASED_TEST_IMPORTS lists only cross-package specifiers for @objectstack/plugin-approvals (@objectstack/objectql and friends — used here for the fake engine's dispatch assertions, which the ablation does not touch). The observation is self-validating in the safe direction: a dist-resolved test would have stayed green through a guard-removing mutation — the classic false green — and this one went red without any build, which is the proof the source is what ran.

Restore is byte-identical, proved both sides, and the restore leg was re-run, not assumed:

hash before ablation = f2fc42771870a507bf5334b048ea7b0aa40c3e41
hash ablated = 7cb0defc3291c9f19547fd6fa9f3a2bfeb80f1e8
hash restored = f2fc42771870a507bf5334b048ea7b0aa40c3e41
(identical to `git rev-parse d0602162d:` + the file path)
ABLATION markers remaining = 0
restore leg: Tests 10 passed (10)

Suites and gates — all on the final commit 5f538cdf2, exit codes captured before any pipe

• pnpm --filter '@objectstack/plugin-approvals^...' build — dependency closure built first (os-verify-lock: VERDICT command-exit 0).
• pnpm --filter @objectstack/plugin-approvals test — Test Files 25 passed (25) / Tests 507 passed (507) (VERDICT command-exit 0).
• pnpm --filter @objectstack/plugin-approvals typecheck — EXIT=0, script name echoed (tsc --noEmit), not a zero-match.
• Gate union re-derived with node scripts/pm/dispatch-gates.mjs, no paths passed — it derived the 4 changed paths from the merge base itself. Every listed family plus the convention-triggered ones ran green, each quoted from the gate's own verdict line: slot-lookup"ratchet holds: 107 unswept site(s) in 25 file(s), none new" · engine-double-contract"OK — 333 pinned, 133 in the DEBT ledger, 2 exempt" · where-matcher"265 matcher(s) discovered, 265 answer the combinator battery correctly or refuse it loudly" · query-options-erasure"ratchet holds: 67 unswept non-test site(s) in 17 file(s), none new" · test-source-alias"OK — 72 packages with tests scanned" · type-source-resolution"OK — 76 packages with a tsconfig.json scanned" · nul-bytes"OK (scanned 6091 text file(s) ... no raw ASCII control bytes)" · i18n"OK (9 package(s) — all bundles in sync)" · type-check-coverage"OK — 64/77 workspace packages type-checked" · plus changeset-gate-self-tests, objectui-changeset, cross-package-test-inputs, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check-affected-docs, all EXIT=0.
• pnpm check:type-check-debt --re-measure (whole workspace built first) — EXIT=0, "33 ledger entr(ies) re-measured in 308.2s, 1924 raw tsc error(s) total, none above its recorded number. surplus: none". Round 1 had to declare this one un-run; it is run here.

Two gate findings were fixed in-branch rather than routed around, and both are worth a reviewer's eye:

1. query-options-erasure went red — the new sys_member read had picked up the file's habitual as any, and the file is grandfathered for its existing erasures only. The read now carries no assertion (ApprovalEngine.find accepts it as written); the count is back to its baseline 67.
2. type-check-debt --re-measure went red at +21 — plugin-approvals excludes **/*.test.ts from its tsconfig.json, so pnpm --filter ... typecheck reported exit 0 over a pin file carrying 21 raw TS2339/TS18048, all from reading pending_approvers straight off the ApprovalRequestRow / ApprovalNodeAutoOutcome union. Narrowed through an opened() helper that refuses the auto-approval arm rather than casting past it. Re-measured: the package is back to its frozen 348, contributing 0. ⛔ No ledger entry was raised.

Out of scope, filed

• [finding] ApprovalRequestRow declares no organization_id, but openNodeRequest stamps one on the row it writes and returns #10331 — ApprovalRequestRow (the published contract) declares no organization_id, though openNodeRequest stamps one on the row it writes and returns. Surfaced by the hidden-layer typecheck above; the pins read the stamp off the persisted row instead, which is a workaround.
• approvals: the team approver expansion is not organization-screened either — a cross-org team resolves into a request's approver slate #10230 — team is not org-screened either. Not addressed here; out of scope for this card.
• [finding] plugin-sharing: TeamGraphService.managerOf discards its organizationId argument — the manager-chain screen cited as this repo's governed side is not in this repo #10231 — TeamGraphService.managerOf discards its organizationId argument. Not addressed here.

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

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

29 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 75e9301372987d99b458d168d88d926bf09b9003.

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

What this run could not see

• the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 5 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 75e9301372987d99b458d168d88d926bf09b9003 → packageMentionDocs.

Which tree this was computed on

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

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