Skip to content

fix(objectql): flat-input envelope wins consistently across get/getOwnPropertyDescriptor (#12601) - #12752

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-12601-flat-input-wrapper-key-precedence
Aug 27, 2026
Merged

fix(objectql): flat-input envelope wins consistently across get/getOwnPropertyDescriptor (#12601)#12752
os-zhuang merged 3 commits into
mainfrom
claude/issue-12601-flat-input-wrapper-key-precedence

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#12601

What

installFlatInput's (packages/objectql/src/hook-wrappers.ts) get trap has
always given id / options / ast / data precedence to the WRAPPER
(envelope) — a direct read like ctx.input.id never falls through to the
record payload. getOwnPropertyDescriptor checked the payload FIRST instead,
so for a record whose payload happened to declare a field sharing one of
those four names, a direct read and a descriptor read disagreed about the
identical key:

constraw={data: {id: 'PAYLOAD-ID',subject: 'help'},options: {},id: 'WRAPPER-ID'};input.id// 'WRAPPER-ID' (get)Object.getOwnPropertyDescriptor(input,'id').value// 'PAYLOAD-ID' (descriptor, pre-fix)

Per the maintainer ruling (comment 5434984840, Option A — "the envelope wins consistently"):
id/options/ast/data are reserved names on the hook flat-input face.
getOwnPropertyDescriptor now checks them first too, matching get's order,
so every instrument that reads one of the four names for a value agrees. The
payload's own field, if it declares one of these names, stays a legal record
field — untouched in storage — reachable at input.data.NAME, just not
through the flat face.

enumerable on the reserved-name descriptor still depends on whether data
also owns the name — unchanged, this is what keeps Object.keys/spread/
Object.entries carrying the envelope's value under the reserved name exactly
as they did before this fix (they already agreed with get, since a value
read goes through get), rather than silently dropping a key ownKeys still
offers.

ownKeys is unchanged — per the dispatch fence, Object.getOwnPropertyNames(target.data)
stays exactly as #12578 landed it. Whether symbol keys should be exposed
remains #12603's open question, untouched here.

Premise re-measurement (on current origin/main, before this PR's fix)

Driven through wrapDeclarativeHook against the real proxy, per the dispatch's
repro script:

get: 'WRAPPER-ID'
getOwnPropertyDescriptor(...).value: 'PAYLOAD-ID' (disagreed)
Reflect.ownKeys(...).includes('id'): true
{ ...input }.id: 'WRAPPER-ID'

Reproduces exactly as the issue and dispatch predicted. ownKeys and spread
already agreed with get before this fix (spread reads a value through
get); only a raw descriptor read disagreed. Confirmed the fix closes exactly
that gap — see tests below.

Regression coverage — per instrument, per name

packages/objectql/src/hook-input-envelope-precedence.test.ts — a case per
reserved name (id, options, ast, and data itself, i.e. a payload field
literally named data), each asserting get / getOwnPropertyDescriptor /
spread / Object.entries all agree on the envelope's value, and
input.data.NAME still returns the payload's; an edge case for an
insert-shaped envelope with no wrapper id (the reserved name then resolves
to absent, not to the payload's value, matching get's existing
unconditional wrapper-only read); a positive control on a non-reserved key;
and the pre-existing non-collision declared-exception case repeated as a
guardrail on this fix specifically.

packages/runtime/src/sandbox/hook-input-envelope-precedence.integration.test.ts
pins the load-bearing consequence named in the dispatch: a sandboxed hook body
reading a same-named field through unwrapProxyToPlain's Object.entries
walk sees the envelope's value — now by contract (this fix), not by the
accident its own header documents (measured: this file is already green
pre-fix, because ownKeys + a coincidentally-enumerable payload descriptor +
get's pre-existing precedence happened to line up anyway).

Ablation

Prediction written and committed (test(objectql,runtime): pin #12601 envelope-precedence regression, pre-fix) before the fix commit. Formal
ablation cycle (commit-based, trap-guarded, absolute paths): hook-wrappers.ts
overwritten with the pre-fix blob from that commit, anchored greps confirm the
mutation landed both directions, the new regression file re-run —

Test Files 1 failed (1)
Tests 5 failed | 2 passed (7)

exact match to the prediction, by count and by named set (the 4
per-name collision cases + the insert-shaped edge case fail; the positive
control and the non-collision declared-exception case stay green). Positive
control run in the same mutated window: all 4 sibling trap-set suites
(hook-input-ownkeys-agreement, hook-input-descriptor-mirror,
hook-input-mutation-traps, hook-input-shape-contract) — 34/34 green,
proving the cut was surgical. Restored via git checkout HEAD -- PATH;
proven via git diff HEAD empty and git hash-object equal to the HEAD blob,
both times.

Docs

content/docs/automation/hooks.mdx — a Callout at the flat ctx.input
section states the four reserved names loudly, with the ctx.input.data.NAME
escape hatch.

Changeset

patch on @objectstack/objectql, argued in the file: same trap set, same
shape, same scope as the two immediately preceding fixes on this proxy
(#12397, #12578), both shipped patch — no persisted data moves, and
every path that already worked (get, ownKeys, spread, Object.entries)
is unaffected; only a direct getOwnPropertyDescriptor(...).value read on a
name colliding with a reserved name changes.

Clause ② — contract accept/reject, surface width

Rejects an inconsistency, does not widen the surface. No field, key,
capability, or export is added or removed. The wrapper-first precedence rule
already existed (in get, and in the pre-existing ownKeys "WRAPPER KEYS
remain excluded" design intent) — this PR extends the same existing rule to
getOwnPropertyDescriptor so it stops disagreeing with its siblings. If
anything the change is a narrowing: a payload field colliding with one of the
four reserved names was PARTIALLY reachable through the flat face before this
fix (via a direct descriptor read only, inconsistently with every other
instrument) and is now reachable through none of the flat face's instruments
— only through input.data.NAME, which was always the documented route.

Gates

Full detail in the structured report on the issue. Summary: dispatch-named
(check:engine-double-contract, check:where-matcher,
check:objectql-double-limit, check:query-options-erasure) green; full
@objectstack/objectql (243 files / 4239 tests) and @objectstack/runtime
(197 files / 2889 tests) suites green; both packages' typecheck clean;
~37 additional path-derived gates (docs/changeset/cross-package/static-analysis
families) green; check:type-check-coverage green;
check:type-check-debt --re-measureNOT MEASURED (its own prerequisite
refusal: needs the full ~80-package workspace closure built, which lint.yml
does and this local run did not attempt); check:pm-half-statesNOT
MEASURED
(prerequisite refusal: no valid GitHub credential in this
container for that script's own /rate_limit probe — unrelated to this
diff).


Generated by Claude Code

…e-fix
Written and run against unmodified origin/main before any fix lands, to serve
as the ablation prediction: packages/objectql/src/hook-input-envelope-precedence.test.ts
reproduces the get/getOwnPropertyDescriptor disagreement per reserved name
(id/options/ast/data) and fails 5 of 7 cases (named set recorded in the dev's
scratchpad prediction); the runtime-side consequence pin
(packages/runtime/src/sandbox/hook-input-envelope-precedence.integration.test.ts)
is already green pre-fix, "by accident" per its own header comment.
Part of #12601
…iptor/ownKeys (#12601)
installFlatInput's getOwnPropertyDescriptor checked the record payload
(`data`) before the reserved wrapper names (`id`/`options`/`ast`/`data`),
while `get` always checked the wrapper first — so a payload field sharing one
of those four names made a direct read (`input.id`) and a descriptor read
(`Object.getOwnPropertyDescriptor(input, 'id').value`) disagree about the
same key.
Per the maintainer ruling (Option A, envelope wins consistently): the four
names are reserved on the flat face. getOwnPropertyDescriptor now checks them
first, matching get's order; enumerable still depends on whether `data` also
owns the name (unchanged ownKeys, #12578), which is what keeps spread/
Object.entries carrying the envelope's value under the reserved name exactly
as before. The payload's own value stays reachable at input.data.<name>.
Part of #12601
…angeset (#12601)
content/docs/automation/hooks.mdx now states loudly, at the Before Hook flat
ctx.input section, that id/options/ast/data are reserved and always resolve
to the envelope — a payload field sharing one of those names is reachable
only at ctx.input.data.<name>.
Changeset (patch, argued in the file): same trap set, same shape, same scope
as the two immediately preceding fixes here (#12397, #12578), both patch — no
persisted data moves, and the paths that already worked (get, ownKeys,
spread, Object.entries) are unaffected; only a direct descriptor-value read
on a name that collides with a reserved name changes.
Part of #12601
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 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 168941cea851ca77a42ce5db01b77f1c91273beepackageMentionDocs.

Which tree this was computed on

This run read content/docs from 4eec8dd897e5edb61a79459f0c5006ba4f0d5027 — the merge of head 6e1297bc42d05659eaf68fc65de3d38f17141b30 into base 168941cea851ca77a42ce5db01b77f1c91273bee, 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 4eec8dd897e5edb61a79459f0c5006ba4f0d5027 && git checkout 4eec8dd897e5edb61a79459f0c5006ba4f0d5027
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 168941cea851ca77a42ce5db01b77f1c91273bee 6e1297bc42d05659eaf68fc65de3d38f17141b30 && git checkout -B drift-repro 168941cea851ca77a42ce5db01b77f1c91273bee && git merge --no-ff 6e1297bc42d05659eaf68fc65de3d38f17141b30
node scripts/docs-audit/affected-docs.mjs --json 168941cea851ca77a42ce5db01b77f1c91273bee

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

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 27, 2026
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — ACCEPT on substance; enqueueing on all-green

Reviewer of record: domain:engine PM seat (#6367). Verified at head 6e1297bc.

The fence held, and I checked it the way it could fail

The dispatch's hard fence was: do not turn ownKeys' Object.getOwnPropertyNames into Reflect.ownKeys, because that answers #12603 — an open maintainer question about symbol keys — as a side effect of this card.

hook-wrappers.ts:682 ownKeys(target) {
return target.data && typeof target.data === 'object'
? Object.getOwnPropertyNames(target.data)
: [];
}
`Reflect.ownKeys` added by this diff: 0

Byte-identical to what #12578 landed. #12603 stays genuinely open rather than quietly resolved.

The enumerable coupling is the sharp part, and it is not what the ruling literally asked for

The ruling said "align the descriptor trap with get". The naive way to do that is to move the reserved-name branch first and keep what it already returned:

returndesc ? { ...desc,enumerable: false} : undefined;// the old line

That would have been wrong in a new way, and it is worth spelling out why, because it is the trap this card is about, one turn further on. ownKeys answers Object.getOwnPropertyNames(target.data) — so when the payload owns id, ownKeyslistsid. A descriptor saying enumerable: false for a key ownKeys offers means Object.keys and spread silently drop it: enumeration and own-key listing disagree, which is exactly the #12578 defect class rebuilt on the other side.

What landed instead:

constpayloadOwnsName=!!data&&typeofdata==='object'&&Object.prototype.hasOwnProperty.call(data,prop);return{ ...desc,configurable: true,enumerable: payloadOwnsName};

enumerable is now true exactly when ownKeys lists the name. So spread carries the key — with the envelope's value, because a spread reads values through get — and all four instruments agree. When the payload does not own the name, it stays non-enumerable, preserving the original intent of keeping wrapper keys out of Object.keys.

Getting from "make the descriptor match get" to "and keep enumerable tied to ownKeys or you have made a new disagreement" is the part a careful implementer adds and an incurious one does not.

The insert-shaped edge case is the right call

A reserved name with no wrapper value resolves to absent, not to the payload's value. That matches get's existing unconditional wrapper-only read; the alternative — falling back to the payload when the envelope has no id — would have made the precedence rule conditional on envelope shape, which is how this whole family of disagreements starts.

Honest about what the sandbox pin does and does not prove

"measured: this file is already green pre-fix, because ownKeys + a coincidentally-enumerable payload descriptor + get's pre-existing precedence happened to line up anyway"

A pin that is green before the fix is usually worthless, and saying so rather than quietly counting it as evidence is right. Its value here is different and real: it converts an accidental alignment into a contractual one, so the next change to any of those three parts fails loudly. Naming that distinction is what makes it a pin rather than decoration.

Ablation: 5 failed / 2 passed, exact match to a prediction committed before the fix, by count and named set, with all four sibling trap-set suites (34/34) green in the same mutated window — that co-run is what shows the cut was surgical rather than broad.

Fences

packages/spec 0 · content/docs/releases/** 0 · docs/adr/** 0 · .claude/** 0. content/docs/automation/hooks.mdx is hand-written and in scope — the ruling explicitly asked for the reserved names to be documented in the hook envelope docs.

Clause ② reasoned rather than answered by reflex, and the conclusion is right: this narrows. A payload field colliding with a reserved name was partially reachable through the flat face (via a raw descriptor read alone, inconsistently with every other instrument) and is now reachable through none of it — only via input.data.NAME, which was always the documented route.

Enqueueing once every check is green, not the required subset. check:type-check-debt --re-measure and check:pm-half-states are correctly recorded as NOT MEASURED prerequisite refusals rather than folded into a blanket pass.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 27, 2026 17:02
@os-zhuang
os-zhuang enabled auto-merge August 27, 2026 17:02
@os-zhuang
os-zhuang added this pull request to the merge queueAug 27, 2026
Merged via the queue into main with commit 86df0c9Aug 27, 2026
35 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-12601-flat-input-wrapper-key-precedence branch August 27, 2026 17:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-zhuang@claude