PR Review — @HenryPengZou

Overall clean feature with well-structured CRUD. A few issues from security to correctness need attention before merge.

🔴 CRITICAL — Must Fix

1. Global memory_enabled setting is per-server, not per-user
server/routes/memory.js:89-105 — appSettingsDb.get('memory_enabled') is a single global key. If User A disables memory, it's disabled for all users. This should be a per-user setting (e.g. column in user_memories or a separate user-settings table).

2. syncMemoryFiles writes to shared ~/.claude/MEMORY.md — multi-user conflict
server/utils/memoryPrompt.js:68-79 — On a shared server, the last user to save overwrites ~/.claude/MEMORY.md and ~/.claude/CLAUDE.md with their memories. The file path is not user-scoped. Either namespace the file (e.g. MEMORY-{userId}.md), or drop file sync and rely solely on system prompt injection. If this is strictly single-user, document that assumption explicitly.

🟠 HIGH — Strongly Recommend Fixing

3. req.params.id not validated as integer
server/routes/memory.js — PUT :id, PATCH :id/toggle, DELETE :id pass req.params.id directly to DB methods without parsing. Add parseInt() + isNaN guard to reject non-numeric IDs.

4. Memory content injected without sanitization into system prompts
server/utils/memoryPrompt.js:31 — User-supplied m.content is interpolated directly into the system prompt. A malicious memory like "Ignore all prior instructions..." is a prompt injection vector. Consider:

• Limiting memory content length (e.g. 500 chars per entry)
• Capping total memory count per user (e.g. 50)
• Stripping markdown headings (#) that could confuse prompt structure

5. No content length validation on create/update
server/routes/memory.js:28,43 — Only checks !content.trim() but no max length. Users could store arbitrarily large memories, bloating every system prompt.

6. Delete has no confirmation
src/components/settings/MemoryContent.jsx:136 — handleDelete fires immediately on click. Add window.confirm() or a two-step delete to prevent accidental data loss.

🟡 MEDIUM — Suggested Improvements

• feat(chat): make file paths in agent replies clickable to open in editor #7 Hardcoded English strings in MemoryContent.jsx and Settings.jsx:1817. The codebase uses useTranslation() / t() everywhere else — these should too.
• [Bug]: Dropdown list UI adaptation issue #8 try { ... } catch (err) { throw err; } in every memoryDb method is a no-op. Remove or add meaningful error handling.
• feat: compute_node_guid_and_fix_ui #9 Gemini memory injection (gemini-cli.js:615-618) prepends to the user prompt instead of a system instruction — inconsistent with other providers.
• test: validate auto-pr-review skill #10 idx_user_memories_enabled indexes is_enabled alone (2 possible values, not selective). A composite index (user_id, is_enabled) would actually help, or just drop this index.
• [Bug]: Pull the newest code and the frontend went empty #11 No debounce on syncMemoryFiles — rapid CRUD triggers excessive filesystem writes.

✅ Looks Good

• DB migration is idempotent (CREATE TABLE IF NOT EXISTS)
• CRUD routes follow existing codebase patterns
• authenticateToken middleware correctly protects all endpoints
• syncMemoryFiles uses start/end markers to avoid clobbering existing CLAUDE.md
• Frontend component is clean with proper state management
• userId correctly threaded through all 4 providers in index.js

Verdict: Do not merge as-is. Fix #1 and #2 (critical design issues that will cause bugs with multiple users). #3–#6 are straightforward hardening. The rest are quality improvements.

@HenryPengZou

Thanks for the thorough review @Zhang-Henry! All critical and high-priority issues are addressed
in commits 2129a6c and 4f940e7.

🔴 CRITICAL

1 — memory_enabled is now per-user, not global

• Added memory_enabled column to the users table (with migration for existing DBs)
• Replaced appSettingsDb.get('memory_enabled') with memoryDb.getMemoryEnabled(userId) /
  setMemoryEnabled(userId, bool)
• Each user now independently controls their own memory toggle

2 — Multi-user file conflict fully resolved

• ~/.claude/MEMORY.md → renamed to ~/.claude/MEMORY-{userId}.md (user-scoped)
• Removed ~/.claude/CLAUDE.md managed section entirely — this was the last shared resource
  that could cause overwrites. It's no longer needed because Option A (web UI) injects memories via
  buildMemoryBlock() directly from the DB into system prompts and never reads any file.

🟠 HIGH

3 — req.params.id validated as integer

• Added parseId() helper with parseInt() + isNaN + positive check
• All :id routes (PUT, PATCH, DELETE) return 400 for non-numeric IDs

4 — Memory content sanitized against prompt injection

• Content is sanitized (markdown headings # stripped) before system prompt injection via
  sanitizeContent()
• Max 500 characters per memory (enforced in DB layer + API validation)
• Max 50 memories per user (enforced in memoryDb.create())

5 — Content length validation on create/update

• API returns 400 error if content exceeds 500 characters
• Frontend textareas have maxLength={500}

6 — Delete confirmation added

• handleDelete now calls window.confirm() before executing

🟡 MEDIUM

8 — Removed no-op try/catch wrappers

• All memoryDb methods now throw directly instead of try { ... } catch (err) { throw err; }

9 — Gemini injection documented

• Added comment explaining why Gemini injects memory into user prompt (CLI has no system
  instruction API, only --prompt flag)

10 — Index fixed

• Changed idx_user_memories_enabled(is_enabled) → composite
  idx_user_memories_user_enabled(user_id, is_enabled)

Not addressed (low priority)

• 7 (i18n) — Deferred; would require adding translation keys across the codebase
• 11 (debounce syncMemoryFiles) — Low impact for typical usage patterns; can add if needed