Implement Review 3 updates for VM and CTF Flag Integration

- Added detailed review documentation for Hacktivity compatibility. - Addressed critical issue: implemented missing `games#create` action in GamesController. - Updated `MissionsController#show` to handle VM-required missions differently. - Clarified deployment order for migration to remove unique index. - Added recommendations for strong parameters in `games#create`. - Documented necessary changes for flag submission and VM set display. - Verified compatibility of model structures and updated flag submission logic.
2026-02-20 13:50:46 +00:00 · 2025-11-28 14:00:27 +00:00
parent dd720130c7
commit 752fb2c4f9
4 changed files with 1679 additions and 57 deletions
--- a/planning_notes/vms_and_flags/IMPLEMENTATION_PLAN.md
+++ b/planning_notes/vms_and_flags/IMPLEMENTATION_PLAN.md
@@ -1,7 +1,11 @@
 # VM and CTF Flag Integration - Implementation Plan

-**Last Updated**: After Review 1 (2025-11-27)  
-**Review Document**: `planning_notes/vms_and_flags/review1/REVIEW.md`
+**Last Updated**: After Hacktivity Compatibility Review (2025-11-28)  
+**Review Documents**: 
+- `planning_notes/vms_and_flags/review1/REVIEW.md`
+- `planning_notes/vms_and_flags/review2/REVIEW.md`
+- `planning_notes/vms_and_flags/review3/REVIEW.md`
+- `planning_notes/vms_and_flags/review3/HACKTIVITY_COMPATIBILITY.md`

 ## Overview

@@ -18,8 +22,17 @@ This plan integrates VMs and CTF flag submission into BreakEscape. Players will
 - The `secgen_scenario` column ALREADY EXISTS in `break_escape_missions` (migration 20251125000001)
 - Routes are defined in `BreakEscape::Engine.routes.draw` block (not `namespace :break_escape`)
 - Game model uses `initialize_player_state` callback (no DEFAULT_PLAYER_STATE constant)
- Use existing `games#create` action pattern for game creation
- Flag submission to Hacktivity should use POST `/vms/auto_flag_submit` API endpoint
+- **`games#create` action does NOT exist** - routes declare it but controller doesn't implement it (Review 3 finding)
+- Current game creation happens in `MissionsController#show` via `find_or_create_by!`
+- `window.breakEscapeConfig` already exists - must EXTEND, not replace
+
+### Hacktivity Compatibility (Verified Against Hacktivity Codebase)
+- **Association name**: Hacktivity uses `sec_gen_batch` (with underscore), not `secgen_batch`
+- **Flag submission**: Use `FlagService.process_flag()` for proper scoring, streaks, and ActionCable notifications
+- **VmSet display**: No `display_name` method - use `sec_gen_batch.title` instead
+- **Flag matching**: Case-insensitive (`lower(flag_key) = ?`)
+- **Console URL**: `/hacktivities/:event_id/challenges/:sec_gen_batch_id/vm_sets/:vm_set_id/vms/:id/ovirt_console`
+- **VM context needs**: `event_id` and `sec_gen_batch_id` for constructing console URLs

 ---

@@ -63,19 +76,291 @@ This plan integrates VMs and CTF flag submission into BreakEscape. Players will

 ### Client Configuration

