diff --git a/.changeset/retire-delete-by-id-before-hook-repoint.md b/.changeset/retire-delete-by-id-before-hook-repoint.md new file mode 100644 index 0000000000..63e18de3f7 --- /dev/null +++ b/.changeset/retire-delete-by-id-before-hook-repoint.md @@ -0,0 +1,80 @@ +--- +"@objectstack/objectql": major +"@objectstack/spec": patch +--- + + + +feat(objectql)!: retire `delete()`'s by-id `beforeDelete` REPOINT, aligning it with `update()` (#6752) + +A `beforeDelete` handler on a **by-id** `delete()` may no longer move the +delete onto a different row by assigning `ctx.input.id`. The rebind is +**refused** with `HookTargetRebindError` / `ERR_HOOK_TARGET_REBIND` +(`path: 'by-id'`) — exactly what the `update()` twin and both per-row paths +already raised. Nothing is deleted, and `afterDelete` and the roll-up +recompute never run. + +**The rule is now one line, on both verbs: a by-id target is immutable in a +`before*` handler.** + +| | CLEARED id | REBOUND to another id | +| ------------------ | -------------- | ------------------------- | +| `update()` by-id | refused | refused | +| `delete()` by-id | refused | **refused** (was honoured)| +| either, per-row | refused (D4) | refused (D4) | + +**Removed keys and their prescriptions (FROM → TO):** + +| Wrote | Write instead | +| --- | --- | +| `beforeDelete` handler: `ctx.input.id = otherId` | `await ctx.ql.delete(object, otherId)` for that row explicitly, and let the addressed delete proceed — or `throw` from the handler to stop it | +| `beforeDelete` handler repointing to delete a set | have the **caller** pass `{ multi: true, where: … }` | + +Writing the **same** id back is unaffected and stays legal: the check is +`input.id !== id`, the `update()` check verbatim, so a handler that reads the +id or assigns it to itself is not caught. + +**This removes a capability that WORKED, and the reasoning has to be read that +way.** `delete()` had a re-resolution for a repointed target since #5272: it +re-read the new target's pre-image and rebound `previous`, so `afterDelete` and +the summary recompute saw the row actually deleted. Nothing stale ever leaked, +and the case that retires a rebind on `update()` — the write landing on a row +whose pre-image, `readonlyWhen` locks and validation rules were never evaluated +— did not apply to it. That is why #5574's engine half (PR #6697) deliberately +left the asymmetry standing and filed it as its own question. + +The 2026-08-09 maintainer ruling on #6752 retires it anyway, on three measured +axes: + +- **Compatibility cost, measured: zero.** A repository-wide grep for assignments + into a hook's `input.id`, re-run on this PR's base rather than inherited, + finds six sites and all six are this family's own pins. No consumer anywhere + repoints — not in the framework, plugins, examples or docs. +- **One rule beats two correct rules.** Two verbs answering the same slot + differently is something every hook author must hold in memory, and the + justification for the split lived in an ADR, not at the call site. +- **The surface is a footgun independent of the mechanism.** "A hook silently + redirects which row gets deleted" is a top-grade hazard for authored — and + especially AI-authored — handlers. Correctness of a mechanism does not justify + the surface it exposes. + +Aligning the other way — building `update()` the same re-resolution — stays +excluded by #5574's recorded ruling ("do not silently pick re-resolution +instead"). + +The re-read block in `delete()`'s by-id branch is **deleted, not bypassed**: its +guard was `input.id !== id && input.id`, precisely the case the refusal now +throws on, so it became unreachable the moment the refusal landed. The single +pre-dispatch pre-image read that binds `previous` for `beforeDelete` is a +different read and is untouched. + +Recorded as **ADR-0058 Amendment II.2**; the `hook-target-rebind-errors.ts` +"what this does NOT cover" section is gone, because there is no longer an +exception to remember. The #5272 pin asserting the repoint was honoured is +**flipped to assert the refusal**, not deleted, with a new negative control +pinning that a same-id rewrite stays legal. + +Supersedes the scope note in the pending `bulk-write-before-hooks-per-row` +changeset ("a `beforeDelete` handler that repoints the target is unaffected"), +which described PR #6697's deliberate carve-out and is closed by this change in +the same release. diff --git a/docs/adr/0058-expression-and-predicate-surface.md b/docs/adr/0058-expression-and-predicate-surface.md index 7460847f25..60c54e1013 100644 --- a/docs/adr/0058-expression-and-predicate-surface.md +++ b/docs/adr/0058-expression-and-predicate-surface.md @@ -332,12 +332,13 @@ > NAMES the retired capability, so an author whose handler stopped working > learns what changed instead of watching a write land somewhere unexpected. > -> **Scope, stated precisely, because the two verbs do NOT answer alike.** +> **Scope, stated precisely. Both verbs now answer alike** — see Amendment II.2, +> which closed the one cell this amendment left open. > > | | CLEARED id | REBOUND to another id | > |---|---|---| > | `update()` by-id | refused | refused | -> | `delete()` by-id | refused | **honoured** (#5272's re-read, unchanged) | +> | `delete()` by-id | refused | refused (#6752 — was **honoured** as delivered here) | > | either, per-row | refused (D4) | refused (D4) | > > The CLEARED column is uniform because the ladder reorder leaves it no answer @@ -345,17 +346,18 @@ > that branch is chosen before any handler runs. That is the capability this > amendment retires, and it is the one the ruling names. > -> The REBOUND column is not uniform, and the asymmetry is principled rather -> than an oversight. The case against honouring a rebind is that the write would -> land on a row whose pre-image, `readonlyWhen` locks and validation rules were -> never evaluated — and on `delete()` that is simply not true: #5272 already -> RE-RESOLVES the new target, re-reading its pre-image and rebinding `previous` -> before `afterDelete` or the summary recompute can see it. `update()` has no -> such mechanism and would have to grow one, which is the "silently pick -> re-resolution instead" this ruling forbids. So `update()` refuses and -> `delete()` keeps honouring, until the delete-side repoint is ruled on as its -> own question (#6752) — deliberately NOT folded in here as a rider on an -> ordering change. +> The REBOUND column was **not** uniform as this amendment shipped, and the +> asymmetry was principled rather than an oversight. The case against honouring +> a rebind is that the write would land on a row whose pre-image, `readonlyWhen` +> locks and validation rules were never evaluated — and on `delete()` that was +> simply not true: #5272 RE-RESOLVED the new target, re-reading its pre-image +> and rebinding `previous` before `afterDelete` or the summary recompute could +> see it. `update()` has no such mechanism and would have to grow one, which is +> the "silently pick re-resolution instead" this ruling forbids. So `update()` +> refused and `delete()` kept honouring, with the delete-side repoint carved out +> to be ruled on as its own question (#6752) — deliberately NOT folded in here +> as a rider on an ordering change. **→ Ruled on, and retired, in Amendment II.2 +> below.** > > Premise for the retirement, checked against `origin/main`: the only > `ctx.input.id` assignment in the whole repository was one engine test forcing @@ -375,6 +377,68 @@ --- +> **Amendment II.2 (2026-08, #6752 maintainer ruling) — `delete()`'s by-id +> REPOINT is RETIRED too. The REBOUND column is now uniform.** +> _Settles the one cell Amendment II.1 carved out by name. The rule across both +> verbs is now sayable in one line: **a by-id target is immutable in a `before*` +> handler.**_ +> +> **What changes.** A `beforeDelete` handler that assigns `ctx.input.id` a +> DIFFERENT id no longer moves the delete to that row. It is refused with the +> same `HookTargetRebindError` / `ERR_HOOK_TARGET_REBIND` the `update()` twin +> and both per-row paths (D4) already raise, with `path: 'by-id'` and +> `expectedId` / `observedId` naming both ends of the move. Nothing is deleted; +> `afterDelete` and the summary recompute never run. Writing back the SAME id is +> untouched and still legal — the refusal is `input.id !== id`, the `update()` +> check verbatim, so a handler that reads the id or assigns it to itself is not +> caught. +> +> **This is a behaviour REMOVAL of a mechanism that worked, and the reasoning +> has to be read that way.** #5272's re-resolution was internally correct: it +> re-read the new target's pre-image and rebound `previous`, so nothing stale +> ever reached `afterDelete` or the roll-up. No defect was found in it, and the +> second bullet of the three-options paragraph above genuinely did not apply to +> it. It is retired on three measured axes instead: +> +> - **Compatibility cost, measured: zero.** A repository-wide grep for +> assignments into a hook's `input.id` — re-run against the base of the +> implementing PR, not inherited from the card — finds six sites, all of them +> tests in `packages/objectql/src/`: four are this family's OWN pins (the +> per-row rebind refusal, both by-id CLEARED refusals, the same-id negative +> control), one is the repoint pin being flipped here, and the sixth +> (`engine.test.ts`) clears the id to force the #2982 fail-closed assertion. +> **No consumer anywhere repoints** — not in the framework, not in the +> plugins, not in the examples, not in the docs. The removal cannot break code +> that does not exist. +> - **One rule beats two correct rules.** Two verbs answering the same slot +> differently is a thing every hook author must hold in memory, and the +> justification for the split ("`delete()` re-resolves, `update()` cannot") +> lives in an ADR, not at the call site where a handler is written. +> - **The surface is a footgun independent of the mechanism.** "A hook silently +> redirects which row gets deleted" is a top-grade hazard for authored — and +> especially AI-authored — handlers. Correctness of a mechanism does not +> justify the surface it exposes. +> +> ⛔ **Route 3 stays excluded.** Aligning the other way — growing `update()` the +> same re-resolution — remains forbidden by #5574's recorded ruling ("do not +> silently pick re-resolution instead"), and this amendment does not reopen it. +> The alignment was only ever going to run in this direction; the open question +> was whether to align at all. +> +> **What replaces it.** Exactly what replaced the CLEARED lever, and for the +> same reasons: to delete a DIFFERENT row, call `ctx.api` / `ctx.ql` for that row +> explicitly; to delete MANY rows, have the caller pass `{ multi: true, where: … }`; +> to stop this delete, throw from the handler. +> +> **The mechanism removed with it.** The re-read block in `delete()`'s by-id +> branch is DELETED, not merely bypassed — its guard was `input.id !== id && +> input.id`, precisely the case the refusal now throws on, so it became +> unreachable code the moment the refusal landed. The single pre-dispatch +> pre-image read that binds `previous` for `beforeDelete` (#5846 (a) / #6697) is +> a different read and is untouched. + +--- + ## TL;DR ObjectStack exposes **~50 authorable declarations** that hold an expression — formulas, visibility/required/readonly predicates, validation rules, hook conditions, flow/edge conditions, sharing-rule conditions, RLS `using`/`check`, action/view/app visibility, notification/ETL/export/sync/connector conditions — and they all funnel through **one authoring primitive** (`ExpressionInputSchema` → `{ dialect: 'cel', source }`, helpers `cel`/`F`/`P`). The authoring surface is already unified and clean. diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 6e9e50931f..46f403a7aa 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -346,6 +346,15 @@ The action LOCATION vocabulary loses `global_nav` in this step (#6888, ADR-0049, - **`declarative-apis-endpoints-live`** — `stack.apis[] (every declared ApiEndpoint — REVIEW REQUIRED BEFORE UPGRADING)` → the same declarations, re-read as LIVE HTTP routes: `path` moved under `/api/v1/apps//`, and every entry that declares `authRequired: false` re-confirmed as an intentionally anonymous endpoint carrying `rateLimit: { enabled: true, … }` - Why not automatic: This is the one protocol-17 entry that turns metadata ON rather than off, so read it as a SECURITY review item and not as a rename. Before 17 the declarative endpoint surface executed NOTHING: no route was mounted for a declared `path`, no matcher existed, and every key — `authRequired` included — parsed green and gated nothing (#4936, which refused a non-empty `apis:` outright for exactly that reason). Protocol 17 ships the executor (#5040) and narrows that refusal to a per-endpoint publish gate: an endpoint that PASSES the gate is mounted and serves real traffic as soon as the stack is published. So an `apis:` block written against an older major — or one restored from a pre-#4936 source, or authored from a doc that predates the refusal — changes meaning without changing a byte: what used to be inert documentation becomes an execution entry point into the data and automation pipelines. Nothing about that transition can be applied mechanically, because the judgment it needs is "did the author of this endpoint mean for the internet to reach it?" — and the one key where a wrong answer is unrecoverable is `authRequired`. Its schema default is `true`, so an omission is SAFE and needs no review; an EXPLICIT `authRequired: false` is the only thing that opens anonymous access, and under ADR-0121 D6 it now also requires an armed `rateLimit` (`enabled: true` — the key defaults to `false`, so a budget written without it meters nothing) or the stack refuses to publish. ⚠️ If you author endpoints in TypeScript, annotate them with `ApiEndpoint` — the AUTHOR state — so that omitting `authRequired` compiles: `const e: ApiEndpoint = { name, path, method, type, target }` is legal and is the safe shape this paragraph prescribes. `ApiEndpointParsed` is the POST-parse type (defaults materialized, ADR-0122), where `authRequired` is required — annotating a declaration with it forces you to write the key out, and being made to think about a key whose only unrecoverable value is `false` is the one thing this entry is trying to avoid (#5227). Hold a parse RESULT with `ApiEndpointParsed`; write declarations as `ApiEndpoint`. Grep every `apis:` entry for `authRequired: false` before you upgrade, delete the ones that were never meant to be public, and arm a budget on the ones that were. The path move is the mechanical-looking half and is still yours: ADR-0121 D1/D2 confine a declared path to your own namespace carve-out (`/api/v1/apps//…`), the namespace comes from an explicit `manifest.namespace` with no derivation fallback, and the subpath is the only part you name — rewriting it for you would silently change a URL third parties call. - Done when: You have READ every entry of every `apis:` block, not just the ones that fail to publish. Concretely: (1) each declared `path` is `/api/v1/apps//` and the stack declares that `manifest.namespace` explicitly; (2) every entry declaring `authRequired: false` is one you INTEND to be reachable without a session, and each carries `rateLimit: { enabled: true, windowMs, maxRequests }` — entries that were not intended to be anonymous have the key removed so the safe default (`true`) applies; (3) `objectstack validate` passes, which also proves no endpoint declares a shape 17.x cannot execute (`type: script` / `proxy`, mapping `transform`, an `object_operation` missing `objectParams`, `cacheTtl` on a non-GET method, `inputMapping` on find/get/delete, or two endpoints claiming one METHOD + path); and (4) after publishing, each endpoint answers as you expect — an anonymous request to a session-only endpoint returns 401 rather than data. +- **`delete-by-id-before-hook-repoint-retired`** — `a `beforeDelete` handler on a BY-ID `delete()` assigning `ctx.input.id` a DIFFERENT id, to move the delete onto that row` → delete the other row explicitly — `ctx.ql.delete(object, otherId)` / `ctx.api` — and let the addressed delete proceed or `throw` from the handler to stop it; to delete MANY rows, have the CALLER pass `{ multi: true, where: … }`. Writing the SAME id back is unaffected and stays legal. + - Why not automatic: The by-id target of an `update()` or `delete()` is now IMMUTABLE inside a `before*` handler, on both verbs, cleared or rebound. `delete()` was the last cell of that table still answering differently: it HONOURED a repoint, re-resolving the new target by re-reading its pre-image and rebinding `previous` (#5272), so `afterDelete` and the roll-up recompute saw the row actually deleted. It now refuses with `HookTargetRebindError` / `ERR_HOOK_TARGET_REBIND`, `path: 'by-id'`, exactly as the `update()` twin and both per-row paths (ADR-0058 Amendment II.1 / D4) already did. + +Read this as a RULING, not a defect report — that distinction is the reason the entry is worth its length. #5272's re-resolution was internally CORRECT and nothing stale ever leaked from it; the case that retires a rebind on `update()` (the write landing on a row whose pre-image, `readonlyWhen` locks and validation rules were never evaluated) simply did not apply to it. #5574's engine half (PR #6697) therefore left the asymmetry standing on purpose rather than folding a behaviour removal into an ordering change, and filed it as #6752. The 2026-08-09 maintainer ruling on that card closed it on three measured axes instead: compatibility cost zero (a repository-wide grep for assignments into a hook's `input.id`, re-run on the implementing PR's base, found six sites and ALL SIX are this family's own pins — no consumer anywhere repoints); one rule across both verbs beats two individually-correct rules an author has to memorize, since the justification for the split lived in an ADR rather than at the call site; and "a hook silently redirects which row gets deleted" is a top-grade footgun for authored — especially AI-authored — handlers however correctly the redirect is implemented. Correctness of a mechanism does not justify the surface it exposes. Aligning the other way, by building `update()` the same re-resolution, stays excluded by #5574's own recorded ruling ("do not silently pick re-resolution instead"). + +Why this is a D3 semantic TODO and not a D2 conversion, on the same two grounds as `hook-register-empty-object-target-refused` and `hook-context-session-roles-retired` at this step: FIRST, there is no source to convert — a `HookContext` is constructed per write and never persisted, so no `sys_metadata` row, example or template can carry the assignment. SECOND, the only place it is ever SPELLED is inside a handler body: author-written JS/TS, or a sandboxed script whose context is `unknown`. A declarative transform cannot safely rewrite an assignment inside free-form code, and the intent is not recoverable anyway — only the author knows whether the repoint meant "delete that row INSTEAD" or "delete that row TOO". + +What makes this one cheaper to meet than its two siblings, and worth saying because it bounds the work: the removed capability has an ENFORCED channel at run time. The refusal throws before anything is written and its message NAMES the retired capability and the three replacement routes, so a handler that still repoints fails loudly and self-describingly on its first execution rather than going quiet. This ledger entry is the channel that reaches an upgrader BEFORE that first execution. #6752, #5272, #5574, PR #6697, ADR-0058 Amendment II.2. + - Done when: No `beforeDelete` handler assigns `ctx.input.id` anything but the id it arrived with — grep handler bodies for assignments into `input.id` and rewrite each into an explicit `ctx.ql.delete()` for the other row, a caller-side `{ multi: true, where: … }`, or a `throw`. A delete-heavy smoke run completes with no `HookTargetRebindError` (`ERR_HOOK_TARGET_REBIND`, `path: 'by-id'`, `event: 'beforeDelete'`) — and any that does raise names its `expectedId` and `observedId`, which identifies the handler that moved the target. - **`driver-aggregate-undeclared-key-aliases-removed`** — `driver aggregate() call argument — query.aggregate and aggregations[].func` → query.aggregations and aggregations[].function — the spellings QueryASTSchema and AggregationNodeSchema have always declared - Why not automatic: `SqlDriver.aggregate` and `RemoteTransport.aggregate` each read two aliases the Query Protocol has never declared: `query.aggregations || query.aggregate` and `agg.function || agg.func`. "Never declared" is measured, not assumed — `git log -S` over `data/query.zod.ts` finds no commit that ever introduced either name, there is no `retiredKey()` tombstone and no alias-table entry for them (the file's only alias table is `SortNode`'s `direction` → `order`), and neither appears in any upgrade guide or release note. So this entry does not record a declared surface being withdrawn; it records a LENIENCY being withdrawn, which is why it is here rather than behind a tombstone. The only writers in this repository were the two driver packages' own fixtures — #4984's family, where a fixture spelling the alias keeps the tolerant limb green forever and no test in existence can go red on its deletion — so ADR-0049 enforce-or-remove applies once those are re-spelt. ⚠️ Do NOT read this across to `dashboard`/`page` measures: `aggregate` IS the canonical key there and `func` IS a declared, loudly-suggesting alias (`DatasetMeasureSchema`, ui/dataset.zod.ts). That neighbouring vocabulary is untouched, and it is the most likely reason an off-repo caller ever wrote these keys on a QUERY — one habit, two surfaces, only one of which declared it. This is a driver CALL ARGUMENT — code, never stack metadata — so there is no source for the D2 chain to rewrite and deliberately no schema tombstone: nothing ever ran a query through `QueryASTSchema.parse()` on this path. The enforced channel is tsc at the call site, once the parameter is `DriverQuery` — and for an untyped JS caller there is no enforced channel at all, which is exactly why this ledger entry has to exist: the generated upgrade guide is the only way such a reader learns of the rename. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484), `storage-service-list-retired` (#5540) and `actor-user-roles-to-positions` (#6011). ADR-0049 / ADR-0087, #6321 (PR #6404). - Done when: No caller passes `aggregate:` to a driver's `aggregate()`, and no aggregation entry spells its function `func:`; both are written `aggregations:` / `function:`. An inline literal still using either old spelling no longer type-checks (TS2353 at the call site). An untyped JS caller that keeps writing `aggregate:` silently receives no aggregate column — the grouping still happens, the measure is simply absent — and one that keeps writing `func:` receives INVALID_QUERY / 400 naming the undeclared function, identically on the local driver and the Turso remote transport. diff --git a/packages/objectql/src/bulk-write-per-row-hooks.test.ts b/packages/objectql/src/bulk-write-per-row-hooks.test.ts index da4151e3e9..76550d9b1a 100644 --- a/packages/objectql/src/bulk-write-per-row-hooks.test.ts +++ b/packages/objectql/src/bulk-write-per-row-hooks.test.ts @@ -727,19 +727,27 @@ describe('[#5574 / D4] `input.id` is not a reroute lever, and is refused loudly' expect(await engine.count('task', {})).toBe(1); }); - it('still HONOURS a by-id `beforeDelete` REPOINT — deliberately not retired here', async () => { - // ⚠️ The one place the two verbs answer a rebind differently, pinned so the - // asymmetry is a decision on the record rather than something a later - // reader "tidies up" in either direction. + it('REFUSES a by-id `beforeDelete` REPOINT — the last cell, retired by #6752', async () => { + // ⚠️ FLIPPED, deliberately, not deleted. This case used to assert the exact + // opposite — `still HONOURS a by-id beforeDelete REPOINT` — and pinned the + // ONE place the two verbs answered a rebind differently. Read the flip as + // the record of a ruling, not as a bug being fixed: // - // The case against honouring a rebind is that the write lands on a row - // whose pre-image and rules were never evaluated. On `delete()` that is not - // true: #5272 RE-RESOLVES the new target — re-reads its pre-image and - // rebinds `previous` — before `afterDelete` or the summary recompute can - // see it. `update()` has no such mechanism and refusing there is what keeps - // this PR from inventing one (the ruling forbids picking re-resolution - // silently). Retiring the delete-side repoint is its own question, filed as - // #6752. + // * The honoured behaviour was CORRECT. #5272 RE-RESOLVED the new target, + // re-reading its pre-image and rebinding `previous`, so `afterDelete` + // and the summary recompute saw the row actually deleted. Nothing stale + // ever leaked. No defect was found in it. + // * It is retired anyway (maintainer ruling on #6752, 2026-08-09) on three + // measured axes: compatibility cost zero (no consumer in the repository + // repoints), one rule across both verbs beats two individually-correct + // rules an author must memorize, and "a hook silently redirects which + // row gets deleted" is a top-grade footgun regardless of how correctly + // the redirect is implemented. + // * ⛔ The other alignment — building `update()` the same re-resolution — + // stays excluded by #5574's ruling. Do not "restore symmetry" that way. + // + // ADR-0058 Amendment II.2. The rule is now one line: a by-id target is + // immutable in a `before*` handler. const seen: unknown[] = []; const { engine } = await boot([ hook('repoint', 'beforeDelete', (ctx) => { @@ -750,14 +758,53 @@ describe('[#5574 / D4] `input.id` is not a reroute lever, and is refused loudly' const decoy: any = await engine.insert('task', { title: 'decoy', status: 'todo' }); const target: any = await engine.insert('task', { title: 'target', status: 'todo' }); - await engine.delete('task', { where: { id: decoy.id } }); + const err = await engine + .delete('task', { where: { id: decoy.id } }) + .then(() => null, (e) => e); + + // The refusal, and its full envelope — the same one the `update()` twin and + // the per-row paths raise. `expectedId`/`observedId` name BOTH ends of the + // attempted move, which is what tells an author which handler did it. + expect(err).toBeInstanceOf(HookTargetRebindError); + expect(err.code).toBe(HOOK_TARGET_REBIND_ERROR_CODE); + expect(err.name).toBe('HookTargetRebindError'); + expect(err.object).toBe('task'); + expect(err.event).toBe('beforeDelete'); + expect(err.path).toBe('by-id'); + expect(err.expectedId).toBe(decoy.id); + expect(err.observedId).toBe(String(target.id)); + expect(err.message).toContain('RETIRED'); + expect(err.message).toContain('REBOUND'); - // The REPOINTED row is the one that went… + // Refused means NOTHING was deleted — not the row the caller named, and not + // the row the handler aimed at. Both are still there. const left: any[] = await engine.find('task', {}); - expect(left.map((r) => r.title)).toEqual(['decoy']); - // …and `previous` was re-read for it, so the after phase describes the row - // actually deleted rather than the one the caller named. - expect(seen).toEqual(['target']); + expect(left.map((r) => r.title).sort()).toEqual(['decoy', 'target']); + // And the after phase never ran at all: there is no delete to describe. + expect(seen).toEqual([]); + }); + + it('leaves a by-id `beforeDelete` that rewrites the SAME id alone', async () => { + // The negative control for the flip above, and the measurement it rests on: + // the refusal is `input.id !== id` — the `update()` check verbatim — so a + // handler that assigns the id back to itself is NOT caught. This was legal + // before #6752 and stays legal after; what is retired is the REPOINT, not + // touching the slot. + const seen: unknown[] = []; + const { engine } = await boot([ + hook('rewrite', 'beforeDelete', (ctx) => { + (ctx.input as any).id = (ctx.input as any).id; + }), + hook('observe', 'afterDelete', (ctx) => { seen.push((ctx.previous as any)?.title); }, undefined, 'task', { priority: 200 }), + ]); + const row: any = await engine.insert('task', { title: 'a', status: 'todo' }); + + await engine.delete('task', { where: { id: row.id } }); + + // The addressed row went, and `afterDelete` describes THAT row — the + // originally-addressed one, which is now the only row it can ever describe. + expect(await engine.count('task', {})).toBe(0); + expect(seen).toEqual(['a']); }); it('leaves an untouched `input.id` alone — the refusal is not a trap', async () => { diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 826be6866b..8ff7d3bc80 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -8897,34 +8897,37 @@ export class ObjectQL implements IObjectQLEngine { bindPreImage(priorRecord); } await this.triggerHooks('beforeDelete', hookContext); - // A `beforeDelete` hook may still REPOINT the target id, and #5272's - // answer to that is unchanged: the pre-image bound above describes the - // OLD id, so it must not ride into `afterDelete` — or into the summary - // recompute — as though it described the new target. Re-read it. + // [#6752] The retired lever, refused — the `update()` twin's check, + // verbatim, because the rule is now ONE rule: a by-id target is + // immutable in a `before*` handler. // - // [#5574] What this PR does NOT do, deliberately: retire the repoint. - // The `update()` twin below refuses a rebind, and the asymmetry is - // principled rather than an oversight — `delete()` has a working - // RE-RESOLUTION for the new target (this block, delivered by #5272 with - // its own pins), so nothing stale reaches a consumer, while `update()` - // has none and would have to grow one. Building that is exactly the - // "silently pick re-resolution instead" the ruling forbids, so the two - // paths answer differently until the repoint itself is ruled on. Filed - // as #6752; do not fold it in as a rider here. - if (wantsPreImage && hookContext.input.id !== id && hookContext.input.id) { - priorRecord = await readPreImage(hookContext.input.id); - bindPreImage(priorRecord); - } - // CLEARING the id is a different question and this PR does settle it, - // because the ladder reorder leaves it no answer of its own: it used to - // convert the write into a PREDICATE delete over the caller's `where` - // by falling through to the branch below, and the ladder is now decided - // before any handler runs (a per-row `before*` context is built from the - // matched row set, so it must be). Ignoring it would delete the - // ORIGINAL row while the handler believes it cancelled the targeting; - // honouring it has nothing left to honour. Refused by name — ADR-0058 - // Amendment II.1, the capability the ruling names. - if (!hookContext.input.id) { + // Both halves of it used to be answered separately here. CLEARING the + // id was already refused (ADR-0058 Amendment II.1): it worked by + // falling through to the predicate branch, and the ladder is now + // resolved before any handler runs, so there is no ladder left to + // re-enter. REBINDING to another id was still HONOURED, by re-reading + // the new target's pre-image and rebinding `previous` (#5272) so + // nothing stale reached `afterDelete` or the summary recompute. + // + // The 2026-08-09 ruling on #6752 retires that second half. #5272's + // mechanism was internally correct — that is not what was weighed. What + // was weighed: the measured compatibility cost is zero (no consumer in + // the repository repoints), one rule across both verbs beats two + // individually-correct rules an author has to memorize, and a hook that + // silently redirects WHICH ROW GETS DELETED is a top-grade footgun for + // authored — especially AI-authored — handlers. Correctness of the + // mechanism does not justify the surface. + // + // ⛔ Route 3 (growing the same re-resolution for `update()`) stays + // excluded by #5574's ruling: "do not silently pick re-resolution + // instead". The alignment goes the other way, and this is that edit. + // + // The re-read that used to sit here is GONE, not merely bypassed: its + // guard was `input.id !== id && input.id`, which is exactly the case + // this refusal now throws on, so it became unreachable. `readPreImage` + // survives for the pre-dispatch read above — the only read left. A + // handler writing back the SAME id is untouched, as on `update()`. + if (hookContext.input.id !== id) { throw new HookTargetRebindError({ object, event: 'beforeDelete', path: 'by-id', expectedId: id, observedId: hookContext.input.id, diff --git a/packages/objectql/src/hook-target-rebind-errors.ts b/packages/objectql/src/hook-target-rebind-errors.ts index b2ce232877..6f75090d78 100644 --- a/packages/objectql/src/hook-target-rebind-errors.ts +++ b/packages/objectql/src/hook-target-rebind-errors.ts @@ -42,26 +42,45 @@ * with `id` ALREADY bound to its row, and rebinding it retargets nothing — so * one error serves both paths. * - * ## What this error does NOT cover: `delete()`'s by-id REPOINT - * - * The two verbs answer a rebind differently on the by-id path, and the - * asymmetry is a scope decision rather than an oversight — read it before - * "fixing" it either way. - * - * `delete()` has had a working RE-RESOLUTION for a repointed target since - * #5272: when a `beforeDelete` handler moves `input.id`, the engine re-reads - * that row's pre-image and rebinds `previous`, so nothing stale reaches - * `afterDelete` or the summary recompute. The second bullet above simply is not - * true there. `update()` has no such mechanism, and building one would be the - * "silently pick re-resolution instead" the ruling forbids — so `update()` - * refuses a rebind and `delete()` keeps honouring one, until the repoint itself - * is ruled on as its own question (#6752). - * - * A CLEARED id is a different question and both verbs answer it the same way, - * because the ladder reorder leaves it no answer of its own: clearing used to - * convert the write into a PREDICATE write over the caller's `where`, and the - * ladder is now resolved before any handler runs. That is the capability the - * ruling names, and it is what this error is for on the `delete()` path. + * ## What this error covers: every cell, on both verbs + * + * There is no longer an exception to remember. A `before*` handler may not move + * `ctx.input.id` — cleared or rebound, by-id or per-row, `update()` or + * `delete()` — and each of the four cells answers with this error: + * + * | | CLEARED id | REBOUND to another id | + * |---|---|---| + * | `update()` by-id | refused | refused | + * | `delete()` by-id | refused | refused (#6752) | + * | either, per-row | refused (D4) | refused (D4) | + * + * The last cell to arrive was `delete()`'s by-id REPOINT, and it is worth + * knowing why it arrived LATE rather than with the rest — the record matters + * more than the outcome here, because the mechanism it removed was not broken. + * + * `delete()` had a working RE-RESOLUTION for a repointed target from #5272 + * until #6752: when a `beforeDelete` handler moved `input.id`, the engine + * re-read that row's pre-image and rebound `previous`, so nothing stale reached + * `afterDelete` or the summary recompute. The second bullet above — the write + * landing on a row nothing was computed against — was simply not true there, so + * `delete()` kept honouring a rebind while `update()` refused one, and #5574's + * engine half (PR #6697) deliberately left the asymmetry standing rather than + * folding a behaviour removal into an ordering change. + * + * The 2026-08-09 ruling on #6752 closed it, and NOT by finding a defect: the + * measured compatibility cost was zero (no consumer in the repository + * repointed), one rule across both verbs beats two individually-correct rules + * an author must memorize, and a hook that silently redirects which row gets + * deleted is a top-grade footgun for authored handlers. Correctness of the + * mechanism did not justify the surface. ⛔ The symmetric alternative — + * building `update()` the same re-resolution — stays excluded by #5574's own + * ruling ("do not silently pick re-resolution instead"); the alignment was + * always going to run this direction. + * + * A CLEARED id never had a verb-specific answer, because the ladder reorder + * leaves it none of its own: clearing used to convert the write into a + * PREDICATE write over the caller's `where`, and the ladder is now resolved + * before any handler runs. * * ## Why `code` is an `ERR_`-prefixed operational code, not a wire code * @@ -141,9 +160,11 @@ function buildMessage(info: { `predicate path has to read its matched rows first, to build one context per row — so there ` + `is no ladder left to re-enter.` : ` The capability this used to have is RETIRED: rebinding 'input.id' in a '${event}' handler ` + - `moved the write to another row. The engine now reads that row's pre-image, evaluates its ` + - `'readonlyWhen' locks and runs its validation rules BEFORE dispatching, so honouring a ` + - `rebind would write a row that none of those checks ever saw.` + `moved the write to another row. The engine now resolves the target BEFORE the before phase ` + + `and computes the whole write against it — the pre-image, the 'readonlyWhen' locks, the ` + + `validation rules — so a by-id target is immutable once a handler runs, on BOTH verbs. ` + + `'delete()' honoured a rebind until #6752 by re-resolving the new target; that is retired ` + + `too, so one rule now covers both.` : ` On a predicate write a '${event}' context arrives with 'id' ALREADY bound to its row and the ` + `dispatch decided, so rebinding it retargets nothing (ADR-0058 Addendum II, D4). It is refused ` + `rather than ignored, because a silent no-op is the failure this contract exists to abolish.`; diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index 92921c6058..21f33ad528 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -581,6 +581,13 @@ "toMajor": 17, "rationale": "This is the one protocol-17 entry that turns metadata ON rather than off, so read it as a SECURITY review item and not as a rename. Before 17 the declarative endpoint surface executed NOTHING: no route was mounted for a declared `path`, no matcher existed, and every key — `authRequired` included — parsed green and gated nothing (#4936, which refused a non-empty `apis:` outright for exactly that reason). Protocol 17 ships the executor (#5040) and narrows that refusal to a per-endpoint publish gate: an endpoint that PASSES the gate is mounted and serves real traffic as soon as the stack is published. So an `apis:` block written against an older major — or one restored from a pre-#4936 source, or authored from a doc that predates the refusal — changes meaning without changing a byte: what used to be inert documentation becomes an execution entry point into the data and automation pipelines. Nothing about that transition can be applied mechanically, because the judgment it needs is \"did the author of this endpoint mean for the internet to reach it?\" — and the one key where a wrong answer is unrecoverable is `authRequired`. Its schema default is `true`, so an omission is SAFE and needs no review; an EXPLICIT `authRequired: false` is the only thing that opens anonymous access, and under ADR-0121 D6 it now also requires an armed `rateLimit` (`enabled: true` — the key defaults to `false`, so a budget written without it meters nothing) or the stack refuses to publish. ⚠️ If you author endpoints in TypeScript, annotate them with `ApiEndpoint` — the AUTHOR state — so that omitting `authRequired` compiles: `const e: ApiEndpoint = { name, path, method, type, target }` is legal and is the safe shape this paragraph prescribes. `ApiEndpointParsed` is the POST-parse type (defaults materialized, ADR-0122), where `authRequired` is required — annotating a declaration with it forces you to write the key out, and being made to think about a key whose only unrecoverable value is `false` is the one thing this entry is trying to avoid (#5227). Hold a parse RESULT with `ApiEndpointParsed`; write declarations as `ApiEndpoint`. Grep every `apis:` entry for `authRequired: false` before you upgrade, delete the ones that were never meant to be public, and arm a budget on the ones that were. The path move is the mechanical-looking half and is still yours: ADR-0121 D1/D2 confine a declared path to your own namespace carve-out (`/api/v1/apps//…`), the namespace comes from an explicit `manifest.namespace` with no derivation fallback, and the subpath is the only part you name — rewriting it for you would silently change a URL third parties call." }, + { + "surface": "a `beforeDelete` handler on a BY-ID `delete()` assigning `ctx.input.id` a DIFFERENT id, to move the delete onto that row", + "replacement": "delete the other row explicitly — `ctx.ql.delete(object, otherId)` / `ctx.api` — and let the addressed delete proceed or `throw` from the handler to stop it; to delete MANY rows, have the CALLER pass `{ multi: true, where: … }`. Writing the SAME id back is unaffected and stays legal.", + "migrationId": "delete-by-id-before-hook-repoint-retired", + "toMajor": 17, + "rationale": "The by-id target of an `update()` or `delete()` is now IMMUTABLE inside a `before*` handler, on both verbs, cleared or rebound. `delete()` was the last cell of that table still answering differently: it HONOURED a repoint, re-resolving the new target by re-reading its pre-image and rebinding `previous` (#5272), so `afterDelete` and the roll-up recompute saw the row actually deleted. It now refuses with `HookTargetRebindError` / `ERR_HOOK_TARGET_REBIND`, `path: 'by-id'`, exactly as the `update()` twin and both per-row paths (ADR-0058 Amendment II.1 / D4) already did.\n\nRead this as a RULING, not a defect report — that distinction is the reason the entry is worth its length. #5272's re-resolution was internally CORRECT and nothing stale ever leaked from it; the case that retires a rebind on `update()` (the write landing on a row whose pre-image, `readonlyWhen` locks and validation rules were never evaluated) simply did not apply to it. #5574's engine half (PR #6697) therefore left the asymmetry standing on purpose rather than folding a behaviour removal into an ordering change, and filed it as #6752. The 2026-08-09 maintainer ruling on that card closed it on three measured axes instead: compatibility cost zero (a repository-wide grep for assignments into a hook's `input.id`, re-run on the implementing PR's base, found six sites and ALL SIX are this family's own pins — no consumer anywhere repoints); one rule across both verbs beats two individually-correct rules an author has to memorize, since the justification for the split lived in an ADR rather than at the call site; and \"a hook silently redirects which row gets deleted\" is a top-grade footgun for authored — especially AI-authored — handlers however correctly the redirect is implemented. Correctness of a mechanism does not justify the surface it exposes. Aligning the other way, by building `update()` the same re-resolution, stays excluded by #5574's own recorded ruling (\"do not silently pick re-resolution instead\").\n\nWhy this is a D3 semantic TODO and not a D2 conversion, on the same two grounds as `hook-register-empty-object-target-refused` and `hook-context-session-roles-retired` at this step: FIRST, there is no source to convert — a `HookContext` is constructed per write and never persisted, so no `sys_metadata` row, example or template can carry the assignment. SECOND, the only place it is ever SPELLED is inside a handler body: author-written JS/TS, or a sandboxed script whose context is `unknown`. A declarative transform cannot safely rewrite an assignment inside free-form code, and the intent is not recoverable anyway — only the author knows whether the repoint meant \"delete that row INSTEAD\" or \"delete that row TOO\".\n\nWhat makes this one cheaper to meet than its two siblings, and worth saying because it bounds the work: the removed capability has an ENFORCED channel at run time. The refusal throws before anything is written and its message NAMES the retired capability and the three replacement routes, so a handler that still repoints fails loudly and self-describingly on its first execution rather than going quiet. This ledger entry is the channel that reaches an upgrader BEFORE that first execution. #6752, #5272, #5574, PR #6697, ADR-0058 Amendment II.2." + }, { "surface": "driver aggregate() call argument — query.aggregate and aggregations[].func", "replacement": "query.aggregations and aggregations[].function — the spellings QueryASTSchema and AggregationNodeSchema have always declared", @@ -1458,6 +1465,13 @@ "toMajor": 17, "rationale": "This is the one protocol-17 entry that turns metadata ON rather than off, so read it as a SECURITY review item and not as a rename. Before 17 the declarative endpoint surface executed NOTHING: no route was mounted for a declared `path`, no matcher existed, and every key — `authRequired` included — parsed green and gated nothing (#4936, which refused a non-empty `apis:` outright for exactly that reason). Protocol 17 ships the executor (#5040) and narrows that refusal to a per-endpoint publish gate: an endpoint that PASSES the gate is mounted and serves real traffic as soon as the stack is published. So an `apis:` block written against an older major — or one restored from a pre-#4936 source, or authored from a doc that predates the refusal — changes meaning without changing a byte: what used to be inert documentation becomes an execution entry point into the data and automation pipelines. Nothing about that transition can be applied mechanically, because the judgment it needs is \"did the author of this endpoint mean for the internet to reach it?\" — and the one key where a wrong answer is unrecoverable is `authRequired`. Its schema default is `true`, so an omission is SAFE and needs no review; an EXPLICIT `authRequired: false` is the only thing that opens anonymous access, and under ADR-0121 D6 it now also requires an armed `rateLimit` (`enabled: true` — the key defaults to `false`, so a budget written without it meters nothing) or the stack refuses to publish. ⚠️ If you author endpoints in TypeScript, annotate them with `ApiEndpoint` — the AUTHOR state — so that omitting `authRequired` compiles: `const e: ApiEndpoint = { name, path, method, type, target }` is legal and is the safe shape this paragraph prescribes. `ApiEndpointParsed` is the POST-parse type (defaults materialized, ADR-0122), where `authRequired` is required — annotating a declaration with it forces you to write the key out, and being made to think about a key whose only unrecoverable value is `false` is the one thing this entry is trying to avoid (#5227). Hold a parse RESULT with `ApiEndpointParsed`; write declarations as `ApiEndpoint`. Grep every `apis:` entry for `authRequired: false` before you upgrade, delete the ones that were never meant to be public, and arm a budget on the ones that were. The path move is the mechanical-looking half and is still yours: ADR-0121 D1/D2 confine a declared path to your own namespace carve-out (`/api/v1/apps//…`), the namespace comes from an explicit `manifest.namespace` with no derivation fallback, and the subpath is the only part you name — rewriting it for you would silently change a URL third parties call." }, + { + "surface": "a `beforeDelete` handler on a BY-ID `delete()` assigning `ctx.input.id` a DIFFERENT id, to move the delete onto that row", + "replacement": "delete the other row explicitly — `ctx.ql.delete(object, otherId)` / `ctx.api` — and let the addressed delete proceed or `throw` from the handler to stop it; to delete MANY rows, have the CALLER pass `{ multi: true, where: … }`. Writing the SAME id back is unaffected and stays legal.", + "migrationId": "delete-by-id-before-hook-repoint-retired", + "toMajor": 17, + "rationale": "The by-id target of an `update()` or `delete()` is now IMMUTABLE inside a `before*` handler, on both verbs, cleared or rebound. `delete()` was the last cell of that table still answering differently: it HONOURED a repoint, re-resolving the new target by re-reading its pre-image and rebinding `previous` (#5272), so `afterDelete` and the roll-up recompute saw the row actually deleted. It now refuses with `HookTargetRebindError` / `ERR_HOOK_TARGET_REBIND`, `path: 'by-id'`, exactly as the `update()` twin and both per-row paths (ADR-0058 Amendment II.1 / D4) already did.\n\nRead this as a RULING, not a defect report — that distinction is the reason the entry is worth its length. #5272's re-resolution was internally CORRECT and nothing stale ever leaked from it; the case that retires a rebind on `update()` (the write landing on a row whose pre-image, `readonlyWhen` locks and validation rules were never evaluated) simply did not apply to it. #5574's engine half (PR #6697) therefore left the asymmetry standing on purpose rather than folding a behaviour removal into an ordering change, and filed it as #6752. The 2026-08-09 maintainer ruling on that card closed it on three measured axes instead: compatibility cost zero (a repository-wide grep for assignments into a hook's `input.id`, re-run on the implementing PR's base, found six sites and ALL SIX are this family's own pins — no consumer anywhere repoints); one rule across both verbs beats two individually-correct rules an author has to memorize, since the justification for the split lived in an ADR rather than at the call site; and \"a hook silently redirects which row gets deleted\" is a top-grade footgun for authored — especially AI-authored — handlers however correctly the redirect is implemented. Correctness of a mechanism does not justify the surface it exposes. Aligning the other way, by building `update()` the same re-resolution, stays excluded by #5574's own recorded ruling (\"do not silently pick re-resolution instead\").\n\nWhy this is a D3 semantic TODO and not a D2 conversion, on the same two grounds as `hook-register-empty-object-target-refused` and `hook-context-session-roles-retired` at this step: FIRST, there is no source to convert — a `HookContext` is constructed per write and never persisted, so no `sys_metadata` row, example or template can carry the assignment. SECOND, the only place it is ever SPELLED is inside a handler body: author-written JS/TS, or a sandboxed script whose context is `unknown`. A declarative transform cannot safely rewrite an assignment inside free-form code, and the intent is not recoverable anyway — only the author knows whether the repoint meant \"delete that row INSTEAD\" or \"delete that row TOO\".\n\nWhat makes this one cheaper to meet than its two siblings, and worth saying because it bounds the work: the removed capability has an ENFORCED channel at run time. The refusal throws before anything is written and its message NAMES the retired capability and the three replacement routes, so a handler that still repoints fails loudly and self-describingly on its first execution rather than going quiet. This ledger entry is the channel that reaches an upgrader BEFORE that first execution. #6752, #5272, #5574, PR #6697, ADR-0058 Amendment II.2." + }, { "surface": "driver aggregate() call argument — query.aggregate and aggregations[].func", "replacement": "query.aggregations and aggregations[].function — the spellings QueryASTSchema and AggregationNodeSchema have always declared", diff --git a/packages/spec/src/migrations/entries/semantic/17.delete-by-id-before-hook-repoint-retired.ts b/packages/spec/src/migrations/entries/semantic/17.delete-by-id-before-hook-repoint-retired.ts new file mode 100644 index 0000000000..aa06ffb1e0 --- /dev/null +++ b/packages/spec/src/migrations/entries/semantic/17.delete-by-id-before-hook-repoint-retired.ts @@ -0,0 +1,64 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { SemanticMigration } from '../../types.js'; + +export const entry: SemanticMigration = { + id: 'delete-by-id-before-hook-repoint-retired', + surface: + 'a `beforeDelete` handler on a BY-ID `delete()` assigning `ctx.input.id` a DIFFERENT id, ' + + 'to move the delete onto that row', + replacement: + 'delete the other row explicitly — `ctx.ql.delete(object, otherId)` / `ctx.api` — and let ' + + 'the addressed delete proceed or `throw` from the handler to stop it; to delete MANY rows, ' + + "have the CALLER pass `{ multi: true, where: … }`. Writing the SAME id back is unaffected " + + 'and stays legal.', + reason: + 'The by-id target of an `update()` or `delete()` is now IMMUTABLE inside a `before*` ' + + 'handler, on both verbs, cleared or rebound. `delete()` was the last cell of that table ' + + 'still answering differently: it HONOURED a repoint, re-resolving the new target by ' + + "re-reading its pre-image and rebinding `previous` (#5272), so `afterDelete` and the " + + 'roll-up recompute saw the row actually deleted. It now refuses with ' + + '`HookTargetRebindError` / `ERR_HOOK_TARGET_REBIND`, `path: \'by-id\'`, exactly as the ' + + '`update()` twin and both per-row paths (ADR-0058 Amendment II.1 / D4) already did.\n\n' + + 'Read this as a RULING, not a defect report — that distinction is the reason the entry ' + + 'is worth its length. #5272\'s re-resolution was internally CORRECT and nothing stale ' + + 'ever leaked from it; the case that retires a rebind on `update()` (the write landing on ' + + 'a row whose pre-image, `readonlyWhen` locks and validation rules were never evaluated) ' + + 'simply did not apply to it. #5574\'s engine half (PR #6697) therefore left the asymmetry ' + + 'standing on purpose rather than folding a behaviour removal into an ordering change, and ' + + 'filed it as #6752. The 2026-08-09 maintainer ruling on that card closed it on three ' + + 'measured axes instead: compatibility cost zero (a repository-wide grep for assignments ' + + "into a hook's `input.id`, re-run on the implementing PR's base, found six sites and ALL " + + 'SIX are this family\'s own pins — no consumer anywhere repoints); one rule across both ' + + 'verbs beats two individually-correct rules an author has to memorize, since the ' + + 'justification for the split lived in an ADR rather than at the call site; and "a hook ' + + 'silently redirects which row gets deleted" is a top-grade footgun for authored — ' + + 'especially AI-authored — handlers however correctly the redirect is implemented. ' + + 'Correctness of a mechanism does not justify the surface it exposes. Aligning the other ' + + 'way, by building `update()` the same re-resolution, stays excluded by #5574\'s own ' + + 'recorded ruling ("do not silently pick re-resolution instead").\n\n' + + 'Why this is a D3 semantic TODO and not a D2 conversion, on the same two grounds as ' + + '`hook-register-empty-object-target-refused` and `hook-context-session-roles-retired` at ' + + 'this step: FIRST, there is no source to convert — a `HookContext` is constructed per ' + + 'write and never persisted, so no `sys_metadata` row, example or template can carry the ' + + 'assignment. SECOND, the only place it is ever SPELLED is inside a handler body: ' + + 'author-written JS/TS, or a sandboxed script whose context is `unknown`. A declarative ' + + 'transform cannot safely rewrite an assignment inside free-form code, and the intent is ' + + 'not recoverable anyway — only the author knows whether the repoint meant "delete that ' + + 'row INSTEAD" or "delete that row TOO".\n\n' + + 'What makes this one cheaper to meet than its two siblings, and worth saying because it ' + + 'bounds the work: the removed capability has an ENFORCED channel at run time. The refusal ' + + 'throws before anything is written and its message NAMES the retired capability and the ' + + 'three replacement routes, so a handler that still repoints fails loudly and self-' + + 'describingly on its first execution rather than going quiet. This ledger entry is the ' + + 'channel that reaches an upgrader BEFORE that first execution. #6752, #5272, #5574, ' + + 'PR #6697, ADR-0058 Amendment II.2.', + acceptanceCriteria: + 'No `beforeDelete` handler assigns `ctx.input.id` anything but the id it arrived with — ' + + 'grep handler bodies for assignments into `input.id` and rewrite each into an explicit ' + + '`ctx.ql.delete()` for the other row, a caller-side `{ multi: true, where: … }`, or a ' + + '`throw`. A delete-heavy smoke run completes with no `HookTargetRebindError` ' + + "(`ERR_HOOK_TARGET_REBIND`, `path: 'by-id'`, `event: 'beforeDelete'`) — and any that does " + + 'raise names its `expectedId` and `observedId`, which identifies the handler that moved ' + + 'the target.', +}; diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 7133a29139..e6f785236e 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -2004,6 +2004,66 @@ const step17: MigrationStep = { + '(4) after publishing, each endpoint answers as you expect — an anonymous request to ' + 'a session-only endpoint returns 401 rather than data.', }, + { + id: 'delete-by-id-before-hook-repoint-retired', + surface: + 'a `beforeDelete` handler on a BY-ID `delete()` assigning `ctx.input.id` a DIFFERENT id, ' + + 'to move the delete onto that row', + replacement: + 'delete the other row explicitly — `ctx.ql.delete(object, otherId)` / `ctx.api` — and let ' + + 'the addressed delete proceed or `throw` from the handler to stop it; to delete MANY rows, ' + + "have the CALLER pass `{ multi: true, where: … }`. Writing the SAME id back is unaffected " + + 'and stays legal.', + reason: + 'The by-id target of an `update()` or `delete()` is now IMMUTABLE inside a `before*` ' + + 'handler, on both verbs, cleared or rebound. `delete()` was the last cell of that table ' + + 'still answering differently: it HONOURED a repoint, re-resolving the new target by ' + + "re-reading its pre-image and rebinding `previous` (#5272), so `afterDelete` and the " + + 'roll-up recompute saw the row actually deleted. It now refuses with ' + + '`HookTargetRebindError` / `ERR_HOOK_TARGET_REBIND`, `path: \'by-id\'`, exactly as the ' + + '`update()` twin and both per-row paths (ADR-0058 Amendment II.1 / D4) already did.\n\n' + + 'Read this as a RULING, not a defect report — that distinction is the reason the entry ' + + 'is worth its length. #5272\'s re-resolution was internally CORRECT and nothing stale ' + + 'ever leaked from it; the case that retires a rebind on `update()` (the write landing on ' + + 'a row whose pre-image, `readonlyWhen` locks and validation rules were never evaluated) ' + + 'simply did not apply to it. #5574\'s engine half (PR #6697) therefore left the asymmetry ' + + 'standing on purpose rather than folding a behaviour removal into an ordering change, and ' + + 'filed it as #6752. The 2026-08-09 maintainer ruling on that card closed it on three ' + + 'measured axes instead: compatibility cost zero (a repository-wide grep for assignments ' + + "into a hook's `input.id`, re-run on the implementing PR's base, found six sites and ALL " + + 'SIX are this family\'s own pins — no consumer anywhere repoints); one rule across both ' + + 'verbs beats two individually-correct rules an author has to memorize, since the ' + + 'justification for the split lived in an ADR rather than at the call site; and "a hook ' + + 'silently redirects which row gets deleted" is a top-grade footgun for authored — ' + + 'especially AI-authored — handlers however correctly the redirect is implemented. ' + + 'Correctness of a mechanism does not justify the surface it exposes. Aligning the other ' + + 'way, by building `update()` the same re-resolution, stays excluded by #5574\'s own ' + + 'recorded ruling ("do not silently pick re-resolution instead").\n\n' + + 'Why this is a D3 semantic TODO and not a D2 conversion, on the same two grounds as ' + + '`hook-register-empty-object-target-refused` and `hook-context-session-roles-retired` at ' + + 'this step: FIRST, there is no source to convert — a `HookContext` is constructed per ' + + 'write and never persisted, so no `sys_metadata` row, example or template can carry the ' + + 'assignment. SECOND, the only place it is ever SPELLED is inside a handler body: ' + + 'author-written JS/TS, or a sandboxed script whose context is `unknown`. A declarative ' + + 'transform cannot safely rewrite an assignment inside free-form code, and the intent is ' + + 'not recoverable anyway — only the author knows whether the repoint meant "delete that ' + + 'row INSTEAD" or "delete that row TOO".\n\n' + + 'What makes this one cheaper to meet than its two siblings, and worth saying because it ' + + 'bounds the work: the removed capability has an ENFORCED channel at run time. The refusal ' + + 'throws before anything is written and its message NAMES the retired capability and the ' + + 'three replacement routes, so a handler that still repoints fails loudly and self-' + + 'describingly on its first execution rather than going quiet. This ledger entry is the ' + + 'channel that reaches an upgrader BEFORE that first execution. #6752, #5272, #5574, ' + + 'PR #6697, ADR-0058 Amendment II.2.', + acceptanceCriteria: + 'No `beforeDelete` handler assigns `ctx.input.id` anything but the id it arrived with — ' + + 'grep handler bodies for assignments into `input.id` and rewrite each into an explicit ' + + '`ctx.ql.delete()` for the other row, a caller-side `{ multi: true, where: … }`, or a ' + + '`throw`. A delete-heavy smoke run completes with no `HookTargetRebindError` ' + + "(`ERR_HOOK_TARGET_REBIND`, `path: 'by-id'`, `event: 'beforeDelete'`) — and any that does " + + 'raise names its `expectedId` and `observedId`, which identifies the handler that moved ' + + 'the target.', + }, { id: 'driver-aggregate-undeclared-key-aliases-removed', // No backticks in `surface`: the upgrade-guide renderer wraps this string