Skip to content

fix(approvals): screen expanded team members to the request's organization - #10739

Merged
os-warren merged 3 commits into
mainfrom
claude/issue-10547-team-member-org-screen
Aug 21, 2026
Merged

fix(approvals): screen expanded team members to the request's organization#10739
os-warren merged 3 commits into
mainfrom
claude/issue-10547-team-member-org-screen

Conversation

@os-warren

@os-warrenos-warren commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Fixes#10547

Reachability first — the card asked to be verified before it was believed

#10547 filed itself as a code reading, not a probe, and triage made the
measurement the first deliverable. Measured on this tree, one fixture, before
any change (origin/main blob b7a5aed8):

[PROBE M1] org_a request, org_a team, org_b-only member -> pending_approvers = ["u_outsider"]
[PROBE M6] mixed team -> pending_approvers = ["u_outsider","u_insider"]
[PROBE M8] 6-member team -> sys_member reads = 0
[PROBE C-b] threw = NOTHING

The fixture is the one the card describes: sys_team.organization_id = org_a
(so it passes #10230's screen) and one sys_team_member row naming a user
whose only sys_member row is in org_b. He entered pending_approvers, and
sys_member was never read at all. The hole is reachable — the premise
holds.

The two unreachability hypotheses the card raised, checked rather than assumed

  • better-auth's remove-member cascade — real, and it does close the
    first-named route.
    deleteMember (better-auth 1.7.1,
    plugins/organization/adapter.mjs:305) deletes the user's teamMember rows,
    and ObjectStack enables the feature it is gated on (teams: { enabled: true },
    plugin-auth/src/auth-manager.ts:2249). A member removed from the
    organization through that endpoint is taken off that organization's teams.
  • But the cascade is org-scoped, and the shape survives outside it. It
    enumerates the teams whose organizationId equals the organization the
    member was removed from, and deletes only within those teams. Routes it does
    not cover, all still live:
  • RLS is not a mitigation here: the expansion reads under SYSTEM_CTX.

The fix

expandTeamUsers now screens the expanded members with the provably-outside
(fail-open)
posture managerIsProvablyOutsideOrg and
teamIsProvablyOutsideOrg already pin, in one $in read for the whole
slate:

  • membership rows exist and none is the request's organization (present and
    negative) — dropped, loudly;
  • no rows, an unreadable sys_member, a possibly-truncated read, or a request
    carrying no organization (absent) — routing unchanged; the last case
    performs no read at all.

Holding membership elsewhere is not disqualifying; holding none here is.

A truncated read fails open deliberately: this read is the only evidence
that a member is a tenant here, so an incomplete result must not be spent as
proof of absence — that would invert the posture into fail-closed exactly where
it must not. #3807 is the recorded cost of reading an absent fact as a negative
one (null on a platform object means "owned by no organization", which is what
a seed writes).

Both call sites are covered, because both route through expandTeamUsers: the
static team branch (screened against the request org) and the
expression / resolveAs: 'team' branch (screened against directoryOrg, the
directory actually being consulted) — each against the same organization whose
tenancy the team itself just proved.

#7497 is not ruled on here. The change grants no reads and applies no read
screen to any approver type that lacks one today — the same reading #10153
recorded for its own screen. #10230 landed under this posture without waiting on
#7497, and so does this.

Pins — two directions, because one direction is not a test