-The game view MUST set `window.breakEscapeConfig` before game initialization:
+The game view already sets `window.breakEscapeConfig` in `app/views/break_escape/games/show.html.erb`. 
+**EXTEND** the existing config (don't replace it) by adding VM-related fields:

-```javascript
-// Set in app/views/break_escape/games/show.html.erb
-window.breakEscapeConfig = {
-  gameId: <%= @game.id %>,
-  hacktivityMode: <%= BreakEscape::Mission.hacktivity_mode? %>,
-  vmSetId: <%= @game.player_state['vm_set_id'] || 'null' %>
-};
+```erb
+<%# app/views/break_escape/games/show.html.erb - EXTEND existing config %>
+<script nonce="<%= content_security_policy_nonce %>">
+  window.breakEscapeConfig = {
+    // EXISTING fields (keep these):
+    gameId: <%= @game.id %>,
+    apiBasePath: '<%= game_path(@game) %>',
+    assetsPath: '/break_escape/assets',
+    csrfToken: '<%= form_authenticity_token %>',
+    // NEW fields for VM/flag integration:
+    hacktivityMode: <%= BreakEscape::Mission.hacktivity_mode? %>,
+    vmSetId: <%= @game.player_state['vm_set_id'] || 'null' %>
+  };
+</script>
 ```

 ---

+## Hacktivity Interface Reference
+
+This section documents how BreakEscape interfaces with Hacktivity's models and services. Use this as a reference when implementing the integration.
+
+### Hacktivity Data Model
+
+```
+┌─────────────────┐     ┌─────────────────┐     ┌─────────────────┐
+│     Event       │────<│  SecGenBatch    │────<│     VmSet       │
+│ (hacktivity)    │     │  (challenge)    │     │ (user's VMs)    │
+└─────────────────┘     └─────────────────┘     └─────────────────┘
+                              │                        │
+                              │                        │ has_many
+                              │                        ▼
+                              │                 ┌─────────────────┐
+                              │                 │       Vm        │
+                              │                 │ (single VM)     │
+                              │                 └─────────────────┘
+                              │                        │
+                              │                        │ has_many
+                              │                        ▼
+                              │                 ┌─────────────────┐
+                              └─────────────────│      Flag       │
+                                                │ (CTF flag)      │
+                                                └─────────────────┘
+```
+
+### Key Hacktivity Models
+
+#### `VmSet`
+Represents a user's allocated set of VMs for a challenge.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| `id` | integer | Primary key |
+| `user_id` | integer | Owner (nullable - unallocated sets have nil) |
+| `sec_gen_batch_id` | integer | Parent challenge |
+| `team_id` | integer | Team owner (for team challenges) |
+| `relinquished` | boolean | Whether VMs have been returned |
+| `build_status` | string | `"pending"`, `"success"`, or `"error"` |
+| `activated` | boolean | Whether VMs are currently running |
+| `score` | decimal | Current score for this VM set |
+
+**Associations:**
+```ruby
+belongs_to :user, optional: true
+belongs_to :sec_gen_batch      # NOTE: underscore, not camelCase
+belongs_to :team, optional: true
+has_many :vms
+```
+
+#### `Vm`
+Represents a single virtual machine.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| `id` | integer | Primary key |
+| `title` | string | VM name (e.g., "desktop", "server", "web_server") |
+| `ip_address` | string | VM's IP address (may be nil) |
+| `enable_console` | boolean | Whether user can access graphical console |
+| `state` | string | Current state ("up", "down", "offline", etc.) |
+| `vm_set_id` | integer | Parent VM set |
+
+**Associations:**
+```ruby
+belongs_to :vm_set
+has_many :flags
+```
+
+#### `Flag`
+Represents a CTF flag to be discovered and submitted.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| `id` | integer | Primary key |
+| `flag_key` | string | The flag string (e.g., "flag{s3cr3t_v4lu3}") |
+| `solved` | boolean | Whether flag has been submitted |
+| `solved_date` | datetime | When flag was submitted |
+| `vm_id` | integer | Parent VM |
+
+**Associations:**
+```ruby
+belongs_to :vm
+```
+
+#### `SecGenBatch`
+Represents a challenge/lab configuration.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| `id` | integer | Primary key |
+| `scenario` | string | SecGen scenario path (e.g., "scenarios/ctf/example.xml") |
+| `title` | string | Challenge display name |
+| `event_id` | integer | Parent event |
+
+**Associations:**
+```ruby
+belongs_to :event
+has_many :vm_sets
+```
+
+### Querying VmSets for a User
+
+To find VM sets that match a BreakEscape mission's `secgen_scenario`:
+
+```ruby
+# BreakEscape stores scenario identifier in mission.secgen_scenario
+# Hacktivity stores it in sec_gen_batch.scenario
+
+::VmSet.joins(:sec_gen_batch)
+       .where(sec_gen_batches: { scenario: mission.secgen_scenario })
+       .where(user: user, relinquished: false)
+       .where.not(build_status: ['pending', 'error'])
+       .includes(:vms, :sec_gen_batch)
+       .order(created_at: :desc)
+```
+
+**Important Notes:**
+- Use `sec_gen_batches` (plural with underscore) as the table name
+- Filter out `pending` and `error` build statuses
+- Always eager-load `:vms` and `:sec_gen_batch` to avoid N+1 queries
+- VmSet has no `display_name` method - use `vm_set.sec_gen_batch.title`
+
+### Flag Submission Service
+
+**DO NOT** update flags directly. Use Hacktivity's `FlagService` for proper scoring:
+
+```ruby
+# WRONG - bypasses scoring, streaks, notifications
+flag.update!(solved: true, solved_date: Time.current)
+
+# CORRECT - handles everything
+::FlagService.process_flag(vm, flag_key, user, flash)
+```
+
+**What `FlagService.process_flag` handles:**
+1. Case-insensitive flag matching (`lower(flag_key) = ?`)
+2. Score calculation (percent-based or early-bird scoring)
+3. User streak tracking (gamification)
+4. Result updates for leaderboards
+5. ActionCable notifications for real-time scoreboard updates
+6. Late submission penalties (if configured)
+
+**Calling from BreakEscape (no flash context):**
+```ruby
+def submit_to_hacktivity(flag_key)
+  vm_set = ::VmSet.find_by(id: player_state['vm_set_id'])
+  return unless vm_set
+  
+  vm_set.vms.each do |vm|
+    flag = vm.flags.where("lower(flag_key) = ?", flag_key.downcase).first
+    next unless flag
+    
+    # Create mock flash since we're not in web request context
+    mock_flash = OpenStruct.new
+    mock_flash.define_singleton_method(:[]=) { |k, v| 
+      Rails.logger.info "[BreakEscape] Flag: #{k}: #{v}" 
+    }
+    
+    ::FlagService.process_flag(vm, flag_key, vm_set.user || player, mock_flash)
+    return
+  end
+end
+```
+
+### VM Console URL Construction
+
+To launch a VM's graphical console in Hacktivity, construct this URL:
+
+```
+/hacktivities/:event_id/challenges/:sec_gen_batch_id/vm_sets/:vm_set_id/vms/:vm_id/ovirt_console
+```
+
+**Required IDs (store in VM context during game creation):**
+- `event_id` - from `vm_set.sec_gen_batch.event_id`
+- `sec_gen_batch_id` - from `vm_set.sec_gen_batch_id`
+- `vm_set_id` - from `vm_set.id`
+- `vm_id` - from `vm.id`
+
+**Client-side URL construction:**
+```javascript
+function getConsoleUrl(vm) {
+  if (!window.breakEscapeConfig?.hacktivityMode) return null;
+  
+  return `/hacktivities/${vm.event_id}/challenges/${vm.sec_gen_batch_id}` +
+         `/vm_sets/${vm.vm_set_id}/vms/${vm.id}/ovirt_console`;
+}
+```
+
+**Notes:**
+- This triggers async console file generation
+- User must be authorized (own the VM set or be admin/VIP)
+- VM must be in "up" state for console to work
+- Console access may start timers for timed assessments
+
+### Authorization Rules
+
+Hacktivity uses Pundit policies. A user can access a VmSet if:
+
+```ruby
+# From VmSetPolicy and VmPolicy
+def user_allocated_vm_set?
+  admin? || 
+  scoped_vip_by_user?(vm_set.user) ||
+  (vm_set.user == user && user.has_event_role?(vm_set.sec_gen_batch.event)) ||
+  vm_set.team&.users&.exists?(user.id)
+end
+```
+
+**In plain terms:**
+1. User is admin, OR
+2. User is VIP with scope over the VM owner, OR
+3. User owns the VM set AND has a role in the event, OR
+4. User is a member of the team that owns the VM set
+
+### Detecting Hacktivity Mode
+
+Check if BreakEscape is running inside Hacktivity:
+
+```ruby
+# In Mission model or helper
+def self.hacktivity_mode?
+  defined?(::VmSet) && defined?(::SecGenBatch) && defined?(::FlagService)
+end
+```
+
+### Error Handling
+
+When interfacing with Hacktivity models:
+
+```ruby
+def submit_to_hacktivity(flag_key)
+  return unless defined?(::VmSet)
+  
+  begin
+    # ... Hacktivity integration code ...
+  rescue ActiveRecord::RecordNotFound => e
+    Rails.logger.error "[BreakEscape] VM set not found: #{e.message}"
+  rescue => e
+    Rails.logger.error "[BreakEscape] Hacktivity integration error: #{e.message}"
+    Rails.logger.error e.backtrace.first(5).join("\n")
+    # Don't re-raise - flag is still valid in BreakEscape even if Hacktivity sync fails
+  end
+end
+```
+
+### Summary: Integration Checklist
+
+When implementing Hacktivity integration, ensure:
+
+- [ ] Use `sec_gen_batch` (underscore) not `secgen_batch`
+- [ ] Use `sec_gen_batches` (plural) as table name in queries
+- [ ] Filter out `relinquished: true` VM sets
+- [ ] Filter out `build_status: ['pending', 'error']`
+- [ ] Use `FlagService.process_flag()` for flag submission
+- [ ] Use case-insensitive flag matching
+- [ ] Eager-load `:vms, :sec_gen_batch` associations
+- [ ] Store `event_id` and `sec_gen_batch_id` in VM context for console URLs
+- [ ] Use `sec_gen_batch.title` for display (no `display_name` method)
+- [ ] Wrap Hacktivity calls in `defined?(::VmSet)` checks
+- [ ] Handle errors gracefully without breaking BreakEscape functionality
+
+---
+
 ## Database Changes

 ### Schema Updates
@@ -146,18 +431,26 @@ def requires_vms?
 end

 # Get valid VM sets for this mission (Hacktivity mode only)
+# 
+# HACKTIVITY COMPATIBILITY NOTES:
+# - Hacktivity uses `sec_gen_batch` (with underscore), not `secgen_batch`
+# - The `scenario` field contains the XML path (e.g., "scenarios/ctf/foo.xml")
+# - VmSet doesn't have a `display_name` method - use sec_gen_batch.title instead
+# - Always eager-load :vms and :sec_gen_batch to avoid N+1 queries
 def valid_vm_sets_for_user(user)
  return [] unless self.class.hacktivity_mode? && requires_vms?
  
  # Query Hacktivity's vm_sets where:
-  # - scenario matches
-  # - user owns it
+  # - scenario matches our secgen_scenario
+  # - user owns it (or is on the team)
  # - not relinquished
-  # NOTE: Must use joins for nested association query
+  # - build completed successfully
  ::VmSet.joins(:sec_gen_batch)
         .where(sec_gen_batches: { scenario: secgen_scenario })
         .where(user: user, relinquished: false)
-         .includes(:vms)
+         .where.not(build_status: ['pending', 'error'])
+         .includes(:vms, :sec_gen_batch)
+         .order(created_at: :desc)
 end
 ```

@@ -237,16 +530,23 @@ def build_vm_context
  context = { vms: [], flags: [] }
  
  # Hacktivity mode with VM set
+  # NOTE: Hacktivity uses sec_gen_batch (with underscore), not secgen_batch
  if player_state['vm_set_id'].present? && defined?(::VmSet)
-    vm_set = ::VmSet.find_by(id: player_state['vm_set_id'])
+    vm_set = ::VmSet.includes(:vms, :sec_gen_batch).find_by(id: player_state['vm_set_id'])
    if vm_set
+      # Build VM data with all info needed for console URLs
+      # Hacktivity console URL pattern: /hacktivities/:event_id/challenges/:sec_gen_batch_id/vm_sets/:vm_set_id/vms/:id/ovirt_console
      context[:vms] = vm_set.vms.map do |vm|
        {
          id: vm.id,
          title: vm.title,
          description: vm.description,
          ip_address: vm.ip_address,
-          vm_set_id: vm_set.id
+          vm_set_id: vm_set.id,
+          enable_console: vm.enable_console,
+          # Additional IDs needed for constructing Hacktivity console URLs
+          event_id: vm_set.sec_gen_batch&.event_id,
+          sec_gen_batch_id: vm_set.sec_gen_batch_id
        }
      end
      
@@ -254,6 +554,9 @@ def build_vm_context
      context[:flags] = vm_set.vms.flat_map do |vm|
        vm.flags.map(&:flag_key)
      end
+      
+      # Store challenge info for display
+      context[:challenge_title] = vm_set.sec_gen_batch&.title
    end
  end
  
@@ -275,23 +578,27 @@ def submit_flag(flag_key)
  
  valid_flags = extract_valid_flags_from_scenario
  
-  unless valid_flags.include?(flag_key)
+  # Case-insensitive flag matching (matches Hacktivity behavior)
+  matching_flag = valid_flags.find { |f| f.downcase == flag_key.downcase }
+  
+  unless matching_flag
    return { success: false, message: 'Invalid flag' }
  end
  
-  # Mark flag as submitted
+  # Mark flag as submitted (use the canonical version from scenario)
  player_state['submitted_flags'] ||= []
-  player_state['submitted_flags'] << flag_key
+  player_state['submitted_flags'] << matching_flag
  save!
  
  # Submit to Hacktivity if in that mode
  submit_to_hacktivity(flag_key) if player_state['vm_set_id'].present?
  
-  { success: true, message: 'Flag accepted', flag: flag_key }
+  { success: true, message: 'Flag accepted', flag: matching_flag }
 end

 def flag_submitted?(flag_key)
-  player_state['submitted_flags']&.include?(flag_key)
+  # Case-insensitive check
+  player_state['submitted_flags']&.any? { |f| f.downcase == flag_key.downcase }
 end

 private
@@ -311,29 +618,40 @@ def extract_valid_flags_from_scenario
  flags.uniq
 end

+# UPDATED based on Hacktivity code review:
+# Uses FlagService.process_flag() for proper scoring, streaks, and notifications
 def submit_to_hacktivity(flag_key)
  return unless defined?(::VmSet) && player_state['vm_set_id'].present?
  
  vm_set = ::VmSet.find_by(id: player_state['vm_set_id'])
  return unless vm_set
  
-  # Find the flag in the vm_set
+  # Find the VM with this flag
  vm_set.vms.each do |vm|
-    flag = vm.flags.find_by(flag_key: flag_key)
+    # Case-insensitive flag lookup (matches Hacktivity's FlagService behavior)
+    flag = vm.flags.where("lower(flag_key) = ?", flag_key.downcase).first
    next unless flag
    
-    # Use Hacktivity's auto_flag_submit API endpoint for proper scoring
-    # POST /vms/auto_flag_submit (JSON API)
    begin
-      # Hacktivity::FlagSubmissionService handles scoring, validation, etc.
-      Hacktivity::FlagSubmissionService.new(
-        flag: flag,
-        user: player # or current_user via controller context
-      ).submit!
+      # Use Hacktivity's FlagService for proper scoring, streaks, and notifications
+      # This handles:
+      # - Score calculation (percent or early-bird scoring)
+      # - Streak tracking for gamification
+      # - Result updates for the user
+      # - ActionCable notifications for scoreboards
+      #
+      # We create a mock flash object since we're not in a web request context
+      mock_flash = OpenStruct.new
+      mock_flash.define_singleton_method(:[]=) do |key, value|
+        Rails.logger.info "[BreakEscape] Hacktivity flag result: #{key}: #{value}"
+      end
+      
+      ::FlagService.process_flag(vm, flag_key, vm_set.user || player, mock_flash)
+      Rails.logger.info "[BreakEscape] Submitted flag #{flag_key} to Hacktivity via FlagService"
      
-      Rails.logger.info "[BreakEscape] Submitted flag #{flag_key} to Hacktivity via API for vm_set #{vm_set.id}"
    rescue => e
      Rails.logger.error "[BreakEscape] Failed to submit flag to Hacktivity: #{e.message}"
+      Rails.logger.error e.backtrace.first(5).join("\n")
      # Flag is still marked submitted in BreakEscape even if Hacktivity sync fails
    end
    
@@ -348,12 +666,14 @@ end

 ### 1. Games Controller (`app/controllers/break_escape/games_controller.rb`)

-#### Update `create` Action (Existing)
+#### Add `create` Action (NEW - Does Not Currently Exist)

-The existing `games#create` action handles game creation. Extend it to accept VM context:
+**IMPORTANT**: Despite `config/routes.rb` declaring `resources :games, only: [:show, :create]`, the `create` action is NOT implemented. Games are currently created via `MissionsController#show` using `find_or_create_by!`.
+
+**This action must be implemented from scratch:**

 ```ruby
-# In existing create action
+# Add to app/controllers/break_escape/games_controller.rb
 def create
  @mission = Mission.find(params[:mission_id])
  authorize @mission if defined?(Pundit)
@@ -384,7 +704,10 @@ def create
    initial_player_state['standalone_flags'] = flags
  end
  
-  # Create game - callbacks will handle scenario generation with VM context
+  # CRITICAL: Set player_state BEFORE save! so callbacks can read vm_set_id
+  # Callback order is:
+  # 1. before_create :generate_scenario_data_with_context (reads player_state['vm_set_id'])
+  # 2. before_create :initialize_player_state (adds default fields)
  @game = Game.new(
    player: current_player,
    mission: @mission
@@ -394,6 +717,27 @@ def create
  
  redirect_to game_path(@game)
 end
+
+private
+
+def game_create_params
+  params.permit(:mission_id, :vm_set_id, standalone_flags: [])
+end
+```
+
+#### Add `new` Action (NEW - For VM Set Selection)
+
+```ruby
+# Add to app/controllers/break_escape/games_controller.rb
+def new
+  @mission = Mission.find(params[:mission_id])
+  authorize @mission if defined?(Pundit)
+  
+  if @mission.requires_vms?
+    @available_vm_sets = @mission.valid_vm_sets_for_user(current_user)
+    @existing_games = Game.where(player: current_player, mission: @mission)
+  end
+end
 ```

 #### Add `submit_flag` Action (NEW)
@@ -564,7 +908,8 @@ The routes follow the existing engine pattern in `config/routes.rb`:
 BreakEscape::Engine.routes.draw do
  resources :missions, only: [:index, :show]
  
-  resources :games, only: [:show, :create] do
+  # UPDATED: Add :new to games resource
+  resources :games, only: [:new, :show, :create] do
    member do
      get :scenario
      get :ink
@@ -585,7 +930,94 @@ end
 ```

 **NOTE**: The existing routes structure uses `BreakEscape::Engine.routes.draw`, not `namespace :break_escape`. 
-Games are created via `games#create` (POST /break_escape/games) with `mission_id` parameter.
+
+**CHANGES NEEDED**:
+1. Add `:new` to `only: [:new, :show, :create]` 
+2. Add `post :flags` to the member block
+
+### 2. Missions Controller (`app/controllers/break_escape/missions_controller.rb`)
+
+#### Update `show` Action (Critical Change)
+
+**IMPORTANT**: The current `show` action uses `find_or_create_by!` which won't work with VM context. Update it to handle VM missions differently:
+
+```ruby
+# app/controllers/break_escape/missions_controller.rb
+def show
+  @mission = Mission.find(params[:id])
+  authorize @mission if defined?(Pundit)
+
+  if @mission.requires_vms?
+    # VM missions need explicit game creation with VM set selection
+    # Redirect to games#new which shows VM set selection UI
+    redirect_to new_game_path(mission_id: @mission.id)
+  else
+    # Legacy behavior for non-VM missions - auto-create game
+    @game = Game.find_or_create_by!(
+      player: current_player,
+      mission: @mission
+    )
+    redirect_to game_path(@game)
+  end
+end
+```
+
+### 3. Add Games#new View
+
+```erb
+<%# app/views/break_escape/games/new.html.erb %>
+<div class="game-setup">
+  <h1><%= @mission.name %></h1>
+  <p><%= @mission.description %></p>
+
+  <% if @mission.requires_vms? %>
+    <h2>Select VM Environment</h2>
+    
+    <% if @available_vm_sets.any? %>
+      <div class="vm-set-list">
+        <% @available_vm_sets.each do |vm_set| %>
+          <%= form_with url: break_escape.games_path, method: :post, local: true do |f| %>
+            <%= f.hidden_field :mission_id, value: @mission.id %>
+            <%= f.hidden_field :vm_set_id, value: vm_set.id %>
+            
+            <div class="vm-set-card">
+              <%# NOTE: VmSet doesn't have display_name - use sec_gen_batch.title instead %>
+              <h3><%= vm_set.sec_gen_batch&.title || "VM Set ##{vm_set.id}" %></h3>
+              <p><%= pluralize(vm_set.vms.count, 'VM') %></p>
+              <ul class="vm-list">
+                <% vm_set.vms.each do |vm| %>
+                  <li><%= vm.title %><%= " (#{vm.ip_address})" if vm.ip_address.present? %></li>
+                <% end %>
+              </ul>
+              <%= f.submit "Start with this VM Set", class: "btn btn-primary" %>
+            </div>
+          <% end %>
+        <% end %>
+      </div>
+      
+      <% if @existing_games.any? %>
+        <h3>Continue Existing Game</h3>
+        <% @existing_games.each do |game| %>
+          <%= link_to "Continue (started #{time_ago_in_words(game.started_at)} ago)", 
+                      game_path(game), class: "btn btn-secondary" %>
+        <% end %>
+      <% end %>
+    <% else %>
+      <div class="alert alert-warning">
+        <p>This mission requires VMs but you don't have any available VM sets.</p>
+        <p>Please provision VMs through Hacktivity first.</p>
+      </div>
+    <% end %>
+    
+  <% else %>
+    <%# Non-VM mission - just start %>
+    <%= form_with url: break_escape.games_path, method: :post, local: true do |f| %>
+      <%= f.hidden_field :mission_id, value: @mission.id %>
+      <%= f.submit "Start Mission", class: "btn btn-primary" %>
+    <% end %>
+  <% end %>
+</div>
+```

 ---

@@ -607,11 +1039,15 @@ For simplicity, update the existing mission card to POST to `games#create` with
 <%= form_with url: break_escape.games_path, method: :post, local: true do |f| %>
  <%= f.hidden_field :mission_id, value: mission.id %>
  
+  <%# HACKTIVITY COMPATIBILITY: VmSet doesn't have display_name method %>
+  <%# Use sec_gen_batch.title instead %>
  <% if mission.requires_vms? && @vm_sets_by_mission[mission.id]&.any? %>
    <div class="vm-set-selection">
      <label>Select VM Set:</label>
      <%= f.select :vm_set_id, 
-          options_from_collection_for_select(@vm_sets_by_mission[mission.id], :id, :display_name),
+          @vm_sets_by_mission[mission.id].map { |vs| 
+            ["#{vs.sec_gen_batch&.title} (#{vs.vms.count} VMs)", vs.id] 
+          },
          { include_blank: 'Choose VM Set...' },
          { required: true, class: 'form-control' } %>
    </div>
@@ -636,22 +1072,62 @@ For simplicity, update the existing mission card to POST to `games#create` with

 ### 2. Add `hacktivityMode` Config to Game View

-**IMPORTANT**: Set `window.breakEscapeConfig` in the game show view:
+**IMPORTANT**: EXTEND the existing `window.breakEscapeConfig` (lines 113-118 of show.html.erb):

 ```erb
 <%# app/views/break_escape/games/show.html.erb %>
-<script>
+<%# Find the existing window.breakEscapeConfig block and ADD these fields: %>
+<script nonce="<%= content_security_policy_nonce %>">
  window.breakEscapeConfig = {
+    // EXISTING fields (already present - DO NOT REMOVE):
    gameId: <%= @game.id %>,
+    apiBasePath: '<%= game_path(@game) %>',
+    assetsPath: '/break_escape/assets',
+    csrfToken: '<%= form_authenticity_token %>',
+    // NEW fields to add:
    hacktivityMode: <%= BreakEscape::Mission.hacktivity_mode? %>,
-    vmSetId: <%= @game.player_state['vm_set_id'] || 'null' %>,
-    playerHandle: "<%= j(@game.player.try(:handle) || @game.player.try(:name) || 'Player') %>"
+    vmSetId: <%= @game.player_state['vm_set_id'] || 'null' %>
  };
 </script>
 ```
+
+### 3. Update MissionsController#show for VM Missions
+
+**Problem**: The current `MissionsController#show` uses `find_or_create_by!` which prevents multiple games per mission.
+
+**Solution**: For missions requiring VMs, redirect to game selection instead of auto-creating:
+
+```ruby
+# app/controllers/break_escape/missions_controller.rb
+def show
+  @mission = Mission.find(params[:id])
+  authorize @mission if defined?(Pundit)
+  
+  if @mission.requires_vms? && BreakEscape::Mission.hacktivity_mode?
+    # VM missions need VM set selection first
+    # Redirect to games index filtered by this mission
+    # User will select or create a game there
+    redirect_to break_escape.games_path(mission_id: @mission.id)
+  else
+    # Legacy behavior for non-VM missions (standalone mode)
+    @game = Game.find_or_create_by!(player: current_player, mission: @mission)
+    redirect_to game_path(@game)
+  end
+end
+```
    
 **Styling**: Add CSS to `app/assets/stylesheets/break_escape/` directory as needed.

+### 4. Add CSS Link Tags to show.html.erb
+
+Add link tags for the new minigame CSS files to `app/views/break_escape/games/show.html.erb`:
+
+```erb
+<%# Add after other minigame CSS links (around line 52) %>
+<link rel="stylesheet" href="/break_escape/css/minigames/vm-launcher.css">
+<link rel="stylesheet" href="/break_escape/css/minigames/flag-station.css">
+```
+
 ---

 ## Scenario ERB Template Updates
@@ -1218,18 +1694,31 @@ async function handleFlagStation(pcObject) {
    // Avoids unnecessary fetch to server
    const submittedFlags = window.gameState?.submittedFlags || [];
    
-    // Filter expected flags to only those for THIS flag station
+    // The flag station's scenarioData.flags contains expected flags for THIS station
    const flagStationId = pcObject.scenarioData.id || pcObject.scenarioData.name;
-    const expectedFlags = pcObject.scenarioData.flags || [];
    
    window.startFlagStationMinigame({
        ...pcObject.scenarioData,
        id: flagStationId
-    }, submittedFlags, expectedFlags);
+    }, submittedFlags);
 }
 ```

 **Note**: Using `type: "vm-launcher"` and `type: "flag-station"` directly is consistent with existing patterns like `type: "workstation"`, `type: "notepad"`, etc.
+
+### 5. Update main.js to Populate submittedFlags
+
+After loading the scenario, populate `window.gameState.submittedFlags` for the flag station minigame:
+
+```javascript
+// In main.js, after scenario is loaded:
+// Look for where scenario is fetched and gameState is initialized
+
+// Add after scenario data is received:
+if (scenarioData.submittedFlags) {
+    window.gameState.submittedFlags = scenarioData.submittedFlags;
+}
+```
 ```

 ### 5. Add Submitted Flags to Scenario Bootstrap
