feat(memory): Add pluggable memory provider support with fallback - #1068
feat(memory): Add pluggable memory provider support with fallback#1068tautvydasLiekis wants to merge 2 commits into
Conversation
Kimchi Code Review
Summary📊 Review Score: 76/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Tests were added in 🔒 Security concerns found: 📝 Found 6 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 |
There was a problem hiding this comment.
📊 Review Score: 76/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 3/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Tests were added in src/extensions/agents/memory/memory.test.ts covering config-driven loading, missing/invalid config, malformed JSON, provider ordering, exception containment, and file fallback. The mock in src/extensions/agents/manager/agent-runner.test.ts was updated for the new async resolveMemoryBlock export.
🔒 Security concerns found: loadConfiguredProviders() dynamically imports arbitrary module paths read from <agent-dir>/memory-providers.json without validating that the path is absolute, under a trusted directory, or not a symlink. If an attacker can write to that config file, they can achieve code execution on the next agent spawn. This is intentional as a plugin mechanism, but the expanded attack surface should be documented and ideally sandboxed.
📝 Found 6 issue(s). See inline comments for details.
- Validate module paths before dynamic import: absolute, existing, non-symlink only; import via pathToFileURL() for platform-correct ESM resolution on all OSes - Accept only non-empty string blocks from providers so a provider returning undefined cannot leak into the Promise<string> contract - Contain load failures: the lazy load promise resets on unexpected errors instead of wedging as rejected forever - Extend the provider contract with an optional context carrying the memory scope and read-only mode, so providers can tailor output; two-argument implementations stay compatible Co-Authored-By: Kimchi <noreply@kimchi.dev>
|
This PR has been marked as stale because it has not had recent activity. It will be closed tomorrow if no further activity occurs. |
Linked issue
Why
Subagent system prompts are built inside
agent-runner.ts, so external extensions have no hook to supply agent memory — previously every subagent got a hardwired per-agentMEMORY.mdblock, which is flat, unranked (the full index is injected relevant or not), and can't be supplied by semantic memory backends (OpenViking, mem0, etc.). To let any memory DB provide subagent memory without growing harness-side integration code per backend, the resolution point had to become generic. This is the harness-side foundation that thekimchi-openvikingpackage (and its generic HTTP adapter presets for mem0/Supermemory/Letta/Zep) plugs into.What
AgentMemoryProvidercontract inmemory.ts({ name, buildBlock(agentName, cwd): Promise<string | null> }) with an ordered registry: first non-null block wins, then file memory.<agent-dir>/memory-providers.json([{"name", "module"}]) are dynamic-imported and shape-validated before registration — the harness keeps zero provider-specific code; adding a backend is a config edit, not a code change.resolveMemoryBlock): malformed manifests, unloadable modules, throwing providers, and all-null results are skipped silently, falling back to the existing file-basedbuildMemoryBlock/buildReadOnlyMemoryBlock— behavior is byte-identical for an empty registry.agent-runner.tsnow builds the memory block viaresolveMemoryBlockinstead of calling the file builders directly.README.mddocumenting the contract, scopes, registration paths, and resolution semantics.Testing:
npx vitest run src/extensions/agents— 506 tests across 46 files; newmemory.test.tscovers registry ordering, exception containment, missing/malformed manifests, invalid entries, shape mismatches, unloadable modules, and file fallback in write and read-only modes.Checklist
pnpm run test)pnpm run check)