feat: agentmemory v0.3.0 — semantic search, memory evolution, profiles#4
Merged
feat: agentmemory v0.3.0 — semantic search, memory evolution, profiles#4
Conversation
Add 10 major capabilities for production-grade agentic memory: - Hybrid BM25 + vector semantic search with Reciprocal Rank Fusion - 6 embedding providers (Gemini, OpenAI, Voyage, Cohere, OpenRouter, local) - Memory versioning with relationships (supersedes/extends/derives/contradicts) - Persistent BM25 + vector indices with debounced KV writes - Progressive disclosure search (compact results → expandable full details) - Project profiles (top concepts, files, conventions, common errors) - Chronological timeline navigation around anchor points - Auto-forget with TTL expiry and contradiction detection - Ordered provider fallback chain with automatic failover - Full JSON export/import round-trip (merge/replace/skip strategies) 17 new source files, 11 new test files, 14 modified files. 144 tests pass, build 39.88 KB. 10 MCP tools, 28 REST endpoints, 21 functions.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRelease v0.3.0: adds hybrid BM25+vector search with embedding providers and persistence, vector index and serialization, provider fallbacks and local xenova support, memory versioning/relations, six new SDK memory functions (search/timeline/profile/relations/export-import/auto-forget), expanded types/schemas, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HybridSearch
participant BM25 as BM25Index
participant Embedding as EmbeddingProvider
participant Vector as VectorIndex
participant KV as StateKV
Client->>HybridSearch: search(query, limit)
HybridSearch->>BM25: bm25.search(query, limit*2)
alt embedding & vector available
HybridSearch->>Embedding: embed(query)
Embedding-->>HybridSearch: queryEmbedding
HybridSearch->>Vector: vector.search(queryEmbedding)
Vector-->>HybridSearch: vectorResults
HybridSearch->>HybridSearch: combine BM25 + vector ranks (RRF)
else fallback to BM25
HybridSearch-->>HybridSearch: use BM25-only results (vectorScore=0)
end
HybridSearch->>KV: fetch observations for top results
KV-->>HybridSearch: observations
HybridSearch-->>Client: HybridSearchResult[] (combined, enriched)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
src/health/monitor.ts-16-18 (1)
16-18:⚠️ Potential issue | 🟡 MinorUnsafe cast from
unknowntostringmay cause downstream issues.If the event payload is not a string (e.g.,
undefined,null, or an object), casting it directly tostringwill silently propagate an unexpected value intoconnectionState. Consider adding validation:🛡️ Proposed fix with runtime check
sdk.on("connection_state", (state?: unknown) => { - connectionState = state as string; + connectionState = typeof state === "string" ? state : "unknown"; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/health/monitor.ts` around lines 16 - 18, The handler for sdk.on("connection_state") is unsafely casting an unknown to string; change it to validate the payload before assignment by checking typeof state === "string" (or explicitly handling null/undefined) and only then set connectionState = state, otherwise log or set a safe default/keep previous value; update the anonymous listener attached to sdk.on("connection_state", ...) and any code that relies on connectionState to expect that validation instead of a blind cast.src/state/vector-index.ts-5-7 (1)
5-7:⚠️ Potential issue | 🟡 MinorPotential byte alignment issue in
base64ToFloat32.
Buffer.from(b64, "base64")may return a Buffer withbyteOffset > 0(since Buffers can be slices of a shared ArrayBuffer pool). Using.bufferdirectly could include unintended bytes. Use explicit offset/length:🐛 Proposed fix
function base64ToFloat32(b64: string): Float32Array { - return new Float32Array(Buffer.from(b64, "base64").buffer); + const buf = Buffer.from(b64, "base64"); + return new Float32Array(buf.buffer, buf.byteOffset, buf.byteLength / 4); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/vector-index.ts` around lines 5 - 7, The base64ToFloat32 function currently uses new Float32Array(Buffer.from(b64, "base64").buffer) which can produce wrong results when the Buffer is a slice (non-zero byteOffset); update base64ToFloat32 to create a Uint8Array from the Buffer (or capture buffer.byteOffset and byteLength) and then build the Float32Array using the underlying ArrayBuffer with the correct byteOffset and byteLength (divide length by 4 for the Float32Array length) so the Float32Array views the exact bytes of the decoded base64.src/functions/evict.ts-132-149 (1)
132-149:⚠️ Potential issue | 🟡 MinorA memory can be double-counted and double-deleted in one pass.
If
forgetAfteris expired andisLatest === false, the same record increments both stats and triggers two deletes (Line [132]-Line [149]).💡 Proposed fix
for (const mem of memories) { if (mem.forgetAfter) { - const expiry = new Date(mem.forgetAfter).getTime(); - if (now > expiry) { + const expiry = Date.parse(mem.forgetAfter); + if (Number.isFinite(expiry) && now > expiry) { stats.expiredMemories++; if (!dryRun) { await kv.delete(KV.memories, mem.id).catch(() => {}); } + continue; } } if (mem.isLatest === false && mem.createdAt) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/evict.ts` around lines 132 - 149, The two independent blocks (the forgetAfter handling and the isLatest handling) can both operate on the same memory causing double-counting and two deletes; modify the logic in the evict function so that once a memory is identified as expired and deleted in the forgetAfter branch you skip further processing for that memory (e.g., set a local deleted flag or use continue/else) to prevent re-checking the isLatest branch; ensure you only increment the appropriate stats (stats.expiredMemories or stats.nonLatestMemories) and call kv.delete(KV.memories, mem.id) at most once per memory (replace the second await kv.delete(...) with a no-op when the deleted flag is set).test/smart-search.test.ts-152-159 (1)
152-159:⚠️ Potential issue | 🟡 MinorLimit test does not verify actual limit behavior.
The mock
searchFn(line 112) ignores the_limitparameter and always returns allsearchResults. The assertiontoBeLessThanOrEqual(2)always passes sincesearchResultshas exactly 2 items. This test doesn't verify that the limit is actually forwarded to the search function.🐛 Suggested fix to verify limit behavior
- const searchFn = async (_query: string, _limit: number) => searchResults; + const searchFn = vi.fn(async (_query: string, limit: number) => + searchResults.slice(0, limit) + ); registerSmartSearchFunction(sdk as never, kv as never, searchFn);Then update the limit test:
it("respects limit parameter in compact mode", async () => { const result = (await sdk.trigger("mem::smart-search", { query: "auth", limit: 1, })) as { mode: string; results: CompactSearchResult[] }; - expect(result.results.length).toBeLessThanOrEqual(2); + expect(result.results.length).toBe(1); + expect(searchFn).toHaveBeenCalledWith("auth", 1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smart-search.test.ts` around lines 152 - 159, The test "respects limit parameter in compact mode" is ineffective because the mock searchFn ignores its _limit param and always returns the full searchResults; update the mock searchFn (the mocked function referenced as searchFn around line 112) to respect and apply the _limit argument (e.g., return searchResults.slice(0, _limit) or similar) so the limit is actually forwarded, then change the assertion in the test that calls sdk.trigger("mem::smart-search", { query: "auth", limit: 1 }) to assert the result.results length matches the requested limit (e.g., toBeLessThanOrEqual(1) or toEqual(1) depending on desired behavior).src/providers/embedding/openrouter.ts-6-18 (1)
6-18:⚠️ Potential issue | 🟡 MinorHardcoded
dimensionsmay mismatch configurable model.The
dimensionsproperty is hardcoded to 1536, which is correct for the default modelopenai/text-embedding-3-small. However, if a user configuresOPENROUTER_EMBEDDING_MODELto a different model (e.g., one producing 768 or 3072 dimensions), the mismatch could cause issues in the vector index.Consider either:
- Documenting that only 1536-dimension models are supported
- Allowing dimension configuration via environment variable
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/embedding/openrouter.ts` around lines 6 - 18, The dimensions property in OpenRouterEmbeddingProvider is hardcoded to 1536 which can mismatch a custom model set via getEnvVar("OPENROUTER_EMBEDDING_MODEL"); update the class to derive dimensions from a new env var (e.g. OPENROUTER_EMBEDDING_DIMENSIONS) or a small model-to-dimension mapping: in the constructor (OpenRouterEmbeddingProvider) read and parse getEnvVar("OPENROUTER_EMBEDDING_DIMENSIONS") as an integer, validate it (>0) and assign it to this.dimensions, falling back to 1536 if not provided, and optionally warn or throw if the provided dimensions conflict with known model sizes for the model from getEnvVar("OPENROUTER_EMBEDDING_MODEL").src/functions/profile.ts-92-102 (1)
92-102:⚠️ Potential issue | 🟡 Minor
totalObservationsonly reflects observations from the most recent 20 sessions.
sessionCountincludes all project sessions, buttotalObservationsis calculated only from the 20 sessions processed in the loop. This inconsistency could be misleading—users might expect the total count to cover all sessions.Consider either:
- Documenting this behavior in the profile schema/response
- Summing
observationCountfrom allprojectSessionsfor accuracy📝 Option 2: Calculate total from all sessions
+ const totalObsAllSessions = projectSessions.reduce( + (sum, s) => sum + s.observationCount, + 0, + ); + const profile: ProjectProfile = { project: data.project, updatedAt: new Date().toISOString(), topConcepts, topFiles, conventions: extractConventions(topConcepts, topFiles), commonErrors: uniqueErrors, recentActivity: recentActivity.slice(0, 10), sessionCount: projectSessions.length, - totalObservations: totalObs, + totalObservations: totalObsAllSessions, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/profile.ts` around lines 92 - 102, totalObservations is currently computed from only the 20 sessions processed in the loop, causing inconsistency with sessionCount (which uses all projectSessions); update the calculation to sum observationCount across all projectSessions before building the ProjectProfile (use the existing projectSessions array and its observationCount field) so totalObservations reflects the true total for the project, then construct the ProjectProfile with that summed value for totalObservations.src/state/index-persistence.ts-22-31 (1)
22-31:⚠️ Potential issue | 🟡 MinorStale vector data may persist if the vector index becomes empty.
When
this.vector.size === 0, thesave()method skips writing the vector data but doesn't delete the existing "vectors" key. On subsequentload(), the stale vector data will be restored.Consider deleting the vector key when the index is empty:
🐛 Proposed fix
async save(): Promise<void> { if (this.timer) { clearTimeout(this.timer); this.timer = null; } await this.kv.set(KV.bm25Index, "data", this.bm25.serialize()); if (this.vector && this.vector.size > 0) { await this.kv.set(KV.bm25Index, "vectors", this.vector.serialize()); + } else { + await this.kv.delete(KV.bm25Index, "vectors").catch(() => {}); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/index-persistence.ts` around lines 22 - 31, The save() method currently skips writing vector data when this.vector.size === 0, leaving any previous "vectors" entry in the KV store and causing stale data to be reloaded; update save() to remove the stored vectors key when the index is empty (use this.kv.delete or equivalent for KV.bm25Index/"vectors") so that an empty this.vector results in no persisted vectors, and ensure load() will not restore stale vectors from KV; reference save(), load(), this.vector, KV.bm25Index, and the "vectors" key when making the change.src/providers/embedding/index.ts-23-38 (1)
23-38:⚠️ Potential issue | 🟡 MinorNon-null assertions may throw if
EMBEDDING_PROVIDERis forced without the corresponding API key.When
EMBEDDING_PROVIDERis explicitly set (e.g.,EMBEDDING_PROVIDER=gemini),detectEmbeddingProvider()returns that value without verifying the API key exists. The subsequentgetEnvVar("GEMINI_API_KEY")!will then beundefined, and the provider constructor will receive an empty string or throw.Consider adding validation or letting the provider constructors handle the missing key gracefully (they already throw, but the
!assertion is misleading).🛡️ Proposed fix
switch (detected) { case "gemini": - return new GeminiEmbeddingProvider(getEnvVar("GEMINI_API_KEY")!); + return new GeminiEmbeddingProvider(getEnvVar("GEMINI_API_KEY")); case "openai": - return new OpenAIEmbeddingProvider(getEnvVar("OPENAI_API_KEY")!); + return new OpenAIEmbeddingProvider(getEnvVar("OPENAI_API_KEY")); case "voyage": - return new VoyageEmbeddingProvider(getEnvVar("VOYAGE_API_KEY")!); + return new VoyageEmbeddingProvider(getEnvVar("VOYAGE_API_KEY")); case "cohere": - return new CohereEmbeddingProvider(getEnvVar("COHERE_API_KEY")!); + return new CohereEmbeddingProvider(getEnvVar("COHERE_API_KEY")); case "openrouter": - return new OpenRouterEmbeddingProvider(getEnvVar("OPENROUTER_API_KEY")!); + return new OpenRouterEmbeddingProvider(getEnvVar("OPENROUTER_API_KEY"));The provider constructors already handle missing keys by throwing descriptive errors, so the non-null assertion is unnecessary and potentially misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/embedding/index.ts` around lines 23 - 38, The switch uses non-null assertions on getEnvVar(...) which is misleading and can mask undefined when EMBEDDING_PROVIDER is forced; update the factory to either validate the API key before calling the provider constructors or remove the trailing "!" and let the provider constructors (e.g., GeminiEmbeddingProvider, OpenAIEmbeddingProvider, VoyageEmbeddingProvider, CohereEmbeddingProvider, OpenRouterEmbeddingProvider) handle missing keys and throw; reference detectEmbeddingProvider and getEnvVar to locate the logic and ensure you either check getEnvVar(...) !== undefined and throw a clear error there or pass the raw value into the constructors without using the non-null assertion.
🧹 Nitpick comments (24)
src/telemetry/setup.ts (1)
85-97: Consider adding a compile-time check for mapping completeness.The
as unknown asdouble assertion is a pragmatic workaround forObject.fromEntriesreturning{ [k: string]: V }, but it bypasses TypeScript's ability to verify that all interface keys are present in the mapping arrays. If a new field is added toCountersorHistogramsbut not to the corresponding*_NAMESarray, runtime access would returnundefinedwith no compile-time warning.One option is a type-level exhaustiveness check using a satisfies constraint on the arrays:
♻️ Optional: Add compile-time exhaustiveness check
-const COUNTER_NAMES: Array<[keyof Counters, string]> = [ +const COUNTER_NAMES = [ ["observationsTotal", "observations.total"], ["compressionSuccess", "compression.success"], // ... other entries ["profileGenerated", "profile.generated"], -]; +] as const satisfies ReadonlyArray<[keyof Counters, string]>;This ensures the array covers all keys of
Countersat compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/telemetry/setup.ts` around lines 85 - 97, Add a compile-time exhaustiveness check by typing the name arrays so TypeScript verifies they include every key of the target interfaces: change COUNTER_NAMES and HISTOGRAM_NAMES declarations to use a "satisfies" constraint against Record<keyof Counters, string> and Record<keyof Histograms, string> (or an equivalent mapped type) so missing keys produce a compile error; then you can build counters and histograms from those arrays (using Object.fromEntries or a reduce) and cast to Counters/Histograms without the unsafe "as unknown as" bypass—keep NOOP_COUNTER/NOOP_HISTOGRAM and meter.createCounter/meter.createHistogram logic the same, only adjust the array types and final cast to rely on the new compile-time check.src/providers/embedding/gemini.ts (1)
27-36: Consider adding a timeout to the fetch call.The fetch request has no timeout, which could cause the embedding operation to hang indefinitely if the Gemini API is unresponsive. This is especially important in batch scenarios where upstream consumers may be waiting.
♻️ Proposed fix with AbortSignal timeout
const response = await fetch(`${API_BASE}?key=${this.apiKey}`, { method: "POST", headers: { "Content-Type": "application/json" }, + signal: AbortSignal.timeout(30_000), body: JSON.stringify({ requests: chunk.map((t) => ({ model: "models/text-embedding-004", content: { parts: [{ text: t }] }, })), }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/embedding/gemini.ts` around lines 27 - 36, The fetch call that posts embeddings to `${API_BASE}?key=${this.apiKey}` has no timeout and can hang; wrap the request in an AbortController with a configurable timeout (e.g., a constant or class setting), start a timer that calls controller.abort() after the timeout, pass controller.signal to fetch, and clear the timer on success; update the code around the fetch invocation (the POST that builds requests from chunk and model "models/text-embedding-004") to handle aborts/errors gracefully (treat AbortError as a timeout and surface a clear error).src/functions/export-import.ts (2)
117-129: Clarify "merge" strategy semantics in documentation or code comments.The current "merge" strategy overwrites existing items with imported data (same as default
kv.setbehavior). This might be unexpected—users may interpret "merge" as "add new items only, preserve existing." Consider either:
- Renaming to "overwrite" or "upsert" for clarity, or
- Adding a code comment explaining the behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/export-import.ts` around lines 117 - 129, The "merge" strategy in the import loop (inside the for (const session of importData.sessions) block using the strategy variable, kv.set(KV.sessions, session.id, session)) actually overwrites existing sessions rather than preserving them; update the code or docs to make the semantics explicit: either rename the strategy value from "merge" to "overwrite" or "upsert" throughout the codebase (references: strategy, importData.sessions, kv.set, KV.sessions, stats.skipped) or add a concise inline comment above this loop explaining that "merge" will overwrite existing items (i.e., perform upsert/overwrite) so callers know the behavior.
96-115: Sequential deletes in "replace" strategy may be slow for large datasets.Each entity is deleted individually with separate async calls. For large memory stores, this could be slow. Consider batching or documenting expected performance characteristics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/export-import.ts` around lines 96 - 115, The "replace" branch in export-import.ts performs sequential deletes (see strategy === "replace", kv.list, kv.delete over existing, existingMem, existingSummaries and their observation loops) which will be slow for large datasets; change deletion to batched/parallel deletes: collect ids per collection (sessions, observations, memories, summaries), split into reasonable-size chunks, and issue concurrent delete operations (e.g., using Promise.all over chunked kv.delete calls or a kv.bulkDelete API if available) to reduce round-trips and improve throughput, ensuring you still await completion and handle errors per-chunk.src/state/schema.ts (1)
26-36: Consider case normalization for Jaccard similarity.The tokenization doesn't normalize case, so "Error" and "error" would be treated as distinct tokens. This could affect contradiction detection accuracy if memory content varies in capitalization.
♻️ Proposed fix with case normalization
export function jaccardSimilarity(a: string, b: string): number { - const setA = new Set(a.split(/\s+/).filter((t) => t.length > 2)); - const setB = new Set(b.split(/\s+/).filter((t) => t.length > 2)); + const setA = new Set(a.toLowerCase().split(/\s+/).filter((t) => t.length > 2)); + const setB = new Set(b.toLowerCase().split(/\s+/).filter((t) => t.length > 2)); if (setA.size === 0 && setB.size === 0) return 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/schema.ts` around lines 26 - 36, The jaccardSimilarity function currently treats differently-cased tokens as distinct; update the tokenization to normalize case before creating sets (e.g., call toLowerCase() on inputs or lowercase each token when building setA and setB) so "Error" and "error" match, keeping the existing length filter and return logic in jaccardSimilarity.package.json (1)
38-42: Consider upgradingzodto the latest major version.The dependency additions are sensible:
zodfor schema validation and@xenova/transformersas an optional dependency for local embeddings appropriately avoids forcing the heavy ML dependency on users. However,zodis currently pinned to^3.23.0while the latest stable version is4.3.6(as of Feb 2026). The@xenova/transformersversion^2.17.0is appropriate and will allow auto-updates to2.17.2(the latest). Consider upgradingzodto version 4.x after testing for any breaking changes in your validation schemas.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 38 - 42, Update the zod dependency in package.json from "^3.23.0" to a 4.x release (e.g. "^4.3.6"), run the project's test suite and type checks, and fix any breaking changes in validation schemas or usages (search for Zod types/usages in the codebase such as ZodSchema, z.object(), z.string(), parse/parseAsync, safeParse, and any custom refinements) to conform to Zod v4 APIs; after adjustments, update package-lock or yarn.lock and verify integration tests and build succeed.src/types.ts (1)
248-251: TightenEmbeddingConfig.providertyping.Line [249] is currently
string, so provider typos won’t be caught at compile time and will fail later at runtime. Consider a dedicated string-literal union for supported embedding providers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` around lines 248 - 251, The provider field on EmbeddingConfig is too permissive as plain string; define a string-literal union type (e.g., type SupportedEmbeddingProvider = 'openai' | 'huggingface' | 'cohere' | ...) listing the allowed embedding providers, replace provider?: string with provider?: SupportedEmbeddingProvider on the EmbeddingConfig interface, and update any usages/assignments to use the new union (or cast/validate inputs) so typos are caught at compile time; reference the EmbeddingConfig interface and the new SupportedEmbeddingProvider type when making the change.src/providers/fallback-chain.ts (1)
6-8: Fail fast for empty fallback chains.If
providersis empty, the object is constructed successfully and only fails on first call. Validate upfront to surface config errors earlier.💡 Proposed fix
constructor(private providers: MemoryProvider[]) { + if (providers.length === 0) { + throw new Error("Fallback chain requires at least one provider"); + } this.name = `fallback(${providers.map((p) => p.name).join(" -> ")})`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/fallback-chain.ts` around lines 6 - 8, The constructor in the FallbackChain class currently allows an empty providers array and defers failure; update the constructor to validate that the injected providers array (constructor parameter providers / this.providers) is non-empty and throw a clear, early Error if it's empty; preserve existing behavior of setting this.name using providers.map(...) when non-empty so callers immediately get a configuration error instead of failing on first method call.test/timeline.test.ts (1)
116-147: Add boundary tests for invalid window and malformed date anchors.Given the new index math paths, add cases for fractional/negative
before/afterand invalid ISO-like anchors to prevent runtime regressions.💡 Suggested additional tests
+ it("returns invalid_window for non-finite/fractional window inputs", async () => { + const result = (await sdk.trigger("mem::timeline", { + anchor: "2026-02-01T12:00:00Z", + before: 1.5, + after: -1, + })) as { entries: TimelineEntry[]; reason?: string }; + + expect(result.entries).toEqual([]); + expect(result.reason).toBe("invalid_window"); + }); + + it("returns invalid_anchor for malformed ISO anchor", async () => { + const result = (await sdk.trigger("mem::timeline", { + anchor: "2026-99-99", + })) as { entries: TimelineEntry[]; reason?: string }; + + expect(result.entries).toEqual([]); + expect(result.reason).toBe("invalid_anchor"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/timeline.test.ts` around lines 116 - 147, Add boundary tests in test/timeline.test.ts around the sdk.trigger("mem::timeline") calls to cover invalid window values and malformed anchors: add cases that call sdk.trigger with fractional (e.g., 0.5) and negative (e.g., -1) before/after values and assert the call rejects/throws, and add cases that pass malformed ISO-like anchors (e.g., "2026-02-30T12:00:00", "not-a-date") and assert the call rejects/throws or returns an empty entries array; target the same test harness and types (TimelineEntry, sdk.trigger("mem::timeline")) so failures surface if input validation or index math regressions occur.test/fallback-chain.test.ts (1)
44-93: Add an explicit empty-chain test case.Given constructor-level validation is important here, add one test for
new FallbackChainProvider([])behavior.💡 Suggested additional test
+ it("throws when initialized with an empty provider list", () => { + expect(() => new FallbackChainProvider([])).toThrow( + "Fallback chain requires at least one provider", + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fallback-chain.test.ts` around lines 44 - 93, Add a test that verifies constructor-level validation by asserting that instantiating FallbackChainProvider with an empty array throws: create a new unit test that calls new FallbackChainProvider([]) and expects it to throw (use rejects/toThrow or toThrow for sync construction) to ensure the constructor enforces non-empty providers; reference the FallbackChainProvider constructor in the test and assert an appropriate error is thrown.src/eval/schemas.ts (1)
83-87: Require at least one smart-search selector (queryorexpandIds).Current schema accepts
{}. That pushes an invalid request shape into runtime logic instead of rejecting at validation.💡 Proposed schema refinement
export const SmartSearchInputSchema = z.object({ query: z.string().optional(), expandIds: z.array(z.string()).optional(), limit: z.number().int().positive().optional(), -}); +}).refine((v) => Boolean(v.query) || (v.expandIds?.length ?? 0) > 0, { + message: "Either query or expandIds must be provided", +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eval/schemas.ts` around lines 83 - 87, SmartSearchInputSchema currently allows an empty object which bypasses validation; update SmartSearchInputSchema to require at least one selector by adding a refinement on the object (SmartSearchInputSchema) that checks that either query is a non-empty string or expandIds is a non-empty array (treat empty array as invalid), and provide a clear validation message (e.g., "either query or expandIds must be provided"). Keep existing types for query, expandIds and limit and add the refine/superRefine to enforce the presence rule.test/auto-forget.test.ts (1)
119-149: Assert deletion side effect in low-value eviction test.This test checks
result.lowValueObsbut not thatobs_oldwas actually removed frommem:obs:ses_1.💡 Proposed assertion
expect(result.lowValueObs).toContain("obs_old"); + const deleted = await kv.get("mem:obs:ses_1", "obs_old"); + expect(deleted).toBeNull();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/auto-forget.test.ts` around lines 119 - 149, The test "evicts low-value old observations" currently asserts that result.lowValueObs contains "obs_old" but doesn't verify the side-effect removal from the store; update the test to fetch the observation after running sdk.trigger (using the same kv API used to seed it, e.g., kv.get("mem:obs:ses_1", "obs_old")) and assert the entry is gone (null/undefined or not found) to ensure the eviction actually deleted the obs. Reference: the test block named "evicts low-value old observations", the seeded key "mem:obs:ses_1"/"obs_old", the kv.set used to create it, and the sdk.trigger("mem::auto-forget", {}) call.src/providers/embedding/cohere.ts (1)
21-45: Extract shared embedding HTTP helper to avoid provider drift.This request/parse/error path duplicates logic in
src/providers/embedding/voyage.ts(Lines 21-45 there). Centralizing timeout, error normalization, and output validation will reduce future inconsistencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/embedding/cohere.ts` around lines 21 - 45, The embedBatch implementation in Cohere (embedBatch, API_URL, this.apiKey) duplicates HTTP request/response/error parsing logic found in voyage.ts — extract a shared helper (e.g. requestEmbedding or fetchEmbeddings) that centralizes timeout handling, request headers/body construction, normalized error creation, JSON parsing and output validation (ensure embeddings is an array of number[] and throw a normalized Error if not), and then update Cohere.embedBatch and Voyage.embedBatch to call that helper and convert number[] to Float32Array; ensure the helper surface accepts provider-specific payload fields (model/input_type/texts) and returns number[][] so provider methods only map to Float32Array.test/export-import.test.ts (1)
177-205: Strengthen replace-strategy test to cover all cleared stores.This test only checks old session removal. Please also assert old observations/memories/summaries are gone to fully validate replace semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/export-import.test.ts` around lines 177 - 205, Update the "import with replace strategy clears existing data first" test to also assert that old observations, memories and summaries are removed after the replace import: after calling sdk.trigger("mem::import", { exportData, strategy: "replace" }) add checks using the same kv helper (kv.get/kv.list or appropriate store accessors) to confirm entries under the stores "mem:observations", "mem:memories" and "mem:summaries" that correspond to the pre-existing test fixtures are gone (e.g., expected keys like the existing observation id(s), memory id(s) and summary id(s) return null/empty), so the test verifies all four stores (sessions, observations, memories, summaries) were cleared.test/index-persistence.test.ts (1)
86-101: Minor: Timer advance value exceeds debounce threshold.Line 96 uses
5000mswhile the actualDEBOUNCE_MSinsrc/state/index-persistence.tsis3000ms. The test passes but using the exact threshold (or importing the constant) would make the test more precise and maintainable.♻️ Suggestion
- vi.advanceTimersByTime(5000); + vi.advanceTimersByTime(3000);Or better, export and import the constant:
import { DEBOUNCE_MS } from "../src/state/index-persistence.js"; // ... vi.advanceTimersByTime(DEBOUNCE_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index-persistence.test.ts` around lines 86 - 101, Update the test so its timer advance matches the actual debounce threshold: replace the hard-coded 5000ms with the DEBOUNCE_MS constant from src/state/index-persistence.ts (import DEBOUNCE_MS into test/index-persistence.test.ts) and use vi.advanceTimersByTime(DEBOUNCE_MS) before awaiting timers; this ensures the behavior of IndexPersistence.scheduleSave is tested against the real debounce value.test/profile.test.ts (1)
143-151: Cache test could verify data consistency.The test verifies
cached: truebut doesn't assert that the returned profile data matches the first call. This would catch bugs where caching returns stale or incorrect data.♻️ Suggested enhancement
it("returns cached profile if fresh", async () => { - await sdk.trigger("mem::profile", { project: "my-project" }); + const first = (await sdk.trigger("mem::profile", { project: "my-project" })) as { + profile: ProjectProfile; + cached: boolean; + }; const result = (await sdk.trigger("mem::profile", { project: "my-project", })) as { profile: ProjectProfile; cached: boolean }; expect(result.cached).toBe(true); + expect(result.profile.project).toBe(first.profile.project); + expect(result.profile.sessionCount).toBe(first.profile.sessionCount); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/profile.test.ts` around lines 143 - 151, Update the "returns cached profile if fresh" test to also assert that the profile payload returned from the second sdk.trigger("mem::profile", { project: "my-project" }) equals the profile returned by the first call: capture the first call's return (e.g., first = await sdk.trigger("mem::profile", { project: "my-project" }) as { profile: ProjectProfile; cached: boolean }) then call again to get result and assert both result.profile deep-equals first.profile in addition to expect(result.cached).toBe(true); this ensures the cached data (ProjectProfile) is consistent with the original.test/embedding-provider.test.ts (1)
6-48: Test coverage is incomplete for all embedding providers.The factory supports 6 providers (Gemini, OpenAI, Voyage, Cohere, OpenRouter, local) but only Gemini and OpenAI are tested. Consider adding tests for the remaining providers to ensure the factory correctly instantiates each one.
♻️ Suggested additional test cases
it("returns VoyageEmbeddingProvider when VOYAGE_API_KEY is set", () => { process.env["VOYAGE_API_KEY"] = "test-key"; const provider = createEmbeddingProvider(); expect(provider?.name).toBe("voyage"); }); it("returns CohereEmbeddingProvider when COHERE_API_KEY is set", () => { process.env["COHERE_API_KEY"] = "test-key"; const provider = createEmbeddingProvider(); expect(provider?.name).toBe("cohere"); }); it("returns OpenRouterEmbeddingProvider when OPENROUTER_API_KEY is set", () => { process.env["OPENROUTER_API_KEY"] = "test-key"; const provider = createEmbeddingProvider(); expect(provider?.name).toBe("openrouter"); }); it("returns LocalEmbeddingProvider when EMBEDDING_PROVIDER is 'local'", () => { process.env["EMBEDDING_PROVIDER"] = "local"; const provider = createEmbeddingProvider(); expect(provider?.name).toBe("local"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/embedding-provider.test.ts` around lines 6 - 48, The test suite for createEmbeddingProvider is missing coverage for Voyage, Cohere, OpenRouter and the local provider; add tests mirroring the existing Gemini/OpenAI tests that set process.env["VOYAGE_API_KEY"], process.env["COHERE_API_KEY"], process.env["OPENROUTER_API_KEY"], and process.env["EMBEDDING_PROVIDER"]="local" respectively, call createEmbeddingProvider(), and assert the returned provider is the expected class/name (VoyageEmbeddingProvider, CohereEmbeddingProvider, OpenRouterEmbeddingProvider, LocalEmbeddingProvider and/or provider?.name equals "voyage"/"cohere"/"openrouter"/"local"); place these new it(...) blocks alongside the existing tests and rely on the same beforeEach/afterEach env setup.test/relations.test.ts (1)
12-47: Consider extracting shared test utilities.The
mockKV()andmockSdk()helper functions are duplicated across multiple test files (relations.test.ts,profile.test.ts,smart-search.test.ts,index-persistence.test.ts). This increases maintenance burden when the mock interface needs to change.♻️ Suggested refactor: Extract to shared test helper
Create a shared test utility file:
// test/helpers/mock-kv.ts export function mockKV() { const store = new Map<string, Map<string, unknown>>(); return { get: async <T>(scope: string, key: string): Promise<T | null> => { return (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[]) : []; }, }; } export function mockSdk() { const functions = new Map<string, Function>(); return { registerFunction: (opts: { id: string }, handler: Function) => { functions.set(opts.id, handler); }, registerTrigger: () => {}, trigger: async (id: string, data: unknown) => { const fn = functions.get(id); if (!fn) throw new Error(`No function: ${id}`); return fn(data); }, getFunction: (id: string) => functions.get(id), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/relations.test.ts` around lines 12 - 47, Extract the duplicated mock helpers into a shared test helper module and import them where needed: move the mockKV and mockSdk functions into a new file (e.g., export mockKV and mockSdk) and update tests (relations.test.ts, profile.test.ts, smart-search.test.ts, index-persistence.test.ts) to import and use those exported functions instead of local copies; ensure the exported helpers preserve the same signatures (mockKV.get/set/delete/list and mockSdk.registerFunction/registerTrigger/trigger/getFunction) so existing tests compile without further changes.src/state/search-index.ts (1)
137-151: Consider adding validation indeserialize()for robustness.The
deserialize()method assumes the JSON structure is valid. Malformed data (e.g., from corrupted storage) would cause cryptic runtime errors. Consider adding basic validation or try-catch with meaningful error messages.🛡️ Suggested defensive deserialization
static deserialize(json: string): SearchIndex { const idx = new SearchIndex(); - const data = JSON.parse(json); + let data: { + entries?: [string, IndexEntry][]; + inverted?: [string, string[]][]; + docTerms?: [string, [string, number][]][]; + totalDocLength?: number; + }; + try { + data = JSON.parse(json); + } catch { + throw new Error("SearchIndex: invalid JSON in deserialize"); + } + if (!data.entries || !data.inverted || !data.docTerms) { + throw new Error("SearchIndex: missing required fields in serialized data"); + } for (const [key, val] of data.entries) { idx.entries.set(key, val); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/search-index.ts` around lines 137 - 151, SearchIndex.deserialize currently assumes JSON has the exact shape and will throw cryptic errors on malformed input; wrap JSON.parse in try-catch and validate the top-level object and required properties (entries, inverted, docTerms, totalDocLength) before using them, checking that entries is an array of [key, val], inverted is an array of [term, ids], docTerms is an array of [id, counts] and totalDocLength is a number; on validation failure throw a clear Error (or return a rejected result) describing which property is invalid so callers can handle corrupted storage gracefully. Use the existing method name SearchIndex.deserialize and the internal maps idx.entries, idx.invertedIndex, idx.docTermCounts when rebuilding after validation.src/providers/embedding/local.ts (1)
33-51: Potential race condition in lazy initialization.If multiple concurrent calls to
embed()orembedBatch()occur before the extractor is initialized, multiple pipelines could be created simultaneously due to the non-atomic check-then-set pattern.Consider using a promise-based lock to ensure single initialization:
🔒 Proposed fix
export class LocalEmbeddingProvider implements EmbeddingProvider { readonly name = "local"; readonly dimensions = 384; private extractor: Awaited<ReturnType<Pipeline>> | null = null; + private initPromise: Promise<Awaited<ReturnType<Pipeline>>> | null = null; // ... embed and embedBatch unchanged ... private async getExtractor() { if (this.extractor) return this.extractor; + if (this.initPromise) return this.initPromise; + this.initPromise = this.initExtractor(); + this.extractor = await this.initPromise; + return this.extractor; + } + + private async initExtractor() { let transformers: { pipeline: Pipeline }; try { // `@ts-ignore` - optional peer dependency transformers = await import("@xenova/transformers"); } catch { throw new Error( "Install `@xenova/transformers` for local embeddings: npm install `@xenova/transformers`", ); } - this.extractor = await transformers.pipeline( + return await transformers.pipeline( "feature-extraction", "Xenova/all-MiniLM-L6-v2", ); - return this.extractor; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/embedding/local.ts` around lines 33 - 51, getExtractor currently checks this.extractor then initializes, which can race when embed() or embedBatch() call concurrently; make initialization atomic by adding a Promise-based lock (e.g., this.extractorPromise) that is set immediately when initialization starts, await that same promise in concurrent callers, and clear it on failure. Specifically: in getExtractor set this.extractorPromise before awaiting import("@xenova/transformers") and pipeline("feature-extraction", ...), assign this.extractor when successful and resolve the promise, and on catch reject/clear this.extractorPromise so future calls can retry; ensure embed() and embedBatch() rely on getExtractor() as before so all callers await the single initialization promise.src/providers/embedding/openai.ts (1)
21-44: Consider adding a request timeout for resilience.The
fetchcall has no timeout configured. In production, network issues could cause requests to hang indefinitely, blocking the embedding pipeline.⏱️ Proposed fix using AbortSignal
async embedBatch(texts: string[]): Promise<Float32Array[]> { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30_000); + const response = await fetch(API_URL, { method: "POST", headers: { Authorization: `Bearer ${this.apiKey}`, "Content-Type": "application/json", }, body: JSON.stringify({ model: "text-embedding-3-small", input: texts, }), + signal: controller.signal, }); + clearTimeout(timeoutId); if (!response.ok) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/embedding/openai.ts` around lines 21 - 44, The embedBatch function currently calls fetch without a timeout; update embedBatch to create an AbortController, pass controller.signal to fetch (using API_URL and this.apiKey as before), and start a timer (e.g., 10s or a configurable value) that calls controller.abort() on expiry; ensure you clear the timer after fetch completes or errors so it doesn't leak. Keep existing error handling for non-ok responses, but surface abort errors clearly (they will be thrown when controller aborts) and ensure the controller and timer are created/cleared inside embedBatch to avoid cross-request interference.test/hybrid-search.test.ts (1)
55-67: Consider adding a test for hybrid search with a mock vector index.The current tests cover BM25-only scenarios thoroughly, but there's no test for when a vector index is provided. This would exercise the reciprocal rank fusion (RRF) logic and ensure
vectorScorecontributes tocombinedScore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/hybrid-search.test.ts` around lines 55 - 67, Add a new unit test that constructs a HybridSearch instance with a mocked vector index to exercise RRF/score fusion: create an observation via makeObs and index it into bm25 and into a simple mock vector index that returns a non-empty vector result for the query, set the kv with kv.set as in the existing test, instantiate HybridSearch(bm25, mockVectorIndex, null, kv as never), call hybrid.search("auth"), and assert that results include the observation and that vectorScore > 0 and combinedScore reflects contribution from both vectorScore and bm25Score (e.g., combinedScore > bm25Score); reference HybridSearch, search, vectorScore, bm25Score, and the existing makeObs/bm25/kv usage to place the test alongside the BM25-only test.src/state/hybrid-search.ts (1)
104-112: Parallelize enrichment KV reads to reduce latency.These lookups are independent; sequential awaits increase tail latency for larger limits.
♻️ Proposed refactor
- const enriched: HybridSearchResult[] = []; - for (const r of results.slice(0, limit)) { - const obs = await this.kv.get<CompressedObservation>( - KV.observations(r.sessionId), - r.obsId, - ); - if (obs) { - enriched.push({ observation: obs, ...r }); - } - } - return enriched; + const enriched = await Promise.all( + results.slice(0, limit).map(async (r) => { + const obs = await this.kv.get<CompressedObservation>( + KV.observations(r.sessionId), + r.obsId, + ); + return obs ? ({ observation: obs, ...r } as HybridSearchResult) : null; + }), + ); + return enriched.filter((r): r is HybridSearchResult => r !== null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/hybrid-search.ts` around lines 104 - 112, The loop is doing sequential awaits for enrichment which increases latency; change the enrichment in getEnriched observations to run KV reads in parallel by mapping results.slice(0, limit) to an array of promises that call this.kv.get with KV.observations(r.sessionId) and r.obsId, then await Promise.all on that array, filter out missing results, and push/return enriched entries (maintaining the shape { observation: obs, ...r }); update references around results, limit, this.kv.get, KV.observations, CompressedObservation, and enriched to use the resolved promise array instead of sequential awaits.src/functions/smart-search.ts (1)
32-33: Avoid re-listing sessions for every expanded ID.You can fetch sessions once per request and reuse them. Current flow performs redundant KV list scans.
♻️ Proposed refactor
if (data.expandIds && data.expandIds.length > 0) { + const sessions = await kv.list<{ id: string }>(KV.sessions); const expanded: Array<{ obsId: string; sessionId: string; observation: CompressedObservation; }> = []; @@ for (const obsId of data.expandIds) { - const obs = await findObservation(kv, obsId); + const obs = await findObservation(kv, sessions, obsId); @@ async function findObservation( kv: StateKV, + sessions: Array<{ id: string }>, obsId: string, ): Promise<CompressedObservation | null> { - const sessions = await kv.list<{ id: string }>(KV.sessions); for (const session of sessions) {Also applies to: 75-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/smart-search.ts` around lines 32 - 33, The code currently calls KV/session listing repeatedly inside the loop over data.expandIds (around findObservation(kv, obsId)); instead, call the session-listing function once (e.g., listSessions or whatever code calls kv.list/kv.listSessions) before the loop, store the sessions result in a local variable, and then reuse that variable inside the loop when processing each obsId and in the similar block around lines 75-80; move session fetching out of both loops and reference the cached sessions variable instead of re-listing.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
package.jsonsrc/config.tssrc/eval/schemas.tssrc/functions/auto-forget.tssrc/functions/consolidate.tssrc/functions/context.tssrc/functions/evict.tssrc/functions/export-import.tssrc/functions/profile.tssrc/functions/relations.tssrc/functions/remember.tssrc/functions/smart-search.tssrc/functions/timeline.tssrc/health/monitor.tssrc/index.tssrc/mcp/server.tssrc/providers/embedding/cohere.tssrc/providers/embedding/gemini.tssrc/providers/embedding/index.tssrc/providers/embedding/local.tssrc/providers/embedding/openai.tssrc/providers/embedding/openrouter.tssrc/providers/embedding/voyage.tssrc/providers/fallback-chain.tssrc/providers/index.tssrc/state/hybrid-search.tssrc/state/index-persistence.tssrc/state/schema.tssrc/state/search-index.tssrc/state/vector-index.tssrc/telemetry/setup.tssrc/triggers/api.tssrc/types.tssrc/xenova.d.tstest/auto-forget.test.tstest/embedding-provider.test.tstest/export-import.test.tstest/fallback-chain.test.tstest/hybrid-search.test.tstest/index-persistence.test.tstest/profile.test.tstest/relations.test.tstest/smart-search.test.tstest/timeline.test.tstest/vector-index.test.ts
Comment on lines
+98
to
+104
| export function loadEmbeddingConfig(): EmbeddingConfig { | ||
| const env = getMergedEnv(); | ||
| return { | ||
| provider: env["EMBEDDING_PROVIDER"] || undefined, | ||
| bm25Weight: parseFloat(env["BM25_WEIGHT"] || "0.4"), | ||
| vectorWeight: parseFloat(env["VECTOR_WEIGHT"] || "0.6"), | ||
| }; |
There was a problem hiding this comment.
Validate embedding weights before returning config.
Lines 102-103 can return NaN (or invalid ranges), which can break hybrid score math downstream.
💡 Proposed fix
export function loadEmbeddingConfig(): EmbeddingConfig {
const env = getMergedEnv();
+ const bm25Weight = Number.parseFloat(env["BM25_WEIGHT"] || "0.4");
+ const vectorWeight = Number.parseFloat(env["VECTOR_WEIGHT"] || "0.6");
+ const safeBm25 = Number.isFinite(bm25Weight) ? bm25Weight : 0.4;
+ const safeVector = Number.isFinite(vectorWeight) ? vectorWeight : 0.6;
return {
provider: env["EMBEDDING_PROVIDER"] || undefined,
- bm25Weight: parseFloat(env["BM25_WEIGHT"] || "0.4"),
- vectorWeight: parseFloat(env["VECTOR_WEIGHT"] || "0.6"),
+ bm25Weight: safeBm25,
+ vectorWeight: safeVector,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function loadEmbeddingConfig(): EmbeddingConfig { | |
| const env = getMergedEnv(); | |
| return { | |
| provider: env["EMBEDDING_PROVIDER"] || undefined, | |
| bm25Weight: parseFloat(env["BM25_WEIGHT"] || "0.4"), | |
| vectorWeight: parseFloat(env["VECTOR_WEIGHT"] || "0.6"), | |
| }; | |
| export function loadEmbeddingConfig(): EmbeddingConfig { | |
| const env = getMergedEnv(); | |
| const bm25Weight = Number.parseFloat(env["BM25_WEIGHT"] || "0.4"); | |
| const vectorWeight = Number.parseFloat(env["VECTOR_WEIGHT"] || "0.6"); | |
| const safeBm25 = Number.isFinite(bm25Weight) ? bm25Weight : 0.4; | |
| const safeVector = Number.isFinite(vectorWeight) ? vectorWeight : 0.6; | |
| return { | |
| provider: env["EMBEDDING_PROVIDER"] || undefined, | |
| bm25Weight: safeBm25, | |
| vectorWeight: safeVector, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config.ts` around lines 98 - 104, In loadEmbeddingConfig, validate the
parsed bm25Weight and vectorWeight values before returning: after parsing in
loadEmbeddingConfig, check Number.isFinite for each and if NaN fallback to the
defaults (0.4 for bm25Weight, 0.6 for vectorWeight), clamp each value into the
[0,1] range, and if their sum is not >0 normalize them so they sum to 1 (or
revert to the defaults if normalization is impossible); update the returned
object to use these validated/normalized values (referencing
loadEmbeddingConfig, bm25Weight, and vectorWeight).
Comment on lines
+113
to
+124
| export const ExportImportInputSchema = z.object({ | ||
| exportData: z.object({ | ||
| version: z.literal("0.3.0"), | ||
| exportedAt: z.string(), | ||
| sessions: z.array(z.unknown()), | ||
| observations: z.record(z.string(), z.array(z.unknown())), | ||
| memories: z.array(z.unknown()), | ||
| summaries: z.array(z.unknown()), | ||
| profiles: z.array(z.unknown()).optional(), | ||
| }), | ||
| strategy: z.enum(["merge", "replace", "skip"]).optional(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file to confirm the code state
wc -l src/eval/schemas.tsRepository: rohitg00/agentmemory
Length of output: 87
🏁 Script executed:
# Read the specific section mentioned in the review
sed -n '100,130p' src/eval/schemas.tsRepository: rohitg00/agentmemory
Length of output: 828
🏁 Script executed:
# Search for usages of ExportImportInputSchema
rg "ExportImportInputSchema" -A 3 -B 3Repository: rohitg00/agentmemory
Length of output: 379
🏁 Script executed:
# Look for related import/export logic and how data is used after validation
rg "importData|exportData|export.*import" -t ts -A 5Repository: rohitg00/agentmemory
Length of output: 8836
🏁 Script executed:
# Let's find the ExportData type definition
rg "type ExportData|interface ExportData" -A 10Repository: rohitg00/agentmemory
Length of output: 480
🏁 Script executed:
# Read the full export-import.ts file to understand the import logic
cat -n src/functions/export-import.tsRepository: rohitg00/agentmemory
Length of output: 6618
🏁 Script executed:
# Also check for Session, CompressedObservation, Memory, SessionSummary type definitions
rg "type Session|interface Session|type Memory|interface Memory|type CompressedObservation|interface CompressedObservation|type SessionSummary|interface SessionSummary" -A 5Repository: rohitg00/agentmemory
Length of output: 1481
Replace z.unknown() with schemas matching the TypeScript interface definitions.
The schema validates only version and exportedAt; sessions, observations, memories, and summaries bypass structural validation via z.unknown(). This allows malformed data to be written directly to KV state without catching missing or invalid required fields (e.g., missing id on Session, invalid type on Memory).
Define Zod schemas for Session, CompressedObservation, Memory, and SessionSummary that match the TypeScript interfaces, or import existing Zod validators if available elsewhere in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/eval/schemas.ts` around lines 113 - 124, The ExportImportInputSchema
currently uses z.unknown() for sessions, observations, memories, and
summaries—replace those with concrete Zod validators that match the TypeScript
interfaces: create or import Session schema (for entries in sessions array),
CompressedObservation schema (for values in observations record arrays), Memory
schema (for entries in memories array), and SessionSummary schema (for entries
in summaries array), then use those schemas in ExportImportInputSchema (e.g.,
sessions: z.array(SessionSchema), observations: z.record(z.string(),
z.array(CompressedObservationSchema)), memories: z.array(MemorySchema),
summaries: z.array(SessionSummarySchema). Ensure optional profiles remains
unchanged or is similarly validated if a Profile schema exists.
Comment on lines
+21
to
+45
| async embedBatch(texts: string[]): Promise<Float32Array[]> { | ||
| const response = await fetch(API_URL, { | ||
| method: "POST", | ||
| headers: { | ||
| Authorization: `Bearer ${this.apiKey}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| model: "voyage-code-3", | ||
| input: texts, | ||
| input_type: "document", | ||
| }), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const err = await response.text(); | ||
| throw new Error(`Voyage embedding failed (${response.status}): ${err}`); | ||
| } | ||
|
|
||
| const data = (await response.json()) as { | ||
| data: Array<{ embedding: number[] }>; | ||
| }; | ||
|
|
||
| return data.data.map((d) => new Float32Array(d.embedding)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full file structure and the specific file
find . -name "voyage.ts" -type fRepository: rohitg00/agentmemory
Length of output: 99
🏁 Script executed:
# Read the full voyage.ts file to understand the complete implementation
cat -n ./src/providers/embedding/voyage.tsRepository: rohitg00/agentmemory
Length of output: 1699
🏁 Script executed:
# Check if there are other embedding providers to see if they implement timeout/validation
find ./src/providers/embedding -name "*.ts" -type f | head -10Repository: rohitg00/agentmemory
Length of output: 317
🏁 Script executed:
# Check how other providers handle timeouts and response validation
cat -n ./src/providers/embedding/openai.tsRepository: rohitg00/agentmemory
Length of output: 1667
🏁 Script executed:
# Check another provider (Cohere) to see if they use timeouts or validation
cat -n ./src/providers/embedding/cohere.tsRepository: rohitg00/agentmemory
Length of output: 1678
🏁 Script executed:
# Check if there's a fetch wrapper or timeout configuration anywhere in the codebase
rg -i "timeout|AbortController" --type ts --type jsRepository: rohitg00/agentmemory
Length of output: 1698
🏁 Script executed:
# Check one of the hooks to see how timeout is implemented
cat -n ./src/hooks/pre-tool-use.tsRepository: rohitg00/agentmemory
Length of output: 2172
🏁 Script executed:
# Check the EmbeddingProvider type definition to understand the interface
cat -n ./src/types.tsRepository: rohitg00/agentmemory
Length of output: 7242
🏁 Script executed:
# Verify if embedBatch is called with known input counts to see if validation mismatch is possible
rg "embedBatch" --type ts -B 2 -A 2Repository: rohitg00/agentmemory
Length of output: 3692
🏁 Script executed:
# Check if there are tests for embedBatch to see expected behavior
find ./test -name "*embedding*" -o -name "*voyage*" | head -5Repository: rohitg00/agentmemory
Length of output: 97
🏁 Script executed:
# Read the embedding provider test file
cat -n ./test/embedding-provider.test.tsRepository: rohitg00/agentmemory
Length of output: 2242
🏁 Script executed:
# Check if there's any wrapper or middleware handling that might add timeouts
rg "VoyageEmbeddingProvider|embedBatch" --type ts -B 5 -A 5 | head -50Repository: rohitg00/agentmemory
Length of output: 3004
Add timeout and response validation to embedBatch method.
The fetch call lacks an abort timeout and can hang indefinitely. Additionally, the response processing doesn't validate that the embedding count matches the input count. Both are critical at the external API boundary.
Suggested fixes:
- Add
signal: AbortSignal.timeout(30000)(or appropriate duration) to the fetch options - Validate response structure and count: check that
data.dataexists anddata.data.length === texts.lengthbefore mapping
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/providers/embedding/voyage.ts` around lines 21 - 45, The embedBatch
method's fetch can hang and its response isn't validated; update embedBatch to
add a timeout by passing signal: AbortSignal.timeout(30000) (or chosen ms) into
the fetch options, and after parsing JSON validate that the parsed object has a
data array (e.g., check typeof data === "object" && Array.isArray(data.data))
and that data.data.length === texts.length, throwing a descriptive Error if not;
also verify each item has an embedding array before converting to new
Float32Array(d.embedding) to avoid runtime errors.
Comment on lines
+39
to
+40
| const queryEmbedding = await this.embeddingProvider.embed(query); | ||
| const vectorResults = this.vector.search(queryEmbedding, limit * 2); |
There was a problem hiding this comment.
Fallback to BM25 when embedding/vector retrieval fails.
A transient embedding failure currently bubbles up and breaks search. Degrade to BM25 instead of failing the request.
💡 Proposed fix
- const queryEmbedding = await this.embeddingProvider.embed(query);
- const vectorResults = this.vector.search(queryEmbedding, limit * 2);
+ let vectorResults: Array<{ obsId: string; sessionId: string; score: number }>;
+ try {
+ const queryEmbedding = await this.embeddingProvider.embed(query);
+ vectorResults = this.vector.search(queryEmbedding, limit * 2);
+ } catch {
+ return this.enrichResults(
+ bm25Results.map((r) => ({
+ obsId: r.obsId,
+ sessionId: r.sessionId,
+ bm25Score: r.score,
+ vectorScore: 0,
+ combinedScore: r.score,
+ })),
+ limit,
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const queryEmbedding = await this.embeddingProvider.embed(query); | |
| const vectorResults = this.vector.search(queryEmbedding, limit * 2); | |
| let vectorResults: Array<{ obsId: string; sessionId: string; score: number }>; | |
| try { | |
| const queryEmbedding = await this.embeddingProvider.embed(query); | |
| vectorResults = this.vector.search(queryEmbedding, limit * 2); | |
| } catch { | |
| return this.enrichResults( | |
| bm25Results.map((r) => ({ | |
| obsId: r.obsId, | |
| sessionId: r.sessionId, | |
| bm25Score: r.score, | |
| vectorScore: 0, | |
| combinedScore: r.score, | |
| })), | |
| limit, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state/hybrid-search.ts` around lines 39 - 40, When embedding or vector
retrieval fails for queryEmbedding (this.embeddingProvider.embed) or
vector.search, catch the error and degrade to a BM25 text-only search instead of
letting the exception bubble up; update the search flow around queryEmbedding
and vector.search to try embedding/search in a try-catch, and on catch call your
BM25 search method (e.g., this.bm25.search or equivalent) with the original
query and limit so the request succeeds with BM25 results.
Critical: - auto-forget: filter TTL-deleted memories before contradiction pass - timeline: validate before/after params and anchor date - api: add auth check to profile endpoint Major: - config: NaN fallback for embedding weights - config: detectEmbeddingProvider uses explicit env when provided - relations: persist evolved memory before flipping old to non-latest - relations: get-related traverses KV.relations for full graph - remember: version increments from existing instead of hardcoded 2 - smart-search: validate query type and clamp limit 1-100 - hybrid-search: fallback to BM25 when embedding fails - evict: prevent double-counting expired + non-latest memories - index: log warnings instead of silently swallowing errors
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/functions/remember.ts (1)
34-49:⚠️ Potential issue | 🟠 MajorPersist the new memory before demoting the current latest to avoid lineage corruption.
Line [44]-Line [46] flips the existing record to non-latest before Line [71] writes the replacement. If the second write fails (or concurrent requests interleave), the chain can end up inconsistent. Also, Line [62]-Line [64] can use the superseded version captured during the scan instead of re-searching.
💡 Proposed fix
const existingMemories = await kv.list<Memory>(KV.memories); let supersededId: string | undefined; + let supersededVersion = 1; + let supersededMemory: Memory | undefined; const lowerContent = data.content.toLowerCase(); for (const existing of existingMemories) { - if (!existing.isLatest) continue; + if (existing.isLatest === false) continue; const similarity = jaccardSimilarity( lowerContent, existing.content.toLowerCase(), ); if (similarity > 0.7) { - existing.isLatest = false; - await kv.set(KV.memories, existing.id, existing); supersededId = existing.id; + supersededVersion = existing.version ?? 1; + supersededMemory = existing; break; } } @@ - version: supersededId - ? (existingMemories.find((m) => m.id === supersededId)?.version ?? - 1) + 1 - : 1, + version: supersededId ? supersededVersion + 1 : 1, parentId: supersededId, supersedes: supersededId ? [supersededId] : [], isLatest: true, }; await kv.set(KV.memories, memory.id, memory); + if (supersededMemory) { + supersededMemory.isLatest = false; + await kv.set(KV.memories, supersededMemory.id, supersededMemory); + }Also applies to: 62-64, 71-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/remember.ts` around lines 34 - 49, The current loop demotes the found existing memory (setting existing.isLatest = false and kv.set) before the new memory is persisted, risking lineage loss if the subsequent write fails; instead, when you detect a similar existing memory in the scan (using existingMemories, jaccardSimilarity, and Memory), capture and store that existing record in supersededId/supersededMemory, then first persist the new memory record to kv (kv.set for the new id), and only after the new memory is successfully written update the captured existing memory to isLatest = false and write it back; also avoid re-scanning by reusing the captured supersededMemory for the later update and ensure error handling/rollback if either write fails.src/index.ts (1)
152-163:⚠️ Potential issue | 🟡 MinorRebuild condition ignores empty vector index.
When BM25 is restored but the vector index is empty (or wasn't persisted), semantic search silently degrades to BM25-only mode without attempting to rebuild embeddings. This matches the concern from the previous review.
💡 Proposed fix: Include vector index in rebuild decision
- if (!loaded?.bm25 || loaded.bm25.size === 0) { + const needsBm25Rebuild = !loaded?.bm25 || loaded.bm25.size === 0; + const needsVectorRebuild = embeddingProvider && vectorIndex && vectorIndex.size === 0; + + if (needsBm25Rebuild || needsVectorRebuild) { const indexCount = await rebuildIndex(kv).catch((err) => { console.warn(`[agentmemory] Failed to rebuild search index:`, err); return 0; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 152 - 163, The rebuild condition only checks loaded?.bm25 and misses the case where the vector/semantic index is present but empty, so update the if condition to also detect an empty or missing vector index (the persisted embeddings/hnsw structure) and trigger rebuildIndex(kv) accordingly; modify the guard around loaded?.bm25 to also check the vector index property (e.g., loaded.vectorIndex, loaded.hnsw, or whatever field holds embeddings) for undefined/null or size/length === 0, then call rebuildIndex(kv) and keep the existing logging and indexPersistence.scheduleSave behavior.
🧹 Nitpick comments (4)
src/index.ts (1)
174-176: Consider logging persistence save failures on shutdown.The empty
.catch(() => {})silently swallows save errors. If persistence fails, the user won't know their index wasn't saved, potentially leading to unexpected rebuild times on next startup.📝 Proposed fix
indexPersistence.stop(); - await indexPersistence.save().catch(() => {}); + await indexPersistence.save().catch((err) => { + console.warn(`[agentmemory] Failed to save index on shutdown:`, err); + }); await sdk.shutdown();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 174 - 176, Replace the empty swallow on indexPersistence.save() with error handling that logs failures: call indexPersistence.stop() as before, then await indexPersistence.save() but catch the error and pass it to a logger (e.g., processLogger.error or console.error) with a clear message that persistence save failed; finally proceed to await sdk.shutdown(). Ensure you reference the existing indexPersistence.save() call and include the error object in the log so save failures are visible on shutdown.src/config.ts (2)
124-132: Unsafe type assertion on provider list.The
as FallbackConfig["providers"]cast trusts that all comma-separated values are validProviderTypeentries. Invalid provider names (e.g., typos like"opanai") will pass through and may cause runtime errors increateFallbackProvider.🛡️ Proposed fix: Validate provider names
+const VALID_PROVIDERS = new Set(["anthropic", "gemini", "openai", "openrouter", "cohere", "voyage"]); + export function loadFallbackConfig(): FallbackConfig { const env = getMergedEnv(); const raw = env["FALLBACK_PROVIDERS"] || ""; const providers = raw .split(",") .map((p) => p.trim()) - .filter(Boolean) as FallbackConfig["providers"]; + .filter((p): p is FallbackConfig["providers"][number] => + Boolean(p) && VALID_PROVIDERS.has(p) + ); return { providers }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 124 - 132, The loadFallbackConfig function currently casts the parsed FALLBACK_PROVIDERS list to FallbackConfig["providers"] unsafely; validate each comma-separated value from getMergedEnv()["FALLBACK_PROVIDERS"] against the authoritative ProviderType set (or allowedProviders array/enum) before returning so typos like "opanai" are rejected or filtered out. Update loadFallbackConfig to map/trim the values, keep only entries that exist in the ProviderType/allowedProviders lookup, and either log/throw on unknown names or exclude them, ensuring createFallbackProvider receives only valid ProviderType values.
100-106: Consider clamping weight values to valid range.The
isNaNfallback is good, but weights outside[0, 1]or weights that don't sum to a reasonable value could still produce unexpected RRF scores. For instance, negative weights would invert ranking.🛡️ Optional: Add range validation
export function loadEmbeddingConfig(): EmbeddingConfig { const env = getMergedEnv(); - const bm25Weight = parseFloat(env["BM25_WEIGHT"] || "0.4"); - const vectorWeight = parseFloat(env["VECTOR_WEIGHT"] || "0.6"); + let bm25Weight = parseFloat(env["BM25_WEIGHT"] || "0.4"); + let vectorWeight = parseFloat(env["VECTOR_WEIGHT"] || "0.6"); + bm25Weight = isNaN(bm25Weight) || bm25Weight < 0 ? 0.4 : Math.min(bm25Weight, 1); + vectorWeight = isNaN(vectorWeight) || vectorWeight < 0 ? 0.6 : Math.min(vectorWeight, 1); return { provider: env["EMBEDDING_PROVIDER"] || undefined, - bm25Weight: isNaN(bm25Weight) ? 0.4 : bm25Weight, - vectorWeight: isNaN(vectorWeight) ? 0.6 : vectorWeight, + bm25Weight, + vectorWeight, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 100 - 106, Clamp parsed weights to the [0,1] range and normalize them so they form a valid proportion: after reading bm25Weight and vectorWeight (from parseFloat and the current isNaN fallbacks), clamp each value to 0..1, then if both are zero revert to the defaults (0.4 and 0.6); otherwise divide each by (bm25Weight + vectorWeight) so they sum to 1. Update the returned bm25Weight and vectorWeight values accordingly to ensure neither is negative/out-of-range and the pair is a proper weighting.src/state/hybrid-search.ts (1)
117-128: Consider parallelizing KV fetches for better latency.The sequential
awaitloop fetches observations one at a time. Forlimit=20, this means 20 round trips. Parallelizing would reduce latency.⚡ Proposed parallel fetch
private async enrichResults( results: Array<{ obsId: string; sessionId: string; bm25Score: number; vectorScore: number; combinedScore: number; }>, limit: number, ): Promise<HybridSearchResult[]> { - const enriched: HybridSearchResult[] = []; - for (const r of results.slice(0, limit)) { - const obs = await this.kv.get<CompressedObservation>( + const sliced = results.slice(0, limit); + const observations = await Promise.all( + sliced.map((r) => + this.kv.get<CompressedObservation>( + KV.observations(r.sessionId), + r.obsId, + ), + ), + ); + const enriched: HybridSearchResult[] = []; + for (let i = 0; i < sliced.length; i++) { + const obs = observations[i]; + const r = sliced[i]; - KV.observations(r.sessionId), - r.obsId, - ); if (obs) { enriched.push({ observation: obs, ...r }); } } return enriched; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/hybrid-search.ts` around lines 117 - 128, The loop that builds enriched: HybridSearchResult[] performs sequential awaits which causes N round-trips; change it to fire all KV fetches in parallel by mapping results.slice(0, limit) to an array of promises calling this.kv.get<CompressedObservation>(KV.observations(r.sessionId), r.obsId), await Promise.all on that array, then combine the resolved observations with their corresponding result objects (from results.slice(0, limit)) to produce the enriched array, filtering out any missing obs before returning; update the method that contains enriched (the hybrid search result assembly) to use this pattern to reduce latency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/triggers/api.ts`:
- Around line 369-384: The api::smart-search handler registered via
sdk.registerFunction should perform the same parameter validation as
mem::smart-search before calling sdk.trigger: after authenticating with
checkAuth, check that either req.body.query is present OR req.body.expandIds is
a non-empty array, and if neither is provided return a 400 response with a body
like { error: "query or expandIds is required" }; keep the rest of the handler
(calling sdk.trigger("mem::smart-search", req.body) and returning 200)
unchanged.
---
Duplicate comments:
In `@src/functions/remember.ts`:
- Around line 34-49: The current loop demotes the found existing memory (setting
existing.isLatest = false and kv.set) before the new memory is persisted,
risking lineage loss if the subsequent write fails; instead, when you detect a
similar existing memory in the scan (using existingMemories, jaccardSimilarity,
and Memory), capture and store that existing record in
supersededId/supersededMemory, then first persist the new memory record to kv
(kv.set for the new id), and only after the new memory is successfully written
update the captured existing memory to isLatest = false and write it back; also
avoid re-scanning by reusing the captured supersededMemory for the later update
and ensure error handling/rollback if either write fails.
In `@src/index.ts`:
- Around line 152-163: The rebuild condition only checks loaded?.bm25 and misses
the case where the vector/semantic index is present but empty, so update the if
condition to also detect an empty or missing vector index (the persisted
embeddings/hnsw structure) and trigger rebuildIndex(kv) accordingly; modify the
guard around loaded?.bm25 to also check the vector index property (e.g.,
loaded.vectorIndex, loaded.hnsw, or whatever field holds embeddings) for
undefined/null or size/length === 0, then call rebuildIndex(kv) and keep the
existing logging and indexPersistence.scheduleSave behavior.
---
Nitpick comments:
In `@src/config.ts`:
- Around line 124-132: The loadFallbackConfig function currently casts the
parsed FALLBACK_PROVIDERS list to FallbackConfig["providers"] unsafely; validate
each comma-separated value from getMergedEnv()["FALLBACK_PROVIDERS"] against the
authoritative ProviderType set (or allowedProviders array/enum) before returning
so typos like "opanai" are rejected or filtered out. Update loadFallbackConfig
to map/trim the values, keep only entries that exist in the
ProviderType/allowedProviders lookup, and either log/throw on unknown names or
exclude them, ensuring createFallbackProvider receives only valid ProviderType
values.
- Around line 100-106: Clamp parsed weights to the [0,1] range and normalize
them so they form a valid proportion: after reading bm25Weight and vectorWeight
(from parseFloat and the current isNaN fallbacks), clamp each value to 0..1,
then if both are zero revert to the defaults (0.4 and 0.6); otherwise divide
each by (bm25Weight + vectorWeight) so they sum to 1. Update the returned
bm25Weight and vectorWeight values accordingly to ensure neither is
negative/out-of-range and the pair is a proper weighting.
In `@src/index.ts`:
- Around line 174-176: Replace the empty swallow on indexPersistence.save() with
error handling that logs failures: call indexPersistence.stop() as before, then
await indexPersistence.save() but catch the error and pass it to a logger (e.g.,
processLogger.error or console.error) with a clear message that persistence save
failed; finally proceed to await sdk.shutdown(). Ensure you reference the
existing indexPersistence.save() call and include the error object in the log so
save failures are visible on shutdown.
In `@src/state/hybrid-search.ts`:
- Around line 117-128: The loop that builds enriched: HybridSearchResult[]
performs sequential awaits which causes N round-trips; change it to fire all KV
fetches in parallel by mapping results.slice(0, limit) to an array of promises
calling this.kv.get<CompressedObservation>(KV.observations(r.sessionId),
r.obsId), await Promise.all on that array, then combine the resolved
observations with their corresponding result objects (from results.slice(0,
limit)) to produce the enriched array, filtering out any missing obs before
returning; update the method that contains enriched (the hybrid search result
assembly) to use this pattern to reduce latency.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/config.tssrc/functions/auto-forget.tssrc/functions/evict.tssrc/functions/relations.tssrc/functions/remember.tssrc/functions/smart-search.tssrc/functions/timeline.tssrc/index.tssrc/state/hybrid-search.tssrc/triggers/api.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/functions/smart-search.ts
- src/functions/evict.ts
- src/functions/timeline.ts
- src/functions/relations.ts
- src/functions/auto-forget.ts
- remember: persist new memory before demoting old (write ordering) - remember: capture superseded version during scan (no re-search) - api: validate query/expandIds on smart-search endpoint (return 400) - index: rebuild when vector index is empty (not just BM25) - index: log persistence save failures on shutdown - config: clamp embedding weights to [0, 1] - config: validate fallback provider names against known set - hybrid-search: parallelize KV fetches with Promise.all
This was referenced Feb 27, 2026
6 tasks
This was referenced Mar 18, 2026
This was referenced Apr 4, 2026
Merged
This was referenced Apr 13, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
forgetAfterfield + Jaccard contradiction detection (threshold 0.9)Changes
v0.2.0 → v0.3.0 comparison
Test plan
npm run build— 0 type errors, 39.88 KB bundlenpm test— 144 tests pass, 14 integration skipped (needs running server)GEMINI_API_KEY→ logs "Embedding provider: gemini (768 dims)"POST /agentmemory/smart-searchreturns compact resultsPOST /agentmemory/timelinereturns chronological observationsGET /agentmemory/profile?project=...returns aggregated profileGET /agentmemory/export→POST /agentmemory/importround-tripPOST /agentmemory/auto-forget?dryRun=trueshows candidatesSummary by CodeRabbit
New Features
Chores