fix(cli): activate contract migration in riverctl room operations (#193)

* fix(cli): activate contract migration in riverctl room operations

The ensure_room_migrated() function was fully implemented but never
called. When bundled WASM changed (e.g., after a release), riverctl
would try to GET/UPDATE the new contract key which had no state yet,
causing timeouts.

Two bugs fixed:
1. Wire ensure_room_migrated() into get_room() and subscribe_and_stream()
   so migration runs automatically before any room operation.
2. Fix the migration-needed check: load_rooms() already regenerates the
   stored key to match current WASM, so comparing stored vs expected was
   always equal. Now checks previous_contract_key instead.

Closes #192

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor(cli): simplify migration log and test assertions

- Replace unwrap_or("unknown") with unwrap() since we already early-return
  when previous_contract_key_str is None
- Simplify test assertions using as_deref() pattern
- Remove unnecessary mut binding in test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(cli): remove double migration call in subscribe_and_stream

subscribe_and_stream called ensure_room_migrated then get_room which
also calls ensure_room_migrated. Removed the explicit call since
get_room handles migration internally. Also added assertion that
previous_contract_key is None for rooms with current WASM.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(cli): address review findings from skeptical reviewer

1. Clear previous_contract_key after ALL successful migration paths
   (timeout and catch-all), not just the "already exists" path.
   Prevents redundant migration re-attempts on subsequent calls.

2. Restore "Room not found" guard in subscribe_and_stream that was
   lost when refactoring to use get_room for migration.

3. Fix misleading test comment about load_rooms behavior during
   add_room (file doesn't exist yet, so no regeneration occurs).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: sanity

cli/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "riverctl"
-version = "0.1.50"
+version = "0.1.51"
 edition = "2021"
 authors = ["Freenet Project"]
 description = "Command-line interface for River decentralized chat on Freenet"

cli/src/api.rs

@@ -353,7 +353,10 @@ impl ApiClient {
         room_owner_key: &VerifyingKey,
         subscribe: bool,
     ) -> Result<ChatRoomStateV1> {
-        let contract_key = self.owner_vk_to_contract_key(room_owner_key);
+        // Ensure room is migrated to the current contract version before any GET.
+        // This handles the case where bundled WASM changed (e.g., after a release)
+        // and no other client has migrated the state to the new contract key yet.
+        let contract_key = self.ensure_room_migrated(room_owner_key).await?;
         info!("Getting room state for contract: {}", contract_key.id());
 
         let get_request = ContractRequest::Get {
@@ -865,20 +868,23 @@ impl ApiClient {
 
         let signing_key = SigningKey::from_bytes(&room_info.signing_key_bytes);
         let room_state = room_info.state.clone();
-        let stored_contract_key_str = room_info.contract_key.clone();
         let previous_contract_key_str = room_info.previous_contract_key.clone();
 
-        // Check if migration is needed
-        let expected_key_str = expected_key.id().to_string();
-        if stored_contract_key_str == expected_key_str {
-            // Already on current contract version
+        // Check if migration is needed. load_rooms() already regenerates the
+        // contract_key to match the current WASM and saves the old key in
+        // previous_contract_key. If previous_contract_key is None, the room
+        // is already on the current contract version.
+        if previous_contract_key_str.is_none() {
             return Ok(expected_key);
         }
 
+        // Safe to unwrap: we returned early above when previous_contract_key_str is None.
+        let prev_key_str = previous_contract_key_str.as_deref().unwrap();
+        let new_key_display = expected_key.id().to_string();
         info!(
             "Room contract version changed, migrating: {} -> {}",
-            &stored_contract_key_str[..12.min(stored_contract_key_str.len())],
-            &expected_key_str[..12.min(expected_key_str.len())]
+            &prev_key_str[..12.min(prev_key_str.len())],
+            &new_key_display[..12.min(new_key_display.len())]
         );
 
         // Try to GET from the new contract first - maybe someone else already migrated
@@ -925,6 +931,7 @@ impl ApiClient {
                         &result,
                     )
                     .await;
+                    self.clear_previous_contract_key(room_owner_key)?;
                     return Ok(result);
                 }
             };
@@ -935,7 +942,6 @@ impl ApiClient {
                 info!("New contract already exists, updating local reference");
                 self.storage
                     .update_contract_key(room_owner_key, &expected_key)?;
-                // Clear previous_contract_key since migration is complete
                 self.clear_previous_contract_key(room_owner_key)?;
                 Ok(expected_key)
             }
@@ -961,6 +967,7 @@ impl ApiClient {
                     &result,
                 )
                 .await;
+                self.clear_previous_contract_key(room_owner_key)?;
                 Ok(result)
             }
         }
@@ -1778,7 +1785,9 @@ impl ApiClient {
         self.send_delta(room_owner_key, delta).await
     }
 
-    /// Helper to send a delta to the network
+    /// Helper to send a delta to the network.
+    /// Assumes migration has already been triggered by the caller (via get_room
+    /// or ensure_room_migrated), so owner_vk_to_contract_key returns the correct key.
     async fn send_delta(
         &self,
         room_owner_key: &VerifyingKey,
@@ -2408,16 +2417,11 @@ impl ApiClient {
         initial_messages: usize,
         format: OutputFormat,
     ) -> Result<()> {
-        // Get the contract key for the room
-        let room = self.storage.get_room(room_owner_key)?.ok_or_else(|| {
+        // Verify room exists in local storage before attempting to subscribe
+        self.storage.get_room(room_owner_key)?.ok_or_else(|| {
             anyhow!("Room not found in local storage. You may need to create or join it first.")
         })?;
 
-        let (_signing_key, _room_state, contract_key_str) = room;
-        // Parse the stored contract key string as a ContractInstanceId
-        let contract_instance_id = ContractInstanceId::try_from(contract_key_str.clone())
-            .map_err(|e| anyhow!("Invalid contract key: {}", e))?;
-
         // Print header for human format
         if matches!(format, OutputFormat::Human) {
             eprintln!(
@@ -2431,9 +2435,10 @@ impl ApiClient {
         let mut new_message_count = 0;
         let start_time = std::time::Instant::now();
 
-        // Always fetch current room state to pre-populate seen_messages,
-        // so that the first subscription update (which may be a full state)
-        // doesn't dump all existing messages as "new".
+        // Fetch current room state to pre-populate seen_messages and trigger
+        // migration if needed (get_room calls ensure_room_migrated internally).
+        let contract_key = self.owner_vk_to_contract_key(room_owner_key);
+        let contract_instance_id = *contract_key.id();
         {
             let room_state = self.get_room(room_owner_key, false).await?;
 

cli/src/storage.rs

@@ -347,6 +347,58 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_load_rooms_sets_previous_contract_key_on_mismatch() {
+        let (storage, _temp_dir) = create_test_storage();
+        let owner_sk = create_test_signing_key();
+        let owner_vk = owner_sk.verifying_key();
+        let state = create_test_state(&owner_sk);
+
+        // Store with a fake old contract key (simulating a WASM change)
+        let fake_old_key = {
+            let code = freenet_stdlib::prelude::ContractCode::from(vec![1u8; 100]);
+            let params = freenet_stdlib::prelude::Parameters::from(vec![1u8]);
+            ContractKey::from_params_and_code(params, &code)
+        };
+        storage
+            .add_room(&owner_vk, &owner_sk, state, &fake_old_key)
+            .unwrap();
+
+        // Verify initial state: add_room stores the fake_old_key verbatim (the file
+        // doesn't exist yet when load_rooms runs inside add_room, so no regeneration).
+        let raw_storage: RoomStorage =
+            serde_json::from_str(&std::fs::read_to_string(&storage.storage_path).unwrap()).unwrap();
+        let owner_key_str = bs58::encode(owner_vk.as_bytes()).into_string();
+        assert_eq!(
+            raw_storage.rooms[&owner_key_str].previous_contract_key,
+            None
+        );
+
+        // Now load_rooms should detect the mismatch and set previous_contract_key
+        let loaded = storage.load_rooms().unwrap();
+        let room_info = loaded.rooms.get(&owner_key_str).unwrap();
+
+        // The contract key should now be the current WASM-derived key
+        let expected = expected_contract_key(&owner_vk);
+        assert_eq!(room_info.contract_key, expected.id().to_string());
+
+        // previous_contract_key should be set to the fake old key
+        assert_eq!(
+            room_info.previous_contract_key.as_deref(),
+            Some(fake_old_key.id().to_string().as_str())
+        );
+
+        // Verify the update was persisted to disk
+        let persisted: RoomStorage =
+            serde_json::from_str(&std::fs::read_to_string(&storage.storage_path).unwrap()).unwrap();
+        assert_eq!(
+            persisted.rooms[&owner_key_str]
+                .previous_contract_key
+                .as_deref(),
+            Some(fake_old_key.id().to_string().as_str())
+        );
+    }
+
     #[test]
     fn test_storage_roundtrip() {
         let (storage, _temp_dir) = create_test_storage();
@@ -371,5 +423,15 @@ mod tests {
             retrieved_state.configuration.configuration.max_members,
             state.configuration.configuration.max_members
         );
+
+        // When WASM hasn't changed, previous_contract_key must be None
+        // (ensures ensure_room_migrated returns early without network calls)
+        let loaded = storage.load_rooms().unwrap();
+        let owner_key_str = bs58::encode(owner_vk.as_bytes()).into_string();
+        assert_eq!(
+            loaded.rooms[&owner_key_str].previous_contract_key,
+            None,
+            "previous_contract_key should be None when WASM hasn't changed"
+        );
     }
 }