@@ -1646,14 +2135,19 @@ end
 - [ ] 1.11 Write model tests

 ### Phase 2: Controllers & Views
- [ ] 2.1 Update GamesController `create` action to accept `vm_set_id` and `standalone_flags` params
- [ ] 2.2 Add GamesController `submit_flag` action
- [ ] 2.3 Add GamesController helper methods: `find_flag_rewards`, `process_flag_rewards`, etc.
- [ ] 2.4 Update routes.rb: Add `post :flags` to games member routes
- [ ] 2.5 Update missions index view to support VM set selection
- [ ] 2.6 Add `window.breakEscapeConfig` to game show view (critical for client)
- [ ] 2.7 Update GamesController `scenario` action: Include `submittedFlags`
- [ ] 2.8 Write controller tests
+- [ ] 2.1 **IMPLEMENT** GamesController `create` action (does NOT exist - must create from scratch)
+- [ ] 2.2 **IMPLEMENT** GamesController `new` action (for VM set selection)
+- [ ] 2.3 Add GamesController `submit_flag` action
+- [ ] 2.4 Add GamesController helper methods: `find_flag_rewards`, `process_flag_rewards`, etc.
+- [ ] 2.5 Add GamesController strong parameters: `game_create_params`
+- [ ] 2.6 Update routes.rb: Add `:new` to games resource AND `post :flags` to member routes
+- [ ] 2.7 **UPDATE** MissionsController `show` to redirect VM missions to games#new
+- [ ] 2.8 Create `app/views/break_escape/games/new.html.erb` for VM set selection
+- [ ] 2.9 Update missions index view to support VM set selection (optional)
+- [ ] 2.10 EXTEND `window.breakEscapeConfig` in game show view (add `hacktivityMode`, `vmSetId`)
+- [ ] 2.11 Update GamesController `scenario` action: Include `submittedFlags`
+- [ ] 2.12 Add CSS link tags for new minigame styles to show.html.erb
+- [ ] 2.13 Write controller tests

 ### Phase 3: Client-Side Minigames
 - [ ] 3.1 Create `public/break_escape/js/minigames/vm-launcher/vm-launcher-minigame.js`
