diff --git a/CHANGELOG.md b/CHANGELOG.md index c25dfc0..23324e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ The format is based on Keep a Changelog, and this project currently tracks chang - Memory search matches against body content in addition to metadata, with metadata weighted higher for relevance. - Memory search tokenizer handles Han characters for multilingual queries. - Fixed duplicate response in React TUI caused by double Enter key submission in the input handler. +- Fixed concurrent permission modals overwriting each other in TUI default mode when the LLM returns multiple tool calls in one response; `_ask_permission` now serialises callers via an `asyncio.Lock` so each modal is shown and resolved before the next one is emitted. ### Changed diff --git a/src/openharness/ui/backend_host.py b/src/openharness/ui/backend_host.py index fba5a3d..f9bac2a 100644 --- a/src/openharness/ui/backend_host.py +++ b/src/openharness/ui/backend_host.py @@ -65,6 +65,7 @@ class ReactBackendHost: self._request_queue: asyncio.Queue[FrontendRequest] = asyncio.Queue() self._permission_requests: dict[str, asyncio.Future[bool]] = {} self._question_requests: dict[str, asyncio.Future[str]] = {} + self._permission_lock = asyncio.Lock() self._busy = False self._running = True # Track last tool input per name for rich event emission @@ -647,27 +648,28 @@ class ReactBackendHost: return options async def _ask_permission(self, tool_name: str, reason: str) -> bool: - request_id = uuid4().hex - future: asyncio.Future[bool] = asyncio.get_running_loop().create_future() - self._permission_requests[request_id] = future - await self._emit( - BackendEvent( - type="modal_request", - modal={ - "kind": "permission", - "request_id": request_id, - "tool_name": tool_name, - "reason": reason, - }, + async with self._permission_lock: + request_id = uuid4().hex + future: asyncio.Future[bool] = asyncio.get_running_loop().create_future() + self._permission_requests[request_id] = future + await self._emit( + BackendEvent( + type="modal_request", + modal={ + "kind": "permission", + "request_id": request_id, + "tool_name": tool_name, + "reason": reason, + }, + ) ) - ) - try: - return await asyncio.wait_for(future, timeout=300) - except asyncio.TimeoutError: - log.warning("Permission request %s timed out after 300s, denying", request_id) - return False - finally: - self._permission_requests.pop(request_id, None) + try: + return await asyncio.wait_for(future, timeout=300) + except asyncio.TimeoutError: + log.warning("Permission request %s timed out after 300s, denying", request_id) + return False + finally: + self._permission_requests.pop(request_id, None) async def _ask_question(self, question: str) -> str: request_id = uuid4().hex diff --git a/tests/test_ui/test_react_backend.py b/tests/test_ui/test_react_backend.py index 964c1df..908b06a 100644 --- a/tests/test_ui/test_react_backend.py +++ b/tests/test_ui/test_react_backend.py @@ -406,3 +406,51 @@ async def test_backend_host_apply_provider_select_command_shows_single_segment_t assert should_continue is True user_event = next(item for item in events if item.type == "transcript_item" and item.item and item.item.role == "user") assert user_event.item.text == "/provider" + + +@pytest.mark.asyncio +async def test_concurrent_ask_permission_are_serialised(): + """Concurrent _ask_permission calls must be serialised so the frontend + never receives two overlapping modal_request events. + + Without _permission_lock the second call emits a modal_request before the + first future is resolved, overwriting the frontend's modal state. The first + tool then silently waits 300 s and gets Permission denied. + """ + host = ReactBackendHost(BackendHostConfig(api_client=StaticApiClient("unused"))) + + emitted_order: list[str] = [] + + async def _fake_emit(event: BackendEvent) -> None: + if event.type == "modal_request" and event.modal: + emitted_order.append(str(event.modal.get("request_id", ""))) + + host._emit = _fake_emit # type: ignore[method-assign] + + async def _ask_and_approve(tool: str) -> bool: + # Start the ask; a background task resolves the future once it appears. + async def _resolver(): + # Busy-wait until this tool's future is registered. + while True: + await asyncio.sleep(0) + for rid, fut in list(host._permission_requests.items()): + if not fut.done(): + fut.set_result(True) + return + + asyncio.create_task(_resolver()) + return await host._ask_permission(tool, "reason") + + # Fire two permission requests concurrently. + result_a, result_b = await asyncio.gather( + _ask_and_approve("write_file"), + _ask_and_approve("bash"), + ) + + assert result_a is True + assert result_b is True + # With the lock in place the two modal_request events must be emitted + # sequentially (one completes before the other starts), so exactly two + # distinct request IDs must have been emitted. + assert len(emitted_order) == 2 + assert emitted_order[0] != emitted_order[1]