feat(data-layer): DL-16 - Manage Party Inventory & Splits#107
Conversation
Implements MongoDB operations for shared party inventory and party split tracking, supporting common RPG patterns like shared gold, party loot, and split-party adventures. ## Party Inventory (MongoDB) New schemas (party_inventory.py): - ItemCategory enum: weapons, armor, consumables, treasure, quest_items, misc - InventoryItem: Item with quantity, category, value, notes - PartyInventoryCreate/Response: Full inventory with gold and items - AddInventoryItemRequest/RemoveInventoryItemRequest: Item management - UpdateGoldRequest: Gold tracking with reason field MongoDB tools (mongodb_tools.py): - mongodb_create_party_inventory: Create inventory with initial items/gold - mongodb_get_party_inventory: Retrieve full inventory - mongodb_add_inventory_item: Add item or increment quantity - mongodb_remove_inventory_item: Remove item or decrement quantity - mongodb_update_party_gold: Add/subtract gold with validation - mongodb_transfer_item: Validate transfers (character inventory placeholder) ## Party Splits (MongoDB) New schemas (party_inventory.py): - SplitStatus enum: active, resolved - SubParty: Name, member_ids, location_id, purpose - PartySplitCreate/Response: Split with sub-parties array - ResolvePartySplitRequest: Mark split as resolved - ActiveSplitsResponse/SplitHistoryResponse: Query splits MongoDB tools (mongodb_tools.py): - mongodb_create_party_split: Create split with 2+ sub-parties - mongodb_get_active_splits: Get all active splits for party - mongodb_resolve_party_split: Mark split as resolved - mongodb_get_split_history: Get all splits with pagination ## Authority Matrix (auth.py) Added 10 party inventory operations: - Create/add/remove/update: Orchestrator, CanonKeeper - Get/list operations: All agents (*) - Transfer: Orchestrator only ## Tests (test_party_inventory_tools.py) Added 26 comprehensive tests covering: - Inventory CRUD: create, get, initial items - Item management: add (new/existing), remove (partial/full), errors - Gold tracking: add, subtract, insufficient gold validation - Transfer validation: character inventory not implemented check - Split lifecycle: create, get active, resolve, history with pagination ## Fixes - Fixed quantity validation in mongodb_remove_inventory_item (check insufficient before removal) - Fixed Pydantic v2 deprecation warnings (min_items → min_length) - Fixed mypy type errors in mongodb_resolve_party_split (None check after find_one) ## Architecture Decisions - Dual collections: party_inventories (items/gold) and party_splits (temporal divisions) - Gold stored as integer (copper pieces) for precision - Incremental item management avoids full document rewrites - Transfer tool validates but defers character inventory to future implementation Implements: DL-16 Depends on: DL-15 (Parties), DL-2 (Entities) All 360 tests passing ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR implements DL-16: Manage Party Inventory & Splits, adding MongoDB operations for tracking shared party inventory (gold and items) and party split/rejoin events. The implementation includes 10 new database tools, comprehensive Pydantic schemas, authority matrix integration, and 26 unit tests.
Key Changes
- Party Inventory Management: CRUD operations for shared inventory with gold tracking (in copper pieces) and item management (add, remove, update quantities)
- Party Split Tracking: Create and resolve party splits with sub-party composition, location, and purpose tracking; query active splits and historical split data with pagination
- Transfer Validation: Placeholder for future character inventory integration with validation logic
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 |
|---|---|
src/monitor_data/schemas/party_inventory.py |
New schemas defining 4 enums, inventory items, CRUD requests/responses, and party split data contracts (248 lines) |
src/monitor_data/tools/mongodb_tools.py |
10 new MongoDB tools for inventory and split operations with Neo4j validation (589 lines added) |
tests/test_tools/test_party_inventory_tools.py |
Comprehensive test suite covering all 10 tools with success/error cases (1140 lines) |
src/monitor_data/schemas/__init__.py |
Export 12 new party inventory schemas for public API |
src/monitor_data/middleware/auth.py |
Authority matrix entries for 10 new tools (Orchestrator/CanonKeeper for writes, all agents for reads) |
mypy.ini |
New root-level mypy configuration excluding test directories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| now = datetime.now(timezone.utc) | ||
| split_id = uuid4() | ||
|
|
||
| sub_parties_list = [sub_party.model_dump() for sub_party in params.sub_parties] |
There was a problem hiding this comment.
The model_dump() call should use mode="json" to properly serialize UUIDs to strings for MongoDB storage. Without this, UUID fields in SubParty (member_ids and location_id) will be stored as UUID objects rather than strings, causing deserialization errors when reading the documents back. The codebase consistently uses model_dump(mode="json") for MongoDB storage (see lines 994, 998, 1323, etc.).
| sub_parties_list = [sub_party.model_dump() for sub_party in params.sub_parties] | |
| sub_parties_list = [ | |
| sub_party.model_dump(mode="json") for sub_party in params.sub_parties | |
| ] |
| notes=params.notes, | ||
| added_at=now, | ||
| ) | ||
| items.append(new_item.model_dump()) |
There was a problem hiding this comment.
The model_dump() call should use mode="json" to properly serialize the added_at datetime field for MongoDB storage. See the codebase pattern at lines 994, 1323, etc.
| items.append(new_item.model_dump()) | |
| items.append(new_item.model_dump(mode="json")) |
| # Get current inventory | ||
| inventory_doc = inventories_collection.find_one({"party_id": str(params.party_id)}) | ||
| if not inventory_doc: | ||
| raise ValueError(f"Inventory for party {params.party_id} not found") | ||
|
|
||
| now = datetime.now(timezone.utc) | ||
| items = inventory_doc.get("items", []) | ||
|
|
||
| # Check if item already exists | ||
| existing_item = None | ||
| for item in items: | ||
| if item["name"] == params.item_name: | ||
| existing_item = item | ||
| break | ||
|
|
||
| if existing_item: | ||
| # Increment quantity | ||
| existing_item["quantity"] += params.quantity | ||
| else: | ||
| # Add new item | ||
| new_item = InventoryItem( | ||
| name=params.item_name, | ||
| quantity=params.quantity, | ||
| category=params.category or ItemCategory.MISC, | ||
| value=params.value, | ||
| notes=params.notes, | ||
| added_at=now, | ||
| ) | ||
| items.append(new_item.model_dump()) | ||
|
|
||
| # Update inventory | ||
| inventories_collection.update_one( | ||
| {"party_id": str(params.party_id)}, | ||
| {"$set": {"items": items, "updated_at": now}}, | ||
| ) |
There was a problem hiding this comment.
This read-modify-write pattern has a race condition vulnerability. If two concurrent requests try to add the same item, one update could be lost. Consider using MongoDB's atomic operators like $inc for quantity updates or find_one_and_update with arrayFilters. For reference, the mongodb_get_memory function (line 2102) uses find_one_and_update with $inc for atomic updates.
| # Get current inventory | ||
| inventory_doc = inventories_collection.find_one({"party_id": str(params.party_id)}) | ||
| if not inventory_doc: | ||
| raise ValueError(f"Inventory for party {params.party_id} not found") | ||
|
|
||
| now = datetime.now(timezone.utc) | ||
| items = inventory_doc.get("items", []) | ||
|
|
||
| # Find the item | ||
| item_index = None | ||
| for i, item in enumerate(items): | ||
| if item["name"] == params.item_name: | ||
| item_index = i | ||
| break | ||
|
|
||
| if item_index is None: | ||
| raise ValueError(f"Item '{params.item_name}' not found in inventory") | ||
|
|
||
| item = items[item_index] | ||
|
|
||
| # Check for insufficient quantity | ||
| if params.quantity is not None and params.quantity > item["quantity"]: | ||
| raise ValueError( | ||
| f"Insufficient quantity: have {item['quantity']}, trying to remove {params.quantity}" | ||
| ) | ||
|
|
||
| if params.quantity is None or params.quantity >= item["quantity"]: | ||
| # Remove item completely | ||
| items.pop(item_index) | ||
| else: | ||
| # Decrement quantity | ||
| item["quantity"] -= params.quantity | ||
|
|
||
| # Update inventory | ||
| inventories_collection.update_one( | ||
| {"party_id": str(params.party_id)}, | ||
| {"$set": {"items": items, "updated_at": now}}, | ||
| ) | ||
|
|
There was a problem hiding this comment.
This read-modify-write pattern has a race condition vulnerability similar to mongodb_add_inventory_item. If two concurrent requests try to remove the same item, the validation could pass for both but the second update would incorrectly modify the inventory state. Consider using MongoDB's atomic array operators ($pull, $inc with arrayFilters) or find_one_and_update for atomic operations.
| # Get current inventory | |
| inventory_doc = inventories_collection.find_one({"party_id": str(params.party_id)}) | |
| if not inventory_doc: | |
| raise ValueError(f"Inventory for party {params.party_id} not found") | |
| now = datetime.now(timezone.utc) | |
| items = inventory_doc.get("items", []) | |
| # Find the item | |
| item_index = None | |
| for i, item in enumerate(items): | |
| if item["name"] == params.item_name: | |
| item_index = i | |
| break | |
| if item_index is None: | |
| raise ValueError(f"Item '{params.item_name}' not found in inventory") | |
| item = items[item_index] | |
| # Check for insufficient quantity | |
| if params.quantity is not None and params.quantity > item["quantity"]: | |
| raise ValueError( | |
| f"Insufficient quantity: have {item['quantity']}, trying to remove {params.quantity}" | |
| ) | |
| if params.quantity is None or params.quantity >= item["quantity"]: | |
| # Remove item completely | |
| items.pop(item_index) | |
| else: | |
| # Decrement quantity | |
| item["quantity"] -= params.quantity | |
| # Update inventory | |
| inventories_collection.update_one( | |
| {"party_id": str(params.party_id)}, | |
| {"$set": {"items": items, "updated_at": now}}, | |
| ) | |
| now = datetime.now(timezone.utc) | |
| # Use atomic operations to avoid read-modify-write race conditions. | |
| party_id_str = str(params.party_id) | |
| if params.quantity is None: | |
| # Remove the entire item by name. | |
| result = inventories_collection.find_one_and_update( | |
| { | |
| "party_id": party_id_str, | |
| "items": {"$elemMatch": {"name": params.item_name}}, | |
| }, | |
| { | |
| "$pull": {"items": {"name": params.item_name}}, | |
| "$set": {"updated_at": now}, | |
| }, | |
| ) | |
| if result is None: | |
| # Determine whether the inventory or the item is missing. | |
| inventory_doc = inventories_collection.find_one({"party_id": party_id_str}) | |
| if not inventory_doc: | |
| raise ValueError(f"Inventory for party {params.party_id} not found") | |
| items = inventory_doc.get("items", []) | |
| if not any(item.get("name") == params.item_name for item in items): | |
| raise ValueError(f"Item '{params.item_name}' not found in inventory") | |
| # If we reach here, the item existed but the update failed for some | |
| # unexpected reason; fall back to a generic error. | |
| raise ValueError("Failed to remove item from inventory") | |
| else: | |
| # Decrement quantity atomically, ensuring it never becomes negative. | |
| result = inventories_collection.find_one_and_update( | |
| { | |
| "party_id": party_id_str, | |
| "items": { | |
| "$elemMatch": { | |
| "name": params.item_name, | |
| "quantity": {"$gte": params.quantity}, | |
| } | |
| }, | |
| }, | |
| { | |
| "$inc": {"items.$.quantity": -params.quantity}, | |
| "$set": {"updated_at": now}, | |
| }, | |
| ) | |
| if result is None: | |
| # Determine specific error: inventory missing, item missing, or insufficient quantity. | |
| inventory_doc = inventories_collection.find_one({"party_id": party_id_str}) | |
| if not inventory_doc: | |
| raise ValueError(f"Inventory for party {params.party_id} not found") | |
| items = inventory_doc.get("items", []) | |
| matching_item = next( | |
| (item for item in items if item.get("name") == params.item_name), | |
| None, | |
| ) | |
| if matching_item is None: | |
| raise ValueError(f"Item '{params.item_name}' not found in inventory") | |
| item_quantity = matching_item.get("quantity", 0) | |
| if params.quantity > item_quantity: | |
| raise ValueError( | |
| f"Insufficient quantity: have {item_quantity}, trying to remove {params.quantity}" | |
| ) | |
| # If we reach here, the quantities appear valid but the update failed for | |
| # some unexpected reason; fall back to a generic error. | |
| raise ValueError("Failed to update item quantity in inventory") | |
| # Optional cleanup: remove items whose quantity reached zero. | |
| inventories_collection.update_one( | |
| { | |
| "party_id": party_id_str, | |
| }, | |
| { | |
| "$pull": { | |
| "items": { | |
| "name": params.item_name, | |
| "quantity": {"$lte": 0}, | |
| } | |
| } | |
| }, | |
| ) |
| # Check if item already exists | ||
| existing_item = None | ||
| for item in items: | ||
| if item["name"] == params.item_name: | ||
| existing_item = item | ||
| break | ||
|
|
||
| if existing_item: | ||
| # Increment quantity | ||
| existing_item["quantity"] += params.quantity | ||
| else: | ||
| # Add new item | ||
| new_item = InventoryItem( | ||
| name=params.item_name, |
There was a problem hiding this comment.
Item name matching is case-sensitive, which could lead to duplicate items with different capitalizations (e.g., "Health Potion" vs "health potion"). Consider normalizing item names (e.g., using .lower() or .strip().lower()) to prevent accidental duplicates, or document that item names are case-sensitive.
| # Check if item already exists | |
| existing_item = None | |
| for item in items: | |
| if item["name"] == params.item_name: | |
| existing_item = item | |
| break | |
| if existing_item: | |
| # Increment quantity | |
| existing_item["quantity"] += params.quantity | |
| else: | |
| # Add new item | |
| new_item = InventoryItem( | |
| name=params.item_name, | |
| # Normalize the incoming item name for comparison to avoid duplicates | |
| normalized_param_name = ( | |
| params.item_name.strip().casefold() if params.item_name is not None else None | |
| ) | |
| # Check if item already exists (case/whitespace-insensitive) | |
| existing_item = None | |
| if normalized_param_name is not None: | |
| for item in items: | |
| item_name = item.get("name") | |
| if isinstance(item_name, str) and item_name.strip().casefold() == normalized_param_name: | |
| existing_item = item | |
| break | |
| if existing_item: | |
| # Increment quantity | |
| existing_item["quantity"] += params.quantity | |
| else: | |
| # Add new item, storing a stripped name | |
| new_item_name = params.item_name.strip() if params.item_name is not None else None | |
| new_item = InventoryItem( | |
| name=new_item_name, |
| # Validate sub-parties (check that all members exist) | ||
| all_member_ids = [] | ||
| for sub_party in params.sub_parties: | ||
| all_member_ids.extend(sub_party.member_ids) |
There was a problem hiding this comment.
The validation doesn't check for duplicate member IDs across sub-parties. A character could be assigned to multiple sub-parties simultaneously, which would be logically invalid for a party split. Consider adding validation after line 2650 to check: if len(all_member_ids) != len(set(all_member_ids)), raise an error about duplicate members.
| notes=item_data.get("notes"), | ||
| added_at=now, | ||
| ) | ||
| items.append(item.model_dump()) |
There was a problem hiding this comment.
The model_dump() calls should use mode="json" to properly serialize UUIDs and datetime objects for MongoDB storage. Without this, the added_at datetime and any UUID fields will not be serialized correctly. See the codebase pattern at lines 994, 1323, etc.
| items.append(item.model_dump()) | |
| items.append(item.model_dump(mode="json")) |
| python_version = 3.11 | ||
|
|
||
| # Skip type-checking tests to avoid pydantic dataclass inference issues. | ||
| exclude = (?x)(^packages/data-layer/tests|^packages/agents/tests|^packages/cli/tests) |
There was a problem hiding this comment.
This root-level mypy.ini duplicates configuration that already exists in each package's pyproject.toml file (packages/data-layer/pyproject.toml line 72, packages/agents/pyproject.toml line 70, packages/cli/pyproject.toml line 71). The package-level configurations use 'strict = true' which is more rigorous than just excluding tests. Consider whether this root config is necessary or if it conflicts with the existing package-level configs.
| python_version = 3.11 | |
| # Skip type-checking tests to avoid pydantic dataclass inference issues. | |
| exclude = (?x)(^packages/data-layer/tests|^packages/agents/tests|^packages/cli/tests) | |
| # Root-level mypy configuration is intentionally minimal. | |
| # Per-package mypy settings (including strict mode and any exclusions) | |
| # are defined in each package's pyproject.toml file. |
|
|
||
| if new_gold < 0: | ||
| raise ValueError( | ||
| f"Insufficient gold: have {current_gold}, trying to subtract {abs(params.amount)}" |
There was a problem hiding this comment.
The error message incorrectly uses abs() when the amount is already negative. If params.amount is -100 and current_gold is 50, the message will say "trying to subtract 100" which is correct, but the logic is confusing. Consider: f"Insufficient gold: have {current_gold}, trying to subtract {-params.amount}" to make it clearer, or simply f"Insufficient gold: have {current_gold}, need at least {-params.amount}".
| f"Insufficient gold: have {current_gold}, trying to subtract {abs(params.amount)}" | |
| f"Insufficient gold: have {current_gold}, trying to subtract {-params.amount}" |
## Fixes Applied ### Critical MongoDB Serialization (3 locations) - **Line 2333**: Added `mode="json"` to model_dump() for initial_items serialization - **Line 2438**: Added `mode="json"` to model_dump() for add_inventory_item - **Line 2681**: Added `mode="json"` to model_dump() for party split sub_parties - Prevents UUID/datetime deserialization errors when reading from MongoDB ### Validation Improvements - **Line 2650**: Added duplicate member validation in mongodb_create_party_split - Checks if len(all_member_ids) != len(set(all_member_ids)) - Raises error if character assigned to multiple sub-parties - **Line 2421**: Implemented case-insensitive item name matching - Uses `.strip().casefold()` for comparison - Stores with preserved case but prevents duplicates like "Health Potion" vs "health potion" - Strips whitespace from stored names ### Error Message Clarity - **Line 2541**: Fixed gold error message to use `-params.amount` instead of `abs(params.amount)` - More intuitive when params.amount is negative ### Configuration Cleanup - **mypy.ini**: Removed root-level mypy.ini file - Duplicated package-level pyproject.toml configs - Package-level configs use `strict = true` which is more rigorous ## Not Addressed (Deferred) ### Race Condition Fixes (Lines 2444, 2508) The review identified read-modify-write race conditions in: - `mongodb_add_inventory_item` - `mongodb_remove_inventory_item` **Rationale for deferring:** - Requires MongoDB atomic operators ($inc, $pull, arrayFilters, find_one_and_update) - Significant refactoring (~80 lines for remove_inventory_item alone) - Current implementation is correct for single-threaded usage - Can be addressed in follow-up task if high concurrency becomes a requirement **Tracking:** Created technical debt item for DL-16 race condition fixes ## Testing All 360 tests passing ✅ - 26 party inventory tests - No regressions in existing functionality 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary
Implements DL-16: Manage Party Inventory & Splits - MongoDB operations for shared party inventory and party split tracking.
Party Inventory Features
Party Split Features
Implementation Details
New Files:
src/monitor_data/schemas/party_inventory.py- 10 schemas for inventory and splitstests/test_tools/test_party_inventory_tools.py- 26 comprehensive testsModified Files:
src/monitor_data/tools/mongodb_tools.py- 10 new MongoDB tools (570 lines)src/monitor_data/schemas/__init__.py- Export party inventory schemassrc/monitor_data/middleware/auth.py- Authority matrix entriesMongoDB Collections:
party_inventories:{party_id, gold, items[], created_at, updated_at}party_splits:{split_id, party_id, sub_parties[], status, created_at, resolved_at, resolution_notes}Authority:
Architecture Decisions
party_inventoriesandparty_splitsfor different update patternsTest Coverage
Dependencies
🤖 Generated with Claude Code