Implement Story/Scene/Turn CRUD operations (DL-4)#66
Conversation
| \nTests not detected in this PR. Please add/confirm coverage where applicable. |
Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive CRUD operations for Stories (Neo4j), Scenes (MongoDB), and Turns (MongoDB) with cross-database validation and status transition enforcement, completing the DL-4 use case for managing narrative structures.
Key Changes:
- Neo4j Story operations with universe/participant validation and scene count aggregation
- MongoDB Scene operations with status transition enforcement (active → finalizing → completed)
- MongoDB Turn append functionality with speaker validation and entity verification
- MongoDB client infrastructure with automatic index creation for scenes collection
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
packages/data-layer/tests/test_tools/test_story_tools.py |
13 unit tests covering story CRUD with universe/PC validation, pagination, and filtering |
packages/data-layer/tests/test_tools/test_scene_tools.py |
18 unit tests covering scene/turn CRUD with status transitions and entity validation |
packages/data-layer/src/monitor_data/tools/neo4j_tools.py |
Adds 4 story operations (create, get, update, list) with PARTICIPATES edge management |
packages/data-layer/src/monitor_data/tools/mongodb_tools.py |
New file with 5 scene/turn operations with cross-database validation against Neo4j |
packages/data-layer/src/monitor_data/schemas/stories.py |
New file defining Story schemas with StoryType/StoryStatus enums and pagination support |
packages/data-layer/src/monitor_data/schemas/scenes.py |
New file defining Scene/Turn schemas with field validators for speaker/entity consistency |
packages/data-layer/src/monitor_data/db/mongodb.py |
New MongoDB client with connection pooling and automatic index creation |
packages/data-layer/src/monitor_data/middleware/auth.py |
Updates authority matrix for scene/turn operations (CanonKeeper, Narrator, and universal access) |
packages/data-layer/src/monitor_data/db/__init__.py |
Adds MongoDB client exports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_append_turn_with_entity( | ||
| mock_get_mongo: Mock, | ||
| mock_get_neo4j: Mock, | ||
| mock_mongodb_client: Mock, | ||
| mock_neo4j_client: Mock, | ||
| scene_data: Dict[str, Any], | ||
| entity_data: Dict[str, Any], | ||
| ): | ||
| """Test appending turn with entity speaker.""" | ||
| mock_get_mongo.return_value = mock_mongodb_client | ||
| mock_get_neo4j.return_value = mock_neo4j_client | ||
|
|
||
| # Mock MongoDB collection | ||
| collection = mock_mongodb_client.get_collection.return_value | ||
| collection.find_one.return_value = scene_data | ||
| collection.update_one.return_value = Mock(modified_count=1) | ||
|
|
||
| # Mock Neo4j entity check | ||
| mock_neo4j_client.execute_read.return_value = [{"id": entity_data["id"]}] | ||
|
|
||
| entity_id = UUID(entity_data["id"]) | ||
| params = TurnCreate( | ||
| speaker=Speaker.ENTITY, | ||
| entity_id=entity_id, | ||
| text="I attack the orc!", | ||
| ) | ||
|
|
||
| result = mongodb_append_turn(UUID(scene_data["scene_id"]), params) | ||
|
|
||
| assert result.text == "I attack the orc!" | ||
| assert result.speaker == Speaker.ENTITY | ||
| assert result.entity_id == entity_id | ||
|
|
There was a problem hiding this comment.
There's no test coverage for the TurnCreate validation that ensures entity_id is provided when speaker is ENTITY (defined in scenes.py lines 36-42). Consider adding a test that attempts to create a turn with speaker=Speaker.ENTITY but without providing an entity_id to verify the validation works correctly.
There was a problem hiding this comment.
Added test test_turn_create_entity_without_entity_id that validates entity_id is required when speaker is ENTITY. Commit: a3089d9
| if params.status is not None: | ||
| set_clauses.append("s.status = $status") | ||
| update_params["status"] = params.status.value | ||
| # If completing the story, set completed_at | ||
| if params.status.value == "completed": |
There was a problem hiding this comment.
The story update function doesn't enforce status transitions like the scene update function does (lines 224-241 in mongodb_tools.py). According to the acceptance criteria in the PR description, status transitions should follow: planned → active → completed → archived. Consider adding validation to ensure only valid status transitions are allowed, similar to how mongodb_update_scene handles SceneStatus transitions.
There was a problem hiding this comment.
Added status transition validation to neo4j_update_story enforcing planned → active → completed/abandoned transitions, matching the Scene implementation pattern. Also added 4 tests for valid and invalid transitions. Commit: a3089d9
| _instance: Optional["MongoDBClient"] = None | ||
| _client: Optional[MongoClient] = None | ||
| _db: Optional[Database] = None | ||
|
|
There was a problem hiding this comment.
The class attributes _instance, _client, and _db are declared but never used. The singleton pattern is implemented at the module level using _mongodb_client_instance (line 163). These class attributes should be removed to avoid confusion, or if they were intended for use, the implementation should be corrected to use them.
| _instance: Optional["MongoDBClient"] = None | |
| _client: Optional[MongoClient] = None | |
| _db: Optional[Database] = None |
There was a problem hiding this comment.
Removed unused class attributes _instance, _client, and _db from MongoDBClient. The singleton pattern is properly implemented at module level. Commit: 4eee22c
| if self._client is None: | ||
| self._client = MongoClient(self.uri) | ||
| self._db = self._client[self.database_name] | ||
| self._create_indexes() |
There was a problem hiding this comment.
Index creation runs on every connect() call (line 67). While create_index is idempotent in MongoDB, it still involves network round-trips. Consider adding a flag to track whether indexes have been created to avoid redundant index creation attempts, especially during testing scenarios where reset_mongodb_client() might be called frequently.
There was a problem hiding this comment.
Added _indexes_created flag to track index creation state and prevent redundant index operations on repeated connect() calls. Commit: 4eee22c
| def test_create_scene_with_participants( | ||
| mock_get_mongo: Mock, | ||
| mock_get_neo4j: Mock, | ||
| mock_mongodb_client: Mock, | ||
| mock_neo4j_client: Mock, | ||
| story_data: Dict[str, Any], | ||
| universe_data: Dict[str, Any], | ||
| entity_data: Dict[str, Any], | ||
| ): | ||
| """Test scene creation with participating entities.""" | ||
| mock_get_mongo.return_value = mock_mongodb_client | ||
| mock_get_neo4j.return_value = mock_neo4j_client | ||
|
|
||
| # Mock Neo4j story/universe check and entity check | ||
| mock_neo4j_client.execute_read.side_effect = [ | ||
| [{"story_id": story_data["id"], "universe_id": universe_data["id"]}], | ||
| [{"id": entity_data["id"]}], # entity check | ||
| ] | ||
|
|
||
| # Mock MongoDB collection | ||
| collection = mock_mongodb_client.get_collection.return_value | ||
| collection.insert_one.return_value = Mock(inserted_id="mongo_obj_id") | ||
|
|
||
| entity_id = UUID(entity_data["id"]) | ||
| params = SceneCreate( | ||
| story_id=UUID(story_data["id"]), | ||
| universe_id=UUID(universe_data["id"]), | ||
| title="Opening Scene", | ||
| participating_entities=[entity_id], | ||
| ) | ||
|
|
||
| result = mongodb_create_scene(params) | ||
|
|
||
| assert entity_id in result.participating_entities | ||
| assert mock_neo4j_client.execute_read.call_count == 2 | ||
|
|
There was a problem hiding this comment.
There's test coverage for invalid story validation (test_create_scene_invalid_story) but no corresponding test for invalid participating entities. The implementation validates participating entities exist in Neo4j (mongodb_tools.py lines 70-81), but this validation path is not tested. Consider adding a test like test_create_scene_invalid_entity to ensure this validation works correctly.
There was a problem hiding this comment.
Added test test_create_scene_invalid_entity that validates participating_entities exist in Neo4j. Commit: a3089d9
| scenes.append( | ||
| SceneResponse( | ||
| scene_id=UUID(scene_doc["scene_id"]), | ||
| story_id=UUID(scene_doc["story_id"]), | ||
| universe_id=UUID(scene_doc["universe_id"]), | ||
| title=scene_doc["title"], | ||
| purpose=scene_doc["purpose"], | ||
| status=SceneStatus(scene_doc["status"]), | ||
| order=scene_doc.get("order"), | ||
| location_ref=UUID(scene_doc["location_ref"]) | ||
| if scene_doc.get("location_ref") | ||
| else None, | ||
| participating_entities=[ | ||
| UUID(eid) for eid in scene_doc.get("participating_entities", []) | ||
| ], | ||
| turns=turns, | ||
| proposed_changes=[UUID(pid) for pid in scene_doc.get("proposed_changes", [])], | ||
| canonical_outcomes=[ | ||
| UUID(cid) for cid in scene_doc.get("canonical_outcomes", []) | ||
| ], | ||
| summary=scene_doc.get("summary", ""), | ||
| created_at=scene_doc["created_at"], | ||
| updated_at=scene_doc["updated_at"], | ||
| completed_at=scene_doc.get("completed_at"), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The logic for converting scene documents to SceneResponse objects is duplicated between mongodb_get_scene (lines 177-194) and mongodb_list_scenes (lines 335-360). Consider extracting this into a helper function to reduce code duplication and improve maintainability.
There was a problem hiding this comment.
Extracted helper function _convert_scene_doc_to_response to eliminate duplicate scene conversion logic between mongodb_get_scene and mongodb_list_scenes. Commit: 4eee22c
| update_query = "MATCH (s:Story {id: $id})\n" "SET " + set_clause + "\n" "RETURN s" | ||
|
|
||
| result = client.execute_write(update_query, update_params) | ||
| s = result[0]["s"] |
| """ | ||
|
|
||
| import os | ||
| from typing import Optional, Dict, Any, List |
There was a problem hiding this comment.
Import of 'Dict' is not used.
Import of 'Any' is not used.
Import of 'List' is not used.
| from typing import Optional, Dict, Any, List | |
| from typing import Optional |
There was a problem hiding this comment.
Removed unused imports Dict, Any, and List from mongodb.py. Commit: 4eee22c
| """ | ||
|
|
||
| from datetime import datetime, timezone | ||
| from typing import Optional, List, Dict, Any |
There was a problem hiding this comment.
Import of 'List' is not used.
| from typing import Optional, List, Dict, Any | |
| from typing import Optional, Dict, Any |
There was a problem hiding this comment.
Removed unused import List from mongodb_tools.py. Commit: 4eee22c
| Turns are individual exchanges within scenes. | ||
| """ | ||
|
|
||
| from datetime import datetime, timezone |
There was a problem hiding this comment.
Import of 'timezone' is not used.
| from datetime import datetime, timezone | |
| from datetime import datetime |
There was a problem hiding this comment.
Removed unused import timezone from scenes.py. Commit: 4eee22c
|
@copilot apply changes based on the comments in this thread |
…ons, fix comments Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| MATCH (s:Story {id: $id}) | ||
| OPTIONAL MATCH (s)-[:HAS_SCENE]->(sc:Scene) | ||
| OPTIONAL MATCH (pc)-[:PARTICIPATES]->(s) | ||
| WHERE pc:EntityArchetype OR pc:EntityInstance |
There was a problem hiding this comment.
Optional match drops stories with no participants
The neo4j_get_story query uses OPTIONAL MATCH (pc)-[:PARTICIPATES]->(s) followed by WHERE pc:EntityArchetype OR pc:EntityInstance, which null‑filters the row whenever no participant node is found. For any story that legitimately has zero PARTICIPATES edges (the common case for a newly created story without PCs), this WHERE turns the optional match into an inner match and the entire query returns nothing, so neo4j_get_story returns None and neo4j_list_stories (which uses the same pattern below) will omit the story altogether. The label filter needs to be part of the pattern or allow pc IS NULL so stories without participants are still returned.
Useful? React with 👍 / 👎.
DL-4: Manage Stories, Scenes, Turns ✅
Summary
Successfully implemented complete CRUD operations for Stories (Neo4j), Scenes (MongoDB), and Turns (MongoDB) with comprehensive test coverage.
Recent Changes (Code Review Fixes - Part 2)
Implementation Checklist
Test Results
All Code Review Comments Addressed
✅ Fixed unused imports (Dict, Any, List, timezone)
✅ Removed unused class attributes in MongoDBClient
✅ Added index creation flag to prevent redundant operations
✅ Fixed authority comments in mongodb_tools.py
✅ Removed unused variable 's' in neo4j_update_story
✅ Extracted helper functions for turn/scene conversion
✅ Added TurnCreate validation test
✅ Added story status transition validation
✅ Added tests for invalid entity validation
✅ Added tests for invalid location validation
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.