24f6c5fb24
* 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>
122 lines
3.8 KiB
TypeScript
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();
|
|
});
|
|
});
|