Uh oh!
There was an error while loading. Please reload this page.
fix(objectql): decide the read-only strip on hook-write provenance, so a hook can clear a field the caller also sent - #14258
Conversation
…14088) `stripReadonlyFields` asked `Object.is(payload[k], supplied[k])` to answer "who wrote this key". Value equality cannot: when a hook writes the value the caller also sent, the comparison reads it as "the hook never touched it" and deletes the hook's write. Measured downstream on a `readonly` `completed_at` cleared by a reopen hook against a caller that round-tripped the record — the row committed `in_progress` carrying its old completion timestamp, silently. Record the keys the before-phase hook chain actually ASSIGNS (`recordHookPayloadWrites`), armed after the caller's entry snapshot and sealed at the post-hook confluence both update branches share, and let the strip keep a key a hook demonstrably wrote. Not a `null` sentinel: `0`, `''`, `false` and shared references collide identically. Not a relaxation: a caller-supplied read-only value no hook wrote is still stripped, and a caller cannot enter the record because echoing a value is not an assignment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
…insertion (#14088) Pure line rot: the #14088 import block shifts every subsequent engine.ts line by five, so all 15 anchors on content/docs/permissions/system-context.mdx moved. Repaired with the gate's own `--fix`; no prose changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
📓 Docs Drift CheckThis PR changes 1 package(s): 2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 15 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 1e32c142f7baffc21a5eafda24c5c8162b458f34 && git checkout 1e32c142f7baffc21a5eafda24c5c8162b458f34
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin d63c8a25216c59b9bf2652cf5197aa526a2c00b2 7a486e32f78821e3decda1c49dddb767c8ad77b7 && git checkout -B drift-repro d63c8a25216c59b9bf2652cf5197aa526a2c00b2 && git merge --no-ff 7a486e32f78821e3decda1c49dddb767c8ad77b7
node scripts/docs-audit/affected-docs.mjs --json d63c8a25216c59b9bf2652cf5197aa526a2c00b2
|
…adonly-strip-provenance
The merge driver treats content/docs/permissions/system-context.mdx as generated and does not text-merge it, so the merge left this branch's pre-merge copy in place — which would have dropped the rows PR #14249 added on main. Took main's copy wholesale and re-derived the anchors from the MERGED tree with the repo's own tooling (pnpm gen:system-context-census). Result verified against main's copy: 12 rows differ and every difference is an engine.ts line number inside backticks; all prose is byte-identical, so #14249's rows survive intact. The 15 rewritten anchors are the same 15 this branch's engine.ts insertion shifts, and the census totals are unchanged (109 read sites / 145 anchors / 27 declared non-read), so this is line rot and not a population change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
Uh oh!
There was an error while loading. Please reload this page.
Closes#14088
stripReadonlyFieldsaskedObject.is(payload[name], supplied[name])to answer who wrote this key. Value equality cannot carry that question, and this is it failing: when a hook writes the value the caller also sent, the comparison reads it as the hook never touched it and deletes the hook's write.Reproduced first, on
origin/main's predicate, then fixed. The reproduction is the ablation below.The defect, and why it is the same defect twice
#5591 / #6339 retired the key-SET judgement because it made the strip's own written contract —
runtimeOwnedStripWarning's promise that "hook-written keys are NOT caller-supplied" — true only by accident. Value equality is accidental in precisely the same way, andnull == nullis just its most common collision. So this is #6339's residual item, not a new defect, and that framing is load-bearing: the repair is not a better comparison, it is the end of comparing.Measured downstream (published 17.2.0,
duly_task): areadonlycompleted_atstamped by abeforeUpdatehook on the transition intodoneand cleared on the transition out. Reopening works — until the caller also sendscompleted_at: null, which is what a form round-trip of the whole record does.Object.is(null, null)istrue, the hook's clear is stripped, and the row commitsstatus = in_progressstill carrying its old completion timestamp, with no error. That row is the one a validation rule structurally cannot catch ("a completed task must carry a completion timestamp" has no purchase on its inverse), nothing downstream can tell it from a genuinely completed one, and every on-time metric readingcompleted_atcounts it.The repair: provenance, recorded — never inferred, never a sentinel
New
packages/objectql/src/hook-write-provenance.ts.recordHookPayloadWritesreturns a write-through view of the update payload that records the fact that an assignment executed — never the payload's contents.engine.update()arms it athookContextconstruction and seals it at the post-hook confluence #13657 already uses, wherehookContext.input.datais final on both update branches.stripReadonlyFieldsgains an optionalhookWrittenKeysand keeps a key a hook demonstrably assigned.Both branches consume the one sealed record. Not a second derivation: two notions of "a hook wrote this key" that disagree in one edge case would be worse than the defect they each closed, and a bulk write reaching a different verdict about authorship than a by-id write is the #3106 / #4441 divergence verbatim.
⛔ Not a
nullspecial case.0,'',falseand a shared object reference collide identically, and the same whole-record write-back idiom echoes all of them. The0twin is pinned as its own test and it fails under ablation alongside thenullone.The forgery boundary — the one place a mistake is worse than the bug
A hook-owned key is a key the strip stops defending, so this is the part that had to be structural rather than careful. The record cannot be reached by caller data, because:
encryptSecretFields,normalizeMultiValueFields, the strips). Between those two points the only code that runs is before-phase hook code. Sealing is not tidiness: a recorder still armed forencryptSecretFieldswould attribute an engine write to a hook, which on a caller-forged secret column is exactly the escalation this is built to prevent;null, sending aProxy, sending a getter — none of them is aset.The record therefore only ever turns a strip into a keep, and only for a key a hook assigned. Every other key is decided by the two-part test that was already there, unchanged.
The forgery pin at
engine-readonly-strip-signal.test.ts:290-292is untouched and passes. It callsstripReadonlyFieldswith no provenance argument, and absent provenance the function is byte-identical to before.The discriminator, pinned in both directions. The same caller payload —
completed_at: nullover a stored timestamp — now clears when a hook wrote the null and is still stripped (with the same warn) when no hook did. Two opposite verdicts on byte-identical caller input is what value equality cannot deliver and a record can; it is also what separates this from "stopped stripping read-only fields".Known limit, deliberately fail-safe, pinned as a test
A hook that replaces the payload (
ctx.input.data = { ...ctx.input.data, x: 1 }) rather than mutating it leaves no attributable record — the replacement's keys are indistinguishable from the caller's because most of them are the caller's — sosealreturnsundefined(not an empty set) and that call falls back to the previous value comparison, i.e. it keeps the old over-strip. Reading a replacement's keys as hook-owned would launder a caller's forgedcreated_byinto a platform write, so the fallback direction is the only safe one. Two tests pin it, including the one proving the fallback is the whole pre-repair behaviour and not a new hole. The pre-existing shallow-snapshot limit (a hook mutating a caller object in place) is unchanged, for the same argument.Measured, including what the card did not measure
Verification tree:
7a486e32f(every reading below is from that tree, taken after the final commit — re-run there after the merge round, not carried over from the pre-merge head).Merge round.
mainlanded PR #14249 after this branch's merge base and re-anchored other rows ofcontent/docs/permissions/system-context.mdx, the generated census page, leaving the PRdirty. Resolved by mergingorigin/main(a merge commit — never a rebase, never a force-push). The repo's merge driver deliberately does not text-merge that page and says so, which left this branch's pre-merge copy in place — that would have silently dropped #14249's rows, so main's copy was taken wholesale and the anchors re-derived from the merged tree with the repo's ownpnpm gen:system-context-census. It did not refuse, and the census totals are unchanged (109 read sites / 145 anchors / 27 declared non-read), so this was line rot and not a population change. Verified against main's copy: 12 rows differ and every difference is anengine.tsline number inside backticks — all prose byte-identical.All six content files of this PR are byte-identical across the merge (
git rev-parseon each blob,efda8da21vs7a486e32f): only the generated page moved. That is what lets the ablation reading below stand as measured — the file it mutates,validation/rule-validator.ts, is the same blob at both heads.Whole package, unablated, re-run on the merged tree:
pnpm --filter @objectstack/objectql exec vitest run --maxWorkers=2—Test Files 253 passed (253)·Tests 4379 passed (4379), wrapperVERDICT command-exit 0. Identical to the pre-merge run, so the merge changed nothing behavioural. Dependency closure rebuilt first (pnpm --filter '@objectstack/objectql^...' build,VERDICT command-exit 0), sincemainmovedpackages/specandpackages/typesunderneath it.Ablation — the reproduction. Provenance consult deleted from
rule-validator.ts(marker count 1 to 0 confirmed on disk; blob1f205514to0e478d69; restored blob compared back to the HEAD blob andgit diff HEADempty). Every import in these suites is relative and in-package, so vitest runs the TS source and nodist/is in the resolution path — the mutation changing behaviour with no rebuild is itself the proof.7 failed | 45 passed (52)— and which 7 is the finding:completed_at: nullexpected '2026-08-01T09:00:00.000Z' not to be '2026-08-01T09:00:00.000Z'— the stale timestamp, exactly as reported0expected 42 to be +0expected null to be '2026-08-05T10:00:00.000Z'expected '2026-08-01T09:00:00.000Z' to be nullexpected null to be '2026-08-05T10:00:00.000Z'onFieldsDroppedstrictReadonlyWritesrefusing a hook's own clearTwo things that answers. The stamp direction is broken symmetrically — the card measures only the clear, and the framing is exactly as wide as the tree. And both update branches were broken, so both had to be fixed; a by-id-only repair would have left the predicate path corrupt.
Green under ablation, i.e. genuine negative controls: the bare
{ status }reopen, the partial patch,isSystem, every forgery face, the replacement fallback, all recorder unit tests, and the whole ofengine-readonly-strip-signal.test.tsincluding the forgery pin. The fix moves the collision cases and nothing else.Gates. Re-derived on the merged tree by
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackwith no paths passed — it read the change set itself (7 paths vs merge basea39b02a6b). The family grew from 33 to 58, because the change set now carries acontent/docs/**path and that pulls in the whole docs family; nothing dropped out. All 58 run, every exit code captured by redirect-then-read and never through a pipe. 54 green, pluscheck:nul-bytesandcheck-engine-double-contractrun outside the derived list. Four readings that are not greens, and none is a finding:check-system-context-census— the reason for this round; green on the merged tree, and its--self-testpasses alongside.check-engine-split-ratioexit 2 = CANNOT COMPUTE (shallow container clone). Report-only by default, and its exit 2 is documented as distinct from a finding. NOT MEASURED locally; the scheduled workflow checks out at full depth.check-test-completenessexit 3 = PREREQUISITE NOT MET (it grades a saved turbo log; none exists locally, and its own text says to record this as NOT MEASURED). NOT MEASURED locally.check:dual-build-cjs-loadsexit 3 andspec check:skill-examplesexit 1 — both PREREQUISITE NOT MET for the same reason: they read builtdist/output, and this container built only the@objectstack/objectqldependency closure, not all 56 packages. Both refuse rather than conclude ("⛔ This is NOT a pass: nothing was measured"/"a verdict now would be… a FALSE GREEN"), and both name packages this diff does not touch (client-react,hono,apps/*). NOT MEASURED locally; CI builds everything. They are new to this round only because the docs path widened the family — neither was a green that turned red.Typecheck
pnpm --filter @objectstack/objectql typecheckgreen on the merged tree too (VERDICT command-exit 0), and measured to cover the new tests rather than assumed:tsc -p tsconfig.test.json --listFilesfinds all three new/edited files, and the 242 error lines are exactly the ledgered debt with none attributed to them.Lint, as a declared narrowing. Repo-wide
pnpm lintwas not run; the five changed files were,--format json, 5/5 linted (none ignored), 0 errors / 0 warnings. The narrowing is measurable rather than merely narrow: this repo runs oneeslint.config.mjswhich never enables type-aware linting for any file — stated ateslint.config.mjs:326-335with a measured positive control — so no untouched file's verdict can move as a consequence of this diff.Scope
engine.ts'smulti: truebatch-hook path is untouched.dispatchPerRowBeforeHooksis not modified; the predicate branch participates only by reading the same sealed record at the same shared seam. #14099 remains open and is not addressed here.⭐ One thing worth the next dispatch's attention: this record does answer #14099's detection question. That card's own suggested direction asks for an engine that can tell "a hook mutated the payload on a
multiupdate" — and after the seal, on the predicate branch,hookWrittenKeysis exactly that set, non-empty precisely when a batch hook wrote the shared SET clause. Deliberately not acted on here.Session, kept in prose so it survives body edits:
https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5🤖 Generated with Claude Code
https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
Generated by Claude Code