Skip to content

docs(objectql): state the per-row before* contract in the two hook-wrapper docblocks - #18637

Merged
huangyiirene merged 3 commits into
mainfrom
claude/issue-18331-hook-wrappers-per-row-docblocks
Sep 17, 2026
Merged

huangyiirene merged 3 commits into
mainfrom
claude/issue-18331-hook-wrappers-per-row-docblocks

Conversation

@huangyiirene

Copy link
Copy Markdown
Collaborator

Fixes #18331

Clause-②: no

Two docblocks in packages/objectql/src/hook-wrappers.ts stated the retired batch model for the before* dispatch of a predicate (multi: true) bulk write in the present tense, while ruling #16074 / ADR-0058 Addendum II (D1/D2) and the engine beneath them both dispatch per matched row and bind that row's pre-image. Prose only: no behaviour change, no assertion change.

Session: https://claude.ai/code/session_01CqmCgU5RGDoJYhHUMVp2af

The two sites, located by symbol

site symbol retired sentence
1 the docblock attached to the exported hookRecordState (it opens "Choose the record-shaped object the condition formula evaluates against", and describes pickRecordPayload) "Its before* dispatch still fires once for the batch with no prior row, so that payload is left exactly as it is"
2 pickPreviousPayload, section "When it is ABSENT" "it fires ONCE for N matched rows, so there is no single prior record to bind; previous stays unbound"

What the code actually does, on this branch's base: dispatchPerRowBeforeHooks (engine.ts) loops the matched rows and builds each context with previous: coerceBooleanFields(schema, row); the call sites for beforeUpdate and beforeDelete both take that path whenever the object has hooks and at least one row matched. A live behavioural pin already asserts it — hook-condition-bulk-previous.test.ts, "the bulk write #5037 and #5038 both rejected now SUCCEEDS, firing per row", boots a real engine, runs multi: true with a previous.done != true transition condition on beforeUpdate, and asserts one dispatch per row.

R2 — does the text ship? Measured, and the two sites do NOT behave the same

Measured the way #17975 did, against the real published tarball (npm pack @objectstack/objectql@17.4.0), then again against a local build of this branch's final head.

  • Site 1 SHIPS. The retired sentence is present in the published tarball at dist/util-Dw5ZTIII.d.ts:8039 and the matching .d.mts. The reason is worth recording: hookRecordState has two leading comment blocks in the source — the detached pickRecordPayload docblock, then its own — and the dts bundler emits both verbatim onto declare function hookRecordState(...) (published .d.ts line 8061). That file is the shared chunk behind both the . and ./core entrypoints, so every consumer's editor surfaces the retired sentence on hover.
  • Site 2 does NOT ship. fires ONCE for N matched rows is absent from every file in the published tarball. pickPreviousPayload is module-private; it appears in dist/ only as an {@link} reference inside another docblock.
  • After this change, on the final head: site 1's replacement is in dist/util-DWbR4Mtj.d.ts:8223 + .d.mts, site 2's unique marker ("any update-shaped context") is in src once and in zero dist files, and neither fires once nor fires ONCE survives anywhere in dist/.

Because site 1 moves published text, this carries a patch changeset, matching the four in-repo precedents for prose repairs inside published packages and the sibling PR #18625. It adds no exported symbol, no key and no accepted value — the accept set was widened by PR #17249 in packages/spec, not here — hence Clause-②: no.

What the second site says instead, and why it does not name a dispatch

The natural replacement for site 2 was to name the one before* context that still carries no row — the opt-in dispatchUnscopedMultiWrite dispatch, fired once for a multi: true write with no caller predicate at all. That draft was withdrawn before pushing: wrapDeclarativeHook is applied at registration (hook-binder.ts:221), but the engine's own note records that no metadata binding can set that flag today, so naming it as an absence case this function observes would be a claim whose reachability cannot be shown. The sentence now states what is true by construction — any update-shaped context whose prior row is not in hand — and then says plainly that a predicate bulk write is no longer one of those.

Vocabulary is deliberately aligned with what PR #18625 landed in this package (per matched row, on the single-record shape, ADR-0058 Addendum II D1/D2) rather than inventing a second spelling. #18625's own ROW-INVARIANT-IN-EFFECT wording is about which rewrites are admitted, which neither of these two sites discusses, so it is not copied in as padding.

Tests and gates, on final head 0cd458243a

  • pnpm --filter @objectstack/objectql exec vitest run --maxWorkers=2299 test files, 4995 tests, all passed, lock verdict command-exit 0.
  • pnpm --filter @objectstack/objectql typecheck — green, including check:test-typecheck (40 files / 234 errors / 65 pinned signatures held in the shrink-only ledger, unchanged).
  • Dependency-closure build — turbo run build --filter=@objectstack/objectql 14/14 successful.
  • eslint . --no-inline-config over the whole repo: 6811 files, 0 errors, 0 warnings. No narrowing was needed, so no narrowing is claimed.
  • Gate families derived on that same sha with scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack: 54 derived, 54 run, 0 unrun (reconciled with --ran, each line carrying its exit code captured before any pipe). 51 green; the three non-zero ones are below.
