Skip to content

feat(data-layer): Implement DL-1 Manage Multiverse/Universes#47

Merged
spuentesp merged 16 commits into
masterfrom
dl-1-clean
Dec 28, 2025
Merged

feat(data-layer): Implement DL-1 Manage Multiverse/Universes#47
spuentesp merged 16 commits into
masterfrom
dl-1-clean

Conversation

@spuentesp
Copy link
Copy Markdown
Owner

Foundational data layer infrastructure for managing Neo4j graph hierarchy (Omniverse → Multiverse → Universe).

Changes

  • Add Universe/Multiverse Pydantic schemas with validation
  • Implement Neo4j client with secure credential management (env vars only)
  • Add CRUD tools: create, get, list, update, delete universe
  • Add authority middleware (CanonKeeper for writes)
  • Add 45 unit tests with 84% coverage

Security

  • ✅ No hardcoded credentials - requires NEO4J_PASSWORD env var
  • ✅ Clean git history (no secrets)

Fixes #31

Copilot AI review requested due to automatic review settings December 28, 2025 03:08
@gitguardian
Copy link
Copy Markdown

gitguardian Bot commented Dec 28, 2025

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
23833754 Triggered Generic Password 7d6ccc8 packages/data-layer/src/monitor_data/db/neo4j.py View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 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".

Comment thread packages/data-layer/src/monitor_data/tools/neo4j_tools.py
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements foundational infrastructure for managing the Neo4j graph hierarchy (Omniverse → Multiverse → Universe) in the MONITOR data layer. It establishes secure patterns for database operations with proper authority enforcement and comprehensive test coverage.

Key Changes:

  • Implements complete CRUD operations for Universe and Multiverse nodes with Neo4j
  • Adds authority middleware enforcing role-based access control (CanonKeeper for writes)
  • Establishes secure credential management requiring environment variables only

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packages/data-layer/src/monitor_data/tools/neo4j_tools.py Implements CRUD operations for Universe/Multiverse with Neo4j Cypher queries
packages/data-layer/src/monitor_data/tools/__init__.py Exports neo4j tools for external consumption
packages/data-layer/src/monitor_data/schemas/universe.py Defines Pydantic schemas for Universe/Multiverse create/update/response/filter operations
packages/data-layer/src/monitor_data/schemas/base.py Establishes base enums (CanonLevel, Authority, etc.) and metadata models
packages/data-layer/src/monitor_data/schemas/__init__.py Exports base and universe schemas
packages/data-layer/src/monitor_data/middleware/auth.py Implements authority matrix and enforcement functions for role-based access control
packages/data-layer/src/monitor_data/middleware/__init__.py Exports auth middleware components
packages/data-layer/src/monitor_data/db/neo4j.py Neo4j client wrapper with transaction management and credential validation
packages/data-layer/src/monitor_data/db/__init__.py Exports Neo4j client and factory function
packages/data-layer/src/monitor_data/server.py Removes unused import, updates placeholder comments
packages/data-layer/tests/test_tools/test_universe_tools.py Comprehensive unit tests for universe CRUD operations (45 tests)
packages/data-layer/tests/test_middleware/test_auth.py Unit tests for authority enforcement middleware
packages/data-layer/tests/test_db/test_neo4j.py Unit tests for Neo4j client initialization and credential handling
packages/data-layer/tests/conftest.py Test fixtures for mock clients and sample data
.gitignore Adds test coverage artifacts to ignore list

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/data-layer/src/monitor_data/tools/neo4j_tools.py
Comment thread packages/data-layer/src/monitor_data/tools/neo4j_tools.py Outdated
Comment on lines +398 to +402
update_query = f"""
MATCH (u:Universe {{id: $id}})
SET {set_clause}
RETURN u
"""
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

Similar to the list query, this dynamic query construction using f-strings for the SET clause poses a security risk. While field names are controlled by the code, this pattern could be vulnerable if extended carelessly in the future.

Consider using a safer query builder pattern or add explicit validation that set_clauses only contains expected field assignments.

Copilot uses AI. Check for mistakes.
Comment on lines +92 to +94
genre: Optional[str] = Field(None, max_length=100)
tone: Optional[str] = Field(None, max_length=100)
tech_level: Optional[str] = Field(None, max_length=100)
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The optional string fields (genre, tone, tech_level) in UniverseUpdate lack minimum length validation when provided. If someone passes an empty string, it will bypass the None check but may not be the intended behavior.

Consider adding min_length=1 to these fields to match the validation in UniverseCreate, ensuring consistency and preventing empty string updates.

