Skip to content

Commit aae0cb3

Browse files
claude[bot]claude
andauthored
fix(plugin-sharing): let system writes materialize sharing rules — drop the isSystem skips in bindRuleHooks (#13533) (#14528)
* fix(plugin-sharing): let system writes materialize sharing rules `bindRuleHooks` returned early on `ctx.session.isSystem` in `afterInsert` and `afterUpdate`, so a system-context write that moved a record into a sharing rule's criteria materialized no `sys_record_share` row. Approval write-back is exactly that write — an approval node with `lockRecord: true` mirrors the decision onto the subject record under a system context, because while the record is locked no other write can land — so a teammate who depended on the criteria rule could not see an approved record until somebody re-evaluated the rule or restarted the server. Removes three early returns: the two named above, and the one on the `beforeUpdate` / `beforeDelete` row-set stash they depended on. That third one is load-bearing: without a stash `readAffectedRows` reports the row set as `unbounded`, which would have sent every single-row system update down the object-wide revoke plus asynchronous re-grant branch instead of the per-record recompute. Retires the `SYSTEM_WRITE_SKIP_NOTICE` channel that announced the skip. `afterDelete` keeps its skip on separate grounds — its payload is revocation, which `record-share-cascade.ts` delivers for system writes on its own account. The two pins of the old behaviour are reversed and registered rather than deleted: `system-write-skip-notice.test.ts` is renamed to `system-write-materialisation.test.ts` and carries a row-by-row register of every old expectation against its new counterpart, and `bulk-recompute.test.ts` keeps its case under a docblock recording what it used to assert and why. `content/docs/permissions/system-context.mdx` is updated because `check:system-context-census` requires it: the page anchors every `isSystem` read site and states census-derived counts, and three sites vanished. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * chore(sharing): regenerate the isSystem census page and the engine-double ledger after merging main Discharges the os-regen deferral the merge commit recorded. `content/docs/permissions/system-context.mdx` is os-regen-driven and both sides moved it, so the merge sequence took main's side and this commit re-applies the branch's hand-written half on top: row 30 now describes the one surviving skip (`afterDelete`, whose payload `record-share-cascade.ts` delivers) instead of all four, rough edge 2 records the reversal rather than the retired INFO notice, and the seven census-derived counts move with the population. Re-derived on the merged tree rather than carried over — `check:system-context-census` reports "OK — 106 elevation read sites in 20 packages across 45 files, all anchored; 140 anchors resolve, 27 declared non-read". `scripts/engine-double-contract.pinned.json` learns the test file's new path. The two rows are repathed, not renumbered: `--write` reports "692 (file, verb) row(s), 2 added or grown, 2 lost. No seam losses — this regeneration only records new seams." Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * chore(sharing): re-anchor the isSystem census page after the second main merge Patch round 1. `content/docs/permissions/system-context.mdx` is os-regen-driven and both sides moved it again, so the merge stopped there and neither side could simply win: main's #14383 (#12981 batch 9) moved row 37's `share-link-service.ts` anchors by +6, while this branch moved row 36's `sharing-plugin.ts` anchor by +11 and rewrote row 30 plus rough edge 2. The resolution takes row 37 from main and row 36 from this branch — the two sides moved different rows, so there was no real overlap, only the driver's refusal to text-merge a generated page. Verified by re-deriving rather than by inspection: `check:system-context-census` reports "OK — 106 elevation read sites in 20 packages across 45 files, all anchored; 140 anchors resolve, 27 declared non-read". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * docs(sharing): patch round 2 — deferred-revoke precision, bu-tree docblock, operator cost sentence Documentation-only follow-up on the contract review's non-blocking §5 notes 2, 3 and 4. No code path changes: the only .ts edit is a docblock body. - system-context.mdx row 30: "Lose: nothing" glossed the one shape where the cascade's delivery is the queued orphan sweep rather than a synchronous revoke. The cell now says the revoke is delivered but deferred on the unbounded shape. - bu-tree-recompute.ts: the docblock still described bindRuleHooks' materialisation skip as current. It now says what bindRuleHooks does after this change and why this file's own hooks never skipped system writes. - changeset: one sentence on the operator-visible cost of seed- and import-time system writes on rule-covered objects, and "unexported" becomes "not exported from the package entry point". Level stays patch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * chore(sharing): re-anchor the isSystem census page after the fourth main merge Discharges the os-regen deferral recorded by the merge commit. Pure re-anchoring: 20 insertions / 20 deletions, all of them line numbers the generator re-derived from the merged tree (main moved engine.ts, sharing-service.ts, auth-plugin.ts, rest-server.ts and last-admin-guard.ts). No prose changed on either side -- normalising every digit away leaves the before/after byte-identical. The engine-double-contract ledger needed no repair: its textual auto-merge was already byte-exact, and `--write` reported 697 rows with 0 added, 0 grown and 0 lost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent b3a63d3 commit aae0cb3

10 files changed

Lines changed: 759 additions & 519 deletions
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
---
2+
"@objectstack/plugin-sharing": patch
3+
---
4+
5+
fix(plugin-sharing): let system writes materialize sharing rules — drop the `isSystem` skips in `bindRuleHooks` (#13533)
6+
7+
A criteria sharing rule declares a promise: `status == "approved"` means the
8+
named recipients can see the record. `bindRuleHooks` did not keep that promise
9+
when the platform was the writer. Its `afterInsert` and `afterUpdate` hooks
10+
returned early on `ctx.session.isSystem`, so a system-context write that moved a
11+
record INTO a rule's criteria materialized no `sys_record_share` row at all.
12+
13+
The path that made this a real outage rather than a boot-time curiosity is
14+
approval write-back. An approval node with `lockRecord: true` mirrors the
15+
decision onto the subject record under a system context — that is the only write
16+
that can land while the record is locked — so a manager approving a leave
17+
request produced exactly the skip above. A teammate who relied on the rule could
18+
not see the record, and nothing repaired it until somebody ran
19+
`POST /api/v1/sharing/rules/:id/evaluate` or restarted the server. The failure
20+
was invisible from a manager or admin view, which reads through the profile's
21+
`viewAllRecords` grant and never consults a sharing rule at all.
22+
23+
**What changes.** A system write now materializes exactly as a user write does —
24+
grants on the way into a rule's criteria, revokes on the way out, per record,
25+
synchronously with the write. Three early returns are gone: the two on
26+
`afterInsert` / `afterUpdate`, and the one on the `beforeUpdate` / `beforeDelete`
27+
row-set stash they depended on (without that stash an `after` hook reads the row
28+
set as *unbounded*, which would have turned every single-row system update into
29+
an object-wide revoke plus an asynchronous re-grant). The INFO line that
30+
announced the skip — `[sharing-rule] sharing materialisation skipped for isSystem
31+
writes; re-evaluate rules or restart to backfill` — is retired with it, along
32+
with the `SYSTEM_WRITE_SKIP_NOTICE` constant, which was not exported from the
33+
package entry point. Operationally this means seed- and import-time system writes
34+
on rule-covered objects now pay per-record sharing evaluation at write time — the
35+
cost a user write of the same shape has always paid, with the
36+
`kernel:bootstrapped` backfill still reconciling behind it.
37+
38+
**What does not change.** `afterDelete` still skips system writes, on separate
39+
grounds: what it skips is revocation, and `record-share-cascade.ts` delivers that
40+
on every sharing-capable object, stashing for system writes on its own account.
41+
The `kernel:bootstrapped` boot backfill still runs and is still needed — it
42+
reaches rows no hook saw, and it is the only pass that purges a deactivated
43+
rule's grants. No new option, flag or declarative switch: the fix is the removal.
44+
45+
No published export moves; `SYSTEM_WRITE_SKIP_NOTICE` was never re-exported from
46+
the package entry point.

‎content/docs/permissions/system-context.mdx‎

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ the seed loader replaying package fixtures, a plugin's boot reconciler, a
99
service self-write, a migration.
1010

1111
This page is **the authority** for what that flag actually does. It exists
12-
because the flag is not one concept: it is a single boolean read at **109
12+
because the flag is not one concept: it is a single boolean read at **106
1313
distinct sites across 20 packages**, and knowing three of those behaviours gives
14-
no hint that the other hundred-and-six exist. Every documented app-side bug
14+
no hint that the other hundred-and-three exist. Every documented app-side bug
1515
traced to `isSystem` had the same shape — the metadata was complete and correct,
1616
and the gap was observable only by querying the resulting rows.
1717

@@ -124,18 +124,18 @@ that silently does not happen.
124124

125125
### 3. Sharing (`plugin-sharing`)
126126

127-
The largest single consumer — **20 of the 109 sites**.
127+
The largest single consumer — **17 of the 106 sites**.
128128

129129
| # | Behaviour when `isSystem` | What you get / what you lose | Anchor |
130130
|:--|:---|:---|:---|
131-
| 30 | **Sharing-rule grant materialisation is skipped on all four record-write hooks** | Lose: **no `sys_record_share` rows are created**. A fully configured sharing rule grants **nothing** on seeded data until a rule is re-evaluated or the boot backfill runs. This is the behaviour that motivated #4707. Since #6783 the skip is no longer silent — it emits an INFO notice (rough edge 2) | `rule-hooks.ts:250`, `:274`, `:293`, `:322` |
131+
| 30 | **Sharing-rule REVOCATION is skipped on the record-`afterDelete` hook** — and on that hook only | Lose: nothing permanently — the revoke is **delivered, but deferred on the unbounded shape**. The payload belongs to another subscriber: `record-share-cascade.ts` binds on every sharing-capable object and stashes for system writes on its own account (#5103). When the deleted ids are enumerable it revokes inline; when they are not — a predicate delete whose row set the stash could not resolve — it hands the reclaim to a queued background orphan sweep instead, so the share rows outlive the deleted records until that sweep runs, with the boot orphan sweep behind it. No surviving record loses access either way, and a restart re-runs the same sweep. This is one subscriber declining work another owns, not elevation silencing a consequence. ⚠️ **Grant MATERIALISATION no longer asks** — the `afterInsert` / `afterUpdate` skips, and the `before*` stash skip that fed them, were removed by the 2026-08-31 ruling on #13533; a system write materialises exactly as a user write does | `rule-hooks.ts:292` |
132132
| 31 | Sharing write verdict short-circuits to `allow` | Get: writes pass the sharing gate unconditionally | `plugin-sharing/src/sharing-service.ts:677` |
133133
| 32 | Record visibility / manage-shares checks return true | Get: no ownership or Modify-All requirement | `plugin-sharing/src/sharing-service.ts:943`, `:1030`, `:1787` |
134134
| 33 | `grant()` skips the enforcement + manage-shares assertions | Get: the rule evaluator can materialise through the public API. Note it is **not** a bare skip: the system branch asserts the grant is not *inert* instead (a grant on an object no verdict can consult is refused) | `plugin-sharing/src/sharing-service.ts:1238` |
135135
| 34 | `revoke()` deletes directly, **before** the non-manual-source guard | Get: the evaluator can revoke its own grants. Lose: the `CONFLICT` guard that warns a rule-materialised grant will be silently re-granted on the next reconcile | `plugin-sharing/src/sharing-service.ts:1476` (guard at `:1501`) |
136136
| 35 | `listShares()` skips the management gate | Get: full enumeration of who can see a record | `plugin-sharing/src/sharing-service.ts:1528` |
137-
| 36 | `sys_record_share` reads are **not** self-scoped | Get: tenant-wide share listing without `manage_sharing` | `sharing-plugin.ts:1077` |
138-
| 37 | Share-link policy `enabled` check bypassed; system callers re-enter under a system context | Get: link creation/resolution while the policy is off | `plugin-sharing/src/share-link-service.ts:449`, `:503`, `:507`, `:580`, `:610` |
137+
| 36 | `sys_record_share` reads are **not** self-scoped | Get: tenant-wide share listing without `manage_sharing` | `sharing-plugin.ts:1088` |
138+
| 37 | Share-link policy `enabled` check bypassed; system callers re-enter under a system context | Get: link **creation** while the policy is off — resolution is **not** bypassed since #14033 (`publicSharing.enabled` is a standing policy held at every redemption): a link minted this way does not resolve until the block is enabled | `plugin-sharing/src/share-link-service.ts:449`, `:503`, `:507`, `:580`, `:610` |
139139
| 38 | Sharing-rule provenance stamp skipped | Lose: the row is not marked as an admin customization — seeder / `defineRule` / boot reconcilers are "the package door" | `sharing-rule-provenance.ts:47` |
140140
| 39 | Sharing-rule service write + delete paths return early | Lose: the manage-rules gate on the service surface, and the platform-global-rule delete guard | `sharing-rule-service.ts:157`, `:382` |
141141

@@ -217,14 +217,32 @@ should recognise it instead of re-deriving it.
217217
(`plugin-security/src/claim-seed-ownership.ts`). Any *third* system write
218218
path gets neither. If you add one, stamp ownership yourself.
219219

220-
2. **Sharing materialisation is skipped, and now says so.** Row 30 produces
220+
2. **Sharing materialisation is no longer skipped — the rough edge is closed,
221+
and it is recorded here rather than deleted.** This entry used to describe
221222
"configured but inert": nine installed sharing rules, matching records,
222-
correct positions — and `sys_record_share` empty. The boot backfill does
223-
eventually fix it, so the behaviour is not wrong; it was undiscoverable.
224-
The INFO notice tracked as #6783 **has since shipped**
225-
(`rule-hooks.ts:274`, `:293` call `noteSystemWriteSkipped`), so the skip is
226-
now observable — the earlier edition of this page said it was not, and that
227-
sentence was already stale when the anchors were re-censused.
223+
correct positions — and `sys_record_share` empty, repaired only by a
224+
re-evaluation or a restart. The reading that made it a rough edge rather
225+
than a defect was that the boot backfill eventually fixes it, so the
226+
behaviour was correct and merely undiscoverable; #6783 shipped an INFO
227+
notice on that reasoning.
228+
229+
That reading did not survive contact with a runtime write. An approval
230+
node's write-back is a system write — `lockRecord: true` means only a
231+
platform write can land while the record is locked — and it happens long
232+
after boot, so no backfill was coming: a manager approved a request and the
233+
teammate who depended on the criteria rule `status == "approved"` could not
234+
see it at all. The maintainer ruled on 2026-08-31 (#13533) that the skip was
235+
a **bug**, because a sharing rule's declared semantics is a published
236+
promise and `isSystem` names the operator, never a consequence that need not
237+
happen. Both materialisation skips and the notice that announced them are
238+
gone; row 30 is now the `afterDelete` skip alone, which survives on the
239+
separate ground that another subscriber delivers that payload.
240+
241+
⚠️ The observability half of that reading is worth keeping in mind
242+
independently: the only signal a builder ever had was one INFO line, and
243+
nothing in the docs or the tests said "after approval, when does the team
244+
see it?". A compensating path that exists but that nobody can be expected to
245+
know about is not a compensating path.
228246

229247
3. **Strict write observability is inert under elevation.** Row 22: a caller
230248
that asked to be told loudly about dropped fields is told nothing, because
@@ -251,7 +269,7 @@ Ownership injection, `readonly` bypass and sharing materialisation are
251269
independent decisions, and a seed loader plausibly wants the first two but not
252270
the third. The concept is nevertheless **staying as one boolean**:
253271

254-
- **Shipped semantics.** `isSystem` is a published contract with 109 read sites
272+
- **Shipped semantics.** `isSystem` is a published contract with 106 read sites
255273
in 20 packages. Splitting it is a breaking contract change across all of them.
256274
(The ruling was taken when the census read 80 sites in 18 packages; the count
257275
has grown, which strengthens rather than weakens the argument.)
@@ -308,12 +326,12 @@ still holds equal to the census on every pull request:
308326
| Appearances of the bare identifier `isSystem` in non-test sources | 813 | — |
309327
| — parsed as a declaration | 22 | ✅ |
310328
| — parsed as an object-literal / type key (producers and option objects) | 310 | — |
311-
| — parsed as a property **read** | 115 | ✅ |
329+
| — parsed as a property **read** | 112 | ✅ |
312330
| — parsed in some other syntactic position (a local, a cast, a conditional) | 9 | ✅ |
313331
| — the remainder: text inside comments and string literals | 358 | — |
314332
| Of those reads: reads of one of the unrelated metadata fields | 6 | ✅ |
315-
| Of those reads: reads of `ExecutionContext.isSystem` | **109** | ✅ |
316-
| — behaviour-bearing (rows 1–61 above) | 105 | ✅ |
333+
| Of those reads: reads of `ExecutionContext.isSystem` | **106** | ✅ |
334+
| — behaviour-bearing (rows 1–61 above) | 102 | ✅ |
317335
| — carry the flag onward only (rows 62–65 above) | 4 | ✅ |
318336
| Packages containing at least one elevation read | **20** | ✅ |
319337
| Files containing at least one elevation read | 45 | ✅ |

‎packages/plugins/plugin-sharing/src/boot-backfill.test.ts‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,17 @@
33
/**
44
* [#2926 ③] Boot backfill of sharing-rule grants.
55
*
6-
* Rule grants are materialized by write hooks, which deliberately skip
7-
* `isSystem` writes (rule-hooks.ts) — so records created by the boot-time
8-
* seed loader (always `isSystem`) never produced `sys_record_share` rows:
9-
* demo data shipping with matching sharing rules was broken out of the box
10-
* until an admin "touched" each record at runtime. `backfillRuleGrants`
11-
* reconciles every active rule once per boot, idempotently.
6+
* Rule grants are materialized by write hooks, and this pass reconciles every
7+
* rule once per boot, idempotently, for the rows those hooks did not reach.
8+
*
9+
* [#13533] The original reason for that gap was that the hooks skipped
10+
* `isSystem` writes, so boot-time seed rows (always `isSystem`) never produced
11+
* `sys_record_share` rows and demo data shipped inert. That skip is gone — a
12+
* system write now materialises like any other — but this pass is NOT
13+
* obsolete, and the two remaining reasons are what these tests cover: rows
14+
* written while a rule was inactive or before its hooks were bound are still
15+
* unreached by any hook, and this is the only pass that PURGES a deactivated
16+
* rule's grants (#4433), which no write-path hook can do.
1217
*/
1318

1419
import { describe, it, expect, beforeEach, vi } from 'vitest';

‎packages/plugins/plugin-sharing/src/bu-tree-recompute.ts‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -205,13 +205,17 @@ export function writeCanChangeExpansion(objectName: string, event: string, hookC
205205
* nothing here needs the pre-write state. What the revoke needs is the tree as
206206
* it is NOW, and the write is what makes that true.
207207
*
208-
* **System writes are NOT skipped**, which is the opposite of what
209-
* `bindRuleHooks` does and is deliberate. Its skip is about grant
210-
* MATERIALISATION, which the boot backfill re-does anyway. Here the payload is
211-
* REVOCATION, and the realistic production trigger for a re-parent is an HRIS
212-
* or directory sync — a system write. Skipping those would leave the hole open
213-
* on the very path most likely to open it. `primary-bu-projection.ts` reached
214-
* the same conclusion for the same table.
208+
* **System writes are NOT skipped**, and this file's hooks never carried an
209+
* `isSystem` branch to skip them with. `bindRuleHooks` no longer skips them
210+
* either: its `afterInsert` / `afterUpdate` materialisation skips, and the
211+
* `before*` stash skip that fed them, were removed by the 2026-08-31 ruling on
212+
* #13533, so a system write there materialises grants exactly as a user write
213+
* does — the one skip it keeps is `afterDelete` revocation, which
214+
* `record-share-cascade.ts` delivers instead. Here the payload is REVOCATION,
215+
* and the realistic production trigger for a re-parent is an HRIS or directory
216+
* sync — a system write. Skipping those would leave the hole open on the very
217+
* path most likely to open it. `primary-bu-projection.ts` reached the same
218+
* conclusion for the same table.
215219
*/
216220
export function bindBusinessUnitTreeRecompute(
217221
engine: MinimalEngine,

‎packages/plugins/plugin-sharing/src/bulk-recompute.test.ts‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,11 @@ import {
4040
interface Row { [k: string]: any }
4141

4242
const SYS = { isSystem: true, positions: [], permissions: [] } as any;
43-
/** A non-system session — the hooks deliberately skip `isSystem` writes. */
43+
/**
44+
* The default session for this suite: an interactive admin. [#13533] It is no
45+
* longer the only session the hooks act on — a system write takes the same
46+
* path — so this is a default, not a precondition.
47+
*/
4448
const ADMIN_SESSION = { isSystem: false, userId: 'admin' };
4549

4650
type HookEntry = { event: string; handler: (ctx: any) => any; options: Row };
@@ -343,16 +347,37 @@ describe('#4779 predicate (multi) writes recompute sharing rules', () => {
343347
expect(ruleShares(engine)).toEqual([]);
344348
});
345349

346-
it('leaves system-context bulk writes to the boot backfill, as before', async () => {
350+
/**
351+
* [#13533] REVERSED, not deleted. This test used to read:
352+
*
353+
* it('leaves system-context bulk writes to the boot backfill, as before')
354+
* … expect(ruleShares(engine)).toHaveLength(2); // untouched by the hooks
355+
*
356+
* and it pinned a real behaviour: `bindRuleHooks` skipped `session.isSystem`
357+
* on `afterUpdate` (and on the `before*` stash that fed it), so a system bulk
358+
* write changed no grants and `kernel:bootstrapped`'s backfill owned the
359+
* repair. The maintainer reversed that on 2026-08-31 (verbatim, untranslated:
360+
* 「裁定:系统写参与逐记录共享物化 —— 删除 `plugin-sharing` 两个钩子里的
361+
* `isSystem` 跳过,⛔ 不加声明式开关、不以文档代修。」), on the ground that a
362+
* rule's declared semantics is a published promise and `isSystem` names the
363+
* OPERATOR, never a consequence that need not happen.
364+
*
365+
* So the same write is now expected to do what the identical user-context
366+
* write two tests up does — and the assertion below is deliberately the same
367+
* shape as that one, because "the same" is the whole content of the ruling.
368+
*/
369+
it('recomputes system-context bulk writes too — identically to a user write (#13533)', async () => {
347370
seed(2, 'east');
348371
await rules.evaluateRule('srule_east', SYS);
372+
expect(ruleShares(engine)).toHaveLength(2);
349373

350374
await engine.simulateBulkUpdate(
351375
'opportunity', { region: 'east' }, { region: 'west' }, { isSystem: true },
352376
);
353377

354-
// Untouched by the hooks — `kernel:bootstrapped`'s backfill owns seeds.
355-
expect(ruleShares(engine)).toHaveLength(2);
378+
// Moved OUT of the criteria, so the grants are revoked — exactly as the
379+
// user-context twin above revokes them.
380+
expect(ruleShares(engine)).toEqual([]);
356381
});
357382

358383
it('does not touch manual shares — only rule-materialised ones', async () => {

0 commit comments

Comments
 (0)