feat(data-layer): DL-14 - Manage Relationships & State Tags#103
Conversation
Implements comprehensive relationship and state tag management in Neo4j for entity-to-entity connections and dynamic entity status tracking. ## Relationships (Neo4j edges) New schemas (relationships.py): - RelationshipType enum: 7 types (MEMBER_OF, OWNS, KNOWS, ALLIED_WITH, HOSTILE_TO, LOCATED_IN, PARTICIPATES_IN) - Direction enum: OUTGOING, INCOMING, BOTH for query filtering - RelationshipCreate/Update/Response: Full CRUD with property storage - RelationshipFilter/ListResponse: Flexible querying by entity, type, direction Neo4j tools (neo4j_tools.py): - neo4j_create_relationship: Create typed edges with entity validation - neo4j_get_relationship: Retrieve by Neo4j internal ID - neo4j_list_relationships: Filter by entity/type/direction with pagination - neo4j_update_relationship: Update properties, preserve created_at - neo4j_delete_relationship: Remove relationship edge ## State Tags (Dynamic entity status) New schemas (relationships.py): - StateTag enum: 16 tags for entity instance status - Vital: alive, dead, unconscious, wounded - Visibility: hidden, revealed - Disposition: hostile, friendly, neutral - Combat: prone, grappled, restrained, incapacitated - Mental: charmed, frightened, stunned, confused - StateTagUpdate/Response: Atomic add/remove operations Neo4j tools (neo4j_tools.py): - neo4j_update_state_tags: Atomic tag updates with archetype validation - neo4j_get_state_tags: Retrieve current tags for entity ## Base Schemas (base.py) Exported existing enums from relationships for external use ## Authority Rules (auth.py) Added 7 authority rules: - Relationship writes: CanonKeeper only - Relationship reads: All agents - State tag writes: CanonKeeper only - State tag reads: All agents ## Tests Added 23 comprehensive tests (test_relationship_tools.py): - Relationship CRUD: create (entity validation, all types), get, list (direction/type filtering), update, delete - State tags: add, remove, atomic add+remove, archetype validation, get - Error handling: entity not found, relationship not found, archetype validation - All tests use proper mocking patterns with @patch decorators ## Architecture Decisions - Relationships as Neo4j edges enable graph traversal and rich queries - State tags only on instances (not archetypes) enforces instance-specific state - Atomic tag updates prevent race conditions in concurrent modifications - Direction enum supports flexible relationship queries (incoming/outgoing/both) - All write operations require CanonKeeper authority for data integrity Implements: DL-14 Dependencies: DL-2 (Entity Management) Enables: DL-15 (Parties), DL-25 (Combat), gameplay mechanics All 23 tests passing ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
github-actions
Bot
added
area/data-layer
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14a9e57e77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if entity_check[0]["type"] == "archetype": | ||
| raise ValueError( | ||
| f"Cannot set state tags on archetype {params.entity_id}. " | ||
| "State tags are only valid on entity instances." |
There was a problem hiding this comment.
The archetype guard in neo4j_update_state_tags checks entity_check[0]["type"] == "archetype", but the preceding query populates type from e.entity_type, whose values are things like character or location for both archetypes and instances. The archetype flag is stored in is_archetype, so this condition never triggers and callers can successfully add/remove state tags on archetypes, despite the function claiming to forbid it. Any neo4j_update_state_tags call against an archetype node will mutate its tags instead of raising, undermining the intended instance-only restriction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR implements DL-14: Manage Relationships & State Tags, introducing a comprehensive Neo4j-based system for managing typed relationships between entities and dynamic state tags for entity status tracking.
Key Changes:
- 7 relationship types (MEMBER_OF, OWNS, KNOWS, ALLIED_WITH, HOSTILE_TO, LOCATED_IN, PARTICIPATES_IN) with full CRUD operations
- 16 state tags for tracking entity status (vital status, visibility, disposition, combat states, mental states)
- 7 new tools with CanonKeeper authority for write operations and universal read access
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
packages/data-layer/src/monitor_data/schemas/relationships.py |
New schema definitions for relationship and state tag operations, including 3 enums and 7 schema classes |
packages/data-layer/src/monitor_data/tools/neo4j_tools.py |
Added 5 relationship management tools and 2 state tag tools with Neo4j Cypher queries |
packages/data-layer/tests/test_tools/test_relationship_tools.py |
Comprehensive test suite with 23 tests covering CRUD operations, validation, and edge cases |
packages/data-layer/src/monitor_data/schemas/__init__.py |
Exported all new relationship and state tag schemas for external use |
packages/data-layer/src/monitor_data/middleware/auth.py |
Added authority rules for 7 new tools (5 CanonKeeper-only writes, 2 universal reads) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Verify relationship exists | ||
| existing = neo4j_get_relationship(relationship_id) | ||
| if not existing: | ||
| raise ValueError(f"Relationship {relationship_id} not found") | ||
|
|
||
| # Update properties (preserve created_at) | ||
| updated_props = { | ||
| **params.properties, | ||
| "created_at": existing.created_at.isoformat() if existing.created_at else None, | ||
| } | ||
|
|
||
| update_query = """ | ||
| MATCH ()-[r]->() | ||
| WHERE id(r) = $rel_id | ||
| SET r = $props | ||
| RETURN id(r) as rel_id | ||
| """ | ||
|
|
||
| result = client.execute_write( | ||
| update_query, {"rel_id": int(relationship_id), "props": updated_props} | ||
| ) | ||
|
|
||
| if not result: | ||
| raise ValueError(f"Failed to update relationship {relationship_id}") | ||
|
|
||
| # Return updated relationship | ||
| updated = neo4j_get_relationship(relationship_id) | ||
| if not updated: | ||
| raise ValueError(f"Relationship {relationship_id} not found after update") | ||
| return updated |
There was a problem hiding this comment.
The function performs three separate database queries: one to verify the relationship exists, one to update it, and one to retrieve the updated relationship. This creates race conditions and is inefficient. Consider combining the update and retrieval into a single query that returns the full relationship data after update, eliminating the need for the final query. This would also prevent race conditions where the relationship could be modified or deleted between the update and retrieval.
| WHEN e.state_tags IS NULL THEN $add_tags | ||
| ELSE [tag IN coalesce(e.state_tags, []) + $add_tags WHERE NOT tag IN $remove_tags] |
There was a problem hiding this comment.
The Cypher query doesn't handle duplicate tags that might already exist in the list. If a tag is already present in state_tags and is also in add_tags, it will appear twice in the resulting array. Consider using DISTINCT or a SET operation to ensure tags are unique: SET e.state_tags = [tag IN apoc.coll.toSet(coalesce(e.state_tags, []) + $add_tags) WHERE NOT tag IN $remove_tags] or similar approach to maintain uniqueness.
| WHEN e.state_tags IS NULL THEN $add_tags | |
| ELSE [tag IN coalesce(e.state_tags, []) + $add_tags WHERE NOT tag IN $remove_tags] | |
| WHEN e.state_tags IS NULL THEN apoc.coll.toSet($add_tags) | |
| ELSE [tag IN apoc.coll.toSet(coalesce(e.state_tags, []) + $add_tags) WHERE NOT tag IN $remove_tags] |
| type(r) as rel_type, properties(r) as props | ||
| """ | ||
|
|
||
| result = client.execute_read(query, {"rel_id": int(relationship_id)}) |
There was a problem hiding this comment.
The function converts relationship_id to int without error handling. If a non-numeric string is passed, this will raise a ValueError with a confusing message. Consider adding explicit validation or a try-except block to provide a clearer error message like "Invalid relationship ID format: must be a numeric string".
| result = client.execute_write(delete_query, {"rel_id": int(relationship_id)}) | ||
|
|
There was a problem hiding this comment.
The function converts relationship_id to int without error handling. If a non-numeric string is passed, this will raise a ValueError with a confusing message. Consider adding explicit validation or a try-except block to provide a clearer error message like "Invalid relationship ID format: must be a numeric string".
| result = client.execute_write(delete_query, {"rel_id": int(relationship_id)}) | |
| try: | |
| rel_id = int(relationship_id) | |
| except (TypeError, ValueError): | |
| raise ValueError("Invalid relationship ID format: must be a numeric string") from None | |
| result = client.execute_write(delete_query, {"rel_id": rel_id}) |
| def neo4j_update_state_tags(params: StateTagUpdate) -> StateTagResponse: | ||
| """ | ||
| Update state tags on an entity instance atomically. | ||
|
|
||
| Authority: CanonKeeper only | ||
| Use Case: DL-14 | ||
|
|
||
| Args: | ||
| params: State tag update parameters | ||
|
|
||
| Returns: | ||
| StateTagResponse with updated tags | ||
|
|
||
| Raises: | ||
| ValueError: If entity not found or is an archetype | ||
| """ | ||
| client = get_neo4j_client() | ||
|
|
||
| # Validate entity exists and is an instance | ||
| entity_check = client.execute_read( | ||
| """ | ||
| MATCH (e:Entity {id: $entity_id}) | ||
| RETURN e.id as id, e.entity_type as type | ||
| """, | ||
| {"entity_id": str(params.entity_id)}, | ||
| ) | ||
|
|
||
| if not entity_check: | ||
| raise ValueError(f"Entity {params.entity_id} not found") | ||
|
|
||
| if entity_check[0]["type"] == "archetype": | ||
| raise ValueError( | ||
| f"Cannot set state tags on archetype {params.entity_id}. " | ||
| "State tags are only valid on entity instances." | ||
| ) | ||
|
|
||
| # Convert tags to strings | ||
| add_tag_strs = [tag.value for tag in params.add_tags] | ||
| remove_tag_strs = [tag.value for tag in params.remove_tags] | ||
|
|
||
| # Update tags atomically | ||
| update_query = """ | ||
| MATCH (e:Entity {id: $entity_id}) | ||
| SET e.state_tags = | ||
| CASE | ||
| WHEN e.state_tags IS NULL THEN $add_tags | ||
| ELSE [tag IN coalesce(e.state_tags, []) + $add_tags WHERE NOT tag IN $remove_tags] | ||
| END | ||
| RETURN e.state_tags as tags | ||
| """ | ||
|
|
||
| result = client.execute_write( | ||
| update_query, | ||
| { | ||
| "entity_id": str(params.entity_id), | ||
| "add_tags": add_tag_strs, | ||
| "remove_tags": remove_tag_strs, | ||
| }, | ||
| ) | ||
|
|
||
| tags = result[0]["tags"] if result and result[0]["tags"] else [] | ||
|
|
||
| return StateTagResponse(entity_id=params.entity_id, state_tags=tags) |
There was a problem hiding this comment.
The function doesn't validate that at least one of add_tags or remove_tags is non-empty. While technically valid to call with both empty lists, it results in a no-op database write operation. Consider adding validation to require at least one operation, or document that empty operations are intentionally allowed.
| def neo4j_create_relationship(params: RelationshipCreate) -> RelationshipResponse: | ||
| """ | ||
| Create a typed relationship (edge) between two entities. | ||
|
|
||
| Authority: CanonKeeper only | ||
| Use Case: DL-14 | ||
|
|
||
| Args: | ||
| params: Relationship creation parameters | ||
|
|
||
| Returns: | ||
| RelationshipResponse with created relationship data | ||
|
|
||
| Raises: | ||
| ValueError: If either entity doesn't exist | ||
| """ | ||
| client = get_neo4j_client() | ||
|
|
||
| # Validate both entities exist | ||
| from_exists = client.execute_read( | ||
| "MATCH (e:Entity {id: $entity_id}) RETURN e.id", | ||
| {"entity_id": str(params.from_entity_id)}, | ||
| ) | ||
| if not from_exists: | ||
| raise ValueError(f"From entity {params.from_entity_id} not found") | ||
|
|
||
| to_exists = client.execute_read( | ||
| "MATCH (e:Entity {id: $entity_id}) RETURN e.id", | ||
| {"entity_id": str(params.to_entity_id)}, | ||
| ) | ||
| if not to_exists: | ||
| raise ValueError(f"To entity {params.to_entity_id} not found") | ||
|
|
||
| # Create relationship with properties | ||
| now = datetime.now(timezone.utc) | ||
| props = {**params.properties, "created_at": now.isoformat()} | ||
|
|
||
| create_query = f""" | ||
| MATCH (from:Entity {{id: $from_id}}) | ||
| MATCH (to:Entity {{id: $to_id}}) | ||
| CREATE (from)-[r:{params.rel_type.value} $props]->(to) | ||
| RETURN id(r) as rel_id, type(r) as rel_type, properties(r) as props | ||
| """ | ||
|
|
||
| result = client.execute_write( | ||
| create_query, | ||
| { | ||
| "from_id": str(params.from_entity_id), | ||
| "to_id": str(params.to_entity_id), | ||
| "props": props, | ||
| }, | ||
| ) | ||
|
|
||
| if not result: | ||
| raise ValueError("Failed to create relationship") | ||
|
|
||
| rel_data = result[0] | ||
| return RelationshipResponse( | ||
| relationship_id=str(rel_data["rel_id"]), | ||
| from_entity_id=params.from_entity_id, | ||
| to_entity_id=params.to_entity_id, | ||
| rel_type=params.rel_type, | ||
| properties=rel_data["props"], | ||
| created_at=now, | ||
| ) | ||
|
|
There was a problem hiding this comment.
There is no validation to prevent creating self-referencing relationships where from_entity_id equals to_entity_id. While this might be intentional for some relationship types (e.g., an entity owning itself), for most relationship types like KNOWS, ALLIED_WITH, or HOSTILE_TO, self-references don't make semantic sense. Consider adding validation or at minimum a test case to document the expected behavior for self-referencing relationships.
| result = client.execute_write( | ||
| update_query, {"rel_id": int(relationship_id), "props": updated_props} |
There was a problem hiding this comment.
The function converts relationship_id to int without error handling. If a non-numeric string is passed, this will raise a ValueError with a confusing message. Consider adding explicit validation or a try-except block to provide a clearer error message like "Invalid relationship ID format: must be a numeric string".
| result = client.execute_write( | |
| update_query, {"rel_id": int(relationship_id), "props": updated_props} | |
| try: | |
| rel_id_int = int(relationship_id) | |
| except (TypeError, ValueError): | |
| raise ValueError("Invalid relationship ID format: must be a numeric string") | |
| result = client.execute_write( | |
| update_query, {"rel_id": rel_id_int, "props": updated_props} |
| query_params["entity_id"] = str(params.entity_id) | ||
|
|
||
| if params.rel_type: | ||
| where_clauses.append(f"type(r) = '{params.rel_type.value}'") |
There was a problem hiding this comment.
The relationship type filter is vulnerable to Cypher injection. The rel_type.value is inserted directly into the query string using string formatting rather than being passed as a parameter. While rel_type is validated as an enum, using string interpolation in query construction is an anti-pattern that could lead to issues if the enum validation is ever bypassed or modified. Use parameterized queries instead by adding the type filter to the WHERE clause with a parameter.
| where_clauses.append(f"type(r) = '{params.rel_type.value}'") | |
| where_clauses.append("type(r) = $rel_type") | |
| query_params["rel_type"] = params.rel_type.value |
| # Verify relationship exists | ||
| existing = neo4j_get_relationship(relationship_id) | ||
| if not existing: | ||
| raise ValueError(f"Relationship {relationship_id} not found") | ||
|
|
||
| delete_query = """ | ||
| MATCH ()-[r]->() | ||
| WHERE id(r) = $rel_id | ||
| DELETE r | ||
| RETURN count(r) as deleted_count | ||
| """ | ||
|
|
||
| result = client.execute_write(delete_query, {"rel_id": int(relationship_id)}) |
There was a problem hiding this comment.
The function performs two separate database queries: first to verify the relationship exists, then to delete it. This creates a race condition where the relationship could be deleted by another operation between these two calls. Consider combining these into a single query that attempts to delete and returns an error if nothing was deleted, or use a transaction to ensure atomicity.
| entity2_id = uuid4() | ||
|
|
||
| # Test each relationship type | ||
| for rel_type in RelationshipType: |
There was a problem hiding this comment.
This for-loop may attempt to iterate over a non-iterable instance of class type.
Addresses all 14 review comments from Copilot code review:
## Critical Fixes
**P1: Archetype validation bug** (line 3888)
- Fixed query to check `e.is_archetype` instead of `e.entity_type`
- Now correctly prevents state tags on archetypes
- Updated test mocks to use `{"is_archetype": True/False}`
**Cypher injection prevention** (line 3706)
- Parameterized rel_type filter in list_relationships
- Changed from string interpolation to query parameter
- Note: CREATE statement (line 3602) cannot parameterize rel_type due to Neo4j limitation, but enum validation provides safety
## Data Integrity Fixes
**DELETE count query** (line 3838)
- Fixed query to use `WITH r DELETE r RETURN count(*)`
- Previously returned 0 because r was deleted before count
- Now correctly returns deleted count
**Duplicate tags handling** (line 3901)
- Rewrote state_tags update query to use REDUCE for deduplication
- Remove tags first, then add, then deduplicate
- If same tag in both add/remove, addition takes precedence
## Validation Improvements
**Relationship ID validation** (lines 3651, 3798, 3842)
- Added try-except blocks for int conversion
- Provides clear error: "Invalid relationship ID format: must be a numeric string"
- Applied to get, update, and delete functions
**Empty tags validation** (line 3917)
- Requires at least one of add_tags or remove_tags to be non-empty
- Prevents no-op database write operations
**Self-reference validation** (line 3627)
- Prevents self-referencing relationships for KNOWS, ALLIED_WITH, HOSTILE_TO
- Allows OWNS and other types where self-reference may be valid
- Provides clear error message
## Architecture Notes
- CREATE statement line 3602: Cannot parameterize relationship type due to Neo4j Cypher limitations. Enum validation provides security.
- Race condition comments (lines 3593, 3808, 3841): Acknowledged but acceptable given entity/relationship validation and Neo4j ACID properties
All 23 tests passing ✅
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary
Implements DL-14: Manage Relationships & State Tags - Comprehensive Neo4j-based relationship management and dynamic state tag system for entity connections and status tracking.
Relationships (Neo4j Edges)
7 Relationship Types:
MEMBER_OF- Entity belongs to organization/groupOWNS- Entity owns another entity/objectKNOWS- Social relationship (acquaintance)ALLIED_WITH- Formal alliance relationshipHOSTILE_TO- Antagonistic relationshipLOCATED_IN- Spatial containmentPARTICIPATES_IN- Event/activity participation5 Relationship Tools:
neo4j_create_relationship- Create typed edges with entity validationneo4j_get_relationship- Retrieve by Neo4j internal IDneo4j_list_relationships- Filter by entity/type/direction with paginationneo4j_update_relationship- Update properties, preserve created_atneo4j_delete_relationship- Remove relationship edgeQuery Features:
State Tags (Dynamic Entity Status)
16 State Tags:
alive,dead,unconscious,woundedhidden,revealedhostile,friendly,neutralprone,grappled,restrained,incapacitatedcharmed,frightened,stunned,confused2 State Tag Tools:
neo4j_update_state_tags- Atomic add/remove operations with archetype validationneo4j_get_state_tags- Retrieve current tags for entityKey Features:
Implementation
Files Changed
New:
packages/data-layer/src/monitor_data/schemas/relationships.py(172 lines)Modified:
packages/data-layer/src/monitor_data/tools/neo4j_tools.py(+450 lines)Modified:
packages/data-layer/src/monitor_data/middleware/auth.pyModified:
packages/data-layer/src/monitor_data/schemas/__init__.pyNew:
packages/data-layer/tests/test_tools/test_relationship_tools.py(692 lines)Authority Matrix
neo4j_create_relationshipneo4j_get_relationshipneo4j_list_relationshipsneo4j_update_relationshipneo4j_delete_relationshipneo4j_update_state_tagsneo4j_get_state_tagsTest Coverage
✅ 23 tests, all passing
Relationship CRUD (15 tests)
State Tags (8 tests)
Architecture Decisions
Dependencies
Requires: DL-2 (Entity Management) - Validates entities exist before creating relationships
Enables:
Testing
All 23 tests passing ✅
Implements: DL-14
🤖 Generated with Claude Code