feat: add atomic memory consolidation (Store.Consolidate + MCP mem_consolidate)#246
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an atomic “memory consolidation” operation to merge multiple observations into a single new observation, and exposes it via a new MCP tool (mem_consolidate) for agent-driven memory garbage collection.
Changes:
- Introduces
Store.Consolidate()to soft-delete N observations and insert 1 consolidated observation within a singlewithTx()transaction (including sync mutation enqueueing). - Registers and implements MCP tool
mem_consolidatein the agent profile with project auto-detection. - Adds store-layer tests covering soft-delete behavior, atomic rollback, sync mutation enqueueing, and skipping non-existent IDs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/store/store.go | Adds Store.Consolidate() transactional soft-delete + insert with sync mutation enqueueing. |
| internal/mcp/mcp.go | Registers mem_consolidate tool and adds request handling/argument parsing. |
| internal/store/store_test.go | Adds TDD tests for consolidate behavior (delete + insert, rollback, sync mutations, missing IDs). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Phase 1: Soft-delete sources (ignore non-existent) | ||
| for _, id := range idsToDelete { | ||
| obs, err := s.getObservationTx(tx, id) | ||
| if err == sql.ErrNoRows { | ||
| continue // silently ignore non-existent or already-deleted | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
idsToDelete is processed as-is. If the caller passes duplicate IDs, this will enqueue multiple delete sync mutations for the same observation (second UPDATE will be a no-op due to deleted_at IS NULL, but the mutation still gets enqueued). Consider de-duplicating idsToDelete (and optionally filtering out <=0 IDs) before the loop to keep sync mutations consistent and avoid unnecessary work.
| // Parse ids_to_delete from JSON array | ||
| rawIDs, _ := req.GetArguments()["ids_to_delete"].([]interface{}) | ||
| var idsToDelete []int64 | ||
| for _, raw := range rawIDs { | ||
| switch v := raw.(type) { | ||
| case float64: | ||
| idsToDelete = append(idsToDelete, int64(v)) | ||
| case json.Number: | ||
| n, _ := v.Int64() | ||
| idsToDelete = append(idsToDelete, n) | ||
| } |
There was a problem hiding this comment.
The ids_to_delete parsing silently truncates float64 values and ignores json.Number conversion errors, which can lead to surprising behavior for non-integer inputs (e.g., 1.9 becomes 1) and makes it hard to debug bad requests. Since this tool is destructive, consider validating that every element is a positive integer and returning a clear error if any element is invalid/out of range, instead of best-effort parsing.
| // Empty session_id should cause the INSERT to fail (NOT NULL constraint) | ||
| _, err = s.Consolidate([]int64{id1}, AddObservationParams{ | ||
| SessionID: "", // this should cause failure |
There was a problem hiding this comment.
This test comment says the INSERT should fail due to a NOT NULL constraint, but an empty string is still non-NULL in SQLite. The failure here is more likely due to the observations.session_id foreign key referencing sessions(id) (or another constraint), so the comment should be updated to reflect the actual reason the insert fails to keep the test self-explanatory.
| // Empty session_id should cause the INSERT to fail (NOT NULL constraint) | |
| _, err = s.Consolidate([]int64{id1}, AddObservationParams{ | |
| SessionID: "", // this should cause failure | |
| // Empty session_id is invalid and should cause the INSERT to fail. | |
| _, err = s.Consolidate([]int64{id1}, AddObservationParams{ | |
| SessionID: "", // invalid session_id; this should cause failure |
Summary / Resumen
Adds
Store.Consolidate()— an atomic operation that soft-deletes N source observations and creates a single new consolidated observation within a singlewithTx()transaction. Also registers themem_consolidateMCP tool inProfileAgent.Changes
Store Layer
Consolidate(idsToDelete []int64, p AddObservationParams) (int64, error): Atomic soft-delete + insert in single withTx()MCP Layer
mem_consolidatein ProfileAgenthandleConsolidate()with project auto-detection (same pattern as handleSave)Tests (4 TDD)
Closes #242