Skip to content

fix(objectql): retire sys_fetch_previous_delete so the delete-path per-object gate is honest (#5929) - #6794

Merged
os-zhuang merged 4 commits into
mainfrom
claude/issue-5929-retire-delete-prev-builtin
Aug 8, 2026
Merged

fix(objectql): retire sys_fetch_previous_delete so the delete-path per-object gate is honest (#5929)#6794
os-zhuang merged 4 commits into
mainfrom
claude/issue-5929-retire-delete-prev-builtin

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #5929

The premise, verified before anything was removed

The card was filed 08-06 and both files it names were rewritten on 08-08 by #6697, so every claim was re-anchored on current main rather than taken from the issue text.

Premise as filed: delete()'s per-object prior-row gate is constant true on any kernel-managed engine, because ObjectQLPlugin's sys_fetch_previous_delete registers on beforeDelete with object: '*'. CONFIRMED.

Premise as upgraded by #6697's measurement: the builtin is circular — the engine binds previous before beforeDelete dispatches, so the builtin's !ctx.previous guard is permanently false and its only remaining effect is holding open the gate that makes it redundant. CONFIRMED, and re-measured rather than trusted (see A1 below).

The measurement #6697 left behind, cited as asked — packages/objectql/src/plugin.ts (pre-change lines 920–939), inside the sys_fetch_previous_update retirement block:

delete() reads its pre-image when wantsPreImage is true, and that gate is hasHooksFor('beforeDelete', object) || hasHooksFor('afterDelete', object) || summaries. The builtin is itself a beforeDelete hook on '*', so it makes the FIRST term true for every object — and then the engine's read binds previous before the builtin runs, so the builtin's own !ctx.previous guard is false and it issues no findOne. It is circular: the builtin's only remaining effect is to hold open the gate that makes it redundant.

Other consumers of the retired name — checked first, as instructed. Grep for the quoted exact name across the repo (declarations, registrations, docs, JSON) returned exactly three hits: the declaration and the plugin.ts comment above it, plus one doc comment in packages/plugins/plugin-auth/src/last-admin-guard.ts which names the builtin as a co-producer of ctx.previous and states in the same sentence that the guard "still never consumes it". No live consumer. That comment is corrected here rather than left describing a hook that no longer exists.

The work

1. Retired sys_fetch_previous_delete (packages/objectql/src/plugin.ts) under ADR-0049 enforce-or-remove, replaced by a ⛔ RETIRED block in the shape #5846 used for its update-side twin: the argument, the residual shape, and — new — an explicit statement of what the retirement does not buy, so the next reader does not over-read the result.

Also corrected in the same file: the binder's debug line still advertised previousData as something these builtins produce. With both fetch-previous builtins retired, it named a producer that no longer exists.

2. The delete-path per-object gate. Enumerated fresh from current main (engine.ts, by-id branch). It has three terms:

# Term Touched?
1 hasHooksFor('beforeDelete', object) no
2 hasHooksFor('afterDelete', object) no
3 getSummaryDescriptors(object).length > 0 no

There is no summary/validation term beyond #3, and needsPriorRecord(schema) is deliberately absent (its own comment says why: delete() evaluates no validation rules, so the term would buy a read with no reader). The predicate branch carries its own twin gate, terms 1 and 2 only, likewise untouched.

I changed no term. The gate was already written honestly; what was dishonest was its first term's answer, and the '*' registration was the entire cause. Retiring the builtin is the whole fix. What engine.ts gains is the A2 enumeration recorded beside wantsPreImage, so the honest gate is not mistaken for a usually-false one.

3. Acceptance pins — new packages/objectql/src/engine-delete-prior-read-scope.test.ts (16 cases), the delete-side counterpart of engine-update-prior-read-scope.test.ts:

4. The by-id delete REPOINT is untouched (#6752 pending). bulk-write-per-row-hooks.test.ts's still HONOURS a by-id beforeDelete REPOINT — deliberately not retired here is green. Nothing in this diff changes when the repoint's re-read happens: the re-read is guarded by the same wantsPreImage value computed once before the before phase, and that value's computation is unchanged — only what one of its inputs answers on a kernel.

A1 — "retiring the builtin changes no observable binding anywhere"

ANSWERED: true. Reverse-verified in the direction the card asked for.

Predicted: with the builtin removed and no gate change, the full @objectstack/objectql suite stays green, with no failure at all — grep having already established that no test names the builtin as a live registration, only as prose.

Actual: Test Files 149 passed (149) · Tests 2575 passed (2575). Zero failures. The premise has no hole.

A2 — "does another '*' delete-phase builtin silently re-open the gate?"

ANSWERED: no '*'-spelled one remains — but four packages register delete-phase hooks with object absent, which is the same thing by another spelling. Enumerated exhaustively; every delete-phase registration in non-test source was read and classified:

Package / file Events Engine-visible scope Holds the gate open?
objectql plugin.ts beforeDelete object: '*' retired by this PR
plugin-auth identity-write-guard.ts beforeDelete no object ⇒ global yes — filters isManaged(ctx.object) inside the handler
plugin-sharing record-share-cascade.ts before+afterDelete no object ⇒ global yes — filters targets(objectName) inside the handler
service-storage file-reference-lifecycle.ts before+afterDelete no object ⇒ global yes — filters activeFileFields(object) inside the handler
plugin-audit audit-writers.ts before+afterDelete global minus excludeObjects: AUDIT_EXCLUDED_OBJECTS (#5860) yes, except on excluded objects — which really do skip the read
plugin-auth last-admin-guard.ts beforeDelete ×4 object-scoped no
plugin-sharing rule-hooks.ts, primary-bu-projection.ts, sharing-plugin.ts delete phases object-scoped no
service-storage attachment-lifecycle.ts, attachment-access-hooks.ts delete phases object: 'sys_attachment' no
plugin-audit comment-access-hooks.ts beforeDelete object: 'sys_comment' no

The four global ones are real consumers with real handlers — they merely decide applicability at dispatch time rather than at registration time. The gate answering "yes" for them is the gate working, not a second instance of #5929: what made the builtin a defect was that it consumed nothing. Narrowing any of them to the objects it actually serves is that package's own card; plugin-audit's excludeObjects face is the worked example of how, and this PR pins that the engine half of it works on both delete phases. This is stated in engine.ts and in the changeset so nobody expects a skip on a full kernel that will not appear.

Reverse verification of the new pins

A pin that cannot fail is not a pin. Predicted-then-actual, with the builtin temporarily restored from origin/main and the new file re-run:

Predicted: exactly two cases red — registers NO beforeDelete hook of its own (hasHooksFor true) and a single-id delete on a hook-free object performs NO prior-row read (delta 1, not 0). The previous still bound from ONE read case stays green, because the builtin's guard short-circuits and it adds no read. Every bare-engine case stays green (a bare engine never carried the builtin).

Actual: Tests 2 failed | 14 passed (16)registers NO beforeDelete hook of its own: expected true to be false; a single-id delete on a hook-free object performs NO prior-row read: expected 1 to be +0. Nothing else moved.

That second run is also the cleanest independent confirmation of the circularity claim: with the builtin present, the hooked-object case still measured exactly one read — so the builtin issued none, exactly as #6697 recorded.

Two harness bugs were found and fixed by this exercise rather than papered over: the counting driver's deleteMany/updateMany/count routed through their own find(), charging the engine for reads it never issued; and bindHooksToEngine refuses an absent object (#4001), so the excludeObjects face had to be registered through engine.registerHook — the first version's "excluded object skips the read" passed for the wrong reason, because the hook had never registered at all.

Scope

packages/objectql/src/plugin.ts (the retirement) · packages/objectql/src/engine.ts (delete-gate region comment only — no term changed) · one stale doc comment in plugin-auth/src/last-admin-guard.ts · the new test file · .changeset/. No spec changes, no plugin-audit edits, nothing in the registerObject/registry region (#5543) or resolveMasterDetailParent(s) (#6457). origin/main merged immediately before opening this PR.


Generated by Claude Code

claude added 2 commits August 8, 2026 17:46
…per-object gate is honest (#5929)

`delete()`'s prior-row read is gated per object on
`hasHooksFor('beforeDelete', object) || hasHooksFor('afterDelete', object)
|| getSummaryDescriptors(object).length > 0`. On any kernel-hosted engine the
first term was constant true, because `ObjectQLPlugin` registered its own
`sys_fetch_previous_delete` builtin with `object: '*'` — so the per-object skip
the gate exists to perform never happened outside the bare engines unit tests
boot.

The builtin could not use what it held open. Since #5272 (by-id) and #6697
(predicate path, per matched row) the engine reads the pre-image and binds
`previous` before `beforeDelete` dispatches, so its `!ctx.previous` guard was
permanently false and it issued no read. Its only remaining effect was holding
open the gate that made it redundant. Retired under ADR-0049 enforce-or-remove;
the measurement #5846 recorded in `plugin.ts` was re-verified on this branch
rather than taken on trust.

The gate's three terms are unchanged — no term was added or removed. What
changed is that term 1 now reflects real hooks. `engine.ts` gains the
enumeration of the delete-phase hooks that still register globally
(plugin-auth, plugin-sharing, service-storage; plugin-audit narrows at the
engine face with `excludeObjects`), so nobody reads a skip into a trace that
will not show one.

New `engine-delete-prior-read-scope.test.ts` pins the three terms per object,
the `excludeObjects` subtraction on both phases, the predicate path's twin
gate, and — on a real `ObjectKernel` + `ObjectQLPlugin`, the only configuration
where the defect was observable — the zero-read skip and the still-bound
`previous`. It replays the retired builtin's own shape and measures its guard
short-circuiting, so "the guard can no longer be true" stays a measurement.

The by-id `beforeDelete` REPOINT behaviour is deliberately untouched (#6752).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UNT8SWDEsDQp2TrmSBizKq
@vercel

vercel Bot commented Aug 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 8, 2026 6:23pm

Request Review

@github-actions github-actions Bot added the size/l label Aug 8, 2026
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/objectql, @objectstack/plugin-auth.

19 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx (via @objectstack/objectql)
  • content/docs/data-modeling/formulas.mdx (via packages/objectql)
  • content/docs/deployment/cli.mdx (via @objectstack/plugin-auth)
  • content/docs/deployment/migration-from-objectql.mdx (via @objectstack/objectql)
  • content/docs/deployment/production-readiness.mdx (via @objectstack/plugin-auth)
  • content/docs/deployment/vercel.mdx (via @objectstack/objectql)
  • content/docs/kernel/contracts/cache-service.mdx (via @objectstack/plugin-auth)
  • content/docs/kernel/runtime-services/examples.mdx (via packages/objectql)
  • content/docs/kernel/services-checklist.mdx (via @objectstack/objectql, @objectstack/plugin-auth)
  • content/docs/kernel/services.mdx (via @objectstack/objectql)
  • content/docs/permissions/authentication.mdx (via @objectstack/objectql, @objectstack/plugin-auth)
  • content/docs/permissions/sso.mdx (via @objectstack/plugin-auth)
  • content/docs/plugins/index.mdx (via @objectstack/objectql, @objectstack/plugin-auth)
  • content/docs/plugins/packages.mdx (via @objectstack/objectql, @objectstack/plugin-auth)
  • content/docs/protocol/kernel/index.mdx (via @objectstack/objectql)
  • content/docs/protocol/objectql/query-syntax.mdx (via packages/objectql)
  • content/docs/protocol/objectql/state-machine.mdx (via @objectstack/objectql)
  • content/docs/releases/implementation-status.mdx (via @objectstack/objectql, @objectstack/plugin-auth)
  • content/docs/releases/v9.mdx (via @objectstack/plugin-auth)

Advisory only. 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-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Aug 8, 2026
claude added 2 commits August 8, 2026 18:23
…hets (#5929)

Two lint.yml gates caught the new test file on the first full run:

* `check:slot-lookup` / `no-restricted-syntax` — `kernel.getService('objectql')
  as any` erased the slot's contract for every `engine.*` call in the kernel
  section, which is where the measurement lives. Typed as
  `getService<ObjectQL>('objectql')`.
* `check:query-options-erasure` — three `count(obj, {} as any)` calls grew the
  test-surface count 263 → 266. The empty options bag is already on contract;
  the assertion was never needed.

No assertion changed; the file still passes 16/16.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UNT8SWDEsDQp2TrmSBizKq
@os-zhuang
os-zhuang marked this pull request as ready for review August 8, 2026 19:21
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 8, 2026
Merged via the queue into main with commit 4fedb11 Aug 8, 2026
25 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-5929-retire-delete-prev-builtin branch August 8, 2026 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants