Data scope via dynamic sharing rules; rescore results when a bonus is decided

[baozhoutao]

Work item #2.

What changed

The 28 statically declared per-unit criteria sharing rules are gone. Data scope is derived from the plan configuration at runtime and written as sharing-rule data, per plan, so onboarding a customer organization takes no metadata change and a grant can be taken away again.

File	Change
src/services/sharing-service.ts	new — derives the per-plan rule set from participating subjects, their leaders and the staff assignments; re-asserts it at every business trigger and reconciles (deactivates what the plan no longer implies)
src/objects/entry.object.ts	kpi_check_task and kpi_adjustment gain a stored plan lookup — a rule condition can only compare columns of its own object
src/hooks/plan.hook.ts	publish provisions + reconciles; new PlanFreezeSharingHook downgrades a closed/archived plan to read-only
src/hooks/sheet.hook.ts	stamps plan on check tasks; re-asserts the check-task and result rules
src/hooks/adjustment.hook.ts	stamps plan; re-asserts result rules; rescoring is now non-blocking like everywhere else
src/hooks/bonus.hook.ts	new BonusAfterDecideHook — rescores results on a bonus decision (checklist #7)
src/security/index.ts	executive leader narrowed to own scope; reporter row-level security widened to let non-person dimensions through
src/data/align-demo-units.ts	new, temporary demo-fixture tenancy alignment — see below
src/security/sharing-rules.ts	deleted, with the sharingRules config key; requires: ['sharing'] stays
test/sharing.test.ts, scripts/e2e-flow.mjs	21 unit cases (derivation, reconcile, close policy, env gate); T38–T57 for the non-admin data scope

Why sharing rules rather than per-record shares

sys_record_share is engine-owned (POST /api/v1/data/sys_record_share → 405), and the grant entry point refuses every non-user recipient — "group/position recipients are delivered via sharing rules (ADR-0111 D7)" — while buildReadFilter reads only user rows. Unit-wide delivery is a sharing-rule concern by design: the evaluator expands one rule into per-user rows and recomputes them when unit membership or the unit tree moves.

Rules are per plan, and reconciled

Keyed by unit alone, a rule is unrevokable: replace 市场部's leader and the old leader keeps edit on that unit's sheets in every future plan. So criteria carry the plan ({ plan, subject }, { plan, branch }, { plan, unit }, { plan, person }), names are namespaced kpi_p<plan>_…, and publish reconciles — every kpi-managed rule of that plan not in the desired set is deactivated, along with any legacy plan-agnostic kpi_share_* rule. Proven end to end by T54–T56: replace the leader on a new plan version, the old leader loses the new plan's sheets and keeps the old plan's.

Closed / archived plans keep READ and lose EDIT. Leaders and reporters must still see history; sheets and snapshots are already immutable, so withdrawing edit is the same discipline expressed in the data scope. T57.

Two platform behaviours handled explicitly

isSystem writes do not materialise shares — the platform says so in plugin-sharing's rule hooks and names re-evaluating the rule as the remedy. Every record in this app is created by a hook under the system context, so each trigger re-asserts the rules for the objects it just wrote. The re-assert never touches active (an admin's off switch stays off) and skips any customized row.

Demo-fixture tenancy — temporary, tracked by objectstack-ai/objectstack#14547. Seeded sys_business_unit rows carry organization_id = NULL while a rule is stamped with the caller's organization, and recipient expansion compares them strictly. src/data/align-demo-units.ts stamps the seeded units the way an admin-created unit already is, at kernel:bootstrapped, gated by an allowlist over the platform's own determination (NODE_ENV || 'production' — unset means production, so the gate is default-closed) and narrowed to the ids this app seeds with a null organization. Delete the file and its call when #14547 ships.

Verification

At 00253652abebf2741896f88335659996817585b1, against a freshly built artifact (pnpm build) and a wiped DB:

• pnpm verify — green (validate · typecheck · 55 unit tests, 5 files)
• pnpm e2e — 59 passed, 0 failed, run twice

