Repository navigation
approvals: the manager approver resolves sys_user.manager_id with no organization screen, while every sibling approver expansion is org-scoped #10153
Description
Activity
Triage: lands in
packages/plugins/plugin-approvals/src/approval-service.ts⇒domain:services+security, type Bug,pm:queue.Grading rationale: of the two directions the filer declines to argue, direction 1 (screen
lookupManagerthe way every sibling expansion is screened) is the default under the standing meta-rule that when one column has two consumers behaving inconsistently, the governed side wins — the org screen has been the ruled treatment ofsys_user.manager_idsince cloud#1195, and approvals is an unscreened straggler of that same work, not a deliberately different contract. The "resolves to nobody" story already exists (any approver expansion can come back empty; the existing unresolved-approver path handles it). Direction 2 (authoring-time invariant only) is refuted by the card itself: pre-gate rows are the exact residual cloud#1369 reports.Scope fence for the dispatch: this restores the invariant for the
managerbranch only — it does NOT decide #7497 (whether approver routing implies read visibility), which stays an open design card; if implementation shows the screen requires answering #7497 first, stop and report the fork.
Generated by Claude Code
Claim: PM seat #6021 (
domain:services), sessionsession_01PnJHU45vPJj5UQrxe946Bx.- Branch:
claude/issue-10153-manager-approver-org-screen - Worktree:
/home/user/objectstack-issue-10153(⛔ 不在共享主检出上编辑;⛔ 禁git stash) - File surface:
packages/plugins/plugin-approvals/src/approval-service.ts+ 其测试 + changeset - Container & model:
claude-opus-5 - Clause-②: no —— 判据写明以便被推翻:本卡收紧的是内部 approver 展开的范围,不是端点的接受/拒绝契约;分诊亦指出"解析不到人"的既有路径本就存在(任何 approver 展开都可能返回空,未解析 approver 的处理路径已在)。
- Serial constraints: 与同批的 Attachment lifecycle bookkeeping ignores updates — a
file_idre-point orphans the oldsys_filewith no tombstone (retention leak) #10171(service-storage)、SettingsService accepts a write before its engine is bound and answers "resolved" while nothing reaches sys_setting — everykernel:readyhook registered frominit()is inside that window #10159(SettingsService)面不相交。
⚠️ 重定级绊线(写进派单):若实现过程中发现该改动会让某个调用者从"被接受"变成"被拒绝"(例如审批创建在无人可解析时由成功转为失败),⇒ 那就是 Clause-② yes,本车道当前没有claude-fable-5⇒ ⛔ 停下回报,不要继续实现。我按内容判为 no,但判据在上面,可被证伪。⚠️ 范围围栏(分诊原文,我照搬):本卡只恢复manager分支的不变式,不决定 #7497(approver routing 是否蕴含读可见性),那仍是一张开放设计卡。⛔ 若实现显示必须先回答 #7497 才能加这道 org 屏,停下报分叉。
Generated by Claude Code
- Branch:
{ "issue": 10153, "status": "blocked", "branch": "claude/issue-10153-manager-approver-org-screen", "pr": null, "premise_still_valid": true, "summary": "STOPPED AT THE RE-TIERING TRIPWIRE — no fix implemented, by instruction. The premise holds and I reproduced it on my branch: with an org_a request whose submitter's manager u_mgr_b holds a sys_member row ONLY in org_b, the `manager` approver resolves him into pending_approvers (['u_mgr_b']), while on the SAME tree and the same run the sibling `position` expansion drops a cross-org holder to the dead literal (['position:cfo']) and still resolves a same-org holder (['u_pos_a']). But the Clause-2 wire tripped: I measured that TODAY a node whose sole approver is a cross-org `manager` under onEmptyApprovers:'fail' OPENS successfully (status 'pending'), whereas a SCREENED type in the identical shape THROWS NO_APPROVERS — because approval-service.ts:1588 classifies a slate holding only `type:value` literals as EMPTY, and the 'fail' policy throws on it. Applying the screen therefore moves that input from accepted to refused, which is the dispatch's named example verbatim ('an approval submission that used to succeed now fails when no manager resolves'). Per the fence I stopped before writing the fix. SEPARATELY, the warrant does NOT hold as stated: `team` is a sibling graph expansion that is NOT org-scoped (expandTeamUsers takes no org argument and is the only graph type not wrapped in `bounded`), measured — an org_a request routed to an org_b team resolves ['u_team_b']; and the spec's own APPROVER_ORG_SCOPED table declares `team: false` alongside `manager: false`. I also could not find the 'governed side' the triage leans on anywhere in this repo: provablyDisjointOrgs / screenToTenant return zero hits (counter-checked against expandPositionUsers, 10 files), and this repo's own manager-chain reader TeamGraphService.managerOf(userId, _organizationId) discards its org argument — so within framework, approvals is not a straggler among screened peers. I did NOT hit the #7497 fork, but I flag that sys_user carries no organization_id, so any manager screen must be a sys_member membership test — the same read filterApproversWhoCanRead performs for read-visibility, putting the implementation one line from #7497's territory. The branch carries a measurement harness only (6 assertions pinning CURRENT behaviour, clearly marked to be inverted by the fix); no PR is open.", "tests": "All measurements on worktree /home/user/objectstack-issue-10153, branch head 13c7ac54b (merge base 04096f17e).\n\nHarness: packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts — `npx vitest run <file> --reporter=verbose --disable-console-intercept`, EXIT=0, 6/6 passed. Real stdout:\n [PROBE A] request org = org_a pending_approvers = [\"u_mgr_b\"] <- premise: cross-org manager RESOLVES\n [PROBE B] pending_approvers = [\"position:cfo\"] <- sibling screened, same tree\n [PROBE B2] pending_approvers = [\"u_pos_a\"] <- screen is not reject-everything\n [PROBE C-a] opened OK, status = pending approvers = [\"u_mgr_b\"] <- Clause-2: ACCEPTED today\n [PROBE C-b] threw = NO_APPROVERS: approval node 'approve_step' on opportunity/opp1 resolved to no concrete app... <- Clause-2: REFUSED when screened\n [PROBE W] org_a request, org_b team -> pending_approvers = [\"u_team_b\"] <- warrant falsified\n\nZero-grep counter-check (required by this lane): `grep -rn \"provablyDisjoint\"` and `grep -rn \"screenToTenant\"` over *.ts/*.mts/*.md both returned NO output; the same sweep for the neighbouring known-present term `expandPositionUsers` returned 10 files (spec/automation/approval.zod.ts, core/security/grant-validity.ts, plugin-sharing/position-graph.ts, plugin-approvals/approval-service.ts, ...), so the search works and the zeros are real.\n\nPackage suite: `pnpm --filter @objectstack/plugin-approvals test` under scripts/pm/os-verify-lock.sh — VERDICT command-exit 0; 'Test Files 25 passed (25) / Tests 503 passed (503)'. `pnpm --filter @objectstack/plugin-approvals typecheck` EXIT=0, script name echoed in the output (not a zero-match).\n\nDependency closure built first: `pnpm --filter '@objectstack/plugin-approvals^...' build` — os-verify-lock VERDICT command-exit 0.\n\nGate union re-derived on the FINAL commit 13c7ac54b with `node scripts/pm/dispatch-gates.mjs` (no paths passed; it derived 2 changed paths from the merge base itself). Exit codes captured with `EXIT=$?` on the command, never through a pipe; each verdict quoted from the gate's own line:\n check:engine-double-contract EXIT=0 'check-engine-double-contract: OK - 333 pinned, 133 in the DEBT ledger, 2 exempt.'\n check:where-matcher EXIT=0 'where-matcher conformance holds: 265 matcher(s) discovered, 265 answer the combinator battery correctly or refuse it loudly'\n check:test-source-alias EXIT=0 'check-test-source-alias OK - 72 packages with tests scanned'\n check:type-source-resolution EXIT=0 'check-type-source-resolution OK - 76 packages with a tsconfig.json scanned'\n check:slot-lookup EXIT=0 'slot-lookup ratchet holds: 107 unswept site(s) in 25 file(s), none new'\n check:nul-bytes EXIT=0 'check-nul-bytes: OK (scanned 6076 text file(s) ... no raw ASCII control bytes)'\n check:cross-package-test-inputs EXIT=0\n check:query-options-erasure EXIT=0 'query-options-erasure ratchet holds: 67 unswept non-test site(s) in 17 file(s), none new'\n check:i18n EXIT=0 'check-i18n-bundles: OK (9 package(s) - all bundles in sync, no undeclared authoring keys).' [first run exited 1 as PREREQUISITE NOT MET / 'Nothing was checked' - the CLI was unbuilt; I built @objectstack/cli and re-ran for a real verdict rather than reporting the throw as a pass]\n scripts/docs-audit/check-affected-docs.mjs EXIT=0\nDECLARED NARROWING: I did not run check:type-check-coverage / check:type-check-debt --re-measure, which need the whole workspace closure built. There is no PR, so CI will not run them for this branch either; a re-dispatched agent shipping the actual fix must run them. Nothing else was skipped.\n\nNO ABLATION was performed, and none is meaningful here: an ablation exists to prove a NEW guard can fail, and I wrote no guard. The equivalent evidence for this run is PROBE B/B2 - the sibling screen is shown live and shown non-degenerate (it drops the cross-org holder AND keeps the same-org one) on the same tree in the same run.", "open_questions": [ { "question": "Clause-2 re-tiering: the graded 'no' is falsified by measurement. Applying the org screen to `manager` turns an accepted call into a refused one for flows authored with onEmptyApprovers:'fail' whose sole approver is a cross-org `manager` — measured as PROBE C-a (opens today, status 'pending') vs PROBE C-b (a screened type in the identical shape throws NO_APPROVERS). The mechanism is approval-service.ts:1588: `!approvers.some(a => a && !a.includes(':'))` counts a slate holding only the `manager:<value>` literal as EMPTY, and 'fail' throws on empty. The flip is confined to the non-default policy (default admin_rescue still opens the request), and the accept/reject CONTRACT itself is unchanged — only which inputs land in a bucket that already exists and is already reached by every screened sibling. Is that still Clause-2: yes?", "options": [ "A — Accept the falsification: grade Clause-2: yes and re-dispatch at claude-fable-5. Costs one dispatch cycle; the branch already carries a green, gate-clean measurement harness so the re-dispatched agent starts from evidence, not from scratch.", "B — Rule that a narrowing which moves inputs into a pre-existing, authored policy bucket is not a contract change, keep Clause-2: no, and re-dispatch at opus with this measurement recorded on the card so the reviewer sees the flip was known and accepted.", "C — Split the card: implement the screen with an explicit carve-out so a screened-out manager does not count as an empty slate (e.g. keep the literal but exempt it from the onEmptyApprovers test), which removes the accept->refuse flip entirely. This is itself a behaviour decision about onEmptyApprovers semantics and is bigger than the card as fenced." ], "recommendation": "A. Not because B is clearly wrong — B is a defensible reading, and the contract-shape argument for it is real — but because the asymmetry favours it: the dispatch made the tripwire unconditional and named this exact scenario as its example, so the cost of a wrong 'proceed' (a security-labelled card shipped at the wrong tier, discovered in review or in the merge queue) is much higher than the cost of a wrong 'stop' (one dispatch cycle, with the measurement work already banked on the branch). C should be rejected outright as scope creep unless the maintainer wants the onEmptyApprovers semantics reopened." }, { "question": "The direction-1 warrant needs restating before the fix is written, whoever writes it. 'Every sibling approver expansion is org-scoped' is false: `team` is not (measured, PROBE W), and packages/spec/src/automation/approval.zod.ts declares `team: false` next to `manager: false` in APPROVER_ORG_SCOPED. That table answers ADR-0105 D9 retargetability rather than screening, so it does not refute direction 1 — but it does mean `manager` and `team` currently sit in one declared class, and moving `manager` out alone splits it.", "options": [ "A — Screen `manager` only (the card as fenced), and let #10230 decide `team` separately.", "B — Screen both in one change, on the grounds that they are one declared class and splitting it leaves the same hole open under a different approver type.", "C — Re-examine whether the class is right at all before screening either." ], "recommendation": "A, with the reasoning written into the PR body rather than left implicit: the two are NOT symmetric on the facts. `sys_team` carries an organization_id, so a team id transitively names exactly one organization and an author who picks a team has already picked an org; `sys_user.manager_id` names a row in a global identity table that carries no tenancy fact at all, which is why `manager` is the sharper hole. Splitting the class is defensible on that difference — it just has to be argued, not assumed, because the card's stated warrant does not survive contact with the source." }, { "question": "Not a fork I hit, but the next implementer will: sys_user has no organization_id, so the only tenancy fact available for a manager screen is a sys_member membership test — which is the same read filterApproversWhoCanRead already performs for read-visibility (approver-org-scope.ts). Does asserting 'the manager must be a member of the request org' as TENANCY, using that read, stay clear of #7497?", "options": [ "A — Yes, and say so explicitly in the code: two of the three org-scoped siblings (org_membership_level, and position's second limb) already screen on sys_member.organization_id, so using it for `manager` is the sibling treatment, not a read-visibility ruling.", "B — No — route the card through #7497 first." ], "recommendation": "A. The sibling precedent is concrete and in-file, and the change grants no reads and applies no read screen to any type that lacks one today, so it decides nothing #7497 asks. But the implementer should write that argument into the code comment, because the read looks identical to the D9 one and the next reader will assume it settled #7497 unless told otherwise." } ], "out_of_scope_findings": [ "filed as #10230: the `team` approver expansion is not org-screened either — expandTeamUsers takes no organization argument and is the only graph type not wrapped in `bounded(...)`; an org_a request routed to an org_b team resolves that team's members (measured). This is the finding that falsifies #10153's stated warrant.", "filed as #10231 (label `finding`): plugin-sharing's TeamGraphService.managerOf(userId, _organizationId) discards its organization argument, and provablyDisjointOrgs / screenToTenant exist nowhere in this repo (counter-checked) — so the 'governed side' the triage rationale invokes is cloud/enterprise-side, and within framework BOTH manager-chain readers are unscreened. Records a correction to the meta-rule's premise rather than proposing a patch." ] }
Generated by Claude Code
os-dev-report
(Re-posted: the HTML-comment marker on my previous comment was stripped by GitHub's body sanitizer after it was stored, so that copy is invisible to a marker scan. This one leads with the literal text and carries the same report; angle brackets are removed throughout because the sanitizer ate those too. Supersedes the comment immediately above.)
{ "issue": 10153, "status": "blocked", "branch": "claude/issue-10153-manager-approver-org-screen", "pr": null, "premise_still_valid": true, "summary": "STOPPED AT THE RE-TIERING TRIPWIRE — no fix implemented, by instruction. The premise holds and I reproduced it on my branch: with an org_a request whose submitter's manager u_mgr_b holds a sys_member row ONLY in org_b, the `manager` approver resolves him into pending_approvers (['u_mgr_b']), while on the SAME tree and the same run the sibling `position` expansion drops a cross-org holder to the dead literal (['position:cfo']) and still resolves a same-org holder (['u_pos_a']). But the Clause-2 wire tripped: I measured that TODAY a node whose sole approver is a cross-org `manager` under onEmptyApprovers:'fail' OPENS successfully (status 'pending'), whereas a SCREENED type in the identical shape THROWS NO_APPROVERS — because approval-service.ts:1588 classifies a slate holding only type:value literals as EMPTY, and the 'fail' policy throws on it. Applying the screen therefore moves that input from accepted to refused, which is the dispatch's named example verbatim ('an approval submission that used to succeed now fails when no manager resolves'). Per the fence I stopped before writing the fix. SEPARATELY, the warrant does NOT hold as stated: `team` is a sibling graph expansion that is NOT org-scoped (expandTeamUsers takes no org argument and is the only graph type not wrapped in `bounded`), measured — an org_a request routed to an org_b team resolves ['u_team_b']; and the spec's own APPROVER_ORG_SCOPED table declares `team: false` alongside `manager: false`. I also could not find the 'governed side' the triage leans on anywhere in this repo: provablyDisjointOrgs / screenToTenant return zero hits (counter-checked against expandPositionUsers, 10 files), and this repo's own manager-chain reader TeamGraphService.managerOf(userId, _organizationId) discards its org argument — so within framework, approvals is not a straggler among screened peers. I did NOT hit the #7497 fork, but I flag that sys_user carries no organization_id, so any manager screen must be a sys_member membership test — the same read filterApproversWhoCanRead performs for read-visibility, putting the implementation one line from #7497's territory. The branch carries a measurement harness only (6 assertions pinning CURRENT behaviour, clearly marked to be inverted by the fix); no PR is open.", "tests": "All measurements on worktree /home/user/objectstack-issue-10153, branch head 13c7ac54b (merge base 04096f17e).\n\nHarness: packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts — run as `npx vitest run [that path] --reporter=verbose --disable-console-intercept`, EXIT=0, 6/6 passed. Real stdout:\n [PROBE A] request org = org_a pending_approvers = [u_mgr_b] -- premise: cross-org manager RESOLVES\n [PROBE B] pending_approvers = [position:cfo] -- sibling screened, same tree\n [PROBE B2] pending_approvers = [u_pos_a] -- screen is not reject-everything\n [PROBE C-a] opened OK, status = pending approvers = [u_mgr_b] -- Clause-2: ACCEPTED today\n [PROBE C-b] threw = NO_APPROVERS: approval node 'approve_step' on opportunity/opp1 resolved to no concrete app... -- Clause-2: REFUSED when screened\n [PROBE W] org_a request, org_b team gives pending_approvers = [u_team_b] -- warrant falsified\n\nZero-grep counter-check (required by this lane): grep -rn for provablyDisjoint and for screenToTenant over *.ts/*.mts/*.md both returned NO output; the same sweep for the neighbouring known-present term expandPositionUsers returned 10 files (spec/automation/approval.zod.ts, core/security/grant-validity.ts, plugin-sharing/position-graph.ts, plugin-approvals/approval-service.ts, ...), so the search works and the zeros are real.\n\nPackage suite: `pnpm --filter @objectstack/plugin-approvals test` under scripts/pm/os-verify-lock.sh — VERDICT command-exit 0; 'Test Files 25 passed (25) / Tests 503 passed (503)'. `pnpm --filter @objectstack/plugin-approvals typecheck` EXIT=0, script name echoed in the output (not a zero-match).\n\nDependency closure built first: pnpm --filter '@objectstack/plugin-approvals^...' build — os-verify-lock VERDICT command-exit 0.\n\nGate union re-derived on the FINAL commit 13c7ac54b with `node scripts/pm/dispatch-gates.mjs` (no paths passed; it derived the 2 changed paths from the merge base itself). Exit codes captured with EXIT=$? on the command, never through a pipe; each verdict quoted from the gate's own line:\n check:engine-double-contract EXIT=0 'check-engine-double-contract: OK - 333 pinned, 133 in the DEBT ledger, 2 exempt.'\n check:where-matcher EXIT=0 'where-matcher conformance holds: 265 matcher(s) discovered, 265 answer the combinator battery correctly or refuse it loudly'\n check:test-source-alias EXIT=0 'check-test-source-alias OK - 72 packages with tests scanned'\n check:type-source-resolution EXIT=0 'check-type-source-resolution OK - 76 packages with a tsconfig.json scanned'\n check:slot-lookup EXIT=0 'slot-lookup ratchet holds: 107 unswept site(s) in 25 file(s), none new'\n check:nul-bytes EXIT=0 'check-nul-bytes: OK (scanned 6076 text file(s) ... no raw ASCII control bytes)'\n check:cross-package-test-inputs EXIT=0\n check:query-options-erasure EXIT=0 'query-options-erasure ratchet holds: 67 unswept non-test site(s) in 17 file(s), none new'\n check:i18n EXIT=0 'check-i18n-bundles: OK (9 package(s) - all bundles in sync, no undeclared authoring keys).' [first run exited 1 as PREREQUISITE NOT MET / 'Nothing was checked' - the CLI was unbuilt; I built @objectstack/cli and re-ran for a real verdict rather than reporting the throw as a pass]\n scripts/docs-audit/check-affected-docs.mjs EXIT=0\nDECLARED NARROWING: I did not run check:type-check-coverage / check:type-check-debt --re-measure, which need the whole workspace closure built. There is no PR, so CI will not run them for this branch either; a re-dispatched agent shipping the actual fix must run them. Nothing else was skipped.\n\nNO ABLATION was performed, and none is meaningful here: an ablation exists to prove a NEW guard can fail, and I wrote no guard. The equivalent evidence for this run is PROBE B/B2 - the sibling screen is shown live and shown non-degenerate (it drops the cross-org holder AND keeps the same-org one) on the same tree in the same run.", "open_questions": [ { "question": "Clause-2 re-tiering: the graded 'no' is falsified by measurement. Applying the org screen to `manager` turns an accepted call into a refused one for flows authored with onEmptyApprovers:'fail' whose sole approver is a cross-org `manager` — measured as PROBE C-a (opens today, status 'pending') vs PROBE C-b (a screened type in the identical shape throws NO_APPROVERS). The mechanism is approval-service.ts:1588, whose empty-slate test counts a slate holding only the type:value literal as EMPTY, and 'fail' throws on empty. The flip is confined to the non-default policy (default admin_rescue still opens the request), and the accept/reject CONTRACT itself is unchanged — only which inputs land in a bucket that already exists and is already reached by every screened sibling. Is that still Clause-2: yes?", "options": [ "A — Accept the falsification: grade Clause-2: yes and re-dispatch at claude-fable-5. Costs one dispatch cycle; the branch already carries a green, gate-clean measurement harness so the re-dispatched agent starts from evidence, not from scratch.", "B — Rule that a narrowing which moves inputs into a pre-existing, authored policy bucket is not a contract change, keep Clause-2: no, and re-dispatch at opus with this measurement recorded on the card so the reviewer sees the flip was known and accepted.", "C — Split the card: implement the screen with an explicit carve-out so a screened-out manager does not count as an empty slate (e.g. keep the literal but exempt it from the onEmptyApprovers test), which removes the accept-to-refuse flip entirely. This is itself a behaviour decision about onEmptyApprovers semantics and is bigger than the card as fenced." ], "recommendation": "A. Not because B is clearly wrong — B is a defensible reading, and the contract-shape argument for it is real — but because the asymmetry favours it: the dispatch made the tripwire unconditional and named this exact scenario as its example, so the cost of a wrong 'proceed' (a security-labelled card shipped at the wrong tier, discovered in review or in the merge queue) is much higher than the cost of a wrong 'stop' (one dispatch cycle, with the measurement work already banked on the branch). C should be rejected outright as scope creep unless the maintainer wants the onEmptyApprovers semantics reopened." }, { "question": "The direction-1 warrant needs restating before the fix is written, whoever writes it. 'Every sibling approver expansion is org-scoped' is false: `team` is not (measured, PROBE W), and packages/spec/src/automation/approval.zod.ts declares `team: false` next to `manager: false` in APPROVER_ORG_SCOPED. That table answers ADR-0105 D9 retargetability rather than screening, so it does not refute direction 1 — but it does mean `manager` and `team` currently sit in one declared class, and moving `manager` out alone splits it.", "options": [ "A — Screen `manager` only (the card as fenced), and let #10230 decide `team` separately.", "B — Screen both in one change, on the grounds that they are one declared class and splitting it leaves the same hole open under a different approver type.", "C — Re-examine whether the class is right at all before screening either." ], "recommendation": "A, with the reasoning written into the PR body rather than left implicit: the two are NOT symmetric on the facts. sys_team carries an organization_id, so a team id transitively names exactly one organization and an author who picks a team has already picked an org; sys_user.manager_id names a row in a global identity table that carries no tenancy fact at all, which is why `manager` is the sharper hole. Splitting the class is defensible on that difference — it just has to be argued, not assumed, because the card's stated warrant does not survive contact with the source." }, { "question": "Not a fork I hit, but the next implementer will: sys_user has no organization_id, so the only tenancy fact available for a manager screen is a sys_member membership test — which is the same read filterApproversWhoCanRead already performs for read-visibility (approver-org-scope.ts). Does asserting 'the manager must be a member of the request org' as TENANCY, using that read, stay clear of #7497?", "options": [ "A — Yes, and say so explicitly in the code: two of the three org-scoped siblings (org_membership_level, and position's second limb) already screen on sys_member.organization_id, so using it for `manager` is the sibling treatment, not a read-visibility ruling.", "B — No — route the card through #7497 first." ], "recommendation": "A. The sibling precedent is concrete and in-file, and the change grants no reads and applies no read screen to any type that lacks one today, so it decides nothing #7497 asks. But the implementer should write that argument into the code comment, because the read looks identical to the D9 one and the next reader will assume it settled #7497 unless told otherwise." } ], "out_of_scope_findings": [ "filed as #10230: the `team` approver expansion is not org-screened either — expandTeamUsers takes no organization argument and is the only graph type not wrapped in the `bounded` helper; an org_a request routed to an org_b team resolves that team's members (measured). This is the finding that falsifies #10153's stated warrant.", "filed as #10231 (label `finding`): plugin-sharing's TeamGraphService.managerOf(userId, _organizationId) discards its organization argument, and provablyDisjointOrgs / screenToTenant exist nowhere in this repo (counter-checked) — so the 'governed side' the triage rationale invokes is cloud/enterprise-side, and within framework BOTH manager-chain readers are unscreened. Records a correction to the meta-rule's premise rather than proposing a patch." ] }
Generated by Claude Code
4 remaining items
Claim(第二轮): PM seat #6021, session
session_01PnJHU45vPJj5UQrxe946Bx.- Branch:
claude/issue-10153-manager-approver-org-screen(沿用第一轮的分支13c7ac54b,其上已有测量脚手架) - Worktree:
/home/user/objectstack-issue-10153(⛔ 禁git stash) - File surface:
packages/plugins/plugin-approvals/src/approval-service.ts+ 测试 + changeset - Container & model:
claude-opus-5 - Clause-②: yes(已由第一轮测量确立,非推定)
- Serial constraints: ⛔ approvals: the
teamapprover expansion is not organization-screened either — a cross-org team resolves into a request's approver slate #10230 排在本卡之后——同一文件approval-service.ts,单热面,禁并发。与enforceFilesCapability/enforceFeedsCapabilityarebeforeInsert-only — a re-point via update dodges the capability opt-in on the NEW parent object #10170(plugin-audit/src/audit-writers.ts)面不相交。
⚠️ 门禁偏离,明示授权,写在这里备查维护者 2026-08-20 授权:fable 用尽期间,Clause-② 卡可在
claude-opus-5上开发,落地前等分诊审核。⇒ 本卡按 opus 派出,补偿控制是"分诊审核后才合并":⛔ PM 不自行翻 ready、不自行武装 auto-merge。PR 就绪后交分诊/维护者复核,由他们决定是否入队。这个控制是这次降级之所以安全的原因,不得省略。
第一轮的产出(测量脚手架、Clause-② 证据、两条被证伪的分诊依据 #10230 / #10231)见 comment
5357409793,本轮从证据开始。
Generated by Claude Code
- Branch:
- added a commit that references this issue
on Aug 20, 2026 os-dev-report
(Leading with the literal marker text: the HTML-comment form was measured on this very card in round 1 to be stripped by GitHub's body sanitizer after storage, making the comment invisible to a marker scan. Angle brackets avoided throughout for the same reason.)
{ "issue": 10153, "status": "done", "branch": "claude/issue-10153-manager-approver-org-screen", "pr": "https://github.com/objectstack-ai/objectstack/pull/10334", "premise_still_valid": true, "summary": "ROUND 2, implemented. All six of round 1's readings reproduce unchanged on today's main (branch merged origin/main first: merge base moved 04096f17e -> 67630c452, 9 commits): cross-org manager resolves, sibling position is screened and its screen is non-degenerate, the Clause-2 accept-to-reject flip is real, and team is still unscreened. lookupManager now takes the request organization and drops a manager who is PROVABLY outside it via a sys_member membership test: membership rows exist and none is in the request's org => screened out; no rows at all, a failed read, or a request with no organization => routing unchanged and (for the last) no read performed. That shape is the card's own direction-1 wording ('refuse when provablyDisjointOrgs holds') and this file's twice-stated posture on addressing paths; it is also load-bearing, since a strict must-prove-membership screen would drop every manager approver on any stack that stamps an organization but never materializes sys_member rows -- measured: this repo's own type:manager OOO fixture (CTX carries tenantId 't1', seeds no sys_member) and app-showcase's by_manager demo node are both such stacks. CLASS SPLIT, argued not assumed (the card's stated warrant does not survive the source; #10230/#10231 record that): sys_team carries an organization_id so a team id transitively names exactly one organization, while sys_user.manager_id names a row in a global identity table with NO tenancy fact at all -- that asymmetry is why manager can leave the APPROVER_ORG_SCOPED class alone. APPROVER_ORG_SCOPED itself is untouched (it answers D9 retargetability, not screening). I did NOT hit the #7497 fork: two of the three org-scoped siblings already screen on sys_member.organization_id, sys_user offers no other tenancy fact, no reads are granted and no read screen is applied to any type that lacked one -- and that argument is written into the code, at length, so the next reader does not conclude #7497 was settled here.", "tests": "Worktree /home/user/objectstack-issue-10153. FINAL COMMIT 5f538cdf2 (clean tree); every gate figure below was produced on that commit.\n\nROUND-1 RE-CONFIRMATION on merged main (commit a366f0717), before any fix: 6/6 passed, EXIT=0, same readings as round 1 -- [PROBE A] ['u_mgr_b'] / [B] ['position:cfo'] / [B2] ['u_pos_a'] / [C-a] opened, status pending / [C-b] threw NO_APPROVERS / [W] ['u_team_b'].\n\nPINS (manager-approver-org-screen.test.ts, round 1's harness inverted as its own header instructed) -- 10 passed, EXIT=0:\n [PROBE A] pending_approvers = ['manager:owner_id'] INVERTED (was ['u_mgr_b'])\n [PROBE A2] pending_approvers = ['u_mgr_b'] same-org manager STILL resolves (2nd direction)\n [PROBE A3] pending_approvers = ['u_mgr_b'] no membership row anywhere: unchanged\n [PROBE A4] request org = null, ['u_mgr_b'] no request org: unchanged, no read\n [PROBE B] ['position:cfo'] / [B2] ['u_pos_a'] sibling screen live and non-degenerate\n [PROBE C-a] threw NO_APPROVERS THE ACCEPT-TO-REJECT FLIP (was: opened, pending)\n [PROBE C-a2] status = pending, ['manager:owner_id'] DEFAULT policy still opens -- the flip is confined\n [PROBE C-b] threw NO_APPROVERS unchanged\n [PROBE W] ['u_team_b'] team still unscreened, #10230, deliberately untouched\nNo ADR-0112 envelope is minted: 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.\n\nABLATION. Predicted signature stated FIRST: neuter managerIsProvablyOutsideOrg => A, C-a, C-a2 fail; A2/A3/A4/B/B2/C-b/W stay green; 3 failed / 7 passed. Observed exactly that, with the predicted messages ('expected [u_mgr_b] to deeply equal [manager:owner_id]' x2, 'expected null to be truthy'). REBUILD STATEMENT, argued from the files: no rebuild was required and that is provable, not assumed -- the subject is imported relatively ('./approval-service.js'), which vitest resolves to this package's src/, and check-test-source-alias.mjs's KNOWN_UNALIASED_TEST_IMPORTS lists only CROSS-package specifiers for @objectstack/plugin-approvals (objectql etc., used here only for the fake engine's dispatch assertions, untouched by the ablation). The observation is self-validating in the safe direction: a dist-resolved test would have stayed GREEN through a guard-removing mutation (the false-green signature); it went red with no build, which proves source is what ran. RESTORE PROOF: before f2fc42771870a507bf5334b048ea7b0aa40c3e41, ablated 7cb0defc3291c9f19547fd6fa9f3a2bfeb80f1e8, restored f2fc42771870a507bf5334b048ea7b0aa40c3e41 (identical to the committed blob), 0 ABLATION markers left, and the RESTORE LEG was re-run: 10 passed.\n\nDEPENDENCY CLOSURE BUILT FIRST: pnpm --filter '@objectstack/plugin-approvals^...' build -- os-verify-lock VERDICT command-exit 0.\nPACKAGE SUITE: pnpm --filter @objectstack/plugin-approvals test under os-verify-lock -- VERDICT command-exit 0, 'Test Files 25 passed (25) / Tests 507 passed (507)' (503 before this card + 4 new pins).\nTYPECHECK: EXIT=0 with the script name echoed ('tsc --noEmit') -- not a zero-match. NOTE it is structurally blind here; see the second gate finding below.\n\nGATE UNION re-derived on the FINAL commit with 'node scripts/pm/dispatch-gates.mjs', no paths passed (it derived the 4 changed paths from the merge base itself). Exit codes captured on the command, never through a pipe; each verdict quoted from the gate's own line:\n check:slot-lookup EXIT=0 'slot-lookup ratchet holds: 107 unswept site(s) in 25 file(s), none new'\n check:engine-double-contract EXIT=0 'check-engine-double-contract: OK - 333 pinned, 133 in the DEBT ledger, 2 exempt.'\n check:where-matcher EXIT=0 '265 matcher(s) discovered, 265 answer the combinator battery correctly or refuse it loudly'\n check:query-options-erasure EXIT=0 'ratchet holds: 67 unswept non-test site(s) in 17 file(s), none new'\n check:test-source-alias EXIT=0 'check-test-source-alias OK - 72 packages with tests scanned'\n check:type-source-resolution EXIT=0 'check-type-source-resolution OK - 76 packages with a tsconfig.json scanned'\n check:nul-bytes EXIT=0 'check-nul-bytes: OK (scanned 6091 text file(s) ... no raw ASCII control bytes)'\n check:i18n EXIT=0 'check-i18n-bundles: OK (9 package(s) - all bundles in sync, no undeclared authoring keys).'\n check:type-check-coverage EXIT=0 'OK - 64/77 workspace packages type-checked (plus the root), 13 in the DEBT ledger, 1 exempt.'\n check:type-check-debt --re-measure EXIT=0 '33 ledger entr(ies) re-measured in 308.2s, 1924 raw tsc error(s) total, none above its recorded number. surplus: none'\n check:changeset-gate-self-tests EXIT=0 check:objectui-changeset EXIT=0\n check:cross-package-test-inputs EXIT=0 'OK: 12 package(s) read outside themselves, all declared'\n check-adr-0087-registration.mjs EXIT=0 check-changeset-no-major.mjs EXIT=0\n check-empty-changeset.mjs EXIT=0 check-cross-package-test-inputs.mjs EXIT=0\n docs-audit/check-affected-docs.mjs EXIT=0\nNO NARROWING DECLARED -- round 1 had to declare check:type-check-coverage / check:type-check-debt --re-measure un-run; both are run here, on a fully built workspace.\n\nTWO GATES WENT RED AND WERE FIXED AT SOURCE, not routed around:\n 1. check:query-options-erasure: 'erasure count grew 10 -> 11. The file is grandfathered for its EXISTING sites only; new ones must carry the declared options type.' My new sys_member read had picked up the file's habitual 'as any'. Assertion dropped (ApprovalEngine.find accepts the literal as written); back to the baseline 67.\n 2. check:type-check-debt --re-measure: '@objectstack/plugin-approvals: TEST_DEBT records 348 raw tsc error(s), tsc --noEmit now reports 369 (+21).' plugin-approvals excludes **/*.test.ts from its tsconfig, so 'pnpm --filter ... typecheck' reported EXIT=0 over a file carrying 21 real 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 to exactly 348, my file contributing 0. NO ledger entry was raised. (Round 1's harness carried the same 21 errors unmeasured, since round 1 never ran this gate.)\n Also mechanical: check:engine-double-contract asked for the pinned ledger to follow the harness file's rename; ran --write, and the diff is exactly the two path renames, no coverage lost.", "open_questions": [], "out_of_scope_findings": [ "filed as #10331 (label finding, domain:spec): ApprovalRequestRow -- the PUBLISHED contract in packages/spec/src/contracts/approval-service.ts, lines 84-262 -- declares no organization_id, though openNodeRequest stamps one on the row it writes and returns. Zero-grep counter-checked (pending_approvers hits line 96 in the same file, so the search works). Surfaced only by the hidden-layer typecheck; the pins now read the stamp off the persisted sys_approval_request row, which is a workaround, not a resolution.", "not re-filed, already open: #10230 (team is not org-screened either -- serialized behind this PR, same file) and #10231 (TeamGraphService.managerOf discards its organizationId). Neither is addressed here; the W pin asserts team is still unscreened so this PR's non-treatment of it is recorded rather than ambiguous." ] }⛔ Do NOT auto-merge — PR #10334 is draft and stays that way pending triage review
Clause-②: yes, developed atclaude-opus-5under the maintainer's 2026-08-20 authorization. The compensating control is that triage reviews before merge: the PR is draft, nothing arms auto-merge, and the accept-to-reject flip is stated at the very top of the PR body with the C-a / C-b / C-a2 readings and the affected-caller statement, so a reviewer does not have to discover it.
Generated by Claude Code
PM 复核(第二轮):ACCEPT —— PR #10334 保持 draft,⛔ 未翻 ready、未武装 auto-merge,交分诊审核
按 2026-08-20 的门禁偏离授权,本卡在 opus 上开发,补偿控制是"分诊审核后才落地"。我没有省掉它。按内容核实:4 文件(changeset ·
approval-service.ts· 新 pin 文件 ·engine-double-contract.pinned.json的机械改名),packages/spec零触碰,类型债台账文件未动。⭐ 最有价值的一处:它没有实现"看起来更严格"的那个屏
它选的是**"可证明在组织之外才筛掉"(
provablyDisjointOrgs语义),而不是"必须证明是成员才放行"。这不是保守,是测出来的**:严格版会在任何盖了组织戳却从不物化sys_member行的栈上把每一个 manager 审批人都筛掉——而这样的栈在本仓就有两个:本仓自己的type:managerOOO fixture(CTX 带tenantId 't1',不 seedsys_member)和 app-showcase 的by_manager演示节点。⇒ 严格版会以"安全收紧"的名义静默打断两条现成路径,且 CI 未必看得出来。这正是我在派单里要"双向 pin"想防的东西,而它比双向 pin 走得更远:把第三、第四个方向也钉住了(A3 无成员行 ⇒ 不变;A4 请求无组织 ⇒ 不变且不发起读)。
⭐ 它拒绝伪造 ADR-0112 合规
原话:"没有铸造任何 envelope:
NO_APPROVERS的抛出是既有代码、是个裸Error,没有 code/status 可断言,编一个出来就是虚构。"我在派单里写了"若铸拒绝则须断言 code 与 status"。它没有为了对上这条要求去发明一个错误码,而是说明了为什么这条不适用。照着模板把字段填满,比空着更危险。
⭐ 两个门禁变红,它在源头修,而不是绕过去
check:query-options-erasure从 10 涨到 11 —— 它新加的sys_member读顺手沿用了该文件的习惯性as any。去掉断言(ApprovalEngine.find本就接受字面量写法),回到基线 67。check:type-check-debt --re-measure报 plugin-approvals 从 348 涨到 369(+21)。根因是这个包的 tsconfig 排除了**/*.test.ts⇒pnpm --filter … typecheck在一个带着 21 个真实 TS2339/TS18048 的文件上照样报 EXIT=0。它用一个拒绝 auto-approval 那条臂的opened()助手去收窄类型,而不是用断言把类型问题盖过去,重测精确回到 348、自身贡献 0,且未抬台账。
⚠️ 这条要我自己认领一半:第一轮它声明过没跑--re-measure(需要整工作区构建),我接受了那个 narrowing。声明是诚实的,但后果是那 21 个错误在第一轮的脚手架里未被测量地躺着——而这正是"声明式收窄"存在的意义:它让后一轮必须去跑,然后就被抓到了。流程按设计工作了,但我该在接受 narrowing 时就说出"这个包的 typecheck 对测试文件结构性失明"。逐条核实
- ✅ 先在今日 main 上复现第一轮的 6 条读数(merge base 移了 9 个提交)再动手,六条全部不变。
- ✅ 类型分割是论证出来的:
sys_team带organization_id(team id 传递地指定唯一组织),而sys_user.manager_id指向的全局身份表没有任何租户事实——所以manager可以单独离开APPROVER_ORG_SCOPED类,而该表本身未被触碰(它回答 D9 可重定向性,不是屏蔽)。 - ✅ 没有触到 Design: does approver routing imply record read visibility? (#7345 model half) #7497,且把"为什么这没有裁定 Design: does approver routing imply record read visibility? (#7345 model half) #7497"写进了代码(三个 org-scoped 同级里有两个已在
sys_member.organization_id上筛,sys_user没有别的租户事实,本改动不授予任何读、也不给任何原本没有读屏的类型加读屏)。 - ✅ C-a2 把翻转的边界钉住:默认
admin_rescue策略仍然开单——accept→reject 的翻转只发生在非默认的'fail'策略上。 - ✅ W pin 把"没治 team"记录成事实而非含糊:approvals: the
teamapprover expansion is not organization-screened either — a cross-org team resolves into a request's approver slate #10230 仍未被筛,本 PR 明确不碰。 - ✅ 消融预测 3 红 7 绿、逐条含预测消息,实测吻合;
git hash-object三态一致,且重跑了还原腿(10 passed)。 - ⭐ 重建声明是自证的:"一个走 dist 的测试在守卫被移除后会保持绿(那正是假绿的签名);它在没有构建的情况下变红了,这证明跑的是 source。" 这比通常那种论证更强——它把观察本身变成了证据。
衍生 #10331(
domain:spec)ApprovalRequestRow——packages/spec/src/contracts/approval-service.ts里的已发布契约——不声明organization_id,而openNodeRequest却在它写入并返回的行上盖了这个戳。零命中带反向对照(同文件pending_approvers命中 :96,证明检索是活的)。⭐ 它没有顺手去packages/spec修,而是立卡——正是我划的红线。后续
#10230(
team同样未做组织屏)继续排队,⛔ 不在本 PR 落地前派出:同一文件,且分诊复核可能改变本卡的形状,现在开工会让它建在流沙上。
Generated by Claude Code
Triage seat: hung
needs:contract-review— delivering PR #10334 declaresClause-②: yes(accept→reject flip ononEmptyApprovers: 'fail'slates) and was developed atclaude-opus-5under the 2026-08-20 fable-exhaustion authorization, whose compensating control is a contract-review-tier second review before landing. Recorded at the services PM's request so the parked PR is visible to the review chain instead of living only in seat memory. PR stays draft, auto-merge unarmed, until a triage sub-round running verifiably at the contract-review tier clears this label. This fire's sub-round is skipped: the tier fuse reading (get_sessionmachine reading) is unavailable in this environment, and a self-declared tier is not a reading — parked is the safe state.
Generated by Claude Code
Contract review (triage seat): PASS —
needs:contract-reviewcleared; the services seat may proceed to ready/queue PR #10334.Authorization: this session's environment cannot take the tier fuse's machine reading, and the maintainer ordered the review live (verbatim: 「你应该处理所有的 needs:contract-review 这些」) — a recorded human release of the parked queue, not a silent downgrade. Reviewer is not the dispatching seat (condition ② holds).
Reviewed the Clause-② delta against the diff, not the report:
- The flip is confined and pinned. Only a node authored with non-default
onEmptyApprovers: 'fail'whose slate ends up empty moves from accepted to refused; the defaultadmin_rescuestill opens (C-a2) and the throw is the pre-existing bareNO_APPROVERS— asserting an ADR-0112 envelope on it would indeed be a fiction, so the message-prefix assertion is right. - The screen only ever narrows, and fail-open limbs are the file's ruled posture (absent tenancy fact / failed read / org-less request ⇒ unchanged), load-bearing for stacks without
sys_memberrows — measured on the repo's own fixture, not asserted. - Design: does approver routing imply record read visibility? (#7345 model half) #7497 is genuinely not decided here — the two-facts argument (both org-scoped siblings already screen on
sys_member.organization_id;sys_userholds no other tenancy fact) holds, and no read is granted anywhere. - The
teamsplit from the declared class is defensible on thesys_team-carries-organization_idasymmetry, and the W pin records the non-treatment; approvals: theteamapprover expansion is not organization-screened either — a cross-org team resolves into a request's approver slate #10230/[finding] plugin-sharing:TeamGraphService.managerOfdiscards itsorganizationIdargument — the manager-chain screen cited as this repo's governed side is not in this repo #10231 carry the residue. - Pins are two-directional across both policies (A–A4, B/B2, C-a/C-a2/C-b, W); the engine-double contract entries are registered; changeset documents the flip loudly.
Generated by Claude Code
- The flip is confined and pinned. Only a node authored with non-default
收口:PR #10334 已合并(01:55),按内容核验通过
在
origin/main上实测,用特异标志物而非宽泛词(后者证明不了任何事):packages/plugins/plugin-approvals/src/approval-service.ts:1376 private async lookupManager(userId: string, organizationId?: string | null): Promise<string | null>签名确实多了组织参数 —— 合并前是单参数
lookupManager(userId: string)。对照项expandApprovers返回 4,证明这次读取有效(否则一个读不到的文件也会给出「零命中」)。放行链完整:
Clause-②判 yes → 挂needs:contract-review→ 分诊席独立审查 PASS(审查者非派发席)→ 标签清除 → PM 翻 ready + 武装 auto-merge → 队列合并。补偿控制从头到尾没有被绕过。孪生半边已派发
#10230(
teamapprover 同样未做组织筛选)此前串行等待本卡落地,现已解禁并派发。我在它的 claim 里把本 PR 指定为已获批准的先例形状,但同时写明⚠️ 不可照抄:sys_team有organization_id列,而sys_user.manager_id指向的全局身份表没有租户事实 —— 两侧的组织归属可得性不同,screen 的形状要各自测量而不是从这里推断。
Generated by Claude Code
- added a commit that references this issue
on Aug 21, 2026 - added 3 commits that reference this issue
on Aug 23, 2026
Observed while implementing the read-only crossing-
manager_idaudit for cloud#1369 (not fixed there — that card is explicitly read-only and files no writes). Recording it here because a fix would land inplugin-approvals, not in cloud.What was measured
In
packages/plugins/plugin-approvals/src/approval-service.ts,expandApprovershands the directory organization to every graph-shaped approver expansion:team→expandTeamUsers(value, directoryOrg)department→expandBusinessUnitUsers(value, directoryOrg)position→expandPositionUsers(value, directoryOrg)org_membership_level→expandMembershipTierUsers(value, directoryOrg)The
managerbranch does not:and
lookupManager(same file) reads the column directly under a system context, taking no organization argument at all:Why it matters
sys_useris a global identity table with noorganization_idcolumn, so nothing else in this path supplies the tenancy fact. If asys_user.manager_idcrosses an organization boundary, an approval step withapproverType: 'manager'routes the submission to an approver in another organization — an out-of-tenant person granted approval authority over the record.The hierarchy consumer of the same column has been screened since cloud#1195 (
HierarchyScopeResolver.screenToTenantdrops userssys_memberproves are outside the caller's org). Approvals is a different consumer of the same column and was not covered by that work, so the same row behaves in opposite directions: it silently narrows anown_and_reportsowner set and widens approval routing.Not asserting the fix shape
Two directions exist and they are not equivalent, so this is filed for triage rather than argued:
lookupManagerthe way the sibling expansions are screened (refuse / fall through whenprovablyDisjointOrgsholds), which makes the approval slot resolve to nobody and needs a story for that;Whether approver routing should imply tenancy at all overlaps with the open design question in #7497.
Related
Verified against
907c11d2cee5fcdd420e2eedf79dc6081d08f5c3(the SHA cloud pins today).