Skip to content

feat(memory): add relation graph for conflict detection and retrieval#708

Open
mvanhorn wants to merge 3 commits intovolcengine:mainfrom
mvanhorn:feat/memory-relation-graph
Open

feat(memory): add relation graph for conflict detection and retrieval#708
mvanhorn wants to merge 3 commits intovolcengine:mainfrom
mvanhorn:feat/memory-relation-graph

Conversation

@mvanhorn
Copy link
Contributor

Summary

  • Add a typed relation model between memories (supersedes, contradicts, related_to, derived_from) for conflict detection, supersession tracking, and related-context retrieval
  • Integrate automatic relation creation into the dedup pipeline: MERGE decisions produce supersedes relations, DELETE decisions produce contradicts relations
  • Extend HierarchicalRetriever.retrieve() with an optional follow_relations parameter that filters superseded memories and surfaces related context
  • Add GET /memories/{uri}/relations API endpoint for querying memory relations

Motivation

OpenViking's memory deduplicator operates on vector similarity alone. When memories contradict each other (e.g., "user prefers dark mode" vs "user switched to light mode"), the system has no structured way to track the relationship between them. In #269, @qin-ctx noted that "relation table capability hasn't been built" when discussing how to clean up superseded memories.

This affects three areas:

  1. Dedup quality - MERGE decisions discard the old memory without recording the relationship
  2. Retrieval accuracy - no way to prefer newer memories over superseded ones
  3. Memory cleanup - [Feature]: openviking如何将单个的长期记忆、资源删除掉?连同因为有这些资源在而引发的额外自动记忆 #269's original question (how to delete individual memories and their derived context) remains unsolved without relations

Example flow

Session 1: "I prefer dark mode"  -> creates viking://user/memories/preferences/theme_001
Session 5: "I switched to light mode" -> creates theme_002
    Dedup: MERGE decision -> relation: theme_002 --supersedes--> theme_001
Retrieval: query "theme preference" -> returns theme_002, notes theme_001 is superseded

Changes

File Change
openviking/storage/memory_relation_store.py New - MemoryRelationStore with CRUD, RelationType enum, MemoryRelation dataclass
openviking/storage/collection_schemas.py Add memory_relation_collection schema for future VikingDB persistence
openviking/session/memory_deduplicator.py Accept optional relation_store, create relations on MERGE/DELETE
openviking/retrieve/hierarchical_retriever.py Accept optional memory_relation_store, add follow_relations param, filter superseded
openviking/server/models.py Add MemoryRelationResponse and MemoryRelationListResponse
openviking/server/routers/memory_relations.py New - GET /memories/{uri}/relations endpoint
tests/unit/storage/test_relation_store.py Unit tests for relation store CRUD
tests/unit/session/test_dedup_relations.py Integration test: dedup MERGE -> supersedes relation

Evidence

Source Evidence Engagement
#269 qin-ctx: "relation table capability hasn't been built" Maintainer-acknowledged gap
deduplicator.py MERGE/DELETE decisions exist but create no relation audit trail Core code gap
hierarchical_retriever.py MAX_RELATIONS = 5 constant defined but unused for memory relations Dead code signal
Reddit "Built an AI memory system based on cognitive science" 96 upvotes, 91 comments
Reddit "hybrid memory system that learns from its own mistakes" 14 upvotes, 24 comments

The MAX_RELATIONS = 5 constant already exists in hierarchical_retriever.py but is only used for resource relations, not memory relations. This PR extends the concept to the memory layer, where qin-ctx identified the gap in #269.

Alternatives considered

Embedding relations in Context metadata fields. Simpler but doesn't support bidirectional queries or the kind of cleanup described in #269.

Test plan

  • pytest tests/unit/storage/test_relation_store.py - CRUD operations, dedup, deletion, round-trip serialization
  • pytest tests/unit/session/test_dedup_relations.py - MERGE creates supersedes, DELETE creates contradicts, no-store no-crash, skip creates nothing
  • Verify no new external dependencies added
  • Review relation_store parameter is optional everywhere (backward compatible)

Generated with Claude Code | Refs: #269

Add a lightweight typed relation model between memories to enable
conflict detection during dedup, supersession tracking, and
related-context retrieval.

Changes:
- New MemoryRelationStore with CRUD operations and supersession queries
- New RelationType enum: supersedes, contradicts, related_to, derived_from
- MemoryDeduplicator now creates supersedes/contradicts relations on
  MERGE/DELETE decisions when a relation store is provided
- HierarchicalRetriever supports follow_relations parameter to filter
  superseded memories and surface related context
- New API endpoint GET /memories/{uri}/relations for relation queries
- Collection schema for future VikingDB persistence
- Unit tests for relation store CRUD and dedup integration

Refs: volcengine#269
Copy link
Collaborator

@qin-ctx qin-ctx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This PR introduces a typed memory relation graph (supersedes, contradicts, related_to, derived_from) to enable conflict detection during dedup and retrieval filtering. The data model and store API are well-designed, but two issues need attention before merging:

  1. Bug: The supersedes relation direction is inverted for MERGE decisions — the surviving (updated) memory gets marked as superseded, causing it to be filtered out during retrieval.
  2. Incomplete wiring: The relation store, router, dedup integration, and retriever integration are all defined but never connected in service initialization, making the feature dead code.

Additionally, ~50% of changed files contain only ruff formatting changes unrelated to the feature. Consider splitting those into a separate PR.

)

for action in result.actions:
if action.decision == MemoryActionDecision.MERGE:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking) The supersedes relation direction is inverted for MERGE decisions.

