feat(sharing): criteria_json is declaratively required — ADR-0113 P2, the first consumer (P1 browser-verified) (#4044)

The field ADR-0113 was written for, cashing in the split #4031 landed.

sys_sharing_rule.criteria_json flips to required: true — the posture that was un-declarable before the split: write contract only (insert must provide, update may not null out, rows predating the gate rest, an admin can still active:false an over-broad legacy rule), deliberately NO storage.notNull because deployed tenants' legacy null rows are the case the split exists for.

The #3929 guards consolidate by layer: the declaration owns missing/null/explicit-clear plus the Setup form's marker and client validation; bindRuleCriteriaGuard narrows to the non-null match-all shapes required cannot express ('{}', vacuous $and/$or, unparsable JSON — the shape-naming message still wins, hooks fire before validation); defineRule keeps the API seam; the evaluator stays fail-closed (ADR-0049). Empty-state registry entry carries the declarative pointer.

P1 verified in the browser with zero Console changes (objectui form-view bridge is already metadata-driven): the served metadata carries criteria_json.required: true, and the Setup sharing-rule form renders Criteria* — positive control Object*, negative control Description. The dogfood legacy-row fixture moved to the driver seam: with the declaration in place, unbinding the hook no longer fakes a pre-gate row through the engine — the defense-in-depth the flip bought, caught by CI.

plugin-sharing 198/198; the bound sharing-rule-criteria-required proof 5/5 against a booted stack; check:empty-state + check:liveness green; patch changeset (not breaking — the same writes were already rejected by the hook).

Author: os-zhuang

.changeset/criteria-json-declaratively-required.md

@@ -0,0 +1,17 @@
+---
+"@objectstack/plugin-sharing": patch
+---
+
+feat(sharing): `sys_sharing_rule.criteria_json` is declaratively required (ADR-0113 P2)
+
+The field the ADR was written for: `required: true` as the write contract —
+insert must provide, update may not null out, legacy null rows rest, an admin
+can still `active: false` an over-broad legacy rule. Deliberately NO
+`storage.notNull`: deployed tenants' legacy nulls are the case the split
+exists for. The Setup form's required marker and client validation now derive
+from the declaration.
+
+Not breaking: a rule without criteria was already rejected by the #3929 hook
+guard; the guard narrows to the non-null match-all shapes `required` cannot
+express ('{}', vacuous $and/$or, unparsable JSON), `defineRule` keeps the API
+seam, and the evaluator stays fail-closed (ADR-0049).

packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts

@@ -136,13 +136,18 @@ export const SysSharingRule = ObjectSchema.create({
 
     criteria_json: Field.textarea({
       label: 'Criteria',
-      // [#3896] Mandatory in substance, `required: false` in metadata: the
-      // column is nullable in every already-deployed tenant (rows predating
-      // this gate), and flipping it to `required` would only translate into a
-      // destructive NOT NULL migration that those nulls block. The invariant
-      // is enforced where it can also explain itself — `bindRuleCriteriaGuard`
-      // fails the INSERT, and `defineRule` fails the API call.
-      required: false,
+      // [#3896 → ADR-0113] Mandatory in substance AND now in metadata. Under
+      // the old tri-binding this was un-declarable — `required: true` implied
+      // a NOT NULL migration that every deployed tenant's legacy null rows
+      // block. ADR-0113 split the knob: this is the WRITE contract only (an
+      // insert must provide it, an update may not null it out, rows predating
+      // the gate rest — an admin can still `active: false` an over-broad
+      // legacy rule). Deliberately NO `storage.notNull`: those legacy nulls
+      // are exactly the case the split exists for. The declaration is the
+      // first line; `bindRuleCriteriaGuard` still rejects the NON-null
+      // match-all shapes `required` cannot express, and `defineRule` guards
+      // the API path with the shape-naming message.
+      required: true,
       // Rendered as a visual criteria builder scoped to the selected object's
       // fields (dependsOn: object_name), storing the same JSON FilterCondition.
       // An "Edit as JSON" fallback keeps hand-authored / advanced filters

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

@@ -90,11 +90,17 @@ export function unbindAllRuleHooks(engine: MinimalEngine): number {
  * (ADR-0078). Failing the write instead tells the admin the criteria is
  * missing while they are still looking at the form.
  *
- * Update semantics are deliberately narrower than insert: only a patch that
- * SUPPLIES `criteria_json` is checked. An existing row left over from before
- * this guard has a null criteria, and an admin must still be able to
- * `active: false` it — demanding a criteria to switch off an over-broad rule
- * would be exactly backwards.
+ * LAYERING since ADR-0113: the field now declares `required: true` (write
+ * contract), so the record validator already rejects a missing/null criteria
+ * on insert and an explicit null-out on update — with the same non-regression
+ * update semantics this hook pioneered (only a patch that SUPPLIES
+ * `criteria_json` is checked; a legacy null row can still be `active: false`d).
+ * This hook remains for what `required` cannot express: the NON-null
+ * match-all shapes — `'{}'`, an all-vacuous `$and`/`$or`, unparsable JSON, a
+ * bare scalar — where `isMatchAllCriteria` is the judge and the rejection
+ * names the offending shape instead of saying "required". Defense in depth on
+ * the null cases costs one property check and keeps this guard's better
+ * message on paths where hooks fire before validation.
  */
 export function bindRuleCriteriaGuard(engine: MinimalEngine, logger?: MinimalLogger): void {
   if (typeof engine.registerHook !== 'function') return;

packages/qa/dogfood/test/sharing-rule-criteria-required.dogfood.test.ts

@@ -31,8 +31,6 @@ import { bootStack, type VerifyStack } from '@objectstack/verify';
 const RULES = '/sharing/rules';
 const SYS = { isSystem: true } as const;
 const OBJECT = 'showcase_project';
-/** Package id of the sys_sharing_rule criteria guard (plugin-sharing). */
-const GUARD_PACKAGE = 'plugin-sharing:rule-criteria-guard';
 
 describe('#3896 — a sharing rule POSTed without criteria is rejected, never made match-all', () => {
   let stack: VerifyStack;
@@ -129,27 +127,27 @@ describe('#3896 — a sharing rule POSTed without criteria is rejected, never ma
   });
 
   it('a criteria-less rule already IN the table shares nothing and loses its grants', async () => {
-    // Reproduce a row from BEFORE the guard existed: drop the guard hooks,
-    // write the row, put them back. Nothing else can produce this shape now —
-    // which is the point of the previous test.
+    // Reproduce a row from BEFORE the gate existed. Since ADR-0113 P2 the
+    // engine path is DOUBLY closed — the hook guard and the declarative
+    // `required: true` (record validator) — so unbinding the hook is no longer
+    // enough to author this shape through the engine, which is exactly the
+    // defense-in-depth the flip bought. A true legacy row predates all of it:
+    // write it at the DRIVER seam, the same level a pre-gate deployment's
+    // storage actually holds it.
     const id = 'srule_3896_legacy';
-    ql.unregisterHooksByPackage(GUARD_PACKAGE);
-    try {
-      await ql.insert('sys_sharing_rule', {
-        id,
-        name: 'legacy_match_all_3896',
-        label: 'Legacy match-all',
-        object_name: OBJECT,
-        criteria_json: null,
-        recipient_type: 'position',
-        recipient_id: 'sales_rep',
-        access_level: 'read',
-        active: true,
-      }, { context: SYS });
-    } finally {
-      const { bindRuleCriteriaGuard } = await import('@objectstack/plugin-sharing');
-      bindRuleCriteriaGuard(ql);
-    }
+    const driver = (ql as any).getDriverForObject('sys_sharing_rule');
+    expect(driver, 'sys_sharing_rule driver must be resolvable').toBeTruthy();
+    await driver.create('sys_sharing_rule', {
+      id,
+      name: 'legacy_match_all_3896',
+      label: 'Legacy match-all',
+      object_name: OBJECT,
+      criteria_json: null,
+      recipient_type: 'position',
+      recipient_id: 'sales_rep',
+      access_level: 'read',
+      active: true,
+    });
 
     const res = await stack.apiAs(admin, 'POST', `${RULES}/${id}/evaluate`, {});
     expect(res.status).toBeLessThan(300);

packages/spec/scripts/liveness/empty-state-registry.mts

@@ -103,8 +103,8 @@ export const EMPTY_STATE_REGISTRY: EmptyStateEntry[] = [
     property: 'condition',
     semantics: 'closed',
     rationale:
-      "The #3896 outcome, recorded so the answer is findable: a sharing rule's criteria is REQUIRED, and a rule that reaches storage without one grants nothing. There is no 'share every record' rule — object-wide read is the object's organization-wide default (`sharingModel`); the match-all shape only ever existed as a failure mode. Carries no permissive statement to scan (that is what being closed means), so it is exempt from the staleness check and exists purely as the catalogue answer to 'what does an empty criteria do?'.",
-    evidence: 'packages/spec/src/security/sharing.zod.ts',
+      "The #3896 outcome, recorded so the answer is findable: a sharing rule's criteria is REQUIRED, and a rule that reaches storage without one grants nothing. There is no 'share every record' rule — object-wide read is the object's organization-wide default (`sharingModel`); the match-all shape only ever existed as a failure mode. Since ADR-0113 the requirement is DECLARATIVE too: `sys_sharing_rule.criteria_json` carries `required: true` (write contract — legacy null rows rest, no NOT NULL migration), with the hook guard narrowed to the non-null match-all shapes `required` cannot express. Carries no permissive statement to scan (that is what being closed means), so it is exempt from the staleness check and exists purely as the catalogue answer to 'what does an empty criteria do?'.",
+    evidence: 'packages/spec/src/security/sharing.zod.ts; packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts (criteria_json required: true, ADR-0113); packages/plugins/plugin-sharing/src/rule-hooks.ts (shape guard)',
   },
 
   {