PinDirection
M1member provably outside is screened out
M2member of this org still routes
M3nosys_member row at all still routes (absent fact, #3807)
M4unreadable sys_member still routes (fail-open on a fault)
M5request with no organization still routes, and reads nothing
M6mixed team: outsider dropped and insider kept, one expansion
M7member of this org and another still routes
M8one read for the whole slate, not one per person
M9the drop is loud — users, both organizations, the card
M10a truncated read fails open
E1/E2both directions through the expression path
C-b / C-b2the flip, and its confinement to one non-default policy

M6 is the anti-"screen everyone" pin in a single assertion: an implementation
that dropped every member yields ['team:team_a'], an unscreened one yields
both users, and only the correct one yields exactly the insider. Pinning M1
alone would sit green over the first of those — which is what PR #10546's
ablation leg B demonstrated on the sibling card.

Ablation — predicted before it was run, both legs, hash-proved

Predicted signature, written before mutating: reverting approval-service.ts
to origin/main should turn the screen pins red (M1, M6, M8, M9, M10, E1,
C-b, C-b2) while leaving every absent-fact limb green (M2, M3, M4, M5, M7,
E2), because those limbs assert behaviour the fix does not change.

Observed, exactly that: 8 failed | 6 passed (14) on the reverted tree,
14 passed (14) restored. The six that pass in both states are the
fail-open limbs, which is what makes them evidence rather than decoration.

Restore proved byte-identical, not asserted:

blob at base 72d75ebb5 b7a5aed819fd9f546b9a9838d467d7c7d6075fd9
BEFORE_HASH (mutation) b7a5aed819fd9f546b9a9838d467d7c7d6075fd9
AFTER_HASH (restore) f7c52f891022bc04efbc76cb30e1aa8af5e4f90c = committed blob

Resolution — measured, not assumed:packages/plugins/plugin-approvals/dist
does not exist, and plugin-approvals was never rebuilt between the two
legs, yet editing src/approval-service.ts alone flipped 8 tests red and back
to green. The suite therefore resolves the subject through src/ (the relative
./approval-service.js import), so the ablation measured the tree it claims to.

Release condition

The screen can only act on tenancy facts that exist. A deployment that stamps
an organization on its approval requests but does not materialize sys_member
rows sees no behaviour change at all
— by design, and it is the same residual
#10230 shipped with (teams seeded organization_id: null keep today's routing).
This is a condition on the fix's security value, not a footnote: the hole is
closed for stacks that record membership, and unchanged for stacks that do not.

Behaviour change

Confined to one non-default policy. A node whose only approver is a team staffed
entirely by users provably outside the organization now resolves to nobody:
under the default onEmptyApprovers: 'admin_rescue' it still opens (routed to
the dead team: plus the team id literal, as any unresolved slate is); under
onEmptyApprovers: 'fail' it now throws NO_APPROVERS where it previously
opened. That throw is pre-existing code and a bare Error, not a minted
ADR-0112 envelope, so C-b asserts the message shape and invents no code /
status.

Verification

Gate union derived with node scripts/pm/dispatch-gates.mjs, no path
arguments
, after the final commit on a clean tree, at a5c2b69ae. Exit codes
captured before any pipe.

Green: package typecheck, package test (520 passed),
check:changeset-gate-self-tests, check:objectui-changeset,
check:slot-lookup, check:test-source-alias, check:type-source-resolution,
check:cross-package-test-inputs, check-adr-0087-registration,
check-changeset-no-major, check-empty-changeset, check-affected-docs,
check:query-options-erasure, check:type-check-coverage,
check:engine-double-contract, check:where-matcher, check:i18n,
check:nul-bytes, check:route-envelope --self-test,
check:dispatcher-error-vocabulary --self-test.

Two gates went red and were fixed rather than waved through:

  • check:engine-double-contract"RETAINED [delete] / [update]: … pins 1
    engine double(s) that the pinned ledger does not record"
    . Regenerated with
    --write (2 rows added, 0 lost), committed as a5c2b69ae.
  • check:i18n"PREREQUISITE NOT MET — the workspace CLI is not built …
    Nothing was checked"
    . That is a not-measured, not a verdict. Built
    @objectstack/cli and re-ran: green.

Class #10309, fourth consecutive time in this lane: the derivation did
not name check:route-envelope or check:dispatcher-error-vocabulary. Both
were added by hand and both are green. Committing the ledger fix also changed
the answer: re-deriving on the final diff pulled in
check:cross-package-test-inputs (green — "13 package(s) read outside
themselves, all declared"
), which the first derivation could not have named.
The union is a function of the final diff, not the initial one, which is why
it is derived after the last commit rather than before the first.

Not done

check:type-check-debt --re-measure is not run here — it requires a full
workspace build, and the shared verify lock did not yield one inside this
session's budget. Declared, not silently skipped. Its structural half
(check:type-check-coverage) is green, the package typecheck is green, and
plugin-approvals carries no test-typecheck debt ledger entry, so the expected
risk is low; CI runs the farm regardless.

@objectstack/plugin-auth's TEST_DEBT was not touched and --lower was never
run.


Generated by Claude Code

…ation (#10547)
#10230 made a `team` approver prove the TEAM's tenancy and deferred its
members on purpose. `sys_team_member` carries `team_id` and `user_id` and no
organization column, so a team that passed that screen still routed every
user id it listed.
Measured on a fixture before the change, not read off the schema — an `org_a`
request against an `org_a` team whose member holds membership only in `org_b`:
[PROBE M1] pending_approvers = ["u_outsider"]
[PROBE M8] sys_member reads = 0
The expansion now screens the members with the provably-outside (fail-open)
posture `managerIsProvablyOutsideOrg` and `teamIsProvablyOutsideOrg` already
pin, in ONE `$in` read for the whole slate:
- membership rows exist and none is the request's organization
=> present and NEGATIVE => dropped, loudly;
- no rows, an unreadable table, a possibly-truncated read, or a request
carrying no organization
=> ABSENT => routing unchanged, and the last case reads nothing.
A truncated read fails OPEN deliberately: this read is the only evidence that
a member IS a tenant here, so incomplete evidence must not be spent as proof
of absence. #3807 is the recorded cost of reading an absent fact as a
negative one.
Decides nothing #7497 asks: no reads are granted and no read screen is applied
to any approver type that lacks one today.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0f14f70b-575c-5f2b-a235-4000a55db042
`check:engine-double-contract` reported the new
`team-member-org-screen.test.ts` double as RETAINED — pinned coverage the
ledger did not yet record, so it protected nothing. Regenerated with
`--write`: 2 rows added (one delete, one update), none lost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0f14f70b-575c-5f2b-a235-4000a55db042
@github-actions

github-actionsBot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

📓 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 5f2e54cc66330cbc53a17f6e3746acdfcdc14704.

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 5f2e54cc66330cbc53a17f6e3746acdfcdc14704packageMentionDocs.

Which tree this was computed on

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

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

@os-warren
os-warren marked this pull request as ready for review August 21, 2026 14:25
@os-warren
os-warren added this pull request to the merge queueAug 21, 2026
Merged via the queue into main with commit c5d0c2fAug 21, 2026
32 checks passed
@os-warren
os-warren deleted the claude/issue-10547-team-member-org-screen branch August 21, 2026 14:40
@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32492087926 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Console Pin Gate — 失败步骤: Build the Console SPA at the pinned objectui SHA

    ✗ Build failed in 6.52s
    

↳ 失败原因 是判读的关键:超时Test timed out in … / Hook timed out in …)多半是负载/时序,不是本 PR 的回归;
断言AssertionError: …)才指向真实的行为改变。两者的 FAIL 行长得一模一样,只有这一行能区分。

