From 11220f4fef2d2dcbca78d8a800a1445cc7c24e8a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 05:19:03 +0000 Subject: [PATCH 1/3] =?UTF-8?q?feat(sharing):=20criteria=5Fjson=20is=20dec?= =?UTF-8?q?laratively=20required=20=E2=80=94=20ADR-0113=20P2,=20the=20firs?= =?UTF-8?q?t=20consumer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The field the ADR was written for: flips required: true (write contract — legacy null rows rest, an admin can still active:false an over-broad legacy rule, no NOT NULL migration; deliberately NO storage.notNull, those nulls are the case the split exists for). The Setup form gets its required marker from this declaration (P1 verifies the Console side). The #3929 guards consolidate by layer instead of vanishing: the record validator now owns the missing/null/explicit-clear cases declaratively; the beforeInsert/beforeUpdate hook narrows to the NON-null match-all shapes required cannot express ('{}', vacuous $and/$or, unparsable, scalars), where isMatchAllCriteria judges and the message names the shape (hooks fire before validation, so the better message still wins on the direct path); defineRule keeps the API seam; the evaluator fail-closed stays (ADR-0049). Empty-state registry entry updated with the declarative pointer. plugin-sharing 198/198; the bound sharing-rule-criteria-required proof 5/5 against a booted stack; check:empty-state + check:liveness green. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QuViRSR1j6GJjf9qGbnqFX --- .../src/objects/sys-sharing-rule.object.ts | 19 ++++++++++++------- .../plugins/plugin-sharing/src/rule-hooks.ts | 16 +++++++++++----- .../scripts/liveness/empty-state-registry.mts | 4 ++-- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts b/packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts index 21cccab5d5..c712446e27 100644 --- a/packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts +++ b/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 diff --git a/packages/plugins/plugin-sharing/src/rule-hooks.ts b/packages/plugins/plugin-sharing/src/rule-hooks.ts index 1b0a8f83e3..1de75b4879 100644 --- a/packages/plugins/plugin-sharing/src/rule-hooks.ts +++ b/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; diff --git a/packages/spec/scripts/liveness/empty-state-registry.mts b/packages/spec/scripts/liveness/empty-state-registry.mts index 80bda83a6c..d91a52d948 100644 --- a/packages/spec/scripts/liveness/empty-state-registry.mts +++ b/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)', }, { From 770e31d09934e88b0c9f700779b1fc330857bdd9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 05:49:28 +0000 Subject: [PATCH 2/3] chore(sharing): changeset for the criteria_json required flip Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QuViRSR1j6GJjf9qGbnqFX --- .../criteria-json-declaratively-required.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 .changeset/criteria-json-declaratively-required.md diff --git a/.changeset/criteria-json-declaratively-required.md b/.changeset/criteria-json-declaratively-required.md new file mode 100644 index 0000000000..637dcc0b4a --- /dev/null +++ b/.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). From 8e84ac5334c4476bb0172d668671d79ae021332d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 05:59:20 +0000 Subject: [PATCH 3/3] test(dogfood): the legacy criteria-less row is seeded at the driver seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'already IN the table' case needs a row from before the gate existed. Pre-P2 it could fake one by unbinding the hook and inserting through the engine; the declarative required: true closed that path too — the record validator now rejects it with the hook unbound, which is exactly the defense-in-depth the flip bought (and what CI correctly caught; the local 5/5 was a stale plugin-sharing dist, the same trap for the fourth time). A true legacy row predates every gate, so it is now written at the DRIVER seam — the level a pre-gate deployment's storage actually holds it — and the unused guard-package unbind ceremony is gone with it. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QuViRSR1j6GJjf9qGbnqFX --- ...ing-rule-criteria-required.dogfood.test.ts | 42 +++++++++---------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/packages/qa/dogfood/test/sharing-rule-criteria-required.dogfood.test.ts b/packages/qa/dogfood/test/sharing-rule-criteria-required.dogfood.test.ts index 65b97ba575..eb868cad68 100644 --- a/packages/qa/dogfood/test/sharing-rule-criteria-required.dogfood.test.ts +++ b/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);