fix(objectql)!: registerHook refuses an empty object target and a self-cancelling scope (#6573)

.changeset/registerhook-empty-target-refusal.md

@@ -0,0 +1,69 @@
+---
+"@objectstack/objectql": major
+"@objectstack/spec": patch
+---
+
+<!-- adr-0087: registered hook-register-empty-object-target-refused -->
+
+fix(objectql)!: `engine.registerHook` refuses an empty `object` target and a scope whose two faces cancel out (#6573)
+
+`engine.registerHook` is a **public API**, and two option shapes that used to
+register successfully now **throw at registration**. Both were already
+refused on the metadata path by #4281 (`HookSchema.object`'s refine and
+`hook-binder.ts`'s `normalizeObjects`); the code path went through neither, so
+the ruling stopped at the door it never reached.
+
+**1. An empty `object` target — `''`, `[]`, `['']`, or any blank list member.**
+
+```ts
+engine.registerHook('afterUpdate', h, { object: '' })    // was: a GLOBAL hook
+engine.registerHook('afterUpdate', h, { object: [] })    // was: never fires
+engine.registerHook('afterUpdate', h, { object: [''] })  // was: never fires
+```
+
+`''` is falsy, so the allow face was skipped entirely and the entry registered
+as a **global** hook — #4281's headline failure mode, blank intent becoming the
+broadest possible blast radius, reproduced verbatim. `[]` and `['']` are truthy
+but admit no object name, so the entry could never fire (ADR-0078, no silently
+inert declaration). All three now throw, reusing #4281's wording.
+
+- FROM `{ object: '' }` (or `[]` / `['']`) → TO: name the object(s),
+  `{ object: 'account' }` / `{ object: ['account', 'contact'] }`; or, if firing
+  on every object is the intent, spell the wildcard — `{ object: '*' }` — or
+  omit `object` entirely. `object: undefined` is unchanged and still means
+  "global".
+
+**2. An allow face fully cancelled by the exclusion face.**
+
+```ts
+engine.registerHook('afterUpdate', h, { object: 'account', excludeObjects: 'account' })
+engine.registerHook('afterUpdate', h, { object: ['a', 'b'], excludeObjects: ['a', 'b'] })
+```
+
+`excludeObjects` (#5928) subtracts from `object`, so when `object` is a finite
+enumeration and every name in it is also excluded, the admitted set is empty and
+the entry can never fire — the same ADR-0078 inert declaration #5928's three
+refusals exist to prevent, reached by arithmetic instead of by one bad name and
+therefore outside that ruling's letter. Only a **finite** allow face is decided:
+`object: '*'` and an absent `object` admit an open universe (objects can be
+registered into a running engine), so they are left alone, and the one exclusion
+that would empty them — `'*'` — is already refused by #5928.
+
+- FROM a fully-cancelled pair → TO: widen `object` (or drop it for a global
+  hook), or remove the overlapping names from `excludeObjects`. **Partial**
+  cancellation is untouched — `{ object: ['a', 'b'], excludeObjects: ['b'] }` is
+  exactly what the exclusion face is for.
+
+**What did NOT change:** `hookMatchesObject`'s reading of `object: ''` as
+"global" is deliberately left as-is. Teaching the matcher that `''` is an
+unmatchable name would silently convert a hook firing on every object into one
+firing on none — the same class of defect pointing the other way. The shapes are
+closed at the registration door, so no live entry can carry them.
+
+**Callers that forward a non-literal `object`** now surface these refusals
+instead of registering a broken entry: `RecordChangeTrigger.start` forwards a
+flow start node's `config.objectName` verbatim, and `ObjectQL.create({ hooks })`
+forwards each `hook.object`. A flow authored with a blank `objectName` used to
+bind a record-change trigger to **every** object; it now fails to bind, loudly —
+the throw is caught by the automation engine's per-flow bind guard, which warns
+and leaves the flow unbound for the binding audit to re-report.

docs/protocol-upgrade-guide.md

@@ -436,6 +436,13 @@ The plugin manifest loses its whole `loading` block in this step (#4914, ADR-004
 - **`view-filter-rule-value-shaped-by-operator`** — `ui.ViewFilterRule value — the third key of a view filter rule, on every carrier of ViewFilterRuleSchema: ListView.filter, a list view tab filter, Page.filterBy, a related-list component filter and a lookup picker filter. It accepted any declared scalar or array for EVERY operator; the accepted shape is now decided by the rule operator — in / not_in require an array, between requires exactly two bounds, and every other operator is unchanged` → an ARRAY for in / not_in (a single value becomes a one-element list: value: "won" becomes value: ["won"]), and a two-element [min, max] array for between. The empty list [] stays legal for in / not_in and keeps its meaning. Nothing else moves: a scalar operator carrying an array, a string operator carrying a number, and a unary operator carrying an ignored value all still parse
   - Why not automatic: A publish-time gate catching up to a query-time one, not a new rule. #5869 / PR #6209 closed the RUNTIME half: `assertListComparandShapes` (@objectstack/objectql, filter-comparand-shape.ts) refuses a lowered `{ stage: { $nin: "won" } }` with a named 400 INVALID_FILTER, and before that it was a 500. The authoring surface stayed silent, so the failure was two-stage: the view published cleanly and only broke when someone opened it. That file names this very schema as the reachable authoring source of the defect. The tightening MIRRORS that gate exactly — three constraints, one for one — and deliberately goes no further, because #5685 already ruled on the opposite error: a schema stricter than the runtime "in ways the runtime deliberately allows" was the WRONG side and was widened to match. So `in: []` is still accepted (a declared predicate both drivers implement), `equals: ["a","b"]` is still accepted (it lowers to a deep-equality comparand), and `is_empty: ""` is still accepted (the null predicates take their direction from the operator NAME — convertComparison ignores the value position, and the ObjectUI client deliberately sends a truthy placeholder there). ⚠️ Metadata AT REST is deliberately NOT rewritten, and there is no D2 conversion. A D2 entry replays a shape the platform once WROTE and renamed; this shape was never written by any first-party producer (every in / not_in rule in this repo, in objectui and in the cloud repo already carries an array — measured) and has never EXECUTED, since it 400s on first render today. Coercing it at load would be the platform guessing intent rather than replaying a rename, and it cannot guess honestly: value: "" would become the predicate [""] (a real filter on the empty string) rather than the "not filled in yet" a console row means, and between: 5 has no defensible second bound at all. The read path does not re-validate stored rows (applyConversionsToStoredItem never validates, by its own contract), so no stored view becomes unreadable; what changes is that RE-SAVING such a view is refused at the write gate naming `value`, instead of storing a filter that 400s. ADR-0049 / ADR-0078 / ADR-0112.
   - Done when: Grep your authored views, pages and related-list components for a filter rule whose operator is in, not_in or between (including the alias spellings nin / notIn / notin) and whose value is not an array of the right arity, then wrap or complete it. `os validate` / `os lint` now report each one by path with the operator, the received shape and the corrected shape, so the sweep is mechanical rather than by eye. Two checks are worth doing where it looks unnecessary: a rule reading `operator: "in", value: ""` is an UNFINISHED row, not a filter — decide what it was meant to select rather than mechanically rewriting it to [""], which is a real and different predicate. And a view that already carried one of these shapes was never returning filtered rows: it answered 400 INVALID_FILTER on render (#5869), so re-check what the view is supposed to show rather than assuming the old result set was correct.
+- **`hook-register-empty-object-target-refused`** — `engine.registerHook(event, handler, { object: '' | [] | [''] }), and a scope whose `excludeObjects` cancels its `object` entirely` → name the object(s) — `object: 'account'` / `object: ['account', 'contact']` — or, for a global hook, `object: '*'` or no `object` key at all; for a cancelled scope, widen `object` or drop the overlapping names from `excludeObjects`
+  - Why not automatic: #4281 ruled that an empty hook target is not "no target" and closed the shape at the two METADATA doors — `HookSchema.object`'s refine and `hook-binder.ts`'s `normalizeObjects`. `engine.registerHook`, the CODE door, goes through neither, so all three spellings still registered, each producing a defect the author did not write: `''` is FALSY, so the allow face was skipped entirely and the entry became a GLOBAL hook (#4281's headline failure mode — blank intent taking the broadest possible blast radius); `[]` and `['']` are truthy but admit no object name, so the entry could never fire. #5928 then added the `excludeObjects` face, which brought a fourth shape reached by arithmetic rather than by one bad name: an `object` list every member of which is also excluded admits nothing, so that entry can never fire either. All four are ADR-0078 silently-inert declarations, and all four are now refused at REGISTRATION.
+
+No mechanical rewrite exists, in either direction. The refused values carry no recoverable intent — `object: ''` could have meant `'*'` (what it actually did) or a specific object name the author forgot to fill in, and those are opposite registrations; choosing between them is a judgment the chain cannot make. Nor could the MATCHING read be changed instead: teaching the matcher that `''` is an unmatchable name would silently convert a hook firing on every object into one firing on none — the same class of defect pointing the other way, which is why #5928 declined to do it in passing.
+
+This is a RUNTIME registration API, not stored metadata, so — like `hook-context-session-roles-retired` at this step — there is no `sys_metadata` row for the D2 chain to rewrite and the ledger entry is the notification channel. One metadata surface reaches it INDIRECTLY and is the reason this is not purely a code-side note: a `record-change` flow's start node forwards `config.objectName` verbatim into `registerHook` (`RecordChangeTrigger.start`), so a flow authored with a blank `objectName` used to bind a trigger to EVERY object in the tenant. It now fails to bind instead, loudly — the automation engine's per-flow bind guard warns and the `kernel:bootstrapped` binding audit re-reports it — which is the correct end state, but it is an observable change for that flow. #6573, #4281, #4001, #5928, ADR-0078.
+  - Done when: No `registerHook` call site passes an empty `object` target, and none passes an `excludeObjects` list covering every name in its `object` list. Every `record-change` flow start node declares a non-blank `config.objectName`, or omits the key if the flow is genuinely meant to fire on every object. Boot completes with no "[ObjectQL] Hook ... declares an empty `object` target" throw and no "[record-change] ... not bound" warning naming a flow you expect to fire.
 
 ---
 

packages/objectql/src/engine.ts

@@ -846,13 +846,14 @@ function hookTargetList(target: string | string[] | undefined): string[] {
  *
  * The allow half keeps the TRUTHINESS test both copies used verbatim, rather
  * than the `!== undefined` that reads more precisely. They differ on exactly one
- * input, `object: ''`, which today registers a GLOBAL hook (falsy ⇒ no filter) —
- * #4281's failure mode surviving on the code path it never covered, since it
- * closed the metadata path at the schema and the binder. Flipping it here would
- * turn a hook that fires on everything into one that fires on nothing, silently,
- * inside a PR about a different face of the contract. Out of scope by
- * construction: preserved, pinned in `hook-exclude-objects.test.ts`, and filed
- * separately.
+ * input, `object: ''`, which reads here as a GLOBAL hook (falsy ⇒ no filter).
+ * That read is deliberately UNCHANGED, and #6573 is why: flipping it would turn
+ * a hook firing on everything into one firing on nothing, silently — the same
+ * class of defect pointing the other way. The shape is closed at the
+ * registration door instead ({@link assertValidHookObject}), so no live entry
+ * can carry `''` and this branch is unreachable for it in practice. It stays
+ * because this function is also called directly on hand-built entries, and
+ * because a matcher should describe one rule, not re-litigate the door's.
  */
 export function hookMatchesObject(
   entry: Pick<HookEntry, 'object' | 'excludeObjects'>,
@@ -929,6 +930,105 @@ function assertValidHookExcludeObjects(
   }
 }
 
+/**
+ * [#6573] Registration-time refusal for the `object` (ALLOW) face, closing
+ * #4281 / #4001's "an empty target is not *no* target" ruling on the path that
+ * ruling never reached.
+ *
+ * #4281 shut this shape at the two METADATA doors — `HookSchema.object`'s
+ * refine in `packages/spec`, and `normalizeObjects` in `hook-binder.ts`. The
+ * code door, `engine.registerHook`, goes through neither, so the same three
+ * spellings still walked in here, and the matching read (see
+ * {@link hookMatchesObject}) turns each of them into a defect:
+ *
+ *  - **`''`** is FALSY, so the allow half is skipped entirely and the entry
+ *    registers as a GLOBAL hook. #4281's headline failure mode exactly — blank
+ *    intent becoming the broadest possible blast radius — reproduced verbatim
+ *    on the uncovered path.
+ *  - **`[]` / `['']`** (or any blank member) are truthy but admit no object
+ *    name, so the entry can never fire: registered "successfully", inert
+ *    forever. ADR-0078's silently-inert declaration.
+ *
+ * Refused at REGISTRATION rather than fixed in the matcher, and that choice is
+ * the whole point of the separate card. Teaching `hookMatchesObject` to read
+ * `''` as a real (unmatchable) name would silently convert a hook firing on
+ * every object into one firing on none — the same class of defect pointing the
+ * other way, which is why #5928 declined to do it in passing. A throw at the
+ * door changes no dispatch and leaves nothing to misread.
+ *
+ * Note the asymmetry with {@link assertValidHookExcludeObjects}, which
+ * deliberately ACCEPTS `[]`: on the subtract face an empty list is the honest
+ * spelling of "subtract nothing", identical to omitting the key. On the allow
+ * face an empty list says "admit nothing", which is a hook that can never fire.
+ * Same value, opposite meaning, because the faces compose in opposite
+ * directions — and `[]` is named in #4281's own message, so accepting it here
+ * would contradict the ruling this reuses.
+ */
+function assertValidHookObject(
+  object: string | string[] | undefined,
+  event: string,
+): void {
+  if (object === undefined) return;
+  const names = hookTargetList(object);
+  const empty =
+    names.length === 0
+    || names.some((name) => typeof name !== 'string' || name.trim().length === 0);
+  if (!empty) return;
+  throw new Error(
+    `[ObjectQL] Hook '${event}' declares an empty \`object\` target. An empty target is `
+    + 'not "no target": `\'\'` is falsy, so the allow face is skipped entirely and the hook '
+    + 'registers on EVERY object, while `[]` and `[\'\']` admit no object name at all, so the '
+    + 'hook could never fire (ADR-0078: no silently inert declaration). Name the object(s) — '
+    + "`object: 'account'` or `object: ['account', 'contact']` — or, if firing on every "
+    + "object really is the intent, write the wildcard explicitly: `object: '*'`.",
+  );
+}
+
+/**
+ * [#6573] Refuse a scope whose two faces cancel each other out —
+ * `{ object: 'account', excludeObjects: 'account' }` and its list forms.
+ *
+ * The exclusion face subtracts from the allow face, so when the allow face is a
+ * FINITE enumeration and every name in it is also excluded, the admitted set is
+ * empty and the entry can never fire. #5928 named only three refusals (`''`,
+ * `['']`, and `'*'` in the excludes) and this shape falls outside their letter,
+ * so it was left registering silently — the same ADR-0078 inert declaration
+ * those three exist to prevent, reached by arithmetic instead of by a single
+ * bad name.
+ *
+ * Only a finite allow face can be decided here. `'*'` (and an absent `object`)
+ * admits an OPEN universe — `applyObjectRegistryMutation` registers objects
+ * into a running engine — so no finite exclusion list can empty it, and those
+ * scopes are left alone. `'*'` inside `excludeObjects` is the one exclusion
+ * that WOULD empty them, and it is already refused by
+ * {@link assertValidHookExcludeObjects}.
+ *
+ * Runs after both faces have been validated individually, so every name here is
+ * a non-blank string and the exclusion list carries no wildcard.
+ */
+function assertHookScopeNotSelfCancelling(
+  object: string | string[] | undefined,
+  excludeObjects: string | string[] | undefined,
+  event: string,
+): void {
+  if (object === undefined || excludeObjects === undefined) return;
+  const allow = hookTargetList(object);
+  // An open allow face cannot be emptied by a finite subtraction.
+  if (allow.length === 0 || allow.includes('*')) return;
+  const deny = hookTargetList(excludeObjects);
+  if (deny.length === 0) return;
+  const denied = new Set(deny);
+  if (!allow.every((name) => denied.has(name))) return;
+  throw new Error(
+    `[ObjectQL] Hook '${event}' excludes every object its \`object\` target admits `
+    + `(object: ${JSON.stringify(allow)}, excludeObjects: ${JSON.stringify(deny)}), leaving `
+    + 'a hook that can never fire (ADR-0078: no silently inert declaration). An exclusion '
+    + 'subtracts from a WIDER allow face — widen `object` (or drop it for a global hook) or '
+    + 'remove the overlapping names from `excludeObjects`; if the hook really should not be '
+    + 'registered, do not register it.',
+  );
+}
+
 /** Function registry entry — see `registerFunction`. */
 export interface FunctionEntry {
   handler: HookHandler;
@@ -1448,9 +1548,15 @@ export class ObjectQL implements IObjectQLEngine {
     /** Stable name from metadata (set by `bindHooksToEngine`). */
     hookName?: string;
   }) {
-    // [#5928] Refuse an exclusion face that subtracts nothing (`''`) or
-    // everything (`'*'`) before anything is registered or reported.
+    // Refuse a scope that is statically decidable as meaningless before
+    // anything is registered or reported. Each face is checked on its own
+    // first, so the combined check below can assume well-formed names.
+    // [#5928] An exclusion face that subtracts nothing (`''`) or everything (`'*'`).
     assertValidHookExcludeObjects(options?.excludeObjects, event);
+    // [#6573] An allow face that names nothing (`''` → global, `[]`/`['']` → never fires).
+    assertValidHookObject(options?.object, event);
+    // [#6573] Two well-formed faces that cancel out (`'account'` minus `'account'`).
+    assertHookScopeNotSelfCancelling(options?.object, options?.excludeObjects, event);
     // [#3195] Guard against enum-vs-dispatch drift: a hook on an event the
     // engine never triggers would register "successfully" and then silently
     // never fire. Warn loudly rather than swallow it. Not a hard reject — a