BreakEscape/planning_notes/validate_client_actions/review1.md

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:

# 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:

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:

# 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:

# 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:

# 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:

# 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:

# 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

# 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:

## 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:

# 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:

# 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:

# 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)

# 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:

# 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:

# 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:

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:

# 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