Skip to content

fix(engine): set_null on a multiple:true reference removes the deleted member; emptied set stored as [], never null; drop the #9437 interim 409 hold - #9520

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-9438-set-null-member-removal
Aug 18, 2026
Merged

fix(engine): set_null on a multiple:true reference removes the deleted member; emptied set stored as [], never null; drop the #9437 interim 409 hold#9520
os-zhuang merged 1 commit into
mainfrom
claude/issue-9438-set-null-member-removal

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#9438

What this does

deleteBehavior: 'set_null' on a multiple: true reference field now removes the deleted member from the stored array and writes what remains, instead of nulling the whole slot (which discarded every other live reference). The interim restrict escalation shipped with #9437 as an explicit holding position is removed in the same stroke — it was built to be removed, and the ruling that unblocked this card has landed.

  • Member removal: next = current.filter(v => String(v) !== String(id)), then write next. Whole-member comparison — the same reading the dependents narrowing already applies — so an id that is a prefix of another neither loses its own member nor takes its neighbor's.
  • Residual shape is consumed, not decided here: an emptied set is written as [], never null — the representation FieldSchema pins as verbatim contract (packages/spec/src/data/field.zod.ts, the multiple and required doc blocks; [Decision] Multi-value lookup after set_null member removal: does the emptied array read back as [] or null? #9447, maintainer ruling 2026-08-18, binding for every writer). The engine comment cites that file rather than restating the sentence.
  • Interim revert: the escalation if, its multiValueHold flag, and the TEMPORARY developerMessage limb are gone. Completion check from the dispatch: grep -c for multiValueHold, TEMPORARY, and 9438 in packages/objectql/src/engine.ts — all three are 0.
  • Untouched, deliberately: the fix(engine): probe a multiple:true reference field with a spelling its storage answers #9437 probe repair ($contains + exact element-wise narrowing), the required-FK escalation, cascade, explicit restrict, and the single-valued set_null limb.

Out of scope, still open: #9476 is not addressed here (the validator still passes [] on a required + multiple: true lookup; the ruled non-empty semantics remain documented-but-unenforced). This change neither helps nor hinders it mechanically — the member-removal write goes through the ordinary update path, so when #9476 lands its enforcement, an emptying cascade repair on a required multi-value lookup will surface that contradiction loudly, which is the #9438 issue's own "required FK cannot be honestly cleared" reasoning one level up. Worth a sentence on that card, left to its implementer.

Tests

Union run at 18c20564a5 (the head of this branch; working tree clean at every run quoted below).

  • packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts — the hold suite is rewritten into the member-removal suite: defaulted and explicit set_null member removal, the emptying case asserted literally (toEqual([]), not.toBeNull(), Array.isArray), whole-member prefix safety, multi-row independence, restrict-with-its-own-sentence (no TEMPORARY / no card citation outliving the hold), plus the four controls. 17/17 green. Full package suite: 3828/3828 green.
  • packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts — on a real SqlDriver (better-sqlite3) through the real data-plane delete, asserting on the database via the driver's own connection: ["acc_a","acc_b"] minus deleted acc_a re-reads as ["acc_b"] and acc_b still resolves; the emptying case re-reads as [], explicitly not null. 7/7 green. Full package suite: 2510/2510 green.
  • Typecheck: @objectstack/objectql and @objectstack/runtime both clean.

Reverse verification (all three legs; runtime resolves objectql from dist, so each ablation leg was rebuilt and proved live via scripts/ablation-dist-preflight.mjs before its colour was trusted; the restore leg proved all markers absent from dist)

  • Leg 1 — member-removal write ablated alone (null write restored, hold still removed): the member-removal pins went RED in the expected direction — expected null to deeply equal [ 'acc_b' ] and expected null to deeply equal [] — 5 unit + 2 integration failures, everything else green.
  • Leg 2 — escalation-revert ablated alone (hold re-added, member removal kept): the old 409 behaviour returned — the same 7 success pins went RED on DELETE_RESTRICTED refusals.
  • Leg 3 — widen direction for the "guard does not fire" control: multiValued forced true made the single-valued set_null control go RED (expected [] to be null), proving it non-vacuous. Run at the unit layer (src-resolved, no dist involved). This is the direction used, per the over-fire caution in the dispatch: removal-direction ablations cannot exercise that control.

Gates

Derived via node scripts/pm/dispatch-gates.mjs over the changed paths; all derived families run locally and green at this tree: changeset-gate-self-tests, cross-package-test-inputs (pnpm + node forms), durability-log-level, objectui-changeset, stack-collection-maps, adr-0087-registration, changeset-no-major, empty-changeset, engine-split-ratio, affected-docs, query-options-erasure, where-matcher, engine-double-contract, type-check-coverage, type-check-debt (--re-measure, after the full packages closure build), nul-bytes. The ratchet families (query-options-erasure, where-matcher, engine-double-contract, nul-bytes) were re-run at the committed head 18c20564a5; type-check-debt ran on the byte-identical tree the commit was created from.

Changeset: .changeset/cascade-set-null-multivalue-member-removal.md (patch, @objectstack/objectql) — user-visible: deletes that returned 409 under the interim now succeed and remove the member.


Generated by Claude Code

…d member, emptied set stored as [] per the ruled FieldSchema contract; drop the #9437 interim 409 hold
The set_null limb on a set-valued foreign key now filters the deleted id
out of the stored array and writes what remains; an emptied set is written
as [], never null — consuming the representation FieldSchema pins
(packages/spec/src/data/field.zod.ts, multiple/required doc blocks, #9447
maintainer ruling 2026-08-18). The temporary restrict escalation and its
TEMPORARY developerMessage limb shipped with #9437 are removed in the same
stroke, as ruled.
Closes#9438.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/objectql, touching 4 documentable anchor(s).

6 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/data-api.mdx(via set_null (literal))
  • content/docs/data-modeling/field-types.mdx(via set_null (literal))
  • content/docs/data-modeling/fields.mdx(via set_null (literal))
  • content/docs/data-modeling/validation-rules.mdx(via set_null (literal))
  • content/docs/deployment/troubleshooting.mdx(via set_null (literal))
  • content/docs/protocol/objectql/types.mdx(via set_null (literal))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v15.mdx(via set_null (literal))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: ObjectQL (symbol, 59 pages)

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 origin/mainpackageMentionDocs.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

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

Copy link
Copy Markdown
ContributorAuthor

PM review — accepted. Every instruction in the brief is discharged, and two of them are discharged better than specified.

The prefix hazard, carried to the write side unprompted

Whole-member comparison — the same reading the dependents narrowing already applies — so an id that is a prefix of another neither loses its own member nor takes its neighbor's.

#9437 found that $contains over-matches on the read side (acc_1 matching a row holding acc_10) and narrowed exactly to fix it. The same hazard exists on the write side here, in a different function, and a naive substring or loose comparison would silently drop the wrong member — a data-loss bug in the very fix for a data-loss bug. Nothing in the brief told you to look for it. Reusing the existing reading rather than writing a second comparison is also what keeps the two from drifting apart later.

The citation discipline held

The engine comment cites that file rather than restating the sentence.

That was the instruction and it is the part most likely to be quietly ignored, because paraphrasing reads better in situ. The spec seat owns that sentence (#9447#9493, df0c12de7); an engine-local restatement would have become a second source of truth that can drift from FieldSchema without any gate noticing.

The completion check, run and reported

grep -c multiValueHold | TEMPORARY | 9438 in packages/objectql/src/engine.ts -> 0, 0, 0

The interim was built to be removable and is now removed without residue. An orphaned TEMPORARY marker outliving the thing it described is exactly the failure the citation scheme was designed to prevent, and you closed it rather than leaving it to be noticed later.

Three-leg reverse verification, each leg proved live

Ablating each limb separately is what makes the two-change PR legible: leg 1 restores the null write with the hold still gone (member-removal pins RED, expected null to deeply equal [ 'acc_b' ]); leg 2 re-adds the hold with member removal kept (the same pins RED on 409). Neither leg could have been inferred from the other.

Leg 3 is the one I want on the record. The "guard does not fire" control cannot fail under an ablation that removes the guard — it is green by construction. You ablated the other way, forcing multiValued true, and the single-valued control went RED (expected [] to be null), proving it non-vacuous — and you noted it ran at the unit layer, src-resolved, no dist involved. That is the #9437 lesson carried forward correctly into a different shape, not just repeated.

And the restore leg proving all markers absent from dist closes the loop on the artifact question: packages/runtime resolves objectql from dist, so an unrebuilt ablation would have run the fixed code and gone green — a false "the test doesn't cover it".

Asserting on the database through the driver's own connection["acc_a","acc_b"] minus acc_a re-reading as ["acc_b"] with acc_b still resolving, and the emptying case as [] with not.toBeNull() — is the right proof shape. Asserting the literal type rather than a truthiness check is what makes the ruled contract actually pinned.

#9476 — correctly out of scope, and the forward note is useful

when #9476 lands its enforcement, an emptying cascade repair on a required multi-value lookup will surface that contradiction loudly, which is the #9438 issue's own "required FK cannot be honestly cleared" reasoning one level up

That is a real observation and you were right to leave it as a sentence for that card's implementer rather than act on it. It also means #9476's implementer should expect this interaction rather than discover it — worth carrying over when they pick it up.


One thing for you to close out: the docs

The Docs Drift Check lists six hand-written pages naming set_null. On #9437 I ruled no doc edits, because the interim was designed to be deleted and six pages describing a temporary refusal would have to be unwritten. I said then that the docs question "becomes real and permanent when #9438 answers the semantics; it should be settled there, once."

This is that moment, and the answer is now permanent: set_null on a multiple: true reference removes the member and empties to []. None of those six pages says so.

My call: do not edit them in this PR. Six hand-written pages spanning surfaces this diff does not otherwise touch is doc churn riding a code change, and the contract sentence already landed where it belongs (FieldSchema). But the gap is real and should not evaporate now that the semantics are settled.

Please file it unassigned as an out-of-scope finding before you close out — the way you handled #9476 — naming the six pages and the one-sentence gap each needs. You have the context; whoever picks it up will not. ⛔ The one release-owned page in that list stays read-only regardless.

Next

CI is mid-flight, nothing red, and Lint & Repo Gates is reporting — so your base is post-rename and this will not hit the block that cost #9437 and #9458 an hour tonight. I'll flip and enqueue with mergeMethod: SQUASH once green.

Fixes #9438 is correct on this PR — it is the card's real fix. (The no-closing-keyword rule applied only to #9437, which shipped the holding position.)


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 18, 2026 07:21
@os-zhuang
os-zhuang added this pull request to the merge queueAug 18, 2026
Merged via the queue into main with commit 91c6c28Aug 18, 2026
26 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-9438-set-null-member-removal branch August 18, 2026 07:43
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.

cascadeDeleteRelations' set_null limb nulls the WHOLE multi-value array, dropping every other live reference

2 participants

@os-zhuang@claude