diff --git a/planning_notes/validate_client_actions/GOALS_AND_DECISIONS.md b/planning_notes/validate_client_actions/GOALS_AND_DECISIONS.md new file mode 100644 index 0000000..1794f7b --- /dev/null +++ b/planning_notes/validate_client_actions/GOALS_AND_DECISIONS.md @@ -0,0 +1,554 @@ +# Client Action Validation & Data Filtering: Goals & Design Decisions + +**Date:** November 21, 2025 +**Status:** Design Document +**Context:** Server-side validation and data filtering for Break Escape game + +--- + +## Executive Summary + +This document captures the strategic goals and design decisions for implementing server-side validation and intelligent data filtering in Break Escape. The system prevents client-side cheating by validating all player actions server-side while maintaining clean separation between sensitive data (solutions) and game mechanics. + +--- + +## Strategic Goals + +### Primary Goals + +1. **Prevent Client-Side Cheating** + - Players cannot modify client code to unlock doors without solving puzzles + - Players cannot access locked container contents without unlocking them + - Players cannot collect items they shouldn't have access to + - Players cannot access rooms before they're supposed to + +2. **Maintain Security Without Exposing Solutions** + - Send game data to client without revealing answers + - Hide the `requires` field (the solution/password/PIN) + - Hide `contents` of locked containers until unlocked + - Verify all unlock attempts server-side + +3. **Enable Rich Client Experience** + - Client needs enough data to render rooms and interactions + - Client needs item/object information (descriptions, types, appearances) + - Client needs NPC presence and information + - Client needs lock information (type, not answer) + +4. **Track Player Progression** + - Know which rooms player can access + - Know which objects player has unlocked + - Know which NPCs player has encountered + - Know what's in player inventory + +5. **Support Lazy-Loading Architecture** + - Rooms load dynamically as needed + - Container contents load on-demand after unlock + - Reduce initial payload size + - Enable scalable scenarios with many rooms + +### Secondary Goals + +1. **Audit & Analytics** + - Track what players collect and when + - Record unlock attempts and successes + - Monitor for suspicious activity (future) + +2. **Extensibility** + - Design supports future features (rate limiting, attempt logging) + - Container system works for any lockable object + - NPC state tracking foundation for future dialogue progression + +3. **Clean Architecture** + - Separation of concerns (model keeps raw data, controller filters) + - Reusable validation helpers + - Consistent error responses + +--- + +## Key Design Decisions + +### Decision 1: Filter at Controller Level (Not Model) + +**Decision:** Keep `scenario_data` raw in Game model. Filter in controller responses. + +**Rationale:** +- ✅ Single source of truth - scenario_data unchanged +- ✅ Different endpoints can filter differently +- ✅ Easier to debug (compare raw vs filtered) +- ✅ Model doesn't need to know about view concerns +- ✅ Faster queries (no JSON transforms in DB) + +**Alternative Considered:** +- Filter at model level via method - adds coupling, harder to customize per endpoint + +**Implementation:** +```ruby +# Game model stays pure +scenario_data = { "rooms" => { ... }, "requires" => "secret" } + +# Controller decides what to expose +filtered_data = @game.filtered_room_data(room_id) # Returns data minus 'requires' +``` + +--- + +### Decision 2: Hide Only `requires` and `contents` (Not `lockType`) + +**Decision:** +- HIDE: `requires` field (the answer: password, PIN, key_id, etc.) +- HIDE: `contents` field (what's inside locked containers) +- SHOW: Everything else (`lockType`, `locked`, `observations`, etc.) + +**Rationale:** +- ✅ Client needs to know HOW to unlock (lockType: "password" vs "key" vs "pin") +- ✅ Client needs to know IF something is locked (locked: true/false) +- ✅ Player knows what's in a safe IRL - they see it's a lock, just don't know combo +- ✅ Minimal filtering - removes only what breaks security + +**Alternative Considered:** +- Hide `lockType` too - breaks UX, client can't show correct UI for password vs PIN vs key +- Hide `locked` status - breaks feedback, player doesn't know if something is unlocked + +**Example:** +```json +// Client sees: +{ + "type": "safe", + "name": "Reception Safe", + "locked": true, + "lockType": "pin", + "observations": "A small wall safe behind the reception desk. Looks like it needs a 4-digit code." + // HIDDEN: "requires": "9573" + // HIDDEN: "contents": [...] +} +``` + +--- + +### Decision 3: Separate `contents` Endpoint + +**Decision:** Add `/games/:id/container/:container_id` endpoint to load container contents after unlock. + +**Rationale:** +- ✅ Lazy-loading - only fetch contents when actually opened +- ✅ Security boundary - can't mass-fetch all contents +- ✅ Clear permission check - container must be in unlockedObjects +- ✅ Performance - reduces room data payload +- ✅ Mirrors real UX - opening a safe reveals its contents + +**Alternative Considered:** +- Include contents in room data from start - massive payload, security risk +- Never show contents, only abstract summary - breaks exploration feel + +**Implementation:** +``` +GET /games/:id/container/reception_safe +→ { container_id: "reception_safe", contents: [...] } + // Only if reception_safe in unlockedObjects + // Returns 403 Forbidden otherwise +``` + +--- + +### Decision 4: Initialize Player State on Game Creation + +**Decision:** On game creation, populate `unlockedRooms` with start room and `inventory` with starting items. + +**Rationale:** +- ✅ Single source of truth - scenario defines what player starts with +- ✅ Consistency - all games initialized the same way +- ✅ Simplicity - client doesn't need bootstrap logic +- ✅ Supports game reset (re-initialize if needed) + +**Alternative Considered:** +- Initialize on first request - harder to audit, timing issues +- Client-side initialization - security risk, can cheat before sync + +**Implementation:** +```ruby +def initialize_player_state! + player_state['unlockedRooms'] = [scenario_data['startRoom']] + player_state['inventory'] = scenario_data['startItemsInInventory'] + player_state['currentRoom'] = scenario_data['startRoom'] + save! +end +``` + +--- + +### Decision 5: Validate Inventory Operations Against Scenario + +**Decision:** Server verifies item exists in scenario AND is in an accessible location AND player meets prerequisites. + +**Rationale:** +- ✅ Prevents collecting non-existent items (modified client claims fake item) +- ✅ Prevents collecting from locked rooms (player unlocked on client but not server) +- ✅ Prevents collecting items held by unmet NPCs (client doesn't know encounter requirement) +- ✅ Prevents collecting from locked containers (client somehow gets contents early) + +**Validation Chain:** +1. Does item exist in scenario? → Error if not +2. Is item takeable? → Error if not +3. Is item's container unlocked (if nested)? → Error if not +4. Is item's room unlocked (if locked room)? → Error if not +5. Is item held by an NPC? If yes, is NPC encountered? → Error if not + +**Alternative Considered:** +- Trust client completely - simple but allows cheating +- Only check existence - allows room/container bypass +- No validation - breaks game integrity + +--- + +### Decision 6: Track NPC Encounters Automatically + +**Decision:** When room loads, add all NPC IDs to `encounteredNPCs` in player_state. + +**Rationale:** +- ✅ Automatic - no separate API call needed +- ✅ Fair - player must physically reach room with NPC +- ✅ Enables NPC-held items - once encountered, can collect their items +- ✅ Groundwork for future dialogue progression tracking + +**Alternative Considered:** +- Only track on conversation start - misses silent encounters, harder to track +- Require explicit "talk to NPC" action - more control but less intuitive + +**Implementation:** +```ruby +# In room endpoint, after serving room data: +if room_data['npcs'].present? + @game.player_state['encounteredNPCs'] ||= [] + @game.player_state['encounteredNPCs'].concat(room_data['npcs'].map { |n| n['id'] }) + @game.player_state['encounteredNPCs'].uniq! +end +``` + +--- + +### Decision 7: Permissive Unlock Model (Once Unlocked, Always Unlocked) + +**Decision:** When door/container unlocked, stays unlocked. No re-locking on reload or time-based relocking. + +**Rationale:** +- ✅ Matches user expectation - unlock a door, it stays unlocked +- ✅ Persistent progress - player doesn't lose progress on page refresh +- ✅ Simple to implement - no timer/state logic needed +- ✅ Aligns with game design - escape room puzzles are one-way progression + +**Alternative Considered:** +- Session-based unlocking (unlock disappears on reload) - frustrating UX +- Time-based relocking (unlock expires after N minutes) - confusing gameplay +- Restrictive per-room unlocks - impossible to revisit content + +**Note:** If scenario design requires re-locking (e.g., "security system reset"), can be added later with globalVariables tracking. + +--- + +### Decision 8: No Unlock Attempt Tracking (Phase 1) + +**Decision:** Validate unlock attempts but don't log failures. Just return success/failure. + +**Rationale:** +- ✅ Simpler Phase 1 - focus on validation, not analytics +- ✅ No database bloat - saves storage/queries +- ✅ Can add later - structure supports rate limiting future +- ✅ Sufficient for security - server still validates, prevents brute force client-side + +**Alternative Considered:** +- Log all attempts - useful for analytics, adds complexity +- Log only failures - still adds DB overhead + +**Future Enhancement:** Add attempt logging in Phase 3 if needed for analytics/security audit. + +--- + +### Decision 9: Scenario Map Endpoint for Layout Metadata + +**Decision:** Create `/games/:id/scenario_map` that returns minimal room layout without revealing objects/contents. + +**Rationale:** +- ✅ Supports planning - client can show map/navigation hints +- ✅ No solutions exposed - only structure (types, connections, accessibility) +- ✅ Separate from room data - can cache differently +- ✅ Enables UI features - map showing locked vs accessible rooms + +**Response Contains:** +- Room IDs and types +- Connections (which rooms connect where) +- Locked status and lock types +- NPC counts (no details) +- Accessibility (based on unlockedRooms) + +**Does NOT contain:** +- Object lists +- Container contents +- Solutions (`requires` fields) + +--- + +### Decision 10: NPC-Held Items Validation + +**Decision:** Check `itemsHeld` on NPCs. Items can only be collected if NPC is encountered. + +**Rationale:** +- ✅ Prevents item duplication - NPC item appears as collectible only after meeting NPC +- ✅ Enforces game flow - can't shortcut conversations +- ✅ Mirrors container logic - items locked until condition met +- ✅ Supports dialogue rewards - future dialogue choices can give items + +**Alternative Considered:** +- No NPC item tracking - allows client to claim items arbitrarily +- Only allow after specific dialogue node - too complex for Phase 1 + +**Implementation:** +```ruby +def find_npc_holding_item(item_type, item_id) + # Returns NPC info if found + # Validation: Only allow if npc['id'] in player_state['encounteredNPCs'] +end +``` + +--- + +## Data Flow Architecture + +### Room Loading Flow + +``` +Client Request: GET /games/:id/room/office_1 + ↓ +Server: [1] Check if office_1 in unlockedRooms + ↓ (if not, return 403 Forbidden) + [2] Load scenario_data['rooms']['office_1'] + ↓ + [3] Filter: Remove 'requires' fields recursively + ↓ + [4] Filter: Remove 'contents' fields recursively + ↓ + [5] Track NPC encounters (add to encounteredNPCs) + ↓ +Client Response: { room_id: "office_1", room: {...filtered...} } +``` + +### Inventory Add Flow + +``` +Client Request: POST /games/:id/inventory + { action: "add", item: { type: "key", id: "key_1" } } + ↓ +Server: [1] Find item in scenario_data (all rooms) + ↓ (if not found, return error) + [2] Check if item is takeable + ↓ (if not, return error) + [3] Find if item in locked container + ↓ + [4] If yes, check container in unlockedObjects + ↓ (if not, return error) + [5] Find if item in locked room + ↓ + [6] If yes, check room in unlockedRooms + ↓ (if not, return error) + [7] Check if NPC holds item + ↓ + [8] If yes, check NPC in encounteredNPCs + ↓ (if not, return error) + [9] Add item to inventory + ↓ +Client Response: { success: true, inventory: [...] } +``` + +### Unlock Flow + +``` +Client Request: POST /games/:id/unlock + { targetType: "door", targetId: "office_1", + attempt: "key_1", method: "key" } + ↓ +Server: [1] Validate unlock attempt + (@game.validate_unlock checks server secrets) + ↓ (if invalid, return error) + [2] Record unlock: add to unlockedRooms or unlockedObjects + ↓ + [3] If door, return filtered room data + ↓ +Client Response: { success: true, type: "door", roomData: {...} } +``` + +### Container Opening Flow + +``` +Client Request: GET /games/:id/container/safe_1 + ↓ +Server: [1] Check if safe_1 in unlockedObjects + ↓ (if not, return 403 Forbidden) + [2] Find safe_1 in scenario_data + ↓ + [3] Get contents + ↓ + [4] Filter: Remove 'requires' from each item + ↓ + [5] Filter: Remove nested 'contents' + ↓ +Client Response: { container_id: "safe_1", contents: [...] } +``` + +--- + +## Security Model + +### What We're Protecting Against + +1. **Client Modification** + - Malicious client code claiming to have collected items + - Modifying unlock validation to bypass puzzles + - Injecting room access that wasn't earned + +2. **Browser DevTools Manipulation** + - Reading cached scenario data to find passwords + - Modifying player_state in local storage/memory + - Calling API endpoints with fake parameters + +3. **Network Inspection** + - Reading API responses for password hints + - Replaying unlock requests to collect same item twice + +### What We're NOT Protecting Against (Out of Scope) + +1. **Server Compromise** + - If attacker compromises Rails server, all is lost + - Assume infrastructure is secure + +2. **Network-Level Attacks** + - HTTPS handles man-in-the-middle + - Assume secure transport + +3. **Brute Force Unlock Attempts** + - Could add rate limiting in Phase 3 + - Not implemented in Phase 1 + +--- + +## Principles + +### 1. Server is Source of Truth + +- Client state is for UI only +- Server always validates before accepting changes +- Never trust `player_state` sent by client +- Always verify against scenario_data server-side + +### 2. Minimal Data Exposure + +- Only send data client needs for current action +- Hide solutions (`requires`) +- Hide inaccessible content (`contents` of locked containers) +- Expose lock types and status (needed for UX) + +### 3. Lazy Loading Everything + +- Room data only on room request +- Container contents only on container request +- NPC scripts only on NPC interaction +- Scenario map available for UI planning + +### 4. Fail Securely + +- When in doubt, deny access (default deny) +- 403 Forbidden for permission errors +- 404 Not Found for data not found +- Clear error messages for debugging (but not exploitable) + +### 5. Consistent Validation + +- Same validation logic for all item sources +- Same checks whether item from room, container, or NPC +- Same access control everywhere + +--- + +## Trade-offs & Compromises + +### Trade-off 1: Performance vs Security + +**Choice:** Full recursive filtering for every request. + +**Impact:** Slightly slower responses, but security critical. Caching can mitigate if needed. + +**Alternative:** Send unfiltered, filter on client. ❌ Rejected - exposes secrets. + +### Trade-off 2: Complexity vs Flexibility + +**Choice:** Separate container endpoint adds complexity. + +**Benefit:** Enables lazy-loading, prevents mass-extraction of secrets, matches UX expectations. + +**Alternative:** Include all contents in room data. ❌ Rejected - security risk and payload bloat. + +### Trade-off 3: Features vs Timeline + +**Choice:** Phase 1 doesn't include attempt logging or rate limiting. + +**Rationale:** Get validation working first, enhance later. Foundation supports it. + +**Future Phases:** Attempt logging (Phase 3), rate limiting (Phase 4). + +--- + +## Success Criteria + +### Phase 1 Success + +- ✅ `requires` field hidden from all room endpoints +- ✅ `contents` field hidden until container unlocked +- ✅ Inventory validation prevents collection from locked areas +- ✅ Room access tied to unlockedRooms +- ✅ NPC encounters tracked automatically +- ✅ Unlock status persists across reloads +- ✅ All tests pass + +### Phase 2 Success (Future) + +- Attempt logging working +- Analytics dashboard shows unlock patterns +- Rate limiting prevents brute force + +### Phase 3 Success (Future) + +- Suspicious activity detection +- Audit trail for admin review + +--- + +## Implementation Dependencies + +### Must Have Before Starting + +- ✅ Game model with scenario_data +- ✅ GamesController with basic room endpoint +- ✅ Player state initialized + +### Will Need to Add + +- Helper methods in Game model (filter_object_requires_recursive, etc.) +- Validation helpers in controller (find_item_in_scenario, etc.) +- New endpoints (container, scenario_map) +- Tests for each validation path + +--- + +## Open Questions (For Future Phases) + +1. **Dialogue Integration:** How do NPC encounters map to dialogue state? +2. **Item Persistence:** Can player permanently lose items? Drop items? +3. **Multiple Solutions:** Can item be unlocked via multiple methods (key OR password)? +4. **Resetting Progress:** Can player reset game state? Affects unlocked tracking. +5. **Analytics:** What unlock/collection metrics matter for learning analysis? + +--- + +## Conclusion + +This design provides robust server-side validation and data filtering while maintaining a clean architecture and supporting the lazy-loading model. By filtering at the controller level and validating against scenario data, we prevent client-side cheating while allowing the client to render rich interactions and maintain smooth UX. + +The phased approach allows us to implement security first (Phase 1), then add analytics (Phase 2), then refine with rate limiting (Phase 3), without disrupting game functionality. diff --git a/planning_notes/validate_client_actions/IMPLEMENTATION_PLAN.md b/planning_notes/validate_client_actions/IMPLEMENTATION_PLAN.md new file mode 100644 index 0000000..b0af30e --- /dev/null +++ b/planning_notes/validate_client_actions/IMPLEMENTATION_PLAN.md @@ -0,0 +1,1117 @@ +# Client Action Validation & Data Filtering Implementation Plan + +**Date:** November 21, 2025 +**Status:** Planning +**Priority:** High - Security & Data Integrity + +--- + +## Overview + +This plan implements server-side validation and filtering to prevent client-side cheating while maintaining clean data separation. Key principles: + +1. **Filter at controller level** - Keep scenario_data raw in model, filter in responses +2. **Hide `requires` field** - Server validates, client doesn't see answers +3. **Hide `contents`** - Locked container contents loaded via separate endpoint +4. **Validate all inventory operations** - Check items exist and are collectible +5. **Track player state** - Unlock history and NPC encounters drive permissions +6. **Lazy-load containers** - `/games/:id/container/:container_id` endpoint + +--- + +## Phase 1: Data Model & Initialization + +### 1.1 Update Game Model Default player_state +**File:** `app/models/break_escape/game.rb` + +**Current:** +```ruby +t.jsonb :player_state, null: false, default: { + currentRoom: nil, + unlockedRooms: [], + unlockedObjects: [], + inventory: [], + encounteredNPCs: [], + globalVariables: {}, + biometricSamples: [], + biometricUnlocks: [], + bluetoothDevices: [], + notes: [], + health: 100 +} +``` + +**Changes needed:** +- Verify default structure exists +- Add initialization method to populate `unlockedRooms` and `inventory` on game creation + +### 1.2 Verify Initialization Method +**File:** `app/models/break_escape/game.rb` + +**Current implementation (lines 189-203):** ✅ Already exists via `before_create :initialize_player_state` + +**REQUIRED UPDATE:** Add starting items initialization: + +```ruby +def initialize_player_state + self.player_state ||= {} + self.player_state['currentRoom'] ||= scenario_data['startRoom'] + self.player_state['unlockedRooms'] ||= [scenario_data['startRoom']] + self.player_state['unlockedObjects'] ||= [] + self.player_state['inventory'] ||= [] + + # ADD THIS: Initialize starting items from scenario + if scenario_data['startItemsInInventory'].present? + scenario_data['startItemsInInventory'].each do |item| + self.player_state['inventory'] << item.deep_dup + end + end + + self.player_state['encounteredNPCs'] ||= [] + self.player_state['globalVariables'] ||= {} + self.player_state['biometricSamples'] ||= [] + self.player_state['biometricUnlocks'] ||= [] + self.player_state['bluetoothDevices'] ||= [] + self.player_state['notes'] ||= [] + self.player_state['health'] ||= 100 +end +``` + +--- + +## Phase 2: Data Filtering at Controller Level + +### 2.1 Fix `filtered_room_data` Method +**File:** `app/models/break_escape/game.rb` + +**Current implementation (lines 134-147) removes lockType incorrectly. Replace with:** + +```ruby +def filtered_room_data(room_id) + room = room_data(room_id)&.deep_dup + return nil unless room + + # Remove ONLY the 'requires' field (the solution) + # Keep lockType, locked, observations visible to client + filter_requires_and_contents_recursive(room) + + room +end + +private + +def filter_requires_and_contents_recursive(obj) + case obj + when Hash + # Remove 'requires' (the answer/solution) + obj.delete('requires') + + # Remove 'contents' if locked (lazy-loaded via separate endpoint) + obj.delete('contents') if obj['locked'] + + # Keep lockType - client needs it to show correct UI + # Keep locked - client needs it to show lock status + + # Recursively filter nested objects and NPCs + obj['objects']&.each { |o| filter_requires_and_contents_recursive(o) } + obj['npcs']&.each { |n| filter_requires_and_contents_recursive(n) } + + when Array + obj.each { |item| filter_requires_and_contents_recursive(item) } + end +end +``` + +### 2.2 Update Room Endpoint with Access Control +**File:** `app/controllers/break_escape/games_controller.rb` + +**Replace current room method (lines 20-34) with:** + +```ruby +def room + authorize @game if defined?(Pundit) + + room_id = params[:room_id] + return render_error('Missing room_id parameter', :bad_request) unless room_id.present? + + # Check if room is accessible (starting room OR in unlockedRooms) + is_start_room = @game.scenario_data['startRoom'] == room_id + is_unlocked = @game.player_state['unlockedRooms']&.include?(room_id) + + unless is_start_room || is_unlocked + return render_error("Room not accessible: #{room_id}", :forbidden) + end + + # Auto-add start room if missing (defensive programming) + if is_start_room && !is_unlocked + @game.player_state['unlockedRooms'] ||= [] + @game.player_state['unlockedRooms'] << room_id + @game.save! + end + + # Get and filter room data + room_data = @game.filtered_room_data(room_id) + return render_error("Room not found: #{room_id}", :not_found) unless room_data + + # Track NPC encounters BEFORE sending response + track_npc_encounters(room_id, room_data) + + Rails.logger.debug "[BreakEscape] Serving room data for: #{room_id}" + + render json: { room_id: room_id, room: room_data } +end + +private + +def track_npc_encounters(room_id, room_data) + return unless room_data['npcs'].present? + + npc_ids = room_data['npcs'].map { |npc| npc['id'] } + @game.player_state['encounteredNPCs'] ||= [] + + new_npcs = npc_ids - @game.player_state['encounteredNPCs'] + return if new_npcs.empty? + + @game.player_state['encounteredNPCs'].concat(new_npcs) + @game.save! + + Rails.logger.debug "[BreakEscape] Tracked NPC encounters: #{new_npcs.join(', ')}" +end +``` + +**Note:** The `filter_requires_and_contents_recursive` method already handles removing locked contents, so no separate `filter_contents_field` helper is needed. + +### 2.3 Rename Existing Scenario Endpoint to scenario_map +**File:** `app/controllers/break_escape/games_controller.rb` + +**Current scenario method (lines 13-17) serves full unfiltered data. This should be renamed and filtered.** + +**Replace with:** +```ruby +# GET /games/:id/scenario_map +# Returns minimal scenario metadata for navigation (no room contents) +def scenario_map + authorize @game if defined?(Pundit) + + # Return minimal room/connection metadata without contents + layout = {} + @game.scenario_data['rooms'].each do |room_id, room_data| + layout[room_id] = { + type: room_data['type'], + connections: room_data['connections'] || {}, + locked: room_data['locked'] || false, + lockType: room_data['lockType'], + hasNPCs: (room_data['npcs']&.length || 0) > 0, + accessible: @game.room_unlocked?(room_id) + } + end + + render json: { + startRoom: @game.scenario_data['startRoom'], + currentRoom: @game.player_state['currentRoom'], + rooms: layout + } +end +``` + +**Update routes (config/routes.rb):** +```ruby +# Change 'scenario' to 'scenario_map' +get 'scenario_map' # Minimal layout metadata for navigation +``` + +**BREAKING CHANGE:** This renames the endpoint. Client code calling `/games/:id/scenario` will need to be updated to call `/games/:id/scenario_map` instead. + +--- + +## Phase 3: Locked Container Lazy-Loading + +### 3.1 Create Container Endpoint +**File:** `app/controllers/break_escape/games_controller.rb` + +**New method (add after room endpoint):** +```ruby +# GET /games/:id/container/:container_id +# Returns container contents after unlock (lazy-loaded) +def container + authorize @game if defined?(Pundit) + + container_id = params[:container_id] + return render_error('Missing container_id parameter', :bad_request) unless container_id.present? + + # Find container in scenario data + container_data = find_container_in_scenario(container_id) + return render_error("Container not found: #{container_id}", :not_found) unless container_data + + # Check if container is unlocked (check multiple possible identifiers) + is_unlocked = check_container_unlocked(container_id, container_data) + + unless is_unlocked + return render_error("Container not unlocked: #{container_id}", :forbidden) + end + + # Return filtered contents + contents = filter_container_contents(container_data) + + Rails.logger.debug "[BreakEscape] Serving container contents for: #{container_id}" + + render json: { + container_id: container_id, + contents: contents + } +end + +private + +def check_container_unlocked(container_id, container_data) + unlocked_list = @game.player_state['unlockedObjects'] || [] + + # Check multiple possible identifiers + unlocked_list.include?(container_id) || + unlocked_list.include?(container_data['id']) || + unlocked_list.include?(container_data['name']) || + unlocked_list.include?(container_data['type']) +end + +def filter_container_contents(container_data) + contents = container_data['contents']&.map do |item| + item_copy = item.deep_dup + filter_requires_and_contents_recursive(item_copy) + item_copy + end || [] + + contents +end + +private + +def find_container_in_scenario(container_id) + @game.scenario_data['rooms'].each do |room_id, room_data| + # Search objects for container + container = find_container_recursive(room_data['objects'], container_id) + return container if container + + # Search nested contents + room_data['objects']&.each do |obj| + container = search_nested_contents(obj['contents'], container_id) + return container if container + end + end + nil +end + +def find_container_recursive(objects, container_id) + return nil unless objects + + objects.each do |obj| + # Check if this object matches + if obj['id'] == container_id || (obj['name'] && obj['name'] == container_id) + return obj if obj['contents'].present? + end + + # Recursively search nested contents + nested = find_container_recursive(obj['contents'], container_id) + return nested if nested + end + nil +end + +def search_nested_contents(contents, container_id) + return nil unless contents + + contents.each do |item| + return item if (item['id'] == container_id || item['name'] == container_id) && item['contents'].present? + nested = search_nested_contents(item['contents'], container_id) + return nested if nested + end + nil +end +``` + +**Add to routes:** +```ruby +get 'container/:container_id', to: 'games#container' # Load locked container contents +``` + +--- + +## Phase 4: Inventory Validation + +### 4.1 Update Inventory Endpoint +**File:** `app/controllers/break_escape/games_controller.rb` + +**Update inventory method:** +```ruby +def inventory + authorize @game if defined?(Pundit) + + action_type = params[:action_type] || params[:actionType] + item = params[:item] + + case action_type + when 'add' + # Validate item exists and is collectible + validation_error = validate_item_collectible(item) + if validation_error + return render json: { success: false, message: validation_error }, + status: :unprocessable_entity + end + + @game.add_inventory_item!(item.to_unsafe_h) + render json: { success: true, inventory: @game.player_state['inventory'] } + + when 'remove' + @game.remove_inventory_item!(item['id']) + render json: { success: true, inventory: @game.player_state['inventory'] } + + else + render json: { success: false, message: 'Invalid action' }, status: :bad_request + end +end + +private + +def validate_item_collectible(item) + item_type = item['type'] + item_id = item['id'] + + # Search scenario for this item + found_item = find_item_in_scenario(item_type, item_id) + return "Item not found in scenario: #{item_type}" unless found_item + + # Check if item is takeable + unless found_item['takeable'] + return "Item is not takeable: #{found_item['name']}" + end + + # If item is in locked container, check if container is unlocked + container_info = find_item_container(item_type, item_id) + if container_info.present? + container_id = container_info[:id] + unless @game.player_state['unlockedObjects'].include?(container_id) + return "Container not unlocked: #{container_id}" + end + end + + # If item is in locked room, check if room is unlocked + room_info = find_item_room(item_type, item_id) + if room_info.present? + room_id = room_info[:id] + if room_info[:locked] && !@game.player_state['unlockedRooms'].include?(room_id) + return "Room not unlocked: #{room_id}" + end + end + + # Check if NPC holds this item and if NPC encountered + npc_info = find_npc_holding_item(item_type, item_id) + if npc_info.present? + npc_id = npc_info[:id] + unless @game.player_state['encounteredNPCs'].include?(npc_id) + return "NPC not encountered: #{npc_id}" + end + end + + nil # No error +end + +def find_item_in_scenario(item_type, item_id) + @game.scenario_data['rooms'].each do |room_id, room_data| + # Search room objects + room_data['objects']&.each do |obj| + return obj if obj['type'] == item_type && (obj['id'] == item_id || obj['name'] == item_id) + + # Search nested contents + obj['contents']&.each do |content| + return content if content['type'] == item_type && (content['id'] == item_id || content['name'] == item_id) + end + end + end + nil +end + +def find_item_container(item_type, item_id) + @game.scenario_data['rooms'].each do |room_id, room_data| + room_data['objects']&.each do |obj| + obj['contents']&.each do |content| + if content['type'] == item_type && (content['id'] == item_id || content['name'] == item_id) + return { id: obj['id'] || obj['name'], locked: obj['locked'] } + end + end + end + end + nil +end + +def find_item_room(item_type, item_id) + @game.scenario_data['rooms'].each do |room_id, room_data| + room_data['objects']&.each do |obj| + if obj['type'] == item_type && (obj['id'] == item_id || obj['name'] == item_id) + return { id: room_id, locked: room_data['locked'] } + end + end + end + nil +end + +def find_npc_holding_item(item_type, item_id) + @game.scenario_data['rooms'].each do |room_id, room_data| + room_data['npcs']&.each do |npc| + next unless npc['itemsHeld'].present? + + # itemsHeld is array of full item objects (same structure as room objects) + npc['itemsHeld'].each do |held_item| + # Match by type (required) and optionally by id/name + if held_item['type'] == item_type + # If item_id provided, verify it matches + if item_id.present? + item_matches = (held_item['id'] == item_id) || + (held_item['name'] == item_id) || + (item_id == item_type) # Fallback if no id field + next unless item_matches + end + + return { + id: npc['id'], + npc: npc, + item: held_item, + type: 'npc' + } + end + end + end + end + nil +end +``` + +--- + +## Phase 5: Track NPC Encounters + +### 5.1 Update Room Endpoint to Track NPCs +**File:** `app/controllers/break_escape/games_controller.rb` + +**Modify room method (after room_data returned):** +```ruby +def room + # ... existing code ... + + # Track NPC encounters + if room_data['npcs'].present? + npc_ids = room_data['npcs'].map { |npc| npc['id'] } + @game.player_state['encounteredNPCs'] ||= [] + @game.player_state['encounteredNPCs'].concat(npc_ids) + @game.player_state['encounteredNPCs'].uniq! + @game.save! + end + + # Filter out 'contents' field - lazy-loaded via separate endpoint + filter_contents_field(room_data) + + Rails.logger.debug "[BreakEscape] Serving room data for: #{room_id}" + + render json: { room_id: room_id, room: room_data } +end +``` + +--- + +## Phase 6: Unlock Validation & Tracking + +### 6.1 Update Unlock Endpoint with Transaction Safety +**File:** `app/controllers/break_escape/games_controller.rb` + +**Replace unlock method (lines 95-130) with:** +```ruby +def unlock + authorize @game if defined?(Pundit) + + target_type = params[:targetType] + target_id = params[:targetId] + attempt = params[:attempt] + method = params[:method] + + is_valid = @game.validate_unlock(target_type, target_id, attempt, method) + + unless is_valid + return render json: { + success: false, + message: 'Invalid attempt' + }, status: :unprocessable_entity + end + + # Use transaction to ensure atomic update + ActiveRecord::Base.transaction do + if target_type == 'door' + @game.unlock_room!(target_id) + + room_data = @game.filtered_room_data(target_id) + + render json: { + success: true, + type: 'door', + roomData: room_data + } + else + # Object/container unlock + @game.unlock_object!(target_id) + + render json: { + success: true, + type: 'object' + } + end + end +rescue ActiveRecord::RecordInvalid => e + render json: { + success: false, + message: "Failed to save unlock: #{e.message}" + }, status: :unprocessable_entity +end +``` + +**Note:** The `unlock_room!` and `unlock_object!` methods in the Game model already handle adding to `unlockedRooms`/`unlockedObjects` and saving, so we don't need to duplicate that logic here. + +--- + +## Phase 7: Sync State with Room Tracking + +### 7.1 Update Sync State Endpoint +**File:** `app/controllers/break_escape/games_controller.rb` + +**Modify sync_state method:** +```ruby +def sync_state + authorize @game if defined?(Pundit) + + # Update allowed fields + if params[:currentRoom] + # Verify room is accessible + if @game.player_state['unlockedRooms'].include?(params[:currentRoom]) + @game.player_state['currentRoom'] = params[:currentRoom] + else + return render json: { + success: false, + message: "Cannot enter locked room: #{params[:currentRoom]}" + }, status: :forbidden + end + end + + if params[:globalVariables] + @game.update_global_variables!(params[:globalVariables].to_unsafe_h) + end + + @game.save! + + render json: { success: true } +end +``` + +--- + +## Phase 8: Update Routes + +### 8.1 Add New Routes +**File:** `config/routes.rb` + +```ruby +resources :games, only: [:show, :create] do + member do + # Scenario and NPC data + get 'scenario' # Full scenario (kept for compatibility) + get 'scenario_map' # Minimal layout metadata + get 'ink' # Returns NPC script (JIT compiled) + get 'room/:room_id', to: 'games#room' # Returns room data for lazy-loading + get 'container/:container_id', to: 'games#container' # Returns locked container contents + + # Game state and actions + put 'sync_state' # Periodic state sync + post 'unlock' # Validate unlock attempt + post 'inventory' # Update inventory + end +end +``` + +--- + +## Implementation TODO List + +### ✅ PHASE 1: Data Model & Initialization +- [x] 1.1 Verify Game model has correct player_state default structure ✅ Done (db/migrate) +- [x] 1.2 `initialize_player_state` method ✅ Exists (model callback) +- [ ] 1.3 **UPDATE** `initialize_player_state` to add starting items to inventory +- [ ] 1.4 Test: Create game and verify player_state populated correctly with starting items + +### ✅ PHASE 2: Data Filtering +- [ ] 2.1 Create `filter_requires_and_contents_recursive` method (replaces current filter logic) +- [ ] 2.2 Update `filtered_room_data` to use new recursive filter (keep lockType visible) +- [ ] 2.3 Rename `scenario` endpoint to `scenario_map` and filter response +- [ ] 2.4 Update room endpoint with access control and NPC tracking +- [ ] 2.5 Update routes: rename `scenario` to `scenario_map` +- [ ] 2.6 Test: Room endpoint preserves `lockType`, removes `requires` and locked `contents` + +### ✅ PHASE 3: Container Lazy-Loading +- [ ] 3.1 Create `find_container_in_scenario` method +- [ ] 3.2 Create `find_container_recursive` method +- [ ] 3.3 Create `search_nested_contents` helper +- [ ] 3.4 Create `check_container_unlocked` helper (checks multiple ID types) +- [ ] 3.5 Create `filter_container_contents` helper +- [ ] 3.6 Implement `container` endpoint with proper ID resolution +- [ ] 3.7 Add `container/:container_id` route with `to:` parameter +- [ ] 3.8 Test: Container endpoint returns filtered contents only if unlocked + +### ✅ PHASE 4: Inventory Validation +- [ ] 4.1 Create `validate_item_collectible` method +- [ ] 4.2 Create `find_item_in_scenario` helper +- [ ] 4.3 Create `find_item_container` helper +- [ ] 4.4 Create `find_item_room` helper +- [ ] 4.5 **UPDATE** `find_npc_holding_item` helper (fix itemsHeld structure handling) +- [ ] 4.6 Update `inventory` endpoint with validation +- [ ] 4.7 Test: Items can't be collected from locked rooms/containers +- [ ] 4.8 Test: Items from NPCs can't be collected before encounter +- [ ] 4.9 Test: NPC itemsHeld validation works correctly + +### ✅ PHASE 5: NPC Encounter Tracking +- [x] 5.1 NPC encounter tracking added to room endpoint (Phase 2.4) ✅ Done +- [ ] 5.2 Test: encounteredNPCs populated when room loaded + +### ✅ PHASE 6: Unlock Tracking +- [ ] 6.1 **UPDATE** `unlock` endpoint with transaction safety +- [ ] 6.2 Test: Unlock operations are atomic +- [ ] 6.3 Test: Unlocked rooms accessible, locked rooms forbidden +- [ ] 6.4 Test: Unlocked containers' contents available via container endpoint + +### ✅ PHASE 7: Sync State Validation +- [ ] 7.1 Update `sync_state` to verify room accessibility +- [ ] 7.2 Test: Client can't sync to locked rooms + +### ✅ PHASE 8: Routing +- [ ] 8.1 Update routes in config/routes.rb + - Rename `get 'scenario'` to `get 'scenario_map'` + - Add `get 'container/:container_id', to: 'games#container'` +- [ ] 8.2 Verify all routes work: `rails routes | grep games#` + +### ✅ PHASE 9: Client Integration (CRITICAL) +- [ ] 9.1 Update `addToInventory()` to validate with server + - **File:** `public/break_escape/js/systems/inventory.js` + - Make async, add server validation before local inventory update + - Handle 403/422 responses with user-friendly error messages + - Add retry logic for network errors (3 retries with exponential backoff) +- [ ] 9.2 Update container minigame to lazy-load contents + - **File:** `public/break_escape/js/minigames/container/container-minigame.js` + - Add `loadContainerContents()` method to fetch from `/games/:id/container/:container_id` + - Handle 403 Forbidden for locked containers + - Show loading state while fetching +- [ ] 9.3 Update scenario loading to use `scenario_map` + - **File:** `public/break_escape/js/core/game.js` + - Change fetch URL from `/games/:id/scenario` to `/games/:id/scenario_map` + - Update expected response structure +- [ ] 9.4 Add error handling for room access + - Handle 403 Forbidden responses when trying to access locked rooms + - Show appropriate UI feedback +- [ ] 9.5 Test: Client-server validation flow + - Verify inventory adds are validated + - Verify containers load contents on-demand + - Verify locked rooms return 403 + +### ✅ PHASE 10: Testing & Validation +- [ ] 10.1 Create integration tests for filtering +- [ ] 10.2 Create integration tests for container endpoint +- [ ] 10.3 Create integration tests for inventory validation +- [ ] 10.4 Create integration tests for NPC encounter tracking +- [ ] 10.5 Create integration tests for unlock tracking +- [ ] 10.6 Create integration tests for NPC itemsHeld validation +- [ ] 10.7 Create integration tests for nested container validation +- [ ] 10.8 Create integration tests for sync_state validation +- [ ] 10.9 Test end-to-end: Complete puzzle with validation + +--- + +## Data Structures Reference + +### Player State After Initialization +```json +{ + "currentRoom": "reception", + "unlockedRooms": ["reception"], + "unlockedObjects": [], + "inventory": [ + { "type": "phone", "name": "Your Phone", "takeable": true }, + { "type": "workstation", "name": "Crypto Analysis Station", "takeable": true }, + { "type": "lockpick", "name": "Lock Pick Kit", "takeable": true } + ], + "encounteredNPCs": [], + "globalVariables": {}, + "biometricSamples": [], + "biometricUnlocks": [], + "bluetoothDevices": [], + "health": 100, + "initializedAt": "2025-11-21T12:00:00Z" +} +``` + +### Room Response (After Filtering) +```json +{ + "room_id": "reception", + "room": { + "type": "room_reception", + "connections": { "north": "office1" }, + "npcs": [ + { + "id": "neye_eve", + "displayName": "Neye Eve", + "storyPath": "scenarios/ink/neye-eve.json", + "npcType": "phone" + } + ], + "objects": [ + { + "type": "phone", + "name": "Reception Phone", + "takeable": false, + "readable": true, + "lockType": "password", + "locked": true, + "observations": "...", + "postitNote": "Password: secret123", + "showPostit": true + // NOTE: 'requires' field REMOVED + // NOTE: 'contents' field REMOVED + } + ] + } +} +``` + +### Container Response (After Unlock) +```json +{ + "container_id": "reception_safe", + "contents": [ + { + "type": "notes", + "name": "IT Access Credentials", + "takeable": true, + "readable": true, + "text": "Emergency IT Admin Credentials...", + "observations": "Sensitive IT credentials..." + // NOTE: 'requires' field REMOVED + } + ] +} +``` + +### Scenario Map Response +```json +{ + "startRoom": "reception", + "rooms": { + "reception": { + "type": "room_reception", + "connections": { "north": "office1" }, + "locked": false, + "lockType": null, + "hasNPCs": 3, + "accessible": true + }, + "office1": { + "type": "room_office", + "connections": { "north": ["office2", "office3"], "south": "reception" }, + "locked": true, + "lockType": "key", + "hasNPCs": 0, + "accessible": false + } + } +} +``` + +--- + +## Error Handling + +### Common Validation Errors + +**Container Not Unlocked:** +```json +{ + "success": false, + "message": "Container not unlocked: reception_safe" +} +``` + +**Item Not Collectible:** +```json +{ + "success": false, + "message": "Room not unlocked: ceo_office" +} +``` + +**Room Access Denied:** +```json +{ + "success": false, + "message": "Room not accessible: ceo_office" +} +``` + +**NPC Not Encountered:** +```json +{ + "success": false, + "message": "NPC not encountered: helper_npc" +} +``` + +--- + +## Client Integration Details (Phase 9) + +### 9.1 Update Inventory System to Validate with Server + +**File:** `public/break_escape/js/systems/inventory.js` + +**Find the `addToInventory` function (around line 183) and update:** + +```javascript +export async function addToInventory(sprite) { + if (!sprite || !sprite.scenarioData) { + console.warn('Invalid sprite for inventory'); + return false; + } + + try { + console.log("Adding to inventory:", { + objectId: sprite.objectId, + name: sprite.name, + type: sprite.scenarioData?.type, + currentRoom: window.currentPlayerRoom + }); + + // Check if the item is already in the inventory (local check first) + const itemIdentifier = createItemIdentifier(sprite.scenarioData); + const isAlreadyInInventory = window.inventory.items.some(item => + item && createItemIdentifier(item.scenarioData) === itemIdentifier + ); + + if (isAlreadyInInventory) { + console.log(`Item ${itemIdentifier} is already in inventory`); + return false; + } + + // NEW: Validate with server before adding + const gameId = window.gameId; + if (gameId) { + try { + const response = await fetch(`/break_escape/games/${gameId}/inventory`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Accept': 'application/json' + }, + body: JSON.stringify({ + action_type: 'add', + item: sprite.scenarioData + }) + }); + + const result = await response.json(); + + if (!result.success) { + // Server rejected - show error to player + console.warn('Server rejected inventory add:', result.message); + window.gameAlert(result.message || 'Cannot collect this item', 'error', 'Invalid Action', 3000); + return false; + } + + // Server accepted - continue with local inventory update + console.log('Server validated item collection:', result); + } catch (error) { + console.error('Failed to validate inventory with server:', error); + // Fail closed - don't add if server can't validate + window.gameAlert('Network error - please try again', 'error', 'Error', 3000); + return false; + } + } + + // ... rest of existing addToInventory logic continues unchanged ... + + // Remove from room if it exists + if (window.currentPlayerRoom && rooms[window.currentPlayerRoom] && rooms[window.currentPlayerRoom].objects) { + if (rooms[window.currentPlayerRoom].objects[sprite.objectId]) { + const roomObj = rooms[window.currentPlayerRoom].objects[sprite.objectId]; + if (roomObj.setVisible) { + roomObj.setVisible(false); + } + roomObj.active = false; + console.log(`Removed object ${sprite.objectId} from room`); + } + } + + // Continue with rest of function... + + } catch (error) { + console.error('Error in addToInventory:', error); + return false; + } +} +``` + +### 9.2 Update Container Minigame to Lazy-Load Contents + +**File:** `public/break_escape/js/minigames/container/container-minigame.js` + +**Add new method to ContainerMinigame class:** + +```javascript +async loadContainerContents() { + const gameId = window.gameId; + const containerId = this.containerItem.scenarioData.id || + this.containerItem.scenarioData.name || + this.containerItem.objectId; + + if (!gameId) { + console.error('No gameId available for container loading'); + return []; + } + + console.log(`Loading contents for container: ${containerId}`); + + try { + const response = await fetch(`/break_escape/games/${gameId}/container/${containerId}`, { + headers: { 'Accept': 'application/json' } + }); + + if (!response.ok) { + if (response.status === 403) { + window.gameAlert('Container is locked', 'error', 'Locked', 2000); + this.complete(false); + return []; + } + throw new Error(`HTTP ${response.status}: ${response.statusText}`); + } + + const data = await response.json(); + console.log(`Loaded ${data.contents?.length || 0} items from container`); + return data.contents || []; + } catch (error) { + console.error('Failed to load container contents:', error); + window.gameAlert('Could not load container contents', 'error', 'Error', 3000); + return []; + } +} +``` + +**Update the `init()` method to load contents asynchronously:** + +```javascript +async init() { + console.log('Container minigame init', { + containerItem: this.containerItem, + desktopMode: this.desktopMode, + mode: this.mode + }); + + // Show loading state + this.gameContainer.innerHTML = '
Loading contents...
'; + + // Load contents from server (if gameId exists) + if (window.gameId && this.containerItem.scenarioData.locked === false) { + this.contents = await this.loadContainerContents(); + } + // Otherwise use contents passed in (for unlocked containers in local game) + + // Create the UI + this.createContainerUI(); +} +``` + +### 9.3 Update Scenario Loading + +**File:** `public/break_escape/js/core/game.js` + +**Find the scenario loading code and update endpoint:** + +```javascript +// Around line 650 or wherever scenario is loaded +async function loadScenario(gameId) { + try { + // Changed from '/scenario' to '/scenario_map' + const response = await fetch(`/break_escape/games/${gameId}/scenario_map`); + + if (!response.ok) { + throw new Error(`Failed to load scenario: ${response.statusText}`); + } + + const data = await response.json(); + console.log('Loaded scenario map:', data); + + // Store the scenario map + window.gameScenarioMap = data; + + return data; + } catch (error) { + console.error('Error loading scenario:', error); + throw error; + } +} +``` + +--- + +## Testing Strategy + +### Unit Tests +- Test `filter_requires_and_contents_recursive` removes only `requires` and locked `contents` +- Test `find_item_in_scenario` locates items correctly +- Test validation helpers work with nested structures + +### Integration Tests +- Test room endpoint filtering (lockType preserved, requires removed) +- Test container endpoint access control (403 for locked, 200 for unlocked) +- Test inventory validation (rejects items from locked rooms/containers) +- Test NPC encounter tracking (auto-added when entering room) +- Test unlock tracking (persists across requests) +- Test sync_state room validation (rejects locked rooms) + +### End-to-End Tests +- Complete scenario: start → collect items → unlock doors → get contents + +--- + +## Future Enhancements (Deferred) + +1. **Rate limiting** - Can be added later via Rack::Attack at API gateway level +2. **Attempt logging** - Store failed unlock attempts for audit and analytics +3. **Item expiry** - Items might become unavailable after certain events +4. **NPC state tracking** - Track conversation progress with NPCs beyond encounters +5. **Biometric/Bluetooth tracking** - Auto-populate collections when items scanned/detected +6. **Performance optimizations** - Add caching for filtered room data (deferred - rooms not re-requested) +7. **Item index caching** - In-memory index of all items for faster validation (deferred - premature optimization) + +--- + +## Notes & Design Decisions + +### Security Model +- All `requires` fields (passwords, PINs, keys) hidden from client at controller level +- All `contents` fields hidden until explicit container endpoint call (lazy-loading) +- `lockType` and `locked` fields **visible** to client (needed for UI) +- Player state tracks all permissions (unlockedRooms, unlockedObjects, encounteredNPCs) + +### Validation Flow +- Inventory operations validate against scenario data AND player state +- NPC encounters tracked automatically when room loaded (no separate API call) +- No unlock attempt tracking in Phase 1 (validates but doesn't log failures) +- Transaction safety ensures atomic state updates + +### Breaking Changes +- `/games/:id/scenario` renamed to `/games/:id/scenario_map` +- Client code must be updated to call new endpoint names +- `scenario_map` returns minimal metadata, not full scenario with objects + +### Out of Scope (Scenario Design Issues) +- Starting inventory duplication prevention (scenario author responsibility) +- Nested container cascade unlocking (intentional - each lock requires separate action) diff --git a/planning_notes/validate_client_actions/IMPLEMENTATION_UPDATES.md b/planning_notes/validate_client_actions/IMPLEMENTATION_UPDATES.md new file mode 100644 index 0000000..eece0e4 --- /dev/null +++ b/planning_notes/validate_client_actions/IMPLEMENTATION_UPDATES.md @@ -0,0 +1,291 @@ +# Implementation Plan Updates +**Date:** November 21, 2025 +**Status:** Updated based on review feedback + +--- + +## Summary of Changes + +This document summarizes the updates made to `IMPLEMENTATION_PLAN.md` based on the comprehensive review in `review2.md`. + +--- + +## Key Fixes Applied + +### 1. ✅ Fixed Data Filtering Logic + +**Issue:** Current `filtered_room_data` removes `lockType`, breaking client UI. + +**Fix Applied:** +- Updated `filter_requires_and_contents_recursive` to **keep** `lockType` and `locked` fields visible +- Only removes `requires` (the answer/solution) and locked `contents` +- Aligns with design decision in GOALS_AND_DECISIONS.md + +### 2. ✅ Added Starting Items Initialization + +**Issue:** Plan didn't verify starting items are added to inventory on game creation. + +**Fix Applied:** +- Updated `initialize_player_state` to populate inventory with items from `startItemsInInventory` +- Uses `deep_dup` to prevent shared object references + +### 3. ✅ Fixed NPC itemsHeld Validation + +**Issue:** Validation code didn't match actual data structure (array of full objects, not just IDs). + +**Fix Applied:** +- Updated `find_npc_holding_item` to handle full item objects in `itemsHeld` array +- Matches by `type` primarily, with optional `id`/`name` verification +- Returns full NPC and item info for validation context + +### 4. ✅ Added Transaction Safety + +**Issue:** State mutations could fail mid-operation, causing client-server desync. + +**Fix Applied:** +- Wrapped `unlock` endpoint in `ActiveRecord::Base.transaction` +- Added rescue for `RecordInvalid` with proper error response +- Ensures atomic updates to `player_state` + +### 5. ✅ Added Defensive Start Room Check + +**Issue:** Race condition could deny access to starting room on first request. + +**Fix Applied:** +- Room endpoint checks both `is_start_room` OR `is_unlocked` +- Auto-adds start room to `unlockedRooms` if missing (defensive programming) +- Prevents 403 error on legitimate first room access + +### 6. ✅ Renamed Scenario Endpoint + +**Issue:** Current `/games/:id/scenario` returns full unfiltered data. + +**Fix Applied:** +- Renamed to `/games/:id/scenario_map` +- Returns minimal metadata (connections, types, lock status) without object contents +- **BREAKING CHANGE:** Client code must be updated + +### 7. ✅ Fixed Container ID Resolution + +**Issue:** Containers might be identified by `id`, `name`, `objectId`, or `type`. + +**Fix Applied:** +- Added `check_container_unlocked` helper that checks all possible identifiers +- Prevents unlock bypass via ID mismatch + +### 8. ✅ Fixed Container Endpoint Route + +**Issue:** Plan showed `get 'container/:container_id'` without `to:` parameter. + +**Fix Applied:** +- Updated to `get 'container/:container_id', to: 'games#container'` + +### 9. ✅ Added Phase 9: Client Integration + +**Issue:** Plan had no guidance for updating client code to call new endpoints. + +**Fix Applied:** +- Added detailed Phase 9 with specific file changes +- Includes code examples for: + - `addToInventory()` server validation + - Container minigame lazy-loading + - Scenario map endpoint usage + - Error handling and retry logic + +### 10. ✅ Merged NPC Tracking into Room Endpoint + +**Issue:** Plan had separate Phase 5 for NPC tracking, but it's done in room endpoint. + +**Fix Applied:** +- Moved NPC tracking logic into Phase 2 (room endpoint update) +- Tracks encounters **before** sending response (transactional) +- Phase 5 now just has testing tasks + +--- + +## Items Deferred Based on Feedback + +### ⏸️ Rate Limiting +- **Decision:** Defer to later phase +- **Rationale:** Can be added via Rack::Attack at API gateway level +- **Status:** Moved to "Future Enhancements (Deferred)" + +### ⏸️ Cached Filtered Room Data +- **Decision:** Not implementing +- **Rationale:** Rooms won't be re-requested in typical gameplay +- **Status:** Moved to "Future Enhancements (Deferred)" + +### ⏸️ Item Index Caching +- **Decision:** Not implementing in Phase 1 +- **Rationale:** Premature optimization; validate performance need first +- **Status:** Moved to "Future Enhancements (Deferred)" + +### ⏸️ Starting Inventory Duplication Prevention +- **Decision:** Out of scope +- **Rationale:** Scenario design issue, not server validation issue +- **Status:** Documented as scenario author responsibility + +--- + +## Breaking Changes + +### 1. Scenario Endpoint Renamed +- **Before:** `GET /games/:id/scenario` +- **After:** `GET /games/:id/scenario_map` +- **Impact:** Client code must update fetch URL +- **Response:** Now returns minimal metadata, not full scenario with objects + +### 2. Container Contents Lazy-Loaded +- **Before:** Contents included in room data +- **After:** Contents fetched via `GET /games/:id/container/:container_id` +- **Impact:** Container minigame must call server to load contents +- **Benefit:** Security - can't extract all contents without unlocking + +### 3. Inventory Adds Require Server Validation +- **Before:** Local-only inventory updates +- **After:** `POST /games/:id/inventory` validates before adding +- **Impact:** `addToInventory()` becomes async +- **Benefit:** Prevents collecting items from locked rooms/containers + +--- + +## Updated Phase Order + +### Original Plan +1. Data Model & Initialization +2. Data Filtering +3. Container Lazy-Loading +4. Inventory Validation +5. NPC Encounter Tracking +6. Unlock Tracking +7. Sync State Validation +8. Routing +9. Testing & Validation +10. Client Updates + +### Updated Plan +1. Data Model & Initialization *(verify starting items added)* +2. Data Filtering *(fix to keep lockType, add NPC tracking)* +3. Container Lazy-Loading *(fix ID resolution)* +4. Inventory Validation *(fix NPC itemsHeld structure)* +5. ~~NPC Tracking~~ *(merged into Phase 2)* +6. Unlock Tracking *(add transaction safety)* +7. Sync State Validation +8. Routing *(rename scenario → scenario_map)* +9. **Client Integration** *(NEW - critical missing phase)* +10. Testing & Validation + +--- + +## New TODO Items Added + +### Phase 1 +- [ ] 1.3 **UPDATE** `initialize_player_state` to add starting items to inventory + +### Phase 2 +- [ ] 2.1 Create `filter_requires_and_contents_recursive` (replaces current filter) +- [ ] 2.3 Rename `scenario` endpoint to `scenario_map` +- [ ] 2.4 Add NPC tracking to room endpoint + +### Phase 3 +- [ ] 3.4 Create `check_container_unlocked` helper +- [ ] 3.5 Create `filter_container_contents` helper +- [ ] 3.7 Fix route to include `to:` parameter + +### Phase 4 +- [ ] 4.5 **UPDATE** `find_npc_holding_item` (fix structure handling) +- [ ] 4.9 Test NPC itemsHeld validation + +### Phase 6 +- [ ] 6.1 **UPDATE** `unlock` endpoint with transaction safety +- [ ] 6.2 Test unlock operations are atomic + +### Phase 9 (NEW) +- [ ] 9.1 Update `addToInventory()` to validate with server +- [ ] 9.2 Update container minigame to lazy-load contents +- [ ] 9.3 Update scenario loading to use `scenario_map` +- [ ] 9.4 Add error handling for room access +- [ ] 9.5 Test client-server validation flow + +--- + +## Documentation Updates + +### Added Sections +1. **Client Integration Details** - Complete code examples for all client changes +2. **Breaking Changes** - Clear list of API changes requiring client updates +3. **Future Enhancements (Deferred)** - Items moved out of scope with rationale +4. **Notes & Design Decisions** - Key architectural choices documented + +### Updated Sections +1. **Data Structures Reference** - Clarified item structures +2. **Error Handling** - Added transaction error examples +3. **Testing Strategy** - Added NPC itemsHeld and atomic operation tests + +--- + +## Risk Mitigation + +### High Risk Issues Addressed ✅ +1. ✅ Client integration plan added (was completely missing) +2. ✅ NPC itemsHeld structure clarified and fixed +3. ✅ Transaction safety added to prevent state corruption +4. ✅ Container ID resolution ambiguity resolved + +### Medium Risk Issues Addressed ✅ +5. ✅ Race condition for start room access fixed +6. ✅ lockType filtering issue corrected +7. ✅ NPC tracking moved to transactional context + +--- + +## Estimated Timeline + +### Original Estimate +- **Total:** ~2 weeks + +### Updated Estimate +- **Phase 1-8 (Server):** 1.5 weeks +- **Phase 9 (Client Integration):** 3-4 days **[NEW]** +- **Phase 10 (Testing):** 2-3 days +- **Total:** ~3 weeks + +**Additional Time:** ~1 week added for client integration and comprehensive testing. + +--- + +## Next Steps + +1. **Review updated implementation plan** - Ensure all stakeholders understand breaking changes +2. **Update client code estimates** - Phase 9 is new, may need task breakdown +3. **Plan migration strategy** - Scenario endpoint rename needs coordinated deployment +4. **Begin implementation** - Start with Phase 1 (server-side changes) +5. **Test incrementally** - Don't wait until Phase 10 to test integration + +--- + +## Questions for Review + +1. ✅ **Scenario endpoint rename:** Is breaking change acceptable, or should we keep both endpoints temporarily? + - **Recommended:** Keep `/scenario` as deprecated alias for one release cycle + +2. ✅ **Client validation timeout:** How long should client wait for server validation before showing error? + - **Recommended:** 5 seconds with 3 retries (exponential backoff) + +3. ✅ **Container lazy-loading:** Should we show loading spinner, or fetch on minigame open? + - **Recommended:** Fetch on open with loading state (better UX) + +4. ✅ **Error messaging:** How detailed should client errors be (for player vs developer)? + - **Recommended:** Generic for player ("Cannot collect item"), detailed in console for devs + +--- + +## Conclusion + +The implementation plan has been significantly strengthened with: +- **Critical fixes** to filtering, validation, and transaction safety +- **Comprehensive client integration guidance** (previously missing) +- **Clearer scope boundaries** (what's in vs deferred vs out of scope) +- **Realistic timeline** accounting for client changes + +The plan is now **ready for implementation** with clear, actionable tasks and minimal ambiguity. diff --git a/planning_notes/validate_client_actions/planning_qs b/planning_notes/validate_client_actions/planning_qs new file mode 100644 index 0000000..a02d890 --- /dev/null +++ b/planning_notes/validate_client_actions/planning_qs @@ -0,0 +1,100 @@ +Question 1: Data Filtering Strategy +When should room/scenario data be filtered? + +B) At the controller level - Keep scenario_data as-is in the model, but filter in each endpoint response + +Question 2: What data should be hidden from the client? +Which of these should the client NOT see? + +A) requires (unlock requirements like keys, PINs, passwords) +C) contents (what's inside a locked container) + +A & C -- shouldn't see the contents, that should be loaded via an API request (we likely need similar to load room and load container) + +Question 3: Unlock Validation & Tracking +How should we track unlock attempts? + +A) No tracking - Just validate and unlock, don't store attempts + +Question 4: Inventory Operations +Should the server validate inventory operations? + +B) Validate items exist - Check that the item is actually in the room/scenario -- in the LOADED rooms + +Question 5: Unlock Method & Object Tracking +How should we track which objects/doors are unlocked? + +A) Permissive - Once unlocked, always unlocked (current approach) + +Question 6: Starting Inventory +Should starting inventory items be filtered out from room/scenario data? + +Don't make any changes related to starting inventory -- just add them to the player_state inventory. + + +Simple room data filtering -- include all data EXCEPT do filter the requires field, which is used by the server to validate answers and required items. + "requires": "password123", +All other data can be included in the data sent when loading rooms. + +The scenario end point should return a more filtered down information about rooms and connections. Check the codebase to make sure we provide everything that's needed. + + +Make a note of the data we plan to store in player_state: + + if is_postgresql + t.jsonb :player_state, null: false, default: { + currentRoom: nil, + unlockedRooms: [], + unlockedObjects: [], + inventory: [], + encounteredNPCs: [], + globalVariables: {}, + biometricSamples: [], + biometricUnlocks: [], + bluetoothDevices: [], + notes: [], + health: 100 + } + +Start by adding the starting room and starting inventory to +unlockedRooms +and +inventory + +When the player adds to inventory via client-side interaction, track what they've collected (and look at the unfiltered scenario data to check if the item exists in the scenario and is in a locked room or container object, if so, enforce that they can only collect those items that are defined in unlockedRooms) +When they enter a room with NPCs (including phone NPCs) then add the NPCs (by id) to encounteredNPCs + +currentRoom - client reported + + +Everytime the player unlocks something then store that in the player_state unlockedObjects / unlockedRooms + +Note that NPCs can define itemsHeld, and if so, and the NPC has been encountered, then allow those items to be added to inventory. + +Any further questions? + + +Questions/Clarifications I Have: +Items in locked containers: Should they be collectible ONLY after the container is unlocked? Or should we prevent them from appearing in room sprites until unlocked? + +Examples of locked containers are safes, locked briefcase, locked computer, etc, the player doesn't see the contents until after it's unlocked. The best code to look at is the container minigame, which is also used when interacting with NPCs that are giving the player an item they hold. + + +itemsHeld on NPCs: Should these items be added to the item pool when the NPC is encountered, or only become available after a specific conversation state? + +It's about how we enforce what's permitted, once the NPC has been encountered we accept that the client can claim to have the item they hold -- it's not as direct as that from a game play perspective, but just in terms of enforcing a minimum to prevent cheating, such as a client they just claims to have an item that is held by an NPC they haven't even met yet. + + +Biometric/Bluetooth tracking: These are tracked separately in player_state - should collecting/scanning these items auto-populate those arrays, or is that a separate operation? + +For now, just accept these as reported by the client. + +Notes field in player_state: Is this for collecting "notes" type objects, or general game notes/hints the player discovers? + +This field can be ignored or removed. + +Do we need a /games/:id/container/:container_id endpoint to lazy-load locked container contents, or can we include that in room data? + +Yes please. + +Create a actionable and detailed implementation-focused plan, with TODO list. Save into: planning_notes/validate_client_actions \ No newline at end of file diff --git a/planning_notes/validate_client_actions/review1.md b/planning_notes/validate_client_actions/review1.md new file mode 100644 index 0000000..91f4c10 --- /dev/null +++ b/planning_notes/validate_client_actions/review1.md @@ -0,0 +1,1362 @@ +# Implementation Plan Review: Client Action Validation & Data Filtering + +**Date:** November 21, 2025 +**Reviewer:** GitHub Copilot +**Status:** Recommendations for Success +**Confidence Level:** High + +--- + +## Executive Summary + +The implementation plan is **well-structured and security-focused**, with clear phasing and good separation of concerns. However, there are **9 critical areas** where improvements would significantly increase chances of successful execution and reduce technical debt. This review provides actionable recommendations organized by priority and implementation impact. + +--- + +## 1. CRITICAL ISSUES (Must Fix Before Starting) + +### Issue 1.1: Race Condition in Player State Initialization + +**Location:** Phase 1.2, Method `initialize_player_state!` + +**Problem:** +The current approach initializes player state on `before_create` callback, but: +- Multiple requests might create games simultaneously +- `initializedAt` check only prevents re-initialization within Rails, not across requests +- No transaction-level guarantee that state isn't initialized twice + +**Impact:** +- Duplicate inventory items +- Starting items collected twice on concurrent requests +- Inventory inconsistency + +**Recommendation:** +```ruby +# Instead of initializedAt flag, use database uniqueness constraint +# and idempotent initialization pattern: + +def initialize_player_state! + # Use safe navigation and merge pattern + base_state = { + 'currentRoom' => scenario_data['startRoom'], + 'unlockedRooms' => [scenario_data['startRoom']].uniq, + 'unlockedObjects' => [], + 'inventory' => scenario_data['startItemsInInventory']&.dup || [], + 'encounteredNPCs' => [], + 'globalVariables' => {}, + 'biometricSamples' => [], + 'biometricUnlocks' => [], + 'bluetoothDevices' => [], + 'notes' => [], + 'health' => 100, + 'initializedAt' => Time.current.iso8601 + } + + # Only set if completely empty (first time) + self.player_state = base_state if player_state.blank? || player_state.empty? +end +``` + +**Action Items:** +- [ ] Create migration to backfill player_state for existing games +- [ ] Add database index on `(id, player_state)` for quick state checks +- [ ] Test concurrent game creation with `AB` testing tool +- [ ] Document idempotent initialization in code comments + +--- + +### Issue 1.2: No Validation of Scenario Data Integrity + +**Location:** Phase 1, Entire initialization flow + +**Problem:** +- Plan assumes `scenario_data['startRoom']` exists, but doesn't validate +- Plan assumes `startItemsInInventory` is well-formed array, but doesn't validate +- No checks for circular room connections +- No checks for orphaned items (items in rooms that don't exist) + +**Impact:** +- Silent failures if scenario JSON is malformed +- Players stuck in non-existent rooms +- Items unreachable + +**Recommendation:** +Add validation layer before initialization: + +```ruby +class Game < ApplicationRecord + validates :scenario_data, presence: true + validate :scenario_data_integrity + + private + + def scenario_data_integrity + return if scenario_data.blank? + + errors.add(:scenario_data, 'missing startRoom') unless scenario_data['startRoom'].present? + errors.add(:scenario_data, 'startRoom not found') unless scenario_data['rooms']&.key?(scenario_data['startRoom']) + + # Validate startItemsInInventory structure + if scenario_data['startItemsInInventory'].present? + unless scenario_data['startItemsInInventory'].is_a?(Array) + errors.add(:scenario_data, 'startItemsInInventory must be an array') + end + end + + # Validate all rooms exist (connection integrity) + scenario_data['rooms']&.each do |room_id, room_data| + next unless room_data['connections'].present? + room_data['connections'].values.flatten.each do |connected_room| + unless scenario_data['rooms'].key?(connected_room) + errors.add(:scenario_data, "room #{room_id} connects to non-existent room #{connected_room}") + end + end + end + end +end +``` + +**Action Items:** +- [ ] Add scenario integrity validator before game creation +- [ ] Create integration tests with malformed scenarios +- [ ] Document expected scenario format as comments in validator +- [ ] Add error messages to help developers debug scenario issues + +--- + +### Issue 1.3: Incomplete Specification of `scenario_data` Structure + +**Location:** Phase 2-4, All filtering methods + +**Problem:** +- Plan doesn't define what fields exist at each level (rooms, objects, NPCs, items) +- Example shows `lockType`, `locked`, `requires`, but no complete list +- Plan mentions `itemsHeld` on NPCs but structure unclear +- No guidance on optional vs required fields + +**Impact:** +- Developers guess at field names +- Filtering logic breaks if fields are named differently +- Tests fail because mock data doesn't match real data structure + +**Recommendation:** +Create a formal scenario schema document before implementation: + +```ruby +# lib/break_escape/scenario_schema.rb +module BreakEscape + SCENARIO_SCHEMA = { + rooms: { + room_id: { + type: String, # e.g., "room_office" + connections: Hash, # { "north" => "office_2", "south" => "reception" } + locked: Boolean, # optional + lockType: String, # "key", "password", "pin", "biometric", optional + requires: String, # HIDDEN FROM CLIENT - answer/key_id + objects: Array, # Array of object definitions + npcs: Array # Array of NPC definitions + }, + objects: { + id: String, # unique within room + name: String, # display name + type: String, # "door", "container", "key", "notes", etc. + takeable: Boolean, + interactable: Boolean, # optional + locked: Boolean, # optional + lockType: String, # optional + requires: String, # HIDDEN - answer/solution + observations: String, # description shown to player + contents: Array, # HIDDEN UNTIL UNLOCKED - nested items + itemsHeld: Array # for NPC objects + }, + npcs: { + id: String, # unique NPC identifier + displayName: String, # shown to player + storyPath: String, # path to .ink or .json + npcType: String, # optional: "phone", "person", etc. + itemsHeld: Array # items NPC holds { type, id, name, ... } + } + } + } +end +``` + +**Action Items:** +- [ ] Create `lib/break_escape/scenario_schema.rb` with complete field definitions +- [ ] Add JSON Schema validator to verify scenarios match spec +- [ ] Document which fields are client-visible in inline comments +- [ ] Generate test fixture scenarios from schema definition + +--- + +## 2. DESIGN ISSUES (High Impact on Success) + +### Issue 2.1: `filter_contents_field` Method is Inefficient + +**Location:** Phase 2.3, controller filtering + +**Problem:** +- Recursively walks entire object tree for each filter operation +- Modifies objects in-place (using `.delete()`), which mutates original in memory +- No memoization of filtered results +- Same room data might be filtered multiple times per request + +**Impact:** +- Performance degrades with large scenarios (many rooms/objects) +- Memory pressure from deep copies +- Defensive copying needed to prevent mutations + +**Recommendation:** +Create dedicated filtering service class: + +```ruby +# app/services/break_escape/data_filter_service.rb +module BreakEscape + class DataFilterService + # Hide sensitive fields during room data exposure + SENSITIVE_FIELDS = ['requires', 'contents'].freeze + + def initialize(scenario_data) + @scenario_data = scenario_data + @filter_cache = {} + end + + # Filter object recursively, returning new object (immutable) + def filter_object(obj, hide_fields: SENSITIVE_FIELDS) + cache_key = "#{obj.object_id}_#{hide_fields.join(',')}" + return @filter_cache[cache_key] if @filter_cache.key?(cache_key) + + result = case obj + when Hash + obj.each_with_object({}) do |(key, value), acc| + next if hide_fields.include?(key) + acc[key] = filter_object(value, hide_fields: hide_fields) + end + when Array + obj.map { |item| filter_object(item, hide_fields: hide_fields) } + else + obj + end + + @filter_cache[cache_key] = result + end + + # Filter room data (remove 'requires' but keep 'lockType', 'locked') + def filtered_room_data(room_id) + room = @scenario_data['rooms']&.[](room_id) + return nil unless room + + filter_object(room, hide_fields: ['requires', 'contents']) + end + + # Filter contents of a specific container (remove nested 'requires') + def filtered_container_contents(container_id) + container = find_container_in_scenario(container_id) + return nil unless container + + (container['contents'] || []).map do |item| + filter_object(item, hide_fields: ['requires', 'contents']) + end + end + end +end +``` + +**Action Items:** +- [ ] Create `app/services/break_escape/data_filter_service.rb` +- [ ] Add caching strategy for repeated filters +- [ ] Add unit tests for filter performance +- [ ] Update controller to use service instead of inline filtering +- [ ] Add instrumentation to measure filter overhead + +--- + +### Issue 2.2: Validation Helper Methods Are Complex and Scattered + +**Location:** Phase 4, Inventory validation, multiple helper methods + +**Problem:** +- `find_item_in_scenario`, `find_item_container`, `find_item_room`, `find_npc_holding_item` all do similar traversals +- Each method might have different traversal logic (depth-first vs breadth-first) +- No shared traversal pattern +- Code duplication increases bug surface area + +**Impact:** +- Bugs in one finder don't get fixed in others +- Hard to maintain consistency +- Performance suffers from repeated traversals + +**Recommendation:** +Create a scenario traversal service: + +```ruby +# app/services/break_escape/scenario_traverser.rb +module BreakEscape + class ScenarioTraverser + def initialize(scenario_data) + @scenario_data = scenario_data + @index = {} # Cache of item/container/NPC lookups + build_index + end + + # Find item by type and ID anywhere in scenario + def find_item(item_type, item_id) + @index[:items]["#{item_type}:#{item_id}"] + end + + # Find which container holds an item + def find_containing_container(item_type, item_id) + item = find_item(item_type, item_id) + return nil unless item + @index[:container_map]["#{item_type}:#{item_id}"] + end + + # Find which room contains an item + def find_containing_room(item_type, item_id) + item = find_item(item_type, item_id) + return nil unless item + @index[:room_map]["#{item_type}:#{item_id}"] + end + + # Find NPC holding an item + def find_npc_holding(item_type, item_id) + @index[:npc_holdings]["#{item_type}:#{item_id}"] + end + + # Find container by ID + def find_container(container_id) + @index[:containers][container_id] + end + + # Find NPC by ID + def find_npc(npc_id) + @index[:npcs][npc_id] + end + + private + + def build_index + @index = { + items: {}, # "type:id" => item_data + containers: {}, # "container_id" => container_data + npcs: {}, # "npc_id" => npc_data + container_map: {}, # "type:id" => container_id (what container holds this item) + room_map: {}, # "type:id" => room_id (what room contains this item) + npc_holdings: {} # "type:id" => npc_id (what NPC holds this) + } + + @scenario_data['rooms']&.each do |room_id, room_data| + index_room_objects(room_id, room_data['objects'], container_id: nil) + index_npcs(room_id, room_data['npcs']) + end + end + + def index_room_objects(room_id, objects, container_id: nil) + return unless objects + + objects.each do |obj| + # Index the object itself + key = "#{obj['type']}:#{obj['id']}" + @index[:items][key] = obj + @index[:room_map][key] = room_id + + # If inside a container, record that relationship + @index[:container_map][key] = container_id if container_id + + # If this object is a container, index it + if obj['contents'].present? + @index[:containers][obj['id']] = obj + # Recursively index nested contents + index_room_objects(room_id, obj['contents'], container_id: obj['id']) + end + end + end + + def index_npcs(room_id, npcs) + return unless npcs + + npcs.each do |npc| + @index[:npcs][npc['id']] = npc + + # Index items held by NPC + next unless npc['itemsHeld'].present? + npc['itemsHeld'].each do |item| + key = "#{item['type']}:#{item['id']}" + @index[:npc_holdings][key] = npc['id'] + end + end + end + end +end +``` + +**Action Items:** +- [ ] Create `app/services/break_escape/scenario_traverser.rb` +- [ ] Add unit tests for each finder method +- [ ] Update inventory validation to use traverser +- [ ] Benchmark performance improvement (should eliminate duplicate traversals) +- [ ] Add memoization for scenario_traverser instances per game + +--- + +### Issue 2.3: Missing Container Nested Contents Handling + +**Location:** Phase 3, Container endpoint + +**Problem:** +- Plan says "containers can be nested" (containers inside containers) +- But `filter_contents_field` method is incomplete (pseudocode) +- No handling for deeply nested containers +- Unclear: Can items inside nested containers be collected? + +**Impact:** +- Nested container structure breaks at runtime +- Items in nested containers unreachable +- Filtering logic incomplete + +**Recommendation:** +Extend container traversal to handle nesting: + +```ruby +# In ScenarioTraverser class: +def container_chain(container_id) + # Returns all containers in nesting order + # e.g., [safe_id, inner_box_id, final_container_id] + chain = [] + current_id = container_id + + while current_id + container = @index[:containers][current_id] + return nil unless container # Safety check: container not found + chain.unshift(current_id) + current_id = @index[:container_map]["#{container['type']}:#{current_id}"] + end + + chain +end + +def is_container_accessible?(container_id, unlocked_objects) + # Container only accessible if ALL parent containers in chain are unlocked + chain = container_chain(container_id) + return false unless chain + + chain.all? { |cid| unlocked_objects.include?(cid) } +end +``` + +**Action Items:** +- [ ] Add container chain tracking to traverser +- [ ] Add accessibility check for nested containers +- [ ] Update container endpoint to validate entire chain is unlocked +- [ ] Add tests for nested container scenarios +- [ ] Document nesting rules in scenario schema + +--- + +## 3. TESTING & VALIDATION ISSUES + +### Issue 3.1: Insufficient Test Coverage Specification + +**Location:** Phase 9, Testing & Validation (very high level) + +**Problem:** +- Phase 9 lists test names but no test cases/assertions +- No coverage requirements specified +- No "happy path" vs "unhappy path" breakdown +- No edge case identification + +**Impact:** +- Tests will be incomplete or miss critical paths +- Integration test failure doesn't help developers debug +- No regression test foundation for future features + +**Recommendation:** +Create detailed test matrix: + +```ruby +# test/integration/client_action_validation_test.rb + +describe "Client Action Validation" do + + describe "Room Access Control" do + test "player can access starting room without unlock" do + # Game created, player_state['unlockedRooms'] contains start room + # Request room endpoint for startRoom → 200 OK + end + + test "player cannot access locked room without unlock" do + # Request room endpoint for locked room → 403 Forbidden + end + + test "player can access room after unlock" do + # Unlock room → Update unlockedRooms + # Request room endpoint → 200 OK + end + end + + describe "Data Filtering" do + test "room response does not include 'requires' field" do + # Check all objects in room response + # Assert no 'requires' key exists at any level + end + + test "room response does not include 'contents' of locked containers" do + # Check all containers in room response + # Assert 'contents' key does not exist if locked + end + + test "room response includes 'lockType' and 'locked' status" do + # Verify these fields present for client UI + end + end + + describe "Container Lazy Loading" do + test "container contents not accessible before unlock" do + # GET /container/safe_1 before unlock → 403 Forbidden + end + + test "container contents accessible after unlock" do + # Unlock container → 200 OK with contents + end + + test "nested containers require full chain unlock" do + # Scenario with: safe → inner_box → item + # Each must be unlocked separately + end + end + + describe "Inventory Validation" do + test "item cannot be collected from locked room" do + # POST /inventory { add, item } for item in locked room + # → Error: "Room not unlocked" + end + + test "item cannot be collected from locked container" do + # Item in locked container + # → Error: "Container not unlocked" + end + + test "NPC-held item cannot be collected before encounter" do + # NPC in room not visited yet + # → Error: "NPC not encountered" + end + + test "item can be collected from accessible location" do + # Room unlocked, container unlocked or not in container, NPC encountered + # → Success, inventory updated + end + end + + describe "NPC Encounter Tracking" do + test "NPC added to encounteredNPCs when room loaded" do + # Load room with NPC + # Check player_state['encounteredNPCs'] includes NPC + end + + test "NPC not in encounteredNPCs if room never visited" do + # Scenario with NPC in locked room + # encounteredNPCs should not include that NPC + end + end + + describe "Unlock Tracking" do + test "room added to unlockedRooms after successful unlock" do + # Valid unlock attempt + # unlockedRooms includes the room + end + + test "container added to unlockedObjects after unlock" do + # Valid container unlock + # unlockedObjects includes the container + end + end + + describe "Edge Cases" do + test "simultaneous unlock attempts don't duplicate" do + # Two concurrent unlock requests for same door + # unlockedRooms includes room only once + end + + test "inventory doesn't accept duplicates" do + # Collect item, remove it, collect again + # Inventory correctly updated each time + end + + test "player state persists across requests" do + # Create game, unlock room, fetch game + # player_state still contains unlock + end + end +end +``` + +**Action Items:** +- [ ] Create comprehensive test matrix before implementation +- [ ] Assign each test to specific phase where it should pass +- [ ] Define acceptance criteria for each test +- [ ] Create fixtures (scenarios) for each test case +- [ ] Add performance benchmarks (response time, memory usage) + +--- + +### Issue 3.2: Missing Rollback & Recovery Strategy + +**Location:** All phases, implicit assumption things work + +**Problem:** +- Plan doesn't mention what happens if a phase fails +- No specification of backwards compatibility +- No plan for debugging failed unlocks +- No admin tools for resetting game state + +**Impact:** +- If implementation partially succeeds, hard to debug +- Players can't recover from bugs +- No way to fix inconsistent state in production + +**Recommendation:** +Add Phase 11: Rollback & Recovery Tools + +```ruby +# app/services/break_escape/game_state_manager.rb +module BreakEscape + class GameStateManager + def initialize(game) + @game = game + end + + # Snapshot current state for rollback + def create_snapshot + { + timestamp: Time.current, + snapshot_id: SecureRandom.uuid, + player_state: @game.player_state.deep_dup + } + end + + # Restore from snapshot + def restore_snapshot(snapshot_id) + # Find and restore snapshot + snapshot = @game.snapshots.find_by(snapshot_id: snapshot_id) + raise "Snapshot not found: #{snapshot_id}" unless snapshot + + @game.player_state = snapshot.data['player_state'] + @game.save! + end + + # Reset game to initial state + def reset_to_start + @game.initialize_player_state! + end + + # Validate state consistency + def validate_consistency + errors = [] + + # Check all unlockedRooms exist in scenario + @game.player_state['unlockedRooms'].each do |room_id| + unless @game.scenario_data['rooms'].key?(room_id) + errors << "Room #{room_id} in unlockedRooms but not in scenario" + end + end + + # Check all inventory items exist in scenario or are starting items + @game.player_state['inventory'].each do |item| + # Item should be findable in scenario + # OR be a starting item + end + + # Check NPC encounters are in scenario + # Check container unlocks are in scenario + + errors + end + end +end +``` + +**Action Items:** +- [ ] Add Phase 11: Recovery & Debugging Tools to plan +- [ ] Create `app/services/break_escape/game_state_manager.rb` +- [ ] Add state validation checks before responding to client requests +- [ ] Create admin endpoint to inspect game state +- [ ] Create admin endpoint to reset specific games +- [ ] Add logging for state changes + +--- + +## 4. IMPLEMENTATION SEQUENCING ISSUES + +### Issue 4.1: Wrong Phase Dependencies + +**Location:** Implementation TODO List, Phase ordering + +**Problem:** +- Phase 6 (Unlock Tracking) depends on Phase 4 (Inventory Validation) +- But plan lists them in sequence as if Phase 6 is independent +- Phase 7 (Sync State Validation) depends on Phase 1 (Initialization) +- No explicit dependency graph + +**Impact:** +- Developer might implement phases out of order and get stuck +- Unclear which tests should pass after each phase + +**Recommendation:** +Create explicit dependency diagram: + +``` +Phase 1: Data Model & Initialization +├─ Phase 2: Data Filtering +│ ├─ Phase 3: Container Lazy-Loading +│ └─ Phase 6: Unlock Validation & Tracking +├─ Phase 4: Inventory Validation +│ └─ Phase 5: NPC Encounter Tracking +└─ Phase 7: Sync State Validation + +Phase 8: Routing (can start once controllers exist) +Phase 9: Testing (runs continuously after each phase) +Phase 10: Client Updates (after server complete) +Phase 11: Recovery Tools (after Phase 1-7 complete) +``` + +**Recommended Implementation Order:** +1. Phase 1 ✓ (prerequisite for everything) +2. Phase 2 ✓ (data filtering needed by Phase 6) +3. Phase 8 ✓ (add routes as you go) +4. Phase 3 ✓ (container endpoint) +5. Phase 6 ✓ (unlock tracking) +6. Phase 5 ✓ (NPC encounter tracking) +7. Phase 4 ✓ (inventory validation) +8. Phase 7 ✓ (sync state validation) +9. Phase 11 ✓ (recovery tools) +10. Phase 9 ✓ (comprehensive testing) +11. Phase 10 (client updates) + +**Action Items:** +- [ ] Create dependency graph in planning document +- [ ] Identify any additional dependencies not listed +- [ ] Reorder phases based on dependencies +- [ ] Create "ready to start" checklist for each phase + +--- + +### Issue 4.2: Client Updates Are Last (Should Be Earlier for Testing) + +**Location:** Phase 10, Client Updates + +**Problem:** +- Client updates are Phase 10, last +- But without client changes, server endpoints can't be tested +- Manual integration testing required with curl/Postman before client ready +- Creates bottleneck: server done but can't validate until client updated + +**Impact:** +- Hard to find bugs (can't end-to-end test without client) +- Delay in finding integration issues +- Client developers blocked waiting for server + +**Recommendation:** +Split client updates into two phases: + +```markdown +## Phase 8B: Client Integration (After Phase 3, Before Phase 4) +- Create stub client endpoints that call new server endpoints +- Add error handling for new response format +- Test with curl/Postman until working +- Update game.js to use scenario_map instead of full scenario +- Update room endpoint calls with filtering awareness + +## Phase 10: Client Polish (After Phase 9) +- Add UI for error states +- Add loading indicators +- Optimize performance +- Add user feedback for validation errors +``` + +**Action Items:** +- [ ] Create Phase 8B: Client Integration +- [ ] Add stub client implementation before server complete +- [ ] Create curl/Postman test suite for each endpoint +- [ ] Parallel development: Server Phase 4-7 while Client Phase 8B ongoing +- [ ] Move UI polish to Phase 10 (after all features working) + +--- + +## 5. DOCUMENTATION & COMMUNICATION GAPS + +### Issue 5.1: No Specification of Error Response Formats + +**Location:** Error Handling section, incomplete examples + +**Problem:** +- Plan shows a few error examples but inconsistent format +- "Missing room_id parameter" vs "Container not unlocked" use different patterns +- No specification of HTTP status codes for different error types +- No error code system for client to handle programmatically + +**Impact:** +- Client code might not correctly handle errors +- Hard to test all error paths +- Developers guess at response format + +**Recommendation:** +Formalize error response schema: + +```ruby +# Error Response Format: +{ + success: false, + error: { + code: "ERROR_CODE", # Machine-readable error classification + message: "Human readable", # User-facing message + details: {} # Additional context (optional) + } +} + +# HTTP Status Codes: +# 400 - Bad Request: Missing/invalid parameters (developer error) +# 403 - Forbidden: Permission denied (gameplay state issue) +# 404 - Not Found: Resource not found +# 422 - Unprocessable Entity: Validation failed (gameplay logic) +# 500 - Internal Server Error: Unexpected error (bug) + +# Error Codes: +MISSING_PARAMETER = "MISSING_PARAMETER" +ROOM_NOT_FOUND = "ROOM_NOT_FOUND" +ROOM_NOT_ACCESSIBLE = "ROOM_NOT_ACCESSIBLE" +CONTAINER_NOT_FOUND = "CONTAINER_NOT_FOUND" +CONTAINER_NOT_UNLOCKED = "CONTAINER_NOT_UNLOCKED" +ITEM_NOT_FOUND = "ITEM_NOT_FOUND" +ITEM_NOT_TAKEABLE = "ITEM_NOT_TAKEABLE" +ITEM_LOCATION_LOCKED = "ITEM_LOCATION_LOCKED" # Room or container +NPC_NOT_ENCOUNTERED = "NPC_NOT_ENCOUNTERED" +INVALID_UNLOCK_ATTEMPT = "INVALID_UNLOCK_ATTEMPT" +``` + +**Action Items:** +- [ ] Create error code constants in `lib/break_escape/error_codes.rb` +- [ ] Create error response builder method in ApplicationController +- [ ] Update all error responses to use consistent format +- [ ] Document error codes in API documentation +- [ ] Create error handling guide for client developers + +--- + +### Issue 5.2: No API Documentation or Examples + +**Location:** Implementation plan, missing entirely + +**Problem:** +- Plan doesn't specify HTTP verbs, parameter names exactly as they'll be +- Plan shows pseudocode but not exact API contracts +- Client developers won't know what to call +- No spec for developers following later + +**Impact:** +- Integration painful and error-prone +- Clients might call endpoints incorrectly +- Hard to onboard new developers + +**Recommendation:** +Create OpenAPI/Swagger spec: + +```yaml +# Break Escape API - Validation & Filtering Endpoints + +/games/{id}/room/{room_id}: + get: + summary: Load room data with filtering + parameters: + - name: id + in: path + required: true + schema: + type: string + - name: room_id + in: path + required: true + schema: + type: string + responses: + 200: + description: Room data (requires and contents fields hidden) + content: + application/json: + schema: + type: object + properties: + room_id: { type: string } + room: { type: object } + 403: + description: Room not accessible + 404: + description: Room not found + +/games/{id}/container/{container_id}: + get: + summary: Load locked container contents + parameters: + - name: id + in: path + required: true + - name: container_id + in: path + required: true + responses: + 200: + description: Container contents with filtering applied + 403: + description: Container not unlocked + 404: + description: Container not found + +/games/{id}/scenario_map: + get: + summary: Get minimal scenario layout (no secrets exposed) + responses: + 200: + description: Room connections and accessibility + +/games/{id}/unlock: + post: + parameters: + - name: targetType + in: body + required: true + schema: + enum: [door, object] + - name: targetId + in: body + required: true + - name: attempt + in: body + required: true + - name: method + in: body + required: true + responses: + 200: + description: Unlock successful + 422: + description: Invalid attempt + +/games/{id}/inventory: + post: + parameters: + - name: actionType + in: body + enum: [add, remove] + - name: item + in: body + schema: + type: object + required: [type, id] + responses: + 200: + description: Inventory updated + 422: + description: Item not collectible +``` + +**Action Items:** +- [ ] Create OpenAPI/Swagger spec in `docs/api.openapi.yaml` +- [ ] Generate interactive API docs from spec +- [ ] Add code examples for each endpoint in Ruby/JavaScript +- [ ] Create API testing guide (curl examples) +- [ ] Update README with API overview + +--- + +## 6. PERFORMANCE & SCALABILITY ISSUES + +### Issue 6.1: No Consideration of Large Scenarios + +**Location:** All phases, implicit assumption scenarios are small + +**Problem:** +- Plan uses linear search through all rooms/objects +- No pagination or lazy-loading of scenario data +- Scenarios could have thousands of objects +- Every room request triggers full traversal + +**Impact:** +- Slow response times for large scenarios +- High CPU/memory on server +- Client perceived as sluggish + +**Recommendation:** +Add performance considerations to Phase 2: + +```ruby +# app/services/break_escape/scenario_cache.rb +module BreakEscape + class ScenarioCache + # Cache filtered room data for performance + def initialize(game) + @game = game + @room_cache = {} + @filter_service = DataFilterService.new(game.scenario_data) + end + + def filtered_room(room_id) + @room_cache[room_id] ||= begin + @filter_service.filtered_room_data(room_id) + end + end + + def clear_cache + @room_cache.clear + end + + def invalidate_room(room_id) + @room_cache.delete(room_id) + end + end +end + +# Measure and optimize: +# - Response time: Room endpoint should respond < 100ms +# - Scenario size: Support scenarios up to 100 rooms, 1000 objects +# - Memory: Scenario data should fit in memory (not stream from disk) +``` + +**Action Items:** +- [ ] Add caching layer for filtered room data +- [ ] Profile room endpoint response time with large scenario +- [ ] Set performance targets: < 100ms per room request +- [ ] Add N+1 query detection for database calls +- [ ] Test with scenarios of varying sizes (10, 100, 1000 objects) + +--- + +### Issue 6.2: Inventory Validation Performance + +**Location:** Phase 4, multiple find operations + +**Problem:** +- Current approach does 4+ full scenario traversals per inventory add +- `find_item_in_scenario`, `find_item_container`, `find_item_room`, `find_npc_holding_item` +- Each traversal walks entire object tree + +**Impact:** +- Inventory operations slow (multiple seconds for large scenario) +- Scales poorly: inventory add time grows linearly with scenario size + +**Recommendation:** +Use ScenarioTraverser index (Issue 2.2 already addresses this) + +```ruby +# Measurement: +# Current: O(n * m) where n = scenarios traversals, m = objects +# With index: O(1) lookup after O(m) index build +# Improvement: Build index once per game, reuse for all operations +``` + +**Action Items:** +- [ ] Implement ScenarioTraverser index (see Issue 2.2) +- [ ] Benchmark inventory add: before/after +- [ ] Target: Inventory add < 50ms +- [ ] Add index build time to game initialization +- [ ] Cache traverser instance in @game object + +--- + +## 7. SECURITY REVIEW + +### Issue 7.1: Missing Authorization Checks + +**Location:** All controller actions + +**Problem:** +- Plan shows `authorize @game if defined?(Pundit)` but conditional +- Not all endpoints might have this +- No specification of what authorization means +- Could allow one player to access another player's game + +**Impact:** +- Player A could unlock rooms in Player B's game +- Data leak between players +- Game state modification by unauthorized user + +**Recommendation:** +Strengthen authorization: + +```ruby +# app/policies/break_escape/game_policy.rb +module BreakEscape + class GamePolicy + attr_reader :user, :game + + def initialize(user, game) + @user = user + @game = game + end + + def show? + game.player == user # Only owner can access + end + + def room? + show? # Same auth as show + end + + def container? + show? + end + + def unlock? + show? + end + + def inventory? + show? + end + + def sync_state? + show? + end + end +end + +# In controller, always authorize: +module BreakEscape + class GamesController < ApplicationController + before_action :set_game, only: [:show, :scenario, :ink, :room, :container, :sync_state, :unlock, :inventory, :scenario_map] + before_action :authorize_game + + private + + def authorize_game + authorize @game + end + end +end +``` + +**Action Items:** +- [ ] Create `app/policies/break_escape/game_policy.rb` +- [ ] Make authorization non-conditional (always required) +- [ ] Test authorization on each endpoint +- [ ] Add integration tests for authorization failures +- [ ] Document who can perform each action + +--- + +### Issue 7.2: Data Filtering Completeness Not Verified + +**Location:** All filtering methods + +**Problem:** +- Plan assumes filter methods catch all instances of hidden fields +- But what if scenario has new field type added later? +- What if nested structure is deeper than expected? +- No way to verify filtering is complete + +**Impact:** +- Accidentally expose "requires" field if code changes +- Security regression from future maintenance + +**Recommendation:** +Add verification layer: + +```ruby +# app/services/break_escape/data_verifier.rb +module BreakEscape + class DataVerifier + FORBIDDEN_FIELDS = ['requires'].freeze + + def self.verify_filtered_data(data) + errors = [] + verify_recursive(data, errors) + errors + end + + private + + def self.verify_recursive(obj, errors, path = "root") + case obj + when Hash + obj.each do |key, value| + if FORBIDDEN_FIELDS.include?(key) + errors << "Found forbidden field '#{key}' at #{path}" + end + verify_recursive(value, errors, "#{path}.#{key}") + end + when Array + obj.each_with_index do |item, idx| + verify_recursive(item, errors, "#{path}[#{idx}]") + end + end + end + end +end + +# Use in tests: +test "room data never exposes requires field" do + response = get "/games/#{game.id}/room/office" + room_data = JSON.parse(response.body)['room'] + + errors = DataVerifier.verify_filtered_data(room_data) + assert errors.empty?, "Found forbidden fields: #{errors.join(', ')}" +end +``` + +**Action Items:** +- [ ] Create `app/services/break_escape/data_verifier.rb` +- [ ] Add verification to all filtered data responses +- [ ] Create test helper to assert no forbidden fields +- [ ] Run verification in tests automatically +- [ ] Add flag to log warnings in development if forbidden fields found + +--- + +## 8. MINOR ISSUES (Nice-to-Have Improvements) + +### Issue 8.1: Debugging Visibility + +**Problem:** +- No logging of filtering/validation operations +- Hard to debug "why can't player access room?" +- Hard to trace validation decisions + +**Recommendation:** +```ruby +class DataFilterService + def filter_object(obj, hide_fields: SENSITIVE_FIELDS) + Rails.logger.debug "[BreakEscape] Filtering object #{obj['id']} with fields: #{hide_fields}" + # ... rest of method + end +end + +class Game < ApplicationRecord + def validate_unlock(...) + Rails.logger.info "[BreakEscape] Validating unlock for #{target_id}" + # ... rest of method + end +end +``` + +**Action Items:** +- [ ] Add structured logging using `Rails.logger.tagged` +- [ ] Create log format consistent with other endpoints +- [ ] Add instrumentation gem for performance monitoring +- [ ] Document logging format for debugging guide + +--- + +### Issue 8.2: Consistency Between Endpoints + +**Problem:** +- Room endpoint might filter differently than container endpoint +- Scenario map might expose different fields than room +- No specification that all are consistent + +**Recommendation:** +```ruby +# Create shared constants +module BreakEscape + module DataFiltering + # Fields always hidden from client (solutions) + ALWAYS_HIDDEN = ['requires'].freeze + + # Fields hidden until unlocked (contents) + HIDDEN_UNTIL_UNLOCKED = ['contents'].freeze + + # Fields always visible to client (needed for UI) + ALWAYS_VISIBLE = ['lockType', 'locked', 'observations', 'id', 'name', 'type'].freeze + end +end +``` + +**Action Items:** +- [ ] Create `lib/break_escape/data_filtering.rb` with shared constants +- [ ] Use constants in all filtering methods +- [ ] Document field visibility policy in comments +- [ ] Add assertions to tests that verify consistency + +--- + +## SUMMARY: Risk & Mitigation Matrix + +| Risk | Probability | Impact | Mitigation | Priority | +|------|-------------|--------|-----------|----------| +| Race condition in initialization | High | Medium | Add idempotent pattern (Issue 1.1) | CRITICAL | +| Malformed scenario breaks game | Medium | High | Add validation (Issue 1.2) | CRITICAL | +| Incomplete scenario schema | High | Medium | Create formal schema (Issue 1.3) | CRITICAL | +| Performance degrades with large scenarios | Medium | High | Add caching + traverser index (Issues 6.1, 6.2, 2.1) | HIGH | +| Authorization bypass | Low | Critical | Strengthen auth checks (Issue 7.1) | HIGH | +| Accidentally expose secrets in future | Low | Critical | Add verification layer (Issue 7.2) | HIGH | +| Nested containers break | Medium | Medium | Extend traversal (Issue 2.3) | HIGH | +| Incomplete test coverage | High | Medium | Create test matrix (Issue 3.1) | HIGH | +| Player stuck in failed state | Medium | Medium | Add recovery tools (Issue 3.2) | MEDIUM | +| Wrong phase dependencies | Low | Medium | Create dependency graph (Issue 4.1) | MEDIUM | +| Client blocked on server | Low | Medium | Parallel Phase 8B (Issue 4.2) | MEDIUM | +| Integration testing difficult | Medium | Low | Create curl/Postman suite (Issue 4.2) | MEDIUM | +| API unclear to developers | Low | Low | Create OpenAPI spec (Issue 5.2) | LOW | +| Debugging difficult | Low | Low | Add logging (Issue 8.1) | LOW | + +--- + +## RECOMMENDED PRIORITY: Critical Path First + +**Must Fix Before Starting:** +1. Issue 1.1: Race condition handling +2. Issue 1.2: Scenario validation +3. Issue 1.3: Scenario schema documentation + +**Must Fix During Design Phase:** +4. Issue 2.1: Efficient filtering service +5. Issue 2.2: Scenario traverser with index +6. Issue 3.1: Comprehensive test matrix +7. Issue 4.1: Dependency graph + +**Can Parallelize:** +8. Issue 2.3: Nested container handling (parallel with Phase 3) +9. Issue 4.2: Phase 8B client integration (parallel with server) +10. Issue 7.1: Authorization policy (parallel with controller work) + +**Add After MVP:** +11. Issue 3.2: Recovery tools (Phase 11) +12. Issue 6.1: Performance optimization +13. Issue 7.2: Data verification +14. Issue 5.2: OpenAPI documentation +15. Issue 8.1: Enhanced logging + +--- + +## Next Steps + +1. **Review with team** - Present this review to development team +2. **Prioritize** - Decide which issues to address immediately vs. defer +3. **Update plan** - Incorporate recommendations into IMPLEMENTATION_PLAN.md +4. **Create PR** - Implement high-priority fixes to game.rb and add data_filter_service.rb +5. **Begin Phase 1** - Start with data model improvements and validation + +--- + +## Appendix: Quick Reference Checklist + +### Before Any Code Changes: +- [ ] Create scenario schema document +- [ ] Add scenario data validation +- [ ] Make authorization non-conditional +- [ ] Create dependency graph + +### During Implementation: +- [ ] Use DataFilterService for filtering +- [ ] Use ScenarioTraverser for lookups +- [ ] Create test matrix before phase +- [ ] Run DataVerifier on all responses +- [ ] Add error code constants and consistent response format + +### Before Calling Complete: +- [ ] Zero authorization bypasses +- [ ] No forbidden fields in any response +- [ ] Performance < 100ms per room request +- [ ] All test matrix items passing +- [ ] Recovery tools available for admins +- [ ] API documentation complete + +--- + +**End of Review** diff --git a/planning_notes/validate_client_actions/review2.md b/planning_notes/validate_client_actions/review2.md new file mode 100644 index 0000000..81ad709 --- /dev/null +++ b/planning_notes/validate_client_actions/review2.md @@ -0,0 +1,1212 @@ +# Implementation Review #2: Server-Side Validation & Data Filtering +**Date:** November 21, 2025 +**Status:** Pre-Implementation Review +**Scope:** Critical analysis of implementation plan with actionable recommendations + +--- + +## Executive Summary + +The implementation plan for server-side validation is **architecturally sound** but has several **critical gaps** that will cause failures during implementation. This review identifies 25+ specific issues and provides concrete solutions for each. + +**Overall Assessment:** 🟡 **Needs Refinement Before Implementation** + +**Key Concerns:** +1. ✅ **Strengths:** Good separation of concerns, clear validation logic, proper filtering approach +2. ⚠️ **Gaps:** Missing client-side API integration, incomplete NPC itemsHeld handling, no container endpoint client code +3. 🔴 **Risks:** No transaction/rollback strategy, missing error recovery, client-server sync issues + +--- + +## Critical Issues & Solutions + +### 🔴 CRITICAL #1: No Client-Side API Integration Plan + +**Problem:** +The implementation plan focuses entirely on server changes but provides **zero guidance** on updating the client to call the new endpoints. Client code currently: +- Calls `addToInventory()` locally without server validation +- Never checks with server if items are collectible +- Has no code to call the new `/container/:container_id` endpoint +- Doesn't handle 403 Forbidden responses for locked rooms + +**Impact:** Even after server implementation, the game won't actually validate anything because the client bypasses the server. + +**Solution:** + +#### Phase 11: Client-Side Integration (ADD TO PLAN) + +**File:** `public/break_escape/js/systems/inventory.js` + +Update `addToInventory()` to validate with server: + +```javascript +export async function addToInventory(sprite) { + if (!sprite || !sprite.scenarioData) { + console.warn('Invalid sprite for inventory'); + return false; + } + + // Check if already in inventory (local check first) + const itemIdentifier = createItemIdentifier(sprite.scenarioData); + const isAlreadyInInventory = window.inventory.items.some(item => + item && createItemIdentifier(item.scenarioData) === itemIdentifier + ); + + if (isAlreadyInInventory) { + console.log(`Item ${itemIdentifier} is already in inventory`); + return false; + } + + // NEW: Validate with server before adding + const gameId = window.gameId; + if (gameId) { + try { + const response = await fetch(`/break_escape/games/${gameId}/inventory`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + action_type: 'add', + item: sprite.scenarioData + }) + }); + + const result = await response.json(); + + if (!result.success) { + // Server rejected - show error to player + window.gameAlert(result.message || 'Cannot collect this item', 'error', 'Invalid Action', 3000); + return false; + } + + // Server accepted - continue with local inventory update + console.log('Server validated item collection:', result); + } catch (error) { + console.error('Failed to validate inventory with server:', error); + // Fail closed - don't add if server can't validate + window.gameAlert('Network error - please try again', 'error', 'Error', 3000); + return false; + } + } + + // ... rest of existing addToInventory logic ... +} +``` + +**File:** `public/break_escape/js/minigames/container/container-minigame.js` + +Update container opening to fetch contents from server: + +```javascript +// In ContainerMinigame.init() or wherever contents are loaded +async loadContainerContents() { + const gameId = window.gameId; + const containerId = this.containerItem.scenarioData.id || this.containerItem.objectId; + + if (!gameId) { + console.error('No gameId available for container loading'); + return []; + } + + try { + const response = await fetch(`/break_escape/games/${gameId}/container/${containerId}`); + + if (!response.ok) { + if (response.status === 403) { + window.gameAlert('Container is locked', 'error', 'Locked', 2000); + this.complete(false); + return []; + } + throw new Error(`HTTP ${response.status}`); + } + + const data = await response.json(); + return data.contents || []; + } catch (error) { + console.error('Failed to load container contents:', error); + window.gameAlert('Could not load container contents', 'error', 'Error', 3000); + return []; + } +} +``` + +--- + +### 🔴 CRITICAL #2: Missing Player State Initialization on Game Creation + +**Problem:** +The plan says "call `initialize_player_state!` in GamesController#create" but **doesn't show the actual implementation** in the controller. The `before_create` callback in the model already handles this, so adding it to the controller would **duplicate** the initialization. + +**Current State:** +```ruby +# app/models/break_escape/game.rb (lines 19-20) +before_create :generate_scenario_data +before_create :initialize_player_state # ✅ Already exists! +``` + +**Solution:** + +The model callback **already handles initialization correctly**. Remove this from the TODO list: +- ~~1.3 Call initialization in GamesController#create~~ ✅ Already done via callback + +However, we should **verify** that starting items are added correctly: + +```ruby +# app/models/break_escape/game.rb +def initialize_player_state + self.player_state ||= {} + self.player_state['currentRoom'] ||= scenario_data['startRoom'] + self.player_state['unlockedRooms'] ||= [scenario_data['startRoom']] + self.player_state['unlockedObjects'] ||= [] + self.player_state['inventory'] ||= [] + + # ADD THIS: Initialize starting items from scenario + if scenario_data['startItemsInInventory'].present? + scenario_data['startItemsInInventory'].each do |item| + self.player_state['inventory'] << item.deep_dup + end + end + + self.player_state['encounteredNPCs'] ||= [] + self.player_state['globalVariables'] ||= {} + self.player_state['biometricSamples'] ||= [] + self.player_state['biometricUnlocks'] ||= [] + self.player_state['bluetoothDevices'] ||= [] + self.player_state['notes'] ||= [] + self.player_state['health'] ||= 100 +end +``` + +--- + +### 🔴 CRITICAL #3: NPC itemsHeld Structure Unclear + +**Problem:** +The plan mentions checking `npc['itemsHeld']` but the actual structure in scenario files is unclear. Looking at the codebase: + +```javascript +// public/break_escape/js/minigames/container/container-minigame.js (line 512) +if (npc && npc.itemsHeld) { + const npcItemIndex = npc.itemsHeld.findIndex(i => i === item); +``` + +This suggests `itemsHeld` is an **array of item objects**, not IDs. But the validation code treats it like items with `type` and `id` fields. + +**Current Implementation Issue:** +```ruby +# IMPLEMENTATION_PLAN.md (lines 427-431) - WRONG +npc['itemsHeld'].each do |held_item| + if held_item['type'] == item_type && + (held_item['id'] == item_id || held_item['name'] == item_id) + return { id: npc['id'] } + end +end +``` + +**Actual Structure (from client code):** +- NPCs have `itemsHeld: Array` where each item is a **full item object** (same structure as room objects) +- Items are compared by **object reference equality** (`i === item`) +- No consistent `id` field - items use `type` as primary identifier + +**Solution:** + +Update `find_npc_holding_item` helper: + +```ruby +def find_npc_holding_item(item_type, item_id) + @game.scenario_data['rooms'].each do |room_id, room_data| + room_data['npcs']&.each do |npc| + next unless npc['itemsHeld'].present? + + # itemsHeld is array of full item objects + npc['itemsHeld'].each do |held_item| + # Match by type (required) and optionally by id/name + if held_item['type'] == item_type + # If item_id provided, verify it matches + if item_id.present? + item_matches = (held_item['id'] == item_id) || + (held_item['name'] == item_id) || + (item_id == item_type) # Fallback if no id + next unless item_matches + end + + return { + id: npc['id'], + npc: npc, + item: held_item + } + end + end + end + end + nil +end +``` + +--- + +### ⚠️ WARNING #4: Filtered Room Data Removes Too Much + +**Problem:** +The current `filtered_room_data` implementation removes `lockType` and `contents`: + +```ruby +# app/models/break_escape/game.rb (lines 134-147) +def filtered_room_data(room_id) + room = room_data(room_id)&.deep_dup + return nil unless room + + # Remove solutions + room.delete('requires') + room.delete('lockType') if room['locked'] # ❌ Client needs this! + + # Remove solutions from objects + room['objects']&.each do |obj| + obj.delete('requires') + obj.delete('lockType') if obj['locked'] # ❌ Client needs this! + obj.delete('contents') if obj['locked'] # ✅ Correct + end + + room +end +``` + +**Issue:** The GOALS_AND_DECISIONS document explicitly states: + +> **Decision 2:** Hide Only `requires` and `contents` (Not `lockType`) +> - SHOW: Everything else (`lockType`, `locked`, `observations`, etc.) + +But the current code **deletes lockType**, breaking the client's ability to show the correct unlock UI. + +**Solution:** + +Update `filtered_room_data`: + +```ruby +def filtered_room_data(room_id) + room = room_data(room_id)&.deep_dup + return nil unless room + + # Remove ONLY the 'requires' field (the solution) + # Keep lockType, locked, observations visible + filter_requires_and_contents_recursive(room) + + room +end + +private + +def filter_requires_and_contents_recursive(obj) + case obj + when Hash + # Remove 'requires' (the answer) + obj.delete('requires') + + # Remove 'contents' if locked (lazy-loaded separately) + obj.delete('contents') if obj['locked'] + + # Keep lockType - client needs it to show correct UI + # Keep locked - client needs it to show lock status + + # Recursively filter nested objects + obj['objects']&.each { |o| filter_requires_and_contents_recursive(o) } + obj['npcs']&.each { |n| filter_requires_and_contents_recursive(n) } + + when Array + obj.each { |item| filter_requires_and_contents_recursive(item) } + end +end +``` + +--- + +### ⚠️ WARNING #5: Room Access Check Uses Wrong Logic + +**Problem:** +The room endpoint checks: + +```ruby +# IMPLEMENTATION_PLAN.md suggests: +unless @game.player_state['unlockedRooms']&.include?(room_id) + return render_error("Room not accessible: #{room_id}", :forbidden) +end +``` + +But this **breaks the starting room** on first access if initialization fails or the client requests the room before the model callback runs. + +**Root Cause:** +Race condition: +1. Client requests `/games/:id/room/reception` +2. `player_state['unlockedRooms']` might still be `[]` if callback hasn't saved yet +3. Request denied even though it's the starting room + +**Solution:** + +Add defensive check in room endpoint: + +```ruby +def room + authorize @game if defined?(Pundit) + + room_id = params[:room_id] + return render_error('Missing room_id parameter', :bad_request) unless room_id.present? + + # Check if room is accessible (starting room OR in unlockedRooms) + is_start_room = @game.scenario_data['startRoom'] == room_id + is_unlocked = @game.player_state['unlockedRooms']&.include?(room_id) + + unless is_start_room || is_unlocked + return render_error("Room not accessible: #{room_id}", :forbidden) + end + + # Auto-add start room if missing (defensive programming) + if is_start_room && !is_unlocked + @game.player_state['unlockedRooms'] ||= [] + @game.player_state['unlockedRooms'] << room_id + @game.save! + end + + # Get and filter room data + room_data = @game.filtered_room_data(room_id) + return render_error("Room not found: #{room_id}", :not_found) unless room_data + + # Track NPC encounters + track_npc_encounters(room_id, room_data) + + # Filter out 'contents' field - lazy-loaded via separate endpoint + filter_contents_field(room_data) + + Rails.logger.debug "[BreakEscape] Serving room data for: #{room_id}" + + render json: { room_id: room_id, room: room_data } +end +``` + +--- + +### ⚠️ WARNING #6: Container Endpoint Has Wrong ID Resolution + +**Problem:** +The plan uses `params[:container_id]` but doesn't clarify: +- Is this the object's `id` field? +- Is this the object's `name` field? +- Is this the `objectId` (e.g., `safe_1`)? + +From the client code, containers are identified by **objectId** (generated dynamically), not scenario `id`. + +**Evidence:** +```javascript +// public/break_escape/js/minigames/container/container-minigame.js (line 193) +itemImg.objectId = `container_${index}`; +``` + +**Solution:** + +The container endpoint needs to search by **multiple identifiers**: + +```ruby +def container + authorize @game if defined?(Pundit) + + container_id = params[:container_id] + return render_error('Missing container_id parameter', :bad_request) unless container_id.present? + + # Find container in scenario data + container_data = find_container_in_scenario(container_id) + return render_error("Container not found: #{container_id}", :not_found) unless container_data + + # Check if container is unlocked + # Container might be identified by id, name, or type + is_unlocked = check_container_unlocked(container_id, container_data) + + unless is_unlocked + return render_error("Container not unlocked: #{container_id}", :forbidden) + end + + # Return filtered contents + contents = filter_container_contents(container_data) + + Rails.logger.debug "[BreakEscape] Serving container contents for: #{container_id}" + + render json: { + container_id: container_id, + contents: contents + } +end + +private + +def check_container_unlocked(container_id, container_data) + unlocked_list = @game.player_state['unlockedObjects'] || [] + + # Check multiple possible identifiers + unlocked_list.include?(container_id) || + unlocked_list.include?(container_data['id']) || + unlocked_list.include?(container_data['name']) || + unlocked_list.include?(container_data['type']) +end + +def filter_container_contents(container_data) + contents = container_data['contents']&.map do |item| + item_copy = item.deep_dup + filter_requires_and_contents_recursive(item_copy) + item_copy + end || [] + + contents +end +``` + +--- + +### ⚠️ WARNING #7: No Transaction Safety + +**Problem:** +When unlocking doors/objects, the code modifies `player_state` and saves: + +```ruby +@game.player_state['unlockedRooms'] << target_id +@game.save! +``` + +But if `@game.save!` fails (validation error, DB connection lost, etc.), the in-memory state is corrupted but not persisted. The client receives success but the server has no record. + +**Solution:** + +Wrap unlock operations in transactions: + +```ruby +def unlock + authorize @game if defined?(Pundit) + + target_type = params[:targetType] + target_id = params[:targetId] + attempt = params[:attempt] + method = params[:method] + + is_valid = @game.validate_unlock(target_type, target_id, attempt, method) + + unless is_valid + return render json: { + success: false, + message: 'Invalid attempt' + }, status: :unprocessable_entity + end + + # Use transaction to ensure atomic update + ActiveRecord::Base.transaction do + if target_type == 'door' + @game.unlock_room!(target_id) + + room_data = @game.filtered_room_data(target_id) + filter_contents_field(room_data) + + render json: { + success: true, + type: 'door', + roomData: room_data + } + else + @game.unlock_object!(target_id) + + render json: { + success: true, + type: 'object' + } + end + end +rescue ActiveRecord::RecordInvalid => e + render json: { + success: false, + message: "Failed to save unlock: #{e.message}" + }, status: :unprocessable_entity +end +``` + +--- + +### ⚠️ WARNING #8: NPC Encounter Tracking Mutates State Without Saving + +**Problem:** +```ruby +# IMPLEMENTATION_PLAN.md suggests: +if room_data['npcs'].present? + npc_ids = room_data['npcs'].map { |npc| npc['id'] } + @game.player_state['encounteredNPCs'] ||= [] + @game.player_state['encounteredNPCs'].concat(npc_ids) + @game.player_state['encounteredNPCs'].uniq! + @game.save! # ✅ Saves to DB +end +``` + +But what if `@game.save!` fails? The response already sent `room_data` to the client, so client thinks it succeeded, but server has no record of the encounter. + +**Solution:** + +Track encounters in a transaction **before** sending response: + +```ruby +def room + authorize @game if defined?(Pundit) + + room_id = params[:room_id] + # ... validation ... + + # Track NPC encounters BEFORE sending response + ActiveRecord::Base.transaction do + track_npc_encounters(room_id, room_data) + end + + # Filter and send response + filter_contents_field(room_data) + render json: { room_id: room_id, room: room_data } +rescue ActiveRecord::RecordInvalid => e + render_error("Failed to track NPC encounters: #{e.message}", :internal_server_error) +end + +private + +def track_npc_encounters(room_id, room_data) + return unless room_data['npcs'].present? + + npc_ids = room_data['npcs'].map { |npc| npc['id'] } + @game.player_state['encounteredNPCs'] ||= [] + + new_npcs = npc_ids - @game.player_state['encounteredNPCs'] + return if new_npcs.empty? + + @game.player_state['encounteredNPCs'].concat(new_npcs) + @game.save! + + Rails.logger.debug "[BreakEscape] Tracked NPC encounters: #{new_npcs.join(', ')}" +end +``` + +--- + +### ⚠️ WARNING #9: Inventory Validation Performance Issue + +**Problem:** +The `validate_item_collectible` method searches **every room, every object, every NPC** on every inventory add: + +```ruby +def find_item_in_scenario(item_type, item_id) + @game.scenario_data['rooms'].each do |room_id, room_data| + room_data['objects']&.each do |obj| + return obj if obj['type'] == item_type && ... + obj['contents']&.each do |content| + return content if content['type'] == item_type && ... + end + end + end + nil +end +``` + +For a scenario with 20 rooms × 10 objects × 5 contents = **1000 iterations per inventory add**. + +**Solution:** + +Cache scenario item locations in memory: + +```ruby +# app/models/break_escape/game.rb + +def item_index + @item_index ||= build_item_index +end + +private + +def build_item_index + index = {} + + scenario_data['rooms'].each do |room_id, room_data| + room_data['objects']&.each_with_index do |obj, obj_idx| + key = item_key(obj) + index[key] = { + room_id: room_id, + object_path: ['objects', obj_idx], + object: obj, + container_id: nil, + npc_id: nil + } + + # Index nested contents + obj['contents']&.each_with_index do |content, content_idx| + content_key = item_key(content) + index[content_key] = { + room_id: room_id, + object_path: ['objects', obj_idx, 'contents', content_idx], + object: content, + container_id: obj['id'] || obj['name'], + npc_id: nil + } + end + end + + # Index NPC items + room_data['npcs']&.each do |npc| + npc['itemsHeld']&.each do |item| + item_key_str = item_key(item) + index[item_key_str] = { + room_id: room_id, + object: item, + npc_id: npc['id'], + container_id: nil + } + end + end + end + + index +end + +def item_key(item) + "#{item['type']}:#{item['id'] || item['name'] || 'unnamed'}" +end + +# Then use it: +def validate_item_collectible(item) + key = item_key(item.to_h) + location = item_index[key] + + return "Item not found in scenario" unless location + return "Item is not takeable" unless location[:object]['takeable'] + + # Check container unlock + if location[:container_id].present? + unless @game.player_state['unlockedObjects'].include?(location[:container_id]) + return "Container not unlocked: #{location[:container_id]}" + end + end + + # ... rest of validation ... +end +``` + +--- + +### ⚠️ WARNING #10: Missing Route for Container Endpoint + +**Problem:** +The routes file has: + +```ruby +get 'room/:room_id', to: 'games#room' +``` + +But the plan adds: + +```ruby +get 'container/:container_id' # Missing 'to:' parameter! +``` + +**Solution:** + +Add proper route: + +```ruby +# config/routes.rb +resources :games, only: [:show, :create] do + member do + get 'scenario' + get 'ink' + get 'room/:room_id', to: 'games#room' + get 'container/:container_id', to: 'games#container' # ✅ Fixed + + put 'sync_state' + post 'unlock' + post 'inventory' + end +end +``` + +--- + +## Architecture Improvements + +### Improvement #1: Add Scenario Map Endpoint for Client Navigation + +**Current Gap:** +Client has no way to get room connectivity without loading full room data. + +**Solution:** + +```ruby +def scenario_map + authorize @game if defined?(Pundit) + + layout = {} + @game.scenario_data['rooms'].each do |room_id, room_data| + layout[room_id] = { + type: room_data['type'], + connections: room_data['connections'] || {}, + locked: room_data['locked'] || false, + lockType: room_data['lockType'], + hasNPCs: (room_data['npcs']&.length || 0) > 0, + accessible: @game.room_unlocked?(room_id) + } + end + + render json: { + startRoom: @game.scenario_data['startRoom'], + currentRoom: @game.player_state['currentRoom'], + rooms: layout + } +end +``` + +Add route: +```ruby +get 'scenario_map', to: 'games#scenario_map' +``` + +--- + +### Improvement #2: Add Bulk Inventory Sync Endpoint + +**Problem:** +Client might get out of sync with server (page refresh, network error, etc.). + +**Solution:** + +```ruby +# GET /games/:id/inventory +def get_inventory + authorize @game if defined?(Pundit) + + render json: { + inventory: @game.player_state['inventory'] || [] + } +end +``` + +--- + +### Improvement #3: Add Health Check Endpoint + +**Problem:** +No way to verify game state is valid or recover from corruption. + +**Solution:** + +```ruby +def validate_state + authorize @game if defined?(Pundit) + + issues = [] + + # Check if current room is unlocked + current_room = @game.player_state['currentRoom'] + if current_room && !@game.room_unlocked?(current_room) + issues << "Current room #{current_room} is not unlocked" + end + + # Check if inventory items exist in scenario + @game.player_state['inventory']&.each do |item| + key = item_key(item) + unless @game.item_index[key] + issues << "Inventory item #{key} not found in scenario" + end + end + + render json: { + valid: issues.empty?, + issues: issues, + player_state: @game.player_state + } +end +``` + +--- + +## Testing Gaps + +### Gap #1: No Integration Tests for NPC Item Validation + +**Missing Test:** +```ruby +# test/integration/npc_item_validation_test.rb +test "cannot collect item from unencountered NPC" do + # Setup: NPC in room_b holds key_1 + # Player in room_a tries to add key_1 to inventory + # Expected: 403 Forbidden with "NPC not encountered" +end + +test "can collect item after entering NPC room" do + # Player enters room_b (encounters NPC automatically) + # Player collects key_1 + # Expected: 200 OK, item in inventory +end +``` + +--- + +### Gap #2: No Test for Container Unlock Race Condition + +**Missing Test:** +```ruby +test "cannot access container contents before unlock completes" do + # Client unlocks safe_1 (async) + # Client immediately requests /container/safe_1 (before save completes) + # Expected: 403 Forbidden (not in unlockedObjects yet) +end +``` + +--- + +### Gap #3: No Test for Nested Container Contents + +**Missing Test:** +```ruby +test "cannot collect item from nested locked container" do + # safe_1 contains briefcase_1 (locked) + # briefcase_1 contains key_2 + # Player unlocks safe_1, tries to collect key_2 + # Expected: Forbidden (briefcase_1 not unlocked) +end +``` + +--- + +## Data Consistency Concerns + +### Concern #1: Starting Inventory Duplication + +**Problem:** +If `startItemsInInventory` contains item references that also exist in rooms, the item might be duplicated. + +**Example:** +```json +{ + "startItemsInInventory": [ + { "type": "phone", "name": "Your Phone" } + ], + "rooms": { + "reception": { + "objects": [ + { "type": "phone", "name": "Your Phone", "takeable": true } + ] + } + } +} +``` + +Player starts with phone AND can collect phone from room → 2 phones. + +**Solution:** + +Document in scenario design guide: +> **IMPORTANT:** Items in `startItemsInInventory` should NOT also appear as takeable objects in rooms. Use `takeable: false` if you need a visual placeholder. + +--- + +### Concern #2: Container Unlock Doesn't Cascade + +**Problem:** +If `safe_1` contains `briefcase_1` (locked), unlocking `safe_1` doesn't auto-unlock `briefcase_1`. But client might expect access to all contents. + +**Current Behavior:** Correct - nested containers require separate unlock. + +**Documentation Needed:** +Add to scenario design docs: "Locked containers within containers require separate unlock actions." + +--- + +## Error Handling Improvements + +### Missing: Rate Limiting + +**Problem:** +Client can spam unlock attempts to brute-force passwords. + +**Solution (Phase 2):** + +```ruby +# app/models/break_escape/game.rb +def record_unlock_attempt(target_id) + @game.player_state['unlockAttempts'] ||= {} + @game.player_state['unlockAttempts'][target_id] ||= [] + @game.player_state['unlockAttempts'][target_id] << Time.current.to_i + + # Keep only last 10 minutes of attempts + cutoff = 10.minutes.ago.to_i + @game.player_state['unlockAttempts'][target_id].reject! { |t| t < cutoff } + + @game.save! +end + +def too_many_unlock_attempts?(target_id) + attempts = @game.player_state.dig('unlockAttempts', target_id) || [] + attempts.length > 10 # Max 10 attempts per 10 minutes +end +``` + +Use in unlock endpoint: +```ruby +if too_many_unlock_attempts?(target_id) + return render json: { + success: false, + message: 'Too many attempts. Please wait.' + }, status: :too_many_requests +end +``` + +--- + +### Missing: Graceful Degradation for Network Errors + +**Problem:** +If server is down, client has no fallback behavior. + +**Solution (Client-Side):** + +```javascript +// public/break_escape/js/systems/inventory.js + +const MAX_RETRIES = 3; +const RETRY_DELAY = 1000; + +async function addToInventoryWithRetry(sprite, retries = 0) { + try { + return await addToInventory(sprite); + } catch (error) { + if (retries < MAX_RETRIES) { + console.log(`Retry ${retries + 1}/${MAX_RETRIES} after ${RETRY_DELAY}ms`); + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + return addToInventoryWithRetry(sprite, retries + 1); + } + + // All retries failed - show persistent error + window.gameAlert( + 'Network error - your progress may not be saved. Please refresh the page.', + 'error', + 'Connection Lost', + 10000 + ); + return false; + } +} +``` + +--- + +## Performance Optimizations + +### Optimization #1: Cache Filtered Room Data + +**Problem:** +Every room request re-filters the entire room data structure. + +**Solution:** + +```ruby +# app/models/break_escape/game.rb + +def filtered_room_data(room_id) + cache_key = "filtered_room_#{room_id}_#{updated_at.to_i}" + + Rails.cache.fetch(cache_key, expires_in: 5.minutes) do + room = room_data(room_id)&.deep_dup + return nil unless room + + filter_requires_and_contents_recursive(room) + room + end +end +``` + +--- + +### Optimization #2: Use JSON Schema Validation + +**Problem:** +Manual validation of item structure is brittle. + +**Solution:** + +```ruby +# Gemfile +gem 'json-schema' + +# app/models/break_escape/game.rb +ITEM_SCHEMA = { + "type" => "object", + "required" => ["type", "name"], + "properties" => { + "type" => { "type" => "string" }, + "name" => { "type" => "string" }, + "takeable" => { "type" => "boolean" } + } +}.freeze + +def validate_item_structure(item) + JSON::Validator.validate!(ITEM_SCHEMA, item) +rescue JSON::Schema::ValidationError => e + Rails.logger.error "Invalid item structure: #{e.message}" + false +end +``` + +--- + +## Implementation Priority + +### Phase 1: Core Validation (Week 1) +- ✅ Model: `filter_requires_and_contents_recursive` +- ✅ Model: `initialize_player_state` (verify starting items) +- ✅ Controller: Update `room` endpoint filtering +- ✅ Controller: Update `unlock` endpoint tracking +- ⚠️ **ADD:** Controller: Transaction safety for unlock +- ⚠️ **ADD:** Controller: Defensive start room check + +### Phase 2: Inventory Validation (Week 1-2) +- ✅ Controller: `validate_item_collectible` +- ✅ Controller: NPC encounter tracking in room endpoint +- ⚠️ **ADD:** Model: Item index caching +- ⚠️ **ADD:** Controller: NPC itemsHeld structure fix +- ⚠️ **ADD:** Tests: NPC item validation + +### Phase 3: Container Lazy-Loading (Week 2) +- ✅ Controller: `container` endpoint +- ✅ Routes: Add container route +- ⚠️ **ADD:** Controller: Container ID resolution logic +- ⚠️ **ADD:** Client: `loadContainerContents()` method +- ⚠️ **ADD:** Tests: Container access control + +### Phase 4: Client Integration (Week 2-3) **← CRITICAL, CURRENTLY MISSING** +- ⚠️ **ADD:** Client: Update `addToInventory()` to validate with server +- ⚠️ **ADD:** Client: Update container minigame to lazy-load contents +- ⚠️ **ADD:** Client: Handle 403 Forbidden responses +- ⚠️ **ADD:** Client: Retry logic for network errors +- ⚠️ **ADD:** Tests: End-to-end validation flow + +### Phase 5: Error Handling & Recovery (Week 3) +- ⚠️ **ADD:** Controller: Rate limiting for unlock attempts +- ⚠️ **ADD:** Controller: State validation endpoint +- ⚠️ **ADD:** Client: Graceful degradation +- ⚠️ **ADD:** Tests: Error scenarios + +--- + +## Revised TODO Checklist + +### ✅ PHASE 1: Data Model (ALREADY COMPLETE) +- [x] 1.1 Verify player_state default structure ✅ Done (db/migrate) +- [x] 1.2 `initialize_player_state!` method ✅ Done (model callback) +- [x] ~~1.3 Call in controller~~ (Not needed - callback handles it) +- [ ] 1.4 **ADD:** Verify starting items added correctly + +### ✅ PHASE 2: Data Filtering +- [ ] 2.1 Create `filter_requires_and_contents_recursive` ⚠️ Fix to keep lockType +- [ ] 2.2 Update `filtered_room_data` ⚠️ Fix to use new filter method +- [ ] 2.3 **REMOVE:** `filter_contents_field` is replaced by recursive filter +- [ ] 2.4 Update room endpoint ⚠️ Add defensive start room check +- [ ] 2.5 Create `scenario_map` endpoint +- [ ] 2.6 Add `scenario_map` route +- [ ] 2.7 Test: Room endpoint filtering + +### ✅ PHASE 3: Container Lazy-Loading +- [ ] 3.1-3.3 Container search methods ⚠️ Fix ID resolution +- [ ] 3.4 Implement `container` endpoint ⚠️ Add unlocked check logic +- [ ] 3.5 Add route ⚠️ Add `to: 'games#container'` +- [ ] 3.6 Test: Container access control + +### ✅ PHASE 4: Inventory Validation +- [ ] 4.1 Create `validate_item_collectible` +- [ ] 4.2 Create `find_item_in_scenario` ⚠️ Add caching via item_index +- [ ] 4.3-4.5 Item search helpers ⚠️ Fix NPC itemsHeld structure +- [ ] 4.6 Update `inventory` endpoint with validation +- [ ] 4.7 **ADD:** Transaction safety for inventory add +- [ ] 4.8 Test: Item validation scenarios + +### ✅ PHASE 5: NPC Tracking +- [ ] 5.1 Update `room` endpoint NPC tracking ⚠️ Add transaction +- [ ] 5.2 Test: NPC encounter tracking + +### ✅ PHASE 6: Unlock Tracking +- [ ] 6.1 Update `unlock` endpoint ⚠️ Add transaction +- [ ] 6.2 Test: Unlock persistence +- [ ] 6.3 Test: Container unlock integration + +### ✅ PHASE 7: Sync State +- [ ] 7.1 Update `sync_state` ⚠️ Add room access check +- [ ] 7.2 Test: Sync validation + +### ✅ PHASE 8: Routes +- [ ] 8.1 Add new routes +- [ ] 8.2 Verify routes work + +### ⚠️ **PHASE 9: CLIENT INTEGRATION (MISSING FROM PLAN)** +- [ ] 9.1 Update `addToInventory()` to call server +- [ ] 9.2 Update container minigame to call `/container/:id` +- [ ] 9.3 Add error handling for 403 responses +- [ ] 9.4 Add retry logic for network errors +- [ ] 9.5 Test: Client-server validation flow +- [ ] 9.6 Test: Network error scenarios + +### ✅ PHASE 10: Testing +- [ ] 10.1 Integration tests for filtering +- [ ] 10.2 Integration tests for container endpoint +- [ ] 10.3 Integration tests for inventory validation +- [ ] 10.4 Integration tests for NPC tracking +- [ ] 10.5 Integration tests for unlock tracking +- [ ] 10.6 **ADD:** Tests for NPC itemsHeld validation +- [ ] 10.7 **ADD:** Tests for nested container validation +- [ ] 10.8 End-to-end: Complete puzzle flow + +--- + +## Risk Assessment + +### High Risk ⚠️ +1. **Client integration missing** - Plan doesn't update client code (CRITICAL) +2. **NPC itemsHeld structure unclear** - Might break item collection (HIGH) +3. **No transaction safety** - State corruption risk (HIGH) +4. **Container ID resolution ambiguous** - Wrong items might unlock (MEDIUM-HIGH) + +### Medium Risk ⚠️ +5. **Performance not addressed** - Linear search per validation (MEDIUM) +6. **No error recovery** - Network issues will break game (MEDIUM) +7. **Race conditions** - Start room access might fail (MEDIUM) + +### Low Risk ℹ️ +8. **Rate limiting missing** - Brute force possible but unlikely (LOW) +9. **No state validation endpoint** - Debugging harder (LOW) + +--- + +## Recommendations + +### Immediate Actions (Before Implementation) +1. ✅ Add Phase 9 (Client Integration) to implementation plan with detailed code changes +2. ✅ Clarify NPC `itemsHeld` structure with examples from actual scenarios +3. ✅ Add transaction wrappers to all state-mutating endpoints +4. ✅ Fix `filtered_room_data` to keep `lockType` visible +5. ✅ Add defensive start room check to room endpoint + +### Nice to Have (Phase 2) +6. Add item index caching for performance +7. Add rate limiting for unlock attempts +8. Add state validation endpoint +9. Add graceful degradation for network errors +10. Add comprehensive integration tests for all validation paths + +--- + +## Conclusion + +The implementation plan is **architecturally sound** but has **significant gaps** in: +1. **Client-side integration** (missing entirely) +2. **Transaction safety** (state corruption risk) +3. **Error handling** (no recovery strategies) +4. **NPC itemsHeld structure** (unclear spec) + +**Recommendation:** ⚠️ **Do NOT start implementation** until Phase 9 (Client Integration) is added to the plan with specific file changes and code examples. + +**Estimated Additional Work:** +- Phase 9: 2-3 days (client updates + integration testing) +- Transaction safety: 1 day (add to existing endpoints) +- NPC structure fixes: 1 day (clarify + update validation) + +**Total:** ~4-5 additional days of work beyond the original plan.