feat(memory): add self-evolving memory ecosystem (ADR-001)#69
Conversation
|
Kilo Code Review could not run — your account is out of credits. Add credits or switch to a free model to enable reviews on this change. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Memory Evolution system, including API routes, agent adapters, observability, scheduling loops, and core services for managing memory candidates, consolidation, decay, and evaluation. The review feedback highlights several critical security, reliability, and performance issues. These include a command injection vulnerability in Redis synchronization, storing raw encryption keys in the database, a missing database table definition, an invalid SQLite JSON path syntax, potential concurrent execution issues with setInterval, and inefficient O(N^2) tokenization in loops.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const { execSync } = await import('child_process'); | ||
| const pass = process.env.REDIS_PASSWORD || '62778f211595bf46081ca824ada432011bdd500959d93b2222a910280e942005'; | ||
| const payload = Object.entries(event).map(([k,v]) => k+' '+v).join(' '); | ||
| execSync("docker exec redis sh -c 'redis-cli -a "+pass+" XADD memory_events * agent_id "+agentId+" "+payload+"'", { timeout: 5000 }); |
There was a problem hiding this comment.
The syncToRedis method uses execSync with direct string interpolation of agentId and payload into a shell command. This creates a critical Command Injection vulnerability if an attacker can control or manipulate the agent ID or event metadata.
Additionally, spawning a shell process on every sync is highly inefficient.
Remediation:
Use spawnSync with an array of arguments to execute the command safely without a shell, preventing command injection entirely.
const { spawnSync } = await import('child_process');
const pass = process.env.REDIS_PASSWORD || '62778f211595bf46081ca824ada432011bdd500959d93b2222a910280e942005';
const args = ['exec', 'redis', 'redis-cli', '-a', pass, 'XADD', 'memory_events', '*', 'agent_id', agentId];
for (const [k, v] of Object.entries(event)) {
args.push(k, v);
}
spawnSync('docker', args, { timeout: 5000 });| private ensureTables(): void { | ||
| this.db.exec("CREATE TABLE IF NOT EXISTS memory_evolution_leases (id TEXT PRIMARY KEY, loop_run_id TEXT NOT NULL, role TEXT NOT NULL CHECK(role IN ('memory_evaluator','memory_consolidator','memory_pruner')), runtime TEXT NOT NULL DEFAULT 'local', status TEXT NOT NULL DEFAULT 'prepared' CHECK(status IN ('prepared','running','completed','failed')), memory_candidate_id TEXT, metadata TEXT NOT NULL DEFAULT '{}', created_at TEXT NOT NULL, updated_at TEXT NOT NULL);CREATE INDEX IF NOT EXISTS idx_mel_loop_run ON memory_evolution_leases(loop_run_id);CREATE INDEX IF NOT EXISTS idx_mel_role ON memory_evolution_leases(role);CREATE INDEX IF NOT EXISTS idx_mel_candidate ON memory_evolution_leases(memory_candidate_id);"); | ||
| } |
There was a problem hiding this comment.
The memory_candidates table is queried and updated across multiple services (e.g., MemoryEvolutionService, MemoryEvolutionScheduler, MemoryEvolutionObservability), but it is never created in schema.ts or any ensureTables method. This will cause immediate runtime SQL errors (no such table: memory_candidates) on almost all API endpoints and background loops.
Please ensure the memory_candidates table is defined and created, along with appropriate indexes on fields like status, promotion_status, and JSON paths.
| private ensureTables(): void { | |
| this.db.exec("CREATE TABLE IF NOT EXISTS memory_evolution_leases (id TEXT PRIMARY KEY, loop_run_id TEXT NOT NULL, role TEXT NOT NULL CHECK(role IN ('memory_evaluator','memory_consolidator','memory_pruner')), runtime TEXT NOT NULL DEFAULT 'local', status TEXT NOT NULL DEFAULT 'prepared' CHECK(status IN ('prepared','running','completed','failed')), memory_candidate_id TEXT, metadata TEXT NOT NULL DEFAULT '{}', created_at TEXT NOT NULL, updated_at TEXT NOT NULL);CREATE INDEX IF NOT EXISTS idx_mel_loop_run ON memory_evolution_leases(loop_run_id);CREATE INDEX IF NOT EXISTS idx_mel_role ON memory_evolution_leases(role);CREATE INDEX IF NOT EXISTS idx_mel_candidate ON memory_evolution_leases(memory_candidate_id);"); | |
| } | |
| private ensureTables(): void { | |
| this.db.exec("CREATE TABLE IF NOT EXISTS memory_candidates (id TEXT PRIMARY KEY, title TEXT, content TEXT, memory_type TEXT, store TEXT, source_ref TEXT, status TEXT, promotion_status TEXT, human_required INTEGER, sensitivity TEXT, metadata TEXT NOT NULL DEFAULT '{}', created_at TEXT NOT NULL, updated_at TEXT NOT NULL); CREATE INDEX IF NOT EXISTS idx_mc_status ON memory_candidates(status); CREATE INDEX IF NOT EXISTS idx_mc_promo ON memory_candidates(promotion_status); CREATE TABLE IF NOT EXISTS memory_evolution_leases (id TEXT PRIMARY KEY, loop_run_id TEXT NOT NULL, role TEXT NOT NULL CHECK(role IN ('memory_evaluator','memory_consolidator','memory_pruner')), runtime TEXT NOT NULL DEFAULT 'local', status TEXT NOT NULL DEFAULT 'prepared' CHECK(status IN ('prepared','running','completed','failed')), memory_candidate_id TEXT, metadata TEXT NOT NULL DEFAULT '{}', created_at TEXT NOT NULL, updated_at TEXT NOT NULL); CREATE INDEX IF NOT EXISTS idx_mel_loop_run ON memory_evolution_leases(loop_run_id); CREATE INDEX IF NOT EXISTS idx_mel_role ON memory_evolution_leases(role); CREATE INDEX IF NOT EXISTS idx_mel_candidate ON memory_evolution_leases(memory_candidate_id);"); | |
| } |
| setCandidateScope(candidateId: string, agentId: string, shared: boolean): void { | ||
| this.db.prepare("UPDATE memory_candidates SET metadata=json_set(COALESCE(metadata,'{}'),'$.agent_id',?,'.shared',?) WHERE id=?").run(agentId, shared?1:0, candidateId); | ||
| } |
There was a problem hiding this comment.
In SQLite's json_set function, all JSON path arguments must start with a $ character. The path '.shared' is invalid and will cause SQLite to throw a runtime error (JSON path error) whenever setCandidateScope is called.
Fix: Change '.shared' to '$.shared'.
| setCandidateScope(candidateId: string, agentId: string, shared: boolean): void { | |
| this.db.prepare("UPDATE memory_candidates SET metadata=json_set(COALESCE(metadata,'{}'),'$.agent_id',?,'.shared',?) WHERE id=?").run(agentId, shared?1:0, candidateId); | |
| } | |
| setCandidateScope(candidateId: string, agentId: string, shared: boolean): void { | |
| this.db.prepare("UPDATE memory_candidates SET metadata=json_set(COALESCE(metadata,'{}'),'$.agent_id',?,'$.shared',?) WHERE id=?").run(agentId, shared?1:0, candidateId); | |
| } |
| async runConsolidationLoop(): Promise<LoopResult> { | ||
| const started = new Date().toISOString(); const errors: string[] = []; let processed = 0; | ||
| try { | ||
| const candidates = this.db.prepare("SELECT id FROM memory_candidates WHERE status IN ('candidate','promoted') AND promotion_status != 'rejected'").all() as any[]; | ||
| const ids = candidates.map((c: any) => c.id); | ||
| if (ids.length < 2) return this.logLoop('consolidation', 0, [], started); | ||
| const tok = (s: string) => new Set((s||'').toLowerCase().split(/\W+/).filter(Boolean)); | ||
| const all = ids.map((id: string) => this.db.prepare('SELECT * FROM memory_candidates WHERE id=?').get(id) as any); | ||
| for (let i = 0; i < all.length; i++) for (let j = i + 1; j < all.length; j++) { | ||
| if (!all[i] || !all[j]) continue; | ||
| const a = tok(all[i].title + ' ' + all[i].content), b = tok(all[j].title + ' ' + all[j].content); | ||
| const inter = new Set([...a].filter(x => b.has(x))).size, union = new Set([...a, ...b]).size; | ||
| if (union > 0 && inter / union > 0.85) { | ||
| const keeper = all[i].content.length >= all[j].content.length ? all[i] : all[j]; | ||
| const victim = keeper.id === all[i].id ? all[j] : all[i]; | ||
| this.db.prepare("UPDATE memory_candidates SET metadata=json_set(COALESCE(metadata,'{}'),'$.consolidated_into',?,'$.consolidated_at',?),status='rejected',promotion_status='rejected',updated_at=? WHERE id=?").run(keeper.id, new Date().toISOString(), new Date().toISOString(), victim.id); | ||
| processed++; | ||
| } | ||
| } | ||
| } catch (e) { errors.push((e as Error).message); } | ||
| return this.logLoop('consolidation', processed, errors, started); | ||
| } |
There was a problem hiding this comment.
The current implementation of runConsolidationLoop has several critical issues:
- Genealogy Corruption / Double Consolidation: When a candidate is consolidated (marked as rejected), it is not removed or skipped from the all array. In subsequent iterations, an already-rejected candidate can still be compared and consolidated again, overwriting its consolidated_into metadata and breaking the provenance chain.
- N+1 Query Bottleneck: It queries each candidate individually in a loop (this.db.prepare('SELECT * FROM memory_candidates WHERE id=?').get(id)). This should be queried all at once.
- O(N^2) Tokenization: It tokenizes the same candidate content repeatedly inside the nested loop.
Fix:
Query all candidates at once, precompute the token sets, and track consolidated IDs to skip them.
async runConsolidationLoop(): Promise<LoopResult> {
const started = new Date().toISOString(); const errors: string[] = []; let processed = 0;
try {
const candidates = this.db.prepare("SELECT * FROM memory_candidates WHERE status IN ('candidate','promoted') AND promotion_status != 'rejected'").all() as any[];
if (candidates.length < 2) return this.logLoop('consolidation', 0, [], started);
const tok = (s: string) => new Set((s||'').toLowerCase().split(/\W+/).filter(Boolean));
const candidatesWithSets = candidates.map(c => ({
c,
tokens: tok((c.title || '') + ' ' + (c.content || ''))
}));
const consolidatedIds = new Set<string>();
for (let i = 0; i < candidatesWithSets.length; i++) {
if (consolidatedIds.has(candidatesWithSets[i].c.id)) continue;
for (let j = i + 1; j < candidatesWithSets.length; j++) {
if (consolidatedIds.has(candidatesWithSets[j].c.id)) continue;
const a = candidatesWithSets[i].tokens;
const b = candidatesWithSets[j].tokens;
const inter = new Set([...a].filter(x => b.has(x))).size;
const union = new Set([...a, ...b]).size;
if (union > 0 && inter / union > 0.85) {
const keeperObj = (candidatesWithSets[i].c.content || '').length >= (candidatesWithSets[j].c.content || '').length ? candidatesWithSets[i] : candidatesWithSets[j];
const victimObj = keeperObj.c.id === candidatesWithSets[i].c.id ? candidatesWithSets[j] : candidatesWithSets[i];
this.db.prepare("UPDATE memory_candidates SET metadata=json_set(COALESCE(metadata,'{}'),'$.consolidated_into',?,'$.consolidated_at',?),status='rejected',promotion_status='rejected',updated_at=? WHERE id=?").run(keeperObj.c.id, new Date().toISOString(), new Date().toISOString(), victimObj.c.id);
consolidatedIds.add(victimObj.c.id);
processed++;
if (victimObj.c.id === candidatesWithSets[i].c.id) {
break;
}
}
}
}
} catch (e) { errors.push((e as Error).message); }
return this.logLoop('consolidation', processed, errors, started);
}| start(): void { | ||
| if (this.config.consolidation_enabled) this.timers.push(setInterval(() => this.runConsolidationLoop().catch(()=>{}), this.config.consolidation_interval_ms)); | ||
| if (this.config.decay_enabled) this.timers.push(setInterval(() => this.runDecayLoop().catch(()=>{}), this.config.decay_interval_ms)); | ||
| if (this.config.eval_enabled) this.timers.push(setInterval(() => this.runEvalGateLoop().catch(()=>{}), this.config.eval_interval_ms)); | ||
| if (this.config.drift_check_enabled) this.timers.push(setInterval(() => this.runDriftCheck().catch(()=>{}), 86400000)); | ||
| } |
There was a problem hiding this comment.
Using setInterval for long-running asynchronous tasks (like consolidation, decay, and evaluation loops) is a major reliability risk. If a loop execution takes longer than the interval (e.g., due to database locks or high load), multiple instances of the same loop will run concurrently.
Since SQLite is a single-writer database, concurrent write operations from overlapping loops will trigger SQLITE_BUSY errors and database locks.
Fix:
Use recursive setTimeout patterns to ensure that the next execution is only scheduled after the current execution has fully completed.
start(): void {
const runAndSchedule = (fn: () => Promise<unknown>, interval: number) => {
const execute = async () => {
try { await fn(); } catch {}
this.timers.push(setTimeout(execute, interval));
};
this.timers.push(setTimeout(execute, interval));
};
if (this.config.consolidation_enabled) runAndSchedule(() => this.runConsolidationLoop(), this.config.consolidation_interval_ms);
if (this.config.decay_enabled) runAndSchedule(() => this.runDecayLoop(), this.config.decay_interval_ms);
if (this.config.eval_enabled) runAndSchedule(() => this.runEvalGateLoop(), this.config.eval_interval_ms);
if (this.config.drift_check_enabled) runAndSchedule(() => this.runDriftCheck(), 86400000);
}| private getOrCreateKey(agentId: string): Buffer { | ||
| const row = this.db.prepare('SELECT key_hash FROM memory_encryption_keys WHERE agent_id=?').get(agentId) as any; | ||
| if (row) return Buffer.from(row.key_hash, 'hex'); | ||
| const password = process.env['ENCRYPTION_KEY_'+agentId.toUpperCase()] || process.env.ENCRYPTION_KEY || 'dev-key'; | ||
| const key = scryptSync(password, randomBytes(16), 32); | ||
| this.db.prepare('INSERT OR REPLACE INTO memory_encryption_keys (agent_id,key_hash,created_at) VALUES (?,?,?)').run(agentId, key.toString('hex'), new Date().toISOString()); | ||
| return key; | ||
| } |
There was a problem hiding this comment.
The getOrCreateKey method derives an encryption key using scryptSync with a random salt, but then stores the raw derived key directly in the database (key_hash column).
Storing the encryption keys in the same database as the encrypted data defeats the purpose of encryption at rest (if an attacker steals the database file, they get both the ciphertext and the keys to decrypt it).
Recommendation:
Do not store raw encryption keys in the database. Instead, load the keys directly from environment variables or a secure Key Management Service (KMS) at runtime.
| detectContradictions(): ContradictionAlert[] { | ||
| const alerts: ContradictionAlert[] = []; | ||
| const candidates = this.db.prepare("SELECT * FROM memory_candidates WHERE status IN ('candidate','promoted')").all() as any[]; | ||
| for (let i = 0; i < candidates.length; i++) for (let j = i + 1; j < candidates.length; j++) { | ||
| const a = candidates[i], b = candidates[j]; | ||
| if (a.memory_type !== b.memory_type) continue; | ||
| const aW = new Set((a.content||'').toLowerCase().split(/\W+/).filter(Boolean)); | ||
| const bW = new Set((b.content||'').toLowerCase().split(/\W+/).filter(Boolean)); | ||
| const overlap = new Set([...aW].filter(x => bW.has(x))).size, union = new Set([...aW,...bW]).size; | ||
| if (union > 0 && overlap/union > 0.5 && /always|must|never|prohibited/.test(a.content||'') && /always|must|never|prohibited/.test(b.content||'')) { | ||
| alerts.push({ id: randomUUID(), candidate_a: a.id, candidate_b: b.id, contradiction_type: 'semantic_overlap_rules', severity: overlap/union > 0.8 ? 'high' : 'medium', detected_at: new Date().toISOString() }); | ||
| } | ||
| } | ||
| return alerts; | ||
| } |
There was a problem hiding this comment.
In detectContradictions, the word sets aW and bW are computed inside the nested O(N^2) loop. This means for N candidates, the content is split and tokenized O(N^2) times, which will severely block the Node.js event loop as the database grows.
Fix: Precompute the word sets for all candidates once before entering the nested loops.
detectContradictions(): ContradictionAlert[] {
const alerts: ContradictionAlert[] = [];
const candidates = this.db.prepare("SELECT * FROM memory_candidates WHERE status IN ('candidate','promoted')").all() as any[];
const candidatesWithSets = candidates.map(c => ({
c,
words: new Set((c.content || '').toLowerCase().split(/\W+/).filter(Boolean))
}));
for (let i = 0; i < candidatesWithSets.length; i++) {
for (let j = i + 1; j < candidatesWithSets.length; j++) {
const { c: a, words: aW } = candidatesWithSets[i];
const { c: b, words: bW } = candidatesWithSets[j];
if (a.memory_type !== b.memory_type) continue;
const overlap = new Set([...aW].filter(x => bW.has(x))).size;
const union = new Set([...aW, ...bW]).size;
if (union > 0 && overlap/union > 0.5 && /always|must|never|prohibited/.test(a.content||'') && /always|must|never|prohibited/.test(b.content||'')) {
alerts.push({ id: randomUUID(), candidate_a: a.id, candidate_b: b.id, contradiction_type: 'semantic_overlap_rules', severity: overlap/union > 0.8 ? 'high' : 'medium', detected_at: new Date().toISOString() });
}
}
}
return alerts;
}| if(bp[2]>0.85){ | ||
| const[i,j]=bp; const keeper=cs[i].content.length>=cs[j].content.length?cs[i]:cs[j]; | ||
| const victim=keeper.id===cs[i].id?cs[j]:cs[i]; surv=keeper.id; merged.push(victim.id); | ||
| this.db.prepare("UPDATE memory_candidates SET metadata=json_set(COALESCE(metadata,'{}'),'$.consolidated_into',?,'$.consolidated_at',?,'$.consolidation_score',?),status='rejected',promotion_status='rejected',updated_at=? WHERE id=?").run(keeper.id,new Date().toISOString(),bp[2],new Date().toISOString(),victim.id); | ||
| } |
There was a problem hiding this comment.
If content is null or undefined for any candidate, calling cs[i].content.length will throw a TypeError: Cannot read properties of null (reading 'length') and crash the consolidation process.
Fix: Use optional chaining or fallback to an empty string: (cs[i].content || '').length.
| if(bp[2]>0.85){ | |
| const[i,j]=bp; const keeper=cs[i].content.length>=cs[j].content.length?cs[i]:cs[j]; | |
| const victim=keeper.id===cs[i].id?cs[j]:cs[i]; surv=keeper.id; merged.push(victim.id); | |
| this.db.prepare("UPDATE memory_candidates SET metadata=json_set(COALESCE(metadata,'{}'),'$.consolidated_into',?,'$.consolidated_at',?,'$.consolidation_score',?),status='rejected',promotion_status='rejected',updated_at=? WHERE id=?").run(keeper.id,new Date().toISOString(),bp[2],new Date().toISOString(),victim.id); | |
| } | |
| if(bp[2]>0.85){ | |
| const[i,j]=bp; const keeper=(cs[i].content || '').length>=(cs[j].content || '').length?cs[i]:cs[j]; | |
| const victim=keeper.id===cs[i].id?cs[j]:cs[i]; surv=keeper.id; merged.push(victim.id); | |
| this.db.prepare("UPDATE memory_candidates SET metadata=json_set(COALESCE(metadata,'{}'),'$.consolidated_into',?,'$.consolidated_at',?,'$.consolidation_score',?),status='rejected',promotion_status='rejected',updated_at=? WHERE id=?").run(keeper.id,new Date().toISOString(),bp[2],new Date().toISOString(),victim.id); | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33be087b70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import type { Database } from 'better-sqlite3'; | ||
| import { MemoryEvolutionService } from '../services/memory-evolution-service'; | ||
|
|
||
| export function createMemoryEvolutionRoutes(db: Database): Router { |
There was a problem hiding this comment.
This factory is never imported or passed to router.use(...) in packages/server/src/routes/index.ts (I checked the route registrations: /memory is mounted, but no /memory-evolution or /v1/memory-evolution route is). Since the app only exposes routes through createRoutes(...) under /api, every endpoint documented in this new router currently returns 404, so agents cannot ingest, evolve, retrieve, or manage memory-evolution leases through HTTP.
Useful? React with 👍 / 👎.
| const candidate = { id: 'mc-'+Date.now(), title: metadata?.title || 'Trace from '+agent_id, content, memory_type: memory_type || 'operational_memory', status: 'candidate', promotion_status: 'proposed', created_at: new Date().toISOString() }; | ||
| res.status(201).json({ status: 'ingested', candidate }); |
There was a problem hiding this comment.
For any agent calling POST /ingest, this only creates an in-memory object and returns 201 without inserting into memory_candidates or using the existing MemoryCandidateService. Subsequent /retrieve, /quality/:id, promotion checks, and scheduler loops all read from memory_candidates, so the returned candidate id is immediately unusable and the trace is effectively dropped despite the success response.
Useful? React with 👍 / 👎.
| const crossAgentUsage = 0.5; | ||
| const {w1,w2,w3,w4,w5,w6} = this.WEIGHTS; | ||
| const composite = w1*discrimination+w2*stability+w3*novelty+w4*consolidation+w5*decayResistance+w6*crossAgentUsage; | ||
| return {candidateId:c.id,discrimination,stability,novelty,consolidation,decayResistance,crossAgentUsage,composite,promotionEligible:composite>=0.7&&discrimination>=0.6}; |
There was a problem hiding this comment.
When evaluatePromotion() is called for a normal unpromoted memory_candidates row, discrimination is at most 0.5, but this return value requires discrimination >= 0.6; therefore candidate and review-required memories can never be eligible regardless of their composite score or benchmark result. In practice /promote/:id can only report eligible for rows that are already promoted, which breaks the promotion path this endpoint is supposed to evaluate.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| setCandidateScope(candidateId: string, agentId: string, shared: boolean): void { | ||
| this.db.prepare("UPDATE memory_candidates SET metadata=json_set(COALESCE(metadata,'{}'),'$.agent_id',?,'.shared',?) WHERE id=?").run(agentId, shared?1:0, candidateId); |
There was a problem hiding this comment.
When any caller uses setCandidateScope, SQLite rejects '.shared' as a bad JSON path because JSON paths must start at $, so the update fails before either the agent id or shared flag is stored. That leaves scoped retrieval unable to see memories that were supposed to be marked shared.
Useful? React with 👍 / 👎.
| const { execSync } = await import('child_process'); | ||
| const pass = process.env.REDIS_PASSWORD || '62778f211595bf46081ca824ada432011bdd500959d93b2222a910280e942005'; | ||
| const payload = Object.entries(event).map(([k,v]) => k+' '+v).join(' '); | ||
| execSync("docker exec redis sh -c 'redis-cli -a "+pass+" XADD memory_events * agent_id "+agentId+" "+payload+"'", { timeout: 5000 }); |
There was a problem hiding this comment.
When syncToRedis is called with an agent-controlled agentId or event value containing shell metacharacters such as a single quote, those values are concatenated directly into an execSync shell command and can break out of the quoted sh -c payload. This turns memory-event sync into host command execution in environments where Redis sync is enabled; use argument-based execution or escape Redis fields instead of building one shell string.
Useful? React with 👍 / 👎.
7e383d4 to
c60b36f
Compare
1 participant
Summary
Test plan