Skip to content

fix(objectql): mirror data's own descriptor in the flat-input Proxy instead of synthesising one - #12581

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-12397-flat-input-descriptor-mirror
Aug 26, 2026
Merged

fix(objectql): mirror data's own descriptor in the flat-input Proxy instead of synthesising one#12581
os-warren merged 1 commit into
mainfrom
claude/issue-12397-flat-input-descriptor-mirror

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#12397

installFlatInput's getOwnPropertyDescriptor trap answered every key data carries with one fixed literal and never read data's real descriptor. It now mirrors the own descriptor, forcing configurable: true.

Reproduction, before repairing

The card's case, measured on origin/main at 5fbd58e0dbeforehook-wrappers.ts was touched — the new pin file run against the unfixed trap, 5 of its 6 cases red:

 FAIL REPRODUCTION — a key defined non-enumerable reports non-enumerable
AssertionError: expected { value: 1, writable: true, …(2) } to deeply equal { value: 1, writable: false, …(2) }
{
"configurable": true,
- "enumerable": false, ← what `data` actually holds
+ "enumerable": true, ← what the trap reported
"value": 1,
- "writable": false,
+ "writable": true,
}
FAIL an INHERITED key has no own descriptor to mirror, and no longer gets a fabricated one
AssertionError: expected { value: [Function toString], …(3) } to be undefined
+ Received: { "configurable": true, "enumerable": true, "value": [Function toString], "writable": true }
FAIL an accessor on the payload: the trap neither throws nor RUNS the getter
AssertionError: expected 1 to be +0 ← a descriptor read invoked author code

Object.keys(input) was right the whole time, which is what makes the shape bad: two instruments over one payload, disagreeing.

The fix, and the one attribute that cannot be mirrored

configurable is forcedtrue. The proxy target is the { data, options, id? } wrapper, which does not carry the record key, and a proxy may not report a property its target lacks as non-configurable — a verbatim mirror throws TypeError on any key data holds as configurable: false, and takes Object.keys and spread down with it, since both reach every listed key through this trap. That leg is pinned explicitly (INVARIANT — a data key held non-configurable …), because a mirror that only works on ordinary keys is the same defect one layer along.

Two further consequences, both pinned:

  • A descriptor read no longer runs author code. The synthesis evaluated data[prop] to fill value; a mirror copies get/set across untouched.
  • Inherited keys.prop in data is true for the whole prototype chain, so the synthesis answered for toString too — an own, enumerable, writable data property no payload has ever held. Only an own key has a descriptor to mirror. 'toString' in input and the read itself are unchanged.

What this PR does NOT decide

Whether a record payload may carry an accessor at all, and what the engine should do persisting one (it persists a payload by evaluating it), is a contract question about the payload — triage was explicit that it must not be settled inside this card. It is not settled here, and the implementation did not have to take a position to land:

So the fork stands open, unchanged, and this PR neither widens nor narrows what a payload may hold.

Clause ② — judged, with the diff that supports it

 .changeset/flat-input-descriptor-mirror.md | 52 +++++
.../src/hook-input-descriptor-mirror.test.ts | 244 +++++++++++++++++++++
packages/objectql/src/hook-wrappers.ts | 43 +++-
3 files changed, 337 insertions(+), 2 deletions(-)

Not an accept-set narrowing. One source file, one trap body. No packages/spec/**, no *.zod.ts, no exported type, signature or .d.ts-shaping export — nothing a caller passes, and nothing a consumer type-checks or validates against, changed. What changed is what one runtime trap reports.

It is still observable behaviour on a shipped surface, and one reported answer gets strictly stricter (own-ness for inherited keys: fabricated descriptor → undefined), so it is carried as a patch changeset with the consumer evidence below rather than as a bare internal change.

Consumer sweep — downstream direction, stated because the claim is otherwise unreadable.pnpm --filter '...@objectstack/objectql' (prefix form) is the downstream set. Measured:

  • grep -rn "getOwnPropertyDescriptor" packages/*/src --include=*.ts (non-test) → the only hits are this trap and an unrelated one in packages/spec/src/shared/lazy-schema.ts. No in-repo consumer reads a descriptor off the flat input.
  • @objectstack/runtime is the one downstream package that pins this proxy. body-runner.test.ts, perrow-dispatch-signal.integration.test.ts, script-runner.test.ts41 passed (41), run after pnpm --filter '@objectstack/runtime^...' build because those tests reach objectql through package exports (its dist), not a vitest src alias — so this measured the built fix, not the source.
  • Enumeration is untouched by construction: ownKeys still lists exactly data's own enumerable keys and the mirror reports those as enumerable, so Object.keys, spread, Object.entries and the sandbox's unwrapProxyToPlain are byte-identical. The sibling delete ctx.input.<field> in a hook is a silent no-op — the flat-input Proxy traps get/set/has/ownKeys but not deleteProperty, and every read-back agrees the delete worked #12277 pin file asserts all three and stays 6/6 green.