Suggested change
genre: Optional[str] = Field(None, max_length=100)
tone: Optional[str] = Field(None, max_length=100)
tech_level: Optional[str] = Field(None, max_length=100)
genre: Optional[str] = Field(None, min_length=1, max_length=100)
tone: Optional[str] = Field(None, min_length=1, max_length=100)
tech_level: Optional[str] = Field(None, min_length=1, max_length=100)

Copilot uses AI. Check for mistakes.
Comment on lines +148 to +162
def get_neo4j_client() -> Neo4jClient:
"""
Get or create the global Neo4j client instance.

Returns:
Neo4jClient instance

Note:
This returns a singleton client. Call connect() before using.
"""
global _client
if _client is None:
_client = Neo4jClient()
_client.connect()
return _client
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The singleton pattern in get_neo4j_client() lacks thread safety. In a concurrent environment, multiple threads could simultaneously check if _client is None and create multiple client instances, potentially causing connection pool issues or race conditions.

Consider adding thread-safe initialization using threading.Lock or using a thread-safe singleton pattern.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Comment thread packages/data-layer/src/monitor_data/schemas/base.py Outdated
spuentesp and others added 2 commits December 28, 2025 00:13
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI commented Dec 28, 2025

@spuentesp I've opened a new pull request, #48, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Copy Markdown

Copilot AI commented Dec 28, 2025

@spuentesp I've opened a new pull request, #49, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@spuentesp
Copy link
Copy Markdown
Owner Author

@copilot open a new pull request to apply changes based on the comments in this thread

Copy link
Copy Markdown

Copilot AI commented Dec 28, 2025

@spuentesp I've opened a new pull request, #50, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +50 to +87
# Verify omniverse exists
verify_query = """
MATCH (o:Omniverse {id: $omniverse_id})
RETURN o.id as id
"""
result = client.execute_read(
verify_query, {"omniverse_id": str(params.omniverse_id)}
)
if not result:
raise ValueError(f"Omniverse {params.omniverse_id} not found")

# Create multiverse
multiverse_id = uuid4()
create_query = """
MATCH (o:Omniverse {id: $omniverse_id})
CREATE (m:Multiverse {
id: $id,
omniverse_id: $omniverse_id,
name: $name,
system_name: $system_name,
description: $description,
created_at: datetime($created_at)
})
CREATE (o)-[:CONTAINS]->(m)
RETURN m
"""
created_at = datetime.now(timezone.utc)
client.execute_write(
create_query,
{
"id": str(multiverse_id),
"omniverse_id": str(params.omniverse_id),
"name": params.name,
"system_name": params.system_name,
"description": params.description,
"created_at": created_at.isoformat(),
},
)
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

There's a potential race condition between the verify query (line 55-57) and the create query (line 77). If the omniverse is deleted between these two operations, the CREATE query will fail. Consider combining these into a single transaction or handling the potential error case where the parent node no longer exists.

Copilot uses AI. Check for mistakes.
Comment on lines +157 to +204
# Verify multiverse exists
verify_query = """
MATCH (m:Multiverse {id: $multiverse_id})
RETURN m.id as id
"""
result = client.execute_read(
verify_query, {"multiverse_id": str(params.multiverse_id)}
)
if not result:
raise ValueError(f"Multiverse {params.multiverse_id} not found")

# Create universe
universe_id = uuid4()
create_query = """
MATCH (m:Multiverse {id: $multiverse_id})
CREATE (u:Universe {
id: $id,
multiverse_id: $multiverse_id,
name: $name,
description: $description,
genre: $genre,
tone: $tone,
tech_level: $tech_level,
canon_level: $canon_level,
confidence: $confidence,
authority: $authority,
created_at: datetime($created_at)
})
CREATE (m)-[:CONTAINS]->(u)
RETURN u
"""
created_at = datetime.now(timezone.utc)
client.execute_write(
create_query,
{
"id": str(universe_id),
"multiverse_id": str(params.multiverse_id),
"name": params.name,
"description": params.description,
"genre": params.genre,
"tone": params.tone,
"tech_level": params.tech_level,
"canon_level": params.canon_level.value,
"confidence": params.confidence,
"authority": params.authority.value,
"created_at": created_at.isoformat(),
},
)
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

Similar race condition exists here between the multiverse existence check (line 162-164) and the universe creation (line 189-204). If the multiverse is deleted between these operations, the CREATE will fail. Consider using a single transaction or handling the error gracefully.

Copilot uses AI. Check for mistakes.
Comment on lines +474 to +481