In the MERGE flow, the candidate memory is not created (decision=NONE). Its content is merged into the existing memory, which survives with updated content. However, this code creates:

pending://... --supersedes--> existing_memory_uri

When HierarchicalRetriever._apply_memory_relations() later calls is_superseded(existing_uri), it returns True because there's a supersedes relation targeting the existing URI. This causes the surviving, updated memory to be filtered out of retrieval results.

The PR description's example assumes MERGE produces two distinct URIs (theme_001 and theme_002), but the actual code flow is:

  • Candidate has no URI → falls back to pending:// scheme
  • Existing memory keeps its URI and gets updated content
  • The relation incorrectly marks the kept memory as superseded

For MERGE, either:

  1. Don't create a supersedes relation (MERGE is an update, not a replacement), or
  2. Use a different relation type (e.g., derived_from) to record the merge audit trail without affecting retrieval filtering

from openviking.server.models import MemoryRelationListResponse, MemoryRelationResponse, Response
from openviking.storage.memory_relation_store import MemoryRelationStore, RelationType

router = APIRouter(prefix="/api/v1/memories", tags=["memory-relations"])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Design] (blocking) The entire feature is dead code — nothing is wired up.

  1. This router is never registered in app.py (app.include_router not called, not added to routers/__init__.py)
  2. MemoryRelationStore is never instantiated during service startup
  3. MemoryDeduplicator instantiation in compressor.py:68 doesn't pass relation_store
  4. HierarchicalRetriever instantiation in viking_fs.py:647,792 doesn't pass memory_relation_store

Please either:

  • Add the wiring code (service init → store creation → inject into dedup/retriever/router), or
  • Explicitly document this as Phase 1 (data model only) with a follow-up issue for the wiring

_ctx: RequestContext = Depends(get_request_context),
) -> Response:
"""Get typed relations for a memory URI.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] (non-blocking) RelationType(type) raises ValueError for invalid enum values, resulting in a 500 instead of 400.

Also, the direction parameter accepts any string without validation — passing direction="invalid" silently returns empty results.

Consider:

try:
    relation_type = RelationType(type) if type else None
except ValueError:
    return Response(status="error", error=ErrorInfo(code="INVALID_ARGUMENT", message=f"Invalid relation type: {type}"))

if direction not in ("outgoing", "incoming", "both"):
    return Response(status="error", error=ErrorInfo(code="INVALID_ARGUMENT", message=f"Invalid direction: {direction}"))

Or use FastAPI's enum parameter validation by defining direction as a Literal["outgoing", "incoming", "both"].

{"FieldName": "target_uri", "FieldType": "string"},
{"FieldName": "relation_type", "FieldType": "string"},
{"FieldName": "created_at", "FieldType": "date_time"},
{"FieldName": "metadata", "FieldType": "string"},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] (non-blocking) The schema defines account_id but MemoryRelation dataclass has no corresponding field. When you implement VikingDB persistence, this mismatch will need to be resolved — either add account_id to the dataclass or remove it from the schema.

results.sort(key=lambda x: x.score, reverse=True)
return results

async def _apply_memory_relations(self, matched: List[MatchedContext]) -> List[MatchedContext]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] (non-blocking) _apply_memory_relations has no test coverage. Given the supersedes direction bug identified in _record_dedup_relations, an integration test that exercises the full dedup → retrieval pipeline would catch this class of issues.

Consider adding a test that:

  1. Creates a MERGE dedup result and records relations
  2. Runs _apply_memory_relations on a result set containing the merged memory
  3. Asserts the merged memory is not filtered out

…dd validation

- Fix inverted supersedes direction in MERGE dedup: the surviving
  (existing) memory now supersedes the candidate, not the other way
  around. Previously, _apply_memory_relations would filter out the
  surviving memory during retrieval.
- Wire up the full feature:
  - Register memory_relations router in __init__.py and app.py
  - Pass MemoryRelationStore to MemoryDeduplicator via compressor.py
  - Pass memory_relation_store to HierarchicalRetriever via viking_fs.py
- Add input validation in memory_relations router:
  - RelationType enum validation returns 400 instead of 500
  - Direction parameter validation
- Add account_id field to MemoryRelation dataclass to match the
  VikingDB collection schema

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mvanhorn
Copy link
Contributor Author

Addressed all feedback in 74a93b4:

  • Supersedes direction bug: Flipped source/target in MERGE relations so the surviving memory supersedes the candidate, not the other way around
  • Dead code wiring: Registered memory_relations_router in app.py, passed MemoryRelationStore to MemoryDeduplicator via compressor.py, and passed memory_relation_store to HierarchicalRetriever via viking_fs.py
  • Enum validation: RelationType and direction parameters now return 400 with a clear message instead of 500
  • Schema mismatch: Added account_id field to MemoryRelation dataclass to match the collection schema

@mvanhorn
Copy link
Contributor Author

Addressed all feedback in 74a93b4:

  1. Supersedes direction fixed: In MERGE dedup, the surviving (existing) memory now supersedes the candidate. Previously the direction was inverted - _apply_memory_relations would have filtered out the surviving memory during retrieval.

  2. Feature wired up:

    • Registered memory_relations router in __init__.py and app.py
    • MemoryRelationStore passed to MemoryDeduplicator via compressor.py
    • memory_relation_store passed to HierarchicalRetriever via viking_fs.py
  3. Input validation: RelationType enum validation now returns 400 (not 500) for invalid values. Added direction parameter validation.

  4. Schema alignment: Added account_id field to MemoryRelation dataclass to match collection_schemas.py.

Re: integration test coverage for the dedup-to-retrieval pipeline - agreed that would strengthen confidence. Happy to add in a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants