feat(memory): add persistent memory layer for cross-session learning#149
feat(memory): add persistent memory layer for cross-session learning#149
Conversation
Kimchi Code Review
Summary📊 Review Score: 82/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Comprehensive test coverage added including: content-scanner tests for prompt injection patterns (9 test cases), file-io tests for atomic writes and entry splitting, memory-store tests for CRUD operations and limits, memory-tool tests for action dispatch, and integration tests for the extension registration. Tests properly use temporary directories and clean up after execution. 📝 Found 5 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 82/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 3/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Comprehensive test coverage added including: content-scanner tests for prompt injection patterns (9 test cases), file-io tests for atomic writes and entry splitting, memory-store tests for CRUD operations and limits, memory-tool tests for action dispatch, and integration tests for the extension registration. Tests properly use temporary directories and clean up after execution.
📝 Found 5 issue(s). See inline comments for details.
| export async function writeMemoryFile(filePath: string, entries: string[]): Promise<void> { | ||
| mkdirSync(dirname(filePath), { recursive: true }) | ||
| const content = entries.length > 0 ? entries.join(ENTRY_DELIMITER) : "" | ||
| const tmpPath = join(dirname(filePath), `.${Date.now()}.tmp`) |
There was a problem hiding this comment.
The catch block silently swallows ALL errors from lock(), not just the case where the file doesn't exist. If locking fails due to permissions, lock contention timeout, or system errors, the code proceeds without a lock, risking data corruption. Additionally, if the file doesn't exist, multiple concurrent writers could race: both proceed without locks, write to different temp files, and the last rename wins, causing the first write to be silently lost.
💡 Suggestion: Check specifically for file-not-exist errors (e.g., by checking err.code === 'ENOENT' or verifying !existsSync(filePath) in the catch block). If the error is not ENOENT, re-throw it. Consider creating an empty file first if it doesn't exist, then locking it, to ensure locking always occurs.
| // Acquire cross-platform file lock before writing | ||
| let release: (() => Promise<void>) | undefined | ||
| try { | ||
| release = await lock(filePath, { retries: { retries: 10, factor: 2, minTimeout: 50, maxTimeout: 1000 } }) |
There was a problem hiding this comment.
If writeFile succeeds but rename fails (e.g., due to permissions or disk full), the temporary file at tmpPath is not cleaned up, leaving orphaned temp files in the memory directory. The finally block only releases the lock, it doesn't unlink the temp file.
💡 Suggestion: Add cleanup logic in a catch block or finally block to remove tmpPath if it exists and the rename failed. For example: try { await writeFile(...); await rename(...) } catch (err) { await unlink(tmpPath).catch(() => {}); throw err }.
| import { dirname, join } from "node:path" | ||
| import { lock } from "proper-lockfile" | ||
|
|
||
| const ENTRY_DELIMITER = "\n§\n" |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The ENTRY_DELIMITER constant \n§\n is defined identically in both file-io.ts and memory-store.ts. This duplication risks inconsistency if one file is modified without updating the other.
💡 Suggestion: Export ENTRY_DELIMITER from file-io.ts (or a shared constants file) and import it in memory-store.ts to ensure a single source of truth.
| if ((err as { code?: string }).code === "ENOENT") return [] | ||
| throw err | ||
| } | ||
| } |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The temp file name uses only Date.now() without randomness, which could theoretically collide if two operations occur within the same millisecond (e.g., on a fast system or if the clock hasn't updated). While the file lock prevents concurrent collisions, if the lock cannot be acquired (file doesn't exist case), collisions become possible.
💡 Suggestion: Append a random suffix to the temp filename, e.g., \.${Date.now()}-${randomBytes(4).toString('hex')}.tmp, to guarantee uniqueness.
| private _ok(target: MemoryTarget, message: string): MemoryToolResult { | ||
| const entries = this._get(target) | ||
| const current = entries.join(ENTRY_DELIMITER).length | ||
| const limit = this._limit(target) |
There was a problem hiding this comment.
ℹ️🏗️ Design
The _resolveSingleMatch method truncates matched entries to 80 characters in the error message, but uses a slice without checking if the truncation actually occurred, and doesn't indicate that truncation happened.
💡 Suggestion: Consider adding an ellipsis indicator only when truncation occurs, or ensure the truncation logic is consistent with the displayed message to avoid confusion when users see partial strings.
castabar
left a comment
castabar
left a comment
There was a problem hiding this comment.
Review
Clean architecture overall. Two stores (MEMORY.md / USER.md), §-delimited, char-bounded, injected once at session_start as a frozen snapshot. Reasonable approach.
Issues
1. Locking bug — real race condition (file-io.ts:258-263)
When the target file doesn't exist yet (first write), proper-lockfile throws and the catch silently proceeds with no lock:
```ts
try {
release = await lock(filePath, ...)
} catch {
// File doesn't exist yet — proceed without lock
}
```
Two concurrent writers both fail to lock → both write different tmp paths → both rename → last rename wins, first writer's entry is silently dropped. Fix: use a separate always-present lock file (e.g. `filePath + ".lock"`) or ensure the target file exists before locking.
2. ENTRY_DELIMITER defined in two places
file-io.ts:6 and memory-store.ts:4 both define const ENTRY_DELIMITER = "\n§\n". If one changes they silently diverge. Move to types.ts and import from there.
3. Dynamic import inside hot path (file-io.ts:266)
```ts
const { rename } = await import("node:fs/promises")
```
rename is available as a static import at the top of the module. This dynamic import runs on every write. Just import it statically.
4. _resolveSingleMatch — ambiguous multi-match behavior (memory-store.ts:580-597)
When matches.length > 1 but all matched entries are identical (file manually edited with dupes), uniqueTexts.size === 1 so it silently uses the first match. The user gets a success response but may not know which duplicate was targeted. At minimum document this.
5. Content scanner false positives
/you\s+are\s+now\s+/i blocks "you are now able to use this feature" in legitimate notes. The read_secrets pattern blocks any note that mentions cat ~/.env even in a "don't do this" context. These will produce confusing user-facing errors.
6. types.test.ts is noise
The test just assigns type literals and asserts equality — it tests nothing at runtime. Remove or replace with something meaningful.
Minor
_ok()returns live entries whileformatForSystemPromptreturns the frozen snapshot. The agent sees fresh counts but a stale prompt — document this so future maintainers don't "fix" it.- No test covering that
{{MEMORY}}/{{USER_PROFILE}}placeholders are actually replaced in the rendered system prompt. ctxis cast twice viaas unknown as { memorySnapshot?: ... }in two different files. Consider a shared interface.proper-lockfilelast released 2021, no longer actively maintained. Not a blocker but worth noting.
The locking bug (#1) is the only real correctness issue. Fix the lock-on-nonexistent-file race before merging.
Co-Authored-By: Kimchi <noreply@kimchi.dev>
Co-Authored-By: Kimchi <noreply@kimchi.dev>
Co-Authored-By: Kimchi <noreply@kimchi.dev>
…to orchestrator and single-model system prompts Co-Authored-By: Kimchi <noreply@kimchi.dev>
Co-Authored-By: Kimchi <noreply@kimchi.dev>
… to system prompt builders
Add optional memoryBlock and userProfileBlock parameters to buildOrchestratorSystemPrompt
and buildSingleModelSystemPrompt, with corresponding .replace() calls for {{MEMORY}} and
{{USER_PROFILE}} placeholders. buildSubagentSystemPrompt is intentionally unchanged as
subagent prompts should never receive memory.
Co-Authored-By: Kimchi <noreply@kimchi.dev>
- Create src/extensions/memory/index.ts entry point - Wire memoryExtension into extensionFactories in cli.ts - Add tests for tool registration and session_start snapshot - Biome format fixes applied Co-Authored-By: Kimchi <noreply@kimchi.dev>
- Lock companion .lock file instead of target to avoid race when target doesn't exist yet (proper-lockfile throws on missing files). - Move ENTRY_DELIMITER to types.ts so it's defined once. - Use static rename import instead of dynamic import in hot path. - Add JSDoc on _resolveSingleMatch documenting duplicate handling. - Add test for replace on duplicate-identical entries. - Narrow role_hijack pattern with article requirement (a/an/the) to avoid false positives on benign phrases like 'you are now able to'. - Add test for benign 'you are now' without article. - Extract MemoryContext interface in types.ts, use in index.ts and prompt-enrichment.ts to eliminate repeated inline casts. - Remove types.test.ts (noise — tests TS type system at runtime). - Add JSDoc on formatForSystemPrompt and _ok documenting the live entries vs frozen-snapshot distinction. Co-Authored-By: Kimchi <noreply@kimchi.dev>
castabar
force-pushed
the
feature/memory-layer
branch
from
11f3d88 to
9333086
Compare
… to model
- Memory extension now owns its own before_agent_start handler and appends
memory blocks directly to the system prompt built by prompt-enrichment,
removing the brittle ctx-based snapshot approach (ctx is a fresh object
per emit() call, so writes to it were silently dropped)
- Remove {{MEMORY}}/{{USER_PROFILE}} placeholders from prompt templates and
all memoryBlock/userProfileBlock params from prompt-transformer functions
- Fix wrapResult in memory-tool: read action was returning empty string as
text content; model received blank tool result and hallucinated an image
attachment — now surfaces entries in the content the model sees
- Remove MemoryContext type and getMemorySnapshot export (no longer needed)
1 participant
Memory Layer Implementation
Adds a persistent memory system to the Kimchi harness. Memory survives across sessions and is injected directly into the system prompt by the memory extension itself.
What Changed
Core Components
memoryextension (src/extensions/memory/): New extension with full toolingmemorytool: Supports 4 actions against two targets:add/replace/remove/readmemorytarget: agent notes (env facts, conventions, tool quirks)usertarget: user profile (name, role, preferences, timezone)MEMORY.md/USER.md, no frontmatterfs.rename()+proper-lockfileSystem Prompt Integration
The memory extension registers its own
before_agent_starthandler. Afterprompt-enrichmentbuilds the system prompt, memory appends its blocks directly:No placeholders in prompt templates. No cross-extension coordination required.
Frozen snapshot: memory loads once at
session_start— mid-session writes don't alter the running prompt, preserving prefix cache stability.Subagent isolation: memory is not injected into subagent prompts (stateless workers).
Files Added/Modified
Session Log Examples
Writing preferences (two mutations in one session)
User tells the model to use
rginstead ofgrep:User follows up with
fd:User mentions their timezone:
Successful recall from system prompt (next session)
No tool calls. Both answers came directly from the system prompt.
Design Decisions
Self-contained injection: The memory extension owns its
before_agent_starthandler rather than relying on{{MEMORY}}placeholders in prompt templates. The runner chains handlers — each receives the system prompt built by the previous one — so memory safely appends after orchestration without any coupling.Frozen snapshots: Mid-session writes update disk but not the running system prompt. Next session picks up the new state.
Char limits vs tokens: Tokens vary by model. Characters are deterministic and match Hermes's implementation (2200/1375).
Testing
Security
Content scanner blocks: prompt injection, role hijacking, secret exfiltration via
curl/wget, secret file reads (.env,.netrc), invisible unicode.Kimchi Summary
What changed
Adds a persistent memory extension that lets the agent save and recall durable facts and user preferences across sessions via a new
memorytool. Also removes several obsolete documentation files.Why
Agents currently lose context about the user, project conventions, and environment quirks between sessions. A persistent memory layer reduces repetitive discovery, prevents recurring mistakes, and personalises behaviour by injecting known facts back into the system prompt.
Key changes
src/cli.ts: registersmemoryExtension.src/extensions/memory/index.ts: bootstraps the extension, captures a frozen snapshot onsession_start, and injects it into the system prompt duringbefore_agent_start.src/extensions/memory/memory-tool.ts: exposes amemorytool supportingadd,replace,remove, andreadactions againstmemory(agent notes) oruser(user profile) targets.src/extensions/memory/memory-store.ts: implements CRUD with per-store character limits, deduplication, substring-based matching for updates, and frozen snapshots to keep prefix caches stable.src/extensions/memory/file-io.ts: atomic §-delimited file I/O usingproper-lockfilefor safe concurrent access.src/extensions/memory/content-scanner.ts: rejects prompt injection, role hijacking, secret exfiltration (curl/wgetwith secrets, reading.env, SSH backdoors), and invisible unicode characters before persistence.src/extensions/memory/types.ts: defines store types andENTRY_DELIMITER.package.json/pnpm-lock.yaml: addsproper-lockfileand@types/proper-lockfiledependencies.docs/permissions.md,docs/phase-guidelines-research.md,docs/sandboxing.md,docs/troubleshooting.md,docs/superpowers/specs/2026-04-15-mcp-tool-result-offload-design.md.Impact
MEMORY.mdandUSER.mdunder the agent directory.2200formemory,1375foruser. Attempts to exceed these limits are rejected.