if deps["sources"] > 0 or deps["axioms"] > 0 or deps["entities"] > 0:
raise ValueError(
f"Universe {universe_id} has dependent data: "
f"{deps['sources']} sources, {deps['axioms']} axioms, {deps['entities']} entities. "
f"Use force=True to cascade delete."
)

Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The cascade delete query using OPTIONAL MATCH path = (u)-[*0..]->(dependent) is potentially dangerous as it can traverse the entire graph without limits. This could inadvertently delete unrelated nodes if there are unexpected relationships. Consider adding a limit to the relationship depth or being more explicit about which relationship types to traverse for deletion.

Copilot uses AI. Check for mistakes.
"id": str(uuid4()),
"name": "Test Omniverse",
"description": "Test omniverse for unit tests",
"created_at": "2024-01-01T00:00:00",
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The test data uses a hardcoded date "2024-01-01T00:00:00" which is in the past. While this is acceptable for tests, consider using a date that won't appear outdated as time passes, or make it clear this is intentionally a static test date.

Copilot uses AI. Check for mistakes.
Copilot AI and others added 5 commits December 28, 2025 03:18
- Replace unbounded variable-length path (u)-[*0..]->(dependent) with explicit relationship types
- Add specific relationship traversal: HAS_SOURCE, HAS_AXIOM, HAS_STORY, HAS_SCENE, HAS_THREAD, IN_UNIVERSE
- Limit depth to 2 levels (Universe -> Story -> Scene/Thread)
- Add node type validation before deletion to prevent accidental data loss
- Only delete expected node types: Source, Axiom, Story, Scene, PlotThread, EntityArchetype, EntityInstance

Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
- Add WHERE x IS NOT NULL filters to all collection comprehensions
- Prevents null values from OPTIONAL MATCH from being included in dependents list
- Addresses code review feedback

Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
Fix unbounded cascade delete in neo4j_delete_universe
Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
fix(data-layer): Add thread-safe singleton pattern to Neo4j client
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +504 to +512
// Flatten into single list
WITH u, sources + axioms + stories + scenes + threads + entities AS dependents
UNWIND dependents AS dependent
// Final safety check: only delete expected node types
WITH u, dependent
WHERE dependent:Source OR dependent:Axiom OR dependent:Story OR
dependent:Scene OR dependent:PlotThread OR
dependent:EntityArchetype OR dependent:EntityInstance
WITH collect(DISTINCT dependent) + u AS nodes
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The cascade deletion query has a potential issue when the universe has no dependents. When dependents is an empty list, the UNWIND dependents AS dependent operation will produce no rows, which means the universe node u won't be included in the final deletion. This could result in a count of 0 being returned even though the universe should be deleted.

Consider modifying the query to handle the empty dependents case, or add the universe to the nodes list unconditionally before unwinding.

Suggested change
// Flatten into single list
WITH u, sources + axioms + stories + scenes + threads + entities AS dependents
UNWIND dependents AS dependent
// Final safety check: only delete expected node types
WITH u, dependent
WHERE dependent:Source OR dependent:Axiom OR dependent:Story OR
dependent:Scene OR dependent:PlotThread OR
dependent:EntityArchetype OR dependent:EntityInstance
WITH collect(DISTINCT dependent) + u AS nodes
// Flatten into single list and filter to expected node types, always include universe
WITH [x IN sources + axioms + stories + scenes + threads + entities
WHERE x:Source OR x:Axiom OR x:Story OR
x:Scene OR x:PlotThread OR
x:EntityArchetype OR x:EntityInstance] + [u] AS nodes

Copilot uses AI. Check for mistakes.
Comment on lines +344 to +365
@patch("monitor_data.tools.neo4j_tools.get_neo4j_client")
def test_delete_universe_with_force(
mock_get_client: Mock,
mock_neo4j_client: Mock,
universe_data: Dict[str, Any],
):
"""Test universe deletion with force=True cascades."""
mock_get_client.return_value = mock_neo4j_client

# Mock universe exists
mock_neo4j_client.execute_read.return_value = [{"u": universe_data}]

# Mock cascade deletion
mock_neo4j_client.execute_write.return_value = [{"deleted_count": 10}]

universe_id = UUID(universe_data["id"])
result = neo4j_delete_universe(universe_id, force=True)

assert result["deleted"] is True
assert result["force"] is True
assert result["deleted_count"] == 10

Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

Missing test coverage for the edge case where force=True is used but the universe has no dependent data. The cascade delete query in this scenario may not work correctly due to the UNWIND operation on an empty list. Consider adding a test case that verifies force deletion works correctly when there are no dependencies.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +61
"""
Unit tests for Neo4j client.

Tests cover:
- Neo4jClient initialization
- Password requirement enforcement
- Connection management
"""

