fix(approval): scope pending requests to turn lifecycle - #2086
Conversation
|
Superseded by #2087 using the requested branch naming convention. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts approval lifecycle handling so that approvals are no longer auto-rejected by a default timeout, and instead are cleaned up via explicit source-scoped cancellation when the owning run/stream ends.
Changes:
- Make
ApprovalRuntime.wait_for_response()wait indefinitely by default (timeout=None), while keeping explicit timeout support. - Cancel foreground-turn approval requests via
cancel_by_sourcewhen aKimiSoul.run()-created approval source exits. - In
KimiCLI.run(), introduce an internal cancel event to cleanly cancel abandoned runs/streams without mutating the caller’scancel_event, and propagate external cancellations correctly. - Add/adjust tests and update changelogs to reflect the new behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/core/test_notifications.py | Adds coverage for abandoning KimiCLI.run() streams and for external cancellation propagation. |
| tests/core/test_approval_runtime.py | Adds tests for indefinite default waiting, explicit timeout, and run-scoped cancellation behavior. |
| src/kimi_cli/soul/kimisoul.py | Tracks whether a foreground approval source was created and cancels by source in finally. |
| src/kimi_cli/approval_runtime/runtime.py | Changes default approval wait timeout from 300s to indefinite (None). |
| src/kimi_cli/app.py | Uses an internal run cancel event to cancel abandoned runs without mutating the external cancel event; selectively swallows internal RunCancelled. |
| docs/zh/release-notes/changelog.md | Documents removal of the 5-minute auto-timeout behavior in Core approvals. |
| docs/en/release-notes/changelog.md | Same as above (English release notes). |
| CHANGELOG.md | Same as above (root changelog). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) |
There was a problem hiding this comment.
wait_for_response() now awaits the underlying waiter indefinitely when timeout is None. If the calling task is cancelled (e.g., run cancellation), asyncio.shield(waiter) will raise CancelledError to 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 calls future.exception()), or otherwise avoid setting un-retrieved exceptions on orphaned waiters during cancellation cleanup.
| @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" | ||
|
|
There was a problem hiding this comment.
The explicit-timeout behavior is still tested at ApprovalRuntime.wait_for_response(), but this no longer verifies that the timeout feedback (e.g. "approval timed out") is propagated through the higher-level Approval.request()/ApprovalResult.rejection_error() surface. Since the PR description calls out keeping explicit timeout behavior covered for future policy/config use, consider reintroducing an integration-style test that exercises Approval.request() and asserts the resulting rejection includes the timeout feedback (to prevent regressions back to the generic Rejected by user).
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Summary
Context
This separates lifecycle correctness from timeout policy. The old 300s fallback prevented some hangs, but it also caused active approvals to be rejected as if the user rejected them. This PR uses source-scoped lifecycle cleanup as the primary completion path instead.
Tests