From 6f56c1290e76e47d4ac3ee3da9478d8809cfec66 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 20:39:36 +0000 Subject: [PATCH 1/3] fix(objectql): refuse a symbol key at the flat-input set/defineProperty traps (#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 ctx.input is a JS-runtime artifact leaking toward storage. installFlatInput's `set` and `defineProperty` traps now throw a TypeError naming the key kind and the surface, before the write ever reaches `data`, instead of routing it through silently the way #12277 routes every other mutation. Measured pre-fix, on origin/main: a symbol-keyed `set` succeeded silently, the value reached `data` and persisted to the row the engine stores, and only `Reflect.ownKeys`/`getOwnPropertySymbols` omitted it from enumeration -- two instruments said "own", enumeration said "no", while the persisted row held it regardless. That is exactly the "hiding what you persist" shape #12277/#12397/ #12578 exist to abolish; this closes it from the write side instead of the enumeration side (Option B, publishing symbols via Reflect.ownKeys, was declined -- it would have made an undeclarable key kind a published contract). `ownKeys` itself is untouched, per the ruling: ownKeys can never observe a symbol key that set/defineProperty never let onto `data`. Inverts the pin `hook-input-ownkeys-agreement.test.ts` carried OPEN since #12578 into a REFUSAL pin, in place -- the same case, turned around, not a second assertion stacked beside the first. Part of #12603 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- .changeset/hook-input-symbol-key-refusal.md | 58 ++++++++++ content/docs/automation/hooks.mdx | 19 ++++ .../src/hook-input-ownkeys-agreement.test.ts | 81 +++++++++----- packages/objectql/src/hook-wrappers.ts | 104 ++++++++++++++++-- 4 files changed, 225 insertions(+), 37 deletions(-) create mode 100644 .changeset/hook-input-symbol-key-refusal.md diff --git a/.changeset/hook-input-symbol-key-refusal.md b/.changeset/hook-input-symbol-key-refusal.md new file mode 100644 index 0000000000..2b13c6e883 --- /dev/null +++ b/.changeset/hook-input-symbol-key-refusal.md @@ -0,0 +1,58 @@ +--- +'@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) From 651b9888ebc5cf75765cd6ae3744e6e408518db1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 20:40:07 +0000 Subject: [PATCH 2/3] predict: ablation for #12603 refusal pin Prediction, written BEFORE mutating, per AGENTS.md ablation discipline. Subject: packages/objectql/src/hook-wrappers.ts, the `set` and `defineProperty` traps' `typeof prop === 'symbol'` refusal guards (refuseSymbolPayloadKey calls). Mutation: remove both guard calls, restoring silent pass-through to the pre-#12603 behaviour (symbol-keyed writes route into `data` unrefused). Predicted result under the mutation: - RED (exactly 1 test): packages/objectql/src/hook-input-ownkeys-agreement.test.ts "[#12603] REFUSAL, not agreement -- a symbol key is rejected before it can ever reach data" -- both `expect(setThrew).toBeInstanceOf(TypeError)` and `expect(definePropertyThrew).toBeInstanceOf(TypeError)` fail because neither write throws under the mutation; `raw.data` ends up holding the symbol key instead of omitting it. - GREEN, unaffected (positive control, mutated in the SAME window): packages/objectql/src/hook-input-mutation-traps.test.ts "POSITIVE CONTROL -- an assignment in the same call still lands" -- an ordinary string-keyed assignment never touches the symbol branch, so removing the guard changes nothing for it. - The other 4 cases in hook-input-ownkeys-agreement.test.ts (REPRODUCTION, enumerable-face, absent-key agreement, positive control, declared exception) stay GREEN -- none of them write a symbol key. Named set: 1 red (the inverted pin), 41 total tests across the 5 hook-input suites measured earlier this run, so 40 green expected under the mutation. From 90add5560a1ceede5d0228bd51704df422a4c8fc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 20:59:32 +0000 Subject: [PATCH 3/3] docs(changeset): answer the ADR-0087 disposition question for #12603 check-adr-0087-registration flagged the changeset as declaring a breaking change (the BREAKING-CHANGE-footer regex matched an ordinary hard-wrapped sentence starting "breaking change does not burn a major version...") with no adr-0087: disposition marker. Whatever tripped the regex, the underlying verdict is correct on the merits -- this changeset does argue an accept-set narrowing -- so the fix is to answer the question, not to reword around it. Category: no-migration-prescription, verified against the real predicates (scripts/check-adr-0087-registration.mjs: parseChangeset/hasMigrationPrescription) rather than assumed: - hasMigrationPrescription(body) is false both before and after adding the marker -- the changeset's "**Migration.**" paragraph is prose guidance ("use a string key instead"), not a heading, an arrow rewrite, a FROM/TO label, or a rewrite table, so it never sets framedSection or matches REWRITE_RE. - runtime-interface-only does not apply and was ruled out mechanically, not just judgment: exportedTypeDeclaration only recognizes an exported interface/type/class/enum ("const/function are absent on purpose -- this category is about a TYPE surface"), and every symbol this diff touches (installFlatInput, refuseSymbolPayloadKey) is a private function; the one exported function in the file whose behaviour changed (wrapDeclarativeHook) had no type-declaration change, only a Proxy trap's runtime behaviour, so naming it would be a category the gate's own predicate cannot verify true. ADR-0087 addendum (2026-08-13) authorizes the category: "no-migration- prescription -- REFUSED when the changeset's own body carries a migration prescription" -- ctx.input carries no spec/Zod field, object definition, or stored representation for objectstack migrate meta to act on; a symbol key was never a declarable metadata surface in the first place, so there is nothing here for a migration to rewrite. Part of #12603 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- .changeset/hook-input-symbol-key-refusal.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/hook-input-symbol-key-refusal.md b/.changeset/hook-input-symbol-key-refusal.md index 2b13c6e883..2b19d26b82 100644 --- a/.changeset/hook-input-symbol-key-refusal.md +++ b/.changeset/hook-input-symbol-key-refusal.md @@ -56,3 +56,5 @@ 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. + +