From a14c7a4ee8c960686241f277c1306bae65443a46 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 15:11:53 +0000 Subject: [PATCH 1/4] fix(objectql): correct mergeObjectDefinitions docblock to the real, closed merge set (#12680) The docblock claimed "other props: later value wins"; the implementation only ever merged fields/validations/indexes additively plus the three guarded scalars label/pluralLabel/description, silently discarding every other top-level prop on an extend contributor. Corrects the docblock to say so explicitly, and adds a pin (registry-object-extension-nonenumerated-prop-discard.test.ts) that hands mergeObjectDefinitions a non-enumerated top-level prop via the public SchemaRegistry API and asserts it does not survive the fold, with a guarded scalar as a positive control. No runtime behaviour changed; mergeObjectDefinitions is not exported. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- ...-object-definitions-docblock-closed-set.md | 51 ++++++++++ ...tension-nonenumerated-prop-discard.test.ts | 98 +++++++++++++++++++ packages/objectql/src/registry.ts | 26 ++++- 3 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 .changeset/registry-merge-object-definitions-docblock-closed-set.md create mode 100644 packages/objectql/src/registry-object-extension-nonenumerated-prop-discard.test.ts diff --git a/.changeset/registry-merge-object-definitions-docblock-closed-set.md b/.changeset/registry-merge-object-definitions-docblock-closed-set.md new file mode 100644 index 0000000000..d8509261d3 --- /dev/null +++ b/.changeset/registry-merge-object-definitions-docblock-closed-set.md @@ -0,0 +1,51 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): correct `mergeObjectDefinitions`'s docblock to the real, closed merge set (#12680) + +The docblock on `mergeObjectDefinitions` (`packages/objectql/src/registry.ts`) +said: + +> Fields are merged additively. **Other props: later value wins.** + +The implementation has never done the second half. It merges exactly: +`fields` (additively), `validations` (additively), `indexes` (additively), +and the three guarded scalars `label` / `pluralLabel` / `description` +(last-writer-wins, subject to the `tenantAuthored` yield rule). **Every +other top-level prop an `extend` contributor carries is silently +discarded** — `merged` starts as `{ ...base }` and nothing outside that list +is ever copied onto it. + +This is a **docs-only correction plus a regression pin — zero runtime +behaviour changed.** `mergeObjectDefinitions` is not exported; nothing about +what it does was touched, only what the comment above it claims. Shipped as +`patch` rather than omitted because the file's behaviour is now backed by an +enforced pin (see below) where before it was backed by nothing but an +inaccurate comment — that is a real (if internal-only) improvement to the +package worth a version bump, and this repo's convention reserves "no +changeset" for changes with no user-facing effect of any kind, not for +"no runtime diff." There is no public API surface to widen or narrow (the +function is module-private), so there is nothing here for `check:*` gates +that watch exported shapes to see. + +Why this matters: an author reading the old docblock and shipping an +`objectExtensions` entry carrying, say, `tenancy: { enabled: false }` would +have gotten a **silent no-op** on a security-relevant key — no error, no +warning, the base's existing value simply wins as if the extension had never +named the key. That near-miss is the reason this card exists (found while +resolving cloud#1653's investigation into exactly that override path). The +corrected docblock says the discard out loud; a new pin +(`registry-object-extension-nonenumerated-prop-discard.test.ts`) hands +`mergeObjectDefinitions` (via the public `SchemaRegistry` API) an `extend` +contributor carrying a non-enumerated top-level prop and asserts the merged +result does not carry it, with a guarded scalar as a positive control in the +same fold. `icon` is the pin's fixture — a real, spec-legal, security-neutral +top-level prop — deliberately **not** `tenancy`, so the pin does not read as +license to special-case that key elsewhere. + +⛔ Out of scope, explicitly: implementing "later value wins" for the +undocumented remainder (making `tenancy` / `permissions` extender-writable is +a separate, much larger decision triage fenced off this card) and adding a +runtime warning on the silent drop (a real question, filed separately rather +than folded in here). diff --git a/packages/objectql/src/registry-object-extension-nonenumerated-prop-discard.test.ts b/packages/objectql/src/registry-object-extension-nonenumerated-prop-discard.test.ts new file mode 100644 index 0000000000..0a95138385 --- /dev/null +++ b/packages/objectql/src/registry-object-extension-nonenumerated-prop-discard.test.ts @@ -0,0 +1,98 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#12680] `mergeObjectDefinitions`'s docblock used to promise "other props: + * later value wins" while the implementation only ever merged `fields` / + * `validations` / `indexes` (additively) and the three guarded scalars + * `label` / `pluralLabel` / `description` (last-writer-wins). Every other + * top-level prop an `extend` contributor carries was — and still is — + * silently discarded: `merged = { ...base }` and nothing else is copied. + * + * That mismatch is what let cloud#1653's investigation nearly conclude that a + * host could override a framework object's `tenancy` declaration through the + * extension seam — the docblock said so. The docblock was corrected (this + * PR); THIS FILE pins the closed merge set as a fact of the implementation so + * prose and behaviour cannot silently drift apart again. + * + * `icon` is the fixture: a real, spec-legal top-level `ServiceObject` prop + * with no security weight (`tenancy` — the motivating example — is + * deliberately NOT baked into a test, to keep this pin from reading as + * license to write a `tenancy`-specific test elsewhere). + * + * `label` is the positive control: it IS in the guarded-scalar set, so it + * must change under the very same extend-fold. Without it, a fold that did + * nothing at all would pass the negative assertion for the wrong reason. + */ + +import { describe, it, expect } from 'vitest'; +import { SchemaRegistry } from './registry.js'; + +const OWNER_PKG = 'app.owner'; +const EXTENDER_PKG = 'app.extender'; +const OBJECT_NAME = 'merge_fold_nonenumerated_prop_probe'; + +function silentRegistry(): SchemaRegistry { + const r = new SchemaRegistry({ multiTenant: false }); + r.logLevel = 'silent'; + return r; +} + +describe('mergeObjectDefinitions — closed merge set (#12680)', () => { + it('discards a non-enumerated top-level prop (`icon`) from an extend contributor silently', () => { + const r = silentRegistry(); + r.registerObject( + { + name: OBJECT_NAME, + label: 'Base Label', + icon: 'base-icon', + fields: { name: { name: 'name', type: 'text', label: 'Name' } }, + } as any, + OWNER_PKG, + ); + r.registerObject( + { + name: OBJECT_NAME, + // Enumerated scalar — must win (positive control). + label: 'Extender Label', + // NON-enumerated top-level prop — not in the closed merge set, must + // NOT survive the fold, and no error/warning marks the drop. + icon: 'extender-icon', + } as any, + EXTENDER_PKG, + undefined, + 'extend', + ); + + const resolved = r.getObject(OBJECT_NAME) as any; + + // Positive control: the guarded scalar DID fold — proves the extend + // contributor was actually applied, not skipped for some unrelated reason. + expect(resolved.label).toBe('Extender Label'); + + // The pin: the non-enumerated prop was silently discarded. The base's + // value survives untouched — the extension's value never lands. + expect(resolved.icon).toBe('base-icon'); + }); + + it('discards a non-enumerated prop the BASE never declared, rather than materializing it from the extension', () => { + const r = silentRegistry(); + r.registerObject( + { + name: OBJECT_NAME, + label: 'Base Label', + fields: { name: { name: 'name', type: 'text', label: 'Name' } }, + // no `icon` at all on the base + } as any, + OWNER_PKG, + ); + r.registerObject( + { name: OBJECT_NAME, icon: 'extender-icon' } as any, + EXTENDER_PKG, + undefined, + 'extend', + ); + + const resolved = r.getObject(OBJECT_NAME) as any; + expect(resolved.icon).toBeUndefined(); + }); +}); diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 2b289e739f..5f061cd845 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -126,11 +126,29 @@ type ObjectFoldScalarKey = (typeof OBJECT_FOLD_SCALAR_KEYS)[number]; /** * Deep merge two ServiceObject definitions. - * Fields are merged additively. Other props: later value wins. * - * [#8460] …except that "later value wins" is now conditional for the three - * SCALARS. `tenantAuthored` names the scalars the fold's BASE has authored away - * from the packaged owner's value; an extender yields on those. See + * The merge set is CLOSED and enumerable — this is NOT "every prop, later + * value wins": + * - `fields`, `validations`, `indexes` are merged ADDITIVELY. + * - The three {@link OBJECT_FOLD_SCALAR_KEYS} (`label`, `pluralLabel`, + * `description`) are overridden last-writer-wins, subject to the + * `tenantAuthored` yield rule below. + * + * **Every other top-level prop on `extension` is silently discarded.** + * `merged` starts as `{ ...base }` and nothing outside the list above is ever + * copied onto it — an extender that ships e.g. `tenancy: { enabled: false }` + * or `permissions: {...}` gets a valid-but-inert no-op: no error, no warning, + * the base's existing value simply wins as if the extension had never named + * the key (issue #12680). This is deliberate for security-relevant keys — + * "any extender may override any prop" would make `tenancy`/`permissions` + * extender-writable, which is a separate, much bigger decision that has NOT + * been made — but it means the docblock is the only place a reader can learn + * that the drop is silent; keep this comment in sync with the merge set below + * if it ever changes. + * + * [#8460] …the SCALAR override above is conditional. `tenantAuthored` names + * the scalars the fold's BASE has authored away from the packaged owner's + * value; an extender yields on those. See * {@link SchemaRegistry.tenantAuthoredScalars} for why the set is computed once * over the base rather than re-derived from the running `merged`. */ From 3e23512e7a44ef8f5817ede58ddc026c8b6e803c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 15:13:04 +0000 Subject: [PATCH 2/4] =?UTF-8?q?ABLATION=20PREDICTION=20(#12680)=20?= =?UTF-8?q?=E2=80=94=20written=20before=20any=20mutation.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutation to be applied: registry.ts's mergeObjectDefinitions() will get an added pass-through loop that copies every top-level `extension` key NOT in {fields, validations, indexes, label, pluralLabel, description} onto `merged` (last-writer-wins) — i.e. give it the literal behaviour the old, false docblock promised ("other props: later value wins"). Predicted RED (exact named set, in packages/objectql/src/registry-object-extension-nonenumerated-prop-discard.test.ts): 1) "mergeObjectDefinitions — closed merge set (#12680) > discards a non-enumerated top-level prop (`icon`) from an extend contributor silently" — fails on `expect(resolved.icon).toBe('base-icon')`: under the mutation `resolved.icon` becomes 'extender-icon'. 2) "mergeObjectDefinitions — closed merge set (#12680) > discards a non-enumerated prop the BASE never declared, rather than materializing it from the extension" — fails on `expect(resolved.icon).toBeUndefined()`: under the mutation `resolved.icon` becomes 'extender-icon'. Predicted count: exactly 2 failing tests, 1 failing file (this pin file). Both failures are assertion failures (not crashes) — the `label` assertion inside test (1) executes and passes BEFORE the failing `icon` assertion, so the file-level failure is caused by the pin's own icon assertions, not by an unrelated crash. Predicted GREEN (positive control, external to the new pin file, exercising the SAME fold machinery — proves the mutation cut the intended thing, not the whole suite): packages/objectql/src/registry-object-overlay-layer.test.ts > "ADR-0029 D9.2 — the overlay REPLACES the base layer, bit for bit" > "extenders still fold on top of whichever layer is the base" — asserts an extend contributor's `fields` merge (`ext_field` appears, `packaged_only` does not). The mutation only adds a pass-through for keys OUTSIDE {fields, validations, indexes, label, pluralLabel, description}, so this fields-only assertion is untouched and must stay green. Also predicted GREEN, as a broader control: every other test in registry-object-overlay-layer.test.ts and the rest of the registry*.test.ts suite (393 tests measured pre-mutation across 27 files, minus the 2 predicted above) — none of them assert on a non-enumerated top-level scalar prop surviving or not surviving an extend fold, so none should be sensitive to this specific mutation. From d43a99979825ffadfd69e8b5fd4b2362308cf0ca Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 15:51:36 +0000 Subject: [PATCH 3/4] docs(objectql): name the ADR-0029 D9.3 consequence of _provenance in the mergeObjectDefinitions docblock (#12680) PM review on PR #12742 (comment 5441664628): the ablation's collateral failure (registry-object-overlay-layer.test.ts > "an extender declaring priority 140 does not become the base layer") showed that copying _provenance through would let a third-party extend contributor flip a tenant-authored object's provenance to the extending package's -- reaching the exact outcome ADR-0029 D9.3's priority-reranking guard exists to prevent, by a different route. That finding lived only in the PR body; this adds it to the docblock itself, naming both the real prop and the test that catches it, so the next reader learns the measured consequence rather than only the deferred tenancy/permissions one. No behaviour change; no test change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- packages/objectql/src/registry.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 5f061cd845..86c461db82 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -146,6 +146,15 @@ type ObjectFoldScalarKey = (typeof OBJECT_FOLD_SCALAR_KEYS)[number]; * that the drop is silent; keep this comment in sync with the merge set below * if it ever changes. * + * This is not only a hypothetical: `_provenance` is a real top-level prop an + * `extend` contributor carries TODAY. Copying it through (as "later value + * wins" would) lets a third-party extender flip a tenant-authored object's + * `_provenance` to the extending package's — reaching, by a different route, + * the exact outcome ADR-0029 D9.3's priority-reranking guard exists to + * prevent. `registry-object-overlay-layer.test.ts` ("an extender declaring + * priority 140 does not become the base layer") fails if this merge set is + * widened to copy it through. + * * [#8460] …the SCALAR override above is conditional. `tenantAuthored` names * the scalars the fold's BASE has authored away from the packaged owner's * value; an extender yields on those. See From 4e20d452f456c4b71e2a6d6bfa2d237ec2f28c31 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 15:57:31 +0000 Subject: [PATCH 4/4] fix(objectql): correct a false authoring-time claim the previous docblock pass introduced (#12680) PM review on PR #12742 (comment 5441694465) caught that the docblock's "no error, no warning" framing for tenancy/permissions was false for the normal declarative authoring path: ObjectExtensionSchema (packages/spec/src/data/object.zod.ts) is .strict() -- its shape is exactly extend/fields/label/pluralLabel/description/validations/indexes/priority, so an objectExtensions entry naming tenancy, permissions, or any other undeclared key fails LOUDLY at authoring time with a prescription (#4001), already documented at content/docs/data-modeling/object-extensions.mdx. Independently re-verified both citations (packages/spec/src/data/object.zod.ts and packages/spec/src/shared/strict-object.ts's `.strict()` call) before writing this -- not taken on the PM's word. Rewrote the paragraph to say both true things: mergeObjectDefinitions itself does silently discard every non-enumerated prop (unchanged, still correct), and declarative authors never reach that silence because the schema rejects the key first -- the discard is reachable only by a caller that bypasses the schema (a direct, programmatic registerObject('extend') call, exactly how this package's own pin exercises the rule). Removed the now-false claim that "the docblock is the only place a reader can learn the drop is silent" -- the docs page and the schema's own guidance block already say so. The _provenance/ADR-0029 D9.3 paragraph from the previous commit is unaffected and unchanged in substance (only reflowed for line length) -- _provenance is stamped by applyProtection() on every 'extend' registration regardless of schema validation, so unlike tenancy/permissions it is not schema-blocked. No behaviour change; no test change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- packages/objectql/src/registry.ts | 33 ++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 86c461db82..e931dfd694 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -136,18 +136,29 @@ type ObjectFoldScalarKey = (typeof OBJECT_FOLD_SCALAR_KEYS)[number]; * * **Every other top-level prop on `extension` is silently discarded.** * `merged` starts as `{ ...base }` and nothing outside the list above is ever - * copied onto it — an extender that ships e.g. `tenancy: { enabled: false }` - * or `permissions: {...}` gets a valid-but-inert no-op: no error, no warning, - * the base's existing value simply wins as if the extension had never named - * the key (issue #12680). This is deliberate for security-relevant keys — - * "any extender may override any prop" would make `tenancy`/`permissions` - * extender-writable, which is a separate, much bigger decision that has NOT - * been made — but it means the docblock is the only place a reader can learn - * that the drop is silent; keep this comment in sync with the merge set below - * if it ever changes. + * copied onto it — a caller that hands this function an extension body + * carrying an undeclared key, e.g. `tenancy: { enabled: false }`, gets a + * valid-but-inert no-op HERE: no error, no warning from this function, the + * base's existing value simply wins as if the extension had never named the + * key (issue #12680). * - * This is not only a hypothetical: `_provenance` is a real top-level prop an - * `extend` contributor carries TODAY. Copying it through (as "later value + * Declarative authors never reach that silence, though: `ObjectExtensionSchema` + * (`packages/spec/src/data/object.zod.ts`) is `.strict()` — its shape is + * exactly `extend` / `fields` / `label` / `pluralLabel` / `description` / + * `validations` / `indexes` / `priority`, so an `objectExtensions` entry + * naming `tenancy`, `permissions`, or any other key outside that list fails + * LOUDLY at authoring time with a prescription (#4001; the same rule is + * documented at `content/docs/data-modeling/object-extensions.mdx`). This + * function's discard is reachable only by a caller that bypasses that + * schema — a direct, programmatic `registry.registerObject(def, pkg, + * undefined, 'extend', …)` call, which is how this package's own pin + * (`registry-object-extension-nonenumerated-prop-discard.test.ts`) exercises + * the rule. Extender-writable `tenancy`/`permissions` would in any case be a + * separate, much bigger decision that has NOT been made. + * + * Unlike `tenancy`/`permissions`, this one is not blocked by schema: + * `_provenance` is a real top-level prop an `extend` contributor carries + * TODAY. Copying it through (as "later value * wins" would) lets a third-party extender flip a tenant-authored object's * `_provenance` to the extending package's — reaching, by a different route, * the exact outcome ADR-0029 D9.3's priority-reranking guard exists to