DL-6: Comprehensive Story Outlines & Plot Threads#96
Conversation
…implementation Implements full narrative engine support for MONITOR with beat progression tracking, mystery mechanics, foreshadowing/payoff, and pacing metrics. ## Story Outlines (MongoDB) New schemas (story_outlines.py): - StoryBeat: Beat progression with status tracking, scene completion mapping - MysteryStructure: Clue discovery mechanics with visibility states - BranchingPoint: Multi-path narrative support - PacingMetrics: Automatic tension/completion calculation - StoryOutlineCreate/Update/Response: Full CRUD with beat manipulation MongoDB tools (mongodb_tools.py): - mongodb_create_story_outline: Create outline with pacing calculation - mongodb_get_story_outline: Retrieve outline with all beats/mystery - mongodb_update_story_outline: Partial updates with beat operations (add, remove, update, reorder beats; mark clues discovered) ## Plot Threads (Neo4j) New schemas (story_outlines.py): - PlotThreadCreate/Update/Response: Thread with relationships - ThreadDeadline: Time pressure mechanics - PlotThreadFilter/ListResponse: Flexible querying Neo4j tools (neo4j_tools.py): - neo4j_create_plot_thread: Create thread + 5 relationship types (HAS_THREAD, ADVANCED_BY, INVOLVES, FORESHADOWS, REVEALS) - neo4j_get_plot_thread: Retrieve with all relationships - neo4j_update_plot_thread: Update with status validation - neo4j_list_plot_threads: Filter by story, type, status, entity ## Base Schemas (base.py) Added 9 enums for narrative tracking: - PlotThreadType, PlotThreadStatus, BeatStatus - StoryStructureType, ArcTemplate - ThreadPriority, ThreadUrgency - ClueVisibility, PayoffStatus ## Tests Added 34 comprehensive tests (test_story_outline_tools.py, test_plot_thread_tools.py): - Story outline CRUD (13 tests): create, get, update with beat manipulation - Plot thread CRUD (21 tests): create, get, update, list with filtering - Status transitions, relationship management, error handling - All 194 tests passing ## Fixes - Fixed WHERE clause placement in neo4j_list_plot_threads for correct filtering ## Architecture Decisions - Beats in MongoDB (flexible planning) vs Threads in Neo4j (canonical truth) - Additive-only relationships to preserve narrative history - Status state machines with validated transitions - Pacing metrics computed from beat completion ratios Implements: DL-6, supports P-1, P-8, ST-1, ST-4, CF-3 🤖 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: f3bf46e802
ℹ️ 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".
| for clue_list_name in ["core_clues", "bonus_clues", "red_herrings"]: | ||
| for clue in mystery.get(clue_list_name, []): | ||
| if str(clue.get("clue_id")) == clue_id_str: |
There was a problem hiding this comment.
Marking a clue as discovered assumes doc["mystery_structure"] is a mapping, but outlines are created with mystery_structure=None. When mark_clue_discovered is set on an outline that never had a mystery structure, mystery is None and mystery.get(...) raises an AttributeError before any update is written. This makes the update endpoint crash for the default case of outlines without mysteries rather than returning a controlled error or ignoring the request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR implements DL-6: Comprehensive Story Outlines & Plot Threads, adding a sophisticated narrative engine to track story progression, plot threads, mystery mechanics, and pacing metrics across MongoDB and Neo4j.
Key Changes
- Story Outlines in MongoDB: Flexible planning system with beat progression, mystery structure support, branching narratives, and automatic pacing calculation
- Plot Threads in Neo4j: Canonical thread tracking with 5 relationship types (HAS_THREAD, ADVANCED_BY, INVOLVES, FORESHADOWS, REVEALS) supporting foreshadowing/payoff patterns
- New Enums: 9 narrative-focused enums (PlotThreadType, PlotThreadStatus, BeatStatus, etc.) in base.py for type-safe state management
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
packages/data-layer/src/monitor_data/schemas/base.py |
Added 9 new enums for narrative tracking (PlotThreadType, BeatStatus, ArcTemplate, ThreadPriority, ThreadUrgency, ClueVisibility, PayoffStatus, etc.) |
packages/data-layer/src/monitor_data/schemas/story_outlines.py |
New 374-line schema file defining StoryBeat, MysteryStructure, PacingMetrics, PlotThread with full CRUD schemas |
packages/data-layer/src/monitor_data/tools/mongodb_tools.py |
Added 353 lines with story outline CRUD operations, beat manipulation (add/remove/update/reorder), mystery clue tracking, and automatic pacing calculation |
packages/data-layer/src/monitor_data/tools/neo4j_tools.py |
Added 526 lines with plot thread CRUD operations, 5 relationship types, status transition validation, and flexible filtering/pagination |
packages/data-layer/tests/test_tools/test_story_outline_tools.py |
New 595-line test file with 13 comprehensive tests for story outline operations |
packages/data-layer/tests/test_tools/test_plot_thread_tools.py |
New 886-line test file with 21 comprehensive tests for plot thread operations including relationship management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if params.reorder_beats: | ||
| beats_by_id = {str(b.beat_id): b for b in current_beats} | ||
| reordered: list[StoryBeat] = [] | ||
| for beat_id in params.reorder_beats: | ||
| beat_id_str = str(beat_id) | ||
| if beat_id_str in beats_by_id: | ||
| beat = beats_by_id[beat_id_str] | ||
| beat.order = len(reordered) | ||
| reordered.append(beat) | ||
| current_beats = reordered |
There was a problem hiding this comment.
The reorder beats logic silently drops beats that aren't included in the reorder_beats list. If a user provides an incomplete list of beat IDs, any beats not in that list will be lost without warning. This could lead to accidental data loss.
Consider adding validation to ensure that reorder_beats contains all existing beat IDs, or document this behavior clearly. For example:
if params.reorder_beats:
beats_by_id = {str(b.beat_id): b for b in current_beats}
if len(params.reorder_beats) != len(beats_by_id):
raise ValueError(
f"reorder_beats must include all {len(beats_by_id)} beat IDs. "
f"Got {len(params.reorder_beats)} IDs instead."
)
# ... rest of logic| beats_by_id = {str(b.beat_id): b for b in current_beats} | ||
| for updated_beat in params.update_beats: | ||
| beat_id_str = str(updated_beat.beat_id) | ||
| if beat_id_str in beats_by_id: | ||
| beats_by_id[beat_id_str] = updated_beat | ||
| current_beats = list(beats_by_id.values()) |
There was a problem hiding this comment.
When updating beats, the logic replaces the entire beat with the updated beat but loses the original ordering. The dictionary beats_by_id doesn't preserve insertion order guarantees when converting back to a list on line 1047. This could cause beats to be reordered unintentionally.
To preserve the original order, iterate through current_beats and replace in place:
if params.update_beats:
update_map = {str(b.beat_id): b for b in params.update_beats}
for i, beat in enumerate(current_beats):
beat_id_str = str(beat.beat_id)
if beat_id_str in update_map:
current_beats[i] = update_map[beat_id_str]This ensures beats maintain their order unless explicitly reordered.
| beats_by_id = {str(b.beat_id): b for b in current_beats} | |
| for updated_beat in params.update_beats: | |
| beat_id_str = str(updated_beat.beat_id) | |
| if beat_id_str in beats_by_id: | |
| beats_by_id[beat_id_str] = updated_beat | |
| current_beats = list(beats_by_id.values()) | |
| update_map = {str(b.beat_id): b for b in params.update_beats} | |
| for i, beat in enumerate(current_beats): | |
| beat_id_str = str(beat.beat_id) | |
| if beat_id_str in update_map: | |
| current_beats[i] = update_map[beat_id_str] |
| default_factory=list, | ||
| description="PlotThreads that must be active for this beat to trigger", | ||
| ) | ||
| created_at: datetime = Field(default_factory=datetime.utcnow) |
There was a problem hiding this comment.
datetime.utcnow() is deprecated as of Python 3.12 and will be removed in a future version. The code should use datetime.now(timezone.utc) instead for timezone-aware datetime objects. This is particularly important since other parts of the codebase (like mongodb_tools.py line 895) already use the recommended approach.
You'll need to add the timezone import and update the default factory:
from datetime import datetime, timezone
created_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc))| OPTIONAL MATCH (t)-[:INVOLVES]->(e) | ||
| WHERE e:EntityArchetype OR e:EntityInstance |
There was a problem hiding this comment.
The WHERE clause on line 2813 will conflict with the WHERE clause from line 2810 in the generated Cypher query. In Cypher, you cannot have multiple WHERE clauses at the same level without combining them. This WHERE clause should be part of the OPTIONAL MATCH pattern using AND, or the entity filtering should be moved into the OPTIONAL MATCH clause itself.
Consider changing line 2812-2813 to:
OPTIONAL MATCH (t)-[:INVOLVES]->(e)
WHERE (e:EntityArchetype OR e:EntityInstance)
Or better yet, use the pattern directly in the MATCH:
OPTIONAL MATCH (t)-[:INVOLVES]->(e:EntityArchetype|EntityInstance)
| OPTIONAL MATCH (t)-[:INVOLVES]->(e) | |
| WHERE e:EntityArchetype OR e:EntityInstance | |
| OPTIONAL MATCH (t)-[:INVOLVES]->(e:EntityArchetype|EntityInstance) |
| OPTIONAL MATCH (t)-[:INVOLVES]->(e) | ||
| WHERE e:EntityArchetype OR e:EntityInstance |
There was a problem hiding this comment.
Similar to the issue in neo4j_list_plot_threads, this WHERE clause on line 2520 is placed incorrectly in the Cypher query. It appears after an OPTIONAL MATCH without being part of that clause's pattern, and before other OPTIONAL MATCH statements. This will cause Cypher syntax errors.
The WHERE clause should be part of the OPTIONAL MATCH pattern. Consider using:
OPTIONAL MATCH (t)-[:INVOLVES]->(e:EntityArchetype|EntityInstance)
Or if the union syntax isn't supported, wrap it properly:
OPTIONAL MATCH (t)-[:INVOLVES]->(e)
WHERE (e:EntityArchetype OR e:EntityInstance)
However, note that this WHERE applies only to the immediately preceding OPTIONAL MATCH, so the parentheses around the OR condition may help clarify intent.
| le=1.0, | ||
| description="Story completion percentage (completed_beats / total_beats)", | ||
| ) | ||
| last_updated: datetime = Field(default_factory=datetime.utcnow) |
There was a problem hiding this comment.
datetime.utcnow() is deprecated as of Python 3.12 and will be removed in a future version. The code should use datetime.now(timezone.utc) instead for timezone-aware datetime objects. This is particularly important since other parts of the codebase (like mongodb_tools.py line 895) already use the recommended approach.
You'll need to add the timezone import and update the default factory:
from datetime import datetime, timezone
last_updated: datetime = Field(default_factory=lambda: datetime.now(timezone.utc))|
|
||
| beat_id: UUID = Field(default_factory=uuid4) | ||
| title: str = Field(min_length=1, max_length=200) | ||
| description: str = Field(max_length=2000) |
There was a problem hiding this comment.
The description field is required but doesn't have a default value. This could make it cumbersome to create beats where a detailed description isn't immediately available. Consider either making it optional or providing a default empty string for flexibility:
description: str = Field(default="", max_length=2000)This is especially important since beats may be created during planning phases when full details aren't yet determined.
| description: str = Field(max_length=2000) | |
| description: str = Field(default="", max_length=2000) |
| REVEALED = "revealed" | ||
| HIDDEN = "hidden" | ||
| DISCOVERED = "discovered" |
There was a problem hiding this comment.
The ordering of enum values in ClueVisibility doesn't follow a logical progression. The typical visibility progression for a mystery clue would be: HIDDEN → DISCOVERED → REVEALED. However, the current ordering is REVEALED, HIDDEN, DISCOVERED. Consider reordering to reflect the natural progression:
class ClueVisibility(str, Enum):
"""Visibility status for mystery clues."""
HIDDEN = "hidden"
DISCOVERED = "discovered"
REVEALED = "revealed"This makes the enum values progress from least to most visible, which is more intuitive.
| REVEALED = "revealed" | |
| HIDDEN = "hidden" | |
| DISCOVERED = "discovered" | |
| HIDDEN = "hidden" | |
| DISCOVERED = "discovered" | |
| REVEALED = "revealed" |
| # Handle beat operations | ||
| current_beats = [StoryBeat(**b) for b in doc.get("beats", [])] | ||
|
|
||
| # Update existing beats | ||
| if params.update_beats: | ||
| beats_by_id = {str(b.beat_id): b for b in current_beats} | ||
| for updated_beat in params.update_beats: | ||
| beat_id_str = str(updated_beat.beat_id) | ||
| if beat_id_str in beats_by_id: | ||
| beats_by_id[beat_id_str] = updated_beat | ||
| current_beats = list(beats_by_id.values()) | ||
|
|
||
| # Remove beats | ||
| if params.remove_beat_ids: | ||
| remove_ids = {str(bid) for bid in params.remove_beat_ids} | ||
| current_beats = [b for b in current_beats if str(b.beat_id) not in remove_ids] | ||
|
|
||
| # Add beats | ||
| if params.add_beats: | ||
| current_beats.extend(params.add_beats) | ||
|
|
||
| # Reorder beats | ||
| if params.reorder_beats: | ||
| beats_by_id = {str(b.beat_id): b for b in current_beats} | ||
| reordered: list[StoryBeat] = [] | ||
| for beat_id in params.reorder_beats: | ||
| beat_id_str = str(beat_id) | ||
| if beat_id_str in beats_by_id: | ||
| beat = beats_by_id[beat_id_str] | ||
| beat.order = len(reordered) | ||
| reordered.append(beat) | ||
| current_beats = reordered |
There was a problem hiding this comment.
The order of beat operations could lead to unexpected results. The current order is: update_beats → remove_beats → add_beats → reorder_beats. This means:
- If you update a beat and then try to reorder, the beat will be in the reordered list
- If you add new beats and then reorder, the new beats might not be included in the reorder operation
- If you remove a beat that's in the reorder list, it will silently fail to appear in the reordered result
Consider documenting this operation order clearly, or validating that operations don't conflict. For example, if reorder_beats is provided, it might be clearer to require that no other beat operations are included in the same update, or ensure that reorder_beats is only applied to the final beat list after all other operations.
Addressed all 9 review comments from Copilot code review: ## Critical Fixes 1. **P1: Guard clue discovery** - Added None check for mystery_structure to prevent AttributeError when marking clues discovered on outlines without mystery structures 2. **Validate reorder_beats** - Added validation to ensure reorder_beats includes all beat IDs to prevent silent data loss 3. **Preserve beat order** - Fixed update_beats to preserve original order by iterating and replacing in place instead of using dictionary 4. **Fix deprecated datetime** - Replaced datetime.utcnow() with datetime.now(timezone.utc) in story_outlines.py for Python 3.12+ compatibility ## Code Improvements 5. **Clarify WHERE clause** - Added parentheses around OR condition in neo4j_get_plot_thread for clarity 6. **Make description optional** - Added default empty string to StoryBeat.description for flexibility during planning 7. **Reorder enum values** - Reordered ClueVisibility enum to follow logical progression: HIDDEN → DISCOVERED → REVEALED 8. **Document beat operations** - Added comprehensive documentation of beat operation order and potential conflicts in docstring All 194 tests passing ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary
Implements DL-6: Manage Story Outlines & Plot Threads with comprehensive narrative engine support including beat progression tracking, mystery mechanics, foreshadowing/payoff patterns, and automatic pacing metrics.
Story Outlines (MongoDB)
✨ New file:
src/monitor_data/schemas/story_outlines.py(377 lines)Schemas
MongoDB Tools (
mongodb_tools.py+340 lines)mongodb_create_story_outline: Create outline with automatic pacing calculationmongodb_get_story_outline: Retrieve outline with all beats/mystery structuremongodb_update_story_outline: Partial updates supporting:Plot Threads (Neo4j)
Schemas
Neo4j Tools (
neo4j_tools.py+526 lines)neo4j_create_plot_thread: Creates thread node + 5 relationship types:HAS_THREAD(Story → PlotThread)ADVANCED_BY(PlotThread → Scene)INVOLVES(PlotThread → Entity)FORESHADOWS(Event → PlotThread)REVEALS(Event → PlotThread)neo4j_get_plot_thread: Retrieve with all relationship collectionsneo4j_update_plot_thread: Update with status transition validationneo4j_list_plot_threads: Filter by story_id, type, status, priority, entity_idBase Schemas (
base.py+83 lines)Added 9 new enums for narrative tracking:
PlotThreadType,PlotThreadStatus,BeatStatusStoryStructureType,ArcTemplateThreadPriority,ThreadUrgencyClueVisibility,PayoffStatusTests
✨ New files:
tests/test_tools/test_story_outline_tools.py(590 lines, 13 tests)tests/test_tools/test_plot_thread_tools.py(840 lines, 21 tests)Coverage
All 194 tests passing ✅
Bug Fixes
neo4j_list_plot_threads()- moved filter clause before OPTIONAL MATCH statements for correct Cypher query executionArchitecture Highlights
Design Decisions
Narrative Engine Features
Use Cases Implemented
Testing Checklist
🤖 Generated with Claude Code