gate exit reading
pnpm check:cross-package-test-inputs 1 Known gate defect, already filed as #18621 — it reds whenever packages/spec/dist/ exists, which the dependency-closure build creates. The flag is rooted in packages/cli/test/init-created-files-summary.e2e.test.ts descending packages/spec/dist/; neither path is in this diff. Named, not chased, and no twin card filed.
pnpm check:dual-build-cjs-loads 3 PREREQUISITE NOT MET, in the gate's own words: it reads built output and most packages have no dist/ in this worktree. Nothing was measured — recorded as NOT MEASURED, not as a pass and not as a red. CI's Build Core covers it.
node scripts/check-engine-split-ratio.mjs --days 90 2 The gate refuses to answer because this container's clone is shallow and its oldest visible commit sits inside the 90-day window. NOT MEASURED. It is an ADR-0076 D7 repo-history metric, orthogonal to a comment-only diff; CI checks out with the history it needs.

Acceptance notes

R3, judged occurrence by occurrence — both extra name-matches are false positives, and no edit is owed to either. Sweeping fires once / fires ONCE over packages/objectql/src returns 4 hits: the 2 repaired here, plus

  • hook-input-shape-contract.test.ts:136 — "before* now fires once PER MATCHED ROW (D1), so the count is the row count". This states the current contract, correctly, and the assertion under it (expect(seen).toHaveLength(2) for a two-row batch) pins the per-row model. Correct as written.
  • plugin-metadata-event-outage.test.ts:33 — "It would also fire once per event during an outage, where the boot line fires once per process". A statement about a degradation log line's emission frequency; unrelated to hook dispatch in every respect. Correct as written.

Neither file was touched. No follow-up card is owed for either on this measurement; the seat decides.

Under-count check inside the declared surface. The rest of hook-wrappers.ts mentions the batch model in five more places, and every one of them is already correct: the HookConditionError note ("the before* phase was dispatched ONCE for the whole batch") and the retired-helpers block are past-tense history, which is correct as history; "The batch-scoped hookContext still exists inside update()/delete(), but it is never handed to triggerHooks" is a true present-tense statement; and the unevaluableConditionError note already says "a predicate write dispatches before* once per matched row now". Only the two cited docblocks were defective, so the card's count is exact for this file.

Noted, not filed. The detached docblock shape at site 1 — a block describing pickRecordPayload left sitting above hookRecordState, which the dts bundler then publishes as if it were part of hookRecordState's own documentation — is an observation about layout, not a defect in any of the three filing classes: the text is accurate after this change and the emit is TypeScript behaving as documented. Successor that would meet it: any PR re-documenting these helpers. Recording it because it is why site 1 ships at all, which was not obvious from the source.

Out of surface, untouched: packages/objectql/CHANGELOG.md still carries the retired phrasing in released entries. That is a dated record of what was true then and is correctly left alone.


Generated by Claude Code

…ore* contract

`pickRecordPayload`'s docblock and `pickPreviousPayload`'s "when `previous` is
ABSENT" list both stated the retired batch model in the present tense: that a
predicate (`multi: true`) bulk write's `before*` dispatch fires once for the
batch with no prior row, so `previous` stays unbound.

ADR-0058 Addendum II (ruling #16074, clauses D1/D2) retired that. The engine
dispatches `before*` per matched row and binds that row's pre-image
(`dispatchPerRowBeforeHooks`, `previous: coerceBooleanFields(schema, row)`).
The only `before*` context that still names no row is the opt-in
`dispatchUnscopedMultiWrite` dispatch, fired once for a `multi: true` write
carrying no caller predicate at all — which is what the second docblock now
names.

Prose only: no behaviour change, no assertion change.

Claude-Session: https://claude.ai/code/session_01CqmCgU5RGDoJYhHUMVp2af
Co-authored-by: Claude <noreply@anthropic.com>
…ocks

Brings in PR #18625 (card #17975), the sibling prose repair in the same
contract family, so this branch's wording can be aligned with the spelling
that actually shipped.

Claude-Session: https://claude.ai/code/session_01CqmCgU5RGDoJYhHUMVp2af
Co-authored-by: Claude <noreply@anthropic.com>
…bulary, add the changeset

Follow-up to the same two sites after merging PR #18625 (card #17975), the
sibling prose repair that landed in this package.

- Both sites now use the package's shipped spelling for the dispatch model:
  per matched row, on the single-record shape, ADR-0058 Addendum II D1/D2.
- The second site no longer names the opt-in `dispatchUnscopedMultiWrite`
  dispatch. `wrapDeclarativeHook` is applied at registration
  (`hook-binder.ts:221`), but no metadata binding can set that flag today, so
  naming it as an absence case this function observes would be a claim that
  cannot be shown to be reachable. The truthful statement — any update-shaped
  context whose prior row is not in hand — is true by construction.
- Adds the `patch` changeset: the first docblock is emitted verbatim onto the
  exported `hookRecordState` declaration in the published `.d.ts`.

Prose only: no behaviour change, no assertion change.

Claude-Session: https://claude.ai/code/session_01CqmCgU5RGDoJYhHUMVp2af
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/s documentation Improvements or additions to documentation tooling labels Sep 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

⚠️ 1 changed file(s) yielded no anchor (packages/objectql/src/hook-wrappers.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files. Nothing else in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 1 changed package(s)).

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/objectql/src/hook-wrappers.ts) — pages documenting those are invisible to this run
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 17 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 25c9a8317bab7e3eea22a1468d928641de06161epackageMentionDocs.

@huangyiirene
huangyiirene marked this pull request as ready for review September 17, 2026 11:36
@huangyiirene
huangyiirene added this pull request to the merge queue Sep 17, 2026
Merged via the queue into main with commit 922c755 Sep 17, 2026
36 checks passed
@huangyiirene
huangyiirene deleted the claude/issue-18331-hook-wrappers-per-row-docblocks branch September 17, 2026 12:01
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/s tooling

Projects

None yet

2 participants