refactor: add entity ID and search param validation, rename textLemmatized field, update tests (#4843)

This commit is contained in:
Kartik
2026-04-15 20:57:09 +05:30
committed by GitHub
parent 9692726db4
commit e6d6276bb9
10 changed files with 667 additions and 41 deletions
+100 -13
View File
@@ -82,6 +82,58 @@ function rejectTopLevelEntityParams(
}
}
/**
* Validates and normalizes an entity ID.
* - Trims leading/trailing whitespace
* - Rejects empty or whitespace-only strings
* - Rejects strings containing internal whitespace
* @returns The trimmed entity ID, or undefined if input is undefined
* @throws Error if entity ID is invalid
*/
function validateAndTrimEntityId(
value: string | undefined,
name: string,
): string | undefined {
if (value === undefined) return undefined;
const trimmed = value.trim();
if (trimmed === "") {
throw new Error(
`Invalid ${name}: cannot be empty or whitespace-only. Provide a valid identifier.`,
);
}
if (/\s/.test(trimmed)) {
throw new Error(
`Invalid ${name}: cannot contain whitespace. Provide a valid identifier without spaces.`,
);
}
return trimmed;
}
/**
* Validates search parameters.
* @throws Error if threshold or topK are invalid
*/
function validateSearchParams(threshold?: number, topK?: number): void {
if (threshold !== undefined) {
if (typeof threshold !== "number" || isNaN(threshold)) {
throw new Error("threshold must be a valid number");
}
if (threshold < 0 || threshold > 1) {
throw new Error(
`Invalid threshold: ${threshold}. Must be between 0 and 1 (inclusive).`,
);
}
}
if (topK !== undefined) {
if (typeof topK !== "number" || isNaN(topK) || !Number.isInteger(topK)) {
throw new Error("topK must be a valid integer");
}
if (topK < 0) {
throw new Error(`Invalid topK: ${topK}. Must be a non-negative integer.`);
}
}
}
export class Memory {
private config: MemoryConfig;
private customInstructions: string | undefined;
@@ -276,6 +328,13 @@ export class Memory {
messages: string | Message[],
config: AddMemoryOptions,
): Promise<SearchResult> {
// Validate messages input
if (messages === undefined || messages === null) {
throw new Error(
"messages is required and cannot be undefined or null. Provide a string or array of messages.",
);
}
await this._ensureInitialized();
await this._captureEvent("add", {
message_count: Array.isArray(messages) ? messages.length : 1,
@@ -283,14 +342,12 @@ export class Memory {
has_filters: !!config.filters,
infer: config.infer,
});
const {
userId,
agentId,
runId,
metadata = {},
filters = {},
infer = true,
} = config;
const { metadata = {}, filters = {}, infer = true } = config;
// Validate and trim entity IDs
const userId = validateAndTrimEntityId(config.userId, "userId");
const agentId = validateAndTrimEntityId(config.agentId, "agentId");
const runId = validateAndTrimEntityId(config.runId, "runId");
// Convert camelCase entity params to snake_case for storage (matches API and search/getAll filters)
if (userId) filters.user_id = metadata.user_id = userId;
@@ -804,14 +861,32 @@ export class Memory {
// Reject top-level entity params - must use filters instead
rejectTopLevelEntityParams(config as Record<string, any>, "search");
// Validate search parameters (before applying defaults)
validateSearchParams(config.threshold, config.topK);
// Validate and trim entity IDs in filters
const normalizedFilters = config.filters
? {
...config.filters,
user_id: validateAndTrimEntityId(config.filters.user_id, "user_id"),
agent_id: validateAndTrimEntityId(
config.filters.agent_id,
"agent_id",
),
run_id: validateAndTrimEntityId(config.filters.run_id, "run_id"),
}
: {};
await this._ensureInitialized();
const { topK = 20, threshold = 0.1 } = config;
await this._captureEvent("search", {
query_length: query.length,
topK: config.topK,
topK,
has_filters: !!config.filters,
});
const { topK = 100, threshold = 0.1 } = config;
let effectiveFilters: Record<string, any> = { ...(config.filters || {}) };
let effectiveFilters: Record<string, any> = { ...normalizedFilters };
// Apply enhanced metadata filtering if advanced operators are detected
if (this._hasAdvancedOperators(effectiveFilters)) {
@@ -1113,11 +1188,23 @@ export class Memory {
// Reject top-level entity params - must use filters instead
rejectTopLevelEntityParams(config as Record<string, any>, "getAll");
// Validate topK if provided (before applying defaults)
validateSearchParams(undefined, config.topK);
await this._ensureInitialized();
const { topK = 100, filters = {} } = config;
const { topK = 20 } = config;
// Validate and trim entity IDs in filters
const filters = {
...(config.filters || {}),
user_id: validateAndTrimEntityId(config.filters?.user_id, "user_id"),
agent_id: validateAndTrimEntityId(config.filters?.agent_id, "agent_id"),
run_id: validateAndTrimEntityId(config.filters?.run_id, "run_id"),
};
await this._captureEvent("get_all", {
topK: topK,
topK,
has_user_id: !!filters.user_id,
has_agent_id: !!filters.agent_id,
has_run_id: !!filters.run_id,
+1 -1
View File
@@ -260,7 +260,7 @@ export class MemoryVectorStore implements VectorStore {
};
if (this.filterVector(memoryVector, filters)) {
const text = payload.text_lemmatized || payload.data || "";
const text = payload.textLemmatized || payload.data || "";
candidates.push({ id: row.id, payload, tokens: this.tokenize(text) });
}
}
@@ -186,9 +186,9 @@ export class PGVector implements VectorStore {
: "";
const searchQuery = `
SELECT id, ts_rank_cd(to_tsvector('simple', payload->>'text_lemmatized'), plainto_tsquery('simple', $1)) AS score, payload
SELECT id, ts_rank_cd(to_tsvector('simple', payload->>'textLemmatized'), plainto_tsquery('simple', $1)) AS score, payload
FROM ${this.collectionName}
WHERE to_tsvector('simple', payload->>'text_lemmatized') @@ plainto_tsquery('simple', $1)
WHERE to_tsvector('simple', payload->>'textLemmatized') @@ plainto_tsquery('simple', $1)
${filterClause}
ORDER BY score DESC
LIMIT $2
@@ -469,7 +469,7 @@ describe("Memory – auto-initialization", () => {
},
llm: {
provider: "openai",
config: { apiKey: "sk-fake", model: "gpt-4-turbo-preview" },
config: { apiKey: "sk-fake", model: "gpt-5-mini" },
},
historyDbPath: ":memory:",
disableHistory: true,
+1 -1
View File
@@ -75,7 +75,7 @@ function createMemory(overrides: Partial<MemoryConfig> = {}): Memory {
},
llm: {
provider: "openai",
config: { apiKey: "test-key", model: "gpt-4-turbo-preview" },
config: { apiKey: "test-key", model: "gpt-5-mini" },
},
historyDbPath: ":memory:",
...overrides,
+1 -1
View File
@@ -75,7 +75,7 @@ function createMemory(): Memory {
},
llm: {
provider: "openai",
config: { apiKey: "test-key", model: "gpt-4-turbo-preview" },
config: { apiKey: "test-key", model: "gpt-5-mini" },
},
historyDbPath: ":memory:",
});
+2 -2
View File
@@ -69,7 +69,7 @@ function createMemory(overrides: Partial<MemoryConfig> = {}): Memory {
},
llm: {
provider: "openai",
config: { apiKey: "test-key", model: "gpt-4-turbo-preview" },
config: { apiKey: "test-key", model: "gpt-5-mini" },
},
historyDbPath: ":memory:",
...overrides,
@@ -94,7 +94,7 @@ describe("Memory - Initialization", () => {
},
llm: {
provider: "openai",
config: { apiKey: "test-key", model: "gpt-4" },
config: { apiKey: "test-key", model: "gpt-5-mini" },
},
};
const mem = Memory.fromConfig(config);
@@ -0,0 +1,299 @@
/**
* Unit tests for OSS SDK input validation.
*
* Validates fixes for:
* - Undefined/null message handling in add()
* - Threshold bounds validation (must be 0-1) in search()
* - TopK validation (must be non-negative) in search() and getAll()
* - Whitespace-only entity ID rejection in add(), search(), getAll()
*/
/// <reference types="jest" />
import { Memory } from "../src/memory";
jest.setTimeout(15000);
// Mock Google modules to prevent @google/genai crash in CI
jest.mock("../src/embeddings/google", () => ({
GoogleEmbedder: jest.fn(),
}));
jest.mock("../src/llms/google", () => ({
GoogleLLM: jest.fn(),
}));
jest.mock("../src/llms/openai", () => ({
OpenAILLM: jest.fn().mockImplementation(() => ({
generateResponse: jest.fn().mockResolvedValue(
JSON.stringify({
memory: [{ id: "0", text: "test memory", attributed_to: "user" }],
}),
),
})),
}));
const mockEmbedding = new Array(1536).fill(0.1);
jest.mock("../src/embeddings/openai", () => ({
OpenAIEmbedder: jest.fn().mockImplementation(() => ({
embed: jest.fn().mockResolvedValue(mockEmbedding),
embedBatch: jest.fn().mockResolvedValue([mockEmbedding]),
})),
}));
describe("Memory Input Validation", () => {
let memory: Memory;
const testUserId = "test-user-validation";
beforeAll(async () => {
memory = new Memory({
version: "v1.1",
embedder: {
provider: "openai",
config: { apiKey: "test-key", model: "text-embedding-3-small" },
},
llm: {
provider: "openai",
config: { apiKey: "test-key", model: "gpt-5-mini" },
},
vectorStore: {
provider: "memory",
config: { collectionName: "validation-test" },
},
});
// Wait for initialization
await new Promise((resolve) => setTimeout(resolve, 2000));
});
afterAll(async () => {
try {
await memory.reset();
} catch (e) {
// ignore cleanup errors
}
});
describe("add() message validation", () => {
it("should throw error when messages is undefined", async () => {
await expect(
// @ts-ignore - intentionally passing undefined
memory.add(undefined, { userId: testUserId }),
).rejects.toThrow("messages is required");
});
it("should throw error when messages is null", async () => {
await expect(
// @ts-ignore - intentionally passing null
memory.add(null, { userId: testUserId }),
).rejects.toThrow("messages is required");
});
});
describe("search() threshold validation", () => {
it("should throw error when threshold > 1.0", async () => {
await expect(
memory.search("test query", {
filters: { user_id: testUserId },
threshold: 1.5,
}),
).rejects.toThrow("Invalid threshold");
});
it("should throw error when threshold = 1.1", async () => {
await expect(
memory.search("test query", {
filters: { user_id: testUserId },
threshold: 1.1,
}),
).rejects.toThrow("Invalid threshold");
});
it("should throw error when threshold is negative", async () => {
await expect(
memory.search("test query", {
filters: { user_id: testUserId },
threshold: -0.5,
}),
).rejects.toThrow("Invalid threshold");
});
it("should throw error when threshold = -0.1", async () => {
await expect(
memory.search("test query", {
filters: { user_id: testUserId },
threshold: -0.1,
}),
).rejects.toThrow("Invalid threshold");
});
it("should accept threshold = 0 (edge case)", async () => {
const result = await memory.search("test query", {
filters: { user_id: testUserId },
threshold: 0,
});
expect(result).toBeDefined();
expect(result.results).toBeDefined();
});
it("should accept threshold = 1.0 (edge case)", async () => {
const result = await memory.search("test query", {
filters: { user_id: testUserId },
threshold: 1.0,
});
expect(result).toBeDefined();
expect(result.results).toBeDefined();
});
it("should accept threshold = 0.5 (normal valid value)", async () => {
const result = await memory.search("test query", {
filters: { user_id: testUserId },
threshold: 0.5,
});
expect(result).toBeDefined();
expect(result.results).toBeDefined();
});
});
describe("search() topK validation", () => {
it("should throw error when topK is negative", async () => {
await expect(
memory.search("test query", {
filters: { user_id: testUserId },
topK: -5,
}),
).rejects.toThrow("Invalid topK");
});
it("should throw error when topK = -1", async () => {
await expect(
memory.search("test query", {
filters: { user_id: testUserId },
topK: -1,
}),
).rejects.toThrow("Invalid topK");
});
it("should accept topK = 0 (returns empty)", async () => {
const result = await memory.search("test query", {
filters: { user_id: testUserId },
topK: 0,
});
expect(result).toBeDefined();
expect(result.results).toBeDefined();
});
it("should accept topK = 20 (normal value)", async () => {
const result = await memory.search("test query", {
filters: { user_id: testUserId },
topK: 20,
});
expect(result).toBeDefined();
expect(result.results).toBeDefined();
});
});
describe("add() entity ID validation", () => {
it("should throw error when userId is whitespace-only", async () => {
await expect(
memory.add("test message", { userId: " " }),
).rejects.toThrow("Invalid userId");
});
it("should throw error when userId is tabs and newlines", async () => {
await expect(
memory.add("test message", { userId: "\t\n\t" }),
).rejects.toThrow("Invalid userId");
});
it("should throw error when agentId is whitespace-only", async () => {
await expect(
memory.add("test message", { agentId: " " }),
).rejects.toThrow("Invalid agentId");
});
it("should throw error when runId is whitespace-only", async () => {
await expect(
memory.add("test message", { runId: " " }),
).rejects.toThrow("Invalid runId");
});
it("should throw error when userId contains internal whitespace", async () => {
await expect(
memory.add("test message", { userId: "user 123" }),
).rejects.toThrow("Invalid userId: cannot contain whitespace");
});
it("should throw error when userId contains tab character", async () => {
await expect(
memory.add("test message", { userId: "user\t123" }),
).rejects.toThrow("Invalid userId: cannot contain whitespace");
});
it("should accept userId with leading/trailing whitespace (trimmed)", async () => {
// Should not throw - leading/trailing whitespace is trimmed
const result = await memory.add("test message", {
userId: " valid-user ",
});
expect(result).toBeDefined();
});
});
describe("search() filter entity ID validation", () => {
it("should throw error when user_id in filters is whitespace-only", async () => {
await expect(
memory.search("test query", {
filters: { user_id: " " },
}),
).rejects.toThrow("Invalid user_id");
});
it("should throw error when agent_id in filters is whitespace-only", async () => {
await expect(
memory.search("test query", {
filters: { agent_id: " " },
}),
).rejects.toThrow("Invalid agent_id");
});
it("should throw error when user_id contains internal whitespace", async () => {
await expect(
memory.search("test query", {
filters: { user_id: "user 123" },
}),
).rejects.toThrow("Invalid user_id: cannot contain whitespace");
});
it("should accept user_id with leading/trailing whitespace (trimmed)", async () => {
const result = await memory.search("test query", {
filters: { user_id: " valid-user " },
});
expect(result).toBeDefined();
expect(result.results).toBeDefined();
});
});
describe("getAll() validation", () => {
it("should throw error when user_id is whitespace-only", async () => {
await expect(
memory.getAll({ filters: { user_id: " " } }),
).rejects.toThrow("Invalid user_id");
});
it("should throw error when topK is negative", async () => {
await expect(
memory.getAll({ filters: { user_id: testUserId }, topK: -1 }),
).rejects.toThrow("Invalid topK");
});
it("should throw error when user_id contains internal whitespace", async () => {
await expect(
memory.getAll({ filters: { user_id: "user 123" } }),
).rejects.toThrow("Invalid user_id: cannot contain whitespace");
});
it("should accept user_id with leading/trailing whitespace (trimmed)", async () => {
const result = await memory.getAll({
filters: { user_id: " valid-user " },
});
expect(result).toBeDefined();
expect(result.results).toBeDefined();
});
});
});