Conversation
WalkthroughAdds a memory feature: new Express memory routes with admin auth, a MemoryService singleton using embeddings and DB functions, a Postgres migration with pgvector and search functions, and Zod schemas (data + API) exported from shared-schemas. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@backend/src/api/routes/memory/index.routes.ts`:
- Around line 26-37: Prettier is flagging formatting in the router.post handler;
reformat the block around the POST '/conversations' route so it complies with
project Prettier rules (fix indentation, line breaks and trailing commas) —
ensure the async handler signature (AuthRequest, Response, NextFunction), the
validation call to storeConversationRequestSchema.safeParse(req.body), and the
AppError throw (using ERROR_CODES.INVALID_INPUT) are each placed/indented
according to the repo's Prettier config so CI lint passes; run Prettier (or the
project's format script) on backend/src/api/routes/memory/index.routes.ts to
apply the changes.
- Around line 39-41: The userId value pulled from req.body or req.query (const
userId = req.body?.userId || req.user?.id || DEFAULT_USER_ID) is bypassing
schema validation; update the request validation to include userId in the route
schema or add a short guard that validates/coerces req.body.userId and
req.query.userId (type string, allowed format/length) before using it, and only
fall back to req.user?.id or DEFAULT_USER_ID after validation; apply the same
change for the other occurrences of userId in this file where you read
req.body?.userId or req.query?.userId to ensure consistent validation and
prevent unexpected types.
- Around line 67-72: Validate the route parameter id as a UUID before calling
memoryService.getConversation or memoryService.deleteConversation: parse
req.params.id (e.g., using zod.string().uuid() or a UUID regex) and if invalid
throw new AppError('Invalid id', 400, ERROR_CODES.INVALID_INPUT) so the handler
returns 400 instead of letting Postgres error; ensure you perform this check in
the GET and DELETE handlers that use req.params.id (and still fall back to
userId = (req.query?.userId as string) || req.user?.id || DEFAULT_USER_ID).
In `@backend/src/services/memory/memory.service.ts`:
- Around line 216-220: The logger.info calls that currently include the raw
search text (the invocation using logger.info with fields userId, resultCount:
conversations.length, query: data.query.substring(0, 50)) should be changed to
avoid logging plaintext queries; replace the query field with a safe
representation such as the query length, a truncated/redacted preview (e.g.,
first N characters plus "...") or a deterministic hash (e.g., SHA-256) of
data.query, and do the same for the other logger.info instance referenced (also
applies to the block around the second occurrence). Ensure you still include
userId and resultCount but never the full data.query; compute the hash/preview
before logging and use that variable in the logger.info call.
- Around line 101-158: storeConversation currently opens a DB transaction before
calling generateEmbedding, which holds locks while doing external API calls; fix
it by resolving the embeddingModel via getEmbeddingModel, then compute
summaryEmbedding (from generateConversationSummary) and all per-message
embeddings (e.g., map messages -> generateEmbedding, optionally with
Promise.all) before calling this.dbManager.getPool().connect(); only after
embeddings are ready, connect, BEGIN the transaction, and perform the INSERTs
using the precomputed summaryEmbedding and message embeddings (referencing
storeConversation, generateEmbedding, getEmbeddingModel,
generateConversationSummary, and the message loop) so the transaction only wraps
DB inserts.
- Around line 197-203: The MemoryService methods are not setting the Postgres
role context used by RLS (app.user_id), causing INSERT/DELETE to fail and some
SELECTs to bypass RLS; update MemoryService to set the session-local context
before any memory table operations (e.g., in storeConversation, the delete
method, and the query paths that call dbManager.getPool().query) by executing a
SET LOCAL/ set_config('app.user_id', <userId>, true) on the same
transaction/connection (or reuse the RealtimeAuthService pattern) so every
subsequent query in that transaction uses the correct app.user_id for RLS
enforcement.
🧹 Nitpick comments (2)
shared-schemas/src/memory-api.schema.ts (1)
38-45: Avoiddefault().optional()if defaults should apply.
In Zod, wrapping a default with.optional()bypasses the default onundefined. If you want defaults, drop.optional()(or drop.default()and keep optional).♻️ Suggested tweak
- limit: z.number().int().positive().max(100).default(10).optional(), + limit: z.number().int().positive().max(100).default(10), - threshold: z - .number() - .min(0) - .max(1) - .default(0) - .optional() + threshold: z + .number() + .min(0) + .max(1) + .default(0)Apply the same change in
searchMessagesRequestSchema.Also applies to: 66-73
backend/src/services/memory/memory.service.ts (1)
16-18: Make the default embedding model configurable and verify availability.
Hard-coding the model couples this service to a specific provider/model name; consider env/config override and confirm the model is supported by OpenRouter/AI config.As per coding guidelines, prefer environment-based configuration.♻️ Example tweak
-const DEFAULT_EMBEDDING_MODEL = 'openai/text-embedding-3-small'; +const DEFAULT_EMBEDDING_MODEL = + process.env.DEFAULT_EMBEDDING_MODEL || 'openai/text-embedding-3-small';
| // Use userId from body, or JWT user, or default to 'system' | ||
| const userId = req.body?.userId || req.user?.id || DEFAULT_USER_ID; | ||
|
|
There was a problem hiding this comment.
Validate userId sourced from body/query.
userId bypasses schema validation here; consider adding it to the request schema or validating it separately to avoid unexpected types and align with API contracts.
Also applies to: 68-69, 91-92, 116-117, 141-142
🤖 Prompt for AI Agents
In `@backend/src/api/routes/memory/index.routes.ts` around lines 39 - 41, The
userId value pulled from req.body or req.query (const userId = req.body?.userId
|| req.user?.id || DEFAULT_USER_ID) is bypassing schema validation; update the
request validation to include userId in the route schema or add a short guard
that validates/coerces req.body.userId and req.query.userId (type string,
allowed format/length) before using it, and only fall back to req.user?.id or
DEFAULT_USER_ID after validation; apply the same change for the other
occurrences of userId in this file where you read req.body?.userId or
req.query?.userId to ensure consistent validation and prevent unexpected types.
| const { id } = req.params; | ||
| const userId = (req.query?.userId as string) || req.user?.id || DEFAULT_USER_ID; | ||
|
|
||
| const conversation = await memoryService.getConversation(userId, id); | ||
| if (!conversation) { | ||
| throw new AppError('Conversation not found', 404, ERROR_CODES.NOT_FOUND); |
There was a problem hiding this comment.
Validate :id as UUID before DB calls.
Invalid UUIDs will surface as 500s from Postgres. Consider parsing params and returning 400 with INVALID_INPUT.
🛠️ Example adjustment (apply in GET and DELETE)
- const { id } = req.params;
+ const idValidation = z.string().uuid().safeParse(req.params.id);
+ if (!idValidation.success) {
+ throw new AppError('Invalid conversation id', 400, ERROR_CODES.INVALID_INPUT);
+ }
+ const { data: id } = idValidation;Note: add import { z } from 'zod'; if you use this.
Also applies to: 90-94
🤖 Prompt for AI Agents
In `@backend/src/api/routes/memory/index.routes.ts` around lines 67 - 72, Validate
the route parameter id as a UUID before calling memoryService.getConversation or
memoryService.deleteConversation: parse req.params.id (e.g., using
zod.string().uuid() or a UUID regex) and if invalid throw new AppError('Invalid
id', 400, ERROR_CODES.INVALID_INPUT) so the handler returns 400 instead of
letting Postgres error; ensure you perform this check in the GET and DELETE
handlers that use req.params.id (and still fall back to userId =
(req.query?.userId as string) || req.user?.id || DEFAULT_USER_ID).
| const result = await this.dbManager.getPool().query(sql, [ | ||
| userId, | ||
| JSON.stringify(queryEmbedding), | ||
| data.limit || 10, | ||
| data.threshold || 0, | ||
| data.metadataFilter ? JSON.stringify(data.metadataFilter) : null, | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the app sets the RLS user context anywhere.
rg -n "app\.user_id|set_config\('app\.user_id'|SET LOCAL app\.user_id" backend/srcRepository: InsForge/InsForge
Length of output: 598
🏁 Script executed:
# Let's also look at the memory.service.ts file to understand the query execution pattern
wc -l backend/src/services/memory/memory.service.tsRepository: InsForge/InsForge
Length of output: 110
🏁 Script executed:
# Look for database initialization and connection pooling setup
rg -n "getPool|dbManager|pg\.Pool" backend/src --type ts -A 3Repository: InsForge/InsForge
Length of output: 50373
🏁 Script executed:
# Search for database migrations that might define RLS policies
fd -t f "*.sql" backend --exec grep -l "app\.user_id\|CURRENT_SETTING\|RLS\|ROW LEVEL SECURITY" {} \;Repository: InsForge/InsForge
Length of output: 345
🏁 Script executed:
# Check for any middleware or context setting for app.user_id
rg -n "SET LOCAL|set_config|app\.user_id" backend/src --type ts --type js -B 2 -A 2Repository: InsForge/InsForge
Length of output: 2086
🏁 Script executed:
# Read memory.service.ts to see the full context around the flagged lines
sed -n '100,120p' backend/src/services/memory/memory.service.tsRepository: InsForge/InsForge
Length of output: 879
🏁 Script executed:
# Check the query methods around lines 197, 246, 290, 341
sed -n '190,210p' backend/src/services/memory/memory.service.tsRepository: InsForge/InsForge
Length of output: 664
🏁 Script executed:
# Check lines around 246
sed -n '240,260p' backend/src/services/memory/memory.service.tsRepository: InsForge/InsForge
Length of output: 704
🏁 Script executed:
# Check lines around 290 and 341
sed -n '285,295p' backend/src/services/memory/memory.service.ts
sed -n '335,350p' backend/src/services/memory/memory.service.tsRepository: InsForge/InsForge
Length of output: 959
🏁 Script executed:
# Search for any place where app.user_id might be set globally in backend
rg "app\.user_id|'app\.user_id'|\"app\.user_id\"" backend/src --type ts -B 2 -A 2Repository: InsForge/InsForge
Length of output: 43
🏁 Script executed:
# Look for DatabaseManager implementation to see if there's a hook
find backend/src -name "*database*.ts" -type f | head -5Repository: InsForge/InsForge
Length of output: 301
🏁 Script executed:
# Check DatabaseManager implementation
cat backend/src/infra/database/database.manager.tsRepository: InsForge/InsForge
Length of output: 5422
🏁 Script executed:
# Check if storeConversation sets app.user_id (full method)
sed -n '96,180p' backend/src/services/memory/memory.service.tsRepository: InsForge/InsForge
Length of output: 2716
🏁 Script executed:
# Search for any middleware or context setting for database operations
rg "set_config|SET LOCAL|current_setting" backend/src --type ts -B 3 -A 3 | head -80Repository: InsForge/InsForge
Length of output: 3340
🏁 Script executed:
# Check if there's any pool initialization hook
rg "Pool|afterConnect|middleware" backend/src/infra/database --type ts -B 2 -A 2Repository: InsForge/InsForge
Length of output: 1366
🏁 Script executed:
# Let me verify the exact RLS policy implementation to confirm the issue
sed -n '180,210p' backend/src/infra/database/migrations/023_create-memory-schema.sqlRepository: InsForge/InsForge
Length of output: 1301
🏁 Script executed:
# Check if there's any middleware setting app.user_id before request reaches memory service
rg "app\.user_id" backend/src --type ts --type js -B 3 -A 3Repository: InsForge/InsForge
Length of output: 43
🏁 Script executed:
# Verify the search_conversations and search_messages functions check app.user_id
sed -n '190,220p' backend/src/infra/database/migrations/023_create-memory-schema.sqlRepository: InsForge/InsForge
Length of output: 908
🏁 Script executed:
# Check if get_conversation_with_messages function or search functions set app.user_id internally
grep -n "set_config\|SET LOCAL" backend/src/infra/database/migrations/023_create-memory-schema.sqlRepository: InsForge/InsForge
Length of output: 43
🏁 Script executed:
# Look at the search_conversations and search_messages functions to see if they handle the RLS context
sed -n '250,290p' backend/src/infra/database/migrations/023_create-memory-schema.sqlRepository: InsForge/InsForge
Length of output: 403
🏁 Script executed:
# Let me search for where userId parameter is used in the search functions - maybe it sets context
grep -A 30 "CREATE OR REPLACE FUNCTION memory.search_conversations" backend/src/infra/database/migrations/023_create-memory-schema.sql | head -40Repository: InsForge/InsForge
Length of output: 823
🏁 Script executed:
# Check the full migration file size and look for search_conversations and search_messages functions
wc -l backend/src/infra/database/migrations/023_create-memory-schema.sqlRepository: InsForge/InsForge
Length of output: 131
🏁 Script executed:
# Find the search_conversations function
grep -n "search_conversations\|search_messages" backend/src/infra/database/migrations/023_create-memory-schema.sqlRepository: InsForge/InsForge
Length of output: 174
🏁 Script executed:
# Let me see the full search_conversations function to understand the complete picture
sed -n '89,130p' backend/src/infra/database/migrations/023_create-memory-schema.sqlRepository: InsForge/InsForge
Length of output: 1256
🏁 Script executed:
# Also check search_messages function
sed -n '131,170p' backend/src/infra/database/migrations/023_create-memory-schema.sqlRepository: InsForge/InsForge
Length of output: 1142
🏁 Script executed:
# Check if RLS is enabled on the memory tables
grep -n "ALTER TABLE.*memory\." backend/src/infra/database/migrations/023_create-memory-schema.sql | head -20Repository: InsForge/InsForge
Length of output: 183
🏁 Script executed:
# Check the complete flow - are there any other places where app.user_id is set in memory operations
rg "storeConversation|getConversation|deleteConversation|searchConversation" backend/src --type ts -B 2 -A 5 | head -60Repository: InsForge/InsForge
Length of output: 4501
🏁 Script executed:
# Check if storeConversation with BEGIN transaction is subject to RLS on INSERT
sed -n '96,135p' backend/src/services/memory/memory.service.tsRepository: InsForge/InsForge
Length of output: 1435
Set app.user_id context for RLS policies on all memory table operations.
The migration enforces RLS policies using current_setting('app.user_id'), but MemoryService never sets this context. This causes INSERT operations in storeConversation (line 105+) and DELETE at line 341 to fail RLS policy checks, and direct queries at lines 197, 246, 290 bypass RLS enforcement entirely. Use SET LOCAL app.user_id = $1 within transactions or a connection hook like RealtimeAuthService does with set_config().
Also applies to: 246–252, 290–291, 341–342
🤖 Prompt for AI Agents
In `@backend/src/services/memory/memory.service.ts` around lines 197 - 203, The
MemoryService methods are not setting the Postgres role context used by RLS
(app.user_id), causing INSERT/DELETE to fail and some SELECTs to bypass RLS;
update MemoryService to set the session-local context before any memory table
operations (e.g., in storeConversation, the delete method, and the query paths
that call dbManager.getPool().query) by executing a SET LOCAL/
set_config('app.user_id', <userId>, true) on the same transaction/connection (or
reuse the RealtimeAuthService pattern) so every subsequent query in that
transaction uses the correct app.user_id for RLS enforcement.
9c34447 to
24ff7f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@shared-schemas/src/memory-api.schema.ts`:
- Around line 1-5: Prettier is failing due to multi-line import formatting;
change the import statement so each import is on a single line: keep "import { z
} from 'zod';" and collapse the multi-line import of
conversationWithMessagesSchema and conversationSearchResultSchema from
'./memory.schema' into a single-line import, updating the import that references
conversationWithMessagesSchema and conversationSearchResultSchema accordingly so
the file compiles and satisfies the formatter.
♻️ Duplicate comments (1)
backend/src/services/memory/memory.service.ts (1)
195-199: Avoid logging raw query text.The search query may contain sensitive user content. Consider logging only the query length or a hash instead of the substring.
🛡️ Safer logging example
logger.info('Conversation search completed', { userId, resultCount: conversations.length, - query: data.query.substring(0, 50), + queryLength: data.query.length, });
🧹 Nitpick comments (1)
backend/src/infra/database/migrations/023_create-memory-schema.sql (1)
20-31: Consider adding a foreign key constraint onuser_id.The
user_idcolumn is defined asTEXTwithout a foreign key reference to a users table. This allows orphaned memory data if users are deleted. If a users table with a compatible key exists, consider adding a foreign key withON DELETE CASCADEto maintain referential integrity.Also, for security-sensitive deployments, consider whether Row-Level Security (RLS) policies should be added to enforce user isolation at the database level, rather than relying solely on application-layer checks.
| import { z } from 'zod'; | ||
| import { | ||
| conversationWithMessagesSchema, | ||
| conversationSearchResultSchema, | ||
| } from './memory.schema'; |
There was a problem hiding this comment.
Fix Prettier formatting to pass CI.
The pipeline is failing due to import formatting. The multi-line import should be a single line per Prettier configuration.
🔧 Proposed fix
import { z } from 'zod';
-import {
- conversationWithMessagesSchema,
- conversationSearchResultSchema,
-} from './memory.schema';
+import { conversationWithMessagesSchema, conversationSearchResultSchema } from './memory.schema';📝 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.
| import { z } from 'zod'; | |
| import { | |
| conversationWithMessagesSchema, | |
| conversationSearchResultSchema, | |
| } from './memory.schema'; | |
| import { z } from 'zod'; | |
| import { conversationWithMessagesSchema, conversationSearchResultSchema } from './memory.schema'; |
🧰 Tools
🪛 GitHub Actions: Lint and Format Check
[error] 2-2: prettier/prettier: Replace ⏎··conversationWithMessagesSchema,⏎··conversationSearchResultSchema,⏎ with ·conversationWithMessagesSchema,·conversationSearchResultSchema·
🪛 GitHub Check: lint-and-format
[failure] 2-2:
Replace ⏎··conversationWithMessagesSchema,⏎··conversationSearchResultSchema,⏎ with ·conversationWithMessagesSchema,·conversationSearchResultSchema·
🤖 Prompt for AI Agents
In `@shared-schemas/src/memory-api.schema.ts` around lines 1 - 5, Prettier is
failing due to multi-line import formatting; change the import statement so each
import is on a single line: keep "import { z } from 'zod';" and collapse the
multi-line import of conversationWithMessagesSchema and
conversationSearchResultSchema from './memory.schema' into a single-line import,
updating the import that references conversationWithMessagesSchema and
conversationSearchResultSchema accordingly so the file compiles and satisfies
the formatter.
Summary
How did you test this change?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.