feat(tools/gemini): declare file-path resources for parallel execution (#2723)

Author: VascoSch92

openhands-tools/openhands/tools/gemini/edit/definition.py

@@ -1,13 +1,15 @@
 """Edit tool definition (Gemini-style)."""
 
 from collections.abc import Sequence
+from pathlib import Path
 from typing import TYPE_CHECKING
 
 from pydantic import Field, PrivateAttr
 from rich.text import Text
 
 from openhands.sdk.tool import (
     Action,
+    DeclaredResources,
     Observation,
     ToolAnnotations,
     ToolDefinition,
@@ -132,6 +134,19 @@ def visualize(self) -> Text:
 class EditTool(ToolDefinition[EditAction, EditObservation]):
     """Tool for editing files via find/replace."""
 
+    def declared_resources(self, action: Action) -> DeclaredResources:
+        """Lock on the target file path so concurrent edits to the same
+        file are serialized, while edits to different files run in parallel.
+        """
+        assert isinstance(action, EditAction)
+        path = Path(action.file_path)
+        if not path.is_absolute():
+            assert self.meta is not None, (
+                "workspace_root required to resolve relative paths"
+            )
+            path = Path(self.meta["workspace_root"]) / path
+        return DeclaredResources(keys=(f"file:{path.resolve()}",), declared=True)
+
     @classmethod
     def create(
         cls,
@@ -166,6 +181,7 @@ def create(
                     openWorldHint=False,
                 ),
                 executor=executor,
+                meta={"workspace_root": working_dir},
             )
         ]
 

openhands-tools/openhands/tools/gemini/read_file/definition.py

@@ -1,13 +1,15 @@
 """Read file tool definition (Gemini-style)."""
 
 from collections.abc import Sequence
+from pathlib import Path
 from typing import TYPE_CHECKING
 
 from pydantic import Field
 from rich.text import Text
 
 from openhands.sdk.tool import (
     Action,
+    DeclaredResources,
     Observation,
     ToolAnnotations,
     ToolDefinition,
@@ -107,6 +109,20 @@ def visualize(self) -> Text:
 class ReadFileTool(ToolDefinition[ReadFileAction, ReadFileObservation]):
     """Tool for reading file contents with pagination support."""
 
+    def declared_resources(self, action: Action) -> DeclaredResources:
+        """Lock on the target file path so a read never sees
+        partially-written content from a concurrent write.
+        Reads of different files run in parallel.
+        """
+        assert isinstance(action, ReadFileAction)
+        path = Path(action.file_path)
+        if not path.is_absolute():
+            assert self.meta is not None, (
+                "workspace_root required to resolve relative paths"
+            )
+            path = Path(self.meta["workspace_root"]) / path
+        return DeclaredResources(keys=(f"file:{path.resolve()}",), declared=True)
+
     @classmethod
     def create(
         cls,
@@ -141,6 +157,7 @@ def create(
                     openWorldHint=False,
                 ),
                 executor=executor,
+                meta={"workspace_root": working_dir},
             )
         ]
 

openhands-tools/openhands/tools/gemini/write_file/definition.py

@@ -1,13 +1,15 @@
 """Write file tool definition (Gemini-style)."""
 
 from collections.abc import Sequence
+from pathlib import Path
 from typing import TYPE_CHECKING
 
 from pydantic import Field, PrivateAttr
 from rich.text import Text
 
 from openhands.sdk.tool import (
     Action,
+    DeclaredResources,
     Observation,
     ToolAnnotations,
     ToolDefinition,
@@ -99,6 +101,19 @@ def visualize(self) -> Text:
 class WriteFileTool(ToolDefinition[WriteFileAction, WriteFileObservation]):
     """Tool for writing complete file contents."""
 
+    def declared_resources(self, action: Action) -> DeclaredResources:
+        """Lock on the target file path so concurrent writes to the same
+        file are serialized, while writes to different files run in parallel.
+        """
+        assert isinstance(action, WriteFileAction)
+        path = Path(action.file_path)
+        if not path.is_absolute():
+            assert self.meta is not None, (
+                "workspace_root required to resolve relative paths"
+            )
+            path = Path(self.meta["workspace_root"]) / path
+        return DeclaredResources(keys=(f"file:{path.resolve()}",), declared=True)
+
     @classmethod
     def create(
         cls,
@@ -133,6 +148,7 @@ def create(
                     openWorldHint=False,
                 ),
                 executor=executor,
+                meta={"workspace_root": working_dir},
             )
         ]
 

tests/tools/gemini/conftest.py

@@ -0,0 +1,13 @@
+"""Shared fixtures for Gemini tool tests."""
+
+from unittest.mock import MagicMock
+
+import pytest
+
+
+@pytest.fixture
+def fake_conv_state(tmp_path):
+    """Minimal mock ConversationState with a workspace directory."""
+    cs = MagicMock()
+    cs.workspace.working_dir = str(tmp_path)
+    return cs

tests/tools/gemini/edit/test_edit.py

@@ -1,6 +1,8 @@
 """Tests for edit tool."""
 
-from openhands.tools.gemini.edit.definition import EditAction
+from pathlib import Path
+
+from openhands.tools.gemini.edit.definition import EditAction, EditTool
 from openhands.tools.gemini.edit.impl import EditExecutor
 
 
@@ -148,3 +150,35 @@ def test_edit_multiline_replacement(tmp_path):
     assert not obs.is_error
     assert obs.replacements_made == 1
     assert test_file.read_text() == "def foo():\n    print('new')\n    return 2\n"
+
+
+def test_declared_resources_locks_on_file_path(fake_conv_state):
+    """declared_resources returns a file-path key for per-file locking."""
+    tool = EditTool.create(conv_state=fake_conv_state)[0]
+    action = EditAction(file_path="/a/b.py", old_string="x", new_string="y")
+    resources = tool.declared_resources(action)
+    assert resources.declared is True
+    assert len(resources.keys) == 1
+    assert resources.keys[0] == f"file:{Path('/a/b.py').resolve()}"
+
+
+def test_declared_resources_different_files_different_keys(fake_conv_state):
+    """Different file paths produce different resource keys."""
+    tool = EditTool.create(conv_state=fake_conv_state)[0]
+    a = tool.declared_resources(
+        EditAction(file_path="/a.py", old_string="", new_string="x")
+    )
+    b = tool.declared_resources(
+        EditAction(file_path="/b.py", old_string="", new_string="x")
+    )
+    assert a.keys != b.keys
+
+
+def test_declared_resources_relative_path_resolves_against_workspace(fake_conv_state):
+    """Relative paths must resolve against workspace_root, not process CWD."""
+    tool = EditTool.create(conv_state=fake_conv_state)[0]
+    workspace = fake_conv_state.workspace.working_dir
+    resources = tool.declared_resources(
+        EditAction(file_path="src/foo.py", old_string="", new_string="x")
+    )
+    assert resources.keys[0] == f"file:{(Path(workspace) / 'src' / 'foo.py').resolve()}"

tests/tools/gemini/read_file/test_read_file.py

@@ -1,6 +1,8 @@
 """Tests for read_file tool."""
 
-from openhands.tools.gemini.read_file.definition import ReadFileAction
+from pathlib import Path
+
+from openhands.tools.gemini.read_file.definition import ReadFileAction, ReadFileTool
 from openhands.tools.gemini.read_file.impl import ReadFileExecutor
 
 
@@ -108,3 +110,29 @@ def test_read_file_offset_beyond_length(tmp_path):
 
     assert obs.is_error
     assert "beyond" in obs.text.lower()
+
+
+def test_declared_resources_locks_on_file_path(fake_conv_state):
+    """declared_resources returns a file-path key for per-file locking."""
+    tool = ReadFileTool.create(conv_state=fake_conv_state)[0]
+    action = ReadFileAction(file_path="/a/b.py")
+    resources = tool.declared_resources(action)
+    assert resources.declared is True
+    assert len(resources.keys) == 1
+    assert resources.keys[0] == f"file:{Path('/a/b.py').resolve()}"
+
+
+def test_declared_resources_different_files_different_keys(fake_conv_state):
+    """Different file paths produce different resource keys."""
+    tool = ReadFileTool.create(conv_state=fake_conv_state)[0]
+    a = tool.declared_resources(ReadFileAction(file_path="/a.py"))
+    b = tool.declared_resources(ReadFileAction(file_path="/b.py"))
+    assert a.keys != b.keys
+
+
+def test_declared_resources_relative_path_resolves_against_workspace(fake_conv_state):
+    """Relative paths must resolve against workspace_root, not process CWD."""
+    tool = ReadFileTool.create(conv_state=fake_conv_state)[0]
+    workspace = fake_conv_state.workspace.working_dir
+    resources = tool.declared_resources(ReadFileAction(file_path="src/foo.py"))
+    assert resources.keys[0] == f"file:{(Path(workspace) / 'src' / 'foo.py').resolve()}"

tests/tools/gemini/test_cross_tool_locking.py

@@ -0,0 +1,44 @@
+"""Cross-tool test: Gemini tools and FileEditorTool must produce the same
+resource key for the same file so that the parallel executor serializes
+access correctly across tool boundaries.
+"""
+
+from pathlib import Path
+
+from openhands.tools.file_editor.definition import FileEditorAction, FileEditorTool
+from openhands.tools.gemini.edit.definition import EditAction, EditTool
+from openhands.tools.gemini.read_file.definition import ReadFileAction, ReadFileTool
+from openhands.tools.gemini.write_file.definition import WriteFileAction, WriteFileTool
+
+
+def test_gemini_and_file_editor_produce_same_key(fake_conv_state):
+    """A Gemini relative path and a FileEditorTool absolute path for the same
+    file must yield identical resource keys."""
+    workspace = fake_conv_state.workspace.working_dir
+    abs_path = str(Path(workspace) / "src" / "foo.py")
+
+    # Gemini tools with a relative path
+    edit_tool = EditTool.create(conv_state=fake_conv_state)[0]
+    read_tool = ReadFileTool.create(conv_state=fake_conv_state)[0]
+    write_tool = WriteFileTool.create(conv_state=fake_conv_state)[0]
+
+    gemini_edit_key = edit_tool.declared_resources(
+        EditAction(file_path="src/foo.py", old_string="", new_string="x")
+    ).keys[0]
+    gemini_read_key = read_tool.declared_resources(
+        ReadFileAction(file_path="src/foo.py")
+    ).keys[0]
+    gemini_write_key = write_tool.declared_resources(
+        WriteFileAction(file_path="src/foo.py", content="x")
+    ).keys[0]
+
+    # FileEditorTool with an absolute path
+    file_editor_tool = FileEditorTool.create(conv_state=fake_conv_state)[0]
+    file_editor_key = file_editor_tool.declared_resources(
+        FileEditorAction(command="view", path=abs_path)
+    ).keys[0]
+
+    # All must agree
+    assert gemini_edit_key == file_editor_key
+    assert gemini_read_key == file_editor_key
+    assert gemini_write_key == file_editor_key

tests/tools/gemini/write_file/test_write_file.py

@@ -1,6 +1,8 @@
 """Tests for write_file tool."""
 
-from openhands.tools.gemini.write_file.definition import WriteFileAction
+from pathlib import Path
+
+from openhands.tools.gemini.write_file.definition import WriteFileAction, WriteFileTool
 from openhands.tools.gemini.write_file.impl import WriteFileExecutor
 
 
@@ -91,3 +93,31 @@ def test_write_file_empty_content(tmp_path):
     assert obs.is_new_file
     assert (tmp_path / "empty.txt").exists()
     assert (tmp_path / "empty.txt").read_text() == ""
+
+
+def test_declared_resources_locks_on_file_path(fake_conv_state):
+    """declared_resources returns a file-path key for per-file locking."""
+    tool = WriteFileTool.create(conv_state=fake_conv_state)[0]
+    action = WriteFileAction(file_path="/a/b.py", content="x")
+    resources = tool.declared_resources(action)
+    assert resources.declared is True
+    assert len(resources.keys) == 1
+    assert resources.keys[0] == f"file:{Path('/a/b.py').resolve()}"
+
+
+def test_declared_resources_different_files_different_keys(fake_conv_state):
+    """Different file paths produce different resource keys."""
+    tool = WriteFileTool.create(conv_state=fake_conv_state)[0]
+    a = tool.declared_resources(WriteFileAction(file_path="/a.py", content="x"))
+    b = tool.declared_resources(WriteFileAction(file_path="/b.py", content="x"))
+    assert a.keys != b.keys
+
+
+def test_declared_resources_relative_path_resolves_against_workspace(fake_conv_state):
+    """Relative paths must resolve against workspace_root, not process CWD."""
+    tool = WriteFileTool.create(conv_state=fake_conv_state)[0]
+    workspace = fake_conv_state.workspace.working_dir
+    resources = tool.declared_resources(
+        WriteFileAction(file_path="src/foo.py", content="x")
+    )
+    assert resources.keys[0] == f"file:{(Path(workspace) / 'src' / 'foo.py').resolve()}"