Skip to content

Commit 0c666c3

Browse files
refactor(objectql): move the last 28 hand-series private docblocks out of engine.ts with the codemod's third pass (slice 2 of 2) (#30415)
<!-- claude-projects-attribution: {"github_login":"os-bill"} --> _Requested by **Bill** · [project thread](https://claude.ai/code/project/chan_016iMXWSZDzqWRWvWzdNo8wg?thread=cmsg_016iMXWSZDzqWRWvWzdNo8wg1KEKUvnWZ8oYQ388cWv3dF)_ Fixes #30053 Clause-②: no Metadata-protocol: none ## 维护者速读 - 改了什么:第二片,也是最后一片。用 #30341 里加的 `--hand-docs`(不带 `--modules`)在 `main` `38717d34` 上生成:其余 10 个手写模块的 28 块私有 docblock(858 行)从 `engine.ts` 搬到函数上。`engine.ts` 12,618 → 11,752 行。生成物未手改,脚本本片未改。另把 liveness 台账 `query.groupBy` 一行的证据改指 `engine-delete-and-aggregate.ts#aggregate`。 - 为什么改:维护者在 #30012 上说「我觉得文件还是太长,还有进一步的拆分优化空间吗」「可以后续立卡处理」「合并之后再继续」。两片合计搬走 64 块 1,787 行,`engine.ts` 从 13,550 行降到 11,752 行。 - 风险与回滚:只搬注释。撤销证明逐字节还原 base;公有 `.d.ts` 相等;objectql 测试 base 550 文件 / 10,967 = head;回滚就是 revert。 - 先看:① `engine.ts` 顶部被剪掉的 4 个 import 绑定;② `query.json` 的 `groupBy` 一行(第一片时 `searchFields` 的同类问题见 #30367 / #30368,这次 `check:liveness` 进了门禁清单,推前已过)。 - 两分钟验证:`node scripts/codemod-split-engine.mjs --hand-docs --verify --base 38717d3`,应看到 `28 moved (858 lines) across 10 module(s), 36 already moved at base`、`12618 -> 11752 lines`,退出码 0;`pnpm --filter @objectstack/spec check:liveness` 退出码 0。 - 需要你做的:改动共 1,875 行(未超 3,000 行线),本 PR 保持草稿,按席位安排落地。 ## What changes - **The generated slice.** `node scripts/codemod-split-engine.mjs --hand-docs --write` on `main` `38717d34`, unedited. With no `--modules` it moves every remaining private hand-series docblock; the 36 the first slice moved are skipped as already moved. The script is unchanged since #30341. - **Four import bindings pruned** from `engine.ts`, because a moved docblock's `{@link}` was their only reader (`noUnusedLocals` counts a link): `RELATION_FILTER_ID_CAP` (its whole `import { type … }` goes), `resolveFilterTokens`, and `collectInternalReadFields` / `collectCredentialFields` (their whole import goes). - **`packages/spec/liveness/query.json`, row `query.groupBy`.** The third leg cited `engine.ts#aggregate`; the body has lived in `engine-delete-and-aggregate.ts` since the codemod split, and the last mention of `groupBy` in `engine.ts`, a private docblock, moved in this slice. The leg now cites `engine-delete-and-aggregate.ts#aggregate`, and the note gains one dated sentence. Without it `check:liveness` reds exactly as it did after the first slice (#30367). - **Changeset.** `patch` on `@objectstack/objectql` and `@objectstack/spec` (the ledger ships in the spec package). ### What moved in this slice (10 modules, 28 docblocks, 858 lines) | Hand module | Docblocks | Lines | Before → after | |:--|--:|--:|--:| | `engine-driver-options.ts` | 6 | 148 | 403 → 550 | | `engine-hook-dispatch.ts` | 4 | 147 | 240 → 388 | | `engine-file-reference-hydration.ts` | 5 | 128 | 453 → 581 | | `engine-referential-integrity.ts` | 2 | 123 | 232 → 358 | | `engine-execute-transactions.ts` | 2 | 73 | 204 → 280 | | `engine-query-lowering.ts` | 3 | 69 | 178 → 249 | | `engine-realtime-publish.ts` | 2 | 66 | 518 → 587 | | `engine-count-aggregate.ts` | 1 | 41 | 209 → 254 | | `engine-validation-messages.ts` | 2 | 40 | 193 → 236 | | `engine-reference-name-visible.ts` | 1 | 23 | 238 → 265 | `packages/objectql/src/engine.ts`: 12,618 → 11,752 lines. ### After both slices All 64 private hand-series stubs that had a docblock now keep it beside their body. On the class stay: the 20 public stubs (their docblock is the contract), `validationMessageContext` (its function already carries a richer docblock; merging two is an edit, not a move) and `noteAdmittedValueShapeViolation` (no docblock). `--hand-docs` on this head plans 0 moves. ## Verification All at head `cc44c59f` unless stated. - **Proof.** `--hand-docs --verify --base 38717d3`: exit 0. `28 moved (858 lines) across 10 module(s), 36 already moved at base; engine.ts 12618 -> 11752 lines; undoing every move rebuilds the base, undone the same way, for engine.ts and its 21 hand module(s) byte-for-byte, once the 4 import binding(s) only a moved docblock linked are pruned`. - **Determinism.** The same `--write` on a stale checkout of `main` (`f672e0dc`, the same `engine.ts` and hand modules) printed the same output tree sha256 `6c71d47c…7b38` as the run on `38717d34`. - **`.d.ts`.** `packages/objectql/dist` built at base `38717d34` twice and at head twice. Base 1 = base 2 and head 1 = head 2, byte for byte. Base against head: `core` and `index` (`.d.ts` and `.d.mts`) are byte-identical; `util-HASH` has 193/193 statements and 334/334 private members, and its public surface is **EQUAL**. The raw difference is the 858 lines of moved private docblocks. - **Ablation (on the real tree).** Through `scripts/ablation-replace.mjs`, each restored to the HEAD blob: one character changed in a moved docblock (`inAfterAbortUnit`'s, in `engine-hook-dispatch.ts`) → `--verify` exits 1 naming the line; one character changed in a docblock that stays on the class (`checkDriversHealth`) → exits 1 naming the line. The battery's own ablations ran on #30341 and the script is unchanged; `--self-test` 105 cases pass. - **Suites.** objectql `test`: base `38717d34` 550 files / 10,967 passed, head 550 / 10,967. objectql `typecheck`: exit 0. - **Liveness.** `pnpm --filter @objectstack/spec check:liveness`: exit 1 before the `groupBy` repoint (only that row), exit 0 after. Separately, all 36 ledger rows that cite `engine.ts` were read: each one's key (camelCase or snake_case) is still named in `engine.ts` on this head. - **Gates.** `dispatch-gates --commands` derived 62 commands (`check:liveness` among them), each run after a full `turbo run build --filter='!@objectstack/docs'` (exit 0). All 62 exit 0; `--ran`: 62 derived, 62 run, 0 NOT-MEASURED. ## Acceptance notes - As in the first slice, `{@link}`s inside a moved docblock now sit in the hand module and are not rewritten. - The `protocol.ts` half of #30053 (157 stubs, about 4,000 lines) is not in this card's claim. The seat files it separately once its own serial constraints clear. ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. (a ledger row only) - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran. - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. (the designated thread branch, recreated from `main`; second of two slices) - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: its generated artifacts are regenerated and committed. (a liveness row; nothing generated reads it) - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Utp6CYHBS7P5hkUQrNHkHZ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8fa738f commit 0c666c3

13 files changed

Lines changed: 951 additions & 924 deletions
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/objectql': patch
3+
'@objectstack/spec': patch
4+
---
5+
6+
refactor(objectql): the private docblocks of the last 28 hand-series stubs in `engine.ts` move into their hand modules, onto the functions that hold their bodies; no behaviour or public declaration changes
7+
8+
Clause-②: no
9+
Metadata-protocol: none
10+
11+
- The second and last slice of the `engine.ts` codemod's `--hand-docs` pass: 10 hand modules, 858 docblock lines. With the first slice, every private hand-series stub that had a docblock now keeps it beside its body; public and protected docblocks stay on the class. `engine.ts` goes from 12,618 to 11,752 lines.
12+
- The liveness ledger's `query.groupBy` row now cites `engine-delete-and-aggregate.ts#aggregate`, where the body lives, because `engine.ts` no longer names the key.
13+
- Nothing to do on upgrade: the exported names, the public `.d.ts` surface and what every method does are the same.

‎packages/objectql/src/engine-count-aggregate.ts‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,11 @@
4848
* `rejectCredentialAggregation` was not that package's first load. The
4949
* published build bundles both files into one, where the cycle does not
5050
* exist.
51+
*
52+
* [#30053 codemod] A PRIVATE method's docblock has since moved here,
53+
* onto its function (`--hand-docs` of `scripts/codemod-split-engine.mjs`),
54+
* and the class keeps none for it; a public or protected method's docblock
55+
* stays on the class, the contract, and its function keeps the placeholder.
5156
*/
5257

5358
import type { EngineAggregateOptions, EngineCountOptions, QueryAST } from '@objectstack/spec/data';
@@ -162,7 +167,47 @@ export async function count(
162167
return opCtx.result as number;
163168
}
164169

165-
/** The body of `ObjectQL#rejectCredentialAggregation`; the method's docblock there is its contract. */
170+
/**
171+
* Fail-closed guard (ADR-0100 / #3171 / #7922): refuse to aggregate over a
172+
* field whose value is withheld on the generic read path. Such fields reach
173+
* this guard through **two independent collectors**, and it needs both:
174+
*
175+
* - {@link collectCredentialFields} — keyed by field TYPE (`secret` /
176+
* `password`). A GROUP BY / MIN / MAX / array_agg over such a column would
177+
* surface the stored `secret:<id>` ref or the password value.
178+
* - {@link collectInternalReadFields} — keyed by the `internal: true` FLAG
179+
* (#7728). ADR-0100's third credential channel is a one-way hash living in
180+
* an ordinary `text` column, which no type-keyed collector can ever reach;
181+
* the flag is that channel's opt-in declaration. Without this half the
182+
* guard had the same type-vs-flag blind spot #7728 fixed on the read path:
183+
* a flagged column was omitted from `find`/`findOne` yet freely groupable
184+
* here, so the flag's promise ("never returned on the generic data path")
185+
* stopped at the edge of `aggregate()`.
186+
*
187+
* Post-hoc masking is not available on this path — the value is already a
188+
* group key by the time there is a row, and masking group keys corrupts the
189+
* result. So we reject instead.
190+
*
191+
* Neither collector carries a `managedBy` exemption, so the union does not
192+
* acquire one, deliberately. Read-masking exempts `password` on better-auth
193+
* objects so login reads still see the stored value; *aggregating* a
194+
* credential is never legitimate, least of all on an identity table, where it
195+
* is an inference oracle over hashes.
196+
*
197+
* Only the two output-bearing positions on `EngineAggregateOptions` carry
198+
* field names: `aggregations[].field` (skip COUNT(*) — undefined or '*') and
199+
* `groupBy[]` (a string, or a `{ field }` bucket object).
200+
*
201+
* The refusal carries the ADR-0112 envelope — `INVALID_FIELD` / 400, located
202+
* at the object and the field(s) named. It used to be a bare `Error` with no
203+
* `code` or `status`, so a 400-class author refusal reached the data door
204+
* 500-shaped (`INTERNAL_ERROR`, an undeclared fault). The code matches the
205+
* analytics door's own refusal of an `internal: true` member and the engine's
206+
* structured-JSON group-by refusal: the verdict is about the NAME being a
207+
* field that is never returned on a generic data exit, which `INVALID_FIELD`
208+
* is the catalog member for. The envelope now binds the `secret` / `password`
209+
* case too, which shared the bare `Error`.
210+
*/
166211
export function rejectCredentialAggregation(
167212
this: Pick<CountAggregateHost, '_registry'>,
168213
object: string,

‎packages/objectql/src/engine-driver-options.ts‎

Lines changed: 153 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,11 @@
5656
* This module imports the engine only as a TYPE, so there is no module cycle
5757
* at run time, and nothing re-exports it, so the published surface does not
5858
* move.
59+
*
60+
* [#30053 codemod] A PRIVATE method's docblock has since moved here,
61+
* onto its function (`--hand-docs` of `scripts/codemod-split-engine.mjs`),
62+
* and the class keeps none for it; a public or protected method's docblock
63+
* stays on the class, the contract, and its function keeps the placeholder.
5964
*/
6065

6166
import { DriverOptionsSchema, isTenancyDisabled } from '@objectstack/spec/data';
@@ -189,7 +194,96 @@ export function mergeDriverOptionsIntoHookView(
189194
return addsNothing ? callerBag : { ...bag, ...driverOptions };
190195
}
191196

192-
/** The body of `ObjectQL#buildDriverOptions`; the method's docblock there is its contract. */
197+
/**
198+
* Build the DriverOptions blob passed to every IDataDriver call.
199+
*
200+
* # The tenant read scope is the wall's verdict (ADR-0131 D8)
201+
*
202+
* The driver's native tenant scope — `tenantId` (equality) and `tenantIds`
203+
* (the `group` union), which `applyTenantScope` compiles — is the Layer 0
204+
* verdict projected onto `DriverOptions`, never a second derivation beside
205+
* it. One function decides ({@link computeTenantLayer0Verdict},
206+
* `@objectstack/core`), and the engine reads it from one of two places:
207+
*
208+
* - **the verdict the wall RECORDED on this operation**
209+
* ({@link OperationContext.tenantLayer0Verdict}, passed in as
210+
* `scope.recordedVerdict`) — present where the security layer composed its
211+
* wall onto the predicate: a read, or a predicate write. It already carries
212+
* what only that layer can know (the exempt `PLATFORM_ADMIN` on a
213+
* posture-permitting object, the verified platform owner's read, the
214+
* delegator's intersection), so the driver reaches exactly the rows Layer 0
215+
* admits;
216+
* - **otherwise, the same function over the engine's own facts** — the
217+
* posture in force ({@link resolveEnginePosture}), the context's active
218+
* organization and access set, the object's tenancy — with no admin
219+
* exemption, since the engine holds no permission sets. That covers a
220+
* system context, a by-id write, and a composition with no security layer
221+
* mounted.
222+
*
223+
* Projected: `none` (the `single` posture; a non-tenant, tenancy-disabled or
224+
* federated object; an exempt platform admin) → no scope; `organization` →
225+
* `tenantId`; `organizations` → `tenantIds`, with the active organization as
226+
* `tenantId` (the driver keys its scope on `tenantId` being present); `deny`
227+
* → the active organization if there is one — the narrowest scope a driver
228+
* can express, and where the wall ran its deny sentinel is already in the
229+
* predicate. Under `single` the driver therefore no longer narrows a read to
230+
* the active organization: the posture is one tenant, and isolation inside it
231+
* was never the driver's to add (ADR-0131 D11).
232+
*
233+
* # The write target is not a read scope
234+
*
235+
* An insert (`scope.writeTarget`) keeps the active organization as
236+
* `tenantId` in EVERY posture — `injectTenantOnInsert` stamps the row with
237+
* it, the counter scopes by it, and an upsert's identity probe refuses a
238+
* merge into another organization's row by it — and the `group` membership
239+
* set beside it, unchanged.
240+
*
241+
* # Withheld for two object postures
242+
*
243+
* 1. **`tenancy.enabled: false`** (ADR-0066 platform-global posture, e.g.
244+
* `sys_license`): stamping the caller's active-org tenantId there would
245+
* org-scope a global catalog at the driver, and its NULL-org rows would
246+
* vanish for authenticated org-context reads while anonymous reads still
247+
* see them (#3249).
248+
*
249+
* 2. **`external != null`** — a federated object (ADR-0015), whose schema
250+
* is owned by the REMOTE database (#7738). The driver turns `tenantId`
251+
* into `organization_id = :tenant`, and
252+
* against a remote table that carries no such column that is a SQL error
253+
* on Postgres/MySQL — or worse on SQLite, whose quoted-identifier
254+
* fallback reinterprets the unresolvable identifier as the string
255+
* literal `'organization_id'`, makes both disjuncts constant-false, and
256+
* answers **0 rows with HTTP 200**: a correctly-bound external object
257+
* silently reads empty.
258+
*
259+
* Note the reason is NOT "the remote happens to lack the column". The
260+
* column the driver detects is the platform's OWN: `applySystemFields`
261+
* (`resolveInjectedSystemColumns`) injects `organization_id` into every
262+
* object it registers, with no `external` branch, and
263+
* `SqlDriver.registerExternalObject` is DDL-free by design and runs no
264+
* introspection — so it computes the tenant column from the platform's
265+
* field set, never from the remote's. On a federated object
266+
* `organization_id`'s presence is therefore always the injection and
267+
* never evidence about the remote, which leaves the engine no ground on
268+
* which to scope by it. Tenant isolation for federated data belongs to
269+
* the remote and to the layers above (RBAC/RLS, the datasource binding),
270+
* not to a predicate the platform guesses onto someone else's table.
271+
*
272+
* # Declared keys only
273+
*
274+
* The result holds the keys `DriverOptionsSchema` declares and nothing else
275+
* (`pickDeclaredDriverOptions`, `engine-driver-options.ts`). `base` is
276+
* usually a caller's whole option bag — the query, the write-observability
277+
* callbacks and the other engine-level keys ride in it — and only its
278+
* declared keys are kept: an explicitly-passed `base.tenantId` /
279+
* `base.transaction` / `base.bypassTenantAudit` is deliberate caller intent
280+
* and still wins, under every verdict; everything else in the bag is the
281+
* engine's, never a driver's. That is why a verb hands its driver THIS
282+
* object and keeps `input.options` as the hooks' additive view
283+
* (`mergeDriverOptionsIntoHookView`). System / isSystem callers may still
284+
* cross tenants by clearing `tenantId` themselves on the resulting object;
285+
* this helper does not mask the system path.
286+
*/
193287
export function buildDriverOptions(
194288
this: Pick<DriverOptionsHost, 'isClosedUnitHandle' | 'txStore' | 'transactionCoversDriverFor' | '_registry' | 'driverWriteTarget' | 'driverReadScope' | 'wallExemptedOperation'>,
195289
object: string,
@@ -315,7 +409,16 @@ export function buildDriverOptions(
315409
return opts;
316410
}
317411

318-
/** The body of `ObjectQL#driverReadScope`; the method's docblock there is its contract. */
412+
/**
413+
* [ADR-0131 D8] The driver's native read scope for one operation — the Layer
414+
* 0 verdict projected onto `DriverOptions` (see {@link buildDriverOptions}).
415+
* `undefined` is "no scope"; the caller has already excluded a
416+
* tenancy-disabled or federated object.
417+
*
418+
* A recorded verdict that does not parse as `TenantLayer0VerdictSchema`
419+
* reads as absent — never as an organization — and the engine then decides
420+
* over its own facts, which carry no admin exemption: fail toward the wall.
421+
*/
319422
export function driverReadScope(
320423
this: Pick<DriverOptionsHost, 'engineLayer0Verdict'>,
321424
execCtx: ExecutionContext | undefined,
@@ -339,7 +442,11 @@ export function driverReadScope(
339442
}
340443
}
341444

342-
/** The body of `ObjectQL#engineLayer0Verdict`; the method's docblock there is its contract. */
445+
/**
446+
* The Layer 0 verdict over the engine's OWN facts — what {@link driverReadScope}
447+
* falls back to where no wall was recorded. The one ladder,
448+
* `computeTenantLayer0Verdict`, with the exemption's facts withheld.
449+
*/
343450
export function engineLayer0Verdict(
344451
this: Pick<DriverOptionsHost, 'resolveEnginePosture'>,
345452
execCtx: ExecutionContext | undefined,
@@ -362,7 +469,22 @@ export function engineLayer0Verdict(
362469
});
363470
}
364471

365-
/** The body of `ObjectQL#wallExemptedOperation`; the method's docblock there is its contract. */
472+
/**
473+
* [ADR-0131 D8 · 8c-1] Did the wall's EXEMPTION lift this operation's scope?
474+
* True when the recorded verdict is `none` while the same ladder over the
475+
* engine's own facts — every rung but the exemption — would have walled it.
476+
* That is plugin-security's own `crossedByPlatformAdmin` test (the ladder
477+
* asked a second time with only the exemption withheld), with the engine's
478+
* existing fallback ({@link engineLayer0Verdict}, what an unrecorded
479+
* operation is scoped by) as the second asking: no rung is computed beside
480+
* the one ladder, and a `single` posture answers `none` both ways, so it is
481+
* no crossing. The #12974 owner bypass records `none` on READS only, where
482+
* the declaration is inert.
483+
*
484+
* Absence is not exemption: a by-id write, a system context or a stack with
485+
* no security layer records nothing, and answers false. A recorded value
486+
* that does not parse answers false too — junk never lifts anything.
487+
*/
366488
export function wallExemptedOperation(
367489
this: Pick<DriverOptionsHost, 'engineLayer0Verdict'>,
368490
execCtx: ExecutionContext | undefined,
@@ -378,7 +500,12 @@ export function wallExemptedOperation(
378500
return this.engineLayer0Verdict(execCtx, activeOrganization).kind !== 'none';
379501
}
380502

381-
/** The body of `ObjectQL#driverWriteTarget`; the method's docblock there is its contract. */
503+
/**
504+
* The write target of an insert or upsert: the active organization, in every
505+
* posture, with the `group` membership set beside it — the scope the
506+
* driver's insert readback and upsert identity probe narrow by (ADR-0105 D5:
507+
* the active organization is where a write lands).
508+
*/
382509
export function driverWriteTarget(
383510
this: Pick<DriverOptionsHost, 'resolveEnginePosture'>,
384511
execCtx: ExecutionContext | undefined,
@@ -389,7 +516,27 @@ export function driverWriteTarget(
389516
return { tenantId: execCtx.tenantId as string, ...(tenantIds ? { tenantIds: [...tenantIds] } : {}) };
390517
}
391518

392-
/** The body of `ObjectQL#resolveEnginePosture`; the method's docblock there is its contract. */
519+
/**
520+
* [#8844] The tenancy posture IN FORCE for this engine, by the same two-step
521+
* `app-plugin.organizationWallActive` uses and for the same reason: the
522+
* injected provider is the `tenancy` service's answer (what is actually
523+
* ENFORCED, degraded boots included), and the env resolution is what the
524+
* operator ASKED for — the best fact available to a lean embedding that
525+
* mounts no SecurityPlugin.
526+
*
527+
* ⛔ Never `resolveMultiOrgEnabled()`: ADR-0105 D1 demoted that boolean to a
528+
* back-compat INPUT of `resolveTenancyPosture()`, so it reads `false` on a
529+
* deployment configured the documented way and would put a fully walled
530+
* install on the single-tenant branch below — the third recurrence of that
531+
* shape is what `app-plugin` documents.
532+
*
533+
* Read live per call, never cached: nothing here may freeze a verdict a later
534+
* boot phase can still change. `resolveTenancyPosture()` THROWS on an
535+
* unrecognized `OS_TENANCY_POSTURE` (deliberately — a typo'd posture must not
536+
* silently remove the wall); a write is not the place that refusal belongs,
537+
* and an unreadable posture is exactly the case where guessing `single` would
538+
* be worst, so it degrades to the walled reading and the write is refused.
539+
*/
393540
export function resolveEnginePosture(
394541
this: Pick<DriverOptionsHost, 'tenancyPostureProvider'>,
395542
): TenancyPosture {

0 commit comments

Comments
 (0)