PASS T39  发布按方案配置写入动态共享规则,条数与方案配置精确相符 — rules=25 expected=25
PASS T39b 规则条件按方案隔离:每条规则的条件里都带本方案 id — 25 条
PASS T41  部门填报人员只看到本部门填报单,含本次新发布的那张 — 3 张
PASS T43  部门填报人员能填写实际值并提交 — status=branch_checking score=103
PASS T44  分公司核对人员只看到本分公司的核对任务 — 2 条
PASS T51  填报人员看到本部门结果与本人到人结果,看不到他人的 — dept=true own=true other=false
PASS T53  分管领导看到所分管主体的部门结果,看不到非分管主体的结果
PASS T54  换人后新方案只为新任分管领导建规则,老领导没有新方案的规则
PASS T55  换掉分管领导:老领导看不到新方案的填报单,历史方案的仍看得到
PASS T57  方案关闭后共享降为只读 — 25 条规则,其中可编辑 0 条

admin	department reporter	executive leader	branch checker

Note on acceptance criterion 1

It expects a sys_record_share row whose recipient is the unit. The platform delivers unit scope as per-user rows minted from the rule, so the row exists with recipient_type: 'user', source: 'rule' — asserted by T40. Every other criterion passes as written.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_01VR2khJ3Me96btawVsfG6jb

[baozhoutao]

Code review report (os-project-dev-review)

Tier: full (high-risk: modifies existing hooks and permission sets)
Verdict: NOT MERGEABLE
General quality (/code-review): 9 raised, 6 upheld after checking them against the platform sources. Not upheld: (a) readScope: 'own' on kpi_entry_line / kpi_bonus being "inert" — details are gated on the read path by the ADR-0055 master derivation (plugin-security/src/security-plugin.ts:6168 computeControlledByParentFilter, composed into the read filter), not only by buildReadFilter; (b) unit_and_subordinates being a widening over the deleted business_unit rules — it is the recipient type the work item itself specifies; (c) leader own on kpi_check_task / kpi_adjustment leaving nothing visible — that is exactly what the work item asked for. Mechanism itself checks out: sys_sharing_rule data writes are the platform-designated channel (sharing-plugin.ts:399 bindRuleRebindTriggers re-binds and calls evaluateRule on every rule write, with no isSystem skip), { field: value } is a valid non-match-all criteria_json (rule-criteria.ts), unit_and_subordinates and user are both expanded (sharing-rule-service.ts:1060+), and the new RLS predicate lowers cleanly (|| → $or, != → $ne, formula/src/cel-to-filter.ts:299).

