fix(approval): scope pending requests to turn lifecycle#2087
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts approval-request lifecycle handling so approvals don’t get auto-rejected after a fixed timeout, and instead are cancelled deterministically when their owning run/stream ends.
Changes:
- Make
ApprovalRuntime.wait_for_response()wait indefinitely by default (timeout now opt-in). - Cancel foreground-turn approvals via
cancel_by_sourcewhen aKimiSoulturn finishes/aborts, and cancel abandonedKimiCLI.run()streams using an internal cancel event. - Add/adjust tests to cover indefinite waiting, explicit timeout behavior, abandoned-stream cleanup, and external cancellation propagation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/kimi_cli/approval_runtime/runtime.py |
Changes approval wait default from finite timeout to indefinite (opt-in timeout remains). |
src/kimi_cli/soul/kimisoul.py |
Introduces per-turn foreground ApprovalSource tracking and cancels its pending approvals on exit. |
src/kimi_cli/app.py |
Adds internal run-cancel event to cleanly cancel abandoned KimiCLI.run() streams without mutating caller cancel state; propagates external cancellation. |
tests/core/test_approval_runtime.py |
Adds coverage for “indefinite by default”, explicit timeout path, and run-cancel lifecycle scoping. |
tests/core/test_notifications.py |
Adds coverage that abandoning a stream cancels pending approvals; verifies external cancel still raises RunCancelled. |
CHANGELOG.md |
Documents the user-visible change to approval timeout behavior. |
docs/en/release-notes/changelog.md |
Syncs release notes entry for the approval timeout behavior change. |
docs/zh/release-notes/changelog.md |
Adds corresponding Chinese release note entry for the approval timeout behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…eanup # Conflicts: # CHANGELOG.md # docs/en/release-notes/changelog.md # docs/zh/release-notes/changelog.md
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 520223d473
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <<<<<<< HEAD | ||
| - 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 | ||
| ======= | ||
| - Core: Fix yolo mode reminder being lost after context compaction — the non-interactive-mode guidance ("don't call AskUserQuestion, plan-mode toggles are auto-approved") is now re-injected on the first LLM step after each compaction while yolo remains active, instead of being silently dropped when the original reminder is folded into the compaction summary | ||
| >>>>>>> origin/main |
There was a problem hiding this comment.
The Unreleased section still contains Git conflict markers (<<<<<<<, =======, >>>>>>>) instead of a resolved entry, so this commit publishes raw merge syntax and two competing bullets verbatim. That is user-visible in release notes and can break tooling that parses CHANGELOG.md as clean markdown; the same unresolved block is also mirrored into docs/en/release-notes/changelog.md and docs/zh/release-notes/changelog.md.
Useful? React with 👍 / 👎.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aa249d7cd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| except asyncio.CancelledError: | ||
| if self._waiters.get(request_id) is waiter: | ||
| self._waiters.pop(request_id, None) |
There was a problem hiding this comment.
ApprovalRuntime.wait_for_response reuses a single future per request ID, but this new CancelledError handler removes that shared future from _waiters whenever any waiting task is cancelled. If two coroutines are waiting on the same request and one is cancelled (for example, one caller aborts while another still awaits), resolve()/cancel_by_source() can no longer find the waiter to complete, leaving the remaining waiter hung indefinitely despite the request being resolved.
Useful? React with 👍 / 👎.
The outer ``assert runtime.approval_runtime is not None`` does not propagate into nested ``async def`` bodies, so pyright flagged the ``runtime.approval_runtime.create_request(...)`` / ``get_request(...)`` calls inside the new abandoned-stream / external-cancel / ACP-cancel tests. Re-assert at the top of each closure so the type narrows there.
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Summary
approvals are not auto-rejected after 5 minutes.
ApprovalSource/cancel_by_source lifecycle when the owning KimiSoul run
exits.
without mutating the caller-owned cancel_event.
Closes #1823. Supersedes #1837 — thanks @bloodycoder for the prototype
and problem write-up that pushed us to ask what the 300s timer was
actually defending against.
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.
The
cancel_by_sourceprimitive used here has existed since #1552 butwas never wired into the foreground-turn lifecycle; PR #1724 later added
a 300s
wait_for_responsetimeout as a defense against orphanedapproval futures (the parallel-subagent sink-switching bug it was
actually fixing). This PR completes the wiring and removes the
now-redundant timer.
Behavior change
Rejected: approval timed outsince 1.38,Rejected by useron ≤1.37)cancel_by_sourceinKimiSoul.runfinallyKimiCLI.runabandonment detection_waiterswait_for_response'sexcept CancelledErrorwait_for_response(timeout=N)background_agentapprovalsagent_runnerfinally onlyTests
tests/core/test_approval_runtime.py— default-no-timeout (parametrizedfor both source kinds), explicit-timeout still works, turn-cancel
cleanup asserts foreground-only (not background) cancellation
tests/core/test_notifications.py—KimiCLI.runabandonment cleanup,external cancel propagation
tests/acp/test_session_notifications.py— ACP transport pathindependent regression
Run:
uv run pytest tests/core/test_approval_runtime.py tests/core/test_notifications.py tests/acp/test_session_notifications.pymake check-kimi-cli