fix: use rename-then-move to replace exe on Windows

Author: om-mapari

CHANGELOG.md

@@ -5,6 +5,11 @@ All notable changes to Shello CLI will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [0.7.1] - 2026-03-23
+
+### Fixed
+- Update command: replace running exe via rename-then-move on Windows to avoid `[Errno 13] Permission denied` on locked executables
+
 ## [0.7.0] - 2026-03-23
 
 ### Fixed

shello_cli/__init__.py

@@ -1,4 +1,4 @@
 """Shello CLI
 AI Assistant with Command Execution"""
 
-__version__ = "0.7.0"
+__version__ = "0.7.1"

shello_cli/update/executable_updater.py

@@ -117,48 +117,52 @@ def replace_executable(self, new_binary_path: str, target_path: str) -> None:
         Raises:
             UpdateError: If replacement fails
         """
-        backup_path = f"{target_path}.backup"
+        old_path = f"{target_path}.old"
         new_binary_moved = False
-        
+
         try:
-            # Step 1: Create backup of current executable
-            if os.path.exists(target_path):
-                shutil.copy2(target_path, backup_path)
-            
-            # Step 2: Replace executable with new binary
+            # Step 1: On Windows, rename the running exe out of the way first.
+            # Windows locks running executables against overwrite but allows rename.
+            if os.name == 'nt':
+                if os.path.exists(old_path):
+                    os.remove(old_path)
+                if os.path.exists(target_path):
+                    os.rename(target_path, old_path)
+
+            # Step 2: Move new binary into place
             shutil.move(new_binary_path, target_path)
             new_binary_moved = True
-            
+
             # Step 3: Set executable permissions on Unix systems
-            if os.name != 'nt':  # Not Windows
+            if os.name != 'nt':
                 current_permissions = os.stat(target_path).st_mode
                 os.chmod(target_path, current_permissions | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
-            
-            # Step 4: Remove backup on success
-            if os.path.exists(backup_path):
-                os.remove(backup_path)
-                
+
+            # Step 4: Remove old binary on success (best effort — may still be locked)
+            if os.path.exists(old_path):
+                try:
+                    os.remove(old_path)
+                except Exception:
+                    pass  # Will be cleaned up on next run
+
         except Exception as e:
-            # Restore from backup on failure
-            if os.path.exists(backup_path):
+            # Restore old binary on failure
+            if os.name == 'nt' and os.path.exists(old_path):
                 try:
-                    # Only restore if we successfully moved the new binary
-                    if new_binary_moved:
-                        shutil.move(backup_path, target_path)
-                    else:
-                        # If move failed, just remove the backup
-                        os.remove(backup_path)
+                    if new_binary_moved and os.path.exists(target_path):
+                        os.remove(target_path)
+                    os.rename(old_path, target_path)
                 except Exception as restore_error:
                     raise UpdateError(
                         f"Failed to replace executable and restore backup: {e}. "
                         f"Restore error: {restore_error}"
                     )
-            
-            # Clean up new binary if it exists and wasn't moved
+
+            # Clean up new binary if it wasn't moved
             if not new_binary_moved and os.path.exists(new_binary_path):
                 try:
                     os.remove(new_binary_path)
                 except Exception:
-                    pass  # Best effort cleanup
-            
+                    pass
+
             raise UpdateError(f"Failed to replace executable: {e}")

tests/test_executable_updater.py

@@ -173,190 +173,149 @@ def test_verify_binary_nonexistent_file(self):
     def test_replace_executable_success(self):
         """Test successful executable replacement."""
         updater = ExecutableUpdater("test-owner", "test-repo")
-        
-        # Create temporary files
+
         with tempfile.NamedTemporaryFile(delete=False, mode='w') as old_file:
             old_file.write("old executable")
             old_path = old_file.name
-        
+
         with tempfile.NamedTemporaryFile(delete=False, mode='w') as new_file:
             new_file.write("new executable")
             new_path = new_file.name
-        
-        backup_path = f"{old_path}.backup"
-        
+
+        old_bak = f"{old_path}.old"
+
         try:
-            # Replace executable
             updater.replace_executable(new_path, old_path)
-            
-            # Verify new content
+
             with open(old_path, 'r') as f:
                 content = f.read()
             assert content == "new executable"
-            
-            # Verify backup was removed
-            assert not os.path.exists(backup_path)
-            
-            # Verify new binary was moved (not copied)
+
+            # .old file cleaned up on success
+            assert not os.path.exists(old_bak)
+            # new binary was moved, not copied
             assert not os.path.exists(new_path)
-            
+
         finally:
-            # Cleanup
-            if os.path.exists(old_path):
-                os.remove(old_path)
-            if os.path.exists(new_path):
-                os.remove(new_path)
-            if os.path.exists(backup_path):
-                os.remove(backup_path)
-
-    def test_replace_executable_creates_backup(self):
-        """Test that backup is created before replacement."""
+            for p in [old_path, new_path, old_bak]:
+                if os.path.exists(p):
+                    os.remove(p)
+
+    def test_replace_executable_uses_rename_on_windows(self):
+        """On Windows the old exe is renamed before placing the new one (avoids lock)."""
         updater = ExecutableUpdater("test-owner", "test-repo")
-        
-        # Create temporary files
+
         with tempfile.NamedTemporaryFile(delete=False, mode='w') as old_file:
             old_file.write("old executable")
             old_path = old_file.name
-        
+
         with tempfile.NamedTemporaryFile(delete=False, mode='w') as new_file:
             new_file.write("new executable")
             new_path = new_file.name
-        
-        backup_path = f"{old_path}.backup"
-        
-        # Patch shutil.copy2 to verify backup is created
-        original_copy2 = __import__('shutil').copy2
-        copy2_called = [False]
-        
-        def mock_copy2(src, dst):
-            copy2_called[0] = True
-            return original_copy2(src, dst)
-        
+
+        old_bak = f"{old_path}.old"
+        rename_calls = []
+        original_rename = os.rename
+
+        def tracking_rename(src, dst):
+            rename_calls.append((src, dst))
+            return original_rename(src, dst)
+
         try:
-            with patch('shutil.copy2', side_effect=mock_copy2):
+            with patch('os.name', 'nt'), patch('os.rename', side_effect=tracking_rename):
                 updater.replace_executable(new_path, old_path)
-            
-            # Verify backup was created during the process
-            assert copy2_called[0], "Backup creation (copy2) was not called"
-            
-            # Verify final state: new content in place, backup removed
+
+            # rename should have been called to move old exe out of the way
+            assert any(dst == old_bak for _, dst in rename_calls), \
+                "Expected old exe to be renamed to .old before replacement"
+
             with open(old_path, 'r') as f:
-                content = f.read()
-            assert content == "new executable"
-            assert not os.path.exists(backup_path)
-            
+                assert f.read() == "new executable"
+
         finally:
-            # Cleanup
-            if os.path.exists(old_path):
-                os.remove(old_path)
-            if os.path.exists(new_path):
-                os.remove(new_path)
-            if os.path.exists(backup_path):
-                os.remove(backup_path)
+            for p in [old_path, new_path, old_bak]:
+                if os.path.exists(p):
+                    os.remove(p)
 
     def test_replace_executable_restores_on_failure(self):
-        """Test that backup is restored when replacement fails."""
+        """Test that old exe is restored when replacement fails."""
         updater = ExecutableUpdater("test-owner", "test-repo")
-        
-        # Create temporary files
+
         with tempfile.NamedTemporaryFile(delete=False, mode='w') as old_file:
             old_file.write("original content")
             old_path = old_file.name
-        
+
         with tempfile.NamedTemporaryFile(delete=False, mode='w') as new_file:
             new_file.write("new content")
             new_path = new_file.name
-        
-        backup_path = f"{old_path}.backup"
-        
+
+        old_bak = f"{old_path}.old"
+
         try:
-            # Patch shutil.move to fail during replacement
-            with patch('shutil.move', side_effect=OSError("Simulated disk full")):
+            with patch('os.name', 'nt'), patch('shutil.move', side_effect=OSError("Simulated disk full")):
                 with pytest.raises(UpdateError) as exc_info:
                     updater.replace_executable(new_path, old_path)
-                
+
                 assert "Failed to replace executable" in str(exc_info.value)
-            
-            # Verify original file still exists (backup was removed after restore)
+
+            # Original file should be restored
             assert os.path.exists(old_path)
             with open(old_path, 'r') as f:
-                content = f.read()
-            assert content == "original content"
-            
-            # Verify backup was removed after restore
-            assert not os.path.exists(backup_path)
-            
+                assert f.read() == "original content"
+
+            assert not os.path.exists(old_bak)
+
         finally:
-            # Cleanup
-            if os.path.exists(old_path):
-                os.remove(old_path)
-            if os.path.exists(new_path):
-                os.remove(new_path)
-            if os.path.exists(backup_path):
-                os.remove(backup_path)
+            for p in [old_path, new_path, old_bak]:
+                if os.path.exists(p):
+                    os.remove(p)
 
     @patch('os.name', 'posix')
     @patch('os.chmod')
     def test_replace_executable_sets_permissions_unix(self, mock_chmod):
         """Test that executable permissions are set on Unix systems."""
         updater = ExecutableUpdater("test-owner", "test-repo")
-        
-        # Create temporary files
+
         with tempfile.NamedTemporaryFile(delete=False, mode='w') as old_file:
             old_file.write("old")
             old_path = old_file.name
-        
+
         with tempfile.NamedTemporaryFile(delete=False, mode='w') as new_file:
             new_file.write("new")
             new_path = new_file.name
-        
+
         try:
             updater.replace_executable(new_path, old_path)
-            
-            # Verify chmod was called
             assert mock_chmod.called
-            
+
         finally:
-            # Cleanup
-            if os.path.exists(old_path):
-                os.remove(old_path)
-            if os.path.exists(new_path):
-                os.remove(new_path)
-            if os.path.exists(f"{old_path}.backup"):
-                os.remove(f"{old_path}.backup")
+            for p in [old_path, new_path, f"{old_path}.old"]:
+                if os.path.exists(p):
+                    os.remove(p)
 
     def test_replace_executable_cleans_up_on_failure(self):
         """Test that temporary files are cleaned up when replacement fails."""
         updater = ExecutableUpdater("test-owner", "test-repo")
-        
-        # Create temporary files
+
         with tempfile.NamedTemporaryFile(delete=False, mode='w') as old_file:
             old_file.write("old")
             old_path = old_file.name
-        
+
         with tempfile.NamedTemporaryFile(delete=False, mode='w') as new_file:
             new_file.write("new")
             new_path = new_file.name
-        
-        backup_path = f"{old_path}.backup"
-        
+
+        old_bak = f"{old_path}.old"
+
         try:
-            # Force failure during replacement
-            with patch('shutil.move', side_effect=OSError("Disk full")):
+            with patch('os.name', 'nt'), patch('shutil.move', side_effect=OSError("Disk full")):
                 with pytest.raises(UpdateError):
                     updater.replace_executable(new_path, old_path)
-            
-            # Verify cleanup: backup should be removed after restore
-            assert not os.path.exists(backup_path)
-            
-            # Original file should be restored
+
+            assert not os.path.exists(old_bak)
             assert os.path.exists(old_path)
-            
+
         finally:
-            # Cleanup
-            if os.path.exists(old_path):
-                os.remove(old_path)
-            if os.path.exists(new_path):
-                os.remove(new_path)
-            if os.path.exists(backup_path):
-                os.remove(backup_path)
+            for p in [old_path, new_path, old_bak]:
+                if os.path.exists(p):
+                    os.remove(p)