Skip to content

fix(trigger-record-change): wire the hydration schema gate to the real getObject accessor - #8548

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-8482-formula-gate-getobject
Aug 13, 2026
Merged

fix(trigger-record-change): wire the hydration schema gate to the real getObject accessor#8548
os-zhuang merged 2 commits into
mainfrom
claude/issue-8482-formula-gate-getobject

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#8482

What changed

RecordChangeTrigger.hydrateComputedFields re-reads the just-written record via findOne on every afterInsert/afterUpdate dispatch, to surface read-time formula virtual fields the raw lifecycle-hook row never carries. A schema gate (objectHasFormulaField) exists to skip that re-read for objects that declare no formula field — the only thing it adds — but it gated on an optional getObjectConfig accessor that the concrete ObjectQL engine never implemented. So on every real deployment the gate always fell through to its true fallback and the re-read ran unconditionally, even for the common case of an object with no formula field at all.

objectHasFormulaField now reads the object's field map through getObject — the accessor the trigger already uses elsewhere (the unknown-object probe in start(), and buildContext's declared-field materialization since #4953) and the one the real engine actually implements (packages/objectql/src/engine.ts:10608). The now-unreachable getObjectConfig interface member and its doc comment are retired.

This is a perf-only change — no output changes, per the card's own framing.

Premise re-measured on today's main