@@ -1662,8 +2156,10 @@ end
 - [ ] 3.4 Update `public/break_escape/js/systems/interactions.js`: Handle `type: "vm-launcher"` and `type: "flag-station"`
 - [ ] 3.5 Create CSS: `public/break_escape/css/minigames/vm-launcher.css`
 - [ ] 3.6 Create CSS: `public/break_escape/css/minigames/flag-station.css`
- [ ] 3.7 Create test files: `test-vm-launcher-minigame.html`, `test-flag-station-minigame.html`
- [ ] 3.8 Test minigames standalone
+- [ ] 3.7 Add CSS link tags to `app/views/break_escape/games/show.html.erb`
+- [ ] 3.8 Update `main.js` to populate `window.gameState.submittedFlags` from scenario response
+- [ ] 3.9 Create test files: `test-vm-launcher-minigame.html`, `test-flag-station-minigame.html`
+- [ ] 3.10 Test minigames standalone

 ### Phase 4: ERB Templates
 - [ ] 4.1 Create example scenario: `scenarios/enterprise_breach/scenario.json.erb`
@@ -1684,6 +2180,40 @@ end

 ---

+## Revised Implementation Order (After Review 3)
+
+Based on Review 3 findings, the implementation order is revised to ensure controller infrastructure is in place before model changes depend on it:
+
+### Phase 1: Controller Infrastructure (FIRST - Critical)
+1. **Implement `games#create` action** (does NOT exist, must be created from scratch)
+2. **Implement `games#new` action** (for VM set selection page)
+3. **Update routes.rb** to add `:new` to games resource and `post :flags`
+4. **Update `MissionsController#show`** to redirect VM missions to `games#new`
+5. **Create `games/new.html.erb` view** for VM set selection
+
+### Phase 2: Database Migration
+1. Run migration to remove unique index on games
+2. Verify existing games unaffected
+
+### Phase 3: Model Changes
+1. Rename `generate_scenario_data` → `generate_scenario_data_with_context` 
+2. Add `build_vm_context` method
+3. Extend `initialize_player_state` with VM/flag fields
+4. Add flag submission methods
+
+### Phase 4: Frontend & Minigames
+1. Create VM launcher and flag station minigames
+2. Update show.html.erb with extended config
+3. Update interactions.js for new object types
+
+### Phase 5: ERB Templates & Testing
+1. Create example scenarios
+2. End-to-end testing
+
+**Key Insight**: The callback timing (`player_state` set before `save!`) is correct and sound. The issue was assuming `games#create` already existed.
+
+---
+
 ## Risk Analysis

 ### High Risk
@@ -1735,3 +2265,6 @@ This plan provides a complete, actionable roadmap for integrating VMs and CTF fl
 | Date | Review | Changes Made |
 |------|--------|--------------|
 | 2025-11-27 | Review 1 (`review1/REVIEW.md`) | Fixed duplicate migration, corrected routes structure, fixed invalid AR query, updated ERB patterns for null safety, changed from `pcMode` to `type` property, corrected flag submission to use Hacktivity API, added `window.breakEscapeConfig` documentation |