import os
import pytest

from monitor_data.db.neo4j import Neo4jClient


def test_neo4j_client_requires_password():
"""Test that Neo4jClient raises error when password is not provided."""
# Clear password env var if it exists
original_password = os.environ.get("NEO4J_PASSWORD")
if "NEO4J_PASSWORD" in os.environ:
del os.environ["NEO4J_PASSWORD"]

try:
with pytest.raises(ValueError, match="Neo4j password is required"):
Neo4jClient()
finally:
# Restore original password
if original_password:
os.environ["NEO4J_PASSWORD"] = original_password


def test_neo4j_client_accepts_explicit_password():
"""Test that Neo4jClient works with explicit password parameter."""
# Clear password env var
original_password = os.environ.get("NEO4J_PASSWORD")
if "NEO4J_PASSWORD" in os.environ:
del os.environ["NEO4J_PASSWORD"]

try:
client = Neo4jClient(password="explicit_password")
assert client.password == "explicit_password"
finally:
# Restore original password
if original_password:
os.environ["NEO4J_PASSWORD"] = original_password


def test_neo4j_client_uses_env_password():
"""Test that Neo4jClient uses password from environment variable."""
os.environ["NEO4J_PASSWORD"] = "env_password"

client = Neo4jClient()
assert client.password == "env_password"


def test_neo4j_client_explicit_overrides_env():
"""Test that explicit password parameter overrides environment variable."""
os.environ["NEO4J_PASSWORD"] = "env_password"

client = Neo4jClient(password="explicit_password")
assert client.password == "explicit_password"
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The Neo4j client tests do not cover the scenario where execute_read or execute_write are called without connecting first. While the implementation raises a RuntimeError in this case, it would be valuable to add test coverage for this error handling to ensure the error message is clear and the behavior is as expected.