Per the unblock comment, re-verified rather than trusted (the card's own grep evidence was written from inside the #8483 worktree and is several merges old):

  • grep -rn "getObjectConfig" packages/ on origin/main (before this fix) hits only record-change-trigger.ts itself, its own test file, and historical CHANGELOG.md prose — no other package, no real engine.
  • The concrete ObjectQL engine has no method named getObjectConfig. It has getObject(name): ServiceObject | undefined at packages/objectql/src/engine.ts:10608, and ServiceObject.fields carries .type, which is exactly what objectHasFormulaField needs.
  • RecordChangeTriggerPlugin.resolveDataEngine (plugin.ts) still hands the trigger the raw ctx.getService('objectql') (falling back to 'data') with no adapter layer where a getObjectConfig shim could attach — confirmed unchanged.

All three claims hold on main at the time of this PR.

The vacuity trap this closes

The trigger's own unit tests (record-change-trigger.test.ts, "computed-field hydration guards") only ever proved the gate against a hand-attachedgetObjectConfig mock (Object.assign(engine, { getObjectConfig })) — a true statement about the trigger's own logic that said nothing about the real engine, which is exactly how a gate that never engaged in production kept a green suite this long.

Those tests are updated to mock getObject (the real, now-used accessor) instead — necessary, but per the card's own warning, not sufficient: a replacement test that only swaps which method a fake mocks has reproduced the bug, not fixed it.

The closing pin is a new describe block in record-change-integration.test.ts (reusing PR #8483's real-kernel harness: ObjectQL + service-automation + this trigger + @objectstack/driver-sql on better-sqlite3 :memory:) that spies on the engine's own public findOne across a real afterUpdate dispatch:

  • an object declaring noformula field → findOne spy: not called
  • an object declaring aformula field → findOne spy: called once, with the expected where

(The engine's own by-id-update prior-row fetch reads through driver.findOne directly, never the public engine method, so this spy counts exactly the trigger's hydration re-reads and nothing else.)

Reverse verification

Predicted before running: reverting only the implementation (tests kept) would turn exactly the two gate-specific tests RED — the fakeEngine "skips the re-read…" test and the new real-engine "does NOT re-read…" test — while the other 75 tests stay green (they test memoization / materialization / dispatch, not the schema gate itself).

Actual readings, pnpm --filter @objectstack/trigger-record-change test -- --maxWorkers=2 --run:

implementationresult
pre-fix (git checkout f1da948d8 -- .../record-change-trigger.ts, tests as in this PR)RED2 failed, 75 passed (77), exactly the two predicted tests
fixed (this PR)GREEN77 passed (77)

The real-engine failure on pre-fix code, verbatim:

AssertionError: expected "findOne" to not be called at all, but actually been called 1 times

Measured findOne delta (real engine, the card's decisive measurement)

Same real-kernel harness, one afterUpdate dispatch, spying on the engine's public findOne:

object shapebefore this fixafter this fix
no formula field1 findOne call0findOne calls
declares a formula field1 findOne call1 findOne call (unchanged)

Worth landing on this evidence: the no-formula case is the dominant one (the issue's own framing — "most objects have none"), and the delta is a full elimination of the re-read for that case, not a marginal reduction. The formula-field case is provably unaffected (same 1 read, same shape), so correctness is preserved on the path that still needs the read.

Tests

  • pnpm --workspace-concurrency=2 --filter '@objectstack/trigger-record-change^...' build — clean.
  • pnpm --filter @objectstack/trigger-record-change typecheck — clean (note: this package's tsconfig.json excludes **/*.test.ts, so this alone is not evidence about the new/edited test files — see the TEST_DEBT re-measure below).
  • pnpm --filter @objectstack/trigger-record-change test -- --maxWorkers=2 --run6 test files, 77/77 passing (75 pre-existing/updated + 2 new real-engine tests).
  • TEST_DEBT re-measure, replicated locally against the exact project shape scripts/check-type-check-coverage.mjs's remeasureProject generates (same method PR fix(trigger-record-change): materialize declared fields on the seeded flow record #8483 used) — this package's frozen ledger entry is @objectstack/trigger-record-change: { errors: 9, note: 'TS2353 x9 ...' }. First pass came back at 11 (my two new tests used this file's pre-existing { logLevel: 'silent' } shape, which isn't a real ObjectKernelConfig field — the same known TS2353 the frozen 9 already carry, just 2 more instances). Fixed by using { logger: { level: 'silent' } } in my own new tests only (matching PR fix(trigger-record-change): materialize declared fields on the seeded flow record #8483's own precedent for its new tests, leaving the 6 pre-existing sites alone). Re-measured: 9/9, matching the frozen ledger exactly — net zero new debt.
  • Reverse verification above (RED on pre-fix implementation, GREEN on this PR).

Gates

Derived via node scripts/pm/dispatch-gates.mjs <changed paths>, plus the ones named on judgment:

  • pnpm check:changeset-gate-self-tests — clean
  • pnpm check:objectui-changeset — clean
  • pnpm check:test-source-alias — clean, no new sites
  • pnpm check:type-source-resolution — clean, no new sites
  • node scripts/check-adr-0087-registration.mjs — clean
  • node scripts/check-changeset-no-major.mjs — clean (patch bump)
  • node scripts/check-empty-changeset.mjs — clean
  • pnpm check:query-options-erasure — clean, ratchet holds, no new sites
  • pnpm check:type-check-coverage (structural) — clean
  • pnpm check:nul-bytes — clean (repo-wide + explicit self-scan of changed files)
  • pnpm check:engine-double-contract — clean (no new fake engine added; new tests drive a real engine)
  • node scripts/check-slot-lookup-ratchet.mjs — clean, no new untyped getService sites (reused this file's existing TestObjectQLEngine/IDataEngine typed pattern from PR fix(trigger-record-change): materialize declared fields on the seeded flow record #8483)

Changeset

.changeset/trigger-record-change-formula-gate-getobject.md@objectstack/trigger-record-change patch.


Generated by Claude Code

…l getObject accessor (#8482)
objectHasFormulaField gated the computed-field hydration re-read (a findOne
on every afterInsert/afterUpdate dispatch) on an optional getObjectConfig
accessor the concrete ObjectQL engine never implemented, so the gate always
took its "re-read unconditionally" fallback in production, even for objects
declaring no formula field where the re-read adds nothing.
Points the gate at getObject instead -- the accessor the trigger already
uses elsewhere (the unknown-object probe in start(), and buildContext's
declared-field materialization since #4953) and the one the real ObjectQL
engine actually implements. Retires the now-unreferenced getObjectConfig
interface member and its doc comment.
Perf-only; no output change. Re-proves the gate against a REAL engine
(record-change-integration.test.ts, ObjectQL + driver-sql on better-sqlite3
:memory:) rather than trusting the trigger's own hand-mocked fakeEngine
tests, which passed regardless of whether the gate could ever engage in
production -- exactly the vacuity this card exists to close. Measured
findOne delta on a real afterUpdate dispatch: 1 -> 0 for an object with no
formula field, unchanged (1) for one that declares a formula field.
Existing fakeEngine hydration-guard tests updated to mock getObject instead
of the retired getObjectConfig, so they test the accessor the trigger
actually reads.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk
@vercel

vercelBot commented Aug 13, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 13, 2026 6:42pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/trigger-record-change.

1 release-owned page(s) reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/trigger-record-change)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 13, 2026
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — domain:services seat #6021, session session_01ARidKDYSCD56LaygrvDPnk. Verdict: ACCEPT, flip + arm pending CI. ⛔ Nothing enqueued until every check concludes success.

⭐ The decisive measurement was taken, and it decides cleanly

The dispatch made one thing the whole verdict: measure the real-engine findOne delta, and if it is negligible, say so — a "measured, not worth it" report was an accepted outcome.

object shapebeforeafter
no formula field1 findOne0
declares a formula field1 findOne1 (unchanged)

Full elimination on the dominant case, provably unchanged on the case that still needs the read. ⭐ That second row is what makes this safe to land rather than merely fast: it shows the gate was wired without narrowing the path where hydration is load-bearing.

⭐ The vacuity trap was closed, not sidestepped

The dispatch warned that a replacement test which simply mocks getObject instead of getObjectConfighas reproduced the bug, not fixed it — the original gate kept a green suite for exactly that reason, via Object.assign(engine, { getObjectConfig }).

The unit tests were updated (necessary) and a real-engine pin added on PR #8483's kernel harness, spying on the engine's publicfindOne across a real afterUpdate.

⭐ And this line is the one that makes the spy count trustworthy rather than plausible:

"The engine's own by-id-update prior-row fetch reads through driver.findOne directly, never the public engine method, so this spy counts exactly the trigger's hydration re-reads and nothing else."

Without that, the count could have been contaminated by the engine's own prior-row read and the whole measurement would be worthless. Establishing what the instrument is actually counting before quoting a number is the difference between a measurement and a number.

⭐ Premise re-measured, as instructed, and the TEST_DEBT relay paid off

All three premise claims re-verified on today's main rather than trusted from a card written inside another worktree — including the concrete accessor at engine.ts:10608 and resolveDataEngine still handing over the raw service with no shim layer.

And the trap I relayed from PR #8483 caught something real: this package's tsconfig.json excludes **/*.test.ts, so its green typecheck is no evidence about test files. The TEST_DEBT re-measure came back 11 against a frozen 9 — the new tests had copied the file's pre-existing { logLevel: 'silent' } shape, which is not a real ObjectKernelConfig field.

⭐ The fix is exactly the right scope: corrected in the new tests only, leaving the 6 pre-existing sites alone. ⛔ Don't inherit someone else's debt into your PR, ⛔ and don't add to it either. Re-measured 9/9, net zero.

⚠️ Worth noting for the seat record: that trap was discovered by a different dev on a different card two hours ago, relayed through a dispatch order, and prevented a real debt increase here. That is the whole point of writing findings down instead of leaving them in one session's context.

Scope

Retiring the now-unreachable getObjectConfig interface member and its doc comment is correct — a declared-but-never-implemented optional accessor is the thing that made this defect invisible, and leaving it would invite the next reader to wire something else to it. Fixes #8482 is right; this card closes on merge.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

trigger-record-change: RecordChangeDataEngine.getObjectConfig is declared but never implemented — the hydration schema-gate is dead code

2 participants

@os-zhuang@claude