Skip to content

Commit 2fe0d87

Browse files
committed
fix(objectql): set-to-undefined of an engine-hidden readonly key is a no-op, not a hook write (#16344)
Contract-review finding F1. A `beforeUpdate` hook that assigns a hidden key from the payload it was shown (`data.x = data.x`) reads `undefined` and re-creates the key holding it. Three mechanisms then agreed the wrong way: the recorder's `set` trap counted it as a hook write, the hand-back skipped the key because `k in target`, and the static strip kept it on that record — so a driver was handed `{ x: undefined }`. On the memory driver that ERASES the stored read-only value; on a knex-backed one `formatInput` does not drop `undefined` and `builder.update(payload)` hands knex an undefined binding — a bare compile-time Error outside the ADR-0112 envelope. Neither is "the record the engine intends to persist". At the confluence the key is now deleted, dropped from the sealed record, and the ordinary hand-back puts the caller's value back for the strip to judge. The write reads exactly as it would with no hook at all: stripped, reported on `onFieldsDropped`, warned, refused under `strictReadonlyWrites`. Dropping the key from `hookWrittenKeys` is load-bearing, not tidiness: handing the caller's value back over a key the record still calls hook-owned would credit the forgery with hook provenance. The narrowing reaches only keys this card's pass hid, and only the one value no driver can store, so #14088's deliberate blindness to VALUE is unchanged for every key a hook can see. The pin moves in both directions: the stored value now STANDS (`completed_at === STAMPED`, the seed), and the FORGED negative stays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
1 parent 6375bc2 commit 2fe0d87

2 files changed

Lines changed: 72 additions & 15 deletions

File tree

packages/objectql/src/engine-readonly-strip-caller-values.test.ts

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -758,7 +758,7 @@ describe('the strip reads hook-write PROVENANCE, not value equality (#14088)', (
758758

759759
// ── The measured consequence of "an assignment ran", stated out loud ───────
760760

761-
it('MEASURED: a lone self-assigning hook can no longer leave the CALLER value on the key (#16344)', async () => {
761+
it('MEASURED: a lone self-assigning hook is a NO-OP on a hidden key — the STORED value stands (#16344)', async () => {
762762
// ⚠️ RECORDING BEHAVIOUR, NOT BLESSING IT. The direct consequence of the
763763
// mechanism #14088 chose: the record says an ASSIGNMENT RAN and is
764764
// deliberately blind to the VALUE (that blindness is the whole repair — it
@@ -794,19 +794,33 @@ describe('the strip reads hook-write PROVENANCE, not value equality (#14088)', (
794794
// ⭐ [#16344] THE VERDICT MOVED, and this is the record of it rather than a
795795
// deletion. Everything above still describes the recording mechanism
796796
// exactly; what changed is what a `beforeUpdate` hook is SHOWN. The
797-
// caller's forged `completed_at` is now hidden from the hook, so
797+
// caller's forged `completed_at` is hidden from the hook, so
798798
// `ctx.input.data.completed_at` reads `undefined` and the self-assign
799-
// writes THAT — a hook write of `undefined`, faithfully persisted, rather
800-
// than the caller's timestamp promoted to hook-owned.
799+
// re-creates the key holding THAT.
801800
//
802-
// Two things follow, and both are the point:
803-
// - the laundering route this case existed to make VISIBLE is now CLOSED.
804-
// A no-op-looking line can no longer confer hook provenance on a value
805-
// the caller minted, because the line can no longer reach that value.
806-
// - the line is still not a no-op. It is an assignment, so the hook owns
807-
// the key and the column is written — with what the hook assigned. That
808-
// is #14088 working as designed, and it is why this case stays pinned
809-
// with the new reading instead of being dropped as fixed.
801+
// ⛔ And a hook write of `undefined` is NOT persisted as one. The
802+
// confluence treats set-to-undefined of a HIDDEN key as the no-op the line
803+
// actually is: the key is deleted, its entry leaves the record, and the
804+
// ordinary hand-back puts the caller's value back for the strip to judge.
805+
// The write then reads EXACTLY as it would have with no hook at all —
806+
// stripped, reported, warned, refused under `strictReadonlyWrites`.
807+
//
808+
// The direction matters, and it is why this is not `toBeUndefined()`:
809+
// - persisting the `undefined` ERASES the stored value on the memory
810+
// driver and hands knex an undefined binding on a SQL one (a bare
811+
// compile-time Error, outside the ADR-0112 envelope). Neither is "the
812+
// record the engine intends to persist", which is this card's whole
813+
// subject.
814+
// - the laundering route this case existed to make VISIBLE stays CLOSED.
815+
// A no-op-looking line still cannot confer hook provenance on a value
816+
// the caller minted — the FORGED negative below is what pins that, and
817+
// it must stay red-able: drop the record-narrowing at the confluence and
818+
// the caller's timestamp is handed back onto a key the record still
819+
// calls hook-owned, and it commits.
820+
//
821+
// ⚠ The blindness to VALUE stated above is unchanged for every key the
822+
// hook can actually SEE. This narrowing reaches only keys hidden by this
823+
// card, and only the one value — `undefined` — that no driver can store.
810824
const FORGED = '1999-01-01T00:00:00.000Z';
811825
engine.registerHook('beforeUpdate', async (ctx: any) => {
812826
ctx.input.data.completed_at = ctx.input.data.completed_at;
@@ -817,9 +831,12 @@ describe('the strip reads hook-write PROVENANCE, not value equality (#14088)', (
817831
id: 't_20', status: 'in_progress', completed_at: FORGED,
818832
});
819833

834+
// The FORGED negative, KEPT: the laundering route stays closed.
820835
expect(task('t_20').completed_at).not.toBe(FORGED);
821-
expect(task('t_20').completed_at).toBeUndefined();
822-
expect(warns).toEqual([]);
836+
// ⭐ The re-pin. The stored value STANDS — not erased, not `undefined`.
837+
expect(task('t_20').completed_at).toBe(STAMPED);
838+
// ...and the write is reported exactly as an un-hooked one would be.
839+
expect(warns.some((w) => w.includes("Field 'completed_at'"))).toBe(true);
823840
});
824841

825842
it('the recording is transparent to a hook reading its own payload', async () => {

packages/objectql/src/engine.ts

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11762,7 +11762,7 @@ export class ObjectQL implements IObjectQLEngine {
1176211762
// deliberately keeps the pre-#14088 over-strip instead.
1176311763
const sealedHookWrites = hookWrites?.seal(hookContext.input.data);
1176411764
if (sealedHookWrites) hookContext.input.data = sealedHookWrites.data as any;
11765-
const hookWrittenKeys = sealedHookWrites?.hookWrittenKeys;
11765+
let hookWrittenKeys = sealedHookWrites?.hookWrittenKeys;
1176611766

1176711767
// ── [#16344] HAND BACK what was hidden from the hooks ────────────────
1176811768
//
@@ -11782,17 +11782,57 @@ export class ObjectQL implements IObjectQLEngine {
1178211782
// below reads that record for provenance — so the caller's own forgery
1178311783
// would be handed the one credential (`hookWrittenKeys`) that stops it
1178411784
// being stripped. The exact laundering #14088 exists to prevent.
11785+
//
11786+
// ⛔ ...and SET-TO-UNDEFINED of a hidden key is a NO-OP, not a hook
11787+
// write. A hook that assigns a hidden key from the payload it was shown
11788+
// (`data.x = data.x`, the shape #14088's own pin names) reads
11789+
// `undefined` and RE-CREATES the key holding it. Left alone, three
11790+
// mechanisms agree the wrong way: the recorder's `set` trap counts it as
11791+
// a hook write, the hand-back below skips the key because `k in target`,
11792+
// and the strip keeps it on that record — so a driver is handed
11793+
// `{ x: undefined }`. On the memory driver that ERASES the stored
11794+
// read-only value; on a knex-backed one `formatInput` does not drop
11795+
// `undefined` and `builder.update(payload)` hands knex an undefined
11796+
// binding, a bare compile-time `Error` OUTSIDE the ADR-0112 envelope.
11797+
// Neither is "the record the engine intends to persist", which is the
11798+
// whole subject of this card.
11799+
//
11800+
// Undoing it here — delete the key, drop it from the record, let the
11801+
// ordinary hand-back put the caller's value back for the strip to judge
11802+
// — makes the write read EXACTLY as it would have with no hook at all:
11803+
// stripped, `onFieldsDropped` reporting it, the WARN said, and
11804+
// `strictReadonlyWrites` refusing. That identity IS the invariant this
11805+
// hide/hand-back pair exists to hold.
11806+
//
11807+
// ⛔ Dropping the key from `hookWrittenKeys` is NOT optional and is not
11808+
// tidiness: leaving it there while handing the caller's value back over
11809+
// it would credit the caller's forgery with hook provenance — the exact
11810+
// laundering the note above refuses, arrived at from the other side. The
11811+
// narrowing reaches only keys THIS pass hid, and only the one value no
11812+
// driver can store; a hook write of any real value is untouched, so the
11813+
// recorder's deliberate blindness to VALUE (#14088) is unchanged for
11814+
// every key a hook can actually see.
1178511815
if (readonlyHiddenFromHooks) {
1178611816
const restoreTargets = new Set<Record<string, unknown> | null | undefined>([
1178711817
hookContext.input.data as Record<string, unknown> | null | undefined,
1178811818
opCtx.data as Record<string, unknown> | null | undefined,
1178911819
]);
11820+
const undoneSelfAssigns = new Set<string>();
1179011821
for (const target of restoreTargets) {
1179111822
if (!target || typeof target !== 'object') continue;
1179211823
for (const [k, v] of Object.entries(readonlyHiddenFromHooks)) {
11824+
if (k in target && target[k] === undefined) {
11825+
delete target[k];
11826+
undoneSelfAssigns.add(k);
11827+
}
1179311828
if (!(k in target)) target[k] = v;
1179411829
}
1179511830
}
11831+
if (undoneSelfAssigns.size > 0 && hookWrittenKeys !== undefined) {
11832+
const narrowed = new Set(hookWrittenKeys);
11833+
for (const k of undoneSelfAssigns) narrowed.delete(k);
11834+
hookWrittenKeys = narrowed;
11835+
}
1179611836
}
1179711837

1179811838
// ── [#13657] The POST-hook half of the declared-field door ──────────

0 commit comments

Comments
 (0)