Skip to content

Commit fd6572b

Browse files
os-zhuangclaude
andauthored
feat(plugin-sharing): one INFO line when isSystem writes materialise zero sharing grants (#6783) (#6963)
Demand 3 of #4707, maintainer-ruled 2026-08-06. The sharing-rule record-write hooks skip isSystem sessions, so a seed run lands rows on an object an ACTIVE rule covers and creates no sys_record_share rows. The skip is correct — the kernel:bootstrapped backfill heals it — but it was completely silent, which is indistinguishable from a broken sharing configuration (hotcrm#640). afterInsert and afterUpdate now emit SYSTEM_WRITE_SKIP_NOTICE once per object per hook-binding generation, carrying the ruled wording verbatim plus the object and its active rules. One line per batch, never per row. Deliberately unchanged: the skip itself, the absence of any new switch, and afterDelete's silence — a delete skips revocation, not materialisation, and no re-evaluation or restart can reach a grant whose record is gone. Claude-Session: https://claude.ai/code/session_01BM1tNf5U3nEbHKR4fo5qVQ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 10ec2f0 commit fd6572b

3 files changed

Lines changed: 526 additions & 3 deletions

File tree

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
---
2+
"@objectstack/plugin-sharing": patch
3+
---
4+
5+
feat(plugin-sharing): an `isSystem` write batch that materialises zero sharing grants now says so, once (#6783)
6+
7+
The sharing-rule record-write hooks skip `isSystem` sessions, so a seed run — or
8+
any internal write batch — lands rows on an object an **active** sharing rule
9+
covers and creates no `sys_record_share` rows at all. The skip is correct: the
10+
`kernel:bootstrapped` backfill reconciles every rule and `evaluateRule` is
11+
idempotent, so the state heals. What was wrong is that nothing said so.
12+
13+
hotcrm#640 is the specimen: a fresh install with 9 active sharing rules, 9
14+
accounts matching their criteria, users holding the right positions — and an
15+
empty `sys_record_share`. Every visible layer said "configured". The only way to
16+
learn that the seed path had skipped materialisation was to query the table,
17+
find it empty, and read `plugin-sharing`'s source.
18+
19+
**What changed.** The two skips that drop grant materialisation — `afterInsert`
20+
and `afterUpdate` — now emit one INFO line naming the behaviour and both
21+
remedies:
22+
23+
```
24+
[sharing-rule] sharing materialisation skipped for isSystem writes; re-evaluate rules or restart to backfill
25+
```
26+
27+
with the object and the active rules on it as metadata.
28+
29+
**One line per batch, not per row.** The notice is latched per object per hook
30+
binding generation, so a seed batch writing 500 rows produces exactly one line.
31+
The defect being fixed is silence; a per-row flood would be the same defect with
32+
a different symptom. The latch re-arms with the binding — `bindRuleRebindTriggers`
33+
re-binds the package on every `sys_sharing_rule` write — so a changed rule set
34+
gets its own notice instead of inheriting the previous generation's silence.
35+
36+
**INFO, not warn or error**, deliberately: the behaviour is correct and
37+
self-healing, and warning about a subsystem working as designed is how operators
38+
learn to ignore it.
39+
40+
Deliberately unchanged:
41+
42+
- **The skip itself.** No write now materialises grants that did not before, and
43+
no `sys_record_share` row is created, updated or revoked by this change.
44+
- **No new switch or flag.** The notice is unconditional.
45+
- **`afterDelete` stays silent.** A delete skips *revocation*, not
46+
materialisation, and the remedy the line names cannot repair that class:
47+
`evaluateRule` iterates records that still exist, so neither re-evaluating a
48+
rule nor restarting can reach a grant whose record is gone. That class belongs
49+
to the record-delete share cascade and the boot orphan sweep.
50+
51+
The line is a statement about the write path, not a claim that grants were owed —
52+
whether a given seeded row satisfies a rule's criteria is exactly the query the
53+
skip exists to avoid, so answering it here would cost the skip its purpose.

packages/plugins/plugin-sharing/src/rule-hooks.ts

Lines changed: 98 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,33 @@ export const RULE_REBIND_TRIGGER_PACKAGE = 'plugin-sharing:rule-rebind';
2929
*/
3030
export const RULE_CRITERIA_GUARD_PACKAGE = 'plugin-sharing:rule-criteria-guard';
3131

32+
/**
33+
* [#6783] The one INFO line an `isSystem` write batch gets when it lands rows
34+
* on an object that an ACTIVE sharing rule covers and materialises no grants.
35+
*
36+
* The wording after the tag is the maintainer's, verbatim (ruling on #4707,
37+
* 2026-08-06, demand 3): it names the behaviour AND both remedies, because the
38+
* whole defect being fixed is that neither was discoverable. hotcrm#640 is the
39+
* specimen — a fresh install with 9 active rules, 9 matching accounts and an
40+
* empty `sys_record_share`, where every visible layer said "configured" and
41+
* nothing said "inert". The only way to learn the truth was to query the table,
42+
* find it empty, and read this file.
43+
*
44+
* It is a statement about the WRITE PATH, not a claim that grants were owed:
45+
* whether a given seeded row would have matched a rule's criteria is precisely
46+
* the query the skip exists to avoid, so answering it here would cost the skip
47+
* its reason to exist. Worded this way the line is true in both cases — it says
48+
* materialisation did not run, and where the answer comes from when it does.
49+
*
50+
* INFO, deliberately, not `warn`: the behaviour is CORRECT (the
51+
* `kernel:bootstrapped` backfill in `sharing-plugin.ts` reconciles every rule
52+
* and `evaluateRule` is idempotent), so a warning would train operators to
53+
* ignore a subsystem that is working as designed.
54+
*/
55+
export const SYSTEM_WRITE_SKIP_NOTICE =
56+
'[sharing-rule] sharing materialisation skipped for isSystem writes; ' +
57+
're-evaluate rules or restart to backfill';
58+
3259
interface MinimalEngine {
3360
registerHook(event: string, handler: (ctx: any) => any | Promise<any>, options?: {
3461
object?: string | string[];
@@ -99,6 +126,11 @@ export const ruleRegrantQueue = new RuleRegrantQueue();
99126
* skipped recompute entirely and left stale `sys_record_share` rows granting
100127
* access the rules no longer imply.
101128
*
129+
* [#6783] The two skips that drop GRANT MATERIALISATION (`afterInsert`,
130+
* `afterUpdate`) now emit {@link SYSTEM_WRITE_SKIP_NOTICE} once per object per
131+
* binding generation. The skips themselves are unchanged — the behaviour is
132+
* correct and the boot backfill heals it; only the silence was the defect.
133+
*
102134
* Caller is responsible for invoking {@link unbindAllRuleHooks} before
103135
* re-binding when the rule set changes.
104136
*/
@@ -109,10 +141,55 @@ export function bindRuleHooks(
109141
logger?: MinimalLogger,
110142
): void {
111143
const objects = new Set<string>();
144+
/** Active rule names per object — the `rules:` field of the #6783 notice. */
145+
const activeRuleNames = new Map<string, string[]>();
112146
for (const r of rules) {
113147
if (r.active === false) continue;
114-
if (r.object_name) objects.add(r.object_name);
148+
if (!r.object_name) continue;
149+
objects.add(r.object_name);
150+
const named = activeRuleNames.get(r.object_name) ?? [];
151+
named.push(String(r.name ?? r.id ?? ''));
152+
activeRuleNames.set(r.object_name, named);
115153
}
154+
155+
/**
156+
* [#6783] Objects whose current silent window has already been reported.
157+
*
158+
* Scoped to this binding generation on purpose. The signal being added is
159+
* "materialisation did not run here", which is a property of the OBJECT and
160+
* of the rule set bound to it — not of the row — so a seed batch of N rows
161+
* must produce ONE line, never N. The failure mode being fixed is silence;
162+
* trading it for a per-row flood would replace one defect with another, and
163+
* an operator who scrolls past the line is exactly as uninformed as one who
164+
* was never told.
165+
*
166+
* The latch re-arms with the binding: `bindRuleRebindTriggers` unbinds and
167+
* re-binds this whole package on every `sys_sharing_rule` write, so a rule
168+
* set that changed gets its own notice rather than inheriting the previous
169+
* generation's silence.
170+
*/
171+
const notified = new Set<string>();
172+
173+
/**
174+
* Emit {@link SYSTEM_WRITE_SKIP_NOTICE} at most once per object per binding
175+
* generation. Never throws: this runs on the write path ahead of the hooks'
176+
* own `try`, and a logger that throws must not fail an operator's write. The
177+
* latch is claimed BEFORE the log so a throwing logger cannot turn one
178+
* suppressed line into one throw per row.
179+
*/
180+
const noteSystemWriteSkipped = (objectName: string): void => {
181+
if (notified.has(objectName)) return;
182+
notified.add(objectName);
183+
try {
184+
logger?.info?.(SYSTEM_WRITE_SKIP_NOTICE, {
185+
object: objectName,
186+
rules: activeRuleNames.get(objectName) ?? [],
187+
});
188+
} catch {
189+
/* a logger that throws must not fail the write */
190+
}
191+
};
192+
116193
for (const objectName of objects) {
117194
const opts = { object: objectName, packageId: SHARING_RULE_HOOK_PACKAGE, priority: 180 };
118195

@@ -162,7 +239,11 @@ export function bindRuleHooks(
162239
const affectedFrom = (ctx: any): AffectedRows => readAffectedRows(ctx);
163240

164241
engine.registerHook('afterInsert', async (ctx: any) => {
165-
if ((ctx?.session as any)?.isSystem) return;
242+
if ((ctx?.session as any)?.isSystem) {
243+
// [#6783] The skip stays exactly as it was; it just stops being silent.
244+
noteSystemWriteSkipped(objectName);
245+
return;
246+
}
166247
try {
167248
const data = ctx?.result ?? ctx?.input?.data ?? {};
168249
const id = String((data as any)?.id ?? ctx?.input?.id ?? '');
@@ -177,7 +258,13 @@ export function bindRuleHooks(
177258
engine.registerHook('beforeDelete', stashAffectedRows, opts);
178259

179260
engine.registerHook('afterUpdate', async (ctx: any) => {
180-
if ((ctx?.session as any)?.isSystem) return;
261+
if ((ctx?.session as any)?.isSystem) {
262+
// [#6783] An `isSystem` update INTO a rule's criteria owes grants the
263+
// same way an insert does, and `evaluateRule` is diff-based, so the
264+
// notice's remedy is true for both directions of an update.
265+
noteSystemWriteSkipped(objectName);
266+
return;
267+
}
181268
try {
182269
const affected = affectedFrom(ctx);
183270
if (affected.kind === 'rows') {
@@ -191,6 +278,14 @@ export function bindRuleHooks(
191278
}, opts);
192279

193280
engine.registerHook('afterDelete', async (ctx: any) => {
281+
// [#6783] Deliberately silent, unlike the insert/update skips above.
282+
// What a delete skips is REVOCATION, not materialisation, and the
283+
// notice's remedy would be false here: `evaluateRule` iterates records
284+
// that still exist, so no re-evaluation and no restart can reach a grant
285+
// whose record is gone (the orphan named at the tail of #4779). That
286+
// class is owned by `record-share-cascade.ts` — which stashes for system
287+
// writes on its own account (#5103) — and by the boot orphan sweep, so
288+
// an INFO line here would point an operator at a repair that cannot run.
194289
if ((ctx?.session as any)?.isSystem) return;
195290
try {
196291
const affected = affectedFrom(ctx);

0 commit comments

Comments
 (0)