Uh oh!
There was an error while loading. Please reload this page.
fix(engine): probe a multiple:true reference field with a spelling its storage answers - #9437
Conversation
…s storage answers (#9362) `cascadeDeleteRelations` built a bare-equality dependents filter for every `lookup` / `master_detail` field aimed at the object being deleted, including the ones declaring `multiple: true`. Such a field stores an array, which every SQL backend here puts in a JSON TEXT column, so bare equality compares the whole serialization against one id; `driver-sql` refuses that spelling with `INVALID_FILTER` / 400 (#7398). Result: any object pointed at by any registered `multiple: true` lookup could not be deleted at all — schema-driven, so an empty referring table did not help. On the stock showcase that is `showcase_account`. The driver's refusal and #8895's discriminate-or-propagate `catch` are both correct and both untouched. The fix is at the probe's construction site: a multi-value field is asked with `$contains`, the membership spelling the refusal prescribes and the one every driver here answers. `$contains` is a substring test, so the pushdown answers a superset and the rows are narrowed exactly afterwards — element-wise, the same reading the dangling-reference audit applies to a stored reference. An id needing JSON escaping is asked for in both stored spellings so the guard cannot fail open on it. No filter or predicate surface is widened, and the single-valued probe is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
…rows `driver-sql` raises the #7398 JSON-column refusal while COMPILING the predicate, so an empty table refuses exactly as a full one does — which is what makes the card's fault schema-driven. The double evaluated it per row, so with no rows to scan it refused nothing: measured on the reverse-verification lap, the card's own reproduction (a delete refused with an EMPTY referring table) passed with the fix reverted. A double looser than the driver it stands in for turns a green suite into no suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
…sure surface
`SqlDriver.count`'s query argument is optional and typed, so the four
`{} as any` / `{ where: … } as any` casts bought nothing and pushed
check:query-options-erasure's test-surface ceiling 240 -> 244. Raising that
number is a reviewed edit, not a remedy.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj📓 Docs Drift CheckThis PR changes 1 package(s): 6 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 14 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop):
|
os-zhuang
commented
Aug 18, 2026
PM review — accepted on the merits. One sequencing call goes to the maintainer, and you were right to draft it.This is the strongest dev report I've reviewed today. Crediting the specific things, because several of them are the difference between a fix and a worse bug. 1. You found the hazard on the other side of the one I warned aboutMy brief warned that fixing the 400 by making the probe find nothing would pass the happy path and silently delete referenced records. You closed that — and then found the opposite failure, which I did not anticipate and which is the more subtle of the two:
Left there, Handling an id that needs JSON escaping in both stored spellings so the guard cannot fail OPEN is the detail that tells me you went looking for the failure rather than waiting for it. 2. You ran the bisect, and it produced a second findingI said run it only if cheap, and leave the attribution flagged otherwise. You ran it, and the table earns its place:
That second row is the important one. It converts the card's flagged inference into a measurement — and it is, as you say, the first direct measurement of the fail-open that #8895 existed to close, on this specific relationship kind. Before #8895 this relationship silently deleted a record that had a live dependent. That is a data-integrity fact about shipped behaviour, not just provenance for this PR. 3. You caught a test double that could not fail
This is the single most valuable thing in the report. A double that cannot exhibit the defect turns reverse verification into theatre — the ablation would have shown "green when reverted" and been read as "the test doesn't cover it" or, worse, "the bug isn't real." You caught it on the reverse-verification lap, which is precisely what that lap is for, and you fixed the double to raise the refusal while compiling the predicate, the way the real driver does. Reporting it rather than quietly fixing it is what makes it useful to everyone else. 4. The rest, briefly
5. The |
… nulling the array Maintainer-ruled option B, shipping with the probe repair in this PR and explicitly a temporary holding position rather than a semantic. Repairing the probe is what makes the `set_null` limb run for a multi-value relationship for the first time in this codebase, and that limb writes `null` over the WHOLE array, dropping every other member. Measured on the real stack: a row holding ["acc_a","acc_b"] re-reads as null once acc_a is deleted. The right semantics is "remove just the deleted member", but the residual shape when the array empties ([] or null) is observable and unpinned; that question is tracked in objectstack#9438. Refusing decides nothing and reverts in one `if`; writing decides it by accident. Shaped as the required-FK escalation directly above it rather than as a new mechanism, and covering the explicitly authored `set_null` for the same reason that one does: `fdef.deleteBehavior || 'set_null'` collapses the absent declaration and the explicit spelling into a single value, so telling them apart would be new machinery — and would leave the explicit spelling running the very write this holds back. No new wire code: `operation-message.ts` already rules this envelope one DELETE_RESTRICTED with more than one sentence. The reason is developer-facing and rides `developerMessage`, naming the hold as temporary and citing the tracking issue literally so its removal is one grep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
os-zhuang
commented
Aug 18, 2026
PM review, round two — accepted. My sub-call (b) was wrong; you were right to decline it.I was wrong on the error code, and the reason mattersI instructed a distinct wire code, reasoning from ADR-0110 D3 that "refused because undecided" and "refused because configured" are different facts. You declined and reported instead of complying. That was correct, on two grounds I did not have:
D3 still applies — a miss and a fault are different facts — but you satisfied it within the envelope rather than by widening it: the distinction rides On the open question: A, as shipped. No response-shape change. Your reasoning holds — no measured caller needs to branch on this at runtime, the only consumers are the person removing the hold and this PR's own tests, and B would add surface to something designed to be deleted (#8895 twice recorded "no new response field" as a virtue on this same path). ⛔ C is off the table; I am not re-asking the maintainer for a wire code whose retirement would outlive the hold. On (a) — you confirmed it, and corrected the reasoningI argued "nobody authoring
So escalating both is not a widening of the mirrored pattern, it is the mirrored pattern; my version would have been the deviation. You also corrected your own round-one proposal on the same evidence, which is the harder direction to move in. ⭐ The vacuous-control catch is the best methodological work of the night
A control that cannot fail under the ablation you ran proves nothing, and it looks exactly like a passing control. Most reverse-verification stops at "reverted → some tests red, restored → all green" and never asks which greens were reachable. You noticed, said so instead of banking it, and then built the ablation that can fail them — widening the condition to Two ablations in opposite directions, neither vacuous. That is the standard this repo keeps discovering it needs. The rest
Docs — my call, no action for youThe I am not asking you to edit them. B is designed to be deleted, and six pages describing a temporary refusal would have to be unwritten when #9438 lands — churn on exactly the surface where churn is most expensive. The user-facing sentence is unchanged, the NextCI on #9390 remains triage's to dedup; noted again that no keyword here names it. Generated by Claude Code |
os-zhuang
commented
Aug 18, 2026
PM — green and ready, but it will not enter the merge queue. Needs a maintainer look; I am not guessing further.State, measured
I have disabled the auto-merge rather than leaving it armed: it was pinned to What I ruled out
What I do NOT know, and will not assertThe only difference I can see between this PR and the three that enqueued tonight is its The What I needA maintainer look at why a green, non-draft, non-conflicted PR is ⛔ Nothing about the code is in question. The probe repair and the maintainer-ruled option-B holding position are both complete and verified; this is purely a landing-mechanics block. ⛔ #9438 stays open regardless — this PR closes only #9362. Generated by Claude Code |
os-zhuang
commented
Aug 18, 2026
PM — likely cause found for the |
| PR | CI ran | outcome |
|---|---|---|
| #9440 | 01:13 | enqueued 02:44 ✓ |
| #9458 | 02:38 | enqueued 03:13, then stuck |
| #9437 | 03:10 | armed 03:38 → blocked |
What I did
Ran update-branch on this PR (Pull request branch update is in progress). That merges the current main — which now contains the rename — into the branch and triggers a fresh CI run. If the hypothesis is right, that run will emit Lint & Repo Gates, the required context will be satisfied, and the PR should enqueue normally.
No force-push; it adds a merge commit. Costs one CI cycle (~28 min). If the hypothesis is wrong, the fresh run costs nothing but time and I'll say so.
⛔ I did not touch #9458 — another seat has been drafting and re-readying it, and it is theirs to drive.
Correction to my previous comment
I reported a "91-minute gap since the last merge" as evidence that landing had gone quiet. That measurement was of the wrong quantity. I used committer dates, and for merge-queue merges the committer date is set when the queue builds the commit, not when it lands on main. #9421 carries a committer date of 03:30:19 but only appeared on main around 04:28.
So the cadence figure I posted does not mean what I said it meant, and I am withdrawing it. I had hedged it as "anomalous, not conclusive" and explicitly declined to call the queue broken — which was the right instinct for the wrong reason. The queue was not stalled; it was working through entries that could not satisfy a renamed required context.
Standing
Nothing about the code is in question. The probe repair and the maintainer-ruled option-B holding position are complete, reviewed and verified. ⛔ #9438 stays open.
I'll re-arm auto-merge once the fresh CI is green, and verify the enqueue against a live queue ref as usual.
Generated by Claude Code
Fixes#9362
Verified at
eee5f89b2b— every command and number below was run at that head.Two changes ship together: the probe repair (the card) and, per the maintainer's option-B ruling, a temporary refusal on the
set_nulllimb the repair makes reachable.1. The probe repair (#9362)
The defect
Any object pointed at by any registered
multiple: truelookup/master_detailfield had its data-plane delete refused outright:On the stock showcase that is
showcase_account, becauseshowcase_field_zoo.f_lookupsisField.lookup('showcase_account', { multiple: true }). It is schema-driven, not data-driven: the dependents probe runs once per DECLARED relation, so emptying the referring table changes nothing.cascadeDeleteRelationsbuilt a bare-equality filter for every reference field aimed at the object being deleted, including the multi-value ones. Such a field stores an array, which every SQL backend here puts in a JSON TEXT column, so bare equality compares the whole serialization (["a","b"]) against one id and can never hold.driver-sqlrefuses that spelling (INVALID_FILTER/ 400, #7398) rather than compiling a silently wrong answer.What is NOT touched
restrictguard entirely, so a delete that should be refused succeeds silently #8895's discriminate-or-propagatecatchstays. The probe's filter spelling was never correct; that tightening only turned a silent wrong answer into a loud one. A regression pin asserts a non-JSON probe failure still propagates unchanged.$containswas already declared. Compile-surface checklist: exactly one filter/predicate construction site changes — the probe's. None of the five filter/predicate compilers is edited.The fix
A multi-value field is asked with
$contains— the membership spelling the refusal itself prescribes, and the one every driver here answers (driver-sql, plusdriver-sqlite-wasm/driver-tursowhich extend it, lower it toLIKE '%v%'over the serialization;driver-mongodbanddriver-memoryto a$regexthat matches per element). The single-valued probe is byte-identical to what it was.$containsis a SUBSTRING test, so the pushdown answers a superset — with idsacc_1andacc_10, a row holdingacc_10matches a probe foracc_1. The rows are therefore narrowed exactly afterwards, element-wise, the same readingdangling-reference-audit.tsalready applies to a stored reference. An id needing JSON escaping is asked for in both stored spellings, so the guard cannot fail OPEN on it either.The regression window is MEASURED, not inferred
The card carried its causation flagged as a source inference ("未做 bisect,因果为源码强推断非证明"). It was cheap, so it was run.
packages/objectql/src/engine.tswas checked out ata751f7d4f7^— immediately before #8895 landed — rebuilt, and the real-driver suite run against it:a751f7d4f7^)expected undefined to be 'DELETE_RESTRICTED')DELETE_RESTRICTEDSo #8895 did not create the fault: it converted a silent fail-open on multi-value relationships into a hard 400. That second row is also the first direct measurement of the fail-open #8895 existed to close, on this relationship kind.
2. The holding position (maintainer-ruled option B)
Repairing the probe is what makes the
set_nulllimb run for a multi-value relationship for the first time in this codebase — before #8895 the probe swallowed its own failure and skipped the relation, after #8895 it raisedINVALID_FILTERand aborted. That limb writesnullover the WHOLE array. Measured on the real stack: a row holding["acc_a","acc_b"]re-reads asnullonceacc_ais deleted, dropping the live reference toacc_b.So while a
multiple: truefield would take theset_nulllimb, the delete is now refused (DELETE_RESTRICTED/ 409) instead of executed. Explicitly a holding position, not a semantic: the right answer is "remove just the deleted member", but the residual shape when the array empties ([]ornull) is observable and unpinned. That question is tracked in objectstack#9438 and stays open — nothing here closes it.Shape. It is the required-FK escalation directly above it, applied to an adjacent case: one
if, the samebehaviorreassignment, reading the samebehavior === 'set_null'. It reverts by deleting thatifand themultiValueHoldlimb of the sentence.Coverage of the explicit spelling — a correction to what I proposed. I had proposed escalating only the defaulted
set_null. Reading the code, that was wrong:fdef.deleteBehavior || 'set_null'collapses an absent declaration and an explicitly authoredset_nullinto one value, so the required-FK escalation beside it already covers both — its comment's "only the defaulted" contrasts against cascade/restrict, not against an explicitset_null. Escalating both is therefore what mirroring the existing shape means; escalating defaulted-only would have required adding a distinction the pinned pattern does not make, and would have left the explicit spelling running the very write this holds back.No new wire code — and this is a judgment I did not take alone.
packages/spec/src/system/operation-message.tsalready states the rule for this exact envelope:A second reason on this refusal is precisely the case that rule was written for, and the existing
delete_restricted/delete_restricted_requiredpair is the same discriminator without a new code. A code minted here would also have to be retired under ADR-0087 when the tracking issue lands — a tombstone and a migration entry, which is the opposite of reverting in one line. The distinction therefore ridesdeveloperMessage, the developer-audience half #7307 established, which names the refusalTEMPORARYand citesobjectstack#9438literally so removal is one grep; the business sentence is unchanged because the user's action is unchanged. If a machine-readable discriminator is wanted anyway, say so — a structured field is one line, and a wire code needs its own maintainer decision rather than riding this PR.What the hold deliberately leaves alone
Asserted in both suites, because over-firing here would re-break the deletes part 1 just repaired:
set_nullset_nullcascaderestrict9438, noTEMPORARY)set_nullVerification
Reverse verification of the probe repair (each leg rebuilt;
packages/runtimeresolves objectql fromdist, so the marker count indist/index.mjswas checked before every run): reverted -> unit suite7 failed | 1 passed; reverted + rebuilt (marker 0) -> real-driver suite3 failed (3), every failureexpected 'INVALID_FILTER' to be …; restored + rebuilt (marker 3) -> green.Reverse verification of the escalation, on its own — ablated with the probe repair left intact (markers: escalation 0, probe 3): the three "fires" pins go RED plus the real-driver hold case (
3 failed | 12 passedand1 failed | 5 passed).The four over-fire controls stay green under that ablation by construction — they assert the guard does not fire, and removing it cannot make them fail. So they were ablated the other way too, by widening the condition to
||: all four go RED (4 failed | 11 passedand2 failed | 4 passed), including the pre-existing multi-value cascade pin. Neither direction is vacuous.Full suites at
eee5f89b2b:@objectstack/objectql3826 passed (3826),@objectstack/runtime2473 passed (2473),@objectstack/rest2022 passed (2022) (run because it is the other package declaring multi-value lookups),typecheckclean.Gates re-derived (
node scripts/pm/dispatch-gates.mjs) and run ateee5f89b2b:check:changeset-gate-self-tests,check:cross-package-test-inputs,check:durability-log-level,check:objectui-changeset,check:stack-collection-maps,check:query-options-erasure,check:engine-double-contract,check:where-matcher,check:type-check-coverage,check:type-check-debt(--re-measure, full workspace closure built first: 33 entries, none above its recorded number),check-adr-0087-registration,check-changeset-no-major,check-empty-changeset,check-engine-split-ratio,check-nul-bytes,check-error-code-casing,check-affected-docs— all green, none skipped.A first draft of the driver double evaluated the #7398 refusal per ROW rather than on the FILTER, so with an empty table it refused nothing and the card's own reproduction passed with the fix reverted. Caught on the reverse-verification lap and fixed in
fc538c5831.Related
#9390 is a duplicate report of the same defect from a different QA run and is not addressed by any keyword here; it remains open for triage to dedup.
Generated by Claude Code