feat(memory): Memory V2 schema — scoped types, relevance scoring, maintenance#156
feat(memory): Memory V2 schema — scoped types, relevance scoring, maintenance#156
Conversation
|
New PR opened -- automated review will run on the next push. To trigger a manual review, comment |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
Pull request overview
Implements “Memory V2” with CC-compatible memory types/scopes, relevance scoring + maintenance, and adds hook deployment verification plus a PreToolUse LRU cache.
Changes:
- Migrates memory model to 4 types with scope + relevance/verification metadata, including DB migration + indexes.
- Adds background memory maintenance (dedupe/decay/stale cleanup/reference verification/reindex) and compaction “summary-bridge” extraction.
- Adds hook deployment integrity checking and performance improvements (PreToolUse caching), plus adjusts instruction rule loading behavior.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/test/session/instruction-rules.test.ts | Updates expectations for loading both global + project rules with same filename. |
| packages/opencode/test/memory/summary-bridge.test.ts | Adds coverage for parsing “Memory Candidates” sections from summaries. |
| packages/opencode/test/memory/maintenance.test.ts | Adds coverage for maintenance cycle behaviors (dedupe/decay/stale/verify/promote). |
| packages/opencode/test/memory/file.test.ts | Updates tests for new frontmatter fields and legacy-type mapping. |
| packages/opencode/test/hook/verify.test.ts | Adds coverage for hook deployment verification results. |
| packages/opencode/src/session/prompt.ts | Triggers background maintenance at session start; injects agent-scoped memory. |
| packages/opencode/src/session/message-v2.ts | Adds transcriptOnly and filters such parts from model message conversion. |
| packages/opencode/src/session/instruction.ts | Loads both global and project rules (no same-filename override). |
| packages/opencode/src/memory/types.ts | Defines new type system, legacy mappings, and scope model. |
| packages/opencode/src/memory/summary-bridge.ts | Extracts memory candidates from compaction summaries into long-term memory tracking. |
| packages/opencode/src/memory/store.ts | Extends DB store with scope/agent/relevance APIs and promotion support. |
| packages/opencode/src/memory/promoter.ts | Implements auto-promotion from personal → project scope based on access count. |
| packages/opencode/src/memory/memory.sql.ts | Adds new columns + indexes for Memory V2 schema. |
| packages/opencode/src/memory/maintenance.ts | Implements background maintenance cycle (dedupe/decay/remove/verify/reindex). |
| packages/opencode/src/memory/injector.ts | Moves to DB-first, relevance-scored, token-budgeted memory injection. |
| packages/opencode/src/memory/file.ts | Adds backward-compatible frontmatter parsing + new serialization fields. |
| packages/opencode/src/memory/extractor.ts | Updates extraction to new schema; adds consolidation/index updates. |
| packages/opencode/src/hook/verify.ts | Adds verification for orphan/missing/non-executable hook scripts. |
| packages/opencode/src/hook/index.ts | Exposes new verify API and hook cache clearing. |
| packages/opencode/src/hook/execute.ts | Adds PreToolUse LRU cache and parallel hook execution behavior. |
| packages/opencode/src/config/config.ts | Adds memory config options (token cap, consolidation, default scope). |
| packages/opencode/migration/20260410120000_memory_v2_schema/migration.sql | Migrates schema + maps legacy types to new CC-compatible types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Deduplicate agent entries that are already in general entries | ||
| const generalIds = new Set(entries.map((e) => e.id)) | ||
| agentEntries = agentEntries.filter((e) => !generalIds.has(e.id)) |
There was a problem hiding this comment.
agentEntries is fetched as a subset of entries (via listByAgent after list), then immediately filtered out because generalIds contains all IDs from entries. This makes the "Agent-Specific Knowledge" section unreachable. Consider either (a) excluding agent-tagged rows from the initial list() call (e.g., list only where agent IS NULL) or (b) deriving agentEntries from entries and removing them from the general groups instead of deduplicating by ID.
| function relevanceWeight(entry: Memory.Info): number { | ||
| const daysSinceUpdate = (Date.now() - entry.timeUpdated) / (1000 * 60 * 60 * 24) | ||
| const recencyWeight = 1.0 / (1 + daysSinceUpdate / 30) | ||
| return entry.relevanceScore * recencyWeight * Math.log2(entry.accessCount + 2) | ||
| } |
There was a problem hiding this comment.
Relevance weighting and auto-promotion rely on accessCount, but this loader never increments access_count for entries it actually injects. Since MemoryStore.get() (the only place that increments access_count) is not used here, accessCount is likely to remain 0 and the scoring/promotion logic won’t behave as intended. Consider adding a batch "mark accessed" update for the selected entries (increment access_count and optionally record last-access time).
| if (!existing) return undefined | ||
| const previousScope = existing.scope ?? "personal" | ||
| yield* db((d) => | ||
| d | ||
| .update(MemoryTable) | ||
| .set({ | ||
| scope: targetScope, | ||
| promoted_from: previousScope, | ||
| time_updated: Date.now(), |
There was a problem hiding this comment.
previousScope defaults to "personal" when existing.scope is null, but the DB schema/migration sets a NOT NULL default of "project". This will record an incorrect promoted_from value for legacy rows. Default to "project" (or map based on the actual stored value) so promoted_from reflects the real prior scope.
| yield* db((d) => | ||
| d | ||
| .update(MemoryTable) | ||
| .set({ relevance_score: score, time_updated: Date.now() }) |
There was a problem hiding this comment.
updateRelevance sets time_updated = Date.now(). Since timeUpdated is also used for recency weighting and decay calculations, this makes old entries look freshly updated whenever maintenance decays them, which can counteract the intended "inactivity" decay behavior. Consider not touching time_updated when only changing relevance_score (or introduce a dedicated timestamp for relevance maintenance).
| .set({ relevance_score: score, time_updated: Date.now() }) | |
| .set({ relevance_score: score }) |
| const values: Record<string, unknown> = { time_updated: Date.now() } | ||
| if (input.topic !== undefined) values.topic = input.topic | ||
| if (input.name !== undefined) values.topic = input.name | ||
| if (input.description !== undefined) values.description = input.description | ||
| if (input.type !== undefined) values.type = input.type | ||
| if (input.scope !== undefined) values.scope = input.scope | ||
| if (input.content !== undefined) values.content = input.content | ||
| if (input.relevanceScore !== undefined) values.relevance_score = input.relevanceScore | ||
| if (input.timeLastVerified !== undefined) values.time_last_verified = input.timeLastVerified | ||
| yield* db((d) => d.update(MemoryTable).set(values).where(eq(MemoryTable.id, input.id)).run()) |
There was a problem hiding this comment.
update() always sets time_updated even when the only change is time_last_verified. Maintenance calls svc.update({ timeLastVerified: ... }), which will continually refresh timeUpdated and interfere with recency/decay semantics. Consider allowing maintenance-only updates that do not bump time_updated (e.g., a separate markVerified() method or an option to disable touching time_updated).
| for (const candidate of candidates) { | ||
| try { | ||
| MemoryExtractor.trackDecision(sessionID, candidate.name, candidate.content) | ||
| } catch (err) { | ||
| log.warn("failed to extract memory candidate", { error: err, name: candidate.name }) | ||
| } |
There was a problem hiding this comment.
All parsed "Memory Candidates" are currently funneled into MemoryExtractor.trackDecision(...), which stores them as type reference with a Decision:-prefixed name. This doesn’t match the candidate examples in tests (e.g., user preferences, project structure) and will misclassify memories. Consider encoding the intended type/scope in the candidate format and mapping to the appropriate extractor (user/feedback/project/reference) instead of treating everything as a decision.
| // Background memory maintenance (non-blocking) | ||
| if (startHookCfg.memory?.enabled !== false) { | ||
| import("@/memory/maintenance").then(({ MemoryMaintenance }) => { | ||
| MemoryMaintenance.run(startProjectDir).catch(() => {}) | ||
| }).catch(() => {}) | ||
| } |
There was a problem hiding this comment.
Background memory maintenance is gated only by memory.enabled. The config schema also introduces memory.consolidation, but it’s not consulted here, so users can’t disable this maintenance cycle independently of the entire memory system. Consider gating this block on memory.consolidation !== false (and/or reusing the same option name consistently).
| consolidation: z.boolean().optional().describe("Enable background memory consolidation on session start"), | ||
| scope: z.enum(["personal", "project", "global"]).optional().describe("Default scope for new memories"), |
There was a problem hiding this comment.
New config options memory.consolidation and memory.scope are added to the schema but appear unused in the codebase (no references found). Either wire them into memory creation/maintenance behavior (default scope for new memories; ability to toggle consolidation/maintenance) or remove them to avoid dead config surface area.
| consolidation: z.boolean().optional().describe("Enable background memory consolidation on session start"), | |
| scope: z.enum(["personal", "project", "global"]).optional().describe("Default scope for new memories"), |
| const matched = entries.filter((entry) => matchesTool(entry.matcher, toolName)) | ||
| if (matched.length === 0) return { action: "pass" } | ||
|
|
||
| const results = await Promise.all(matched.map((entry) => runHook(entry, env))) | ||
|
|
||
| for (const entry of entries) { | ||
| if (!matchesTool(entry.matcher, toolName)) continue | ||
| // Collect messages in original order, stopping after the first "block". | ||
| // This preserves sequential message semantics while hooks execute in parallel. | ||
| const messages: string[] = [] | ||
| let blocked = false | ||
|
|
||
| const result = await runHook(entry, env) | ||
| for (const result of results) { | ||
| if (result.message) messages.push(result.message) | ||
| if (result.action === "block") { | ||
| return { action: "block", message: messages.join("\n") } | ||
| blocked = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
runHooks now executes all matched hooks via Promise.all, even if an earlier hook would block. This changes behavior from short-circuit evaluation and can run additional side-effectful hooks unnecessarily (and can break ordering dependencies between hooks). If hook ordering/short-circuiting is part of the contract, consider running sequentially or adding an explicit opt-in for parallel execution.
| if (msg.parts.length === 0) continue | ||
|
|
||
| // Strip transcript-only parts before converting to model messages | ||
| const activeParts = msg.parts.filter((part) => !("transcriptOnly" in part && part.transcriptOnly)) |
There was a problem hiding this comment.
activeParts correctly strips transcriptOnly parts, but later logic in this function still inspects msg.parts (e.g., aborted-error handling / other part-based checks). This can lead to inconsistent behavior where transcript-only parts influence branching even though they’re not emitted to model messages. Consider switching those downstream checks to use activeParts (and optionally skipping messages when activeParts.length === 0).
| if (msg.parts.length === 0) continue | |
| // Strip transcript-only parts before converting to model messages | |
| const activeParts = msg.parts.filter((part) => !("transcriptOnly" in part && part.transcriptOnly)) | |
| // Strip transcript-only parts before converting to model messages | |
| const activeParts = msg.parts.filter((part) => !("transcriptOnly" in part && part.transcriptOnly)) | |
| if (activeParts.length === 0) continue |
…ntenance - Migrate 6 legacy types to 4 CC-compatible types (user/feedback/project/reference) - Add three-tier scope (personal/project/global) with auto-promotion - Add relevance scoring with time-based decay and reference verification - Add maintenance cycle: merge duplicates, decay, remove stale, verify refs - Add summary-bridge for memory extraction from compaction output - Add hook deployment verification (verify.ts) - Add hook execution LRU cache (100 entries, 5s TTL) for PreToolUse perf - Extend config with max_memory_tokens - DB migration: scope, description, agent, relevance_score columns + 3 indexes - Tests: 31 new tests for maintenance, promoter, summary-bridge, verify Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…of agent object Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t decay loop updateRelevance was setting time_updated: Date.now() which reset the decay clock on each maintenance cycle, causing decay to fire only once. Now only relevance_score is updated, preserving the original time_updated for proper multi-cycle exponential decay. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9 test cases covering: - Empty DB + no file → undefined - DB entries → "# Memory" header with sections - Type grouping (project/user/feedback/reference) - Agent-specific entry loading - Agent entry deduplication - Relevance weight sorting (high score first) - Token budget truncation - MEMORY.md file fallback - Description field in output Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
terisuke
force-pushed
the
feat/memory-v2-schema
branch
from
e682e05 to
3e4ddd8
Compare
Summary
verify.ts)Files Changed (22 files, +1889/-92)
New Modules
src/memory/maintenance.ts— Background maintenance cycle (219L)src/memory/promoter.ts— Scope auto-promotion (52L)src/memory/summary-bridge.ts— Compaction memory extraction (63L)src/hook/verify.ts— Hook deployment integrity check (127L)Modified
src/memory/types.ts— CC-compatible 4-type system + scopessrc/memory/store.ts— listByScope, listByAgent, updateRelevance, promotesrc/memory/injector.ts— Token-budgeted loading with relevance scoringsrc/memory/extractor.ts— Session state tracking, consolidationsrc/memory/file.ts— Backward-compatible frontmatter parsingsrc/hook/execute.ts— LRU cache for PreToolUse hookssrc/config/config.ts— max_memory_tokens configsrc/session/prompt.ts— Maintenance trigger + memory injectionMigration
migration/20260410120000_memory_v2_schema/migration.sqlTest plan
Known follow-ups
consolidateOnSessionStartandmergeDuplicateshave near-identical logic — deduplicate in follow-upupdateRelevancescore is unclamped — add bounds check in follow-up🤖 Generated with Claude Code