-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(approval): scope pending requests to turn lifecycle #2086
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import contextlib | ||
|
|
||
| import pytest | ||
| from kosong.tooling.empty import EmptyToolset | ||
|
|
@@ -13,7 +14,7 @@ | |
| reset_current_approval_source, | ||
| set_current_approval_source, | ||
| ) | ||
| from kimi_cli.soul import run_soul | ||
| from kimi_cli.soul import RunCancelled, run_soul | ||
| from kimi_cli.soul.agent import Agent as SoulAgent | ||
| from kimi_cli.soul.context import Context | ||
| from kimi_cli.soul.kimisoul import KimiSoul | ||
|
|
@@ -46,6 +47,87 @@ async def test_approval_runtime_create_wait_and_resolve() -> None: | |
| assert runtime.list_pending() == [] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize( | ||
| "source", | ||
| [ | ||
| ApprovalSource(kind="foreground_turn", id="turn-no-timeout"), | ||
| ApprovalSource( | ||
| kind="background_agent", | ||
| id="task-no-timeout", | ||
| agent_id="a1234567", | ||
| subagent_type="coder", | ||
| ), | ||
| ], | ||
| ) | ||
| async def test_approval_runtime_wait_for_response_waits_indefinitely_by_default( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| source: ApprovalSource, | ||
| ) -> None: | ||
| """Approval requests must wait until the user responds unless explicitly cancelled.""" | ||
| runtime = ApprovalRuntime() | ||
| request = runtime.create_request( | ||
| request_id=f"req-no-timeout-{source.kind}", | ||
| tool_call_id=f"call-no-timeout-{source.kind}", | ||
| sender="WriteFile", | ||
| action="edit file", | ||
| description="Write file /tmp/test.txt", | ||
| display=[], | ||
| source=source, | ||
| ) | ||
|
|
||
| async def fail_on_finite_timeout(awaitable, timeout=None): | ||
| if timeout is not None: | ||
| raise TimeoutError | ||
| return await awaitable | ||
|
|
||
| monkeypatch.setattr(asyncio, "wait_for", fail_on_finite_timeout) | ||
|
|
||
| waiter = asyncio.create_task(runtime.wait_for_response(request.id)) | ||
| try: | ||
| await asyncio.sleep(0) | ||
| if waiter.done(): | ||
| with pytest.raises(ApprovalCancelledError): | ||
| await waiter | ||
| pytest.fail("wait_for_response used a finite default timeout") | ||
|
|
||
| record = runtime.get_request(request.id) | ||
| assert record is not None | ||
| assert record.status == "pending" | ||
|
|
||
| assert runtime.resolve(request.id, "approve") is True | ||
| response, feedback = await waiter | ||
| assert response == "approve" | ||
| assert feedback == "" | ||
| finally: | ||
| if not waiter.done(): | ||
| waiter.cancel() | ||
| with contextlib.suppress(asyncio.CancelledError): | ||
| await waiter | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_approval_runtime_wait_for_response_explicit_timeout() -> None: | ||
| runtime = ApprovalRuntime() | ||
| request = runtime.create_request( | ||
| request_id="req-timeout", | ||
| tool_call_id="call-timeout", | ||
| sender="WriteFile", | ||
| action="edit file", | ||
| description="Write file /tmp/test.txt", | ||
| display=[], | ||
| source=ApprovalSource(kind="foreground_turn", id="turn-timeout"), | ||
| ) | ||
|
|
||
| with pytest.raises(ApprovalCancelledError): | ||
| await runtime.wait_for_response(request.id, timeout=0.05) | ||
|
|
||
| record = runtime.get_request(request.id) | ||
| assert record is not None | ||
| assert record.status == "cancelled" | ||
| assert record.feedback == "approval timed out" | ||
|
|
||
|
Comment on lines
+109
to
+129
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_approval_runtime_cancel_by_source() -> None: | ||
| runtime = ApprovalRuntime() | ||
|
|
@@ -205,75 +287,69 @@ async def fake_ensure_fresh(_runtime): | |
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_approval_runtime_wait_for_response_times_out() -> None: | ||
| """wait_for_response should raise ApprovalCancelledError after timeout | ||
| instead of hanging forever when no resolve happens. | ||
| async def test_kimisoul_run_cancels_own_foreground_approvals_on_cancel( | ||
| runtime, tmp_path, monkeypatch | ||
| ) -> None: | ||
| assert runtime.approval_runtime is not None | ||
| request_created = asyncio.Event() | ||
|
|
||
| Regression test for: subagent approval requests that are never resolved | ||
| cause the entire session to hang permanently. | ||
| """ | ||
| runtime = ApprovalRuntime() | ||
| request = runtime.create_request( | ||
| request_id="req-timeout", | ||
| tool_call_id="call-timeout", | ||
| sender="WriteFile", | ||
| action="edit file", | ||
| description="Write file /tmp/test.txt", | ||
| display=[], | ||
| source=ApprovalSource(kind="foreground_turn", id="turn-timeout"), | ||
| ) | ||
| async def fake_turn(self, user_message): | ||
| source = get_current_approval_source_or_none() | ||
| assert source is not None | ||
| assert source.kind == "foreground_turn" | ||
| foreground_request = runtime.approval_runtime.create_request( | ||
| request_id="req-foreground-cancelled", | ||
| tool_call_id="call-foreground-cancelled", | ||
| sender="WriteFile", | ||
| action="edit file", | ||
| description="write foreground file", | ||
| display=[], | ||
| source=source, | ||
| ) | ||
| runtime.approval_runtime.create_request( | ||
| request_id="req-background-still-pending", | ||
| tool_call_id="call-background-still-pending", | ||
| sender="WriteFile", | ||
| action="edit file", | ||
| description="write background file", | ||
| display=[], | ||
| source=ApprovalSource(kind="background_agent", id="task-still-running"), | ||
| ) | ||
| request_created.set() | ||
| await runtime.approval_runtime.wait_for_response(foreground_request.id) | ||
|
|
||
| # Use a very short timeout to avoid slow tests | ||
| with pytest.raises(ApprovalCancelledError): | ||
| await runtime.wait_for_response(request.id, timeout=0.05) | ||
| async def fake_ensure_fresh(_runtime): | ||
| return None | ||
|
|
||
| # After timeout, the request should be cancelled and cleaned up | ||
| record = runtime.get_request(request.id) | ||
| assert record is not None | ||
| assert record.status == "cancelled" | ||
| assert record.feedback == "approval timed out" | ||
| monkeypatch.setattr(KimiSoul, "_turn", fake_turn) | ||
| monkeypatch.setattr(runtime.oauth, "ensure_fresh", fake_ensure_fresh) | ||
|
|
||
| soul = KimiSoul( | ||
| SoulAgent( | ||
| name="test", | ||
| system_prompt="test prompt", | ||
| toolset=EmptyToolset(), | ||
| runtime=runtime, | ||
| ), | ||
| context=Context(file_backend=tmp_path / "history.jsonl"), | ||
| ) | ||
|
|
||
| cancel_event = asyncio.Event() | ||
| run_task = asyncio.create_task( | ||
| run_soul(soul, "ping", _drain_ui_messages, cancel_event, runtime=runtime) | ||
| ) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_approval_request_timeout_carries_feedback_to_result() -> None: | ||
| """Timeout feedback must survive round-trip through ``Approval.request``. | ||
|
|
||
| Regression test: when the 300s ``wait_for_response`` safety timeout fires | ||
| (e.g. the user stepped away from their session), ``_cancel_request`` sets | ||
| ``record.feedback = "approval timed out"`` before raising | ||
| ``ApprovalCancelledError``. ``Approval.request`` must read that feedback | ||
| back into the returned ``ApprovalResult`` — otherwise the resulting | ||
| ``ToolRejectedError`` falls back to the generic "Rejected by user" brief, | ||
| hiding the timeout cause from the user. | ||
| """ | ||
| from kimi_cli.soul.approval import Approval, ApprovalState | ||
| from kimi_cli.soul.toolset import current_tool_call | ||
| from kimi_cli.wire.types import ToolCall | ||
| await asyncio.wait_for(request_created.wait(), timeout=1.0) | ||
| cancel_event.set() | ||
| with pytest.raises(RunCancelled): | ||
| await asyncio.wait_for(run_task, timeout=1.0) | ||
|
|
||
| runtime = ApprovalRuntime() | ||
| approval = Approval(state=ApprovalState(), runtime=runtime) | ||
| foreground = runtime.approval_runtime.get_request("req-foreground-cancelled") | ||
| assert foreground is not None | ||
| assert foreground.status == "cancelled" | ||
| assert foreground.response == "reject" | ||
|
|
||
| token = current_tool_call.set( | ||
| ToolCall(id="test", function=ToolCall.FunctionBody(name="Shell", arguments=None)) | ||
| ) | ||
| try: | ||
| request_task = asyncio.create_task( | ||
| approval.request(sender="Shell", action="shell_exec", description="ls") | ||
| ) | ||
| while not runtime.list_pending(): | ||
| await asyncio.sleep(0) | ||
| pending = runtime.list_pending()[0] | ||
| # Drive the timeout path directly instead of waiting 300s: this is | ||
| # the same internal call ``wait_for_response`` makes when its own | ||
| # timeout expires (runtime.py uses ``feedback="approval timed out"``). | ||
| runtime._cancel_request(pending.id, feedback="approval timed out") | ||
| result = await request_task | ||
| finally: | ||
| current_tool_call.reset(token) | ||
|
|
||
| assert result.approved is False | ||
| assert result.feedback == "approval timed out" | ||
| # The user-visible rejection surface reflects the real reason rather | ||
| # than the generic "Rejected by user" fallback. | ||
| err = result.rejection_error() | ||
| assert err.brief == "Rejected: approval timed out" | ||
| background = runtime.approval_runtime.get_request("req-background-still-pending") | ||
| assert background is not None | ||
| assert background.status == "pending" | ||
| assert runtime.approval_runtime.list_pending() == [background] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait_for_response()now awaits the underlying waiter indefinitely whentimeout is None. If the calling task is cancelled (e.g., run cancellation),asyncio.shield(waiter)will raiseCancelledErrorto the caller while leaving the stored waiter future pending; later lifecycle cleanup (e.g.,cancel_by_source) sets an exception on that future, which can trigger noisy "Future exception was never retrieved" warnings because nothing is awaiting it anymore. Consider ensuring waiter exceptions are always consumed (e.g., add a done-callback that callsfuture.exception()), or otherwise avoid setting un-retrieved exceptions on orphaned waiters during cancellation cleanup.