Library-specific checks:
1 Scope creep: ✅ none — every modified hunk maps to the work item or to the maintainer-authorized fixture. One item recorded, not blocking: src/security/index.ts:94 changes DeptReporterPermissionSet.rowLevelSecurity, a set the work item said to leave unchanged ("其余权限集不变"); it is a widening (person-dimension-only restriction) but it is what makes "部门/分公司结果共享给单元(read)" actually visible, and it is left in trace (code comment + docs/04 #18 + T51).
2 Checklist vs code: ✅ consistent — items 2/3/5/7 land in code as claimed, item 1's ⚠️ is honest, item 4's ✅ is scoped to sheets + approval (which is what T46/T47 assert). Item 6: I re-ran the static half here — pnpm validate green, pnpm typecheck clean, pnpm test 42/42 — the e2e half is not re-runnable under a read-only review (no dev server), so it stands on the author's evidence. Idempotency (a) holds: ensureRule keys on the rule name, so repeated advancement re-asserts and never duplicates. Leader permission set (b) is genuinely own on all seven listed objects. Bonus rescore (c) fires on approved→/rejected→ transitions with the sheet already approved/archived, and total_score is a rollup+formula so criterion 5's equality is sound. Fixture narrowing (d): ② and ③ hold, ① does not — see F6. e2e (e): no timeout-passes-as-green; waitUntil returns false on timeout and each assertion is still evaluated. T43's admin fallback runs after the T43 assertion, so it cannot self-pass — but it does let T44–T49 stay green with T43 red.
3 Forbidden traces: ✅ none — no node_modules/platform edits, no patches/, working tree clean; sys_sharing_rule data writes are the designated channel, not a bypass; the authorized sys_business_unit.organization_id fixture carries removal guidance in four places (README demo-seeds note, align-demo-units.ts header, objectstack.config.ts call site, docs/04 #18). Its narrowing is where it falls short — F6.
4 Hard rulings landed: ✅ — no new numeric fields in this diff, so the four-part declaration does not apply; the only new user-visible copy is admin-facing sharing-rule label/description text in Setup, which is business language with no internal code names (nit in F10).

Findings:
🔴 | src/services/sharing-service.ts:245 (provisionPlanSharing) | No revocation path. The function only creates/re-asserts; nothing deactivates a rule the current plan configuration no longer implies, and the criteria are plan-agnostic ({ subject: unit }). Replace 市场部's leader C with D in a new plan version and publish: D's rule is written, kpi_share_sheet_leader_bu_market_<C> stays active: true, so C keeps edit on every 市场部 filing sheet — including the new version's and all future ones. Same shape for a subject dropped from 参与主体 and for kpi_share_result_person_<emp> after an employee leaves 到人分工. This is the privilege-retention path check 4 asks about. | Exit: after building the intent set, sweep kpi_share_* rules for this plan's objects and set active: false on the ones not in it (or scope the criteria by plan).
🟡 | src/services/sharing-service.ts:201 (ensureRule payload) | Every re-assert forces active: true and overwrites label/criteria/recipient/access_level unconditionally. sys_sharing_rule.active is the admin's documented way to withdraw access ("Turn off to withdraw the access this rule granted"), and customized marks an admin's edits as keep-on-update — an admin who deactivates or tunes a kpi_share_* rule in Setup has it silently restored on the next publish, sheet approval, bonus decision or adjustment. | Exit: skip the row when customized is set, and do not force active back on.
🟡 | src/security/index.ts:130 + README role table + ExecLeaderPermissionSet docstring | 分管领导 kpi_result went org → own, but the only result rule minted for a leader is { person: leader }. Department/branch rows carry person = null and are shared only to the subject unit's members, and a leader is normally not a member (the e2e's own user C has no BU membership). So a leader now sees only their own leader-dimension row, while the README ("所分管主体"), the set's own docstring ("所分管主体的填报单与结果查看") and docs/04 #18 all promise the subject's results. No e2e case covers leader result visibility. | Exit: either mint a { unit: <subject> } result rule for the configured leader, or correct README + docstring to say leader/person rows only.
🟡 | src/services/sharing-service.ts:203-208 (ensureRule insert vs update) | organization_id is stamped on the insert branch only; the update branch omits it. A rule first written while its anchor unit had no organization stays organization-less forever — and re-publishing, the documented compensation, takes the update path and never repairs it. Two consequences: recipient expansion filters sys_business_unit/_member on organization equality (business-unit-graph.ts:270), so the scope stays silently empty; and a null-organization rule gets SYSTEM_CTX unscoped in criteriaContext (sharing-rule-service.ts:1016), i.e. it sweeps every tenant's records. | Exit: include organization_id in the update payload.
🟡 | src/services/sharing-service.ts:191 (ensureRule lookup) | findOne({ where: { name } }) is not organization-scoped, but sys_sharing_rule declares { fields: ['name'], unique: 'organization' } — names are unique per tenant. The lookup runs under sudo(), so on a multi-organization install a publish in org B that hits an identically named row in org A will updateById A's rule, rewriting its criteria, recipient and access level instead of inserting B's own. | Exit: carry the same organization_id in the where that the insert stamps.
🟡 | src/data/align-demo-units.ts:40 (isDemoEnvironment) | Narrowing ① of the maintainer's ruling is "dev/test only"; the code implements "not production" — NODE_ENV unset returns true. The platform's own convention is the opposite (cli/src/commands/doctor.ts:160 env.NODE_ENV || 'production'; os start forces production when unset), so this guard is default-open against a documented default-closed platform. Blast radius is bounded by narrowings ② and ③ (which do hold, including the second-pass filter), but the ruling's precondition is not met as written. | Exit: allowlist explicitly — ['dev','development','test'].includes(nodeEnv), matching the seed's own env: ['dev','test'].
⚪ | src/hooks/sheet.hook.ts:197, bonus.hook.ts:70, adjustment.hook.ts:71 | Re-assert fan-out: { objects: ['kpi_result'] } rewrites every result rule — one per subject plus one per 到人分工 row — on each sheet approval/archive, each bonus decision and each adjustment landing. Each write synchronously drives listRules + unbindAllRuleHooks + bindRuleHooks + evaluateRule (a criteria sweep capped at 5000 rows). A plan with 500 assigned employees turns one bonus approval into ~500 rule writes and ~500 rebind+sweep cycles inside the request, each with a window where the sharing hooks are unbound. Invisible at demo scale.
⚪ | src/hooks/adjustment.hook.ts:69 | The sharing re-assert is wrapped in try/catch (correct — it must not block), but the adjacent regenerateResults is still unwrapped, so an adjustment approval still fails if rescoring throws. Pre-existing, but it now sits one line above the opposite policy in the same block, and SheetAfterTransitionHook/BonusAfterDecideHook both treat rescoring as non-blocking.
⚪ | scripts/e2e-flow.mjs (T39, T43) | T39 asserts kpiRules.length >= 21 — a floor, not the derived count, so an over-generating regression would still pass. T43's admin fallback is correctly placed after the assertion, but it decouples T44–T49 from the reporter path it is meant to exercise.
⚪ | src/services/sharing-service.ts:95-130 | Admin-facing rule copy: 填报单共享给市场部 is minted for a unit_and_subordinates recipient, so the Setup list understates the grant (the subtree is included). Also, when one person is both a leader and an assignee, the single kpi_share_result_person_* rule keeps whichever description was pushed first (分管领导), which is only accurate by luck.
⚪ | PR description, "What changed" table | "reporter row-level security restricted to the person dimension" reads as a narrowing; the change is a widening (the predicate now lets non-person dimensions through). The code comment and docs/04 describe it correctly — only the PR body is misleading.

---

Generated by Claude Code

[baozhoutao]

Code review report (os-project-dev-review) — Round 2 — fix-point re-check

Tier: full (fix points only; no re-review of settled areas)
Head re-checked: 0025365 (fixes a4b5354..0025365, +417/−94 over 15 files)
Verdict: MERGEABLE
General quality (/code-review): not re-run — this round only re-checks the six findings and looks for regressions introduced by the fixes. Static gates re-run here at 0025365: pnpm validate green (182 fields, +2 for the new plan lookups), pnpm typecheck clean, pnpm test 55 passed (was 42 — 13 new cases, none removed or loosened).

Library-specific checks:
1 Scope creep: ✅ none — the fixes stay inside the finding surface. Two additions are new but load-bearing for the fix: plan lookups on kpi_check_task / kpi_adjustment (src/objects/entry.object.ts:182,330), needed because a rule's criteria can only compare columns on its own object and cannot hop through the sheet; and PlanFreezeSharingHook.
2 Checklist vs code: ✅ consistent — docs/04 #18 now carries the four implementation notes and points at T54–T57; README gained the per-plan / reconcile / read-only-after-close paragraph, matching the code.
3 Forbidden traces: ✅ none — no node_modules/platform edits, no patches, tree clean.
4 Hard rulings landed: ✅ — the two new lookup fields are not numeric, so the four-part numeric declaration does not apply; new admin-facing copy (…及其下级, (已结束,只读)) is business language and now states the true grant width.

Findings — round-1 items:
🔴 1 revocation | CLOSED | Rules are now per plan: names are prefixed kpi_p<planSlug>_ (sharing-service.ts:84) and every criteria carries the plan id (sharing-service.ts:109-224; test/sharing.test.ts "每条规则的条件都带方案" asserts criteria.plan on all intents). reconcilePlanSharing (sharing-service.ts:314-334) lists the org's rules, keeps only names matching this plan's prefix or the legacy kpi_share_ prefix (:325), and sets active: false on any that is not in the desired set — skipping rows already inactive or customized. It is wired on both write paths: PlanPublishHook → provisionPlanSharing(api, id, { reconcile: true }) (plan.hook.ts:181) and the new PlanFreezeSharingHook on close/archive (plan.hook.ts:238-256). The replaced-leader hole is closed twice over — a new plan version gets a new prefix, so the old leader's rule is scoped to the old plan by its own criteria, and it is not in the new plan's desired set. Evidence is not weakened: test/sharing.test.ts adds a four-case "对账" block (leader swap, subject removal, assignment removal, legacy-prefix disjointness) and a three-case read-only block; e2e T54 asserts the new plan has a rule for D and none for C by scanning the rule table, T55 that C sees the old plan's sheet but not the new one, T56 that D sees the new one. PlanFreezeSharingHook is afterUpdate and fully wrapped in try/catch, so closing a plan cannot fail on a sharing error.
🟡 2 active / customized | CLOSED | active is gone from the update payload and appears only on the insert (sharing-service.ts:256-259); a customized === true row returns skipped_customized and a manually deactivated row returns skipped_inactive (:262-263), both counted and logged at info rather than overwritten. An admin's withdrawal now survives every re-assert.
🟡 3 leader results | CLOSED | A second leader rule ${prefix}result_leader_<unit>_<leader> with criteria: { plan, unit }, recipient user, read (sharing-service.ts:180-190) — the department/branch rows a leader could not reach before. Unit-tested by name and shape; e2e T53 signs in as C and asserts the 市场部 department row is visible and a non-managed unit's row is not. README and the permission-set docstring are now true.
🟡 4 organization_id on update | CLOSED | organization_id moved into the shared payload, so both insert and update write it (sharing-service.ts:256). A unit with no organization is now refused outright with KPI_SHARING_NO_ORG (:356), counted as failed, logged, and skipped — non-blocking, so publish still proceeds.
🟡 5 org-scoped lookup | CLOSED | findOne({ where: { name, organization_id } }) (sharing-service.ts:247), matching the object's { fields: ['name'], unique: 'organization' } index. reconcilePlanSharing likewise scopes its scan by organization_id (:321).
🟡 6 env gate | CLOSED | Now an allowlist over the platform's own determination: resolveEnvironmentMode returns env.NODE_ENV || 'production' and isDemoEnvironment tests membership in {dev, development, test} (align-demo-units.ts:39-59). I verified the platform really spells it that way — cli/src/commands/start.ts:161-162 (NODE_ENV === 'test' ? 'test' : (NODE_ENV || 'production')) and :419-421 (forces production only when unset), cli/src/commands/serve.ts:1706-1708 (--dev sets development when unset, otherwise production). And the fixture still runs under pnpm dev: os dev spawns serve with --dev (cli/src/commands/dev.ts:460) which sets NODE_ENV='development' before boot, and vitest sets test. Four new unit cases pin unset/production/dev-development-test/staging. The header's side note also checks out — runtime/src/seed-loader.ts contains no env gate at all, so the seed's own env: ['dev','test'] is not enforced and this guard is the only one.

Round-1 ⚪ items also addressed: the adjustment hook's regenerateResults is now inside the try/catch alongside the sharing re-assert (adjustment.hook.ts:70-76), T39 moved from >= 21 to an exact derived count (25) plus a new T39b asserting every rule's criteria carries the plan id, and the rule labels now say …及其下级, matching the unit_and_subordinates width.

Regressions introduced by the fixes: none blocking. New writes are confined to two readonly lookups populated by the system context at insert (sheet.hook.ts:171 for check tasks, adjustment.hook.ts:31 for adjustments, the latter mirroring the existing subject stamp), and the one new hook is afterUpdate + try/catch. No new e2e wait passes on timeout: T53 and T57 both re-read after waitUntil and assert on the re-read, so a timeout fails the case.

Findings (new, all record-only):
⚪ | src/services/sharing-service.ts:325 | The legacy kpi_share_* deactivation branch is never exercised: a fresh e2e database contains no legacy rules, and T50 only asserts none are present. The branch is correct by inspection but unproven — it matters only for a database that ran the previous build.
⚪ | src/objects/entry.object.ts:182,330 | kpi_check_task / kpi_adjustment rows created before this change carry plan = null and therefore match none of the new criteria, so their sharing silently disappears on an existing database. Invisible to a fresh-DB e2e; a one-off backfill (or a plan-scoped re-stamp) is what a live instance would need.
⚪ | src/services/sharing-service.ts:84,175 | Rule-name length is close to the platform's maxLength: 100. Worst case — a 32-char lowercase plan id (kept verbatim by ruleSlug) plus a 32-char lowercase unit id plus a hashed leader id — reaches ~104 on ${prefix}sheet_leader_<unit>_<leader> and would be rejected by validation, silently costing that leader access. The unit test only exercises short seeded ids; capping ruleSlug output for the plan segment would remove the edge entirely.
⚪ | src/services/sharing-service.ts:263 | skipped_inactive cannot distinguish "an admin withdrew this" from "our own reconcile deactivated it", so a rule the app deactivated can never be revived by the app. No reachable path today (publish is a one-shot draft → published, and closed → published is not a legal transition per plan.object.ts:80-83), but the two provenances deserve separating before a re-publish path exists.
⚪ | src/services/sharing-service.ts:352-357 | The null-organization refusal is stricter than the platform requires: BusinessUnitGraphService only applies an organization filter when organizationId is truthy (business-unit-graph.ts:270), so a null-org rule expands correctly on a deployment whose units are also null-org. On such a deployment the app now writes no sharing rules at all and says so only in a server log. Harmless in this app's dev path (the fixture guarantees an organization) and the safer direction, but worth a second look before a single-posture install.
⚪ | PR description, "What changed" table | Still says "reporter row-level security restricted to the person dimension"; the change is a widening. Code comment and docs/04 are correct — only the PR body reads backwards.

---

Generated by Claude Code