+| 2025-11-27 | Review 2 (`review2/REVIEW.md`) | Clarified to EXTEND breakEscapeConfig not replace, simplified Hacktivity flag submission (direct model update), added MissionsController#show update for VM missions, fixed function signature mismatch, added gameState.submittedFlags population note, added CSS link tags to checklist |
+| 2025-11-27 | Review 3 (`review3/REVIEW.md`) | **Critical**: Discovered `games#create` action does NOT exist (routes declared it but controller didn't implement it). Updated plan to implement action from scratch. Added `games#new` action and view for VM set selection. Added MissionsController#show update. Validated callback timing approach is correct. Added revised implementation order prioritizing controller infrastructure. |
+| 2025-11-28 | Hacktivity Compatibility (`review3/HACKTIVITY_COMPATIBILITY.md`) | **Reviewed Hacktivity codebase** for compatibility. Key findings: (1) Use `FlagService.process_flag()` instead of direct model update for proper scoring/streaks/notifications, (2) VmSet uses `sec_gen_batch` (with underscore) not `secgen_batch`, (3) VmSet has no `display_name` method - use `sec_gen_batch.title` instead, (4) Added `event_id` and `sec_gen_batch_id` to VM context for console URLs, (5) Added case-insensitive flag matching to match Hacktivity behavior, (6) Added eager loading with `.includes(:vms, :sec_gen_batch)`, (7) Filter out pending/error build_status VM sets. |
--- a/planning_notes/vms_and_flags/review2/REVIEW.md
+++ b/planning_notes/vms_and_flags/review2/REVIEW.md
@@ -0,0 +1,345 @@
+# VM and CTF Flag Integration - Plan Review 2
+
+**Reviewer**: AI Review  
+**Date**: November 27, 2025  
+**Document Reviewed**: `planning_notes/vms_and_flags/IMPLEMENTATION_PLAN.md` (post-Review 1)
+
+---
+
+## Executive Summary
+
+The updated implementation plan addresses all critical issues from Review 1. The plan is now technically accurate and ready for implementation with a few remaining issues.
+
+**Critical Issues**: 0 (all resolved from Review 1)  
+**Medium Issues**: 3  
+**Minor Issues**: 6  
+
+---
+
+## Issues Fixed Since Review 1 ✓
+
+1. ✓ Duplicate `secgen_scenario` migration removed
+2. ✓ Routes structure corrected to use `BreakEscape::Engine.routes.draw`
+3. ✓ `Game::DEFAULT_PLAYER_STATE` reference removed, uses `initialize_player_state` callback
+4. ✓ ERB patterns updated with null-safety
+5. ✓ ActiveRecord query corrected to use `joins`
+6. ✓ Changed `pcMode` to use `type` property directly
+7. ✓ Added `window.breakEscapeConfig` documentation
+
+---
+
+## Medium Issues
+
+### 1. ⚠️ `window.breakEscapeConfig` Already Exists - Extend, Don't Replace
+
+**Problem**: The plan documents adding `window.breakEscapeConfig` but it already exists in `app/views/break_escape/games/show.html.erb` (lines 113-118):
+
+```erb
+window.breakEscapeConfig = {
+  gameId: <%= @game.id %>,
+  apiBasePath: '<%= game_path(@game) %>',
+  assetsPath: '/break_escape/assets',
+  csrfToken: '<%= form_authenticity_token %>'
+};
+```
+
+The plan's suggested config is a partial replacement:
+```javascript
+window.breakEscapeConfig = {
+  gameId: <%= @game.id %>,
+  hacktivityMode: <%= BreakEscape::Mission.hacktivity_mode? %>,
+  vmSetId: <%= @game.player_state['vm_set_id'] || 'null' %>,
+  playerHandle: "..."
+};
+```
+
+**Impact**: Missing `apiBasePath`, `assetsPath`, and `csrfToken` will break existing functionality.
+
+**Fix**: Document that the config should be EXTENDED, not replaced:
+```erb
+window.breakEscapeConfig = {
+  gameId: <%= @game.id %>,
+  apiBasePath: '<%= game_path(@game) %>',
+  assetsPath: '/break_escape/assets',
+  csrfToken: '<%= form_authenticity_token %>',
+  // NEW fields for VM/flag integration:
+  hacktivityMode: <%= BreakEscape::Mission.hacktivity_mode? %>,
+  vmSetId: <%= @game.player_state['vm_set_id'] || 'null' %>
+};
+```
+
+---
+
+### 2. ⚠️ `Hacktivity::FlagSubmissionService` May Not Exist
+
+**Problem**: The plan assumes a `Hacktivity::FlagSubmissionService` class exists:
+
+```ruby
+Hacktivity::FlagSubmissionService.new(
+  flag: flag,
+  user: player
+).submit!
+```
+
+This service class likely doesn't exist in Hacktivity - the user mentioned using the `auto_flag_submit` API endpoint.
+
+**Impact**: Code will fail with `NameError: uninitialized constant Hacktivity::FlagSubmissionService`.
+
+**Fix**: Use HTTP request to the auto_flag_submit endpoint instead:
+
+```ruby
+def submit_to_hacktivity(flag_key)
+  return unless defined?(::VmSet) && player_state['vm_set_id'].present?
+  
+  vm_set = ::VmSet.find_by(id: player_state['vm_set_id'])
+  return unless vm_set
+  
+  # Find the flag in the vm_set
+  vm_set.vms.each do |vm|
+    flag = vm.flags.find_by(flag_key: flag_key)
+    next unless flag
+    
+    # Use Hacktivity's auto_flag_submit API endpoint
+    # This handles scoring, validation, and all flag submission logic
+    begin
+      response = Net::HTTP.post(
+        URI('/vms/auto_flag_submit'),
+        { flag_key: flag_key, vm_id: vm.id }.to_json,
+        'Content-Type' => 'application/json'
+      )
+      
+      if response.code == '200'
+        Rails.logger.info "[BreakEscape] Submitted flag #{flag_key} to Hacktivity API"
+      else
+        Rails.logger.warn "[BreakEscape] Hacktivity flag submission returned #{response.code}"
+      end
+    rescue => e
+      Rails.logger.error "[BreakEscape] Failed to submit flag to Hacktivity: #{e.message}"
+    end
+    
+    return # Only submit once
+  end
+end
+```
+
+**OR** if direct model access is preferred (simpler, but bypasses Hacktivity logic):
+```ruby
+# Simple approach - just mark flag as solved
+flag.update!(solved: true, solved_date: Time.current) if flag.respond_to?(:solved)
+```
+
+---
+
+### 3. ⚠️ `MissionsController#show` Creates Game with `find_or_create_by!`
+
+**Problem**: The current `MissionsController#show` action uses `find_or_create_by!`:
+
+```ruby
+@game = Game.find_or_create_by!(
+  player: current_player,
+  mission: @mission
+)
+```
+
+The plan proposes allowing multiple games per mission (for different VM sets), but this code will always return the first existing game, never creating a new one with a different VM set.
+
+**Impact**: Users won't be able to create multiple games per mission with different VM sets.
+
+**Fix**: The plan needs to address how game creation flow changes:
+
+**Option A** (Plan's implied approach): Remove auto-creation from `show`, require explicit `games#create` call
+```ruby
+# MissionsController#show - don't auto-create
+def show
+  @mission = Mission.find(params[:id])
+  authorize @mission if defined?(Pundit)
+  # Don't create game here - redirect to game selection or creation
+  redirect_to break_escape.games_path(mission_id: @mission.id)
+end
+```
+
+**Option B** (Backward compatible): Keep auto-creation for missions without VMs, require selection for VM missions
+```ruby
+def show
+  @mission = Mission.find(params[:id])
+  authorize @mission if defined?(Pundit)
+  
+  if @mission.requires_vms?
+    # Redirect to game selection/creation page
+    redirect_to new_game_path(mission_id: @mission.id)
+  else
+    # Legacy behavior for non-VM missions
+    @game = Game.find_or_create_by!(player: current_player, mission: @mission)
+    redirect_to game_path(@game)
+  end
+end
+```
+
+---
+
+## Minor Issues
+
+### 4. 📝 Routes Already Correct - No Change Needed
+
+The routes.rb shown in the plan matches the existing structure except for the `post :flags` addition. The plan should clarify this is the ONLY change needed:
+
+```ruby
+# ONLY ADD THIS LINE:
+post :flags          # NEW: Submit flag endpoint
+```
+
+---
+
+### 5. 📝 `gameState.submittedFlags` Not Set on Game Load
+
+**Problem**: The plan's `handleFlagStation` reads from `window.gameState?.submittedFlags`:
+
+```javascript
+const submittedFlags = window.gameState?.submittedFlags || [];
+```
+
+But the plan also says the scenario endpoint should return `submittedFlags`. If `gameState` is populated from the scenario response, this should work. However:
+
+1. The plan doesn't show where `window.gameState.submittedFlags` is populated from the scenario response
+2. The `main.js` game initialization would need to copy this from scenario to gameState
+
+**Fix**: Add note that `main.js` should populate `window.gameState.submittedFlags` from `scenario.submittedFlags` after loading.
+
+---
+
+### 6. 📝 Missing CSS Imports in show.html.erb
+
+The plan creates CSS files:
+- `public/break_escape/css/minigames/vm-launcher.css`
+- `public/break_escape/css/minigames/flag-station.css`
+
+But doesn't document adding the link tags to `show.html.erb`.
+
+**Fix**: Add to checklist:
+```erb
+<link rel="stylesheet" href="/break_escape/css/minigames/vm-launcher.css">
+<link rel="stylesheet" href="/break_escape/css/minigames/flag-station.css">
+```
+
+---
+
+### 7. 📝 `startFlagStationMinigame` Signature Mismatch
+
+**Problem**: The function is defined with 2 parameters:
+```javascript
+export function startFlagStationMinigame(flagStation, submittedFlags = []) {
+```
+
+But called with 3 parameters in `handleFlagStation`:
+```javascript
+window.startFlagStationMinigame({
+    ...pcObject.scenarioData,
+    id: flagStationId
+}, submittedFlags, expectedFlags);  // <-- 3 params
+```
+
+**Impact**: The third parameter `expectedFlags` is passed but ignored.
+
+**Fix**: Either:
+- A) Remove third param from call site (flags are already in flagStation.flags)
+- B) Update function signature to accept third param
+
+Since `flagStation.flags` already contains expected flags, the call should be:
+```javascript
+window.startFlagStationMinigame({
+    ...pcObject.scenarioData,
+    id: flagStationId
+}, submittedFlags);
+```
+
+---
+
+### 8. 📝 Missing `current_user` vs `current_player` Clarification
+
+**Problem**: The plan uses `current_player` but refers to `current_user` in some places for Hacktivity integration:
+
+```ruby
+initial_player_state['vm_set_id'] = vm_set.id
+# ...later...
+user: player # or current_user via controller context
+```
+
+In BreakEscape standalone mode, `current_player` returns a `DemoUser`. In Hacktivity mode, `current_user` (from Devise) is the actual user.
+
+**Fix**: Document that VM set queries should use `current_user` when available:
+```ruby
+def valid_vm_sets_for_user(user)
+  # user should be current_user from Devise in Hacktivity mode
+  # In standalone mode, this returns empty array (no VmSet model)
+end
+```
+
+---
+
+### 9. 📝 Minigame CSS Uses `border-radius: 0` But This Is Already Default
+
+The CSS styling notes say to use `border-radius: 0` for pixel-art consistency, but the CSS examples already show this. Not an issue, just redundant documentation.
+
+---
+
+## Verification Checklist
+
+Items verified as correct in the updated plan:
+
+| Item | Status | Notes |
+|------|--------|-------|
+| Migration for secgen_scenario | ✓ Correct | Already exists, marked as complete |
+| Routes structure | ✓ Correct | Uses Engine.routes.draw |
+| initialize_player_state pattern | ✓ Correct | Extends existing callback |
+| Type property usage | ✓ Correct | Uses `type: "vm-launcher"` |
+| ERB null-safety | ✓ Correct | Uses `&.`, `dig`, `|| 'null'` |
+| ActiveRecord joins query | ✓ Correct | Uses proper joins syntax |
+| Controller action pattern | ✓ Correct | Uses games#create |
+
+---
+
+## Recommendations
+
+### 1. Add Integration Test for Flag Submission Flow
+
+The testing plan covers unit tests but should include a full integration test:
+1. Create mission with `secgen_scenario`
+2. Create VM set with flags
+3. Create game with `vm_set_id`
+4. Submit flag via endpoint
+5. Verify flag in `submitted_flags`
+6. Verify reward processed
+7. Verify Hacktivity flag marked solved
+
+### 2. Consider Flag Validation Edge Cases
+
+- What if flag is submitted but Hacktivity API is down?
+- What if flag is valid in BreakEscape but not found in Hacktivity?
+- Should there be a retry mechanism?
+
+### 3. Document Player Handle Source
+
+The plan mentions `playerHandle` in config but doesn't specify where it comes from for different player types (DemoUser vs Hacktivity User).
+
+---
+
+## Summary of Required Plan Updates
+
+| Issue | Severity | Action |
+|-------|----------|--------|
+| 1. breakEscapeConfig extension | Medium | Update to show extending, not replacing |
+| 2. FlagSubmissionService | Medium | Change to HTTP request or direct model |
+| 3. MissionsController#show | Medium | Document how game creation flow changes |
+| 4. Routes clarification | Minor | Clarify only `post :flags` is new |
+| 5. gameState.submittedFlags | Minor | Add note about population |
+| 6. CSS imports | Minor | Add to checklist |
+| 7. Function signature | Minor | Fix parameter mismatch |
+| 8. current_user clarification | Minor | Document which user to use |
+
+---
+
+## Conclusion
+
+The plan is significantly improved from Review 1. All critical issues have been resolved. The remaining medium issues are around integration details that should be clarified before implementation begins. The plan is **ready for implementation** with the above fixes applied.
+
+**Overall Rating**: Ready for implementation (with minor corrections)
--- a/planning_notes/vms_and_flags/review3/HACKTIVITY_COMPATIBILITY.md
+++ b/planning_notes/vms_and_flags/review3/HACKTIVITY_COMPATIBILITY.md
@@ -0,0 +1,389 @@
+# Hacktivity Compatibility Review
+
+**Date**: November 28, 2025  
+**Reviewed**: Hacktivity codebase at `/home/cliffe/Files/Projects/Code/Hacktivity/`
+
+---
+
+## Executive Summary
+
+After reviewing Hacktivity's codebase, our implementation plan is **mostly compatible** with a few issues that need addressing. The main findings relate to:
+
+1. ✅ Model structure matches our assumptions
+2. ⚠️ Flag submission uses `FlagService`, not direct model update
+3. ⚠️ `scenario` field naming differs from our assumption  
+4. ⚠️ `auto_flag_submit` endpoint requires VM name, not flag_key alone
+5. ⚠️ No `display_name` method on VmSet
+6. ✅ Console URL pattern is correct
+
+---
+
+## Model Structure Analysis
+
+### VmSet Model
+```ruby
+# Hacktivity: app/models/vm_set.rb
+class VmSet < ApplicationRecord
+  belongs_to :user, optional: true
+  belongs_to :sec_gen_batch           # <-- NOT secgen_batch
+  belongs_to :cluster, optional: true
+  belongs_to :target_node, optional: true
+  belongs_to :team, optional: true
+  has_many :vms
+end
+```
+
+**Compatibility Issue**: Our plan uses `secgen_batch` but Hacktivity uses `sec_gen_batch` (with underscore).
+
+### VM Model
+```ruby
+# Hacktivity: app/models/vm.rb
+class Vm < ApplicationRecord
+  belongs_to :vm_set
+  belongs_to :node, optional: true
+  has_many :flags
+  has_many :snapshots
+  
+  # Has ip_address field
+  # Has title field (VM name like "desktop", "server", etc.)
+end
+```
+
+**Compatibility**: ✅ Matches our assumptions - VMs have `title`, `ip_address`, and `has_many :flags`.
+
+### Flag Model
+```ruby
+# Hacktivity: app/models/flag.rb
+class Flag < ApplicationRecord
+  belongs_to :vm
+end
+
+# Schema:
+# - flag_key (string)
+# - solved (boolean)
+# - solved_date (datetime)
+# - failed_attempts (integer)
+# - vm_id (integer)
+```
+
+**Compatibility**: ✅ Matches our assumptions - flags have `flag_key`, `solved`, `solved_date`.
+
+### SecGenBatch Model
+```ruby
+# Hacktivity: app/models/sec_gen_batch.rb
+class SecGenBatch < ApplicationRecord
+  # Key field:
+  # - scenario (string) - contains path like "scenarios/ctf/foo.xml"
+  
+  belongs_to :event
+  has_many :vm_sets
+end
+```
+
+**Compatibility Issue**: 
+- Hacktivity field: `scenario` (path to XML file like `"scenarios/ctf/foo.xml"`)
+- Our plan assumes: `secgen_scenario` on Mission model
+
+This is OK - we just need to match the `scenario` field value.
+
+---
+
+## Flag Submission Analysis
+
+### FlagService (Hacktivity)
+```ruby
+# Hacktivity: app/services/flag_service.rb
+def self.process_flag(vm, submitted_flag, user, flash)
+  # 1. Find matching flags (case-insensitive)
+  vm.flags.where("lower(flag_key) = ?", submitted_flag.downcase).each do |flag|
+    if !flag.solved
+      mark_flag_as_solved!(flag: flag, vm_set: vm_set, user: user, flash: flash)
+    end
+  end
+  
+  # 2. Calculate score (percent or early-bird)
+  # 3. Update vm_set.score
+  # 4. Update user's Result
+  # 5. Send ActionCable notifications
+end
+
+def self.mark_flag_as_solved!(flag:, vm_set:, user:, flash:)
+  flag.solved = true
+  flag.solved_date = DateTime.current
+  flag.save
+  
+  # Update streaks
+  # Update vm_set first_flag_date / completed_flags_date
+end
+```
+
+**Compatibility Issue**: Our plan's direct model update approach is too simplistic. It misses:
+- Score calculation (percent-based or early-bird)
+- Streak tracking
+- Result updates  
+- ActionCable notifications
+
+**Recommendation**: Call `FlagService.process_flag()` instead of direct update.
+
+### auto_flag_submit Endpoint
+```ruby
+# Hacktivity: app/controllers/vms_controller.rb
+def auto_flag_submit
+  submitted_flag = params[:flag]
+  submitted_vmname = params[:vm_name]  # <-- Requires VM name!
+
+  if submitted_vmname =~ /server|grader/i
+    @vm = Vm.find_by(ovirt_vm_name: submitted_vmname)
+    FlagService.process_flag(@vm, submitted_flag, @vm.vm_set&.user, flash)
+  end
+end
+```
+
+**Compatibility Issue**: This endpoint expects `vm_name` (the ovirt_vm_name) and only works for server/grader VMs. It's designed for automated grading from within VMs, not for client-side submission.
+
+**Recommendation**: Don't use `auto_flag_submit`. Instead, use `flag_submit` action which is the user-facing endpoint:
+
+```ruby
+# POST /events/:event_id/challenges/:sec_gen_batch_id/vm_sets/:vm_set_id/vms/:id/flag_submit
+def flag_submit
+  authorize(@vm)
+  submitted_flag = params[:flag]
+  FlagService.process_flag(@vm, submitted_flag, current_user, flash)
+  # ...
+end
+```
+
+---
+
+## Console/VM Launch URL Analysis
+
+### ovirt_console Endpoint
+```ruby
+# Route: POST /events/:event_id/challenges/:sec_gen_batch_id/vm_sets/:vm_set_id/vms/:id/ovirt_console
+def ovirt_console
+  authorize(@vm)
+  
+  # Handles timer start for timed tests
+  # Dispatches console command asynchronously
+  # Console file is generated and stored on @vm.console_file
+end
+```
+
+**URL Helper**:
+```ruby
+ovirt_console_event_sec_gen_batch_vm_set_vm_path(event, sec_gen_batch, vm_set, vm)
+# Example: /hacktivities/5/challenges/10/vm_sets/123/vms/456/ovirt_console
+```
+
+**Compatibility**: ✅ Our plan can construct this URL, but we need:
+- `event_id` (from `vm_set.sec_gen_batch.event_id`)
+- `sec_gen_batch_id` (from `vm_set.sec_gen_batch_id`)
+- `vm_set_id`
+- `vm_id`
+
+The actual SPICE file download is handled asynchronously - clicking the button triggers the generation.
+
+---
+
+## VmSet Query Analysis
+
+### Finding User's VM Sets
+Our plan proposes:
+```ruby
+::VmSet.joins(:sec_gen_batch)
+       .where(sec_gen_batches: { scenario: secgen_scenario })
+       .where(user: user, relinquished: false)
+```
+
+**Compatibility**: ✅ This is correct, but:
+- Table name is `sec_gen_batches` (verified in schema)
+- Column is `scenario` (verified)
+- Association is `sec_gen_batch` (not `secgen_batch`)
+
+### No `display_name` Method
+Our plan's view uses:
+```erb
+<%= f.select :vm_set_id, 
+    options_from_collection_for_select(@available_vm_sets, :id, :display_name) %>
+```
+
+**Compatibility Issue**: VmSet doesn't have a `display_name` method.
+
+**Available Fields**:
+- `secgen_prefix` - e.g., `"hacktivity_5_10_0_abc123"`
+- `sec_gen_batch.title` - The challenge title
+- `vms.count` - Number of VMs
+
+**Fix**: Use a custom method or inline logic:
+```erb
+<%= f.select :vm_set_id, 
+    @available_vm_sets.map { |vs| ["#{vs.sec_gen_batch.title} (#{vs.vms.count} VMs)", vs.id] } %>
+```
+
+---
+
+## Authorization Considerations
+
+### VmPolicy
+```ruby
+def user_allocated_vm_set?
+  @vm_set = @record.vm_set
+  admin? || 
+  scoped_vip_by_user?(@vm_set.user) || 
+  (@vm_set.user == @user && @user.has_event_role?(@vm_set.sec_gen_batch.event)) ||
+  @vm_set.team&.users&.exists?(@user.id)
+end
+```
+
+**Key Requirements**:
+1. User must own the vm_set (`vm_set.user == current_user`)
+2. User must have event role (`user.has_event_role?(event)`)
+3. OR user is on the team that owns the vm_set
+4. OR user is admin/VIP
+
+**Compatibility**: ✅ Our plan correctly queries user's own vm_sets.
+
+---
+
+## Required Plan Updates
+
+### 1. Fix `submit_to_hacktivity` Method
+
+**Current (Wrong)**:
+```ruby
+def submit_to_hacktivity(flag_key)
+  flag.update!(solved: true, solved_date: Time.current)
+end
+```
+
+**Corrected**:
+```ruby
+def submit_to_hacktivity(flag_key)
+  return unless defined?(::VmSet) && player_state['vm_set_id'].present?
+  
+  vm_set = ::VmSet.find_by(id: player_state['vm_set_id'])
+  return unless vm_set
+  
+  # Find the flag and its VM
+  vm_set.vms.each do |vm|
+    flag = vm.flags.find_by("lower(flag_key) = ?", flag_key.downcase)
+    next unless flag
+    
+    # Use FlagService for proper scoring, streaks, and notifications
+    # NOTE: We pass nil for flash since we're not in a web request context
+    #       The service will still update the flag and scores
+    ::FlagService.process_flag(vm, flag_key, vm_set.user || player, OpenStruct.new(
+      :[]= => ->(k,v) { Rails.logger.info "[BreakEscape] Flag result: #{k}: #{v}" }
+    ))
+    
+    return # Only process once
+  end
+end
+```
+
+### 2. Fix VmSet Query
+
+**Current (Minor Issue)**:
+```ruby
+::VmSet.joins(:sec_gen_batch)
+```
+
+**Corrected** (already correct, just verify):
+```ruby
+::VmSet.joins(:sec_gen_batch)
+       .where(sec_gen_batches: { scenario: secgen_scenario })
+       .where(user: user, relinquished: false)
+       .includes(:vms, :sec_gen_batch)  # Add eager loading
+```
+
+### 3. Fix VM Set Display
+
+**Current (Won't Work)**:
+```erb
+options_from_collection_for_select(@available_vm_sets, :id, :display_name)
+```
+
+**Corrected**:
+```erb
+@available_vm_sets.map { |vs| 
+  ["#{vs.sec_gen_batch.title} (#{vs.vms.count} VMs)", vs.id] 
+}
+```
+
+### 4. Add VM Launch URL Helper
+
+Add a helper method to construct the console URL:
+
+```ruby
+# In Game model or a helper
+def hacktivity_console_url(vm)
+  return nil unless defined?(::Rails) && vm.respond_to?(:vm_set)
+  
+  vm_set = vm.vm_set
+  sec_gen_batch = vm_set.sec_gen_batch
+  event = sec_gen_batch.event
+  
+  # Use Hacktivity's route helpers
+  Rails.application.routes.url_helpers.ovirt_console_event_sec_gen_batch_vm_set_vm_path(
+    event, sec_gen_batch, vm_set, vm
+  )
+end
+```
+
+### 5. Update ERB VM Context Structure
+
+**Current**:
+```ruby
+context[:vms] = vm_set.vms.map do |vm|
+  {
+    id: vm.id,
+    title: vm.title,
+    description: vm.description,
+    ip_address: vm.ip_address,
+    vm_set_id: vm_set.id
+  }
+end
+```
+
+**Enhanced** (add console URL info):
+```ruby
+context[:vms] = vm_set.vms.map do |vm|
+  {
+    id: vm.id,
+    title: vm.title,
+    description: vm.description,
+    ip_address: vm.ip_address,
+    vm_set_id: vm_set.id,
+    enable_console: vm.enable_console,
+    # Store IDs needed to construct console URL client-side
+    event_id: vm_set.sec_gen_batch.event_id,
+    sec_gen_batch_id: vm_set.sec_gen_batch_id
+  }
+end
+```
+
+---
+
+## Summary of Required Changes
+
+| Item | Severity | Change Required |
+|------|----------|-----------------|
+| Flag submission method | High | Use `FlagService.process_flag()` instead of direct update |
+| VmSet display_name | Medium | Use inline map instead of collection method |
+| VM context structure | Medium | Add event_id, sec_gen_batch_id for console URLs |
+| Eager loading | Low | Add `.includes(:vms, :sec_gen_batch)` to VmSet query |
+
+---
+
+## Verified Compatible Items
+
+| Item | Status | Notes |
+|------|--------|-------|
+| VmSet → sec_gen_batch association | ✅ | Uses `belongs_to :sec_gen_batch` |
+| Vm → flags association | ✅ | Uses `has_many :flags` |
+| Flag model structure | ✅ | Has `flag_key`, `solved`, `solved_date` |
+| VmSet user ownership | ✅ | Has `belongs_to :user, optional: true` |
+| VM ip_address field | ✅ | Available on Vm model |
+| VM title field | ✅ | Available (e.g., "desktop", "server") |
+| Console endpoint | ✅ | POST to nested route works |
+| Scenario matching | ✅ | Can match on `sec_gen_batches.scenario` |
--- a/planning_notes/vms_and_flags/review3/REVIEW.md
+++ b/planning_notes/vms_and_flags/review3/REVIEW.md
@@ -0,0 +1,355 @@
+# VM and CTF Flag Integration - Plan Review 3
+
+**Reviewer**: AI Review  
+**Date**: November 27, 2025  
+**Document Reviewed**: `planning_notes/vms_and_flags/IMPLEMENTATION_PLAN.md` (post-Review 1 & 2)
+
+---
+
+## Executive Summary
+
+This review validates the solutions implemented after Reviews 1 and 2. The callback timing approach is sound, but there's one remaining critical issue: the `games#create` action doesn't actually exist in the codebase, despite the routes declaring it. Additionally, there are several assumptions that need validation.
+
+**Critical Issues**: 1  
+**Medium Issues**: 2  
+**Minor Issues**: 3  
+
+---
+
+## Issues Fixed Since Previous Reviews ✓
+
+### From Review 1
+1. ✓ Duplicate `secgen_scenario` migration removed
+2. ✓ Routes structure corrected to use `BreakEscape::Engine.routes.draw`
+3. ✓ `Game::DEFAULT_PLAYER_STATE` reference removed
+4. ✓ ERB patterns updated with null-safety
+5. ✓ ActiveRecord query corrected to use `joins`
+
+### From Review 2
+1. ✓ `window.breakEscapeConfig` extension documented (not replacement)
+2. ✓ Hacktivity flag submission approach clarified
+3. ✓ Function signature mismatch fixed
+4. ✓ Routes clarified - only `post :flags` needed
+
+---
+
+## Critical Issues
+
+### 1. ❌ `games#create` Action Does Not Exist
+
+**Problem**: The plan assumes a `games#create` action exists that can be extended:
+
+```ruby
+# Plan assumes this exists:
+def create
+  @mission = Mission.find(params[:mission_id])
+  # ...
+end
+```
+
+However, searching the codebase shows:
+- `config/routes.rb` declares `resources :games, only: [:show, :create]`
+- `app/controllers/break_escape/games_controller.rb` does NOT have a `create` method
+- Games are actually created in `MissionsController#show` via `find_or_create_by!`
+
+```ruby
+# MissionsController#show (actual code)
+def show
+  @mission = Mission.find(params[:id])
+  authorize @mission if defined?(Pundit)
+
+  @game = Game.find_or_create_by!(
+    player: current_player,
+    mission: @mission
+  )
+
+  redirect_to game_path(@game)
+end
+```
+
+**Impact**: The plan's `games#create` implementation has nowhere to go. The VM context won't be passed during game creation.
+
+**Callback Timing Analysis**:
+The plan proposes:
+1. Set `@game.player_state = initial_player_state` (with `vm_set_id`)
+2. Call `@game.save!`
+3. `before_create :generate_scenario_data_with_context` reads `player_state['vm_set_id']`
+4. `before_create :initialize_player_state` adds defaults
+
+This timing IS correct - attributes set before `save!` are available to `before_create` callbacks. **The approach is sound**, but needs an actual controller action to implement it.
+
+**Fix Options**:
+
+**Option A (Recommended)**: Implement the missing `games#create` action
+
+```ruby
+# app/controllers/break_escape/games_controller.rb
+def create
+  @mission = Mission.find(params[:mission_id])
+  authorize @mission if defined?(Pundit)
+  
+  initial_player_state = {}
+  
+  if params[:vm_set_id].present? && defined?(::VmSet)
+    vm_set = ::VmSet.find(params[:vm_set_id])
+    initial_player_state['vm_set_id'] = vm_set.id
+  end
+  
+  if params[:standalone_flags].present?
+    initial_player_state['standalone_flags'] = Array(params[:standalone_flags])
+  end
+  
+  @game = Game.new(
+    player: current_player,
+    mission: @mission
+  )
+  @game.player_state = initial_player_state
+  @game.save!
+  
+  redirect_to game_path(@game)
+end
+```
+
+**Option B**: Modify `MissionsController#show` to accept params
+
+```ruby
+# MissionsController#show
+def show
+  @mission = Mission.find(params[:id])
+  authorize @mission if defined?(Pundit)
+  
+  initial_player_state = {}
+  initial_player_state['vm_set_id'] = params[:vm_set_id] if params[:vm_set_id].present?
+  
+  # Can't use find_or_create_by! with pre-set player_state
+  @game = Game.find_by(player: current_player, mission: @mission)
+  
+  if @game.nil?
+    @game = Game.new(player: current_player, mission: @mission)
+    @game.player_state = initial_player_state
+    @game.save!
+  end
+  
+  redirect_to game_path(@game)
+end
+```
+
+**Option C**: Create a new action specifically for VM games
+
+```ruby
+# MissionsController
+def create_vm_game
+  @mission = Mission.find(params[:id])
+  # ...
+end
+```
+
+**Recommendation**: Use Option A. It aligns with the existing routes (`resources :games, only: [:show, :create]`) and provides a clear separation between viewing missions and creating games.
+
+---
+
+## Medium Issues
+
+### 2. ⚠️ Unique Index Removal Timing
+
+**Problem**: The plan removes the unique index on `[player_type, player_id, mission_id]`:
+
+```ruby
+remove_index :break_escape_games,
+             name: 'index_games_on_player_and_mission',
+             if_exists: true
+```
+
+The current `MissionsController#show` uses `find_or_create_by!`. If a user visits a mission page:
+1. Before migration: One game per player+mission (enforced by unique index)
+2. After migration: Still one game (due to `find_or_create_by!` behavior)
+
+But if `games#create` is called with different `vm_set_id`:
+- Before migration: Database error (unique constraint violation)
+- After migration: New game created successfully
+
+**Risk**: If migration runs but controller changes aren't deployed simultaneously, existing behavior changes unexpectedly.
+
+**Fix**: Document deployment order:
+1. Deploy controller changes (games#create action) as disabled/hidden
+2. Run migration to remove unique index
+3. Enable/expose the new functionality
+
+---
+
+### 3. ⚠️ `MissionsController#show` Still Uses `find_or_create_by!`
+
+**Problem**: Even after implementing `games#create`, `MissionsController#show` still auto-creates games:
+
+```ruby
+@game = Game.find_or_create_by!(player: current_player, mission: @mission)
+```
+
+This creates games WITHOUT VM context when users directly visit `/missions/:id`.
+
+**Scenarios**:
+- User visits `/missions/5` → Game created without vm_set_id
+- User visits `/games/new?mission_id=5&vm_set_id=123` → Game created WITH vm_set_id
+- User visits `/missions/5` again → Gets the first game (no VMs)
+
+**Fix**: Update `MissionsController#show` for VM-required missions:
+
+```ruby
+def show
+  @mission = Mission.find(params[:id])
+  authorize @mission if defined?(Pundit)
+
+  if @mission.requires_vms?
+    # Redirect to game selection/creation page for VM missions
+    redirect_to new_game_path(mission_id: @mission.id)
+  else
+    # Legacy behavior for non-VM missions
+    @game = Game.find_or_create_by!(player: current_player, mission: @mission)
+    redirect_to game_path(@game)
+  end
+end
+```
+
+Or if always going through explicit creation:
+```ruby
+def show
+  @mission = Mission.find(params[:id])
+  # Don't auto-create, show mission info page
+  # with "Start Game" button that POSTs to games#create
+end
+```
+
+---
+
+## Minor Issues
+
+### 4. 📝 `new` Action Not Defined for Game Selection
+
+The fix for Medium Issue #3 suggests redirecting to `new_game_path(mission_id:)`, but there's no `games#new` action defined in the routes or controller.
+
+**Fix**: Add to routes and controller:
+```ruby
+# routes.rb
+resources :games, only: [:new, :show, :create] do
+  # ...
+end
+
+# games_controller.rb
+def new
+  @mission = Mission.find(params[:mission_id])
+  authorize @mission if defined?(Pundit)
+  
+  if @mission.requires_vms?
+    @available_vm_sets = @mission.valid_vm_sets_for_user(current_user)
+    @existing_games = Game.where(player: current_player, mission: @mission)
+  end
+  
+  # Render game selection/creation page
+end
+```
+
+---
+
+### 5. 📝 Missing View for Game Selection Page
+
+If users need to select a VM set before starting, a view is needed:
+
+```erb
+<%# app/views/break_escape/games/new.html.erb %>
+<h1><%= @mission.name %></h1>
+
+<% if @mission.requires_vms? %>
+  <h2>Select VM Set</h2>
+  <% @available_vm_sets.each do |vm_set| %>
+    <%= button_to "Start with #{vm_set.name}", 
+                  games_path(mission_id: @mission.id, vm_set_id: vm_set.id),
+                  method: :post %>
+  <% end %>
+  
+  <% if @available_vm_sets.empty? %>
+    <p>No VM sets available. Please provision VMs first.</p>
+  <% end %>
+<% else %>
+  <%= button_to "Start Game", 
+                games_path(mission_id: @mission.id),
+                method: :post %>
+<% end %>
+```
+
+---
+
+### 6. 📝 Strong Parameters for `games#create`
+
+The plan doesn't show strong parameters for the create action. Rails best practice:
+
+```ruby
+private
+
+def game_create_params
+  params.permit(:mission_id, :vm_set_id, standalone_flags: [])
+end
+```
+
+---
+
+## Validation Checklist
+
+| Item | Status | Notes |
+|------|--------|-------|
+| Callback timing logic | ✓ Correct | Pre-set player_state available to before_create |
+| Route structure | ✓ Correct | Only add `post :flags` |
+| ERB null-safety patterns | ✓ Correct | Uses `&.`, `dig`, `\|\| 'null'` |
+| `window.breakEscapeConfig` | ✓ Correct | Now documented as extension |
+| Minigame framework usage | ✓ Correct | `startMinigame(type, null, params)` |
+| Hacktivity flag submission | ✓ Correct | Direct model update approach |
+| `games#create` action | ❌ Missing | Must be implemented |
+| `games#new` action | ❌ Missing | Needed for VM selection flow |
+
+---
+
+## Summary of Required Changes
+
+### Critical (Must Fix Before Implementation)
+
+1. **Add `games#create` action** to `app/controllers/break_escape/games_controller.rb`
+   - Accept `mission_id`, `vm_set_id`, `standalone_flags` params
+   - Set `player_state` before `save!`
+   - Handle authorization
+
+### Medium (Should Fix)
+
+2. **Update `MissionsController#show`** to handle VM-required missions differently
+3. **Document migration deployment order** to avoid race conditions
+
+### Optional (Nice to Have)
+
+4. Add `games#new` action and view for VM set selection
+5. Add strong parameters for `games#create`
+
+---
+
+## Revised Implementation Order
+
+1. **Phase 1: Controller Infrastructure**
+   - Implement `games#create` action
+   - Add `games#new` action and view (optional but recommended)
+   - Update `MissionsController#show` for VM missions
+
+2. **Phase 2: Database Migration**
+   - Run migration to remove unique index
+   - Verify existing games unaffected
+
+3. **Phase 3: Model Changes**
+   - Add `generate_scenario_data_with_context` callback
+   - Add `build_vm_context` method
+   - Add flag submission methods
+
+4. **Phase 4: Frontend**
+   - Implement VM launcher minigame
+   - Implement flag station minigame
+   - Update show.html.erb config
+
+5. **Phase 5: Integration**
+   - Update scenario templates with VM ERB syntax
+   - Add scenarios using new features
+   - Test end-to-end flow