Skip to content

fix(engine): judge the required multi-value cascade escalation per row, not per field (#9688) - #9987

Merged
os-elon merged 3 commits into
mainfrom
claude/issue-9688-cascade-required-multivalue-per-row
Aug 19, 2026
Merged

fix(engine): judge the required multi-value cascade escalation per row, not per field (#9688)#9987
os-elon merged 3 commits into
mainfrom
claude/issue-9688-cascade-required-multivalue-per-row

Conversation

@os-elon

Copy link
Copy Markdown
Collaborator

Fixes#9688

What changed

cascadeDeleteRelations escalated set_nullrestrict on fdef.required === truebefore the multiValued branch and beforedependents had been probed, so the refusal landed for every row referencing the deleted record, whatever else that row's set held.

The escalation's own rationale is what bounds it: it exists because clearing a required FK issues an UPDATE the child's validator rejects with a misleading "<field> is required" 400. On a multiple: true field the set_null limb does not clear the slot — since #9438 it removes the deleted member and writes the remainder — so that failure is reachable only for a row the removal would empty.

The judgement is now made per row, after the dependents probe and after the exact multi-value narrowing:

row state after member removaloutcome
remainder non-emptymember removal proceeds (#9438 semantics, accepted by the #9447 ruling)
remainder empty (deleted member was the last)DELETE_RESTRICTED / 409 stands — [] violates required under #9447, and is rejected by the record validator since #9476
both kinds presentthe whole delete is refused, and dependentCount counts only the rows that would be emptied

The single-valued half of the escalation is unchanged and still evaluated where it was: clearing a scalar FK always writes null, so its premise needs no row to hold.

remainderAfterMemberRemoval is a new private static shared by the per-row judgement and the set_null write — one function, two call sites, so the predicate that clears the write cannot predict a shape the write would not produce.

Why this is safe now — re-derived on origin/main, not recalled

The card was blocked because required-means-non-empty was declared but not enforced. Verified by reading the source (not a grep for a claim about it): packages/objectql/src/validation/record-validator.ts defines

functionisEmptyForRequired(def: FieldDef,value: unknown): boolean{if(isMissing(value))returntrue;returnisMultiValueField(def)&&Array.isArray(value)&&value.length===0;}

and both required read sites call it, so [] on a required multi-value field is refused on INSERT and UPDATE (#9476, merged as #9780). isMissing is untouched by this PR — its short-circuit is what keeps [] on a non-required multi-value field flowing to the array-shape branch, and the two predicates stay separate.

Verification

The "before" was reproduced first, on the unmodified tree: the #9625 fixture passed, with the engine's own log line showing the refusal it pins — Cannot delete acct (r_1): 1 dependent roster record(s) reference it via accounts (accounts is required, so it cannot be cleared), thrown from cascadeDeleteRelations, set left [acct_a, acct_b].

Reverse verification — the fix reverted to origin/main's engine.ts with the new tests in place, then restored (byte-identical to the committed fix, verified with git diff --quiet HEAD). No rebuild was needed and none would have helped: the suite imports the subject as ./engine.js, a relative in-package specifier vitest resolves to src/engine.ts, so no dist/ sits between the test and the change. Predicted direction was 3 red / 2 green, and that is what ran:

× [#9625→#9688] a required MULTI-VALUE lookup now REMOVES the member when the remainder stays non-empty
× [#9688] `dependentCount` counts ONLY the rows that would be emptied, and the whole delete is refused
× [#9688] a DEFAULTED set_null on a required multi-value lookup is judged the same way, in both directions
Tests 3 failed | 10 passed (13)
- "dependentCount": 1,
+ "dependentCount": 2,

The two that stayed green are the ones that should — the last-member refusal and the authored-restrict control pass under the old broad escalation too, which is exactly why they are controls rather than discriminators.

The #9625 pin was changed deliberately, not repaired.[#9625] refuses a required MULTI-VALUE lookup even when member removal would leave the set non-empty is now [#9625→#9688] a required MULTI-VALUE lookup now REMOVES the member when the remainder stays non-empty, carrying a comment that says the assertion is the inverse of what #9625 pinned and why the ruling inverted it. Four pins sit beside it:

  • last-member removal is still refused (⭐ the pin that makes the narrowing safe, asserted on the ADR-0112 envelope — codeandstatus — because an over-narrowed engine would fail it by throwing the child's own required 400 instead, which a bare toThrow() would accept);
  • dependentCount reports 1 of 2 referencing rows, and neither row is written — the refusal precedes every member-removal write;
  • a defaultedset_null reaches the same per-row judgement as the explicit one, pinned in both directions on one row;
  • an authoreddeleteBehavior: 'restrict' on a required multi-value field is not narrowed at all — the control that keeps the narrowing inside the escalation.

Gates. Union re-derived with node scripts/pm/dispatch-gates.mjs (no paths passed — the script derives its own change set) and run at adfc69dde, the head of this branch after merging origin/main. All 25 exit 0, each captured with a redirect before $?, never through a pipe:

check:changeset-gate-self-tests · check:cross-package-test-inputs · check:doc-anchors · check:docs-audit-scope · check:docs-redirects · check:durability-log-level · check:objectui-changeset · check:published-readme-links · check:role-word · check:slot-lookup · check:stack-collection-maps · check:query-options-erasure · check:engine-double-contract · check:where-matcher · check:type-check-coverage · check:nul-bytes · spec check:empty-state / check:liveness / check:strictness-ledger / check:variant-docs · check-adr-0087-registration · check-changeset-no-major · check-empty-changeset · check-engine-split-ratio · docs-audit/check-affected-docs

Their own verdict lines, quoted rather than inferred: check-engine-double-contract: OK — 321 pinned, 133 in the DEBT ledger, 2 exempt; where-matcher conformance holds: 259 matcher(s) discovered … 0 silently-wrong … none new; check-type-check-coverage: OK — 64/77 workspace packages type-checked; ✓ No empty-frontmatter changeset introduced by this diff (1 declaring changeset(s) added). No baseline moved in either direction — the tree carries no modified ledger artifact.

Package-scoped, at the same head: pnpm --filter @objectstack/objectql test221 files / 3899 tests passed; pnpm --filter @objectstack/objectql typecheck → clean (dependency closure built first with pnpm --filter '@objectstack/objectql^...' build).

Declared narrowing:check:type-check-debt --re-measure was not run locally — it demands a full workspace build, and objectql's tsconfig.json excludes **/*.test.ts with no test-typecheck-debt.json ledger entry for the package, so this PR's only test-file edit is outside every program it re-measures. CI runs it on the whole farm regardless.

Docs

Four passages stated the blanket refusal as fact and would have contradicted the engine the moment this merged — the exact defect class #9625 was filed for. Updated in the same PR: protocol/objectql/types.mdx (the "Required foreign keys" callout), api/data-api.mdx (the DELETE section), data-modeling/field-types.mdx (the deleteBehavior table row), deployment/troubleshooting.mdx (the "delete restricted" cause list).

Out of scope, filed instead

#9984content/docs/data-modeling/validation-rules.mdx still carries a warn callout saying the required-means-non-empty half is "declared but not yet enforced … validate emptiness in application code until that lands (tracked in #9476)". Every claim in it is false since #9476 landed, and it instructs authors to write redundant checks. It is the docs twin of #9781, which corrected the same stale premise in code comments. Not touched here — #9984 remains open and is not addressed by this PR.

The refusal's developerMessage for the last-member case still reads (<field> is required, so it cannot be cleared). Left verbatim: it is accurate enough for an emptied set, and rewording it for the multi-value case would mint a new operation-message key and its locale surface, which this card does not ask for.


Generated by Claude Code

@os-elonClaude

Copy link
Copy Markdown
CollaboratorAuthor

Hot-file region declaration — packages/objectql/src/engine.ts

Per the #9688 fence (concurrent-editor cap 3). Located by symbol, never by line number:

  • cascadeDeleteRelations — two regions inside it:
    • the required-FK escalation (behavior === 'set_null' && fdef.required === true), which becomes requiredSetNull + the single-valued-only escalation, and the const multiValued declaration that moves one statement up to sit above it;
    • the statement immediately after the dependents narrowing and the dependents.length === 0 skip, where the deferred per-row judgement is added, and the multiValued limb of the set_null write, which now calls the shared helper instead of inlining the filter.
  • remainderAfterMemberRemoval — a new private static, added directly above cascadeDeleteRelations's JSDoc, beside its sibling storedReferenceIncludes.
  • The method-level JSDoc of cascadeDeleteRelations.

Nothing else in the file is touched: no other method, no import, no field. find, update, delete, referenceProbeFilter and storedReferenceIncludes are read-only dependencies of the change.

Merged origin/main before opening this PR (merge commit at branch head, base 8fa4e2a76). Will merge again if a sibling lands on the file.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/objectql, touching 3 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, 60 pages)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

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 5ed8ee6808cc7551dc209a75a15f870bde352691packageMentionDocs.

Which tree this was computed on

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

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

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 5ed8ee6808cc7551dc209a75a15f870bde352691 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-elonClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT on substance; enqueue held on the load-bearing jobs

Reviewed against GitHub. ⛔ Not flipping ready until Lint & Repo Gates / TypeScript Type Check / Test Core report their own conclusions — the aggregate is not a verdict.

Divergence — including the hot file this card was warned about

Merge base 8fa4e2a76; main advanced 3 commits.

proberesult
⚠️packages/objectql/src/engine.ts touched on main since merge base0
all 7 PR files touched on main0 each
git merge-tree conflicts against current main0
positive control on a file main did move1

engine.ts is the file this lane has the region discipline for; it did not move under you.

The design decision that makes this safe

remainderAfterMemberRemoval as one function called by both the judgement and the write is the right call, and its docblock states the failure it prevents rather than just describing what it does:

a predicate computing the remainder differently could clear a delete and then have the write land the empty required set the judgement exists to refuse

That is the whole risk of this change in one sentence, and the shape removes it structurally instead of by two readings that happen to match today. It also reuses storedReferenceIncludes' String(v) !== String(id) reading, so the member removed is exactly the member that made the row a dependent — the narrowing and the removal cannot disagree about what a reference is.

Equally important and easy to miss: dependents = emptied runs before the restrict branch, so dependentCount, the localized count and the developerMessage all read the reduced set. Three numbers, one source. The test asserts both the structured field and the message text (1, not 2) — telling the operator and the developer different numbers is exactly how this kind of fix half-lands.

The dependentCount change is a real wire-visible delta, and it is disclosed

Counting only emptied rows is a behaviour change beyond the headline. It is stated in the changeset, in all four docs passages, and pinned by a dedicated test. It is also required for coherence — a refusal that names rows it no longer objects to is a second defect, which the card called out. Correct, and correctly surfaced rather than smuggled.

Verification

Reverse verification predicted 3 red / 2 green before running, and observed exactly that. Predicting direction in advance is the strongest form of the check — it cannot be rationalised after the fact. The 2 that stayed green are correctly explained as controls: the last-member refusal and the authored-restrict case both pass under the old broad escalation, which is precisely why they cannot detect the change and why they belong.

That reverse verification is also its own proof that no rebuild was needed. The report argues the suite reaches src/engine.ts through a relative in-package specifier; the ablation demonstrates it — reverting src without touching dist moved 3 tests to red. A claim about resolution paths backed by an observation rather than an assertion.

⭐ The authored-restrict control is the one I would have asked for if it were missing: "an implementation that judged emptiness for every multi-value field would sit green while quietly overriding an authored refusal." And the last-member test's note on why a bare toThrow() would be wrong — an over-narrowed engine would throw the child validator's required 400, a different code and status naming a field not on acct at all — is the ADR-0112 code+status discipline applied where it actually bites.

The #9625 fixture was updated deliberately with the reasoning in the test body, not repaired to green, and the old comment's now-dead clause about [] satisfying required was removed rather than left to rot. That is what that fixture was pinned for.

One note on the declared narrowing

The stated reason for skipping check:type-check-debt --re-measure covers only the test-file edit (objectql's tsconfig excludes **/*.test.ts). This PR also edits engine.ts, which is in that program — so the stated reasoning does not cover the whole diff. The conclusion still holds, by a different route: pnpm --filter @objectstack/objectql typecheck came back clean, so that entry cannot have drifted up. Worth stating precisely because a narrowing is only as good as the surface it names. CI runs the farm regardless.

Out-of-scope finding

#9984validation-rules.mdx still tells authors to "validate emptiness in application code until that lands (tracked in #9476)". False since #9476 landed, and it instructs redundant work. The docs twin of #9781, found by a card that had every reason to stop at code. Filed unassigned for triage.

Enqueuing on the jobs' own conclusions.


Generated by Claude Code

@os-elon
os-elon marked this pull request as ready for review August 19, 2026 12:23
@os-elon
os-elon added this pull request to the merge queueAug 19, 2026
Merged via the queue into main with commit b735507Aug 19, 2026
27 checks passed
@os-elon
os-elon deleted the claude/issue-9688-cascade-required-multivalue-per-row branch August 19, 2026 12:42
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 refuses a required multi-value lookup delete even when member removal would leave the set non-empty

2 participants

@os-elon@claude