Declared narrowing: the full downstream closure was not run locally — CI runs the farm.

Verification

Union derived, not recalled: node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack over the real changeset. All 26 matched families (21 path-derived + 5 convention-triggered) re-run at the final head 68122f71b2, each exit code captured before any pipe, 26/26 RC=0:

check:changeset-gate-self-tests check:cross-package-test-inputs check:durability-log-level
check:objectql-double-limit check:objectui-changeset check:page-declaration-shape
check:published-files check:slot-lookup check:test-source-alias
check:type-source-resolution check:nul-bytes check:query-options-erasure
check:type-check-coverage check:engine-double-contract check:where-matcher
check-adr-0087-registration check-changeset-no-major check-ci-filter-parity
check-comment-mask-adoption check-cross-package-test-inputs check-empty-changeset
check-engine-split-ratio check-plugin-teardown-shape docs-audit/check-affected-docs
docs-audit/check-drift-comment pm/release-rehearsal-clone --self-test

Suites, at the same head:

  • pnpm --filter @objectstack/objectql test238 files, 4197 passed (4197)
  • pnpm --filter @objectstack/objectql typecheck → RC 0

check:type-check-debt — measured by a declared narrowing, not skipped

The 26th family (check:type-check-debt, i.e. check-type-check-coverage.mjs --re-measure) refuses without a built workspace closure, and its throw means NOT MEASURED. Rather than leave it there, the program it would run for the one ledgered package this diff touches was reproduced directly: tsc --noEmit over packages/objectql with the tests included (they are excluded from the package's own tsconfig.json, which is exactly why the package carries a TEST_DEBT entry and why pnpm typecheck cannot see this file).

Result — 355 errors, of which 0 are in the new file (and 0 in the sibling hook-input-mutation-traps.test.ts). The composition reproduces the ledger's recorded 354 to the unit — TS2339 x115, TS2554 x113, TS7006 x36, TS2345 x24, TS2749 x14, TS2322 x14, TS6133 x9, TS2550 x8, TS18048 x8, TS2353 x4, TS6196 x1 — plus the single TS6059 rootDir diagnostic that measureTestDebt drops (dropRootDirDiagnostics: true), which the ledger note names explicitly. The recorded number cannot drift upward from this change, and no other ledger entry is in the diff.

That measurement also changed the code: the pin file originally used Object.hasOwn, which is ES2022 against this workspace's lib: ["ES2020"] — it would have added two TS2550 and reddened the ratchet invisibly to pnpm typecheck. It now uses Object.prototype.hasOwnProperty.call, with the reason on the line.

Ablation — one leg, predicted in writing first

Prediction, written before the run: reverting the mirror to the synthesis literal turns exactly 5 of 6 cases in the new file red (REPRODUCTION, writable: false, INVARIANT, INHERITED, accessor), leaves POSITIVE CONTROL green, and leaves the sibling #12277 file 6/6 green — the second half being the point: the existing pins cannot catch this defect, because the synthesis is the truth for ordinary assigned keys.

Observed, exactly:

PRE-MUTATION: injected=1 synthesis=0
LEG0 (committed) → Test Files 2 passed (2) · Tests 12 passed (12)
POST-MUTATION: injected=0 synthesis=1 ← anchored grep -cF, taken BEFORE any result was read
LEG1 (mutated) → Tests 5 failed | 7 passed (12)
× REPRODUCTION · × writable: false · × INVARIANT · × INHERITED · × accessor
RESTORE: injected=1 synthesis=0 · RESTORE_GIT_DIFF_RC=0 ← `git diff --exit-code` clean

Restore ran from a trap … EXIT INT TERM and touched only the one mutated path (never git checkout HEAD -- .). No rebuild leg is owed on either side: the pin file imports ./hook-wrappers.js relatively inside its own package, so vitest transforms src directly and no dist sits on the resolution path — evidenced by the pre-fix and post-fix runs differing with no build between them.

No existing test assertion was reversed.

Recorded, not ridden along


Generated by Claude Code

…12397)
`installFlatInput`'s `getOwnPropertyDescriptor` trap answered every key `data`
carries with one fixed literal and never read `data`'s real descriptor. Since
#12277 routed `defineProperty` into `data`, a hook can put a key on the record
payload with non-default attributes, and the synthesis reported the defaults
back regardless — `enumerable: false` read back as `enumerable: true` while
`Object.keys` correctly omitted the key.
The trap now mirrors `data`'s own descriptor, forcing `configurable: true`
because the proxy target is the wrapper, which does not carry the key: a
verbatim mirror is a proxy-invariant violation and throws `TypeError` on any
key held non-configurable, taking `Object.keys` and spread with it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o
@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 — 14 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 3f41a21527c611620295632e692bcefd56bd67edpackageMentionDocs.

