fix(spec,objectql,sharing,storage): state per-row vs record dispatch on the hook contract (#6966) - #7101
Merged
Merged
Conversation
…on the hook contract (#6966) A predicate (`multi: true`) write dispatches its lifecycle hooks once per matched row — `after*` since #5038, `before*` since #5574 — on a context deliberately indistinguishable from a single-id write's. That is the feature, and it erased the only signal several handlers had: before #5574 a bulk `before*` fired once with `input.id` present-but-`undefined`, so "no id" meant "this call stands for N rows". Every guard written on it silently inverted rather than failing. Adds `HookContext.dispatch` — `{ mode: 'record' | 'per-row', index, scope }` — bound by the engine at every write dispatch site (insert, update, delete, both phases), at the point the dispatch ladder is decided. Optional, and an absent marker reads as "not per-row", so existing handlers keep their behaviour. `scope` is one object shared by every dispatch of one write across both phases: the seam handlers used to get by stashing on the context, which only ever worked because a single-id write reuses one context across its pair. Deliberately not the `isPredicateBulkWrite` discriminator #5574 retired under ADR-0049: that one inferred "bulk" at the consumer from `input.id` and `options.multi` and ended with no producer and no reachable consumer. This one is engine-produced and has readers. Behaviour fixed: - plugin-sharing — the `before*` stash of a write's affected row set was landing on a per-row context the `after*` phase never saw, so every bulk update or delete on a ruled object revoked all of that object's rule grants and queued a full asynchronous re-grant, once per matched row, with the repeats racing each other. Access was never widened; a bounded write now takes the bounded path again, the cap still applies to the union, and the `after*` work runs once per write instead of N times (the bounded branch was quadratic in batch size). - service-storage — the `beforeDelete` id pre-resolution was dead on every path and `afterDelete` was doing one `sys_file` lookup per row where the batch fits one `$in`. The pre-resolution query is gone entirely: the engine has already matched the rows. `beforeUpdate` copy-on-claim no longer runs per row against a batch-scoped payload, removing a row-conditioned rewrite of a shared SET clause (out of contract under ADR-0058 Addendum II D3). Stale comments in both packages asserting that predicate writes never populate `input.id`, and that the engine reuses one `HookContext` across a write's before/after pair, are corrected where they sit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcLMsZRoR3daV3QzPXgwxK
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 4 package(s): 110 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
github-actions
Bot
added
documentation
…nership note (#6966) The comment said the bulk-update file-ownership hole was "filed separately"; it is #7102. A pointer a reader can follow beats a promise they cannot check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcLMsZRoR3daV3QzPXgwxK
…#6966) `HookContext.dispatch`'s JSDoc claims every dispatched context carries it — which is what makes `ctx.dispatch?.mode` safe to read with no "what if the engine did not bind it" branch. The per-path cases prove the four write paths; this proves the claim itself, and covers the one context a reader might worry about: update()/delete() keep a batch-scoped `hookContext` on the predicate path, and no handler ever sees it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcLMsZRoR3daV3QzPXgwxK
…ch (#6966) `content/docs/references/data/hook.mdx` is generated from the Zod schema, so the new key has to land there too — `check:docs` is what caught it. The row renders all three members inline (three keys, under the renderer's four-key limit), so it carries no `…` elision and none of the `any` trap the `roles` tombstone note warns about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcLMsZRoR3daV3QzPXgwxK
…s "no rows" (#6966) The accumulator only exists because a `before*` dispatch created it, so an empty one means every id it was handed was null — "we do not know", not "nothing changed". Reading it as an empty row set would silently skip the cleanup entirely, which is the direction #4757 was filed for and the rule this module states for its resolve path. Adds direct unit cover for the accumulator: the union across rows without re-querying the predicate, dedup across the two subscribers that both stash on every row, the empty-union verdict, and the cap applied to the union. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcLMsZRoR3daV3QzPXgwxK
`HookDispatch` is a new public export, so the surface snapshot moves with it — 0 breaking, 1 added. Caught by CI's `check:api-surface`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcLMsZRoR3daV3QzPXgwxK
This was referenced Aug 9, 2026
os-zhuang
marked this pull request as ready for review
August 9, 2026 19:13
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 9, 2026
Contributor
Author
|
Queue steward — kicked from the merge queue at 19:13Z. NOT re-queued: this is a new signature. Read from the complete log archive, not the tail (SKILL note 7). Signature
The self-test passed and the gate's own Ledger verdictNo row in any of the four signature tables on #5810 matches ⇒ new signature ⇒ ⛔ this PR was not re-queued as-is. The queue currently holds zero entries; this PR is out of it and Preliminary reading (a reading, not a ruling — the diagnosis belongs to the lane)This has the shape of the "入队与落地 A" generated-artifact hazard, not a flake. Four readings:
Recommended action⛔ A re-run will not help. A re-run reuses the original merge ref (SKILL note 5); the regeneration is missing from that ref, so it will fail identically. This needs a new commit. Four-step per "入队与落地 A":
Scope and yield⛔ Steward authority is landing-only: no merge, no ready/draft flip, no re-queue, no code, no claim changes. Re-queueing after the fix is the lane PM's call. 让行 check: no lane comment on this PR in the last 30 minutes (latest comments are the Vercel and docs-drift bots, 15:51Z / 16:18Z) ⇒ no yield; no double-handling risk. Generated by Claude Code |
…main (#6966) Two generated artifacts move with the merge: - `export-origins/` is new on main (#4796) and had never seen `HookDispatch`. - `api-surface/` needed a real regeneration, not just my own entry. The merge of main into this branch produced a file byte-identical to THIS branch's side, silently dropping the five exports #7123 added on main (`SEARCH_VIRTUAL_TYPES`, `foldAsciiCase`, `asciiCaseInsensitiveContains`, `asciiCaseInsensitiveRegexSource`, `isVirtualSearchField`). These paths carry `merge=os-regen` in .gitattributes precisely so a merge regenerates rather than picks a side; it did not here, and `check:api-surface` is what caught it. Regenerating yields the union — both main's five and this branch's `HookDispatch`. Read the export-origins diff as its gate asks: one line, `HookDispatch` under `src/data/hook.zod.ts`, the same origin as `HookContext`. Not a re-home, not a new dual-source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcLMsZRoR3daV3QzPXgwxK
Contributor
Author
|
Dequeued on The failure was mine, not a batch-mate's (the queue ref was 1. 2. Merging
Also re-measured on the merged tree after rebuilding the closure: Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6966.
A predicate (
multi: true) write dispatches its lifecycle hooks once per matched row —after*since #5038,before*since #5574 (ADR-0058 Addendum II) — on a context deliberately indistinguishable from a single-id write's, so a handler written for one record works unchanged on a batch.That indistinguishability is the feature. It also erased the only signal several handlers had: before #5574 a bulk
before*fired once withinput.idpresent-but-undefined, so "input.idis empty" meant "this call stands for N rows". Every guard written on it silently inverted rather than failing — a per-row context has an id, so the guard now answers "single write" for every row of a batch.The marker, and how its shape was measured
Bound by the engine at every write dispatch site (insert, update, delete, both phases), at the point the dispatch ladder is decided. It has to come from the engine: the verdict is
isByIdWrite/isPredicatePath, which also consults driver capability (updateMany/deleteManypresence), andasScalarIdis deliberately unexported to stop the plugin side re-deriving it fromoptions.multi(#4434 / #4550).Each member earned its place against a consumer, not against taste:
moderowIndex?: number, absent ⇒ record) is indistinguishable from a hand-built context that carries no marker at all — andHookContextSchemais deliberately non-strict, so absent is a shape the parse accepts. Measured on the drivers infile-reference-lifecycle.test.ts, which build contexts by hand.indexindex === 0is how a handler does batch-scoped work oncemodealone the two live consumers cannot express "once per write". Measured: without it,plugin-sharing's boundedafterUpdatebranch is quadratic —affectedFrom(ctx)returns the whole union and is then recomputed once per row (1000 rows ⇒ 10⁶recomputeRowcalls), and the unbounded branch issues 1001 object-wide revokes where one is correct. Both are pinned.scopebefore*toafter*HookContext. Probed on the real engine before designing: a value set on a per-rowbeforeUpdate/beforeDeletecontext reads backundefinedon everyafter*context.sizeOptional, and an absent marker reads as "not a per-row dispatch" — the back-compatible direction. Reads carry no marker: a read has no fan-out. The claim that every dispatched context carries one is pinned as an invariant across all six write events, not just path by path — which is what makes
ctx.dispatch?.modesafe to read with no "did the engine bind it" branch, and which covers the one context a reader might worry about (update()/delete()keep a batch-scopedhookContexton the predicate path; no handler ever sees it).This is not the discriminator #5574 retired.
isPredicateBulkWritewas removed under ADR-0049 for having neither a producer nor a reachable consumer: it inferred "bulk" at the consumer frominput.id+options.multi, and answeredfalseeverywhere once every dispatch carried an id. This one is produced where the verdict is made and has readers.hook-wrappers.ts's retirement note stands unchanged; the contrast is written into the new JSDoc so a future reader does not mistake one for the other.Behaviour fixed
plugin-sharing— the card called this site "comments only". It is not.stashAffectedRowsparks the write's affected row set for theafter*half. On a predicate write that stash was landing on a per-row context theafterphase never sees, soreadAffectedRowsansweredunbounded/resolve-failedand both subscribers (rule recompute, record-share cascade) took their safe branch: every bulk update or delete on a ruled object revoked all of that object's rule grants and queued a full asynchronous re-grant — once per matched row, with the repeats racing each other's re-grants.Access is never widened (this is the ruling's "over-granting is an incident, under-granting is a wobble" direction), so it is not a fail-open — but it is a large silent behaviour change, and #4779's own tests could not see it because they drive a simulator modelling the pre-#5574 batch dispatch. A bounded write now takes the bounded path again: rows are unioned as the engine hands them over (no predicate re-query —
resolveAffectedRowsshort-circuits on a boundinput.id, so "resolve once and reuse" would have frozen the batch to row 0's id and under-revoked the rest, which is the fail-open direction), the cap still applies to the union, and theafter*work runs once per write.service-storage(site 1) — thebeforeDeleteid pre-resolution was dead on every path, and its stash could not have reached theafterphase anyway. The pre-resolution query is now gone entirely: the engine has already matched the doomed rows, sobeforeDeletecollects what it is handed andafterDeletereleases the batch in onesys_filelookup over an$in.beforeUpdatecopy-on-claim no longer runs once per row against a batch-scoped payload, which removes a row-conditioned rewrite of a sharedSETclause (out of contract under ADR-0058 Addendum II D3).Stale comments (site 3) corrected where they sit, in both packages.
Where I read the card differently
engine.findper batch". Re-enabling the old hook today would be worse than leaving it dead: per-rowafterDeletecontexts are spread copies of the batch context, so all N would see the full stashed id list and each release the whole set — N lookups over N ids, plus the extraengine.find(object, { where }). The fix therefore is not "restore the guard" but "collect what the engine already matched, release once".afterUpdateinfile-reference-lifecycle.tsis deliberately left running per row. Its guard readsids.length !== 1, which no longer means what its comment said — but the reconciliation it now performs is the safer outcome, and restoring the skip would leave a copied file owned by nobody while N records still reference it, which the sweep may collect. The genuine defect there (a predicate update gives N records one file id under an exclusive-ownership model) is older and wider than this card and belongs to service-storage — filed as A predicate update writing a file field gives N records one file id under an exclusive-ownership model #7102, and referenced from the comment.lint.ymlhas 48pnpm check:gates as the dispatch said; 52 across all workflow files.Verification
Reverse-verified: pre-fix sources restored with the new tests kept, predictions recorded first, then run.
objectqlservice-storageplugin-sharingOne deviation, and it strengthens the case: for "grants every matched row", I predicted the unbounded branch would leave the grant set empty; it actually left
['opp0','opp1']of three. The residue comes from the queued async re-grants interleaving with the repeated per-row revokes — so the pre-fix behaviour is not merely wasteful, it is racy within a single write.Green after the fix:
objectql2786 (+58 in the touched file),plugin-sharing414,service-storage324,spec9247.Gates run locally, all clean:
check:empty-changeset,check:changeset-gate-self-tests,check:kernel-hook-pairs,check:engine-double-contract,check:required-contexts,check:type-check-coverage,check:type-check-debt(needs the built closure first),check:published-files,check:query-options-erasure,check:startup-registry-verdict,check:strictness-ledger,check:docs, pluspnpm lintandpnpm typecheck(126/126 tasks).Three generated baselines move with the schema key and are committed:
packages/spec/authorable-surface/data.json,docs/audits/2026-07-unknown-key-strictness-ledger.counts.md, andcontent/docs/references/data/hook.mdx(the last was caught by CI'scheck:docs, not locally — my local sweep had missed it).Under the full parallel
pnpm test,@objectstack/types(node.test.tshost-app resolution) and four@objectstack/lintlazy-module-loading tests flake; each passes in isolation and via turbo (lint: 68 files / 1771 tests green), and neither package is touched by this diff.Refs: #6966, #5574, #5038, #6656, #4434, #4550, #4779, #7102, ADR-0049, ADR-0058 Addendum II.