Copilot uses AI. Check for mistakes.
min_length=1, max_length=200, description="e.g., 'D&D 5e', 'Marvel 616'"
)
description: str = Field(min_length=1, max_length=2000)
authority: Authority = Field(default=Authority.SYSTEM)
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The authority field is defined in MultiverseCreate schema but is not stored in the database or returned in MultiverseResponse. If Multiverse nodes are not meant to track authority metadata (since they're structural rather than canonical), this field should be removed from MultiverseCreate. If authority should be tracked, it needs to be added to the creation query and response schema.

Suggested change
authority: Authority = Field(default=Authority.SYSTEM)

Copilot uses AI. Check for mistakes.
from enum import Enum
from uuid import UUID

from pydantic import BaseModel, Field, field_validator
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

Import of 'field_validator' is not used.

Suggested change
from pydantic import BaseModel, Field, field_validator
from pydantic import BaseModel, Field

Copilot uses AI. Check for mistakes.
@spuentesp
Copy link
Copy Markdown
Owner Author

@copilot apply changes based on the comments in this thread

Copy link
Copy Markdown

Copilot AI commented Dec 28, 2025

@spuentesp I've opened a new pull request, #51, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 2 commits December 28, 2025 03:45
…e, add tests

Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
fix: Address cascade delete bug, add missing test coverage, remove unused code
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +468 to +478
RETURN count(DISTINCT s) AS sources,
count(DISTINCT a) AS axioms,
count(DISTINCT e) AS entities
"""
dep_result = client.execute_read(dependency_query, {"id": str(universe_id)})
deps = dep_result[0]

if deps["sources"] > 0 or deps["axioms"] > 0 or deps["entities"] > 0:
raise ValueError(
f"Universe {universe_id} has dependent data: "
f"{deps['sources']} sources, {deps['axioms']} axioms, {deps['entities']} entities. "
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The dependency check query (lines 462-470) does not include Stories in its dependency count, but the force cascade delete query (lines 484-512) does delete Stories and their related nodes (Scenes, PlotThreads). This creates an inconsistency where:

  1. A Universe with Stories but no Sources/Axioms/Entities would pass the dependency check (force=False)
  2. Yet those Stories would be silently deleted during a force=True cascade delete

The dependency check should also count Stories to provide accurate information about what will be deleted. Add a count for Stories to match the cascade delete behavior.

Suggested change
RETURN count(DISTINCT s) AS sources,
count(DISTINCT a) AS axioms,
count(DISTINCT e) AS entities
"""
dep_result = client.execute_read(dependency_query, {"id": str(universe_id)})
deps = dep_result[0]
if deps["sources"] > 0 or deps["axioms"] > 0 or deps["entities"] > 0:
raise ValueError(
f"Universe {universe_id} has dependent data: "
f"{deps['sources']} sources, {deps['axioms']} axioms, {deps['entities']} entities. "
OPTIONAL MATCH (u)-[:HAS_STORY]->(st:Story)
RETURN count(DISTINCT s) AS sources,
count(DISTINCT a) AS axioms,
count(DISTINCT e) AS entities,
count(DISTINCT st) AS stories
"""
dep_result = client.execute_read(dependency_query, {"id": str(universe_id)})
deps = dep_result[0]
if (
deps["sources"] > 0
or deps["axioms"] > 0
or deps["entities"] > 0
or deps["stories"] > 0
):
raise ValueError(
f"Universe {universe_id} has dependent data: "
f"{deps['sources']} sources, {deps['axioms']} axioms, "
f"{deps['entities']} entities, {deps['stories']} stories. "

Copilot uses AI. Check for mistakes.
Comment on lines +408 to +412
update_query = f"""
MATCH (u:Universe {{id: $id}})
SET {set_clause}
RETURN u
"""
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The update query uses an f-string to interpolate the SET clause (line 408), which is constructed from user-controlled field values. While the current implementation carefully validates each field and uses parameterized queries for the values, this pattern is inconsistent with the explicit avoidance of f-strings in neo4j_list_universes (lines 305-319) where a similar pattern is used but with explicit string joining.

For consistency and to maintain the security posture documented in the comments (lines 300-302), consider using the same explicit string joining approach here instead of f-strings.

Suggested change
update_query = f"""
MATCH (u:Universe {{id: $id}})
SET {set_clause}
RETURN u
"""
update_query = (
"MATCH (u:Universe {id: $id})\n"
"SET " + set_clause + "\n"
"RETURN u"
)

Copilot uses AI. Check for mistakes.
Comment on lines +490 to +492
// Collect story dependencies (1 level deep from Story)
OPTIONAL MATCH (story)-[:HAS_SCENE]->(scene:Scene)
OPTIONAL MATCH (story)-[:HAS_THREAD]->(thread:PlotThread)
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The cascade delete query has a potential issue with chained OPTIONAL MATCH statements. On line 491-492, the OPTIONAL MATCH for scene and thread depends on the story variable from line 489's OPTIONAL MATCH. If no stories exist, story will be NULL, and these subsequent OPTIONAL MATCH statements may not behave as expected.

Consider restructuring the query to handle this more explicitly, or add a WHERE clause to check if story is not NULL before matching its relationships, to ensure the query behavior is clear and predictable.

Suggested change
// Collect story dependencies (1 level deep from Story)
OPTIONAL MATCH (story)-[:HAS_SCENE]->(scene:Scene)
OPTIONAL MATCH (story)-[:HAS_THREAD]->(thread:PlotThread)
// Collect story dependencies (1 level deep from Story), matched from universe
OPTIONAL MATCH (u)-[:HAS_STORY]->(:Story)-[:HAS_SCENE]->(scene:Scene)
OPTIONAL MATCH (u)-[:HAS_STORY]->(:Story)-[:HAS_THREAD]->(thread:PlotThread)

Copilot uses AI. Check for mistakes.
@spuentesp
Copy link
Copy Markdown
Owner Author

@copilot apply changes based on the comments in [this thread] to this branch (#47 (review))

Copy link
Copy Markdown

Copilot AI commented Dec 28, 2025

@spuentesp I've opened a new pull request, #52, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 2 commits December 28, 2025 03:58
…uery security, and fix chained OPTIONAL MATCH

Co-authored-by: spuentesp <112034353+spuentesp@users.noreply.github.com>
fix: Align dependency checks with cascade delete behavior
@spuentesp spuentesp self-assigned this Dec 28, 2025
@spuentesp spuentesp merged commit ff895c5 into master Dec 28, 2025
2 checks passed
spuentesp pushed a commit that referenced this pull request Jan 4, 2026
… audit

- DL-1: Multiverse/Universes (PR #47)
- DL-2: Archetypes & Instances (PR #53)
- DL-3: Facts & Events (PR #54)
- DL-4: Stories/Scenes/Turns (PR #66)

All merged and complete. Added DL_IMPLEMENTATION_AUDIT.md to track
implementation status across all data layer use cases.
@spuentesp spuentesp deleted the dl-1-clean branch April 25, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/data-layer Data layer changes type/tests Tests touched

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DL-1: Manage Multiverse/Universes

4 participants