Skip to content

fix(objectql): refuse a symbol key at the flat-input set/defineProperty traps (#12603) - #12801

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-12603-hook-input-symbol-key-refusal
Aug 27, 2026
Merged

fix(objectql): refuse a symbol key at the flat-input set/defineProperty traps (#12603)#12801
os-zhuang merged 3 commits into
mainfrom
claude/issue-12603-hook-input-symbol-key-refusal

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#12603

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 silently routing it into the record payload (data) the way every string-keyed write is routed.

constcacheKey=Symbol('cache-entry');ctx.input[cacheKey]=value;// TypeError: Cannot set Symbol(cache-entry) on hook input: a symbol key is// not a valid record-payload field. A record payload is a declarable,// string-keyed field set ...

ownKeys itself is untouched — still Object.getOwnPropertyNames(data), exactly as #12578 landed it (per the dispatch brief: do not re-open #12578's own boundary).

Why (the ruling)

Maintainer ruling, 2026-08-27, on #12603Option 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, 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 exactly the shape #12277/#12397/#12578 exist to abolish; this closes the gap from the write side rather than the enumeration side.

Premise, measured myself before changing anything

Drove wrapDeclarativeHook directly against the real proxy on origin/main (ead731756, worktree matched by content hash), with a positive control (an ordinary string-key write, confirmed it persists) proving the harness itself was live:

ReadingResult
symbol-keyed set succeeds silently✅ confirmed — no throw, raw.data[sym] set
defineProperty with a symbol key succeeds✅ confirmed — no throw, raw.data[sym] set
key absent from Object.keys/spread/ownKeys✅ confirmed — Reflect.ownKeys/Object.keys/spread all omit it
value persists to data and is handed onward✅ confirmed — Object.getOwnPropertySymbols(raw.data) includes it, raw.data is the plain object the wrapper restores onto ctx.input after the hook runs (what the engine's downstream input.data read consumes)

All four readings match the ruling's stated rationale exactly — no contradiction, so this proceeded rather than stopping to report.

The pin: inverted, not added

hook-input-ownkeys-agreement.test.ts carried the symbol case OPEN since #12578 (Symbol.for('objectstack.test.12578'), asserting the disagreement — two instruments say "own", enumeration says "no", and the payload the engine persists holds it). Found with a positive control first (searched for "symbol" across the sibling suites; the ownKeys-agreement file's own doc comment and pin surfaced immediately, confirming the search channel was live before concluding anything).

That case is turned around in place — same it(), same file, no second assertion stacked beside it — into a REFUSAL pin: both set and defineProperty now throw a TypeError naming the key kind (/symbol/i) and the surface (/hook input/i), and nothing reaches data.

Ablation

Prediction committed before mutating (git commit --allow-empty, 651b9888e): removing both typeof prop === 'symbol' guards should turn exactly 1 of 41 tests across the 5 hook-input suites red (the inverted pin), the other 40 stay green, and the sibling positive control (hook-input-mutation-traps.test.ts — "POSITIVE CONTROL — an assignment in the same call still lands") stays green in the same mutated window.

Measured: exact match. Test Files 1 failed | 4 passed (5), Tests 1 failed | 40 passed (41); the one failure named is precisely the inverted pin (expected undefined to be an instance of TypeError). Mutation confirmed on disk with anchored grep -c on the guard text (1→0, twice) and the marker text (0→2) before any measurement; the set/defineProperty guard removal made refuseSymbolPayloadKey transiently unused, which failed only the DTS half of the rebuild (TS6133) — the ESM/CJS build (what the source-resolving vitest run actually exercises for a same-package relative import) built clean, so the measurement is unaffected. Restore proven: git hash-object on the restored file equals the HEAD blob exactly, git diff HEAD is empty, zero marker residue, and a clean rebuild + re-run afterward is green again (5 passed (5), 41 passed (41)).

Changeset — minor, argued

Every sibling in this family (#12277, #12397, #12578, #12601) shipped patch because each closed an instrument disagreement — the accepted set of writes never changed. This card is different in kind: a write that used to succeed now throws, on a surface every hook body touches. That is the same shape 8cc8401 (@objectstack/objectql 17.2.0, "BREAKING (accept-set tightening)") argued minor for, under this repo's launch-window convention (scripts/check-changeset-no-major.mjs: breaking changes ship minor, not major, while the stack versions in lockstep). Full reasoning in .changeset/hook-input-symbol-key-refusal.md.

Docs

Added a Callout block to content/docs/automation/hooks.mdx (Before Hook section, beside the existing reserved-names callout) documenting the refusal and the accidental-spread scenario the dispatch brief called out.

Gates

Derived via node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack from the actual changed paths (4 files: hook-wrappers.ts, the inverted test, the changeset, the docs edit), re-derived after the final commit — unchanged. 43 path/convention-matched local families run, all green except two unrelated-to-this-diff NOT MEASURED reads, both resolved:

  • check:skill-examples — first run hit a pre-existing prerequisite gap (@objectstack/client-react had no dist in this fresh worktree, unrelated to this diff). Built its dependency closure + itself, re-ran: green (260 prose examples type-check across 3 surfaces).
  • pm/check-half-states — exit 3, PREREQUISITE NOT MET (no valid GitHub credential in this container) — NOT MEASURED, not a red, per the gate's own documented exit-code contract.

Named-in-brief families, all green: check:engine-double-contract, check:where-matcher, check:objectql-double-limit, check:query-options-erasure.

check:type-check-debt (the ratchet covering **/*.test.ts, which the package's own typecheck script excludes) required the full workspace closure built (pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*', 70/70 successful) — green, none above its recorded number, confirming the edited test file (part of objectql's tracked TEST_DEBT re-measure) typechecks clean.

packages/objectql's own pnpm typecheck and pnpm --filter @objectstack/objectql exec vitest run (the 5 hook-input trap-set suites, 41/41) both green.

Serial slot

Both prerequisites (#12578 via #12602, #12601 as 86df0c99e) confirmed landed by content on origin/main before starting. Searched for another open PR touching hook-wrappers.ts with a positive control first (a generic open-PR query returned 12 results, proving the channel was live) — zero hits for the file, so this is a clean single-writer claim.


Generated by Claude Code

…ty 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
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.
@github-actionsgithub-actionsBot added size/m documentation Improvements or additions to documentation tests tooling labels Aug 27, 2026
@github-actions

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

3 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 15 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 96dc446c9c19063edfae26ae30ff75143ef0c5b7packageMentionDocs.

Which tree this was computed on

This run read content/docs from 2b43e9098b8e2770b10b8a6f53f7d93f116cfce0 — the merge of head 90add5560a1ceede5d0228bd51704df422a4c8fc into base 96dc446c9c19063edfae26ae30ff75143ef0c5b7, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 2b43e9098b8e2770b10b8a6f53f7d93f116cfce0 && git checkout 2b43e9098b8e2770b10b8a6f53f7d93f116cfce0
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 96dc446c9c19063edfae26ae30ff75143ef0c5b7 90add5560a1ceede5d0228bd51704df422a4c8fc && git checkout -B drift-repro 96dc446c9c19063edfae26ae30ff75143ef0c5b7 && git merge --no-ff 90add5560a1ceede5d0228bd51704df422a4c8fc
node scripts/docs-audit/affected-docs.mjs --json 96dc446c9c19063edfae26ae30ff75143ef0c5b7

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

Reviewer-of-record hand-read of the docs, answering the drift check's "nothing to list — not a clean bill of health" above. That bot cannot see a page that states a rule by its inputs while the diff changed the emitter; this diff is exactly that shape, so the null result was a prompt to read, not a clearance.

Positive control first: 13 pages under content/docs reach ctx.input, so the search channel was live before any conclusion was drawn from it.

Result: no falsified sentence. One near-miss, reported rather than rewritten.

content/docs/automation/hook-bodies.mdx:207 states "an undeclared key is neither rejected nor stripped". That paragraph is scoped to an unknown field — string-keyed by construction — and describes the write-path validator, which sits downstream of this proxy; the new refusal fires upstream of it. Ambiguous rather than false, so it stays as-is here. Worth knowing for whoever next tightens that paragraph.

One hypothesis raised and killed by measurement, which is why the read was worth doing.

hook-bodies.mdx:186 records that an action'sctx.input is "its params bag, not a record". Had actions shared this proxy, the new error text — "a symbol key is not a valid record-payload field. A record payload is a declarable, string-keyed field set" — would be stating a rationale that does not hold on that surface: a shipped, green, wrong-reason error message.

Traced instead of assumed. installFlatInput has one caller, wrapDeclarativeHook (hook-wrappers.ts:446), which has exactly one non-test caller of its own: hook-binder.ts:221, the hook binder. Action bodies never reach it. The message is correct on every surface it can fire from.

Independently re-verified by content on origin/main, not taken from the report:


Generated by Claude Code

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
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

Fixed the Check Changeset / check-adr-0087-registration failure (job 98666592763). New commit 90add5560, pushed.

What tripped it: not the quoted "BREAKING (accept-set tightening)" precedent as I first guessed from the excerpt -- direct execution against the real predicates (parseChangeset / breakingDeclaration) showed the actual match is the gate's BREAKING[ -]CHANGE footer regex, which matched an ordinary hard-wrapped sentence in my own prose: the line literally starts breaking change does not burn a major version.... 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 in writing, not reword around it.

Category chosen: no-migration-prescription. ADR-0087 addendum (2026-08-13) sentence: "no-migration-prescription is refused by a body that carries a migration prescription." Verified directly (not assumed) with the gate's own exported predicate:

hasMigrationPrescription(parsed.body) === false

both before and after adding the marker -- the changeset's **Migration.** paragraph is prose guidance ("use a string key instead"), not a heading (so it never opens framedSection), and has no arrow, no FROM/TO label, and no rewrite table, so findMigrationPrescription returns null.

Trap 2 (self-contradiction) did not bite -- confirmed by running the predicate against the body WITH the new marker line included (it scans the whole body), still null/false.

runtime-interface-only ruled out mechanically, not just by judgment: read exportedTypeDeclaration (the verifier) directly -- it only recognizes an exported interface/type/class/enum ("const/function are absent on purpose -- this category is about a TYPE surface"). Every symbol this diff touches (installFlatInput, refuseSymbolPayloadKey) is a module-private function; the one exported function whose behaviour changed (wrapDeclarativeHook) had no type-declaration change, only a Proxy trap's runtime behaviour. So no symbol in this diff can satisfy step 1 of that category's own verification -- it isn't a close call, it's inapplicable by construction.

Local gate output:

Before (HEAD 651b9888e, reproducing the CI failure exactly):

✗ check-adr-0087-registration: 1 problem(s).
• .changeset/hook-input-symbol-key-refusal.md
declares a breaking change (BREAKING) but no `adr-0087:` disposition marker.

After (HEAD 90add5560):

✓ check-adr-0087-registration: 1 declared-breaking changeset(s), each carrying an ADR-0087 disposition.
.changeset/hook-input-symbol-key-refusal.md [BREAKING] not-required (no-migration-prescription)
reason: A JS-runtime Proxy trap now throws instead of silently accepting a write; no spec or
Zod field, object definition, or stored representation is added, removed, or renamed. A
symbol key was never a declarable metadata surface for objectstack migrate meta to know
about, so there is nothing here for a migration to rewrite -- the Migration section above is
authoring guidance for hook bodies, not a metadata-ledger prescription, and carries no arrow
or FROM/TO pair.

node scripts/check-adr-0087-registration.mjs --self-test: 212 assertions, green both before and after (the checker itself is untouched).

Also re-ran node scripts/check-changeset-no-major.mjs and node scripts/check-empty-changeset.mjs against the new HEAD (both green, no regression), and re-ran the 5 objectql hook-input trap-set suites (41/41 green, no regression). Did not edit docs/adr/**. The fix is exactly the two-line diff (one blank line, one disposition comment) -- no reword of the **Migration.** section, since trap 2 did not force one.

Pushed sha: 90add5560a1ceede5d0228bd51704df422a4c8fc.


Generated by Claude Code


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 27, 2026 21:27
@os-zhuang
os-zhuang enabled auto-merge August 27, 2026 21:27
@os-zhuang
os-zhuang added this pull request to the merge queueAug 27, 2026
Merged via the queue into main with commit c34f693Aug 27, 2026
35 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-12603-hook-input-symbol-key-refusal branch August 27, 2026 21:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

May a hook-input record payload carry a SYMBOL key — and if so must enumeration publish it? (the half #12578 deliberately left open)

2 participants

@os-zhuang@claude