Rename get_last_history to pop_last_history and fix related issues (#85)

* Update FileHistoryManager to return actual metadata in get_metadata method

* Update test_history.py

* Rename get_last_history to pop_last_history for clarity

* Add debug logging to FileHistoryManager and test file

* rename method and fix lint

* fix lint

* fix intg test

* add multiple undo test case

* remove prints

* bump version

---------

Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: Hoang Tran <descience.thh10@gmail.com>

Author: 3 people

openhands_aci/editor/editor.py

@@ -426,7 +426,7 @@ def undo_edit(self, path: Path) -> CLIResult:
         Implement the undo_edit command.
         """
         current_text = self.read_file(path).expandtabs()
-        old_text = self._history_manager.get_last_history(path)
+        old_text = self._history_manager.pop_last_history(path)
         if old_text is None:
             raise ToolError(f'No edit history found for {path}.')
 

openhands_aci/editor/history.py

@@ -58,26 +58,38 @@ def add_history(self, file_path: Path, content: str):
 
         self.cache.set(metadata_key, metadata)
 
-    def get_last_history(self, file_path: Path) -> Optional[str]:
-        """Get the most recent history entry for a file."""
+    def pop_last_history(self, file_path: Path) -> Optional[str]:
+        """Pop and return the most recent history entry for a file."""
         metadata_key = self._get_metadata_key(file_path)
         metadata = self.cache.get(metadata_key, {'entries': [], 'counter': 0})
         entries = metadata['entries']
 
         if not entries:
             return None
 
-        # Get the last entry without removing it
-        last_counter = entries[-1]
+        # Pop and remove the last entry
+        last_counter = entries.pop()
         history_key = self._get_history_key(file_path, last_counter)
         content = self.cache.get(history_key)
 
         if content is None:
             self.logger.warning(f'History entry not found for {file_path}')
-            return None
+        else:
+            # Remove the entry from the cache
+            self.cache.delete(history_key)
+
+        # Update metadata
+        metadata['entries'] = entries
+        self.cache.set(metadata_key, metadata)
 
         return content
 
+    def get_metadata(self, file_path: Path):
+        """Get metadata for a file (for testing purposes)."""
+        metadata_key = self._get_metadata_key(file_path)
+        metadata = self.cache.get(metadata_key, {'entries': [], 'counter': 0})
+        return metadata  # Return the actual metadata, not a copy
+
     def clear_history(self, file_path: Path):
         """Clear history for a given file."""
         metadata_key = self._get_metadata_key(file_path)
@@ -105,8 +117,3 @@ def get_all_history(self, file_path: Path) -> List[str]:
                 history.append(content)
 
         return history
-
-    def get_metadata(self, file_path: Path):
-        """Get metadata for a file (for testing purposes)."""
-        metadata_key = self._get_metadata_key(file_path)
-        return self.cache.get(metadata_key, {'entries': [], 'counter': 0})

poetry.lock

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "openhands-aci"
-version = "0.2.4"
+version = "0.2.5"
 description = "An Agent-Computer Interface (ACI) designed for software development agents OpenHands."
 authors = ["OpenHands"]
 license = "MIT"
@@ -26,8 +26,9 @@ grep-ast = "0.3.3"
 flake8 = "*"
 whatthepatch = "^1.0.6"
 binaryornot = "^0.4.4"
-pytest = "^8.3.4"
-pre-commit = "^4.1.0"
+
+
+
 
 
 [tool.poetry.group.dev.dependencies]

pyproject.toml

@@ -241,22 +241,23 @@ def test_large_history_insert():
         large_content = 'x' * (1024 * 1024)
 
         # Try to insert the large content multiple times
-        for i in range(100):
+        num_files = 100
+        for i in range(num_files):
             try:
                 manager.add_history(Path(f'test_file_{i}.txt'), large_content)
             except Exception as e:
                 pytest.fail(f'Error occurred on iteration {i}: {str(e)}')
 
         # Check if we can still retrieve the last entry
-        last_content = manager.get_last_history(Path('test_file_99.txt'))
+        last_content = manager.pop_last_history(Path(f'test_file_{num_files - 1}.txt'))
         assert (
             last_content == large_content
         ), 'Failed to retrieve the last inserted content'
 
         # Check if the number of cache entries is correct
         cache_entries = list(manager.cache)
         assert (
-            len(cache_entries) == 200
-        ), f'Expected 200 cache entries (100 content + 100 metadata), but found {len(cache_entries)}'
-
-    print('Large history insert test completed successfully')
+            len(cache_entries)
+            == num_files * 2
+            - 1  # The cache entry for file content was removed, only metadata remains
+        ), f'Expected {num_files * 2 - 1} cache entries ({num_files - 1} content + {num_files} metadata), but found {len(cache_entries)}'

tests/integration/editor/test_peak_memory.py

@@ -356,6 +356,35 @@ def test_undo_edit(editor):
     assert 'test file' in test_file.read_text()  # Original content restored
 
 
+def test_multiple_undo_edits(editor):
+    editor, test_file = editor
+    # Make an edit to be undone
+    _ = editor(
+        command='str_replace',
+        path=str(test_file),
+        old_str='test file',
+        new_str='sample file v1',
+    )
+    # Make another edit to be undone
+    _ = editor(
+        command='str_replace',
+        path=str(test_file),
+        old_str='sample file v1',
+        new_str='sample file v2',
+    )
+    # Undo the last edit
+    result = editor(command='undo_edit', path=str(test_file))
+    assert isinstance(result, CLIResult)
+    assert 'Last edit to' in result.output
+    assert 'sample file v1' in test_file.read_text()  # Previous content restored
+
+    # Undo the first edit
+    result = editor(command='undo_edit', path=str(test_file))
+    assert isinstance(result, CLIResult)
+    assert 'Last edit to' in result.output
+    assert 'test file' in test_file.read_text()  # Original content restored
+
+
 def test_validate_path_invalid(editor):
     editor, test_file = editor
     invalid_file = test_file.parent / 'nonexistent.txt'

tests/integration/test_oh_editor.py

@@ -100,3 +100,43 @@ def test_clear_history_resets_counter():
         metadata = manager.get_metadata(path)
         assert len(metadata['entries']) == 1
         assert metadata['entries'][0] == 0  # First key should be 0
+
+
+def test_pop_last_history_removes_entry():
+    """Test that pop_last_history removes the latest entry."""
+    with tempfile.NamedTemporaryFile() as temp_file:
+        path = Path(temp_file.name)
+        manager = FileHistoryManager()
+
+        # Add some entries
+        manager.add_history(path, 'content1')
+        manager.add_history(path, 'content2')
+        manager.add_history(path, 'content3')
+
+        # Pop the last history entry
+        last_entry = manager.pop_last_history(path)
+        assert last_entry == 'content3'
+
+        # Check that the entry has been removed
+        metadata = manager.get_metadata(path)
+        assert len(metadata['entries']) == 2
+
+        # Pop the last history entry again
+        last_entry = manager.pop_last_history(path)
+        assert last_entry == 'content2'
+
+        # Check that the entry has been removed
+        metadata = manager.get_metadata(path)
+        assert len(metadata['entries']) == 1
+
+        # Pop the last history entry one more time
+        last_entry = manager.pop_last_history(path)
+        assert last_entry == 'content1'
+
+        # Check that all entries have been removed
+        metadata = manager.get_metadata(path)
+        assert len(metadata['entries']) == 0
+
+        # Try to pop last history when there are no entries
+        last_entry = manager.pop_last_history(path)
+        assert last_entry is None