From 2073a15462362d47f1ec8c82f2a513ff5c0f17e3 Mon Sep 17 00:00:00 2001 From: qer Date: Mon, 27 Apr 2026 19:07:53 +0800 Subject: [PATCH] fix(approval): scope pending requests to turn lifecycle --- CHANGELOG.md | 1 + docs/en/release-notes/changelog.md | 1 + docs/zh/release-notes/changelog.md | 1 + src/kimi_cli/app.py | 27 ++- src/kimi_cli/approval_runtime/runtime.py | 4 +- src/kimi_cli/soul/kimisoul.py | 11 +- tests/core/test_approval_runtime.py | 208 ++++++++++++++++------- tests/core/test_notifications.py | 122 ++++++++++++- 8 files changed, 300 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d3bf5ef07..f25f2122ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ Only write entries that are worth mentioning to users. ## Unreleased +- Core: Approval requests no longer auto-timeout after 5 minutes, which previously surfaced as `Rejected by user`; active foreground and subagent approvals now wait indefinitely for user response - Shell: Fix `/usage` remaining quota rendering — the progress bar, warning colors, and `% left` label now all use the remaining quota ratio consistently, so high remaining quota shows as green/full and near-exhausted quota shows as yellow or red - Shell: Show active background agent task count in the prompt status bar — the existing `⚙ bash: N` badge only counted background Shell tasks and filtered out background Agent subagents, so when many subagents were running the prompt looked idle and users could not tell work was in progress; the toolbar now renders `⚙ bash: N` and `⚙ agent: N` as two independent badges (each hidden when its count is 0) and drops the agent badge first when the terminal is too narrow to fit both - Auth: Fix managed model list refresh silently failing for OAuth users with expired tokens — the background `/models` sync now detects 401 responses, forces an OAuth token refresh, and retries with the refreshed token; if the refresh fails or the refreshed token is still rejected, it falls back to the originally configured static API key instead of skipping the provider diff --git a/docs/en/release-notes/changelog.md b/docs/en/release-notes/changelog.md index 4ca9342a49..34b3841dfa 100644 --- a/docs/en/release-notes/changelog.md +++ b/docs/en/release-notes/changelog.md @@ -4,6 +4,7 @@ This page documents the changes in each Kimi Code CLI release. ## Unreleased +- Core: Approval requests no longer auto-timeout after 5 minutes, which previously surfaced as `Rejected by user`; active foreground and subagent approvals now wait indefinitely for user response - Shell: Fix `/usage` remaining quota rendering — the progress bar, warning colors, and `% left` label now all use the remaining quota ratio consistently, so high remaining quota shows as green/full and near-exhausted quota shows as yellow or red - Shell: Show active background agent task count in the prompt status bar — the existing `⚙ bash: N` badge only counted background Shell tasks and filtered out background Agent subagents, so when many subagents were running the prompt looked idle and users could not tell work was in progress; the toolbar now renders `⚙ bash: N` and `⚙ agent: N` as two independent badges (each hidden when its count is 0) and drops the agent badge first when the terminal is too narrow to fit both - Auth: Fix managed model list refresh silently failing for OAuth users with expired tokens — the background `/models` sync now detects 401 responses, forces an OAuth token refresh, and retries with the refreshed token; if the refresh fails or the refreshed token is still rejected, it falls back to the originally configured static API key instead of skipping the provider diff --git a/docs/zh/release-notes/changelog.md b/docs/zh/release-notes/changelog.md index ebf1214a72..13f75c5cb8 100644 --- a/docs/zh/release-notes/changelog.md +++ b/docs/zh/release-notes/changelog.md @@ -4,6 +4,7 @@ ## 未发布 +- Core：修复审批请求 5 分钟自动超时并被误报为 `Rejected by user` 的问题；现在活跃的前台和子 Agent 审批请求都会无限等待用户响应 - Shell：修复 `/usage` 剩余额度渲染错误——进度条、告警颜色和 `% left` 文案现在都统一基于剩余额度比例计算，剩余额度充足时显示为绿色满格，接近耗尽时显示为黄色或红色 - Shell：在提示框状态栏显示当前正在运行的后台 Agent 任务数——原有的 `⚙ bash: N` 徽章只统计后台 Shell 任务，把后台 Agent 子代理过滤掉了，所以多个子代理同时在跑时提示框看起来像空闲，用户无法判断工作是否还在进行；现在状态栏会渲染 `⚙ bash: N` 与 `⚙ agent: N` 两个相互独立的徽章（任一计数为 0 时自动隐藏），终端太窄无法同时容纳两者时优先丢弃 agent 徽章 - Auth：修复 OAuth 用户 access token 过期时托管模型列表刷新静默失败的问题——后台 `/models` 同步任务现在会检测 401 响应，强制进行 OAuth token 刷新并用刷新后的 token 重试；如果刷新本身失败或刷新后的 token 仍被拒绝，则回退到最初配置的静态 API 密钥，而不是跳过该 provider diff --git a/src/kimi_cli/app.py b/src/kimi_cli/app.py index 684d447c67..b2c663517b 100644 --- a/src/kimi_cli/app.py +++ b/src/kimi_cli/app.py @@ -23,7 +23,7 @@ from kimi_cli.llm import augment_provider_with_env_vars, create_llm, model_display_name from kimi_cli.session import Session from kimi_cli.share import get_share_dir -from kimi_cli.soul import run_soul +from kimi_cli.soul import RunCancelled, run_soul from kimi_cli.soul.agent import Runtime, load_agent from kimi_cli.soul.context import Context from kimi_cli.soul.kimisoul import KimiSoul @@ -618,28 +618,49 @@ async def _ui_loop_fn(wire: Wire) -> None: assert self._runtime.root_wire_hub is not None self._runtime.root_wire_hub.unsubscribe(root_hub_queue) + run_cancel_event = asyncio.Event() + + async def _mirror_external_cancel() -> None: + await cancel_event.wait() + run_cancel_event.set() + + external_cancel_task = asyncio.create_task(_mirror_external_cancel()) soul_task = asyncio.create_task( run_soul( self.soul, user_input, _ui_loop_fn, - cancel_event, + run_cancel_event, runtime=self._runtime, ) ) + wire_shut_down = False try: wire_ui = await wire_future while True: msg = await wire_ui.receive() yield msg except QueueShutDown: + wire_shut_down = True pass finally: # stop consuming Wire messages stop_ui_loop.set() + cleanup_cancelled_run = False + if not wire_shut_down and not soul_task.done() and not cancel_event.is_set(): + cleanup_cancelled_run = True + run_cancel_event.set() # wait for the soul task to finish, or raise - await soul_task + try: + await soul_task + except RunCancelled: + if not cleanup_cancelled_run: + raise + finally: + external_cancel_task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await external_cancel_task async def run_shell( self, command: str | None = None, *, prefill_text: str | None = None diff --git a/src/kimi_cli/approval_runtime/runtime.py b/src/kimi_cli/approval_runtime/runtime.py index 3938001ed8..6c778ad73c 100644 --- a/src/kimi_cli/approval_runtime/runtime.py +++ b/src/kimi_cli/approval_runtime/runtime.py @@ -83,7 +83,7 @@ def create_request( return request async def wait_for_response( - self, request_id: str, timeout: float = 300.0 + self, request_id: str, timeout: float | None = None ) -> tuple[ApprovalResponseKind, str]: waiter = self._waiters.get(request_id) request = self._requests.get(request_id) @@ -97,6 +97,8 @@ async def wait_for_response( return request.response, request.feedback waiter = asyncio.get_running_loop().create_future() self._waiters[request_id] = waiter + if timeout is None: + return await asyncio.shield(waiter) try: return await asyncio.wait_for(asyncio.shield(waiter), timeout=timeout) except TimeoutError: diff --git a/src/kimi_cli/soul/kimisoul.py b/src/kimi_cli/soul/kimisoul.py index 8cf0aff53b..9b5f543639 100644 --- a/src/kimi_cli/soul/kimisoul.py +++ b/src/kimi_cli/soul/kimisoul.py @@ -513,12 +513,12 @@ async def run( skip_user_prompt_hook: bool = False, ): approval_source_token = None + created_approval_source: ApprovalSource | None = None turn_started = False turn_finished = False if get_current_approval_source_or_none() is None: - approval_source_token = set_current_approval_source( - ApprovalSource(kind="foreground_turn", id=uuid.uuid4().hex) - ) + created_approval_source = ApprovalSource(kind="foreground_turn", id=uuid.uuid4().hex) + approval_source_token = set_current_approval_source(created_approval_source) try: # Refresh OAuth tokens on each turn to avoid idle-time expirations. await self._runtime.oauth.ensure_fresh(self._runtime) @@ -628,6 +628,11 @@ async def run( finally: if turn_started and not turn_finished: wire_send(TurnEnd()) + if created_approval_source is not None and self._runtime.approval_runtime is not None: + self._runtime.approval_runtime.cancel_by_source( + created_approval_source.kind, + created_approval_source.id, + ) if approval_source_token is not None: reset_current_approval_source(approval_source_token) diff --git a/tests/core/test_approval_runtime.py b/tests/core/test_approval_runtime.py index e71d321944..910616afab 100644 --- a/tests/core/test_approval_runtime.py +++ b/tests/core/test_approval_runtime.py @@ -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" + + @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] diff --git a/tests/core/test_notifications.py b/tests/core/test_notifications.py index 136d618083..78da4ce6d6 100644 --- a/tests/core/test_notifications.py +++ b/tests/core/test_notifications.py @@ -12,11 +12,11 @@ from kosong.tooling.empty import EmptyToolset from kimi_cli.app import KimiCLI -from kimi_cli.approval_runtime import ApprovalSource +from kimi_cli.approval_runtime import ApprovalSource, get_current_approval_source_or_none from kimi_cli.background import TaskRuntime, TaskSpec from kimi_cli.llm import LLM from kimi_cli.notifications import NotificationEvent -from kimi_cli.soul import StatusSnapshot, _current_wire, run_soul +from kimi_cli.soul import RunCancelled, StatusSnapshot, _current_wire, run_soul from kimi_cli.soul.agent import Agent, Runtime from kimi_cli.soul.context import Context from kimi_cli.soul.kimisoul import KimiSoul @@ -323,6 +323,124 @@ async def _collect() -> None: assert [response.request_id for response in seen_responses] == ["req-run-bridge-1"] +@pytest.mark.asyncio +async def test_kimi_cli_run_cancels_abandoned_approval_stream( + runtime: Runtime, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + assert runtime.approval_runtime is not None + + async def fake_turn(self, _user_message): + source = get_current_approval_source_or_none() + assert source is not None + request = runtime.approval_runtime.create_request( + request_id="req-run-abandoned-approval", + tool_call_id="call-run-abandoned-approval", + sender="WriteFile", + action="edit file", + description="write file", + display=[], + source=source, + ) + await runtime.approval_runtime.wait_for_response(request.id) + + async def fake_ensure_fresh(_runtime): + return None + + monkeypatch.setattr(KimiSoul, "_turn", fake_turn) + monkeypatch.setattr(runtime.oauth, "ensure_fresh", fake_ensure_fresh) + + soul = KimiSoul( + Agent( + name="Approval Stream Agent", + system_prompt="System prompt.", + toolset=EmptyToolset(), + runtime=runtime, + ), + context=Context(file_backend=tmp_path / "history.jsonl"), + ) + cli = KimiCLI(soul, runtime, {}) + cancel_event = asyncio.Event() + stream = cli.run("ping", cancel_event) + + request: ApprovalRequest | None = None + for _ in range(10): + msg = await asyncio.wait_for(anext(stream), timeout=1.0) + if isinstance(msg, ApprovalRequest): + request = msg + break + assert request is not None + + await asyncio.wait_for(stream.aclose(), timeout=1.0) + + record = runtime.approval_runtime.get_request("req-run-abandoned-approval") + assert record is not None + assert record.status == "cancelled" + assert record.response == "reject" + assert runtime.approval_runtime.list_pending() == [] + assert not cancel_event.is_set() + + +@pytest.mark.asyncio +async def test_kimi_cli_run_propagates_external_cancel_event( + runtime: Runtime, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + assert runtime.approval_runtime is not None + + async def fake_turn(self, _user_message): + source = get_current_approval_source_or_none() + assert source is not None + request = runtime.approval_runtime.create_request( + request_id="req-run-external-cancel", + tool_call_id="call-run-external-cancel", + sender="WriteFile", + action="edit file", + description="write file", + display=[], + source=source, + ) + await runtime.approval_runtime.wait_for_response(request.id) + + async def fake_ensure_fresh(_runtime): + return None + + monkeypatch.setattr(KimiSoul, "_turn", fake_turn) + monkeypatch.setattr(runtime.oauth, "ensure_fresh", fake_ensure_fresh) + + soul = KimiSoul( + Agent( + name="Approval Stream Agent", + system_prompt="System prompt.", + toolset=EmptyToolset(), + runtime=runtime, + ), + context=Context(file_backend=tmp_path / "history.jsonl"), + ) + cli = KimiCLI(soul, runtime, {}) + cancel_event = asyncio.Event() + stream = cli.run("ping", cancel_event) + + request: ApprovalRequest | None = None + for _ in range(10): + msg = await asyncio.wait_for(anext(stream), timeout=1.0) + if isinstance(msg, ApprovalRequest): + request = msg + break + assert request is not None + + cancel_event.set() + with pytest.raises(RunCancelled): + while True: + await asyncio.wait_for(anext(stream), timeout=1.0) + + record = runtime.approval_runtime.get_request("req-run-external-cancel") + assert record is not None + assert record.status == "cancelled" + + @pytest.mark.asyncio async def test_kimi_cli_run_replays_pending_approvals_from_previous_turn(runtime: Runtime) -> None: assert runtime.approval_runtime is not None