diff --git a/.changeset/hook-input-symbol-key-refusal.md b/.changeset/hook-input-symbol-key-refusal.md new file mode 100644 index 0000000000..2b19d26b82 --- /dev/null +++ b/.changeset/hook-input-symbol-key-refusal.md @@ -0,0 +1,60 @@ +--- +'@objectstack/objectql': minor +--- + +fix(objectql): the flat-input proxy REFUSES a symbol key at `set`/`defineProperty` instead of silently persisting it (#12603) + +**Bump level, argued**: `minor`, not `patch`. Every sibling in this family (#12277, +#12397, #12578, #12601) shipped `patch` because each closed an instrument +DISAGREEMENT — the accepted set of writes never changed, only which read-back +told the truth about them. This card is different in kind: `ctx.input[sym] = +value` and `Object.defineProperty(ctx.input, sym, …)` **used to succeed**, and +now **throw**. That is a narrowing of the accept set on `ctx.input` — a surface +every hook body touches — which is the exact shape `8cc8401` +(`@objectstack/objectql` 17.2.0, "BREAKING (accept-set tightening)") argued +`minor` for under this repo's launch-window convention (pre-1.0 semantics: a +breaking change does not burn a major version while the stack versions in +lockstep — see `scripts/check-changeset-no-major.mjs`). `patch` here would +under-declare a change that can turn a passing hook into a throwing one. + +**What changed.** `installFlatInput`'s `set` and `defineProperty` traps +(`packages/objectql/src/hook-wrappers.ts`) now refuse a symbol-keyed write with +a `TypeError` naming the key kind and the surface, instead of routing it into +the record payload (`data`) the way every string-keyed write is routed. +Measured on the pre-fix tree: a symbol-keyed `set` succeeded silently, the +value reached `data` and persisted to the row the engine stores, and only +`Object.getOwnPropertySymbols` / `Reflect.ownKeys` omitted it from enumeration +— two instruments said "own", enumeration said "no", while the persisted row +held it regardless. + +**Why a refusal, not a fourth instrument fix.** Maintainer ruling, 2026-08-27 +(Option C, refusal arm), on the payload-contract question #12578 measured and +deliberately left open rather than decided: a record payload is a declarable, +**string-keyed** field set — no metadata schema can declare a symbol field, so +a symbol key on this surface is a JS-runtime artifact leaking toward storage, +not a legal payload field. Option B (publish symbols too, via +`Reflect.ownKeys`) was declined — it would have made an undeclarable key kind a +published contract instead of closing the question. Hiding a key the engine +nonetheless persists is precisely the shape #12277/#12397/#12578 exist to +abolish; refusing the write at the boundary closes that gap from the other +side, before persistence rather than after enumeration. + +`ownKeys` itself is **untouched** — still `Object.getOwnPropertyNames(data)`, +exactly as #12578 landed it. With the write refused, `data` can never carry a +symbol key for that trap (or `Reflect.ownKeys`) to disagree about, so there is +nothing left for this card to change there. + +**Migration.** Code that wrote a symbol key onto `ctx.input` — almost always by +accident, e.g. spreading an object that carried a symbol-keyed cache entry onto +the payload — now throws instead of silently losing the write to enumeration. +Use a string key, or keep the value off the payload entirely (a local +variable, or a WeakMap keyed by the record) if it was never meant to be +stored. No other hook-input read/write path changes: reads, `has`, `delete`, +and every string-keyed write behave exactly as before. + +Inverts the pin `hook-input-ownkeys-agreement.test.ts` carried OPEN since +#12578 (the disagreement, deliberately left standing) into a REFUSAL pin +(the write throws, nothing persists) — the same case, turned around in place, +not a second assertion stacked beside the first. + + diff --git a/content/docs/automation/hooks.mdx b/content/docs/automation/hooks.mdx index 6f9e3d3dc7..20f588800f 100644 --- a/content/docs/automation/hooks.mdx +++ b/content/docs/automation/hooks.mdx @@ -161,6 +161,25 @@ envelope's value for a reserved name (`packages/objectql/src/hook-wrappers.ts`, #12601). + +A record payload is a declarable, string-keyed field set — no metadata schema +can declare a symbol field. Writing one onto `ctx.input`, whether by +assignment or `Object.defineProperty`, throws a `TypeError` naming the key and +the surface instead of silently accepting it: + +```ts +const cacheKey = Symbol('cache-entry'); +handler: async (ctx) => { + ctx.input[cacheKey] = value; // throws: symbol keys are not a valid record-payload field +}; +``` + +This most often happens by accident — spreading an object that carries a +symbol-keyed cache entry onto the payload. Use a string key, or keep the value +off the payload entirely if it is not meant to be stored (`packages/objectql/src/hook-wrappers.ts`, +#12603). + + ```typescript import { Hook } from '@objectstack/spec/data'; diff --git a/packages/objectql/src/hook-input-ownkeys-agreement.test.ts b/packages/objectql/src/hook-input-ownkeys-agreement.test.ts index c91b2d1745..24a0dd42e7 100644 --- a/packages/objectql/src/hook-input-ownkeys-agreement.test.ts +++ b/packages/objectql/src/hook-input-ownkeys-agreement.test.ts @@ -49,13 +49,17 @@ * That disagreement is the trap's whole purpose (the payload-diff idiom must * see record fields only) and is pinned as DECLARED so it cannot be mistaken * for a residue of the defect above. - * - SYMBOL KEYS carry the identical disagreement and are deliberately left - * carrying it. Publishing them is a one-word change here - * (`Object.getOwnPropertyNames` -> `Reflect.ownKeys`), but whether a record - * payload may hold a symbol key at all is a question about the PAYLOAD - * contract — the boundary #12397 drew and this card does not cross. It is - * reported open on #12578 and pinned below in its open state, so answering - * it changes a recorded fact instead of an unnoticed one. + * - SYMBOL KEYS used to carry the identical disagreement, pinned open below: + * the write succeeded silently, persisted into `data`, and only the + * enumeration face omitted it. [#12603] The maintainer ruling (2026-08-27, + * Option C refusal arm) answered the payload-contract question this file + * left open: a record payload is a declarable, string-keyed field set, and + * no metadata schema can declare a symbol field. `set` and `defineProperty` + * now REFUSE a symbol-keyed write, loudly, before it ever reaches `data` — + * Option B (publish via `Reflect.ownKeys`) was declined, because it would + * have made the undeclarable key kind a published contract instead of + * closing it. The case below is INVERTED accordingly, in place: it used to + * pin the disagreement open, and now pins the refusal. * * `wrapDeclarativeHook` is driven directly rather than through `ObjectQL`, for * the reason the sibling trap-set files give: the subject is the wrapper's @@ -195,31 +199,52 @@ describe('[#12578] the flat-input `ownKeys` reports the payload own-key set, and expect(seen.readMulti).toBe(false); }); - it('OPEN QUESTION, pinned in its open state — a symbol key carries the same disagreement', async () => { - // Reported on #12578 rather than decided here: publishing symbol keys - // through `ownKeys` is `Reflect.ownKeys` in one line, but whether the - // record payload may CARRY a symbol key is a payload-contract question and - // a maintainer floor (#12397's boundary). + it('[#12603] REFUSAL, not agreement — a symbol key is rejected before it can ever reach data', async () => { + // INVERTS the OPEN QUESTION pin this case used to carry (verbatim, before + // this card): `input[sym] = value` succeeded silently, persisted into + // `data`, and only `Reflect.ownKeys`/`getOwnPropertySymbols` omitted it — + // two instruments said own, enumeration said no, and the payload the + // engine persisted held it regardless. // - // What the measurement establishes, and what this case records: symbol keys - // already reach `data` through the `set` trap and already persist. So the - // open question is about what the enumeration face should PUBLISH, not - // about what a hook can already put on the row. + // Maintainer ruling, 2026-08-27, Option C refusal arm: a record payload is + // a declarable, string-keyed field set — no metadata schema can declare a + // symbol field, so the write is refused at the boundary instead of hidden + // after it lands. There is no longer a persisted symbol key for the three + // instruments to disagree about, so this is a refusal pin, not an + // agreement pin — asserted for both traps the ruling names. const raw: any = { data: { subject: 'help' }, options: {} }; - const sym = Symbol.for('objectstack.test.12578'); - const seen: Record = {}; + const sym = Symbol.for('objectstack.test.12603'); + + let setThrew: unknown; + await runHook(raw, (input) => { + try { + input[sym] = 'symvalue'; + } catch (e) { + setThrew = e; + } + }); + expect(setThrew).toBeInstanceOf(TypeError); + const setMessage = (setThrew as TypeError).message; + expect(setMessage).toMatch(/symbol/i); // names the key kind + expect(setMessage).toMatch(/hook input/i); // names the surface + // Refused BEFORE `data` is touched — nothing persisted, sibling field intact. + expect(Object.getOwnPropertySymbols(raw.data)).toEqual([]); + expect(raw.data.subject).toBe('help'); + + let definePropertyThrew: unknown; await runHook(raw, (input) => { - input[sym] = 'symvalue'; - seen.ownness = ownness(input, sym); - seen.symbols = Object.getOwnPropertySymbols(input); + try { + Object.defineProperty(input, sym, { value: 'dp-value', enumerable: true, configurable: true }); + } catch (e) { + definePropertyThrew = e; + } }); + expect(definePropertyThrew).toBeInstanceOf(TypeError); + expect((definePropertyThrew as TypeError).message).toMatch(/symbol/i); + expect(Object.getOwnPropertySymbols(raw.data)).toEqual([]); - // Today: two instruments say own, enumeration says no — the defect's shape, - // deliberately left standing on this half. - expect(seen.ownness).toEqual({ enumeration: false, hasOwnProperty: true, descriptor: true }); - expect(seen.symbols).toEqual([]); - // …while the payload the engine persists holds it. - expect(Object.getOwnPropertySymbols(raw.data)).toEqual([sym]); - expect((raw.data as any)[sym]).toBe('symvalue'); + // The three instruments now agree there is no such key at all — the + // refusal closes the disagreement this file otherwise exists to police. + expect(ownness(raw.data, sym)).toEqual(NOT_OWN); }); }); diff --git a/packages/objectql/src/hook-wrappers.ts b/packages/objectql/src/hook-wrappers.ts index 26a035dd7d..a6239076f2 100644 --- a/packages/objectql/src/hook-wrappers.ts +++ b/packages/objectql/src/hook-wrappers.ts @@ -511,7 +511,60 @@ export function wrapDeclarativeHook( * `get` and `getOwnPropertyDescriptor` traps below for the instrument-by- * instrument account, and `content/docs/automation/hooks.mdx` (Hook Context) * for the author-facing statement of the same rule. + * + * [#12603] ⛔ SYMBOL keys are REFUSED, not routed. `set` and `defineProperty` + * throw a `TypeError` for a symbol-keyed write instead of letting it reach + * `data` — see those two traps below for the refusal and + * `refuseSymbolPayloadKey` for the error text. See "Symbol keys" below the + * reserved-name callout in `content/docs/automation/hooks.mdx`. + */ + +/** + * [#12603] Maintainer ruling, 2026-08-27, Option C refusal arm: a record + * payload is a declarable, string-keyed field set — no metadata schema can + * declare a symbol field, so a symbol key on this flat `ctx.input` face is a + * JS-runtime artifact leaking toward storage, not a legal payload field. + * + * Thrown from the `set` and `defineProperty` traps, BEFORE either touches + * `data` — so a refused write never persists and never needs hiding from + * enumeration. That is the shape change from the pre-#12603 state: a + * symbol-keyed write used to succeed silently, reach `data`, and persist, + * while only `Object.getOwnPropertySymbols` / `Reflect.ownKeys` omitted it + * (pinned open in `hook-input-ownkeys-agreement.test.ts` under #12578). Hiding + * a key the engine nonetheless persisted is exactly the shape #12277, #12397 + * and #12578 exist to abolish; refusing the write outright is the other way + * to close that gap, and the one this ruling chose (Option B — publish + * symbols via `Reflect.ownKeys` — was declined for minting an undeclarable + * key kind as contract). + * + * A plain `TypeError`, not a new subclass: this fires from INSIDE the hook + * BODY (an author wrote `input[sym] = …`), the same category as the native + * `TypeError` the `defineProperty` trap already lets through for a + * non-configurable descriptor the target does not carry (see that trap's own + * comment) — an author-code defect, not a declarative-layer diagnostic like + * `HookConditionError`. It is therefore subject to the ordinary handler error + * path: `onError: 'log'` can swallow it, `retryPolicy` can retry it, exactly + * as any other throw from the handler body (unlike `HookConditionError`, + * which is deliberately raised OUTSIDE that path — see the comment on that + * class for why the two are not the same shape). + * + * The message names the key kind (a symbol, not "a bad key"), the surface + * (hook input), and the fix (a string key, or keep the value off the payload) + * — written for the accidental case the ruling calls out: an author spreading + * an object that happens to carry a symbol-keyed cache entry onto `ctx.input`. */ +function refuseSymbolPayloadKey(prop: symbol, trap: 'set' | 'defineProperty'): never { + const verb = trap === 'set' ? 'Cannot set' : 'Cannot define'; + throw new TypeError( + `${verb} ${String(prop)} on hook input: a symbol key is not a valid record-payload field. ` + + 'A record payload is a declarable, string-keyed field set — no metadata schema can declare ' + + 'a symbol field, so a symbol key here would be a JS-runtime artifact leaking toward storage. ' + + 'Use a string key, or keep the value off the payload entirely (e.g. a local variable) if it ' + + 'is not meant to be stored. This often happens by accident, such as spreading an object that ' + + 'carries a symbol-keyed cache entry onto ctx.input.' + ); +} + function installFlatInput(ctx: HookContext): () => void { const raw: any = ctx.input ?? {}; const looksWrapped = @@ -545,7 +598,14 @@ function installFlatInput(ctx: HookContext): () => void { } return Reflect.get(target, prop, receiver); }, + // [#12603] Symbol keys are REFUSED here, before anything else runs — a + // symbol can never equal one of the four reserved (string) names, so the + // check can sit first without disturbing that branch below. See + // `refuseSymbolPayloadKey` for why this throws instead of routing. set(target, prop, value) { + if (typeof prop === 'symbol') { + refuseSymbolPayloadKey(prop, 'set'); + } if (prop === 'id' || prop === 'options' || prop === 'ast' || prop === 'data') { (target as any)[prop] = value; return true; @@ -600,12 +660,23 @@ function installFlatInput(ctx: HookContext): () => void { // A throw is a diagnosis; the silence was not. Omitting `configurable` // entirely (the common spelling, and every spelling `Object.assign` and // spread produce) is unaffected. + // + // [#12603] Symbol keys are refused here too, identically to `set` and for + // the same reason — see `refuseSymbolPayloadKey`. defineProperty(target, prop, desc) { + if (typeof prop === 'symbol') { + refuseSymbolPayloadKey(prop, 'defineProperty'); + } if (prop === 'id' || prop === 'options' || prop === 'ast' || prop === 'data') { return Reflect.defineProperty(target, prop, desc); } return Reflect.defineProperty(ensureData(), prop, desc); }, + // [#12603] `deleteProperty` is deliberately NOT guarded: since `set` and + // `defineProperty` now refuse every symbol-keyed write before it reaches + // `data`, a symbol key can never be there to delete. `delete input[sym]` + // falls through exactly as it always has for any key `data` does not + // own — a harmless no-op reporting success, not a persistence lie. has(target, prop) { if (prop === 'id' || prop === 'options' || prop === 'ast' || prop === 'data') { return prop in target; @@ -670,15 +741,30 @@ function installFlatInput(ctx: HookContext): () => void { // SAME value everywhere it is read. See the descriptor trap's own comment // for the full account. // - // SYMBOL KEYS are deliberately still absent, and this is NOT a finding - // that they do not belong on a payload: `Reflect.ownKeys(data)` here would - // additionally publish them, and whether the record payload may carry a - // symbol key at all is a question about the PAYLOAD contract (they already - // reach `data` through the `set` trap and already persist — measured), not - // about this trap. It is open, reported on #12578, and the day it is - // answered "yes" this line becomes `Reflect.ownKeys`. Until then the - // symbol half of the disagreement is pinned AS open in the sibling test, - // so an answer changes a recorded fact rather than an unnoticed one. + // [#12603] SYMBOL KEYS are absent here for a settled reason now, not an + // open one: the maintainer ruling (2026-08-27, Option C refusal arm) + // answered the payload-contract question this trap's comment used to + // leave open ("may a record payload carry a symbol key at all?") with + // NO — a record payload is a declarable, string-keyed field set, and no + // metadata schema can declare a symbol field. `set` and `defineProperty` + // now REFUSE a symbol-keyed write before it ever reaches `data` (see + // `refuseSymbolPayloadKey`), so a symbol can no longer BE an own key of + // `data` for this trap to omit or report. + // + // This trap itself is deliberately UNCHANGED by that ruling — + // `Object.getOwnPropertyNames(target.data)` stays exactly what #12578 + // landed. `Reflect.ownKeys(data)` (Option B) was the alternative the + // ruling declined: publishing symbols through enumeration would mint an + // undeclarable key kind as contract, which is the opposite of what was + // ruled. With the write refused at the boundary, the two spellings would + // agree anyway — `data` can never carry a symbol key for them to differ + // on — so there is no remaining reason to touch this line, and #12578's + // own ruling (this card must not re-litigate `ownKeys`) forbids it. + // + // What used to be pinned OPEN in `hook-input-ownkeys-agreement.test.ts` + // (the instrument disagreement, deliberately left standing) is now + // pinned as a REFUSAL in the same file: the write throws, so there is no + // persisted symbol key left for the three instruments to disagree about. ownKeys(target) { return target.data && typeof target.data === 'object' ? Object.getOwnPropertyNames(target.data)