Files
rohitg00--agentmemory/test/vector-index-populate.test.ts
Rohit Ghumare 24f6c5fb24 fix(search): BM25 unicode + vector index live-write — improved (closes #295, supersedes #296) (#327)
* fix #2: BM25 tokenizer now preserves Cyrillic/Unicode via \p{L}\p{N} regex

Signed-off-by: Nikita Nosov <20nik.nosov21@gmail.com>

* fix #1: add getVectorIndex/setVectorIndex + vector embedding in rebuildIndex

Signed-off-by: Nikita Nosov <20nik.nosov21@gmail.com>

* fix #1+#3: vectorIndex.add in remember/observe/compress + setVectorIndex in index.ts

Signed-off-by: Nikita Nosov <20nik.nosov21@gmail.com>

* fix: add migration script, Cyrillic BM25 tests, vector-populate tests, rebuildIndex vector test

Adds migrateVectorIndex utility for re-embedding when embedding provider dimensions change. Adds comprehensive test coverage for Cyrillic tokenization, vector index population on remember/rebuildIndex, and migration edge cases.

Signed-off-by: Nikita Nosov <20nik.nosov21@gmail.com>

* chore: remove internal research spec from PR branch

Signed-off-by: Nikita Nosov <20nik.nosov21@gmail.com>

* refactor(search): centralize vector-index writes via vectorIndexAddGuarded

Pre-merge hardening on top of @nik1t7n's PR #296.

Original PR had vectorIndex.add() inlined at four sites (remember,
observe, compress, rebuildIndex) with the same try/catch shape repeated
each time. That open-codes three sharp edges per site:

1. Missing dimension guard at the write boundary. Per #248, persistence
   load already refuses to start when persisted vectors mismatch the
   active provider's dimensions — but the live-write path had no
   symmetric check. A mis-configured embedding provider returning a
   different-length Float32Array would silently corrupt the in-memory
   index until next restart cleared it.

2. No input clipping. Memory.content can be arbitrarily large (the
   MemorySlot path even allows 20KB content per slot). Most embedding
   providers cap input around 8k tokens and 400 on overflow. A single
   oversize memory could throw at the provider and lose the vector
   entry; worse, on local-Ollama-style providers it can cost real
   inference time.

3. Inconsistent error logging (rebuildIndex had `catch {}` swallowing
   the error entirely, the other three sites logged via logger.warn
   with slightly different shape).

Consolidates into a single exported `vectorIndexAddGuarded()` in
src/functions/search.ts:

  - Reads vectorIndex + currentEmbeddingProvider from module-level
    state (same singletons the PR added).
  - Clips embed input to 16k chars (EMBED_MAX_CHARS) — well under
    every provider's documented limit, leaves headroom for
    chars-per-token variance across languages.
  - Validates embedding.length === provider.dimensions before
    calling vi.add(); logs structured warning and skips on mismatch.
  - Single logger.warn on embed failure with provider name + error
    message + indexed-item kind ("memory" | "observation" |
    "synthetic") + id.
  - Returns boolean (true on success) so callers can branch if they
    want, but every existing call site fire-and-forgets — the contract
    is "soft-fail: index entry skipped, upstream save still succeeds".

Rewrites the four call sites (remember.ts, observe.ts synthetic
branch, compress.ts post-LLM, search.ts rebuildIndex memories + obs
loops) to call vectorIndexAddGuarded() instead of the inline try/catch.

Side effect: cleans up the lingering `catch {}` in rebuildIndex that
was swallowing rebuild-time embed failures silently — every failure
now surfaces in logs at a consistent shape.

Tests:
- test/multimodal.test.ts mocked `../src/functions/search.js` and
  exposed only getSearchIndex; needed vectorIndexAddGuarded added to
  the mock surface or test fails with "No vectorIndexAddGuarded export
  is defined on the search.js mock". Added the export to the mock.
- All other tests (search-index Cyrillic, search rebuildIndex vector,
  vector-index-populate, migrate-vector-index) green on the helper
  refactor — no behavioural change beyond the three guards above.

886 / 886 tests pass. Build clean.

Out-of-scope (deferred to follow-ups):
- Blocking await on every save. The embed call is on the request hot
  path. For local Ollama / on-device providers this is fine; for
  network providers it adds 200ms-2s latency per remember/observe.
  Should be async / fire-and-forget with a write-behind queue —
  separate perf PR after this lands.
- Periodic persistence flush of the vector index. Today the index is
  serialized at process exit (per IndexPersistence). Runtime adds
  between flushes survive in memory but die on crash. Acceptable for
  this PR; future PR can add a debounced flush.
- Batching in rebuildIndex. Today the rebuild path embeds one doc at
  a time. embedBatch() exists on every provider and would be ~5-10x
  faster for cold restarts on large corpora. Defer.

* fix(search): tighter migrate guard + rebuild vector clear + test hygiene

Five reviewer findings applied. Five skipped with reason:

APPLIED:

1. migrate-vector-index.ts filter now requires m.content (and rejects
   empty/whitespace). Without this, undefined m.content would concat
   to "title undefined" and poison the embed input. Memory.content is
   declared string but #277-era data shapes had cases where it could
   be missing — defensive filter.

2. migrate-vector-index.ts success expression now derives from the
   failed counter (success: failed === 0). Previously hardcoded true
   so callers couldn't rely on the boolean alone.

3. search.ts rebuildIndex now calls vectorIndex?.clear() before the
   repopulation loops. Symmetric to the BM25 idx.clear() that was
   already there. Without this, memories deleted between runs leave
   orphan embeddings in the vector store after every rebuild.

4. test/vector-index-dimensions.test.ts assertion tightened from
   toBeGreaterThan(0) to toBe(1) for both totalProcessed and
   vectorSize. The seeded fixture has exactly one observation; loose
   assertion would hide regressions where the migration produced
   extra/duplicate vectors.

5. test/vector-index-populate.test.ts switched to beforeEach/afterEach
   for the setVectorIndex / setEmbeddingProvider lifecycle. Manual
   cleanup in each test body was easy to drift on additions.

PLUS:

6. compress.ts BM25 add now wrapped in try/catch matching the
   remember.ts pattern for parity. Helps cross-file symmetry even
   though SearchIndex.add doesn't throw on valid input.

SKIPPED (with reason):

- Wrapping vectorIndexAddGuarded calls in additional try/catch at
  remember.ts / observe.ts / compress.ts: the helper has internal
  try/catch (src/functions/search.ts:67-89). It NEVER throws. Adding
  outer try/catch is dead code. The reviewer missed the helper's
  contract.
- vector-index-dimensions.test.ts mockKV.delete -> real deletion: the
  tests in that file don't exercise deletion semantics, and
  migrateVectorIndex itself never calls kv.delete. Touching it would
  add noise without effect on any assertion.
- recordAudit + Promise.allSettled in remember.ts: out of scope.
  recordAudit is already called earlier in the function. Parallelizing
  cascade-update with vector-add changes failure semantics (cascade-
  update is currently fire-and-forget) — separate perf PR if wanted.
- validateDimensions(N) assertion in vector-index-dimensions.test.ts:
  vectorSize is the COUNT of stored vectors, not their dimensions.
  Adding a validateDimensions check on the result would require
  exposing the new index. Out of scope for assertion tightening.

886 / 886 tests pass. Build clean.

* fix(migrate): per-session isolation + dim guard + drop unused oldIndex param

Four reviewer findings applied. One skipped with reason:

APPLIED:

1. Per-session isolation in migrateVectorIndex observations phase. The
   previous shape wrapped the entire sessions for-loop in one try/catch
   — a single bad session (kv.list throws, embedBatch rejects, etc.)
   aborted every subsequent session and silently truncated the
   migration. The loop now has per-session try/catch; failures
   increment `failed`, append the session id to a new `failedSessions`
   array on the result, and the loop continues. The kv.list<sessions>
   call itself is now guarded separately — if THAT fails the whole
   migration aborts (no sessions to iterate), but with `failed`
   recorded and `failedSessions` empty so the caller can distinguish.

2. Drop `oldIndex` parameter from migrateVectorIndex signature. It was
   dead — no read path inside the function. Three test call sites
   updated to match the new signature
   migrateVectorIndex(kv, newProvider). Confused the API contract; if
   we ever DO need a previous-index argument it can come back with a
   real use case.

3. Dim guard on every embedding result inside migrateVectorIndex, in
   both the memories phase and the per-session observations phase.
   Extracted into a local isValidEmbedding() helper that mirrors the
   shape of search.ts::vectorIndexAddGuarded's guard: log structured
   warn + skip on mismatch. Without this, a misconfigured provider
   returning the wrong-length Float32Array would corrupt the rebuilt
   index, defeating the #248 persistence-load guard.

4. Rename `vi` -> `vectorIndex` in test/vector-index-populate.test.ts
   so the let binding doesn't shadow the vitest `vi` import. Practical
   impact: future tests in that file can use `vi.fn()` / `vi.spyOn()`
   without renaming this binding first. Cosmetic but cheap.

SKIPPED (with reason):

- Use embedBatch in rebuildIndex (search.ts): perf optimization, not
  correctness. Already called out as deferred in the previous review's
  commit message. Cold-restart speed matters but the fix here is
  bigger refactor (collect-into-batches with metadata, validate each,
  add only successful) and worth its own PR. Will pick up when we
  also do the write-behind queue for live-write batching.

Result envelope:
  Added `failedSessions: string[]` to MigrateVectorIndexResult so the
  caller can decide whether to retry per-session or just abort.
  Existing fields (success, totalProcessed, failed, vectorSize)
  unchanged in shape.

886 / 886 tests pass. Build clean.

* fix(migrate): attribute batch-level failures to correct count + distinguish list-fail

Two new reviewer findings on migrate-vector-index. Six earlier findings
re-flagged in the same review block had already been addressed in prior
commits on this branch (verified inline):

  - compress.ts BM25 try/catch (commit d2098a5)
  - migrate filter requires m.content (commit d2098a5)
  - migrate success: failed === 0 (commit d2098a5)
  - rebuildIndex vector clear (commit d2098a5)
  - observe.ts / remember.ts outer try/catch — SKIPPED with reason
    (vectorIndexAddGuarded has internal try/catch, NEVER throws)
  - mockKV.delete no-op — SKIPPED with reason (no test exercises it)

NEW FINDINGS APPLIED:

1. Catch block in memories phase counted +1 for the entire batch
   regardless of batch size. If kv.list returned 250 memories and
   embedBatch threw, `failed` reflected 1 missed embedding when
   reality is 250.

   textMems is now declared outside the try so the catch can read its
   length. If kv.list itself threw before textMems was populated
   (length 0), we fall back to +1 (something failed but the batch
   size is unknown). If embedBatch threw, `failed += textMems.length`
   so the counter is honest.

2. kv.list<sessions> failure path returned `failed: 1,
   failedSessions: []`. Indistinguishable from "0 sessions, all OK"
   from the caller's perspective — they can't tell whether the
   enumeration itself blew up or simply found no sessions.

   The path now pushes a marker string "<sessions-list-failed>" into
   failedSessions before returning. Marker keeps the existing schema
   (failedSessions: string[]) — no boolean flag added — and gives
   callers a sentinel they can grep for. Per-session entries continue
   to use real session ids, so the marker is unambiguous.

886 / 886 tests pass. Build clean.

* test: swap Cyrillic non-ASCII tokenizer fixtures for Greek

The two regression tests added in this branch for the BM25 non-ASCII
tokenizer fix used Russian-language fixtures
("Проверка памяти", "Тестируем поиск по кириллице", etc). Switching
to Greek-language equivalents ("Προβολή μνήμης",
"Δοκιμάζουμε αναζήτηση σε ελληνικά", etc).

Reasoning: the test fixtures need to exercise the same code path —
non-ASCII Unicode letters that the old ASCII-only \w regex stripped.
Greek code points (U+0370..U+03FF) hit \p{L} identically to Cyrillic
(U+0400..U+04FF), so the regression coverage is preserved. Greek is
also less likely to be misread as politically loaded content in a
public repo.

Test ids renamed from obs_cyrillic to obs_greek; test names from
"indexes and finds Cyrillic text" to "indexes and finds non-ASCII
(Greek) text" (and the mixed-script variant likewise). Branch name
stays as-is to keep PR continuity but the shipped test fixtures are
language-neutral going forward.

11/11 search-index tests pass with the swap. Full suite 886/886.

---------

Signed-off-by: Nikita Nosov <20nik.nosov21@gmail.com>
Co-authored-by: Nikita Nosov <20nik.nosov21@gmail.com>
2026-05-13 15:27:43 +01:00

122 lines
3.8 KiB
TypeScript

import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
vi.mock("../src/logger.js", () => ({
logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() },
}));
vi.mock("../src/state/keyed-mutex.js", () => ({
withKeyedLock: <T>(_key: string, fn: () => Promise<T>) => fn(),
}));
import { registerRememberFunction } from "../src/functions/remember.js";
import { setVectorIndex, setEmbeddingProvider, getVectorIndex } from "../src/functions/search.js";
import { VectorIndex } from "../src/state/vector-index.js";
import type { EmbeddingProvider } from "../src/types.js";
function mockKV() {
const store = new Map<string, Map<string, unknown>>();
return {
get: async <T>(scope: string, key: string): Promise<T | null> =>
(store.get(scope)?.get(key) as T) ?? null,
set: async <T>(scope: string, key: string, data: T): Promise<T> => {
if (!store.has(scope)) store.set(scope, new Map());
store.get(scope)!.set(key, data);
return data;
},
delete: async (scope: string, key: string): Promise<void> => {
store.get(scope)?.delete(key);
},
list: async <T>(scope: string): Promise<T[]> => {
const entries = store.get(scope);
return entries ? (Array.from(entries.values()) as T[]) : [];
},
};
}
function mockSdk() {
const functions = new Map<string, Function>();
return {
registerFunction: (id: string, handler: Function) => {
functions.set(id, handler);
},
registerTrigger: () => {},
trigger: async (input: { function_id: string; payload: unknown }) => {
const fn = functions.get(input.function_id);
if (!fn) throw new Error(`unknown fn ${input.function_id}`);
return fn(input.payload);
},
};
}
describe("vector index population on remember", () => {
const mockEmbedder: EmbeddingProvider = {
name: "test",
dimensions: 3,
embed: async (_text: string) => new Float32Array([0.1, 0.2, 0.3]),
embedBatch: async (_texts: string[]) =>
_texts.map(() => new Float32Array([0.1, 0.2, 0.3])),
};
let vectorIndex: VectorIndex;
beforeEach(() => {
vectorIndex = new VectorIndex();
setVectorIndex(vectorIndex);
setEmbeddingProvider(mockEmbedder);
});
afterEach(() => {
setVectorIndex(null);
setEmbeddingProvider(null);
});
it("calls vectorIndex.add() when remember saves a memory", async () => {
const sdk = mockSdk();
const kv = mockKV();
registerRememberFunction(sdk as never, kv as never);
const result = await sdk.trigger({
function_id: "mem::remember",
payload: { content: "Test memory for vector indexing", type: "fact" },
});
expect((result as { success: boolean }).success).toBe(true);
expect(vectorIndex.size).toBe(1);
});
it("calls vectorIndex.add() with short content (0% similarity dedup)", async () => {
const sdk = mockSdk();
const kv = mockKV();
registerRememberFunction(sdk as never, kv as never);
await sdk.trigger({
function_id: "mem::remember",
payload: { content: "First unique memory", type: "fact" },
});
await sdk.trigger({
function_id: "mem::remember",
payload: { content: "Second completely different memory", type: "fact" },
});
expect(vectorIndex.size).toBe(2);
});
it("handles missing embedder gracefully (vectorIndex stays null)", async () => {
// Override beforeEach setup: this case wants null state explicitly.
setVectorIndex(null);
setEmbeddingProvider(null);
const sdk = mockSdk();
const kv = mockKV();
registerRememberFunction(sdk as never, kv as never);
const result = await sdk.trigger({
function_id: "mem::remember",
payload: { content: "This should work without vector index", type: "fact" },
});
expect((result as { success: boolean }).success).toBe(true);
expect(getVectorIndex()).toBeNull();
});
});