comments from Calvin

Author: VascoSch92

openhands-tools/openhands/tools/terminal/definition.py

@@ -236,15 +236,13 @@ class TerminalTool(ToolDefinition[TerminalAction, TerminalObservation]):
     """A ToolDefinition subclass that automatically initializes a TerminalExecutor with auto-detection."""  # noqa: E501
 
     def declared_resources(self, action: Action) -> DeclaredResources:  # noqa: ARG002
-        from openhands.tools.terminal.impl import TerminalExecutor
-
         # When using the tmux backend, TmuxPanePool handles concurrency
         # internally via pane-level isolation — opt out of framework
         # serialization so parallel calls are allowed.
         # When using the subprocess backend there is only a single
         # session, so we declare a resource key to serialize terminal
         # calls against each other without blocking unrelated tools.
-        if isinstance(self.executor, TerminalExecutor) and self.executor.is_pooled:
+        if getattr(self.executor, "is_pooled", False):
             return DeclaredResources(keys=(), declared=True)
         return DeclaredResources(keys=("terminal:session",), declared=True)
 

openhands-tools/openhands/tools/terminal/impl.py

@@ -361,16 +361,15 @@ def _execute_pooled(
         """Execute *action* in pool mode with proper checkout/checkin.
 
         All pane lifecycle (checkout, optional replace, checkin) is
-        contained here so there is exactly one checkout and one checkin
-        per call, and the checkin is guaranteed by ``finally``.
+        managed by the pool's context manager so there is exactly one
+        checkout and one checkin per call.
         """
-        terminal = self._pool.checkout()  # type: ignore[union-attr]
-        try:
+        with self._pool.pane() as handle:  # type: ignore[union-attr]
             reset_text: str | None = None
 
-            if action.reset or terminal._closed:
-                self._discard_session(terminal)
-                terminal = self._pool.replace(terminal)  # type: ignore[union-attr]
+            if action.reset or handle.terminal._closed:
+                self._discard_session(handle.terminal)
+                handle.terminal = self._pool.replace(handle.terminal)  # type: ignore[union-attr]
                 reset_text = self._RESET_TEXT
                 logger.info(
                     f"Terminal pane replaced (reset) working_dir: {self._working_dir}"
@@ -383,7 +382,7 @@ def _execute_pooled(
                         exit_code=0,
                     )
 
-            session = self._wrap_session(terminal)
+            session = self._wrap_session(handle.terminal)
             self._prepare_pooled_session(session)
 
             cmd_action = (
@@ -409,8 +408,6 @@ def _execute_pooled(
                 )
 
             return self._mask_observation(observation, conversation)
-        finally:
-            self._pool.checkin(terminal)  # type: ignore[union-attr]
 
     def __call__(
         self,

openhands-tools/openhands/tools/terminal/terminal/tmux_pane_pool.py

@@ -10,7 +10,8 @@
 import time
 import uuid
 from collections import deque
-from contextlib import suppress
+from collections.abc import Iterator
+from contextlib import contextmanager, suppress
 from dataclasses import dataclass, field
 from typing import Final
 
@@ -48,6 +49,13 @@ def close(self) -> None:
             self._closed = True
 
 
+@dataclass(slots=True)
+class PaneHandle:
+    """Mutable handle to a checked-out pane, for use as a context manager target."""
+
+    terminal: PooledTmuxTerminal
+
+
 @dataclass(slots=True)
 class TmuxPanePool:
     """Thread-safe pool of tmux panes for parallel terminal execution.
@@ -272,3 +280,17 @@ def replace(self, old_terminal: PooledTmuxTerminal) -> PooledTmuxTerminal:
 
         logger.debug(f"Replaced pane {old_pane_id} -> {new_pane_id}")
         return new_terminal
+
+    @contextmanager
+    def pane(self, timeout: float | None = None) -> Iterator[PaneHandle]:
+        """Context manager: checkout a pane, yield a handle, checkin on exit.
+
+        The yielded :class:`PaneHandle` is mutable — callers that call
+        :meth:`replace` should assign the new terminal back to
+        ``handle.terminal`` so that the correct pane is checked in.
+        """
+        handle = PaneHandle(self.checkout(timeout=timeout))
+        try:
+            yield handle
+        finally:
+            self.checkin(handle.terminal)