feat(models): @embed directive + write-time hook + auto-HNSW (Phase 5 of #510) - #747
Conversation
…hase 5 of #510) Schema-level RAG without app code: marking a field with `@embed(source: "<field>", model: "<logical-name>")` makes Harper compute the embedding on every originating write, store it on the record, and index it via HNSW — no application hooks required. Parser (resources/graphql.ts): - 'embed' added to knownGraphQLDirectives; 'Vector' added to PRIMITIVE_TYPES (with a coerceType case in Table.ts that accepts Float32Array / Array<number> / typed-array views). - `@embed` populates `property.embed = { source, model }` and sets `property.version = "embed:<model>"` so the existing schema-load version-change-driven reindex pathway (databases.ts:1111) fires on model swap. (Note: today that pathway re-indexes existing vectors but does NOT re-embed source fields through the new model; full re-embed backfill is a follow-up.) - Auto-attaches `property.indexed = { type: 'HNSW' }` when no explicit `@indexed` is present — the issue's "no app code needed" statement only holds with auto-indexing. - Validates both `source:` and `model:` are present and string-typed; rejects variables / non-string nodes with a directive-location error so typoed schemas fail loudly instead of silently producing `undefined`. API + write-time hook (resources/Table.ts): - Static `userEmbedders` map and `embedAttributes` filtered list (the list is recomputed in `updatedAttributes()` so in-place schema reload at `databases.ts:940` doesn't leave a stale snapshot). - `Table.setEmbedAttribute(name, embedder)` static method mirrors the `setComputedAttribute` registration pattern; component authors override the default via this surface. - Default embedder auto-registered at schema load when an attribute carries `@embed` — sits OUTSIDE the if/else-if resolver chain so `@embed`-decorated fields still get their `customIndex.propertyResolver` wired for HNSW. - `buildEmbedBefore` chained into the existing `preCommitBlobsForRecordBefore` slot at the main put/patch site — the embedder mutates the recordUpdate during the transaction's `before` phase, so commit blocks on it (sync-by-default; queued mode deferred to a follow-up). Default embedder + replicated-write predicate (resources/models/embedHook.ts): - `createDefaultEmbedder({source, model})` returns a callback that calls `Models.embed(record[source], { model, inputType: 'document' })` and returns the first vector. `Models` is lazy-required so the module is unit-testable without dragging the transaction stack into module load. - `buildEmbedBefore` skips on ALL three replication-receiver signals: `options.isNotification === true` (cluster-subscribe path in Table.ts:325-373), `context.replicateFrom === false` (REST `x-replicate-from: none`), and `context.alreadyLogged === true` (local audit replay at replayLogs.ts:52). On receivers the originator's vector is stored as-is, preserving cross-cluster embedding-space consistency. - Embedder errors are caught and rethrown as a sanitized message `Failed to compute embedding for attribute "<name>"` — backend URLs, model IDs, and API-key tails stay in server logs only. Tracking: #510 Closes #632 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
17 tests covering createDefaultEmbedder + buildEmbedBefore:
- default embedder: source-field read, document inputType, null/undefined
source handling, non-string source stringification.
- buildEmbedBefore replication predicate: skip on
`options.isNotification === true` (cluster-subscribe),
`context.replicateFrom === false` (REST suppression),
`context.alreadyLogged === true` (replay); fire on
local-originating writes where replicateFrom is undefined.
- source-field semantics: skip when source not in payload (PATCH
that omits source survives via patch-merge), clear embedding to
null when source is explicitly null, run when source is present.
- per-attribute correctness: skip attributes whose source is
absent in multi-`@embed` tables; skip attributes with no
registered embedder; write null when embedder returns null.
- error sanitization: backend errors (URLs, API-key tails) are
NOT propagated to the caller; the sanitized message references
only the attribute name.
Test seam `__setEmbedFnForTest` lets the suite drive the embedder
without instantiating Harper's full transaction stack.
Tracking: #510
Refs #632
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Spins up a fake Ollama HTTP server inside the test, points Harper's models.embedding.default config at it, deploys a schema with @embed, and exercises three paths end-to-end. Replaces the "manual smoke test" bullet that was on the PR description with a CI-runnable test. Covered paths: 1. Happy path — POST a record → fake-ollama returns a deterministic 3-element Float32 vector → record stores it at the @embed-decorated field. Confirms exact-byte equality between the fake's response and what's persisted. 2. Source-unchanged PATCH — PATCH a record with a non-source field → fake-ollama call count stays flat; the existing embedding survives via patch-merge. 3. Replication-receiver skip (REST path) — POST with x-replicate-from: none header and a pre-supplied vector → no embed call is made; the supplied vector is stored as-is. The cluster-subscribe replication path (options.isNotification === true) is already covered by the embedHook unit tests. The fake-ollama server tracks call count + last inputs, so we assert not just on "vector got written" but on "embedder fired exactly when we expect, and not when we don't". The fake matches Ollama's actual /api/embed wire format (POST { model, input: string[] } → { embeddings: number[][] }) so the real OllamaBackend code path runs unchanged. Local note: macOS dev machines without `harper-integration-test-setup-loopback` can run via `HARPER_INTEGRATION_TEST_FORCE_LOOPBACK=1` to bypass the loopback address pool. CI uses the pool natively. Refs #632 #510 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reviewed; no blockers found. |
…s commit
Three bugs surfaced once the new integration test exercised the full REST
write path against a fake-Ollama HTTP server. All three were fixed in this
patch; the integration test now passes 4/4 locally and exercises the bugs
that would otherwise have shipped silent.
1) `require('./Models.ts')` doesn't survive the dist build (the `.ts`
extension stays literal at runtime; production resolves from
`dist/*.js`). Switched to `require('#src/resources/models/Models')`
which goes through package.json conditional exports — resolves to
`.ts` under the `typestrip` condition (unit tests) and to `dist/*.js`
in production.
2) The embedder was wired into the pre-commit `before` slot, which is
awaited at `Promise.all(completions)` at txn-commit time — AFTER each
write's `commit(...)` closure has already stored the record. That
pattern works for blob byte-writes (which reference pre-allocated
blob IDs) but not for `@embed` where the vector itself must be on
the record at commit time. Moved the embedder call to fire BEFORE
`transaction.addWrite(...)` and threaded the resulting promise back
through `_writeUpdate` → `create()` / `update()` via `when()` so the
caller chain (and ultimately the static-create transaction wrapper)
awaits the embedding before committing.
3) The default embedder returned a `Float32Array`, but Harper's record
encoder mangles typed arrays via `updateAndFreeze` (it enumerates
them as `{0,1,2,...}` maps, breaking the byte round-trip). The
integration test confirmed: a record POSTed with a plain
`Array<number>` round-trips correctly; one with a `Float32Array`
came back as zero-bytes. Changed the default embedder to return
`Array<number>` (HNSW's `propertyResolver` and `customIndex.index`
both accept it). Unit test updated to assert the Array shape.
Integration test (`integrationTests/server/embed-directive.test.ts`)
exercises:
- happy path: POST → embedder runs → vector stored, decoded back
- source-unchanged PATCH: no new embed call; existing vector survives
- REST `x-replicate-from: none`: embedder skipped, supplied vector stored
Refs #632 #510
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two real blockers from PR #747 review: 1) Default embedder not refreshed on model change. The `!this.userEmbedders[attribute.name]` guard prevented refreshing the auto-generated default embedder when a schema reload via `databases.ts:940` in-place `attributes.splice()` calls `updatedAttributes()` again on the existing Table class — `userEmbedders` carried over, so a `@embed(model: "B")` swap from `"A"` left writes silently using the old model. Track user-set override names in `static userSetEmbedders: Set<string>` and only skip refresh for those; defaults are refreshed every time. `setEmbedAttribute` populates the set. 2) `ArrayBuffer.isView` branch reinterpreted bytes. `new Float32Array(value.buffer)` copies the raw bytes of the backing ArrayBuffer from offset 0 — when `value` is a `Float64Array`, the 8-byte-per-element encoding gets reinterpreted as 4-byte float32s (garbage). Same issue with typed-array subarrays (nonzero `byteOffset`). Use `Float32Array.from(value)` to iterate element values and convert numerically. Both flagged inline by claude-bot on `resources/Table.ts:3433` and `resources/Table.ts:4822`. Integration test still passes 4/4, unit tests 17/17. Refs #632 #510 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
left a comment
There was a problem hiding this comment.
It seems like we are entirely missing support for embeddings with getFromSource writes. And using embeddings with caching tables actually seems like one of the most compelling uses cases.
My Gemini review caught a number of things:
1. Unhandled Promise in TableResource.update leads to Transaction Commits before Embed resolves — CRITICAL (Confirmed)
File: Table.ts
Domain: concurrency
What: The update(updates, fullUpdate) instance method of TableResource calls this._writeUpdate(...) which returns a Promise when @embed is active. However, update() completely ignores this promise and returns this
synchronously.
Why it matters: Because update() returns synchronously, the calling application will immediately proceed to run await instance.save() . However, the asynchronous embedder operation is still pending, meaning the write has not been
registered in the transaction's write list yet. The transaction will either commit without saving the updated record (causing silent data loss) or crash with a "Transaction already closed" error when the embedder finally resolves and
tries to invoke addWrite() on a committed transaction.
Code:
// lines 1243-1247
return when(loading, () => {
this.#changes = updates;
this._writeUpdate(id, this.#changes, false);
return this;
});
...
// line 1251-1252
this._writeUpdate(id, this.#changes, fullUpdate);
return this;
Suggested fix:
Wrap both invocations of _writeUpdate inside the update method with Harper's when() helper so that the promise is properly propagated back to the caller when an asynchronous hook is active:
return when(loading, () => {
this.#changes = updates;
return when(this._writeUpdate(id, this.#changes, false), () => this);
});
...
return when(this._writeUpdate(id, this.#changes, fullUpdate), () => this);
──────
2. coerceType for Vector returns Float32Array which is mangled by the Record Encoder — HIGH (Confirmed)
File: Table.ts
Domain: data-integrity
What: coerceType converts incoming Vector attribute values into a Float32Array . However, as documented in createDefaultEmbedder , Harper's record encoder ( updateAndFreeze ) does not cleanly serialize typed arrays, instead
mangling them into {0: val1, 1: val2, ...} objects in the database.
Why it matters: While the default embedder circumvents this by casting its result back to Array , any user-supplied vectors (e.g., replication receiver paths, programmatic writes, or manual API uploads) will pass through
coerceType and be coerced to a Float32Array . These will get stored in the database in a mangled object format, corrupting the vector column and breaking subsequent HNSW indexing and search operations.
Code:
case 'Vector':
if (value === null || value === 'null') return null;
if (value instanceof Float32Array) return value;
if (Array.isArray(value)) return Float32Array.from(value);
if (ArrayBuffer.isView(value)) return Float32Array.from(value as any);
throw new SyntaxError();
Suggested fix:
Coerce vectors to standard JavaScript arrays ( number[] ) instead of Float32Array s, ensuring they bypass record-encoder mangling while remaining compatible with HNSW:
case 'Vector':
if (value === null || value === 'null') return null;
if (value instanceof Float32Array) return Array.from(value);
if (Array.isArray(value)) return value.map(Number);
if (ArrayBuffer.isView(value)) return Array.from(value as any);
throw new SyntaxError();
──────
3. Model Change via Schema Reload causes Dimension Mismatches or Mixed-Model Vectors in HNSW — MEDIUM (Confirmed)
File: graphql.ts and embedHook.ts (comments)
Domain: data-integrity / config
What: Modifying the @embed(model: "...") argument in a schema file updates the attribute version, triggering a schema reload and an HNSW reindex. However, existing rows are not automatically re-embedded through the new model.
Why it matters: If a user changes their embedding model (e.g., swapping a 384-dimension model for a 1536-dimension model), existing rows retain their old-dimension vectors. Loading these mixed-dimension vectors into the same HNSW
index
will either trigger a crash during index construction or severely pollute the embedding space, returning garbage query results.
Suggested fix:
- Document this operational hazard explicitly.
- Consider adding validation to block schema updates that change model definitions on populated tables unless the vector attribute is cleared, or plan a background job pathway to invalidate and backfill the vector column when a
model version change is detected.
──────
4. Malformed @embed Directives Fail Silently during Schema Deployment — LOW (Confirmed)
File: graphql.ts
Domain: api / config
What: When a malformed @embed directive is encountered (e.g., omitting source or model , or supplying non-string literals), the schema compiler logs via console.error and skips registering the @embed property metadata, but
continues compiling.
Why it matters: Because no exception is thrown, the overall schema installation succeeds with a 200 OK . The field is silently registered as a regular, un-embedded Vector attribute. Users receive no indication that their @embed
directive was discarded unless they inspect the server console logs.
Code:
if (!embedDefinition.source || !embedDefinition.model) {
console.error(
`@embed on "${property.name}" requires both "source" and "model" arguments, at`,
directive.loc
);
}
Suggested fix:
Throw a descriptive ClientError to halt compilation and fail the schema deployment loudly, returning a helpful error message to the client:
if (!embedDefinition.source || !embedDefinition.model) {
throw new ClientError(
`@embed on "${property.name}" requires both "source" and "model" arguments`,
400
);
}
──────
5. Dependency on globalThis.logger in embedHook.ts — LOW (Likely)
File: embedHook.ts
Domain: config
What: Failure logging in embedHook.ts relies on resolving the logger dynamically via (globalThis as any).logger rather than using standard module imports.
Why it matters: Relying on globalThis for logger references deviates from ES module standards, bypasses static type checks, and can lead to unhandled runtime exceptions if the logger isn't populated on globalThis in specific
worker threads.
Code:
const logger = (globalThis as any).logger;
logger?.error?.(`Embedder for attribute "${attr.name}" failed:`, err);
Suggested fix:
Import the standard Harper logger statically:
import { logger } from '#src/utility/logging/logger.ts';
Kris's review (with Gemini cross-check) flagged six items + one nit; all applied. Local: 17/17 unit + 4/4 integration green. CRITICAL — silent data loss on PATCH: TableResource.update() called this._writeUpdate(...) at two sites (lines ~1247 and ~1257) and dropped the returned promise. With @embed active, _writeUpdate returns a pending promise; without awaiting it, the caller's `save()` ran before the write was registered on the txn — the embed never reached storage and the txn committed empty (or crashed with "transaction already closed" when the embedder finally resolved). Threaded both call sites through `when(...)` matching the pattern already in put/create. HIGH — Float32Array storage mangling on user-supplied vectors: coerceType returned a Float32Array for `type: 'Vector'`. The record encoder mangles typed arrays via `updateAndFreeze` into `{0,1,2,...}` maps — the same bug we already worked around in `createDefaultEmbedder` by emitting `Array<number>`. coerceType is on the input path for user-supplied vectors (REST writes, replication receivers, programmatic API), so it had the same hazard. Switched to `Array<number>` here too. MEDIUM — getFromSource cache writes did not run the embedder: Caching-table populations (`sourcedFrom()` → `getFromSource()`) build their own write op at Table.ts:~4511 and call `addWrite` directly, bypassing `_writeUpdate`. So an `@embed` declared on a caching table silently never embedded. Wired `buildEmbedBefore` into that path with the same "await before addWrite" pattern. This is the canonical use case Kris highlighted (and matches #750's derived-cache-table follow- up — caching tables are where embeddings on derived data live). LOW — malformed @embed silently degraded: Missing `source:` or `model:` only logged `console.error` and let schema deployment succeed — the resulting Vector field was registered as a regular un-embedded attribute. Promote to a thrown Error so the component install fails loudly. Note: this breaks the file's existing "log and continue" convention for other directive validations (dup primary key etc.); justified inline because @embed's silent-degrade failure mode is harder to diagnose than a missing primary key. LOW — globalThis.logger: Replaced with a lazy `require('#src/utility/logging/logger')` resolver. The cleaner static `import { logger } from ...` trips the documented `common_utils.ts ↔ harper_logger.ts` CJS cycle (ERR_REQUIRE_CYCLE_MODULE) the moment this module loads via a unit-test path that bypasses the full transaction stack. Lazy resolution sidesteps the cycle while still going through standard module resolution (not globalThis lookup). NITS — applied: - Parallelize per-attribute embedders via Promise.all (Kris). Typical schemas have one @embed field per table, but multi-@embed shouldn't serialize HTTP roundtrips for no benefit. - Simplify `Object.prototype.hasOwnProperty.call(record, key)` to `key in record` (Kris's "so paranoid, we have prototype-pollution protection (freezing)"). Open question (not in this commit): Should `Vector` storage preserve Float64/Float16 precision? Today everything flattens to Array<number> and is re-narrowed when HNSW needs Float32 for distance computation. Kris asked about Float16 specifically; deferring to PR thread. Refs #632 #510 #750 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
claude-bot's re-review of 47bd103 flagged missing test coverage for the source-changing PATCH path. Writing those tests surfaced the actual bug — the `when(...)` wrap 47bd103 added to `TableResource.update()` is unreachable in production for REST PATCH and PUT. ROOT CAUSE — legacy URLSearchParams branch drops the promise: Table.patch(target, recordUpdate) and Table.put(target, record) both detect the back-compat `URLSearchParams` argument shape and route through: (this as any).update(target, false); // promise DISCARDED return this.save() as any; REST dispatch goes through the transactional wrapper which calls `resource.put(data, query)` / `resource.patch(data, query)` — `query` is a URLSearchParams, so the URLSearchParams branch is the CANONICAL REST path, not the "standard path" branch that 47bd103 fixed. The embedder's promise on `update()` was settled-and-discarded; `save()` ran on a write that hadn't yet been staged on the txn, so on a busy worker the embedder mutation arrived after the in-memory commit had already taken the un-embedded record. Fixed both put() and patch() with the same shape used in the standard path: `when(this.update(target, ...), () => this.save())`. TESTS — three new integration tests in embed-directive.test.ts: - PATCH-with-source — PATCH `{content: '...'}` on a seeded row; verifies embed fires exactly once and stored vector matches `deterministicVector(updatedContent)`, not the seed. - PUT-with-source — same shape via REST PUT; locks in the parallel `Table.put()` fix. - Caching-table @embed — installs a `resources.js` that exposes `CachedEmbedSource extends Resource` and calls `CachedEmbedDoc.sourcedFrom(CachedEmbedSource)`. GET on the cache triggers `getFromSource` → cache write → embed wiring at `Table.ts:~4520`. Verifies the cached row's vector matches the source-returned content, and a cache-hit GET does not re-fire the embedder. Closes claude-bot blocker #2 directly. Local: 17/17 embedHook unit + 13/13 caching unit + 7/7 integration green. Refs #632 #510 #747 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…view fixes Cross-model adjudication of PR #747 (agy + codex) surfaced the structural limit of the previous embed wiring: `buildEmbedBefore` ran at `_writeUpdate`- time against whatever `recordUpdate` happened to be at that moment. For the canonical instance-mutation pattern documented in `unitTests/resources/ transaction.test.js`: let row = await Table.update(id, {}); row.content = 'new text'; await row.save(); `_writeUpdate` saw the empty `{}` payload, returned `undefined` from `buildEmbedBefore` (no source field in the record), and the txn committed the row without ever firing the embedder. Silent data drift on a programmatic API surface Harper documents in its own tests. ROOT CAUSE FIX — validate-time embed hook: - `Table.ts` validate closure now runs the embedder against the FINAL `recordUpdate` (after instance mutations, before `updateAndFreeze` for full updates). Returning the embedder's promise from `validate` makes the txn machinery await it before invoking the per-write `commit(...)`. - `DatabaseTransaction.save` and `LMDBTransaction.commit` are now async-validate-aware. If `validate?.(txnTime)` returns a promise, both code paths defer `before` / `beforeIntermediate` / `commit` until it settles, and the resulting promise propagates into the txn completions so the whole transaction commit awaits it. - The retry path (operation.saved === true) preserves the existing behavior of running ONLY `commit(...)`, not re-firing `before` / `beforeIntermediate` — subscription events stay 1:1 with writes. OTHER FINDINGS APPLIED: - `databases.ts:1106` — `changed` predicate now compares `attribute.embed` JSON-shape so a `@embed(source: "fieldA")` → `@embed(source: "fieldB")` swap (same model, same `version`) triggers a refresh of `embedAttributes` / `userEmbedders` on schema reload. Without this the model-version check above caught model swaps but not source swaps. - `embedHook.ts` — custom embedders set via `setEmbedAttribute` may return `Float32Array` or any typed-array; new `normalizeVector` step flattens to `Array<number>` at the boundary (same reason the default embedder already does this — record encoder mangles typed arrays via `updateAndFreeze`). Default embedder is unaffected. - `embedHook.ts` — skip the embedder call when the source-field payload is a CRDT operation (`{__op__, value}`). Harper today only supports numeric `add` (`resources/crdt.ts`); non-numeric `__op__` payloads would have stringified to "[object Object]" and burned an embedder API call before `validate()`'s unwrap (at `Table.ts:3131`) discarded the wrapper. - `embedHook.ts` docstring on `buildEmbedBefore` updated — was stale, described serial execution but the implementation is `Promise.all`. TESTS: Integration (`integrationTests/server/embed-directive.test.ts`, 7 → 8): - New `EmbedMutator` custom resource exercises the instance-mutation pattern via `EmbedDoc.update(id, {}); row.content = ...; row.save()` and verifies the embedder fires against the FINAL recordUpdate. Closes the codex-cross-model blocker directly. - Schema declares both `EmbedDoc` (existing) and `CachedEmbedDoc` (added earlier in this branch for the caching-table path). Unit (`unitTests/resources/models/embedHook.test.js`, 17 → 19): - `Float32Array` embedder output is normalized to `Array<number>`. - CRDT-op source payloads skip the embedder entirely. Existing 17 embedHook unit tests + 13 caching unit tests stay green; 3 subscription tests that are pre-existing flakes on `47bd103c` baseline remain flaky here (better, not worse: 3 fails on baseline, 2 fails post- refactor — refactor does not introduce new flake). NOT IN SCOPE (filed implicitly via the adjudication, won't land here): - Schema-reload model-change re-embed-all-records backfill — tracked under #750 (derived-cache-table) where the iterate-and-rewrite primitive naturally lives. - `@embed` deploy-time schema-shape validation (non-Vector target, typo'd source) — write-time error is acceptable for now. - Batch `Table.put(id, [...])` race on shared `#savingOperation` — only affects the `loadAsInstance === false` programmatic path; REST array PUT goes through `Resource.put`'s per-element fresh-resource branch which has no shared state. - Float16 / Float64 storage representation — Kris's open question on `Table.ts:4858`; deferred to follow-up against #510 / #750. Refs #632 #510 #747 #750 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI Format Check on 41573bf flagged `resources/databases.ts` even though local `npm run format:check` passed — Prettier's inline-comment-between- `||`-operators handling appears to differ between environments. Moving the comment to a single block above the whole `const changed = ...` expression makes the formatting stable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…test Two follow-ups from the 41573bf CI run: 1) `resources/databases.ts` prettier — the `Coerce envGet values to native types before passing to RocksDB` change on main (bc725ea / 46109e6) landed without prettier collapsing the per-key spread ternaries into single lines. The pre-merge files were each Prettier-clean on their own, but the merged file isn't. Applied `prettier --write` to fix the spread formatting; my new `attribute.embed` block-comment is untouched. 2) `caching table with @embed` integration test — the `getFromSource` design (Table.ts:~4275) resolves the GET BEFORE the cache write's txn commits ("we don't want to wait for the transaction"). On a slow CI runner the subsequent `search_by_hash` can race the commit and return `[]`. Replaced the single `search_by_hash` with a short polling loop (10 × 25ms = 250ms max) — the race window in practice is sub-100ms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 47bd103 commit message and the two JSDoc blocks in `embedHook.ts` blamed `updateAndFreeze` for mangling typed arrays into `{0,1,2,...}` maps. Verified against the code that's false on both counts: - `updateAndFreeze` (`resources/tracked.ts:393`) for a `Float32Array` input falls through to line 438 and returns the typed array unchanged — no `getRecord`, no `getChanges`, no rewrite. - msgpackr's default Encoder (which Harper's `RecordEncoder` extends with only `useBigIntExtension = true`) IS what doesn't round-trip typed arrays. Test: `enc.encode(new Float32Array([0.1, 0.2, 0.3]))` produces `c4 0c 00 00 00 00 00 00 00 00 00 00 00 00` — msgpack `bin8` of length 12 with zeroed bytes — which decodes back to a Node `Buffer` of zeros. Source bytes are lost. - The fix-direction is the same (flatten to `Array<number>` at the boundary), but the diagnosis is "msgpackr's default Encoder", not "`updateAndFreeze`". `structuredClone: true` on msgpackr would preserve typed arrays via ext type `0x07` if we wanted to. Updated both comment blocks. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex's second cross-model pass (on commit 41573bf's validate-time refactor) flagged two blockers that the existing 8-test integration suite didn't exercise. Both are real, both are now closed: ROOT CAUSE 1 — writes[] order violated under async validate: `save()` was returning `validateResult.then(runBeforeAndCommit)`. With multiple async-validate writes (multiple @embed writes in one txn, or cache-population sourceWrite + a user-write in the same txn), each write's `runBeforeAndCommit` fired from its own `.then` callback — meaning whichever embedder HTTP roundtrip finished first committed first. Same-key last-write-wins, audit order, and subscription event order all became non-deterministic. Fix: split the lifecycle into Phase 1 (validate, kicked off in writes[] order) → Phase 2 (await all validates via the existing `Promise.all`) → Phase 3 (`before` / `beforeIntermediate` / `commit` in writes[] order, for any write `save()` marked `deferredCommit`) → Phase 4 (await any before/beforeIntermediate promises raised during Phase 3, then close the txn). The synchronous fast-path for non-@embed writes is unchanged — `save()` runs the full lifecycle inline and Phase 3 finds nothing to do. ROOT CAUSE 2 — orphaned before/beforeIntermediate promises: `before` / `beforeIntermediate` raised inside `runBeforeAndCommit`'s `.then` callback got pushed into `this.completions` — but by the time the callback fired, `commit()` had already snapshotted and reset that array. The freshly-pushed promises landed in a new empty array that no `Promise.all` was waiting on, so the txn closed before they settled. Affected paths: @embed on a sourcedFrom cache table (the canonical use case Kris highlighted) and @embed on a table with a Blob attribute (`beforeIntermediate = preCommitBlobsForRecordBefore`). Fix: Phase 3 (above) runs `before` / `beforeIntermediate` in the same closure scope as `this.completions`, then Phase 4 reads and awaits the completions array if Phase 3 added anything. PLUS — embedder-rejection no longer leaks the txn: The validate `.then`-chain previously had no reject handler. An embedder failure rejected `Promise.all(completions)`, which short- circuited the close path — the read txn was never released and any pre-saved blobs weren't cleaned up. The reject handler on the outer `when(...)` (both the sync-immediate path inside `save()` and the deferred path on `commit()`) now calls `this.abort()` before rethrowing. NEW TEST: `multi-write @embed in one txn: writes[] order is preserved` — a custom `MultiEmbedMutator` resource does two `EmbedDoc.put(...)` calls on the same id inside one txn, with different content per call. The second PUT MUST win deterministically — locks in the Phase 3 ordering. The test fails reliably (different vector / wrong content) under the pre-fix code where embedders were chained per-`.then`. VERIFICATION: - 9 / 9 @embed integration tests pass locally - 19 / 19 embedHook unit tests pass - 13 / 13 caching unit tests pass - 33 / 33 CRUD unit tests pass (no regression on the sync write path) The fix preserves agy's earlier "validated correct & robust" verdict on the sync path: for non-@embed writes the closure allocation is the only delta, no microtask hops added. Refs #632 #510 #747 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…per-foreign pattern After three rounds of cross-model review (agy + codex) on the validate-time @embed refactor (41573bf, bcbeef7), each round surfaced new edge cases in the underlying txn machinery — the trajectory wasn't converging cleanly: - Round 2 codex flagged instance-mutation bypass. I closed it by moving `buildEmbedBefore` from `_writeUpdate`-time into `write.validate` and making `DatabaseTransaction.save` / `LMDBTransaction.commit` async- validate-aware. - Round 3 codex + agy both independently flagged mixed sync+async write ordering inversion: sync writes commit during Phase 1, async writes defer to Phase 3 — `PUT(content)` then `PATCH(tag)` on the same id apply as PATCH-then-PUT, losing the last write. Plus uncaught throws in Phase 3, plus immediateCommit local rocks-txn not aborted on embedder rejection. The pattern review (`harper-architect` skill + direct read of `@computed` and `preCommitBlobsForRecordBefore`) shows Harper has NO precedent for async write-time record-mutation: - `@computed` is a READ-time resolver (values calculated on access, not stored). - `preCommitBlobsForRecordBefore` does SYNC blob-ID allocation; the async work writes blob bytes independently of the record. - Cluster-replication receive and audit-log replay both ingest already- computed values. The txn machinery has implicit ordering invariants — `addWrite` order → `operation.commit()` order — that don't admit async mutation cleanly. The validate-time refactor was inventing a new pattern at the deepest layer of the storage engine, and the cumulative surface (reject paths, mixed sync/ async order, blob/source-write interactions, retry semantics) was costing us a round of new findings each iteration. REVERTED: - 41573bf's validate-time-embed refactor (Table.ts validate-closure embed hook; DatabaseTransaction.ts + LMDBTransaction.ts async-validate plumbing). Restored `_writeUpdate`-time hook (agy approved this shape in round 2). - bcbeef7's Phase 1/3/4 commit-pipeline + abort plumbing. - The `EmbedMutator` / `MultiEmbedMutator` integration tests that depended on validate-time semantics. RETAINED from the same commits (independent improvements, no validate-time dependency): - `databases.ts` schema-reload `attribute.embed` JSON-shape comparison (codex-sig-2; bare 1-line fix). - `embedHook.ts` `normalizeVector` (typed-array → Array<number>) and CRDT-op `__op__` source-skip predicate (codex-sig-3, codex-sig-4). - `embedHook.ts` corrected typed-array round-trip docstring (5c58b28). - Unit tests covering both. - Caching-test commit-race poll loop (1bf8af4). - Prettier on merged `databases.ts` (1bf8af4). - put/patch URLSearchParams legacy-promise-drop fix from 05f19e6 — a pre-existing real bug independent of the validate-time work. KNOWN LIMITATION documented in `embedHook.ts`: Instance-mutation pattern (`update(id, {}); row.content = '...'; row.save()`) doesn't pick up @embed. `_writeUpdate` sees the empty `{}` and skips the hook. The pattern is documented in `unitTests/resources/transaction.test.js` but is uncommon in REST / programmatic application flows. The proper fix belongs above the resource layer (pre-txn embedder middleware in the `transactional()` wrapper), tracked as a follow-up to #510 / #632 / #750. PR now ships with 7 integration tests (all passing locally and on CI through 1bf8af4). All `@embed` write paths exercised by these tests work correctly: - Happy-path POST - Source-unchanged PATCH (existing embedding survives) - Source-changing PATCH (re-embed; the canonical write-time fix) - PUT (full-record update; same write-time fix) - Replication-receiver skip - Caching-table @embed via `sourcedFrom` - Schema-with-@embed table creation + auto-HNSW Refs #632 #510 #747 #750 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…check Round-4 cross-model review of the simplified post-revert shape — both reviewers (agy + codex) confirmed the `_writeUpdate`-time hook is correct. codex flagged one important edge: bulk PUT with duplicate ids in the same payload commits in embedder-resolution order, not payload order. ROOT CAUSE — bulk array path uses `Promise.all` over `record.map`: `Table.put(target, [...records])` at `resources/Table.ts:1548` and the static `Resource.put` collection-array branch at `resources/Resource.ts:109` both iterate the array and `Promise.all` the per-element writes. For tables WITHOUT `@embed`, `_writeUpdate` is synchronous and `Promise.all` over `.map` preserves payload order because each iteration completes synchronously before the next begins. For tables WITH `@embed`, `_writeUpdate` returns a promise (embedder HTTP roundtrip) — elements race and the one whose embedder resolves last commits last. For unique ids per payload (the common case), no behavior change — each row writes once. For DUPLICATE ids in one payload, a slower-embedding element earlier in the payload would commit AFTER a faster one later in the payload, inverting last-write-wins semantics. FIX — sequence array elements when the table has `@embed` attributes: `Table.put`: when `TableResource.embedAttributes?.length`, replace the `Promise.all(record.map(...))` with a sequential Promise chain. Non- `@embed` tables keep the parallel fast path (no behavior change). `Resource.put`: when `resourceClass.embedAttributes?.length`, sequence the per-element `resourceClass.getResource(...).put(...)` chain. Same reasoning — only `@embed` tables sequence. Test: new integration test `bulk PUT with duplicate ids` — sends a 2- element payload with the same id and different content. The second must win. Fails reliably on the pre-fix code where embedder timing decides. PLUS — agy nice-to-have: drop the redundant `Float32Array` check in `normalizeVector`. `ArrayBuffer.isView` covers all TypedArrays including `Float32Array`; the `instanceof Float32Array` branch above it was duplicative. Both reviewers' round-4 verdict: no blockers, this and the dedup were the only items raised. Local: 8/8 integration + 19/19 unit + clean build, prettier, lint. Refs #632 #510 #747 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
….put bulk-sequencing Both round-5 reviewers (agy: significant, codex: suggestion) flagged that the new `bulk PUT with duplicate ids` integration test only exercises `Resource.put`'s collection-array branch, not `Table.put`'s `Array.isArray(record)` branch. The branches are structurally identical (same Promise-chain sequencing pattern under the same `embedAttributes?.length` guard); the Table.put path is reachable only via programmatic `put(target, [array])` from callers that bypass `Resource.put`'s static dispatch (e.g. tables with `loadAsInstance: false`). Adding a contrived fixture for marginal coverage of structurally-identical code is poor ROI; document the relationship instead so future readers understand which test covers which code path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…bered by my revert
CI was red on `unitTests/apiTests/undefinedIdInUrl-test.mjs` (7 failures —
PATCH/PUT to `/DogIntPK/null`, `/DogAnyPK/{null,undefined,NaN,true}` all
returning 500/204 instead of 400). Root cause: my `git checkout 05f19e6 --`
revert during the validate-time rollback (commit `e0233037`) restored
`Table.ts` to its pre-`d10b483b` state, accidentally reverting Chris
Barber's `checkValidId` → `ClientError(400)` fix that had landed on main.
This re-applies just the `checkValidId` block from `d10b483b` — converting
the four `Error` throws to `ClientError(_, 400)` and adding the explicit
`isNaN` reject for numeric PKs. No other files affected; no @embed
behavior change.
Verified locally: `undefinedIdInUrl-test.mjs` 18/18 pass, `embed-directive.test.ts`
8/8 pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks Kris — all three addressed in
−377 lines net. Codex review clean; unit + integration suites green. 🤖 Posted by Claude on Nathan's behalf |
8bae12c to
5e5ce29
Compare
kriszyp
left a comment
There was a problem hiding this comment.
This is going to be awesome, good job.
|
Heads-up on a friendly seam where this meets the HNSW int8 work in #894. #894 makes the index store vectors as int8 for fast graph traversal (the per-node-visit vector decode was 54–71% of search/insert CPU; quantizing the index gave ~4.9× search and a ~3× smaller index). This PR's One small coordination point, no conflict (#894 is index-only): after this lands, Nice work on the — Claude |
…cation; note source-subscribe gap (#632) The isNotification skip in buildEmbedBefore is correct for cluster-replication receivers (the peer already computed the vector), but proactive source-subscribe pushes into a caching table also set isNotification and so skip embed today. Documented as a known gap, tracked with the derived-caching follow-up (#750). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…le note Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ches (#632) - graphql.ts: fix the model-version comment (reindex re-indexes stored vectors, it does not re-embed — the "(as @computed does)" parallel was wrong). - embed-directive.test.ts: collapse decodeVector to the only live branch (vectors are stored/returned as plain number[]); the Float32Array/Buffer/Uint8Array branches were dead since the Vector type was dropped. Fix deterministicVector's doc (returns number[], not Float32Array). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ig (#632) - updatedAttributes() now prunes userEmbedders/userSetEmbedders entries for attributes that are no longer @embed, so dropping the directive doesn't leave a stale embedder or block a default refresh on re-add. - describe_table now surfaces the @embed {source, model} config alongside the auto-added HNSW index. - Tests: embedRegistry.test.js (default registration, override survives reload, prune-on-drop) + a describe assertion in the integration suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Auto-HNSW attach + the non-HNSW rejection now resolve AFTER the directive loop, so an explicit @indexed is honored regardless of directive order. Previously the check ran inside the @embed branch, before a later @indexed(type: "BTREE") could overwrite the auto-added HNSW — silently leaving the vector column unindexed. Tests: embedDirective.test.js — auto-HNSW when no @indexed, accepts explicit HNSW, rejects a non-HNSW @indexed with ClientError(400). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On a loadAsInstance:false table the Table.put array branch reaches multiple async _writeUpdate() calls directly. Each stores its write in the shared instance field #savingOperation, and the deferred save() read that shared field — so when one element's @embed hook resolved, save() could commit a *later* element's operation before that element's vector was written (a timing race introduced by the async embed hook). Capture each element's operation synchronously and save the captured op (via extracted #saveOperation) instead of the shared field. Test: embedBulkWrite.test.js uses staggered embed timing (fast element + delayed element); it fails without this fix (the delayed element's vector is lost) and passes with it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@embed targets a vector column, but the parser only validated source/model — a scalar target (e.g. `embedding: String @embed(...)`) would parse, then store the number[] vector on a non-array attribute and auto-HNSW-index a non-vector. Reject a non-array target at parse time with ClientError(400), alongside the existing non-HNSW-index rejection. Test: embedDirective.test.js — scalar @embed target rejected with 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#632) Tighten the @embed target-type check (per genuine Codex review): validating only the outer `array` type let `[String]`/`[Int]` through, which would make a model call and then fail write validation when the float vector hits a non-float column. Require the element type to be Float at schema load. Test: embedDirective.test.js — [String] @embed target rejected with 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… hygiene (#632) Per genuine Codex review: - @embed(source: "...") referencing a non-existent field now fails schema load with ClientError(400). Previously it loaded fine and silently left the vector column unpopulated (the source key never appears in write payloads). - embedRegistry.test.js now calls setupTestDBPath() so a standalone run uses the test sandbox, not the developer's default Harper paths. Tests: embedDirective.test.js — unknown @embed source rejected with 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…632) Re-add a concise note (trimmed earlier) at the _writeUpdate hook: the embedder runs against the write payload before table validation. Consequences, accepted as a known limitation (the validate-time alternative was tried and reverted as Harper-foreign): an invalid write still calls the backend, and a tracked-instance mutation that sets the source via accessors after update() won't re-embed. Proper fix is a resource-layer re-embed, tracked as a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#632) Per genuine Gemini review: the source-field existence check used `source in attributesObject`, but attributesObject is a plain object, so `in` matched inherited prototype keys — @embed(source: "toString") / "constructor" bypassed the unknown-field validation and silently left the column unpopulated. Use Object.hasOwn so only declared fields pass. Test: embedDirective.test.js — @embed(source: "toString") rejected with 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A non-string @embed arg (e.g. @embed(source: 123, ...)) logged then fell through to the generic missing-arg error; throw a specific ClientError(400) right away, consistent with @embed's other loud-fail validations. Test: embedDirective.test.js — non-string @embed arg rejected with 400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ckground commit (#632) A reviewer had to reverse-engineer that an embedder failure on the getFromSource cache-fill path can't hard-fail the caller (the client GET already resolved with fresh source data before the hook runs; failure is handled by the outer error handler and the row re-embeds next read) and that source-resolution errors are the ones eligible for the stale-data fallback. Document it so it isn't re-raised. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Phase 5 of #510. An
@embedschema directive that, on write, embeds a source field into a vector attribute and HNSW-indexes it — no app code.Three lines replace a custom embedding pipeline.
What ships
resources/graphql.ts): an@embed(source, model)branch. Both args are required and must be string literals — a malformed directive fails schema install with aClientError(400). When the attribute has no explicit@indexed, HNSW indexing is auto-attached. The model is recorded as the attribute version so a model change triggers a reindex.resources/Table.ts):Table.setEmbedAttribute(name, embedder)lets a component author replace the default embedder (e.g. multi-field concatenation or a differentinputType).resources/models/embedHook.ts,resources/Table.ts): on a write whose payload includes the source field, the embedder runs and writes the vector onto the record before the write commits (sync-by-default). It also wires the cache-fill (getFromSource) path so caching tables embed on source fetch.number[]. Harper's record encoder does not round-trip typed arrays, so any embedder output (incl.Float32Array) is flattened at the boundary. Declaring the attribute[Float]reuses the existing array type and HNSW path — no new primitive type.isNotification), RESTx-replicate-from: none, or audit-log replay — so receivers store the originator's vector verbatim and the cluster stays in one embedding space.Failed to compute embedding for attribute "<name>".Design notes
Vectorprimitive. A vector column is just[Float]; HNSW already indexes a plainnumber[]. (Reviewed and dropped in favor of reusing[Float].)Known limitation
Proactive source-subscribe pushes into a caching table set
isNotification, so they currently skip embedding (the predicate can't yet distinguish a cluster-replication receive from an external source push). The lazygetFromSourcepath and direct writes embed normally. Closing the subscribe path is folded into the derived-caching work (#750).Tests
unitTests/resources/models/embedHook.test.js): default embedder, the replication-skip signals, source-presence semantics (embed on present, clear on null, skip on omit), typed-array →number[]normalization, CRDT-op skip, and error sanitization.integrationTests/server/embed-directive.test.ts): end-to-end against an in-test fake Ollama — schema install + auto-HNSW, POST/PUT/PATCH embed behavior, replication-receiver skip, and the caching-table path.Out of scope (follow-ups)
Closes #632. Part of #510.
🤖 Generated with Claude Code