跨 PR 相同签名(24h,按失败测试文件聚合):

  • ⚠️本次没有可用的聚合签名(日志里没有能解析出测试文件名的 FAIL 行)—— 这不是「没有同签名的其他 PR」,是这一轮没测到。跨 PR 聚合本次不可用,请手工比对其他 PR 的同类评论。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 15 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 看上面的「跨 PR 相同签名」;已有汇总 issue ⇒ flaky/环境问题实锤,去那张 issue 上谈,修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

os-warren pushed a commit that referenced this pull request Aug 21, 2026
…sink
Drops the `?` from `warn` on ApprovalServiceOptions['logger'] so a sink
declaring an optional `error` always has a guaranteed durability-report
channel (#9754, #10556). `error` stays optional. This is the thirteenth of
the card's thirteen mechanical repairs -- held out of #10691 to serialize
against PR #10547, which owned this file while open; that fence cleared
when #10739 merged.
Deletes the now-stale row from the shrink-only
scripts/optional-error-sink-contract.baseline.json (the checker itself
named the row as no-longer-red). The other two rows -- plugin-security's
default-sink design call and service-settings' no-fallback design call --
are untouched; both remain escalated to the maintainer.
No construction site needed a widening fix: the sole non-test call
(ApprovalsServicePlugin.start) passes ctx.logger, whose warn is already
required by the core Logger contract.
Part of #10556.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0f14f70b-575c-5f2b-a235-4000a55db042
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

approvals: sys_team_member carries no tenancy fact — a same-org team still routes to a member who holds no membership in the request's organization

2 participants

@os-warren@claude