Which tree this was computed on

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

⚠️ 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 26, 2026
@os-warren
os-warren marked this pull request as ready for review August 26, 2026 12:08
@os-warren
os-warren enabled auto-merge August 26, 2026 12:08
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — accepted, flipped ready, auto-merge armed

Measured: 3 files, +337/−2, all in-scope. One source file, one new pin file, one changeset. No drive-by.

The fork really was left open — verified in the diff, not taken on the report's word

This was the one thing the dispatch was strict about, so I read the trap body rather than the summary. The whole repair is two lines:

constown=Object.getOwnPropertyDescriptor(data,prop);if(own)return{ ...own,configurable: true};

The spread carries get/set across untouched, so the trap takes no position on whether a record payload may hold an accessor — it reports whatever is there under every answer to that question. Routing and persistence are not in the diff. forkReported: false is therefore the right outcome, not an evasion: the implementation genuinely did not need to decide it.

Worth naming why the other obvious shape would have been a position: flattening an accessor to { value: data[prop] } decides the question by evaluation — and that is exactly the side-effecting synthesis this PR removes.

Two bugs fell out of "mirror, don't synthesise", and both are real

Neither was in the card. Both are consequences of doing it properly:

  • A descriptor read no longer runs author code. The old synthesis evaluated data[prop] to fill value, so asking a payload holding a getter for its descriptor invoked the getter. A mirror copies the accessor across instead.
  • Inherited keys no longer get a fabricated own descriptor.prop in data is true up the whole prototype chain, so the synthesis answered for toString — reporting it as an own, enumerable, writable data property no payload has ever held. Only an own key has a descriptor to mirror; Object.getOwnPropertyDescriptor fixes this by construction rather than by an added guard.

That second one is a behaviour change in the strict direction (a descriptor becomes undefined), and it is correctly declared rather than buried, with a downstream sweep behind it: zero non-test consumers read a descriptor off the flat input, and @objectstack/runtime — the one downstream package pinning this proxy — is 41/41 green, run after building the dependency closure because those tests reach objectql through package exports to dist, not a src alias. Getting that right is the difference between a real consumer sweep and a green run against stale artefacts.

The configurable forcing is explained where it will be read

The in-source comment states the constraint and its blast radius: the proxy target is the { data, options, id? } wrapper, which does not carry the record key, so mirroring configurable verbatim throws TypeError on any key data holds non-configurable — and takes Object.keys and spread down with it, since both reach every listed key through this trap. And it says plainly that the forcing is the proxy's constraint, not a claim about the payload. That is the sentence a future reader needs.

Ablation

Predicted 5 of 6 failures in the new pin file and that the sibling #12277 trap-set file stays 6/6 green — i.e. 5 of 12 across both. Observed exactly that, the same five by name. The green sibling is the load-bearing half: it shows the existing #12277 pins cannot catch this defect, because the synthesis is the truth for ordinary assigned keys. Mutation proved on disk with a two-way anchor before any result was read; restore touched only the mutated path (explicitly not git checkout HEAD -- .) and was verified with git diff --exit-code.

The not-measured that was handled better than a not-measured usually is

check:type-check-debt refused to run without a built workspace closure. Rather than leave it dark or call it not-applicable, the dev reproduced the program it would run over the one ledgered package in the diff, and matched the ledger's recorded composition error-code by error-code (TS2339×115, TS2554×113, TS7006×36, …) — so the recorded number cannot drift upward from this change.

And that substitute measurement caught a real defect in the dev's own new test file: it used Object.hasOwn (ES2022) against this workspace's lib: ES2020, which would have added TS2550s and reddened the ratchet invisibly to pnpm typecheck, because the package tsconfig excludes tests — which is precisely why that package carries a TEST_DEBT entry in the first place. Now Object.prototype.hasOwnProperty.call, with the reason on the line. A gate refusing to measure is not an excuse to stop measuring, and this is what the alternative looks like.

Follow-on

#12578 filed for the adjacent ownKeys asymmetry — an own non-enumerable key is absent from Object.getOwnPropertyNames(input) while hasOwnProperty and (after this PR) the descriptor trap both report it as own; symbol keys are never enumerated either. Correctly not repaired here: Reflect.ownKeys(data) would newly expose symbol keys and cuts across the sandbox's documented "materialises only what ownKeys enumerates" contract, whose own test double models the trap as Reflect.ownKeys. That is a second card, not a wider diff.

The applyMutationsToInput bigint residual stays recorded, not filed — the charter's condition was "only if that file is already open", and it is not; this fix is entirely in objectql. Correct call.

⚠️ Report marker eaten by the sanitizer again — fifth instance today, cross-posted to #12133. The authoritative report is the second comment on #12397.

CI is the remaining gate.


Generated by Claude Code

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

2 participants

@os-warren@claude